feat(perf): measure the two-acquisition race, and land the apt fix #408 missed by doublegate · Pull Request #409 · doublegate/RustyNES

doublegate · 2026-08-19T13:28:58Z

Two things, stacked on #408: the measurement for v2.3.9 item B, and the apt-ordering fix that #408 was supposed to carry and did not (see the correction note below).

The measurement

v2.3.9 item B recorded a hypothesis rather than a defect: the needs_nes render arm — taken exactly when a debugger or tool panel is open — acquires the emulator lock twice per redraw. Once to copy the framebuffer the user will see, and again for run_shell_ui, where panels read &mut Nes. The guard is dropped between them so composite work does not hold the emulator, which means the emulation thread can take the lock in that gap. If it does, the screen shows frame N while a panel describes N+1 — a confidently wrong answer in Pixel Provenance, whose whole purpose is explaining the pixel you are looking at.

It was written down as a hypothesis with the experiment attached, because this line has already retracted one conclusion drawn from reading rather than measuring. This is the experiment.

Nes::cycle() is read at both acquisitions and compared. The choice of quantity is load-bearing: it is cumulative and monotonic, and produce_one_frame holds the lock across a whole frame, so any difference at all means at least one complete frame landed in the gap — there is no partial-frame reading to misinterpret. No frame counter exists on Nes, and adding one would have been a second source of truth for something cycle() already answers.

Both counters are kept, not just the hits. "The race did not fire" and "nothing was observed" both read as zero hits, and only the denominator separates them — this release's own distinction, applied to its own instrument. The denominator counts redraws where the race could have fired (both readings present: a ROM loaded and the arm run twice). Counting ROM-less redraws would dilute the rate toward zero and manufacture the reassuring answer.

Surfaced in the Performance panel as "panel/screen frame skew — N of M redraws (X%)". The hover text says what a zero does and does not mean: it bounds the effect over that capture, it is not proof the race cannot happen. A measurement that reads as a verdict is how the next person stops looking.

debug-hooks-gated throughout, so the shipped default carries neither the two reads nor the counters. Two tests pin the counter's semantics.

It deliberately fixes nothing. The rate has to be observed on a real session before a fix is chosen, because the obvious fix — merging the two acquisitions — puts composite work back under the emulator lock, which is exactly the regression v2.3.0 fixed.

Correction: the apt-ordering fix missed #408

Review on #408 caught two real defects in apt-install-retry.sh, and I replied there saying they were "fixed in ab30c828". That commit was never on #408's branch. A compound shell command containing its git checkout was blocked by a privilege-escalation guard, so nothing in it ran — including the checkout — and I committed the fix onto this branch instead. The subsequent git push origin feat/v2.3.9-crucible pushed the unchanged local ref: a silent no-op.

So the fix rides here instead, unchanged:

Elevation outermost, timeout inside. With timeout outermost the signal goes to the elevation helper, which may not forward it, leaving apt-get orphaned while still holding the dpkg lock — so every retry then fails on the lock rather than the original problem. A retry loop that guarantees its own retries fail is worse than none.
DEBIAN_FRONTEND=noninteractive. A config prompt blocks on stdin that never arrives in CI, burning the whole timeout budget — the same failure the script bounds, through a door it had left open.

Re-verified by stubbing the elevation helper, timeout and sleep: fail-fail-succeed names the attempt it succeeded on and exits 0. shellcheck clean.

Verification

fmt, shellcheck, actionlint tree-wide, workspace clippy, frontend under debug-hooks (522 tests), both wasm32 invocations, rustdoc.

No emulation-core file is touched, and no accuracy path — so test-roms should be skipped here, A5's negative demonstration again.

