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:
@@ -702,13 +702,19 @@ pub async fn start_live_transcription_session(
|
|||||||
status_channel: Channel<LiveStatusMessage>,
|
status_channel: Channel<LiveStatusMessage>,
|
||||||
) -> Result<StartLiveTranscriptionResponse, String> {
|
) -> Result<StartLiveTranscriptionResponse, String> {
|
||||||
ensure_main_window(&window)?;
|
ensure_main_window(&window)?;
|
||||||
// Phase 1: acquire the lifecycle lock long enough to reserve the
|
// Phase 1: acquire the lifecycle lock and hold it across the full
|
||||||
// single live-session slot. Held across `ensure_model_loaded`
|
// startup sequence — model load, audio-path resolution, worker
|
||||||
// because we want start/stop concurrency to remain serialised; the
|
// spawn, AND installation of the RunningLiveSession in
|
||||||
// dangerous pattern (lock held across a JoinHandle.await) was on
|
// `live_state.running`. The lock is released by the explicit drop
|
||||||
// the stop path, not here. Released explicitly before the
|
// in Phase 2 (see comment near the bottom of this function).
|
||||||
// RunningLiveSession is installed in `live_state.running` so the
|
//
|
||||||
// symmetric stop path doesn't observe a half-initialised state.
|
// 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 lifecycle = live_state.lifecycle.lock().await;
|
||||||
{
|
{
|
||||||
let running = live_state.running.lock().unwrap();
|
let running = live_state.running.lock().unwrap();
|
||||||
|
|||||||
Reference in New Issue
Block a user