mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 08:03:12 +00:00
fix(crash-reporting): stop the codex POSIX teardown claiming a group that was already gone
terminatePosixTree's default group signal swallowed every process.kill error and then recorded a self_tree_kill unconditionally, so an ESRCH — proof the group was already gone and this teardown killed nothing — still put a suspect in the five-second render-process-gone attribution window. Every sibling group-kill in the tree already records only on a proven signal: terminateDedicatedPosixGroup in this same file, forceKillPosixPtyProcessGroups, and the claude account-login teardown. This makes the outlier match them.
This commit is contained in:
@@ -1,7 +1,14 @@
|
||||
import type { ChildProcess } from 'node:child_process'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
findSelfInitiatedTreeKills,
|
||||
resetSelfInitiatedTreeKillLogForTest
|
||||
} from '../crash-reporting/self-initiated-tree-kill-log'
|
||||
import { terminateCodexAppServerProcessTree } from './codex-app-server-process-teardown'
|
||||
|
||||
/** Above pid_max on every supported POSIX host, so the group signal is a real ESRCH. */
|
||||
const UNREACHABLE_PGID = 2_147_483_647
|
||||
|
||||
function child() {
|
||||
return {
|
||||
pid: 1234,
|
||||
@@ -10,6 +17,10 @@ function child() {
|
||||
}
|
||||
|
||||
describe('terminateCodexAppServerProcessTree', () => {
|
||||
beforeEach(() => {
|
||||
resetSelfInitiatedTreeKillLogForTest()
|
||||
})
|
||||
|
||||
it('waits for the Windows tree kill before releasing the wrapper', async () => {
|
||||
const target = child()
|
||||
const release = Promise.withResolvers<void>()
|
||||
@@ -120,6 +131,55 @@ describe('terminateCodexAppServerProcessTree', () => {
|
||||
expect(target.kill).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
/**
|
||||
* `selfInitiatedTreeKillCount` decides whether a `render-process-gone` was
|
||||
* ours. A group that had already exited was killed by nobody, so crediting it
|
||||
* puts a suspect in the five-second window that Orca never issued. Exercised
|
||||
* through the real `process.kill(-pgid)` because the swallow being tested
|
||||
* lives in the production default, not in an injectable seam.
|
||||
*/
|
||||
it('does not claim a snapshot group that was already gone', async () => {
|
||||
const target = { pid: UNREACHABLE_PGID, kill: vi.fn(() => true) as ChildProcess['kill'] }
|
||||
|
||||
await expect(
|
||||
terminateCodexAppServerProcessTree(target, undefined, {
|
||||
platform: 'darwin',
|
||||
captureDescendants: async () => ({
|
||||
rootPgid: UNREACHABLE_PGID,
|
||||
descendants: [],
|
||||
capturedAtMs: 1
|
||||
}),
|
||||
terminateDescendants: async () => true
|
||||
})
|
||||
).resolves.toBe(true)
|
||||
|
||||
expect(target.kill).toHaveBeenLastCalledWith('SIGKILL')
|
||||
expect(findSelfInitiatedTreeKills(Date.now())).toEqual([])
|
||||
})
|
||||
|
||||
it('claims a snapshot group the signal actually reached', async () => {
|
||||
const target = child()
|
||||
const signalProcessGroup = vi.fn()
|
||||
|
||||
await expect(
|
||||
terminateCodexAppServerProcessTree(target, undefined, {
|
||||
platform: 'darwin',
|
||||
captureDescendants: async () => ({ rootPgid: 1234, descendants: [], capturedAtMs: 1 }),
|
||||
terminateDescendants: async () => true,
|
||||
signalProcessGroup
|
||||
})
|
||||
).resolves.toBe(true)
|
||||
|
||||
expect(signalProcessGroup).toHaveBeenCalledWith(1234, 'SIGKILL')
|
||||
expect(findSelfInitiatedTreeKills(Date.now())).toEqual([
|
||||
expect.objectContaining({
|
||||
pid: 1234,
|
||||
site: 'codex-app-server-teardown',
|
||||
scope: 'posix-process-group'
|
||||
})
|
||||
])
|
||||
})
|
||||
|
||||
it('tears down 40 dedicated groups without process-table scans or cross-group fanout', async () => {
|
||||
const killMocks = Array.from({ length: 40 }, () => vi.fn(() => true))
|
||||
const targets = killMocks.map((kill, index) => ({
|
||||
|
||||
@@ -128,19 +128,25 @@ async function terminatePosixTree(
|
||||
if (descendantsExited && snapshot.rootPgid === rootPid) {
|
||||
const signalGroup =
|
||||
deps.signalProcessGroup ??
|
||||
((pgid: number, signal: NodeJS.Signals) => {
|
||||
try {
|
||||
process.kill(-pgid, signal)
|
||||
} catch {
|
||||
// Group already exited.
|
||||
}
|
||||
((pgid: number, signal: NodeJS.Signals) => process.kill(-pgid, signal))
|
||||
let groupSignalled = false
|
||||
try {
|
||||
signalGroup(snapshot.rootPgid, 'SIGKILL')
|
||||
groupSignalled = true
|
||||
} catch {
|
||||
// Group already exited: still the desired outcome, but nothing here
|
||||
// killed it, and a breadcrumb for a kill we never landed is a false
|
||||
// suspect in the render-process-gone window.
|
||||
}
|
||||
if (groupSignalled) {
|
||||
// Outside the try, like terminateDedicatedPosixGroup: that catch is the
|
||||
// already-gone contract, not a breadcrumb handler.
|
||||
recordSelfInitiatedTreeKill({
|
||||
pid: snapshot.rootPgid,
|
||||
site: 'codex-app-server-teardown',
|
||||
scope: 'posix-process-group'
|
||||
})
|
||||
signalGroup(snapshot.rootPgid, 'SIGKILL')
|
||||
recordSelfInitiatedTreeKill({
|
||||
pid: snapshot.rootPgid,
|
||||
site: 'codex-app-server-teardown',
|
||||
scope: 'posix-process-group'
|
||||
})
|
||||
}
|
||||
}
|
||||
if (!descendantsExited) {
|
||||
child.kill('SIGCONT')
|
||||
|
||||
Reference in New Issue
Block a user