Skip to content

feat(packstore): reclaim unreferenced loose files before packing - #315

Merged
wesm merged 5 commits into
mainfrom
packstore-sweep-orphans
Nov 6, 2026
Merged

feat(packstore): reclaim unreferenced loose files before packing#315
wesm merged 5 commits into
mainfrom
packstore-sweep-orphans

Conversation

@wesm

@wesm wesm commented Nov 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

A store whose maintenance runs under a short time budget can keep unreferenced loose files on disk indefinitely. Pack removes orphans only at its very end, after verification and packing, so when those steps use up the budget every pass, a large abandoned upload is never reclaimed.

Maintainer.SweepOrphans runs only the orphan removal. A caller can run it first, with its own budget, and get the space back whether or not Pack finishes:

stats, err := maintainer.SweepOrphans(ctx)
// stats.LooseOrphansRemoved, stats.LooseOrphansDeferred, stats.Suppressed

It shares its removal code (removeLooseOrphans) with Pack, so both follow the same rules:

  • It holds the maintenance lease, so it waits for in-flight writes. A write publishes its file before the catalog references it, so sweeping during a write could delete live content.
  • It removes nothing, and reports Suppressed, when the reference inventory is incomplete.
  • It checks cancellation before each file, so an expired budget does not keep the lease while the rest of a large shard is deleted.
  • An orphan the store has no permission to remove is skipped and counted in LooseOrphansDeferred instead of failing the sweep. Before this change, one such file in an early shard blocked every orphan after it, and failed Pack at its end, on every pass. PackStats reports the same count.
  • On a new store whose root does not exist yet, there is nothing to sweep, so it returns success. Callers can run it before the first Pack.

It does not verify, pack, or repack, so it costs much less than Pack.

🤖 Generated with Claude Code

@wesm @claude
feat(packstore): reclaim unreferenced loose files before packing
8a48a2c 
A store whose maintenance runs under a short time budget can keep
unreferenced loose files on disk for as long as verification or packing
uses up that budget. `Pack` removes orphans only at its end, so a large
abandoned upload can stay on disk across every pass.

`SweepOrphans` runs only that removal. A caller can run it first, with its
own budget, and reclaim space whether or not `Pack` finishes. It keeps the
same safety rules as `Pack`: it waits for in-flight writes through the
maintenance lease, and it removes nothing when the reference inventory is
incomplete.

