fix(txn): don't drop writes staged while read iterators defer the commit#1860
fix(txn): don't drop writes staged while read iterators defer the commit#1860kriszyp wants to merge 9 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where lingering transactions (transactions committed while read iterators are still open) would silently drop staged writes because this.writes was cleared prematurely. It also ensures that failures in deferred commits are caught, logged, and aborted to prevent unhandled promise rejections and clean up resources. Additionally, a comprehensive test suite has been added to verify these behaviors. Feedback suggests wrapping the commit call in a try-catch block to handle synchronous errors and improving the promise detection check to avoid potential false positives.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
… iterators retain the handle Replaces the LINGERING deferral (commit deferred to doneReadTxn() when the last iterator finishes) on the RocksDB path: a commit with outstanding read iterators now replays its staged writes onto a fresh transaction (ERR_TRY_AGAIN-replay style: entries reload through the new transaction, isRetry suppresses re-appending audit/txn-log entries, conflicts re-resolve through the normal retry ladder) and commits them before the awaited commit chain resolves. The original native handle stays open solely for the iterators and is aborted empty when they finish. This closes the whole class of deferred-commit loss windows: the long-transaction monitor can no longer abort already-acknowledged writes (it now just releases the read snapshot of an overdue CLOSED transaction via releaseReadTxn()), a hung stream can no longer strand acknowledged writes, and a terminal commit failure now rejects the awaited chain instead of being loggable-only. save() no longer stages writes into a retained (non-OPEN) handle - internal txnForContext writers commit immediately on a fresh transaction instead of silently riding a handle that gets aborted. Per review discussion with Kris on PR #1860 (cb1kenobi's monitor finding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tp9CaX9wmJTwbNRkDWATQC
| (txn.db as any)?.name + (url ? ' path: ' + url : '') | ||
| }` | ||
| ); | ||
| txn.releaseReadTxn(); |
There was a problem hiding this comment.
Blocker: new monitor branch (the fix for cb1kenobi's write-loss finding) has no test coverage.
This CLOSED branch is the direct fix for the bug cb1kenobi found during this PR's review (a long-transaction monitor that could abort an already-acknowledged transaction's writes when a read iterator outlived STORAGE_MAXTRANSACTIONOPENTIME). The fix is structurally sound — releasing only the read snapshot via releaseReadTxn() instead of touching writes — but neither unitTests/resources/lingeringWriteCommit.test.js nor unitTests/resources/txn-tracking.test.js exercises this exact scenario: a transaction that committed via the replay path (outstanding iterators) while its native read handle is then held open past the monitor's timeout.
txn-tracking.test.js already has the pattern to extend (setTxnExpiration(20) + delay(150), see "aborts a write-bearing txn open too long..."). Given this branch exists specifically because a real regression was found in this exact area during this PR's own review cycle, it's worth pinning down with a test that: starts a search iterator, writes behind it (triggering the replay-commit), lowers txnExpiration, holds the iterator open past the limit, and asserts the write survives and the snapshot is released via releaseReadTxn() rather than force-aborted.
Suggested fix: add a test case alongside lingeringWriteCommit.test.js's existing iterator scenarios that drives the monitor tick while an iterator from a replay-committed transaction is still open.
| transaction = replayTransaction; | ||
| } | ||
| // with options.transaction set this is a retry round — the save loop above already | ||
| // re-staged the writes into it |
There was a problem hiding this comment.
Suggestion (non-blocking): this replay-commit transaction.commit() submits real work to the storage engine, same as the "no more reads" branch below (line ~464), but only that other branch calls enterWriteQueue()/leaveWriteQueue() around its commit() (lines ~479-484). Writes committed through this outstanding-iterators replay path — a real, if less common, write source — won't be reflected in the write-transaction-queue-depth analytics gauge, understating the backlog during workloads that mix long-held iterators with writes. Consider wrapping this commitResolution the same way for consistent accounting.
RocksDB's clear() is an unsynchronized native range-delete on the libuv threadpool — unlike LMDB's clear, which goes through the same single-writer instruction queue as put/remove, it has no coordination with in-flight transaction commits. setupTestApp()'s reseed path fired tables.X.clear() without awaiting it, so the delete-all range op could still be in flight when the following sequential PUT loop reinserted records, racing the stale delete against the fresh insert and losing the row. This surfaced as CI-only failures on PR #1860 (Unit Test Node.js v22/v26): "check headers on get" 404s on a record written moments earlier, and FourProp coming back empty from search_by_value/SQL — consistent with the reseeded rows being wiped by a late-arriving clear from the previous test file's setup. Awaiting the clears serializes them before the reseed starts, closing the race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A commit issued while a search iterator still holds the read transaction goes LINGERING: the native commit is deferred until the last iterator finishes (doneReadTxn). But the LINGERING fall-through in commit() cleared this.writes, so the deferred commit found an empty write set and aborted the native transaction — silently dropping every staged write after the caller was already told the commit succeeded. Keep the writes across the deferral, and log a deferred-commit failure loudly instead of leaking it as an unhandled rejection (the awaited chain resolved when the transaction went LINGERING, so a later failure has no caller left to reject). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…review) Codex review caught that the post-lingering case never exercised the retained transaction: the transactional wrapper only reuses an OPEN context transaction, so the late write runs on a fresh transaction. Assert that real behavior — independent immediate commit that leaves the lingering deferral intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…l review) Gemini review: a deferred lingering commit that fails terminally has no error middleware left to clean up — without an abort, this.writes stays populated and savedBlobs temp files leak. Abort in the catch so cleanupUnusedBlobs runs; assert the cleanup in the failure test. Also drain an event-loop turn before the unhandled-rejection assertions (delivery is a later macrotask). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…here) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ndle on terminal commit failure (bot review) With no async completions pending, commit() runs synchronously inside doneReadTxn's LINGERING branch, so a sync throw bypassed the .catch() and propagated into the iterator consumer. Wrap in try/catch routing to the same log+abort handler. Also abort the native transaction on a terminal non-conflict commit failure (mirrors the retry-exhaustion give-up pattern) so the handle doesn't leak. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… iterators retain the handle Replaces the LINGERING deferral (commit deferred to doneReadTxn() when the last iterator finishes) on the RocksDB path: a commit with outstanding read iterators now replays its staged writes onto a fresh transaction (ERR_TRY_AGAIN-replay style: entries reload through the new transaction, isRetry suppresses re-appending audit/txn-log entries, conflicts re-resolve through the normal retry ladder) and commits them before the awaited commit chain resolves. The original native handle stays open solely for the iterators and is aborted empty when they finish. This closes the whole class of deferred-commit loss windows: the long-transaction monitor can no longer abort already-acknowledged writes (it now just releases the read snapshot of an overdue CLOSED transaction via releaseReadTxn()), a hung stream can no longer strand acknowledged writes, and a terminal commit failure now rejects the awaited chain instead of being loggable-only. save() no longer stages writes into a retained (non-OPEN) handle - internal txnForContext writers commit immediately on a fresh transaction instead of silently riding a handle that gets aborted. Per review discussion with Kris on PR #1860 (cb1kenobi's monitor finding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tp9CaX9wmJTwbNRkDWATQC
… state; don't forward head txn to next chain (cross-model review) Three fixes from the cross-model review of the replay rework: - Audit-entry loss (Codex blocker): audit/txn-log entries batch natively on the transaction they were staged into and are written only by its commit attempt; the original handle here never attempts one, so its batch dies with the doneReadTxn() abort. The replay transaction must therefore NOT be marked isRetry (and must not inherit the original's onCommit) so the replayed stagings re-append their entries into its own batch. Test asserts exactly one audit entry after a lingering replay commit. - Read-reference theft (Codex): consume the commit's base read reference exactly once per read handle via an explicit baseReadRefConsumed flag (reset when getReadTxn creates a handle), instead of inferring rounds from options.transaction - a second top-level commit() (wrapper commit after an explicit in-handler commit) no longer steals an iterator's reference. - Multi-store cascade (Codex, pre-existing): never forward options.transaction (a retry/replay round's head-store handle) into this.next.commit() - the next store must commit its own writes through its own transaction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tp9CaX9wmJTwbNRkDWATQC
RocksDB's clear() is an unsynchronized native range-delete on the libuv threadpool — unlike LMDB's clear, which goes through the same single-writer instruction queue as put/remove, it has no coordination with in-flight transaction commits. setupTestApp()'s reseed path fired tables.X.clear() without awaiting it, so the delete-all range op could still be in flight when the following sequential PUT loop reinserted records, racing the stale delete against the fresh insert and losing the row. This surfaced as CI-only failures on PR #1860 (Unit Test Node.js v22/v26): "check headers on get" 404s on a record written moments earlier, and FourProp coming back empty from search_by_value/SQL — consistent with the reseeded rows being wiped by a late-arriving clear from the previous test file's setup. Awaiting the clears serializes them before the reseed starts, closing the race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
7a5e3fc to
55e246a
Compare
| const history = await LingerTable.getHistoryOfRecord('linger-write'); | ||
| assert.equal(history.length, 1, 'the replayed write must carry exactly one audit entry (not zero, not two)'); | ||
| assert.deepEqual(unhandled, [], 'the replay commit must not leak an unhandled rejection'); | ||
| }); |
There was a problem hiding this comment.
Low (test coverage): no regression test exercises the monitor's new CLOSED branch (releaseReadTxn).
These tests pin the commit-time replay and the iterator-drain release, but not the path the prior Medium was actually about: the long-transaction monitor firing on a committed (open === CLOSED) transaction whose iterator still holds the read snapshot past STORAGE_MAXTRANSACTIONOPENTIME. That path now runs the new releaseReadTxn() branch in startMonitoringTxns() — the code added specifically to neutralize the finding — and it has no direct coverage.
The risk is low because the write is already durable at ack time (the replay commit is part of the awaited chain, as the first test asserts), so the monitor can't drop an acknowledged write even untested. But a regression test would lock in the branch that exists to prevent exactly the prior bug. txn-tracking.test.js already uses setTxnExpiration(20) to drive the monitor; a case there (or here) that holds an iterator open across a committed write, lets the monitor tick, then asserts the committed record survives, the snapshot is released (context.transaction.transaction === null), and nothing floats an unhandled rejection would close the gap.
—
Generated by Barber AI
Resolves the DIRTY/CONFLICTING merge state against main (34 commits ahead since the merge-base, 7 touching resources/DatabaseTransaction.ts). - unitTests/apiTests/setupTestApp.mjs: both sides independently added the same `await Promise.all([...clear()])` fix with only differing comments; kept a merged comment covering both the general async-clear hazard and the RocksDB-specific race. - resources/DatabaseTransaction.ts: auto-merged cleanly line-by-line, but main's ERR_TRY_AGAIN retry rework (dcd44e4) had changed the `transaction` local in commit() from `let` to `const` (that rework never reassigns it — it resets the same native handle in place). This PR's outstanding-iterators replay path reassigns `transaction = replayTransaction`, which the line-based merge didn't detect as a conflict. Reverted to `let` so the replay assignment is valid; not a build error 3-way merge would catch mid- merge since main and the PR branch never touch that exact line. Build passes; unitTests/resources/**/*.js: 1254 passing / 3 pre-existing failures (confirmed present on origin/main alone, in sourceApplyConflictRetry.test.js, unrelated to this PR). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
Summary
A commit issued while a search iterator still holds the read transaction cannot commit the native handle — the iterator's native cursor lives inside it (
GetIteratorwraps the transaction's write batch + snapshot, and RocksDB invalidates it on commit). The old code deferred the entire native commit todoneReadTxn()(the LINGERING state), and the LINGERING fall-through clearedthis.writes, so the deferred commit found an empty write set and aborted the native transaction with the staged writes in it — every write silently dropped after the caller was told the commit succeeded.Found while root-causing the #1785 follow-up; empirically confirmed on main — the new test fails without the fix.
Approach (reworked after review discussion)
The first version of this PR preserved
this.writesacross the deferral so the deferred commit worked. Review (cb1kenobi) showed the deferral itself is the hazard: any path that keeps the last iterator from finishing cleanly — the long-transaction monitor's timeout abort, a hung stream — still dropped acknowledged writes. So this now eliminates the deferred commit entirely on the RocksDB path:commit()with outstanding iterators replays the staged writes onto a fresh transaction and commits them immediately, ERR_TRY_AGAIN-replay style: entries reload through the new transaction, commit handlers re-base on them (CAS re-resolves against current state), and conflicts surface through the normal retry ladder. The replay is part of the awaited commit chain, so the caller's acknowledgment now covers actual durability — and a terminal commit failure rejects the awaited chain instead of being loggable-only.isRetry: audit/txn-log entries batch natively on the transaction they were staged into and are only written by its commit attempt (the ERR_TRY_AGAIN replay relies on the original's failed commit having already written its batch; here the original never attempts one). The replayed stagings re-append their entries into the replay transaction's own batch — the test asserts exactly one audit entry (no loss, no duplication).TRANSACTION_STATE.LINGERINGis now LMDB-only (LMDB overrides all the relevant methods; its path is unchanged).save()no longer stages writes into a retained non-OPEN handle (they would be discarded by that abort) — internaltxnForContextwriters now commit immediately on a fresh transaction.releaseReadTxn()), preserving the open-time bound for hung iterators without re-introducing write loss (the cb1kenobi finding).baseReadRefConsumed), so retry rounds, immediate-commit re-entries, and a second top-levelcommit()can't steal an iterator's reference and abort the handle under it.this.next.commit()no longer inherits a retry/replay round's head-storeoptions.transaction(pre-existing bug; the next store must commit through its own transaction).Where to look
DatabaseTransaction.commit()(the replay) and the simplifieddoneReadTxn().releaseReadTxn().unitTests/resources/lingeringWriteCommit.test.js— asserts immediate durability while the iterator is still open, exactly-one audit entry, independent post-close writes, and that a terminally failing replay commit rejects the awaited chain while the iterator survives.renewAfterCommit/CommitAndTryCreateSnapshot+ iterator-reseek primitive in rocksdb-js would let the original handle commit directly (the old TODO's intent) and replace the replay; noted as follow-up work.Likely a v5.1 patch candidate — the bug exists on the release branch.
Cross-model reviewed (Codex traced repo paths; Gemini via direct API on the diff): Codex's audit-entry blocker, read-reference theft, and next-chain transaction leak are fixed in the final commits; coverage: codex ✓ / gemini ✗ (agy hung) → api ✓.
Generated by an LLM (Claude Fable 5 + Claude Sonnet 5), supervised by Kris.
🤖 Generated with Claude Code