fs-store observe: treat transitional states as empty bitfields

[YuniqueUnic]

Description

BaoFileStorage::bitfield() currently panics when an observer sees
Initial, Loading, or Poisoned.

BaoFileStorageSubscriber::forward() reads borrow().bitfield() immediately
when an observe stream starts, so a subscriber can panic if it attaches while
an entry is still loading, being replaced, or left in a transitional state
after a failed operation.

This change treats those transitional states as Bitfield::empty() instead.

Why

For observe streams, an empty bitfield preserves the "not complete yet"
semantics without turning a transient internal state into a fatal failure.

This also matches the surrounding fs-store behavior more closely:
other paths already treat these states as recoverable/no-op states instead of
requiring an unconditional abort.

Breaking Changes

Notes & open questions

Tests

• add bitfield_is_empty_for_transitional_states
• add subscriber_forward_emits_empty_bitfield_for_transitional_state

Change checklist

• Self-review.
• Documentation updates following the style guide, if relevant.
• Tests if relevant.
• All breaking changes documented.

[rklaehn]

Thanks for reporting. I will look into this as soon as we got our latest iroh release done.

I am not sure if empty is a good guard value, you would then see the bitmap going from empty to whatever, which looks like you suddenly downloaded a bunch of stuff. But it should definitely not panic!

[YuniqueUnic]

Thanks @rklaehn

Agree that empty() might mislead observers about progress.

Would a Bitfield::unknown() variant (or returning Option) be a cleaner way to represent "state in flux" without overloading the meaning of empty()?

[rklaehn]

Either that or somehow wait until it is loaded. I must confess that I haven't worked in the bowels of blobs for a long time, so I will have to take a look at this next week.

Is it easy to write a test where you see this panic while using the public API? That would be quite helpful.

[YuniqueUnic]

I dug back into this and I now have a small public-API repro for the same failure site.

This is not the exact MyApp restart race I originally hit, but it does reproduce the panic through store.observe(...) using only the fs-store public API.

The minimal setup is:

1. create a blob large enough to be stored as an owned data file (> 16 KiB),
2. shut the store down,
3. delete the backing .data file so reloading that entry becomes Poisoned,
4. reopen the store and call observe(hash).

Dropped into src/store/fs.rs's test module, this is enough:

#[tokio::test]
async fn test_observe_poisoned_entry_after_missing_data_file() -> TestResult<()> {
    let testdir = tempfile::tempdir()?;
    let db_dir = testdir.path().join("db");
    let data = test_data(1024 * 16 + 1);
    let hash = Hash::new(&data);

    {
        let store = FsStore::load(&db_dir).await?;
        let tt = store.add_bytes(data).await?;
        assert_eq!(tt.hash, hash);
        store.shutdown().await?;
    }

    let data_path = Options::new(&db_dir).path.data_path(&hash);
    fs::remove_file(&data_path)?;

    let store = FsStore::load(&db_dir).await?;
    let _ = store.observe(hash).await;
    store.shutdown().await?;
    Ok(())
}

On the parent of my patch (8e0fa7a), this logs:

thread 'iroh-blob-store-*' panicked ... poisoned storage should not be used

On my patch, the same call no longer panics and instead yields an empty bitfield.

So this seems to demonstrate the public-API problem: observe() can reach a transitional internal state and panic instead of staying recoverable.

[rklaehn]

Did you manage to get this to panic without getting the store into an inconsistent state?

I think the big issue is that the observe stream does not have any way to express when an entry is "broken". I was trying to keep observe as just a bitfield stream for convenience, but maybe that was wrong.

When an entry is broken you basically have 2 options: 1. fix it by nuking it, then return empty. But that means that maybe we broke something. 2. report broken by making the result of observe a Result<Bitfield, ...>. But the latter than has the big can of worms of sending the error over the wire.

Not sure what is the best way forward, considering options.

[YuniqueUnic]

I think there may be a simpler way to do this without turning every observe item into Result<Bitfield, ...> on
the wire.

The payload can stay as plain bitfields, and a broken entry can just terminate the observe request / stream via the
existing failure path.

So I'd expect the behavior to be:

• wait through Initial / Loading
• treat Poisoned / broken as a terminal observe failure

That keeps empty meaning "valid entry, no verified data yet", instead of overloading it to mean "broken".

Roughly:

• observe() waits for load()
• healthy entries keep yielding Bitfields
• broken entries return an error and end the stream
• no new error-bearing observe item type needed