audit: sync code that could be async in fbuild-build (sub-issue of #813) #820

Intro

This audits crates/fbuild-build/ for synchronous code that could run on the daemon's tokio runtime instead of std::thread / synchronous-subprocess pools. fbuild-build is the highest-leverage crate for the meta migration in #813 because:

• It hosts the per-compile hot path (compile_source → FbuildZccacheService::compile_blocking → runtime.block_on(svc.compile(req))). That block_on is the most costly anti-pattern in the daemon today: every TU compile blocks one tokio worker for the full gcc duration plus the embedded ZccacheService::compile round-trip, even though the embedded service is already async-native on the same runtime.
• It owns all parallel-compile fan-out (parallel.rs + compile_many.rs stage-2 workers) on std::thread::scope, which tokio-console cannot see.
• It owns the entire per-platform BuildOrchestrator surface — 14 sync orchestrators. Whether the trait method becomes async fn is the single biggest design question for audit: go fully async — whole-app tokio runtime sharing for tokio-console (meta) #813's end-state.

The good news: fbuild-build never reaches for raw std::process::Command or std::thread::spawn directly — both are routed through wrappers (fbuild_core::subprocess::run_command, std::thread::scope) that are easy to replace centrally. The cascade is bounded; converting subprocess::run_command to async plus an async fn build on BuildOrchestrator would mechanically cascade through every finding below.

Findings

The BuildOrchestrator trait question

Should BuildOrchestrator::build become async fn build(...)?

Pros (in order of leverage)

1. One runtime end-to-end. Today fbuild-daemon calls orchestrator.build(&params) from inside an axum HTTP handler. That handler is on the tokio runtime; the call into build is sync and blocks the worker for the entire compile (seconds to minutes). Making build async lets the worker yield naturally at every .await point — the dominant pattern in audit: go fully async — whole-app tokio runtime sharing for tokio-console (meta) #813's motivation.
2. Tokio-console visibility for the whole build pipeline. Every tokio::spawn inside an async build (parallel compiles, linker, esptool) shows up in the console. Today the console sees the HTTP request task and then a silent block.
3. Structured cancellation. The daemon's tokio::sync::watch shutdown signal can tokio::select! against the build future and abort all in-flight subprocesses transitively. Sync build can only check a flag between phases.
4. Per-request deadlines compose (audit: blocking operations with no timeout (meta) #802). The partner audit at audit: blocking operations with no timeout (meta) #802 wants tokio::time::timeout(...) around build calls. That only works on an async future.
5. Mechanical cascade. Once compile_source and subprocess::run_command are async, every per-platform build impl is mostly .await insertions — no logic changes.

Cons (and mitigations)

1. 14 orchestrator impls to rewrite. Apollo3, AVR, CH32V, ESP32, ESP8266, NRF52, NXP LPC, RP2040, SAM, Silabs, STM32, Teensy, Renesas, plus the synthetic generic_arm path. Each is hundreds of lines of orchestration. Mitigation: the actual changes are mechanical — async fn build + .await on every compile_* / link_* / subprocess::* call. No business-logic changes. Land it as one PR per orchestrator behind a feature gate.
2. async fn in traits — dyn support. Rust 1.94.1 supports native async fn in traits, but Box<dyn BuildOrchestrator> doesn't compose cleanly with native AFITs (the returned future isn't Send-bounded by default; you'd need return_type_notation or a typed-future return). Mitigation: use #[async_trait] (the async-trait crate). It already works with &dyn / Box<dyn>. The runtime overhead (boxed future per call) is trivial vs the build time itself.
3. PlatformSupport::install_deps and Compiler::compile_one need the same treatment. Both are sync traits called from inside build. Mitigation: same async_trait, same mechanical cascade.
4. compile_many orchestration code lives in fbuild-build and calls get_orchestrator(platform).build(...) from a sync thread-pool worker today. Mitigation: compile_many itself becomes async (see CRITICAL row for run_stage2). The stage-2 fan-out becomes FuturesUnordered<async_compute> instead of thread::scope.

Recommendation

Convert BuildOrchestrator::build to async fn via #[async_trait]. The mechanical cascade is bounded, the wins are load-bearing for #813's end-state, and the only "real" design question — async_trait vs native AFITs — has an obvious near-term answer (async_trait while dyn support for native AFITs is still rough). Revisit native AFITs in a follow-up once return_type_notation stabilises and Rust's dyn AsyncTrait ergonomics catch up.

What was searched

Patterns (via Grep on crates/fbuild-build/src/):

• std::thread::spawn|thread::spawn — 0 hits (all parallelism goes through std::thread::scope).
• JoinHandle|\.join\(\) — 2 hits, both thread::scope joins in parallel.rs + compile_many.rs.
• std::thread::scope|scope.spawn — 2 sites: parallel.rs:80-143, compile_many.rs:665-736.
• rayon|par_iter|ThreadPool — 0 direct uses (rayon shows up only in upstream zccache::fingerprint, which is documented as intentionally sync-on-CPU-pool in zccache_embedded.rs:274).
• Command::new|process::Command — 0 direct uses; all subprocess routing goes through fbuild_core::subprocess::run_command(_*).
• subprocess::run_command (the wrapper) — 28 call sites across compiler.rs, pipeline/compile.rs, linker.rs, every *_linker.rs, esp32/orchestrator/embed.rs, esp32/orchestrator/boot_artifacts.rs, stm32/orchestrator/arduino_mbed.rs, script_runtime.rs, symbol_analyzer/mod.rs, shrink/probe.rs.
• Mutex::new|RwLock::new — 7 hits. None held across .await today (no .await in the crate). The 3 that would matter post-conversion are listed under MEDIUM findings.
• std::sync::mpsc|crossbeam — build_output.rs exposes std::sync::mpsc::Sender<String> on BuildParams.log_sender. Cross-crate API; flagged for the partner audit.
• std::fs:: — 329 occurrences across 50 files. The ones on the per-compile / per-build hot path are listed (compile_many seed/copy, framework_core_cache hydrate/store, pipeline compile_local_libraries, compiler.rs output writes).
• block_on — 1 real call site: zccache_embedded.rs:216 inside compile_blocking. (Plus four docstring references and one in compile_backend.rs documenting the runtime handle.)
• impl BuildOrchestrator for|fn build\( — confirmed 14 sync fn build impls across the platform tree, plus the trait itself in lib.rs:201-204.

Files specifically read end-to-end: compile_many.rs, parallel.rs, compiler.rs (compile_source + trait), pipeline/mod.rs, pipeline/compile.rs, pipeline/sequential.rs, zccache_embedded.rs, compile_backend.rs, linker.rs (subprocess sites), avr/avr_linker.rs (representative per-platform linker), esp32/orchestrator/build.rs (representative orchestrator top-level), esp32/orchestrator/embed.rs, build_output.rs, framework_core_cache.rs.

Out of scope (deliberately left alone)

• zccache::fingerprint's rayon parallelism — documented in zccache_embedded.rs:274 as intentionally sync-on-CPU-pool. Genuinely CPU-bound (BLAKE3 + path walks). Keep on rayon; only wrap callers in spawn_blocking when they're invoked from the runtime.
• std::thread::available_parallelism() in parallel.rs:17, compile_many.rs:71/83, stage2_jobs_per_worker — pure computation, no I/O.
• Test-only code — every #[cfg(test)] block in parallel.rs, compile_many.rs, framework_core_cache.rs, compiler_tests.rs uses std::thread::sleep / std::fs::write legitimately and is not on the production runtime.
• Drop impls — none in fbuild-build reach for sync I/O on a hot path.
• One-shot startup config reads — e.g. arduino_props.rs parsing platform.txt at orchestrator construction; fine to keep sync.
• std::sync::Mutex<BuildLog> in pipeline/sequential.rs:116 and esp32/orchestrator/build.rs:595 — flagged MEDIUM, but the lock is only held for sync log.push / collect_warnings. Per tokio's "do not block the runtime" guidance, sync Mutex is legal in async code as long as the critical section doesn't .await. Document the contract rather than swap the type.
• COMPILER_IDENTITY_CACHE (compiler.rs:63) — same as above; cache lookup never crosses .await.

Sub-issue of #813.