diff --git a/.github/workflows/antigravity-review.yml b/.github/workflows/antigravity-review.yml index 34e7f8a6..4a5abbbc 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 @@ -95,5 +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: | + # 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/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 >>> diff --git a/CHANGELOG.md b/CHANGELOG.md index 68f11259..7d07b78a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,151 @@ 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**. + + **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. + + 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. + + 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 + `#[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 + 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 + 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 + 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 +195,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 diff --git a/crates/rustynes-core/src/nes.rs b/crates/rustynes-core/src/nes.rs index 0b2fda5b..fff7935a 100644 --- a/crates/rustynes-core/src/nes.rs +++ b/crates/rustynes-core/src/nes.rs @@ -173,9 +173,59 @@ 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. + /// + /// # 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, } 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 +293,7 @@ impl Nes { exec_log: Vec::new(), #[cfg(feature = "debug-hooks")] exec_logging: false, + timeline_generation: 0, }) } @@ -282,6 +333,7 @@ impl Nes { exec_log: Vec::new(), #[cfg(feature = "debug-hooks")] exec_logging: false, + timeline_generation: 0, }) } @@ -350,6 +402,7 @@ impl Nes { exec_log: Vec::new(), #[cfg(feature = "debug-hooks")] exec_logging: false, + timeline_generation: 0, }) } @@ -411,6 +464,7 @@ impl Nes { exec_log: Vec::new(), #[cfg(feature = "debug-hooks")] exec_logging: false, + timeline_generation: 0, }) } @@ -488,12 +542,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 +2138,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 +3494,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); @@ -4388,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 new file mode 100644 index 00000000..c798af88 --- /dev/null +++ b/crates/rustynes-frontend/src/atomic_write.rs @@ -0,0 +1,1444 @@ +//! 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. +//! +//! - **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 +//! 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. +//! +//! # 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; +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 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`). +/// +/// `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`. +/// 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; + +/// 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` +/// 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; + } + // 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. +/// +/// 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? +/// +/// 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 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. +/// +/// 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. +/// +/// `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, +{ + let mut attempt: u32 = 0; + loop { + match op() { + Ok(()) => return Ok(()), + Err(e) => { + attempt += 1; + if attempt >= RENAME_ATTEMPTS || !transient(&e) { + return Err(e); + } + if base_backoff_ms > 0 { + std::thread::sleep(std::time::Duration::from_millis( + base_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, + RENAME_BACKOFF_MS, + ) +} + +/// 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 +/// 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 **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. A post-rename sync failure is wrapped so its message says +/// 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) +} + +/// `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 +/// 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`). +/// +/// 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 +/// 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, + 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() + && !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: Option = None; + + 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); + } + // 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. + // + // `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 { + // 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. + // + // 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 + && 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) { + let _ = fs::remove_file(&tmp); + return Err(e); + } + + 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. +/// +/// 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) -> io::Result<()> { + 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(()), + 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; + // `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. +/// +/// 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. +/// +/// 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))] +#[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 { + 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"); + } + + /// 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 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"); + } + + /// 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* + /// 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 + /// 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(), sync_parent_dir) + .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() { + 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"); + + // ...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 ---- + + #[test] + fn the_retry_loop_returns_immediately_on_success() { + let mut calls = 0; + rename_with_retry_using( + || { + calls += 1; + Ok(()) + }, + is_transient_rename_error, + 0, + ) + .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, + 0, + ); + 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, + 0, + ); + 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" + ); + + // ...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 + /// 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, + 0, + ); + 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" + ); + 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); + assert!( + !directory_fsync_is_unsupported(&e), + "{name} must PROPAGATE -- it is a real write failure, not an \ + unsupported barrier" + ); + } + } +} diff --git a/crates/rustynes-frontend/src/cheats.rs b/crates/rustynes-frontend/src/cheats.rs index 687b8ff8..171631ca 100644 --- a/crates/rustynes-frontend/src/cheats.rs +++ b/crates/rustynes-frontend/src/cheats.rs @@ -143,29 +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; - } + // 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(), }; - match toml::to_string_pretty(&file) { - Ok(s) => { - if let Err(e) = fs::write(&path, s) { - 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")))] @@ -212,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); @@ -229,7 +242,8 @@ mod tests { enabled: true, }], &[], - ); + ) + .expect("save"); save( tmp.path(), &h(0x02), @@ -238,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"); } @@ -284,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/config.rs b/crates/rustynes-frontend/src/config.rs index 4b61ecdd..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 @@ -2111,277 +2123,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. - // - // `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. - // - // 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.) + // Serialize, then hand the bytes to the shared atomic writer. // - // `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. + // 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. // - // `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 @@ -2461,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/debugger/cheat_panel.rs b/crates/rustynes-frontend/src/debugger/cheat_panel.rs index b8dd778b..0ee3536a 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,26 @@ 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() { + // 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(), + ); + } + ui.horizontal(|ui| { let r = ui.add( egui::TextEdit::singleline(&mut state.add_text) @@ -665,8 +697,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/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 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..81683183 100644 --- a/crates/rustynes-frontend/src/save_state.rs +++ b/crates/rustynes-frontend/src/save_state.rs @@ -92,10 +92,24 @@ 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))?; - } - fs::write(&path, state).map_err(|e| map_io(&path, 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 + // 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) } 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). diff --git a/docs/libretro/UPSTREAM_SYNC.md b/docs/libretro/UPSTREAM_SYNC.md index 4a7c6911..512838bf 100644 --- a/docs/libretro/UPSTREAM_SYNC.md +++ b/docs/libretro/UPSTREAM_SYNC.md @@ -136,3 +136,85 @@ 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 +``` + +### 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, 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. diff --git a/scripts/_agy_comment_body.sh b/scripts/_agy_comment_body.sh new file mode 100755 index 00000000..5668f9ce --- /dev/null +++ b/scripts/_agy_comment_body.sh @@ -0,0 +1,75 @@ +#!/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 +} + +# 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 -v m="$AGY_ROUND_MARK" ' + $0 == m { 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 old mode 100644 new mode 100755 index db684954..decbda30 --- a/scripts/agy-review-selftest.sh +++ b/scripts/agy-review-selftest.sh @@ -30,16 +30,56 @@ 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_STALE_JQ='/,/'\$/p" "$SCRIPT_DIR/agy-review.sh" \ - | sed "1s/^SELECT_STALE_JQ='//; \$s/'\$//" + 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)" [ -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 +91,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 +106,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 $?)" + +# 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 '%s\n
\nNEW\n
' "$AGY_ROUND_MARK")" \ + "$(printf '%s\n' "$marked_archive" | agy_drop_oldest_round)" -# Regression #1, pinned: an unknown id must not select everything. The caller now refuses to run -# the delete at all in this case, but the filter itself is checked so the two guards are -# independent rather than one relying on the other. -check "an id of 0 still excludes nothing real" "111 222 999" "$(select_ids 0)" +# 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)" -# A different id in the set behaves the same way, so the exclusion is genuinely by value. -check "excludes whichever id it is given" "222 999" "$(select_ids 111)" +# 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. +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. @@ -128,6 +242,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 104517b5..fb7fc862 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}" @@ -100,21 +155,33 @@ 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. +# >>> SELFTEST-EXTRACT: ours-comment filter +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 +# <<< SELFTEST-EXTRACT REPO="${GITHUB_REPOSITORY:?GITHUB_REPOSITORY not set}" @@ -155,6 +222,9 @@ diff_file= diff_err= meta_file= prompt_file= out_file= raw= body_file= agy_diff_ # 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() { @@ -544,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 )) @@ -561,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 @@ -600,65 +696,102 @@ 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 '%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" + 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" + 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 + # 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 + + # 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 + 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 - # 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