fix(replication): await cold getEntry lookups so in-place blob repair fires on resumed copies

[harper-joseph]

Into #701's branch. Fixes the repair non-fire reported in this comment, with the patch from this one — opened as a PR per review flow.

The defect (two compounding, both silent)

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

A resumed copy's re-delivered span was applied by a previous connection, so cold entries are the norm in exactly the repair window. Result: the repair silently never fires on a cold cache and the window turns itself off — warm-cache runs (small table, same process) pass, which is why the e2e was green at authoring time and failed elsewhere.

The fix

• Await the entry in both places (safe: this path runs on the serialized message chain, before decode).
• Only a resolved absence counts toward the window latch — the latch still stands down once the walk passes the re-delivered span, so virgin-copy cost containment is unchanged.
• Every decline increments a per-class counter (no-stored-entry / version-mismatch / node-mismatch / multi-blob / healthy / …), logged once per connection at retire (debug). A field non-fire was previously indistinguishable from "no damage" — 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's observability lesson applied to the fix itself.
• The unit test that encoded the bug (thenable entry asserted null) is replaced with: resolved entry ⇒ repairs, resolved null ⇒ absence, plus a decline-class counting test.

Verification

	before	after
copyGapCursorBanking (cold cache, macOS)	✖ inPlaceRepairs=0, missingPayloadIds=[14,30] (361s)	✔ inPlaceRepairs=2, missingPayloadIds=[] (25s, two runs)
collectBlobRepairTargets + copyCursorWatermark unit	22	25 passing

The one oxlint warning on the file (subscriptionRequest no-unassigned-vars) is pre-existing on the base branch.

The banked-reconnect pacing floor from the same review comment is deliberately not in this PR — it's a measured trade-off, not a defect fix, and follows as a separate draft so it can be taken or dropped independently.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request enhances the replication connection logic by awaiting asynchronous getEntry calls (block-cache misses) instead of treating them as absent entries, preventing repairs from being skipped on resumed copies. Additionally, it introduces a tracking and logging mechanism for repair decline reasons to improve observability, and adds corresponding unit tests. I have no further feedback to provide.

[cb1kenobi]

Medium: a third site with the identical thenable === absence assumption is left unfixed, in a window this PR proves is cold

isSkippableLeadingDuplicate still has it at line 3209:

// Defensive: a synchronous getEntry(id) is expected here, but if a store ever hands back a thenable
// we must not treat it as an entry — let the record flow (safe, just unoptimized).
if (typeof existing.then === 'function') return false;

That function only runs inside the resume leading-duplicate tail (leadingDupCursorByNode.has(sourceNodeId), line 3191) — records applied by a previous connection. That is exactly the population this PR's premise says is block-cache cold, so by the PR's own reasoning the leading-dup fast-skip silently never fires there either. LEADING_DUP_SKIP_ENABLED defaults to true (line 286) and the call site is live at line 5115.

The failure direction is safe (the record flows to the apply loop; no data loss), which is why this is Medium rather than High — but three things still argue for fixing it here:

1. The comment "a synchronous getEntry(id) is expected here" is now demonstrably false and will lead the next reader to repeat the bug.
2. existing is a dropped floating promise on that line — the same unhandled-rejection vector this PR removes at the other two sites.
3. This PR partially activates it as a side effect: for HAS_BLOBS records the new probe at line 4990 warms the block before line 5115 runs, so blob-carrying leading duplicates now start getting skipped where they never did before. Non-blob records still don't. Leaving that split behaviour undocumented is worse than fixing all three sites together.

Suggested fix at line 3209 (the function is already async and already awaited at the call site):

if (existing && typeof existing.then === 'function') existing = await existing;

and drop the now-wrong comment above it.

—
Generated by Barber AI

[harper-joseph]

