audit: blocking operations with no timeout in fbuild-daemon (sub-issue of #802)

[zackees]

Intro

This issue is the fbuild-daemon sub-audit of meta issue #802. I walked the entire crates/fbuild-daemon/src/ tree (broker, handlers, device_manager, status_manager, context, main) looking for blocking operations whose runtime is not bounded by tokio::time::timeout(...) or some other wall-clock cap. The daemon is the single long-lived process every fbuild operation routes through, so unbounded ops here are uniquely dangerous: one stuck handler stalls the daemon's tokio workers, which stalls every other in-flight HTTP/WS client. I deliberately excluded #[cfg(test)] blocks, Drop impls, and tokio::sync::watch::Receiver::changed().await used as shutdown signals.

Findings

Severity	File:line	Operation	Why unbounded	Suggested fix
CRITICAL	crates/fbuild-daemon/src/handlers/operations/monitor.rs:227	run_monitor_loop inner tokio::time::timeout(recv_timeout, rx.recv()) loops with // No overall timeout: just keep waiting when caller passes req.timeout=None	The recv-timeout is just the iteration tick; the outer loop has no wall-clock cap. A monitor request with no timeout field (the common CLI case) hangs the handler until the WS client gives up or the daemon is killed.	Require an explicit max-monitor-duration ceiling (e.g. clamp req.timeout to Some(MAX_MONITOR_SECS) on the handler entry, default 300 s), or have run_monitor_loop accept a separate hard_deadline: Option<Instant> argument that the handler always sets even when the user didn't.
CRITICAL	crates/fbuild-daemon/src/handlers/operations/monitor.rs:244	ctx.serial_manager.open_port(...).await inside the POST /api/monitor handler entry	No tokio::time::timeout(...) wrapper. USB re-enumeration on Windows can stall an open_port for tens of seconds (the daemon code elsewhere acknowledges this — see PendingAttachGuard comments), and a serial driver bug can stall it forever. The HTTP response is delayed for the full hang.	Wrap with tokio::time::timeout(Duration::from_secs(30), ctx.serial_manager.open_port(...)) and return 504 Gateway Timeout (or a structured OperationResponse::fail) on Err(_). Same fix applies anywhere else open_port().await is reached from a handler.
CRITICAL	crates/fbuild-daemon/src/handlers/operations/build.rs:262 (streaming path) and crates/fbuild-daemon/src/handlers/operations/build.rs:497 (non-streaming path)	lock.lock().await for the per-project Mutex	The streaming path warns after 10 s but its loop { tokio::time::timeout(LOCK_WAIT_WARN, &mut acquire) } re-loops on Err(_) instead of giving up. The non-streaming lock.lock().await is a bare unbounded await. A wedged previous build (which itself can hang on an esptool/avrdude that never returns — see below) keeps the project lock forever and every subsequent build for that project hangs.	Add a hard upper bound (e.g. 30 min) in addition to the warn threshold. After the cap, fail the request with OperationResponse::fail("project lock not acquired within Ns; previous build may be wedged; see fbuild daemon locks") so the client gets a deterministic answer. Same fix in install_deps.rs:44, deploy.rs:197, emulator/select.rs:249.
CRITICAL	crates/fbuild-daemon/src/handlers/operations/deploy.rs:418	tokio::task::spawn_blocking(move || { ...esptool/avrdude/picotool... }).await for the full deploy pipeline	The .await on the join handle is not wrapped in tokio::time::timeout(...). esptool / avrdude / picotool subprocess invocations are themselves blocking calls inside the closure with no upper-bound; an ESP32 stuck in download mode with a flaky USB CDC line or an avrdude hanging on a non-responsive bootloader will lock the deploy handler for the full subprocess timeout chain (often minutes), and a kernel-level stall is forever.	Wrap the .await with a per-platform cap (e.g. 5 min for ESP, 90 s for AVR), and after timeout call clear_preemption + spawn a cleanup task; return a structured failure to the client. The post-deploy recovery spawn_blocking at deploy.rs:772 needs the same treatment.
CRITICAL	crates/fbuild-daemon/src/handlers/operations/deploy.rs:237 and crates/fbuild-daemon/src/handlers/emulator/select.rs build path	Inline build path tokio::task::spawn_blocking(|| orchestrator.build(...)).await with no timeout	The streaming build path properly polls STREAM_STATUS_INTERVAL (build.rs:332) and just continues forever — but the deploy + test-emu build flows use a bare .await with no outer cap. A wedged C compiler (e.g. process inheriting a stuck OS lock) hangs the handler indefinitely.	Either (a) reuse the streaming build path's tokio::time::timeout(STREAM_STATUS_INTERVAL, &mut task) pattern with a hard max-build-duration cap, or (b) at minimum wrap the bare .await with a tokio::time::timeout(Duration::from_secs(30 * 60), ...).
CRITICAL	crates/fbuild-daemon/src/handlers/operations/reset.rs:44	tokio::task::spawn_blocking(|| reset_device(...)).await	DTR/RTS toggling can block on a serial port that is wedged at the OS-driver level (Windows USB CDC). Bare .await — no timeout.	tokio::time::timeout(Duration::from_secs(10), ...). Reset is fundamentally a fast operation; 10 s is more than generous.
CRITICAL	crates/fbuild-daemon/src/handlers/websockets.rs:98	socket.recv().await waiting for the WS attach handshake message in handle_serial_ws	A WS client that connects, completes the WS upgrade, then sends nothing keeps this handler suspended until the OS TCP keepalive eventually fires. While the PendingAttachGuard does keep self-eviction from firing, the connection still consumes a tokio task slot indefinitely.	Wrap with tokio::time::timeout(Duration::from_secs(10), socket.recv()); on timeout send an Error frame and close. Same pattern applies to the monitor-session WS at websockets.rs:815.
CRITICAL	crates/fbuild-daemon/src/handlers/emulator/runners.rs:247, crates/fbuild-daemon/src/handlers/emulator/avr8js_npm.rs:14, crates/fbuild-daemon/src/handlers/emulator/avr8js_npm.rs:143	fbuild_core::subprocess::run_command(&["npm"/"node"/"simavr", ...], ...) called synchronously from an async fn	These are blocking subprocess invocations directly on the tokio executor — npm install avr8js@0.21.0 can hang for minutes on registry connectivity issues, and node --version / simavr --help will hang forever if the binary is replaced with something that blocks on stdin. Running synchronously also pegs a tokio worker for the duration.	Move to tokio::process::Command with explicit stdout(piped()).stdin(null()), wait via tokio::time::timeout(Duration::from_secs(10), child.wait_with_output()). For npm install give a longer cap (e.g. 5 min) but still cap. At minimum wrap each call with spawn_blocking and tokio::time::timeout.
CRITICAL	crates/fbuild-daemon/src/context.rs:387 → crates/fbuild-daemon/src/device_manager.rs:223	serialport::available_ports() is a synchronous OS-level call invoked from refresh_devices which is reached from every /api/devices/* HTTP handler (handlers/devices.rs:18, 35, 108, 187) and from deploy.rs:367, 415	Windows USB enumeration is known to be slow (20-30 ms common, can spike into seconds under driver contention) and runs directly on a tokio worker. While not a forever-hang in normal cases, a wedged USB stack can stall a worker indefinitely; the handler also has no timeout wrapper.	Move the sync call into tokio::task::spawn_blocking(...) and wrap with tokio::time::timeout(Duration::from_secs(5), ...). The last_refresh_at cache (already there) already amortizes the cost for back-to-back calls; the change is purely runtime safety.
HIGH	crates/fbuild-daemon/src/broker/backend.rs:89 (read_frame), :97, :113 (write_frame_bytes)	read_frame(&mut stream)? / write_frame_bytes(&mut stream, ...)? on interprocess::local_socket streams in handle_backend_connection	No set_read_timeout / set_write_timeout configured on the local-socket. A peer that opens the broker backend pipe and sends nothing — or sends a partial frame — ties up a thread (via std::thread::spawn at line 69) forever. Multiple slow-loris peers can spawn unbounded threads.	Call stream.set_read_timeout(Some(Duration::from_secs(5)))? and stream.set_write_timeout(Some(Duration::from_secs(5)))? immediately after accept. Return early on timeout. Consider bounding the per-connection thread count too (e.g. via a semaphore).
HIGH	crates/fbuild-daemon/src/broker/session.rs:81 and :111	AsyncBrokerSession::adopt(request).await and self.inner.request(...).await in FbuildBrokerSession::adopt / request	No tokio::time::timeout(...) wrapper. If the broker is unhealthy (responsive enough to accept the connection but never replies on the negotiation frame), adopt hangs the entire daemon startup sequence, and request hangs any HTTP/WS handler that routes through the broker frame path.	Wrap both with tokio::time::timeout(Duration::from_secs(10), ...) and map Err(_) into BrokerError::Connect("broker negotiation timed out") / BrokerError::Request("broker request timed out").
HIGH	crates/fbuild-daemon/src/main.rs:361 background GC loop	let _guard = gc_mutex.lock().await; followed by sync dc.run_gc() directly in the async task	run_gc is called sync on the tokio executor (not spawn_blocking), so a slow filesystem operation pegs a worker. The /api/cache/gc HTTP handler at cache.rs:49 correctly uses spawn_blocking; the background loop should match. The mutex itself is unbounded — but since only this background loop and the gc handler ever take it, it's contention-only, not a deadlock vector.	Wrap dc.run_gc() in tokio::task::spawn_blocking(...), matching handlers/cache.rs:49. The lock acquisition itself is fine because the only contender is the manual handler which is rare.
HIGH	crates/fbuild-daemon/src/handlers/emulator/shared.rs:308 and crates/fbuild-daemon/src/handlers/emulator/avr8js_headless.rs:176	Post-loop child.wait().await after child.kill().await	If the OS kill is processed but the child's exit-status reaper hangs (rare but observed on Windows with certain driver-resident children), child.wait().await is unbounded. The main tokio::select! loop is well-bounded; this cleanup branch is the gap.	Wrap with tokio::time::timeout(Duration::from_secs(5), child.wait()).await. On timeout, log and proceed — the child was already kill()'d and the containment group will reap it on daemon exit.
MEDIUM	crates/fbuild-daemon/src/status_manager.rs:118, 138, 151, 171, 190	write_atomic() performs sync std::fs::write + std::fs::rename directly from StatusManager::update* which is called from handlers via &self.status_manager	Sync I/O on the tokio executor. Won't hang forever in practice but a Windows AV-locked file or a slow disk can stall the writes for hundreds of ms, blocking the worker.	Either (a) move the writes into a dedicated background task fed by a channel, or (b) tokio::task::spawn_blocking each write_atomic call. Option (a) is preferable since status writes are fire-and-forget.
MEDIUM	crates/fbuild-daemon/src/main.rs:580	std::thread::sleep(Duration::from_millis(500)) in the bind-listener retry loop	Sync sleep on the runtime thread during daemon startup. Capped (3 attempts x 500 ms = 1.5 s max), so it can't hang forever — but it does block the runtime from initializing for the full window.	Convert the retry loop to async: use tokio::time::sleep(Duration::from_millis(500)).await and make bind_listener_with_retry async.

What was searched

• Command::new / \.output\(\) / \.wait\(\) / \.wait_with_output\(\)
• tokio::process::Command
• reqwest::* (none in daemon — good)
• \.lock\(\)\.await / \.read\(\)\.await / \.write\(\)\.await (async-Mutex / async-RwLock)
• \.lock\(\)\.unwrap / \.read\(\)\.unwrap / \.write\(\)\.unwrap (sync-Mutex / sync-RwLock — assessed for in-async-fn use)
• recv\(\)\.await / recv_timeout / socket\.recv\(\)
• \.await (broad sweep through handlers + broker)
• TcpStream / TcpListener::bind / axum::serve
• interprocess::local_socket / read_frame / write_frame
• device_manager / serial_manager / broker / status_manager modules read for retry-loop patterns
• thread::sleep / tokio::time::sleep for use inside loops without an outer wall-clock cap
• tokio::task::spawn_blocking (all 6 call sites in this crate)
• serialport::available_ports (sync OS call invocation sites)
• fbuild_core::subprocess::run_command (sync subprocess call sites)

Out-of-scope notes

• tokio::sync::watch::Receiver::changed().await in main.rs:397 (shutdown signal — intentional unbounded wait, as the audit spec excludes).
• The serial_manager.open_port/acquire_writer/attach_reader internals live in fbuild-serial and are covered by sub-issue audit: blocking operations with no timeout in fbuild-serial (sub-issue of #802) #803.
• Deploy-side esptool / avrdude / picotool subprocess invocations themselves live in fbuild-deploy and are covered by sub-issue audit: blocking operations with no timeout in fbuild-deploy (sub-issue of #802) #804. This audit only flags the daemon-side handler that awaits them without a timeout.
• containment_harness.rs is a test fixture binary (sleeps to keep a containment group alive for the harness) — intentional, not a finding.
• tests.rs / #[cfg(test)] blocks throughout were skipped per spec.

[zackees]

Closed by PR #843 (workspace-wide timeout sweep).