[JoeMatt]

A frame-advance request could overtake an earlier queued controller/option change if it arrived between the owner’s command drain and frame-budget read. This caused the Pro Controller assertion in #1031’s ThreadSanitizer job to intermittently run a frame with the old keypad mapping.

Publish the advance budget through the same FIFO as commands, and calculate its completion target on the owner. A gated regression waits for the request to be published before releasing the owner; it fails against the old implementation and passes with the fix. Existing in-flight-frame accounting remains covered.

Validation on Studio: 54/54 ThreadSanitizer tests and 158/158 release tests passed. Scheduler and real-core/controller tests each passed 15 consecutive sanitizer runs; after tightening the regression’s publication handshake, the scheduler test was rebuilt and checked again. Independent review found no remaining issues. No sanitizer suppressions.

Refs #967

CI follow-up: the macOS runner exposed scheduler-sensitive timing assertions. The wait-hook test now holds its command until three callbacks arrive (five-second failure deadline), including a deliberately slow first UI callback. Sleep still asserts its no-early-return contract; upper sleep latency stays in the informational timer report because OS scheduling/coalescing is outside that contract. A fast queued call is checked as fast only when its measured wait is below the busy threshold.

Validation: reproduced both original macOS failures with injected coarse sleep/slow callback; updated test passes those conditions, plus 10/10 TSan runs. Fault controls fail as intended when sleep returns immediately or the wait hook invokes only once. Independent review: no findings.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-20T00:27:53.953512Z	c6b84a4	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

Regression: linux-arm64

Regression Test Results

ROM	Status	Details	Diff
jagniccc	✅ PASS	0 pixels differ	-
yarc	✅ PASS	0 pixels differ	-
jagniccc (determinism)	✅ PASS	identical across runs	-
yarc (determinism)	✅ PASS	identical across runs	-
jagniccc (frameskip)	✅ PASS	skip=0 matches skip=3	-
yarc (frameskip)	✅ PASS	skip=0 matches skip=3	-
jagniccc (save state)	✅ PASS	round-trip matches	-
yarc (save state)	✅ PASS	round-trip matches	-
jagniccc (rewind)	✅ PASS	rewind matches	-
yarc (rewind)	✅ PASS	rewind matches	-

Platform: Linux aarch64

_{Updated by CI at 2026-10-20T01:35:34.867Z}

[github-actions]

Regression: macos-arm64

Regression Test Results

ROM	Status	Details	Diff
jagniccc	✅ PASS	0 pixels differ	-
yarc	✅ PASS	0 pixels differ	-
jagniccc (determinism)	✅ PASS	identical across runs	-
yarc (determinism)	✅ PASS	identical across runs	-
jagniccc (frameskip)	✅ PASS	skip=0 matches skip=3	-
yarc (frameskip)	✅ PASS	skip=0 matches skip=3	-
jagniccc (save state)	✅ PASS	round-trip matches	-
yarc (save state)	✅ PASS	round-trip matches	-
jagniccc (rewind)	✅ PASS	rewind matches	-
yarc (rewind)	✅ PASS	rewind matches	-

Platform: Darwin arm64

_{Updated by CI at 2026-10-20T02:19:23.716Z}

[github-actions]

🌙 Kimi review

Verdict: The fix is correct and the regression test genuinely discriminates old vs new behavior; one edge-case defect in the new return path.

Findings:

• app/src/emu_thread.cpp:126 — if (!target) return false; conflates abandonment with a legitimate zero target. target is advanceDone_ + advanceLeft_ + n, computed after the lambda was accepted and run. It is 0 whenever n == 0 and no frames have run or been budgeted yet. So advance(0) on a fresh thread now queues a no-op command and then reports failure, where the old code returned true immediately (target advanceDone_ + 0 satisfied trivially by the wait predicate). Worse, the result is history-dependent: advance(0) returns false before any frame has run and true after. If the intent is to detect a rejected call(), the return value can't carry that signal — call() needs to report acceptance separately (e.g., an out-param or an optional), or the zero case needs to be special-cased before the check. If advance(0) is a documented non-call, this is cosmetic; the diff doesn't establish that.

Notes on things I checked and am not flagging:

• The regression test's gating logic holds up. With the old code, the UI publishes advanceLeft_ directly while the owner is held after its drain; on release the owner runs the frame before its next drain, so the frame misses the queued mutation and frameSawCommand is false. With the fix, the budget is behind the mutation in the FIFO and the owner sees budget only after applying it. The advancePublished probe correctly distinguishes the two (nonzero advanceLeft_ vs. queue depth ≥ 2), and the mutation is sequenced into the queue before advance()'s lambda because post precedes the caller thread's spawn.
• Completion accounting is sound: reading advanceDone_ + advanceLeft_ on the owner correctly excludes a frame already in flight (it has already subtracted its budget and will add to advanceDone_), and moving both the read and the fetch_add onto the owner thread removes the cross-thread ordering question entirely. Concurrent advance() callers serialize through the queue, so targets can't interleave.
• No missed-wakeup risk: the budget publication now happens inside call(), which must already notify the loop to be executed at all, and the wait predicate (advanceDone_ >= target) is atomic-based, so completion between call()'s return and advanceCv_.wait is still caught.
• The pre-existing early-release condition advanceLeft_.load() == 0 in the wait predicate is unchanged by this diff; out of scope here.
• The C89/memory-access/state.h house rules don't apply — this is the C++ frontend harness, no emulated-memory or savestate-format changes, and no hardware timing claims to ground-check.

