fix(storage) [BRNS-DESK-053]: reserve data-guard backups atomically - #76
Merged
Merged
Conversation
…es where the backup goes
`read_marker` deliberately accepts a predecessor's `.installed-at` instead of
failing boot — the release DMG over a real `~/.brains` aborted on that exact
file, measured 2026-08-12 — and represents it as `legacy:` plus 40 characters
of whatever the old file said. `check_with` then hands that string to
`create_backup`, which spelled it straight into a directory name:
backups_dir.join(format!("{version}_{}", now_ms()))
So an upgrade's defensive backup was named by text this app never wrote. The
marker a shipped release actually left behind is a bare timestamp —
`legacy:2026-07-12T12:26:02.652632+00:00` — and a colon is illegal in a Windows
filename: `dir\legacy:x` is read as an alternate data stream, so
`create_dir_all` fails and the upgrade loses its backup at the one moment the
backup is the entire point. Replacing the colon alone would not have been the
fix. The same string may carry separators, in which case `join` puts the
backup somewhere other than `<root>-backups`; dot segments, which put it above
that directory; a trailing dot or space, which Windows silently drops, so two
labels become one directory; or a device name, which Windows resolves ahead of
the filesystem.
`safe_label` converts it to one bounded component before `join` ever sees it,
at the single seam both callers pass through: keep `[A-Za-z0-9._-]`, map
everything else to `-`, trim the dots and dashes off both ends, fall back to
`unknown` when nothing is left, cap at 64 characters, and prefix `x-` when the
label's own first dot-segment is an MS-DOS device name. It is the alphabet
`attachments::judgement::safe_name` already uses here, and deliberately not
that function: that one is private to attachments, falls back to `"file"`, has
no device-name guard, and its exact outputs are pinned by attachment tests.
The label is computed identically on every OS, which is what lets a macOS test
prove the Windows shape rather than waiting for hardware. Collision is by
design — many hostile spellings map to one label, and the millisecond timestamp
stays what makes a backup unique, exactly as it already was for two upgrades of
the same version.
Nothing else moves. A well-formed version is unchanged, so a normal upgrade
writes the directory it always wrote and no existing backup is renamed or
orphaned. `list_backups` still splits on the last underscore, and a label may
keep its own underscores because the timestamp never contains one. No caller
reads `BackupInfo.version` for display today, and `evidence::prior_use` picks
the recovery source by timestamp and content, so the label's shape reaches no
user-facing surface.
Eleven tests, in the two places they belong: the table of hostile inputs sits
beside the pure function in `backups.rs`, and the behaviour goes through
`check_with` in `data_guard/tests.rs` — a colon-bearing legacy marker still
gets its backup, seven path-shaped markers each land as a direct child of the
backups directory with nothing written beside the data dir, and a sanitized
backup round-trips back out through the listing with its timestamp and run
count intact.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t the last one alphabetically Self-review of the first commit turned up a defect older than this ticket, in the code the sanitizer sits next to. `rotate_backups` sorted the directory names as strings and deleted everything past the fifth. That was only ever right because every name started with the same shape of version number; the moment two labels differ — `orphaned_...` against `1.4.2_...`, or a sanitized `legacy-...` against either — the ordering is alphabetical, not chronological, and the pruning deletes by spelling. The worst case is the one this module exists to prevent: `dismiss_recovery` writes `orphaned_<stamp>`, rotation immediately ranks `o` behind five `1.x` names and removes the backup that was written one line earlier, so "start fresh" stops being reversible. Both `rotate_backups` and `list_backups` now read the timestamp out of the name through one shared `stamp_of`, which is the field that was meant all along. Two more, both mine, both from the same commit: The stamp may not repeat. Many hostile spellings collapse onto one label by design, so the timestamp is the only thing keeping two backups apart — and `create_dir_all` succeeds on a directory that already exists. Two calls in the same millisecond used to merge into one directory and copy over each other. `create_backup` now steps the stamp until the path is free. And the cap has to be the last thing that happens. Applied before the trim it spent the budget on the punctuation that was about to be removed and threw away the version behind it, and applied before the `x-` device prefix it left the result two characters over the bound it was there to enforce. Tests: rotation with mixed labels (the `orphaned` case, proven by a probe that deleted the just-written backup before the fix), two backups in one millisecond, a bound that keeps the version rather than the punctuation, and a Windows device name driven through a well-formed marker — the `legacy:` prefix makes that branch unreachable from a predecessor's file, which is now said out loud in the doc comment rather than left for the next reader to work out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nir-ssvlabs
approved these changes
Aug 14, 2026
nir-ssvlabs
left a comment
nir-ssvlabs
left a comment
There was a problem hiding this comment.
This closes both halves of what #74 left open, and the enumeration comes back clean. reserve_backup_dir makes the create be the claim — fs::create_dir returns AlreadyExists rather than succeeding on an existing directory, the stamp steps on that error, and contents are copied only after the claim lands. That is the atomic version of the loop @sebastian-ssvlabs flagged.
The data_guard_lock is the part I'd have accepted a follow-up for, and it's here instead: I enumerated from the dangerous operation rather than from the lock — every data_guard:: reach on Storage (check, accept_recovery, dismiss_recovery, list_backups, restore_settings, read_marker) takes it, and the only caller outside that seam is tests/legacy_marker.rs. So the four exists()-then-write pairs on the restore side are covered by serialization rather than left to be audited one at a time, which is the outcome I argued for.
- 🟢 The six
expect("data guard lock poisoned")sites turn a panic inside any guard action into a panic in every later one. That is the conventional Rust trade and probably right here, but this particular lock wraps the path that already propagates into Taurisetup— so a poisoned guard is an app that will not start, not a degraded one. If you want the softer behaviour,lock().unwrap_or_else(|e| e.into_inner())keeps the guard usable after a panic.
Checked: the reservation's atomicity and error handling, lock coverage enumerated from every data_guard:: call site, and that Storage is the only production seam. Not read: the 500 lines of new tests — the Rust jobs cover them and the mechanism is where the risk was.
Merge: ✅ into dev once Rust (macOS/Windows) land green — both were still pending at review time. Pixel eval is the repo-wide red job, not this branch.
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
4 participants
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.
Ticket: BRNS-DESK-053 — sanitize data-guard backup directory components on every OS.
Supersedes #74. PR #74 remains open and unchanged to preserve its review history; this replacement rebuilds its patch with signed commits.
Changes
create_dir, so concurrent callers cannot copy snapshots into the same path.Storagehandle, covering concurrent backup, restore, marker, and recovery-cleanup flows.Validation
cargo fmt --all --checkcargo test -p brains-storage— 110 passedcargo clippy --workspace --all-targets -- -D warningsnpm run lint:sizenpm run lint:resourcescargo test --workspaceexercises the affected crates; two pre-existing macOSsecurityd/keychain sandbox tests still fail outside this change.All CI lanes, including Pixel eval, remain required before merge.