From cb9635e5128dc64cca59950df9f08a3adc52d670 Mon Sep 17 00:00:00 2001 From: zackees Date: Tue, 30 Jun 2026 11:02:24 -0700 Subject: [PATCH] fix(daemon): write daemon status via sync write_atomic (#865) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `StatusManager::write_atomic` bridged to the async `fbuild_core::fs::write_atomic` via `tokio::task::block_in_place` + `Handle::block_on`. `block_in_place` panics inside a current-thread tokio runtime — which is the default flavor of `#[tokio::test]` — so every unit test that constructed a `DaemonContext` (which constructs a `StatusManager`, whose `new` calls `flush()`, which calls `write_atomic`) panicked on macOS + Windows CI. Three named tests are affected: - handlers::health::tests::shutdown_refuses_non_force_when_operation_in_progress - handlers::locks::tests::lock_status_reports_held_project_locks_without_stale_entries - handlers::locks::tests::clear_locks_removes_only_unheld_project_lock_entries Fix: add `fbuild_core::fs::write_atomic_sync` — same write-tempfile / fsync / rename semantics as the async helper, but `std::fs` end-to-end so it has no runtime-flavor requirement. Switch `StatusManager::write_atomic` to call it. The status file's atomicity contract is preserved verbatim; the only behavior change is that the call is no longer runtime-aware. Two new regression tests in `fs::tests`: - `write_atomic_sync_runs_inside_current_thread_runtime` (the exact scenario that was panicking) - `write_atomic_sync_works_without_runtime` (cold-bootstrap path) Closes #865 --- crates/fbuild-core/src/fs.rs | 113 +++++++++++++++++++++ crates/fbuild-daemon/src/status_manager.rs | 42 +++----- 2 files changed, 129 insertions(+), 26 deletions(-) diff --git a/crates/fbuild-core/src/fs.rs b/crates/fbuild-core/src/fs.rs index 56526965..1e17700e 100644 --- a/crates/fbuild-core/src/fs.rs +++ b/crates/fbuild-core/src/fs.rs @@ -142,6 +142,88 @@ pub async fn write_atomic( Ok(()) } +/// Synchronous companion to [`write_atomic`] — same atomic-rename +/// semantics, but `std::fs` end-to-end so it's callable from a +/// `current-thread` tokio runtime without any block-on / `block_in_place` +/// gymnastics. +/// +/// FastLED/fbuild#865: `StatusManager::write_atomic` previously bridged +/// to the async [`write_atomic`] via `block_in_place` + `Handle::block_on`. +/// `block_in_place` panics inside a current-thread runtime, which is the +/// default flavor of `#[tokio::test]`, so every unit test that touched +/// the status writer panicked on macOS + Windows CI. +/// +/// Use this from any sync caller (the daemon's status writer, in-process +/// snapshots, test harnesses). Prefer [`write_atomic`] from async paths +/// that need to avoid blocking the reactor on the fsync. +pub fn write_atomic_sync( + path: impl AsRef, + content: impl AsRef<[u8]>, +) -> std::io::Result<()> { + use std::io::Write as _; + + let path = path.as_ref(); + let nonce = WRITE_ATOMIC_NONCE.fetch_add(1, Ordering::Relaxed); + let pid = std::process::id(); + + let mut tmp_name = path + .file_name() + .map(|s| s.to_os_string()) + .unwrap_or_default(); + if tmp_name.is_empty() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "write_atomic_sync: target path has no file name", + )); + } + tmp_name.push(format!(".tmp.{pid}.{nonce}")); + let tmp_path = match path.parent() { + Some(parent) => parent.join(&tmp_name), + None => std::path::PathBuf::from(&tmp_name), + }; + + // Mirror `write_atomic`'s parent-must-exist contract. + if let Some(parent) = path.parent() { + if !parent.as_os_str().is_empty() { + match std::fs::metadata(parent) { + Ok(_) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + return Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + format!( + "write_atomic_sync: parent directory does not exist: {}", + parent.display() + ), + )); + } + Err(e) => return Err(e), + } + } + } + + { + let mut file = std::fs::File::create(&tmp_path)?; + if let Err(e) = file.write_all(content.as_ref()) { + let _ = std::fs::remove_file(&tmp_path); + return Err(e); + } + if let Err(e) = file.sync_all() { + let _ = std::fs::remove_file(&tmp_path); + return Err(e); + } + // Drop before rename — same Windows MoveFileExW share-mode + // constraint that `write_atomic` documents. + drop(file); + } + + if let Err(e) = std::fs::rename(&tmp_path, path) { + let _ = std::fs::remove_file(&tmp_path); + return Err(e); + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -194,6 +276,37 @@ mod tests { assert_eq!(err.kind(), std::io::ErrorKind::NotFound); } + // FastLED/fbuild#865 regression: `write_atomic_sync` must be safe to + // call inside a current-thread tokio runtime (the default flavor of + // `#[tokio::test]`) because that's the path the daemon's status + // writer takes from unit tests. The previous async bridge panicked + // on `block_in_place` here. + #[tokio::test] + async fn write_atomic_sync_runs_inside_current_thread_runtime() { + let dir = tempdir().unwrap(); + let target = dir.path().join("state.json"); + write_atomic_sync(&target, b"sync-payload").unwrap(); + let got = std::fs::read(&target).unwrap(); + assert_eq!(got, b"sync-payload"); + } + + #[test] + fn write_atomic_sync_works_without_runtime() { + let dir = tempdir().unwrap(); + let target = dir.path().join("state.json"); + write_atomic_sync(&target, b"no-runtime").unwrap(); + let got = std::fs::read(&target).unwrap(); + assert_eq!(got, b"no-runtime"); + } + + #[test] + fn write_atomic_sync_errors_when_parent_missing() { + let dir = tempdir().unwrap(); + let target = dir.path().join("does_not_exist").join("state.json"); + let err = write_atomic_sync(&target, b"x").unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::NotFound); + } + #[tokio::test] async fn write_atomic_handles_concurrent_writes() { // Two concurrent writes to the same target must each complete diff --git a/crates/fbuild-daemon/src/status_manager.rs b/crates/fbuild-daemon/src/status_manager.rs index 1e7b7cf1..c808eaf4 100644 --- a/crates/fbuild-daemon/src/status_manager.rs +++ b/crates/fbuild-daemon/src/status_manager.rs @@ -191,15 +191,13 @@ impl StatusManager { } } - /// Atomic write: route through the canonical async helper - /// `fbuild_core::fs::write_atomic` so the daemon-side state file - /// uses the same write-tempfile / fsync / rename path as every - /// other state-file writer (FastLED/fbuild#844 bridge pair 6). - /// - /// Called from synchronous code paths inside the tokio runtime, so - /// we bridge to the async helper via `block_in_place` + - /// `Handle::block_on`. Matches the pattern in - /// `fbuild_packages::toolchain::esp32_metadata::resolve_toolchain_url_sync`. + /// Atomic write: route through the sync helper + /// `fbuild_core::fs::write_atomic_sync` so the daemon-side state + /// file uses the same write-tempfile / fsync / rename path as + /// every other state-file writer (FastLED/fbuild#844 bridge + /// pair 6) without any tokio-runtime-flavor sniffing + /// (FastLED/fbuild#865 — the previous async bridge panicked + /// inside current-thread runtimes used by unit tests). fn write_atomic(&self, status: &DaemonStatus) { let json = match serde_json::to_string_pretty(status) { Ok(j) => j, @@ -216,23 +214,15 @@ impl StatusManager { let _ = std::fs::create_dir_all(parent); } - let path = self.path.clone(); - let result = if let Ok(handle) = tokio::runtime::Handle::try_current() { - tokio::task::block_in_place(|| { - handle.block_on(fbuild_core::fs::write_atomic(&path, json.as_bytes())) - }) - } else { - // No tokio runtime — spin up a one-shot. Path hit only by - // unit tests and the (rare) standalone bootstrap before the - // daemon's runtime exists. - match tokio::runtime::Runtime::new() { - Ok(rt) => rt.block_on(fbuild_core::fs::write_atomic(&path, json.as_bytes())), - Err(e) => { - tracing::warn!("failed to create tokio runtime for status write: {}", e); - return; - } - } - }; + // FastLED/fbuild#865: write the daemon status synchronously. + // The previous bridge used `tokio::task::block_in_place` + + // `Handle::block_on`, which panics inside a current-thread + // runtime — the default flavor of `#[tokio::test]`, so unit + // tests touching this path failed on macOS + Windows CI. + // `write_atomic_sync` has identical write+fsync+rename + // semantics via `std::fs`, so the status file's atomicity + // contract is preserved without any runtime-flavor sniffing. + let result = fbuild_core::fs::write_atomic_sync(&self.path, json.as_bytes()); if let Err(e) = result { tracing::warn!( "failed to atomically write daemon status to {}: {}",