Skip to content

feat(replication): verify resume-claimed copy ranges with key checksums#554

Open
ldt1996 wants to merge 6 commits into
mainfrom
feat/copy-range-checksum
Open

feat(replication): verify resume-claimed copy ranges with key checksums#554
ldt1996 wants to merge 6 commits into
mainfrom
feat/copy-range-checksum

Conversation

@ldt1996

@ldt1996 ldt1996 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Detection layer for #553; the at-rest follow-up agreed in the #538 review.

On a resume the sender first checks quiescence: any write to the database (any origin, any table) since copyStartTime and verification is silently skipped, because the claimed-delivered range is only invariant while nothing writes (the receiver's copy is frozen mid-copy). Quiescent, it checksums the key range the cursor claims was delivered (prior tables in copy order plus the resume table through afterKey inclusive; keys only via a type-tagged canonical form since ordered-binary yields BigInt past 2^53 and Harper's BigInt.prototype.toJSON override rules out JSON; local-only and route-excluded tables skipped; wall-clock paced; capped, with a whole-scan time budget). The checksums ship as a COPY_RANGE_CHECKSUM message carrying the exact bounds, cap, and copyStartTime used; the receiver re-checks quiescence on its own audit log (before and after its scan) and compares over the identical range. Under the checked precondition the comparison is exact: a mismatch means the range provably was not delivered.

Alert-only: an error log with per-table key counts plus a copyResumeRangeMismatch marker on the connection. No re-copy and no cursor surgery. Old peers interoperate unchanged in both directions. Busy databases skip; the poisoned-cursor class this targets (#537) presents at post-incident resyncs and quiescent restarts, where the gate passes.

The checksum core, range-scan helper, and quiescence gate are pure exports with unit tests, including a golden vector pinning the wire values; replication suite 325 passing.

Lavinia, via Claude

🤖 Generated with Claude Code

ldt1996 and others added 2 commits July 9, 2026 09:54
…ms, alert-only

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…scan budget, golden vector (verify)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ldt1996 ldt1996 requested a review from a team as a code owner July 9, 2026 07:15

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a resume-range checksum verification mechanism to ensure consistency during replication copy resumes. It adds rolling checksum generation, table range scanning with event-loop yielding, and comparison helpers, along with comprehensive unit tests. The review feedback suggests defensive programming improvements, specifically adding null checks and replacing unsafe type assertions in checksumTableRange and compareRangeChecksums.

Comment thread replication/replicationConnection.ts Outdated
Comment thread replication/replicationConnection.ts
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@ldt1996 ldt1996 marked this pull request as draft July 9, 2026 09:44
@ldt1996

ldt1996 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

The integration failures on this PR are not from this diff: main's own push-triggered integration run (28985156996, after #527/#529 landed) fails the identical test set (copy-progress wedge recovery, open-but-idle wedge recovery, non-replicated database, deploy replication, cached blobs), and a workflow_dispatch of an adjacent branch without the main merge runs all four shards green. PR checks run on the merge ref, so this branch inherits main's breakage.

Lavinia, via Claude

@ldt1996 ldt1996 marked this pull request as ready for review July 9, 2026 11:39
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kriszyp

kriszyp commented Jul 9, 2026

Copy link
Copy Markdown
Member

I don't understand why we would expect a table to be the same before and after a copy (only the case if no concurrent writes, rare).

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks fine, no issues, but it's a bit over my head.

… exact or silent (review)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ldt1996

ldt1996 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

You're right, and half of it was already priced in: the checksum is keys-only precisely so updates and upserts (the common concurrent-write class) never move it. But key-set churn since copyStartTime — new keys sorting into the already-delivered range, or deletes — does drift the comparison legitimately, and an alert that is expected to false-fire under load is not worth having.

Reworked in 873d9de so your objection becomes the precondition instead of a caveat: both sides check their audit logs for any write to the database since copyStartTime (one bounded seek per log, re-checked after the scan to close the mid-scan race). Any write anywhere: skip silently, no checksum, no alert. Fully quiescent: the claimed range is invariant, the comparison is exact with no tolerance, and a mismatch means the range provably was not delivered. Exact or silent.

The honest scope that falls out: on a busy database this rarely fires, and that is fine — the poisoned-cursor class it exists to catch (#537) presents at post-incident resyncs and quiescent restarts, which is exactly where the gate passes. Our stage cluster's poisoned cursors were all in that window.

Lavinia, via Claude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants