Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions .github/release-notes/v2.3.7.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
RustyNES **v2.3.7 "Overtone"** adds the APU counterpart of Pixel Provenance: point at a moment in the frame and read *why it sounds like that*. An overtone is the structure inside a sound that a single pitch reading throws away, and that is the gap this closes — the Audio Scope already plotted the waveform, the Audio Mixer already set the gains, and nothing at all linked a sample back to the instruction that caused it.

The release's real subject is narrower and less comfortable: **the trap the feature inherited, and the three further places it turned out to be hiding.**

**The accuracy contract is verified, not asserted.** Both `rustynes-apu` and `rustynes-core` change here, so AccuracyCoin was re-run rather than reasoned about: **141/141 (100.00%)** on the authoritative RAM decoder, nestest 0-diff.

## Audio Provenance

Two halves, deliberately shaped as the audio mirror of Pixel Provenance.

**Register attribution** answers *what wrote this register, and from which instruction* — the last write to each of `$4000-$4017` with its value, the CPU cycle, the PC, and whether an instruction or the APU's own reset performed it. Last write, not a history: a ring would need a retention policy nobody has a principled value for, and the Event Viewer already keeps the per-frame write *sequence*. This keeps the per-register *cause*, which it does not.

**The mix trace** answers *what were the channels actually doing* — one record per CPU cycle carrying the mixed sample, the expansion contribution, and all five channels' raw pre-mix outputs. Per CPU cycle rather than per output sample, because that is the cadence at which the mix is genuinely computed; `blip` decimates to 44.1 kHz afterwards, so a per-sample trace would describe the decimator rather than the chip.

Channel values are the **raw** pre-mix outputs, not the values after the frontend's mixer gains. A record scaled by the user's sliders would describe the slider rather than the hardware.

Surfaced at **Tools → Audio → Audio Provenance**. Output-only, runtime-default-off, and not serialized, so the deterministic audio contract is unaffected whether it is armed or not.

## The trap it inherited, closed up front — then found three more times

Pixel Provenance shipped **non-functional for four releases** because run-ahead's per-frame rollback cleared its store *after* the visible frame was harvested and *before* the frontend released the emulator lock. The UI could never observe a populated record, and a comment two lines above the clear asserted the opposite.

Audio Provenance rides the identical rollback. So the carry landed **in the same change as the feature**, not after a bug report — `take_audio_provenance` / `put_audio_provenance` around `restore_quiet` in `RunAhead::finish`, with a control test proving a plain run populates the trace, so a failure of the run-ahead test cannot be misread as a bad assertion.

That would have been the end of it, except the fix's own enumeration was wrong.

### Running the Latency Oracle or the RAM Atlas emptied both panels

`Nes::restore_inner` clears both provenance stores — correct for a genuine timeline change, wrong for a restore that puts back the state the user is still looking at. `rustynes-probe` has **three** such restores, and none of them used the stash:

| Path | How often |
| --- | --- |
| `Probe::run_uncounted` | once per trial, and a latency measurement runs up to **21** |
| `latency::measure_in_place` | the final restore, outside every per-trial guard |
| the RAM Atlas panel's `TimelineGuard` | once per observation |

Both stores are **cumulative**. "Which instruction last wrote this" can point thousands of frames back — a palette byte written at level load, a `$4008` linear-counter reload written once during init — so the records were not rebuilt by the next frame. They were gone for the session.

Two things let this through, and neither was carelessness:

1. **The v2.3.6 fix enumerated one caller, not the mechanism.** `RunAhead::finish` was the path the report named; the fix was correct there and stopped there. `docs/pixel-provenance.md` then called run-ahead "the one caller that needs the exception" — a correct rule with an incomplete list under it.
2. **The test named for the contract could not see the breach.** `measure_in_place_restores_the_live_timeline` compares `nes.snapshot()` before and after, and provenance is deliberately **not** in the snapshot. It asserted something strictly weaker than the contract it is named for, and passed throughout.

Closed by moving the stash into a shared `TrialGuard` — the guard that already carried rewind capture across a trial, for the same underlying reason: state that lives outside the save state is not carried by a snapshot round trip. Four independent mutations pin it, one per store and one per site, so a fix that put back only one store or guarded only one restore fails.

## `$4014` and `$4016` were documented as attributed, and were not

The bus handles both without routing through `Apu::write_register` — `$4014` because OAM DMA is a bus-level burst, `$4016` because it is the controller strobe. So the two writes most worth attributing produced empty slots, under docs that said otherwise. Both are now recorded at the point the bus handles them.

## Two defects found by measurement, not by reading

**The APU throughput bench reshaped the plumbing three times.** `apu_throughput`, built for this release, caught three regressions that were invisible in the diff and that no amount of reading would have found. The bench itself had to be corrected first: it omitted the end-of-cycle pair the bus really performs, which was ~23% of true per-cycle cost.

**Fuzzing the save-state parse boundary found four panics where hand-tracing found one.** The VRC7 OPLL parse was walked by hand, one panic was found and fixed, and a maximally-hostile all-`0xFF` payload was used to confirm. That payload *concealed* a second defect: all-ones set `update_requests` to all-ones, which forced a recompute that hid an `eg_shift` panic. A randomized sweep found three more in minutes. Save states are untrusted input.

## Also fixed

