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

integrationTests/fixtures/transaction-context-reads/resources.js

@@ -97,3 +97,52 @@ 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 };
+	}
+}
+
+// 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 };
+	}
+}

integrationTests/resources/transaction-context-reads.test.ts

@@ -104,13 +104,44 @@ 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('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 },
+		});
+		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 () => {

resources/DatabaseTransaction.ts

@@ -16,6 +16,8 @@ import type { Entry } from './RecordEncoder.ts';
 import { toBufferKey } from 'ordered-binary';
 
 const trackedTxns = new Set<DatabaseTransaction>();
+// 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
@@ -325,6 +327,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<any, Map<unknown, TransactionWrite>>;
@@ -383,6 +394,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.
+	snapshotFree = false;
 
 	getReadTxn(disableSnapshot?: boolean): ReadTransaction {
 		this.readTxnRefCount = (this.readTxnRefCount || 0) + 1;
@@ -409,7 +425,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 +737,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 ? 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);
 			}
@@ -997,15 +1021,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.endScopeOwnership();
 							this.releaseContext(!!options.doneWriting);
-							this.next = null;
 							let txnTime = this.timestamp;
 							this.timestamp = 0; // reset the timestamp as well
-							return Promise.all(completions).then(() => {
-								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.endScopeOwnership();
+									throw error;
+								}
+							);
 						},
 						(error) => {
 							// Coordinated transactions surface conflicts as RETRY_NOW (handled in the
@@ -1083,6 +1119,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.endScopeOwnership();
 								this.releaseContext(!!options.doneWriting);
 								throw error;
 							}
@@ -1094,6 +1133,7 @@ export class DatabaseTransaction implements Transaction {
 						cleanupUnusedBlobs(write.savedBlobs, collectRetainedFileIds(write.store.getEntry(write.key)?.value));
 				}
 				this.clearWrites();
+				if (options.doneWriting) this.endScopeOwnership();
 				this.releaseContext(!!options.doneWriting);
 				const txnResolution: CommitResolution = {
 					txnTime: this.timestamp,
@@ -1102,16 +1142,39 @@ 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) => ({
-							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.endScopeOwnership();
+								throw error;
+							}
+						);
 					txnResolution.next = nextResolution as any;
 				}
+				this.completeMidScopeCommit(options);
 				return txnResolution;
 			},
 			(error) => {
@@ -1120,6 +1183,44 @@ 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.
+	 */
+	/** 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);
+	}
+
+	/** 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 +1233,7 @@ export class DatabaseTransaction implements Transaction {
 					cleanupUnusedBlobs(write.savedBlobs, collectRetainedFileIds(write.store.getEntry(write.key)?.value));
 			}
 		} finally {
+			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

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	 */

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;

resources/transaction.ts

@@ -45,7 +45,9 @@ export function transaction<T>(
 		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;