fix: a Postgres connection was held across every celery round trip (4295)
Operator, 2026-09-23: *"something about changing the cap number is blocking to the website... it shouldn't be"*. Nothing here was slow in itself. A database connection was held across work that is slow, and that is why it surfaced as the whole site stalling rather than as one slow page. `lane_view` took the session and kept it open through a celery inspect whose budget is 11s. The System tab polls that endpoint every 15s — and with a lane not answering, every inspect runs to nearly its full budget, so each poll pinned a connection for most of the interval. SQLAlchemy's default pool is 5 plus 10 overflow. Two browser tabs, `/api/system/health` doing the same thing, and a cap change adding two more inspects exhausts it, and every OTHER request then waits for a connection. Split so the database work finishes before the broker work starts: - `lane_settings(session)` reads the caps and the oldest running task, then the session closes. `lane_view(settings)` does the inspect with none held. - `store_lane_cap(session, …)` validates and commits, then the session closes. `push_lane_cap(lane, …)` does the live push with none held. And a second finding while measuring it: **raising a cap now costs no broker round trip at all.** The first cut only knew on/off, so it inspected on every raise to find out whether the pool needed lowering — the control meant to be instant still waited out an inspect. `store_lane_cap` returns the PREVIOUS cap so the push knows the direction; only a lowering needs to say anything. The guard is structural, not timed: `lane_view` and `push_lane_cap` must not ACCEPT a session. A timing test would be flaky, and a call-order test would pass against a version that took the session and merely used it early. `/api/system/health` has the same shape and is NOT fixed here — it is rate-limited by `refresh_if_stale` so it does not inspect on every request. Worth doing, separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVjrnpQjRgHdvq95rASoiR
This commit is contained in:
@@ -255,3 +255,81 @@ async def _refreshed(db, name: str, expected: int) -> None:
|
||||
row = await _lane_row(db, name)
|
||||
await db.refresh(row)
|
||||
assert row.slots_cap == expected, "a refused write must store nothing"
|
||||
|
||||
|
||||
# --- no database connection is held across a broker round trip ---------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_lane_read_holds_no_session_while_it_inspects(monkeypatch):
|
||||
"""Operator, 2026-09-23: *"something about changing the cap number is
|
||||
blocking to the website... it shouldn't be"*.
|
||||
|
||||
Nothing here was slow in itself. A Postgres connection was held across a
|
||||
celery inspect whose budget is eleven seconds, on a page that polls every
|
||||
fifteen — so with a lane not answering, each poll pinned a connection for
|
||||
most of the interval. SQLAlchemy's default pool is five plus ten overflow;
|
||||
two browser tabs, the health endpoint doing the same, and a cap change
|
||||
adding more inspects exhausts it, and every OTHER request then waits on a
|
||||
connection. It surfaced as the whole site stalling rather than as one slow
|
||||
page, which is why it took a screenshot to find.
|
||||
|
||||
Asserted STRUCTURALLY rather than by timing: `lane_view` must not accept a
|
||||
session at all. A timing test would be flaky, and a mock-call-order test
|
||||
would pass against a version that took the session and merely used it
|
||||
early — the property that matters is that it CANNOT.
|
||||
"""
|
||||
import inspect as _inspect
|
||||
|
||||
from backend.app.services.worker_control import lane_settings, lane_view
|
||||
|
||||
assert "session" not in _inspect.signature(lane_view).parameters, (
|
||||
"lane_view takes a session again; the broker work must run with none held"
|
||||
)
|
||||
# And the DB half still exists, so the split did not simply lose the reads.
|
||||
assert "session" in _inspect.signature(lane_settings).parameters
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_cap_write_holds_no_session_while_it_pushes():
|
||||
"""The same property on the write path, where it was worse: a cap change
|
||||
could make three broker round trips, each with a connection held."""
|
||||
import inspect as _inspect
|
||||
|
||||
from backend.app.services.worker_control import push_lane_cap, store_lane_cap
|
||||
|
||||
assert "session" in _inspect.signature(store_lane_cap).parameters
|
||||
assert "session" not in _inspect.signature(push_lane_cap).parameters, (
|
||||
"push_lane_cap takes a session again; the push must run with none held"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_raising_a_cap_costs_no_broker_round_trip_at_all(
|
||||
client, db, no_live_workers, monkeypatch,
|
||||
):
|
||||
"""The common case must be instant. Raising a cap is permission, not a
|
||||
request — the sizing pass spends it — so there is nothing to tell the
|
||||
broker, and the operator's `+` should answer immediately rather than
|
||||
waiting out an inspect."""
|
||||
from backend.app.services import worker_control as wc
|
||||
|
||||
calls = []
|
||||
monkeypatch.setattr(
|
||||
wc, "inspect_lanes_sync", lambda: calls.append("inspect") or {},
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
wc, "set_lane_slots_sync",
|
||||
lambda *a, **k: calls.append("resize") or (True, None),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
wc, "set_lane_enabled_sync",
|
||||
lambda *a, **k: calls.append("consumers") or (True, None),
|
||||
)
|
||||
|
||||
await client.post("/api/system/workers/worker", json={"slots_cap": 1})
|
||||
calls.clear()
|
||||
resp = await client.post("/api/system/workers/worker", json={"slots_cap": 6})
|
||||
|
||||
assert resp.status_code == 200
|
||||
assert calls == [], f"raising a cap talked to the broker: {calls}"
|
||||
|
||||
Reference in New Issue
Block a user