Skip to content

test(server): prevent worker PID collisions in ACP rotation fixture - #190

Merged
lukemaj merged 1 commit into
fork/v2from
fix/166-acp-fixture-pids
Oct 8, 2026
Merged

lukemaj merged 1 commit into
fork/v2from
fix/166-acp-fixture-pids

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What: Make the ACP process-tree rotation test pass independently of its worker PID.
Why: Its synthetic parent/child ranges collide with process.pid, causing valid runtime self-protection to retain the CI pairs 1111/11111 and 1127/11127.
So what: The single PR is ready for maintainer review on fork/v2; it remains unmerged.

Closes #189. Refs #166.

Allocate the fixture root, parents, children and tombstones above the worker PID, following the fully inspected open upstream candidate pingdotgg/t3code#16633 at c8175050793c3bc46995479f97ed0c6af2259ad0. Preserve all budget, rotation, tombstone, snapshot and cleanup assertions. Run the rotation test for the native PID and four fixed parent/child collisions; restore the real PID with Effect.ensuring. Production code is unchanged.

Upstream bug candidate: This is an upstream test-fixture defect, already proposed in #16633. No merged fix exists in the inspected ACP history at upstream/main 9b0df1358f9f9c3277bcb33777c2435b88ee75c4; #17299 leaves this test in place and only changes the runtime's process-group import. Windows candidates #17274/#17080 do not address this cause.

Proof:

  • At base 36d5c0169fc042930aa36e41248c1292cec781d0, Node 24.13.1, forced worker PIDs 1111 and 1127 reproduce the exact CI retained pairs. Regression-only parameterization fails all four collision cases before allocation correction, while the native case passes. Commands and causal evidence.
  • Host-neutral full process-tree file: 27/27 pass, including five rotation cases. Scoped lint, formatting and whitespace checks pass. Server-only typecheck exits 0 with suggestions only; fork-check passes. Final proof.
  • Each heavy batch starts only after measuring one-minute load <8. No servers, browsers, live userdata, repo-wide checks, version bumps or installs.
  • Independent exact-head review passed: GPT-6-Luna, Codex, max effort, Prism reviewer defaults, no actionable findings. Verbatim verdict. Latest review/independent=success on eb4d7f6c3a36288fad64429fa5304869a3deba07, creator lukemaj, matches authenticated gh account.

Upstream edit inventory relative to the PR base:

Upstream file Added Removed Owner
apps/server/src/provider/acp/AcpSessionRuntime.processTree.test.ts 94 79 fork-maintenance

The single test hunk exceeds 15 lines because parameterization and unconditional PID restoration indent the existing test body. Ignoring whitespace shows only PID allocation, fixed regression cases and restoration changes. All existing behavioral assertions remain. The other two edited files are fork-owned metadata: one allowlist line and one primary-owner entry.

Requirements and who asked: User requested one small PR, deterministic reproduction and regression, upstream-first investigation, fork ownership metadata and visible independent exact-head review.
Deleted: No production change, skips, waivers, retries, helper abstraction, UI evidence or repo-wide proof.
Bottleneck: The fixture aliases the protected worker PID; removing that alias makes the existing teardown oracle meaningful.
Checked myself: Reproduced both exact CI signatures and all four permanent collision cases before fixing; inspected complete upstream candidate and runtime protection; checked the complete final diff and focused green proof.

Agent Observer cost record published with required publish --task toolboxmd/chromeria#189 --repo toolboxmd/chromeria --pr 190; usage attribution unavailable, estimated cost unknown. CI remains in progress; macOS focused proof is not Linux cgroup live verification.

Exact head: eb4d7f6c3a36288fad64429fa5304869a3deba07. Exact current base: 36d5c0169fc042930aa36e41248c1292cec781d0. No rebase needed. Workspace: /Users/lukaszmaj/dev/toolboxmd/chromeria-worktrees/166-acp-pid.

Website impact: none, because only a server test fixture and fork maintenance inventory change. Version policy: no bump, as required by repository policy and this task.

Implemented with GPT-6.1-Sol, medium reasoning, Codex harness in T3 Code. Independently reviewed with GPT-6-Luna, max reasoning effort, Codex harness, Prism reviewer defaults.

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

