diff --git a/apps/backend/analysis_runtime.py b/apps/backend/analysis_runtime.py index 909b9138..3e3547aa 100644 --- a/apps/backend/analysis_runtime.py +++ b/apps/backend/analysis_runtime.py @@ -807,15 +807,20 @@ def complete_measurement( # defense-in-depth so an operator who exports the env var globally # doesn't leak a stale `transcription` key into the measurement row. measurement_result.pop("transcription", None) - self._update_measurement_row( + updated = self._update_measurement_row( run_id, status="completed", result=measurement_result, provenance=provenance, diagnostics=diagnostics, error=None, + guard_terminal=True, ) - self._enqueue_requested_followups(run_id) + # Only enqueue the downstream pipeline if this completion actually landed. + # If the run was interrupted in the TOCTOU window the update no-ops, and + # enqueuing here would resurrect a fresh pipeline for an interrupted run. + if updated: + self._enqueue_requested_followups(run_id) def fail_measurement( self, @@ -832,6 +837,7 @@ def fail_measurement( provenance=provenance, diagnostics=diagnostics, error=error, + guard_terminal=True, ) def create_pitch_note_attempt( @@ -949,11 +955,11 @@ def complete_pitch_note_attempt( ).fetchone() if attempt_row is None: raise KeyError(f"Unknown pitch/note translation attempt {attempt_id}") - conn.execute( + cursor = conn.execute( """ UPDATE pitch_note_translation_attempts SET status = ?, result_json = ?, provenance_json = ?, diagnostics_json = ?, error_json = ?, updated_at = ? - WHERE id = ? + WHERE id = ? AND status NOT IN ('completed', 'failed', 'interrupted') """, ( "completed", @@ -965,6 +971,12 @@ def complete_pitch_note_attempt( attempt_id, ), ) + if cursor.rowcount == 0: + # Already terminal — e.g. interrupt_run flipped this attempt to + # 'interrupted' while its (now-orphaned) subprocess was still + # finishing. Do NOT resurrect it to 'completed' or hijack the + # run's preferred pointer (the documented resurrection bug). + return conn.execute( """ UPDATE analysis_runs @@ -1110,11 +1122,11 @@ def complete_mt3_attempt( ).fetchone() if attempt_row is None: raise KeyError(f"Unknown mt3 attempt {attempt_id}") - conn.execute( + cursor = conn.execute( """ UPDATE mt3_attempts SET status = ?, result_json = ?, provenance_json = ?, diagnostics_json = ?, error_json = ?, updated_at = ? - WHERE id = ? + WHERE id = ? AND status NOT IN ('completed', 'failed', 'interrupted') """, ( "completed", @@ -1126,6 +1138,10 @@ def complete_mt3_attempt( attempt_id, ), ) + if cursor.rowcount == 0: + # Already terminal (interrupted while the orphaned MT3 subprocess + # was still finishing) — do not resurrect or hijack the pointer. + return conn.execute( """ UPDATE analysis_runs @@ -1292,11 +1308,11 @@ def complete_interpretation_attempt( ).fetchone() if attempt_row is None: raise KeyError(f"Unknown interpretation attempt {attempt_id}") - conn.execute( + cursor = conn.execute( """ UPDATE interpretation_attempts SET status = ?, grounded_measurement_output_id = ?, grounded_pitch_note_attempt_id = ?, result_json = ?, provenance_json = ?, diagnostics_json = ?, error_json = ?, updated_at = ? - WHERE id = ? + WHERE id = ? AND status NOT IN ('completed', 'failed', 'interrupted') """, ( "completed", @@ -1310,6 +1326,10 @@ def complete_interpretation_attempt( attempt_id, ), ) + if cursor.rowcount == 0: + # Already terminal (interrupted between the is_run_interrupted + # gate and here) — do not resurrect or hijack the pointer. + return conn.execute( """ UPDATE analysis_runs @@ -1334,7 +1354,7 @@ def fail_interpretation_attempt( """ UPDATE interpretation_attempts SET status = ?, grounded_measurement_output_id = ?, grounded_pitch_note_attempt_id = ?, result_json = ?, provenance_json = ?, diagnostics_json = ?, error_json = ?, updated_at = ? - WHERE id = ? + WHERE id = ? AND status NOT IN ('completed', 'failed', 'interrupted') """, ( "failed", @@ -1703,13 +1723,26 @@ def _update_measurement_row( provenance: dict[str, Any] | None = None, diagnostics: dict[str, Any] | None = None, error: dict[str, Any] | None = None, - ) -> None: + guard_terminal: bool = False, + ) -> bool: + # When guard_terminal is set, refuse to transition a measurement row that + # is already terminal. This closes the interrupt TOCTOU: interrupt_run can + # flip the row to 'interrupted' after _execute_measurement_run's + # is_run_interrupted check but before complete/fail. Without the guard the + # late writer resurrects it — and, via complete_measurement, would enqueue + # a fresh follow-up pipeline for an interrupted run. Returns True iff a row + # was actually updated. + terminal_guard = ( + " AND status NOT IN ('completed', 'failed', 'interrupted')" + if guard_terminal + else "" + ) with self._connect() as conn: - conn.execute( - """ + cursor = conn.execute( + f""" UPDATE measurement_outputs SET status = ?, result_json = ?, provenance_json = ?, diagnostics_json = ?, error_json = ?, updated_at = ? - WHERE run_id = ? + WHERE run_id = ?{terminal_guard} """, ( status, @@ -1721,6 +1754,8 @@ def _update_measurement_row( run_id, ), ) + updated = cursor.rowcount > 0 + return updated def _update_attempt_row( self, @@ -1738,7 +1773,7 @@ def _update_attempt_row( f""" UPDATE {table} SET status = ?, result_json = ?, provenance_json = ?, diagnostics_json = ?, error_json = ?, updated_at = ? - WHERE id = ? + WHERE id = ? AND status NOT IN ('completed', 'failed', 'interrupted') """, ( status, @@ -1814,6 +1849,17 @@ def _enqueue_requested_followups(self, run_id: str) -> None: ).fetchone() if run_row is None: return + # Re-assert measurement is still completed before enqueuing. complete_measurement + # commits the measurement-complete update in one transaction and calls this in a + # separate one; an interrupt_run committing in between (measurement worker thread + # vs. event-loop thread) would otherwise leave inert 'queued' follow-up rows on an + # already-interrupted run. The caller's `if updated:` gate can't see this window. + measurement_row = conn.execute( + "SELECT status FROM measurement_outputs WHERE run_id = ?", + (run_id,), + ).fetchone() + if measurement_row is None or measurement_row["status"] != "completed": + return pitch_note_exists = conn.execute( "SELECT 1 FROM pitch_note_translation_attempts WHERE run_id = ? LIMIT 1", (run_id,), diff --git a/apps/backend/server.py b/apps/backend/server.py index 7f4762f3..4b95ff32 100644 --- a/apps/backend/server.py +++ b/apps/backend/server.py @@ -10,7 +10,7 @@ import sys import tempfile import threading -from datetime import datetime +from datetime import datetime, timedelta from math import isfinite from pathlib import Path from typing import Any, Callable @@ -246,6 +246,15 @@ def _interrupt_active_child_processes(run_id: str) -> list[str]: _TEMP_FILE_REGISTRY: dict[str, tuple[str, datetime]] = {} +# Guards _TEMP_FILE_REGISTRY across request handlers and the eviction loop. +_FILE_CACHE_LOCK = threading.Lock() +# How long a request-scoped temp-file path stays cached before it is evicted. +# Both _cache_temp_file/_pop_cached_temp_file and _evict_expired_cache_entries +# referenced this constant, but it was never defined — so any call to that path +# raised NameError. The eviction loop sleeps 300s between sweeps, so the only +# reason this never surfaced is that the path is currently unwired; defining it +# turns the latent landmine into working, exercisable code. +_FILE_CACHE_TTL_SECONDS = 900 def _cache_temp_file(request_id: str, temp_path: str, now: datetime | None = None) -> None: @@ -469,6 +478,19 @@ def _read_subprocess_stream( pass +def _stream_text(value: Any) -> Any: + """Coerce a subprocess stream (str | bytes | None) to text. + + Mirrors the real branch's text=True contract so the mock branch is faithful: + a patched subprocess.run that yields bytes streams — or a TimeoutExpired + carrying bytes — would otherwise make callers' ``marker in stderr`` (str in + bytes) raise TypeError instead of reaching the timeout/error classification. + """ + if isinstance(value, bytes): + return value.decode("utf-8", errors="replace") + return value + + def _run_streamed_subprocess( *, command: list[str], @@ -489,14 +511,14 @@ def _run_streamed_subprocess( except subprocess.TimeoutExpired as exc: return { "returncode": None, - "stdout": exc.stdout, - "stderr": exc.stderr, + "stdout": _stream_text(exc.stdout), + "stderr": _stream_text(exc.stderr), "timedOut": True, } return { "returncode": result.returncode, - "stdout": result.stdout, - "stderr": result.stderr, + "stdout": _stream_text(result.stdout), + "stderr": _stream_text(result.stderr), "timedOut": False, } @@ -1126,11 +1148,42 @@ def _execute_measurement_run( run_standard: bool, run_fast: bool, ) -> dict[str, Any]: - source_artifact = runtime.get_source_artifact(run_id) - source_local_path = runtime.require_local_artifact_path( - source_artifact.get("path"), - purpose="Source audio artifact for measurement", - ) + # Resolve the source artifact under a guard. If this raises (e.g. the + # source audio was swept by artifact cleanup, or is unresolvable in a + # hosted profile) it would otherwise escape to _measurement_worker_loop's + # bare except, leaving the measurement stage stuck 'running' forever with + # no reaper — and the whole run would poll indefinitely. Terminalize it, + # mirroring _execute_pitch_note_attempt / _execute_mt3_attempt. + try: + source_artifact = runtime.get_source_artifact(run_id) + source_local_path = runtime.require_local_artifact_path( + source_artifact.get("path"), + purpose="Source audio artifact for measurement", + ) + except Exception as exc: + runtime.fail_measurement( + run_id, + error={ + "code": "MEASUREMENT_SOURCE_UNAVAILABLE", + "message": str(exc), + "retryable": False, + "phase": ERROR_PHASE_LOCAL_DSP, + }, + provenance=_build_measurement_provenance( + run_separation=run_separation, + run_transcribe=run_transcribe, + run_standard=run_standard, + run_fast=run_fast, + ), + ) + return { + "ok": False, + "statusCode": 500, + "errorCode": "MEASUREMENT_SOURCE_UNAVAILABLE", + "message": str(exc), + "retryable": False, + "diagnostics": None, + } execution = _run_measurement_subprocess( runtime=runtime, run_id=run_id, @@ -1299,21 +1352,30 @@ def _execute_pitch_note_attempt( ) command.extend(["--stem-output-dir", stem_output_dir]) - result = subprocess.run( - command, - capture_output=True, - text=True, - check=False, - timeout=600, + # Route through the registered-subprocess helper so an interrupt/delete + # can terminate this child (Demucs+torchcrepe hold ~2-4GB) instead of + # orphaning it until the 600s timeout. _run_streamed_subprocess registers + # the Popen in _ACTIVE_CHILD_PROCESSES under this stage key and terminates + # it on its own timeout, mirroring the measurement stage. + result = _run_streamed_subprocess( + command=command, + timeout_seconds=600, + run_id=run_id, + stage_key="pitchNoteTranslation", ) - if result.returncode != 0: + if result["returncode"] != 0: + stderr_tail = result["stderr"][-500:] if result["stderr"] else "no stderr" + if result["timedOut"]: + raise RuntimeError( + f"Pitch/note translation subprocess timed out after 600s: {stderr_tail}" + ) raise RuntimeError( - f"Pitch/note translation subprocess failed (exit {result.returncode}): " - f"{result.stderr[-500:] if result.stderr else 'no stderr'}" + f"Pitch/note translation subprocess failed (exit {result['returncode']}): " + f"{stderr_tail}" ) - pitch_note_payload = json.loads(result.stdout) + pitch_note_payload = json.loads(result["stdout"]) transcription_detail = None if isinstance(pitch_note_payload, dict): transcription_detail = pitch_note_payload.get("transcriptionDetail") @@ -1452,26 +1514,33 @@ def _execute_mt3_attempt( # MT3 model load can take ~30s on first call; long tracks add # several minutes of inference. 1800s gives generous headroom # while still bounding the worker. - result = subprocess.run( - command, - capture_output=True, - text=True, - check=False, - timeout=1800, + # Register the child (multi-GB JAX/t5x) so interrupt/delete can reclaim + # it rather than orphaning it until the 1800s timeout — same rationale as + # the pitch_note stage. + result = _run_streamed_subprocess( + command=command, + timeout_seconds=1800, + run_id=run_id, + stage_key="mt3", ) - if result.returncode != 0: - stderr_tail = result.stderr[-2000:] if result.stderr else "" + if result["returncode"] != 0: + stderr_tail = result["stderr"][-2000:] if result["stderr"] else "" if any(marker in stderr_tail for marker in _MT3_NOT_AVAILABLE_MARKERS): raise _Mt3UnavailableError( f"MT3 backend unavailable: {stderr_tail[-500:]}" ) + if result["timedOut"]: + raise RuntimeError( + f"MT3 subprocess timed out after 1800s: " + f"{stderr_tail[-500:] if stderr_tail else 'no stderr'}" + ) raise RuntimeError( - f"MT3 subprocess failed (exit {result.returncode}): " + f"MT3 subprocess failed (exit {result['returncode']}): " f"{stderr_tail[-500:] if stderr_tail else 'no stderr'}" ) - mt3_payload = json.loads(result.stdout) + mt3_payload = json.loads(result["stdout"]) if not isinstance(mt3_payload, dict): raise RuntimeError( f"MT3 subprocess produced non-dict JSON output: {type(mt3_payload).__name__}" @@ -2008,6 +2077,46 @@ def _generate_files_api() -> Any: def _execute_interpretation_attempt( runtime: AnalysisRuntime, attempt: dict[str, Any], +) -> dict[str, Any]: + """Terminalizing wrapper around the interpretation attempt body. + + Any exception during setup (grounding lookup, source-artifact resolution, + profile/config resolution, genai client construction) happens BEFORE the + Gemini call's own try/except. Without this guard such an exception would + escape to _interpretation_worker_loop's bare except, which only logs+sleeps, + leaving the attempt stuck 'running' forever with no reaper + (recover_incomplete_attempts only runs at process startup). Mirror the + defensive pattern in _execute_pitch_note_attempt / _execute_mt3_attempt: + terminalize the attempt so the UI sees a failed state instead of polling + forever. The status guard in fail_interpretation_attempt keeps this a no-op + if the run was already interrupted. + """ + attempt_id = str(attempt["attemptId"]) + try: + return _execute_interpretation_attempt_inner(runtime, attempt) + except Exception as exc: + runtime.fail_interpretation_attempt( + attempt_id, + error={ + "code": "INTERPRETATION_SETUP_FAILED", + "message": str(exc), + "retryable": True, + "phase": ERROR_PHASE_GEMINI, + }, + ) + return { + "ok": False, + "statusCode": 500, + "errorCode": "INTERPRETATION_SETUP_FAILED", + "message": str(exc), + "retryable": True, + "diagnostics": None, + } + + +def _execute_interpretation_attempt_inner( + runtime: AnalysisRuntime, + attempt: dict[str, Any], ) -> dict[str, Any]: run_id = str(attempt["runId"]) profile_id = _coerce_string(attempt.get("profileId"), "producer_summary") diff --git a/apps/backend/tests/test_analysis_runtime.py b/apps/backend/tests/test_analysis_runtime.py index ae1ae119..2080ae78 100644 --- a/apps/backend/tests/test_analysis_runtime.py +++ b/apps/backend/tests/test_analysis_runtime.py @@ -933,3 +933,211 @@ def test_stft_spectrogram_exposes_sample_rate_on_public_ref(self) -> None: self.assertNotIn("path", by_kind["spectrogram_stft"]) self.assertNotIn("contentSha256", by_kind["spectrogram_stft"]) self.assertNotIn("provenance", by_kind["spectrogram_stft"]) + + +class StagedRunInterruptResurrectionTests(unittest.TestCase): + """Regression guard: a late/orphaned stage writer must not resurrect a run + that was interrupted while the stage's subprocess was still finishing. + + Before the staged-run lifecycle fix the stage executors could call + complete/fail on an attempt the interrupt had already flipped to + 'interrupted', flipping it back to a terminal-success/failed state and + (for measurement) enqueuing a fresh downstream pipeline for a cancelled run. + """ + + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory(prefix="asa_resurrect_test_") + + def tearDown(self) -> None: + self.temp_dir.cleanup() + + def _runtime(self): + from analysis_runtime import AnalysisRuntime + + return AnalysisRuntime(Path(self.temp_dir.name) / "runtime", max_pending_per_stage=4) + + def _run_with_completed_measurement(self, runtime, *, interpretation_mode="off"): + created = runtime.create_run( + filename="track.mp3", + content=b"fake-audio", + mime_type="audio/mpeg", + pitch_note_mode="stem_notes", + pitch_note_backend="auto", + interpretation_mode=interpretation_mode, + interpretation_profile="producer_summary", + interpretation_model="gemini-2.5-flash" if interpretation_mode != "off" else None, + ) + run_id = created["runId"] + runtime.reserve_next_measurement_run() + runtime.complete_measurement( + run_id, + payload={"bpm": 128, "durationSeconds": 60.0}, + provenance={"schemaVersion": "measurement.v1"}, + diagnostics={"backendDurationMs": 1000}, + ) + return run_id + + def test_complete_pitch_note_attempt_does_not_resurrect_interrupted_attempt(self) -> None: + runtime = self._runtime() + run_id = self._run_with_completed_measurement(runtime) + attempt = runtime.reserve_next_pitch_note_attempt() + self.assertIsNotNone(attempt) + attempt_id = str(attempt["attemptId"]) + + runtime.interrupt_run(run_id) + + # The now-orphaned subprocess finishes and reports success. + runtime.complete_pitch_note_attempt( + attempt_id, + result={"transcriptionMethod": "stub", "noteCount": 0, "notes": []}, + provenance={"backendId": "auto"}, + ) + + with runtime._connect() as conn: + status = conn.execute( + "SELECT status FROM pitch_note_translation_attempts WHERE id = ?", + (attempt_id,), + ).fetchone()[0] + preferred = conn.execute( + "SELECT preferred_pitch_note_attempt_id FROM analysis_runs WHERE id = ?", + (run_id,), + ).fetchone()[0] + self.assertEqual(status, "interrupted") + self.assertIsNone(preferred) + + def test_fail_pitch_note_attempt_does_not_overwrite_interrupted_attempt(self) -> None: + runtime = self._runtime() + run_id = self._run_with_completed_measurement(runtime) + attempt = runtime.reserve_next_pitch_note_attempt() + attempt_id = str(attempt["attemptId"]) + + runtime.interrupt_run(run_id) + + # Interrupt kills the child → non-zero exit → fail_pitch_note_attempt. + runtime.fail_pitch_note_attempt( + attempt_id, + error={ + "code": "PITCH_NOTE_TRANSLATION_FAILED", + "message": "subprocess terminated", + "retryable": True, + "phase": "pitch_note_translation", + }, + ) + + with runtime._connect() as conn: + status = conn.execute( + "SELECT status FROM pitch_note_translation_attempts WHERE id = ?", + (attempt_id,), + ).fetchone()[0] + self.assertEqual(status, "interrupted") + + def test_complete_mt3_attempt_does_not_resurrect_interrupted_attempt(self) -> None: + runtime = self._runtime() + run_id = self._run_with_completed_measurement(runtime) + mt3_attempt_id = runtime.create_mt3_attempt(run_id) + self.assertTrue(runtime.reserve_mt3_attempt(mt3_attempt_id)) + + runtime.interrupt_run(run_id) + + runtime.complete_mt3_attempt( + mt3_attempt_id, + result={"tracks": []}, + provenance={"checkpointId": "test"}, + ) + + with runtime._connect() as conn: + status = conn.execute( + "SELECT status FROM mt3_attempts WHERE id = ?", + (mt3_attempt_id,), + ).fetchone()[0] + preferred = conn.execute( + "SELECT preferred_mt3_attempt_id FROM analysis_runs WHERE id = ?", + (run_id,), + ).fetchone()[0] + self.assertEqual(status, "interrupted") + self.assertIsNone(preferred) + + def test_complete_measurement_after_interrupt_does_not_enqueue_followups(self) -> None: + runtime = self._runtime() + created = runtime.create_run( + filename="track.mp3", + content=b"fake-audio", + mime_type="audio/mpeg", + pitch_note_mode="stem_notes", + pitch_note_backend="auto", + interpretation_mode="async", + interpretation_profile="producer_summary", + interpretation_model="gemini-2.5-flash", + ) + run_id = created["runId"] + runtime.reserve_next_measurement_run() # → measurement 'running' + + runtime.interrupt_run(run_id) + + # A racing/orphaned measurement subprocess reports success after the + # interrupt. The guard must no-op the status flip AND skip enqueuing the + # downstream pitch-note/interpretation pipeline for a cancelled run. + runtime.complete_measurement( + run_id, + payload={"bpm": 128, "durationSeconds": 60.0}, + provenance={"schemaVersion": "measurement.v1"}, + diagnostics={"backendDurationMs": 1000}, + ) + + with runtime._connect() as conn: + measurement_status = conn.execute( + "SELECT status FROM measurement_outputs WHERE run_id = ?", + (run_id,), + ).fetchone()[0] + pn_count = conn.execute( + "SELECT COUNT(*) FROM pitch_note_translation_attempts WHERE run_id = ?", + (run_id,), + ).fetchone()[0] + interp_count = conn.execute( + "SELECT COUNT(*) FROM interpretation_attempts WHERE run_id = ?", + (run_id,), + ).fetchone()[0] + self.assertEqual(measurement_status, "interrupted") + self.assertEqual(pn_count, 0) + self.assertEqual(interp_count, 0) + + def test_enqueue_followups_bails_when_measurement_interrupted_after_complete(self) -> None: + # Cross-transaction race: complete_measurement committed its update + # (updated=True), then interrupt_run committed (measurement -> interrupted), + # then the measurement worker resumes and calls _enqueue_requested_followups. + # The status re-check must see 'interrupted' and enqueue nothing, so a + # cancelled run can't accrue inert 'queued' follow-up rows. + runtime = self._runtime() + created = runtime.create_run( + filename="track.mp3", + content=b"fake-audio", + mime_type="audio/mpeg", + pitch_note_mode="stem_notes", + pitch_note_backend="auto", + interpretation_mode="async", + interpretation_profile="producer_summary", + interpretation_model="gemini-2.5-flash", + ) + run_id = created["runId"] + runtime.reserve_next_measurement_run() + runtime.interrupt_run(run_id) # measurement -> interrupted + + # The racing worker's now-stale enqueue call. + runtime._enqueue_requested_followups(run_id) + + with runtime._connect() as conn: + pn = conn.execute( + "SELECT COUNT(*) FROM pitch_note_translation_attempts WHERE run_id = ?", + (run_id,), + ).fetchone()[0] + interp = conn.execute( + "SELECT COUNT(*) FROM interpretation_attempts WHERE run_id = ?", + (run_id,), + ).fetchone()[0] + mt3 = conn.execute( + "SELECT COUNT(*) FROM mt3_attempts WHERE run_id = ?", + (run_id,), + ).fetchone()[0] + self.assertEqual(pn, 0) + self.assertEqual(interp, 0) + self.assertEqual(mt3, 0) diff --git a/apps/backend/tests/test_server.py b/apps/backend/tests/test_server.py index 4f5899ef..8495baf4 100644 --- a/apps/backend/tests/test_server.py +++ b/apps/backend/tests/test_server.py @@ -5673,6 +5673,10 @@ def test_mt3_timeout_expired_terminalizes_attempt(self) -> None: error = mt3_stage["error"] self.assertIsNotNone(error) self.assertEqual(error["code"], "MT3_TRANSCRIPTION_FAILED") + # Faithful mock (str streams, not bytes) means the executor reaches + # the timeout-specific branch instead of dying on a str-in-bytes + # TypeError that the broad except would have masked. + self.assertIn("timed out", error["message"]) def test_source_artifact_failure_terminalizes_attempt(self) -> None: """If get_source_artifact raises (e.g. a missing artifact row), the @@ -5783,5 +5787,174 @@ def test_mt3_stage_public_status_annotated_after_normalization(self) -> None: ) +class StageSetupFailureTerminalizationTests(unittest.TestCase): + """Regression guard for the measurement (#7) and interpretation (#3/#6) + setup-failure terminalizers from the 2026-05-30 review. + + Both executors resolve the source artifact / grounding BEFORE their inner + request's own try/except. If that setup raises — e.g. artifact cleanup swept + the source audio while the stage sat queued, or a hosted profile can't + resolve the path — the exception used to escape to the worker loop's bare + except (which only logs+sleeps), leaving the stage stuck 'running' forever + with no reaper (recover_incomplete_attempts runs only at process startup). + The fix wraps setup so the stage terminalizes to 'failed' instead. The MT3 + sibling already has Mt3ExecutorTests.test_source_artifact_failure_*; these + cover the two stages that gained the same guard in this change. + """ + + def test_measurement_source_failure_terminalizes_run(self) -> None: + from analysis_runtime import AnalysisRuntime + + with tempfile.TemporaryDirectory(prefix="asa_measurement_src_fail_") as temp_dir: + runtime = AnalysisRuntime(Path(temp_dir) / "runtime") + created = runtime.create_run( + filename="track.mp3", + content=b"fake-audio", + mime_type="audio/mpeg", + pitch_note_mode="off", + pitch_note_backend="auto", + interpretation_mode="off", + interpretation_profile="producer_summary", + interpretation_model=None, + ) + run_id = created["runId"] + runtime.reserve_next_measurement_run() # measurement -> 'running' + + # Source audio swept by artifact cleanup before the subprocess + # starts: require_local_artifact_path raises FileNotFoundError. + # Without the terminalizer this escapes to _measurement_worker_loop's + # bare except and the stage wedges in 'running' forever (finding #7). + with patch.object( + runtime, + "require_local_artifact_path", + side_effect=FileNotFoundError("source audio missing"), + ): + result = server._execute_measurement_run( + runtime, + run_id, + request_id=run_id, + run_separation=False, + run_transcribe=False, + run_standard=True, + run_fast=False, + ) + + self.assertFalse(result["ok"]) + self.assertEqual(result["errorCode"], "MEASUREMENT_SOURCE_UNAVAILABLE") + snapshot = runtime.get_run(run_id) + measurement_stage = snapshot["stages"]["measurement"] + # The key assertion: terminal, NOT stuck 'running'. + self.assertEqual(measurement_stage["status"], "failed") + self.assertEqual( + measurement_stage["error"]["code"], "MEASUREMENT_SOURCE_UNAVAILABLE" + ) + + def test_interpretation_setup_failure_terminalizes_attempt(self) -> None: + from analysis_runtime import AnalysisRuntime + + with tempfile.TemporaryDirectory(prefix="asa_interp_setup_fail_") as temp_dir: + runtime = AnalysisRuntime(Path(temp_dir) / "runtime") + created = runtime.create_run( + filename="track.mp3", + content=b"fake-audio", + mime_type="audio/mpeg", + pitch_note_mode="off", + pitch_note_backend="auto", + interpretation_mode="async", + interpretation_profile="producer_summary", + interpretation_model="gemini-2.5-flash", + ) + run_id = created["runId"] + runtime.reserve_next_measurement_run() + runtime.complete_measurement( + run_id, + payload={"bpm": 120.0}, + provenance={}, + diagnostics={}, + ) + attempt = runtime.reserve_next_interpretation_attempt() # interp -> 'running' + self.assertIsNotNone(attempt, "interpretation attempt should be reservable") + + # Grounding lookup raises before the Gemini call's own try/except + # (e.g. a delete-race on the run/measurement rows). Without the + # wrapping terminalizer this escapes to _interpretation_worker_loop's + # bare except and the attempt strands 'running' forever (findings + # #3/#6) — the UI then polls indefinitely with no error surfaced. + with patch.object( + runtime, + "get_interpretation_grounding", + side_effect=KeyError("missing grounding"), + ): + result = server._execute_interpretation_attempt(runtime, attempt) + + self.assertFalse(result["ok"]) + self.assertEqual(result["errorCode"], "INTERPRETATION_SETUP_FAILED") + snapshot = runtime.get_run(run_id) + interpretation_stage = snapshot["stages"]["interpretation"] + # The key assertion: terminal, NOT stuck 'running'. + self.assertEqual(interpretation_stage["status"], "failed") + self.assertEqual( + interpretation_stage["error"]["code"], "INTERPRETATION_SETUP_FAILED" + ) + + +class TempFileCacheTests(unittest.TestCase): + """Exercises the request-scoped temp-file cache path. + + _cache_temp_file / _pop_cached_temp_file / _evict_expired_cache_entries + referenced _FILE_CACHE_LOCK and _FILE_CACHE_TTL_SECONDS, which were never + defined — so any call raised NameError the moment that branch ran. These + tests drive the path directly so the landmine cannot silently return. + """ + + def setUp(self) -> None: + server._TEMP_FILE_REGISTRY.clear() + + def tearDown(self) -> None: + server._TEMP_FILE_REGISTRY.clear() + + def _make_temp_file(self) -> str: + handle = tempfile.NamedTemporaryFile(prefix="asa_cache_test_", delete=False) + handle.close() + path = handle.name + self.addCleanup(lambda: Path(path).exists() and Path(path).unlink()) + return path + + def test_cache_then_pop_returns_path_before_expiry(self) -> None: + path = self._make_temp_file() + base = datetime(2026, 1, 1, 0, 0, 0) + server._cache_temp_file("req-1", path, now=base) + with patch.object(server, "_current_time", return_value=base + timedelta(seconds=1)): + self.assertEqual(server._pop_cached_temp_file("req-1"), path) + # A successful pop also removes the entry from the registry. + self.assertNotIn("req-1", server._TEMP_FILE_REGISTRY) + + def test_pop_evicts_and_cleans_up_an_expired_entry(self) -> None: + path = self._make_temp_file() + base = datetime(2026, 1, 1, 0, 0, 0) + server._cache_temp_file("req-2", path, now=base) + with patch.object( + server, + "_current_time", + return_value=base + timedelta(seconds=server._FILE_CACHE_TTL_SECONDS + 1), + ): + self.assertIsNone(server._pop_cached_temp_file("req-2")) + self.assertNotIn("req-2", server._TEMP_FILE_REGISTRY) + self.assertFalse(Path(path).exists()) + + def test_evict_expired_cache_entries_sweeps_and_cleans_up(self) -> None: + path = self._make_temp_file() + base = datetime(2026, 1, 1, 0, 0, 0) + server._cache_temp_file("req-3", path, now=base) + with patch.object( + server, + "_current_time", + return_value=base + timedelta(seconds=server._FILE_CACHE_TTL_SECONDS + 1), + ): + server._evict_expired_cache_entries() + self.assertNotIn("req-3", server._TEMP_FILE_REGISTRY) + self.assertFalse(Path(path).exists()) + + if __name__ == "__main__": unittest.main() diff --git a/apps/ui/src/App.tsx b/apps/ui/src/App.tsx index a4953ea1..2cf7fc41 100644 --- a/apps/ui/src/App.tsx +++ b/apps/ui/src/App.tsx @@ -46,6 +46,7 @@ import { Phase1Result, } from './types'; import type { AnalysisResultsProps } from './components/AnalysisResults'; +import { ErrorBoundary } from './components/ErrorBoundary'; import { loadPhase2RequestedPreference, savePhase2RequestedPreference, @@ -1405,6 +1406,7 @@ export default function App() { )} {phase1ForRender ? ( + @@ -1448,6 +1450,7 @@ export default function App() { } /> + ) : null} diff --git a/apps/ui/src/components/ErrorBoundary.tsx b/apps/ui/src/components/ErrorBoundary.tsx new file mode 100644 index 00000000..c9203e0d --- /dev/null +++ b/apps/ui/src/components/ErrorBoundary.tsx @@ -0,0 +1,90 @@ +import { Component, type ErrorInfo, type ReactNode } from 'react'; + +interface ErrorBoundaryProps { + children: ReactNode; + /** Optional render-prop fallback. Receives the captured error and a reset callback. */ + fallback?: (error: Error, reset: () => void) => ReactNode; + /** Heading for the default fallback. */ + title?: string; + /** Notified when an error is captured (e.g. for diagnostics/telemetry). */ + onError?: (error: Error, info: ErrorInfo) => void; +} + +interface ErrorBoundaryState { + error: Error | null; +} + +/** + * Catches render and lazy-import failures in its subtree so a single throw (or a + * chunk-load failure in a lazily-loaded view) cannot blank the entire app. + * + * The default fallback is recoverable: "Try again" clears the error state and + * re-renders the subtree — which recovers a transient *render* error (the lazy + * module already resolved). It does NOT recover a failed *chunk load*: + * React.lazy caches the rejected import, so re-rendering re-throws the cached + * rejection. "Reload page" (a hard reload) is the reliable recovery for a + * chunk-load failure. + * + * Intentionally self-contained (no design-system imports): an error boundary + * must stay renderable even when the component tree it guards — potentially + * including shared UI primitives — is what failed. + */ +export class ErrorBoundary extends Component { + state: ErrorBoundaryState = { error: null }; + + static getDerivedStateFromError(error: Error): ErrorBoundaryState { + return { error }; + } + + componentDidCatch(error: Error, info: ErrorInfo): void { + console.error('[ErrorBoundary] subtree render failure', error, info); + this.props.onError?.(error, info); + } + + reset = (): void => { + this.setState({ error: null }); + }; + + render(): ReactNode { + const { error } = this.state; + if (!error) { + return this.props.children; + } + + if (this.props.fallback) { + return this.props.fallback(error, this.reset); + } + + return ( +
+
+

+ {this.props.title ?? 'This view failed to render'} +

+

+ Your analysis is safe — retry to re-render it, or reload the page. +

+ {error.message ? ( +

{error.message}

+ ) : null} +
+
+ + +
+
+ ); + } +} diff --git a/apps/ui/src/services/analyzer.ts b/apps/ui/src/services/analyzer.ts index ce416b7d..f527e841 100644 --- a/apps/ui/src/services/analyzer.ts +++ b/apps/ui/src/services/analyzer.ts @@ -9,11 +9,21 @@ import { projectPhase2FromRun, projectStemSummaryFromRun, } from './analysisRunsClient'; -import { createUserCancelledError, mapBackendError } from './backendPhase1Client'; +import { createClientTimeoutError, createUserCancelledError, mapBackendError } from './backendPhase1Client'; import { MEASUREMENT_LABEL, INTERPRETATION_LABEL, INTERPRETATION_SKIPPED_LABEL } from './phaseLabels'; import { validatePhase2Consistency } from './phase2Validator'; const DEFAULT_POLL_INTERVAL_MS = 1_000; +// Per-poll fetch deadline. Generous: the box may be under multi-GB MT3/Demucs +// memory pressure, so a snapshot read can be momentarily slow on a healthy run. +// A single poll exceeding this fails the run (consistent with other transport +// errors) rather than hanging the client forever. +const DEFAULT_POLL_REQUEST_TIMEOUT_MS = 60_000; +// Overall wall-clock backstop. Worst single-run case is measurement + +// max(pitch-note 600s, MT3 1800s) (concurrent peers) + interpretation, so a +// real run finishes well under this; it only bounds a backend that never +// terminalizes a stage. +const DEFAULT_RUN_WALL_CLOCK_MS = 90 * 60 * 1_000; export interface AnalyzeAudioUpdate { runId: string; @@ -32,6 +42,10 @@ export interface AnalyzeAudioOptions { signal?: AbortSignal; onRunUpdate?: (update: AnalyzeAudioUpdate) => void; pollIntervalMs?: number; + /** Per-poll fetch deadline (ms). Defaults to DEFAULT_POLL_REQUEST_TIMEOUT_MS. */ + pollRequestTimeoutMs?: number; + /** Overall wall-clock budget for the run (ms). Defaults to DEFAULT_RUN_WALL_CLOCK_MS. */ + maxRunDurationMs?: number; transcribe?: boolean; phase2Requested?: boolean; phase2ConfigEnabled?: boolean; @@ -64,6 +78,49 @@ function delay(ms: number, signal?: AbortSignal): Promise { }); } +export async function raceWithDeadline( + work: Promise, + timeoutMs: number, + timeoutMessage: string, +): Promise { + // If the deadline wins the race, `work` is abandoned; attach a no-op catch so + // a later rejection can't surface as an unhandled promise rejection. + work.catch(() => {}); + + let timer: ReturnType | undefined; + const deadline = new Promise((_, reject) => { + timer = globalThis.setTimeout(() => { + reject(createClientTimeoutError(timeoutMessage, timeoutMs)); + }, timeoutMs); + }); + + try { + return await Promise.race([work, deadline]); + } finally { + if (timer !== undefined) { + globalThis.clearTimeout(timer); + } + } +} + +function pollSnapshotWithDeadline( + runId: string, + apiBaseUrl: string, + signal: AbortSignal | undefined, + timeoutMs: number, +): Promise { + // The per-poll fetch has no native deadline, so a wedged backend would hang + // this await forever — which also prevents the wall-clock budget in the poll + // loop from ever being re-checked. Race it against a deadline. We must NOT + // feed a timeout signal into getAnalysisRun: fetchJson converts any aborted + // signal into USER_CANCELLED, which would mislabel a timeout as a user cancel. + return raceWithDeadline( + getAnalysisRun(runId, { apiBaseUrl, signal }), + timeoutMs, + 'Timed out waiting for an analysis status update from the backend.', + ); +} + function buildPhase2SkippedLog( audioMetadata: DiagnosticLogEntry['audioMetadata'], requestId: string | undefined, @@ -256,13 +313,26 @@ export async function monitorAnalysisRun( let interpretationReported = false; let stemSummaryQueued = false; + const startedAt = Date.now(); + const maxRunDurationMs = analysisOptions?.maxRunDurationMs ?? DEFAULT_RUN_WALL_CLOCK_MS; + const pollRequestTimeoutMs = + analysisOptions?.pollRequestTimeoutMs ?? DEFAULT_POLL_REQUEST_TIMEOUT_MS; + try { while (true) { throwIfUserCancelled(analysisOptions?.signal); - let snapshot = await getAnalysisRun(runId, { - apiBaseUrl: appConfig.apiBaseUrl, - signal: analysisOptions?.signal, - }); + if (Date.now() - startedAt > maxRunDurationMs) { + throw createClientTimeoutError( + 'Analysis exceeded the maximum time budget before completing.', + maxRunDurationMs, + ); + } + let snapshot = await pollSnapshotWithDeadline( + runId, + appConfig.apiBaseUrl, + analysisOptions?.signal, + pollRequestTimeoutMs, + ); throwIfUserCancelled(analysisOptions?.signal); if (hasInterpretationProfileAttempt(snapshot, 'stem_summary')) { @@ -325,12 +395,18 @@ export async function monitorAnalysisRun( interpretationConfigEnabled, ) ) { - snapshot = await createInterpretationAttempt(runId, { - apiBaseUrl: appConfig.apiBaseUrl, - interpretationProfile: 'stem_summary', - interpretationModel: modelName, - signal: analysisOptions?.signal, - }); + // Bound this POST too: a wedged enqueue would otherwise hang the await + // and bypass the wall-clock budget (only re-checked at the loop top). + snapshot = await raceWithDeadline( + createInterpretationAttempt(runId, { + apiBaseUrl: appConfig.apiBaseUrl, + interpretationProfile: 'stem_summary', + interpretationModel: modelName, + signal: analysisOptions?.signal, + }), + pollRequestTimeoutMs, + 'Timed out enqueuing the stem-summary interpretation request.', + ); stemSummaryQueued = true; emitRunUpdate(snapshot); } diff --git a/apps/ui/src/services/backendPhase1Client.ts b/apps/ui/src/services/backendPhase1Client.ts index e2074c18..05f7082d 100644 --- a/apps/ui/src/services/backendPhase1Client.ts +++ b/apps/ui/src/services/backendPhase1Client.ts @@ -77,6 +77,13 @@ export function createUserCancelledError(message = "Analysis was cancelled by th return new BackendClientError("USER_CANCELLED", message); } +export function createClientTimeoutError( + message = "The UI timed out waiting for the local DSP backend response.", + timeoutMs?: number, +): BackendClientError { + return new BackendClientError("CLIENT_TIMEOUT", message, { timeoutMs }); +} + export interface AnalyzePhase1Options { apiBaseUrl: string; timeoutMs?: number; @@ -932,6 +939,21 @@ function parseOptionalAcidDetail(value: unknown): AcidDetail | null { }; } +function parseOptionalPerBandRt60(value: unknown): ReverbDetail['perBandRt60'] { + if (value === undefined || value === null) return null; + if (!isRecord(value)) return null; + const result: NonNullable = {}; + const low = toNumber(value.low); + const lowMids = toNumber(value.lowMids); + const highMids = toNumber(value.highMids); + const highs = toNumber(value.highs); + if (low !== null) result.low = low; + if (lowMids !== null) result.lowMids = lowMids; + if (highMids !== null) result.highMids = highMids; + if (highs !== null) result.highs = highs; + return result; +} + function parseOptionalReverbDetail(value: unknown): ReverbDetail | null { if (value === undefined || value === null) return null; if (!isRecord(value)) return null; @@ -940,6 +962,11 @@ function parseOptionalReverbDetail(value: unknown): ReverbDetail | null { isWet: value.isWet === true, tailEnergyRatio: toNumber(value.tailEnergyRatio), measured: value.measured === true, + // Carry the per-band RT60 and pre-delay subfields the Phase 2 prompt is + // told it may cite (reverbDetail.perBandRt60.*, reverbDetail.preDelayMs). + // Dropping them here made legitimate citations fail the existence check. + perBandRt60: parseOptionalPerBandRt60(value.perBandRt60), + preDelayMs: toNumber(value.preDelayMs), }; } @@ -957,6 +984,11 @@ function parseOptionalVocalDetail(value: unknown): VocalDetail | null { vocalEnergyRatio, formantStrength, mfccLikelihood, + // Carry the Demucs-ghost-stem proxies the Phase 2 prompt may cite + // (vocalDetail.stemEnergyRatio, vocalDetail.stemOtherCorrelation). Kept as + // number|null so the citation path resolves whenever the backend measured it. + stemEnergyRatio: toNumber(value.stemEnergyRatio), + stemOtherCorrelation: toNumber(value.stemOtherCorrelation), }; } diff --git a/apps/ui/src/services/phase2Validator.ts b/apps/ui/src/services/phase2Validator.ts index a936f075..5b98f471 100644 --- a/apps/ui/src/services/phase2Validator.ts +++ b/apps/ui/src/services/phase2Validator.ts @@ -742,10 +742,14 @@ function normalizeKey(key: string): string { return key .toLowerCase() .replace(/\s+/g, ' ') - .replace(/major/g, 'major') - .replace(/minor/g, 'minor') - .replace(/maj/g, 'major') - .replace(/min(?!or)/g, 'minor') + // Collapse "maj"/"major" → "major" and "min"/"minor" → "minor" in a single + // pass each. The previous form ran `maj`→`major` AFTER `major`→`major`, so + // the "maj" inside an already-spelled "major" was rewritten again, yielding + // "majoror" — which made a valid "Maj" abbreviation mismatch a spelled-out + // "major" and raised a FALSE key-contradiction. Word boundaries keep this + // from touching unrelated substrings. + .replace(/\bmaj(?:or)?\b/g, 'major') + .replace(/\bmin(?:or)?\b/g, 'minor') .trim(); } diff --git a/apps/ui/tests/services/analyzer.test.ts b/apps/ui/tests/services/analyzer.test.ts index a9ab3757..228654f1 100644 --- a/apps/ui/tests/services/analyzer.test.ts +++ b/apps/ui/tests/services/analyzer.test.ts @@ -59,7 +59,7 @@ vi.mock('../../src/services/phase2Validator', () => ({ })); import { BackendClientError } from '../../src/services/backendPhase1Client'; -import { analyzeAudio } from '../../src/services/analyzer'; +import { analyzeAudio, raceWithDeadline } from '../../src/services/analyzer'; const phase1Result: Phase1Result = { bpm: 128, @@ -748,3 +748,109 @@ describe('analyzeAudio', () => { consoleErrorSpy.mockRestore(); }); }); + +describe('analyzeAudio polling timeouts', () => { + const runningSnapshot = () => + makeRunSnapshot({ + stages: { + measurement: { + status: 'running', + authoritative: true, + result: null, + provenance: null, + diagnostics: null, + error: null, + }, + pitchNoteTranslation: { + status: 'blocked', + authoritative: false, + preferredAttemptId: null, + attemptsSummary: [], + result: null, + provenance: null, + diagnostics: null, + error: null, + }, + interpretation: { + status: 'blocked', + authoritative: false, + preferredAttemptId: null, + attemptsSummary: [], + result: null, + provenance: null, + diagnostics: null, + error: null, + }, + }, + }); + + it('fails with CLIENT_TIMEOUT when a single poll exceeds the per-request deadline', async () => { + createAnalysisRunMock.mockResolvedValue(runningSnapshot()); + // A wedged backend: the poll fetch never resolves. Without a per-poll + // deadline this would hang the client forever (review finding #4). + getAnalysisRunMock.mockImplementation(() => new Promise(() => {})); + + const file = new File(['audio-data'], 'track.mp3', { type: 'audio/mpeg' }); + const onError = vi.fn(); + + await analyzeAudio(file, 'gemini-2.5-pro', null, vi.fn(), vi.fn(), onError, { + pollRequestTimeoutMs: 10, + pollIntervalMs: 0, + }); + + expect(onError).toHaveBeenCalledTimes(1); + const error = onError.mock.calls[0]?.[0]; + expect(error).toBeInstanceOf(BackendClientError); + expect((error as BackendClientError).code).toBe('CLIENT_TIMEOUT'); + }); + + it('fails with CLIENT_TIMEOUT when the overall run budget is exceeded', async () => { + createAnalysisRunMock.mockResolvedValue(runningSnapshot()); + // Never reaches a terminal stage, so only the wall-clock budget can end it. + getAnalysisRunMock.mockResolvedValue(runningSnapshot()); + + const file = new File(['audio-data'], 'track.mp3', { type: 'audio/mpeg' }); + const onError = vi.fn(); + + // Negative budget forces the wall-clock guard to trip on the first iteration. + await analyzeAudio(file, 'gemini-2.5-pro', null, vi.fn(), vi.fn(), onError, { + maxRunDurationMs: -1, + pollIntervalMs: 0, + }); + + expect(onError).toHaveBeenCalledTimes(1); + const error = onError.mock.calls[0]?.[0]; + expect(error).toBeInstanceOf(BackendClientError); + expect((error as BackendClientError).code).toBe('CLIENT_TIMEOUT'); + }); +}); + +describe('raceWithDeadline', () => { + it('resolves with the work value when work settles before the deadline', async () => { + await expect(raceWithDeadline(Promise.resolve('snapshot'), 1_000, 'too slow')).resolves.toBe( + 'snapshot', + ); + }); + + it('rejects with CLIENT_TIMEOUT when the deadline wins', async () => { + const neverSettles = new Promise(() => {}); + const rejection = raceWithDeadline(neverSettles, 5, 'too slow'); + await expect(rejection).rejects.toBeInstanceOf(BackendClientError); + await expect(rejection).rejects.toMatchObject({ code: 'CLIENT_TIMEOUT' }); + }); + + it('does not surface the abandoned work rejection after the deadline wins', async () => { + // The work rejects AFTER the deadline fires; the neutralizing catch inside + // raceWithDeadline must keep it from becoming an unhandled rejection. + let rejectWork: (reason: unknown) => void = () => {}; + const work = new Promise((_, reject) => { + rejectWork = reject; + }); + + await expect(raceWithDeadline(work, 5, 'too slow')).rejects.toBeInstanceOf(BackendClientError); + + rejectWork(new Error('late backend failure')); + // Let the rejection settle; a missing neutralizer would surface here. + await new Promise((resolve) => globalThis.setTimeout(resolve, 10)); + }); +}); diff --git a/apps/ui/tests/services/backendPhase1Client.test.ts b/apps/ui/tests/services/backendPhase1Client.test.ts index ea4c736f..2abb13f0 100644 --- a/apps/ui/tests/services/backendPhase1Client.test.ts +++ b/apps/ui/tests/services/backendPhase1Client.test.ts @@ -247,6 +247,8 @@ const validPayload = { isWet: true, tailEnergyRatio: 0.35, measured: true, + perBandRt60: { low: 1.4, lowMids: 1.1, highMids: 0.8, highs: 0.5 }, + preDelayMs: 22.5, }, vocalDetail: { hasVocals: false, @@ -254,6 +256,8 @@ const validPayload = { vocalEnergyRatio: 0.02, formantStrength: 0.05, mfccLikelihood: 0.1, + stemEnergyRatio: 0.12, + stemOtherCorrelation: 0.41, }, supersawDetail: { isSupersaw: false, @@ -401,7 +405,18 @@ describe('parseBackendAnalyzeResponse', () => { expect(parsed.phase1.acidDetail?.confidence).toBe(0.12); expect(parsed.phase1.reverbDetail?.isWet).toBe(true); expect(parsed.phase1.reverbDetail?.rt60).toBe(1.2); + // Contract-drift guard: the reverb/vocal subfields the Phase 2 prompt may + // cite must survive parsing, or legitimate citations fail the existence check. + expect(parsed.phase1.reverbDetail?.perBandRt60).toEqual({ + low: 1.4, + lowMids: 1.1, + highMids: 0.8, + highs: 0.5, + }); + expect(parsed.phase1.reverbDetail?.preDelayMs).toBe(22.5); expect(parsed.phase1.vocalDetail?.hasVocals).toBe(false); + expect(parsed.phase1.vocalDetail?.stemEnergyRatio).toBe(0.12); + expect(parsed.phase1.vocalDetail?.stemOtherCorrelation).toBe(0.41); expect(parsed.phase1.supersawDetail?.isSupersaw).toBe(false); expect(parsed.phase1.bassDetail?.type).toBe('punchy'); expect(parsed.phase1.kickDetail?.kickCount).toBe(256); diff --git a/apps/ui/tests/services/errorBoundary.test.ts b/apps/ui/tests/services/errorBoundary.test.ts new file mode 100644 index 00000000..e3cff3d5 --- /dev/null +++ b/apps/ui/tests/services/errorBoundary.test.ts @@ -0,0 +1,19 @@ +import { describe, it, expect } from 'vitest'; + +import { ErrorBoundary } from '../../src/components/ErrorBoundary'; + +// Vitest runs in the `node` environment (no jsdom), so we exercise the +// boundary's capture logic directly rather than rendering it. The fallback UI +// render is covered by tests/smoke/error-boundary.spec.ts, which aborts the +// lazy AnalysisResults chunk and asserts the fallback (alert + actions) renders. +describe('ErrorBoundary', () => { + it('captures a thrown error into render state via getDerivedStateFromError', () => { + const error = new Error('AnalysisResults failed to render'); + expect(ErrorBoundary.getDerivedStateFromError(error)).toEqual({ error }); + }); + + it('treats a chunk-load failure the same as any other error', () => { + const chunkError = new Error('Loading chunk 42 failed'); + expect(ErrorBoundary.getDerivedStateFromError(chunkError).error).toBe(chunkError); + }); +}); diff --git a/apps/ui/tests/services/phase2Validator.test.ts b/apps/ui/tests/services/phase2Validator.test.ts index deaf4fd4..0e5a01e7 100644 --- a/apps/ui/tests/services/phase2Validator.test.ts +++ b/apps/ui/tests/services/phase2Validator.test.ts @@ -1484,3 +1484,130 @@ describe('Loudness action presence (objective safety net)', () => { expect(loudnessViolations(result)).toHaveLength(0); }); }); + +describe('normalizeKey abbreviation handling', () => { + it('does not flag a contradiction when Phase 2 abbreviates a major key as "Maj"', () => { + // Regression: normalizeKey used to rewrite "major" into "majoror", so a + // valid "A Maj" mention mismatched the spelled-out "A major" and raised a + // FALSE key contradiction. + const phase1 = createBasePhase1({ key: 'A major' }); + const phase2 = createBasePhase2({ + trackCharacter: 'Bright arpeggiated lead in A Maj throughout the drop.', + }); + + const result = validatePhase2Consistency(phase1, phase2); + + expect(result.violations.find((v) => v.field === 'key')).toBeUndefined(); + }); + + it('matches a "Min" abbreviation against a spelled-out minor key', () => { + const phase1 = createBasePhase1({ key: 'F minor' }); + const phase2 = createBasePhase2({ trackCharacter: 'Dark sustained pad in F Min.' }); + + const result = validatePhase2Consistency(phase1, phase2); + + expect(result.violations.find((v) => v.field === 'key')).toBeUndefined(); + }); + + it('still flags a genuine key contradiction (the fix did not weaken detection)', () => { + const phase1 = createBasePhase1({ key: 'F minor' }); + const phase2 = createBasePhase2({ trackCharacter: 'Uplifting riff in A major.' }); + + const result = validatePhase2Consistency(phase1, phase2); + + expect(result.violations.find((v) => v.field === 'key')).toBeDefined(); + }); +}); + +describe('Phase 1 citable-field contract (reverb/vocal subfields)', () => { + it('collects the reverb/vocal subfields the Phase 2 prompt is allowed to cite', () => { + // Cross-app contract guard for the camelCase field-drop class of bug: a + // field the prompt may cite must survive into Phase1Result and be visible to + // the citation-existence checker. These four were silently dropped by the + // parser, so legitimate citations to them failed. + const phase1 = createBasePhase1({ + reverbDetail: { + rt60: 1.2, + isWet: true, + tailEnergyRatio: 0.3, + measured: true, + perBandRt60: { low: 1.4, lowMids: 1.1, highMids: 0.8, highs: 0.5 }, + preDelayMs: 22.5, + }, + vocalDetail: { + hasVocals: true, + confidence: 0.7, + vocalEnergyRatio: 0.4, + formantStrength: 0.6, + mfccLikelihood: 0.5, + stemEnergyRatio: 0.12, + stemOtherCorrelation: 0.41, + }, + }); + + const paths = collectPhase1FieldPaths(phase1); + + for (const citable of [ + 'reverbDetail.preDelayMs', + 'reverbDetail.perBandRt60.low', + 'reverbDetail.perBandRt60.lowMids', + 'reverbDetail.perBandRt60.highMids', + 'reverbDetail.perBandRt60.highs', + 'vocalDetail.stemEnergyRatio', + 'vocalDetail.stemOtherCorrelation', + ]) { + expect(paths.has(citable)).toBe(true); + } + }); + + it('accepts a recommendation that cites the previously-dropped reverb/vocal fields', () => { + // End-to-end guard for the actual user-facing failure: a recommendation + // citing these paths used to fail the existence check because the parser + // dropped them. With the parser fixed, validatePhase2Consistency must raise + // no MISSING_CITATION for them. + const droppedFieldPaths = [ + 'reverbDetail.preDelayMs', + 'reverbDetail.perBandRt60.low', + 'vocalDetail.stemEnergyRatio', + 'vocalDetail.stemOtherCorrelation', + ]; + const phase1 = createBasePhase1({ + reverbDetail: { + rt60: 1.2, + isWet: true, + tailEnergyRatio: 0.3, + measured: true, + perBandRt60: { low: 1.4, lowMids: 1.1, highMids: 0.8, highs: 0.5 }, + preDelayMs: 22.5, + }, + vocalDetail: { + hasVocals: true, + confidence: 0.7, + vocalEnergyRatio: 0.4, + formantStrength: 0.6, + mfccLikelihood: 0.5, + stemEnergyRatio: 0.12, + stemOtherCorrelation: 0.41, + }, + }); + const phase2 = createBasePhase2({ + abletonRecommendations: [ + { + device: 'Reverb', + category: 'REVERB', + parameter: 'Pre-delay', + value: '22 ms', + reason: 'Match the measured pre-delay, per-band decay and vocal-stem presence.', + phase1Fields: droppedFieldPaths, + }, + ], + }); + + const result = validatePhase2Consistency(phase1, phase2); + + const droppedFieldCitationErrors = result.violations.filter( + (v) => v.type === 'MISSING_CITATION' && droppedFieldPaths.includes(String(v.phase2Value)), + ); + expect(droppedFieldCitationErrors).toHaveLength(0); + }); +}); diff --git a/apps/ui/tests/smoke/error-boundary.spec.ts b/apps/ui/tests/smoke/error-boundary.spec.ts new file mode 100644 index 00000000..6a15453e --- /dev/null +++ b/apps/ui/tests/smoke/error-boundary.spec.ts @@ -0,0 +1,164 @@ +import { test, expect } from '@playwright/test'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Smoke coverage for the ErrorBoundary FALLBACK RENDER. + * + * Unit tests cover the boundary's capture logic (getDerivedStateFromError), but + * not the fallback UI itself (Vitest is node-env). This drives the real + * upload -> run -> results flow against a mocked backend, then ABORTS the lazy + * AnalysisResults chunk request to force a load failure, and asserts the + * ErrorBoundary fallback renders instead of a blank page. That chunk-load + * failure is the production failure mode the boundary exists to contain. + * + * Model: tests/smoke/transcription-pianoroll.spec.ts (same mocked run-lifecycle + * pattern). Smoke runs the Vite dev server (playwright.config.ts webServer), so + * the lazy module is served unbundled at a stable, hashless path containing + * "AnalysisResults". + */ + +const testDir = path.dirname(fileURLToPath(import.meta.url)); +const RUN_ID = 'run_smoke_error_boundary_001'; + +// Minimal valid Phase1Result — enough for the results surface (and thus the +// lazy AnalysisResults import) to mount once measurement completes. +const PHASE1_RESULT = { + bpm: 126, + bpmConfidence: 0.93, + key: 'F minor', + keyConfidence: 0.88, + timeSignature: '4/4', + durationSeconds: 210.6, + lufsIntegrated: -7.9, + truePeak: -0.2, + stereoWidth: 0.69, + stereoCorrelation: 0.84, + spectralBalance: { + subBass: -0.7, + lowBass: 1.2, + lowMids: 0.0, + mids: -0.3, + upperMids: 0.4, + highs: 1.0, + brilliance: 0.8, + }, +}; + +const BASE_RUN = { + runId: RUN_ID, + requestedStages: { + pitchNoteMode: 'off', + pitchNoteBackend: 'auto', + interpretationMode: 'off', + interpretationProfile: 'producer_summary', + interpretationModel: null, + }, + artifacts: { + sourceAudio: { + artifactId: 'artifact_error_boundary_001', + filename: 'silence.wav', + mimeType: 'audio/wav', + sizeBytes: 2048, + contentSha256: 'abc123', + path: 'uploads/test.wav', + }, + }, +}; + +const NOT_REQUESTED_STAGE = { + status: 'not_requested', + authoritative: false, + preferredAttemptId: null, + attemptsSummary: [], + result: null, + provenance: null, + diagnostics: null, + error: null, +}; + +async function stubEstimate(page: import('@playwright/test').Page) { + let hits = 0; + await page.route('**/api/analysis-runs/estimate', async (route) => { + hits += 1; + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + requestId: 'req_estimate_error_boundary_001', + estimate: { + durationSeconds: 210.6, + totalLowMs: 107000, + totalHighMs: 203000, + stages: [{ key: 'local_dsp', label: 'Local DSP analysis', lowMs: 22000, highMs: 38000 }], + }, + }), + }); + }); + return () => hits; +} + +/** Mocks create-run (POST) + a completed-measurement snapshot (GET). */ +async function mockRunLifecycle(page: import('@playwright/test').Page) { + await page.route('**/api/analysis-runs', async (route) => { + if (route.request().method() !== 'POST') { + await route.fallback(); + return; + } + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + ...BASE_RUN, + stages: { + measurement: { status: 'queued', authoritative: true, result: null, provenance: null, diagnostics: null, error: null }, + pitchNoteTranslation: NOT_REQUESTED_STAGE, + interpretation: NOT_REQUESTED_STAGE, + }, + }), + }); + }); + + await page.route(`**/api/analysis-runs/${RUN_ID}`, async (route) => { + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + ...BASE_RUN, + stages: { + measurement: { + status: 'completed', + authoritative: true, + result: PHASE1_RESULT, + provenance: null, + diagnostics: { timings: { totalMs: 980, analysisMs: 900, serverOverheadMs: 80, flagsUsed: [], fileSizeBytes: 2048, fileDurationSeconds: 210.6, msPerSecondOfAudio: 4.6 } }, + error: null, + }, + pitchNoteTranslation: NOT_REQUESTED_STAGE, + interpretation: NOT_REQUESTED_STAGE, + }, + }), + }); + }); +} + +test('ErrorBoundary fallback renders when the AnalysisResults chunk fails to load', async ({ page }) => { + const getEstimateHits = await stubEstimate(page); + await mockRunLifecycle(page); + + // Force the lazy AnalysisResults chunk to fail loading. Registered before + // navigation so it intercepts the dynamic import when measurement completes. + await page.route('**/AnalysisResults**', (route) => route.abort('failed')); + + await page.goto('/', { waitUntil: 'networkidle' }); + await page.setInputFiles('#audio-upload', path.resolve(testDir, './fixtures/silence.wav')); + await expect.poll(() => getEstimateHits()).toBeGreaterThanOrEqual(1); + await page.getByRole('button', { name: /Run Analysis/i }).click(); + + // The boundary catches the lazy-import rejection and renders its recoverable + // fallback — the app is NOT blanked. + await expect(page.getByRole('alert')).toBeVisible(); + await expect(page.getByText('The analysis results view failed to render')).toBeVisible(); + await expect(page.getByRole('button', { name: /Try again/i })).toBeVisible(); + await expect(page.getByRole('button', { name: /Reload page/i })).toBeVisible(); +});