fix(tags): stop two windows from erasing each other's pinned tags

[PathGao]

The defect

save_pinned_tag and remove_pinned_tag are unsynchronised read-modify-write cycles over one shared file:

let mut tags = read_pinned_tags(&app);   // read the whole file
...                                       // edit the list
crate::atomic_write(&pinned_tags_path(&app)?, json.as_bytes())   // write the whole file

Tauri dispatches commands on a thread pool and every window can call these. Two windows both read the same list, both write back a full copy, and the second write silently drops what the first recorded.

Why atomic_write does not already cover this

This is the natural assumption, and it is the reason the bug is easy to walk past. atomic_write rules out a torn file — temp file, fsync, rename, so no reader ever sees half a JSON document. A lost update is a different failure: both writers produce a whole, valid file, and the second one is simply built from a snapshot taken before the first one landed.

window A: read [x] ─── add "a" ─────────── write [x, a]
window B:      read [x] ─── add "b" ─────────────────── write [x, b]
on disk:  [x]                             [x, a]        [x, b]   ← "a" gone

Atomicity is a property of each individual write. It says nothing about the interval between a read and the write derived from it.

The trigger is not theoretical

savePinnedTagIfNeeded runs from each window's own close handler — appExit, destroyWindowAfterTabsClosed, and the close-requested path in MarkdownViewer.svelte. Quitting two tagged windows with ⌘Q (or a system shutdown) runs both cycles at once. TitleBar.togglePinnedTag and clearTag also fire their invoke without awaiting it.

Same defect class as #405, different fix, and the difference matters

#405 fixed recent-files being clobbered by re-reading live storage instead of serialising an in-memory snapshot. A re-read alone was sufficient there because localStorage is per-document and single-threaded — an RMW cycle is atomic by construction, so there is no interval to protect. Rust commands have no such property. The same shape here needs an explicit lock, not a second re-read.

The fix

The cycle moves into update_pinned_tags, which holds a new AppState.pinned_tags: Mutex<()> across read → edit → write. Same shape and the same lock_recover as the existing window_registry: Mutex<HashMap<…>> — no new concurrency primitive and no async runtime.

save_pinned_tag / remove_pinned_tag keep their signatures; they resolve the path and the state and hand the edit to the guarded cycle. save_pinned_tag_at / remove_pinned_tag_at take the lock and path directly, which is what makes the race testable without a live AppHandle.

Three decisions worth stating

The lock does not cover plain reads. list_pinned_tags stays unlocked. The file is only ever replaced by atomic_write's rename, so a reader racing a writer opens either the whole previous list or the whole next one — both lists Markpad actually wrote. The read is safe; the cycle is not.

Poisoning is recovered, matching the rest of the file. lock_recover already does this for window_registry, startup_files and the watcher map. The argument extends here even though this mutex guards a file: atomic_write publishes by rename, so a panic inside the cycle leaves the pre-existing pinned-tags.json intact — there is no half-applied state for the next holder to inherit. Propagating the poison would instead disable pinning for the rest of the session.

No other Rust command has this shape. Grepped every read_to_string / fs::write / atomic_write in lib.rs, setup.rs, tab_transfer.rs. save_theme, save_file, save_file_binary, save_window_state and the VSIX theme install all write a whole value handed down from the frontend — no read step in Rust, so no cycle. save_window_state shares a file across windows but windowSession.persistState gates on isMainWindow, so only one window writes it. The broker in tab_transfer.rs is in-memory and already Mutex-guarded. setup.rs runs once at install time. pinned-tags.json is the only one.

Tests

concurrent_edits_do_not_overwrite_one_another runs 8 savers and 8 removers × 4 rounds against one file in a private temp directory (never the real app_config_dir) and asserts the property: the final file contains exactly the keep-* pins the writers asked for and none of the doomed-* ones they removed.

It asserts the surviving set rather than any mechanism on purpose. This repo has been bitten twice by tests that were green on macOS for the wrong reason — a race test that passed because the interleaving did not occur, and an is_err() assertion on a branch macOS never executes. The set of surviving tag names is what the user loses when this breaks, and it is checked identically on every platform.

Counter-proof, with the lock removed and nothing else changed (5 runs, macOS/APFS):

	pins that survived	unpins that stuck
unlocked (master's shape)	1–4 of 8	1–5 of 8
locked	8 of 8	8 of 8

The unlocked runs also failed outright with File exists (os error 17) and No such file or directory (os error 2). atomic_write names its temp file from the target name, the pid and a nanosecond clock reading; two threads of one process that land on the same reading collide on create_new, and the loser's cleanup then deletes the temp file the winner was about to rename. Serialising the cycle removes that exposure for this file too. Recorded in the doc comment.

Two supporting tests: re-pinning a tag updates it in place rather than appending a duplicate, and a writer that panics mid-cycle does not lock out the next one.

Verification

cargo test    139 / 139   (136 before, +3)
cargo clippy  3 warnings  — the pre-existing baseline, none from this change
npm test      540 / 540
npm run check 0 errors, 0 warnings

cargo fmt --check is clean for window_runtime.rs; the repo's 52 pre-existing diffs are all in lib.rs / setup.rs and were left alone.

Not covered

• Cross-process. The mutex is per-process. Two Markpad processes editing the file would still race; tauri-plugin-single-instance is what makes that not the normal case, and no file lock was added.
• The other atomic_write call sites. The temp-name collision above is a property of atomic_write, not of this file. It needs two threads writing the same target at the same nanosecond, which for documents means two tabs on one file — narrowed by fix(tabs): one tab per file path #413 and fix(tabs): ask the filesystem whether two paths name the same file #416 but not proven impossible. Reported here rather than fixed; a fix belongs with atomic_write.
• read_pinned_tags_at's silent fallback. An unreadable or unparseable file still yields an empty list, which the next write then persists. Under the lock this is only reachable through damage from outside Markpad, so the behaviour is unchanged and documented rather than altered.
• No live multi-window run. Verified by threads against the real functions, not by quitting two tagged windows by hand.

🤖 Generated with Claude Code