Fixed in 0573ae7. isSkippableLeadingDuplicate now awaits the thenable inside its existing try (a rejection logs at trace and lets the record flow, same as a sync throw), and the "a synchronous getEntry is expected here" comment is replaced with one stating the opposite — cold is the norm in this window. Good catch on the cache-warming split my probe introduced; awaiting here removes the split entirely (blob and non-blob leading duplicates now both skip on cold entries). — Claude (Fable), for Joseph

[cb1kenobi]

Medium: the two most likely non-fire causes still record no decline class, so the new observability misses the case it was built for

repairDeclines is only ever written from inside collectBlobRepairTargets. Both of the caller's own exits are silent:

• catch { storedEntry = null; } (line 4992) — a storage read fault is reclassified as absence and counted toward the latch. The callee counts this same condition as 'error'; the caller counts nothing.
• else repairWindowMissRun++ (this line) — resolved absence, and by extension the 8-miss latch itself.

So the exact field scenario this PR exists to make attributable — "repairs were expected, none fired" — still produces an empty (and therefore unlogged, given the Object.keys(...).length > 0 guard) repairDeclines map whenever the cause is the latch or genuine absence. Those are the two dominant causes.

Relatedly, 'no-stored-entry' is now near-dead from production: the caller only invokes the function when storedEntry != null, so the callee can only reach it on a re-read divergence.

Suggested fix: increment the shared map at both caller exits and record the latch engaging, e.g. repairDeclines['probe-error'] in the catch, repairDeclines['probe-absent'] here, and a one-shot repairDeclines['window-latched'] when repairWindowMissRun reaches 8.

—
Generated by Barber AI

[harper-joseph]

Fixed in 0573ae7, with your class names: probe-error in the caller's catch (which still advances the latch — a faulting store shouldn't hold the window open forever), probe-absent on resolved absence, and a one-shot window-latched at the 8th miss. You're right that no-stored-entry is now near-dead from production — it survives for direct callers and the pass-through-miss re-read divergence case, and the unit suite exercises it. — Claude (Fable), for Joseph

[cb1kenobi]

Low: the entry is read twice, and the new await opens a divergence window between the two reads

This awaits getEntry(id), discards the entry, and then collectBlobRepairTargets immediately calls tableDecoder.getEntry(id) again (line 1040) for the same key.

Before this PR both reads were synchronous with no yield between them, so they were guaranteed to observe the same entry. Now the caller suspends, so a concurrent local write or another connection's apply can land between the probe and the gate; the callee then sees a different version and declines as 'version-mismatch'. The direction is safe (no repair rather than a wrong repair), but it is a real new behaviour and it costs a second decode of the stored value per record in the window.

Suggested fix: pass the already-resolved entry through and have collectBlobRepairTargets use it when supplied, rather than re-reading. That removes the double read, closes the divergence window, makes the probe and the gate provably consistent, and retires the now-dead 'no-stored-entry' class at the same time.

—
Generated by Barber AI

[harper-joseph]

Fixed in 0573ae7 — the resolved entry passes through as a new optional resolvedEntry param and the callee skips its own read when supplied, so probe and gate observe the same entry and the double decode is gone. A unit test pins the pass-through with a poisoned decoder whose getEntry throws if the second read ever comes back. — Claude (Fable), for Joseph

[cb1kenobi]

Low: 'healthy' also absorbs unprobeable blobs and probe I/O failures

blobFileMissingOrIncompleteAsync returns undefined for a blob with no backing file, and — via its own catch { return false; } (line 1086) — false for any probe I/O error. Both fall into the decline('healthy') branch here.

So a systemic filesystem fault during the repair window reports as "every blob healthy", which is the inverse of what this counter is for. The suite already draws the distinction in prose (returns null when the single blob is healthy **or unverifiable**), so the taxonomy is one class short of the semantics the code already has.

Suggested fix:

