diff --git a/Cargo.lock b/Cargo.lock index 7be5bc4e6..c0879f5e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5375,6 +5375,7 @@ dependencies = [ "toml", "unicode-width", "uuid", + "worker-runtime", ] [[package]] diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index 9a61c328b..8d1e509c1 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -31,5 +31,6 @@ async-trait.workspace = true agen.workspace = true [dev-dependencies] +worker-runtime.workspace = true async-trait.workspace = true tempfile = { workspace = true } diff --git a/crates/tui/src/backend_worker_picker.rs b/crates/tui/src/backend_worker_picker.rs index 4e97a8943..6821d1c19 100644 --- a/crates/tui/src/backend_worker_picker.rs +++ b/crates/tui/src/backend_worker_picker.rs @@ -24,13 +24,19 @@ use crate::inline_terminal::with_inline_terminal; const MAX_ROWS: usize = 10; const VIEWPORT_LINES: u16 = MAX_ROWS as u16 + 4; +#[derive(Debug)] +enum RestoreAttemptError { + Conflict, + Unknown(io::Error), +} + #[async_trait::async_trait] trait BackendWorkerLifecycle { async fn restore( &self, target: &client::BackendRuntimeTarget, request: client::BackendWorkerRestoreRequest, - ) -> Result; + ) -> Result; async fn observe( &self, @@ -46,17 +52,17 @@ impl BackendWorkerLifecycle for LiveBackendWorkerLifecycle { &self, target: &client::BackendRuntimeTarget, request: client::BackendWorkerRestoreRequest, - ) -> Result { + ) -> Result { restore_backend_worker(target, request) .await .map_err(|error| { if error.is_restore_observation_conflict() { - return io::Error::other("Restore observation changed; refresh the Worker list and select again. No new Restore was started."); + return RestoreAttemptError::Conflict; } - io::Error::other(format!( + RestoreAttemptError::Unknown(io::Error::other(format!( "failed to restore Backend Worker {}: {error}", target.display_label() - )) + ))) }) } @@ -152,6 +158,7 @@ pub(crate) async fn run( attach_target, intent, restore_request, + confirm_restore_recovery, ) .await?; return console::run_backend_runtime(attach_target).await; @@ -163,13 +170,61 @@ async fn prepare_selected_worker( mut target: client::BackendRuntimeTarget, intent: BackendWorkerPickerIntent, restore_request: Option, + mut confirm_recovery: impl FnMut(&str) -> io::Result, ) -> Result { if intent == BackendWorkerPickerIntent::Resume { let request = restore_request.ok_or_else(|| { io::Error::other("Worker has no current Restore observation; refresh and select again") })?; - let response = lifecycle.restore(&target, request).await?; - ensure_restore_accepted(&response)?; + loop { + let unknown = match lifecycle.restore(&target, request.clone()).await { + Ok(response) if response.result.state == BackendWorkerRestoreState::Accepted => { + break; + } + Ok(response) => { + let error = ensure_restore_accepted(&response).unwrap_err(); + if response.result.state != BackendWorkerRestoreState::ReconciliationRequired { + return Err(error); + } + error + } + Err(RestoreAttemptError::Conflict) => { + return Err(io::Error::other( + "Restore observation changed; refresh the Worker list and select again. No new Restore was started.", + )); + } + Err(RestoreAttemptError::Unknown(error)) => error, + }; + // Refresh is evidence only: GET cannot reconcile a pending Restore. + // A changed observation must never replace this admitted/unknown tuple. + let observation = match lifecycle.observe(&target).await { + Ok(observation) => match observation.observation { + WorkerSessionAvailability::LiveProtocol => "live_protocol".into(), + WorkerSessionAvailability::RetainedSnapshot { .. } => { + "retained_snapshot".into() + } + WorkerSessionAvailability::Unavailable { reason, message } => format!( + "{reason:?}: {}", + message.chars().take(512).collect::() + ), + }, + Err(error) => format!("refresh failed: {error}"), + }; + let detail = format!( + "Restore outcome unknown/pending: {}. Current observation: {}. Recovery request_id={}, expected_observation_token={}. Retry only this exact request; do not start a new Restore intent.", + unknown.to_string().chars().take(512).collect::(), + observation.chars().take(512).collect::(), + request.request_id, + request.expected_observation_token, + ); + // Every subsequent POST requires a new explicit user confirmation. + let retry = confirm_recovery(&detail).map_err(|error| { + io::Error::other(format!("{detail}; recovery prompt failed: {error}")) + })?; + if !retry { + return Err(io::Error::other(detail)); + } + } let observation = lifecycle.observe(&target).await?; require_live_resume_observation(observation.observation)?; return Ok(target); @@ -180,6 +235,29 @@ async fn prepare_selected_worker( Ok(target) } +fn confirm_restore_recovery(detail: &str) -> io::Result { + with_inline_terminal(8, |terminal| { + loop { + terminal.draw(|frame| { + let message = vec![ + Line::from("Restore needs explicit result recovery"), + Line::from("Enter: retry the SAME request Esc/Ctrl-C: cancel recovery"), + Line::from(detail), + ]; + frame.render_widget( + Paragraph::new(message).wrap(ratatui::widgets::Wrap { trim: false }), + frame.area(), + ); + })?; + match poll_event()? { + Some(Action::Submit) => return Ok(true), + Some(Action::Cancel) => return Ok(false), + _ => {} + } + } + }) +} + fn is_resume_candidate(worker: &BackendWorkerSummary) -> bool { worker.availability == protocol::subscription::SubscriptionWorkerAvailability::Observed && worker.state == "stopped" @@ -670,7 +748,7 @@ mod tests { &self, _target: &client::BackendRuntimeTarget, _request: client::BackendWorkerRestoreRequest, - ) -> Result { + ) -> Result { self.calls.lock().unwrap().push("restore"); Ok(self.restore_response.clone()) } @@ -701,6 +779,312 @@ mod tests { serde_json::from_value(value).unwrap() } + // Exercise the existing picker flow against real Runtime admission, not a + // fake request -> operation-ID association. Only execution and transport are + // faked; Runtime owns the pending operation and receipt reconciliation. + #[derive(Default)] + struct PendingRuntimeBackend { + operations: std::sync::Mutex>, + } + + impl worker_runtime::execution::WorkerExecutionBackend for PendingRuntimeBackend { + fn backend_id(&self) -> &str { + "tui-recovery-test" + } + + fn spawn_worker( + &self, + _: worker_runtime::execution::WorkerExecutionSpawnRequest, + ) -> worker_runtime::execution::WorkerExecutionSpawnResult { + worker_runtime::execution::WorkerExecutionSpawnResult::connected( + protocol::WorkerStateSnapshot { + last_command_id: 0, + last_finished_submission_request_id: None, + state: protocol::WorkerState::Idle, + }, + Vec::new(), + ) + } + + fn restore_worker( + &self, + request: worker_runtime::execution::WorkerExecutionRestoreRequest, + ) -> worker_runtime::execution::WorkerExecutionSpawnResult { + use worker_runtime::execution::*; + let mut operations = self.operations.lock().unwrap(); + operations.push(request.operation_id); + if operations.len() == 1 { + WorkerExecutionSpawnResult::ReconciliationRequired { + result: WorkerExecutionResult::errored( + WorkerExecutionOperation::Restore, + "unknown execution acknowledgement", + ), + worker_state: None, + workdir_attachments: Vec::new(), + } + } else { + WorkerExecutionSpawnResult::connected( + protocol::WorkerStateSnapshot { + last_command_id: 0, + last_finished_submission_request_id: None, + state: protocol::WorkerState::Idle, + }, + Vec::new(), + ) + } + } + + fn stop_worker( + &self, + _: &worker_runtime::identity::WorkerRef, + ) -> worker_runtime::execution::WorkerExecutionResult { + worker_runtime::execution::WorkerExecutionResult::accepted( + worker_runtime::execution::WorkerExecutionOperation::Stop, + ) + } + + fn dispatch_input( + &self, + _: &worker_runtime::identity::WorkerRef, + _: worker_runtime::interaction::WorkerInput, + ) -> worker_runtime::execution::WorkerExecutionResult { + panic!("Restore must not dispatch input") + } + } + + struct RuntimeRecoveryLifecycle { + runtime: worker_runtime::Runtime, + worker: worker_runtime::identity::WorkerRef, + lose_first_reply: bool, + requests: std::sync::Mutex< + Vec<( + client::BackendRuntimeTarget, + client::BackendWorkerRestoreRequest, + )>, + >, + observations: std::sync::Mutex, + } + + #[async_trait::async_trait] + impl BackendWorkerLifecycle for RuntimeRecoveryLifecycle { + async fn restore( + &self, + target: &client::BackendRuntimeTarget, + request: client::BackendWorkerRestoreRequest, + ) -> Result { + self.requests + .lock() + .unwrap() + .push((target.clone(), request.clone())); + assert_eq!(target.worker_id, self.worker.worker_id.to_string()); + let result = self + .runtime + .restore_worker_operation( + &self.worker, + serde_json::from_value(serde_json::to_value(request).unwrap()).unwrap(), + ) + .map_err(|error| match error { + worker_runtime::error::RuntimeError::RestoreObservationConflict { .. } => { + RestoreAttemptError::Conflict + } + error => RestoreAttemptError::Unknown(io::Error::other(error.to_string())), + })?; + if self.lose_first_reply && self.requests.lock().unwrap().len() == 1 { + return Err(RestoreAttemptError::Unknown(io::Error::new( + io::ErrorKind::ConnectionReset, + "reply lost after admission", + ))); + } + let mut response = restore_response(result.state); + response.runtime_id = target.runtime_id.clone(); + response.worker_id = target.worker_id.clone(); + Ok(response) + } + + async fn observe( + &self, + _: &client::BackendRuntimeTarget, + ) -> Result { + *self.observations.lock().unwrap() += 1; + Ok(session_observation( + serde_json::json!({"availability": "live_protocol"}), + )) + } + } + + fn pending_runtime_lifecycle( + lose_first_reply: bool, + ) -> ( + RuntimeRecoveryLifecycle, + std::sync::Arc, + client::BackendRuntimeTarget, + client::BackendWorkerRestoreRequest, + ) { + use worker_runtime::catalog::*; + let backend = std::sync::Arc::new(PendingRuntimeBackend::default()); + let runtime = + worker_runtime::Runtime::with_execution_backend(Default::default(), backend.clone()) + .unwrap(); + let archive = worker_runtime::profile_archive::ProfileSourceArchive::build( + worker_runtime::profile_archive::ProfileSourceArchiveInput { + id: "tui-recovery-profile".into(), + entrypoints: std::collections::BTreeMap::from([( + "builtin:coder".into(), + "coder.dcdl".into(), + )]), + imports: Default::default(), + sources: std::collections::BTreeMap::from([("coder.dcdl".into(), "{}".into())]), + }, + ) + .unwrap(); + let worker = runtime + .create_worker(CreateWorkerRequest { + worker_id: protocol::WorkerId::now_v7(), + create_fingerprint: "tui-recovery-create".into(), + profile: ProfileSelector::Builtin("builtin:coder".into()), + display_name: None, + profile_source: ProfileSourceArchiveSource::Embedded { archive }, + config_bundle: None, + initial_input: None, + workdir_attachment_requests: Vec::new(), + workdir_attachments: Vec::new(), + worker_observation_enabled: false, + worker_observation_grants: Vec::new(), + workspace_api: None, + memory_settings: Some(manifest::WorkspaceMemorySettingsSnapshot { + workspace_id: "local".into(), + settings_revision: 1, + language: "English".into(), + }), + subjektiv_attached: false, + }) + .unwrap(); + runtime.stop_worker(&worker.worker_ref, None).unwrap(); + let request = client::BackendWorkerRestoreRequest { + expected_observation_token: runtime + .worker_detail(&worker.worker_ref) + .unwrap() + .restore_observation_token + .unwrap(), + request_id: "tui-original-request".into(), + }; + let target = client::BackendRuntimeTarget::new( + "http://127.0.0.1:3000", + "workspace-a", + "runtime-a", + worker.worker_id.to_string(), + ); + ( + RuntimeRecoveryLifecycle { + runtime, + worker: worker.worker_ref, + lose_first_reply, + requests: Default::default(), + observations: Default::default(), + }, + backend, + target, + request, + ) + } + + #[tokio::test] + async fn resume_explicit_transport_and_pending_retries_preserve_tuple_and_runtime_operation() { + for lost_reply in [true, false] { + let (lifecycle, backend, target, request) = pending_runtime_lifecycle(lost_reply); + let mut confirmations = 0; + let prepared = prepare_selected_worker( + &lifecycle, + target.clone(), + BackendWorkerPickerIntent::Resume, + Some(request.clone()), + |detail| { + confirmations += 1; + assert_eq!( + backend.operations.lock().unwrap().len(), + 1, + "no POST before explicit retry confirmation" + ); + assert!(detail.contains(&format!("request_id={}", request.request_id))); + assert!(detail.contains(&format!( + "expected_observation_token={}", + request.expected_observation_token + ))); + assert_ne!( + lifecycle + .runtime + .worker_detail(&lifecycle.worker) + .unwrap() + .restore_observation_token + .as_ref(), + Some(&request.expected_observation_token), + "fresh observation must not replace recovery tuple" + ); + Ok(true) + }, + ) + .await + .unwrap(); + assert_eq!(prepared, target); + assert_eq!(confirmations, 1); + assert_eq!( + lifecycle.requests.lock().unwrap().as_slice(), + &[(target.clone(), request.clone()), (target, request)] + ); + let operations = backend.operations.lock().unwrap(); + assert_eq!(operations.len(), 2); + assert_eq!( + operations[0], operations[1], + "Runtime pending lifecycle operation must not be replaced" + ); + assert_eq!( + *lifecycle.observations.lock().unwrap(), + 2, + "GET after uncertainty and after acceptance" + ); + } + } + + #[tokio::test] + async fn resume_cancelled_unknown_recovery_has_no_automatic_retry_and_reports_original_identity() + { + let (lifecycle, backend, target, request) = pending_runtime_lifecycle(true); + let error = prepare_selected_worker( + &lifecycle, + target, + BackendWorkerPickerIntent::Resume, + Some(request.clone()), + |_| Ok(false), + ) + .await + .unwrap_err() + .to_string(); + assert!(error.contains(&request.request_id)); + assert!(error.contains(&request.expected_observation_token)); + assert_eq!(backend.operations.lock().unwrap().len(), 1); + assert_eq!(lifecycle.requests.lock().unwrap().len(), 1); + assert_eq!(*lifecycle.observations.lock().unwrap(), 1); + } + + #[tokio::test] + async fn resume_stale_conflict_never_offers_same_intent_retry() { + let (lifecycle, backend, target, mut request) = pending_runtime_lifecycle(false); + request.expected_observation_token = "stale-token".into(); + let error = prepare_selected_worker( + &lifecycle, + target, + BackendWorkerPickerIntent::Resume, + Some(request), + |_| panic!("stale conflict must not offer unknown-result retry"), + ) + .await + .unwrap_err() + .to_string(); + assert!(error.contains("refresh the Worker list")); + assert!(backend.operations.lock().unwrap().is_empty()); + assert_eq!(*lifecycle.observations.lock().unwrap(), 0); + } + #[tokio::test] async fn resume_restores_before_requiring_live_observation() { let lifecycle = FakeBackendWorkerLifecycle { @@ -725,6 +1109,7 @@ mod tests { expected_observation_token: "observed-worker-generation".into(), request_id: "resume-operation".into(), }), + |_| Ok(false), ) .await .unwrap(); @@ -759,6 +1144,7 @@ mod tests { expected_observation_token: "observed-worker-generation".into(), request_id: "resume-operation".into(), }), + |_| Ok(false), ) .await .unwrap_err() diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index a418d7e65..6589f6230 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -29112,9 +29112,28 @@ fn restore_internal_observed_worker( explicit.then_some(request_id.as_str()), )?; let request_id = request.request_id.clone(); - let result = - WorkspaceWorker::resolve(api, &observed.worker.runtime_id, &observed.worker.worker_id)? - .restore_internal(request)?; + let result = match WorkspaceWorker::resolve( + api, + &observed.worker.runtime_id, + &observed.worker.worker_id, + )? + .restore_internal(request) + { + Ok(result) => result, + Err(error) => { + // A definitive non-admission conflict is not unknown delivery. Retire + // only this pinned intent, without retrying it or exchanging its + // observation. A later deliberate invocation may observe anew. + if matches!(error.error, Error::RestoreObservationConflict) { + api.config_store.settle_internal_worker_restore_intent( + api.workspace_id(), + &observed.worker, + &request_id, + )?; + } + return Err(error); + } + }; if result.state != server_api::WorkerRestoreState::ReconciliationRequired { api.config_store.settle_internal_worker_restore_intent( api.workspace_id(), diff --git a/crates/workspace-server/src/server/tests/worker_operations_tests.rs b/crates/workspace-server/src/server/tests/worker_operations_tests.rs index e6cf86e84..7489e2c03 100644 --- a/crates/workspace-server/src/server/tests/worker_operations_tests.rs +++ b/crates/workspace-server/src/server/tests/worker_operations_tests.rs @@ -1279,6 +1279,181 @@ async fn restore_guard_pending_recovery_ignores_new_eligibility_and_reuses_opera ); } +#[tokio::test] +async fn restore_guard_internal_stale_conflict_retires_intent_for_later_explicit_restore() { + let workspace = tempfile::tempdir().unwrap(); + let (mut api, execution) = test_api_with_recording_backend(workspace.path()).await; + execution.accept_restores(); + for domain in ["orchestrator-restore", "subject-restore"] { + let identity = spawn_ticket_check_source(&api, domain); + let handle = + WorkspaceWorker::resolve(&api, &identity.runtime_id, &identity.worker_id).unwrap(); + handle + .stop(&WorkerOperationContext::Backend, operation_lifecycle()) + .await + .unwrap(); + let observed = api.runtime.worker(&identity).unwrap(); + let pinned = api + .config_store + .pin_internal_worker_restore_intent( + api.workspace_id(), + &identity, + domain, + observed.restore_observation_token.as_deref(), + None, + ) + .unwrap(); + // A distinct operation wins between the internal observation/pin and admission. + let competing = restore_request_for(&api, &identity, &format!("{domain}-competing")); + assert_eq!( + handle + .restore(&WorkerOperationContext::Backend, competing) + .await + .unwrap() + .state, + server_api::WorkerRestoreState::Accepted + ); + handle + .stop(&WorkerOperationContext::Backend, operation_lifecycle()) + .await + .unwrap(); + let count = execution.restore_operations.lock().unwrap().len(); + let error = restore_internal_worker_intent(&api, &identity, domain.into()).unwrap_err(); + assert!(matches!(error.error, Error::RestoreObservationConflict)); + assert_eq!( + execution.restore_operations.lock().unwrap().len(), + count, + "a conflict must not automatically exchange observation and retry" + ); + assert_eq!(api.runtime.worker(&identity).unwrap().state, "stopped"); + // Retirement, unlike in-memory refresh, must survive caller journal restart. + api.config_store = + Arc::new(SqliteWorkspaceStore::open(api.config.database_path.clone()).unwrap()); + let fresh = api.runtime.worker(&identity).unwrap(); + let next = api + .config_store + .pin_internal_worker_restore_intent( + api.workspace_id(), + &identity, + domain, + fresh.restore_observation_token.as_deref(), + None, + ) + .unwrap(); + assert_ne!(pinned.request_id, next.request_id); + assert_ne!( + pinned.expected_observation_token, + next.expected_observation_token + ); + assert_eq!( + next.expected_observation_token, + fresh.restore_observation_token.unwrap() + ); + // Only this separate deliberate invocation dispatches the new intent. + assert_eq!( + restore_internal_worker_intent(&api, &identity, domain.into()) + .unwrap() + .state, + server_api::WorkerRestoreState::Accepted + ); + assert_eq!( + execution.restore_operations.lock().unwrap().len(), + count + 1 + ); + // Explicit historical identity is immutable, never rewritten to the new token. + let original = api + .config_store + .pin_internal_worker_restore_intent( + api.workspace_id(), + &identity, + domain, + None, + Some(&pinned.request_id), + ) + .unwrap(); + assert_eq!(original, pinned); + } +} + +#[tokio::test] +async fn restore_guard_internal_conflict_same_generation_has_distinct_fresh_intents() { + let workspace = tempfile::tempdir().unwrap(); + let (api, execution) = test_api_with_recording_backend(workspace.path()).await; + execution.accept_restores(); + let identity = spawn_ticket_check_source(&api, "internal-other-owner-pending"); + let handle = WorkspaceWorker::resolve(&api, &identity.runtime_id, &identity.worker_id).unwrap(); + handle + .stop(&WorkerOperationContext::Backend, operation_lifecycle()) + .await + .unwrap(); + let original = restore_request_for(&api, &identity, "original-pending-owner"); + *execution.restore_failure.lock().unwrap() = Some( + worker_runtime::execution::WorkerExecutionSpawnResult::Errored( + worker_runtime::execution::WorkerExecutionResult::errored( + worker_runtime::execution::WorkerExecutionOperation::Restore, + "unknown delivery", + ), + ), + ); + assert_eq!( + handle + .restore(&WorkerOperationContext::Backend, original.clone()) + .await + .unwrap() + .state, + server_api::WorkerRestoreState::ReconciliationRequired + ); + let operation = execution.restore_operations.lock().unwrap()[0]; + let observed = api.runtime.worker(&identity).unwrap(); + let count = execution.restore_operations.lock().unwrap().len(); + for domain in ["orchestrator-restore", "subject-restore"] { + let mut prior = None; + for _ in 0..2 { + let pinned = api + .config_store + .pin_internal_worker_restore_intent( + api.workspace_id(), + &identity, + domain, + observed.restore_observation_token.as_deref(), + None, + ) + .unwrap(); + if let Some(prior) = prior.replace(pinned.clone()) { + assert_ne!(pinned.request_id, prior.request_id); + assert_eq!( + pinned.expected_observation_token, + prior.expected_observation_token + ); + } + let error = restore_internal_worker_intent(&api, &identity, domain.into()).unwrap_err(); + assert!(matches!(error.error, Error::RestoreObservationConflict)); + assert_eq!(execution.restore_operations.lock().unwrap().len(), count); + } + } + // Retiring competing rejected intents does not retire or replace an admitted owner. + assert_eq!( + handle + .restore(&WorkerOperationContext::Backend, original) + .await + .unwrap() + .state, + server_api::WorkerRestoreState::Accepted + ); + assert_eq!( + execution.reconcile_operations.lock().unwrap().as_slice(), + &[operation] + ); + assert!( + execution + .restore_operations + .lock() + .unwrap() + .iter() + .all(|id| *id == operation) + ); +} + #[tokio::test] async fn restore_guard_internal_intent_survives_client_store_restart_without_replacing_token() { let workspace = tempfile::tempdir().unwrap(); diff --git a/crates/workspace-server/src/store.rs b/crates/workspace-server/src/store.rs index 4e31dbc8f..cda452604 100644 --- a/crates/workspace-server/src/store.rs +++ b/crates/workspace-server/src/store.rs @@ -2196,7 +2196,11 @@ impl SqliteWorkspaceStore { return Ok(server_api::WorkerRestoreRequest { request_id, expected_observation_token }); } let observed_token = observed_token.ok_or(Error::RestoreObservationConflict)?; - let request_id = explicit_request_id.map(str::to_owned).unwrap_or_else(|| format!("{domain_key}:{observed_token}")); + // A separate deliberate invocation after a settled conflict may + // still observe the same generation (e.g. another owner is pending). + // Give it a new identity rather than colliding with a retired row; + // retries of unsettled delivery always return the pinned tuple above. + let request_id = explicit_request_id.map(str::to_owned).unwrap_or_else(|| format!("{domain_key}:{}", uuid::Uuid::now_v7())); tx.execute("INSERT INTO worker_restore_intents(workspace_id,runtime_id,worker_id,domain_key,request_id,expected_token,settled) VALUES(?1,?2,?3,?4,?5,?6,0)", params![workspace_id, worker.runtime_id, worker.worker_id, domain_key, request_id, observed_token])?; Ok(server_api::WorkerRestoreRequest { request_id, expected_observation_token: observed_token.to_owned() }) diff --git a/docs/architecture/worker-restore-observations.md b/docs/architecture/worker-restore-observations.md index eab92478a..4efcb74cd 100644 --- a/docs/architecture/worker-restore-observations.md +++ b/docs/architecture/worker-restore-observations.md @@ -43,8 +43,8 @@ Preparation completion is attached to that same pending operation, not a replace | Workspace alias, scoped and Worker-control HTTP | Mandatory public request; common target/auth reception and the same Runtime admission | | Embedded / remote Runtime providers and management HTTP | Typed Runtime request and coordination transport, including original observation and lifecycle operation owner | | `WorkerRestore` Tool | Observed token in Tool arguments and durable Tool-call-derived request identity | -| Existing TUI Restore/resume path | Selected row's observation token and retained request; no new TUI entrance | -| Internal Coder, Orchestrator, subject reuse and compensation | Domain intent journal pins the request/token before dispatch and retains it through unknown result and restart; it does not own lifecycle status or bypass Runtime validation | +| Existing TUI Restore/resume path | Selected row's token and exact request retained through the existing flow. Unknown/pending results refresh Session observation and prompt for an explicit same-tuple retry; cancellation does not retry and reports the original tuple. Conflict exits for fresh selection, never exchanges the failed intent. No new TUI Restore entrance | +| Internal Coder, Orchestrator, subject reuse and compensation | Domain intent journal pins the request/token before dispatch and retains it through unknown result and restart. A definitive observation conflict retires only that rejected intent without automatic retry; a later deliberate invocation may pin fresh observation with a fresh request ID. It does not own lifecycle status or bypass Runtime validation | | Runtime startup / pending reconciliation | Private recovery of the existing persisted Restore owner and operation; not an external unguarded new-intent endpoint | The Workspace internal intent journal stores client retry identity only. Runtime remains the lifecycle authority; it is not a second Restore state machine, Worker ledger or cross-screen lock. diff --git a/docs/report/2026-10-06-t701-validation.md b/docs/report/2026-10-06-t701-validation.md index 462af3db3..fa9025f75 100644 --- a/docs/report/2026-10-06-t701-validation.md +++ b/docs/report/2026-10-06-t701-validation.md @@ -47,3 +47,32 @@ After those fixes, a full Server run reported **618 passed / 1 failed**, solely Additional exploratory Runtime feature checks did **not** pass: `--no-default-features` test/check cannot resolve existing unconditional `workspace_request` / `reqwest` imports; the test target additionally references disabled `fs_store`. HTTP-only check also lacks the existing retention and futures dependencies. Inspection of original HEAD confirms those imports/gates already exist; no isolated baseline build was performed for these configurations. These are recorded as failed exploratory checks, not semantic passes or newly supported configurations. The default Runtime configuration, which contains fs-store and ws-server, is exercised in full above. This Ticket does not redesign unrelated Runtime feature gating. Raw session validation logs are retained under ignored `target/t701-validation/`, not shipped as product source. Immutable published source, independent verdict and any subsequent fix/review evidence belong to the explicit repository Merge Request. + +## Independent review and caller recovery fixes + +The independent Reviewer recorded `request_changes` on MR `01a11064-502b-7222-9494-b1952fbcb1d5`, source `fdce12a47bdd408e6e1a0275dfb218951f1a2f1f`, for two concrete omitted recovery scenarios. These are implementation defects, not baseline failures: + +- **`restore_tui_loses_recovery_identity`:** The existing TUI Resume/Restore flow exited on unknown/pending result and discarded the tuple. It now retains the request throughout that flow, refreshes Session observation without treating GET as settlement, and offers explicit same-tuple recovery confirmation. Each additional POST requires user confirmation. Conflict and terminal rejection do not offer same-intent recovery; cancellation makes no additional POST and reports the original identity. No new Restore entrance, new execution operation, input or automatic retry loop is introduced. New TUI tests use the **real Runtime**, with only execution and lost transport acknowledgements faked, to assert identical target/request and unchanged lifecycle operation ID across transport and reconciliation-required recovery. Runtime is a test-only TUI dependency. +- **`restore_internal_conflict_never_retires_intent`:** A definitive stale conflict now settles only its pinned client intent before returning the original conflict; unknown/pending delivery remains pinned. A later deliberate generic orchestrator/subject invocation can use fresh observation. Fresh generic requests use a new UUID identity, including when two rejected invocations see the same generation, without changing unsettled retry identity. Tests cover both generic domains, ABA between pin and admission, journal restart, no automatic retry, immutable historical identity, repeated same-generation conflict, and preservation/reconciliation of a different admitted pending operation. + +The first compile of the new TUI fixture omitted `last_finished_submission_request_id` in two WorkerState snapshots and failed; the fixtures were corrected before the passing focused/full tests below. + +Post-fix validation: + +| Command | Evidence | +| --- | --- | +| root `cargo check` | Passed | +| TUI focused `backend_worker_picker::tests::resume_` | 5 passed, including 3 new real-Runtime recovery tests | +| Server focused `restore_guard_internal_` | 3 passed, including both new conflict retirement tests and existing unknown/restart recovery | +| `cargo test -p tui -- --test-threads=4` | 334 passed | +| `cargo test -p worker-runtime -- --test-threads=4` | 332 library + 34 binary tests passed | +| First post-fix full Server run | 620 passed / 1 failed: the same inherited stale-checker assertion | +| Post-fix isolated checker | Failed the same assertion; not hidden or treated as a pass | +| Additional post-fix full Server run, same four threads and no skips | 621 library + 11 binary tests passed | +| Formatting / diff whitespace checks | Passed | + +The Web/API/Worker/client source is unchanged by this fix; its initial published-source validation above remains applicable. The additional full Server pass does not resolve or erase either post-fix checker failure. Raw fix logs are `target/t701-validation/{tui-recovery-fix-target,server-intent-fix-target,fix-root-check,fix-full-tui,fix-full-worker-runtime,fix-full-yoi-workspace-server,fix-checker-isolated,fix-server-additional}.log`. + +### Review tooling boundary encountered + +A first builtin Reviewer spawn with read-only delegation was rejected with `Merge Request Reviewer SubWorkers must include writable delegated scope`, despite the read-only source-review role. The accepted configuration kept product source read-only and delegated write/command authority only to isolated ignored `target/t701-review-validation` scratch. The Reviewer did not implement changes or delegate implementation. A clearer API distinction between read-only review source and writable validation scratch would avoid this misleading setup failure.