From b358449501036ee145ee8652a30334a4f1c1b29c Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 20 Aug 2026 08:14:56 -0600 Subject: [PATCH 1/5] fix(txn): make writes after a mid-scope commit atomic with the scope that owns them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Measured on the base: two writes across two databases after a mid-scope commit, handler throws, both still present. `open` was standing in for two things — whether a physical 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 both dispatchers read `open` for the second question. 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, so CLOSED never gains a second meaning and the default path is untouched. The new generation is snapshot-free: committing mid-scope is how a handler asks to stop reading a pinned snapshot, and re-pinning one would take that back. Ownership is a constructor flag on a #private field, so application code cannot mark a transaction no scope owns. Rotation is refused when the scope is finished, when a timeout poisoned it, when a commit FAILED, and while read iterators still hold the native handle. This ends an engine divergence rather than adding behavior: harperdb 4.7 ships lmdb and no RocksDB at all, and LMDBTransaction leaves a non-final commit OPEN, so post-commit writes have always staged and rolled back there. 8 of the 12 unit cases here pass on LMDB unchanged. The 4.7 -> 5.x move onto RocksDB is what dropped the property, which is why this targets 5.2.x. Rebased onto #2232, whose attachOwnedTransaction() seeds the per-handle bookkeeping at the source and whose supervisedWriteRoots brings these transactions under the long-transaction monitor — the gap that was this change's one objection to a patch release. An earlier doneReadTxn() state check enforcing the same invariant is dropped in favour of it; keeping it also declined the release abort() performs. Closes #2238. Co-Authored-By: Claude Opus 5 --- .../transaction-context-reads/resources.js | 28 ++ .../transaction-context-reads.test.ts | 32 ++- resources/DatabaseTransaction.ts | 83 +++++- resources/ResourceInterface.ts | 15 + resources/Table.ts | 3 + resources/transaction.ts | 4 +- .../resumeAfterMidScopeCommit.test.js | 267 ++++++++++++++++++ 7 files changed, 419 insertions(+), 13 deletions(-) create mode 100644 unitTests/resources/resumeAfterMidScopeCommit.test.js diff --git a/integrationTests/fixtures/transaction-context-reads/resources.js b/integrationTests/fixtures/transaction-context-reads/resources.js index 5bd19a6c31..585db31802 100644 --- a/integrationTests/fixtures/transaction-context-reads/resources.js +++ b/integrationTests/fixtures/transaction-context-reads/resources.js @@ -97,3 +97,31 @@ export class DashWriteThenSearch extends Resource { return { variant: 'write-then-search', companyId, count: snapshots.length, snapshots }; } } + +// POST-COMMIT ATOMICITY — commit the per-request transaction mid-handler, then write two records +// and throw. Those writes belong to the request transaction's pending final commit, so the failure +// must leave neither of them behind. +export class DashCommitWriteThrow extends Resource { + static loadAsInstance = false; + async get(query) { + const suffix = paramId(query) ?? 'x'; + await tables.Company.get('c1'); + await transaction.commit(this); + await tables.Company.put({ id: `atomic-company-${suffix}`, name: 'should not survive' }); + await tables.ScoreSnapshot.put({ id: `atomic-snap-${suffix}`, companyId: 'atomic-co', score: 1 }); + throw new Error('deliberate failure after the mid-handler commit'); + } +} + +// The same shape that succeeds: both post-commit writes must be durable once the request completes. +export class DashCommitWriteOk extends Resource { + static loadAsInstance = false; + async get(query) { + const suffix = paramId(query) ?? 'x'; + await tables.Company.get('c1'); + await transaction.commit(this); + await tables.Company.put({ id: `ok-company-${suffix}`, name: 'kept' }); + await tables.ScoreSnapshot.put({ id: `ok-snap-${suffix}`, companyId: 'atomic-co', score: 2 }); + return { variant: 'commit-write-ok', suffix }; + } +} diff --git a/integrationTests/resources/transaction-context-reads.test.ts b/integrationTests/resources/transaction-context-reads.test.ts index 6989323d61..0734673bb4 100644 --- a/integrationTests/resources/transaction-context-reads.test.ts +++ b/integrationTests/resources/transaction-context-reads.test.ts @@ -104,13 +104,35 @@ suite('Transaction context: closed txn in ALS still reads latest', { skip: skipS strictEqual(body.count, SNAP_IDS.length); }); - test('commit-then-search (txn closed in ALS) still returns all rows', async () => { + test('commit-then-search still returns all rows, on the rotated generation', async () => { const body = await dashCount('/DashCommitThenSearch/'); - // Guards the misdiagnosis: the request txn is genuinely closed before the search - // (open: 1 -> 0), yet the closed slot reads latest committed state, not empty. + // The explicit commit closes the generation it committed and the scope rotates to a fresh + // OPEN, snapshot-free one, because the request scope still owes a final commit. Either way the + // search must read the latest committed state — which is what the original misdiagnosis + // (empty results from a closed slot) claimed it would not. strictEqual(body.txnOpenBefore, 1, 'txn was open before the explicit commit'); - strictEqual(body.txnOpenAfter, 0, 'txn is closed in ALS before the search'); - strictEqual(body.count, SNAP_IDS.length, 'closed-txn-in-ALS must still see all snapshots'); + strictEqual(body.txnOpenAfter, 1, 'the scope rotates to a fresh open generation after its own commit'); + strictEqual(body.count, SNAP_IDS.length, 'the rotated generation must still see all snapshots'); + }); + + test('writes made after a mid-handler commit roll back when the request fails', async () => { + const r = await fetch(`${httpURL}/DashCommitWriteThrow/?company=rollback`, { + headers: { Authorization: auth }, + }); + ok(r.status >= 500, `expected the handler failure to surface, got ${r.status}`); + for (const path of ['/Company/atomic-company-rollback', '/ScoreSnapshot/atomic-snap-rollback']) { + const probe = await fetch(`${httpURL}${path}`, { headers: { Authorization: auth } }); + strictEqual(probe.status, 404, `${path} must not exist: it was written after the mid-handler commit`); + } + }); + + test('writes made after a mid-handler commit are durable when the request succeeds', async () => { + const r = await fetch(`${httpURL}/DashCommitWriteOk/?company=keep`, { headers: { Authorization: auth } }); + ok(r.status < 300, `DashCommitWriteOk expected 2xx, got ${r.status}`); + for (const path of ['/Company/ok-company-keep', '/ScoreSnapshot/ok-snap-keep']) { + const probe = await fetch(`${httpURL}${path}`, { headers: { Authorization: auth } }); + ok(probe.status < 300, `${path} must be durable once the request completes, got ${probe.status}`); + } }); test('lazily-returned search iterated during post-commit serialization returns all rows', async () => { diff --git a/resources/DatabaseTransaction.ts b/resources/DatabaseTransaction.ts index ac8a68e770..b7f362b3c5 100644 --- a/resources/DatabaseTransaction.ts +++ b/resources/DatabaseTransaction.ts @@ -325,6 +325,15 @@ type RocksTransactionWithRetry = RocksTransaction & { isRetry?: boolean }; export class DatabaseTransaction implements Transaction { #context: Context; + // Whether a resources/transaction.ts scope owns this instance — i.e. a final commit or abort is + // guaranteed to follow. Only such a transaction may be rotated to a new generation by a mid-scope + // commit (see rotateAfterMidScopeCommit); anything else must commit each later write immediately, + // because nothing would commit staged ones. Settable only at construction, so it cannot be turned on + // for a transaction that is already attached to a context and running. + #scopeOwned: boolean; + constructor(options?: { scopeOwned?: boolean }) { + this.#scopeOwned = options?.scopeOwned === true; + } writes: TransactionWrite[] = []; // the set of writes to commit if the conditions are met // the last staged write per store and key, used to chain repeat writes to the same key (linkWrite) declare writesByKey?: Map>; @@ -383,6 +392,11 @@ export class DatabaseTransaction implements Transaction { // Set once the retained read handle's write intents have been released (see commit()'s // outstanding-iterators branch), so a retry round cannot re-fire the release. declare writesAbandoned?: boolean; + // Set once a mid-scope commit has rotated this instance to a new generation: every native + // transaction it opens from then on reads WITHOUT a snapshot. Committing mid-scope is how a handler + // asks to stop reading a pinned snapshot, so re-pinning one for the rest of the scope would take + // back what it asked for. + declare snapshotFree?: boolean; getReadTxn(disableSnapshot?: boolean): ReadTransaction { this.readTxnRefCount = (this.readTxnRefCount || 0) + 1; @@ -409,7 +423,12 @@ export class DatabaseTransaction implements Transaction { // snapshot that blocks compaction. Only applied when creating the transaction fresh; an // already-open transaction keeps whatever snapshot mode it was created with. // `coordinatedRetry` signals IsBusy write conflicts as RETRY_NOW rather than ERR_BUSY. - this.attachOwnedTransaction(new RocksTransaction(this.db.store, { coordinatedRetry: true, disableSnapshot })); + this.attachOwnedTransaction( + new RocksTransaction(this.db.store, { + coordinatedRetry: true, + disableSnapshot: disableSnapshot || this.snapshotFree, + }) + ); if (this.timestamp) { this.transaction.setTimestamp(this.timestamp); @@ -716,7 +735,10 @@ export class DatabaseTransaction implements Transaction { if (!transaction && this.open === TRANSACTION_STATE.OPEN) transaction = this.transaction; let immediateCommit = false; if (!transaction) { - transaction = new RocksTransaction(operation.store.store as RocksStore); + transaction = new RocksTransaction( + operation.store.store as RocksStore, + this.snapshotFree ? { coordinatedRetry: true, disableSnapshot: true } : undefined + ); if (operation.store.rootStore !== this.db.rootStore) { harperLogger.warn?.('Created new transaction in save, but the store does match existing store', transaction.id); } @@ -997,11 +1019,14 @@ export class DatabaseTransaction implements Transaction { // now reset transactions tracking; this transaction be reused and committed again this.retries = 0; // reset per-native-transaction retry counter so a reused DatabaseTransaction's next batch starts fresh this.clearWrites(); + if (options.doneWriting) this.#scopeOwned = false; this.releaseContext(!!options.doneWriting); - this.next = null; let txnTime = this.timestamp; this.timestamp = 0; // reset the timestamp as well return Promise.all(completions).then(() => { + // Only once the chained store's commit has settled, as on the synchronous path: a + // partially failed mid-scope commit must not leave the scope resumable. + this.completeMidScopeCommit(options); return { txnTime, }; @@ -1083,6 +1108,9 @@ export class DatabaseTransaction implements Transaction { // back-reference here too, or transaction.ts's onComplete() (which has no // rejection handler of its own) would leave a long-lived context pinning this // CLOSED wrapper forever. + // A failed commit must never be followed by a resumed segment: this generation is + // finished and its durability is unknown, so ownership goes with it. + this.#scopeOwned = false; this.releaseContext(!!options.doneWriting); throw error; } @@ -1094,6 +1122,7 @@ export class DatabaseTransaction implements Transaction { cleanupUnusedBlobs(write.savedBlobs, collectRetainedFileIds(write.store.getEntry(write.key)?.value)); } this.clearWrites(); + if (options.doneWriting) this.#scopeOwned = false; this.releaseContext(!!options.doneWriting); const txnResolution: CommitResolution = { txnTime: this.timestamp, @@ -1106,12 +1135,18 @@ export class DatabaseTransaction implements Transaction { options.transaction ? { ...options, transaction: undefined } : options ); if ((nextResolution as any)?.then) - return (nextResolution as any)?.then((nextResolution) => ({ - txnTime: this.timestamp, - next: nextResolution, - })); + return (nextResolution as any)?.then((nextResolution) => { + // Only once the chained store's own commit has SETTLED: rotating first would leave the + // scope resumable after a partially failed mid-scope commit. + this.completeMidScopeCommit(options); + return { + txnTime: this.timestamp, + next: nextResolution, + }; + }); txnResolution.next = nextResolution as any; } + this.completeMidScopeCommit(options); return txnResolution; }, (error) => { @@ -1120,6 +1155,38 @@ export class DatabaseTransaction implements Transaction { } ); } + /** + * A successful commit that is NOT the scope's final one leaves the scope still running and still + * responsible for a commit. Rotate to a fresh OPEN generation so the rest of the scope's writes + * stage into it and are committed — or rolled back — as one unit, instead of each committing itself + * the moment it is made. Every dispatch path keeps its plain `open === OPEN` check; CLOSED never + * gains a second meaning. + * + * Deliberately not rotated when: the scope is finished (`doneWriting`), nothing owns this instance, + * a timeout poisoned it, or a commit failed — a failed or uncertain commit must never be followed by + * a resumed segment that can commit on its own. Nor when read iterators still hold the native + * handle: that handle belongs to them until they drain, so there is nothing to rotate into and those + * writes keep today's immediate-commit path. + */ + /** + * Finish a commit: the chain goes with it, then the scope may rotate. A link left attached and CLOSED + * would be reused by txnForContext for the next write to that database and commit itself, surviving a + * rollback of the rotated head — the cross-store leftover this rotation exists to prevent. Every + * commit path must run this, and none may do one half without the other. + */ + private completeMidScopeCommit(options: CommitOptions): void { + this.next = null; + this.rotateAfterMidScopeCommit(options); + } + + /** See completeMidScopeCommit, which is the only caller and carries the reasoning. */ + private rotateAfterMidScopeCommit(options: CommitOptions): void { + if (options.doneWriting || this.timedOut || this.transaction || !this.#scopeOwned) return; + this.open = TRANSACTION_STATE.OPEN; + this.snapshotFree = true; + this.writesAbandoned = false; + } + abort(): void { while (this.readTxnsUsed > 0) this.doneReadTxn(); // release the read snapshot when we abort, we assume we don't need it // Defensively release any native handle whose reference bookkeeping was already consumed. @@ -1132,6 +1199,8 @@ export class DatabaseTransaction implements Transaction { cleanupUnusedBlobs(write.savedBlobs, collectRetainedFileIds(write.store.getEntry(write.key)?.value)); } } finally { + this.#scopeOwned = false; // the scope is over; nothing may rotate this instance again + this.snapshotFree = false; this.clearWrites(); // A timeout-poisoned abort (abortDueToTimeout()) is the one abort that is NOT "reuse-free": // Resource.ts's dispatcher deliberately keeps joining a `timedOut` transaction (instead of diff --git a/resources/ResourceInterface.ts b/resources/ResourceInterface.ts index 057f6274d4..a97cc8b746 100644 --- a/resources/ResourceInterface.ts +++ b/resources/ResourceInterface.ts @@ -75,6 +75,21 @@ export interface Context { * so code that commits mid-handler can keep using its context. LMDBTransaction does not release, * so there the completed transaction itself stays in the slot — also safe to call, but retained. * `null` was the previous released marker and is still accepted defensively. + * + * A transaction its own handler commits mid-scope is rotated to a fresh open generation, so the rest + * of that scope's writes are committed — or rolled back — with the scope's final commit rather than + * each committing itself immediately. Two cases keep the older per-write behavior: a commit made + * while a read iterator still holds the transaction's handle (that handle is the iterator's until it + * drains), and a commit that failed. LMDB has always behaved this way; the RocksDB path now matches. + * + * The engines still differ on reads. A rotated RocksDB generation is snapshot-free, so the rest of + * the scope keeps seeing other writers' committed data — which is what committing mid-scope asks for. + * LMDB cannot open a snapshot-free read transaction, so there the scope keeps its snapshot. + * + * Because the writes after a mid-scope commit stage rather than commit one at a time, a single + * mid-scope commit does not bound how much a long handler holds in memory. A handler streaming a + * large volume should keep committing (a checkpoint every N records), which commits each batch and + * rotates again. */ transaction?: DatabaseTransaction | null; /** If the operation that will be performed with this context should check user authorization */ diff --git a/resources/Table.ts b/resources/Table.ts index dc4a2836e4..0e7c5f11b4 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -5632,6 +5632,9 @@ export function makeTable(options) { // commit is tracked with its own identity (DatabaseTransaction.ts's trackOutstandingCommit), // so a wedged second-store commit is named just as precisely as a wedged first one. transaction.next.startedFrom = transaction.startedFrom; + // A second database joined after a mid-scope commit belongs to the same snapshot-free + // generation as the head, or its reads would re-pin what the commit just unpinned. + transaction.next.snapshotFree = transaction.snapshotFree; if (transaction.open === TRANSACTION_STATE.CLOSED) { // if the current transaction is already closed, we need to retain that state on new databases we work with transaction.next.open = TRANSACTION_STATE.CLOSED; diff --git a/resources/transaction.ts b/resources/transaction.ts index b2dd2abcdf..8ad2e2600d 100644 --- a/resources/transaction.ts +++ b/resources/transaction.ts @@ -45,7 +45,9 @@ export function transaction( return callback(context.transaction); // nothing to be done, already in open transaction } - const transaction = new DatabaseTransaction(); + // scopeOwned: onComplete/onError below guarantee this instance a final commit or an abort, which is + // what lets a mid-scope commit rotate it instead of leaving later writes to commit themselves. + const transaction = new DatabaseTransaction({ scopeOwned: true }); context.transaction = transaction; if (context.timestamp) transaction.timestamp = context.timestamp; if (context.replicatedConfirmation) transaction.replicatedConfirmation = context.replicatedConfirmation; diff --git a/unitTests/resources/resumeAfterMidScopeCommit.test.js b/unitTests/resources/resumeAfterMidScopeCommit.test.js new file mode 100644 index 0000000000..6cb625859b --- /dev/null +++ b/unitTests/resources/resumeAfterMidScopeCommit.test.js @@ -0,0 +1,267 @@ +const assert = require('assert'); +const { setupTestDBPath } = require('../testUtils'); +const { table } = require('#src/resources/databases'); +const { setMainIsWorker } = require('#js/server/threads/manageThreads'); +const { transaction } = require('#src/resources/transaction'); +const { TRANSACTION_STATE } = require('#src/resources/DatabaseTransaction'); +const serverUtilities = require('#src/server/serverHelpers/serverUtilities'); + +// A handler that commits its own transaction mid-scope is a documented pattern, and its enclosing +// transaction() scope still has a final commit to make. The writes it makes after that commit +// therefore belong to that pending commit: they stage, and they roll back with it. Before this, each +// one committed immediately and individually, so a handler that failed halfway left the earlier half +// durable — which is how a failed cluster delete could mark a cluster TERMINATED with its instances +// still RUNNING. +// Runs on BOTH engines: LMDBTransaction already left a non-final commit OPEN (LMDBTransaction.ts's +// commit), so post-commit writes have always staged and rolled back with the scope there. This change +// brings the RocksDB path to that behavior. Four cases below are RocksDB-only, each for a stated +// engine reason — read-after-write, snapshot-free reads, and the two that reach into rocksdb-js. +const isLMDB = process.env.HARPER_STORAGE_ENGINE === 'lmdb'; +const rocksOnly = isLMDB ? it.skip : it; + +describe('Writes after a mid-scope commit rejoin the scope', () => { + let A, B, Other; + before(async function () { + setupTestDBPath(); + setMainIsWorker(true); + A = table({ table: 'ResumeA', attributes: [{ name: 'id', isPrimaryKey: true }, { name: 'v' }] }); + B = table({ table: 'ResumeB', attributes: [{ name: 'id', isPrimaryKey: true }, { name: 'v' }] }); + // A genuinely separate DATABASE, not just a second table: only this reaches txnForContext's + // `transaction.next` chain, which is where a link created after the mid-scope commit could + // otherwise be left committing on its own. + Other = table({ + database: 'resume_other', + table: 'ResumeOther', + attributes: [{ name: 'id', isPrimaryKey: true }, { name: 'v' }], + }); + }); + + async function scanValues(context) { + const out = []; + for await (const record of A.search([{ attribute: 'v', comparator: 'greater_than', value: -1 }], context)) + out.push(record.v); + return out.sort((x, y) => x - y); + } + + it('rolls post-commit writes back with the scope, across two stores', async function () { + const context = {}; + await assert.rejects( + transaction(context, async () => { + await A.put('pre', { v: 1 }, context); + await B.put('pre', { v: 1 }, context); + await context.transaction.commit(); + await A.put('post', { v: 2 }, context); + await B.put('post', { v: 2 }, context); + throw new Error('forced failure after the mid-scope commit'); + }), + /forced failure/ + ); + assert.ok(await A.get('pre'), 'what the explicit commit committed must stay committed'); + assert.ok(await B.get('pre')); + assert.equal(await A.get('post'), undefined, 'a write made after the commit must roll back with the scope'); + assert.equal(await B.get('post'), undefined, 'including on a chained second store'); + }); + + it('commits post-commit writes when the scope completes', async function () { + const context = {}; + await transaction(context, async () => { + await A.put('ok1', { v: 1 }, context); + await context.transaction.commit(); + await A.put('ok2', { v: 2 }, context); + await B.put('ok2', { v: 2 }, context); + assert.equal(context.transaction.open, TRANSACTION_STATE.OPEN, 'the write must have reopened the scope'); + }); + for (const [t, id] of [ + [A, 'ok1'], + [A, 'ok2'], + [B, 'ok2'], + ]) + assert.ok(await t.get(id), `${id} must be durable once the scope commits`); + }); + + it('rolls back a post-commit write to a second DATABASE, through the chained link', async function () { + const context = {}; + await assert.rejects( + transaction(context, async () => { + await A.put('chain-pre', { v: 1 }, context); + await context.transaction.commit(); + await A.put('chain-post', { v: 2 }, context); + await Other.put('chain-post', { v: 2 }, context); // creates transaction.next after the commit + assert.ok(context.transaction.next, 'premise: a chained link must have been created'); + throw new Error('forced failure'); + }), + /forced failure/ + ); + assert.ok(await A.get('chain-pre')); + assert.equal(await A.get('chain-post'), undefined); + assert.equal(await Other.get('chain-post'), undefined, 'the chained database must roll back too'); + }); + + it('commits a post-commit write to a second DATABASE when the scope completes', async function () { + const context = {}; + await transaction(context, async () => { + await context.transaction.commit(); + await A.put('chain-ok', { v: 1 }, context); + await Other.put('chain-ok', { v: 1 }, context); + }); + assert.ok(await A.get('chain-ok')); + assert.ok(await Other.get('chain-ok'), 'the chained database must be durable'); + }); + + // 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. + // RocksDB only: forces a terminal failure through rocksdb-js's Transaction. + rocksOnly('does not rotate after a failed mid-scope commit', async function () { + const { Transaction } = require('@harperfast/rocksdb-js'); + const originalCommit = Transaction.prototype.commit; + const targetDb = A.primaryStore.store.db; + let forcedFailures = 0; + const context = {}; + try { + await transaction(context, async () => { + await A.put('poison-seed', { v: 1 }, context); + Transaction.prototype.commit = function (...args) { + if (this.store?.db !== targetDb) return originalCommit.apply(this, args); + forcedFailures++; + Transaction.prototype.commit = originalCommit; // one forced failure only + return Promise.reject(Object.assign(new Error('forced terminal failure'), { code: 'ERR_CORRUPTION' })); + }; + await assert.rejects(context.transaction.commit(), /forced terminal failure/); + assert.notEqual( + context.transaction?.open, + TRANSACTION_STATE.OPEN, + 'a failed commit must not leave an open generation behind' + ); + await A.put('after-failed-commit', { v: 2 }, context); + }); + } finally { + Transaction.prototype.commit = originalCommit; + } + assert.ok(forcedFailures > 0, 'premise: the explicit commit must actually have failed'); + assert.equal(await A.get('poison-seed'), undefined, 'the failed commit committed nothing'); + assert.ok( + await A.get('after-failed-commit'), + 'a write after a failed commit must commit itself rather than stage into a generation nobody will commit' + ); + }); + + // RocksDB only: LMDB does not guarantee read-after-write within a transaction. + rocksOnly('reads its own post-commit writes', async function () { + const context = {}; + await transaction(context, async () => { + await context.transaction.commit(); + await A.put('own', { v: 7 }, context); + assert.equal((await A.get('own', context))?.v, 7); + }); + }); + + // Committing mid-handler is documented as the way to stop reading a pinned snapshot, so every read + // shape has to keep seeing other writers for the rest of the scope — including a search() iterator, + // which holds its own handle. This is what the resumed transaction's snapshot-free mode buys. + // RocksDB only: only RocksDB can open a snapshot-free read transaction; LMDB keeps its snapshot. + rocksOnly('keeps reads seeing other writers for the rest of the scope', async function () { + await A.put('shared', { v: 1 }, {}); + const context = {}; + const pointReads = []; + await transaction(context, async () => { + await A.get('shared', context); + await context.transaction.commit(); + for (let v = 2; v <= 4; v++) { + await A.put('shared', { v }, {}); // another writer: its OWN context, not the ambient one + pointReads.push((await A.get('shared', context)).v); + pointReads.push((await transaction(() => A.get('shared'))).v); // the migration-guide form + } + await A.put('own-write', { v: 50 }, context); // resume the scope, then keep reading + const before = await scanValues(context); + await A.put('scan-visible', { v: 99 }, {}); // another writer again + const after = await scanValues(context); + assert.ok( + after.includes(99) && !before.includes(99), + 'a search() after the commit must still pick up another writer’s row' + ); + }); + assert.deepEqual(pointReads, [2, 2, 3, 3, 4, 4], 'every read after the commit must see the latest value'); + }); + + // The resume is scoped by ownership, not by state: without an owning scope there is no pending + // commit, so a write must commit itself rather than stage into a transaction nobody will commit. + it('does not resume a transaction with no owning scope', async function () { + const context = {}; + await transaction(context, async () => { + await A.put('scoped', { v: 1 }, context); + }); + // The scope is over. These writes have no pending commit and must be durable on their own. + await A.put('after-scope-1', { v: 1 }, context); + await A.put('after-scope-2', { v: 2 }, context); + assert.ok(await A.get('after-scope-1'), 'a write after the scope ended must commit itself'); + assert.ok(await A.get('after-scope-2')); + }); + + // A read between the commit and the writes must not quietly cost the guarantee. Committing + // mid-handler in order to re-read is the documented reason to commit at all, so if an intervening + // read dropped the scope's transaction from the context the guarantee would be absent in exactly + // the shape people use. + it('keeps post-commit writes atomic even when a read comes between', async function () { + const context = {}; + await assert.rejects( + transaction(context, async () => { + await A.put('seed', { v: 0 }, context); + await context.transaction.commit(); + await A.get('seed', context); + await scanValues(context); + await A.put('after-read', { v: 1 }, context); + throw new Error('forced failure'); + }), + /forced failure/ + ); + assert.equal(await A.get('after-read'), undefined, 'the write after an intervening read must roll back too'); + }); + + // The one shape the guarantee does NOT cover, pinned so it cannot change silently: with an iterator + // still holding the native handle at commit time, that handle belongs to the iterator until it + // drains, so there is nothing to rotate into and the writes keep committing individually. This is + // the checkpoint-over-a-search loop, and it is stated on Context.transaction for that reason. + // RocksDB only: the retained-handle branch is the RocksDB commit path. + rocksOnly('does not rotate while an iterator still holds the handle, and says so', async function () { + await A.put('iter-seed', { v: 1 }, {}); + const context = {}; + let openState; + await transaction(context, async () => { + const results = await A.search([{ attribute: 'v', comparator: 'greater_than', value: -1 }], context); + const iterator = results[Symbol.asyncIterator](); + await iterator.next(); // hold the handle open + await A.put('iter-write', { v: 2 }, context); + await context.transaction.commit(); + openState = context.transaction.open; + await A.put('iter-post', { v: 3 }, context); + while (!(await iterator.next()).done); + }); + assert.notEqual(openState, TRANSACTION_STATE.OPEN, 'a retained handle must block the rotation'); + assert.ok( + await A.get('iter-post'), + 'and the write after that commit therefore commits on its own — the documented carve-out' + ); + }); + + it('still lets a checkpoint loop commit repeatedly inside one scope', async function () { + const context = {}; + await transaction(context, async () => { + for (let i = 0; i < 4; i++) { + await A.put(`ckpt-${i}`, { v: i }, context); + await transaction.commit(context); + } + }); + for (let i = 0; i < 4; i++) assert.ok(await A.get(`ckpt-${i}`), `ckpt-${i} must persist`); + }); + + it('resumes under an ambient operation-handler context too', async function () { + await serverUtilities.processLocalTransaction( + { body: { operation: 'test_registered_op', hdb_user: { username: 'internal_bookkeeping' } } }, + async () => { + await A.put('ambient', { v: 1 }); + return { message: 'ok' }; + } + ); + assert.ok(await A.get('ambient'), 'the ambient path must be unaffected'); + }); +}); From 812f0bbc3fb75a9be9452f6d41293665e6b7f942 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 20 Aug 2026 08:39:46 -0600 Subject: [PATCH 2/5] fix(txn): surrender ownership on a chained failure, keep the scope flags together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-rebase review, four real findings. A failed CHAINED-store commit did not surrender ownership — only the head's own error branch did. A handler that writes databases A and B, commits mid-scope inside a try/catch (the shape people write precisely because a mid-scope commit can conflict), and sees B fail could then commit again: that second commit rotated with ownership still set, and completeMidScopeCommit dropped the failed link before its abort could run cleanupUnusedBlobs. Both commit paths now clear ownership when a completion rejects, as the head-failure branch already did. `snapshotFree` was cleared on abort but not on a final commit, so a context that outlives its scope — a final commit deferred by an undrained iterator leaves this instance in the slot — kept opening snapshot-free handles, tearing later multi-key reads across concurrent writers. Both flags now leave together through endScopeOwnership(), so no exit can clear one and keep the other. The blind-write options ternary coupled `coordinatedRetry` to the snapshot switch, which moved blind writes onto the RETRY_NOW ladder after a mid-scope commit and the ERR_BUSY ladder before it. Only the snapshot switch belongs to a rotated generation. Two coverage corrections. The ownership-gate test proved the wrong path: after the scope it went through the released placeholder, so txnForContext built an ImmediateTransaction and never consulted ownership. It now uses an unowned OPEN DatabaseTransaction — the replayLogs.ts shape, where a regression to "anything rotates" would stage replay's post-commit writes into a generation its loop never commits. And the integration file had lost its original guard, since an ordinary mid-handler commit now rotates rather than leaving the slot closed; a new undrained-iterator route keeps the handle held, blocks the rotation, and pins that a genuinely closed slot still reads latest committed state rather than empty. Co-Authored-By: Claude Opus 5 --- .../transaction-context-reads/resources.js | 21 ++++++ .../transaction-context-reads.test.ts | 9 +++ resources/DatabaseTransaction.ts | 70 +++++++++++++------ .../resumeAfterMidScopeCommit.test.js | 28 +++++++- 4 files changed, 102 insertions(+), 26 deletions(-) diff --git a/integrationTests/fixtures/transaction-context-reads/resources.js b/integrationTests/fixtures/transaction-context-reads/resources.js index 585db31802..9fcd7a2aa6 100644 --- a/integrationTests/fixtures/transaction-context-reads/resources.js +++ b/integrationTests/fixtures/transaction-context-reads/resources.js @@ -125,3 +125,24 @@ export class DashCommitWriteOk extends Resource { return { variant: 'commit-write-ok', suffix }; } } + +// CLOSED-SLOT READ GUARD — the original point of this fixture, preserved now that an ordinary +// mid-handler commit rotates the scope to a fresh open generation instead of leaving it closed. An +// undrained iterator holds the native handle, so the commit cannot rotate and the slot stays genuinely +// CLOSED; the search that follows must still read the latest committed state rather than empty. +export class DashUndrainedThenSearch extends Resource { + static loadAsInstance = false; + async get(query) { + const companyId = paramId(query); + const held = tables.ScoreSnapshot.search({ + conditions: [{ attribute: 'companyId', comparator: 'equals', value: companyId }], + }); + const iterator = held[Symbol.asyncIterator](); + await iterator.next(); // hold the handle open, do not drain + const ctx = this.getContext(); + await transaction.commit(this); + const txnOpenAfter = ctx?.transaction?.open; + const snapshots = await searchSnapshots(companyId); + return { variant: 'undrained-then-search', companyId, txnOpenAfter, count: snapshots.length, snapshots }; + } +} diff --git a/integrationTests/resources/transaction-context-reads.test.ts b/integrationTests/resources/transaction-context-reads.test.ts index 0734673bb4..36e0aa37f9 100644 --- a/integrationTests/resources/transaction-context-reads.test.ts +++ b/integrationTests/resources/transaction-context-reads.test.ts @@ -115,6 +115,15 @@ suite('Transaction context: closed txn in ALS still reads latest', { skip: skipS strictEqual(body.count, SNAP_IDS.length, 'the rotated generation must still see all snapshots'); }); + test('a genuinely closed slot still returns all rows (original guard, via an undrained iterator)', async () => { + // The commit-then-search case above now rotates to a fresh OPEN generation, so it no longer + // exercises a CLOSED slot. An undrained iterator holds the handle, which blocks the rotation and + // keeps the slot closed — the shape this file was written to pin. + const body = await dashCount('/DashUndrainedThenSearch/'); + strictEqual(body.txnOpenAfter, 0, 'a retained handle must block the rotation, leaving the slot closed'); + strictEqual(body.count, SNAP_IDS.length, 'a closed slot must still read latest committed state, not empty'); + }); + test('writes made after a mid-handler commit roll back when the request fails', async () => { const r = await fetch(`${httpURL}/DashCommitWriteThrow/?company=rollback`, { headers: { Authorization: auth }, diff --git a/resources/DatabaseTransaction.ts b/resources/DatabaseTransaction.ts index b7f362b3c5..ec1434c354 100644 --- a/resources/DatabaseTransaction.ts +++ b/resources/DatabaseTransaction.ts @@ -737,7 +737,7 @@ export class DatabaseTransaction implements Transaction { if (!transaction) { transaction = new RocksTransaction( operation.store.store as RocksStore, - this.snapshotFree ? { coordinatedRetry: true, disableSnapshot: true } : undefined + this.snapshotFree ? { disableSnapshot: true } : undefined ); if (operation.store.rootStore !== this.db.rootStore) { harperLogger.warn?.('Created new transaction in save, but the store does match existing store', transaction.id); @@ -1019,18 +1019,27 @@ export class DatabaseTransaction implements Transaction { // now reset transactions tracking; this transaction be reused and committed again this.retries = 0; // reset per-native-transaction retry counter so a reused DatabaseTransaction's next batch starts fresh this.clearWrites(); - if (options.doneWriting) this.#scopeOwned = false; + if (options.doneWriting) this.endScopeOwnership(); this.releaseContext(!!options.doneWriting); let txnTime = this.timestamp; this.timestamp = 0; // reset the timestamp as well - return Promise.all(completions).then(() => { - // Only once the chained store's commit has settled, as on the synchronous path: a - // partially failed mid-scope commit must not leave the scope resumable. - this.completeMidScopeCommit(options); - return { - txnTime, - }; - }); + return Promise.all(completions).then( + () => { + // Only once the chained store's commit has settled, as on the synchronous path: a + // partially failed mid-scope commit must not leave the scope resumable. + this.completeMidScopeCommit(options); + return { + txnTime, + }; + }, + (error) => { + // As on the synchronous path: a completion that failed (a chained store's commit, + // a replication confirmation) leaves this commit partly landed, so ownership goes + // with it rather than letting a later commit rotate on top. + this.#scopeOwned = false; + throw error; + } + ); }, (error) => { // Coordinated transactions surface conflicts as RETRY_NOW (handled in the @@ -1110,7 +1119,7 @@ export class DatabaseTransaction implements Transaction { // CLOSED wrapper forever. // A failed commit must never be followed by a resumed segment: this generation is // finished and its durability is unknown, so ownership goes with it. - this.#scopeOwned = false; + this.endScopeOwnership(); this.releaseContext(!!options.doneWriting); throw error; } @@ -1122,7 +1131,7 @@ export class DatabaseTransaction implements Transaction { cleanupUnusedBlobs(write.savedBlobs, collectRetainedFileIds(write.store.getEntry(write.key)?.value)); } this.clearWrites(); - if (options.doneWriting) this.#scopeOwned = false; + if (options.doneWriting) this.endScopeOwnership(); this.releaseContext(!!options.doneWriting); const txnResolution: CommitResolution = { txnTime: this.timestamp, @@ -1135,15 +1144,25 @@ export class DatabaseTransaction implements Transaction { options.transaction ? { ...options, transaction: undefined } : options ); if ((nextResolution as any)?.then) - return (nextResolution as any)?.then((nextResolution) => { - // Only once the chained store's own commit has SETTLED: rotating first would leave the - // scope resumable after a partially failed mid-scope commit. - this.completeMidScopeCommit(options); - return { - txnTime: this.timestamp, - next: nextResolution, - }; - }); + return (nextResolution as any)?.then( + (nextResolution) => { + // Only once the chained store's own commit has SETTLED: rotating first would leave the + // scope resumable after a partially failed mid-scope commit. + this.completeMidScopeCommit(options); + return { + txnTime: this.timestamp, + next: nextResolution, + }; + }, + (error) => { + // A chained store's commit failed, so this multi-store commit half-landed. Surrender + // ownership as the head's own failure branch does: a handler that catches this and + // commits again must not rotate on top of it, and must not have the failed link + // dropped from the chain before its abort can clean up its blobs. + this.#scopeOwned = false; + throw error; + } + ); txnResolution.next = nextResolution as any; } this.completeMidScopeCommit(options); @@ -1174,6 +1193,12 @@ export class DatabaseTransaction implements Transaction { * rollback of the rotated head — the cross-store leftover this rotation exists to prevent. Every * commit path must run this, and none may do one half without the other. */ + /** Both scope flags leave together, so no exit can clear one and keep the other. */ + private endScopeOwnership(): void { + this.#scopeOwned = false; + this.snapshotFree = false; + } + private completeMidScopeCommit(options: CommitOptions): void { this.next = null; this.rotateAfterMidScopeCommit(options); @@ -1199,8 +1224,7 @@ export class DatabaseTransaction implements Transaction { cleanupUnusedBlobs(write.savedBlobs, collectRetainedFileIds(write.store.getEntry(write.key)?.value)); } } finally { - this.#scopeOwned = false; // the scope is over; nothing may rotate this instance again - this.snapshotFree = false; + this.endScopeOwnership(); // the scope is over; nothing may rotate this instance again this.clearWrites(); // A timeout-poisoned abort (abortDueToTimeout()) is the one abort that is NOT "reuse-free": // Resource.ts's dispatcher deliberately keeps joining a `timedOut` transaction (instead of diff --git a/unitTests/resources/resumeAfterMidScopeCommit.test.js b/unitTests/resources/resumeAfterMidScopeCommit.test.js index 6cb625859b..b48c2a634e 100644 --- a/unitTests/resources/resumeAfterMidScopeCommit.test.js +++ b/unitTests/resources/resumeAfterMidScopeCommit.test.js @@ -3,7 +3,7 @@ const { setupTestDBPath } = require('../testUtils'); const { table } = require('#src/resources/databases'); const { setMainIsWorker } = require('#js/server/threads/manageThreads'); const { transaction } = require('#src/resources/transaction'); -const { TRANSACTION_STATE } = require('#src/resources/DatabaseTransaction'); +const { DatabaseTransaction, TRANSACTION_STATE } = require('#src/resources/DatabaseTransaction'); const serverUtilities = require('#src/server/serverHelpers/serverUtilities'); // A handler that commits its own transaction mid-scope is a documented pattern, and its enclosing @@ -183,8 +183,30 @@ describe('Writes after a mid-scope commit rejoin the scope', () => { assert.deepEqual(pointReads, [2, 2, 3, 3, 4, 4], 'every read after the commit must see the latest value'); }); - // The resume is scoped by ownership, not by state: without an owning scope there is no pending - // commit, so a write must commit itself rather than stage into a transaction nobody will commit. + // The gate is ownership, not state — and the case it exists for is a DatabaseTransaction sitting OPEN + // in a context that no transaction() scope owns: crash-recovery replay (replayLogs.ts) commits in a + // loop at timestamp boundaries, and Table.ts builds one directly. If the gate regressed to "anything + // rotates", replay would stage its post-commit writes into a generation its loop never commits. The + // released-slot test below cannot see that: it goes through the placeholder, so txnForContext builds + // an ImmediateTransaction and never consults ownership at all. + rocksOnly('does not rotate a transaction no scope owns', async function () { + const context = { transaction: new DatabaseTransaction() }; + await A.put('unowned-1', { v: 1 }, context); + assert.ok(context.transaction, 'premise: the unowned transaction is still the context’s'); + await context.transaction.commit(); + assert.notEqual( + context.transaction.open, + TRANSACTION_STATE.OPEN, + 'an unowned transaction must stay closed after its own commit — nothing guarantees another commit' + ); + await A.put('unowned-2', { v: 2 }, context); + assert.ok(await A.get('unowned-1')); + assert.ok( + await A.get('unowned-2'), + 'a write after an unowned transaction’s commit must commit itself, not stage into a generation nobody commits' + ); + }); + it('does not resume a transaction with no owning scope', async function () { const context = {}; await transaction(context, async () => { From ab8fbe708e06dd3ea2c65f601757d267812d433a Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 20 Aug 2026 08:49:04 -0600 Subject: [PATCH 3/5] fix(txn): route both rejection exits through endScopeOwnership, cover the chained failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review caught the same asymmetry in the fix that introduced the helper: the two new completion-rejection exits set `#scopeOwned` directly, so they surrendered ownership while leaving `snapshotFree` set. There is now exactly one direct write to that field (inside the helper), and every exit goes through it. Adds the test that was missing for the chained-database failure itself: the head commits, the chained store's commit is forced to fail terminally, the handler catches it, and the scope must not be rotated — a later write commits itself instead of staging on a half-landed multi-store commit. Co-Authored-By: Claude Opus 5 --- resources/DatabaseTransaction.ts | 4 +- .../resumeAfterMidScopeCommit.test.js | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/resources/DatabaseTransaction.ts b/resources/DatabaseTransaction.ts index ec1434c354..4a2676209d 100644 --- a/resources/DatabaseTransaction.ts +++ b/resources/DatabaseTransaction.ts @@ -1036,7 +1036,7 @@ export class DatabaseTransaction implements Transaction { // As on the synchronous path: a completion that failed (a chained store's commit, // a replication confirmation) leaves this commit partly landed, so ownership goes // with it rather than letting a later commit rotate on top. - this.#scopeOwned = false; + this.endScopeOwnership(); throw error; } ); @@ -1159,7 +1159,7 @@ export class DatabaseTransaction implements Transaction { // ownership as the head's own failure branch does: a handler that catches this and // commits again must not rotate on top of it, and must not have the failed link // dropped from the chain before its abort can clean up its blobs. - this.#scopeOwned = false; + this.endScopeOwnership(); throw error; } ); diff --git a/unitTests/resources/resumeAfterMidScopeCommit.test.js b/unitTests/resources/resumeAfterMidScopeCommit.test.js index b48c2a634e..0bad8b5880 100644 --- a/unitTests/resources/resumeAfterMidScopeCommit.test.js +++ b/unitTests/resources/resumeAfterMidScopeCommit.test.js @@ -146,6 +146,44 @@ describe('Writes after a mid-scope commit rejoin the scope', () => { }); // RocksDB only: LMDB does not guarantee read-after-write within a transaction. + // A CHAINED store's commit can fail while the head's succeeded, leaving the multi-store commit half + // landed. A handler that catches that (the shape people write, because a mid-scope commit can + // conflict) must not get a rotated scope on its next commit: the failed link would be dropped from + // the chain before its abort could clean up, and the rest of the scope would stage on a partial commit. + rocksOnly('surrenders ownership when a chained database’s commit fails', async function () { + const { Transaction } = require('@harperfast/rocksdb-js'); + const originalCommit = Transaction.prototype.commit; + const chainedDb = Other.primaryStore.store.db; + let forcedFailures = 0; + const context = {}; + try { + await transaction(context, async () => { + await A.put('chain-fail-head', { v: 1 }, context); + await Other.put('chain-fail-link', { v: 1 }, context); + Transaction.prototype.commit = function (...args) { + if (this.store?.db !== chainedDb) return originalCommit.apply(this, args); + forcedFailures++; + Transaction.prototype.commit = originalCommit; // one forced failure only + return Promise.reject(Object.assign(new Error('forced chained failure'), { code: 'ERR_CORRUPTION' })); + }; + await assert.rejects(context.transaction.commit(), /forced chained failure/); + assert.notEqual( + context.transaction?.open, + TRANSACTION_STATE.OPEN, + 'a half-landed multi-store commit must not leave the scope rotated' + ); + await A.put('chain-fail-after', { v: 2 }, context); + }); + } finally { + Transaction.prototype.commit = originalCommit; + } + assert.ok(forcedFailures > 0, 'premise: the chained commit must actually have failed'); + assert.ok( + await A.get('chain-fail-after'), + 'a write after a half-landed commit must commit itself rather than stage into a rotated generation' + ); + }); + rocksOnly('reads its own post-commit writes', async function () { const context = {}; await transaction(context, async () => { From 882380ad1fcaf6d9821cfd2c671448a23c2c437c Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 20 Aug 2026 08:57:58 -0600 Subject: [PATCH 4/5] fix(txn): surrender ownership on a synchronous chained-commit throw too Round-3 review: `this.next.commit()` throwing synchronously reached neither the async chained rejection handler nor the outer one, so after the head had already committed the scope stayed owned and a later commit could rotate on top of a half-landed multi-store commit. The chained commit is now wrapped, and a sync throw surrenders ownership the same way the async rejection does. Two smaller review points: `snapshotFree` is a real initialized field rather than `declare`, so rotation no longer adds a hidden-class transition to every DatabaseTransaction; and the rotated blind-write read options are one shared frozen object instead of an allocation per native transaction. Co-Authored-By: Claude Opus 5 --- resources/DatabaseTransaction.ts | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/resources/DatabaseTransaction.ts b/resources/DatabaseTransaction.ts index 4a2676209d..7c7d831e8a 100644 --- a/resources/DatabaseTransaction.ts +++ b/resources/DatabaseTransaction.ts @@ -16,6 +16,8 @@ import type { Entry } from './RecordEncoder.ts'; import { toBufferKey } from 'ordered-binary'; const trackedTxns = new Set(); +// Read options for a rotated generation's native transactions; shared because they never vary. +const SNAPSHOT_FREE = Object.freeze({ disableSnapshot: true }); // Logical transactions the monitor supervises for their WRITES, kept apart from trackedTxns because the // two have different units and different consumers: trackedTxns is per-link, bounds a read snapshot, and // is what the read-queue-depth metric counts, while this holds one entry per logical transaction — the @@ -396,7 +398,7 @@ export class DatabaseTransaction implements Transaction { // transaction it opens from then on reads WITHOUT a snapshot. Committing mid-scope is how a handler // asks to stop reading a pinned snapshot, so re-pinning one for the rest of the scope would take // back what it asked for. - declare snapshotFree?: boolean; + snapshotFree = false; getReadTxn(disableSnapshot?: boolean): ReadTransaction { this.readTxnRefCount = (this.readTxnRefCount || 0) + 1; @@ -737,7 +739,7 @@ export class DatabaseTransaction implements Transaction { if (!transaction) { transaction = new RocksTransaction( operation.store.store as RocksStore, - this.snapshotFree ? { disableSnapshot: true } : undefined + this.snapshotFree ? SNAPSHOT_FREE : undefined ); if (operation.store.rootStore !== this.db.rootStore) { harperLogger.warn?.('Created new transaction in save, but the store does match existing store', transaction.id); @@ -1140,9 +1142,16 @@ export class DatabaseTransaction implements Transaction { // now run any other transactions options.timestamp = this.timestamp; // as above: the next store must not inherit this store's explicit native transaction - const nextResolution = this.next?.commit( - options.transaction ? { ...options, transaction: undefined } : options - ); + let nextResolution; + try { + nextResolution = this.next?.commit(options.transaction ? { ...options, transaction: undefined } : options); + } catch (error) { + // A synchronous throw reaches neither rejection handler below, and the head has already + // committed — surrender ownership here too, or the scope stays resumable on top of a + // half-landed multi-store commit. + this.endScopeOwnership(); + throw error; + } if ((nextResolution as any)?.then) return (nextResolution as any)?.then( (nextResolution) => { From 035819140356706911b070adf5a035a67854d6f9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 20 Aug 2026 09:11:49 -0600 Subject: [PATCH 5/5] test(txn): cover the synchronous chained throw, make the ambient test real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two test-honesty gaps from round 4. The chained-failure case only rejected a promise, so the synchronous-throw cleanup the previous commit added — which leaves the chained commit through different code — was untested. It is now parameterised over both. The ambient operation-handler test was titled as verifying resumption but its body never committed mid-scope, so it exercised no rotation at all. It now commits mid-scope under processLocalTransaction, writes after it, and throws — the central-manager shape — asserting the pre-commit write stays and the post-commit write rolls back. Co-Authored-By: Claude Opus 5 --- .../resumeAfterMidScopeCommit.test.js | 110 +++++++++++------- 1 file changed, 68 insertions(+), 42 deletions(-) diff --git a/unitTests/resources/resumeAfterMidScopeCommit.test.js b/unitTests/resources/resumeAfterMidScopeCommit.test.js index 0bad8b5880..f6173c6c60 100644 --- a/unitTests/resources/resumeAfterMidScopeCommit.test.js +++ b/unitTests/resources/resumeAfterMidScopeCommit.test.js @@ -2,7 +2,7 @@ const assert = require('assert'); const { setupTestDBPath } = require('../testUtils'); const { table } = require('#src/resources/databases'); const { setMainIsWorker } = require('#js/server/threads/manageThreads'); -const { transaction } = require('#src/resources/transaction'); +const { transaction, contextStorage } = require('#src/resources/transaction'); const { DatabaseTransaction, TRANSACTION_STATE } = require('#src/resources/DatabaseTransaction'); const serverUtilities = require('#src/server/serverHelpers/serverUtilities'); @@ -150,39 +150,50 @@ describe('Writes after a mid-scope commit rejoin the scope', () => { // landed. A handler that catches that (the shape people write, because a mid-scope commit can // conflict) must not get a rotated scope on its next commit: the failed link would be dropped from // the chain before its abort could clean up, and the rest of the scope would stage on a partial commit. - rocksOnly('surrenders ownership when a chained database’s commit fails', async function () { - const { Transaction } = require('@harperfast/rocksdb-js'); - const originalCommit = Transaction.prototype.commit; - const chainedDb = Other.primaryStore.store.db; - let forcedFailures = 0; - const context = {}; - try { - await transaction(context, async () => { - await A.put('chain-fail-head', { v: 1 }, context); - await Other.put('chain-fail-link', { v: 1 }, context); - Transaction.prototype.commit = function (...args) { - if (this.store?.db !== chainedDb) return originalCommit.apply(this, args); - forcedFailures++; - Transaction.prototype.commit = originalCommit; // one forced failure only - return Promise.reject(Object.assign(new Error('forced chained failure'), { code: 'ERR_CORRUPTION' })); - }; - await assert.rejects(context.transaction.commit(), /forced chained failure/); - assert.notEqual( - context.transaction?.open, - TRANSACTION_STATE.OPEN, - 'a half-landed multi-store commit must not leave the scope rotated' - ); - await A.put('chain-fail-after', { v: 2 }, context); - }); - } finally { - Transaction.prototype.commit = originalCommit; - } - assert.ok(forcedFailures > 0, 'premise: the chained commit must actually have failed'); - assert.ok( - await A.get('chain-fail-after'), - 'a write after a half-landed commit must commit itself rather than stage into a rotated generation' - ); - }); + // A rejected promise and a synchronous throw leave the chained commit through different code, so both + // have to surrender ownership. + for (const mode of ['rejects', 'throws synchronously']) { + rocksOnly(`surrenders ownership when a chained database’s commit ${mode}`, async function () { + const { Transaction } = require('@harperfast/rocksdb-js'); + const originalCommit = Transaction.prototype.commit; + const chainedDb = Other.primaryStore.store.db; + const tag = mode === 'rejects' ? 'reject' : 'throw'; + let forcedFailures = 0; + const context = {}; + try { + await transaction(context, async () => { + await A.put(`chain-${tag}-head`, { v: 1 }, context); + await Other.put(`chain-${tag}-link`, { v: 1 }, context); + Transaction.prototype.commit = function (...args) { + if (this.store?.db !== chainedDb) return originalCommit.apply(this, args); + forcedFailures++; + Transaction.prototype.commit = originalCommit; // one forced failure only + const error = Object.assign(new Error(`forced chained ${tag}`), { code: 'ERR_CORRUPTION' }); + if (mode === 'rejects') return Promise.reject(error); + throw error; + }; + await assert.rejects( + (async () => context.transaction.commit())(), + new RegExp(`forced chained ${tag}`), + 'the failure must reach the caller either way' + ); + assert.notEqual( + context.transaction?.open, + TRANSACTION_STATE.OPEN, + 'a half-landed multi-store commit must not leave the scope rotated' + ); + await A.put(`chain-${tag}-after`, { v: 2 }, context); + }); + } finally { + Transaction.prototype.commit = originalCommit; + } + assert.ok(forcedFailures > 0, 'premise: the chained commit must actually have failed'); + assert.ok( + await A.get(`chain-${tag}-after`), + 'a write after a half-landed commit must commit itself rather than stage into a rotated generation' + ); + }); + } rocksOnly('reads its own post-commit writes', async function () { const context = {}; @@ -314,14 +325,29 @@ describe('Writes after a mid-scope commit rejoin the scope', () => { for (let i = 0; i < 4; i++) assert.ok(await A.get(`ckpt-${i}`), `ckpt-${i} must persist`); }); - it('resumes under an ambient operation-handler context too', async function () { - await serverUtilities.processLocalTransaction( - { body: { operation: 'test_registered_op', hdb_user: { username: 'internal_bookkeeping' } } }, - async () => { - await A.put('ambient', { v: 1 }); - return { message: 'ok' }; - } + // The ambient operation-handler context is the shape central-manager runs in, so the rotation has to + // hold there and not only on an explicit `transaction(context, …)` scope. + rocksOnly('rotates under an ambient operation-handler context', async function () { + await assert.rejects( + serverUtilities.processLocalTransaction( + { body: { operation: 'test_registered_op', hdb_user: { username: 'internal_bookkeeping' } } }, + async () => { + const context = contextStorage.getStore(); + await transaction(context, async () => { + await A.put('ambient-pre', { v: 1 }, context); + await context.transaction.commit(); + await A.put('ambient-post', { v: 2 }, context); + throw new Error('forced ambient failure'); + }); + } + ), + /forced ambient failure/ + ); + assert.ok(await A.get('ambient-pre'), 'what the mid-scope commit committed must stay committed'); + assert.equal( + await A.get('ambient-post'), + undefined, + 'a write after the mid-scope commit must roll back under an ambient context too' ); - assert.ok(await A.get('ambient'), 'the ambient path must be unaffected'); }); });