Author

Justin Mazzola Paluska <jmp@cloudflare.com> 2025-10-14 09:20:45 -0400

Parents

781e611

Tree

36fba05

Throw from ActorSqlite::sync() when breaking the output gate

In the recent implementation of allowUnconfirmed for SRS-backed
DOs (#5138), we changed the implementation of `ActorSqlite::sync()` to
wait for `lastCommit.addBranch()` instead of `outputGate.wait()`.

There are a few different ways for the `outputGate` state to change in a
way that isn't reflected in `lastCommit`:

- `deleteAll()` does `commitTasks.add(outputGate.lockWhile(...))`
  without affecting `lastCommit`.

- `onCriticalError()` does the same, to break the output gate on
  critical sqlite error.

- `InternalDebugInfoApi::blockOutputGate()` could break the
  `outputGate` (though this only seems to be used for tests).

- In general, since `outputGate` is passed in by reference to
  `ActorSqlite`, anything outside of `ActorSqlite` with the reference
  could break the `outputGate`.

For example, @jclee notes that if a user does the following:

1. DO starts an explicit transaction
2. DO executes an sqlite query that triggers a critical error, but catches and ignores it
3. DO awaits `sync()`

the `sync()` implementation before #5138 would throw, but the
implementation from #5138 would not.  It may not matter too much
because if the `outputGate` breaks, the DO is doomed, but we might as
well get the behavior right for `sync()`.

We can solve this by either fixing up all places that break the
`outputGate` to also break `lastCommit` (messy and
abstraction-breaking in the last 2 cases) or by joining `lastCommit`
and the `outputGate`.  We do the latter because it’s easier, cleaner,
and will catch any future `outputGate` breakage.

Claude wrote the test based the situation @jclee noted.  Without the
fix to ActorSqlite::onNoPendingFlush(), the test fails because
`sync()` does not throw.