From 6c212a0d2cc79b62397cf1b8d98e0e69eb41c8be Mon Sep 17 00:00:00 2001 From: Jake Date: Thu, 14 May 2026 17:47:09 +0100 Subject: [PATCH] =?UTF-8?q?agent:=20lumotia=20=E2=80=94=20Phase=20B.1=20fi?= =?UTF-8?q?x=20misleading=20comment=20on=20start=5Flive=20lifecycle=20orde?= =?UTF-8?q?ring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase B.1 survey finding (commit 5725836 cancellable Whisper inference + bounded drain + lock-over-await). The upper comment on `start_live_transcription_session` claimed: Released explicitly before the RunningLiveSession is installed in `live_state.running` so the symmetric stop path doesn't observe a half-initialised state. The actual code (lines 801-811) does the opposite: it installs the RunningLiveSession FIRST, then drops the lifecycle guard. That ordering is the SAFER one — a concurrent stop_live acquiring lifecycle observes a fully-installed `running` slot or none, never a half-initialised state. The bug was in the comment, not the code. Future-reader trap: an atomiser-grade review of the locking discipline would have trusted the comment over the code and "fixed" the code to match — reintroducing the half-initialised window between drop and install. Rewrote the Phase 1 comment to describe the actual behaviour (hold-through-install + Phase 2 drop) and explain why holding is intentional. Phase 2 comment already accurate; left untouched. Other B.1 findings: * 8 existing unit tests cover the atomiser-targetable surface (Race-A drop sets abort flag, drain_timeout helpers defend NaN/inf/negative/ zero/no-inflight cases, channel-loss observability, tracing target convention). * Race-B drain-timeout end-to-end test remains a known gap. The SAFETY comment in drain_inference acknowledges it ("requires a wedged whisper-rs, which is hard to fixture"). Closing it would require refactoring LiveSessionRuntime for testability — invasive, no identified residual bug in the audited 60-line drain_inference body. * stop_live comments + code consistent. No fix needed. Verification: - cargo test -p lumotia --lib commands::live: 17/17 (no change to test count — comment-only edit) - cargo clippy -p lumotia --all-targets -- -D warnings: clean - cargo fmt --check: clean --- src-tauri/src/commands/live.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src-tauri/src/commands/live.rs b/src-tauri/src/commands/live.rs index 19bcfaa..df33c7a 100644 --- a/src-tauri/src/commands/live.rs +++ b/src-tauri/src/commands/live.rs @@ -702,13 +702,19 @@ pub async fn start_live_transcription_session( status_channel: Channel, ) -> Result { ensure_main_window(&window)?; - // Phase 1: acquire the lifecycle lock long enough to reserve the - // single live-session slot. Held across `ensure_model_loaded` - // because we want start/stop concurrency to remain serialised; the - // dangerous pattern (lock held across a JoinHandle.await) was on - // the stop path, not here. Released explicitly before the - // RunningLiveSession is installed in `live_state.running` so the - // symmetric stop path doesn't observe a half-initialised state. + // Phase 1: acquire the lifecycle lock and hold it across the full + // startup sequence — model load, audio-path resolution, worker + // spawn, AND installation of the RunningLiveSession in + // `live_state.running`. The lock is released by the explicit drop + // in Phase 2 (see comment near the bottom of this function). + // + // Holding through the install is intentional: a concurrent + // `stop_live_transcription_session` acquiring lifecycle MUST + // observe either a fully-installed `running` slot or none at all, + // never a half-initialised state. The dangerous pattern of holding + // the lock across a `JoinHandle.await` is on the stop path, not + // here — start hands the handle off to RunningLiveSession and + // never awaits it from inside this function. let lifecycle = live_state.lifecycle.lock().await; { let running = live_state.running.lock().unwrap();