Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions crates/fbuild-core/src/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Path>,
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::*;
Expand Down Expand Up @@ -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
Expand Down
42 changes: 16 additions & 26 deletions crates/fbuild-daemon/src/status_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {}: {}",
Expand Down
Loading