Enforce a record lock's transaction ordering instead of taking it best-effort

integrationTests/resources/record-lock-concurrency.test.ts

@@ -147,4 +147,21 @@ suite(`record locks serialize across ${WORKERS} workers`, { skip: skipSuite }, (
 		const attempt = await post('LockedIncrement', { id, timeout: 300 });
 		strictEqual(attempt.status, 423, `locked: ${JSON.stringify(attempt.body)}`);
 	});
+
+	test('a lock taken after a write in the same transaction is refused when a write beat it there (409)', async () => {
+		// The 409 has to reach the client as the request status, not only as an internal throw.
+		const clean = 'mixed-clean';
+		await put(`${clean}-scratch`, { n: 0, holders: 0 });
+		await put(clean, { n: 0, holders: 0 });
+		const taken = await post('MixedTransactionLock', { id: clean });
+		strictEqual(taken.status, 200, `lock after a write still works: ${JSON.stringify(taken.body)}`);
+		strictEqual((await get(clean)).body?.n, 1, 'the increment made under that lock landed');
+
+		const contended = 'mixed-contended';
+		await put(`${contended}-scratch`, { n: 0, holders: 0 });
+		await put(contended, { n: 0, holders: 0 });
+		const refused = await post('MixedTransactionLock', { id: contended, interleave: true });
+		strictEqual(refused.status, 409, `refused: ${JSON.stringify(refused.body)}`);
+		strictEqual((await get(contended)).body?.n, 99, 'the write that beat the lock stands, unclobbered');
+	});
 });

integrationTests/resources/record-lock-concurrency/resources.js

@@ -9,7 +9,9 @@
 //   see each other, so holders overlap across threads and increments are lost.
 // POST /LockHold/ { id, lease } — take a held lock ({ hold: true }) and return without releasing it;
 //   only its lease ends it (the abandoned-holder case).
+// POST /MixedTransactionLock/ { id, interleave? } — lock after a write in the same transaction; see below.
 import { threadId } from 'node:worker_threads';
+import { setTimeout } from 'node:timers/promises';
 
 let workerMutex = Promise.resolve();
 function withWorkerMutex(fn) {
@@ -57,3 +59,22 @@ export class LockHold extends Resource {
 		};
 	}
 }
+
+// POST /MixedTransactionLock/ { id, interleave? }
+//   Locks a record after deleting the scratch row in the same request transaction (REST wraps the
+//   handler in transaction(request, ...)). `interleave` lands a plain write in between, which the
+//   lock must refuse 409 rather than commit behind.
+export class MixedTransactionLock extends Resource {
+	static loadAsInstance = false;
+
+	async post(query, body) {
+		const id = body?.id ?? 'default';
+		await tables.Counter.delete(`${id}-scratch`);
+		await setTimeout(20);
+		if (body?.interleave) await transaction({ sourceApply: true }, () => tables.Counter.put({ id, n: 99 }));
+		const record = await tables.Counter.lock(id);
+		record.set('n', (record.getProperty('n') ?? 0) + 1);
+		await record.save();
+		return { id, n: record.getProperty('n'), worker: threadId };
+	}
+}

resources/DatabaseTransaction.ts

