Abort over-time write transactions instead of force-committing (#1407)

[kriszyp]

Fixes #1407.

Summary

The long-transaction monitor force-commit()d a still-open, in-use transaction and reused it. That (a) committed a partial write set the application never asked to commit (atomicity violation) and (b) could desync secondary-index maintenance from the on-disk record, leaving orphaned index entries that only a full index rebuild repairs. Observed in production on v4.7.19; the same force-commit-and-reuse design is present in v5.

This makes the monitor abort and poison a write-bearing transaction that exceeds the open-transaction limit instead of force-committing it. Once poisoned (timedOut), any subsequent addWrite/commit throws an actionable transactionOpenTooLongError (503), so the request rolls back cleanly and the application owns how it splits long-running work. Core keeps the consistency guarantee; it no longer silently mutates committed state to paper over app-side slowness.

Per the issue's "design recommendation", this is the abort-and-surface option (confirmed with the team), applied symmetrically to both transaction engines: the RocksDB DatabaseTransaction monitor and the LMDBTransaction monitor.

What changed

• DatabaseTransaction / LMDBTransaction monitors: a timed-out write-bearing txn is aborted via abortDueToTimeout() (sets timedOut, forces open = CLOSED before releasing the read snapshot so the discard path is taken, then abort()). Guards in addWrite/commit throw transactionOpenTooLongError once poisoned.
• Read-only long transactions keep their prior behavior (no atomicity/index risk — e.g. a large scan/export), so ongoing reads aren't poisoned.
• Multi-store: hasPendingWrites() walks the next chain, so a transaction that reads database A (head, no writes of its own) and writes database B (next) is correctly treated as write-bearing rather than misclassified read-only and force-committed. abortDueToTimeout() poisons + aborts the whole chain.
• Canonical-source apply / replay excluded: sourceApply and isReplay transactions keep the prior force-commit behavior — they have no resubscribe/resume path, so aborting a write would drop it while the resume cursor advances past it (a harper-pro#348-class permanent divergence).
• Tests (unitTests/resources/txn-tracking.test.js): write-bearing abort leaves no record/orphaned index entry; multi-store write on the next chain is not force-committed; source-apply txn is preserved. Pass on both RocksDB and LMDB.

Where to look / open items

• transactionOpenTooLongError status code — now 422 (was 503; cross-model review and the Gemini bot both flagged 503 as inviting auto-retry of the same doomed transaction). 422 signals the request itself must change. Swap to 408/400 if you'd prefer.
• In-flight-commit race (low severity, not fixed here). If the monitor aborts a write txn whose async commit() is already in flight (awaiting before hooks), the resumed continuation does a second --readTxnsUsed (→ -1) and a double transaction.abort() that the pre-existing try/catch swallows. Cross-model review confirmed the data is still correctly rolled back and the request errors out — the only artifact is an inert negative counter on a dead txn object. I left the hot commit-continuation untouched rather than add a guard under a race that's hard to unit-test; flagging it for a reviewer's call.

Reviewed cross-model (Gemini + Harper-domain deep-review); the two findings above are the only items not fully resolved in the diff. Generated with Claude Opus 4.8 (1M context).

[gemini-code-assist]

Code Review

This pull request addresses issue #1407 by aborting and poisoning write-bearing transactions that exceed the open-transaction limit, rather than silently force-committing them. This change preserves atomicity and prevents secondary-index corruption. Key updates include introducing a timedOut state, throwing a transactionOpenTooLongError on subsequent writes or commits, and adding robust unit tests. Feedback on these changes suggests wrapping txn.commit() in LMDBTransaction.ts with proper error handling to avoid unhandled rejections, refactoring abortDueToTimeout to ensure all transactions in a chain are poisoned even if an individual abort throws an error, and changing the HTTP status code for transaction timeouts from 503 to 408 to discourage inappropriate client retries.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[claude]

Reviewed; no blockers found.

[cb1kenobi]

This is weird ergonomics to throw a function. Instead I would recommend declaring a TransactionOpenTooLongError class that extends ServerError, then throw a new instance.

[kriszyp]

Agreed — the factory-function pattern is unusual. Will refactor to a TransactionOpenTooLongError class extending ServerError, then throw new TransactionOpenTooLongError() at the call sites in a follow-up commit.

— KrAIs (claude-sonnet-4-6)

[cb1kenobi]

Why the second loop? Could the first loop do the abort?

[kriszyp]

The separation is intentional: the first loop poisons every link in the chain (timedOut=true, open=CLOSED) before any abort() call. If abort() throws at link N in a combined loop, links N+1…M remain un-poisoned — their timedOut is still false, making them eligible to force-commit on the next monitor pass. Splitting the loops guarantees the poison-all-first invariant even if any abort() throws.

— KrAIs (claude-sonnet-4-6)

[kriszyp]

Flagging a reproduction this PR should resolve. In an exploratory pass (qa-explorer, main 28db4fde4) we hit the silent-partial-drop variant of #1407 from the custom-resource handler side:

• A custom resource handler whose transaction exceeds storage.maxTransactionOpenTime (default 30s) gets force-committed mid-handler by the long-transaction monitor. Writes made before the fire point are silently lost; writes after it land on a fresh implicit txn and survive; the request still returns HTTP 200 ("count: 6" but only 1/6 rows durable, reproduced 2/2).
• Narrow trigger: write(s) → idle async suspend with no intervening write (a bare await/slow external I/O > maxTransactionOpenTime) → write(s). Bulk writes do not trigger it (each put() resets txn.timeout at DatabaseTransaction.ts:115). Natural shape: a handler that writes a pending row, awaits a slow external API (>30s), then writes the result — the pre-await row is the casualty.

This PR's abort-instead-of-force-commit is exactly the right direction (the client should get an error / the writes should roll back atomically, rather than a silent 200 + partial drop). One thing worth confirming before merge: that the abort path covers the custom-resource handler force-commit (not just the replication/apply path), and that the caller receives a non-2xx. Repro: integrationTests/qa-scratch/qa314-overtime-atomicity.test.ts (happy to port it into a regression test for this PR).

— filed by KrAIs (Claude) on Kris's behalf

[kriszyp]

Follow-up on the custom-resource-handler repro flagged above — the promised regression test has landed: integrationTests/resources/txn-overtime-atomicity.test.ts (+ fixture in the sibling txn-overtime-atomicity/ dir), pushed to this branch.

Confirmed: the abort-and-poison fix covers the custom-resource-handler path, not just replication/apply. The test drives a custom Resource whose handler writes a row, awaits past storage.maxTransactionOpenTime with no intervening write (the exact narrow trigger described above), then writes a second row:

• On this branch: the request returns 422 and neither the pre-await nor the post-await row is durably committed — atomic rollback, no partial drop.
• On main (pre-Force-committing an over-time transaction leaves orphaned secondary-index entries (atomicity violation) #1407 baseline, checked against the same test): the request returns 200 and both rows land (the force-commit + fresh-implicit-txn split), confirming the test is a real regression guard and not a tautology.

Also ran the PR's own unitTests/resources/txn-tracking.test.js and the full test:unit:resources suite (852 passing) against this branch — no regressions.

One incidental note, unrelated to this PR's diff: npm run build on this branch currently fails tsc type-checking at resources/DatabaseTransaction.ts:305 (Promise<number | void> not assignable to MaybePromise<void>) because the branch predates a rocksdb-js@2.2.0 type-widening compat cast that's already on main (transaction.commit() as Promise<void>). Doesn't block dist emission (noEmitOnError is off) or this test, but worth a rebase before merge to pick up a clean tsc pass.

— filed by KrAIs (Claude) on Kris's behalf