Stop reads from extending the open-transaction limit while uncommitted writes are held

[kriszyp]

Why

getReadTxn() re-armed this.timeout = txnExpiration on every read. A handler that keeps reading while holding staged writes therefore resets its own clock forever and is immortal to the #1407/#1411 long-transaction monitor — while those staged writes hold verification-table write intents that other writers' coordinatedRetry commits park on.

That is the mechanism behind the remaining half of harper#2001: an orphaned handler whose client had already disconnected (the pre-dispatch#12 heartbeat: stage lastSeen, then scan queues on every tick of a 25s long-poll) held its intents for hours while the monitor never fired. Proven interventionally on a test cluster — a handler staging one write and then sitting idle for 90s was aborted at the limit, while the same handler reading every 2s held its intents for 120s (4× the limit) and committed successfully with zero monitor engagement.

What

The limit becomes a true idle limit:

• writes always re-arm it (addWrite) — a transaction that keeps writing stays alive as long as work keeps coming, which is the behavior a long-running write job needs;
• reads re-arm it only while no uncommitted writes are held (getReadTxn) — so a handler that wrote once and then only reads cannot keep its write intents alive by reading;
• a multi-store chain counts as active while any link has been written recently — a head that only reads database A is not aborted out from under continuing writes to database B.

The harper#2001 orphan is still reaped: it stages lastSeen once and thereafter only reads, so nothing re-arms it.

Deliberately narrow:

• Committed transactions still re-arm. Their intents went with the commit, and the monitor already bounds a retained read snapshot separately via releaseReadTxn(). (this.writes is not cleared on commit, so gating on hasPendingWrites() alone would have changed behavior for committed-then-streaming transactions that hold no intents at all.)
• Read-only transactions still re-arm. A long search()/export holds no intents and no one parks on it; the regression pair pins this half explicitly.
• The chain is consulted, not just the head — a transaction that reads database A and writes database B holds the write on next.

Hot-path note: the dominant case (single-store, never written) short-circuits on two field reads before hasPendingWrites().

Relationship to the other #2001 work

Complementary, not overlapping — this bounds any write-holding transaction that outlives the limit, including orphans with no disconnect signal (hung upstream, keep-alive socket that never closes):

• Abort a request-scoped transaction promptly when its client disconnects #2047 (client-disconnect abort) kills the holder at the moment of disconnect — the common case, and the upstream leak source. Touches the same file but not this line; expect a trivial conflict hunk depending on merge order.
• Release the retained read handle's write intents when an outstanding-iterator commit replays #2050 + rocksdb-js#747 (abandonWrites) release the intents a committed transaction retains for its outstanding iterators — a different holder class this change does not touch.

Testing

unitTests/resources/txn-tracking.test.js, four arms at identical duration and cadence, which is what makes them a discrimination rather than a single assertion:

• write-then-only-reads → aborted with the open-transaction error, write rolled back;
• continuous writer → survives well past the limit and commits normally;
• read-only reader → still lives;
• direct-construction chain check: a read on a head whose next holds the writes does not re-arm, with a control proving it re-arms again once the chain is drained.

Two arms fail on origin/main; the two "still alive" arms pass on both, which is the guard against this change simply shortening every transaction's life.

integrationTests/database/longtxn-secondary-index.test.ts (QA-176) is unmodified and passes: Q1 drives 8 indexed writes 350ms apart against a 1s limit and still completes 200 with a consistent index, because writes re-arm. RocksDB-only: LMDBTransaction.getReadTxn() re-arms unconditionally too, but that engine has no verification-table park to wedge and is deprecated — flagging the asymmetry rather than changing a deprecated engine's timeout semantics without a driving case.

test:unit:resources: 1354 passing. test:unit:main could not be run locally (shared ~/harper/database LOCK across worktrees) — relying on CI.

Review coverage

Degraded, disclosed per the cross-model rule: Gemini contributed findings; the Codex leg exited rc=-1 after producing findings (recovered from its log and addressed); grok was version-gated; the same-family fallback failed auth. So one clean outside lens plus a partial second, not the usual two.

Findings addressed: the new assertions ran unconditionally in the LMDB suite (Codex, real — guarded); no multi-store coverage (Gemini — added, and it fails on base); hasPendingWrites() on the read hot path (both — fast-pathed). Gemini also raised a blocker that hasPendingWrites() might only check the head — dismissed: it walks the next chain, and the new next-chain test pins that.

Generated with Claude Fable 5.

[gemini-code-assist]

Code Review

