Skip to content

fix(server): process-group kills never reach every process the user owns - #14461

Merged
juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/refuse-pid1-group-kill
Sep 30, 2026
Merged

juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/refuse-pid1-group-kill

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Running the server's provider tests killed every process the developer's user owned on the machine: the T3 server, cloudflared, and every running agent. It happened three times on cups while agents worked on the OpenCode 2 stack.

Several tests fake the process spawner and report every spawned process as pid 1 (ChildProcessSpawner.ProcessId(1), for example in ProviderRegistry.test.ts). The OpenCode runtime cleans up a command by killing its process group with process.kill(-pid, "SIGKILL"). With pid 1 that is kill(-1, SIGKILL), which on POSIX signals every process the caller is allowed to signal. A pid of 0 would signal the server's own group.

Fix

process/processGroup.ts adds signalProcessGroup(pid, signal). It refuses a pid of 0 or 1, or one that is not an integer, by throwing ESRCH, the same error a group that has already exited gives, so every caller's existing handling stays the same. Every process-group signal in the server now goes through it:

  • the OpenCode runtime's command and server cleanup (opencodeRuntime.ts);
  • the OpenCode server ledger's stop and liveness probe;
  • the ACP runtime's owned process group;
  • Pi's kill and liveness probe.

A real child's pid is never 0 or 1, so production behaviour doesn't change.

Verification

  • processGroup.test.ts: pids 1, 0, −1, NaN and 1.5 are refused with ESRCH, using signal 0 only, so the test is harmless even without the guard; a real spawned group is still killed. Without the guard the first test fails (pid 1: expected undefined to be 'ESRCH'). Both runs were inside a user and PID namespace (unshare -U --map-current-user -p -f --mount-proc), so nothing outside could be signalled.
  • The fake-pid-1 test files pass in that sandbox: ProviderRegistry, CodexDriver, providerMaintenanceRunner, providerSnapshot.
  • OpenCodeServerLedger.test.ts and AcpSessionRuntime.processTree.test.ts pass outside the sandbox (they observe real process groups, so they fail inside it on the base branch too). The exception is preserves exact argv and strips wrapper-only environment before exec, which fails on the base branch too on this machine because it expects node in /usr/bin or /bin.
  • tsc exits 0 for apps/server, and lint is clean on the touched files.

main has the same two opencodeRuntime.ts kills; this PR only targets V2.

🤖 Generated with Claude Code


Devin Review

A fake spawner in tests reports pid 1, and the OpenCode runtime cleans up
with process.kill(-pid). That became kill(-1, SIGKILL), which signals every
process the user owns: running the provider tests killed the T3 server,
cloudflared and every agent on the machine.

signalProcessGroup refuses a pid of 0, 1 or a non-integer with ESRCH, like
a group that already exited, and every server process-group signal
(OpenCode runtime and server ledger, ACP, Pi) goes through it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 4.9 KiB — 6.8 KiB ✅
Codex Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Codex Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.4 KiB — 29.3 KiB ✅
Codex Live turn messages — 2 — 8 ✅
Claude Total thread wire — 4.9 KiB — 6.8 KiB ✅
Claude Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Claude Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Claude Live turn WebSocket decoded — 20.8 KiB — 29.3 KiB ✅
Claude Live turn messages — 2 — 8 ✅

Baseline: unavailable · PR result: 38d9984 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 106.1 KiB
  • Claude decoded thread snapshot: 106.4 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@macroscopeapp

macroscopeapp Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The runtime change is a focused process-group safety fix with bounded impact and tests covering both rejected and valid PIDs. The new test file also adds a file-level static-analysis suppression directive, so human review is warranted.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge
juliusmarminge merged commit 6562235 into t3code/codex-turn-mapping Sep 30, 2026
23 of 24 checks passed
@juliusmarminge
juliusmarminge deleted the v2/refuse-pid1-group-kill branch September 30, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant