Order replication base copy: control-plane tables before bulk tables (#421) by kriszyp · Pull Request #422 · HarperFast/harper-pro

kriszyp · 2026-06-19T05:26:57Z

Fixes #421.

Summary

The system database base copy iterated tables in insertion order (for (const tableName in tables)), so a large hdb_analytics table (~node-local telemetry, can reach millions of rows) gated convergence of small control-plane tables. In particular hdb_deployment stayed behind hdb_analytics for the whole copy, so deploy_component's awaitDeploymentRow kept hitting its 120s timeout after any restart-triggered full copy (observed live on JJill preprod 5.1.5).

This orders the base copy so control-plane tables (hdb_deployment, hdb_nodes) stream first and bulk tables (hdb_analytics) last; everything else keeps insertion order. orderTablesForCopy is a pure function of the table-name set, so it stays deterministic across runs — which the resume skip-loop depends on. Only the system DB has these names, so user databases are unaffected.

Where to look

The cross-version resume guard is the subtle part. The skip-loop trusts that every table before the cursor's currentTable was already copied — true only if the resume runs under the same order that built the cursor. Reordering would otherwise let a copy interrupted under the old order and resumed under the new one (exactly the upgrade scenario this fixes) silently skip hdb_deployment behind hdb_analytics. So a COPY_ORDER_VERSION is stamped through COPY_START (message[2]) → persisted resume cursor (copyOrder) → copyResume; a leader resuming a cursor whose order version doesn't match (or is absent/pre-versioning) recopies from scratch instead of trusting the skip (isCopyResumeOrderCompatible). The mixed-version leader/follower matrix was traced and is safe in all four permutations (recopy is idempotent puts; the copyStartTime anchor is captured before the reset, so the post-copy audit-replay window is preserved).

Tests

Unit tests for orderTablesForCopy (priority placement, stability, determinism, permutation-preservation) and isCopyResumeOrderCompatible (match / stale / pre-versioning-undefined). Full replication unit suite passes (140 + 11).

Open items for the reviewer

Integration regression test deferred. The issue's last AC asks for a cluster integration test (large hdb_analytics + restart, assert hdb_deployment converges in bounded time). A faithful repro needs system-table manipulation and convergence-timing assertions (heavy + timing-flaky), and the existing base-copy harnesses operate on user DBs where the control-plane names don't apply. I covered the fix logic at the unit level and propose the integration test as a fast-follow — happy to add it here if preferred.
Latent, pre-existing: the multi-subscription copyResume clobber (keeps the last subscription's cursor) is now load-bearing for the order-version guard. Single-cursor in practice for the system DB; I added a comment noting the assumption rather than changing behavior.

Cross-model reviewed (Codex + Gemini + Harper replication-domain pass): no blockers; the perf suggestion and guard-test gap surfaced there are addressed in this diff.

🤖 Generated by Claude (Opus 4.8, 1M context).

…e copy (#421) The system database base copy iterated tables in insertion order, so a large hdb_analytics table gated convergence of small control-plane tables (hdb_deployment), causing replicated deploys to fail after any restart-triggered full copy. Order the base copy so control-plane tables (hdb_deployment, hdb_nodes) stream first and bulk tables (hdb_analytics) last; everything else keeps insertion order. Reordering would otherwise break the resume skip-loop across an upgrade: a cursor built under the old order, resumed under the new one, could skip tables the old order had not yet reached. Stamp a COPY_ORDER_VERSION through COPY_START -> resume cursor -> copyResume; a leader resuming a cursor whose order version doesn't match (or is absent, pre-versioning) recopies from scratch instead of trusting the skip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

gemini-code-assist · 2026-06-19T05:27:02Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

claude · 2026-06-19T05:29:23Z

Reviewed; no blockers found.

gemini-code-assist · 2026-06-19T06:29:00Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

cb1kenobi · 2026-06-20T15:41:25Z

+ *
+ * Exported for `unitTests/replication/orderTablesForCopy.test.mjs`; production calls it inline below.
+ */
+export function orderTablesForCopy(tableNames: string[]): string[] {


This feels overkill. I would have kept things simple and hard coded a priority next to the table names. I wouldn't think it really matters if hdb_deployment goes before or after hdb_nodes as long as hdb_analytics goes last.

The previous commit's new case asserted (undefined, 1) as "a real pre-barrier leader's cursor" — wrong: COPY_ORDER_VERSION is #421/#422's copy-table-order versioning, unrelated to and unchanged by this PR, so a pre-#876 leader already sends copyOrder:1 and is accepted, not rejected. undefined is a pre-#421 leader instead. Caught by codex in this task's own pre-push review round. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…timestamp (#878) * Anchor the replication base copy on a log-order barrier instead of a timestamp `copyStartTime` was `Date.now()`, captured before the key-order table walk and published as the follower's resume cursor. But a transaction's log key is fixed when the transaction is CREATED while its log batch is appended when it COMMITS, so the transaction log is written out of key order — measured at 47 inversions per 1000 concurrent transactions. A transaction created just before the anchor and committed during the copy therefore sits behind the anchor numerically and after it physically: the walk had already passed its row, and the post-copy tail's per-entry range predicate (`timestamp > start`) excluded it. Neither side delivered it, and nothing ever would. The leader now commits a `copyBarrier` entry before `COPY_START` and anchors on its transaction key. Everything still in flight commits — and so appends — after that entry, and the tail resumes with `exactStart` + `resumeAfterExactStart`, after which the range predicate stops consulting the key at all. The boundary iterable is built before the walk, because `getRange` resolves the position and maps the log file eagerly, so a long copy cannot outlive the barrier it is anchored to. The same append-order resume is used on reconnect whenever the cursor names a `copyBarrier` entry, so the anchor survives a disconnect between `COPY_COMPLETE` and the in-flight commit. An ordinary cursor keeps the timestamp range, so the default subscription path is unchanged and pays nothing. `COPY_ORDER_VERSION` is bumped to 2 and now versions what the anchor MEANS as well as the table order: a v1 cursor surrenders its anchor along with its walk position rather than carrying a timestamp anchor into a leader that cannot make it good, and a v2 cursor whose barrier has left the log restarts instead of resuming onto a guarantee nothing can keep. The anchor is validated on both peers before it reaches any state. Not covered, and stated in the PR: a relayed origin's log, where the barrier does not exist and the timestamp range still applies. Fixes #876 Dispatch-Task: hp-basecopy-inflight-write-loss Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HE8PfPhHn1o1Wss4uH3ULD * Degrade to wall-clock anchoring when a barrier write fails at call time findCopyBarrierTable() duck-types Table.writeCopyBarrier() as available whenever the method exists on the class, but the method itself hard-throws when the table's actual primaryStore isn't RocksDB. STORAGE_IS_ROCKSDB is a config snapshot that can disagree with a table's real engine (observed here: it read true in a replication worker thread whose harper instance was actually configured for LMDB, because env.get(CONFIG_PARAMS.STORAGE_ENGINE) returned undefined in that context). Previously this mismatch bubbled the thrown Error up through COPY_START's handler into the outer catch, which closed the connection with 1008 and killed the whole subscription -- reproduced deterministically via integrationTests/cluster/fullyConnectedReplication.test.mjs's LMDB suite (4-node full mesh), where every base copy of a non-empty database hits this path. Every other "can't form a barrier" case in this feature already degrades to the pre-#876 wall-clock anchor instead of refusing (empty database, a core submodule predating writeCopyBarrier, a boundary that fails its probe). This makes writeCopyBarrier() failing at call time follow the same rule: catch, warn once, and fall through to Date.now() -- exactly the invariant the surrounding code already states in its own comments, just missing this one call site. Verified: fullyConnectedReplication.test.mjs 10/10 (both RocksDB and LMDB suites); RocksDB suite's per-instance logs confirm it still writes and uses real barriers (zero fallback warnings); LMDB suite's logs show the new catch firing and the connection surviving. addNodeFullCopy, copyResumeCursorGap, and directionalFlowReplication cluster tests still pass 1/1. harper-pro test:unit 1254/0. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix resumeAnchorKind going stale relative to a cleared copyResume Adjudicated finding from the pre-push review of the prior commit: resumeAnchorKind is classified against the ORIGINAL copyResume, before the three guards above (order-version mismatch, invalid anchor, missing table) can clear it. Once cleared, a stale resumeAnchorKind === 'barrier' combined with the previous commit's new catch around writeCopyBarrier() lets a fresh-barrier-write failure fall through to treating Date.now() as if it named a real barrier entry -- something the prior commit's throw-and-close behavior made unreachable, but that catching the throw newly exposed. Gate the stale classification on copyResume still being present. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix txnlogTearReplication's frame oracle to tolerate a base-copy barrier frame Full CI (Cluster Integration Tests 6/6) failed after the rebase: "holds 61 frames for 60 rows; the oracle maps frame k to row k and needs one frame per row". Root cause: this test's before() hook does a plain add_node join on RocksDB with an empty table, which is exactly the base-copy path this PR changed -- it now commits one copyBarrier control frame into the joining leader's local transaction log before any row is ever inserted (confirmed via the run's own debug log: "bulk copy starting/complete" precedes the test's insertRows calls). That shifts every later frame's index by one, breaking the test's "frame k is row k" assumption -- an assumption that predates this PR and was true only because no base copy had ever written into this log before. Writing the barrier as a control entry through an arbitrary table (rather than some log-level-only channel) is this PR's own already-adjudicated design decision, shared with the existing lockBarrier/reloadMarker control types in the same log -- not something to redesign from a rebase task. The fix is at the right altitude: the oracle now filters to frames that actually carry one of the test's own rows (the same marker-matching check it already does per-frame, just applied as a filter instead of assumed by position), so a leading control frame no longer breaks the count. A corrupted or missing row still fails loudly, since the filtered count would then miss TOTAL. Verified: the test passes twice in a row (clean re-runs, not the same process), and its diagnostic line now reports the row-frame count ("of 60") rather than the raw frame count. harper-pro test:unit 1254/0 unaffected. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Bound the txnlog tear oracle's tolerated non-row frame count Adjudicated finding from the pre-push review of the prior commit: filtering to row-carrying frames alone would also hide a barrier-per-retry proliferation -- the reconnect-loop risk replication/DESIGN.md names as the reason fail-closed was withdrawn. Bound non-row frames to the one base-copy barrier this test's single add_node can produce, so that regression would fail this test again. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Reorder the txnlog tear oracle's checks and loosen its non-row bound Two issues the prior commit's review found in its own bound check: 1. It ran before the row-count check, so a corrupted/missing row (which also depresses the row-frame count) was misdiagnosed as barrier proliferation instead of as itself. Row-count now checked first. 2. The exact <= 1 bound is flake-prone: a legitimate reconnect between add_node and B's first cursor can mint a second barrier on retry, which is not the proliferation risk the bound exists to catch. Loosened to <= 3 -- a wedged retry loop mints one barrier per attempt and blows well past a couple of incidental early reconnects. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Release the boundary-probe iterator instead of abandoning it The pre-COPY_START probe pulled one entry via `getRange(...)[Symbol.iterator]().next()` and then dropped the iterator without calling `.return()`. Single-log getRange wires `return` through to the underlying log iterator specifically for early-exit cleanup; leaving it uncalled means every base copy that forms a barrier boundary leaves that handle open until GC. Wrap the probe pull in try/finally so it's released immediately. The multi-log aggregate branch of RocksTransactionLogStore.getRange has no `.return()` at all, so this only closes the gap on the single-log path; the aggregate path's lack of a teardown hook is a pre-existing, broader core limitation this PR doesn't touch. Found by an independent Gemini review pass. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix classifyResumeAnchor's docstring to match its one caller's behavior The comment claimed callers fail closed on 'unreadable'. The only caller (the copy-resume anchor check at replicationConnection.ts:5972) only branches on `=== 'barrier'`; 'other' and 'unreadable' both fall through identically to degrading to the pre-#876 timestamp anchor, same as DESIGN.md's stated philosophy for every other unformed boundary. Doc didn't match code. Found by an independent Gemini review pass. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Retry immediately when the live-tail boundary check fails The backstop that clears a failed append-order auditLogIterable fell through to `await nextTransaction` instead of retrying right away. `nextTransaction` only resolves on a future commit; on an otherwise idle database, whatever the failed boundary couldn't deliver would sit stranded until an unrelated write happened to unstick it. Add a `continue` so the loop rebuilds an ordinary range from currentSequenceId immediately, the same way it would on a fresh pass. Found independently by both Codex and Gemini review passes. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Align a test description with classifyResumeAnchor's corrected docstring Same stale "fail closed" framing as the JSDoc fixed in the prior commit; the assertion itself (return value === 'unreadable') was already correct and unchanged. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Disable RocksDB flush-on-exit for the unit-test process CI started failing after the rebase's rocksdb-js 2.9.1 -> 2.10.0 bump (from main's Sync Core commits): all 1253+ tests passed, then the process crashed with "Failed to flush database during close: ... No such file or directory". Root cause: two independent `process.on('exit', ...)` listeners race. unitTestSetup.cjs registers first (via --require) and rmSync's the whole shared STORAGE_PATH; RocksTransactionLogStore.ts's own exit listener (registered later, whenever a test first imports it) then tries to flush every still-known RocksDB log into directories that listener just deleted. Node fires exit listeners in registration order, so this ordering was always latent -- 2.10.0 apparently just started throwing on it where 2.9.1 didn't. The flush-on-exit hook exists for crash/replay coverage, which lives in integrationTests (grep confirms zero unitTests reference HARPER_NO_FLUSH_ON_EXIT); unit tests close their own databases explicitly and don't need it. Setting the env var here, before any test file can import RocksTransactionLogStore.ts, skips registering that second listener entirely. Reproduced and verified locally against the post-rebase package-lock.json (my node_modules had gone stale after the rebase bumped it; `npm ci` was needed to see the crash at all) -- 2/2 clean runs after the fix, both crashing before it. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Test isCopyResumeOrderCompatible against today's real COPY_ORDER_VERSION The existing case only checked a hypothetical 1-vs-2 version mismatch; a real pre-#876 leader's cursor has no copyOrder at all, and today's COPY_ORDER_VERSION is still 1, not 2 — add that case and stop implying the hypothetical one covers it. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Correct the isCopyResumeOrderCompatible test's own inaccurate claim The previous commit's new case asserted (undefined, 1) as "a real pre-barrier leader's cursor" — wrong: COPY_ORDER_VERSION is #421/#422's copy-table-order versioning, unrelated to and unchanged by this PR, so a pre-#876 leader already sends copyOrder:1 and is accepted, not rejected. undefined is a pre-#421 leader instead. Caught by codex in this task's own pre-push review round. Dispatch-Task: pr-maint-352723a0e8c43f4ff8e901f0fc934706 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Anchor the base copy on the last committed log entry instead of writing a barrier The copy no longer commits a copyBarrier entry to form its boundary: the last committed entry of the local log is already one. The log yields entries only up to its committed watermark, which rocksdb-js advances past a transaction only after its RocksDB commit succeeds and only across a contiguous prefix, so every transaction still in flight appends after that entry whatever its key. - findLastCommittedLogKey scans a widening key window back from now (1s, 1min, 1h, 1d, whole log) and takes the last entry yielded; an empty log gets an ordinary range from its start, which is already a boundary. - classifyResumeAnchor now asks only whether the anchor names an entry: an exact match resumes in append order, anything else keeps the timestamp resume, so pre-#876 Date.now() cursors are honoured unchanged. - Gate on auditStore.reusableIterable rather than the STORAGE_IS_ROCKSDB config snapshot, which misreports the engine in a worker. - Drops the core submodule bump (harper#2697 is no longer needed) and the txnlogTearReplication oracle loosening, which only tolerated barrier frames. - Restores the #737 DESIGN.md paragraph an earlier rebase of this branch lost. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Bisect for the latest committed log key instead of scanning a window A quiet log made the widening window fall through to a whole-history scan on the copy's setup path. A range seeks by key and its first entry sits at the seek position, so bisecting on one pulled entry per probe finds the largest committed key in a bounded number of probes without reading history. Also drops the unit-test exit-flush override: main fixed the underlying exit ordering in #893, and the override masked that change's lifecycle test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Pin that a same-version copy cursor is accepted Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Correct the anchor design note after bisection, and say why an empty log sends the wall clock Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

kriszyp requested review from cb1kenobi and kylebernhardy June 19, 2026 05:27

kriszyp marked this pull request as ready for review June 19, 2026 06:28

kriszyp requested a review from a team as a code owner June 19, 2026 06:28

kriszyp mentioned this pull request Jun 19, 2026

Incomplete (truncated) source blob is misclassified as transient → permanently wedges replication cursor (ENOENT is handled; 'Blob is incomplete' is not) #429

Closed

cb1kenobi approved these changes Jun 20, 2026

View reviewed changes

kriszyp merged commit e56d4a4 into main Jun 22, 2026
62 of 64 checks passed

kriszyp deleted the kris/system-copy-priority branch June 22, 2026 12:29

This was referenced Jul 1, 2026

Replication W13: Base-copy & catch-up path (consolidation & correctness) #510

Open

[Epic] Replication reliability & scale — foundations, adaptive topology, and locking #430

Open

Navigation Menu

Repository navigation

Order replication base copy: control-plane tables before bulk tables (#421) - #422

Order replication base copy: control-plane tables before bulk tables (#421)#422
kriszyp merged 1 commit into
mainfrom
kris/system-copy-priority

kriszyp commented Jun 19, 2026

Uh oh!

gemini-code-assist Bot commented Jun 19, 2026

Uh oh!

claude Bot commented Jun 19, 2026

Uh oh!

gemini-code-assist Bot commented Jun 19, 2026

Uh oh!

cb1kenobi Jun 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kriszyp commented Jun 19, 2026

Summary

Where to look

Tests

Open items for the reviewer

Uh oh!

gemini-code-assist Bot commented Jun 19, 2026

Uh oh!

claude Bot commented Jun 19, 2026

Uh oh!

gemini-code-assist Bot commented Jun 19, 2026

Uh oh!

cb1kenobi Jun 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants