Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Concurrency and Consistency' documentation page detailing Harper's concurrency primitives, native record locks, optimistic concurrency, and consistency limits. It also updates several existing documentation files to reference this new guide. The review feedback focuses on maintaining style consistency in lists within options.md and overview.md by using em dashes instead of hyphens and bolding link text to match surrounding items.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-749 This preview will update automatically when you push new commits. |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-749 This preview will update automatically when you push new commits. |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
This is great. I will note that we could have used record.quantity-- instead of subtractFrom in reserveOne since we already hold the exact quantity, and the cleaner usage might be a good demonstration of the invariants that lock facilitates.
🤖 Reviewed with Cursor-grok
|
|
||
| Locks leave normal conflict resolution unchanged in both node and cluster scope. Earlier writes or another lock in the same transaction can fix its timestamp before a later locked update. That update can lose to an intervening commit even after reading its value, with all writers participating and no clock skew or crash. Acquiring several locks in one request therefore does not establish serializable multi-record read/check/write. Atomic deltas help only where the invariant permits them. Clock skew or a deliberately future transaction timestamp can also let a predecessor's write outrank its successor's under last-write-wins. | ||
|
|
||
| Cluster locks provide exclusion, not global serializable transactions or quorum durability. On a clean handoff, the home retains **release dependencies**, the replication positions the next holder must receive before using the record. Home restart, generation change, or eviction from the home's bounded dependency cache can lose those dependencies even after a clean release. Recovery then uses a **barrier**, a check that earlier writes have settled and replicated. An unreachable predecessor can block acquisition; a native commit that settles after that barrier can leave a successor reading stale data. Client retries, replication headers, and origin pinning do not establish a stronger guarantee. Define and verify any stronger cross-record or cross-node requirement before designing additional coordination. |
There was a problem hiding this comment.
[major] The page says "an unreachable predecessor can block acquisition". The lock contract (harper/resources/record-locks.md:929-937) describes the opposite risk. If predecessor A commits and becomes unreachable before replicating, the recovery barrier still succeeds, and successor B is admitted and reads the stale value. A reader who designs around "blocking" will miss a lost-update path in exactly the case this sentence covers.
|
|
||
| On a table, acquires a native record lock and returns a mutable Resource instance in the supplied or ambient transaction. Lock before other writes and before reading the condition, then update the returned record and `await record.save()`. The default transaction-scoped lock releases on commit or abort. | ||
|
|
||
| Options are `scope` (`'node'` or `'cluster'`), `lease` and `timeout` (milliseconds, each defaulting to `30000`), and `hold` (default `false`). Cluster coordination requires Harper Pro; explicit `scope: 'cluster'` fails closed when unavailable. Locks are cooperative: ordinary unlocked writes still proceed. This server API does not perform authorization or expose a REST method. |
There was a problem hiding this comment.
[major] On Harper Pro with replication.recordLocks unset, an implicit-scope lock() on a replicated table does not fall back to node scope. Pro registers a fail-closed transport (harper-pro/replication/recordLockTransport.ts:351-359, where ownsCoordination returns true), so the call rejects with 503. The page says only an explicit scope: 'cluster' fails closed, and reference/database/concurrency.md:50 describes only the core-only fallback. Readers on Pro will see 503s the page does not predict.
| return transaction(async () => { | ||
| const record = await tables.Inventory.lock(id, { scope: 'node', timeout: 1000 }); | ||
| if (!(record.quantity >= 1)) { | ||
| throw Object.assign(new Error('Insufficient stock'), { statusCode: 409 }); |
There was a problem hiding this comment.
[minor] The worked example throws 409 for insufficient stock. The page's own Errors table (reference/database/concurrency.md:94) uses 409 for lease expiry, and the stale-edit section says application errors must be distinguishable from a bare 409. A request held past the 30000ms lease therefore rolls back with a 409 that the client reads as "out of stock". The "one reservation, one error" outcome at reference/database/concurrency.md:85 also ignores the 423 a second caller gets when the first request transaction outlives the 1000ms timeout.
| | `cluster_set_routes` | Adds routes to the replication routes config (PATCH/upsert) | super_user | | ||
| | `cluster_get_routes` | Returns the current replication routes config | super_user | | ||
| | `cluster_delete_routes` | Removes routes from the replication routes config | super_user | | ||
| | `record_lock_apply_homes` | Applies an agreed record-lock home map in Harper Pro on RocksDB; see [native record locks](../database/concurrency.md#native-record-locks) | super_user | |
There was a problem hiding this comment.
[minor] The row says the operation "applies" a home map. A first call can return success with outcome: "staged" and stay inactive until a follow-up call with drained: true. Cluster locks return 503 until then. reference/database/concurrency.md:52 names the stage/drain/activate steps but not the staged response, so an operator can take the first success as activation.
|
|
||
| When correctness requires cross-node coordination, set `scope: 'cluster'` explicitly. This fails closed if the Harper Pro cluster-lock transport and home-map configuration are unavailable. With an implicit default, a core-only deployment can fall back to node scope; the existence of `lock()` alone does not establish cluster protection. Revisit an explicit `scope: 'node'` before adding nodes. | ||
|
|
||
| Cluster locks require RocksDB, Harper Pro with [`replication.recordLocks: true`](../configuration/options.md#replication), and an agreed home map for the database. The map names every participating node; a record's **home** is the node that arbitrates its locks. A **generation** identifies one agreed version of that map. Enabling the configuration flag alone does not create a map. Pro's `record_lock_apply_homes` operation coordinates the transition for an operator-supplied participant list. Follow its versioned [home-map setup and transition procedure](https://github.com/HarperFast/harper-pro/blob/v5.3.2/replication/RECORD_LOCK_HOMES_DESIGN.md#the-operation): agree the complete participant list, stage and stop the old generation, drain outstanding locks, then activate the agreed generation. Do not derive separate maps from each node's current view of reachable peers. |
There was a problem hiding this comment.
[minor] The docs present record_lock_apply_homes as super_user-gated. They omit the accepted gap that Pro itself warns about at startup (harper-pro/replication/recordLockTransport.ts:1481, tracked as harper-pro#869): any principal that a peer resolves as a cluster node can apply a home-map transition, without operator proof. Pro tells operators to treat this as a prerequisite before relying on cluster locks in production, so the docs should say so too.
|
Quick ping @hdbjeff still waiting on you to reply to Kris' review. Thanks! |
|
Heads-up: the v5 reference and the 5.4 release notes now live in the code repos (documentation#788, harper#3944, harper-pro#1237).
— Claude Opus 5.5 |
Summary
Document Harper's native concurrency controls so readers can choose transactions, atomic deltas, and record locks before building application coordination. This supplies the public source Dawson requested in skills PR 127.
For the human reviewer
Native controls are the first recommendation, with participating-writer, timestamp, and recovery limits kept explicit. This replaces conflicting advice that prompted unnecessary application coordination.
Cluster setup links the versioned Pro procedure rather than duplicating its operator runbook. The guide supplies the prerequisites and API boundaries.
The post-review delta is one Operations API index entry plus formatter alignment in its table (21 changed lines). It was build- and format-checked, not sent through another model review; the linked operation and setup guidance were already reviewed.
Changes
Concurrency and Consistency maps requirements to native controls, documents lock options and cluster setup and participating writers, and gives a tested reservation example. It separates optimistic storage conflicts from stale edits and explains timestamp and recovery limits and browser-origin consistency.
Resource API adds
lock(), qualifies resolver snapshot consistency, and replaces external-coordinator-first advice with native controls and explicit precondition support.Database API qualifies snapshot consistency for lock acquisition.
Configuration options documents the Pro lock flag; the database overview and 5.3 release notes link the guide.
MCP configuration corrects quota serialization advice; Scheduler distinguishes duplicate scheduler delivery from record locking.
Reference sidebar adds the guide to Database navigation.
Operations API index links the Pro home-map operation to the setup guide.
Product and architecture tour
Choosing coordination
Verification baseline
Verification
1before the competing commit and2after acquisition. Both workers ran the example from background timers and committed correctly. Locking and saving a missing key created it; an unseeded reservation returned insufficient stock.After this canonical guide merges, the skills rule can switch to generated mode using
reference/v5/database/concurrency.mdfrom the docs build.Generated by OpenAI Codex; posted via @hdbjeff.
Related PRs: 2 independent
Complexity: complicated
Review-Coverage: authored=codex; ran=gemini,claude; adjudicated=domain; blocked=cursor-composer(not-installed),cursor-muse(not-installed); declined=cursor-grok,cursor-kimi,muse; rounds=3; full=2 @ ca6a100
Review-Attention: read ~3m (decisions: cluster-setup-reference, lock-first-guidance) @ ca6a100