mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-22 03:49:18 +02:00
fix(cover): release the DB connection before the FTP thumbnail download (#2572)
GET /printers/{id}/cover took its printer row via Depends(get_db), whose
yield-dependency session stays open for the whole request — including the
3MF cover download (up to 8 remote paths x retries with backoff, minutes
under FTP contention). One pooled connection sat idle-in-transaction the
entire time; on a large farm a wall of dashboards drained the pool. The
route now fetches the printer in a short-lived async_session() and releases
the connection before the download (expire_on_commit=False keeps printer.*
readable). Pinned by a signature-inspection guard that fails if get_db is
ever re-added.
fix(print-start): release the DB connection across plate detection and 3MF download (#2572)
on_print_start held one session from top to bottom of the handler, across
two slow I/O blocks that need no database: the plate-detection camera grab
and, on the new-archive path, the multi-path 3MF FTP download (its own
comments cite worst cases of tens of minutes). The connection sat idle-in-
transaction for both, once per starting print. It now commits at each
boundary — only read SELECTs have run on those paths (every write branch
returns earlier), so the commit persists nothing and simply returns the
connection to the pool for the I/O; the next query re-acquires, and
expire_on_commit=False keeps printer.* readable.
fix(startup): connect to printers concurrently so the API serves within seconds (#2572)
init_printer_connections awaited each printer's connection serially, and
connect_printer ends in a fixed 1s settle wait. The MQTT connect is non-
blocking (connect_async + loop_start), so that 1s x fleet size was pure
serial dead air the FastAPI lifespan blocked on before uvicorn began
serving — ~100s before port 8000 responded on a 93-printer farm. The
connections are now started with asyncio.gather, so the step takes ~1s
regardless of fleet size. return_exceptions=True isolates each result: one
unreachable printer no longer aborts the rest, or startup itself.
This commit is contained in:
@@ -571,6 +571,32 @@ class TestPrintersAPI:
|
||||
# ========================================================================
|
||||
|
||||
|
||||
class TestCoverPoolHygiene:
|
||||
"""Regression guard for the /cover DB-connection leak (issue #2572)."""
|
||||
|
||||
def test_cover_does_not_hold_a_get_db_session(self):
|
||||
"""The cover endpoint must NOT take a ``Depends(get_db)`` session.
|
||||
|
||||
``get_db`` is a ``yield`` dependency, so its session stays open for the
|
||||
whole request — including the 3MF cover download (up to 8 remote paths ×
|
||||
retries with backoff, minutes under FTP contention), pinning one pooled
|
||||
DB connection ``idle in transaction`` the entire time. The endpoint
|
||||
fetches the printer in a short-lived ``async with async_session()`` and
|
||||
releases the connection before the FTP work. If someone re-adds a
|
||||
``Depends(get_db)`` param, this fails.
|
||||
"""
|
||||
import inspect
|
||||
|
||||
from backend.app.api.routes.printers import get_db, get_printer_cover
|
||||
|
||||
for name, param in inspect.signature(get_printer_cover).parameters.items():
|
||||
dependency = getattr(param.default, "dependency", None)
|
||||
assert dependency is not get_db, (
|
||||
f"get_printer_cover re-introduced a get_db-held session via parameter {name!r} — "
|
||||
"it would stay open for the entire FTP cover download (issue #2572)"
|
||||
)
|
||||
|
||||
|
||||
class TestPrinterDataIntegrity:
|
||||
"""Tests for printer data integrity."""
|
||||
|
||||
|
||||
@@ -1804,6 +1804,37 @@ class TestInitPrinterConnections:
|
||||
|
||||
mock_manager.connect_printer.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_one_failing_printer_does_not_abort_the_rest(self):
|
||||
"""A single unreachable printer must not abort startup or the others (#2572).
|
||||
|
||||
The connections are gathered with return_exceptions=True. The old serial
|
||||
``await`` loop had no error handling, so the first printer that raised
|
||||
propagated straight out of init_printer_connections and failed the
|
||||
FastAPI lifespan — taking the whole app down over one bad row, and
|
||||
skipping every printer after it. This asserts the isolation: every
|
||||
printer is still attempted and the exception never escapes.
|
||||
"""
|
||||
mock_db = AsyncMock()
|
||||
printers = [MagicMock(id=i, name=f"p{i}", is_active=True) for i in range(3)]
|
||||
mock_result = MagicMock()
|
||||
mock_result.scalars.return_value.all.return_value = printers
|
||||
mock_db.execute.return_value = mock_result
|
||||
|
||||
async def connect(printer):
|
||||
if printer.id == 1:
|
||||
raise ConnectionError("printer 1 is unreachable")
|
||||
return True
|
||||
|
||||
with patch("backend.app.services.printer_manager.printer_manager") as mock_manager:
|
||||
mock_manager.connect_printer = AsyncMock(side_effect=connect)
|
||||
|
||||
# Must not raise despite printer 1 failing.
|
||||
await init_printer_connections(mock_db)
|
||||
|
||||
# All three were still attempted (not aborted at the failing one).
|
||||
assert mock_manager.connect_printer.call_count == 3
|
||||
|
||||
|
||||
class TestAmsChangeCallback:
|
||||
"""Tests for AMS change callback functionality."""
|
||||
|
||||
Reference in New Issue
Block a user