diff --git a/crates/merge-request/src/lib.rs b/crates/merge-request/src/lib.rs index aa7409d14..c339c3a31 100644 --- a/crates/merge-request/src/lib.rs +++ b/crates/merge-request/src/lib.rs @@ -210,12 +210,7 @@ impl MergeRequestThreadEvent { } fn bound_bodies(&mut self) { match self { - Self::Review(value) => { - truncate_body(&mut value.body); - for finding in &mut value.findings { - truncate_body(&mut finding.body); - } - } + Self::Review(_) => {} Self::ReviewRevoked(value) => truncate_body(&mut value.reason), Self::ReviewCancelled(value) => truncate_body(&mut value.reason), Self::Comment(value) => truncate_body(&mut value.body), @@ -939,10 +934,6 @@ impl MergeRequestStore { &self, i: SubmitMergeRequestReview, ) -> Result { - bounded_body("review body", &i.body)?; - for finding in &i.findings { - bounded_body("review finding", &finding.body)?; - } let mut c = self.lock()?; let t = c.transaction()?; let g: Option<(String, String, String, String, String, String)> = t diff --git a/crates/merge-request/tests/store.rs b/crates/merge-request/tests/store.rs index 5cf2837b2..e9e3e83e5 100644 --- a/crates/merge-request/tests/store.rs +++ b/crates/merge-request/tests/store.rs @@ -376,6 +376,49 @@ fn approve_for( fn approve(s: &MergeRequestStore, subject: &str, token: &str) -> ReviewEvent { approve_for(s, "MR", "R", subject, token) } +#[test] +fn review_submission_preserves_long_body_and_findings_on_reload() { + for (body, finding_body) in [ + ("b".repeat(16 * 1024 + 1), "finding".into()), + ("review".into(), "指摘".repeat(4096)), + ] { + let (_dir, store) = fixture(); + open(&store); + request(&store, "source", "token"); + let findings = vec![ReviewFinding { + severity: FindingSeverity::Major, + code: None, + path: None, + line: None, + body: finding_body, + }]; + let submitted = store + .submit_review(SubmitMergeRequestReview { + merge_request_id: "MR".into(), + ticket_id: "T".into(), + current_subject_ref: "source".into(), + capability_token: "token".into(), + decision: ReviewDecision::RequestChanges, + body: body.clone(), + findings: findings.clone(), + now: at(4), + }) + .unwrap(); + assert_eq!(submitted.body, body); + assert_eq!(submitted.findings, findings); + let reloaded = store.get_by_id("W", "MR").unwrap(); + let saved = reloaded + .thread + .iter() + .find_map(|event| match event { + MergeRequestThreadEvent::Review(review) => Some(review), + _ => None, + }) + .unwrap(); + assert_eq!(saved, &submitted); + } +} + #[test] fn review_submission_authorization_rejects_invalid_grants_before_side_effects() { let (_d, store) = fixture(); diff --git a/crates/server-api/src/lib.rs b/crates/server-api/src/lib.rs index 6456dd4c1..aa151c016 100644 --- a/crates/server-api/src/lib.rs +++ b/crates/server-api/src/lib.rs @@ -7348,6 +7348,8 @@ pub struct WorkingDirectoryCleanupTarget { #[cfg_attr(feature = "typescript", derive(ts_rs::TS))] #[serde(deny_unknown_fields)] pub struct WorkingDirectoryRemovalRequest { + /// Stable for an exact resend; a new explicit request uses a new identity. + pub operation_id: String, pub reason: String, } @@ -7886,6 +7888,9 @@ pub struct CurrentWorkerWorkdirAttachmentResponse { #[serde(skip_serializing_if = "Option::is_none")] pub environment: Option, pub attached: bool, + /// The Workdir was already attached to this Worker; alias is its existing name. + #[serde(default)] + pub already_attached: bool, } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, JsonSchema)] @@ -13691,9 +13696,11 @@ mod tests { capabilities: workdir::WorkdirSessionCapabilities::EMPTY, environment: None, attached: false, + already_attached: false, }; let value = serde_json::to_value(&response).unwrap(); - assert_eq!(value.as_object().unwrap().len(), 5); + assert_eq!(value.as_object().unwrap().len(), 6); + assert_eq!(value["already_attached"], false); assert!(value.get("connection_id").is_none()); assert_eq!( serde_json::from_value::(value).unwrap(), diff --git a/crates/subjektiv/src/journal/binding.rs b/crates/subjektiv/src/journal/binding.rs index e80846ff0..2f113bfb5 100644 --- a/crates/subjektiv/src/journal/binding.rs +++ b/crates/subjektiv/src/journal/binding.rs @@ -202,6 +202,29 @@ impl SubjektivStore { }) } + /// End a body after the Host has confirmed its Runtime removal. This is not a + /// completed Journal handoff: source coverage, positions and saved receipts stay + /// unchanged. Revocation and closing generations are atomic, so no retained Job + /// can write after deletion even while Backend Job cleanup is being retried. + pub fn release_removed_journal_body(&self, host: &JournalHostContext) -> JournalResult<()> { + self.database.try_transaction(|tx| { + super::store::check_session_host(self, tx, host)?; + let attr = host.attribution.as_ref().expect("checked Session authority"); + let binding: Option = tx.query_row( + "SELECT binding_id FROM journal_writer_bindings WHERE subject_id=?1 AND runtime_id=?2 AND worker_id=?3 ORDER BY rowid DESC LIMIT 1", + params![host.subject_id, attr.runtime_id, attr.worker_id], |r| r.get(0), + ).optional()?; + let binding = binding.unwrap_or_else(|| crate::issued_id("journal-removed-body")); + tx.execute("INSERT OR IGNORE INTO journal_writer_bindings(binding_id,subject_id,runtime_id,worker_id,active) VALUES(?1,?2,?3,?4,0)",params![binding,host.subject_id,attr.runtime_id,attr.worker_id])?; + tx.execute("UPDATE journal_writer_bindings SET active=0 WHERE subject_id=?1 AND runtime_id=?2 AND worker_id=?3",params![host.subject_id,attr.runtime_id,attr.worker_id])?; + // Bind legacy attributed Sessions to an inactive lifetime, never to the + // successor. Retained unfinished history is not an active writer. + tx.execute("INSERT OR IGNORE INTO journal_session_bindings(session_id,binding_id) SELECT session_id,?4 FROM subject_session_attributions WHERE subject_id=?1 AND runtime_id=?2 AND worker_id=?3",params![host.subject_id,attr.runtime_id,attr.worker_id,binding])?; + tx.execute("UPDATE journal_generations SET active=0 WHERE journal_id IN (SELECT j.id FROM journals j JOIN subject_session_attributions a ON a.session_id=j.source_session_id WHERE a.subject_id=?1 AND a.runtime_id=?2 AND a.worker_id=?3)",params![host.subject_id,attr.runtime_id,attr.worker_id])?; + Ok(()) + }) + } + /// The Host must hold its lifecycle fence, confirm Idle, reconcile all committed original /// Sessions (including the save/notification gap), and exclude queued/running/Unknown Jobs. /// This transaction rejects retained writers, pending source bytes and active generations. diff --git a/crates/subjektiv/src/journal/store.rs b/crates/subjektiv/src/journal/store.rs index 019c90a7e..ddce6d4fd 100644 --- a/crates/subjektiv/src/journal/store.rs +++ b/crates/subjektiv/src/journal/store.rs @@ -653,6 +653,32 @@ impl SubjektivStore { Ok(()) }) } + /// Read the exact retained Job/attempt snapshot without granting writer + /// authority or creating a generation. Used for outcome recovery after removal. + pub fn retained_journal_generation( + &self, + host: &JournalHostContext, + id: &str, + binding: &JobAttemptBinding, + ) -> JournalResult> { + self.database.try_with_connection(|tx| { + check_host(self, tx, host, false)?; + check_session_host(self, tx, host)?; + journal(tx, host, id, true)?; + let raw: Option = tx.query_row( + "SELECT snapshot_json FROM journal_generations WHERE journal_id=?1 AND job_id=?2 AND attempt_id=?3", + params![id, binding.job_id, binding.attempt_id], |r| r.get(0), + ).optional()?; + raw.map(|raw| { + let generation: JournalGeneration = serde_json::from_str(&raw)?; + if generation.journal_id != id || &generation.job != binding { + return Err(conflict("retained generation identity mismatch")); + } + Ok(generation) + }).transpose() + }) + } + pub fn read_journal_generation( &self, host: &JournalHostContext, diff --git a/crates/worker-runtime/src/runtime.rs b/crates/worker-runtime/src/runtime.rs index 88c6b2fa3..72d78e499 100644 --- a/crates/worker-runtime/src/runtime.rs +++ b/crates/worker-runtime/src/runtime.rs @@ -103,7 +103,18 @@ impl Drop for WorkdirOperationGuard { } } -fn workdir_operation_busy() -> RuntimeError { +fn workdir_operation_busy(id: &str, action: &str, blocking: &WorkdirOperation) -> RuntimeError { + let (busy_reason, active_use_count) = match blocking { + WorkdirOperation::Use(count) => ("active_use", *count), + WorkdirOperation::Cleanup => ("cleanup_in_progress", 0), + }; + tracing::warn!(target: "yoi::workdir_cleanup", + working_directory_id = id, + action, + busy_source = "runtime_operation", + busy_reason, + active_use_count, + "Workdir operation blocked by busy resource"); RuntimeError::WorkingDirectory( crate::working_directory::WorkingDirectoryDiagnostic::rejected( "working_directory_cleanup_resource_busy", @@ -1150,8 +1161,12 @@ impl Runtime { let backend = { let mut state = self.lock()?; state.ensure_running()?; - if state.workdir_operations.contains_key(working_directory_id) { - return Err(workdir_operation_busy()); + if let Some(blocking) = state.workdir_operations.get(working_directory_id) { + return Err(workdir_operation_busy( + working_directory_id, + "cleanup_working_directory", + blocking, + )); } if let Some(worker_id) = state.worker_id_for_workdir(working_directory_id) { return Err(RuntimeError::InvalidRequest(format!( @@ -6344,11 +6359,8 @@ impl RuntimeState { } fn ensure_workdir_not_removing(&self, id: &str) -> Result<(), RuntimeError> { - if matches!( - self.workdir_operations.get(id), - Some(WorkdirOperation::Cleanup) - ) { - return Err(workdir_operation_busy()); + if let Some(blocking @ WorkdirOperation::Cleanup) = self.workdir_operations.get(id) { + return Err(workdir_operation_busy(id, "admit_use", blocking)); } Ok(()) } @@ -9409,6 +9421,84 @@ mod tests { .block_on(subscription.recv()) } + #[test] + fn busy_admission_logs_runtime_use_and_cleanup_blockers() { + use std::io::{Read, Seek, SeekFrom}; + + let mut output = tempfile::tempfile().unwrap(); + let writer = output.try_clone().unwrap(); + let subscriber = tracing_subscriber::fmt() + .with_ansi(false) + .without_time() + .with_writer(move || writer.try_clone().unwrap()) + .finish(); + tracing::subscriber::with_default(subscriber, || { + tracing::callsite::rebuild_interest_cache(); + let (runtime, backend) = runtime_and_backend(); + let first = runtime.reserve_workdir_use(["workdir-used"]).unwrap(); + let second = runtime.reserve_workdir_use(["workdir-used"]).unwrap(); + assert_workdir_busy( + runtime + .cleanup_working_directory("workdir-used") + .unwrap_err(), + ); + drop((first, second)); + assert!(runtime.inner.lock().unwrap().workdir_operations.is_empty()); + + let (entered, release) = WorkdirEffectGate::install(&backend.cleanup_gate); + let cleanup_runtime = runtime.clone(); + let cleanup = std::thread::spawn(move || { + cleanup_runtime.cleanup_working_directory("workdir-removing") + }); + entered + .recv_timeout(std::time::Duration::from_secs(5)) + .unwrap(); + assert_workdir_busy( + runtime + .cleanup_working_directory("workdir-removing") + .unwrap_err(), + ); + assert_workdir_busy( + runtime + .reserve_workdir_use(["workdir-removing"]) + .unwrap_err(), + ); + release.send(()).unwrap(); + assert_workdir_busy(cleanup.join().unwrap().unwrap_err()); + assert!(runtime.inner.lock().unwrap().workdir_operations.is_empty()); + }); + output.seek(SeekFrom::Start(0)).unwrap(); + let mut log = String::new(); + output.read_to_string(&mut log).unwrap(); + let events: Vec<_> = log + .lines() + .filter(|line| line.contains("busy_source=")) + .collect(); + assert_eq!(events.len(), 3, "{log}"); + for (event, (id, action, reason, count)) in events.iter().zip([ + ("workdir-used", "cleanup_working_directory", "active_use", 2), + ( + "workdir-removing", + "cleanup_working_directory", + "cleanup_in_progress", + 0, + ), + ("workdir-removing", "admit_use", "cleanup_in_progress", 0), + ]) { + for field in [ + "WARN".to_string(), + "yoi::workdir_cleanup".to_string(), + format!("working_directory_id=\"{id}\""), + format!("action=\"{action}\""), + "busy_source=\"runtime_operation\"".to_string(), + format!("busy_reason=\"{reason}\""), + format!("active_use_count={count}"), + ] { + assert!(event.contains(&field), "missing {field}: {event}"); + } + } + } + #[test] fn workdir_removal_reserves_only_its_target_and_releases_on_failure() { let (runtime, backend) = runtime_and_backend(); diff --git a/crates/worker-runtime/src/working_directory/occupancy.rs b/crates/worker-runtime/src/working_directory/occupancy.rs index c3e477bb5..031d1cda7 100644 --- a/crates/worker-runtime/src/working_directory/occupancy.rs +++ b/crates/worker-runtime/src/working_directory/occupancy.rs @@ -28,7 +28,19 @@ pub(super) struct Lease { id: String, } -fn busy() -> WorkingDirectoryDiagnostic { +fn busy(id: &str, action: &str, blocking: &Entry) -> WorkingDirectoryDiagnostic { + let (busy_reason, active_use_count) = match blocking { + Entry::Use(count) => ("active_use", *count), + Entry::Create => ("creation_in_progress", 0), + Entry::Cleanup => ("cleanup_in_progress", 0), + }; + tracing::warn!(target: "yoi::workdir_cleanup", + working_directory_id = id, + action, + busy_source = "materializer_occupancy", + busy_reason, + active_use_count, + "Workdir operation blocked by busy resource"); WorkingDirectoryDiagnostic::new( "working_directory_cleanup_resource_busy", "A filesystem resource is busy or changed during removal. Release the resource and retry.", @@ -42,7 +54,9 @@ impl Occupancy { .lock() .unwrap_or_else(|error| error.into_inner()); match entries.get_mut(id) { - Some(Entry::Cleanup | Entry::Create) => return Err(busy()), + Some(blocking @ (Entry::Cleanup | Entry::Create)) => { + return Err(busy(id, "acquire_use", blocking)); + } Some(Entry::Use(count)) => *count += 1, None => { entries.insert(id.to_string(), Entry::Use(1)); @@ -101,8 +115,8 @@ impl Occupancy { .entries .lock() .unwrap_or_else(|error| error.into_inner()); - if entries.contains_key(id) { - return Err(busy()); + if let Some(blocking) = entries.get(id) { + return Err(busy(id, "acquire_creation", blocking)); } entries.insert(id.to_string(), Entry::Create); Ok(Lease { @@ -116,8 +130,8 @@ impl Occupancy { .entries .lock() .unwrap_or_else(|error| error.into_inner()); - if entries.contains_key(id) { - return Err(busy()); + if let Some(blocking) = entries.get(id) { + return Err(busy(id, "acquire_cleanup", blocking)); } entries.insert(id.to_string(), Entry::Cleanup); Ok(Lease { @@ -131,6 +145,71 @@ impl Occupancy { mod tests { use super::*; + #[test] + fn busy_admission_logs_blocker_and_use_count_without_changing_error() { + use std::io::{Read, Seek, SeekFrom}; + + let mut output = tempfile::tempfile().unwrap(); + let writer = output.try_clone().unwrap(); + let subscriber = tracing_subscriber::fmt() + .with_ansi(false) + .without_time() + .with_writer(move || writer.try_clone().unwrap()) + .finish(); + tracing::subscriber::with_default(subscriber, || { + tracing::callsite::rebuild_interest_cache(); + let occupancy = Occupancy::default(); + for (id, leases) in [ + ( + "workdir-used", + vec![ + occupancy.acquire_use("workdir-used").unwrap(), + occupancy.acquire_use("workdir-used").unwrap(), + ], + ), + ( + "workdir-creating", + vec![occupancy.acquire_creation("workdir-creating").unwrap()], + ), + ( + "workdir-removing", + vec![occupancy.acquire_cleanup("workdir-removing").unwrap()], + ), + ] { + let error = occupancy.acquire_cleanup(id).unwrap_err(); + assert_eq!(error.code, "working_directory_cleanup_resource_busy"); + assert_eq!( + error.message, + "A filesystem resource is busy or changed during removal. Release the resource and retry." + ); + drop(leases); + drop(occupancy.acquire_cleanup(id).unwrap()); + } + }); + output.seek(SeekFrom::Start(0)).unwrap(); + let mut log = String::new(); + output.read_to_string(&mut log).unwrap(); + let events: Vec<_> = log.lines().collect(); + assert_eq!(events.len(), 3, "{log}"); + for (event, (id, reason, count)) in events.iter().zip([ + ("workdir-used", "active_use", 2), + ("workdir-creating", "creation_in_progress", 0), + ("workdir-removing", "cleanup_in_progress", 0), + ]) { + for field in [ + "WARN".to_string(), + "yoi::workdir_cleanup".to_string(), + format!("working_directory_id=\"{id}\""), + "action=\"acquire_cleanup\"".to_string(), + "busy_source=\"materializer_occupancy\"".to_string(), + format!("busy_reason=\"{reason}\""), + format!("active_use_count={count}"), + ] { + assert!(event.contains(&field), "missing {field}: {event}"); + } + } + } + #[test] fn use_and_cleanup_leases_release_during_unwind() { let occupancy = Occupancy::default(); diff --git a/crates/worker/src/feature/builtin/manage_workdir.rs b/crates/worker/src/feature/builtin/manage_workdir.rs index 12bcb209a..25254599f 100644 --- a/crates/worker/src/feature/builtin/manage_workdir.rs +++ b/crates/worker/src/feature/builtin/manage_workdir.rs @@ -776,25 +776,39 @@ impl WorkspaceHttpWorkdirBackend { ATTACH_TOOL, "working_directory_id", )?; - let alias_key = workdir::WorkdirAttachmentAlias::new(alias) - .map_err(|error| ToolError::InvalidArgument(error.to_string()))?; - if self.session_router.session(&alias_key).is_some() { - return Err(ToolError::InvalidArgument(format!( - "attachment alias `{alias}` already exists" - ))); - } let response = self.attach_response(alias, working_directory_id)?; - if let Err(error) = self.session_router.attach( - alias_key.clone(), + if !response.attachment.attached + || response.attachment.working_directory_id != working_directory_id + { + return Err(ToolError::ExecutionFailed( + "Workdir attachment response does not match the target".into(), + )); + } + // Backend ownership decides the name. A requested alias cannot rename + // an existing attachment or create a second route to the same Workdir. + let attached_alias = &response.attachment.alias; + let alias_key = workdir::WorkdirAttachmentAlias::new(attached_alias.clone()) + .map_err(|error| ToolError::ExecutionFailed(error.to_string()))?; + if let Some(session) = self.session_router.session(&alias_key) { + if session.workdir().id().as_str() != working_directory_id { + return Err(ToolError::ExecutionFailed("Local Workdir attachment differs from Backend; refresh attachments before retrying".into())); + } + } else if let Err(error) = self.session_router.attach( + alias_key, WorkspaceAttachedWorkdirSession::handle_for_workdir_with_environment( self.client.clone(), - alias, + attached_alias, working_directory_id, response.attachment.capabilities, response.environment.clone(), ), ) { - let rollback = self.detach(alias); + if response.attachment.already_attached { + return Err(ToolError::ExecutionFailed(format!( + "attach Workdir session route: {error}; existing Backend attachment preserved" + ))); + } + let rollback = self.detach(attached_alias); return Err(ToolError::ExecutionFailed(match rollback { Ok(_) => { format!("attach Workdir session route: {error}; Backend attachment rolled back") @@ -805,7 +819,11 @@ impl WorkspaceHttpWorkdirBackend { })); } workdir_output( - format!("Attached Workdir {working_directory_id} as {alias}"), + if response.attachment.already_attached { + format!("Workdir {working_directory_id} is already attached as {attached_alias}") + } else { + format!("Attached Workdir {working_directory_id} as {attached_alias}") + }, &response, ) } @@ -817,15 +835,31 @@ impl WorkspaceHttpWorkdirBackend { ) -> Result { let workspace_id = encode_path_segment(self.workspace_id()?); self.authorize_workdir("workdir.attach", working_directory_id)?; - self.execute_json::(WorkspaceRequest::json( - WorkspaceRequestMethod::Post, - format!("/api/w/{workspace_id}/workers/self/workdir-attachments"), - serde_json::to_string(&WorkdirAttachRequest { - alias: alias.to_string(), - working_directory_id: working_directory_id.to_string(), - }) - .map_err(decode_error)?, - )) + let response = self + .client + .execute(WorkspaceRequest::json( + WorkspaceRequestMethod::Post, + format!("/api/w/{workspace_id}/workers/self/workdir-attachments"), + serde_json::to_string(&WorkdirAttachRequest { + alias: alias.to_string(), + working_directory_id: working_directory_id.to_string(), + }) + .map_err(decode_error)?, + )) + .map_err(|error| { + ToolError::ExecutionFailed(format!("Workspace Workdir request failed: {error}")) + })?; + if response.status == 409 + && let Ok(error) = + serde_json::from_str::(&response.body) + && error.error == "workdir_occupied" + { + return Err(ToolError::StructuredConflict { + code: "workdir_occupied".into(), + message: "Workdir is occupied by another Worker".into(), + }); + } + decode_response(response) } fn detach(&self, alias: &str) -> Result { @@ -860,7 +894,11 @@ impl WorkspaceHttpWorkdirBackend { ) } - fn delete(&self, input: WorkdirDeleteInput) -> Result { + fn delete( + &self, + input: WorkdirDeleteInput, + operation_id: String, + ) -> Result { let workdir_id = validate_identity( &input.working_directory_id, DELETE_TOOL, @@ -874,6 +912,7 @@ impl WorkspaceHttpWorkdirBackend { WorkspaceRequestMethod::Delete, format!("/api/w/{workspace_id}/working-directories/{workdir_path}"), serde_json::to_string(&WorkdirRemovalRequest { + operation_id, reason: reason.to_string(), }) .map_err(decode_error)?, @@ -884,7 +923,7 @@ impl WorkspaceHttpWorkdirBackend { response.disposition, removal_guidance(response.failure_category.as_deref()), if response.retryable { - " After resolving the cause, repeat this WorkdirDelete with the same id and reason; current safety conditions will be rechecked." + " After resolving the cause, a new explicit WorkdirDelete request will recheck current safety conditions. This failed request will not be automatically re-executed." } else { "" } @@ -1047,9 +1086,10 @@ impl Tool for WorkspaceHttpWorkdirTool { self.backend.detach_managed(&input.alias, None).await } - WorkdirOperation::Delete => self - .backend - .delete(parse_input::(input_json)?), + WorkdirOperation::Delete => self.backend.delete( + parse_input::(input_json)?, + ctx.call_id.to_string(), + ), } } } @@ -1111,8 +1151,14 @@ fn removal_guidance(category: Option<&str>) -> &'static str { Some("runtime_unavailable" | "provider_unavailable") => { "Restore the Runtime connection; the registry remains until deletion is confirmed." } - Some("provider_observation_unknown" | "provider_cleanup_outcome_unknown") => { - "Deletion is not confirmed. Re-observe with normal removal after restoring provider access; the registry is retained." + Some("provider_cleanup_outcome_unknown") => { + "Deletion outcome is unknown. Restore provider access and reconcile the existing request; do not submit a fresh deletion intent. Recovery only observes the target and never repeats deletion." + } + Some("provider_observation_unknown") => { + "The target could not be observed before deletion. Restore provider access before a new request." + } + Some("request_interrupted") => { + "The request was interrupted before execution. A new explicit request is required." } Some("unsupported_target") => { "Ask the owner for the provider's supported cleanup operation." @@ -1312,6 +1358,8 @@ struct WorkdirAttachmentResponse { working_directory_id: String, capabilities: WorkdirSessionCapabilities, attached: bool, + #[serde(default)] + already_attached: bool, } #[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] @@ -1467,8 +1515,44 @@ mod tests { }) } + #[tokio::test] + async fn delete_call_identity_distinguishes_new_requests_from_exact_resends() { + let client = Arc::new(RecordingWorkspaceClient::new( + (0..3) + .map(|_| { + response(json!({ + "working_directory_id": "wd-request", + "disposition": "attention_required", + "retryable": true, + "failure_category": "resource_busy" + })) + }) + .collect(), + )); + let tool = WorkspaceHttpWorkdirTool { + backend: WorkspaceHttpWorkdirBackend::new(client.clone()), + operation: WorkdirOperation::Delete, + }; + let input = r#"{"working_directory_id":"wd-request","reason":"remove unused Workdir"}"#; + for id in ["delete-1", "delete-1", "delete-2"] { + tool.execute(input, ToolExecutionContext::new(id, "batch", 0)) + .await + .unwrap(); + } + let bodies: Vec = client + .requests() + .iter() + .map(|r| serde_json::from_str(r.body.as_deref().unwrap()).unwrap()) + .collect(); + assert_eq!(bodies[0], bodies[1]); + assert_eq!(bodies[0]["operation_id"], "delete-1"); + assert_eq!(bodies[2]["operation_id"], "delete-2"); + assert_eq!(bodies[0]["reason"], bodies[2]["reason"]); + assert!(delete_schema()["properties"].get("operation_id").is_none()); + } + #[test] - fn ordinary_workdir_delete_reports_safe_cause_and_exact_retry_guidance() { + fn ordinary_workdir_delete_reports_safe_cause_and_new_request_guidance() { let client = Arc::new(RecordingWorkspaceClient::new(vec![response(json!({ "working_directory_id": "wd-retry", "disposition": "attention_required", @@ -1477,10 +1561,13 @@ mod tests { }))])); let backend = WorkspaceHttpWorkdirBackend::new(client.clone()); let output = backend - .delete(WorkdirDeleteInput { - working_directory_id: "wd-retry".into(), - reason: "remove unused Workdir".into(), - }) + .delete( + WorkdirDeleteInput { + working_directory_id: "wd-retry".into(), + reason: "remove unused Workdir".into(), + }, + "delete-call-1".to_string(), + ) .unwrap(); assert!(output.summary.contains("Confirm its owner")); assert!( @@ -1488,7 +1575,11 @@ mod tests { .summary .contains("Do not recursively delete mounted data") ); - assert!(output.summary.contains("same id and reason")); + assert!( + output + .summary + .contains("new explicit WorkdirDelete request") + ); assert!(output.summary.len() <= 600); let content: serde_json::Value = serde_json::from_str(output.content.as_deref().unwrap()).unwrap(); @@ -1726,10 +1817,13 @@ mod tests { .unwrap(); backend.detach("checkout").unwrap(); backend - .delete(WorkdirDeleteInput { - working_directory_id: "wd-created".to_string(), - reason: "remove stale Workdir".to_string(), - }) + .delete( + WorkdirDeleteInput { + working_directory_id: "wd-created".to_string(), + reason: "remove stale Workdir".to_string(), + }, + "delete-call-1".to_string(), + ) .unwrap(); let requests = client.requests(); @@ -1767,7 +1861,102 @@ mod tests { assert_eq!(requests[4].method, WorkspaceRequestMethod::Delete); let body: serde_json::Value = serde_json::from_str(requests[4].body.as_deref().unwrap()).unwrap(); - assert_eq!(body, json!({"reason": "remove stale Workdir"})); + assert_eq!( + body, + json!({"reason": "remove stale Workdir", "operation_id": "delete-call-1"}) + ); + } + + #[test] + fn attach_reports_other_worker_occupancy_without_exposing_backend_details() { + let client = Arc::new(RecordingWorkspaceClient::new(vec![WorkspaceResponse { + status: 409, + body: serde_json::to_string(&server_api::RepositoryApiError::new( + 409, + "workdir_occupied", + "private backend detail", + Vec::new(), + )) + .unwrap(), + }])); + let backend = WorkspaceHttpWorkdirBackend::new(client.clone()); + let error = backend + .attach(WorkdirAttachInput { + alias: "work".into(), + working_directory_id: "occupied".into(), + }) + .unwrap_err(); + assert!( + matches!(error, ToolError::StructuredConflict { code, message } if code == "workdir_occupied" && message == "Workdir is occupied by another Worker") + ); + assert_eq!(client.requests().len(), 1); + } + + #[test] + fn attach_reuses_backend_alias_without_duplicate_routes_or_detach() { + let client = Arc::new(RecordingWorkspaceClient::new((0..3).map(|_| response(json!({ + "workspace_id": "workspace/test", "alias": "existing", "working_directory_id": "wd-existing", + "capabilities": {"bits": 63}, "attached": true, "already_attached": true, + "environment": workdir::WorkdirSessionEnvironment::root(WorkdirSessionCapabilities::ALL) + }))).collect())); + let backend = WorkspaceHttpWorkdirBackend::new(client.clone()); + let occupied_alias = workdir::WorkdirAttachmentAlias::new("other").unwrap(); + backend + .session_router + .attach( + occupied_alias.clone(), + WorkspaceAttachedWorkdirSession::handle_for_workdir( + client.clone(), + "other", + "wd-other", + ), + ) + .unwrap(); + for alias in ["new-name", "existing", "other"] { + let output = backend + .attach(WorkdirAttachInput { + alias: alias.into(), + working_directory_id: "wd-existing".into(), + }) + .unwrap(); + assert_eq!( + output.summary, + "Workdir wd-existing is already attached as existing" + ); + let body: serde_json::Value = + serde_json::from_str(output.content.as_deref().unwrap()).unwrap(); + assert_eq!(body["alias"], "existing"); + assert_eq!(body["already_attached"], true); + } + assert!( + backend + .session_router + .session(&workdir::WorkdirAttachmentAlias::new("new-name").unwrap()) + .is_none() + ); + assert_eq!( + backend + .session_router + .session(&occupied_alias) + .unwrap() + .workdir() + .id() + .as_str(), + "wd-other" + ); + assert!( + backend + .session_router + .session(&workdir::WorkdirAttachmentAlias::new("existing").unwrap()) + .is_some() + ); + assert_eq!(client.requests().len(), 3); + assert!( + client + .requests() + .iter() + .all(|request| request.method == WorkspaceRequestMethod::Post) + ); } #[tokio::test] @@ -2377,10 +2566,13 @@ mod tests { ); assert!( backend - .delete(WorkdirDeleteInput { - working_directory_id: "wd".into(), - reason: "remove unused".into() - }) + .delete( + WorkdirDeleteInput { + working_directory_id: "wd".into(), + reason: "remove unused".into(), + }, + "delete-intent".into(), + ) .is_err() ); assert!(backend.detach_managed("checkout", None).await.is_err()); diff --git a/crates/worker/src/feature/builtin/manage_workdir/wip.rs b/crates/worker/src/feature/builtin/manage_workdir/wip.rs index f34ab4330..81eff90da 100644 --- a/crates/worker/src/feature/builtin/manage_workdir/wip.rs +++ b/crates/worker/src/feature/builtin/manage_workdir/wip.rs @@ -914,7 +914,7 @@ impl WipOperationHandler for ItemHandler { let mut result = raw; result["workdir_path"] = json!(item_path(Domain::Workdir, &self.id)); result["attachments_path"] = json!(ATTACHMENTS); - result["attachment"] = input + result["attachment"] = result .get("alias") .and_then(Json::as_str) .and_then(|alias| self.provider.attached_reference(alias, &self.id)) @@ -1421,10 +1421,19 @@ pub(crate) mod tests { json!({"workspace_id":"test-workspace", "runtime_id":"runtime", "item":raw, "diagnostics":[]}) } (WorkspaceRequestMethod::Post, "workers/self/workdir-attachments") => { - state.generation += 1; - let raw = json!({"alias":body["alias"], "working_directory_id":body["working_directory_id"], "connection_id":format!("connection-{}", state.generation), "capabilities":{"bits":63}}); - state.attachments.push(raw); - json!({"workspace_id":"test-workspace", "alias":body["alias"], "working_directory_id":body["working_directory_id"], "capabilities":{"bits":63}, "environment":workdir::WorkdirSessionEnvironment::root(workdir::WorkdirSessionCapabilities::ALL), "attached":true}) + let existing = state + .attachments + .iter() + .find(|raw| raw["working_directory_id"] == body["working_directory_id"]) + .cloned(); + let already_attached = existing.is_some(); + let raw = existing.unwrap_or_else(|| { + state.generation += 1; + let raw = json!({"alias":body["alias"], "working_directory_id":body["working_directory_id"], "connection_id":format!("connection-{}", state.generation), "capabilities":{"bits":63}}); + state.attachments.push(raw.clone()); + raw + }); + json!({"workspace_id":"test-workspace", "alias":raw["alias"], "working_directory_id":raw["working_directory_id"], "capabilities":{"bits":63}, "environment":workdir::WorkdirSessionEnvironment::root(workdir::WorkdirSessionCapabilities::ALL), "attached":true, "already_attached":already_attached}) } (WorkspaceRequestMethod::Delete, p) if p.starts_with("workers/self/workdir-attachments/") => diff --git a/crates/worker/src/feature/builtin/manage_workdir/wip/model_tests.rs b/crates/worker/src/feature/builtin/manage_workdir/wip/model_tests.rs index 669cbfe3d..58d933937 100644 --- a/crates/worker/src/feature/builtin/manage_workdir/wip/model_tests.rs +++ b/crates/worker/src/feature/builtin/manage_workdir/wip/model_tests.rs @@ -413,7 +413,16 @@ async fn model_repository_create_attach_selected_entries_read_and_detach_preserv .invoke(&workdir, "attach", json!({"alias":"checkout"})) .await; assert_eq!(attached["attached"], true); + assert_eq!(attached["already_attached"], false); assert_eq!(attached["attachment"]["$case"], "available"); + let (reused, reused_entries) = f + .invoke(&workdir, "attach", json!({"alias":"another-name"})) + .await; + assert_eq!(reused["already_attached"], true); + assert_eq!(reused["alias"], "checkout"); + assert_eq!(reused["attachment"], attached["attachment"]); + assert_eq!(reused_entries, entries); + assert_eq!(f.client.catalog.state.lock().unwrap().attachments.len(), 1); assert_eq!( f.writes("workdir-session/operations"), 0, @@ -432,7 +441,7 @@ async fn model_repository_create_attach_selected_entries_read_and_detach_preserv .unwrap() .contains("isolated content") ); - assert_eq!(f.writes("workdir-attachments"), 1); + assert_eq!(f.writes("workdir-attachments"), 2); f.invoke(&attachment, "detach", json!({})).await; assert!( f.execute("Inspect", json!({"path":attachment,"refresh":true})) diff --git a/crates/worker/src/feature/builtin/manage_workdir/wip/result.rs b/crates/worker/src/feature/builtin/manage_workdir/wip/result.rs index 904c44ce3..5eeb50127 100644 --- a/crates/worker/src/feature/builtin/manage_workdir/wip/result.rs +++ b/crates/worker/src/feature/builtin/manage_workdir/wip/result.rs @@ -132,6 +132,7 @@ pub(super) fn attached() -> TypeExpr { field("capabilities", true, TypeExpr::Json), field("environment", true, TypeExpr::Json), field("attached", true, TypeExpr::Boolean), + field("already_attached", true, TypeExpr::Boolean), field("workdir_path", true, TypeExpr::Entry), field("attachments_path", true, TypeExpr::Entry), field("attachment", true, TypeExpr::Union { cases: vec![ diff --git a/crates/workspace-server/src/lib.rs b/crates/workspace-server/src/lib.rs index eb414dcfe..83b94b69f 100644 --- a/crates/workspace-server/src/lib.rs +++ b/crates/workspace-server/src/lib.rs @@ -144,6 +144,12 @@ pub enum Error { TicketAssignmentConflict(String), #[error("Workdir attachment conflict: {0}")] WorkdirAttachmentConflict(String), + #[error("Workdir {workdir_id} is already attached to Worker {runtime_id}:{worker_id}")] + WorkdirOccupied { + workdir_id: String, + runtime_id: String, + worker_id: String, + }, #[error("Workspace permission denied: {0}")] WorkspacePermissionDenied(String), #[error("Workspace config update conflict: {0}")] diff --git a/crates/workspace-server/src/retention.rs b/crates/workspace-server/src/retention.rs index c0dc1e64a..1d808bba5 100644 --- a/crates/workspace-server/src/retention.rs +++ b/crates/workspace-server/src/retention.rs @@ -325,7 +325,7 @@ impl SqliteWorkspaceStore { let now = Utc::now().to_rfc3339(); self.with_conn_mut(|conn| { let tx=conn.transaction_with_behavior(TransactionBehavior::Immediate)?; - let policy=load_removal_policy(&tx,&req.workspace_id,&req.worker,erased_journal)?.ok_or_else(|| StoreError::InvalidInput(format!("policy-missing:{}",req.workspace_id)))?; + let policy=load_removal_policy(&tx,&req.workspace_id,&req.worker,erased_journal,true)?.ok_or_else(|| StoreError::InvalidInput(format!("policy-missing:{}",req.workspace_id)))?; let worker=match load_worker(&tx,&req.workspace_id,&req.worker)? { Some(v)=>v, None=>{ @@ -385,7 +385,7 @@ impl SqliteWorkspaceStore { if !plan.blockers.is_empty(){return Err(StoreError::InvalidInput(format!("blocked:{}",serde_json::to_string(&plan.blockers).unwrap())));} let reconciliation=crate::store::subject_deletion_removal_matches(&tx,workspace_id,&plan.worker,&plan.operation_id,&plan.input_fingerprint)?; if !reconciliation { - let policy=load_removal_policy(&tx,workspace_id,&plan.worker,plan.policy_id==ERASED_JOURNAL_POLICY_ID)?.ok_or_else(||StoreError::InvalidInput(format!("policy-missing:{workspace_id}")))?; + let policy=load_removal_policy(&tx,workspace_id,&plan.worker,plan.policy_id==ERASED_JOURNAL_POLICY_ID,plan.policy_id=="backend-subject-removal-archive-v1")?.ok_or_else(||StoreError::InvalidInput(format!("policy-missing:{workspace_id}")))?; if policy.policy_id!=plan.policy_id||policy.policy_digest!=plan.policy_digest{ mark_stale(&tx,&plan,"policy content changed")?;tx.commit()?; return Err(stale_error(&plan,"policy content changed")); @@ -1068,6 +1068,7 @@ fn load_removal_policy( w: &str, worker: &RuntimeWorkerRef, erased_journal: bool, + preserve_subject_history: bool, ) -> crate::Result> { let Some(mut policy) = load_policy(c, w)? else { return Ok(None); @@ -1135,6 +1136,25 @@ fn load_removal_policy( policy.archive_retention = update.archive_retention; policy.diagnostics_disposition = update.diagnostics_disposition; policy.diagnostics_retention_seconds = None; + } else if preserve_subject_history && policy.session_disposition == SessionDisposition::Purge && c.query_row( + "SELECT EXISTS(SELECT 1 FROM worker_create_reservations WHERE workspace_id=?1 AND runtime_id=?2 AND worker_id=?3 AND substr(singleton_key,1,10)='subjektiv:')", + params![w, worker.runtime_id, worker.worker_id], + |r| r.get::<_, bool>(0), + )? { + // Removing a Subject body releases occupancy, not its original history. + // A configured Purge must not silently erase an unfinished Journal. Explicit + // Session erasure uses the separate, proof-bound branch above. + let update = WorkerRetentionPolicyUpdate { + policy_id: "backend-subject-removal-archive-v1".into(), + session_disposition: SessionDisposition::Archive, + metadata_disposition: policy.metadata_disposition, + archive_retention: policy.archive_retention, + diagnostics_disposition: policy.diagnostics_disposition, + diagnostics_retention_seconds: policy.diagnostics_retention_seconds, + }; + policy.policy_id = update.policy_id.clone(); + policy.policy_digest = worker_retention_policy_digest(&update); + policy.session_disposition = update.session_disposition; } Ok(Some(policy)) } @@ -1167,7 +1187,7 @@ fn require_current_subject_deletion_plan( return Ok(()); } if crate::store::subject_deletion_worker_kind(c, &plan.workspace_id, &plan.worker)?.is_some() { - let current = load_removal_policy(c, &plan.workspace_id, &plan.worker, false)? + let current = load_removal_policy(c, &plan.workspace_id, &plan.worker, false, true)? .ok_or_else(|| StoreError::InvalidInput("Subject deletion policy missing".into()))?; if plan.session_disposition != current.session_disposition || plan.metadata_disposition != current.metadata_disposition @@ -2934,7 +2954,7 @@ mod tests { .is_err() ); let fresh = store - .with_conn(|c| load_removal_policy(c, "w", &req().worker, false)) + .with_conn(|c| load_removal_policy(c, "w", &req().worker, false, true)) .unwrap() .unwrap(); assert_eq!(fresh.session_disposition, SessionDisposition::Purge); diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 426d476b7..98a32276d 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -2396,16 +2396,29 @@ impl WorkerRemovalService { )); } }; - match self.worker_projection.publish_ordered(|| { - self.store - .commit_worker_removal( - &self.workspace_id, - &prepared.plan.operation_id, - &prepared.plan.input_fingerprint, - &result, - ) - .map(|commit| ((), commit.catalog)) - }) { + let commit_result = + match subjektiv_journal::finish_worker_removal(self, target, &prepared, &result, || { + self.worker_projection.publish_ordered(|| { + self.store + .commit_worker_removal( + &self.workspace_id, + &prepared.plan.operation_id, + &prepared.plan.input_fingerprint, + &result, + ) + .map(|commit| ((), commit.catalog)) + }) + }) { + Ok(result) => result, + Err(error) => { + return Ok(worker_remove_error_response( + StatusCode::CONFLICT, + "subject_removal_completion_unconfirmed", + &error.to_string(), + )); + } + }; + match commit_result { Ok(()) => { subjektiv_journal::after_worker_removal(self, &result); Ok(worker_remove_success_response(target)) @@ -2780,16 +2793,34 @@ impl WorkerRemovalService { )); } }; - match self.worker_projection.publish_ordered(|| { - self.store - .commit_worker_removal( - &self.workspace_id, - &plan.operation_id, - &plan.input_fingerprint, - &retention_result, - ) - .map(|commit| ((), commit.catalog)) - }) { + let commit_result = match subjektiv_journal::finish_worker_removal( + self, + target, + &prepared, + &retention_result, + || { + self.worker_projection.publish_ordered(|| { + self.store + .commit_worker_removal( + &self.workspace_id, + &plan.operation_id, + &plan.input_fingerprint, + &retention_result, + ) + .map(|commit| ((), commit.catalog)) + }) + }, + ) { + Ok(result) => result, + Err(error) => { + return Ok(worker_remove_error_response( + StatusCode::CONFLICT, + "subject_removal_completion_unconfirmed", + &error.to_string(), + )); + } + }; + match commit_result { Ok(()) => {} Err(error) => { let _ = self.store.fail_worker_removal( @@ -21958,15 +21989,17 @@ async fn scoped_attach_current_worker_workdir( run_worker_workdir_db_stage_locked(&api, &_session_guard, move |api| -> ApiResult<_> { let existing = api .store - .list_worker_workdir_links(&api.config.workspace_id, &stage_worker)? + .list_workdir_worker_links(&api.config.workspace_id, &stage_workdir_id)? .into_iter() - .find(|link| link.unlinked_at.is_none() && link.alias == stage_alias); + .next(); if let Some(existing) = existing { - if existing.workdir_id != stage_workdir_id { - return Err(Error::WorkdirAttachmentConflict(format!( - "Worker {}:{} attachment alias `{stage_alias}` is already bound to Workdir {}", - stage_worker.runtime_id, stage_worker.worker_id, existing.workdir_id - )).into()); + if existing.worker != stage_worker { + return Err(Error::WorkdirOccupied { + workdir_id: stage_workdir_id, + runtime_id: existing.worker.runtime_id, + worker_id: existing.worker.worker_id, + } + .into()); } let effective_capabilities = existing.capabilities.intersection(capabilities); if effective_capabilities != existing.capabilities { @@ -21976,6 +22009,21 @@ async fn scoped_attach_current_worker_workdir( } Ok(Some((existing, effective_capabilities))) } else { + if let Some(existing) = api + .store + .list_worker_workdir_links(&api.config.workspace_id, &stage_worker)? + .into_iter() + .find(|link| link.unlinked_at.is_none() && link.alias == stage_alias) + { + return Err(Error::WorkdirAttachmentConflict(format!( + "Worker {}:{} attachment alias '{}' is already bound to Workdir {}", + stage_worker.runtime_id, + stage_worker.worker_id, + stage_alias, + existing.workdir_id + )) + .into()); + } Ok(None) } }) @@ -21988,7 +22036,7 @@ async fn scoped_attach_current_worker_workdir( .map_err(ApiError::from)?; return Ok(Json(server_api::CurrentWorkerWorkdirAttachmentResponse { workspace_id: api.config.workspace_id.clone(), - alias: alias.to_string(), + alias: link.alias.clone(), working_directory_id: workdir_id.to_string(), capabilities: effective_capabilities, environment: Some( @@ -22003,6 +22051,7 @@ async fn scoped_attach_current_worker_workdir( .await?, ), attached: true, + already_attached: true, })); } close_current_worker_attachment_session_locked(&api, &worker).await?; @@ -22064,6 +22113,7 @@ async fn scoped_attach_current_worker_workdir( capabilities, environment: Some(environment), attached: true, + already_attached: false, })) } @@ -22307,6 +22357,7 @@ async fn detach_current_worker_workdir( capabilities: workdir::WorkdirSessionCapabilities::EMPTY, environment: None, attached: false, + already_attached: false, }) } @@ -28096,7 +28147,14 @@ async fn cleanup_runtime_working_directory( .host()? .run(move || { let registered_runtime = - registered_workdir_runtime_id(&api, &path.working_directory_id)?; + match registered_workdir_runtime_id(&api, &path.working_directory_id) { + Ok(runtime) => runtime, + Err(error) => api + .config_store + .completed_workdir_removal(api.workspace_id(), &path.working_directory_id)? + .map(|removed| removed.runtime_id) + .ok_or(error)?, + }; if registered_runtime != path.runtime_id { return Err(ApiError::from(Error::WorkspacePermissionDenied( "Workdir does not belong to the requested Runtime".to_string(), @@ -28107,6 +28165,7 @@ async fn cleanup_runtime_working_directory( &path.working_directory_id, &source_actor, &request.reason, + &request.operation_id, ) .map_err(ApiError::from) }) @@ -29394,6 +29453,7 @@ async fn cleanup_working_directory( &path.working_directory_id, &source_actor, &request.reason, + &request.operation_id, ) .map_err(ApiError::from) }) @@ -30133,8 +30193,14 @@ fn workdir_removal_failure_guidance(category: &str) -> &'static str { "runtime_unavailable" | "provider_unavailable" => { "Runtime is unavailable. Reconnect it, then retry deletion; the Workdir registry is retained." } - "provider_observation_unknown" | "provider_cleanup_outcome_unknown" => { - "Runtime has not confirmed deletion. Retry to re-observe the target; the Workdir registry is retained." + "provider_cleanup_outcome_unknown" => { + "Deletion outcome is unknown. Reconcile the existing request; recovery only observes and never repeats deletion." + } + "provider_observation_unknown" => { + "The target could not be observed before deletion. Restore provider access before a new request." + } + "request_interrupted" => { + "The request was interrupted before execution. A new explicit request is required." } "unsupported_target" => "This provider does not support Workdir deletion.", _ => { @@ -30170,7 +30236,7 @@ fn current_workdir_removal_attempt_owner() -> Result fn workdir_removal_attempt_is_orphaned(operation: &WorkdirRemovalOperation) -> Result { let Some(owner) = operation.attempt_owner else { - return Ok(operation.attempt_id.is_none()); + return Ok(true); }; match observe_workdir_removal_owner(owner.process_id) { WorkdirRemovalOwnerObservation::Running { @@ -30276,12 +30342,12 @@ fn execute_reserved_workdir_removal_with_provider( recovery: bool, provider: &dyn WorkdirRemovalRuntimeProvider, ) -> Result { - if operation.state == WorkdirRemovalOperationState::Completed - && operation.disposition == Some(WorkdirRemovalDisposition::Removed) - { + if operation.state != WorkdirRemovalOperationState::Pending { return Ok(operation); } - let operation = if recovery && operation.state == WorkdirRemovalOperationState::Pending { + let unstarted = operation.attempt_id.is_none(); + let reconcile_only = recovery || !unstarted; + let operation = if reconcile_only { if !workdir_removal_attempt_is_orphaned(&operation)? { return Err(Error::WorkdirAttachmentConflict(format!( "Workdir removal operation `{}` still has a live attempt owner", @@ -30305,6 +30371,19 @@ fn execute_reserved_workdir_removal_with_provider( api.workdir_remove_attempt_owner, )? }; + if operation.state != WorkdirRemovalOperationState::Pending { + return Ok(operation); + } + if reconcile_only { + if unstarted { + return api.config_store.fail_workdir_removal_operation( + &operation, + "request_interrupted", + true, + ); + } + return reconcile_workdir_removal(api, &operation, provider); + } let guards = match api.config_store.workdir_removal_guards(&operation) { Ok(guards) => guards, Err( @@ -30396,10 +30475,16 @@ fn execute_reserved_workdir_removal_with_provider( let deleted = match deleted { Ok(deleted) => deleted, Err(error) => { - let (category, retryable) = classify_workdir_provider_error(&error); - return api - .config_store - .fail_workdir_removal_operation(&operation, category, retryable); + if let RuntimeRegistryError::RuntimeOperationFailed { code, .. } = &error + && let Some(category) = workdir_cleanup_diagnostic_category(code) + { + return api.config_store.fail_workdir_removal_operation( + &operation, + category, + category != "unsupported_target", + ); + } + return api.config_store.record_workdir_removal_unknown(&operation); } }; let deletion_confirmed = runtime_reports_workdir_not_found(&deleted) @@ -30421,6 +30506,9 @@ fn execute_reserved_workdir_removal_with_provider( category, "Runtime did not confirm Workdir removal; registry retained" ); + if deleted.state == InternalWorkerOperationState::Accepted { + return api.config_store.record_workdir_removal_unknown(&operation); + } return api.config_store.fail_workdir_removal_operation( &operation, category, @@ -30444,42 +30532,71 @@ fn workdir_removal_execution_lock( .clone()) } +fn reserve_requested_workdir_removal( + api: &WorkspaceApi, + working_directory_id: &str, + source_actor: &str, + reason: &str, + request_id: &str, +) -> Result { + let operation_id = crate::workdir_removal::workdir_removal_request_id( + api.workspace_id(), + source_actor, + request_id, + )?; + if let Some(existing) = api + .config_store + .get_workdir_removal_operation(api.workspace_id(), &operation_id)? + { + if existing.working_directory_id != working_directory_id + || existing.source_actor != source_actor + || existing.reason != reason + { + return Err(Error::WorkdirAttachmentConflict( + "Workdir removal request ID was reused with different input".into(), + )); + } + return Ok(existing); + } + let Some(workdir) = api + .config_store + .get_workdir_registry(api.workspace_id(), working_directory_id)? + else { + return api + .config_store + .completed_workdir_removal(api.workspace_id(), working_directory_id)? + .ok_or_else(|| { + Error::InvalidInput(format!("Unknown Workdir \x60{working_directory_id}\x60")) + }); + }; + let intent = workdir_removal_intent(&workdir, source_actor, reason, request_id)?; + api.config_store.reserve_workdir_removal_operation(&intent) +} + fn execute_workdir_removal( api: &WorkspaceApi, working_directory_id: &str, source_actor: &str, reason: &str, + request_id: &str, ) -> Result { let reason = reason.trim(); - if reason.is_empty() || reason.len() > 500 { + if reason.is_empty() || reason.len() > 500 || reason.chars().any(char::is_control) { return Err(Error::InvalidInput( - "Workdir removal reason must be between 1 and 500 bytes".to_string(), + "Workdir removal reason must be between 1 and 500 bytes without controls".into(), )); } let lock = workdir_removal_execution_lock(api, working_directory_id)?; let _guard = lock .lock() - .map_err(|_| Error::Store("Workdir removal lock was poisoned".to_string()))?; - - let operation = if let Some(existing) = - api.config_store.find_workdir_removal_operation_by_intent( - api.workspace_id(), - working_directory_id, - source_actor, - reason, - )? { - existing - } else { - let workdir = api - .config_store - .get_workdir_registry(api.workspace_id(), working_directory_id)? - .ok_or_else(|| { - Error::InvalidInput(format!("Unknown Workdir `{working_directory_id}`")) - })?; - let intent = workdir_removal_intent(&workdir, source_actor, reason)?; - api.config_store - .reserve_workdir_removal_operation(&intent)? - }; + .map_err(|_| Error::Store("Workdir removal lock was poisoned".into()))?; + let operation = reserve_requested_workdir_removal( + api, + working_directory_id, + source_actor, + reason, + request_id, + )?; execute_reserved_workdir_removal(api, operation, false) .map(|operation| workdir_removal_response(&operation)) } @@ -30490,35 +30607,66 @@ fn execute_workdir_removal_for_workspace_deletion( parent_operation_id: &str, ) -> Result { let source_actor = format!("workspace-deletion:{parent_operation_id}"); - let reason = "Workspace deletion"; + let request_id = format!("{parent_operation_id}:{working_directory_id}"); let lock = workdir_removal_execution_lock(api, working_directory_id)?; let _guard = lock .lock() - .map_err(|_| Error::Store("Workdir removal lock was poisoned".to_string()))?; - let operation = if let Some(existing) = - api.config_store.find_workdir_removal_operation_by_intent( - api.workspace_id(), - working_directory_id, - &source_actor, - reason, - )? { - existing - } else { - let workdir = api - .config_store - .get_workdir_registry(api.workspace_id(), working_directory_id)? - .ok_or_else(|| { - Error::InvalidInput(format!("Unknown Workdir `{working_directory_id}`")) - })?; - let intent = workdir_removal_intent(&workdir, &source_actor, reason)?; - api.config_store - .reserve_workdir_removal_operation(&intent)? - }; + .map_err(|_| Error::Store("Workdir removal lock was poisoned".into()))?; + let operation = reserve_requested_workdir_removal( + api, + working_directory_id, + &source_actor, + "Workspace deletion", + &request_id, + )?; api.config_store .append_workspace_deletion_child_operation(parent_operation_id, &operation.operation_id)?; execute_reserved_workdir_removal(api, operation, false) } +// Recovery may confirm an external effect, but never issues another deletion. +fn reconcile_workdir_removal( + api: &WorkspaceApi, + operation: &WorkdirRemovalOperation, + provider: &dyn WorkdirRemovalRuntimeProvider, +) -> Result { + let current = api + .config_store + .get_workdir_registry(&operation.workspace_id, &operation.working_directory_id)?; + if current.is_none() { + if api + .config_store + .completed_workdir_removal(&operation.workspace_id, &operation.working_directory_id)? + .is_some_and(|removed| { + removed.materialization_fingerprint == operation.materialization_fingerprint + }) + { + return api.config_store.commit_workdir_removal_removed(operation); + } + return api.config_store.fail_workdir_removal_operation( + operation, + "authority_changed", + false, + ); + } + if current.as_ref().is_some_and(|record| { + crate::workdir_removal::workdir_materialization_fingerprint(record) + != operation.materialization_fingerprint + }) { + return api.config_store.fail_workdir_removal_operation( + operation, + "authority_changed", + false, + ); + } + match provider.observe_workdir(&operation.runtime_id, &operation.working_directory_id) { + Ok(observed) if runtime_reports_workdir_not_found(&observed) => { + api.config_store.commit_workdir_removal_removed(operation) + } + _ => api.config_store.record_workdir_removal_unknown(operation), + } +} + fn recover_workdir_removals(api: &WorkspaceApi) -> Result<()> { for operation in api .config_store @@ -30962,6 +31110,7 @@ async fn execute_runtime_cleanup_with_context( removal_candidate.workdir_id.as_str(), &requester, &format!("cleanup target {}", removal_candidate.target_id), + &uuid::Uuid::now_v7().to_string(), ) }) .await?; @@ -35042,8 +35191,7 @@ fn finalize_spawn_compensation_after_worker_delete( api, workdir_id, "backend:worker_spawn_compensation", - "remove Workdir created by rejected Worker spawn", - ) { + "remove Workdir created by rejected Worker spawn", &uuid::Uuid::now_v7().to_string(),) { Ok(result) if result.disposition == WorkingDirectoryRemovalDisposition::Removed => {} Ok(result) => diagnostics.push(spawn_compensation_diagnostic( "worker_spawn_compensation_workdir_cleanup_failed", @@ -35079,8 +35227,7 @@ fn cleanup_spawn_created_workdirs( api, workdir_id, "backend:worker_spawn_compensation", - "remove Workdir created before rejected Worker spawn", - ) { + "remove Workdir created before rejected Worker spawn", &uuid::Uuid::now_v7().to_string(),) { Ok(result) if result.disposition == WorkingDirectoryRemovalDisposition::Removed => {} Ok(result) => diagnostics.push(spawn_compensation_diagnostic( "worker_spawn_compensation_workdir_cleanup_failed", @@ -38742,6 +38889,7 @@ impl ApiError { status.as_u16(), match self.error { Error::RestoreObservationConflict => "restore_observation_conflict", + Error::WorkdirOccupied { .. } => "workdir_occupied", Error::RuntimeBindingIdConflict { .. } => "stale_binding", Error::RuntimeBindingFingerprintConflict { .. } => "fingerprint_in_use", _ => status.canonical_reason().unwrap_or("error"), @@ -38765,6 +38913,7 @@ fn api_error_status(error: &Error) -> StatusCode { } Error::TicketAssignmentConflict(_) | Error::WorkdirAttachmentConflict(_) + | Error::WorkdirOccupied { .. } | Error::WorkspaceConfigConflict(_) | Error::RuntimeBindingConflict(_) | Error::RuntimeBindingIdConflict { .. } @@ -62463,7 +62612,8 @@ mod tests { let app = build_router(api.clone()); for (workdir_id, path, reason) in fixtures { - let body = serde_json::to_vec(&json!({ "reason": reason })).unwrap(); + let body = serde_json::to_vec(&json!({ "reason": reason, "operation_id": workdir_id })) + .unwrap(); let response = app .clone() .oneshot(runtime_source_request( @@ -62497,11 +62647,14 @@ mod tests { ); let operation = api .config_store - .find_workdir_removal_operation_by_intent( + .get_workdir_removal_operation( TEST_WORKSPACE_ID, - workdir_id, - "worker:runtime-source:worker-source", - reason, + &crate::workdir_removal::workdir_removal_request_id( + TEST_WORKSPACE_ID, + "worker:runtime-source:worker-source", + workdir_id, + ) + .unwrap(), ) .unwrap() .unwrap(); @@ -62509,6 +62662,29 @@ mod tests { operation.source_actor, "worker:runtime-source:worker-source" ); + for request_id in [workdir_id.to_string(), format!("{workdir_id}-fresh")] { + let replay = app + .clone() + .oneshot(runtime_source_request( + &identity, + Some("worker-source"), + "DELETE", + &path, + serde_json::to_vec(&json!({"reason": reason, "operation_id": request_id})) + .unwrap(), + )) + .await + .unwrap(); + assert_eq!(replay.status(), StatusCode::OK); + let result: WorkingDirectoryRemovalResponse = serde_json::from_slice( + &to_bytes(replay.into_body(), usize::MAX).await.unwrap(), + ) + .unwrap(); + assert_eq!( + result.disposition, + WorkingDirectoryRemovalDisposition::Removed + ); + } assert_eq!(operation.state, WorkdirRemovalOperationState::Completed); assert_eq!( operation.disposition, @@ -62551,7 +62727,10 @@ mod tests { None, "DELETE", &runtime_only_path, - serde_json::to_vec(&json!({ "reason": "runtime only" })).unwrap(), + serde_json::to_vec( + &json!({ "reason": "runtime only", "operation_id": "denied-removal" }), + ) + .unwrap(), )) .await .unwrap(); @@ -62570,7 +62749,10 @@ mod tests { worker_runtime::auth::RUNTIME_REQUEST_SOURCE_PROOF_HEADER, "not-a-valid-proof", ) - .body(Body::from(json!({ "reason": "invalid proof" }).to_string())) + .body(Body::from( + json!({ "reason": "invalid proof", "operation_id": "denied-removal" }) + .to_string(), + )) .unwrap(), ) .await @@ -62586,7 +62768,8 @@ mod tests { .uri(&unauthenticated_path) .header(CONTENT_TYPE, "application/json") .body(Body::from( - json!({ "reason": "unauthenticated" }).to_string(), + json!({ "reason": "unauthenticated", "operation_id": "denied-removal" }) + .to_string(), )) .unwrap(), ) @@ -62627,7 +62810,7 @@ mod tests { build_router(api.clone()), "DELETE", &path, - Some(json!({ "reason": reason })), + Some(json!({ "reason": reason, "operation_id": workdir_id })), &token, StatusCode::OK, ) @@ -62641,11 +62824,14 @@ mod tests { ); let operation = api .config_store - .find_workdir_removal_operation_by_intent( + .get_workdir_removal_operation( TEST_WORKSPACE_ID, - workdir_id, - &format!("account:account-{TEST_WORKSPACE_ID}"), - reason, + &crate::workdir_removal::workdir_removal_request_id( + TEST_WORKSPACE_ID, + &format!("account:account-{TEST_WORKSPACE_ID}"), + workdir_id, + ) + .unwrap(), ) .unwrap() .unwrap(); @@ -62978,8 +63164,13 @@ mod tests { .unwrap(); record.materialization_status = "pending".into(); api.store.upsert_workdir_registry(&record).unwrap(); - let intent = - workdir_removal_intent(&record, "account:owner", "remove docs checkout").unwrap(); + let intent = workdir_removal_intent( + &record, + "account:owner", + "remove docs checkout", + "test-removal-request", + ) + .unwrap(); let pending = api .config_store .reserve_workdir_removal_operation(&intent) @@ -63045,6 +63236,130 @@ mod tests { ); } + #[tokio::test] + async fn current_worker_attach_uses_workdir_ownership_before_requested_alias() { + let mut fixture = manual_worker_assignment_fixture().await; + let identity = + worker_runtime::auth::RuntimeIdentityMaterial::generate(&fixture.worker.runtime_id) + .unwrap(); + configure_runtime_request_auth(&mut fixture.api, &identity, &fixture.worker.runtime_id); + let api = &fixture.api; + let before = api + .store + .list_worker_workdir_links(TEST_WORKSPACE_ID, &fixture.worker) + .unwrap(); + let path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-attachments"); + for alias in ["docs", "different-name", "checkout"] { + let response = build_router(api.clone()) + .oneshot(runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &path, + serde_json::to_vec( + &json!({"alias": alias, "working_directory_id": fixture.docs_workdir_id}), + ) + .unwrap(), + )) + .await + .unwrap(); + let status = response.status(); + let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); + assert_eq!(status, StatusCode::OK, "{}", String::from_utf8_lossy(&body)); + let response: server_api::CurrentWorkerWorkdirAttachmentResponse = + serde_json::from_slice(&body).unwrap(); + assert!(response.attached && response.already_attached); + assert_eq!(response.alias, "docs"); + assert_eq!(response.working_directory_id, fixture.docs_workdir_id); + assert!(response.environment.is_some()); + assert_eq!( + api.store + .list_worker_workdir_links(TEST_WORKSPACE_ID, &fixture.worker) + .unwrap(), + before + ); + } + api.store + .detach_worker_workdir( + TEST_WORKSPACE_ID, + &fixture.worker, + Some(&fixture.docs_workdir_id), + "test detach", + ) + .unwrap(); + sync_runtime_worker_workdir_attachments(api, &fixture.worker).unwrap(); + for (alias, expected) in [ + ("checkout", StatusCode::CONFLICT), + ("new-docs", StatusCode::OK), + ] { + let response = build_router(api.clone()) + .oneshot(runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &path, + serde_json::to_vec( + &json!({"alias": alias, "working_directory_id": fixture.docs_workdir_id}), + ) + .unwrap(), + )) + .await + .unwrap(); + let status = response.status(); + let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); + assert_eq!(status, expected, "{}", String::from_utf8_lossy(&body)); + if status == StatusCode::OK { + let response: server_api::CurrentWorkerWorkdirAttachmentResponse = + serde_json::from_slice(&body).unwrap(); + assert!(response.attached); + assert!(!response.already_attached); + assert_eq!(response.alias, "new-docs"); + } + } + let mut other = api + .store + .get_worker_registry(TEST_WORKSPACE_ID, &fixture.worker) + .unwrap() + .unwrap(); + other.worker.worker_id = "another-owner".into(); + api.store.upsert_worker_registry(&other).unwrap(); + let mut occupied = api + .store + .list_workdir_worker_links(TEST_WORKSPACE_ID, &fixture.docs_workdir_id) + .unwrap() + .pop() + .unwrap(); + api.store + .detach_worker_workdir( + TEST_WORKSPACE_ID, + &fixture.worker, + Some(&fixture.docs_workdir_id), + "transfer fixture", + ) + .unwrap(); + sync_runtime_worker_workdir_attachments(api, &fixture.worker).unwrap(); + occupied.worker = other.worker; + api.store.attach_worker_workdir(&occupied).unwrap(); + let response = build_router(api.clone()) + .oneshot(runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &path, + serde_json::to_vec( + &json!({"alias": "checkout", "working_directory_id": fixture.docs_workdir_id}), + ) + .unwrap(), + )) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::CONFLICT); + let error: server_api::RepositoryApiError = + serde_json::from_slice(&to_bytes(response.into_body(), usize::MAX).await.unwrap()) + .unwrap(); + assert_eq!(error.error, "workdir_occupied"); + } + #[tokio::test] async fn current_worker_attachment_list_and_stale_detach_use_signed_identity() { let mut fixture = manual_worker_assignment_fixture().await; @@ -63755,6 +64070,7 @@ mod tests { &record, "account:owner", &format!("remove {working_directory_id}"), + working_directory_id, ) .unwrap(); let operation = api @@ -63764,6 +64080,212 @@ mod tests { (operation, summary) } + #[tokio::test] + async fn workdir_removal_failure_never_recovers_as_a_job_and_new_request_can_remove() { + let workspace = tempfile::tempdir().unwrap(); + init_clean_git_workspace(workspace.path()); + let api = test_api(workspace.path()).await; + let (operation, summary) = reserve_removal_fixture(&api, "terminal-busy"); + let provider = FakeWorkdirRemovalProvider::new( + workdir_removal_result( + InternalWorkerOperationState::Accepted, + Some(summary.clone()), + Vec::new(), + ), + workdir_removal_result( + InternalWorkerOperationState::Rejected, + None, + vec![RuntimeDiagnostic { + code: "working_directory_cleanup_resource_busy".into(), + severity: HostDiagnosticSeverity::Warning, + message: "busy".into(), + }], + ), + ); + let failed = + execute_reserved_workdir_removal_with_provider(&api, operation, false, &provider) + .unwrap(); + assert_eq!(failed.state, WorkdirRemovalOperationState::Failed); + for recovery in [false, true] { + assert_eq!( + execute_reserved_workdir_removal_with_provider( + &api, + failed.clone(), + recovery, + &provider + ) + .unwrap(), + failed + ); + } + assert_eq!(provider.cleanup_calls(), 1); + assert!( + api.config_store + .recoverable_workdir_removal_operations(api.workspace_id(), 100) + .unwrap() + .is_empty() + ); + let new_request = reserve_requested_workdir_removal( + &api, + &failed.working_directory_id, + &failed.source_actor, + &failed.reason, + "new-explicit-request", + ) + .unwrap(); + assert_ne!(new_request.operation_id, failed.operation_id); + assert!( + reserve_requested_workdir_removal( + &api, + &failed.working_directory_id, + &failed.source_actor, + "changed reason", + "new-explicit-request" + ) + .is_err() + ); + assert!( + reserve_requested_workdir_removal( + &api, + "different-workdir", + &failed.source_actor, + &failed.reason, + "new-explicit-request" + ) + .is_err() + ); + let success = FakeWorkdirRemovalProvider::new( + workdir_removal_result( + InternalWorkerOperationState::Accepted, + Some(summary), + Vec::new(), + ), + confirmed_workdir_cleanup_result(), + ); + let removed = + execute_reserved_workdir_removal_with_provider(&api, new_request, false, &success) + .unwrap(); + assert_eq!( + removed.disposition, + Some(WorkdirRemovalDisposition::Removed) + ); + let already_removed = execute_workdir_removal( + &api, + &failed.working_directory_id, + &failed.source_actor, + &failed.reason, + "another-explicit-request", + ) + .unwrap(); + assert_eq!( + already_removed.disposition, + WorkingDirectoryRemovalDisposition::Removed + ); + assert_eq!( + execute_reserved_workdir_removal_with_provider(&api, failed.clone(), false, &success) + .unwrap(), + failed + ); + assert_eq!(success.cleanup_calls(), 1); + } + + #[tokio::test] + async fn workdir_removal_unknown_outcome_recovery_only_observes_and_confirms_absence() { + let workspace = tempfile::tempdir().unwrap(); + init_clean_git_workspace(workspace.path()); + let api = test_api(workspace.path()).await; + let (operation, summary) = reserve_removal_fixture(&api, "lost-delete-response"); + let provider = FakeWorkdirRemovalProvider::new( + workdir_removal_result( + InternalWorkerOperationState::Accepted, + Some(summary.clone()), + Vec::new(), + ), + confirmed_workdir_cleanup_result(), + ); + *provider.cleanup.lock().unwrap() = + Some(Err(RuntimeRegistryError::RuntimeOperationFailed { + runtime_id: "runtime-test".into(), + code: "transport_timeout".into(), + message: "response lost".into(), + })); + let unknown = + execute_reserved_workdir_removal_with_provider(&api, operation, false, &provider) + .unwrap(); + assert_eq!(unknown.state, WorkdirRemovalOperationState::Pending); + assert_eq!( + unknown.failure_category.as_deref(), + Some("provider_cleanup_outcome_unknown") + ); + assert!(!unknown.retryable); + assert!( + reserve_requested_workdir_removal( + &api, + &unknown.working_directory_id, + &unknown.source_actor, + &unknown.reason, + "do-not-fork-unknown" + ) + .is_err() + ); + *provider.observation.lock().unwrap() = Some(Ok(workdir_removal_result( + InternalWorkerOperationState::Accepted, + Some(summary), + Vec::new(), + ))); + let still_unknown = + execute_reserved_workdir_removal_with_provider(&api, unknown, true, &provider).unwrap(); + assert_eq!(still_unknown.state, WorkdirRemovalOperationState::Pending); + assert_eq!(provider.cleanup_calls(), 1); + *provider.observation.lock().unwrap() = Some(Ok(confirmed_workdir_cleanup_result())); + let removed = + execute_reserved_workdir_removal_with_provider(&api, still_unknown, true, &provider) + .unwrap(); + assert_eq!( + removed.disposition, + Some(WorkdirRemovalDisposition::Removed) + ); + assert_eq!(provider.cleanup_calls(), 1); + assert!( + api.store + .get_workdir_registry(api.workspace_id(), &removed.working_directory_id) + .unwrap() + .is_none() + ); + } + + #[tokio::test] + async fn workdir_removal_unstarted_request_recovery_does_not_start_deletion() { + let workspace = tempfile::tempdir().unwrap(); + init_clean_git_workspace(workspace.path()); + let api = test_api(workspace.path()).await; + let (operation, summary) = reserve_removal_fixture(&api, "unstarted-removal"); + let provider = FakeWorkdirRemovalProvider::new( + workdir_removal_result( + InternalWorkerOperationState::Accepted, + Some(summary), + Vec::new(), + ), + confirmed_workdir_cleanup_result(), + ); + let interrupted = + execute_reserved_workdir_removal_with_provider(&api, operation, true, &provider) + .unwrap(); + assert_eq!(interrupted.state, WorkdirRemovalOperationState::Failed); + assert_eq!( + interrupted.failure_category.as_deref(), + Some("request_interrupted") + ); + assert_eq!(provider.cleanup_calls(), 0); + assert!(provider.observation.lock().unwrap().is_some()); + assert!( + api.store + .get_workdir_registry(api.workspace_id(), &interrupted.working_directory_id) + .unwrap() + .is_some() + ); + } + #[tokio::test] async fn injectable_provider_covers_cleanup_not_found_unknown_dirty_and_unsupported() { let workspace = tempfile::tempdir().unwrap(); @@ -63886,9 +64408,26 @@ mod tests { confirmed_workdir_cleanup_result(), ); let dirty_attempt_id = dirty.attempt_id.clone(); + let replay = execute_reserved_workdir_removal_with_provider( + &api, + dirty.clone(), + false, + &clean_retry_provider, + ) + .unwrap(); + assert_eq!(replay, dirty); + assert_eq!(clean_retry_provider.cleanup_calls(), 0); + let new_request = reserve_requested_workdir_removal( + &api, + &dirty.working_directory_id, + &dirty.source_actor, + &dirty.reason, + "new-clean-request", + ) + .unwrap(); let removed_after_retry = execute_reserved_workdir_removal_with_provider( &api, - dirty, + new_request, false, &clean_retry_provider, ) @@ -64012,7 +64551,8 @@ mod tests { } #[tokio::test] - async fn workdir_removal_retry_rechecks_pending_materialization_changes_and_attachments() { + async fn new_workdir_removal_request_rechecks_pending_materialization_changes_and_attachments() + { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let api = test_api(workspace.path()).await; @@ -64060,8 +64600,16 @@ mod tests { ), confirmed_workdir_cleanup_result(), ); + let request = reserve_requested_workdir_removal( + &api, + &failed.working_directory_id, + &failed.source_actor, + &failed.reason, + "dirty-request", + ) + .unwrap(); let dirty = - execute_reserved_workdir_removal_with_provider(&api, failed, false, &dirty_provider) + execute_reserved_workdir_removal_with_provider(&api, request, false, &dirty_provider) .unwrap(); assert_eq!(dirty.disposition, Some(WorkdirRemovalDisposition::Retained)); assert!(dirty.retryable); @@ -64077,9 +64625,21 @@ mod tests { ), confirmed_workdir_cleanup_result(), ); - let occupied = - execute_reserved_workdir_removal_with_provider(&api, dirty, false, &occupied_provider) - .unwrap(); + let request = reserve_requested_workdir_removal( + &api, + &dirty.working_directory_id, + &dirty.source_actor, + &dirty.reason, + "occupied-request", + ) + .unwrap(); + let occupied = execute_reserved_workdir_removal_with_provider( + &api, + request, + false, + &occupied_provider, + ) + .unwrap(); assert_eq!( occupied.failure_category.as_deref(), Some("blocked_by_live_authority") @@ -64106,14 +64666,18 @@ mod tests { ), confirmed_workdir_cleanup_result(), ); - let removed = execute_reserved_workdir_removal_with_provider( + let request = reserve_requested_workdir_removal( &api, - occupied.clone(), - false, - &retry_provider, + &occupied.working_directory_id, + &occupied.source_actor, + &occupied.reason, + "final-request", ) .unwrap(); - assert_eq!(removed.operation_id, occupied.operation_id); + let removed = + execute_reserved_workdir_removal_with_provider(&api, request, false, &retry_provider) + .unwrap(); + assert_ne!(removed.operation_id, occupied.operation_id); assert_ne!(removed.attempt_id, occupied.attempt_id); assert_eq!( removed.disposition, @@ -64212,7 +64776,15 @@ mod tests { let result = execute_reserved_workdir_removal_with_provider(&api, operation, false, &provider) .unwrap(); - assert!(result.retryable); + assert_eq!(result.retryable, changed_identity); + assert_eq!( + result.state, + if changed_identity { + WorkdirRemovalOperationState::Failed + } else { + WorkdirRemovalOperationState::Pending + } + ); assert_eq!( result.failure_category.as_deref(), Some(if changed_identity { @@ -64246,7 +64818,7 @@ mod tests { } #[tokio::test] - async fn concurrent_orphan_recovery_runs_delayed_provider_cleanup_once() { + async fn concurrent_orphan_recovery_observes_once_without_reissuing_cleanup() { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let api = test_api(workspace.path()).await; @@ -64301,7 +64873,9 @@ mod tests { .iter() .filter(|result| { result.as_ref().is_ok_and(|operation| { - operation.disposition == Some(WorkdirRemovalDisposition::Removed) + operation.state == WorkdirRemovalOperationState::Pending + && operation.failure_category.as_deref() + == Some("provider_cleanup_outcome_unknown") }) }) .count(), @@ -64314,7 +64888,13 @@ mod tests { .count(), 1 ); - assert_eq!(provider.cleanup_calls(), 1); + assert_eq!(provider.cleanup_calls(), 0); + assert!( + api.store + .get_workdir_registry(TEST_WORKSPACE_ID, "orphan-race") + .unwrap() + .is_some() + ); } #[test] @@ -64343,65 +64923,64 @@ mod tests { } #[tokio::test] - async fn durable_workdir_removal_retries_provider_unavailable_without_deleting_registry() { + async fn failed_workdir_removal_replays_without_reexecution_and_new_request_is_distinct() { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let api = test_api(workspace.path()).await; seed_cleanup_workdir(&api, "missing-clean-workdir", "present", "clean"); - - let first = execute_workdir_removal( - &api, - "missing-clean-workdir", - "account:owner", - "remove stale clean Workdir", - ) - .unwrap(); - assert_eq!( - first.disposition, - WorkingDirectoryRemovalDisposition::AttentionRequired - ); - assert!(first.retryable); + let request = |id| { + execute_workdir_removal( + &api, + "missing-clean-workdir", + "account:owner", + "remove stale clean Workdir", + id, + ) + .unwrap() + }; + let saved = |id| { + api.config_store + .get_workdir_removal_operation( + api.workspace_id(), + &crate::workdir_removal::workdir_removal_request_id( + api.workspace_id(), + "account:owner", + id, + ) + .unwrap(), + ) + .unwrap() + .unwrap() + }; + let first = request("request-1"); assert_eq!( first.failure_category.as_deref(), Some("runtime_unavailable") ); + let first_record = saved("request-1"); + assert_eq!(request("request-1"), first); + assert_eq!(saved("request-1"), first_record); + recover_workdir_removals(&api).unwrap(); + assert_eq!(saved("request-1"), first_record); assert!( - api.store - .get_workdir_registry(&api.config.workspace_id, "missing-clean-workdir") + api.config_store + .recoverable_workdir_removal_operations(api.workspace_id(), 100) .unwrap() - .is_some() + .is_empty() ); - - let first_attempt_id = api - .config_store - .find_workdir_removal_operation_by_intent( - &api.config.workspace_id, + assert_eq!(request("request-2"), first); + assert_ne!(saved("request-2").operation_id, first_record.operation_id); + assert_ne!(saved("request-2").attempt_id, first_record.attempt_id); + assert!( + execute_workdir_removal( + &api, "missing-clean-workdir", "account:owner", - "remove stale clean Workdir", + "different reason", + "request-1" ) - .unwrap() - .unwrap() - .attempt_id; - let retry = execute_workdir_removal( - &api, - "missing-clean-workdir", - "account:owner", - "remove stale clean Workdir", - ) - .unwrap(); - assert_eq!(retry, first); - let operation = api - .config_store - .find_workdir_removal_operation_by_intent( - &api.config.workspace_id, - "missing-clean-workdir", - "account:owner", - "remove stale clean Workdir", - ) - .unwrap() - .unwrap(); - assert_ne!(operation.attempt_id, first_attempt_id); + .is_err() + ); } #[tokio::test] @@ -64418,6 +64997,7 @@ mod tests { "occupied-workdir", "account:owner", "remove occupied Workdir", + &uuid::Uuid::now_v7().to_string(), ) .unwrap(); assert_eq!( @@ -64451,6 +65031,7 @@ mod tests { &record, "account:owner", "do not steal a live provider call", + "test-removal-request", ) .unwrap(); let reserved = api @@ -64493,8 +65074,13 @@ mod tests { .get_workdir_registry(&api.config.workspace_id, "recovery-workdir") .unwrap() .unwrap(); - let intent = - workdir_removal_intent(&record, "account:owner", "recover provider cleanup").unwrap(); + let intent = workdir_removal_intent( + &record, + "account:owner", + "recover provider cleanup", + "test-removal-request", + ) + .unwrap(); let reserved = api .config_store .reserve_workdir_removal_operation(&intent) @@ -64520,12 +65106,12 @@ mod tests { .get_workdir_removal_operation(&api.config.workspace_id, &intent.operation_id) .unwrap() .unwrap(); - assert_eq!(operation.state, WorkdirRemovalOperationState::Failed); + assert_eq!(operation.state, WorkdirRemovalOperationState::Pending); assert_ne!(operation.attempt_id, interrupted_attempt.attempt_id); - assert!(operation.retryable); + assert!(!operation.retryable); assert_eq!( operation.failure_category.as_deref(), - Some("runtime_unavailable") + Some("provider_cleanup_outcome_unknown") ); assert!( api.store diff --git a/crates/workspace-server/src/server/subjektiv_journal.rs b/crates/workspace-server/src/server/subjektiv_journal.rs index d1b573327..3c987f33d 100644 --- a/crates/workspace-server/src/server/subjektiv_journal.rs +++ b/crates/workspace-server/src/server/subjektiv_journal.rs @@ -101,6 +101,20 @@ pub(super) fn prepare(api: &WorkspaceApi, reservation: &BackendJobReservation) - authorize_source(api, &attribution, "journal.job.input.read")?; authorize_source(api, &attribution, "journal.job.input.supply")?; let host = host(api, &store, grant)?; + let attr = store + .session_attribution(&grant.session_id) + .map_err(|e| Error::Store(e.to_string()))? + .ok_or_else(|| { + Error::WorkspacePermissionDenied("Journal Session attribution missing".into()) + })?; + if store + .journal_worker_was_released(&attr.subject_id, &attr.runtime_id, &attr.worker_id) + .map_err(domain)? + { + return Err(Error::WorkspacePermissionDenied( + "released Journal body cannot dispatch another attempt".into(), + )); + } let input = saved_input(&reservation.job.request, grant)?; let g = store .prepare_journal_input( @@ -640,6 +654,18 @@ fn dispatch( .map_err(|_| Error::Store("Journal lifecycle lock poisoned".into()))?; authorize_source(api, &attribution, "journal.recording.request")?; let store = store(api)?; + if store + .journal_worker_was_released( + &attribution.subject_id, + &attribution.runtime_id, + &attribution.worker_id, + ) + .map_err(domain)? + { + // Deletion can retain an unfinished prefix. It must not mint attempts or + // transfer the removed body's write capability to its successor. + return Ok(()); + } let host = JournalHostContext::validated_session(&store, attribution.clone()).map_err(domain)?; if let Some(g) = store @@ -1823,8 +1849,8 @@ fn finish_erased_release( .release_subject_singleton(api.workspace_id(), &lease, binding_id, &operation) } -/// Normal deletion cannot substitute for Subject release, even for a stopped or disabled body. -/// Explicit erasure remains a separate monotonic source/diagnostics destruction path. +/// Ordinary removal owns writer revocation and occupancy release. Before execution +/// only preserve/erase source requirements are checked; an active writer is not a blocker. pub(super) fn prepare_worker_removal( service: &WorkerRemovalService, runtime: &RuntimeRegistry, @@ -1852,7 +1878,6 @@ pub(super) fn prepare_worker_removal( &service.journal_registration, ) .map_err(|e| Error::Store(e.to_string()))?; - let host = JournalHostContext::validated_session(&store, attr.clone()).map_err(domain)?; let source = runtime_api::SessionPublicSource::Retained { worker_id: worker_runtime::identity::WorkerId::parse(&target.worker_id) .ok_or_else(|| Error::Store("Worker identity invalid".into()))?, @@ -1869,25 +1894,9 @@ pub(super) fn prepare_worker_removal( finish_erased_release(&api, &attr)?; return Ok(Some(proof)); } - let Some(binding_id) = host.writer_binding_id() else { - return Err(Error::RepositoryConflict( - "Subject writer evidence is missing; ordinary deletion cannot bypass release".into(), - )); - }; - let writer = JournalWriterBinding { - subject_id: attr.subject_id.clone(), - runtime_id: attr.runtime_id.clone(), - worker_id: attr.worker_id.clone(), - binding_id: binding_id.into(), - }; - if store.journal_writer_active(&writer).map_err(domain)? { - return Err(Error::RepositoryConflict("Subject remains occupied: restore to actual Idle, finish Journal recording, then explicitly release before ordinary deletion".into())); - } - if store.journal_retention_required(&host).map_err(domain)? { - return Err(Error::RepositoryConflict( - "Released Subject has unresolved Journal material; retain occupancy and source".into(), - )); - } + // Do not require a live Controller, complete Journal, or prior release here. + // Runtime removal is the physical fence; finish_worker_removal revokes the + // exact old body only after its successful, matching retention receipt. Ok(None) } @@ -1990,20 +1999,6 @@ pub(super) fn validate_removal_retention( // its retained permanent fence, including recovery after the aggregate is gone. return Ok(()); } - let binding_id = host.writer_binding_id().ok_or_else(|| { - Error::RepositoryConflict("Subject writer evidence missing during removal recovery".into()) - })?; - let binding = JournalWriterBinding { - subject_id: attr.subject_id.clone(), - runtime_id: attr.runtime_id.clone(), - worker_id: attr.worker_id.clone(), - binding_id: binding_id.into(), - }; - if store.journal_writer_active(&binding).map_err(domain)? { - return Err(Error::RepositoryConflict( - "Prepared removal cannot bypass explicit Subject release".into(), - )); - } let source = runtime_api::SessionPublicSource::Retained { worker_id: worker_runtime::identity::WorkerId::parse(&target.worker_id) .ok_or_else(|| Error::Store("Worker identity invalid".into()))?, @@ -2011,37 +2006,160 @@ pub(super) fn validate_removal_retention( let runtime = service.runtime.upgrade().ok_or_else(|| { Error::Store("Runtime unavailable for source retention confirmation".into()) })?; - if store.journal_retention_required(&host).map_err(domain)? { - if prepared.runtime_request.session_disposition - != worker_runtime::retention::SessionDisposition::Archive - || prepared.runtime_request.archive_id.is_none() - { - return Err(Error::RepositoryConflict( - "unreflected Journal requires a retained original Session archive".into(), - )); + let retention_required = store.journal_retention_required(&host).map_err(domain)?; + if prepared.runtime_request.session_disposition + == worker_runtime::retention::SessionDisposition::Purge + && prepared.runtime_request.archive_id.is_none() + && !retention_required + && store + .journal_worker_was_released(&attr.subject_id, &attr.runtime_id, &attr.worker_id) + .map_err(domain)? + { + // Compatibility for an already issued, history-complete ordinary Purge + // plan. New Subject plans always Archive. Keep the old fingerprint and + // disposition; never turn this into an erasure or allow an active writer. + // A missing Runtime Worker is NOT success: execute_worker_retention still + // has to return the exact persisted receipt before anything can commit. + // A pending receipt may outlive the physical aggregate while Runtime's + // in-memory catalog still contains the Worker. Pin release is best effort + // in this historical branch; exact Runtime retention replay still enforces + // pins if the source exists, and requires a receipt if it is already gone. + if let Err(error) = retain_source(&runtime, &service.workspace_id, &attr, source, true) { + tracing::debug!(%error, "Historical Purge source unavailable; reconcile exact Runtime receipt"); } - if let Err(original) = retain_source(&runtime, &service.workspace_id, &attr, source, false) - { - // Recovery may already have transferred the physical source before its Backend - // catalog commit; authenticate the exact reserved archive, never another Session. - let archive = runtime_api::SessionPublicSource::Archived { - archive_id: prepared - .runtime_request - .archive_id - .clone() - .expect("checked"), - }; - retain_source(&runtime, &service.workspace_id, &attr, archive, false) - .map_err(|_| original)?; - } - } else { - // This stopped/removal-fenced Session has no admitted remainder; purge need not - // force a generation. A failed release conservatively blocks ordinary deletion. - retain_source(&runtime, &service.workspace_id, &attr, source, true)?; + return Ok(()); + } + if prepared.runtime_request.session_disposition + != worker_runtime::retention::SessionDisposition::Archive + || prepared.runtime_request.archive_id.is_none() + { + return Err(Error::RepositoryConflict( + "Subject removal retains its original Session; explicit erasure is required for Purge" + .into(), + )); + } + let release = !retention_required; + if let Err(original) = retain_source(&runtime, &service.workspace_id, &attr, source, release) { + // A lost reply may follow the physical transfer. Reconcile only the exact + // reserved archive and Session, whether or not an admitted remainder exists. + let archive = runtime_api::SessionPublicSource::Archived { + archive_id: prepared + .runtime_request + .archive_id + .clone() + .expect("checked"), + }; + retain_source(&runtime, &service.workspace_id, &attr, archive, release) + .map_err(|_| original)?; } Ok(()) } +/// A successful Runtime receipt, not catalog liveness, authorizes ending the old +/// writer. The ordinary removal commit releases occupancy atomically with catalog +/// deletion. Until then a crash leaves the singleton occupied and the same receipt +/// can safely repeat this step; no Controller restore or Journal generation is needed. +pub(super) fn finish_worker_removal( + service: &WorkerRemovalService, + target: &RuntimeWorkerRef, + prepared: &crate::retention::PreparedWorkerRemoval, + result: &worker_runtime::retention::WorkerRetentionExecutionResult, + commit: impl FnOnce() -> T, +) -> Result { + if let Some(api) = service.journal_dispatch_api.upgrade() + && subjektiv_deletion::deleting_worker(&api, target)? + { + // Full Subject deletion already fenced the domain. Its canonical retention + // commit, not ordinary Journal writer validation, owns completion. + return Ok(commit()); + } + let Some(attr) = removal_attribution(service, target)? else { + return Ok(commit()); + }; + let request = &prepared.runtime_request; + if prepared.plan.worker != *target + || request.workspace_id != service.workspace_id + || request.source_runtime_id != target.runtime_id + || request.worker_id.to_string() != target.worker_id + || !result.source_removed + || result.worker_id != request.worker_id + || result.operation_id != request.operation_id + || result.input_fingerprint != request.input_fingerprint + || result.session_disposition != request.session_disposition + || result.diagnostics_disposition != request.diagnostics_disposition + || result.diagnostics_retained + != (request.diagnostics_disposition + == worker_runtime::retention::DiagnosticsDisposition::Retain) + { + return Err(Error::RepositoryConflict( + "Subject removal receipt does not match its plan".into(), + )); + } + if request.session_disposition == worker_runtime::retention::SessionDisposition::Archive { + let archive = result.archive.as_ref().ok_or_else(|| { + Error::RepositoryConflict("Subject removal archive receipt is missing".into()) + })?; + if Some(&archive.archive_id) != request.archive_id.as_ref() + || archive.workspace_id != request.workspace_id + || archive.source_runtime_id != request.source_runtime_id + || archive.source_worker_id != request.worker_id + || archive.source_session_id != attr.session_id + || archive.operation_id != request.operation_id + || archive.input_fingerprint != request.input_fingerprint + || archive.policy_id != request.policy_id + { + return Err(Error::RepositoryConflict( + "Subject removal archive identity mismatch".into(), + )); + } + } + let api = service.journal_dispatch_api.upgrade().ok_or_else(|| { + Error::Store("Workspace API unavailable for Subject removal completion".into()) + })?; + let gate = lifecycle_lock(&api, &attr.subject_id)?; + let _guard = gate + .lock() + .map_err(|_| Error::Store("Journal lifecycle lock poisoned".into()))?; + let store = store(&api)?; + let host = JournalHostContext::validated_session(&store, attr.clone()).map_err(domain)?; + if store.journal_session_erased(&host).map_err(domain)? { + if request.session_disposition == worker_runtime::retention::SessionDisposition::Archive { + // Erasure may win after Runtime archive transfer. Do not publish that + // archive or skip writer revocation: the same removal retry must take + // erased_archive_removal_proof and its physical-erasure commit path. + return Err(Error::RepositoryConflict( + "Session erased during removal; reconcile the same removal operation".into(), + )); + } + return Ok(commit()); + } + store.release_removed_journal_body(&host).map_err(domain)?; + // Result delivery takes the same Subject gate. Revocation is durable; release + // the gate while reconciling jobs, then recheck erasure before publishing. + drop(_guard); + for session in store + .subject_session_attributions(&attr.subject_id) + .map_err(|e| Error::Store(e.to_string()))? + .into_iter() + .filter(|a| a.runtime_id == target.runtime_id && a.worker_id == target.worker_id) + { + reconcile_session_jobs(&api, &session, false)?; + } + let _guard = gate + .lock() + .map_err(|_| Error::Store("Journal lifecycle lock poisoned".into()))?; + if store.journal_session_erased(&host).map_err(domain)? + && request.session_disposition == worker_runtime::retention::SessionDisposition::Archive + { + return Err(Error::RepositoryConflict( + "Session erased during removal; reconcile the same removal operation".into(), + )); + } + // Keep erasure excluded through the catalog commit: otherwise a tombstone + // could win between revocation and publication of the retained archive. + Ok(commit()) +} + #[derive(Deserialize)] #[serde(deny_unknown_fields)] pub(super) struct Empty {} @@ -2196,7 +2314,28 @@ pub(super) fn reconcile_unknown( .as_ref() .ok_or_else(|| Error::InvalidInput("Journal recovery requires a grant".into()))?; let store = store(api)?; - let host = host(api, &store, grant)?; + let attr = store + .session_attribution(&grant.session_id) + .map_err(|e| Error::Store(e.to_string()))? + .filter(|a| a.subject_id == grant.subject_id) + .ok_or_else(|| Error::WorkspacePermissionDenied("Journal attribution mismatch".into()))?; + let released = store + .journal_worker_was_released(&attr.subject_id, &attr.runtime_id, &attr.worker_id) + .map_err(domain)?; + let host = if released { + // Recovery reads immutable evidence, never renews a legacy writer grant. + let host = JournalHostContext::validated_session(&store, attr.clone()).map_err(domain)?; + if !grant.binding_id.is_empty() + && host.writer_binding_id() != Some(grant.binding_id.as_str()) + { + return Err(Error::WorkspacePermissionDenied( + "Journal recovery binding mismatch".into(), + )); + } + host + } else { + host(api, &store, grant)? + }; let erased = store.journal_session_erased(&host).map_err(domain)?; if !erased { job.load_executable_request()?; @@ -2204,70 +2343,93 @@ pub(super) fn reconcile_unknown( job_id: job.request.job_id.clone(), attempt_id: attempt.attempt_id.clone(), }; - let g = if legacy_descriptor(&job.request) { - // Only reconcile already-saved legacy receipts; never redispatch descriptors. + let generation = if released { store - .prepare_journal_generation( - &host, - &grant.journal_id, - binding, - grant.selected_ranges.clone(), - ) + .retained_journal_generation(&host, &grant.journal_id, &binding) .map_err(domain)? + } else if legacy_descriptor(&job.request) { + Some( + store + .prepare_journal_generation( + &host, + &grant.journal_id, + binding, + grant.selected_ranges.clone(), + ) + .map_err(domain)?, + ) } else { - let input = saved_input(&job.request, grant)?; - let generation = store - .prepare_journal_input(&host, &input, binding) - .map_err(domain)?; - input.validate_generation(&generation).map_err(domain)?; - generation + // Preserve active-body recovery: its saved intent can still establish + // the original generation fence. A released body must only read it. + Some( + store + .prepare_journal_input(&host, &saved_input(&job.request, grant)?, binding) + .map_err(domain)?, + ) }; - let ctx = JournalJobContext::validated_recovery(&store, &host, &g.id, g.job.clone()) - .map_err(domain)?; - match journal::service::finish(&store, &ctx) { - Ok(completion) => { - let submission = BackendJobResultSubmission { - job_id: job.request.job_id.clone(), - attempt_id: attempt.attempt_id.clone(), - input_digest: attempt.input_digest.clone(), - result: serde_json::to_value(completion) - .map_err(|e| Error::Store(e.to_string()))?, - }; - let current = api.store.reconcile_backend_journal_attempt( - api.workspace_id(), - attempt, - Some(&submission), - &Utc::now().to_rfc3339(), - )?; - let saved = api - .store - .get_backend_job(api.workspace_id(), &job.request.job_id)? - .ok_or_else(|| Error::Store("reconciled Job missing".into()))?; - notification_delivery::deliver_job_result( - api, - &BackendJobResultAcceptance { - job: saved, - attempt: current.clone(), - replayed: false, - }, - ); - if current.worker_cleanup_state == Some(BackendJobWorkerCleanupState::Completed) { - drain_after_completion(api, &job.request); - if job.input_disposition == crate::backend_job::JobInputDisposition::Available { - api.store.reclaim_backend_job_input( - api.workspace_id(), - &job.request.job_id, - &job.input_digest, - crate::backend_job::JobInputRetirementReason::Completed, - )?; + if let Some(g) = generation { + if g.selected_ranges != grant.selected_ranges { + return Err(Error::WorkspacePermissionDenied( + "Journal recovery selection mismatch".into(), + )); + } + if !legacy_descriptor(&job.request) { + saved_input(&job.request, grant)? + .validate_generation(&g) + .map_err(domain)?; + } + let ctx = JournalJobContext::validated_recovery(&store, &host, &g.id, g.job.clone()) + .map_err(domain)?; + match journal::service::finish(&store, &ctx) { + Ok(completion) => { + let submission = BackendJobResultSubmission { + job_id: job.request.job_id.clone(), + attempt_id: attempt.attempt_id.clone(), + input_digest: attempt.input_digest.clone(), + result: serde_json::to_value(completion) + .map_err(|e| Error::Store(e.to_string()))?, + }; + let current = api.store.reconcile_backend_journal_attempt( + api.workspace_id(), + attempt, + Some(&submission), + &Utc::now().to_rfc3339(), + )?; + let saved = api + .store + .get_backend_job(api.workspace_id(), &job.request.job_id)? + .ok_or_else(|| Error::Store("reconciled Job missing".into()))?; + notification_delivery::deliver_job_result( + api, + &BackendJobResultAcceptance { + job: saved, + attempt: current.clone(), + replayed: false, + }, + ); + if current.worker_cleanup_state == Some(BackendJobWorkerCleanupState::Completed) + { + if !released { + drain_after_completion(api, &job.request); + } + if job.input_disposition + == crate::backend_job::JobInputDisposition::Available + { + api.store.reclaim_backend_job_input( + api.workspace_id(), + &job.request.job_id, + &job.input_digest, + crate::backend_job::JobInputRetirementReason::Completed, + )?; + } + } else { + api.schedule_backend_job_worker_cleanup(¤t); } - } else { - api.schedule_backend_job_worker_cleanup(¤t); + return Ok(()); } - return Ok(()); + Err(JournalError::Conflict(_) | JournalError::PermissionDenied(_)) => {} + Err(error) => return Err(domain(error)), } - Err(JournalError::Conflict(_) | JournalError::PermissionDenied(_)) => {} - Err(error) => return Err(domain(error)), } } if attempt.worker_cleanup_state != Some(BackendJobWorkerCleanupState::Completed) { @@ -2286,11 +2448,17 @@ pub(super) fn reconcile_unknown( false }; if !confirmed_purge { + if released { + // Source deletion already fenced this generation. An unbound Unknown + // still lacks execution-cleanup proof: preserve Unknown, but it no + // longer owns the Subject or blocks removal of the original body. + return Ok(()); + } return Err(Error::RepositoryConflict( "Unknown Journal attempt has no committed stop/Purge fence; not retried".into(), )); } - if !erased { + if !erased && !released { close(api, &job.request, attempt)?; } api.store.reconcile_backend_journal_attempt( @@ -2299,7 +2467,7 @@ pub(super) fn reconcile_unknown( None, &Utc::now().to_rfc3339(), )?; - if !erased { + if !erased && !released { // Resume the same saved body input only after canonical execution/domain fences. // dispatch selects its retained intent before it can capture any newer source. let attr = store @@ -2316,6 +2484,10 @@ fn reconcile_session_jobs( attr: &crate::subjektiv::SubjectSessionAttribution, erased: bool, ) -> Result<()> { + let released = !erased + && store(api)? + .journal_worker_was_released(&attr.subject_id, &attr.runtime_id, &attr.worker_id) + .map_err(domain)?; let mut after: Option = None; loop { let jobs = api.store.list_backend_journal_jobs( @@ -2330,6 +2502,29 @@ fn reconcile_session_jobs( .store .get_backend_job_attempt(api.workspace_id(), &job.request.job_id, &id)? .ok_or_else(|| Error::Store("Journal current attempt missing".into()))?; + if released { + // Writer revocation has already fenced all Journal mutations. End + // pending/Unknown attempts without claiming their input was reflected; + // saved input, operations and original source remain retained. + if !attempt.state.terminal() { + attempt = api.store.finish_backend_job_attempt( + api.workspace_id(), + &job.request.job_id, + &id, + BackendJobAttemptState::Failed, + "journal_body_released", + "Source Worker was removed; unfinished Journal history retained", + &Utc::now().to_rfc3339(), + )?; + } + api.schedule_backend_job_worker_cleanup(&attempt); + if attempt.state == BackendJobAttemptState::Unknown { + // A saved checkpoint may settle success; otherwise retain Unknown + // until canonical Job-worker cleanup has supplied its Purge fence. + reconcile_unknown(api, job, &attempt)?; + } + continue; + } if !erased && legacy_descriptor(&job.request) && job.input_disposition == crate::backend_job::JobInputDisposition::Available diff --git a/crates/workspace-server/src/server/tests/subjektiv_journal_tests.rs b/crates/workspace-server/src/server/tests/subjektiv_journal_tests.rs index 303158afe..a746f39a7 100644 --- a/crates/workspace-server/src/server/tests/subjektiv_journal_tests.rs +++ b/crates/workspace-server/src/server/tests/subjektiv_journal_tests.rs @@ -1263,6 +1263,7 @@ async fn unknown_partial_waits_for_durable_purge_fence_and_retains_sources_when_ "a stale cleanup snapshot must not overwrite newer evidence" ); subjektiv_journal::reconcile_unknown(&f.api, &dispatched.job, &unknown).unwrap(); + f.execution.take_inputs(); f.api .store .update_worker_retention( @@ -1475,7 +1476,7 @@ async fn journal_reserved_restart_dispatches_exact_intent_once_and_unbound_unkno } #[tokio::test] -async fn subject_source_removal_is_fenced_and_pin_releases_after_saved_completion() { +async fn stopped_source_pin_releases_after_saved_completion_and_removal() { let f = RuntimeFixture::new().await; let dispatched = f.dispatch_input(); let worker = dispatched.attempt.worker.as_ref().unwrap(); @@ -1492,17 +1493,6 @@ async fn subject_source_removal_is_fenced_and_pin_releases_after_saved_completio }, ) .unwrap(); - let response = WorkerRemovalService::new(&f.api) - .execute_target_removal_on_host(f.api.runtime.as_ref(), &f.source, "archive original", None) - .await - .unwrap(); - assert_eq!( - response.status, - StatusCode::CONFLICT.as_u16(), - "{}", - response.body - ); - assert!(response.body.contains("journal_handoff_incomplete")); assert!(session_root(f.dir.path(), &f.source).exists()); assert!( f.api @@ -1575,44 +1565,6 @@ async fn subject_source_removal_is_fenced_and_pin_releases_after_saved_completio !retention.pins(f.session).unwrap().is_empty(), "saved reflection alone does not release the still-owned source" ); - // This deterministic fixture has no Controller to prove a live disconnect. - // Seed an explicitly completed historical release, as in the archive-recovery - // component tests; connected release proof is exercised separately. - let lease = f - .api - .store - .require_current_worker_singleton_owner(TEST_WORKSPACE_ID, &f.source) - .unwrap() - .unwrap(); - let binding_id = f.host.writer_binding_id().unwrap(); - let operation = "completed-source-release-before-archive"; - f.api - .config_store - .reserve_subject_release(TEST_WORKSPACE_ID, &lease, binding_id, operation) - .unwrap(); - f.store - .release_journal_writer(&JournalWriterBinding { - subject_id: f - .request - .grants - .subjektiv_journal - .as_ref() - .unwrap() - .subject_id - .clone(), - runtime_id: f.source.runtime_id.clone(), - worker_id: f.source.worker_id.clone(), - binding_id: binding_id.into(), - }) - .unwrap(); - f.api - .config_store - .confirm_subject_release_proof(TEST_WORKSPACE_ID, &lease, binding_id, operation, false) - .unwrap(); - f.api - .config_store - .release_subject_singleton(TEST_WORKSPACE_ID, &lease, binding_id, operation) - .unwrap(); let response = WorkerRemovalService::new(&f.api) .execute_target_removal_on_host(f.api.runtime.as_ref(), &f.source, "archive original", None) .await @@ -1625,7 +1577,7 @@ async fn subject_source_removal_is_fenced_and_pin_releases_after_saved_completio ); assert!( retention.pins(f.session).unwrap().is_empty(), - "completed reflection and explicit release allow archive and source pin release" + "completed reflection and confirmed removal allow archive and source pin release" ); assert!(!session_root(f.dir.path(), worker).exists()); assert!( @@ -1634,7 +1586,7 @@ async fn subject_source_removal_is_fenced_and_pin_releases_after_saved_completio .worker_session_archive(TEST_WORKSPACE_ID, &f.source, &f.session.to_string()) .unwrap() .is_some(), - "the explicitly released source is archived after saved reflection" + "the removed source is archived after saved reflection" ); } @@ -2244,6 +2196,114 @@ impl SubjectRemovalFixture { .release_subject_singleton(TEST_WORKSPACE_ID, &lease, &binding.binding_id, operation) .unwrap(); } + fn pending_journal(&self) -> BackendJobReservation { + self.pending_journal_input(false) + } + + fn pending_journal_input(&self, legacy: bool) -> BackendJobReservation { + let capture = + subjektiv_journal::capture_input(&self.api, &self.attr, 0, 0, 48 * 1024).unwrap(); + let ranges = subjektiv::journal::recording::capture_ranges(&capture).unwrap(); + let id = self + .store + .accept_journal_sources(&self.host, "unfinished history", &ranges) + .unwrap(); + let input = subjektiv::journal::recording::JournalInput::build( + &self.store, + &self.host, + &id, + |order, offset, budget| { + subjektiv_journal::capture_input(&self.api, &self.attr, order, offset, budget) + .map_err(|e| JournalError::Conflict(e.to_string())) + }, + ) + .unwrap(); + let mut request = + subjektiv_journal::recording_request(&self.host, self.attr.clone(), &id, input) + .unwrap(); + if legacy { + let grant = request.grants.subjektiv_journal.as_mut().unwrap(); + grant.binding_id.clear(); + request.input = + serde_json::json!({"journal_id":id, "selected_ranges":grant.selected_ranges}); + } + let reserved = self + .api + .store + .reserve_backend_job(TEST_WORKSPACE_ID, &request, &Utc::now().to_rfc3339()) + .unwrap(); + if legacy { + self.store + .prepare_journal_generation( + &self.host, + &id, + subjektiv::api::JobAttemptBinding { + job_id: request.job_id.clone(), + attempt_id: reserved.attempt.attempt_id.clone(), + }, + request + .grants + .subjektiv_journal + .as_ref() + .unwrap() + .selected_ranges + .clone(), + ) + .unwrap(); + } else { + subjektiv_journal::prepare(&self.api, &reserved).unwrap(); + } + reserved + } + + fn assert_removed_with_history(&self) -> String { + assert!( + self.api + .store + .get_worker_registry(TEST_WORKSPACE_ID, &self.target) + .unwrap() + .is_none() + ); + let key = crate::subjektiv::subject_worker_singleton_key(&self.attr.subject_id).unwrap(); + assert!( + self.api + .store + .current_worker_singleton_owner(TEST_WORKSPACE_ID, &key) + .unwrap() + .is_none() + ); + assert!( + self.store + .journal_worker_was_released( + &self.attr.subject_id, + &self.target.runtime_id, + &self.target.worker_id + ) + .unwrap() + ); + assert!(!self.store.journal_session_erased(&self.host).unwrap()); + assert!(!session_root(self.dir.path(), &self.target).exists()); + self.api + .config_store + .worker_session_archive(TEST_WORKSPACE_ID, &self.target, &self.attr.session_id) + .unwrap() + .expect("ordinary deletion retains original history") + .archive_id + } + + fn assert_occupied(&self) { + let key = crate::subjektiv::subject_worker_singleton_key(&self.attr.subject_id).unwrap(); + assert_eq!( + self.api + .store + .current_worker_singleton_owner(TEST_WORKSPACE_ID, &key) + .unwrap() + .unwrap() + .worker, + self.target + ); + } + fn remove(&self) -> worker::WorkspaceResponse { let key = self .api @@ -2293,6 +2353,811 @@ async fn subject_release_never_substitutes_catalog_idle_or_stopped_for_controlle assert!(f.store.journal_for_session(&f.host).unwrap().is_none()); } +#[tokio::test] +async fn stopped_subject_removal_releases_occupancy_and_preserves_unfinished_history() { + let f = SubjectRemovalFixture::new().await; + let reserved = f.pending_journal(); + let grant = reserved + .job + .request + .grants + .subjektiv_journal + .as_ref() + .unwrap(); + let before = f.store.read_journal(&f.host, &grant.journal_id).unwrap(); + // Even a Workspace Purge policy is not implicit authority to erase Subject history. + let policy = f + .api + .config_store + .worker_retention_policy(TEST_WORKSPACE_ID) + .unwrap() + .unwrap(); + let purge = f + .api + .config_store + .update_worker_retention_policy( + TEST_WORKSPACE_ID, + &policy.policy_digest, + &crate::retention::WorkerRetentionPolicyUpdate { + policy_id: "test-purge".into(), + session_disposition: worker_runtime::retention::SessionDisposition::Purge, + metadata_disposition: policy.metadata_disposition, + archive_retention: policy.archive_retention, + diagnostics_disposition: policy.diagnostics_disposition, + diagnostics_retention_seconds: policy.diagnostics_retention_seconds, + }, + ) + .unwrap(); + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + let archive = f.assert_removed_with_history(); + let runtime_api::SessionMaterialCatalogAvailability::Page { page } = f + .api + .runtime + .session_material_catalog( + &f.target.runtime_id, + runtime_api::SessionMaterialCatalogRequest { + workspace_id: TEST_WORKSPACE_ID.into(), + source: runtime_api::SessionPublicSource::Archived { + archive_id: archive, + }, + expected_session_id: f.attr.session_id.clone(), + expected_catalog_sha256: None, + offset: 0, + limit: 100, + }, + ) + .unwrap() + else { + panic!("original history must remain readable") + }; + assert_eq!(page.descriptors.len(), 2); + let after = f.store.read_journal(&f.host, &grant.journal_id).unwrap(); + assert_eq!(before.process_order, after.process_order); + assert_eq!(before.process_byte, after.process_byte); + assert!(f.store.journal_retention_required(&f.host).unwrap()); + assert!( + f.store + .active_journal_generation(&f.host, &grant.journal_id) + .is_err() + ); + assert!(subjektiv_journal::prepare(&f.api, &reserved).is_err()); + let job = f + .api + .store + .get_backend_job(TEST_WORKSPACE_ID, &reserved.job.request.job_id) + .unwrap() + .unwrap(); + assert_eq!(job.state, BackendJobState::Failed); + assert_eq!( + job.load_executable_request().unwrap().input, + reserved.job.request.input + ); + assert_eq!( + f.api + .config_store + .worker_retention_policy(TEST_WORKSPACE_ID) + .unwrap() + .unwrap(), + purge + ); + + let Json(next) = scoped_create_workspace_worker( + State(f.api.clone()), + AxumPath(ScopedWorkspacePath { + workspace_id: TEST_WORKSPACE_ID.into(), + }), + HeaderMap::new(), + Json(CreateWorkspaceWorkerRequest { + runtime_id: f.target.runtime_id.clone(), + display_name: "replacement".into(), + singleton_key: None, + profile: Some("builtin:companion".into()), + ticket_assignment: None, + initial_submit: vec![], + workdir_attachments: vec![], + feature_connections: server_api::WorkspaceWorkerFeatureConnectionsRequest { + subjektiv: Some(server_api::SubjektivWorkerConnectionRequest { + subject_id: f.attr.subject_id.clone(), + }), + }, + control_operation_id: None, + execution_grant: None, + }), + ) + .await + .unwrap(); + assert_ne!(next.worker_id, f.target.worker_id); + let key = crate::subjektiv::subject_worker_singleton_key(&f.attr.subject_id).unwrap(); + let successor = f + .api + .store + .current_worker_singleton_owner(TEST_WORKSPACE_ID, &key) + .unwrap() + .unwrap(); + assert_eq!(successor.worker.worker_id, next.worker_id); + let replay = WorkerRemovalService::new(&f.api) + .execute_target_removal_on_host( + f.api.runtime.as_ref(), + &f.target, + "repeat old deletion", + None, + ) + .await + .unwrap(); + assert_eq!(replay.status, StatusCode::OK.as_u16(), "{}", replay.body); + assert_eq!( + f.api + .store + .current_worker_singleton_owner(TEST_WORKSPACE_ID, &key) + .unwrap() + .unwrap(), + successor + ); + assert!(subjektiv_journal::bind_host_session(&f.api, f.attr.clone()).is_err()); + subjektiv_journal::drain_after_completion(&f.api, &reserved.job.request); + assert_eq!( + f.api + .store + .get_backend_job(TEST_WORKSPACE_ID, &reserved.job.request.job_id) + .unwrap() + .unwrap() + .current_attempt, + 1 + ); +} + +#[tokio::test] +async fn subject_removal_archive_failure_does_not_release_writer_or_occupancy() { + let f = SubjectRemovalFixture::new().await; + let archive_parent = f + .dir + .path() + .join(".test-embedded-runtime-store/archives/workers"); + std::fs::create_dir_all(archive_parent.parent().unwrap()).unwrap(); + std::fs::write(&archive_parent, "injected archive failure").unwrap(); + assert_ne!(f.remove().status, StatusCode::OK.as_u16()); + f.assert_occupied(); + assert!( + !f.store + .journal_worker_was_released( + &f.attr.subject_id, + &f.target.runtime_id, + &f.target.worker_id + ) + .unwrap() + ); + assert!(session_root(f.dir.path(), &f.target).exists()); + let old = f + .api + .config_store + .recover_worker_removal_execution(TEST_WORKSPACE_ID, &f.target) + .unwrap() + .unwrap(); + std::fs::remove_file(archive_parent).unwrap(); + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + f.assert_removed_with_history(); + assert_eq!( + f.api + .config_store + .recover_worker_removal_execution(TEST_WORKSPACE_ID, &f.target) + .unwrap() + .unwrap() + .plan + .operation_id, + old.plan.operation_id + ); +} + +#[tokio::test] +async fn subject_removal_reopens_after_writer_revocation_before_catalog_commit() { + let f = SubjectRemovalFixture::new().await; + let conn = rusqlite::Connection::open(&f.api.config.database_path).unwrap(); + conn.execute_batch("CREATE TRIGGER interrupt_subject_delete BEFORE DELETE ON worker_registry BEGIN SELECT RAISE(ABORT,'injected metadata failure'); END;").unwrap(); + assert_ne!(f.remove().status, StatusCode::OK.as_u16()); + assert!(!session_root(f.dir.path(), &f.target).exists()); + assert!( + f.store + .journal_worker_was_released( + &f.attr.subject_id, + &f.target.runtime_id, + &f.target.worker_id + ) + .unwrap() + ); + f.assert_occupied(); // Runtime deletion alone is not a successful Backend deletion. + conn.execute_batch("DROP TRIGGER interrupt_subject_delete;") + .unwrap(); + drop(conn); + let f = f.reopen().await; + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + f.assert_removed_with_history(); +} + +#[tokio::test] +async fn subject_removal_rejects_mismatched_runtime_receipts_before_revocation() { + let f = SubjectRemovalFixture::new().await; + let inventory = f.api.runtime.worker_retention_inventory(&f.target).unwrap(); + let plan = f + .api + .config_store + .plan_worker_removal( + &crate::retention::WorkerRemovalPlanRequest { + workspace_id: TEST_WORKSPACE_ID.into(), + worker: f.target.clone(), + reason: "receipt recovery".into(), + journal_erasure: None, + }, + &inventory, + ) + .unwrap(); + let prepared = f + .api + .config_store + .prepare_worker_removal_execution(TEST_WORKSPACE_ID, &plan.plan_id, &plan.input_fingerprint) + .unwrap(); + let service = WorkerRemovalService::new(&f.api); + subjektiv_journal::validate_removal_retention(&service, &f.target, &prepared).unwrap(); + let result = f + .api + .runtime + .execute_worker_retention(&f.target, prepared.runtime_request.clone()) + .unwrap(); + for variant in 0..5 { + let mut wrong = result.clone(); + match variant { + 0 => wrong.source_removed = false, + 1 => wrong.operation_id.push_str("-wrong"), + 2 => wrong.input_fingerprint.push_str("-wrong"), + 3 => wrong + .archive + .as_mut() + .unwrap() + .source_session_id + .push_str("-wrong"), + _ => wrong.archive = None, + } + assert!( + subjektiv_journal::finish_worker_removal(&service, &f.target, &prepared, &wrong, || ()) + .is_err() + ); + f.assert_occupied(); + assert!( + !f.store + .journal_worker_was_released( + &f.attr.subject_id, + &f.target.runtime_id, + &f.target.worker_id + ) + .unwrap() + ); + } + // Simulate a lost Runtime reply: ordinary retry must recover its exact receipt. + let f = f.reopen().await; + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + f.assert_removed_with_history(); +} + +#[tokio::test] +async fn subject_removal_unknown_job_settles_after_cleanup_without_restarting_old_writer() { + let f = SubjectRemovalFixture::new().await; + let reserved = f.pending_journal(); + let dispatched = f + .api + .dispatch_backend_job_reservation(reserved.clone()) + .unwrap(); + let worker = dispatched.attempt.worker.as_ref().unwrap(); + seed_session( + &session_root(f.dir.path(), worker), + session_store::new_session_id(), + message("delivered immutable Journal input"), + session_store::LoggedSessionHistoryOrigin::LocalHumanInput, + ); + f.api + .store + .update_worker_retention( + TEST_WORKSPACE_ID, + worker, + "pinned", + &Utc::now().to_rfc3339(), + ) + .unwrap(); + f.api + .finish_backend_job_attempt( + &reserved.job.request.job_id, + &dispatched.attempt.attempt_id, + BackendJobAttemptState::Unknown, + "response_lost", + "test lost result", + ) + .unwrap(); + wait_for_backend_job_worker_cleanup( + &f.api, + &reserved.job.request.job_id, + &dispatched.attempt.attempt_id, + BackendJobWorkerCleanupState::Failed, + ) + .await; + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + f.assert_removed_with_history(); + assert_eq!( + f.api + .store + .get_backend_job(TEST_WORKSPACE_ID, &reserved.job.request.job_id) + .unwrap() + .unwrap() + .state, + BackendJobState::Unknown + ); + f.api + .store + .update_worker_retention( + TEST_WORKSPACE_ID, + worker, + "normal", + &Utc::now().to_rfc3339(), + ) + .unwrap(); + let attempt = f + .api + .store + .get_backend_job_attempt( + TEST_WORKSPACE_ID, + &reserved.job.request.job_id, + &dispatched.attempt.attempt_id, + ) + .unwrap() + .unwrap(); + f.api.schedule_backend_job_worker_cleanup(&attempt); + wait_for_backend_job_worker_cleanup( + &f.api, + &reserved.job.request.job_id, + &dispatched.attempt.attempt_id, + BackendJobWorkerCleanupState::Completed, + ) + .await; + // Cleanup's own reconciliation owns the CAS; do not race it with a stale snapshot. + tokio::time::timeout(std::time::Duration::from_secs(10), async { + loop { + let job = f + .api + .store + .get_backend_job(TEST_WORKSPACE_ID, &reserved.job.request.job_id) + .unwrap() + .unwrap(); + if job.state == BackendJobState::Failed { + break; + } + tokio::task::yield_now().await; + } + }) + .await + .unwrap(); + // Exercise restart recovery against the persisted state while the asynchronous + // cleanup task is still allowed to hold this fixture's Runtime owner lock. + subjektiv_journal::recover(&f.api).unwrap(); + let job = f + .api + .store + .get_backend_job(TEST_WORKSPACE_ID, &reserved.job.request.job_id) + .unwrap() + .unwrap(); + assert_eq!(job.state, BackendJobState::Failed); + assert_eq!(job.current_attempt, 1); + assert_eq!( + job.load_executable_request().unwrap().input, + reserved.job.request.input + ); + assert!(f.store.journal_retention_required(&f.host).unwrap()); +} + +#[tokio::test] +async fn subject_removal_erasure_between_runtime_receipt_and_commit_requires_reconciliation() { + let f = SubjectRemovalFixture::new().await; + let inventory = f.api.runtime.worker_retention_inventory(&f.target).unwrap(); + let plan = f + .api + .config_store + .plan_worker_removal( + &crate::retention::WorkerRemovalPlanRequest { + workspace_id: TEST_WORKSPACE_ID.into(), + worker: f.target.clone(), + reason: "erase race".into(), + journal_erasure: None, + }, + &inventory, + ) + .unwrap(); + let prepared = f + .api + .config_store + .prepare_worker_removal_execution(TEST_WORKSPACE_ID, &plan.plan_id, &plan.input_fingerprint) + .unwrap(); + let service = WorkerRemovalService::new(&f.api); + subjektiv_journal::validate_removal_retention(&service, &f.target, &prepared).unwrap(); + let result = f + .api + .runtime + .execute_worker_retention(&f.target, prepared.runtime_request.clone()) + .unwrap(); + // The domain fence commits, but the original moved to the not-yet-published archive. + assert!( + f.api + .erase_subjektiv_journal_session(&f.attr.session_id, "erase-during-removal") + .is_err() + ); + assert!(f.store.journal_session_erased(&f.host).unwrap()); + let mut committed = false; + assert!( + subjektiv_journal::finish_worker_removal(&service, &f.target, &prepared, &result, || { + committed = true; + }) + .is_err() + ); + assert!(!committed); + f.assert_occupied(); + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + f.assert_erased_removal(); + assert!( + f.store + .journal_worker_was_released( + &f.attr.subject_id, + &f.target.runtime_id, + &f.target.worker_id + ) + .unwrap() + ); +} + +#[tokio::test] +async fn subject_removal_prior_archive_policy_receipt_remains_recoverable() { + let f = SubjectRemovalFixture::new().await; + f.seed_completed_release_for_archive_recovery(); + let configured = f + .api + .config_store + .worker_retention_policy(TEST_WORKSPACE_ID) + .unwrap() + .unwrap(); + let conn = rusqlite::Connection::open(&f.api.config.database_path).unwrap(); + conn.execute_batch("CREATE TRIGGER interrupt_legacy_removal BEFORE DELETE ON worker_registry BEGIN SELECT RAISE(ABORT,'injected metadata failure'); END;").unwrap(); + assert_ne!(f.remove().status, StatusCode::OK.as_u16()); + let prepared = f + .api + .config_store + .recover_worker_removal_execution(TEST_WORKSPACE_ID, &f.target) + .unwrap() + .unwrap(); + assert_eq!(prepared.plan.policy_id, configured.policy_id); + assert_eq!(prepared.plan.policy_digest, configured.policy_digest); + assert_eq!( + prepared.plan.state, + crate::retention::WorkerRemovalPlanState::Failed + ); + assert!(!session_root(f.dir.path(), &f.target).exists()); + conn.execute_batch("DROP TRIGGER interrupt_legacy_removal;") + .unwrap(); + drop(conn); + let f = f.reopen().await; + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + f.assert_removed_with_history(); + let recovered = f + .api + .config_store + .recover_worker_removal_execution(TEST_WORKSPACE_ID, &f.target) + .unwrap() + .unwrap(); + assert_eq!(prepared.plan.operation_id, recovered.plan.operation_id); + assert_eq!( + prepared.plan.input_fingerprint, + recovered.plan.input_fingerprint + ); + assert_eq!(prepared.plan.policy_digest, recovered.plan.policy_digest); +} + +async fn assert_subject_removal_recovers_unknown_snapshot(legacy: bool, completed: bool) { + let f = SubjectRemovalFixture::new().await; + let reserved = f.pending_journal_input(legacy); + let grant = reserved + .job + .request + .grants + .subjektiv_journal + .as_ref() + .unwrap(); + if completed { + // Fixture a committed domain checkpoint whose final Job response was lost. + // No Controller/model delivery is claimed by these direct domain writes. + let g = f + .store + .active_journal_generation(&f.host, &grant.journal_id) + .unwrap() + .unwrap(); + let ctx = JournalJobContext::validated(&f.store, &f.host, &g.id, g.job).unwrap(); + let record = f + .store + .execute_journal( + &ctx, + JournalOperation { + request_id: "saved-record".into(), + expected_state: g.initial_journal.state_token, + action: JournalAction::ConfirmUnchanged, + covered_ranges: grant.selected_ranges.clone(), + }, + ) + .unwrap(); + let position = f + .store + .execute_journal( + &ctx, + JournalOperation { + request_id: "saved-position".into(), + expected_state: record.state_token, + action: JournalAction::UpdatePosition { + current_position: "Saved before lost response".into(), + }, + covered_ranges: vec![], + }, + ) + .unwrap(); + f.store + .execute_journal( + &ctx, + JournalOperation { + request_id: "saved-checkpoint".into(), + expected_state: position.state_token, + action: JournalAction::Checkpoint { + record_operation_ids: vec!["saved-record".into()], + }, + covered_ranges: grant.selected_ranges.clone(), + }, + ) + .unwrap(); + subjektiv::journal::service::finish(&f.store, &ctx).unwrap(); + } + // With no bound execution Worker, the store records cleanup Completed. The + // unfinished case must remain Unknown (absence is not a Purge receipt). + f.api + .store + .finish_backend_job_attempt( + TEST_WORKSPACE_ID, + &reserved.job.request.job_id, + &reserved.attempt.attempt_id, + BackendJobAttemptState::Unknown, + "lost-response", + "fixture retained outcome", + &Utc::now().to_rfc3339(), + ) + .unwrap(); + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + f.assert_removed_with_history(); + let job = f + .api + .store + .get_backend_job(TEST_WORKSPACE_ID, &reserved.job.request.job_id) + .unwrap() + .unwrap(); + assert_eq!( + job.state, + if completed { + BackendJobState::Completed + } else { + BackendJobState::Unknown + } + ); + assert_eq!(job.current_attempt, 1); +} + +#[tokio::test] +async fn subject_removal_recovers_completed_unknown_without_recursive_subject_lock() { + assert_subject_removal_recovers_unknown_snapshot(false, true).await; +} + +#[tokio::test] +async fn subject_removal_recovers_legacy_completed_unknown_without_writer_authority() { + assert_subject_removal_recovers_unknown_snapshot(true, true).await; +} + +#[tokio::test] +async fn subject_removal_retains_legacy_unbound_unknown_without_blocking_deletion() { + assert_subject_removal_recovers_unknown_snapshot(true, false).await; +} + +#[tokio::test] +async fn subject_removal_prior_purge_receipt_recovers_without_inventing_erasure() { + assert_prior_purge_receipt(false).await; +} + +#[tokio::test] +async fn subject_removal_prior_pending_purge_receipt_recovers_with_stale_runtime_catalog() { + assert_prior_purge_receipt(true).await; +} + +async fn assert_prior_purge_receipt(stale_catalog: bool) { + let f = SubjectRemovalFixture::new().await; + f.seed_completed_release_for_archive_recovery(); + let configured = f + .api + .config_store + .worker_retention_policy(TEST_WORKSPACE_ID) + .unwrap() + .unwrap(); + f.api + .config_store + .update_worker_retention_policy( + TEST_WORKSPACE_ID, + &configured.policy_digest, + &crate::retention::WorkerRetentionPolicyUpdate { + policy_id: "old-ordinary-purge".into(), + session_disposition: worker_runtime::retention::SessionDisposition::Purge, + metadata_disposition: configured.metadata_disposition, + archive_retention: configured.archive_retention, + diagnostics_disposition: worker_runtime::retention::DiagnosticsDisposition::Purge, + diagnostics_retention_seconds: None, + }, + ) + .unwrap(); + let lease = f + .api + .config_store + .retained_worker_singleton_lease(TEST_WORKSPACE_ID, &f.target) + .unwrap() + .unwrap(); + let conn = rusqlite::Connection::open(&f.api.config.database_path).unwrap(); + // Model the pre-upgrade planner's policy choice only; restore the exact Subject + // reservation before execution. All subsequent retention/receipt paths are real. + conn.execute("UPDATE worker_create_reservations SET singleton_key=NULL,singleton_generation=NULL WHERE workspace_id=?1 AND worker_id=?2", + rusqlite::params![TEST_WORKSPACE_ID, f.target.worker_id]).unwrap(); + let inventory = f.api.runtime.worker_retention_inventory(&f.target).unwrap(); + let plan = f + .api + .config_store + .plan_worker_removal( + &crate::retention::WorkerRemovalPlanRequest { + workspace_id: TEST_WORKSPACE_ID.into(), + worker: f.target.clone(), + reason: "pre-upgrade ordinary Purge".into(), + journal_erasure: None, + }, + &inventory, + ) + .unwrap(); + conn.execute("UPDATE worker_create_reservations SET singleton_key=?3,singleton_generation=?4 WHERE workspace_id=?1 AND worker_id=?2", + rusqlite::params![TEST_WORKSPACE_ID, f.target.worker_id, lease.key, lease.generation]).unwrap(); + drop(conn); + assert_eq!( + plan.session_disposition, + worker_runtime::retention::SessionDisposition::Purge + ); + let prepared = f + .api + .config_store + .prepare_worker_removal_execution(TEST_WORKSPACE_ID, &plan.plan_id, &plan.input_fingerprint) + .unwrap(); + if stale_catalog { + // Fixture the Runtime delete-to-final-receipt crash window. Its live + // catalog deliberately remains stopped while the aggregate is gone. + let request = &prepared.runtime_request; + let pending = worker_runtime::retention::WorkerRetentionExecutionResult { + operation_id: request.operation_id.clone(), + input_fingerprint: request.input_fingerprint.clone(), + worker_id: request.worker_id, + session_disposition: request.session_disposition, + diagnostics_disposition: request.diagnostics_disposition, + archive: None, + source_removed: false, + diagnostics_retained: false, + }; + let receipt = f + .dir + .path() + .join(".test-embedded-runtime-store/retention/operations") + .join(format!("{}.json", plan.operation_id)); + std::fs::create_dir_all(receipt.parent().unwrap()).unwrap(); + std::fs::write( + receipt, + serde_json::to_vec(&serde_json::json!({ + "schema_version":1, "request":request, "result":pending, + })) + .unwrap(), + ) + .unwrap(); + std::fs::remove_dir_all(session_root(f.dir.path(), &f.target).parent().unwrap()).unwrap(); + assert_eq!(f.api.runtime.worker(&f.target).unwrap().state, "stopped"); + } else { + let result = f + .api + .runtime + .execute_worker_retention(&f.target, prepared.runtime_request.clone()) + .unwrap(); + assert!(result.source_removed); + } + f.api + .config_store + .fail_worker_removal( + TEST_WORKSPACE_ID, + &plan.operation_id, + &plan.input_fingerprint, + "metadata_commit_failed", + ) + .unwrap(); + let f = if stale_catalog { f } else { f.reopen().await }; + let response = f.remove(); + assert_eq!( + response.status, + StatusCode::OK.as_u16(), + "{}", + response.body + ); + assert!(!f.store.journal_session_erased(&f.host).unwrap()); + assert!( + f.api + .store + .get_worker_registry(TEST_WORKSPACE_ID, &f.target) + .unwrap() + .is_none() + ); + let recovered = f + .api + .config_store + .recover_worker_removal_execution(TEST_WORKSPACE_ID, &f.target) + .unwrap() + .unwrap(); + assert_eq!(recovered.plan.operation_id, plan.operation_id); + assert_eq!(recovered.plan.input_fingerprint, plan.input_fingerprint); + assert_eq!( + recovered.plan.state, + crate::retention::WorkerRemovalPlanState::Succeeded + ); +} + #[tokio::test] async fn disabled_subject_removal_does_not_admit_new_public_history_or_reserve_a_job() { let f = SubjectRemovalFixture::new().await; @@ -2300,7 +3165,7 @@ async fn disabled_subject_removal_does_not_admit_new_public_history_or_reserve_a let response = f.remove(); assert_eq!( response.status, - StatusCode::CONFLICT.as_u16(), + StatusCode::OK.as_u16(), "{}", response.body ); @@ -2309,7 +3174,7 @@ async fn disabled_subject_removal_does_not_admit_new_public_history_or_reserve_a .store .get_worker_registry(TEST_WORKSPACE_ID, &f.target) .unwrap() - .is_some() + .is_none() ); assert!(f.store.journal_for_session(&f.host).unwrap().is_none()); assert!( @@ -2319,6 +3184,7 @@ async fn disabled_subject_removal_does_not_admit_new_public_history_or_reserve_a .unwrap() .is_empty() ); + f.assert_removed_with_history(); } #[tokio::test] @@ -2374,7 +3240,7 @@ async fn disabled_subject_removal_retains_only_the_already_admitted_prefix() { let response = f.remove(); assert_eq!( response.status, - StatusCode::CONFLICT.as_u16(), + StatusCode::OK.as_u16(), "{}", response.body ); @@ -2383,7 +3249,7 @@ async fn disabled_subject_removal_retains_only_the_already_admitted_prefix() { .store .get_worker_registry(TEST_WORKSPACE_ID, &f.target) .unwrap() - .is_some() + .is_none() ); assert!( f.api @@ -2397,11 +3263,12 @@ async fn disabled_subject_removal_retains_only_the_already_admitted_prefix() { Some(id.as_str()) ); assert!(f.store.journal_retention_required(&f.host).unwrap()); - assert!(session_root(f.dir.path(), &f.target).exists()); + assert!(!session_root(f.dir.path(), &f.target).exists()); + f.assert_removed_with_history(); } #[tokio::test] -async fn attributed_unmaterialized_subject_still_requires_actual_idle_release() { +async fn attributed_unmaterialized_subject_removal_releases_without_restoring_controller() { let f = SubjectRemovalFixture::with_materialized_source(false).await; let retention = source_retention(f.dir.path()); let session = f.attr.session_id.parse().unwrap(); @@ -2426,19 +3293,20 @@ async fn attributed_unmaterialized_subject_still_requires_actual_idle_release() let response = f.remove(); assert_eq!( response.status, - StatusCode::CONFLICT.as_u16(), + StatusCode::OK.as_u16(), "{}", response.body ); - assert_eq!(retention.pins(session).unwrap().len(), 1); + assert!(retention.pins(session).unwrap().is_empty()); assert!( f.api .store .get_worker_registry(TEST_WORKSPACE_ID, &f.target) .unwrap() - .is_some() + .is_none() ); assert!(f.store.journal_for_session(&f.host).unwrap().is_none()); + f.assert_removed_with_history(); } #[tokio::test] @@ -2448,11 +3316,11 @@ async fn missing_materialized_subject_source_blocks_removal_without_marking_it_e let response = f.remove(); assert_eq!( response.status, - StatusCode::CONFLICT.as_u16(), + StatusCode::SERVICE_UNAVAILABLE.as_u16(), "{}", response.body ); - assert!(response.body.contains("journal_handoff_incomplete")); + assert!(response.body.contains("runtime_retention_failed")); assert!( f.api .store @@ -2468,6 +3336,7 @@ async fn missing_materialized_subject_source_blocks_removal_without_marking_it_e .unwrap() .is_empty() ); + f.assert_occupied(); } fn source_retention(root: &std::path::Path) -> session_store::retention::SessionRetentionStore { diff --git a/crates/workspace-server/src/server/tests/workdir_session_close_tests.rs b/crates/workspace-server/src/server/tests/workdir_session_close_tests.rs index 7221d6430..5bb808785 100644 --- a/crates/workspace-server/src/server/tests/workdir_session_close_tests.rs +++ b/crates/workspace-server/src/server/tests/workdir_session_close_tests.rs @@ -314,6 +314,7 @@ impl Fixture { &workdir_id, "account:close-test", "remove normally after session close", + &uuid::Uuid::now_v7().to_string(), ) }) .await diff --git a/crates/workspace-server/src/server/tests/worker_drive_tests.rs b/crates/workspace-server/src/server/tests/worker_drive_tests.rs index f92b6d172..c0001b321 100644 --- a/crates/workspace-server/src/server/tests/worker_drive_tests.rs +++ b/crates/workspace-server/src/server/tests/worker_drive_tests.rs @@ -652,6 +652,7 @@ async fn source_workdir_removal_keeps_published_drive_bytes_and_receipt() { &removal_id, "account:drive-test", "source no longer needed", + "drive-source-removal", ) }) .await diff --git a/crates/workspace-server/src/server_workspace_config_tests.rs b/crates/workspace-server/src/server_workspace_config_tests.rs index 1c932be1a..9bbc39641 100644 --- a/crates/workspace-server/src/server_workspace_config_tests.rs +++ b/crates/workspace-server/src/server_workspace_config_tests.rs @@ -819,6 +819,7 @@ mod workspace_config_integration { }, "owner".into(), WorkingDirectoryRemovalRequest { + operation_id: "unsupported-logical-cleanup".into(), reason: "unsupported logical cleanup".into() } ) diff --git a/crates/workspace-server/src/store.rs b/crates/workspace-server/src/store.rs index 289f6d411..ae6df14a0 100644 --- a/crates/workspace-server/src/store.rs +++ b/crates/workspace-server/src/store.rs @@ -3253,8 +3253,8 @@ impl SqliteWorkspaceStore { }, ) .optional()?; - // Subject occupancy survives removal until an explicit release - // proof. It must not resurrect the removed creation reservation: + // An unconfirmed legacy removal cannot release Subject occupancy. + // Completed removal or explicit release does; never resurrect a reservation: // Runtime callbacks require a live reservation or catalog member. if stored.as_ref().is_some_and(|row| row.6 == "removed") { return Err(Error::RepositoryConflict(format!( @@ -11443,49 +11443,7 @@ impl ControlPlaneStore for SqliteWorkspaceStore { record.workdir_id ))); } - let active_for_alias = tx - .query_row( - r#"SELECT workspace_id, runtime_id, worker_id, workdir_id, alias, capabilities, linked_at, unlinked_at, connection_id - FROM worker_workdir_links - WHERE workspace_id = ?1 AND runtime_id = ?2 AND worker_id = ?3 - AND alias = ?4 AND unlinked_at IS NULL"#, - params![ - record.workspace_id, - record.worker.runtime_id, - record.worker.worker_id, - record.alias, - ], - read_worker_workdir_link_record, - ) - .optional()?; - if let Some(mut active) = active_for_alias { - if active.workdir_id == record.workdir_id { - let capabilities = active.capabilities.intersection(record.capabilities); - if capabilities != active.capabilities { - tx.execute( - r#"UPDATE worker_workdir_links - SET capabilities = ?1 - WHERE workspace_id = ?2 AND runtime_id = ?3 AND worker_id = ?4 - AND workdir_id = ?5 AND alias = ?6 AND unlinked_at IS NULL"#, - params![ - encode_workdir_link_capabilities(capabilities)?, - record.workspace_id, - record.worker.runtime_id, - record.worker.worker_id, - record.workdir_id, - record.alias, - ], - )?; - active.capabilities = capabilities; - } - tx.commit()?; - return Ok(active); - } - return Err(Error::WorkdirAttachmentConflict(format!( - "Worker {}:{} attachment alias `{}` is already bound to Workdir {}", - record.worker.runtime_id, record.worker.worker_id, record.alias, active.workdir_id - ))); - } + // Ownership is keyed by Workdir, not by the requested local name. let active_for_workdir = tx .query_row( r#"SELECT workspace_id, runtime_id, worker_id, workdir_id, alias, capabilities, linked_at, unlinked_at, connection_id @@ -11495,10 +11453,45 @@ impl ControlPlaneStore for SqliteWorkspaceStore { read_worker_workdir_link_record, ) .optional()?; - if let Some(active) = active_for_workdir { + if let Some(mut active) = active_for_workdir { + if active.worker != record.worker { + return Err(Error::WorkdirOccupied { + workdir_id: record.workdir_id.clone(), + runtime_id: active.worker.runtime_id, + worker_id: active.worker.worker_id, + }); + } + let capabilities = active.capabilities.intersection(record.capabilities); + if capabilities != active.capabilities { + tx.execute( + r#"UPDATE worker_workdir_links SET capabilities = ?1 + WHERE workspace_id = ?2 AND runtime_id = ?3 AND worker_id = ?4 + AND workdir_id = ?5 AND alias = ?6 AND unlinked_at IS NULL"#, + params![ + encode_workdir_link_capabilities(capabilities)?, + active.workspace_id, active.worker.runtime_id, active.worker.worker_id, + active.workdir_id, active.alias, + ], + )?; + active.capabilities = capabilities; + } + tx.commit()?; + return Ok(active); + } + let active_for_alias = tx + .query_row( + r#"SELECT workspace_id, runtime_id, worker_id, workdir_id, alias, capabilities, linked_at, unlinked_at, connection_id + FROM worker_workdir_links + WHERE workspace_id = ?1 AND runtime_id = ?2 AND worker_id = ?3 + AND alias = ?4 AND unlinked_at IS NULL"#, + params![record.workspace_id, record.worker.runtime_id, record.worker.worker_id, record.alias], + read_worker_workdir_link_record, + ) + .optional()?; + if let Some(active) = active_for_alias { return Err(Error::WorkdirAttachmentConflict(format!( - "Workdir {} is already attached to Worker {}:{}", - record.workdir_id, active.worker.runtime_id, active.worker.worker_id + "Worker {}:{} attachment alias `{}` is already bound to Workdir {}", + record.worker.runtime_id, record.worker.worker_id, record.alias, active.workdir_id ))); } // Direct attachments (including ordinary recovery/reconnection) @@ -16746,7 +16739,7 @@ fn current_worker_singleton_owner( WHEN substr(owner.singleton_key,1,10)='subjektiv:' AND reservation.state='removed' \ AND NOT EXISTS(SELECT 1 FROM subject_worker_releases r WHERE r.workspace_id=owner.workspace_id AND r.worker_id=owner.worker_id AND r.runtime_id=owner.runtime_id AND r.singleton_key=owner.singleton_key AND r.singleton_generation=owner.generation AND r.completed=1) \ AND NOT EXISTS(SELECT 1 FROM subject_create_recoveries r WHERE r.workspace_id=owner.workspace_id AND r.worker_id=owner.worker_id AND r.runtime_id=owner.runtime_id AND r.singleton_key=owner.singleton_key AND r.singleton_generation=owner.generation AND r.create_fingerprint=reservation.create_fingerprint AND r.completed=1) \ - AND NOT EXISTS(SELECT 1 FROM worker_removal_operations r JOIN subject_deletion_workers d ON d.workspace_id=r.workspace_id AND d.runtime_id=r.runtime_id AND d.worker_id=r.worker_id WHERE r.workspace_id=owner.workspace_id AND r.runtime_id=owner.runtime_id AND r.worker_id=owner.worker_id AND r.state='succeeded' AND owner.singleton_key='subjektiv:'||d.subject_id AND (d.prior_removal_operation_id=r.operation_id OR (d.prior_removal_operation_id IS NULL AND d.is_job=0 AND r.session_disposition='archive' AND r.archive_retention_kind='forever' AND r.metadata_disposition='tombstone'))) \ + AND NOT EXISTS(SELECT 1 FROM worker_removal_operations r WHERE r.workspace_id=owner.workspace_id AND r.worker_id=owner.worker_id AND r.runtime_id=owner.runtime_id AND r.state='succeeded') \ AND NOT EXISTS(SELECT 1 FROM subject_deletion_workers d WHERE d.workspace_id=owner.workspace_id AND d.runtime_id=owner.runtime_id AND d.worker_id=owner.worker_id AND owner.singleton_key='subjektiv:'||d.subject_id AND d.originals_absent=1) \ THEN 'created' ELSE reservation.state END \ FROM worker_singleton_owners owner \ @@ -26065,6 +26058,24 @@ INSERT INTO worker_registry ( 2 ); + for alias in ["another-name", "docs"] { + let requested = WorkerWorkdirLinkRecord { + alias: alias.into(), + ..link.clone() + }; + assert_eq!( + store.attach_worker_workdir(&requested).unwrap(), + downgraded_link + ); + assert_eq!( + store + .list_worker_workdir_links("local-dev", &worker.worker) + .unwrap() + .len(), + 2 + ); + } + let second_worker = WorkerRegistryRecord { worker: RuntimeWorkerRef::new("embedded", "2"), display_name: "Browser 2".to_string(), @@ -26080,7 +26091,7 @@ INSERT INTO worker_registry ( }; assert!(matches!( store.attach_worker_workdir(&workdir_conflict), - Err(Error::WorkdirAttachmentConflict(_)) + Err(Error::WorkdirOccupied { .. }) )); assert!(matches!( store.detach_worker_workdir("local-dev", &worker.worker, Some("wrong-workdir"), "6",), diff --git a/crates/workspace-server/src/subject_deletion_store.rs b/crates/workspace-server/src/subject_deletion_store.rs index 275f81bb9..2bbdef024 100644 --- a/crates/workspace-server/src/subject_deletion_store.rs +++ b/crates/workspace-server/src/subject_deletion_store.rs @@ -1289,8 +1289,9 @@ mod tests { reservation(&s, &body, "body", Some("subjektiv:subject")); receipt(&s, &body, "succeeded", "purge", "purge"); let body_receipt = format!("receipt-{}-succeeded", body.worker_id); - // A different Subject's already-removed body remains occupied until its - // own deletion/release. Deleting a delegating Subject must not release it. + // Another Subject's completed removal already releases its occupancy. + // Deleting the delegating Subject must preserve that independent outcome + // and must not erase the other Subject's Job. let other_body = worker(33); reservation(&s, &other_body, "other-body", Some("subjektiv:another")); receipt(&s, &other_body, "succeeded", "purge", "purge"); @@ -1307,6 +1308,11 @@ mod tests { }).unwrap(); let archive_before = s.worker_session_archive("w", &jw, "session").unwrap(); assert!(archive_before.is_some()); + assert!( + s.current_worker_singleton_owner("w", "subjektiv:another") + .unwrap() + .is_none() + ); s.prepare_subject_deletion("w", "subject", "delete", std::slice::from_ref(&other_body)) .unwrap(); assert_eq!(saved_prior(&s, "subject", &jw), Some(job_receipt.clone())); @@ -1352,7 +1358,7 @@ mod tests { assert!( s.current_worker_singleton_owner("w", "subjektiv:another") .unwrap() - .is_some() + .is_none() ); s.with_conn(|c| { let old:(String,String,String)=c.query_row("SELECT state,session_disposition,metadata_disposition FROM worker_removal_operations WHERE operation_id=?1",[&job_receipt],|r|Ok((r.get(0)?,r.get(1)?,r.get(2)?)))?; diff --git a/crates/workspace-server/src/workdir_removal.rs b/crates/workspace-server/src/workdir_removal.rs index 46378b3a7..c7f3b62e4 100644 --- a/crates/workspace-server/src/workdir_removal.rs +++ b/crates/workspace-server/src/workdir_removal.rs @@ -1,24 +1,15 @@ -//! Persists durable deletion intent and attempts for Repository-backed Workdirs. +//! Records one Repository-backed Workdir removal request and its result. //! -//! Deletion intent fixes Workspace, Workdir, owning Runtime, Repository/materialization, actor, -//! and reason. Endpoints, host paths, and provider handles do not grant caller deletion authority. +//! Caller-generated request identities distinguish an exact resend from a new explicit request; +//! reason text is audit input, never a replay key. Failed and completed records are terminal, +//! including legacy failed records marked retryable. Retryability only advises a new explicit +//! request after resolving its cause; it is not continuing authority to delete. +//! +//! Only one unresolved request per Workdir is allowed. Attempts are claimed transactionally and +//! fenced by process/start marker and attempt identity. Recovery observes uncertain external +//! effects but never reissues deletion. Confirmed absence and the matching materialization permit +//! registry removal and the saved result to commit together. Unconfirmed effects remain pending. //! External Workdirs follow grant revocation, not this operation. -//! -//! Only one pending operation per Workdir is allowed; attempts are claimed transactionally. -//! Process IDs and startup markers are retained. Recovery reclaims the exact owner/attempt by -//! compare-and-set only after confirming the previous owner is gone; live or unobservable owners -//! are not displaced. In-process locks alone are insufficient. -//! -//! Server execution revalidates stored materialization and current attachments, reservations, -//! assignments, retention, and creation operations, then observes occupancy and cleanliness via -//! Runtime. Dirty resources remain; unknown observations or provider failures retain the registry -//! and record attention/retryability. Only confirmed deletion or exact working_directory_not_found -//! is deletion evidence; registry removal and completed/removed commit in the same transaction. -//! -//! Normal deletion, Workspace deletion, cleanup, and startup recovery use the same reserved Server -//! operation execution. Failed DB commits after provider deletion are reobserved through that -//! operation. Recovery relies on provider identity and results, not an exactly-once guarantee -//! across all interruption points. use chrono::Utc; use rusqlite::{Connection, OptionalExtension, TransactionBehavior, params}; @@ -118,10 +109,22 @@ pub fn workdir_materialization_fingerprint(record: &WorkdirRegistryRecord) -> St hex_sha256(&bytes) } +pub fn workdir_removal_request_id( + workspace_id: &str, + source_actor: &str, + request_id: &str, +) -> Result { + validate_bounded("operation id", request_id, 256)?; + let bytes = serde_json::to_vec(&(workspace_id, source_actor, request_id)) + .map_err(|error| Error::Store(error.to_string()))?; + Ok(format!("wdr_{}", &hex_sha256(&bytes)[..32])) +} + pub fn workdir_removal_intent( record: &WorkdirRegistryRecord, source_actor: &str, reason: &str, + request_id: &str, ) -> Result { validate_bounded("source actor", source_actor, MAX_ACTOR_BYTES)?; validate_bounded("reason", reason, MAX_REASON_BYTES)?; @@ -146,7 +149,7 @@ pub fn workdir_removal_intent( )); }; Ok(WorkdirRemovalIntent { - operation_id: format!("wdr_{}", &request_fingerprint[..32]), + operation_id: workdir_removal_request_id(&record.workspace_id, source_actor, request_id)?, request_fingerprint, workspace_id: record.workspace_id.clone(), working_directory_id: record.workdir_id.clone(), @@ -285,17 +288,10 @@ impl SqliteWorkspaceStore { let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; let operation = require_operation(&tx, workspace_id, operation_id, request_fingerprint)?; - if operation.state == WorkdirRemovalOperationState::Completed - && operation.disposition == Some(WorkdirRemovalDisposition::Removed) - { + if operation.state != WorkdirRemovalOperationState::Pending { tx.commit()?; return Ok(operation); } - if operation.state == WorkdirRemovalOperationState::Failed && !operation.retryable { - return Err(Error::InvalidInput(format!( - "Workdir removal operation `{operation_id}` is not retryable" - ))); - } if let Some((expected_owner, expected_attempt_id)) = recovery_expected { if operation.state != WorkdirRemovalOperationState::Pending || operation.attempt_owner != expected_owner @@ -473,7 +469,7 @@ impl SqliteWorkspaceStore { &operation.operation_id, &operation.request_fingerprint, )?; - if current.state == WorkdirRemovalOperationState::Completed { + if current.state != WorkdirRemovalOperationState::Pending { tx.commit()?; return Ok(current); } @@ -502,6 +498,39 @@ impl SqliteWorkspaceStore { }) } + /// Keep an uncertain external outcome for observation, not automatic re-execution. + pub fn record_workdir_removal_unknown( + &self, + operation: &WorkdirRemovalOperation, + ) -> Result { + self.with_conn_mut(|conn| { + let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; + let current = require_operation( + &tx, + &operation.workspace_id, + &operation.operation_id, + &operation.request_fingerprint, + )?; + if current.state != WorkdirRemovalOperationState::Pending { + tx.commit()?; + return Ok(current); + } + require_matching_attempt(operation, ¤t)?; + tx.execute( + "UPDATE workdir_removal_operations SET retryable=0, disposition='attention_required', failure_category='provider_cleanup_outcome_unknown', attempt_owner_pid=NULL, attempt_owner_start_marker=NULL, updated_at=?1 WHERE workspace_id=?2 AND operation_id=?3", + params![Utc::now().to_rfc3339(), operation.workspace_id, operation.operation_id], + )?; + let updated = require_operation( + &tx, + &operation.workspace_id, + &operation.operation_id, + &operation.request_fingerprint, + )?; + tx.commit()?; + Ok(updated) + }) + } + pub fn commit_workdir_removal_removed( &self, operation: &WorkdirRemovalOperation, @@ -531,7 +560,7 @@ impl SqliteWorkspaceStore { &operation.operation_id, &operation.request_fingerprint, )?; - if current.state == WorkdirRemovalOperationState::Completed { + if current.state != WorkdirRemovalOperationState::Pending { tx.commit()?; return Ok(current); } @@ -541,24 +570,29 @@ impl SqliteWorkspaceStore { &tx, &operation.workspace_id, &operation.working_directory_id, - )? - .ok_or_else(|| Error::RegistryInconsistency( - "Workdir registry row disappeared before durable removal commit".to_string(), - ))?; - require_operation_materialization(operation, ®istry)?; - require_no_removal_blockers( - &tx, - &operation.workspace_id, - &operation.working_directory_id, )?; - let deleted = tx.execute( - "DELETE FROM workdir_registry WHERE workspace_id=?1 AND workdir_id=?2", - params![operation.workspace_id, operation.working_directory_id], - )?; - if deleted != 1 { - return Err(Error::RegistryInconsistency( - "Workdir registry deletion did not remove exactly one row".to_string(), - )); + if let Some(registry) = registry { + require_operation_materialization(operation, ®istry)?; + require_no_removal_blockers( + &tx, + &operation.workspace_id, + &operation.working_directory_id, + )?; + tx.execute( + "DELETE FROM workdir_registry WHERE workspace_id=?1 AND workdir_id=?2", + params![operation.workspace_id, operation.working_directory_id], + )?; + } else { + let removed: bool = tx.query_row( + "SELECT EXISTS(SELECT 1 FROM workdir_removal_operations WHERE workspace_id=?1 AND workdir_id=?2 AND materialization_fingerprint=?3 AND state='completed' AND disposition='removed')", + params![operation.workspace_id, operation.working_directory_id, operation.materialization_fingerprint], + |row| row.get(0), + )?; + if !removed { + return Err(Error::RegistryInconsistency( + "Workdir registry disappeared without confirmed removal".to_string(), + )); + } } } let now = Utc::now().to_rfc3339(); @@ -585,20 +619,18 @@ impl SqliteWorkspaceStore { }) } - pub fn find_workdir_removal_operation_by_intent( + pub fn completed_workdir_removal( &self, workspace_id: &str, working_directory_id: &str, - source_actor: &str, - reason: &str, ) -> Result> { self.with_conn(|conn| { conn.query_row( &format!( - "{} WHERE workspace_id=?1 AND workdir_id=?2 AND source_actor=?3 AND reason=?4 ORDER BY created_at DESC, operation_id DESC LIMIT 1", + "{} WHERE workspace_id=?1 AND workdir_id=?2 AND state='completed' AND disposition='removed' ORDER BY completed_at DESC, operation_id DESC LIMIT 1", operation_select_sql() ), - params![workspace_id, working_directory_id, source_actor, reason], + params![workspace_id, working_directory_id], read_operation, ) .optional() @@ -614,7 +646,7 @@ impl SqliteWorkspaceStore { self.with_conn(|conn| { let mut statement = conn.prepare( &format!( - "{} WHERE workspace_id=?1 AND (state='pending' OR (state='failed' AND retryable=1)) ORDER BY updated_at ASC, operation_id ASC LIMIT ?2", + "{} WHERE workspace_id=?1 AND state='pending' ORDER BY updated_at ASC, operation_id ASC LIMIT ?2", operation_select_sql() ), )?; @@ -1007,8 +1039,13 @@ mod tests { #[tokio::test] async fn exact_replay_reuses_operation_and_conflicting_intent_fails() { let (store, workdir) = seeded_store().await; - let intent = - workdir_removal_intent(&workdir, "worker:W-1", "remove stale Workdir").unwrap(); + let intent = workdir_removal_intent( + &workdir, + "worker:W-1", + "remove stale Workdir", + "test-removal-request", + ) + .unwrap(); let first = store.reserve_workdir_removal_operation(&intent).unwrap(); let replay = store.reserve_workdir_removal_operation(&intent).unwrap(); assert_eq!(replay, first); @@ -1023,10 +1060,15 @@ mod tests { } #[tokio::test] - async fn failed_attempt_is_retryable_and_completed_retry_replays() { + async fn failed_request_is_terminal_and_new_request_has_independent_result() { let (store, workdir) = seeded_store().await; - let intent = - workdir_removal_intent(&workdir, "workspace-api", "remove clean Workdir").unwrap(); + let intent = workdir_removal_intent( + &workdir, + "workspace-api", + "remove clean Workdir", + "test-removal-request", + ) + .unwrap(); let reserved = store.reserve_workdir_removal_operation(&intent).unwrap(); let first = store .begin_workdir_removal_attempt( @@ -1049,7 +1091,32 @@ mod tests { attempt_owner(), ) .unwrap(); - assert_ne!(retry.attempt_id, first.attempt_id); + assert_eq!(retry, failed); + assert!( + store + .recoverable_workdir_removal_operations("workspace-a", 100) + .unwrap() + .is_empty() + ); + let next_intent = workdir_removal_intent( + &workdir, + "workspace-api", + "remove clean Workdir", + "new-request", + ) + .unwrap(); + let next = store + .reserve_workdir_removal_operation(&next_intent) + .unwrap(); + let retry = store + .begin_workdir_removal_attempt( + &next.workspace_id, + &next.operation_id, + &next.request_fingerprint, + attempt_owner(), + ) + .unwrap(); + assert_ne!(retry.operation_id, failed.operation_id); let completed = store.commit_workdir_removal_removed(&retry).unwrap(); assert_eq!( completed.disposition, @@ -1075,7 +1142,13 @@ mod tests { #[tokio::test] async fn recovery_and_completion_are_fenced_by_attempt_identity() { let (store, workdir) = seeded_store().await; - let intent = workdir_removal_intent(&workdir, "workspace-api", "remove Workdir").unwrap(); + let intent = workdir_removal_intent( + &workdir, + "workspace-api", + "remove Workdir", + "test-removal-request", + ) + .unwrap(); let reserved = store.reserve_workdir_removal_operation(&intent).unwrap(); let first = store .begin_workdir_removal_attempt( @@ -1130,8 +1203,13 @@ mod tests { use std::sync::{Arc, Barrier}; let (store, workdir) = seeded_store().await; - let intent = - workdir_removal_intent(&workdir, "workspace-api", "remove clean Workdir").unwrap(); + let intent = workdir_removal_intent( + &workdir, + "workspace-api", + "remove clean Workdir", + "test-removal-request", + ) + .unwrap(); let operation = store.reserve_workdir_removal_operation(&intent).unwrap(); let barrier = Arc::new(Barrier::new(3)); let provider_calls = Arc::new(AtomicUsize::new(0)); @@ -1218,8 +1296,13 @@ mod tests { "2", ) .unwrap(); - let intent = - workdir_removal_intent(&workdir, "workspace-api", "remove clean Workdir").unwrap(); + let intent = workdir_removal_intent( + &workdir, + "workspace-api", + "remove clean Workdir", + "test-removal-request", + ) + .unwrap(); store.reserve_workdir_removal_operation(&intent).unwrap(); let error = store @@ -1236,8 +1319,13 @@ mod tests { #[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 intent = workdir_removal_intent( + &workdir, + "workspace-api", + "remove clean Workdir", + "test-removal-request", + ) + .unwrap(); let pending = store.reserve_workdir_removal_operation(&intent).unwrap(); let error = store @@ -1251,11 +1339,22 @@ mod tests { store .reserve_worker_workdir_attachment("workspace-a", "workdir-a", "reservation-b", "3") .unwrap(); + let next_intent = workdir_removal_intent( + &workdir, + "workspace-api", + "remove clean Workdir", + "new-request", + ) + .unwrap(); + let next = store + .reserve_workdir_removal_operation(&next_intent) + .unwrap(); + assert_ne!(next.operation_id, failed.operation_id); let retry = store .begin_workdir_removal_attempt( - &failed.workspace_id, - &failed.operation_id, - &failed.request_fingerprint, + &next.workspace_id, + &next.operation_id, + &next.request_fingerprint, attempt_owner(), ) .unwrap(); @@ -1324,7 +1423,13 @@ mod tests { Ok(()) }) .unwrap(); - let intent = workdir_removal_intent(&workdir, "workspace-api", "remove Workdir").unwrap(); + let intent = workdir_removal_intent( + &workdir, + "workspace-api", + "remove Workdir", + "test-removal-request", + ) + .unwrap(); let operation = store.reserve_workdir_removal_operation(&intent).unwrap(); let categories = |guards: Vec| { guards @@ -1447,8 +1552,13 @@ mod tests { #[tokio::test] async fn retained_completion_keeps_registry_and_is_auditable() { let (store, workdir) = seeded_store().await; - let intent = - workdir_removal_intent(&workdir, "workspace-api", "inspect dirty Workdir").unwrap(); + let intent = workdir_removal_intent( + &workdir, + "workspace-api", + "inspect dirty Workdir", + "test-removal-request", + ) + .unwrap(); let operation = store.reserve_workdir_removal_operation(&intent).unwrap(); let retained = store .complete_workdir_removal_retained( @@ -1477,10 +1587,12 @@ mod tests { attempt_owner(), ) .unwrap(); - assert_eq!(retry.state, WorkdirRemovalOperationState::Pending); - assert!(retry.attempt_id.is_some()); - assert_eq!(retry.disposition, None); - assert_eq!(retry.failure_category, None); - assert!(retry.retryable); + assert_eq!(retry, retained); + assert!( + store + .recoverable_workdir_removal_operations("workspace-a", 100) + .unwrap() + .is_empty() + ); } } diff --git a/openapi/server-api.json b/openapi/server-api.json index ce8bb687a..f1139939d 100644 --- a/openapi/server-api.json +++ b/openapi/server-api.json @@ -20698,11 +20698,16 @@ "WorkingDirectoryRemovalRequest": { "additionalProperties": false, "properties": { + "operation_id": { + "description": "Stable for an exact resend; a new explicit request uses a new identity.", + "type": "string" + }, "reason": { "type": "string" } }, "required": [ + "operation_id", "reason" ], "type": "object" @@ -22715,7 +22720,7 @@ "info": { "title": "Yoi Server API", "version": "0.1.0", - "x-yoi-source-digest": "sha256:8cc3c45d082e52b1ea9a014721008aeb4c0699b987729d362c0c14a6f485df10" + "x-yoi-source-digest": "sha256:2ac605f7ea70d0c725f9993a5c59c4a8c4266cf69184902db7b3ea7fd5932224" }, "openapi": "3.1.0", "paths": { diff --git a/web/workspace/src/lib/generated/companion-api.ts b/web/workspace/src/lib/generated/companion-api.ts index 4469f53ce..ff89b5de4 100644 --- a/web/workspace/src/lib/generated/companion-api.ts +++ b/web/workspace/src/lib/generated/companion-api.ts @@ -1,6 +1,6 @@ // @generated by yoi-companion-openapi-typescript@1. Do not edit. // Input: openapi/server-api.json -// Input contract: 0.1.0 (sha256:8cc3c45d082e52b1ea9a014721008aeb4c0699b987729d362c0c14a6f485df10) +// Input contract: 0.1.0 (sha256:2ac605f7ea70d0c725f9993a5c59c4a8c4266cf69184902db7b3ea7fd5932224) // Options: companion-operation-schema-closure,openapi-3.1,strict-integer-ranges,sorted-output // Output: web/workspace/src/lib/generated/companion-api.ts // Regenerate/check: cargo run -q -p server-api --example generate_companion_api_types -- --check diff --git a/web/workspace/src/lib/generated/repository-api.ts b/web/workspace/src/lib/generated/repository-api.ts index 8136cac42..7a4a4bee8 100644 --- a/web/workspace/src/lib/generated/repository-api.ts +++ b/web/workspace/src/lib/generated/repository-api.ts @@ -1,6 +1,6 @@ // @generated by yoi-repository-openapi-typescript@1. Do not edit. // Input: openapi/server-api.json -// Input contract: 0.1.0 (sha256:8cc3c45d082e52b1ea9a014721008aeb4c0699b987729d362c0c14a6f485df10) +// Input contract: 0.1.0 (sha256:2ac605f7ea70d0c725f9993a5c59c4a8c4266cf69184902db7b3ea7fd5932224) // Options: repository-operation-schema-closure,openapi-3.1,format:uint64=safe-number,strict-safe-integers,sorted-output // Output: web/workspace/src/lib/generated/repository-api.ts // Regenerate/check: cargo run -q -p server-api --example generate_repository_openapi_types -- --check diff --git a/web/workspace/src/lib/generated/runtime-api.ts b/web/workspace/src/lib/generated/runtime-api.ts index 81567aa0b..25cf9167f 100644 --- a/web/workspace/src/lib/generated/runtime-api.ts +++ b/web/workspace/src/lib/generated/runtime-api.ts @@ -1,6 +1,6 @@ // @generated by yoi-runtime-openapi-typescript@1. Do not edit. // Input: openapi/server-api.json -// Input contract: 0.1.0 (sha256:8cc3c45d082e52b1ea9a014721008aeb4c0699b987729d362c0c14a6f485df10) +// Input contract: 0.1.0 (sha256:2ac605f7ea70d0c725f9993a5c59c4a8c4266cf69184902db7b3ea7fd5932224) // Options: runtime-operation-schema-closure,openapi-3.1,strict-integer-ranges,sorted-output // Output: web/workspace/src/lib/generated/runtime-api.ts // Regenerate/check: cargo run -q -p server-api --example generate_runtime_api_types -- --check diff --git a/web/workspace/src/lib/generated/ticket-api.ts b/web/workspace/src/lib/generated/ticket-api.ts index 1dca671b6..119fde58e 100644 --- a/web/workspace/src/lib/generated/ticket-api.ts +++ b/web/workspace/src/lib/generated/ticket-api.ts @@ -1,6 +1,6 @@ // @generated by yoi-ticket-browser-openapi-typescript@1. Do not edit. // Input: openapi/server-api.json -// Input contract: 0.1.0 (sha256:8cc3c45d082e52b1ea9a014721008aeb4c0699b987729d362c0c14a6f485df10) +// Input contract: 0.1.0 (sha256:2ac605f7ea70d0c725f9993a5c59c4a8c4266cf69184902db7b3ea7fd5932224) // Options: ticket-objective-merge-request-browser-operation-schema-closure,openapi-3.1,strict-integer-ranges,sorted-output // Output: web/workspace/src/lib/generated/ticket-api.ts // Regenerate/check: cargo run -q -p server-api --example generate_ticket_api_types -- --check diff --git a/web/workspace/src/lib/generated/workdir-api.ts b/web/workspace/src/lib/generated/workdir-api.ts index 87851a08c..59b59e534 100644 --- a/web/workspace/src/lib/generated/workdir-api.ts +++ b/web/workspace/src/lib/generated/workdir-api.ts @@ -1,6 +1,6 @@ // @generated by yoi-workdir-openapi-typescript@1. Do not edit. // Input: openapi/server-api.json -// Input contract: 0.1.0 (sha256:1053b09bb77ff0fc09fec2d3f96af04b76974c5ea52870f805381302e1b29fe4) +// Input contract: 0.1.0 (sha256:2ac605f7ea70d0c725f9993a5c59c4a8c4266cf69184902db7b3ea7fd5932224) // Options: workdir-operation-schema-closure,openapi-3.1,strict-integer-ranges,sorted-output // Output: web/workspace/src/lib/generated/workdir-api.ts // Regenerate/check: cargo run -q -p server-api --example generate_workdir_api_types -- --check @@ -33,7 +33,7 @@ export type WorkingDirectoryOccupancy = { display_name: string; linked_at: strin export type WorkingDirectoryRemovalDisposition = "removed" | "retained" | "attention_required"; -export type WorkingDirectoryRemovalRequest = { reason: string; }; +export type WorkingDirectoryRemovalRequest = { operation_id: string; reason: string; }; export type WorkingDirectoryRemovalResponse = { disposition: WorkingDirectoryRemovalDisposition; failure_category?: string | null; retryable: boolean; working_directory_id: string; }; diff --git a/web/workspace/src/lib/generated/worker-launch-api.ts b/web/workspace/src/lib/generated/worker-launch-api.ts index d78c4e037..9ff1cfd42 100644 --- a/web/workspace/src/lib/generated/worker-launch-api.ts +++ b/web/workspace/src/lib/generated/worker-launch-api.ts @@ -1,6 +1,6 @@ // @generated by yoi-worker-openapi-typescript@1. Do not edit. // Input: openapi/server-api.json -// Input contract: 0.1.0 (sha256:8cc3c45d082e52b1ea9a014721008aeb4c0699b987729d362c0c14a6f485df10) +// Input contract: 0.1.0 (sha256:2ac605f7ea70d0c725f9993a5c59c4a8c4266cf69184902db7b3ea7fd5932224) // Options: worker-operation-schema-closure,openapi-3.1,strict-integer-ranges,sorted-output // Output: web/workspace/src/lib/generated/worker-launch-api.ts // Regenerate/check: cargo run -q -p server-api --example generate_worker_launch_api_types -- --check diff --git a/web/workspace/src/lib/workspace/settings/workdir-removal.ts b/web/workspace/src/lib/workspace/settings/workdir-removal.ts index 4c7194e86..faace48b6 100644 --- a/web/workspace/src/lib/workspace/settings/workdir-removal.ts +++ b/web/workspace/src/lib/workspace/settings/workdir-removal.ts @@ -30,7 +30,9 @@ const causes: Readonly> = { provider_observation_unknown: "The provider could not confirm the remaining Workdir. Restore provider access so the next removal can recheck its current contents.", provider_cleanup_outcome_unknown: - "The provider has not confirmed deletion. Restore provider access and retry the same removal request to recheck the remaining target. The Workdir stays listed until deletion is confirmed.", + "The provider has not confirmed deletion. Restore provider access and reconcile the same request. Recovery only observes the target; it never repeats deletion.", + request_interrupted: + "The request was interrupted before execution. Submit a new request if removal is still wanted.", authority_invalid: "The removal authority could not be verified. Ask the Workspace administrator to verify the Workdir target and owning Runtime before requesting removal again.", provider_identity_changed: @@ -125,11 +127,39 @@ async function removalBody(response: Response): Promise { } } +// 要求IDは結果不明時だけ保持する。確定した失敗の次の明示要求は別IDとなる。 +export class WorkdirRemovalRequests { + private readonly unresolved = new Map(); + + async remove( + fetchFn: typeof fetch, + workspaceId: string, + runtimeId: string, + workdirId: string, + ): Promise { + const key = JSON.stringify([workspaceId, runtimeId, workdirId]); + const operationId = this.unresolved.get(key) ?? crypto.randomUUID(); + this.unresolved.set(key, operationId); + const result = await removeWorkdir( + fetchFn, + workspaceId, + runtimeId, + workdirId, + operationId, + ); + if (result.failure_category !== "provider_cleanup_outcome_unknown") { + this.unresolved.delete(key); + } + return result; + } +} + export async function removeWorkdir( fetchFn: typeof fetch, workspaceId: string, runtimeId: string, workdirId: string, + operationId: string, ): Promise { const response = await fetchFn( workspaceApiPath( @@ -141,8 +171,10 @@ export async function removeWorkdir( { method: "DELETE", headers: { "content-type": "application/json" }, - // An ordinary repeated request uses the existing durable Backend removal authority. - body: JSON.stringify({ reason: "User requested Workdir deletion" }), + body: JSON.stringify({ + operation_id: operationId, + reason: "User requested Workdir deletion", + }), }, ); if (!response.ok) { diff --git a/web/workspace/src/routes/w/[workspaceId]/settings/runtimes/[runtimeId]/workdirs/PageContent.svelte b/web/workspace/src/routes/w/[workspaceId]/settings/runtimes/[runtimeId]/workdirs/PageContent.svelte index eaf74d5f0..07c10039e 100644 --- a/web/workspace/src/routes/w/[workspaceId]/settings/runtimes/[runtimeId]/workdirs/PageContent.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/settings/runtimes/[runtimeId]/workdirs/PageContent.svelte @@ -2,7 +2,7 @@ import { loadJson, workspaceApiPath } from '#lib/workspace/api/http.ts'; import { parseRuntimeCleanupPlan } from '#lib/workspace/api/runtime-workers.ts'; import { parseWorkingDirectoryListResponse } from '#lib/workspace/api/workdirs.ts'; - import { canRemoveWorkdir, removalCause, removalGuard, removeWorkdir, WorkdirRemovalError } from '#lib/workspace/settings/workdir-removal.ts'; + import { canRemoveWorkdir, removalCause, removalGuard, WorkdirRemovalRequests, WorkdirRemovalError } from '#lib/workspace/settings/workdir-removal.ts'; import { formatCurrentWorkdirCheckout } from '#lib/workspace/settings/workdir-checkout.ts'; import type { CleanupWorkdirCandidate, RuntimeCleanupPlanResponse, WorkingDirectorySummary } from '#lib/workspace/sidebar/types.ts'; import type { PageData as RouteData } from './$types'; @@ -10,6 +10,7 @@ type PageProps = { data: ResolvedPageData }; let { data }: PageProps = $props(); + const removalRequests = new WorkdirRemovalRequests(); let cleanupBusyTarget = $state(null); let refreshing = $state(false); let refreshError = $state(null); @@ -72,7 +73,7 @@ cleanupBusyTarget = workdir.working_directory_id; feedback = null; try { - const result = await removeWorkdir(fetch, workspaceId, runtimeId, workdir.working_directory_id); + const result = await removalRequests.remove(fetch, workspaceId, runtimeId, workdir.working_directory_id); if (workspaceId !== data.workspaceId || runtimeId !== data.runtimeId) return; feedback = { id: workdir.working_directory_id, diff --git a/web/workspace/tests/workdir-removal.test.ts b/web/workspace/tests/workdir-removal.test.ts index 97dfe4aa0..dd3aa422c 100644 --- a/web/workspace/tests/workdir-removal.test.ts +++ b/web/workspace/tests/workdir-removal.test.ts @@ -6,6 +6,7 @@ import { canRemoveWorkdir, removalCause, removeWorkdir, + WorkdirRemovalRequests, } from "../src/lib/workspace/settings/workdir-removal.ts"; import type { CleanupWorkdirCandidate, @@ -57,6 +58,7 @@ Deno.test("removal wire boundary rejects malformed results and mismatched identi "workspace", "runtime", "dir", + "request-1", ); } catch { rejected = true; @@ -72,6 +74,7 @@ Deno.test("removal response size is bounded before decoding provider content", a "workspace", "runtime", "dir", + "request-1", ); } catch { rejected = true; @@ -179,7 +182,7 @@ Deno.test("pending unknown cleanup permits ordinary removal while current dirty ); }); -Deno.test("ordinary retries send the same scoped DELETE body and expose only allowlisted HTTP causes", async () => { +Deno.test("new requests after confirmed failure get distinct IDs and expose only allowlisted HTTP causes", async () => { const requests: { path: string; body: string; method: string }[] = []; const fetchFn = (async (path: RequestInfo | URL, init?: RequestInit) => { requests.push({ @@ -194,8 +197,9 @@ Deno.test("ordinary retries send the same scoped DELETE body and expose only all failure_category: "mount_present", }); }) as typeof fetch; + const removalRequests = new WorkdirRemovalRequests(); for (let i = 0; i < 2; i++) { - const result = await removeWorkdir( + const result = await removalRequests.remove( fetchFn, "workspace/x", "runtime/x", @@ -204,8 +208,9 @@ Deno.test("ordinary retries send the same scoped DELETE body and expose only all assert(result.retryable && result.failure_category === "mount_present"); } assert( - JSON.stringify(requests[0]) === JSON.stringify(requests[1]), - "retry changed ordinary deletion request", + JSON.parse(requests[0].body).operation_id !== + JSON.parse(requests[1].body).operation_id, + "new explicit request reused a failed request identity", ); assert( requests[0].path === @@ -213,8 +218,9 @@ Deno.test("ordinary retries send the same scoped DELETE body and expose only all ); assert( requests[0].method === "DELETE" && - requests[0].body === - JSON.stringify({ reason: "User requested Workdir deletion" }), + JSON.parse(requests[0].body).reason === + "User requested Workdir deletion" && + typeof JSON.parse(requests[0].body).operation_id === "string", ); for ( const [status, category] of [ @@ -236,6 +242,7 @@ Deno.test("ordinary retries send the same scoped DELETE body and expose only all "workspace", "runtime", "dir", + "request-1", ); } catch (error) { message = error instanceof Error ? error.message : String(error); @@ -246,3 +253,34 @@ Deno.test("ordinary retries send the same scoped DELETE body and expose only all ); } }); + +Deno.test("unknown removal outcomes keep the same identity and body until a terminal result", async () => { + for (const transportLoss of [false, true]) { + const calls: string[] = []; + const requester = new WorkdirRemovalRequests(); + const fetchFn = (async (_path: RequestInfo | URL, init?: RequestInit) => { + calls.push(String(init?.body)); + if (calls.length === 1 && transportLoss) { + throw new Error("connection lost"); + } + return Response.json({ + working_directory_id: "dir", + disposition: "attention_required", + retryable: calls.length !== 1, + failure_category: calls.length === 1 + ? "provider_cleanup_outcome_unknown" + : "resource_busy", + }); + }) as typeof fetch; + await requester.remove(fetchFn, "workspace", "runtime", "dir").catch(() => + undefined + ); + await requester.remove(fetchFn, "workspace", "runtime", "dir"); + await requester.remove(fetchFn, "workspace", "runtime", "dir"); + assert(calls[0] === calls[1], "unknown outcome changed request identity"); + assert( + calls[1] !== calls[2], + "confirmed failure kept old request identity", + ); + } +});