Skip to content

test(ssh): establish the zombie fixture before checking replacement - #591

Merged
lukemaj merged 2 commits into
mainfrom
test/589-unreaped-zombie-race
Oct 20, 2026
Merged

lukemaj merged 2 commits into
mainfrom
test/589-unreaped-zombie-race

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What: Make the unreaped managed-server test establish its zombie fixture before checking replacement.
Why: Publishing the PID before shell exec lets the shell reap the killed child, and SIGKILL alone does not establish process exit.
So what: The test retains real zombie and replacement coverage; maintainers review this PR for main without changing the launch script.

Problem

The fixture printed a background child's PID before its shell became a non-reaping parent, then killed that child and immediately asserted ps reported Z. Those operations could race either shell reaping or process exit, producing the intermittent expected '' to include 'Z' failure in #589. Reliable CI supports the current Objective across Chromeria changes.

Change

Have the shell execute a Node parent before killing the inherited child. Register SIGCHLD before sending SIGKILL, and publish the PID only from that signal handler. The existing stdout read becomes an exit notification. The parent does not own a Node child-process handle, so it does not reap this child; its keepalive timer only preserves the parent's lifetime and performs no polling or synchronization.

Retain the independent ps zombie assertion, stale PID/runtime records, and checks that the launcher starts a different live managed server. Register the carried test edit in scripts/fork-upstream-edits.txt with exactly one primary owner, fork-maintenance, in docs/fork-features.md. No launch-script change, version bump, or website impact.

Adapted from the same-fixture fix in pingdotgg/t3code#20052, inspected at b3d651a5999b6de57e5c919d51a458263f5eff5e. Upstream main still polls; merged #22252 fixes a separate handover test and does not repair this fixture.

Verification

Linux 7.2.5-3-omarchy, Node v26.8.2. Original starting base: 9378189f41e8cfa3e8e4ffe8a30f92e17b22bbc1. Rebased without conflicts onto current origin/main at 14bd851ff977049acce4cd2746b65cb9c442555b. Current candidate: 3340818a4375b20d6a9e4f3a19365d8a0da7301c.

  • Before: 50/50 consecutive independent executions of node_modules/.bin/vp test run packages/ssh/src/tunnel.test.ts -t 'replaces a managed server that exited but was not reaped' passed. The intermittent CI failure was not reproduced locally; this is not a demonstrated red/green cycle.
  • After the original test fix: the same command passed 50/50 consecutive independent executions, exercising the real zombie assertion and replacement each time. The rebase preserved the test diff.
  • node_modules/.bin/vp test run packages/ssh/src/tunnel.test.ts: all 30 tests passed, including a fresh run on the rebased candidate.
  • bash scripts/fork-check.sh --upstream upstream: reproduced the missing-allowlist failure before registration; passed on the current candidate (fork-features: OK (58 features), fork-check: OK).
  • node_modules/.bin/vp lint packages/ssh/src/tunnel.test.ts: passed.
  • node_modules/.bin/vp fmt --check packages/ssh/src/tunnel.test.ts: passed.
  • node_modules/.bin/tsc-rs --noEmit -p packages/ssh/tsconfig.json: passed.
  • git diff --check: passed. No repo-wide checks or browser verification run. CI results are tracked by this PR.

Elon record

Requirements and who asked: The user and #589 require deterministic zombie setup without sleeps or polling, preserving exited-but-unreaped managed-server replacement coverage.
Deleted: Retrying/skipping the test, polling, production changes, a Spec, version bumps, and unrelated upstream handover changes.
Bottleneck: Observe child exit only after the parent can no longer reap it; a SIGCHLD notification supplies that event.
Checked myself: Read #589 and both complete upstream PR diffs, inspected the fixture and unchanged assertions, ran 50 baseline and 50 candidate repetitions plus the full test file, and passed scoped static checks.

Closes #589

Model: gpt-6.1-sol. Harness: Codex.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent review found no supported blocking findings on f78bfe3359496c94fc09c3fc83dd70e0495d37eb.
Why: The reviewer verified event-driven exit synchronization, genuine zombie state, retained replacement assertions, and scoped cleanup.
So what: Maintainers can assess this candidate and its CI results; no merge was performed.

Independent reviewer: gpt-6.1-sol through Codex, delegated review round 589-review-f78bfe3359-r1. Verdict relayed unchanged:

Clean verdict: no supported blocking findings for 9378189f41e8cfa3e8e4ffe8a30f92e17b22bbc1..f78bfe3359496c94fc09c3fc83dd70e0495d37eb.

Checked packages/ssh/src/tunnel.test.ts, surrounding launch logic in tunnel.ts, and Effect’s scoped cleanup: the Node parent registers SIGCHLD before killing its inherited child without reaping it; synchronization is event-driven, the timer only keeps the parent alive, and ps/Z, replacement assertions, and cleanup remain intact.

Review was read-only; no tests ran, and reported check results plus pending repetition/full-file results were not independently verified.

Author verification has since completed: 50/50 candidate repetitions and all 30 tests in the full file passed; targeted lint, format, and SSH-package typecheck passed. The reviewer did not rerun tests.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S labels Oct 20, 2026
@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 4.9 KiB 5.0 KiB +23 B (+0.5%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB +23 B (+1.9%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.8 KiB 20.9 KiB +41 B (+0.2%) 29.3 KiB ✅
Codex Live turn messages 1 2 +1 (+100.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB −24 B (−0.5%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB −24 B (−2.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB −41 B (−0.2%) 29.3 KiB ✅
Claude Live turn messages 2 1 −1 (−50.0%) 8 ✅

Baseline: 14bd851 · PR result: 3340818 · 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: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

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

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent review passed on 3340818a4375b20d6a9e4f3a19365d8a0da7301c, and CI's Fork Stack Model check is now green.
Why: The rebased test has its required allowlist entry and exactly one primary owner.
So what: The PR remains unmerged; other CI checks are still running.

Independent gpt-6.1-sol / Codex reviewer, round 589-review-3340818a43-r2, verdict relayed unchanged:

Clean review of 3340818a4375b20d6a9e4f3a19365d8a0da7301c against 14bd851ff977049acce4cd2746b65cb9c442555b: no actionable findings.

Reviewed the complete three-file diff: allowlisting and exactly one fork-maintenance owner correctly reference #589/#591; the test is byte-identical to the earlier reviewed candidate, preserving SIGCHLD synchronization and independent zombie/replacement assertions; no merge commits were found.

No tests or repository writes performed; reported runtime proof was not independently rerun, and comment/status publication remains with the parent.

Author reran bash scripts/fork-check.sh --upstream upstream successfully and node_modules/.bin/vp test run packages/ssh/src/tunnel.test.ts with 30/30 tests passing. Fresh scoped static checks also passed. CI Fork Stack Model succeeded. Full handoff and Elon record: #589 (comment).

@lukemaj
lukemaj merged commit 5660324 into main Oct 20, 2026
25 of 26 checks passed
@lukemaj
lukemaj deleted the test/589-unreaped-zombie-race branch October 20, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 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.

test(ssh): unreaped-server launch test races the zombie state

1 participant