mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-20 17:26:33 +02:00
fix(camera): redact credentials, contain failures, and stop the external-camera test claiming a connection it never opened
Review follow-ups on the external-camera capture coalescing.
The coalescing was transplanted from camera.py, which is keyed by printer IP
and so has nothing to hide in a log line. These keys carry the camera URL, and
an RTSP camera URL routinely embeds user:pass@ - so the five new log lines
printed the password, one of them at warning level, where it reaches support
bundles. All five now go through _log_key(), which redacts before truncating:
slicing first can cut the URL short of the @ the pattern anchors on and leave
the password intact, which is why every other URL log in the module already
does it in that order.
_capture_frame_uncoalesced gained the blanket catch its camera.py counterpart
has. That is load-bearing once captures are shared: the wrapper hands one
task's outcome to every caller waiting on it and can only give a follower its
own turn for an outcome it recognises, so an escaping exception reached all of
them at once and none retried - one caller's failure becoming N. The per-type
helpers catch narrowly (aiohttp.ClientError / OSError / timeouts), so the
guarantee belongs here rather than resting on their coverage. CancelledError
is re-raised ahead of it, since the wrapper distinguishes a cancelled leader
from a failed one.
test_connection reports whether it shared a capture. It reaches capture_frame
like any other consumer, so a test landing while Obico is polling got that
frame back and answered "connected" for a connection it never made - the one
answer a connection test must not give silently. It still shares rather than
forcing its own capture, because forcing one would open the second handle to a
single-reader device that this whole mechanism exists to prevent. The response
carries `coalesced`, which also gives capture_in_flight() the consumer its
camera.py counterpart has in the Diagnose tool, and the Test button says
"shared with a capture already running" instead of a bare success.
Tests 12 -> 20: an unexpected error reported as a failed capture, a raising
leader whose follower still gets a frame, the three coalesced states, and
redaction on each log line that can carry a URL. The raising-leader test
patches _capture_rtsp_frame rather than _capture_frame_uncoalesced, since a
stand-in installed in the latter's place sits above the catch and would test
the wrapper against a shape it can no longer be handed.
This commit is contained in:
maziggy
@@ -224,7 +224,20 @@ def _discard_inflight_capture(key: tuple[str, str, str | None], task: asyncio.Ta
|
||||
if _inflight_captures.get(key) is task:
|
||||
del _inflight_captures[key]
|
||||
if not task.cancelled() and task.exception() is not None:
|
||||
logger.debug("In-flight external-camera capture for %s ended in an exception", key[0])
|
||||
logger.debug("In-flight external-camera capture for %s ended in an exception", _log_key(key))
|
||||
|
||||
|
||||
def _log_key(key: tuple[str, str, str | None]) -> str:
|
||||
"""Render an in-flight key for a log line, with credentials redacted.
|
||||
|
||||
Unlike camera.py's coalescing — which is keyed by IP address and so has
|
||||
nothing to hide — these keys carry the camera URL, and an RTSP camera URL
|
||||
routinely embeds ``user:pass@``. Redact before truncating: slicing first
|
||||
can cut the URL short of the ``@`` the pattern anchors on and leave the
|
||||
password in the log, which is why every other URL log in this module does
|
||||
it in this order.
|
||||
"""
|
||||
return redact_url_credentials(key[0])[:50] if key[0] else "None"
|
||||
|
||||
|
||||
async def capture_frame(
|
||||
@@ -277,23 +290,25 @@ async def capture_frame(
|
||||
except TimeoutError:
|
||||
# shield() keeps the capture running for whoever else is still
|
||||
# waiting on it - giving up is this caller's decision alone.
|
||||
logger.warning("Gave up waiting %ss on the in-flight external-camera capture for %s", timeout, key[0])
|
||||
logger.warning(
|
||||
"Gave up waiting %ss on the in-flight external-camera capture for %s", timeout, _log_key(key)
|
||||
)
|
||||
return None
|
||||
except asyncio.CancelledError:
|
||||
# Distinguish "the capture I joined was cancelled" from "I was
|
||||
# cancelled". Only the former is ours to recover from.
|
||||
if not leader.cancelled():
|
||||
raise
|
||||
logger.info("In-flight external-camera capture for %s was cancelled; capturing our own", key[0])
|
||||
logger.info("In-flight external-camera capture for %s was cancelled; capturing our own", _log_key(key))
|
||||
continue
|
||||
if frame is not None:
|
||||
logger.debug(
|
||||
"Reusing in-flight external-camera capture for %s: %d bytes (no second connection opened)",
|
||||
key[0],
|
||||
_log_key(key),
|
||||
len(frame),
|
||||
)
|
||||
return frame
|
||||
logger.debug("In-flight external-camera capture for %s failed; capturing our own", key[0])
|
||||
logger.debug("In-flight external-camera capture for %s failed; capturing our own", _log_key(key))
|
||||
else:
|
||||
return None
|
||||
|
||||
@@ -320,27 +335,48 @@ async def _capture_frame_uncoalesced(
|
||||
|
||||
Callers want that wrapper, not this: it opens a connection
|
||||
unconditionally, which is the collision #2705/#2707 are about.
|
||||
|
||||
Failure is reported as ``None``, never as an exception. That is load-
|
||||
bearing now that captures are shared: the coalescing wrapper hands one
|
||||
task's outcome to every caller waiting on it, and it can only give a
|
||||
follower its own turn for an outcome it can recognise. An exception
|
||||
escaping here would instead propagate to every follower at once —
|
||||
turning one caller's failure into N — and none of them would retry.
|
||||
The per-type helpers below each catch what they expect and return None,
|
||||
but they catch narrowly (``aiohttp.ClientError``/``OSError``/timeouts),
|
||||
so this is the structural guarantee rather than one contingent on their
|
||||
coverage. Mirrors ``_capture_camera_frame_bytes_uncoalesced`` in
|
||||
camera.py, which ends in the same blanket catch for the same reason.
|
||||
"""
|
||||
if snapshot_url:
|
||||
# Redact before truncating — slicing first can cut the URL short of the
|
||||
# ``@`` the pattern anchors on and leave the password in the log.
|
||||
logger.debug("capture_frame using snapshot override url=%s...", redact_url_credentials(snapshot_url)[:50])
|
||||
return await _capture_snapshot(snapshot_url, timeout)
|
||||
logger.debug(
|
||||
"capture_frame called: type=%s, url=%s...",
|
||||
camera_type,
|
||||
redact_url_credentials(url)[:50] if url else "None",
|
||||
)
|
||||
if camera_type == "mjpeg":
|
||||
return await _capture_mjpeg_frame(url, timeout)
|
||||
elif camera_type == "rtsp":
|
||||
return await _capture_rtsp_frame(url, timeout)
|
||||
elif camera_type == "snapshot":
|
||||
return await _capture_snapshot(url, timeout)
|
||||
elif camera_type == "usb":
|
||||
return await _capture_usb_frame(url, timeout)
|
||||
else:
|
||||
logger.warning("Unknown camera type: %s", camera_type)
|
||||
try:
|
||||
if snapshot_url:
|
||||
# Redact before truncating — slicing first can cut the URL short of the
|
||||
# ``@`` the pattern anchors on and leave the password in the log.
|
||||
logger.debug("capture_frame using snapshot override url=%s...", redact_url_credentials(snapshot_url)[:50])
|
||||
return await _capture_snapshot(snapshot_url, timeout)
|
||||
logger.debug(
|
||||
"capture_frame called: type=%s, url=%s...",
|
||||
camera_type,
|
||||
redact_url_credentials(url)[:50] if url else "None",
|
||||
)
|
||||
if camera_type == "mjpeg":
|
||||
return await _capture_mjpeg_frame(url, timeout)
|
||||
elif camera_type == "rtsp":
|
||||
return await _capture_rtsp_frame(url, timeout)
|
||||
elif camera_type == "snapshot":
|
||||
return await _capture_snapshot(url, timeout)
|
||||
elif camera_type == "usb":
|
||||
return await _capture_usb_frame(url, timeout)
|
||||
else:
|
||||
logger.warning("Unknown camera type: %s", camera_type)
|
||||
return None
|
||||
except asyncio.CancelledError:
|
||||
# Cancellation is not a capture failure and must stay distinguishable:
|
||||
# the wrapper checks ``leader.cancelled()`` to decide whether a
|
||||
# follower may take its own turn.
|
||||
raise
|
||||
except Exception:
|
||||
logger.exception("External camera capture failed for %s", redact_url_credentials(url)[:50] if url else "None")
|
||||
return None
|
||||
|
||||
|
||||
@@ -691,12 +727,26 @@ async def test_connection(url: str, camera_type: str) -> dict:
|
||||
"""Test camera connection.
|
||||
|
||||
Returns:
|
||||
Dict with {success: bool, error?: str, resolution?: str}
|
||||
Dict with {success: bool, error?: str, resolution?: str, coalesced: bool}
|
||||
|
||||
``coalesced`` is True when the frame came from a capture that was already
|
||||
running rather than from a connection this test opened. Captures are shared
|
||||
(see ``capture_frame``), so a test that lands while Obico is polling — or
|
||||
while any other one-shot consumer is mid-capture — gets that frame back and
|
||||
would otherwise report a healthy connection it never made, which is the one
|
||||
answer a *connection test* must not give silently. Forcing an uncoalesced
|
||||
capture here would be worse: it would open the second handle to a
|
||||
single-reader device that this whole mechanism exists to prevent. So the
|
||||
test still shares, and says so. Mirrors the ``coalesced_capture`` code the
|
||||
built-in diagnostic reports for the same situation (camera_diagnose.py).
|
||||
"""
|
||||
logger.info("Testing camera connection: type=%s, url=%s...", camera_type, redact_url_credentials(url)[:50])
|
||||
# Sampled before the call, while it can still distinguish "someone else is
|
||||
# mid-capture" from "I am the one capturing".
|
||||
coalesced = capture_in_flight(url, camera_type)
|
||||
try:
|
||||
frame = await capture_frame(url, camera_type, timeout=10)
|
||||
logger.info("Capture result: %s bytes", len(frame) if frame else 0)
|
||||
logger.info("Capture result: %s bytes%s", len(frame) if frame else 0, " (coalesced)" if coalesced else "")
|
||||
|
||||
if frame:
|
||||
# Try to get resolution from JPEG header
|
||||
@@ -715,15 +765,15 @@ async def test_connection(url: str, camera_type: str) -> dict:
|
||||
except (IndexError, ValueError):
|
||||
pass # Resolution detection is optional; fall back to default
|
||||
|
||||
return {"success": True, "resolution": resolution}
|
||||
return {"success": True, "resolution": resolution, "coalesced": coalesced}
|
||||
else:
|
||||
return {"success": False, "error": "Failed to capture frame from camera"}
|
||||
return {"success": False, "error": "Failed to capture frame from camera", "coalesced": coalesced}
|
||||
|
||||
except Exception as e:
|
||||
# Sanitize error message - don't expose internal details
|
||||
error_type = type(e).__name__
|
||||
logger.error("Camera connection test failed: %s", e)
|
||||
return {"success": False, "error": f"Connection failed: {error_type}"}
|
||||
return {"success": False, "error": f"Connection failed: {error_type}", "coalesced": coalesced}
|
||||
|
||||
|
||||
async def generate_mjpeg_stream(
|
||||
|
||||
@@ -306,3 +306,188 @@ async def test_capture_in_flight_reports_the_window(patch_capture):
|
||||
await asyncio.sleep(0)
|
||||
|
||||
assert capture_in_flight("/dev/video1", "usb") is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Failure must arrive as None, never as an exception
|
||||
# ---------------------------------------------------------------------------
|
||||
#
|
||||
# `test_failed_leader_does_not_poison_its_followers` above covers a leader that
|
||||
# RETURNS None. A leader that RAISES is a different path: the wrapper's retry
|
||||
# loop only catches TimeoutError and CancelledError, so an escaping exception
|
||||
# would reach every follower at once and none of them would take a turn of
|
||||
# their own — one caller's failure becoming N. The per-type helpers catch
|
||||
# narrowly (aiohttp.ClientError / OSError / timeouts), so the guarantee lives
|
||||
# in _capture_frame_uncoalesced's own blanket catch.
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_unexpected_error_is_reported_as_a_failed_capture():
|
||||
"""Not every failure is an OSError. An IncompleteReadError is an EOFError,
|
||||
which none of the per-type helpers catch."""
|
||||
|
||||
async def raising(url, timeout):
|
||||
raise asyncio.IncompleteReadError(partial=b"", expected=4)
|
||||
|
||||
import backend.app.services.external_camera as ec
|
||||
|
||||
original = ec._capture_snapshot
|
||||
ec._capture_snapshot = raising
|
||||
try:
|
||||
result = await ec._capture_frame_uncoalesced("http://cam/snap", "snapshot", 5, None)
|
||||
finally:
|
||||
ec._capture_snapshot = original
|
||||
assert result is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_raising_leader_does_not_take_its_followers_down_with_it(monkeypatch):
|
||||
"""The whole point of coalescing is that one caller's connection serves
|
||||
several. It must not also mean one caller's crash fails several.
|
||||
|
||||
Patches the per-type helper rather than ``_capture_frame_uncoalesced``,
|
||||
deliberately: the guarantee lives in that function's blanket catch, so a
|
||||
stand-in installed in its place would test the wrapper against a shape the
|
||||
wrapper can no longer be handed.
|
||||
"""
|
||||
gate = asyncio.Event()
|
||||
attempts: list[str] = []
|
||||
|
||||
async def raise_then_succeed(url, timeout):
|
||||
attempts.append(url)
|
||||
if len(attempts) == 1:
|
||||
await gate.wait()
|
||||
raise RuntimeError("ffmpeg died in a way nobody catches")
|
||||
return FRAME_B
|
||||
|
||||
monkeypatch.setattr(ec_module, "_capture_rtsp_frame", raise_then_succeed)
|
||||
|
||||
leader = asyncio.create_task(capture_frame("rtsp://cam/1", "rtsp", timeout=5))
|
||||
await asyncio.sleep(0)
|
||||
follower = asyncio.create_task(capture_frame("rtsp://cam/1", "rtsp", timeout=5))
|
||||
await asyncio.sleep(0)
|
||||
gate.set()
|
||||
|
||||
leader_result, follower_result = await asyncio.gather(leader, follower, return_exceptions=True)
|
||||
|
||||
assert not isinstance(leader_result, BaseException), f"leader raised {leader_result!r}"
|
||||
assert not isinstance(follower_result, BaseException), f"follower raised {follower_result!r}"
|
||||
assert leader_result is None, "the leader's own capture failed, so it gets None"
|
||||
assert follower_result == FRAME_B, "the follower took its own turn and succeeded"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The connection test must not claim a connection it never opened
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_connection_reports_when_it_shared_someone_elses_capture(patch_capture):
|
||||
"""A test landing while Obico is mid-poll gets that frame back. Reporting a
|
||||
bare success would credit a connection this test never made — and forcing
|
||||
its own would open the second handle the coalescing exists to prevent."""
|
||||
from backend.app.services.external_camera import test_connection
|
||||
|
||||
gate = asyncio.Event()
|
||||
capture = patch_capture(RecordingCapture(frames=(FRAME_A,), gate=gate))
|
||||
|
||||
other = asyncio.create_task(capture_frame("rtsp://cam/1", "rtsp", timeout=5))
|
||||
await _let_leader_start(capture)
|
||||
|
||||
tested = asyncio.create_task(test_connection("rtsp://cam/1", "rtsp"))
|
||||
await asyncio.sleep(0)
|
||||
gate.set()
|
||||
|
||||
result = await tested
|
||||
await other
|
||||
|
||||
assert result["success"] is True
|
||||
assert result["coalesced"] is True
|
||||
assert capture.count == 1, "no second connection was opened"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_connection_reports_its_own_capture_as_not_coalesced(patch_capture):
|
||||
from backend.app.services.external_camera import test_connection
|
||||
|
||||
capture = patch_capture(RecordingCapture(frames=(FRAME_A,)))
|
||||
result = await test_connection("rtsp://cam/1", "rtsp")
|
||||
|
||||
assert result["success"] is True
|
||||
assert result["coalesced"] is False
|
||||
assert capture.count == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_connection_reports_coalesced_on_the_failure_path_too(patch_capture):
|
||||
"""The flag describes where the answer came from, not whether it was good."""
|
||||
from backend.app.services.external_camera import test_connection
|
||||
|
||||
capture = patch_capture(RecordingCapture(frames=(None,)))
|
||||
result = await test_connection("rtsp://cam/1", "rtsp")
|
||||
|
||||
assert result["success"] is False
|
||||
assert result["coalesced"] is False
|
||||
assert capture.count == 1
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Credentials must not reach the log
|
||||
# ---------------------------------------------------------------------------
|
||||
#
|
||||
# camera.py's coalescing is keyed by IP address and has nothing to redact.
|
||||
# These keys carry the camera URL, and an RTSP camera URL routinely embeds
|
||||
# user:pass@ — which is why every other URL log in the module redacts.
|
||||
|
||||
CREDENTIALED_URL = "rtsp://admin:hunter2@192.168.1.50:554/Streaming/Channels/101"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_reuse_log_line_redacts_the_password(patch_capture, caplog):
|
||||
gate = asyncio.Event()
|
||||
capture = patch_capture(RecordingCapture(frames=(FRAME_A,), gate=gate))
|
||||
|
||||
with caplog.at_level("DEBUG", logger=ec_module.__name__):
|
||||
leader = asyncio.create_task(capture_frame(CREDENTIALED_URL, "rtsp", timeout=5))
|
||||
await _let_leader_start(capture)
|
||||
follower = asyncio.create_task(capture_frame(CREDENTIALED_URL, "rtsp", timeout=5))
|
||||
await asyncio.sleep(0)
|
||||
gate.set()
|
||||
await asyncio.gather(leader, follower)
|
||||
|
||||
assert not [r.getMessage() for r in caplog.records if "hunter2" in r.getMessage()]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_gave_up_waiting_log_line_redacts_the_password(patch_capture, caplog):
|
||||
"""This one is a warning, so it shows at the default level and lands in
|
||||
support bundles."""
|
||||
gate = asyncio.Event()
|
||||
capture = patch_capture(RecordingCapture(frames=(FRAME_A,), gate=gate))
|
||||
|
||||
with caplog.at_level("DEBUG", logger=ec_module.__name__):
|
||||
leader = asyncio.create_task(capture_frame(CREDENTIALED_URL, "rtsp", timeout=5))
|
||||
await _let_leader_start(capture)
|
||||
assert await capture_frame(CREDENTIALED_URL, "rtsp", timeout=0) is None
|
||||
gate.set()
|
||||
await leader
|
||||
|
||||
messages = [r.getMessage() for r in caplog.records]
|
||||
assert any("Gave up waiting" in m for m in messages), "the timeout path did not run"
|
||||
assert not [m for m in messages if "hunter2" in m]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_failed_capture_log_line_redacts_the_password(patch_capture, caplog):
|
||||
gate = asyncio.Event()
|
||||
capture = patch_capture(RecordingCapture(frames=(None, FRAME_B), gate=gate))
|
||||
|
||||
with caplog.at_level("DEBUG", logger=ec_module.__name__):
|
||||
leader = asyncio.create_task(capture_frame(CREDENTIALED_URL, "rtsp", timeout=5))
|
||||
await _let_leader_start(capture)
|
||||
follower = asyncio.create_task(capture_frame(CREDENTIALED_URL, "rtsp", timeout=5))
|
||||
await asyncio.sleep(0)
|
||||
gate.set()
|
||||
await asyncio.gather(leader, follower)
|
||||
|
||||
assert not [r.getMessage() for r in caplog.records if "hunter2" in r.getMessage()]
|
||||
|
||||
Reference in New Issue
Block a user