Skip to content

fix(acp): settle deferred terminal frames - #4192

Open
probepark wants to merge 1 commit into
devfrom
fix-acp-deferred-terminal
Open

fix(acp): settle deferred terminal frames#4192
probepark wants to merge 1 commit into
devfrom
fix-acp-deferred-terminal

Conversation

@probepark

Copy link
Copy Markdown
Collaborator

Summary

  • derive SDK frame correlation through one shared helper so live ingest and deferred replay merge terminal identity from both the envelope and decoded event
  • log incomplete or mismatched terminal drops at error level without treating benign settled duplicates as failures
  • attribute watchdog progress only to frames whose non-conflicting correlation exactly owns the active prompt, so unrelated/session-scoped traffic cannot keep a wedged turn alive

Correlation rationale

Terminal ownership still requires the complete commandId + turnId identity fence. The fix restores the missing correlation inputs during deferred replay rather than weakening matching. Terminal events use strictCorrelationFrom(frame, event) through the same helper as live ingest; nonterminal routing retains correlationFrom, while watchdog attribution separately rejects conflicting envelope/event claims because those frames must not mutate or extend another turn's bound.

Watchdog reconciliation

Unrelated traffic proves the session host process is alive, but it does not prove progress on this prompt. Only exact prompt-owned frames now restart the per-gap watchdog and update tool/model activity. This preserves long inference/tool protection when the host is demonstrably working on the active turn while keeping a finite bound on a wedged turn amid unrelated host traffic.

Tests

  • confirmed the new split-correlation pre-ack terminal regression failed on dev with a frozen virtual clock (Timed out waiting for pre-acknowledgement prompt completion)
  • bun test packages/coding-agent/test/acp-prompt-settle-on-completion.test.ts packages/coding-agent/test/acp-prompt-watchdog.test.ts packages/coding-agent/test/sdk-acp-prompt-terminal.test.ts packages/coding-agent/test/acp-prompt-conformance.test.ts packages/coding-agent/test/acp-session-reconnect.test.ts packages/coding-agent/test/sdk-acp-two-client-race.test.ts packages/coding-agent/test/sdk-acp-production-path.test.ts packages/coding-agent/test/sdk-acp-adapter.test.ts packages/coding-agent/test/acp-event-mapper.test.ts — 95 pass
  • bun --cwd=packages/coding-agent run check — pass
  • bun run lint — pass

Review

  • round 1: one major found — permissive nonterminal correlation could let conflicting envelope/event IDs refresh the watchdog; fixed with conflict-detecting watchdog attribution and regression coverage
  • round 1 minor findings (late-duplicate logging and vacuous stale-frame waits) were also fixed
  • round 2: no critical or major issues; no minor findings left open

Follow-up

The unrelated startup warning was confirmed out of scope: the settings schema contains task.enableLsp but no bare task.enable. This PR does not change settings reconciliation.

Fixes #4190

A terminal that arrived before prompt acknowledgement could lose event-owned correlation during replay, leaving session/prompt pending forever. Shared strict derivation now preserves ownership, dropped terminals are observable, and unrelated traffic cannot indefinitely extend the turn watchdog.

Lore-id: a4190cde
Constraint: terminal ownership must require exact commandId and turnId correlation
Rejected: loosen correlation matching | stale or foreign turns could settle the active prompt
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: 95 ACP tests; coding-agent check; repository lint

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — pre-ack watchdog rejection loses ownership before the late acknowledgement

Reviewed exact head 39c64b2d840a3365a0c6fc272e7ac576907fe018. The deferred-terminal fix is directionally correct, but this head does not cover the newly confirmed pre-ack watchdog ownership sequence.

Causal gap

  • prompt() waits for record.adapter.prompt() at acp-agent.ts:1461-1466. If the watchdog expires first, #rejectPrompt clears record.activePrompt and settles the waiter at :2344-2366; because acknowledgement has not arrived, waiter.correlation is empty and no settledPromptCorrelations tombstone is recorded.
  • When the late acknowledgement eventually arrives, :1472-1475 assigns its exact commandId/turnId, but there is no retired-waiter branch that installs that now-authoritative identity into settledPromptCorrelations before replay/return.
  • After successor prompt B is acknowledged, fully correlated nonterminal A frames are not fenced to B's exact correlation. At :2208-2268, A is absent from settledPromptCorrelations; tool_execution_start can populate record.toolArgs at :2249-2256, and A wire updates can reach publication. The exact terminal fence does not protect nonterminal message/tool traffic.

Required focused regression

Add one virtual-clock test with this exact order:

  1. Dispatch prompt A and withhold its SDK acknowledgement.
  2. Expire A's prompt watchdog while waiter.correlation is still empty; assert the watchdog rejection occurred.
  3. Deliver A's late accepted acknowledgement carrying complete exact commandId + turnId; assert A rejects to the ACP caller and that identity is retired/tombstoned.
  4. Start and acknowledge prompt B.
  5. Deliver fully correlated A message_end/message_update and tool_execution_start frames.
  6. Assert B receives no A message/tool publication, no tool/activity mutation, and no watchdog refresh/rearm.
  7. Deliver a deliberately correlationless session event; assert it remains publishable but still does not refresh B's watchdog.
  8. Include private sentinel text in A message/tool payloads and assert any diagnostic metadata contains neither sentinel nor private payload data.

The smallest acceptable fix is to keep the rejected waiter observable until a late acknowledgement can install its exact correlation tombstone, then fail closed for complete nonterminal correlations that do not exactly match acknowledged B. Do not suppress deliberately correlationless session events.

Current hosted CI does not exercise this sequence, so green checks cannot override the ownership hole.


Signed-off-by: Yeachan Heo bellman@gajae-code.dev

VERDICT: REQUEST_CHANGES

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants