Release transactions held by web clients that are gone

[mattcosta5651]

The problem

On the web all tabs share one drift server in a worker, and a client transaction is a series of round trips. The server keeps the open transaction at the head of _executorBacklog, so every other client waits for its turn.

A tab closed mid-transaction never sends its commit, and drift cannot currently notice. MessagePort has no close event for a context going away — WebPortToChannel.channel says as much in its doc comment — and ServerImplementation.serve only removes a closed channel from _activeChannels; the executors that channel opened are never released. The abandoned transaction stays at the head of the backlog and every later transaction in every surviving tab waits behind it until all clients are gone.

I found this in a Flutter web app whose end-to-end test opens a second tab and closes it. That tab was closed 17ms into a cache transaction, and the first tab then sat in a wanderagent_cache transaction for 422 seconds that had not run a single statement, with nothing else open in its own connection. A two-tab reproduction in Chromium hangs on 2.34.2/2.35.0 and completes with this change.

The change

Liveness. Clients take a uniquely named Web Lock and hold it for their whole lifetime, sending the name in ServeDriftDatabase.clientLock behind a new protocol version v5. The worker requests the same lock; the browser grants it only once the client's context is destroyed, which is the signal that the client is gone, and the worker then ends that client's channel. This builds on the locks getter and the AcquireLock extension already in wasm_setup/shared.dart. Where Web Locks are unavailable, or either side predates v5, behaviour is unchanged.

Release. ServerImplementation records which channel opened each transaction or exclusive executor. When a channel closes, it rolls back and releases what that channel still held — newest first, so a nested executor ends before its parent. An executor granted to a channel that closed while waiting for its turn is abandoned the same way, and _loadExecutor now throws for a released executor rather than waiting for a turn that can never come.

Tests

Two tests in remote_test.dart, in the style of the existing nested transactions case, using a client whose channel stops delivering messages without a rollback or close — what a closed tab does. Both time out against current develop and pass with this change:

• a closed client's open transaction is rolled back, and another client can then write;
• a transaction granted to a client that closed while queued behind another transaction is abandoned too.

dart analyze is clean and the rest of remote_test.dart still passes. I have not been able to run the browser suites on this machine, so the worker side has been exercised through an app build rather than drift's own web tests — worth a look from someone who can run those.

Happy to adjust anything here, including whether this deserves its own protocol version or should ride along without one.

🤖 Generated with Claude Code

[simolus3]

I agree with the fix of using navigator locks to detect this. I'm happy to run tests with a real worker (we also do that in CI), but I have a few minor comments first.

[mattcosta5651]

All five points are addressed in the latest push, thanks — crypto.randomUUID(), no protocol bump, one _ManagedExecutor map, the plain ! back in _loadExecutor, and concurrent abandons.

One thing I'd like your call on, from the ! discussion. Reverting that also removed the _waitForTurn guard, and those were doing two different jobs. With the plain predicate, a statement that arrives for a transaction which has already committed, rolled back or been abandoned can never see its id at the head of the backlog, so it waits in _waitForTurn forever rather than reaching _loadExecutor to fail.

You're right that a closed client can't send statements — but an unawaited statement from a live client can, which is the case the ArgumentError about "don't await all operations made inside a transaction" already warns about. This isn't caused by the patch: it's how the code behaves today, and my earlier version had only incidentally changed it.

I've left the simple form here since you asked for it. If you'd like the fail-fast guard as well, it's two lines and I'm happy to push it:

return !_managedExecutors.containsKey(transactionId) ||
    (_executorBacklog.isNotEmpty && _executorBacklog.first == transactionId);

Also worth flagging for your worker-based run: the branch now drains statements already in flight against an executor before rolling it back. A handler resolves its executor and then yields, so a rollback starting in between could let that statement run outside its transaction — the assertion that would catch it is stripped in release builds. There's a test for it in remote_test.dart.

🤖 Generated with Claude Code

[simolus3]

Thanks! I have merged this in 9d2330b, also added an integration test.