From ca8f0a9f36c487bd7dc5f2f74a41c4333c5f5da2 Mon Sep 17 00:00:00 2001 From: Eric Seidel Date: Tue, 21 Apr 2026 11:21:49 -0700 Subject: [PATCH] fix: atomic state writes and surface flush errors in disk_io (#344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 `.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 | 141 +++++++++++++++++++++++++++++++++-- 1 file changed, 134 insertions(+), 7 deletions(-) diff --git a/library/src/cache/disk_io.rs b/library/src/cache/disk_io.rs index eef1495..1018db4 100644 --- a/library/src/cache/disk_io.rs +++ b/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(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(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(path: &P) -> anyhow::Result @@ -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( + &self, + _: S, + ) -> std::result::Result { + 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 { + 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")); + } }