Skip to content

Commit cd57dce

Browse files
fix(interview-coach): end-during-connect race, model gate, grade timing
Three findings from review on #391, plus one same-class gap found while checking: - endInterview cleared clientRef but never touched connectAbortRef, so ownsSession() still reported the in-flight startInterview as the owner. Since onConnected/onBotReady flip the UI to "active" before connect() resolves, End could be pressed mid-connect and the resumed call would then run setSession("active") and attach audio after the user had ended the session. endInterview now aborts and clears the controller, and the success path additionally requires clientRef.current === client. Replayed the interleaving: previously ended {session:"active", audioAttached:true}; now {session:"idle", audioAttached:false}. Same class, unflagged: component unmount disconnected the client but did not abort an in-flight connect, so it ran to completion against an already disconnected client. The unmount cleanup now aborts first. - /health only proved the Ollama daemon answered. An unpulled OLLAMA_MODEL or OLLAMA_GRADE_MODEL still reported ready, so the failure surfaced later inside the background pipeline once WebRTC was already negotiated. It now parses /api/tags and reports the missing model(s) with the pull command, tolerating the usual `:latest` omission. Covered by 7 cases including untagged config, explicit tags, a wrong explicit tag, and an empty daemon. - _wait_until_coach_quiet treated the pre-speech gap as silence. Grading is launched right after the tool ack, before the follow-up has been generated, so bot_speaking was still False and the wait returned in ~450ms — putting the grader's Ollama request in competition with the coach's own response. Added a bot_speech_turns counter so a waiter can tell "not started yet" from "already finished", and the wait now requires speech to start (bounded by start_timeout_secs, since the coach may not speak) before waiting it out. Measured with a follow-up starting at 0.8s and lasting 1.5s: previously returned at 0.36s mid-speech, now 2.78s after it finished; with no speech at all it still returns, at 4.36s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 8faf2d3 commit cd57dce

3 files changed

Lines changed: 78 additions & 9 deletions

File tree

apps/moss-interview-coach/backend/server.py

Lines changed: 59 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,9 @@ def __init__(self) -> None:
125125
self.last_question: str | None = None
126126
self.bot_buf: list[str] = []
127127
self.bot_speaking: bool = False
128+
# Incremented each time the coach starts speaking. Lets a waiter tell
129+
# "has not started yet" apart from "already finished".
130+
self.bot_speech_turns: int = 0
128131
self._grade_generation: int = 0
129132
self._grade_tasks: set[asyncio.Task[None]] = set()
130133
self._grade_lock = asyncio.Lock()
@@ -303,6 +306,7 @@ async def process_frame(self, frame: Frame, direction: FrameDirection) -> None:
303306

304307
if isinstance(frame, BotStartedSpeakingFrame):
305308
self._assist.bot_speaking = True
309+
self._assist.bot_speech_turns += 1
306310

307311
if isinstance(frame, BotStoppedSpeakingFrame):
308312
self._assist.bot_speaking = False
@@ -457,12 +461,33 @@ async def _wait_until_coach_quiet(
457461
assist: InterviewAssistState,
458462
*,
459463
timeout_secs: float,
464+
start_timeout_secs: float = 4.0,
460465
) -> None:
466+
"""Block until the coach's spoken follow-up is done.
467+
468+
Grading is launched straight after the tool ack, while the follow-up is
469+
still being generated — so `bot_speaking` is False and the gap before
470+
speech starts looks identical to silence. Waiting on silence alone
471+
therefore returned after ~450ms and put the grader's Ollama request in
472+
competition with the coach's own response, which hurts on a single local
473+
GPU. Wait for speech to actually begin first, then for it to finish.
474+
"""
461475
deadline = time.perf_counter() + timeout_secs
462-
# First wait out any active TTS / speaking window.
463-
while assist.bot_speaking and time.perf_counter() < deadline:
476+
start_deadline = min(deadline, time.perf_counter() + start_timeout_secs)
477+
turns_before = assist.bot_speech_turns
478+
479+
# Wait for the follow-up to start. Bounded, because the coach may not speak
480+
# at all for this turn; the turn counter also covers an utterance that began
481+
# and ended between polls.
482+
while (
483+
not assist.bot_speaking
484+
and assist.bot_speech_turns == turns_before
485+
and time.perf_counter() < start_deadline
486+
):
464487
await asyncio.sleep(0.12)
465-
# Small quiet period so a multi-segment utterance can finish.
488+
489+
# Then wait it out, plus a short quiet period so a multi-segment utterance
490+
# can finish before grading starts.
466491
quiet_for = 0.0
467492
while time.perf_counter() < deadline:
468493
if assist.bot_speaking:
@@ -857,17 +882,44 @@ async def lifespan(app: FastAPI):
857882
)
858883