…ut it v2.3.9 item B recorded a hypothesis: the `needs_nes` render arm — taken exactly when a debugger or tool panel is open — acquires the emulator lock TWICE per redraw. Once to copy the framebuffer the user will see, and again sixty lines later for `run_shell_ui`, where panels read `&mut Nes`. The guard is dropped between them so the composite work does not hold the emulator, which means the emulation thread can take the lock in that gap. If it does, the screen shows frame N while a panel describes N+1 — a confidently wrong answer in Pixel Provenance, whose whole purpose is explaining the pixel you are looking at. That was written down as a hypothesis rather than a defect, with the experiment attached, because this line has already retracted one conclusion drawn from reading rather than measuring. This is the experiment. `Nes::cycle()` is read at both acquisitions and the readings compared. The choice of quantity matters: it is cumulative and monotonic, and `produce_one_frame` holds the lock across a WHOLE frame, so any difference at all means at least one complete frame landed in the gap — there is no partial-frame reading to misinterpret. No frame counter exists on `Nes`, and adding one would have been a second source of truth for something the cycle counter already answers. Both counters are kept, not just the hits. "The race did not fire" and "nothing was observed" both read as zero hits, and only the denominator separates them — the same distinction this release keeps insisting on, applied to its own instrument. The denominator counts redraws where the race COULD have fired: both readings present, meaning a ROM is loaded and the arm ran twice. Counting ROM-less redraws would dilute the rate toward zero and manufacture the reassuring answer. Surfaced in the Performance panel as a rate with the counts beside it, and the hover text says what a zero does and does not mean: it BOUNDS the effect over that capture, it is not proof the race cannot happen. A measurement that reads as a verdict is how the next person stops looking. `debug-hooks`-gated throughout, so the shipped default carries neither the two reads nor the counters. Two tests pin the counter's semantics — that an unobserved redraw is not a clean one, and that hits and observations move independently. What this does NOT do is fix anything. The rate has to be observed on a real session with a panel open before a fix is chosen, because the obvious fix — merging the two acquisitions — puts the composite work back under the emulator lock, which is exactly the regression v2.3.0 fixed. Verified: fmt, workspace clippy with the feature OFF, `debug-hooks` and `full` frontend combos, both wasm32 invocations, rustdoc, and the frontend suite at 522. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Review on #408 found two problems with `.github/scripts/apt-install-retry.sh`, and both would have made it worse than nothing on the exact path it exists to protect. ELEVATION MUST BE OUTERMOST, WITH `timeout` INSIDE IT. The original had `timeout` on the outside, which sends the signal to the elevation helper rather than to `apt-get`. The helper may not forward it, leaving `apt-get` orphaned while still holding the dpkg lock — so every subsequent retry fails on the lock rather than on the original problem. A retry loop that guarantees its own retries fail is worse than no retry loop. `DEBIAN_FRONTEND=noninteractive`, for the same class of reason. A package that prompts for configuration blocks on stdin that will never arrive in CI, burning the whole timeout budget waiting for a human who is not there — the exact failure this script bounds, arriving through a door it had left open. Passed explicitly because the environment is scrubbed on elevation. Re-verified with the same stubs: fail-fail-succeed still names the attempt it succeeded on and exits 0. shellcheck clean. Both are recorded in the plan beside the item rather than only here, because the lesson generalises past this script: wrapping a command for reliability puts the wrapper in the signal path, and the wrapper's own failure modes then belong to the thing it was protecting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

coderabbitai · 2026-08-19T13:29:08Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 411f25a1-ce95-448c-9be7-db8e635d7e7f

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

Adds a debug-hooks-gated measurement for potential “panel/screen frame skew” caused by the frontend’s two separate emulator-lock acquisitions in the needs_nes render path, and carries the missed apt-install-retry.sh ordering/CI-hardening fix into this branch.

Changes:

Instrument the needs_nes render path to compare Nes::cycle() at the framebuffer-copy acquisition vs the UI acquisition, tracking hits and observations and surfacing the rate in the Perf panel (debug-only).
Add RenderPerf/PerfView plumbing and tests for the new counters under debug-hooks.
Fix apt provisioning wrapper to put elevation outermost and to enforce noninteractive installs.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`to-dos/plans/v2.3.9-crucible-plan.md`	Documents the two apt-script ordering defects and rationale.
`crates/rustynes-frontend/src/perf.rs`	Adds lock-gap counters, stats plumbing, and debug-only tests.
`crates/rustynes-frontend/src/debugger/perf_panel.rs`	Displays the lock-gap skew rate (debug-only) in the Perf panel.
`crates/rustynes-frontend/src/app.rs`	Captures `Nes::cycle()` at both lock acquisitions and records observations.
`.github/scripts/apt-install-retry.sh`	Adjusts apt retry command structure (sudo ordering + noninteractive installs).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…rrections Four findings from the #409 review, one of them a real defect in the instrument this PR exists to add. `RenderPerf::clear()` did not reset the new `lock_gap_*` counters. It is documented as a regime-change reset, so a ROM change or pacing-regime change would have cleared every sample ring and left the numerator and denominator of the skew rate standing — mixing two populations into one percentage and presenting it as a single measurement. That is exactly the defect the `wait` series had before it, and the comment explaining that fix sits four lines above the place the new counters were missing from. Adding to `stats()` and forgetting `clear()` is evidently the shape of this mistake; the test now pins it and the mutation fails without the reset. The `cycle_at_fb` comment said the reading is taken "at the moment the framebuffer is copied". It is taken on ACQUIRING the lock, a few lines earlier. Equivalent — the emulator cannot advance while the guard is held, so every reading inside that scope names the same frame — but "close enough to be misleading" is how prose stops being checked, and a reviewer asking means the next reader would have. The comment now says where it is read and why that is the same thing. `DEBIAN_FRONTEND` is now set with `env` rather than as a bare assignment to the elevation helper. Both work on a standard runner, but the bare form additionally requires SETENV in sudoers, and on a stricter host it fails by refusing to run at all — breaking the wrapper rather than degrading it. Raised independently by both reviewers, which is usually a sign the point is real. And the counters use plain `+= 1`: one increment per redraw cannot overflow a `u64` in any run that terminates, so `saturating_add` implied a bound worth reasoning about where there is none. Verified: shellcheck, the stub replay (fail-fail-succeed still names its attempt and exits 0), fmt, workspace clippy, frontend under `debug-hooks` at 523 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