lukemaj commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

What: Focused proof passes for final head eb4d7f6c3a36288fad64429fa5304869a3deba07.
Why: Collision-free fixture identities preserve runtime self-protection and all original rotation assertions.
So what: PR #190 awaits independent exact-head review and the cost record, then can leave draft without merging.

Base refreshed before final proof/review: 36d5c0169fc042930aa36e41248c1292cec781d0, current origin/fork/v2. No rebase was needed. Commit hook formatted only staged files; verified clean worktree and unchanged tested tree after commit.

Host-neutral Node 24.13.1 proof, with immediate per-batch one-minute load check (<8):

  • From apps/server: pnpm exec vp test run src/provider/acp/AcpSessionRuntime.processTree.test.ts --maxWorkers=1 --maxConcurrency=1: 27/27 passed, 4.96s. Includes native worker and permanent 1111/1127/11111/11127 cases. Reuses the exact unchanged committed test tree.
  • From root: pnpm exec vp lint apps/server/src/provider/acp/AcpSessionRuntime.processTree.test.ts: passed.
  • pnpm exec vp fmt --check apps/server/src/provider/acp/AcpSessionRuntime.processTree.test.ts docs/fork-features.md: passed.
  • From apps/server, final-head pnpm exec tsc --noEmit: exit 0, suggestions only, no errors/warnings. Batch load 4.769. This is the affected server package, not repo-wide typecheck.
  • bash scripts/fork-check.sh: passed, fork-features: OK (19 features), fork-check: OK.
  • git diff --check HEAD^ HEAD, clean worktree, exact remote PR head and base: passed.

Changed upstream file: apps/server/src/provider/acp/AcpSessionRuntime.processTree.test.ts +94/-79; ignoring whitespace +25/-10. The broad hunk is indentation from deterministic parameterization and finalizer restoration, not a runtime rewrite. The other two files carry one fork-maintenance metadata line each.

Review pending: visible T3 Code reviewer thread sub.sub.sub.thread-9c8e27a973f0.dispatcher-c7ac4ae7f4c9.dispatcher-8e3f1223df4f.reviewer-6186d476e11c, Prism selected GPT-6-Luna, Codex harness, exclusive detached read-only worktree /Users/lukaszmaj/dev/toolboxmd/chromeria-worktrees/189-acp-review. Parent authenticated gh account lukemaj posted review/independent=pending on exact head; final verdict will be relayed unchanged.

Limits: macOS focused tests are not Linux cgroup live proof; CI owns Linux and repo-wide validation and is still running. Production unchanged; no waiver or retry fix. No versions bumped, no merge/install/deployment. Worktrees remain for this open PR.

@lukemaj

lukemaj commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Agent work on this PR

Estimated cost unknown · 0 responses · 49 sessions · 7.8 h wall time

Model Responses Tokens Estimated cost

Flags: 4 human corrections · 89 large tool outputs · 108 repeated commands · 11 repeated failures · 107 repeated reads · 17 repeated skill loads · 48 sessions with usage bound to no task · 1 session without usage records

Details: snapshot, prices, coverage, counters
  • Task toolboxmd/chromeria#189: outcome unknown (recorded acceptance only; a finished process never implies it).
  • Proof: test(server): make ACP rotation fixture independent of worker PID #189
  • Snapshot 444e7bc65bb308204df06e4ad1efdb3166adbbb3cf9e8d5dac9a1a6cdd808f5b, records up to 2026-10-08 21:28 UTC.
  • 49 sessions on claude, codex; AgentsMD 14.6.0.
  • Not counted: 6,273 responses (at least $137.71) in sessions shared with other PRs that worked in no single PR's checkout.
  • 5,005 responses in these sessions worked on other PRs and are counted there.
  • Totals reconcile with the measured sessions: yes. Evidence complete: no.
  • Prices: list-price estimate from T3 local rate table (path withheld) as of 2026-09-28, schedule 696aae45933d0a684a015d3f08cfb6f1aa2fef02e7d272b4689e3a692ea04fff. Unknown prices stay unknown, never zero.
    • T3 LiteLLM rate table when present; bundled schedule covers the rest.
  • Native session usage or worker ownership is unavailable.
  • Harness-reported cost: none reported.
  • Usage totals are not billing. Subscription spending is separate and is never posted as spend.
  • Crashed runs are counted separately: 0.

