fix(blob): gate blob unlink on the delete/update committing (#1364) - #1369
Merged
Conversation
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. |
removeEntry deleted a record's blob files up front, before (and independent of) the record removal committing. When the removal didn't actually land — an expiration scan whose transaction is force-committed without the delete, or an aborted/version-conflicted removal — the record survived while its blob files were unlinked, an orphaned reference that wedges replication on ENOENT. Defer the blob deletion until the removal's commit promise resolves (it resolves truthy on commit, false on a conditional-version miss, and rejects on abort), so the unlink only happens once the delete is durable. Falls back to immediate deletion when the store's remove() is synchronous. Can only ever skip an unlink, never lose data; a skipped unlink leaves a reclaimable file (swept by cleanupOrphans). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kriszyp
force-pushed
the
kris/blob-unlink-recheck-1364
branch
from
June 18, 2026 02:04
f89d7a0 to
45cf04b
Compare
kriszyp
marked this pull request as ready for review
June 18, 2026 02:56
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
kriszyp
added a commit
that referenced
this pull request
Jun 19, 2026
The harper-bench host serves harper + harper-pro via a repo-scoped just-in-time supervisor (one job at a time), not an org-level runner — org/enterprise runners aren't routed jobs for these repos. Also merges current main to pick up the blob fixes (#1353, #1364/#1369) the stale branch was missing. Co-Authored-By: Claude Opus 4.8 (1M context) <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
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.
Fixes #1364.
Summary
removeEntrydeleted a record's blob files up front, before (and independent of) the record removal actually committing. The fix defers the blob deletion until the removal's commit promise resolves, so the unlink only happens once the delete is durable.Purpose
Second of the two orphan-producing paths in the JJill preprod replication incident. When the Cache expiration scan's transaction is force-committed without a given delete — or any removal aborts/version-conflicts — the record survives while its blob files are unlinked. That orphaned reference makes a later copy/replication stream hit
sendBlob()ENOENT and stall permanently. (Receiver-side tolerance for a missing blob is #1353; this stops the orphan being created.)Approach
store.remove()returns a promise that resolves when the write commits (truthy on commit,falseon a conditional-version miss) and rejects on abort — the same promise the eviction scan already awaits. SoremoveEntrynow schedules the blob unlink only after that promise resolves committed, and skips it otherwise. Falls back to immediate deletion whenremove()is synchronous (already durable).This can only ever skip an unlink, never lose data; a skipped unlink leaves a reclaimable orphaned file (swept by
cleanupOrphans).Where to look
resources/RecordEncoder.ts—removeEntry. This is the whole change. It works for both the LMDB and RocksDB eviction paths because the removal is queued into the transaction thatevict()later commits, so its promise resolves at that commit — no change toevict()itself.Notes
Table.tsevict()error-handling) — different layers/functions.getEntryre-check). That was abandoned: it raced the commit (skipping legitimate unlinks under load) and a deferredgetEntrycould fire on a closed store and segfault. Gating on the commit promise avoids both. History squashed.agy) leg hung with no output — no second outside-model coverage from Gemini.🤖 Generated by Claude (Opus 4.8, 1M context).