HarperFast / HarperFast/harper-pro

Record locks: make cluster-scoped lock() work at threads.count > 1 (owner-worker relay, per-isolate row queue, every-thread registration)

Open
#852 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
3
Forks
0
Avg merge
1d 21h
Merged PRs (30d)
80

Description

## Summary

Cluster record locks (harper-pro#438, PR #822) work correctly with one http worker and are **not usable at `threads.count > 1`**. Three separate items are behind that; the first is the one an operator hits immediately, the second is a correctness gap, the third is a known hole from core's design note §11.

The feature is default-off, so nothing shipped is affected today — but this is what stands between "opt-in, single-worker" and "usable at the default worker count", and it gates any later decision to enable record locks by default.

## 1. A local `lock()` is not relayed to the coordinating worker

Core fails closed off the owner thread — `Cluster record lock coordination is not owned by this worker thread; retry so the request reaches the coordinating thread` (`core/resources/recordLockCoordinator.ts`) — and harper-pro relays **inbound peer** operations to the owner (`replication/recordLockRpc.ts` routes `record_lock_delegate` / `record_lock_recall` / `record_lock_barrier` through the main thread) but does **not** relay an application's own `lock()`.

With `threads.count: 4`, roughly three quarters of HTTP connections land on a worker that answers 503, and a keep-alive client stays pinned to that worker across retries. `replication/recordLockTransport.ts` warns about exactly this at startup:

> `replication.recordLocks is enabled with N http worker threads: a cluster-scoped lock() succeeds only on the worker coordinating its database and answers 503 elsewhere (a keep-alive client must reconnect to retry); run one http worker (threads.count: 1) for uniform lock() service`

**Fix shape:** relay the local acquire (and the matching release) through the main thread to the owner worker, mirroring the inbound path that already exists in `recordLockRpc.ts`. The relay, its 5 s bound, its fail-closed timeout answer and its main-thread routing are all already written for the other direction.

**Design question the implementer must answer:** a lock handle is per-transaction state on the calling worker, while the delegation lives on the owner. Relaying the *acquire decision* is straightforward; deciding what crosses the thread boundary (a grant token the caller's coordinator installs locally, versus the whole handle) is the real design content and should be settled before code.

## 2. `rowQueues` serializes per worker isolate, so a `fenceExternal` can restore a retracted generation

`replication/recordLockHomes.ts`'s `rowQueues` is module-level, so it serializes same-database transitions **per worker isolate**, not per process. `readRow` also sits outside `writeRow`'s transaction while the write replaces the whole row. Two workers can both read `active: g1`; a `fenceExternal` (which writes `active: existing?.active`) landing after a `stage` then restores the `active` the stage retracted — while the operator holds a 200 for that stage and proceeds to activate after the drain. That is two live generations, the exact invariant the home-map design exists to enforce.

Not reached by any test: every fixture runs `threads.count: 1`, and more than one worker is only warned about, not refused.

**Fix shape:** a process-wide guard (cross-worker lock or a main-thread-owned serializer) plus a compare-and-set against the state the plan was decided on, rather than a whole-row replace.

## 3. Full every-serving-thread transport registration

Core's `docs/record-lock-ownership.md` §11 "still owed" list: the transport must be registered on **every** worker that can serve a `lock()`, including a dedicated application worker (harper#2524), and core cannot check that. A worker that never registers never latches `clusterRequiredDatabases`, so a default-scoped `lock()` there takes the Phase 0 node lock alone while a peer runs the cluster protocol — two nodes admitting one key.

## Acceptance

- A cluster-scoped `lock()` succeeds on every http worker, with a cluster integration test running `threads.count > 1` and asserting uniform service (today every record-lock fixture pins `threads.count: 1`).
- A `stage` racing a `fenceExternal` across two workers cannot produce two live generations; covered by a test that exercises both workers.
- The startup warning in `recordLockTransport.ts` is removed or narrowed to whatever genuinely remains.

## Not in scope

The `lockRelease` cross-thread relay gap is related but distinct and already tracked in the same §11 list; fold it in only if the relay from item 1 covers it.

Contributor guide

Open the contributing guide

Research direction

Start with core/resources/recordLockCoordinator.ts, replication/recordLockRpc.ts, replication/recordLockTransport.ts, and replication/recordLockHomes.ts, then read docs/record-lock-ownership.md §11. Run the existing record-lock fixtures and inspect their threads.count: 1 setup before adding multi-worker coverage. Done means uniform lock() service across workers, no conflicting generations during stage/fenceExternal races, and an accurate startup warning.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
backend, databases, distributed-systems, testing
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.