QUALITY-1759: hibernate on first wait_for_events timeout (client) - #15330
Draft
warp-agent-staging[bot] wants to merge 12 commits into
Draft
QUALITY-1759: hibernate on first wait_for_events timeout (client)#15330warp-agent-staging[bot] wants to merge 12 commits into
warp-agent-staging[bot] wants to merge 12 commits into
Conversation
…e-waiting - WaitForEventsExecutor emits an internal WarmWaitWindowExpired event when the watchdog fires, instead of completing the tool call directly. The pending wait stays registered until yielded. - BlocklistAIActionExecutor bridges that into a WaitForEventsYielded executor event, closing the tool call with an empty Completed result without going through the shared cancellation path. - BlocklistAIActionModel handles the yield without emitting FinishedAction, so no follow-up model request is sent and the conversation stays WaitingForEvents. - BlocklistAIHistoryModel gains a dedicated WaitForEventsYielded event so the driver (wired in a later commit) can observe the yield without relying on a ConversationStatus change. - Gated behind FeatureFlag::HibernateOnFirstWaitTimeout; disabling the flag restores the previous repeated long-poll behavior. - Every other exhaustive match over BlocklistAIHistoryEvent / BlocklistAIActionEvent is updated for the two new variants.
…port WARP_SANDBOX_DEADLINE as ERROR - SDKConversationOutputStatus gains YieldedForEvents and YieldedForEventsCheckpointFailed. Both skip idle_on_complete / idle_on_fail entirely so the sandbox always closes immediately. - execute_run's history subscription reacts to the new BlocklistAIHistoryEvent::WaitForEventsYielded by uploading the final checkpoint (run_snapshot_upload, now returning success/failure) and only then resolving the run \xe2\x80\x94 checkpoint failure reports AgentDriverError::WaitForEventsCheckpointFailed (ERROR) instead of a clean yield, since resumability isn't proven. - The WARP_SANDBOX_DEADLINE timer branch now returns AgentDriverError::SandboxDeadlineReached (reported via the existing report_driver_error path) instead of Ok(()), with the exact plan-derived status message from the product spec.
…deadline - wait_for_events_tests.rs: fires the watchdog directly (bypassing the real 1770s timer) to assert WarmWaitWindowExpired is emitted with the resolved duration and used_fallback flag, that the pending entry survives until take_pending_wait, and that disabling the feature flag restores the old direct-completion behavior. - driver_tests.rs: asserts YieldedForEvents/YieldedForEventsCheckpointFailed never defer by idle windows, and that the checkpoint-failure path reports a non-success driver result with the exact sandbox-deadline status message.
Contributor
Author
|
This PR was generated with Warp. Comment |
…ble-uploading it (review findings 6, 7) Finding 6: run_snapshot_upload returned true for a skipped upload (OzHandoff disabled, no cloud task id, or --no-snapshot), which is the right contract for its ordinary best-effort callers but let a wait_for_events hibernation claim a checkpoint it never produced. Add a require_genuine_checkpoint parameter: when true, those same skip conditions now return false, so the hibernation path reports YieldedForEventsCheckpointFailed (-> ERROR) instead of a clean yield whenever checkpointing capability isn't actually available. Finding 7: a successful hibernation uploaded the final checkpoint once from the WaitForEventsYielded handler, then again, unconditionally, from the end-of-run cleanup in run(), breaking the one-checkpoint invariant. Added final_checkpoint_uploaded_for_yield on AgentDriver, set once the yield path has attempted its upload (success or failure), and checked before the end-of-run cleanup's own upload so it never re-uploads. Adds run_snapshot_upload_fails_genuine_requirement_when_oz_handoff_disabled to cover the new gate directly.
…ibernation (re-review finding 3)
run_snapshot_upload(..., require_genuine_checkpoint=true) still lied
when the periodic checkpoint coordinator was in play:
CheckpointCoordinatorHandle::finalize acked with unit regardless of
whether its attempt committed, failed, was skipped, timed out, or its
result channel was dropped, so the caller unconditionally treated
reaching that call as success.
CheckpointCoordinatorHandle::finalize now returns a FinalizeOutcome
(Committed { generation } / NotCommitted { result } / Unknown), and
run_snapshot_upload only reports success for require_genuine_checkpoint
when the outcome is a genuine Committed. Ordinary best-effort callers
are unaffected (any outcome still counts as success for them).
Adds coordinator_finalize_reports_not_committed_when_final_attempt_fails
to cover the fix directly.
…inding 4, client part 1) New CancellationReason variant, mapped to CancellationOutcome::FinalizedExternally so the ordinary cancellation-to-status machinery does not stamp a status when it fires: a dedicated controller path (added next) will write Success directly, since the server already recorded this run as SUCCEEDED via parent completion and CANCELLED would contradict that authoritative state.
…ding 4, client part 2) Recognizes the new self-scoped run_succeeded_by_parent_completion wire event in handle_event_batch (the one case safe to act on despite the otherwise-correct self-event filter in convert_lifecycle_events), emits a new ParentCompletionSucceeded streamer event, and tombstones the run via the existing killed-run mechanism so a later out-of-order buffered event cannot restore an earlier local status. BlocklistAIController::stop_conversation_for_parent_completed_success cancels the in-flight stream/actions with the new ParentOrchestrationCompleted reason and writes Success directly, mirroring the existing fail_conversation_due_to_shell_exit pattern for a dedicated-status-write cancellation outcome.
…op path (finding 4, client part 3) handle_event_batch_emits_parent_completion_succeeded_for_self_scoped_event proves the new wire event emits ParentCompletionSucceeded exactly once and tombstones the run. stop_conversation_for_parent_completed_success_reports_success_and_survives_manual_cancel proves the controller method finalizes Success (not Cancelled) and that a later ManuallyCancelled pane-close cannot overwrite it, mirroring the existing shell-exit regression test's shape.
…entSelf (re-review finding 1) classify_family_event had no arm for run_succeeded_by_parent_completion, so it fell through to Opaque and was silently dropped by the unified family-drain path (used whenever OrchestrationUnifiedStack is enabled), never reaching handle_event_batch's ParentCompletionSucceeded detection. On the unified stack a local descendant would have kept running indefinitely despite the server-authoritative SUCCEEDED -- the exact bug this feature exists to fix. Added drain_family_events_primary_forwards_self_scoped_parent_completion_event to cover the unified-stack path explicitly (the previously-added handle_event_batch test only covered the legacy per-conversation path).
…re-review finding 2) The previous fix reused remember_killed_run_id, a permanent blanket suppression appropriate for a genuinely killed run that never legitimately resumes under the same run_id. A follow-up that resumes a previously SUCCEEDED run under the same task/run_id would have had its run_in_progress, messages, and lifecycle events dropped forever. Added a dedicated parent_completion_terminal_sequence map (with the same bounded-eviction shape as killed_run_id_order) that records only the terminal event's sequence number, and a filtering pass in handle_event_batch that drops events for that run_id at or below the recorded sequence -- a later resumed execution's events, necessarily at a higher sequence, are delivered normally. Added handle_event_batch_parent_completion_tombstone_is_sequence_bounded to cover both halves: an out-of-order duplicate at the terminal sequence is still dropped, and a resumed execution's event at a higher sequence is delivered. Updated the existing test's assertion to check the new mechanism instead of killed_run_ids.
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
Implements the Warp client (Rust) side of the two-repo spec for QUALITY-1759 ("wait_for_events keeps child sandbox alive until MaxInstanceRuntime"). The warp-server side is tracked separately in warpdotdev/warp-server#15629, on branch
factory/quality-1759-wait-hibernation-spec.Cross-repository contract (must match warp-server #15629)
idle_timeout_seconds=1800on thewait_for_eventstool call and persists_server_wait_for_events_yielded=trueat finalization time.watchdog_timeout_for_stamped_seconds, unchanged).WaitForEvents(Completed)result in durable conversation history to close the tool call, but does not start a follow-up model request.idle_on_completewindow, no duplicate upload.BLOCKED.run_succeeded_by_parent_completionevent (with durable retry via the server's reconciliation job); this client recognizes it on both the legacy and unified family-drain event paths and stops the local conversation, projectingSuccess(neverCANCELLED, which would contradict the server's authoritative state), with a sequence-bounded (not permanent) tombstone so a later legitimately-resumed run under the same run_id is not blocked forever.Client-side changes
WaitForEventsExecutor(wait_for_events.rs): on watchdog fire, emits an internalWarmWaitWindowExpiredevent instead of completing the tool call directly over the channel. The pending wait stays registered until yielded. Feature-flag gated (see below); when disabled, falls back to the previous behavior (direct completion → follow-up → re-wait).BlocklistAIActionExecutor: bridgesWarmWaitWindowExpiredinto aWaitForEventsYieldedexecutor event — removes the action fromasync_executing_actionsbefore dropping the pending sender (so the generic async-completion callback discards the future's defensive resolution instead of double-emitting), and closes the tool call with an emptyCompletedresult (distinct from the shared cancellation path, which would reportCancelled).BlocklistAIActionModel: handles the yield by recording the result into durable history without emittingFinishedAction— this is what suppresses the follow-up model request; the conversation status staysWaitingForEvents.BlocklistAIHistoryModel: newWaitForEventsYieldedevent (conversation status is intentionally unchanged, so this can't ride along withUpdatedConversationStatus).AgentDriver: reacts toWaitForEventsYieldedby uploading the final checkpoint (run_snapshot_upload, now gated on genuine commitment -- see below) and then ending the run with a newYieldedForEvents/YieldedForEventsCheckpointFailedoutcome that always skipsidle_on_complete/idle_on_fail. Checkpoint failure is reported as an executionERROR(WaitForEventsCheckpointFailed) rather than a clean yield, since resumability isn't proven.run_snapshot_uploadgenuine-commitment gate: takes arequire_genuine_checkpointparameter. The yield path passestrueand requires the periodicCheckpointCoordinatorHandle::finalizeto report a realFinalizeOutcome::Committed, not just an unconditional ack -- aSkipped/Failed/timed-out/dropped-receiver coordinator outcome, or checkpointing disabled/--no-snapshot, now correctly reportsfalseinstead of a false success.final_checkpoint_uploaded_for_yieldflag onAgentDriveris set once the yield handler has attempted its upload, and checked before the end-of-run cleanup's own unconditional upload so it is skipped when already attempted.AgentDriver/WARP_SANDBOX_DEADLINE: the deadline timer branch now returnsAgentDriverError::SandboxDeadlineReached(reported via the existingreport_driver_errorpath asAgentTaskState::Error) instead ofOk(()), with the exact plan-derived message from the product spec.CancellationReason::ParentOrchestrationCompleted,orchestration_event_streamer.rs,controller.rs): recognizes the new self-scopedrun_succeeded_by_parent_completionwire event on both event-delivery paths --handle_event_batch(legacy per-conversation drain) andclassify_family_event(unified family drain, used wheneverOrchestrationUnifiedStackis enabled; this event has no recognized lifecycle type, so without an explicitParentSelfclassification it silently fell through toOpaqueand was dropped). Emits a newOrchestrationEventStreamerEvent::ParentCompletionSucceededand records only the terminal event's sequence number in a dedicated, bounded map (not the permanentkilled_run_idsset) -- events for that run_id at or below that sequence are dropped, but a later legitimately-resumed execution's events (necessarily at a higher sequence) are still delivered normally.BlocklistAIController::stop_conversation_for_parent_completed_successcancels the in-flight stream/actions with the new reason and writesSuccessdirectly -- mirroring the existingfail_conversation_due_to_shell_exitpattern for a dedicated-status-write cancellation outcome.FeatureFlag::HibernateOnFirstWaitTimeout(added toDOGFOOD_FLAGS). Disabling it restores the old repeated 30-minute long-poll behavior. The local-descendant-stop path is not flag-gated separately: it only fires on a wire event the old server never sends, so it is a no-op against an old server regardless.Rollout: server-first deployment of warpdotdev/warp-server#15629 is required and safe (an old client just keeps its legacy long-poll loop against the new 1800s stamp, and never receives the new local-stop event either). Enabling
FeatureFlag::HibernateOnFirstWaitTimeouton a client talking to an old (pre-TransitionTaskToWaitBlocked) server is unsafe: that server leaves a clean yielded exitINPROGRESS, stranding the task and its billing until the 12-hour stale-task job. Do not enable or distribute this flag before warp-server#15629 is deployed.Deviations from the spec as written
BlocklistAIController/controller.rswould need direct changes ("do not callsend_follow_up_for_conversation"). In the actual code, the follow-up decision lives entirely in a subscription toBlocklistAIActionEvent::FinishedAction. Since the yield path never emitsFinishedAction(it emits a distinctWaitForEventsYieldedevent instead), the follow-up is structurally never triggered. Confirmed correct on re-review.log::info!, not a newTelemetryEventvariant. Accepted by review as shippable. Follow-up filed: QUALITY-1759 follow-up: formal telemetry event for wait_for_events hibernation, not just logs #15335.Revision history (adversarial re-review)
Three rounds of review findings were addressed on this PR:
Round 1 (findings 6, 7, 8 on this repo):
run_snapshot_uploadtreated a skipped upload the same as a successful one. Added therequire_genuine_checkpointgate described above.final_checkpoint_uploaded_for_yield.Round 2 (finding 3 and finding 4 on this repo):
CheckpointCoordinatorHandle::finalizeunconditionally acked.finalizenow returns aFinalizeOutcome(Committed{generation}/NotCommitted{result}/Unknown), andrun_snapshot_uploadonly treats the coordinator path as genuinely committed forFinalizeOutcome::Committed.Round 3 (findings 1 and 2 on this repo):
classify_family_eventhad no arm for the new event type, so it fell through toOpaqueand was silently dropped -- the local child would have kept running indefinitely on the unified stack despite the server-authoritativeSUCCEEDED. Added an explicitParentSelfclassification for it, plusdrain_family_events_primary_forwards_self_scoped_parent_completion_eventcovering that path specifically (the round-2 test only covered the legacyhandle_event_batchpath).killed_run_ids, a permanent blanket suppression meant for a genuinely killed run that never resumes under the same run_id. A follow-up that legitimately resumes a previouslySUCCEEDEDrun under the same run_id would have had itsrun_in_progress, messages, and lifecycle events dropped forever. Replaced it with a dedicated, sequence-bounded map that only discards events at or below the terminal event's own sequence, described above.handle_event_batch_parent_completion_tombstone_is_sequence_boundedcovers both halves (an out-of-order duplicate at the terminal sequence is still dropped; a resumed run's event at a higher sequence is delivered).Linked Issue
ready-to-specorready-to-implement.Testing
Ran the exact validation commands from the spec, plus targeted runs for the new local-descendant-stop code:
cargo test -p warp wait_for_events— 12 passedcargo test -p warp agent_driver— this literal filter matches 0 tests in this codebase (no test path contains that substring); rancargo test -p warp ai::agent_sdk::driverinstead — 367 passedcargo test -p warp orchestration_event_streamer— 73 passed (70 pre-existing + 3 new across rounds 2-3: the self-scoped emit test, the unified-family-drain forwarding test, and the sequence-bounded tombstone test)cargo test -p warp ai::blocklist::controller::tests— 7 passed (6 pre-existing + 1 new:stop_conversation_for_parent_completed_success_reports_success_and_survives_manual_cancel)cargo fmt --all -- --check— cleancargo clippy -p warp --lib --tests --no-deps -- -D warnings— clean. (--all-features, matching AGENTS.md's workspace-wide invocation, fails identically on this branch and onmasterwith pre-existing, unrelated errors inwarp_completer'sargument/v2.rsand anexecution_profilesfeature-combination break; neither is touched by this diff.)New tests added across all three rounds:
wait_for_events_tests.rs(watchdog behavior, fallback,used_fallbackreporting),driver_tests.rs/error_classification_tests.rs(yield outcomes skip idle windows;SandboxDeadlineReached/WaitForEventsCheckpointFailedclassify asERRORwith the exact spec message; the coordinator genuine-checkpoint gate),checkpoint_coordinator_tests.rs(FinalizeOutcomereporting for a failed final attempt),orchestration_event_streamer_tests.rs(the self-scoped event emitsParentCompletionSucceededexactly once via both drain paths, tombstones sequence-bounded, and a resumed run's later event is still delivered), andcontroller_tests.rs(the new stop path reportsSuccessand survives a later manual cancel)../script/run— not done; this is a headless cloud-agent change with no local sandbox/orchestration environment available in this run. The behavior is covered by the unit/integration tests above instead. The full cross-process scenario (real remote timeout -> BLOCKED -> resume; real parent-completion cascade -> local descendant stop) needs a live orchestration environment this sandbox does not have.Agent Mode