Generated with Claude Code (claude-opus-5-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (8a48a2c)

Verdict: Changes require fixes for 1 finding.

Medium

  • packstore/pack.go:1187: If SweepOrphans' context expires during deletion within a large shard, the loop continues processing the entire shard, exceeding the caller's time budget and blocking application mutations under the exclusive maintenance lease. Cancellation during the final shard can also return success.

    Fix: Check ctx.Err() before processing each file. Add a regression test that cancels during deletion and verifies subsequent files remain untouched.

Reviewers: 3x codex, codex (security), codex (test-value) | Synthesis: codex, 9s | Total: 3m45s

@wesm @claude
fix(packstore): stop an orphan sweep inside a shard when canceled
561d509 
A sweep holds the exclusive maintenance lease while it deletes, so
application writes wait until it returns. It checked its context only
between shard directories. A canceled or expired sweep therefore kept
deleting through the rest of a large shard and held the lease past the
caller's budget. When this happened in the last shard, the sweep also
reported success, so the caller could not tell that it had stopped late.

The sweep now checks its context before each file. Orphan removal also
uses the maintainer's identity pin opener, like the other maintenance
removals, so a test can cancel the context in the middle of a deletion.

Generated with Claude Code (claude-opus-5-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (561d509)

Verdict: No findings at or above medium severity.

Reviewers: 3x codex, codex (security), codex (test-value) | Synthesis: codex | Total: 3m14s

@wesm wesm self-assigned this Nov 5, 2026
@wesm @claude
fix(packstore): keep an orphan sweep working on new and partly locked…
2bad2de 
… stores

The sweep is meant to run before Pack on every maintenance pass. On a
new store that has had no writes, the storage root does not exist yet.
Pack creates the root, but the sweep failed on it, so a caller that
followed the documented order got an error on every pass until the
first write. A missing root now means there is nothing to sweep.

One orphan that the store has no permission to remove, such as a file
in a read-only shard directory, stopped the whole sweep. Shards are
visited in a fixed order, so that one file blocked reclaiming every
orphan after it on every pass, and Pack failed at its end for the same
reason. The sweep now skips such a file, as the redundant-copy sweep
already does, and reports it in LooseOrphansDeferred so the caller
can see that space was left behind.

SweepStats becomes OrphanSweepStats so that it is not confused with
PackStats.LooseSwept, which counts packed copies removed from loose
storage.

Generated with Claude Code (claude-opus-5-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (2bad2de)

Verdict: Changes require fixes for 1 finding.

Medium

  • packstore/pack.go:1219: Permission failures while acquiring an orphan's identity/removal pin propagate without errLooseRemovalUnavailable, causing SweepOrphans and Pack to abort before reaching later removable orphans. This affects macOS fallback pin-directory creation and Windows pins requiring DELETE access; current tests cover permission denial only after pin acquisition.

    Fix: Treat permission-denied pin acquisition as a deferred orphan and continue sweeping, while preserving fatal identity and cleanup errors. Add regression coverage verifying that pin-acquisition permission failure is deferred and a later removable orphan is reclaimed.

Reviewers: 3x codex, codex (security), codex (test-value) | Synthesis: codex, 13s | Total: 3m23s

@wesm @claude
fix(packstore): defer orphans whose removal pin is denied
c3fb1ed 
The orphan sweep skipped a file it lacked permission to remove only when
the denial came after the sweep pinned the file. On macOS, an unreadable
file needs a new pin directory in its shard. On Windows, the pin needs
delete access. Either can be denied first, and the sweep then stopped,
so one such file still blocked every orphan after it. Pack failed at its
end for the same reason.

The sweep now defers a permission denial from pin acquisition too. It
checks for permission errors only at that step. A permission error after
pinning can come with failed cleanup, and that case still stops the
sweep so the caller sees it.

Generated with Claude Code (claude-opus-5-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (c3fb1ed)

Verdict: Changes require fixes for 1 finding.

Medium

  • packstore/sweep_test.go:97: Both cases in TestSweepOrphansContinuesPastAnOrphanItCannotRemove visit the removable orphan (hash prefix e5) before the blocked orphan (ec) because shard names are sorted. An implementation that returns success immediately after counting the denied orphan would pass all assertions without proving that later orphans are reclaimed.

    Fix: Make the blocked orphan sort before the removable orphan; swapping their payloads suffices. Retain both cases and their assertions to cover pin-acquisition and removal-claim failures.

Reviewers: 3x codex, codex (security), codex (test-value) | Synthesis: codex, 9s | Total: 3m51s

@wesm @claude
test(packstore): deny the first orphan the sweep visits
30467e4 
The test for a sweep that cannot remove one orphan denied the orphan
that the sweep visited second. A sweep that stopped right after the
denied file therefore still passed, so the test did not show that the
sweep goes on to reclaim later orphans.

The test now denies whichever orphan sorts first, because the sweep
visits file names in sorted order. Picking by order, not by fixed
content, keeps the test valid if its contents change.

Generated with Claude Code (claude-opus-5-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roborev-ci

roborev-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

roborev: Combined Review (30467e4)

Verdict: No findings at or above medium severity.

Reviewers: 3x codex, codex (security), codex (test-value) | Synthesis: codex | Total: 3m12s

@wesm
wesm merged commit ad6c7ed into main Nov 6, 2026
15 checks passed
@wesm
wesm deleted the packstore-sweep-orphans branch November 6, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@wesm wesm

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@wesm