Skip to content

Log turn_in_flight gate rejections and add an in-turn prompt-drop tripwire - #415

Merged
xintaofei merged 2 commits into
xintaofei:mainfrom
ijpq:feat/log-turn-in-flight-gate
Aug 5, 2026
Merged

Log turn_in_flight gate rejections and add an in-turn prompt-drop tripwire#415
xintaofei merged 2 commits into
xintaofei:mainfrom
ijpq:feat/log-turn-in-flight-gate

Conversation

@ijpq

@ijpq ijpq commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Addresses #409.

Three sites reject a concurrent prompt with AcpError::TurnInProgress but emit nothing, so on a desktop run the rejection leaves no backend trace — matching the "logs didn't reveal anything" reported in #409.

Changes

  • debug! at the three gate sites (send_prompt_inner, send_prompt_linked_with_message_id, fork_session), each carrying connection_id. debug! because queue-while-busy is an expected, user-driven condition on a normal path (would be noisy at warn).
  • warn! tripwire before the mid-turn command handler's _ => {}: the gate rejects a second prompt before it is ever enqueued, so a Prompt reaching that handler means an ungated sender broke the gate invariant.

Deliberately out of scope (happy to add if wanted)

  • The 4th TurnInProgress site in the session-activation/rebind path (manager.rs) — a different operation than the three prompt/fork gates.
  • The silent maybe_emit_ext_notification fallback (the real target of codex官方可以,但是codeg不行 #2 in the thread) — an unmapped ext notification returns with no log or probe counter.

Followed the review's suggestions: debug! for the gate vs warn! for the tripwire, connection_id on every line, and the [ACP] prefix.

ijpq and others added 2 commits August 5, 2026 17:13
Three send_prompt*/fork_session sites reject a concurrent prompt with
AcpError::TurnInProgress but emit nothing, so on a desktop run the
rejection leaves no trace (the "logs didn't reveal anything" from xintaofei#409).

- debug! at the three gate sites (send_prompt_inner,
  send_prompt_linked_with_message_id, fork_session): an expected,
  user-driven queue-while-busy condition, carrying connection_id.
- warn! tripwire before the mid-turn command handler's `_ => {}`: a
  Prompt reaching it means an ungated sender broke the gate invariant.

Refs xintaofei#409

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n drops

The two turn_in_flight gate lines were byte-identical and share a module
target with no file/line in the default format, so a reader could not tell
which of the two checks bounced the prompt. Each now names its gate.

maybe_emit_ext_notification is the silent fallback xintaofei#409's second point was
pointing at. All three of its drop paths now log at debug: an unhandled agent
request, an unexpected response dispatch, and an ext notification whose method
no mapper claims. The last is gated on is_known_ext_method so a
_claude/sdkMessage that merely is not an API retry stays quiet -- that method
arrives once per SDK message, and an unguarded line there would sit on a hot
path.

Refs xintaofei#409
@xintaofei

Copy link
Copy Markdown
Owner

Thanks — merging this. I pushed one follow-up commit (0f1135c) rather than asking you for another round, since both items came out of my own review notes.

1. The two gate lines were byte-identical. Both read "[ACP] prompt rejected: a turn is already in flight", and they share the tracing target codeg_lib::acp::manager with no file/line in the default format — so a reader couldn't tell which of the two checks bounced the prompt. They now name their gate: "...at the send gate..." in send_prompt_inner, "...before admission..." in send_prompt_linked_with_message_id.

2. Picked up the maybe_emit_ext_notification fallback — the one your issue's second point was actually pointing at. All three of its drop paths now log at debug: an unhandled agent request (with the method — the responder is still dropped unanswered, same as before, which is worth seeing when triaging a stalled turn), an unexpected response dispatch, and an ext notification whose method no mapper claims.

That last one needed a guard. map_claude_sdk_ext_notification returns None for every _claude/sdkMessage that isn't an API retry, and that method arrives once per SDK message — an unguarded line there would sit on a per-message hot path. This repo has scar tissue for exactly that shape (a kill_tree per-PID debug firehose once grew a server log to 217GB, which is why TARGET_BACKSTOPS pins kill_tree=warn). So the line is gated on is_known_ext_method, which only fires for a method codeg has never heard of. Forgetting to list a newly-mapped method there fails safe — you get noise, not silence — and there's a unit test pinning the invariant.

One correction for the record: the PR description mentions leaving out "the 4th TurnInProgress site in the session-activation/rebind path." There is no fourth site — grep finds exactly three executable return Err(AcpError::TurnInProgress) paths in the tree, and all three are the ones you instrumented. Nothing was missed; the note is just describing something that doesn't exist.

Left alone deliberately, in case anyone wonders later:

  • debug! rather than info! at the gates. LogLevel::default() is Info, so these are off until you raise the level — but queue-while-busy is an expected, user-driven condition, and debug-mode visibility is what the issue asked for.
  • No test asserting the call sites emit. The repo tests log plumbing under src/logging/; asserting emitted output at a call site needs a process-global subscriber and would be racy against the rest of the suite.
  • The tripwire still drops the Prompt rather than surfacing an error to the user. That's a behavior change beyond a logging PR, on a path that can't be reached today.

Green across the board on the final state: 2239 desktop + 2215 server + 12 sacp-tokio Rust tests, all three clippy matrices at -D warnings, eslint, 3388 vitest, tsc --noEmit, and the static export build.

Nice catch on the original issue — the gate genuinely had no trace, and the ext-notification fallback was a real blind spot.

@xintaofei
xintaofei merged commit 907660c into xintaofei:main Aug 5, 2026
7 checks passed
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