agent: lumotia — Phase B.1 fix misleading comment on start_live lifecycle ordering

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
This commit is contained in:
2026-05-14 17:47:09 +01:00
parent ff8dda06d0
commit 6c212a0d2c

View File

@@ -702,13 +702,19 @@ pub async fn start_live_transcription_session(
status_channel: Channel<LiveStatusMessage>,
) -> Result<StartLiveTranscriptionResponse, String> {
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();