From 1b6d4dbfd815b13b8d9081c2bb912a340bbe45ff Mon Sep 17 00:00:00 2001 From: Hare Date: Wed, 21 Oct 2026 01:22:04 +0900 Subject: [PATCH] fix: preserve workdir failure diagnostics across transport --- crates/server-api/src/lib.rs | 8 ++ crates/tools/src/bash.rs | 14 +-- crates/tools/src/file_target.rs | 4 +- crates/tools/tests/integration.rs | 7 +- crates/workdir/src/external.rs | 6 ++ crates/workdir/src/http.rs | 83 ++++++++++++++++-- crates/workdir/src/lib.rs | 56 +++++++++++- crates/workdir/src/local.rs | 18 ++-- crates/workdir/src/local/command_transport.rs | 1 + .../src/local/command_transport/tests.rs | 32 +++++++ crates/workdir/src/router.rs | 2 +- crates/workdir/tests/failure_diagnostics.rs | 85 +++++++++++++++++++ crates/worker-runtime/src/http_server.rs | 11 +++ .../tests/command_transport_tests.rs | 36 ++++++++ crates/worker/src/checkout_http_tests.rs | 1 + .../src/feature/builtin/manage_workdir.rs | 17 +++- crates/worker/tests/controller_test.rs | 4 +- crates/workspace-server/src/server.rs | 23 +++++ .../src/server/tests/api_logging_tests.rs | 59 +++++++++++++ crates/yoi/src/workdir_share.rs | 2 + 20 files changed, 440 insertions(+), 29 deletions(-) create mode 100644 crates/workdir/tests/failure_diagnostics.rs diff --git a/crates/server-api/src/lib.rs b/crates/server-api/src/lib.rs index 1de9be8e8..3b448d409 100644 --- a/crates/server-api/src/lib.rs +++ b/crates/server-api/src/lib.rs @@ -132,6 +132,8 @@ pub struct WorkdirOperationApiError { pub workdir_log: Option, #[serde(skip)] pub denial_reason: Option, + #[serde(skip)] + pub failure_reason: Option, } /// Internal operational correlation. Producers supply fixed-length ID fingerprints and @@ -157,6 +159,7 @@ impl WorkdirOperationApiError { status: error.status, workdir_log: None, denial_reason: None, + failure_reason: None, } } @@ -170,6 +173,7 @@ impl WorkdirOperationApiError { status, workdir_log: None, denial_reason: error.denial_reason, + failure_reason: error.failure_reason, } } } @@ -15047,10 +15051,12 @@ mod workdir_error_diagnostic_tests { #[test] fn workdir_internal_diagnostics_never_enter_public_error_body_or_schema() { let mut error = WorkdirOperationApiError::provider(workdir::http::WorkdirTransportError { + failure_reason: None, code: workdir::http::WorkdirTransportErrorCode::Denied, message: "Workdir operation was denied".to_string(), denial_reason: Some(workdir::WorkdirDenialReason::OsPermissionDenied), }); + error.failure_reason = Some(workdir::WorkdirFailureReason::HttpTimeout); error.workdir_log = Some(WorkdirOperationLogContext { operation: "write", workspace_id_hash: "internal-workspace-fingerprint".to_string(), @@ -15064,10 +15070,12 @@ mod workdir_error_diagnostic_tests { assert_eq!(api_macros::HttpError::status_code(&error), 403); let decoded: WorkdirOperationApiError = serde_json::from_value(wire).unwrap(); assert!(decoded.denial_reason.is_none()); + assert!(decoded.failure_reason.is_none()); assert!(decoded.workdir_log.is_none()); let schema = serde_json::to_value(schemars::schema_for!(WorkdirOperationApiError)).unwrap(); assert!(schema["properties"].get("workdir_log").is_none()); assert!(schema["properties"].get("denial_reason").is_none()); + assert!(schema["properties"].get("failure_reason").is_none()); } } diff --git a/crates/tools/src/bash.rs b/crates/tools/src/bash.rs index 74dcee471..efeaa3acf 100644 --- a/crates/tools/src/bash.rs +++ b/crates/tools/src/bash.rs @@ -158,11 +158,15 @@ async fn command_page( ) -> Result { for attempt in 0..3 { match session.command_output(request.clone()).await { - Err( - workdir::WorkdirError::Transport(_) - | workdir::WorkdirError::Unavailable(_) - | workdir::WorkdirError::OutcomeUnknown(_), - ) if attempt < 2 => { + Err(error) + if attempt < 2 + && matches!( + error.classification_source(), + workdir::WorkdirError::Transport(_) + | workdir::WorkdirError::Unavailable(_) + | workdir::WorkdirError::OutcomeUnknown(_) + ) => + { tokio::time::sleep(std::time::Duration::from_millis(100 << attempt)).await; } result => return result, diff --git a/crates/tools/src/file_target.rs b/crates/tools/src/file_target.rs index f090431ac..c3bfb535e 100644 --- a/crates/tools/src/file_target.rs +++ b/crates/tools/src/file_target.rs @@ -147,7 +147,9 @@ impl FileTarget { pub(crate) fn checked_error(error: workdir::WorkdirError) -> ToolError { use workdir::WorkdirError; let code = match error.classification_source() { - WorkdirError::DenialContext { .. } => unreachable!("diagnostic context unwrapped"), + WorkdirError::DenialContext { .. } | WorkdirError::FailureContext { .. } => { + unreachable!("diagnostic context unwrapped") + } WorkdirError::Conflict(_) | WorkdirError::NotFound(_) => "checkout_stale", WorkdirError::Denied { .. } | WorkdirError::OutOfScope { .. } diff --git a/crates/tools/tests/integration.rs b/crates/tools/tests/integration.rs index accfb59ef..f7b1dd2da 100644 --- a/crates/tools/tests/integration.rs +++ b/crates/tools/tests/integration.rs @@ -546,9 +546,10 @@ impl workdir::WorkdirSession for ExternalValidatingSession { .is_ok() { // The production provider has committed a page, but its response is lost. - return Err(workdir::WorkdirError::Transport( - "injected lost response".into(), - )); + return Err( + workdir::WorkdirError::Transport("injected lost response".into()) + .with_failure_reason(workdir::WorkdirFailureReason::HttpTimeout), + ); } Ok(output) } diff --git a/crates/workdir/src/external.rs b/crates/workdir/src/external.rs index 867d24498..c1c00bfd8 100644 --- a/crates/workdir/src/external.rs +++ b/crates/workdir/src/external.rs @@ -525,6 +525,8 @@ pub struct ExternalWorkdirOperationError { code: crate::http::WorkdirTransportErrorCode, #[serde(default, skip_serializing_if = "Option::is_none")] denial_reason: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + failure_reason: Option, } impl ExternalWorkdirOperationError { @@ -534,11 +536,13 @@ impl ExternalWorkdirOperationError { Self { code: error.code, denial_reason: error.denial_reason, + failure_reason: error.failure_reason, } } pub fn into_transport_error(self) -> WorkdirTransportError { WorkdirTransportError { + failure_reason: self.failure_reason, code: self.code, denial_reason: self.denial_reason, message: format!("External Workdir provider reported {}", self.code.as_str()), @@ -762,6 +766,7 @@ mod tests { ] { let external = ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { + failure_reason: None, code: Code::Denied, message: "/home/operator/private provider-secret".repeat(4096), denial_reason: reason, @@ -788,6 +793,7 @@ mod tests { #[test] fn provider_failures_are_closed_and_drop_provider_authored_diagnostics() { let error = ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { + failure_reason: None, code: crate::http::WorkdirTransportErrorCode::Internal, denial_reason: None, message: "/home/operator/private provider-secret\ncontrol".to_string(), diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index 7464943da..8a035395c 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -275,6 +275,8 @@ pub struct WorkdirTransportError { /// Internal provider diagnostics. Public response boundaries may omit this. #[serde(default, skip_serializing_if = "Option::is_none")] pub denial_reason: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub failure_reason: Option, } impl WorkdirTransportError { @@ -282,6 +284,11 @@ impl WorkdirTransportError { pub fn from_workdir_error(error: &WorkdirError) -> Self { use WorkdirTransportErrorCode as Code; let (code, message) = match error { + WorkdirError::FailureContext { reason, source } => { + let mut transport = Self::from_workdir_error(source); + transport.failure_reason = Some(*reason); + return transport; + } WorkdirError::DenialContext { reason, source } => { let mut transport = Self::from_workdir_error(source); transport.denial_reason = Some(*reason); @@ -301,6 +308,7 @@ impl WorkdirTransportError { code: Code::Unsupported, message: format!("Workdir capability {capability:?} is not available"), denial_reason: None, + failure_reason: None, }; } WorkdirError::UnsupportedOperation(_) => { @@ -359,6 +367,7 @@ impl WorkdirTransportError { code, message: message.to_string(), denial_reason: error.denial_reason(), + failure_reason: error.failure_reason(), } } @@ -408,6 +417,11 @@ impl WorkdirTransportError { Code::Transport => WorkdirError::Transport(self.message), Code::Internal => WorkdirError::OperationFailed, }; + let error = if let Some(reason) = self.failure_reason { + error.with_failure_reason(reason) + } else { + error + }; // Preserve any supplied reason independently of the public classification. // Without a reason, retain the existing classification's deliberate fallback. if let Some(reason) = self.denial_reason { @@ -557,9 +571,10 @@ mod client { operation: WorkdirSessionOperation, ) -> Result { if self.closed.load(Ordering::Acquire) { - return Err(WorkdirError::Unavailable( - "Workdir session is closed".to_string(), - )); + return Err( + WorkdirError::Unavailable("Workdir session is closed".to_string()) + .with_failure_reason(crate::WorkdirFailureReason::SessionClosed), + ); } let url = endpoint( &self.base_url, @@ -767,6 +782,10 @@ mod client { let outcome = WorkdirError::OutcomeUnknown( "checkout transport failed; effects may have occurred".into(), ); + let outcome = match error.failure_reason() { + Some(reason) => outcome.with_failure_reason(reason), + None => outcome, + }; Err(match error.denial_reason() { Some(reason) => WorkdirError::DenialContext { reason, @@ -966,8 +985,10 @@ mod client { ) -> Result { if response.status().is_success() { let bytes = bounded_response_bytes(response).await?; - serde_json::from_slice(&bytes) - .map_err(|_| WorkdirError::Transport("Runtime Workdir response was invalid".into())) + serde_json::from_slice(&bytes).map_err(|_| { + WorkdirError::Transport("Runtime Workdir response was invalid".into()) + .with_failure_reason(crate::WorkdirFailureReason::HttpResponseInvalid) + }) } else { Err(decode_error(response).await) } @@ -983,14 +1004,16 @@ mod client { { return Err(WorkdirError::Transport( "Runtime Workdir response exceeded transport bound".into(), - )); + ) + .with_failure_reason(crate::WorkdirFailureReason::HttpResponseTooLarge)); } let mut bytes = Vec::new(); while let Some(chunk) = response.chunk().await.map_err(http_unavailable)? { if chunk.len() > MAX_RESPONSE_BYTES.saturating_sub(bytes.len()) { return Err(WorkdirError::Transport( "Runtime Workdir response exceeded transport bound".into(), - )); + ) + .with_failure_reason(crate::WorkdirFailureReason::HttpResponseTooLarge)); } bytes.extend_from_slice(&chunk); } @@ -1003,13 +1026,22 @@ mod client { .map(WorkdirTransportError::into_workdir_error) .unwrap_or_else(|_| { WorkdirError::Transport("Runtime Workdir error response was invalid".into()) + .with_failure_reason(crate::WorkdirFailureReason::HttpResponseInvalid) }), Err(error) => error, } } - fn http_unavailable(_error: reqwest::Error) -> WorkdirError { + fn http_unavailable(error: reqwest::Error) -> WorkdirError { + let reason = if error.is_timeout() { + crate::WorkdirFailureReason::HttpTimeout + } else if error.is_connect() { + crate::WorkdirFailureReason::HttpConnect + } else { + crate::WorkdirFailureReason::HttpTransfer + }; WorkdirError::Transport("Runtime Workdir HTTP request failed".to_string()) + .with_failure_reason(reason) } #[cfg(test)] @@ -1017,6 +1049,40 @@ mod client { use super::*; use std::sync::Mutex; + #[tokio::test] + async fn http_failure_reasons_do_not_copy_urls_or_credentials() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let client = Client::builder().no_proxy().build().unwrap(); + // A listening socket that never replies makes a real HTTP timeout. + let error = client + .get(format!("http://{address}/private?token=secret")) + .timeout(std::time::Duration::from_millis(25)) + .send() + .await + .unwrap_err(); + assert!(error.is_timeout()); + let error = http_unavailable(error); + assert_eq!( + error.failure_reason(), + Some(crate::WorkdirFailureReason::HttpTimeout) + ); + assert!(!format!("{error:?}").contains("secret")); + assert!(!format!("{error:?}").contains("/private")); + drop(listener); + let error = client + .get(format!("http://{address}/private?token=secret")) + .timeout(std::time::Duration::from_secs(2)) + .send() + .await + .unwrap_err(); + assert!(error.is_connect()); + assert_eq!( + http_unavailable(error).failure_reason(), + Some(crate::WorkdirFailureReason::HttpConnect) + ); + } + #[derive(Debug, Default)] struct RejectAuthorization { requests: Arc)>>>, @@ -1193,6 +1259,7 @@ mod tests { (WorkdirTransportErrorCode::Internal, 500, "operation failed"), ] { let transport = WorkdirTransportError { + failure_reason: None, code, denial_reason: None, message: if code == WorkdirTransportErrorCode::Conflict { diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index 7f052d5be..1a515ac16 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -401,8 +401,44 @@ impl From<&str> for WorkdirDenial { } } +/// Bounded operational diagnostics, never provider text, paths or credentials. +/// Reasons describe an observation, not retry permission or proof of no effects. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum WorkdirFailureReason { + SessionClosed, + ProviderUnavailable, + CommandTerminalMissing, + HttpTimeout, + HttpConnect, + HttpTransfer, + HttpResponseInvalid, + HttpResponseTooLarge, +} + +impl WorkdirFailureReason { + pub const fn as_str(self) -> &'static str { + match self { + Self::SessionClosed => "session_closed", + Self::ProviderUnavailable => "provider_unavailable", + Self::CommandTerminalMissing => "command_terminal_missing", + Self::HttpTimeout => "http_timeout", + Self::HttpConnect => "http_connect", + Self::HttpTransfer => "http_transfer", + Self::HttpResponseInvalid => "http_response_invalid", + Self::HttpResponseTooLarge => "http_response_too_large", + } + } +} + #[derive(Debug, thiserror::Error)] pub enum WorkdirError { + #[error("{source}")] + FailureContext { + reason: WorkdirFailureReason, + #[source] + source: Box, + }, #[error("Workdir operation denied: {0}")] Denied(WorkdirDenial), @@ -521,12 +557,29 @@ impl WorkdirError { /// and `denial_reason`: enrichment must not alter behavior or retry policy. pub fn classification_source(&self) -> &Self { let mut error = self; - while let Self::DenialContext { source, .. } = error { + while let Self::DenialContext { source, .. } | Self::FailureContext { source, .. } = error { error = source; } error } + pub fn with_failure_reason(self, reason: WorkdirFailureReason) -> Self { + Self::FailureContext { + reason, + source: Box::new(self), + } + } + + pub fn failure_reason(&self) -> Option { + match self { + Self::FailureContext { reason, .. } => Some(*reason), + Self::DenialContext { source, .. } => source.failure_reason(), + Self::SessionClosed => Some(WorkdirFailureReason::SessionClosed), + Self::Unavailable(_) => Some(WorkdirFailureReason::ProviderUnavailable), + _ => None, + } + } + pub fn denied(reason: WorkdirDenialReason, message: impl Into) -> Self { Self::Denied(WorkdirDenial { reason: Some(reason), @@ -540,6 +593,7 @@ impl WorkdirError { match self { Self::Denied(denial) => denial.reason, Self::DenialContext { reason, .. } => Some(*reason), + Self::FailureContext { source, .. } => source.denial_reason(), Self::OutOfScope(_) => Some(Reason::PathOutOfScope), Self::SymlinkOutOfScope { .. } => Some(Reason::SymlinkTargetOutOfScope), Self::ReadOnly(_) => Some(Reason::PathReadOnly), diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index c84970b0f..344cab6a8 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -1210,7 +1210,8 @@ impl LocalWorkdirSession { Err(WorkdirError::Unavailable(format!( "Workdir session for {} is closed", self.inner.workdir.id() - ))) + )) + .with_failure_reason(crate::WorkdirFailureReason::SessionClosed)) } else { Ok(()) } @@ -2546,11 +2547,18 @@ mod tests { .expect("close should wake command output waiters") .unwrap() .unwrap_err(); - assert!(matches!(waiter_error, WorkdirError::Unavailable(_))); + assert!(matches!( + waiter_error.classification_source(), + WorkdirError::Unavailable(_) + )); + assert_eq!( + waiter_error.failure_reason(), + Some(crate::WorkdirFailureReason::SessionClosed) + ); assert!(matches!( WorkdirSession::command_status(&session, command).await, - Err(WorkdirError::Unavailable(_)) + Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_)) )); assert!(matches!( WorkdirSession::read( @@ -2563,7 +2571,7 @@ mod tests { }, ) .await, - Err(WorkdirError::Unavailable(_)) + Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_)) )); let restored = LocalWorkdirSession::materialized_bound( @@ -3531,7 +3539,7 @@ mod tests { .await .unwrap() .unwrap(), - Err(WorkdirError::Unavailable(message)) if message.contains("is closed") + Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(message) if message.contains("is closed")) )); assert!( session.command_snapshot().is_empty(), diff --git a/crates/workdir/src/local/command_transport.rs b/crates/workdir/src/local/command_transport.rs index 5b800c0ab..1af30b1fc 100644 --- a/crates/workdir/src/local/command_transport.rs +++ b/crates/workdir/src/local/command_transport.rs @@ -113,6 +113,7 @@ impl Transport { WorkdirError::Unavailable( "command transport ended before terminal fence".into(), ) + .with_failure_reason(crate::WorkdirFailureReason::CommandTerminalMissing) })?; self.terminal = Some(terminal.clone()); page.terminal = Some(terminal); diff --git a/crates/workdir/src/local/command_transport/tests.rs b/crates/workdir/src/local/command_transport/tests.rs index 796a1f93f..12f20b061 100644 --- a/crates/workdir/src/local/command_transport/tests.rs +++ b/crates/workdir/src/local/command_transport/tests.rs @@ -4,6 +4,38 @@ use super::*; use tempfile::TempDir; +#[tokio::test] +async fn missing_terminal_is_diagnosed_without_changing_error_classification() { + let (sender, receiver) = mpsc::channel(1); + let (_status, terminal_status) = watch::channel(None); + drop(sender); + let mut transport = Transport { + receiver, + pending: None, + cursor: 0, + replay: None, + terminal: None, + terminal_status, + }; + let error = transport + .read(&CommandOutputRequest { + handle: CommandHandle("diagnostic-test".into()), + cursor: 0, + limit: 16, + wait: true, + }) + .await + .unwrap_err(); + assert_eq!( + error.failure_reason(), + Some(crate::WorkdirFailureReason::CommandTerminalMissing) + ); + assert!(matches!( + error.classification_source(), + WorkdirError::Unavailable(_) + )); +} + struct Harness { session: LocalWorkdirSession, _root: TempDir, diff --git a/crates/workdir/src/router.rs b/crates/workdir/src/router.rs index 362504eab..8810b4435 100644 --- a/crates/workdir/src/router.rs +++ b/crates/workdir/src/router.rs @@ -842,7 +842,7 @@ mod tests { assert!(router.is_empty()); assert!(matches!( underlying.command_status(handle).await, - Err(WorkdirError::Unavailable(_)) + Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_)) )); } diff --git a/crates/workdir/tests/failure_diagnostics.rs b/crates/workdir/tests/failure_diagnostics.rs new file mode 100644 index 000000000..a94f4fe9f --- /dev/null +++ b/crates/workdir/tests/failure_diagnostics.rs @@ -0,0 +1,85 @@ +use workdir::external::ExternalWorkdirOperationError; +use workdir::http::{WorkdirTransportError, WorkdirTransportErrorCode as Code}; +use workdir::{WorkdirError, WorkdirFailureReason as Reason}; + +#[test] +fn failure_reasons_survive_internal_transports_without_provider_text() { + for reason in [ + Reason::SessionClosed, + Reason::ProviderUnavailable, + Reason::CommandTerminalMissing, + Reason::HttpTimeout, + Reason::HttpConnect, + Reason::HttpTransfer, + Reason::HttpResponseInvalid, + Reason::HttpResponseTooLarge, + ] { + let source = WorkdirError::Unavailable("secret-token /private/path".into()) + .with_failure_reason(reason); + let transport = WorkdirTransportError::from_workdir_error(&source); + let wire = serde_json::to_string(&transport).unwrap(); + assert!(!wire.contains("secret-token")); + assert!(!wire.contains("/private/path")); + assert_eq!(transport.code, Code::Unavailable); + assert_eq!(transport.code.http_status(), 503); + assert_eq!(transport.message, "Workdir session is unavailable"); + assert_eq!(serde_json::to_value(reason).unwrap(), reason.as_str()); + let decoded: WorkdirTransportError = serde_json::from_str(&wire).unwrap(); + let external = ExternalWorkdirOperationError::from_transport_error(decoded); + let external_wire = serde_json::to_string(&external).unwrap(); + let external: ExternalWorkdirOperationError = serde_json::from_str(&external_wire).unwrap(); + let error = external.into_transport_error().into_workdir_error(); + assert_eq!(error.failure_reason(), Some(reason)); + assert!(matches!( + error.classification_source(), + WorkdirError::Unavailable(_) + )); + assert_eq!( + WorkdirTransportError::from_workdir_error(&error).failure_reason, + Some(reason) + ); + } +} + +#[test] +fn legacy_errors_and_nested_context_preserve_classification() { + let wire = r#"{"code":"unavailable","message":"Workdir session is unavailable"}"#; + let old: WorkdirTransportError = serde_json::from_str(wire).unwrap(); + assert!(old.failure_reason.is_none()); + assert!(matches!( + old.into_workdir_error(), + WorkdirError::Unavailable(_) + )); + let closed = WorkdirTransportError::from_workdir_error(&WorkdirError::SessionClosed); + assert_eq!(closed.failure_reason, Some(Reason::SessionClosed)); + // Reason-looking text never becomes a typed diagnosis. + let generic = WorkdirError::Unavailable("http_timeout".into()); + assert_eq!(generic.failure_reason(), Some(Reason::ProviderUnavailable)); + let error = WorkdirError::DenialContext { + reason: workdir::WorkdirDenialReason::OsPermissionDenied, + source: Box::new( + WorkdirError::OutcomeUnknown("do not retry".into()) + .with_failure_reason(Reason::HttpTransfer), + ), + }; + let error = WorkdirTransportError::from_workdir_error(&error).into_workdir_error(); + assert!(matches!( + error.classification_source(), + WorkdirError::OutcomeUnknown(_) + )); + assert_eq!( + error.denial_reason(), + Some(workdir::WorkdirDenialReason::OsPermissionDenied) + ); + assert_eq!(error.failure_reason(), Some(Reason::HttpTransfer)); +} + +#[test] +fn provider_cannot_inject_free_text_failure_reason() { + assert!( + serde_json::from_str::( + r#"{"code":"unavailable","message":"x","failure_reason":"secret /path"}"# + ) + .is_err() + ); +} diff --git a/crates/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index 57b7d0eb6..a1faefc73 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -2146,6 +2146,7 @@ impl RuntimeHttpWorkdirError { Self { status, payload: WorkdirTransportError { + failure_reason: None, denial_reason: None, code, message: message.into(), @@ -2214,6 +2215,16 @@ impl From for RuntimeHttpWorkdirError { impl IntoResponse for RuntimeHttpWorkdirError { fn into_response(self) -> Response { + if let Some(reason) = self.payload.failure_reason { + tracing::warn!( + target: "worker_runtime::workdir", + event = "workdir_provider_failure", + status = self.status.as_u16(), + code = self.payload.code.as_str(), + failure_reason = reason.as_str(), + "Workdir provider operation failed" + ); + } (self.status, Json(self.payload)).into_response() } } diff --git a/crates/worker-runtime/src/http_server/tests/command_transport_tests.rs b/crates/worker-runtime/src/http_server/tests/command_transport_tests.rs index 643e722ab..de1150159 100644 --- a/crates/worker-runtime/src/http_server/tests/command_transport_tests.rs +++ b/crates/worker-runtime/src/http_server/tests/command_transport_tests.rs @@ -156,6 +156,42 @@ impl Harness { } } +#[tokio::test] +async fn real_http_closed_session_retains_failure_reason() { + let harness = Harness::new().await; + let handle = harness + .remote + .start_command(CommandRequest { + transport_byte_budget: 1024, + command: "printf done".into(), + timeout_secs: 5, + cwd: WorkdirPath::root(), + tool_call_id: None, + }) + .await + .unwrap(); + // Keep the HTTP session identity registered, but close the underlying provider. + harness.session.close().await.unwrap(); + let error = harness + .remote + .command_output(CommandOutputRequest { + handle, + cursor: 0, + limit: 16, + wait: true, + }) + .await + .unwrap_err(); + assert_eq!( + error.failure_reason(), + Some(workdir::WorkdirFailureReason::SessionClosed) + ); + assert!(matches!( + error.classification_source(), + workdir::WorkdirError::Unavailable(_) + )); +} + #[tokio::test] async fn real_http_command_pages_preserve_binary_streams_replay_and_nonzero_exit() { tokio::time::timeout(Duration::from_secs(20), async { diff --git a/crates/worker/src/checkout_http_tests.rs b/crates/worker/src/checkout_http_tests.rs index 9c50f3d0f..d6489ae6c 100644 --- a/crates/worker/src/checkout_http_tests.rs +++ b/crates/worker/src/checkout_http_tests.rs @@ -57,6 +57,7 @@ impl ProviderState { return Err(( StatusCode::UNAUTHORIZED, AxumJson(WorkdirTransportError { + failure_reason: None, denial_reason: None, code: WorkdirTransportErrorCode::Denied, message: "provider authorization denied".into(), diff --git a/crates/worker/src/feature/builtin/manage_workdir.rs b/crates/worker/src/feature/builtin/manage_workdir.rs index ad35374e2..12cfdb65d 100644 --- a/crates/worker/src/feature/builtin/manage_workdir.rs +++ b/crates/worker/src/feature/builtin/manage_workdir.rs @@ -409,10 +409,20 @@ impl WorkdirSession for WorkspaceAttachedWorkdirSession { "mismatched checkout mutation response".into(), )), Ok(_) => Err(Self::mismatch("checkout_execute")), - Err(WorkdirError::Transport(_) | WorkdirError::Unavailable(_)) if mutation => { - Err(WorkdirError::OutcomeUnknown( + Err(error) + if mutation + && matches!( + error.classification_source(), + WorkdirError::Transport(_) | WorkdirError::Unavailable(_) + ) => + { + let outcome = WorkdirError::OutcomeUnknown( "checkout mutation response lost; inspect effects before retry".into(), - )) + ); + Err(match error.failure_reason() { + Some(reason) => outcome.with_failure_reason(reason), + None => outcome, + }) } Err(error) => Err(error), } @@ -1230,6 +1240,7 @@ mod tests { WorkspaceResponse { status, body: serde_json::to_string(&workdir::http::WorkdirTransportError { + failure_reason: None, denial_reason: None, code, message: message.to_string(), diff --git a/crates/worker/tests/controller_test.rs b/crates/worker/tests/controller_test.rs index 077eadc99..52ebdc62c 100644 --- a/crates/worker/tests/controller_test.rs +++ b/crates/worker/tests/controller_test.rs @@ -480,7 +480,7 @@ async fn shutdown_closes_bound_workdir_session() { assert!(matches!( session.command_status(command).await, - Err(WorkdirError::Unavailable(_)) + Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_)) )); } @@ -758,7 +758,7 @@ async fn controller_startup_failure_closes_bound_workdir_session() { tool_call_id: None, }) .await, - Err(WorkdirError::Unavailable(_)) + Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_)) )); } diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index a8ddde657..76d394da4 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -1411,6 +1411,10 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { 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, @@ -13501,6 +13505,7 @@ async fn log_failed_api_response(request: Request, next: Next) -> Response { attachment_alias_hash = context.map(|ctx| ctx.attachment_alias_hash.as_str()), 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()), status = event.status, kind = event.kind.unwrap_or("unknown"), message = event.message.unwrap_or(""), @@ -20989,9 +20994,14 @@ impl InternalWorkdirOperationError { Error::WorkdirSessionOpenFailed { transport, .. } => transport.denial_reason, _ => None, }; + let failure_reason = match &error.error { + Error::WorkdirSessionOpenFailed { transport, .. } => transport.failure_reason, + _ => None, + }; let mut error = server_api::WorkdirOperationApiError::api(error.into_repository_api_error()); error.denial_reason = reason; + error.failure_reason = failure_reason; error } Self::Provider(error) => server_api::WorkdirOperationApiError::provider(error), @@ -27162,6 +27172,7 @@ async fn serve_external_workdir_provider( Some(ExternalProviderCommand::Operation { operation_id, operation, response }) => { if revoking { let _ = response.send(Err(WorkdirTransportError { + failure_reason: None, denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Unavailable, message: "External Workdir provider shutdown is in progress".to_string(), @@ -27175,6 +27186,7 @@ async fn serve_external_workdir_provider( }; if pending.len() >= pending_limit { let _ = response.send(Err(WorkdirTransportError { + failure_reason: None, denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Unavailable, message: "External Workdir provider is at its in-flight operation limit".to_string(), @@ -27186,6 +27198,7 @@ async fn serve_external_workdir_provider( Ok(operation) => operation, Err(message) => { let _ = response.send(Err(WorkdirTransportError { + failure_reason: None, denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, message, @@ -27243,6 +27256,7 @@ async fn serve_external_workdir_provider( Err(error.into_transport_error()) } ExternalWorkdirOperationOutcome::Cancelled => Err(WorkdirTransportError { + failure_reason: None, denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Unavailable, message: "External Workdir operation was cancelled".to_string(), @@ -36896,6 +36910,7 @@ struct ApiErrorLog { diagnostics: Vec, workdir_log: Option, denial_reason: Option, + failure_reason: Option, } impl ApiErrorLog { @@ -36910,6 +36925,7 @@ impl ApiErrorLog { diagnostics: Vec::new(), workdir_log: error.workdir_log.clone(), denial_reason: error.denial_reason, + failure_reason: error.failure_reason, }; } let mut log = Self::from_repository_error(&server_api::RepositoryApiError::new( @@ -36924,6 +36940,7 @@ impl ApiErrorLog { )); log.workdir_log = error.workdir_log.clone(); 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. @@ -36940,6 +36957,7 @@ impl ApiErrorLog { Self { workdir_log: None, denial_reason: None, + failure_reason: None, kind: error .diagnostics .first() @@ -37254,6 +37272,7 @@ impl IntoResponse for ApiError { let log = ApiErrorLog { workdir_log: None, denial_reason: None, + failure_reason: None, kind: self .diagnostics .first() @@ -37601,6 +37620,7 @@ mod tests { assert!(matches!(operation, WorkdirSessionOperation::Read(_))); response .send(Err(WorkdirTransportError { + failure_reason: None, denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::NotFound, message: "logical file was not found".to_string(), @@ -41788,6 +41808,7 @@ mod tests { let error = ApiErrorLog { workdir_log: None, denial_reason: None, + failure_reason: None, kind: "ticket_backend_error".to_string(), message: "sqlite error: FOREIGN KEY constraint failed".to_string(), diagnostics: vec![RuntimeDiagnostic { @@ -41929,6 +41950,7 @@ mod tests { ), ] { let response = InternalWorkdirOperationError::Provider(WorkdirTransportError { + failure_reason: None, denial_reason: None, code, message: "safe provider message".to_string(), @@ -41953,6 +41975,7 @@ mod tests { workdir_operation_transport_error(workdir::WorkdirError::InvalidGlob("[".to_string())); let remote = workdir_operation_transport_error( WorkdirTransportError { + failure_reason: None, denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, message: "Workdir operation request is invalid".to_string(), 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 0b2320b17..fc883168b 100644 --- a/crates/workspace-server/src/server/tests/api_logging_tests.rs +++ b/crates/workspace-server/src/server/tests/api_logging_tests.rs @@ -563,6 +563,7 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() { )); response .send(Err(WorkdirTransportError { + failure_reason: None, denial_reason: reason, code, message: "body-secret /private/provider/path".to_string(), @@ -600,6 +601,64 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() { 64 ); } + for reason in [ + workdir::WorkdirFailureReason::SessionClosed, + workdir::WorkdirFailureReason::CommandTerminalMissing, + workdir::WorkdirFailureReason::HttpTimeout, + ] { + let request = runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &path, + serde_json::to_vec(&server_api::CurrentWorkerWorkdirOperationRequest { + target_workdir: "checkout".into(), + operation: WorkdirSessionOperation::CommandOutput(workdir::CommandOutputRequest { + handle: handle.clone(), + cursor: 0, + limit: 16, + wait: true, + }), + }) + .unwrap(), + ); + let provider = async { + let Some(ExternalProviderCommand::Operation { + operation, + response, + .. + }) = commands.recv().await + else { + panic!("expected output request") + }; + assert!(matches!( + operation, + WorkdirSessionOperation::CommandOutput(_) + )); + response + .send(Err(WorkdirTransportError { + code: Code::Unavailable, + message: "body-secret /private/provider/path".into(), + denial_reason: None, + failure_reason: Some(reason), + })) + .unwrap(); + }; + let ((status, body, events), ()) = + tokio::join!(request_with_logs(app.clone(), request), provider); + assert_eq!(status, StatusCode::SERVICE_UNAVAILABLE); + assert_eq!( + body, + json!({"code":"unavailable", "message":"Workdir session is unavailable"}) + ); + assert_eq!(events.len(), 1, "{events:?}"); + assert_eq!(events[0]["failure_reason"], reason.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(); + assert!(!log.contains("body-secret")); + assert!(!log.contains("/private/provider/path")); + } let provider = async { let Some(ExternalProviderCommand::Operation { response, .. }) = commands.recv().await else { diff --git a/crates/yoi/src/workdir_share.rs b/crates/yoi/src/workdir_share.rs index 6dc60bc1c..f760be2b0 100644 --- a/crates/yoi/src/workdir_share.rs +++ b/crates/yoi/src/workdir_share.rs @@ -836,6 +836,7 @@ where .unwrap_or_else(|_| ExternalWorkdirOperationOutcome::Failed { error: ExternalWorkdirOperationError::from_transport_error( WorkdirTransportError { + failure_reason: None, denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Internal, message: "External Workdir operation executor failed".to_string(), @@ -954,6 +955,7 @@ async fn execute_operation( Ok(result) => ExternalWorkdirOperationOutcome::Completed { result }, Err(message) => ExternalWorkdirOperationOutcome::Failed { error: ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { + failure_reason: None, denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, message,