diff --git a/CHANGELOG.md b/CHANGELOG.md index b53708d7..ede1f645 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,19 @@ All notable changes to the Fovea project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.5.8] - 2026-06-30 + +The 0.5.8 patch is the third and final audit-driven hardening release — the model-service slice — fixing resource and concurrency defects in the Python inference service ([#192](https://github.com/parafovea/fovea/pull/192)). Nothing is breaking. + +### Fixed + +- The `/detection/detect` and `/tracking/track` routes leaked an OpenCV `VideoCapture` handle (and decoder memory) whenever a frame read or mask decode failed, since the capture was released only on the success path. The capture is now released in a `try/finally` on every exit path, and an open failure raises a clean error (`model-service/.../routes/detection.py`, `tracking.py`). +- Synchronous VLM, transcription, and diarization inference ran on the asyncio event loop, so a single request froze all concurrent requests, the `/health` probe, and telemetry export until it finished. The blocking calls are now offloaded with `asyncio.to_thread` (`model-service/.../use_cases/summarize_video.py`, `routes/transcribe.py`, `routes/diarize.py`). +- ffprobe/ffmpeg subprocesses were awaited with no timeout, so a malformed or slow media file could wedge a request indefinitely and leak the child process. These calls now go through a shared helper that bounds the wait and kills and reaps the process on timeout (`model-service/.../services/audio_processing.py`). +- `ModelManager` had no concurrency guard, so two requests loading the same not-yet-loaded model could both pass the "already loaded?" check and double-load (risking OOM). Load, unload, and eviction are now serialized by a reentrant model lock with a post-acquire re-check (`model-service/.../services/model_management.py`). +- Id-based video resolution hardcoded `/videos`, returning 404 on any deployment whose video volume is mounted elsewhere; it now reads the configured `video_data_root` (`model-service/.../use_cases/summarize_video.py`). +- The admin reconfigure token is now compared with `hmac.compare_digest` (constant-time) instead of `!=` (`model-service/.../routes/admin.py`). + ## [0.5.7] - 2026-06-30 The 0.5.7 patch is the second of three audit-driven hardening releases — the frontend slice — fixing data-loss and stale-cache defects in the annotation UI ([#190](https://github.com/parafovea/fovea/pull/190)). Nothing is breaking. diff --git a/annotation-tool/package.json b/annotation-tool/package.json index 1cfe1d18..654e4cb2 100644 --- a/annotation-tool/package.json +++ b/annotation-tool/package.json @@ -1,7 +1,7 @@ { "name": "@fovea/annotation-tool", "private": true, - "version": "0.5.7", + "version": "0.5.8", "type": "module", "scripts": { "dev": "vite", diff --git a/docs/docs/project/changelog.md b/docs/docs/project/changelog.md index f2af0367..e681e002 100644 --- a/docs/docs/project/changelog.md +++ b/docs/docs/project/changelog.md @@ -11,6 +11,19 @@ All notable changes to the Fovea project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.5.8] - 2026-06-30 + +The 0.5.8 patch is the third and final audit-driven hardening release — the model-service slice — fixing resource and concurrency defects in the Python inference service ([#192](https://github.com/parafovea/fovea/pull/192)). Nothing is breaking. + +### Fixed + +- The `/detection/detect` and `/tracking/track` routes leaked an OpenCV `VideoCapture` handle (and decoder memory) whenever a frame read or mask decode failed, since the capture was released only on the success path. The capture is now released in a `try/finally` on every exit path, and an open failure raises a clean error (`model-service/.../routes/detection.py`, `tracking.py`). +- Synchronous VLM, transcription, and diarization inference ran on the asyncio event loop, so a single request froze all concurrent requests, the `/health` probe, and telemetry export until it finished. The blocking calls are now offloaded with `asyncio.to_thread` (`model-service/.../use_cases/summarize_video.py`, `routes/transcribe.py`, `routes/diarize.py`). +- ffprobe/ffmpeg subprocesses were awaited with no timeout, so a malformed or slow media file could wedge a request indefinitely and leak the child process. These calls now go through a shared helper that bounds the wait and kills and reaps the process on timeout (`model-service/.../services/audio_processing.py`). +- `ModelManager` had no concurrency guard, so two requests loading the same not-yet-loaded model could both pass the "already loaded?" check and double-load (risking OOM). Load, unload, and eviction are now serialized by a reentrant model lock with a post-acquire re-check (`model-service/.../services/model_management.py`). +- Id-based video resolution hardcoded `/videos`, returning 404 on any deployment whose video volume is mounted elsewhere; it now reads the configured `video_data_root` (`model-service/.../use_cases/summarize_video.py`). +- The admin reconfigure token is now compared with `hmac.compare_digest` (constant-time) instead of `!=` (`model-service/.../routes/admin.py`). + ## [0.5.7] - 2026-06-30 The 0.5.7 patch is the second of three audit-driven hardening releases — the frontend slice — fixing data-loss and stale-cache defects in the annotation UI ([#190](https://github.com/parafovea/fovea/pull/190)). Nothing is breaking. diff --git a/docs/package.json b/docs/package.json index a12ea546..5f3514f5 100644 --- a/docs/package.json +++ b/docs/package.json @@ -1,6 +1,6 @@ { "name": "docs", - "version": "0.5.7", + "version": "0.5.8", "private": true, "scripts": { "docusaurus": "docusaurus", diff --git a/model-service/package.json b/model-service/package.json index 937c5d2c..073f5fa3 100644 --- a/model-service/package.json +++ b/model-service/package.json @@ -1,6 +1,6 @@ { "name": "fovea-model-service", - "version": "0.5.7", + "version": "0.5.8", "description": "AI model inference service for video annotation", "scripts": { "dev": "source venv/bin/activate && uvicorn src.main:app --reload --host 0.0.0.0 --port 8000", diff --git a/model-service/pyproject.toml b/model-service/pyproject.toml index d1ba3649..971975ef 100644 --- a/model-service/pyproject.toml +++ b/model-service/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "fovea-model-service" -version = "0.5.7" +version = "0.5.8" description = "Model service for fovea video annotation tool" requires-python = ">=3.12" dependencies = [ diff --git a/model-service/src/application/services/audio_processing.py b/model-service/src/application/services/audio_processing.py index a43d5d58..1ed6c8d4 100644 --- a/model-service/src/application/services/audio_processing.py +++ b/model-service/src/application/services/audio_processing.py @@ -25,6 +25,51 @@ class AudioProcessingError(Exception): """Raised when audio processing operations fail.""" +#: Bounded wait for an ffprobe metadata probe. Probes read only a few +#: stream headers and should return in well under a second; the ceiling +#: keeps a wedged ffprobe from hanging a request forever. +_FFPROBE_TIMEOUT_SECONDS = 30 + +#: Bounded wait for an ffmpeg decode/transcode that streams full audio. +#: Larger than the probe ceiling because decoding a long file legitimately +#: takes longer. +_FFMPEG_DECODE_TIMEOUT_SECONDS = 300 + + +async def _communicate_with_timeout( + process: asyncio.subprocess.Process, timeout: float +) -> tuple[bytes, bytes]: + """Await ``process.communicate()`` with a bounded timeout. + + On timeout the process is killed and reaped before the error + propagates, so a wedged ffprobe/ffmpeg cannot leak a child process or + hang the request indefinitely. + + Parameters + ---------- + process : asyncio.subprocess.Process + The running subprocess. + timeout : float + Maximum seconds to wait for completion. + + Returns + ------- + tuple[bytes, bytes] + The process stdout and stderr. + + Raises + ------ + AudioProcessingError + If the process does not finish within ``timeout``. + """ + try: + return await asyncio.wait_for(process.communicate(), timeout=timeout) + except TimeoutError as exc: + process.kill() + await process.wait() + raise AudioProcessingError(f"Audio subprocess timed out after {timeout:g}s") from exc + + async def has_audio_stream(video_path: str) -> bool: """Check if video file contains audio streams. @@ -56,7 +101,7 @@ async def has_audio_stream(video_path: str) -> bool: stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, ) - stdout, _ = await process.communicate() + stdout, _ = await _communicate_with_timeout(process, _FFPROBE_TIMEOUT_SECONDS) has_audio = stdout.decode().strip() == "audio" span.set_attribute("audio.has_stream", has_audio) @@ -109,7 +154,7 @@ async def get_audio_info(video_path: str) -> dict[str, str | int | float]: stderr=asyncio.subprocess.PIPE, ) - stdout, stderr = await process.communicate() + stdout, stderr = await _communicate_with_timeout(process, _FFPROBE_TIMEOUT_SECONDS) if process.returncode != 0: error_msg = stderr.decode() if stderr else "Unknown error" @@ -277,7 +322,7 @@ async def chunk_audio_file( stderr=asyncio.subprocess.PIPE, ) - stdout, stderr = await process.communicate() + stdout, stderr = await _communicate_with_timeout(process, _FFPROBE_TIMEOUT_SECONDS) if process.returncode != 0: error_msg = stderr.decode() if stderr else "Unknown error" @@ -351,7 +396,9 @@ async def load_audio_array( stderr=asyncio.subprocess.PIPE, ) - stdout, stderr = await process.communicate() + stdout, stderr = await _communicate_with_timeout( + process, _FFMPEG_DECODE_TIMEOUT_SECONDS + ) if process.returncode != 0: error_msg = stderr.decode() if stderr else "Unknown error" @@ -403,7 +450,7 @@ async def get_audio_duration(audio_path: str) -> float: stderr=asyncio.subprocess.PIPE, ) - stdout, stderr = await process.communicate() + stdout, stderr = await _communicate_with_timeout(process, _FFPROBE_TIMEOUT_SECONDS) if process.returncode != 0: error_msg = stderr.decode() if stderr else "Unknown error" diff --git a/model-service/src/application/services/model_management.py b/model-service/src/application/services/model_management.py index acb6ac9d..3dcfccb3 100644 --- a/model-service/src/application/services/model_management.py +++ b/model-service/src/application/services/model_management.py @@ -12,10 +12,12 @@ from __future__ import annotations +import asyncio import logging import time from collections import OrderedDict -from collections.abc import Callable +from collections.abc import AsyncIterator, Callable +from contextlib import asynccontextmanager from pathlib import Path from typing import TYPE_CHECKING, Any @@ -166,6 +168,14 @@ def __init__( self.model_load_times: dict[str, float] = {} self.model_memory_usage: dict[str, int] = {} + # Serializes load/unload/evict so two concurrent loads of the same + # not-yet-loaded task cannot both pass the "already loaded?" check + # (the eviction await inside load_model is a yield point). The lock + # is reentrant within a single task via _mutate_models so that + # load_model can drive eviction/unload while holding it. + self._model_lock = asyncio.Lock() + self._lock_holder: asyncio.Task[Any] | None = None + self.device = self._detect_device() self.cpu_only_mode = self.device == "cpu" @@ -265,32 +275,56 @@ def get_lru_model(self) -> str | None: # --- Loading / unloading ------------------------------------------------------ + @asynccontextmanager + async def _mutate_models(self) -> AsyncIterator[None]: + """Hold the model lock for the duration of the block. + + Reentrant within a single asyncio task: ``load_model`` acquires the + lock and then calls ``evict_lru_model``/``unload_model``, which use + this same guard. The nested calls run in the same task that already + holds the lock, so they pass through without re-acquiring (an + ``asyncio.Lock`` is not reentrant and would otherwise deadlock). + """ + current = asyncio.current_task() + if self._lock_holder is current: + yield + return + + async with self._model_lock: + self._lock_holder = current + try: + yield + finally: + self._lock_holder = None + @tracer.start_as_current_span("evict_lru_model") async def evict_lru_model(self) -> str | None: """Evict the least recently used model from memory.""" - lru_task = self.get_lru_model() - if lru_task is None: - logger.warning("No models to evict") - return None + async with self._mutate_models(): + lru_task = self.get_lru_model() + if lru_task is None: + logger.warning("No models to evict") + return None - logger.info(f"Evicting LRU model: {lru_task}") - await self.unload_model(lru_task) - return lru_task + logger.info(f"Evicting LRU model: {lru_task}") + await self.unload_model(lru_task) + return lru_task @tracer.start_as_current_span("unload_model") async def unload_model(self, task_type: str) -> None: """Unload a model from memory.""" - if task_type not in self.loaded_models: - logger.warning(f"Model {task_type} not loaded") - return + async with self._mutate_models(): + if task_type not in self.loaded_models: + logger.warning(f"Model {task_type} not loaded") + return - logger.info(f"Unloading model: {task_type}") - del self.loaded_models[task_type] - del self.model_load_times[task_type] - del self.model_memory_usage[task_type] + logger.info(f"Unloading model: {task_type}") + del self.loaded_models[task_type] + del self.model_load_times[task_type] + del self.model_memory_usage[task_type] - self._probe().empty_cache() - logger.info(f"Model {task_type} unloaded successfully") + self._probe().empty_cache() + logger.info(f"Model {task_type} unloaded successfully") @tracer.start_as_current_span("load_model") async def load_model(self, task_type: str) -> Any: @@ -298,6 +332,16 @@ async def load_model(self, task_type: str) -> Any: if task_type not in self.tasks: raise ValueError(f"Invalid task type: {task_type}") + async with self._mutate_models(): + return await self._load_model_locked(task_type) + + async def _load_model_locked(self, task_type: str) -> Any: + """Load a model with the model lock already held. + + Re-checks ``loaded_models`` after the lock is acquired so a + coroutine that lost the race returns the instance loaded by the + winner instead of double-loading and risking OOM. + """ if task_type in self.loaded_models: self.loaded_models.move_to_end(task_type) logger.info(f"Model {task_type} already loaded, moved to end") diff --git a/model-service/src/application/use_cases/summarize_video.py b/model-service/src/application/use_cases/summarize_video.py index 38afa0fe..7d89eead 100644 --- a/model-service/src/application/use_cases/summarize_video.py +++ b/model-service/src/application/use_cases/summarize_video.py @@ -8,6 +8,7 @@ from __future__ import annotations +import asyncio import io import logging import os @@ -287,14 +288,18 @@ async def execute_with_vlm( ) = await self._maybe_transcribe(request, video_path, span) logger.info(f"Loading VLM model: {model_name}") - self._vlm.load() + # VLM load/generate/unload are blocking CPU/GPU calls; run + # them off the event loop so concurrent requests, /health, + # and OTLP export are not starved. + await asyncio.to_thread(self._vlm.load) try: prompt = get_persona_prompt(persona_role, information_need) logger.info(f"Generating summary with {len(images)} frames") visual_start_time = time.time() overrides = request.generation_overrides - reasoned = self._vlm.generate_reasoned_from_images( + reasoned = await asyncio.to_thread( + self._vlm.generate_reasoned_from_images, images, prompt, max_tokens=( @@ -361,7 +366,7 @@ async def execute_with_vlm( reasoning_trace=reasoned.thinking, ) finally: - self._vlm.unload() + await asyncio.to_thread(self._vlm.unload) logger.info("VLM model unloaded") except SummarizationError: @@ -700,21 +705,28 @@ def _safe(value: str) -> str: return str(value).replace("\r", "").replace("\n", "") -def get_video_path_for_id(video_id: str, data_dir: str = "/videos") -> str | None: +def get_video_path_for_id(video_id: str, data_dir: str | None = None) -> str | None: """Resolve video ID to file path. Parameters ---------- video_id : str Video identifier from request. - data_dir : str - Base directory containing video files. + data_dir : str | None + Base directory containing video files. When None, the configured + ``video_data_root`` setting is used so deployments whose videos do + not live at ``/videos`` resolve correctly. Returns ------- str | None Full path to video file, or None if not found. """ + if data_dir is None: + from src.infrastructure.config.settings import get_settings + + data_dir = str(get_settings().video_data_root) + # CodeQL sanitizer: ``re.fullmatch`` with a constant pattern restricts # ``video_id`` to a safe character set. The matched branch clears the # taint tracker's path-injection flag. diff --git a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/admin.py b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/admin.py index e5dff683..761f9378 100644 --- a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/admin.py +++ b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/admin.py @@ -19,6 +19,7 @@ from __future__ import annotations +import hmac import logging from typing import Literal @@ -113,7 +114,7 @@ def _require_admin_token(x_admin_token: str | None) -> None: status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Admin reconfigure is not enabled on this service", ) - if not x_admin_token or x_admin_token != expected: + if not x_admin_token or not hmac.compare_digest(x_admin_token or "", expected): raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid or missing admin token", diff --git a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/detection.py b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/detection.py index a811ea80..b5693c21 100644 --- a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/detection.py +++ b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/detection.py @@ -117,31 +117,39 @@ async def detect_objects( ) cap = cv2.VideoCapture(str(video_path)) - fps = cap.get(cv2.CAP_PROP_FPS) - total_frames = int(cap.get(cv2.CAP_PROP_FRAME_COUNT)) - - frame_numbers = list(request.frame_numbers) - if not frame_numbers: - frame_numbers = [0, total_frames // 2, max(total_frames - 1, 0)] - - frame_inputs: list[DetectObjectsFrameInput] = [] - for frame_num in frame_numbers: - if frame_num >= total_frames: - continue - cap.set(cv2.CAP_PROP_POS_FRAMES, frame_num) - ret, frame = cap.read() - if not ret: - continue - frame_rgb = cv2.cvtColor(frame, cv2.COLOR_BGR2RGB).astype(np.uint8) - timestamp = frame_num / fps if fps > 0 else 0.0 - frame_inputs.append( - DetectObjectsFrameInput( - frame_number=frame_num, - timestamp=timestamp, - image=frame_rgb, + try: + if not cap.isOpened(): + raise HTTPException( + status_code=400, + detail=f"Failed to open video for decoding: {request.video_id}", ) - ) - cap.release() + + fps = cap.get(cv2.CAP_PROP_FPS) + total_frames = int(cap.get(cv2.CAP_PROP_FRAME_COUNT)) + + frame_numbers = list(request.frame_numbers) + if not frame_numbers: + frame_numbers = [0, total_frames // 2, max(total_frames - 1, 0)] + + frame_inputs: list[DetectObjectsFrameInput] = [] + for frame_num in frame_numbers: + if frame_num >= total_frames: + continue + cap.set(cv2.CAP_PROP_POS_FRAMES, frame_num) + ret, frame = cap.read() + if not ret: + continue + frame_rgb = cv2.cvtColor(frame, cv2.COLOR_BGR2RGB).astype(np.uint8) + timestamp = frame_num / fps if fps > 0 else 0.0 + frame_inputs.append( + DetectObjectsFrameInput( + frame_number=frame_num, + timestamp=timestamp, + image=frame_rgb, + ) + ) + finally: + cap.release() dto_request = detection_request_schema_to_dto(request, video_path) execution_input = DetectObjectsExecutionInput(request=dto_request, frames=frame_inputs) diff --git a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/diarize.py b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/diarize.py index e7d79e20..153d2174 100644 --- a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/diarize.py +++ b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/diarize.py @@ -20,6 +20,7 @@ from __future__ import annotations +import asyncio import logging import os import time @@ -144,7 +145,9 @@ async def diarize( start = time.time() try: - result: DiarizationResult = model.diarize(audio_path_real) + # Diarization is a blocking CPU/GPU call; offload to a worker + # thread so it does not stall the event loop. + result: DiarizationResult = await asyncio.to_thread(model.diarize, audio_path_real) except Exception as exc: logger.exception("Diarization failed") raise HTTPException(status_code=500, detail=f"Diarization failed: {exc}") from exc diff --git a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/tracking.py b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/tracking.py index 23cca4e0..025edd27 100644 --- a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/tracking.py +++ b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/tracking.py @@ -127,43 +127,53 @@ async def track_objects( ) cap = cv2.VideoCapture(str(video_path)) - fps = cap.get(cv2.CAP_PROP_FPS) - total_frames = int(cap.get(cv2.CAP_PROP_FRAME_COUNT)) - width = int(cap.get(cv2.CAP_PROP_FRAME_WIDTH)) - height = int(cap.get(cv2.CAP_PROP_FRAME_HEIGHT)) - - frame_numbers = list(request.frame_numbers) - if not frame_numbers: - frame_numbers = list(range(total_frames)) - - # Decode initial masks - decoded_masks: list[np.ndarray[tuple[int, ...], np.dtype[np.bool_]]] = [] - for mask_b64 in request.initial_masks: - try: - mask_bytes = base64.b64decode(mask_b64) - mask_array = np.frombuffer(mask_bytes, dtype=np.uint8).reshape(height, width) - decoded_masks.append(mask_array.astype(np.bool_)) - except Exception as e: + try: + if not cap.isOpened(): raise HTTPException( - status_code=400, detail=f"Invalid mask encoding: {e!s}" - ) from e - - # Extract frames - frames_rgb: list[np.ndarray[tuple[int, ...], np.dtype[np.uint8]]] = [] - processed_frame_numbers: list[int] = [] - timestamps: list[float] = [] - for frame_num in frame_numbers: - if frame_num >= total_frames: - continue - cap.set(cv2.CAP_PROP_POS_FRAMES, frame_num) - ret, frame = cap.read() - if not ret: - continue - frame_rgb = cv2.cvtColor(frame, cv2.COLOR_BGR2RGB).astype(np.uint8) - frames_rgb.append(frame_rgb) - processed_frame_numbers.append(frame_num) - timestamps.append(frame_num / fps if fps > 0 else 0.0) - cap.release() + status_code=400, + detail=f"Failed to open video for decoding: {request.video_id}", + ) + + fps = cap.get(cv2.CAP_PROP_FPS) + total_frames = int(cap.get(cv2.CAP_PROP_FRAME_COUNT)) + width = int(cap.get(cv2.CAP_PROP_FRAME_WIDTH)) + height = int(cap.get(cv2.CAP_PROP_FRAME_HEIGHT)) + + frame_numbers = list(request.frame_numbers) + if not frame_numbers: + frame_numbers = list(range(total_frames)) + + # Decode initial masks + decoded_masks: list[np.ndarray[tuple[int, ...], np.dtype[np.bool_]]] = [] + for mask_b64 in request.initial_masks: + try: + mask_bytes = base64.b64decode(mask_b64) + mask_array = np.frombuffer(mask_bytes, dtype=np.uint8).reshape( + height, width + ) + decoded_masks.append(mask_array.astype(np.bool_)) + except Exception as e: + raise HTTPException( + status_code=400, detail=f"Invalid mask encoding: {e!s}" + ) from e + + # Extract frames + frames_rgb: list[np.ndarray[tuple[int, ...], np.dtype[np.uint8]]] = [] + processed_frame_numbers: list[int] = [] + timestamps: list[float] = [] + for frame_num in frame_numbers: + if frame_num >= total_frames: + continue + cap.set(cv2.CAP_PROP_POS_FRAMES, frame_num) + ret, frame = cap.read() + if not ret: + continue + frame_rgb = cv2.cvtColor(frame, cv2.COLOR_BGR2RGB).astype(np.uint8) + frames_rgb.append(frame_rgb) + processed_frame_numbers.append(frame_num) + timestamps.append(frame_num / fps if fps > 0 else 0.0) + finally: + cap.release() if not frames_rgb: raise HTTPException(status_code=400, detail="No valid frames to process") diff --git a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/transcribe.py b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/transcribe.py index 27bfaade..06d08a75 100644 --- a/model-service/src/infrastructure/adapters/inbound/fastapi/routes/transcribe.py +++ b/model-service/src/infrastructure/adapters/inbound/fastapi/routes/transcribe.py @@ -12,6 +12,7 @@ from __future__ import annotations +import asyncio import logging import os import time @@ -124,14 +125,13 @@ async def transcribe( start = time.time() try: - # Loaders are synchronous; running inside the FastAPI worker - # thread is fine for a 14 s demo clip but production CPU - # deployments will want to offload to a thread pool. Pass - # language=None when the caller did not specify one — some - # loaders (faster-whisper) treat an empty string as a hard - # lookup miss rather than "auto-detect". - result: TranscriptionResult = model.transcribe( - audio_path_real, language=request.language or None + # Loaders are synchronous and blocking; offload to a worker thread + # so the transcription does not stall the event loop (concurrent + # requests, /health, and OTLP export). Pass language=None when the + # caller did not specify one; some loaders (faster-whisper) treat + # an empty string as a hard lookup miss rather than "auto-detect". + result: TranscriptionResult = await asyncio.to_thread( + model.transcribe, audio_path_real, language=request.language or None ) except Exception as exc: logger.exception("Transcription failed") diff --git a/model-service/test/services/test_audio_processing.py b/model-service/test/services/test_audio_processing.py index 839cfe94..3b0d3a12 100644 --- a/model-service/test/services/test_audio_processing.py +++ b/model-service/test/services/test_audio_processing.py @@ -9,6 +9,7 @@ from __future__ import annotations +import asyncio import json import subprocess from typing import Any @@ -19,6 +20,7 @@ from src.application.services.audio_processing import ( AudioProcessingError, + _communicate_with_timeout, check_ffmpeg_available, check_ffprobe_available, chunk_audio_file, @@ -43,6 +45,35 @@ def _fake_process( return proc +@pytest.mark.asyncio +async def test_communicate_with_timeout_kills_hung_process(): + """A subprocess whose communicate() never returns must be killed and reaped, + and surface AudioProcessingError, rather than hanging the request forever.""" + proc = MagicMock() + + async def never_returns() -> tuple[bytes, bytes]: + await asyncio.sleep(10) + return (b"", b"") + + proc.communicate = never_returns + proc.kill = MagicMock() + proc.wait = AsyncMock() + + with pytest.raises(AudioProcessingError, match="timed out"): + await _communicate_with_timeout(proc, timeout=0.01) + + proc.kill.assert_called_once() + proc.wait.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_communicate_with_timeout_returns_output_when_fast(): + """When the subprocess finishes within the timeout, its output is returned.""" + proc = MagicMock() + proc.communicate = AsyncMock(return_value=(b"out", b"err")) + assert await _communicate_with_timeout(proc, timeout=5) == (b"out", b"err") + + @pytest.fixture def patch_subprocess() -> Any: """Patch ``asyncio.create_subprocess_exec`` with an AsyncMock. diff --git a/model-service/test/test_model_manager.py b/model-service/test/test_model_manager.py index 4285b0d0..069eac8b 100644 --- a/model-service/test/test_model_manager.py +++ b/model-service/test/test_model_manager.py @@ -5,6 +5,7 @@ and configuration validation. """ +import asyncio import tempfile from pathlib import Path from unittest.mock import patch @@ -413,6 +414,35 @@ async def test_load_model_invalid_task(self, model_manager): with pytest.raises(ValueError, match="Invalid task type"): await model_manager.load_model("invalid_task") + @pytest.mark.asyncio + async def test_concurrent_load_of_same_task_loads_once(self, model_manager): + """Two concurrent loads of the same not-yet-loaded task must serialize on + the model lock and re-check after acquiring, so the underlying model is + loaded exactly once and both callers receive the same instance. Guards the + load-model TOCTOU (without the lock + re-check, a second loader could + double-load and risk OOM).""" + load_calls = 0 + + def fake_impl(task_type, model_config): + nonlocal load_calls + load_calls += 1 + return {"model": f"instance-{load_calls}"} + + with ( + patch.object(model_manager, "_load_model_implementation", side_effect=fake_impl), + patch.object(model_manager, "check_memory_available", return_value=True), + patch("torch.cuda.is_available", return_value=True), + patch("torch.cuda.memory_allocated", return_value=0), + ): + first, second = await asyncio.gather( + model_manager.load_model("video_summarization"), + model_manager.load_model("video_summarization"), + ) + + assert load_calls == 1 + assert first is second + assert model_manager.loaded_models["video_summarization"] is first + @pytest.mark.asyncio async def test_load_model_with_eviction(self, model_manager): """Test loading model when memory needs to be freed.""" diff --git a/package.json b/package.json index 744614ab..5cf5b1d7 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "fovea", "private": true, - "version": "0.5.7", + "version": "0.5.8", "description": "FOVEA - Flexible Ontology Visual Event Analyzer", "packageManager": "pnpm@10.15.0", "engines": { diff --git a/server/package.json b/server/package.json index 93e8b71d..63355ba1 100644 --- a/server/package.json +++ b/server/package.json @@ -1,6 +1,6 @@ { "name": "@fovea/server", - "version": "0.5.7", + "version": "0.5.8", "private": true, "type": "module", "scripts": {