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

[cb1kenobi]

A record lock taken inside an explicit transaction() now enforces its ordering instead of taking it best-effort: a write made under the lock is never ordered behind a write that was made before the lock, and where that cannot be arranged the acquisition is refused 409 rather than committed silently behind the writer that beat it.

Until now lock() pinned the transaction's commit clock to the acquisition only when no write had been staged yet. With a write already staged the clock was left alone, so a plain write landing between the clock and the acquisition outranked the lock's own write under last-write-wins — the caller read the fresh record through #reloadLocked, wrote it back, and the write vanished with no error. Closes #2712.

For the human reviewer

1. The framing verdict was resolved rather than cleared, and the contract was then narrowed — this is the entry to rule on. The planning round returned Framing-Verdict: better-alternative-exists and the alternative was adopted in full: instead of pinning only a clock that is still zero and carrying a provenance field to exempt a clock an earlier lock had pinned, the native transaction is asked on every fresh acquisition whether the clock can still move. The reviewer's counterexample for the exemption was decisive and is now a test — lock A at t1, plain write to B at t2, lock B at t3: leaving the clock at t1 let the t2 write, made before B was locked at all, outrank B's own locked write. Implementation evidence then forced a second change the planning round could not see: put, patch, create and delete all stage natively at once inside an explicit scope (Table.put calls _writeUpdate and then save()), so a clock frozen below the acquisition is the ordinary state of any transaction that writes before it locks, not an edge case. Refusing all of them would 409 every such caller — including two existing tests that encode the pattern — while refusing nothing that is actually unsafe, and would leave harper#2707's enlistment API no better off than its current workaround. So what is enforced is the ordering statement, not commit timestamp >= acquiredAt: a frozen transaction still commits its locked write below its own acquisition, and is refused only when something actually landed in the window that leaves open. The graded leg raised that narrowing as a blocker on every round and asks for a human contract decision; it is kept, with that disqualifier, and the cost is asserted directly rather than implied. If you want the stronger absolute contract, it is one branch — return the conflict unconditionally when the clock cannot move — and the price is the compatibility break above.
2. Requirements assessment. The requirement is warranted and correctly sized. The issue offers "pin the clock, or throw 409" as equally acceptable and rules out only silent best-effort; this does both, choosing per acquisition on a fact the storage engine owns. The obvious alternative to fixing it here — stamping the lock's write alone through operation.lockStamp, as the ImmediateTransaction path already does — was rejected on a storage-format fact rather than taste: it would give a locally-originated write a record version newer than the transaction it is filed under, and getAppliedWriteVersion deliberately gates that dual-version representation to sourceApply || isReplay (harper#2412). Nothing in flight supersedes it. It is a prerequisite for relaxing harper#2707's "no writes in scope" enlistment workaround, which can now become "try, and handle the 409" — that follow-up is not in this PR.
3. A second lock in one scope now moves the clock, which changes a documented outcome for the first. Both locked writes commit at the later acquisition, so a plain write made between the two acquisitions now loses to the first lock's write as well as to the second. That is unavoidable — an explicit transaction has one commit clock — and it is the direction that makes the lock's own guarantee true, but it does reverse "a later plain write wins" for that narrow window. The test that asserted the old behaviour (minor-3) is rewritten rather than deleted, and the limitation is stated in the design note.
4. The clock is probed, not inferred, and that is a deliberate cost. nativeWritesStaged was originally also a fast path for the clock decision, until the final round found that _writeDelete's lost-delete guard returns without staging and without marking itself skipped, so the flag could read true for a clock that was still movable and produce a false 409. The native setTimestamp probe is now the only authority for the clock, which costs one thrown ERR_TIMESTAMP_FROZEN per acquisition in a transaction that already writes — against a native key acquisition, a possible cluster round and a store read. The flag survives only where being conservatively wrong costs nothing: it gates the read-snapshot release.
5. The read-snapshot widening is adjacent scope, taken deliberately. A scoped lock already dropped the transaction's read snapshot, but only when the transaction had staged nothing at all. The snapshot is also the commit basis, so with a deferred write present a stale one made the scope's own conditional put refuse against a record #reloadLocked had just re-read fresh — the same write loss by a different route, and without it the clock fix produces no user-visible change on that path. It can be split out if you would rather review it separately.
6. Two residual losses are documented rather than fixed. A hold lock, and any scoped lock taken while read iterators still hold the handle, keep the stale snapshot and can still have a write refused at commit with no error reaching the caller — pre-existing, strictly narrowed by this change, raised by the Gemini leg and recorded under "Not enforced" in the design note. And a concurrent delete inside the window is caught only on a table that keeps a versioned tombstone; measured on a table with auditing off, the delete leaves no entry at all, which is equally why it leaves nothing for the locked write to lose to.
7. The review is unadjudicated. The Harper-domain adjudicator was SIGKILLed on every round on this machine (exit--1, empty output, sandbox on or off), so there is no adjudicated decision ledger and Human-Review-Need floors at 4 by deterministic policy rather than by judgement. The outside coverage in the footer is real and unaffected — codex, Gemini and a Cursor lens all ran — but the dispositions in this section are the author's, taken against the graded leg's findings.
8. How this sits beside the other in-flight lock work. harper#2716 ("A defaulted lock() scope comes from the table's declaration", PR Resolve a defaulted lock() scope from the table's replicate declaration, so enabling cluster record locks cannot re-scope a plugin's node-local lock #2903) changes lock()'s scope resolution — resolveLockOptions, a rescope() helper, the coordinator and transport checks around the native wait — plus resources/DESIGN.md and the root index. This change touches only the clock decision that runs after those checks, a new method pair on DatabaseTransaction, and the Phase 0 half of resources/record-locks.md while Resolve a defaulted lock() scope from the table's replicate declaration, so enabling cluster record locks cannot re-scope a plugin's node-local lock #2903 edits its Phase 1 scope section; the two are disjoint hunks in every shared file and are expected to merge cleanly in either order. The design note also stays out of resources/DESIGN.md entirely, so it does not contend with harper#2711's prune of that file (PR Preserve a write's local-only marking on every mutation and recovery path #2902).
9. Unrelated observation, not fixed here. save(record, true) on a scoped-locked instance whose write was staged at lock() time silently stages nothing — the fullUpdate flag and the record argument are dropped and the commit handler early-returns with no changes. It behaves identically on main, so it is out of this diff, but it deserves its own issue.