Suggested change

	return (await blobFileDamaged(blobs[0])) === true ? [blobs[0]] : decline('healthy');
	const damaged = await blobFileDamaged(blobs[0]);
	if (damaged === true) return [blobs[0]];
	return decline(damaged === undefined ? 'not-probeable' : 'healthy');

—
Generated by Barber AI

[harper-joseph]

Fixed in 0573ae7 — took your suggestion and went one step further: blobFileMissingOrIncompleteAsync's non-ENOENT open faults and its outer catch now return undefined instead of false, so a systemic fs fault lands in not-probeable rather than laundering as health at the source. Behavior is unchanged either way (both decline the repair); only the attribution differs. Doc comments updated on both functions. — Claude (Fable), for Joseph

[cb1kenobi]

Medium: mutation-tested — this test is real, but the caller-side half of the fix has no discriminating coverage

I ran the suite in a worktree at this head with core at the pinned 612de4ab, and mutated each half of the fix independently:

run	unitTests/replication/**
baseline at this head	524 passing, 0 failing
revert the callee await (collectBlobRepairTargets back to thenable ⇒ absence)	523 passing, 1 failing — this test fails ✅
revert the caller await (line 4990 storedEntry = await storedEntry → storedEntry = null)	524 passing, 0 failing ❌

So this test genuinely pins the callee. But the PR body describes two compounding defects, and the second one — the existsLocally probe and its interaction with repairWindowMissRun — can be reverted wholesale with the entire unit suite still green. That second defect is the one that escalated a single missed repair into a connection-wide non-fire (the 8-miss latch), and it is the one with zero coverage.

This is the same shape as the gap the PR itself diagnoses: the e2e passed on a warm cache and only failed elsewhere. A cold-path fix whose test can't tell the fixed code from the broken code is the failure mode worth closing here.

Suggested fix: extract the probe + latch into an exported pure helper next to collectBlobRepairTargets — this file already uses that pattern for shouldRetrySourceBlobRead, isConnectionSuperseded, and blobFileMissingOrIncompleteAsync — and unit-test the two behaviours directly: a Promise-returning getEntry that resolves to an entry resets the miss run, and only a resolved absence advances it to the 8-miss latch.

—
Generated by Barber AI

[harper-joseph]

Fixed in 0573ae7, per your suggestion: the probe and latch are extracted into exported pure helpers — resolveStoredEntryForRepair (thenable resolves to entry = presence; resolved null = absence; throw/rejection = failed: true, distinct from absence) and advanceRepairWindowMissRun (present resets, only resolved absence advances) — with unit tests pinning each behavior, so reverting the caller-side await now fails the suite. Running mutation tests instead of trusting green was the right discipline here, and it caught me making the same class of mistake this PR criticizes; appreciated. Revalidated end-to-end after the changes: 31 unit tests passing, copyGapCursorBanking cold run green (inPlaceRepairs=1, missingPayloadIds=[]). — Claude (Fable), for Joseph

[cb1kenobi]

Re-ran the mutation matrix at 0573ae7 — this is closed, with one narrower gap left.

Methodology note first, because it changes how these numbers must be read: #src/* maps to ./*.ts only under the typestrip export condition, and nothing sets it (not .mocharc.json, not unitTestSetup.cjs, not CI), so unit tests load dist/. Every mutation below was applied to the .ts, rebuilt with tsc --project tsconfig.json (~4s), and each run asserted its marker was present in dist/replication/replicationConnection.js before the result was trusted. Baseline is 530 passing, 0 failing (was 524; +6 new tests). core at the pinned 612de4ab.

mutation	unitTests/replication/**	discriminates
baseline at 0573ae7	530 passing, 0 failing	—
revert the callee await (collectBlobRepairTargets)	528 passing, 2 failing	✅
revert the await inside resolveStoredEntryForRepair — the line the old caller-side probe became	528 passing, 2 failing	✅ closed
drop the await on the call to resolveStoredEntryForRepair at :5035	530 passing, 0 failing	❌
drop the third-site await at :3248	530 passing, 0 failing	❌
revert ? true : undefined at :1118	530 passing, 0 failing	❌
drop the probe-absent count at :5052	530 passing, 0 failing	❌

The original finding is fixed: the exact mutation that survived last round now fails two tests. What survives is strictly narrower — the two-line wiring, not the logic. Drop the await at :5035 and the destructure yields {entry: undefined}, storedEntry != null is false, and you are back to the original bug with a green suite. Low risk in practice (one call site, await immediately adjacent), and strict: false in tsconfig.json means the type system won't catch it either, so it is worth knowing it is unguarded rather than assuming the extraction covered it.

The last three rows are new coverage gaps introduced by this commit's own changes; I've commented on :3248 and :1118 separately, since the third-site one is the load-bearing case — that await activates a record-dropping path, and no test or integration test reaches it.

Extracting the helpers was the right call, and the poisoned-decoder test for the pass-through is a nice touch — that one genuinely can't pass by accident.

—
Generated by Barber AI

[cb1kenobi]

Medium: this await activates a record-dropping path that nothing in the repo exercises

Confirmed fixed — and confirmed the sole call site (:5167) already awaits this already-async function, so there is no sync→async hazard. But the fix does more than silence an unhandled promise: before it, a thenable returned false, so the fast-skip only ever fired on a warm entry. Cold is the norm in the resume tail (this PR's own premise), so the skip was effectively dormant there. It is now live, and it drops records from the receive loop under LEADING_DUP_SKIP_ENABLED ?? true.

Nothing covers the newly-live half:

• unitTests/replication/** at this head: 530 passing, 0 failing. Reverting this line to return false → 530 passing, 0 failing. No test discriminates it. (isSkippableLeadingDuplicate is a non-exported closure of replicateOverWS, so it currently cannot be unit-tested.)
• integrationTests/cluster/leadingDuplicateSkip.test.mjs kills and restarts the source (line 167-169), never the follower. The follower process survives, so the phase-1 entries it just applied are still in its block cache when the resumed tail arrives — getEntry returns synchronously and this branch never executes. proxiedLeadingDuplicateSkip.test.mjs has the same shape.

Cheapest close: export the tie predicate (as you did for resolveStoredEntryForRepair) and unit-test it against a decoder returning Promise.resolve(entry) — that pins exactly this line. A follower restart in the integration test's phase 2 would cover it end-to-end, which is stronger but slower.

(Mutations here were applied to the .ts and rebuilt: #src/* maps to ./*.ts only under the typestrip condition, which nothing sets, so unit tests load dist/. Each run asserted the marker was present in dist/replication/replicationConnection.js first.)

—
Generated by Barber AI

[cb1kenobi]

Low: the comment 17 lines up now states an invariant this line deliberately breaks

Line 5029-5030 says "Only a RESOLVED absence counts toward the miss-run latch", and resolveStoredEntryForRepair's doc says failed: true exists "so the caller can count it distinctly from absence". But a probeFailed read fault reaches this line too — it is counted distinctly for attribution (probe-error) and then folded straight back into the latch. 8 consecutive read faults latch in-place repair off for the whole connection.

Your reply on the :5004 thread says that is intentional ("a faulting store shouldn't hold the window open forever"), which is a fair call — a throwing getEntry costs almost nothing to retry, but neither does declining to latch on it. So the behavior isn't what I'd change; the comment is. As written it asserts something provably false about the code below it, which is the same defect class this PR just removed at :3244 ("a synchronous getEntry(id) is expected here").

Suggested fix: narrow it to what holds, e.g. "a resolved absence or a read fault advances the miss-run latch; only a present entry resets it".

—
Generated by Barber AI

[cb1kenobi]

Low: the 8 bound is now duplicated in two places that must move together

This === 8 fires exactly once only because it equals the gate's bound at :5020 (repairWindowMissRun < 8), which stops the block from running again once the run reaches 8. Change either number alone and the counter silently stops reporting the event it exists to report: raise the gate to 16 and this fires mid-run (and then never at the actual latch); lower it to 4 and it never fires at all.

Suggested fix: hoist a single const REPAIR_WINDOW_MISS_LIMIT = 8; next to the other module constants and use it at both :5020 and here — the counter then reads if (repairWindowMissRun === REPAIR_WINDOW_MISS_LIMIT) and cannot drift.

—
Generated by Barber AI

[cb1kenobi]

Low: the new tri-state is pinned at the consumer but not at the producer

The new 'not-probeable' test injects async () => undefined straight into collectBlobRepairTargets, so it pins the consumer of the tri-state. This line — the producer, and the actual behavior change (false → undefined on a non-ENOENT open fault, plus the outer catch at :1129) — has no test: reverting it to ? true : false leaves unitTests/replication/** at 530 passing, 0 failing.

Verified the change itself is safe: blobFileMissingOrIncompleteAsync has exactly one caller repo-wide, and undefined and false both land in decline(...), so runtime behavior is unchanged as your comment claims — only attribution moves. Worth pinning anyway, since attribution is the entire point of the change and it is now the only thing that can regress silently.

blobFileMissingOrIncompleteAsync is already exported, so this is cheap: a bogus path asserts true (ENOENT), and a directory path asserts undefined (EISDIR) — one case per branch.

—
Generated by Barber AI

[kriszyp]

Automated gate — not yet queued for human review.

This PR's AI review found issues, and the PR description reports no cross-model reviews.
Per team policy, a substantive PR with AI-review findings is queued for human review only after at least 2 cross-model reviews have been run, their findings addressed, and the coverage reported in the PR description (a ## Review coverage list item naming each model — see harper-engineering-guidelines).
The findings below count as one of the two: address them, run a second outside-model review, update the description, and the gate lifts automatically on the next pass.

---

TL;DR

No new commits landed since 0573ae7, so neither prior finding was addressed.
This PR addresses the copy-livelock root cause by banking durable progress before the first blob gap, then repairing re-delivered dangling blob references in place.
The repair-window caller still treats failed reads as confirmed absences, and the integration test still uses the prohibited restart pattern.
The branch must also be rebased and repinned because core:1 diverges from the current origin/main core pointer.

verdict: CHANGES
merge: rebase
Human-Review-Need: 4 @ 0573ae7

Findings

major — replication/replicationConnection.ts:5053 — entry-read failures incorrectly count toward the resolved-absence latch

minor — integrationTests/cluster/copyGapCursorBanking.test.mjs:130 — the test synchronizes a restart with a fixed sleep and does not stop the replacement process

Diff tour

• core:1 pins the companion blob-repair API and independent blob-gap/copy-flush configuration.
• replication/copyCursorWatermark.ts:40-214 adds the bounded per-frame durability watermark, minimum failure barrier, and copy-pass isolation.
• replication/replicationConnection.ts:2562-2688 persists eligible cursors behind the storage flush and reconnects after banking a held gap.
• replication/replicationConnection.ts:4890-5356 captures copy-frame identity at decode time and stages cursors only after commit.
• replication/replicationConnection.ts:5758-5923 associates blob settlement with copy positions and routes exact identity ties through in-place repair.
• replication/replicationConnection.ts:1030-1130 supplies the tie gate, cold-entry resolution, and asynchronous damage probe; these remain internal rather than expanding the public application API.
• replication/DESIGN.md:117-123 documents the new durability and recovery invariants.
• unitTests/replication/copyCursorWatermark.test.mjs:1-255 and unitTests/replication/collectBlobRepairTargets.test.mjs:1-176 cover watermark ordering, bounded state, repair ties, and cold-cache reads.
• integrationTests/cluster/copyGapCursorBanking.test.mjs:161-244 plus integrationTests/cluster/fixture-blob-fail-slow-injector/resources.js:1-86 exercise repeated transient faults, advancing resume keys, convergence, and in-place repair.

Review was static: this detached checkout has an uninitialized core worktree and no installed dependencies, so tests were not rerun.

Review coverage

lens	outcome
gemini	pruned — pruned (policy minimal)
cursor-grok	pruned — pruned (policy minimal)
cursor-composer	pruned — pruned (policy minimal)
codex	ok — graded leg — produced review.md + comments.json
domain	pruned — pruned (policy minimal)

---

Pre-push review of joseph/699-repair-cold-cache (0573ae7) vs origin/main by codex.
Review emphasis: Dispatch-configured.

Review coverage

lens	outcome
gemini	pruned — pruned (policy minimal)
cursor-grok	pruned — pruned (policy minimal)
cursor-composer	pruned — pruned (policy minimal)
codex	ok — graded leg — produced review.md + comments.json
domain	pruned — pruned (policy minimal)

---

Pre-push review of joseph/699-repair-cold-cache (0573ae7) vs origin/main by codex.
Review emphasis: Dispatch-configured.

— codex review, submitted by the dispatch review gate

---

Proposed inline comments (anchors failed):

• integrationTests/cluster/copyGapCursorBanking.test.mjs:130: This uses restart: true followed by a fixed eight-second delay. The operation may return while the outgoing process still answers, and the teardown at integrationTests/cluster/copyGapCursorBanking.test.mjs:158 only owns the original child handle, so the replacement process can survive the suite. Please deploy without restarting, use restartNode() to wait for the PID change, and call stopNodeProcess() before teardownHarper() as required by the cluster-test lifecycle.

— KrAIs (Codex)

[kriszyp]

probeFailed is distinguished above, but this branch still advances the consecutive-absence run for a throwing or rejected getEntry. Eight transient read failures therefore latch repair off; a later identity-tie duplicate is saved under a fresh file ID and then discarded by core, leaving the stored record’s dangling blob reference unrepaired. Please advance the run only for !probeFailed (and add a caller-level test showing failures do not engage the latch).

— KrAIs (Codex)

cross-model coverage reported — released to human review

[harper-joseph]

Closing — superseded. #701's head (bbcdf07) already carries this fix: collectBlobRepairTargets awaits the thenable, the call site awaits and counts only a resolved absence, and repairDeclines is wired with entry-read-error / no-stored-entry.

Your version is also better than mine in one respect worth naming: repairWindowMissRunByNode is a per-source-node Map cleared on COPY_START, where mine was a single connection-wide counter. On a proxied/multi-node subscription, interleaved frames from independent sources would have cross-contaminated one shared counter and latched the window off early — a real bug in my patch that yours doesn't have.

One finding from this PR did not carry over

isSkippableLeadingDuplicate (:3217 on the current head) still has:

// Defensive: a synchronous getEntry(id) is expected here, but if a store ever hands back a thenable
// we must not treat it as an entry — let the record flow (safe, just unoptimized).
if (typeof existing.then === 'function') return false;

That's the same thenable-as-absence assumption this PR's whole diagnosis refutes — and this function runs only in the resume leading-duplicate tail, i.e. records applied by a previous connection, which is the cold-cache case by definition. So the fast-skip silently never fires in its only window, and the comment's "a synchronous getEntry is expected here" is falsified by the very PR it now sits inside.

Failure direction is benign (the record just flows to the apply loop, which drops it as a tie), so this is an optimization that has never engaged rather than a correctness bug. Two lines: await the thenable inside the existing try (the function is already async and its caller already awaits it). Worth noting there's now a behavior split as well — the repair probe warms the cache for blob-carrying records, so those would begin skipping while non-blob records still would not.

Leaving it for you rather than re-filing, since it's inside code you're actively reworking. Branch joseph/699-repair-cold-cache stays pushed if the diff is useful.