fix(frontend): write the config atomically, so an interrupted save cannot destroy it - #420
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
494e7c7 to
693c7b4
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens the frontend config persistence path by making Config::save_to resilient to interruption: it writes to a sibling temporary file and atomically renames it over config.toml, preventing partial/truncated writes from destroying user configuration.
Changes:
- Replace direct
fs::write(path, ...)with “write temp + rename over target” to avoid truncation windows. - Add a unit test that verifies a successful save leaves no temp file behind, round-trips correctly, and that subsequent saves replace the previous config.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…mports
Two review points, both small and both real.
The `shader_presets` note said "same correction as `hd_packs` above".
`hd_packs` is defined eighteen lines BELOW it. A cross-reference that
sends the reader the wrong way is worse than none, and it is the kind of
error that survives because nobody checks a direction word.
The test had been inserted above the module's `use` statements. The
insertion point right after `mod tests {` is before the imports, not
after -- the same slip caught on #420, from the same habit of anchoring
on the `mod tests {` line.
…mports
Two review points, both small and both real.
The `shader_presets` note said "same correction as `hd_packs` above".
`hd_packs` is defined eighteen lines BELOW it. A cross-reference that
sends the reader the wrong way is worse than none, and it is the kind of
error that survives because nobody checks a direction word.
The test had been inserted above the module's `use` statements. The
insertion point right after `mod tests {` is before the imports, not
after -- the same slip caught on #420, from the same habit of anchoring
on the `mod tests {` line.
de7f517 to
17109f5
Compare
|
The blocking finding is correct, and it is the sharpest one this PR has had: the change stopped one step short of the guarantee it claimed. Fixed in fsync — accepted
Now Unique scratch name — accepted, without the dependencyThe temp file now carries the process id. Your reasoning holds exactly: a bare
What the fix did to the testsThe leftover-scratch assertion had to change, and the reason is worth stating: it rebuilt A mutation then exposed a real gap: deleting the cleanup on the rename-failure path left every test passing. One thing I will not claimDropping the The verbosity nitpickSame standing answer as the other PRs: |
…#414) * docs(changelog): backfill the empty [Unreleased] section, and reject the gate for it `[Unreleased]` was empty while three merged PRs carried user-visible change: #407's Divergence Lens (a whole new tool panel and the v2.3.8 "Parallax" marquee), #409's two-acquisition lock-race measurement, and be the single source of truth for user-visible change did not mention any of them. Backfilled now rather than at cut time. The reasons are recoverable from the commit bodies today and would have to be reconstructed from diffs later, which is how a release section ends up describing what changed instead of why. The obvious gate -- a PR touching `crates/*/src/**` must also touch `CHANGELOG.md` -- was measured against the last 50 first-parent merges before being proposed, per this release's own bar that a CI change is demonstrated against real history rather than argued. It would have gone red on 8 of them. Three are the genuine misses above. The other five are this repo's normal workflow: features land bare and the release-cut PR composes the whole section at once (the probe engine, the Latency Oracle, the RAM Atlas, the throttle instrument), plus one docs PR that touched only `//!` preambles. A 62% false-positive rate against the project's own history. A gate that fires on the normal workflow is suppressed within a week, and a suppressed gate is worse than no gate because it still reads as coverage. Rejected as specified, recorded with its numbers per the convention `docs/performance.md` sets for rejected optimizations. The measurement also relocates the defect. The failure mode is not "a feature landed without an entry" -- at the moment each of those PRs merged, nothing was wrong. It is that v2.3.8 landed its marquee and was never cut, so the cut PR that writes the section never ran and v2.3.9 opened on top of an empty [Unreleased]. No per-PR gate addresses that. What would is a release-ceremony check keyed on the cut, which is named in the plan as a decision rather than built here. * docs(changelog): the Divergence Lens entry described one tenth of #407 Review was right on both counts. The entry cited #407 as "v2.3.8 'Parallax' item A" and described only the pixel localisation, omitting that the feature is reachable at all. #407 is the whole of v2.3.8. Beyond item A it lands the frontend panel under Tools -> Analysis, trial-scoped provenance capture, an AUDIO lens resolving a divergence to the CPU cycle, and the pixel-cause explanation that closes item B without bisection -- so a located difference comes back as a cause rather than a coordinate. It also carries a real fix found inside the work: the Lens left the emulator thirty frames ahead of where it started, because a trial restores the anchor on the way IN and not on the way out (deliberate -- it is what lets the Lens read the trial's final frame off `nes`) and the outermost caller never put the timeline back. An entry that omits the panel describes a library, not a release. This is the reason the backfill exists at all, reproduced at smaller scale inside the backfill: the further a section is written from the work, the more of it is missing. * docs(changelog): drop the note about the changelog's own maintenance Review is right that this belongs elsewhere. The paragraph explained that #407, #409 and #410 merged without an entry -- a fact about how this document is maintained, not about what changed for a user. The CHANGELOG's own header already says it is the record of user-visible change, so a section describing its gaps is the one thing in it that is not. It is not lost: the full account, including the measured rejection of the per-PR gate and its 62% false-positive rate, is item D of the v2.3.9 plan, and the reasoning is in this branch's commit bodies. The companion nitpick -- that the entries read like architecture decision records rather than concise release notes -- is declined. That is this project's CHANGELOG voice, not an accident of this PR: every released section explains the mechanism and what was believed before the measurement. Matching a generic house style would make these entries inconsistent with the file they are joining. * fix(frontend): omit the latency map when empty, so the claim about it is true Review found that the entry claimed something `#[serde(default)]` does not provide, and the claim was wrong rather than imprecise. `serde(default)` covers LOADING a config that lacks the key. It says nothing about SAVING, and the TOML serializer emits an empty table for an empty map -- so a user who never opened the Latency Oracle would have had their config rewritten with a bare `[input.latency_reports]` on the first save after upgrading. Checked rather than reasoned about: serializing a default `Config` and grepping the output puts the table at line 100. Fixed by making the claim true rather than by weakening it. A `skip_serializing_if` keeps the key out of the file until there is something to store, and the CHANGELOG now separates the two guarantees instead of attributing both to `default`. The same false claim turns out to sit on two SHIPPED fields -- `graphics.hd_packs` (v1.5.0) and `graphics.shader_presets` (v1.2.0) -- both saying a pre-feature config "is byte-identical" when it is only byte-identical until the first save. Those are corrected in PROSE only, deliberately: adding `skip_serializing_if` there would change the file two shipped features write, which is a separate decision with its own risk, and the wrong half was the claim rather than the behaviour. The distinction now stated at each site is that `serde(default)` is a LOAD guarantee. Two mutations, both directions. Removing `skip_serializing_if` fails the test -- it is the original defect -- and so does an over-eager `skip_serializing_if` that always returns true, which would silently discard real measurements. A one-directional test here would have passed against a field that never persists anything at all. * docs(config): correct a direction word, and move the test below the imports Two review points, both small and both real. The `shader_presets` note said "same correction as `hd_packs` above". `hd_packs` is defined eighteen lines BELOW it. A cross-reference that sends the reader the wrong way is worse than none, and it is the kind of error that survives because nobody checks a direction word. The test had been inserted above the module's `use` statements. The insertion point right after `mod tests {` is before the imports, not after -- the same slip caught on #420, from the same habit of anchoring on the `mod tests {` line. * test(config): round-trip the latency map, not just its key Review is right that the string checks were half a test. `contains( "latency_reports")` verifies the KEY and says nothing about the VALUE, so a `skip_serializing_if` on a field whose `Deserialize` had drifted would still produce exactly the right text and load back as something else -- and it is the load side the documentation promises. Both directions now round-trip: the empty config re-parses to an empty map (so the omitted key is genuinely equivalent to an absent one, which is the whole claim), and the populated one re-parses to the same `RememberedLatency` it was given.
aa951a1 to
a0ec8fc
Compare
|
Two of the three blocking findings hold and are fixed in Symlink destruction — accepted, and the best catch on this PRThis is a regression the fix itself would have introduced, not a pre-existing bug. The path is now resolved with Pinned by Concurrency — accepted, with the reachability statedThe scratch name now carries a module-level atomic counter as well as the pid. Config saves are driven from the UI thread, so two concurrent I would rather say this than imply coverage: removing the counter is caught by no test, because a collision needs two threads inside this function at once and the callers cannot produce that. I ran the mutation; it passes. ( Let-chains — declined, not reproducible
Let-chains are stable in edition 2024. Checked rather than argued:
Suite at 542. |
|
One real hardening accepted, one suggestion accepted, one claim declined on the same evidence as last round. CWE-377 — accepted, and the ordering is the interesting part
Two things worth recording:
Permissions TOCTOU — acceptedThe mode is applied at creation via
Let-chains — declined again, same evidence
A compile error would not be subtle. This is the second round for this finding; if there is a specific toolchain or edition where it reproduces, that would be worth knowing, but the shipped configuration is not it. Verification
|
2eed54d to
cad45ab
Compare
`fs::write` truncates the target and then writes. Anything that interrupts it -- a crash, a kill, a full disk -- leaves the user holding a truncated or empty `config.toml`, which is every keybinding, palette, shader preset, HD-pack mapping and per-game setting they have. That window stopped being theoretical when saves became automatic. The function is called from more than a dozen places and several are not user actions at all: closing a ROM, moving a mixer slider, and (v2.3.9) finishing a Latency Oracle measurement all save without being asked. Now write to a SIBLING scratch file, fsync it, and rename over the target. Each part earns its place, and two of the three came from review rather than from the first draft. SIBLING, because across a filesystem boundary `rename` is not a rename -- a `$TMPDIR` on another mount would silently degrade this back to a copy, reintroducing the failure being fixed. FSYNC, because `fs::write` returns once the bytes are in the page cache, not once they are on the medium. `rename` is atomic with respect to other processes, but a power loss between the two leaves the directory entry pointing at a file whose contents never reached disk -- exactly the outcome this function claims to prevent. The parent directory is synced afterwards too, best-effort and Unix-gated, since on POSIX the entry `rename` creates is itself only a cache update until the directory is synced. A PROCESS-ID in the scratch name, because a bare `.tmp` is shared: two RustyNES instances saving at once would write the same file and one would rename the other's half-written bytes over the config. A stale scratch file from a crashed run also cannot block a later save, since that run has a different id. `tempfile::NamedTempFile` would do this more tidily and is deliberately not used -- `tempfile` is a DEV-dependency, and promoting it to a runtime dependency of a shipped binary is a supply-chain decision, not a cleanup. The existing file's PERMISSIONS are carried onto the replacement. This is the one thing write-then-rename gives up relative to a truncating write: `fs::write` preserves an existing file's mode, while a fresh scratch file takes the umask default and the rename carries that mode with it. A user who had tightened `config.toml` to 0600 would have found it quietly widened by an automatic save they never asked for. Failure ordering is deliberate. A failed rename leaves the old config intact -- the stale-but-valid file is the one worth keeping -- and both failure paths remove the scratch file so a full disk does not accumulate them, best-effort because that removal can fail for the same reason the write did. Three tests. The leftover-scratch assertion reads the DIRECTORY rather than reconstructing a filename, because with a pid in the name a reconstruction would check a file no implementation creates -- an assertion that passes for the wrong reason. The permissions assertion was written before its fix and FAILED (`left: 420, right: 384`), which is stronger evidence than a mutation. And a mutation exposed a real gap: deleting the rename-failure cleanup left every test passing, so `a_failed_rename_cleans_up_after_itself` covers it by renaming onto a directory, which fails portably. Stated rather than papered over: dropping the `sync_all` is caught by NO test here. Durability is only observable across a power loss, which an in-process test cannot stage. The fsync is justified by the mechanism, not by coverage. NOT extended to `save_state.rs` and `cheats.rs`, which use the same truncating `fs::write` on user data -- and a truncated save state is arguably worse, being a user's game progress. They want the same treatment through a shared helper rather than a third copy of this reasoning, which is a wider change than this one.
…unique per call Two of three blocking findings hold. The third does not. SYMLINK -- real, and a regression the fix itself would have introduced. `fs::write` follows a symlink and writes through to its target; `fs::rename` replaces the link. A user who has symlinked `config.toml` into a dotfiles repository, which is a common setup, would have found the link silently converted to a regular file on the first automatic save, and their repository would stop receiving changes. The path is now resolved with `canonicalize` before anything is written, and everything downstream -- the scratch sibling, the permission copy, the rename, the directory sync -- uses the resolved target. `canonicalize` fails when the file does not exist yet, which is the first-save case, so it falls back to the path as given. Pinned by `saving_through_a_symlink_writes_the_target_and_keeps_the_link`, and the mutation that reverts the resolution fails it. CONCURRENCY -- accepted, though not reachable today. Config saves are driven from the UI thread, so two concurrent `save_to` calls cannot happen; but that is a property of the callers rather than of this function, and a relaxed fetch-add on a module-level counter makes the guarantee structural for one instruction. Stated plainly: removing the counter is caught by NO test, because a collision needs two threads inside this function at once and the callers cannot produce that. LET-CHAINS -- declined, not reproducible. The claim was that `if let Some(..) = .. && let Ok(..) = ..` is unstable and will not compile. Let-chains are stable in edition 2024, `Cargo.toml` sets `edition = "2024"` and `rust-toolchain.toml` pins 1.96.0, `main` already carries the same construct in `debugger/mod.rs:2292`, and this branch compiles clean under `-D warnings` across the workspace and all four wasm32 combinations. The counter is a module-level `static` rather than a function-local one because clippy's `items_after_statements` fires on the latter -- and it is right that an item declared mid-function reads as if it were scoped to that point when it is not.
…et's mode Two review points, one a real hardening and one a repeat of a claim the build refutes. CWE-377 -- accepted. `File::create` follows symlinks and truncates, so a predictable scratch name is a surface: something pre-created at that path as a link to another file would be silently truncated and overwritten by the save. `create_new(true)` makes the open exclusive, so anything already there is a failed save rather than a destroyed file. Two things worth recording about it. The scratch file is a sibling of the user's own config, not a world-writable directory, so an attacker who can plant a file there already owns the config -- this is defence in depth rather than a live hole, and it costs one call. And exclusive creation was NOT safe to adopt two commits ago: with a bare `.tmp` name, a stale file from a crashed run would have failed every subsequent save. The pid and per-call counter removed that failure mode, so the objection that ruled it out no longer applies. The order the three changes landed in mattered. Permissions TOCTOU -- accepted. The mode is now applied at creation via `OpenOptionsExt::mode` rather than chmod-ed afterwards, closing a window in which the file existed at the umask default -- briefly wider than the config the user had tightened. `open(2)` masks the requested mode with the umask, so creation alone can land narrower than the original; the exact mode is still set after, which makes the pair narrow-then-correct rather than widen-then-narrow. Let-chains -- declined again, same evidence. Let-chains are stable in edition 2024; `Cargo.toml` sets it, `rust-toolchain.toml` pins stable 1.96.0, `main` carries the same construct at `debugger/mod.rs:2292`, and this branch compiles clean under `-D warnings` across the workspace and all four wasm32 combinations. A compile error would not be subtle. `an_occupied_scratch_path_fails_the_save_without_truncating_it` plants a symlink decoy at every scratch name the process can produce next and asserts both halves: the decoy's target is not truncated, and the existing config still holds the PREVIOUS save rather than being damaged by the failed one. Reverting `create_new` to `create` fails it.
Review found the symlink fix surviving inside its own gap, and it is the more likely of the two cases in practice. An INTACT link resolves through `canonicalize`. A BROKEN one -- a freshly created dotfiles link whose target does not exist yet -- makes `canonicalize` fail with `NotFound`, and the fallback then used the link's own path, so the rename destroyed exactly the setup the resolution exists to protect. `ln -s` then launch is the natural order, so the broken case is the one a user hits first. Resolution is now its own function with the two cases named. `read_link` is what handles the broken link, and its `EINVAL` on a non-symlink is how "nothing to follow" is distinguished from "broken link" without a second `symlink_metadata` call. A relative destination is joined to the link's own directory. A mutation then exposed something about the fix rather than the bug: removing the `canonicalize` branch entirely did NOT fail the intact-link test, because `read_link` satisfies it -- one hop is enough for one link. It is not enough for a CHAIN: `read_link` resolves exactly one level, so `a -> b -> c` would write to `b` and destroy the second link. `saving_through_a_chain_of_symlinks_reaches_the_real_file` covers that, and removing `canonicalize` now fails it. The branch was load-bearing all along; nothing said so. Three symlink tests where there was one, each mutation-checked: intact, broken, and chained. The remaining review points are answered rather than changed. The `let _ =` on `set_permissions` and on the directory `sync_all` are best-effort BY DESIGN -- failing a save because a chmod or a directory fsync failed would cost the user the settings change they asked for, to protect a property that is already a hardening rather than the correctness guarantee. And `canonicalize` swallowing a non-`NotFound` error is now moot: every failure falls through to `read_link`, which either resolves the link or returns the path unchanged, and any genuine permission problem surfaces at the open or the rename with a real error. Let-chains, third round: stable in edition 2024, `main` carries the same construct at `debugger/mod.rs:2292`, and `CI success` is green on this very PR. That check is the disproof.
…t an orphan
One blocking claim refuted by measurement, its accompanying suggestion
accepted, and a related point from the v2.4.0 plan review applied to the
code it describes.
THE BARE-FILENAME SAVE FAILURE DOES NOT REPRODUCE. The claim was that
`Path::new("config.toml").parent()` yields `Some("")` and that
`create_dir_all("")` then fails with ENOENT, aborting the save. The first
half is right and the second is not -- measured rather than argued:
parent() -> Some("")
create_dir_all("") -> Ok
File::open("") -> Err(No such file or directory)
So the save is unaffected. What the third line shows is that the
SUGGESTION was right for a different reason: the parent-directory fsync
sat behind `let Ok(dir) = File::open(parent)`, so for a relative target it
silently did not happen. A durability step quietly skipped is worse than
one that fails, because nothing reports it. An empty parent now resolves
to `.`, which is the directory it means.
RETRY PAST AN OCCUPIED SCRATCH NAME. Review of the v2.4.0 plan raised
pid reuse against the naming scheme, and it lands on this code:
`create_new` turns a collision into a failed save, and a crashed run can
leave an orphaned scratch file that a later run with the same pid meets
on its first save. Unlikely, and a lost save is a real cost to a user who
would have no idea why. Advancing the counter and opening once more turns
it into nothing at all, since the counter only increases within a
process.
Stated rather than implied: the retry is NOT covered by a test, and the
mutation that removes it passes. A test would have to predict which
scratch name the next save picks, and the counter is process-global, so
predicting it means reimplementing the naming inside the test -- an
assertion that agrees with itself. That is the third property here
justified by mechanism rather than coverage, alongside the `sync_all` and
the counter itself, and each is named in place rather than left to be
assumed covered.
The existing occupied-scratch test still passes for the right reason: it
plants decoys at sixty-four consecutive names, so the retry finds one too
and the save still fails without truncating anything.
992427d to
3a6a33d
Compare
Antigravity review (Gemini via Ultra)This PR updates the configuration save mechanism to use an atomic write-then-rename strategy, ensuring durability against interruptions while preserving symlinks and file permissions. Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
Followed on from #414's review finding, which sent me to read
Config::save_toproperly rather than just the derive above it.The window
fs::writetruncates the target and then writes. Anything that interrupts it — a crash, a kill, a full disk — leaves the user holding a truncated or emptyconfig.toml. That file is every keybinding, palette, shader preset, HD-pack mapping and per-game setting they have.It stopped being theoretical when saves became automatic.
save()is called from more than a dozen places and several are not user actions at all — closing a ROM, moving a mixer slider, and (v2.3.9) finishing a Latency Oracle measurement all save without being asked. A user who never opens Settings can still be mid-save when something goes wrong.The fix, and the two details that matter
Write to a sibling temp file, then
renameover the target.renameis not a rename, so a$TMPDIRon another mount would silently degrade this back to a copy — the failure mode being fixed, reintroduced by the fix.renameis atomic on both shipped platforms. POSIX guarantees it;std::fs::renamemaps toMoveFileExwithMOVEFILE_REPLACE_EXISTINGon Windows, so overwriting an existing file works there too (asserted in the test).The failure ordering is deliberate. A failed rename leaves the old config intact and the temp file behind — the stale-but-valid file is the one worth keeping. A failed write removes the temp file so a full disk does not accumulate them, and that removal is itself best-effort, because it can fail for the same reason the write did.
Verification
Two mutations, each failing the test:
renamepathagainThe test asserts three things, because each alone is satisfiable by a broken implementation: no temp file left behind, the value round-trips, and a second save replaces the first (the
rename-over-existing case).Gates: fmt, workspace clippy,
full, both wasm32 invocations, rustdoc, suite at 535.Deliberately not extended
save_state.rsandcheats.rsuse the same truncatingfs::writeon user data — and a truncated save state is arguably worse than a truncated config, since it is a user's game progress. They want the same treatment via a shared helper rather than a third copy of this comment, which is a wider change than the review thread that produced this one justifies. Named here so it reads as a decision.