fs store: two root causes of "poisoned storage should not be used" panic (persist + load NotFound)

[mkdir700]

Warning

Disclosure: this issue was drafted with the help of Claude Code based on a real bug we hit and fixed downstream — the panic, the trigger sequence, and the two code paths are from actual production debugging, but the writeup itself was AI-assisted. If anything here is inaccurate, mischaracterized, or otherwise not actionable, please feel free to close — no offense taken.

Summary

BaoFileStorage::bitfield() panicking on the Poisoned arm (src/store/fs/bao_file.rs:410) is a known symptom — PR #214 proposes to soften that arm. This issue is about the two upstream code paths that poison a handle when they should not, both of which are still present on main and in v0.100.0. Fixing them removes the panic at the source and is independent of (and complementary to) the symptom-level fix discussed in #214.

Both were reproduced from a downstream app (uniclipboard, a clipboard-sync tool that fetches blobs via iroh-blobs) and crash the iroh-blob-store worker task, leaving the store unusable until process restart.

Root cause 1 — HashContext::persist() poisons non-Partial handles

src/store/fs.rs:990-1009:

async fn persist(&self) {
    self.state.send_if_modified(|guard| {
        let hash = &self.id;
        let BaoFileStorage::Partial(fs) = guard.take() else {
            return false;
        };
        // ...
        false
    });
}

BaoFileStorage::take() is mem::replace(self, BaoFileStorage::Poisoned) — it always swaps. When the current variant is Complete (or anything other than Partial), the let-else returns early after the state has been replaced with Poisoned. The original Complete state is discarded and never restored.

A later observe(hash) → BaoFileStorageSubscriber::forward → BaoFileStorage::bitfield() then hits the Poisoned arm and panics.

Observed trigger sequence (UTC timestamps from a failing run):

1. 13:40:13 — fetch blob 8878b5704b…; in-memory handle reaches Complete.
2. 13:40:13 — export with ExportMode::TryReference. Metadata DB transitions to Complete { data_location: External(...) }; in-memory state still Complete.
3. Some time later the entity manager evicts the handle (ShutdownCause::Idle) and calls persist(). State is Complete, so the let-else returns early — but take() has already swapped to Poisoned.
4. 13:41:32 — user re-copies the same file; receiver calls store.blobs().observe(hash); subscriber dispatches bitfield() on the poisoned state.

thread 'iroh-blob-store-2' panicked at iroh-blobs-0.100.0/src/store/fs/bao_file.rs:410:17:
poisoned storage should not be used

Proposed fix — check the variant before taking:

async fn persist(&self) {
    self.state.send_if_modified(|guard| {
        let hash = &self.id;
        if !matches!(&*guard, BaoFileStorage::Partial(_)) {
            return false;
        }
        let BaoFileStorage::Partial(fs) = guard.take() else {
            unreachable!("variant checked above");
        };
        // ...rest unchanged...
        false
    });
}

A more defensive variant would change BaoFileStorage::take() itself to only swap when a predicate matches, but that has broader API impact.

Root cause 2 — HashContext::load() poisons on every IO error, including NotFound

src/store/fs.rs:286-302:

match self.global.db.get(self.id).await {
    Ok(state) => match BaoFileStorage::open(state, self).await {
        Ok(handle) => handle,
        Err(_) => BaoFileStorage::Poisoned,
    },
    Err(_) => BaoFileStorage::Poisoned,
}

BaoFileStorage::open calls std::fs::File::open(path) for DataLocation::External(...) (src/store/fs/bao_file.rs:596). When the external path is missing, the error is io::ErrorKind::NotFound — a fully recoverable condition (re-fetch from the network rewrites the metadata). Mapping it to Poisoned means the very next observe(hash) panics.

This conflates two distinct failure modes:

1. Stale metadata, target file gone — recoverable, should surface as "blob absent".
2. Real on-disk fault (permission denied, corrupted outboard, disk full) — should stay loud / Poisoned.

In our case the drift comes from ExportMode::TryReference caches being pruned by the application or older releases while the metadata DB still points at them. The application-side reconciliation is a separate concern; the panic on observe is the platform-level bug.

Proposed fix — route through a small helper that special-cases NotFound:

fn state_from_open_error(err: &io::Error) -> BaoFileStorage {
    match err.kind() {
        io::ErrorKind::NotFound => BaoFileStorage::NonExisting,
        _ => BaoFileStorage::Poisoned,
    }
}

// in load():
Ok(state) => match BaoFileStorage::open(state, self).await {
    Ok(handle) => handle,
    Err(e) => state_from_open_error(&e),
},

Downstream then sees the blob as absent and goes through the normal download → ImportBao path, which rewrites the metadata entry from the fresh fetch.

Relationship to PR #214

PR #214 changes bitfield() so the Poisoned arm returns Bitfield::empty() instead of panicking. That is a useful symptom-level guard for any future code path that ends up Poisoned, but:

• it does not stop healthy Complete handles from being poisoned by persist(),
• it does not stop missing-file recoverable cases from being poisoned by load(),
• and the design discussion on fs-store observe: treat transitional states as empty bitfields #214 (empty vs. Result<Bitfield, ...> vs. waiting on Loading) hasn't converged.

The two fixes here are orthogonal to that decision and can land independently.

Regression test

A deterministic code-level test for root cause 1 is straightforward:

• Construct a BaoFileStorage::Complete(...) inside a watch::channel.
• Run the buggy snippet — assert the state ends up Poisoned and bitfield() panics.
• Run the patched snippet — assert the state is unchanged and bitfield() returns Bitfield::complete(...).

We have such a test (the buggy one marked #[should_panic(expected = \"poisoned storage should not be used\")], so it passes today and will start failing the day upstream merges an equivalent fix) and an end-to-end smoke test exercising add_bytes → observe → export_with_opts(TryReference) → sync_db → observe over many distinct hashes to churn the entity-manager actor pool. Happy to send these as a PR alongside the fixes if useful.

Honest caveat on a "fully deterministic" public-API repro

For root cause 1, we tried but did not get a deterministic public-API repro that panics on upstream and passes on the patch:

• The buggy persist() is send_if_modified(|guard| ... false). tokio::sync::watch only signals changed() when the closure returns true, so a subscriber parked on changed() never wakes up after the buggy take() — a concurrent test that hopes to see the panic via the subscriber path stalls.
• Through FsStore, the production race window is closed quickly by the entity-manager: after on_shutdown → persist poisons the state, the actor either reset()s the handle when the inbox is non-empty or recycle()s it back to the pool. The observable window is a few instructions inside the manager actor; staging a test there from outside the crate would need new test hooks.

The code-level test pins the broken semantics; the smoke test verifies the patched path is healthy under repeated use. Root cause 2 does have a clean public-API repro (the one @YuniqueUnic already posted in #214): create a >16 KiB blob → shutdown → delete the .data file → reopen → observe. With root cause 2 fixed (NotFound → NonExisting), observe would yield an empty bitfield instead of panicking, regardless of #214.

Environment

• iroh-blobs v0.100.0 (also verified on main at the time of filing — both code paths unchanged)
• Downstream consumer: uniclipboard, receiver-side blob fetch + ExportMode::TryReference cache