Token counters by model (native counter semantics; never added across semantics):

Selected rates (USD per million tokens). These rates value the report at the selected schedule date; they do not establish historical prices or subscription spending.

Model Input tier Input Cache read Cache write Other output Reasoning

Other output and reasoning are priced without double counting inclusive native output. Missing rates remain unknown.

Local measurement from native records; usage totals are not billing. Updated in place by agent-observer publish.

@github-actions

github-actions Bot commented Oct 8, 2026

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 — 5.0 KiB — 6.8 KiB ✅
Codex Thread snapshot wire — 3.8 KiB — 4.9 KiB ✅
Codex Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.9 KiB — 29.3 KiB ✅
Codex Live turn messages — 2 — 8 ✅
Claude Total thread wire — 5.0 KiB — 6.8 KiB ✅
Claude Thread snapshot wire — 3.8 KiB — 4.9 KiB ✅
Claude Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Claude Live turn WebSocket decoded — 21.2 KiB — 29.3 KiB ✅
Claude Live turn messages — 2 — 8 ✅

Baseline: unavailable · PR result: eb4d7f6 · 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 8, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent exact-head review passed with no actionable findings.
Why: The reviewer verified the fixture cause, disjoint allocation, restoration, retained assertions and fork ownership against the exact PR candidate.
So what: The authenticated account records review/independent=success; PR #190 can leave draft without merging.

Reviewer thread: sub.sub.sub.thread-9c8e27a973f0.dispatcher-c7ac4ae7f4c9.dispatcher-8e3f1223df4f.reviewer-6186d476e11c. Reviewer report relayed unchanged below:

Exact-head verdict

success: no actionable findings for PR #190 at head eb4d7f6c3a36288fad64429fa5304869a3deba07, based on fork/v2 at 36d5c0169fc042930aa36e41248c1292cec781d0. I verified the detached worktree, GitHub PR head/base, and merge base. The worktree remained clean.

The fixture cause is supported by the code: server() keys its synthetic identity to process.pid, and the fixture controller stores identities by PID. The production guard deliberately protects the current PID and its process group/session. The old ranges could overwrite the fixture’s server entry; the new ranges place the root, parents, children, and tombstones above the spoofed worker PID and keep those ranges disjoint. See the fixture and restoration and the production guard. Effect.ensuring restores the PID, and the existing budget, rotation, missing-process, pass-count, snapshot-count, and cleanup assertions remain.

The upstream candidate #16633 is open; its full diff contains the same allocation correction. The inspected upstream main still has the old ranges. Production code is unchanged: the PR changes only the test fixture and fork-maintenance metadata. The test file is assigned to fork-maintenance in the feature map and appears in the upstream-edit allowlist (feature map, allowlist).

Reviewer identity: GPT-6-Luna (gpt-6-luna), Codex harness, max reasoning effort, with Prism reviewer defaults and no override. This was a source-only review; I did not rerun checks. The final Issue comment records the focused proof, including tsc exit 0 and fork-check passing. No blocker or additional discriminating check is needed for this verdict. The reported cost attribution remains unknown and is outside the code review verdict.

@lukemaj
lukemaj marked this pull request as ready for review October 8, 2026 21:33
@lukemaj

lukemaj commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

What: PR #190 is open, ready for review and unmerged at eb4d7f6c3a36288fad64429fa5304869a3deba07.
Why: Focused proof and exact-head independent review pass; the Linux Server 4 CI job containing the reported failure is now green too.
So what: Delivery is complete; the maintainer decides merge, and worktrees remain until merge finalization.

Linux Server 4: https://github.com/toolboxmd/chromeria/actions/runs/37846842942/job/113549717557 (success). Latest exact-head review/independent: success, creator lukemaj, matching authenticated gh account. Live base still 36d5c0169fc042930aa36e41248c1292cec781d0. Both author and reviewer worktrees clean. The final T3 PR-link inventory contains #190, this task's sole PR. Auxiliary workflows triggered by leaving draft are still in progress; no failing check was observed. Cost remains unknown because Observer lacks usage attribution, as disclosed in its published record.

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.

1 participant