Skip to content

test: commit ACP replay status before the agent's answers leave - #1010

Merged
rynfar merged 1 commit into
pylonfrom
fix/acp-registry-mode-picker-flake
Oct 4, 2026
Merged

rynfar merged 1 commit into
pylonfrom
fix/acp-registry-mode-picker-flake

Conversation

@rynfar

@rynfar rynfar commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

Root cause

The "agent's own mode picker" tests in AcpRegistryAdapterV2.test.ts (and every ACP registry/Grok replay that checks completeness) depend on apps/server/scripts/acp-replay-agent.ts. That script had two races. Both were in the harness, not in the adapter.

  1. Send-before-record. For each emit_inbound entry the agent wrote the frame to stdout and only then ran advance() → writeStatus(). A trailing runtime_exit advanced the cursor again after that. When the client receives the final answer, openSession completes, the scope closes, the agent is killed, and the status is read. If the client gets there first, it reads an earlier cursor (ACP replay did not consume all frames for stored-mode-pick, cursor 5/6 on run 37165516635), or the kill lands before the last write.
  2. Non-atomic status write. writeFileSync truncates the file and then writes it. A reader in between, or a kill in between, sees an empty file (Failed to decode ACP replay status, SyntaxError: Unexpected end of JSON input on run 37159438983 and PR feat(web): update providers on all connected machines (upstream) #992).

The adapter does not drop or reorder requests. The adapter-side assertComplete path in AcpAdapterV2 is exposed to race 2 as well, because it reads the status while the agent is still alive. Every checked-in ACP fixture ends with an inbound answer plus runtime_exit, so no transcript has a trailing client frame that needs a separate wait.

Fix

  • The agent consumes a whole batch of inbound frames, including a trailing runtime_exit. It commits the status, then sends the batch. A client that has seen a frame therefore always reads a status that counts it.
  • The status is written to a sibling file and moved into place with renameSync, so readers never see a partial file. A kill mid-write leaves the previous complete status.
  • On a mismatch, the failure status is written before the error answer is sent.
  • New contract test apps/server/scripts/acp-replay-agent.test.ts. It reads the status synchronously the moment each answer arrives, SIGKILLs the agent on the final answer as teardown does, and asserts the status counts every frame seen.

Repro rate

before after
AcpRegistryAdapterV2.test.ts, 8–16 parallel loops + 12 CPU hogs (macOS, 18 cores) 1 / 312 failed (empty-status decode) 0 / 104 failed
new contract test 20 / 20 failed (final status cursor 3/5) 20 / 20 passed

Validation

  • vp run -F t3 typecheck: passes; it covers scripts/.
  • vp lint / vp fmt --check on the changed files: clean.
  • OrchestratorReplayFixtures.integration.test.ts: all ACP registry and Grok fixtures pass. Locally, claude_result_is_error/claudeAgent fails with or without this change, because my shell's CLAUDE_CONFIG_DIR leaks into the expected message.

🤖 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 ACP replay agent sent each inbound frame before recording it in its
status file, and rewrote the status in place. A client that reacted to
the final answer could stop the agent or read the status first, seeing
an earlier cursor ("did not consume all frames") or a truncated file
("Failed to decode ACP replay status").

The agent now consumes a batch of inbound frames (and a trailing runtime
exit), commits the status atomically via rename, then sends the batch.
Mismatches are recorded before the error answer is sent.

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 labels Oct 4, 2026
@rynfar
rynfar merged commit b8c6acf into pylon Oct 4, 2026
22 checks passed
@github-actions

github-actions Bot commented Oct 4, 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 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 +3 B (+0.1%) 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 +3 B (+0.2%) 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: e34eb4e · PR result: 83838d7 · 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:M 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