Skip to content

Commit 06db79a

Browse files
QUALITY-1759: require a genuine checkpoint for hibernation, avoid double-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.
1 parent 9651e19 commit 06db79a

2 files changed

Lines changed: 94 additions & 13 deletions

File tree

app/src/ai/agent_sdk/driver.rs

Lines changed: 50 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -588,6 +588,12 @@ pub struct AgentDriver {
588588
/// pure no-op for local and disabled runs.
589589
snapshot_file_writer: Option<snapshot::DeclarationsWriterHandle>,
590590

591+
/// Set once the `WaitForEventsYielded` handler has already attempted the final
592+
/// checkpoint upload for this run's clean exit (see `SDKConversationOutputStatus::
593+
/// YieldedForEvents` / `YieldedForEventsCheckpointFailed`), so the unconditional
594+
/// end-of-run cleanup in `run` does not redundantly upload a second final checkpoint.
595+
final_checkpoint_uploaded_for_yield: bool,
596+
591597
/// Whether the driver should skip dispatching the initial
592598
/// `StartFromAmbientRunPrompt`. Mirror of `AgentDriverOptions::skip_initial_turn`,
593599
/// sourced from the `--skip-initial-turn` CLI flag. Read by `execute_run`
@@ -1059,6 +1065,7 @@ impl AgentDriver {
10591065
parent_run_id: parent_run_id_for_self,
10601066
third_party_harness_model_config,
10611067
snapshot_file_writer,
1068+
final_checkpoint_uploaded_for_yield: false,
10621069
skip_initial_turn,
10631070
strict_mcp_startup,
10641071
mcp_startup_timeout: mcp_startup_timeout.unwrap_or(MCP_SERVER_STARTUP_TIMEOUT),
@@ -1108,6 +1115,7 @@ impl AgentDriver {
11081115
parent_run_id: None,
11091116
third_party_harness_model_config: None,
11101117
snapshot_file_writer: None,
1118+
final_checkpoint_uploaded_for_yield: false,
11111119
skip_initial_turn: false,
11121120
strict_mcp_startup: false,
11131121
mcp_startup_timeout: MCP_SERVER_STARTUP_TIMEOUT,
@@ -1327,7 +1335,13 @@ impl AgentDriver {
13271335
(reason={actual_reason:?}): {finalization_result:?}"
13281336
);
13291337
}
1330-
let _ = Self::run_snapshot_upload(&foreground).await;
1338+
let already_uploaded_for_yield = foreground
1339+
.spawn(|me, _| me.final_checkpoint_uploaded_for_yield)
1340+
.await
1341+
.unwrap_or(false);
1342+
if !already_uploaded_for_yield {
1343+
let _ = Self::run_snapshot_upload(&foreground, false).await;
1344+
}
13311345

13321346
if tx.send(result).is_err() {
13331347
report_error!("Caller did not wait for agent driver to finish");
@@ -4078,11 +4092,23 @@ impl AgentDriver {
40784092
let checkpoint_task_id = me.task_id;
40794093
let checkpoint_foreground = ctx.spawner();
40804094
ctx.spawn(
4081-
async move { Self::run_snapshot_upload(&checkpoint_foreground).await },
4082-
move |_me, checkpoint_succeeded, _ctx| {
4095+
async move {
4096+
// `require_genuine_checkpoint=true`: a hibernation may only
4097+
// claim success when a resumable checkpoint was actually
4098+
// produced. Unlike most callers of `run_snapshot_upload`, a
4099+
// skipped upload (checkpointing disabled, no cloud task id,
4100+
// or `--no-snapshot`) is not "nothing to fail" here -- it
4101+
// means the closed wait tool call has no persisted result to
4102+
// rehydrate from, so it must not be reported as success.
4103+
Self::run_snapshot_upload(&checkpoint_foreground, true).await
4104+
},
4105+
move |me, checkpoint_succeeded, _ctx| {
40834106
log::info!(
40844107
"Ambient agent idle lifecycle: event=run_completion_immediate task_id={checkpoint_task_id:?} terminal_view_id={terminal_id:?} outcome=yielded_for_events checkpoint_succeeded={checkpoint_succeeded}"
40854108
);
4109+
// Attempted (successfully or not) here, so the unconditional
4110+
// end-of-run cleanup must not upload a second final checkpoint.
4111+
me.final_checkpoint_uploaded_for_yield = true;
40864112
let status = if checkpoint_succeeded {
40874113
SDKConversationOutputStatus::YieldedForEvents
40884114
} else {
@@ -4492,22 +4518,30 @@ impl AgentDriver {
44924518
/// Invoke the end-of-run snapshot upload pipeline if the feature flag is enabled and this
44934519
/// driver is associated with a cloud task. Returns whether the upload is known to have
44944520
/// succeeded. Most callers treat this as best-effort and ignore the result; a caller that
4495-
/// must prove resumability (e.g. the `wait_for_events` warm-wait-window yield) inspects it.
4521+
/// must prove resumability (e.g. the `wait_for_events` warm-wait-window yield) inspects it
4522+
/// and passes `require_genuine_checkpoint=true`.
44964523
///
4497-
/// A skipped upload (feature disabled, no task id, `--no-snapshot`) counts as success:
4498-
/// there was nothing to fail. The periodic checkpoint coordinator is itself best-effort and
4499-
/// does not report per-attempt success, so reaching its `finalize` call also counts as
4500-
/// success; only the legacy one-shot upload's timeout is currently observable as failure.
4524+
/// When `require_genuine_checkpoint` is `false` (the default for ordinary end-of-run
4525+
/// cleanup), a skipped upload (feature disabled, no task id, `--no-snapshot`) counts as
4526+
/// success: there was nothing to fail. When it is `true`, those same skip conditions
4527+
/// return `false` instead: a hibernation cannot claim a checkpoint it never produced, so
4528+
/// the caller must treat that as a failed yield rather than a clean one. The periodic
4529+
/// checkpoint coordinator is itself best-effort and does not report per-attempt success,
4530+
/// so reaching its `finalize` call also counts as success; only the legacy one-shot
4531+
/// upload's timeout is currently observable as failure.
45014532
#[tracing::instrument(skip_all, fields(tags.cloud_agent = true))]
4502-
async fn run_snapshot_upload(spawner: &ModelSpawner<Self>) -> bool {
4533+
async fn run_snapshot_upload(
4534+
spawner: &ModelSpawner<Self>,
4535+
require_genuine_checkpoint: bool,
4536+
) -> bool {
45034537
if !FeatureFlag::OzHandoff.is_enabled() {
4504-
return true;
4538+
return !require_genuine_checkpoint;
45054539
}
45064540

45074541
// Snapshot upload is only meaningful for cloud task runs, so short-circuit before
45084542
// pulling the rest of the context onto this task.
45094543
let Ok((
4510-
Some(task_id),
4544+
maybe_task_id,
45114545
snapshot_disabled,
45124546
upload_timeout,
45134547
script_timeout,
@@ -4524,11 +4558,14 @@ impl AgentDriver {
45244558
})
45254559
.await
45264560
else {
4527-
return true;
4561+
return !require_genuine_checkpoint;
4562+
};
4563+
let Some(task_id) = maybe_task_id else {
4564+
return !require_genuine_checkpoint;
45284565
};
45294566
if snapshot_disabled {
45304567
log::info!("Skipping snapshot upload because --no-snapshot was specified");
4531-
return true;
4568+
return !require_genuine_checkpoint;
45324569
}
45334570

45344571
// An active coordinator replaces the legacy upload below. Budget must come from

app/src/ai/agent_sdk/driver_tests.rs

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1016,6 +1016,50 @@ fn sandbox_deadline_reports_error_before_exit() {
10161016
);
10171017
}
10181018

1019+
// ── QUALITY-1759 revision: genuine-checkpoint gate and no double upload ────
1020+
1021+
/// `run_snapshot_upload` with `require_genuine_checkpoint=true` must not claim success
1022+
/// when checkpointing is not actually available (here: `FeatureFlag::OzHandoff` disabled,
1023+
/// its default state in tests unless explicitly overridden). A hibernation cannot claim a
1024+
/// checkpoint it never produced.
1025+
#[test]
1026+
fn run_snapshot_upload_fails_genuine_requirement_when_oz_handoff_disabled() {
1027+
App::test((), |mut app| async move {
1028+
initialize_app_for_terminal_view(&mut app);
1029+
let temp = TempDir::new().unwrap();
1030+
let working_dir = dunce::canonicalize(temp.path()).unwrap();
1031+
let terminal_view = add_window_with_terminal(&mut app, None);
1032+
let driver_handle = app.add_model(|ctx| {
1033+
let terminal_driver =
1034+
super::terminal::TerminalDriver::create_from_existing_view(terminal_view, ctx);
1035+
AgentDriver::new_for_test(working_dir.clone(), terminal_driver, ctx)
1036+
});
1037+
1038+
let (tx, rx) = futures::channel::oneshot::channel::<(bool, bool)>();
1039+
driver_handle.update(&mut app, |_, ctx| {
1040+
let spawner = ctx.spawner();
1041+
ctx.spawn(
1042+
async move {
1043+
let genuine = AgentDriver::run_snapshot_upload(&spawner, true).await;
1044+
let best_effort = AgentDriver::run_snapshot_upload(&spawner, false).await;
1045+
let _ = tx.send((genuine, best_effort));
1046+
},
1047+
|_, _, _| {},
1048+
);
1049+
});
1050+
let (genuine_result, best_effort_result) = rx.await.unwrap();
1051+
1052+
assert!(
1053+
!genuine_result,
1054+
"a hibernation must not claim checkpoint success when checkpointing is unavailable"
1055+
);
1056+
assert!(
1057+
best_effort_result,
1058+
"ordinary best-effort cleanup must still treat a skipped upload as success"
1059+
);
1060+
});
1061+
}
1062+
10191063
#[test]
10201064
fn task_env_vars_include_parent_run_id_when_present() {
10211065
let task_id: AmbientAgentTaskId = "550e8400-e29b-41d4-a716-446655440000".parse().unwrap();

0 commit comments

Comments
 (0)