mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-09-30 03:01:21 +02:00
fix(camera): release DB connection before streaming, not after (#2572)
/camera/stream took its printer row via Depends(get_db). get_db is a yield dependency, so its session stayed open until the response body finished streaming — for a live MJPEG stream, as long as the browser tab is open (hours). Every open camera tile pinned one pooled DB connection idle-in-transaction, draining the pool on large farms. Fetch the printer in a short-lived async_session() and release the connection before returning the StreamingResponse. expire_on_commit= False keeps the already-loaded columns readable during the stream.
This commit is contained in:
@@ -5,6 +5,7 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
## [1.2.5b2] - Unreleased
|
||||
|
||||
### Fixed
|
||||
- **Live camera stream held a database connection open for its entire duration (#2572, reporter @Jostxxl)** — The `/camera/stream` MJPEG endpoint took its printer row via `Depends(get_db)`, but `get_db` is a `yield` dependency: its session isn't released until the response body finishes streaming, which for a live stream is however long the browser tab stays open — minutes to hours. On a large farm every open camera tile therefore pinned one pooled DB connection `idle in transaction`, so a wall of dashboards could drain the pool on its own (a top contributor to the exhaustion in #2572). The endpoint now fetches the printer in a short-lived session and releases the connection *before* it starts streaming (`expire_on_commit=False` keeps the already-loaded columns readable). Pinned by a regression test that fails if a `get_db`-held session is ever re-added to the route. Part of the broader effort to stop holding sessions across slow MQTT/FTP/camera/3MF work.
|
||||
- **PostgreSQL connection-pool exhaustion on large printer farms (#2572, reporter @Jostxxl)** — On a ~93-printer farm the SQLAlchemy pool (hard-coded `pool_size=10` + `max_overflow=20` = 30 connections) was repeatedly saturated with all connections `idle in transaction`; unrelated API requests then waited out the 30-second pool timeout or failed in the auth middleware, and an unauthenticated `/api/v1/printers` probe took ~25s to return 401. Three things fed the pressure: the pool was fixed and not configurable; every authenticated request re-queried `auth_enabled` from the DB (the middleware alone opened a session per request just to probe it); and the pool was small for a farm. This change (a) makes pool sizing configurable via `DB_POOL_SIZE` / `DB_MAX_OVERFLOW` / `DB_POOL_TIMEOUT` / `DB_POOL_RECYCLE` env vars and raises the PostgreSQL default to `20` + `80` (100 total) with `pool_pre_ping` and a 1800s `pool_recycle`; (b) caches the `auth_enabled` probe for 30s — only the *enabled* result is ever cached, so a stale read can only ever fail closed (require auth), never open, and any toggle invalidates it immediately; and (c) adds a `GET /api/v1/system/db-pool` diagnostic exposing the resolved config plus live `checked_out` / `checked_in` / `overflow` gauges (read without checking out a connection, so it stays truthful under saturation). Note: connections being held across slow MQTT/FTP/camera/3MF work — the underlying reason transactions sit idle — is a deeper session-hygiene change tracked separately; this drop relieves and instruments the problem and makes the farm sizing configurable. See the PostgreSQL wiki page for large-farm tuning and the required `max_connections` headroom.
|
||||
- **P1S camera still black on every page load, recovering only after ~20 minutes (#2521, reporter @nnimby848)** — The previous round of fixes did not take, and the reporter re-tested on two daily builds to say so. The fan-out barrier added last time — a replacement stream waits for the displaced one's socket to close before dialling, so a printer that allows a single camera connection never sees two at once — was correct, and was being **bypassed**. `shutdown_broadcaster()` *popped* the broadcaster out of the registry and only then awaited its teardown, so for the duration of the socket close the registry slot sat empty. A `/camera/stream` request landing in that window found nothing, minted a broadcaster with no predecessor to wait for, and dialled port 6000 immediately. The barrier only engages when the displaced broadcaster is still findable — and the one path that tears a stream down on purpose removed it first, disabling the barrier in exactly the case it was written for. A page reload fires `/camera/stop` and the new `/camera/stream` **concurrently**, which is why it reproduced on essentially every load. The printer then held two connections, kept feeding the orphan, and starved the live viewer: the new socket connects (the reporter's logs show `Chamber image: connected`) and then receives nothing until the printer's TCP keepalive reaps the dead one — **his 20 minutes, to the minute**. The stopped broadcaster now stays in the registry so the next viewer chains behind its socket close, which is what the barrier always intended. Pinned by a test that counts *actual* sockets through the real stop-then-restream race and fails with `2` against the old code; the existing barrier tests placed the broadcaster into the registry by hand, which is precisely why they never caught this.
|
||||
- **Every camera page load attached two viewers and abandoned one (#2521)** — Found while reproducing the above, and the reason it fired on *every* load rather than occasionally. The stream-token query runs whether or not authentication is enabled, and the camera page subscribes to it: the first render produced an `<img src>` with no token, the token arrived, and the re-render **changed the src**. The browser aborts the in-flight request and issues a second one — and with auth disabled no token is required, so *both* reached the backend and attached to the fan-out. The reporter's HAR shows it exactly: two requests to the same stream URL, same cache-buster, one without `token=` and one with. His backend log shows the consequence, `subscribers=2`, on a printer that allows one connection. The src is now rendered only once the token query has settled — one URL, one request, one viewer — and an auth-disabled install whose token endpoint fails still streams, because it never needed a token.
|
||||
|
||||
@@ -12,6 +12,7 @@ from fastapi.responses import Response, StreamingResponse
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
from backend.app.core import database
|
||||
from backend.app.core.auth import (
|
||||
RequireCameraStreamTokenIfAuthEnabled,
|
||||
RequirePermissionIfAuthEnabled,
|
||||
@@ -609,7 +610,6 @@ async def camera_stream(
|
||||
printer_id: int,
|
||||
request: Request,
|
||||
fps: int = 10,
|
||||
db: AsyncSession = Depends(get_db),
|
||||
_: None = RequireCameraStreamTokenIfAuthEnabled,
|
||||
):
|
||||
"""Stream live video from printer camera as MJPEG.
|
||||
@@ -628,7 +628,22 @@ async def camera_stream(
|
||||
printer_id: Printer ID
|
||||
fps: Target frames per second (default: 10, max: 30)
|
||||
"""
|
||||
printer = await get_printer_or_404(printer_id, db)
|
||||
# Fetch the printer in a short-lived session so the pooled DB connection is
|
||||
# released BEFORE we start streaming. A live MJPEG stream runs for as long
|
||||
# as the browser tab stays open (potentially hours); holding the
|
||||
# Depends(get_db) session across it pinned one pooled connection per open
|
||||
# camera tab per printer — a top contributor to pool exhaustion on large
|
||||
# farms (issue #2572). expire_on_commit=False keeps the printer's already-
|
||||
# loaded columns readable after the session closes, and everything below
|
||||
# reads only scalar attributes (model, ip_address, access_code,
|
||||
# external_camera_*) — no lazy loads.
|
||||
#
|
||||
# Reference async_session via the module (not a top-level import binding) so
|
||||
# the session maker is looked up at call time — that keeps it in sync with
|
||||
# reinitialize_database() and lets the test harness's patch of
|
||||
# backend.app.core.database.async_session take effect here.
|
||||
async with database.async_session() as db:
|
||||
printer = await get_printer_or_404(printer_id, db)
|
||||
|
||||
# Check for external camera first
|
||||
if printer.external_camera_enabled and printer.external_camera_url:
|
||||
|
||||
@@ -811,3 +811,29 @@ class TestCameraAPI:
|
||||
assert response.status_code == 200
|
||||
result = response.json()
|
||||
assert result["cameras"] == []
|
||||
|
||||
|
||||
class TestCameraStreamPoolHygiene:
|
||||
"""Regression guard for the camera-stream DB-connection leak (issue #2572)."""
|
||||
|
||||
def test_camera_stream_does_not_hold_a_get_db_session(self):
|
||||
"""The MJPEG stream endpoint must NOT take a ``Depends(get_db)`` session.
|
||||
|
||||
``get_db`` is a ``yield`` dependency, so its session stays open until the
|
||||
response body is fully consumed — for a live MJPEG stream that is the
|
||||
whole time the browser tab is open (hours), pinning one pooled DB
|
||||
connection per open camera tab per printer. The endpoint fetches the
|
||||
printer in a short-lived ``async with async_session()`` instead and
|
||||
releases the connection before streaming. If someone re-adds a
|
||||
``Depends(get_db)`` param, this fails.
|
||||
"""
|
||||
import inspect
|
||||
|
||||
from backend.app.api.routes.camera import camera_stream, get_db
|
||||
|
||||
for name, param in inspect.signature(camera_stream).parameters.items():
|
||||
dependency = getattr(param.default, "dependency", None)
|
||||
assert dependency is not get_db, (
|
||||
f"camera_stream re-introduced a get_db-held session via parameter {name!r} — "
|
||||
"it would stay open for the entire stream (issue #2572)"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user