fix(blob): clean up replication-received blobs when a source apply is skipped (harper-pro#406) - #1341
Merged
Merged
Conversation
… skipped On a replication/source apply, the record's blobs are saved out-of-band by receiveBlobs BEFORE the apply runs. If that apply then loses a version conflict (precedesExistingVersion <= 0 -> write.skipped), the received blob was never unlinked: startPreCommitBlobsForRecord only tracked saveBeforeCommit blobs in write.savedBlobs, so the skip/abort cleanup had nothing to remove. On an active multi-node caching cluster these orphans accumulate (observed: ~1000+ true orphan blobs on one preprod node). See harper-pro#406. - startPreCommitBlobsForRecord: new `trackPersistedBlobs` flag — also track an already-saved (fileId-set) blob, but ONLY when set. It's passed as options?.isNotification from _writeUpdate, so only source/replication applies track already-saved blobs. A local write carrying an already-saved blob references another row's blob (cross-row reuse, which pack() rejects, or a same-key unchanged-attribute update), and must NOT have it unlinked on abort/skip — gating to the source-apply path avoids that data-loss hazard. - cleanupUnusedBlobs(blobs, retainedFileIds?): never deletes a blob whose fileId the committed (winning) record still references. The skip/abort cleanup sites in LMDBTransaction and DatabaseTransaction pass collectRetainedFileIds(write.store.getEntry(write.key)?.value) as that guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Reviewed; no blockers found. |
kriszyp
marked this pull request as ready for review
June 17, 2026 05:48
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Member
Author
|
CI note: every check is green except Integration Tests 4/6 (Windows, Node.js v24), which has been persistently slow/stuck on this PR — it hung ~19 min on the first run, and the fresh-runner re-run is again running long while all other integration shards (Bun, Node v22/v24/v26, and the other 5 Windows shards) and the full unit suite pass. This looks like Windows-runner flakiness on that one shard, not the change (the diff is core blob/transaction logic with no platform-specific behavior; the earlier v24 unit segfault was also a confirmed flake that passed on re-run). Flagging so it doesn't block review — re-trigger that shard if it doesn't settle. — Claude (Opus 4.8) |
kriszyp
added a commit
that referenced
this pull request
Jun 18, 2026
…complete() (#1341 regression) (#1376) startPreCommitBlobsForRecord with trackPersistedBlobs=true (added in #1341 for orphan-blob cleanup) added already-saving blobs (fileId set) to blobsNeedingSaving, so complete() awaited their in-flight save promises before the commit ran. This creates a deadlock in backlog-recovery catch-up: 1. Many catch-up records arrive quickly; outstandingCommits exceeds MAX_OUTSTANDING_COMMITS (150), causing ws.pause() in replicationConnection.ts. 2. Partially-received blob streams (chunks still in kernel/WS buffer) can't receive more data while the WS is paused. 3. After blobTimeout (120s), those streams time out → hasBlobGap=true → complete() rejects → commit fails → onCommit() never fires → outstandingCommits never decrements → WS stays paused forever. Fix: split the tracking into blobsNeedingSaving (awaited in complete()) and blobsToTrackOnly (tracked for cleanup only, not awaited). Already-saving blobs go into blobsToTrackOnly. Their durability is already tracked by outstandingBlobsToFinish in replicationConnection.ts — blocking the commit on them here is redundant and dangerous. The orphan-blob cleanup from #1341 (harper-pro#406) is preserved: all blobs (both lists) end up in write.savedBlobs via allTrackedBlobs. Fixes backlog-recovery catch-up stall in CI (B stalls at ~315/800 for the full convergence window). See harper-pro#414. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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
Fixes a replicated-blob orphan leak (harper-pro#406). On a replication/source apply, the record's blobs are saved out-of-band by
receiveBlobsbefore the apply runs. If that apply then loses a version conflict (precedesExistingVersion <= 0→write.skipped), the received blob was never unlinked —startPreCommitBlobsForRecordonly trackedsaveBeforeCommitblobs inwrite.savedBlobs, so the skip/abort cleanup had nothing to remove. On an active multi-node caching cluster these accumulate: an audit-awarecleanup_orphan_blobssweep on one preprod node reclaimed 1059 true-orphan blobs (~84% of its blob files).Changes
resources/blob.tsstartPreCommitBlobsForRecord(record, store, saveInRecord?, trackPersistedBlobs?): whentrackPersistedBlobsis set, also track an already-saved (fileId-set) blob so the skip/abort cleanup can unlink it. Gated deliberately (see below).cleanupUnusedBlobs(blobs, retainedFileIds?): never deletes a blob whose fileId the committed (winning) record still references.collectRetainedFileIds(record)helper.resources/Table.ts:preCommitBlobsForRecordBeforeforwards the flag; the_writeUpdatecall site passesoptions?.isNotification.resources/LMDBTransaction.ts(×2) andresources/DatabaseTransaction.ts(×3): the skip/abort cleanup sites passcollectRetainedFileIds(write.store.getEntry(write.key)?.value).Where to look
isNotification). A received blob is owned by that one write (a unique fileId saved for it). A local write that carries an already-saved blob instead references another row's blob — and deleting that on abort/skip would corrupt the owning record. (Cross-model review caught exactly this in the first cut, where tracking was unconditional; this gating + the retained guard is the fix.)pack()already rejects reusing a blob across records, but the rejection throws aftersavedBlobsis populated, so the resulting abort would have triggered the bad delete — hence gating, not just relying on the guard.Tests
unitTests/resources/blob.test.js: 3 new tests —trackPersistedBlobsgating, theretainedFileIdsguard (deletes non-retained, keeps retained), andcollectRetainedFileIds. Existing "updating an unrelated attribute does not unlink a still-referenced blob" still passes.unitTests/resources/**suite: 815 passing, 0 failing. Lint clean.Review provenance / caveats
isNotificationgating above; Codex re-reviewed the revision and confirmed the blocker is resolved with no new findings.agy) leg timed out/produced no output both rounds, and the worktree-isolatedreviewersubagent could not launch (harnessWorktreeCreatehook error). I did the Harper-domain pass myself instead. So this change has had Codex + author domain review but not the Gemini or Opus-reviewer legs — worth a careful human data-integrity pass.🤖 Generated by Claude (Opus 4.8). Fix for harper-pro#406.