fix!: return the sync outcome from BobState::run

[anchpop]

Problem

BobState is defined the following way:

pub struct BobState {
    namespace: Option<NamespaceId>,
    peer: PublicKey,
    progress: Option<SyncOutcome>,
}

With progress being Option<SyncOutcome>.

The BobState::into_outcome function assumed that progress would never be None when it was called:

/// Consume self and get the [`SyncOutcome`] for this connection.
pub fn into_outcome(self) -> SyncOutcome {
    self.progress.unwrap()
}

But this was not always true. If you look at handle_connection, it does this:

let mut state = BobState::new(peer);
let res = state
    .run(/* ... */)
// ...
let outcome = state.into_outcome();

In other words, handle_connection assumes that after run() returns, even when it returns an error, progress is Some(...).

But there is a code path where this is not true. If you look at the implementation of BobState::run, it sets progress to None, then sets it back to Some(...) only in some cases:

// In BobState::run

// self.progress is set to None
let last_progress = self.progress.take().unwrap();

// ...
// Then, self.progress is supposed to be set back to Some(...) a little later in the function: 
// ...

let (reply, progress) = next.map_err(|e| self.fail(e))?;
self.progress = Some(progress); 

But maybe you can see the issue. if next is Err(...), we will hit the ? and self.progress will stay None. And it is possible for next to be Err(...). When this happened to me, the trigger was some failure in the docs engine that I haven't fully diagnosed, but any error from sync_process_message produces it. But the point is, if next is Err, then self.progress is None, which means BobState::into_outcome will panic!

(I hit this in production)

Solution

IMO, there is a mismatch in the code, where BobState::progress should

1. only be accessed on a successful sync
2. always be available on a successful sync

But currently, it is just an Option<...> on BobState, which means it can be accessed on an unsuccessful sync, and the type system doesn't even guarantee that it's always available on a successful sync.

Both can be fixed if we take it out of BobState entirely, and move it to the value BobState::run returns in the success case:

pub async fn run(...) -> Result<(NamespaceId, SyncOutcome), AcceptError>

By doing this, we make it impossible to even think of accessing the SyncOutcome in run's error case, and removes the need for a .unwrap.

I also took the opportunity to clean up run_alice slightly, as it also had an unnecessary .unwrap. (That one is safe, but a simple change to the code removes it entirely.)

Breaking Changes

Type signature of BobState::run changes:

pub async fn run(...) -> Result<NamespaceId, AcceptError> // before
pub async fn run(...) -> Result<(NamespaceId, SyncOutcome), AcceptError> // after

BobState::into_outcome is removed.

Change checklist

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

[thewtex]

We hit this same panic, and can add the trigger, what it takes down, and a deterministic repro.

Trigger: leaving a document while a run this node accepted is between messages. LiveActor::leave calls set_sync(namespace, false). The peer's next Message::Sync then fails in replica_if_syncing ("sync is not enabled for replica"), BobState::run returns through the ? with progress == None, and handle_connection panics in into_outcome() (src/net/codec.rs:290 in 0.101.0). Only the accepting side is affected. A run that arrives after the leave is rejected by the accept callback and is harmless.

It takes down the whole engine, not one connection. The live actor's running_sync_accept arm does res.context("running_sync_accept closed")? (src/engine/live.rs:278 in 0.101.0). The panic's JoinError ends run_inner, and shutdown() stops gossip and the sync actor. Every later call on that engine fails: for us, the next leave with a closed-channel error and the next start_sync with sending to iroh_docs actor failed.

Repro (deterministic on 0.101.0).

1. Node B: a doc with at least one entry (with none, the run ends after one message) and live sync started.
2. Peer A: dial B on DOCS_ALPN directly, send Init carrying an empty replica's sync_initial_message(), and read B's reply.
3. B: leave the doc.
4. A: send any Message::Sync. It is refused before its contents are read.

B's accept task panics every time. Between two ordinary engines this is a race, which may be why it looks intermittent in production.

This PR fixes it. With this PR's diff (e7233d1) applied to 0.101.0, the repro passed 20/20; unmodified 0.101.0 failed 3/3. We're carrying self.progress.unwrap_or_default() in into_outcome as a local patch in the meantime and would be glad to drop it for this. Happy to contribute the repro as a test here if that would help it land.