fix(milesaudiomanager): Prevent multithread crashing in MilesAudioManager

[xezon]

Merge with Rebase

• Fixes Race condition in MilesAudioManager::findPlayingAudioFrom #2715

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

[greptile-apps]

Greptile Summary

This PR introduces a threading fix for MilesAudioManager that prevents data races between the main thread and the MSS Timer thread. The core mechanism replaces immediate list erasure (which could invalidate iterators observed by the MSS timer) with a two-phase deferred-release design: an unlocked phase calls into Miles API and marks items PS_Stopped, then a locked phase (under per-list CriticalSectionClass) safely erases completed items from the lists.

• Threading model overhaul: Adds four CriticalSectionClass instances guarding m_playingStreams, m_playingSounds, m_playing3DSounds, and m_fadingAudio; all erasures from these lists are now performed under the corresponding lock, while Miles API calls are deliberately kept outside the lock to avoid deadlock with Miles' own internal mutex during notifyOfAudioCompletion callbacks.
• State machine extension: Adds PS_Stopping as an intermediate state (set atomically by the MSS timer via InterlockedCompareExchange); PS_Stopped now signals that handles have been released and the item is ready for erasure under lock.
• API cleanup: nextMusicTrack/prevMusicTrack now return AsciiString (eliminating the separate getMusicTrackName), a new stopPlayingAudio/fadePlayingAudio split separates stop-and-mark from fade-and-defer, and a new findActiveMusic helper centralises active-music lookup.

Confidence Score: 4/5

The threading fix is architecturally sound and the two-phase design is correctly implemented, but the deferred-erasure model introduces subtle edge cases that deserve thorough in-game listen testing before merging.

The core concurrency design is correctly implemented: CriticalSections protect list iteration, Miles API calls stay outside the lock to avoid deadlock with Miles' internal mutex, and the InterlockedCompareExchange two-step transition is safe because only the main thread drives items to PS_Stopped while the MSS timer only sets PS_Stopping. The outstanding TODO item (Test and listen) flags that audio behaviour changes have not yet been validated against audible regressions.

MilesAudioManager.cpp — specifically the deferred-erasure path for 3D sounds in processPlayingList, where PS_Stopped items are not caught by the PS_Stopping guard in the unlocked first phase and will be iterated again on the next frame before the locked phase erases them.

Important Files Changed

Filename	Overview
Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp	Core threading fix: deferred two-phase (stop/release) design with per-list critical sections; removes stoppedAudio list, splits releasePlayingAudio into stopPlayingAudio+releasePlayingAudio, and applies InterlockedCompareExchange for status transitions.
Core/GameEngineDevice/Include/MilesAudioDevice/MilesAudioManager.h	Adds PS_Stopping state, four CriticalSectionClass members, new stopPlayingAudio/fadePlayingAudio/findActiveMusic helpers, removes processStoppedList and getMusicTrackName; PlayingAudio gains m_fade and Short m_framesFaded fields with a static_assert on m_status size.
Dependencies/Utility/Utility/interlocked_adapter.h	New header providing an inline InterlockedCompareExchange overload for legacy MSVC less than 1300 that casts through PVOID; uses pragma once and is correctly marked inline.
Core/GameEngine/Include/Common/GameAudio.h	nextMusicTrack/prevMusicTrack signatures changed from void to AsciiString return; getMusicTrackName pure virtual removed.
Generals/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp	Updated MSG_META_DEMO_MUSIC_NEXT/PREV_TRACK handlers to use the AsciiString return value from nextMusicTrack/prevMusicTrack instead of a separate getMusicTrackName call.
GeneralsMD/Code/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp	Same CommandXlat update as the Generals variant: uses AsciiString return from nextMusicTrack/prevMusicTrack.

Sequence Diagram

sequenceDiagram
    participant MT as Main Thread
    participant MSS as MSS Timer Thread
    participant L as CriticalSectionCS
    Note over MT: processPlayingList Phase 1 unlocked
    MT->>MT: for each item call stopPlayingAudio if PS_Stopping
    Note over MSS: EOS callback fires
    MSS->>L: acquire m_playingSoundsCS
    L-->>MSS: lock acquired
    MSS->>MSS: findPlayingAudioFrom compare handle
    MSS->>L: release lock
    MSS->>MSS: notifyOfAudioCompletion CAS PS_Playing to PS_Stopping
    Note over MT: processPlayingList Phase 2 locked
    MT->>L: acquire m_playingSoundsCS
    L-->>MT: lock acquired
    MT->>MT: erase PS_Stopped items releasePlayingAudio
    MT->>L: release lock

Loading

_{Reviews (5): Last reviewed commit: "fix(milesaudiomanager): Prevent multithr..." | Re-trigger Greptile}

[Caball009]

Double atomic operation looks suspicious.

[xezon]

It was written this way to make sure these states advance in order atomically.

If we did release->m_status = PS_Stopped here and the other thread tried to set it to PS_Stopping, then PS_Stopped may not win the race.

[Caball009]

Could you rebase this to get the CI replay check working again?

[xezon]

Could you rebase this to get the CI replay check working again?

Done