Make a transaction's commit wait for the long-transaction monitor's force-commit instead of acking in-flight writes

[kriszyp]

A transaction that the long-transaction monitor force-commits now resolves only after that commit lands, and rejects if it failed. Before this change, a source-apply (replication / external-source) transaction held open past the limit could tell its caller that its writes were committed while the monitor's native commit was still in flight. If that commit then failed, the error was only logged at debug level.

This is what turned main red: Unit Test at 4365c0c5b, Node.js v22 leg, unitTests/resources/txn-tracking.test.js:457 "names a source-apply txn that the monitor is not reaping". After transaction({ sourceApply: true }) resolved, the test read back the previous test's value (4002 !== 8). The triggering merge, ci(review-coverage): count a Review-Coverage footer only when the pre-push helper could have written it, only touches a CI script, and the failure appears on one leg only. A slow runner disk exposed a real ordering defect; the defect itself is not new.

Mechanism

1. The monitor's force-commit branch calls txn.commit() without awaiting it. That commit marks every staged write saved, sets open = CLOSED, and detaches the native handle before submitting.
2. The handler then returns and transaction() calls commit({ doneWriting: true }) on the same instance. That call finds no unsaved writes and no handle, so it takes the synchronous path and returns immediately.

Reproduced locally by stalling the native commit for 200 ms: transaction() resolved at about 159 ms and get() returned the value from before the transaction.

For the human reviewer

• Planning verdict: better-alternative-exists, adopted. My first plan tracked any in-flight commit on the instance and cleared it on settle. The reviewer showed that loses a monitor failure that settles before the handler returns: the owner's final commit would still ack. The chosen design is the reviewer's monitor-scoped one. The monitor stores the promise its commit returned as monitorCommit (the whole logical outcome, including chained stores). Owner commits join it at commit() entry; retry rounds bypass the join via options.transaction. The monitor clears it on success and keeps it on failure. Explicit mid-scope commits keep their current semantics.
• Failure cleanup is release-only, not abort(). Look hardest here. Round 1 used abort(). Round 2 (Codex) traced a partially landed multi-store commit where abort()'s blob cleanup would unlink files that a landed head's audit entries still reference. The monitor's failed commit has already run its terminal cleanup, so the owner now only releases the context. Tradeoff: blob files of writes that never landed stay on disk instead of being deleted. That is also the behaviour on main today, where the phantom success cleared writes without any cleanup.
• Fix to adjacent code that already existed: a synchronous throw from this.next.commit() in the async success path skipped the landed head's clearWrites / releaseRecordLocks. It now becomes a rejected completion, which is how the synchronous path already handles it. It has a no-op observer, so a later bookkeeping throw cannot leave it unhandled.
• Declined: Gemini's point that a synchronous throw inside the monitor's commit now reaches the owner as a rejection instead of a synchronous throw. Write-bearing commits already return promises, transaction.ts handles that, and on main this case gave the owner a phantom synchronous success.
• Not in this PR (pre-existing, recorded as findings): the LMDB monitor has a narrower version of the same window, limited to multi-store chains and replication confirmation. Separately, abort() from onError while a monitor commit is still in flight can unlink blobs the landing record references.
• A failed monitor commit is replayed to every mid-scope commit in the scope, and only the final doneWriting commit clears it. This is deliberate, so a checkpointing handler cannot swallow it and continue.
• The invariant is recorded in resources/DESIGN.md.

Verification

• New tests in txn-tracking.test.js gate the real RocksDB native commit once the monitor has submitted it. They cover three orderings: success (the owner stays pending until release, then reads back the new value), failure while the owner waits, and failure that settled before the handler returned (which also asserts the context is released). All three fail on origin/main with the intended assertions and pass with the fix.
• A synchronous next-store throw test fails on the prior commit ("the landed head's write set must be cleared") and passes with the fix.
• npm run test:unit:resources (RocksDB): 3649 passing, 0 failing. The LMDB run (HARPER_STORAGE_ENGINE=lmdb) of the same suite: 2806 passing, 0 failing.
• npm run test:unit:main: 6288 passing, 1 failing. The failure is environmental: gitCredentials.test.js asserts that GIT_SSH_COMMAND, GIT_CONFIG_GLOBAL and GIT_EDITOR are absent, and the agent harness exports them. With those unset, that file shows 19 passing.
• oxlint, prettier, tsc build and check:design-docs are clean.
• End-to-end route: not observable end to end in a deterministic way, because it needs a native commit slower than the handler tail. Covered by unit tests against the real transaction() → DatabaseTransaction → RocksDB path. test:integration:all was not run locally; CI runs it.

Refs ci(review-coverage): count a Review-Coverage footer only when the pre-push helper could have written it (the merge where main went red; not the cause)

Signed: Claude Opus 5.5

🤖 Generated with Claude Code

Complexity: complicated

Origin — the dispatch brief this PR was written from

harper main is red: Unit Test at 4365c0c

main on HarperFast/harper went from green to red. Failing workflows: Unit Test at 4365c0c.

Find the merge that broke it and get main green. Walk that workflow's recent push runs on main oldest-to-newest to find the FIRST red head SHA and the PR it came from, then pull the failing test names from that run and from the current head. A failure on every matrix leg (Node version, Bun, uWS, Windows) is a regression; one leg is usually a flake.

Then either fix it or, when the culprit is a single merge and the fix is not obvious, open a revert and say so — main being green is worth more than the change being preserved. Record the fingerprint on the initiatives/ci-health board doc either way (prose row beginning with the test path, never a bare ref), and add your PR to initiatives/testing-and-deploy > "CI & build infrastructure" as a bare-ref line so it renders as a card.

_{Dispatch: task main-red-kriszyp_harper_4365c0c5b_4d7eefda · queued by automation · ran by claude/opus/high · worker kzyp-xps-1}

_{Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=5; full=1 @ 426bafd}

_{Human-Review-Need: 3 (decisions: owner-joins-monitor-commit, failure-kept-until-final-commit, release-only-on-final-failure, rocks-only-scope) @ 426bafd}

[gemini-code-assist]

Code Review

This pull request implements a mechanism where a transaction owner's commit waits for the monitor's force-commit, ensuring proper synchronization and cleanup during timeouts or multi-store commit failures. It introduces the monitorCommit promise to track the monitor's commit progress, handles synchronous errors when committing chained transactions to ensure bookkeeping for landed stores is completed, and updates the design documentation and test suite to cover these scenarios. No review comments were provided, and the implementation looks solid, so I have no feedback to provide.

[claude]

Suggestion (non-blocking): This bullet reads as a universal invariant, but the mechanism it describes (monitorCommit) is RocksDB-only — LMDBTransaction.commit()/startMonitoringTxns() overrides this path entirely and still force-commits fire-and-forget (only harperLogger.debug?.-logged on failure), so LMDB retains a narrower version of the same window. The PR body already discloses this ("the LMDB monitor has a narrower version of the same window..."), so worth making that scope explicit here too, mirroring the existing "RocksDB-write-path only" callout at DatabaseTransaction.ts:374-376.

[claude]

Reviewed; no blockers found. Left one non-blocking suggestion on the new DESIGN.md bullet to make its RocksDB-only scope explicit, since LMDBTransaction retains the narrower pre-fix window (already disclosed in the PR body).