docs(storage): record why a moved device number cannot be repaired on its own - #1868
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
P1. The E2E fixture now exercises the auto-heal branch. apps/desktop/e2e/storage-root-conflict.spec.ts:69-73 stales only dev (inode intact), which is precisely onlyTheDeviceNumberMoved's condition. The repair dialog never opens, the [storage-root] root-identity conflict gate signal never prints, appParkedAtRepairGate times out at 30s, and the marker-untouched assertion fails because the marker was rewritten. The CI plan guarantees the desktop e2e job runs for this PR. Stale ino in the spec instead, mirroring the PR's own foreign-inode fixture.
P2. ino equality does not prove a remount (raised independently by both reviewers). Inode numbers are unique only within one mounted filesystem; a workspace copied to another volume can get the same root-directory inode, and the code then silently keeps the old rootId and rewrites the marker, so two independent roots share one logical identity. The (dev, ino) pair plus a durable volume identifier would make both this case and the remount case decidable; that belongs in @maka/storage/root-authority, not the desktop layer.
P2. The classification is not bound to the repaired candidate. onlyTheDeviceNumberMoved reads and stores a boolean (storage-root-startup.ts:44,65), then a separate snapshot is prepared (:66-69), then the repair commits based on the stale boolean (:71-73). If the marker is swapped between the two reads, a foreign candidate is adopted without confirmation. Carry the classification on the repair candidate itself.
P3. The remount check re-reads and re-parses the storage-owned marker with a raw unbounded readFile (storage-root-startup.ts:37-44), bypassing readBoundedMarkerFile's 1KB bound and the schema validation; a marker replaced with a FIFO blocks forever, the exact hang this PR exists to fix. The same unanswerable condition also stays fatal for headless (packages/headless/src/headless-storage.ts:51), while storage's adoptStorageRootOnImport already auto-repairs drift. Expose the classification from the storage layer instead of re-deriving it.
1a07121 to
f1f7634
Compare
…answer for The storage-root marker records `dev` and `ino`. Copying a workspace gives the copy its own inode, which is the case the identity check exists to catch and the only one worth stopping a person for. A device number carries no such meaning across restarts: the kernel hands one out per mount, so an unmoved directory reports a different `dev` after its volume is mounted again while keeping the inode it always had. Asking about that is worse than useless. It is routine, there is nothing for a person to decide, and the question arrives through a dialog raised before any window exists — which does not appear. The launch hangs with the process alive and nothing printed, which is what this looked like from the outside. The check stays exactly as strict where it means something: `markerMatchesIdentity` is untouched, so a genuinely copied workspace still reaches the person. Only the desktop recovery layer learns to recognise the one case that carries no information, and it returns false whenever the answer is not certain. Adds one test and rewrites none: the existing assertion that an ambiguous marker still asks is the behaviour this must not change.
It runs before any window exists, which is the only place it could be read, and it carries a fixed sentence with no path, no identity and no secret.
… a second time downstream Review found three problems with the desktop-side version, and all three come from the same choice: classifying the drift by re-reading the marker after the storage layer had already read it. - The e2e spec stales `dev` and leaves `ino` intact, which is precisely the auto-repair condition. The repair dialog would never open, the gate signal never print, and the test would wait 30s for a hang that had been fixed. It stales the inode now, which is the drift that should reach a person. - The classification was read into a boolean, a separate snapshot was then prepared, and the repair committed on the stale boolean. A marker swapped between the two reads would adopt a foreign candidate with nobody asked. - The re-read used a raw unbounded `readFile`, bypassing the bounded read and schema validation the storage layer applies — a marker replaced with a FIFO would block forever, which is the exact hang this change exists to remove. `StorageRootIdentityRepairCandidate` now carries `drift`, decided from the marker it was prepared against, under the identity assertions that already guard that read. The desktop layer reads the answer instead of deriving it. `drift` is a reason rather than a boolean because inode equality does not prove a remount: inode numbers are unique only within one mounted filesystem, so a workspace copied to another volume can carry the same root-directory inode. That case is `unknown` and still reaches the person. Naming it a remount would silently give two independent roots one logical identity.
f1f7634 to
28835c7
Compare
… looks the same Repairing a moved `dev` without asking rested on the claim that only a remount produces one. It does not. Inode numbers are unique within a single mounted filesystem, so a workspace restored onto another volume can report the same root-directory inode as the original — a different `dev` beside a matching `ino`, which is exactly the pattern that was being auto-repaired. Startup would then rewrite the marker with no dialog and hand back the original rootId, leaving two unrelated directories sharing one identity. The docstring said that case was a question for the person; the code answered it. Nothing in the marker can tell the two apart, so the classification is gone rather than narrowed, and the drift is a question again. The comment now sits on `markerMatchesIdentity`, where the next person will be tempted, and points at `adoptStorageRootOnImport` as the way in for a copy — where the caller states which rootId it expects. A desktop test pins it: a marker whose only stale field is `dev` reaches the person, and declining leaves the marker byte for byte as it was. Restoring the auto-repair reddens that test and two that were already there, which is why the original change had to rewrite them.
|
Review finding confirmed, and the auto-repair is gone. The claim the change rested on was that a moved device number can only come from a remount. It cannot. Inode numbers are unique within one mounted filesystem, so a workspace restored onto an external volume can report the same root-directory inode as the original. That presents as a different dev beside a matching ino, which is exactly the pattern onlyTheDeviceNumberMoved recognised, so startup would rewrite the marker with no dialog and hand back the original rootId. Two unrelated directories would then share one identity, silently. The docstring already said that case was a question for the person; the code answered it. Nothing in the marker distinguishes the two, so I removed the classification rather than narrowing it. There is no drift field and no second answer downstream, and every identity mismatch is a question again. The explanation now sits on markerMatchesIdentity, where the next person will be tempted to add this back, and points at adoptStorageRootOnImport as the sanctioned way in for a copied workspace, where the caller states which rootId it expects. What remains is the test that pins the claim: a marker whose only stale field is dev reaches the person, and declining leaves the marker byte for byte as it was. Negative control: putting the auto-repair back reddens that test (expected true, actual false on "only the person can tell a remount from a restored copy") and also reddens the two tests that were already there for declining and for a swapped root. Those two are the ones the original change had to rewrite from dev to ino, which in hindsight was the signal. The complaint behind the PR is real and still open: the repair dialog is raised before any window exists, so it may not appear and the launch hangs with nothing printed. That is a dialog problem, not a reason to adopt a root without asking. #1869 at least makes that hang say which step it is stuck on. A proper fix belongs in how the question is asked, not in skipping it. Test delta against origin/main on this machine: storage 241 failing before and after, identical failure set (all node:sqlite, missing in this local Node). Desktop 73 failing before and after; the sets differ only by one load-flaky suite that passes when run alone. |
hqhq1025
changed the title
2 participants
This PR started out trying to stop the storage-root repair dialog from asking a
person about a device number they cannot answer for, and ended up concluding
that the question cannot be answered from the marker at all. What is left is the
reasoning and a tripwire, not a behaviour change.
What happened
The first attempt classified a marker mismatch as
remountedwhen the inodematched and only the device number had moved, and repaired it without asking.
That is wrong. A differing device number is exactly the cross-volume case, and
inode numbers are only unique within one mounted filesystem — so a workspace
restored onto another volume can present the same inode as the original. The
auto-repair would then hand back the original root id for a genuinely different
root, silently, in the one situation where being wrong costs the most.
Nothing in the marker separates those two cases, so the classification was
removed rather than narrowed. Every mismatch is a question for the person again,
which is what main already does.
What this PR now contains
markerMatchesIdentityrecording why the device number cannot betrusted on its own, placed where the next person will be tempted to trust it,
and pointing at
adoptStorageRootOnImportas the sanctioned path for a copy.asking. They guard against the auto-repair being reintroduced; they do not
guard anything this PR adds.
What is still open
A routine volume remount still parks the launch at a repair dialog. Solving it
needs an identity signal the marker does not currently carry — a volume UUID, or
an explicit adopt step — not a cleverer reading of
devandino. That is aseparate change and is not attempted here.