viewit
5eeed97514
Found during a targeted code review, not from a user report: - _run_jpeg_loop()/_run_h264_loop() operated directly on the shared self._proc_jpeg/self._proc_h264 instance attributes in their cleanup, unlike _run_mjpeg_loop() (already fixed for exactly this) which uses a local `proc` reference. If a loop's task is cancelled - e.g. by CameraCache.reset() after the printer rotates its stream URL on reboot - while a new task has already started and assigned its own process to the shared attribute, the cancelled task's cleanup killed the NEWER process instead of its own, leaking its own ffmpeg child as an orphan. Applied the same local-variable + identity-check pattern already used by _run_mjpeg_loop. - handle_api_settings_post and handle_api_update_apply were the only two of ~84 handlers that called `await request.json()` without a try/except - every other handler follows the established pattern of returning a clean 400 for a malformed body. A trivial malformed request to either endpoint produced an unhandled 500 with a full traceback instead. New tests in tests/test_camera_process_race.py, tests/test_settings.py, and tests/test_update_check.py cover both.
84 lines
3.1 KiB
Python
84 lines
3.1 KiB
Python
"""Camera ffmpeg process-handle race (code review finding).
|
|
|
|
_run_jpeg_loop()/_run_h264_loop() used to operate on the shared instance
|
|
attribute (self._proc_jpeg / self._proc_h264) in their cleanup, instead of a
|
|
local reference to the process they themselves started - the same bug
|
|
_run_mjpeg_loop() already had fixed with a documented local-`proc` pattern.
|
|
|
|
If a loop's task is cancelled (e.g. via CameraCache.reset() after a stream
|
|
URL rotation) while a new task has already started and assigned its own
|
|
process to the shared attribute, the cancelled task's cleanup would kill
|
|
and null out the NEWER process instead of its own - leaking its own actual
|
|
ffmpeg child as an orphan that nothing ever cleans up again.
|
|
"""
|
|
import asyncio
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
from kobrax_moonraker_bridge import CameraCache
|
|
|
|
|
|
def _fake_proc(name):
|
|
"""A minimal stand-in for asyncio.subprocess.Process good enough to
|
|
drive _run_jpeg_loop()'s read/kill/wait/returncode usage."""
|
|
proc = MagicMock(name=name)
|
|
proc.returncode = 0
|
|
proc.kill = MagicMock()
|
|
proc.wait = AsyncMock()
|
|
proc.stdout = MagicMock()
|
|
proc.stderr = MagicMock()
|
|
proc.stderr.read = AsyncMock(return_value=b"")
|
|
return proc
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_jpeg_loop_cleanup_does_not_kill_a_newer_process():
|
|
cache = CameraCache()
|
|
cache._url = "http://printer/live/streamtoken"
|
|
|
|
old_proc = _fake_proc("old")
|
|
new_proc = _fake_proc("new")
|
|
|
|
# old_proc's stdout.read blocks forever until cancelled - simulating the
|
|
# loop being stuck reading from a stale connection, same as the real bug.
|
|
stuck = asyncio.Event()
|
|
|
|
async def old_stdout_read(_n):
|
|
await stuck.wait()
|
|
return b""
|
|
|
|
old_proc.stdout.read = old_stdout_read
|
|
|
|
create_calls = []
|
|
|
|
async def fake_create_subprocess_exec(*args, **kwargs):
|
|
create_calls.append(1)
|
|
return old_proc if len(create_calls) == 1 else new_proc
|
|
|
|
with patch("kobrax_moonraker_bridge.asyncio.create_subprocess_exec",
|
|
side_effect=fake_create_subprocess_exec), \
|
|
patch("kobrax_moonraker_bridge._find_ffmpeg", return_value="ffmpeg"):
|
|
task = asyncio.create_task(cache._run_jpeg_loop())
|
|
# Let the loop start and assign old_proc to the shared attribute.
|
|
await asyncio.sleep(0.05)
|
|
assert cache._proc_jpeg is old_proc
|
|
|
|
# Simulate a second loop iteration's process already having been
|
|
# assigned to the shared attribute before the cancelled task's
|
|
# cleanup runs - the exact race window from the bug report.
|
|
cache._proc_jpeg = new_proc
|
|
|
|
task.cancel()
|
|
try:
|
|
await asyncio.wait_for(task, timeout=2.0)
|
|
except (asyncio.CancelledError, asyncio.TimeoutError):
|
|
pass
|
|
|
|
# The cancelled task must have killed/waited on ITS OWN process (old_proc),
|
|
# not the newer one that had already taken over the shared attribute.
|
|
old_proc.kill.assert_called_once()
|
|
new_proc.kill.assert_not_called()
|
|
# And it must not have clobbered the newer process's slot.
|
|
assert cache._proc_jpeg is new_proc
|