Skip to content

Fix AudioOut release-notifier lost wakeup - #855

Closed
thehobbit85 wants to merge 1 commit into
seedds:mainfrom
thehobbit85:codex/fix-audio-release-wakeup
Closed

thehobbit85 wants to merge 1 commit into
seedds:mainfrom
thehobbit85:codex/fix-audio-release-wakeup

Conversation

@thehobbit85

@thehobbit85 thehobbit85 commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

AudioOut can consume PCM and place a buffer on its released queue while its notifier sleeps without signaling the guest. This caused the earlier #691 sanitizer failure; the same production code is on main.

The notifier disarmed with a release-only store, then rechecked a separate pending flag. A producer could set pending, observe the previous armed value and omit its semaphore post; the notifier could miss that publication and sleep. Local ASan reproduction captured correct PCM and a queued release with pending=1, armed=0. Temporary phase instrumentation confirmed the helper remained at its semaphore wait; a diagnostic post immediately delivered the release. That instrumentation is not in this PR.

Disarm using an acquire-release exchange. An earlier producer exchange is acquired before the pending recheck; otherwise the producer or the notifier's re-arm owns the semaphore post. This changes no callback timing, PCM processing, queue limits, logging policy or wait deadlines. The original 22 fixture checks remain, with four added checks for 100,000 individual handoffs and diagnostic failure details. The reference/symptom entry explains the cause; a narrow impact rule maps the asynchronous notifier to AudioOut handles.

Current integration and validation

Rebased the unchanged repair onto pinned main cd859c38a8f8890ddbb57a23d6f45e23ba9e6651, producing ccb4029a252d2840a3d2545bea6acaa07bfc4a83. Main still uses the release-only disarm store, so the wakeup fix remains needed. Canonical codex/fix-audio-release-wakeup tracks the same existing fork/codex/fix-audio-release-wakeup branch. Main ancestry, range-diff and whitespace checks pass; all applicable discussion/review/inline feedback was read with pagination and no new request was found.

Fresh on this head: all 26 native audio-statistics checks and 17 documentation/usage tests pass. Audio production, fixtures, headers, generated profile and target inputs match freshly checked #691 d0e54fa7a073ad5d3464da1de11a08b55df3efdb; its native, ASan/UBSan and TSan statistics runs each pass 26 checks. Nine LLVM-IR comparisons prove that the two new, unused header declarations do not alter audio code generation. These checks support reuse of the original stress evidence below, without claiming that the 300-run campaign was repeated.

Fresh game discovery still finds only Z-A, without a smoke route or baseline; four selected games remain unavailable. No game or extract rebuild was run. This pass's shared upstream graphics/filesystem coverage is recorded separately; historical shader-only comparisons do not establish gameplay. Local Xcode27.0/Metal.921 and unavailable LLVM23 limitations remain. Current-head CI is pending; previous-head successes are historical.

Historical repair validation

Validation on e0bdbbc49ca00b5f65a146083cf3fee934c2d18a, based on main c0f341335b56ee61af5827b51349e68bddb65c61:

  • The old production code fails the new stress case naturally (run 25, handoff 45,749, correct PCM but no event). Earlier unmodified-fixture diagnostic runs also reproduced it.
  • Fixed ASan/UBSan: 300 complete runs, 26 checks each and 33 million individual buffer releases, zero failures. TSan passes the statistics and 100,000-buffer SPSC fixtures.
  • Native audio lifecycle, statistics, SPSC and device fixtures pass; suspend/resume and induced device-stall recovery pass. Native host build passes with existing graphics warnings. Documentation/usage tests pass (17), compile database has 212 sources, and diff checks pass. Independent source/ordering review passed; optimized code retains the acquire RMW.
  • The required five-game harness check exits 2, because Hades, Scarlet, Engage and Luigi's Mansion 3 Mac builds are unavailable. Z-A is the only discovered Mac build and lacks a smoke route. These choices cover manifest-backed AudioOut callers, event-driven Wwise refill and output lifecycle. No gameplay ran, no extracts were rebuilt, and no game regression pass is claimed. Static impact reports Hades and Scarlet affected, 117 profiles unknown; shader/graphics/profile inputs are unchanged.
  • Local Xcode 27.0/Metal .921 differs from supported CI Xcode 27.2/.922; LLVM 23 clang-tidy is unavailable locally. All seven exact-head CI checks subsequently passed, including LLVM 23; verified workflow links are recorded below.

