fix(storage) [BRNS-DESK-053]: an old install's marker no longer decides where the backup goes - #74
fix(storage) [BRNS-DESK-053]: an old install's marker no longer decides where the backup goes#74stefan-ssv-labs wants to merge 2 commits into
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>
| if !candidate.exists() { | ||
| break candidate; | ||
| } | ||
| stamp += 1; |
There was a problem hiding this comment.
🔴 Critical: this collision-avoidance loop is check-then-act (candidate.exists(), then fs::create_dir_all outside the loop), not atomic — create_dir_all succeeds silently when the directory already exists, it does not error. Two genuinely concurrent calls to create_backup (not the sequential same-thread calls the new two_backups_in_the_same_millisecond_stay_two_backups test exercises) can both observe !candidate.exists() for the same stamp before either creates it, both succeed at create_dir_all on the identical path, and both then copy backup contents into it — reproducing exactly the silent-merge bug this PR sets out to fix (AC4: "distinct backups are never silently collapsed into one path").
This is reachable in this codebase as-is, not just theoretical: guard_dismiss is dispatched via tauri::async_runtime::spawn in src-tauri/src/remote/mod.rs's message handler, which is documented to allow several calls in flight concurrently, and nothing in the call chain serializes them — StorageState is a plain Arc<Storage> with no lock, and Storage::dismiss_recovery/create_backup all take &self. (The UI's own dismiss button is debounced via working in RecoveryOffer.svelte, but that only protects the one first-party caller, not the remote/local-server transport.)
A small mutex around "pick a free stamp and create the dir", or swapping create_dir_all for fs::create_dir (which errors AlreadyExists) inside the retry loop, would close this without touching the sanitization logic.
There was a problem hiding this comment.
Confirmed independently at 54aa7199, including the reachability half:
backups.rs:169-175breaks out of the loop on!candidate.exists(), andcreate_dir_allis at 176, outside it — so the check and the act are separated exactly as you describe.- The dispatch is unserialized in the general case, not just for
guard_dismiss:remote/mod.rs's catch-all arm doestauri::async_runtime::spawn(async move { ops::dispatch(&app, &op, &args) }), so every op name routes onto its own task;StorageState(pub Arc<Storage>)(src-tauri/src/lib.rs:63) carries no lock, anddismiss_recovery(&self, …)takes a shared reference. - The file's own comment at 163-166 names this hazard ("
create_dir_allsucceeds on a directory that already exists … would copy both snapshots into it") immediately above the guard that does not close it — so the intent is right and only the atomicity is missing.
fs::create_dir inside the loop is the minimal correct fix and I'd take it. Worth noting for whoever picks this up: it closes create_backup, but the concurrency it exposes is structural rather than local to this function — see my review body. Resolving is yours.
stefan-ssv-labs
left a comment
stefan-ssv-labs
left a comment
There was a problem hiding this comment.
🛑 review-pr (self-review): 1 blocker remains; see the existing canonical inline thread (comment).\n\n
nir-ssvlabs
left a comment
nir-ssvlabs
left a comment
There was a problem hiding this comment.
Caution
Not approving as-is: the collision guard this PR adds is check-then-act, so the silent-merge it sets out to fix (AC4) survives under concurrency. Confirmed in @sebastian-ssvlabs's thread rather than re-filed here.
The sanitization itself is the right shape — one seam, one join, computation identical on every OS so a macOS test can prove the Windows behaviour, and the traversal case asserted against backup_path.parent() rather than against the string.
🟠 High · the concurrency is structural, so fixing the loop alone leaves the class open (blocking alongside the 🔴): @stefan-ssv-labs fs::create_dir inside the retry loop closes create_backup, and it should go in. But the exposure isn't local to that function: remote/mod.rs's catch-all arm spawns every op onto its own task via ops::dispatch, StorageState(pub Arc<Storage>) (src-tauri/src/lib.rs:63) holds no lock, and the guard's entry points all take &self. backups.rs carries four more exists()-then-write pairs on the restore side (295, 319, 328, 336) reachable through the same dispatch. Patching one loop is correct and insufficient — a mutex around the guard's mutating entry points is what actually closes it, and is the smaller change than auditing each pair.
- 🔵
Pixel eval (headless)is red. The failed-step log tails out at checkout/cleanup with no assertion, and this branch touches no TypeScript, Svelte, manifest or resource file — so it reads as environmental. Worth confirming it also fails ondevbefore anyone waves it through; right now the only evidence it's unrelated is that it should be.
Checked: create_backup's check/act split against the source, the dispatch path from remote/mod.rs through ops::dispatch to StorageState, every exists() site in backups.rs, and both Rust jobs (macOS + Windows) passing. Not read: safe_label's individual character cases and the 15 new tests — the Rust suite covers them on both platforms and the sanitization is not where the defect is.
Merge: ⛔ not yet into dev — the 🔴 stands, and I'd resolve the structural half in the same pass rather than shipping a loop fix that leaves four sibling pairs behind it.
4 participants
Ticket: BRNS-DESK-053 — brains-desktop parity W0.2 — sanitize data-guard backup directory components on every OS
Board: https://app.mybrains.ai/boards/1d99e87c-2f14-4034-87ff-749cd8468487 (findings dataset, row
BRNS-DESK-053)What was wrong
InstallMarker::legacydeliberately represents a predecessor's unparseable.installed-atas the version stringlegacy:<40 chars of whatever that file said>. On a version changecheck_withhands that string straight tocreate_backup, which builds<root>-backups/<version>_<timestamp>.A colon is not legal in a Windows filename, and
dir\legacy:xis read as an NTFS alternate data stream rather than a directory — socreate_dir_allfails. That error does not stay local:check_withpropagates it out throughStorage::check_guardinto Tauri'ssetup(src-tauri/src/lib.rs:455), so the upgrade does not lose its backup so much as lose the app. The release DMG over a real~/.brainsaborting on this file is the incident the guard was written for, measured live 2026-08-12.Colons are only the observed case. Marker data is arbitrary: separators and
..makejoinresolve outside the backups directory entirely, a Windows reserved device name (CON,COM1, …) cannot be created as a directory, and a trailing dot is silently dropped by Windows, which quietly turns two labels into one directory.What changed
src/engines/storage/src/data_guard/backups.rs— one new private function at the single seam where a version becomes a path component:safe_label(version)keeps[A-Za-z0-9._-]and maps everything else to-, trims leading/trailing.and-, falls back tounknownwhen nothing survives, prefixesx-when the segment before the first dot is a Windows reserved device name, and caps the result at 64 characters — the cap last, prefix included, then a final trailing-punctuation trim. The computation is identical on every OS, which is what lets a macOS test prove the Windows shape.Two defects the self-review turned up in the same file, one of them older than this ticket:
rotate_backupsdeleted by spelling, not by age. It sorted directory names as strings and dropped everything past the fifth. That only worked while every name started with the same shape of version number.dismiss_recoverywritesorphaned_<stamp>, which sorts behind five1.x_...names — so rotation deleted the backup written one line earlier and "start fresh" stopped being reversible. Proven with a probe before the fix. Bothrotate_backupsandlist_backupsnow read the timestamp out of the name through one sharedstamp_of.create_dir_allsucceeds on a directory that already exists. Two calls in the same millisecond merged into one directory and copied over each other.create_backupnow steps the stamp until the path is free.Nothing else moved: existing backup directories are still listed, the
<label>_<stamp>parse is unchanged in shape, andrestore_from/restore_settings/ the recovery selection inevidence.rs(which picks by timestamp and content, never by label) are untouched.Requirement → change
safe_label+ the singlejoinincreate_backupnothing_that_looks_like_a_path_stays_one,windows_hostile_characters_are_replaced,a_path_shaped_legacy_marker_cannot_escape_the_backups_directory(7 hostile inputs; assertsbackup_path.parent() == Some(paths.backups)and that the tempdir still holds exactly.brains+.brains-backups)legacy:<marker>version still gets its upgrade backupthe_field_observed_legacy_marker_becomes_a_filename,a_legacy_marker_full_of_colons_still_gets_its_backuplist_backupsviarsplit_once('_')+stamp_ofa_sanitized_legacy_backup_round_trips_through_the_listing,underscores_survive_for_the_listing_parsecreate_backuptwo_backups_in_the_same_millisecond_stay_two_backupsbrains-storagesuite, 94 → 109 testsstamp_of+sort_by_key(Reverse(...))rotation_keeps_the_newest_backups_whatever_they_are_calledVerification
Run in a worktree off
origin/dev@b3aae57,CARGO_TARGET_DIRshared with the main checkout.cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test -p brains-storagecargo test --workspacenpm run lint:sizebackups.rs525 lines,data_guard/tests.rs559 — both under the 600 hard ceiling; the inline test module carries the rule-3 size note)npm run lint:resourcesnpm run stage:resourcesin the fresh worktreeThe two
cargo test --workspacefailures arebrains-local-agents --test headless_sandbox:a_confined_run_reads_its_login_but_writes_no_secretanda_scheduled_run_is_confined_and_still_does_its_work, both failing onsecurityd must answer (the CLI reads its own login). They are environmental, not from this branch — the identical two tests fail on a cleanmaincheckout built into a separate target directory. Everything else in the workspace is green.Frontend checks were not run: this branch touches no TypeScript, Svelte, manifest or resource-declaring file.
Windows is not covered locally. macOS has no reserved device names and accepts a colon in a filename, so the Windows failure mode cannot be reproduced here. That is why the sanitizer is a pure string function with no
cfgbranch: the tests assert the computed name, which is identical on every OS, rather than the filesystem's reaction to it. CI's Rust job is a macOS + Windows matrix, so the windows-latest leg runs the same assertions natively.Reproduction
.installed-atin%USERPROFILE%\.brains— any content that is not this app's JSON marker, e.g. a bare2026-07-12T12:26:02.652632+00:00.read_markeradoptslegacy:2026-07-12T12:26:02.652632+00:00,create_backuptries to create a directory with that name,create_dir_allfails on the colon, and the error surfaces out of Taurisetup— the app does not start.legacy-2026-07-12T12-26-02.652632-00-00_<stamp>, the backup lands, andlist_backupsshows it under that version.Shipping impact
Rust-only, inside the
brains-storageengine. No manifest, resource, capability,externalBin, signing or updater surface is touched, and nothing needs restaging. No persisted-state migration: existing backup directories keep their names and are still listed, and the rotation fix changes only which of them is considered newest — by timestamp, which is what the name always carried.The user-visible change is the one this ticket is about: an upgrade from a legacy install on Windows now completes instead of aborting at boot. On macOS the visible difference is a legacy backup's directory name, and the fact that dismissing a recovery offer no longer risks deleting the backup that made the dismissal reversible.
Regression provenance
Parity execution intake from #47 and its required rebase review #47 (review), restamped against
dev@b3aae578e662da59ae3013cb248f1eb6a96642f5.Follow-ups filed, not fixed here
create_backupaborts boot rather than logging and continuing — the propagation path described above. Widening that is a behaviour change to the guard's contract and belongs in its own ticket.storage/src/attachments/judgement.rs:192,model/src/attachments.rs:288, andsafe_label). They differ in fallback, bound and device handling, and two of them have their exact outputs pinned by tests, so unifying them is a separate change.Merge method: squash (do not merge-commit or rebase-merge).
🤖 Generated with Claude Code