avoid crash after concurrent undo action failure

[davepacheco]

Fixes #473.

[davepacheco]

(Writing this for my future self as much as anybody, but I would welcome scrutiny!)

In terms of analyzing this change: the only behavioral change here is to relax two assertions that previously caused a correct program to panic. Does the program now do the right thing there? There are two obvious cases.

First is where two undo actions fail concurrently By that, I mean that the executor kicked off two tasks and both tasks finished and reported their TaskCompletion with an error before the main executor task read either one off its queue. (the timing doesn't actually matter). In this case, when processing the first message, we'll attempt to propagate the SgnsUndoFailed node, recording the undo failure and then invoking saga_stuck(). This will set self.stopping, check node_tasks, find it non-empty, and do nothing else. This is the same as before. The loop then processes the second task's completion. When calling node_task_done(), that function notices that we're stopping with no tasks remaining and sets the exec_state to Done. Then we propagate the second SgnsUndoFailed node. This is where we used to crash. Now, we record another undo action failure and call saga_stuck() again. This time, it's a no-op. The saga is now Done, so the main loop bails out, and the executor comes to rest. This looks right.

The second case is where a log is recovered that contains two undo action failures. Recovery calls saga_stuck(), which sets stopping, finds no tasks running (since nothing is running yet), so marks the saga Done. Then it finds the second UndoFailure and calls saga_stuck() again. Before, this would crash, as in the live case. Now, it's a no-op again.

Both of these cases are exercised in new test cases in this PR.

Can we make more general statements?

• This change doesn't affect any programs that didn't previously crash.
• It was already true before this change that when a saga became stopping (because an undo action failed), propagate() could be called again on various different kinds of nodes. The other three kinds of nodes all checked explicitly for stopping and avoided kicking off new work in that case. It's only the SgnsUndoFailed node that assumed (erroneously) this wasn't possible.
• In terms of what happens when previously it would have crashed:
  • saga_stuck(): in the case where it would have crashed and now doesn't, the state is stopping and Done. In that case, this function is now a no-op.
  • SgnsUndoFailed::propagate(): the only thing this does is record the undo action error (which is correct and still has a valid assertion that it hasn't been done already) and call saga_stuck(), which we've said is safe in this case.

We might also look at callers of those functions, which now continue on when they wouldn't have before.

• The only other caller of saga_stuck() is the recovery path. It'll now proceed with the rest of recovery. I expect most of that is orthogonal to the way we panicked and so this should be fine, but it's hard to say for sure, but also: I think the worst that can happen is that we panic later in recovery or gracefully fail to recover the saga, neither of which is worse than what happened before this change. Once recovery has finished, we should be able to resume the saga. The test case exercises that at least in one path, this works as expected.
• There are various callers of propagate():
  • Tail calls in SgnsFailed::propagate(): these don't assume anything about what it did or didn't do (other than not panic)
  • Calls in SgnsUndone::propagate(): after these, the caller may wind up trying to call propagate() on other nodes. That should be fine -- their state is unrelated to the UndoFailed one that was just propagated. At least, these should be fine as long as it's generally okay to call propagate() with a saga being stopping and Done, which I believe it is (see above -- they generally just record node-specific state and then check for stopping and bail out).
  • The call in the main loop: this one we analyzed above.

It's unfortunate that we don't document and enforce stronger invariants in this code, but as discussed, this is all a lot less rigorous than we'd like. Given RFD 704, it's not clear that it's worth investing a lot more here, at least right now.