Bank bulk-copy progress across blob-gap reconnects and repair missing blob files on copy re-delivery

[kriszyp]

Fixes “5.2.2 blob-gap wedge is bounded but banks zero copy progress”. Companion core change: “Repair dangling blob references when a base copy re-delivers the record; add replication.blobGapReconnectMs”.

Bulk-copy progress now banks through transient blob failures, and exact-identity re-deliveries can repair a dangling single-blob reference in place. Conflicts with current main are resolved, and the core submodule points to the current head of #2177 (c3d9c7b8). “fix(replication): await cold getEntry lookups so in-place blob repair fires on resumed copies” is no longer required for its primary cold-cache fix because this branch already awaits that read; merging it wholesale would also reverse intentional repair and uncertainty policies here. Its separate leading-duplicate promise/presence refinement can be considered as a focused follow-up.

For the human reviewer

1. Filesystem uncertainty currently selects a repair target and holds the delivery if repair declines, rather than treating the state as unknown and allowing fresh-save/backfill. This favors reference integrity but can retry indefinitely under persistent file-descriptor or I/O pressure; the choice is a small, reversible classifier change.
2. The leading-duplicate fast skip uses file presence while the repair path checks header completeness. Sharing the fuller classifier closes a PENDING-stub edge case at the cost of another probe on this hot path.
3. A drained barrier with newly banked progress reconnects immediately rather than waiting for blobGapReconnectMs. This minimizes re-streamed bytes but can turn frequent faults into frequent flushes and reconnects; adding a pacing floor is reversible.
4. The failure barrier remains latched for the connection lifetime, including a same-socket COPY_START. Resetting it per pass would let a restarted walk bank again, but would diverge from the existing connection-scoped hasBlobGap behavior.
5. The repair window closes after eight consecutive stored-entry misses and does not reopen before another COPY_START. This bounds cold lookups heuristically; an explicit duplicate-span signal would be more reliable but crosses the Pro/core boundary.
6. Pro reimplements core's exact duplicate tie rule (version and source node). Exposing core's actual decision would prevent silent drift but expands the companion API.
7. The repair occurs synchronously in the receive path rather than relying only on proactive blob backfill. This closes the dangling-reference window but adds storage and filesystem work to replication apply.
8. In-place repair is restricted to rows with one stored file-backed blob. Multi-blob rows remain backfill territory because positional pairing is not proven safe.
9. replication.blobGapReconnectMs is a separate operator-visible setting rather than being derived from blobTimeout. That makes gap recovery independently tunable, but adds configuration surface.

Verification

• npx mocha --require unitTests/unitTestSetup.cjs unitTests/replication/collectBlobRepairTargets.test.mjs unitTests/replication/copyCursorWatermark.test.mjs: 26 passing.
• npm run test:unit: 744 passing; one unrelated existing key-custody mode failure (0600 actual vs. 0644 expected).
• npm run lint:required: passed.
• npm run test:integration -- integrationTests/cluster/copyGapCursorBanking.test.mjs: passed with 5 injected faults, 0 watchdog fires, 3 banked reconnects, resume keys [13,17], 40/40 readable records, and 2 in-place repairs.
• npm run build: emitted the build and retained the existing unrelated Numeric, WebSocket _socket, and error-property TypeScript errors.
• npm run test:integration:all: analytics and the first clone-node test passed, then the run could not acquire shared loopback slots at 127.0.0.2 and 127.0.0.7; it was stopped after repeated contention. The focused regression used available addresses and cleaned up successfully.

Review coverage

Authored by GPT-5 Codex. Exact-head cross-model review: claude-opus-5 passed; cursor-grok-4.5-high passed and contributed one adjudicated edge case; Harper domain adjudication with claude-opus-5 passed; Gemini returned no output; Cursor Composer was pruned by the automatic review policy. Review receipt at 6195fdf74e59.

_{Human-Review-Need: 4 @ 696a4a9}

[gemini-code-assist]

Code Review

This pull request introduces position-indexed durability watermarks for bulk-copy resume cursors via a new CopyCursorWatermark class, allowing replication to resume from the last successful walk position before a transient blob fault rather than restarting from scratch. It also adds configuration for blobGapReconnectMs to tune gap cycles separately from blob timeouts, along with comprehensive unit and integration tests. Feedback on the implementation highlights a potential performance bottleneck in CopyCursorWatermark.stageCursor where a loop checking for unsettled tags could be optimized from O(frameIndex - tail.index) to O(unsettled tags) by traversing the sorted doubly-linked list instead.

[kriszyp]

Docs companion for the new config keys: documentation#628 — Document replication.blobGapReconnectMs and bulk-copy cursor flush options. — Claude (Fable)

[claude]

Reviewed; no blockers found.

The prior finding (non-ENOENT open failures returning true in blobFileMissingOrIncompleteAsync, selecting blobs for repair and latching the barrier on lock pressure) is fixed in commit 696a4a9 — the open catch now returns undefined for non-ENOENT errors, and the outer catch does the same. Both Codex suggestions (O(unsettled-tags) separator scan in stageCursor, repairWindowMissRunByNode.clear() on COPY_START) are incorporated.

[harper-joseph]

Read the watermark class and the wiring. The things I'd flagged as the risky parts of this shape are handled: pass/anchor captured at decode time (with passAtPersist re-validated at every async edge) closes the same-socket COPY_START staging race; the persist-failure path still holds the cursor via pendingCopyCursor ??= cursor; settleBlob runs before flushDurableCopyCursor in the blob .finally; unrecoverable-source blobs still settle un-gapped so #403's advance-past survives; and non-copy frames pay nothing because copyFrameIndex is undefined. The stageCursor tail-replacement giving |staged| ≤ unsettled tags + 1 is a nice bound.

Three things from measuring the live cluster tonight.

1. Field evidence for review note 4 — and it says the pair must ship together

