From 3187f8a6af6c1e4620137d1562d99bf25e3227aa Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 14:30:46 -0400 Subject: [PATCH 1/2] fix(frontend): report a failed latency-config save instead of swallowing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review flagged the ignored `save()` result as blocking — the style guide names swallowed errors explicitly — and it was right for a reason sharper than the general rule: the thing being hidden is the measurement silently not being persisted, which is a quieter version of the bug that block was written to fix. I merged before acting on the finding, so this is the follow-up. Two channels, because they reach different people. Stderr, matching the FDS BIOS path's existing convention for a config write that matters, and the panel's own status line for the user who is looking at the panel and nowhere else — they opened it to ask about this game, so "your measurement was not saved" belongs beside the answer. Looking at the failure path turned up a second defect of my own in the same block. `take_unpersisted` marks the value written BEFORE the caller's save runs, so a failed save was already recorded as a success. That is now explicit rather than accidental: the failure is reported and the value STAYS marked, deliberately. Un-marking would look more diligent and be worse. It would retry on the very next frame, so a full disk or a read-only config becomes a per-frame write attempt and a per-frame log line — one failure amplified into a flood. The user has been told, the measurement is still on screen, and re-measuring is an explicit retry. Both halves are pinned by tests, including the one asserting no retry, because the tempting change is the one that silently degrades. Also reworded a comment. It quoted the anti-pattern verbatim, and my own verification grep then matched the COMMENT rather than the code and reported a swallowed save that no longer existed. The same lexical trap `AGENTS.md` records for closing keywords, where quoting the pattern reproduces it — so the comment now describes the form instead of spelling it. Verified: fmt, workspace clippy, `debug-hooks` and `full` combos, both wasm32 invocations, rustdoc, frontend suite at 531. Co-Authored-By: Claude Opus 5 --- .../src/debugger/latency_panel.rs | 66 +++++++++++++++++++ crates/rustynes-frontend/src/debugger/mod.rs | 18 ++++- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/crates/rustynes-frontend/src/debugger/latency_panel.rs b/crates/rustynes-frontend/src/debugger/latency_panel.rs index 55932ac6..e3f32a4c 100644 --- a/crates/rustynes-frontend/src/debugger/latency_panel.rs +++ b/crates/rustynes-frontend/src/debugger/latency_panel.rs @@ -139,6 +139,23 @@ impl LatencyPanel { Some(current) } + /// Report that the config write for the last `take_unpersisted` failed. + /// + /// Surfaces the failure where the user is already looking — they opened this + /// panel to ask a question about this game, and "your measurement was not + /// saved" belongs beside the answer rather than only in a terminal they may + /// not have. + /// + /// **Does NOT un-mark the value as persisted**, deliberately. Un-marking + /// would retry on the very next frame, so a full disk or a read-only config + /// becomes a per-frame write attempt and a per-frame log line — turning one + /// failure into a flood. The user has been told, the measurement is still on + /// screen, and re-measuring is an explicit retry. That trade is worth stating + /// because the opposite choice looks more diligent and is worse. + pub fn note_persist_failure(&mut self, err: &dyn core::fmt::Display) { + self.status = format!("Measured, but could not be saved: {err}"); + } + /// Whether this session already holds a report — measured or remembered. /// /// The restore is driven off this rather than off a ROM-load hook, so it @@ -632,6 +649,55 @@ mod tests { assert_eq!(second.frames, 1); } + /// A failed save must be visible in the panel, not only on stderr. + /// + /// The user opened this panel to ask about this game; "your measurement was + /// not saved" belongs beside the answer. A `let _ = save()` — the form review + /// flagged — put it nowhere at all. + #[test] + fn a_failed_save_is_reported_in_the_panel() { + let mut panel = LatencyPanel { + report: Some(report(Some(3), Confidence::Unanimous)), + frame_ms: 16.639, + ..LatencyPanel::default() + }; + let taken = panel.take_unpersisted(); + assert!(taken.is_some(), "premise: there was something to write"); + + panel.note_persist_failure(&"permission denied"); + + assert!( + panel.status.contains("could not be saved"), + "the failure must be stated, not implied: {}", + panel.status + ); + assert!( + panel.status.contains("permission denied"), + "and it must carry the reason: {}", + panel.status + ); + } + + /// A failure does NOT schedule a retry on the next frame. + /// + /// Un-marking would look more diligent and be worse: a read-only config + /// would then produce a write attempt and a log line every frame. The user + /// has been told and re-measuring is an explicit retry. + #[test] + fn a_failed_save_does_not_retry_every_frame() { + let mut panel = LatencyPanel { + report: Some(report(Some(3), Confidence::Unanimous)), + frame_ms: 16.639, + ..LatencyPanel::default() + }; + assert!(panel.take_unpersisted().is_some()); + panel.note_persist_failure(&"disk full"); + assert!( + panel.take_unpersisted().is_none(), + "a failed write must not become a per-frame write attempt" + ); + } + /// A RESTORED measurement came from the config, so it must not be written /// back — otherwise opening a game would rewrite the file for nothing. #[test] diff --git a/crates/rustynes-frontend/src/debugger/mod.rs b/crates/rustynes-frontend/src/debugger/mod.rs index 104eb3d2..17a70e16 100644 --- a/crates/rustynes-frontend/src/debugger/mod.rs +++ b/crates/rustynes-frontend/src/debugger/mod.rs @@ -2319,9 +2319,21 @@ impl DebuggerOverlay { && let Some(key) = nes.as_deref().map(Self::rom_key_of) { config.input.latency_reports.insert(key, remembered); - // Best-effort, matching `hd_packs`: a failed write must not take - // down the UI, and the measurement is still on screen either way. - let _ = config.save(); + // Reported, not swallowed. An ignored `save()` result was the + // first form here and review flagged it: the style guide names + // ignored return values as a blocking issue, and this one hides + // the exact failure the whole feature exists to prevent — the + // measurement silently not being persisted. A quieter version of + // the bug this block was written to fix. (Review on #410.) + // + // Two channels because they reach different people: stderr for a + // terminal launch, matching the FDS BIOS path's convention, and + // the panel's own status line for the user who is looking at the + // panel and nowhere else. + if let Err(e) = config.save() { + eprintln!("rustynes: could not persist the latency measurement: {e}"); + self.latency_ui.note_persist_failure(&e); + } } } // v2.3.6 workstream C — the RAM Atlas. Needs `&mut Nes`: it observes a From a3cb2d899bfed268c01b87291889d8d2c0ea81ec Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 15:49:43 -0400 Subject: [PATCH 2/2] refactor(frontend): take the failure by `impl Display`, and stop quoting the anti-pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both accepted. Copilot: the test's doc comment still contained the literal discard expression, which is the exact string this PR's own description records as having matched the COMMENT rather than any code during verification — reporting a defect that was already fixed. Removed, with the reason written down in its place so the next author does not helpfully restore it for clarity. Antigravity: `impl core::fmt::Display` is the more idiomatic signature than `&dyn core::fmt::Display` here, and it lets the two test call sites drop a `&` around a string literal. The dynamic form bought nothing -- there is one production call site on a cold path, so there is no code size to trade away. --- .../src/debugger/latency_panel.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/rustynes-frontend/src/debugger/latency_panel.rs b/crates/rustynes-frontend/src/debugger/latency_panel.rs index e3f32a4c..eb84dad7 100644 --- a/crates/rustynes-frontend/src/debugger/latency_panel.rs +++ b/crates/rustynes-frontend/src/debugger/latency_panel.rs @@ -152,7 +152,7 @@ impl LatencyPanel { /// failure into a flood. The user has been told, the measurement is still on /// screen, and re-measuring is an explicit retry. That trade is worth stating /// because the opposite choice looks more diligent and is worse. - pub fn note_persist_failure(&mut self, err: &dyn core::fmt::Display) { + pub fn note_persist_failure(&mut self, err: impl core::fmt::Display) { self.status = format!("Measured, but could not be saved: {err}"); } @@ -652,8 +652,12 @@ mod tests { /// A failed save must be visible in the panel, not only on stderr. /// /// The user opened this panel to ask about this game; "your measurement was - /// not saved" belongs beside the answer. A `let _ = save()` — the form review - /// flagged — put it nowhere at all. + /// not saved" belongs beside the answer. Discarding the save result — the + /// form this PR replaced — put it nowhere at all. + /// + /// The literal discard expression is deliberately NOT spelled out here. It + /// was, and a grep for it during verification matched this comment instead of + /// any code, reporting a defect that had already been fixed. #[test] fn a_failed_save_is_reported_in_the_panel() { let mut panel = LatencyPanel { @@ -664,7 +668,7 @@ mod tests { let taken = panel.take_unpersisted(); assert!(taken.is_some(), "premise: there was something to write"); - panel.note_persist_failure(&"permission denied"); + panel.note_persist_failure("permission denied"); assert!( panel.status.contains("could not be saved"), @@ -691,7 +695,7 @@ mod tests { ..LatencyPanel::default() }; assert!(panel.take_unpersisted().is_some()); - panel.note_persist_failure(&"disk full"); + panel.note_persist_failure("disk full"); assert!( panel.take_unpersisted().is_none(), "a failed write must not become a per-frame write attempt"