Repair dangling blob references when a base copy re-delivers the record; add replication.blobGapReconnectMs

[kriszyp]

Core companion to “Bank bulk-copy progress across blob-gap reconnects and repair missing blob files on copy re-delivery”. A transient replication save can leave a committed record pointing at a missing, PENDING, or incomplete blob file. When Pro later receives the exact same record, it can now repair that existing local file ID instead of saving a new orphan that the duplicate-skipped record will never reference.

The repair is fail-atomic: it holds the blob lock, writes and verifies a sibling temporary file, flushes it, and renames it over the target before releasing either lock. A failed repair leaves the referenced file byte-for-byte unchanged. The orphan sweeper respects active repair locks and still reclaims stale repair files.

Core also adds the shared header classifier and the replication configuration keys consumed by Pro: blobGapReconnectMs, copyCursorFlushBytes, and copyCursorFlushIntervalMs.

For the human reviewer

1. Identity is enforced at the Pro boundary. Core verifies the reported size, received byte count, and stored descriptor size. The companion caller admits repairs only for the same record version and source node, restricts pairing to exactly one positional file-backed blob, and rejects the delivery on any declined repair. A content digest would move this invariant into core, but requires a coordinated protocol change.
2. Repair stays under the existing file ID. This avoids rewriting and re-replicating the record descriptor. The alternative is allocating a new file ID and performing a convergent record write across both repositories.
3. Replacement files are deliberately uncompressed. Missing and PENDING files do not retain their prior encoding metadata. Repairs explicitly write a consistent uncompressed header/body and always fdatasync before rename; the tradeoff is possible storage growth for a formerly compressed blob.
4. The synchronous probe is only the locked final check. Pro performs its candidate scan asynchronously and calls this path only after establishing an exact duplicate. Compressed-body corruption that cannot be established from the header remains the asynchronous repair sweep's responsibility.
5. The core primitive has unit coverage; the trigger is cross-repo. The companion integration test is the end-to-end proof that the copy receiver selects and repairs the target. Prior companion runs repaired both damaged records in place and left all 40 referenced payloads readable; those runs preceded the final core-only hardening commits.

Verification

• npm run build — passing.
• npm run lint:required — passing.
• npx prettier --check resources/blob.ts unitTests/resources/blob.test.js and git diff --check — passing.
• npx mocha unitTests/resources/blob.test.js — 98 passing.
• npx mocha unitTests/validation/configValidator.test.js — 72 passing.
• npm run test:unit:resources — 1,581 passing, 15 pending, with the same three unrelated baseline failures in random-access defaults and replay-structure representation.

Review coverage

Full reviews ran through Claude, Gemini, and the Harper domain adjudicator. They found and drove fixes for atomic failure handling, source/descriptor size agreement, lock cleanup, normal-save state isolation, orphan-sweeper coordination, and sliced-blob rejection. Exact-head delta review at c3d9c7b8fc8d used Claude and Gemini. Claude graded the remaining core findings as minor scope boundaries and explicitly confirmed that fdatasync is imported and that the settled WriteStream retains neither an FD nor a timer. The automatic score remains 4 because the low-risk documentation delta pruned domain adjudication while Gemini repeated that already-refuted stream-lifetime finding.

_{Human-Review-Need: 4 @ c3d9c7b}

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to repair copy-apply duplicate records when their referenced blob files are missing or incomplete on disk. It adds the blobFileMissingOrIncomplete function in resources/blob.ts to synchronously verify the integrity of file-backed blobs, and integrates this check into the table write path in resources/Table.ts via recordReferencesMissingBlob. Additionally, new replication configuration parameters (blobGapReconnectMs, copyCursorFlushBytes, and copyCursorFlushIntervalMs) are added to the terms and validator, and comprehensive unit tests are provided for the new blob integrity checks. There are no review comments, so I have no feedback to provide.

[claude]

Reviewed; no blockers found.

[kriszyp]

Design rework since the original draft (per Kris): Table.ts is now untouched — the repair is a new blob-level primitive (repairBlobFile) that overwrites the damaged file under its existing fileId, invoked by harper-pro's receiver on identity-tie re-deliveries. PR description is updated; earlier review context predating the rework is superseded. — Claude (Fable)

[harper-joseph]

Drafted fix for the stampPendingBestEffort lock race flagged inline (apply with git apply). Same shape as the suggested remedy: re-acquire the :blob lock before the re-stamp, hold it through the async write (the pattern the idle-timeout retry stamp already uses), and skip the stamp entirely if the lock is contended — a concurrent writer's outcome supersedes this failed attempt, so racing a stale PENDING header over their finalized write is never acceptable.

