From b3c042937322e9ff17a2146e410dd50669aa4d42 Mon Sep 17 00:00:00 2001 From: maziggy Date: Thu, 16 Jul 2026 08:38:42 +0200 Subject: [PATCH] fix(camera): release DB connection before streaming, not after (#2572) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /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. --- CHANGELOG.md | 1 + backend/app/api/routes/camera.py | 19 ++++++++++++-- backend/tests/integration/test_camera_api.py | 26 ++++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 218597a54..4b0ce5bbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 `` 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. diff --git a/backend/app/api/routes/camera.py b/backend/app/api/routes/camera.py index 0914992b2..b07874bcd 100644 --- a/backend/app/api/routes/camera.py +++ b/backend/app/api/routes/camera.py @@ -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: diff --git a/backend/tests/integration/test_camera_api.py b/backend/tests/integration/test_camera_api.py index 2b3b2996c..b511ab40c 100644 --- a/backend/tests/integration/test_camera_api.py +++ b/backend/tests/integration/test_camera_api.py @@ -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)" + )