fix(acp): replace turn_interrupted boolean with monotonic turn counter#424
Open
fix(acp): replace turn_interrupted boolean with monotonic turn counter#424
Conversation
The previous approach used a shared AtomicBool flag to suppress stale TurnLifecycle::Completed events from cancelled tasks. This had an unavoidable TOCTOU race: when a user interrupted and quickly sent a new message, the flag was reset for the new turn before the old task could check it, allowing stale events to interfere with subsequent turns. Two prior attempts (PRs #416, #418) tried to fix this with variations of the same boolean+counter architecture. Each fix introduced a new bug: #416 caused hangs, #418 brought back hidden responses. This commit replaces the architecture entirely: - AtomicBool → AtomicU64 monotonic turn counter in ACP backend - Each spawned task captures its own turn ID at spawn time - ALL tail events (ErrorEvent + Completed + idle timer) are guarded by a single turn_id match check - TUI's pending_stale_completes counter is removed (no longer needed) The monotonic counter eliminates the race because it only increments— there is no "reset" that a stale task can observe. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <[email protected]>
…ate channel When a cancelled prompt returns AFTER a new prompt has installed its update sender in the shared active_update_tx slot, the old prompt's unconditional uninstall (setting to None) wipes the new prompt's channel. This causes the new prompt's response events to be routed to the persistent channel instead of the update handler, making the response invisible in the TUI. Fix: add a generation counter to the active_update_tx slot. Each prompt() and load_session() call gets a unique generation and only uninstalls if its generation still matches the slot. This prevents stale prompts from interfering with newer ones. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <[email protected]>
theahura
force-pushed
the
fix/stale-completed-turn-counter
branch
from
0858208 to
9056f1c
Compare
Add integration test verifying sequential prompt responses work after the generation counter fix. Remove temporary TURN_DEBUG tracing. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <[email protected]>
…te notifications block_task() can return before all SessionNotification events are delivered. The previous approach of calling close_update_channel() immediately after prompt() would kill the channel too early, causing late notifications to be lost. This was especially problematic in the interrupt+re-prompt flow where the first prompt's close could wipe the second prompt's channel. Replace close_update_channel() calls from prompt callers with a done_tx/done_rx oneshot signal. After prompt() returns, the done signal fires and the update_handler drains remaining events with a 500ms timeout before exiting. The active_update_tx slot is no longer cleared between turns — the next prompt() overwrites it. Also fix the notification handler to fall through to persistent_tx when try_send fails (closed or full channel), preventing inter-turn notifications from being silently dropped. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <[email protected]> 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
🤖 Generated with Nori
turn_interrupted: Arc<AtomicBool>withturn_id: Arc<AtomicU64>monotonic turn counter to eliminate TOCTOU race that caused agent responses to be hidden after interruptpending_stale_completescounter — no longer needed since the ACP backend now reliably suppresses all stale eventsThis is the third attempt at fixing this interrupt race condition (after PRs #416 and #418). The previous boolean+counter architecture was fundamentally flawed — each fix introduced a new bug. The monotonic counter eliminates the race entirely because it only increments and is never reset.
Test Plan
just fixandjust fmtcleanShare Nori with your team: https://www.npmjs.com/package/nori-skillsets