diff --git a/src/scribe/routes/projects.py b/src/scribe/routes/projects.py index d0ca0f9..d014892 100644 --- a/src/scribe/routes/projects.py +++ b/src/scribe/routes/projects.py @@ -1,5 +1,4 @@ """Project management routes.""" -import asyncio import logging from quart import Blueprint, jsonify, request @@ -13,6 +12,7 @@ from scribe.services.projects import ( delete_project, get_project, get_project_for_user, + get_project_summaries, get_project_summary, list_projects_for_user, update_project, @@ -31,16 +31,28 @@ async def list_projects_route(): include_summary = request.args.get("include_summary", "").lower() in ("1", "true") projects = await list_projects_for_user(uid, status=status) if include_summary: - # Fetch all summaries in parallel — one backend pass instead of N+1 frontend calls - async def _attach(project_dict: dict) -> dict: + # Batched: four queries plus two, in two sessions, for ALL projects. + # This replaced an asyncio.gather over a per-project summary that opened + # its own session and then one more per milestone — ~250 concurrent + # checkouts against a pool of 15, all waiting out the 30s timeout and + # starving every other route on the instance (#2384). + # + # Grouped by OWNER because a shared project's counts belong to its + # owner's records, matching what the per-project path passed. + by_owner: dict[int, list[dict]] = {} + for p in projects: + by_owner.setdefault(p.get("user_id") or uid, []).append(p) + for owner_uid, owned in by_owner.items(): try: - owner_uid = project_dict.get("user_id") or uid # user_id now in to_dict() - summary = await get_project_summary(owner_uid, project_dict["id"]) - project_dict["summary"] = summary + summaries = await get_project_summaries( + owner_uid, [p["id"] for p in owned] + ) except Exception: - pass - return project_dict - projects = list(await asyncio.gather(*[_attach(p) for p in projects])) + logger.warning("Project summaries failed", exc_info=True) + continue + for p in owned: + if p["id"] in summaries: + p["summary"] = summaries[p["id"]] return jsonify({"projects": projects}) diff --git a/src/scribe/services/milestones.py b/src/scribe/services/milestones.py index a37295d..679aae5 100644 --- a/src/scribe/services/milestones.py +++ b/src/scribe/services/milestones.py @@ -156,26 +156,82 @@ async def get_milestone_progress(milestone_id: int) -> dict: for status, count in rows.fetchall(): status_counts[status] = count - total = sum(status_counts.values()) - cancelled = status_counts.get("cancelled", 0) - completed = status_counts.get("done", 0) - # Cancelled tasks are resolved work, not pending — exclude them from the - # percent-complete denominator so a milestone whose only open task was - # cancelled still reaches 100% (and auto-collapses) instead of stalling. - active_total = total - cancelled - pct = round(completed / active_total * 100, 1) if active_total > 0 else 0.0 + # Same rule as the batch path, computed in one place so the two cannot + # drift on the cancelled-exclusion. + return _progress_from_counts(status_counts) - return { - "total": total, - "completed": completed, - "pct": pct, - "status_counts": { - "todo": status_counts.get("todo", 0), - "in_progress": status_counts.get("in_progress", 0), - "done": status_counts.get("done", 0), - "cancelled": cancelled, - }, - } + +def _progress_from_counts(status_counts: dict[str, int]) -> dict: + """The progress shape, computed from already-fetched counts. + + Split out of get_milestone_progress so the batch path can reuse the rule + rather than restate it — the cancelled-exclusion below is easy to get + subtly different in a second copy, and then two screens disagree about + whether a milestone is finished. + """ + total = sum(status_counts.values()) + cancelled = status_counts.get("cancelled", 0) + completed = status_counts.get("done", 0) + # Cancelled tasks are resolved work, not pending — excluded from the + # denominator so a milestone whose only open task was cancelled reaches + # 100% instead of stalling. + active_total = total - cancelled + return { + "total": total, + "completed": completed, + "pct": round(completed / active_total * 100, 1) if active_total > 0 else 0.0, + "status_counts": { + "todo": status_counts.get("todo", 0), + "in_progress": status_counts.get("in_progress", 0), + "done": status_counts.get("done", 0), + "cancelled": cancelled, + }, + } + + +async def get_project_milestone_summaries( + user_id: int, project_ids: list[int] +) -> dict[int, list[dict]]: + """Milestone summaries for MANY projects in two queries total. + + The per-project version below is a nested fan-out: one query to list a + project's milestones, then one more per milestone for its progress. Called + for 25 projects concurrently it asked for ~250 pooled connections against a + pool of 15, and every one of them waited out the 30-second checkout timeout + (#2384). This does the same work in two queries and one session. + """ + if not project_ids: + return {} + + async with async_session() as session: + milestones = list((await session.execute( + select(Milestone).where( + Milestone.user_id == user_id, + Milestone.project_id.in_(project_ids), + Milestone.deleted_at.is_(None), + ).order_by(Milestone.order_index.asc(), Milestone.created_at.asc()) + )).scalars().all()) + + counts: dict[int, dict[str, int]] = {} + if milestones: + rows = await session.execute( + select(Note.milestone_id, Note.status, func.count(Note.id)) + .where( + Note.milestone_id.in_([m.id for m in milestones]), + Note.status.isnot(None), + Note.deleted_at.is_(None), + ) + .group_by(Note.milestone_id, Note.status) + ) + for milestone_id, status, count in rows.fetchall(): + counts.setdefault(milestone_id, {})[status] = count + + out: dict[int, list[dict]] = {pid: [] for pid in project_ids} + for m in milestones: + entry = m.to_dict() + entry.update(_progress_from_counts(counts.get(m.id, {}))) + out.setdefault(m.project_id, []).append(entry) + return out async def get_project_milestone_summary(user_id: int, project_id: int) -> list[dict]: diff --git a/src/scribe/services/projects.py b/src/scribe/services/projects.py index 0a51c99..cfae26e 100644 --- a/src/scribe/services/projects.py +++ b/src/scribe/services/projects.py @@ -127,6 +127,86 @@ async def delete_project(user_id: int, project_id: int) -> bool: return True +async def get_project_summaries( + user_id: int, project_ids: list[int] +) -> dict[int, dict]: + """Summaries for MANY projects — four queries and one session, total. + + Replaces an `asyncio.gather` over the per-project version below, which was + a nested fan-out: each project opened its own session for three queries, + then called the milestone summary, which opened one more per milestone. For + 25 projects that asked for roughly 250 pooled connections at once against a + pool of 15 (SQLAlchemy's default 5 + 10 overflow), so most of them sat out + the 30-second checkout timeout and everything else on the instance queued + behind them — including unrelated routes, which is why /api/settings + returned 500 while /api/projects took 30.9s (#2384). + + The comment it replaced 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: the browser had at least been + serialising those calls. + """ + if not project_ids: + return {} + + async with async_session() as session: + task_rows = await session.execute( + select(Note.project_id, Note.status, func.count(Note.id)) + .where( + Note.user_id == user_id, + Note.project_id.in_(project_ids), + Note.status.isnot(None), + Note.deleted_at.is_(None), + ) + .group_by(Note.project_id, Note.status) + ) + task_counts: dict[int, dict[str, int]] = {} + for project_id, status, count in task_rows.fetchall(): + task_counts.setdefault(project_id, {})[status] = count + + note_rows = await session.execute( + select(Note.project_id, func.count(Note.id)) + .where( + Note.user_id == user_id, + Note.project_id.in_(project_ids), + Note.status.is_(None), + Note.deleted_at.is_(None), + ) + .group_by(Note.project_id) + ) + note_counts = {pid: count for pid, count in note_rows.fetchall()} + + # Deliberately NOT filtered by deleted_at, matching the per-project + # version: "last activity" includes trashing something. + activity_rows = await session.execute( + select(Note.project_id, func.max(Note.updated_at)) + .where(Note.user_id == user_id, Note.project_id.in_(project_ids)) + .group_by(Note.project_id) + ) + last_activity = {pid: ts for pid, ts in activity_rows.fetchall()} + + from scribe.services.milestones import get_project_milestone_summaries + milestones = await get_project_milestone_summaries(user_id, project_ids) + + return { + pid: { + # All three lifecycle keys present so consumers can sum without + # `?? 0` guards — the frontend declares them required, and + # `undefined + N` renders as NaN. + "task_counts": { + "todo": 0, "in_progress": 0, "done": 0, + **task_counts.get(pid, {}), + }, + "note_count": note_counts.get(pid, 0), + "last_activity": ( + last_activity[pid].isoformat() if last_activity.get(pid) else None + ), + "milestone_summary": milestones.get(pid, []), + } + for pid in project_ids + } + + async def get_project_summary(user_id: int, project_id: int) -> dict: """Return task counts by status, note count, and last activity.""" async with async_session() as session: diff --git a/tests/test_services_project_summaries.py b/tests/test_services_project_summaries.py new file mode 100644 index 0000000..07dde57 --- /dev/null +++ b/tests/test_services_project_summaries.py @@ -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