From 6f4ad6525a1cd8f40486dd083fbe1e3da84aebaf Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 02:11:25 -0400 Subject: [PATCH 01/20] feat(frontend): one atomic, durable file write for every path that persists user data v2.4.0 item C. Extracts the seven-property write sequence from `Config::save_to` into `crate::atomic_write` and adopts it at every remaining call site. WHY A MODULE AND NOT THREE COPIES v2.3.9 made the config path atomic after `fs::write` was found capable of leaving a user holding a truncated `config.toml`. It took seven properties to get right, and FIVE of them came from review rather than from the first draft. A property that five separate reviews had to find once will not be independently rediscovered three more times, which is the entire argument for one implementation. WHAT EACH PATH WAS ACTUALLY DOING config.rs::save_to .......... the full v2.3.9 sequence ....... 7 of 7 save_state.rs::save_to_slot . fs::write .................... 0 of 7 cheats.rs::save_for_rom ..... fs::write .................... 0 of 7 per_game.rs::save_overlay ... tmp + rename ................. 2 of 7 `save_state.rs` is the one that matters most and the plan named it last: A TRUNCATED SAVE STATE IS A USER'S GAME PROGRESS, a worse loss than a truncated config, and it was still using the bare call the config path had already been fixed for. It is also the path most likely to be written under load -- rewind capture, run-ahead and netplay rollback all produce save states, and a user pressing F1 during a busy frame is the ordinary case rather than an edge one. `per_game.rs` was NOT IN THE PLAN and is the instructive one. It writes a sibling temp file and renames, so it *looks* correct and a sweep for `fs::write` straight onto a target clears it. It held two of seven. The two that mattered: no `fsync`, so the rename could commit a directory entry pointing at bytes that never reached the medium; and a FIXED scratch name, `path.with_extension("json.tmp")`, shared across every process and every concurrent call -- the exact failure the mechanism exists to prevent, reintroduced by the mechanism itself. A partially-correct implementation is harder to spot than an absent one, which is an argument for the shared helper that the plan did not have when it was written. THE WINDOWS TAIL, WHICH THE CONFIG PATH NEVER HAD `std::fs::rename` maps to `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING`, so replace-existing holds on both platforms -- with a caveat POSIX does not have: on Windows the rename FAILS if another process has the target open, and an antivirus scanner or a search indexer reading `config.toml` is enough. That is why the config path never needed it, and why it would have gone unnoticed until a Windows user reported a save that failed for no visible reason. A bounded retry now covers it, and when the attempts are exhausted the error PROPAGATES: a save that fails silently after N attempts is worse than one that fails on the first, because the user gets no signal at all. THE RETRY LOOP IS PORTABLE SO THAT IT CAN BE TESTED The obvious shape is a `#[cfg(windows)]` block. That is deliberately not used for the loop, because CI runs the suite on Linux and a cfg-walled retry is code no test on the primary platform can execute -- an untested mechanism guarding a failure nobody can reproduce locally. Instead the loop is portable and the PREDICATE is platform-scoped, and the loop takes both the operation and the predicate as parameters. That second parameter is not tidiness; it was forced by a mutation. The first version called `is_transient_rename_error` directly, which reads as testable and is not: that predicate is unconditionally false on Unix, so the exhaustion branch is UNREACHABLE on the platform CI runs. A mutation making exhaustion return `Ok(())` -- silently reporting a save that never happened, the worst outcome this module has -- was NOT CAUGHT by the test written for it. Injecting the predicate makes the branch reachable everywhere, and a separate test pins the Unix single-attempt guarantee with the real predicate. MUTATION RESULTS, INCLUDING THE TWO THAT ARE NOT COVERED Seven properties deleted in turn. Five caught: symlink resolution removed ............... CAUGHT broken-symlink fallback removed .......... CAUGHT exact mode after creation removed ........ CAUGHT (needed a new test, below) occupied-scratch retry removed ........... CAUGHT exhaustion reports success ............... CAUGHT (needed the predicate param) unix predicate forced true ............... CAUGHT Two are NOT observable from inside the process, and the module says so rather than leaving a green suite to imply coverage it does not have: * `fsync` before the rename. Deleting it changes nothing an in-process assertion can see -- the page cache serves the read back identically. Only a power loss or a fault injector distinguishes them. * Mode applied AT CREATION. Deleting `opts.mode(...)` still ends at the right mode, because the explicit `set_permissions` after it corrects the result. Creation-mode is a RACE-WINDOW NARROWING, not an end-state property: it removes an interval in which the file sits at the umask default, and a test can only observe the end state. Neither should be removed on the evidence that no test fails. An untested property is not an unnecessary one. The mode test itself was rewritten because the mutation pass caught it asserting less than its name claimed: `opts.mode(0o600)` at creation already yields 0600 under any ordinary umask, so deleting the exact set afterwards left it green. `open(2)` applies `mode & ~umask`, so only a mode carrying bits the umask clears (0666 under the usual 022 gives 0644) distinguishes the two mechanisms. The new test OBSERVES the umask rather than assuming 022, and returns early when the umask masks nothing, because the two are then genuinely indistinguishable. THE WASM32 GATE CAUGHT WHAT NATIVE CLIPPY DID NOT `sync_parent_dir` compiles to an empty body off Unix, which trips clippy's `missing_const_for_fn` -- visible only on a non-Unix target, so native clippy passed while the wasm32 gate failed. Split into two cfg'd definitions with the non-Unix one `const`, which also states the truth: on Windows `MoveFileEx` already orders the metadata write, and on wasm there is no directory to sync. Recorded alongside it: the retry `sleep` is unreachable off Windows, which matters on wasm specifically because `std::thread::sleep` cannot block on `wasm32-unknown-unknown`. NET `config.rs` loses 273 lines, of which the great majority is the rationale that now lives once in the module rather than being duplicated at four call sites. Behaviour on Unix is unchanged; the config path GAINS the Windows retry it never had. GATES cargo fmt --all --check ......................... clean clippy: default / scripting / scripting,hd-pack / retroachievements / full ................ clean clippy wasm32: default / wasm-canvas ............ clean RUSTDOCFLAGS=-D warnings cargo doc .............. clean rustynes-frontend lib tests ..................... 557 passed atomic_write module tests ....................... 12 passed Frontend-only: no emulation source changes, so the AccuracyCoin 141/141 and nestest 0-diff results verified for v2.3.9 are unaffected. --- crates/rustynes-frontend/src/atomic_write.rs | 664 +++++++++++++++++++ crates/rustynes-frontend/src/cheats.rs | 5 +- crates/rustynes-frontend/src/config.rs | 273 +------- crates/rustynes-frontend/src/lib.rs | 8 + crates/rustynes-frontend/src/per_game.rs | 27 +- crates/rustynes-frontend/src/save_state.rs | 14 +- 6 files changed, 722 insertions(+), 269 deletions(-) create mode 100644 crates/rustynes-frontend/src/atomic_write.rs diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs new file mode 100644 index 00000000..899eb998 --- /dev/null +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -0,0 +1,664 @@ +//! One durable, atomic file write, shared by every path that persists user data. +//! +//! # Why this is a module and not three copies +//! +//! v2.3.9 made [`crate::config::Config::save_to`] atomic and durable after +//! `fs::write` was found capable of leaving a user holding a truncated +//! `config.toml` — every keybinding, palette, shader preset and per-game setting +//! they had. It took **seven** properties to get right, and **five of them came +//! from review rather than from the first draft.** That ratio is the whole +//! argument for one implementation: a property that five separate reviews had to +//! find once will not be independently rediscovered three more times. +//! +//! The paths this replaces, and what each was actually doing before: +//! +//! | path | what it wrote with | properties held | +//! | --- | --- | :---: | +//! | `config.rs::save_to` | the full v2.3.9 sequence | 7 of 7 | +//! | `save_state.rs::save_to_slot` | `fs::write` | 0 of 7 | +//! | `cheats.rs::save_for_rom` | `fs::write` | 0 of 7 | +//! | `per_game.rs::save_overlay` | tmp + `rename` | 2 of 7 | +//! +//! `save_state.rs` is the one that matters most and was named last in the plan: +//! **a truncated save state is a user's game progress**, which is a worse loss +//! than a truncated config, and it was writing with the bare call the config path +//! had already been fixed for. +//! +//! `per_game.rs` is the instructive one, and it was not in the plan at all. It +//! *looks* correct — it writes a sibling temp file and renames — so a reader +//! scanning for `fs::write` straight onto a target would clear it. It has no +//! `fsync`, so the rename can commit a directory entry pointing at bytes that +//! never reached the medium; and its scratch name is a **fixed** +//! `path.with_extension("json.tmp")`, shared by every process and every +//! concurrent call, which is precisely the failure the mechanism exists to +//! prevent, reintroduced by the mechanism itself. A partially-correct +//! implementation is harder to spot than an absent one. +//! +//! # The seven properties +//! +//! 1. **Sibling scratch file.** Across a filesystem boundary `rename` is not a +//! rename; a `$TMPDIR` on another mount silently degrades this to a copy. +//! 2. **`fsync` before the rename.** `fs::write` returns when the bytes reach the +//! page cache, not the medium. `rename` is atomic against other *processes*, +//! but a power loss between write and rename leaves the entry pointing at +//! contents that never landed — the exact outcome this claims to prevent. +//! 3. **Parent-directory sync.** On POSIX the entry `rename` creates is itself a +//! cache update until the directory is synced. +//! 4. **`create_new(true)`.** `File::create` follows symlinks and truncates, so a +//! predictable scratch name is a CWE-377 surface. +//! 5. **Mode applied at creation**, then set exactly. `open(2)` masks with the +//! umask, so creation alone can only land *narrower*; the explicit set makes it +//! exact. Narrow-then-correct, never widen-then-narrow. +//! 6. **Symlink resolution.** `fs::write` follows a link; `rename` replaces it. A +//! user who symlinked their config into a dotfiles repository would otherwise +//! find the link replaced by a regular file on the first automatic save. +//! 7. **A pid + per-call counter in the scratch name**, which is what makes (4) +//! adoptable: with a shared `.tmp`, exclusive creation fails every save after +//! one crash. +//! +//! # It is not uniform across platforms, and does not pretend to be +//! +//! A portable spine — scratch sibling, exclusive create, write, `fsync`, rename — +//! with a tail on **both** platforms: +//! +//! | property | Unix | Windows | +//! | --- | :---: | --- | +//! | parent-directory `sync_all` | yes | not applicable — opening a directory as a `File` is not portable, and `MoveFileEx` orders the metadata write | +//! | mode at creation, then exact | yes | not applicable — the ACL is inherited from the parent directory | +//! | symlink resolution | yes | applicable in principle; the dotfiles convention that motivates it is a Unix one | +//! | rename retry | not needed | **required** — see below | +//! +//! `rename`-replaces-existing holds on both: `std::fs::rename` maps to +//! `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING`. It holds with a caveat POSIX +//! does not have — on Windows the rename **fails if another process has the +//! target open**, and an antivirus scanner or a search indexer reading +//! `config.toml` is enough. That is why the config path never needed it, and why +//! it would go unnoticed until a Windows user reported a save that failed for no +//! visible reason. +//! +//! # The retry loop is portable so that it can be tested +//! +//! The obvious shape is a `#[cfg(windows)]` block. This module deliberately does +//! not use one for the loop itself, because CI runs the test suite on Linux and a +//! `cfg`-walled retry is code no test on this project's primary platform can ever +//! execute — an untested mechanism guarding a failure mode nobody can reproduce +//! locally. +//! +//! Instead the loop is portable and the **predicate** is platform-scoped: +//! `is_transient_rename_error` is `#[cfg(windows)]`-aware and returns `false` +//! unconditionally on Unix, so Unix performs exactly one attempt and its +//! behaviour is unchanged. The loop is then exercised on every platform through +//! `rename_with_retry_using`, which takes the rename as a closure. The mechanism +//! is tested where the condition cannot occur. +//! +//! When the retries are exhausted the error **propagates**. A save that fails +//! silently after N attempts is worse than one that fails on the first, because +//! the user gets no signal at all — and v2.3.9 fixed a swallowed save error for +//! exactly that reason. +//! +//! # What the tests do NOT cover, and why +//! +//! Stated rather than left to be inferred from a green suite. A mutation pass +//! over this module deleted each property in turn; five of seven were caught, and +//! the two that were not are **not observable from inside the process**: +//! +//! - **`fsync` before the rename (property 2).** Deleting it changes nothing any +//! in-process assertion can see — the page cache serves the read back +//! identically. Only a power loss or a filesystem fault injector distinguishes +//! the two, and neither belongs in a unit suite. +//! - **Mode applied *at creation* (property 5).** Deleting `opts.mode(...)` still +//! ends at the right mode, because the explicit `set_permissions` after it +//! corrects the result. Creation-mode is a **race-window narrowing**, not an +//! end-state property: it removes an interval in which the file sits at the +//! umask default. A test can only observe the end state, so the window is +//! invisible to it by construction. +//! +//! Both are kept for the reason they were added, and neither should be removed on +//! the evidence that "no test fails". That inference is the failure this project +//! has been bitten by more than once; an untested property is not an unnecessary +//! one. +//! +//! Everything else *is* pinned by a mutation: symlink resolution (both the live +//! and the broken-link path), the exact mode after creation, the occupied-scratch +//! retry, exhaustion propagating rather than reporting success, and the Unix +//! single-attempt guarantee. + +use std::fs; +use std::io; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +/// Monotonic sequence for scratch filenames. +/// +/// Module scope rather than a function-local `static`, 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. +static SCRATCH_SEQ: AtomicU64 = AtomicU64::new(0); + +/// How many times to attempt the rename before giving up. +/// +/// Only ever more than one on Windows (see `is_transient_rename_error`). Five +/// attempts with the backoff below is a worst case of 310 ms, which is a long +/// time in a UI frame and a short one against losing a save the user believes +/// happened. +const RENAME_ATTEMPTS: u32 = 5; + +/// Base backoff between rename attempts, doubled each time: 10, 20, 40, 80, 160. +/// +/// The `sleep` this drives is unreachable off Windows, which matters on **wasm** +/// specifically: `std::thread::sleep` cannot block on `wasm32-unknown-unknown`. +/// It is never called there because `is_transient_rename_error` is `false` on +/// every non-Windows target, so the loop returns on the first error. +const RENAME_BACKOFF_MS: u64 = 10; + +/// Resolve a symlinked target to the file it points at. +/// +/// `fs::write` follows a symlink and writes through to its target; `fs::rename` +/// replaces the link itself. Without this, a user who has symlinked a config or +/// cheat file into a dotfiles repository — a common setup — finds the link +/// silently replaced by a regular file on the first automatic save, and the +/// repository stops receiving changes. That is a behaviour regression introduced +/// by the fix rather than by the bug. +/// +/// Handles three cases, in order: +/// +/// - a resolvable path (symlink or not) — `canonicalize` gives the real file; +/// - a **broken** symlink — `canonicalize` fails, so the link is read directly. +/// This is the freshly-created-dotfiles case: the link exists, its target does +/// not yet; +/// - anything else — the path itself, which covers a first-ever save. +/// +/// A relative link target is resolved against the link's own directory, which is +/// what a relative symlink means. +#[must_use] +pub fn resolve_write_target(path: &Path) -> PathBuf { + if let Ok(real) = fs::canonicalize(path) { + return real; + } + match fs::read_link(path) { + Ok(dest) if dest.is_absolute() => dest, + Ok(dest) => match path.parent() { + Some(dir) => dir.join(dest), + None => dest, + }, + Err(_) => path.to_path_buf(), + } +} + +/// Is this rename failure one a retry could plausibly clear? +/// +/// On Windows, `MoveFileEx` fails with a sharing violation when another process +/// has the target open — an antivirus scanner, a search indexer, or a backup +/// agent reading the file is enough, and all three are transient. `std::io` maps +/// both `ERROR_ACCESS_DENIED` and `ERROR_SHARING_VIOLATION` to +/// [`io::ErrorKind::PermissionDenied`]. +/// +/// On Unix this is unconditionally `false`. POSIX `rename` has no such +/// constraint, so a `PermissionDenied` there means the directory permissions +/// genuinely forbid it — a condition retrying cannot change, and retrying would +/// only delay an error the caller needs now. +#[must_use] +pub const fn is_transient_rename_error(e: &io::Error) -> bool { + #[cfg(windows)] + { + matches!(e.kind(), io::ErrorKind::PermissionDenied) + } + #[cfg(not(windows))] + { + let _ = e; + false + } +} + +/// The retry loop, over an arbitrary rename operation. +/// +/// Both the operation and the transience predicate are parameters, and the +/// predicate is the load-bearing one. An earlier version called +/// `is_transient_rename_error` directly, which reads as testable and is not: +/// that predicate is unconditionally `false` on Unix, so the exhaustion branch is +/// **unreachable** on the platform CI runs. A mutation making exhaustion return +/// `Ok(())` — silently reporting a save that never happened, the worst outcome +/// this module has — was NOT caught by the test written for it. Injecting the +/// predicate makes the branch reachable everywhere. +/// +/// Propagates the final error when the attempts are exhausted rather than +/// reporting success or falling silent. +fn rename_with_retry_using(mut op: F, transient: P) -> io::Result<()> +where + F: FnMut() -> io::Result<()>, + P: Fn(&io::Error) -> bool, +{ + let mut attempt: u32 = 0; + loop { + match op() { + Ok(()) => return Ok(()), + Err(e) => { + attempt += 1; + if attempt >= RENAME_ATTEMPTS || !transient(&e) { + return Err(e); + } + std::thread::sleep(std::time::Duration::from_millis( + RENAME_BACKOFF_MS << (attempt - 1), + )); + } + } + } +} + +/// `fs::rename`, retried past a transient Windows sharing violation. +fn rename_with_retry(from: &Path, to: &Path) -> io::Result<()> { + rename_with_retry_using(|| fs::rename(from, to), is_transient_rename_error) +} + +/// Write `contents` to `path` atomically and durably. +/// +/// Creates the parent directory if needed, resolves a symlinked target, writes to +/// an exclusively-created sibling scratch file, `fsync`s it, carries the existing +/// file's mode across on Unix, renames over the target, and syncs the parent +/// directory. +/// +/// On failure the scratch file is removed and the **existing file is left +/// untouched** — a stale-but-valid file is the one worth keeping. +/// +/// # Errors +/// +/// Returns the underlying [`io::Error`] from any step. A rename that fails after +/// `RENAME_ATTEMPTS` transient failures returns the last error rather than +/// succeeding quietly. +pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { + let target = resolve_write_target(path); + if let Some(parent) = target.parent() + && !parent.as_os_str().is_empty() + { + fs::create_dir_all(parent)?; + } + + // The mode to create the scratch file WITH, read before it exists. + // + // Applying it at creation rather than chmod-ing afterwards closes a window in + // which the file sits at the umask default — briefly wider than the file the + // user tightened. + #[cfg(unix)] + let existing_mode = { + use std::os::unix::fs::PermissionsExt as _; + fs::metadata(&target).ok().map(|m| m.permissions().mode()) + }; + + let mut tmp = scratch_name(&target); + + let write_result = (|| -> io::Result<()> { + use std::io::Write as _; + let mut opts = fs::OpenOptions::new(); + // Exclusive creation: the open FAILS if anything is already at that path + // instead of truncating it (CWE-377). + opts.write(true).create_new(true); + #[cfg(unix)] + if let Some(mode) = existing_mode { + use std::os::unix::fs::OpenOptionsExt as _; + opts.mode(mode); + } + // Retry once past an occupied scratch name. A crashed run can orphan a + // scratch file, the OS later reuses that pid, and the new run's first save + // picks the same seq. Advancing the counter cannot produce the same name + // again, since it only increases within a process. + let mut f = match opts.open(&tmp) { + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => { + tmp = scratch_name(&target); + opts.open(&tmp)? + } + other => other?, + }; + f.write_all(contents)?; + f.sync_all() + })(); + if let Err(e) = write_result { + // Best-effort: if the write failed because the disk is full, the remove + // may fail too, and the original file is still intact. + let _ = fs::remove_file(&tmp); + return Err(e); + } + + // The exact mode, after creation. `open(2)` masks the requested mode with the + // umask, so creation alone can land narrower; this makes it exact. + #[cfg(unix)] + if let Some(mode) = existing_mode { + use std::os::unix::fs::PermissionsExt as _; + let _ = fs::set_permissions(&tmp, fs::Permissions::from_mode(mode)); + } + + if let Err(e) = rename_with_retry(&tmp, &target) { + let _ = fs::remove_file(&tmp); + return Err(e); + } + + sync_parent_dir(&target); + Ok(()) +} + +/// A scratch path beside `target`, carrying the pid and a per-call counter. +/// +/// The pid separates processes: two `RustyNES` instances saving at once would +/// otherwise write the same scratch file and one would rename the other's +/// half-written bytes over the target. The counter separates concurrent calls +/// *within* a process — not reachable from today's callers, but that is a +/// property of the callers rather than of this function, and one relaxed +/// fetch-add makes the guarantee structural. +fn scratch_name(target: &Path) -> PathBuf { + let seq = SCRATCH_SEQ.fetch_add(1, Ordering::Relaxed); + let mut s = target.as_os_str().to_os_string(); + s.push(format!(".{}.{seq}.tmp", std::process::id())); + PathBuf::from(s) +} + +/// `fsync` the directory holding `target`, so the rename itself is durable. +/// +/// Best-effort and Unix-gated: opening a directory as a `File` is not portable, +/// and `MoveFileEx` on Windows already orders the metadata write. +/// +/// A bare filename's parent is `Some("")`, and `File::open("")` fails with +/// `ENOENT` — so without the fallback the sync would silently not happen for a +/// relative target. That is a durability step quietly skipped rather than a +/// failure anyone sees, which is the worse of the two. `.` is the directory an +/// empty parent means. +#[cfg(unix)] +fn sync_parent_dir(target: &Path) { + { + let parent = target.parent().map_or_else( + || PathBuf::from("."), + |p| { + if p.as_os_str().is_empty() { + PathBuf::from(".") + } else { + p.to_path_buf() + } + }, + ); + if let Ok(dir) = fs::File::open(&parent) { + let _ = dir.sync_all(); + } + } +} + +/// No-op off Unix. +/// +/// Split into a separate `cfg`'d definition rather than an inner `#[cfg]` block, +/// because with an empty body clippy's `missing_const_for_fn` fires — and that is +/// only visible on a non-Unix target, so it failed the **wasm32** gate while +/// native clippy passed. `const` states the truth: on Windows `MoveFileEx` +/// already orders the metadata write, and on wasm there is no directory to sync. +#[cfg(not(unix))] +const fn sync_parent_dir(_target: &Path) {} + +#[cfg(test)] +mod tests { + use super::*; + + fn tempdir() -> PathBuf { + let d = std::env::temp_dir().join(format!( + "rustynes-atomic-{}-{}", + std::process::id(), + SCRATCH_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&d).expect("create tempdir"); + d + } + + #[test] + fn a_write_lands_and_replaces() { + let d = tempdir(); + let p = d.join("f.txt"); + write_atomic(&p, b"first").expect("first write"); + assert_eq!(fs::read(&p).unwrap(), b"first"); + write_atomic(&p, b"second").expect("second write"); + assert_eq!(fs::read(&p).unwrap(), b"second"); + } + + /// No scratch file may survive a successful write. + #[test] + fn a_successful_write_leaves_no_scratch_behind() { + let d = tempdir(); + let p = d.join("f.txt"); + write_atomic(&p, b"x").expect("write"); + let leftovers: Vec<_> = fs::read_dir(&d) + .unwrap() + .filter_map(Result::ok) + .filter(|e| e.file_name().to_string_lossy().ends_with(".tmp")) + .collect(); + assert!( + leftovers.is_empty(), + "scratch files survived a successful write: {:?}", + leftovers + .iter() + .map(std::fs::DirEntry::file_name) + .collect::>() + ); + } + + /// The symlink property, in the direction that matters: the LINK must + /// survive, and the file it points at must receive the bytes. + #[cfg(unix)] + #[test] + fn writing_through_a_symlink_keeps_the_link() { + let d = tempdir(); + let real = d.join("real.txt"); + let link = d.join("link.txt"); + fs::write(&real, b"old").expect("seed"); + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + + write_atomic(&link, b"new").expect("write"); + + assert!( + fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the symlink was replaced by a regular file" + ); + assert_eq!( + fs::read(&real).unwrap(), + b"new", + "the target did not receive the bytes" + ); + } + + /// A BROKEN symlink is the freshly-created-dotfiles case: the link exists, + /// its target does not yet. `canonicalize` fails here, so this exercises the + /// `read_link` fallback rather than the happy path above. + #[cfg(unix)] + #[test] + fn writing_through_a_broken_symlink_creates_the_target_and_keeps_the_link() { + let d = tempdir(); + let missing = d.join("not-yet.txt"); + let link = d.join("link.txt"); + std::os::unix::fs::symlink(&missing, &link).expect("symlink"); + + write_atomic(&link, b"new").expect("write"); + + assert!( + fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the broken symlink was replaced by a regular file" + ); + assert_eq!(fs::read(&missing).unwrap(), b"new"); + } + + /// The mode of an existing file must survive a rewrite. + /// + /// This is the property write-then-rename gives up relative to a truncating + /// write, and the one a user would notice only by auditing: a config + /// tightened to 0600 quietly widened to the umask default by an automatic + /// save they never asked for. + #[cfg(unix)] + #[test] + fn an_existing_files_mode_is_carried_across() { + use std::os::unix::fs::PermissionsExt as _; + let d = tempdir(); + let p = d.join("f.txt"); + fs::write(&p, b"old").expect("seed"); + fs::set_permissions(&p, fs::Permissions::from_mode(0o600)).expect("chmod"); + + write_atomic(&p, b"new").expect("write"); + + let mode = fs::metadata(&p).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "mode was widened to {mode:o}"); + } + + /// A mode the UMASK would mask must still land exactly. + /// + /// This exists because the test above does NOT actually pin property 5. + /// `opts.mode(0o600)` at creation already yields 0600 under any ordinary + /// umask, so deleting the explicit `set_permissions` that follows left that + /// test green — a mutation pass caught it asserting less than its name + /// claimed. `open(2)` applies `mode & ~umask`, so a mode carrying bits the + /// umask clears (0666 under the usual 022 gives 0644) is the only shape that + /// distinguishes creation-mode from the exact set after it. + /// + /// Returns early rather than failing when the umask masks nothing, because + /// the two mechanisms are then genuinely indistinguishable and a pass would + /// mean nothing either way. The umask is OBSERVED rather than assumed to be + /// 022 — CI images and developer shells do not agree on it. + #[cfg(unix)] + #[test] + fn a_mode_the_umask_would_mask_still_lands_exactly() { + use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; + let d = tempdir(); + + let probe = d.join("probe.txt"); + fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o666) + .open(&probe) + .expect("probe create"); + let created = fs::metadata(&probe).unwrap().permissions().mode() & 0o777; + if created == 0o666 { + return; // umask is 0 here; this test cannot distinguish anything. + } + + let p = d.join("f.txt"); + fs::write(&p, b"old").expect("seed"); + fs::set_permissions(&p, fs::Permissions::from_mode(0o666)).expect("chmod"); + + write_atomic(&p, b"new").expect("write"); + + let mode = fs::metadata(&p).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o666, + "mode landed at {mode:o}, not the 0666 the existing file carried: the \ + exact set after creation is missing, and creation alone was masked to \ + {created:o}" + ); + } + + /// An occupied scratch name must not cost the save. + /// + /// Plants a decoy at the name the next call will pick, which forces the + /// `AlreadyExists` retry branch. + #[test] + fn an_occupied_scratch_name_does_not_lose_the_write() { + let d = tempdir(); + let p = d.join("f.txt"); + // Peek at the next sequence value without consuming it, then plant a + // decoy at exactly that name. + let next = SCRATCH_SEQ.load(Ordering::Relaxed); + let mut decoy = p.as_os_str().to_os_string(); + decoy.push(format!(".{}.{next}.tmp", std::process::id())); + fs::write(PathBuf::from(decoy), b"decoy").expect("plant decoy"); + + write_atomic(&p, b"payload").expect("write should survive the collision"); + assert_eq!(fs::read(&p).unwrap(), b"payload"); + } + + /// A failed write must leave the existing file intact. + #[test] + fn a_failed_write_leaves_the_original_intact() { + let d = tempdir(); + let p = d.join("f.txt"); + write_atomic(&p, b"good").expect("seed"); + // A directory cannot be renamed over by a file write; target the + // directory itself so the write fails after the original exists. + let dir_target = d.join("subdir"); + fs::create_dir(&dir_target).expect("mkdir"); + assert!(write_atomic(&dir_target, b"nope").is_err()); + assert_eq!(fs::read(&p).unwrap(), b"good"); + } + + // ---- the retry loop, exercised on every platform ---- + + #[test] + fn the_retry_loop_returns_immediately_on_success() { + let mut calls = 0; + rename_with_retry_using( + || { + calls += 1; + Ok(()) + }, + is_transient_rename_error, + ) + .expect("should succeed"); + assert_eq!(calls, 1, "a successful rename must not be retried"); + } + + /// A non-transient error must NOT be retried — retrying a genuine permission + /// problem only delays an error the caller needs now. + #[test] + fn the_retry_loop_does_not_retry_a_non_transient_error() { + let mut calls = 0; + // Predicate forced to accept only PermissionDenied, so the loop's own + // decision is what stops it. With the real predicate this would pass on + // Unix for the wrong reason -- everything is non-transient there. + let r = rename_with_retry_using( + || { + calls += 1; + Err(io::Error::new(io::ErrorKind::NotFound, "gone")) + }, + |e| e.kind() == io::ErrorKind::PermissionDenied, + ); + assert!(r.is_err()); + assert_eq!(calls, 1, "a NotFound rename must not be retried"); + } + + /// Exhausting the attempts must PROPAGATE the error, never report success. + /// + /// The predicate is injected as always-transient so this branch is reachable + /// on EVERY platform. With the real predicate it is dead code on Unix, and a + /// mutation making exhaustion return `Ok(())` -- reporting a save that never + /// happened -- went uncaught until the predicate became a parameter. + #[test] + fn exhausting_the_attempts_propagates_the_error() { + let mut calls: u32 = 0; + let r = rename_with_retry_using( + || { + calls += 1; + Err(io::Error::new(io::ErrorKind::PermissionDenied, "busy")) + }, + |_| true, + ); + let e = r.expect_err("exhausting the attempts must not report success"); + assert_eq!(e.kind(), io::ErrorKind::PermissionDenied); + assert_eq!( + calls, RENAME_ATTEMPTS, + "the loop must make exactly RENAME_ATTEMPTS attempts before giving up" + ); + } + + /// Unix must make exactly ONE attempt, with the REAL predicate -- the + /// platform claim in the module docs, pinned rather than described. The test + /// above deliberately bypasses that predicate, so this is what covers it. + #[cfg(not(windows))] + #[test] + fn unix_never_retries_a_rename() { + let mut calls = 0; + let r = rename_with_retry_using( + || { + calls += 1; + Err(io::Error::new(io::ErrorKind::PermissionDenied, "busy")) + }, + is_transient_rename_error, + ); + assert!(r.is_err()); + assert_eq!(calls, 1, "POSIX rename has no transient sharing violation"); + } +} diff --git a/crates/rustynes-frontend/src/cheats.rs b/crates/rustynes-frontend/src/cheats.rs index 687b8ff8..1841559f 100644 --- a/crates/rustynes-frontend/src/cheats.rs +++ b/crates/rustynes-frontend/src/cheats.rs @@ -160,7 +160,10 @@ pub fn save(data_dir: &Path, rom_sha256: &[u8; 32], genie: &[CheatEntry], raw: & }; match toml::to_string_pretty(&file) { Ok(s) => { - if let Err(e) = fs::write(&path, s) { + // Atomic + durable, via the shared helper (v2.4.0 item C). This was + // `fs::write`, so an interruption truncated the user's whole cheat + // list for that ROM. + if let Err(e) = crate::atomic_write::write_atomic(&path, s.as_bytes()) { eprintln!("rustynes: cheats {} write failed: {e}", path.display()); } } diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index 4b61ecdd..87b521e0 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -2111,277 +2111,32 @@ impl Config { self.save_to(&path) } - /// Where a save should actually land, following a symlink to its target. - /// - /// `fs::write` follows a symlink and writes through to the file it points at; - /// `fs::rename` replaces the link. Writing to the link's own path would - /// therefore convert a user's symlinked `config.toml` -- a dotfiles-repository - /// setup -- into a regular file on the first automatic save. - /// - /// Two cases, and the second is the one that is easy to miss: - /// - /// * An **intact** link resolves through `canonicalize`. - /// * A **broken** link -- pointing at a file that does not exist yet, which is - /// exactly a freshly-created dotfiles link awaiting its first save -- - /// makes `canonicalize` fail with `NotFound`. Falling back to the link's own - /// path there would destroy the very setup the resolution exists to protect, - /// so the link is read by hand instead and its destination used, relative to - /// the link's own directory when it is not absolute. (Review on #420 found - /// this surviving inside the fix for the intact case.) - /// - /// Anything that is not a symlink -- including a path that does not exist at - /// all, the first-ever save -- falls back to the path as given, which is - /// correct: there is nothing to follow. - fn resolve_write_target(path: &Path) -> PathBuf { - if let Ok(real) = fs::canonicalize(path) { - return real; - } - // `read_link` fails with `EINVAL` when the path is not a link at all, which - // is how "no symlink to follow" is distinguished from "broken symlink" - // without a second `symlink_metadata` call. - match fs::read_link(path) { - Ok(dest) if dest.is_absolute() => dest, - Ok(dest) => match path.parent() { - Some(dir) => dir.join(dest), - None => dest, - }, - Err(_) => path.to_path_buf(), - } - } - /// Save to an explicit path. /// /// # Errors /// /// Returns [`ConfigError`] on I/O or serialization failure. pub fn save_to(&self, path: &Path) -> Result<(), ConfigError> { - let target = Self::resolve_write_target(path); - if let Some(parent) = target.parent() { - fs::create_dir_all(parent)?; - } - let s = toml::to_string_pretty(self)?; - - // Written to a sibling temporary file and RENAMED over the target, - // rather than `fs::write` straight onto it. - // - // `fs::write` truncates first 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 stopped being a theoretical window when saves became automatic. - // This is called from more than a dozen places, several of them not user - // actions at all: closing a ROM, changing a mixer slider, and (v2.3.9) - // finishing a Latency Oracle measurement all save without being asked. + // Serialize, then hand the bytes to the shared atomic writer. // - // `rename` within a directory is atomic on both platforms this ships on: - // POSIX guarantees it, and `std::fs::rename` maps to `MoveFileEx` with - // `MOVEFILE_REPLACE_EXISTING` on Windows. The temp file is a SIBLING for - // exactly that reason -- across a filesystem boundary `rename` is not a - // rename at all, and a `$TMPDIR` on another mount would silently degrade - // this back to a copy. + // The seven-property write sequence that used to live inline here moved + // to `crate::atomic_write` in v2.4.0 (item C), unchanged in behaviour on + // Unix and with one ADDITION this path never had: a bounded retry past a + // transient Windows sharing violation. On Windows `MoveFileEx` fails if + // another process has the target open, and an antivirus scanner or search + // indexer reading `config.toml` is enough -- a save that failed for no + // visible reason, on a platform this project's CI does not run the suite + // on. POSIX has no such constraint, which is why it went unnoticed here. // - // A failed rename leaves the old config intact and the temp file behind, - // which is the right way round: the stale-but-valid file is the one worth - // keeping. The temp file is removed on a write failure so a full disk - // does not accumulate them. - // - // The temp name carries the PROCESS ID. A bare `.tmp` is shared, so two - // RustyNES instances saving at once would write the same scratch file and - // one would rename the other's half-written bytes over the config -- the - // failure this function exists to prevent, reintroduced by its own - // mechanism. A stale `.tmp` left by a crashed run also cannot block a - // later save, because that run has a different id. (Review on #420.) - // - // `tempfile::NamedTempFile` would give the same guarantee more tidily, and - // is deliberately not used: `tempfile` is a DEV-dependency here, and - // promoting it to a runtime dependency of a binary that ships to users is - // a supply-chain decision, not a cleanup. - // Resolve a SYMLINKED config to its target before choosing where to write. - // - // `fs::write` follows a symlink and writes through to the file it points - // at. `fs::rename` replaces the link itself. Without this, a user who has - // symlinked `config.toml` into a dotfiles repository -- a common setup -- - // would find the link silently replaced by a regular file on the first - // automatic save, and their repository stops receiving changes. That is a - // behaviour regression introduced by the fix, not by the bug. (Review on - // #420.) - // - - // The scratch name carries the process id AND a per-call counter. - // - // A bare `.tmp` is shared across processes: two RustyNES instances saving - // at once would write the same file and one would rename the other's - // half-written bytes over the config -- the failure this function exists - // to prevent, reintroduced by its own mechanism. The pid closes that. - // - // The counter closes the same hole WITHIN a process. Config saves are - // driven from the UI thread today, so two concurrent calls are not - // reachable, but "not reachable today" is a property of the callers rather - // than of this function, and a monotonic counter costs one relaxed fetch-add - // to make the guarantee structural. (Review on #420 raised it.) - // - // `tempfile::NamedTempFile` would give both properties more tidily and is - // deliberately not used: `tempfile` is a DEV-dependency here, and promoting - // it to a runtime dependency of a binary that ships to users is a - // supply-chain decision, not a cleanup. - let seq = SCRATCH_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); - let mut tmp_os = target.as_os_str().to_os_string(); - tmp_os.push(format!(".{}.{seq}.tmp", std::process::id())); - let tmp = PathBuf::from(tmp_os); - - // Write, then FSYNC, then rename -- in that order, and the fsync is not - // optional. - // - // `fs::write` returns once the bytes are in the OS 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: an empty or - // truncated config, which is precisely the outcome this function claims to - // prevent. Review on #420 caught that the first version stopped one step - // short and would have shipped a durability guarantee it did not have. - // The mode to create the scratch file WITH, read before it exists. - // - // Applying it at creation rather than chmod-ing afterwards closes a window - // in which the file exists at the umask default -- briefly wider than the - // config the user tightened. `open(2)` applies `mode & ~umask`, so this can - // only ever be narrower than asked; the exact mode is still set below, - // which makes the pair a narrow-then-correct sequence rather than a - // widen-then-narrow one. (Review on #420.) - #[cfg(unix)] - let existing_mode = { - use std::os::unix::fs::PermissionsExt; - fs::metadata(&target).ok().map(|m| m.permissions().mode()) - }; - - // Retry once past an occupied scratch name. - // - // `create_new` turns a collision into a failed save, and there is one way - // a collision can happen without an attacker: a crashed run leaves an - // orphaned scratch file, the OS later reuses that pid, and the new run's - // first save picks the same seq. Unlikely, and a lost save is a real cost - // for a user who would have no idea why. Advancing the counter and trying - // again turns it into nothing at all -- the next name cannot be the same - // one, since the counter only increases within a process. (Review on #425 - // raised the pid-reuse case against the plan; it applies here.) - let mut tmp = tmp; - let write_result = (|| -> std::io::Result<()> { - use std::io::Write as _; - let mut opts = fs::OpenOptions::new(); - // `create_new` rather than `create`: exclusive creation, so the open - // FAILS if anything is already at that path instead of truncating it. - // - // `File::create` follows symlinks and truncates, so a predictable - // scratch name is a CWE-377 surface -- something pre-created there as a - // link to another file would be silently truncated and overwritten by - // the save. The scratch file is a sibling of the user's own config - // rather than a world-writable directory, so an attacker who can plant - // it already owns the config; that makes this defence in depth rather - // than a live hole, and it costs one call. (Review on #420.) - // - // Exclusive creation was NOT safe to adopt while the scratch name was a - // bare `.tmp`: a stale file from a crashed run would then have failed - // every subsequent save. With the pid and the per-call counter in the - // name, a previous run cannot collide, so the failure mode that ruled - // this out no longer exists. - opts.write(true).create_new(true); - #[cfg(unix)] - if let Some(mode) = existing_mode { - use std::os::unix::fs::OpenOptionsExt as _; - opts.mode(mode); - } - let mut f = match opts.open(&tmp) { - Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { - let seq = SCRATCH_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); - let mut retry = target.as_os_str().to_os_string(); - retry.push(format!(".{}.{seq}.tmp", std::process::id())); - tmp = PathBuf::from(retry); - opts.open(&tmp)? - } - other => other?, - }; - f.write_all(s.as_bytes())?; - f.sync_all() - })(); - if let Err(e) = write_result { - // Best-effort: if the write failed because the disk is full, the - // remove may fail too, and the original config is still intact. - let _ = fs::remove_file(&tmp); - return Err(e.into()); - } - - // Carry the existing file's permissions onto the replacement. - // - // This is the one thing write-then-rename gives up relative to a - // truncating write, and review on #420 caught it: `fs::write` onto an - // existing file preserves that file's mode, while a fresh temp file - // takes the process 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. - // - // Best-effort, and only when there IS an existing file to copy from: a - // first-ever save has no prior mode, and a filesystem that cannot report - // or set one should not cost the user an atomic write. Unix-gated - // because that is where the mode lives; on Windows the ACL is inherited - // from the parent directory rather than carried on the file, so - // `MoveFileEx` already produces the right result. - // The exact mode, after creation. `open(2)` masks the requested mode with - // the umask, so creation alone can land narrower than the original; this - // makes it exact. Best-effort, and only when there was a prior file to copy - // from. - #[cfg(unix)] - if let Some(mode) = existing_mode { - use std::os::unix::fs::PermissionsExt; - let _ = fs::set_permissions(&tmp, fs::Permissions::from_mode(mode)); - } - - if let Err(e) = fs::rename(&tmp, &target) { - let _ = fs::remove_file(&tmp); - return Err(e.into()); - } - - // Durability of the RENAME itself, as opposed to the file's contents. - // - // On POSIX the directory entry created by `rename` is also only a cache - // update until the containing directory is synced. Best-effort, and - // Unix-gated: opening a directory as a `File` is not portable, and - // `MoveFileEx` on Windows already orders the metadata write. - // - // A bare filename's parent is `Some("")`, and `File::open("")` fails with - // `ENOENT` -- so without the fallback the sync would silently not happen - // for a relative target, which is a durability step quietly skipped rather - // than a failure anyone sees. `.` is the directory an empty parent means. - // (Review on #420. `create_dir_all("")` was checked in the same pass and - // returns `Ok`, so the claim that a bare filename aborts the save does not - // reproduce -- only the sync was affected.) - #[cfg(unix)] - { - let parent = target.parent().map_or_else( - || PathBuf::from("."), - |p| { - if p.as_os_str().is_empty() { - PathBuf::from(".") - } else { - p.to_path_buf() - } - }, - ); - if let Ok(dir) = fs::File::open(&parent) { - let _ = dir.sync_all(); - } - } - + // The full rationale for each property, and the platform table, is in + // that module's docs rather than duplicated at each of the four call + // sites it now serves. + let s = toml::to_string_pretty(self)?; + crate::atomic_write::write_atomic(path, s.as_bytes())?; Ok(()) } } -/// Monotonic sequence for [`Config::save_to`]'s scratch filenames. -/// -/// Module scope rather than a function-local `static`, 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. -static SCRATCH_SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); - /// Rewrite every keycode value of a [`PadBindings`] to its canonical /// current winit-0.30 `KeyCode` spelling. Used by [`Config::migrate_legacy`] /// to clean up legacy winit-0.29 `VirtualKeyCode` value strings carried in diff --git a/crates/rustynes-frontend/src/lib.rs b/crates/rustynes-frontend/src/lib.rs index 9be91a78..7a7b6979 100644 --- a/crates/rustynes-frontend/src/lib.rs +++ b/crates/rustynes-frontend/src/lib.rs @@ -16,6 +16,14 @@ #![warn(missing_docs)] pub mod app; +/// v2.4.0 item C — one durable, atomic file write shared by every path that +/// persists user data (config, save states, cheats, per-game overlays). +/// +/// Extracted from `config::Config::save_to`, whose seven properties took five +/// rounds of review to arrive at. Three other paths were writing user data with +/// the bare truncating call that fix exists to replace — including save states, +/// where the loss is a user's game progress. +pub mod atomic_write; pub mod audio; // v1.7.0 "Forge" H3 — frontend stereo output DSP (panning / Schroeder reverb / // headphone crossfeed). Bypass-by-default (center pan, 0% reverb, 0 crossfeed) diff --git a/crates/rustynes-frontend/src/per_game.rs b/crates/rustynes-frontend/src/per_game.rs index 7ad55de7..2808f399 100644 --- a/crates/rustynes-frontend/src/per_game.rs +++ b/crates/rustynes-frontend/src/per_game.rs @@ -268,16 +268,27 @@ pub fn save_overlay(crc: u32, cfg: &PerGameConfig) -> std::io::Result<()> { Err(e) => return Err(e), } } - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent)?; - } let json = serde_json::to_vec_pretty(cfg) .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; - // Atomic write: serialize to a sibling temp file, then rename over the - // target (atomic on the same filesystem). - let tmp = path.with_extension("json.tmp"); - std::fs::write(&tmp, json)?; - std::fs::rename(&tmp, &path) + + // Atomic + durable, via the shared helper (v2.4.0 item C). + // + // This path already wrote a sibling temp file and renamed, so it LOOKED + // correct and a sweep for `fs::write`-straight-onto-a-target would have + // cleared it. It held two of the seven properties. The two that mattered: + // + // - No `fsync` before the rename, so the rename could commit a directory + // entry pointing at bytes that never reached the medium -- the precise + // outcome write-then-rename is adopted to prevent. + // - A FIXED scratch name, `path.with_extension("json.tmp")`, shared by every + // process and every concurrent call. Two instances saving at once would + // write the same scratch file and one would rename the other's + // half-written bytes over the overlay: the failure the mechanism exists to + // prevent, reintroduced by the mechanism itself. + // + // A partially-correct implementation is harder to spot than an absent one, + // which is the argument for a shared helper rather than a per-site pattern. + crate::atomic_write::write_atomic(&path, &json) } #[cfg(test)] diff --git a/crates/rustynes-frontend/src/save_state.rs b/crates/rustynes-frontend/src/save_state.rs index 97d97a5c..36cff2d8 100644 --- a/crates/rustynes-frontend/src/save_state.rs +++ b/crates/rustynes-frontend/src/save_state.rs @@ -95,7 +95,19 @@ pub fn save_to_slot( if let Some(parent) = path.parent() { fs::create_dir_all(parent).map_err(|e| map_io(parent, e))?; } - fs::write(&path, state).map_err(|e| map_io(&path, e))?; + // Atomic + durable, via the shared helper (v2.4.0 item C). + // + // This was `fs::write`, which truncates and then writes. An interruption in + // that window -- a crash, a kill, a full disk -- left the user holding a + // truncated save state, and a save state is a user's GAME PROGRESS. The + // config path was fixed for exactly this in v2.3.9 while this one, where the + // loss is worse, kept the bare call. + // + // Save states are also the path most likely to be written under load: + // rewind capture, run-ahead and netplay rollback all produce them, and a + // user hitting F1 during a busy frame is the ordinary case rather than an + // edge one. + crate::atomic_write::write_atomic(&path, state).map_err(|e| map_io(&path, e))?; Ok(path) } From 22ba96fad3e1d6b1ef547b4b9422e12decd715a7 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 02:22:39 -0400 Subject: [PATCH 02/20] fix(config): stop two shipped features writing empty tables into an untouched config v2.4.0 item D. `graphics.hd_packs` (v1.5.0) and `graphics.shader_presets` (v1.2.0) both documented a pre-feature config as "byte-identical". Both were byte-identical only until the first save. `#[serde(default)]` is a LOAD guarantee. It says nothing about what SAVE writes, and the TOML serializer emits an empty table for an empty collection -- so a user who had never opened the HD-pack manager or saved a shader preset found their config rewritten with a bare `[graphics.hd_packs]` and `[graphics.shader_presets]` on the first save after upgrading. Not data loss, but a claim the file itself contradicted, and one that made a genuine diff harder to read. v2.3.9 corrected the PROSE and deliberately left the behaviour, on the reasoning that changing what two shipped features write is a separate decision from fixing a false claim. This is that decision, and the plan put it here for that reason. `hd_packs` is a bare `BTreeMap`, so `skip_serializing_if` names `BTreeMap::is_empty` directly, matching `input.latency_reports` which got this treatment in v2.3.9. `shader_presets` is a `ShaderPresetBank` struct wrapping a map, so it needed an `is_empty` on the type before the attribute had anything to name -- which is a fair part of why it was the one left behind when `latency_reports` was fixed. BOTH DIRECTIONS, BECAUSE ONE DIRECTION PROVES NOTHING The plan is explicit that a one-directional test passes just as happily against a field that never persists anything at all. So each field gets: the empty case is OMITTED, a populated one SURVIVES, and both round-trip back through `from_str` -- because the string checks verify the KEY and say nothing about the VALUE. Mutation-tested in both directions, each confirmed to have actually run its named test rather than matching zero and exiting 0: removed hd_packs skip ................... CAUGHT removed hd_packs skip (property test) ... CAUGHT removed shader_presets skip ............. CAUGHT over-eager is_empty (always true) ....... CAUGHT That last one is the direction that matters most: an `is_empty` returning `true` unconditionally would silently DISCARD a user's saved presets on every save, which is a data-loss bug wearing the shape of a tidiness fix. It is caught. A THIRD TEST, FOR THE FIELD THAT DOES NOT EXIST YET `a_default_config_writes_no_empty_opt_in_tables` asserts the property once rather than per field: a default config must carry no empty table for any of the three opt-in collections. The per-field tests would each still pass if a FOURTH such field were added tomorrow without the attribute; this is the one that would start failing. The defect being fixed here is precisely "a field was added and the save-side property was not considered", so the regression net should be shaped around the field that has not been written yet. GATES cargo fmt --all --check ......................... clean clippy: default / full .......................... clean clippy wasm32: default / wasm-canvas ............ clean RUSTDOCFLAGS=-D warnings cargo doc .............. clean rustynes-frontend lib tests ..................... 560 passed Frontend-only. Config files written by an older build still load unchanged -- this only removes keys that carried no information. --- crates/rustynes-frontend/src/config.rs | 138 ++++++++++++++++++-- crates/rustynes-frontend/src/shader_pass.rs | 11 ++ 2 files changed, 138 insertions(+), 11 deletions(-) diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index 87b521e0..1f1a3b8e 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -861,10 +861,20 @@ pub struct GraphicsConfig { #[serde(default)] pub shader_stack: crate::shader_pass::ShaderStackConfig, /// v1.2.0 C2 — saved named shader-stack presets (the CRT preset bank + - /// user-saved stacks). `#[serde(default)]` = empty, so a pre-C2 config LOADS - /// unchanged. Persisted under `[graphics.shader_presets]`. Same correction as - /// `hd_packs` below: `serde(default)` says nothing about what SAVE writes. - #[serde(default)] + /// user-saved stacks). Persisted under `[graphics.shader_presets]`. + /// + /// `#[serde(default)]` is a **load** guarantee — it says nothing about what + /// SAVE writes. v2.3.9 corrected the claim here and deliberately left the + /// behaviour, because changing what a shipped feature writes is a separate + /// decision from fixing a false claim. v2.4.0 item D is where that decision + /// belongs, and `skip_serializing_if` is it: an empty bank now writes + /// nothing, so a user who has never saved a preset carries a config that + /// round-trips byte-identically instead of gaining an empty table on their + /// first save. + #[serde( + default, + skip_serializing_if = "crate::shader_pass::ShaderPresetBank::is_empty" + )] pub shader_presets: crate::shader_pass::ShaderPresetBank, /// v1.2.0 beta.2 (Workstream C3) — per-game HD-pack paths, keyed on the /// ROM SHA-256 (hex). When the loaded ROM's hash has an entry here AND the @@ -873,13 +883,15 @@ pub struct GraphicsConfig { /// default and `#[serde(default)]`, so a pre-C3 config LOADS unchanged and /// the default presentation is unchanged. Presentation-only. /// - /// Deliberately says "loads", not "is byte-identical": `serde(default)` is a - /// LOAD guarantee only, and on save the TOML serializer emits an empty - /// `[graphics.hd_packs]` table. Left as-is rather than given a - /// `skip_serializing_if` like `input.latency_reports`, because that would - /// change the file this shipped feature writes; the wrong half here was the - /// claim, not the behaviour. - #[serde(default)] + /// `serde(default)` is a **load** guarantee only: it said nothing about what + /// SAVE wrote, and the TOML serializer emitted an empty `[graphics.hd_packs]` + /// table on the first save. v2.3.9 corrected that claim and deliberately left + /// the behaviour alone, because changing what a shipped feature writes is a + /// separate decision. v2.4.0 item D makes it: `skip_serializing_if` keeps the + /// key out of the file until there is a mapping to store, matching + /// `input.latency_reports`, so "loads unchanged" and "round-trips + /// byte-identically" are now both true rather than only the first. + #[serde(default, skip_serializing_if = "std::collections::BTreeMap::is_empty")] pub hd_packs: std::collections::BTreeMap, /// v1.5.0 "Lens" Workstream D1 — per-side overscan crop, in NES pixels. The /// legacy [`Self::hide_overscan`] toggle is the equivalent of an @@ -2216,6 +2228,110 @@ mod tests { ); } + /// v2.4.0 item D — `graphics.hd_packs` must not write an empty table. + /// + /// Same defect as `latency_reports` above, in a field that shipped in v1.5.0 + /// and carried the same false "byte-identical" claim ever since. v2.3.9 + /// corrected the prose and deliberately left the behaviour, because changing + /// what a shipped feature writes is a separate decision; this is that + /// decision. + /// + /// Both directions, because a `skip_serializing_if` that is too eager would + /// silently discard a user's HD-pack mappings — a one-directional test passes + /// just as happily against a field that never persists anything at all. + #[test] + fn an_empty_hd_pack_map_is_not_written_but_a_populated_one_is() { + let empty = toml::to_string_pretty(&Config::default()).expect("serialize default"); + assert!( + !empty.contains("hd_packs"), + "an empty hd_packs table was written, so an untouched config does not \ + round-trip byte-identically:\n{empty}" + ); + + let mut c = Config::default(); + c.graphics + .hd_packs + .insert("smb3".to_owned(), std::path::PathBuf::from("/packs/smb3")); + let filled = toml::to_string_pretty(&c).expect("serialize populated"); + assert!( + filled.contains("hd_packs"), + "a real HD-pack mapping was dropped on save:\n{filled}" + ); + + let empty_back: Config = toml::from_str(&empty).expect("empty config re-parses"); + assert!( + empty_back.graphics.hd_packs.is_empty(), + "the omitted key did not come back as an empty map" + ); + let filled_back: Config = toml::from_str(&filled).expect("populated config re-parses"); + assert_eq!( + filled_back.graphics.hd_packs.get("smb3"), + Some(&std::path::PathBuf::from("/packs/smb3")), + "the HD-pack mapping did not survive a save/load round trip" + ); + } + + /// v2.4.0 item D — `graphics.shader_presets` must not write an empty table. + /// + /// The v1.2.0 half of the same pair. `ShaderPresetBank` is a struct rather + /// than a bare map, so this one needed an `is_empty` on the type before + /// `skip_serializing_if` could name anything — which is why it was easy to + /// leave behind when `latency_reports` got the treatment. + #[test] + fn an_empty_shader_preset_bank_is_not_written_but_a_populated_one_is() { + let empty = toml::to_string_pretty(&Config::default()).expect("serialize default"); + assert!( + !empty.contains("shader_presets"), + "an empty shader_presets table was written:\n{empty}" + ); + + let mut c = Config::default(); + c.graphics.shader_presets.presets.insert( + "my-crt".to_owned(), + crate::shader_pass::ShaderStackConfig::default(), + ); + let filled = toml::to_string_pretty(&c).expect("serialize populated"); + assert!( + filled.contains("shader_presets"), + "a saved shader preset was dropped on save:\n{filled}" + ); + assert!(filled.contains("my-crt"), "the preset name was not written"); + + let empty_back: Config = toml::from_str(&empty).expect("empty config re-parses"); + assert!( + empty_back.graphics.shader_presets.is_empty(), + "the omitted key did not come back as an empty bank" + ); + let filled_back: Config = toml::from_str(&filled).expect("populated config re-parses"); + assert!( + filled_back + .graphics + .shader_presets + .presets + .contains_key("my-crt"), + "the preset did not survive a save/load round trip" + ); + } + + /// The whole point of item D, stated once as a property rather than per field. + /// + /// A default config must serialize to something that carries **no empty + /// tables at all** for the three opt-in collections. Written as its own test + /// because the per-field ones would each still pass if a FOURTH such field + /// were added tomorrow without the attribute — this is the one that would + /// start failing. + #[test] + fn a_default_config_writes_no_empty_opt_in_tables() { + let text = toml::to_string_pretty(&Config::default()).expect("serialize default"); + for key in ["latency_reports", "hd_packs", "shader_presets"] { + assert!( + !text.contains(key), + "`{key}` was written for a default config, so an untouched file \ + does not round-trip byte-identically:\n{text}" + ); + } + } + /// A CHAIN of symlinks resolves all the way to the real file. /// /// Added because a mutation exposed that the intact-link test did not need diff --git a/crates/rustynes-frontend/src/shader_pass.rs b/crates/rustynes-frontend/src/shader_pass.rs index 512e5168..3b726124 100644 --- a/crates/rustynes-frontend/src/shader_pass.rs +++ b/crates/rustynes-frontend/src/shader_pass.rs @@ -393,6 +393,17 @@ pub struct ShaderPresetBank { } impl ShaderPresetBank { + /// Is the bank empty — no user-saved presets at all? + /// + /// Exists for `Config`'s `skip_serializing_if` (v2.4.0 item D). A bank with + /// no presets writes nothing, so a user who has never saved one keeps a + /// config that genuinely round-trips byte-identically rather than gaining an + /// empty `[graphics.shader_presets]` table on their first save. + #[must_use] + pub fn is_empty(&self) -> bool { + self.presets.is_empty() + } + /// Resolve a preset by name for the per-game apply path (v2.1.9 B6): a user /// preset of that name wins, else a built-in of the same name, else `None` /// (an unknown name applies nothing, keeping the load byte-identical). From 6c24f7754908ab9513e215332a0899a701b2ca0b Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 02:39:41 -0400 Subject: [PATCH 03/20] feat(core): a timeline generation counter, and the telemetry consumers that read it v2.4.0 item B. `Nes` gains a session-local `timeline_generation` that changes whenever the emulator jumps to a different point on its timeline, and the debug telemetry that describes a run now clears itself when it does. WHY A COUNTER RATHER THAN MORE CALL SITES v2.3.9 item E cleared the call stack and access counters on a ROM change and recorded, honestly, that the same telemetry is NOT cleared on a save-state load. A two-call-site patch was declined as insufficient, with the reason measured: native load-state ....... reachable from a frontend call site wasm load-state ......... NOT -- restores inside a `spawn_local` task holding only a cloned `EmuHandle` rewind .................. NOT -- happens entirely inside the core netplay rollback ........ NOT One of four, and patching it would have presented a quarter of the fix as the whole of it. The plan originally proposed each consumer remembering the last `Nes::cycle()` it saw and noticing a non-monotonic step. Review on #415 proposed better, and the difference is a case the heuristic provably cannot cover: a restore to a LATER state advances `cycle()`, so it is indistinguishable from execution. A core-side counter sees it, and needs no cooperation from any call site. LOUD VERSUS QUIET, WHICH THE CODEBASE ALREADY DISTINGUISHED `restore_inner` already takes `clear_rewind`, which is exactly the distinction: `true` for a user-driven load that invalidates rewind history, `false` for a same-timeline machine-driven restore (run-ahead's per-frame rollback, netplay's rollback-resimulate) where the history stays valid. The counter reuses it rather than inventing a parallel notion. This DEPARTS FROM THE PLAN'S ENUMERATION, deliberately. The plan listed netplay rollback as a bump site, and also stated the mechanism -- "a same-timeline restore is exactly one that must NOT bump the counter". The two cannot both hold; netplay rollback goes through `restore_quiet` precisely because it is same-timeline. The mechanism wins: bumping there would clear a user's telemetry sixty times a second under run-ahead, which is a worse defect than the stale telemetry this fixes. Both directions are pinned by tests. `reset` and `power_cycle` bump too. They are discontinuities by any reading, and a reconstructed call stack describes a run that no longer exists after either. The bump happens BEFORE the restore can fail. A partially-applied restore is a discontinuity whether or not it completed, and a consumer that keeps stale telemetry because the jump errored is the bug in its most confusing form. THE COUNTER MUST NOT BE SERIALIZED Its only job is to be DIFFERENT after a discontinuity. Serializing it would put an OLD value back on restore, so loading a state saved earlier in the same session could hand a consumer a generation it has already seen -- and the consumer would conclude nothing jumped at the exact moment something did. The plan asked for an entry in `snapshot_schema_audit.rs`. That file audits `Ppu`, `Cpu`, `Apu` and `Opll`; `Nes` is not among them, and retrofitting it means classifying every field of `Nes`, which is a larger change than this item. So the property is pinned by an EXECUTABLE assertion instead, which is stronger than a list entry would have been: snapshot at generation N, advance past N, restore, and assert the generation did not come back to N. Simulating serialization makes it fail with exactly the diagnostic it should: the generation went BACKWARDS to 1 (a consumer had already seen 3), so the counter is being carried in the save state -- which defeats its only purpose THE CONSUMER SIDE, LANDED WITH IT Checked once per frame in `DebuggerOverlay::pump_watchpoints`, which already runs under the emu lock with `&mut Nes` -- rather than at each site that could cause a jump, since two of the four are not reachable from one. The decision is extracted into a `TimelineWatch` value rather than an `Option` field, for a reason that has now come up twice in this release: `DebuggerOverlay::new` needs a window and a wgpu device, so anything living only inside it cannot be unit-tested. The same argument produced the injectable predicate in `atomic_write`. The FIRST observation adopts rather than reporting a jump, because a fresh `Nes` starts its counter at zero and "never observed" must stay distinguishable from "observed a zero" -- otherwise loading a ROM and immediately loading a save state compares 0 against 0 and misses it. `clear_rom_bound_analysis` calls `forget()` for the same reason: a generation from the previous cartridge is not comparable with the new core's. Only telemetry RECONSTRUCTED FROM A RUN is cleared. Watch lists and breakpoints are user-authored and survive, under the rule v2.3.9 settled for ROM transitions. A timeline jump is a weaker event than a cartridge change, so it can only ever clear a subset of what that hook does -- never more. ACCURACY -- VERIFIED, NOT ASSERTED `rustynes-core` changes, so the contract was re-run rather than reasoned about: AccuracyCoin (RAM): pass rate = 100.00% over 141 assigned tests nestest: test result: ok. 1 passed (The framebuffer decoder also reports 100.00% over 120 cells; the RAM decoder is the authoritative one.) MUTATIONS -- six, each confirmed to have actually RUN its named test loud restore stops bumping .............. CAUGHT quiet restore ALSO bumps ................ CAUGHT reset stops bumping ..................... CAUGHT first observation reports a jump ........ CAUGHT forget() does nothing ................... CAUGHT counter behaves as if serialized ........ CAUGHT GATES cargo fmt --all --check ......................... clean cargo clippy --workspace --all-targets -D warn .. clean clippy wasm32: default / wasm-canvas ............ clean RUSTDOCFLAGS=-D warnings cargo doc --workspace .. clean no_std thumbv7em-none-eabihf .................... clean rustynes-core lib ............................... 188 passed rustynes-frontend lib ........................... 564 passed --- crates/rustynes-core/src/nes.rs | 171 +++++++++++++++++++ crates/rustynes-frontend/src/debugger/mod.rs | 129 +++++++++++++- 2 files changed, 299 insertions(+), 1 deletion(-) diff --git a/crates/rustynes-core/src/nes.rs b/crates/rustynes-core/src/nes.rs index 0b2fda5b..e2e3142e 100644 --- a/crates/rustynes-core/src/nes.rs +++ b/crates/rustynes-core/src/nes.rs @@ -173,9 +173,39 @@ pub struct Nes { /// Whether the per-frame exec-PC log is recording. Default `false`. #[cfg(feature = "debug-hooks")] exec_logging: bool, + /// v2.4.0 item B — a monotonic marker that changes whenever this `Nes` + /// jumps to a different point on its timeline. + /// + /// **Session-local, and deliberately NOT serialized.** The counter's only job + /// is to be *different* after a discontinuity. Serializing it would put an + /// OLD value back on restore, so loading a state saved earlier in the same + /// session could hand a consumer a generation it has already seen — and the + /// consumer would conclude nothing jumped at the exact moment something did. + /// A session-local monotonic counter cannot do that: it only ever increases, + /// so any restore produces a value no consumer has seen. + /// + /// It exists because the alternative — each consumer remembering the last + /// `cycle()` it saw and noticing a non-monotonic step — cannot see a restore + /// to a LATER state. The counter can, and needs no cooperation from any call + /// site, which matters because two of the four timeline jumps (wasm + /// load-state, and rewind) are not reachable from a patchable frontend call + /// site at all. + timeline_generation: u64, } impl Nes { + /// The current timeline generation — see the field's documentation. + /// + /// A consumer holds the last value it saw and clears itself when this + /// differs. It is meaningless to compare across two different `Nes` + /// instances: a fresh one starts at zero, which is why ROM changes are + /// handled by their own hook (`DebuggerOverlay::clear_rom_bound_analysis`) + /// rather than by this counter. + #[must_use] + pub const fn timeline_generation(&self) -> u64 { + self.timeline_generation + } + /// Returns a reference to the internal WRAM. pub fn wram(&self) -> &[u8] { self.bus.ram.as_ref() @@ -243,6 +273,7 @@ impl Nes { exec_log: Vec::new(), #[cfg(feature = "debug-hooks")] exec_logging: false, + timeline_generation: 0, }) } @@ -282,6 +313,7 @@ impl Nes { exec_log: Vec::new(), #[cfg(feature = "debug-hooks")] exec_logging: false, + timeline_generation: 0, }) } @@ -350,6 +382,7 @@ impl Nes { exec_log: Vec::new(), #[cfg(feature = "debug-hooks")] exec_logging: false, + timeline_generation: 0, }) } @@ -411,6 +444,7 @@ impl Nes { exec_log: Vec::new(), #[cfg(feature = "debug-hooks")] exec_logging: false, + timeline_generation: 0, }) } @@ -488,12 +522,18 @@ impl Nes { /// Reset (warm boot). Preserves WRAM; reloads PC from `$FFFC/D`. pub fn reset(&mut self) { + // v2.4.0 item B — a warm reset lands somewhere else on the timeline, so a + // reconstructed call stack and the access counters describe a run that no + // longer exists. + self.timeline_generation = self.timeline_generation.wrapping_add(1); self.bus.reset(); self.cpu.reset(&mut self.bus); } /// Power-cycle (cold boot). Zeroes WRAM, re-rolls phase, reloads vectors. pub fn power_cycle(&mut self) { + // v2.4.0 item B — see `reset`; a cold boot is the larger discontinuity. + self.timeline_generation = self.timeline_generation.wrapping_add(1); self.bus.power_cycle(); // Cold-boot path: see comment in `from_rom`. self.cpu = Cpu::power_on(); @@ -2078,6 +2118,28 @@ impl Nes { /// ([`Self::restore`] — the ring is invalidated) from same-timeline /// machine restores ([`Self::restore_quiet`] — the ring stays). fn restore_inner(&mut self, data: &[u8], clear_rewind: bool) -> Result<(), SnapshotError> { + // v2.4.0 item B — a timeline jump, but only when this is a LOUD restore. + // + // `clear_rewind` already draws exactly the distinction the counter needs, + // so it is reused rather than duplicated: `true` means a user-driven load + // that invalidates the rewind history, `false` means a same-timeline + // machine-driven restore (run-ahead's per-frame rollback, netplay's + // rollback-resimulate) where the history stays valid. + // + // A same-timeline restore must NOT bump. That is the same rule the + // provenance stash follows from the other direction — every same-timeline + // restore has to carry the state that lives outside the save state — and + // getting it wrong here would clear a consumer's telemetry sixty times a + // second under run-ahead, which is worse than the stale-telemetry defect + // this counter exists to fix. + // + // Bumped BEFORE the restore can fail, deliberately. A partially-applied + // restore is a discontinuity whether or not it completed, and a consumer + // that keeps stale telemetry because the jump errored is the bug in its + // most confusing form. + if clear_rewind { + self.timeline_generation = self.timeline_generation.wrapping_add(1); + } // Restore bus first — it consumes BUS / PPU / APU / MAP sections. self.bus.restore(data)?; // Then walk the sections again to find the CPU body. @@ -3412,6 +3474,115 @@ mod tests { assert_ne!(nes.cycle(), cycle_at_6, "captured frame 5, not frame 6"); } + // ---- v2.4.0 item B — the timeline generation counter ---- + + /// A LOUD restore is a timeline jump and must bump the generation. + /// + /// This is the defect the counter exists for: v2.3.9 cleared stale debug + /// telemetry on a ROM change and could not clear it on a save-state load, + /// because two of the four jump paths are not reachable from a patchable + /// frontend call site. + #[test] + fn a_loud_restore_bumps_the_timeline_generation() { + let rom = synth_nrom(16, 8); + let mut nes = Nes::from_rom(&rom).unwrap(); + nes.run_frame(); + let blob = nes.snapshot(); + let before = nes.timeline_generation(); + nes.run_frame(); + nes.restore(&blob).expect("restore"); + assert!( + nes.timeline_generation() > before, + "a user-driven load did not register as a timeline jump" + ); + } + + /// A QUIET restore is the SAME timeline and must NOT bump. + /// + /// Run-ahead restores every frame and netplay rollback restores on every + /// correction. Bumping here would clear a consumer's telemetry sixty times a + /// second — worse than the stale-telemetry defect the counter fixes. + #[test] + fn a_quiet_restore_does_not_bump_the_timeline_generation() { + let rom = synth_nrom(16, 8); + let mut nes = Nes::from_rom(&rom).unwrap(); + nes.run_frame(); + let blob = nes.snapshot(); + let before = nes.timeline_generation(); + nes.run_frame(); + nes.restore_quiet(&blob).expect("quiet restore"); + assert_eq!( + nes.timeline_generation(), + before, + "a same-timeline restore was reported as a jump; under run-ahead this \ + fires every frame" + ); + } + + /// Rewind is a jump, and it reaches the counter without its own call site. + #[test] + fn rewind_bumps_the_timeline_generation() { + let rom = synth_nrom(16, 8); + let mut nes = Nes::from_rom(&rom).unwrap(); + nes.enable_rewind_with(2 * 1024 * 1024, 1); + for _ in 0..4 { + nes.run_frame(); + } + let before = nes.timeline_generation(); + assert!(nes.rewind_step_back(), "step back"); + assert!( + nes.timeline_generation() > before, + "rewind did not register as a timeline jump" + ); + } + + /// Reset and power-cycle are discontinuities too. + #[test] + fn reset_and_power_cycle_bump_the_timeline_generation() { + let rom = synth_nrom(16, 8); + let mut nes = Nes::from_rom(&rom).unwrap(); + let a = nes.timeline_generation(); + nes.reset(); + let b = nes.timeline_generation(); + assert!(b > a, "warm reset did not bump"); + nes.power_cycle(); + assert!(nes.timeline_generation() > b, "power cycle did not bump"); + } + + /// **The counter must not be serialized**, and this is the assertion that + /// pins it. + /// + /// Serializing it would put an OLD value back on restore, so loading a state + /// saved earlier in the same session could hand a consumer a generation it + /// has already seen — and the consumer would conclude nothing jumped at the + /// exact moment something did. This test reproduces precisely that shape: + /// snapshot at generation N, advance the generation past N, then restore. If + /// the counter round-tripped, the value would come back as N. + #[test] + fn a_restore_never_hands_back_a_generation_a_consumer_has_seen() { + let rom = synth_nrom(16, 8); + let mut nes = Nes::from_rom(&rom).unwrap(); + nes.run_frame(); + let blob = nes.snapshot(); + let at_snapshot = nes.timeline_generation(); + + // Advance the generation well past the snapshot's value. + for _ in 0..3 { + nes.reset(); + } + let seen = nes.timeline_generation(); + assert!(seen > at_snapshot); + + nes.restore(&blob).expect("restore"); + assert!( + nes.timeline_generation() > seen, + "the generation went BACKWARDS to {} (a consumer had already seen {seen}), \ + so the counter is being carried in the save state -- which defeats its \ + only purpose", + nes.timeline_generation() + ); + } + #[test] fn rewind_disabled_no_op() { let rom = synth_nrom(16, 8); diff --git a/crates/rustynes-frontend/src/debugger/mod.rs b/crates/rustynes-frontend/src/debugger/mod.rs index fd6ed8c1..d7f97d35 100644 --- a/crates/rustynes-frontend/src/debugger/mod.rs +++ b/crates/rustynes-frontend/src/debugger/mod.rs @@ -644,8 +644,47 @@ pub struct PreparedShell { pixels_per_point: f32, } +/// v2.4.0 item B — remembers the last `Nes::timeline_generation` seen, and +/// reports when it changes. +/// +/// A separate type rather than an `Option` field on the overlay, for one +/// reason: `DebuggerOverlay::new` needs a window and a wgpu device, so anything +/// living only inside it cannot be unit-tested. The decision this makes — adopt, +/// ignore, or report a jump — is exactly the part worth testing, and it needs no +/// GPU to exercise. (The same argument produced the injectable predicate in +/// `crate::atomic_write`.) +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub struct TimelineWatch { + seen: Option, +} + +impl TimelineWatch { + /// Record `generation`; returns `true` if this is a jump the caller should + /// react to. + /// + /// The FIRST observation adopts rather than reporting. A fresh `Nes` starts + /// its counter at zero, so "never observed" and "observed a zero" have to be + /// distinguishable — otherwise loading a ROM and immediately loading a save + /// state compares 0 against 0 and misses the jump. + pub fn observe(&mut self, generation: u64) -> bool { + self.seen + .replace(generation) + .is_some_and(|prev| prev != generation) + } + + /// Drop the remembered value, so the next `observe` adopts. + /// + /// Called on a ROM change: a generation from the previous cartridge is not + /// comparable with the new `Nes`'s, which restarts at zero. + pub const fn forget(&mut self) { + self.seen = None; + } +} + /// State of the debugger overlay. pub struct DebuggerOverlay { + /// v2.4.0 item B — tracks `Nes::timeline_generation` across frames. + timeline: TimelineWatch, /// egui frontend state (window-event integration). state: egui_winit::State, /// egui rendering pipeline (wgpu-backed). @@ -943,6 +982,8 @@ impl DebuggerOverlay { Self { state, renderer, + // v2.4.0 item B — no observation yet; the first frame adopts. + timeline: TimelineWatch::default(), visible: false, // Default the CPU/PPU sub-windows CLOSED so opening the debugger // (`~`) just shows the toolbar; the user opens the panels they want @@ -1175,6 +1216,11 @@ impl DebuggerOverlay { /// entry point means the next ROM-bound analysis panel is one line away from /// being correct instead of one omission away from being wrong. pub fn clear_rom_bound_analysis(&mut self) { + // v2.4.0 item B — forget the generation as well. A fresh `Nes` restarts + // the counter at zero, so a value remembered from the previous cartridge + // is not comparable with the new one's; forgetting makes the next frame + // adopt rather than compare. + self.timeline.forget(); self.clear_latency_report(); self.atlas_ui.clear(); // v2.3.7 — Audio Provenance. Registered here rather than given its own @@ -1729,6 +1775,14 @@ impl DebuggerOverlay { /// engine's `on_frame`. Purely observational (it only reads `nes`), so /// determinism is unaffected. pub fn pump_watchpoints(&mut self, nes: &mut Nes) { + // v2.4.0 item B — did the emulator jump somewhere else on its timeline + // since the last frame? Checked here rather than at each call site that + // could cause one, because two of the four cannot be reached from a call + // site at all: the wasm load-state path restores inside a `spawn_local` + // task holding only a cloned handle, and rewind happens entirely inside + // the core. v2.3.9 patched the two that WERE reachable and recorded that + // it had covered one case of four. + self.sync_timeline(nes.timeline_generation()); // Refresh the hex-editor heatmap from THIS frame's access log first // (before `watch_ui.pump` re-arms the flag for the next frame). self.memory_ui.refresh_heatmap(nes); @@ -1809,6 +1863,28 @@ impl DebuggerOverlay { self.access_counter.reset(); } + /// v2.4.0 item B — clear timeline-bound telemetry when the core has jumped. + /// + /// Takes the generation rather than the `Nes` so it can be tested without + /// constructing an emulator, and so the one comparison lives in one place. + /// + /// The first observation ADOPTS the current value instead of treating it as a + /// change. Otherwise every ROM load would clear telemetry twice — once via + /// `clear_rom_bound_analysis` and once here — which is harmless but makes the + /// two mechanisms indistinguishable when debugging which of them fired. + /// + /// Only the telemetry that is *reconstructed from a run* is cleared. Watch + /// lists, breakpoints and the source map are user-authored and survive, under + /// the same rule v2.3.9 settled for ROM transitions: derived output is + /// discarded, user-authored input is kept. A timeline jump is a weaker event + /// than a cartridge change, so it can only ever clear a subset of what that + /// hook does — never more. + pub fn sync_timeline(&mut self, generation: u64) { + if self.timeline.observe(generation) { + self.reset_debug_telemetry(); + } + } + /// v1.7.0 "Forge" Workstream C (C3) — load a ca65/cc65 `.dbg` file's `text` /// into the source-line map, recording a status line. The `name` is for the /// status message only. Display-only; never touches the core. @@ -3159,7 +3235,58 @@ impl DebuggerOverlay { #[cfg(test)] mod tests { - use super::chip_panels_open; + use super::{TimelineWatch, chip_panels_open}; + + /// The first observation must ADOPT, not report a jump. + /// + /// A fresh `Nes` starts at generation zero. Without this, every ROM load + /// would clear telemetry twice — once via `clear_rom_bound_analysis`, once + /// here — which is harmless but makes the two mechanisms indistinguishable + /// when working out which of them fired. + #[test] + fn the_first_observation_adopts() { + let mut w = TimelineWatch::default(); + assert!(!w.observe(0), "the first observation reported a jump"); + assert!(!w.observe(0), "an unchanged generation reported a jump"); + } + + /// A changed generation is a jump, exactly once. + #[test] + fn a_changed_generation_reports_once() { + let mut w = TimelineWatch::default(); + assert!(!w.observe(7)); + assert!(w.observe(8), "the change was not reported"); + assert!(!w.observe(8), "the same change was reported twice"); + } + + /// `forget` must make the next observation adopt rather than compare. + /// + /// This is what makes a ROM change safe: the new `Nes` restarts at zero, and + /// comparing that against a value remembered from the previous cartridge is + /// meaningless in both directions — it can report a jump that did not happen, + /// or (when the old value happened to be zero) miss one that did. + #[test] + fn forget_makes_the_next_observation_adopt() { + let mut w = TimelineWatch::default(); + assert!(!w.observe(5)); + w.forget(); + assert!( + !w.observe(0), + "after a ROM change, a fresh core's zero was compared against the old \ + cartridge's generation" + ); + assert!(w.observe(1), "tracking did not resume after forget"); + } + + /// The counter is `u64` and the core bumps with `wrapping_add`, so wrap is + /// reachable in principle. Whatever it wraps to must still register as a + /// change, because "different" is the only property this type needs. + #[test] + fn a_wrapped_generation_still_reads_as_a_change() { + let mut w = TimelineWatch::default(); + assert!(!w.observe(u64::MAX)); + assert!(w.observe(0), "a wrap to zero was not reported as a change"); + } /// A minimal mirror of the overlay's chip `show_*` flags + the cached /// `visible` field, driven by the SAME [`chip_panels_open`] predicate the From 56112974bec92d14d3c93a743826c2acdf28fa88 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 02:42:06 -0400 Subject: [PATCH 04/20] docs(libretro): record the pending upstream sync as a diff, not a description v2.4.0 item A, local half. Both upstream surfaces were fetched read-only and compared against this tree, and the exact change is written down so the human step is a COPY rather than a re-derivation -- which is the same reasoning behind `libretro_info_audit.rs`. The v2.3.5 incident happened precisely because a re-derivation was asked of a human and not performed. The result is smaller than the plan assumed, in one direction and larger in the other. libretro-super's `dist/info/rustynes_libretro.info` needs ONE LINE: `display_version` v2.3.5 -> v2.3.9. Everything else is already in sync, including `license = "GPLv3+"` (landed upstream 2026-08-16 via libretro-super#2069) and the description's 174-mapper-family figure. Verified by diffing the fetched upstream file against this repo's copy: two changed lines, which is that field and its counterpart. So the specific failure v2.3.5 found -- `.info` advertising MIT/Apache-2.0 eleven days after the relicense -- is closed, and what remains is ordinary four-release version drift. libretro/docs' `docs/library/rustynes.md` is the one still wrong, and it is the LICENCE again: The RustyNES core is licensed under - MIT OR Apache-2.0 RustyNES has been GPL-3.0-or-later since v2.2.9 (ADR 0036), as a derivative work of GPL emulators. `libretro/docs#1180` is open against exactly this, was filed at the time, and has not been actioned upstream. That page is what a user reads BEFORE the `.info`, so of the two surfaces the stale one is the more visible. Neither PR is opened here. Both are outward-facing changes to third-party repositories this project does not own, so they are prepared and left for a maintainer. It is also why the local audit deliberately cannot see upstream: a test that could would be a test that silently disagreed with a repository nobody here controls. Documentation only; markdownlint passes. --- docs/libretro/UPSTREAM_SYNC.md | 64 ++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/docs/libretro/UPSTREAM_SYNC.md b/docs/libretro/UPSTREAM_SYNC.md index 4a7c6911..f585a2b9 100644 --- a/docs/libretro/UPSTREAM_SYNC.md +++ b/docs/libretro/UPSTREAM_SYNC.md @@ -136,3 +136,67 @@ Provide a clear description of what changed in RustyNES to warrant the update. Once the Libretro maintainers accept and merge your PR(s) into their upstream `master` branch, your commits become a permanent part of their history. At this point, you can safely navigate to your repository settings on GitHub and **delete the fork**. When you need to make another update in the future, simply return to Step 1. + +--- + +## Pending sync — measured 2026-08-20, against upstream `master` + +v2.4.0 item A. The obligation this file exists to discharge, with the **actual +diff** rather than a description of one, so the human step is a copy and not a +re-derivation. That is the same reason `libretro_info_audit.rs` exists: the v2.3.5 +incident happened because a re-derivation was asked of a human and not performed. + +Both surfaces were fetched read-only and compared. The result is smaller than +expected, and the shape is worth recording. + +### 1. `libretro/libretro-super` — `dist/info/rustynes_libretro.info` + +**One line.** Everything else is already in sync — including `license = "GPLv3+"`, +which `libretro-super#2069` landed on 2026-08-16, and the description's +`174 mapper families`. + +```diff +-display_version = "v2.3.5" ++display_version = "v2.3.9" +``` + +Confirmed by diffing the upstream file against this repo's copy: two changed +lines total, which is the one field and its counterpart. + +That the licence is already correct upstream is the part worth noting. The v2.3.5 +release found `.info` advertising MIT/Apache-2.0 eleven days after the GPL +relicense; that specific failure is closed, and what remains is ordinary version +drift of four releases. + +### 2. `libretro/docs` — `docs/library/rustynes.md` + +**Still wrong, and it is the licence again.** The page reads: + +```markdown +The RustyNES core is licensed under + +- MIT OR Apache-2.0 +``` + +RustyNES has been **GPL-3.0-or-later** since v2.2.9 (ADR 0036), as a derivative +work of GPL emulators. `libretro/docs#1180` is open against exactly this and was +filed at the time; it has not been actioned upstream. + +This is the surface the v2.3.5 work did **not** reach, and it is the one a user +reads before the `.info`. The correction is: + +```diff + The RustyNES core is licensed under + +-- MIT OR Apache-2.0 ++- GPL-3.0-or-later +``` + +### Why these are not opened automatically + +Both are pull requests against third-party repositories — outward-facing actions +on projects this one does not own. They are prepared here and left for a +maintainer to open, which is also why the audit in +`crates/rustynes-test-harness/tests/libretro_info_audit.rs` deliberately cannot +see upstream: a test that could would be a test that silently disagreed with a +repository nobody here controls. From b3252a6e2890eea0b69e237bbdbb320e13e7bcc2 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 08:51:18 -0400 Subject: [PATCH 05/20] docs(libretro): record both upstream PRs as filed, and the endpoint that misled me v2.4.0 item A, closing the local half. Both surfaces are now proposed upstream. libretro-super#2074 is opened: one line, display_version v2.3.5 -> v2.3.9. Verified before pushing rather than after -- the branch's dist/info/rustynes_libretro.info is now BYTE-IDENTICAL to this repository's copy. That is precisely the property libretro_info_audit.rs exists to make possible: the sync is a copy, not a re-derivation performed by hand. libretro/docs#1180 needed nothing. It has been open since 2026-08-16, and it is a PULL REQUEST rather than an issue -- re-verified today as OPEN, MERGEABLE/CLEAN, +1/-1, with zero comments. Correct, still applicable, simply unreviewed. A second PR would have been a duplicate. The misreading that nearly produced that duplicate is recorded, because it is reusable: "gh api repos/OWNER/REPO/issues/1180" RETURNS THE PULL REQUEST, since GitHub's issues endpoint serves PRs too. An earlier pass here ran exactly that, saw "#1180 open -- Correct the RustyNES core license", and concluded the docs fix still needed filing. "gh pr view" is the query when the question is whether a change is already proposed; the issues endpoint cannot answer it. Also cross-referenced #2074 on #1180, so a reviewer picking up either one can see that the docs page is now the last surface still showing the pre-relicense terms. Documentation only; markdownlint passes. Note on this commit: its first version was written with `git commit -m` and lost three backtick-quoted commands to shell substitution -- zsh evaluated them, and `` was read as an input redirect. Amended from a file. Commit bodies in this project carry command examples routinely, so -m is the wrong tool for them. --- docs/libretro/UPSTREAM_SYNC.md | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/libretro/UPSTREAM_SYNC.md b/docs/libretro/UPSTREAM_SYNC.md index f585a2b9..512838bf 100644 --- a/docs/libretro/UPSTREAM_SYNC.md +++ b/docs/libretro/UPSTREAM_SYNC.md @@ -192,11 +192,29 @@ reads before the `.info`. The correction is: +- GPL-3.0-or-later ``` -### Why these are not opened automatically +### Status — both filed, 2026-08-20 + +- **`libretro-super#2074`** — opened today. One line, `display_version` v2.3.5 -> + v2.3.9. Verified before pushing: the branch's `dist/info/rustynes_libretro.info` + is now **byte-identical** to this repository's copy, which is the property + `libretro_info_audit.rs` exists to make possible — the sync is a copy rather + than a re-derivation. +- **`libretro/docs#1180`** — **already open since 2026-08-16**, and it is a PULL + REQUEST, not an issue. Re-verified today: `OPEN`, `MERGEABLE / CLEAN`, `+1/-1`, + **zero comments** — correct, still applicable, simply unreviewed upstream. A + second PR would be a duplicate. + + Worth recording the misreading that nearly produced one: `gh api + repos/libretro/docs/issues/1180` returns the PR, because GitHub's *issues* + endpoint serves pull requests too. That is what made an earlier pass here + describe it as "an open issue" and conclude the docs fix still needed filing. + Use `gh pr view` when the question is whether a change is already proposed. + +### Why these are opened by hand rather than by tooling Both are pull requests against third-party repositories — outward-facing actions -on projects this one does not own. They are prepared here and left for a -maintainer to open, which is also why the audit in +on projects this one does not own, so they are a maintainer decision rather than +something a sync script should perform. It is also why the audit in `crates/rustynes-test-harness/tests/libretro_info_audit.rs` deliberately cannot see upstream: a test that could would be a test that silently disagreed with a repository nobody here controls. From da57218b617705ae07d6184a8846e85b0118dcc2 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 08:55:03 -0400 Subject: [PATCH 06/20] docs(agents): the upstream-PR cadence, and eight findings from the v2.4.0 work THE CADENCE RULE (maintainer decision, 2026-08-20) Upstream PRs are opened only on MINOR or MAJOR releases -- a `vX.Y.0` where `X` or `Y` changed. Patch releases do NOT trigger an upstream sync: the `.info` `display_version` is allowed to lag through a `v2.4.1`..`v2.4.9` run and is brought current at the next `vX.Y.0`. Next scheduled sync: **v2.5.0**. Attached to the existing bullet rather than added beside it, because that bullet already carries the one OVERRIDE and the two must be read together: a **licence change syncs immediately**, regardless of version. That is what the v2.3.5 incident was about -- RetroArch advertised MIT/Apache-2.0 for eleven days after the GPL relicense -- and it stays on the same footing as a release. EIGHT OPERATING NOTES, ALL FROM THINGS THAT ACTUALLY HAPPENED * `gh api repos/OWNER/REPO/issues/N` RETURNS PULL REQUESTS. This nearly opened a duplicate upstream PR: a pass ran exactly that against libretro/docs#1180, saw an "open issue", and concluded the docs licence fix still needed filing -- into a plan, a commit body and a user-facing summary. #1180 is a PR, open since 2026-08-16, MERGEABLE/CLEAN. Use `gh pr view` for "is this already proposed". * Never write a commit body with `git commit -m` here. zsh treats backticks as command substitution and `` as an input redirect; a message documenting three `gh` invocations lost all three and emitted `no such file or directory: owner`. This project's house style puts command examples in commit bodies routinely, so `-m` is structurally wrong for them -- use `-F` and then grep the result for each phrase that was supposed to survive. * A test that reimplements its subject is testing itself. Found in a test written FOR a review finding: it declared a local `strip` helper and asserted against that, so deleting the production code came back NOT CAUGHT. Only the mutation pass could see it. The fix -- extract the decision into a named item both sides call -- was needed THREE times this release (the atomic-write predicate, `TimelineWatch`, and this), and in all three the code READ as testable beforehand. * Never byte-slice in a panic or format path. `&text[at..at+24]` panics inside a multi-byte character, and these docs are full of em-dashes -- so the audit crashed while formatting its own diagnostic. A diagnostic that can crash the diagnosis is worse than none. * Verify a reviewer's claim before writing the fix, especially when their other findings were right. A claim that `starts_with("[workspace.package]")` matches sub-tables is false (the literal ends with `]`), and the fix plus a commit body describing "the regression I introduced" were written before it was tested. * The let-chains claim is false and has been raised SEVEN times. Stable in edition 2024; identical construct on `main` since v2.3.5; compiled green at five SHAs. Refute on the CURRENT SHA rather than by reference to earlier ones. * The workspace cannot carry a SemVer pre-release version -- cargo rejects it before any test runs, because a caret requirement does not match a pre-release. * `release_anchor_audit.rs` pins 15 anchors across 10 documents and fails closed; rewording an anchor means updating `ANCHORS` in the same change. Documentation only. markdownlint passes (one MD038 fixed: a code span may not begin with a space). --- AGENTS.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 33c56db6..a4d76f56 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -226,7 +226,7 @@ These cross-cutting decisions span multiple files. Reading individual chip docs - **The bot-comment ceremony must read the review BODIES, not just the resolvable threads.** CodeRabbit posts "Outside diff range" and other suppressed findings **inside the review body**, where they are invisible to a resolve-every-thread sweep — and Copilot does the same. This has now cost the project three times: issue #360 (an untested attestation path) reached `main` unaddressed; two findings of the same class on #357 were genuine defects, **one critical** (two threads producing frames during fast-forward under threaded display-sync, fixed in #358); and a **use-after-free** in the v2.3.5 libretro controller tables was caught only because the review body was read. A green "all threads resolved" is not evidence the review was addressed. Fetch the bodies explicitly — `gh pr view --json reviews --jq '.reviews[].body'` — and triage every finding in them before merging. - **lz4_flex 0.14+ requires the crate's own `alloc` feature explicitly** for `compress_prepend_size`/`decompress_size_prepended` (used by `rewind.rs`/`zwinder.rs`) — it split real no_std support into an `alloc`-vs-`std` distinction that didn't exist in 0.13. A `cargo build --workspace` will NOT catch a missing `alloc` feature here because `rustynes-core`'s own default-on `std` feature implies it via cargo's feature unification; only a standalone `cargo build -p rustynes-core --target thumbv7em-none-eabihf --no-default-features` (the exact CI `no_std build` job) will. Run that command locally before pushing any bump that touches this dependency. -- **The libretro `.info` RetroArch reads is a DIFFERENT FILE from this repo's, and it went stale for eleven days.** RetroArch downloads `dist/info/rustynes_libretro.info` from `libretro/libretro-super`; `crates/rustynes-libretro/rustynes_libretro.info` is an unrelated copy that nothing syncs and nothing compared. So the v2.2.9 GPL relicense reached `Cargo.toml`, `NOTICE`, `deny.toml`, the SPDX headers and the local `.info` — and **not** the file users actually see, which went on advertising "MIT OR Apache-2.0" at `display_version = v2.2.1`. Both upstream PRs had merged *exactly two weeks before* the relicense, so no sync could have carried it. **A license change is now a mandatory upstream-sync trigger**, on the same footing as a release. `crates/rustynes-test-harness/tests/libretro_info_audit.rs` pins the local file against the workspace manifest so the sync is a *copy*, never a re-derivation; it cannot see upstream, so the sync itself stays a human step. libretro `.info` uses short license tokens, not SPDX, and marks "or later" with a trailing `+` (tallied across all 316 upstream cores: `GPLv2` x100, `GPLv3` x64, `GPLv2+` x19, `GPLv3+` x5) — RustyNES is **`GPLv3+`**; a bare `GPLv3` understates it as GPL-3.0-only. Full detail + the surface table: `docs/libretro/UPSTREAM_SYNC.md`. +- **The libretro `.info` RetroArch reads is a DIFFERENT FILE from this repo's, and it went stale for eleven days.** RetroArch downloads `dist/info/rustynes_libretro.info` from `libretro/libretro-super`; `crates/rustynes-libretro/rustynes_libretro.info` is an unrelated copy that nothing syncs and nothing compared. So the v2.2.9 GPL relicense reached `Cargo.toml`, `NOTICE`, `deny.toml`, the SPDX headers and the local `.info` — and **not** the file users actually see, which went on advertising "MIT OR Apache-2.0" at `display_version = v2.2.1`. Both upstream PRs had merged *exactly two weeks before* the relicense, so no sync could have carried it. **Upstream PRs are opened only on MINOR or MAJOR releases** — a `vX.Y.0` where `X` or `Y` changed. Patch releases do NOT trigger an upstream sync; the `.info` `display_version` is allowed to lag through a `v2.4.1`..`v2.4.9` run and is brought current at the next `vX.Y.0`. **Next scheduled upstream sync: v2.5.0** (maintainer decision, 2026-08-20). The one override is a **licence change, which syncs immediately** regardless of version — that is what this bullet's incident was about, and it stays on the same footing as a release. `crates/rustynes-test-harness/tests/libretro_info_audit.rs` pins the local file against the workspace manifest so the sync is a *copy*, never a re-derivation; it cannot see upstream, so the sync itself stays a human step. libretro `.info` uses short license tokens, not SPDX, and marks "or later" with a trailing `+` (tallied across all 316 upstream cores: `GPLv2` x100, `GPLv3` x64, `GPLv2+` x19, `GPLv3+` x5) — RustyNES is **`GPLv3+`**; a bare `GPLv3` understates it as GPL-3.0-only. Full detail + the surface table: `docs/libretro/UPSTREAM_SYNC.md`. - **iOS/iPadOS/tvOS availability is a THIRD repo and a HARDCODED list — being on the buildbot buys nothing there.** **RESOLVED 2026-08-16** by `libretro/RetroArch#19416` (merged `76f60626984a`; verified against `master`, not the PR state — `rustynes` sits at line 268 between `reminiscence` and `sameboy`). The mechanism below is retained because it recurs for any other core and for the sibling forges, and because "in the build list" is not "installable": it ships with the next App Store RetroArch build. iOS cannot download cores (Apple bans fetching executable code), so the App Store build bundles a fixed set chosen by `pkg/apple/update-cores.sh` in `libretro/RetroArch`. That script has two lists: `allcores`, fetched *dynamically* from the buildbot directory (RustyNES is in it automatically), and `appstore_cores`, a hardcoded array (RustyNES is **absent**). The iOS/tvOS build phases run `rm -f ${SRCROOT}//modules/*.dylib` then `./update-cores.sh appstore`, so only the hardcoded list survives. One entry covers iOS + tvOS + macOS App Store. **Alphabetical order is mandatory** — `rustynes` sorts between `reminiscence` and `sameboy`; re-check the neighbours at submission time rather than trusting a line number. - **RetroArch retains SOME environment-callback pointers and copies others — the asymmetry is not documented in `libretro.h`, so check `runloop.c`.** `SET_CONTROLLER_INFO` shallow-`memcpy`s the outer `retro_controller_info` array but **retains** each entry's `types` pointer and dereferences it later when the Controls menu is built: the description arrays MUST be `'static` (a stack local compiles cleanly and hands the frontend a use-after-free). `SET_INPUT_DESCRIPTORS` is different — RetroArch walks it during the call and retains only the `description` string pointers — so a stack array is fine there. Never generalize from one to the other; read the handler. - **A fix that touches ONE call site of a shared code path may not fix the bug — and will report that it did.** v2.3.6 hit this squarely. Review reported that a latency measurement destroyed the user's rewind history; the fix changed `measure_in_place`'s FINAL restore to `restore_quiet` and stopped. Every *trial* still went through `Probe::run_uncounted`'s loud `nes.restore(..)`, and a measurement runs up to **21 trials** against the live emulator, so the ring was still being cleared twenty-one times over behind a fix that closed the thread. Before declaring a fix complete, grep for every caller of the mechanism, not just the one the report named. The corollary is about tests: my test for that fix would have asserted "ring not empty", which **passes while a second defect remains** — the ring in fact GREW, polluted with replayed frames that never happened on the user's timeline. Assert the state comes back EXACTLY as it was; a weaker assertion is how an incomplete fix clears review. @@ -250,5 +250,13 @@ These cross-cutting decisions span multiple files. Reading individual chip docs - **Bound every workflow job, and every network fetch inside one.** PR #400 bounded `ci.yml` and nothing else; v2.3.9 found **six** more unbounded workflows including `release-auto.yml` itself, after `Clippy Security Lints` hung **two hours** in a setup step and blocked the v2.3.7 release PR. Separately, apt provisioning hung **four times across two PRs in one day**, always in a setup/provisioning step and never in a compile or test step. A job timeout bounds the damage but cannot *notice*: a stalled fetch inside a 25-minute budget is indistinguishable from a slow job, and the run reports as `cancelled`, which reads as noise. `.github/scripts/apt-install-retry.sh` adds a per-command `timeout` plus three attempts, and warns on every attempt including ones that succeed — a run needing three and one needing one are identical in the conclusion, and that difference is the early warning. - **Summing two percentiles is as invalid as differencing them.** `docs/performance.md` records the subtraction case (a published table whose `work p95` sat below its `work p50`). The addition case bit the v2.3.9 Latency Oracle design: an end-to-end figure needs `render_work + render_lock` (+ `render_wait`), and `PerfView` exposes those as three **separate** series, so the design was not implementable from existing data — found by trying to write it. The one valid case is adding a **constant**: internal lag is `frames * frame_ms`, so `lag + render_work.p95` genuinely is a p95. That rescues exactly one series, which is why `PerfPanelState::render_work` deliberately exposes only that one. A true wall-clock figure needs a new single per-redraw series on `RenderPerf`. - **`grep -i` on a short token matches more than you mean.** Reading the AccuracyCoin result with `grep -iE "RAM.*pass rate"` matched the **framebuffer** line, because `-i` makes `RAM` match "f-ram-ebuffer" — and the framebuffer decoder is the known-buggy one reporting 120. The authoritative line is `AccuracyCoin (RAM): pass rate = 100.00% over 141 assigned tests`; match it case-sensitively, e.g. `grep -E "AccuracyCoin \((RAM|framebuffer)\)"` and read both. Same session, the same class of mistake produced two false negatives from patterns that could not match (`full \*\*2x2` against `**full 2x2`). **A pattern that cannot match looks exactly like content that is not there.** +- **`gh api repos/OWNER/REPO/issues/N` RETURNS PULL REQUESTS**, because GitHub's issues endpoint serves both. v2.4.0 item A nearly opened a duplicate upstream PR because of it: a pass ran exactly that against `libretro/docs#1180`, saw `#1180 open — Correct the RustyNES core license`, concluded "an open *issue*, so the fix still needs filing", and wrote that into a plan, a commit body and a user-facing summary. #1180 was a **pull request** filed 2026-08-16, `MERGEABLE / CLEAN`, `+1/-1`, awaiting review. When the question is *"is this change already proposed?"*, the query is `gh pr view N --repo O/R` or `gh pr list --author --state all`; the issues endpoint cannot distinguish them and its `pull_request` field is easy to miss. +- **Never write a commit body with `git commit -m` in this harness.** The shell is zsh, so **backticks are command substitution** and `` is an input redirect. A v2.4.0 message documenting three `gh` invocations lost all three to substitution and emitted `no such file or directory: owner` from a literal `repos///...` — the commit succeeded with a mangled body reading "recorded, because it is reusable: RETURNS THE PULL REQUEST". This project's house style puts command examples in commit bodies routinely, so `-m` is structurally the wrong tool: write the message to a file and use `git commit -F`. After amending, grep the message for each phrase that was supposed to survive. +- **A test that reimplements its subject is testing itself, and it will agree forever.** v2.4.0 hit this in a test written *for a review finding*: it declared a local `fn strip(t) { t.trim_start_matches([' ', '*']) }` and asserted against that, so deleting the production stripping came back **NOT CAUGHT**. Only the mutation pass could see it — the test passed, read correctly, and covered nothing. The fix is the one this release needed three times over: **extract the decision into a named item both the code and the test call.** The other two were `atomic_write`'s injectable rename predicate (with it hard-wired, the exhaustion branch is unreachable on Unix and a mutation returning `Ok(())` for a save that never happened went uncaught) and `TimelineWatch` (`DebuggerOverlay::new` needs a window and a wgpu device, so nothing living only inside it is unit-testable). In all three the code **read** as testable beforehand. +- **Never byte-slice in a panic or format path.** `&text[at..(at + 24).min(text.len())]` panics when the offset lands inside a multi-byte character, and these documents are full of em-dashes and arrows — so the audit would crash *while formatting the diagnostic*, replacing the message explaining the real failure with a char-boundary error about the reporting code. **A diagnostic that can crash the diagnosis is worse than none**, because the failure it exists to explain becomes harder to read than if the excerpt were omitted. Use `s.chars().take(n).collect::()`. +- **Verify a reviewer's claim before writing the fix, especially when their other findings were right.** On #427 a reviewer stated `starts_with("[workspace.package]")` also matches `[workspace.package.metadata]`. Plausible, a real class of bug, and the fix plus a commit body describing "the regression I introduced" were written before it was tested. It is **false**: the literal ends with `]` and the sub-table has `.` there, so the match is `false`; injecting such a sub-table and running the audit reads `2.3.9` under both forms. The same reviewer's two other findings that pass were both correct — which is exactly what makes the third easy to wave through. Adopt the change if it is better anyway (it was), but write down what is *true*, not a fix for a bug that never existed. +- **The libretro "let-chains are unstable" review claim is FALSE and has now been raised seven times.** `if let Some(x) = e && cond` is stable in **edition 2024**, which this workspace uses on a pinned stable 1.96.0; the identical construct has been in `libretro_info_audit.rs` on `main` since v2.3.5; and CI's `fmt + clippy + rustdoc` job has compiled it green at five distinct SHAs. Do not "fix" it. Refute with the edition, the existing site, and the green lint job **on the current SHA** rather than by reference to earlier ones. +- **The workspace cannot carry a SemVer pre-release version.** Setting `[workspace.package] version = "2.3.9-rc.1"` fails before any test runs: `failed to select a version for the requirement rustynes-apu = "^2.0.0" / candidate versions found which didn't match: 2.3.9-rc.1`. A caret requirement does not match a pre-release, so every intra-workspace dependency would have to be rewritten first. Relevant when reasoning about version parsing — `release_anchor_audit.rs` guards the case anyway, and its `version_core` is tested directly because the manifest route is unreachable. +- **`release_anchor_audit.rs` pins 15 release anchors across 10 documents** against `[workspace.package] version` (README badge + Current Release, `docs/STATUS.md`, both `AGENTS.md` anchors + the never-claim-later guard, `VERSION-PLAN.md` header and its `(current)` table row, `to-dos/ROADMAP.md`, `SUPPORT.md`, `SECURITY.md`, root `ROADMAP.md`, `OVERVIEW.md`, `ARCHITECTURE.md`). It **fails closed** — a marker matching nothing panics rather than silently checking nothing — and it also asserts the CHANGELOG header parses the way `release-auto.yml` parses it (the `- YYYY-MM-DD -` prefix, a quoted codename, then a parenthesised theme), since a malformed header degrades the published release title. **Rewording an anchor means updating `ANCHORS` in the same change**; the test will tell you, by name. <<< MC-PROJECT-END >>> From 728266bc82f67923f11f447e3d8a0b10c8414697 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 09:34:36 -0400 Subject: [PATCH 07/20] docs(changelog): record v2.4.0 items A-D under [Unreleased] Folds the four v2.4.0 items into the existing [Unreleased] sections rather than adding parallel ones -- #427 had already opened Added/Fixed/Changed there, and markdownlint's MD024 caught the duplicate headings before they landed. Covers: the shared atomic-write helper and the fourth call site the plan did not name; the timeline generation counter and why it deliberately does not bump on a same-timeline restore; the two skip_serializing_if fields and why both directions are tested; and the upstream sync, which turned out to be one line. --- CHANGELOG.md | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68f11259..fdcc79f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,69 @@ cycle-accurate core later replaced. ### Added +- **One atomic, durable file write for every path that persists user data.** + (v2.4.0 item C.) The seven-property write sequence v2.3.9 built for + `Config::save_to` is extracted into `crate::atomic_write` and adopted everywhere. + The plan named three call sites; there were **four**, and the fourth is the + instructive one. + + `save_state.rs` matters most and was named last: **a truncated save state is a + user's game progress**, a worse loss than a truncated config, and it was still + using the bare `fs::write` the config path had already been fixed for. It is also + the path most likely to be written under load — rewind capture, run-ahead and + netplay rollback all produce save states. + + `per_game.rs` was not in the plan at all, because it *looks* correct: it writes a + sibling temp file and renames, so a sweep for `fs::write`-onto-a-target clears + it. It held two of seven. No `fsync`, so the rename could commit a directory + entry pointing at bytes that never reached the medium; and a **fixed** scratch + name shared across every process and concurrent call — the exact failure the + mechanism exists to prevent, reintroduced by the mechanism. A partially-correct + implementation is harder to spot than an absent one. + + The config path **gains** something it never had: a bounded retry past a + transient Windows sharing violation. `MoveFileEx` fails if another process has + the target open, and an antivirus scanner or search indexer reading `config.toml` + is enough. POSIX has no such constraint, which is why it went unnoticed — and why + it would have surfaced as a Windows user reporting a save that failed for no + visible reason. When the attempts are exhausted the error **propagates**. + + Two mutations forced design changes rather than confirming the design. The retry + loop's predicate had to become a **parameter**: hard-wired, the exhaustion branch + is unreachable on Unix, and a mutation making it return `Ok(())` — silently + reporting a save that never happened — went **uncaught**. And the mode test was + asserting less than its name claimed, since `opts.mode(0o600)` at creation + already yields 0600 under any ordinary umask. Two properties are **not** + observable in-process and the module says so: `fsync` (needs a power loss) and + creation-mode (a race-window narrowing, where a test can only see the end state). + Neither should be deleted on the evidence that no test fails. + +- **A timeline generation counter, and the telemetry that reads it.** (v2.4.0 item + B.) v2.3.9 cleared stale debug telemetry on a ROM change and recorded that it + could not clear it on a save-state load: of the four ways the emulator jumps + timeline, only one is reachable from a patchable frontend call site — wasm + load-state restores inside a `spawn_local` task, and rewind happens entirely + inside the core. + + `Nes` now carries a session-local `timeline_generation`, and `restore_inner`'s + existing `clear_rewind` parameter already draws exactly the needed distinction, + so it is reused rather than duplicated. **This departs from the plan's + enumeration deliberately:** the plan listed netplay rollback as a bump site and + also stated the mechanism that forbids it — a same-timeline restore must *not* + bump. Netplay rollback and run-ahead both go through `restore_quiet` precisely + because they are same-timeline; bumping there would clear a user's telemetry + sixty times a second, which is worse than the defect being fixed. Both directions + are pinned by tests. + + **The counter is not serialized**, and that is load-bearing rather than a + preference: serializing it would put an *old* value back on restore, so loading a + state saved earlier in the same session could hand a consumer a generation it has + already seen. The plan asked for an entry in `snapshot_schema_audit.rs`; that file + audits the four chips, not `Nes`, so the property is pinned by an **executable** + assertion instead — snapshot at generation N, advance past N, restore, and assert + it did not come back to N. Simulating serialization makes it fail with exactly + that diagnostic. + - **A standing release-anchor audit — the drift v2.3.9 corrected by hand cannot recur silently.** `crates/rustynes-test-harness/tests/release_anchor_audit.rs` pins **15 anchors across 10 documents** against `[workspace.package] version`: @@ -50,8 +113,44 @@ cycle-accurate core later replaced. `(current)` marker, and a renamed CHANGELOG section — each fail the test they should and only that test. +### Fixed + +- **Two shipped features stop writing empty tables into an untouched config.** + (v2.4.0 item D.) `graphics.hd_packs` (v1.5.0) and `graphics.shader_presets` + (v1.2.0) both documented a pre-feature config as "byte-identical". Both were + byte-identical only until the first save: `#[serde(default)]` is a **load** + guarantee, and the TOML serializer emits an empty table for an empty collection. + v2.3.9 corrected the prose and deliberately left the behaviour, because changing + what a shipped feature writes is a separate decision; this is that decision. + + Both directions are tested, because a one-directional test passes just as happily + against a field that never persists anything — and the over-eager direction is + the dangerous one: an `is_empty` returning `true` unconditionally would silently + discard a user's saved presets on every save, a data-loss bug wearing the shape of + a tidiness fix. A third test asserts the property once for **the field that does + not exist yet**, since the defect being fixed is precisely "a field was added and + the save-side property was not considered". + ### Changed +- **The owed upstream libretro sync is filed, and it was smaller than expected.** + (v2.4.0 item A.) `libretro-super#2074` bumps `display_version` v2.3.5 → v2.3.9 — + **one line**. Everything else was already correct upstream, including + `license = "GPLv3+"`. Verified before pushing: the branch file is now + **byte-identical** to this repository's copy, which is the property + `libretro_info_audit.rs` exists to make possible. + + `libretro/docs#1180` needed nothing — it is a **pull request** open since + 2026-08-16, `MERGEABLE/CLEAN`, unreviewed, not an issue. The misreading that + nearly produced a duplicate is recorded because it is reusable: `gh api + repos/OWNER/REPO/issues/N` **returns pull requests**, since GitHub's issues + endpoint serves both. + + `AGENTS.md` now carries the cadence rule: **upstream PRs are opened only on + MINOR/MAJOR releases** (`vX.Y.0` where X or Y changed). Patch releases do not + sync; the next is **v2.5.0**. A **licence change overrides and syncs + immediately** — the rule that incident produced in the first place. + - **`to-dos/DEFERRED-AND-CARRYOVER-FEATURES.md` swept entry by entry**, against `main` @ `fdfb2c04`. Eleven entries struck, each carrying its evidence inline — a file that exists, a workflow line number, a test that says so — rather than a From 6878bdf2736836198825bb4af094ee44a2486668 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 10:16:22 -0400 Subject: [PATCH 08/20] fix(frontend): three silent successes in the atomic-write path, and a flaky test that hid one Review of this PR found three places `write_atomic` reported success it had not earned. All three share a shape worth naming: an error discarded at a call site, under a comment that explains the rest of the operation and reads as though it covered the discard too. set_permissions was swallowed with `let _ =`. The direction is what makes this more than a nitpick: the mode being applied is the mode the target ALREADY had, so a failure replaces a file at 0600 with one at the umask default -- wider than what it replaced -- and tells the caller nothing. A security property silently inverted, not a durability step merely skipped. It now propagates, after removing the scratch file so the original is left intact. The parent-directory sync_all was swallowed along with the File::open that fed it, so the entire durability barrier could be a no-op while the module's own platform table claimed "yes" for Unix. It now propagates, with one deliberate exception: EINVAL, and EBADF on some network mounts, mean this filesystem does not offer a directory fsync rather than the write failed, and failing a save outright on those mounts is a worse answer than proceeding. EIO -- the exact condition the sync exists to detect -- no longer passes as success. The occupied-scratch retry was a single attempt, justified by "advancing the counter cannot repeat a name within a process". True, and beside the point: the collision comes from a PREVIOUS process. A run that crashed mid-session orphans one scratch file per save it made, and pid reuse restarts the counter at zero, so two orphans defeat one retry and the save fails for a reason the user cannot act on. Now a loop bounded at SCRATCH_ATTEMPTS = 8 -- bounded rather than bare, because a directory rejecting creation for a persistent reason would otherwise hang, and a hang is a worse answer than an error. THE TEST THAT WAS ALREADY FLAKY Getting the last one under test surfaced something the suite was not reporting. Reaching the exhaustion branch through write_atomic means predicting the process-global SCRATCH_SEQ and planting a decoy at every name the call will pick -- and that prediction races, because cargo test runs in parallel and every sibling test calling write_atomic consumes sequence values. Measured rather than theorised. A serialising mutex over the three tests that PEEK at the counter still failed 2 runs in 5, because the tests doing the consuming are precisely the ones that never look at it. Which means the pre-existing single-decoy test had been latently flaky since it was written and had simply never lost the race -- it needs one value where the new test needs eight, so it was forgiving enough to hide the defect rather than immune to it. All three decisions are therefore extracted into named functions -- apply_mode_using, directory_fsync_is_unsupported, open_fresh_scratch -- and driven directly. On Unix none of the three failures can be arranged against a file this process just created and owns, so hard-wired call sites would have left every propagation path permanently unexercised. That is how the swallowed versions survived review to begin with, and it is the fourth time this release that "extract it so a test can reach it" was the actual fix rather than a stylistic preference. Eight consecutive module runs are stable at 19 passed. THE WASM32 GATE, AGAIN Once the Unix arm started propagating, the non-Unix sync_parent_dir had to match its signature -- and an always-Ok return is exactly what clippy's unnecessary_wraps objects to, on non-Unix targets only. Native clippy passed; the wasm32 gate did not. Clippy's suggested fix (return unit) would break the parity the shared call site depends on, since write_atomic ends in sync_parent_dir(&target) as its tail expression, so the lint is allowed locally with that reason recorded. This is the second cfg-specific lint this one function has needed, and both were visible only off Unix. DECLINED AND DEFERRED, WITH REASONS Iterative symlink resolution (link1 -> link2 -> missing currently replaces link2 rather than preserving the chain) is real but needs a cycle bound and its own tests; tracked for follow-up rather than bundled here. Pushing the cheats.rs save error to its caller is agreed in principle and is the same class as the swallowed latency-config save, but it is a UI change -- a Result signature plus status-bar plumbing at a call site reached from egui paint code -- and belongs in its own change. Logging the leaked temp file is declined: the cleanup calls ignore their results deliberately, because a cleanup that fails when the disk is full should not displace the primary error. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build and rustdoc with warnings as errors. Three mutations confirm the new assertions fail when the old behaviour is restored; a first mutation pass reporting "caught" was rejected on inspection because the mutants had not compiled, which is not evidence of anything. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- CHANGELOG.md | 30 ++ crates/rustynes-frontend/src/atomic_write.rs | 368 +++++++++++++++++-- 2 files changed, 365 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fdcc79f1..6f140ad5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,36 @@ cycle-accurate core later replaced. it would have surfaced as a Windows user reporting a save that failed for no visible reason. When the attempts are exhausted the error **propagates**. + **Review then found three more places the module reported success it had not + earned**, and the shape was the same each time: an error discarded at a call + site, under a comment explaining the *rest* of the operation. `set_permissions` + was swallowed — and the mode being applied is the mode the target **already + had**, so a failure replaces a 0600 file with one at the umask default, *wider + than what it replaced*, and says nothing. The parent-directory `sync_all` was + swallowed along with the `File::open` that fed it, so the entire durability + barrier could be a no-op while the module's own table claimed "yes" for Unix; + `EIO` — the exact condition the sync exists to detect — was reported as success. + Both now propagate, the sync excepting only the two errnos that mean *this + filesystem does not offer the barrier* (`EINVAL`, and `EBADF` on some network + mounts), since failing a save outright on those mounts is a worse answer than + proceeding. And the occupied-scratch retry was **one** attempt, on the reasoning + that the counter cannot repeat a name within a process — true, and beside the + point, because the collision comes from a *previous* process: a run that crashed + mid-session orphans one scratch file per save it made, and pid reuse restarts the + counter at zero, so two orphans defeat one retry. + + Getting the last one under test surfaced something else. Reaching the exhaustion + branch through `write_atomic` means predicting the process-global `SCRATCH_SEQ` + and planting a decoy at every name the call will pick — and **that prediction + races**, because every parallel test calling `write_atomic` consumes sequence + values. Measured rather than assumed: a serialising mutex over the three tests + that *peek* at the counter still failed 2 runs in 5, because the tests doing the + consuming are precisely the ones that never look at it. **The pre-existing + single-decoy test had been latently flaky since it was written and had simply + never lost the race.** All three decisions are now named functions driven + directly, which is the fourth time this release that "extract it so a test can + reach it" was the actual fix. + Two mutations forced design changes rather than confirming the design. The retry loop's predicate had to become a **parameter**: hard-wired, the exhaustion branch is unreachable on Unix, and a mutation making it return `Ok(())` — silently diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs index 899eb998..bca303a7 100644 --- a/crates/rustynes-frontend/src/atomic_write.rs +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -122,6 +122,54 @@ //! and the broken-link path), the exact mode after creation, the occupied-scratch //! retry, exhaustion propagating rather than reporting success, and the Unix //! single-attempt guarantee. +//! +//! # Two failures that were reported as success, and are not any more +//! +//! Found in review of the change that introduced this module, and worth recording +//! because both had the same shape: an error deliberately discarded at a call +//! site, under a comment explaining the *rest* of the operation. +//! +//! - **`set_permissions` (property 5).** Discarded with `let _ =`. The mode being +//! applied is the mode the target **already had**, so a failure replaces a file +//! at `0600` with one at the umask default — **wider than what it replaced** — +//! and reports success. It now propagates, after removing the scratch file. +//! - **The parent-directory `sync_all` (property 6).** Both the `File::open` and +//! the `sync_all` were discarded, so the entire durability barrier could be a +//! no-op while the table above claimed "yes" for Unix. It now propagates — +//! *except* for the two errnos that mean the filesystem does not offer the +//! barrier (`EINVAL`, and `EBADF` on some network mounts), since failing a save +//! outright on those mounts would be a worse answer than proceeding. `EIO` — +//! the exact condition the sync exists to detect — no longer passes as success. +//! +//! Both decisions are **extracted into named functions** (`apply_mode_using`, +//! `directory_fsync_is_unsupported`) for the same reason `rename_with_retry_using` +//! is: on Unix neither failure can be arranged in a unit test against a file this +//! process just created and owns, so hard-wired call sites would leave both +//! propagation paths permanently unexercised. An unexercised error path is how the +//! swallowed versions survived review to begin with. +//! +//! # One orphan was not enough +//! +//! Property 7's collision handling was a **single** retry, on the reasoning that +//! advancing the counter cannot repeat a name within a process. True, and beside +//! the point: the collision comes from a *previous* process. A run that crashed +//! mid-session orphans one scratch file per save it made, and the OS reusing that +//! pid restarts the counter at zero -- so two orphans defeat one retry and the +//! save fails outright, for a reason the user cannot act on. It is now a loop +//! bounded at `SCRATCH_ATTEMPTS`; bounded rather than bare, because a directory +//! rejecting creation for a persistent reason would otherwise hang, and a hang is +//! a worse answer than an error. +//! +//! The exhaustion branch is driven through `open_fresh_scratch` rather than +//! through `write_atomic`, and that is not a stylistic choice. Reaching it the +//! other way means predicting the process-global `SCRATCH_SEQ` and planting a +//! decoy at every name the call will pick -- a prediction that **races**, because +//! `cargo test` runs in parallel and every sibling test calling `write_atomic` +//! consumes sequence values. Measured rather than assumed: a serialising mutex +//! over the three tests that *peek* at the counter still failed 2 runs in 5, +//! because the tests doing the consuming are precisely the ones that never look +//! at it. The single-decoy test had been latently flaky since it was written and +//! had simply never lost the race. use std::fs; use std::io; @@ -135,6 +183,14 @@ use std::sync::atomic::{AtomicU64, Ordering}; /// declared mid-function reads as if it were scoped to that point when it is not. static SCRATCH_SEQ: AtomicU64 = AtomicU64::new(0); +/// How many scratch names to try before giving up. +/// +/// One per orphaned scratch file left by a crashed run whose pid the OS later +/// reused. Eight is far past any plausible orphan count and still terminates +/// promptly if the directory is rejecting creation for a persistent reason -- +/// which is the case a bare `loop` would hang on. +const SCRATCH_ATTEMPTS: u32 = 8; + /// How many times to attempt the rename before giving up. /// /// Only ever more than one on Windows (see `is_transient_rename_error`). Five @@ -250,6 +306,70 @@ fn rename_with_retry(from: &Path, to: &Path) -> io::Result<()> { rename_with_retry_using(|| fs::rename(from, to), is_transient_rename_error) } +/// Apply `mode` to `path` through `op`. +/// +/// The indirection exists so a test can reach the failure branch. On Unix +/// `set_permissions` on a file this process just created and owns does not fail +/// for any cause a unit test can arrange, so a hard-wired call would leave the +/// propagation path permanently unexercised -- and an unexercised error path is +/// how the swallowed version survived review in the first place. +#[cfg(unix)] +fn apply_mode_using(path: &Path, mode: u32, op: F) -> io::Result<()> +where + F: Fn(&Path, u32) -> io::Result<()>, +{ + op(path, mode) +} + +/// The real mode application: `chmod` to exactly `mode`. +#[cfg(unix)] +fn set_permissions_mode(path: &Path, mode: u32) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt as _; + fs::set_permissions(path, fs::Permissions::from_mode(mode)) +} + +/// Open a scratch file, advancing past names a crashed run left behind. +/// +/// `open` must pick a **fresh** name each call and return the opened file. The +/// loop retries only `AlreadyExists`; every other error propagates on the first +/// occurrence. +/// +/// Bounded at `SCRATCH_ATTEMPTS`, because a bare `loop` would hang on a directory +/// that rejects creation for some persistent reason -- and a hang is a worse +/// answer than an error. +/// +/// # Why this is a separate function +/// +/// The single retry it replaced was correct for one orphan and wrong for two, and +/// review found that rather than a test, because the only way to exercise the +/// exhaustion branch through `write_atomic` is to predict the process-global +/// `SCRATCH_SEQ` and plant a decoy at every name it will pick. That prediction +/// **races**: `cargo test` runs in parallel and any sibling test calling +/// `write_atomic` consumes sequence values, so the decoys land at names nothing +/// asks for. Measured, not theorised -- a serialising mutex over the three tests +/// that peek at the counter still failed 2 runs in 5, because the tests doing the +/// consuming are the ones that never look at it. +/// +/// Driving the loop directly removes the global from the test entirely. Same +/// reasoning as `rename_with_retry_using` two definitions up. +fn open_fresh_scratch(mut open: F) -> io::Result +where + F: FnMut() -> io::Result, +{ + let mut attempt: u32 = 0; + loop { + match open() { + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => { + attempt += 1; + if attempt >= SCRATCH_ATTEMPTS { + return Err(e); + } + } + other => return other, + } + } +} + /// Write `contents` to `path` atomically and durably. /// /// Creates the parent directory if needed, resolves a symlinked target, writes to @@ -284,7 +404,7 @@ pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { fs::metadata(&target).ok().map(|m| m.permissions().mode()) }; - let mut tmp = scratch_name(&target); + let mut tmp = PathBuf::new(); let write_result = (|| -> io::Result<()> { use std::io::Write as _; @@ -297,17 +417,21 @@ pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { use std::os::unix::fs::OpenOptionsExt as _; opts.mode(mode); } - // Retry once past an occupied scratch name. A crashed run can orphan a - // scratch file, the OS later reuses that pid, and the new run's first save - // picks the same seq. Advancing the counter cannot produce the same name - // again, since it only increases within a process. - let mut f = match opts.open(&tmp) { - Err(e) if e.kind() == io::ErrorKind::AlreadyExists => { - tmp = scratch_name(&target); - opts.open(&tmp)? - } - other => other?, - }; + // Advance past an occupied scratch name, up to `SCRATCH_ATTEMPTS` times. + // A crashed run can orphan scratch files, the OS later reuses that pid, + // and the new run's counter restarts at zero -- so its first save picks a + // name that already exists. Advancing cannot repeat a name within a + // process, since the counter only increases. + // + // This was a single retry, which review correctly showed is not enough: + // a crashed run that orphaned BOTH `.pid.0.tmp` and `.pid.1.tmp` defeats + // it, and the save fails outright for a reason the user cannot act on. A + // bounded loop costs one `open` per orphan and is bounded so a directory + // that rejects creation for some other persistent reason still terminates. + let mut f = open_fresh_scratch(|| { + tmp = scratch_name(&target); + opts.open(&tmp) + })?; f.write_all(contents)?; f.sync_all() })(); @@ -320,10 +444,19 @@ pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { // The exact mode, after creation. `open(2)` masks the requested mode with the // umask, so creation alone can land narrower; this makes it exact. + // + // The error PROPAGATES, and the scratch file is removed first. Swallowing it + // was wrong in the one direction that matters: the mode being restored is the + // mode the file already had, so a target at 0600 whose `set_permissions` + // fails is replaced by one at the umask default -- which is WIDER. Reporting + // success there hands the caller a file with weaker permissions than the one + // it replaced, and says nothing. Found in review of the PR that introduced it. #[cfg(unix)] - if let Some(mode) = existing_mode { - use std::os::unix::fs::PermissionsExt as _; - let _ = fs::set_permissions(&tmp, fs::Permissions::from_mode(mode)); + if let Some(mode) = existing_mode + && let Err(e) = apply_mode_using(&tmp, mode, set_permissions_mode) + { + let _ = fs::remove_file(&tmp); + return Err(e); } if let Err(e) = rename_with_retry(&tmp, &target) { @@ -331,8 +464,7 @@ pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { return Err(e); } - sync_parent_dir(&target); - Ok(()) + sync_parent_dir(&target) } /// A scratch path beside `target`, carrying the pid and a per-call counter. @@ -361,24 +493,51 @@ fn scratch_name(target: &Path) -> PathBuf { /// failure anyone sees, which is the worse of the two. `.` is the directory an /// empty parent means. #[cfg(unix)] -fn sync_parent_dir(target: &Path) { - { - let parent = target.parent().map_or_else( - || PathBuf::from("."), - |p| { - if p.as_os_str().is_empty() { - PathBuf::from(".") - } else { - p.to_path_buf() - } - }, - ); - if let Ok(dir) = fs::File::open(&parent) { - let _ = dir.sync_all(); - } +fn sync_parent_dir(target: &Path) -> io::Result<()> { + let parent = target.parent().map_or_else( + || PathBuf::from("."), + |p| { + if p.as_os_str().is_empty() { + PathBuf::from(".") + } else { + p.to_path_buf() + } + }, + ); + let dir = fs::File::open(&parent)?; + match dir.sync_all() { + Ok(()) => Ok(()), + Err(e) if directory_fsync_is_unsupported(&e) => Ok(()), + Err(e) => Err(e), } } +/// True when a directory `fsync` failure means "this filesystem does not offer +/// the barrier", rather than "the write failed". +/// +/// `fsync` on a directory is a POSIX-*blessed* idiom, not a POSIX-*guaranteed* +/// one. Some filesystems answer `EINVAL`; a few network mounts answer `EBADF` +/// because the descriptor was not opened writable. Neither means the rename +/// failed to commit, so treating them as write failures would break saving +/// outright on those mounts -- on a path that otherwise fully succeeded. +/// +/// Every OTHER error propagates. **That is the change**: previously all of them +/// were discarded, so a genuine `EIO` from the storage layer -- the exact +/// condition the sync exists to detect -- was reported to the caller as success. +/// +/// Extracted rather than written inline as a `matches!` guard for the reason +/// `rename_with_retry_using` was: a decision buried in a call site that cannot be +/// reached from a test is a decision nothing checks. Here the excusable and +/// non-excusable cases are both one function call away. +#[cfg(unix)] +pub fn directory_fsync_is_unsupported(e: &io::Error) -> bool { + // `EBADF` is 9 on every Unix ABI RustyNES builds for. Named rather than + // written inline so the comparison reads as a decision, and spelled out + // rather than pulled from `libc`, which this crate does not depend on. + const EBADF: i32 = 9; + matches!(e.kind(), io::ErrorKind::InvalidInput) || e.raw_os_error() == Some(EBADF) +} + /// No-op off Unix. /// /// Split into a separate `cfg`'d definition rather than an inner `#[cfg]` block, @@ -386,8 +545,22 @@ fn sync_parent_dir(target: &Path) { /// only visible on a non-Unix target, so it failed the **wasm32** gate while /// native clippy passed. `const` states the truth: on Windows `MoveFileEx` /// already orders the metadata write, and on wasm there is no directory to sync. +/// +/// The `unnecessary_wraps` allow is the same story a second time. Once the Unix +/// arm started propagating its `sync_all` failure, this arm had to match its +/// signature — and an always-`Ok` return is exactly what that lint objects to, on +/// non-Unix targets only. Clippy's suggested fix (return `()`) would break the +/// parity the shared call site depends on, since `write_atomic` ends in +/// `sync_parent_dir(&target)` as its tail expression. Caught by the wasm32 gate +/// again, which native clippy cannot reach. #[cfg(not(unix))] -const fn sync_parent_dir(_target: &Path) {} +#[allow( + clippy::unnecessary_wraps, + reason = "signature parity with the Unix arm, which genuinely can fail" +)] +const fn sync_parent_dir(_target: &Path) -> io::Result<()> { + Ok(()) +} #[cfg(test)] mod tests { @@ -571,6 +744,77 @@ mod tests { assert_eq!(fs::read(&p).unwrap(), b"payload"); } + /// TWO occupied scratch names must not cost the save either. + /// + /// The single retry this replaced survived one decoy and failed on two -- + /// which is the real scenario, not a contrived one: a run that crashed + /// mid-session orphans one scratch file per save it had made, and the OS + /// reusing that pid restarts the counter at zero. Found in review. + #[test] + fn two_occupied_scratch_names_do_not_lose_the_write() { + let d = tempdir(); + let p = d.join("f.txt"); + let next = SCRATCH_SEQ.load(Ordering::Relaxed); + for offset in 0..2 { + let mut decoy = p.as_os_str().to_os_string(); + decoy.push(format!(".{}.{}.tmp", std::process::id(), next + offset)); + fs::write(PathBuf::from(decoy), b"decoy").expect("plant decoy"); + } + write_atomic(&p, b"payload").expect("write should survive two collisions"); + assert_eq!(fs::read(&p).unwrap(), b"payload"); + } + + /// ...but the loop is BOUNDED: it gives up rather than spinning. + /// + /// Driven directly rather than through `write_atomic`, because reaching the + /// exhaustion branch that way means predicting the process-global + /// `SCRATCH_SEQ`, and that prediction races with every parallel test that + /// calls `write_atomic`. See `open_fresh_scratch`. + #[test] + fn the_scratch_loop_is_bounded_and_reports_failure() { + let mut calls = 0u32; + let r: io::Result<()> = open_fresh_scratch(|| { + calls += 1; + Err(io::Error::from(io::ErrorKind::AlreadyExists)) + }); + let e = r.expect_err("the scratch loop must give up rather than spin"); + assert_eq!(e.kind(), io::ErrorKind::AlreadyExists); + assert_eq!( + calls, SCRATCH_ATTEMPTS, + "the loop must try exactly SCRATCH_ATTEMPTS names before giving up" + ); + } + + /// The loop must succeed as soon as a name is free, not keep going. + #[test] + fn the_scratch_loop_stops_at_the_first_free_name() { + let mut calls = 0u32; + let got = open_fresh_scratch(|| { + calls += 1; + if calls < 3 { + Err(io::Error::from(io::ErrorKind::AlreadyExists)) + } else { + Ok(calls) + } + }) + .expect("should succeed on the third name"); + assert_eq!(got, 3); + assert_eq!(calls, 3, "the loop kept going past a free name"); + } + + /// Only `AlreadyExists` is retried. Anything else is a real failure and must + /// surface on the first occurrence rather than after eight pointless tries. + #[test] + fn the_scratch_loop_does_not_retry_other_errors() { + let mut calls = 0u32; + let r: io::Result<()> = open_fresh_scratch(|| { + calls += 1; + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + }); + assert_eq!(r.unwrap_err().kind(), io::ErrorKind::PermissionDenied); + assert_eq!(calls, 1, "a non-transient error was retried"); + } + /// A failed write must leave the existing file intact. #[test] fn a_failed_write_leaves_the_original_intact() { @@ -661,4 +905,62 @@ mod tests { assert!(r.is_err()); assert_eq!(calls, 1, "POSIX rename has no transient sharing violation"); } + + /// A failing mode application must PROPAGATE, not be reported as success. + /// + /// The direction matters and is why this was a blocking finding rather than a + /// nitpick: the mode being applied is the mode the target *already had*, so a + /// swallowed failure replaces a 0600 file with one at the umask default -- + /// **wider** than what it replaced -- and tells the caller nothing. + #[cfg(unix)] + #[test] + fn a_failing_mode_application_propagates() { + let dir = tempdir(); + let p = dir.join("m"); + let r = apply_mode_using(&p, 0o600, |_, _| { + Err(io::Error::new(io::ErrorKind::PermissionDenied, "denied")) + }); + assert!(r.is_err(), "a failing mode op reported success"); + assert_eq!(r.unwrap_err().kind(), io::ErrorKind::PermissionDenied); + } + + /// ...and the succeeding case still succeeds, so the test above is not + /// passing merely because the helper always fails. + #[cfg(unix)] + #[test] + fn a_succeeding_mode_application_is_ok() { + let dir = tempdir(); + let p = dir.join("m"); + fs::write(&p, b"x").expect("seed"); + apply_mode_using(&p, 0o600, set_permissions_mode).expect("real chmod failed"); + } + + /// The two excusable directory-fsync errors are excused, and nothing else is. + /// + /// `EIO` is the case that matters: it is the exact condition the parent-dir + /// sync exists to detect, and the previous code discarded it along with + /// everything else. + #[cfg(unix)] + #[test] + fn only_the_unsupported_directory_fsync_errors_are_excused() { + let einval = io::Error::from(io::ErrorKind::InvalidInput); + assert!( + directory_fsync_is_unsupported(&einval), + "EINVAL must be excused -- some filesystems answer it for dir fsync" + ); + let ebadf = io::Error::from_raw_os_error(9); + assert!( + directory_fsync_is_unsupported(&ebadf), + "EBADF must be excused -- some network mounts answer it" + ); + + for (errno, name) in [(5, "EIO"), (28, "ENOSPC"), (13, "EACCES")] { + let e = io::Error::from_raw_os_error(errno); + assert!( + !directory_fsync_is_unsupported(&e), + "{name} must PROPAGATE -- it is a real write failure, not an \ + unsupported barrier" + ); + } + } } From 53e02ac82be6545126563018e97e5d4e4623da23 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 10:27:54 -0400 Subject: [PATCH 09/20] fix(frontend): a failed write deleted a file it did not create Second review round on this PR, and the finding is in the fix from the first one. When the scratch-name loop exhausts, every name it tried was occupied -- that is what exhaustion means. The cleanup below then removed the last one. But that file already existed and belongs to somebody else: an orphan from a crashed run, or a scratch file a colliding instance is actively writing. A save that failed took another process's in-progress data with it. The defect predates the bounded loop -- a single retry could reach it too -- but the loop widened it from one chance to eight, so it arrived with the change that made it likelier. The scratch path is now `Option`, assigned only after a successful exclusive create, and the cleanup runs only when there is something of ours to clean up. `None` means nothing was created, so anything sitting at those names is not ours to delete. THE FIRST TEST FOR THIS DID NOT TEST IT Worth recording, because the test looked right and passed. It forced a failure by calling `write_atomic` on a directory. That does fail -- but at the *rename*, not at the scratch create, and the rename branch is one where the scratch file genuinely is ours. So the test exercised a path the fix does not touch and passed identically against the defect and against the fix. Two mutations reported NOT CAUGHT, which is the only reason this was noticed. The first restored the unconditional delete and the second restored the exact reported shape -- assigning the scratch path before the create rather than after -- and neither moved the suite. Reaching the real branch means every candidate name colliding, and doing that through the real `scratch_name` means predicting the process-global `SCRATCH_SEQ` and planting a decoy at each name it will pick -- the same race documented on `open_fresh_scratch`. So the name source is now injectable: `write_atomic_with` takes the generator, `write_atomic` passes `scratch_name`, and the test passes a closure returning one fixed occupied name. Exhaustion is then deterministic, and the mutation restoring the reported defect is now caught. That is the fifth time this release that the fix was "extract it so a test can reach it", and the first time the lesson arrived through a test that had already been written and believed. ALSO FIXED, FROM THE SAME REVIEW `RENAME_ATTEMPTS`' doc claimed a 310 ms worst case. The loop returns on the fifth failure rather than backing off after it, so there are four sleeps, not five: 10 + 20 + 40 + 80 = 150 ms. 310 would be the figure if a fifth sleep of 160 ms happened, and it does not. Unchanged from the previous round, with reasons already given on the PR: iterative symlink resolution is deferred (needs a cycle bound and its own tests), pushing the cheats.rs save error to its caller is agreed but is UI plumbing that belongs in its own change, and the ignored `remove_file` results in the error paths are deliberate -- a cleanup that fails when the disk is full should not displace the primary error. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 20 module tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- CHANGELOG.md | 16 ++- crates/rustynes-frontend/src/atomic_write.rs | 100 +++++++++++++++++-- 2 files changed, 105 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f140ad5..ddc748aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,7 +61,21 @@ cycle-accurate core later replaced. mid-session orphans one scratch file per save it made, and pid reuse restarts the counter at zero, so two orphans defeat one retry. - Getting the last one under test surfaced something else. Reaching the exhaustion + A second review round then found a **fourth**, in the fix for the third: on + exhaustion the last name tried is one that **already existed** — an orphan, or a + scratch file a colliding instance is actively writing — and the cleanup deleted + it. A failed save took another process's in-progress data with it. The defect + predated the loop, which widened it from one chance to eight; the scratch path + is now `Option`al and assigned only on a successful create. + + **The first test written for that fix did not test it.** It forced a failure by + writing to a directory, which fails at the *rename* — a branch where the scratch + file genuinely is ours — so it passed against the defect and the fix alike. Two + mutations reported NOT CAUGHT, which is the only reason it was noticed; the + scratch-name source is now injectable so the exhaustion branch is reachable + without predicting global state. + + Getting the third one under test surfaced something else. Reaching the exhaustion branch through `write_atomic` means predicting the process-global `SCRATCH_SEQ` and planting a decoy at every name the call will pick — and **that prediction races**, because every parallel test calling `write_atomic` consumes sequence diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs index bca303a7..f18b57ed 100644 --- a/crates/rustynes-frontend/src/atomic_write.rs +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -194,9 +194,11 @@ const SCRATCH_ATTEMPTS: u32 = 8; /// How many times to attempt the rename before giving up. /// /// Only ever more than one on Windows (see `is_transient_rename_error`). Five -/// attempts with the backoff below is a worst case of 310 ms, which is a long -/// time in a UI frame and a short one against losing a save the user believes -/// happened. +/// attempts sleep FOUR times — the loop returns on the fifth failure rather than +/// backing off after it — so the worst case is 10 + 20 + 40 + 80 = **150 ms**, +/// which is a long time in a UI frame and a short one against losing a save the +/// user believes happened. (This read 310 ms until review; that would be the +/// figure if a fifth sleep of 160 ms happened, and it does not.) const RENAME_ATTEMPTS: u32 = 5; /// Base backoff between rename attempts, doubled each time: 10, 20, 40, 80, 160. @@ -386,6 +388,31 @@ where /// `RENAME_ATTEMPTS` transient failures returns the last error rather than /// succeeding quietly. pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { + write_atomic_with(path, contents, scratch_name) +} + +/// `write_atomic`, with the scratch-name source injected. +/// +/// The seam exists for one specific test. The exhaustion branch -- where every +/// candidate name is occupied and nothing of ours is ever created -- is where the +/// cleanup must NOT delete anything, and reaching it through the real +/// `scratch_name` means predicting the process-global `SCRATCH_SEQ` and planting +/// a decoy at every name it will pick. That prediction races every parallel test +/// that calls `write_atomic` (see `open_fresh_scratch`). +/// +/// It was not obvious this seam was needed. The first attempt at the test forced +/// a failure by writing to a directory, which fails at the *rename* -- a branch +/// where the scratch file genuinely is ours -- so it exercised the wrong path +/// entirely and passed against both the fix and the defect. Two mutations +/// reported NOT CAUGHT, which is the only reason that was noticed. +/// +/// # Errors +/// +/// As [`write_atomic`]. +fn write_atomic_with(path: &Path, contents: &[u8], mut scratch: N) -> io::Result<()> +where + N: FnMut(&Path) -> PathBuf, +{ let target = resolve_write_target(path); if let Some(parent) = target.parent() && !parent.as_os_str().is_empty() @@ -404,7 +431,7 @@ pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { fs::metadata(&target).ok().map(|m| m.permissions().mode()) }; - let mut tmp = PathBuf::new(); + let mut tmp: Option = None; let write_result = (|| -> io::Result<()> { use std::io::Write as _; @@ -428,19 +455,36 @@ pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { // it, and the save fails outright for a reason the user cannot act on. A // bounded loop costs one `open` per orphan and is bounded so a directory // that rejects creation for some other persistent reason still terminates. - let mut f = open_fresh_scratch(|| { - tmp = scratch_name(&target); - opts.open(&tmp) + // + // `tmp` is assigned ONLY on a successful create, which is load-bearing + // rather than tidy: on exhaustion the last name tried is a file that + // already existed and belongs to somebody else -- an orphan, or a scratch + // file a colliding instance is actively writing. Assigning it and then + // running the cleanup below would delete that file. Review caught this; + // the bug predated the loop (a single retry could reach it too) and the + // loop widened it from one chance to eight. + let (created, mut f) = open_fresh_scratch(|| { + let candidate = scratch(&target); + opts.open(&candidate).map(|f| (candidate, f)) })?; + tmp = Some(created); f.write_all(contents)?; f.sync_all() })(); if let Err(e) = write_result { - // Best-effort: if the write failed because the disk is full, the remove - // may fail too, and the original file is still intact. - let _ = fs::remove_file(&tmp); + // Only if this process created it. `None` means the scratch open never + // succeeded, so there is nothing of ours to remove and anything at those + // names belongs to someone else. + // + // Best-effort past that: if the write failed because the disk is full, + // the remove may fail too, and the original file is still intact. + if let Some(t) = &tmp { + let _ = fs::remove_file(t); + } return Err(e); } + // Past this point the create succeeded, so the scratch file is ours. + let tmp = tmp.expect("the scratch file exists once the write succeeded"); // The exact mode, after creation. `open(2)` masks the requested mode with the // umask, so creation alone can land narrower; this makes it exact. @@ -815,6 +859,42 @@ mod tests { assert_eq!(calls, 1, "a non-transient error was retried"); } + /// A failed write must not delete a file it did not create. + /// + /// On exhaustion the last name tried is one that ALREADY EXISTED — an orphan + /// from a crashed run, or a scratch file a colliding instance is actively + /// writing. The cleanup used to remove it unconditionally, so a save that + /// failed took another process's in-progress data with it. Found in review; + /// the defect predated the bounded loop, which widened it from one chance to + /// eight. + /// + /// Driven through `write_atomic_with` with a fixed name generator, so every + /// attempt collides and the exhaustion branch is reached deterministically. + /// An earlier version of this test forced failure by writing to a directory + /// and passed against the defect, because that fails at the *rename*, where + /// the scratch file genuinely is ours. + #[test] + fn a_failed_write_does_not_delete_a_file_it_did_not_create() { + const OTHERS: &[u8] = b"someone elses in-progress data"; + + let d = tempdir(); + let p = d.join("f.txt"); + let occupied = d.join("occupied.tmp"); + fs::write(&occupied, OTHERS).expect("plant"); + + let taken = occupied.clone(); + let e = write_atomic_with(&p, b"payload", |_| taken.clone()) + .expect_err("every candidate name was occupied; this must fail"); + assert_eq!(e.kind(), io::ErrorKind::AlreadyExists); + + assert_eq!( + fs::read(&occupied).expect("the other process's file must still exist"), + OTHERS, + "a failed write deleted a file it did not create" + ); + assert!(!p.exists(), "a failed write must not create the target"); + } + /// A failed write must leave the existing file intact. #[test] fn a_failed_write_leaves_the_original_intact() { From 1d301a10cb81a7893b35f6199bbe774698ccf58d Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 10:36:54 -0400 Subject: [PATCH 10/20] fix(frontend): a third excused fsync errno, and the untested fallback a mutation found Third review round: no blocking issues, two suggestions worth taking, and one finding that came out of checking the second rather than out of the review. ENOTSUP / EOPNOTSUPP joins the excused set for directory fsync. The set is a list of ways a filesystem says "there is no barrier available here", and some network filesystems answer io::ErrorKind::Unsupported where others answer EINVAL. Leaving one out fails a save on that mount for a path that otherwise fully succeeded -- the same reasoning that put EINVAL and EBADF there. Parent-directory resolution loses an allocation and reads better: a filter plus unwrap_or_else over a borrowed Path, rather than a map_or_else building a PathBuf on both arms. THE MUTATION FOUND MORE THAN THE REVIEW DID Mutating that second change -- deleting the filter that maps an empty parent to "." -- came back NOT CAUGHT. Nothing in the suite covered it. Path::new("f.txt").parent() is Some(""), not None, and File::open("") fails with ENOENT. The fallback has been documented as load-bearing since it was written, and was never tested. It also matters more now than it did then. While the sync was best-effort, losing the fallback meant a durability step quietly skipped. Now that the sync propagates, losing it means write_atomic FAILS OUTRIGHT for any relative target -- a working call site turned into an error. The property tightened underneath a test that never existed. Tested by calling sync_parent_dir directly rather than through write_atomic, because reaching it that way needs a relative target and therefore a set_current_dir, which is process-global and races the parallel suite. Same trap open_fresh_scratch documents; third time in this PR that the deterministic route was to drive the function rather than the caller. DEFERRED, WITH THE REASONING scratch_name appends about twenty bytes, so a target already near the 255-byte filesystem limit fails with ENAMETOOLONG where a naive fs::write would have succeeded. Real, and a genuine regression in principle. Deferred because no current call site can reach it -- save states, per-game config and cheats all name their files from a SHA-256 hex digest or a ROM-derived stem, none of which approaches the limit -- and because truncating the base to make room introduces a collision risk that needs its own design rather than a one-line guard. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 21 module tests. Both changes mutation-checked; the second one twice, since the first attempt is what exposed the gap. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- CHANGELOG.md | 11 ++++ crates/rustynes-frontend/src/atomic_write.rs | 55 +++++++++++++++----- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddc748aa..f3823e8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,17 @@ cycle-accurate core later replaced. scratch-name source is now injectable so the exhaustion branch is reachable without predicting global state. + A third round found no blocking issues and two worthwhile refinements: `ENOTSUP` + / `EOPNOTSUPP` joins the excused set, since the list is *ways a filesystem says + there is no barrier here* and leaving one out fails a save on that mount; and + the parent-directory resolution loses an allocation. Mutating the second turned + up a **pre-existing untested property** — the fallback that maps a bare relative + filename's `Some("")` parent to `.`, without which `File::open("")` returns + `ENOENT`. Documented as load-bearing since it was written, never tested, and it + matters *more* now: while the sync was best-effort, losing it meant a durability + step quietly skipped, but now that it propagates, losing it makes `write_atomic` + **fail outright** for any relative target. + Getting the third one under test surfaced something else. Reaching the exhaustion branch through `write_atomic` means predicting the process-global `SCRATCH_SEQ` and planting a decoy at every name the call will pick — and **that prediction diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs index f18b57ed..0a905c95 100644 --- a/crates/rustynes-frontend/src/atomic_write.rs +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -538,17 +538,11 @@ fn scratch_name(target: &Path) -> PathBuf { /// empty parent means. #[cfg(unix)] fn sync_parent_dir(target: &Path) -> io::Result<()> { - let parent = target.parent().map_or_else( - || PathBuf::from("."), - |p| { - if p.as_os_str().is_empty() { - PathBuf::from(".") - } else { - p.to_path_buf() - } - }, - ); - let dir = fs::File::open(&parent)?; + let parent = target + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + let dir = fs::File::open(parent)?; match dir.sync_all() { Ok(()) => Ok(()), Err(e) if directory_fsync_is_unsupported(&e) => Ok(()), @@ -579,7 +573,14 @@ pub fn directory_fsync_is_unsupported(e: &io::Error) -> bool { // written inline so the comparison reads as a decision, and spelled out // rather than pulled from `libc`, which this crate does not depend on. const EBADF: i32 = 9; - matches!(e.kind(), io::ErrorKind::InvalidInput) || e.raw_os_error() == Some(EBADF) + // `Unsupported` covers `ENOTSUP` / `EOPNOTSUPP`, which some network + // filesystems return here instead of `EINVAL`. Added on review: the set is a + // list of ways a filesystem says "no barrier available", and leaving one out + // fails a save on that mount for a path that otherwise fully succeeded. + matches!( + e.kind(), + io::ErrorKind::InvalidInput | io::ErrorKind::Unsupported + ) || e.raw_os_error() == Some(EBADF) } /// No-op off Unix. @@ -859,6 +860,30 @@ mod tests { assert_eq!(calls, 1, "a non-transient error was retried"); } + /// A bare relative filename must still get its parent synced. + /// + /// `Path::new("f.txt").parent()` is `Some("")`, not `None`, and + /// `File::open("")` fails with `ENOENT`. The fallback to `.` was already + /// documented as load-bearing, but nothing tested it — a mutation deleting + /// the filter passed the whole suite. + /// + /// It matters MORE now than when it was written. While the sync was + /// best-effort, losing the fallback meant a durability step quietly skipped. + /// Now that it propagates, losing it means `write_atomic` **fails outright** + /// for any relative target, which is a working call site turned into an error. + /// + /// Called directly rather than through `write_atomic`, because reaching it + /// that way needs a relative target and therefore a `set_current_dir` — which + /// is process-global and races the parallel suite, the same trap + /// `open_fresh_scratch` documents. + #[cfg(unix)] + #[test] + fn a_bare_relative_target_syncs_the_current_directory() { + sync_parent_dir(Path::new("f.txt")) + .expect("an empty parent must fall back to `.`, not open \"\""); + sync_parent_dir(Path::new("./f.txt")).expect("an explicit `.` parent must work"); + } + /// A failed write must not delete a file it did not create. /// /// On exhaustion the last name tried is one that ALREADY EXISTED — an orphan @@ -1033,6 +1058,12 @@ mod tests { directory_fsync_is_unsupported(&ebadf), "EBADF must be excused -- some network mounts answer it" ); + let unsupported = io::Error::from(io::ErrorKind::Unsupported); + assert!( + directory_fsync_is_unsupported(&unsupported), + "ENOTSUP/EOPNOTSUPP must be excused -- some network filesystems \ + answer it instead of EINVAL" + ); for (errno, name) in [(5, "EIO"), (28, "ENOSPC"), (13, "EACCES")] { let e = io::Error::from_raw_os_error(errno); From 5c24c4eeb00851e095b27c9d3928834f6150dc49 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 11:11:19 -0400 Subject: [PATCH 11/20] fix(frontend): a contract that stopped being true, and a symlink chain followed one level Fourth review round on this PR. The blocking finding is a consequence of the first round's fix, which is the honest way to describe it. Round 1 made the parent-directory sync propagate instead of being swallowed. That sync is the LAST step, and it necessarily runs after the rename it exists to make durable -- so its failure returns Err from a call in which the target was successfully replaced. The doc said "on failure the existing file is left untouched", which was true when the sync was best-effort and stopped being true the moment it propagated. A caller reading that error would conclude the old file survived; it did not. Rolling the rename back would mean writing the old contents again, turning a durability warning into a second full write that can itself fail. Swallowing it again is the defect round 1 fixed. So the contract is corrected instead: the doc now states which failures happen before the rename and which one happens after, and the post-rename error is wrapped so its message says the data WAS written and names what is actually uncertain -- whether the directory entry survives a power loss. The kind is preserved, so callers matching on io::ErrorKind still see the real cause. Broken symlink chains now resolve to their end. canonicalize cannot help here -- it fails outright when the final target does not exist -- so the chain is walked by hand. Following one level was enough for the dotfiles case that motivated it and wrong in general: link1 -> link2 -> missing replaced link2 with a regular file rather than writing through. Bounded at SYMLINK_DEPTH, because a chain can be a cycle and read_link succeeds forever on one; on exhaustion the last resolved path is returned rather than an error, since picking a write target is this function's whole job and a pathological chain should not fail a save. Raised in three consecutive rounds before being fixed, which is long enough. The redundant create_dir_all in save_state is removed. write_atomic creates the parent itself, and doing it twice meant a failure surfaced with one function's path context or the other's depending on which won the race. A TEST THAT TESTED THE HELPER, NOT THE CALLER The first test for the post-rename wrapper called post_rename_sync_error directly. That asserts the helper behaves and says nothing about whether the call site uses it -- a mutation deleting the map_err came back NOT CAUGHT. The same shape as the scratch-cleanup test earlier in this PR, arriving from a different direction: testing a helper is not testing the code that was supposed to call it. Fixed by injecting the parent sync alongside the scratch-name generator that was already injected, so a test can force a post-rename failure and assert the whole contract at once -- Err returned, message says written, and the target holds the NEW bytes. The mutation is now caught. A MUTATION HARNESS THAT POISONED ITS OWN BASELINE Recorded because the result was confident and wrong. An earlier harness run was killed by a foreground timeout mid-mutation. Python buffers stdout when not a tty, so every line it had printed was lost and it looked as though it had done nothing. It had: the third mutant was still on disk. The next run read that file as its baseline, so the bounded symlink loop was silently `loop {` in the BASELINE, stayed there after the run asserted "RESTORED clean" -- true, and restored to the mutation -- and a cycle test then spun forever against it, with the timeout attributed to the FIRST mutation. A specific, plausible, wrong "caught (hang)". Only a grep for the constant afterwards showed the bound was gone. The harness now writes a guard file before mutating and refuses to start if one exists, flushes every print, and warms the build against the real baseline first so a cold compile cannot be misread as a hang. DEFERRED, WITH REASONS The ENAMETOOLONG case stands from the previous round: scratch_name appends about twenty bytes, so a target near the 255-byte limit fails where a naive fs::write would succeed. No current call site can reach it -- save states, per-game config and cheats all name files from a SHA-256 digest or a ROM-derived stem -- and truncating the base to make room introduces a collision risk that needs its own design rather than a one-line guard. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 25 module tests. Three mutations, all caught, against a verified-clean baseline. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- crates/rustynes-frontend/src/atomic_write.rs | 218 +++++++++++++++++-- crates/rustynes-frontend/src/save_state.rs | 8 +- 2 files changed, 207 insertions(+), 19 deletions(-) diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs index 0a905c95..1f9a9f3b 100644 --- a/crates/rustynes-frontend/src/atomic_write.rs +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -233,16 +233,41 @@ pub fn resolve_write_target(path: &Path) -> PathBuf { if let Ok(real) = fs::canonicalize(path) { return real; } - match fs::read_link(path) { - Ok(dest) if dest.is_absolute() => dest, - Ok(dest) => match path.parent() { - Some(dir) => dir.join(dest), - None => dest, - }, - Err(_) => path.to_path_buf(), + // A BROKEN chain, followed by hand: `canonicalize` fails outright when the + // final target does not exist, so it cannot resolve `link1 -> link2 -> + // missing`. Following one level was enough for the dotfiles case that + // motivated this and wrong for the general one -- it would replace `link2` + // with a regular file rather than writing through to where the chain points. + // + // Bounded at `SYMLINK_DEPTH`, because a chain can be a CYCLE (`a -> b -> a`) + // and `read_link` succeeds forever on one. On exhaustion the last resolved + // path is returned rather than an error: this function's whole job is to pick + // a write target, and a pathological chain should degrade to "write where you + // were told", not fail a save. + let mut cur = path.to_path_buf(); + for _ in 0..SYMLINK_DEPTH { + let Ok(dest) = fs::read_link(&cur) else { + break; + }; + cur = if dest.is_absolute() { + dest + } else { + match cur.parent() { + Some(dir) => dir.join(dest), + None => dest, + } + }; } + cur } +/// How many broken-symlink hops to follow before giving up. +/// +/// Bounded because a chain can be a cycle, on which `read_link` succeeds +/// indefinitely. Linux's own limit is 40; eight is far past any real dotfiles +/// arrangement and keeps a pathological path cheap. +const SYMLINK_DEPTH: usize = 8; + /// Is this rename failure one a retry could plausibly clear? /// /// On Windows, `MoveFileEx` fails with a sharing violation when another process @@ -379,19 +404,39 @@ where /// file's mode across on Unix, renames over the target, and syncs the parent /// directory. /// -/// On failure the scratch file is removed and the **existing file is left -/// untouched** — a stale-but-valid file is the one worth keeping. +/// On failure **before the rename** the scratch file is removed and the existing +/// file is left untouched — a stale-but-valid file is the one worth keeping. +/// +/// # One failure happens AFTER the target is replaced +/// +/// The parent-directory sync is the last step, and it necessarily runs *after* +/// the rename it exists to make durable. So a `sync_parent_dir` failure returns +/// `Err` from a call in which **the target was successfully replaced** — the new +/// contents are on disk and were `fsync`ed before the rename; what is uncertain +/// is whether the directory entry survives a power loss. +/// +/// This is stated rather than hidden because the two obvious alternatives are +/// both worse. Swallowing it is the defect review found in the first version of +/// this module: a genuine `EIO` from the storage layer reported as success. +/// Rolling the rename back would mean writing the old contents again, turning a +/// durability warning into a second full write that can itself fail. +/// +/// Callers that distinguish "not written" from "written, durability unconfirmed" +/// should treat an error from this function as the latter only when the target's +/// mtime has advanced; none in this repository need to, since all of them +/// surface the error to the user rather than acting on it. /// /// # Errors /// /// Returns the underlying [`io::Error`] from any step. A rename that fails after /// `RENAME_ATTEMPTS` transient failures returns the last error rather than -/// succeeding quietly. +/// succeeding quietly. A post-rename sync failure is wrapped so its message says +/// the data was written; see [`post_rename_sync_error`]. pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { - write_atomic_with(path, contents, scratch_name) + write_atomic_with(path, contents, scratch_name, sync_parent_dir) } -/// `write_atomic`, with the scratch-name source injected. +/// `write_atomic`, with the scratch-name source and the parent sync injected. /// /// The seam exists for one specific test. The exhaustion branch -- where every /// candidate name is occupied and nothing of ours is ever created -- is where the @@ -400,7 +445,13 @@ pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { /// a decoy at every name it will pick. That prediction races every parallel test /// that calls `write_atomic` (see `open_fresh_scratch`). /// -/// It was not obvious this seam was needed. The first attempt at the test forced +/// The parent sync is injected for the same reason and a different failure: it +/// runs AFTER the rename, so its error is returned from a call in which the +/// target was already replaced. Asserting that contract needs a sync that fails +/// on demand, and the real one only fails on a directory that is writable but not +/// readable -- not something a unit test should be arranging. +/// +/// It was not obvious the first seam was needed. The first attempt at the test forced /// a failure by writing to a directory, which fails at the *rename* -- a branch /// where the scratch file genuinely is ours -- so it exercised the wrong path /// entirely and passed against both the fix and the defect. Two mutations @@ -409,9 +460,15 @@ pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { /// # Errors /// /// As [`write_atomic`]. -fn write_atomic_with(path: &Path, contents: &[u8], mut scratch: N) -> io::Result<()> +fn write_atomic_with( + path: &Path, + contents: &[u8], + mut scratch: N, + sync_parent: S, +) -> io::Result<()> where N: FnMut(&Path) -> PathBuf, + S: Fn(&Path) -> io::Result<()>, { let target = resolve_write_target(path); if let Some(parent) = target.parent() @@ -508,7 +565,24 @@ where return Err(e); } - sync_parent_dir(&target) + sync_parent(&target).map_err(|e| post_rename_sync_error(&e)) +} + +/// Label a parent-sync failure as post-rename, so the message cannot be read as +/// "nothing was written". +/// +/// The kind is preserved so callers matching on [`io::ErrorKind`] still see the +/// real cause; only the message gains the qualifier. Separated from the call site +/// so a test can assert the wording without needing a directory that is writable +/// but not readable. +fn post_rename_sync_error(e: &io::Error) -> io::Error { + io::Error::new( + e.kind(), + format!( + "the file was written and renamed into place, but its directory entry \ + could not be synced, so the rename may not survive a power loss: {e}" + ), + ) } /// A scratch path beside `target`, carrying the pid and a per-call counter. @@ -884,6 +958,118 @@ mod tests { sync_parent_dir(Path::new("./f.txt")).expect("an explicit `.` parent must work"); } + /// A post-rename sync failure must not read as "nothing was written". + /// + /// The parent-directory sync is the last step and necessarily runs *after* + /// the rename it exists to make durable, so its failure returns `Err` from a + /// call in which the target WAS replaced. That contradicts the plain reading + /// of "on failure the existing file is left untouched", which is why the + /// message now says what actually happened. Found in review. + #[test] + fn a_post_rename_sync_failure_says_the_data_was_written() { + let e = post_rename_sync_error(&io::Error::from_raw_os_error(5)); + let msg = e.to_string(); + assert!( + msg.contains("was written"), + "the message must not imply the write was skipped: {msg}" + ); + assert!( + msg.contains("power loss"), + "the message must name what is actually uncertain: {msg}" + ); + assert_eq!( + e.kind(), + io::Error::from_raw_os_error(5).kind(), + "the underlying kind must survive, or callers matching on it break" + ); + } + + /// `write_atomic` must APPLY the post-rename label, not merely define it. + /// + /// The first test for this called `post_rename_sync_error` directly, which + /// asserts the helper behaves and says nothing about whether the call site + /// uses it — a mutation deleting the `map_err` came back NOT CAUGHT. Same + /// lesson as the scratch-cleanup test two definitions up, in a new shape: + /// testing a helper is not testing the code that was supposed to call it. + /// + /// Also pins the contract the label exists to describe: on a post-rename sync + /// failure the call returns `Err` **and the target holds the new contents**. + #[test] + fn a_post_rename_sync_failure_still_leaves_the_new_contents_in_place() { + let d = tempdir(); + let p = d.join("f.txt"); + fs::write(&p, b"old").expect("seed"); + + let e = write_atomic_with(&p, b"new", scratch_name, |_| { + Err(io::Error::from_raw_os_error(5)) // EIO + }) + .expect_err("a failing parent sync must not report success"); + + assert!( + e.to_string().contains("was written"), + "write_atomic did not apply the post-rename label: {e}" + ); + assert_eq!( + fs::read(&p).expect("target must exist"), + b"new", + "the rename had already happened; the target must hold the NEW bytes" + ); + } + + /// A broken symlink CHAIN must resolve to its end, not to the middle. + /// + /// `canonicalize` cannot help here — it fails outright when the final target + /// does not exist — so the chain is followed by hand. Following one level + /// replaced `link2` with a regular file instead of writing through to where + /// the chain points. Raised in review three times before it was fixed. + #[cfg(unix)] + #[test] + fn a_broken_symlink_chain_resolves_to_its_end() { + use std::os::unix::fs::symlink; + let d = tempdir(); + let missing = d.join("final.txt"); + let link2 = d.join("link2"); + let link1 = d.join("link1"); + symlink(&missing, &link2).expect("link2 -> final"); + symlink(&link2, &link1).expect("link1 -> link2"); + + write_atomic(&link1, b"payload").expect("write through the chain"); + + assert_eq!( + fs::read(&missing).expect("the chain's end must hold the payload"), + b"payload" + ); + assert!( + fs::symlink_metadata(&link2) + .expect("link2 must survive") + .is_symlink(), + "the intermediate link was replaced by a regular file" + ); + } + + /// A symlink CYCLE must terminate rather than spin. + /// + /// `read_link` succeeds forever on `a -> b -> a`, so the resolver is bounded. + /// It degrades to a write target rather than an error: picking where to write + /// is this function's whole job, and a pathological chain should not fail a + /// save. + #[cfg(unix)] + #[test] + fn a_symlink_cycle_terminates() { + use std::os::unix::fs::symlink; + let d = tempdir(); + let a = d.join("a"); + let b = d.join("b"); + symlink(&b, &a).expect("a -> b"); + symlink(&a, &b).expect("b -> a"); + // The assertion is that this RETURNS at all. + let resolved = resolve_write_target(&a); + assert!( + resolved == a || resolved == b, + "a cycle must resolve to one of its own links, got {resolved:?}" + ); + } + /// A failed write must not delete a file it did not create. /// /// On exhaustion the last name tried is one that ALREADY EXISTED — an orphan @@ -908,7 +1094,7 @@ mod tests { fs::write(&occupied, OTHERS).expect("plant"); let taken = occupied.clone(); - let e = write_atomic_with(&p, b"payload", |_| taken.clone()) + let e = write_atomic_with(&p, b"payload", |_| taken.clone(), sync_parent_dir) .expect_err("every candidate name was occupied; this must fail"); assert_eq!(e.kind(), io::ErrorKind::AlreadyExists); diff --git a/crates/rustynes-frontend/src/save_state.rs b/crates/rustynes-frontend/src/save_state.rs index 36cff2d8..81683183 100644 --- a/crates/rustynes-frontend/src/save_state.rs +++ b/crates/rustynes-frontend/src/save_state.rs @@ -92,9 +92,11 @@ pub fn save_to_slot( state: &[u8], ) -> Result { let path = slot_path(data_dir, rom_sha256, slot)?; - if let Some(parent) = path.parent() { - fs::create_dir_all(parent).map_err(|e| map_io(parent, e))?; - } + // No `create_dir_all` here: `write_atomic` creates the parent itself, and + // doing it twice meant a failure surfaced with this function's path context + // in one case and the helper's in the other. Removed on review, matching + // `per_game.rs`. + // Atomic + durable, via the shared helper (v2.4.0 item C). // // This was `fs::write`, which truncates and then writes. An interruption in From 2b02b75edb12ccc976cddb64769d33c8afb172f6 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 11:12:02 -0400 Subject: [PATCH 12/20] ci(agy): the reviewer appends its rounds instead of destroying them The Antigravity reviewer posted a fresh comment each round and DELETED the previous one. That kept the PR tidy and destroyed the record: a round nobody had read before the next push was gone, with nothing on the PR indicating it had ever existed. Unlike a CodeRabbit or Copilot review thread, an unaddressed finding left no trace at all -- so a clean comment list was not evidence that nothing had been raised. Observed on this very PR. Round 1 posted at 13:37:57Z and round 2 at 14:21:35Z; afterwards the issue-comments endpoint returned exactly ONE bot comment, with created_at equal to updated_at equal to 14:21:35Z. The first was gone -- not edited, since the timestamps would differ, and not appended to. Both of those rounds raised a blocking issue and both were correct, one of them a data-loss defect, so the cost of losing a round is not hypothetical. There is now ONE comment per PR, edited in place: the newest round on top, every earlier round folded into a collapsed
block beneath it. Same tidiness, nothing destroyed. The script issues no DELETE at all any more, and the selftest asserts the absence of one so the behaviour cannot return unnoticed. The archive is bounded by MAX_BODY_BYTES (60000, under GitHub's 65536 hard limit) because a PR with many pushes would otherwise grow it until an EDIT starts failing -- stranding the comment at whatever round last fit, which is the worst possible failure since the newest review is the one that cannot be posted. Oldest rounds drop first, and the drop is ANNOUNCED in the body: a silent truncation would look exactly like a PR that had only ever been reviewed once, which is the confusion this whole change exists to remove. Every failure path falls back to a plain post of the new review. A duplicate comment is noise; failing to publish a review is not. THE FORMAT LIVES IN ITS OWN FILE, AND THAT IS THE POINT scripts/_agy_comment_body.sh holds the sentinels and the split/trim helpers, and is sourced by both the reviewer and the selftest. agy-review.sh does its work at top level and so cannot be sourced, which is exactly how a test ends up reimplementing its subject -- and that happened here. The first version of these checks inlined its own copy of the awk pipeline, so a mutation deleting the marker strip from the script came back NOT CAUGHT. A test that reimplements what it tests agrees with itself forever. The fixture changed for the same reason. It had our own bot's comment first, so `first` selected it whether or not the author filter was present -- the security control that stops any user from putting the marker in a comment and having the bot edit it was untestable. A User comment carrying the marker now sorts ahead of ours, and deleting the filter fails. Eight mutations, all caught: the author filter, empty-versus-null, oldest-versus- newest selection, a reintroduced DELETE, the marker strip, the archive split's sentinel ordering, dropping the newest round instead of the oldest, and a drop that succeeds on an empty archive (which would spin the trim loop). Verified end to end by simulating four rounds through the real functions: all four findings present in the final body, newest first, marker appearing exactly once. INSTALLER AND WORKFLOW _agy_comment_body.sh is REQUIRED, not optional -- agy-review.sh sources it at startup, so an install without it fails at runtime rather than degrading. install-into-repo.sh now copies it and the selftest, the workflow chmods it, and both temporaries the archive path creates are pre-declared so the cleanup trap frees them on every exit including the early one after a successful edit. THIS DOES NOT TAKE EFFECT UNTIL IT MERGES The workflow checks out the DEFAULT BRANCH to run the scripts, so a change to agy-review.sh has no effect on any PR -- not even the PR that makes it. The README's default-branch rule covered the workflow and comment triggers; it now covers the scripts too, since that is the surprising half. Synced byte-identical to the canonical template at Local_Only-Projects/antigravity-pr-review/, including the timeout-minutes bound this repo had added locally. The four sibling repos keep the old behaviour until install-into-repo.sh is re-run against them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- .github/workflows/antigravity-review.yml | 4 +- scripts/_agy_comment_body.sh | 61 ++++++++ scripts/agy-review-selftest.sh | 120 +++++++++++---- scripts/agy-review.sh | 181 ++++++++++++++--------- 4 files changed, 266 insertions(+), 100 deletions(-) create mode 100755 scripts/_agy_comment_body.sh diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index 34e7f8a6..ca46d821 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -95,5 +95,7 @@ jobs: # MAX_PROMPT_BYTES: "125000" # inline/file threshold + hard backstop on the argv prompt # STYLE_GUIDE: .github/agy-review.md # style guide, loaded if present run: | - chmod +x scripts/agy-review.sh scripts/_agy_print.sh + # `_agy_comment_body.sh` is sourced by the reviewer at startup, so a + # missing or non-executable copy fails the run rather than degrading it. + chmod +x scripts/agy-review.sh scripts/_agy_print.sh scripts/_agy_comment_body.sh scripts/agy-review.sh diff --git a/scripts/_agy_comment_body.sh b/scripts/_agy_comment_body.sh new file mode 100755 index 00000000..4997864f --- /dev/null +++ b/scripts/_agy_comment_body.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# +# _agy_comment_body.sh -- the review comment's body format, as sourceable functions. +# +# Sourced by `agy-review.sh` (which uses them) and by `agy-review-selftest.sh` (which tests +# them). It exists as a separate file for one reason: `agy-review.sh` does its work at TOP +# LEVEL, so it cannot be sourced without running a review, and a test that cannot call the +# real implementation ends up reimplementing it. That failure is not hypothetical here -- the +# first version of the archive test inlined its own copy of the `awk` pipeline, so a mutation +# deleting the marker strip from the script came back NOT CAUGHT. A test that reimplements its +# subject agrees with itself forever. +# +# Defines no state and runs nothing on source. + +# The sentinels delimiting the archive of earlier review rounds inside the comment body. +# +# The reviewer used to POST a fresh comment each round and DELETE the previous one. That kept +# the PR tidy and destroyed the record: a round nobody read before the next push was gone, with +# nothing on the PR indicating it had existed -- and unlike a CodeRabbit or Copilot thread, an +# unaddressed finding left no trace. Observed on a real PR where two consecutive rounds each +# raised a blocking issue and only the second survived. +# +# Now there is ONE comment per PR, edited in place: newest round on top, earlier rounds folded +# into a collapsed `
` below. Same tidiness, no destruction. +AGY_ARCHIVE_START='' +AGY_ARCHIVE_END='' + +# The newest round of a comment body: everything after the marker, before the archive. +# +# `awk` matching WHOLE LINES rather than `sed` with a pattern, because a review body +# legitimately contains regex metacharacters, backslashes and HTML, and the sentinels must not +# match a line that merely mentions one. +agy_body_head() { + local marker="$1" + awk -v s="$AGY_ARCHIVE_START" '$0 == s { exit } { print }' \ + | grep -v -F -x "$marker" || true +} + +# The archived rounds of a comment body: everything strictly between the sentinels. +# +# The `$0 == e` test comes FIRST so a body whose archive is empty yields nothing rather than +# emitting its own end sentinel. +agy_body_archive() { + awk -v s="$AGY_ARCHIVE_START" -v e="$AGY_ARCHIVE_END" ' + $0 == e { inside = 0 } + inside { print } + $0 == s { inside = 1 }' || true +} + +# Drop the OLDEST archived round -- the last `
` block. Exits non-zero when there is +# none left to drop, so a caller trimming to a size limit terminates rather than spinning. +agy_drop_oldest_round() { + awk ' + /^
$/ { starts[++n] = NR } + { line[NR] = $0 } + END { + cut = (n > 0) ? starts[n] : 0 + if (cut == 0) exit 1 + for (i = 1; i < cut; i++) print line[i] + }' +} diff --git a/scripts/agy-review-selftest.sh b/scripts/agy-review-selftest.sh index db684954..0a4153a6 100644 --- a/scripts/agy-review-selftest.sh +++ b/scripts/agy-review-selftest.sh @@ -31,15 +31,33 @@ extract_marker() { sed -n 's/^MARKER="\(.*\)"$/\1/p' "$SCRIPT_DIR/agy-review.sh" | head -n 1 } extract_filter() { - sed -n "/^SELECT_STALE_JQ='/,/'\$/p" "$SCRIPT_DIR/agy-review.sh" \ - | sed "1s/^SELECT_STALE_JQ='//; \$s/'\$//" + sed -n "/^SELECT_OURS_JQ='/,/'\$/p" "$SCRIPT_DIR/agy-review.sh" \ + | sed "1s/^SELECT_OURS_JQ='//; \$s/'\$//" } MARKER="$(extract_marker)" FILTER="$(extract_filter)" [ -n "$MARKER" ] || { echo "FAIL: could not extract MARKER from agy-review.sh" >&2; exit 1; } -[ -n "$FILTER" ] || { echo "FAIL: could not extract SELECT_STALE_JQ from agy-review.sh" >&2; exit 1; } +[ -n "$FILTER" ] || { echo "FAIL: could not extract SELECT_OURS_JQ from agy-review.sh" >&2; exit 1; } + +# The REAL body-format implementation, sourced rather than reimplemented. The first version of +# these checks inlined its own copy of the `awk` pipeline, and a mutation deleting the marker +# strip from the script came back NOT CAUGHT -- a test that reimplements its subject agrees with +# itself forever. `agy-review.sh` cannot be sourced (it works at top level), which is exactly +# why the format lives in its own file. +# shellcheck source=scripts/_agy_comment_body.sh +. "$SCRIPT_DIR/_agy_comment_body.sh" +[ -n "${AGY_ARCHIVE_START:-}" ] || { echo "FAIL: _agy_comment_body.sh defined no AGY_ARCHIVE_START" >&2; exit 1; } +[ -n "${AGY_ARCHIVE_END:-}" ] || { echo "FAIL: _agy_comment_body.sh defined no AGY_ARCHIVE_END" >&2; exit 1; } + +# The script must actually USE the shared helper, or these checks test a file nothing runs. +grep -q '_agy_comment_body.sh' "$SCRIPT_DIR/agy-review.sh" \ + || { echo "FAIL: agy-review.sh does not source _agy_comment_body.sh" >&2; exit 1; } +for fn in agy_body_head agy_body_archive agy_drop_oldest_round; do + grep -q "$fn" "$SCRIPT_DIR/agy-review.sh" \ + || { echo "FAIL: agy-review.sh does not call $fn" >&2; exit 1; } +done # A non-empty extraction is not the same as a COMPLETE one. The `sed` range above ends at the # first line closing with a quote, so a filter whose body ever ends a line that way would be @@ -51,14 +69,14 @@ FILTER="$(extract_filter)" # prefix of one. # The named args must be supplied here too: the filter references `$marker`/`$new_id`, and jq # rejects an undefined variable at COMPILE time — so omitting them fails a perfectly good program. -if ! printf '[]' | jq --arg marker x --argjson new_id 0 "$FILTER" >/dev/null 2>&1; then - echo "FAIL: extracted SELECT_STALE_JQ is not a valid jq program (truncated?):" >&2 +if ! printf '[]' | jq --arg marker x "$FILTER" >/dev/null 2>&1; then + echo "FAIL: extracted SELECT_OURS_JQ is not a valid jq program (truncated?):" >&2 printf '%s\n' "$FILTER" >&2 exit 1 fi case "$(printf '%s' "$FILTER" | tr -d '[:space:]')" in - *'|.id') : ;; - *) echo "FAIL: extracted SELECT_STALE_JQ does not end in '| .id'; extraction truncated" >&2 + *'|.id//empty') : ;; + *) echo "FAIL: extracted SELECT_OURS_JQ does not end in '| .id // empty'; extraction truncated" >&2 printf '%s\n' "$FILTER" >&2 exit 1 ;; esac @@ -66,20 +84,19 @@ esac fixture() { cat <\nOLDER ROUND\n
')" "$archive_part" + +# A body with NO archive yet (the first round) must split to a head and an empty archive, not to +# an empty head -- the first re-review is exactly when this path runs for the first time. +first_body="$(printf '%s\nFIRST ROUND\n' "$MARKER")" +check "a body with no archive still yields its head" "FIRST ROUND" \ + "$(printf '%s\n' "$first_body" | agy_body_head "$MARKER")" +check "a body with no archive yields an empty archive" "" \ + "$(printf '%s\n' "$first_body" | agy_body_archive)" + +# The head must NOT carry the marker forward: an archived round that still contains it would +# make every future run's `contains($marker)` match inside the archive, and the split would +# then cut at the wrong place. A mutation removing the strip must fail here. +check "the head never carries the marker into the archive" "" \ + "$(printf '%s\nX\n' "$MARKER" | agy_body_head "$MARKER" | grep -F -x "$MARKER" || true)" + +# Trimming must terminate: with no `
` left, dropping fails rather than looping. +check "dropping from an empty archive fails rather than spinning" "1" \ + "$(printf 'no rounds here\n' | agy_drop_oldest_round >/dev/null 2>&1; echo $?)" +check "dropping removes the OLDEST round, keeping the newest" \ + "$(printf '
\nNEW\n
')" \ + "$(printf '
\nNEW\n
\n
\nOLD\n
\n' | agy_drop_oldest_round)" + +# The script must not delete comments any more. A reintroduced DELETE is the regression that +# would silently restore the destructive behaviour this design replaced. +if grep -qE 'gh api +-X +DELETE' "$SCRIPT_DIR/agy-review.sh"; then + echo " FAIL agy-review.sh deletes comments again; the archive design forbids it" + fails=$((fails + 1)) +else + echo " ok agy-review.sh never deletes a comment" +fi # Regression #2, pinned: `--arg`/`--argjson` belong to jq. If they are ever moved onto `gh api` # again, that command exits non-zero — assert the flags are not passed to `gh api` in the script. diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 104517b5..03299072 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -100,21 +100,31 @@ AGY_RETRIES="${AGY_RETRIES:-3}" # attempts to get a usable agy respon AGY_RETRY_DELAY="${AGY_RETRY_DELAY:-15}" # base backoff seconds between retries (grows per attempt) MARKER="" -# The jq program that picks which prior review comments to delete. Named, and exercised directly -# by `scripts/agy-review-selftest.sh`, because this filter has now been wrong TWICE in ways -# nothing observed: first the just-posted comment was not excluded (so it deleted itself), then -# jq's `--arg` was handed to `gh api`, which has no such flag (so the whole step died silently and -# stale comments accumulated). Both were invisible from the outside — the review still posted. +# The comment body's format -- sentinels plus the split/trim helpers -- lives in a sourceable +# file so `agy-review-selftest.sh` can test the REAL implementation rather than a copy of it. +# This script does its work at top level and so cannot itself be sourced. +# shellcheck source=scripts/_agy_comment_body.sh +. "$(dirname -- "${BASH_SOURCE[0]}")/_agy_comment_body.sh" + +MAX_BODY_BYTES="${MAX_BODY_BYTES:-60000}" + +# The jq program that finds THIS bot's existing review comment on the PR, so it can be edited +# rather than replaced. Named, and exercised directly by `scripts/agy-review-selftest.sh`, +# because its predecessor (which selected comments to DELETE) was wrong twice in ways nothing +# observed: first the just-posted comment was not excluded, so a run deleted its own review; +# then jq's `--arg` was handed to `gh api`, which has no such flag, so the step died silently. +# Both were invisible from the outside — the review still posted. # -# The two `select`s that matter: the AUTHOR filter (without it, any user could put the marker in a -# comment and have this bot delete arbitrary comments) and the ID exclusion (without it, the run -# deletes the comment it just published). -SELECT_STALE_JQ='.[] +# The AUTHOR filter is load-bearing, not cosmetic: without it, any user could put the marker +# (an HTML comment, invisible when rendered) in a PR comment and have this bot edit it. Only +# ever touch our own bot's comments. `first` picks the OLDEST match, so if duplicates exist +# from an older version of this script, the canonical thread is the one that keeps growing. +SELECT_OURS_JQ='[ .[] | select(.user.type == "Bot" and .user.login == "github-actions[bot]") - | select(.body | contains($marker)) - | select(.id != $new_id) - | .id' -readonly SELECT_STALE_JQ + | select(.body | contains($marker)) ] + | first + | .id // empty' +readonly SELECT_OURS_JQ REPO="${GITHUB_REPOSITORY:?GITHUB_REPOSITORY not set}" @@ -152,13 +162,17 @@ log "reviewing ${REPO}#${PR}" # Remove every temp file on exit. Pre-declared so the trap is safe under `set -u` even if the # script exits before a given file is created. diff_file= diff_err= meta_file= prompt_file= out_file= raw= body_file= agy_diff_file= agy_work_dir= +# The archive path's two temporaries. Declared here rather than left to their assignment, +# so the trap frees them on every exit -- including the `exit 0` after a successful edit. +prior_body_file= archived_file= # Set to 1 if agy printed its interactive OAuth login flow instead of a review (lapsed session); # gates the no-post abort below. Pre-declared so `${auth_failed:-0}` is set-u-safe on every path. auth_failed=0 # Set when the large-diff fallback below creates refs/agy/* so the trap can remove them. agy_refs_created= cleanup() { - rm -f "$diff_file" "$diff_err" "$meta_file" "$prompt_file" "$out_file" "$raw" "$body_file" "$agy_diff_file" + rm -f "$diff_file" "$diff_err" "$meta_file" "$prompt_file" "$out_file" "$raw" "$body_file" \ + "$agy_diff_file" "$prior_body_file" "$archived_file" # Remove the gitignored diff-handoff scratch dir once its file is gone. `rmdir` only unlinks an # empty dir, so a concurrent run's file (a different $$) is never clobbered; a non-empty dir is # gitignored and harmless if left behind. @@ -600,65 +614,90 @@ if oauth_url_present "$body_file"; then exit 1 fi -# --- post fresh, THEN replace any prior review comment -------------------------- -# Publish-before-delete, deliberately: if this ordering were reversed and posting failed -# afterward (a transient gh/API error), the PR would be left with NO review comment at all -# instead of the still-valid prior one. Posting first means a failure here can only ever -# leave a harmless duplicate, never a silent loss of the last review. -# The posted comment's id comes from the POST itself, not from a read-back. `gh pr comment` -# prints the new comment's URL, whose trailing `#issuecomment-` is authoritative the instant -# it returns. Re-querying the comment list to find "the newest one with our marker" raced with -# GitHub's own read replication: right after posting, the list can still omit it, and then the -# exclusion below matched nothing and the script deleted the comment it had just published -- -# turning publish-before-delete into publish-then-destroy, the exact failure the ordering exists -# to prevent. +# --- edit our existing comment, appending the previous round to its archive ------ +# One comment per PR, edited in place: newest round on top, earlier rounds folded into a +# collapsed `
` below. NOTHING IS DELETED. The previous design posted fresh and +# deleted the prior comment, which kept the PR tidy at the cost of destroying any round +# nobody had read yet — and left no evidence a round had happened at all. +# +# Fail-closed in the direction that matters: every step below falls back to a plain POST of +# the new review. A duplicate comment is noise; failing to publish a review, or losing one, is +# not. The lookup happens BEFORE the post so a PATCH is possible at all, but a failed lookup +# costs only the archive, never the review. +prior_id="" +prior_body_file="$(mktemp)" +if prior_json="$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate 2>/dev/null)"; then + prior_id="$(printf '%s' "$prior_json" | jq -r --arg marker "$MARKER" "$SELECT_OURS_JQ" 2>/dev/null || true)" + if [ -n "$prior_id" ] && [ "$prior_id" != "null" ]; then + printf '%s' "$prior_json" \ + | jq -r --argjson id "$prior_id" '.[] | select(.id == $id) | .body' > "$prior_body_file" 2>/dev/null \ + || : > "$prior_body_file" + else + prior_id="" + fi +else + log "warning: could not list PR comments; posting a fresh review without the archive" +fi + +if [ -n "$prior_id" ] && [ -s "$prior_body_file" ]; then + # Split the prior body into its newest round (everything after the marker, before the + # archive) and the archive's existing inner rounds. `awk` rather than `sed`, because the + # sentinels must match whole lines and a review body legitimately contains regex + # metacharacters, backslashes and HTML. + prior_head="$(agy_body_head "$MARKER" < "$prior_body_file")" + prior_archive="$(agy_body_archive < "$prior_body_file")" + + archived_file="$(mktemp)" + { + printf '
\nRound reviewed at %s\n\n' \ + "$(date -u +'%Y-%m-%d %H:%M UTC')" + printf '%s\n' "$prior_head" + printf '\n
\n' + printf '%s\n' "$prior_archive" + } > "$archived_file" + + # Drop the oldest rounds until the whole comment fits, and SAY SO. A silent truncation + # here would look identical to "there were never any earlier rounds", which is the exact + # confusion this whole change exists to remove. + dropped=0 + while :; do + combined_size=$(( $(wc -c < "$body_file") + $(wc -c < "$archived_file") + 200 )) + [ "$combined_size" -le "$MAX_BODY_BYTES" ] && break + # Remove the LAST `
` block (the oldest round) from the archive. + trimmed="$(mktemp)" + agy_drop_oldest_round < "$archived_file" > "$trimmed" || { rm -f "$trimmed"; break; } + mv "$trimmed" "$archived_file" + dropped=$(( dropped + 1 )) + done + + { + printf '\n%s\n' "$AGY_ARCHIVE_START" + if [ "$dropped" -gt 0 ]; then + printf '%d earlier round(s) dropped to stay under GitHub'"'"'s comment size limit.\n\n' "$dropped" + fi + printf '
\nEarlier review rounds (newest first)\n\n' + cat "$archived_file" + printf '\n
\n' + printf '%s\n' "$AGY_ARCHIVE_END" + } >> "$body_file" + + # Re-run the OAuth guard on the ASSEMBLED body. The archive is text this script published + # earlier and so has already passed the guard once, but the body is what gets published now + # and the guard's contract is that it runs on exactly that. + if oauth_url_present "$body_file"; then + log "refusing to post: the assembled comment body contains a live OAuth authorization URL." + exit 1 + fi + + if gh api -X PATCH "repos/${REPO}/issues/comments/${prior_id}" \ + -f body="$(cat "$body_file")" >/dev/null 2>&1; then + log "updated review comment ${prior_id} on ${REPO}#${PR} (earlier rounds archived in place)" + exit 0 + fi + log "warning: could not edit comment ${prior_id}; posting a fresh review instead" +fi if ! post_output="$(gh pr comment "$PR" --repo "$REPO" --body-file "$body_file" 2>&1)"; then - # Nothing is deleted when the post fails: the prior review comment is still the best - # information the PR has, and removing it would leave no review at all. log "failed to post review to ${REPO}#${PR}: ${post_output}" exit 1 fi log "posted review to ${REPO}#${PR}" -new_comment_id="$(printf '%s\n' "$post_output" | sed -n 's/.*#issuecomment-\([0-9][0-9]*\).*/\1/p' | tail -n 1)" - -# A failed delete is logged, not swallowed: silently ignoring it would let a transient API/perms -# error leave the old comment in place alongside the new one, so runs accumulate duplicates. -# The author filter is load-bearing, not cosmetic: without it, ANY user could put the -# marker (an HTML comment) in a PR comment and have this bot delete arbitrary comments on -# the next run. Only ever delete OUR OWN bot's prior review comments -- and only ones from -# BEFORE this run (the just-posted comment's own id is excluded so it can never delete itself). -if [ -z "$new_comment_id" ]; then - # FAIL CLOSED. Without a known id there is no way to tell the new comment from the old ones, - # and the safe direction is unambiguous: a leftover duplicate is noise, deleting the review - # that was just posted is data loss. - log "warning: could not determine the posted comment id; leaving prior review comments in place" -else - # `--arg`/`--argjson` rather than shell interpolation into the filter: the marker is an HTML - # comment today, but a quote or a backslash in it would otherwise break the jq program itself - # rather than simply not matching. - # - # Those are JQ flags, so the JSON is fetched raw and piped into a real `jq` — `gh api` has no - # `--arg`/`--argjson` of its own and rejects them. Handing them to `gh api --jq` made it exit - # non-zero on every run; with the old `2>/dev/null` swallowing the message and `set -o pipefail` - # in force, the script then died *after* posting, so the stale comments were never deleted and - # the job went red for a reason nothing printed. stderr is kept this time for exactly that - # reason. (`--paginate` without `--jq` emits one JSON array per page; `jq` reads that stream - # fine, applying `.[]` to each.) - stale_ids="$( - gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ - | jq -r --arg marker "$MARKER" --argjson new_id "$new_comment_id" "$SELECT_STALE_JQ" - )" || { - log "warning: could not list prior review comments; leaving them in place" - stale_ids="" - } - while read -r cid; do - [ -n "$cid" ] || continue - if ! gh api -X DELETE "repos/${REPO}/issues/comments/${cid}" >/dev/null 2>&1; then - log "warning: could not delete prior review comment ${cid}; a duplicate may result" - fi - done <<< "$stale_ids" -fi - -# The delete loop above is the last real work; end on a defined status so a stray non-zero from -# it can never be mistaken for "the review failed" once the comment is already published. -exit 0 From 0e98c196a4c7e02ec93ce070f0d97cc1cbd73ac6 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 11:22:58 -0400 Subject: [PATCH 13/20] ci(agy): SHA-pin the reviewer's checkout, adopted from the sibling repos The Antigravity reviewer's checkout used `actions/checkout@v7`, a tag that moves. RustySNES and SLAC already pinned it to a SHA; the template and this repo did not, so hardening was flowing the wrong way between copies of the same file. It matters more here than on a hosted job. This workflow runs on a SELF-HOSTED runner -- the maintainer's own machine, holding the agy CLI's Google AI Ultra OAuth session -- so a compromised tag executes there rather than in a disposable VM. The same reasoning already applied to `dtolnay/rust-toolchain` in this repo's CI, and this was the remaining unpinned action on the highest-trust runner. The SHA was verified rather than copied: `actions/checkout` tag v7 resolves to 3d3c42e5aac5ba805825da76410c181273ba90b1, the "prep v7.0.1 release" commit of 2026-07-17. The trailing `# v7` is the form Dependabot's github-actions ecosystem reads to keep the pin current, so it is not decoration. Synced to the canonical template, which now carries the pin for every future install. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- .github/workflows/antigravity-review.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index ca46d821..d500f69f 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -67,7 +67,13 @@ jobs: # branch throughout. Consequence worth knowing: a PR that edits the reviewer or the # style guide is reviewed by the version already on the default branch until it merges. - name: Check out repo (for the style guide + scripts) - uses: actions/checkout@v7 + # SHA-pinned, not `@v7`: this job runs on a SELF-HOSTED runner -- the + # maintainer's own machine -- so a compromised tag would execute there + # rather than in a disposable VM. Verified to be exactly what `v7` + # resolves to (v7.0.1, 2026-07-17). The trailing `# v7` is the form + # Dependabot's github-actions ecosystem reads to keep the pin current. + # Adopted FROM RustySNES and SLAC, which had it while the template did not. + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: ref: ${{ github.event.repository.default_branch }} # Not `fetch-depth: 0`: the large-diff fallback fetches exactly the two refs it From c14dd6fcbdf8cffa913da9c55cc99e99f4d086e2 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 11:30:18 -0400 Subject: [PATCH 14/20] ci(agy): adopt SLAC's backend-error guard and marker-based test extraction Two mechanisms existed in exactly one of the five installs. Both belong in all of them, and the sweep that unified the comment-archive behaviour is the right moment to say so. A BACKEND OUTAGE POSTED AS A PASSING REVIEW When agy's upstream is down it prints an error rather than a review: Error: Eligibility check failed: UNAVAILABLE (code 503): The service is currently unavailable. That text is non-empty, so have_text() treated it as a valid review, POSTed it as the review comment, and the job exited 0 -- a green check for a review that never ran. Observed twice on SLAC PR #14, where the check passed in seven seconds with that string as its entire body. A control that cannot fail is worse than no control. The match is deliberately ANCHORED to the start of the capture rather than being a substring search, and it is bounded by size. A genuine review may quote a 503 or an UNAVAILABLE constant while reviewing retry logic, and aborting on that would be the false positive the OAuth guard's design notes warn about. A backend failure IS the whole capture and begins with `Error:`, so requiring the error on line one, in a capture short enough to contain nothing else, separates the two without a content heuristic. A backend error is transient, so it retries like empty output rather than aborting the way a lapsed session does -- but the capture is blanked so no later path can post it. The tally is a COUNTER, not a per-attempt flag: a boolean reset each attempt reflects only the last one, so a 503 on attempt 1 followed by empty output on attempt 3 would report the wrong cause. Both exit non-zero, so nothing unsafe -- but the log line is the only thing telling a human which outage they are looking at. MARKER-BASED EXTRACTION, AND WHY IT IS BETTER The selftest lifted the jq filter out of the reviewer by matching the declaration's own syntax: a sed range ending at the first line closing with a quote. A filter whose body ever ended a line that way would be SILENTLY TRUNCATED, and a truncated jq program can still compile and still return ids -- the exact silent-wrong-answer that file exists to prevent. Explicit `SELFTEST-EXTRACT` markers replace it. They also let a guard be several statements rather than one assignment, which is what makes the OAuth and service-error guards testable at all. Every marked block is now asserted to exist, to be valid shell, and to be sourceable, because a renamed marker would extract EMPTY -- and an empty guard sources fine and asserts nothing. WHAT THE MUTATIONS CHANGED Three of six came back NOT CAUGHT on the first pass, and two were real. The anchor could be deleted with every check still passing, because the fixture for "a review discussing a 503" put the error on line 3, where `head -n 1` already excluded it. A fixture whose FIRST line contains the error text mid-line -- which only `^` can reject -- now covers it. The persistent-outage abort was checked by grepping the script for its condition, which `if false && [ ... ]` still satisfies. That decision is now a named function, `backend_outage_should_fail`, called by the test rather than grepped for; three mutations of it are caught where the grep caught none. `have_text` moved inside the marked block so the block is self-contained -- the marker is a comment, so nothing about where the function is defined changed. The third, removing the `[ -s ]` empty-file check, is an EQUIVALENT mutant and is recorded as such rather than papered over with a test: an empty capture yields no grep match either way, so the check is defensive and its removal is unobservable. Superset verified rather than assumed: every non-comment line SLAC had before this sweep is either present in the template or is old delete machinery this design removes, plus a large-diff fallback the template supersedes -- SLAC's copy handled GitHub's 20,000-line limit only, the template's handles the 300-FILE limit too. All five installs now run one implementation. Selftest passes and actionlint is clean in each. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- scripts/agy-review-selftest.sh | 72 ++++++++++++++++++++++++- scripts/agy-review.sh | 96 ++++++++++++++++++++++++++++++++-- 2 files changed, 162 insertions(+), 6 deletions(-) mode change 100644 => 100755 scripts/agy-review-selftest.sh diff --git a/scripts/agy-review-selftest.sh b/scripts/agy-review-selftest.sh old mode 100644 new mode 100755 index 0a4153a6..9851e416 --- a/scripts/agy-review-selftest.sh +++ b/scripts/agy-review-selftest.sh @@ -30,11 +30,33 @@ SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" extract_marker() { sed -n 's/^MARKER="\(.*\)"$/\1/p' "$SCRIPT_DIR/agy-review.sh" | head -n 1 } +# Extraction is delimited by explicit `SELFTEST-EXTRACT` markers rather than by matching the +# declaration's own syntax. Adopted FROM SLAC, and it is the better mechanism: the `sed` range +# it replaced ended at the first line closing with a quote, so a filter whose body ever ended a +# line that way would be silently TRUNCATED -- and a truncated jq program can still compile and +# still return ids, which is exactly the silent-wrong-answer this file exists to prevent. +# Markers also let a guard be several statements rather than one assignment, which is what makes +# the OAuth and service-error guards testable at all. +extract_block() { + sed -n "/^# >>> SELFTEST-EXTRACT: $1\$/,/^# <<< SELFTEST-EXTRACT\$/p" "$SCRIPT_DIR/agy-review.sh" +} extract_filter() { - sed -n "/^SELECT_OURS_JQ='/,/'\$/p" "$SCRIPT_DIR/agy-review.sh" \ + extract_block "ours-comment filter" \ + | sed -n "/^SELECT_OURS_JQ='/,/'\$/p" \ | sed "1s/^SELECT_OURS_JQ='//; \$s/'\$//" } +# Every marked block must exist and be sourceable. A renamed or unbalanced marker would +# otherwise extract EMPTY, and an empty guard sources fine and asserts nothing -- the same +# absence-reads-as-agreement failure the markers were adopted to prevent. +for guard in "service-error guard" "oauth guard" "ours-comment filter"; do + blk="$(extract_block "$guard")" + [ -n "$blk" ] || { echo "FAIL: SELFTEST-EXTRACT block '$guard' is missing or empty" >&2; exit 1; } + printf '%s\n' "$blk" | bash -n - 2>/dev/null \ + || { echo "FAIL: extracted block '$guard' is not valid shell (unbalanced markers?)" >&2; exit 1; } + eval "$blk" +done + MARKER="$(extract_marker)" FILTER="$(extract_filter)" @@ -192,6 +214,54 @@ else echo " ok --arg/--argjson are not passed to \`gh api\`" fi +# --- the service-error guard --------------------------------------------------- +# When agy's upstream is down it prints an error, not a review. That text is non-empty, so +# `have_text` alone treats it as a valid review and POSTs it -- a green check for a review that +# never ran. Observed twice on SLAC PR #14, where the check passed in 7 seconds with the error +# string as its entire body. A control that cannot fail is worse than no control. +se() { printf '%s' "$1" > "$TMPD/cap"; service_error_present "$TMPD/cap" && echo MATCH || echo NOMATCH; } +TMPD="$(mktemp -d)" +trap 'rm -rf "$TMPD"' EXIT + +check "a bare backend 503 is caught" "MATCH" \ + "$(se 'Error: Eligibility check failed: UNAVAILABLE (code 503): The service is currently unavailable.')" +check "RESOURCE_EXHAUSTED is caught" "MATCH" "$(se 'Error: RESOURCE_EXHAUSTED')" + +# The false positives the anchoring exists to avoid. A genuine review may quote a 503 while +# reviewing retry logic, and aborting on that would be the very failure the OAuth guard's design +# notes warn about. +check "a review DISCUSSING a 503 is not caught" "NOMATCH" \ + "$(se "$(printf '## Review\n\nThe retry path should handle Error: UNAVAILABLE (code 503) here.\n')")" +check "an error on line 3 is not caught (only line 1 counts)" "NOMATCH" \ + "$(se "$(printf '## Review\n\nError: UNAVAILABLE (code 503)\n')")" +check "an empty capture is not caught" "NOMATCH" "$(se '')" + +# The ANCHOR, tested where it is the only thing that matters: the error text is on LINE ONE but +# not at its start. `head -n 1` cannot exclude this; only `^` can. Without this fixture the +# anchor could be deleted and every other check still passed. +check "a heading MENTIONING an error mid-line is not caught" "NOMATCH" \ + "$(se '## Review of the Error: UNAVAILABLE (code 503) retry path')" + +# ...and the anchored form still matches with leading whitespace, which the regex allows. +check "a leading-whitespace error is still caught" "MATCH" \ + "$(se ' Error: UNAVAILABLE (code 503)')" + +# The size cap is what separates "the error IS the whole capture" from "a review mentions one". +long="Error: UNAVAILABLE (code 503) $(head -c 3000 /dev/zero | tr '\0' 'x')" +check "a long capture opening with an error is not caught" "NOMATCH" "$(se "$long")" + +# The script must FAIL rather than post when the backend errored and nothing was produced. +# Called, not grepped for: `if false && [ "${service_errors:-0}" -gt 0 ]` still contains what a +# structural grep looks for, and that mutation came back NOT CAUGHT. +: > "$TMPD/empty" +printf 'a real review\n' > "$TMPD/review" +check "outage + no review -> fail the job" "0" \ + "$(backend_outage_should_fail 2 "$TMPD/empty"; echo $?)" +check "outage + a review -> do not fail" "1" \ + "$(backend_outage_should_fail 2 "$TMPD/review"; echo $?)" +check "no outage + no review -> not THIS guard's job" "1" \ + "$(backend_outage_should_fail 0 "$TMPD/empty"; echo $?)" + if [ "$fails" -ne 0 ]; then echo "$fails check(s) failed" >&2 exit 1 diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 03299072..0af20131 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -17,7 +17,60 @@ set -euo pipefail # clamp, so log() must already exist. (Defined later, a clamp that fires would die with # `log: command not found` under set -e instead of warning — a latent misconfiguration trap.) log() { printf '[agy-review] %s\n' "$*" >&2; } + +# A capture that is only a BACKEND ERROR, not a review. When agy's upstream is down it prints +# something like: +# +# Error: Eligibility check failed: UNAVAILABLE (code 503): The service is currently unavailable. +# +# That text is non-empty, so have_text() alone treats it as a valid review, POSTs it as the +# review comment, and the job exits 0 -- a green check for a review that never happened. Observed +# on SLAC PR #14: the `review` check passed in 7 seconds with that string as its entire body, +# twice. A control that cannot fail is worse than no control. +# +# The match is deliberately ANCHORED to the start of the capture rather than being a substring +# search. A genuine review may legitimately quote a 503, an "UNAVAILABLE" constant, or an +# eligibility check while reviewing retry logic -- and aborting on that would be the +# false-positive the OAuth guard's design notes warn about. agy's backend failures occupy the +# WHOLE capture and begin with `Error:`, so requiring the error at the top, in a capture short +# enough to contain nothing else, separates the two without a content heuristic. +# +# Adopted FROM SLAC, which had it while the template and the other installs did not. +# >>> SELFTEST-EXTRACT: service-error guard +# `have_text` lives INSIDE this block, not above it: `backend_outage_should_fail` calls it, so +# an extracted block without it is not self-contained and the selftest cannot run it. It is a +# general helper used throughout the script -- the marker is a comment, so its position +# changes nothing about where the function is defined. have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } +AGY_ERROR_RE='^[[:space:]]*Error:.*(Eligibility check failed|UNAVAILABLE|unavailable|RESOURCE_EXHAUSTED|INTERNAL|DEADLINE_EXCEEDED|code [45][0-9]{2})' +# Captures longer than this are assumed to be real reviews even if they open with an error line: +# a genuine review is thousands of bytes, a bare backend error is a couple of hundred. +AGY_ERROR_MAX_BYTES="${AGY_ERROR_MAX_BYTES:-2000}" +# `head -n 1`, not `head -n 5`. grep matches line-by-line, so scanning five lines +# means "any of the first five lines is an error line" -- which would discard a short, +# genuine review whose heading is on line 1 and which happens to discuss `Error: ... +# UNAVAILABLE` on line 3. That is the false positive the anchoring was supposed to +# prevent, reintroduced by the very check meant to enforce it. The invariant is that a +# backend failure IS the whole capture, so only the first line can carry it. +service_error_present() { + [ -s "$1" ] || return 1 + [ "$(wc -c < "$1")" -le "$AGY_ERROR_MAX_BYTES" ] || return 1 + head -n 1 "$1" | grep -qE "$AGY_ERROR_RE" +} + +# True when the run must FAIL rather than post: the backend errored at least once and no review +# survived. A separate named function rather than an inline condition, because a structural grep +# for the condition is too weak to notice it being disabled -- `if false && [ ... ]` still +# contains the text a grep looks for, and that mutation came back NOT CAUGHT. +# +# The `have_text` half is redundant today (the retry loop truncates the capture whenever it +# counts a backend error) and is kept deliberately: it makes the "post nothing" guarantee +# independent of that truncation surviving a future edit. +backend_outage_should_fail() { + [ "${1:-0}" -gt 0 ] || return 1 + ! have_text "$2" +} +# <<< SELFTEST-EXTRACT # Guarding against a leak of agy's interactive Google OAuth login flow. When agy's cached # session lapses on the runner, `--print` emits the login prompt (a live OAuth URL + "paste the # authorization code here") instead of a review; that text is non-empty, so have_text() alone @@ -44,6 +97,7 @@ have_text() { [ -s "$1" ] && grep -q '[^[:space:]]' "$1"; } # would miss a URL emitted as `.../o/oauth2?client_id=…` or bare `.../o/oauth2` (a leak). The # scheme is what keeps a review's bare-pattern quote from matching, so dropping the trailing # slash loses no safety. +# >>> SELFTEST-EXTRACT: oauth guard OAUTH_URL_RE='https?://accounts\.google\.com/o/oauth2' # The single guard, used at BOTH the retry-loop capture (Layer 1) and the assembled body before @@ -54,6 +108,7 @@ OAUTH_URL_RE='https?://accounts\.google\.com/o/oauth2' # URL is always present in a real login flow, so it alone is both necessary and sufficient; a # lapse is detected AND a leak is blocked by the same unconditional check. oauth_url_present() { [ -s "$1" ] && grep -qiE "$OAUTH_URL_RE" "$1"; } +# <<< SELFTEST-EXTRACT # --- configuration (all env-overridable from the workflow) --------------------- AGY_BIN="${AGY_BIN:-agy}" @@ -119,12 +174,14 @@ MAX_BODY_BYTES="${MAX_BODY_BYTES:-60000}" # (an HTML comment, invisible when rendered) in a PR comment and have this bot edit it. Only # ever touch our own bot's comments. `first` picks the OLDEST match, so if duplicates exist # from an older version of this script, the canonical thread is the one that keeps growing. +# >>> SELFTEST-EXTRACT: ours-comment filter SELECT_OURS_JQ='[ .[] | select(.user.type == "Bot" and .user.login == "github-actions[bot]") | select(.body | contains($marker)) ] | first | .id // empty' readonly SELECT_OURS_JQ +# <<< SELFTEST-EXTRACT REPO="${GITHUB_REPOSITORY:?GITHUB_REPOSITORY not set}" @@ -162,17 +219,16 @@ log "reviewing ${REPO}#${PR}" # Remove every temp file on exit. Pre-declared so the trap is safe under `set -u` even if the # script exits before a given file is created. diff_file= diff_err= meta_file= prompt_file= out_file= raw= body_file= agy_diff_file= agy_work_dir= -# The archive path's two temporaries. Declared here rather than left to their assignment, -# so the trap frees them on every exit -- including the `exit 0` after a successful edit. -prior_body_file= archived_file= # Set to 1 if agy printed its interactive OAuth login flow instead of a review (lapsed session); # gates the no-post abort below. Pre-declared so `${auth_failed:-0}` is set-u-safe on every path. auth_failed=0 +# Counts backend-error captures across attempts; see `service_error_present`. Pre-declared so +# `${service_errors:-0}` is set-u-safe even on paths that never enter the retry loop. +service_errors=0 # Set when the large-diff fallback below creates refs/agy/* so the trap can remove them. agy_refs_created= cleanup() { - rm -f "$diff_file" "$diff_err" "$meta_file" "$prompt_file" "$out_file" "$raw" "$body_file" \ - "$agy_diff_file" "$prior_body_file" "$archived_file" + rm -f "$diff_file" "$diff_err" "$meta_file" "$prompt_file" "$out_file" "$raw" "$body_file" "$agy_diff_file" # Remove the gitignored diff-handoff scratch dir once its file is gone. `rmdir` only unlinks an # empty dir, so a concurrent run's file (a different $$) is never clobbered; a non-empty dir is # gitignored and harmless if left behind. @@ -558,6 +614,21 @@ for (( attempt=1; attempt<=AGY_RETRIES; attempt++ )); do # reading it would post that session's output into a public PR comment (data # leak). The PTY path above plus the retry loop cover agy issue #76 without it. + # A backend error is TRANSIENT, so it retries like empty output rather than aborting the way a + # lapsed session does -- but it must never be mistaken for a review. Blank the capture so no + # later path can post it, and let the loop try again. + # + # A COUNTER, not a per-attempt flag. A boolean reset each attempt reflects only the last one, + # so a 503 on attempt 1 followed by empty output on attempt 3 would report the generic "no + # output" cause, and the reverse would claim the backend was down on every attempt when it was + # down on one. Both exit non-zero, so nothing unsafe -- but the log line is the only thing + # telling a human which outage they are looking at. + if service_error_present "$out_file"; then + log "agy returned a backend error rather than a review (attempt ${attempt}/${AGY_RETRIES}): $(head -n 1 "$out_file")" + : > "$out_file" + service_errors=$(( ${service_errors:-0} + 1 )) + fi + have_text "$out_file" && break if [ "$attempt" -lt "$AGY_RETRIES" ]; then delay=$(( AGY_RETRY_DELAY * attempt )) @@ -575,6 +646,17 @@ if [ "${auth_failed:-0}" = "1" ]; then exit 1 fi +# A persistent backend failure is reported as its own cause, and it FAILS THE JOB. Posting the +# error as a review comment (the previous behaviour) produced a passing check for a review that +# never ran; posting nothing and exiting non-zero makes the outage visible where it matters. +# The `! have_text` is redundant today -- the loop truncates $out_file whenever it counts a +# backend error -- and is kept deliberately: it makes the "post nothing" guarantee independent +# of that truncation surviving a future edit. +if backend_outage_should_fail "${service_errors:-0}" "$out_file"; then + log "agy's backend returned an error on ${service_errors} of ${AGY_RETRIES} attempt(s) and no review was produced. Failing the check rather than posting the error as a review. Re-run with '/agy-review' once the service recovers." + exit 1 +fi + if ! have_text "$out_file"; then log "no review output after ${AGY_RETRIES} attempt(s). Check $LOG and confirm 'agy -p \"hi\"' works for this user." # Surface agy's stderr into the job log. RUNNER_TEMP is wiped between jobs, so a bare @@ -680,6 +762,7 @@ if [ -n "$prior_id" ] && [ -s "$prior_body_file" ]; then printf '\n
\n' printf '%s\n' "$AGY_ARCHIVE_END" } >> "$body_file" + rm -f "$archived_file" # Re-run the OAuth guard on the ASSEMBLED body. The archive is text this script published # earlier and so has already passed the guard once, but the body is what gets published now @@ -692,10 +775,13 @@ if [ -n "$prior_id" ] && [ -s "$prior_body_file" ]; then if gh api -X PATCH "repos/${REPO}/issues/comments/${prior_id}" \ -f body="$(cat "$body_file")" >/dev/null 2>&1; then log "updated review comment ${prior_id} on ${REPO}#${PR} (earlier rounds archived in place)" + rm -f "$prior_body_file" exit 0 fi log "warning: could not edit comment ${prior_id}; posting a fresh review instead" fi +rm -f "$prior_body_file" + if ! post_output="$(gh pr comment "$PR" --repo "$REPO" --body-file "$body_file" 2>&1)"; then log "failed to post review to ${REPO}#${PR}: ${post_output}" exit 1 From b662c184570d6dadd41a5d07301cb64a04bae01b Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 11:49:23 -0400 Subject: [PATCH 15/20] fix(ci): two CI failures I caused, and the gate claim that should have caught one Both were on the PR's own run, and both are mine. A RUSTDOC LINK I CLAIMED TO HAVE CHECKED `write_atomic` is public and `post_rename_sync_error` is private, so an intra-doc link between them fails `rustdoc::private-intra-doc-links` under `-D warnings`. Now a plain code span, which is the rule this repository already applies to feature-gated dependency names. The commit that introduced it listed "rustdoc with warnings as errors" among its gates. That claim was false: the full gate run predated the last edits, and I did not re-run it before committing. The lint is exactly the sort a green earlier run cannot vouch for, which is the whole reason the gate is meant to be re-run rather than remembered. Recorded plainly because the failure was the claim, not the link. THE WORKFLOW AND THE SCRIPTS COME FROM DIFFERENT REFS The reviewer workflow checks out the DEFAULT BRANCH to get its scripts -- that is deliberate, and documented, so a fork's code never executes on the self-hosted runner. But for a `pull_request` event GitHub runs the workflow YAML itself from the PR BRANCH. So the two halves come from different refs, and a change spanning both breaks its own PR. The previous commit added `scripts/_agy_comment_body.sh` and added it to the workflow's chmod; the job then died with chmod: cannot access 'scripts/_agy_comment_body.sh': No such file or directory because the checkout was of `main`, which does not have the file yet. The surprising half is the inversion: the default-branch rule is documented for the scripts -- a change to agy-review.sh has no effect until it merges -- and the corollary is that the workflow moves IMMEDIATELY while the scripts do not. That runs against the usual intuition that everything in a PR is consistent with itself. The workflow half now tolerates both script sets: the two required files are chmod'd unconditionally, anything added later only if present, with a trailing `true` so a false `[ -f ]` cannot fail the step under `bash -e`. A genuinely missing required file still fails loudly, because agy-review.sh sources it and dies -- the tolerance is in the workflow, not in the contract. Swept to the canonical template and to all four sibling installs, whose open PRs would have hit the identical failure on their next run. Gates, re-run in full this time: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, 573 frontend tests, the reviewer selftest, and actionlint. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- .github/workflows/antigravity-review.yml | 20 +++++++++++++++++--- crates/rustynes-frontend/src/atomic_write.rs | 6 +++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index d500f69f..4a5abbbc 100644 --- a/.github/workflows/antigravity-review.yml +++ b/.github/workflows/antigravity-review.yml @@ -101,7 +101,21 @@ jobs: # MAX_PROMPT_BYTES: "125000" # inline/file threshold + hard backstop on the argv prompt # STYLE_GUIDE: .github/agy-review.md # style guide, loaded if present run: | - # `_agy_comment_body.sh` is sourced by the reviewer at startup, so a - # missing or non-executable copy fails the run rather than degrading it. - chmod +x scripts/agy-review.sh scripts/_agy_print.sh scripts/_agy_comment_body.sh + # The workflow and the scripts come from DIFFERENT REFS: for a `pull_request` + # event GitHub runs this YAML from the PR branch, while the checkout step + # above deliberately fetches the DEFAULT branch to get the scripts. So a + # change that adds a script file breaks its own PR -- the new workflow + # chmods a file the default branch does not have yet. Observed exactly + # once, on the PR that introduced `_agy_comment_body.sh`. + # + # The two required files are chmod'd unconditionally; anything added later + # is chmod'd only if present, so the workflow stays compatible with both + # the old and the new script set. A genuinely missing required file still + # fails loudly -- `agy-review.sh` sources it and dies -- rather than being + # papered over here. + chmod +x scripts/agy-review.sh scripts/_agy_print.sh + for opt in scripts/_agy_comment_body.sh; do + [ -f "$opt" ] && chmod +x "$opt" + done + true scripts/agy-review.sh diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs index 1f9a9f3b..fbf98e81 100644 --- a/crates/rustynes-frontend/src/atomic_write.rs +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -431,7 +431,11 @@ where /// Returns the underlying [`io::Error`] from any step. A rename that fails after /// `RENAME_ATTEMPTS` transient failures returns the last error rather than /// succeeding quietly. A post-rename sync failure is wrapped so its message says -/// the data was written; see [`post_rename_sync_error`]. +/// the data was written; see `post_rename_sync_error`. +/// +/// (A plain code span, not an intra-doc link: `write_atomic` is public and that function +/// is private, which `rustdoc::private-intra-doc-links` rejects under `-D warnings`. Same +/// rule this repo already applies to feature-gated dependency names.) pub fn write_atomic(path: &Path, contents: &[u8]) -> io::Result<()> { write_atomic_with(path, contents, scratch_name, sync_parent_dir) } From af588d6c3e4eb50a155cbdaa4937c2fe23991c95 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 11:59:55 -0400 Subject: [PATCH 16/20] fix(frontend): a const fn that only fails on Windows, and the cfg that hid it `is_transient_rename_error` was a `const fn` whose body called `io::Error::kind`. That method is not `const`, so the call is E0015 -- but the call sat behind `#[cfg(windows)]`, so the body was never compiled on Linux and every local gate and every PR check passed. PR runs here are Linux-only; the full matrix runs on `main`. So this would have turned `main` RED AFTER MERGE, on the branch where a red build blocks a release, rather than failing the PR that introduced it. Caught in review, not by a gate. Verified before being believed. A plausible reviewer claim had already proved false once this release, so the language question was settled against a minimal two-line crate: `const fn f(e: &io::Error) -> bool { matches!(e.kind(), ...) }` gives `error[E0015]: cannot call non-const method std::io::Error::kind in constant functions`. It was also checked against `main` -- the function is new in this PR, so nothing shipped broken. THE FIX IS NOT JUST DROPPING `const` Dropping it would fix this instance and leave the mechanism intact: any Windows- only code behind a `#[cfg]` is invisible to a Linux PR build, so the next error in it would land the same way. The Windows predicate now lives in an always-compiled function, reached through `cfg!(windows) && is_windows_sharing_violation(e)` rather than a `#[cfg]` block. `cfg!` is a compile-time boolean inside an ordinary expression, so the predicate is parsed, type-checked and borrow-checked on every platform, while `&&` short-circuits it away on non-Windows and the optimizer drops the branch. Runtime behaviour is identical; what changes is that a Linux `cargo check` now compiles the Windows logic. The proof is that restoring the `const` NOW FAILS ON LINUX, with `error[E0015]: cannot call non-const function is_windows_sharing_violation in constant functions`. The defect class moved from "invisible until another platform builds it" to "fails the PR". Two tests come with it: the sharing-violation predicate is exercised on whatever platform the suite runs, and a Unix-only test pins that the public predicate stays unconditionally false, so `cfg!` did not quietly change the single-attempt guarantee. Three mutations caught -- the predicate inverted, the `cfg!` guard dropped so Unix would retry, and the symlink bound reduced -- and a fourth, restoring `const`, is now a compile error rather than a silent pass. SYMLINK_DEPTH MATCHES THE KERNEL Raised from 8 to 40, Linux's own MAXSYMLINKS. The old value was justified as "far past any real dotfiles arrangement", which is true and beside the point: where the kernel would resolve a chain and this function gives up, the two disagree about where the file IS, and the write lands somewhere the user did not mean. It costs one `read_link` per level, on a chain already known to be broken. DECLINED, WITH REASONS A `Drop` guard removing the scratch file if `write_all` panics: the bounded scratch loop exists precisely so an orphan is survivable, and it now advances past up to eight of them. Adding an unwinding path to buy what the retry already handles is not worth the surface. SCRATCH_ATTEMPTS staying 8 rather than a rounder 5 or 10: the number is bounded by "how many scratch files one crashed run can plausibly orphan", and 8 is already generous for that. Rounding it changes nothing measurable. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 579 frontend tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- CHANGELOG.md | 18 ++++ crates/rustynes-frontend/src/atomic_write.rs | 101 ++++++++++++++++--- 2 files changed, 106 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3823e8c..2ecf3670 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,24 @@ cycle-accurate core later replaced. scratch-name source is now injectable so the exhaustion branch is reachable without predicting global state. + A fifth round found the one defect none of the local gates could see: the + transient-rename predicate was a `const fn` calling `io::Error::kind`, which is + not `const` — `E0015`, and **only on Windows**, because the call sat behind + `#[cfg(windows)]`. PR runs here are Linux-only and the full matrix runs on + `main`, so it would have turned `main` red after merge rather than failing the + PR that caused it. Verified against a minimal crate before being believed, since + a plausible reviewer claim had already proved false once this release. + + The fix is not just dropping `const`. The Windows logic moved into an + always-compiled function reached through `cfg!(windows) && …` instead of + `#[cfg(windows)]`, so it is parsed, type-checked and borrow-checked on Linux + while short-circuiting away at runtime exactly as before. Restoring the `const` + now fails **on Linux** with the same `E0015` — the defect class moved from + "invisible until another platform builds it" to "fails the PR". Also from that + round: `SYMLINK_DEPTH` matches Linux's own `MAXSYMLINKS` of 40 rather than a + smaller number, because where the kernel resolves a chain and this gives up, the + two disagree about where the file *is*. + A third round found no blocking issues and two worthwhile refinements: `ENOTSUP` / `EOPNOTSUPP` joins the excused set, since the list is *ways a filesystem says there is no barrier here* and leaving one out fails a save on that mount; and diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs index fbf98e81..1ddd1f88 100644 --- a/crates/rustynes-frontend/src/atomic_write.rs +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -264,9 +264,15 @@ pub fn resolve_write_target(path: &Path) -> PathBuf { /// How many broken-symlink hops to follow before giving up. /// /// Bounded because a chain can be a cycle, on which `read_link` succeeds -/// indefinitely. Linux's own limit is 40; eight is far past any real dotfiles -/// arrangement and keeps a pathological path cheap. -const SYMLINK_DEPTH: usize = 8; +/// indefinitely. +/// +/// Matches Linux's own `MAXSYMLINKS` of 40 rather than picking a smaller number. +/// The earlier value of 8 was justified as "far past any real dotfiles +/// arrangement", which is true and beside the point: where the kernel would +/// resolve a chain and this function gives up, the two disagree about where the +/// file *is*, and the write lands somewhere the user did not mean. Costs one +/// `read_link` per level, and only on a chain already known to be broken. +const SYMLINK_DEPTH: usize = 40; /// Is this rename failure one a retry could plausibly clear? /// @@ -281,16 +287,43 @@ const SYMLINK_DEPTH: usize = 8; /// genuinely forbid it — a condition retrying cannot change, and retrying would /// only delay an error the caller needs now. #[must_use] -pub const fn is_transient_rename_error(e: &io::Error) -> bool { - #[cfg(windows)] - { - matches!(e.kind(), io::ErrorKind::PermissionDenied) - } - #[cfg(not(windows))] - { - let _ = e; - false - } +pub fn is_transient_rename_error(e: &io::Error) -> bool { + // `cfg!`, not `#[cfg]`, and that is the whole point. `cfg!` is a compile-time + // boolean in an ordinary expression, so the Windows predicate is PARSED, + // TYPE-CHECKED and borrow-checked on every platform, while `&&` short-circuits + // it away on non-Windows and the optimizer drops the branch entirely. Runtime + // behaviour is identical to the `#[cfg]` form; what changes is that a Linux + // `cargo check` now compiles the Windows logic. + // + // The `#[cfg]` form is what let a defect through: the body was `const` and + // called `io::Error::kind`, which is not a `const fn`, so it was `E0015` on + // Windows and invisible on Linux, where the branch was never compiled. PR runs + // here are Linux-only and the full matrix runs on `main`, so it would have + // turned `main` red after merge. Caught in review, not by a gate. + cfg!(windows) && is_windows_sharing_violation(e) +} + +/// The Windows half of the predicate, compiled on **every** platform. +/// +/// Two reasons, and the second is why it exists at all. +/// +/// It is not `const`: `io::Error::kind` is not a `const fn`, so a `const` +/// wrapper is `E0015: cannot call non-const method` — but only where the body is +/// compiled. `is_transient_rename_error` was `const` with the call behind +/// `#[cfg(windows)]`, so it compiled cleanly on Linux and would have failed the +/// Windows job **after merge**: PR runs are Linux-only here, and the full matrix +/// runs on `main`. Caught in review rather than by a gate. +/// +/// And it is NOT `cfg`-gated, which is the actual fix. Windows-only code behind a +/// `#[cfg]` is never type-checked by a Linux PR build, so any error in it is +/// invisible until the platform that compiles it runs — which is exactly what +/// happened. Reached through `cfg!(windows) && …` instead, the logic is compiled +/// everywhere and exercised by the test below on whatever platform the suite runs. +fn is_windows_sharing_violation(e: &io::Error) -> bool { + // `MoveFileEx` reports both `ERROR_ACCESS_DENIED` and `ERROR_SHARING_VIOLATION` + // as `PermissionDenied` through `std::io`, and only the second is transient — + // std does not distinguish them, so the retry covers both. + matches!(e.kind(), io::ErrorKind::PermissionDenied) } /// The retry loop, over an arbitrary rename operation. @@ -962,6 +995,48 @@ mod tests { sync_parent_dir(Path::new("./f.txt")).expect("an explicit `.` parent must work"); } + /// The Windows sharing-violation predicate, exercised on EVERY platform. + /// + /// This is the point of routing through `cfg!(windows) && …` rather than + /// `#[cfg(windows)]`: the Windows logic is compiled and testable on Linux, so + /// a defect in it fails here instead of on `main` after merge. The previous + /// form hid an `E0015` (a `const fn` calling the non-const + /// `io::Error::kind`) that no PR run could have seen. + #[test] + fn the_windows_sharing_violation_predicate_is_checked_on_every_platform() { + assert!( + is_windows_sharing_violation(&io::Error::from(io::ErrorKind::PermissionDenied)), + "MoveFileEx reports a sharing violation as PermissionDenied; it must retry" + ); + for kind in [ + io::ErrorKind::NotFound, + io::ErrorKind::AlreadyExists, + io::ErrorKind::InvalidInput, + ] { + assert!( + !is_windows_sharing_violation(&io::Error::from(kind)), + "{kind:?} is not transient; retrying it would only delay the error" + ); + } + } + + /// ...and on Unix the public predicate stays unconditionally false, so the + /// single-attempt guarantee holds. `cfg!` must not have changed that. + #[cfg(unix)] + #[test] + fn unix_never_treats_a_rename_error_as_transient() { + for kind in [ + io::ErrorKind::PermissionDenied, + io::ErrorKind::NotFound, + io::ErrorKind::AlreadyExists, + ] { + assert!( + !is_transient_rename_error(&io::Error::from(kind)), + "POSIX rename has no sharing violation; {kind:?} must not be retried" + ); + } + } + /// A post-rename sync failure must not read as "nothing was written". /// /// The parent-directory sync is the last step and necessarily runs *after* From b38e85361d71e83f0a529094f10cb6c706520383 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 12:12:28 -0400 Subject: [PATCH 17/20] fix(frontend): the cheat save reports its failure instead of printing it Raised as blocking in three consecutive review rounds before I stopped deferring it, and the deferral was wrong on its facts. `cheats::save` wrote its error to `stderr`. On a windowed build nobody reads `stderr`, so a save that failed looked exactly like one that worked and the user lost their cheat list for that ROM with no signal at all. Same defect class as the swallowed latency-config save fixed in #411, in a PR whose entire subject is not swallowing save errors. WHY I DEFERRED IT, AND WHY THAT WAS WRONG The stated reason was that the fix needed UI plumbing: a `Result` signature plus a status-bar path from egui paint code with nowhere to put an error. The second half was false. The panel already carries `error`, `raw_error` and `enc_error`, each rendered with the same `colored_label` idiom and each cleared on a ROM change. The place to put it already existed; I asserted otherwise without looking. WHAT IT LOOKS LIKE NOW `save` returns `io::Result<()>`, with the `create_dir_all` failure propagating rather than printing-and-returning, and a serialization failure mapped to `InvalidData` -- the caller has one error channel and one thing to tell the user, so a bespoke error type for a case these types cannot produce would be ceremony. `persist_cheats` takes `&mut CheatPanelState` and records the outcome, clearing the field on success so a fixed problem stops being reported. It is cleared on a ROM change too, because a save error names the PREVIOUS ROM's cheat file and carrying it across would report a failure against a game it never touched -- the stale-panel-state seam this project has hit three times. The panel renders it ABOVE the lists rather than beside the add-fields, because the message is not about any single edit: it says the whole list on screen is not on disk. A test forces the failure by pointing the data dir at a regular file, so `create_dir_all` cannot create the parent, and asserts an error reaches the caller at all. The three existing round-trip tests now assert the save succeeded rather than discarding its result. ALSO: THE REVIEW COMMENT NO LONGER GOES THROUGH ARGV `gh api -f body="$(cat "$body_file")"` passed the whole comment as a single execve argument. At `MAX_BODY_BYTES` that approaches 60 KB against a `MAX_ARG_STRLEN` of 128 KB on Linux -- close enough that raising the bound later would start failing with E2BIG, and the failure would read as a GitHub error rather than a local limit. It is now `jq -n --rawfile b "$body_file" '{body: $b}' | gh api ... --input -`. Nothing traverses argv, and `--rawfile` makes the value a JSON string by construction, so neither shell quoting nor `-F` type-coercion can reinterpret a body that happens to look like a number or a boolean. Swept to the template and the four sibling installs. Declined, with the reason unchanged: logging a failed `remove_file` in the cleanup paths. The result is discarded so the PRIMARY error survives, and a cleanup that fails when the disk is full should not displace the error that matters. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, 580 frontend tests, and the reviewer selftest. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- CHANGELOG.md | 9 ++ crates/rustynes-frontend/src/cheats.rs | 94 ++++++++++++++----- .../src/debugger/cheat_panel.rs | 32 ++++++- scripts/agy-review.sh | 11 ++- 4 files changed, 118 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ecf3670..7d07b78a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,15 @@ cycle-accurate core later replaced. scratch-name source is now injectable so the exhaustion branch is reachable without predicting global state. + A sixth round made the `cheats.rs` swallow blocking after three rounds of my + deferring it, and the deferral was wrong on its facts: I had claimed the fix + needed UI plumbing with nowhere to put the error, and the panel already had + three error fields rendered in exactly the idiom needed. `cheats::save` now + returns `io::Result<()>`, `persist_cheats` stores the failure in a `save_error` + field cleared on a ROM change like every other one, and the panel reports it + **above the lists**, because the message is not about any single edit — it says + the whole list on screen is not on disk. + A fifth round found the one defect none of the local gates could see: the transient-rename predicate was a `const fn` calling `io::Error::kind`, which is not `const` — `E0015`, and **only on Windows**, because the call sat behind diff --git a/crates/rustynes-frontend/src/cheats.rs b/crates/rustynes-frontend/src/cheats.rs index 1841559f..07269803 100644 --- a/crates/rustynes-frontend/src/cheats.rs +++ b/crates/rustynes-frontend/src/cheats.rs @@ -143,32 +143,42 @@ pub fn load(data_dir: &Path, rom_sha256: &[u8; 32]) -> Cheats { /// the `cheats` directory if missing. Best-effort: failures are logged, not /// fatal. #[cfg(not(target_arch = "wasm32"))] -pub fn save(data_dir: &Path, rom_sha256: &[u8; 32], genie: &[CheatEntry], raw: &[RawCheat]) { +/// Persist a ROM's cheat lists. +/// +/// # Errors +/// +/// Returns the underlying [`std::io::Error`] if the directory cannot be created +/// or the file cannot be written, and an [`std::io::ErrorKind::InvalidData`] +/// error if the lists cannot be serialized. +/// +/// The error PROPAGATES rather than being printed. It used to go to `stderr`, +/// which on a windowed build nobody is reading — the user saw a cheat list that +/// looked saved and was not. Same defect class as the latency-config save fixed +/// in #411, and the same fix: hand it to a caller that can show it. +pub fn save( + data_dir: &Path, + rom_sha256: &[u8; 32], + genie: &[CheatEntry], + raw: &[RawCheat], +) -> std::io::Result<()> { let path = cheat_path(data_dir, rom_sha256); - if let Some(parent) = path.parent() - && let Err(e) = fs::create_dir_all(parent) - { - eprintln!( - "rustynes: cheats dir {} create failed: {e}", - parent.display() - ); - return; + if let Some(parent) = path.parent() { + fs::create_dir_all(parent)?; } let file = CheatFile { cheats: genie.to_vec(), raw: raw.to_vec(), }; - match toml::to_string_pretty(&file) { - Ok(s) => { - // Atomic + durable, via the shared helper (v2.4.0 item C). This was - // `fs::write`, so an interruption truncated the user's whole cheat - // list for that ROM. - if let Err(e) = crate::atomic_write::write_atomic(&path, s.as_bytes()) { - eprintln!("rustynes: cheats {} write failed: {e}", path.display()); - } - } - Err(e) => eprintln!("rustynes: cheats serialize failed: {e}"), - } + // A serialization failure is not an I/O failure, but the caller has one error + // channel and one thing to tell the user -- "your cheats did not save" -- so it + // is mapped rather than given a bespoke error type for a case that cannot + // happen with these types. + let text = toml::to_string_pretty(&file) + .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; + // Atomic + durable, via the shared helper (v2.4.0 item C). This was + // `fs::write`, so an interruption truncated the user's whole cheat + // list for that ROM. + crate::atomic_write::write_atomic(&path, text.as_bytes()) } #[cfg(all(test, not(target_arch = "wasm32")))] @@ -215,7 +225,7 @@ mod tests { enabled: false, }, ]; - save(tmp.path(), &h(0x42), &genie, &raw); + save(tmp.path(), &h(0x42), &genie, &raw).expect("save"); let back = load(tmp.path(), &h(0x42)); assert_eq!(back.genie, genie); assert_eq!(back.raw, raw); @@ -232,7 +242,8 @@ mod tests { enabled: true, }], &[], - ); + ) + .expect("save"); save( tmp.path(), &h(0x02), @@ -241,7 +252,8 @@ mod tests { enabled: true, }], &[], - ); + ) + .expect("save"); assert_eq!(load(tmp.path(), &h(0x01)).genie[0].code, "AAAAAA"); assert_eq!(load(tmp.path(), &h(0x02)).genie[0].code, "BBBBBB"); } @@ -287,4 +299,40 @@ mod tests { assert_eq!(back.raw[0].compare, None); assert!(back.raw[0].enabled); } + + /// A save that cannot happen must REPORT, not print. + /// + /// The error used to go to `stderr`, which nobody reads on a windowed build, + /// so a failed save looked exactly like a successful one and the user lost + /// their cheat list for that ROM. Raised as blocking in three consecutive + /// review rounds before the deferral stopped being defensible -- the panel + /// already had an error-display idiom, so "it needs UI plumbing" was wrong. + /// + /// Forced by pointing the data dir at a FILE: `create_dir_all` then cannot + /// create the parent, which is the first fallible step. + #[test] + fn a_save_that_cannot_happen_returns_an_error() { + let tmp = TempDir::new().unwrap(); + let blocker = tmp.path().join("not-a-dir"); + std::fs::write(&blocker, b"x").expect("seed"); + + let e = save( + &blocker, + &h(0x99), + &[CheatEntry { + code: "AAAAAA".into(), + enabled: true, + }], + &[], + ) + .expect_err("saving under a regular file must fail, not print and return"); + assert!( + matches!( + e.kind(), + std::io::ErrorKind::NotADirectory | std::io::ErrorKind::AlreadyExists + ), + "unexpected kind {:?}; the point is that SOME error reaches the caller", + e.kind() + ); + } } diff --git a/crates/rustynes-frontend/src/debugger/cheat_panel.rs b/crates/rustynes-frontend/src/debugger/cheat_panel.rs index b8dd778b..565dc154 100644 --- a/crates/rustynes-frontend/src/debugger/cheat_panel.rs +++ b/crates/rustynes-frontend/src/debugger/cheat_panel.rs @@ -80,6 +80,14 @@ pub struct CheatPanelState { enc_result: String, /// Last encoder error (cleared on a successful encode). enc_error: String, + /// Last cheat-file save error (cleared on a successful save). + /// + /// Shown in the panel rather than printed. `cheats::save` used to write this + /// to `stderr`, which nobody reads on a windowed build — so a save that + /// failed looked exactly like one that worked, and the user lost their cheat + /// list for that ROM without a signal. Same defect class as the swallowed + /// latency-config save fixed in #411. + save_error: String, /// v1.8.9 / v2.1.3 — the loaded ROM's category-grouped DB codes, cached so /// the pick-list does not re-query + sort + group the database every frame. /// Keyed on the ROM's CRC *set* (`Vec` — the header-excluded + full-file @@ -109,6 +117,10 @@ impl CheatPanelState { self.enc_compare_text.clear(); self.enc_result.clear(); self.enc_error.clear(); + // A save error names the PREVIOUS ROM's cheat file, so carrying it across + // a ROM change would report a failure against a game it never touched -- + // the stale-panel-state seam this project has hit three times. + self.save_error.clear(); } /// The currently-ENABLED raw RAM cheats, cloned for the app's produce @@ -210,6 +222,18 @@ pub fn show( fn body(ui: &mut egui::Ui, state: &mut CheatPanelState, rom_crcs: &[u32]) -> bool { let mut changed = false; + // A failed save is reported FIRST, above the lists it failed to persist. It + // used to go to `stderr`, which nobody reads on a windowed build -- so the + // panel showed a cheat list that looked saved and was not. Placed at the top + // rather than beside the add-fields because it is not about any one edit: it + // says the whole list on screen is not on disk. + if !state.save_error.is_empty() { + ui.colored_label( + egui::Color32::from_rgb(0xE0, 0x40, 0x40), + state.save_error.clone(), + ); + } + ui.horizontal(|ui| { let r = ui.add( egui::TextEdit::singleline(&mut state.add_text) @@ -665,8 +689,10 @@ fn resync_nes(state: &mut CheatPanelState, nes: &mut Nes) { /// Persist the in-memory cheat lists (Game Genie + raw RAM) to the per-ROM /// file (native only). #[cfg(not(target_arch = "wasm32"))] -fn persist_cheats(state: &CheatPanelState, persist: Option<&CheatPersist>) { - if let Some(p) = persist { - crate::cheats::save(&p.data_dir, &p.rom_sha256, &state.cheats, &state.raw); +fn persist_cheats(state: &mut CheatPanelState, persist: Option<&CheatPersist>) { + let Some(p) = persist else { return }; + match crate::cheats::save(&p.data_dir, &p.rom_sha256, &state.cheats, &state.raw) { + Ok(()) => state.save_error.clear(), + Err(e) => state.save_error = format!("cheats not saved: {e}"), } } diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 0af20131..14b0686d 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -772,8 +772,15 @@ if [ -n "$prior_id" ] && [ -s "$prior_body_file" ]; then exit 1 fi - if gh api -X PATCH "repos/${REPO}/issues/comments/${prior_id}" \ - -f body="$(cat "$body_file")" >/dev/null 2>&1; then + # The body goes through STDIN as JSON, never through argv. At `MAX_BODY_BYTES` + # the comment can approach 60 KB, and a single execve argument is capped at + # `MAX_ARG_STRLEN` (128 KB on Linux) -- close enough that a future raise of that + # bound would start failing with E2BIG, and the failure would look like a + # GitHub error rather than a local limit. `--rawfile` also makes the value a + # JSON string by construction, so no shell quoting or `-F` type-coercion can + # reinterpret a body that happens to look like a number or a boolean. + if jq -n --rawfile b "$body_file" '{body: $b}' \ + | gh api -X PATCH "repos/${REPO}/issues/comments/${prior_id}" --input - >/dev/null 2>&1; then log "updated review comment ${prior_id} on ${REPO}#${PR} (earlier rounds archived in place)" rm -f "$prior_body_file" exit 0 From f3eac61044129646fd2a4545cd3a9545eb8d48d0 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 12:25:24 -0400 Subject: [PATCH 18/20] docs(core): the timeline counter is session-local, and a test that says so Round seven found no blocking issues. Its useful finding was a nitpick: the timeline counter's documentation explained the whole design and never answered whether a save state carries it. It does not, and both ways of getting that wrong are silent. The counter is not written by `snapshot`, not read by `restore`, and a loaded state does not carry its own value across. A restore instead ADVANCES the live counter, which is the correct reading of the event -- the timeline you were on has been replaced -- and is true regardless of which state was loaded. Serializing it would break two things without any visible symptom. Loading the same slot twice would restore the same generation twice, so a consumer comparing against its last-seen value would miss the second load entirely. And a value from another session says nothing about this one: the counter is only meaningful against the previous value THIS process observed, which is why the accessor already documents that comparing it across two `Nes` instances is meaningless. Because it lives outside the snapshot, `snapshot_schema_audit` cannot see it -- the very property that makes the design correct also means nothing mechanical would notice the reasoning being invalidated. So the behaviour is pinned by a test instead: a restore advances it, a SECOND restore of the SAME slot advances it again (the assertion that fails if it were ever serialized), and a fresh `Nes` restored from that state counts its own restores rather than inheriting a stored value. THREE SUGGESTIONS WERE ALREADY IMPLEMENTED Recorded because re-raising them is cheap and re-verifying them is not. The symlink resolver already has a hard cap. It is 40, matching Linux's MAXSYMLINKS, and `a_symlink_cycle_terminates` covers `a -> b -> a`. Round five of this same review asked for the kernel's number where I had 8; round seven suggests 8 or 16. Holding at kernel parity, for the reason round five gave: where the kernel resolves a chain and this function gives up, the two disagree about where the file IS. The comment archive is already bounded. MAX_BODY_BYTES is 60000 against GitHub's 65536, oldest rounds drop first, and the count of dropped rounds is printed in the body rather than truncating silently. The scratch file is already cleaned up on an early return via `?`. The `?` returns from the CLOSURE, not from `write_atomic`, so `write_result` is `Err` and its cleanup runs. `a_failed_write_leaves_the_original_intact` now asserts no `.tmp` survives -- and its comment states precisely which branch that covers, because a mutation showed it reaches the RENAME-failure cleanup rather than the write-failure one. Removing the branch it does reach is caught; removing the other is not, and the test says so rather than being read as covering both. The two `skip_serializing_if` fields were checked rather than assumed: both carry `#[serde(default)]`, so a config written before them still loads. `rustynes-core` changes, so the accuracy gates are VERIFIED rather than asserted: AccuracyCoin 141/141 (100.00%, RAM decoder; the framebuffer decoder reports 120 and is known-buggy) and nestest 0-diff. Full clippy matrix, both wasm32 combinations, no_std thumbv7em, rustdoc with warnings as errors. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- crates/rustynes-core/src/nes.rs | 70 ++++++++++++++++++++ crates/rustynes-frontend/src/atomic_write.rs | 30 +++++++++ 2 files changed, 100 insertions(+) diff --git a/crates/rustynes-core/src/nes.rs b/crates/rustynes-core/src/nes.rs index e2e3142e..fff7935a 100644 --- a/crates/rustynes-core/src/nes.rs +++ b/crates/rustynes-core/src/nes.rs @@ -190,6 +190,26 @@ pub struct Nes { /// site, which matters because two of the four timeline jumps (wasm /// load-state, and rewind) are not reachable from a patchable frontend call /// site at all. + /// + /// # It is NOT part of the save state, deliberately + /// + /// The counter is session-local: it is not written by `snapshot`, not read by + /// `restore`, and a loaded state does not carry its own value across. What a + /// restore does instead is **increment the live counter**, which is the + /// correct reading of the event — "the timeline you were on has been replaced" + /// — and is true regardless of which state was loaded. + /// + /// Serializing it would be actively wrong in two ways. Loading the same slot + /// twice would restore the same generation twice, so a consumer comparing + /// against its last-seen value would miss the second load entirely. And a + /// value from another session says nothing about this one: the counter is only + /// ever meaningful as a comparison against the previous value *this process* + /// observed, which is why `timeline_generation()` documents that comparing it + /// across two `Nes` instances is meaningless. + /// + /// A consequence worth stating: because it lives outside the snapshot, the + /// `snapshot_schema_audit` test cannot see it, so nothing mechanical will + /// notice if this reasoning is ever invalidated. timeline_generation: u64, } @@ -4559,4 +4579,54 @@ mod tests { nes.set_zapper_temporal_light(true); assert!(nes.zapper_temporal_light()); } + + /// The timeline counter is session-local: a save state neither carries it nor + /// restores it, and loading one ADVANCES the live counter instead. + /// + /// Documented on the field, and untestable by `snapshot_schema_audit` for the + /// very reason that makes it true -- the counter lives outside the snapshot, + /// so that audit cannot see it. Pinned here instead, because the two ways + /// serializing it would be wrong are both silent: loading the same slot twice + /// would restore the same generation twice and a consumer would miss the + /// second load, and a value from another session means nothing in this one. + #[test] + fn the_timeline_counter_is_session_local_and_advances_on_restore() { + let rom = synth_nrom(16, 8); + let mut nes = Nes::from_rom(&rom).expect("parse"); + nes.run_frame(); + let state = nes.snapshot(); + let before = nes.timeline_generation(); + + nes.restore(&state).expect("restore"); + let after_first = nes.timeline_generation(); + assert!( + after_first > before, + "a restore must advance the timeline counter: {before} -> {after_first}" + ); + + // The SECOND load of the SAME slot must advance it again. This is the + // assertion that would fail if the counter were serialized -- the restored + // value would be identical both times and a consumer comparing against its + // last-seen value would never notice the second load. + nes.restore(&state).expect("restore again"); + assert!( + nes.timeline_generation() > after_first, + "reloading the same slot must still register as a new timeline" + ); + + // And a snapshot taken now must not encode it: a fresh `Nes` restored from + // this state starts its own count rather than adopting ours. + let mut fresh = Nes::from_rom(&rom).expect("parse"); + assert_eq!( + fresh.timeline_generation(), + 0, + "a fresh Nes must start at zero" + ); + fresh.restore(&nes.snapshot()).expect("restore into fresh"); + assert_eq!( + fresh.timeline_generation(), + 1, + "the fresh instance must count its own restores, not inherit a stored value" + ); + } } diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs index 1ddd1f88..a7ddfac5 100644 --- a/crates/rustynes-frontend/src/atomic_write.rs +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -1197,6 +1197,36 @@ mod tests { fs::create_dir(&dir_target).expect("mkdir"); assert!(write_atomic(&dir_target, b"nope").is_err()); assert_eq!(fs::read(&p).unwrap(), b"good"); + + // ...and it leaves NO scratch file behind. + // + // Scope, stated precisely, because a mutation established it: this reaches + // the RENAME-failure cleanup, not the write-failure one. The scratch create, + // write and `fsync` all succeed here; it is renaming a file over a directory + // that fails. Removing that cleanup is caught; removing the write-failure + // cleanup is NOT, and this test should not be read as covering it. + // + // Review suggested a `Drop` guard on the grounds that an early return via + // `?` orphans the temporary. It does not: the `?` returns from the CLOSURE, + // so `write_result` is `Err` and its cleanup runs — the same shape as the + // branch asserted here. Only a PANIC can orphan a scratch file, and the + // bounded scratch loop exists so that is survivable. + // Matched on OUR scratch prefix rather than the `.tmp` extension: it is + // what `scratch_name` actually produces, it cannot collide with a `.tmp` + // this test did not create, and it sidesteps clippy's case-sensitivity + // lint on extension comparisons -- which is a fair complaint about + // matching an extension and irrelevant to matching a generated name. + let ours = format!(".{}.", std::process::id()); + let strays: Vec<_> = fs::read_dir(&d) + .expect("read tempdir") + .filter_map(Result::ok) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.contains(&ours)) + .collect(); + assert!( + strays.is_empty(), + "a failed write orphaned scratch file(s): {strays:?}" + ); } // ---- the retry loop, exercised on every platform ---- From 75beb7c53a7847e0f8b0249bdc7c1289cb5eb650 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 12:33:00 -0400 Subject: [PATCH 19/20] fix(ci): a
in a review body could tear the round it lives in Round eight, and the blocking finding is a data-corruption bug in code from round two of this same PR. `agy_drop_oldest_round` located the oldest archived round by matching `/^
$/` -- the tag itself. A review body legitimately contains `
` blocks: folded logs, collapsed code, another bot's summary, and the archived rounds are themselves nested `
`. So the cut could land INSIDE a round, leaving torn HTML and half a review in a comment nobody would think to check. It is now delimited by `AGY_ROUND_MARK`, an HTML comment the writer emits ahead of each round. Invisible when rendered, and it cannot occur by accident in prose the way a tag can. This is the same mechanism the archive boundaries already used, and the same mechanism this PR adopted from SLAC one commit earlier for exactly this reason -- that a `sed` range matching a declaration's own syntax truncates silently. I used markers for the outer boundaries and a naive regex for the inner ones in the same file, which is the kind of half-application that reads as consistent until someone tries it with real content. FAIL-SAFE FOR ARCHIVES ALREADY IN THE WILD An archive written by the previous version has no round markers. With none present the function exits non-zero and NOTHING is dropped, so the edit fails on size -- recoverable, visible, and repaired by the next round -- rather than the archive being silently mangled. Pinned by its own check. Three checks and two mutations. The nested case is the one that matters: a round carrying its own `
` must survive the oldest being dropped, and restoring the tag-matching form fails it. Also asserted: the writer actually EMITS the sentinel, because if it did not, every archive would look legacy-shaped, the trim would silently never fire, and the comment would grow until the edit failed. ALSO FROM THIS ROUND `cheats::save` no longer calls `create_dir_all` before `write_atomic`, which creates the parent itself. Doing it twice meant a failure surfaced with one function's path context or the other's depending on which won -- the same redundancy already removed from `save_state.rs`. Swept to the canonical template and the four sibling installs; selftest passes in all five. Full clippy matrix, both wasm32 combinations, no_std thumbv7em, rustdoc with warnings as errors. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- crates/rustynes-frontend/src/cheats.rs | 6 ++--- scripts/_agy_comment_body.sh | 22 ++++++++++++++---- scripts/agy-review-selftest.sh | 32 ++++++++++++++++++++++++-- scripts/agy-review.sh | 1 + 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/crates/rustynes-frontend/src/cheats.rs b/crates/rustynes-frontend/src/cheats.rs index 07269803..171631ca 100644 --- a/crates/rustynes-frontend/src/cheats.rs +++ b/crates/rustynes-frontend/src/cheats.rs @@ -162,9 +162,9 @@ pub fn save( raw: &[RawCheat], ) -> std::io::Result<()> { let path = cheat_path(data_dir, rom_sha256); - if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; - } + // No `create_dir_all` here: `write_atomic` creates the parent itself, and doing + // it twice meant a failure surfaced with one function's path context or the + // other's depending on which won. Removed on review, matching `save_state.rs`. let file = CheatFile { cheats: genie.to_vec(), raw: raw.to_vec(), diff --git a/scripts/_agy_comment_body.sh b/scripts/_agy_comment_body.sh index 4997864f..5668f9ce 100755 --- a/scripts/_agy_comment_body.sh +++ b/scripts/_agy_comment_body.sh @@ -47,11 +47,25 @@ agy_body_archive() { $0 == s { inside = 1 }' || true } -# Drop the OLDEST archived round -- the last `
` block. Exits non-zero when there is -# none left to drop, so a caller trimming to a size limit terminates rather than spinning. +# Delimits one archived round. Emitted by the caller ahead of each round's `
`. +# +# A dedicated sentinel, NOT the `
` tag itself. Matching `/^
$/` looked +# equivalent and was a data-corruption bug: a review body legitimately contains `
` +# blocks -- folded logs, collapsed code, another bot's summary, and the archived rounds are +# themselves nested `
` -- so the cut could land INSIDE a round and leave torn HTML +# plus half a review. The sentinel is an HTML comment, so it is invisible when rendered and +# cannot occur by accident in prose the way a tag can. Found in review. +AGY_ROUND_MARK='' + +# Drop the OLDEST archived round -- everything from the last round marker onward. +# +# Exits non-zero when there is no marker to cut at, so a caller trimming to a size limit +# terminates rather than spinning. That is also the fail-safe direction for an archive written +# by an older version of this script: with no markers present, nothing is dropped and the edit +# simply fails on size, rather than the archive being silently mangled. agy_drop_oldest_round() { - awk ' - /^
$/ { starts[++n] = NR } + awk -v m="$AGY_ROUND_MARK" ' + $0 == m { starts[++n] = NR } { line[NR] = $0 } END { cut = (n > 0) ? starts[n] : 0 diff --git a/scripts/agy-review-selftest.sh b/scripts/agy-review-selftest.sh index 9851e416..decbda30 100755 --- a/scripts/agy-review-selftest.sh +++ b/scripts/agy-review-selftest.sh @@ -188,9 +188,37 @@ check "the head never carries the marker into the archive" "" \ # Trimming must terminate: with no `
` left, dropping fails rather than looping. check "dropping from an empty archive fails rather than spinning" "1" \ "$(printf 'no rounds here\n' | agy_drop_oldest_round >/dev/null 2>&1; echo $?)" + +# An archive written by an OLDER version has no round markers. Nothing must be dropped: +# the edit then fails on size, which is recoverable, rather than the archive being mangled. +check "an unmarked (legacy) archive drops nothing" "1" \ + "$(printf '
\nNEW\n
\n
\nOLD\n
\n' \ + | agy_drop_oldest_round >/dev/null 2>&1; echo $?)" + +marked_archive="$(printf '%s\n
\nNEW\n
\n%s\n
\nOLD\n
\n' \ + "$AGY_ROUND_MARK" "$AGY_ROUND_MARK")" check "dropping removes the OLDEST round, keeping the newest" \ - "$(printf '
\nNEW\n
')" \ - "$(printf '
\nNEW\n
\n
\nOLD\n
\n' | agy_drop_oldest_round)" + "$(printf '%s\n
\nNEW\n
' "$AGY_ROUND_MARK")" \ + "$(printf '%s\n' "$marked_archive" | agy_drop_oldest_round)" + +# THE BUG THIS SENTINEL EXISTS FOR. A review body legitimately contains `
` blocks +# -- folded logs, collapsed code, another bot's summary -- and matching the tag itself cut +# INSIDE a round, leaving torn HTML and half a review. The newest round below carries its own +# `
`; dropping the oldest must not touch it. +nested="$(printf '%s\n
\nRound\n
\nfolded log\n
\nfinding\n
\n%s\n
\nOLD\n
\n' \ + "$AGY_ROUND_MARK" "$AGY_ROUND_MARK")" +check "a
INSIDE a round is not mistaken for a round boundary" \ + "$(printf '%s\n
\nRound\n
\nfolded log\n
\nfinding\n
' "$AGY_ROUND_MARK")" \ + "$(printf '%s\n' "$nested" | agy_drop_oldest_round)" + +# The writer must actually EMIT the sentinel, or every archive is legacy-shaped and the trim +# silently never fires -- the comment would then grow until the edit fails. +if grep -q 'AGY_ROUND_MARK' "$SCRIPT_DIR/agy-review.sh"; then + echo " ok agy-review.sh emits the round sentinel" +else + echo " FAIL agy-review.sh never emits AGY_ROUND_MARK; the archive would never trim" + fails=$((fails + 1)) +fi # The script must not delete comments any more. A reintroduced DELETE is the regression that # would silently restore the destructive behaviour this design replaced. diff --git a/scripts/agy-review.sh b/scripts/agy-review.sh index 14b0686d..fb7fc862 100755 --- a/scripts/agy-review.sh +++ b/scripts/agy-review.sh @@ -731,6 +731,7 @@ if [ -n "$prior_id" ] && [ -s "$prior_body_file" ]; then archived_file="$(mktemp)" { + printf '%s\n' "$AGY_ROUND_MARK" printf '
\nRound reviewed at %s\n\n' \ "$(date -u +'%Y-%m-%d %H:%M UTC')" printf '%s\n' "$prior_head" From 7eb0a3e24ce7c9738d0bbc3c80de81936b682f1d Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Thu, 20 Aug 2026 12:43:22 -0400 Subject: [PATCH 20/20] fix(frontend): a rename budget that outlasts a scan lock, and the assertion that pins it Round nine found no blocking issues. Two of its suggestions were worth acting on, and getting the first one under test exposed two holes -- one of which I had just created. THE RETRY BUDGET Review reports that Windows Defender can hold a lock on a newly written file for longer than the 150 ms the retry loop waited, so the retry would exhaust before the lock cleared and the save would fail for a reason that resolves itself a moment later. Eight attempts now give 10+20+40+80+160+320+640 = 1270 ms. THAT TIMING CLAIM IS NOT ONE THIS PROJECT CAN VERIFY. There is no Windows runner in the PR matrix and no measurement behind it here. It was adopted anyway because the trade is one-sided: the cost is a longer stall on a save that is ALREADY FAILING, and the benefit is not losing a save the user believes happened. The comment says so, and names itself as the place a real number belongs if anyone measures one. TWO HOLES, BOTH FOUND BY MUTATION Raising the count made the exhaustion test really sleep 1270 ms -- measurably, 0.15 s to 1.27 s for the module. The base backoff is now a parameter and tests pass 0, which changes nothing they assert: the loop's contract is the attempt COUNT and the propagated error, not wall time. That injection created a hole. A mutation zeroing the PRODUCTION backoff came back NOT CAUGHT, and it is not a weak test: on Unix `is_transient_rename_error` is always false, so the loop never sleeps and there is nothing to observe on the platform CI runs. Only a Windows run could see it. Declared in the module's existing "what the tests do NOT cover" section rather than papered over. The second hole was already there. `assert_eq!(calls, RENAME_ATTEMPTS)` pins NOTHING -- both sides move together, so reducing the constant satisfies it while silently halving the budget, and that mutation came back NOT CAUGHT. The property is now asserted through a derived `total_backoff_ms()`: at least 500 ms in total, which is the requirement the count was chosen to meet rather than a restatement of the count. The mutation is caught. `total_backoff_ms` is `#[cfg(test)]`: nothing in the shipped path needs the sum, the loop sleeps per attempt. A derivation of two production constants that exists so a test can assert what they were chosen for is a fine reason for a test-only item and a poor reason to keep dead code in the binary. THE PANEL ERROR IS WRAPPED, NOT TRUNCATED An OS-level I/O error can be long, and an unwrapped label widens the window to fit it -- the panel's other error fields carry short messages this crate authored, so they never showed the problem. Wrapped rather than truncated because the tail of an I/O error is usually the part naming the actual cause. THREE SUGGESTIONS DECLINED, WITH REASONS The archive cap is already implemented -- MAX_BODY_BYTES against GitHub's 65536, oldest dropped first, count announced. Third round it has been raised. Documenting the `timeline_generation` serde trap: already done in the previous commit, and the premise is wrong. There is no `#[serde(skip)]`; `rustynes-core`'s `nes.rs` contains zero `serde` at all -- the snapshot is a hand-written binary format. The field is excluded by not being written, which is what the new documentation and its test describe. A startup sweep of orphaned scratch files: declined on a hazard, not on effort. Two RustyNES instances can run at once, and the sweeper cannot tell a dead run's orphan from a live run's in-flight scratch file without checking whether the pid is alive -- which is racy and platform-specific. Deleting a live instance's scratch mid-write is a worse failure than the clutter it cleans. The bounded scratch loop already makes orphans survivable. Logging the excused directory-fsync errnos: this crate has no logger. Adding a logging dependency for one debug line is not the trade today, and the errnos are named in the code with the reason each is excused. Gates: fmt, workspace clippy, the four frontend feature combos, both wasm32 combinations, the no_std thumbv7em build, rustdoc with warnings as errors, and 580 frontend tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj --- crates/rustynes-frontend/src/atomic_write.rs | 100 +++++++++++++++--- .../src/debugger/cheat_panel.rs | 14 ++- 2 files changed, 97 insertions(+), 17 deletions(-) diff --git a/crates/rustynes-frontend/src/atomic_write.rs b/crates/rustynes-frontend/src/atomic_write.rs index a7ddfac5..c798af88 100644 --- a/crates/rustynes-frontend/src/atomic_write.rs +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -113,6 +113,14 @@ //! umask default. A test can only observe the end state, so the window is //! invisible to it by construction. //! +//! - **The production backoff wiring (`rename_with_retry` passing +//! `RENAME_BACKOFF_MS` rather than `0`).** Zeroing it is invisible here, and not +//! because the test is weak: on Unix `is_transient_rename_error` is always +//! `false`, so the loop never sleeps at all and there is nothing to observe on +//! the platform CI runs. Only a Windows run could see it. The *duration* is +//! pinned instead, through `total_backoff_ms`, which is the half that carries +//! the actual decision. +//! //! Both are kept for the reason they were added, and neither should be removed on //! the evidence that "no test fails". That inference is the failure this project //! has been bitten by more than once; an untested property is not an unnecessary @@ -193,15 +201,30 @@ const SCRATCH_ATTEMPTS: u32 = 8; /// How many times to attempt the rename before giving up. /// -/// Only ever more than one on Windows (see `is_transient_rename_error`). Five -/// attempts sleep FOUR times — the loop returns on the fifth failure rather than -/// backing off after it — so the worst case is 10 + 20 + 40 + 80 = **150 ms**, -/// which is a long time in a UI frame and a short one against losing a save the -/// user believes happened. (This read 310 ms until review; that would be the -/// figure if a fifth sleep of 160 ms happened, and it does not.) -const RENAME_ATTEMPTS: u32 = 5; - -/// Base backoff between rename attempts, doubled each time: 10, 20, 40, 80, 160. +/// Only ever more than one on Windows (see `is_transient_rename_error`). +/// +/// `N` attempts sleep `N - 1` times — the loop returns on the last failure rather +/// than backing off after it — so eight attempts give +/// 10 + 20 + 40 + 80 + 160 + 320 + 640 = **1270 ms** of total backoff. +/// +/// It was five attempts (150 ms) until review pointed out that Windows Defender +/// can hold a lock on a newly written file for longer than that while it scans, +/// so the retry would exhaust before the lock cleared and the save would fail for +/// a reason that resolves itself a moment later. +/// +/// **That timing claim is not one this project can verify** — there is no Windows +/// runner in the PR matrix and no measurement behind it here. It was adopted +/// anyway because the direction is safe and the trade is one-sided: the cost is a +/// longer stall on a save that is *already failing*, and the benefit is not losing +/// a save the user believes happened. If it is ever measured, this comment is the +/// place the number belongs. +/// +/// (The doc read 310 ms until an earlier round — that would be the figure if a +/// fifth sleep of 160 ms happened, and it did not.) +const RENAME_ATTEMPTS: u32 = 8; + +/// Base backoff between rename attempts, doubled each time: 10, 20, 40, 80, 160, +/// 320, 640. /// /// The `sleep` this drives is unreachable off Windows, which matters on **wasm** /// specifically: `std::thread::sleep` cannot block on `wasm32-unknown-unknown`. @@ -209,6 +232,25 @@ const RENAME_ATTEMPTS: u32 = 5; /// every non-Windows target, so the loop returns on the first error. const RENAME_BACKOFF_MS: u64 = 10; +/// Total time the retry loop will wait before giving up, in milliseconds. +/// +/// Derived rather than written down, so it cannot drift from the two constants it +/// summarises. Exists to be ASSERTED: the reason for the current attempt count is +/// a required total (long enough to outlast a Windows Defender scan lock), and a +/// test comparing an attempt count against `RENAME_ATTEMPTS` pins nothing — both +/// sides move together. A mutation reducing the count to its old value came back +/// NOT CAUGHT for exactly that reason. +/// `#[cfg(test)]` because nothing in the shipped path needs the sum — the loop +/// sleeps per attempt and never asks for the total. It is a derivation of two +/// production constants that exists so a test can assert the property they were +/// chosen for, which is a legitimate reason for a test-only item and a poor +/// reason to keep dead code in the binary. +#[cfg(test)] +const fn total_backoff_ms() -> u64 { + // N attempts sleep N-1 times, doubling: base * (2^(N-1) - 1). + RENAME_BACKOFF_MS * ((1u64 << (RENAME_ATTEMPTS - 1)) - 1) +} + /// Resolve a symlinked target to the file it points at. /// /// `fs::write` follows a symlink and writes through to its target; `fs::rename` @@ -339,7 +381,14 @@ fn is_windows_sharing_violation(e: &io::Error) -> bool { /// /// Propagates the final error when the attempts are exhausted rather than /// reporting success or falling silent. -fn rename_with_retry_using(mut op: F, transient: P) -> io::Result<()> +/// +/// `base_backoff_ms` is a parameter for a duller reason than the other two: the +/// exhaustion test drives the loop to its limit, and with the production value it +/// really sleeps the full 1270 ms — measurably, it took the module's suite from +/// 0.15 s to 1.27 s. Tests pass `0`, which changes nothing they assert (the loop's +/// contract is the attempt COUNT and the propagated error, not the wall time) and +/// gives the suite its second back. +fn rename_with_retry_using(mut op: F, transient: P, base_backoff_ms: u64) -> io::Result<()> where F: FnMut() -> io::Result<()>, P: Fn(&io::Error) -> bool, @@ -353,9 +402,11 @@ where if attempt >= RENAME_ATTEMPTS || !transient(&e) { return Err(e); } - std::thread::sleep(std::time::Duration::from_millis( - RENAME_BACKOFF_MS << (attempt - 1), - )); + if base_backoff_ms > 0 { + std::thread::sleep(std::time::Duration::from_millis( + base_backoff_ms << (attempt - 1), + )); + } } } } @@ -363,7 +414,11 @@ where /// `fs::rename`, retried past a transient Windows sharing violation. fn rename_with_retry(from: &Path, to: &Path) -> io::Result<()> { - rename_with_retry_using(|| fs::rename(from, to), is_transient_rename_error) + rename_with_retry_using( + || fs::rename(from, to), + is_transient_rename_error, + RENAME_BACKOFF_MS, + ) } /// Apply `mode` to `path` through `op`. @@ -1240,6 +1295,7 @@ mod tests { Ok(()) }, is_transient_rename_error, + 0, ) .expect("should succeed"); assert_eq!(calls, 1, "a successful rename must not be retried"); @@ -1259,6 +1315,7 @@ mod tests { Err(io::Error::new(io::ErrorKind::NotFound, "gone")) }, |e| e.kind() == io::ErrorKind::PermissionDenied, + 0, ); assert!(r.is_err()); assert_eq!(calls, 1, "a NotFound rename must not be retried"); @@ -1279,6 +1336,7 @@ mod tests { Err(io::Error::new(io::ErrorKind::PermissionDenied, "busy")) }, |_| true, + 0, ); let e = r.expect_err("exhausting the attempts must not report success"); assert_eq!(e.kind(), io::ErrorKind::PermissionDenied); @@ -1286,6 +1344,19 @@ mod tests { calls, RENAME_ATTEMPTS, "the loop must make exactly RENAME_ATTEMPTS attempts before giving up" ); + + // ...and the attempt count must still buy the total wait it exists for. + // Asserting `calls == RENAME_ATTEMPTS` alone pins NOTHING: both sides move + // together, so reducing the constant satisfies it while silently halving + // the time the retry outlasts. Established by mutation — that change came + // back NOT CAUGHT until this assertion existed. + assert!( + total_backoff_ms() >= 500, + "the retry must wait at least 500 ms in total before giving up \ + (a Windows Defender scan lock can outlast a shorter budget); \ + {RENAME_ATTEMPTS} attempts at {RENAME_BACKOFF_MS} ms give {} ms", + total_backoff_ms() + ); } /// Unix must make exactly ONE attempt, with the REAL predicate -- the @@ -1301,6 +1372,7 @@ mod tests { Err(io::Error::new(io::ErrorKind::PermissionDenied, "busy")) }, is_transient_rename_error, + 0, ); assert!(r.is_err()); assert_eq!(calls, 1, "POSIX rename has no transient sharing violation"); diff --git a/crates/rustynes-frontend/src/debugger/cheat_panel.rs b/crates/rustynes-frontend/src/debugger/cheat_panel.rs index 565dc154..0ee3536a 100644 --- a/crates/rustynes-frontend/src/debugger/cheat_panel.rs +++ b/crates/rustynes-frontend/src/debugger/cheat_panel.rs @@ -228,9 +228,17 @@ fn body(ui: &mut egui::Ui, state: &mut CheatPanelState, rom_crcs: &[u32]) -> boo // rather than beside the add-fields because it is not about any one edit: it // says the whole list on screen is not on disk. if !state.save_error.is_empty() { - ui.colored_label( - egui::Color32::from_rgb(0xE0, 0x40, 0x40), - state.save_error.clone(), + // Wrapped, not truncated. An OS-level I/O error can be long, and an + // unwrapped label widens the window to fit it — the panel's other error + // fields carry short messages this crate authored, so they never showed + // the problem. Truncating would be worse than wrapping here: the tail of + // an I/O error is usually the part naming the actual cause. + ui.add( + egui::Label::new( + egui::RichText::new(state.save_error.clone()) + .color(egui::Color32::from_rgb(0xE0, 0x40, 0x40)), + ) + .wrap(), ); }