Skip to content

fix(tabs): ask the filesystem whether two paths name the same file - #416

Merged
PathGao merged 1 commit into
masterfrom
fix/canonical-path-identity
Aug 3, 2026
Merged

fix(tabs): ask the filesystem whether two paths name the same file#416
PathGao merged 1 commit into
masterfrom
fix/canonical-path-identity

Conversation

@PathGao

@PathGao PathGao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Three independent fixes hit the same wall and each left the same note in the code:

comparison what breaks when the spelling differs
refuseIfLossilyDecoded targetPath !== tab.path Save As typed as /notes/Legacy.md for a tab opened as /notes/legacy.md writes mojibake over the original
claimPath (#413) exact equality two tabs on one file, two auto-save timers, each overwriting the other
the reopen guard (#412) receiving.path === filePath re-opening under another spelling discards unsaved edits

Three fixes writing "this needs the backend to canonicalize" is the signal it belongs at the source.

Case is not the whole problem

Verified on this machine before choosing anything:

write A.md → read via a.md   → same content, same inode (16146078)
write café.md as NFC → open as NFD → same inode
                     → open as CAFÉ.MD (upper + NFD) → same inode

NFC and NFD are different code points, not different case. toLowerCase() cannot reach that by construction — no amount of case folding makes café equal café. APFS is normalization-insensitive as well as case-insensitive, and asking the filesystem answers case, normalization and symlinks at once, using that volume's folding rules rather than ones we guessed.

(A process note worth recording: a first probe using Python's os.path.realpath suggested canonicalization was useless — Python does not fold case. Rust's fs::canonicalize goes through realpath(3) and returns the on-disk name. The conclusion was wrong until the probe was rewritten in the language that will actually run.)

Identity is stored beside the path, not instead of it

Canonicalizing tab.path itself was tempting — it would have made four call sites I could not touch correct for free. It was rejected:

  1. It is lossy where the user can see it. Open ~/notes/today.md (a symlink) and the tab renames itself to 2026-08-03.md; so do the title bar, Copy Path, and the recents entry. That is exactly the implicit behaviour change the project's own guidance argues against.
  2. It is unstable. Delete the file and recreate it under another spelling and the canonical form changes, so tab.path mutates on its own.
  3. It cannot always be computed. Save As names a file that does not exist yet, and canonicalize fails.

So path stays what the user opened and pathKey carries what the filesystem says. The degradation direction is safe: a missing key falls back to exact string equality — today's behaviour — so a missed entry point loses the improvement without introducing a bug.

Every one of the 12 assignments to tab.path is paired with a pathKey assignment; a stale key is worse than no key.

What VS Code does, and why we can do better

Read from source rather than memory: ExtUri.getComparisonKey is uri.path.toLowerCase() behind _ignorePathCasing, decided by uri.scheme === Schemas.file ? !isLinux : truea platform heuristic, not a question to the filesystem. IUriIdentityService.asCanonicalUri likewise uses a capability bit, and "canonical" means the first spelling seen, LRU-cached.

It has the predicted bug on record: microsoft/vscode#123660foo.dart and FOO.DART on a case-sensitive volume, only one shown, and the wrong one opened.

VS Code cannot do better: its file layer is abstract, with remote, virtual and in-memory providers and no canonicalize to call. Markpad's is std::fs. Deviating from the mainstream implementation here is deliberate, and the reason is that our constraints are looser.

One thing we copy exactly: VS Code never rewrites the URI, it only folds at comparison time. That is the same split as path / pathKey.

Symlinks resolve

atomic_write already follows them on purpose ("resolve symlinks so we update the real file"). For the question all three guards are asking — would writing here destroy the file behind that buffer — a link and its target are already one file. Treating them as two documents produces exactly the two-timers-one-file race this is meant to prevent. Display is unaffected, which is the payoff of the two-field split.

(dev, ino) was considered and rejected: it would also fold hard links, but atomic_write writes a temp file and renames over the target, so every save changes the inode and a stored key goes stale immediately.

Cost

loadMarkdown awaits one canonicalization before any decision, and that single I/O is what lets every later comparison be synchronous. refuseIfLossilyDecoded keeps targetPath !== tab.path as a cheap pre-check, so Ctrl+S and the 1.5s auto-save add no I/O at all.

Behaviour on case-sensitive volumes

This is where toLowerCase breaks and asking does not. The Rust test probes the volume's actual behaviour before asserting:

  • folding volume → both spellings map to one key, and the key is the on-disk name (not the first spelling opened, which would make the answer depend on open order);
  • sensitive volume → canonicalize('/vol/a.md') returns Err, and the two files keep two identities. toLowerCase would have merged two genuinely distinct documents and then closed one of the user's tabs — vscode#123660.

The normalization test probes the same way (if fs::metadata(&nfd).is_ok()).

Tests

scripts/pathIdentityCaseFolding.test.ts drives the real TabManager and documentSession against a stubbed backend. The stub contains no toLowerCase — it answers with the name stored in its directory, the way realpath does.

vs master 4 pass / 4 fail → 8 / 8

One red per gap (② has two routes): Save As under another spelling still overwrites the source · one file spelled two ways is one tab (open route) · (Save As route) · following a link to another spelling of the current file does not destroy unsaved edits. The four greens are the control group — a genuinely different file still gets its own tab, an explicit revert still discards, an unresolvable path still compares literally.

The counter-proof caught a bad test of my own. Gap ③'s first version simply re-opened the other spelling — and it was green on master, because master builds a duplicate tab, the content lands in the new one, and the old tab's edits survive. Gap ③ had degenerated into gap ②. It only reaches the #412 guard through { navigate: true } — following a link the author wrote. Without running the counter-proof it would have shipped as a test that can never fail.

npm run check   436 files, 0 errors, 0 warnings
npm test        508 / 508   (was 500)
cargo test      136 / 136   (was 132)
cargo clippy    3 warnings, identical to baseline

One existing assertion was adjusted rather than overridden: lossyDecodeSaveGuard.test.ts matched refuseIfLossilyDecoded(tab, selected) literally and the signature gained a parameter. It is now a prefix match; both intents — picking the source file again must be refused, the guard must precede save_file_content — are unchanged, with a comment pointing at the new behavioural test.

Not covered

Recommended as the immediate follow-up: claimPath's first claim still compares literally in four entry points, all in MarkdownViewer.svelte, which was owned by another change while this was written — goBack/goForward, renameTab, insertTransferredTab, and openMarkdownTargetInNewTab's addTab. All of them call loadMarkdown afterwards, and back-registration means every subsequent comparison is correct, but the claim itself can still create a duplicate tab that is not reclaimed. Closing it is roughly four small changes threading the resolved key through.

  • Hard links do not fold. Two hard links to one inode canonicalize to two paths. Unsolvable by path — and (dev, ino) is ruled out above. A genuine dead end.
  • resolveExternalChange still compares the watcher's path literally. Low risk (we supplied that path to watch_file, so it comes back as written), but incomplete.
  • The recents list still de-duplicates literally, so two spellings can appear twice.
  • normalizeComparableMarkdownPath still folds only Windows/UNC. It answers a different question — does this link point at the open file — but the two mechanisms now disagree in kind, and that is worth unifying later.
  • TOCTOU: the key is resolved once. Delete and recreate under another spelling while the tab is open and it goes stale — worst case a missed merge, i.e. today's behaviour, never a wrong merge.
  • Windows end-to-end is untested; \\?\ stripping has a unit test, NTFS behaviour does not.

🤖 Generated with Claude Code

Three independent fixes each hit the same wall and each left the same
note - the lossy-save guard, one-tab-per-path, and the reopen guard all
compare paths with exact string equality, so on macOS and Windows
`/notes/A.md` and `/notes/a.md` read as two files. Three fixes writing
"this needs the backend to canonicalize" is the signal that it belongs
at the source.

Case is not the whole problem. APFS is normalization-insensitive too:
`café.md` written NFC opens as NFD and both are one inode, and NFC and
NFD are different code points, not different case - so `toLowerCase`
cannot reach it by construction. Asking the filesystem answers case,
normalization and symlinks at once, using that volume's own folding
rules rather than ones we guessed.

Identity is stored beside the path, not instead of it. Canonicalizing
`tab.path` would rename a tab opened through a symlink to its target,
changing the title, the recent-files entry and Copy Path to something
the user never typed; it is also unstable (delete and recreate under
another spelling and the value changes) and cannot always be computed
(Save As names a file that does not exist yet). `pathKey` is the
filesystem's answer, `path` stays what the user opened, and a missing
key degrades to exact string equality - today's behaviour - so a missed
entry point loses the improvement without introducing a bug.

VS Code folds case with `toLowerCase` behind a platform check rather
than asking the filesystem, and has the resulting bug on record as
microsoft/vscode#123660: two genuinely distinct files on a
case-sensitive volume, one of them unreachable. It cannot do better -
its file layer is abstract, with remote and virtual providers and no
`canonicalize` to call. Ours is `std::fs`.

Symlinks resolve, because `atomic_write` already follows them
deliberately: for the question all three guards are asking - would
writing here destroy the file behind that buffer - a link and its target
are already one file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao force-pushed the fix/canonical-path-identity branch from e22df28 to 73a6e8a Compare August 3, 2026 05:25
@PathGao

PathGao commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Linux and Windows cargo test fixed. The tests were wrong; the production code was right on all three platforms — and the failure pattern is what proves it.

Linux:   2 failed  — assertion failed: canonical_identity(&lowered).is_err()
                     assertion failed: canonical_identity(&nfd).is_err()
Windows: 1 failed  — the NFD one only
macOS:   0 failed

Both failures are the same line: assert!(canonical_identity(&x).is_err()). I assumed a nonexistent file makes canonicalize fail — but that is contradicted by a fallback I wrote in the same round: when the target does not exist (Save As to a new file), it canonicalises the parent and rejoins the filename, and returns Ok.

The distribution is the diagnosis:

case normalization failures
macOS APFS folds folds 0 — both spellings exist, so only the if branch ever ran
Windows NTFS folds does not 1 — only NFD reaches the else branch
Linux ext4 does not does not 2

macOS could never execute the other half. Cross-platform CI was not a burden here; it was the only place that branch runs.

The fix asserts the property, not the mechanism

- assert!(canonical_identity(&lowered).is_err());              // "the lookup failed"
+ assert_ne!(by_lowered.as_ref(), Some(&by_real), "two files must keep two identities");

That is what isSameFilePath actually consumes. The old assertion could pass and still prove nothing.

Also moved assert_eq!(file_name, "Alpha.md") out of the else branch — it should hold on both kinds of volume, and buried there it had never executed on Linux. And added a platform-independent assertion covering the property the fallback needs: a name that resolves to nothing must not borrow an existing file's identity, which would merge a Save As target into an unrelated open document.

Evidence per platform

  • macOS, case-sensitive APFS created with hdiutil and TMPDIR pointed at it: reproduced CI verbatim before the fix — panicked at src/lib.rs:561:13: assertion failed: canonical_identity(&lowered).is_err() — and 136/136 after. That is the Linux case failure, really executed.
  • The NFD failure could not be reproduced here. I tried ExFAT expecting UTF-16 comparison; macOS's VFS folds normalization on it anyway (measured: normalization-insensitive? True). So it rests on two substitutes rather than reasoning: the NFD else branch is the same code shape as the case else branch, which was really executed above; and the new platform-independent assertion exercises the underlying property on every volume.
  • Windows case branch was already green — independent evidence that canonicalize folds case on NTFS and returns the on-disk name.

No assertion was weakened, and no #[cfg] skip was added. Both tests still probe the volume's real behaviour and assert on both branches; the net is one assertion more.

npm run check   436 files, 0 errors
npm test        508 / 508
cargo test      136 / 136
cargo clippy    3 warnings, identical to baseline

@PathGao
PathGao merged commit 13f58f3 into master Aug 3, 2026
4 checks passed
@PathGao
PathGao deleted the fix/canonical-path-identity branch August 3, 2026 07:09
PathGao added a commit that referenced this pull request Aug 6, 2026
…itch (#471)

One editor, one implicit model, and a tab switch that overwrote it with
`editor.setValue(content)`. `TextModel.setValue` is *defined* to throw
the undo stack away — `_setValueFromTextBuffer` runs
`this._commandManager.clear()` under the comment "Destroy my edit history
and settings" — so leaving a document and coming back cost the user
everything they could have undone (#391).

Each tab now has its own `ITextModel`, and a switch is
`editor.setModel(...)`. Undo belongs to the document, which is Monaco's
intended usage and how VS Code works. `tabs.svelte.ts` already said so in
a comment: "Text undo belongs to Monaco's model, not to the tab."

## Lifetime

Monaco models are registered with the model service and are NOT collected
while registered, so the invariant is: the set of live models is exactly
the set of live tabs. It is held by reconciliation, not by a dispose
paired with each removal — `retainTabModels(liveTabIds)` asks "which
models have no tab?" after every removal, so a route nobody has thought
of yet is still covered. Three call sites, because there are three places
a tab stops existing: `closeTab` (the close button, Ctrl/Cmd+W,
close-others, close-to-the-right, a clean tab losing its path in
`claimPath`, a tab moved to another window, a rolled-back transfer),
`closeAll`, and `restoreState`, which replaces the whole array and is the
one removal that never calls `closeTab`.

Disposing a model the editor has attached needs no special case:
`CodeEditorWidget._attachModel` registers
`model.onWillDispose(() => this.setModel(null))`, so the editor detaches
itself and the activation effect attaches the new tab's model in the same
flush. A window close destroys the webview, which frees everything.

The models live in `utils/tabModels.ts`, not on `Tab`: a model field on
the store would either drag Monaco back into the startup chunk (~86% of
the startup JavaScript, kept out by a dynamic import and locked by
`monacoStartupGraph.test.ts`) or be typed `any`. That module imports
Monaco for its TYPES only, which is erased.

## Close and reopen: the undo history is gone, and says so

Monaco cannot preserve an undo stack across `dispose()`. Closing a tab
disposes its model, so reopening the document gives a new tab, a new
model and an empty history — same as VS Code, and the same reason. A
cross-window move is the same case: a model is a JavaScript object in one
webview and cannot travel, so the arriving tab starts fresh. Both are
pinned by tests rather than left to be discovered.

## Assumptions that had to change

- Keyed by TAB ID, not by path. A path is not stable for the life of a
  tab (Save As, rename, following a link, back/forward), a model's URI
  cannot change after creation, untitled tabs have no path, and
  `createModel` throws on a duplicate URI. A UUID collides with nothing,
  including the two-tabs-one-path window that #413/#416 are about.
- The language was applied once, at `create`. Now that the model outlives
  every route that repoints a tab, it is re-applied on acquire — which
  also fixes a tab that follows a link from `.md` into `.ts` keeping the
  old language.
- The word count and the language reading arrived for free, because
  `setValue` fires a content change as a side effect. `setModel` does
  not — no content changed, a different document arrived — so the switch
  asks for them explicitly (`syncStatusFromModel`, which also replaces
  two hand-rolled copies of the word count).
- `editor.dispose()` no longer takes the model with it. That is the
  point: `StandaloneEditor` only disposes a model it built itself, and
  this component is unmounted every time a tab goes to reading mode.
- The view state is still saved and restored by hand. It is the editor's,
  not the model's, and it has to be applied after the model it describes
  is attached.

## What `setValue` is still for

External writes — a reload from disk, an accepted external change, a
truncated buffer completed, a task checkbox toggled from the preview, a
link followed inside the tab — hand the tab a DIFFERENT document, and an
undo stack from the old one would splice two texts together. Those still
go through `setValue` and still clear undo, unchanged from today; the
`getValue()` comparison is what tells them from an ordinary switch.
Making them undoable with `pushEditOperation` is what #391 suggests and
is deliberately NOT here: it would let Ctrl+Z resurrect a buffer that the
truncation and lossy-decode guards (#374, #379) exist to keep away from
the file, which is a second behaviour change and deserves its own.

## Verification

`npm run check` 0 errors, `npm test` 700/700, `cargo test` 157/157,
`npm audit` 0 vulnerabilities, `npm run build` clean and Monaco still in
its own chunk.

Falsification: with `scripts/undoHistoryPerTab.test.ts` kept and the two
source files reverted to master, 11 of its 17 assertions go red —
including "undo did nothing after a round trip through another tab" and
"models built during this test: 0 !== 2" on every closing path. The six
that stay green are deliberate locks on behaviour this change preserves
(an external write still clears undo, a navigate clears it, a rename does
not, the word count, the view state, and a negative control that undo
does not reach past the opened state).

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant