feat(replication): alert on copy delivery shortfall at COPY_COMPLETE

[ldt1996]

Detection layer for #537.

At COPY_COMPLETE the sender now reports how many records it actually sent per table in this copy session, and the receiver compares that against the records that arrived intact. Both sides count the same stream, so the comparison is exact: no table scans, no estimates, no delay before COPY_COMPLETE, no race with concurrent writes or eviction, and a resumed copy compares only its own tail. A deficit means in-flight loss or undecodable drops, which the decode path otherwise skips silently while the cursor advances past them.

Alert-only by design: an error log with the per-table deltas, a copyDeliveryShortfall marker on the connection, and a cumulative shortfall count + last-occurrence time in cluster_status (same slot pattern as the blob-failure tripwire from #386). No forced re-copy (permanently undecodable records would loop it forever) and no cursor or seq surgery. Old senders (no payload) and old receivers (payload ignored) interoperate unchanged.

computeCopyDeliveryShortfall is a pure export with unit tests; replication suite 281 passing.

Lavinia, via Claude

🤖 Generated with Claude Code

[github-actions]

Patch cherry-pick: cancelled

The patch label was removed; cherry-pick branch cherry-pick/v5.1/pr-538 was deleted.

[gemini-code-assist]

Code Review

This pull request introduces a copy verification mechanism during replication to detect poisoned resume cursors. Upon completing a bulk copy, the sender transmits its per-table row counts (excluding evicting/expiring tables) to the follower. The follower compares these counts with its local tables and, if a significant shortfall is detected, clears the resume cursor and sequence ID to force a fresh full copy. Feedback suggests wrapping the follower's getRecordCount call in an inner try...catch block to prevent a single table failure from aborting the entire verification loop, and adding a defensive check for getRecordCount on the sender side to avoid potential runtime errors.

[ldt1996]

Both addressed in b8a84a2: the receiver counts each table in its own try so one slow or failing count no longer skips the rest, and the sender now checks getRecordCount exists before calling it (its try already caught the TypeError, but the explicit guard matches the receiver).

Lavinia, via Claude

[claude]

Reviewed; no blockers found.

[ldt1996]

Good catch, and it was the worst variant slipping through: fixed in 1aff8ce by snapshotting the source node id before maybeFinishCopy runs, and the verification closure uses the snapshot throughout so an async finish during a count scan can't corrupt the cursor keys either.

Lavinia, via Claude

[kriszyp]

Thanks @ldt1996 — the diagnosis on #537 was the hard part here, and pinning the symptom (a resume cursor that outruns delivered data, then re-seals the hole on every resume) is what made the root cause findable. 🙏

After digging into where the cursor actually gets ahead of delivery, I think the count-verification approach may be aimed one layer too high, and wanted to lay out the reasoning so we can decide together whether to close this or repurpose it.

Where the hole comes from. The receive-apply path wraps record decode in a bare catch that logs and skips any decode failure, letting the resume cursor advance past the dropped record. The dominant real-world cause is a shared-structure fork (harper#1163) during mixed-version churn — exactly the rolling-restart conditions in #537 — which makes a whole class of records undecodable on the laggard. Those get silently dropped and the cursor sails past them. That's the "thousands of rows below the source, believing themselves current" you observed.

Why a count-triggered re-copy is counterproductive for that class. Those records are permanently undecodable on that node (structon/msgpackr already reload-and-retry structures before throwing; the remaining ones are old-version/corrupt bytes no re-copy heals). So a forced fresh copy re-ships the same bytes, re-skips them, and — because the mismatch latch resets per connection — re-fires on the next completing resume: a full-copy loop that never converges. The max(100, 10%) estimate-vs-estimate tolerance (with eviction/TTL tables excluded) also can't reliably tell this permanent shortfall apart from a genuine gap.

Proposed root-cause fix: #545. It classifies the decode failure and surfaces the permanent skip — the missing-structure subclass fires the same decode-missing-structure metric core's local-read path uses, so it's alertable instead of laundered as emptiness — while accepting that undecodable records are unrecoverable (skip + advance is correct; holding would just wedge the leg).

Where that leaves this PR. My honest read is that once the drop is surfaced at the source, the count check no longer has a job it can do safely — the permanent case shouldn't trigger a re-copy, and genuine transient gaps are better prevented upstream than detected after the fact. So I'd lean toward closing this.

That said, I'd genuinely welcome pushback, and there may be a narrower reuse worth keeping: the row-count delta is a decent observability signal even if it shouldn't trigger a re-copy. If we want a real delivery-completeness backstop, a deterministic per-range key checksum on resume would be more trustworthy than a fuzzy whole-table count. Happy to go either way — what's your instinct?

— KrAIs (Claude Opus 4.8), with Kris

[ldt1996]

@kriszyp You're right about the re-copy action: against undecodable records the per-connection latch loops exactly as you describe, and that's not tunable away. My instinct is to keep the instrument and drop the action: rework this to alert-only, a loud log plus a cluster_status shortfall delta, never a forced copy. That keeps one general tripwire for gap classes #545 can't see (it classifies decode failures specifically) at the cost of a log line for a false positive, and it matches your observability fallback. The per-range checksum sounds like the right long-term backstop and I'd file it as a follow-up either way. If that sounds right I'll push the rework here; if you'd rather keep the surface minimal, closing works too.

Lavinia, via Claude

[kriszyp]

Reviewed via Claude review-queue (full review: harper-pro-538-1aff8ce.md). The copyFromNodeId race from the prior review is fixed correctly this time.

One thing worth talking through: the delivery-count verification still does a serial per-table getRecordCount scan at COPY_COMPLETE on both legs of the copy. Kris's read: this is expensive, and it's not clear getRecordCount can really be precise/correct as a point-in-time check while concurrent writes are landing on the table — a count taken mid-stream-of-writes doesn't have a stable target to compare against, so even a correctly-implemented atomic count could still legitimately mismatch expected vs. actual depending on timing, independent of whether the copy itself succeeded. Worth deciding whether this verification is fundamentally the wrong tool for a live table (vs., say, tracking a monotonic delivered-count as part of the copy protocol itself, which doesn't need a scan and isn't racing concurrent writes), or whether there's a way to bound it to a stable snapshot.

Still no test covering this path.

[ldt1996]

Reworked in 0125ce5 along the lines you suggested: both sides now count the copy stream itself. The sender tallies records actually sent per table and ships that on COPY_COMPLETE; the receiver tallies what arrived intact, so decode drops are exactly what fail to count. The comparison is exact, which retires the estimate concerns, the serial count scans and their COPY_COMPLETE delay, and the concurrent-write race in one move. On a deficit it logs an error and stashes the delta on the connection; no re-copy and no cursor or seq surgery, which also moots the multi-source seq-reset concern. computeCopyDeliveryShortfall is a pure export with unit tests.

Fresh motivation from this week's validation runs: after the harper#1696 fixes converged everything else, we still found four rows across two tables silently missing behind fully valid cursors, invisible to every current signal. This tripwire covers the in-flight class; for at-rest completeness I still think the per-range checksum is the right follow-up and I'd file it as its own issue.

Lavinia, via Claude