Skip to content

test: keep ACP process-tree fake PIDs from aliasing the worker PID - #1007

Merged
rynfar merged 1 commit into
pylonfrom
fix/acp-processtree-rotation
Oct 3, 2026
Merged

rynfar merged 1 commit into
pylonfrom
fix/acp-processtree-rotation

Conversation

@rynfar

@rynfar rynfar commented Oct 3, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes the recurring Test Server 2 failure in AcpSessionRuntime.processTree.test.ts > terminatePosixOwnedProcessTree > rotates more than 64 live parents without scanning retained tombstones (ACP process tree 100 retained owned processes: 1088, 11088, and the same error for 1001/11001 and 1047/11047).

Root cause

This was a fixture PID collision. It was not a timing problem, and no merged change introduced a production regression.

  • The test seeds the fake controller with server() at the real process.pid and adds 130 fake parents at PIDs 1000–1129 plus children at +10000 in the same Map keyed by PID.
  • On fresh ubuntu-24.04 runners the vitest worker's real PID often falls in 1000–1129. When it does, the fake parent replaces the server entry. controller.identity(process.pid) then returns that parent, so current.pgid/current.sid become the parent's group.
  • terminatePosixOwnedProcessTree intentionally never signals process.pid or anything in the current process group or session. It skips that parent and its child, and the final residual check reports exactly N, N+10000. The production behavior is correct.
  • The failing PIDs changed from run to run because the worker PID depends on how many processes started before it on the runner. Changes in shard and job composition move that number, which is why the failures started after unrelated merges.

it.live and the 10 ms sleeps don't matter here. Because grace: 0, the outcome depends only on the PID.

Evidence

Reproduced by setting process.pid in a temporary copy of the file (removed before committing):

forced process.pid before after
unchanged / 1130 pass pass
1001 retained owned processes: 1001, 11001 pass
1047 retained owned processes: 1047, 11047 pass
1088 retained owned processes: 1088, 11088 pass
11088 retained owned processes: 1088, 11088 pass
1000, 1129, 11000, 11129 (in collision range) pass

The before messages match the CI errors exactly.

Fix

The test now allocates its synthetic parent, child, and tombstone PIDs above Linux PID_MAX_LIMIT (2^22; macOS PIDs stay below 100000), so they can never alias a real worker PID. Every assertion is unchanged, including snapshotCalls === 10, fixture.processes.size === 1, and the ≤64 work bound per pass. No production code changed.

Validation

  • vp test run src/provider/acp/AcpSessionRuntime.processTree.test.ts ×50: 50 passed, 0 failed
  • AcpSessionRuntime.test.ts + AcpSessionRuntime.processTree.test.ts: 37/37 passed
  • vp run -F t3 typecheck: clean; scoped vp lint / vp fmt --check: clean

Impact: test-only. No change to runtime, providers, wire contracts, or clients.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

The 130-parent rotation fixture used fake PIDs 1000-1129 and 11000-11129.
When the vitest worker's real process.pid landed in that range (common on
fresh ubuntu-24.04 runners), the fake parent replaced the server identity,
and terminatePosixOwnedProcessTree correctly refused to signal its own PID,
process group and session, failing with "retained owned processes: N, N+10000".

Allocate the fixture's synthetic PIDs above Linux PID_MAX_LIMIT (2^22) so
they can never alias a real PID. Assertions are unchanged.

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:S labels Oct 3, 2026
@rynfar
rynfar merged commit b81cf69 into pylon Oct 3, 2026
22 checks passed
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ The exact PR base did not have a successful artifact. Baseline uses the latest successful main measurement shown below.

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

Baseline: cda6633 · PR result: 6387524 · 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 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