Abort a request-scoped transaction promptly when its client disconnects

[kriszyp]

A client that disconnects mid-request used to leave its request-scoped transaction open — holding staged writes and native RocksDB write intents — until the handler's promise settled or the 30–60 s long-transaction monitor swept it. harper#2001 traced a live incident to exactly this: an orphan held past a disconnect wedged a later, conflicting commit 27 s afterwards, and with it that worker's write path until restart. With this PR, once a request's signal aborts, the request cannot write any more — in whatever transaction, through whatever API spelling — and the writes it had staged are released at once.

How it works

• Cancellation is enforced where a write is admitted. Every transaction created for a request carries its signal on the chain root — set in transaction() and in txnForContext's per-context self-committing transaction, the two places a transaction is created for a request context. admitRequestWrite() runs in both engines' addWrite and at every deferred-save admission: an aborted signal poisons the chain and the write is refused with requestAbortedError (499). So a static-API write in a catch — which joins the poisoned context transaction — a fresh transaction(context, …), and a retained instance writing after its scope ended are all refused alike, as the Context.signal contract now states. sourceApply never carries the signal; it has no resume path (harper-pro#348).
• The subscription belongs to the writes, not to the scope. The first admitted write subscribes the chain root to the signal, and the listener is released once the chain's writes and commit attempts settle or the root aborts. A read-only request registers nothing and always completes, so an iterator streaming a response is never cut off; a write-bearing chain — including a self-committing write parked in pre-commit work — is poisoned and aborted the moment the client goes, through the same abortAndPoison() the monitor's abortDueToTimeout() uses.
• Native submission, not entry into commit(), is the point of no destructive return. A poison landing while a submitted attempt is in flight flags the chain but aborts nothing, so the attempt reaches its native outcome; aborting would race cleanup against an unknown outcome and unlink blob files a successful commit references (Over-time transaction abort deletes the pre-saved deploy payload blob, leaving later deploys referencing a destroyed blob (regression from #1411, 5.1.21) #2062). For a disconnect that covers the whole chain: once any store has submitted, the chain's unsubmitted stores ride the cascade instead of being aborted, so a multi-store commit never splits (Force-committing an over-time transaction leaves orphaned secondary-index entries (atomicity violation) #1407) — and both engines' resume after pre-commit work honors that, so a chained LMDB store parked in a blob save still lands. The submission marker is set immediately before each native commit, after main's database-closing assert so a closing database cannot leave the root marked submitted.
• Transaction-owned iterators are tracked and closed when unreachable. Table.search() registers each result set with the transaction owning its read reference; closeOwnedReadIterators() runs their onDone across the chain. The wrapper closes them only when the callback threw, since then nothing was returned to consume them; both monitors' CLOSED arm is the backstop — and the LMDB monitor had no CLOSED arm at all, so a poisoned link pinned an LMDB read snapshot for the life of the process.
• Stalled submitted commits are observed, never aborted. Past the limit the monitor defers; after a short diagnostic grace it poisons fresh follow-up work, reclaims read snapshots separate from the submitted writes, and logs once. A pre-submit attempt on an already-poisoned link gets no commit-phase grace: its continuation throws before committing, so sparing it would only pin its snapshot.
• Background work must not inherit a request's context. The analytics flush was armed from inside whichever request recorded the first sample of a period, so it — and the scheduled tasks it starts, which write through the static Resource API — ran under that request's AsyncLocalStorage context: attributed to its user, and under request-lifetime cancellation, refused once that one client had gone.
• Supporting fixes. A write-only transaction never released its native handle on abort(). Request signals accept any number of listeners, and the uWS fallback signal is created once. uWS WebSocket connections never had a working disconnect signal; the upgrade's controller is now driven by the adapter's 'close'. Both engines funnel commit-chain failures through one wrapper-level abort, LMDB's abort cascade skips a child whose native commit is outstanding, and a lingering LMDB write's throwaway transaction owns its own cancellation.

The invariants are recorded in resources/DESIGN.md — A client disconnect cancels the request's writes, indexed from the root DESIGN.md.

For the human reviewer

Framing-Verdict: better-alternative-exists (114e2b20e2d7)

Both planning verdicts were non-clearing, and both were resolved by adopting the alternative. The first planning recheck held that cancellation should belong to the request lifetime rather than to the one transaction open when the event fired; you ruled to adopt it. The second, run after that implementation, kept that framing and proposed a write-owned subscription in place of a per-scope listener, on two facts: a retained instance's self-committing write had no listener at all, and every read-only request paid for one. I adopted it. The change a caller will notice: every write on a request after its client disconnected is refused with 499 — including in a catch/finally, a fresh transaction(context, …), and a retained instance. Work that must outlive the client needs a context without the signal, as deployment tracking already builds.

Decisions taken here, and where to push back:

• Cancellation is on by default, and the storage layer is coupled to request lifetime. Existing apps' post-disconnect writes change behavior at once; the opt-out is a signal-free context. Write admission is the only placement that also covers writes made outside any scope.
• ALS-inherited background work is audited, not proven clean. A timer armed inside a request inherits that request's context. I checked the plausible lazily-armed writers — analytics (fixed above), the scheduler (armed at startup and component load), expiration cleanup (writes through removeEntry), the caching source-fill (its own signal-free sourceContext). Anything else armed inside a request that writes through the static API after its client left now gets 499 where it used to write as that request's user.
• The monitor can still split a chain. A disconnect after submission spares the whole chain, but the monitor's own timeout path still aborts an unsubmitted store whose pre-commit work stalled past COMMIT_PHASE_GRACE (~10 min) — deliberately, since that store's source has stalled.
• After a failed final commit, iterators the callback opened but did not return are left to the monitor, which can pin a read snapshot for up to the open-transaction limit — in exchange for not closing an iterator the callback may have handed out (main's lingeringWriteCommit test depends on that).
• @decide hooks still run on an already-aborted request before its write is refused; a decider that ignores the signal can make a paid model call for a dead request. An early check before the hooks is easy to add.
• A WebSocket close aborts writes still in flight on that connection, clean close or not; ws.send(write); ws.close() without waiting for the ack is not durable.
• requestAbortedError is ServerError with a non-standard 499; the class name is visible in logs and metrics.
• setMaxListeners(0) on every request signal also silences leak warnings for application listeners such as generateStream({ signal }); with one listener per write-bearing chain it could be relaxed.
• MQTT is not covered: DurableSubscriptionsSession.createContext() copies no signal.
• MAX_DEFERRED_POISON_TICKS = 2 is a constant, not derived from timeoutBudget.
• A slow post-submit completion can trip the stalled-commit escalation on a write that already landed. nativeCommitSubmitted (and hence deferForCommitInFlight's "outcome unknown" branch) stays true until commit()'s whole returned promise settles — which includes waiting on confirmReplication for a peer that is slow or down (resources/DatabaseTransaction.ts:2914, resources/DatabaseTransaction.ts:2090-2099). If that wait outlives MAX_DEFERRED_POISON_TICKS * txnExpiration, poisonAfterStalledSubmittedCommit() sets timedOut = true on the chain root even though the native write is already durable — and clearAttemptState() never clears timedOut, so a mid-scope explicit commit() that was going to rotate and keep taking writes instead gets a permanent 503 on its next write, for a commit that actually succeeded. Surfaced by round 18's independent review, not reproduced with a test; the fix suggested there is ending the "submitted" state when the native commit itself resolves, before the post-commit completions (confirmation, flush) are awaited — worth a look before merge, but it predates this rebase and is not something this pass changed.
• Default-path cost is unmeasured. Every async commit adds one wrapper promise and two closures; each write-bearing request cycle adds and removes one listener, and a context with no scope pays that per write. Reviewers proposed folding the settle bookkeeping into performCommit; worth a write-throughput benchmark before merge.
• No end-to-end socket test. Every disconnect test drives a synthetic AbortController; a transport that stopped aborting its signal would pass them all.
• Main's read-range guard changes what a late consumer sees. Draining a RocksDB iterator whose snapshot this PR reclaimed now throws ReadSnapshotExpiredError instead of ending early; LMDB still drains to done. Two tests encode that split.
• Size: the review / review bot cannot finish on this diff. On e04ef3a4, attempt 1 hit the workflow's 30-minute timeout and attempt 2 ended Prompt is too long, as on five earlier heads: the 19-thread context snapshot (~33K tokens) sits outside the bot's readable directory, and the session runs out of context. Its last completed verdict, on e2c2107c1, was "no blockers found"; this head's own diff is identical to that one outside resources/DESIGN.md. The check is not required. The submission-boundary and iterator-ownership work is separable in principle, but the cancellation is unsafe without it.

Verification

• test:unit:resources on the pushed head: LMDB 2758 passing / 0 failing; RocksDB 3551 / 0. CI Unit Test passed on Node 22/24/26 and Windows at the previous head; the last commit changes only two monitor log statements. Both engines were also green at each intermediate commit of this round.
• Suites: txn-tracking.test.js Disconnect abort, transactionCommitBoundary.test.js, resumeAfterMidScopeCommit.test.js, Request.test.js and deploymentRecorder.test.js.
• Every new or reversed test was run against the code with its fix removed, and failed: a scope opened on an aborted signal; a synchronously staged write on one; write A rolled back, then a follow-up transaction() on the same request refused; a static-API write after a read-only scope; a retained instance's write after its scope ended; a self-committing write parked in blob pre-commit work, aborted on disconnect; the chain subscribing only while it owns writes; a lingering LMDB write's throwaway owning its cancellation; a real two-database commit landing whole when the disconnect follows the head's native submission, and its LMDB counterpart with the chained store parked in a before hook; the analytics flush running outside the arming request's context (also run in full-suite order, since crud.test.js leaves analytics disabled).
• Tests from main adapted, with the reason in each: @decide's cancellation test (the decider still sees the signal; the write is now refused), two iterator tests (main's read-range guard names an expired snapshot on RocksDB; LMDB still drains to done), and a commit-phase test moved off id 2071, which main's staggered-cascade test now uses on the same table.
• integrationTests/resources/qa716-lingering-write-commit.test.ts (RocksDB) passes locally, 4/4. It landed on main after this PR's old base, and its first CI run here caught that the monitor reclaimed an abandoned iterator by closing it silently; both monitors now log every such reclaim.
• npm run build, lint:required, prettier --check and check:design-docs clean.
• Rebased onto current main (c87f4b174). The 38-commit review history was squashed to one commit before the rebase; conflicts in DatabaseTransaction.ts, LMDBTransaction.ts, Table.ts, Request.ts and DESIGN.md resolved keeping both sides, with main's database-closing assert placed before the native-submission marker.
• Two further mechanical rebases as main kept moving during PR maintenance (0304f2120, then 794cb5dbc). The only real conflict was splicing main's new monitorCommit force-commit-wait feature (a fresh top-level commit() now awaits the monitor's own in-flight force-commit instead of racing it) into this PR's restructured commit()/performCommit(); round 17's independent review confirmed the splice sound by code trace. resources/DESIGN.md also needed pruning back under check:design-docs's 1000-line budget twice, as unrelated main commits kept nudging the same file over — condensed bullets, no invariant dropped, kept the disconnect section as two bullets (not one) per review feedback on scanability.
• Not run: no end-to-end socket test; test:unit:main does not run in this environment, so CI is its gate.

Complexity: complicated

Origin — the dispatch brief this PR was written from

Abort a request-scoped transaction promptly when its client disconnects

Maintain #2047 (Abort a request-scoped transaction promptly when its client disconnects) on branch fix/abort-request-transaction-on-client-disconnect. Read the current PR head, mergeability, and latest check runs before acting; the dispatch observation may be stale. First read the remote head for refs/heads/fix/abort-request-transaction-on-client-disconnect without updating local refs and require it to equal this task's observed head 05b9429; if it differs, stop with needs-input because remote history changed after dispatch. Then fetch only refs/heads/main from origin into refs/remotes/origin/main. If Git reports a paused rebase for this task's exact generation branch, require that rebase's recorded original head to equal 05b9429 and its recorded onto commit to equal the fetched origin/main tip; stop with needs-input on either mismatch. Only then resume it without repeating the local-HEAD ancestry check or starting another rebase. Otherwise record the remote SHA, verify it is an ancestor of local HEAD, and rebase onto the fetched base. If Context carries companion instructions, follow them exactly for every named gitlink; they override ordinary conflict handling. Preserve both sides' intent for every other conflict. Stop with needs-input on semantic conflicts. Force-with-lease is authorized only for this rebase. Immediately before pushing, re-read the PR's base ref and body plus every companion PR head/state named in Context. The base must still be main, and the declarations and submodule bindings must still match Context. If an open companion advanced, update its named gitlink to the new exact head and rerun relevant tests. If it merged, point the named gitlink at the companion repository's current default-branch tip after verifying that tip contains the merge, then rerun relevant tests. If it closed without merging, stop with needs-input and do not publish its abandoned head. If the base, declaration, binding or any other companion state changed ambiguously, stop with needs-input. Then push the rebased head with git push --force-with-lease=refs/heads/fix/abort-request-transaction-on-client-disconnect:EXPECTED_HEAD_SHA origin HEAD:refs/heads/fix/abort-request-transaction-on-client-disconnect, substituting the recorded SHA. If the remote head changes, stop with needs-input; never overwrite intervening remote work. Do this before evaluating CI. Then inspect CI for the resulting current PR head, not old-head failures. Wait for relevant pending checks, diagnose remaining failures, fix them, run relevant tests, and push in THIS task. Do not create a separate CI-fix task. Stop with needs-input for judgment calls or an unavailable CI result; report exactly what was verified. No merge is authorized.

_{Dispatch: task pr-maint-6c2520a0c60c651c74a639056dfc8b35 · queued by automation · ran by claude/sonnet/xhigh · worker kzyp-xps-1}

_{Review-Coverage: authored=claude; ran=gemini,codex,cursor-kimi,cursor-composer; adjudicated=domain; declined=cursor-grok,cursor-muse; rounds=18; full=3 @ e04ef3a}

_{Human-Review-Need: 4 (decisions: disconnect-spares-whole-submitted-chain, signal-is-request-lifetime, request-aborted-error-class, stalled-submit-poisons-follow-up, mqtt-out-of-scope) @ e04ef3a}

[gemini-code-assist]

Code Review

This pull request introduces immediate transaction aborts upon client disconnect to prevent orphaned write-bearing transactions from holding native write intents. It adds a disconnected poison flag to DatabaseTransaction, registers an 'abort' listener on context.signal during transaction execution, propagates poison flags across multi-store transaction chains, and wires up real abort signals for uWS WebSocket connections. Comprehensive unit tests have been added to verify these behaviors. I have no feedback to provide as there are no review comments.

[claude]

Reviewed; no blockers found.

[cb1kenobi]

Reviewed e4846cef — no open issues. This PR looks good, nice job!

Finding 6 is fixed, not merely mitigated — and this time I could prove it. The last round could only confirm it structurally: the forced-failure probe never reached the patched native commit(), so the recommendation was to rescore it Low. This push makes a real forced failure reachable, so here is the measurement instead.

The root cause is in utility/when.ts:4-13: when(value, callback, reject) only wires reject when value is a promise, and even then it guards value's rejection, not the promise the callback returns. With completions empty, value is null, so callback() runs synchronously and its rejected promise is handed back untouched — abortAfterCommitError was unreachable. The new try/catch plus the explicit .catch() on the returned chain covers both the synchronous throw and the returned rejection, and still covers a Promise.all(completions) rejection as before.

I instrumented abortAfterCommitError with a marker, rebuilt dist/ each time (one isolated match), and drove the new test's forced ERR_CORRUPTION rejection through the stubbed native Transaction.prototype.commit:

	reaches abortAfterCommitError	chained store open	test
5eba8aee (prior head, same test copied in)	0	OPEN	fails
e4846cef (this head)	1	CLOSED	passes

At head the chained store also discards its staged writes and neither store's row is visible afterwards. abortAfterCommitError is typed : never and rethrows, so commit() still rejects — the .catch() does not swallow the failure.

The test earns its keep: dropping just the returned-chain catch (return commitResult in place of return commitResult.catch(...), rebuilt, one isolated marker in dist/) takes the file from 16 passing / 0 failing to 15 passing / 1 failing, and the failure is precisely "the failed commit must abort its chained store".

Double-abort was the thing I most wanted to rule out, since the inner failure branch and the new outer handler can both fire. It is safe: releaseReadTxn() goes through detachOwnedTransaction() so the handle is nulled before a second pass, drainCompletions() early-returns on an empty list, and abort() ends with clearWrites(). No path double-frees a native handle.

Everything fixed in the previous round stayed fixed — the delta is 1 commit and 2 files, and git diff confirms nothing outside performCommit's tail moved. Spot-checked anyway: the getReadTxn bootstrap block is still gone (debugLongTransactionsReads.test.js 4 passing on both engines at head and at base 29072607), the LMDB monitor's CLOSED branch is intact at LMDBTransaction.ts:431, and transaction.ts:147 still passes abort(true) alongside closeOwnedReadIterators().

Full unitTests/resources against the true merge base, both engines: RocksDB 1684 passing / 0 failing at head vs 1658 / 0 at base; LMDB 1451 / 0 vs 1425 / 0. No failures on either side, and neither known flake band came up this run. prettier --check, oxlint, and tsc --noEmit are clean on both changed files.

One CI note: Integration Tests 2/6 (Windows, Node.js v24) is red, but it fails on readiness timeouts (Probe /SeoPageCache/ did not become ready within 120000ms, ECONNREFUSED after restart_service), it also failed at the previous head 5eba8aee, and the same shard is failing on main in four of the last eight commits. Not attributable to this change.

—
Generated by Barber AI

[cb1kenobi]

On LMDB, a disconnect after the head store has submitted still aborts a chained store waiting in before-hooks, which splits a multi-store commit. Guard those resume throws with poisonedMidCommit the same way RocksDB does, so the unsubmitted store rides the cascade.

—
Reviewed 613785c

[cb1kenobi]

The latest commit only changes how the long-transaction monitors log reclaiming abandoned iterators, so every snapshot release is visible. No new blocking defects showed up on this head. The already-raised LMDB pre-commit resume still throws on disconnect without a poisonedMidCommit guard; wrap those throws like RocksDB so a disconnect after the head submits cannot split a multi-store commit.

—
Reviewed 732a3d3

[kriszyp]

@cb1kenobi re: LMDB pre-commit resume throws on disconnect

Good catch — confirmed and fixed in 05b9429. The whole-chain spare set poisonedMidCommit on the LMDB child, and the cascade admitted its continuation, but the resume after its before hooks still threw on disconnected, so the head landed and the child was discarded. The resume now skips those two throws under poisonedMidCommit, matching DatabaseTransaction.performCommit; a disconnect before any store submits still throws there.

New regression in transactionCommitBoundary.test.js ("lets an LMDB child parked in pre-commit work land when a disconnect follows the head submission") drives exactly your sequence and failed with Transaction was aborted because the client disconnected before the fix. I also checked the other poison throw sites in both engines: they are fresh-write admission or already guarded. Full test:unit:resources is green on both engines.

— Claude Opus 5.5

[cb1kenobi]

The LMDB pre-commit resume now honors the chain-wide submission boundary and avoids splitting the commit after a disconnect. The regression test covers the previously reported failure. No new blocking defects were found on changed lines.

—
Reviewed 05b9429

[cb1kenobi]

The last blocking split is closed: after a disconnect, an LMDB child parked in before-hooks now rides the cascade once any store has submitted, instead of throwing 499 and dropping its writes. A regression test covers that sequence. Nothing else on the changed lines is a confirmed remaining blocker.

—
Reviewed 05b9429

[cb1kenobi]

The new commit only repairs a missing word in the disconnect-cancellation design note. It introduces no confirmed blocking issue on a changed line.

—
Reviewed 5137f20

[cb1kenobi]

The latest commit only condenses the design notes to satisfy their line budget. No confirmed blocking defect was introduced on changed lines.

—
Reviewed e2c2107

[cb1kenobi]

The latest changes only condense existing design documentation. No confirmed blocking defect remains on changed lines.

—
Reviewed e04ef3a