859884

885+
def _ollama_model_available(available: set[str], wanted: str) -> bool:
886+
"""Ollama lists fully-qualified tags (`llama3.1:latest`); config omits `:latest`."""
887+
return wanted in available or (":" not in wanted and f"{wanted}:latest" in available)
888+
889+
860890
@app.get("/health")
861891
async def health() -> dict[str, Any]:
862892
ollama_ok = False
863893
ollama_error: str | None = None
894+
missing_models: list[str] = []
864895
try:
865896
base = OLLAMA_BASE_URL.removesuffix("/v1")
866897
async with httpx.AsyncClient(timeout=2.0) as client:
867898
resp = await client.get(f"{base}/api/tags")
868-
ollama_ok = resp.status_code == 200
869-
if not ollama_ok:
870-
ollama_error = f"status={resp.status_code}"
899+
if resp.status_code != 200:
900+
ollama_error = f"status={resp.status_code}"
901+
else:
902+
payload = resp.json()
903+
entries = payload.get("models") or []
904+
available = {
905+
str(entry.get("name") or entry.get("model") or "")
906+
for entry in entries
907+
if isinstance(entry, dict)
908+
}
909+
available.discard("")
910+
# A responding daemon is not enough. An unpulled model would only
911+
# fail later, inside the background pipeline, after WebRTC is
912+
# already negotiated — so refuse the interview here instead.
913+
missing_models = sorted(
914+
{OLLAMA_MODEL, OLLAMA_GRADE_MODEL}
915+
- {m for m in {OLLAMA_MODEL, OLLAMA_GRADE_MODEL} if _ollama_model_available(available, m)}
916+
)
917+
if missing_models:
918+
ollama_error = "missing Ollama model(s): " + ", ".join(
919+
f"{m} (ollama pull {m})" for m in missing_models
920+
)
921+
else:
922+
ollama_ok = True
871923
except Exception as exc: # noqa: BLE001
872924
ollama_error = str(exc)
873925

@@ -884,6 +936,7 @@ async def health() -> dict[str, Any]:
884936
"moss_index_names": all_index_names(),
885937
"ollama_ok": ollama_ok,
886938
"ollama_error": ollama_error,
939+
"ollama_missing_models": missing_models,
887940
"ollama_model": OLLAMA_MODEL,
888941
"ollama_grade_model": OLLAMA_GRADE_MODEL,
889942
"whisper_model": WHISPER_MODEL,

apps/moss-interview-coach/frontend/app/page.tsx

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,12 @@ export default function HomePage() {
282282
}, []);
283283

284284
const endInterview = useCallback(async () => {
285+
// onConnected / onBotReady can flip the UI to "active" while connect() is
286+
// still awaiting, so End can be pressed with a startInterview still in
287+
// flight. Retire its controller too, or that call would still own the
288+
// session and resurrect it when it resumes.
289+
connectAbortRef.current?.abort();
290+
connectAbortRef.current = null;
285291
const client = clientRef.current;
286292
clientRef.current = null;
287293
setSession("idle");
@@ -505,9 +511,11 @@ export default function HomePage() {
505511
abort.signal,
506512
);
507513
if (abort.signal.aborted) throw abortReason(abort.signal);
508-
// A newer interview may have taken over while we were connecting.
509-
// Retire quietly rather than presenting this one as the live session.
510-
if (!ownsSession()) {
514+
// A newer interview may have taken over, or the user may have ended the
515+
// session, while we were connecting. Both are checked: the controller
516+
// covers a newer start, and clientRef covers endInterview clearing the
517+
// live client. Retire quietly rather than presenting this as live.
518+
if (!ownsSession() || clientRef.current !== client) {
511519
if (clientRef.current === client) clientRef.current = null;
512520
try {
513521
await client.disconnect();
@@ -561,7 +569,12 @@ export default function HomePage() {
561569

562570
useEffect(() => {
563571
return () => {
572+
// Abort first: an in-flight startInterview would otherwise run to
573+
// completion against a client we are about to disconnect.
574+
connectAbortRef.current?.abort();
575+
connectAbortRef.current = null;
564576
void clientRef.current?.disconnect();
577+
clientRef.current = null;
565578
};
566579
}, []);
567580

apps/moss-interview-coach/frontend/package-lock.json

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)