From 6810f51f460ac0027628156bab5945423c148c3e Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 05:53:58 +0900 Subject: [PATCH 1/8] fix: retain safe Drive and config adapter failure diagnostics --- crates/worker/src/feature/builtin.rs | 1 + .../src/feature/builtin/backend_diagnostic.rs | 195 ++++++ .../src/feature/builtin/drive/backend.rs | 237 +++++-- .../feature/builtin/drive/backend_tests.rs | 644 ++++++++++++++++++ .../builtin/workspace_config/backend.rs | 117 +++- .../builtin/workspace_config/backend_tests.rs | 580 ++++++++++++++++ .../builtin/workspace_config/content.rs | 74 +- .../workspace_config/production_tests.rs | 161 ++++- .../feature/builtin/workspace_config/wip.rs | 9 +- 9 files changed, 1947 insertions(+), 71 deletions(-) create mode 100644 crates/worker/src/feature/builtin/backend_diagnostic.rs create mode 100644 crates/worker/src/feature/builtin/drive/backend_tests.rs create mode 100644 crates/worker/src/feature/builtin/workspace_config/backend_tests.rs diff --git a/crates/worker/src/feature/builtin.rs b/crates/worker/src/feature/builtin.rs index dac21d1d5..26b4d9a4e 100644 --- a/crates/worker/src/feature/builtin.rs +++ b/crates/worker/src/feature/builtin.rs @@ -4,6 +4,7 @@ //! same descriptor-approved registry path used by feature modules. They are not //! an external plugin-loading surface. +mod backend_diagnostic; pub mod backend_job_result; pub(crate) mod bash; diff --git a/crates/worker/src/feature/builtin/backend_diagnostic.rs b/crates/worker/src/feature/builtin/backend_diagnostic.rs new file mode 100644 index 000000000..ac7d7db44 --- /dev/null +++ b/crates/worker/src/feature/builtin/backend_diagnostic.rs @@ -0,0 +1,195 @@ +//! Internal adapter diagnostics are independent of public failure classifications. +//! +//! Only closed vocabularies, numeric status and opaque correlation tokens cross this boundary. +//! In particular, a transport error's Display/Debug and server-controlled text are never recorded. +use sha2::{Digest, Sha256}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Adapter { + Drive, + WorkspaceConfig, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Stage { + Request, + Response, + Completion, + Reconcile, + Attach, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Reason { + Deadline, + TaskJoin, + TransportUnavailable, + InvalidPath, + Communication, + Decode, + RequestSize, + ResponseSize, + ResponseLength, + Identity, + InvalidMetadata, + BackendRejected, + AccessCeiling, + AttachConversion, + BackendUnknown, + UnrecognizedResponse, + NotCommittedSnapshot, + IncompleteReceipt, + Reconciled, +} + +/// One logical call, including its original-request reconciliation. Request identities are hashed +/// rather than logged verbatim: callers and responses need not contain safe log text. +#[derive(Clone)] +pub(super) struct Diagnostic { + adapter: Adapter, + operation: &'static str, + mutation: bool, + correlation: String, + dispatch: tracing::Dispatch, + stage_override: Option, +} + +impl Diagnostic { + pub(super) fn new( + adapter: Adapter, + operation: &'static str, + mutation: bool, + request_id: Option<&str>, + ) -> Self { + Self { + adapter, + operation, + mutation, + correlation: request_id + .map(|id| { + let hash: String = Sha256::digest(id.as_bytes()) + .iter() + .map(|b| format!("{b:02x}")) + .collect(); + format!("request:{hash}") + }) + .unwrap_or_else(|| uuid::Uuid::now_v7().to_string()), + dispatch: tracing::dispatcher::get_default(Clone::clone), + stage_override: None, + } + } + + pub(super) fn at_stage(&self, stage: Stage) -> Self { + let mut diagnostic = self.clone(); + diagnostic.stage_override = Some(stage); + diagnostic + } + + pub(super) fn record(&self, stage: Stage, reason: Reason, status: Option) { + let stage = self.stage_override.unwrap_or(stage); + tracing::dispatcher::with_default(&self.dispatch, || { + tracing::warn!( + target: "worker::backend_diagnostic", + adapter = ?self.adapter, + operation = self.operation, + mutation = self.mutation, + correlation = %self.correlation, + stage = ?stage, + reason = ?reason, + status, + "Workspace adapter diagnostic" + ); + }); + } + + pub(super) fn transport(&self, error: &crate::worker::WorkspaceClientError) { + use crate::worker::WorkspaceClientError; + self.record( + Stage::Request, + match error { + WorkspaceClientError::Unavailable(_) => Reason::TransportUnavailable, + WorkspaceClientError::InvalidPath(_) => Reason::InvalidPath, + WorkspaceClientError::Request(_) => Reason::Communication, + }, + None, + ); + } +} + +#[cfg(test)] +pub(super) mod tests { + use super::{Adapter, Diagnostic, Reason, Stage}; + use std::sync::{Arc, Mutex}; + use tracing::{Event, Metadata, Subscriber, field::Visit, span}; + + #[test] + fn unsafe_request_identity_is_correlated_without_logging_its_value() { + let capture = Capture::default(); + tracing::dispatcher::with_default(&capture.dispatch(), || { + for id in [ + "secret /host/path\ncredential", + "secret /host/path\ncredential", + "different", + ] { + Diagnostic::new(Adapter::Drive, "mutate", true, Some(id)).record( + Stage::Request, + Reason::Communication, + None, + ); + } + }); + let text = capture.text(); + for secret in ["secret", "/host/path", "credential", "different"] { + assert!(!text.contains(secret)); + } + let correlations: Vec<_> = text + .lines() + .map(|line| { + line.split("correlation=") + .nth(1) + .unwrap() + .split(';') + .next() + .unwrap() + }) + .collect(); + assert_eq!(correlations.len(), 3); + assert_eq!(correlations[0], correlations[1]); + assert_ne!(correlations[0], correlations[2]); + } + + #[derive(Clone, Default)] + pub(crate) struct Capture(pub(crate) Arc>>); + impl Capture { + pub(crate) fn dispatch(&self) -> tracing::Dispatch { + tracing::Dispatch::new(self.clone()) + } + pub(crate) fn text(&self) -> String { + self.0.lock().unwrap().join("\n") + } + } + struct Fields(String); + impl Visit for Fields { + fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) { + use std::fmt::Write; + write!(&mut self.0, "{}={value:?};", field.name()).unwrap(); + } + } + impl Subscriber for Capture { + fn enabled(&self, metadata: &Metadata<'_>) -> bool { + metadata.target() == "worker::backend_diagnostic" + } + fn new_span(&self, _: &span::Attributes<'_>) -> span::Id { + span::Id::from_u64(1) + } + fn record(&self, _: &span::Id, _: &span::Record<'_>) {} + fn record_follows_from(&self, _: &span::Id, _: &span::Id) {} + fn event(&self, event: &Event<'_>) { + let mut fields = Fields(String::new()); + event.record(&mut fields); + self.0.lock().unwrap().push(fields.0); + } + fn enter(&self, _: &span::Id) {} + fn exit(&self, _: &span::Id) {} + } +} diff --git a/crates/worker/src/feature/builtin/drive/backend.rs b/crates/worker/src/feature/builtin/drive/backend.rs index 4c799d1ef..5730dcf0a 100644 --- a/crates/worker/src/feature/builtin/drive/backend.rs +++ b/crates/worker/src/feature/builtin/drive/backend.rs @@ -11,6 +11,8 @@ //! cases retain an unknown outcome with the original request ID. Never blindly resend with a new ID. use std::{sync::Arc, time::Duration}; +use super::super::backend_diagnostic::{Adapter, Diagnostic, Reason, Stage}; + use crate::worker::{ WorkspaceBinaryRequest, WorkspaceClient, WorkspaceRequest, WorkspaceRequestMethod, }; @@ -23,6 +25,7 @@ const RESPONSE_LIMIT: usize = 2 * 1024 * 1024; #[derive(Clone)] pub struct DriveBackend { client: Arc, + deadline: Duration, } #[derive(Debug, Clone, thiserror::Error)] @@ -49,7 +52,10 @@ pub enum DriveError { impl DriveBackend { pub fn new(client: Arc) -> Self { - Self { client } + Self { + client, + deadline: DEADLINE, + } } pub fn workspace(&self) -> Result<&str, DriveError> { self.client @@ -61,15 +67,28 @@ impl DriveBackend { Ok(format!("/api/w/{}/drive", encode(self.workspace()?))) } pub(super) fn validate_entry(&self, entry: &DriveEntry) -> Result<(), DriveError> { - self.validate_ref(&entry.entry)?; + self.validate_entry_at( + entry, + &Diagnostic::new(Adapter::Drive, "validate_entry", false, None), + ) + } + fn validate_entry_at( + &self, + entry: &DriveEntry, + diagnostic: &Diagnostic, + ) -> Result<(), DriveError> { + self.validate_ref(&entry.entry) + .inspect_err(|_| diagnostic.record(Stage::Completion, Reason::Identity, None))?; if let Some(parent) = &entry.parent { - self.validate_ref(parent)?; + self.validate_ref(parent) + .inspect_err(|_| diagnostic.record(Stage::Completion, Reason::Identity, None))?; } if (entry.last_mutation_id.len() > 128 || entry.last_mutation_id.chars().any(char::is_control) || (entry.parent.is_some() && entry.last_mutation_id.is_empty())) || entry.size.is_some_and(|n| n > DRIVE_FILE_MAX_BYTES) { + diagnostic.record(Stage::Completion, Reason::InvalidMetadata, None); return Err(DriveError::Unavailable); } Ok(()) @@ -90,11 +109,13 @@ impl DriveBackend { request: WorkspaceRequest, mutation: bool, request_id: &str, + diagnostic: &Diagnostic, ) -> Result { let client = self.client.clone(); + let deadline = self.deadline; let response = tokio::time::timeout( - DEADLINE, - tokio::task::spawn_blocking(move || client.execute_with_timeout(request, DEADLINE)), + deadline, + tokio::task::spawn_blocking(move || client.execute_with_timeout(request, deadline)), ) .await; let unknown = || { @@ -105,10 +126,20 @@ impl DriveBackend { } }; let response = response - .map_err(|_| unknown())? - .map_err(|_| unknown())? - .map_err(|_| unknown())?; + .map_err(|_| { + diagnostic.record(Stage::Request, Reason::Deadline, None); + unknown() + })? + .map_err(|_| { + diagnostic.record(Stage::Request, Reason::TaskJoin, None); + unknown() + })? + .map_err(|error| { + diagnostic.transport(&error); + unknown() + })?; if response.body.len() > RESPONSE_LIMIT { + diagnostic.record(Stage::Response, Reason::ResponseSize, None); return Err(if mutation { unknown() } else { @@ -121,42 +152,57 @@ impl DriveBackend { response.body.as_bytes(), mutation, request_id, + diagnostic, )); } - serde_json::from_str(&response.body).map_err(|_| unknown()) + serde_json::from_str(&response.body).map_err(|_| { + diagnostic.record(Stage::Response, Reason::Decode, None); + unknown() + }) } async fn get( &self, - suffix: &str, + suffix: &'static str, query: &Q, + diagnostic: &Diagnostic, ) -> Result { let query = query_string(query)?; self.json( WorkspaceRequest::get(format!("{}{suffix}{query}", self.base()?)), false, "", + diagnostic, ) .await } pub async fn capabilities(&self) -> Result { - self.get("/capabilities", &serde_json::json!({})).await + self.get( + "/capabilities", + &serde_json::json!({}), + &Diagnostic::new(Adapter::Drive, "capabilities", false, None), + ) + .await } pub async fn root(&self) -> Result { + let diagnostic = Diagnostic::new(Adapter::Drive, "root", false, None); let entry: DriveEntry = self .json( WorkspaceRequest::get(format!("{}/root", self.base()?)), false, "", + &diagnostic, ) .await?; - self.validate_entry(&entry)?; + self.validate_entry_at(&entry, &diagnostic)?; if entry.kind != DriveEntryKind::Folder || entry.parent.is_some() { + diagnostic.record(Stage::Completion, Reason::InvalidMetadata, None); return Err(DriveError::Unavailable); } Ok(entry) } pub async fn metadata(&self, entry: &DriveEntryRef) -> Result { self.validate_ref(entry)?; + let diagnostic = Diagnostic::new(Adapter::Drive, "metadata", false, None); let result: DriveEntry = self .get( "/metadata", @@ -164,10 +210,12 @@ impl DriveBackend { entry_workspace_id: entry.workspace_id.clone(), id: entry.node_id.clone(), }, + &diagnostic, ) .await?; - self.validate_entry(&result)?; + self.validate_entry_at(&result, &diagnostic)?; if result.entry != *entry { + diagnostic.record(Stage::Completion, Reason::Identity, None); return Err(DriveError::Unavailable); } Ok(result) @@ -179,6 +227,7 @@ impl DriveBackend { after: Option, ) -> Result { self.validate_ref(parent)?; + let diagnostic = Diagnostic::new(Adapter::Drive, "list", false, None); let result: DriveListResponse = self .get( "/list", @@ -188,6 +237,7 @@ impl DriveBackend { limit, after, }, + &diagnostic, ) .await?; self.validate_page( @@ -195,33 +245,43 @@ impl DriveBackend { limit .unwrap_or(DRIVE_PAGE_MAX_LIMIT) .min(DRIVE_PAGE_MAX_LIMIT), + &diagnostic, )?; if result .entries .iter() .any(|entry| entry.parent.as_ref() != Some(parent)) { + diagnostic.record(Stage::Completion, Reason::Identity, None); return Err(DriveError::Unavailable); } Ok(result) } pub async fn search(&self, query: DriveSearchQuery) -> Result { - let result: DriveListResponse = self.get("/search", &query).await?; + let diagnostic = Diagnostic::new(Adapter::Drive, "search", false, None); + let result: DriveListResponse = self.get("/search", &query, &diagnostic).await?; self.validate_page( &result, query .limit .unwrap_or(DRIVE_SEARCH_MAX_LIMIT) .min(DRIVE_SEARCH_MAX_LIMIT), + &diagnostic, )?; Ok(result) } - fn validate_page(&self, page: &DriveListResponse, limit: u32) -> Result<(), DriveError> { + fn validate_page( + &self, + page: &DriveListResponse, + limit: u32, + diagnostic: &Diagnostic, + ) -> Result<(), DriveError> { if page.entries.len() > limit as usize { + diagnostic.record(Stage::Completion, Reason::ResponseSize, None); return Err(DriveError::Limit); } for entry in &page.entries { - self.validate_entry(entry)?; + self.validate_entry_at(entry, diagnostic)?; } Ok(()) } @@ -231,6 +291,7 @@ impl DriveBackend { max_bytes: u32, ) -> Result { self.validate_ref(entry)?; + let diagnostic = Diagnostic::new(Adapter::Drive, "read_text", false, None); let result: DriveReadTextResponse = self .get( "/read-text", @@ -239,10 +300,16 @@ impl DriveBackend { id: entry.node_id.clone(), max_bytes, }, + &diagnostic, ) .await?; - self.validate_entry(&result.entry)?; - if result.entry.entry != *entry || result.text.len() > max_bytes as usize { + self.validate_entry_at(&result.entry, &diagnostic)?; + if result.entry.entry != *entry { + diagnostic.record(Stage::Completion, Reason::Identity, None); + return Err(DriveError::Unavailable); + } + if result.text.len() > max_bytes as usize { + diagnostic.record(Stage::Completion, Reason::ResponseSize, None); return Err(DriveError::Unavailable); } Ok(result) @@ -255,6 +322,7 @@ impl DriveBackend { if total as usize > limit { return Err(DriveError::Limit); } + let diagnostic = Diagnostic::new(Adapter::Drive, "read_chunk", false, None); let mut bytes = Vec::with_capacity(total as usize); // Even an empty file gets a read bound to the observed committed mutation and current authorization. loop { @@ -274,21 +342,42 @@ impl DriveBackend { max_response_bytes: DRIVE_CHUNK_MAX_BYTES as usize, }; let client = self.client.clone(); + let deadline = self.deadline; let response = tokio::time::timeout( - DEADLINE, + deadline, tokio::task::spawn_blocking(move || { - client.execute_binary_with_timeout(request, DEADLINE) + client.execute_binary_with_timeout(request, deadline) }), ) .await - .map_err(|_| DriveError::Unavailable)? - .map_err(|_| DriveError::Unavailable)? - .map_err(|_| DriveError::Unavailable)?; + .map_err(|_| { + diagnostic.record(Stage::Request, Reason::Deadline, None); + DriveError::Unavailable + })? + .map_err(|_| { + diagnostic.record(Stage::Request, Reason::TaskJoin, None); + DriveError::Unavailable + })? + .map_err(|error| { + diagnostic.transport(&error); + DriveError::Unavailable + })?; + if response.body.len() > DRIVE_CHUNK_MAX_BYTES as usize { + diagnostic.record(Stage::Response, Reason::ResponseSize, None); + return Err(DriveError::Unavailable); + } if !(200..300).contains(&response.status) { - return Err(classify(response.status, &response.body, false, "")); + return Err(classify( + response.status, + &response.body, + false, + "", + &diagnostic, + )); } let expected = total.saturating_sub(offset).min(length) as usize; if response.body.len() != expected { + diagnostic.record(Stage::Response, Reason::ResponseLength, None); return Err(DriveError::Unavailable); } bytes.extend_from_slice(&response.body); @@ -301,22 +390,40 @@ impl DriveBackend { &self, id: &str, result: Result, + diagnostic: &Diagnostic, ) -> Result { // A parseable success can still be an invalid completion. Classify it // before reconciliation so it also gets one original-request inquiry. let result = result.and_then(|response| { - self.validate_completion(id, &response) + self.validate_completion(id, &response, diagnostic) .map_err(|_| DriveError::OutcomeUnknown(id.into()))?; Ok(response) }); match result { - Err(DriveError::OutcomeUnknown(_)) => match self.status(id).await { + Err(DriveError::OutcomeUnknown(_)) => match self + .status_with_diagnostic(id, &diagnostic.at_stage(Stage::Reconcile)) + .await + { Ok(DriveRequestStatusResponse { state: DriveRequestState::Committed, response: Some(response), .. - }) => Ok(response), // status validates both IDs and entry metadata - _ => Err(DriveError::OutcomeUnknown(id.into())), + }) => { + diagnostic.record(Stage::Reconcile, Reason::Reconciled, None); + Ok(response) // status validates both IDs and entry metadata + } + Ok(status) => { + diagnostic.record( + Stage::Reconcile, + match status.state { + DriveRequestState::Uncommitted => Reason::NotCommittedSnapshot, + DriveRequestState::Committed => Reason::IncompleteReceipt, + }, + None, + ); + Err(DriveError::OutcomeUnknown(id.into())) + } + Err(_) => Err(DriveError::OutcomeUnknown(id.into())), }, other => other, } @@ -325,12 +432,14 @@ impl DriveBackend { &self, id: &str, response: &DriveMutationResponse, + diagnostic: &Diagnostic, ) -> Result<(), DriveError> { if response.request_id != id { + diagnostic.record(Stage::Completion, Reason::Identity, None); return Err(DriveError::Unavailable); } if let Some(entry) = &response.entry { - self.validate_entry(entry)?; + self.validate_entry_at(entry, diagnostic)?; } Ok(()) } @@ -338,9 +447,11 @@ impl DriveBackend { &self, request: DriveMutationRequest, ) -> Result { + let diagnostic = Diagnostic::new(Adapter::Drive, "mutate", true, Some(&request.request_id)); let body = serde_json::to_string(&request) .map_err(|_| DriveError::Invalid("invalid mutation".into()))?; if body.len() > 512 * 1024 { + diagnostic.record(Stage::Request, Reason::RequestSize, None); return Err(DriveError::Limit); } let result = self @@ -352,16 +463,20 @@ impl DriveBackend { ), true, &request.request_id, + &diagnostic, ) .await; - self.reconcile(&request.request_id, result).await + self.reconcile(&request.request_id, result, &diagnostic) + .await } pub async fn upload( &self, query: DriveUploadQuery, bytes: Vec, ) -> Result { + let diagnostic = Diagnostic::new(Adapter::Drive, "upload", true, Some(&query.request_id)); if bytes.len() > DRIVE_FILE_MAX_BYTES as usize { + diagnostic.record(Stage::Request, Reason::RequestSize, None); return Err(DriveError::Limit); } let path = format!("{}/upload{}", self.base()?, query_string(&query)?); @@ -372,29 +487,56 @@ impl DriveBackend { max_response_bytes: RESPONSE_LIMIT, }; let client = self.client.clone(); + let deadline = self.deadline; let result = tokio::time::timeout( - DEADLINE, + deadline, tokio::task::spawn_blocking(move || { - client.execute_binary_with_timeout(request, DEADLINE) + client.execute_binary_with_timeout(request, deadline) }), ) .await; let result = match result { + Ok(Ok(Ok(response))) if response.body.len() > RESPONSE_LIMIT => { + diagnostic.record(Stage::Response, Reason::ResponseSize, None); + Err(DriveError::OutcomeUnknown(query.request_id.clone())) + } Ok(Ok(Ok(response))) if (200..300).contains(&response.status) => { - serde_json::from_slice(&response.body) - .map_err(|_| DriveError::OutcomeUnknown(query.request_id.clone())) + serde_json::from_slice(&response.body).map_err(|_| { + diagnostic.record(Stage::Response, Reason::Decode, None); + DriveError::OutcomeUnknown(query.request_id.clone()) + }) } Ok(Ok(Ok(response))) => Err(classify( response.status, &response.body, true, &query.request_id, + &diagnostic, )), - _ => Err(DriveError::OutcomeUnknown(query.request_id.clone())), + Err(_) => { + diagnostic.record(Stage::Request, Reason::Deadline, None); + Err(DriveError::OutcomeUnknown(query.request_id.clone())) + } + Ok(Err(_)) => { + diagnostic.record(Stage::Request, Reason::TaskJoin, None); + Err(DriveError::OutcomeUnknown(query.request_id.clone())) + } + Ok(Ok(Err(error))) => { + diagnostic.transport(&error); + Err(DriveError::OutcomeUnknown(query.request_id.clone())) + } }; - self.reconcile(&query.request_id, result).await + self.reconcile(&query.request_id, result, &diagnostic).await } pub async fn status(&self, request_id: &str) -> Result { + let diagnostic = Diagnostic::new(Adapter::Drive, "request_status", false, Some(request_id)); + self.status_with_diagnostic(request_id, &diagnostic).await + } + async fn status_with_diagnostic( + &self, + request_id: &str, + diagnostic: &Diagnostic, + ) -> Result { if request_id.is_empty() || request_id.len() > 256 { return Err(DriveError::Invalid("invalid request ID".into())); } @@ -403,13 +545,15 @@ impl DriveBackend { WorkspaceRequest::get(format!("{}/requests/{}", self.base()?, encode(request_id))), false, "", + diagnostic, ) .await?; if response.request_id != request_id { + diagnostic.record(Stage::Reconcile, Reason::Identity, None); return Err(DriveError::Unavailable); } if let Some(result) = &response.response { - self.validate_completion(request_id, result)?; + self.validate_completion(request_id, result, diagnostic)?; } Ok(response) } @@ -463,11 +607,21 @@ fn query_string(value: &T) -> Result { format!("?{}", pairs.join("&")) }) } -fn classify(status: u16, body: &[u8], mutation: bool, id: &str) -> DriveError { +fn classify( + status: u16, + body: &[u8], + mutation: bool, + id: &str, + diagnostic: &Diagnostic, +) -> DriveError { if let Ok(error) = serde_json::from_slice::(body) { - if error.classification == DriveFailureClassification::Unknown { + if error.classification == DriveFailureClassification::Unknown + || error.code == DriveApiErrorCode::OutcomeUnknown + { + diagnostic.record(Stage::Response, Reason::BackendUnknown, Some(status)); return DriveError::OutcomeUnknown(id.into()); } + diagnostic.record(Stage::Response, Reason::BackendRejected, Some(status)); return match error.code { DriveApiErrorCode::Denied => DriveError::Denied, DriveApiErrorCode::NotFound => DriveError::NotFound, @@ -478,6 +632,7 @@ fn classify(status: u16, body: &[u8], mutation: bool, id: &str) -> DriveError { DriveApiErrorCode::OutcomeUnknown => DriveError::OutcomeUnknown(id.into()), }; } + diagnostic.record(Stage::Response, Reason::UnrecognizedResponse, Some(status)); if matches!(status, 401 | 403) { DriveError::Denied } else if mutation { @@ -486,3 +641,7 @@ fn classify(status: u16, body: &[u8], mutation: bool, id: &str) -> DriveError { DriveError::Unavailable } } + +#[cfg(test)] +#[path = "backend_tests.rs"] +mod tests; diff --git a/crates/worker/src/feature/builtin/drive/backend_tests.rs b/crates/worker/src/feature/builtin/drive/backend_tests.rs new file mode 100644 index 000000000..46109b3cc --- /dev/null +++ b/crates/worker/src/feature/builtin/drive/backend_tests.rs @@ -0,0 +1,644 @@ +//! Production Drive adapter fault injection. Only the Workspace transport boundary is substituted; +//! retained fixture effects and original-request inquiries are inspected independently of replies. +//! The HTTP case additionally connects the adapter to reqwest and an isolated loopback router. +use super::*; +use crate::feature::builtin::backend_diagnostic::{Reason, tests::Capture}; +use crate::worker::{WorkspaceBinaryResponse, WorkspaceClientError, WorkspaceResponse}; +use std::sync::{Condvar, Mutex}; +use tracing::instrument::WithSubscriber; + +const SECRET: &str = "secret-token /private/drive credential=unsafe"; +const ID: &str = "host-request-1"; + +fn entry() -> DriveEntry { + DriveEntry { + entry: DriveEntryRef { + workspace_id: "workspace".into(), + node_id: "2".into(), + }, + parent: Some(DriveEntryRef { + workspace_id: "workspace".into(), + node_id: "1".into(), + }), + name: SECRET.into(), + kind: DriveEntryKind::File, + last_mutation_id: "old-request".into(), + size: Some(5), + content_type: Some("text/plain".into()), + updated_by: SECRET.into(), + updated_at: SECRET.into(), + latest_url: SECRET.into(), + } +} +fn mutation() -> DriveMutationRequest { + DriveMutationRequest { + request_id: ID.into(), + mutation: DriveMutation::UpdateText { + id: entry().entry, + expected_mutation_id: "old-request".into(), + text: SECRET.into(), + content_type: "text/plain".into(), + }, + } +} +fn json(value: impl Serialize) -> WorkspaceResponse { + WorkspaceResponse { + status: 200, + body: serde_json::to_string(&value).unwrap(), + } +} +#[derive(Clone, Copy, Debug)] +enum Fault { + Communication, + Unavailable, + InvalidPath, + Decode, + Size, + Length, + Identity, + Metadata, + Rejected, + Unknown, + Unrecognized, + Incomplete, + Panic, + Deadline, + None, +} +#[derive(Debug, Default)] +struct State { + mutations: usize, + saved: Option, + requests: Vec, + binary: Vec, + released: bool, + started: bool, + finished: bool, +} +#[derive(Debug)] +struct Client { + fault: Fault, + receipt: Fault, + state: Mutex, + changed: Condvar, + started: tokio::sync::Notify, +} +impl Client { + fn new(fault: Fault, receipt: Fault) -> Arc { + Arc::new(Self { + fault, + receipt, + state: Mutex::new(State::default()), + changed: Condvar::new(), + started: tokio::sync::Notify::new(), + }) + } + fn fail( + &self, + fault: Fault, + success: WorkspaceResponse, + ) -> Result { + match fault { + Fault::Communication => Err(WorkspaceClientError::Request(SECRET.into())), + Fault::Unavailable => Err(WorkspaceClientError::Unavailable(SECRET.into())), + Fault::InvalidPath => Err(WorkspaceClientError::InvalidPath(SECRET.into())), + Fault::Panic => panic!("injected adapter task panic"), + Fault::Decode => Ok(WorkspaceResponse { + status: 200, + body: SECRET.into(), + }), + Fault::Size => Ok(WorkspaceResponse { + status: 200, + body: SECRET.repeat(RESPONSE_LIMIT / SECRET.len() + 1), + }), + Fault::Rejected | Fault::Unknown => Ok(WorkspaceResponse { + status: 409, + body: serde_json::to_string(&DriveApiError { + code: DriveApiErrorCode::Conflict, + message: SECRET.into(), + classification: if matches!(fault, Fault::Unknown) { + DriveFailureClassification::Unknown + } else { + DriveFailureClassification::NotCommitted + }, + }) + .unwrap(), + }), + Fault::Unrecognized => Ok(WorkspaceResponse { + status: 503, + body: SECRET.into(), + }), + _ => Ok(success), + } + } + fn release(&self) { + self.state.lock().unwrap().released = true; + self.changed.notify_all(); + } + fn wait(&self, finished: bool) { + let (state, timeout) = self + .changed + .wait_timeout_while(self.state.lock().unwrap(), Duration::from_secs(5), |s| { + if finished { !s.finished } else { !s.started } + }) + .unwrap(); + assert!(!timeout.timed_out()); + assert!(if finished { + state.finished + } else { + state.started + }); + } +} +struct Release(Arc); +impl Drop for Release { + fn drop(&mut self) { + self.0.release(); + } +} +impl WorkspaceClient for Client { + fn workspace_id(&self) -> Option<&str> { + Some("workspace") + } + fn kind(&self) -> &str { + "drive-diagnostic-fixture" + } + fn is_available(&self) -> bool { + true + } + fn execute( + &self, + request: WorkspaceRequest, + ) -> Result { + let is_status = request.path.contains("/requests/"); + let fault = if is_status { self.receipt } else { self.fault }; + let mutation = request.method == WorkspaceRequestMethod::Post; + { + let mut state = self.state.lock().unwrap(); + if mutation + && !matches!( + fault, + Fault::Rejected | Fault::InvalidPath | Fault::Unavailable + ) + { + let input: DriveMutationRequest = + serde_json::from_str(request.body.as_ref().unwrap()).unwrap(); + assert_eq!(input, super::tests::mutation()); + state.mutations += 1; + if let DriveMutation::UpdateText { text, .. } = input.mutation { + state.saved = Some(text); + } + } + state.requests.push(request); + state.started = true; + self.started.notify_one(); + self.changed.notify_all(); + if matches!(fault, Fault::Deadline) { + let (_state, timeout) = self + .changed + .wait_timeout_while(state, Duration::from_secs(5), |s| !s.released) + .unwrap(); + assert!( + !timeout.timed_out(), + "transport must be explicitly released" + ); + } + } + let mut metadata = entry(); + if matches!(fault, Fault::Identity) { + metadata.entry.node_id = "3".into(); + } + if matches!(fault, Fault::Metadata) { + metadata.size = Some(DRIVE_FILE_MAX_BYTES + 1); + } + let completion = DriveMutationResponse { + request_id: if matches!(fault, Fault::Identity) { + "other-request" + } else { + ID + } + .into(), + entry: Some(metadata.clone()), + }; + let value = if is_status { + json(DriveRequestStatusResponse { + request_id: if matches!(fault, Fault::Identity) { + "other-request" + } else { + ID + } + .into(), + state: if matches!( + fault, + Fault::None | Fault::Identity | Fault::Metadata | Fault::Incomplete + ) { + DriveRequestState::Committed + } else { + DriveRequestState::Uncommitted + }, + response: if matches!(fault, Fault::None | Fault::Identity | Fault::Metadata) { + Some(completion) + } else { + None + }, + }) + } else if mutation { + json(completion) + } else { + json(metadata) + }; + if !is_status { + self.state.lock().unwrap().finished = true; + } + self.changed.notify_all(); + self.fail(fault, value) + } + fn execute_binary( + &self, + request: WorkspaceBinaryRequest, + ) -> Result { + let upload = request.method == WorkspaceRequestMethod::Put; + { + let mut state = self.state.lock().unwrap(); + if upload { + state.mutations += 1; + state.saved = Some(SECRET.into()); + } + state.binary.push(request); + } + let success = if upload { + json(DriveMutationResponse { + request_id: ID.into(), + entry: Some(entry()), + }) + } else { + WorkspaceResponse { + status: 200, + body: if matches!(self.fault, Fault::Length) { + "x" + } else { + "12345" + } + .into(), + } + }; + self.fail(self.fault, success) + .map(|r| WorkspaceBinaryResponse { + status: r.status, + body: r.body.into_bytes(), + }) + } +} +fn safe(capture: &Capture, reason: Reason, public: &str) { + let text = capture.text(); + assert!(text.contains(&format!("reason={reason:?}")), "{text}"); + assert!(text.contains("adapter=Drive"), "{text}"); + assert!(text.contains("correlation="), "{text}"); + for secret in [ + SECRET, + "secret-token", + "/private/drive", + "credential=unsafe", + ] { + assert!(!text.contains(secret), "internal diagnostic leaked content"); + assert!(!public.contains(secret), "public error leaked content"); + } +} +fn reason(fault: Fault) -> Reason { + match fault { + Fault::Communication => Reason::Communication, + Fault::Unavailable => Reason::TransportUnavailable, + Fault::InvalidPath => Reason::InvalidPath, + Fault::Decode => Reason::Decode, + Fault::Size => Reason::ResponseSize, + Fault::Length => Reason::ResponseLength, + Fault::Identity => Reason::Identity, + Fault::Metadata => Reason::InvalidMetadata, + Fault::Rejected => Reason::BackendRejected, + Fault::Unknown => Reason::BackendUnknown, + Fault::Unrecognized => Reason::UnrecognizedResponse, + Fault::Panic => Reason::TaskJoin, + Fault::Deadline => Reason::Deadline, + Fault::Incomplete => Reason::IncompleteReceipt, + Fault::None => panic!("no fault"), + } +} +#[tokio::test] +async fn production_read_failures_retain_public_classification_and_safe_cause() { + for fault in [ + Fault::Communication, + Fault::Unavailable, + Fault::InvalidPath, + Fault::Decode, + Fault::Size, + Fault::Identity, + Fault::Metadata, + Fault::Rejected, + Fault::Unknown, + Fault::Unrecognized, + Fault::Panic, + ] { + let client = Client::new(fault, Fault::None); + let backend = DriveBackend::new(client.clone()); + let capture = Capture::default(); + let error = backend + .metadata(&entry().entry) + .with_subscriber(capture.dispatch()) + .await + .unwrap_err(); + match fault { + Fault::Size => assert!(matches!(error, DriveError::Limit)), + Fault::Rejected => assert!(matches!(error, DriveError::Conflict)), + Fault::Unknown => assert!(matches!(error, DriveError::OutcomeUnknown(_))), + _ => assert!(matches!(error, DriveError::Unavailable)), + } + safe(&capture, reason(fault), &error.to_string()); + let state = client.state.lock().unwrap(); + assert_eq!(state.requests.len(), 1); + assert_eq!(state.mutations, 0); + } +} +#[tokio::test] +async fn production_mutation_loss_reconciles_only_original_identity_and_never_resends() { + for fault in [ + Fault::Communication, + Fault::Decode, + Fault::Size, + Fault::Identity, + Fault::Metadata, + Fault::Unknown, + Fault::Unrecognized, + Fault::Panic, + ] { + for receipt in [ + Fault::None, + Fault::Identity, + Fault::Metadata, + Fault::Decode, + Fault::Communication, + Fault::Unrecognized, + Fault::Length, + Fault::Incomplete, + ] { + let client = Client::new(fault, receipt); + let backend = DriveBackend::new(client.clone()); + let capture = Capture::default(); + let result = backend + .mutate(mutation()) + .with_subscriber(capture.dispatch()) + .await; + if matches!(receipt, Fault::None) { + assert_eq!(result.unwrap().request_id, ID); + } else { + assert!(matches!(result, Err(DriveError::OutcomeUnknown(ref id)) if id == ID)); + } + safe(&capture, reason(fault), ""); + let text = capture.text(); + if matches!(receipt, Fault::Length | Fault::Incomplete) { + safe( + &capture, + if matches!(receipt, Fault::Length) { + Reason::NotCommittedSnapshot + } else { + Reason::IncompleteReceipt + }, + "", + ); + } + assert!(text.contains("stage=Reconcile"), "{text}"); + let correlations: std::collections::BTreeSet<_> = text + .lines() + .filter_map(|l| l.split("correlation=").nth(1)) + .filter_map(|l| l.split(';').next()) + .collect(); + assert_eq!( + correlations.len(), + 1, + "original loss and inquiry must correlate" + ); + assert!( + !text.contains(ID), + "request identity must be opaque in logs" + ); + let state = client.state.lock().unwrap(); + assert_eq!(state.mutations, 1); + assert_eq!(state.saved.as_deref(), Some(SECRET)); + assert_eq!(state.requests.len(), 2); + assert_eq!(state.requests[1].method, WorkspaceRequestMethod::Get); + assert!(state.requests[1].path.ends_with(&format!("/requests/{ID}"))); + } + } +} +#[tokio::test] +async fn production_rejection_has_no_effect_or_status_inquiry() { + let client = Client::new(Fault::Rejected, Fault::None); + let backend = DriveBackend::new(client.clone()); + let capture = Capture::default(); + let error = backend + .mutate(mutation()) + .with_subscriber(capture.dispatch()) + .await + .unwrap_err(); + assert!(matches!(error, DriveError::Conflict)); + safe(&capture, Reason::BackendRejected, &error.to_string()); + let state = client.state.lock().unwrap(); + assert_eq!(state.mutations, 0); + assert_eq!(state.requests.len(), 1); +} +#[tokio::test] +async fn production_binary_length_transport_and_upload_loss_are_diagnosed() { + for fault in [ + Fault::Length, + Fault::Communication, + Fault::Unavailable, + Fault::Panic, + ] { + let client = Client::new(fault, Fault::None); + let backend = DriveBackend::new(client.clone()); + let capture = Capture::default(); + let error = backend + .bytes(&entry(), 5) + .with_subscriber(capture.dispatch()) + .await + .unwrap_err(); + assert!(matches!(error, DriveError::Unavailable)); + safe(&capture, reason(fault), &error.to_string()); + assert_eq!(client.state.lock().unwrap().binary.len(), 1); + } + for fault in [Fault::Decode, Fault::Communication, Fault::Size] { + let client = Client::new(fault, Fault::Unrecognized); + let backend = DriveBackend::new(client.clone()); + let capture = Capture::default(); + let query = DriveUploadQuery { + operation: DriveUploadOperation::Update, + request_id: ID.into(), + entry_workspace_id: "workspace".into(), + parent_id: None, + name: None, + id: Some("2".into()), + expected_mutation_id: Some("old-request".into()), + content_type: "text/plain".into(), + size: 5, + sha256: "hash".into(), + }; + let error = backend + .upload(query, b"12345".to_vec()) + .with_subscriber(capture.dispatch()) + .await + .unwrap_err(); + assert!(matches!(error, DriveError::OutcomeUnknown(ref id) if id == ID)); + safe(&capture, reason(fault), &error.to_string()); + let state = client.state.lock().unwrap(); + assert_eq!(state.mutations, 1); + assert_eq!(state.binary.len(), 1); + assert_eq!(state.requests.len(), 1); + assert!(state.requests[0].path.ends_with(&format!("/requests/{ID}"))); + } +} +#[tokio::test] +async fn production_deadline_does_not_cancel_effect_or_infer_uncommitted_from_snapshot() { + for mutation_call in [false, true] { + let client = Client::new(Fault::Deadline, Fault::Unrecognized); + let release = Release(client.clone()); + let mut backend = DriveBackend::new(client.clone()); + backend.deadline = Duration::from_secs(1); + let capture = Capture::default(); + let request = async { + if mutation_call { + backend.mutate(mutation()).await.map(|_| ()) + } else { + backend.metadata(&entry().entry).await.map(|_| ()) + } + } + .with_subscriber(capture.dispatch()); + tokio::pin!(request); + tokio::select! { + biased; + _ = client.started.notified() => {}, + _ = &mut request => panic!("request must start before timeout"), + } + { + let state = client.state.lock().unwrap(); + assert_eq!(state.mutations, usize::from(mutation_call)); + assert!(!state.released && !state.finished); + } + let result = request.await; + let error = result.unwrap_err(); + assert!(if mutation_call { + matches!(error, DriveError::OutcomeUnknown(ref id) if id == ID) + } else { + matches!(error, DriveError::Unavailable) + }); + safe(&capture, Reason::Deadline, &error.to_string()); + drop(release); + let waiting = client.clone(); + tokio::task::spawn_blocking(move || waiting.wait(true)) + .await + .unwrap(); + let state = client.state.lock().unwrap(); + assert_eq!(state.mutations, usize::from(mutation_call)); + assert_eq!(state.saved.is_some(), mutation_call); + assert_eq!(state.requests.len(), if mutation_call { 2 } else { 1 }); + } +} + +#[tokio::test] +async fn http_reply_loss_keeps_original_receipt_and_retained_effect_without_reexecution() { + use axum::{ + Router, + extract::State as HttpState, + routing::{get, post}, + }; + #[derive(Default)] + struct Stored { + updates: usize, + content: String, + ids: Vec, + } + struct Server(tokio::task::JoinHandle<()>); + impl Drop for Server { + fn drop(&mut self) { + self.0.abort(); + } + } + let stored = Arc::new(Mutex::new(Stored::default())); + let router = Router::new() + .route("/api/w/workspace/drive/metadata", get(|| async { SECRET })) + .route( + "/api/w/workspace/drive/mutate", + post( + |HttpState(state): HttpState>>, + body: axum::body::Bytes| async move { + let request: DriveMutationRequest = serde_json::from_slice(&body).unwrap(); + let mut state = state.lock().unwrap(); + state.ids.push(request.request_id); + if let DriveMutation::UpdateText { + text, + expected_mutation_id, + .. + } = request.mutation + { + assert_eq!(expected_mutation_id, "old-request"); + state.content = text; + state.updates += 1; + } else { + panic!("unexpected mutation"); + } + // The effect is retained; only the success response is malformed. + SECRET + }, + ), + ) + .route( + "/api/w/workspace/drive/requests/{id}", + get( + |HttpState(state): HttpState>>, + axum::extract::Path(id): axum::extract::Path| async move { + let state = state.lock().unwrap(); + assert_eq!(state.ids, [id.clone()]); + axum::Json(DriveRequestStatusResponse { + request_id: id.clone(), + state: DriveRequestState::Committed, + response: Some(DriveMutationResponse { + request_id: id, + entry: Some(entry()), + }), + }) + }, + ), + ) + .with_state(stored.clone()); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let server = Server(tokio::spawn(async move { + axum::serve(listener, router).await.unwrap(); + })); + let client = Arc::new(crate::worker::TestWorkspaceHttpClient::new( + "workspace", + format!("http://{address}"), + )); + let backend = DriveBackend::new(client); + let capture = Capture::default(); + let error = backend + .metadata(&entry().entry) + .with_subscriber(capture.dispatch()) + .await + .unwrap_err(); + assert!(matches!(error, DriveError::Unavailable)); + safe(&capture, Reason::Decode, &error.to_string()); + let capture = Capture::default(); + let response = backend + .mutate(mutation()) + .with_subscriber(capture.dispatch()) + .await + .unwrap(); + assert_eq!(response.request_id, ID); + safe(&capture, Reason::Decode, ""); + assert!(capture.text().contains("reason=Reconciled")); + let state = stored.lock().unwrap(); + assert_eq!(state.updates, 1); + assert_eq!(state.content, SECRET); + assert_eq!(state.ids, [ID]); + drop(state); + drop(server); +} diff --git a/crates/worker/src/feature/builtin/workspace_config/backend.rs b/crates/worker/src/feature/builtin/workspace_config/backend.rs index f10c60eda..d5d58e376 100644 --- a/crates/worker/src/feature/builtin/workspace_config/backend.rs +++ b/crates/worker/src/feature/builtin/workspace_config/backend.rs @@ -7,6 +7,7 @@ //! undecodable mutation responses do not prove non-commit; dispatched requests are never resent. //! This adapter does not reimplement Backend source-persistence transactions or activation. use std::sync::Arc; +use std::time::Duration; use async_trait::async_trait; use serde::{Serialize, de::DeserializeOwned}; @@ -20,6 +21,7 @@ use super::{ ATTACHMENT_ALIAS, CONTENT_ROOT, ConfigAccess, ConfigAttachError, ConfigAttachment, DEADLINE, WorkspaceConfigAttachmentBackend, }; +use crate::feature::builtin::backend_diagnostic::{Adapter, Diagnostic, Reason, Stage}; use crate::wip::WipOperationError; use crate::worker::{ WorkspaceClient, WorkspaceClientError, WorkspaceRequest, WorkspaceRequestMethod, @@ -34,11 +36,21 @@ const MAX_RESPONSE_BYTES: usize = 2 * 1024 * 1024; #[derive(Clone)] pub struct WorkspaceConfigBackend { client: Arc, + deadline: Duration, } impl WorkspaceConfigBackend { pub fn new(client: Arc) -> Self { - Self { client } + Self { + client, + deadline: DEADLINE, + } + } + + #[cfg(test)] + pub(super) fn with_deadline(mut self, deadline: Duration) -> Self { + self.deadline = deadline; + self } fn base(&self) -> Result { @@ -47,6 +59,7 @@ impl WorkspaceConfigBackend { .workspace_id() .filter(|id| !id.is_empty()) .ok_or_else(|| { + diagnostic("authority", false).record(Stage::Request, Reason::Identity, None); failure( ProtocolErrorCode::PermissionDenied, "Workspace config authority unavailable", @@ -69,11 +82,13 @@ impl WorkspaceConfigBackend { &self, request: WorkspaceRequest, mutation: bool, + diagnostic: &Diagnostic, ) -> Result { let response = self .client - .execute_with_timeout(request, DEADLINE) + .execute_with_timeout(request, self.deadline) .map_err(|error| { + diagnostic.transport(&error); if mutation && !matches!(error, WorkspaceClientError::InvalidPath(_)) { unknown() } else { @@ -84,6 +99,7 @@ impl WorkspaceConfigBackend { } })?; if response.body.len() > MAX_RESPONSE_BYTES { + diagnostic.record(Stage::Response, Reason::ResponseSize, Some(response.status)); return Err(if mutation { unknown() } else { @@ -96,8 +112,18 @@ impl WorkspaceConfigBackend { if !response.is_success() { if let Ok(error) = serde_json::from_str::(&response.body) { if error.classification == WorkspaceConfigFailureClassification::Unknown { + diagnostic.record( + Stage::Response, + Reason::BackendUnknown, + Some(response.status), + ); return Err(unknown()); } + diagnostic.record( + Stage::Response, + Reason::BackendRejected, + Some(response.status), + ); // Do not reflect arbitrary transport/provider text into model output. let code = match error.status { 401 | 403 | 404 => ProtocolErrorCode::PermissionDenied, @@ -111,6 +137,11 @@ impl WorkspaceConfigBackend { "Workspace config request rejected without commit", )); } + diagnostic.record( + Stage::Response, + Reason::UnrecognizedResponse, + Some(response.status), + ); return Err(if mutation { unknown() } else { @@ -125,6 +156,7 @@ impl WorkspaceConfigBackend { }); } serde_json::from_str(&response.body).map_err(|_| { + diagnostic.record(Stage::Response, Reason::Decode, Some(response.status)); if mutation { unknown() } else { @@ -138,10 +170,17 @@ impl WorkspaceConfigBackend { /// GET checks the current grant even when the Worker has not attached yet. pub fn current(&self) -> Result, WipOperationError> { + self.current_diagnosed(&diagnostic("current", false)) + } + + fn current_diagnosed( + &self, + diagnostic: &Diagnostic, + ) -> Result, WipOperationError> { let result: Option = - self.request(WorkspaceRequest::get(self.base()?), false)?; + self.request(WorkspaceRequest::get(self.base()?), false, diagnostic)?; if let Some(attachment) = &result { - self.validate_attachment(attachment)?; + self.validate_attachment(attachment, diagnostic)?; } Ok(result) } @@ -149,6 +188,7 @@ impl WorkspaceConfigBackend { fn validate_attachment( &self, attachment: &WorkspaceConfigAttachment, + diagnostic: &Diagnostic, ) -> Result<(), WipOperationError> { if Some(attachment.workspace_id.as_str()) != self.client.workspace_id() || attachment.alias != ATTACHMENT_ALIAS @@ -160,6 +200,7 @@ impl WorkspaceConfigBackend { .bytes() .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_')) { + diagnostic.record(Stage::Response, Reason::Identity, None); return Err(failure( ProtocolErrorCode::PermissionDenied, "Invalid Workspace config attachment identity", @@ -173,14 +214,34 @@ impl WorkspaceConfigBackend { suffix: &str, request: &R, mutation: bool, + ) -> Result { + let operation = match suffix { + "/observe" => "observe", + "/read" => "read", + "/commit" => "commit", + "" => "attach", + _ => "post", + }; + self.post_diagnosed(suffix, request, mutation, diagnostic(operation, mutation)) + .await + } + + pub(super) async fn post_diagnosed( + &self, + suffix: &str, + request: &R, + mutation: bool, + diagnostic: Diagnostic, ) -> Result { let body = serde_json::to_string(request).map_err(|_| { + diagnostic.record(Stage::Request, Reason::Decode, None); failure( ProtocolErrorCode::InvalidArguments, "Invalid Workspace config request", ) })?; if body.len() > MAX_RESPONSE_BYTES { + diagnostic.record(Stage::Request, Reason::RequestSize, None); return Err(failure( ProtocolErrorCode::ResourceLimitExceeded, "Workspace config request exceeds limit", @@ -192,12 +253,16 @@ impl WorkspaceConfigBackend { body, ); let this = self.clone(); + let request_diagnostic = diagnostic.clone(); tokio::time::timeout( - DEADLINE, - tokio::task::spawn_blocking(move || this.request(request, mutation)), + self.deadline, + tokio::task::spawn_blocking(move || { + this.request(request, mutation, &request_diagnostic) + }), ) .await .map_err(|_| { + diagnostic.record(Stage::Completion, Reason::Deadline, None); if mutation { unknown() } else { @@ -208,6 +273,7 @@ impl WorkspaceConfigBackend { } })? .map_err(|_| { + diagnostic.record(Stage::Completion, Reason::TaskJoin, None); if mutation { unknown() } else { @@ -223,18 +289,22 @@ impl WorkspaceConfigBackend { &self, ) -> Result, WipOperationError> { let this = self.clone(); + let diagnostic = diagnostic("current", false); + let request_diagnostic = diagnostic.clone(); tokio::time::timeout( - DEADLINE, - tokio::task::spawn_blocking(move || this.current()), + self.deadline, + tokio::task::spawn_blocking(move || this.current_diagnosed(&request_diagnostic)), ) .await .map_err(|_| { + diagnostic.record(Stage::Completion, Reason::Deadline, None); failure( ProtocolErrorCode::ResourceLimitExceeded, "Workspace config request deadline exceeded", ) })? .map_err(|_| { + diagnostic.record(Stage::Completion, Reason::TaskJoin, None); failure( ProtocolErrorCode::Internal, "Workspace config transport unavailable", @@ -249,10 +319,14 @@ impl WorkspaceConfigAttachmentBackend for WorkspaceConfigBackend { self.client.is_available() } fn validate_access(&self, access: Option) -> Result<(), ConfigAttachError> { - let current = self.current().map_err(attach_error)?; + let diagnostic = diagnostic("validate_access", false); + let current = self + .current_diagnosed(&diagnostic) + .map_err(|error| attach_error(error, &diagnostic))?; if access == Some(ConfigAccess::ReadWrite) && current.is_some_and(|a| a.access == WorkspaceConfigAccess::ReadOnly) { + diagnostic.record(Stage::Attach, Reason::AccessCeiling, None); return Err(ConfigAttachError::Denied); } Ok(()) @@ -271,10 +345,16 @@ impl WorkspaceConfigAttachmentBackend for WorkspaceConfigBackend { ConfigAccess::ReadWrite => WorkspaceConfigAccess::ReadWrite, }), }; - let attachment: WorkspaceConfigAttachment = - self.post("", &request, true).await.map_err(attach_error)?; - self.validate_attachment(&attachment) - .map_err(|_| ConfigAttachError::OutcomeUnknown)?; + let diagnostic = diagnostic("attach", true); + let attachment: WorkspaceConfigAttachment = self + .post_diagnosed("", &request, true, diagnostic.clone()) + .await + .map_err(|error| attach_error(error, &diagnostic))?; + self.validate_attachment(&attachment, &diagnostic) + .map_err(|_| { + diagnostic.record(Stage::Attach, Reason::Identity, None); + ConfigAttachError::OutcomeUnknown + })?; Ok(ConfigAttachment { connection_id: attachment.connection_id, access: match attachment.access { @@ -286,7 +366,8 @@ impl WorkspaceConfigAttachmentBackend for WorkspaceConfigBackend { } } -fn attach_error(error: WipOperationError) -> ConfigAttachError { +fn attach_error(error: WipOperationError, diagnostic: &Diagnostic) -> ConfigAttachError { + diagnostic.record(Stage::Attach, Reason::AttachConversion, None); match error { WipOperationError::OutcomeUnknown(_) => ConfigAttachError::OutcomeUnknown, WipOperationError::Protocol(error) => match error.code { @@ -298,6 +379,10 @@ fn attach_error(error: WipOperationError) -> ConfigAttachError { } } +pub(super) fn diagnostic(operation: &'static str, mutation: bool) -> Diagnostic { + Diagnostic::new(Adapter::WorkspaceConfig, operation, mutation, None) +} + pub(super) fn failure(code: ProtocolErrorCode, message: &str) -> WipOperationError { WipOperationError::Protocol(ProtocolError { code, @@ -307,3 +392,7 @@ pub(super) fn failure(code: ProtocolErrorCode, message: &str) -> WipOperationErr pub(super) fn unknown() -> WipOperationError { WipOperationError::OutcomeUnknown("Workspace config outcome unknown; refresh before any subsequent operation; no automatic retry".into()) } + +#[cfg(test)] +#[path = "backend_tests.rs"] +mod tests; diff --git a/crates/worker/src/feature/builtin/workspace_config/backend_tests.rs b/crates/worker/src/feature/builtin/workspace_config/backend_tests.rs new file mode 100644 index 000000000..b617107c7 --- /dev/null +++ b/crates/worker/src/feature/builtin/workspace_config/backend_tests.rs @@ -0,0 +1,580 @@ +//! Production WorkspaceConfigBackend with only WorkspaceClient substituted. The transport fixture +//! applies effects before injected reply loss, retains its state for rereading, and gates blocking +//! completion explicitly. These ordinary worker lib tests do not claim Backend transaction coverage. +use super::*; +use crate::feature::builtin::backend_diagnostic::tests::Capture; +use crate::worker::WorkspaceResponse; +use std::sync::{Condvar, Mutex}; +use tracing::instrument::WithSubscriber; + +const UNSAFE: &str = "secret-token /private/config credential=unsafe"; + +#[derive(Clone, Copy, Debug)] +enum Fault { + Unavailable, + InvalidPath, + Communication, + Decode, + Size, + Rejected(u16), + Unknown, + Unrecognized, + Identity, + Panic, + Deadline, +} + +#[derive(Debug, Default)] +struct Effects { + requests: usize, + mutations: usize, + saved: Option, + completed: bool, + released: bool, +} + +#[derive(Debug)] +struct Client { + fault: Fault, + state: Mutex, + changed: Condvar, + started: tokio::sync::Notify, +} +impl Client { + fn new(fault: Fault) -> Arc { + Arc::new(Self { + fault, + state: Mutex::new(Effects::default()), + changed: Condvar::new(), + started: tokio::sync::Notify::new(), + }) + } + fn finish(&self) { + self.state.lock().unwrap().completed = true; + self.changed.notify_all(); + } + fn release(&self) { + self.state.lock().unwrap().released = true; + self.changed.notify_all(); + } + fn wait_completed(&self) { + let (state, timeout) = self + .changed + .wait_timeout_while(self.state.lock().unwrap(), Duration::from_secs(5), |s| { + !s.completed + }) + .unwrap(); + assert!( + !timeout.timed_out() && state.completed, + "blocking request did not finish" + ); + } +} +struct Release(Arc); +impl Drop for Release { + fn drop(&mut self) { + self.0.release(); + } +} + +fn attachment() -> WorkspaceConfigAttachment { + WorkspaceConfigAttachment { + workspace_id: "workspace".into(), + connection_id: "connection-1".into(), + alias: ATTACHMENT_ALIAS.into(), + content_path: CONTENT_ROOT.into(), + working_directory_id: "grant".into(), + access: WorkspaceConfigAccess::ReadWrite, + name: UNSAFE.into(), + purpose: UNSAFE.into(), + already_attached: false, + } +} + +impl WorkspaceClient for Client { + fn workspace_id(&self) -> Option<&str> { + Some("workspace") + } + fn kind(&self) -> &str { + "diagnostic-test-boundary" + } + fn is_available(&self) -> bool { + true + } + fn execute( + &self, + request: WorkspaceRequest, + ) -> Result { + { + let mut state = self.state.lock().unwrap(); + state.requests += 1; + if request.method == WorkspaceRequestMethod::Post + && !request.path.ends_with("/read") + && !matches!( + self.fault, + Fault::Unavailable | Fault::InvalidPath | Fault::Rejected(_) + ) + { + state.mutations += 1; + state.saved = request.body.clone(); + } + self.started.notify_one(); + if matches!(self.fault, Fault::Deadline) { + let (_state, timeout) = self + .changed + .wait_timeout_while(state, Duration::from_secs(5), |s| !s.released) + .unwrap(); + assert!(!timeout.timed_out(), "test must release blocked transport"); + } + } + self.finish(); + match self.fault { + Fault::Unavailable => Err(WorkspaceClientError::Unavailable(UNSAFE.into())), + Fault::InvalidPath => Err(WorkspaceClientError::InvalidPath(UNSAFE.into())), + Fault::Communication => Err(WorkspaceClientError::Request(UNSAFE.into())), + Fault::Panic => panic!("injected blocking task failure"), + Fault::Decode => Ok(WorkspaceResponse { + status: 200, + body: UNSAFE.into(), + }), + Fault::Size => Ok(WorkspaceResponse { + status: 200, + body: UNSAFE.repeat(MAX_RESPONSE_BYTES / UNSAFE.len() + 1), + }), + Fault::Rejected(_) | Fault::Unknown => { + let status = match self.fault { + Fault::Rejected(status) => status, + _ => 503, + }; + Ok(WorkspaceResponse { + status, + body: serde_json::to_string(&WorkspaceConfigApiError { + status, + code: UNSAFE.into(), + message: UNSAFE.into(), + classification: if matches!(self.fault, Fault::Unknown) { + WorkspaceConfigFailureClassification::Unknown + } else { + WorkspaceConfigFailureClassification::NotCommitted + }, + }) + .unwrap(), + }) + } + Fault::Unrecognized => Ok(WorkspaceResponse { + status: 503, + body: UNSAFE.into(), + }), + Fault::Identity | Fault::Deadline => { + let mut value = attachment(); + if matches!(self.fault, Fault::Identity) { + value.workspace_id = UNSAFE.into(); + } + Ok(WorkspaceResponse { + status: 200, + body: serde_json::to_string(&value).unwrap(), + }) + } + } + } +} + +fn assert_safe(capture: &Capture, reason: Reason, stage: Stage, public: &str) { + let text = capture.text(); + assert!( + text.contains(&format!("reason={reason:?}")), + "missing reason: {text}" + ); + assert!( + text.contains(&format!("stage={stage:?}")), + "missing stage: {text}" + ); + assert!(text.contains("adapter=WorkspaceConfig"), "{text}"); + assert!(text.contains("correlation="), "{text}"); + for secret in [ + UNSAFE, + "secret-token", + "/private/config", + "credential=unsafe", + ] { + assert!( + !text.contains(secret), + "diagnostics leaked unsafe boundary data" + ); + assert!( + !public.contains(secret), + "public result leaked unsafe boundary data" + ); + } +} +fn public_error( + result: Result, + code: Option, +) -> String { + match (result, code) { + (Err(WipOperationError::OutcomeUnknown(message)), None) => { + assert!(message.contains("no automatic retry")); + message + } + (Err(WipOperationError::Protocol(error)), Some(expected)) => { + assert_eq!(error.code, expected); + error.message + } + _ => panic!("unexpected public error classification"), + } +} + +#[tokio::test] +async fn production_reply_failures_retain_reasons_without_retrying_mutations() { + let cases = [ + ( + Fault::Unavailable, + Reason::TransportUnavailable, + Stage::Request, + ProtocolErrorCode::Internal, + false, + ), + ( + Fault::InvalidPath, + Reason::InvalidPath, + Stage::Request, + ProtocolErrorCode::Internal, + false, + ), + ( + Fault::Communication, + Reason::Communication, + Stage::Request, + ProtocolErrorCode::Internal, + true, + ), + ( + Fault::Decode, + Reason::Decode, + Stage::Response, + ProtocolErrorCode::Internal, + true, + ), + ( + Fault::Size, + Reason::ResponseSize, + Stage::Response, + ProtocolErrorCode::ResourceLimitExceeded, + true, + ), + ( + Fault::Unknown, + Reason::BackendUnknown, + Stage::Response, + ProtocolErrorCode::Internal, + true, + ), + ( + Fault::Unrecognized, + Reason::UnrecognizedResponse, + Stage::Response, + ProtocolErrorCode::Internal, + true, + ), + ( + Fault::Panic, + Reason::TaskJoin, + Stage::Completion, + ProtocolErrorCode::Internal, + true, + ), + ]; + for (fault, reason, stage, read_code, applies) in cases { + for mutation in [false, true] { + let client = Client::new(fault); + let backend = WorkspaceConfigBackend::new(client.clone()); + let capture = Capture::default(); + let result = backend + .post::( + if mutation { "/commit" } else { "/read" }, + &serde_json::json!({"content": UNSAFE}), + mutation, + ) + .with_subscriber(capture.dispatch()) + .await; + let code = if (mutation && !matches!(fault, Fault::InvalidPath)) + || matches!(fault, Fault::Unknown) + { + None + } else { + Some(read_code) + }; + let public = public_error(result, code); + assert_safe(&capture, reason, stage, &public); + let state = client.state.lock().unwrap(); + assert_eq!(state.requests, 1); + assert_eq!(state.mutations, usize::from(mutation && applies)); + assert_eq!(state.saved.is_some(), mutation && applies); + } + } +} + +#[tokio::test] +async fn production_typed_rejections_preserve_public_codes_and_never_apply() { + for (status, code) in [ + (403, ProtocolErrorCode::PermissionDenied), + (409, ProtocolErrorCode::ValidatorMismatch), + (413, ProtocolErrorCode::ResourceLimitExceeded), + (422, ProtocolErrorCode::InvalidArguments), + (500, ProtocolErrorCode::Internal), + ] { + for mutation in [false, true] { + let client = Client::new(Fault::Rejected(status)); + let backend = WorkspaceConfigBackend::new(client.clone()); + let capture = Capture::default(); + let public = public_error( + backend + .post::( + if mutation { "/commit" } else { "/read" }, + &(), + mutation, + ) + .with_subscriber(capture.dispatch()) + .await, + Some(code), + ); + assert_safe(&capture, Reason::BackendRejected, Stage::Response, &public); + assert!(capture.text().contains(&format!("status={status}"))); + let state = client.state.lock().unwrap(); + assert_eq!(state.requests, 1); + assert_eq!(state.mutations, 0); + assert!(state.saved.is_none()); + } + } +} + +#[tokio::test] +async fn production_deadline_is_unknown_after_effect_and_blocking_completion_is_not_retried() { + for mutation in [false, true] { + let client = Client::new(Fault::Deadline); + let release = Release(client.clone()); + let backend = + WorkspaceConfigBackend::new(client.clone()).with_deadline(Duration::from_secs(1)); + let capture = Capture::default(); + let body = serde_json::json!({"content": UNSAFE}); + let request = backend + .post::( + if mutation { "/commit" } else { "/read" }, + &body, + mutation, + ) + .with_subscriber(capture.dispatch()); + tokio::pin!(request); + tokio::select! { + biased; + _ = client.started.notified() => {}, + _ = &mut request => panic!("request must start and apply its effect before timeout is observed"), + } + { + let state = client.state.lock().unwrap(); + assert_eq!(state.mutations, usize::from(mutation)); + assert_eq!(state.saved.is_some(), mutation); + assert!(!state.completed && !state.released); + } + let result = request.await; + let public = public_error( + result, + if mutation { + None + } else { + Some(ProtocolErrorCode::ResourceLimitExceeded) + }, + ); + assert_safe(&capture, Reason::Deadline, Stage::Completion, &public); + drop(release); + let completed = client.clone(); + tokio::task::spawn_blocking(move || completed.wait_completed()) + .await + .unwrap(); + let state = client.state.lock().unwrap(); + assert_eq!(state.requests, 1); + assert_eq!(state.mutations, usize::from(mutation)); + assert_eq!(state.saved.is_some(), mutation); + } +} + +#[tokio::test] +async fn production_current_identity_and_task_loss_remain_safe() { + for (fault, reason, stage, code) in [ + ( + Fault::Identity, + Reason::Identity, + Stage::Response, + ProtocolErrorCode::PermissionDenied, + ), + ( + Fault::Panic, + Reason::TaskJoin, + Stage::Completion, + ProtocolErrorCode::Internal, + ), + ( + Fault::Decode, + Reason::Decode, + Stage::Response, + ProtocolErrorCode::Internal, + ), + ] { + let client = Client::new(fault); + let backend = WorkspaceConfigBackend::new(client.clone()); + let capture = Capture::default(); + let result = backend + .current_async() + .with_subscriber(capture.dispatch()) + .await; + let message = match result { + Err(WipOperationError::Protocol(error)) => { + assert_eq!(error.code, code); + error.message + } + _ => panic!("unexpected current classification"), + }; + assert_safe(&capture, reason, stage, &message); + assert_eq!(client.state.lock().unwrap().mutations, 0); + } +} + +#[tokio::test] +async fn production_attach_conversion_retains_original_reason_and_effect_count() { + for (fault, reason, expected, mutations) in [ + ( + Fault::Communication, + Reason::Communication, + ConfigAttachError::OutcomeUnknown, + 1, + ), + ( + Fault::Identity, + Reason::Identity, + ConfigAttachError::OutcomeUnknown, + 1, + ), + ( + Fault::Rejected(403), + Reason::BackendRejected, + ConfigAttachError::Denied, + 0, + ), + ( + Fault::Rejected(422), + Reason::BackendRejected, + ConfigAttachError::InvalidRequest, + 0, + ), + ( + Fault::Rejected(500), + Reason::BackendRejected, + ConfigAttachError::Unavailable, + 0, + ), + ] { + let client = Client::new(fault); + let backend = WorkspaceConfigBackend::new(client.clone()); + let capture = Capture::default(); + assert_eq!( + backend + .attach("host-key-not-logged", None) + .with_subscriber(capture.dispatch()) + .await, + Err(expected) + ); + assert_safe(&capture, reason, Stage::Attach, expected.message()); + let text = capture.text(); + if !matches!(fault, Fault::Identity) { + assert!(text.contains("reason=AttachConversion"), "{text}"); + } + assert!(!text.contains("host-key-not-logged")); + let correlations: std::collections::BTreeSet<_> = text + .lines() + .filter_map(|line| line.split("correlation=").nth(1)) + .filter_map(|tail| tail.split(';').next()) + .collect(); + assert_eq!( + correlations.len(), + 1, + "request and attach conversion must correlate" + ); + let state = client.state.lock().unwrap(); + assert_eq!(state.requests, 1); + assert_eq!(state.mutations, mutations); + assert_eq!(state.saved.is_some(), mutations == 1); + } +} + +#[tokio::test] +async fn production_oversized_request_is_rejected_before_dispatch() { + let client = Client::new(Fault::Communication); + let backend = WorkspaceConfigBackend::new(client.clone()); + let capture = Capture::default(); + let public = public_error( + backend + .post::("/commit", &"x".repeat(MAX_RESPONSE_BYTES), true) + .with_subscriber(capture.dispatch()) + .await, + Some(ProtocolErrorCode::ResourceLimitExceeded), + ); + assert_safe(&capture, Reason::RequestSize, Stage::Request, &public); + let state = client.state.lock().unwrap(); + assert_eq!(state.requests, 0); + assert_eq!(state.mutations, 0); +} + +#[tokio::test] +async fn production_current_deadline_remains_bounded_and_has_no_mutation() { + let client = Client::new(Fault::Deadline); + let release = Release(client.clone()); + let backend = WorkspaceConfigBackend::new(client.clone()).with_deadline(Duration::from_secs(1)); + let capture = Capture::default(); + let request = backend.current_async().with_subscriber(capture.dispatch()); + tokio::pin!(request); + tokio::select! { + biased; + _ = client.started.notified() => {}, + _ = &mut request => panic!("current request must enter the gate before timeout is observed"), + } + assert_eq!(client.state.lock().unwrap().mutations, 0); + let error = request.await; + let message = match error { + Err(WipOperationError::Protocol(error)) => { + assert_eq!(error.code, ProtocolErrorCode::ResourceLimitExceeded); + error.message + } + _ => panic!("current deadline must remain a public resource-limit error"), + }; + assert_safe(&capture, Reason::Deadline, Stage::Completion, &message); + drop(release); + let completed = client.clone(); + tokio::task::spawn_blocking(move || completed.wait_completed()) + .await + .unwrap(); + let state = client.state.lock().unwrap(); + assert_eq!(state.requests, 1); + assert_eq!(state.mutations, 0); +} + +#[tokio::test] +async fn production_request_encoding_failure_is_safe_and_never_dispatched() { + struct Unencodable; + impl Serialize for Unencodable { + fn serialize(&self, _: S) -> Result { + Err(serde::ser::Error::custom(UNSAFE)) + } + } + let client = Client::new(Fault::Communication); + let backend = WorkspaceConfigBackend::new(client.clone()); + let capture = Capture::default(); + let result = backend + .post::("/commit", &Unencodable, true) + .with_subscriber(capture.dispatch()) + .await; + let message = public_error(result, Some(ProtocolErrorCode::InvalidArguments)); + assert_safe(&capture, Reason::Decode, Stage::Request, &message); + let state = client.state.lock().unwrap(); + assert_eq!(state.requests, 0); + assert_eq!(state.mutations, 0); +} diff --git a/crates/worker/src/feature/builtin/workspace_config/content.rs b/crates/worker/src/feature/builtin/workspace_config/content.rs index b2f67d342..ae6c64b41 100644 --- a/crates/worker/src/feature/builtin/workspace_config/content.rs +++ b/crates/worker/src/feature/builtin/workspace_config/content.rs @@ -36,9 +36,11 @@ use wip_protocol::{ }; use super::backend::{ - MAX_DEPTH, MAX_NODES, MAX_PATH_BYTES, MAX_TEXT_BYTES, WorkspaceConfigBackend, failure, unknown, + MAX_DEPTH, MAX_NODES, MAX_PATH_BYTES, MAX_TEXT_BYTES, WorkspaceConfigBackend, diagnostic, + failure, unknown, }; use super::{CONTENT_ROOT, WorkspaceConfigFeature}; +use crate::feature::builtin::backend_diagnostic::{Diagnostic, Reason, Stage}; use crate::wip::{ WipCallContext, WipOperationError, WipOperationHandler, WipOperationOutput, WipProjection, WipProjectionKind, WipSubtreeProvider, contextual_reference, json_to_wip, @@ -138,6 +140,23 @@ async fn observe( connection: &str, paths: Vec, depth: u32, +) -> Result { + observe_diagnosed( + backend, + connection, + paths, + depth, + diagnostic("observe", false), + ) + .await +} + +async fn observe_diagnosed( + backend: &WorkspaceConfigBackend, + connection: &str, + paths: Vec, + depth: u32, + diagnostic: Diagnostic, ) -> Result { if paths.len() > MAX_NODES || depth > MAX_DEPTH { return Err(invalid()); @@ -148,7 +167,7 @@ async fn observe( } } let result: WorkspaceConfigObserveResponse = backend - .post( + .post_diagnosed( "/observe", &WorkspaceConfigObserveRequest { connection_id: connection.into(), @@ -156,6 +175,7 @@ async fn observe( depth, }, false, + diagnostic.clone(), ) .await?; if result.connection_id != connection @@ -164,24 +184,37 @@ async fn observe( || result.validator.len() > 4096 || result.digest.is_empty() { + diagnostic.record( + Stage::Response, + if result.connection_id != connection { + Reason::Identity + } else { + Reason::InvalidMetadata + }, + None, + ); return Err(failure( ProtocolErrorCode::Internal, "Invalid Workspace config metadata", )); } if result.entrypoints.is_empty() || result.entrypoints.len() > MAX_NODES { + diagnostic.record(Stage::Response, Reason::InvalidMetadata, None); return Err(failure( ProtocolErrorCode::Internal, "Invalid Workspace config entrypoints", )); } for path in &result.entrypoints { - validate_source(path)?; + validate_source(path) + .inspect_err(|_| diagnostic.record(Stage::Response, Reason::InvalidMetadata, None))?; } let mut seen = BTreeSet::new(); for node in &result.nodes { if !node.path.is_empty() { - validate_source(&node.path)?; + validate_source(&node.path).inspect_err(|_| { + diagnostic.record(Stage::Response, Reason::InvalidMetadata, None) + })?; } if !seen.insert(&node.path) || node.validator.is_empty() @@ -198,6 +231,7 @@ async fn observe( suffix.is_some_and(|s| s.split('/').count() <= depth as usize) }) { + diagnostic.record(Stage::Response, Reason::InvalidMetadata, None); return Err(failure( ProtocolErrorCode::Internal, "Invalid Workspace config metadata scope", @@ -490,9 +524,10 @@ fn content_type(value: Option<&Value>) -> Result Result { + let diagnostic = diagnostic("read", false); let response: WorkspaceConfigReadResponse = self .backend - .post( + .post_diagnosed( "/read", &WorkspaceConfigReadRequest { connection_id: self.attachment.connection_id.clone(), @@ -500,6 +535,7 @@ impl ConfigHandler { validator: self.node.validator.clone(), }, false, + diagnostic.clone(), ) .await?; if response.path != self.node.path @@ -507,12 +543,14 @@ impl ConfigHandler { || Some(&response.digest) != self.node.digest.as_ref() || Some(&response.content_type) != self.node.content_type.as_ref() { + diagnostic.record(Stage::Response, Reason::Identity, None); return Err(failure( ProtocolErrorCode::Internal, "Invalid Workspace config read response", )); } if response.content.len() > MAX_TEXT_BYTES { + diagnostic.record(Stage::Response, Reason::ResponseSize, None); return Err(failure( ProtocolErrorCode::ResourceLimitExceeded, "Workspace config source exceeds text limit", @@ -630,12 +668,17 @@ impl WipOperationHandler for ConfigHandler { context: WipCallContext, ) -> Result { let dispatched = std::sync::atomic::AtomicBool::new(false); + let diagnostic = diagnostic( + "operation", + !matches!(operation, "read" | "read_lines" | "search_text"), + ); tokio::time::timeout( super::DEADLINE, - self.execute(operation, arguments, context, &dispatched), + self.execute(operation, arguments, context, &dispatched, &diagnostic), ) .await .map_err(|_| { + diagnostic.record(Stage::Completion, Reason::Deadline, None); if dispatched.load(std::sync::atomic::Ordering::Acquire) { unknown() } else { @@ -655,6 +698,7 @@ impl ConfigHandler { arguments: &BTreeMap, context: WipCallContext, dispatched: &std::sync::atomic::AtomicBool, + operation_diagnostic: &Diagnostic, ) -> Result { if !self.operations.iter().any(|name| name == operation) { return Err(failure( @@ -798,9 +842,10 @@ impl ConfigHandler { }; let changes = self.canonical_changes(changes).await?; dispatched.store(true, std::sync::atomic::Ordering::Release); + let diagnostic = operation_diagnostic.clone(); let result: WorkspaceConfigCommitResponse = self .backend - .post( + .post_diagnosed( "/commit", &WorkspaceConfigCommitRequest { connection_id: self.attachment.connection_id.clone(), @@ -812,28 +857,37 @@ impl ConfigHandler { }, }, true, + diagnostic.clone(), ) .await?; // Commit response supplies ROOT state. Obtain the exact post-commit PATH // validator, never substitute the root validator for a file validator. - let updated = observe( + let updated = observe_diagnosed( &self.backend, &self.attachment.connection_id, vec![self.node.path.clone()], 0, + diagnostic.at_stage(Stage::Reconcile), ) .await .map_err(|_| unknown())?; if updated.digest != result.digest || updated.validator != result.validator { + diagnostic.record(Stage::Reconcile, Reason::Identity, None); return Err(unknown()); } let node = updated .nodes .iter() .find(|n| n.path == self.node.path) - .ok_or_else(unknown)?; + .ok_or_else(|| { + diagnostic.record(Stage::Reconcile, Reason::InvalidMetadata, None); + unknown() + })?; let value = json_to_wip(&json!({"digest": result.digest, - "activation": "Saved to canonical Workspace configuration; existing Worker manifests are not rewritten"})).map_err(|_| unknown())?; + "activation": "Saved to canonical Workspace configuration; existing Worker manifests are not rewritten"})).map_err(|_| { + diagnostic.record(Stage::Completion, Reason::Decode, None); + unknown() + })?; Ok(WipOperationOutput::native_with_validator( value, object_validator( diff --git a/crates/worker/src/feature/builtin/workspace_config/production_tests.rs b/crates/worker/src/feature/builtin/workspace_config/production_tests.rs index 71649555c..27ee6f161 100644 --- a/crates/worker/src/feature/builtin/workspace_config/production_tests.rs +++ b/crates/worker/src/feature/builtin/workspace_config/production_tests.rs @@ -1,8 +1,10 @@ -//! Worker-side E2E across real Feature invocation, WIP Client/Host and typed -//! Workspace transport. Canonical persistence/projection tests live in Backend; -//! this deterministic router models its published auth/CAS/response contract. +//! Worker-side integration across real Feature invocation, WIP Client/Host and the production +//! WorkspaceConfigBackend. Only WorkspaceClient transport is substituted by a deterministic router +//! retaining applied effects and modeling the published auth/CAS/response contract. These ordinary +//! worker lib tests do not replace canonical persistence/projection tests owned by Backend. use super::*; use crate::feature::FeatureRegistryBuilder; +use crate::feature::builtin::backend_diagnostic::tests::Capture; use crate::hook::HookRegistryBuilder; use crate::wip::{WipMountRegistry, WipRuntime}; use crate::worker::{ @@ -16,6 +18,7 @@ use serde_json::{Value as Json, json}; use server_api::*; use std::collections::{BTreeMap, BTreeSet}; use std::sync::{Arc, Mutex}; +use tracing::instrument::WithSubscriber; #[derive(Debug)] struct State { @@ -26,6 +29,10 @@ struct State { entries: BTreeMap, commits: Vec, requests: Vec, + applied_commits: usize, + malformed_read_reply: Option<&'static str>, + malformed_observe_reply: Option<&'static str>, + lose_postcommit_observe_reply: bool, fail_commit: bool, fail_current: bool, lose_commit_reply: bool, @@ -58,6 +65,10 @@ impl Router { ]), commits: Vec::new(), requests: Vec::new(), + applied_commits: 0, + malformed_read_reply: None, + malformed_observe_reply: None, + lose_postcommit_observe_reply: false, fail_commit: false, fail_current: false, lose_commit_reply: false, @@ -222,6 +233,11 @@ impl WorkspaceClient for Router { } match suffix { "/observe" => { + if state.lose_postcommit_observe_reply && state.applied_commits > 0 { + return Err(WorkspaceClientError::Request( + "secret-token /private/host/config".into(), + )); + } let request: WorkspaceConfigObserveRequest = serde_json::from_value(input).unwrap(); assert!(request.depth <= 8 && request.paths.len() <= 256); let mut kinds = @@ -268,13 +284,23 @@ impl WorkspaceClient for Router { ) }) .collect(); - Ok(response(&WorkspaceConfigObserveResponse { + let mut result = WorkspaceConfigObserveResponse { connection_id: state.active.as_ref().unwrap().connection_id.clone(), validator: state.validator(""), digest: state.tree_digest(), entrypoints: vec!["main.dcdl".into()], nodes, - })) + }; + match state.malformed_observe_reply { + Some("connection") => { + result.connection_id = "secret-token /private/host/config".into() + } + Some("entrypoints") => result.entrypoints.clear(), + Some("scope") => result.nodes[0].path = "outside/path.dcdl".into(), + None => {} + Some(_) => unreachable!(), + } + Ok(response(&result)) } "/read" => { let request: WorkspaceConfigReadRequest = serde_json::from_value(input).unwrap(); @@ -290,13 +316,20 @@ impl WorkspaceClient for Router { WorkspaceConfigFailureClassification::NotCommitted, )); }; - Ok(response(&WorkspaceConfigReadResponse { + let mut result = WorkspaceConfigReadResponse { path: request.path.clone(), content: content.clone(), content_type: *content_type, digest: digest(content), validator: state.validator(&request.path), - })) + }; + match state.malformed_read_reply { + Some("identity") => result.path = "secret-token /private/host/config".into(), + Some("size") => result.content = "x".repeat(256 * 1024 + 1), + None => {} + Some(_) => unreachable!(), + } + Ok(response(&result)) } "/commit" => { let request: WorkspaceConfigCommitRequest = serde_json::from_value(input).unwrap(); @@ -403,6 +436,7 @@ impl WorkspaceClient for Router { )); } state.entries = entries; + state.applied_commits += 1; if state.lose_commit_reply { state.lose_commit_reply = false; return Err(WorkspaceClientError::Request( @@ -797,14 +831,20 @@ async fn post_commit_observation_mismatch_reports_unknown_without_replaying_writ let path = "/workspace-config/main.dcdl"; observe_interface(&runtime, path, true).await; client.state.lock().unwrap().mismatched_commit_reply = Some(field); + let capture = Capture::default(); let error = call(&runtime, path, "write", json!({"content":"saved"})) + .with_subscriber(capture.dispatch()) .await .unwrap_err() .to_string(); assert!(error.contains("unknown"), "{field}: {error}"); + assert!(capture.text().contains("stage=Reconcile")); + assert!(capture.text().contains("reason=Identity")); + assert!(!capture.text().contains("saved")); let state = client.state.lock().unwrap(); assert_eq!(state.commits.len(), 1); assert_eq!(state.entries["main.dcdl"].1, "saved"); + assert_eq!(state.applied_commits, 1); } } @@ -1155,3 +1195,110 @@ async fn workspace_config_backend_validators_reject_path_and_connection_transpla assert!(result.content.unwrap().contains("model = updated")); assert_eq!(client.state.lock().unwrap().commits.len(), 1); } + +#[tokio::test] +async fn production_read_validation_retains_reason_without_committing() { + for (fault, reason) in [("identity", "Identity"), ("size", "ResponseSize")] { + let client = Router::new(); + let (feature, runtime) = runtime(client.clone()); + feature.attach("initial", None).await.unwrap(); + let path = "/workspace-config/main.dcdl"; + observe_interface(&runtime, path, true).await; + client.state.lock().unwrap().malformed_read_reply = Some(fault); + let capture = Capture::default(); + let error = call(&runtime, path, "read", json!({})) + .with_subscriber(capture.dispatch()) + .await + .unwrap_err() + .to_string(); + let text = capture.text(); + assert!(text.contains(&format!("reason={reason}")), "{text}"); + assert!(text.contains("stage=Response")); + assert!(!error.contains("unknown")); + assert!(!text.contains("secret-token") && !text.contains("/private/host")); + assert!(!error.contains("secret-token") && !error.contains("/private/host")); + let state = client.state.lock().unwrap(); + assert_eq!(state.applied_commits, 0); + assert_eq!(state.entries["main.dcdl"].1, "model = old"); + } +} + +#[tokio::test] +async fn production_observation_validation_retains_reason_without_committing() { + for (fault, reason) in [ + ("connection", "Identity"), + ("entrypoints", "InvalidMetadata"), + ("scope", "InvalidMetadata"), + ] { + let client = Router::new(); + let (feature, runtime) = runtime(client.clone()); + feature.attach("initial", None).await.unwrap(); + client.state.lock().unwrap().malformed_observe_reply = Some(fault); + let capture = Capture::default(); + let error = runtime + .tree("/workspace-config/main.dcdl".into(), 0, true) + .with_subscriber(capture.dispatch()) + .await + .unwrap_err() + .to_string(); + let text = capture.text(); + assert!(text.contains(&format!("reason={reason}")), "{text}"); + assert!(text.contains("stage=Response")); + assert!(!text.contains("secret-token") && !text.contains("/private/host")); + assert!(!error.contains("secret-token") && !error.contains("/private/host")); + assert_eq!(client.state.lock().unwrap().applied_commits, 0); + } +} + +#[tokio::test] +async fn production_postcommit_transport_loss_retains_cause_and_unknown_without_retry() { + let client = Router::new(); + let (feature, runtime) = runtime(client.clone()); + feature.attach("initial", None).await.unwrap(); + let path = "/workspace-config/main.dcdl"; + observe_interface(&runtime, path, true).await; + client.state.lock().unwrap().lose_postcommit_observe_reply = true; + let capture = Capture::default(); + let error = call( + &runtime, + path, + "write", + json!({"content":"saved-before-observation-loss"}), + ) + .with_subscriber(capture.dispatch()) + .await + .unwrap_err() + .to_string(); + assert!(error.contains("unknown")); + let text = capture.text(); + assert!( + text.lines().any( + |event| event.contains("reason=Communication") && event.contains("stage=Reconcile") + ), + "{text}" + ); + assert!( + !text.contains("reason=UnrecognizedResponse"), + "must not invent a response-decode cause for transport loss" + ); + assert!(!text.contains("secret-token") && !text.contains("/private/host")); + assert!(!text.contains("saved-before-observation-loss")); + assert!(!error.contains("secret-token") && !error.contains("/private/host")); + let correlations: BTreeSet<_> = text + .lines() + .filter_map(|line| line.split("correlation=").nth(1)) + .filter_map(|tail| tail.split(';').next()) + .collect(); + assert_eq!( + correlations.len(), + 1, + "lost postcommit observation must correlate to original commit" + ); + let state = client.state.lock().unwrap(); + assert_eq!(state.commits.len(), 1); + assert_eq!(state.applied_commits, 1); + assert_eq!( + state.entries["main.dcdl"].1, + "saved-before-observation-loss" + ); +} diff --git a/crates/worker/src/feature/builtin/workspace_config/wip.rs b/crates/worker/src/feature/builtin/workspace_config/wip.rs index e03c90db3..0f77f854c 100644 --- a/crates/worker/src/feature/builtin/workspace_config/wip.rs +++ b/crates/worker/src/feature/builtin/workspace_config/wip.rs @@ -161,7 +161,14 @@ impl WipOperationHandler for AttachHandler { "attachment_path": format!("/workdir-attachments/{}", encode_identity(&result.connection_id)), "content_path": CONTENT_ROOT, "already_attached": result.already_attached, - })).map_err(|_| WipOperationError::OutcomeUnknown("Completed config attachment response unavailable".into()))?; + })).map_err(|_| { + super::backend::diagnostic("attach_output", true).record( + crate::feature::builtin::backend_diagnostic::Stage::Attach, + crate::feature::builtin::backend_diagnostic::Reason::Decode, + None, + ); + WipOperationError::OutcomeUnknown("Completed config attachment response unavailable".into()) + })?; Ok(WipOperationOutput::native(value)) } } From 28b7cc414bb66d065cded13ccd75f2997fd43352 Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 06:04:39 +0900 Subject: [PATCH 2/8] fix: retain external workdir results independently of audit failures --- crates/server-api/src/lib.rs | 8 +- crates/tools/src/file_target.rs | 1 + crates/workdir/src/http.rs | 89 ++- crates/workdir/src/lib.rs | 6 + .../src/feature/builtin/manage_workdir.rs | 100 +++- crates/workspace-server/src/server.rs | 176 ++++-- .../src/server/external_operation_audit.rs | 36 ++ .../server/tests/workspace_admission_tests.rs | 8 +- .../workspace_admission_tests/audit_tests.rs | 546 ++++++++++++++++++ 9 files changed, 907 insertions(+), 63 deletions(-) create mode 100644 crates/workspace-server/src/server/external_operation_audit.rs create mode 100644 crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs diff --git a/crates/server-api/src/lib.rs b/crates/server-api/src/lib.rs index 3b448d409..951a21566 100644 --- a/crates/server-api/src/lib.rs +++ b/crates/server-api/src/lib.rs @@ -125,6 +125,8 @@ pub struct WorkdirOperationApiError { pub message: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub diagnostics: Option>, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub audit_failures: Vec, #[serde(skip, default = "default_repository_error_status")] status: u16, /// Server-local metadata retained by generated error response extensions, never the wire body. @@ -156,6 +158,7 @@ impl WorkdirOperationApiError { code: None, message: error.message, diagnostics: Some(error.diagnostics), + audit_failures: Vec::new(), status: error.status, workdir_log: None, denial_reason: None, @@ -170,6 +173,7 @@ impl WorkdirOperationApiError { code: Some(error.code), message: error.message, diagnostics: None, + audit_failures: Vec::new(), status, workdir_log: None, denial_reason: error.denial_reason, @@ -7522,7 +7526,9 @@ pub struct CurrentWorkerWorkdirOperationRequest { #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, JsonSchema)] #[serde(transparent)] -pub struct CurrentWorkerWorkdirOperationResponse(pub workdir::http::WorkdirSessionOperationResult); +pub struct CurrentWorkerWorkdirOperationResponse( + pub workdir::http::WorkdirSessionOperationResponse, +); #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, JsonSchema)] #[cfg_attr(feature = "typescript", derive(ts_rs::TS))] diff --git a/crates/tools/src/file_target.rs b/crates/tools/src/file_target.rs index c3bfb535e..af307e0cc 100644 --- a/crates/tools/src/file_target.rs +++ b/crates/tools/src/file_target.rs @@ -167,6 +167,7 @@ pub(crate) fn checked_error(error: workdir::WorkdirError) -> ToolError { | WorkdirError::IsDirectory { .. } | WorkdirError::InvalidGlob { .. } | WorkdirError::InvalidRegex { .. } => "checkout_invalid", + WorkdirError::NotDispatched => "checkout_not_dispatched", WorkdirError::Unavailable { .. } | WorkdirError::SessionClosed => "checkout_unavailable", // IO/transport failures may arrive after effects have begun. Do not infer // no-effects from their text or strip explicit OutcomeUnknown evidence. diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index f99f804c5..805aea2de 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -119,6 +119,51 @@ pub enum WorkdirSessionOperationResult { CommandCancel, } +/// Audit persistence is independent of the provider result and never authorizes a resend. +/// Only server-generated correlation and closed classifications cross this boundary. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct WorkdirAuditFailure { + pub operation_id: String, + pub stage: WorkdirAuditStage, + pub operation_outcome: WorkdirAuditOperationOutcome, + pub reason: WorkdirAuditFailureReason, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum WorkdirAuditStage { + BeforeDispatch, + DispatchRejected, + ResponseReceived, + ResponseUnknown, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] +pub enum WorkdirAuditOperationOutcome { + NotDispatched, + Succeeded, + Failed { code: WorkdirTransportErrorCode }, + Unknown, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum WorkdirAuditFailureReason { + AuditStoreUnavailable, +} + +/// A successful operation can still have failed audit persistence. Consumers must +/// surface that degradation separately, without discarding the result or resending. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +pub struct WorkdirSessionOperationResponse { + #[serde(flatten)] + pub result: WorkdirSessionOperationResult, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub audit_failures: Vec, +} + /// Dispatch one typed operation against any provider-neutral Workdir session. /// /// Runtime HTTP handlers, outbound External providers, and Backend brokers can @@ -208,6 +253,7 @@ pub enum WorkdirTransportErrorCode { NotFound, Conflict, OutcomeUnknown, + NotDispatched, Unsupported, InvalidRequest, Denied, @@ -231,6 +277,7 @@ impl WorkdirTransportErrorCode { Self::NotFound => "not_found", Self::Conflict => "conflict", Self::OutcomeUnknown => "outcome_unknown", + Self::NotDispatched => "not_dispatched", Self::Unsupported => "unsupported", Self::InvalidRequest => "invalid_request", Self::Denied => "denied", @@ -261,7 +308,7 @@ impl WorkdirTransportErrorCode { | Self::SymlinkTargetIsDirectory | Self::IsDirectory | Self::SymlinkDirectoryNotTraversed => 400, - Self::Unavailable => 503, + Self::Unavailable | Self::NotDispatched => 503, Self::Io | Self::Internal | Self::OutcomeUnknown => 500, Self::Transport => 502, } @@ -298,6 +345,9 @@ impl WorkdirTransportError { Code::OutcomeUnknown, "Workdir operation may have effects; inspect before retrying", ), + WorkdirError::NotDispatched => { + (Code::NotDispatched, "Workdir operation was not dispatched") + } WorkdirError::NotFound(_) => (Code::NotFound, "Workdir path was not found"), WorkdirError::Conflict(_) => ( Code::Conflict, @@ -410,6 +460,7 @@ impl WorkdirTransportError { }, Code::UnknownCommand => WorkdirError::UnknownCommand("".to_string()), Code::Unavailable => WorkdirError::Unavailable(self.message), + Code::NotDispatched => WorkdirError::NotDispatched, Code::Io => WorkdirError::Io { path: "".into(), source: std::io::Error::other(self.message), @@ -1210,6 +1261,37 @@ pub use client::{ mod tests { use super::*; + #[test] + fn operation_response_keeps_success_and_independent_audit_failure() { + let original = + WorkdirSessionOperationResult::CommandStart(crate::CommandHandle("command-a".into())); + let response = WorkdirSessionOperationResponse { + result: original.clone(), + audit_failures: vec![WorkdirAuditFailure { + operation_id: "op-00000000-0000-7000-8000-000000000001".into(), + stage: WorkdirAuditStage::ResponseReceived, + operation_outcome: WorkdirAuditOperationOutcome::Succeeded, + reason: WorkdirAuditFailureReason::AuditStoreUnavailable, + }], + }; + let json = serde_json::to_value(&response).unwrap(); + assert_eq!(json["operation"], "command_start"); + assert_eq!(json["result"], "command-a"); + assert_eq!( + json["audit_failures"][0]["operation_outcome"]["kind"], + "succeeded" + ); + assert_eq!( + serde_json::from_value::(json).unwrap(), + response + ); + let normal = serde_json::to_value(&original).unwrap(); + let decoded: WorkdirSessionOperationResponse = + serde_json::from_value(normal.clone()).unwrap(); + assert!(decoded.audit_failures.is_empty()); + assert_eq!(serde_json::to_value(decoded).unwrap(), normal); + } + #[test] fn transport_error_round_trip_keeps_public_classification() { for (code, expected_status, expected_error) in [ @@ -1229,6 +1311,11 @@ mod tests { 409, "The target file's content or existence changed since it was last observed", ), + ( + WorkdirTransportErrorCode::NotDispatched, + 503, + "not dispatched", + ), (WorkdirTransportErrorCode::Unsupported, 400, "unsupported"), (WorkdirTransportErrorCode::Denied, 403, "denied"), ( diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index 1a515ac16..c208253f0 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -409,6 +409,7 @@ pub enum WorkdirFailureReason { SessionClosed, ProviderUnavailable, CommandTerminalMissing, + AuditStoreUnavailable, HttpTimeout, HttpConnect, HttpTransfer, @@ -422,6 +423,7 @@ impl WorkdirFailureReason { Self::SessionClosed => "session_closed", Self::ProviderUnavailable => "provider_unavailable", Self::CommandTerminalMissing => "command_terminal_missing", + Self::AuditStoreUnavailable => "audit_store_unavailable", Self::HttpTimeout => "http_timeout", Self::HttpConnect => "http_connect", Self::HttpTransfer => "http_transfer", @@ -466,6 +468,10 @@ pub enum WorkdirError { #[error("Workdir session is unavailable: {0}")] Unavailable(String), + /// The broker has proof that this operation never reached the provider. + #[error("Workdir operation was not dispatched")] + NotDispatched, + #[error("Workdir operation failed")] OperationFailed, diff --git a/crates/worker/src/feature/builtin/manage_workdir.rs b/crates/worker/src/feature/builtin/manage_workdir.rs index 12cfdb65d..d8f2302f2 100644 --- a/crates/worker/src/feature/builtin/manage_workdir.rs +++ b/crates/worker/src/feature/builtin/manage_workdir.rs @@ -291,6 +291,11 @@ impl WorkspaceAttachedWorkdirSession { .execute(request) .map_err(workspace_workdir_error)?; if !response.is_success() { + if let Ok(error) = + serde_json::from_str::(&response.body) + { + notify_workdir_audit_failures(&error.audit_failures); + } return Err( serde_json::from_str::(&response.body) .map(workdir::http::WorkdirTransportError::into_workdir_error) @@ -302,11 +307,14 @@ impl WorkspaceAttachedWorkdirSession { }), ); } - serde_json::from_str(&response.body).map_err(|error| { - WorkdirError::Transport(format!( - "failed to decode Workspace Workdir operation result: {error}" - )) - }) + let response: workdir::http::WorkdirSessionOperationResponse = + serde_json::from_str(&response.body).map_err(|_| { + WorkdirError::Transport( + "failed to decode Workspace Workdir operation result".into(), + ) + })?; + notify_workdir_audit_failures(&response.audit_failures); + Ok(response.result) } fn mismatch(expected: &str) -> WorkdirError { @@ -316,6 +324,27 @@ impl WorkspaceAttachedWorkdirSession { } } +// Notification is separate from the operation return: callers retain successful command +// handles, checkout observations and original failures, and must not resend for audit loss. +fn notify_workdir_audit_failures(failures: &[workdir::http::WorkdirAuditFailure]) { + use sha2::Digest; + for failure in failures { + // Response strings are untrusted. Never copy them into the receiving process log. + let operation_id_hash = sha2::Sha256::digest(failure.operation_id.as_bytes()) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect::(); + tracing::error!( + event = "external_workdir_audit_failed", + operation_id_hash, + audit_stage = ?failure.stage, + audit_reason = ?failure.reason, + operation_outcome = ?failure.operation_outcome, + "External Workdir audit persistence failed; operation result is independent; do not resend" + ); + } +} + fn workspace_workdir_error(error: WorkspaceClientError) -> WorkdirError { match error { WorkspaceClientError::Unavailable(message) => WorkdirError::Unavailable(message), @@ -1653,6 +1682,67 @@ mod tests { } } + #[tokio::test] + async fn attached_session_keeps_operation_results_when_audit_persistence_failed() { + let failure = json!({ + "operation_id":"op-safe-id", "stage":"response_received", + "reason":"audit_store_unavailable", "operation_outcome":{"kind":"succeeded"} + }); + let mut responses = vec![response(json!({ + "operation":"command_start", "result":"retained-command", + "audit_failures":[failure] + }))]; + for (code, status, outcome) in [ + ("denied", 403, json!({"kind":"failed","code":"denied"})), + ("outcome_unknown", 500, json!({"kind":"unknown"})), + ("not_dispatched", 503, json!({"kind":"not_dispatched"})), + ] { + let mut failure = failure.clone(); + failure["operation_outcome"] = outcome; + responses.push(WorkspaceResponse { status, body: json!({ + "code":code, "message":"safe provider classification", "audit_failures":[failure] + }).to_string() }); + } + let client = Arc::new(RecordingWorkspaceClient::new(responses)); + let session = WorkspaceAttachedWorkdirSession::handle(client.clone(), "checkout"); + let handle = session + .start_command(CommandRequest { + command: "true".into(), + timeout_secs: 1, + transport_byte_budget: workdir::MAX_COMMAND_TRANSPORT_BYTES, + cwd: workdir::WorkdirPath::root(), + tool_call_id: None, + }) + .await + .unwrap(); + assert_eq!(handle.0, "retained-command"); + let request = workdir::CheckoutRequest { + target: workdir::WorkdirPath::new("file.txt").unwrap(), + validator: vec![1], + operation: workdir::CheckoutOperation::Write { + content: b"new".to_vec(), + expected_hash: [0; 32], + }, + }; + assert!(matches!( + session.checkout_execute(request.clone()).await, + Err(WorkdirError::Denied(_)) + )); + assert!(matches!( + session.checkout_execute(request.clone()).await, + Err(WorkdirError::OutcomeUnknown(_)) + )); + assert!(matches!( + session.checkout_execute(request).await, + Err(WorkdirError::NotDispatched) + )); + assert_eq!( + client.requests().len(), + 4, + "audit loss must not trigger a resend" + ); + } + #[tokio::test] async fn attached_session_does_not_send_worker_local_bash_spill_path() { let client = Arc::new(RecordingWorkspaceClient::new(vec![response(json!({ diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 76d394da4..69a1b035d 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -26,6 +26,7 @@ use subjektiv::api::*; mod backend_job_profile; +mod external_operation_audit; mod subject_authorization; mod subjektiv_job; mod subjektiv_journal; @@ -933,6 +934,10 @@ struct ExternalProviderAuditContext { /// output confirmation from the old connection; unconfirmed cleanup is not reported as success. /// Only confirmed provider shutdown can replace per-command confirmation. Reconnection does /// not prove that missing commands have stopped. +/// +/// Required request auditing fails closed before dispatch. Completion audit failure cannot +/// undo a provider result: it is emitted independently and retained in the current API +/// response, including successful command handles. Audit loss never triggers a resend. struct ExternalProviderWorkdirSession { workdir: workdir::Workdir, connection: Arc, @@ -991,18 +996,22 @@ impl ExternalProviderWorkdirSession { async fn audit_operation( &self, operation_id: &ExternalWorkdirOperationId, - operation_kind: &str, + operation_kind: &'static str, outcome: &str, - ) -> std::result::Result<(), workdir::WorkdirError> { + stage: workdir::http::WorkdirAuditStage, + operation_outcome: workdir::http::WorkdirAuditOperationOutcome, + ) -> std::result::Result<(), workdir::http::WorkdirAuditFailure> { let Some(audit) = &self.audit else { return Ok(()); }; let audit = audit.clone(); + let operation_id_for_notification = operation_id.as_str().to_owned(); + let operation_kind_for_notification = operation_kind; let operation_id = operation_id.clone(); let operation_kind = operation_kind.to_owned(); let outcome = outcome.to_owned(); let connection = self.connection.clone(); - audit + let result = audit .store .db_executor() .run(move || { @@ -1024,12 +1033,17 @@ impl ExternalProviderWorkdirSession { &Utc::now().to_rfc3339_opts(SecondsFormat::Nanos, true), ) }) - .await - .map_err(|error| { - workdir::WorkdirError::Unavailable(format!( - "External Workdir operation audit failed: {error}" - )) - }) + .await; + result.map_err(|_| { + let failure = workdir::http::WorkdirAuditFailure { + operation_id: operation_id_for_notification, + stage, + operation_outcome, + reason: workdir::http::WorkdirAuditFailureReason::AuditStoreUnavailable, + }; + external_operation_audit::notify(failure.clone(), operation_kind_for_notification); + failure + }) } async fn operate( @@ -1079,8 +1093,19 @@ impl ExternalProviderWorkdirSession { } let operation_id = ExternalWorkdirOperationId::new(format!("op-{}", Uuid::now_v7())) .map_err(workdir::WorkdirError::InvalidArgument)?; - self.audit_operation(&operation_id, operation_kind, "requested") - .await?; + use workdir::http::{WorkdirAuditOperationOutcome as Outcome, WorkdirAuditStage as Stage}; + self.audit_operation( + &operation_id, + operation_kind, + "requested", + Stage::BeforeDispatch, + Outcome::NotDispatched, + ) + .await + .map_err(|_| { + workdir::WorkdirError::NotDispatched + .with_failure_reason(workdir::WorkdirFailureReason::AuditStoreUnavailable) + })?; let (response, receive) = tokio::sync::oneshot::channel(); let mut cancel_guard = ExternalOperationCancelGuard { sender: self.connection.sender.clone(), @@ -1097,39 +1122,72 @@ impl ExternalProviderWorkdirSession { .await .is_err() { - self.audit_operation(&operation_id, operation_kind, "provider_unavailable") - .await?; - return Err(workdir::WorkdirError::Unavailable( - "External Workdir provider disconnected".to_string(), - )); + cancel_guard.disarm(); + // A rejected channel send proves there was no provider dispatch. + let _audit_result = self + .audit_operation( + &operation_id, + operation_kind, + "not_dispatched", + Stage::DispatchRejected, + Outcome::NotDispatched, + ) + .await; + return Err(workdir::WorkdirError::NotDispatched + .with_failure_reason(workdir::WorkdirFailureReason::ProviderUnavailable)); } - let result = match tokio::time::timeout(operation_timeout, receive).await { + let (result, stage) = match tokio::time::timeout(operation_timeout, receive).await { Ok(result) => { cancel_guard.disarm(); match result { - Ok(result) => result.map_err(WorkdirTransportError::into_workdir_error), - Err(_) => Err(workdir::WorkdirError::Unavailable( - "External Workdir provider disconnected".to_string(), - )), + Ok(result) => ( + result.map_err(WorkdirTransportError::into_workdir_error), + Stage::ResponseReceived, + ), + Err(_) => ( + Err(workdir::WorkdirError::OutcomeUnknown( + "External Workdir provider response lost; inspect before retrying" + .to_string(), + )), + Stage::ResponseUnknown, + ), } } Err(_) => { cancel_guard.cancel(); - Err(workdir::WorkdirError::Unavailable( - "External Workdir provider operation timed out".to_string(), - )) + ( + Err(workdir::WorkdirError::OutcomeUnknown( + "External Workdir provider response timed out; inspect before retrying" + .to_string(), + )), + Stage::ResponseUnknown, + ) } }; - self.audit_operation( - &operation_id, - operation_kind, - if result.is_ok() { - "completed" - } else { - "failed" + let outcome = match &result { + Ok(_) => Outcome::Succeeded, + Err(error) + if matches!( + error.classification_source(), + workdir::WorkdirError::OutcomeUnknown(_) + ) => + { + Outcome::Unknown + } + Err(error) => Outcome::Failed { + code: WorkdirTransportError::from_workdir_error(error).code, }, - ) - .await?; + }; + let audit_outcome = match outcome { + Outcome::Succeeded => "completed", + Outcome::Failed { .. } => "failed", + Outcome::Unknown => "outcome_unknown", + Outcome::NotDispatched => unreachable!("provider result follows dispatch"), + }; + // Failure is notified independently, never substituted for the provider result. + let _audit_result = self + .audit_operation(&operation_id, operation_kind, audit_outcome, stage, outcome) + .await; result } @@ -13169,26 +13227,38 @@ impl server_api::ServerApi for ServerApiContractService { ), ..Default::default() }; - let Json(response) = scoped_execute_current_worker_workdir_operation( - State( - self.workspace_api() - .map_err(server_api::WorkdirOperationApiError::api)? - .clone(), - ), - AxumPath(ScopedWorkspacePath { workspace_id }), - current_worker_contract_headers(&context).map_err(|error| { - let mut error = server_api::WorkdirOperationApiError::api(error); - error.workdir_log = Some(fallback_log); - error - })?, - Json(WorkspaceWorkdirSessionOperationRequest { - target_workdir: request.target_workdir, - operation: request.operation, - }), - ) - .await - .map_err(InternalWorkdirOperationError::into_contract_error)?; - Ok(server_api::CurrentWorkerWorkdirOperationResponse(response)) + let (result, audit_failures) = + external_operation_audit::capture(scoped_execute_current_worker_workdir_operation( + State( + self.workspace_api() + .map_err(server_api::WorkdirOperationApiError::api)? + .clone(), + ), + AxumPath(ScopedWorkspacePath { workspace_id }), + current_worker_contract_headers(&context).map_err(|error| { + let mut error = server_api::WorkdirOperationApiError::api(error); + error.workdir_log = Some(fallback_log); + error + })?, + Json(WorkspaceWorkdirSessionOperationRequest { + target_workdir: request.target_workdir, + operation: request.operation, + }), + )) + .await; + match result { + Ok(Json(result)) => Ok(server_api::CurrentWorkerWorkdirOperationResponse( + workdir::http::WorkdirSessionOperationResponse { + result, + audit_failures, + }, + )), + Err(error) => { + let mut error = error.into_contract_error(); + error.audit_failures = audit_failures; + Err(error) + } + } } async fn external_workdir_grant_create( diff --git a/crates/workspace-server/src/server/external_operation_audit.rs b/crates/workspace-server/src/server/external_operation_audit.rs new file mode 100644 index 000000000..1b36808d9 --- /dev/null +++ b/crates/workspace-server/src/server/external_operation_audit.rs @@ -0,0 +1,36 @@ +//! Per-request audit notifications, independent of provider result classification. +//! +//! The scope follows the operation future, not a session-wide last-result slot: concurrent +//! requests cannot consume one another's diagnostics. Every failure is also emitted immediately +//! for direct session callers and requests cancelled before a response. This is notification, +//! not a replacement durable audit ledger or crash-recovery/effect-deduplication guarantee. + +use std::{cell::RefCell, future::Future}; +use workdir::http::WorkdirAuditFailure; + +tokio::task_local! { + static FAILURES: RefCell>; +} + +pub(super) async fn capture(future: impl Future) -> (T, Vec) { + FAILURES + .scope(RefCell::new(Vec::new()), async { + let result = future.await; + let failures = FAILURES.with(|failures| failures.take()); + (result, failures) + }) + .await +} + +pub(super) fn notify(failure: WorkdirAuditFailure, operation: &'static str) { + tracing::error!( + event = "external_workdir_audit_failed", + operation, + operation_id = %failure.operation_id, + audit_stage = ?failure.stage, + audit_reason = ?failure.reason, + operation_outcome = ?failure.operation_outcome, + "External Workdir audit persistence failed; operation result is independent; do not resend" + ); + let _ = FAILURES.try_with(|failures| failures.borrow_mut().push(failure)); +} diff --git a/crates/workspace-server/src/server/tests/workspace_admission_tests.rs b/crates/workspace-server/src/server/tests/workspace_admission_tests.rs index 877ce5dff..facd8fe80 100644 --- a/crates/workspace-server/src/server/tests/workspace_admission_tests.rs +++ b/crates/workspace-server/src/server/tests/workspace_admission_tests.rs @@ -1,3 +1,5 @@ +mod audit_tests; + use super::*; use sha2::Digest; use std::sync::atomic::AtomicI64; @@ -459,7 +461,7 @@ async fn provider_error_and_disconnect_release_dispatch_admission() { assert_json( result, if disconnect { - StatusCode::SERVICE_UNAVAILABLE + StatusCode::INTERNAL_SERVER_ERROR } else { StatusCode::FORBIDDEN }, @@ -575,8 +577,8 @@ async fn provider_timeout_releases_admission_without_wall_clock_wait() { let (long, _reply) = f.start_read("1", &mut a).await; tokio::time::pause(); tokio::time::advance(std::time::Duration::from_secs(31)).await; - let body = assert_json(long.await.unwrap(), StatusCode::SERVICE_UNAVAILABLE).await; - assert_eq!(body["code"], "unavailable"); + let body = assert_json(long.await.unwrap(), StatusCode::INTERNAL_SERVER_ERROR).await; + assert_eq!(body["code"], "outcome_unknown"); tokio::time::resume(); let gate = f.server.admission_for_workspace(TEST_WORKSPACE_ID).await; drop( diff --git a/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs b/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs new file mode 100644 index 000000000..212198b6f --- /dev/null +++ b/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs @@ -0,0 +1,546 @@ +//! Integration coverage for signed loopback HTTP -> production admission/auth -> external +//! Workdir session -> real SQLite audit persistence. Only the external provider boundary +//! is substituted with a controlled command receiver. Temporary DBs, listeners and bounded +//! requests are isolated per case; these run in the normal workspace-server library suite. + +use super::*; +use std::io::Write; +use tracing::instrument::WithSubscriber; + +const AUDIT_SECRET: &str = "audit-store-password-secret"; +const WRITE_SECRET: &[u8] = b"write-content-secret"; +const PATH_SECRET: &str = "private-request-path-secret.txt"; +const PROVIDER_SECRET: &str = "/host/private/provider-secret"; + +type ProviderReply = + oneshot::Sender>; + +#[derive(Clone, Default)] +struct CapturedAuditLog(Arc>>); + +impl Write for CapturedAuditLog { + fn write(&mut self, bytes: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(bytes); + Ok(bytes.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +struct PendingHttpWrite { + task: tokio::task::JoinHandle<(StatusCode, Value)>, + proof: String, +} + +impl Drop for PendingHttpWrite { + fn drop(&mut self) { + self.task.abort(); + } +} + +struct ProviderProbe { + receiver: mpsc::Receiver, + dispatches: usize, +} + +impl ProviderProbe { + async fn receive_write(&mut self, pending: &mut PendingHttpWrite) -> (String, ProviderReply) { + let command = tokio::time::timeout(BOUND, async { + tokio::select! { + command = self.receiver.recv() => command.expect("provider channel closed"), + response = &mut pending.task => { + panic!("HTTP completed before write dispatch: {:?}", response.unwrap()); + } + } + }) + .await + .expect("signed HTTP write never reached provider"); + let ExternalProviderCommand::Operation { + operation_id, + operation, + response, + } = command + else { + panic!("expected write dispatch, received cancellation") + }; + self.dispatches += 1; + let WorkdirSessionOperation::Write(request) = operation else { + panic!("expected mutation, got {operation:?}") + }; + assert_eq!( + request.path, + workdir::WorkdirPath::new(PATH_SECRET).unwrap() + ); + assert_eq!(request.content, WRITE_SECRET); + assert_eq!(request.expected_hash, None); + (operation_id.as_str().to_owned(), response) + } + + fn assert_dispatches(&mut self, expected: usize) { + while let Ok(command) = self.receiver.try_recv() { + if matches!(command, ExternalProviderCommand::Operation { .. }) { + self.dispatches += 1; + } + } + assert_eq!( + self.dispatches, expected, + "unexpected mutation dispatch/retry" + ); + } +} + +impl Fixture { + fn attach_audited_writer(&self) -> ProviderProbe { + let receiver = self.attach("1"); + // Setup changes only: real signed requests still traverse production authorization. + // All three capability sources must agree before the session is first resolved. + let connection = rusqlite::Connection::open(&self.api.config.database_path).unwrap(); + assert_eq!( + connection + .execute( + "UPDATE external_workdir_grants SET permissions='read_write' \ + WHERE workspace_id=?1 AND grant_id='external-1'", + [TEST_WORKSPACE_ID], + ) + .unwrap(), + 1 + ); + assert_eq!( + connection + .execute( + "UPDATE worker_workdir_links SET capabilities='read_write' \ + WHERE workspace_id=?1 AND runtime_id='workdirless-runtime' \ + AND worker_id='1' AND alias='attachment' AND unlinked_at IS NULL", + [TEST_WORKSPACE_ID], + ) + .unwrap(), + 1 + ); + let mut registry = self.api.external_workdir_providers.lock().unwrap(); + let provider = registry.connections.get_mut("external-1").unwrap(); + Arc::get_mut(provider).unwrap().capabilities = + workdir::WorkdirSessionCapabilities::READ_WRITE; + ProviderProbe { + receiver, + dispatches: 0, + } + } +} + +struct AuditHttpFixture { + fixture: Fixture, + logs: CapturedAuditLog, + client: reqwest::Client, + base_url: String, + server: tokio::task::JoinHandle<()>, +} + +impl Drop for AuditHttpFixture { + fn drop(&mut self) { + self.server.abort(); + } +} + +impl AuditHttpFixture { + async fn new() -> (Self, ProviderProbe) { + let fixture = Fixture::new().await; + let provider = fixture.attach_audited_writer(); + let logs = CapturedAuditLog::default(); + let writer = logs.clone(); + let subscriber = tracing_subscriber::fmt() + .without_time() + .with_ansi(false) + .json() + .flatten_event(true) + .with_writer(move || writer.clone()) + .finish(); + let dispatch = tracing::Dispatch::new(subscriber); + // Axum spawns connection tasks: scope the subscriber around each request rather + // than relying on the listener task's thread-local subscriber being inherited. + let app = fixture.app.clone().layer(axum::middleware::from_fn( + move |request: Request, next: axum::middleware::Next| { + let dispatch = dispatch.clone(); + async move { next.run(request).with_subscriber(dispatch).await } + }, + )); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let base_url = format!("http://{}", listener.local_addr().unwrap()); + let server = tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + let client = reqwest::Client::builder() + .no_proxy() + .timeout(BOUND) + .build() + .unwrap(); + ( + Self { + fixture, + logs, + client, + base_url, + server, + }, + provider, + ) + } + + fn fail_audit(&self, before_dispatch: bool) { + let filter = if before_dispatch { "=" } else { "<>" }; + rusqlite::Connection::open(&self.fixture.api.config.database_path) + .unwrap() + .execute_batch(&format!( + "CREATE TRIGGER fail_operation_audit BEFORE INSERT ON audit_events \ + WHEN NEW.action='external_workdir_operation' AND NEW.outcome {filter} 'requested' \ + BEGIN SELECT RAISE(FAIL, '{AUDIT_SECRET}'); END;" + )) + .unwrap(); + } + + fn start_write(&self) -> PendingHttpWrite { + let path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-session/operations"); + let body = serde_json::to_vec(&WorkspaceWorkdirSessionOperationRequest { + target_workdir: "attachment".into(), + operation: WorkdirSessionOperation::Write(workdir::WriteRequest { + path: workdir::WorkdirPath::new(PATH_SECRET).unwrap(), + content: WRITE_SECRET.to_vec(), + expected_hash: None, + }), + }) + .unwrap(); + let request = self.fixture.request( + Some("1"), + &path, + body.clone(), + START, + worker_runtime::auth::WORKSPACE_REQUEST_PERMISSION, + ); + let proof = request.headers()[worker_runtime::auth::RUNTIME_REQUEST_SOURCE_PROOF_HEADER] + .to_str() + .unwrap() + .to_owned(); + let url = format!("{}{path}", self.base_url); + let client = self.client.clone(); + let headers = request.headers().clone(); + let task = tokio::spawn(async move { + let response = client + .post(url) + .headers(headers) + .body(body) + .send() + .await + .unwrap(); + let status = response.status(); + assert_eq!(response.headers()[CONTENT_TYPE], "application/json"); + let bytes = response.bytes().await.unwrap(); + (status, serde_json::from_slice(&bytes).unwrap()) + }); + PendingHttpWrite { task, proof } + } + + async fn finish( + &self, + mut pending: PendingHttpWrite, + expected_status: StatusCode, + ) -> (Value, Vec) { + let (status, body) = tokio::time::timeout(BOUND, &mut pending.task) + .await + .expect("HTTP did not finish; possible automatic retry") + .unwrap(); + assert_eq!(status, expected_status, "{body}"); + let text = String::from_utf8(self.logs.0.lock().unwrap().clone()).unwrap(); + let encoded_content = serde_json::to_string(WRITE_SECRET).unwrap(); + for output in [body.to_string(), text.clone()] { + for secret in [ + AUDIT_SECRET, + std::str::from_utf8(WRITE_SECRET).unwrap(), + encoded_content.as_str(), + PATH_SECRET, + PROVIDER_SECRET, + pending.proof.as_str(), + self.fixture._root.path().to_str().unwrap(), + ] { + assert!(!output.contains(secret), "sensitive data escaped: {output}"); + } + } + let events = text + .lines() + .map(|line| serde_json::from_str(line).unwrap()) + .collect(); + (body, events) + } + + fn assert_audits(&self, expected: &[(&str, &str)]) { + let connection = + rusqlite::Connection::open(&self.fixture.api.config.database_path).unwrap(); + let mut statement = connection + .prepare( + "SELECT request_id, outcome, summary FROM audit_events \ + WHERE workspace_id=?1 AND action='external_workdir_operation' \ + AND target_id='external-1' ORDER BY rowid", + ) + .unwrap(); + let rows: Vec<(String, String, String)> = statement + .query_map([TEST_WORKSPACE_ID], |row| { + Ok((row.get(0)?, row.get(1)?, row.get(2)?)) + }) + .unwrap() + .collect::>() + .unwrap(); + let actual: Vec<(&str, &str)> = rows + .iter() + .map(|(operation_id, outcome, _)| (operation_id.as_str(), outcome.as_str())) + .collect(); + assert_eq!(actual, expected, "persisted audits: {rows:?}"); + for (_, _, summary) in rows { + assert!(summary.contains("operation_kind=write"), "{summary}"); + for secret in [ + AUDIT_SECRET, + PATH_SECRET, + PROVIDER_SECRET, + "write-content-secret", + ] { + assert!( + !summary.contains(secret), + "sensitive audit summary: {summary}" + ); + } + } + } + + fn apply_provider_write(&self) { + std::fs::write(self.effect_path(), WRITE_SECRET).unwrap(); + } + + fn effect_path(&self) -> std::path::PathBuf { + self.fixture._root.path().join("provider-effect") + } +} + +fn write_result() -> WorkdirSessionOperationResult { + WorkdirSessionOperationResult::Write(workdir::WriteResult { + bytes_written: WRITE_SECRET.len(), + created: true, + }) +} + +fn assert_audit_failure( + body: &Value, + events: &[Value], + stage: &str, + debug_stage: &str, + outcome: Value, + debug_outcome: &str, +) -> String { + let failures = body["audit_failures"] + .as_array() + .expect("independent audit receipt"); + assert_eq!(failures.len(), 1, "{body}"); + let operation_id = failures[0]["operation_id"].as_str().unwrap(); + assert!(operation_id.starts_with("op-"), "{body}"); + assert_eq!( + failures[0], + json!({ + "operation_id": operation_id, + "stage": stage, + "reason": "audit_store_unavailable", + "operation_outcome": outcome, + }) + ); + let notifications: Vec<&Value> = events + .iter() + .filter(|event| event["event"] == "external_workdir_audit_failed") + .collect(); + assert_eq!(notifications.len(), 1, "{events:?}"); + let event = notifications[0]; + assert_eq!(event["operation_id"], operation_id); + assert_eq!(event["operation"], "write"); + assert_eq!(event["audit_stage"], debug_stage); + assert_eq!(event["audit_reason"], "AuditStoreUnavailable"); + assert_eq!(event["operation_outcome"], debug_outcome); + operation_id.to_owned() +} + +fn assert_failure_reason(events: &[Value], reason: &str) { + let errors: Vec<&Value> = events + .iter() + .filter(|event| event["event"] == "api_error") + .collect(); + assert_eq!(errors.len(), 1, "{events:?}"); + assert_eq!(errors[0]["failure_reason"], reason, "{events:?}"); +} + +#[tokio::test] +async fn successful_write_after_audit_failure_preserves_result_and_does_not_repeat_mutation() { + let (h, mut provider) = AuditHttpFixture::new().await; + h.fail_audit(false); + let mut pending = h.start_write(); + let (dispatched_id, reply) = provider.receive_write(&mut pending).await; + h.apply_provider_write(); + reply.send(Ok(write_result())).unwrap(); + let (mut body, events) = h.finish(pending, StatusCode::OK).await; + let receipt_id = assert_audit_failure( + &body, + &events, + "response_received", + "ResponseReceived", + json!({"kind":"succeeded"}), + "Succeeded", + ); + assert_eq!(receipt_id, dispatched_id); + body.as_object_mut().unwrap().remove("audit_failures"); + assert_eq!(body, serde_json::to_value(write_result()).unwrap()); + assert_eq!(std::fs::read(h.effect_path()).unwrap(), WRITE_SECRET); + provider.assert_dispatches(1); + h.assert_audits(&[(&dispatched_id, "requested")]); +} + +#[tokio::test] +async fn denied_write_after_audit_failure_preserves_provider_error_and_separate_receipt() { + let (h, mut provider) = AuditHttpFixture::new().await; + h.fail_audit(false); + let mut pending = h.start_write(); + let (dispatched_id, reply) = provider.receive_write(&mut pending).await; + let mut original = WorkdirTransportError::from_workdir_error(&workdir::WorkdirError::Denied( + PROVIDER_SECRET.into(), + )); + let expected_message = original.message.clone(); + original.message = PROVIDER_SECRET.into(); + reply.send(Err(original)).unwrap(); + let (body, events) = h.finish(pending, StatusCode::FORBIDDEN).await; + assert_eq!(body["code"], "denied"); + assert_eq!(body["message"], expected_message); + let receipt_id = assert_audit_failure( + &body, + &events, + "response_received", + "ResponseReceived", + json!({"kind":"failed", "code":"denied"}), + "Failed { code: Denied }", + ); + assert_eq!(receipt_id, dispatched_id); + assert!(!h.effect_path().exists()); + provider.assert_dispatches(1); + h.assert_audits(&[(&dispatched_id, "requested")]); +} + +#[tokio::test] +async fn lost_write_response_after_audit_failure_stays_unknown_without_repeating_effect() { + let (h, mut provider) = AuditHttpFixture::new().await; + h.fail_audit(false); + let mut pending = h.start_write(); + let (dispatched_id, reply) = provider.receive_write(&mut pending).await; + h.apply_provider_write(); + // The provider applied the effect, but the server cannot infer that from response loss. + drop(reply); + let (body, events) = h.finish(pending, StatusCode::INTERNAL_SERVER_ERROR).await; + assert_eq!(body["code"], "outcome_unknown"); + let receipt_id = assert_audit_failure( + &body, + &events, + "response_unknown", + "ResponseUnknown", + json!({"kind":"unknown"}), + "Unknown", + ); + assert_eq!(receipt_id, dispatched_id); + assert_eq!(std::fs::read(h.effect_path()).unwrap(), WRITE_SECRET); + provider.assert_dispatches(1); + h.assert_audits(&[(&dispatched_id, "requested")]); +} + +#[tokio::test] +async fn before_write_audit_failure_is_not_dispatched_and_has_no_provider_effect() { + let (h, mut provider) = AuditHttpFixture::new().await; + h.fail_audit(true); + let (body, events) = h + .finish(h.start_write(), StatusCode::SERVICE_UNAVAILABLE) + .await; + assert_eq!(body["code"], "not_dispatched"); + assert_audit_failure( + &body, + &events, + "before_dispatch", + "BeforeDispatch", + json!({"kind":"not_dispatched"}), + "NotDispatched", + ); + assert_failure_reason(&events, "audit_store_unavailable"); + assert!( + body.get("failure_reason").is_none(), + "internal reason leaked: {body}" + ); + assert!(!h.effect_path().exists()); + provider.assert_dispatches(0); + h.assert_audits(&[]); +} + +#[tokio::test] +async fn rejected_write_send_with_audit_failure_is_not_dispatched_not_unknown() { + let (h, mut provider) = AuditHttpFixture::new().await; + h.fail_audit(false); + provider.receiver.close(); + let (body, events) = h + .finish(h.start_write(), StatusCode::SERVICE_UNAVAILABLE) + .await; + assert_eq!(body["code"], "not_dispatched"); + let receipt_id = assert_audit_failure( + &body, + &events, + "dispatch_rejected", + "DispatchRejected", + json!({"kind":"not_dispatched"}), + "NotDispatched", + ); + assert_failure_reason(&events, "provider_unavailable"); + assert!(!h.effect_path().exists()); + provider.assert_dispatches(0); + h.assert_audits(&[(&receipt_id, "requested")]); +} + +#[tokio::test] +async fn successful_write_persists_requested_and_completed_with_same_operation_id() { + let (h, mut provider) = AuditHttpFixture::new().await; + let mut pending = h.start_write(); + let (operation_id, reply) = provider.receive_write(&mut pending).await; + h.apply_provider_write(); + reply.send(Ok(write_result())).unwrap(); + let (body, events) = h.finish(pending, StatusCode::OK).await; + assert_eq!(body, serde_json::to_value(write_result()).unwrap()); + assert!( + events + .iter() + .all(|event| event["event"] != "external_workdir_audit_failed") + ); + assert_eq!(std::fs::read(h.effect_path()).unwrap(), WRITE_SECRET); + provider.assert_dispatches(1); + h.assert_audits(&[(&operation_id, "requested"), (&operation_id, "completed")]); +} + +#[tokio::test] +async fn provider_reported_unknown_preserves_received_response_and_unknown_effects() { + let (h, mut provider) = AuditHttpFixture::new().await; + h.fail_audit(false); + let mut pending = h.start_write(); + let (dispatched_id, reply) = provider.receive_write(&mut pending).await; + reply + .send(Err(WorkdirTransportError::from_workdir_error( + &workdir::WorkdirError::OutcomeUnknown(PROVIDER_SECRET.into()), + ))) + .unwrap(); + let (body, events) = h.finish(pending, StatusCode::INTERNAL_SERVER_ERROR).await; + assert_eq!(body["code"], "outcome_unknown"); + let receipt_id = assert_audit_failure( + &body, + &events, + "response_received", + "ResponseReceived", + json!({"kind":"unknown"}), + "Unknown", + ); + assert_eq!(receipt_id, dispatched_id); + provider.assert_dispatches(1); + h.assert_audits(&[(&dispatched_id, "requested")]); +} From 7394ec6784727e44d9f4b10776631627abda3b1c Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 06:13:07 +0900 Subject: [PATCH 3/8] fix: retain safe Workdir open authorization and response failure causes Keep bounded reasons and derived failure stages separate from public classification. Reject provider-authored raw error text on transport decoding. Preserve identity mismatch when cleanup fails and retain its handle for explicit close without opening again. Add signed Runtime HTTP, Registry/API logging, remote mismatch, authorization no-dispatch and deferred fault coverage. Focused tests and Workdir suite pass; full-source validation follows T773 audit contract merge. --- crates/workdir/src/http.rs | 21 +- crates/workdir/src/lib.rs | 95 ++++ crates/workdir/tests/failure_diagnostics.rs | 31 + crates/workdir/tests/remote_denial_reasons.rs | 103 ++++ crates/worker-runtime/src/http_server.rs | 26 +- .../tests/runtime_workdir_failure_tests.rs | 528 ++++++++++++++++++ .../src/worker_backend/deferred_workdir.rs | 163 +++++- crates/workspace-server/src/hosts.rs | 17 +- .../src/hosts/tests/workspace_auth_tests.rs | 68 +++ crates/workspace-server/src/server.rs | 6 +- .../src/server/tests/api_logging_tests.rs | 56 ++ 11 files changed, 1093 insertions(+), 21 deletions(-) create mode 100644 crates/worker-runtime/src/http_server/tests/runtime_workdir_failure_tests.rs diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index f99f804c5..889a7eb93 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -375,12 +375,14 @@ impl WorkdirTransportError { use WorkdirTransportErrorCode as Code; let error = match self.code { Code::NotFound => WorkdirError::NotFound("".into()), - Code::Conflict => WorkdirError::Conflict(self.message), + Code::Conflict => WorkdirError::Conflict( + "The target file's content or existence changed since it was last observed; read the file again before retrying".into(), + ), Code::OutcomeUnknown => WorkdirError::OutcomeUnknown( "Workdir operation may have effects; inspect before retrying".into(), ), - Code::Unsupported => WorkdirError::UnsupportedOperation(self.message), - Code::InvalidRequest => WorkdirError::InvalidArgument(self.message), + Code::Unsupported => WorkdirError::UnsupportedOperation("Workdir operation is not supported".into()), + Code::InvalidRequest => WorkdirError::InvalidArgument("Workdir operation request is invalid".into()), Code::Denied => WorkdirError::Denied(crate::WorkdirDenial { reason: self.denial_reason, // Provider-authored text is not trusted diagnostic data. @@ -409,12 +411,12 @@ impl WorkdirTransportError { target: "".into(), }, Code::UnknownCommand => WorkdirError::UnknownCommand("".to_string()), - Code::Unavailable => WorkdirError::Unavailable(self.message), + Code::Unavailable => WorkdirError::Unavailable("Workdir session is unavailable".into()), Code::Io => WorkdirError::Io { path: "".into(), - source: std::io::Error::other(self.message), + source: std::io::Error::other("Workdir I/O operation failed"), }, - Code::Transport => WorkdirError::Transport(self.message), + Code::Transport => WorkdirError::Transport("Workdir transport failed".into()), Code::Internal => WorkdirError::OperationFailed, }; let error = if let Some(reason) = self.failure_reason { @@ -549,7 +551,8 @@ mod client { if opened.workdir_id.as_str() != workdir_id.as_str() { return Err(WorkdirError::Unavailable( "Runtime opened a session for a different Workdir".to_string(), - )); + ) + .with_failure_reason(crate::WorkdirFailureReason::SessionIdentityMismatch)); } Ok(Self { client, @@ -608,6 +611,7 @@ mod client { WorkdirError::Unavailable(format!( "Runtime returned a mismatched Workdir operation result; expected {expected}" )) + .with_failure_reason(crate::WorkdirFailureReason::OperationResultMismatch) } } @@ -770,7 +774,8 @@ mod client { } Ok(_) if mutation => Err(WorkdirError::OutcomeUnknown( "mismatched response after checkout mutation".into(), - )), + ) + .with_failure_reason(crate::WorkdirFailureReason::OperationResultMismatch)), Ok(_) => Err(Self::mismatch("checkout_execute")), Err(error) if mutation diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index 1a515ac16..10ba3bc6c 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -414,6 +414,24 @@ pub enum WorkdirFailureReason { HttpTransfer, HttpResponseInvalid, HttpResponseTooLarge, + OperationResultMismatch, + SessionIdentityMismatch, + SessionOpenTaskFailed, + SessionOpenFailed, + AuthorizationMissing, + AuthorizationStale, + AuthorizationUnavailable, + SigningIdentityUnavailable, + AuthorizationSignFailed, + RuntimeStopped, + RuntimeStateUnavailable, + RuntimeBackendUnavailable, + RuntimeOwnerMismatch, + RuntimeInvalidRequest, + WorkingDirectoryNotFound, + WorkingDirectoryNotActive, + WorkingDirectoryRecordInvalid, + WorkingDirectoryAccessUnavailable, } impl WorkdirFailureReason { @@ -427,6 +445,83 @@ impl WorkdirFailureReason { Self::HttpTransfer => "http_transfer", Self::HttpResponseInvalid => "http_response_invalid", Self::HttpResponseTooLarge => "http_response_too_large", + Self::OperationResultMismatch => "operation_result_mismatch", + Self::SessionIdentityMismatch => "session_identity_mismatch", + Self::SessionOpenTaskFailed => "session_open_task_failed", + Self::SessionOpenFailed => "session_open_failed", + Self::AuthorizationMissing => "authorization_missing", + Self::AuthorizationStale => "authorization_stale", + Self::AuthorizationUnavailable => "authorization_unavailable", + Self::SigningIdentityUnavailable => "signing_identity_unavailable", + Self::AuthorizationSignFailed => "authorization_sign_failed", + Self::RuntimeStopped => "runtime_stopped", + Self::RuntimeStateUnavailable => "runtime_state_unavailable", + Self::RuntimeBackendUnavailable => "runtime_backend_unavailable", + Self::RuntimeOwnerMismatch => "runtime_owner_mismatch", + Self::RuntimeInvalidRequest => "runtime_invalid_request", + Self::WorkingDirectoryNotFound => "working_directory_not_found", + Self::WorkingDirectoryNotActive => "working_directory_not_active", + Self::WorkingDirectoryRecordInvalid => "working_directory_record_invalid", + Self::WorkingDirectoryAccessUnavailable => "working_directory_access_unavailable", + } + } + + /// The observed failing boundary, not a dispatch or effects assertion. + pub const fn stage(self) -> WorkdirFailureStage { + use WorkdirFailureStage as Stage; + match self { + Self::SessionClosed => Stage::Session, + Self::ProviderUnavailable => Stage::Provider, + Self::CommandTerminalMissing => Stage::Command, + Self::HttpTimeout | Self::HttpConnect | Self::HttpTransfer => Stage::Http, + Self::HttpResponseInvalid + | Self::HttpResponseTooLarge + | Self::OperationResultMismatch => Stage::ResponseValidation, + Self::SessionIdentityMismatch + | Self::SessionOpenTaskFailed + | Self::SessionOpenFailed + | Self::WorkingDirectoryNotFound + | Self::WorkingDirectoryNotActive + | Self::WorkingDirectoryRecordInvalid + | Self::WorkingDirectoryAccessUnavailable => Stage::SessionOpen, + Self::AuthorizationMissing + | Self::AuthorizationStale + | Self::AuthorizationUnavailable + | Self::SigningIdentityUnavailable + | Self::AuthorizationSignFailed => Stage::AuthorizationPreparation, + Self::RuntimeStopped + | Self::RuntimeStateUnavailable + | Self::RuntimeBackendUnavailable + | Self::RuntimeOwnerMismatch + | Self::RuntimeInvalidRequest => Stage::Runtime, + } + } +} + +/// Internal operational boundary derived only from a bounded reason. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum WorkdirFailureStage { + Session, + Provider, + Command, + Http, + ResponseValidation, + SessionOpen, + AuthorizationPreparation, + Runtime, +} + +impl WorkdirFailureStage { + pub const fn as_str(self) -> &'static str { + match self { + Self::Session => "session", + Self::Provider => "provider", + Self::Command => "command", + Self::Http => "http", + Self::ResponseValidation => "response_validation", + Self::SessionOpen => "session_open", + Self::AuthorizationPreparation => "authorization_preparation", + Self::Runtime => "runtime", } } } diff --git a/crates/workdir/tests/failure_diagnostics.rs b/crates/workdir/tests/failure_diagnostics.rs index a94f4fe9f..b39336f04 100644 --- a/crates/workdir/tests/failure_diagnostics.rs +++ b/crates/workdir/tests/failure_diagnostics.rs @@ -83,3 +83,34 @@ fn provider_cannot_inject_free_text_failure_reason() { .is_err() ); } + +#[test] +fn provider_authored_operational_messages_never_enter_error_display_or_debug() { + for code in [ + Code::Conflict, + Code::OutcomeUnknown, + Code::Unsupported, + Code::InvalidRequest, + Code::Unavailable, + Code::Io, + Code::Transport, + Code::Internal, + ] { + let wire = serde_json::json!({ + "code": code, "message":"/host/private command-secret body-secret proof-secret key-secret Authorization: Bearer bearer-secret", + "failure_reason": "session_open_task_failed" + }); + let decoded: WorkdirTransportError = serde_json::from_value(wire).unwrap(); + let error = decoded.into_workdir_error(); + for text in [ + format!("{error}"), + format!("{error:?}"), + serde_json::to_string(&WorkdirTransportError::from_workdir_error(&error)).unwrap(), + ] { + assert!(!text.contains("secret"), "{text}"); + assert!(!text.contains("/host/private"), "{text}"); + } + assert_eq!(error.failure_reason(), Some(Reason::SessionOpenTaskFailed)); + assert_eq!(WorkdirTransportError::from_workdir_error(&error).code, code); + } +} diff --git a/crates/workdir/tests/remote_denial_reasons.rs b/crates/workdir/tests/remote_denial_reasons.rs index f14acc816..6769fb136 100644 --- a/crates/workdir/tests/remote_denial_reasons.rs +++ b/crates/workdir/tests/remote_denial_reasons.rs @@ -216,3 +216,106 @@ async fn remote_checkout_diagnostics_preserve_mutation_outcome_and_read_failures } } } + +#[tokio::test] +async fn mismatched_http_results_keep_cause_and_mutation_uncertainty() { + use workdir::{ + CheckoutOperation, CheckoutRequest, CommandHandle, CommandOutputRequest, + WorkdirFailureReason as Failure, + }; + for case in ["variant", "bytes", "mutation", "identity"] { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + let server = thread::spawn(move || { + reply( + &listener, + "200 OK", + if case == "identity" { + r#"{"session_id":"s","workdir_id":"wrong-secret","capabilities":{"bits":63}}"# + } else { + r#"{"session_id":"s","workdir_id":"w","capabilities":{"bits":63}}"# + }, + ); + if case != "identity" { + let result = if case == "bytes" { + serde_json::json!({"operation":"command_output","result":{ + "chunks": [], "next_cursor": 123, "terminal": null + }}) + } else { + serde_json::to_value( + workdir::http::WorkdirSessionOperationResult::CommandCancel, + ) + .unwrap() + }; + reply(&listener, "200 OK", &result.to_string()); + } + }); + let opened = RemoteWorkdirSession::open( + reqwest::Client::builder() + .no_proxy() + .timeout(Duration::from_secs(5)) + .build() + .unwrap(), + format!("http://{address}").parse().unwrap(), + "request-secret", + Workdir::new("w").id().clone(), + OpenWorkdirSessionRequest::default(), + ) + .await; + let error = if case == "identity" { + opened.unwrap_err() + } else { + let remote = opened.unwrap(); + match case { + "variant" => remote + .stat(StatRequest { + path: WorkdirPath::new("file-secret").unwrap(), + }) + .await + .unwrap_err(), + "bytes" => remote + .command_output(CommandOutputRequest { + handle: CommandHandle("command-secret".into()), + cursor: 0, + limit: 16, + wait: false, + }) + .await + .unwrap_err(), + "mutation" => remote + .checkout_execute(CheckoutRequest { + target: WorkdirPath::new("file-secret").unwrap(), + validator: vec![1], + operation: CheckoutOperation::Write { + content: b"body-secret".to_vec(), + expected_hash: [0; 32], + }, + }) + .await + .unwrap_err(), + _ => unreachable!(), + } + }; + server.join().unwrap(); + let expected = if case == "identity" { + Failure::SessionIdentityMismatch + } else { + Failure::OperationResultMismatch + }; + assert_eq!(error.failure_reason(), Some(expected), "{case}: {error:?}"); + let transport = WorkdirTransportError::from_workdir_error(&error); + assert_eq!( + transport.code, + if case == "mutation" { + Code::OutcomeUnknown + } else { + Code::Unavailable + } + ); + let wire = serde_json::to_string(&transport).unwrap(); + assert!(!wire.contains("secret"), "{wire}"); + assert!(!format!("{error:?}").contains("secret")); + let forwarded = transport.into_workdir_error(); + assert_eq!(forwarded.failure_reason(), Some(expected)); + } +} diff --git a/crates/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index aefc66973..1a6aeefe9 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -2187,7 +2187,25 @@ impl RuntimeHttpWorkdirError { } fn runtime(error: RuntimeError) -> Self { - match error { + use workdir::WorkdirFailureReason as Reason; + let reason = match &error { + RuntimeError::WorkingDirectory(diagnostic) => match diagnostic.code.as_str() { + "working_directory_not_found" => Reason::WorkingDirectoryNotFound, + "working_directory_not_active" => Reason::WorkingDirectoryNotActive, + "working_directory_record_invalid" => Reason::WorkingDirectoryRecordInvalid, + "working_directory_repository_access_unavailable" => { + Reason::WorkingDirectoryAccessUnavailable + } + _ => Reason::SessionOpenFailed, + }, + RuntimeError::RuntimeStopped => Reason::RuntimeStopped, + RuntimeError::StatePoisoned => Reason::RuntimeStateUnavailable, + RuntimeError::ExecutionBackendUnavailable { .. } => Reason::RuntimeBackendUnavailable, + RuntimeError::WorkspaceOwnerMismatch { .. } => Reason::RuntimeOwnerMismatch, + RuntimeError::InvalidRequest(_) => Reason::RuntimeInvalidRequest, + _ => Reason::SessionOpenFailed, + }; + let mut response = match error { RuntimeError::WorkingDirectory(diagnostic) if diagnostic.code == "working_directory_not_found" => { @@ -2200,7 +2218,9 @@ impl RuntimeHttpWorkdirError { WorkdirTransportErrorCode::Unavailable, "Workdir session is unavailable", ), - } + }; + response.payload.failure_reason = Some(reason); + response } } @@ -2222,6 +2242,7 @@ impl IntoResponse for RuntimeHttpWorkdirError { status = self.status.as_u16(), code = self.payload.code.as_str(), failure_reason = reason.as_str(), + failure_stage = reason.stage().as_str(), "Workdir provider operation failed" ); } @@ -2434,6 +2455,7 @@ pub enum RuntimeHttpServerError { #[cfg(test)] mod tests { mod command_transport_tests; + mod runtime_workdir_failure_tests; #[cfg(feature = "fs-store")] mod session_material_tests; diff --git a/crates/worker-runtime/src/http_server/tests/runtime_workdir_failure_tests.rs b/crates/worker-runtime/src/http_server/tests/runtime_workdir_failure_tests.rs new file mode 100644 index 000000000..55873420b --- /dev/null +++ b/crates/worker-runtime/src/http_server/tests/runtime_workdir_failure_tests.rs @@ -0,0 +1,528 @@ +//! Session-open integration through the production signed router, Runtime owner/binding +//! checks, HTTP DTO and tracing boundary. Only the execution backend is replaced +//! for deterministic lookup/open faults and inert owner-binding Worker fixtures; +//! no handler, middleware or grant is replaced. Runs in the normal http-server lib +//! suite with isolated loopback sockets. + +use super::*; +use crate::working_directory::WorkingDirectoryDiagnostic; +use std::io::{self, Write}; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::Duration; + +const OPEN_PATH: &str = "/v1/working-directories/private-workdir-marker/sessions"; +const PRIVATE_DIAGNOSTIC: &str = concat!( + "raw-runtime-error-marker /private/host-path-marker ", + "command=private-command-marker contents=private-file-marker ", + "proof=private-proof-marker Authorization: Bearer private-header-marker ", + "working_directory_not_found authorization_stale runtime_stopped", +); +const PRIVATE_MARKERS: &[&str] = &[ + "raw-runtime-error-marker", + "/private/host-path-marker", + "private-command-marker", + "private-file-marker", + "private-proof-marker", + "private-header-marker", + "private-workdir-marker", + "private-owner-marker", +]; + +#[derive(Clone, Default)] +struct LogCapture(Arc>>); + +impl Write for LogCapture { + fn write(&mut self, bytes: &[u8]) -> io::Result { + self.0.lock().unwrap().extend_from_slice(bytes); + Ok(bytes.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +impl LogCapture { + fn subscriber(&self) -> impl tracing::Subscriber + Send + Sync + 'static { + let writer = self.clone(); + tracing_subscriber::fmt() + .with_ansi(false) + .without_time() + .with_max_level(tracing::Level::WARN) + .with_writer(move || writer.clone()) + .finish() + } + + fn take(&self) -> String { + String::from_utf8(std::mem::take(&mut *self.0.lock().unwrap())).unwrap() + } +} + +fn assert_private_absent(text: &str) { + for marker in PRIVATE_MARKERS { + assert!( + !text.contains(marker), + "private input escaped error boundary" + ); + } +} + +fn assert_failure_log(log: &str, status: StatusCode, code: &str, reason: &str, stage: &str) { + let events: Vec<_> = log + .lines() + .filter(|line| line.contains("workdir_provider_failure")) + .collect(); + assert_eq!(events.len(), 1, "one diagnostic per failed request: {log}"); + let event = events[0]; + for field in [ + "WARN".to_string(), + "worker_runtime::workdir".to_string(), + format!("status={}", status.as_u16()), + format!("code=\"{code}\""), + format!("failure_reason=\"{reason}\""), + format!("failure_stage=\"{stage}\""), + ] { + assert!(event.contains(&field), "missing {field}: {event}"); + } + assert_private_absent(log); +} + +fn assert_failure_body(body: &str, code: &str, message: &str, reason: &str) { + assert_private_absent(body); + // Exact safe transport vocabulary; raw diagnostics and stage are not wire fields. + assert_eq!( + serde_json::from_str::(body).unwrap(), + serde_json::json!({"code": code, "message": message, "failure_reason": reason}), + ); +} + +struct Harness { + server: tokio::task::JoinHandle<()>, + client: reqwest::Client, + base: String, + identity: RuntimeIdentityMaterial, + trust: WorkspaceIssuerTrustRecord, +} + +impl Drop for Harness { + fn drop(&mut self) { + self.server.abort(); + } +} + +impl Harness { + async fn new(runtime: Runtime) -> Self { + let identity = RuntimeIdentityMaterial::generate("runtime-failure-test-key").unwrap(); + let trust = workspace_trust(&identity); + let app = runtime_http_router_with_workspace_auth( + runtime, + None, + workspace_test_auth(vec![trust.clone()]), + ); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let base = format!("http://{}", listener.local_addr().unwrap()); + let server = tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + Self { + server, + client: reqwest::Client::builder() + .no_proxy() + .timeout(Duration::from_secs(5)) + .build() + .unwrap(), + base, + identity, + trust, + } + } + + async fn send( + &self, + claims: &WorkspaceCapabilityClaims, + body: Vec, + workspace: &str, + ) -> (StatusCode, String, String) { + let token = issue_workspace_capability_token(&self.identity.signing_key().unwrap(), claims) + .unwrap(); + let response = self + .client + .post(format!("{}{OPEN_PATH}", self.base)) + .bearer_auth(&token) + .header(header::CONTENT_TYPE, "application/json") + .header(RUNTIME_WORKSPACE_SCOPE_HEADER, workspace) + .header("x-private-test-header", "private-header-marker") + .body(body) + .send() + .await + .unwrap(); + let status = StatusCode::from_u16(response.status().as_u16()).unwrap(); + let headers = format!("{:?}", response.headers()); + let body = response.text().await.unwrap(); + assert_private_absent(&headers); + for output in [&body, &headers] { + assert!(!output.contains(&token), "signed bearer token leaked"); + assert!( + !output.contains(&self.identity.private_key), + "signing key leaked" + ); + } + (status, body, token) + } + + async fn open(&self) -> (StatusCode, String, String) { + let body = serde_json::to_vec(&OpenWorkdirSessionRequest::default()).unwrap(); + let claims = signed_claims(&self.trust, "POST", OPEN_PATH, &body); + self.send(&claims, body, "workspace-a").await + } +} + +#[derive(Clone, Copy)] +enum FailureAt { + Lookup, + Open, +} + +struct FailingBackend { + at: FailureAt, + code: &'static str, + lookups: AtomicUsize, + opens: AtomicUsize, +} + +impl FailingBackend { + fn runtime(at: FailureAt, code: &'static str) -> (Runtime, Arc) { + let backend = Arc::new(Self { + at, + code, + lookups: AtomicUsize::new(0), + opens: AtomicUsize::new(0), + }); + let runtime = + Runtime::with_execution_backend(RuntimeOptions::default(), backend.clone()).unwrap(); + (runtime, backend) + } + + fn failure(&self) -> WorkingDirectoryDiagnostic { + WorkingDirectoryDiagnostic::rejected(self.code, PRIVATE_DIAGNOSTIC) + } +} + +impl WorkerExecutionBackend for FailingBackend { + fn backend_id(&self) -> &str { + "runtime-failure-test" + } + + fn spawn_worker(&self, request: WorkerExecutionSpawnRequest) -> WorkerExecutionSpawnResult { + AcceptingBackend.spawn_worker(request) + } + + fn dispatch_input(&self, worker: &WorkerRef, input: WorkerInput) -> WorkerExecutionResult { + AcceptingBackend.dispatch_input(worker, input) + } + + fn stop_worker(&self, worker: &WorkerRef) -> WorkerExecutionResult { + AcceptingBackend.stop_worker(worker) + } + + fn working_directory( + &self, + id: &str, + ) -> Result { + self.lookups.fetch_add(1, Ordering::SeqCst); + match self.at { + FailureAt::Lookup => Err(self.failure()), + FailureAt::Open => Ok(WorkingDirectoryStatus { + summary: crate::catalog::WorkingDirectorySummary { + working_directory_id: id.into(), + display_name: None, + repository_id: "repository-test".into(), + creation_selector: None, + creation_ref: None, + creation_tree: None, + current_selector: None, + current_ref: None, + current_tree: None, + observed_at_epoch_seconds: None, + materializer_kind: crate::catalog::MaterializerKind::RuntimeGitClone, + cleanup_target: None, + status: crate::catalog::WorkingDirectoryStatusKind::Active, + cleanliness: None, + occupied_by: None, + }, + }), + } + } + + fn open_workdir_session( + &self, + _: &str, + ) -> Result { + self.opens.fetch_add(1, Ordering::SeqCst); + Err(self.failure()) + } +} + +// Current-thread execution keeps the server tasks inside the thread-local tracing +// capture across awaits, without replacing the process-global subscriber. +#[tokio::test(flavor = "current_thread")] +async fn signed_http_session_open_preserves_runtime_availability_and_owner_failures() { + let capture = LogCapture::default(); + let _guard = tracing::subscriber::set_default(capture.subscriber()); + tracing::callsite::rebuild_interest_cache(); + tokio::time::timeout(Duration::from_secs(15), async { + let stopped = Runtime::new_memory(); + stopped.stop_runtime().unwrap(); + let (owned, backend) = FailingBackend::runtime(FailureAt::Lookup, "must_not_reach_backend"); + let mut request = task_request("bind owner"); + request.config_bundle = None; + request.memory_settings.as_mut().unwrap().workspace_id = "workspace-a".into(); + owned + .create_worker_scoped( + &RuntimeWorkspaceScope::new("workspace-a", "private-owner-marker"), + request, + ) + .unwrap(); + for (runtime, status, code, message, reason) in [ + ( + stopped, + StatusCode::SERVICE_UNAVAILABLE, + "unavailable", + "Workdir session is unavailable", + "runtime_stopped", + ), + ( + Runtime::new_memory(), + StatusCode::SERVICE_UNAVAILABLE, + "unavailable", + "Workdir session is unavailable", + "runtime_backend_unavailable", + ), + ( + owned, + StatusCode::NOT_FOUND, + "not_found", + "Workdir session was not found", + "runtime_owner_mismatch", + ), + ] { + let harness = Harness::new(runtime).await; + capture.take(); + let (actual, body, token) = harness.open().await; + assert_eq!(actual, status); + assert_failure_body(&body, code, message, reason); + let log = capture.take(); + assert_failure_log(&log, status, code, reason, "runtime"); + assert!(!log.contains(&token)); + assert!(!log.contains(&harness.identity.private_key)); + } + assert_eq!(backend.lookups.load(Ordering::SeqCst), 0); + assert_eq!(backend.opens.load(Ordering::SeqCst), 0); + }) + .await + .expect("bounded Runtime failure HTTP requests"); +} + +#[tokio::test(flavor = "current_thread")] +async fn signed_http_session_open_allowlists_diagnostic_codes_without_guessing_unknown_causes() { + let capture = LogCapture::default(); + let _guard = tracing::subscriber::set_default(capture.subscriber()); + tracing::callsite::rebuild_interest_cache(); + tokio::time::timeout(Duration::from_secs(20), async { + for (diagnostic, reason) in [ + ("working_directory_not_found", "working_directory_not_found"), + ( + "working_directory_not_active", + "working_directory_not_active", + ), + ( + "working_directory_record_invalid", + "working_directory_record_invalid", + ), + ( + "working_directory_repository_access_unavailable", + "working_directory_access_unavailable", + ), + // Plausible provider text/code is not evidence of a typed cause. + ("authorization_stale", "session_open_failed"), + ( + "working_directory_not_found_private-proof-marker", + "session_open_failed", + ), + ] { + for at in [FailureAt::Lookup, FailureAt::Open] { + let (runtime, backend) = FailingBackend::runtime(at, diagnostic); + let harness = Harness::new(runtime).await; + capture.take(); + let (status, body, token) = harness.open().await; + let (expected, code, message) = if diagnostic == "working_directory_not_found" { + ( + StatusCode::NOT_FOUND, + "not_found", + "Workdir session was not found", + ) + } else { + ( + StatusCode::SERVICE_UNAVAILABLE, + "unavailable", + "Workdir session is unavailable", + ) + }; + assert_eq!(status, expected, "{diagnostic}"); + assert_failure_body(&body, code, message, reason); + let log = capture.take(); + assert_failure_log(&log, status, code, reason, "session_open"); + assert!(!log.contains(&token)); + assert!(!log.contains(&harness.identity.private_key)); + assert_eq!(backend.lookups.load(Ordering::SeqCst), 1, "no hidden retry"); + assert_eq!( + backend.opens.load(Ordering::SeqCst), + usize::from(matches!(at, FailureAt::Open)), + ); + } + } + }) + .await + .expect("bounded diagnostic fault matrix"); +} + +#[tokio::test(flavor = "current_thread")] +async fn signed_http_session_open_rejects_bad_capability_scope_and_owner_binding_before_backend() { + let capture = LogCapture::default(); + let _guard = tracing::subscriber::set_default(capture.subscriber()); + tracing::callsite::rebuild_interest_cache(); + tokio::time::timeout(Duration::from_secs(15), async { + let (runtime, backend) = + FailingBackend::runtime(FailureAt::Open, "working_directory_record_invalid"); + let mut request = task_request("foreign workspace binding"); + request.config_bundle = None; + request.memory_settings.as_mut().unwrap().workspace_id = "other-workspace".into(); + let foreign_worker = runtime + .create_worker_scoped( + &RuntimeWorkspaceScope::new("other-workspace", "https://backend.test"), + request, + ) + .unwrap() + .worker_ref; + let harness = Harness::new(runtime).await; + for case in ["permission", "body", "workspace", "owner_binding"] { + let body = if case == "owner_binding" { + serde_json::to_vec(&OpenWorkdirSessionRequest { + owner_worker_id: Some(foreign_worker.worker_id.to_string()), + }) + .unwrap() + } else { + b"{}".to_vec() + }; + let mut claims = signed_claims(&harness.trust, "POST", OPEN_PATH, &body); + if case == "permission" { + claims.operation = "workers:read".into(); + } + if case == "workspace" { + claims.issuer_workspace_id = "other-workspace".into(); + } + let sent = if case == "body" { + b"{ }".to_vec() + } else { + body + }; + capture.take(); + let (status, body, token) = harness.send(&claims, sent, "workspace-a").await; + assert_eq!( + status, + if case == "owner_binding" { + StatusCode::NOT_FOUND + } else { + StatusCode::UNAUTHORIZED + }, + "{case}", + ); + assert_private_absent(&body); + let log = capture.take(); + assert_private_absent(&log); + assert!(!log.contains(&token)); + assert!(!log.contains(&harness.identity.private_key)); + if case == "owner_binding" { + assert_failure_body( + &body, + "not_found", + "Workdir session was not found", + "working_directory_not_found", + ); + assert_failure_log( + &log, + status, + "not_found", + "working_directory_not_found", + "session_open", + ); + } else { + let wire: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!(wire["error"]["code"], "unauthorized"); + assert!( + !log.contains("workdir_provider_failure"), + "auth rejection is not a provider fault", + ); + } + } + assert_eq!(backend.lookups.load(Ordering::SeqCst), 0); + assert_eq!(backend.opens.load(Ordering::SeqCst), 0); + }) + .await + .expect("bounded authorization rejection requests"); +} + +#[tokio::test(flavor = "current_thread")] +async fn runtime_error_response_keeps_raw_details_private_and_unclassified_causes_generic() { + let capture = LogCapture::default(); + let _guard = tracing::subscriber::set_default(capture.subscriber()); + tracing::callsite::rebuild_interest_cache(); + for (error, status, code, message, reason, stage) in [ + ( + RuntimeError::ExecutionBackendUnavailable { + message: PRIVATE_DIAGNOSTIC.into(), + }, + StatusCode::SERVICE_UNAVAILABLE, + "unavailable", + "Workdir session is unavailable", + "runtime_backend_unavailable", + "runtime", + ), + ( + RuntimeError::StatePoisoned, + StatusCode::SERVICE_UNAVAILABLE, + "unavailable", + "Workdir session is unavailable", + "runtime_state_unavailable", + "runtime", + ), + ( + RuntimeError::InvalidRequest(PRIVATE_DIAGNOSTIC.into()), + StatusCode::BAD_REQUEST, + "invalid_request", + "Workdir operation request is invalid", + "runtime_invalid_request", + "runtime", + ), + ( + RuntimeError::StoreCorrupt { + operation: "session_open", + path: PathBuf::from("/private/host-path-marker"), + message: PRIVATE_DIAGNOSTIC.into(), + }, + StatusCode::SERVICE_UNAVAILABLE, + "unavailable", + "Workdir session is unavailable", + "session_open_failed", + "session_open", + ), + ] { + capture.take(); + let response = RuntimeHttpWorkdirError::runtime(error).into_response(); + assert_eq!(response.status(), status); + let bytes = to_bytes(response.into_body(), 16384).await.unwrap(); + let body = std::str::from_utf8(&bytes).unwrap(); + assert_failure_body(body, code, message, reason); + assert_failure_log(&capture.take(), status, code, reason, stage); + } +} diff --git a/crates/worker-runtime/src/worker_backend/deferred_workdir.rs b/crates/worker-runtime/src/worker_backend/deferred_workdir.rs index eebedde57..9bcb3962d 100644 --- a/crates/worker-runtime/src/worker_backend/deferred_workdir.rs +++ b/crates/worker-runtime/src/worker_backend/deferred_workdir.rs @@ -66,13 +66,32 @@ impl DeferredWorkdirSession { let open = self.open.clone(); let session = tokio::task::spawn_blocking(move || open()) .await - .map_err(|_| WorkdirError::Unavailable("Workdir session opener failed".into()))? - .map_err(WorkdirError::Unavailable)?; + .map_err(|_| { + WorkdirError::Unavailable("Workdir session opener failed".into()) + .with_failure_reason(WorkdirFailureReason::SessionOpenTaskFailed) + })? + .map_err(|_| { + // The opener's string contract cannot prove a more specific cause. + WorkdirError::Unavailable("Workdir session could not be opened".into()) + .with_failure_reason(WorkdirFailureReason::SessionOpenFailed) + })?; if session.workdir().id() != self.workdir.id() { - session.close().await?; - return Err(WorkdirError::Unavailable( - "Workdir session identity mismatch".into(), - )); + if let Err(error) = session.close().await { + // Retain a failed-close owner for explicit close; never use it. + state.closed = true; + state.session = Some(session); + tracing::warn!( + target: "worker_runtime::workdir", + event = "workdir_session_identity_cleanup_failed", + failure_reason = error.failure_reason().map(WorkdirFailureReason::as_str), + denial_reason = error.denial_reason().map(WorkdirDenialReason::as_str), + "Mismatched Workdir session cleanup failed" + ); + } + return Err( + WorkdirError::Unavailable("Workdir session identity mismatch".into()) + .with_failure_reason(WorkdirFailureReason::SessionIdentityMismatch), + ); } if let Some(mut source) = session.subscribe_command_events() { let events = self.events.clone(); @@ -224,10 +243,16 @@ mod tests { }, ); assert_eq!(attempts.load(Ordering::SeqCst), 0); + let failed = session.read(read_request()).await.unwrap_err(); assert!(matches!( - session.read(read_request()).await, - Err(WorkdirError::Unavailable(_)) + failed.classification_source(), + WorkdirError::Unavailable(_) )); + assert_eq!( + failed.failure_reason(), + Some(WorkdirFailureReason::SessionOpenFailed) + ); + assert_eq!(attempts.load(Ordering::SeqCst), 1); assert_eq!( session.read(read_request()).await.unwrap().bytes, b"restored" @@ -253,4 +278,126 @@ mod tests { ); session.close().await.unwrap(); } + + #[tokio::test] + async fn opener_task_and_open_errors_keep_distinct_safe_causes() { + for panic in [false, true] { + let attempts = Arc::new(AtomicUsize::new(0)); + let observed = attempts.clone(); + let session = DeferredWorkdirSession::handle( + "saved", + WorkdirSessionCapabilities::READ_ONLY, + move || { + observed.fetch_add(1, Ordering::SeqCst); + if panic { + panic!( + "/host/private bearer-secret command-secret proof-secret key-secret" + ); + } + Err("/host/private bearer-secret command-secret proof-secret key-secret".into()) + }, + ); + let error = session.read(read_request()).await.unwrap_err(); + assert_eq!(attempts.load(Ordering::SeqCst), 1); + let reason = if panic { + WorkdirFailureReason::SessionOpenTaskFailed + } else { + WorkdirFailureReason::SessionOpenFailed + }; + assert_eq!(error.failure_reason(), Some(reason)); + assert_eq!(reason.stage().as_str(), "session_open"); + let transport = workdir::http::WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.code.http_status(), 503); + assert!( + !serde_json::to_string(&transport) + .unwrap() + .contains("secret") + ); + assert!(!format!("{error:?}").contains("secret")); + session.close().await.unwrap(); + assert_eq!(attempts.load(Ordering::SeqCst), 1); + } + } + + #[derive(Debug)] + struct MismatchedSession { + workdir: Workdir, + closes: Arc, + fail_close: bool, + } + + macro_rules! mismatched_session { + ($( $method:ident($request:ident: $request_type:ty) -> $result:ty; )*) => { + #[async_trait] + impl WorkdirSession for MismatchedSession { + fn workdir(&self) -> &Workdir { &self.workdir } + fn capabilities(&self) -> WorkdirSessionCapabilities { WorkdirSessionCapabilities::READ_ONLY } + $(async fn $method(&self, _: $request_type) -> Result<$result, WorkdirError> { + panic!("mismatched session must never be used"); + })* + async fn close(&self) -> Result<(), WorkdirError> { + let attempt = self.closes.fetch_add(1, Ordering::SeqCst); + if self.fail_close && attempt == 0 { + Err(WorkdirError::Transport("/host/private key-secret".into()) + .with_failure_reason(WorkdirFailureReason::HttpTransfer)) + } else { Ok(()) } + } + } + }; + } + mismatched_session! { + stat(request: StatRequest) -> StatResult; + read(request: ReadRequest) -> ReadResult; + write(request: WriteRequest) -> WriteResult; + edit(request: EditRequest) -> EditResult; + list(request: ListRequest) -> ListResult; + glob(request: GlobRequest) -> GlobResult; + grep(request: GrepRequest) -> GrepResult; + start_command(request: CommandRequest) -> CommandHandle; + command_status(handle: CommandHandle) -> CommandStatus; + command_output(request: CommandOutputRequest) -> CommandOutput; + cancel_command(handle: CommandHandle) -> (); + } + + #[tokio::test] + async fn identity_mismatch_preserves_primary_cause_and_failed_close_owner() { + for fail_close in [false, true] { + let closes = Arc::new(AtomicUsize::new(0)); + let raw: WorkdirSessionHandle = Arc::new(MismatchedSession { + workdir: Workdir::new("wrong-secret"), + closes: closes.clone(), + fail_close, + }); + let attempts = Arc::new(AtomicUsize::new(0)); + let observed = attempts.clone(); + let session = DeferredWorkdirSession::handle( + "saved", + WorkdirSessionCapabilities::READ_ONLY, + move || { + observed.fetch_add(1, Ordering::SeqCst); + Ok(raw.clone()) + }, + ); + let error = session.read(read_request()).await.unwrap_err(); + assert_eq!( + error.failure_reason(), + Some(WorkdirFailureReason::SessionIdentityMismatch) + ); + assert!(!format!("{error:?}").contains("secret")); + assert_eq!(closes.load(Ordering::SeqCst), 1); + if fail_close { + assert!(matches!( + session.read(read_request()).await, + Err(WorkdirError::SessionClosed) + )); + assert_eq!(attempts.load(Ordering::SeqCst), 1); + } + session.close().await.unwrap(); + assert_eq!( + closes.load(Ordering::SeqCst), + if fail_close { 2 } else { 1 } + ); + assert_eq!(attempts.load(Ordering::SeqCst), 1); + } + } } diff --git a/crates/workspace-server/src/hosts.rs b/crates/workspace-server/src/hosts.rs index 4027b221a..7fbb8bf25 100644 --- a/crates/workspace-server/src/hosts.rs +++ b/crates/workspace-server/src/hosts.rs @@ -3995,12 +3995,27 @@ impl WorkdirHttpAuthorization for RemoteWorkdirAuthorization { return authorizer .issue_async(method, path_and_query, operation, None, body) .await - .map_err(|error| WorkdirError::Unavailable(error.message)); + .map_err(|error| { + use workdir::WorkdirFailureReason as Reason; + let reason = match error.code.as_str() { + "workspace_runtime_authorization_stale" => Reason::AuthorizationStale, + "workspace_signing_identity_unavailable" => { + Reason::SigningIdentityUnavailable + } + "workspace_runtime_authorization_sign_failed" => { + Reason::AuthorizationSignFailed + } + _ => Reason::AuthorizationUnavailable, + }; + WorkdirError::Unavailable("Workdir authorization is unavailable".into()) + .with_failure_reason(reason) + }); } authorization.fallback_bearer_token.ok_or_else(|| { WorkdirError::Unavailable( "remote Runtime does not have bearer authorization configured".to_string(), ) + .with_failure_reason(workdir::WorkdirFailureReason::AuthorizationMissing) }) }) } diff --git a/crates/workspace-server/src/hosts/tests/workspace_auth_tests.rs b/crates/workspace-server/src/hosts/tests/workspace_auth_tests.rs index 29c0cb52c..f6b8f1c73 100644 --- a/crates/workspace-server/src/hosts/tests/workspace_auth_tests.rs +++ b/crates/workspace-server/src/hosts/tests/workspace_auth_tests.rs @@ -315,3 +315,71 @@ async fn capability_claims_must_match_current_workspace_identity_before_signing( .unwrap(); verify(&fixture.verifier(), &token).unwrap(); } + +#[tokio::test] +async fn remote_workdir_authorization_failures_keep_safe_reason_without_dispatch_or_fallback() { + use workdir::WorkdirFailureReason as Reason; + use workdir::http::{OpenWorkdirSessionRequest, RemoteWorkdirSession, WorkdirTransportError}; + for case in ["stale", "missing_material", "missing_authorization"] { + let fixture = Fixture::new().await; + if case == "stale" { + fixture + .store + .revoke_workspace_runtime_binding(WORKSPACE, RUNTIME, "revoked") + .unwrap(); + } else if case == "missing_material" { + fixture + .materials + .delete(&fixture.identity.private_material_ref) + .unwrap(); + } + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + listener.set_nonblocking(true).unwrap(); + let authorization = RemoteWorkdirAuthorization { + workspace_authorization: (case != "missing_authorization") + .then(|| fixture.authorizer.clone()), + fallback_bearer_token: (case != "missing_authorization") + .then(|| "bearer-secret".into()), + }; + let error = RemoteWorkdirSession::open_with_authorization( + reqwest::Client::builder().no_proxy().build().unwrap(), + format!("http://{}", listener.local_addr().unwrap()) + .parse() + .unwrap(), + Arc::new(authorization), + workdir::Workdir::new("workdir-secret").id().clone(), + OpenWorkdirSessionRequest::default(), + ) + .await + .unwrap_err(); + let reason = match case { + "stale" => Reason::AuthorizationStale, + "missing_material" => Reason::AuthorizationUnavailable, + _ => Reason::AuthorizationMissing, + }; + assert_eq!(error.failure_reason(), Some(reason)); + assert_eq!(reason.stage().as_str(), "authorization_preparation"); + let transport = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.code.http_status(), 503); + assert_eq!(transport.message, "Workdir session is unavailable"); + assert!( + !serde_json::to_string(&transport) + .unwrap() + .contains("secret") + ); + let debug = format!("{error:?}"); + for secret in [ + "secret", + "workspace-signing/", + "private material", + "Authorization:", + "Bearer ", + ] { + assert!(!debug.contains(secret), "{case}: {debug}"); + } + assert!( + matches!(listener.accept(), Err(error) if error.kind() == std::io::ErrorKind::WouldBlock), + "authorization failure sent HTTP or fell back" + ); + } +} diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index a1b36997f..33bed6818 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -13506,6 +13506,7 @@ async fn log_failed_api_response(request: Request, next: Next) -> Response { request_token_id_hash = context.and(request_token_id_hash.as_deref()), denial_reason = error.and_then(|error| error.denial_reason).map(|reason| reason.as_str()), failure_reason = error.and_then(|error| error.failure_reason).map(|reason| reason.as_str()), + failure_stage = error.and_then(|error| error.failure_reason).map(|reason| reason.stage().as_str()), status = event.status, kind = event.kind.unwrap_or("unknown"), message = event.message.unwrap_or(""), @@ -36942,8 +36943,9 @@ impl ApiErrorLog { log.denial_reason = error.denial_reason; log.failure_reason = error.failure_reason; if log.workdir_log.is_some() { - // Existing public API details are unchanged; do not copy request-derived - // alias/handle text or session-open errors into operational diagnostics. + // Public classification and arbitrary diagnostic text are not safe + // operational causes. Keep only the separately typed reason/stage; + // never infer a reason by parsing request- or provider-authored text. log.kind = format!("workdir_session_operation_api_{}", status.as_u16()); // This can also fail after a command was dispatched. Do not claim // a pre-dispatch refusal or absence of effects in the safe message. diff --git a/crates/workspace-server/src/server/tests/api_logging_tests.rs b/crates/workspace-server/src/server/tests/api_logging_tests.rs index fc883168b..604296ac5 100644 --- a/crates/workspace-server/src/server/tests/api_logging_tests.rs +++ b/crates/workspace-server/src/server/tests/api_logging_tests.rs @@ -605,6 +605,10 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() { workdir::WorkdirFailureReason::SessionClosed, workdir::WorkdirFailureReason::CommandTerminalMissing, workdir::WorkdirFailureReason::HttpTimeout, + workdir::WorkdirFailureReason::OperationResultMismatch, + workdir::WorkdirFailureReason::SessionOpenTaskFailed, + workdir::WorkdirFailureReason::AuthorizationStale, + workdir::WorkdirFailureReason::RuntimeStopped, ] { let request = runtime_source_request( &identity, @@ -653,6 +657,7 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() { ); assert_eq!(events.len(), 1, "{events:?}"); assert_eq!(events[0]["failure_reason"], reason.as_str()); + assert_eq!(events[0]["failure_stage"], reason.stage().as_str()); assert_eq!(events[0]["operation"], "command_output"); assert_eq!(events[0]["workdir_stage"], "provider_dispatch"); let log = serde_json::to_string(&events).unwrap(); @@ -1249,3 +1254,54 @@ async fn non_api_workspace_paths_are_not_logged_as_ticket_api_errors() { assert_eq!(status, StatusCode::BAD_REQUEST); assert!(events.is_empty()); } + +#[tokio::test] +async fn session_open_failure_causes_survive_registry_api_conversion_without_raw_diagnostics() { + use workdir::WorkdirFailureReason as Reason; + for reason in [ + Reason::SessionOpenFailed, + Reason::SessionOpenTaskFailed, + Reason::SessionIdentityMismatch, + Reason::AuthorizationStale, + Reason::AuthorizationUnavailable, + Reason::RuntimeStopped, + Reason::WorkingDirectoryRecordInvalid, + Reason::OperationResultMismatch, + ] { + let mut fixture = manual_worker_assignment_fixture().await; + fixture.runtime.workdir_session_factory = Some(Arc::new(move |_| { + Err(workdir::WorkdirError::Unavailable( + "/host/private body-secret proof-secret key-secret credential-secret".into(), + ) + .with_failure_reason(reason)) + })); + fixture + .api + .runtime + .register_or_replace(fixture.runtime.clone()); + 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 path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-session/operations"); + let request = runtime_source_request(&identity, Some(&fixture.worker.worker_id), "POST", &path, serde_json::to_vec(&json!({ + "target_workdir": "checkout", + "operation": {"operation": "read", "request": {"path":"README.md", "offset":0, "limit":10, "max_bytes":1024}} + })).unwrap()); + let (status, body, events) = + request_with_logs(build_router(fixture.api.clone()), request).await; + assert_eq!(status, StatusCode::BAD_GATEWAY, "{body}"); + assert_eq!( + body["message"], + "workdir_session_open_failed: Workdir session is unavailable" + ); + assert!(body.get("failure_reason").is_none(), "{body}"); + assert!(!body.to_string().contains("secret")); + assert_eq!(events.len(), 1, "{events:?}"); + assert_eq!(events[0]["workdir_stage"], "session_open"); + assert_eq!(events[0]["failure_reason"], reason.as_str()); + assert_eq!(events[0]["failure_stage"], reason.stage().as_str()); + assert_eq!(events[0]["message"], "Workdir operation failed"); + assert_eq!(events[0]["diagnostics"], "[]"); + } +} From 60eabea19adaf65888428b922f71179cb1eec4fe Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 06:13:16 +0900 Subject: [PATCH 4/8] fix: assert retained remote session close diagnostics --- crates/workspace-server/src/hosts.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/workspace-server/src/hosts.rs b/crates/workspace-server/src/hosts.rs index 4027b221a..18fe40e22 100644 --- a/crates/workspace-server/src/hosts.rs +++ b/crates/workspace-server/src/hosts.rs @@ -8866,7 +8866,14 @@ mod tests { }) .await .expect_err("closed session must reject local operation without another request"); - assert!(matches!(error, WorkdirError::Unavailable(_))); + assert!(matches!( + error.classification_source(), + WorkdirError::Unavailable(_) + )); + assert_eq!( + error.failure_reason(), + Some(workdir::WorkdirFailureReason::SessionClosed) + ); server.join().expect("mock remote server finished"); } From 6181426abd675f4313d6300c6a01ddeb2ccdd6df Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 06:27:54 +0900 Subject: [PATCH 5/8] fix: assert retained checkout outcome diagnostics --- crates/workspace-server/src/server.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index a1b36997f..9bd590949 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -37791,10 +37791,15 @@ mod tests { ))) .unwrap(); } + let error = write.await.unwrap().unwrap_err(); assert!(matches!( - write.await.unwrap(), - Err(workdir::WorkdirError::OutcomeUnknown(_)) + error.classification_source(), + workdir::WorkdirError::OutcomeUnknown(_) )); + assert_eq!( + error.failure_reason(), + lose_response.then_some(workdir::WorkdirFailureReason::ProviderUnavailable) + ); } } From b1361d86e9f8f6ce733f22fc4bba8c341a5a36a9 Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 06:39:54 +0900 Subject: [PATCH 6/8] fix: distinguish external Workdir admission dispatch and response failures Integrate the published T773 audit contract without overwriting operation outcomes. Preserve bounded admission, send refusal, response loss/timeout and invalid result reasons independently of audit failures and request stages. Signed HTTP/SQLite/log fault tests pass (10), API logging 14 and relevant focused assertions pass. Root check, docs, generated artifacts, fmt/diff and state-contract naming checks pass. Full six-package normal suite ran with no-fail-fast but is not green: long Unix socket fixture paths and existing shared close assertion plus environment-isolation failures are recorded in target/t774 logs. Isolated focused checks resolved the latter; socket validation requires a granted shorter test root. This is a partial checkpoint, not a reviewed completion. --- crates/workdir/src/lib.rs | 22 +++ crates/workspace-server/src/hosts.rs | 11 +- crates/workspace-server/src/server.rs | 32 +++- .../workspace_admission_tests/audit_tests.rs | 161 +++++++++++++++++- 4 files changed, 212 insertions(+), 14 deletions(-) diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index 7aecce776..c3c3d97e8 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -408,6 +408,11 @@ impl From<&str> for WorkdirDenial { pub enum WorkdirFailureReason { SessionClosed, ProviderUnavailable, + ProviderGrantExpired, + ProviderAdmissionClosed, + ProviderDispatchRejected, + ProviderResponseLost, + ProviderOperationTimeout, CommandTerminalMissing, AuditStoreUnavailable, HttpTimeout, @@ -440,6 +445,11 @@ impl WorkdirFailureReason { match self { Self::SessionClosed => "session_closed", Self::ProviderUnavailable => "provider_unavailable", + Self::ProviderGrantExpired => "provider_grant_expired", + Self::ProviderAdmissionClosed => "provider_admission_closed", + Self::ProviderDispatchRejected => "provider_dispatch_rejected", + Self::ProviderResponseLost => "provider_response_lost", + Self::ProviderOperationTimeout => "provider_operation_timeout", Self::CommandTerminalMissing => "command_terminal_missing", Self::AuditStoreUnavailable => "audit_store_unavailable", Self::HttpTimeout => "http_timeout", @@ -474,6 +484,10 @@ impl WorkdirFailureReason { match self { Self::SessionClosed => Stage::Session, Self::ProviderUnavailable => Stage::Provider, + Self::ProviderGrantExpired | Self::ProviderAdmissionClosed => Stage::ProviderAdmission, + Self::ProviderDispatchRejected => Stage::ProviderDispatch, + Self::ProviderResponseLost | Self::ProviderOperationTimeout => Stage::ProviderResponse, + Self::AuditStoreUnavailable => Stage::Audit, Self::CommandTerminalMissing => Stage::Command, Self::HttpTimeout | Self::HttpConnect | Self::HttpTransfer => Stage::Http, Self::HttpResponseInvalid @@ -505,6 +519,10 @@ impl WorkdirFailureReason { pub enum WorkdirFailureStage { Session, Provider, + ProviderAdmission, + ProviderDispatch, + ProviderResponse, + Audit, Command, Http, ResponseValidation, @@ -518,6 +536,10 @@ impl WorkdirFailureStage { match self { Self::Session => "session", Self::Provider => "provider", + Self::ProviderAdmission => "provider_admission", + Self::ProviderDispatch => "provider_dispatch", + Self::ProviderResponse => "provider_response", + Self::Audit => "audit", Self::Command => "command", Self::Http => "http", Self::ResponseValidation => "response_validation", diff --git a/crates/workspace-server/src/hosts.rs b/crates/workspace-server/src/hosts.rs index 7fbb8bf25..da0b02671 100644 --- a/crates/workspace-server/src/hosts.rs +++ b/crates/workspace-server/src/hosts.rs @@ -6854,9 +6854,16 @@ mod tests { .await .unwrap_err(); assert!(matches!( - error, - WorkdirError::Unavailable(message) if message == Error::DbCapacityExceeded.to_string() + error.classification_source(), + WorkdirError::Unavailable(_) )); + assert_eq!( + error.failure_reason(), + Some(workdir::WorkdirFailureReason::AuthorizationUnavailable) + ); + let transport = workdir::http::WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.message, "Workdir session is unavailable"); + assert!(!format!("{error:?}").contains(&Error::DbCapacityExceeded.to_string())); drop(release); executor.drain().await.unwrap(); } diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 753526c71..11a33bbc6 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -1062,7 +1062,8 @@ impl ExternalProviderWorkdirSession { { return Err(workdir::WorkdirError::Unavailable( "External Workdir grant has expired".to_string(), - )); + ) + .with_failure_reason(workdir::WorkdirFailureReason::ProviderGrantExpired)); } // Cancellation must reach the provider even when waiting output readers // occupy every data-operation slot. The socket separately bounds control requests. @@ -1079,6 +1080,7 @@ impl ExternalProviderWorkdirSession { workdir::WorkdirError::Unavailable( "External Workdir provider admission is closed".to_string(), ) + .with_failure_reason(workdir::WorkdirFailureReason::ProviderAdmissionClosed) })?, ) }; @@ -1089,7 +1091,8 @@ impl ExternalProviderWorkdirSession { { return Err(workdir::WorkdirError::Unavailable( "External Workdir grant has expired".to_string(), - )); + ) + .with_failure_reason(workdir::WorkdirFailureReason::ProviderGrantExpired)); } let operation_id = ExternalWorkdirOperationId::new(format!("op-{}", Uuid::now_v7())) .map_err(workdir::WorkdirError::InvalidArgument)?; @@ -1134,7 +1137,7 @@ impl ExternalProviderWorkdirSession { ) .await; return Err(workdir::WorkdirError::NotDispatched - .with_failure_reason(workdir::WorkdirFailureReason::ProviderUnavailable)); + .with_failure_reason(workdir::WorkdirFailureReason::ProviderDispatchRejected)); } let (result, stage) = match tokio::time::timeout(operation_timeout, receive).await { Ok(result) => { @@ -1148,7 +1151,8 @@ impl ExternalProviderWorkdirSession { Err(workdir::WorkdirError::OutcomeUnknown( "External Workdir provider response lost; inspect before retrying" .to_string(), - )), + ) + .with_failure_reason(workdir::WorkdirFailureReason::ProviderResponseLost)), Stage::ResponseUnknown, ), } @@ -1159,7 +1163,8 @@ impl ExternalProviderWorkdirSession { Err(workdir::WorkdirError::OutcomeUnknown( "External Workdir provider response timed out; inspect before retrying" .to_string(), - )), + ) + .with_failure_reason(workdir::WorkdirFailureReason::ProviderOperationTimeout)), Stage::ResponseUnknown, ) } @@ -1328,6 +1333,7 @@ impl ExternalProviderWorkdirSession { workdir::WorkdirError::Unavailable(format!( "External Workdir provider returned a mismatched result; expected {expected}" )) + .with_failure_reason(workdir::WorkdirFailureReason::OperationResultMismatch) } } @@ -1457,7 +1463,8 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { } Ok(_) if mutation => Err(workdir::WorkdirError::OutcomeUnknown( "mismatched checkout mutation response".into(), - )), + ) + .with_failure_reason(workdir::WorkdirFailureReason::OperationResultMismatch)), Ok(_) => Err(Self::mismatch("checkout_execute")), Err(error) if mutation @@ -37863,10 +37870,19 @@ mod tests { ))) .unwrap(); } + let error = write.await.unwrap().unwrap_err(); assert!(matches!( - write.await.unwrap(), - Err(workdir::WorkdirError::OutcomeUnknown(_)) + error.classification_source(), + workdir::WorkdirError::OutcomeUnknown(_) )); + assert_eq!( + error.failure_reason(), + Some(if lose_response { + workdir::WorkdirFailureReason::ProviderResponseLost + } else { + workdir::WorkdirFailureReason::OperationResultMismatch + }) + ); } } diff --git a/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs b/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs index 212198b6f..a6f76f548 100644 --- a/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs +++ b/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs @@ -260,6 +260,9 @@ impl AuditHttpFixture { PATH_SECRET, PROVIDER_SECRET, pending.proof.as_str(), + worker_runtime::auth::RUNTIME_REQUEST_SOURCE_PROOF_HEADER, + self.fixture.identity.private_key.as_str(), + self.fixture.identity.public_key.as_str(), self.fixture._root.path().to_str().unwrap(), ] { assert!(!output.contains(secret), "sensitive data escaped: {output}"); @@ -363,13 +366,33 @@ fn assert_audit_failure( operation_id.to_owned() } -fn assert_failure_reason(events: &[Value], reason: &str) { +fn assert_failure_reason(events: &[Value], reason: &str, stage: &str) { let errors: Vec<&Value> = events .iter() .filter(|event| event["event"] == "api_error") .collect(); assert_eq!(errors.len(), 1, "{events:?}"); assert_eq!(errors[0]["failure_reason"], reason, "{events:?}"); + assert_eq!(errors[0]["failure_stage"], stage, "{events:?}"); +} + +fn assert_operational_failure( + body: &Value, + events: &[Value], + reason: &str, + stage: &str, + code: &str, + message: &str, +) { + assert_failure_reason(events, reason, stage); + assert_eq!(body["code"], code, "{body}"); + assert_eq!(body["message"], message, "{body}"); + for field in ["failure_reason", "failure_stage"] { + assert!( + body.get(field).is_none(), + "internal diagnostic escaped: {body}" + ); + } } #[tokio::test] @@ -436,7 +459,14 @@ async fn lost_write_response_after_audit_failure_stays_unknown_without_repeating // The provider applied the effect, but the server cannot infer that from response loss. drop(reply); let (body, events) = h.finish(pending, StatusCode::INTERNAL_SERVER_ERROR).await; - assert_eq!(body["code"], "outcome_unknown"); + assert_operational_failure( + &body, + &events, + "provider_response_lost", + "provider_response", + "outcome_unknown", + "Workdir operation may have effects; inspect before retrying", + ); let receipt_id = assert_audit_failure( &body, &events, @@ -467,7 +497,7 @@ async fn before_write_audit_failure_is_not_dispatched_and_has_no_provider_effect json!({"kind":"not_dispatched"}), "NotDispatched", ); - assert_failure_reason(&events, "audit_store_unavailable"); + assert_failure_reason(&events, "audit_store_unavailable", "audit"); assert!( body.get("failure_reason").is_none(), "internal reason leaked: {body}" @@ -494,12 +524,135 @@ async fn rejected_write_send_with_audit_failure_is_not_dispatched_not_unknown() json!({"kind":"not_dispatched"}), "NotDispatched", ); - assert_failure_reason(&events, "provider_unavailable"); + assert_operational_failure( + &body, + &events, + "provider_dispatch_rejected", + "provider_dispatch", + "not_dispatched", + "Workdir operation was not dispatched", + ); assert!(!h.effect_path().exists()); provider.assert_dispatches(0); h.assert_audits(&[(&receipt_id, "requested")]); } +#[tokio::test] +async fn expired_connection_grant_is_not_dispatched_and_logs_admission_reason() { + let (h, mut provider) = AuditHttpFixture::new().await; + { + let mut registry = h.fixture.api.external_workdir_providers.lock().unwrap(); + let connection = registry.connections.get_mut("external-1").unwrap(); + // Keep the persisted grant online so the real handler reaches operate's + // connection-expiry fence rather than the separate session-open fence. + Arc::get_mut(connection).unwrap().expires_at = + Some(Utc::now() - chrono::Duration::seconds(1)); + } + h.fail_audit(false); + let (body, events) = h + .finish(h.start_write(), StatusCode::SERVICE_UNAVAILABLE) + .await; + assert_operational_failure( + &body, + &events, + "provider_grant_expired", + "provider_admission", + "unavailable", + "Workdir session is unavailable", + ); + assert!(body.get("audit_failures").is_none(), "{body}"); + assert!( + events + .iter() + .all(|event| event["event"] != "external_workdir_audit_failed") + ); + assert!(!h.effect_path().exists()); + provider.assert_dispatches(0); + h.assert_audits(&[]); +} + +#[tokio::test] +async fn closed_provider_admission_is_not_dispatched_and_logs_admission_reason() { + let (h, mut provider) = AuditHttpFixture::new().await; + h.fixture + .api + .external_workdir_providers + .lock() + .unwrap() + .connection("external-1") + .unwrap() + .admission + .close(); + h.fail_audit(false); + let (body, events) = h + .finish(h.start_write(), StatusCode::SERVICE_UNAVAILABLE) + .await; + assert_operational_failure( + &body, + &events, + "provider_admission_closed", + "provider_admission", + "unavailable", + "Workdir session is unavailable", + ); + assert!(body.get("audit_failures").is_none(), "{body}"); + assert!( + events + .iter() + .all(|event| event["event"] != "external_workdir_audit_failed") + ); + assert!(!h.effect_path().exists()); + provider.assert_dispatches(0); + h.assert_audits(&[]); +} + +#[tokio::test] +async fn timed_out_write_after_audit_failure_stays_unknown_and_logs_timeout_reason() { + let (mut h, mut provider) = AuditHttpFixture::new().await; + // Keep the HTTP client's independent deadline beyond the unchanged 30s + // provider timeout. Pause only after the signed HTTP request was dispatched, + // avoiding automatic virtual-time advancement during socket/SQLite setup. + h.client = reqwest::Client::builder() + .no_proxy() + .timeout(std::time::Duration::from_secs(40)) + .build() + .unwrap(); + h.fail_audit(false); + let mut pending = h.start_write(); + let (dispatched_id, reply) = provider.receive_write(&mut pending).await; + h.apply_provider_write(); + tokio::time::pause(); + tokio::time::advance(std::time::Duration::from_secs(31)).await; + tokio::time::resume(); + let (body, events) = h.finish(pending, StatusCode::INTERNAL_SERVER_ERROR).await; + assert_operational_failure( + &body, + &events, + "provider_operation_timeout", + "provider_response", + "outcome_unknown", + "Workdir operation may have effects; inspect before retrying", + ); + let receipt_id = assert_audit_failure( + &body, + &events, + "response_unknown", + "ResponseUnknown", + json!({"kind":"unknown"}), + "Unknown", + ); + assert_eq!(receipt_id, dispatched_id); + assert!(reply.is_closed(), "timeout retained its response receiver"); + assert!(matches!( + provider.receiver.try_recv().unwrap(), + ExternalProviderCommand::Cancel { operation_id } + if operation_id.as_str() == dispatched_id.as_str() + )); + assert_eq!(std::fs::read(h.effect_path()).unwrap(), WRITE_SECRET); + provider.assert_dispatches(1); + h.assert_audits(&[(&dispatched_id, "requested")]); +} + #[tokio::test] async fn successful_write_persists_requested_and_completed_with_same_operation_id() { let (h, mut provider) = AuditHttpFixture::new().await; From ca6d4b4b728efa9b9fdaf0d592a061a2ccc58252 Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 20:39:46 +0900 Subject: [PATCH 7/8] fix: audit validated external workdir operation outcomes --- crates/workspace-server/src/server.rs | 593 ++++++++++-------- .../workspace_admission_tests/audit_tests.rs | 202 +++++- 2 files changed, 537 insertions(+), 258 deletions(-) diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 11a33bbc6..f9377801c 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -1046,10 +1046,17 @@ impl ExternalProviderWorkdirSession { }) } - async fn operate( + async fn operate( &self, operation: WorkdirSessionOperation, - ) -> std::result::Result { + validate: F, + ) -> std::result::Result + where + T: Send, + F: FnOnce(std::result::Result) -> Fut + + Send, + Fut: std::future::Future> + Send, + { let operation_kind = external_workdir_operation_kind(&operation); let operation_timeout = external_workdir_operation_timeout(&operation); if self.closed.load(Ordering::Acquire) { @@ -1169,6 +1176,12 @@ impl ExternalProviderWorkdirSession { ) } }; + // Provider dispatch is finished; validation may issue bounded command cleanup + // and must not retain a data-operation slot while awaiting another operation. + drop(_admission); + // A received response is not evidence of operation success until its requested + // type and semantics (including mutation uncertainty) have been checked. + let result = validate(result).await; let outcome = match &result { Ok(_) => Outcome::Succeeded, Err(error) @@ -1275,9 +1288,15 @@ impl ExternalProviderWorkdirSession { // Do not acquire the output lock until cancellation has unblocked any reader. // The reader records its last known cursor before cleanup acknowledges bytes. let _ = self - .operate(WorkdirSessionOperation::CommandCancel( - command.handle.clone(), - )) + .operate( + WorkdirSessionOperation::CommandCancel(command.handle.clone()), + |result| async move { + match result? { + WorkdirSessionOperationResult::CommandCancel => Ok(()), + _ => Err(Self::mismatch("command_cancel")), + } + }, + ) .await; let mut cursor = tokio::time::timeout(std::time::Duration::from_secs(30), command.cursor.lock()) @@ -1292,31 +1311,31 @@ impl ExternalProviderWorkdirSession { } let drain = async { loop { - match self - .operate(WorkdirSessionOperation::CommandOutput( - workdir::CommandOutputRequest { + let output = self + .operate( + WorkdirSessionOperation::CommandOutput(workdir::CommandOutputRequest { handle: command.handle.clone(), cursor: *cursor, limit: workdir::MAX_COMMAND_READ_BYTES, wait: true, + }), + |result| async move { + match result? { + WorkdirSessionOperationResult::CommandOutput(output) => Ok(output), + _ => Err(Self::mismatch("command_output")), + } }, - )) - .await? - { - WorkdirSessionOperationResult::CommandOutput(output) => { - *cursor = output.next_cursor; - if output.terminal.is_some() { - *command.terminal_at.lock().unwrap() = Some(std::time::Instant::now()); - return Ok(()); - } - if output.chunks.is_empty() { - return Err(workdir::WorkdirError::Unavailable( - "External Workdir provider did not confirm command termination" - .to_string(), - )); - } - } - _ => return Err(Self::mismatch("command_output")), + ) + .await?; + *cursor = output.next_cursor; + if output.terminal.is_some() { + *command.terminal_at.lock().unwrap() = Some(std::time::Instant::now()); + return Ok(()); + } + if output.chunks.is_empty() { + return Err(workdir::WorkdirError::Unavailable( + "External Workdir provider did not confirm command termination".to_string(), + )); } } }; @@ -1384,46 +1403,55 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { request: workdir::WorkdirScopeAuthorizationRequest, ) -> std::result::Result<(), workdir::WorkdirError> { self.ensure_capability(workdir::WorkdirSessionCapability::Read)?; - match self - .operate(WorkdirSessionOperation::AuthorizeScope(request)) - .await? - { - WorkdirSessionOperationResult::AuthorizeScope => Ok(()), - _ => Err(Self::mismatch("authorize_scope")), - } + self.operate( + WorkdirSessionOperation::AuthorizeScope(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::AuthorizeScope => Ok(()), + _ => Err(Self::mismatch("authorize_scope")), + } + }, + ) + .await } async fn scope_rules_overlap( &self, request: workdir::WorkdirScopeOverlapRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Read)?; - match self - .operate(WorkdirSessionOperation::ScopeRulesOverlap(request)) - .await? - { - WorkdirSessionOperationResult::ScopeRulesOverlap { overlaps } => Ok(overlaps), - _ => Err(Self::mismatch("scope_rules_overlap")), - } + self.operate( + WorkdirSessionOperation::ScopeRulesOverlap(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::ScopeRulesOverlap { overlaps } => Ok(overlaps), + _ => Err(Self::mismatch("scope_rules_overlap")), + } + }, + ) + .await } async fn checkout_observe( &self, path: workdir::WorkdirPath, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Read)?; - match self - .operate(WorkdirSessionOperation::CheckoutObserve(path.clone())) - .await? - { - WorkdirSessionOperationResult::CheckoutObserve(mut value) - if value.path == path - && !value.validator.is_empty() - && value.validator.len() <= 256 => - { - value.capabilities = value.capabilities.intersection(self.capabilities()); - Ok(value) - } - _ => Err(Self::mismatch("checkout_observe")), - } + self.operate( + WorkdirSessionOperation::CheckoutObserve(path.clone()), + |result| async move { + match result? { + WorkdirSessionOperationResult::CheckoutObserve(mut value) + if value.path == path + && !value.validator.is_empty() + && value.validator.len() <= 256 => + { + value.capabilities = value.capabilities.intersection(self.capabilities()); + Ok(value) + } + _ => Err(Self::mismatch("checkout_observe")), + } + }, + ) + .await } async fn checkout_execute( &self, @@ -1433,63 +1461,67 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { self.ensure_capability(cap)?; let target = request.target.clone(); let mutation = cap != workdir::WorkdirSessionCapability::Read; - match self - .operate(WorkdirSessionOperation::CheckoutExecute(request)) - .await - { - Ok(WorkdirSessionOperationResult::CheckoutExecute(mut value)) - if value.observation.path == target - && !value.observation.validator.is_empty() - && value.observation.validator.len() <= 256 - && matches!( - (&value.output, cap), - ( - workdir::CheckoutOutput::Read(_), - workdir::WorkdirSessionCapability::Read - ) | ( - workdir::CheckoutOutput::Write(_), - workdir::WorkdirSessionCapability::Write - ) | ( - workdir::CheckoutOutput::Edit(_), - workdir::WorkdirSessionCapability::Edit - ) - ) => - { - value.observation.capabilities = value - .observation - .capabilities - .intersection(self.capabilities()); - Ok(value) - } - Ok(_) if mutation => Err(workdir::WorkdirError::OutcomeUnknown( - "mismatched checkout mutation response".into(), - ) - .with_failure_reason(workdir::WorkdirFailureReason::OperationResultMismatch)), - Ok(_) => Err(Self::mismatch("checkout_execute")), - Err(error) - if mutation - && matches!( - error.classification_source(), - workdir::WorkdirError::Unavailable(_) | workdir::WorkdirError::Transport(_) - ) => - { - let outcome = workdir::WorkdirError::OutcomeUnknown( - "checkout provider response lost; inspect effects before retry".into(), - ); - let outcome = match error.failure_reason() { - Some(reason) => outcome.with_failure_reason(reason), - None => outcome, - }; - Err(match error.denial_reason() { - Some(reason) => workdir::WorkdirError::DenialContext { - reason, - source: Box::new(outcome), - }, - None => outcome, - }) - } - Err(error) => Err(error), - } + self.operate( + WorkdirSessionOperation::CheckoutExecute(request), + |result| async move { + match result { + Ok(WorkdirSessionOperationResult::CheckoutExecute(mut value)) + if value.observation.path == target + && !value.observation.validator.is_empty() + && value.observation.validator.len() <= 256 + && matches!( + (&value.output, cap), + ( + workdir::CheckoutOutput::Read(_), + workdir::WorkdirSessionCapability::Read + ) | ( + workdir::CheckoutOutput::Write(_), + workdir::WorkdirSessionCapability::Write + ) | ( + workdir::CheckoutOutput::Edit(_), + workdir::WorkdirSessionCapability::Edit + ) + ) => + { + value.observation.capabilities = value + .observation + .capabilities + .intersection(self.capabilities()); + Ok(value) + } + Ok(_) if mutation => Err(workdir::WorkdirError::OutcomeUnknown( + "mismatched checkout mutation response".into(), + ) + .with_failure_reason(workdir::WorkdirFailureReason::OperationResultMismatch)), + Ok(_) => Err(Self::mismatch("checkout_execute")), + Err(error) + if mutation + && matches!( + error.classification_source(), + workdir::WorkdirError::Unavailable(_) + | workdir::WorkdirError::Transport(_) + ) => + { + let outcome = workdir::WorkdirError::OutcomeUnknown( + "checkout provider response lost; inspect effects before retry".into(), + ); + let outcome = match error.failure_reason() { + Some(reason) => outcome.with_failure_reason(reason), + None => outcome, + }; + Err(match error.denial_reason() { + Some(reason) => workdir::WorkdirError::DenialContext { + reason, + source: Box::new(outcome), + }, + None => outcome, + }) + } + Err(error) => Err(error), + } + }, + ) + .await } async fn checkout_search( &self, @@ -1505,108 +1537,153 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { Op::Grep(_) => Cap::Grep, }; self.ensure_capability(cap)?; - match self - .operate(WorkdirSessionOperation::CheckoutSearch(request)) - .await? - { - WorkdirSessionOperationResult::CheckoutSearch(value) - if matches!( - (&value, cap), - (Result::List(_), Cap::Read) - | (Result::Glob(_), Cap::Glob) - | (Result::Grep(_), Cap::Grep) - ) => - { - Ok(value) - } - _ => Err(Self::mismatch("checkout_search")), - } + self.operate( + WorkdirSessionOperation::CheckoutSearch(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::CheckoutSearch(value) + if matches!( + (&value, cap), + (Result::List(_), Cap::Read) + | (Result::Glob(_), Cap::Glob) + | (Result::Grep(_), Cap::Grep) + ) => + { + Ok(value) + } + _ => Err(Self::mismatch("checkout_search")), + } + }, + ) + .await } async fn stat( &self, request: workdir::StatRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Read)?; - match self.operate(WorkdirSessionOperation::Stat(request)).await? { - WorkdirSessionOperationResult::Stat(value) => Ok(value), - _ => Err(Self::mismatch("stat")), - } + self.operate( + WorkdirSessionOperation::Stat(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::Stat(value) => Ok(value), + _ => Err(Self::mismatch("stat")), + } + }, + ) + .await } async fn read( &self, request: workdir::ReadRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Read)?; - match self.operate(WorkdirSessionOperation::Read(request)).await? { - WorkdirSessionOperationResult::Read(value) => Ok(value), - _ => Err(Self::mismatch("read")), - } + self.operate( + WorkdirSessionOperation::Read(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::Read(value) => Ok(value), + _ => Err(Self::mismatch("read")), + } + }, + ) + .await } async fn read_bytes( &self, request: workdir::ReadBytesRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Read)?; - match self - .operate(WorkdirSessionOperation::ReadBytes(request)) - .await? - { - WorkdirSessionOperationResult::ReadBytes(value) => Ok(value), - _ => Err(Self::mismatch("read_bytes")), - } + self.operate( + WorkdirSessionOperation::ReadBytes(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::ReadBytes(value) => Ok(value), + _ => Err(Self::mismatch("read_bytes")), + } + }, + ) + .await } async fn write( &self, request: workdir::WriteRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Write)?; - match self - .operate(WorkdirSessionOperation::Write(request)) - .await? - { - WorkdirSessionOperationResult::Write(value) => Ok(value), - _ => Err(Self::mismatch("write")), - } + self.operate( + WorkdirSessionOperation::Write(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::Write(value) => Ok(value), + _ => Err(Self::mismatch("write")), + } + }, + ) + .await } async fn edit( &self, request: workdir::EditRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Edit)?; - match self.operate(WorkdirSessionOperation::Edit(request)).await? { - WorkdirSessionOperationResult::Edit(value) => Ok(value), - _ => Err(Self::mismatch("edit")), - } + self.operate( + WorkdirSessionOperation::Edit(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::Edit(value) => Ok(value), + _ => Err(Self::mismatch("edit")), + } + }, + ) + .await } async fn list( &self, request: workdir::ListRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Read)?; - match self.operate(WorkdirSessionOperation::List(request)).await? { - WorkdirSessionOperationResult::List(value) => Ok(value), - _ => Err(Self::mismatch("list")), - } + self.operate( + WorkdirSessionOperation::List(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::List(value) => Ok(value), + _ => Err(Self::mismatch("list")), + } + }, + ) + .await } async fn glob( &self, request: workdir::GlobRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Glob)?; - match self.operate(WorkdirSessionOperation::Glob(request)).await? { - WorkdirSessionOperationResult::Glob(value) => Ok(value), - _ => Err(Self::mismatch("glob")), - } + self.operate( + WorkdirSessionOperation::Glob(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::Glob(value) => Ok(value), + _ => Err(Self::mismatch("glob")), + } + }, + ) + .await } async fn grep( &self, request: workdir::GrepRequest, ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Grep)?; - match self.operate(WorkdirSessionOperation::Grep(request)).await? { - WorkdirSessionOperationResult::Grep(value) => Ok(value), - _ => Err(Self::mismatch("grep")), - } + self.operate( + WorkdirSessionOperation::Grep(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::Grep(value) => Ok(value), + _ => Err(Self::mismatch("grep")), + } + }, + ) + .await } async fn start_command( &self, @@ -1631,62 +1708,68 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { "External Workdir active command limit reached".to_string(), )); } - match self - .operate(WorkdirSessionOperation::CommandStart(request)) - .await? - { - WorkdirSessionOperationResult::CommandStart(provider_handle) => { - let duplicate_handles = { - let commands = self.active_commands.lock().await; - commands - .iter() - .filter(|(_, active)| active.handle == provider_handle) - .map(|(external, _)| external.clone()) - .collect::>() - }; - if !duplicate_handles.is_empty() { - // A provider handle must identify exactly one command in one - // generation. Remove every ambiguous route before attempting - // cleanup so no caller can address the wrong process. - let mut commands = self.active_commands.lock().await; - let mut quarantined = None; - for external_handle in duplicate_handles { - if let Some(command) = commands.remove(&external_handle) { - quarantined.get_or_insert(command); + self.operate( + WorkdirSessionOperation::CommandStart(request), + |result| async move { + match result? { + WorkdirSessionOperationResult::CommandStart(provider_handle) => { + let duplicate_handles = { + let commands = self.active_commands.lock().await; + commands + .iter() + .filter(|(_, active)| active.handle == provider_handle) + .map(|(external, _)| external.clone()) + .collect::>() + }; + if !duplicate_handles.is_empty() { + // A provider handle must identify exactly one command in one + // generation. Remove every ambiguous route before attempting + // cleanup so no caller can address the wrong process. + let mut commands = self.active_commands.lock().await; + let mut quarantined = None; + for external_handle in duplicate_handles { + if let Some(command) = commands.remove(&external_handle) { + quarantined.get_or_insert(command); + } + } + let quarantine_handle = workdir::CommandHandle(format!( + "quarantined-command-{}", + Uuid::now_v7() + )); + let command = quarantined.expect("duplicate command route disappeared"); + *command.terminal_at.lock().unwrap() = None; + commands.insert(quarantine_handle.0.clone(), command.clone()); + drop(commands); + if self + .terminate_provider_command(command.clone(), true) + .await + .is_ok() + { + self.remove_active_command_if_matches(&quarantine_handle, &command) + .await; + } else { + // An earlier read of the reused handle cannot establish that + // the newly started, ambiguous process has been cleaned up. + *command.terminal_at.lock().unwrap() = None; + } + return Err(workdir::WorkdirError::Unavailable( + "External Workdir provider reused an active command handle" + .to_string(), + )); } + let external_handle = + workdir::CommandHandle(format!("external-command-{}", Uuid::now_v7())); + self.active_commands.lock().await.insert( + external_handle.0.clone(), + Arc::new(ExternalProviderCommandState::new(provider_handle)), + ); + Ok(external_handle) } - let quarantine_handle = - workdir::CommandHandle(format!("quarantined-command-{}", Uuid::now_v7())); - let command = quarantined.expect("duplicate command route disappeared"); - *command.terminal_at.lock().unwrap() = None; - commands.insert(quarantine_handle.0.clone(), command.clone()); - drop(commands); - if self - .terminate_provider_command(command.clone(), true) - .await - .is_ok() - { - self.remove_active_command_if_matches(&quarantine_handle, &command) - .await; - } else { - // An earlier read of the reused handle cannot establish that - // the newly started, ambiguous process has been cleaned up. - *command.terminal_at.lock().unwrap() = None; - } - return Err(workdir::WorkdirError::Unavailable( - "External Workdir provider reused an active command handle".to_string(), - )); + _ => Err(Self::mismatch("command_start")), } - let external_handle = - workdir::CommandHandle(format!("external-command-{}", Uuid::now_v7())); - self.active_commands.lock().await.insert( - external_handle.0.clone(), - Arc::new(ExternalProviderCommandState::new(provider_handle)), - ); - Ok(external_handle) - } - _ => Err(Self::mismatch("command_start")), - } + }, + ) + .await } async fn command_status( &self, @@ -1694,13 +1777,16 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { ) -> std::result::Result { self.ensure_capability(workdir::WorkdirSessionCapability::Command)?; let provider_handle = self.provider_command_handle(&external_handle).await?; - match self - .operate(WorkdirSessionOperation::CommandStatus(provider_handle)) - .await? - { - WorkdirSessionOperationResult::CommandStatus(status) => Ok(status), - _ => Err(Self::mismatch("command_status")), - } + self.operate( + WorkdirSessionOperation::CommandStatus(provider_handle), + |result| async move { + match result? { + WorkdirSessionOperationResult::CommandStatus(status) => Ok(status), + _ => Err(Self::mismatch("command_status")), + } + }, + ) + .await } async fn command_output( &self, @@ -1709,29 +1795,33 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { self.ensure_capability(workdir::WorkdirSessionCapability::Command)?; let external_handle = request.handle.clone(); let command = self.provider_command(&external_handle).await?; + let command = &command; let mut cursor = command.cursor.lock().await; request.handle = command.handle.clone(); // If the response is lost, this request cursor still replays the provider page. - match self - .operate(WorkdirSessionOperation::CommandOutput(request)) - .await - { - Ok(WorkdirSessionOperationResult::CommandOutput(output)) => { - *cursor = output.next_cursor; - if output.terminal.is_some() { - command - .terminal_at - .lock() - .unwrap() - .get_or_insert_with(std::time::Instant::now); + self.operate( + WorkdirSessionOperation::CommandOutput(request), + |result| async move { + match result { + Ok(WorkdirSessionOperationResult::CommandOutput(output)) => { + *cursor = output.next_cursor; + if output.terminal.is_some() { + command + .terminal_at + .lock() + .unwrap() + .get_or_insert_with(std::time::Instant::now); + } + drop(cursor); + self.prune_terminal_commands().await; + Ok(output) + } + Ok(_) => Err(Self::mismatch("command_output")), + Err(error) => Err(error), } - drop(cursor); - self.prune_terminal_commands().await; - Ok(output) - } - Ok(_) => Err(Self::mismatch("command_output")), - Err(error) => Err(error), - } + }, + ) + .await } async fn cancel_command( &self, @@ -1739,13 +1829,16 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { ) -> std::result::Result<(), workdir::WorkdirError> { self.ensure_capability(workdir::WorkdirSessionCapability::Command)?; let provider_handle = self.provider_command_handle(&external_handle).await?; - match self - .operate(WorkdirSessionOperation::CommandCancel(provider_handle)) - .await? - { - WorkdirSessionOperationResult::CommandCancel => Ok(()), - _ => Err(Self::mismatch("command_cancel")), - } + self.operate( + WorkdirSessionOperation::CommandCancel(provider_handle), + |result| async move { + match result? { + WorkdirSessionOperationResult::CommandCancel => Ok(()), + _ => Err(Self::mismatch("command_cancel")), + } + }, + ) + .await } async fn close(&self) -> std::result::Result<(), workdir::WorkdirError> { let _close_guard = self.close_lock.lock().await; diff --git a/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs b/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs index a6f76f548..bdf75c837 100644 --- a/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs +++ b/crates/workspace-server/src/server/tests/workspace_admission_tests/audit_tests.rs @@ -46,7 +46,10 @@ struct ProviderProbe { } impl ProviderProbe { - async fn receive_write(&mut self, pending: &mut PendingHttpWrite) -> (String, ProviderReply) { + async fn receive_operation( + &mut self, + pending: &mut PendingHttpWrite, + ) -> (String, WorkdirSessionOperation, ProviderReply) { let command = tokio::time::timeout(BOUND, async { tokio::select! { command = self.receiver.recv() => command.expect("provider channel closed"), @@ -66,6 +69,11 @@ impl ProviderProbe { panic!("expected write dispatch, received cancellation") }; self.dispatches += 1; + (operation_id.as_str().to_owned(), operation, response) + } + + async fn receive_write(&mut self, pending: &mut PendingHttpWrite) -> (String, ProviderReply) { + let (operation_id, operation, response) = self.receive_operation(pending).await; let WorkdirSessionOperation::Write(request) = operation else { panic!("expected mutation, got {operation:?}") }; @@ -75,7 +83,7 @@ impl ProviderProbe { ); assert_eq!(request.content, WRITE_SECRET); assert_eq!(request.expected_hash, None); - (operation_id.as_str().to_owned(), response) + (operation_id, response) } fn assert_dispatches(&mut self, expected: usize) { @@ -200,14 +208,18 @@ impl AuditHttpFixture { } fn start_write(&self) -> PendingHttpWrite { + self.start_operation(WorkdirSessionOperation::Write(workdir::WriteRequest { + path: workdir::WorkdirPath::new(PATH_SECRET).unwrap(), + content: WRITE_SECRET.to_vec(), + expected_hash: None, + })) + } + + fn start_operation(&self, operation: WorkdirSessionOperation) -> PendingHttpWrite { let path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-session/operations"); let body = serde_json::to_vec(&WorkspaceWorkdirSessionOperationRequest { target_workdir: "attachment".into(), - operation: WorkdirSessionOperation::Write(workdir::WriteRequest { - path: workdir::WorkdirPath::new(PATH_SECRET).unwrap(), - content: WRITE_SECRET.to_vec(), - expected_hash: None, - }), + operation, }) .unwrap(); let request = self.fixture.request( @@ -259,6 +271,7 @@ impl AuditHttpFixture { encoded_content.as_str(), PATH_SECRET, PROVIDER_SECRET, + PROVIDER_SECRET.trim_start_matches('/'), pending.proof.as_str(), worker_runtime::auth::RUNTIME_REQUEST_SOURCE_PROOF_HEADER, self.fixture.identity.private_key.as_str(), @@ -276,6 +289,10 @@ impl AuditHttpFixture { } fn assert_audits(&self, expected: &[(&str, &str)]) { + self.assert_operation_audits(expected, "write"); + } + + fn assert_operation_audits(&self, expected: &[(&str, &str)], operation: &str) { let connection = rusqlite::Connection::open(&self.fixture.api.config.database_path).unwrap(); let mut statement = connection @@ -298,7 +315,10 @@ impl AuditHttpFixture { .collect(); assert_eq!(actual, expected, "persisted audits: {rows:?}"); for (_, _, summary) in rows { - assert!(summary.contains("operation_kind=write"), "{summary}"); + assert!( + summary.contains(&format!("operation_kind={operation}")), + "{summary}" + ); for secret in [ AUDIT_SECRET, PATH_SECRET, @@ -697,3 +717,169 @@ async fn provider_reported_unknown_preserves_received_response_and_unknown_effec provider.assert_dispatches(1); h.assert_audits(&[(&dispatched_id, "requested")]); } + +#[derive(Clone, Copy, Debug)] +enum CheckoutReplyFault { + WrongOperation, + WrongPath, + EmptyValidator, + WrongOutput, + ProviderUnavailable, + ProviderTransport, +} + +async fn assert_checkout_unknown_audit(fault: CheckoutReplyFault, fail_completion: bool) { + let (h, mut provider) = AuditHttpFixture::new().await; + if fail_completion { + h.fail_audit(false); + } + let target = workdir::WorkdirPath::new(PATH_SECRET).unwrap(); + let mut pending = h.start_operation(WorkdirSessionOperation::CheckoutExecute( + workdir::CheckoutRequest { + target: target.clone(), + validator: vec![1], + operation: workdir::CheckoutOperation::Write { + content: WRITE_SECRET.to_vec(), + expected_hash: [0; 32], + }, + }, + )); + let (operation_id, operation, reply) = provider.receive_operation(&mut pending).await; + let WorkdirSessionOperation::CheckoutExecute(request) = operation else { + panic!("expected checkout mutation, got {operation:?}"); + }; + assert_eq!(request.target, target); + assert_eq!(request.validator, vec![1]); + assert!( + matches!(request.operation, workdir::CheckoutOperation::Write { ref content, .. } if content == WRITE_SECRET) + ); + h.apply_provider_write(); + let result = match fault { + CheckoutReplyFault::WrongOperation => Ok(write_result()), + CheckoutReplyFault::ProviderUnavailable => Err(WorkdirTransportError::from_workdir_error( + &workdir::WorkdirError::Unavailable(PROVIDER_SECRET.into()), + )), + CheckoutReplyFault::ProviderTransport => Err(WorkdirTransportError::from_workdir_error( + &workdir::WorkdirError::Transport(PROVIDER_SECRET.into()), + )), + _ => { + let mut result = workdir::CheckoutResult { + observation: workdir::CheckoutObservation { + path: target, + validator: vec![2], + kind: workdir::EntryKind::File, + size: WRITE_SECRET.len() as u64, + capabilities: workdir::WorkdirSessionCapabilities::READ_WRITE, + }, + output: workdir::CheckoutOutput::Write(workdir::WriteResult { + bytes_written: WRITE_SECRET.len(), + created: false, + }), + }; + match fault { + CheckoutReplyFault::WrongPath => { + result.observation.path = + workdir::WorkdirPath::new(PROVIDER_SECRET.trim_start_matches('/')).unwrap() + } + CheckoutReplyFault::EmptyValidator => result.observation.validator.clear(), + CheckoutReplyFault::WrongOutput => { + result.output = workdir::CheckoutOutput::Edit(workdir::EditResult { + replacements: 1, + content_hash: [0; 32], + bytes_written: 1, + }) + } + _ => unreachable!(), + } + Ok(WorkdirSessionOperationResult::CheckoutExecute(result)) + } + }; + reply.send(result).unwrap(); + let (body, events) = h.finish(pending, StatusCode::INTERNAL_SERVER_ERROR).await; + assert_eq!(body["code"], "outcome_unknown", "{fault:?}: {body}"); + assert_eq!( + body["message"], + "Workdir operation may have effects; inspect before retrying" + ); + if !matches!( + fault, + CheckoutReplyFault::ProviderUnavailable | CheckoutReplyFault::ProviderTransport + ) { + assert_failure_reason(&events, "operation_result_mismatch", "response_validation"); + } + if fail_completion { + assert_eq!( + body["audit_failures"], + json!([{ + "operation_id": operation_id, + "stage": "response_received", + "reason": "audit_store_unavailable", + "operation_outcome": {"kind":"unknown"}, + }]), + "{fault:?}: {body}" + ); + let audit_logs: Vec<_> = events + .iter() + .filter(|e| e["event"] == "external_workdir_audit_failed") + .collect(); + assert_eq!(audit_logs.len(), 1, "{events:?}"); + let event = audit_logs[0]; + assert_eq!(event["operation_id"], operation_id); + assert_eq!(event["operation"], "checkout_execute"); + assert_eq!(event["audit_stage"], "ResponseReceived"); + assert_eq!(event["operation_outcome"], "Unknown"); + h.assert_operation_audits(&[(&operation_id, "requested")], "checkout_execute"); + } else { + assert!(body.get("audit_failures").is_none(), "{body}"); + assert!( + events + .iter() + .all(|e| e["event"] != "external_workdir_audit_failed") + ); + h.assert_operation_audits( + &[ + (&operation_id, "requested"), + (&operation_id, "outcome_unknown"), + ], + "checkout_execute", + ); + } + assert_eq!(std::fs::read(h.effect_path()).unwrap(), WRITE_SECRET); + provider.assert_dispatches(1); +} + +#[tokio::test] +async fn checkout_invalid_reply_records_unknown_in_healthy_completion_audit() { + for fault in [ + CheckoutReplyFault::WrongOperation, + CheckoutReplyFault::WrongPath, + CheckoutReplyFault::EmptyValidator, + CheckoutReplyFault::WrongOutput, + ] { + assert_checkout_unknown_audit(fault, false).await; + } +} + +#[tokio::test] +async fn checkout_invalid_reply_records_unknown_in_failed_completion_receipt_and_log() { + for fault in [ + CheckoutReplyFault::WrongOperation, + CheckoutReplyFault::WrongPath, + CheckoutReplyFault::EmptyValidator, + CheckoutReplyFault::WrongOutput, + ] { + assert_checkout_unknown_audit(fault, true).await; + } +} + +#[tokio::test] +async fn checkout_uncertain_reply_normalization_matches_audit_outcome() { + for fault in [ + CheckoutReplyFault::ProviderUnavailable, + CheckoutReplyFault::ProviderTransport, + ] { + for fail_completion in [false, true] { + assert_checkout_unknown_audit(fault, fail_completion).await; + } + } +} From 5af9dc8e4aab4b8beca5a08e63b83ee08a131aa5 Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 23:57:01 +0900 Subject: [PATCH 8/8] test: focus checkout hardlink coverage on alias and symlink contracts --- crates/workdir/src/checkout.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/workdir/src/checkout.rs b/crates/workdir/src/checkout.rs index 2b7c243fd..52c8a91ae 100644 --- a/crates/workdir/src/checkout.rs +++ b/crates/workdir/src/checkout.rs @@ -556,15 +556,13 @@ mod tests { )); } #[tokio::test] - async fn checkout_hardlink_save_isolates_alias_and_rejects_symlinks_special_files() { + async fn checkout_hardlink_save_isolates_alias_and_rejects_symlinks() { let root = tempfile::tempdir().unwrap(); std::fs::write(root.path().join("a"), "original").unwrap(); std::fs::hard_link(root.path().join("a"), root.path().join("alias")).unwrap(); std::os::unix::fs::symlink("a", root.path().join("link")).unwrap(); - let _socket = std::os::unix::net::UnixListener::bind(root.path().join("socket")).unwrap(); let session = session(root.path()); assert!(session.checkout_observe(path("link")).await.is_err()); - assert!(session.checkout_observe(path("socket")).await.is_err()); let read = checked_read(&session, session.checkout_observe(path("a")).await.unwrap()).await; session .checkout_execute(CheckoutRequest {