fix: preserve workdir failure diagnostics across transport

This commit is contained in:
2026-10-21 01:22:04 +09:00
parent 4ef7b03651
commit 1b6d4dbfd8
20 changed files with 440 additions and 29 deletions
+8
View File
@@ -132,6 +132,8 @@ pub struct WorkdirOperationApiError {
pub workdir_log: Option<WorkdirOperationLogContext>, pub workdir_log: Option<WorkdirOperationLogContext>,
#[serde(skip)] #[serde(skip)]
pub denial_reason: Option<workdir::WorkdirDenialReason>, pub denial_reason: Option<workdir::WorkdirDenialReason>,
#[serde(skip)]
pub failure_reason: Option<workdir::WorkdirFailureReason>,
} }
/// Internal operational correlation. Producers supply fixed-length ID fingerprints and /// Internal operational correlation. Producers supply fixed-length ID fingerprints and
@@ -157,6 +159,7 @@ impl WorkdirOperationApiError {
status: error.status, status: error.status,
workdir_log: None, workdir_log: None,
denial_reason: None, denial_reason: None,
failure_reason: None,
} }
} }
@@ -170,6 +173,7 @@ impl WorkdirOperationApiError {
status, status,
workdir_log: None, workdir_log: None,
denial_reason: error.denial_reason, denial_reason: error.denial_reason,
failure_reason: error.failure_reason,
} }
} }
} }
@@ -15047,10 +15051,12 @@ mod workdir_error_diagnostic_tests {
#[test] #[test]
fn workdir_internal_diagnostics_never_enter_public_error_body_or_schema() { fn workdir_internal_diagnostics_never_enter_public_error_body_or_schema() {
let mut error = WorkdirOperationApiError::provider(workdir::http::WorkdirTransportError { let mut error = WorkdirOperationApiError::provider(workdir::http::WorkdirTransportError {
failure_reason: None,
code: workdir::http::WorkdirTransportErrorCode::Denied, code: workdir::http::WorkdirTransportErrorCode::Denied,
message: "Workdir operation was denied".to_string(), message: "Workdir operation was denied".to_string(),
denial_reason: Some(workdir::WorkdirDenialReason::OsPermissionDenied), denial_reason: Some(workdir::WorkdirDenialReason::OsPermissionDenied),
}); });
error.failure_reason = Some(workdir::WorkdirFailureReason::HttpTimeout);
error.workdir_log = Some(WorkdirOperationLogContext { error.workdir_log = Some(WorkdirOperationLogContext {
operation: "write", operation: "write",
workspace_id_hash: "internal-workspace-fingerprint".to_string(), 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); assert_eq!(api_macros::HttpError::status_code(&error), 403);
let decoded: WorkdirOperationApiError = serde_json::from_value(wire).unwrap(); let decoded: WorkdirOperationApiError = serde_json::from_value(wire).unwrap();
assert!(decoded.denial_reason.is_none()); assert!(decoded.denial_reason.is_none());
assert!(decoded.failure_reason.is_none());
assert!(decoded.workdir_log.is_none()); assert!(decoded.workdir_log.is_none());
let schema = serde_json::to_value(schemars::schema_for!(WorkdirOperationApiError)).unwrap(); let schema = serde_json::to_value(schemars::schema_for!(WorkdirOperationApiError)).unwrap();
assert!(schema["properties"].get("workdir_log").is_none()); assert!(schema["properties"].get("workdir_log").is_none());
assert!(schema["properties"].get("denial_reason").is_none()); assert!(schema["properties"].get("denial_reason").is_none());
assert!(schema["properties"].get("failure_reason").is_none());
} }
} }
+9 -5
View File
@@ -158,11 +158,15 @@ async fn command_page(
) -> Result<workdir::CommandOutput, workdir::WorkdirError> { ) -> Result<workdir::CommandOutput, workdir::WorkdirError> {
for attempt in 0..3 { for attempt in 0..3 {
match session.command_output(request.clone()).await { match session.command_output(request.clone()).await {
Err( Err(error)
workdir::WorkdirError::Transport(_) if attempt < 2
| workdir::WorkdirError::Unavailable(_) && matches!(
| workdir::WorkdirError::OutcomeUnknown(_), error.classification_source(),
) if attempt < 2 => { workdir::WorkdirError::Transport(_)
| workdir::WorkdirError::Unavailable(_)
| workdir::WorkdirError::OutcomeUnknown(_)
) =>
{
tokio::time::sleep(std::time::Duration::from_millis(100 << attempt)).await; tokio::time::sleep(std::time::Duration::from_millis(100 << attempt)).await;
} }
result => return result, result => return result,
+3 -1
View File
@@ -147,7 +147,9 @@ impl FileTarget {
pub(crate) fn checked_error(error: workdir::WorkdirError) -> ToolError { pub(crate) fn checked_error(error: workdir::WorkdirError) -> ToolError {
use workdir::WorkdirError; use workdir::WorkdirError;
let code = match error.classification_source() { 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::Conflict(_) | WorkdirError::NotFound(_) => "checkout_stale",
WorkdirError::Denied { .. } WorkdirError::Denied { .. }
| WorkdirError::OutOfScope { .. } | WorkdirError::OutOfScope { .. }
+4 -3
View File
@@ -546,9 +546,10 @@ impl workdir::WorkdirSession for ExternalValidatingSession {
.is_ok() .is_ok()
{ {
// The production provider has committed a page, but its response is lost. // The production provider has committed a page, but its response is lost.
return Err(workdir::WorkdirError::Transport( return Err(
"injected lost response".into(), workdir::WorkdirError::Transport("injected lost response".into())
)); .with_failure_reason(workdir::WorkdirFailureReason::HttpTimeout),
);
} }
Ok(output) Ok(output)
} }
+6
View File
@@ -525,6 +525,8 @@ pub struct ExternalWorkdirOperationError {
code: crate::http::WorkdirTransportErrorCode, code: crate::http::WorkdirTransportErrorCode,
#[serde(default, skip_serializing_if = "Option::is_none")] #[serde(default, skip_serializing_if = "Option::is_none")]
denial_reason: Option<crate::WorkdirDenialReason>, denial_reason: Option<crate::WorkdirDenialReason>,
#[serde(default, skip_serializing_if = "Option::is_none")]
failure_reason: Option<crate::WorkdirFailureReason>,
} }
impl ExternalWorkdirOperationError { impl ExternalWorkdirOperationError {
@@ -534,11 +536,13 @@ impl ExternalWorkdirOperationError {
Self { Self {
code: error.code, code: error.code,
denial_reason: error.denial_reason, denial_reason: error.denial_reason,
failure_reason: error.failure_reason,
} }
} }
pub fn into_transport_error(self) -> WorkdirTransportError { pub fn into_transport_error(self) -> WorkdirTransportError {
WorkdirTransportError { WorkdirTransportError {
failure_reason: self.failure_reason,
code: self.code, code: self.code,
denial_reason: self.denial_reason, denial_reason: self.denial_reason,
message: format!("External Workdir provider reported {}", self.code.as_str()), message: format!("External Workdir provider reported {}", self.code.as_str()),
@@ -762,6 +766,7 @@ mod tests {
] { ] {
let external = let external =
ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError {
failure_reason: None,
code: Code::Denied, code: Code::Denied,
message: "/home/operator/private provider-secret".repeat(4096), message: "/home/operator/private provider-secret".repeat(4096),
denial_reason: reason, denial_reason: reason,
@@ -788,6 +793,7 @@ mod tests {
#[test] #[test]
fn provider_failures_are_closed_and_drop_provider_authored_diagnostics() { fn provider_failures_are_closed_and_drop_provider_authored_diagnostics() {
let error = ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { let error = ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError {
failure_reason: None,
code: crate::http::WorkdirTransportErrorCode::Internal, code: crate::http::WorkdirTransportErrorCode::Internal,
denial_reason: None, denial_reason: None,
message: "/home/operator/private provider-secret\ncontrol".to_string(), message: "/home/operator/private provider-secret\ncontrol".to_string(),
+75 -8
View File
@@ -275,6 +275,8 @@ pub struct WorkdirTransportError {
/// Internal provider diagnostics. Public response boundaries may omit this. /// Internal provider diagnostics. Public response boundaries may omit this.
#[serde(default, skip_serializing_if = "Option::is_none")] #[serde(default, skip_serializing_if = "Option::is_none")]
pub denial_reason: Option<crate::WorkdirDenialReason>, pub denial_reason: Option<crate::WorkdirDenialReason>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub failure_reason: Option<crate::WorkdirFailureReason>,
} }
impl WorkdirTransportError { impl WorkdirTransportError {
@@ -282,6 +284,11 @@ impl WorkdirTransportError {
pub fn from_workdir_error(error: &WorkdirError) -> Self { pub fn from_workdir_error(error: &WorkdirError) -> Self {
use WorkdirTransportErrorCode as Code; use WorkdirTransportErrorCode as Code;
let (code, message) = match error { 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 } => { WorkdirError::DenialContext { reason, source } => {
let mut transport = Self::from_workdir_error(source); let mut transport = Self::from_workdir_error(source);
transport.denial_reason = Some(*reason); transport.denial_reason = Some(*reason);
@@ -301,6 +308,7 @@ impl WorkdirTransportError {
code: Code::Unsupported, code: Code::Unsupported,
message: format!("Workdir capability {capability:?} is not available"), message: format!("Workdir capability {capability:?} is not available"),
denial_reason: None, denial_reason: None,
failure_reason: None,
}; };
} }
WorkdirError::UnsupportedOperation(_) => { WorkdirError::UnsupportedOperation(_) => {
@@ -359,6 +367,7 @@ impl WorkdirTransportError {
code, code,
message: message.to_string(), message: message.to_string(),
denial_reason: error.denial_reason(), denial_reason: error.denial_reason(),
failure_reason: error.failure_reason(),
} }
} }
@@ -408,6 +417,11 @@ impl WorkdirTransportError {
Code::Transport => WorkdirError::Transport(self.message), Code::Transport => WorkdirError::Transport(self.message),
Code::Internal => WorkdirError::OperationFailed, 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. // Preserve any supplied reason independently of the public classification.
// Without a reason, retain the existing classification's deliberate fallback. // Without a reason, retain the existing classification's deliberate fallback.
if let Some(reason) = self.denial_reason { if let Some(reason) = self.denial_reason {
@@ -557,9 +571,10 @@ mod client {
operation: WorkdirSessionOperation, operation: WorkdirSessionOperation,
) -> Result<WorkdirSessionOperationResult, WorkdirError> { ) -> Result<WorkdirSessionOperationResult, WorkdirError> {
if self.closed.load(Ordering::Acquire) { if self.closed.load(Ordering::Acquire) {
return Err(WorkdirError::Unavailable( return Err(
"Workdir session is closed".to_string(), WorkdirError::Unavailable("Workdir session is closed".to_string())
)); .with_failure_reason(crate::WorkdirFailureReason::SessionClosed),
);
} }
let url = endpoint( let url = endpoint(
&self.base_url, &self.base_url,
@@ -767,6 +782,10 @@ mod client {
let outcome = WorkdirError::OutcomeUnknown( let outcome = WorkdirError::OutcomeUnknown(
"checkout transport failed; effects may have occurred".into(), "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() { Err(match error.denial_reason() {
Some(reason) => WorkdirError::DenialContext { Some(reason) => WorkdirError::DenialContext {
reason, reason,
@@ -966,8 +985,10 @@ mod client {
) -> Result<T, WorkdirError> { ) -> Result<T, WorkdirError> {
if response.status().is_success() { if response.status().is_success() {
let bytes = bounded_response_bytes(response).await?; let bytes = bounded_response_bytes(response).await?;
serde_json::from_slice(&bytes) serde_json::from_slice(&bytes).map_err(|_| {
.map_err(|_| WorkdirError::Transport("Runtime Workdir response was invalid".into())) WorkdirError::Transport("Runtime Workdir response was invalid".into())
.with_failure_reason(crate::WorkdirFailureReason::HttpResponseInvalid)
})
} else { } else {
Err(decode_error(response).await) Err(decode_error(response).await)
} }
@@ -983,14 +1004,16 @@ mod client {
{ {
return Err(WorkdirError::Transport( return Err(WorkdirError::Transport(
"Runtime Workdir response exceeded transport bound".into(), "Runtime Workdir response exceeded transport bound".into(),
)); )
.with_failure_reason(crate::WorkdirFailureReason::HttpResponseTooLarge));
} }
let mut bytes = Vec::new(); let mut bytes = Vec::new();
while let Some(chunk) = response.chunk().await.map_err(http_unavailable)? { while let Some(chunk) = response.chunk().await.map_err(http_unavailable)? {
if chunk.len() > MAX_RESPONSE_BYTES.saturating_sub(bytes.len()) { if chunk.len() > MAX_RESPONSE_BYTES.saturating_sub(bytes.len()) {
return Err(WorkdirError::Transport( return Err(WorkdirError::Transport(
"Runtime Workdir response exceeded transport bound".into(), "Runtime Workdir response exceeded transport bound".into(),
)); )
.with_failure_reason(crate::WorkdirFailureReason::HttpResponseTooLarge));
} }
bytes.extend_from_slice(&chunk); bytes.extend_from_slice(&chunk);
} }
@@ -1003,13 +1026,22 @@ mod client {
.map(WorkdirTransportError::into_workdir_error) .map(WorkdirTransportError::into_workdir_error)
.unwrap_or_else(|_| { .unwrap_or_else(|_| {
WorkdirError::Transport("Runtime Workdir error response was invalid".into()) WorkdirError::Transport("Runtime Workdir error response was invalid".into())
.with_failure_reason(crate::WorkdirFailureReason::HttpResponseInvalid)
}), }),
Err(error) => error, 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()) WorkdirError::Transport("Runtime Workdir HTTP request failed".to_string())
.with_failure_reason(reason)
} }
#[cfg(test)] #[cfg(test)]
@@ -1017,6 +1049,40 @@ mod client {
use super::*; use super::*;
use std::sync::Mutex; 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)] #[derive(Debug, Default)]
struct RejectAuthorization { struct RejectAuthorization {
requests: Arc<Mutex<Vec<(String, String, Vec<u8>)>>>, requests: Arc<Mutex<Vec<(String, String, Vec<u8>)>>>,
@@ -1193,6 +1259,7 @@ mod tests {
(WorkdirTransportErrorCode::Internal, 500, "operation failed"), (WorkdirTransportErrorCode::Internal, 500, "operation failed"),
] { ] {
let transport = WorkdirTransportError { let transport = WorkdirTransportError {
failure_reason: None,
code, code,
denial_reason: None, denial_reason: None,
message: if code == WorkdirTransportErrorCode::Conflict { message: if code == WorkdirTransportErrorCode::Conflict {
+55 -1
View File
@@ -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)] #[derive(Debug, thiserror::Error)]
pub enum WorkdirError { pub enum WorkdirError {
#[error("{source}")]
FailureContext {
reason: WorkdirFailureReason,
#[source]
source: Box<WorkdirError>,
},
#[error("Workdir operation denied: {0}")] #[error("Workdir operation denied: {0}")]
Denied(WorkdirDenial), Denied(WorkdirDenial),
@@ -521,12 +557,29 @@ impl WorkdirError {
/// and `denial_reason`: enrichment must not alter behavior or retry policy. /// and `denial_reason`: enrichment must not alter behavior or retry policy.
pub fn classification_source(&self) -> &Self { pub fn classification_source(&self) -> &Self {
let mut error = self; let mut error = self;
while let Self::DenialContext { source, .. } = error { while let Self::DenialContext { source, .. } | Self::FailureContext { source, .. } = error {
error = source; error = source;
} }
error error
} }
pub fn with_failure_reason(self, reason: WorkdirFailureReason) -> Self {
Self::FailureContext {
reason,
source: Box::new(self),
}
}
pub fn failure_reason(&self) -> Option<WorkdirFailureReason> {
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<String>) -> Self { pub fn denied(reason: WorkdirDenialReason, message: impl Into<String>) -> Self {
Self::Denied(WorkdirDenial { Self::Denied(WorkdirDenial {
reason: Some(reason), reason: Some(reason),
@@ -540,6 +593,7 @@ impl WorkdirError {
match self { match self {
Self::Denied(denial) => denial.reason, Self::Denied(denial) => denial.reason,
Self::DenialContext { reason, .. } => Some(*reason), Self::DenialContext { reason, .. } => Some(*reason),
Self::FailureContext { source, .. } => source.denial_reason(),
Self::OutOfScope(_) => Some(Reason::PathOutOfScope), Self::OutOfScope(_) => Some(Reason::PathOutOfScope),
Self::SymlinkOutOfScope { .. } => Some(Reason::SymlinkTargetOutOfScope), Self::SymlinkOutOfScope { .. } => Some(Reason::SymlinkTargetOutOfScope),
Self::ReadOnly(_) => Some(Reason::PathReadOnly), Self::ReadOnly(_) => Some(Reason::PathReadOnly),
+13 -5
View File
@@ -1210,7 +1210,8 @@ impl LocalWorkdirSession {
Err(WorkdirError::Unavailable(format!( Err(WorkdirError::Unavailable(format!(
"Workdir session for {} is closed", "Workdir session for {} is closed",
self.inner.workdir.id() self.inner.workdir.id()
))) ))
.with_failure_reason(crate::WorkdirFailureReason::SessionClosed))
} else { } else {
Ok(()) Ok(())
} }
@@ -2546,11 +2547,18 @@ mod tests {
.expect("close should wake command output waiters") .expect("close should wake command output waiters")
.unwrap() .unwrap()
.unwrap_err(); .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!( assert!(matches!(
WorkdirSession::command_status(&session, command).await, WorkdirSession::command_status(&session, command).await,
Err(WorkdirError::Unavailable(_)) Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_))
)); ));
assert!(matches!( assert!(matches!(
WorkdirSession::read( WorkdirSession::read(
@@ -2563,7 +2571,7 @@ mod tests {
}, },
) )
.await, .await,
Err(WorkdirError::Unavailable(_)) Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_))
)); ));
let restored = LocalWorkdirSession::materialized_bound( let restored = LocalWorkdirSession::materialized_bound(
@@ -3531,7 +3539,7 @@ mod tests {
.await .await
.unwrap() .unwrap()
.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!( assert!(
session.command_snapshot().is_empty(), session.command_snapshot().is_empty(),
@@ -113,6 +113,7 @@ impl Transport {
WorkdirError::Unavailable( WorkdirError::Unavailable(
"command transport ended before terminal fence".into(), "command transport ended before terminal fence".into(),
) )
.with_failure_reason(crate::WorkdirFailureReason::CommandTerminalMissing)
})?; })?;
self.terminal = Some(terminal.clone()); self.terminal = Some(terminal.clone());
page.terminal = Some(terminal); page.terminal = Some(terminal);
@@ -4,6 +4,38 @@
use super::*; use super::*;
use tempfile::TempDir; 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 { struct Harness {
session: LocalWorkdirSession, session: LocalWorkdirSession,
_root: TempDir, _root: TempDir,
+1 -1
View File
@@ -842,7 +842,7 @@ mod tests {
assert!(router.is_empty()); assert!(router.is_empty());
assert!(matches!( assert!(matches!(
underlying.command_status(handle).await, underlying.command_status(handle).await,
Err(WorkdirError::Unavailable(_)) Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_))
)); ));
} }
@@ -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::<WorkdirTransportError>(
r#"{"code":"unavailable","message":"x","failure_reason":"secret /path"}"#
)
.is_err()
);
}
+11
View File
@@ -2146,6 +2146,7 @@ impl RuntimeHttpWorkdirError {
Self { Self {
status, status,
payload: WorkdirTransportError { payload: WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code, code,
message: message.into(), message: message.into(),
@@ -2214,6 +2215,16 @@ impl From<workdir::WorkdirError> for RuntimeHttpWorkdirError {
impl IntoResponse for RuntimeHttpWorkdirError { impl IntoResponse for RuntimeHttpWorkdirError {
fn into_response(self) -> Response { 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() (self.status, Json(self.payload)).into_response()
} }
} }
@@ -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] #[tokio::test]
async fn real_http_command_pages_preserve_binary_streams_replay_and_nonzero_exit() { async fn real_http_command_pages_preserve_binary_streams_replay_and_nonzero_exit() {
tokio::time::timeout(Duration::from_secs(20), async { tokio::time::timeout(Duration::from_secs(20), async {
+1
View File
@@ -57,6 +57,7 @@ impl ProviderState {
return Err(( return Err((
StatusCode::UNAUTHORIZED, StatusCode::UNAUTHORIZED,
AxumJson(WorkdirTransportError { AxumJson(WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: WorkdirTransportErrorCode::Denied, code: WorkdirTransportErrorCode::Denied,
message: "provider authorization denied".into(), message: "provider authorization denied".into(),
@@ -409,10 +409,20 @@ impl WorkdirSession for WorkspaceAttachedWorkdirSession {
"mismatched checkout mutation response".into(), "mismatched checkout mutation response".into(),
)), )),
Ok(_) => Err(Self::mismatch("checkout_execute")), Ok(_) => Err(Self::mismatch("checkout_execute")),
Err(WorkdirError::Transport(_) | WorkdirError::Unavailable(_)) if mutation => { Err(error)
Err(WorkdirError::OutcomeUnknown( if mutation
&& matches!(
error.classification_source(),
WorkdirError::Transport(_) | WorkdirError::Unavailable(_)
) =>
{
let outcome = WorkdirError::OutcomeUnknown(
"checkout mutation response lost; inspect effects before retry".into(), "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), Err(error) => Err(error),
} }
@@ -1230,6 +1240,7 @@ mod tests {
WorkspaceResponse { WorkspaceResponse {
status, status,
body: serde_json::to_string(&workdir::http::WorkdirTransportError { body: serde_json::to_string(&workdir::http::WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code, code,
message: message.to_string(), message: message.to_string(),
+2 -2
View File
@@ -480,7 +480,7 @@ async fn shutdown_closes_bound_workdir_session() {
assert!(matches!( assert!(matches!(
session.command_status(command).await, 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, tool_call_id: None,
}) })
.await, .await,
Err(WorkdirError::Unavailable(_)) Err(ref error) if matches!(error.classification_source(), WorkdirError::Unavailable(_))
)); ));
} }
+23
View File
@@ -1411,6 +1411,10 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession {
let outcome = workdir::WorkdirError::OutcomeUnknown( let outcome = workdir::WorkdirError::OutcomeUnknown(
"checkout provider response lost; inspect effects before retry".into(), "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() { Err(match error.denial_reason() {
Some(reason) => workdir::WorkdirError::DenialContext { Some(reason) => workdir::WorkdirError::DenialContext {
reason, 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()), attachment_alias_hash = context.map(|ctx| ctx.attachment_alias_hash.as_str()),
request_token_id_hash = context.and(request_token_id_hash.as_deref()), 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()), 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, status = event.status,
kind = event.kind.unwrap_or("unknown"), kind = event.kind.unwrap_or("unknown"),
message = event.message.unwrap_or(""), message = event.message.unwrap_or(""),
@@ -20989,9 +20994,14 @@ impl InternalWorkdirOperationError {
Error::WorkdirSessionOpenFailed { transport, .. } => transport.denial_reason, Error::WorkdirSessionOpenFailed { transport, .. } => transport.denial_reason,
_ => None, _ => None,
}; };
let failure_reason = match &error.error {
Error::WorkdirSessionOpenFailed { transport, .. } => transport.failure_reason,
_ => None,
};
let mut error = let mut error =
server_api::WorkdirOperationApiError::api(error.into_repository_api_error()); server_api::WorkdirOperationApiError::api(error.into_repository_api_error());
error.denial_reason = reason; error.denial_reason = reason;
error.failure_reason = failure_reason;
error error
} }
Self::Provider(error) => server_api::WorkdirOperationApiError::provider(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 }) => { Some(ExternalProviderCommand::Operation { operation_id, operation, response }) => {
if revoking { if revoking {
let _ = response.send(Err(WorkdirTransportError { let _ = response.send(Err(WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: workdir::http::WorkdirTransportErrorCode::Unavailable, code: workdir::http::WorkdirTransportErrorCode::Unavailable,
message: "External Workdir provider shutdown is in progress".to_string(), 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 { if pending.len() >= pending_limit {
let _ = response.send(Err(WorkdirTransportError { let _ = response.send(Err(WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: workdir::http::WorkdirTransportErrorCode::Unavailable, code: workdir::http::WorkdirTransportErrorCode::Unavailable,
message: "External Workdir provider is at its in-flight operation limit".to_string(), 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, Ok(operation) => operation,
Err(message) => { Err(message) => {
let _ = response.send(Err(WorkdirTransportError { let _ = response.send(Err(WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest,
message, message,
@@ -27243,6 +27256,7 @@ async fn serve_external_workdir_provider(
Err(error.into_transport_error()) Err(error.into_transport_error())
} }
ExternalWorkdirOperationOutcome::Cancelled => Err(WorkdirTransportError { ExternalWorkdirOperationOutcome::Cancelled => Err(WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: workdir::http::WorkdirTransportErrorCode::Unavailable, code: workdir::http::WorkdirTransportErrorCode::Unavailable,
message: "External Workdir operation was cancelled".to_string(), message: "External Workdir operation was cancelled".to_string(),
@@ -36896,6 +36910,7 @@ struct ApiErrorLog {
diagnostics: Vec<RuntimeDiagnostic>, diagnostics: Vec<RuntimeDiagnostic>,
workdir_log: Option<server_api::WorkdirOperationLogContext>, workdir_log: Option<server_api::WorkdirOperationLogContext>,
denial_reason: Option<workdir::WorkdirDenialReason>, denial_reason: Option<workdir::WorkdirDenialReason>,
failure_reason: Option<workdir::WorkdirFailureReason>,
} }
impl ApiErrorLog { impl ApiErrorLog {
@@ -36910,6 +36925,7 @@ impl ApiErrorLog {
diagnostics: Vec::new(), diagnostics: Vec::new(),
workdir_log: error.workdir_log.clone(), workdir_log: error.workdir_log.clone(),
denial_reason: error.denial_reason, denial_reason: error.denial_reason,
failure_reason: error.failure_reason,
}; };
} }
let mut log = Self::from_repository_error(&server_api::RepositoryApiError::new( 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.workdir_log = error.workdir_log.clone();
log.denial_reason = error.denial_reason; log.denial_reason = error.denial_reason;
log.failure_reason = error.failure_reason;
if log.workdir_log.is_some() { if log.workdir_log.is_some() {
// Existing public API details are unchanged; do not copy request-derived // Existing public API details are unchanged; do not copy request-derived
// alias/handle text or session-open errors into operational diagnostics. // alias/handle text or session-open errors into operational diagnostics.
@@ -36940,6 +36957,7 @@ impl ApiErrorLog {
Self { Self {
workdir_log: None, workdir_log: None,
denial_reason: None, denial_reason: None,
failure_reason: None,
kind: error kind: error
.diagnostics .diagnostics
.first() .first()
@@ -37254,6 +37272,7 @@ impl IntoResponse for ApiError {
let log = ApiErrorLog { let log = ApiErrorLog {
workdir_log: None, workdir_log: None,
denial_reason: None, denial_reason: None,
failure_reason: None,
kind: self kind: self
.diagnostics .diagnostics
.first() .first()
@@ -37601,6 +37620,7 @@ mod tests {
assert!(matches!(operation, WorkdirSessionOperation::Read(_))); assert!(matches!(operation, WorkdirSessionOperation::Read(_)));
response response
.send(Err(WorkdirTransportError { .send(Err(WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: workdir::http::WorkdirTransportErrorCode::NotFound, code: workdir::http::WorkdirTransportErrorCode::NotFound,
message: "logical file was not found".to_string(), message: "logical file was not found".to_string(),
@@ -41788,6 +41808,7 @@ mod tests {
let error = ApiErrorLog { let error = ApiErrorLog {
workdir_log: None, workdir_log: None,
denial_reason: None, denial_reason: None,
failure_reason: None,
kind: "ticket_backend_error".to_string(), kind: "ticket_backend_error".to_string(),
message: "sqlite error: FOREIGN KEY constraint failed".to_string(), message: "sqlite error: FOREIGN KEY constraint failed".to_string(),
diagnostics: vec![RuntimeDiagnostic { diagnostics: vec![RuntimeDiagnostic {
@@ -41929,6 +41950,7 @@ mod tests {
), ),
] { ] {
let response = InternalWorkdirOperationError::Provider(WorkdirTransportError { let response = InternalWorkdirOperationError::Provider(WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code, code,
message: "safe provider message".to_string(), message: "safe provider message".to_string(),
@@ -41953,6 +41975,7 @@ mod tests {
workdir_operation_transport_error(workdir::WorkdirError::InvalidGlob("[".to_string())); workdir_operation_transport_error(workdir::WorkdirError::InvalidGlob("[".to_string()));
let remote = workdir_operation_transport_error( let remote = workdir_operation_transport_error(
WorkdirTransportError { WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest,
message: "Workdir operation request is invalid".to_string(), message: "Workdir operation request is invalid".to_string(),
@@ -563,6 +563,7 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() {
)); ));
response response
.send(Err(WorkdirTransportError { .send(Err(WorkdirTransportError {
failure_reason: None,
denial_reason: reason, denial_reason: reason,
code, code,
message: "body-secret /private/provider/path".to_string(), message: "body-secret /private/provider/path".to_string(),
@@ -600,6 +601,64 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() {
64 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 provider = async {
let Some(ExternalProviderCommand::Operation { response, .. }) = commands.recv().await let Some(ExternalProviderCommand::Operation { response, .. }) = commands.recv().await
else { else {
+2
View File
@@ -836,6 +836,7 @@ where
.unwrap_or_else(|_| ExternalWorkdirOperationOutcome::Failed { .unwrap_or_else(|_| ExternalWorkdirOperationOutcome::Failed {
error: ExternalWorkdirOperationError::from_transport_error( error: ExternalWorkdirOperationError::from_transport_error(
WorkdirTransportError { WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: workdir::http::WorkdirTransportErrorCode::Internal, code: workdir::http::WorkdirTransportErrorCode::Internal,
message: "External Workdir operation executor failed".to_string(), message: "External Workdir operation executor failed".to_string(),
@@ -954,6 +955,7 @@ async fn execute_operation(
Ok(result) => ExternalWorkdirOperationOutcome::Completed { result }, Ok(result) => ExternalWorkdirOperationOutcome::Completed { result },
Err(message) => ExternalWorkdirOperationOutcome::Failed { Err(message) => ExternalWorkdirOperationOutcome::Failed {
error: ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { error: ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError {
failure_reason: None,
denial_reason: None, denial_reason: None,
code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest,
message, message,