This pull request modifies 'DatabaseTransaction.ts' to prevent read operations from extending the transaction timeout limit once uncommitted writes exist. This ensures that write intents are not held indefinitely by ongoing reads. Additionally, three new unit tests are added to 'txn-tracking.test.js' to verify this behavior under various conditions, skipping them when the LMDB storage engine is used. There are no review comments, so I have no feedback to provide.

[claude]

Found a blocker in the latest push: chainStillActive (resources/DatabaseTransaction.ts:991-1002) uses the shared timeout field to detect chain activity, but that field is also re-armed by unrelated reads on a write-free link — so a write on one store followed by looping reads on a second store re-arms the write-holding link forever, reintroducing the exact bug this PR fixes. See inline comment for details.

[kriszyp]

Guided tour

The production change is one condition. Most of the diff is tests, and one of them is a correction to an existing anchor.

The change — resources/DatabaseTransaction.ts, getReadTxn()

if ((this.writes.length === 0 && !this.next) || this.open !== TRANSACTION_STATE.OPEN || !this.hasPendingWrites()) {
    this.timeout = txnExpiration;
}

Previously this line was unconditional, so every read reset the clock. Read the three disjuncts as "re-arm when it is safe to":

1. writes.length === 0 && !this.next — the hot path. A single-store transaction that has never written short-circuits on two field reads, before the hasPendingWrites() chain walk. Both review legs flagged the walk on a read path; this is the answer.
2. open !== OPEN — a committed transaction still re-arms. This one is easy to get wrong: this.writes is never cleared on commit, so gating on hasPendingWrites() alone would also stop re-arming committed transactions that are still streaming iterators — which hold no intents at all, and whose snapshots the monitor already bounds separately via releaseReadTxn(). Without this clause the change would start tearing snapshots out from under long streams.
3. !hasPendingWrites() — the actual rule, and it walks the next chain, so a transaction that reads database A and writes database B counts as write-bearing.

The tests — unitTests/resources/txn-tracking.test.js

Three arms at identical duration and read cadence, which is the point: a write-holding reader is aborted, a read-only reader at the same cadence still lives. Without the control arm this change is indistinguishable from "shorten everything."

The chain-walk arm builds its DatabaseTransaction links directly instead of going through the resource API. That is not laziness — chasing a reviewer's finding revealed that the next chain is per-database, and a second database: in this suite resolves to the same store, so no next link forms through tables at all. The pre-existing "multi-store" test in this file has the same gap; its comment claims two databases and it does not get one. Worth a separate look.

The anchor correction — integrationTests/database/longtxn-secondary-index.test.ts

QA-176 Q1 asserted status === 200. That assertion encoded exactly the behavior being removed: the transaction the test was written to drive the monitor against was immortal because it kept reading, so the monitor never fired on it — the test never exercised its own premise. It now follows Q2's existing convention in the same file (assert index consistency, tolerate either status). Under the change the request is aborted and the index comes back phantom=0 missing=0, so the invariant QA-176 exists to protect holds.

The behavior change, stated plainly

A transaction holding uncommitted writes is now capped at maxTransactionOpenTime (default 30s) regardless of read activity. Previously reads extended it without limit. This is #1407's stated policy — "the app owns long-running work; core owns consistency" — applied to a case that was escaping it, but it is a user-visible change: a request that writes and then keeps reading past 30s now gets a 422 where it used to succeed. Q1 was one such case.

Where to look hardest

• Whether that cap is acceptable for real long-running write+read jobs, or wants a larger separate budget.
• Clause 2 above — the committed-transaction carve-out.

Review coverage

Degraded, and disclosed: gemini contributed; the codex leg exited rc=-1 after writing its findings (recovered from its log and addressed — the LMDB-guard finding was real and would have broken CI); grok was version-gated; the same-family fallback failed auth. The CI bots then caught the chain-walk test flaw on the pushed branch.

Posted by Claude Fable 5 on behalf of @kriszyp.

[kriszyp]

Correction: the limit is now an idle limit (writes re-arm it)

Kris caught a real defect in the earlier revision, and it changes the semantics enough to be worth calling out separately from the tour above.

What was wrong. The first shape capped any write-holding transaction at maxTransactionOpenTime regardless of activity. That aborts work in progress: a job that keeps writing for longer than the limit would be killed mid-flight, which is not what the limit is for.

The tell was in the diff. Making that version green required editing QA-176's Q1 anchor (8 indexed writes 350ms apart against a 1s limit) from "expect 200" to "tolerate either status". Rewriting an existing anchor to match new behavior is exactly the signal to re-examine the change rather than the test. That edit is now reverted, and Q1 passes unmodified — status 200, index consistent — because writes re-arm.

The rule now:

