Auto merge of #12744 - tompscanlan:atomic-write, r=epage

fix bug: corruption when cargo killed while writing

### What does this PR try to resolve?

fix  #11386, superseding #12362

### How should we test and review this PR?

Added unit test showing basic equivalency to existing `write(path, content)`. Full test suite should exercise write.
Added tests for cargo add and remove. These are timing tests, so take a bit of time to run. 5-10s each.  They may not fail every time, but do so regularly.  Making the change to these two writes seems to prevent me from failing these tests at all.

### Additional information

This uses tempfile::persist which was an existing dependency. atomicwrites crate, an alternative option for this fix, indicates `tempfile::persist` is the same thing.  Since we already use tempfile as a dep, I stuck with that.

Author: bors

crates/cargo-util/src/paths.rs

@@ -180,6 +180,19 @@ pub fn write<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()>
         .with_context(|| format!("failed to write `{}`", path.display()))
 }
 
+/// Writes a file to disk atomically.
+///
+/// write_atomic uses tempfile::persist to accomplish atomic writes.
+pub fn write_atomic<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> {
+    let path = path.as_ref();
+    let mut tmp = TempFileBuilder::new()
+        .prefix(path.file_name().unwrap())
+        .tempfile_in(path.parent().unwrap())?;
+    tmp.write_all(contents.as_ref())?;
+    tmp.persist(path)?;
+    Ok(())
+}
+
 /// Equivalent to [`write()`], but does not write anything if the file contents
 /// are identical to the given contents.
 pub fn write_if_changed<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> {
@@ -775,6 +788,29 @@ fn exclude_from_time_machine(path: &Path) {
 #[cfg(test)]
 mod tests {
     use super::join_paths;
+    use super::write;
+    use super::write_atomic;
+
+    #[test]
+    fn write_works() {
+        let original_contents = "[dependencies]\nfoo = 0.1.0";
+
+        let tmpdir = tempfile::tempdir().unwrap();
+        let path = tmpdir.path().join("Cargo.toml");
+        write(&path, original_contents).unwrap();
+        let contents = std::fs::read_to_string(&path).unwrap();
+        assert_eq!(contents, original_contents);
+    }
+    #[test]
+    fn write_atomic_works() {
+        let original_contents = "[dependencies]\nfoo = 0.1.0";
+
+        let tmpdir = tempfile::tempdir().unwrap();
+        let path = tmpdir.path().join("Cargo.toml");
+        write_atomic(&path, original_contents).unwrap();
+        let contents = std::fs::read_to_string(&path).unwrap();
+        assert_eq!(contents, original_contents);
+    }
 
     #[test]
     fn join_paths_lists_paths_on_error() {

src/bin/cargo/commands/remove.rs

@@ -270,7 +270,10 @@ fn gc_workspace(workspace: &Workspace<'_>) -> CargoResult<()> {
     }
 
     if is_modified {
-        cargo_util::paths::write(workspace.root_manifest(), manifest.to_string().as_bytes())?;
+        cargo_util::paths::write_atomic(
+            workspace.root_manifest(),
+            manifest.to_string().as_bytes(),
+        )?;
     }
 
     Ok(())

src/cargo/util/toml_mut/manifest.rs

@@ -296,7 +296,7 @@ impl LocalManifest {
         let s = self.manifest.data.to_string();
         let new_contents_bytes = s.as_bytes();
 
-        cargo_util::paths::write(&self.path, new_contents_bytes)
+        cargo_util::paths::write_atomic(&self.path, new_contents_bytes)
     }
 
     /// Lookup a dependency.

tests/testsuite/death.rs

@@ -1,12 +1,12 @@
 //! Tests for ctrl-C handling.
 
+use cargo_test_support::{project, slow_cpu_multiplier};
 use std::fs;
 use std::io::{self, Read};
 use std::net::TcpListener;
 use std::process::{Child, Stdio};
 use std::thread;
-
-use cargo_test_support::{project, slow_cpu_multiplier};
+use std::time;
 
 #[cargo_test]
 fn ctrl_c_kills_everyone() {
@@ -87,6 +87,155 @@ fn ctrl_c_kills_everyone() {
     );
 }
 
+#[cargo_test]
+fn kill_cargo_add_never_corrupts_cargo_toml() {
+    cargo_test_support::registry::init();
+    cargo_test_support::registry::Package::new("my-package", "0.1.1+my-package").publish();
+
+    let with_dependency = r#"
+[package]
+name = "foo"
+version = "0.0.1"
+authors = []
+
+[dependencies]
+my-package = "0.1.1"
+"#;
+    let without_dependency = r#"
+[package]
+name = "foo"
+version = "0.0.1"
+authors = []
+"#;
+
+    for sleep_time_ms in [30, 60, 90] {
+        let p = project()
+            .file("Cargo.toml", without_dependency)
+            .file("src/lib.rs", "")
+            .build();
+
+        let mut cargo = p.cargo("add").arg("my-package").build_command();
+        cargo
+            .stdin(Stdio::piped())
+            .stdout(Stdio::piped())
+            .stderr(Stdio::piped());
+
+        let mut child = cargo.spawn().unwrap();
+
+        thread::sleep(time::Duration::from_millis(sleep_time_ms));
+
+        assert!(child.kill().is_ok());
+        assert!(child.wait().is_ok());
+
+        // check the Cargo.toml
+        let contents = fs::read(p.root().join("Cargo.toml")).unwrap();
+
+        // not empty
+        assert_ne!(
+            contents, b"",
+            "Cargo.toml is empty, and should not be at {} milliseconds",
+            sleep_time_ms
+        );
+
+        // We should have the original Cargo.toml or the new one, nothing else.
+        if std::str::from_utf8(&contents)
+            .unwrap()
+            .contains("[dependencies]")
+        {
+            assert_eq!(
+                std::str::from_utf8(&contents).unwrap(),
+                with_dependency,
+                "Cargo.toml is with_dependency after add at {} milliseconds",
+                sleep_time_ms
+            );
+        } else {
+            assert_eq!(
+                std::str::from_utf8(&contents).unwrap(),
+                without_dependency,
+                "Cargo.toml is without_dependency after add at {} milliseconds",
+                sleep_time_ms
+            );
+        }
+    }
+}
+
+#[cargo_test]
+fn kill_cargo_remove_never_corrupts_cargo_toml() {
+    let with_dependency = r#"
+[package]
+name = "foo"
+version = "0.0.1"
+authors = []
+build = "build.rs"
+
+[dependencies]
+bar = "0.0.1"
+"#;
+    let without_dependency = r#"
+[package]
+name = "foo"
+version = "0.0.1"
+authors = []
+build = "build.rs"
+"#;
+
+    // This test depends on killing the cargo process at the right time to cause a failed write.
+    // Note that we're iterating and using the index as time in ms to sleep before killing the cargo process.
+    // If it is working correctly, we never fail, but can't hang out here all day...
+    // So we'll just run it a few times and hope for the best.
+    for sleep_time_ms in [30, 60, 90] {
+        // new basic project with a single dependency
+        let p = project()
+            .file("Cargo.toml", with_dependency)
+            .file("src/lib.rs", "")
+            .build();
+
+        // run cargo remove the dependency
+        let mut cargo = p.cargo("remove").arg("bar").build_command();
+        cargo
+            .stdin(Stdio::piped())
+            .stdout(Stdio::piped())
+            .stderr(Stdio::piped());
+
+        let mut child = cargo.spawn().unwrap();
+
+        thread::sleep(time::Duration::from_millis(sleep_time_ms));
+
+        assert!(child.kill().is_ok());
+        assert!(child.wait().is_ok());
+
+        // check the Cargo.toml
+        let contents = fs::read(p.root().join("Cargo.toml")).unwrap();
+
+        // not empty
+        assert_ne!(
+            contents, b"",
+            "Cargo.toml is empty, and should not be at {} milliseconds",
+            sleep_time_ms
+        );
+
+        // We should have the original Cargo.toml or the new one, nothing else.
+        if std::str::from_utf8(&contents)
+            .unwrap()
+            .contains("[dependencies]")
+        {
+            assert_eq!(
+                std::str::from_utf8(&contents).unwrap(),
+                with_dependency,
+                "Cargo.toml is not the same as the original at {} milliseconds",
+                sleep_time_ms
+            );
+        } else {
+            assert_eq!(
+                std::str::from_utf8(&contents).unwrap(),
+                without_dependency,
+                "Cargo.toml is not the same as expected at {} milliseconds",
+                sleep_time_ms
+            );
+        }
+    }
+}
+
 #[cfg(unix)]
 pub fn ctrl_c(child: &mut Child) {
     let r = unsafe { libc::kill(-(child.id() as i32), libc::SIGINT) };