fix(txn): make writes after a mid-scope commit atomic with the scope that owns them

[kriszyp]

Closes #2238. Stacked on #2230 (base is kris/txn-after-commit); retarget to main once that merges. Also depends on #2232 — see below.

Targeted at 5.2.x, not 5.3. This is not a new behavior: harperdb 4.7.36 ships lmdb and no RocksDB dependency at all, and on LMDB post-commit writes have always staged and rolled back with the scope (8 of the 12 unit cases here pass on LMDB unchanged, including both two-database rollback cases). So the 4.7 → 5.x upgrade, which moves a database onto RocksDB, silently dropped an atomicity property — and that drop is the mechanism behind a failed cluster delete leaving a cluster marked TERMINATED with its instances still RUNNING. Shipping this only in 5.3 would leave every 4.7 → 5.2.x upgrade carrying the regression for that line's whole life.

A handler that commits its own transaction mid-scope — the documented await getContext().transaction.commit() — got non-atomic writes for the rest of that scope. Each later write was serviced by its own transaction() scope and committed immediately, so a handler that failed halfway left the earlier half durable and unrollbackable. That is the mechanism behind a failed cluster delete leaving a cluster marked TERMINATED with its instances still RUNNING.

open was standing in for two different things: whether a physical transaction generation is open, and whether some scope is still responsible for committing it. An explicit commit closes the generation while the scope keeps running, and the dispatchers read open for the second question.

So a successful commit that is not the scope's final one now rotates the owning transaction to a fresh OPEN generation. Every dispatch path keeps its plain open === OPEN check — CLOSED never gains a second meaning and the default path is untouched. The new generation is snapshot-free, because committing mid-scope is how a handler asks to stop reading a pinned snapshot.

This is not a new behavior so much as the end of an engine divergence. LMDB has always done it (LMDBTransaction.commit sets open = OPEN on any non-final commit); only the RocksDB path committed each later write on its own. 8 of the 12 unit cases pass on LMDB unchanged, including every atomicity case.

For the human reviewer

The step-6 framing verdict was better-alternative-exists, and I adopted it. My spike taught the two dispatch sites a "resumable-CLOSED" state; the planning review argued for rotating to a real OPEN generation instead, which keeps the hot paths unchanged, makes txnForContext create later chain links OPEN for free, and separates a retained iterator's old handle from the new generation. It was right on all three. The implementation here is the reviewer's design, not mine.

1. Post-commit writes stage instead of committing one at a time. await Table.put(x, context) after a mid-scope commit resolves staged, not durable. Consistent with every write before the commit in the same scope, and with LMDB — but observable, and it means a single mid-scope commit no longer bounds how much a long handler holds in memory. A handler streaming a large volume should keep checkpointing; the checkpoint-loop case is tested.

2. The rotated generation reads snapshot-free for the rest of the scope. Required, not incidental: with a snapshot, a search() after a mid-scope commit stopped seeing another writer's row with no intervening write at all — I measured that against base before changing it. The cost is no repeatable read after a mid-scope commit, which is what committing mid-scope asks for. Carried concern I could not fully close: review argued this widens the lost-update window for a post-commit read-modify-write from one write to the rest of the handler. I measured both arms — snapshot-bearing and snapshot-free — in a read-modify-write race against a concurrent writer and they behave identically, so the concern does not distinguish them in that shape. It is one shape, not a proof.

3. Ownership is a constructor flag on a #private field. It went WeakSet → static setter → constructor option across rounds, each time because the previous shape was reachable from application code. The constructor option is still callable by anything that can import the class (it is not a package export), so what it actually guarantees is that no accidental path turns it on for a running transaction. I stopped there rather than iterate again.

