Merge remote-tracking branch 'refs/remotes/origin/develop' into work/T-612-persisted-worker-identity

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:?}"),
+        }
     }
 }

crates/tools/src/error.rs

@@ -43,7 +43,8 @@ impl From<ToolsError> 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<ToolsError> 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:?}"),
+        }
+    }
+}

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]

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"
+        );
     }
 }

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<fs_operation::FsError> 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),

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]