diff --git a/crates/tools/src/edit.rs b/crates/tools/src/edit.rs index dc618130..a303628f 100644 --- a/crates/tools/src/edit.rs +++ b/crates/tools/src/edit.rs @@ -298,7 +298,12 @@ mod tests { .execute(&inp.to_string(), Default::default()) .await .unwrap_err(); - let msg = format!("{err}"); - assert!(msg.contains("modified externally"), "{msg}"); + match err { + ToolError::ExecutionFailed(message) => assert_eq!( + message, + "The target file's content or existence changed since it was last observed; read the file again before retrying: a.txt" + ), + other => panic!("expected execution failure, got {other:?}"), + } } } diff --git a/crates/tools/src/error.rs b/crates/tools/src/error.rs index 78f92993..12b1103c 100644 --- a/crates/tools/src/error.rs +++ b/crates/tools/src/error.rs @@ -43,7 +43,8 @@ impl From for ToolError { | workdir::WorkdirError::Io { .. } | workdir::WorkdirError::Unavailable(_) | workdir::WorkdirError::OperationFailed - | workdir::WorkdirError::Transport(_), + | workdir::WorkdirError::Transport(_) + | workdir::WorkdirError::Conflict(_), ) => ToolError::ExecutionFailed(err.to_string()), ToolsError::FileSystem(_) | ToolsError::WorkdirSession(_) @@ -55,3 +56,48 @@ impl From for ToolError { } } } + +#[cfg(test)] +mod tests { + use super::*; + use workdir::http::{WorkdirTransportError, WorkdirTransportErrorCode}; + + #[test] + fn local_workdir_content_conflict_is_retryable_execution_failure() { + let error = ToolError::from(ToolsError::WorkdirSession( + fs_operation::FsError::Conflict("src/main.rs".to_string()).into(), + )); + + match error { + ToolError::ExecutionFailed(message) => assert_eq!( + message, + "The target file's content or existence changed since it was last observed; read the file again before retrying: src/main.rs" + ), + other => panic!("expected execution failure, got {other:?}"), + } + } + + #[test] + fn remote_workdir_content_conflict_is_retryable_without_host_path() { + let transport = WorkdirTransportError::from_workdir_error( + &workdir::WorkdirError::Conflict("/runtime/private/checkout/src/main.rs".to_string()), + ); + assert_eq!(transport.code, WorkdirTransportErrorCode::Conflict); + assert_eq!( + transport.message, + "The target file's content or existence changed since it was last observed; read the file again before retrying" + ); + let error = ToolError::from(ToolsError::WorkdirSession(transport.into_workdir_error())); + + match error { + ToolError::ExecutionFailed(message) => { + assert_eq!( + message, + "The target file's content or existence changed since it was last observed; read the file again before retrying" + ); + assert!(!message.contains("/runtime/private")); + } + other => panic!("expected execution failure, got {other:?}"), + } + } +} diff --git a/crates/tools/src/write.rs b/crates/tools/src/write.rs index 08e5f038..4eb56755 100644 --- a/crates/tools/src/write.rs +++ b/crates/tools/src/write.rs @@ -219,8 +219,13 @@ mod tests { ) .await .unwrap_err(); - let msg = format!("{err}"); - assert!(msg.contains("modified externally"), "{msg}"); + match err { + ToolError::ExecutionFailed(message) => assert_eq!( + message, + "The target file's content or existence changed since it was last observed; read the file again before retrying: a.txt" + ), + other => panic!("expected execution failure, got {other:?}"), + } } #[tokio::test] diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index eb3620d1..5a9c8c82 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -174,7 +174,10 @@ impl WorkdirTransportError { use WorkdirTransportErrorCode as Code; let (code, message) = match error { WorkdirError::NotFound(_) => (Code::NotFound, "Workdir path was not found"), - WorkdirError::Conflict(_) => (Code::Conflict, "Workdir content changed"), + WorkdirError::Conflict(_) => ( + Code::Conflict, + "The target file's content or existence changed since it was last observed; read the file again before retrying", + ), WorkdirError::Unsupported(capability) => { return Self { code: Code::Unsupported, @@ -673,7 +676,7 @@ mod tests { ( WorkdirTransportErrorCode::Conflict, 409, - "modified externally", + "The target file's content or existence changed since it was last observed", ), (WorkdirTransportErrorCode::Unsupported, 400, "unsupported"), (WorkdirTransportErrorCode::Denied, 403, "denied"), @@ -715,7 +718,12 @@ mod tests { ] { let transport = WorkdirTransportError { code, - message: "safe provider message".to_string(), + message: if code == WorkdirTransportErrorCode::Conflict { + "The target file's content or existence changed since it was last observed; read the file again before retrying" + .to_string() + } else { + "safe provider message".to_string() + }, }; assert_eq!(code.http_status(), expected_status); let workdir_error = transport.clone().into_workdir_error(); @@ -782,5 +790,21 @@ mod tests { transport.into_workdir_error(), WorkdirError::Io { .. } )); + + let error = WorkdirError::Conflict( + "The target file's content or existence changed since it was last observed; read the file again before retrying: /secret/runtime/root/file" + .to_string(), + ); + let transport = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.code, WorkdirTransportErrorCode::Conflict); + assert_eq!( + transport.message, + "The target file's content or existence changed since it was last observed; read the file again before retrying" + ); + assert!(!transport.message.contains("/secret")); + assert_eq!( + transport.into_workdir_error().to_string(), + "The target file's content or existence changed since it was last observed; read the file again before retrying" + ); } } diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index 3e3ae0de..0356936d 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -235,7 +235,7 @@ pub enum WorkdirError { #[error("Workdir transport failed: {0}")] Transport(String), - #[error("Workdir content was modified externally before the operation could be applied: {0}")] + #[error("{0}")] Conflict(String), #[error("unknown Workdir session command: {0}")] @@ -349,7 +349,9 @@ impl From for WorkdirError { fs_operation::FsError::SymlinkTargetIsDirectory { path, target } => { Self::SymlinkTargetIsDirectory { path, target } } - fs_operation::FsError::Conflict(message) => Self::Conflict(message), + fs_operation::FsError::Conflict(path) => Self::Conflict(format!( + "The target file's content or existence changed since it was last observed; read the file again before retrying: {path}" + )), fs_operation::FsError::InvalidGlob(message) => Self::InvalidGlob(message), fs_operation::FsError::InvalidRegex(message) => Self::InvalidRegex(message), fs_operation::FsError::InvalidArgument(message) => Self::InvalidArgument(message), diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 6bbb0738..982ad446 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -1569,7 +1569,10 @@ mod tests { ) .await .unwrap_err(); - assert!(matches!(error, WorkdirError::Conflict(_))); + assert_eq!( + error.to_string(), + "The target file's content or existence changed since it was last observed; read the file again before retrying: notes/item.txt" + ); std::fs::remove_file(dir.path().join("notes/item.txt")).unwrap(); let error = WorkdirSession::write( @@ -1582,7 +1585,38 @@ mod tests { ) .await .unwrap_err(); - assert!(matches!(error, WorkdirError::Conflict(_))); + assert_eq!( + error.to_string(), + "The target file's content or existence changed since it was last observed; read the file again before retrying: notes/item.txt" + ); + } + + #[tokio::test] + async fn write_conflicts_when_observed_absence_becomes_a_file() { + let dir = TempDir::new().unwrap(); + let workdir = make_fs(&dir); + let path = WorkdirPath::new("race.txt").unwrap(); + + let error = WorkdirSession::stat(&workdir, StatRequest { path: path.clone() }) + .await + .unwrap_err(); + assert!(matches!(error, WorkdirError::NotFound(_))); + + std::fs::write(dir.path().join("race.txt"), "created externally").unwrap(); + let error = WorkdirSession::write( + &workdir, + WriteRequest { + path, + content: b"worker content".to_vec(), + expected_hash: None, + }, + ) + .await + .unwrap_err(); + assert_eq!( + error.to_string(), + "The target file's content or existence changed since it was last observed; read the file again before retrying: race.txt" + ); } #[tokio::test]