Note 4 accepts "a zero-progress gap cycle is still unbounded churn if the record just past the banked cursor never heals at the source." On the kohls cluster that is not hypothetical, and I can now say how big it is. Latching faults on v3t sourced from yc0, 60-minute window:

92 total  →  23 distinct blobIds  →  exactly 4 occurrences each

Four recurrences per blob per hour is one per 15 minutes — the 900s watchdog cadence. These are the same 23 blobs failing every cycle, i.e. a fixed population of permanently-damaged references, not fresh transient contention. (I had assumed the opposite in the issue and was wrong; this measurement is what corrected me.)

The prediction that follows: #701 alone would advance each link's resume key to just before its first permanently-damaged blob and then stall there — banking real progress once, then repeating a cheap seek-resume cycle indefinitely. That is exactly note 4's failure mode, and it is guaranteed on this cluster rather than merely possible. It's also a strong argument for the sequencing you chose — harper#2177 is what makes the barrier position move on the next cycle rather than re-latch at the same key. Worth stating in the PR body that the pair is required, not merely convenient, for the field case.

Corollary for the repair's cost note (harper#2177 note 5): the damaged population per source link is small and finite — ~23 here — so the probe-per-tie cost lands on a bounded set, not on every re-delivered row indefinitely.

2. blobGapReconnectMs ordering caveat, worth putting in the rollout note

