Track every outstanding write commit, not just one per thread

[kriszyp]

Follow-up to the harper#2001 investigation, and the "arming-race gap" that PR #2007 documents in-code but deliberately does not fix. Independent of #2007 — this branch is cut from main and the two overlap textually at the arming site, so whichever lands second rebases.

What was wrong

checkOverloaded() rejects new application writes with a 503 once a commit has been outstanding past maxTransactionQueueTime (45s default). It is the only protection against a native rocksdb-js commit promise that never settles, which is the harper#2001 symptom: a worker thread that 503s every write until restarted.

It watched one module-level slot per worker thread, armed by whichever commit found it free:

if (!outstandingCommit) { outstandingCommit = commitResolution; ... }

Two consequences, both meaning a wedge could be entirely invisible:

1. Concurrent commits were sampled, not tracked. Any commit submitted while another held the slot was never watched, and got no second chance when the holder settled. If one of those wedged, no 503 was ever raised, the write queue grew unbounded, and nothing was logged.
2. Retries and chained commits could never arm. A coordinated retry round (this.commit({...options, transaction})) and a chained second database's commit (this.next.commit(...)) are both issued from inside the preceding commit's resolve handler. That handler is a direct reaction on commitResolution; the slot was released by a .catch(() => {}).finally(...) chain hanging two further reactions off it. Reactions run in registration order, so the resolve handler always ran first and observed the slot still held by the already-settled promise.

Confirmed by reducing the promise topology to a standalone repro:

ARM
REENTER (outstandingCommit is set -> NOT armed)
CLEAR

The change

Track every submitted commit in a linked list ordered oldest-first; checkOverloaded() tests the head. That removes the single-slot condition rather than reordering around it — with unconditional tracking the microtask ordering stops mattering at all, so the fix does not depend on it.

A linked list rather than a Set because the write path reads the oldest on every write and must not allocate an iterator to do it, and because commits settle out of order and have to unlink in constant time.

Each attempt is timed from its own submission, so the overload window stays per-attempt rather than accumulating across a retry ladder — which is the semantics the recordCommitLatency comment already documented and the old code failed to deliver.

Also included: the outstanding-iterators replay commit now counts against the write-queue depth. It is a real native commit, and omitting it left write-transaction-queue-depth — per the harper#2001 investigation, the only metric that can observe a wedge — reading zero for exactly that path.

Behavior change worth a reviewer's attention

Strictly more commits are now evaluated against the threshold. Under a genuine stall lasting >45s, a worker will now shed application writes reliably where before it did so only if the stalled commit happened to be the sampled one. That is the documented intent of maxTransactionQueueTime ("Max write queue time before rejecting"), but it is a real change in when 503s appear, and it couples across databases on a thread. Commits under the threshold are unaffected.

Source-applied writes are unaffected: both checkOverloaded() call sites are gated on !context.source, so replication and caching-source applies never reach it and their uncapped retry policy is untouched.

Review

Two Codex passes (design, then final artifact). The design pass corrected my characterization of the retry ladder — only retries 1–2 re-enter immediately and hit the race; retry 3+ goes through delay() and already armed. The artifact pass found no defect in the list itself (double-untrack, out-of-order settlement, already-settled and double-tracked promises all check out) and confirmed the enter/leave accounting is balanced, but caught that my "multi-table chained" test was not chained — chaining forms only across databases, and only when a read initializes the head first. Verified directly and rewritten; the test now asserts context.transaction.next exists so it cannot silently stop covering that path.

Gemini's leg was unavailable this run (socket error), so outside coverage here is Codex-only.

Tests

unitTests/resources/outstandingCommitTracking.test.js — 8 tests. The concurrency test was verified to fail against the old semantics (tracked 1, expected 8) before being kept. Coverage includes head/middle/tail unlink in controlled order, rejection, already-settled and double-tracked promises, and the real cross-database chain.

test:unit:resources 1342 passing, test:unit:main 3966 passing, test:integration:all 1619 tests / 1593 pass / 0 fail, oxlint and prettier clean.

Known gaps, not addressed here

• The retry backoff window is still unmonitored. Retry 3+ waits in delay(...) with nothing outstanding, so a lost continuation there (the harper#1785 shape) is invisible to this mechanism. Needs its own treatment.
• resources/LMDBTransaction.ts has no equivalent protection at all — its own separate sentinel, no start stamp, no checkOverloaded(). Pre-existing; LMDB is deprecated.
• A pre-existing test in unitTests/resources/txn-tracking.test.js looks mis-scoped. Its fixture comment claims two tables in database: 'test' "span two databases (writes to the second live on the transaction's next chain)", and the test at :131 is titled "aborts a multi-store txn whose write lives on the next chain". Same-database tables share one transaction — I measured it — so that test appears not to exercise the next chain it names. Not touched here; flagging for whoever owns it.

None of this root-causes why a native commit promise stops settling. That still needs evidence from a live occurrence.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request replaces the single-slot outstanding commit tracking with a linked-list-based tracking mechanism (OutstandingCommit) in DatabaseTransaction.ts to properly track concurrent commits and detect unsettled promises. It also adds comprehensive unit tests to verify various commit tracking and untracking scenarios. The review feedback suggests adding defensive checks to ensure commitResolution is a valid thenable before tracking or calling .then() on it, and introducing a guard flag to prevent double-untracking of commits.

[claude]

Reviewed; no blockers found.

[kriszyp]

Replying to @kriszyp's comment on resources/DatabaseTransaction.ts about consolidating trackOutstandingCommit's list with the writeTxnQueueDepth lifecycle (couldn't post as a threaded reply — a pending review on the PR is blocking the GitHub API's reply endpoint):

Consolidated it — trackOutstandingCommit() now owns both the linked list and the write-queue high-water mark directly, and enterWriteQueue/leaveWriteQueue plus the separate writeTxnQueueDepth counter are gone. getTransactionQueueDepths() derives writeDepth straight from outstandingCommitCount.

Codex's independent pre-push review flagged the exact same duplication before I'd pushed it, with a concrete argument for why it mattered beyond code cleanliness: the replay-path write-queue omission this PR fixes was an instance of the two mechanisms drifting.

— Claude (Sonnet 5)