What was wrong: the restore sweeper kept a cached blob while any file in the snapshot database that uses one of its chunks had not been restored. Files outside the requested paths, and files that failed under --skip-errors, never are restored, so every blob they share stayed in $TMPDIR until the restore ended.
What changed: the sweeper now asks the restore plan, through its new pendingFileUsesBlob, whether any pending file still references a cached blob. The plan holds only the files this run restores and drops each one once it is restored or has failed, so it already answers that question. The sweeper's own set of restored files and its database query are gone. The sweeper is now built after the plan, since it needs it.
Test:TestRestoreFreesBlobsNoPendingFileNeeds backs up 15 files plus a copy of each, so every blob is shared with a copy. It then restores only the originals by path, and restores everything with every copy made to fail. Both runs must hold at most one cached blob at a time. The old code holds all three in both cases, whatever order the blobs are downloaded in.
Judgement call: the new test uses 16 KiB files to keep the package's test time down, so setupLocalityBackup now takes the file size. Its existing callers pass the same 1 MiB as before.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/vaultik/issues/324.
**What was wrong:** the restore sweeper kept a cached blob while any file in the snapshot database that uses one of its chunks had not been restored. Files outside the requested paths, and files that failed under `--skip-errors`, never are restored, so every blob they share stayed in `$TMPDIR` until the restore ended.
**What changed:** the sweeper now asks the restore plan, through its new `pendingFileUsesBlob`, whether any pending file still references a cached blob. The plan holds only the files this run restores and drops each one once it is restored or has failed, so it already answers that question. The sweeper's own set of restored files and its database query are gone. The sweeper is now built after the plan, since it needs it.
**Test:** `TestRestoreFreesBlobsNoPendingFileNeeds` backs up 15 files plus a copy of each, so every blob is shared with a copy. It then restores only the originals by path, and restores everything with every copy made to fail. Both runs must hold at most one cached blob at a time. The old code holds all three in both cases, whatever order the blobs are downloaded in.
Judgement call: the new test uses 16 KiB files to keep the package's test time down, so `setupLocalityBackup` now takes the file size. Its existing callers pass the same 1 MiB as before.
Model: opus-5-5
internal/vaultik/restore_plan.go line 242: the new blobNeeded(blobHash) sits next to the existing blobsNeeded(fileID). The two names differ by one letter but answer different questions: blobsNeeded lists the blobs a file still has to download, which never includes a cached blob, while blobNeeded says whether any pending file still uses a blob, cached or not. Someone reading the sweeper (restore_sweeper.go line 57) can easily confuse the two. Acceptable: a name that says what it checks, such as pendingFileUsesBlob.
Model: opus-5-5
**`internal/vaultik/restore_plan.go` line 242:** the new `blobNeeded(blobHash)` sits next to the existing `blobsNeeded(fileID)`. The two names differ by one letter but answer different questions: `blobsNeeded` lists the blobs a file still has to download, which never includes a cached blob, while `blobNeeded` says whether any pending file still uses a blob, cached or not. Someone reading the sweeper (`restore_sweeper.go` line 57) can easily confuse the two. Acceptable: a name that says what it checks, such as `pendingFileUsesBlob`.
Model: opus-5-5
A restore of selected paths, or one where files failed under
--skip-errors, kept the blobs those files share in the temporary cache
until it ended: the sweeper freed a blob only once every file in the
snapshot database that uses it had been restored, and files outside the
selection or that failed never are.
The sweeper now asks the restore plan, through pendingFileUsesBlob,
whether any pending file still uses a cached blob. The plan holds only
this run's files and drops each one once it is restored or has failed.
The new test backs up 16 KiB files so it stays quick; the locality
backup helper takes the file size for that.
Model: opus-5-5
restore_plan.go: blobNeeded is renamed pendingFileUsesBlob, and its comment now says it counts a blob whether or not it is cached; the sweeper calls it by the new name.
Rebased onto current next; commit message and PR body updated to match.
Model: opus-5-5
Rework:
- `restore_plan.go`: `blobNeeded` is renamed `pendingFileUsesBlob`, and its comment now says it counts a blob whether or not it is cached; the sweeper calls it by the new name.
- Rebased onto current `next`; commit message and PR body updated to match.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #324.
What was wrong: the restore sweeper kept a cached blob while any file in the snapshot database that uses one of its chunks had not been restored. Files outside the requested paths, and files that failed under
--skip-errors, never are restored, so every blob they share stayed in$TMPDIRuntil the restore ended.What changed: the sweeper now asks the restore plan, through its new
pendingFileUsesBlob, whether any pending file still references a cached blob. The plan holds only the files this run restores and drops each one once it is restored or has failed, so it already answers that question. The sweeper's own set of restored files and its database query are gone. The sweeper is now built after the plan, since it needs it.Test:
TestRestoreFreesBlobsNoPendingFileNeedsbacks up 15 files plus a copy of each, so every blob is shared with a copy. It then restores only the originals by path, and restores everything with every copy made to fail. Both runs must hold at most one cached blob at a time. The old code holds all three in both cases, whatever order the blobs are downloaded in.Judgement call: the new test uses 16 KiB files to keep the package's test time down, so
setupLocalityBackupnow takes the file size. Its existing callers pass the same 1 MiB as before.Model: opus-5-5
internal/vaultik/restore_plan.goline 242: the newblobNeeded(blobHash)sits next to the existingblobsNeeded(fileID). The two names differ by one letter but answer different questions:blobsNeededlists the blobs a file still has to download, which never includes a cached blob, whileblobNeededsays whether any pending file still uses a blob, cached or not. Someone reading the sweeper (restore_sweeper.goline 57) can easily confuse the two. Acceptable: a name that says what it checks, such aspendingFileUsesBlob.Model: opus-5-5
3204bef594to56fc99d1ab56fc99d1abtob559bb1b3cRework:
restore_plan.go:blobNeededis renamedpendingFileUsesBlob, and its comment now says it counts a blob whether or not it is cached; the sweeper calls it by the new name.next; commit message and PR body updated to match.Model: opus-5-5
Review passed.
Model: opus-5-5