fix(recorder): storage-migration same-file guard — never unlink the only copy (#276) - #282
Merged
Merged
Conversation
…unlink the only copy The 'Change storage' drain's copy-failure cleanup ran remove_file(dst) — but when the from/to storage rows resolve to the SAME file (duplicate rows for one path: storages has UNIQUE(name), not UNIQUE(path); symlinked roots alias too), copy_with_crc32's same-file bail routed into that failure arm and dst IS src: the #70 defense against truncating the only copy became an outright unlink of the only copy, destroying the policy's oldest MIGRATION_BATCH segments per attempt (audit finding F1, issue #276). - Pre-guard: same_file(src, dst) before the copy -> flip the DB row in place with NO file I/O (mirrors move_segment_to_archive's in-place flip). A same-path migration is now the safe, useful consolidation it looks like. - Cleanup: an in-place segment deletes NOTHING in either arm (flipped: the row points at the same bytes under the new root; lost-the-race: the file is still the live source). - Defense in depth: the copy-failure arm re-verifies !same_file before removing the partial dst, enforcing the phase invariant 'the unguarded copy phase never deletes a file it did not itself create' even across a rename/symlink swap between guard and copy. Regression test change_storage_drain_same_path_flips_in_place: duplicate storage row on one directory, drain between them -> files untouched, rows flipped, progress recorded, second run a no-op. Verified the test FAILS against the pre-fix code (files destroyed) and passes with the fix — the audit's static trace reproduced at runtime. Fixes #276 Signed-off-by: badbread <badbread@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #276 — P0 footage-loss finding (F1) from the 2026-07-19 adversarial recorder audit. The #70 failure class, resurrected in the "Change storage" migration path.
The bug, reproduced at runtime
When the from/to storage rows resolve to the same file (duplicate rows for one path —
storageshasUNIQUE(name)but notUNIQUE(path)— or symlinked/bind-mounted roots),copy_with_crc32's same-file bail routed into the copy-failure arm, whose cleanup ranremove_file(dst)— and dst IS src. The defense against truncating the only copy became an outright unlink of the only copy: the policy's oldestMIGRATION_BATCH(256) segments destroyed per attempt, silently, with each retry eating the next 256.Runtime-proven, not just traced: the new regression test run against the pre-fix code fails with the files destroyed; with the fix it passes.
The fix
same_file(src, dst)before the copy → flip the DB row in place with zero file I/O (mirrorsmove_segment_to_archive's in-place flip). A same-path migration is now the safe consolidation it looks like — rows repoint, files untouched, progress recorded (no false "stalled").!same_filebefore removing the partial dst, enforcing the audit's phase invariant — the unguarded copy phase never deletes a file it did not itself create — even across a rename/symlink swap between guard and copy.Verification
change_storage_drain_same_path_flips_in_place(duplicate storage row on one dir → drain → files exist, rows flipped, progress = 2, idempotent re-run). Fails on pre-fix code, passes on fix.change_storage_drain_relocates_footage(the normal path) still green.cargo test --workspace✅ (259 recorder tests).Operator note
Until this is deployed, do not run a policy change-storage migration on any install that might carry duplicate storage rows for one directory.