Product and architecture tour

What a lock now promises

What can an application rely on when it calls table.lock() inside its own transaction()?

Before	After
The transaction's commit clock was pinned to the acquisition only if nothing had been staged yet. With a write already staged the ordering was best-effort: a plain write landing between the clock and the acquisition outranked the locked write, and the loss was silent.	The clock is raised to the acquisition wherever the storage engine still allows it. Where it does not, the record is checked and the acquisition is refused 409. Nothing is taken silently.

A write made under a record lock is never ordered behind a write that was made before the lock.
That is weaker than "the transaction commits at or after the acquisition", deliberately: a
transaction whose clock a prior write already froze still commits its locked write below its own
acquisition, and is refused only when something actually landed in the window that leaves open.

The weaker form is what makes the feature usable. put, patch, create and delete each stage natively the moment they are called inside an explicit scope, so a frozen clock is the ordinary state of any transaction that writes before it locks — not an edge case. Refusing all of them would answer 409 to every such caller while refusing nothing that is actually unsafe, and would leave the enlistment API in harper#2707 no better off than the workaround it has today.

The decision at acquisition

What decides whether an acquisition is admitted, and what has to be given back when it is not?

How an acquisition decides

Every fresh acquisition asks the native transaction whether its timestamp can still move, and only falls back to a conflict check when it cannot.

flowchart TD
    A[Native key acquired] --> B{Clock already at or above acquisition?}
    B -->|yes| OK[Register the lock]
    B -->|no| C{Clock set by the caller?}
    C -->|yes| E[Read committed version]
    C -->|no| D{Native setTimestamp accepted?}
    D -->|yes| OK
    D -->|frozen| E
    E --> F{Version inside the window?}
    F -->|no| OK
    F -->|yes| G[Release key, throw 409]

Loading

The probe is made rather than inferred. An earlier revision used a staged-write flag as a fast path here, until the final review round found that _writeDelete's lost-delete guard returns without staging and without marking itself skipped — a flag reading true there would have refused a lock whose clock was still movable. The flag survives only where being conservatively wrong costs nothing, gating the read-snapshot release.

Refusal happens before the handle is registered on the transaction, and every exceptional exit releases the native key and any cluster admission first: releaseRecordLocks() deliberately skips hold handles, so a registered-then-refused hold would have held the key for its whole lease.

Example: transfer under two locks

A transaction locks account A at t1, and a plain writer updates account B at t2. The transaction then locks B at t3 and saves both.

Outcome: the clock advances from t1 to t3, so B's locked write outranks the t2 write that preceded its lock. Before this change the clock stayed at t1 and the t2 write silently won. The cost is stated in the ledger: A's write now also commits at t3, so a plain write to A between the two acquisitions loses to it.

Why the read snapshot moves too

Why does a clock fix alone leave the commonest case unchanged?

Raising the clock alone changes nothing a caller can see on the commonest path. A scoped lock's transaction keeps a read snapshot, and that snapshot is also the basis its own conditional put commits against — so with a deferred write present, a stale basis made the scope's write refuse against the very record #reloadLocked had just re-read fresh. The same write loss, by a different route.

The snapshot was already dropped for a scoped lock, but only when the transaction had staged nothing at all. The precise condition is "no write has reached the native handle", which is the same fact the clock decision needs, so it costs one field. A hold lock, and a scoped lock taken while read iterators still hold the handle, keep the stale basis and are recorded as not enforced.

• Is the weaker ordering contract the right product answer, or should a frozen clock refuse unconditionally and make write-then-lock callers restructure?
• Should the read-snapshot change ship with this, or as its own PR against the same issue?

Changes

The ordering decision itself is two new methods on resources/DatabaseTransaction.ts: lockOrderingConflict() raises the commit clock to the acquisition, leaves a clock the caller set on the context alone, and otherwise returns the committed version that would outrank a write made under the lock. Its private helper asks the native transaction rather than inferring, because a commit handler can return without staging and without marking itself skipped, and a wrong answer there is a false 409. releaseReadSnapshotForLock() gives up the read snapshot — and with it the stale commit basis — unless the handle may already carry a write or iterators still hold it. That "may already carry a write" is the new nativeWritesStaged field, set from the write's outcome in save() and cleared with the native handle it belongs to, on attach and on detach.

resources/Table.ts calls that decision from lock()'s acquisition continuation, before registerRecordLock and releasing the handle on every exceptional exit as well as on the refusal, then replaces the old inline snapshot rule with the helper. It imports the new error from resources/recordLock.ts, which builds the caller-visible 409 naming the transaction's commit timestamp, the acquisition and the version that beat it.

resources/record-locks.md replaces the best-effort description with the enforced statement, its mechanism and the deliberate narrowing; the Phase 0 contract paragraph, the holder-version paragraph and the Phase 1 §10 limitation are corrected to match, and the limits that remain — a caller-supplied future timestamp, both writes of a two-lock transaction sharing the later acquisition, a table that keeps no tombstone, and the hold/open-iterator snapshot cases — are recorded rather than implied. The note stays in record-locks.md, so neither resources/DESIGN.md nor the root index is touched.

Verification

Route (a): the existing record-lock integration test is extended, plus unit-level coverage through the real Table/DatabaseTransaction on RocksDB for the timestamp assertions, which have no HTTP surface.

unitTests/resources/recordLock.test.js adds a mixed-transaction lock ordering (harper#2712) suite: the deferred-write clock raise and the two-lock interleave prove the ordering itself; the frozen-clock refusal, its hold variant (which also proves the native key is not leaked by a refusal) and the uncaught-refusal rollback prove the 409 and its cleanup; the caller-pinned refusal and the untouched future timestamp cover the context clock; the concurrent delete and the scoped→hold upgrade followed by a fresh lock close the two cases review raised; the ImmediateTransaction case pins that it is out of scope. The accepted cost is asserted directly — a frozen clock commits below its own acquisition, above every version visible at it — rather than left implied. minor-3 previously asserted that a second lock did not move the clock; it now asserts the opposite, which is the behaviour change in item 3 above.

integrationTests/resources/record-lock-concurrency.test.ts adds an HTTP case that a write-then-lock request is admitted when nothing intervenes and returns 409 to the client when a plain write beat it, driving a new MixedTransactionLock fixture endpoint whose handler runs in the request's own explicit transaction.

Executed on this machine at 2a7baf9f:

• npm run build — clean.
• npx mocha unitTests/resources/recordLock.test.js unitTests/resources/recordLockCluster.test.js unitTests/resources/sameKeySaveOrder.test.js unitTests/resources/immediateTransactionConflictRetry.test.js — 146 passing, 1 pending.
• npm run test:unit:resources — 3495 passing, 53 pending, 4 failing. All four reproduce on a clean 80f6d1c3 checkout against the same dependency tree: three source-apply conflict retry converges instead of spinning cases, and NativeFullTextDerivedIndexLifecycle failing with @harperfast/fulltext/native reported incompatible runtime capabilities, which this diff does not touch.
• HARPER_STORAGE_ENGINE=lmdb npm run test:unit:resources — 2701 passing, 566 pending, 1 failing (the same full-text native failure). Record locks are 501 on LMDB, so the new cases skip there.
• npm run lint:required and npm run check:design-docs (the runLinter CI job) — both clean; npx prettier --check . clean.
• Fails on base: the changed and added tests were applied alone to a throwaway worktree at the merge base 80f6d1c3 and run there — 76 passing, 8 failing, each on its own assertion rather than a setup error: clock advanced to the second acquisition, the clock sits at the acquisition, the second lock write outranks the plain write before it, lock() rejected rather than taking a backdated lock, and four more. All 84 pass on the branch.

Not run here, with the reason: npm run test:unit:main does not complete on this machine — it stalls in unitTests/components/applicationSpawn.test.js and was killed after 4h35m with no output past 527 tests; it stalls on a clean base too, and this diff is confined to resources/ (which that script excludes) plus test files. npm run test:integration:all cannot run here either: startHarper allocates per-instance loopback addresses from 127.0.0.3 up and only 127.0.0.1 is configured on this host. CI runs both.

Documentation

The 409 is a new caller-visible outcome on a public API, so HarperFast/documentation's record-lock page needs a sentence: acquiring a record lock inside an explicit transaction that has already written can fail with 409 when another writer changed the record first, and the caller should take the lock before writing or retry. No companion PR is opened from here.

Complexity: complicated

_{Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex; blocked=domain(exit--1); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=4; full=4 @ 2a7baf9}

_{Human-Review-Need: 4 @ 2a7baf9}

[gemini-code-assist]

Code Review

This pull request implements mixed-transaction lock ordering (harper#2712) to guarantee that a write made under a lock is never ordered behind a write made before the lock. It introduces conflict detection in DatabaseTransaction and Table that raises the transaction clock to the lock's acquisition time or throws a 409 error if a write has already occurred within the window. Additionally, it updates documentation and adds extensive unit and integration tests to verify these ordering behaviors. I have no feedback to provide as there are no review comments.