Repository navigation
Conversation
tart prune deletes immediately, with no way to preview what it will remove. Pruning has previously selected more data than intended (openai#271, openai#549). Introduce a PruningOperation that both the real and preview paths run through, so selection logic is shared rather than duplicated. In preview mode it records each selection instead of deleting, and excludes already-selected entries when pruneSpaceBudget rebuilds its candidate list -- without that, the loop's only source of progress is the delete it no longer performs, and it never terminates. Preview avoids mutating state: storages initialise read-only so they do not create the TART_HOME tree, --gc is skipped, and Root's pre-command garbage collection is bypassed for a dry run. Preview cannot observe stacked-image content being reattributed when an owner is deleted, so it may select different entries than a real run. The command prints that caveat rather than implying the estimate is exact. Closes openai#1295
The first version of --dry-run could list fewer entries than a real prune deletes. Deleting a stacked cache record hands its shared content to another record, pushing that record's charged size up; the preview never saw this because ownership was computed from the records on disk and simulated removals were filtered out afterwards. Under-reporting is the dangerous direction for a preview, and reproduces the problem openai#1295 exists to prevent. Push the simulated removals into the storage instead, via PrunableStorage.prunables(simulatingRemovalOf:), so ownership and content reference scans recompute exactly as they would after a real delete. The protocol default filters by URL, which is correct for IPSW and local VM storage; VMStorageOCI overrides it. Real runs pass an empty set and behave as before. The reclaimed total is now the net drop in usage across the simulation rather than the sum of successive ownerships, so content that only changes owner is not counted twice and content swept by a deletion's garbage collection is. Drop the blanket stacked-image caveat, which no longer applies, and move the remaining notices after the results so an empty preview does not warn about nothing. Those notices now name the two things still outside a dry run: explicit --gc, and Root's temporary-directory cleanup.
Root's temporary-directory cleanup is skipped for every dry run, not just budget ones, so an age-only preview can also omit content that a real prune releases and then collects. The notice was gated on --space-budget. Also stop accumulating estimatedReclaimedBytes in remove(); updateEstimate() overwrites it from the before/after totals and is the only correct source, since summing per-entry sizes double-counts content that changes owner.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6349d30c81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static func pruneSpaceBudget(prunableStorages: [PrunableStorage], spaceBudgetBytes: UInt64) throws { | ||
| static func pruneOlderThan(prunableStorages: [PrunableStorage], olderThanDate: Date, | ||
| operation: PruningOperation = PruningOperation()) throws { | ||
| let prunables: [Prunable] = try prunableStorages.flatMap { try $0.prunables(simulatingRemovalOf: operation.removedURLs) } |
There was a problem hiding this comment.
On filesystems that update directory atimes during enumeration, this dry-run path reaches VMStorageOCI.list() and recursively traverses cached-image directories, advancing the same access dates used for age and LRU selection. The new test at Tests/TartTests/VMStorageOCITests.swift:623-626 explicitly assigns future atimes to work around this behavior rather than verifying they remain unchanged. Consequently, running a preview before the real prune can make previously old entries appear recently accessed and cause the real --older-than or --space-budget run to select a different set, despite the dry-run promise that no changes are made.
Useful? React with 👍 / 👎.
VMStorageOCI.list() recursively enumerated each cached-image record, and on filesystems that update directory atimes during enumeration that advances the very timestamp age and LRU selection read back. A dry run could therefore make old entries look recently accessed and change what the real prune went on to remove -- while promising it had changed nothing. Cache records are leaves in the namespace, so skip their descendants once one is recognised. Recognition, manifest reads and sizing already address record contents by known path rather than by enumeration, so nothing else needs the traversal, and the real scan stops paying for it too. Record selection timestamps are now preserved exactly, to the nanosecond, with no restoration writes. Enumerated ancestor directories and manifest files still acquire atimes; those are not selection inputs. The preview fixtures previously set future access times so enumeration could not disturb their ordering, which encoded the defect as a workaround. They now use explicit past ordering and assert the timestamps are untouched. Those assertions fail against the previous commit.
|
Good catch — this was real, and the future-atime fixtures were me papering over it rather than noticing it. Fixed in e2d446c. I confirmed the mechanism before changing anything: on APFS, enumerating a directory advances that directory's atime (reading a file inside advances the file's, not the directory's), and Cache records are leaves in the namespace, and recognition, manifest reads and sizing already address their contents by known path rather than by enumeration, so To be precise about the limit of the claim: enumerated ancestor directories and manifest files still acquire atimes. Those are not selection inputs, and avoiding them would need read-without-atime support or a separate metadata index. The fixtures now use explicit past ordering and assert the timestamps are untouched. Those assertions fail against 6349d30 with the expected signature — a record's atime coming back as "now" instead of its fixed past value. Separately, while looking at this I think |
tart prunedeletes straight away, with no way to check what it's about to remove first.Since it has twice taken more than intended (#271, #549), a preview seemed worth having.
--dry-runlists what would go, in the order it would go, with sizes and a total, andtouches nothing on disk.
How it works
Preview and real runs share one selection path: a
PruningOperationpassed intopruneOlderThanandpruneSpaceBudgeteither deletes or records what it picked. A separatepreview implementation would drift from the real one and end up lying, so there isn't one.
Two things needed care.
The budget loop only makes progress because each
delete()shrinks the list it rebuilds.Take the delete away and it never finishes. Preview mode therefore remembers what it picked
and feeds that back in on the next rebuild.
Simply filtering those out afterwards isn't enough. Deleting a stacked cache record hands
its shared content to another record, pushing that record's charged size up — which can take
it over budget, so a real run deletes it too. Ownership is computed from the records on disk,
so a preview that filtered its own results never saw this and could list fewer entries than
the real run deletes. That's the dangerous direction for a preview, so the simulated removals
go into the storage instead, via
PrunableStorage.prunables(simulatingRemovalOf:), andownership and content-reference scans recompute exactly as they would after a real delete.
The protocol default filters by URL, which is right for IPSW and local VM storage;
VMStorageOCIoverrides it. Real runs pass an empty set and behave as before.PruneTestsand the existing stacked-content test both pin this: preview and real deletionmust select the same records in the same order.
Reading the numbers
Per-entry sizes are allocated usage when each entry is selected; the total is the net drop in
usage across the simulation. So content that only changes owner isn't double-counted, content
swept by garbage collection is, and where images share content the entries can add up to more
than the total. Sizes use
allocatedSizeBytes(), sincesizeBytes()overstates sparse diskimages — either way an estimate, not a promise of free space.
Keeping the dry run dry
Three paths would otherwise have written to disk:
--gc, the garbage collectionRootrunsbefore every command (so a dry run mutated state before
Prune.run()was even reached), andbuilding the storages, since
Config.validateTartHome()creates directories despite its name.The first two are skipped, the storages take an optional
readOnlyflag, and existing callersplus automatic low-disk pruning are untouched.
A fourth was subtler and came out of review.
VMStorageOCI.list()recursively enumeratedeach cached-image record, and on filesystems that update directory atimes during enumeration
that advances the very timestamp age and LRU selection read back — so a preview could make old
entries look recently accessed and change what the real prune then removed. Cache records are
leaves in the namespace and their contents are already addressed by known path, so the scan now
skips their descendants. Record selection timestamps are preserved exactly, to the nanosecond,
with no restoration writes; the real scan stops paying for the traversal too. Enumerated
ancestor directories and manifest files still acquire atimes, but those are not selection
inputs.
Those skips are the only things a dry run doesn't model, and it says so after the results when
they apply:
--gccan remove untagged records the preview didn't list, and temporary cleanupcan release content pinned by stale temporary manifests, pushing more images over budget.
Tests and checks
New
PruneTests.swiftcovers both criteria, ordering, termination, shared ownership, contentGC, failure paths and that a real run still deletes. The existing stacked-image test now asserts
preview and deletion agree rather than differ. Its fixtures originally set future access times
so enumeration could not disturb their ordering — which encoded the atime defect above as a
workaround — and now use explicit past ordering and assert the timestamps are untouched.
swift buildandswift testpass (143 tests, 4 skipped, 0 failures) and SwiftFormat needs nochanges, run against a throwaway
TART_HOMEwith/usr/sbinonPATHas CI does. The 4 skipsare the existing Docker-dependent tests skipping themselves.
I couldn't run the Go integration suite locally since it boots real VMs, so that's still
outstanding.
Closes #1295