Skip to content

fix(milesaudiomanager): Prevent multithread crashing in MilesAudioManager#2718

Open
xezon wants to merge 2 commits into
TheSuperHackers:mainfrom
xezon:xezon/fix-milesaudio-threading
Open

fix(milesaudiomanager): Prevent multithread crashing in MilesAudioManager#2718
xezon wants to merge 2 commits into
TheSuperHackers:mainfrom
xezon:xezon/fix-milesaudio-threading

Conversation

@xezon
Copy link
Copy Markdown

@xezon xezon commented May 16, 2026

Merge with Rebase

This change is split into 2 commits. The first cleans up some junk in Miles Audio Manager. And the second applies everything necessary for the fix.

It fixes a rare data race in MilesAudioManager, by preventing accessing potentially invalidated iterators in MilesAudioManager::findPlayingAudioFrom, which runs on the MSS Timer thread. This potentially is no issue in regular builds, because an invalidated iterator might still be usable when the memory was not reused.

Note that it NOT possible to simply defer the event processing coming in MilesAudioManager::notifyOfAudioCompletion to Main thread, because it wants to continue looping sounds immediately, and any delays will introduce audible stutter.

Note that the stop playing audio step is often seperated from the release playing audio step, because it is illegal to call into Miles while holding the new critical section due Miles holding its own internal mutex during callbacks to MilesAudioManager::notifyOfAudioCompletion and Miles API functions.

TODO

  • Test and listen
  • Add pull id to commits
  • Replicate in Generals

@xezon xezon added Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Fix Is fixing something, but is not user facing labels May 16, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 16, 2026

Greptile Summary

This PR fixes a rare data race in MilesAudioManager by introducing a two-phase stop/release pattern: the main thread first calls Miles APIs to stop audio (unlocked), then erases items from the playing lists under a new CriticalSectionClass m_playingCs lock. findPlayingAudioFrom — called on the MSS Timer thread — now holds that same lock while iterating, preventing access to invalidated iterators.

  • PS_Stopping is added as an intermediate status set by the timer thread via InterlockedCompareExchange; the main thread's processPlayingList detects it, calls stopPlayingAudio (Miles APIs, no lock), then the locked second phase erases and deletes.
  • stopPlayingAudio / releasePlayingAudio are split so that the locked release path is guaranteed never to call into Miles (items must be PS_Stopped before deletion, making stopPlayingAudio's early-return the gate).
  • nextMusicTrack / prevMusicTrack are changed to return the new track name, eliminating the separate getMusicTrackName call; the removed m_stoppedAudio list and processStoppedList are replaced by in-place deferred erasure.

Confidence Score: 5/5

The two-phase stop/release design is internally consistent and the threading model is carefully handled; safe to merge after addressing the copyright year nit.

The InterlockedCompareExchange state transitions are sound, findPlayingAudioFrom holds m_playingCs while iterating to prevent iterator invalidation, and releasePlayingAudio in the locked phase is guaranteed to skip Miles because all items are already PS_Stopped. The only finding is a stale copyright year in the new compat header.

No files require special attention beyond the copyright year nit in interlocked_adapter.h.

Important Files Changed

Filename Overview
Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp Core fix: introduces two-phase stop/release pattern with CriticalSectionClass to prevent iterator invalidation on the MSS Timer thread; stopPlayingAudio/releasePlayingAudio split is architecturally sound, locked phase strictly avoids Miles API calls.
Core/GameEngineDevice/Include/MilesAudioDevice/MilesAudioManager.h Adds PS_Stopping state, m_playingCs critical section, removes m_stoppedAudio list and processStoppedList; changes nextMusicTrack/prevMusicTrack return types to AsciiString; static_assert guards interlock size assumption.
Core/GameEngine/Include/Common/GameAudio.h Updates virtual interface: nextMusicTrack/prevMusicTrack now return AsciiString; removes getMusicTrackName (callers now use the return value instead).
Dependencies/Utility/Utility/interlocked_adapter.h New compat shim providing typed InterlockedCompareExchange overload for pre-VS2002 MSVC; correctly uses #pragma once and inline keyword.

Sequence Diagram

sequenceDiagram
    participant MT as Main Thread
    participant TT as MSS Timer Thread
    participant CS as m_playingCs (CriticalSection)
    participant Miles as Miles SDK

    Note over MT: processPlayingList - unlocked phase
    MT->>Miles: AIL_stop_sample / AIL_close_stream
    MT->>MT: InterlockedCAS PS_Playing to PS_Stopped

    TT->>CS: acquire lock
    CS-->>TT: locked
    TT->>TT: findPlayingAudioFrom iterates list
    TT->>CS: release lock
    TT->>TT: notifyOfAudioCompletion sets PS_Stopping

    Note over MT: processPlayingList - locked phase
    MT->>CS: acquire lock
    CS-->>MT: locked
    MT->>MT: erase PS_Stopped items, releasePlayingAudio, delete
    MT->>CS: release lock
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
Dependencies/Utility/Utility/interlocked_adapter.h:3
The copyright year in this newly created file reads `2025`, but the current year is 2026. Per the project's convention, new community-authored files should carry the year they were created.

```suggestion
**	Copyright 2026 TheSuperHackers
```

Reviews (4): Last reviewed commit: "fix(milesaudiomanager): Prevent multithr..." | Re-trigger Greptile

Comment thread Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp
Comment thread Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp
Comment thread Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp Outdated
@xezon xezon force-pushed the xezon/fix-milesaudio-threading branch from a02315e to b3fe6c7 Compare May 16, 2026 18:41
Comment thread Dependencies/Utility/Utility/interlocked_adapter.h Outdated
@xezon xezon force-pushed the xezon/fix-milesaudio-threading branch from b3fe6c7 to 01d9dbd Compare May 16, 2026 19:05
Comment thread Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp
@xezon xezon force-pushed the xezon/fix-milesaudio-threading branch from 01d9dbd to 025e5e7 Compare May 17, 2026 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix Is fixing something, but is not user facing Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in MilesAudioManager::findPlayingAudioFrom

1 participant