The shared repair is separate from #691's documentation feature and #221's controller feature; both now include it as a CI prerequisite. No baseline expansion, suppression, blind retry or timeout increase is used.

Available-game follow-up: a separate Z-A check completed successfully in 25.9 seconds. It reused the same compatible cached chain for both revisions (zero shader build time), freshly compared 28,603 shaders and checked 26,051 pipeline interfaces: no changed MSL/reflection/variants, newly failing shaders or interface errors. There is no smoke route, so no boot, gameplay, audio-runtime, frame-time or device result is claimed. The four unavailable games remain unvalidated. Existing caches and the pinned-toolchain policy were preserved.

The harness report below uses its standard check (full) label; its empty boot/performance columns and chain-only timing delimit the actual coverage:

regress.py check (full): PASS

Baseline c0f341335b56ee61af5827b51349e68bddb65c61 c0f3413, candidate e0bdbbc49ca00b5f65a146083cf3fee934c2d18a e0bdbbc; Apple M2 Max, macOS 26.6, Xcode 27.0; 25.9 s

game status shaders (base → cand) newly failing newly building changed MSL interface (new / known) boot fps p50 frame p99 ms peak MB prepare s new RED
LegendsZA 0100F43008C44000 PASS 28603/28603 → 28603/28603 0 0 0 0 / 0 – – – – – –

LegendsZA 0100F43008C44000: PASS

  • time: chain 25.6 s

Current published head ccb4029a252d2840a3d2545bea6acaa07bfc4a83 has all 7 applicable CI checks successful, including every applicable workflow and job. Verified on 2026-11-01 against exact head/run/job provenance and recorded main cd859c38a8f8890ddbb57a23d6f45e23ba9e6651. Historical results and local limitations above remain separate. Workflows: lint-c, tests.

@thehobbit85
thehobbit85 force-pushed the codex/fix-audio-release-wakeup branch from db8ea2f to ccb4029 Compare November 1, 2026 20:30
seedds added a commit that referenced this pull request Nov 1, 2026
…855)

The notifier cleared notify_armed with a release-only store, then read
notify_pending. A producer could set pending and see the old armed value, so
it did not post, while the notifier read the old pending and went back to
sleep: PCM played and the buffer was released, but the guest's event never
came. Apple-silicon code is stlr then ldapr, which permits this.

Disarm with an acquire-release exchange. If the producer's exchange came
first, the notifier acquires its pending store; otherwise the producer posts,
or finds the notifier already re-armed with its own post.

t_audio_stats gains 100,000 individual handoffs. 400 runs each, old notifier
against the fix: plain 8 failed against 0, ASan/UBSan 74 failed against 0.

Co-authored-by: Eliran Zach <4012041+thehobbit85@users.noreply.github.com>
@seedds

seedds commented Nov 1, 2026

Copy link
Copy Markdown
Owner

Thanks, this is landed on main as c76a424, with you credited as co-author. The code, test and docs are unchanged from this PR.

I checked it independently before landing:

  • Disassembly: the old event_notify is stlr followed by ldapr, which permits the reordering you described. The fix has swpal there.
  • Ordering: every order of the two exchanges, including Stop as a second producer, leaves either the notifier seeing pending or someone posting the semaphore.
  • Stress test, 400 runs each with the old notifier and with the fix: plain -O1 had 8 failed runs against 0, and ASan/UBSan had 74 against 0. The failing runs show event=0 PCM=1 buffer=0 pending=1 armed=0.

No game run was done: the change is one atomic operation with no timing change, so I judged the regression risk low. No gameplay result is claimed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants