perf: cache per-agent container stats + bulk /sync-health lookup (#73) - #983
Conversation
Backend CPU saturation fix for #73. Two backend-only changes: - /agents/{name}/stats: per-agent TTL cache (12s, env-overridable via AGENT_STATS_CACHE_TTL_SECONDS) + asyncio single-flight so concurrent same-agent requests share one container.stats() Docker call instead of N. A generation counter discards an in-flight leader's stale write after lifecycle invalidation; errors (404/400/500) are never cached. invalidate_agent_stats_cache is wired into start/stop/delete. - /agents/sync-health: replace the per-agent N+1 auto-sync lookup with a single scoped get_all_git_auto_sync_enabled(accessible) query, chunked at 900 IN-params to stay under SQLite's host-parameter cap. Mirrors the PERF-269 context-stats cache pattern (in-process, per-worker). No schema change, no new endpoints, no frontend change. Payload shapes byte-for-byte unchanged. Tests (14 passing, no live server): test_73_stats_cache.py (single-flight coalescing, per-agent lock isolation, TTL hit/expiry, invalidation, F4 in-flight race, 404/400 not-cached, defensive env parse, TTL=0 disable, payload parity) + test_73_sync_health_bulk.py (scope, parity, empty/unscoped, multi-chunk merge). The frontend WS-reconnect-backoff half of #73 is a tracked follow-up, not this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-landing review (Claude + Codex adversarial; Codex rated P2) found the new per-agent stats cache was only invalidated by the start/stop/delete router handlers. Internal lifecycle paths — deploy_local, ops restart, subscription re-assign, system restart, rename — change containers via the container_stop/start/remove/rename primitives directly and bypassed those handlers, so GET /stats could serve stale "running" data for up to the cache TTL (12s) after an out-of-band restart, and a renamed-away/reused name could briefly serve a different agent's stats. Move invalidation into the docker_utils lifecycle primitives — the enforced chokepoint for every container op — so all paths invalidate, not just three routers. Remove the now-redundant router-level hooks (keep the separate PERF-269 context-stats invalidation). A function-local import breaks the docker_utils -> agent_service cycle; the helper is best-effort (logged and swallowed) so it can never break the lifecycle op it follows, and rename evicts the OLD name captured before container.rename(). Also add the 500-error-not-cached test flagged by the testing specialist. Tests: 1831 passed, 7 skipped (+7 new: 6 primitive-boundary, 1 500-path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…w log to warning The _invalidate_agent_stats_for docstring overclaimed 'every container lifecycle op' — the Operating Room emergency-stop fast path (routers/ops.py:_stop_agent_container) calls container.stop() directly and relies on the cache TTL (<=12s) instead. Name that exception explicitly. Bump the swallowed-invalidation log from debug to warning so a systematic failure (e.g. the lazy import regressing) is visible in logs rather than only as <=TTL stale stats; the per-call swallow is preserved so a lifecycle op never breaks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merge the three parallel module-level dicts (_agent_stats_cache,
_agent_stats_locks, _agent_stats_gen) into one _agent_stats dict of
_AgentStatsEntry{lock, gen, data, timestamp}. Invalidation marks the slot
stale and bumps gen while KEEPING the entry and reusing its lock — popping
would let a leader that captured the default gen=0 match a post-pop default
and repopulate a just-deleted agent's cache (the F4 race). Correct the
gen-growth comment: retention is bounded by distinct agent names seen over
the process lifetime, not 'fleet size'.
Update the white-box tests to the consolidated structure (via an _is_cached
helper) and add a caplog test asserting the invalidation-failure path now
logs at warning. test_invalidation_during_inflight_discards_stale_write still
passes, proving the refactor preserves the never-pop-gen guarantee.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vybe
left a comment
There was a problem hiding this comment.
LGTM — read the concurrency logic in full. Single-flight (atomic setdefault), generation-guard (captured before Docker await, write gated on unchanged gen), and double-checked locking are all correct; errors never cached. The Codex-P2 fix moving invalidation to the docker_utils primitive chokepoint (container_stop/start/remove/rename) is the right call — all out-of-band lifecycle paths invalidate, and rename evicts the OLD name first. Bulk /sync-health SQL is parameterized (f-string is placeholders only) and chunked at 900 under SQLite's param cap. Payload byte-identical, CI green incl schema-parity, 21 targeted tests. Backend half only — #73 stays open for the frontend WS-backoff follow-up. Validated via /validate-pr.
What & why
Fixes the addressable backend CPU sink in #73: a 4-vCPU instance running ~30 agents with 2-3 browser users sits at ~45% backend CPU. Backend-only changes.
1. TTL cache + single-flight on
GET /api/agents/{name}/stats(the dominant cost)container.stats(stream=False)costs ~1-2s of Docker double-sampling, was uncached, had no request coalescing, and funnels through a 4-worker thread pool. N agents × multiple tabs polling every 10s pinned the CPU.AGENT_STATS_CACHE_TTL_SECONDS, defensively parsed — a bad value never crashes startup;0disables).asyncio.Locksingle-flight: concurrent same-agent misses share one Docker call instead of N.docker_utilslifecycle-primitive boundary (container_stop/start/remove/rename) — the enforced chokepoint for every container op — so all state-changing paths invalidate (UI start/stop/delete, ops restart, deploy_local, subscription re-assign, system restart, rename), not just the API routers. (Earlier revisions hooked only the routers; see the review note below.)2. Bulk the
/api/agents/sync-healthauto-sync lookup (removes an N+1)Replaced the per-agent
get_git_auto_sync_enabled(name)loop with a single scopedget_all_git_auto_sync_enabled(accessible)query, chunked at 900IN-params to stay under SQLite's host-parameter cap. Access scope unchanged (stillget_accessible_agents).Mirrors the PERF-269 context-stats cache pattern (in-process, per-worker). No schema change, no new endpoints, no frontend change. Payload shapes byte-for-byte identical.
Testing
21 unit tests, no live server (Docker layer monkeypatched):
tests/unit/test_73_stats_cache.py— single-flight coalescing (10 concurrent → 1 Docker call), per-agent lock isolation, TTL hit/expiry, invalidation, in-flight invalidation race (generation guard), 404/400/500 not-cached, defensive env parse, TTL=0 disable, payload parity, and primitive-boundary invalidation (stop/start/remove/rename + label fallback + non-agent no-op).tests/unit/test_73_sync_health_bulk.py— scoped/unscoped/empty-set maps, per-agent parity, multi-chunkIN()merge.Pre-landing review (gstack /ship)
container_stop/start/remove/renameprimitives directly — so/statscould serve stale "running" data for ≤ the 12s TTL after an out-of-band restart, and a renamed-away/reused name could briefly serve a different agent's stats. Resolved by moving invalidation to the primitive chokepoint (commit831dff9f). Re-verified: circular import broken via lazy import, coverage strictly expands (no path lost invalidation), rename evicts the OLD name, helper is best-effort and can't break a lifecycle op.ruffon the touched modules adds no new errors (pre-existing repo-wide debt unchanged).Not in this PR (tracked)
utils/websocket.jsflat 5s retry, no backoff/jitter). Frontend-only, separate blast radius → follow-up under perf: Backend CPU saturation at 45% with ~30 agents due to WebSocket churn and polling storms #73. This PR is backend-only by approved scope.Closes the backend half of #73.
🤖 Generated with Claude Code