fix(api): reject trigger schedules that fire below a frequency floor - #6279
fix(api): reject trigger schedules that fire below a frequency floor#6279mmabrouk wants to merge 1 commit into
Conversation
Every schedule fire starts an agent run in its own sandbox, so a '* * * * *' schedule bills 1440 runs a day and, because a run routinely outlives a minute, overlaps itself. Nothing stopped one: _validate_schedule only checked that the expression parsed as five fields. Add a minimum interval, defaulting to 15 minutes and overridable via AGENTA_TRIGGERS_SCHEDULE_MIN_INTERVAL_MINUTES. Both create and edit already route through _validate_schedule, so both are covered, and the 422 names the floor and the offending cadence. The web mirrors the check in validateSchedule so the drawer refuses the value before submitting, and Save is now gated on a valid cron in edit mode too, not only on create. Enforced on write only: schedules stored before this keep firing until edited.
dosubot
Bot
added
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds a configurable minimum cron firing interval. Backend validation scans upcoming occurrences. Frontend validation mirrors the default 15-minute floor. Schedule forms now block invalid schedules in create and edit flows. Tests cover cadence detection, boundaries, errors, and configuration. ChangesSchedule frequency-floor validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to A narrow client-side validation gap allows some leap-day schedules below the 15-minute floor to appear valid and reach the API, where they should still be rejected. The impact is limited to inconsistent validation and user experience, but the leap-day case should receive explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant ScheduleForm
participant validateSchedule
participant TriggerAPI
participant _validate_schedule
ScheduleForm->>validateSchedule: validate cron expression and cadence
validateSchedule-->>ScheduleForm: return client validation result
ScheduleForm->>TriggerAPI: submit valid schedule
TriggerAPI->>_validate_schedule: validate configured frequency floor
_validate_schedule-->>TriggerAPI: accept or reject schedule
TriggerAPI-->>ScheduleForm: return creation result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ad3c2e6-99d2-48b1-a46d-60cf56ff834b
📒 Files selected for processing (10)
api/oss/src/core/triggers/service.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/acceptance/triggers/test_triggers_schedules.pyapi/oss/tests/pytest/unit/triggers/test_triggers_schedule_frequency_floor.pyapi/oss/tests/pytest/unit/triggers/test_triggers_schedules_refresh.pyweb/packages/agenta-entities/src/gatewayTrigger/core/cron.tsweb/packages/agenta-entities/src/gatewayTrigger/index.tsweb/packages/agenta-entities/tests/unit/gatewayTriggerCron.test.tsweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/ScheduleBuilderField.tsxweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/schedule/ScheduleForm.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
| export function smallestCronGapMinutes(expression: string): number | null { | ||
| const runs = nextCronRuns(expression, GAP_SAMPLE_COUNT) | ||
| if (runs.length < 2) return null |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Detect leap-day schedules before accepting them.
nextCronRuns stops after 366 days. With time frozen at 2026-03-01, 0,1 0 29 2 * fires twice one minute apart on February 29, 2028. This function returns null, so validateSchedule accepts a schedule below the cadence floor. Extend cadence sampling to reach valid leap-day occurrences without an unbounded minute scan. Add this case as a regression test.
|
Railway Preview Environment
|
1 participant
The problem
A trigger schedule accepted any cron expression that parsed,
* * * * *included. Nothing checked how often it fired.Every fire starts an agent run in its own Daytona sandbox, so an every-minute schedule bills 1440 runs a day. It is also wrong on its own terms: an agent run routinely takes longer than a minute, so the schedule launches a new run before the previous one finishes and overlaps itself.
This is not hypothetical. A test schedule named "test every minute" was created on staging on 22 August. It ran roughly $12/day and exhausted the shared Daytona organization's credits the next day, which took down agent runs on every environment sharing that account, including a colleague's PR testing on
preview.demo.Here was the entire validation:
The change
A minimum interval between two consecutive fires, defaulting to 15 minutes and overridable per deployment via
AGENTA_TRIGGERS_SCHEDULE_MIN_INTERVAL_MINUTES.POST /triggers/schedules/with* * * * *Schedule may run at most once every 15 minutes, but this one runs every 1 minute.*/5 * * * *Backend.
_validate_scheduleis the single gate, and bothcreate_scheduleandedit_schedulealready call it first, so both paths are covered by one change. The 422 detail names the floor and the actual cadence so the message is actionable rather than "invalid cron".Web.
validateSchedulein the shared cron module adds the same cadence check on top of the existing field-shape check, so the drawer rejects the value without a round-trip. The backend stays the source of truth and its message still surfaces throughtriggerApiErrorMessageif the two ever disagree.One extra fix found on the way:
canSubmitgated edits onisDirtyalone, never on cron validity. Save looked enabled for an invalid expression in edit mode and only bounced offhandleSubmitwith a toast. It is now gated on a valid cron in both modes.Measuring the gap correctly
The interesting part is that comparing two consecutive fires is not enough.
0,59 * * * *fires at:00,:59,:00— the tight gap is the second one.0,1 0 31 1 *only ever fires on 31 January. Scanning from "now" would find nothing at all, so the scan anchors on the expression's first fire, not on the clock.Both sides walk fires from the first one, take the smallest gap, and bail early once a gap falls below the floor, so a runaway expression costs two samples rather than a full window.
Compatibility
Enforced on write only. Schedules stored before this keep firing until someone edits them, so no existing automation breaks on deploy. The four active schedules on staging are daily and weekly and sit well clear of the floor.
The web mirrors the backend default as
MIN_CRON_INTERVAL_MINUTES. A deployment that lowers the backend floor must lower that constant too, or the drawer will refuse a value the API would accept. Exposing the floor over the API would remove the duplication; there is no config endpoint for the web to read today, and adding one felt out of scope here.Tests
api/oss/tests/pytest/unit/triggers/test_triggers_schedule_frequency_floor.py— new. Pins the gap arithmetic, the accept/reject boundary (*/15accepted,*/14rejected), the early exit, the env override, the message text, and that a never-firing expression like0 0 30 2 *does not raise.gatewayTriggerCron.test.ts— the same cases on the web side.*/5to*/30, and a unit test's "a valid cron is accepted" case likewise.Local runs: 172 passed, 4 skipped (
api/oss/tests/pytest/unit/triggers/, the skips are Postgres DAO tests with no database). 35 passed (gatewayTriggerCron.test.ts).ruff format/ruff checkclean,eslint/prettierclean,tsc --noEmitclean on both touched packages.Follow-ups, not in this PR
code-toolbox-language: python, the SDK default. Adding stage, project, and trigger id would have made the incident above self-explaining.