fix(projects): batch the summary queries — the fan-out was exhausting the pool
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 12s
CI & Build / TypeScript typecheck (push) Successful in 43s
CI & Build / integration (push) Successful in 2m33s
CI & Build / Python tests (push) Successful in 3m1s
CI & Build / Build & push image (push) Successful in 44s
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 12s
CI & Build / TypeScript typecheck (push) Successful in 43s
CI & Build / integration (push) Successful in 2m33s
CI & Build / Python tests (push) Successful in 3m1s
CI & Build / Build & push image (push) Successful in 44s
Reported live: Projects and Snippets showed skeletons that never resolved,
/knowledge worked intermittently. The logs named it exactly:
QueuePool limit of size 5 overflow 10 reached, connection timed out, 30.00
GET /api/settings 500 30584.0ms
GET /api/projects 200 30882.9ms
/api/projects was not hanging — it was waiting out the 30-second checkout
timeout and then returning 200 with summaries silently missing, because
_attach swallowed the TimeoutError. Nobody waits 31 seconds, so it read as a
hang.
THE SHAPE: routes/projects.py ran asyncio.gather over every project. Each
_attach called get_project_summary, which opened its own session for three
queries and then called get_project_milestone_summary — which opened one more
session PER MILESTONE. So 25 projects asked for roughly 250 concurrent
checkouts against a pool of 15 (SQLAlchemy's default 5 + 10 overflow).
That is why unrelated routes failed too. Snippets and /knowledge were never
broken; they queued behind the burst and inherited its timeout. /api/settings
returning 500 while /api/projects returned 200 is the same cause wearing two
faces.
The comment above the gather said "one backend pass instead of N+1 frontend
calls". It did remove the N+1 from the network — and recreated it against the
connection pool, where it is worse, because the browser had at least been
serialising those calls.
Now: get_project_summaries() does all projects in four queries and one session,
and get_project_milestone_summaries() does all milestones in two. Two sessions
total for the whole page, independent of how many projects exist.
The progress calculation is extracted to _progress_from_counts and shared by
both the batch and single paths, so the cancelled-exclusion rule cannot drift
into two versions that disagree about whether a milestone is finished.
Tests assert the SESSION COUNT, not just the values. An implementation that
returned identical output while opening a session per project would pass a
correctness test and reproduce the outage.
Deliberately NOT done: raising pool_size. It would move the cliff rather than
remove it, and this endpoint now needs two connections regardless of scale.
Closes #2384.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs
This commit is contained in:
@@ -0,0 +1,128 @@
|
||||
"""Batched project summaries (#2384).
|
||||
|
||||
The bug was not wrong output — it was CONNECTION COUNT. A per-project summary
|
||||
opened its own session and then one more per milestone, and the route fanned
|
||||
that out with asyncio.gather. For 25 projects that asked for roughly 250
|
||||
checkouts against a pool of 15, so most waited out the 30-second timeout and
|
||||
every other route on the instance queued behind them:
|
||||
|
||||
QueuePool limit of size 5 overflow 10 reached, connection timed out
|
||||
GET /api/settings 500 30584.0ms
|
||||
GET /api/projects 200 30882.9ms
|
||||
|
||||
So these tests assert the number of sessions opened, not only the values
|
||||
returned. A version that produced identical output while opening a session per
|
||||
project would pass a correctness test and reproduce the outage.
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
def _session_factory(counter: list[int], results: list):
|
||||
"""A session whose .execute() returns queued results, counting opens."""
|
||||
def _make():
|
||||
s = AsyncMock()
|
||||
s.__aenter__ = AsyncMock(return_value=s)
|
||||
s.__aexit__ = AsyncMock(return_value=False)
|
||||
counter[0] += 1
|
||||
|
||||
async def _execute(*_a, **_kw):
|
||||
rows = results.pop(0) if results else []
|
||||
r = MagicMock()
|
||||
r.fetchall = MagicMock(return_value=rows)
|
||||
r.scalars = MagicMock(return_value=MagicMock(all=lambda: rows))
|
||||
return r
|
||||
s.execute = _execute
|
||||
return s
|
||||
return _make
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_summaries_for_many_projects_open_ONE_session():
|
||||
"""The whole point. Twenty-five projects must not mean twenty-five
|
||||
checkouts — that is the shape that exhausted the pool."""
|
||||
from scribe.services import projects as svc
|
||||
|
||||
opened = [0]
|
||||
rows = [
|
||||
[(1, "todo", 3), (1, "done", 2), (2, "in_progress", 1)], # task counts
|
||||
[(1, 7)], # note counts
|
||||
[], # last activity
|
||||
]
|
||||
with patch.object(svc, "async_session", _session_factory(opened, rows)), \
|
||||
patch("scribe.services.milestones.get_project_milestone_summaries",
|
||||
AsyncMock(return_value={})):
|
||||
out = await svc.get_project_summaries(1, [1, 2])
|
||||
|
||||
assert opened[0] == 1, f"opened {opened[0]} sessions for 2 projects"
|
||||
assert out[1]["task_counts"] == {"todo": 3, "in_progress": 0, "done": 2}
|
||||
assert out[1]["note_count"] == 7
|
||||
# A project with tasks but no notes still reports 0, not a missing key.
|
||||
assert out[2]["task_counts"] == {"todo": 0, "in_progress": 1, "done": 0}
|
||||
assert out[2]["note_count"] == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_every_requested_project_gets_an_entry():
|
||||
"""A project with no notes at all must still appear. The frontend indexes
|
||||
by id and renders `undefined.task_counts` as a crash, not a blank."""
|
||||
from scribe.services import projects as svc
|
||||
|
||||
opened = [0]
|
||||
with patch.object(svc, "async_session", _session_factory(opened, [[], [], []])), \
|
||||
patch("scribe.services.milestones.get_project_milestone_summaries",
|
||||
AsyncMock(return_value={})):
|
||||
out = await svc.get_project_summaries(1, [4, 5, 6])
|
||||
|
||||
assert sorted(out) == [4, 5, 6]
|
||||
for entry in out.values():
|
||||
assert entry["task_counts"] == {"todo": 0, "in_progress": 0, "done": 0}
|
||||
assert entry["note_count"] == 0
|
||||
assert entry["last_activity"] is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_projects_opens_no_session_at_all():
|
||||
"""`.in_([])` is a valid but pointless query; the guard keeps an empty
|
||||
install from paying for a connection to learn it has nothing."""
|
||||
from scribe.services import projects as svc
|
||||
|
||||
opened = [0]
|
||||
with patch.object(svc, "async_session", _session_factory(opened, [])):
|
||||
assert await svc.get_project_summaries(1, []) == {}
|
||||
assert opened[0] == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_milestone_summaries_for_many_projects_open_ONE_session():
|
||||
"""Same property one level down — this was the nested half of the fan-out,
|
||||
a session per MILESTONE, which is what turned 25 into ~250."""
|
||||
from scribe.services import milestones as svc
|
||||
|
||||
m1 = MagicMock(id=10, project_id=1)
|
||||
m1.to_dict = MagicMock(return_value={"id": 10, "title": "A"})
|
||||
m2 = MagicMock(id=11, project_id=2)
|
||||
m2.to_dict = MagicMock(return_value={"id": 11, "title": "B"})
|
||||
|
||||
opened = [0]
|
||||
rows = [[m1, m2], [(10, "done", 2), (10, "todo", 1), (11, "cancelled", 1)]]
|
||||
with patch.object(svc, "async_session", _session_factory(opened, rows)):
|
||||
out = await svc.get_project_milestone_summaries(1, [1, 2])
|
||||
|
||||
assert opened[0] == 1
|
||||
assert out[1][0]["completed"] == 2 and out[1][0]["total"] == 3
|
||||
# Cancelled is excluded from the denominator, so a milestone whose only
|
||||
# task was cancelled reads as complete rather than stalled at 0%.
|
||||
assert out[2][0]["pct"] == 0.0 and out[2][0]["status_counts"]["cancelled"] == 1
|
||||
|
||||
|
||||
def test_both_progress_paths_share_one_rule():
|
||||
"""get_milestone_progress and the batch path must not compute pct
|
||||
differently — two screens disagreeing about whether a milestone is done is
|
||||
exactly the drift this codebase keeps finding."""
|
||||
from scribe.services.milestones import _progress_from_counts
|
||||
|
||||
assert _progress_from_counts({"done": 3, "cancelled": 1})["pct"] == 100.0
|
||||
assert _progress_from_counts({"cancelled": 2})["pct"] == 0.0
|
||||
assert _progress_from_counts({})["total"] == 0
|
||||
Reference in New Issue
Block a user