Two lost-work defects behind the preferences flake family (#333, #341) - #348
Merged
Bidthedog merged 3 commits intoAug 29, 2026
Merged
Conversation
A watcher read is not instantaneous: it opens settings, the active theme and keybindings in turn, and 032 FR-008 makes it retry for up to ~100ms when it catches a partial write. A config write can commit inside that window, so the payload the read produces describes a file that has stopped saying it — and the broadcast arrives at the renderer AFTER the renderer adopted the write that superseded it. The renderer cannot defend itself, which is why the guard is here and not there. Its onChange replaces the whole state because a broadcast is supposed to be the truth, and nothing in the payload ever said which moment it was the truth at. The damage is worse than a stale render, because the preferences tabs compose their next edit from what they were last told. Measured on an idle machine: remove a chord from zoom.in (file correct), the stale broadcast reverts the renderer, remove a chord from zoom.out, and THAT write - a whole document composed from the reverted copy - puts zoom.in back to its shipped value on disk. The Reset control then correctly reports the row as un-overridden, so the click preferences-reset.e2e.ts makes next can never become actionable. FileConfigStore now counts committed writes. readConfigOnce captures that count BEFORE it reads - the only safe order, since capturing afterwards would stamp a stale document with a fresh generation and make it look current, while capturing first can only make a current document look stale, at the cost of one suppressed broadcast the write's own file event immediately replaces. startConfigWatcher tests it at the moment of sending rather than when the read returned, so the remaining window is a single synchronous step. Measured, preferences-reset.e2e.ts alone, idle, one worker, retries off: 5 failed / 14 before, 1 failed / 14 after, and the US1 keybinding failure this diagnosis started from is gone. The survivor is :217, a separate defect. Refs #333, #341
The preferences window rendered its editors from the SHIPPED DEFAULTS while config.get() was still in flight. That read is a real round trip - it re-reads three documents and enumerates the icon-pack directory - so the window was interactive while it was still wrong, and two things went wrong inside the gap. The on-entry snapshot is captured on the render where `loaded` first turns true. An edit made before then is already in the payload that resolves the load, so the snapshot records the EDITED value, and "revert every editor to its state when this window opened" restores the very thing the user was discarding. planRevertAll does not diff - it emits a change for every revertable leaf - so the write is not skipped; the value written is the wrong one. The Key Bindings tab is the worse half: it composes WHOLE documents from what it currently holds. Held before the load, that is the shipped defaults, so a single edit there would write the default keybindings over the user's real ones. Neither is recoverable afterwards, because the window never saw the state it would have to revert to. Retrying the read does not help either: a re-read returns FRESHER content, never the state the window opened with. So the window must not accept the edit at all, which is what this gate does. The toolbar stays mounted; the editors wait. The read resolves in a few milliseconds, so nothing is visible - what it removes is the window between the frame being interactive and it being right. Reproduced deterministically at the component layer by holding config.get() open, which is the ordering the E2E hits by chance. Measured, preferences-reset.e2e.ts alone, idle, one worker, retries off: :217 failed 2 of 14 at baseline, 1 of 14 after the watcher fix, and 0 of 16 now. The run's remaining failure is :486, a further distinct shape #341 names. Refs #341
The three case studies already here are all the test's fault - it read something before the thing that writes it had run. The two added by this branch are not, and the distinction matters because the first instinct on meeting them is to add another wait, and no wait fixes either. Records both mechanisms, what the user loses in each, and the measurements that separated them: 5/14 before, 1/14 after the first fix, 0/16 after the second, then a full gate at 560 E2E with 0 flaky. Refs #333, #341
Bidthedog
added
bug
This was referenced Aug 29, 2026
Bidthedog
deleted the
feature/I333-I341-I286-I342-I347-gate-reliability
branch
August 29, 2026 19:49
Bidthedog
added a commit
that referenced
this pull request
Aug 30, 2026
…g else derives Same eight files as the alpha2 bump, by the same mechanism - the root package.json is the single source publish-gates.mjs reads, and every workspace and the lockfile follow it. The four-way match that gates a publish is filename == package == verdict == tag, so this has to land before v1.0.0-alpha3 is cut, not alongside it. Since alpha2, master has gained the two lost-work defects fixed in #348: a watcher broadcast that could be older than the write it landed after, which the preferences tabs then wrote back to disk, and a preferences window that accepted edits before it had loaded, which made "revert to how this window opened" restore the value being discarded.
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
1 participant
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.
Two product defects, both lost work, both found while chasing the
@prefs @reserve:windowflake family in #341. Neither is a test problem, and no amount of waiting in the test fixes either — the app was telling the renderer something untrue.The watcher could broadcast a read a write had overtaken
A watcher read is not instantaneous: it opens three documents and enumerates the icon-pack directory, and 032 FR-008 makes it retry for up to ~100 ms when it catches a partial write. A config write can commit inside that window, so the payload describes a file that has stopped saying it — and it reaches the renderer after the renderer adopted the write that superseded it.
The renderer cannot defend itself.
onChangereplaces the whole state because a broadcast is supposed to be the truth, and nothing in the payload ever said which moment it was the truth at.The damage is worse than a stale render, because the preferences tabs compose their next edit from what they were last told:
zoom.in— the file is correct.zoom.out— that write is a whole document composed from the reverted copy, so it putszoom.inback to its shipped value on disk.The user's first edit is silently gone. The Reset control then correctly reports the row as un-overridden, which is why the failure presents as a click that never becomes actionable rather than as a wrong value.
FileConfigStorenow counts committed writes;readConfigOncecaptures that count before it reads, and the watcher drops any payload the count has outrun. Capture order is load-bearing — capturing afterwards would stamp a stale document with a fresh generation and make it look current.Preferences accepted edits before it had loaded
The editors rendered from the shipped defaults while
config.get()was in flight. Two things break in that gap, and neither is recoverable afterwards, because the window never saw the state it would have to revert to:loadedfirst turns true, so an edit made before then is already in the payload that resolves the load. "Revert every editor to its state when this window opened" therefore restores the value the user was discarding.planRevertAlldoes not diff — it emits a change for every revertable leaf — so the write is not skipped; the value written is the wrong one.Retrying the read cannot fix either: a re-read returns fresher content, never the state the window opened with. So the editors now wait for
loaded. The toolbar does not, and the read resolves in a few milliseconds, so nothing is visible — what it removes is the window between the frame being interactive and it being right.Measurements
preferences-reset.e2e.tsalone, idle machine, one worker, retries off:Full
npm run gateon this tree: GREEN, all 8 stages, 25m 33s — 560 E2E, 0 flaky.Both regression tests are non-vacuous — each was watched failing against the unfixed code before the fix landed.
What this does NOT resolve
Stated plainly so the remaining issues are not assumed handled:
preferences-reset.e2e.ts:486, seen once in 16 whole-file runs and never in 20 isolated ones, so its cause is outside the test.Closes
Closes #333
Related
Related to #341 — two of its mechanisms are fixed here;
:486remains open thereRelated to #286 — passed in this branch's full gate, still not reproducible
Related to #342 — 0/16 across the worker A/B; reads as a budget question, not a defect
Related to #347 — passed in this branch's full gate, unmeasured