feat(cron): Add comprehensive cron jobs support#316
Open
Quentin-Gillet wants to merge 12 commits intoaegra:mainfrom
Open
feat(cron): Add comprehensive cron jobs support#316Quentin-Gillet wants to merge 12 commits intoaegra:mainfrom
Quentin-Gillet wants to merge 12 commits intoaegra:mainfrom
Conversation
Add full cron job support as a drop-in replacement for LangSmith Deployments: - 6 Agent Protocol endpoints: create, create-for-thread, update, delete, search, count - CronService with schedule validation (croniter), payload persistence, metadata - CronScheduler background task for automatic run triggering - CronORM model with Alembic migration - CRON_ENABLED and CRON_POLL_INTERVAL_SECONDS settings - Health flag for cron subsystem
- 59 unit tests for CronService: create, update, delete, search, count, get_due_crons, advance_next_run, and all helper functions - 15 unit tests for CronScheduler: lifecycle, tick, fire_cron, loop - 30 integration tests for all 6 cron API endpoints including validation errors, filters, pagination, and response structure
_trigger_first_run fires a run immediately when a cron is created. Previously, next_run_date was set to the first scheduled occurrence, causing the scheduler to fire a second run seconds after creation. Fix by advancing next_run_date to the second occurrence so the scheduler starts from the correct interval boundary.
Add IANA timezone support for cron creation and updates; preserve timezone when recomputing next run. Include environment variable docs and a new cron guide, plus unit, integration, and e2e coverage.
…after run creation errors
📝 WalkthroughWalkthroughAdds first-class cron/scheduled-job support: DB migration and ORM, API endpoints and Pydantic models, CronService and CronScheduler, background run cleanup extraction, settings and env examples, docs/examples, tests, and app wiring (lifespan + health flag). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Cron API
participant Service as CronService
participant Scheduler as CronScheduler
participant LangGraph as LangGraph Service
participant DB as Database
rect rgba(100, 150, 200, 0.5)
note over Client,API: Cron creation (immediate run)
Client->>API: POST /runs/crons (assistant_id, schedule, payload)
API->>Service: create_cron(request, user)
Service->>LangGraph: resolve_assistant_id(...)
LangGraph-->>Service: assistant
Service->>DB: INSERT cron (compute next_run_date)
DB-->>Service: cron record
API->>Service: _trigger_first_run(cron)
Service->>LangGraph: _prepare_run(RunCreate)
LangGraph-->>Service: Run (pending)
API-->>Client: 200 OK (run_id, cron_id)
end
rect rgba(200, 150, 100, 0.5)
note over Scheduler,DB: Scheduler polling loop
loop every CRON_POLL_INTERVAL_SECONDS
Scheduler->>DB: SELECT enabled crons WHERE next_run_date <= now
DB-->>Scheduler: due crons
loop for each cron
Scheduler->>Service: build RunCreate from cron.payload
Service->>LangGraph: _prepare_run(RunCreate)
LangGraph-->>Service: Run
Scheduler->>DB: UPDATE cron.next_run_date / disable if ended
DB-->>Scheduler: updated
Scheduler->>Service: schedule_background_cleanup (if stateless)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
libs/aegra-api/src/aegra_api/core/health.py (1)
47-47: Makeflags.cronsreflect runtime configuration
cronsis hardcoded toTrue; whenCRON_ENABLED=false,/infocan report an unavailable capability. Consider wiring it tosettings.cron.CRON_ENABLEDfor accurate introspection.Suggested patch
from aegra_api import __version__ from aegra_api.core.database import db_manager from aegra_api.models.errors import UNAVAILABLE +from aegra_api.settings import settings @@ - flags={"assistants": True, "crons": True}, + flags={"assistants": True, "crons": settings.cron.CRON_ENABLED},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/aegra-api/src/aegra_api/core/health.py` at line 47, The health endpoint currently hardcodes flags={"assistants": True, "crons": True}, which can falsely report crons as available; change the 'crons' flag to reflect runtime config by using the project settings value (e.g., replace the literal True with settings.cron.CRON_ENABLED or the appropriate settings attribute) so the flags dict becomes flags={"assistants": True, "crons": settings.cron.CRON_ENABLED}; ensure you import the settings/crons config symbol referenced (settings.cron.CRON_ENABLED) and keep the key name 'crons' unchanged.libs/aegra-api/src/aegra_api/main.py (1)
285-295: Update_include_core_routersdocstring order list to include Crons.The implementation now includes
crons_router, but the documented router order still skips it, which can mislead future maintenance.Suggested docstring tweak
- Routers are included in consistent order: - 1. Health (no auth) - 2. Assistants (with auth) - 3. Threads (with auth) - 4. Runs (with auth) - 5. Stateless Runs (with auth) - 6. Store (with auth) + Routers are included in consistent order: + 1. Health (no auth) + 2. Assistants (with auth) + 3. Threads (with auth) + 4. Runs (with auth) + 5. Stateless Runs (with auth) + 6. Crons (with auth) + 7. Store (with auth)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/aegra-api/src/aegra_api/main.py` around lines 285 - 295, The docstring for _include_core_routers is out of sync with the implementation because it omits the crons router; update the ordered list in the _include_core_routers docstring to include "Crons (with auth)" in the correct position matching the actual inclusion of crons_router (alongside health, assistants, threads, runs, stateless runs, store) so the documentation reflects the code path that references crons_router.libs/aegra-api/src/aegra_api/services/cron_scheduler.py (1)
92-104: Consider reusingCronService.get_due_cronsto avoid query duplication.This
_find_due_cronsmethod duplicates the query logic fromCronService.get_due_crons(incron_service.py:307-325). Consider instantiatingCronServicewith the session and delegating to it to maintain a single source of truth for the due-crons query.♻️ Suggested approach
+from aegra_api.services.cron_service import CronService + class CronScheduler: ... `@staticmethod` async def _find_due_crons(session: AsyncSession, now: datetime) -> list[CronORM]: """Return enabled crons whose next_run_date has passed.""" - stmt = ( - select(CronORM) - .where( - CronORM.enabled.is_(True), - CronORM.next_run_date <= now, - ) - .order_by(CronORM.next_run_date.asc()) - ) - result = await session.scalars(stmt) - return list(result.all()) + service = CronService(session) + return await service.get_due_crons(now)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/aegra-api/src/aegra_api/services/cron_scheduler.py` around lines 92 - 104, _private method _find_due_crons duplicates the due-crons query from CronService.get_due_crons; replace the duplicate query by creating a CronService instance with the given AsyncSession and call its get_due_crons method instead of reimplementing the select logic. Locate _find_due_crons and change it to delegate to CronService(session).get_due_crons(now) (or the appropriate async call pattern) so the query logic is centralized in CronService.get_due_crons.examples/cron_example/setup.py (1)
35-35: Movehttpximport to the top of the file.The inline import of
httpxinside the function violates the coding guideline requiring all imports at the top of the file. There's no circular dependency concern with this external package.♻️ Proposed fix
Move the import to the top with other imports:
import asyncio from langgraph_sdk import get_client +import httpx async def main() -> None: client = get_client(url="http://localhost:2026") ... - import httpx - async with httpx.AsyncClient() as http:As per coding guidelines: "ALWAYS place imports at the top of the file. Never use inline/lazy imports inside functions unless there is a proven circular dependency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/cron_example/setup.py` at line 35, The inline import "import httpx" must be moved to the module-level imports at the top of examples/cron_example/setup.py; remove the local/in-function import and add "import httpx" with the other top-of-file imports so no inline/lazy import remains (there is no circular dependency here).libs/aegra-api/tests/integration/test_api/test_crons_crud.py (2)
167-169: Remove redundant import.
HTTPExceptionis already imported at line 10. This local import is unnecessary.♻️ Suggested fix
def test_returns_404_when_not_found(self, client, mock_cron_service: AsyncMock) -> None: - from fastapi import HTTPException - mock_cron_service.update_cron.side_effect = HTTPException(404, "Cron 'missing' not found")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/aegra-api/tests/integration/test_api/test_crons_crud.py` around lines 167 - 169, In test_returns_404_when_not_found remove the redundant local import of HTTPException (the line "from fastapi import HTTPException") since HTTPException is already imported at the top of the file; update the test function to rely on the module-level import and delete that duplicate import statement.
193-195: Remove redundant import.Same issue -
HTTPExceptionis already imported at the module level.♻️ Suggested fix
def test_returns_404_when_not_found(self, client, mock_cron_service: AsyncMock) -> None: - from fastapi import HTTPException - mock_cron_service.delete_cron.side_effect = HTTPException(404, "Cron 'missing' not found")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/aegra-api/tests/integration/test_api/test_crons_crud.py` around lines 193 - 195, The test function test_returns_404_when_not_found contains a redundant local import of HTTPException; remove the inner "from fastapi import HTTPException" so the test uses the module-level HTTPException import instead, leaving the rest of the test and the mock_cron_service AsyncMock usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/cron_example/__init__.py`:
- Line 1: Replace the absolute package import in __init__.py with a relative
intra-package import: update the current statement that imports graph to use a
relative import (from .graph import graph) so the module re-exports the graph
symbol consistently with other example modules; keep the exported symbol name
(graph) unchanged so downstream imports continue to work.
In `@libs/aegra-api/src/aegra_api/models/crons.py`:
- Around line 53-72: CronUpdate is missing the checkpoint_during and
multitask_strategy fields that CronCreate defines, but _build_payload in
cron_service.py expects to handle them for both CronCreate | CronUpdate; add
matching optional fields checkpoint_during and multitask_strategy to the
CronUpdate Pydantic model (keeping the same types as in CronCreate) so updates
can modify those values, or if omission was intentional, add a brief comment on
CronUpdate explaining why those fields are exclusionary so _build_payload
handling is clear.
In `@libs/aegra-api/src/aegra_api/settings.py`:
- Around line 217-218: CRON_POLL_INTERVAL_SECONDS must be validated to be > 0;
add a check in the settings class that owns CRON_POLL_INTERVAL_SECONDS (e.g.,
add a pydantic `@validator` for "CRON_POLL_INTERVAL_SECONDS" if using BaseSettings
or perform an explicit runtime check in the class __post_init__/__init__) and
raise a clear ValueError (or ValidationError) when the value is <= 0, ensuring
misconfiguration fails fast with a helpful message referencing
CRON_POLL_INTERVAL_SECONDS.
---
Nitpick comments:
In `@examples/cron_example/setup.py`:
- Line 35: The inline import "import httpx" must be moved to the module-level
imports at the top of examples/cron_example/setup.py; remove the
local/in-function import and add "import httpx" with the other top-of-file
imports so no inline/lazy import remains (there is no circular dependency here).
In `@libs/aegra-api/src/aegra_api/core/health.py`:
- Line 47: The health endpoint currently hardcodes flags={"assistants": True,
"crons": True}, which can falsely report crons as available; change the 'crons'
flag to reflect runtime config by using the project settings value (e.g.,
replace the literal True with settings.cron.CRON_ENABLED or the appropriate
settings attribute) so the flags dict becomes flags={"assistants": True,
"crons": settings.cron.CRON_ENABLED}; ensure you import the settings/crons
config symbol referenced (settings.cron.CRON_ENABLED) and keep the key name
'crons' unchanged.
In `@libs/aegra-api/src/aegra_api/main.py`:
- Around line 285-295: The docstring for _include_core_routers is out of sync
with the implementation because it omits the crons router; update the ordered
list in the _include_core_routers docstring to include "Crons (with auth)" in
the correct position matching the actual inclusion of crons_router (alongside
health, assistants, threads, runs, stateless runs, store) so the documentation
reflects the code path that references crons_router.
In `@libs/aegra-api/src/aegra_api/services/cron_scheduler.py`:
- Around line 92-104: _private method _find_due_crons duplicates the due-crons
query from CronService.get_due_crons; replace the duplicate query by creating a
CronService instance with the given AsyncSession and call its get_due_crons
method instead of reimplementing the select logic. Locate _find_due_crons and
change it to delegate to CronService(session).get_due_crons(now) (or the
appropriate async call pattern) so the query logic is centralized in
CronService.get_due_crons.
In `@libs/aegra-api/tests/integration/test_api/test_crons_crud.py`:
- Around line 167-169: In test_returns_404_when_not_found remove the redundant
local import of HTTPException (the line "from fastapi import HTTPException")
since HTTPException is already imported at the top of the file; update the test
function to rely on the module-level import and delete that duplicate import
statement.
- Around line 193-195: The test function test_returns_404_when_not_found
contains a redundant local import of HTTPException; remove the inner "from
fastapi import HTTPException" so the test uses the module-level HTTPException
import instead, leaving the rest of the test and the mock_cron_service AsyncMock
usage unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 163a6071-2f96-4586-a042-bd11f384cdd3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.env.exampleaegra.jsondocs/docs.jsondocs/feature-support.mdxdocs/getting-started.mdxdocs/guides/assistants.mdxdocs/guides/cron.mdxdocs/migration.mdxdocs/quickstart.mdxdocs/reference/environment-variables.mdxexamples/cron_example/__init__.pyexamples/cron_example/graph.pyexamples/cron_example/setup.pylibs/aegra-api/alembic/versions/20260413201423_add_crons_table.pylibs/aegra-api/pyproject.tomllibs/aegra-api/src/aegra_api/api/crons.pylibs/aegra-api/src/aegra_api/api/stateless_runs.pylibs/aegra-api/src/aegra_api/core/health.pylibs/aegra-api/src/aegra_api/core/orm.pylibs/aegra-api/src/aegra_api/main.pylibs/aegra-api/src/aegra_api/models/__init__.pylibs/aegra-api/src/aegra_api/models/crons.pylibs/aegra-api/src/aegra_api/services/cron_scheduler.pylibs/aegra-api/src/aegra_api/services/cron_service.pylibs/aegra-api/src/aegra_api/services/run_cleanup.pylibs/aegra-api/src/aegra_api/settings.pylibs/aegra-api/tests/e2e/test_crons/__init__.pylibs/aegra-api/tests/e2e/test_crons/test_crons.pylibs/aegra-api/tests/integration/test_api/test_crons_crud.pylibs/aegra-api/tests/unit/test_api/test_crons.pylibs/aegra-api/tests/unit/test_api/test_stateless_runs.pylibs/aegra-api/tests/unit/test_services/test_cron_scheduler.pylibs/aegra-api/tests/unit/test_services/test_cron_service.pylibs/aegra-cli/src/aegra_cli/templates/env.example.template
💤 Files with no reviewable changes (1)
- docs/migration.mdx
Align cron update payload handling, scheduler delegation, health flags, and settings validation. Add regression coverage for the reviewed cron paths and tighten the example package imports.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/aegra-api/src/aegra_api/services/cron_scheduler.py`:
- Around line 132-177: The code currently advances the cron schedule even when
_prepare_run fails; change _fire_cron so it only advances next_run_date or
disables the cron when the run was successfully created. Concretely, introduce a
boolean (e.g., run_created = False) before calling _prepare_run, set run_created
= True after _prepare_run returns successfully in the try block, and in the
except blocks perform logging and call
CronScheduler._cleanup_failed_stateless_thread as now but return or skip the
schedule-advancement logic; only compute/assign next_run_date or set
enabled=False when run_created is True (leave existing commit behavior intact).
This prevents transient _prepare_run failures from advancing the cron schedule
and skipping the due occurrence.
In `@libs/aegra-api/src/aegra_api/services/cron_service.py`:
- Around line 319-328: The current plain SELECT of due crons can return the same
rows to multiple instances; replace it with an atomic claim using an UPDATE ...
RETURNING so rows are reserved before being processed: use SQLAlchemy's
update(CronORM).where(CronORM.enabled.is_(True), CronORM.next_run_date <=
now).values(next_run_date=<new_next_run_value> OR set claimed_at/claimed_by)
.returning(CronORM), execute it with await self.session.execute(...), fetch
returned rows and commit; reference CronORM, self.session.execute (instead of
self.session.scalars), update(), and returning() to locate and implement the
change. Ensure the new_next_run_value or claimed_* fields mark the cron as
claimed so other pollers won't select it.
- Around line 219-244: The patch handler currently only validates timezone when
request.schedule is provided; change it so that when request.timezone is not
None you first validate the timezone and then, if cron.schedule (existing) is
present, recompute and set values["next_run_date"] using
_compute_next_run(schedule, timezone=request.timezone) (raising
HTTPException(422) on invalid timezone), and still merge/update the payload via
_build_payload and existing_payload updates; ensure validation occurs separately
from _is_valid_schedule and that values["payload"] and values["timezone"] (in
payload) are updated consistently when request.timezone is provided even if
request.schedule is absent.
In `@libs/aegra-api/tests/integration/test_health_info.py`:
- Around line 3-21: This test currently constructs FastAPI() and TestClient
directly; replace that with the repo's integration helpers by importing and
using create_test_app() and make_client() from tests/fixtures/clients.py, remove
direct app.include_router(router) and TestClient usage, and create the test
client via make_client(create_test_app()) (or create_test_app() followed by
make_client) so the full integration middleware/dependency stack is exercised;
keep the monkeypatch of settings.cron.CRON_ENABLED and ensure it runs before
creating the client so the flag is reflected in the app under test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2761c5b9-99a5-4703-bbcb-7f2754f080d2
📒 Files selected for processing (13)
examples/cron_example/__init__.pyexamples/cron_example/setup.pylibs/aegra-api/src/aegra_api/core/health.pylibs/aegra-api/src/aegra_api/main.pylibs/aegra-api/src/aegra_api/models/crons.pylibs/aegra-api/src/aegra_api/services/cron_scheduler.pylibs/aegra-api/src/aegra_api/services/cron_service.pylibs/aegra-api/src/aegra_api/settings.pylibs/aegra-api/tests/integration/test_api/test_crons_crud.pylibs/aegra-api/tests/integration/test_health_info.pylibs/aegra-api/tests/unit/test_services/test_cron_scheduler.pylibs/aegra-api/tests/unit/test_services/test_cron_service.pylibs/aegra-api/tests/unit/test_settings.py
✅ Files skipped from review due to trivial changes (3)
- libs/aegra-api/src/aegra_api/models/crons.py
- examples/cron_example/init.py
- libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/cron_example/setup.py
- libs/aegra-api/src/aegra_api/settings.py
Prevent cron schedule advancement when run preparation fails, validate timezone-only cron updates, and atomically claim due crons before firing them.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py`:
- Around line 447-474: The test test_builds_run_create_from_payload is missing
an assertion that verifies the checkpoint field is propagated correctly: cron
payload uses checkpoint_during (in cron_service.py) while
CronScheduler._fire_cron (in cron_scheduler.py) reads payload.get("checkpoint"),
so extend the test to include the checkpoint mapping by adding an assertion on
mock_prepare's run_request (from call_args[0][2]) that run_request.checkpoint
(or the appropriate attribute name used when creating the run) equals the
expected value supplied in the cron payload (e.g., set payload with
"checkpoint_during": True and assert run_request.checkpoint is True) to catch
the regression.
In `@libs/aegra-api/tests/unit/test_services/test_cron_service.py`:
- Line 1146: The test file defines ZoneInfoNotFoundError via an inline import at
line where ZoneInfoNotFoundError is used; move the "from zoneinfo import
ZoneInfoNotFoundError" statement from inside the function to the module-level
import block at the top of test_cron_service.py so the symbol is imported once
for the module (avoid inline imports unless circular or optional), then remove
the local import and reference ZoneInfoNotFoundError directly in the test
function.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14d3f615-0d1a-46e6-b07f-7c9419424410
📒 Files selected for processing (7)
libs/aegra-api/src/aegra_api/services/cron_scheduler.pylibs/aegra-api/src/aegra_api/services/cron_service.pylibs/aegra-api/tests/e2e/test_crons/test_crons.pylibs/aegra-api/tests/fixtures/clients.pylibs/aegra-api/tests/integration/test_health_info.pylibs/aegra-api/tests/unit/test_services/test_cron_scheduler.pylibs/aegra-api/tests/unit/test_services/test_cron_service.py
✅ Files skipped from review due to trivial changes (2)
- libs/aegra-api/tests/fixtures/clients.py
- libs/aegra-api/tests/integration/test_health_info.py
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/aegra-api/tests/e2e/test_crons/test_crons.py
- libs/aegra-api/src/aegra_api/services/cron_scheduler.py
- libs/aegra-api/src/aegra_api/services/cron_service.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py (1)
447-476:⚠️ Potential issue | 🟠 MajorAdd regression coverage for
checkpoint_during→ run request mappingThis test currently verifies only
payload["checkpoint"]. If cron payload creation usescheckpoint_during, this suite won’t catch a silent drop in checkpoint behavior. Please extend this case to assert whichever payload key is canonical across cron creation and scheduler run construction.#!/bin/bash set -euo pipefail echo "== Check checkpoint key usage in scheduler and cron service ==" rg -n --type=py -C3 'checkpoint_during|payload\.get\("checkpoint"\)|checkpoint=' \ libs/aegra-api/src/aegra_api/services/cron_scheduler.py \ libs/aegra-api/src/aegra_api/services/cron_service.py \ libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py echo echo "== Locate RunCreate payload construction sites ==" rg -n --type=py -C4 'RunCreate\(' libs/aegra-api/src/aegra_api/services🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py` around lines 447 - 476, The test only asserts payload["checkpoint"] mapping into the RunCreate but misses the alternate key checkpoint_during; update the test_builds_run_create_from_payload in test_cron_scheduler to include and assert the canonical mapping for checkpoint_during as well (or parametrize to cover both keys) by passing payload with "checkpoint_during": {"checkpoint_id":"abc","checkpoint_ns":""} and/or "checkpoint" and verifying the run_request (obtained from the mocked _prepare_run call in CronScheduler._fire_cron) has run_request.checkpoint equal to the expected dict; ensure you reference the existing mock_prepare/_prepare_run and run_request variables so the test exercises the same code path that constructs RunCreate.
🧹 Nitpick comments (1)
libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py (1)
9-10: Extract async session maker setup to a reusable fixtureThe session context-manager mocking pattern (lines 115–117 and repeated 16 other times) can be consolidated into a shared fixture in
tests/conftest.py:`@pytest.fixture` def mock_async_session_maker() -> Mock: """Mock async session maker for services that call _get_session_maker().""" mock_session = AsyncMock() mock_session.__aenter__ = AsyncMock(return_value=mock_session) mock_session.__aexit__ = AsyncMock(return_value=False) return Mock(return_value=mock_session)Then use it directly in tests:
with patch("aegra_api.services.cron_scheduler._get_session_maker", return_value=mock_async_session_maker):This reduces boilerplate across lines 115–117, 138–140, 170–172, and similar patterns throughout the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py` around lines 9 - 10, Add a reusable pytest fixture named mock_async_session_maker that returns a Mock wrapping an AsyncMock context manager (set __aenter__ to return the async mock and __aexit__ to AsyncMock(return_value=False)), place it in tests conftest so it’s importable, then replace repeated manual context-manager mocks in test_cron_scheduler (and other tests) by importing that fixture and using patch("aegra_api.services.cron_scheduler._get_session_maker", return_value=mock_async_session_maker) in those tests (targets: _get_session_maker and tests that currently set AsyncMock + __aenter__/__aexit__ patterns around lines where session maker is mocked).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py`:
- Around line 447-476: The test only asserts payload["checkpoint"] mapping into
the RunCreate but misses the alternate key checkpoint_during; update the
test_builds_run_create_from_payload in test_cron_scheduler to include and assert
the canonical mapping for checkpoint_during as well (or parametrize to cover
both keys) by passing payload with "checkpoint_during":
{"checkpoint_id":"abc","checkpoint_ns":""} and/or "checkpoint" and verifying the
run_request (obtained from the mocked _prepare_run call in
CronScheduler._fire_cron) has run_request.checkpoint equal to the expected dict;
ensure you reference the existing mock_prepare/_prepare_run and run_request
variables so the test exercises the same code path that constructs RunCreate.
---
Nitpick comments:
In `@libs/aegra-api/tests/unit/test_services/test_cron_scheduler.py`:
- Around line 9-10: Add a reusable pytest fixture named mock_async_session_maker
that returns a Mock wrapping an AsyncMock context manager (set __aenter__ to
return the async mock and __aexit__ to AsyncMock(return_value=False)), place it
in tests conftest so it’s importable, then replace repeated manual
context-manager mocks in test_cron_scheduler (and other tests) by importing that
fixture and using patch("aegra_api.services.cron_scheduler._get_session_maker",
return_value=mock_async_session_maker) in those tests (targets:
_get_session_maker and tests that currently set AsyncMock + __aenter__/__aexit__
patterns around lines where session maker is mocked).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1eabbdb4-56c9-442a-a87c-99a1acaab509
📒 Files selected for processing (2)
libs/aegra-api/tests/unit/test_services/test_cron_scheduler.pylibs/aegra-api/tests/unit/test_services/test_cron_service.py
✅ Files skipped from review due to trivial changes (1)
- libs/aegra-api/tests/unit/test_services/test_cron_service.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add comprehensive cron jobs support to Aegra with full LangGraph SDK compatibility.
Implements CRUD REST API for scheduling agent runs, timezone-aware background scheduler with crash recovery, automatic run lifecycle management, and state persistence for reliable execution.
Type of Change
feat: New featuredocs: Documentation changestest: Tests added/updatedChanges Made
libs/aegra-api/src/aegra_api/api/crons.py): Full REST CRUD operations for cron jobsservices/cron_scheduler.py): Background worker with timezone support, graceful error handling, and crash recoveryservices/cron_service.py): Complete scheduling lifecycle (create, reschedule, automatic cleanup, validation)services/run_cleanup.py): TTL-based cleanup of completed runs with configurable retentioncore/orm.py+ migration): Newcronstable with LangGraph state integrationmodels/crons.py): Pydantic schemas with validationdocs/guides/cron.mdx): Comprehensive guide with examples and best practicesexamples/cron_example/): Working agent with cron setup and configurationTesting
Checklist
make lint)make type-check)make test)Summary by CodeRabbit
New Features
API
Configuration
Examples
Documentation
Tests