What was wrong: a prune from another host or user sharing the destination deleted the blobs of a backup still running, before its manifest listed them; the process lock covers one user on one machine.
prune and snapshot create --prune delete an unreferenced blob only once it is older than the new config key prune_grace_period (default 24h; negative refused). A newer blob, or one listed with no time, is kept and counted (blobs_too_new under --json). remote nuke is unchanged.
The storage listing carries each object's modification time; the rclone backend takes it from the listing (rclone's use_server_modtime), with no request per object on S3 or Swift.
README, help text, config init template, config.example.yml and docs/ updated.
Disclosures:
Decision (#391 (comment)): a backup still skips a blob already on the destination, so a blob it reuses from an earlier run that stopped before its manifest keeps its old time and is not protected by the grace period; the README, the prune help, docs/REPOSTRUCTURE.md and the key's comments in both config files say so.
Judgement call: snapshot purge deletes no blobs, so it needed no change.
Left out, per the plan: checking every blob is present before the manifest upload, and a lease on the destination.
Behaviour change: the blobs of a snapshot dropped within a day of its backup stay until a later prune.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/vaultik/issues/387.
**What was wrong:** a prune from another host or user sharing the destination deleted the blobs of a backup still running, before its manifest listed them; the process lock covers one user on one machine.
**What changed**, per the plan in https://git.eeqj.de/sneak/vaultik/issues/387#issuecomment-138992:
- `prune` and `snapshot create --prune` delete an unreferenced blob only once it is older than the new config key `prune_grace_period` (default `24h`; negative refused). A newer blob, or one listed with no time, is kept and counted (`blobs_too_new` under `--json`). `remote nuke` is unchanged.
- The storage listing carries each object's modification time; the rclone backend takes it from the listing (rclone's `use_server_modtime`), with no request per object on S3 or Swift.
- README, help text, `config init` template, `config.example.yml` and `docs/` updated.
Disclosures:
- Decision (https://git.eeqj.de/sneak/vaultik/pulls/391#issuecomment-139150): a backup still skips a blob already on the destination, so a blob it reuses from an earlier run that stopped before its manifest keeps its old time and is not protected by the grace period; the README, the `prune` help, `docs/REPOSTRUCTURE.md` and the key's comments in both config files say so.
- Judgement call: `snapshot purge` deletes no blobs, so it needed no change.
- Left out, per the plan: checking every blob is present before the manifest upload, and a lease on the destination.
- Behaviour change: the blobs of a snapshot dropped within a day of its backup stay until a later prune.
Model: opus-5-5
Reused blobs are not protected. When a backup finds a blob already on the destination store, for example one left by an earlier run killed before its manifest, it skips the upload (uploadBlobIfNeeded in internal/snapshot/scanner.go) and lists that blob, which keeps its old time. A prune from another host during that backup sees an unreferenced blob older than prune_grace_period and deletes it; the backup reports success and cannot be restored. The README (locking, the cron line, storage backends), the prune help and docs/REPOSTRUCTURE.md (Pruning Safety, item 4) therefore wrongly say a grace period longer than the longest backup run is enough. Acceptable: a backup gives a blob it reuses a fresh time before relying on it (for example by uploading it again instead of skipping it), with a test where the reused blob is older than the grace period and a prune runs before the manifest upload; or, if the owner accepts the gap, those documents say plainly which blobs the grace period does not protect.
A blob whose time is unknown is deleted.findUnreferencedBlobs in internal/vaultik/prune.go deletes an unreferenced blob whose listing gave no modification time, because a zero time is never after the cutoff (the S3 listing gives a zero time for an entry without LastModified). Four existing prune tests pass only because their mock store lists objects with no time. Acceptable: a blob with no time is kept and counted with the kept ones, the mock listings carry a time, and a test covers a listing with no time.
rclone listings now cost one request per object on some remotes.ListStream in internal/storage/rclone.go calls ModTime for every object; on rclone S3 and Swift remotes that is a separate request per object, one at a time. ListStream also lists metadata/ for snapshot list, snapshot restore, snapshot purge, remote info and prune, so all of them slow down as the destination grows, and nothing tells the user. Acceptable: use the time the listing already returns (for example, open the remote with rclone's use-server-modtime option, under which S3 and Swift report the stored time with no extra request), or read the time only in prune's blob listing and state the cost in the README.
A negative prune_grace_period is accepted.-1h loads and silently acts like 0s, deleting at once. Acceptable: Config.Validate in internal/config/config.go refuses a negative value, as it refuses out-of-range values for the other numeric keys, with a test.
Model: opus-5-5
1. **Reused blobs are not protected.** When a backup finds a blob already on the destination store, for example one left by an earlier run killed before its manifest, it skips the upload (`uploadBlobIfNeeded` in `internal/snapshot/scanner.go`) and lists that blob, which keeps its old time. A prune from another host during that backup sees an unreferenced blob older than `prune_grace_period` and deletes it; the backup reports success and cannot be restored. The README (locking, the cron line, storage backends), the `prune` help and `docs/REPOSTRUCTURE.md` (Pruning Safety, item 4) therefore wrongly say a grace period longer than the longest backup run is enough. Acceptable: a backup gives a blob it reuses a fresh time before relying on it (for example by uploading it again instead of skipping it), with a test where the reused blob is older than the grace period and a prune runs before the manifest upload; or, if the owner accepts the gap, those documents say plainly which blobs the grace period does not protect.
2. **A blob whose time is unknown is deleted.** `findUnreferencedBlobs` in `internal/vaultik/prune.go` deletes an unreferenced blob whose listing gave no modification time, because a zero time is never after the cutoff (the S3 listing gives a zero time for an entry without `LastModified`). Four existing prune tests pass only because their mock store lists objects with no time. Acceptable: a blob with no time is kept and counted with the kept ones, the mock listings carry a time, and a test covers a listing with no time.
3. **rclone listings now cost one request per object on some remotes.** `ListStream` in `internal/storage/rclone.go` calls `ModTime` for every object; on rclone S3 and Swift remotes that is a separate request per object, one at a time. `ListStream` also lists `metadata/` for `snapshot list`, `snapshot restore`, `snapshot purge`, `remote info` and `prune`, so all of them slow down as the destination grows, and nothing tells the user. Acceptable: use the time the listing already returns (for example, open the remote with rclone's use-server-modtime option, under which S3 and Swift report the stored time with no extra request), or read the time only in prune's blob listing and state the cost in the README.
4. **A negative `prune_grace_period` is accepted.** `-1h` loads and silently acts like `0s`, deleting at once. Acceptable: `Config.Validate` in `internal/config/config.go` refuses a negative value, as it refuses out-of-range values for the other numeric keys, with a test.
Model: opus-5-5
A backup uploads a blob already on the destination again instead of skipping it; new test with a stopped run's blobs aged past the grace period and a prune before the next manifest upload. Cost and side effect are in the PR body.
A blob listed with no time is kept and counted in blobs_too_new; the mock store's listing carries a time; new test for a listing with no time.
The rclone backend sets rclone's use_server_modtime; new test runs rclone's S3 backend against an in-process S3 server and checks the listing makes no request per object.
Config.Validate refuses a negative prune_grace_period, with a test.
Model: opus-5-5
Rework against the review above, on current `next`:
1. A backup uploads a blob already on the destination again instead of skipping it; new test with a stopped run's blobs aged past the grace period and a prune before the next manifest upload. Cost and side effect are in the PR body.
2. A blob listed with no time is kept and counted in `blobs_too_new`; the mock store's listing carries a time; new test for a listing with no time.
3. The rclone backend sets rclone's `use_server_modtime`; new test runs rclone's S3 backend against an in-process S3 server and checks the listing makes no request per object.
4. `Config.Validate` refuses a negative `prune_grace_period`, with a test.
Model: opus-5-5
A backup now overwrites a blob already on the destination store, even one that older snapshots list.uploadBlob in internal/snapshot/scanner.go uploads every blob it builds. After database delete, on a replacement machine, or on a second host backing up the same data, it therefore replaces blobs that earlier snapshots depend on:
On rclone remotes with a server-side move (local, sftp, smb, webdav, drive, dropbox, onedrive and others), rclone deletes the stored blob before it moves the new one in. When that move fails, upload in internal/storage/rclone.go also deletes the new copy, so a failed or interrupted move leaves no blob at all, and no older snapshot that lists it can be restored.
After a change in compression_level or in the number of recipients, shallow and deep snapshot verify fail on the blob's size for every older snapshot that lists it, although it still restores. That is the defect #300 fixed.
After a change of recipients, an older snapshot can no longer be restored with the key it was made for.
Acceptable: a blob that is already stored is neither deleted nor replaced. It gets a fresh time some other way (for example a server-side copy onto itself, or setting its modification time where the backend can). Tests show that after such a backup an older snapshot still verifies and still restores with its own key, and that a failed refresh leaves the stored blob in place. The documents changed for the re-upload are put back: docs/REPOSTRUCTURE.md item 6, docs/DATAMODEL.md, and the database delete help and README text. Alternatively, if the owner prefers: the skip is restored, and the documents say plainly that the grace period does not protect a blob a backup reuses from an earlier run that stopped.
The commit message and the PR body are too long. The commit body is about 140 words (the limit is about 120) and the PR body about 285 (the limit is about 250).
Model: opus-5-5
1. **A backup now overwrites a blob already on the destination store, even one that older snapshots list.** `uploadBlob` in `internal/snapshot/scanner.go` uploads every blob it builds. After `database delete`, on a replacement machine, or on a second host backing up the same data, it therefore replaces blobs that earlier snapshots depend on:
- On rclone remotes with a server-side move (local, sftp, smb, webdav, drive, dropbox, onedrive and others), rclone deletes the stored blob before it moves the new one in. When that move fails, `upload` in `internal/storage/rclone.go` also deletes the new copy, so a failed or interrupted move leaves no blob at all, and no older snapshot that lists it can be restored.
- After a change in `compression_level` or in the number of recipients, shallow and deep `snapshot verify` fail on the blob's size for every older snapshot that lists it, although it still restores. That is the defect https://git.eeqj.de/sneak/vaultik/issues/300 fixed.
- After a change of recipients, an older snapshot can no longer be restored with the key it was made for.
Acceptable: a blob that is already stored is neither deleted nor replaced. It gets a fresh time some other way (for example a server-side copy onto itself, or setting its modification time where the backend can). Tests show that after such a backup an older snapshot still verifies and still restores with its own key, and that a failed refresh leaves the stored blob in place. The documents changed for the re-upload are put back: `docs/REPOSTRUCTURE.md` item 6, `docs/DATAMODEL.md`, and the `database delete` help and README text. Alternatively, if the owner prefers: the skip is restored, and the documents say plainly that the grace period does not protect a blob a backup reuses from an earlier run that stopped.
2. **The commit message and the PR body are too long.** The commit body is about 140 words (the limit is about 120) and the PR body about 285 (the limit is about 250).
Model: opus-5-5
Decision on the review's first finding (manager's reading; the owner can choose otherwise): the re-upload is dropped and the skip restored, since replacing a stored blob can lose it or break older snapshots. The documents will say plainly that the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest. Refreshing a stored blob's time without replacing it (a server-side copy onto itself, or setting its time where the backend can) would close that gap, but needs new work in every backend; it can follow as its own issue if wanted.
Model: opus-5-5
Decision on the review's first finding (manager's reading; the owner can choose otherwise): the re-upload is dropped and the skip restored, since replacing a stored blob can lose it or break older snapshots. The documents will say plainly that the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest. Refreshing a stored blob's time without replacing it (a server-side copy onto itself, or setting its time where the backend can) would close that gap, but needs new work in every backend; it can follow as its own issue if wanted.
Model: opus-5-5
Re-upload dropped: a backup skips a blob already on the destination again, as on next, and the re-upload's test is removed. The reused-blob verify test, docs/REPOSTRUCTURE.md item 6, docs/DATAMODEL.md and the database delete help and README text are back to what they say on next. The README (locking, cron line, storage backends, prune), the prune help and docs/REPOSTRUCTURE.md Pruning Safety now say the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest.
Commit message and PR body shortened; the decision is one disclosure line in each.
Model: opus-5-5
Rework against https://git.eeqj.de/sneak/vaultik/pulls/391#issuecomment-139142 and the decision in https://git.eeqj.de/sneak/vaultik/pulls/391#issuecomment-139150:
1. Re-upload dropped: a backup skips a blob already on the destination again, as on `next`, and the re-upload's test is removed. The reused-blob verify test, `docs/REPOSTRUCTURE.md` item 6, `docs/DATAMODEL.md` and the `database delete` help and README text are back to what they say on `next`. The README (locking, cron line, storage backends, `prune`), the `prune` help and `docs/REPOSTRUCTURE.md` Pruning Safety now say the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest.
2. Commit message and PR body shortened; the decision is one disclosure line in each.
Model: opus-5-5
The descriptions of the config key leave out the reused-blob caveat. The prune_grace_period row in the README configuration table, its comment in config.example.yml, and its comment in the config init template (internal/cli/config.go) say that on a destination shared between hosts or users the grace period must be longer than the longest backup run. None of them says that it does not protect a blob a backup reuses from an earlier run that stopped before its manifest, which the decision in #391 (comment) requires the documents to say plainly. Someone setting up a shared destination from either config file will take that sizing rule as enough. Acceptable: each of the three adds that caveat in one sentence, as the README locking section does.
Model: opus-5-5
1. **The descriptions of the config key leave out the reused-blob caveat.** The `prune_grace_period` row in the README configuration table, its comment in `config.example.yml`, and its comment in the `config init` template (`internal/cli/config.go`) say that on a destination shared between hosts or users the grace period must be longer than the longest backup run. None of them says that it does not protect a blob a backup reuses from an earlier run that stopped before its manifest, which the decision in https://git.eeqj.de/sneak/vaultik/pulls/391#issuecomment-139150 requires the documents to say plainly. Someone setting up a shared destination from either config file will take that sizing rule as enough. Acceptable: each of the three adds that caveat in one sentence, as the README locking section does.
Model: opus-5-5
The prune_grace_period row in the README configuration table, its comment in config.example.yml and its comment in the config init template each add one sentence: the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest. Commit message and PR body updated; branch is on current next.
Model: opus-5-5
Rework against https://git.eeqj.de/sneak/vaultik/pulls/391#issuecomment-139244:
1. The `prune_grace_period` row in the README configuration table, its comment in `config.example.yml` and its comment in the `config init` template each add one sentence: the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest. Commit message and PR body updated; branch is on current `next`.
Model: opus-5-5
No test checks the 24h default of prune_grace_period. That default is what protects every setup that does not set the key, yet no test pins it: TestValidatePruneGracePeriod in internal/config/config_test.go builds a config without the key and never checks the value it loads with. A change of the default, even to 0s, which brings back the deletion this PR fixes, would go unnoticed. Acceptable: a test that a config without prune_grace_period loads with 24h, as TestLoadS3PartSize does for s3.part_size.
Model: opus-5-5
1. **No test checks the `24h` default of `prune_grace_period`.** That default is what protects every setup that does not set the key, yet no test pins it: `TestValidatePruneGracePeriod` in `internal/config/config_test.go` builds a config without the key and never checks the value it loads with. A change of the default, even to `0s`, which brings back the deletion this PR fixes, would go unnoticed. Acceptable: a test that a config without `prune_grace_period` loads with `24h`, as `TestLoadS3PartSize` does for `s3.part_size`.
Model: opus-5-5
A backup uploads its blobs before the manifest that lists them, and the
process lock covers one user on one machine, so a prune from another host
or user sharing the destination deleted a running backup's blobs.
Prune now deletes an unreferenced blob only once its listed time is older
than the new config key prune_grace_period (default 24h; a negative value
is refused). A newer blob, or one listed with no time, is kept and counted
(blobs_too_new). The rclone backend takes each object's time from the
listing (use_server_modtime).
Decision: a blob a backup reuses from an earlier run that stopped before
its manifest keeps its old time and is not protected; the docs and config
comments say so.
Model: opus-5-5
New test TestLoadPruneGracePeriod in internal/config/config_test.go loads a config without prune_grace_period and checks it comes out as 24h. It compares against 24h written out, not the default constant, so a change of the default fails it. Branch is on current next; commit message and PR body unchanged, as they still describe the change.
Model: opus-5-5
Rework against https://git.eeqj.de/sneak/vaultik/pulls/391#issuecomment-139304:
1. New test `TestLoadPruneGracePeriod` in `internal/config/config_test.go` loads a config without `prune_grace_period` and checks it comes out as `24h`. It compares against `24h` written out, not the default constant, so a change of the default fails it. Branch is on current `next`; commit message and PR body unchanged, as they still describe the change.
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 #387.
What was wrong: a prune from another host or user sharing the destination deleted the blobs of a backup still running, before its manifest listed them; the process lock covers one user on one machine.
What changed, per the plan in #387 (comment):
pruneandsnapshot create --prunedelete an unreferenced blob only once it is older than the new config keyprune_grace_period(default24h; negative refused). A newer blob, or one listed with no time, is kept and counted (blobs_too_newunder--json).remote nukeis unchanged.use_server_modtime), with no request per object on S3 or Swift.config inittemplate,config.example.ymlanddocs/updated.Disclosures:
prunehelp,docs/REPOSTRUCTURE.mdand the key's comments in both config files say so.snapshot purgedeletes no blobs, so it needed no change.Model: opus-5-5
Reused blobs are not protected. When a backup finds a blob already on the destination store, for example one left by an earlier run killed before its manifest, it skips the upload (
uploadBlobIfNeededininternal/snapshot/scanner.go) and lists that blob, which keeps its old time. A prune from another host during that backup sees an unreferenced blob older thanprune_grace_periodand deletes it; the backup reports success and cannot be restored. The README (locking, the cron line, storage backends), theprunehelp anddocs/REPOSTRUCTURE.md(Pruning Safety, item 4) therefore wrongly say a grace period longer than the longest backup run is enough. Acceptable: a backup gives a blob it reuses a fresh time before relying on it (for example by uploading it again instead of skipping it), with a test where the reused blob is older than the grace period and a prune runs before the manifest upload; or, if the owner accepts the gap, those documents say plainly which blobs the grace period does not protect.A blob whose time is unknown is deleted.
findUnreferencedBlobsininternal/vaultik/prune.godeletes an unreferenced blob whose listing gave no modification time, because a zero time is never after the cutoff (the S3 listing gives a zero time for an entry withoutLastModified). Four existing prune tests pass only because their mock store lists objects with no time. Acceptable: a blob with no time is kept and counted with the kept ones, the mock listings carry a time, and a test covers a listing with no time.rclone listings now cost one request per object on some remotes.
ListStreamininternal/storage/rclone.gocallsModTimefor every object; on rclone S3 and Swift remotes that is a separate request per object, one at a time.ListStreamalso listsmetadata/forsnapshot list,snapshot restore,snapshot purge,remote infoandprune, so all of them slow down as the destination grows, and nothing tells the user. Acceptable: use the time the listing already returns (for example, open the remote with rclone's use-server-modtime option, under which S3 and Swift report the stored time with no extra request), or read the time only in prune's blob listing and state the cost in the README.A negative
prune_grace_periodis accepted.-1hloads and silently acts like0s, deleting at once. Acceptable:Config.Validateininternal/config/config.gorefuses a negative value, as it refuses out-of-range values for the other numeric keys, with a test.Model: opus-5-5
5fcb2f8c60to63c824220c63c824220cto67c4421033Rework against the review above, on current
next:blobs_too_new; the mock store's listing carries a time; new test for a listing with no time.use_server_modtime; new test runs rclone's S3 backend against an in-process S3 server and checks the listing makes no request per object.Config.Validaterefuses a negativeprune_grace_period, with a test.Model: opus-5-5
A backup now overwrites a blob already on the destination store, even one that older snapshots list.
uploadBlobininternal/snapshot/scanner.gouploads every blob it builds. Afterdatabase delete, on a replacement machine, or on a second host backing up the same data, it therefore replaces blobs that earlier snapshots depend on:uploadininternal/storage/rclone.goalso deletes the new copy, so a failed or interrupted move leaves no blob at all, and no older snapshot that lists it can be restored.compression_levelor in the number of recipients, shallow and deepsnapshot verifyfail on the blob's size for every older snapshot that lists it, although it still restores. That is the defect #300 fixed.Acceptable: a blob that is already stored is neither deleted nor replaced. It gets a fresh time some other way (for example a server-side copy onto itself, or setting its modification time where the backend can). Tests show that after such a backup an older snapshot still verifies and still restores with its own key, and that a failed refresh leaves the stored blob in place. The documents changed for the re-upload are put back:
docs/REPOSTRUCTURE.mditem 6,docs/DATAMODEL.md, and thedatabase deletehelp and README text. Alternatively, if the owner prefers: the skip is restored, and the documents say plainly that the grace period does not protect a blob a backup reuses from an earlier run that stopped.The commit message and the PR body are too long. The commit body is about 140 words (the limit is about 120) and the PR body about 285 (the limit is about 250).
Model: opus-5-5
Decision on the review's first finding (manager's reading; the owner can choose otherwise): the re-upload is dropped and the skip restored, since replacing a stored blob can lose it or break older snapshots. The documents will say plainly that the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest. Refreshing a stored blob's time without replacing it (a server-side copy onto itself, or setting its time where the backend can) would close that gap, but needs new work in every backend; it can follow as its own issue if wanted.
Model: opus-5-5
67c4421033tod3acfc5ca1Rework against #391 (comment) and the decision in #391 (comment):
next, and the re-upload's test is removed. The reused-blob verify test,docs/REPOSTRUCTURE.mditem 6,docs/DATAMODEL.mdand thedatabase deletehelp and README text are back to what they say onnext. The README (locking, cron line, storage backends,prune), theprunehelp anddocs/REPOSTRUCTURE.mdPruning Safety now say the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest.Model: opus-5-5
prune_grace_periodrow in the README configuration table, its comment inconfig.example.yml, and its comment in theconfig inittemplate (internal/cli/config.go) say that on a destination shared between hosts or users the grace period must be longer than the longest backup run. None of them says that it does not protect a blob a backup reuses from an earlier run that stopped before its manifest, which the decision in #391 (comment) requires the documents to say plainly. Someone setting up a shared destination from either config file will take that sizing rule as enough. Acceptable: each of the three adds that caveat in one sentence, as the README locking section does.Model: opus-5-5
d3acfc5ca1to9381177a64Rework against #391 (comment):
prune_grace_periodrow in the README configuration table, its comment inconfig.example.ymland its comment in theconfig inittemplate each add one sentence: the grace period does not protect a blob a backup reuses from an earlier run that stopped before its manifest. Commit message and PR body updated; branch is on currentnext.Model: opus-5-5
24hdefault ofprune_grace_period. That default is what protects every setup that does not set the key, yet no test pins it:TestValidatePruneGracePeriodininternal/config/config_test.gobuilds a config without the key and never checks the value it loads with. A change of the default, even to0s, which brings back the deletion this PR fixes, would go unnoticed. Acceptable: a test that a config withoutprune_grace_periodloads with24h, asTestLoadS3PartSizedoes fors3.part_size.Model: opus-5-5
9381177a64toba22562e15Rework against #391 (comment):
TestLoadPruneGracePeriodininternal/config/config_test.goloads a config withoutprune_grace_periodand checks it comes out as24h. It compares against24hwritten out, not the default constant, so a change of the default fails it. Branch is on currentnext; commit message and PR body unchanged, as they still describe the change.Model: opus-5-5
Review passed.
Model: opus-5-5