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

Sub-issue of #813 — fbuild-packages audit lens: convert sync code to async on the daemon's tokio runtime. Partner audit is #805 ("blocking HTTP without timeout"); this issue covers the same call sites under a different lens (async-ification). Per #813's intro: the fixes compose — the end-state is a single shared async reqwest::Client configured with .timeout(...) and .connect_timeout(...), so #805's CRITICAL findings collapse to "set a timeout on the shared client" once this issue's plumbing lands.

The crate is in surprisingly good shape on the network side — almost every HTTP call already uses async reqwest::get(...).await. The dominant async anti-pattern is not reqwest::blocking, it's the block_on(...) sandwich: every Package::ensure_installed is a sync trait method that owns an async staged_install, so each implementation does tokio::runtime::Handle::block_on(...). That's ~30 call sites in this crate alone, hides every install from tokio-console, and prevents axum from doing anything else on the worker during a 30-second toolchain download.

The shared async Client proposal

Today every HTTP call in the crate uses the convenience reqwest::get(url), which constructs a brand-new reqwest::Client per request — no connection pooling, no shared timeout config, no shared connector. The seven call sites that need this are:

• downloader.rs:113 (get_with_retry)
• downloader.rs:178 (open_with_retry for streaming download)
• toolchain/clang.rs:147 (clang manifest fetch)
• library/registry.rs:24 / :77 / :189 (PlatformIO registry: 3 calls)
• library/arduino_api.rs:51 (the only reqwest::blocking::* in the crate)

Proposed: add crates/fbuild-packages/src/http.rs:

use std::time::Duration;
use tokio::sync::OnceCell;
use reqwest::Client;

static HTTP_CLIENT: OnceCell<Client> = OnceCell::const_new();

pub async fn client() -> &'static Client {
    HTTP_CLIENT
        .get_or_init(|| async {
            Client::builder()
                .connect_timeout(Duration::from_secs(10))
                .timeout(Duration::from_secs(60 * 10)) // big toolchains
                .pool_idle_timeout(Duration::from_secs(90))
                .user_agent(concat!("fbuild/", env!("CARGO_PKG_VERSION")))
                .build()
                .expect("reqwest client builder is infallible with our config")
        })
        .await
}

Then every call site becomes http::client().await.get(url).send().await — picks up timeouts uniformly (closes #805) and reuses TCP/TLS connections across the daemon's lifetime. The blocking feature on the workspace reqwest = { features = [..., "blocking"] } can be dropped once arduino_api.rs migrates.

OnceCell (tokio) is the right primitive — OnceLock (std) doesn't have async init, and the existing pattern in disk_cache/lease.rs:15 uses OnceLock for non-async state, so the precedent is "tokio for async init, std for sync init".

Findings table

What was searched

Patterns (all under crates/fbuild-packages/src/):

• reqwest:: — 11 hits, all async except arduino_api.rs:51
• Client::(new|default|builder) — 0 hits (no shared client today)
• std::thread::spawn / JoinHandle / thread::scope — 1 hit (library_compiler.rs:206)
• rayon:: / par_iter — 0 hits
• std::process::Command::new / Command::new — 0 hits in this crate (subprocess is via fbuild_core::subprocess::run_command, audited under that crate's sub-issue)
• block_on — 30+ hits across all Package::ensure_installed impls + 4 _sync wrappers + lnk/resolver.rs:117
• Mutex::new / RwLock::new — only std::sync::Mutex for in-process throttling and rusqlite conn protection; none held across .await
• mpsc:: — 0 hits
• OnceLock / OnceCell — OnceLock used for nonce/touch throttle (sync init, correct); no OnceCell yet
• flate2 / tar / zip / xz2 / bzip2 / zstd — out of scope per spec

Modules audited:

• downloader.rs, extractor.rs (extraction excluded per spec)
• install_lock.rs — already correct (tokio::time::sleep, no std::thread::sleep)
• lib.rs (PackageBase::staged_install, block_on_package_future)
• library/library_manager.rs, library/library_downloader.rs, library/library_compiler.rs
• library/arduino_api.rs, library/registry.rs
• toolchain/{clang,avr,arm,arm_gcc8,esp32,esp32_metadata,esp8266,esp_qemu,riscv,rp2040_pqt,teensy_arm}.rs
• Every library/*_core.rs and library/*_framework.rs ensure_installed impl

Full block_on ensure_installed call-site list (severity HIGH, one trait-flip fixes all):
toolchain/avr.rs:115, toolchain/arm.rs:106, toolchain/arm_gcc8.rs:74, toolchain/esp32.rs:169, toolchain/esp8266.rs:98, toolchain/esp_qemu.rs:172, toolchain/riscv.rs:183, toolchain/rp2040_pqt.rs:104, toolchain/teensy_arm.rs:97, library/apollo3_core.rs:131, library/arduino_core.rs:129, library/arduino_core_lpc8xx.rs:171, library/arduino_mbed_core.rs:180, library/attiny_core.rs:106, library/avr_framework.rs:177, library/ch32v_core.rs:140, library/cmsis_atmel.rs:107, library/cmsis_framework.rs:93, library/esp32_framework/mod.rs:136, library/esp32_platform.rs:178, library/esp8266_framework.rs:193, library/nrf52_core.rs:137, library/renesas_core.rs:161, library/rp2040_core.rs:161, library/sam_core.rs:116, library/samd_core.rs:120, library/silabs_core.rs:107, library/stm32_core.rs:129, library/teensy_core.rs:151.

Out-of-scope notes

• Archive extraction (extractor.rs, flate2 / tar / zip / xz2 / bzip2 / zstd): sync crates with no async equivalents fbuild already uses. Per spec, do not rewrite — but each call from async context should be wrapped in tokio::task::spawn_blocking to vacate the tokio worker during a multi-GB ESP-IDF tarball decompress. Currently staged_install calls extractor::extract(&archive_path, &staging_path) directly on the runtime worker (lib.rs:418). That's worth a follow-up issue, but doesn't belong in this audit's scope.
• SHA-256 hashing (sha2::Sha256 in downloader.rs:325, library_compiler.rs:13): CPU-bound, sync is correct. The verify_checksum_async wrapper does the right thing (async file read + sync hash). If invoked from async hot paths, wrap in spawn_blocking if the file is large.
• Test code: lib.rs:502 (test-only tokio::spawn), avr.rs:364 (gated #[ignore] integration test), install_lock.rs test module — all out of scope.
• Drop impls: install_lock.rs:163 InstallLockGuard::drop does sync std::fs::remove_dir_all — must stay sync (Drop is not async).
• In-process Mutex (lib.rs:480-483 mark_package_touch_needed): std::sync::Mutex<HashSet<String>> for in-process throttle, never held across .await. Correct as-is.

Relationship to #805

#805 will land timeouts via the same reqwest::Client::builder() change proposed above. The clean fix is for one of these issues to land the shared http::client() module first (this issue does the lift, #805 then adds the timeout config to the builder) — recommend coordinating so they touch the same file once, not twice.