From 311ce8156c0245fd1902bfd510f3d89e7f738a50 Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 21:42:09 -0700 Subject: [PATCH 1/5] fix(ci): retry ETXTBSY spawns and stop reporting them as missing libraries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes FastLED/fbuild#1366. `Check (ubuntu-latest)` could fail at random on any PR with: probe_linux_exports_the_bundle_on_ld_library_path "without the bundle the stub must fail like a real missing .so" The test writes a shell script, marks it executable, and runs it. Linux refuses to `exec` a file while any process holds a writable descriptor for it — including a `fork`ed child of this process that has not reached its own `exec` yet. libtest runs cases on parallel threads and several of them spawn subprocesses, so a sibling's fork can inherit the descriptor and the exec fails with `ETXTBSY` for a reason that has nothing to do with the script. Evidence it was scheduling, not code: green at `78d8f27e` and `8dd5aace`, red at `ef687774`, then green again on a re-run of the identical tree — where the only delta was a `HashMap` key change in a crate the test cannot reach. ## Two changes, both narrow **Retry, opt-in.** `run_command_retrying_exec_busy` (+ blocking form) retries a spawn that fails with `ETXTBSY`, three attempts, 25 ms backoff, on a `tokio::time::sleep` so it never blocks a runtime worker. Only that error is retried — a missing binary must still fail on the first attempt rather than three times slowly. Deliberately *not* wired into every subprocess in fbuild: retrying an exec is a behavior change, and only callers with the write-then-exec shape need it. `probe_qemu_binary` is the one caller. **`QemuProbe::SpawnFailed`.** The probe collapsed spawn failures and odd exit codes into `Inconclusive`, so a failed exec surfaced through an assertion about shared libraries — which is what made this take a diagnosis cycle instead of being self-describing. "Never started" is now distinct from "started and could not find a .so", and carries the OS error, logged at `warn`. Behavior is unchanged: both still map to `Ok(())`, so the production path is exactly as forgiving as before. ## Verified on Linux, in a container, because the tests are Linux-gated - `a_held_write_handle_blocks_exec_for_the_whole_retry_budget` — fully deterministic: the handle outlives the retry budget, so exec must fail. This reproduces #1366's mechanism with no thread timing at all. - `a_handle_released_mid_window_lets_the_retry_through` — proves the retry is what fixes it. RED/GREEN confirmed: with `EXEC_BUSY_ATTEMPTS = 1` this test fails and the deterministic one still passes. - `only_executable_file_busy_is_retryable` — platform-independent guard that no other error kind gets retried. The 10 ms hold is deliberately small against the ~75 ms budget so a loaded runner cannot turn this into the flake it exists to prevent. fbuild-core (290) and fbuild-toolchain (149) both pass on Linux, and clippy `-D warnings` is clean there. Worth noting the container caught something a Windows host could not: `SpawnFailed`'s payload is only constructed inside a Linux-gated branch, so its unused-field warning simply does not exist on Windows. Co-Authored-By: Claude Opus 5 (1M context) --- crates/fbuild-core/src/subprocess.rs | 173 ++++++++++++++++++ .../src/toolchain/esp_qemu_runtime.rs | 51 +++++- 2 files changed, 219 insertions(+), 5 deletions(-) diff --git a/crates/fbuild-core/src/subprocess.rs b/crates/fbuild-core/src/subprocess.rs index 37f0c643..a145e418 100644 --- a/crates/fbuild-core/src/subprocess.rs +++ b/crates/fbuild-core/src/subprocess.rs @@ -411,6 +411,78 @@ pub fn run_command_blocking( block_on(run_command(args, cwd, env, timeout)) } +/// How many times [`run_command_blocking_retrying_exec_busy`] will try to +/// spawn before giving up. +/// +/// Three is enough for the window this exists to cross. The race is another +/// thread holding a writable descriptor across a `fork`, which lasts as long +/// as it takes that child to reach `exec` — microseconds, not milliseconds. +const EXEC_BUSY_ATTEMPTS: u32 = 3; + +/// Base backoff between spawn attempts; multiplied by the attempt number. +const EXEC_BUSY_BACKOFF: Duration = Duration::from_millis(25); + +/// Whether an OS spawn error is "the file is open for writing somewhere". +/// +/// `ETXTBSY` on Unix. The kernel refuses to `exec` a file while any process +/// holds a writable descriptor for it — and "any process" includes a `fork`ed +/// child of this one that has not reached `exec` yet, which is how the race +/// happens without anyone writing to the file at all. +pub fn is_exec_busy(error: &std::io::Error) -> bool { + error.kind() == std::io::ErrorKind::ExecutableFileBusy +} + +/// [`run_command_blocking`], but retrying a spawn that fails with `ETXTBSY`. +/// +/// Opt-in rather than the default for every subprocess in fbuild, because +/// retrying an exec is a behavior change and only some callers have the +/// write-then-exec shape that provokes it: a file this process just created +/// or extracted and is now running. +/// +/// Only `ETXTBSY` is retried. Every other spawn error is returned on the +/// first attempt — a missing binary should fail immediately, not three times +/// slowly. +/// +/// FastLED/fbuild#1366. +pub fn run_command_blocking_retrying_exec_busy( + args: &[&str], + cwd: Option<&Path>, + env: Option<&[(&str, &str)]>, + timeout: Option, +) -> Result { + block_on(run_command_retrying_exec_busy(args, cwd, env, timeout)) +} + +/// Async form of [`run_command_blocking_retrying_exec_busy`]. +pub async fn run_command_retrying_exec_busy( + args: &[&str], + cwd: Option<&Path>, + env: Option<&[(&str, &str)]>, + timeout: Option, +) -> Result { + if args.is_empty() { + return Err(FbuildError::Other("empty command".to_string())); + } + let timeout = resolve_default_timeout(timeout); + let mut attempt = 1; + loop { + let mut cmd = build_command( + args, cwd, env, /*capture=*/ true, /*stdin_piped=*/ false, + )?; + match process::spawn_tokio_contained(&mut cmd) { + Ok(child) => return wait_and_capture(child, args, timeout).await, + Err(error) if is_exec_busy(&error) && attempt < EXEC_BUSY_ATTEMPTS => { + // `tokio::time::sleep`, not `std::thread::sleep`: this runs on + // a tokio worker and blocking it would stall every other task + // on that thread (FastLED/fbuild#844). + tokio::time::sleep(EXEC_BUSY_BACKOFF * attempt).await; + attempt += 1; + } + Err(error) => return Err(spawn_err(args, error)), + } + } +} + /// Blocking variant of [`run_command_with_stdin`]. pub fn run_command_with_stdin_blocking( args: &[&str], @@ -639,6 +711,107 @@ fn compute_env(program: &str, overlay: Option<&[(&str, &str)]>) -> Option String { + use std::io::Write; + use std::os::unix::fs::PermissionsExt; + let script = dir.join(name); + { + let mut file = std::fs::File::create(&script).expect("create"); + file.write_all( + b"#!/bin/sh +exit 0 +", + ) + .expect("write"); + } + std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755)).expect("chmod"); + script.to_string_lossy().into_owned() + } + + /// A held write handle blocks `exec`, and no amount of retrying helps. + /// + /// This is the mechanism behind FastLED/fbuild#1366 made reproducible. + /// `exec` fails with `ETXTBSY` while *any* process holds the file open for + /// writing — including this one — so the flake, which in CI came from a + /// sibling thread's `fork` inheriting the descriptor, needs no thread + /// timing to reproduce. Fully deterministic: the handle lives longer than + /// the whole retry budget. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn a_held_write_handle_blocks_exec_for_the_whole_retry_budget() { + let tmp = tempfile::TempDir::new().expect("tempdir"); + let path = write_runnable_script(tmp.path(), "held_open.sh"); + + let held = std::fs::OpenOptions::new() + .write(true) + .open(&path) + .expect("reopen for write"); + + let result = run_command_retrying_exec_busy(&[path.as_str()], None, None, None).await; + assert!( + result.is_err(), + "exec must fail while a writable handle is open, got {result:?}" + ); + drop(held); + } + + /// The retry earns its place: a handle released mid-window lets a later + /// attempt through, where a single attempt would have failed. + /// + /// This is the property that actually fixes #1366 — the CI race window is + /// the microseconds between another thread's `fork` and its `exec`, so a + /// second attempt is all it takes. The 10 ms hold is deliberately tiny + /// against the ~75 ms retry budget: a loaded runner can delay the release + /// by several times over and the test still passes, so this cannot become + /// the flake it exists to prevent. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn a_handle_released_mid_window_lets_the_retry_through() { + let tmp = tempfile::TempDir::new().expect("tempdir"); + let path = write_runnable_script(tmp.path(), "released.sh"); + + let held = std::fs::OpenOptions::new() + .write(true) + .open(&path) + .expect("reopen for write"); + // A tokio task rather than a thread with `std::thread::sleep`, which + // the workspace bans for blocking a runtime worker (#844). + tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(10)).await; + drop(held); + }); + + let result = run_command_retrying_exec_busy(&[path.as_str()], None, None, None).await; + assert!( + result.is_ok(), + "a retry must outlast a transient writable handle, got {result:?}" + ); + } + #[tokio::test] async fn run_echo() { let args = if crate::platform::host::is_windows() { diff --git a/crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs b/crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs index 90cb54fe..861012f1 100644 --- a/crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs +++ b/crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs @@ -228,9 +228,20 @@ enum QemuProbe { /// The dynamic linker could not satisfy a dependency (exit code 127). /// Carries the linker's own line when it could be recovered. MissingSharedLibrary(String), - /// Probe could not be interpreted (spawn failure, or some other - /// non-127 exit). Treated as non-fatal: the real run reports it with - /// full context. + /// The binary could not be executed at all — the spawn itself failed. + /// + /// Distinct from [`QemuProbe::Inconclusive`] because the two say + /// completely different things: "started and could not find a .so" is + /// about the runtime bundle, while "never started" is about the file or + /// the OS. Collapsing them made a failed `exec` report itself as a + /// missing shared library, which sent readers looking at QEMU's + /// dependencies for a problem that was never there (FastLED/fbuild#1366). + /// + /// Treated as non-fatal exactly like `Inconclusive`: the real run reports + /// it with full context. + SpawnFailed(String), + /// Probe could not be interpreted (some other non-127 exit). Treated as + /// non-fatal: the real run reports it with full context. Inconclusive, } @@ -261,7 +272,12 @@ fn probe_qemu_binary(qemu_binary: &Path, lib_dir: Option<&Path>) -> QemuProbe { .as_deref() .map(|value| vec![("LD_LIBRARY_PATH", value)]); - let probe_result = fbuild_core::subprocess::run_command_blocking( + // Retrying variant: fbuild probes binaries it has just downloaded and + // extracted, and `exec` on a file any process still holds open for + // writing fails with `ETXTBSY` — including a `fork`ed child of this + // process that has not reached its own `exec` yet. See + // FastLED/fbuild#1366. + let probe_result = fbuild_core::subprocess::run_command_blocking_retrying_exec_busy( &[&qemu_binary.to_string_lossy(), "--version"], None, // cwd env.as_deref(), @@ -279,7 +295,8 @@ fn probe_qemu_binary(qemu_binary: &Path, lib_dir: Option<&Path>) -> QemuProbe { .unwrap_or_else(|| out.stderr.trim().to_string()); QemuProbe::MissingSharedLibrary(detail) } - Ok(_) | Err(_) => QemuProbe::Inconclusive, + Err(error) => QemuProbe::SpawnFailed(error.to_string()), + Ok(_) => QemuProbe::Inconclusive, } } } @@ -298,6 +315,17 @@ fn probe_qemu_binary(qemu_binary: &Path, lib_dir: Option<&Path>) -> QemuProbe { pub(crate) async fn ensure_qemu_can_start(qemu_binary: &Path, project_dir: &Path) -> Result<()> { let missing = match probe_qemu_binary(qemu_binary, None) { QemuProbe::Started | QemuProbe::Inconclusive => return Ok(()), + QemuProbe::SpawnFailed(detail) => { + // Not a runtime-library problem, so downloading the bundle would + // fix nothing. Say what actually happened and let the real run + // report it with full context. + tracing::warn!( + "QEMU probe at {} could not be executed: {}", + qemu_binary.display(), + detail + ); + return Ok(()); + } QemuProbe::MissingSharedLibrary(detail) => detail, }; @@ -316,6 +344,19 @@ pub(crate) async fn ensure_qemu_can_start(qemu_binary: &Path, project_dir: &Path match probe_qemu_binary(qemu_binary, Some(lib_dir.as_path())) { QemuProbe::Started | QemuProbe::Inconclusive => Ok(()), + // Non-fatal for the same reason `Inconclusive` is: the probe learned + // nothing about the runtime bundle. Logged rather than swallowed, + // because "could not execute the binary at all" is a different problem + // from "started and could not find a .so", and the next thing to fail + // will be the real run (FastLED/fbuild#1366). + QemuProbe::SpawnFailed(detail) => { + tracing::warn!( + "QEMU probe at {} could not be executed even with the runtime bundle applied: {}", + qemu_binary.display(), + detail + ); + Ok(()) + } QemuProbe::MissingSharedLibrary(still_missing) => Err(FbuildError::PackageError(format!( "QEMU at {} cannot start even with the fbuild runtime bundle at {} applied.\n\ {}\n\ From 25cc4abcbdbe59995d9fc051caa90c56867a5d2d Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 21:57:50 -0700 Subject: [PATCH 2/5] fix(ci): gate the ETXTBSY tests to Linux and route through the platform facade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three CI failures on the first run of #1368, all from the same two mistakes: - **`Check (macos-latest)`** — `a_held_write_handle_blocks_exec_for_the_whole_retry_budget` failed there. Darwin lets a plain open-for-write coexist with `execve`; only Linux enforces `ETXTBSY`. Gating on `unix` was wrong — macOS is unix and the test could never pass on it. Now gated on the host actually being Linux, through a runtime check that returns early, matching how the QEMU probe test next door already gates itself. - **`Dylint` and `Inventory (linux/macos/windows)`** — the platform-boundary ledger (#1306) flagged three `#[cfg(unix)]` attributes and one `std::os::unix` permissions import as new raw host mechanics in `fbuild-core`. Both are gone: the runtime Linux check replaces the `cfg` attributes, and the fixture now sets the executable bit through `platform::fs::set_executable`, which is the neutral facade that exists for exactly this. No ledger entry needed — the right fix for "you added raw platform mechanics" is to stop adding them. Verified on both hosts: the tests exercise the real behavior on Linux (23 passed in the container) and return early on Windows. `ci/enforce_platform_boundary.py` and `ci/platform_boundary_research.py` both pass locally. Refs FastLED/fbuild#1366 Co-Authored-By: Claude Opus 5 (1M context) --- crates/fbuild-core/src/subprocess.rs | 49 ++++++++++++++++++---------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/crates/fbuild-core/src/subprocess.rs b/crates/fbuild-core/src/subprocess.rs index a145e418..988720aa 100644 --- a/crates/fbuild-core/src/subprocess.rs +++ b/crates/fbuild-core/src/subprocess.rs @@ -733,38 +733,51 @@ mod tests { } } + /// Whether this host enforces `ETXTBSY` on `exec`. + /// + /// Linux only, and that is not pedantry: Darwin lets a plain + /// open-for-write coexist with `execve`, so the same code that reliably + /// fails on Linux succeeds on macOS. Gating on `unix` would have made + /// these tests fail there for a reason that is not a bug. + fn host_enforces_exec_busy() -> bool { + crate::platform::host::current().os() == crate::platform::host::HostOs::Linux + } + /// Build an executable no-op script and return its path. - #[cfg(unix)] - fn write_runnable_script(dir: &std::path::Path, name: &str) -> String { + /// + /// Uses the neutral `platform::fs` facade rather than a per-OS + /// permissions extension trait, so this file stays free of raw host + /// mechanics (the platform-boundary ledger, #1306). + fn write_runnable_script(dir: &std::path::Path, name: &str) -> std::io::Result { use std::io::Write; - use std::os::unix::fs::PermissionsExt; let script = dir.join(name); { - let mut file = std::fs::File::create(&script).expect("create"); + let mut file = std::fs::File::create(&script)?; file.write_all( b"#!/bin/sh exit 0 ", - ) - .expect("write"); + )?; } - std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755)).expect("chmod"); - script.to_string_lossy().into_owned() + crate::platform::fs::set_executable(&script)?; + Ok(script.to_string_lossy().into_owned()) } /// A held write handle blocks `exec`, and no amount of retrying helps. /// /// This is the mechanism behind FastLED/fbuild#1366 made reproducible. - /// `exec` fails with `ETXTBSY` while *any* process holds the file open for + /// Linux refuses to `exec` a file while *any* process holds it open for /// writing — including this one — so the flake, which in CI came from a /// sibling thread's `fork` inheriting the descriptor, needs no thread - /// timing to reproduce. Fully deterministic: the handle lives longer than - /// the whole retry budget. - #[cfg(unix)] + /// timing to reproduce. Fully deterministic: the handle outlives the whole + /// retry budget. #[tokio::test(flavor = "multi_thread")] async fn a_held_write_handle_blocks_exec_for_the_whole_retry_budget() { + if !host_enforces_exec_busy() { + return; + } let tmp = tempfile::TempDir::new().expect("tempdir"); - let path = write_runnable_script(tmp.path(), "held_open.sh"); + let path = write_runnable_script(tmp.path(), "held_open.sh").expect("script"); let held = std::fs::OpenOptions::new() .write(true) @@ -786,13 +799,15 @@ exit 0 /// the microseconds between another thread's `fork` and its `exec`, so a /// second attempt is all it takes. The 10 ms hold is deliberately tiny /// against the ~75 ms retry budget: a loaded runner can delay the release - /// by several times over and the test still passes, so this cannot become - /// the flake it exists to prevent. - #[cfg(unix)] + /// several times over and this still passes, so it cannot become the flake + /// it exists to prevent. #[tokio::test(flavor = "multi_thread")] async fn a_handle_released_mid_window_lets_the_retry_through() { + if !host_enforces_exec_busy() { + return; + } let tmp = tempfile::TempDir::new().expect("tempdir"); - let path = write_runnable_script(tmp.path(), "released.sh"); + let path = write_runnable_script(tmp.path(), "released.sh").expect("script"); let held = std::fs::OpenOptions::new() .write(true) From 58695bca279b021e18041db388909785de7ff1da Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 22:10:25 -0700 Subject: [PATCH 3/5] chore(ci): refresh the platform-boundary inventory for shifted line numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ledger records file/line for every host-mechanic occurrence, so adding the `QemuProbe::SpawnFailed` variant moved two existing `esp_qemu_runtime.rs` entries down by 11 lines and the committed TSV no longer matched. Regenerated with `--write`. The diff is two line numbers — no occurrence added, removed, or reclassified. Refs FastLED/fbuild#1366 Co-Authored-By: Claude Opus 5 (1M context) --- ci/platform_boundary_research.tsv | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/platform_boundary_research.tsv b/ci/platform_boundary_research.tsv index 735e488d..6d44c122 100644 --- a/ci/platform_boundary_research.tsv +++ b/ci/platform_boundary_research.tsv @@ -75,6 +75,6 @@ crates/fbuild-toolchain/src/toolchain/esp_qemu.rs 570 attr_cfg #[cfg(not(windows crates/fbuild-toolchain/src/toolchain/esp_qemu.rs 728 attr_cfg #[cfg(windows)] host_executable host_mechanic crates/fbuild-toolchain/src/toolchain/esp_qemu.rs 755 attr_cfg #[cfg(windows)] host_executable host_mechanic crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 224 attr_cfg #[cfg_attr(not(target_os=),allow(dead_code))] host_executable host_artifact_policy -crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 246 attr_cfg #[cfg(not(target_os=))] host_executable host_artifact_policy -crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 252 attr_cfg #[cfg(target_os=)] host_executable host_artifact_policy +crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 257 attr_cfg #[cfg(not(target_os=))] host_executable host_artifact_policy +crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 263 attr_cfg #[cfg(target_os=)] host_executable host_artifact_policy crates/fbuild-toolchain/tests/qemu_linux_runtime.rs 18 attr_cfg #![cfg(target_os=)] host_executable host_artifact_policy From d63536c6785361d8fb20c557b119d974f5b55083 Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 22:47:11 -0700 Subject: [PATCH 4/5] fix(qemu): name the probe variant for what it can actually claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit was right that `SpawnFailed` over-claimed: `run_command_retrying_exec_busy` returns `Err` for post-spawn failures too — a timeout, a capture error — so labelling every one of them a *spawn* failure mislabels them. That is the same over-claiming this PR exists to remove, one level up. Renamed to `ProbeFailed`: "the probe produced no exit code to interpret". True for both cases, and still distinct from `Inconclusive`, which did get an exit code that simply means nothing. Deliberately not split into spawn-vs-timeout variants, as the review suggested. The error crossing that boundary is already flattened to a string by `subprocess::spawn_err`, so separating them here would mean matching on message text — and inventing a distinction the code cannot actually make is exactly how the original bug happened. The carried string is the underlying error and says which it was. Inventory refreshed for the shifted line numbers. Refs FastLED/fbuild#1366 Co-Authored-By: Claude Opus 5 (1M context) --- ci/platform_boundary_research.tsv | 4 +-- .../src/toolchain/esp_qemu_runtime.rs | 35 ++++++++++++------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/ci/platform_boundary_research.tsv b/ci/platform_boundary_research.tsv index 6d44c122..37b4383e 100644 --- a/ci/platform_boundary_research.tsv +++ b/ci/platform_boundary_research.tsv @@ -75,6 +75,6 @@ crates/fbuild-toolchain/src/toolchain/esp_qemu.rs 570 attr_cfg #[cfg(not(windows crates/fbuild-toolchain/src/toolchain/esp_qemu.rs 728 attr_cfg #[cfg(windows)] host_executable host_mechanic crates/fbuild-toolchain/src/toolchain/esp_qemu.rs 755 attr_cfg #[cfg(windows)] host_executable host_mechanic crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 224 attr_cfg #[cfg_attr(not(target_os=),allow(dead_code))] host_executable host_artifact_policy -crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 257 attr_cfg #[cfg(not(target_os=))] host_executable host_artifact_policy -crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 263 attr_cfg #[cfg(target_os=)] host_executable host_artifact_policy +crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 266 attr_cfg #[cfg(not(target_os=))] host_executable host_artifact_policy +crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs 272 attr_cfg #[cfg(target_os=)] host_executable host_artifact_policy crates/fbuild-toolchain/tests/qemu_linux_runtime.rs 18 attr_cfg #![cfg(target_os=)] host_executable host_artifact_policy diff --git a/crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs b/crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs index 861012f1..846fbb51 100644 --- a/crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs +++ b/crates/fbuild-toolchain/src/toolchain/esp_qemu_runtime.rs @@ -228,18 +228,27 @@ enum QemuProbe { /// The dynamic linker could not satisfy a dependency (exit code 127). /// Carries the linker's own line when it could be recovered. MissingSharedLibrary(String), - /// The binary could not be executed at all — the spawn itself failed. + /// The probe produced no exit code to interpret: the binary could not be + /// executed, or the run failed or timed out before one was available. /// - /// Distinct from [`QemuProbe::Inconclusive`] because the two say - /// completely different things: "started and could not find a .so" is - /// about the runtime bundle, while "never started" is about the file or - /// the OS. Collapsing them made a failed `exec` report itself as a - /// missing shared library, which sent readers looking at QEMU's - /// dependencies for a problem that was never there (FastLED/fbuild#1366). + /// Distinct from [`QemuProbe::Inconclusive`] (which *did* get an exit + /// code, just not one that means anything) because the two say different + /// things about the runtime bundle. Collapsing "never produced a result" + /// into the bundle's own failure mode is what made a failed `exec` report + /// itself as a missing shared library, sending readers after QEMU + /// dependencies for a problem that was never there + /// (FastLED/fbuild#1366). + /// + /// Deliberately *not* split further into spawn-vs-timeout. The error + /// crossing this boundary is already flattened to a string by + /// `subprocess::spawn_err`, so telling them apart here would mean matching + /// on message text — and inventing a distinction the code cannot actually + /// make is precisely the bug above. The carried string is the underlying + /// error, which says which it was. /// /// Treated as non-fatal exactly like `Inconclusive`: the real run reports /// it with full context. - SpawnFailed(String), + ProbeFailed(String), /// Probe could not be interpreted (some other non-127 exit). Treated as /// non-fatal: the real run reports it with full context. Inconclusive, @@ -295,7 +304,7 @@ fn probe_qemu_binary(qemu_binary: &Path, lib_dir: Option<&Path>) -> QemuProbe { .unwrap_or_else(|| out.stderr.trim().to_string()); QemuProbe::MissingSharedLibrary(detail) } - Err(error) => QemuProbe::SpawnFailed(error.to_string()), + Err(error) => QemuProbe::ProbeFailed(error.to_string()), Ok(_) => QemuProbe::Inconclusive, } } @@ -315,12 +324,12 @@ fn probe_qemu_binary(qemu_binary: &Path, lib_dir: Option<&Path>) -> QemuProbe { pub(crate) async fn ensure_qemu_can_start(qemu_binary: &Path, project_dir: &Path) -> Result<()> { let missing = match probe_qemu_binary(qemu_binary, None) { QemuProbe::Started | QemuProbe::Inconclusive => return Ok(()), - QemuProbe::SpawnFailed(detail) => { + QemuProbe::ProbeFailed(detail) => { // Not a runtime-library problem, so downloading the bundle would // fix nothing. Say what actually happened and let the real run // report it with full context. tracing::warn!( - "QEMU probe at {} could not be executed: {}", + "QEMU probe at {} produced no usable result: {}", qemu_binary.display(), detail ); @@ -349,9 +358,9 @@ pub(crate) async fn ensure_qemu_can_start(qemu_binary: &Path, project_dir: &Path // because "could not execute the binary at all" is a different problem // from "started and could not find a .so", and the next thing to fail // will be the real run (FastLED/fbuild#1366). - QemuProbe::SpawnFailed(detail) => { + QemuProbe::ProbeFailed(detail) => { tracing::warn!( - "QEMU probe at {} could not be executed even with the runtime bundle applied: {}", + "QEMU probe at {} produced no usable result even with the runtime bundle applied: {}", qemu_binary.display(), detail ); From fb4c5b0100ec1a28a0053d3b0047ab3b653e3b3e Mon Sep 17 00:00:00 2001 From: zackees Date: Sat, 22 Aug 2026 23:41:32 -0700 Subject: [PATCH 5/5] test(subprocess): arm the releaser before the probe starts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit, on the test written to avoid flakiness: `tokio::spawn` is detached, so its 10 ms timer only arms whenever the runtime first polls it. On a loaded runner that could be after the whole ~75 ms retry budget had elapsed, holding the handle through all three attempts — making this exactly the flake it exists to prevent. The releaser now signals on a oneshot before sleeping, and the probe does not start until that signal arrives, so the timer is always armed first. The test also joins the releaser rather than leaking it. Verified on Linux: both tests still pass. Refs FastLED/fbuild#1366 Co-Authored-By: Claude Opus 5 (1M context) --- crates/fbuild-core/src/subprocess.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/crates/fbuild-core/src/subprocess.rs b/crates/fbuild-core/src/subprocess.rs index 988720aa..06f3fd6b 100644 --- a/crates/fbuild-core/src/subprocess.rs +++ b/crates/fbuild-core/src/subprocess.rs @@ -813,14 +813,26 @@ exit 0 .write(true) .open(&path) .expect("reopen for write"); + + // The releaser signals *before* it sleeps, and the probe does not + // start until that signal arrives. Spawning detached and hoping would + // leave the task's timer unarmed until an arbitrary later point — on a + // loaded runner it could start after the retry budget had already + // elapsed, holding the handle through all three attempts. That would + // make this the flake it exists to prevent. + // // A tokio task rather than a thread with `std::thread::sleep`, which // the workspace bans for blocking a runtime worker (#844). - tokio::spawn(async move { + let (armed_tx, armed_rx) = tokio::sync::oneshot::channel(); + let releaser = tokio::spawn(async move { + let _ = armed_tx.send(()); tokio::time::sleep(Duration::from_millis(10)).await; drop(held); }); + armed_rx.await.expect("releaser task must start"); let result = run_command_retrying_exec_busy(&[path.as_str()], None, None, None).await; + releaser.await.expect("releaser task must finish"); assert!( result.is_ok(), "a retry must outlast a transient writable handle, got {result:?}"