No other substantive issues.

[github-actions]

_{An automated review was posted after this notice; the reviewer is working again.}

[github-actions]

Regression: linux-x64

Regression Test Results

ROM	Status	Details	Diff
jagniccc	✅ PASS	0 pixels differ	-
yarc	✅ PASS	0 pixels differ	-
jagniccc (determinism)	✅ PASS	identical across runs	-
yarc (determinism)	✅ PASS	identical across runs	-
jagniccc (frameskip)	✅ PASS	skip=0 matches skip=3	-
yarc (frameskip)	✅ PASS	skip=0 matches skip=3	-
jagniccc (save state)	✅ PASS	round-trip matches	-
yarc (save state)	✅ PASS	round-trip matches	-
jagniccc (rewind)	✅ PASS	rewind matches	-
yarc (rewind)	✅ PASS	rewind matches	-

Platform: Linux x86_64

_{Updated by CI at 2026-10-20T02:42:31.380Z}

[github-actions]

🌙 Kimi review

Verdict: the fix is sound — publishing the budget through the command FIFO closes the drain/budget-read race correctly. One minor edge-case finding, nothing blocking.

Findings

1. app/src/emu_thread.cpp:126 — if (!target) return false; uses 0 as the call-abandoned sentinel, but 0 is also a legitimately computed target: advance(0) when advanceDone_ + advanceLeft_ == 0 (cold start) now reports failure, where the old code computed target = advanceDone_, found the wait predicate immediately true, and returned success. With n >= 1 the sum can't be 0 short of 2^64 frames, so this only bites the degenerate advance(0) call. If advance(0) isn't a supported invocation, that's fine — but then the sentinel still can't distinguish "abandoned" from "accepted, zero work," so consider having call report acceptance out-of-band (e.g. an optional/bool pair) rather than overloading the value.

Checked and clear

• Target computation inside the queued lambda is correct: commands run on the owner between frames, so advanceDone_ + advanceLeft_ is stable at that point, and FIFO ordering guarantees earlier-posted mutations (and earlier advance requests) are already accounted for. Multiple concurrent advance() callers serialize correctly through the queue.
• No missed wakeup: the advanceCv_ predicate is evaluated under mu_ after call() returns, and the budget increment necessarily precedes it.
• The old cmds_.notify() is subsumed by call()'s normal post-and-notify path.
• The advanceLeft_ == 0 escape in the wait predicate is pre-existing context, untouched here, and remains covered by testAdvanceExact.
• Test access plumbing is complete: befriending both EmuThread and the command-queue class is required (friendship isn't transitive through cmds_), and both friend declarations are present, in the right namespace. The gated regression's detection logic is sound for both implementations — in the fixed code the owner is held so advanceLeft_ stays 0 and detection falls to queue depth; in the old code advanceLeft_ goes nonzero from the UI thread, which is exactly the fault it demonstrates.
• The testWaitHook rework keeps a bounded failure deadline (5 s) and resets observations before the blocked-call phase; the fault-control reasoning holds.

One description-vs-code note, not a defect: the follow-up says sleep "still asserts its no-early-return contract," but testPacingWallClock now passes a 1 ms sleep measured at 0.9 ms — a 100 µs early return slips the gate. That's a defensible timer-granularity tolerance (and it's honestly commented in the code), but the description overstates what the check enforces.

[github-actions]

App packages

Built from 4a230e4 (workflow run).

Platform	Download	Size
Linux x86_64: Flatpak bundle (.flatpak)	app-package-flatpak	19.3 MB
Linux aarch64: .AppImage + .deb	app-package-linux-aarch64	44.5 MB
Linux x86_64: .AppImage + .deb	app-package-linux-x86_64	45.9 MB
macOS (universal): .zip	app-package-macos-universal	30.3 MB
Windows ARM64: portable .zip + -setup.exe	app-package-windows-arm64	41.8 MB
Windows x64: portable .zip + -setup.exe	app-package-windows-x64	45.4 MB

Downloading an artifact needs a GitHub login. macOS: Developer ID signed and notarized (PRs from forks get
no signing secrets and stay ad-hoc signed: right-click > Open, or xattr -dr com.apple.quarantine "Virtual Jaguar.app").
Windows: not code-signed yet, SmartScreen may warn. Linux AppImage: chmod +x it.

_{Updated by CI at 2026-10-20T05:42:08.653Z}