Repository navigation
fix(replication): advance resume cursor past source-missing (ENOENT) blobs instead of wedging (#403) - #405
Merged
Conversation
…blobs instead of wedging Blob replication wedged permanently on an expiration cache table when a source blob was evicted/expired: the sender's blob read failed with ENOENT, it forwarded a BLOB_CHUNK error marker, and the receiver set `hasBlobGap` on the resulting save failure — pinning the resume cursor. Every reconnect re-streamed the same now-missing blob and re-failed, so the cursor never advanced and `blobReplicationFailures` climbed without bound (observed live on preprod.jjl, harper-pro#403). Classify the failure in `receiveBlobs`: a source-reported PERMANENT absence (sender forwards `errorCode: 'ENOENT'`) is unrecoverable, so advance the durable watermark past it — logged loudly via the existing `cluster_status.blobReplicationFailures` metric plus a per-blob error — and leave the diverged record for proactive blob backfill (harper-pro#388). Local/transient save faults, and a transient sender read fault (EIO, EMFILE, timeout) or an older sender that doesn't forward `errorCode`, still set `hasBlobGap` and hold so a reconnect retries — never silently skipping a recoverable blob. New exported pure helpers (`markSourceBlobUnavailable`, `isUnrecoverableSourceBlobError`, `isPermanentSourceBlobErrorCode`) are unit-tested; a stress-gated integration test + source-read-fail fixture exercise the end-to-end wiring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
|
Reviewed; no blockers found. |
kriszyp
marked this pull request as ready for review
June 17, 2026 02:58
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
…ource-missing blob A failed receive-side blob save leaves a header-only stub on disk: saveBlob writes the size header before the body, and the non-deleteOnFailure error path keeps the (non-empty) file rather than unlinking it. For a source-reported permanent (ENOENT) blob the #403 fix advances the resume cursor past it but previously left that stub behind, so stubs accumulated unreclaimed (observed: hundreds of 8-byte files on preprod.jjl) and masqueraded as real empty blobs. Unlink the stub on the advance-past path: a missing file is the unambiguous "needs backfill" signal for harper-pro#388, and nothing references a blob we've decided is unrecoverable. Transient/local gaps are deliberately NOT cleaned — their reconnect re-stream re-saves the same fileId and overwrites the stub. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
heskew
approved these changes
Jun 17, 2026
|
|
||
| const STRESS = process.env.HARPER_RUN_STRESS_TESTS === '1'; | ||
|
|
||
| suite('Source-unavailable blob does not permanently wedge replication', { skip: !STRESS, timeout: 300000 }, (ctx) => { |
Contributor
There was a problem hiding this comment.
Does this need to be explicitly wired up in the ci stress matrix?
This was referenced Jun 19, 2026
kriszyp
added a commit
that referenced
this pull request
Jun 22, 2026
… forwarded statusCode (#429) (#443) * fix(replication): classify gone/corrupt source blobs as permanent via forwarded statusCode (#429) A confidently incomplete/truncated source blob ("Blob is incomplete") was misclassified as a transient failure on the receiver: core threw a plain Error with no `.code`, sendBlobs forwarded `errorCode: undefined`, and isPermanentSourceBlobErrorCode(undefined) → false → hasBlobGap pinned the resume cursor forever, since every reconnect reproduces the identical error. Same failure class as #403, for *incomplete* blobs instead of *missing* ones (#429). Core PR harper#1425 reworks the blob read paths to throw BlobReadError carrying an HTTP-style statusCode (404 gone / 500 corrupt-incomplete / 503 transient) and no raw fs `.code`. This: - forwards `errorStatus` (the BlobReadError statusCode) alongside `errorCode` from sendBlobs, and - classifies 404 and 500 as PERMANENT (advance the resume cursor past + mark sourceBlobUnavailable → #418/#388 backfill + #386 divergence metric), while 503 stays transient (hold the gap, retry on reconnect). Besides resolving #429, the `errorStatus === 404` arm preserves the already- shipped #405 ENOENT advance-past: once harper#1425 lands, core wraps even ENOENT into a code-less BlobReadError(404), so without forwarding/honoring the status a missing source blob would start wedging again. The `errorCode === 'ENOENT'` arm remains for pre-#1425 senders. A sender that forwards neither stays on the safe hold default (mixed-version clusters). Must ship together with harper#1425. Unit coverage added for the new statusCode arm in blobReplicationFailure.test.mjs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(replication): stress integration test for the #429 incomplete-source-blob advance-past Sibling to replicationBlobSourceUnavailable.test.mjs (the ENOENT/404 path); this covers the TRUNCATED/incomplete ("Blob is incomplete", 500) source blob, the case #429 wedged on. - fixture-blob-truncate-source: on the SOURCE, truncates a deterministic subset of blob files (by fileId % MODULUS) to KEEP_BYTES on the write stream's `close`, so the header still records the full size but the body is short. A later replication read hits core's BlobReadError('Blob is incomplete', 500) (harper#1425). - replicationBlobIncompleteSource.test.mjs: asserts the receiver advances the resume cursor past the incomplete blob ("advancing the resume cursor past it") and that the driving error was the incomplete/500 arm (not ENOENT), proving this classifier path. Stress-gated (skip unless HARPER_RUN_STRESS_TESTS=1), like its siblings. Requires a `core` submodule that includes harper#1425's BlobReadError statusCode taxonomy; until that syncs to harper-pro's pinned core, the stress run is a no-op skip. Validated locally only up to fixture load — full 2-node end-to-end is blocked in-sandbox by the same replication-server bind limitation that blocks the existing sibling test; to be exercised by stress CI once core syncs #1425. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(replication): wait for node A's replication port before add_node startHarper returns before the replication worker has bound its secure port (observed ~empirically: the listener appears a beat after 'successfully started'), so firing add_node immediately races into ECONNREFUSED. Poll the port until it accepts (30s budget) before connecting the cluster. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Jun 23, 2026
* test(cluster): promote QA-campaign cluster regression tests Add three cluster regression tests verified passing on main: - replicationConflictDeterminism: LWW convergence, no split-brain, addTo CRDT merge - typedStructReplicationDivergence: randomAccessFields:true replication across pre-diverged/late-join/restart (#1163 guard) - blobOrphanFullCopyConverges: TTL-orphaned blobs don't wedge full-copy (#403/#405/#429 guard) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(cluster): rename QA fixtures to match test names fixture-qa014-conflict -> fixture-replication-conflict-determinism fixture-qa178-struct-dict -> fixture-typed-struct-replication-divergence fixture-qa177-blob-ttl-copy -> fixture-blob-orphan-full-copy-converges Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(lint): prefix unused label param with underscore --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On a replicated table with expiration (cache) enabled, blob replication could wedge permanently. When a source blob was evicted/expired, the sender's blob read failed with
ENOENT, it forwarded aBLOB_CHUNKerror marker, and the receiver treated that like any blob save failure: it sethasBlobGap, pinning the resume cursor. On reconnect the source re-streamed the same now-missing blob, hit the sameENOENT, and the cursor held again — forever.cluster_status.blobReplicationFailuresclimbed without bound and never drained (diagnosed live onpreprod.jjl, harper-pro#403).The fix classifies the failure in
receiveBlobs:sendBlobscatch now forwardserrorCode, and the receiver marks the blob unrecoverable only whenerrorCode === 'ENOENT'. For that case it does not sethasBlobGap, so the durability watermark advances past the blob (logged loudly via the existingblobReplicationFailuresmetric + a per-blob error), leaving the diverged record for proactive backfill (harper-pro#388).EIO/EMFILE/timeout), or an older sender that doesn't forwarderrorCode— still setshasBlobGapand holds, so a reconnect retries. Never silently skips a recoverable blob.Purpose
Unblock the permanent replication wedge that one dead/expired blob caused for an entire connection (the immediate harper-pro#403 production symptom), without reintroducing the silent-loss risk the hold-the-cursor design (#368/#386) exists to prevent.
Where to look
replication/replicationConnection.ts— sender forwardserrorCode(sendBlobscatch); receiver tags the destroy error viamarkSourceBlobUnavailableonly on a permanent code (BLOB_CHUNKhandler); the save.catchinreceiveBlobsbranches onisUnrecoverableSourceBlobError(advance + loud log vs. sethasBlobGap). The watermark advance in.finally/onCommitis unchanged — it already keys on!hasBlobGap.errorCodeis a new optional field on theBLOB_CHUNKerror payload. Old receiver ignores it; old sender omits it (receiver then treats the failure as transient and holds). Both degrade safely to the pre-fix hold behavior.Tests
unitTests/replication/blobReplicationFailure.test.mjs): the new pure helpers, incl. the narrow ENOENT-vs-transient policy (isPermanentSourceBlobErrorCode). Full replication unit suite (126) green locally.integrationTests/cluster/replicationBlobSourceUnavailable.test.mjs+fixture-blob-fail-source-read): stress-gated (HARPER_RUN_STRESS_TESTS=1), asserts the receiver takes the advance branch end-to-end. Not run locally (stress-gated, needs a multi-node loopback cluster) — runs in the stress lane / CI.Review provenance
Cross-model review (Codex + Gemini) ran on this change. Codex caught a real blocker in the first cut — the classification marked all source errors unrecoverable, which would have advanced past transient sender faults and lost recoverable blobs. That's fixed here (narrowed to
errorCode === 'ENOENT', with the helper + tests above). No open review findings remain.🤖 Generated by Claude (Opus 4.8). Fix for harper-pro#403.
Update — stub cleanup (follow-on commit)
A failed receive-side blob save leaves a header-only stub on disk (
saveBlobwrites the size header before the body; the non-deleteOnFailureerror path keeps the non-empty file). The advance-past path nowdeleteBlobs that stub, so a source-missing blob leaves a missing file (the unambiguous "needs backfill" signal for #388) rather than an 8-byte stub that masquerades as a real empty blob and accumulates. Transient/local gaps are deliberately left alone (their reconnect re-stream re-saves the same fileId).While verifying this on
preprod.jjlI measured the blob store with the audit-awarecleanup_orphan_blobssweep: it reclaimed 1059 true-orphan blobs on one node — a separate, larger leak in theSUBSCRIPTION_UPDATEreceive path (received blobs aren't cleaned up when a replicated record's apply is skipped by a version conflict). Filed as #406; out of scope for this PR.