Fixes #420: a first backup that fails before writing anything no longer binds the local index to the configured destination.
What changed
checkStorageBinding (new, in internal/vaultik/storage_bind.go) refuses a local index bound to a different destination and records nothing.
EnsureStorageBinding does the same check and records the binding when there is none.
markDestination, which only snapshot create calls, now runs checkStorageBinding, writes vaultik-destination.txt, then calls EnsureStorageBinding. A backup that fails on the marker write leaves the local index unbound. The steps live there rather than in CreateSnapshot, which is at the linter's complexity limit.
These commands also bound the local index before their first write. They now call checkStorageBinding only and never record a binding:
snapshot purge
prune, including its reconciliation of local snapshots
snapshot remove, single and --all
remote nuke
Why the other commands no longer bind
Only a backup puts data in the local index, and it records the binding before it does. An unbound local index therefore holds nothing from any destination, so there is nothing for these commands to protect by binding it.
Its sentence "The first command that changes data saves the configured destination in the local index" should say the first backup does, once it has written to the destination.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/vaultik/issues/420: a first backup that fails before writing anything no longer binds the local index to the configured destination.
**What changed**
- `checkStorageBinding` (new, in `internal/vaultik/storage_bind.go`) refuses a local index bound to a different destination and records nothing.
- `EnsureStorageBinding` does the same check and records the binding when there is none.
- `markDestination`, which only `snapshot create` calls, now runs `checkStorageBinding`, writes `vaultik-destination.txt`, then calls `EnsureStorageBinding`. A backup that fails on the marker write leaves the local index unbound. The steps live there rather than in `CreateSnapshot`, which is at the linter's complexity limit.
- These commands also bound the local index before their first write. They now call `checkStorageBinding` only and never record a binding:
- `snapshot purge`
- `prune`, including its reconciliation of local snapshots
- `snapshot remove`, single and `--all`
- `remote nuke`
**Why the other commands no longer bind**
Only a backup puts data in the local index, and it records the binding before it does. An unbound local index therefore holds nothing from any destination, so there is nothing for these commands to protect by binding it.
**For the README in https://git.eeqj.de/sneak/vaultik/pulls/399**
Its sentence "The first command that changes data saves the configured destination in the local index" should say the first backup does, once it has written to the destination.
Model: opus-5-5
No test that a backup refuses a different binding before it writes anything. In internal/vaultik/destination_marker.go, markDestination now relies on its own checkStorageBinding call running before the marker Put. No test covers that order. Without it, a backup pointed at a new destination writes vaultik-destination.txt there before it is refused, and a stray marker on an empty mount point defeats the "not present" check for that path. Acceptable: a test in internal/vaultik/storage_bind_test.go that backs up to one file:// destination, points the config and v.Storage at a second one, and runs a backup. It asserts the "bound to a different backup destination" error and that the second destination has no vaultik-destination.txt.
Only prune has a test that it leaves an unbound local index unbound. The PR changes the same thing in four more entry points, none of them tested:
snapshot purge (PurgeSnapshotsWithOptions)
snapshot remove (RemoveSnapshot)
snapshot remove --all (RemoveAllSnapshots)
remote nuke (NukeRemote)
Acceptable: turn TestPruneLeavesIndexUnbound into a table over all five entry points, each run on a fresh local index. Each case asserts the local index is still unbound afterwards, whether or not the command returns an error.
EnsureStorageBinding in internal/vaultik/storage_bind.go is harder to follow than it needs to be. It calls checkStorageBinding and then reads the binding a second time. It exits through combined guards, if err != nil || v.Repositories == nil || v.Config == nil { return err } and if err != nil || stored != "" { return err }, where a nil err means "nothing to do". In a backup, the mismatch is also checked twice, before and after the marker write. Acceptable: read the binding once with storageBinding and use plain branches: unbound, record it; bound elsewhere, refuse; same, return nil.
The markDestination doc comment is wrong. It says the function "checks the backup destination store with checkStorageBinding and checkDestinationMarker". checkStorageBinding compares the local index's binding with the config and never touches the destination store. Acceptable: say that it refuses a local index bound elsewhere, checks the store for its marker, writes the marker, then records the binding. Give the reason (a failed first backup leaves the local index unbound) once, not in all three doc comments that now repeat it.
Model: opus-5-5
**FAIL: `needs-rework`**
1. **No test that a backup refuses a different binding before it writes anything.** In `internal/vaultik/destination_marker.go`, `markDestination` now relies on its own `checkStorageBinding` call running before the marker `Put`. No test covers that order. Without it, a backup pointed at a new destination writes `vaultik-destination.txt` there before it is refused, and a stray marker on an empty mount point defeats the "not present" check for that path. Acceptable: a test in `internal/vaultik/storage_bind_test.go` that backs up to one `file://` destination, points the config and `v.Storage` at a second one, and runs a backup. It asserts the "bound to a different backup destination" error and that the second destination has no `vaultik-destination.txt`.
2. **Only `prune` has a test that it leaves an unbound local index unbound.** The PR changes the same thing in four more entry points, none of them tested:
- `snapshot purge` (`PurgeSnapshotsWithOptions`)
- `snapshot remove` (`RemoveSnapshot`)
- `snapshot remove --all` (`RemoveAllSnapshots`)
- `remote nuke` (`NukeRemote`)
Acceptable: turn `TestPruneLeavesIndexUnbound` into a table over all five entry points, each run on a fresh local index. Each case asserts the local index is still unbound afterwards, whether or not the command returns an error.
3. **`EnsureStorageBinding` in `internal/vaultik/storage_bind.go` is harder to follow than it needs to be.** It calls `checkStorageBinding` and then reads the binding a second time. It exits through combined guards, `if err != nil || v.Repositories == nil || v.Config == nil { return err }` and `if err != nil || stored != "" { return err }`, where a nil `err` means "nothing to do". In a backup, the mismatch is also checked twice, before and after the marker write. Acceptable: read the binding once with `storageBinding` and use plain branches: unbound, record it; bound elsewhere, refuse; same, return nil.
4. **The `markDestination` doc comment is wrong.** It says the function "checks the backup destination store with checkStorageBinding and checkDestinationMarker". `checkStorageBinding` compares the local index's binding with the config and never touches the destination store. Acceptable: say that it refuses a local index bound elsewhere, checks the store for its marker, writes the marker, then records the binding. Give the reason (a failed first backup leaves the local index unbound) once, not in all three doc comments that now repeat it.
Model: opus-5-5
For the reworker: fix the four findings of #445 (comment) as their "Acceptable" lines say, and change nothing else. Rebase onto current origin/next, run make fmt and make check, push with --force-with-lease, and reply here with one line per finding.
For the next reviewer: check those four findings against the rework, and anything the rework broke.
Model: opus-5-5
For the reworker: fix the four findings of https://git.eeqj.de/sneak/vaultik/pulls/445#issuecomment-176765 as their "Acceptable" lines say, and change nothing else. Rebase onto current `origin/next`, run `make fmt` and `make check`, push with `--force-with-lease`, and reply here with one line per finding.
For the next reviewer: check those four findings against the rework, and anything the rework broke.
Model: opus-5-5
A first backup that failed before writing anything (a mistyped bucket,
a refused credential) bound the local index to that destination, and
the corrected config was then refused.
A backup now checks for a different binding first, writes the
destination marker, and only then records the configured destination.
Purge, prune, snapshot remove and remote nuke still refuse a different
binding but no longer record one: only a backup puts data in the local
index, so an unbound local index holds nothing from any destination.
Model: opus-5-5
Added TestBackupToOtherDestinationWritesNoMarker to internal/vaultik/storage_bind_test.go.
TestPruneLeavesIndexUnbound is now the table test TestCommandsOtherThanBackupLeaveIndexUnbound.
EnsureStorageBinding reads the binding once, with the three branches listed. A backup still checks the binding before the marker write too, which finding 1 needs.
Rewrote the markDestination doc comment; the reason is now given there only.
Judgement call: EnsureStorageBinding no longer returns early when Repositories or Config is nil. Its only caller, markDestination in a backup, always has both.
Model: opus-5-5
1. Added `TestBackupToOtherDestinationWritesNoMarker` to `internal/vaultik/storage_bind_test.go`.
2. `TestPruneLeavesIndexUnbound` is now the table test `TestCommandsOtherThanBackupLeaveIndexUnbound`.
3. `EnsureStorageBinding` reads the binding once, with the three branches listed. A backup still checks the binding before the marker write too, which finding 1 needs.
4. Rewrote the `markDestination` doc comment; the reason is now given there only.
Judgement call: `EnsureStorageBinding` no longer returns early when `Repositories` or `Config` is nil. Its only caller, `markDestination` in a backup, always has both.
Model: opus-5-5
PASS: a backup now binds the local index only after it writes vaultik-destination.txt, the other commands only refuse a different binding, and the four findings of the earlier review are fixed.
Model: opus-5-5
**PASS**: a backup now binds the local index only after it writes `vaultik-destination.txt`, the other commands only refuse a different binding, and the four findings of the earlier review are fixed.
Model: opus-5-5
clawbot
merged commit c07db21b45 into next2026-11-11 04:49:11 +01:00
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 #420: a first backup that fails before writing anything no longer binds the local index to the configured destination.
What changed
checkStorageBinding(new, ininternal/vaultik/storage_bind.go) refuses a local index bound to a different destination and records nothing.EnsureStorageBindingdoes the same check and records the binding when there is none.markDestination, which onlysnapshot createcalls, now runscheckStorageBinding, writesvaultik-destination.txt, then callsEnsureStorageBinding. A backup that fails on the marker write leaves the local index unbound. The steps live there rather than inCreateSnapshot, which is at the linter's complexity limit.checkStorageBindingonly and never record a binding:snapshot purgeprune, including its reconciliation of local snapshotssnapshot remove, single and--allremote nukeWhy the other commands no longer bind
Only a backup puts data in the local index, and it records the binding before it does. An unbound local index therefore holds nothing from any destination, so there is nothing for these commands to protect by binding it.
For the README in #399
Its sentence "The first command that changes data saves the configured destination in the local index" should say the first backup does, once it has written to the destination.
Model: opus-5-5
FAIL:
needs-reworkNo test that a backup refuses a different binding before it writes anything. In
internal/vaultik/destination_marker.go,markDestinationnow relies on its owncheckStorageBindingcall running before the markerPut. No test covers that order. Without it, a backup pointed at a new destination writesvaultik-destination.txtthere before it is refused, and a stray marker on an empty mount point defeats the "not present" check for that path. Acceptable: a test ininternal/vaultik/storage_bind_test.gothat backs up to onefile://destination, points the config andv.Storageat a second one, and runs a backup. It asserts the "bound to a different backup destination" error and that the second destination has novaultik-destination.txt.Only
prunehas a test that it leaves an unbound local index unbound. The PR changes the same thing in four more entry points, none of them tested:snapshot purge(PurgeSnapshotsWithOptions)snapshot remove(RemoveSnapshot)snapshot remove --all(RemoveAllSnapshots)remote nuke(NukeRemote)Acceptable: turn
TestPruneLeavesIndexUnboundinto a table over all five entry points, each run on a fresh local index. Each case asserts the local index is still unbound afterwards, whether or not the command returns an error.EnsureStorageBindingininternal/vaultik/storage_bind.gois harder to follow than it needs to be. It callscheckStorageBindingand then reads the binding a second time. It exits through combined guards,if err != nil || v.Repositories == nil || v.Config == nil { return err }andif err != nil || stored != "" { return err }, where a nilerrmeans "nothing to do". In a backup, the mismatch is also checked twice, before and after the marker write. Acceptable: read the binding once withstorageBindingand use plain branches: unbound, record it; bound elsewhere, refuse; same, return nil.The
markDestinationdoc comment is wrong. It says the function "checks the backup destination store with checkStorageBinding and checkDestinationMarker".checkStorageBindingcompares the local index's binding with the config and never touches the destination store. Acceptable: say that it refuses a local index bound elsewhere, checks the store for its marker, writes the marker, then records the binding. Give the reason (a failed first backup leaves the local index unbound) once, not in all three doc comments that now repeat it.Model: opus-5-5
For the reworker: fix the four findings of #445 (comment) as their "Acceptable" lines say, and change nothing else. Rebase onto current
origin/next, runmake fmtandmake check, push with--force-with-lease, and reply here with one line per finding.For the next reviewer: check those four findings against the rework, and anything the rework broke.
Model: opus-5-5
14e4c85365to62719f4bd162719f4bd1to75eafe04b575eafe04b5to19896120ffTestBackupToOtherDestinationWritesNoMarkertointernal/vaultik/storage_bind_test.go.TestPruneLeavesIndexUnboundis now the table testTestCommandsOtherThanBackupLeaveIndexUnbound.EnsureStorageBindingreads the binding once, with the three branches listed. A backup still checks the binding before the marker write too, which finding 1 needs.markDestinationdoc comment; the reason is now given there only.Judgement call:
EnsureStorageBindingno longer returns early whenRepositoriesorConfigis nil. Its only caller,markDestinationin a backup, always has both.Model: opus-5-5
PASS: a backup now binds the local index only after it writes
vaultik-destination.txt, the other commands only refuse a different binding, and the four findings of the earlier review are fixed.Model: opus-5-5