4. Two carve-outs where the guarantee deliberately does not apply, both tested and stated on Context.transaction: a commit made while a read iterator still holds the native handle (that handle is the iterator's until it drains), and a commit that failed — a generation whose durability is unknown must never be followed by a segment that commits as though it landed.

5. The monitor gap is closed by Correct blind-write transaction bookkeeping and bring those transactions under the long-transaction monitor #2232, not here — and that is what makes 5.2.x defensible. A rotated generation whose handle comes from save() would be outside trackedTxns, so the long-transaction monitor could not reap it while it holds write intents (the harper#2001 shape). That was my one objection to a patch release. Correct blind-write transaction bookkeeping and bring those transactions under the long-transaction monitor #2232 already solves it, and properly: a separate supervisedWriteRoots set keyed on the chain root, plus an attachOwnedTransaction() helper that seeds the per-handle bookkeeping in save()'s adopt branch. I verified the stack rather than assuming: with origin/main and Correct blind-write transaction bookkeeping and bring those transactions under the long-transaction monitor #2232's branch both merged in, this branch's suites pass (resume 12/12, transaction 48/48, lingeringWriteCommit 3/3).

   • When rebasing onto Correct blind-write transaction bookkeeping and bring those transactions under the long-transaction monitor #2232, drop the doneReadTxn() guard added here. It enforces "never abort a handle holding writes still awaiting a commit" via a state check; Correct blind-write transaction bookkeeping and bring those transactions under the long-transaction monitor #2232 enforces the same invariant at the source by seeding the base reference, which is the better fix. Verified: removing the guard with Correct blind-write transaction bookkeeping and bring those transactions under the long-transaction monitor #2232 merged keeps all of this branch's tests green. The guard is only load-bearing while this lands without Correct blind-write transaction bookkeeping and bring those transactions under the long-transaction monitor #2232.
   • The abort() handle release this branch added is already on main as 4fbe87d95, and is dropped in the merge.

6. Unrelated collision worth flagging: Correct blind-write transaction bookkeeping and bring those transactions under the long-transaction monitor #2232 conflicts with fix(txn): keep context.transaction usable after its transaction completes #2230. Three cases in adoptedHandleBookkeeping.test.js assert context.transaction === null after a completed transaction (e.g. :112), which fix(txn): keep context.transaction usable after its transaction completes #2230 changes to the shared released placeholder. Whichever merges second needs those three assertions updated — the same edit fix(txn): keep context.transaction usable after its transaction completes #2230 already made to the other suites. Not caused by this PR; found while verifying the stack.

7. The engines still differ on reads, and the interface says so: a rotated RocksDB generation is snapshot-free; LMDB cannot open a snapshot-free read transaction and keeps its snapshot.

Verification

Base behavior measured first: two writes across two stores after a mid-scope commit, handler throws, both still present. Then, with the fix, both roll back.

Three things the tests caught that I would otherwise have shipped:

• Joining reads to a snapshot-bearing resumed transaction was a read-visibility regression — a search() after a mid-scope commit stopped seeing another writer's row. Compared against base rather than assumed.
• The guarantee was conditional on not reading: a nested read scope replaced context.transaction, so commit → read → write → throw left the write durable. Since committing in order to re-read is the reason the pattern exists, the guarantee would have been absent in exactly the shape people use.
• The synchronous commit path rotated the head but left the chained link attached and CLOSED, so a second database's write committed itself and survived the rollback — the exact cross-store leftover this change exists to prevent.

Also corrected against a control rather than assumed: an index scan does not see this scope's uncommitted staged writes (true in a normal scope too), and a static write with no explicit context resolves to the ambient context — so a test using it as "another writer" was measuring nothing.

Coverage: 12 unit cases in unitTests/resources/resumeAfterMidScopeCommit.test.js — two real databases through the transaction.next chain (rollback and success), failed-commit poisoning through a forced terminal native failure, intervening reads, other-writer visibility, the iterator carve-out, no-owning-scope (the harper#1591 hazard), the checkpoint loop, and the ambient operation-handler path. Plus transport-level proof in the existing transaction-context-reads fixture: a handler that commits then writes twice then throws returns 5xx and neither record exists; the success route leaves both durable. Review had shown my earlier "not observable end-to-end" claim was false.

Gates: test:unit:resources 1606 passing / 0 failing on RocksDB, 1400 / 0 with HARPER_STORAGE_ENGINE=lmdb (4 cases pending there, each with its engine reason stated). test:integration -- transaction-context-reads 8/8. Lint and prettier clean. test:unit:main and test:integration:all cannot run in this checkout — other worktrees' Harper processes hold ~/harper/database/system/LOCK — so they run in CI.

Complexity: complicated

_{Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=5 @ 9b89d1b}

_{Human-Review-Need: 4 @ 9b89d1b}

[gemini-code-assist]

Code Review

This pull request implements post-commit atomicity for the RocksDB storage path, aligning its behavior with LMDB. When a transaction is committed mid-handler, the transaction scope rotates to a fresh, open, and snapshot-free generation. This ensures that subsequent writes within the same scope are staged and committed or rolled back together upon request completion, rather than committing individually. Additionally, a shared RELEASED_TRANSACTION placeholder is introduced to safely handle completed transactions on released contexts. I have no feedback to provide as the code changes are well-implemented and the reviewed comment was found to be incorrect.

[claude]

Suggestion (non-blocking): the two large JSDoc blocks here (A successful commit that is NOT the scope's final one... and Finish a commit: the chain goes with it...) describe rotateAfterMidScopeCommit and completeMidScopeCommit respectively, but both sit stacked above endScopeOwnership instead of above the methods they document (which are defined a few lines below). Only the last comment block (Both scope flags leave together...) is actually attached to endScopeOwnership by JS/TS doc-comment convention, so an editor hovering rotateAfterMidScopeCommit/completeMidScopeCommit won't surface their reasoning at all. Move each block to sit directly above its own method.

[claude]

Reviewed; no blockers found. One non-blocking suggestion posted inline (misplaced JSDoc blocks in DatabaseTransaction.ts).

[DavidCockerill]

Approving at this head. No findings in the change itself — the separation of "a generation is physically open" from "a scope is still responsible for committing it" is the right cut, and the rotation falls out of it cleanly.

Two things I want to credit specifically:

• A failed commit deliberately does not rotate. "A commit that FAILED leaves this generation finished and its durability unknown. Rotating after it would let the rest of the scope stage into a generation whose predecessor may not have landed." That is the one case where rotating would be actively harmful, and it is excluded explicitly rather than by omission.
• The engine difference is stated rather than smoothed over — a rotated RocksDB generation is snapshot-free, while LMDB cannot open a snapshot-free read transaction so the scope keeps its snapshot. Two behaviours under one API, documented as two.

The release-line argument is also made the right way round: targeting 5.2.x rather than 5.3 because every 4.7 → 5.2.x upgrade would otherwise carry the regression for that line's whole life, evidenced by 8 of 12 unit cases passing unchanged on LMDB rather than asserted.

One thing worth surfacing for central-manager, since it is not obvious from here. CM main is pinned to harper-pro 5.1.23 because on 5.2.x a second context.transaction.commit() on the same context threw TypeError: Cannot read properties of null (reading 'commit') — 19 sites across 9 files. Tracing that against this head:

• context.transaction is no longer nulled on release; DatabaseTransaction.ts:550/:616 set it to RELEASED_TRANSACTION, and RELEASED_TRANSACTION_SURFACE.commit() returns { txnTime: 0 } — a no-op that succeeds. That sentinel is already on main and is not part of this PR.
• The sentinel alone only stops the crash, though: addWrite() on a released transaction throws "Cannot write to a transaction that has already completed", so a commit-then-write-then-commit handler would fail at the intervening write instead of the second commit.
• This PR is what makes that pattern work, by rotating to a fresh OPEN generation so the post-commit writes stage normally — and atomically, which is the actual point.

So the upstream fix for the CM breakage is the sentinel plus this, and the useful next step on our side is identifying which harper-pro release carries both rather than guarding the remaining CM sites on the assumption they are still needed. Noting it here mostly so the connection is on the record — the 5.2.x targeting decision above looks even better in that light.

Coverage, so the approve is honest: I read the rotation path and its failed-commit exclusion, the open/responsibility split, the engine read difference, and the transaction-slot assignments. I did not read closely the two integration fixtures or the wider Table.ts / ResourceInterface.ts edges. Also worth a re-read once this retargets main — a rebase onto a different #2230 is exactly where the rotation guards could shift.

_{— DAIvid (Claude Opus 5)}