Skip to content

fix(acp): don't lose queued prompts when the drain echo hits a dead connection - #71012

Open
israellot wants to merge 1 commit into
NousResearch:mainfrom
israellot:fix/acp-queued-prompt-loss
Open

fix(acp): don't lose queued prompts when the drain echo hits a dead connection#71012
israellot wants to merge 1 commit into
NousResearch:mainfrom
israellot:fix/acp-queued-prompt-loss

Conversation

@israellot

Copy link
Copy Markdown
Contributor

What & why

The post-turn queued-prompt drain in acp_adapter/server.py can permanently lose a user's queued message when the client disconnects mid-drain:

next_prompt = state.queued_prompts.pop(0)
if conn:
    await conn.session_update(              # <-- unguarded
        session_id,
        acp.update_user_message_text(next_prompt),
    )
await self.prompt(...)                      # <-- never reached

The drain pops the queued text and then awaits the user-message echo unguarded. If the client goes away at that instant, session_update raises (e.g. ConnectionResetError) and the exception escapes prompt() after the pop: the queued text is neither run nor re-queued — permanently lost. User-visible symptom: you type a message while a turn is running, see it acknowledged as "Queued for the next turn (1 queued)", and it silently evaporates.

The fix wraps the drain echo in try/except Exception with a debug log. The echo is cosmetic — the popped prompt is the payload — so a dying connection must never prevent await self.prompt(...) from running the popped text. The transcript persists server-side and replays on reattach, so a missed echo is harmless while a missed turn is data loss. No other session_update call sites are touched: this is the only echo site in the drain flow with the pop-then-lose shape.

Reproduction sketch

  1. Start a turn in an ACP session; while it runs, submit another prompt so it lands in state.queued_prompts.
  2. Have the client connection die right as the first turn finishes (before the drain's user-message echo completes).
  3. Without this fix, the echo raises out of prompt() after the pop — the queued prompt never runs and is gone from the queue.

The regression test reproduces this deterministically by exercising the real prompt() path with run_conversation mocked (first turn enqueues a prompt) and a mock conn whose session_update raises ConnectionResetError on exactly the drain's UserMessageChunk echo. On upstream/main before the fix it fails with the ConnectionResetError escaping and only one run_conversation call; with the fix it asserts the queued text runs as the second turn and the queue is drained.

How to test

bash scripts/run_tests.sh tests/acp/test_server.py -q
# => 87 tests passed, 0 failed

bash scripts/run_tests.sh tests/acp/test_server.py -q -k test_queued_prompt_survives_drain_echo_failure
# RED on upstream/main without the server.py change (ConnectionResetError escapes prompt());
# GREEN with it.

python3 scripts/check-windows-footguns.py acp_adapter/server.py tests/acp/test_server.py
# => No Windows footguns found

Platforms tested

Linux fully verified; Windows/macOS not manually tested (change is pure Python exception handling, no platform-specific I/O; footguns check clean).

Related

@alt-glitch alt-glitch added type/bug Something isn't working comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists labels Jul 24, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused ACP regression fix. Current main still pops state.queued_prompts at acp_adapter/server.py:1962 and awaits the client echo at acp_adapter/server.py:1964 before reaching the recursive queued turn at acp_adapter/server.py:1968; a failed echo can therefore strand the popped text. The PR's guard is scoped to that best-effort echo, while its added test verifies the queued turn still runs after ConnectionResetError.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@israellot
israellot force-pushed the fix/acp-queued-prompt-loss branch from ac6d6d5 to 298289c Compare August 19, 2026 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants