Cluster record locks: the Phase 1 substrate, with Ricart–Agrawala superseded by amortized ownership (#483) - #2498
Cluster record locks: the Phase 1 substrate, with Ricart–Agrawala superseded by amortized ownership (#483)#2498kriszyp wants to merge 23 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements Phase 1 of cluster-wide record locks using a Ricart-Agrawala consensus algorithm over replicated control entries. It introduces a LockCoordinator to manage lock requests, grants, and releases, integrates these control entries into the transaction log and replication flow, and updates transaction committing to enforce monotonic lease deadlines. Feedback on the changes is minimal but highlights a critical issue in resources/Table.ts where an undefined noop reference in a promise catch block could cause a runtime ReferenceError.
| // The round completed inside its lease but the lease elapsed before the handle | ||
| // could take it. The coordinator still holds it, and only this call knows the | ||
| // hold was never handed out. | ||
| Promise.resolve(coordinator.release(id)).catch(noop); |
There was a problem hiding this comment.
Is noop defined or imported in this file? If not, it will cause a ReferenceError at runtime. Since lodash is already imported, you can use lodash.noop or simply an arrow function () => {} to safely catch and ignore the rejection.
| Promise.resolve(coordinator.release(id)).catch(noop); | |
| Promise.resolve(coordinator.release(id)).catch(() => {}); |
5f7b709 to
c4db265
Compare
…l entries (Phase 1 of #483) Phase 0 made the rocksdb-js key lock the sole authority for table.lock(id), exclusive across one node's worker threads. Phase 1 layers cluster-wide exclusion on top with Ricart-Agrawala over three new transaction-log control entries, without new replication frames and without touching the record contract. - auditStore: LOCK_REQUEST/LOCK_GRANT/LOCK_RELEASE action nibbles and the isLockControlType predicate every record-activity surface filters on. - recordLockCoordinator: the per-table state machine, the ClusterLockTransport interface, the per-database registry, and payload encode/decode/validation. - Table: the control-entry writer, receiver routing off the record path, the history/fan-out filters, and the lock() integration. - recordLock/DatabaseTransaction: scope option, a monotonic lease deadline checked synchronously on every staged write and again before the native commit submits, and a contained release hook. Control entries are written with recordId null and the locked key in the payload: _writeUpdate's keyed dedup asks the transaction log for the first entry at (timestamp, tableId, recordId), and the holder's own first write is stamped at exactly ts_R, so an entry carrying the key would answer that lookup and the holder's write would be dropped as a duplicate. Node identity is the node name, never the audit nodeId, which is assigned per node and would order the same pair of requests differently on two nodes. With no transport registered nothing is allocated and behavior is Phase 0's. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
- Synthesize a missing grant only as evidence of a crash. tick() treated any expired peer round as an implied grant, including one the peer had cleanly released. A released record lingers until its bound so replay cannot resurrect it, so a waiter could complete a round the peer never granted while that peer held the key from a newer round. Require the record to be unreleased and no other live round for the same peer to remain. - Bind an entry's identity to the node that wrote it. requester and grantor came from the payload alone, so one participant could write a grant naming every other node and complete a requester's round while the real holder still held. applyEntry now takes the author from the audit header's nodeId, which the receiver translates and relays preserve. - Pack control payloads in record mode. They are read back by the receiving table's decoder, which repurposes a range of positive fixints as structure ids, so an integer record key of 64-127 was written as a bare fixint and read as a structure header. The packer keeps its own dictionary, which nothing can write to, and throws if that ever stops holding. Also: a grant or release now takes a fresh commit time instead of the round's ts_R, which could land behind a peer's catch-up cursor; the coordinator releases its round when a granted lease elapses before the handle can take it; and the per-entry type check and the commit-time lease re-fence avoid allocating. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…broke The Phase 0 suite caught both. - A deliberate unlock() must not invalidate a write already staged under that lock. The new commit-time re-fence used isExpired(), which is true for a released handle too, so a scoped lock that unlocked early failed its commit with 409. The re-fence now asks isLeaseExpired(), which is the condition that actually means the key may belong to someone else; save() keeps the wider check for writes staged after a release. - A coalesced follower re-entering with its remaining budget spread the resolved options, which turned a defaulted 'cluster' scope into an explicitly requested one and made the retry fail closed with 503 on a database with no transport. It now carries the scope only when the caller named it. Also: lock() now rejects rather than throwing synchronously for the runtime fail-closed conditions, since it answers with a promise; only argument validation still throws. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…a literal control byte The composite key joining requester and tsR was authored with a real NUL byte in the source, which makes git classify the file as binary — it disappears from diffs and from every text tool that scans the tree. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
- The commit-time lease fence threw straight out of commit(), skipping the cleanup every other terminal exit performs. A transaction holding a short lease and a long one would lose the 409, and the second key stayed locked with no owner until its own timer fired, with the staged blobs uncollected and the context still pinning a CLOSED transaction. It now runs the same abort() path before throwing. - Concurrent lock() calls on one key stopped coalescing once a transport is registered. What a follower waits on ended at the native acquire, so the leader's await of the cluster round left a window where the key is held but no handle is registered; the follower found nothing, retried, and parked on the leader's own lock for its whole timeout inside a transaction that could not finish until it gave up. The pending promise now spans the cluster round and registration. Both carry fails-without tests: reverting them turns 14 green into 12 green plus 2 red. Also: a round whose wait deadline fires before its own request write settled now defers the wire withdraw until it does, so a late request cannot install a peer round no withdraw matches; the id-to-node-name inversion is cached rather than re-read and unpacked per arriving entry; and control entries dropped on a non-owner worker are counted and re-warned rather than latching to one line. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
… last round All three were regressions introduced by the previous round's fixes, and each now has a test that fails without it. - isLeaseExpired() returned false for a handle released after its deadline, so a holder whose lease lapsed while the event loop was pegged could unlock and still commit a staged write, past every peer's bound. The deadline is now evaluated in one place that both predicates share, and a deliberate release does not make a lapsed lease valid again. - The id-to-node-name cache was one process-global map while the mapping it inverts is per audit store: short ids are minted per database in first-seen order, so id 1 names a different node in each one. An entry arriving on the second database was attributed to the first database's node — either dropped as an identity mismatch, or credited as a grant from a node that never sent one, which is the forgery the header binding exists to prevent. Keyed on the audit store now. - A granted lease was re-armed from `tsR + leaseMs - Date.now()`. tsR comes from a never-decreasing source, so a backward NTP step made the holder's deadline later than what peers bounded from their own observation. The round now carries the monotonic instant tsR was minted at and measures from that. Also: the structure-minting guard clears the dictionary before throwing, so one bad encode cannot make every later grant write fail for the life of the worker. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…e too upgradeToHold refuses to extend a granted cluster round past its deadline, but computed that clamp from the wall-clock expiresAt. A backward step would have turned the upgrade into exactly the extension the clamp exists to refuse, which is the same defect just fixed in joinClusterRound and #complete. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
- isLeaseExpired's contract said a deliberate unlock leaves a staged write valid; the code evaluates the deadline regardless, which is the behavior the round-3 fix deliberately introduced. Corrected both comments to match. - A miss in the node-name cache fell through to a store read, msgpack unpack and full rebuild, so an unmapped id — a relayed or malformed entry — paid exactly the cost the cache was added to remove. A populated cache is trusted now, misses included. - The structure-minting guard replaces the packer instead of truncating its array: msgpackr keeps its own shape-to-id state, so a reused id with an empty dictionary would ship a payload no receiver can resolve. - Participant membership for arriving entries is re-read at most once per tick window instead of once per entry. The grant set an acquire depends on is still taken fresh, since that one is the safety decision. - joinClusterRound derives its deadline from the mint origin rather than a second clock read, so the bound the design says has no margin has none. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
A reviewer read DESIGN's "no remote timestamp is ever compared against a local one" as absolute and filed the stale-replay filter against it. That filter does compare across clocks and has to: a remote ts_R has no local equivalent. Say which comparisons the claim covers, and why the one that crosses is safe. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…esolve Round 4 asked the id-to-node-name cache to trust misses, on the premise that a translated local id always has a name. That premise holds per worker, not across them: ids are minted by whichever worker first talks to a peer, and the invalidation only reaches that worker's own copy. Every other worker then held a map predating the new node and, trusting the miss, discarded its control entries permanently as "origin node could not be resolved". A hit is still trusted; a miss re-reads at most once a second, which keeps the per-entry store read and unpack off the apply thread without stranding a peer. Also adds a test that the coordinator resolves through databases[db][table], the path the replication sink and the transport resolver both use. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
… lost Phase 0 moved under this branch and replaced the flat 409 with lockNotHeldError(handle), which tells a lapsed lease apart from a handle that was handed back. The commit-time fence this branch adds was still minting the flat message, and ClientError is no longer imported there. It now composes with Phase 0: isLeaseExpired() decides whether to fence, lockNotHeldError says why. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
- #complete armed the hold deadline from a second monotonic read, so a pause between the two pushed it out by its own duration, past what peers bounded. It comes from the mint origin now, like joinClusterRound already did. - The payload decoder accepted any non-undefined key, so one whose key decoded to a plain object threw out of applyEntry through keyIdOf, and the replicated apply loop dropped the whole enclosing transaction. Keys are validated for a shape ordered-binary can encode, and applyEntry — the only boundary peer input crosses — contains anything else. - An explicit cluster request on a key the transaction already held node-scoped returned the weaker handle instead of failing closed, because the re-entrant path ran before the check. The check moved ahead of it. - The node-name miss-refresh window drops from 1s to 50ms, so a joining node loses at most a stray entry while a burst of unmapped ids still cannot drive the store. Tests: the receive path (decode, route, no record written), malformed-key containment, and the re-entrant fail-closed case. The release-entry assertion now waits on the condition, since unlock() is synchronous by contract and its durable release lands after — that was a real flake, 1 run in 5. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…, and test the real sink The key validation added last commit was wrong in both directions: it rejected bigint and binary record ids, which ordered-binary encodes fine, and accepted Date, which it does not. Too strict drops a legitimate lock; too loose throws into the replicated apply loop, which is what the check exists to prevent. A test now asserts the predicate and toBufferKey agree on every shape. The receive-path test also went through deliverLockControlEntry directly, which skipped the part with no other coverage. It now attaches a source to a table, pushes an event, and asserts the grant this node owes appears — so the sink's decode, author resolution and routing are all exercised. Removing the one line that intercepts control entries in writeUpdate turns it red. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…s, and a failing coordinator drops one entry - A released peer round lingers only so a replayed request cannot resurrect it, but it was counted against the per-key contention cap. A hot key turns over more than 64 rounds inside one bound, so this node would stop granting to live requesters and starve them. Released rounds are evicted first when the cap is reached; re-admitting a replayed one costs at most a spurious grant to a requester that is gone. - The lockCoordinator getter fails closed on an unusable node identity, which is right for an acquire and wrong on the receive path. lock() answers with a promise, so it now rejects rather than throwing synchronously past the caller's catch; the replication sink drops the one entry with a warning instead of rejecting and stalling the apply loop for everything after it. The starvation case carries a fails-without test: 90 acquire/release rounds on one key, all granted, red the moment released rounds count toward the cap. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…wide cap too The last commit stopped released rounds from consuming the per-key contention budget but left the same defect at the table-wide key cap: a key whose only remaining state is released rounds is not idle, so it holds its slot until those rounds age out, and broad hot-key traffic exhausts the cap and drops fresh keys. Both caps now evict released-only state first — the same rule, applied to both places it belongs. Carries a fails-without test: after filling the table with released-only keys, a fresh key is still granted; red the moment the eviction is skipped. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
The reclamation added last commit runs from the overflow path, so with the table genuinely full of live keys it scanned every key and every peer per arriving request and reclaimed nothing — turning a cap meant to bound work into a source of it, on the replication apply thread. It now runs at most once per tick interval; tick() reclaims these keys anyway once their rounds age out, and this only pulls that forward. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
… rationing the scan The throttle added last commit fixed the CPU amplification by skipping work, which also skipped reclamation that had just become possible — a one-shot peer request arriving inside the window was dropped with room waiting for it. The scan was expensive because its predicate was: it walked every peer of every key to decide whether anything live remained. KeyState now maintains that count as rounds arrive and are released, so the sweep is a flat scan with an O(1) test per key and needs no throttle at all. The regression test fills the table, drops one request, releases a single key and asserts the very next request uses the freed slot. Reintroducing the throttle turns it red. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
… something changes The overflow path still ran a full scan of every key state per arriving request once the table was saturated with live keys — the scan found nothing each time, so a request flood turned the cap into apply-thread work. Every transition that can make a key state released-only now bumps one counter, and a scan that reclaimed nothing records it. A saturated table under a flood scans once rather than once per request, because a dropped request changes nothing, while a release bumps the counter and the very next request rescans. That keeps the property the clock-based throttle got wrong: reclamation is never deferred past the moment it becomes possible. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…eclamation scan The mutation counter was bumped on every state transition, arrivals included, so alternating a request on an existing key with a fresh-key overflow re-armed the scan each time and brought back the amplification the counter exists to prevent. A state can only become released-only when liveness is removed: a round released, a live round dropped, an own round cleared, a deferred queue shrinking. Those four are what bump it now. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
…from the grant set The review found that the approved design's DOWN-exclusion rule breaks the exclusion argument it is part of. Its claim that "asymmetric partitions get the same rule from each side" is the bug, not the argument: with the A-C link down while both stay up and reachable from B, A drops C and C drops A, each asks only B, and B — holding no round of its own — grants both. One key, two holders, and a read-modify-write on each silently loses one under LWW. Exclusion now also requires the transport to mark the peer agreedDown, meaning the DOWN verdict is cluster-agreed and the excluded node is known to have stopped acquiring — which is what the membership epoch already listed as a transport precondition is for. Nothing in core sets it, so core excludes nobody and a requester blocks on an unreachable peer until its own timeout. That trades availability for the invariant this PR exists to hold, which is the same trade every other fail-closed rule here makes. Also: no control entry pins its transaction clock to ts_R any more. A timestamp minted before the write can land the entry behind a peer's replication cursor if anything else on the table commits in between; that was already the reason grants and releases took a fresh commit time, and it is reachable for the request too. Every consumer reads ts_R from the payload, so the log key never had to equal it. DESIGN.md records both, plus the aggregate-memory limit the caps do not bound. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PwYZ6EA8cn1hCuPHEubuko
c4db265 to
f1dd961
Compare
…tized ownership Ricart-Agrawala waits for a grant from every participant, so any single unreachable peer blocks every cluster lock, and each acquisition costs P+1 durable commits and up to P^2-1 frame deliveries with no amortization for a node that locks the same record repeatedly. docs/record-lock-ownership.md specifies the replacement: durable membership epochs agreed by a majority, rendezvous-hashed home nodes derived from them, and volatile per-record delegations that let a delegate serve lock()/unlock() from the Phase 0 key lock with no cluster message until recalled. DESIGN.md's Phase 1 section now opens by saying its arbitration rule is superseded and pointing at the note, so the repo's own design doc does not read as settled. Three planning rounds. Round 2 returned better-alternative-exists against a stateless epoch foundation and is adopted: a forgotten acceptance forks the configuration, so promises and accepted values are persisted before they are acknowledged. Round 3 cleared the framing. Its surviving protocol findings are folded in - retirement anchored at promise receipt, reservations reconstructed after a crash, recall revoking live handles rather than only closing admission, an inherited dependency set instead of a version scalar, and the fact that failing closed does not by itself supply successor freshness. The conflict-resolution and settlement guarantee is left open: it is an API-level choice that sizes the work. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t inherits The cross-model review of this branch found seven majors. Four are in the Ricart-Agrawala state machine the note deletes and are not worth fixing on code that is going away. Three are not: transport re-registration installs a fresh coordinator without fencing the handles the old one issued, so a successor can grant a key with no lease time elapsed; the direct receive callback lets a LockUnavailableError from coordinator construction escape, which the subscription sink already contains; and the commit-time lease fence scans every write on every commit, including in core-only deployments that never register a transport. All three live in code section 11 keeps, so they are obligations on the replacement rather than findings against a rule being removed. Recorded there with the section of this note that already governs each. Refs #483 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design note left one decision open for the human: what lock() promises for conflict ordering and for freshness after a holder crash. It is now exclusion-only, the smallest of the three arms. §10 carries the contract; DESIGN.md points at it as normative rather than keeping a second copy of an API guarantee. Writing that contract down honestly took ten pre-push rounds, and the first nine each found it promising something the code does not: - Not crash-only, and not unreachable on a clean handoff. A caller-supplied future context.timestamp — deliberate Phase 0 behavior, DESIGN.md's "Acquisition timestamp and mixed transactions" — survives a completely clean handoff: the successor is admitted with correct freshness, reads the current value, writes at wall-clock time, and its write is the older one. Limitation 2 is likewise reachable after a clean release, whenever the key's home has lost the handoff's dependency set to a restart, an epoch change or cap eviction. - The contract no longer restates engine conflict resolution. Three separate attempts to state the losing write's disposition were each wrong in a new way — the drop branch splits on the newer stored write's shape, a losing patch on disjoint fields survives intact, CRDT ops fold rather than lose. What is true is simpler: lock() changes nothing about how two conflicting writes resolve, so whatever the pair would do without a lock is what they do with one, silently. - There is no caller-side mitigation for the freshness limitation. X-Replicate-To/confirm= is super-user-gated and is a residency directive first, so recommending it would have narrowed the record's residency and made the limitation MORE reachable. Even with cluster-wide residency it closes none of the three routes: the late-settling commit had not settled when the barrier ran, so it needs fencing, not confirmation. §2 as written needs both deferred arms, not just the quorum-confirmed one. §2's exclusion invariant covers admit *or* commit; what ships excludes admission, and the commit half holds only up to the pre-submission expiry fence. Both limitations ship silent — a 200, no log, no counter — which §10 now records as a deliberate choice rather than an oversight, with the cheap lock-path-only detection routed to #2541. The fenced and quorum-confirmed arms are deferred to harper#2540 rather than dropped, so §7.3 keeps the analysis that produced them. The choice also removes an exception §8 was carrying: with no fencing generation, ordinary writes keep their ungated path with nothing added, which is the property the fenced arm would have given up. §14's first blocker is cleared; the measurement gate is not, and it is the only piece of this work unblocked today. Refs #483, #2540, #2541, #2542, #2546, #2547, HarperFast/harper-pro#824, HarperFast/harper-pro#825 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M6aY2ERiYM8294P2f3aoUq
Phase 0 (#2462) made the rocksdb-js in-memory key lock the sole authority for
table.lock(id),exclusive across one node's worker threads. This branch carries that verb across every node that
replicates the database, over three new transaction-log control entries.
The arbitration rule on this branch is superseded and will not ship as written. Ricart–Agrawala
waits for a grant from every participant, so any single unreachable peer blocks every cluster lock
— harper-pro#822 already records that in its own limitations list — and an acquisition costs
P+1durable commits and up to
P²−1frame deliveries with no amortization for a node that locks the samerecord repeatedly. At
P=12, a cluster size Harper runs today, that is 13 durable commits and 143frame deliveries per lock. A lock any one unreachable peer disables is not usable for the
correctness-critical work
lock()exists for, so the availability limit is the feature's value ratherthan a rollout caveat.
docs/record-lock-ownership.mdspecifies the replacement, and is the substance of this push. Three levels at three very different
rates:
(number, members[], ringVersion)under single-decree agreement over a majority, with acceptor promises and accepted values persisted
before they are acknowledged. Consensus appears here and nowhere else: once per membership change
plus one renewal per node per lease period, never per key and never per lock.
members[]. One arbiter per key is trivially exclusive, which deletes the entire grant statemachine: no deferral queues, no
(tsR, nodeName)tiebreak, no synthesized grants, no split votes,no revocation protocol.
admit critical sections on that key for a bounded time. With a live delegation
lock()/unlock()are pure Phase 0: the local key lock, zero cluster messages. Releasing the application lock does
not release the delegation, so a node writing the same record repeatedly pays one round and then
nothing, and the delegate is in practice the last writer.
Steady-state cost goes from
P+1durable commits andP²−1deliveries per lock to zero of each; afirst lock on a cold key is two unicast messages and one RTT to the home; a handoff is one durable
release plus
P−1deliveries. A node that is down blocks only its own share of the ring, until theepoch advances.
This is close to what harper-pro#438 filed as its own "Phase 1 — single-owner delegation", and to
what #483 describes in prose ("if an exclusive lock is held by one other node, the lock can be
requested from that node"). It differs in deriving the home from replication-group membership rather
than from residency or
server.shards, so it needs no operator sharding configuration — only onecustomer uses sharding today.
What this push contains
docs/record-lock-ownership.md— the design note, through three planning rounds. Round 2 returned
better-alternative-existsagainst a stateless epoch foundation and is adopted: a restarted acceptor that forgets its
acceptance forks the configuration, and no timing argument substitutes for durable state
(§4.0 carries the counterexample so it is not reinvented). Round 3 returned
Framing-Verdict: chosen-approach-sound; its surviving protocol findings are folded into §§4.2,4.4, 6, 7.2, 7.3, 8.
DESIGN.md— the Phase 1 section now opens by saying its arbitration rule is superseded and pointing at the
note, so the repo's own design doc does not read as a settled decision.
No code changed in this push. The Ricart–Agrawala state machine is still here, still gated off, and
is removed when the replacement lands — the note's §11 is the line-by-line split.
What survives the arbitration change, and what does not
Reused unchanged (roughly 60% of the diff, and already through fifteen review rounds):
Packr,recordId: nullwith thekey in the payload, receive routing off the record path, and the filters on every surface that
reports audit entries as record activity.
Table.ts/DatabaseTransaction.ts. It moves from a per-roundlease to a delegation lease with no change to the mechanism.
recordLock.ts's Phase 0 primitives, thescopeoption, the monotonic lease deadline, thecontained release hook,
lock()coalescing, and theLockUnavailableError/ 423 / 409 contract.ClusterLockTransportas the boundary, the per-database registry, fail-closed membership validation, and
ownsCoordination()thread ownership.Removed when the replacement lands:
LOCK_REQUEST/LOCK_GRANT, per-peer round tracking, deferralqueues,
(tsR, nodeName)ordering, synthesized grants, withdraw-on-timeout, and theagreedDownDOWN-exclusion rule this branch's last commit added.
For the human reviewer
The guarantee decision is made (2026-09-09): exclusion-only.
lock()guarantees that at mostone node admits a critical section for a key at a time, and that a node admitted after another
node released cleanly has already applied that node's committed writes — but only while the key's
home still holds that handoff's dependency set. It adds no fencing generation to conflict
resolution and does not confirm locked writes to a quorum, so two things are part of the documented
contract. Neither is crash-only, and both are reachable on a completely clean handoff:
assigned when the write was staged and lease expiry orders admissions, not timestamps.
lock()changes nothing about conflict resolution — whatever two conflicting writes woulddo to the record without a lock is what they do with one, silently. (1a) the predecessor's
clock ran ahead and its write is still in flight; (1b) it stamped a future
context.timestamp— deliberate Phase 0 behavior — then committed, replicated, drained andreleased cleanly, and the successor's write is the one that loses. No crash, no skew, nothing
in flight;
barrier only drains streams from reachable members. Three routes: the predecessor crashed, it
is unreachable, or its native commit settled after the barrier was measured. Two damaged effects
on the one record — the predecessor's transaction not reflected, and the successor's write
computed from the stale value it read.
There is no caller-side mitigation for (2) at all.
X-Replicate-To/confirm=issuper-user-gated (
checkContextPermissions,resources/Table.ts:7258— a 403 for an app caller),and for a super-user it is a residency directive first: a numeric value truncates the record's
residency (
getResidency,:1574) and*falls back to the database's configuredreplication.replicateTo. Either way a successor's barrier can satisfy over members that never heldthe locked write. Even with cluster-wide residency, confirmation closes none of (2)'s three routes —
the late-settling commit needs fencing, so §2's invariant as written needs both deferred arms.
Both limitations also ship silent — a 200, no log line, no counter — which §10 records as a
deliberate choice with the cheap lock-path-only detection routed to Record locks Phase 1: rendezvous home ring, per-record delegations, and drain/recall #2541.
The normative text is §10 of the note, which
DESIGN.mdpoints at rather than restating.Ten pre-push rounds ran on this push, and the first nine each found the contract text promising
something the code does not. In order: crash-only; the late-settling commit filed under
the wrong limitation; clean-handoff safety; the LWW mechanism sentence stated backwards; the
drop-vs-fold rule splitting on the wrong operand; a whole-write loser guarantee false for a patch on
disjoint fields; a per-field one false for CRDT ops; a recommendation to use
X-Replicate-To: N;confirm=Mthat would have narrowed the record's residency and made thelimitation more reachable; that the same header is super-user-gated, so it was never a
caller-side mitigation in the first place; and — round 10, in code rather than in the note — that
the super-user gate itself is bypassable. It converged only when the contract stopped restating
engine conflict-resolution and said the thing that is actually true —
lock()does not change howconflicting writes resolve, and there is no lever a caller can pull. That history is the argument
for §12's schedules asserting the limitations as executable expectations rather than leaving them in
prose: a paragraph about a conflict-resolution edge does not stay true on its own, and this one is
going into user-facing API documentation.
The choice also removes an exception §8 was carrying: with no fencing generation, ordinary writes
keep their ungated path with nothing added, which is the property the fenced arm would have given
up.
One earlier claim in this PR's body is withdrawn. It said a crashed holder's in-flight write is
stamped older than any successor's and so resolves under LWW exactly as if the release had not
overtaken it. That holds for a clean release. It does not hold for a crash: if the crashed
holder's clock runs ahead, its delayed write can carry the greater timestamp and overwrite the
successor. Monotonic lease expiry orders admissions, not timestamps — §7.3 is that hole, and the
table above is the choice about it.
The freshness property this branch gets for free is the one the new transport must pay for.
A grant here rides the grantor's own replication stream behind the grantor's data writes, so
applying a grant implies having applied that grantor's earlier writes to the key. Unicast
delegation messages lose that, and a scalar record version does not restore it — core breaks
equal-
versionconflicts by node name, so a replica can hold a losing value at the sametimestamp and pass a
version ≥ Vtest. §7.1 replaces it with an inherited(origin → position)dependency set on the
LOCK_RELEASEentry.The work is decomposed and the remaining blocker is measurement. Record locks Phase 1: rendezvous home ring, per-record delegations, and drain/recall #2541 (home ring,
delegations, drain and caps, plus the three inherited substrate defects below), Record locks Phase 1: successor freshness — inherited dependency sets and the recovery barrier #2542 (successor
freshness), Record locks Phase 1: durable membership epoch protocol (single-decree agreement per database) harper-pro#825 (the membership epoch protocol), and
Record locks: measure the Phase 1 cost baseline before the protocol change harper-pro#824 (the measurement gate, the only piece unblocked today).
The measurement gate comes before the protocol change, not after it. No number in the note is a
benchmark; they are message counts. Acquisition latency on a real cluster, audit growth per lock,
and throughput with the feature disabled are the first deliverable, and they also set the baseline
the new design has to beat.
The pre-push review ran fifteen rounds on the code that is here. Roughly half the defects it
found were introduced by an earlier round's own fix, which is a property of this state machine
rather than of the review — and it is the strongest single argument for a design whose arbiter is
one node instead of a quorum. That history is preserved in the commit log; the note's §9 records
why quorum voting, a replicated Raft coordinator, and a
server.shards-based owner map were eachconsidered and not chosen.
Three defects the cross-model review found in the substrate, not in the rule being deleted.
They are recorded in §11 of the note as obligations on the replacement, and are not fixed in this
push — the branch does not merge with
mainyet and two of the three are touched by thereplacement anyway. Named so they are not lost: re-registering a transport closes the current
coordinator and installs an empty one without invalidating the handles the old one issued, so a
successor can grant the same key with no lease time elapsed (
resources/Table.ts:5474); aLockUnavailableErrorfrom coordinator construction escapes the direct receive callback, which thesubscription sink already contains (
resources/recordLockCoordinator.ts:908); and the commit-timelease fence scans the whole write set on every commit, including in core-only deployments that
never register a transport, which contradicts the ungated-path goal (
resources/DatabaseTransaction.ts:1213).The review's other four majors are inside the Ricart–Agrawala state machine and are deliberately
left alone.
Two substrate decisions from the original design still stand and are still worth a check.
Control entries carry
recordId: nulland the locked key in the payload, because a control entrycarrying the key answers
_writeUpdate's keyed dedup lookup at exactlyts_Rand silently dropsthe holder's own first write. And node identity is the node name, never the audit
nodeId:short ids are per-node, so a
(ts, nodeId)order would order the same pair differently on twonodes and both would grant.
One bug this push found in
main, filed rather than fixed hereTracing
X-Replicate-Toto decide whether it was a usable mitigation turned up#2546 (P1):
checkContextPermissions(
resources/Table.ts:7258) gates on truthiness, soX-Replicate-To: 0skips the super-user 403and reaches
getResidencywithcount = 0, pinning the record to the receiving node. Anyauthenticated user with write permission can take a record out of replication with one header — 200
response, nothing logged, and the residency persists across updates. Pre-existing on
main, unrelatedto this branch, and out of scope here; the fix is
!= undefinedon both guards.Verification
Docs-only push, so the code gates are the ones already recorded on this branch's head
(
f1dd96180), unchanged here:test:unit:resources2109 passing / 29 pending / 0 failing;test:unit:main5289 passing with twoenvironmental failures that pass when re-run alone;
record-lock-concurrencyintegration 3/3.unitTests/resources/recordLockCoordinator.test.js(21 tests) andunitTests/resources/recordLockCluster.test.jsare unchanged and still describe the arbitrationrule that is being replaced. The note's §12 lists what the replacement's suite must add — most
importantly independent per-node clocks, which the current shared fake clock cannot express.
prettier --checkon both changed files.Refs #483
Complexity: complicated
Review-Coverage: authored=unknown; ran=none; rounds=1 @ cd3fea6
Human-Review-Need: 4 @ cd3fea6