- **VRC7 save states dropped the live FM synthesizer**, so rewind, netplay rollback, and save-state restore all resumed the music from whatever envelope and phase state happened to be held. Banking, IRQ, mirroring and PRG-RAM had always round-tripped correctly.
- **The browser demo applied no per-game header corrections.** Every mapper, mirroring, and region override the desktop frontend applies from the per-game database was skipped in the wasm build.
- **Rad Racer's roadside artifact**, where the PPU spliced a hybrid address from a stale `v` rather than the live one.
- **No CI job carried a timeout**, so a single hung job silently skipped a release for five hours — every job inherited GitHub's six-hour default, and the failure presented as a workflow that had quietly decided not to run.

That fix covered `ci.yml`, and the remaining gap was found the same way the first one was: by blocking this release. `Clippy Security Lints` hung for over two hours in a setup step during the cut, on a job whose observed runtime is two to three minutes. A sweep found **six** unbounded workflows in total — `security.yml`, `android.yml`, `ios.yml`, `web.yml`, `antigravity-review.yml`, and `release-auto.yml`, the release workflow itself. All are bounded now, and the sweep across `.github/workflows/` comes back empty.
- A stale comment in `security.yml` justifying a prebuilt-binary install that no longer applied.

## Upgrading

No action required. Audio Provenance is off by default and costs the shipped default nothing when unarmed. Save states, movies, and netplay replays are unchanged in format; the VRC7 fix means a state saved by v2.3.7 carries FM synthesizer state a v2.3.6 state did not, so v2.3.6 will ignore that trailing data rather than fail.
2 changes: 2 additions & 0 deletions .github/workflows/android.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ jobs:
cross-build:
name: NDK cross-build + UniFFI bindings
runs-on: ubuntu-latest
timeout-minutes: 45
steps:
- uses: actions/checkout@v7
with:
Expand Down Expand Up @@ -100,6 +101,7 @@ jobs:
gradle-bundle:
name: Gradle bundle foss+play release (best-effort packaging)
runs-on: ubuntu-latest
timeout-minutes: 45
# Packaging is informational: the cross-build job above is the real link
# gate. R8/Compose packaging pulls the full Android SDK, so a transient
# tooling hiccup here must not fail the merge.
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/antigravity-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ jobs:
group: agy-review-${{ github.event.pull_request.number || github.event.issue.number }}
cancel-in-progress: false
runs-on: [self-hosted, agy]
# Bounded harder than the hosted jobs, not softer: this runs on the
# maintainer's own hardware, so a hung run holds a real machine rather than
# a disposable VM. Observed runtime is 1-3 minutes.
timeout-minutes: 30
steps:
# Check out the DEFAULT BRANCH, never the PR head. This job runs the checked-out
# `scripts/agy-review.sh` on a self-hosted runner with a token in the environment,
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/ios.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ jobs:
testflight:
name: Build xcframework + TestFlight upload
runs-on: macos-latest
timeout-minutes: 60
# The unattended SCHEDULED refresh is a clean no-op until signing is
# provisioned: the maintainer sets the `IOS_SIGNING_READY` repo variable to
# "true" once the App Store Connect key + fastlane match secrets exist, so a
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/release-auto.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ jobs:
github.event.workflow_run.conclusion == 'success' &&
github.event.workflow_run.event == 'push'
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`.
timeout-minutes: 15
outputs:
should_release: ${{ steps.decide.outputs.should_release }}
tag: ${{ steps.decide.outputs.tag }}
Expand Down
13 changes: 13 additions & 0 deletions .github/workflows/security.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,21 @@ env:
permissions:
contents: read

# Every job carries an explicit `timeout-minutes`, for the reason PR #400
# established for `ci.yml` and this file was missed by: without one a job
# inherits GitHub's SIX-HOUR default, and a hung job then blocks a release while
# reporting nothing at all. That is not hypothetical here — `Clippy Security
# Lints` hung for over two hours during the v2.3.7 cut, in a setup step, on a
# job whose observed runtime is 2-3 minutes, and the release waited on it.
#
# Budgets are generous multiples of observed runtime (audit and deny complete in
# under a minute, clippy in 2-3), because the purpose is to bound a hang, not to
# police normal variance on a cold cache.
jobs:
audit:
name: Dependency Audit
runs-on: ubuntu-latest
timeout-minutes: 15
steps:
- uses: actions/checkout@v7
with:
Expand All @@ -72,6 +83,7 @@ jobs:
deny:
name: Cargo Deny Check
runs-on: ubuntu-latest
timeout-minutes: 15
steps:
- uses: actions/checkout@v7
with:
Expand All @@ -87,6 +99,7 @@ jobs:
clippy-security:
name: Clippy Security Lints
runs-on: ubuntu-latest
timeout-minutes: 25
steps:
- uses: actions/checkout@v7
with:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/web.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ jobs:
build:
name: build demo + docs
runs-on: ubuntu-latest
timeout-minutes: 30
steps:
- uses: actions/checkout@v7
with:
Expand Down Expand Up @@ -175,6 +176,7 @@ jobs:

deploy:
name: deploy to GitHub Pages
timeout-minutes: 15
# Only publish from main / manual dispatch — PRs build + size-gate
# but never deploy.
if: github.event_name != 'pull_request'
Expand Down
Loading