Note 5 says kohls needs the knob because their heal cadence was 15 min. Given the above, I'd add a deployment ordering warning: shortening this knob before harper#2177 is on the source nodes makes things worse, not better. With a fixed damaged set, a 60s cycle doesn't heal 15x faster — it produces 15x more forced reconnects, and each reconnect aborts in-flight receives, which mints a fresh PENDING stub per in-flight blob (abortInFlightBlobsOnClose sets replicationConnectionClosed but not sourceBlobUnavailable, so core's stub branch takes it). That's the amplification loop feeding itself faster.

Post-#2177, with the damaged set draining, the shorter cycle does what it says. So: ship the pair, confirm the fault population is falling, then shorten. Since the knob is a permanent public config key, this ordering is probably worth a line in the config docs too, not just here.

3. Question: is the live path's hasBlobGap latch in scope, or a follow-up?

This PR replaces the per-connection clamp on the copy path. As far as I can see lastDurableSequenceId still advances only under outstandingBlobsToFinish.length === 0 && !hasBlobGap, so on the live path a single transient blob fault still pins the persisted resume cursor for the life of the connection.

That matters here because it's plausibly the upstream cause of the base copies this PR is making resumable: #683's first field trace was a cursor 4 days stale against a 3-day retention horizon, which forced the bounded base-copy resync in the first place. If the live cursor can still be pinned indefinitely by one transient blob fault, that path stays open.

The watermark class looks directly reusable for it — position-indexed today, but the live analog is the same rule keyed by committedSequence instead of frame index. Not asking you to widen this PR; just want to know whether it's a known follow-up or something to file.

Minor

beginPass() clears #staged but deliberately retains tags and the barrier. Since #frame never resets, an unsettled tag from a prior pass pins limit for the whole connection — correct and intended, and bounded in practice because blobsTimer eventually reaps a hung stream into a gapped settle. Might be worth one line in the class comment saying that's what bounds it, since the comment currently argues the memory bound but not the liveness one.

[harper-joseph]

Follow-up with disk-level verification, and it refines one claim in the fix-pair comment on #699 ("#3's fault supply should mostly drain on its own once copies converge and stubs get repaired"). That drain has a topology hole, and this cluster sits exactly in it.

The stubs are verified persistent, not transient

Pulled the recurring fileIds' paths from the forwarded error text and read them on yc0's disk:

blobs/page_cache/1/f5e/ee6:  size=152  header=00fe…  mtime 2026-08-13 23:44:39
blobs/page_cache/1/f5e/ee1:  size=152  header=00fe…  mtime 2026-08-13 23:44:39

PENDING (0xfe) stubs, ~23 hours old, minted before the 5.2.2 upgrade, still referenced by live records (the fault lines name the record URLs). #481's "the re-stream overwrites this stub" is confirmed dead on disk, exactly as harper#2177's analysis says — re-streams mint fresh fileIds, so the stub file is never rewritten and the record's reference never moves.

Both fault populations are fixed sets recurring at the watchdog cadence (every ~15 min):

source	latching faults 60 min	distinct blobIds	recurrence
yc0 → v3t	92	23	exactly 4 each
e9v → v3t	849	217	4 each at the top

Why the pair drains e9v's 217 but not yc0's 23

harper#2177's repair fires on copy-apply identity ties — i.e. on nodes that receive a base copy. And usefully, the healing doesn't require the copy to complete: the walk applies (and repairs) re-delivered records past the barrier even while the cursor is clamped, so one post-fix walk over a damaged record heals it.

• e9v, cd5, v3t are copy receivers — their inbound walks re-deliver their stub-referencing records, #2177 heals the references, their outbound 503 supply collapses. The mesh cascade works.
• yc0 receives no base copy (0 copy starting from in its logs; it has never had a copyCursor row — it is the root source of this topology). Nothing ever re-delivers its 23 records: live replication only re-streams from resume windows, not 23h back; repair_blob_data is repair_blob_data always reports noConnection on multi-worker instances — it runs on the main thread, connections live on http workers #684-broken; the re-stream never overwrites the stub. yc0's 23 damaged references have no healing path in the pair.

Consequence for the two links copying from yc0 (v3t←yc0, cd5←yc0): with banking, each converges up to the first of the 23 damaged keys and then latches there every cycle — the barrier re-latches at the same frame each reconnect, so the resume key freezes permanently at that key. Cheap cycles now (seek-resume, not full re-walk), but the links never exit copy mode, and these links need their copies to complete (their seq cursors are 5.5–7.2 days stale against 3-day retention — a copy that never finishes means they never leave copy mode at all). Note 4's accepted risk isn't an edge case on this cluster; it's the guaranteed steady state for the root source's links.

Ways to close the hole (in rough order of leverage)

1. Source-side reclassification with an age/registry gate: when a copy read hits the source's own stub, isBlobReceiveInFlight is false, and the stub is older than some horizon, forward permanent (404/500) instead of 503. The receiver then advances past it (Blob replication wedges permanently when source blob is gone (ENOENT) on an expiration cache table — held resume cursor never recovers #403 semantics) with divergence debt for Proactive blob backfill: repair already-committed records whose blobs are missing/corrupt (no recovery once the resume cursor advances past them) #388, instead of latching forever. Small, source-only, and consistent with the existing "unrecoverable at source" contract — the data genuinely is unrecoverable from this source.
2. The in-band per-blob re-request follow-up (5.2.2 blob-gap wedge is bounded but banks zero copy progress — the copy cursor never persists during a copy body, and the watchdog's reconnect mints the next cycle's faults #699 direction 2), if it rotates peers — the other three nodes likely hold healthy copies of these 23 blobs, so a third-party fetch heals the receiver even though the source can't serve them.
3. Operational, today, no code: the damaged set is small and enumerable from the fault lines (23 URLs on yc0, …prd-6788002…|mobile etc.). Re-rendering (or deleting, letting the cache re-fill) those records on yc0 replaces the references with fresh blobs and zeroes the root source's fault supply — that plus this pair should let all 6 stuck pairs converge without waiting for a further code change.

One prediction this makes falsifiable post-deploy: if the pair lands and the yc0-sourced links still cycle, the resume key in the new "Resuming interrupted copy … after key K" log line will be frozen at the same K every cycle, and K will be the record just before the first of the 23. That's the signature to look for before concluding anything else is wrong.

[kriszyp]

Design rework since the original draft (per Kris): the dangling-blob repair moved out of core's write path entirely — instead of reclassifying the copy-apply duplicate and rewriting the record, the receiver now streams the re-delivered bytes into the stored record's existing fileId (core repairBlobFile), so the tie-skip stays untouched and the reference heals in place. Two behavioral upgrades landed with it: barrier-drained early reconnect (a cycle that banks progress reconnects immediately after its final cursor persist instead of waiting out the watchdog — gap-heal cadence now tracks walk time; the e2e run shows 0 watchdog fires / 3 banked reconnects) and the leading-duplicate resume tail is covered as well as copy frames. PR description is updated; earlier review context predating the rework is superseded. — Claude (Fable)

[harper-joseph]

Independent verification of head d4b1b75c (+ core 612de4ab), built and run locally (macOS, loopback cluster). I ran the PR's own e2e plus my two #699 reproductions — tests written from the failure before this PR existed, so they're an oracle the PR wasn't tuned against. One result needs attention before merge.

Banking: confirmed fixed, independently

My sustained-fault repro (copyCursorSustainedBlobFaults, fails on main by design) against this branch:

5.2.2:   cursorAdvanced=0   copyResumes=0    watchdog-paced full re-copies
branch:  cursorAdvanced=25  copyResumes=26   watchdogFires=1, converged 1000/1000

The PR's own banking half also behaved: resumeKeys=[13,17,19,26] monotonic, watchdogFires=0, bankedReconnects=4. Orderings verified in code: the banked persist lands (dbisDB.put inside the flush .then) before the early reconnect fires, and writeBlobWithStream's lockHeld contract releases the caller's lock on its completion paths. D1 is dead.

Bonus: in my fault-supply characterization, the receiver now logs the source's actual Blob pending replication error (was the generic in-flight arm on 5.2.2) — classification provenance is now readable from receiver logs.

Blocking: the in-place repair fired 0 times in a clean run of this PR's own e2e

copyGapCursorBanking failed in my environment at this head:

injected=6 watchdogFires=0 bankedReconnects=4 resumeKeys=[13,17,19,26]
B=40/40  missingPayloadIds=[14,30]  inPlaceRepairs=0

Banking worked; the repair never fired; two records ended permanently blob-less — the exact field class the pair exists to fix. The receiver's log shows all 4 banked resumes re-delivering the damaged span and zero Repaired blob file in place lines.

The aperiodic injector makes schedules non-deterministic (your run: bankedReconnects=3, resumeKeys=[13,17]), so the divergence itself is expected — but the invariant (missingPayloadIds=[], inPlaceRepairs>0) is what failed, and there is no way to tell which gate declined: every decline path in collectBlobRepairTargets and repairBlobFile is a silent return null/undefined. That's the same observability shape #699 complained about, now inside the fix — a field non-fire is indistinguishable from "no damage," and my run demonstrates non-fires happen.

Candidates worth instrumenting first (all silent today):

1. getEntry returning a Promise on a block-cache miss → typeof existing.then === 'function' → null. Timing/cache-sensitive — would explain author-machine pass vs my-machine fail, and would also under-fire in the field on large tables where re-delivered records have aged out of cache.
2. The tie condition (existing.nodeId ?? 0) !== sourceNodeId vs what copy-apply snapshot rows actually store for nodeId.
3. repairWindowMissRun latching the window off early.

Ask: a per-decline-class debug counter (or one line per class) before merge, then re-run the e2e wherever it failed. If the cause is (1), an async getEntry await inside the already-async repair probe path seems safe — it's off the apply loop.

Early reconnect has no pacing floor under a moving fault supply

Empirical, same harness both sides: 5.2.2 paced 4 cycles at the watchdog interval; this branch ran 27 copy starts in ~76 s — back-to-back banked cycles — re-streaming 1115 blobs in a 17 s quiet window. It converges (each cycle banks, which is the design), but on a busy mesh with continuous transient contention this is continuous re-stream churn with no floor, and each early reconnect aborts in-flight receives, minting receiver-side PENDING stubs — benign when the repair heals them on the next pass, except per the section above the repair can silently not fire. A modest floor (minimum bank advance per cycle, or a minimum interval between banked reconnects) would bound it. Kohls' fixed damaged sets bank nothing per cycle → still watchdog-paced → unaffected by this.

Note 1's per-frame granularity note also composes with this: "a fault supply dense enough to hit every message banks nothing" → watchdog-paced (safe); it's the middle regime — faults every few messages — that cycles continuously.

Scope observation (release-risk, not correctness)

This PR now carries a second, separable change set: clone-attempt markers, the cloneCopyComplete dbis marker, RECEIVED_VERSION_POSITION reset on clone COPY_START, the leader-version probe gating the system-socket requirement, sync-target filtering by replication.databases/shard, and isLeader: true on the clone's add_node — behavior changes for every HDB_LEADER_URL clone, interacting with #657 which landed two days ago. I ran addNodeFullCopy and cloneReadinessKeySet against the branch — both pass — but none of the seven review rounds were framed around clone regressions. If the goal is not cutting a follow-up release, either split that set or give it its own review framing; two green tests are necessary, not sufficient.

Minor

• The non-copyApply (LMDB/system) synchronous persist path never triggers the barrier-drained early reconnect — those copies still wait out the full watchdog. Parity nit, non-default engine.
• +1 to the inline stampPendingBestEffort blocker — mechanics confirmed on branch code (settle handlers run after writeBlobWithStream's completion paths released the :blob lock); the re-tryLock-and-skip fix is the cheap correct one.

[harper-joseph]

Drafted, validated fixes for the repair non-fire (patches below, apply with git apply). Root cause was two compounding defects, both on the branch head:

1. collectBlobRepairTargets treats a Promise-returning getEntry (block-cache miss) as absence (typeof existing.then === 'function' → null).
2. The call site's existsLocally probe does the same and counts the miss toward repairWindowMissRun — so 8 consecutive cold entries latch the repair window off for the rest of the connection.

On a resumed copy, the re-delivered span was applied by a previous connection, so cold entries are the norm in exactly the repair window. Warm cache (small table, same process, records applied moments ago) fires; cold cache silently never does. That's why it passed on the authoring machine and failed here — and it would under-fire in production on precisely the large tables that matter.

Fix: await the entry in both places; only a resolved absence counts toward the window latch (so the latch still turns the probe off once the walk passes the span); every decline increments a per-class counter, logged once per connection at retire (debug). One existing unit test had encoded the bug — decoderFor({ then: () => {} }) asserting null — replaced with: resolved entry ⇒ repairs, resolved null ⇒ absence, plus a decline-class counting test.

Validation (this branch, macOS, same harness as my earlier report):

	before	after
copyGapCursorBanking	✖ inPlaceRepairs=0, missingPayloadIds=[14,30] (361s)	✔ inPlaceRepairs=2, missingPayloadIds=[] (25s, twice)
collectBlobRepairTargets + copyCursorWatermark unit	22 (one encoding the bug)	25 passing

--- a/replication/replicationConnection.ts
+++ b/replication/replicationConnection.ts
@@ -1015,20 +1015,33 @@
  * uses the normal fresh-save path. A tie guarantees content identity, so bytes-for-bytes overwrite
  * of the existing file is sound; positional pairing is sound for the same reason (identical
  * structure). Never throws: any read/decode surprise returns null (normal path, no repair).
+ *
+ * `getEntry` may return a Promise (block-cache miss) — AWAITED here, never treated as absence: on a
+ * resumed copy the re-delivered span was applied by a PREVIOUS connection, so cold entries are the
+ * norm in exactly the window this function exists for. Callers run on the serialized message chain
+ * (before decode), where awaiting is safe. Every decline increments its class in `declines` so a
+ * field non-fire is attributable — a silent null is indistinguishable from "no damage" (#699's
+ * observability lesson, applied to the fix itself).
  */
 export async function collectBlobRepairTargets(
 	tableDecoder: any,
 	id: any,
 	incomingVersion: number,
 	sourceNodeId: number | undefined,
-	blobFileDamaged: (blob: any) => Promise<boolean | undefined> | boolean | undefined
+	blobFileDamaged: (blob: any) => Promise<boolean | undefined> | boolean | undefined,
+	declines?: Record<string, number>
 ): Promise<any[] | null> {
+	const decline = (reason: string) => {
+		if (declines) declines[reason] = (declines[reason] ?? 0) + 1;
+		return null;
+	};
 	try {
-		if (sourceNodeId === undefined) return null;
-		const existing = tableDecoder?.getEntry?.(id);
-		if (!existing || typeof existing.then === 'function') return null;
-		if (existing.version !== incomingVersion) return null;
-		if ((existing.nodeId ?? 0) !== sourceNodeId) return null;
+		if (sourceNodeId === undefined) return decline('no-source-node');
+		let existing = tableDecoder?.getEntry?.(id);
+		if (existing && typeof existing.then === 'function') existing = await existing;
+		if (!existing) return decline('no-stored-entry');
+		if (existing.version !== incomingVersion) return decline('version-mismatch');
+		if ((existing.nodeId ?? 0) !== sourceNodeId) return decline('node-mismatch');
 		const blobs: any[] = [];
 		findBlobsInObject(existing.value, (blob: any) => {
 			blobs.push(blob);
@@ -1038,10 +1051,10 @@
-		if (blobs.length !== 1) return null;
-		return (await blobFileDamaged(blobs[0])) === true ? [blobs[0]] : null;
+		if (blobs.length !== 1) return decline(blobs.length === 0 ? 'no-file-blobs' : 'multi-blob');
+		return (await blobFileDamaged(blobs[0])) === true ? [blobs[0]] : decline('healthy');
 	} catch {
-		return null;
+		return decline('error');
 	}
 }

@@ call site (~:4961) @@
-					const existsLocally = (() => {
-						try {
-							const entry = tableDecoder?.getEntry?.(id);
-							return entry != null && typeof entry.then !== 'function';
-						} catch {
-							return false;
-						}
-					})();
-					if (existsLocally) {
+					// AWAIT a Promise-returning getEntry (block-cache miss) rather than treating it as
+					// absence: the re-delivered span was applied by a PREVIOUS connection, so its entries are
+					// cold in exactly this window — a sync-only check reads the whole span as missing, which
+					// both skips every repair AND latches the window off after 8 records (observed as
+					// inPlaceRepairs=0 with the damage intact). Awaiting is safe here: this runs on the
+					// serialized message chain, before decode. Only a RESOLVED absence counts toward the
+					// miss-run latch, so the latch still turns the probe off once the walk passes the span.
+					let storedEntry: any = null;
+					try {
+						storedEntry = tableDecoder?.getEntry?.(id);
+						if (storedEntry && typeof storedEntry.then === 'function') storedEntry = await storedEntry;
+					} catch {
+						storedEntry = null;
+					}
+					if (storedEntry != null) {
 						repairWindowMissRun = 0;
 						repairTargets = await collectBlobRepairTargets(
 							tableDecoder,
 							id,
 							auditRecord.version,
 							localSourceNodeId,
-							blobFileMissingOrIncompleteAsync
+							blobFileMissingOrIncompleteAsync,
+							repairDeclines
 						);
 					} else repairWindowMissRun++;

Plus: const repairDeclines: Record<string, number> = {} next to repairWindowMissRun, and one aggregate debug line in retireInstance ('blob repair declines for this connection', only when non-empty). Test-file diff available on request — the substance is described above.

Separately: a floor on the banked reconnect — measured, and it's a genuine trade, not a win

I prototyped the pacing floor I suggested earlier (min(15s, blobGapReconnectMs/2) between banked reconnects, stamped on the shared connection object) and measured it under a sustained moving fault supply (every-Nth-save injector, 1000 records):

	no floor	floor
copy starts in the run	27	7
watchdog fires	1	3
blobs re-received in a 17s quiet window	1115	2994
converged	✔	✔
copyGapCursorBanking	✔	✔ (floor=1.5s at its 3s knob)

The floor cuts reconnect/stub-minting churn 4x — but re-streamed bytes go up, because a floored cycle keeps walking the unbankable tail that your immediate reconnect deliberately truncates. Your design is byte-cheaper; the floor is socket/abort-cheaper. With the repair actually firing (above), the stub-minting half of my original concern is largely self-healing, which weakens the floor's case. So: the floor diff exists and passes everything, but I'd treat it as optional — your call whether reconnect-rate pathology on a busy mesh (~21k reconnects/link/day at 4s cycles) is worth the extra bytes. Happy to drop it.

One residual from my earlier report stands either way: my sustained-fault regression test still shows continuous cycling on this branch (7–27 copy starts where 5.2.2 had 4 watchdog-paced ones) — that's inherent to walk-time-paced healing under a fault supply that never dries, not a defect; I'll adapt that test's oracle once the floor decision settles.

[kriszyp]

I addressed the measured non-fire and the remaining parity/safety gaps:

• Both cold getEntry reads are awaited. A resolved miss alone advances the per-source repair-window latch, and decline classes are aggregated once at warning level when the connection retires.
• The integration oracle now reads every record’s referenced blob through the resource endpoint and verifies its length and hash. It requires both initially damaged records to repair in place; orphan bytes elsewhere on disk cannot satisfy it. The final pair of heads passed twice consecutively with 40/40 references readable and 2 repairs.
• A selected damaged target can no longer fall through to a fresh save when the repair lock, source size, or filesystem probe is uncertain. That delivery holds the cursor and retries.
• Core now holds the blob lock through the failed-repair PENDING marker and verifies the late-bound BLOB_CHUNK.size before finalizing the header.
• The immediate-reconnect path now covers Rocks copy-apply and LMDB/system cursor persistence.
• I kept the no-delay banked reconnect behavior based on your measurement: the floor reduced copy starts but increased re-streamed blobs from 1,115 to 2,994. The PR body calls this out as a socket/abort-vs-bytes tradeoff.
• The clone scope is superseded by the merged “Clone sync verification now detects an in-progress base copy on RocksDB instead of marking the node Available mid-copy”.

The exact-head external review retries are currently degraded by repeated Claude HTTP 529 overloads, so the PR stays draft and the review-need footer remains fail-closed at 4.

— GPT-5 Codex

[harper-joseph]

Warning

Superseded in two places — see the correction below. The "Scope" section at the end is wrong: this PR carries no clone-readiness changes, so there is nothing to split. And the uncertainty-policy finding, while a real mechanism, is self-limiting in measurement and is not a blocker. The rest stands, and the code findings are fixed by 56ac74f/2b16977.

Reviewed 6195fdf7. The core of this — position-indexed banking, in-place repair, per-source-node miss-run (better than the single counter I'd proposed in #720) — I'm satisfied with, and my measurements against the earlier head confirmed the banking half works. One finding on the uncertainty policy, which reviewer note 1 already half-identifies but I think understates.

The uncertainty policy can wedge a link on the same shape as #699

Note 1 says this "can retry indefinitely under persistent file-descriptor or I/O pressure." Tracing it, the loop is tighter than "retry indefinitely" suggests — it's self-latching, and the copy cursor stops for the whole connection:

1. blobFileMissingOrIncompleteAsync returns true on any open/stat/read failure — including EMFILE/ENFILE/EIO (:1095, :1110).
2. So the blob is selected as a repair target, and receiveBlobs takes the repair branch.
3. repairBlobFile re-checks under the lock. Under the same fd pressure its openSync also fails → blobFileMissingOrIncomplete returns undefined → !== true → declines with undefined.
4. finished is undefined → the delivery is failed on purpose: stream.destroy(error), Promise.reject, repair-start-declined (:5770-5774).
5. That rejection settles the blob gapped → barrier at this frame → and per note 4 the barrier is latched for the connection lifetime. Everything past this frame is unbankable until reconnect.
6. Reconnect resumes at the same key, re-delivers the same record, and if fd pressure persists, lands on step 1 again.

The part that concerns me is the interaction of notes 1 and 4. Individually each is a defensible call. Together, a transient environmental condition (fd exhaustion is usually transient and usually correlated across a worker) is converted into a per-connection barrier that only a reconnect clears — and the reconnect immediately re-enters it. That's the #699 shape with a different trigger: bounded per cycle, zero net progress across cycles.

Two things make it worse than the old behavior it replaces. Under 5.2.2 an unprobeable file wasn't specially handled, so the record just fresh-saved and the walk continued (dangling reference, but forward progress). And fd pressure is precisely what a blob-dense copy with blobConcurrency in flight plus a repair probe per duplicate tends to produce, so the trigger is correlated with the workload, not independent of it.

I don't think "hold" is wrong — I think "hold forever on a condition that isn't about this blob" is. Options, cheapest first:

• Distinguish "this file is damaged" from "I can't tell." ENOENT ⇒ true; EMFILE/ENFILE/EIO/EACCES ⇒ undefined, and let undefined take the pre-existing fresh-save path rather than the repair path. Reference integrity is preserved for the case you actually detect, and an environmental fault degrades to today's behavior instead of a barrier. (This is the not-probeable split from fix(replication): await cold getEntry lookups so in-place blob repair fires on resumed copies #720 — I'd understood the reversal as a policy preference, but I don't think the indefinite-hold consequence was the intended half of it.)
• Or keep the hold and bound it: after N consecutive repair-start-declined on the same fileId, fall through to fresh-save and record the dangling reference for backfill. Converts an unbounded wedge into bounded divergence, which is the trade Blob replication wedges permanently when source blob is gone (ENOENT) on an expiration cache table — held resume cursor never recovers #403/Incomplete (truncated) source blob is misclassified as transient → permanently wedges replication cursor (ENOENT is handled; 'Blob is incomplete' is not) #429 already make elsewhere.
• Or decouple note 4 for this case only: let a repair-start-declined barrier reset on the next COPY_START, so a restarted walk can bank again rather than re-latching at the same key.

Worth noting repair-start-declined is counted in repairDeclines, so if this does happen in the field it'll be visible — that's a real improvement over the class of thing it resembles.

Smaller

• Note 2 (leading-dup presence vs. header completeness) is worth closing before merge, not after. The fast skip uses file presence while the repair path checks header completeness, so a PENDING stub reads as "present" and the record gets skipped — meaning the record that most needs repair is the one that can bypass the repair path. The extra probe on the hot path is real, but the skip already reads the entry; this is the same file it will repair moments later.
• isSkippableLeadingDuplicate still returns false on a thenable entry (:3217, "a synchronous getEntry is expected here"). Same cold-cache argument this PR makes everywhere else — that function runs only in the resume tail, so the fast skip has likely never engaged there. Benign direction; two lines. (Carried over from fix(replication): await cold getEntry lookups so in-place blob repair fires on resumed copies #720, which I closed.)
• test:integration:all didn't complete (loopback contention — I hit the same thing; sudo harper-integration-test-setup-loopback re-adds the aliases, they don't survive reboot). Given this branch also carries the clone-path changes, I'd want that suite green before merge rather than the focused test alone.

Scope, restating once

This is 1,316 additions across banking + repair + clone-readiness + config. The banking and repair halves I've now reviewed closely and measured. The clone half — isLeader: true on every clone's add_node, attempt markers, sync-target filtering, the leader-version probe — is unrelated to #699, changes behavior for every HDB_LEADER_URL clone, and interacts with #657. It's also the part no review round has been framed around. If kohls needs the #699 fix soon and this is 5.3-sized as a whole, splitting the clone set is the lever that makes the rest shippable.

Happy to re-run my #699 reproductions against this head if that's useful before merge — they're independent of this implementation and caught the repair non-fire last time. — Claude (Fable), reviewing for Joseph

[harper-joseph]

Correction to my review above, plus measurements against 2b16977. Two things I got wrong.

1. Scope — I was wrong; disregard that section entirely

I said this PR carries clone-readiness changes (isLeader: true on every clone's add_node, attempt markers, sync-target filtering, the leader-version probe) and suggested splitting them out to make the rest shippable. That's not what's here. The file list is nine files — replication, tests, fixtures, the core pointer — with a single incidental cloneCopyComplete read. There is no clone set to split. Apologies for the noise; if you'd started carving that out it would have been wasted work.

2. The uncertainty-policy wedge — real mechanism, overstated severity

I called it self-latching and treated it as the thing to resolve before merge. I've now measured it instead of reasoning about it.

On 6195fdf7 it fired 4x, all on fileId 2a, each one producing Blob save failed for 2a → gap latch, across 4 successive passes. So the mechanism is real and it is not hypothetical. But 2a was then successfully repaired in place later in the same run. The trigger was lock contention on the repair target, which clears on its own — not fd exhaustion, which wouldn't. On 2b16977 it drops to 1 occurrence.

With 2b16977 making the duplicate probe loop sequential (bounding concurrent fds, which was the pressure vector my concern rested on), I no longer consider this a blocker.

Your two commits fix all three findings

56ac74f covers note 2 (fsPromises.access presence → blobFileMissingOrIncompleteAsync(...) !== false completeness, so a PENDING stub no longer reads as skippable — and !== false correctly treats unprobeable as not-skippable) and the :3217 thenable. 2b16977 bounds the probes. Measured effect, same test, same seed:

	6195fdf7	2b16977
repairDeferred	4	1
in-flight blobs aborted by close	7	5
blobs re-received in quiet window	2008	1138
cursor advances / resumes	21 / 23	20 / 21

What the fix does — 1000 records, a fault injected every 7th blob save

	5.2.2	2b16977
durable cursor advances	0	20
advances after first fault	0	20
copy resumes	0	21
distinct files repaired in place	—	146

D1 is dead. Resume keys climb monotonically — 4→7→10→16→21→27→…→81 — rather than sitting frozen.

One aside worth recording: Resuming interrupted copy of database (:6147) is subscriber-side. I'd previously assumed it was sender-side because it read 0 on 5.2.2; it read 0 because D1 meant there was never a cursor to resume from. It's a genuine progress signal, not noise.

What it doesn't fix — relevant to the kohls rollout, not to merging

A source-held PENDING stub is never healed by this pair: receiver-side repair rewrites the receiver's file and cannot touch the source's stub. I planted 3 byte-identical PENDING stubs on the source at 400 records and ran both gap positions:

• head of walk (positions 1, 10, 100): cursorAdvanced=0, bankedReconnects=0, resumes=0
• tail of walk (positions 398, 399, 400): cursorAdvanced=2, bankedReconnects=0, resumes=0

In both, the cursor freezes, the copy never completes, and it falls back to the blobGapReconnectMs watchdog. The tail timeline explains the zero reconnects: copy body ran 125 ms, the two persists landed 37/66 ms later, and the blob failures didn't arrive until +3.7 s — the source retries a 503 for ~3.75 s before forwarding. So the gap arrives after the walk has already drained its staged cursors; nothing is left to persist, so the banking-gated drained-barrier reconnect never fires. That gate is documented at :2661 and it's a reasonable anti-livelock guard — just worth knowing it means the worst case paces at the watchdog interval.

Two things I want to be clear are not criticisms of this PR:

1. The watermark did the safe thing here. Those blobs were still unsettled at persist time, so takeDurable() capped the cursor below them — the durable cursor stops short of the damaged records rather than claiming delivery of them.
2. Banking collapses the per-cycle cost from the whole table to just the tail past the last good record. On a 1.6M-row / 376GB table that's the difference between re-copying 376GB every 15 minutes and re-copying a sliver. That's a large win even without convergence.

The operational consequence is only that the damaged source blobs still have to be cleaned as part of the rollout — the fix pair won't heal them, and any peer copying from a node holding them will stay in copy mode indefinitely. Worth a follow-up issue for source-side reclassification or peer-rotating re-request; happy to file it.

Verdict

Approving. Beyond the above I checked: no wire-format change and no persisted-cursor shape change (same four fields, source vars renamed to per-frame), so a rolling upgrade and a rollback are both safe; the typecheck adds zero new errors (the 30 present are pre-existing on main, and copyCursorWatermark.ts is clean); lint is error-free; and CI is green across all six cluster shards. My earlier ask for the full cluster suite was predicated on the clone changes I was wrong about — CI covers it.

[harper-joseph]

Triage of the three outstanding bot findings, verified against 2b16977. One is live, two are already fixed. This also revises the verdict in my comment above.

Live: the blobFileMissingOrIncompleteAsync open-failure policy (claude, blocker) — I agree

I'd raised this shape in my first review and then downgraded it in my comment above, on the grounds that the only occurrences I could actually produce were benign. That downgrade was wrong, and the bot has the fact I was missing.

I verified the history it cites. 03341fd ("Hold uncertain duplicate blob repairs for retry") changed:

return (error as { code?: string })?.code === 'ENOENT' ? true : false;   // before
return true;                                                             // after

So this is not an inherent tension in the design — it's a widening introduced inside this PR series, and it removes behaviour 5.2.2 had. I re-walked every link of the chain in the current source and they all hold: non-ENOENT open failure → true → selected as repair target → repairBlobFile re-check fails under the same pressure → repair-start-declined → stream.destroy + reject → the else branch at :5844 → hasBlobGap = true, copyBlobGapped = true → barrier latched for the connection lifetime → reconnect re-delivers the same record.

What makes it worth fixing rather than deferring is the correlation, not the probability. This PR adds a probe open plus a potential repair open per duplicate frame, and the workload that exercises it hardest — a blob-dense copy — is the one most able to exhaust descriptors. If it does fire, the symptom is indistinguishable from #699 itself, so it would read in the field as "the fix didn't work."

My own measurements are consistent with the bot's severity being possible but do not test it: I saw 4 declines on 6195fdf7 and 1 on 2b16977, all on one fileId, and all traceable to repair-target lock contention, which cleared on its own (that file was later repaired in place in the same run). Lock contention self-heals; sustained EMFILE/EIO would not. I could not reproduce fd exhaustion, so I can't bound this empirically either way.

One addition to the suggested fix. Returning undefined behaves correctly, but the caller at :1075 is now

return (await blobFileDamaged(blobs[0])) === true ? [blobs[0]] : decline('healthy');

so undefined is recorded as healthy. The decline classes on this head are error, healthy, no-source-node, no-stored-entry, node-mismatch, version-mismatch — the not-probeable class didn't survive absorption. Worth restoring alongside the ENOENT split, or a field non-fire gets attributed to "the file was healthy" when it means "the probe couldn't answer." That distinction is the whole reason the decline counters exist.

Already fixed: repairWindowMissRun reset on COPY_START (claude, non-blocking)

This one is stale. The reset is already there at :3621, inside case COPY_START, immediately after sawCopyRestart:

const copyWasAlreadyActive = inCopyMode;
if (copyWasAlreadyActive) sawCopyRestart = true;
repairWindowMissRunByNode.clear();

The suggested diff also names repairWindowMissRun, which no longer exists — it's repairWindowMissRunByNode, a per-source-node Map (correctly, since one connection can carry several source nodes). No action needed.

Already fixed: the stageCursor separator scan (gemini, medium)

Also stale. The comment describes an O(frameIndex - tail.index) index walk, but stageCursor already does exactly the linked-list traversal the suggestion proposes:

let nextTag = this.#tagHead;
while (nextTag && nextTag.index <= tail.index) nextTag = nextTag.next;
const separated = nextTag !== null && nextTag.index <= frameIndex;

That landed in f787e82. The only remaining for loop in the file is #mergeStagedBetween's, which iterates #staged — bounded by the documented |staged| <= unsettled tags + 1 invariant, not by walk distance. No action needed.

Revised verdict

I said "approving" above; I'm withdrawing that pending the ENOENT split. Everything else in that comment stands — the measurements, the D1 result, the source-held-stub caveat for the rollout, and the retraction of my scope criticism. It's a two-line change on a path that already has the semantics it needs, so I don't think it costs much against how long kohls has been waiting; I'd rather not ship a known regression of the exact failure class this PR fixes. Happy to re-run the reproductions once it's in.

[harper-joseph]

Warning

Retracted — see the consolidated position below. The ENOENT split is a preference, not a blocker: the fd-pressure premise behind it does not hold (this path holds ~1 concurrent descriptor per connection, and 2b16977 removed the only concurrency amplifier). The not-probeable labelling ask stands. Everything else below is unchanged.

@kriszyp — short version of the two long comments above, so the ask is in one place.

One blocking item. Revert the open-failure widening from 03341fd in blobFileMissingOrIncompleteAsync:

return (openErr as { code?: string })?.code === 'ENOENT' ? true : undefined;

and restore a not-probeable decline class alongside it, because the caller at :1075 currently folds undefined into decline('healthy') — which would attribute a field non-fire to "the file was healthy" when it means "the probe couldn't answer."

The reason we'd rather not defer it: it's a regression against 5.2.2 introduced inside this series, and its failure mode is a latched barrier with reconnect cycling and no cursor progress — i.e. indistinguishable from #699 itself, on the workload most likely to trigger it. Two lines, on a path that already has the semantics.

Nothing else is blocking. The two other outstanding bot findings are both already fixed in your branch (details in the comment above) — no action needed on either. CI is green across all six cluster shards, and I re-ran my #699 reproductions on 2b16977: D1 is gone (durable cursor advances 0 → 20, resumes 0 → 21, resume keys climbing monotonically), 146 distinct files repaired in place, and your last two commits improved every axis I measure.

One item for the rollout plan rather than this PR: a source-held PENDING stub is never healed by this pair — receiver-side repair rewrites the receiver's file, not the source's. I measured both a head-of-walk and a tail-of-walk gap; in both the cursor freezes and the copy never completes, falling back to the blobGapReconnectMs watchdog. Banking still collapses the per-cycle cost from the whole table to the tail past the last good record, which is a large win — but the damaged source blobs have to be cleaned separately or peers copying from that node stay in copy mode indefinitely. Happy to file that as a follow-up issue.

I'll re-run the reproductions once the ENOENT change is in.

[harper-joseph]

@kriszyp — retracting the "one blocking item" framing from my comment above. I checked the reachability arithmetic I should have checked before calling it blocking, and it doesn't support the severity. Sorry for the churn; this is the position I'll hold.

The fd-pressure premise doesn't hold up

The blocker argument (mine and the bot's) rests on "this PR adds a probe open plus a repair open per duplicate frame, so the trigger is correlated with the workload." That conflates open rate with concurrent fd count, and exhaustion is about the latter:

• blobFileMissingOrIncompleteAsync opens and closes inside a finally — the fd is held for one stat + one 8-byte read.
• 2b16977 made the probe loop sequential (for (const blob of blobs) if ((await …) !== false)), so it's one blob at a time, not Promise.all over all of them.
• The apply loop is serialized per connection, so it's ~1 concurrent probe fd per link.
• Core's repair path closes in finally too.

That's on the order of one extra descriptor per connection — three or so on a 4-node cluster. Not an exhaustion driver. EMFILE would have to originate elsewhere in the process, and at that point the node has larger problems than this branch. Notably 2b16977 — which the bot acknowledged as landed — is itself what removed the only real concurrency amplifier here.

Which makes your tradeoff defensible, and possibly the better one

Both policies fail only under that same unlikely trigger, so the question is which consequence you'd rather have:

• Yours (uncertainty → hold): a wedge. Loud, bounded, clears when the pressure clears.
• The suggested change (uncertainty → fresh-save): the cursor advances past a blob that may genuinely be damaged, while the tie-skipped record keeps referencing it. Because the cursor advanced, nothing re-delivers that key — so it's closer to silent divergence needing Proactive blob backfill: repair already-committed records whose blobs are missing/corrupt (no recovery once the resume cursor advances past them) #388 backfill than to a deferred repair.

That's exactly what your commit message on 03341fd said you were buying, and I don't think it was the wrong call. Treat the ENOENT split as a preference, not a defect — your call as author, and I'm fine either way.

The one thing I'd still ask for

Restore a not-probeable decline class. The caller is currently

return (await blobFileDamaged(blobs[0])) === true ? [blobs[0]] : decline('healthy');

so an unanswerable probe is counted as healthy. That one has no tradeoff on either side of the above — it's the difference between a field non-fire being diagnosable and it being attributed to "the file was fine." Cheap, and it's the counter you'd actually want if this ever does misbehave in prod.

Verdict

Back to approving. To summarize the whole review in one place, since I've moved around:

• Fixed and verified: the leading-dup thenable, presence-vs-completeness, probe bounding — all three confirmed on 2b16977.
• Measured: D1 is gone (durable cursor advances 0 → 20, resumes 0 → 21, resume keys climbing monotonically); 146 distinct files repaired in place; your last two commits improved every axis (repairDeferred 4 → 1, close-aborted blobs 7 → 5, blobs re-received 2008 → 1138).
• Withdrawn: my scope criticism (there are no clone changes here), and the blocking framing on the uncertainty policy.
• No action needed: the other two bot findings are already fixed in your branch.
• For the rollout, not this PR: source-held PENDING stubs aren't healed by this pair, so those need cleaning separately or peers copying from that node stay in copy mode indefinitely. I'll file that as a follow-up.