Skip to content

unwrap discipline sweep (#844) #847

Description

@zackees

Goal

Audit found 2,238 .unwrap() calls in production code workspace-wide. Existing ban_unwrap_in_daemon_handlers scopes only to crates/fbuild-daemon/src/handlers/**. Per #844: no grandfathering.

Sub-issue of #844.

Scope expansion

  1. Extend ban_unwrap_in_daemon_handlers to cover:
    • All non-test code under crates/fbuild-daemon/src/** (not just handlers/).
    • CLI subcommand entry points under crates/fbuild-cli/src/cli/** (these are user-facing; a panic crashes the CLI with a stack trace).
  2. New lint ban_poison_panic that flags specifically:
    • Mutex::lock().unwrap() / Mutex::lock().expect(_)
    • RwLock::read().unwrap() / RwLock::write().unwrap()
    • Reason: these panic on lock poison, propagating one panic into a cascade across every other lock holder.
    • File scope: workspace-wide for std::sync::Mutex / std::sync::RwLock (tokio variants don't poison).

Top-10 offending files (counts from audit)

File unwrap count
crates/fbuild-build/src/build_fingerprint/mod.rs 101
crates/fbuild-build/src/framework_libs.rs 76
crates/fbuild-daemon/src/models.rs 51
crates/fbuild-build/src/build_info.rs 50
crates/fbuild-packages/src/library/nrf52_core.rs 46
crates/fbuild-packages/src/library/library_compiler.rs 43
crates/fbuild-packages/src/library/framework_library.rs 42
crates/fbuild-build/src/pipeline/library.rs 39
crates/fbuild-packages/src/disk_cache/gc.rs 38
crates/fbuild-packages/src/extractor.rs 36

Identified poison-panic sites (10+)

  • crates/fbuild-daemon/src/broker/backend.rs:149
  • crates/fbuild-daemon/src/broker/service.rs:401 (ENV_LOCK.lock().unwrap())
  • crates/fbuild-daemon/src/context.rs:615 (last_activity.lock().unwrap())
  • crates/fbuild-daemon/src/device_manager.rs — multiple Mutex sites

Migration strategy

Per-crate sweeps, smallest-leaf-first:

  1. fbuild-paths / fbuild-core (likely <50 unwraps)
  2. fbuild-config
  3. fbuild-packages/disk_cache/
  4. fbuild-packages/library/ (the 5 high-count files)
  5. fbuild-build/build_info.rs, build_fingerprint/, framework_libs.rs, pipeline/library.rs
  6. fbuild-daemon/models.rs + device_manager.rs + broker/
  7. fbuild-cli/src/cli/
  8. Enable extended lint + tighten poison-panic lint to zero allowlist

Each unwrap is converted to either:

  • ? for early-return on Result-returning functions (most cases).
  • Explicit match with a typed error variant where context matters.
  • expect(...) with an actionable message for truly-impossible invariants.

Acceptance

  • Extended ban_unwrap lint scope: all of fbuild-daemon/src/ non-test + fbuild-cli/src/cli/
  • New ban_poison_panic lint deployed workspace-wide
  • Zero allowlist entries in either lint
  • Production-code unwrap count goes from 2,238 → close to zero (likely a handful in genuinely-impossible-invariant cases with expect + actionable message)

This is the largest sub-issue in the #844 family. Expect 2-3 weeks of focused work.

References: #844, #826 (origin of ban_unwrap_in_daemon_handlers).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status
    Triage

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions