Repository navigation
fix(replication): resumable bulk clone copy (no restart-from-zero) - #255
Merged
Merged
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 resumable initial bulk clone copy mechanism for replication, allowing followers to resume copying from a persisted primary key cursor after a disconnect rather than restarting from scratch. The changes include tracking the copy state, checkpointing progress, and resuming from the last committed key, supported by new integration tests and updated design documentation. The review feedback highlights three critical issues: a runtime TypeError caused by calling a non-existent .remove method on the LMDB database instead of .delete, incorrect arguments passed to tableToTableEntry (strings instead of table objects) along with a non-null assertion violation, and potential protocol corruption due to a missing encodingStart update when forcing a transaction.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. |
kriszyp
force-pushed
the
replication-clone-keepalive
branch
from
June 1, 2026 13:11
83f9c29 to
ce34c10
Compare
kriszyp
marked this pull request as ready for review
June 2, 2026 12:58
kriszyp
force-pushed
the
replication-clone-keepalive
branch
from
June 2, 2026 17:47
ce34c10 to
751adb6
Compare
…ime) The bulk clone copy walks the primary store in key order, but the follower resumes replication from the audit log in time order. Recording the resume point as max(localTime) of the copied records could skip a write committed during the copy to a key the copy had already passed (its localTime falls below the max), silently losing it on the follower. Capture copyStartTime before iterating and use it as the post-copy resume point, so the audit replay re-delivers every write from the copy window. Refs #241 (Part 2; foundational safety fix preceding resumable copy). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When a follower cloning a large table loses the connection mid-copy, the copy
previously restarted from zero and (for a large enough table) never converged.
Make the bulk copy resumable:
- Leader sends COPY_START{copyStartTime}, flushes a checkpoint transaction every
COPY_CHECKPOINT_RECORDS (timed at copyStartTime so the persisted seqId stays
pinned there), and sends COPY_COMPLETE at the end. On resume it skips tables the
follower already committed (stable iteration order) and continues the in-progress
table after the last committed key.
- Follower tracks a resume cursor {copyStartTime, currentTable, afterKey} and
persists it in the end_txn onCommit — AFTER the batch commits — so the cursor can
never get ahead of committed data (a resume re-copies idempotently, never skips).
On reconnect it sends the cursor as copyResume on the subscription request instead
of restarting; COPY_COMPLETE clears it.
Cursor persistence is entirely pro-side (dbisDB Symbol.for('copyCursor')), no core
change. Happy-path clone verified; mid-copy-disconnect resume test follows.
Refs #241 (Part 2).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Integration test: clone a 2000-row table from a leader with frequent copy checkpoints, kill the follower mid-copy, then restart it on the same data dir. Asserts the copy resumes from the persisted cursor and every record is present afterward (no skipped rows), with first/last rows spot-checked at the resume boundary, and confirms the copy was actually interrupted mid-stream. Refs #241 (Part 2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… only after commit Address two data-loss findings from cross-model review of the resumable copy: - Mid-copy checkpoints emitted an end_txn at copyStartTime, advancing the follower's received-version watermark. monitorSync could read that as "caught up" and mark the clone Available/cloned with rows still uncopied; a later restart would then skip the clone with missing data. Checkpoints now do a plain buffer flush (no sequence update) so progress commits and the resume cursor advances, but the watermark only reaches copyStartTime via the single end_txn emitted after the entire copy completes. - COPY_COMPLETE cleared the resume cursor synchronously, but earlier copied batches may not have committed yet (commits are async). A crash in that window lost both the cursor and the uncommitted rows. Removal is now deferred until COPY_COMPLETE has arrived AND outstanding commits have drained. Test: make the interrupt reliably land mid-copy (more records, aggressive receive throttling, faster poll) now that the copy is lighter without per-batch end_txns. Refs #241 (Part 2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…sync Second round of cross-model review findings on the resumable copy: - The resume cursor was persisted before the batch's blob writes were awaited (onCommit defers commit confirmation until outstandingBlobsToFinish drains for exactly this reason). A crash after the cursor advanced but before a blob finished would skip re-requesting that blob, leaving a record pointing at missing data. Moved cursor persist + clear to after blob completion. - The receive path advanced RECEIVED_VERSION_POSITION per copied record. Since copy records carry their original versions, a record at the leader's latest timestamp could push the watermark to the clone's target mid-copy, and checkSyncStatus marks the clone Available on lastReceivedVersion >= target — marking it cloned/Available with rows still uncopied. Suppress that per-record advance while inCopyMode; the watermark only reaches copyStartTime via the single end_txn after the whole copy completes. Refs #241 (Part 2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
If the last copied rows landed exactly on a checkpoint flush, the flush reset currentTransaction.txnTime and encodingStart, so the post-copy end_txn — guarded by position - encodingStart > 8 — was skipped. Since the per-record received- version advance is now suppressed during the copy, that final end_txn is the only thing that advances seqId/receivedVersion to copyStartTime, so skipping it left the clone unable to ever reach Available (any row count that is an exact multiple of COPY_CHECKPOINT_RECORDS). Emit it unconditionally. Refs #241 (Part 2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
COPY_COMPLETE flipped inCopyMode=false synchronously, so batches still committing afterward skipped the cursor write (their onCommit saw inCopyMode=false), freezing the cursor while later state advanced — a crash in that window could leave seqId at copyStartTime with a stale/removed cursor and skipped rows. Now COPY_COMPLETE only records that completion was signalled; maybeFinishCopy leaves copy mode and clears the cursor together once outstandingCommits drains, so the cursor keeps advancing through the drain and is only dropped after the whole copy is durably committed. Refs #241 (Part 2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ed cursor table - Keeping inCopyMode true through the post-COPY_COMPLETE drain (previous fix) meant the leader's normal audit-log replay frames — which arrive right after COPY_COMPLETE and are time-ordered, not primary-key-ordered — could be persisted as the resume cursor, so a crash before maybeFinishCopy could resume from a bogus key and skip uncopied data. Capture per-frame, at decode time, whether it is a bulk-copy frame (received before COPY_COMPLETE) and only advance the cursor for those. - The resume skip loop only restarted a full copy when the cursor's currentTable was absent from `tables`; a table that is present but no longer replicated slipped past, so the loop never reached it and silently omitted every later table. Detect that via tableToTableEntry and restart the full copy instead. Refs #241 (Part 2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…r table The previous guard called tableToTableEntry(currentTable) with a name string, but that helper reads table.tableName off its argument, so for replicate-by-default it returned truthy for any string and failed to notice the cursor's table was gone — the skip loop then never reached it and omitted every later table. Mirror exactly what the loop requires to visit a table (present in `tables` AND passing the same filter) so a dropped or unreplicated cursor table forces a full restart. Refs #241 (Part 2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- maybeFinishCopy now always exits copy mode (and resets state) once COPY_COMPLETE is received and commits drain; only the cursor removal is gated on a known node id. Previously, if getIdOfRemoteNode returned undefined at COPY_START, inCopyMode stuck true and the watermark stayed suppressed so the node never reached Available (claude[bot] blocker). - Set encodingStart = position before forcing the post-copy txn timestamp, so a forced end_txn after a checkpoint flush can't emit a malformed frame (gemini). - The resume skip loop now passes table OBJECTS to tableToTableEntry (it reads table.tableName) instead of name strings, and uses captured resumeCurrentTable/ resumeAfterKey to drop the non-null assertion (gemini). This also makes a dropped/unreplicated cursor table correctly trigger a full restart. (Not changed: dbisDB.remove — gemini flagged it as needing .delete, but .remove is the correct API for this store, used throughout core/resources; verified by tests.) Refs #241 (Part 2). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
kriszyp
force-pushed
the
replication-clone-resume
branch
from
June 2, 2026 17:52
2033323 to
0fb170e
Compare
kriszyp
added a commit
that referenced
this pull request
Jun 10, 2026
If the last event decoded in a copy frame has no table name (event.table is undefined), the persisted copyCursor would store currentTable: undefined. On every subsequent reconnect the leader checks tables[copyResume.currentTable] — tables[undefined] is always falsy — and the full copy restarts from zero, defeating the resumable-copy feature entirely (#255). Add event.table to the cursor-write guard so a batch whose final decoded event carries no table name leaves the existing (valid) cursor in place instead of overwriting it with an invalid one. A warn log fires in that case to surface the underlying cause. Fixes #321. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of 2 for #241. Stacked on #248 (base
replication-clone-keepalive) — review/merge #248 first; I'll rebase this tomainonce it lands. The diff shown here is Part 2 only.Problem
Even with Part 1's keepalive, a follower cloning a large table that loses the connection mid-copy restarts the bulk copy from zero. For a large enough table it never converges. The copy walks the primary store in key order, but normal replication resumes from the audit log in time order — so naive checkpointing risks skipping rows.
Change
A resumable, skip-safe bulk copy:
COPY_START{copyStartTime}, copies each table in PK order, periodically flushes a checkpoint (plain buffer flush, no sequence update), and sendsCOPY_COMPLETE. On resume it skips tables the follower already committed and continues the in-progress table after the last committed key (copyResumeon the subscription request).{copyStartTime, currentTable, afterKey}(dbisDBSymbol.for('copyCursor')) in theend_txnonCommit, after the batch + its blobs are durable, so the cursor is never ahead of committed data — a resume may re-copy a few rows (idempotent puts) but never skips. On reconnect it sends the cursor;COPY_COMPLETEclears it once outstanding commits drain.copyStartTimevia the single post-copyend_txn(per-record/checkpoint advances are suppressed during the copy), somonitorSynccan't mark the clone Available with rows still uncopied. The post-copyend_txnis emitted unconditionally. Entirely Pro-side; no core change.Where to look (highest-risk first)
COPY_START→ checkpoints →COPY_COMPLETE, resume skip/afterKey) and the follower'sonCommitcursor persistence +maybeFinishCopyinreplication/replicationConnection.ts. The cursor-durability ordering (after blobs), theisCopyFramegate (don't cursor post-COPY_COMPLETEaudit-replay frames), and the watermark-suppression are the subtle parts.Testing
integrationTests/cloneNode/cloneResume.test.mjs— kills a follower mid-copy, restarts it, asserts the copy resumes from the cursor with all rows present (no skip) + first/last spot-checked. Happy-path clone (cloneNode.test.mjs) and reconnect regression both pass; unit suite green.Review notes
This touched a genuinely subtle area, so I ran Codex cross-model review iteratively (6 passes); it surfaced 8 data-integrity issues across rounds — premature-Available via the watermark, cursor-clear-before-commit, cursor-before-blobs, per-record watermark advance, the final-
end_txnboundary skip, copy-mode-exit timing, cursor-from-audit-replay frames, and dropped/unreplicated cursor table — all fixed (see commit history). The resume edge-cases (schema change mid-clone, exact checkpoint boundaries) are the areas most worth a careful human read. Gemini CLI is unavailable in my env; thegemini-code-assist[bot]review on the PR will provide the second model.🤖 Generated with Claude Code (model: Claude Opus 4.7)