fix(replication): hold and reconnect on a sync blob-setup fault instead of skipping the record as undecodable

[heskew]

receiveBlobs runs synchronously inside the decode blob callback, so a local, typically transient blob-store fault (EMFILE, directory creation, a store error registering the in-flight marker) surfaced through the decode catch and was classified as a permanent decode failure — decode-drop counted, resume cursor advanced, a still-deliverable record permanently sealed under it on the follower. This ships in v5.2.2. The fix tags such throws as BlobSetupError at the only place they can originate, adds a hold-blob-setup verdict to classifyReplicationDecodeError (checked before the skip classes), and routes it through the existing missing-decoder hold: DECODE_HOLD metric, inbound latched off first (#440), close(1011), reconnect resumes from the durable cursor and re-requests the record. Also removes the dead shouldCloseOnRecordDecodeFailure helper and realigns its stale docs/tests with the shipped disposition. Fixes #715. Surfaced by @kriszyp's post-approval review on #691.

For the human reviewer

1. Throw-site-as-fault-proxy. Any sync throw from receiveBlobs's prologue is treated as a local transient fault, rather than inspecting the fault itself (an errno allowlist). A data-dependent throw there (e.g. getFileId on a malformed-but-decoded blob ref) would hold-loop instead of skip-and-count. Chosen for simplicity and because the prologue's known failure modes are local; the classifier is isolated, so revisiting is cheap.
2. instanceof across the decode stack (settled by trace, veto-able). The wrapper must cross core's decode stack identity-intact. Traced closed against current core: the blob extension invokes the callback bare (core/resources/blob.ts:2105), msgpackr's unpack catches annotate and rethrow the same object, decodeBlobsWithWrites rethrows raw; constraint documented on the class. The alternative (closure-local fault latch, no identity dependence) remains strictly more robust to future core changes — say the word and I'll switch. The proposed follow-up e2e injection test pins identity against drift either way.
3. Hold-forever vs bounded retry. A persistent local fault (disk full, unwritable blob dir) wedges the leg in an observable reconnect loop (per-attempt warn + metric) rather than eventually skipping. Deliberate Sync faults from receiveBlobs setup are classified as permanent decode failures — cursor advances past a recoverable record #715 semantics: availability sacrificed over silent data loss. Reversible.
4. Accepted blobs kept on hold. The hold path skips the 60s unlink the skip paths schedule, relying on the reconnect re-stream re-saving the same fileIds. If the held record is never re-delivered (audit trimmed during an extended outage), the files linger until Proactive blob backfill: repair already-committed records whose blobs are missing/corrupt (no recovery once the resume cursor advances past them) #388-style backfill — a small bounded leak. Reversible.

Verification

• Unit gate: 719 passing; the 1 failure (injectedKeyCustody /proc fd test) fails identically on untouched main — pre-existing macOS platform limitation.
• New unit tests pin the classifier seam: hold routing, non-demotion when the wrapped cause matches a skip class, cause preservation.
• Integration gate (test:integration:all): 131 tests — 118 pass, 12 skipped (stress-gated), 1 failure (can track child process CPU time) reproduced identically on an untouched origin/main worktree — pre-existing platform failure, not from this change.
• Fails-on-base: structurally unavailable at the unit seam — on base there is no way to express "blob-setup origin"; the fix is the seam. The end-to-end proof (injected sync blob-setup fault → DECODE_HOLD, no cursor advance, record delivered after reconnect) is the proposed follow-up e2e via a maybeInjectDecodeFailureForTest-style hook (see Sync faults from receiveBlobs setup are classified as permanent decode failures — cursor advances past a recoverable record #715 design note).
• No user-facing surface (API/config/schema/CLI) changes — no docs PR needed.
• Patch consideration: defect ships in v5.2.2; per policy, backport only on customer need — flagging, not requesting.

Review coverage

Authored by Claude (Fable 5). Cross-model review at 62c9098: codex ✓ (graded leg), Harper-domain adjudication ✓ (adjudicated severity: minor; the codex major — hold return bypassing blob cleanup — was dropped as not-real with traced evidence: transient-gap rule + retireInstance sweep + crash-safe unwired-stream destroy). gemini ✗ (no-output), cursor-grok ✗ (git fetch timeout), cursor-composer pruned. Step-6 planning-mode CLI did not run before coding (design note went to #715 instead); the implementation-mode full review above is the outside coverage. HEAD since moved only by a comment-level amendment (8357f67) answering the ledger's identity question — no behavior delta since the reviewed SHA.

_{Human-Review-Need: 4 @ 8357f67}

[gemini-code-assist]

Code Review

This pull request refactors replication error handling by introducing BlobSetupError to distinguish transient local blob-store faults from permanent decode errors. Instead of skipping records on local blob setup failures, the system now holds and reconnects to retry. The review feedback suggests enhancing the robustness of the error classification by using a custom property check (e.g., isBlobSetupError) rather than relying solely on instanceof checks, which can fail if errors are wrapped or cloned.

[claude]

Reviewed; no blockers found.

[ldt1996]

I verified the load-bearing claims directly: the hold branch mirrors the missing-decoder hold including the latch-before-close ordering, and returns before the blob unlink, the watermark, and the maxBatchVersion advance; the transient-gap rule in receiveBlobs' catch does leave stubs for the reconnect re-stream to overwrite, so keeping accepted blobs on hold is consistent with existing policy; the identity chain (bare callback invocation in core, msgpackr rethrowing the same object, decodeBlobsWithWrites rethrowing raw) checks out; and shouldCloseOnRecordDecodeFailure has no production caller, so the removal is safe.

Ruling on the four decision points: (1) accept the throw-site proxy; if a data-dependent throw ever lands there, the failure mode is a loud reconnect loop rather than a silent seal, and the classifier isolation makes narrowing to an errno allowlist cheap later. (2) Keep the wrapper; the own-property brand plus the traced chain is enough, and the closure latch remains available if drift is ever caught. (3) Hold-forever is the right #715 semantics. (4) The bounded blob leak is acceptable given re-delivery re-saves the same fileIds.

Two things before this leaves draft:

1. Worth a sanity check that a blob-setup hold firing during copy mode composes with the copy-gap cursor banking and blob-repair-on-re-delivery behavior from Bank bulk-copy progress across blob-gap reconnects and repair missing blob files on copy re-delivery #701 and Prevent copy recovery stalls after blob faults #732: the hold path's guarantee is "resume from the durable cursor and re-request", and those changed what the durable cursor means mid-copy.

2. The end-to-end route for the hold behavior itself is currently a proposed follow-up, and our testing guideline wants an executed route in the PR (proposed test plans are exactly what the verification section is defined against). Since the behavior is observable end-to-end and the injection pattern already exists in this file (maybeInjectDecodeFailureForTest covers the skip path; the Sync faults from receiveBlobs setup are classified as permanent decode failures — cursor advances past a recoverable record #715 variant just needs to throw from inside the blob callback so the wrapper participates), I would rather see the e2e land here, or in the fix(test): make the decode-drop and poisoned-cursor cluster suites actually run, pass, and clean up #718 suite work with a cross-link, than as a design-note intention. That also discharges the reproduce-first invariant that the unit seam structurally cannot.

Lavinia, via Claude (I reviewed this before posting)