feat(cloud): persistent agent-run history — AgentExecution + API (C3) - #378
Conversation
Third slice of Track C: run history/audit survives restarts (UC-S2/S3). Until now runs lived only in agent_runner's in-memory task dict.
- Model (api/models/agent_execution.py): AgentExecution — workspace-scoped (FK CASCADE), user-attributed (FK SET NULL so audit survives user deletion), spec_id/agent_type/model, status lifecycle running→completed/failed/cancelled with CHECK constraints on both closed sets, started_at/finished_at. Migration 005.
- Service (services/execution_log.py): create/list/get/mark_finished with the request session, plus record_execution_result for the background task — opens its own short-lived session (request session is gone by run end), injectable factory, best-effort (a persistence failure never breaks a run).
- Wiring: POST /api/agents/run resolves the audit context (single mode → Personal workspace; team mode → body workspace_id + >= editor check, 400/403; non-numeric legacy subs skip recording) and creates the row before starting the task; start-time failures (404/409/400/500) finish it as failed. agent_runner records completed/failed and — via an asyncio.CancelledError handler — cancelled.
- API (api/routes/executions.py): GET /api/executions (workspace-scoped via get_current_workspace, newest first, limit 1-200) and GET /api/executions/{id} (viewer access to its workspace; 404/403).
- Tests: 9 new (lifecycle, background finisher with injected session factory, workspace scoping, DB CHECK rejection, history API incl. cross-workspace 403, run-endpoint wiring incl. failed-start and legacy-sub skip, team-mode 400/403). Existing agent-route mocks updated for the new execution_id kwarg. Full web-backend suite: 249 passed, 9 skipped (test_cloud_e2e excluded — pre-existing missing 'requests' dep); ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a durable ChangesAgent Execution History
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AgentsRoute
participant ExecutionLog
participant AgentRunner
Client->>AgentsRoute: POST /api/agents/run
AgentsRoute->>AgentsRoute: resolve workspace/user context
AgentsRoute->>ExecutionLog: create_execution(...)
ExecutionLog-->>AgentsRoute: execution record
AgentsRoute->>AgentRunner: start_agent_task(execution_id)
alt startup error
AgentsRoute->>ExecutionLog: mark_execution_finished(failed)
AgentsRoute-->>Client: 404/409/400/500
else success
AgentsRoute-->>Client: 202 Accepted
AgentRunner->>ExecutionLog: record_execution_result(completed/failed/cancelled)
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web-backend/services/agent_runner.py (1)
219-291: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOffload the synchronous DB write off the event loop.
record_execution_resultperforms blocking SQLAlchemy I/O (session open +commit), but it's called directly inside therun_agent_asynccoroutine at each terminal path (lines 219-223, 260, 274, 291). Under cloud multitenancy several runs share one event loop, so a blocking commit here stalls other coroutines (WebSocket broadcasts, other runs) for the duration of the write. Consider awaiting it viaasyncio.to_thread(...)to keep the loop responsive while preserving best-effort semantics.♻️ Example (apply to each call site)
- record_execution_result( - execution_id, - "completed" if success else "failed", - None if success else "Planner execution failed", - ) + await asyncio.to_thread( + record_execution_result, + execution_id, + "completed" if success else "failed", + None if success else "Planner execution failed", + )Note: for the
asyncio.CancelledErrorpath (line 274), the write happens during cancellation; keep it before theraiseand confirm the offloaded call completes (or accept a best-effort miss) so cancellation isn't delayed indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web-backend/services/agent_runner.py` around lines 219 - 291, The terminal-state persistence in run_agent_async is doing blocking database I/O on the event loop via record_execution_result, which can stall other coroutines. Update each call site in the planner success/failure return path, the coder/qa_reviewer/qa_fixer completion path, and the exception and CancelledError handlers to run record_execution_result through asyncio.to_thread (or equivalent offloading) so the event loop stays responsive. Keep the cancellation-path write before re-raising asyncio.CancelledError, and preserve the existing execution_id/status/message arguments and logger/broadcast behavior around it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web-backend/api/models/agent_execution.py`:
- Around line 67-68: The timestamp fields in AgentExecution are currently using
naive DateTime storage, which strips the UTC offset on round-trip. Update the
started_at and finished_at columns in the AgentExecution model to use
timezone-aware DateTime and apply the same change in the
005_create_agent_executions migration so the database preserves UTC values. Make
sure the ExecutionResponse path still reads these fields as timezone-aware
datetimes after the schema change.
In `@apps/web-backend/api/routes/executions.py`:
- Around line 56-58: The execution route is duplicating the auth sub-to-user_id
parsing that already exists in core.permissions.get_current_workspace via
_current_user_id(auth). Replace the inline sub handling in executions.py with a
shared helper such as a public current_user_id(auth) from core.permissions, and
reuse that result before calling check_workspace_access. Keep the legacy-token
parsing rules centralized in one place so the route stays aligned with the
permissions logic.
- Around line 30-65: Convert the two route handlers in list_workspace_executions
and get_workspace_execution from async def to regular def so FastAPI executes
the blocking SQLAlchemy calls in its threadpool instead of blocking the event
loop; keep the existing Depends, list_executions, get_execution, and
check_workspace_access logic unchanged while updating only the function
declarations.
---
Outside diff comments:
In `@apps/web-backend/services/agent_runner.py`:
- Around line 219-291: The terminal-state persistence in run_agent_async is
doing blocking database I/O on the event loop via record_execution_result, which
can stall other coroutines. Update each call site in the planner success/failure
return path, the coder/qa_reviewer/qa_fixer completion path, and the exception
and CancelledError handlers to run record_execution_result through
asyncio.to_thread (or equivalent offloading) so the event loop stays responsive.
Keep the cancellation-path write before re-raising asyncio.CancelledError, and
preserve the existing execution_id/status/message arguments and logger/broadcast
behavior around it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8b83ffe-e2a4-4813-b78f-caed45d30674
📒 Files selected for processing (10)
apps/web-backend/api/models/agent_execution.pyapps/web-backend/api/routes/agents.pyapps/web-backend/api/routes/executions.pyapps/web-backend/main.pyapps/web-backend/migrations/versions/005_create_agent_executions.pyapps/web-backend/services/agent_runner.pyapps/web-backend/services/execution_log.pyapps/web-backend/tests/conftest.pyapps/web-backend/tests/test_api_agents.pyapps/web-backend/tests/test_executions.py
…ar (Sonar) S1192: 'Planner execution failed' was used 3x in agent_runner (broadcast, run record, API result) — hoist to _PLANNER_FAILED so they can't drift. S1481: unused 'outsider' in the team-mode test replaced with _. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing (C3 review) Address CodeRabbit review on the run-history slice: - model + migration 005: started_at/finished_at are DateTime(timezone=True) — values are written aware-UTC and naive columns would drop the offset on Postgres round-trip. - executions routes: plain def instead of async def so the blocking SQLAlchemy work runs in FastAPI's threadpool; reuse the shared sub-parsing. - permissions: _current_user_id promoted to public current_user_id (single place encoding the legacy-token rule); executions + agents routes now reuse it instead of re-parsing inline. - agent_runner: terminal-state writes go through asyncio.to_thread so the event loop isn't stalled by DB I/O; the CancelledError path stays synchronous on purpose (an await there could be interrupted by a second cancellation and lose the write). 51 tests pass (executions + agents + workspaces); ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
1 participant
Third slice of Track C (cloud multitenancy — see docs/strategy/roadmap.md), building on the merged C1 (models/permissions) and C2 (workspace context). Until now agent runs lived only in
agent_runner's in-memory task dict — history died on restart. C3 makes runs durable (UC-S2/S3, audit).What's delivered
AgentExecutionmodel + migration 005: workspace-scoped (FK CASCADE), user-attributed (FK SET NULL — audit survives user deletion),spec_id/agent_type/model, status lifecyclerunning → completed/failed/cancelled, CHECK constraints on both closed sets,started_at/finished_at.services/execution_log.py: request-session CRUD +record_execution_resultfor the background task — it opens its own short-lived session (the request session is gone by run end), with an injectable factory for tests; best-effort by design (a persistence failure never breaks a run).POST /api/agents/runresolves the audit context — single mode → the caller's Personal workspace; team mode → bodyworkspace_id+>= editorcheck (400/403); non-numeric legacy subs skip recording — creates the row before the task starts, and start-time failures (404/409/400/500) finish it asfailed.agent_runnerrecordscompleted/failed, and anasyncio.CancelledErrorhandler recordscancelledoncancel_task().api/routes/executions.py):GET /api/executions(workspace-scoped viaget_current_workspace, newest first,limit1–200) andGET /api/executions/{id}(viewer access to its workspace; 404/403).Tests
9 new tests: lifecycle, background finisher (injected session factory, no-op paths), workspace scoping, DB CHECK rejection, history API incl. cross-workspace 403, run-endpoint wiring (record created +
execution_idthreaded, failed-start →failed, legacy-sub skip), team-mode 400/403. Existing agent-route mocks updated for the newexecution_idkwarg.Full web-backend suite: 249 passed, 9 skipped; ruff clean; routes verified in the OpenAPI schema. (
test_cloud_e2e.pyexcluded from the local run — pre-existing missingrequestsdep in the venv, unrelated.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes