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.