doublegate · 2026-08-19T13:50:25Z

Both accepted, fixed in `6c99d5af`

sudo env DEBIAN_FRONTEND=... rather than a bare assignment — adopted. You and Copilot raised this independently, which is usually a sign the point is real. Both forms work on a standard runner, but the bare assignment additionally requires SETENV in sudoers, and on a stricter host it fails by refusing to run at all — breaking the wrapper rather than degrading it, which is the wrong failure mode for a script whose only job is keeping a flaky step from failing.

+= 1 rather than saturating_add — adopted. One increment per redraw cannot overflow a u64 in any run that terminates, so saturating_add implied a bound worth reasoning about where there is none. Noted at the site so it does not get "hardened" back.

Also fixed from the Copilot threads on this PR, and worth flagging here because it is the substantive one: RenderPerf::clear() did not reset the new counters. It is a regime-change reset, so a ROM change would have cleared every sample ring and left the skew rate's numerator and denominator standing — two populations in one percentage. Now pinned by a test and mutation-checked.

Good to see this run complete, incidentally — the three previous runs on #404, #406 and #407 were all blocked by the datacloud_telemetry PreToolUse hook and could not read their diffs at all.

github-actions · 2026-08-19T13:52:09Z

Antigravity review (Gemini via Ultra)

This PR introduces a performance metric to measure frame skew caused by a lock race during rendering and corrects a deadlock in the CI package installation script by restructuring the sudo and timeout commands.

Blocking issues

None found.

Suggestions

crates/rustynes-frontend/src/perf.rs (line 360): Using &mut self in a const fn (record_lock_gap) was only stabilized in Rust 1.83.0. If the project's Minimum Supported Rust Version (MSRV) is lower, this will break the build. Consider dropping const since the method is not invoked in a const context.
.github/scripts/apt-install-retry.sh (line 46): Security hardening: add -- before "$APT_PACKAGE" (apt-get install -yq -- "$APT_PACKAGE") to guarantee it is evaluated as a positional argument. This prevents option-flag injection if the environment variable happens to start with a hyphen.

Nitpicks

crates/rustynes-frontend/src/debugger/perf_panel.rs (line 267): The #[expect(...)] attribute requires Rust 1.81.0+. If supporting older compilers is a priority, use #[allow(...)] instead.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

…the gate for it `[Unreleased]` was empty while three merged PRs carried user-visible change: #407's Divergence Lens (a whole new tool panel and the v2.3.8 "Parallax" marquee), #409's two-acquisition lock-race measurement, and #410's per-game Latency Oracle persistence. The file that is supposed to be the single source of truth for user-visible change did not mention any of them. Backfilled now rather than at cut time. The reasons are recoverable from the commit bodies today and would have to be reconstructed from diffs later, which is how a release section ends up describing what changed instead of why. The obvious gate -- a PR touching `crates/*/src/**` must also touch `CHANGELOG.md` -- was measured against the last 50 first-parent merges before being proposed, per this release's own bar that a CI change is demonstrated against real history rather than argued. It would have gone red on 8 of them. Three are the genuine misses above. The other five are this repo's normal workflow: features land bare and the release-cut PR composes the whole section at once (the probe engine, the Latency Oracle, the RAM Atlas, the throttle instrument), plus one docs PR that touched only `//!` preambles. A 62% false-positive rate against the project's own history. A gate that fires on the normal workflow is suppressed within a week, and a suppressed gate is worse than no gate because it still reads as coverage. Rejected as specified, recorded with its numbers per the convention `docs/performance.md` sets for rejected optimizations. The measurement also relocates the defect. The failure mode is not "a feature landed without an entry" -- at the moment each of those PRs merged, nothing was wrong. It is that v2.3.8 landed its marquee and was never cut, so the cut PR that writes the section never ran and v2.3.9 opened on top of an empty [Unreleased]. No per-PR gate addresses that. What would is a release-ceremony check keyed on the cut, which is named in the plan as a decision rather than built here.

