diff --git a/config/scripts/macos-computer-helper-owner-loss-processes.mjs b/config/scripts/macos-computer-helper-owner-loss-processes.mjs index b39b035edc2..e30b5215095 100644 --- a/config/scripts/macos-computer-helper-owner-loss-processes.mjs +++ b/config/scripts/macos-computer-helper-owner-loss-processes.mjs @@ -5,19 +5,97 @@ const PROCESS_EXIT_TIMEOUT_MS = 2_000 const PROCESS_POLL_MS = 25 const sleepBuffer = new Int32Array(new SharedArrayBuffer(4)) -function processIdentity(pid) { - try { - const output = execFileSync('ps', ['-p', String(pid), '-o', 'pid=,pgid=,command='], { - encoding: 'utf8' - }).trim() - const match = output.match(/^(\d+)\s+(\d+)\s+(.+)$/) - if (!match) { - return null - } - return { pid: Number(match[1]), pgid: Number(match[2]), command: match[3] } - } catch { +const processIdentityOperations = { + executePs: execFileSync, + signalProcess: process.kill.bind(process) +} + +export function processIdentity(pid, operations = processIdentityOperations) { + if (!Number.isInteger(pid) || pid <= 0) { return null } + try { + const output = operations + .executePs('ps', ['-p', String(pid), '-o', 'pid=,pgid=,command='], { + encoding: 'utf8' + }) + .trim() + const match = output.match(/^(\d+)\s+(\d+)\s+(.+)$/) + if (!match) { + throw new Error(`Could not parse process identity for ${pid}`) + } + return { pid: Number(match[1]), pgid: Number(match[2]), command: match[3] } + } catch (error) { + try { + operations.signalProcess(pid, 0) + } catch (lookupError) { + if (lookupError?.code === 'ESRCH') { + return null + } + } + throw error + } +} + +function matchingDetachedProcesses(identities, expectedCommandFragments) { + return identities.filter( + (identity) => + identity.pgid === identity.pid && + expectedCommandFragments.every((fragment) => identity.command.includes(fragment)) + ) +} + +const matchingProcessOperations = { + processIdentities, + signalProcessIdentity, + waitForIdentityExit +} + +export function killProcessMatchingCommand( + expectedCommandFragments, + operations = matchingProcessOperations +) { + const matches = matchingDetachedProcesses( + operations.processIdentities(), + expectedCommandFragments + ) + if (matches.length === 0) { + return false + } + const errors = [] + for (const match of matches) { + try { + if (operations.signalProcessIdentity(match, expectedCommandFragments[0], 'SIGKILL')) { + operations.waitForIdentityExit(match) + } + } catch (error) { + errors.push(error) + } + } + try { + const remaining = matchingDetachedProcesses( + operations.processIdentities(), + expectedCommandFragments + ) + if (remaining.length > 0) { + errors.push( + new Error( + `Benchmark helper cleanup left matching processes: ${remaining + .map((identity) => identity.pid) + .join(', ')}` + ) + ) + } + } catch (error) { + errors.push(error) + } + if (errors.length === 1) { + throw errors[0] + } + if (errors.length > 1) { + throw new AggregateError(errors, 'Benchmark exact-command cleanup failed') + } + return true } function sleepSync(milliseconds) { @@ -78,11 +156,21 @@ export function spawnBenchmarkProcess(executable, args, options) { }) } -export function signalValidatedProcessGroup(pgid, environmentFragment, signal) { +const processGroupSignalOperations = { + processIdentities, + signalProcess: process.kill.bind(process) +} + +export function signalValidatedProcessGroup( + pgid, + environmentFragment, + signal, + operations = processGroupSignalOperations +) { if (!Number.isInteger(pgid) || pgid <= 0) { return false } - const members = processIdentities(true).filter((identity) => identity.pgid === pgid) + const members = operations.processIdentities(true).filter((identity) => identity.pgid === pgid) if (members.length === 0) { return false } @@ -90,31 +178,67 @@ export function signalValidatedProcessGroup(pgid, environmentFragment, signal) { throw new Error('Benchmark process group no longer belongs to this trial') } const anchor = members[0] + let stopped = false try { - process.kill(anchor.pid, 'SIGSTOP') - const stoppedAnchor = processIdentities(true).find((identity) => identity.pid === anchor.pid) + operations.signalProcess(anchor.pid, 'SIGSTOP') + stopped = true + const stoppedAnchor = operations + .processIdentities(true) + .find((identity) => identity.pid === anchor.pid) if (!sameIdentity(stoppedAnchor, anchor)) { - process.kill(anchor.pid, 'SIGCONT') + stopped = false throw new Error('Benchmark process group anchor changed before signaling') } - process.kill(-pgid, 'SIGSTOP') - const stoppedMembers = processIdentities(true).filter((identity) => identity.pgid === pgid) + operations.signalProcess(-pgid, 'SIGSTOP') + const stoppedMembers = operations + .processIdentities(true) + .filter((identity) => identity.pgid === pgid) if ( stoppedMembers.length === 0 || stoppedMembers.some((identity) => !identity.command.includes(environmentFragment)) ) { - process.kill(-pgid, 'SIGCONT') throw new Error('Benchmark process group changed before signaling') } if (signal !== 'SIGSTOP') { - process.kill(-pgid, signal) + operations.signalProcess(-pgid, signal) + if (signal !== 'SIGKILL') { + operations.signalProcess(-pgid, 'SIGCONT') + } + stopped = false } return true } catch (error) { - if (error.code !== 'ESRCH') { - throw error + const recoveryErrors = [] + if (stopped) { + try { + const recoveryMembers = operations + .processIdentities(true) + .filter( + (identity) => identity.pgid === pgid && identity.command.includes(environmentFragment) + ) + for (const member of recoveryMembers) { + try { + operations.signalProcess(member.pid, 'SIGCONT') + } catch (caught) { + if (caught?.code !== 'ESRCH') { + recoveryErrors.push(caught) + } + } + } + } catch (caught) { + recoveryErrors.push(caught) + } } - return false + if (recoveryErrors.length > 0) { + throw new AggregateError( + [error, ...recoveryErrors], + 'Benchmark process group signal recovery failed' + ) + } + if (error.code === 'ESRCH') { + return false + } + throw error } } @@ -128,28 +252,57 @@ export function processIdentityIsCurrent(identity) { return sameIdentity(processIdentity(identity?.pid), identity) } -export function signalProcessIdentity(identity, expectedCommandFragment, signal) { +const processSignalOperations = { + processIdentity, + signalProcess: process.kill.bind(process) +} + +export function signalProcessIdentity( + identity, + expectedCommandFragment, + signal, + operations = processSignalOperations +) { validateDetachedIdentity(identity, expectedCommandFragment) - const currentIdentity = processIdentity(identity.pid) + const currentIdentity = operations.processIdentity(identity.pid) if (!currentIdentity) { return false } if (!sameIdentity(currentIdentity, identity)) { throw new Error('Recorded benchmark helper PID now belongs to another process') } + let stopped = false try { - process.kill(identity.pid, 'SIGSTOP') - const stoppedIdentity = processIdentity(identity.pid) + operations.signalProcess(identity.pid, 'SIGSTOP') + stopped = true + const stoppedIdentity = operations.processIdentity(identity.pid) if (!sameIdentity(stoppedIdentity, identity)) { - process.kill(identity.pid, 'SIGCONT') + stopped = false throw new Error('Recorded benchmark helper PID changed before signaling') } - process.kill(-identity.pgid, signal) + operations.signalProcess(-identity.pgid, signal) if (signal !== 'SIGKILL') { - process.kill(-identity.pgid, 'SIGCONT') + operations.signalProcess(-identity.pgid, 'SIGCONT') } + stopped = false return true } catch (error) { + let resumeError + if (stopped) { + try { + const recoveryIdentity = operations.processIdentity(identity.pid) + if (sameIdentity(recoveryIdentity, identity)) { + operations.signalProcess(identity.pid, 'SIGCONT') + } + } catch (caught) { + if (caught?.code !== 'ESRCH') { + resumeError = caught + } + } + } + if (resumeError) { + throw new AggregateError([error, resumeError], 'Benchmark helper signal recovery failed') + } if (error.code === 'ESRCH') { return false } @@ -168,23 +321,6 @@ export function killRecordedProcess(recordPath, expectedCommandFragment) { return waitForIdentityExit(record) } -export function killProcessMatchingCommand(expectedCommandFragments) { - const matches = processIdentities().filter( - (identity) => - identity.pgid === identity.pid && - expectedCommandFragments.every((fragment) => identity.command.includes(fragment)) - ) - if (matches.length === 0) { - return false - } - for (const match of matches) { - if (signalProcessIdentity(match, expectedCommandFragments[0], 'SIGKILL')) { - waitForIdentityExit(match) - } - } - return true -} - export function killRecordedAndMatchingProcesses( recordPath, recordedCommandFragment, diff --git a/config/scripts/macos-computer-helper-owner-loss-processes.test.mjs b/config/scripts/macos-computer-helper-owner-loss-processes.test.mjs index 39187914cc0..aa59cbbbaaa 100644 --- a/config/scripts/macos-computer-helper-owner-loss-processes.test.mjs +++ b/config/scripts/macos-computer-helper-owner-loss-processes.test.mjs @@ -7,6 +7,8 @@ import { killProcessMatchingCommand, killRecordedAndMatchingProcesses, killRecordedProcess, + processIdentity, + signalProcessIdentity, signalValidatedProcessGroup, spawnBenchmarkProcess, writeProcessRecord @@ -87,6 +89,58 @@ describeMacOS('macOS helper owner-loss benchmark process cleanup', () => { spawnedPids.delete(childPid) }) + it('resumes marked group members after revalidation fails', () => { + const marker = 'ORCA_OWNER_GROUP=trial' + const members = [ + { pid: 41, pgid: 41, command: `/launcher ${marker}` }, + { pid: 42, pgid: 41, command: `/child ${marker}` } + ] + const signals = [] + let scanCount = 0 + + expect(() => + signalValidatedProcessGroup(41, marker, 'SIGKILL', { + processIdentities: () => { + scanCount += 1 + if (scanCount === 3) { + throw new Error('transient group inspection failure') + } + return members + }, + signalProcess: (pid, signal) => { + signals.push([pid, signal]) + } + }) + ).toThrow('transient group inspection failure') + expect(signals).toEqual([ + [41, 'SIGSTOP'], + [-41, 'SIGSTOP'], + [41, 'SIGCONT'], + [42, 'SIGCONT'] + ]) + }) + + it('does not signal a replacement group anchor', () => { + const marker = 'ORCA_OWNER_GROUP=trial' + const anchor = { pid: 41, pgid: 41, command: `/launcher ${marker}` } + const replacement = { pid: 41, pgid: 41, command: '/unrelated' } + const signals = [] + let scanCount = 0 + + expect(() => + signalValidatedProcessGroup(41, marker, 'SIGKILL', { + processIdentities: () => { + scanCount += 1 + return scanCount === 1 ? [anchor] : [replacement] + }, + signalProcess: (pid, signal) => { + signals.push([pid, signal]) + } + }) + ).toThrow('Benchmark process group anchor changed before signaling') + expect(signals).toEqual([[41, 'SIGSTOP']]) + }) + it('kills a recorded helper in a separate process group', async () => { const temporaryDirectory = mkdtempSync( path.join(tmpdir(), 'orca-owner-benchmark-cleanup-test-') @@ -119,21 +173,129 @@ describeMacOS('macOS helper owner-loss benchmark process cleanup', () => { spawnedPids.delete(helper.pid) }) - it('kills an unrecorded helper using its unique trial command', async () => { + it('kills every unrecorded helper using its unique trial command', async () => { const marker = `orca-owner-unrecorded-${process.pid}-${Date.now()}` - const helper = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1_000)', marker], { - detached: true, - stdio: 'ignore' - }) - spawnedPids.add(helper.pid) - helper.unref() - const exited = new Promise((resolve) => helper.once('exit', resolve)) + const helpers = Array.from({ length: 2 }, () => + spawn(process.execPath, ['-e', 'setInterval(() => {}, 1_000)', marker], { + detached: true, + stdio: 'ignore' + }) + ) + for (const helper of helpers) { + spawnedPids.add(helper.pid) + helper.unref() + } + const exited = Promise.all( + helpers.map((helper) => new Promise((resolve) => helper.once('exit', resolve))) + ) expect(killProcessMatchingCommand([process.execPath, marker])).toBe(true) await exited - expect(() => process.kill(helper.pid, 0)).toThrow() + for (const helper of helpers) { + expect(() => process.kill(helper.pid, 0)).toThrow() + spawnedPids.delete(helper.pid) + } + }) - spawnedPids.delete(helper.pid) + it('continues exact-match cleanup after an earlier match fails', () => { + const marker = `orca-owner-multiple-${process.pid}-${Date.now()}` + const matches = [ + { pid: 41, pgid: 41, command: `/helper ${marker}` }, + { pid: 42, pgid: 42, command: `/helper ${marker}` } + ] + const attempted = [] + let scanCount = 0 + + expect(() => + killProcessMatchingCommand(['/helper', marker], { + processIdentities: () => { + scanCount += 1 + return scanCount === 1 ? matches : [matches[0]] + }, + signalProcessIdentity: (identity) => { + attempted.push(identity.pid) + if (identity.pid === matches[0].pid) { + throw new Error('identity changed') + } + return true + }, + waitForIdentityExit: () => {} + }) + ).toThrow('Benchmark exact-command cleanup failed') + expect(attempted).toEqual([41, 42]) + }) + + it('does not treat an identity query failure as process exit', () => { + const queryError = new Error('transient ps failure') + + expect(() => + processIdentity(41, { + executePs: () => { + throw queryError + }, + signalProcess: () => {} + }) + ).toThrow(queryError) + }) + + it('resumes a helper when post-stop identity inspection fails', () => { + const identity = { pid: 41, pgid: 41, command: '/helper marker' } + const signals = [] + let inspectionCount = 0 + + expect(() => + signalProcessIdentity(identity, 'marker', 'SIGKILL', { + processIdentity: () => { + inspectionCount += 1 + if (inspectionCount === 2) { + throw new Error('transient ps failure after stop') + } + return identity + }, + signalProcess: (pid, signal) => { + signals.push([pid, signal]) + } + }) + ).toThrow('transient ps failure after stop') + expect(signals).toEqual([ + [41, 'SIGSTOP'], + [41, 'SIGCONT'] + ]) + }) + + it('does not signal a replacement PID after post-stop revalidation', () => { + const identity = { pid: 41, pgid: 41, command: '/helper marker' } + const replacement = { pid: 41, pgid: 41, command: '/unrelated' } + const signals = [] + let inspectionCount = 0 + + expect(() => + signalProcessIdentity(identity, 'marker', 'SIGKILL', { + processIdentity: () => { + inspectionCount += 1 + return inspectionCount === 1 ? identity : replacement + }, + signalProcess: (pid, signal) => { + signals.push([pid, signal]) + } + }) + ).toThrow('Recorded benchmark helper PID changed before signaling') + expect(signals).toEqual([[41, 'SIGSTOP']]) + }) + + it('treats a missing PID as process exit after an identity query failure', () => { + const missingProcessError = Object.assign(new Error('missing process'), { code: 'ESRCH' }) + + expect( + processIdentity(41, { + executePs: () => { + throw new Error('ps found no process') + }, + signalProcess: () => { + throw missingProcessError + } + }) + ).toBeNull() }) it('runs unique-command cleanup after an invalid process record', async () => {