@@ -692,6 +692,10 @@ export class DatabaseTransaction implements Transaction {
 	// LMDBTransaction's save() is a no-op — its commit applies `writes` — so there a write can only be
 	// committed by the transaction that holds it.
 	stagesWriteOnSave = true;
+	// Whether a write may have reached `this.transaction`, which is what makes aborting the handle
+	// lossy. Conservative by construction — a commit handler that returns without staging can still set
+	// it — so it gates only the snapshot release, never the clock. Owned by the handle, clears with one.
+	nativeWritesStaged = false;
 	validated = 0;
 	timestamp = 0;
 	retries = 0;
@@ -845,6 +849,7 @@ export class DatabaseTransaction implements Transaction {
 	// Monitor state is not ownership state: it stays with `trackedTxns.add` in getReadTxn().
 	private attachOwnedTransaction(transaction: RocksTransactionWithRetry): void {
 		this.transaction = transaction;
+		this.nativeWritesStaged = false;
 		readTransactionOwners.set(transaction, this);
 		this.rangeReadActive = false;
 		this.readTxnsUsed = 1;
@@ -888,6 +893,7 @@ export class DatabaseTransaction implements Transaction {
 		trackedTxns.delete(this);
 		this.endWriteSupervision();
 		this.transaction = null;
+		this.nativeWritesStaged = false;
 		this.rangeReadActive = false;
 		this.readTxnsUsed = 0;
 		this.readTxnRefCount = 0;
@@ -954,6 +960,46 @@ export class DatabaseTransaction implements Transaction {
 		if (this.#context?.transaction === this) this.#context.transaction = RELEASED_TRANSACTION;
 	}
 
+	/**
+	 * Bring the commit clock up to a record lock's acquisition, or report the version that makes the
+	 * lock unsafe without it — see resources/record-locks.md. A caller's own clock is never moved.
+	 */
+	lockOrderingConflict(store: any, key: unknown, acquiredAt: number, callerTimestamp?: number): number | undefined {
+		if (this.timestamp >= acquiredAt) return undefined;
+		if (!(callerTimestamp > 0 && this.timestamp === callerTimestamp) && this.#raiseClockTo(acquiredAt))
+			return undefined;
+		const committedVersion = store.getEntry(key)?.version;
+		return committedVersion >= this.timestamp ? committedVersion : undefined;
+	}
+
+	/**
+	 * The handle is asked first so the record version, the transaction and the log key never disagree,
+	 * and it is asked rather than inferred: a commit handler can return without staging and without
+	 * marking itself skipped, and refusing a lock whose clock was still movable is a false 409.
+	 */
+	#raiseClockTo(timestamp: number): boolean {
+		if (this.transaction) {
+			try {
+				this.transaction.setTimestamp(timestamp);
+			} catch (error) {
+				if ((error as { code?: string })?.code !== 'ERR_TIMESTAMP_FROZEN') throw error;
+				return false;
+			}
+		}
+		this.timestamp = timestamp;
+		return true;
+	}
+
+	/**
+	 * Give up the read snapshot so what follows reads, and commits against, current state. Refused once
+	 * the handle carries a write or an iterator still holds it — either makes aborting it lossy.
+	 */
+	releaseReadSnapshotForLock(): void {
+		if (this.nativeWritesStaged || this.readTxnsUsed > 1) return;
+		this.releaseReadTxn();
+		this.snapshotFree = true;
+	}
+
 	disregardReadTxn(): void {
 		// Never release a handle carrying staged writes: commit() skips re-staging a write it has marked
 		// saved, so aborting the handle here drops it. The count is clamped because every getReadTxn()
@@ -1390,6 +1436,7 @@ export class DatabaseTransaction implements Transaction {
 		try {
 			completion = operation.commit(writeVersion, operation.entry, this.retries > 0, transaction) as Promise<void>;
 		} finally {
+			if (transaction === this.transaction && !operation.skipped) this.nativeWritesStaged = true;
 			closeWriteInstance(operation);
 		}
 		if (operation.trackRecordVersion)

resources/Table.ts

@@ -45,6 +45,7 @@ import {
 	acquireRecordKey,
 	lockAttemptKey,
 	lockNotHeldError,
+	lockOrderingError,
 	resolveLockOptions,
 	type RecordLockHandle,
 	type RecordLockOptions,
@@ -3520,32 +3521,38 @@ export function makeTable(options) {
 						throw new ServerError('Transaction was closed while waiting for a record lock', 500);
 					}
 				}
-				link.registerRecordLock(handle);
-				if (link.saveCommits && (context as any)?.timestamp) handle.noteCandidateFloor((context as any).timestamp);
 				if (link.open === TRANSACTION_STATE.OPEN && !link.saveCommits) {
-					// Explicit transaction() (not ImmediateTransaction): pin the clock to
-					// acquiredAt when no writes have been staged yet.  When writes already
-					// exist, leave the clock alone (ordering is best-effort; write held records
-					// in their own transaction for the guarantee).  ImmediateTransaction is
-					// excluded (saveCommits=true) — its clock is never pinned in lock();
-					// each save() stamps from the handle's committed version floor instead.
-					if (link.writes.length === 0 && !link.timestamp) {
-						link.timestamp = handle.acquiredAt;
+					// Before registerRecordLock: releaseRecordLocks() skips hold handles, so a
+					// registered-then-refused hold would keep the native key for its whole lease.
+					let conflictingVersion: number | undefined;
+					try {
+						conflictingVersion = link.lockOrderingConflict(
+							primaryStore,
+							id,
+							handle.acquiredAt,
+							(context as any)?.timestamp
+						);
+					} catch (error) {
+						handle.release();
+						throw error as Error;
 					}
-					if (!resolved.hold && link.transaction) {
-						// Scoped lock: the read snapshot may predate the lock; drop it so the
-						// scope reads what it locked.  Hold locks use acquiredAt directly and
-						// do not update the read snapshot.
-						// The timestamp guard matches DatabaseTransaction's own setTimestamp calls: a
-						// deferred update() write leaves the clock at 0, which rocksdb-js rejects.
-						if (link.writes.length === 0 && link.readTxnsUsed <= 1) {
-							link.releaseReadTxn();
-							link.snapshotFree = true;
-						} else if (link.timestamp) link.transaction.setTimestamp(link.timestamp);
+					if (conflictingVersion !== undefined) {
+						handle.release();
+						throw lockOrderingError(
+							`${databaseName}.${tableName}`,
+							link.timestamp,
+							handle.acquiredAt,
+							conflictingVersion
+						);
 					}
 				}
-				// ImmediateTransaction: no clock pinning in lock(); save() stamps each write
-				// from the committed handle floor for both scoped and hold handles.
+				link.registerRecordLock(handle);
+				if (link.saveCommits && (context as any)?.timestamp) handle.noteCandidateFloor((context as any).timestamp);
+				if (link.open === TRANSACTION_STATE.OPEN && !link.saveCommits && !resolved.hold && link.transaction) {
+					// The snapshot is also the scope's commit basis, so one predating the lock refuses the
+					// scope's own write against the record #reloadLocked just re-read fresh.
+					link.releaseReadSnapshotForLock();
+				}
 				return handle;
 			});
 			link.registerPendingLock(primaryStore, keyId, acquisition);

resources/record-locks.md

@@ -21,10 +21,8 @@ out-of-order duplicates and discard.
 **Phase 0 contract.** `lock()` is mutually exclusive only with other `lock()` calls on the same key.
 Plain writes (`put`, `patch`, `delete`, `create`, `invalidate`, `relocate`) are never gated, parked,
 or restaged — they proceed immediately at real wall-clock time. A holder write starts at the lock
-acquisition time, so a later plain write wins under LWW unless the holder first wrote through an
-unpinned mixed explicit transaction. That transaction's later timestamp becomes the handle's floor
-so the holder cannot lose its own subsequent writes; ordering against plain writes between acquisition
-and that transaction timestamp is best-effort. Use `lock()` when the caller needs to read-then-
+acquisition time, so a later plain write wins under LWW, and one that landed **before** the
+acquisition is never ordered after it (harper#2712, below). Use `lock()` when the caller needs to read-then-
 conditionally-write without another holder interleaving, not to serialize arbitrary writers.
 
 Consequences that shape the code:
@@ -78,14 +76,53 @@ Consequences that shape the code:
   takes the record from there — the same basis a chained write picks up (harper#1968): the record
   comes from the prior staged write, the rest of the entry (version, audit chain, blob metadata) stays
   the pre-transaction one.
-- **Scoped lock in an explicit `transaction()` scope.** After acquisition, when no writes have been
-  staged yet (`link.writes.length === 0`), the transaction clock is pinned to `acquiredAt` so the
-  holder write wins over any pre-lock concurrent write. When prior staged writes already exist,
-  ordering is best-effort — no 409 is thrown. In an `ImmediateTransaction` context (no explicit
-  scope), a scoped lock's writes go through the same `update()`-style staging as above; each explicit
-  `update()`+`save()` cycle is stamped with `nextHolderVersion()` independently. A scoped lock acquired
-  outside any explicit `transaction()` scope persists until `unlock()` or the lease expires
-  (ImmediateTransaction's `releaseRecordLocks()` is a no-op for record locks).
+- **A write made under a lock is never ordered behind one made before the lock (harper#2712).** That
+  is the enforced statement, and it is weaker than "the transaction commits at or after the
+  acquisition": a transaction whose clock is already frozen below the acquisition still commits
+  there, it is only refused when something actually landed in the window that leaves open. An
+  explicit scope has one commit clock for every write it makes, and `lock()` moves it up to
+  `handle.acquiredAt` on each fresh acquisition — the second lock in a scope too, because a clock left
+  at the first acquisition lets a plain write made between the two outrank the second lock's own
+  write. Enforced in `DatabaseTransaction.lockOrderingConflict()`, called from `lock()` before
+  `registerRecordLock`. The native transaction is the authority on whether the clock can still move:
+  rocksdb-js freezes it (`ERR_TIMESTAMP_FROZEN`) once a write or transaction-log entry is staged,
+  because the producer has already encoded the old value into that write's record bytes, so
+  `this.timestamp` follows `setTimestamp()` rather than leading it and the record version, the native
+  transaction and the log batch key stay on one value. The probe is made rather than inferred: a commit
+  handler can return without staging and without marking itself skipped, and a flag that read true
+  there would refuse a lock whose clock was still movable. `nativeWritesStaged` is that conservative
+  flag, and it gates only the snapshot release below, where being wrong costs nothing. Two clocks therefore do not move: a frozen one, and one the caller put on
+  the context (`transaction({ timestamp })`), which is their own ordering choice. Either way the
+  record's committed version is read at the acquisition and the lock is refused **409** when something
+  already landed inside the window — the interleave that used to be taken silently. With nothing there
+  the lock proceeds, so `put(); lock()` in one scope still works; it is a conflict, not a ban on
+  locking after writing, and that is deliberate: `put`, `patch`, `create` and `delete` all stage
+  natively at once in an explicit scope, so an unconditional refusal would reject every such caller
+  while refusing nothing that is actually unsafe.
+  Every exit from the check releases the native key and any cluster admission before it throws, which
+  is why it runs ahead of `registerRecordLock`: `releaseRecordLocks()` deliberately skips `hold`
+  handles, so a registered-then-refused hold would keep the key for its whole lease.
+  **Not enforced:** a plain write carrying a caller-supplied _future_ timestamp can still outrank a
+  later holder — `transaction({ timestamp })` puts any value on the clock; a transaction holding two
+  locks commits both writes at the later acquisition, so a plain write made between them loses to the
+  first lock's write as well; and on a table that keeps no tombstone a concurrent delete leaves no
+  version to compare against, which is also why nothing is left for the locked write to lose to.
+  Pinned by "mixed-transaction lock ordering (harper#2712)" in `unitTests/resources/recordLock.test.js`
+  and by the mixed-transaction case in `integrationTests/resources/record-lock-concurrency.test.ts`.
+- **Scoped lock in an explicit `transaction()` scope.** After acquisition the read snapshot is
+  released (`DatabaseTransaction.releaseReadSnapshotForLock()`) so the scope reads — and takes its
+  commit basis from — what it locked rather than a snapshot that predates it; a stale basis makes the
+  scope's own conditional put refuse against a record `#reloadLocked` had just re-read fresh, which is
+  the same write loss by another route. It is refused only once a write has reached the native handle
+  (aborting it would drop that write, the same thing `disregardReadTxn()` refuses) or while read
+  iterators still hold the handle. **Not enforced:** in those two cases, and for a `hold` lock (which
+  writes from `acquiredAt` and leaves the snapshot alone), the stale basis survives and such a write
+  can still be refused at commit without an error reaching the caller. In an `ImmediateTransaction`
+  context (no explicit scope), a scoped lock's writes go through the same `update()`-style staging as
+  above; each explicit `update()`+`save()` cycle is stamped with `nextHolderVersion()` independently,
+  which is never below `acquiredAt`, so the clock rule above has nothing to enforce there. A scoped
+  lock acquired outside any explicit `transaction()` scope persists until `unlock()` or the lease
+  expires (ImmediateTransaction's `releaseRecordLocks()` is a no-op for record locks).
 - **Scoped → hold upgrade.** Calling `lock(id, { hold: true })` while the same transaction already
   holds a scoped lock on the same key upgrades it via `handle.upgradeToHold(lease)`, which flips the
   existing handle object to hold mode in place (new lease timer, `nextHolderVersion()` primed) rather
@@ -148,15 +185,13 @@ never assigned to the link clock: pinning `link.timestamp` would stamp every OTH
 same context before the commit resets it — a concurrent write in the caller's own `Promise.all`, an
 off-key write through the locked instance, the next operation in a retry or replay save loop — with
 the lock's acquisition time, which LWW then silently drops against a newer record version. In an
-explicit OPEN transaction, when the hold is the first write (no prior staged writes), the transaction
-clock is pinned to `handle.acquiredAt`; subsequent saves reuse that pinned clock. When non-hold writes
-were staged before the lock was acquired, the clock is left alone (best-effort ordering; no 409 is
-thrown for the mixed-write case). After a lock-backed record change commits, its transaction timestamp
-advances `handle.noteHolderVersion()`, so a surviving hold's later saves advance past that version
-rather than going backwards and being dropped by LWW. A skipped or rolled-back change never advances
-the floor. Consequently, a mixed transaction can
-make later holder writes outrank a plain write whose timestamp falls between `acquiredAt` and the mixed
-transaction timestamp. A caller-supplied future `context.timestamp` likewise remains the handle's floor
+explicit OPEN transaction the clock is moved up to `handle.acquiredAt` at every fresh acquisition
+(harper#2712) and subsequent saves reuse it; a clock a staged write has already frozen below that
+point, or one the caller set on the context, is left alone and the lock is refused 409 if anything
+landed in the window it leaves open. After a lock-backed record change commits, its transaction
+timestamp advances `handle.noteHolderVersion()`, so a surviving hold's later saves advance past that
+version rather than going backwards and being dropped by LWW. A skipped or rolled-back change never
+advances the floor. A caller-supplied future `context.timestamp` remains the handle's floor
 for the life of the lease; clamping it would put the next holder write behind the handle's own committed
 version and recreate the silent-drop bug.
 
@@ -1026,8 +1061,9 @@ feature that has not yet been measured.
 >    - **(1a) clock skew.** The predecessor's clock ran ahead of its successor's, and its write is
 >      still in flight when the successor writes.
 >    - **(1b) a timestamp pushed ahead on purpose, on a completely clean handoff.** A caller-supplied
->      future `context.timestamp`, or a mixed explicit transaction whose later timestamp becomes the
->      handle's floor — both documented as deliberate Phase 0 behavior (`DESIGN.md:305-309`). The
+>      future `context.timestamp`, which becomes the handle's floor — documented as deliberate Phase 0
+>      behavior. (A mixed explicit transaction no longer reaches here from below the acquisition:
+>      harper#2712 raises that clock, or refuses the lock.) The
 >      predecessor commits, replicates, drains and releases cleanly; the successor is admitted with
 >      correct freshness, reads the current value, and writes at real wall-clock time — and its write
 >      is the older one, silently, because the stored version is stamped in the future. What the