test(computer): harden owner benchmark cleanup

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,

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 () => {