Avoid clobbering concurrently created rc files

CLAUDE.md

@@ -59,7 +59,8 @@ Never risk data loss without explicit user consent. A failed command that preser
 - **No implicit destructive side effects** — never silently delete/overwrite as a side effect of an unrelated operation; make cleanup a separate explicit action the user chooses.
 - **Favor the failing variant on races** — `git reset --keep` (fails if tracked files were modified) over `--hard`; `git checkout --merge` over `--force`. If no safer variant exists, document the risk inline.
 - **Time-of-check vs time-of-use** — be conservative when there's a gap between the safety check and the operation. `wt merge` verifies clean before rebasing, but files could appear before cleanup — don't force-remove during cleanup.
-- **Replace full files, never truncate them** — `fs::write` truncates before it writes, so a crash mid-write leaves the file empty. Every full-file rewrite worktrunk can't put back (shell wrappers, `config.toml`, `approvals.toml`, another tool's `settings.json`) goes through `utils::write_atomically`, which renames a sibling temp file over the target; the spec on that function covers symlinks, mode, and what a rename costs. Adding to a user-owned file (the Bash, Zsh, and PowerShell rc files) instead opens it in append mode, so a stale snapshot cannot replace concurrent edits; offer the addition as one buffer, and report a short write without truncating the file. Don't grow the install path into a rebuild. Removal still rewrites those rc files whole (`uninstall_previewed_lines`), which is why the `write_atomically` spec still names them. Regenerable content (the cache, the `-vv` diagnostic report) keeps the plain write.
+- **Replace full files, never truncate them** — `fs::write` truncates before it writes, so a crash mid-write leaves the file empty. Every full-file rewrite worktrunk can't put back (shell wrappers, `config.toml`, `approvals.toml`, another tool's `settings.json`) goes through `utils::write_atomically`, which renames a sibling temp file over the target; the spec on that function covers symlinks, mode, and what a rename costs. When a user-owned file was observed missing, `utils::write_new_atomically` refuses to overwrite one that appears before persistence. Adding to an existing Bash, Zsh, or PowerShell rc file instead opens it in append mode, so a stale snapshot cannot replace concurrent edits; never truncate the file to recover from an append error. Don't grow the install path into a rebuild. Removal still rewrites those rc files whole (`uninstall_previewed_lines`), which is why the `write_atomically` spec still names them. Regenerable content (the cache, the `-vv` diagnostic report) keeps the plain write.
+- **Shell-config concurrency has a deliberate boundary** — the append lock coordinates Worktrunk installers, and no-clobber creation turns the missing-file race into a fail-and-rerun outcome. An editor save racing rc-file uninstall can still be overwritten; accept this final check-to-rename window because sidecar locks, backups, retries, and extra pre-rename checks do not close it. Revisit after an observed incident or a simpler write design. Worktrunk-owned wrappers and completions use last-writer-wins semantics, as does the merge into another tool's `settings.json`.
 
 These stop where git's own protections stop, and matching git is deliberate in each case. The named spec says why:
 

src/commands/configure_shell.rs

@@ -11,7 +11,7 @@ use worktrunk::styling::{
     INFO_SYMBOL, SUCCESS_SYMBOL, eprint, eprintln, format_bash_with_gutter, format_toml,
     format_with_gutter, hint_message, println, prompt_message, warning_message,
 };
-use worktrunk::utils::write_atomically;
+use worktrunk::utils::{write_atomically, write_new_atomically};
 
 use crate::output::prompt::{PromptResponse, prompt_yes_no_preview};
 use crate::output::shell_integration::shell_extension_label;
@@ -717,14 +717,10 @@ fn configure_shell_file(
                 })?;
             }
 
-            // Write the config content
-            write_atomically(path, &format!("{}\n", config_line)).map_err(|e| {
-                format!(
-                    "Failed to write to {}: {}",
-                    format_path_for_display(path),
-                    e
-                )
-            })?;
+            // Another process may create the rc file after the existence check.
+            // Fail in that case so its contents survive and a rerun can append.
+            write_new_atomically(path, &format!("{}\n", config_line))
+                .map_err(|e| format_new_rc_error(path, &e))?;
 
             Ok(Some(ConfigureResult {
                 shell,
@@ -739,6 +735,21 @@ fn configure_shell_file(
     }
 }
 
+fn format_new_rc_error(path: &Path, error: &io::Error) -> String {
+    let display_path = format_path_for_display(path);
+    if error.kind() == io::ErrorKind::AlreadyExists {
+        if path.is_symlink() && !path.exists() {
+            return format!(
+                "Failed to create {display_path}: path is a dangling symlink; restore its target or remove the link, then rerun"
+            );
+        }
+        return format!(
+            "Failed to create {display_path}: another process created it first; rerun to append"
+        );
+    }
+    format!("Failed to write to {display_path}: {error}")
+}
+
 fn open_locked_rc_file(path: &Path) -> Result<fs::File, String> {
     let file = fs::OpenOptions::new()
         .read(true)
@@ -761,35 +772,6 @@ fn open_locked_rc_file(path: &Path) -> Result<fs::File, String> {
     Ok(file)
 }
 
-/// Offer an rc-file addition in one successful write.
-///
-/// `write_all` can append part of its buffer and then fail on a later write,
-/// hiding how much of the shell line reached the file. A single short write is
-/// instead an error of its own. Never truncate the user-owned file in response:
-/// no portable operation can keep a later concurrent append out of that race.
-fn write_append_buffer(
-    mut write_once: impl FnMut(&[u8]) -> io::Result<usize>,
-    addition: &[u8],
-) -> io::Result<()> {
-    let written = loop {
-        match write_once(addition) {
-            Err(error) if error.kind() == io::ErrorKind::Interrupted => continue,
-            result => break result?,
-        }
-    };
-    if written == addition.len() {
-        return Ok(());
-    }
-
-    Err(io::Error::new(
-        io::ErrorKind::WriteZero,
-        format!(
-            "only {written} of {} bytes were written; the file may end with a partial Worktrunk shell integration line",
-            addition.len()
-        ),
-    ))
-}
-
 /// Recheck an open rc file and append the integration line if it is still absent.
 ///
 /// The caller holds the file's exclusive lock. Re-reading through the same
@@ -812,9 +794,10 @@ fn append_shell_integration_if_missing(
     }
 
     // Add a blank line before config, then the config line with its own newline.
-    // Offer the whole addition as one buffer rather than split formatted writes.
+    // A failed append may leave part of this Worktrunk-owned line behind; never
+    // truncate the user-owned file in an attempt to roll it back.
     let addition = format!("\n{config_line}\n");
-    write_append_buffer(|buffer| file.write(buffer), addition.as_bytes()).map_err(|e| {
+    file.write_all(addition.as_bytes()).map_err(|e| {
         format!(
             "Failed to write to {}: {}",
             format_path_for_display(path),
@@ -1988,6 +1971,85 @@ mod tests {
         assert_eq!(fs::read(&rc).unwrap(), content);
     }
 
+    #[cfg(unix)]
+    #[test]
+    fn test_configure_shell_reports_dangling_symlink() {
+        use std::os::unix::fs::symlink;
+
+        let dir = tempfile::TempDir::new().unwrap();
+        let rc = dir.path().join(".zshrc");
+        let target = dir.path().join("missing-target");
+        symlink(&target, &rc).unwrap();
+
+        let error = configure_shell_file(Shell::Zsh, &rc, false, true, "wt")
+            .err()
+            .expect("dangling symlink should be rejected");
+
+        assert_eq!(
+            error,
+            format!(
+                "Failed to create {}: path is a dangling symlink; restore its target or remove the link, then rerun",
+                format_path_for_display(&rc)
+            )
+        );
+        assert_eq!(fs::read_link(&rc).unwrap(), target);
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_configure_shell_reports_non_symlink_create_error() {
+        let dir = tempfile::TempDir::new().unwrap();
+        let rc = dir.path().join("x".repeat(300));
+
+        let error = configure_shell_file(Shell::Zsh, &rc, false, true, "wt")
+            .err()
+            .expect("overlong filename should be rejected");
+
+        assert!(
+            error.starts_with(&format!(
+                "Failed to write to {}:",
+                format_path_for_display(&rc)
+            )),
+            "{error}"
+        );
+        assert!(!error.contains("dangling symlink"), "{error}");
+    }
+
+    #[test]
+    fn test_new_rc_error_reports_concurrent_creator() {
+        let path = Path::new("/home/user/.zshrc");
+        let error = io::Error::from(io::ErrorKind::AlreadyExists);
+
+        assert_eq!(
+            format_new_rc_error(path, &error),
+            format!(
+                "Failed to create {}: another process created it first; rerun to append",
+                format_path_for_display(path)
+            )
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_new_rc_error_reports_live_symlink_as_concurrent_creator() {
+        use std::os::unix::fs::symlink;
+
+        let dir = tempfile::TempDir::new().unwrap();
+        let target = dir.path().join("target");
+        let path = dir.path().join(".zshrc");
+        fs::write(&target, "# concurrent config\n").unwrap();
+        symlink(target, &path).unwrap();
+        let error = io::Error::from(io::ErrorKind::AlreadyExists);
+
+        assert_eq!(
+            format_new_rc_error(&path, &error),
+            format!(
+                "Failed to create {}: another process created it first; rerun to append",
+                format_path_for_display(&path)
+            )
+        );
+    }
+
     #[test]
     fn test_configure_shell_appends_without_replacing_existing_file() {
         let dir = tempfile::TempDir::new().unwrap();
@@ -2038,82 +2100,6 @@ mod tests {
         )));
     }
 
-    #[test]
-    fn test_append_write_retries_interrupt_and_reports_incomplete_writes() {
-        let original = b"export EDITOR=hx\n";
-        let addition = b"\nif command -v wt; then eval ...\n";
-        let mut short_content = original.to_vec();
-        let mut short_calls = 0;
-
-        let error = write_append_buffer(
-            |buffer| {
-                short_calls += 1;
-                let written = buffer.len().min(12);
-                short_content.extend_from_slice(&buffer[..written]);
-                Ok(written)
-            },
-            addition,
-        )
-        .unwrap_err();
-
-        assert_eq!(error.kind(), io::ErrorKind::WriteZero);
-        assert!(
-            error
-                .to_string()
-                .contains("partial Worktrunk shell integration line")
-        );
-        assert_eq!(
-            short_content,
-            [original.as_slice(), &addition[..12]].concat()
-        );
-        assert_eq!(short_calls, 1, "a short append must not write again");
-
-        let mut interrupted_content = original.to_vec();
-        let mut interrupted_calls = 0;
-        write_append_buffer(
-            |buffer| {
-                interrupted_calls += 1;
-                if interrupted_calls == 1 {
-                    return Err(io::Error::new(
-                        io::ErrorKind::Interrupted,
-                        "injected write error",
-                    ));
-                }
-                interrupted_content.extend_from_slice(buffer);
-                Ok(buffer.len())
-            },
-            addition,
-        )
-        .unwrap();
-
-        assert_eq!(
-            interrupted_content,
-            [original.as_slice(), addition.as_slice()].concat()
-        );
-        assert_eq!(interrupted_calls, 2);
-
-        let mut failed_calls = 0;
-        let error = write_append_buffer(
-            |_| {
-                failed_calls += 1;
-                Err(io::Error::new(
-                    io::ErrorKind::StorageFull,
-                    "injected write error",
-                ))
-            },
-            addition,
-        )
-        .unwrap_err();
-
-        assert_eq!(error.kind(), io::ErrorKind::StorageFull);
-        assert!(
-            !error
-                .to_string()
-                .contains("partial Worktrunk shell integration line")
-        );
-        assert_eq!(failed_calls, 1);
-    }
-
     #[test]
     fn test_append_shell_integration_reports_write_errors() {
         let dir = tempfile::TempDir::new().unwrap();
@@ -2136,7 +2122,6 @@ mod tests {
             "Failed to write to {}:",
             format_path_for_display(&rc)
         )));
-        assert!(!error.contains("partial Worktrunk shell integration line"));
         assert_eq!(fs::read_to_string(&rc).unwrap(), original);
     }
 

src/utils.rs

@@ -138,6 +138,20 @@ pub fn epoch_now() -> u64 {
 /// did not, so a read-only directory holding a writable file now fails with the
 /// file left as it was — the trade Data Safety asks for.
 pub fn write_atomically(path: &Path, content: &str) -> io::Result<()> {
+    write_atomically_with(path, content, true)
+}
+
+/// Create `path` atomically, failing if another writer creates it first.
+///
+/// Use this when the caller observed that the path was absent and must not
+/// overwrite content that appears before the temp file is persisted. A
+/// dangling symlink already occupies the path, so this returns `AlreadyExists`
+/// instead of replacing the link as [`write_atomically`] does.
+pub fn write_new_atomically(path: &Path, content: &str) -> io::Result<()> {
+    write_atomically_with(path, content, false)
+}
+
+fn write_atomically_with(path: &Path, content: &str, overwrite: bool) -> io::Result<()> {
     let target = dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf());
     // A bare filename's parent is `Some("")`, which no directory call accepts.
     let dir = target
@@ -162,7 +176,11 @@ pub fn write_atomically(path: &Path, content: &str) -> io::Result<()> {
     }
 
     temp.as_file().sync_all()?;
-    temp.persist(&target).map_err(|e| e.error)?;
+    if overwrite {
+        temp.persist(&target).map_err(|e| e.error)?;
+    } else {
+        temp.persist_noclobber(&target).map_err(|e| e.error)?;
+    }
 
     Ok(())
 }
@@ -186,6 +204,21 @@ mod tests {
         assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1);
     }
 
+    #[test]
+    fn test_write_new_atomically_creates_but_never_replaces() {
+        let dir = tempfile::tempdir().unwrap();
+        let path = dir.path().join("config.toml");
+
+        write_new_atomically(&path, "first\n").unwrap();
+        assert_eq!(std::fs::read_to_string(&path).unwrap(), "first\n");
+
+        assert!(write_new_atomically(&path, "second\n").is_err());
+        assert_eq!(std::fs::read_to_string(&path).unwrap(), "first\n");
+
+        // A failed persist removes its temp file.
+        assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1);
+    }
+
     /// A file with no target to copy a mode from keeps the temp file's `0600`,
     /// narrower than the `0644` a truncating write leaves under a default
     /// umask.