HarperFast / HarperFast/harper-pro

Sync faults from receiveBlobs setup are classified as permanent decode failures — cursor advances past a recoverable record

Open
#715 1 comment 0 reactions 1 assignee Claimed by @heskew View on GitHub
Dominant language
JavaScript
Stars
3
Forks
0
Avg merge
1d 21h
Merged PRs (30d)
80

Description

Surfaced by @kriszyp's post-approval review on #691 (the inline anchors failed, so the findings live in the review body); verified against `main` @ e2d356d0 during post-merge triage.

## Mechanism (verified)

The apply-path decode `try` (`replication/replicationConnection.ts:4856`) wraps `decodeBlobsWithWrites`, whose blob callback invokes `receiveBlobs` **synchronously** (`:4876` → `:5546`). `receiveBlobs` has a substantial synchronous prologue before any promise machinery:

- `createBlobReceiveStream(blobTimeout)` + `blobsInFlight` registration
- `registerBlobReceiveInFlight(blobId, auditStore?.rootStore)` — touches the local store
- `createBlob(stream, remoteBlob)`
- the save is started synchronously: `decodeFromDatabase(() => saveBlob(localBlob).saving, …)`

A synchronous throw from any of these (EMFILE/EIO, directory-creation failure, a store fault during in-flight registration) propagates through `decodeBlobsWithWrites` into the generic catch (`:4881`), where `classifyReplicationDecodeError` (`:1593`) returns `'skip'` — **treated as permanent: `decode-drop` is counted and the resume cursor advances past a recoverable record.**

All of the transient-vs-permanent care — the `hasBlobGap` latch, the #403 unrecoverable-source split, the #683 watchdog — lives in the **async** `.catch` on the save promise and is bypassed entirely by a sync throw. The catch's own comment claims blob faults can't land there ("its transient (503 → hold) vs permanent split is the async receiveBlobs/#403 path, not this catch") — true for blob *content* reads (lazy handles), not for setup faults.

## Ask (per the review)

Narrow permanent skipping to errors proven to originate from the value decoder, or introduce typed classification so blob-setup/local failures **hold and reconnect** instead of dropping the record.

## Related cleanup on the same surface (same review)

`shouldCloseOnRecordDecodeFailure` (`:574`) has zero production callers — its only occurrence in the file is its definition, while its doc comment still claims "the production caller is the inner value-decode catch in `onWSMessage`". Per the review, `unitTests/replication/closeOnInboundMessageError.test.mjs:92-105` still asserts the close-and-reconnect policy production no longer follows, and the surrounding `closeOnInboundMessageError` docs (`:530-537`) are stale. Remove or rework the dead helper and align docs/tests with the shipped disposition when fixing the classification.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.