Clone sync verification now detects an in-progress base copy on RocksDB instead of marking the node Available mid-copy

[kriszyp]

Fixes #655. On RocksDB, clone_node's sync verification was vacuous: the describe operations report last_updated_record for no table (harper#2091 — RocksTransactionLogStore.getKeys() is a TODO stub), so every database's sync target was 0, checkSyncStatus skipped every database, and the first poll declared "All databases synchronized" — the clone marked itself Available and cloned seconds into a multi-GB base copy. This is what failed the Large-Data clone stress run 31006315068 (Clone row count 31393 != leader 104858); green runs passed only because the copy happened to outrun the test's polling grace window.

A database without a target is now verified instead of skipped: it requires a positive received-version watermark. The watermark is held at 0 for the whole bulk copy and only becomes positive via the final end_txn the sender always emits at copyStartTime — the copy's own completion signal — so this works against leaders whose describe can't report last_updated_record and for empty databases. A database with a target but no socket yet also holds completion, so a lone early socket (the system DB's small copy finishes in seconds) can't complete the check vacuously.

Follow-up commits refine the socket-presence rule:

• v4 leaders (first CI round caught this in cloneFromLegacy): a legacy leader never replicates the system database, so requiring every target's socket wedged the clone. checkSyncStatus now takes requiredSocketDatabases; a non-required database is still verified whenever its socket exists.
• Ratchet (from the follow-up cross-model round): once a target database's socket has been seen, it stays required — its later loss holds completion instead of completing around it.
• Leader version probe (from KrAIs' review thread): whether system's socket is required is a leader capability, not an ordering heuristic — registration_info (present on every leader version) decides it, so v5+ leaders require the system socket up front, closing the reverse race where a small user database completes before the system subscription registers. The probe fails closed: only a positively-read legacy version exempts system.
• Selective replication (from gemini's review thread): getLastUpdatedRecord now filters targets by replication.databases, so a database the clone doesn't subscribe to never becomes a sync target. Sharded entries evaluate the real same-shard predicate (shouldReplicateFromNode semantics — the leader's shard read via get_configuration vs replication.shard), failing closed to keep the target when the leader's configuration is unreadable. The predicate is pure and unit-tested (cloneNode/replicatedDatabases.ts).

Where to look

• The semantics change is targetTime = targetTimestamps[dbName] || 1 plus the missing-socket loop in checkSyncStatus (cloneNode/syncMonitor.ts). Targets and Clone sync monitoring times out only when replication stalls, so large clones no longer fail at a fixed 5-minute cap #649's stall detection are unchanged.
• The sender invariant the fallback leans on is explicit in replication/replicationConnection.ts (copy completion): the !currentTransaction.txnTime branch forces a txn for a zero-record copy so the final end_txn is always emitted at copyStartTime — the code calls it "the sole signal that the copy is synced". Per-record watermark advances are gated on !inCopyMode, mid-copy checkpoint flushes deliberately carry no sequence update, and nothing writes the 1 sentinel to RECEIVED_VERSION_POSITION (only SENDING_TIME_POSITION uses it). Verified live with instrumentation: the data DB's watermark stayed 0 through an entire 1 GB copy and jumped exactly at the final end_txn.

Cross-model review — unresolved concerns (deliberate trade-offs)

The pre-push review (codex + gemini + domain adjudication) verdict was CHANGES on one merged major: completion keys on a sender invariant this diff doesn't control. Responses, left open for human judgment:

• Old leaders (pre-Replicated hdb_analytics floods system transaction logs and spins a worker in native code; system base-copy wedges until worker recycle #480 send discipline) could emit mid-copy sequence updates, turning the watermark positive early. True — but the same updates advance the watermark to copyStartTime, which also satisfies the old target-based check (targets < copyStartTime always), so old-leader clones are no worse than before this PR; new-leader clones become correct.
• Empty database against a hypothetical leader that skips the final end_txn: the clone would stall (visible failure naming the database, node stays Unavailable/not-cloned) instead of silently passing unverified. Current senders provably always emit (branch cited above); for exotic cases --skip-sync-monitor remains the operator escape hatch. Fail-safe direction chosen deliberately.
• Watermark advances at end_txn decode, slightly before the last copy batches commit. Pre-existing semantics for targeted databases, unchanged here; the airtight signal would be exposing copy-mode exit through cluster_status — possible follow-up.
• A replication socket for a database absent from the targets map now demands verification instead of being ignored (adjudicated minor): with targets filtered to the replicated set, a socket exists only for databases being cloned, so a mismatch is anomalous; the strict direction fails safe (stall) rather than passing unverified.

Verification

• Unit: unitTests/cloneNode/syncMonitor.test.mjs — 9 new/updated cases (vacuous-pass regression, watermark completion, missing-socket hold, no-target pending slides the stall deadline, v4-leader socketless system, non-required-still-verified, ratchet-holds-on-drop, data-completes-before-system-socket ordering); the originals fail against the pre-fix build (fails-on-base) and 23/23 pass with the fix. Full harper-pro unit suite: 631 passing; the sole failure (injectedKeyCustody) reads /proc and is Linux-only — pre-existing on macOS, unrelated.
• Live e2e (executed): 1 GB largeClone.test.mjs on macOS. Before the fix: clone flipped Available at 9.1s mid-copy, failed with 3999/10486 rows (same signature as CI). With the fix: clone stayed Unavailable through the whole copy (including wedge/reconnect cycles from the unrelated Bulk-copy send loop wedges with event-loop starvation on macOS; progresses only via watchdog reconnect cycles #656) and the test passed with exact row-count match at 326.5s.
• CI Large-Data stress on this branch (run 31016142074): large-clone passed at 10 GB on a slow runner (24 min wall) — the exact profile that failed on main, where instant-Available raced the copy. Available now means the copy is done.
• cloneFromLegacy (v4 leader) in Cluster 1/6 is the gate for the follow-up commits — validated by the PR CI re-run.

Generated by Claude (Opus 5). Root-cause analysis in #655; the macOS copy wedge observed during repro is #656.

_{Human-Review-Need: 4 @ 65ef227}

[gemini-code-assist]

Code Review

This pull request updates the clone sync monitor to prevent premature sync completion when target timestamps are missing (such as with RocksDB leaders) or when replication sockets have not yet registered. It defaults missing target timestamps to 1 and ensures all target databases have active sockets. Unit tests are added to verify these behaviors. The reviewer identified a critical bug where selective replication of a subset of databases will cause a permanent stall because non-replicated databases in targetTimestamps will never have active sockets, and suggested filtering the targets as well as avoiding for...in loops.

[claude]

Reviewed; no blockers found.

[kriszyp]

Two follow-up commits pushed (plus a merge of latest main):

1. cloneFromLegacy CI failure root-caused and fixed — the first round's missing-socket hold required every target database's socket, but a v4 leader never replicates the system database, so the clone wedged forever against a v4 leader (data: Synchronized + system: no replication socket until the Available timeout). checkSyncStatus now takes requiredSocketDatabases — cloneNode passes the user databases — and a non-required database is still verified whenever its socket exists.

2. Ratchet, from the follow-up cross-model round: once a target database's socket has been seen it stays required, closing the v5 edge where a small user database's copy could complete around a system socket that dropped mid-clone. A never-appearing socket (v4) still can't wedge.

Also: the 10 GB Large-Data stress run on this branch passed — large-clone green in 24 min on a slow runner, the exact profile that failed on main.

— Claude (Opus 5)

[kriszyp]

Integration Tests 3/3 failure analyzed — cloneResume.test.mjs ("resumes the bulk copy after a mid-copy kill"). This is a pre-existing, rare resume defect this PR surfaces, not one it introduces: the killed clone reconnected via the direct seq-cursor path (armed leading-duplicate fast-skip … direct resume cursor) instead of re-requesting the copy, so the remaining rows can never arrive. On main the same scenario fails the test's row-count assert (or in production silently serves a clone missing data while Available); with this PR the monitor correctly holds the node Unavailable instead. Full forensics + the invariant to enforce: #658.

The test passes locally against this branch in 10s (healthy resumes log Requesting full copy of database data on restart, which the failing CI run never did). Re-running the job.

— Claude (Opus 5)

[kriszyp]

Review feedback addressed (commits 4e380b37…752ebc1e). Inline replies are blocked while a pending review exists on this account, so responding here:

@gemini-code-assist's selective-replication thread (resolved):
Valid catch — with replication.databases configured, non-replicated leader databases became sync targets whose sockets never exist, and the missing-socket hold would wedge the clone. Fixed as you suggested: getLastUpdatedRecord now applies isReplicatedDatabase gating (shared with cloneSchemas), so a database the clone doesn't subscribe to never becomes a sync target. Sharded entries additionally evaluate the real same-shard predicate (shouldReplicateFromNode semantics, leader shard via get_configuration), failing closed when the leader's configuration is unreadable; the predicate is pure and unit-tested in cloneNode/replicatedDatabases.ts.

On the for...in point: that loop was already replaced in an earlier commit — the hold now iterates the requiredSocketDatabases array, so there's no prototype-chain exposure.

KrAIs' system-socket ordering thread:
Agreed — the ratchet alone couldn't distinguish a v4 leader from a delayed v5 socket, and the reverse ordering (data fully synced on the first poll, system subscription not yet registered) would return early. Implemented the capability signal you suggested: monitorSync now probes the leader with registration_info — the lightweight version operation present on every leader version (same precedent as targetSupportsStreamingDeploy in core's cliOperations.ts) — and requires the system socket up front for v5+ leaders. The probe fails closed: only a positively-read legacy major version exempts system; a missing/unparseable version or a persistently failing probe (3 retries) requires it, so a transient probe error against a v5 leader can't reopen the race. The seen-socket ratchet remains as an additional layer.

Also added the regression test you asked for: data socket fully synchronized from poll one, system socket absent, then appearing pending on a later poll, then verifying — completion must wait through the whole sequence (holds a v5 clone whose data DB completes before the system socket ever registers).

— Claude (Opus 5)

Also from the follow-up cross-model rounds: the version probe tolerates prefixed version strings, and the sharded-entry target classification evaluates the real same-shard predicate instead of excluding sharded entries (a same-shard database stays a required, verified target).

[gemini-code-assist]

Thank you for the detailed breakdown and for addressing the edge cases regarding selective replication and the system socket ordering. The implementation of the version probe and the seen-socket ratchet effectively closes the identified races. The regression tests added to unitTests/cloneNode/syncMonitor.test.mjs and unitTests/cloneNode/replicatedDatabases.test.mjs provide excellent coverage for these scenarios. This approach is robust and aligns with the fail-closed requirement for sync monitoring.

[DavidCockerill]

The detection design is sound and I verified the protocol invariants it rests on rather than trusting the body: the sender always emits the final end_txn at copyStartTime (replicationConnection.ts:3843-3852) and the receive side suppresses per-record advances while inCopyMode (:4077), so "watermark positive" really does mean "the copy emitted its completion frame". Every can't-tell branch fails closed, including the registration_info version probe, and #649's stall loop is preserved byte-for-byte. The 1 GB live and 10 GB CI evidence is the right kind of evidence and I'm not disputing it.

The blocker is that a resumed clone can never publish Available — detail and the log trace in the thread. It's in this PR's own CI: Integration Tests 3/3 failed 2 of 3 attempts at 3cf4cf14, with the same error, line and file as the blocker currently open on #650. Structural rather than flaky: on a resume the hdb_nodes leader row already exists, so restartWorkers() alone starts and finishes the copy before setNode, and the monitor then polls a watermark of 0 until the stall deadline. A fresh clone can't hit it because no row exists yet — which is exactly why the large-copy runs are green.

One correction worth making explicitly: the #658 attribution for those failures doesn't hold. The line #658 quotes (... armed leading-duplicate fast-skip 127.0.0.3 1 data direct resume cursor 1785944669174) is at line 14043 of the leader's log (…-127_0_0_2-…/hdb.log), describing the leader's reverse subscription back to the clone. The restarted clone's own log in that same run does log Requesting full copy of database data and bulk copy complete 0.4s later. By your own discriminator both failures are healthy resumes with complete data that simply couldn't announce it. The underlying mid-copy cursor hazard in #658 may still be real, but the repro direction needs redoing.

Also, in your favour: the || 1 sentinel is a much narrower concern than the body concedes. Two corrections — the sender discipline it depends on landed in #255 (v5.1.0), not #480; and more decisively, the sentinel only engages when the leader reports no target at all (the RocksDB harper#2091 case or a genuinely empty database). An LMDB leader old enough to emit mid-copy sequence updates reports real targets and takes the strict comparison, so it never sees the sentinel. The intersection is effectively "empty database on a pre-v5.1.0 leader", where an early pass costs nothing. The sentinel's real weakness isn't leader version — it's that 1 has no per-attempt anchor, which is what makes a stale watermark from a prior attempt indistinguishable from this attempt's completion.

On #657 vs #650, since both are open and red for the same reason: I'd converge on this one. It's the smaller, conflict-free base and the only branch with large-copy evidence. But it needs #650's restart watermark restoration ported onto it — scoped by "is this copy known complete", not "is a clone happening", since both prior guards proved unsound across #650's rounds. #650's onCommit watermark move (which this PR's body names as its own unresolved gap) is worth landing as its own PR either way. The airtight version is the one you already name: expose per-database copy-mode exit through cluster_status, which retires the sentinel, the seeding and the !copyCursor guard together.

Reviewed by Claude Opus 5 for @DavidCockerill.

[DavidCockerill]

Round 2 at 65ef2272. Both round-1 findings are closed, and the clone-completion design is better than the port I suggested — replies in the threads.

The short version on the blocker: a random per-clone-operation attemptId, persisted atomically and exported as HARPER_CLONE_ATTEMPT before restartWorkers() so respawned workers inherit it, and re-read rather than re-minted on a resume. Completion filed to on-disk per-database dbisDB at COPY_COMPLETE, invalidated at COPY_START, reseeded under Math.max. I pressed specifically on the stale-value trap and it's genuinely absent: reuse within one clone operation is correct — the copy did finish and the bytes are durable at copyStartTime — while a fresh random id across operations is what makes this sound where !copyCursor and a bare in-progress marker were not. cloneResume.test.mjs is green at head and actually ran: 17.8s against a 120s ceiling, on run_attempt 1 with no retries.

This supersedes my round-1 recommendation. 511bdcb8 is an independent design, not a port of #650's restart seeding — deriveCloneTargets, cloneSyncBaseline and cloneAvailabilityFinalization remain unique to #650, which is unmoved at c8da9d59. So #657 now subsumes #650's purpose, with one exception: #650's onCommit watermark move exists nowhere else and is still worth landing as its own small PR. Suggested order — fix the two below, land #657, land the onCommit move standalone, close the rest of #650.

Two things block the merge, neither in the completion design:

1. A fresh clone from a v5 LMDB leader never publishes Available — system is required as a socket the clone never opens. Verified as a regression against main: cloneReadinessKeySet.test.mjs holds Unavailable for 158s with cloneCount=4000 missing=0, while the same test/shard/Node on main passes in 11.9s. Detail and my two caveats in the thread.
2. runLinter is red on an unused parameter — one character, but it's a required check.

Worth merging main in before re-running CI: the merge-base is 61a1531b while main is at 3a213e6b, so the base has moved a long way (Sync Core #671 plus the replication setup-watchdog series).

One unrelated observation while I was in the logs: Cluster Integration Tests 5/6 is red on main itself across Node v22/v24/v26, same test and assertion, on every recent run. While it stays red it masks genuine cluster regressions on every PR that touches replication.

Reviewed by Claude Opus 5 for @DavidCockerill.

[DavidCockerill]

Blocker — a fresh clone from a v5 LMDB leader never publishes Available. New at this head, and it's a verified regression rather than a flaky test.

getLastUpdatedRecord sets lastUpdated['system'] unconditionally (:613), and this filter keeps system for any leader not positively identified as v4 (systemSocketRequired, :533-545). So on every v5 leader the clone waits for a system replication socket. Against current main it never appears.

Cluster Integration Tests 2/6 at head, integrationTests/cluster/cloneReadinessKeySet.test.mjs:280:

AssertionError: clone never reported Available within 150000ms poll window
[qa762] SUMMARY becameAvailableAt=null predicateFirstSyncedAt=9067
        lastSample={"tRel":158211,"availability":"Unavailable","cloneCount":4000,"missing":0,"extra":0}

Complete data at t=9s, still Unavailable at t=158s. The clone's own log says why, once every 3s:

[cloneNode]: Database data: Synchronized
[cloneNode]: Database system: no replication socket to the leader yet

And there's nothing to wait for — the clone opens exactly one subscription (Connecting to ws://127.0.0.2:9933, db: data), the leader likewise, and cluster_status carries one socket, not two. Same test, same shard, same Node version on main @ 3a213e6b: becameAvailableAt=11889, passes in 12s.

Two things I'll be straight about. I can't pin this to the three new commits — systemSocketRequired already existed at 752ebc1e, and this test only landed on main at 4775fda (2026-08-05 18:53Z), about two hours after 752ebc1e's CI ran, so it has never run against this branch before. And I didn't isolate why the system socket stopped appearing; the strongest lead is the newly-sent isLeader: true (:318, :331) flipping writeNodeLocalOnly at setNode.ts:219 so the leader's hdb_nodes row is persisted LOCAL_ONLY — but subscriptionManager.ts:865-867 shows isLeader was already being inferred true for a clone, so I'd rather flag that as unconfirmed than assert it.

Neither caveat changes the outcome: merged into the current base, this wedges a fresh clone that holds a complete, correct copy. It's the round-1 fail-closed wedge, moved from the resume path to the normal one. Your own comment at :587-592 states the invariant — "its socket never exists, so a target would wedge the sync monitor" — it just isn't applied to system.

Property the fix needs: system should only be a required socket when the clone will actually subscribe to it, derived from the same source that decides subscriptions — not asserted from the leader's major version. Mechanism is your call; a soft requirement with the strict version-probe gate kept for the ordering race would also do it.

[DavidCockerill]

runLinter is red on this line — trivial, but it's a required check so it blocks the merge:

##[error]Parameter 'entry' is declared but never used. Unused parameters should start with a '_'.
Found 57 warnings and 1 error.

_entry fixes it. This is the only new site in the diff that binds an unused parameter by that name — the (entry: any) => boolean occurrences are type annotations and don't trip the rule (they passed lint at 752ebc1e).