A command killed part-way could leave its .tmp- files and directories, encrypted keys included, for good. The next command that takes the state directory lock now deletes them, when the command before it did not finish.
vault.LockStateDir empties the lock file once it holds the lock; the function it returns writes finished there just before releasing it. A command killed while holding the lock leaves no finished.
A holder that finds no finished calls the new secret.RemoveLeftovers on the state directory, each vault, each secret and each version, the only places secret.WriteFileAtomic and secret.TempDirFor put temporary entries. After a command that finished nothing is searched, so the added time stays flat as secrets and versions pile up.
Only names starting with . and containing .tmp- are deleted. vaults.d, secrets.d, versions and unlockers.d are not searched: a vault may be named .tmp-1, and the test keeps one.
A failure to delete is a warning; the command goes on.
#91 already lets unlocker remove remove an unlocker directory with no metadata file; this adds a test for it.
Worth knowing:
The lock file is written in place, never replaced, so a waiting command keeps the same lock.
The in-memory test filesystem now gets a lock file; tests comparing the state directory before and after a command allow for it.
Judgement call: a command that panics still writes finished through its deferred release, so its leftovers are not searched for; a leftover that fails to delete is not retried.
TODO.md keeps the init/vault create exception from #75, which then has no open issue.
Model: opus-5-5
A command killed part-way could leave its `.tmp-` files and directories, encrypted keys included, for good. The next command that takes the state directory lock now deletes them, when the command before it did not finish.
- `vault.LockStateDir` empties the lock file once it holds the lock; the function it returns writes `finished` there just before releasing it. A command killed while holding the lock leaves no `finished`.
- A holder that finds no `finished` calls the new `secret.RemoveLeftovers` on the state directory, each vault, each secret and each version, the only places `secret.WriteFileAtomic` and `secret.TempDirFor` put temporary entries. After a command that finished nothing is searched, so the added time stays flat as secrets and versions pile up.
- Only names starting with `.` and containing `.tmp-` are deleted. `vaults.d`, `secrets.d`, `versions` and `unlockers.d` are not searched: a vault may be named `.tmp-1`, and the test keeps one.
- A failure to delete is a warning; the command goes on.
https://git.eeqj.de/sneak/secret/pulls/91 already lets `unlocker remove` remove an unlocker directory with no metadata file; this adds a test for it.
Worth knowing:
- The lock file is written in place, never replaced, so a waiting command keeps the same lock.
- The in-memory test filesystem now gets a lock file; tests comparing the state directory before and after a command allow for it.
- Judgement call: a command that panics still writes `finished` through its deferred release, so its leftovers are not searched for; a leftover that fails to delete is not retried.
- `TODO.md` keeps the `init`/`vault create` exception from https://git.eeqj.de/sneak/secret/issues/75, which then has no open issue.
Model: opus-5-5
internal/vault/lock.go, removeLeftovers: every command that takes the lock now lists every vault, secret and version directory, so its added time grows with every version ever stored, and versions are never pruned. It is about 0.2 s per command at a thousand secrets with three versions each, and seconds beyond that, where the command took a few milliseconds before. The cleanup must add no noticeable time to every command. Acceptable: the added time stays flat as secrets and versions pile up. For example, search only when the previous holder of the lock did not finish (the lock file can record that), or keep every temporary entry where one listing finds it.
Conflicts with current next: since Ask before removing a secret, version, vault or unlocker (#39) landed, rebasing conflicts in internal/cli/unlockers_corrupt_test.go (the header comment, and the new TestUnlockerRemoveWithoutMetadata beside the reworked TestUnlockerRemoveWithUnreadableMetadata) and in TODO.md. UnlockersRemove now asks before it removes anything. Acceptable: rebased onto current next, with the new test and its comment matching the question step.
Disclosures:
Reviewed against the PR's own base. Not checked again on current next, because of the conflict.
Judgement call: names are matched by "starts with . and contains .tmp-". That is wider than the names the helpers make, which end in .tmp- and digits. I accepted it because only the tool's own directories are searched.
Judgement call: per the comment on #75, I took the init/vault create exception that TODO.md keeps as out of scope. Merging closes that issue and leaves that exception with no open issue.
Model: opus-5-5
FAIL: `needs-rework`
1. `internal/vault/lock.go`, `removeLeftovers`: every command that takes the lock now lists every vault, secret and version directory, so its added time grows with every version ever stored, and versions are never pruned. It is about 0.2 s per command at a thousand secrets with three versions each, and seconds beyond that, where the command took a few milliseconds before. The cleanup must add no noticeable time to every command. Acceptable: the added time stays flat as secrets and versions pile up. For example, search only when the previous holder of the lock did not finish (the lock file can record that), or keep every temporary entry where one listing finds it.
2. Conflicts with current `next`: since `Ask before removing a secret, version, vault or unlocker` (https://git.eeqj.de/sneak/secret/issues/39) landed, rebasing conflicts in `internal/cli/unlockers_corrupt_test.go` (the header comment, and the new `TestUnlockerRemoveWithoutMetadata` beside the reworked `TestUnlockerRemoveWithUnreadableMetadata`) and in `TODO.md`. `UnlockersRemove` now asks before it removes anything. Acceptable: rebased onto current `next`, with the new test and its comment matching the question step.
Disclosures:
- Reviewed against the PR's own base. Not checked again on current `next`, because of the conflict.
- Judgement call: names are matched by "starts with `.` and contains `.tmp-`". That is wider than the names the helpers make, which end in `.tmp-` and digits. I accepted it because only the tool's own directories are searched.
- Judgement call: per the comment on https://git.eeqj.de/sneak/secret/issues/75, I took the `init`/`vault create` exception that `TODO.md` keeps as out of scope. Merging closes that issue and leaves that exception with no open issue.
Model: opus-5-5
clawbot
changed title from Delete .tmp- leftovers when the next command takes the lock (closes #75) to Delete .tmp- leftovers of a killed command when the lock is next taken (closes #75)2026-10-04 18:15:41 +02:00
The search runs only when the lock file lacks the finished mark each command writes just before releasing the lock; TestLockStateDirRemovesLeftoversOnlyAfterKill covers a holder that finished and one that was killed.
Rebased onto current next; TestUnlockerRemoveWithoutMetadata now checks the question says the unlocker is not the last and answers y; both TODO.md entries kept.
Model: opus-5-5
Reworked:
1. The search runs only when the lock file lacks the `finished` mark each command writes just before releasing the lock; `TestLockStateDirRemovesLeftoversOnlyAfterKill` covers a holder that finished and one that was killed.
2. Rebased onto current `next`; `TestUnlockerRemoveWithoutMetadata` now checks the question says the unlocker is not the last and answers `y`; both `TODO.md` entries kept.
Model: opus-5-5
PASS: both findings of the first review are fixed, and I found no defect that would harm a user or break the definition of done.
Judgement call: as in the first review, the init/vault create exception that TODO.md keeps is out of scope; merging closes #75 and leaves that exception with no open issue.
Unverified: a power loss or machine crash part-way through a command was reasoned about, not tested; killed commands were tested.
Model: opus-5-5
PASS: both findings of the first review are fixed, and I found no defect that would harm a user or break the definition of done.
- Judgement call: as in the first review, the `init`/`vault create` exception that `TODO.md` keeps is out of scope; merging closes https://git.eeqj.de/sneak/secret/issues/75 and leaves that exception with no open issue.
- Unverified: a power loss or machine crash part-way through a command was reasoned about, not tested; killed commands were tested.
Model: opus-5-5
A command killed part-way could leave a temporary file or directory of
secret.WriteFileAtomic or secret.TempDirFor, encrypted keys included,
for good. LockStateDir now empties the lock file once it holds the lock
and writes "finished" there just before releasing it. A holder that
does not find that deletes such leftovers from the state directory,
each vault, each secret and each version, the only places those helpers
make them, matching names that start with "." and hold ".tmp-". After a
command that finished nothing is searched, so the added time does not
grow with the number of secrets and versions. A test shows that
`unlocker remove` removes an unlocker directory with no metadata file.
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.
A command killed part-way could leave its
.tmp-files and directories, encrypted keys included, for good. The next command that takes the state directory lock now deletes them, when the command before it did not finish.vault.LockStateDirempties the lock file once it holds the lock; the function it returns writesfinishedthere just before releasing it. A command killed while holding the lock leaves nofinished.finishedcalls the newsecret.RemoveLeftoverson the state directory, each vault, each secret and each version, the only placessecret.WriteFileAtomicandsecret.TempDirForput temporary entries. After a command that finished nothing is searched, so the added time stays flat as secrets and versions pile up..and containing.tmp-are deleted.vaults.d,secrets.d,versionsandunlockers.dare not searched: a vault may be named.tmp-1, and the test keeps one.#91 already lets
unlocker removeremove an unlocker directory with no metadata file; this adds a test for it.Worth knowing:
finishedthrough its deferred release, so its leftovers are not searched for; a leftover that fails to delete is not retried.TODO.mdkeeps theinit/vault createexception from #75, which then has no open issue.Model: opus-5-5
FAIL:
needs-reworkinternal/vault/lock.go,removeLeftovers: every command that takes the lock now lists every vault, secret and version directory, so its added time grows with every version ever stored, and versions are never pruned. It is about 0.2 s per command at a thousand secrets with three versions each, and seconds beyond that, where the command took a few milliseconds before. The cleanup must add no noticeable time to every command. Acceptable: the added time stays flat as secrets and versions pile up. For example, search only when the previous holder of the lock did not finish (the lock file can record that), or keep every temporary entry where one listing finds it.next: sinceAsk before removing a secret, version, vault or unlocker(#39) landed, rebasing conflicts ininternal/cli/unlockers_corrupt_test.go(the header comment, and the newTestUnlockerRemoveWithoutMetadatabeside the reworkedTestUnlockerRemoveWithUnreadableMetadata) and inTODO.md.UnlockersRemovenow asks before it removes anything. Acceptable: rebased onto currentnext, with the new test and its comment matching the question step.Disclosures:
next, because of the conflict..and contains.tmp-". That is wider than the names the helpers make, which end in.tmp-and digits. I accepted it because only the tool's own directories are searched.init/vault createexception thatTODO.mdkeeps as out of scope. Merging closes that issue and leaves that exception with no open issue.Model: opus-5-5
ff721a87e6toa23b4e7db7a23b4e7db7toeb43ef7d49Delete .tmp- leftovers when the next command takes the lock (closes #75)to Delete .tmp- leftovers of a killed command when the lock is next taken (closes #75)Reworked:
finishedmark each command writes just before releasing the lock;TestLockStateDirRemovesLeftoversOnlyAfterKillcovers a holder that finished and one that was killed.next;TestUnlockerRemoveWithoutMetadatanow checks the question says the unlocker is not the last and answersy; bothTODO.mdentries kept.Model: opus-5-5
PASS: both findings of the first review are fixed, and I found no defect that would harm a user or break the definition of done.
init/vault createexception thatTODO.mdkeeps is out of scope; merging closes #75 and leaves that exception with no open issue.Model: opus-5-5
eb43ef7d49to8848006391