[review] matrix-rtc: ghost tasks leak on error-exit and can hang shutdown #67

New issue

Closed

opened 2026-07-07 13:52:34 +00:00 by robocub · 1 comment

robocub commented

2026-07-07 13:52:34 +00:00

Collaborator

Partial-failure and shutdown paths on the Matrix side don't cancel/await their spawned ghost work: try_join! drops sibling futures without cancelling the shutdown/per-ghost tokens (detached tasks); the ghost HTTP client has no request timeout while teardown is awaited inline (shutdown hang); and a watch::Receiver::changed() Err (dropped sender) is treated as benign, busy-looping.

F7-1 — P1 / LOCAL / CONFIRMED

Location: crates/matrix-rtc/src/lib.rs:179 @ 968da57

Defect: tokio::try_join! drops the sibling futures on the first error WITHOUT cancelling the bridge's shutdown token or the per-ghost cancel tokens, detaching every spawned ghost task: GhostHandle{cancel, join} (ghost.rs:475/547) is dropped un-cancelled (CancellationToken drop does not cancel; JoinHandle drop detaches), and the supervisor's on_bridge_complete (supervisor.rs:315) never cancels rb.root on voluntary error exit either.

Failure scenario: Mid-call, matrix::run hits a sync error whose text contains "401"/"403" (matrix.rs:1014, e.g. homeserver restart invalidating the AS-minted token) or the state read at matrix.rs:936 fails → try_join returns Err, ghost_pool.run's future is dropped with active ghosts → each run_ghost task leaks forever: presence-phase ghosts park on a token that can never fire, distribute_key_task keeps re-sending its media key every 60s to a dead call, the E2EE subscriber's post_membership_until_joined retry task (rtc/mod.rs:246, child of the never-cancelled root) keeps retrying, and the ghosts' m.call.member joins are never retracted — phantom Discord users linger in Element Call with no reaper running, and a re-link of the same room gets stale-key interference under the same ghost identities.

Verifier: Re-derived from code: (1) matrix::run really returns Err mid-call (matrix.rs:1008-1015 propagates sync errors containing 401/403/M_UNKNOWN_TOKEN; matrix.rs:936 ? on the state read). (2) tokio::try_join! at lib.rs:179 then drops ghost_pool.run's future; its local ghosts HashMap of GhostHandle{cancel, join} (ghost.rs:475/547) is dropped — CancellationToken drop does not cancel (no DropGuard) and JoinHandle drop detaches, so every run_ghost task leaks with a token that can never fire. (3) Neither lib::run's error path nor the supervisor cancels the bridge root on voluntary error exit: supervisor.rs:254-258 only logs "bridge failed", and on_bridge_complete (315-332) drops rb.root un-cancelled (only stop_bridge:309 and drain:527 cancel). (4) Consequences hold: presence-phase ghosts (ghost.rs:843-865) park forever — call_active_tx never drops because the room event handler registered at matrix.rs:885/893 captures sender clones on the Client, which the leaked tasks themselves keep alive — and their posted m.call.member is never retracted (the bridge's reaper died with rtc::run) → phantom Discord users in Element Call; each E2EE ghost's distribute_key_task keeps re-sending its media key every KEY_HEARTBEAT=60s (ghost.rs:1214, 1555-1583; members_gen pends so the sleep arm fires, recipients come from frozen local state); ghost LiveKit connections stay open (event-drain closes only on cancel, ghost.rs:936 comment); a same-process re-link mints a NEW key for the same ghost identity while the leaked task keeps sending the OLD key at (identity, index 0) → stale-key interference. The code's own comments (ghost.rs:861-863 "Sender drops only on shutdown, which also cancels"; 1565-1568 "cancel will fire imminently") document the invariant this path violates — it is a defect, not deliberate; no test pins it. Minor claim inaccuracies (active-phase ghosts mostly self-clean once unregister_channel closes their frame channels, after a brief hot-spin on the closed mute_rx; the subscriber's post_membership_until_joined leaks only if its join hadn't landed) do not break the core scenario.

Regression test sketch: Two local regression tests. (a) crates/bridge/src/supervisor.rs mod tests: build a Supervisor-shaped RunningBridge entry (or refactor on_bridge_complete's token handling to be testable), push a bridge future that resolves voluntarily with an error label while holding a clone of its bridge_root, drive on_bridge_complete(label), and assert bridge_root.is_cancelled() — pinning that a voluntary/error completion cancels the root so detached children (ghosts, key loops, membership retries) are torn down. (b) crates/matrix-rtc/src/rtc/ghost.rs mod tests (reusing MockSource/MockTransport, e.g. next to key_loop_sends_to_current_recipients_then_stops_on_cancel): spawn distribute_key_loop with a live recipient, then drop the members_gen sender WITHOUT cancelling; use tokio::time (start_paused) to advance past KEY_HEARTBEAT several times and assert the task terminates (handle.await completes) and sends stop, pinning that a closed generation channel with an un-fired cancel token must end key distribution instead of heartbeating to a dead call forever.

F7-3 — P2 / LOCAL / CONFIRMED

Location: crates/matrix-rtc/src/rtc/ghost.rs:323 @ 968da57

Defect: The ghost reqwest::Client is built with no request timeout, and the single-task GhostPool driver awaits ghost teardown INLINE (handle.join.await at ghost.rs:518/574) while run_ghost's teardown does un-raced, un-timed HTTP (membership_handle.await → post_membership PUT retry loop at ghost.rs:1876 only checks cancel BETWEEN attempts; final leave PUT at ghost.rs:937) — one stalled homeserver connection wedges the whole bridge's ghost pool.

Failure scenario: The homeserver TCP connection black-holes (no RST) while a ghost is being released → the leave PUT hangs indefinitely → run_ghost never returns → the driver blocks on handle.join.await for that Release → all subsequent GhostCommands (Assign/Release for every Discord speaker of this bridge) queue forever on the unbounded channel — the Discord→Matrix direction is dead until process restart, and process shutdown for this bridge only ends via the supervisor's 10s force-drop.

Verifier: Every structural step verified in code: (1) ghost.rs:323-326 builds the reqwest client with no .timeout()/.connect_timeout() — reqwest 0.12.28 defaults are no total/connect/read timeout; (2) the GhostPool driver is single-task and awaits ghost teardown inline (Release :572-575, same-key Assign replace :516-519, ReleaseAll/shutdown join_all :494/:584) — while parked there it is outside its select!, so even the biased shutdown arm (:486) cannot preempt it, and the unbounded cmd channel (:327) means all subsequent Assign/SetMuted/Release from the Discord CaptureRouter (:637/:663/:677) silently queue → the bridge's entire Discord→Matrix ghost lifecycle freezes; (3) run_ghost teardown awaits membership_handle (:934) whose retry loop checks cancel only BETWEEN attempts (:1887) — an in-flight PUT (:1876 → .send().await :1940) is un-raced — then does the un-timed, un-raced final leave PUT (:937; also :855/:888/:901), unlike connect_and_publish which IS raced against cancel (:882); (4) no test pins this (the test module covers cap/identity/URLs/key-loop only) and no doc or CLAUDE.md declares the missing timeout deliberate — the :567-571 comment assumes blocking is 'brief'; (5) supervisor TEARDOWN_TIMEOUT=10s force-drop confirmed (bridge/src/supervisor.rs:40). One correction to the claim's wording: reqwest ≥0.12.23 enables TCP_USER_TIMEOUT=30s + keepalive 15s/15s/3 by default on Linux (verified in v0.12.28 source), so the claim's exact kernel-black-hole variant self-recovers in ~30-127s rather than hanging forever; the indefinite 'dead until restart' wedge still occurs when the peer TCP stack stays alive but the HTTP response never comes (hung homeserver process, idle-held proxied connection) — squarely a 'stalled homeserver connection'. Either variant wedges the whole ghost pool for the stall duration with shutdown only ending via the 10s force-drop, so the defect and P2 severity stand; the inline await is itself load-bearing (:507-515 stale-leave race fix), so the fix is a bounded HTTP client/deadline, not removing the await.

Regression test sketch: P2, so not required; if desired: in ghost.rs mod tests, a #[tokio::test(start_paused)] that constructs GhostConfig with an http client pointed at a tokio TcpListener that accepts and never responds, spawns GhostPool::run, sends Assign then Release for one key followed by an Assign for a second key, and asserts the second Assign is processed (second ghost handle/sink observable) within the client's request-timeout bound — fails today because the driver is parked on the first ghost's leave PUT.

F7-2 — P2 / LOCAL / CONFIRMED

Location: crates/matrix-rtc/src/rtc/ghost.rs:863 @ 968da57

Defect: The presence-phase loop (ghost.rs:843-865) and pump_audio (ghost.rs:998-1005) treat watch::Receiver::changed() Err as benign and loop again, but changed() on a dropped sender returns Err IMMEDIATELY every call — if the sender drops while the ghost's cancel token has not fired (exactly the lib.rs:179 try_join drop path, which drops GhostHandle.mute_tx and eventually call_active_tx), the select! becomes a 100% CPU busy-spin; the comments' invariant "sender drops only on shutdown, which also cancels" only holds on the graceful path.

Failure scenario: GhostHandle is dropped without cancel (bridge future force-dropped or try_join error path) → mute_tx drops → pump_audio's res = mute_rx.changed() arm resolves Err instantly on every iteration → one worker thread pegged at 100% per leaked ghost until its frame channel also closes; same for a presence-phase ghost once all call_active_tx clones drop.

Verifier: The failure scenario survives re-derivation from the actual code. (1) Per-ghost cancel tokens are fresh CancellationToken::new() (ghost.rs:535), cancelled only explicitly by the GhostPool driver loop; run_ghost tasks are detached via tokio::spawn (:543), and neither JoinHandle drop nor CancellationToken drop cancels anything. (2) lib.rs:179 try_join! short-circuits on the first Err — and matrix::run has a real mid-run Err path (matrix.rs:1014, fatal 401/403 sync auth error) reachable while ghosts are live — dropping the ghost-pool driver future WITHOUT running its shutdown drain, so GhostHandles (mute_tx) and call_active_tx drop while per-ghost cancel never fires. (3) The process stays alive: supervisor.rs:257 logs "bridge failed" and the multi-tenant supervisor continues. (4) Leaked ghosts then busy-spin: the presence loop (:843-865) selects only on cancel (never) and call_active_rx.changed() (instant Err, ignored at :863) with no other exit; in pump_audio the biased select's always-ready mute_rx.changed() Err arm (:998) starves frame_rx.recv() so even the frame channel closing never breaks the loop — the spin is permanent until process exit, slightly worse than claimed. (5) Not deliberate: no test pins the behavior, and distribute_key_loop (:1566-1568) handles the identical hazard correctly by awaiting cancel with a comment explicitly warning against "busy-looping on a closed channel" — the two cited sites just assume an invariant ("sender drops only on shutdown") that the try_join error path violates. Severity P2 as claimed (needs a bridge-fatal error with live ghosts, but then pegs one worker thread per ghost in a long-lived multi-tenant process).

Regression test sketch: Not required (P2), but if added: in ghost.rs's #[cfg(test)] module, spawn a minimal loop replicating the two select patterns (or refactor the wait into a helper like wait_mute_change(&mut mute_rx, &cancel)); arrange: create watch channel + uncancelled token, drop the sender without cancelling; assert: the helper future completes (or parks awaiting cancel) rather than spinning — e.g. wrap in tokio::time::timeout and count loop iterations via a counter, asserting it stays ~1 instead of thousands. Fix direction mirrors distribute_key_loop:1566 — on changed() Err, await cancel.cancelled() (or break).

Filed from an automated multi-agent code review of master @ 968da57 (8 scoped finders + adversarial verification). Every finding below was re-derived from the code by an independent verifier. Fixes for the confirmed local-verifiable defects (allocator anchor panic F1-1/F1-2/F1-3/F2-1/F2-2, capture-sink panic F2-7, config env-fallback F5-4, opt-out RMW race F5-2) already landed on branch refactor/review-and-split.

Partial-failure and shutdown paths on the Matrix side don't cancel/await their spawned ghost work: `try_join!` drops sibling futures without cancelling the shutdown/per-ghost tokens (detached tasks); the ghost HTTP client has no request timeout while teardown is awaited inline (shutdown hang); and a `watch::Receiver::changed()` Err (dropped sender) is treated as benign, busy-looping. ### F7-1 — P1 / LOCAL / CONFIRMED **Location:** `crates/matrix-rtc/src/lib.rs:179` @ `968da57` **Defect:** tokio::try_join! drops the sibling futures on the first error WITHOUT cancelling the bridge's shutdown token or the per-ghost cancel tokens, detaching every spawned ghost task: GhostHandle{cancel, join} (ghost.rs:475/547) is dropped un-cancelled (CancellationToken drop does not cancel; JoinHandle drop detaches), and the supervisor's on_bridge_complete (supervisor.rs:315) never cancels rb.root on voluntary error exit either. **Failure scenario:** Mid-call, matrix::run hits a sync error whose text contains "401"/"403" (matrix.rs:1014, e.g. homeserver restart invalidating the AS-minted token) or the state read at matrix.rs:936 fails → try_join returns Err, ghost_pool.run's future is dropped with active ghosts → each run_ghost task leaks forever: presence-phase ghosts park on a token that can never fire, distribute_key_task keeps re-sending its media key every 60s to a dead call, the E2EE subscriber's post_membership_until_joined retry task (rtc/mod.rs:246, child of the never-cancelled root) keeps retrying, and the ghosts' m.call.member joins are never retracted — phantom Discord users linger in Element Call with no reaper running, and a re-link of the same room gets stale-key interference under the same ghost identities. **Verifier:** Re-derived from code: (1) matrix::run really returns Err mid-call (matrix.rs:1008-1015 propagates sync errors containing 401/403/M_UNKNOWN_TOKEN; matrix.rs:936 `?` on the state read). (2) tokio::try_join! at lib.rs:179 then drops ghost_pool.run's future; its local `ghosts` HashMap of GhostHandle{cancel, join} (ghost.rs:475/547) is dropped — CancellationToken drop does not cancel (no DropGuard) and JoinHandle drop detaches, so every run_ghost task leaks with a token that can never fire. (3) Neither lib::run's error path nor the supervisor cancels the bridge root on voluntary error exit: supervisor.rs:254-258 only logs "bridge failed", and on_bridge_complete (315-332) drops rb.root un-cancelled (only stop_bridge:309 and drain:527 cancel). (4) Consequences hold: presence-phase ghosts (ghost.rs:843-865) park forever — call_active_tx never drops because the room event handler registered at matrix.rs:885/893 captures sender clones on the Client, which the leaked tasks themselves keep alive — and their posted m.call.member is never retracted (the bridge's reaper died with rtc::run) → phantom Discord users in Element Call; each E2EE ghost's distribute_key_task keeps re-sending its media key every KEY_HEARTBEAT=60s (ghost.rs:1214, 1555-1583; members_gen pends so the sleep arm fires, recipients come from frozen local state); ghost LiveKit connections stay open (event-drain closes only on cancel, ghost.rs:936 comment); a same-process re-link mints a NEW key for the same ghost identity while the leaked task keeps sending the OLD key at (identity, index 0) → stale-key interference. The code's own comments (ghost.rs:861-863 "Sender drops only on shutdown, which also cancels"; 1565-1568 "cancel will fire imminently") document the invariant this path violates — it is a defect, not deliberate; no test pins it. Minor claim inaccuracies (active-phase ghosts mostly self-clean once unregister_channel closes their frame channels, after a brief hot-spin on the closed mute_rx; the subscriber's post_membership_until_joined leaks only if its join hadn't landed) do not break the core scenario. **Regression test sketch:** Two local regression tests. (a) crates/bridge/src/supervisor.rs `mod tests`: build a Supervisor-shaped RunningBridge entry (or refactor on_bridge_complete's token handling to be testable), push a bridge future that resolves voluntarily with an error label while holding a clone of its bridge_root, drive `on_bridge_complete(label)`, and assert `bridge_root.is_cancelled()` — pinning that a voluntary/error completion cancels the root so detached children (ghosts, key loops, membership retries) are torn down. (b) crates/matrix-rtc/src/rtc/ghost.rs `mod tests` (reusing MockSource/MockTransport, e.g. next to `key_loop_sends_to_current_recipients_then_stops_on_cancel`): spawn distribute_key_loop with a live recipient, then drop the members_gen sender WITHOUT cancelling; use tokio::time (start_paused) to advance past KEY_HEARTBEAT several times and assert the task terminates (handle.await completes) and sends stop, pinning that a closed generation channel with an un-fired cancel token must end key distribution instead of heartbeating to a dead call forever. ### F7-3 — P2 / LOCAL / CONFIRMED **Location:** `crates/matrix-rtc/src/rtc/ghost.rs:323` @ `968da57` **Defect:** The ghost reqwest::Client is built with no request timeout, and the single-task GhostPool driver awaits ghost teardown INLINE (`handle.join.await` at ghost.rs:518/574) while run_ghost's teardown does un-raced, un-timed HTTP (`membership_handle.await` → post_membership PUT retry loop at ghost.rs:1876 only checks cancel BETWEEN attempts; final leave PUT at ghost.rs:937) — one stalled homeserver connection wedges the whole bridge's ghost pool. **Failure scenario:** The homeserver TCP connection black-holes (no RST) while a ghost is being released → the leave PUT hangs indefinitely → run_ghost never returns → the driver blocks on `handle.join.await` for that Release → all subsequent GhostCommands (Assign/Release for every Discord speaker of this bridge) queue forever on the unbounded channel — the Discord→Matrix direction is dead until process restart, and process shutdown for this bridge only ends via the supervisor's 10s force-drop. **Verifier:** Every structural step verified in code: (1) ghost.rs:323-326 builds the reqwest client with no .timeout()/.connect_timeout() — reqwest 0.12.28 defaults are no total/connect/read timeout; (2) the GhostPool driver is single-task and awaits ghost teardown inline (Release :572-575, same-key Assign replace :516-519, ReleaseAll/shutdown join_all :494/:584) — while parked there it is outside its select!, so even the biased shutdown arm (:486) cannot preempt it, and the unbounded cmd channel (:327) means all subsequent Assign/SetMuted/Release from the Discord CaptureRouter (:637/:663/:677) silently queue → the bridge's entire Discord→Matrix ghost lifecycle freezes; (3) run_ghost teardown awaits membership_handle (:934) whose retry loop checks cancel only BETWEEN attempts (:1887) — an in-flight PUT (:1876 → .send().await :1940) is un-raced — then does the un-timed, un-raced final leave PUT (:937; also :855/:888/:901), unlike connect_and_publish which IS raced against cancel (:882); (4) no test pins this (the test module covers cap/identity/URLs/key-loop only) and no doc or CLAUDE.md declares the missing timeout deliberate — the :567-571 comment assumes blocking is 'brief'; (5) supervisor TEARDOWN_TIMEOUT=10s force-drop confirmed (bridge/src/supervisor.rs:40). One correction to the claim's wording: reqwest ≥0.12.23 enables TCP_USER_TIMEOUT=30s + keepalive 15s/15s/3 by default on Linux (verified in v0.12.28 source), so the claim's exact kernel-black-hole variant self-recovers in ~30-127s rather than hanging forever; the indefinite 'dead until restart' wedge still occurs when the peer TCP stack stays alive but the HTTP response never comes (hung homeserver process, idle-held proxied connection) — squarely a 'stalled homeserver connection'. Either variant wedges the whole ghost pool for the stall duration with shutdown only ending via the 10s force-drop, so the defect and P2 severity stand; the inline await is itself load-bearing (:507-515 stale-leave race fix), so the fix is a bounded HTTP client/deadline, not removing the await. **Regression test sketch:** P2, so not required; if desired: in ghost.rs mod tests, a #[tokio::test(start_paused)] that constructs GhostConfig with an http client pointed at a tokio TcpListener that accepts and never responds, spawns GhostPool::run, sends Assign then Release for one key followed by an Assign for a second key, and asserts the second Assign is processed (second ghost handle/sink observable) within the client's request-timeout bound — fails today because the driver is parked on the first ghost's leave PUT. ### F7-2 — P2 / LOCAL / CONFIRMED **Location:** `crates/matrix-rtc/src/rtc/ghost.rs:863` @ `968da57` **Defect:** The presence-phase loop (ghost.rs:843-865) and pump_audio (ghost.rs:998-1005) treat watch::Receiver::changed() Err as benign and loop again, but changed() on a dropped sender returns Err IMMEDIATELY every call — if the sender drops while the ghost's cancel token has not fired (exactly the lib.rs:179 try_join drop path, which drops GhostHandle.mute_tx and eventually call_active_tx), the select! becomes a 100% CPU busy-spin; the comments' invariant "sender drops only on shutdown, which also cancels" only holds on the graceful path. **Failure scenario:** GhostHandle is dropped without cancel (bridge future force-dropped or try_join error path) → mute_tx drops → pump_audio's `res = mute_rx.changed()` arm resolves Err instantly on every iteration → one worker thread pegged at 100% per leaked ghost until its frame channel also closes; same for a presence-phase ghost once all call_active_tx clones drop. **Verifier:** The failure scenario survives re-derivation from the actual code. (1) Per-ghost cancel tokens are fresh CancellationToken::new() (ghost.rs:535), cancelled only explicitly by the GhostPool driver loop; run_ghost tasks are detached via tokio::spawn (:543), and neither JoinHandle drop nor CancellationToken drop cancels anything. (2) lib.rs:179 try_join! short-circuits on the first Err — and matrix::run has a real mid-run Err path (matrix.rs:1014, fatal 401/403 sync auth error) reachable while ghosts are live — dropping the ghost-pool driver future WITHOUT running its shutdown drain, so GhostHandles (mute_tx) and call_active_tx drop while per-ghost cancel never fires. (3) The process stays alive: supervisor.rs:257 logs "bridge failed" and the multi-tenant supervisor continues. (4) Leaked ghosts then busy-spin: the presence loop (:843-865) selects only on cancel (never) and call_active_rx.changed() (instant Err, ignored at :863) with no other exit; in pump_audio the biased select's always-ready mute_rx.changed() Err arm (:998) starves frame_rx.recv() so even the frame channel closing never breaks the loop — the spin is permanent until process exit, slightly worse than claimed. (5) Not deliberate: no test pins the behavior, and distribute_key_loop (:1566-1568) handles the identical hazard correctly by awaiting cancel with a comment explicitly warning against "busy-looping on a closed channel" — the two cited sites just assume an invariant ("sender drops only on shutdown") that the try_join error path violates. Severity P2 as claimed (needs a bridge-fatal error with live ghosts, but then pegs one worker thread per ghost in a long-lived multi-tenant process). **Regression test sketch:** Not required (P2), but if added: in ghost.rs's #[cfg(test)] module, spawn a minimal loop replicating the two select patterns (or refactor the wait into a helper like wait_mute_change(&mut mute_rx, &cancel)); arrange: create watch channel + uncancelled token, drop the sender without cancelling; assert: the helper future completes (or parks awaiting cancel) rather than spinning — e.g. wrap in tokio::time::timeout and count loop iterations via a counter, asserting it stays ~1 instead of thousands. Fix direction mirrors distribute_key_loop:1566 — on changed() Err, await cancel.cancelled() (or break). --- *Filed from an automated multi-agent code review of `master` @ `968da57` (8 scoped finders + adversarial verification). Every finding below was re-derived from the code by an independent verifier. Fixes for the confirmed local-verifiable defects (allocator anchor panic F1-1/F1-2/F1-3/F2-1/F2-2, capture-sink panic F2-7, config env-fallback F5-4, opt-out RMW race F5-2) already landed on branch `refactor/review-and-split`.*

robocub referenced this issue

2026-07-10 15:35:15 +00:00

[review] matrix-rtc: LiveKit RoomEvent::Disconnected is swallowed with no recovery (subscriber reaper can wipe real participants) #66