HarperFast / HarperFast/harper

Blob reclamation cancelled at encode time leaks the file when the write aborts

Open
#2,156 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

## Gap

`cancelBlobReclamation` (added in #2145) drops a queued blob reclamation when a record version being written references the file again. It runs during msgpackr encoding — **before** the write commits — so an aborted transaction leaves the file both unreferenced and unqueued.

Sequence:

1. Write A supersedes blob `F`. On commit, `F` is queued for reclamation.
2. Write B encodes a record that references `F` again. `cancelBlobReclamation` removes the queued entry at encode time.
3. B aborts. `F` is referenced by no live record (A's supersession stands) and is no longer queued for reclamation.

`F` then survives until someone runs `cleanup_orphan_blobs`. No data is lost — this is a reclaimable leak, not a dangling reference — but the file is retained indefinitely without operator action.

## Why it was left this way

Cancelling at commit instead of at encode would close the leak but open a worse window: between encode and commit, the reclaimer could unlink `F` while B is committing a record that points at it, producing exactly the dangling reference #2145 exists to prevent. Given the choice between a reclaimable leak on abort and a dangling reference on commit, #2145 took the leak.

## Suggested fix

Make the cancellation transactional rather than speculative: have the encoder record which reclamations it cancelled for a given write, and re-queue them from the transaction's abort path. That mirrors how newly-written blobs are already handled on abort (`cleanupUnusedBlobs(write.savedBlobs, …)` in `LMDBTransaction.ts:277`/`:323`), so the lifecycle hook already exists — this is plumbing from the encoder into it, not a new mechanism.

Re-queueing (rather than unlinking directly) is the safe direction: the file returns to the normal reclamation path, where the snapshot watermark, holds, and retention window all still apply before anything is unlinked.

## Scope

- Narrow: requires a write that re-references a blob already superseded by an earlier committed write, and that write then aborting.
- No data-loss risk in either the current behavior or the fix.
- Test shape: supersede a blob, begin a write that re-references it, abort, then assert the file is reclaimed without an orphan sweep.

## Related

- #2145 — the PR that introduced the cancellation (see `cancelBlobReclamation` in `resources/blob.ts` and its docstring, which records this trade-off).
- #1364 — the same class of "only act once the write commits" reasoning, applied to `removeEntry`.

🤖 Generated by Claude (claude-opus-5)

Contributor guide

Open the contributing guide

Research direction

Start with cancelBlobReclamation and its docstring in resources/blob.ts, then trace how the encoder's cancelled reclamations can reach LMDBTransaction.ts abort handling, especially cleanupUnusedBlobs at lines 277 and 323. Add the regression test described in the issue: supersede a blob, re-reference it in a write, abort, and verify it is reclaimed without an orphan sweep.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
backend, databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.