fix: atomic state writes and surface flush errors in disk_io (#344)

* fix: surface flush errors and write atomically in disk_io::write

`BufWriter`'s `Drop` impl silently discards flush errors. Because
`patches_state.json` and `state.json` are small enough to fit inside
`BufWriter`'s 8 KB buffer, the only time the bytes actually reach disk
is during the implicit flush at drop — and that flush's I/O errors
(transient iOS Data Protection lock, ENOSPC, etc.) were invisible to
the caller. `disk_io::write` returned Ok, `install_patch` returned Ok,
`update()` returned `UpdateInstalled`, but `patches_state.json` was
left at 0 bytes. On the next launch, `load_patches_state` failed to
deserialize and fell back to default (`next_boot_patch: None`), so the
subsequent `checkForUpdate()` saw the server's patch as newly
installable and reported `UpdateStatus.outdated` despite the app
having "successfully" installed it moments earlier.

Change `disk_io::write` to:
  - Write to a sibling `<file>.tmp` and atomically `rename` into place,
    so `path` is never observed in a truncated/empty state by a
    concurrent or post-crash reader.
  - Explicitly unwrap the `BufWriter` via `into_inner()`, which calls
    `flush_buf` and returns any I/O error as `IntoInnerError` instead
    of dropping it on the floor.
  - Clean up the temp file on failure.

Extract `serialize_and_flush` so the flush-error path is unit-testable
without filesystem tricks. Add three tests:
  - temp file is cleaned up after a successful write
  - a failed write preserves the existing file at \`path\`
  - regression: \`serialize_and_flush\` surfaces inner-writer errors
    (confirmed to fail on the pre-fix code)

* style: apply cargo fmt and rename roundtripped -> reloaded for cspell

* test: drop unreachable flush body in FailingWriter

* test: drop Result<()>/? boilerplate in new tests

library/src/cache/disk_io.rs

@@ -3,8 +3,8 @@ use anyhow::{bail, Context};
 use serde::{de::DeserializeOwned, Serialize};
 use std::{
     fs::File,
-    io::{BufReader, BufWriter},
-    path::Path,
+    io::{BufReader, BufWriter, Write},
+    path::{Path, PathBuf},
 };
 
 pub fn write<S, P>(serializable: &S, path: &P) -> anyhow::Result<()>
@@ -24,10 +24,54 @@ where
     std::fs::create_dir_all(containing_dir)
         .with_file_context(FileOperation::CreateDir, containing_dir)?;
 
-    let file = File::create(path).with_file_context(FileOperation::CreateFile, path_as_ref)?;
-    let writer = BufWriter::new(file);
-    serde_json::to_writer_pretty(writer, serializable)
-        .with_context(|| format!("failed to serialize to {:?}", path_as_ref))
+    // Write to a sibling temp file first, then atomically rename into place.
+    // Two problems with writing directly to `path`:
+    //   1. `BufWriter`'s `Drop` impl silently discards flush errors, so a
+    //      transient I/O failure (iOS Data Protection lock, ENOSPC) on the
+    //      final flush leaves a zero-byte file on disk with no error returned.
+    //   2. A crash or power loss between `File::create` (which truncates) and
+    //      the final write would leave an empty/partial file where a valid
+    //      state file used to be.
+    // The sibling-write-then-rename pattern fixes both: the caller sees a
+    // flush error (we unwrap `BufWriter` below), and on-disk `path` is only
+    // replaced by a fully-written sibling via an atomic `rename`.
+    let temp_path = temp_sibling_path(path_as_ref);
+    let file = File::create(&temp_path).with_file_context(FileOperation::CreateFile, &temp_path)?;
+    if let Err(err) = serialize_and_flush(serializable, file)
+        .with_context(|| format!("failed to serialize to {:?}", &temp_path))
+    {
+        // Best-effort cleanup so a failed write doesn't leave orphan temp files.
+        let _ = std::fs::remove_file(&temp_path);
+        return Err(err);
+    }
+    std::fs::rename(&temp_path, path_as_ref)
+        .with_file_context(FileOperation::RenameFile, &temp_path)
+}
+
+/// Serializes `value` as pretty JSON into `writer`, then explicitly unwraps
+/// the internal `BufWriter` so any flush error surfaces to the caller instead
+/// of being silently discarded by `BufWriter`'s `Drop` impl.
+fn serialize_and_flush<S, W>(value: &S, writer: W) -> anyhow::Result<()>
+where
+    S: ?Sized + Serialize,
+    W: Write,
+{
+    let mut buf_writer = BufWriter::new(writer);
+    serde_json::to_writer_pretty(&mut buf_writer, value)?;
+    // `into_inner` calls `flush_buf` internally; any I/O error from writing
+    // out the buffered bytes comes back as `IntoInnerError` rather than being
+    // dropped on the floor.
+    buf_writer
+        .into_inner()
+        .map_err(|e| anyhow::Error::new(e.into_error()))?;
+    Ok(())
+}
+
+/// Returns a sibling path in the same directory with a `.tmp` suffix,
+/// e.g. `/a/b/state.json` -> `/a/b/state.json.tmp`.
+fn temp_sibling_path(path: &Path) -> PathBuf {
+    let file_name = path.file_name().and_then(|s| s.to_str()).unwrap_or("state");
+    path.with_file_name(format!("{file_name}.tmp"))
 }
 
 pub fn read<D, P>(path: &P) -> anyhow::Result<D>
@@ -55,7 +99,7 @@ mod test {
     use serde::{Deserialize, Serialize};
     use tempfile::TempDir;
 
-    use anyhow::{Ok, Result};
+    use anyhow::Result;
 
     #[derive(Serialize, Deserialize, PartialEq, Eq)]
     struct TestStruct {
@@ -94,4 +138,87 @@ mod test {
 
         Ok(())
     }
+
+    #[test]
+    fn write_does_not_leave_temp_file_on_success() {
+        let temp_dir = TempDir::new().unwrap();
+        let path = temp_dir.path().join("state.json");
+        super::write(
+            &TestStruct {
+                a: 1,
+                b: "hi".into(),
+            },
+            &path,
+        )
+        .unwrap();
+        assert!(path.exists());
+        assert!(!temp_dir.path().join("state.json.tmp").exists());
+    }
+
+    #[test]
+    fn write_preserves_existing_file_on_serialization_failure() {
+        // Struct whose Serialize impl always fails — simulates an I/O error
+        // encountered during serialization without needing filesystem tricks.
+        struct FailingSerialize;
+        impl serde::Serialize for FailingSerialize {
+            fn serialize<S: serde::Serializer>(
+                &self,
+                _: S,
+            ) -> std::result::Result<S::Ok, S::Error> {
+                Err(serde::ser::Error::custom("simulated failure"))
+            }
+        }
+
+        let temp_dir = TempDir::new().unwrap();
+        let path = temp_dir.path().join("state.json");
+        let original = TestStruct {
+            a: 42,
+            b: "original".into(),
+        };
+        super::write(&original, &path).unwrap();
+
+        // Second write fails; the existing file at `path` must still hold the
+        // original contents (the failed write goes to the sibling temp file
+        // and never clobbers `path`).
+        assert!(super::write(&FailingSerialize, &path).is_err());
+        let reloaded: TestStruct = super::read(&path).unwrap();
+        assert!(reloaded == original);
+        // Temp file was cleaned up.
+        assert!(!temp_dir.path().join("state.json.tmp").exists());
+    }
+
+    // Regression test for the bug where `BufWriter`'s `Drop` impl silently
+    // discards flush errors, producing a spurious Ok() from `write` while
+    // the on-disk file ended up empty or partial. `serialize_and_flush`
+    // must surface such errors.
+    #[test]
+    fn serialize_and_flush_surfaces_error_from_inner_writer() {
+        // A Write impl that fails on every write call. All of serde_json's
+        // output for a small struct fits inside BufWriter's buffer, so the
+        // inner writer's `write` only gets called when the buffer is drained
+        // — either by an explicit flush/into_inner (fix) or by Drop (bug).
+        struct FailingWriter;
+        impl std::io::Write for FailingWriter {
+            fn write(&mut self, _buf: &[u8]) -> std::io::Result<usize> {
+                Err(std::io::Error::other("simulated flush failure"))
+            }
+            // `BufWriter::into_inner` drains its buffer via the inner
+            // writer's `write`, not its `flush`, so this path is not
+            // exercised by the test. Required by the trait.
+            fn flush(&mut self) -> std::io::Result<()> {
+                Ok(())
+            }
+        }
+
+        let value = TestStruct {
+            a: 1,
+            b: "hi".into(),
+        };
+        let result = super::serialize_and_flush(&value, FailingWriter);
+        assert!(result.is_err());
+        assert!(result
+            .unwrap_err()
+            .to_string()
+            .contains("simulated flush failure"));
+    }
 }