Fix stranded action-result delivery race; document refuted hypotheses for #15659 - #15331
Open
warp-agent-staging[bot] wants to merge 5 commits into
Open
Fix stranded action-result delivery race; document refuted hypotheses for #15659#15331warp-agent-staging[bot] wants to merge 5 commits into
warp-agent-staging[bot] wants to merge 5 commits into
Conversation
…n search
FetchConversationResult::Cancelled has no user-facing cancel affordance
(unlike RunAgents' Reject button), so it is always collateral
cancellation from cleaning up the surrounding conversation's pending
actions, never a deliberate user action.
Previously, this was mapped to ConvertToAPITypeError::Ignore on the
wire (dropping the tool call result entirely) and excluded from
should_trigger_request_upon_completion (never eagerly reported), so
the ConversationSearchAgent subagent on the server could be left
blocked waiting on a result that never arrives on its own, and the
client's own action-completion subscriber would locally mark the
conversation Cancelled without ever contacting the server.
Fix:
- should_trigger_request_upon_completion() now always returns true for
FetchConversation(Cancelled), so its completion always triggers a
follow-up request.
- The wire conversion now sends an explicit FetchConversationResult
Error ("Conversation fetch was cancelled") instead of Ignore, which
the server's existing ConversationSearchAgent Pass 2 error handling
already understands via GetFetchError -> sendFinishSubagentWithError.
Fixes warpdotdev/warp-server#15659.
Contributor
Author
|
This PR was generated with Warp. Comment |
Review found that the previous revision made should_trigger_request_upon_completion()
unconditionally return true for FetchConversation(Cancelled). That result is produced by
the generic action-cancellation machinery for genuine terminal cancellations
(ManuallyCancelled from Stop/pane-close/delete) as well as for collateral
same-conversation cleanup (FollowUpSubmitted{is_for_same_conversation:true}) - not only
the collateral case, as previously assumed. Making it unconditional meant a genuine user
cancellation could be converted into a new outbound request, reviving a conversation the
user had stopped, and could race with the very follow-up request that triggered the
collateral cancellation in the first place.
Fix:
- Revert should_trigger_request_upon_completion() to its original, reason-agnostic
- Keep the wire-serialization fix (FetchConversationResult::Cancelled -> an explicit
Error instead of ConvertToAPITypeError::Ignore). This is safe for both cancellation
paths: it only has any effect when the result is actually included in a subsequent
outbound request, which for a genuine terminal cancellation never happens (no
follow-up is sent), and for the collateral case happens naturally via send_query's
existing prepend-drained-results-into-the-new-request flow -- "the request that owns
it" -- since a new request for the same conversation is already being sent regardless.
- Correct the doc comments on FetchConversationResult::Cancelled and in convert.rs, which
incorrectly asserted the collateral premise as the only cause.
Added regression coverage:
- crates/ai: a corrected unit test confirming FetchConversation(Cancelled) does NOT
unconditionally trigger a follow-up (matches other cancelled results).
- app: two new BlocklistAIController-level tests:
- manual_cancel_of_pending_fetch_conversation_does_not_revive_conversation: a
ManuallyCancelled cancellation of a pending FetchConversation sends no follow-up
request and finalizes the conversation as Cancelled.
- collateral_cancel_of_pending_fetch_conversation_reports_explicit_error_via_owning_request:
a FollowUpSubmitted{is_for_same_conversation:true} cancellation does not mark the
conversation Cancelled, and the drained result serializes to an explicit
FetchConversationResult.Error via the wire conversion instead of being dropped.
- Added a #[cfg(test)] BlocklistAIActionModel::enqueue_pending_action_for_test helper to
support the above without needing the real FetchConversation executor (network/local-fs
I/O) to run.
While investigating why FetchConversationResult::Cancelled could be produced in the literal reported sequence (single prompt, no Stop, no second prompt), I confirmed no client cancellation path fires in that sequence at all -- see the PR description for the full enumeration. This commit does not confirm the reported Cancelled symptom's root cause; the exact trigger remains unconfirmed. It does fix a distinct, real bug found along the way: send_follow_up_for_conversation defers sending a completed action's result when has_active_stream_for_conversation is still true for its conversation. That flag only clears once AfterStreamFinished has been processed for the triggering stream (which calls PendingResponseStreams::cleanup_stream and Conversation::cleanup_completed_response_stream) -- receiving the stream's own Finished(Done) response event alone (mark_request_completed) does not clear it. If an action resolves fast enough (e.g. FetchConversation served from an already in-memory conversation, an already-Ready future) its FinishedAction can be processed in that window, and nothing ever retried the deferred send: the result was stranded in finished_action_results indefinitely. This surfaces as the affected inline block (e.g. a ConversationSearch subagent block) staying in its "still running" state forever, not as "Cancelled" -- so it likely is not what the customer saw, but it is a real, independent, previously-unhandled defect. Fix: flush_stranded_follow_up_for_conversation, called from the AfterStreamFinished normal-completion path once a stream's bookkeeping is actually cleared, retries any conversation left with finished-but-undelivered action results and nothing else pending/running. Diagnostics: added log lines at the two points that matter for a future occurrence -- where send_follow_up_for_conversation defers (observable at the moment a result would otherwise silently vanish) and where the flush recovers a stranded result.
…hange
Two findings from review, both addressed:
1. flush_stranded_follow_up_for_conversation ran unconditionally in the
AfterStreamFinished normal-completion path, including when a stream failed
recoverably after dispatching client actions and scheduled a resume for once it
finishes. If the flush fired in that case, it sent its own follow-up request and
then the scheduled resume fired too, sending a second, redundant request that can
race or collide with the first.
Fix: flush_stranded_follow_up_for_conversation now returns whether it actually
sent a request. The AfterStreamFinished handler reads that before deciding
whether to arm the scheduled resume, and skips scheduling it when the flush
already took ownership of resuming the conversation.
Added flushed_stranded_result_suppresses_a_scheduled_resume_for_the_same_stream,
which drives a fast action's completion alongside a scheduled resume for the same
stream and asserts exactly one request is sent and the resume is never armed.
Added ResponseStream::set_pending_resume_for_test to stage that lifecycle without
needing a real recoverable failure.
2. Reverted crates/ai/src/agent/action_result/convert.rs's Cancelled -> explicit
Error conversion for FetchConversationResult back to
ConvertToAPITypeError::Ignore, along with its doc comments and its two
convert_tests.rs tests. There is no established client path that delivers that
result to ConversationSearchAgent (a mixed user-query/tool-result request is
intercepted by the server as a cancel before the subagent can consume it), so
keeping it broadened the generic cancel wire contract without a proven delivery
contract behind it. crates/ai/src/agent/action_result/{mod.rs,convert.rs,
convert_tests.rs} are now byte-identical to their pre-PR state.
Updated the collateral-cancel controller test to drop its now-invalid
wire-serialization assertion, keeping the still-valid assertion that collateral
cancellation does not mark the conversation Cancelled.
ResponseStream::begin_recovery creates pending_resume with self.recovery.next_attempt() precisely so the retry/resume chain stays bounded by one shared counter. The flush/resume mutual-exclusion fix suppresses that resume when a stranded result flushes instead, but the replacement follow-up request was going out with RecoveryBudget::fresh() -- resetting the counter to zero attempts used on every server action -> fast completion -> post-action recoverable failure sequence, breaking the one-shared-budget invariant. Threaded an explicit RecoveryBudget parameter through send_follow_up_for_conversation and flush_stranded_follow_up_for_conversation. The AfterStreamFinished handler now reads pending_resume before flushing (independent of the stream cleanup that follows) and passes its already-charged budget into the flush; the two pre-existing call sites that aren't replacing a resume keep passing RecoveryBudget::fresh(), unchanged. Added ResponseStream::recovery_for_test and PendingResponseStreams::stream_for_test so flushed_stranded_result_suppresses_a_scheduled_resume_for_the_same_stream can assert the replacement request's stream actually inherited the resume's charged budget (attempts_used == 1), not merely that one request went out. Also fixed both stranding regression tests to use the conversation's real root task_id instead of a fake mismatched one, which is necessary for the flushed request's stream registration to be observable via stream_ids_for_conversation.
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
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.



Description
Investigating https://github.com/warpdotdev/warp-server/issues/15659 ("
search_conversation_historycancels when referencing other conversations via@Conversations/<convo:>"), I could not confirm the root cause of the reportedCancelledsymptom, and I want to be explicit about that up front. This PR does not claim to fix #15659. It:What's refuted, with evidence
FetchConversationResult::Cancelledis only ever produced byAIAgentActionType::cancelled_result(), called fromcancel_running_async_action/cancel_all_running_async_actions_for_conversation/cancel_pending_actioninapp/src/ai/blocklist/action_model.rsandexecute.rs. I enumerated every call site that can trigger this chain:app/src/ai/blocklist/controller.rs:stop_local_agent_conversation(Stop button), conversation deactivation (switching away while in progress), clear-buffer, delete/clear conversations, revert, shell exit, andsend_query's own pre-cancel for a second query submitted to the same conversation (CancellationReason::FollowUpSubmitted{is_for_same_conversation:true}).app/src/terminal/view.rs: the same Stop/deactivation/clear-buffer paths.Every one of these requires an explicit trigger beyond the single reported prompt. Neither
ManuallyCancellednorFollowUpSubmitted(the two hypotheses tested across this PR's revisions) fires in the literal reported sequence. So the customer-visibleCancelledstate is not explained by the client'sFetchConversationcancellation machinery in that sequence — both hypotheses are refuted.I also explored, and reverted, a client-side wire-serialization change (mapping
FetchConversationResult::Cancelledto an explicitErrorinstead ofConvertToAPITypeError::Ignore). It broadened the generic cancel wire contract without a proven delivery path — a mixed user-query/tool-result request is intercepted by the server as a cancel beforeConversationSearchAgentcan consume any explicit result — so it's reverted;crates/ai/src/agent/action_result/{mod.rs,convert.rs,convert_tests.rs}are back to their pre-PR state.The stranding defect (fixed here)
Tracing
send_follow_up_for_conversation(app/src/ai/blocklist/controller.rs), it defers sending a completed action's result whenhas_active_stream_for_conversationis stilltruefor its conversation. That flag only clears once the triggering stream'sAfterStreamFinishedevent has been fully processed (PendingResponseStreams::cleanup_stream+Conversation::cleanup_completed_response_stream). Receiving the stream's ownFinished(Done)response event by itself (mark_request_completed) does not clear it — confirmed by reading both call sites inapp/src/ai/agent/conversation.rs.If an action resolves fast enough — e.g.
FetchConversationserved from an already in-memory conversation viaload_conversation_by_server_token's fast path, which returns an already-Readyfuture — itsFinishedActioncan be processed in that window. Nothing ever retried the deferred send, so the result was stranded infinished_action_resultsindefinitely.This is a real, independent, previously-unhandled defect. Its symptom is the affected inline block (e.g. a
ConversationSearchsubagent block) staying in its "still running" spinner state forever — not "Cancelled". So it likely is not what the customer saw, but it's worth fixing regardless.Fix:
flush_stranded_follow_up_for_conversation, called from theAfterStreamFinishednormal-completion path once a stream's bookkeeping is actually cleared, retries delivery for any conversation left with finished-but-undelivered action results and nothing else pending/running.Follow-on race, also fixed: the flush ran unconditionally on every
AfterStreamFinished{cancellation: None}, including when the stream failed recoverably after dispatching client actions and scheduled an automatic resume for once it finishes (ResponseStream::pending_resume). If the flush fired in that case, its follow-up request and the scheduled resume could both fire, racing or colliding.flush_stranded_follow_up_for_conversationnow reports whether it actually sent a request; theAfterStreamFinishedhandler skips arming the scheduled resume when it did, since that request already takes over resuming the conversation.Recovery-budget inheritance, also fixed:
ResponseStream::begin_recoverychargespending_resumewithself.recovery.next_attempt()precisely so the retry/resume chain stays bounded by one shared counter (MAX_RECOVERY_ATTEMPTS). Suppressing that resume in favor of the flush was sending the replacement follow-up withRecoveryBudget::fresh(), silently resetting the counter on every server action → fast completion → post-action recoverable failure sequence.send_follow_up_for_conversationandflush_stranded_follow_up_for_conversationnow take an explicitRecoveryBudget; theAfterStreamFinishedhandler readspending_resumebefore flushing and passes its already-charged budget through when it replaces that resume.Diagnostics: log lines at the two points that matter for a future occurrence: where
send_follow_up_for_conversationdefers (the moment a result would otherwise silently vanish), and where the flush recovers a stranded result.Open lead for whoever picks this up next
I could not rule out (or confirm) that the server's own subagent/turn-limit handling —
ConversationSearchAgent.Run'sRunWithoutStoppingValidator, retry exhaustion,OnFatalErrorinwarp-server/logic/ai/multi_agent/agents/conversation_search/agent.go— ever produces aCancel-shaped outcome for the delegating tool call independent of any client action. That's a real code path I read but could not trace end-to-end without a live request trace. I don't have prod debugger/BQ access to the two request IDs from the original Slack thread; getting that access would very likely disambiguate this quickly.Testing
app/src/ai/blocklist/controller_tests.rs:manual_cancel_of_pending_fetch_conversation_does_not_revive_conversation— a genuineManuallyCancelledcancellation sends no follow-up and finalizesCancelled.collateral_cancel_of_pending_fetch_conversation_does_not_mark_conversation_cancelled— aFollowUpSubmitted{is_for_same_conversation:true}cancellation does not mark the conversationCancelled.stranded_action_result_is_flushed_once_triggering_stream_completes— drives the exact race (aFinished(Done)response event processed, then a fast action'sFinishedAction, then the stream's ownAfterStreamFinished), asserting the result is deferred (not dropped) and then delivered (not stranded).flushed_stranded_result_suppresses_a_scheduled_resume_for_the_same_stream— drives a fast action's completion alongside a scheduled resume for the same stream, asserting exactly one follow-up request is sent, the scheduled resume is never armed, and the replacement request's stream inherited the resume's already-charged budget (attempts_used() == 1) rather than a fresh one.crates/ai/src/agent/action_result/mod_tests.rs:cancelled_fetch_conversation_does_not_unconditionally_trigger_a_follow_up_request(unrelated to the reverted conversion; guardsshould_trigger_request_upon_completion's unchanged behavior).BlocklistAIActionModel::enqueue_pending_action_for_test,ResponseStream::emit_stream_finished_for_test,ResponseStream::set_pending_resume_for_test,ResponseStream::recovery_for_test,PendingResponseStreams::stream_for_test.Validation commands run:
cargo test -p ai agent::action_result— 6 passed, 0 failed.cargo test -p warp --lib controller::tests::— 31 passed, 0 failed.cargo test -p warp --lib ai::blocklist::— 783/783 passed on the final run; one earlier intermittent failure insecret_redaction::test::test_detect_secrets_no_regexes_configured, confirmed pre-existing and unrelated: it passes in isolation, passes on a clean checkout of this branch with my changes stashed, and flakes non-deterministically across repeated runs regardless of these changes (order/global-state dependent in the test binary, not caused by this diff).cargo fmton all touched files — clean.cargo clippy -p ai --lib --all-features --tests -- -D warnings— clean.cargo clippy -p warp --lib -- -D warnings— clean.crates/ai/src/agent/action_result/{mod.rs,convert.rs,convert_tests.rs}verified byte-identical to pre-PRmasterviagit diff --statagainst the merge base.Visual proof
This is a backend/protocol-level change (Rust conversion + stream/action bookkeeping logic) with no UI code touched, and I could not reproduce the customer's exact flow in this sandbox (no running Warp client or server backend, no authenticated account, no second real conversation to reference). Stated explicitly per the verification skill rather than substituting a code walkthrough for a capture; the regression tests above are the verification available here.
./script/run(not applicable — no UI change, and not achievable in this sandbox; see note above)Agent Mode
CHANGELOG-NONE