activity	re-arms?
a write	always
a read, no uncommitted writes held	yes
a read, uncommitted writes held	no
a write to any link of a multi-store chain	keeps the whole chain alive

So: continuous writers live, long read-only scans live, and the harper#2001 shape — wrote once, then only reads while the client is gone — is reaped at 30s from its last write. That last row is the entire behavior change.

The multi-store clause matters because writes re-arm only the link that receives them; without it a head that only reads database A would be aborted out from under continuing writes to database B (chainStillActive).

Tests are now a four-way discrimination at identical duration and cadence: write-then-idle-reads → aborted; continuous writer → survives and commits; read-only → survives; plus the direct-construction chain check. Two arms fail on origin/main; the two "still alive" arms pass on both, which is what stops this from being a change that merely shortens everything's life.

Posted by Claude Fable 5 on behalf of @kriszyp.

[claude]

chainStillActive masks read-only activity as write activity, reintroducing the exact immortal-transaction bug on a 2-link chain

File: resources/DatabaseTransaction.ts:991-1002 (used at line 1022)

What: chainStillActive treats any link with timeout > 0 as evidence the chain is "still active" from writes, but timeout is also reset by reads on a link that itself holds no pending writes and has no .next — the fast path at line 293 (this.writes.length === 0 && !this.next) re-arms unconditionally on every read of that link, regardless of activity elsewhere in the chain.

Concretely, for a 2-link chain — write on table A (head), then repeated reads on table B (.next, created once via txnForContext in Table.ts:5505 and reused on every subsequent touch of B) — B's own getReadTxn() hits the fast path and re-arms B on every read. When A's own timeout decays to ≤0, the monitor's hasPendingWrites() is true (A's own write) and chainStillActive(A) walks to B and sees B.timeout > 0 (kept alive purely by B's unrelated reads) — so A gets re-armed forever (line 1027), even though nothing is writing to A and B holds no pending write at all.

This is exactly the harper#2001 shape the PR sets out to fix — "stage a write, then loop reading" — just shifted from one store to two. The PR body states the intent as "active while any link has been written recently," but the implementation checks generic timeout > 0, which doesn't distinguish a recent write from a recent unrelated read.

Why it matters: for any transaction that writes one store and then continues reading a second store in a loop (a very plausible long-poll/orphan shape, and the literal inverse of the multi-store test order this PR added), the write-holding link never expires — the idle-limit fix this PR implements is defeated for the multi-store case it explicitly claims to handle. There is no test exercising chainStillActive at all: the existing "multi-store path" test doesn't form a real .next link (both tables resolve to the same database, as the code comment at line ~90 of the test file now acknowledges), and the new direct-construction test only exercises getReadTxn()/hasPendingWrites(), never the monitor's chainStillActive branch.

Suggested fix: track write recency separately from read recency (e.g., a lastWriteTimeout set only in addWrite, checked by chainStillActive instead of the shared timeout field), and add a direct-construction test that pins the head-writes/tail-reads-only ordering to confirm the head is not kept alive by the tail's unrelated reads.

[cb1kenobi]

Medium: chainStillActive can keep an idle write-holding chain immortal, reintroducing the harper#2001 leak in the multi-store blind-write shape.

this.timeout decays only for transactions in trackedTxns, and a txn is added there only by getReadTxn (line 319). A next link that receives writes but is never read is therefore never tracked, so its timeout is armed once by addWrite and never decays. chainStillActive treats link.timeout > 0 as "written recently," but for such a link it is permanently positive — so this branch re-arms the head on every tick and the monitor never reaps it.

This is reachable with the common read-one-DB / blind-write-another pattern: the head does A.get(id) (tracked, decays), then B.put(newId, …) stages via addWrite and resolves the existing entry at commit through the write txn — it never calls getReadTxn on B's link, so B is untracked. If the handler then goes idle while holding B's uncommitted write intents (the harper#2001 orphaned long-poll), the head is re-armed forever and B's intents are held indefinitely, so other writers' coordinated-retry commits park on them. Pre-PR that same idle transaction hit the abort branch and was reaped, so this is a regression inside the PR's own target scenario.

Suggested fix: ensure downstream write-holding links actually decay. Simplest option is to have the monitor decay the links chainStillActive inspects that aren't independently tracked (e.g. if (!trackedTxns.has(link)) link.timeout -= txnExpiration; before the > 0 test), so a written-once idle link eventually reaches 0 and the head is reaped. The direct-construction chain test sets timeout manually and does not exercise monitor decay, so a regression test driving a real read-A/blind-write-B txn to idle would pin this.

—
Generated by Barber AI