diff --git a/client-sdk-rust b/client-sdk-rust index dad794d4..fd48e5c2 160000 --- a/client-sdk-rust +++ b/client-sdk-rust @@ -1 +1 @@ -Subproject commit dad794d414fda9e8c1de83af1c0f190506a15f8f +Subproject commit fd48e5c280579e61dc37d4ca26bf06315a670baa diff --git a/docs/platform-audio-instability-investigation.md b/docs/platform-audio-instability-investigation.md new file mode 100644 index 00000000..5263a4e8 --- /dev/null +++ b/docs/platform-audio-instability-investigation.md @@ -0,0 +1,173 @@ +# macOS PlatformAudio Instability — Investigation & Fix Plan + +*Author: review pass on 2026-07-01. Supersedes the ad-hoc notes in the prior Cursor sessions.* + +## 1. Summary + +The macOS `PlatformAudioIntegrationTest` suite has been crashing (SIGSEGV / SIGABRT) +under repeated run/teardown. Prior investigation (three Cursor sessions on +`feature/additional_ci_improvements` and `feature/platform-audio-stability`, plus a +`rust-sdks` branch of the same name) converged on a real root cause and landed a fix +bundle. This document records an independent review of that work, what I agree and +disagree with, the CI evidence, and a corrected fix plan. + +**Bottom line:** + +- The final root-cause theory is **correct**: the macOS CoreAudio capture worker + (`webrtc::AudioDeviceMac::CaptureWorkerThread`) delivers recorded audio into + `webrtc::AudioTransportImpl` while the peer-connection transports / factory that own + that transport are being destroyed. The landed `AdmProxy::ShutdownAudioIO` fix is the + right shape and empirically removed the segfault. +- But the investigation stopped short in three ways this review corrects: + 1. **A 2026-07-01 rebase silently reset the `client-sdk-cpp` submodule pointer to a + `rust-sdks` commit that does not contain the fix.** + 2. **The post-fix nightlies expose a *new, deterministic* failure** in + `PlatformAudioFramesReachRemote` that is a signaling/negotiation regression, not an + audio-teardown crash. + 3. **A structural teardown race survives the fix** and is the most likely cause of both + the residual "crash at the very first PlatformAudio test" symptom and the + `DataTrackE2ETest.PublishManyTracks` teardown hang. + +## 2. Confirmed root cause (crash) + +Evidence (from CI `.ips` reports, LLDB captures, and the pre-fix Intel triage run): + +``` +webrtc::AudioDeviceMac::CaptureWorkerThread() audio_device_mac.cc:2498 + -> AudioConverterFillComplexBuffer + -> CrashIfClientProvidedBogusAudioBufferList (Apple AudioToolbox) +``` +concurrent with, on the teardown side: +``` +webrtc::AudioTransportImpl::SendProcessedData(...) + -> ...RecordedDataIsAvailable -> DeliverRecordedData + ~JsepTransportController() / ~BundleManager() (transport teardown) +``` + +The capture worker owned by the platform ADM keeps calling into an `AudioTransportImpl` +that a closing peer connection is tearing down → use-after-free / bogus buffer → SIGSEGV +or SIGABRT ("pointer being freed was not allocated"). + +## 3. The landed fix (rust-sdks `feature/platform-audio-stability`, core commit `01fc4c13`, PR-feedback `d3bb1453`) + +- `webrtc-sys` `AdmProxy`: new `ShutdownAudioIO()` + `StopAudioIO()` / `StopPlatformAudioIO()` + helpers that stop recording/playout and detach the `AudioTransport` callback. Called from + `~AdmProxy()`, `Terminate()`, and at platform-ADM refcount 0. +- `PeerConnectionFactory::shutdown_audio_io()` runs `AdmProxy::ShutdownAudioIO()` on the + worker thread **before** `peer_factory_` is destroyed (`~PeerConnectionFactory`). +- `LkRuntime::Drop` calls `shutdown_audio_io()`. +- `PlatformAdmHandle::Drop` stops recording before releasing the ADM reference. +- `FfiRoom::close()` now closes the room (unpublishing tracks, stopping capture) **before** + dropping FFI track handles. +- `FfiServer::dispose()` clears `ffi_handles` / `handle_dropped_txs`. +- (client-sdk-cpp) `PlatformAudioSource` member order swapped so the FFI handle drops before + the shared `PlatformAudioState`. + +**Verdict: keep all of it.** It is correct and directly targets the confirmed race. Two +minor corrections (see §6, Step 2). + +Also landed and **correct** — keep: the `LocalParticipant::published_tracks_by_sid_` mutex +(`5380e42`), an ASan-proven data race between `publishTrack` (app thread) and +`trackPublications()`/`findTrackPublication()` (FFI callback thread). It is a genuine +general-purpose SIGSEGV source, though independent of the ADM crash. + +## 4. Theories that were correctly abandoned + +- **Dangling FFI handles at dispose** — instrumentation showed 0 leaked handles across + 738+ dispose samples. The `ffi_handles.clear()` hygiene was kept anyway (harmless). +- **mach-port / fd / thread leaks** (CoreAudio HAL client leak) — sampler showed all flat. +- **Progressive memory growth** — real (~1 retained `PeerConnection` per connect cycle; + `leaks` clean → reachable retention), but *orthogonal to the crash* (crash hits the first + PlatformAudio test, so it is not cumulative). Root cause identified in this review: see §5. + +## 5. What the prior work missed + +### 5.1 The rebase dropped the fix from the C++ branch + +`feature/platform-audio-stability` (client-sdk-cpp) now pins submodule +`client-sdk-rust` at `dad794d4` — plain `rust-sdks` main, **without** the ADM fix. +`feature/additional_ci_improvements` still pins `d3bb1453` (has the fix). The stability +branch as it stands does not build the fix it claims to test. + +### 5.2 A new, deterministic `FramesReachRemote` regression (not a crash) + +Post-fix nightlies (`additional_ci_improvements`, rust `d3bb1453`), 2026-06-28 and 06-30: + +- **The segfault is gone** — 100+ PlatformAudio iterations, zero SIGSEGV. +- **`PlatformAudioFramesReachRemote` fails 0/26** (all attempts, including the very first), + debug builds only. The log shows, at publish time: + ``` + WARN livekit::rtc_engine::peer_transport] peer connection is closed, cannot create offer + ERROR livekit::rtc_engine::rtc_session] failed to negotiate the publisher: + Rtc(RtcError { InvalidState, "Failed to set local offer sdp: Called in wrong state" }) + ``` + This is a **signaling / negotiation** failure, not audio teardown. It rode in on the + upstream `rust-sdks` delta between the old pin (`8e551062`) and the new base + (`2e83ff6b`). Prime suspect: **#1148 "harden reconnect behaviour"** (also #996 publisher + offer with join, single-PC default). Release PR CI passes; only debug nightly reproduces. + +### 5.3 A structural teardown race survives the fix + +- The FFI tokio runtime is a process-static and is **never shut down**; `dispose()` clears + handles but does **not** wait for `LkRuntime` (and thus the `PeerConnectionFactory` + ADM + + its capture worker) to finish dropping. +- `LK_RUNTIME` is a `Mutex>`: the moment the last strong ref hits zero, a new + `instance()` call constructs a **new** factory/ADM — potentially **while the old one's + `Drop` (including the capture-worker stop) is still running on another thread.** +- `EngineInner::close` deliberately retains `RtcSession` ("so we can still access stats"), + which keeps a whole `PeerConnection` (and its transports) alive per connect cycle — this + is the source of the "reachable growth" from §4, and it defers transport destruction to + arbitrary threads at arbitrary times. + +Together these mean runtime/ADM/transport teardown from test *N* can overlap init from test +*N+1*. This matches the "crash at the start of the first PlatformAudio test" signature and is +the most plausible family for the still-open `PublishManyTracks` teardown hang (120-min +silent timeout on macos-x64; watchdog SIGABRT on linux-x64, where `gdb` is not installed so +no backtrace is produced). + +## 6. Corrected fix plan + +**Step 0 — hygiene (blocking):** +- Re-point the `client-sdk-cpp` `feature/platform-audio-stability` submodule at the fixed + `rust-sdks` tip (currently `d3bb1453`, or the new tip from Step 2/3). +- Install `gdb` on the Linux CI runner so the stall watchdog's cores produce backtraces. + +**Step 1 — attribute the `FramesReachRemote` regression before trusting the fix branch.** +Run the triage workflow (both arches, isolated-frames arm) across three rust pins: +`8e551062` (old baseline), `2e83ff6b` (new upstream base, *no* fix commits), `d3bb1453` +(fix). Signature to look for: `peer connection is closed, cannot create offer` ~6 s into the +test, **debug builds only**. If `2e83ff6b` alone reproduces it, the ADM fix is exonerated and +the regression is an upstream bug to bisect (start at #1148). + +**Step 2 — correct the ADM fix (rust-sdks), low risk:** +- In `StopAudioIO()` / `StopPlatformAudioIO()`: call `StopRecording()`/`StopPlayout()` + **first** (this joins the worker threads), *then* `RegisterAudioCallback(nullptr)`. + WebRTC's `AudioDeviceBuffer` refuses a callback change while media is active, so the + detach-before-stop ordering is a silent no-op today. Stopping first makes the detach real. +- (Investigated and **rejected** as changes: `PlatformAdmHandle::Drop` calling a global + `stop_recording()` is safe — the handle is shared via a static `Weak`, so `Drop` only runs + when the *last* `PlatformAudio` is gone. `AdmProxy::StopRecording()` stopping both ADMs is + benign — the synthetic ADM has no microphone.) + +**Step 3 — close the surviving teardown race (rust-sdks), the genuinely new fix:** +- Add a runtime teardown-completion signal to `LkRuntime`: a static counter + condvar, + incremented when a runtime is created in `instance()` and decremented at the **end** of + `Drop`. `instance()` waits (bounded, ~10 s, with a loud log on timeout) for the previous + runtime to finish dropping before constructing a new factory/ADM — closing the + `Mutex` create-during-teardown window. Optionally, `FfiServer::dispose()` waits on + the same signal after clearing handles for a clean process-exit ordering. +- **(Deferred, higher risk — recommend as follow-up, not landed automatically)** Drop the + retained `RtcSession` in `EngineInner::close` (or snapshot stats at close instead of + retaining the session). This removes the per-cycle `PeerConnection` retention and makes + transport destruction deterministic. It touches reconnection/stats semantics that external + consumers may depend on, so it should be a separately reviewed change with its own CI. + +## 7. Open items / risks + +- `FramesReachRemote` regression root cause (Step 1) — **must** be settled before the branch + can be considered green; it is currently a hard, deterministic failure in debug CI. +- `PublishManyTracks` teardown hang — persists in both post-fix nightlies; needs a Linux + backtrace (blocked on `gdb` install, Step 0). +- The teardown-completion signal adds a bounded wait in `instance()`/`dispose()`; verified by + code inspection to be free of lock-ordering deadlock (the dropping thread never takes the + `LK_RUNTIME` lock), but should be exercised under the triage matrix. diff --git a/include/livekit/local_participant.h b/include/livekit/local_participant.h index 9369a914..2c1d5cf5 100644 --- a/include/livekit/local_participant.h +++ b/include/livekit/local_participant.h @@ -246,6 +246,14 @@ class LIVEKIT_API LocalParticipant : public Participant { /// prune expired @c weak_ptr entries. mutable TrackMap published_tracks_by_sid_; + /// Guards @ref published_tracks_by_sid_. The map is written from the + /// application thread (@ref publishTrack / @ref unpublishTrack) and both read + /// and pruned from the FFI callback thread (@ref trackPublications / + /// @ref findTrackPublication, reached via Room::onEvent). Without this lock + /// those concurrent accesses race and free map nodes out from under each + /// other (heap-use-after-free). Leaf lock: no other lock is taken while held. + mutable std::mutex published_tracks_mutex_; + std::unordered_map rpc_handlers_; // Shared state for RPC invocation tracking. Using shared_ptr so the state diff --git a/include/livekit/platform_audio.h b/include/livekit/platform_audio.h index 4e12343e..66755d96 100644 --- a/include/livekit/platform_audio.h +++ b/include/livekit/platform_audio.h @@ -114,8 +114,8 @@ class LIVEKIT_API PlatformAudioSource { PlatformAudioSource(FfiHandle handle, std::shared_ptr platform_audio) noexcept; - FfiHandle handle_; std::shared_ptr platform_audio_; + FfiHandle handle_; }; /// Platform audio device manager backed by WebRTC's Audio Device Module. diff --git a/src/data_track_stream.cpp b/src/data_track_stream.cpp index 155f4788..6a777377 100644 --- a/src/data_track_stream.cpp +++ b/src/data_track_stream.cpp @@ -52,22 +52,28 @@ void DataTrackStream::init(FfiHandle subscription_handle) { bool DataTrackStream::read(DataTrackFrame& out) { proto::DataTrackStreamReadResponse read_response; bool missing_read_response = false; + std::uint64_t subscription_handle = 0; { const std::scoped_lock lock(mutex_); if (closed_ || eof_) { return false; } + subscription_handle = static_cast(subscription_handle_.get()); + } - const auto subscription_handle = static_cast(subscription_handle_.get()); + // Do not hold mutex_ across sendRequest: readFrameWithTimeout may call close() + // from another thread on timeout, and close() also needs mutex_. + proto::FfiRequest req; + auto* msg = req.mutable_data_track_stream_read(); + msg->set_stream_handle(subscription_handle); + const proto::FfiResponse resp = FfiClient::instance().sendRequest(req); - // Signal the Rust side that we're ready to receive the next frame. - // The Rust SubscriptionTask uses a demand-driven protocol: it won't pull - // from the underlying stream until notified via this request. - proto::FfiRequest req; - auto* msg = req.mutable_data_track_stream_read(); - msg->set_stream_handle(subscription_handle); - const proto::FfiResponse resp = FfiClient::instance().sendRequest(req); + { + const std::scoped_lock lock(mutex_); + if (closed_ || eof_) { + return false; + } if (!resp.has_data_track_stream_read()) { missing_read_response = true; } else { diff --git a/src/local_participant.cpp b/src/local_participant.cpp index 7fda68ac..1e8d9a0f 100644 --- a/src/local_participant.cpp +++ b/src/local_participant.cpp @@ -197,7 +197,10 @@ void LocalParticipant::publishTrack(const std::shared_ptr& track, const T auto publication = std::make_shared(owned_pub); const std::string sid = publication->sid(); - published_tracks_by_sid_[sid] = std::weak_ptr(track); + { + const std::scoped_lock lock(published_tracks_mutex_); + published_tracks_by_sid_[sid] = std::weak_ptr(track); + } track->setPublication(publication); } @@ -237,6 +240,7 @@ void LocalParticipant::unpublishTrack(const std::string& track_sid) { fut.get(); + const std::scoped_lock lock(published_tracks_mutex_); if (auto it = published_tracks_by_sid_.find(track_sid); it != published_tracks_by_sid_.end()) { if (auto t = it->second.lock()) { t->setPublication(nullptr); @@ -247,6 +251,7 @@ void LocalParticipant::unpublishTrack(const std::string& track_sid) { LocalParticipant::PublicationMap LocalParticipant::trackPublications() const { PublicationMap out; + const std::scoped_lock lock(published_tracks_mutex_); for (auto it = published_tracks_by_sid_.begin(); it != published_tracks_by_sid_.end();) { auto t = it->second.lock(); if (!t) { @@ -443,6 +448,7 @@ void LocalParticipant::handleRpcMethodInvocation(uint64_t invocation_id, const s } std::shared_ptr LocalParticipant::findTrackPublication(const std::string& sid) const { + const std::scoped_lock lock(published_tracks_mutex_); auto it = published_tracks_by_sid_.find(sid); if (it == published_tracks_by_sid_.end()) { return nullptr; diff --git a/src/room.cpp b/src/room.cpp index 32dac5eb..61bf373e 100644 --- a/src/room.cpp +++ b/src/room.cpp @@ -434,7 +434,7 @@ void Room::onEvent(const FfiEvent& event) { if (event.message_case() == FfiEvent::kRpcMethodInvocation) { const auto& rpc = event.rpc_method_invocation(); - LocalParticipant* lp = nullptr; + std::shared_ptr lp; { const std::scoped_lock guard(lock_); if (!local_participant_) { @@ -446,7 +446,7 @@ void Room::onEvent(const FfiEvent& event) { // RPC is not targeted at this room's local participant; ignore. return; } - lp = local_participant_.get(); + lp = local_participant_; } // Call outside the lock to avoid deadlocks / re-entrancy issues.