diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml index 2ac8d7b1..82f79a54 100644 --- a/.github/actionlint.yaml +++ b/.github/actionlint.yaml @@ -19,3 +19,27 @@ self-hosted-runner: # session, which is why that workflow cannot run on a hosted runner. See # `Local_Only-Projects/antigravity-pr-review/README.md` in the workspace. - agy + +# v2.3.9 — one suppression, scoped to one file and one message. +# +# `pgo.yml` carries two steps deliberately disabled with `if: false`, each with a +# written rationale above it: the BOLT bench and determinism gates were switched +# off because the harness benches the wrong binary, and "a gate that cannot +# measure its subject is worse than no gate". The steps are kept rather than +# deleted so the shape survives for whoever re-enables them behind a harness that +# benches the BOLT-optimized binary itself. +# +# actionlint's `if-cond` rule flags a constant condition, which is correct in +# general and wrong about intent here. +# +# THE COST, stated rather than buried: this silences the rule for the whole file, +# so a genuinely accidental `if: false` added to `pgo.yml` later would not be +# caught. actionlint has no line-scoped ignore, and the alternatives are worse — +# deleting the steps loses the documented shape, and rewriting the condition to +# something non-constant hides the intent from the reader to satisfy a linter. +# Every other workflow keeps the rule; verified by adding an `if: false` to +# `security.yml` and confirming it still fails. +paths: + .github/workflows/pgo.yml: + ignore: + - 'constant expression "false" in condition' diff --git a/.github/scripts/apt-install-retry.sh b/.github/scripts/apt-install-retry.sh new file mode 100755 index 00000000..a605640a --- /dev/null +++ b/.github/scripts/apt-install-retry.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# Install one apt package, bounded and retried. +# +# v2.3.9 A5b. The cross-compile gate provisions glibc headers for bindgen with a +# bare `apt-get update && apt-get install`. Both are network fetches with no +# timeout of their own, so when a mirror stalls the step hangs until the JOB +# timeout fires — 25 minutes for `libretro-cross` — and the run is reported as +# cancelled rather than as what it was. +# +# That is not hypothetical. During the v2.3.7 cut this hung four separate times +# across two PRs, always in a setup or provisioning step and never in a compile +# or test step: twice in `rust-setup`, once in the armhf provision, once in the +# aarch64 provision. Each cost 25-45 minutes and needed a manual re-run. The +# per-job `timeout-minutes` added in #400 bounded the damage correctly; nothing +# addressed the fragility underneath it. +# +# Two bounds, doing different jobs: +# +# * `timeout` per command, so a stalled fetch fails in minutes rather than +# consuming the job's entire budget. The job timeout is a backstop against a +# hang; this is the thing that actually notices one. +# * Three attempts with linear backoff, because the observed failure is +# transient — a re-run has cleared it every time. +# +# Deliberately NOT a general-purpose apt wrapper: one package, from a workflow +# `env:` (never from event data, which is the injection vector the Actions +# security guidance warns about), and a hard failure if it is unset. +set -euo pipefail + +if [ -z "${APT_PACKAGE:-}" ]; then + echo "::error::APT_PACKAGE is unset; refusing to guess what to install" >&2 + exit 1 +fi + +# Bounds chosen from observed behaviour, not from taste: a healthy `update` on +# these runners is a few seconds and a healthy `install` well under a minute, so +# these are roughly an order of magnitude of headroom. Long enough that a merely +# slow mirror still succeeds; short enough that three full attempts fit inside +# the 25-minute job budget with room for the build that follows. +readonly UPDATE_TIMEOUT=180 +readonly INSTALL_TIMEOUT=300 +readonly ATTEMPTS=3 + +for attempt in $(seq 1 "$ATTEMPTS"); do + if timeout "$UPDATE_TIMEOUT" sudo apt-get update -qq && + timeout "$INSTALL_TIMEOUT" sudo apt-get install -yq "$APT_PACKAGE"; then + echo "Installed ${APT_PACKAGE} on attempt ${attempt}." + exit 0 + fi + # Reported per attempt rather than only on final failure: a run that + # succeeded on attempt 3 looks identical to one that succeeded on attempt 1 + # in the job's conclusion, and the difference is the early warning that the + # mirrors are degrading. + echo "::warning::apt attempt ${attempt}/${ATTEMPTS} for ${APT_PACKAGE} failed or timed out" + if [ "$attempt" -lt "$ATTEMPTS" ]; then + sleep $((attempt * 15)) + fi +done + +echo "::error::Could not install ${APT_PACKAGE} after ${ATTEMPTS} attempts" >&2 +exit 1 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ae1c7f32..e269415f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -106,6 +106,7 @@ jobs: pull-requests: read outputs: code: ${{ steps.filter.outputs.code }} + accuracy: ${{ steps.filter.outputs.accuracy }} steps: # Full history: on `push`, dorny/paths-filter diffs against the before-SHA # using local git, so the default shallow clone (fetch-depth: 1) can miss @@ -138,6 +139,34 @@ jobs: - '!NOTICE' - '!.gitignore' - '!.codegraph/**' + # v2.3.9 A5 — paths that can move an accuracy or visual vector. + # + # `test-roms` used to be FULL-run only, so a regular feature PR never + # ran the accuracy battery and a regression could not be caught on + # the PR that caused it: it landed, turned `main` red, and was fixed + # by a second PR. #396 (a PPU fix that legitimately moved + # `visual_regression__scanline_frame_180`) and #403 (the vector + # update that followed) are the worked example. + # + # Scoped by path rather than by event, because a vector can only move + # if something that PRODUCES one changed. Measured against the last + # 40 merged PRs: 11 touch these paths and 29 do not, so ~72% of PRs + # still pay nothing and the cost model the full-run flag exists for + # is preserved. + # + # `rustynes-gamedb` is in the list for a non-obvious reason: the + # per-game database rewrites the iNES header on load, so it changes + # what the emulator IS before a single cycle runs. That is how the + # v2.3.4 Sachen defect reached users. + accuracy: + - 'crates/rustynes-cpu/**' + - 'crates/rustynes-ppu/**' + - 'crates/rustynes-apu/**' + - 'crates/rustynes-mappers/**' + - 'crates/rustynes-core/**' + - 'crates/rustynes-gamedb/**' + - 'crates/rustynes-test-harness/**' + - 'tests/**' # fmt + clippy + rustdoc share one runner + one compile of the workspace's # dependency graph (clippy and rustdoc differ only in the final pass), so @@ -330,14 +359,30 @@ jobs: test-roms: name: test (test-roms feature) # The SLOWEST job (release-mode compile + the heavy CPU / AccuracyCoin ROM - # batteries, ~20 min). FULL-run only: it is skipped on a regular feature PR - # and runs on push-to-`main` (every merge is accuracy-validated), the merge - # queue, dispatch, the weekly cron, and `release/*` PRs (so a release is - # proven before it is cut). This is the single biggest per-PR time/cost - # saving. Also gated on the fast lint job — don't pay the release compile - # when fmt/clippy already failed. + # batteries, ~20 min). Runs on a FULL run — push-to-`main` (every merge is + # accuracy-validated), the merge queue, dispatch, the weekly cron, and + # `release/*` PRs so a release is proven before it is cut — OR on any PR that + # touches a path able to move an accuracy or visual vector (the `accuracy` + # filter in the `changes` job, which carries the rationale). + # + # v2.3.9 A5. This was FULL-run only, which meant a regular feature PR never + # ran the battery, so an accuracy regression could not be caught on the PR + # that caused it — it landed, turned `main` red, and needed a second PR. + # Sharpening the point: `main`'s ruleset requires exactly ONE status context, + # `CI success`, and a SKIPPED job does not fail the aggregate. So the single + # gate on merging was reporting a pass for a property it had not tested. + # + # The per-PR saving the full-run flag exists for is preserved rather than + # traded away: measured over the last 40 merged PRs, 11 touch accuracy paths + # and 29 do not, so roughly 72% of PRs still skip this entirely. + # + # Also gated on the fast lint job — don't pay the release compile when + # fmt/clippy already failed. needs: [changes, lint, setup] - if: ${{ needs.changes.outputs.code == 'true' && needs.setup.outputs.full == 'true' }} + if: >- + ${{ needs.changes.outputs.code == 'true' + && (needs.setup.outputs.full == 'true' + || needs.changes.outputs.accuracy == 'true') }} runs-on: ubuntu-latest # Typical: ~29 min. Bounded so a hung job cannot hold the # concurrency group -- see the block comment above `concurrency`. @@ -522,10 +567,13 @@ jobs: # regardless of how the sysroot lays out `usr/include`. The cross linker in # that package is unused — this gate is `cargo check` only. - name: Provision the aarch64 glibc headers for bindgen + if: matrix.target == 'aarch64-unknown-linux-gnu' + env: + APT_PACKAGE: gcc-aarch64-linux-gnu + run: .github/scripts/apt-install-retry.sh + - name: Export the aarch64 bindgen sysroot if: matrix.target == 'aarch64-unknown-linux-gnu' run: | - sudo apt-get update -qq - sudo apt-get install -yq gcc-aarch64-linux-gnu echo "BINDGEN_EXTRA_CLANG_ARGS_aarch64_unknown_linux_gnu=--sysroot=/usr/aarch64-linux-gnu -isystem /usr/aarch64-linux-gnu/include" >> "$GITHUB_ENV" # Provision the armhf glibc headers for bindgen, exactly as for aarch64 # above. `gcc-arm-linux-gnueabihf` pulls `libc6-dev-armhf-cross`, landing @@ -533,10 +581,13 @@ jobs: # The cross linker in that package is unused here — this gate is # `cargo check` only; the buildbot remains the authority on linking. - name: Provision the armhf glibc headers for bindgen + if: matrix.target == 'armv7-unknown-linux-gnueabihf' + env: + APT_PACKAGE: gcc-arm-linux-gnueabihf + run: .github/scripts/apt-install-retry.sh + - name: Export the armhf bindgen sysroot if: matrix.target == 'armv7-unknown-linux-gnueabihf' run: | - sudo apt-get update -qq - sudo apt-get install -yq gcc-arm-linux-gnueabihf echo "BINDGEN_EXTRA_CLANG_ARGS_armv7_unknown_linux_gnueabihf=--sysroot=/usr/arm-linux-gnueabihf -isystem /usr/arm-linux-gnueabihf/include" >> "$GITHUB_ENV" # Deliberately NOT the composite action's `targets:` input. That routes # through `dtolnay/rust-toolchain`, which installs the target for the diff --git a/.github/workflows/release-auto.yml b/.github/workflows/release-auto.yml index f8ee0520..a1d806c8 100644 --- a/.github/workflows/release-auto.yml +++ b/.github/workflows/release-auto.yml @@ -62,7 +62,19 @@ jobs: runs-on: ubuntu-latest # Bounded like every other job (v2.3.7). `build` below cannot carry one — # `timeout-minutes` is not valid on a job that uses `uses:` — so its budget - # lives on the jobs inside `release.yml`. + # lives on the jobs inside `release.yml`, which already carry their own. + # + # Challenged in review on #406, which claimed the restriction was lifted in + # late 2022. It was not. GitHub's workflow-syntax and reuse-workflows pages + # state neither way, so it was checked against the schema rather than + # recalled; `actionlint` on exactly this shape: + # + # when a reusable workflow is called with "uses", "timeout-minutes" is not + # available. only following keys are allowed: "name", "uses", "with", + # "secrets", "needs", "if", and "permissions" + # + # So adding one here is a hard syntax error, not the harmless no-op it would + # be if the key were merely ignored. timeout-minutes: 15 outputs: should_release: ${{ steps.decide.outputs.should_release }} diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index db8e2d22..6e37fae1 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -71,6 +71,24 @@ repos: - id: markdownlint args: [--config, .markdownlint.json] + # GitHub Actions workflow linting (v2.3.9). + # + # Pinned to the version installed here, matching how every other hook in this + # file is pinned: an unpinned linter that gains a rule turns a green tree red + # on someone else's machine, which is the trap `markdownlint` already documents + # (the local binary reports rules the pinned v0.39.0 does not). + # + # Added because a review disagreement on #406 could not be settled from + # GitHub's own documentation — whether `timeout-minutes` is valid on a job that + # calls a reusable workflow. `actionlint` encodes the job schema and answered it + # in one command. Seven workflow files were edited during the v2.3.7 cut with no + # schema check at all; `check-yaml` proves a file is YAML, not that it is a + # workflow. + - repo: https://github.com/rhysd/actionlint + rev: v1.7.12 + hooks: + - id: actionlint + # Rust formatting and linting (when code exists) - repo: local hooks: diff --git a/AGENTS.md b/AGENTS.md index 7ad95f1c..0f9f6600 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -243,4 +243,11 @@ These cross-cutting decisions span multiple files. Reading individual chip docs - **The libretro wrapper is where the bugs live, not the core.** A v2.3.5 audit found five defects and every one was a *wrapper* defect with correct emulation behind it: hardcoded 60.0988 fps for every cartridge (**PAL ran 20.2% fast**), `retro_get_region` unimplemented, `retro_reset` unimplemented (**RetroArch's Reset did nothing, ever** — the library default is a literal no-op), `retro_unload_game` unimplemented (Game Genie indices leaked across cartridges), `aspect_ratio = 0.0` (square pixels; the desktop frontend applies 8:7), and no controller info (**the Zapper was unreachable** despite `Nes::set_zapper` being fully implemented). When something looks wrong in RetroArch, suspect what the wrapper *advertises* before suspecting the emulation. Prefer DERIVING declared values from `rustynes_core` constants (`FRAME_DURATION_*`, `DEFAULT_SAMPLE_RATE`) over transcribing them — the 60.0988 literal had lost all connection to the constant it was copied from. - **`cargo test ` that matches nothing prints `0 passed` and exits 0 — that is NOT a pass.** Bit twice in one session: `cargo test --workspace --features test-roms accuracycoin` and `cargo test -p rustynes-cpu nestest` both matched zero tests and looked green. The real invocations are `cargo test -p rustynes-test-harness --features test-roms --test accuracycoin` (prints `pass rate = 100.00% over 141 assigned tests`; **the RAM decoder is authoritative — the framebuffer decoder reports 120 and is known-buggy**) and `--test nestest`. Always confirm a non-zero test count before reporting a gate as green. +- **A SKIPPED job satisfies `CI success`, and `CI success` is `main`'s only required check.** The ruleset `Protect (Default)` requires exactly one status context. That job runs `if: always()` and fails on `contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')` — `skipped` is in **neither** list. So a gate that did not run reports a pass for a property it never tested. This is not theoretical: `test-roms` (the accuracy battery) was FULL-run only, so an accuracy regression could not be caught on the PR that caused it — it landed, turned `main` red, and needed a second PR (#396 introduced the vector move, #403 fixed it; **both PRs were correct, the process was not**). **There is no merge queue** — verified, no `merge_group` event appears in the run history — so nothing caught it later either. v2.3.9 A5 adds an `accuracy` paths-filter (chip crates, `rustynes-core`, `rustynes-gamedb`, `rustynes-test-harness`, `tests/`) so the battery runs on PRs that can actually break it; measured first, 11 of the last 40 merged PRs touch those paths, so ~72% still skip it. When judging whether a gate covers something, check three things separately: which contexts are *required*, what the aggregate treats as failure, and whether the job that tests the property actually **ran**. +- **Every SAME-TIMELINE restore must hand-carry the state that lives outside the save state.** `Nes::restore_inner` clears the pixel- and audio-provenance stores — right for a real timeline change, wrong for a restore that puts back the state the user is still looking at. v2.3.6 wired `take_provenance`/`put_provenance` into `RunAhead::finish`, **the one call site the report named**; `rustynes-probe` had three more (`Probe::run_uncounted` per trial — a latency measurement runs up to 21 — `latency::measure_in_place`'s final restore, and the atlas panel's `TimelineGuard`), so the Latency Oracle and RAM Atlas silently emptied both provenance panels. Both stores are **cumulative**, so nothing rebuilt them. `crates/rustynes-test-harness/tests/snapshot_schema_audit.rs` enumerates the whole excluded set and is the place to look — but its reasons are **free text**, and a keyword pass over them mis-sorted 17 of 26, so the durability question is not machine-checkable. Assert a caller's state returns **byte-identical**, never merely "armed": a store emptied while left armed passes an `is_some()` check (caught in review on #405). +- **`timeout-minutes` is NOT valid on a job that uses `uses:` — and `actionlint` is how you settle questions like that.** GitHub's workflow-syntax and reuse-workflows pages state it neither way, and a #406 review asserted the restriction was lifted in 2022. It was not: actionlint reports the key unavailable and lists the seven allowed (`name`, `uses`, `with`, `secrets`, `needs`, `if`, `permissions`), so adding one is a **hard syntax error**, not an ignored key. `release-auto.yml`'s `build` therefore cannot carry a timeout; its budget lives on the jobs inside `release.yml`. actionlint is installed and, from v2.3.9, a pinned pre-commit hook. **`.github/actionlint.yaml` has existed since v2.2.3** (it declares the self-hosted `agy` label) — extend it, never `Write` over it; doing so lost its rationale and produced a false "adding the config activated a dormant check" finding that reached a commit body before being retracted. +- **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.** + <<< MC-PROJECT-END >>> diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a68a346..5757ff8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -387,7 +387,11 @@ timeout, so one hung job silently skipped a release for five hours. Two details worth keeping. `release-auto.yml`'s `build` job **cannot** carry a timeout, because `timeout-minutes` is not valid on a job that uses `uses:`; its - budget lives on the jobs inside `release.yml`, which already had them. And + budget lives on the jobs inside `release.yml`, which already had them. (Review + challenged this, claiming the restriction was lifted in 2022. It was not — + checked against the schema with `actionlint`, which reports the key as + unavailable and lists the seven that are allowed. Adding one is a syntax error, + not an ignored key.) And `antigravity-review.yml` is bounded *harder* than the hosted jobs rather than softer, because it runs on the maintainer's own hardware, where a hung run holds a real machine instead of a disposable VM. diff --git a/crates/rustynes-frontend/src/debugger/latency_panel.rs b/crates/rustynes-frontend/src/debugger/latency_panel.rs index ff2255b9..90b60ddd 100644 --- a/crates/rustynes-frontend/src/debugger/latency_panel.rs +++ b/crates/rustynes-frontend/src/debugger/latency_panel.rs @@ -101,6 +101,7 @@ pub fn show( state: &mut LatencyPanel, nes: Option<&mut Nes>, current_run_ahead: u32, + render_work: crate::perf::IntervalStats, ) { let can_measure = nes.is_some(); super::detachable_window( @@ -113,7 +114,7 @@ pub fn show( ..Default::default() }, open, - |ui| body(ui, state, can_measure, current_run_ahead), + |ui| body(ui, state, can_measure, current_run_ahead, render_work), ); // Measure AFTER the render — `nes` is free here, not captured by any closure. if std::mem::take(&mut state.measure_requested) { @@ -122,7 +123,13 @@ pub fn show( } /// The panel body, shared by the docked window and the detached OS viewport. -fn body(ui: &mut egui::Ui, state: &mut LatencyPanel, can_measure: bool, current: u32) { +fn body( + ui: &mut egui::Ui, + state: &mut LatencyPanel, + can_measure: bool, + current: u32, + render_work: crate::perf::IntervalStats, +) { ui.label("Measures how many frames this game waits before acting on input."); ui.weak( "Replays the current moment twice — once with a button held, once without — \ @@ -157,6 +164,7 @@ fn body(ui: &mut egui::Ui, state: &mut LatencyPanel, can_measure: bool, current: report, current, state.frame_ms, + render_work, &mut state.pending_apply, ); } @@ -173,6 +181,7 @@ fn report_body( report: &LatencyReport, current: u32, frame_ms: f64, + render_work: crate::perf::IntervalStats, pending_apply: &mut Option, ) { if let Some(frames) = report.frames { @@ -187,6 +196,7 @@ fn report_body( // copied from and ran every PAL cartridge fast. (PR #385 review.) let ms = f64::from(frames) * frame_ms; ui.weak(format!("about {ms:.0} ms of the game's own delay")); + end_to_end(ui, frames, current, frame_ms, render_work); let confidence = match report.confidence { Confidence::Unanimous => "every reacting button agreed", @@ -265,10 +275,175 @@ fn run_measurement(state: &mut LatencyPanel, nes: Option<&mut Nes>) { state.report = Some(report); } +/// v2.3.9 item C (interim) — the game's delay plus the renderer's own work. +/// +/// # What this deliberately is NOT +/// +/// It is **not** the end-to-end wall-clock delay, and the label says so. Two +/// costs are excluded and both are excluded for the same reason: `render_wait` +/// (the blocking present) and `render_lock` (mutex contention) are their own +/// percentile series, and adding two p95s is not the p95 of the sum. +/// +/// That is the error `RenderPerf::work` already exists to avoid — it is kept as +/// a real per-redraw series precisely because `total p95 - wait p95` produced a +/// published table whose `work p95` sat below its `work p50`. Summing has the +/// same defect as differencing; only the direction changes. +/// +/// What makes THIS sum legitimate is that the lag term is a **constant**, not a +/// distribution: adding a constant shifts every percentile by exactly that +/// constant, so `lag + work_p95` is a genuine p95 of `lag + work`. Extending it +/// to a second series would break that, which is why the honest figure is the +/// narrow one. +/// +/// A true wall-clock figure needs a single per-redraw end-to-end series recorded +/// as one sample — an addition to `RenderPerf`, not arithmetic over what exists. +/// See `to-dos/plans/v2.3.9-crucible-plan.md` item C. +fn end_to_end( + ui: &mut egui::Ui, + frames: u32, + current_run_ahead: u32, + frame_ms: f64, + render_work: crate::perf::IntervalStats, +) { + match end_to_end_figure(frames, current_run_ahead, frame_ms, render_work) { + EndToEnd::Unavailable { samples, need } => { + ui.weak(format!( + "end-to-end unavailable: {samples} render samples, need {need}" + )); + } + EndToEnd::Ms { total, effective } => { + ui.label(format!( + "Game delay + render work: about {total:.0} ms (p95)" + )); + if current_run_ahead > 0 { + ui.weak(format!( + "{effective} of {frames} frames remain after run-ahead {current_run_ahead}" + )); + } + ui.weak("Excludes the vblank wait and lock contention — see the panel docs."); + } + } +} + +/// A percentile over a handful of redraws is noise wearing a number's clothing. +const MIN_RENDER_SAMPLES: usize = 60; + +/// The interim end-to-end figure, or why there isn't one. +/// +/// Extracted from the render so the arithmetic is testable rather than trapped +/// in an `egui` closure — the same reason the Divergence Lens lifts its verdict +/// wording out. The two branches are what is worth pinning: a figure and a +/// refusal must stay distinguishable. +#[derive(Debug, Clone, Copy, PartialEq)] +enum EndToEnd { + /// Too few render samples for a percentile to mean anything. + Unavailable { samples: usize, need: usize }, + /// Milliseconds, with the frame count left after run-ahead. + Ms { total: f64, effective: u32 }, +} + +fn end_to_end_figure( + frames: u32, + current_run_ahead: u32, + frame_ms: f64, + render_work: crate::perf::IntervalStats, +) -> EndToEnd { + if render_work.count < MIN_RENDER_SAMPLES { + return EndToEnd::Unavailable { + samples: render_work.count, + need: MIN_RENDER_SAMPLES, + }; + } + // Run-ahead removes up to `depth` frames of the game's own lag, so a figure + // that ignores it overstates what the user experiences — and overstates it + // worst exactly when they have taken this panel's advice. Saturating, not + // wrapping: a depth above the measured lag leaves zero, not `u32::MAX`. + let effective = frames.saturating_sub(current_run_ahead); + EndToEnd::Ms { + total: f64::from(effective) * frame_ms + f64::from(render_work.p95_ms), + effective, + } +} + #[cfg(test)] mod tests { use super::*; + fn work(count: usize, p95_ms: f32) -> crate::perf::IntervalStats { + crate::perf::IntervalStats { + count, + p95_ms, + ..crate::perf::IntervalStats::default() + } + } + + /// Too few samples must produce a REFUSAL, not a small number. A percentile + /// over a handful of redraws is noise, and printing it would be this panel + /// doing the exact thing it exists to refuse. + #[test] + fn too_few_render_samples_declines_rather_than_guessing() { + assert_eq!( + end_to_end_figure(4, 0, 16.639, work(MIN_RENDER_SAMPLES - 1, 3.0)), + EndToEnd::Unavailable { + samples: MIN_RENDER_SAMPLES - 1, + need: MIN_RENDER_SAMPLES, + } + ); + } + + /// The arithmetic: a CONSTANT lag plus ONE percentile series. Valid only + /// because adding a constant shifts every percentile by exactly that + /// constant — which is why a second series may never be added here. + #[test] + fn the_figure_is_lag_plus_one_series() { + let EndToEnd::Ms { total, effective } = end_to_end_figure(4, 0, 16.0, work(600, 3.5)) + else { + panic!("expected a figure"); + }; + assert_eq!(effective, 4); + assert!( + (total - (4.0 * 16.0 + 3.5)).abs() < 1e-6, + "expected 67.5, got {total}" + ); + } + + /// Run-ahead removes frames of the game's own lag, so the figure must shrink + /// by exactly one frame per depth — otherwise the panel overstates latency + /// worst for the users who took its advice. + #[test] + fn run_ahead_is_subtracted_frame_for_frame() { + let (base, with_two) = ( + end_to_end_figure(4, 0, 16.0, work(600, 0.0)), + end_to_end_figure(4, 2, 16.0, work(600, 0.0)), + ); + let ( + EndToEnd::Ms { total: a, .. }, + EndToEnd::Ms { + total: b, + effective, + }, + ) = (base, with_two) + else { + panic!("expected figures"); + }; + assert_eq!(effective, 2); + assert!( + (a - b - 32.0).abs() < 1e-6, + "two frames at 16 ms = 32 ms; got {a} vs {b}" + ); + } + + /// A depth ABOVE the measured lag leaves zero, never a wrapped `u32::MAX`. + #[test] + fn run_ahead_deeper_than_the_lag_saturates_at_zero() { + let EndToEnd::Ms { total, effective } = end_to_end_figure(1, 3, 16.0, work(600, 2.0)) + else { + panic!("expected a figure"); + }; + assert_eq!(effective, 0); + assert!((total - 2.0).abs() < 1e-6, "only render work remains"); + } + fn report(frames: Option, confidence: Confidence) -> LatencyReport { LatencyReport { frames, diff --git a/crates/rustynes-frontend/src/debugger/mod.rs b/crates/rustynes-frontend/src/debugger/mod.rs index f1427621..f2e56846 100644 --- a/crates/rustynes-frontend/src/debugger/mod.rs +++ b/crates/rustynes-frontend/src/debugger/mod.rs @@ -2205,6 +2205,12 @@ impl DebuggerOverlay { // "applied" stay two separate, auditable steps. if self.show_latency { let current = config.input.run_ahead; + // v2.3.9 item C — the render-WORK series, read from the perf panel's + // snapshot rather than plumbed separately, so there is one copy of + // this data in the overlay. Only `work` is offered; see + // `PerfPanelState::render_work` for why it is the only series that + // can legitimately be added to the measured lag. + let render_work = self.perf_ui.render_work(); latency_panel::show( ctx, &mut self.detached_panels, @@ -2212,6 +2218,7 @@ impl DebuggerOverlay { &mut self.latency_ui, nes.as_deref_mut(), current, + render_work, ); if let Some(depth) = self.latency_ui.take_pending_apply() { config.input.run_ahead = depth; diff --git a/crates/rustynes-frontend/src/debugger/perf_panel.rs b/crates/rustynes-frontend/src/debugger/perf_panel.rs index 4bb52814..708bdfef 100644 --- a/crates/rustynes-frontend/src/debugger/perf_panel.rs +++ b/crates/rustynes-frontend/src/debugger/perf_panel.rs @@ -39,6 +39,20 @@ impl PerfPanelState { self.view = view; } + /// The render-WORK series from the current snapshot. + /// + /// v2.3.9 — exposed so the Latency Oracle can add the pipeline's own cost to + /// the game's internal lag without a second copy of the perf plumbing. Only + /// `work` is offered, deliberately: it is the one series that can be added to + /// a constant and still yield a real percentile. Summing it with + /// `render_lock` or `render_wait` would be summing two percentiles, which is + /// not the percentile of the sum — the error `RenderPerf::work` already + /// exists to avoid, in the addition direction. + #[must_use] + pub const fn render_work(&self) -> crate::perf::IntervalStats { + self.view.render_work + } + /// Update the logging status line (destination path / error). #[cfg(not(target_arch = "wasm32"))] pub fn set_log_note(&mut self, note: Option) { diff --git a/to-dos/plans/v2.3.9-crucible-plan.md b/to-dos/plans/v2.3.9-crucible-plan.md new file mode 100644 index 00000000..8d03d1ed --- /dev/null +++ b/to-dos/plans/v2.3.9-crucible-plan.md @@ -0,0 +1,429 @@ +# v2.3.9 "Crucible" — testing, correctness, and what the gates actually cover + +## Goal + +A crucible is where something is tested to destruction rather than inspected. +This release is about the **gates**: what they cover, what they only appear to +cover, and where a regression can still reach `main` unchallenged. + +It is deliberately not a feature release. The v2.3.x line has added five tools in +four releases, and the recurring finding across all of them has not been that the +emulation was wrong — it is that a check reported a pass it had not earned. + +## Item A5 — make `test-roms` reachable at review time (marquee) + +### The gap, stated precisely + +`test-roms` is **full-run-only**. `ci.yml`'s `setup` job computes one `full` flag: + +| Event | `full` | Runs `test-roms`? | +| --- | :---: | :---: | +| regular feature PR | `false` | **no** | +| `release/*` PR | `true` | yes | +| push to `main` | `true` | yes | +| merge queue / dispatch / weekly cron | `true` | yes | + +The cost model behind that is sound and should be preserved: the job is the +slowest in CI (~29 minutes, release-mode compile plus the ROM batteries), a PR +gets many pushes, and keeping it off every push is the single biggest saving. + +The consequence is equally real: **an accuracy regression cannot be caught on the +PR that causes it.** It lands on `main`, turns `main` red, and is then fixed by a +second PR. + +### The worked example, checked rather than recalled + +- **#396** fixed *Rad Racer*'s roadside artifact and touched + `crates/rustynes-ppu/src/ppu.rs`. That legitimately moved the + `visual_regression__scanline_frame_180` vector. The PR was green. +- `main` went red on merge. +- **#403** then updated the vector — touching only the `.snap` and the CHANGELOG. + +Both PRs were correct. The process was not. + +### The proposal: scope by path, not by event + +A visual or accuracy vector can only move if something that *produces* one +changed. A frontend-only PR — which is most of the v2.3.x line — cannot move +`scanline_frame_180` no matter what it does. + +So add a second `dorny/paths-filter` output beside the existing `code` filter, +and run `test-roms` on a PR when it is true: + +```yaml +accuracy: + - 'crates/rustynes-cpu/**' + - 'crates/rustynes-ppu/**' + - 'crates/rustynes-apu/**' + - 'crates/rustynes-mappers/**' + - 'crates/rustynes-core/**' + - 'crates/rustynes-gamedb/**' + - 'crates/rustynes-test-harness/**' + - 'tests/**' +``` + +`test-roms` then runs when `full == 'true'` **OR** `accuracy == 'true'`. + +Checked against the real case: #396 touched `crates/rustynes-ppu/src/ppu.rs` and +#403 touched `crates/rustynes-test-harness/tests/snapshots/`, so **both** would +have run the battery on the PR. The regression would have been visible where it +was introduced. + +`rustynes-gamedb` is in the list for a reason that is not obvious: the per-game +database rewrites the iNES header on load, so it changes what the emulator *is* +before a single cycle runs. That is how the v2.3.4 Sachen defect reached users. + +### What this must not become + +- **Not "run everything on every PR".** Frontend PRs pay nothing, which is what + keeps the saving the current model was built for. +- **Not a replacement for the `main` run.** The full battery still runs on every + merge. This adds a gate; it removes none. +- **The added cost is measured, not assumed.** Over the **last 40 merged PRs**, + **11** touch these paths and **29** do not — so **~72% of PRs still skip the + battery entirely**. The saving the full-run flag exists for is preserved rather + than traded away. Had the split come out near half, the trade-off would have + needed restating rather than adopting. + +### The open question, settled + +The plan's first draft asked whether a **merge queue** is in use, because +`setup` lists `merge_group` as a full-run trigger: if a queue ran the battery +between approval and merge, A5 would only move the failure earlier, and the +argument for it would be much smaller. + +**It is not in use.** Checked rather than assumed: + +- No `merge_group` event appears anywhere in the recent run history — the events + present are `push`, `pull_request`, `workflow_run`, `issue_comment`, `dynamic`. +- `main` has no classic branch protection; the active ruleset `Protect (Default)` + carries `deletion`, `non_fast_forward`, and `required_status_checks` with + exactly **one** required context: `CI success`. + +That last detail sharpens the problem rather than softening it. The single gate +on merging is `CI success`, and on a regular feature PR `CI success` is +**satisfied without the accuracy battery having run** — `test-roms` is skipped, +and a skipped job does not fail the aggregate. So the required check reports a +pass for a property it did not test, which is the same shape as every other +finding in this line. + +A5's value is therefore the full one: there is no later gate that catches this +before `main`. + +### Status: implemented, and how it is verified + +The filter and the `test-roms` condition are in. What backs it: + +- **The gate can now actually fail.** `CI success` runs `if: always()` and fails + when any need's result is `failure` **or `cancelled`** — `skipped` is in + neither list, which is exactly why a skipped `test-roms` satisfied it before. + On an accuracy PR the job now runs, so a failure fails the one required + context and the ruleset blocks the merge. Read from the workflow rather than + assumed. +- **The globs are checked against real file lists**, not eyeballed: #396 + (`crates/rustynes-ppu/src/ppu.rs`) and #403 + (`crates/rustynes-test-harness/tests/snapshots/`) both match, so both would + have run the battery on the PR where the regression was introduced and where + it was fixed. +- **A negative demonstration rides along on this very PR.** It touches + `.github/workflows/ci.yml` and `to-dos/`, neither of which is an accuracy path, + so `test-roms` must be **skipped** here. If it runs, the filter is too broad. +- **The positive demonstration is the next PR after it.** v2.3.8's branch touches + `crates/rustynes-ppu`, so it will be the first PR to exercise the new gate for + real. Confirm the job appears there before treating A5 as done. + +Note `rustynes-probe` is deliberately **absent** from the list. It is an +output-only analysis engine that cannot affect emulation, so a probe-only change +has no way to move a vector; including it would cost 29 minutes for nothing. + +## Item A5b — bound the provisioning steps, not just the jobs + +Found by the release this plan was written alongside, which is the only reason it +is in scope: **four separate hangs across two PRs during the v2.3.7 cut**, every +one in a setup or provisioning step and not one in a compile or test step — twice +in `rust-setup`, once in the armhf provision, once in the aarch64 provision. Each +cost 25-45 minutes and a manual re-run, and one of them blocked the release PR. + +The per-job `timeout-minutes` from #400 did its job: it bounded the damage. What +it cannot do is notice. A stalled `apt-get` inside a 25-minute job is +indistinguishable, from the outside, from a job that is simply slow — and the run +is reported as **cancelled**, which reads as infrastructure noise rather than as +the specific thing that happened. + +`.github/scripts/apt-install-retry.sh` adds the two bounds that were missing, and +they do different jobs: + +- **`timeout` per command**, so a stalled fetch fails in minutes instead of + consuming the job budget. The job timeout is a backstop against a hang; this is + what actually notices one. +- **Three attempts with linear backoff**, because the observed failure is + transient — a re-run cleared it every time. + +Every attempt emits a `::warning::`, including ones that eventually succeed. A +run that needed three attempts and one that needed one are identical in the job's +conclusion, and the difference is the early warning that the mirrors are +degrading. + +Verified by stubbing `sudo`, `timeout` and `sleep` rather than by reading: +all-attempts-fail produces three warnings and exits 1; fail-fail-succeed produces +two warnings, reports the attempt it succeeded on, and exits 0. The unset-package +guard exits 1 rather than guessing. shellcheck clean. + +**Scope discipline:** deliberately not a general-purpose apt wrapper. One +package, taken from a workflow `env:` and never from event data — which is the +injection vector the Actions security guidance names — and a hard failure when +unset. + +## Item A5c — lint the workflows against the schema, not just against YAML + +Seven workflow files were edited during the v2.3.7 cut with no schema check at +all. `check-yaml` proves a file is valid YAML; it says nothing about whether a +key is legal where it appears. + +The gap surfaced as a review disagreement on #406 that GitHub's own +documentation could not settle: is `timeout-minutes` valid on a job that calls a +reusable workflow? Neither the workflow-syntax page nor the reuse-workflows page +states it. `actionlint` encodes the job schema and answered in one command — +it is not, and the seven keys that are allowed are `name`, `uses`, `with`, +`secrets`, `needs`, `if`, `permissions`. Adding one is a **syntax error**, not +an ignored key. + +Adopted as a pre-commit hook, pinned like every other hook in that file for the +reason `markdownlint` already documents there: an unpinned linter that gains a +rule turns a green tree red on someone else's machine. + +Three things came out of adopting it, and two were not the point: + +1. **Two suppressions were needed, both against intent.** `pgo.yml`'s two + deliberately-disabled `if: false` steps trip the `if-cond` rule. Suppressed in + `.github/actionlint.yaml`, scoped to that one file and that one message — and + the scoping was **verified** by adding an `if: false` to `security.yml` and + confirming it still fails. The cost is stated in the config rather than + buried: the rule is now silent for the whole of `pgo.yml`, and actionlint has + no line-scoped ignore. +2. **A finding recorded here was wrong, and the retraction is the lesson.** This + list originally read: *"Adding the config activated a dormant check — the + self-hosted `agy` label was reported as unknown only after `actionlint.yaml` + existed."* + + `.github/actionlint.yaml` **already existed on `main`**, added in v2.2.3, and + already declared `agy` with a fuller rationale than the replacement. It was + overwritten rather than extended — a `Write` on a path that was never read + first, against the standing read-before-write rule. Removing the declaration + is what made actionlint report the label as unknown, so the "dormant check" + was self-inflicted breakage reported as a discovery. + + Restored to `main`'s content with the `paths:` suppression **appended**; the + diff against `main` is now purely additive, checked rather than assumed. + + The real lesson is narrower and more useful than the false one: a config file + for a tool the repo already uses is exactly the kind of file that looks new + because *you* have not seen it. Nothing about the tool being newly adopted as + a **hook** implied its configuration was absent. +3. The repo is otherwise clean. + +## Item B — the v2.3.x frontend sweep, treated as a class + +v2.3.6 found two shipped features that had never worked, and the mechanism in +both cases was frontend wiring that no test covered. v2.3.7 and v2.3.8 each found +another instance of the same class *while building something else*. + +That is now four, which makes it a class rather than a coincidence: + +| Release | What was found | Mechanism | +| --- | --- | --- | +| v2.3.6 | Pixel Provenance empty for every user | run-ahead rollback cleared the store | +| v2.3.6 | "click any pixel" never implemented | no hit-test existed | +| v2.3.7 | Latency Oracle / RAM Atlas emptied both provenance panels | three unguarded probe restores | +| v2.3.8 | the Lens advanced the live emulator 30 frames | no restore on the way out | + +Sweep every v2.3.x frontend feature against the two questions the class implies: + +1. **Can the panel observe what it claims to?** Not "is the core correct" — is + there a path by which the value reaches the UI at the moment it renders. +2. **Does it put back everything it borrowed?** Timeline, rewind ring, + provenance, and anything else that lives outside the save state. + +The second question has a mechanical answer available: enumerate what is NOT +serialized, because that is exactly the set a snapshot round trip cannot restore +and therefore the set every guard has to carry by hand. + +### Question 1's sweep found a CANDIDATE, not a confirmed defect + +"Can the panel observe what it claims to?" turns on the lock discipline, and the +discipline is sound in the place it matters most: `produce_one_frame` runs under +one `emu.lock()` held across the *whole* frame, so a UI read can never catch a +half-filled provenance store. Whatever it sees is some complete frame. + +**Which complete frame is the open question.** In the `needs_nes` render arm — +the branch taken precisely when a debugger or tool panel is open — there are +**two** lock acquisitions per redraw, not one: + +1. `app.rs` ~9730 — copy the framebuffer, index framebuffer, NTSC phase and HD + tiles into staging. This is what the user ends up seeing. +2. `app.rs` ~9944 — "PHASE 1: build the egui UI under a SCOPED emulator lock", + where `run_shell_ui` hands `&mut Nes` to the panels. This is where Pixel + Provenance and Audio Provenance read. + +The guard is dropped between them, deliberately, so the composite/HD work does +not hold the emulator. Which means the emulation thread may acquire the lock in +that gap and produce another frame. If it does, the screen shows frame `N` while +the panel describes frame `N+1`. + +That would be a confidently wrong answer in the one panel whose entire purpose is +explaining the pixel you are looking at — and it is the same *shape* as the +v2.3.6 defect (the panel could not see the frame the user was on), differing only +in degree: empty then, off-by-one now. + +**This is a hypothesis from reading the lock structure. It has NOT been +demonstrated**, and the distinction is the point — this line has already retracted +one conclusion drawn from reading rather than measuring. + +The experiment that settles it, in order of cost: + +1. Record the core's frame counter at **both** acquisitions in the `needs_nes` + arm, under `debug-hooks`, and count redraws where they differ. Non-zero + confirms it; zero over a long capture with a tool panel open bounds it. +2. If confirmed, the fix is not obvious and should not be assumed: merging the + two acquisitions would put the composite work back under the emulator lock, + which is the exact regression v2.3.0 fixed. The likelier shape is capturing + the provenance snapshot at acquisition 1 alongside the framebuffer, so panel + and picture come from one read. +3. Its magnitude also matters before acting: bounded at one frame, and only when + the producer wins that race. + +Recorded as the deliverable of question 1 rather than fixed here, because a fix +whose need is unmeasured is how the composite work ended up under the lock in the +first place. + +### That enumeration already exists — and the sweep against it comes back clean + +`crates/rustynes-test-harness/tests/snapshot_schema_audit.rs` already lists every +field deliberately excluded from the save state, each with a written reason, and +fails the suite when a new field is added without one. No second enumeration is +needed; reuse this one. + +Cross-referencing its **26 output-only fields** against "must a guard carry this +across a same-timeline restore?" splits them cleanly, once the `see ` +indirections are resolved: + +| Category | Fields | Needs a guard? | +| --- | --- | :---: | +| Rebuilt every frame or every fetch | `index_framebuffer`, `prov_frame`, the `prov_*` address cascade, `attrib_pc`/`attrib_cycle`, `dma_attrib_*`, the ten `hd_*` telemetry fields, `last_external` | no | +| **Cumulative** — "which instruction last wrote this", answerable from thousands of frames ago | `write_attrib`, `audio_prov`'s `reg_attrib` | **yes** | + +Both members of the cumulative set are already carried, by `ProvenanceStash` and +`AudioProvenanceStash` respectively. **So the sweep finds no missing guard**, and +that negative result is the useful output: the v2.3.7 fix covered the whole set +rather than merely the reported members. + +### The limitation of that method, which is the actual v2.3.9 finding + +The classification above required **reading prose**. The audit's reasons are free +text: `attrib_cycle` says only "see `attrib_pc`", the ten `hd_*` fields say +"`hd-pack` fetch telemetry" without saying whether that is per-frame, and a +keyword pass over them mis-sorted **17 of 26** on the first attempt. + +So the enumeration exists and the durability question it needs to answer is not +machine-checkable. Nothing stops the next output-only field from being added with +a reason that is true but silent about whether it survives a restore — and the +guard that should carry it from never being written. + +**Candidate work, sized honestly:** give the audit a machine-readable durability +tag per field (`Rebuilt` / `Cumulative`) and assert that every `Cumulative` field +is named by a guard. That converts a prose cross-reference done once into a gate. +It is not free — it needs a way to enumerate guards — so it is a proposal here +rather than a commitment. + +## Item C — carried tool work + +Scoped here rather than left implicit, and explicitly lower priority than A5/B: + +- **Latency Oracle:** per-game persistence. +- **Latency Oracle: the end-to-end millisecond figure.** Carried forward as + "internal lag plus the frontend pipeline cost `perf.rs` already tracks", which + is very nearly right and hides three decisions. Investigated before writing + any of it: + + The panel currently reports `frames * frame_ms` and labels it *"about N ms of + the game's own delay"* — correctly scoped, and derived from the console's own + frame duration rather than a hardcoded 16.639, which is what keeps PAL and + Dendy honest. + + **1. Which figure? There are two, and they are not interchangeable.** + `RenderPerf` deliberately splits `wait` (the blocking present) out of `total`, + because under Fifo a present that blocks until vblank is *correct behaviour, + not a stall*. So: + + | figure | composition | what it answers | + | --- | --- | --- | + | avoidable latency | internal lag + `work` + `lock` | how much could in principle be removed | + | wall-clock delay | internal lag + `total` (wait included) | how long until the pixel is actually on screen | + + Reporting one under the other's name is precisely the class of error this + release exists to find. Report **both**, labelled — the same refusal to + collapse distinct answers that keeps `None` and `Some(0)` apart. + + **2. Run-ahead must be subtracted, and the note omits it.** The Oracle measures + the *game's* internal lag, which is a property of the ROM and independent of + the frontend. Run-ahead then removes up to `depth` frames of it. An end-to-end + figure that ignores the configured depth overstates what the user experiences, + and does so worst exactly when they have taken the panel's own advice. + + **3. The percentile arithmetic, which is where this design broke.** + + The first version of this section argued that the arithmetic was sound: + `perf.rs` records that `work p95 = total p95 - wait p95` is invalid, but + *adding* is different, because internal lag is a **constant** and adding a + constant shifts every percentile by exactly that constant. So `lag_ms + + work_p95` is a real p95. + + That is true, and it is not enough, because **neither figure above is + `lag + one series`**: + + | figure | needs | valid? | + | --- | --- | :---: | + | avoidable latency | `lag + render_work + render_lock` | **no** | + | wall-clock delay | `lag + render_work + render_lock + render_wait` | **no** | + + `PerfView` exposes `render_work`, `render_lock` and `render_wait` as + **separate percentile series**. Summing two p95s is not the p95 of the sum — + the identical error `perf.rs` already retracted a published table for, just in + the addition direction rather than the subtraction one. The constant-shift + argument rescues exactly one series, and both figures need two or three. + + **So item C is not implementable from the data that exists**, and the note it + was carried forward on ("the frontend pipeline cost `perf.rs` already tracks") + is wrong about "already". What is missing is a **single per-redraw series** — + end-to-end pipeline latency recorded as one sample per redraw and percentiled + as one distribution. That is an addition to `RenderPerf`, not a panel change, + and it is the actual first step. + + A cheaper interim that does not lie: report `lag_ms + render_work_p95` alone, + labelled as *game delay plus render work*, and say plainly that the vblank wait + and lock contention are excluded. It answers less than the carried-forward note + promised, and it answers it correctly. +- **RAM Atlas:** export paths (Watch/Cheat seeding, Lua, RetroAchievements + authoring); per-game persistence. + +Both are additive and neither touches the deterministic core. + +## Verification bar + +The standing gate, plus: + +- **Any CI change must be demonstrated against a real historical case**, not + argued. A5's design is already checked against #396 and #403; anything added to + it gets the same treatment. +- **A gate that cannot fail is not a gate.** Any new check lands with a + deliberate demonstration that it goes red for the defect it targets — the + standard the v2.3.8 mutations set. +- AccuracyCoin **141/141** (RAM decoder — the authoritative one; the framebuffer + decoder reports 120 and is known-buggy) and nestest 0-diff, verified with a + non-zero test count confirmed, since a filter matching nothing exits 0. + +## Explicitly out of scope + +- **The upstream libretro `.info` sync** — deferred to **v2.4.0** by maintainer + decision. A licence change overrides that and syncs immediately. +- **`libretro/docs#1180`** — outside this project.