Review is right that this belongs elsewhere. The paragraph explained that #407, #409 and #410 merged without an entry -- a fact about how this document is maintained, not about what changed for a user. The CHANGELOG's own header already says it is the record of user-visible change, so a section describing its gaps is the one thing in it that is not. It is not lost: the full account, including the measured rejection of the per-PR gate and its 62% false-positive rate, is item D of the v2.3.9 plan, and the reasoning is in this branch's commit bodies. The companion nitpick -- that the entries read like architecture decision records rather than concise release notes -- is declined. That is this project's CHANGELOG voice, not an accident of this PR: every released section explains the mechanism and what was believed before the measurement. Matching a generic house style would make these entries inconsistent with the file they are joining.

…the gate for it `[Unreleased]` was empty while three merged PRs carried user-visible change: #407's Divergence Lens (a whole new tool panel and the v2.3.8 "Parallax" marquee), #409's two-acquisition lock-race measurement, and #410's per-game Latency Oracle persistence. The file that is supposed to be the single source of truth for user-visible change did not mention any of them. Backfilled now rather than at cut time. The reasons are recoverable from the commit bodies today and would have to be reconstructed from diffs later, which is how a release section ends up describing what changed instead of why. The obvious gate -- a PR touching `crates/*/src/**` must also touch `CHANGELOG.md` -- was measured against the last 50 first-parent merges before being proposed, per this release's own bar that a CI change is demonstrated against real history rather than argued. It would have gone red on 8 of them. Three are the genuine misses above. The other five are this repo's normal workflow: features land bare and the release-cut PR composes the whole section at once (the probe engine, the Latency Oracle, the RAM Atlas, the throttle instrument), plus one docs PR that touched only `//!` preambles. A 62% false-positive rate against the project's own history. A gate that fires on the normal workflow is suppressed within a week, and a suppressed gate is worse than no gate because it still reads as coverage. Rejected as specified, recorded with its numbers per the convention `docs/performance.md` sets for rejected optimizations. The measurement also relocates the defect. The failure mode is not "a feature landed without an entry" -- at the moment each of those PRs merged, nothing was wrong. It is that v2.3.8 landed its marquee and was never cut, so the cut PR that writes the section never ran and v2.3.9 opened on top of an empty [Unreleased]. No per-PR gate addresses that. What would is a release-ceremony check keyed on the cut, which is named in the plan as a decision rather than built here.

Review is right that this belongs elsewhere. The paragraph explained that #407, #409 and #410 merged without an entry -- a fact about how this document is maintained, not about what changed for a user. The CHANGELOG's own header already says it is the record of user-visible change, so a section describing its gaps is the one thing in it that is not. It is not lost: the full account, including the measured rejection of the per-PR gate and its 62% false-positive rate, is item D of the v2.3.9 plan, and the reasoning is in this branch's commit bodies. The companion nitpick -- that the entries read like architecture decision records rather than concise release notes -- is declined. That is this project's CHANGELOG voice, not an accident of this PR: every released section explains the mechanism and what was believed before the measurement. Matching a generic house style would make these entries inconsistent with the file they are joining.

…the gate for it `[Unreleased]` was empty while three merged PRs carried user-visible change: #407's Divergence Lens (a whole new tool panel and the v2.3.8 "Parallax" marquee), #409's two-acquisition lock-race measurement, and be the single source of truth for user-visible change did not mention any of them. Backfilled now rather than at cut time. The reasons are recoverable from the commit bodies today and would have to be reconstructed from diffs later, which is how a release section ends up describing what changed instead of why. The obvious gate -- a PR touching `crates/*/src/**` must also touch `CHANGELOG.md` -- was measured against the last 50 first-parent merges before being proposed, per this release's own bar that a CI change is demonstrated against real history rather than argued. It would have gone red on 8 of them. Three are the genuine misses above. The other five are this repo's normal workflow: features land bare and the release-cut PR composes the whole section at once (the probe engine, the Latency Oracle, the RAM Atlas, the throttle instrument), plus one docs PR that touched only `//!` preambles. A 62% false-positive rate against the project's own history. A gate that fires on the normal workflow is suppressed within a week, and a suppressed gate is worse than no gate because it still reads as coverage. Rejected as specified, recorded with its numbers per the convention `docs/performance.md` sets for rejected optimizations. The measurement also relocates the defect. The failure mode is not "a feature landed without an entry" -- at the moment each of those PRs merged, nothing was wrong. It is that v2.3.8 landed its marquee and was never cut, so the cut PR that writes the section never ran and v2.3.9 opened on top of an empty [Unreleased]. No per-PR gate addresses that. What would is a release-ceremony check keyed on the cut, which is named in the plan as a decision rather than built here.