--- a/resources/blob.ts
+++ b/resources/blob.ts
@@ -1603,14 +1603,26 @@ export function repairBlobFile(blob: Blob, source: Readable): Promise<void> | un
 		const lockKey = storageInfo.fileId + ':blob';
 		if (!storageInfo.store.tryLock(lockKey)) return undefined;
 		const stampPendingBestEffort = (reason: string) => {
+			// The settle handlers below run AFTER writeBlobWithStream's completion path released the
+			// :blob lock, so a NEW writer for this fileId (a retried repair, or a fresh save — plausible
+			// here, since repair fires on identity-tie duplicate deliveries that can arrive on several
+			// links) may already own the file. Re-acquire before stamping and hold the lock through the
+			// async write (the same pattern as the idle-timeout retry stamp in writeBlobWithStream);
+			// failing to acquire means someone is actively writing — their outcome supersedes this failed
+			// attempt, so the stamp must be skipped, never raced over their finalized header.
+			if (!storageInfo.store.tryLock(lockKey)) return;
 			try {
 				const messageBuffer = Buffer.from(reason);
 				const header = new Uint8Array(HEADER_SIZE);
 				new DataView(header.buffer).setBigInt64(0, BigInt(messageBuffer.length) | (BigInt(PENDING_TYPE) << 48n));
 				writeFile(filePath, Buffer.concat([header, messageBuffer]), (writeError: Error) => {
+					storageInfo.store.unlock(lockKey);
 					if (writeError) logger.debug?.('Error re-stamping pending marker after failed blob repair', writeError);
 				});
-			} catch {}
+			} catch (stampError) {
+				storageInfo.store.unlock(lockKey);
+				logger.debug?.('Error re-stamping pending marker after failed blob repair', stampError);
+			}
 		};
 		try {
 			writeBlobWithStream(blob as any, source, storageInfo, { deferSizeHeader: true, lockHeld: true });

Structural notes: tryLock is the first statement, before the try, so the catch can never unlock a lock this call never took; the unlock lives in the writeFile callback (held through the async write) with the catch as the synchronous-throw fallback. A skipped stamp is safe in both invoking paths — on the rejection path writeBlobWithStream's own failure handling has already left the file in a non-complete state, and on the size-mismatch path the concurrent writer holding the lock is writing the same identity-tie content anyway; the next re-delivery re-probes either way.

Validation extent, honestly: compiles clean under harper-pro's tsc (which builds core/); harper-pro's copyGapCursorBanking passes end-to-end against this change twice (inPlaceRepairs=2, missingPayloadIds=[] — with the repair non-fire fix posted on harper-pro#701, which this pairs with). The stamp paths themselves are failure-path-only, so that e2e doesn't reach them, and the three existing repairBlobFile unit tests don't either (success/decline shapes) — I couldn't run core's own suite from the submodule checkout (its harness needs a standalone core environment), so this diff carries the same unit-coverage gap as the code it replaces. A contention unit test would need a hook to grab the lock between writeBlobWithStream's release and the settle handler — worth adding if you have a seam for it, but the change is strictly narrowing (stamp happens in fewer states than before, never in more).

[harper-joseph]

Reviewed c3d9c7b8. The redesign is a clear improvement over both earlier shapes — I withdrew #2198 because this obsoletes it, and I'd rather have this than the version I was patching.

What's better, specifically:

• Fail-atomic beats guarded-mutation. Temp file → verify → fdatasync → rename means a failed repair provably cannot alter the referenced file. That deletes the entire class of failure-handler bugs the previous shape had (the stamp race I filed, and the "briefly readable as complete before the re-stamp" window that was documented as accepted). Strictly fewer states.
• The classifier is now genuinely shared — the locked repair probe (:1767) and the sweep (:2650) both call blobHeaderIndicatesIncomplete, so they can't drift. That was an open concern from earlier rounds and it's properly closed.
• Whitelist over blacklist. type !== UNCOMPRESSED_TYPE && type !== DEFLATE_TYPE ⇒ incomplete fails safe for any unknown/future type, where the old ERROR/PENDING blacklist would have read it as healthy. Also readUInt16BE(0) reads the full 16-bit type field rather than header[1]'s low byte.
• The repair-temp sweep respects live repairs (tryLock per .repair path, skip on contention, unlock in finally) — the obvious hazard of introducing a second file, handled.
• Slice guard (storageInfo.start/end !== undefined → decline) and the size agreement chain (source-reported ↔ descriptor ↔ received) are good; declining synchronously with undefined so the caller's fresh-save fallback runs is preserved and still pinned by test.

Two findings, both narrow:

1. blobHeaderIndicatesIncomplete now requires an exact size match for uncompressed — this loosens the sweep in one direction and tightens it in another. Old sweep logic treated fileSize < HEADER_SIZE + storedSize as incomplete; the shared version uses fileSize !== HEADER_SIZE + storedSize. Tightening (longer-than-declared is now damaged) is right. But worth confirming the intent for the over-long case in the sweep path specifically: a file with trailing garbage now classifies as incomplete and becomes repair/reclaim-eligible, where before it read as complete. I think that's the behavior you want — I just can't tell from the diff whether it was a deliberate consequence or a side effect of unifying the two checks, and the sweep has a wider blast radius than the repair gate. If deliberate, a line in the function comment saying "exact, not minimum — a longer body means a torn or double-written file" would lock it in.

2. Repairs are written uncompressed even when the original was DEFLATE (reviewer note 3), which interacts with finding 1. The note frames the tradeoff as storage growth, and I agree that's fine. The part I'd double-check is the header the repair writes: the size field must hold the uncompressed length with UNCOMPRESSED_TYPE, so fileSize === HEADER_SIZE + storedSize holds exactly on the repaired file. If a repaired file's header ever disagreed with its body length, the shared classifier would now flag it damaged on the very next probe — a repair loop rather than a convergence. The unit coverage for repair-lands-correct-bytes presumably pins this; explicitly asserting blobFileMissingOrIncomplete === false on the repaired file (if it doesn't already) would make the round-trip invariant airtight, since that's the exact predicate the next delivery re-runs.

Neither is a blocker in my read. 1 is a documentation/intent question, 2 is a belt-and-braces assertion on an invariant I believe already holds.

Scope note, not an objection: the two dead #480 config keys and blobGapReconnectMs ride along here. Registering previously-silent keys is a real fix and I'm glad it's in, but it does mean this PR is repair + config surface — if anything in the config half needs another round, it'd be a shame for it to hold the repair.

I have not run core's suite from my checkout (harper-pro submodule; its harness needs a standalone core env), so I'm relying on your 98-passing blob.test.js run and the companion e2e. With those green, this looks good to me. — Claude (Fable), reviewing for Joseph