From 61bcca9fee61523adc26033feca412f0b7f1e7e1 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 6 Oct 2026 17:24:19 -0700 Subject: [PATCH] fix: stop Codex and OpenCode helper servers when Orca quits or crashes (STA-9254, 2 of 2) (#25753) * fix(supervisor): add a one-shot lifetime that runs past stdin end, and relay a provider's last output A one-shot CLI (claude -p, codex exec -) reads its request until stdin ends, so the supervisor's session rule (stdin end means the owner is closing) would stop it about 1 s into its answer. The one-shot lifetime passes stdin end through and leaves only the owner-death watch and explicit signals to stop it. The supervisor also exited as soon as its provider did, dropping output still in the pipes when the owner reads slowly. It now waits, bounded, for the provider's output to be relayed before exiting. * fix(text-generation): run agent one-shots under the provider supervisor on POSIX The Claude model-list probe, model discovery for every agent, and commit message, pull request and branch name generation all spawned the agent CLI as a plain child of Orca. If Orca quit, crashed or was killed while one was running, nothing stopped it, and a CLI that hung kept running after Orca was gone. They now run under the provider supervisor that native chat already uses, in its one-shot lifetime, so Orca's exit stops the agent's whole process group however Orca exits. A timeout or cancel asks the supervisor to stop (SIGTERM) and only tears the tree down once it has had its full stop time; killing the supervisor first would orphan the agent's group. A missing binary now reaches Orca as the supervisor's exit 127, which is mapped back to the existing not-found message. Windows and WSL keep spawning the agent directly. * fix(supervisor): carry the provider argv on the supervisor's argv, map every spawn error back, and share one stop ladder - The supervisor read the provider's command and arguments from one base64 JSON env string. Linux caps a single env string at 128 KiB, so an argv prompt of about 90-120 KiB, which passes the 120 KiB per-argument guard, failed execve with E2BIG. The provider argv now follows the supervisor script's '--' as real arguments; the env keeps only small fields. - A supervisor that cannot start its provider reports Node's spawn error line and exits 127. That line now becomes the same error a direct spawn emits, so ENOENT still reads as 'not found on PATH' and EACCES or any other spawn error reads as 'failed to start'. - stopSupervisedProvider is the one ask, wait and force ladder: it gives a supervisor its full stop time before forcing. Agent one-shots, the Codex app-server close and the Claude child exit proof now share it, with the same requests, bounds and forced steps as before. * fix(supervisor): report every provider spawn failure on one marked stderr line Node throws most spawn failures (ENOEXEC, ENOTDIR, ELOOP, EPERM, ...) instead of emitting them, and the supervisor had no catch, so it died with exit 1 and a stack trace that the user saw as the agent's failure. A thrown or emitted spawn failure now exits 127 with one marked line carrying whether it was thrown, its code and its message. Orca reads only the last stderr line, so a runtime warning printed earlier cannot hide it, and maps it to the message a direct spawn gave: thrown is 'could not be started', ENOENT is 'not found on PATH', and any other emitted error is 'failed to start'. * fix(supervisor): keep the user's Node options away from the supervisor and hand them to the provider The supervisor runs Electron in Node mode, which honours NODE_OPTIONS and NODE_REPL_EXTERNAL_MODULE. A user value such as a --require of a missing file stopped the supervisor from starting, breaking even native CLIs like Codex that never load it. The launch now takes both out of the supervisor's environment, carries them in its spec, and restores them for the provider only, so a Node-based CLI still gets them. * fix(text-generation): file a forced agent one-shot teardown under its own breadcrumb site The forced tree teardown recorded every self-initiated kill as the Codex app-server's, so a forced commit-message or model-discovery stop read as a Codex teardown in crash breadcrumbs. The teardown now takes the caller's site; source-control stops pass the site their Windows tree kill already uses. * test(text-generation): cover the supervised stop under timeout and output limit; name the direct-child suites The commit-message suites that drive fake children spawn them directly, the unsupervised shape Windows and WSL use, so they now say so. The supervised POSIX stop gets its own compositions: a timed-out Codex generation settles at once but holds the Codex home until its supervisor has stopped (faithful fake, fake timers), and an agent that floods past the output limit is stopped through its real supervisor with no process left behind. * fix(codex): run short-lived app-server sessions under the provider supervisor on POSIX The Codex model-list probe, the hook trust grant and the session index heal each start a short-lived `codex app-server` as a plain child of Orca, and counted on it exiting when its input closes. A wedged Codex (a cold model/list waiting on the network, say) kept running after Orca quit, crashed or was killed. These sessions now run under the provider supervisor that native chat's Codex connection already uses, in its session lifetime, so Orca's exit stops the server's whole process group. The session's end and its deadline share the one stop ladder: end its input (and SIGTERM a session past its deadline), give the supervisor its full stop time, and only then tear the tree down. A missing binary reaches Orca as the supervisor's exit 127 and is mapped back to the spawn error, so the trust-grant telemetry still reads it as a missing binary. Windows and WSL keep spawning the server directly, with the same timings as before. The supervisor now takes only the command line it starts, so the CLI build, which also runs these sessions, no longer pulls in the native chat connection's types. * refactor(supervisor): share the stop of a supervised child process Agent one-shots stop their supervisor with SIGTERM through the shared stop ladder, watching the child's own exit. That adapter moves into one helper so the Codex backfill recovery can use it with its own stop request, instead of a copy. * fix(codex): supervise the app-server that keeps a Codex index backfill alive While Codex rebuilds its session index, Orca keeps a read-only `codex app-server` running for up to an hour so Codex can finish. It was a plain child of Orca with its input held open, so a quit, crash or kill left it running. On POSIX it now runs under the provider supervisor in its session lifetime, so Orca's exit stops its whole process group. Stopping it (done, aborted, or given up) ends its input, as a Codex connection close does; the supervisor then SIGTERMs the group and SIGKILLs it after the grace, and the tree is torn down only if the supervisor outlives its full stop time. Windows and WSL keep the direct spawn and the drain-first probe termination. The spawn and stop of that process move into their own module. * fix(opencode): stop the launch model preflight server with Orca on POSIX Before an OpenCode launch, Orca starts `opencode serve` to read the configured agent and models, then stops it. The server was detached into its own process group and never exits when its input ends, so if Orca quit, crashed or was killed during that preflight (up to 10 s), nothing ever stopped it. On POSIX the server now runs under the provider supervisor in its one-shot lifetime: the preflight's closed input does not stop it, and Orca's exit does. The preflight's teardown asks the supervisor to stop (SIGTERM) and forces the tree only after the supervisor's full stop time; signalling or SIGKILLing the supervisor's own group would orphan the server's. A descendant that ignores SIGTERM is now killed with the group rather than left running once the pipes close. Windows keeps the direct spawn and its existing teardown. * test(codex): pin when supervised session and backfill stops escalate A session past its deadline is SIGTERMed through its supervisor rather than waiting out the stdin-end grace, and its tree is torn down only after the supervisor's full stop time; Windows keeps its deadline kill and 1.5 s close wait. A supervised backfill app-server is stopped by ending its input, with the same full stop time before any teardown. * test(text-generation): run the direct-child suites on the Windows path and cover supervised discovery The commit-message suites that drive fake children mocked the supervisor away on POSIX, so they asserted a direct root SIGKILL that production no longer takes there. They now pin the platform to Windows (with an empty PATH, so host installs cannot answer a bare agent name) and assert the Windows kill, taskkill included. The three tests that check the host's own discovery spawn shape run on the host and read the agent argv past the supervisor's '--'. Model discovery gets its supervised composition: a timed-out Codex discovery settles at once but holds the Codex home until its supervisor has stopped. * fix(supervisor): show a supervised spawn failure in native chat as the spawn error it was Native chat's exit errors carry the provider's stderr tail into Details. Under the supervisor a missing CLI left the supervisor's internal spawn-failure report there instead of Node's own 'spawn ENOENT'. The report, its parser and a display formatter now live in one module; the Codex app-server and Claude stream-json exit errors pass the tail through the formatter, which turns a report back into the spawn error and leaves any other stderr unchanged. * fix(supervisor): report a spawn that failed without a pid instead of crashing on its missing pipes When the provider spawn fails outright (EMFILE, ENFILE), Node emits 'error' later and leaves the child with no pid and no stdio. Piping stdin into the missing pipe threw first, so the supervisor died with exit 1 and a stack trace and never wrote its spawn-failure report. The pipes are now wired only for a provider that started. * refactor(supervisor): share the stop of a supervised child process Agent one-shots stop their supervisor with SIGTERM through the shared stop ladder, watching the child's own exit. That adapter moves into one helper beside the ladder, so other supervised children can use it with their own stop request instead of a copy. The caller's breadcrumb site still reaches the forced teardown. Same request, wait and force as before. * fix(codex,opencode): file forced backfill and preflight teardowns under their own breadcrumb sites A forced teardown of the Codex backfill app-server is filed under 'codex-state-db-backfill-recovery', and one of the OpenCode launch model preflight under 'opencode-launch-model-preflight', instead of the generic 'codex-app-server-teardown'. * test(codex): write the stand-in pid report atomically A loaded host let the test read the pid file between its creation and its write (Unexpected end of JSON input); the stand-in now renames it into place. * fix(supervisor): give a session provider its stdin end and grace when its owner dies The owner-death watch went straight to the group SIGTERM and cancelled any stdin-end grace, so when Orca quit or crashed a session provider such as the Codex app-server never saw the EOF that lets it finish writing its state (auth.json, the state database). A session whose owner is gone now closes as an owner's stdin end does: the provider's stdin is ended, it gets the stdin-end grace, and only then the SIGTERM and SIGKILL ladder. A one-shot already had its EOF at the end of its request, so its owner's death still stops it at once. * fix(opencode): stop the preflight server through the shared supervised stop, and trust only a proven stop The preflight's own stop wrapper waited on the supervisor's pipes and counted a forced teardown as proof, though the teardown reports success even when it found no descendants to check, and a supervisor that failed to reap its group exits 1 with its pipes closed. It now uses stopSupervisedChildProcess with its breadcrumb site, and counts the server stopped only when the supervisor ended on its stop signal or relayed the server's own exit. A forced stop, or an exit of 1, returns no context, as an unverified stop did before. * chore(codex): state the supervised stop time in the probe and trust grant deadline comments * test(codex): assert the signals a supervised stop sends itself, not a SIGKILL the mocked teardown never could * fix(supervisor): kill the rest of the provider group once the provider exits on a stop A requested stop waited out the whole SIGTERM grace for the provider's group even after the provider itself had exited, so a SIGTERM-ignoring helper it left behind held every stop for up to 3 s. Under a stop, the rest of the group is now SIGKILLed as soon as the provider has exited, the same rule its own exit already follows. * test(text-generation): cover a supervised Codex discovery past its output limit Model discovery's supervised stop was covered only under timeout. A Codex discovery that floods past the output limit now runs through a real supervisor: it settles with the too-much-data error, its agent is stopped through the supervisor, and the next discovery on the same Codex home starts only after that agent is gone. The direct-child suites' headers now list exactly the supervised cases that are covered. * fix(supervisor): close a provider whose owner is gone the way its owner closes it Owner death gave every session provider the stdin-end grace, so after an Orca crash a Claude session, whose close is a stdin end plus SIGTERM, could keep working on its turn for a second with nobody watching. The spawn spec now names the provider's close request: 'stdin-end' (the Codex app-server drains and exits on EOF, then gets its grace) or 'stdin-end-and-sigterm' (Claude; the default). A gone owner gets that same request. One constant per provider feeds both its spawn spec and its owner-side close, through one requestProviderClose, so the two cannot drift. One-shots still stop at once. * fix(codex): close short-lived sessions and the backfill app-server the way a Codex connection closes Codex finishes its writes and exits on its stdin end, so the Codex connection's supervisor closes it by ending stdin, and an owner that is gone now gets that same close. The short-lived sessions and the backfill app-server now name the same close request, through one shared constant, in their spawn spec and in their own close: Orca quitting or crashing gives them the stdin end and its grace before SIGTERM, instead of an immediate SIGTERM. A session past its deadline still adds a SIGTERM, since it is wedged. * test(codex,opencode): an owner's death drains Codex before SIGTERM, and the preflight stop no longer waits out the grace The stand-in now records when its stdin ended and when SIGTERM arrived. A SIGKILLed owner leaves a Codex session or backfill app-server its stdin-end grace before SIGTERM, and the OpenCode preflight ends within 1 s of its server's SIGTERM even with a descendant that ignores SIGTERM. * test(codex): give the trust-grant deadline tests room for a supervised start on a loaded host At 500 ms a loaded full run hit the deadline before the supervisor had started the stub, which never wrote the pid the test reads. * fix(supervisor): keep the SIGTERM grace for a session's group after its provider exits Killing the rest of the group the moment the provider exited under a stop also reached native chat's closes, so an MCP server, a tool's child or a dev server still in Claude's or Codex's group was SIGKILLed mid-cleanup instead of getting the rest of the SIGTERM grace. The early group kill now applies only to one-shots, where the saved wait was the point; a session's stop is back to waiting out the grace for its group. * test(claude): pin that Claude's spawn passes its close request explicitly The spawn-spec assertion matched the default close request, so dropping Claude's explicit request still passed. The test now checks that the spec is built with the exit-proof ladder's own constant. * refactor(codex): give the Codex app-server close request its own module Other Codex app-server spawns will name the same close request as the connection does. Holding it in its own small module lets them import it without the connection itself. * build(cli): list the Codex close request and the provider supervisor in the CLI project The command-line build runs the short-lived Codex app-server session, which will name the same close request as the Codex connection. Listing the close request, the provider supervisor it takes its type from, and the spawn-failure report the supervisor uses lets the CLI project typecheck that import without pulling the connection in. * test(codex): give a stand-in 20 s to report its pids on a loaded host At a load average near 50 both owner-death tests timed out waiting for the bundled owner's stand-in at 10 s; they pass alone. * test(codex): start the deadline tests' clocks past the stand-in's start, and cover a server that ignores SIGTERM The session and trust-grant deadline tests ran a 2-4 s deadline from spawn, so a loaded host could stop the stand-in before it wrote its pid. The session test's deadline now outlasts its pid-read budget, and the trust-grant deadlines are 8 s. The trust-grant comment said a wedged server may ignore everything but SIGKILL, but that stub dies on its stdin end; a new POSIX case pins that a server ignoring both its stdin end and SIGTERM is SIGKILLed after the SIGTERM grace. * test(text-generation): check the ENOEXEC start failure only where Node reports one On Linux, glibc's execvp hands an executable that is not a program to /bin/sh, so both a direct and a supervised spawn run it and it exits 127; only macOS throws ENOEXEC. The not-a-program case now runs on macOS only; the path-through-a-file case (ENOTDIR) still covers a thrown start failure everywhere. * test(wsl): follow the backfill's wsl.exe spawn into its new process module The WSL invocation boundary lists files that spawn wsl.exe directly. The backfill recovery's spawn moved into codex-state-db-backfill-recovery-process.ts, so the entry moves with it; the count is unchanged. * test(codex): keep factory child_process mocks loadable now that Codex stops reach the process-table reader The backfill recovery and Codex sessions now stop through the shared supervised teardown, whose process-table reader binds execFile when it loads. Five rate-limit fetcher tests mocked node:child_process with only spawn and failed at load: they now mock the backfill recovery, as their sibling fetcher tests already do. The account add-login tests' child_process mocks gain an execFile stub. * fix(opencode): count no forced preflight stop as proof the server is gone A forced teardown walks and group-kills the supervisor's tree, but the server leads its own detached group, so a teardown verdict of 'exited' does not cover members left in the server's group. Only a supervisor that reaped the group itself, by its own stop or relaying the server's exit, now counts as a proven stop. The Windows session-stop test now says why it sees no direct kill. --- config/tsconfig.cli.json | 20 ++ .../service-account-add-login.test.ts | 12 + .../codex/codex-app-server-client.test.ts | 53 ++++- .../codex-app-server-session-stop.test.ts | 134 +++++++++++ ...dex-app-server-session-supervision.test.ts | 219 ++++++++++++++++++ src/main/codex/codex-app-server-session.ts | 99 +++++--- src/main/codex/codex-model-catalog-probe.ts | 4 +- ...state-db-backfill-recovery-process.test.ts | 79 +++++++ ...odex-state-db-backfill-recovery-process.ts | 75 ++++++ ...e-db-backfill-recovery-supervision.test.ts | 146 ++++++++++++ .../codex-state-db-backfill-recovery.test.ts | 10 +- .../codex/codex-state-db-backfill-recovery.ts | 58 ++--- .../supervised-probe-owner.test-fixture.ts | 151 ++++++++++++ ...ncode-launch-model-context-cleanup.test.ts | 139 ++++++++++- ...e-launch-model-context-supervision.test.ts | 96 ++++++++ .../opencode/opencode-launch-model-context.ts | 72 ++++-- .../codex-fetcher-auth-errors.test.ts | 5 + .../rate-limits/codex-fetcher-backend.test.ts | 5 + .../codex-fetcher-credit-balance.test.ts | 5 + .../codex-fetcher-probe-shutdown.test.ts | 5 + .../codex-fetcher-session-supplement.test.ts | 5 + 21 files changed, 1279 insertions(+), 113 deletions(-) create mode 100644 src/main/codex/codex-app-server-session-stop.test.ts create mode 100644 src/main/codex/codex-app-server-session-supervision.test.ts create mode 100644 src/main/codex/codex-state-db-backfill-recovery-process.test.ts create mode 100644 src/main/codex/codex-state-db-backfill-recovery-process.ts create mode 100644 src/main/codex/codex-state-db-backfill-recovery-supervision.test.ts create mode 100644 src/main/codex/supervised-probe-owner.test-fixture.ts create mode 100644 src/main/opencode/opencode-launch-model-context-supervision.test.ts diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index 7c5d3509e55..725820f664e 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -86,6 +86,21 @@ "../src/main/windows-native-registry.ts", "../src/main/windows/windows-command-line-recovery-health.ts", "../src/main/windows/windows-process-table.ts", + "../src/main/windows/windows-pty-job.ts", + "../src/main/crash-reporting/crash-breadcrumb-store.ts", + "../src/main/crash-reporting/durable-crash-breadcrumb.ts", + "../src/main/crash-reporting/main-process-lifecycle-identity.ts", + "../src/main/crash-reporting/self-initiated-tree-kill-log.ts", + "../src/main/observability/redactor.ts", + "../src/main/observability/tracer.ts", + "../src/main/orca-chromium-process-pids.ts", + "../src/main/own-chromium-tree-kill-guard.ts", + "../src/main/providers/windows-foreground-process-rows.ts", + "../src/main/pty-descendant-exit-verification.ts", + "../src/main/pty-descendant-termination.ts", + "../src/main/pty-process-table-parser.ts", + "../src/main/windows-process-tree-kill.ts", + "../src/main/windows-pty-root-identity.ts", "../src/main/windows/windows-process-table-cim-scan.ts", "../src/main/daemon/daemon-process-start-time.ts", "../src/main/daemon/daemon-process-identity-query.ts", @@ -157,7 +172,12 @@ "../src/main/codex/codex-hook-wsl-runtime.ts", "../src/main/codex/codex-managed-trust-reconciliation.ts", "../src/main/provider-process/provider-process-exit-deadline.ts", + "../src/main/provider-process/managed-provider-process.ts", + "../src/main/provider-process/provider-process-close.ts", "../src/main/provider-process/provider-process-launch.ts", + "../src/main/provider-process/provider-process-supervisor.ts", + "../src/main/provider-process/provider-process-teardown.ts", + "../src/main/provider-process/provider-spawn-failure-report.ts", "../src/main/codex/codex-real-home-hook-entry-plan.ts", "../src/main/codex/codex-real-home-hook-install.ts", "../src/main/codex/codex-real-home-hook-sweep.ts", diff --git a/src/main/codex-accounts/service-account-add-login.test.ts b/src/main/codex-accounts/service-account-add-login.test.ts index 4bde86486cc..d9b963d8bc9 100644 --- a/src/main/codex-accounts/service-account-add-login.test.ts +++ b/src/main/codex-accounts/service-account-add-login.test.ts @@ -82,6 +82,8 @@ describe('CodexAccountService config sync', () => { ) vi.doMock('node:child_process', () => ({ + // The supervised Codex teardown's process-table reader binds execFile when it loads. + execFile: vi.fn(), execFileSync: vi.fn(), spawn: spawnMock })) @@ -146,6 +148,8 @@ describe('CodexAccountService config sync', () => { ) vi.doMock('node:child_process', () => ({ + // The supervised Codex teardown's process-table reader binds execFile when it loads. + execFile: vi.fn(), execFileSync: vi.fn(), spawn: spawnMock })) @@ -254,6 +258,8 @@ describe('CodexAccountService config sync', () => { ) vi.doMock('node:child_process', () => ({ + // The supervised Codex teardown's process-table reader binds execFile when it loads. + execFile: vi.fn(), execFileSync: vi.fn(), spawn: spawnMock })) @@ -411,6 +417,8 @@ describe('CodexAccountService config sync', () => { } ) vi.doMock('node:child_process', () => ({ + // The supervised Codex teardown's process-table reader binds execFile when it loads. + execFile: vi.fn(), execFileSync: vi.fn(), spawn: spawnMock })) @@ -486,6 +494,8 @@ describe('CodexAccountService config sync', () => { const spawnMock = vi.fn() vi.doMock('node:child_process', () => ({ + // The supervised Codex teardown's process-table reader binds execFile when it loads. + execFile: vi.fn(), execFileSync: vi.fn(), spawn: spawnMock })) @@ -532,6 +542,8 @@ describe('CodexAccountService config sync', () => { const spawnMock = vi.fn() vi.doMock('node:child_process', () => ({ + // The supervised Codex teardown's process-table reader binds execFile when it loads. + execFile: vi.fn(), execFileSync: vi.fn(), spawn: spawnMock })) diff --git a/src/main/codex/codex-app-server-client.test.ts b/src/main/codex/codex-app-server-client.test.ts index ce98288bdf0..65b8e2d8b20 100644 --- a/src/main/codex/codex-app-server-client.test.ts +++ b/src/main/codex/codex-app-server-client.test.ts @@ -12,6 +12,10 @@ import { type CodexHookTrustGrantRequest } from './codex-app-server-client' import { killCodexAppServerProcessTree } from './codex-app-server-process-tree-kill' +import { + PROVIDER_SIGTERM_GRACE_MS, + PROVIDER_SUPERVISOR_MAX_STOP_MS +} from '../provider-process/provider-process-supervisor' import { runCodexAppServerSession } from './codex-app-server-session' // Stub codex app-server speaking the same JSONL protocol: initialize → @@ -19,6 +23,11 @@ import { runCodexAppServerSession } from './codex-app-server-session' // via STUB_CONFIG so each test controls listings, errors, and hangs. const STUB_SERVER_SOURCE = ` const config = JSON.parse(process.env.STUB_CONFIG) +// A wedged server ignores SIGTERM and its stdin end, so only SIGKILL ends it. +if (config.scenario === 'wedged') { + process.on('SIGTERM', () => {}) + setInterval(() => {}, 60000) +} require('node:fs').writeFileSync(config.pidFile, String(process.pid)) const trusted = new Set(config.hooks.filter(h => h.trustStatus === 'trusted').map(h => h.key)) let buffer = '' @@ -68,7 +77,7 @@ process.stdin.on('data', (chunk) => { continue } if (message.method === 'initialized') continue - if (config.scenario === 'hang') continue + if (config.scenario === 'hang' || config.scenario === 'wedged') continue if (message.method === 'hooks/list') { if (config.scenario === 'unknown-method') { send({ id: message.id, error: { code: -32601, message: 'Method not found' } }) @@ -88,7 +97,7 @@ process.stdin.on('data', (chunk) => { } } }) -process.stdin.on('end', () => process.exit(0)) +process.stdin.on('end', () => { if (config.scenario !== 'wedged') process.exit(0) }) function writeFileSyncSafe(file, contents) { require('node:fs').writeFileSync(file, contents) } ` @@ -149,6 +158,9 @@ function createStubRequest(options: { } const MANAGED_COMMAND = "/bin/sh '/tmp/orca/codex-hook.sh'" +// The deadline starts at spawn and the stub writes its pid only once it runs; on a loaded host the +// supervisor and stub have taken over 2 s to start, so a shorter deadline can stop it first. +const STUB_START_DEADLINE_MS = 8_000 function managedHook(key: string, trustStatus = 'untrusted'): StubHook { return { key, command: MANAGED_COMMAND, currentHash: `sha256:hash-of-${key}`, trustStatus } @@ -393,34 +405,59 @@ describe('runCodexHookTrustGrantSession', () => { hooks: keys.map((key) => managedHook(key)), expectedTrustKeys: keys, managedCommand: MANAGED_COMMAND, - timeoutMs: 500 + timeoutMs: STUB_START_DEADLINE_MS }) const startedAt = Date.now() await expect(runCodexHookTrustGrantSession(request)).rejects.toBeInstanceOf( CodexAppServerTimeoutError ) - // Why: the reap path must not stack the grace periods on top of the - // deadline — a wedged server may ignore everything but SIGKILL. - expect(Date.now() - startedAt).toBeLessThan(5_000) + // Why: this stub exits on its stdin end and on SIGTERM, so the reap adds no grace on top of + // the deadline; a server that ignores both is the case below. + expect(Date.now() - startedAt - STUB_START_DEADLINE_MS).toBeLessThan(2_000) const childPid = Number(readFileSync(pidFile, 'utf8')) expect(() => process.kill(childPid, 0)).toThrow() }) + it.runIf(process.platform !== 'win32')( + 'stops a server that ignores its stdin end and SIGTERM after the SIGTERM grace', + async () => { + const keys = ['/home/a/.codex/hooks.json:session_start:0:0'] + const { request, pidFile } = createStubRequest({ + scenario: 'wedged', + hooks: keys.map((key) => managedHook(key)), + expectedTrustKeys: keys, + managedCommand: MANAGED_COMMAND, + timeoutMs: STUB_START_DEADLINE_MS + }) + + const startedAt = Date.now() + await expect(runCodexHookTrustGrantSession(request)).rejects.toBeInstanceOf( + CodexAppServerTimeoutError + ) + // The supervisor SIGTERMs its group at the deadline and SIGKILLs it after the grace. + const pastDeadline = Date.now() - startedAt - STUB_START_DEADLINE_MS + expect(pastDeadline).toBeGreaterThanOrEqual(PROVIDER_SIGTERM_GRACE_MS - 100) + expect(pastDeadline).toBeLessThan(PROVIDER_SUPERVISOR_MAX_STOP_MS + 1_000) + const childPid = Number(readFileSync(pidFile, 'utf8')) + expect(() => process.kill(childPid, 0)).toThrow() + } + ) + it('bounds a callback that stalls between RPC requests', async () => { const { request, pidFile } = createStubRequest({ scenario: 'happy', hooks: [], expectedTrustKeys: [], managedCommand: MANAGED_COMMAND, - timeoutMs: 500 + timeoutMs: STUB_START_DEADLINE_MS }) const startedAt = Date.now() await expect( runCodexAppServerSession(request.invocation, async () => new Promise(() => {})) ).rejects.toBeInstanceOf(CodexAppServerTimeoutError) - expect(Date.now() - startedAt).toBeLessThan(5_000) + expect(Date.now() - startedAt - STUB_START_DEADLINE_MS).toBeLessThan(2_000) const childPid = Number(readFileSync(pidFile, 'utf8')) expect(() => process.kill(childPid, 0)).toThrow() }) diff --git a/src/main/codex/codex-app-server-session-stop.test.ts b/src/main/codex/codex-app-server-session-stop.test.ts new file mode 100644 index 00000000000..3972613394f --- /dev/null +++ b/src/main/codex/codex-app-server-session-stop.test.ts @@ -0,0 +1,134 @@ +import { EventEmitter } from 'node:events' +import { PassThrough } from 'node:stream' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../provider-process/provider-process-supervisor' +import { terminateProviderProcessTree } from '../provider-process/provider-process-teardown' +import type * as ProcessTreeKill from './codex-app-server-process-tree-kill' +import { + killCodexAppServerProcessTree, + type CodexAppServerSpawn +} from './codex-app-server-process-tree-kill' +import { CodexAppServerTimeoutError, runCodexAppServerSession } from './codex-app-server-session' + +vi.mock('../provider-process/provider-process-teardown', () => ({ + terminateProviderProcessTree: vi.fn(async () => 'exited' as const) +})) +vi.mock('./codex-app-server-process-tree-kill', async (importOriginal) => ({ + ...(await importOriginal()), + killCodexAppServerProcessTree: vi.fn() +})) + +type FakeChild = EventEmitter & { + pid: number + stdin: PassThrough + stdout: PassThrough + stderr: PassThrough + kill: ReturnType boolean>> +} + +/** A child that answers initialize and then nothing; it exits only on `exitsOn`. */ +function fakeChild(exitsOn: NodeJS.Signals | null): FakeChild { + const child: FakeChild = Object.assign(new EventEmitter(), { + pid: 4242, + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn((signal?: NodeJS.Signals) => { + if (signal === exitsOn) { + child.emit('exit', null, signal) + child.emit('close', null, signal) + } + return true + }) + }) + child.stdin.on('data', (chunk: Buffer) => { + const message = JSON.parse(chunk.toString()) + if (message.method === 'initialize') { + child.stdout.write(`${JSON.stringify({ id: message.id, result: {} })}\n`) + } + }) + return child +} + +const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') + +afterEach(() => { + vi.useRealTimers() + vi.clearAllMocks() + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform) + } +}) + +function startWedgedSession(child: FakeChild, platform: NodeJS.Platform): Promise { + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the session reads only pid, stdio, kill and the exit/close/error events, which the fake implements. + const spawnImpl: CodexAppServerSpawn = () => child as never + return runCodexAppServerSession( + { command: '/bin/codex', cliPath: null, args: ['app-server'], timeoutMs: 1_000 }, + () => new Promise(() => {}), + spawnImpl + ).catch((error: unknown) => error) +} + +describe('runCodexAppServerSession stop', () => { + it('SIGTERMs a supervised session at its deadline instead of waiting out its stdin grace', async () => { + vi.useFakeTimers() + const child = fakeChild('SIGTERM') + let stdinEnded = false + child.stdin.on('finish', () => (stdinEnded = true)) + + const outcome = startWedgedSession(child, 'darwin') + await vi.advanceTimersByTimeAsync(1_000) + + expect(child.kill.mock.calls).toEqual([['SIGTERM']]) + expect(stdinEnded).toBe(true) + expect(await outcome).toBeInstanceOf(CodexAppServerTimeoutError) + expect(terminateProviderProcessTree).not.toHaveBeenCalled() + expect(killCodexAppServerProcessTree).not.toHaveBeenCalled() + }) + + it('tears a supervised session down only once the supervisor has had its full stop time', async () => { + vi.useFakeTimers() + const child = fakeChild(null) + + const outcome = startWedgedSession(child, 'linux') + await vi.advanceTimersByTimeAsync(1_000 + PROVIDER_SUPERVISOR_MAX_STOP_MS - 1) + expect(terminateProviderProcessTree).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1) + + expect(terminateProviderProcessTree).toHaveBeenCalledWith(child, { + site: 'codex-app-server-session', + platform: 'linux' + }) + await vi.advanceTimersByTimeAsync(1_000) + expect(await outcome).toBeInstanceOf(CodexAppServerTimeoutError) + // The session's only own signal is the deadline SIGTERM; only the teardown forces. + expect(child.kill.mock.calls).toEqual([['SIGTERM']]) + expect(killCodexAppServerProcessTree).not.toHaveBeenCalled() + }) + + it('keeps the Windows deadline kill and its 1.5 s close wait', async () => { + vi.useFakeTimers() + const child = fakeChild(null) + + const outcome = startWedgedSession(child, 'win32') + await vi.advanceTimersByTimeAsync(1_000) + expect(killCodexAppServerProcessTree).toHaveBeenCalledTimes(1) + await vi.advanceTimersByTimeAsync(1_499) + expect(terminateProviderProcessTree).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1) + // The shared close's Windows teardown (taskkill of the tree), as the Codex connection's. + expect(terminateProviderProcessTree).toHaveBeenCalledWith(child, { + site: 'codex-app-server-session', + platform: 'win32' + }) + + await vi.advanceTimersByTimeAsync(1_000) + expect(await outcome).toBeInstanceOf(CodexAppServerTimeoutError) + // The session sends no signal of its own on Windows (a SIGTERM there is TerminateProcess); + // the teardowns, mocked here, own the kill. + expect(child.kill).not.toHaveBeenCalled() + expect(killCodexAppServerProcessTree).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/codex/codex-app-server-session-supervision.test.ts b/src/main/codex/codex-app-server-session-supervision.test.ts new file mode 100644 index 00000000000..c7649e3d4f7 --- /dev/null +++ b/src/main/codex/codex-app-server-session-supervision.test.ts @@ -0,0 +1,219 @@ +import { EventEmitter } from 'node:events' +import { existsSync, readFileSync } from 'node:fs' +import { PassThrough } from 'node:stream' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { + PROVIDER_STDIN_END_GRACE_MS, + PROVIDER_SUPERVISOR_MAX_STOP_MS +} from '../provider-process/provider-process-supervisor' +import type { CodexAppServerSpawn } from './codex-app-server-process-tree-kill' +import { CodexAppServerTimeoutError, runCodexAppServerSession } from './codex-app-server-session' +import { classifyCodexTrustGrantError } from './codex-trust-grant-telemetry' +import { PROVIDER_SPAWN_FAILURE_MARKER } from '../provider-process/provider-spawn-failure-report' +import { + alive, + createSupervisedProbeRig, + waitFor, + type SupervisedProbeRig +} from './supervised-probe-owner.test-fixture' + +// Answers initialize, then nothing; it ignores its stdin end, as a wedged Codex can. +const WEDGED_APP_SERVER = String.raw` +require('node:readline').createInterface({ input: process.stdin }).on('line', (line) => { + const message = JSON.parse(line) + if (message.method === 'initialize') { + process.stdout.write(JSON.stringify({ id: message.id, result: {} }) + '\n') + } +}) +` + +const OWNER = String.raw` +import { runCodexAppServerSession } from './codex-app-server-session' +void runCodexAppServerSession( + { command: process.env.ORCA_TEST_STAND_IN!, cliPath: null, args: ['app-server'], timeoutMs: 120_000 }, + () => new Promise(() => {}) +).catch(() => {}) +setInterval(() => {}, 60_000) +` + +const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') +let rig: SupervisedProbeRig | null = null + +afterEach(() => { + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform) + } + rig?.cleanup() + rig = null +}) + +type CapturedSpawn = { program: string; args: string[]; options: Record } + +function captureSpawn(): { spawnImpl: CodexAppServerSpawn; calls: CapturedSpawn[] } { + const calls: CapturedSpawn[] = [] + return { + calls, + spawnImpl: (program, args, options) => { + calls.push({ program, args, options }) + throw new Error('spawn captured') + } + } +} + +function decodedSupervisorSpec(env: unknown): unknown { + const encoded = + typeof env === 'object' && env !== null && 'ORCA_PROVIDER_SUPERVISOR_SPEC' in env + ? String(env.ORCA_PROVIDER_SUPERVISOR_SPEC) + : '' + return JSON.parse(Buffer.from(encoded, 'base64').toString() || 'null') +} + +describe('short-lived Codex app-server session spawn', () => { + it('runs a POSIX session under the provider supervisor in session lifetime', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'darwin' }) + const { spawnImpl, calls } = captureSpawn() + + await expect( + runCodexAppServerSession( + { command: '/bin/codex', cliPath: null, args: ['app-server'], timeoutMs: 1_000 }, + async () => null, + spawnImpl + ) + ).rejects.toThrow('spawn captured') + + const [{ program, args, options }] = calls + expect(program).toBe(process.execPath) + expect(args.slice(args.indexOf('--') + 1)).toEqual(['/bin/codex', 'app-server']) + expect(options.detached).toBe(true) + expect(decodedSupervisorSpec(options.env)).toMatchObject({ + lifetime: 'session', + ownerPid: process.pid + }) + }) + + it('never shows the supervisor spawn-failure marker in an early-exit error', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'darwin' }) + const report = { thrown: false, code: 'ENOENT', message: 'spawn /bin/codex ENOENT' } + const child = Object.assign(new EventEmitter(), { + pid: 4242, + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: () => true + }) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the session reads only pid, stdio, kill and the exit/close/error events, which the fake implements. + const spawnImpl: CodexAppServerSpawn = () => child as never + // A report on an exit other than 127 is no spawn failure, but its text still reaches the user. + child.stderr.once('end', () => { + child.emit('exit', 1, null) + child.emit('close', 1, null) + }) + child.stderr.end(`${PROVIDER_SPAWN_FAILURE_MARKER}${JSON.stringify(report)}\n`) + + const error = await runCodexAppServerSession( + { command: '/bin/codex', cliPath: null, args: ['app-server'], timeoutMs: 5_000 }, + async () => null, + spawnImpl + ).catch((caught: unknown) => caught) + + expect(String(error)).toContain('spawn /bin/codex ENOENT') + expect(String(error)).not.toContain(PROVIDER_SPAWN_FAILURE_MARKER) + }) + + it('spawns a Windows session directly, as before', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const { spawnImpl, calls } = captureSpawn() + + await expect( + runCodexAppServerSession( + { command: 'C:\\codex.exe', cliPath: null, args: ['app-server'], timeoutMs: 1_000 }, + async () => null, + spawnImpl + ) + ).rejects.toThrow('spawn captured') + + const [{ program, args, options }] = calls + expect(decodedSupervisorSpec(options.env)).toBeNull() + expect(program).toBe('C:\\codex.exe') + expect(args).toEqual(['app-server']) + expect(options).not.toHaveProperty('detached') + }) +}) + +describe.runIf(process.platform !== 'win32')('supervised Codex app-server sessions', () => { + it('stops a wedged session at its deadline with SIGTERM to its whole group', async () => { + rig = createSupervisedProbeRig() + const standIn = rig.writeStandIn('codex', WEDGED_APP_SERVER) + + const readPids = rig.readPids + const reported: { pids?: { provider: number; grandchild: number } } = {} + // The deadline starts at spawn, so it outlasts the stand-in's whole start budget: a slow host + // fails the pid read inside that budget instead of stopping a stand-in that never armed. + const startBudgetMs = 8_000 + const session = runCodexAppServerSession( + { + command: standIn, + cliPath: null, + args: ['app-server'], + env: rig.env, + timeoutMs: startBudgetMs + 2_000 + }, + async () => { + reported.pids = await readPids(startBudgetMs) + return new Promise(() => {}) + } + ) + + await expect(session).rejects.toBeInstanceOf(CodexAppServerTimeoutError) + const { pids } = reported + expect(pids).toBeDefined() + if (!pids) { + return + } + // Graceful first: the old deadline SIGKILLed the root, which a Codex mid-write cannot survive. + expect(existsSync(rig.signalFile) && readFileSync(rig.signalFile, 'utf8')).toBe('SIGTERM') + expect(alive(pids.provider)).toBe(false) + // The group ladder reaches a descendant that ignores SIGTERM; a root-only kill misses it. + expect(alive(pids.grandchild)).toBe(false) + }) + + it('drains, then stops the session group, when its owner is SIGKILLed mid-session', async () => { + rig = createSupervisedProbeRig() + const standIn = rig.writeStandIn('codex', WEDGED_APP_SERVER) + const bundle = await rig.bundleOwner(OWNER, __dirname) + const owner = rig.launchOwner(bundle, { ORCA_TEST_STAND_IN: standIn }) + const pids = await rig.readPids() + + owner.kill('SIGKILL') + + expect( + await waitFor( + () => !alive(pids.provider) && !alive(pids.grandchild), + PROVIDER_SUPERVISOR_MAX_STOP_MS + 1_000 + ) + ).toBe(true) + // A gone owner gets the close its owner makes: the stdin end, its grace, and only then SIGTERM. + const events = rig.readEvents() + expect(events['stdin-end']).toBeDefined() + expect((events.SIGTERM ?? 0) - (events['stdin-end'] ?? 0)).toBeGreaterThanOrEqual( + PROVIDER_STDIN_END_GRACE_MS - 50 + ) + }) + + it('reports a missing Codex binary as the spawn error, not an early exit', async () => { + rig = createSupervisedProbeRig() + const error = await runCodexAppServerSession( + { + command: join(rig.dir, 'missing', 'codex'), + cliPath: null, + args: ['app-server'], + timeoutMs: 5_000 + }, + async () => null + ).catch((caught: unknown) => caught) + + expect(error).toMatchObject({ code: 'ENOENT' }) + expect(classifyCodexTrustGrantError(error)).toBe('binary-missing') + }) +}) diff --git a/src/main/codex/codex-app-server-session.ts b/src/main/codex/codex-app-server-session.ts index b9dc838f86c..122d95c16f5 100644 --- a/src/main/codex/codex-app-server-session.ts +++ b/src/main/codex/codex-app-server-session.ts @@ -1,8 +1,12 @@ import type { ChildProcessWithoutNullStreams } from 'node:child_process' -import { waitForProcessExitUntil } from '../provider-process/provider-process-exit-deadline' +import { spawnManagedProviderProcess } from '../provider-process/managed-provider-process' import { resolveProviderChildEnv } from '../provider-process/provider-process-launch' import { stderrIndicatesMissingAppServer } from './codex-app-server-capability-signal' import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' +import { + providerStderrForDisplay, + supervisedProviderSpawnFailure +} from '../provider-process/provider-spawn-failure-report' import { killCodexAppServerProcessTree, spawnCodexAppServerProcess, @@ -34,7 +38,7 @@ export type CodexAppServerInvocation = { /** Env keys stripped from the inherited environment before spawn (e.g. an inherited * CODEX_HOME, so `listCodexHooks` with no `codexHome` lists the real ~/.codex). */ envToDelete?: readonly string[] - /** Whole-session deadline. The codex child is SIGKILLed when it lapses. */ + /** Whole-session deadline. The codex child is stopped when it lapses. */ timeoutMs: number } @@ -76,7 +80,8 @@ export type CodexAppServerRpc = { } const JSON_RPC_METHOD_NOT_FOUND = -32601 -const STDERR_TAIL_MAX_BYTES = 8192 +const STDERR_DETAIL_MAX_CHARS = 400 +const CODEX_APP_SERVER_KILL_SITE = 'codex-app-server-session' /** Codex answering "no such method" is the only response that proves the RPC * surface is absent rather than temporarily failing. */ @@ -94,7 +99,8 @@ export function isCodexMethodNotFoundError(error: unknown): boolean { /** * Runs one short-lived `codex app-server` session over stdio JSON-RPC (JSONL): * spawn → initialize → initialized → body(rpc) → EOF/reap. The child is reaped - * on every path; the session deadline SIGKILLs it. + * on every path; the session deadline stops it. On POSIX it runs under the provider + * supervisor, so an Orca that quits or dies mid-session still stops its group. */ export async function runCodexAppServerSession( invocation: CodexAppServerInvocation, @@ -107,14 +113,26 @@ export async function runCodexAppServerSession( const pairedEnv = invocation.cliPath ? withCliRuntimeOnPath(invocation.cliPath, childEnv) : childEnv - const child = spawnImpl(invocation.command, invocation.args, { - env: pairedEnv, - stdio: ['pipe', 'pipe', 'pipe'], - windowsHide: true - }) as ChildProcessWithoutNullStreams + // The Codex connection's managed process and close policy, so a session closes the same way. + const managed = spawnManagedProviderProcess( + { command: invocation.command, args: invocation.args }, + { + site: CODEX_APP_SERVER_KILL_SITE, + inheritedEnv: pairedEnv, + spawnImpl: (spec) => + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: every stdio slot is 'pipe', so stdin, stdout and stderr exist. + spawnImpl(spec.program, [...(spec.args ?? [])], { + env: spec.env, + stdio: ['pipe', 'pipe', 'pipe'], + windowsHide: true, + ...(spec.detached ? { detached: true } : {}) + }) as ChildProcessWithoutNullStreams + } + ) + const { child } = managed - let stderrTail = '' let exited = false + let exitCode: number | null = null let nextRequestId = 1 let timedOut = false const pending = new Map< @@ -122,11 +140,9 @@ export async function runCodexAppServerSession( { resolve: (r: JsonRpcResponse) => void; reject: (e: Error) => void } >() - const exitPromise = new Promise((resolve) => { - child.on('exit', () => { - exited = true - resolve() - }) + managed.onExit((exit) => { + exited = true + exitCode = exit.code }) // Why: 'error' fires instead of 'exit' when the spawn itself fails // (ENOENT); surface it to every in-flight request or they wait forever. @@ -141,11 +157,6 @@ export async function runCodexAppServerSession( child.on('close', () => { failPending(buildEarlyExitError()) }) - // Why: JSONL can contain non-ASCII hook paths. Stream decoding must retain a - // multibyte character split across pipe chunks or the response becomes invalid JSON. - child.stderr.setEncoding('utf8').on('data', (chunk: string) => { - stderrTail = (stderrTail + chunk).slice(-STDERR_TAIL_MAX_BYTES) - }) // Why: a child can exit between the liveness check and stdin.write(); an // EPIPE must reject the RPC instead of becoming an unhandled stream error. child.stdin.on('error', (error) => { @@ -188,7 +199,10 @@ export async function runCodexAppServerSession( const error = new CodexAppServerTimeoutError( `codex app-server session exceeded ${invocation.timeoutMs}ms (${invocation.command})` ) - killCodexAppServerProcessTree(child) + // A supervised child is stopped by the finally below, which this rejection runs at once. + if (!managed.supervised) { + killCodexAppServerProcessTree(child) + } failPending(error) rejectDeadline(error) }, invocation.timeoutMs) @@ -260,14 +274,26 @@ export async function runCodexAppServerSession( return response.result } + // The supervisor's spawn-failure report reads as Node's own spawn error line. + function stderrDetail(): string { + return providerStderrForDisplay(managed.stderrTail()).trim().slice(0, STDERR_DETAIL_MAX_CHARS) + } + function buildEarlyExitError(): Error { - if (stderrIndicatesMissingAppServer(stderrTail)) { + // A supervisor reports a provider it could not start as exit 127; callers classify that error. + const spawnFailure = managed.supervised + ? supervisedProviderSpawnFailure(exitCode, managed.stderrTail()) + : null + if (spawnFailure) { + return spawnFailure.error + } + if (stderrIndicatesMissingAppServer(managed.stderrTail())) { return new CodexAppServerUnsupportedError( - `codex CLI does not support the app-server subcommand: ${stderrTail.trim().slice(0, 400)}` + `codex CLI does not support the app-server subcommand: ${stderrDetail()}` ) } return new Error( - `codex app-server exited before completing the session${stderrTail ? `: ${stderrTail.trim().slice(0, 400)}` : ''}` + `codex app-server exited before completing the session${managed.stderrTail() ? `: ${stderrDetail()}` : ''}` ) } @@ -287,28 +313,25 @@ export async function runCodexAppServerSession( error instanceof Error && !(error instanceof CodexAppServerUnsupportedError) && !(error instanceof CodexAppServerTimeoutError) && - stderrIndicatesMissingAppServer(stderrTail) + stderrIndicatesMissingAppServer(managed.stderrTail()) ) { throw new CodexAppServerUnsupportedError( - `codex CLI does not support the app-server subcommand: ${stderrTail.trim().slice(0, 400)}` + `codex CLI does not support the app-server subcommand: ${stderrDetail()}` ) } throw error } finally { - try { - child.stdin.end() - } catch { - // stdin may already be destroyed after a kill; reaping below still runs. - } - if (!exited) { - // Why: the server exits promptly on stdin EOF; the grace period only - // bounds a wedged child before the guaranteed SIGKILL reap. - await waitForProcessExitUntil(exitPromise, 1500) - if (!exited) { - killCodexAppServerProcessTree(child) - await waitForProcessExitUntil(exitPromise, 1000) + // A session past its deadline is wedged; its stdin end would only add a grace. + if (timedOut && managed.supervised && !exited) { + try { + child.stdin.end() + } catch { + // A destroyed stdin still leaves the SIGTERM and the close below. } + child.kill('SIGTERM') } + // Ends stdin, gives a supervisor its full stop time (a direct child 1.5 s), then the tree. + await managed.close() clearTimeout(deadline) } } diff --git a/src/main/codex/codex-model-catalog-probe.ts b/src/main/codex/codex-model-catalog-probe.ts index 2cf354259d7..66def74598c 100644 --- a/src/main/codex/codex-model-catalog-probe.ts +++ b/src/main/codex/codex-model-catalog-probe.ts @@ -10,8 +10,8 @@ import type { AgentModelCatalogSuccess } from '../native-chat/agent-model-catalog/agent-model-catalog-store' -// Why 15s: the whole probe session is SIGKILLed at the deadline, and a cold -// `model/list` may pay one network /models fetch behind provider auth. +// Why 15s: the whole probe session is stopped at the deadline (on POSIX its supervisor then gets +// up to PROVIDER_SUPERVISOR_MAX_STOP_MS), and a cold `model/list` may pay one /models fetch. const CODEX_MODEL_CATALOG_PROBE_TIMEOUT_MS = 15_000 export type CodexModelCatalogProbeDeps = Pick< diff --git a/src/main/codex/codex-state-db-backfill-recovery-process.test.ts b/src/main/codex/codex-state-db-backfill-recovery-process.test.ts new file mode 100644 index 00000000000..7f7659dc537 --- /dev/null +++ b/src/main/codex/codex-state-db-backfill-recovery-process.test.ts @@ -0,0 +1,79 @@ +import { EventEmitter } from 'node:events' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { ChildProcessHandle } from '../../shared/child-process/process-spec' +import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../provider-process/provider-process-supervisor' +import { terminateProviderProcessTree } from '../provider-process/provider-process-teardown' +import { stopCodexBackfillRecoveryProcess } from './codex-state-db-backfill-recovery-process' + +vi.mock('../provider-process/provider-process-teardown', () => ({ + terminateProviderProcessTree: vi.fn(async () => 'exited' as const) +})) + +type FakeSupervisor = EventEmitter & { + pid: number + exitCode: number | null + signalCode: NodeJS.Signals | null + stdin: { end: ReturnType } + kill: ReturnType boolean>> +} + +function fakeSupervisor(exitsOnStdinEnd: boolean): FakeSupervisor { + const child: FakeSupervisor = Object.assign(new EventEmitter(), { + pid: 4242, + exitCode: null, + signalCode: null, + stdin: { + end: vi.fn(() => { + if (exitsOnStdinEnd) { + child.exitCode = 0 + child.emit('exit', 0, null) + } + }) + }, + kill: vi.fn(() => true) + }) + return child +} + +function asHandle(child: FakeSupervisor): ChildProcessHandle { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the stop reads only pid, exit state, stdin, kill and the exit event, which the fake implements. + return child as unknown as ChildProcessHandle +} + +afterEach(() => { + vi.useRealTimers() + vi.clearAllMocks() +}) + +describe('stopCodexBackfillRecoveryProcess for a supervised app-server', () => { + it('ends its input and leaves the group stop to the supervisor', async () => { + const child = fakeSupervisor(true) + + await stopCodexBackfillRecoveryProcess(asHandle(child), true) + + expect(child.stdin.end).toHaveBeenCalledOnce() + // A direct SIGTERM would skip the stdin-end grace a Codex close gets. + expect(child.kill).not.toHaveBeenCalled() + expect(terminateProviderProcessTree).not.toHaveBeenCalled() + }) + + it('tears the tree down only once the supervisor has had its full stop time', async () => { + vi.useFakeTimers() + const child = fakeSupervisor(false) + + const stopped = stopCodexBackfillRecoveryProcess(asHandle(child), true) + await vi.advanceTimersByTimeAsync(PROVIDER_SUPERVISOR_MAX_STOP_MS - 1) + expect(terminateProviderProcessTree).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1) + expect(terminateProviderProcessTree).toHaveBeenCalledWith(child, { + site: 'codex-state-db-backfill-recovery' + }) + // The shared close then waits, bounded, for the forced root's exit. + await vi.advanceTimersByTimeAsync(1_000) + await stopped + + // The stop signals nothing itself: the stdin end asks, and only the teardown forces. + expect(child.stdin.end).toHaveBeenCalledOnce() + expect(child.kill).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/codex/codex-state-db-backfill-recovery-process.ts b/src/main/codex/codex-state-db-backfill-recovery-process.ts new file mode 100644 index 00000000000..77bd3e53732 --- /dev/null +++ b/src/main/codex/codex-state-db-backfill-recovery-process.ts @@ -0,0 +1,75 @@ +import type { ChildProcessHandle } from '../../shared/child-process/process-spec' +import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' +import { parseWslUncPath } from '../../shared/wsl-paths' +import { buildWslCodexAppServerArgs } from '../codex-accounts/wsl-codex-command' +import { CODEX_READ_ONLY_APP_SERVER_ARGS } from '../codex-cli/codex-read-only-app-server-args' +import { terminateCodexProbeChild } from '../rate-limits/codex-probe-termination' +import { + createProviderSpawnSpec, + type ProviderCloseRequest +} from '../provider-process/provider-process-supervisor' +import type { CodexAppServerSpawn } from './codex-app-server-process-tree-kill' +import { stopSupervisedChildProcess } from '../provider-process/supervised-child-process-stop' + +const BACKFILL_RECOVERY_KILL_SITE = 'codex-state-db-backfill-recovery' +// Codex drains on its stdin end, as the Codex connection's root-only close asks; one value +// for the spawn spec (a gone Orca) and the stop (Orca stopping it), so they cannot drift. +const BACKFILL_RECOVERY_CLOSE_REQUEST: ProviderCloseRequest = 'stdin-end' + +export type CodexBackfillRecoveryProcess = { child: ChildProcessHandle; supervised: boolean } + +/** Starts the read-only app-server whose presence lets Codex finish its own backfill. */ +export function spawnCodexBackfillRecoveryProcess( + codexHomePath: string, + spawnProcess: CodexAppServerSpawn, + resolveCommand: () => string +): CodexBackfillRecoveryProcess { + const wslHome = process.platform === 'win32' ? parseWslUncPath(codexHomePath) : null + if (wslHome) { + const child = spawnProcess( + 'wsl.exe', + buildWslCodexAppServerArgs( + wslHome.distro, + wslHome.linuxPath, + CODEX_READ_ONLY_APP_SERVER_ARGS + ), + { + stdio: ['pipe', 'ignore', 'ignore'], + windowsHide: true, + env: process.env + } + ) + return { child, supervised: false } + } + const command = resolveCommand() + // Kept alive up to an hour; on POSIX the supervisor stops it with Orca, however Orca exits. + const spawnSpec = createProviderSpawnSpec( + { command, args: [...CODEX_READ_ONLY_APP_SERVER_ARGS], cwd: codexHomePath }, + withCliRuntimeOnPath(command, { ...process.env, CODEX_HOME: codexHomePath }), + process.platform, + { lifetime: 'session', closeRequest: BACKFILL_RECOVERY_CLOSE_REQUEST } + ) + const child = spawnProcess(spawnSpec.program, spawnSpec.args, { + cwd: codexHomePath, + stdio: ['pipe', 'ignore', 'ignore'], + windowsHide: true, + env: spawnSpec.env, + ...(spawnSpec.supervised ? { detached: true } : {}) + }) + return { child, supervised: spawnSpec.supervised } +} + +export async function stopCodexBackfillRecoveryProcess( + child: ChildProcessHandle, + supervised: boolean +): Promise { + if (supervised) { + // A session supervisor turns stdin end into its group stop, as a Codex connection close does. + await stopSupervisedChildProcess(child, { + site: BACKFILL_RECOVERY_KILL_SITE, + closeRequest: BACKFILL_RECOVERY_CLOSE_REQUEST + }) + return + } + await terminateCodexProbeChild(child) +} diff --git a/src/main/codex/codex-state-db-backfill-recovery-supervision.test.ts b/src/main/codex/codex-state-db-backfill-recovery-supervision.test.ts new file mode 100644 index 00000000000..cc3277ecd37 --- /dev/null +++ b/src/main/codex/codex-state-db-backfill-recovery-supervision.test.ts @@ -0,0 +1,146 @@ +import { ChildProcess } from 'node:child_process' +import { existsSync, readFileSync } from 'node:fs' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { CODEX_READ_ONLY_APP_SERVER_ARGS } from '../codex-cli/codex-read-only-app-server-args' +import { + PROVIDER_STDIN_END_GRACE_MS, + PROVIDER_SUPERVISOR_MAX_STOP_MS +} from '../provider-process/provider-process-supervisor' +import type { CodexAppServerSpawn } from './codex-app-server-process-tree-kill' +import type { CodexStateDbBackfillStatus } from './codex-state-db' +import { runCodexStateDbBackfillRecovery } from './codex-state-db-backfill-recovery' +import { + alive, + createSupervisedProbeRig, + waitFor, + type SupervisedProbeRig +} from './supervised-probe-owner.test-fixture' + +const INCOMPLETE: CodexStateDbBackfillStatus = { + kind: 'incomplete', + stateDbPath: '/state.sqlite', + status: 'running' +} + +const OWNER = String.raw` +import { runCodexStateDbBackfillRecovery } from './codex-state-db-backfill-recovery' +void runCodexStateDbBackfillRecovery(process.env.ORCA_TEST_CODEX_HOME!, new AbortController().signal, { + resolveCommand: () => process.env.ORCA_TEST_STAND_IN!, + readStatus: () => ({ kind: 'incomplete', stateDbPath: '/state.sqlite', status: 'running' }) +}).catch(() => {}) +setInterval(() => {}, 60_000) +` + +const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') +let rig: SupervisedProbeRig | null = null + +afterEach(() => { + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform) + } + vi.unstubAllEnvs() + rig?.cleanup() + rig = null +}) + +function decodedSupervisorSpec(env: unknown): unknown { + const encoded = + typeof env === 'object' && env !== null && 'ORCA_PROVIDER_SUPERVISOR_SPEC' in env + ? String(env.ORCA_PROVIDER_SUPERVISOR_SPEC) + : '' + return JSON.parse(Buffer.from(encoded, 'base64').toString() || 'null') +} + +describe('Codex backfill recovery app-server supervision', () => { + it('runs the POSIX recovery app-server under a session-lifetime supervisor', async () => { + Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) + // Never signalled: terminate is a stub here, and a handle-less ChildProcess must not be killed. + const child = new ChildProcess() + const calls: { program: string; args: string[]; options: Record }[] = [] + const spawnProcess: CodexAppServerSpawn = (program, args, options) => { + calls.push({ program, args, options }) + return child + } + const terminate = vi.fn(async () => {}) + const readStatus = vi + .fn() + .mockReturnValueOnce(INCOMPLETE) + .mockReturnValue({ kind: 'complete', stateDbPath: '/state.sqlite' }) + + await expect( + runCodexStateDbBackfillRecovery('/managed-home', new AbortController().signal, { + spawnProcess, + resolveCommand: () => '/bin/codex', + readStatus, + terminate, + sleep: vi.fn(async () => {}), + now: vi.fn(() => 1_000) + }) + ).resolves.toEqual({ outcome: 'completed', spawnCount: 1 }) + + const [{ program, args, options }] = calls + expect(program).toBe(process.execPath) + expect(args.slice(args.indexOf('--') + 1)).toEqual([ + '/bin/codex', + ...CODEX_READ_ONLY_APP_SERVER_ARGS + ]) + expect(options).toMatchObject({ cwd: '/managed-home', detached: true }) + expect(options.env).toMatchObject({ CODEX_HOME: '/managed-home' }) + expect(decodedSupervisorSpec(options.env)).toMatchObject({ + lifetime: 'session', + cwd: '/managed-home' + }) + expect(terminate).toHaveBeenCalledWith(child, true) + }) +}) + +describe.runIf(process.platform !== 'win32')('supervised Codex backfill recovery processes', () => { + it('stops the whole recovery group when the recovery is aborted', async () => { + rig = createSupervisedProbeRig() + for (const [key, value] of Object.entries(rig.env)) { + vi.stubEnv(key, value) + } + const standIn = rig.writeStandIn('codex') + const controller = new AbortController() + + const recovery = runCodexStateDbBackfillRecovery(rig.dir, controller.signal, { + resolveCommand: () => standIn, + readStatus: () => INCOMPLETE + }) + const pids = await rig.readPids() + controller.abort() + + await expect(recovery).resolves.toEqual({ outcome: 'stopped', spawnCount: 1 }) + // Its stdin end comes first; the supervisor then SIGTERMs, and SIGKILLs the group after. + expect(existsSync(rig.signalFile) && readFileSync(rig.signalFile, 'utf8')).toBe('SIGTERM') + expect(alive(pids.provider)).toBe(false) + // A SIGTERM to the root alone left this descendant running for good. + expect(alive(pids.grandchild)).toBe(false) + }) + + it('drains, then stops the recovery group, when its owner is SIGKILLed', async () => { + rig = createSupervisedProbeRig() + const standIn = rig.writeStandIn('codex') + const bundle = await rig.bundleOwner(OWNER, __dirname) + const owner = rig.launchOwner(bundle, { + ORCA_TEST_STAND_IN: standIn, + ORCA_TEST_CODEX_HOME: rig.dir + }) + const pids = await rig.readPids() + + owner.kill('SIGKILL') + + expect( + await waitFor( + () => !alive(pids.provider) && !alive(pids.grandchild), + PROVIDER_SUPERVISOR_MAX_STOP_MS + 1_000 + ) + ).toBe(true) + // A gone owner gets the close its owner makes: the stdin end, its grace, and only then SIGTERM. + const events = rig.readEvents() + expect(events['stdin-end']).toBeDefined() + expect((events.SIGTERM ?? 0) - (events['stdin-end'] ?? 0)).toBeGreaterThanOrEqual( + PROVIDER_STDIN_END_GRACE_MS - 50 + ) + }) +}) diff --git a/src/main/codex/codex-state-db-backfill-recovery.test.ts b/src/main/codex/codex-state-db-backfill-recovery.test.ts index a76c5f65a5a..6577bf3bd53 100644 --- a/src/main/codex/codex-state-db-backfill-recovery.test.ts +++ b/src/main/codex/codex-state-db-backfill-recovery.test.ts @@ -357,7 +357,7 @@ describe('Codex state DB backfill recovery', () => { expect(spawnProcess).toHaveBeenCalledTimes(2) expect(timerDurations).toEqual([5_000, 2_000, 5_000]) expect(terminate).toHaveBeenCalledTimes(1) - expect(terminate).toHaveBeenCalledWith(children[1]) + expect(terminate).toHaveBeenCalledWith(children[1], process.platform !== 'win32') }) it('keeps the successful app-server claimant alive until Codex marks its DB complete', async () => { @@ -377,7 +377,7 @@ describe('Codex state DB backfill recovery', () => { now: vi.fn(() => 1_000) }) ).resolves.toEqual({ outcome: 'completed', spawnCount: 1 }) - expect(terminate).toHaveBeenCalledWith(child) + expect(terminate).toHaveBeenCalledWith(child, process.platform !== 'win32') }) it('hands the Codex shim itself to the spawn chokepoint for native Windows recovery', async () => { @@ -389,6 +389,7 @@ describe('Codex state DB backfill recovery', () => { .fn() .mockReturnValueOnce({ kind: 'incomplete', stateDbPath: 'state.sqlite', status: 'running' }) .mockReturnValue({ kind: 'complete', stateDbPath: 'state.sqlite' }) + const terminate = vi.fn(async () => {}) await runCodexStateDbBackfillRecovery( 'C:\\Users\\alice\\.codex', @@ -397,7 +398,7 @@ describe('Codex state DB backfill recovery', () => { spawnProcess: spawnProcess as never, resolveCommand: () => codexCommand, readStatus, - terminate: vi.fn(async () => {}), + terminate, sleep: vi.fn(async () => {}), now: vi.fn(() => 1_000) } @@ -421,6 +422,9 @@ describe('Codex state DB backfill recovery', () => { ]) expect(spawnOptions.cwd).toBe('C:\\Users\\alice\\.codex') expect(spawnOptions.env?.CODEX_HOME).toBe('C:\\Users\\alice\\.codex') + // Windows has no supervisor: the drain-first probe termination stays its stop. + expect(spawnOptions).not.toHaveProperty('detached') + expect(terminate).toHaveBeenCalledWith(child, false) }) it('retries a live foreign lease until one durable claimant can recover it', async () => { diff --git a/src/main/codex/codex-state-db-backfill-recovery.ts b/src/main/codex/codex-state-db-backfill-recovery.ts index b4acef3b337..85635bf0e6a 100644 --- a/src/main/codex/codex-state-db-backfill-recovery.ts +++ b/src/main/codex/codex-state-db-backfill-recovery.ts @@ -5,16 +5,16 @@ import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-p import { parseWslUncPath } from '../../shared/wsl-paths' import { withManagedHookInstallLock } from '../agent-hooks/managed-hook-install-lock' import { readManagedHookHostIdentity } from '../agent-hooks/managed-hook-owner-identity' -import { buildWslCodexAppServerArgs } from '../codex-accounts/wsl-codex-command' import { resolveCodexCommand } from '../codex-cli/command' -import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' -import { CODEX_READ_ONLY_APP_SERVER_ARGS } from '../codex-cli/codex-read-only-app-server-args' -import { terminateCodexProbeChild } from '../rate-limits/codex-probe-termination' import type { ChildProcessHandle } from '../../shared/child-process/process-spec' import { spawnCodexAppServerProcess, type CodexAppServerSpawn } from './codex-app-server-process-tree-kill' +import { + spawnCodexBackfillRecoveryProcess, + stopCodexBackfillRecoveryProcess +} from './codex-state-db-backfill-recovery-process' import { getOrcaUserDataPath } from './codex-home-paths' import { BACKFILL_PENDING_MIN_SESSION_FILES, @@ -43,7 +43,7 @@ type RecoveryDependencies = { countSessions: (sessionsRoot: string, limit: number) => number now: () => number sleep: (ms: number, signal: AbortSignal) => Promise - terminate: (child: ChildProcessHandle) => Promise + terminate: (child: ChildProcessHandle, supervised: boolean) => Promise } const defaultDependencies: RecoveryDependencies = { @@ -53,7 +53,7 @@ const defaultDependencies: RecoveryDependencies = { countSessions: countCodexSessionFilesUpTo, now: Date.now, sleep: async (ms, signal) => await delay(ms, undefined, { signal }), - terminate: async (child) => await terminateCodexProbeChild(child) + terminate: stopCodexBackfillRecoveryProcess } function finish( @@ -86,35 +86,6 @@ function initialRecoveryDecision( return null } -function spawnRecoveryProcess( - codexHomePath: string, - dependencies: RecoveryDependencies -): ChildProcessHandle { - const wslHome = process.platform === 'win32' ? parseWslUncPath(codexHomePath) : null - if (wslHome) { - return dependencies.spawnProcess( - 'wsl.exe', - buildWslCodexAppServerArgs( - wslHome.distro, - wslHome.linuxPath, - CODEX_READ_ONLY_APP_SERVER_ARGS - ), - { - stdio: ['pipe', 'ignore', 'ignore'], - windowsHide: true, - env: process.env - } - ) - } - const command = dependencies.resolveCommand() - return dependencies.spawnProcess(command, [...CODEX_READ_ONLY_APP_SERVER_ARGS], { - cwd: codexHomePath, - stdio: ['pipe', 'ignore', 'ignore'], - windowsHide: true, - env: withCliRuntimeOnPath(command, { ...process.env, CODEX_HOME: codexHomePath }) - }) -} - /** Keeps a sanctioned app-server claimant alive until Codex completes its own backfill. */ export async function runCodexStateDbBackfillRecovery( codexHomePath: string, @@ -130,7 +101,12 @@ export async function runCodexStateDbBackfillRecovery( const deadline = dependencies.now() + RECOVERY_MAX_TOTAL_MS let spawnCount = 0 while (!signal.aborted && dependencies.now() < deadline && spawnCount < RECOVERY_MAX_SPAWNS) { - const child = spawnRecoveryProcess(codexHomePath, dependencies) + const { child, supervised } = spawnCodexBackfillRecoveryProcess( + codexHomePath, + dependencies.spawnProcess, + dependencies.resolveCommand + ) + const terminate = (): Promise => dependencies.terminate(child, supervised) spawnCount += 1 let childDown = false child.once('error', () => { @@ -145,27 +121,27 @@ export async function runCodexStateDbBackfillRecovery( await dependencies.sleep(RECOVERY_POLL_INTERVAL_MS, signal) const status = dependencies.readStatus(codexHomePath) if (status.kind === 'complete') { - await dependencies.terminate(child) + await terminate() return finish('completed', spawnCount) } if (status.kind === 'unreadable') { - await dependencies.terminate(child) + await terminate() return finish('unreadable', spawnCount) } } } catch (error) { if (!signal.aborted) { - await dependencies.terminate(child) + await terminate() throw error } } if (signal.aborted) { - await dependencies.terminate(child) + await terminate() return finish('stopped', spawnCount) } if (!childDown) { - await dependencies.terminate(child) + await terminate() return finish('gave-up', spawnCount) } if (spawnCount >= RECOVERY_MAX_SPAWNS) { diff --git a/src/main/codex/supervised-probe-owner.test-fixture.ts b/src/main/codex/supervised-probe-owner.test-fixture.ts new file mode 100644 index 00000000000..e5d9b647e00 --- /dev/null +++ b/src/main/codex/supervised-probe-owner.test-fixture.ts @@ -0,0 +1,151 @@ +import { spawn, type ChildProcess } from 'node:child_process' +import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { build } from 'esbuild' + +// A stand-in agent CLI that, like `opencode serve` or a wedged Codex, ignores its stdin end. It +// leaves a SIGTERM-ignoring grandchild in its group, so only a group SIGKILL ends everything, and +// records its stdin end and a SIGTERM (exiting on the latter). `body` adds the caller's protocol. +function standInSource(body: string): string { + return String.raw` +const { spawn } = require('node:child_process') +const fs = require('node:fs') +const record = (event) => + fs.appendFileSync(process.env.ORCA_TEST_EVENTS_FILE, JSON.stringify({ event, at: Date.now() }) + '\n') +process.on('SIGTERM', () => { + record('SIGTERM') + fs.writeFileSync(process.env.ORCA_TEST_SIGNAL_FILE, 'SIGTERM') + process.exit(0) +}) +process.stdin.on('error', () => {}).on('end', () => record('stdin-end')).resume() +const grandchild = spawn( + process.execPath, + ['-e', "process.on('SIGTERM', () => {}); process.stdout.write('armed'); setInterval(() => {}, 60000)"], + { stdio: ['ignore', 'pipe', 'ignore'] } +) +grandchild.stdout.once('data', () => { + // Renamed into place, so a reader never sees a half-written file. + const partial = process.env.ORCA_TEST_PID_FILE + '.partial' + fs.writeFileSync(partial, JSON.stringify({ provider: process.pid, grandchild: grandchild.pid })) + fs.renameSync(partial, process.env.ORCA_TEST_PID_FILE) +}) +setInterval(() => {}, 60000) +${body} +` +} + +export type StandInEvent = { event: 'stdin-end' | 'SIGTERM'; at: number } + +export type SupervisedProbeRig = { + dir: string + pidFile: string + signalFile: string + /** When the stand-in saw its stdin end and its SIGTERM, by its own clock (this host's). */ + readEvents: () => Partial> + env: Record + /** An executable stand-in, run as ` ` like a real agent CLI. */ + writeStandIn: (name: string, body?: string) => string + /** Bundles a TypeScript owner entry, resolved from `resolveDir`, into one plain-Node file. */ + bundleOwner: (source: string, resolveDir: string) => Promise + launchOwner: (bundle: string, env?: Record) => ChildProcess + readPids: (timeoutMs?: number) => Promise<{ provider: number; grandchild: number }> + cleanup: () => void +} + +export function alive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch (error) { + return !(error instanceof Error && 'code' in error && error.code === 'ESRCH') + } +} + +export async function waitFor(predicate: () => boolean, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs + while (!predicate()) { + if (Date.now() >= deadline) { + return false + } + await new Promise((resolve) => setTimeout(resolve, 25)) + } + return true +} + +export function createSupervisedProbeRig(): SupervisedProbeRig { + const dir = mkdtempSync(join(tmpdir(), 'orca-supervised-probe-')) + const pidFile = join(dir, 'pids.json') + const signalFile = join(dir, 'signal') + const eventsFile = join(dir, 'events.jsonl') + const recordedPids = new Set() + const env = { + ORCA_TEST_PID_FILE: pidFile, + ORCA_TEST_SIGNAL_FILE: signalFile, + ORCA_TEST_EVENTS_FILE: eventsFile + } + return { + dir, + pidFile, + signalFile, + readEvents: () => { + const events: Partial> = {} + const lines = existsSync(eventsFile) ? readFileSync(eventsFile, 'utf8').split('\n') : [] + for (const line of lines.filter(Boolean)) { + const { event, at }: StandInEvent = JSON.parse(line) + events[event] ??= at + } + return events + }, + env, + writeStandIn: (name, body = '') => { + const script = join(dir, `${name}.cjs`) + writeFileSync(script, standInSource(body)) + const launcher = join(dir, name) + writeFileSync(launcher, `#!/bin/sh\nexec "${process.execPath}" "${script}" "$@"\n`) + chmodSync(launcher, 0o755) + return launcher + }, + bundleOwner: async (source, resolveDir) => { + const outfile = join(dir, 'owner.cjs') + await build({ + stdin: { contents: source, resolveDir, loader: 'ts', sourcefile: 'owner.ts' }, + bundle: true, + platform: 'node', + format: 'cjs', + target: 'node22', + outfile, + logLevel: 'silent' + }) + return outfile + }, + launchOwner: (bundle, ownerEnv = {}) => { + const owner = spawn(process.execPath, [bundle], { + env: { ...process.env, ...env, ...ownerEnv }, + stdio: 'ignore' + }) + recordedPids.add(owner.pid!) + return owner + }, + readPids: async (timeoutMs = 20_000) => { + if (!(await waitFor(() => existsSync(pidFile), timeoutMs))) { + throw new Error('the stand-in never reported its pids') + } + const pids: { provider: number; grandchild: number } = JSON.parse( + readFileSync(pidFile, 'utf8') + ) + recordedPids.add(pids.provider) + recordedPids.add(pids.grandchild) + return pids + }, + cleanup: () => { + // Only pids this rig recorded, so a failed run never leaves a stand-in behind. + for (const pid of recordedPids) { + if (alive(pid)) { + process.kill(pid, 'SIGKILL') + } + } + rmSync(dir, { recursive: true, force: true }) + } + } +} diff --git a/src/main/opencode/opencode-launch-model-context-cleanup.test.ts b/src/main/opencode/opencode-launch-model-context-cleanup.test.ts index 4acf1ba72de..cf5df3a7d7f 100644 --- a/src/main/opencode/opencode-launch-model-context-cleanup.test.ts +++ b/src/main/opencode/opencode-launch-model-context-cleanup.test.ts @@ -9,12 +9,17 @@ import { } from '../../shared/child-process/process-tree-termination' import { probeOpenCodeLaunchModelContext } from './opencode-launch-model-context' import { readFetchResponseJsonWithinLimit } from '../../shared/fetch-response-body' +import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../provider-process/provider-process-supervisor' +import { terminateProviderProcessTree } from '../provider-process/provider-process-teardown' vi.mock('../../shared/child-process/run-process', () => ({ spawnProcess: vi.fn() })) vi.mock('../../shared/child-process/process-tree-termination', () => ({ signalProcessTree: vi.fn(), forceTerminateProcessTree: vi.fn() })) +vi.mock('../provider-process/provider-process-teardown', () => ({ + terminateProviderProcessTree: vi.fn() +})) vi.mock('../../shared/fetch-response-body', () => ({ readFetchResponseJsonWithinLimit: vi.fn() })) @@ -24,6 +29,9 @@ class ProbeChild extends ChildProcess { override stderr = new PassThrough() override pid = 123456 override exitCode: number | null = null + override signalCode: NodeJS.Signals | null = null + // A handle-less ChildProcess's own kill can signal this test's process group. + override kill = vi.fn((_signal?: NodeJS.Signals | number): boolean => true) override stdio: [PassThrough, PassThrough, PassThrough, undefined, undefined] = [ this.stdin, this.stdout, @@ -52,11 +60,33 @@ function close() { child.emit('exit', 0, null) child.emit('close', 0, null) } +/** How a supervisor ends: its stop signal re-raised once the group is gone, or an exit code. */ +function supervisorExits(code: number | null, signal: NodeJS.Signals | null) { + child.exitCode = code + child.signalCode = signal + child.emit('exit', code, signal) + child.emit('close', code, signal) +} const options = { executable: '/private/opencode', cwd: directory, env: {} } +function decodedSupervisorSpec(env: unknown): unknown { + const encoded = + typeof env === 'object' && env !== null && 'ORCA_PROVIDER_SUPERVISOR_SPEC' in env + ? String(env.ORCA_PROVIDER_SUPERVISOR_SPEC) + : '' + return JSON.parse(Buffer.from(encoded, 'base64').toString() || 'null') +} + +async function untilStopRequested(): Promise { + while (child.kill.mock.calls.length === 0) { + await new Promise((resolve) => setImmediate(resolve)) + } +} + describe('OpenCode model probe termination evidence', () => { beforeEach(() => { vi.resetAllMocks() + Object.defineProperty(process, 'platform', { value: 'darwin' }) vi.mocked(readFetchResponseJsonWithinLimit).mockImplementation((response) => response.json()) child = new ProbeChild() closeDuringFetch = false @@ -82,9 +112,16 @@ describe('OpenCode model probe termination evidence', () => { return true }) vi.mocked(forceTerminateProcessTree).mockResolvedValue(false) + child.kill.mockImplementation((signal) => { + if (signal === 'SIGTERM') { + supervisorExits(null, 'SIGTERM') + } + return true + }) }) afterEach(() => { + vi.useRealTimers() if (platform) { Object.defineProperty(process, 'platform', platform) } @@ -100,7 +137,7 @@ describe('OpenCode model probe termination evidence', () => { ) expect(await probeOpenCodeLaunchModelContext(options)).toBeNull() expect(cancel).toHaveBeenCalledTimes(4) - expect(signalProcessTree).toHaveBeenCalledOnce() + expect(child.kill).toHaveBeenCalledWith('SIGTERM') }) it('accepts an already-closed Windows probe without signaling its former pid', async () => { @@ -112,14 +149,103 @@ describe('OpenCode model probe termination evidence', () => { expect(forceTerminateProcessTree).not.toHaveBeenCalled() }) - it('does not force a POSIX group after the graceful close', async () => { - Object.defineProperty(process, 'platform', { value: 'darwin' }) + it('runs a POSIX probe under a one-shot supervisor and stops it through that supervisor', async () => { expect(await probeOpenCodeLaunchModelContext(options)).toMatchObject({ primaryAgent: 'build' }) - expect(signalProcessTree).toHaveBeenCalledOnce() + + const [spec] = vi.mocked(spawnProcess).mock.calls[0] + expect(spec).toMatchObject({ program: process.execPath, cwd: directory, detached: true }) + const args = spec.args ?? [] + expect(args.slice(args.indexOf('--') + 1)).toEqual([ + '/private/opencode', + 'serve', + '--hostname', + '127.0.0.1', + '--port', + '0' + ]) + // Its stdin ends at once, so a session lifetime would stop the server a second later. + expect(decodedSupervisorSpec(spec.env)).toMatchObject({ lifetime: 'one-shot', cwd: directory }) + expect(child.kill.mock.calls).toEqual([['SIGTERM']]) + // Signalling the supervisor's own group, or SIGKILLing it, would orphan the server's group. + expect(signalProcessTree).not.toHaveBeenCalled() expect(forceTerminateProcessTree).not.toHaveBeenCalled() + expect(terminateProviderProcessTree).not.toHaveBeenCalled() }) - it('keeps a root exit with inherited pipes unverified and avoids its former pid', async () => { + it('forces a POSIX supervisor tree only after its full stop time, and trusts no forced stop', async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + child.kill.mockReturnValue(true) + // A teardown that found no descendants to check proves nothing about the server's group. + vi.mocked(terminateProviderProcessTree).mockImplementation(async () => { + supervisorExits(null, 'SIGKILL') + return null + }) + + const probe = probeOpenCodeLaunchModelContext(options) + await untilStopRequested() + await vi.advanceTimersByTimeAsync(PROVIDER_SUPERVISOR_MAX_STOP_MS - 1) + expect(terminateProviderProcessTree).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1) + + expect(await probe).toBeNull() + expect(terminateProviderProcessTree).toHaveBeenCalledWith(child, { + site: 'opencode-launch-model-preflight' + }) + }) + + it('trusts no forced POSIX stop, even one whose teardown reports the tree gone', async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + child.kill.mockReturnValue(true) + // 'exited' covers the supervisor's tree only; the server leads its own detached group. + vi.mocked(terminateProviderProcessTree).mockImplementation(async () => { + supervisorExits(null, 'SIGKILL') + return 'exited' + }) + + const probe = probeOpenCodeLaunchModelContext(options) + await untilStopRequested() + await vi.advanceTimersByTimeAsync(PROVIDER_SUPERVISOR_MAX_STOP_MS) + + expect(await probe).toBeNull() + }) + + it('trusts no POSIX stop whose supervisor exited 1, which a failed group reap also exits', async () => { + child.kill.mockImplementation(() => { + supervisorExits(1, null) + return true + }) + + expect(await probeOpenCodeLaunchModelContext(options)).toBeNull() + expect(terminateProviderProcessTree).not.toHaveBeenCalled() + }) + + it('accepts a POSIX supervisor that relayed the server exiting on its own', async () => { + child.kill.mockImplementation(() => { + supervisorExits(0, null) + return true + }) + + expect(await probeOpenCodeLaunchModelContext(options)).toMatchObject({ primaryAgent: 'build' }) + }) + + it('never forces a POSIX supervisor whose root exited without closing its pipes', async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + child.kill.mockImplementation(() => { + child.exitCode = 0 + child.emit('exit', 0, null) + return true + }) + + const probe = probeOpenCodeLaunchModelContext(options) + await untilStopRequested() + await vi.advanceTimersByTimeAsync(PROVIDER_SUPERVISOR_MAX_STOP_MS) + + expect(await probe).toBeNull() + expect(terminateProviderProcessTree).not.toHaveBeenCalled() + }) + + it('keeps a Windows root exit with inherited pipes unverified and avoids its former pid', async () => { + Object.defineProperty(process, 'platform', { value: 'win32' }) vi.mocked(signalProcessTree).mockImplementation(async () => { child.exitCode = 0 child.emit('exit', 0, null) @@ -129,7 +255,8 @@ describe('OpenCode model probe termination evidence', () => { expect(forceTerminateProcessTree).not.toHaveBeenCalled() }) - it('requires verified termination when a still-live probe does not close', async () => { + it('requires verified termination when a still-live Windows probe does not close', async () => { + Object.defineProperty(process, 'platform', { value: 'win32' }) vi.mocked(signalProcessTree).mockResolvedValue(true) expect(await probeOpenCodeLaunchModelContext(options)).toBeNull() expect(forceTerminateProcessTree).toHaveBeenCalledOnce() diff --git a/src/main/opencode/opencode-launch-model-context-supervision.test.ts b/src/main/opencode/opencode-launch-model-context-supervision.test.ts new file mode 100644 index 00000000000..2ffa6d7a891 --- /dev/null +++ b/src/main/opencode/opencode-launch-model-context-supervision.test.ts @@ -0,0 +1,96 @@ +import { existsSync, readFileSync } from 'node:fs' +import { afterEach, describe, expect, it } from 'vitest' +import { PROVIDER_SUPERVISOR_MAX_STOP_MS } from '../provider-process/provider-process-supervisor' +import { + alive, + createSupervisedProbeRig, + waitFor, + type SupervisedProbeRig +} from '../codex/supervised-probe-owner.test-fixture' +import { probeOpenCodeLaunchModelContext } from './opencode-launch-model-context' + +// Like `opencode serve`, it never exits on stdin end. It starts listening only after the session +// stdin-end grace, so a supervisor that treated the probe's stdin end as a stop would kill it first. +const SERVE = String.raw` +setTimeout(() => { + const server = require('node:http').createServer((request, response) => { + const url = new URL(request.url, 'http://127.0.0.1') + const location = { directory: url.searchParams.get('location[directory]') } + const model = { id: 'model', providerID: 'provider', enabled: true } + const bodies = { + '/api/model': { location, data: [model] }, + '/api/agent': { location, data: [{ id: 'build', mode: 'primary', hidden: false }] }, + '/api/model/default': { location, data: model }, + '/api/config': [{ type: 'document', info: { default_agent: 'build' } }] + } + response.setHeader('content-type', 'application/json') + response.end(JSON.stringify(bodies[url.pathname])) + }) + server.listen(0, '127.0.0.1', () => { + process.stdout.write('server listening on http://127.0.0.1:' + server.address().port + '\n') + }) +}, 1500) +` + +const OWNER = String.raw` +import { probeOpenCodeLaunchModelContext } from './opencode-launch-model-context' +void probeOpenCodeLaunchModelContext({ + executable: process.env.ORCA_TEST_STAND_IN!, + cwd: process.cwd(), + env: process.env +}).catch(() => {}) +setInterval(() => {}, 60_000) +` + +let rig: SupervisedProbeRig | null = null + +afterEach(() => { + rig?.cleanup() + rig = null +}) + +describe.runIf(process.platform !== 'win32')('supervised OpenCode launch-context probe', () => { + it('reads the context after its stdin end, then stops the whole server group', async () => { + rig = createSupervisedProbeRig() + const executable = rig.writeStandIn('opencode', SERVE) + + const context = await probeOpenCodeLaunchModelContext({ + executable, + cwd: rig.dir, + env: { ...process.env, ...rig.env } + }) + const stoppedAt = Date.now() + const pids = await rig.readPids() + + expect(context).toEqual({ + primaryAgent: 'build', + availableModels: ['provider/model'], + primaryModel: null + }) + expect(existsSync(rig.signalFile) && readFileSync(rig.signalFile, 'utf8')).toBe('SIGTERM') + expect(alive(pids.provider)).toBe(false) + // A descendant that ignores SIGTERM holds none of the probe's pipes, so a closed pipe never + // proved it gone; only the supervisor's group SIGKILL ends it. + expect(alive(pids.grandchild)).toBe(false) + // Once the server exits on its SIGTERM, the rest of its group is killed at once, not after the + // SIGTERM grace; every launch waits on this preflight. + expect(stoppedAt - (rig.readEvents().SIGTERM ?? 0)).toBeLessThan(1_000) + }) + + it('stops the server group when its owner is SIGKILLed mid-probe', async () => { + rig = createSupervisedProbeRig() + const executable = rig.writeStandIn('opencode') + const bundle = await rig.bundleOwner(OWNER, __dirname) + const owner = rig.launchOwner(bundle, { ORCA_TEST_STAND_IN: executable }) + const pids = await rig.readPids() + + owner.kill('SIGKILL') + + expect( + await waitFor( + () => !alive(pids.provider) && !alive(pids.grandchild), + PROVIDER_SUPERVISOR_MAX_STOP_MS + 1_000 + ) + ).toBe(true) + }) +}) diff --git a/src/main/opencode/opencode-launch-model-context.ts b/src/main/opencode/opencode-launch-model-context.ts index ce1098d8acf..134b7b4ab92 100644 --- a/src/main/opencode/opencode-launch-model-context.ts +++ b/src/main/opencode/opencode-launch-model-context.ts @@ -10,6 +10,13 @@ import { withTimeout } from '../../shared/promise-timeout-fallback' import { createOutputSink } from '../../shared/child-process/bounded-output-sink' import { readFetchResponseJsonWithinLimit } from '../../shared/fetch-response-body' import { cancelUnreadResponseBody } from '../lib/unread-response-body' +import type { ChildProcessHandle } from '../../shared/child-process/process-spec' +import { createProviderSpawnSpec } from '../provider-process/provider-process-supervisor' +import { stopSupervisedChildProcess } from '../provider-process/supervised-child-process-stop' + +const OPENCODE_PREFLIGHT_KILL_SITE = 'opencode-launch-model-preflight' +// A supervisor's pipes are its own, so they close as it exits. +const SUPERVISOR_PIPE_CLOSE_MS = 1_000 const model = z.object({ id: z.string(), providerID: z.string() }) const availableModel = model.extend({ enabled: z.boolean() }) @@ -94,15 +101,22 @@ export async function probeOpenCodeLaunchModelContext(options: { expectedPrimaryModel?: string }): Promise { const password = randomBytes(32).toString('base64url') - const child = spawnProcess({ - program: options.executable, - args: ['serve', '--hostname', '127.0.0.1', '--port', '0'], - cwd: options.cwd, - env: { - ...options.env, - OPENCODE_SERVER_USERNAME: 'opencode', - OPENCODE_SERVER_PASSWORD: password + const spawnSpec = createProviderSpawnSpec( + { + command: options.executable, + args: ['serve', '--hostname', '127.0.0.1', '--port', '0'], + cwd: options.cwd }, + { ...options.env, OPENCODE_SERVER_USERNAME: 'opencode', OPENCODE_SERVER_PASSWORD: password }, + process.platform, + // `opencode serve` never exits on stdin end, so on POSIX only its supervisor stops it with Orca. + { lifetime: 'one-shot' } + ) + const child = spawnProcess({ + program: spawnSpec.program, + args: spawnSpec.args, + cwd: options.cwd, + env: spawnSpec.env, detached: true }) const output = createOutputSink(4096) @@ -193,14 +207,42 @@ export async function probeOpenCodeLaunchModelContext(options: { } catch { context = null } finally { - if (!childClosed && !rootExited) { - await signalProcessTree(child, 'SIGTERM') - } - stopped = childClosed || (await withTimeout(closed, 1_500, false)) - // A reaped root's PID can be reused while descendants still hold its pipes. - if (!stopped && !rootExited) { - stopped = await forceTerminateProcessTree(child) + if (spawnSpec.supervised) { + stopped = await stopSupervisedPreflight(child, closed) + } else { + if (!childClosed && !rootExited) { + await signalProcessTree(child, 'SIGTERM') + } + stopped = childClosed || (await withTimeout(closed, 1_500, false)) + // A reaped root's PID can be reused while descendants still hold its pipes. + if (!stopped && !rootExited) { + stopped = await forceTerminateProcessTree(child) + } } } return stopped ? context : null } + +/** + * True only when the server's group is proven gone: the supervisor exited on its own stop or + * relayed the server's exit. A forced stop proves nothing: its teardown reaches the supervisor's + * group, not the server's own detached group. Exit 1 can be a group reap that failed. + */ +async function stopSupervisedPreflight( + child: ChildProcessHandle, + closed: Promise +): Promise { + if (!child.pid) { + // Never started, so there is nothing to stop. + return true + } + const { root, tree } = await stopSupervisedChildProcess(child, { + site: OPENCODE_PREFLIGHT_KILL_SITE + }) + const forced = tree !== null || child.signalCode === 'SIGKILL' + const failedReap = child.signalCode === null && child.exitCode === 1 + if (root !== 'exited' || forced || failedReap) { + return false + } + return withTimeout(closed, SUPERVISOR_PIPE_CLOSE_MS, false) +} diff --git a/src/main/rate-limits/codex-fetcher-auth-errors.test.ts b/src/main/rate-limits/codex-fetcher-auth-errors.test.ts index 5ce9df8675e..20405233c9d 100644 --- a/src/main/rate-limits/codex-fetcher-auth-errors.test.ts +++ b/src/main/rate-limits/codex-fetcher-auth-errors.test.ts @@ -11,6 +11,11 @@ vi.mock('node:child_process', () => ({ spawn: childSpawnMock })) +// The recovery's supervised stop imports the process-table reader, which needs the real execFile. +vi.mock('../codex/codex-state-db-backfill-recovery', () => ({ + startCodexStateDbBackfillRecoveryInBackground: vi.fn() +})) + vi.mock('../codex-cli/command', () => ({ resolveCodexCommand: resolveCodexCommandMock })) diff --git a/src/main/rate-limits/codex-fetcher-backend.test.ts b/src/main/rate-limits/codex-fetcher-backend.test.ts index 3fefe5e3a2f..b1c01a2a6e8 100644 --- a/src/main/rate-limits/codex-fetcher-backend.test.ts +++ b/src/main/rate-limits/codex-fetcher-backend.test.ts @@ -15,6 +15,11 @@ vi.mock('./codex-auth-presence', () => ({ probeCodexAuthPresence: vi.fn(async () => 'present') })) +// The recovery's supervised stop imports the process-table reader, which needs the real execFile. +vi.mock('../codex/codex-state-db-backfill-recovery', () => ({ + startCodexStateDbBackfillRecoveryInBackground: vi.fn() +})) + import { consumeCodexRateLimitResetCredit, fetchCodexRateLimits } from './codex-fetcher' describe('Codex backend rate-limit requests', () => { diff --git a/src/main/rate-limits/codex-fetcher-credit-balance.test.ts b/src/main/rate-limits/codex-fetcher-credit-balance.test.ts index 780687c9a09..016357735e9 100644 --- a/src/main/rate-limits/codex-fetcher-credit-balance.test.ts +++ b/src/main/rate-limits/codex-fetcher-credit-balance.test.ts @@ -14,6 +14,11 @@ vi.mock('./codex-auth-presence', () => ({ probeCodexAuthPresence: vi.fn(async () => 'present') })) +// The recovery's supervised stop imports the process-table reader, which needs the real execFile. +vi.mock('../codex/codex-state-db-backfill-recovery', () => ({ + startCodexStateDbBackfillRecoveryInBackground: vi.fn() +})) + import { fetchCodexRateLimits } from './codex-fetcher' function parseRpcRequest(line: string): { id?: number; method?: string } { diff --git a/src/main/rate-limits/codex-fetcher-probe-shutdown.test.ts b/src/main/rate-limits/codex-fetcher-probe-shutdown.test.ts index d5b2d56b681..791bcf4c7bb 100644 --- a/src/main/rate-limits/codex-fetcher-probe-shutdown.test.ts +++ b/src/main/rate-limits/codex-fetcher-probe-shutdown.test.ts @@ -12,6 +12,11 @@ vi.mock('node:child_process', () => ({ spawn: childSpawnMock })) +// The recovery's supervised stop imports the process-table reader, which needs the real execFile. +vi.mock('../codex/codex-state-db-backfill-recovery', () => ({ + startCodexStateDbBackfillRecoveryInBackground: vi.fn() +})) + vi.mock('node:fs/promises', () => ({ readFile: readFileMock })) diff --git a/src/main/rate-limits/codex-fetcher-session-supplement.test.ts b/src/main/rate-limits/codex-fetcher-session-supplement.test.ts index 232bfc9a83d..44c08613721 100644 --- a/src/main/rate-limits/codex-fetcher-session-supplement.test.ts +++ b/src/main/rate-limits/codex-fetcher-session-supplement.test.ts @@ -14,6 +14,11 @@ vi.mock('./codex-auth-presence', () => ({ probeCodexAuthPresence: vi.fn(async () => 'present') })) +// The recovery's supervised stop imports the process-table reader, which needs the real execFile. +vi.mock('../codex/codex-state-db-backfill-recovery', () => ({ + startCodexStateDbBackfillRecoveryInBackground: vi.fn() +})) + import { fetchCodexRateLimits } from './codex-fetcher' function makeRpcChild(rateLimitResetCredits?: unknown) {