fix: fence Workdir removal retries

2026-09-02 00:25:21 +09:00
parent 9e0d499987
commit 510795f1c5
5 changed files with 216 additions and 109 deletions
@@ -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(&not_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()
        );
    }

@@ -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}");
    }

@@ -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, &current)?;
+            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;
@@ -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

@@ -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.