From 510795f1c5f9ea2f49e1a214a5ab2ab2b38afe1d Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 2 Sep 2026 00:25:21 +0900 Subject: [PATCH] fix: fence Workdir removal retries --- crates/workspace-server/src/server.rs | 188 +++++++++--------- crates/workspace-server/src/store.rs | 17 +- .../workspace-server/src/workdir_removal.rs | 60 +++++- docs/README.md | 15 +- docs/design/durable-operations.md | 45 +++++ 5 files changed, 216 insertions(+), 109 deletions(-) create mode 100644 docs/design/durable-operations.md diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 2498265c..b75048e9 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -9650,10 +9650,7 @@ fn execute_reserved_workdir_removal( ); }; if status.summary.cleanliness.as_deref() != Some("clean") - || !matches!( - status.summary.status, - WorkingDirectoryStatusKind::Active | WorkingDirectoryStatusKind::CleanupPending - ) + || status.summary.status != WorkingDirectoryStatusKind::Active { return api.config_store.complete_workdir_removal_retained( &operation, @@ -12687,50 +12684,33 @@ fn finalize_spawn_compensation_after_worker_delete( if context.cleanup_spawned_workdir { if let Some(workdir_id) = context.prepared_workdir_id { - let runtime_cleanup_succeeded = match api - .runtime - .cleanup_working_directory(&worker.worker.runtime_id, workdir_id) - { - Ok(result) if result.state == WorkerOperationState::Accepted => true, - Ok(result) => { - diagnostics.push(spawn_compensation_diagnostic( - "worker_spawn_compensation_workdir_cleanup_failed", - format!( - "Runtime did not clean up spawn-created Workdir `{workdir_id}` for Worker {}:{}: state={:?}; {}", - worker.worker.runtime_id, - worker.worker.worker_id, - result.state, - runtime_diagnostics_message(&result.diagnostics) - ), - )); - false - } - Err(error) => { - diagnostics.push(spawn_compensation_diagnostic( - "worker_spawn_compensation_workdir_cleanup_failed", - format!( - "Failed to clean up spawn-created Workdir `{workdir_id}` for Worker {}:{}: {}", - worker.worker.runtime_id, - worker.worker.worker_id, - error.message() - ), - )); - false - } - }; - if runtime_cleanup_succeeded { - if let Err(error) = api - .store - .delete_workdir_registry(&api.config.workspace_id, workdir_id) - { - diagnostics.push(spawn_compensation_diagnostic( - "worker_spawn_compensation_workdir_registry_delete_failed", - format!( - "Failed to remove Backend Workdir registry `{workdir_id}` after Runtime cleanup: {}", - sanitize_backend_error(&error.to_string()) - ), - )); - } + match execute_workdir_removal( + api, + workdir_id, + "backend:worker_spawn_compensation", + "remove Workdir created by rejected Worker spawn", + ) { + Ok(result) + if result.disposition == WorkingDirectoryRemovalDisposition::Removed => {} + Ok(result) => diagnostics.push(spawn_compensation_diagnostic( + "worker_spawn_compensation_workdir_cleanup_failed", + format!( + "Durable removal retained spawn-created Workdir `{workdir_id}` for Worker {}:{}: disposition={:?}, retryable={}", + worker.worker.runtime_id, + worker.worker.worker_id, + result.disposition, + result.retryable, + ), + )), + Err(error) => diagnostics.push(spawn_compensation_diagnostic( + "worker_spawn_compensation_workdir_cleanup_failed", + format!( + "Failed to reserve durable removal for spawn-created Workdir `{workdir_id}` for Worker {}:{}: {}", + worker.worker.runtime_id, + worker.worker.worker_id, + sanitize_backend_error(&error.to_string()) + ), + )), } } } @@ -14553,25 +14533,6 @@ fn sync_runtime_workdir_observations( Ok(response.diagnostics) } -fn persist_workdir_cleanup_observation( - api: &WorkspaceApi, - runtime_id: &str, - summary: &WorkingDirectorySummary, -) -> ApiResult<()> { - if summary.status == WorkingDirectoryStatusKind::NotFound { - if let Some(record) = api.store.get_workdir_registry( - &api.config.workspace_id, - summary.working_directory_id.as_str(), - )? { - persist_workdir_not_found(api, record)?; - } - } else { - let record = workdir_record_from_summary(api, runtime_id, summary); - api.store.upsert_workdir_registry(&record)?; - } - Ok(()) -} - fn workdir_runtime_miss_is_not_found(diagnostics: &[RuntimeDiagnostic]) -> bool { diagnostics .iter() @@ -22132,7 +22093,7 @@ mod tests { } #[tokio::test] - async fn confirmed_runtime_miss_removes_registry_record_but_unknown_is_retained() { + async fn provider_not_found_observation_is_retained_until_durable_removal_commits() { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let api = test_api(workspace.path()).await; @@ -22154,11 +22115,13 @@ mod tests { ) .unwrap(); - assert!( + assert_eq!( api.store .get_workdir_registry(TEST_WORKSPACE_ID, "deleted-workdir") .unwrap() - .is_none() + .unwrap() + .materialization_status, + "not_found" ); seed_cleanup_workdir(&api, "unknown-workdir", "present", "clean"); @@ -22188,7 +22151,7 @@ mod tests { } #[tokio::test] - async fn cleanup_not_found_observation_removes_registry_record() { + async fn cleanup_not_found_observation_marks_registry_for_durable_removal() { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let api = test_api(workspace.path()).await; @@ -22199,16 +22162,15 @@ mod tests { .get_workdir_registry(TEST_WORKSPACE_ID, working_directory_id) .unwrap() .unwrap(); - let mut summary = workdir_summary_from_record(&record); - summary.status = WorkingDirectoryStatusKind::NotFound; + persist_workdir_not_found(&api, record).unwrap(); - persist_workdir_cleanup_observation(&api, "runtime-test", &summary).unwrap(); - - assert!( + assert_eq!( api.store .get_workdir_registry(TEST_WORKSPACE_ID, working_directory_id) .unwrap() - .is_none() + .unwrap() + .materialization_status, + "not_found" ); } @@ -22325,8 +22287,33 @@ mod tests { )); } + #[test] + fn only_exact_provider_not_found_is_removal_evidence() { + let not_found = crate::hosts::RuntimeWorkingDirectoryResult { + state: WorkerOperationState::Rejected, + working_directory: None, + diagnostics: vec![RuntimeDiagnostic { + code: "working_directory_not_found".to_string(), + severity: DiagnosticSeverity::Error, + message: "missing".to_string(), + }], + }; + assert!(runtime_reports_workdir_not_found(¬_found)); + + let unknown = crate::hosts::RuntimeWorkingDirectoryResult { + state: WorkerOperationState::Rejected, + working_directory: None, + diagnostics: vec![RuntimeDiagnostic { + code: "working_directory_provider_timeout".to_string(), + severity: DiagnosticSeverity::Error, + message: "timeout".to_string(), + }], + }; + assert!(!runtime_reports_workdir_not_found(&unknown)); + } + #[tokio::test] - async fn durable_workdir_removal_replays_completed_provider_not_found_result() { + async fn durable_workdir_removal_retries_provider_unavailable_without_deleting_registry() { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let api = test_api(workspace.path()).await; @@ -22341,25 +22328,28 @@ mod tests { .unwrap(); assert_eq!( first.disposition, - WorkingDirectoryRemovalDisposition::Removed, - "response: {first:?}" + WorkingDirectoryRemovalDisposition::AttentionRequired + ); + assert!(first.retryable); + assert_eq!( + first.failure_category.as_deref(), + Some("runtime_unavailable") ); - assert!(!first.retryable); assert!( api.store .get_workdir_registry(&api.config.workspace_id, "missing-clean-workdir") .unwrap() - .is_none() + .is_some() ); - let replay = execute_workdir_removal( + let retry = execute_workdir_removal( &api, "missing-clean-workdir", "account:owner", "remove stale clean Workdir", ) .unwrap(); - assert_eq!(replay, first); + assert_eq!(retry, first); let operation = api .config_store .find_workdir_removal_operation_by_intent( @@ -22370,7 +22360,7 @@ mod tests { ) .unwrap() .unwrap(); - assert_eq!(operation.attempt_count, 1); + assert_eq!(operation.attempt_count, 2); } #[tokio::test] @@ -22406,7 +22396,7 @@ mod tests { } #[tokio::test] - async fn recoverable_workdir_removal_converges_after_provider_side_effect() { + async fn recovery_retries_same_operation_and_retains_unknown_provider_result() { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let api = test_api(workspace.path()).await; @@ -22429,16 +22419,17 @@ mod tests { .get_workdir_removal_operation(&api.config.workspace_id, &intent.operation_id) .unwrap() .unwrap(); - assert_eq!(operation.state, WorkdirRemovalOperationState::Completed); + assert_eq!(operation.state, WorkdirRemovalOperationState::Failed); + assert!(operation.retryable); assert_eq!( - operation.disposition, - Some(WorkdirRemovalDisposition::Removed) + operation.failure_category.as_deref(), + Some("runtime_unavailable") ); assert!( api.store .get_workdir_registry(&api.config.workspace_id, "recovery-workdir") .unwrap() - .is_none() + .is_some() ); } @@ -22602,7 +22593,7 @@ mod tests { } #[tokio::test] - async fn cleanup_execution_requires_dirty_confirmation_and_deletes_removed_record() { + async fn cleanup_execution_retains_dirty_and_requires_fresh_provider_not_found() { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let api = test_api(workspace.path()).await; @@ -22631,10 +22622,15 @@ mod tests { workdir_target_ids: vec![dirty_target], confirm_dirty_discard_target_ids: Vec::new(), }; + let retained = execute_runtime_cleanup(&api, "runtime-test", missing_confirmation) + .await + .unwrap_or_else(|err| panic!("cleanup execution: {}", err.error)); + assert_eq!(retained.results[0].status, "retained"); assert!( - execute_runtime_cleanup(&api, "runtime-test", missing_confirmation) - .await - .is_err() + api.store + .get_workdir_registry(&api.config.workspace_id, "workdir-dirty") + .unwrap() + .is_some() ); let delete_removed = ExecuteRuntimeCleanupRequest { expected_plan_revision: plan.revision, @@ -22646,12 +22642,12 @@ mod tests { let response = execute_runtime_cleanup(&api, "runtime-test", delete_removed) .await .unwrap_or_else(|err| panic!("cleanup execution: {}", err.error)); - assert_eq!(response.results[0].status, "deleted"); + assert_eq!(response.results[0].status, "attention_required"); assert!( api.store .get_workdir_registry(&api.config.workspace_id, "workdir-not-found") .unwrap() - .is_none() + .is_some() ); } diff --git a/crates/workspace-server/src/store.rs b/crates/workspace-server/src/store.rs index 271bd05c..de2cd372 100644 --- a/crates/workspace-server/src/store.rs +++ b/crates/workspace-server/src/store.rs @@ -4889,6 +4889,19 @@ impl ControlPlaneStore for SqliteWorkspaceStore { "Workdir {workdir_id} is not registered in Workspace {workspace_id}" ))); } + let removal_pending: bool = tx.query_row( + r#"SELECT EXISTS( + SELECT 1 FROM workdir_removal_operations + WHERE workspace_id = ?1 AND workdir_id = ?2 AND state = 'pending' + )"#, + params![workspace_id, workdir_id], + |row| row.get(0), + )?; + if removal_pending { + return Err(Error::WorkdirAttachmentConflict(format!( + "Workdir {workdir_id} has a pending durable removal operation" + ))); + } let occupied: bool = tx.query_row( r#"SELECT EXISTS( SELECT 1 FROM worker_workdir_links @@ -12182,13 +12195,13 @@ INSERT INTO worker_registry ( configure_sqlite(&conn).unwrap(); apply_migrations(&conn).unwrap(); conn.execute( - "INSERT INTO __yoi_schema_migrations (version, name) VALUES (49, 'future')", + "INSERT INTO __yoi_schema_migrations (version, name) VALUES (50, 'future')", [], ) .unwrap(); let error = apply_migrations(&conn).unwrap_err().to_string(); - assert!(error.contains("schema version 49 is newer"), "{error}"); + assert!(error.contains("schema version 50 is newer"), "{error}"); assert!(error.contains("refusing to serve"), "{error}"); } diff --git a/crates/workspace-server/src/workdir_removal.rs b/crates/workspace-server/src/workdir_removal.rs index 28abc84c..91ed972f 100644 --- a/crates/workspace-server/src/workdir_removal.rs +++ b/crates/workspace-server/src/workdir_removal.rs @@ -106,9 +106,7 @@ CREATE TABLE workdir_removal_operations ( updated_at TEXT NOT NULL, completed_at TEXT, PRIMARY KEY (workspace_id, operation_id), - FOREIGN KEY (workspace_id) REFERENCES workspaces(workspace_id) ON DELETE CASCADE, - FOREIGN KEY (workspace_id, repository_id) - REFERENCES repositories(workspace_id, repository_id) ON DELETE RESTRICT + FOREIGN KEY (workspace_id) REFERENCES workspaces(workspace_id) ON DELETE CASCADE ); CREATE INDEX idx_workdir_removal_operations_recovery ON workdir_removal_operations(workspace_id, state, retryable, updated_at); @@ -278,6 +276,16 @@ impl SqliteWorkspaceStore { operation.working_directory_id )))?; require_operation_materialization(operation, ¤t)?; + let repository_exists: bool = conn.query_row( + "SELECT EXISTS(SELECT 1 FROM repositories WHERE workspace_id=?1 AND repository_id=?2)", + params![operation.workspace_id, operation.repository_id], + |row| row.get(0), + )?; + if !repository_exists { + return Err(Error::RegistryInconsistency( + "Workdir Repository authority is missing".to_string(), + )); + } let active_attachment: bool = conn.query_row( "SELECT EXISTS(SELECT 1 FROM worker_workdir_links WHERE workspace_id=?1 AND workdir_id=?2 AND unlinked_at IS NULL)", params![operation.workspace_id, operation.working_directory_id], @@ -590,7 +598,7 @@ fn validate_intent(intent: &WorkdirRemovalIntent) -> Result<()> { } fn validate_bounded(label: &str, value: &str, max: usize) -> Result<()> { - if value.trim().is_empty() || value.len() > max { + if value.trim().is_empty() || value.len() > max || value.chars().any(char::is_control) { return Err(Error::InvalidInput(format!( "{label} must be non-empty and at most {max} bytes" ))); @@ -924,6 +932,50 @@ mod tests { assert_eq!(replay, completed); } + #[tokio::test] + async fn pending_removal_fences_new_attachment_and_retry_rereads_live_reservation() { + let (store, workdir) = seeded_store().await; + let intent = + workdir_removal_intent(&workdir, "workspace-api", "remove clean Workdir").unwrap(); + let pending = store.reserve_workdir_removal_operation(&intent).unwrap(); + + let error = store + .reserve_worker_workdir_attachment("workspace-a", "workdir-a", "reservation-a", "2") + .unwrap_err(); + assert!(matches!(error, Error::WorkdirAttachmentConflict(_))); + + let failed = store + .fail_workdir_removal_operation(&pending, "provider_unavailable", true) + .unwrap(); + store + .reserve_worker_workdir_attachment("workspace-a", "workdir-a", "reservation-b", "3") + .unwrap(); + let retry = store + .begin_workdir_removal_attempt( + &failed.workspace_id, + &failed.operation_id, + &failed.request_fingerprint, + ) + .unwrap(); + let guards = store.workdir_removal_guards(&retry).unwrap(); + assert!( + guards + .iter() + .any(|guard| guard.category == "pending_attachment") + ); + let retained = store + .complete_workdir_removal_retained( + &retry, + WorkdirRemovalDisposition::Retained, + "blocked_by_live_authority", + ) + .unwrap(); + assert_eq!( + retained.disposition, + Some(WorkdirRemovalDisposition::Retained) + ); + } + #[tokio::test] async fn retained_completion_keeps_registry_and_is_auditable() { let (store, workdir) = seeded_store().await; diff --git a/docs/README.md b/docs/README.md index d02c1065..534bf851 100644 --- a/docs/README.md +++ b/docs/README.md @@ -17,13 +17,14 @@ It is not a dumping ground for external research, old plans, API inventories, or 9. [`development/plugin-development.md`](development/plugin-development.md) — how to build, package, enable, and inspect Yoi Plugins. 10. [`design/memory-knowledge.md`](design/memory-knowledge.md) — generated memory and audit records. 11. [`design/workspace-kanban-orchestrator-runtime.md`](design/workspace-kanban-orchestrator-runtime.md) — how Kanban operations become durable orchestration events and backend-internal routing decisions. -12. [`design/workspace-runtime-docker.md`](design/workspace-runtime-docker.md) — the WebUI / Backend / Runtime split, Docker image layout, worker launch path, and workdir materialization boundary. -13. [`development/server-runtime-auth.md`](development/server-runtime-auth.md) — manual Workspace Server / Runtime public-key exchange and authenticated Runtime startup checks. -14. [`development/work-items.md`](development/work-items.md) — how project work is recorded and reviewed. -15. [`development/rust-testing-strategy.md`](development/rust-testing-strategy.md) — what Yoi Rust tests should prove, where they belong, and how to name them. -16. [`development/validation.md`](development/validation.md) — how to check changes. -17. [`development/workspace-schema-migrations.md`](development/workspace-schema-migrations.md) — how to preflight, apply, verify, and roll back control-plane SQLite schema changes. -18. [`design/standalone-agent-host.md`](design/standalone-agent-host.md) — in-process standalone Worker host の依存方向、authority、lifecycle、非目標。 +12. [`design/durable-operations.md`](design/durable-operations.md) — durable Backend intents that cross Runtime/provider side-effect boundaries, including Workdir removal. +13. [`design/workspace-runtime-docker.md`](design/workspace-runtime-docker.md) — the WebUI / Backend / Runtime split, Docker image layout, worker launch path, and workdir materialization boundary. +14. [`development/server-runtime-auth.md`](development/server-runtime-auth.md) — manual Workspace Server / Runtime public-key exchange and authenticated Runtime startup checks. +15. [`development/work-items.md`](development/work-items.md) — how project work is recorded and reviewed. +16. [`development/rust-testing-strategy.md`](development/rust-testing-strategy.md) — what Yoi Rust tests should prove, where they belong, and how to name them. +17. [`development/validation.md`](development/validation.md) — how to check changes. +18. [`development/workspace-schema-migrations.md`](development/workspace-schema-migrations.md) — how to preflight, apply, verify, and roll back control-plane SQLite schema changes. +19. [`design/standalone-agent-host.md`](design/standalone-agent-host.md) — in-process standalone Worker host の依存方向、authority、lifecycle、非目標。 ## What belongs here diff --git a/docs/design/durable-operations.md b/docs/design/durable-operations.md new file mode 100644 index 00000000..b4799a1d --- /dev/null +++ b/docs/design/durable-operations.md @@ -0,0 +1,45 @@ +# Durable side-effect operations + +A durable side-effect operation is a Backend-owned intent whose execution crosses an authority boundary, such as a Runtime/provider mutation, and must converge across duplicate requests and bounded recovery. The durable record is not a trace of Rust control flow. + +## Durable authority + +The record stores only facts that affect identity, authorization, replay, or the final domain result: + +- a stable operation identity and request fingerprint derived from caller intent; +- the Workspace resource and the resolved authority that exact retries must keep; +- `pending`, `failed`, or `completed` lifecycle state; +- attempt count and timestamps as operational evidence; +- explicit retryability, bounded failure category, and bounded disposition; +- a factual checkpoint only when a non-idempotent provider effect cannot be safely re-observed or repeated. + +A fingerprint excludes Server-generated result identifiers, attempt data, diagnostics, and fresh observations. Reusing one operation identity with a different fingerprint is an error. A completed exact retry replays the committed bounded result. + +`pending` means only that the intent remains open. Function names, validation steps, and provider-call positions are not persisted as lifecycle stages. `failed` records the latest terminal attempt outcome; retryability remains separate metadata. `completed` means the required domain result and disposition are durably committed. + +## Live authority and provider evidence + +Before every attempt, the Backend rereads current Workspace ownership and guards. A previous observation that a resource was detached, unblocked, or clean is not authorization for a later retry. + +Provider timeout, unavailability, an empty response, or an unknown outcome is not authoritative absence. Registry cleanup may use only an explicit provider success contract or the provider's exact not-found evidence. If a provider effect is idempotent and exact not-found can be re-observed, arbitrary execution stages and crash-window checkpoints are unnecessary: recovery repeats observation and converges from last committed facts. + +## Workdir removal + +Workdir removal is one durable side-effect operation in the Workspace Server DB. It binds the Workspace, Workdir, owning Runtime, Repository/materialization identity, source actor, stable intent fingerprint, lifecycle, retry metadata, and bounded result. Runtime URL, provider handle, host path, credentials, and caller-selected Runtime are not operation inputs. + +Each attempt: + +1. resolves or revalidates the persisted same-Workspace Workdir, Runtime, Repository, and materialization identity; +2. checks current attachments, attachment reservations, current assignment occupancy, retention/cleanup holds, and pending materialization authority; +3. retains dirty, occupied, blocked, or otherwise unknown Workdirs without detaching a Worker or forcing deletion; +4. observes the owning Runtime/provider and calls its existing Workdir cleanup only for an eligible clean Workdir; +5. treats only successful provider cleanup or exact `working_directory_not_found` as removal evidence; +6. deletes the Backend Workdir registry row and commits the operation's `completed`/`removed` result in one SQLite transaction. + +A provider error leaves the registry intact and records a bounded `attention_required` result with explicit retryability. Startup recovery lists `pending` and retryable `failed` operations, then executes this same path after rereading live authority. `WorkdirDelete`, Workspace REST removal, Runtime cleanup execution, and recovery must not maintain separate inline provider-delete paths. + +The public request contains only `working_directory_id` plus a bounded reason. The public result contains only the Workdir ID, `removed | retained | attention_required`, retryability, and an optional bounded failure category. Internal operation identifiers, checkpoints, provider paths, and credentials are not public DTO fields. + +## Resilience boundary + +This pattern covers duplicate requests, returned failures, timeouts and unknown outcomes, known partial-completion contracts, and restart recovery from committed facts. It does not provide general exactly-once execution or claim recovery from every instruction-boundary panic, process kill, machine loss, or power failure. A stronger provider guarantee requires a separately specified protocol, checkpoint ordering, reconciliation evidence, and tests.