feat: implement cron window spread backend - #4566
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds schedule windows with duration and percentage formats, validation, persistence, and API presentation. The schedule engine now calculates nominal and effective schedule times, derives deterministic phases, and propagates both timestamps through scheduling and triggering. Task runs now store nullable queue timestamps in ClickHouse. Queue handling records queue wait time and prevents fast-path delivery for future messages. Tests cover schedule timing, recovery, queue timestamps, and future queue eligibility. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Observability mapAs of 19/100 over 417 measured of 433 entry points (base 18, up 1) What this PR changed
FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const firstNominalAt = calculateNextNominalTimestamp(schedule, timezone, afterNominal); | ||
| const firstOccurrence = occurrenceAt(firstNominalAt); | ||
|
|
||
| if (firstOccurrence.effectiveAt.getTime() >= now.getTime()) { | ||
| return { ...firstOccurrence, skippedExpiredOccurrences: false }; | ||
| } |
There was a problem hiding this comment.
🔴 A deploy while a spread schedule is waiting to run silently cancels that run
A scheduled occurrence that is already waiting inside its spread delay is thrown away and replaced by the following occurrence (calculateNextSchedulableOccurrence early-return at internal-packages/schedule-engine/src/engine/scheduleCalculation.ts:94-99) whenever the schedule is re-registered during the delay, so that occurrence never runs.
Impact: With cron spreading enabled, deploying an app while a spread schedule is in its delay window silently skips that scheduled run.
Why re-registration during the delay window drops the pending occurrence
With spreading enabled, occurrence N is enqueued to fire at effectiveAt_N = nominalAt_N + offset, which can be up to a full interval (or the configured window, e.g. 30 minutes) after nominalAt_N.
syncDeclarativeSchedules (apps/webapp/app/v3/services/createBackgroundWorker.server.ts:723,756) calls registerNextTaskScheduleInstance({ instanceId }) on every deploy, with no fromTimestamp. registerNextTaskScheduleInstance then uses fromTimestamp = registrationTime (internal-packages/schedule-engine/src/engine/index.ts:216-238).
If now falls in (nominalAt_N, effectiveAt_N):
firstNominalAt = next(now) = nominalAt_{N+1}firstOccurrence.effectiveAt >= now, so the function returns immediately atscheduleCalculation.ts:97-99and never consults thelatestNominalAtbranch (scheduleCalculation.ts:103-115) that would have found occurrenceNstill eligible.
enqueueScheduledTask then re-enqueues under the same job id scheduled-task-instance:${instanceId}, and SimpleQueue.enqueue (packages/redis-worker/src/queue.ts:107-131) overwrites the existing entry rather than skipping it. The pending occurrence N job is destroyed and never fires.
Before this PR the delay window was effectively zero (the job was always available before its nominal time), so next(now) resolved to the same pending occurrence and nothing was lost.
The latestNominalAt check appears to already encode the right rule; it just needs to be evaluated before accepting firstOccurrence when firstNominalAt skipped past a still-eligible earlier tick.
Prompt for agents
In calculateNextSchedulableOccurrence (internal-packages/schedule-engine/src/engine/scheduleCalculation.ts), the fast path returns the first nominal occurrence after `afterNominal` as soon as its effective time is in the future. When `afterNominal` defaults to `now` (every external caller: deploy sync via syncDeclarativeSchedules, schedule upsert, manual registration), and the previously-registered occurrence is currently sitting inside its spread delay window (now is after its nominal tick but before its effective time), `next(now)` resolves to the FOLLOWING nominal tick. registerNextTaskScheduleInstance then re-enqueues under the same worker job id, and redis-worker's SimpleQueue.enqueue overwrites the pending job — so the in-delay occurrence is silently dropped and never fires. With a 30m window on an hourly cron, roughly half of all deploys would drop a scheduled run.
The existing `latestNominalAt` branch (using previousScheduledTimestamp(now + 1ms)) already computes exactly the occurrence that should win in this situation, but it is only reached when the first occurrence has already expired. Consider evaluating the latest nominal tick whenever it is strictly after `afterNominal`, is not after `now`, and its effective time is still upcoming — preferring it over the later `firstNominalAt` — so that re-registration during a delay window preserves the pending occurrence. Add coverage for: afterNominal == now, now strictly between nominalAt and effectiveAt, spreading enabled.
Was this helpful? React with 👍 or 👎 to provide feedback.
| generatorDescription: cronstrue.toString(options.cron), | ||
| timezone: options.timezone ?? "UTC", | ||
| externalId: options.externalId ? options.externalId : null, | ||
| ...normalizeScheduleWindow(options.window), |
There was a problem hiding this comment.
🟡 Editing a schedule in the dashboard silently removes its configured delay window
A schedule's saved delay window is wiped out (normalizeScheduleWindow(options.window) at apps/webapp/app/v3/services/upsertTaskSchedule.server.ts:166) whenever the schedule is saved from the dashboard, because the dashboard form never carries the window value.
Impact: A window configured through the API is silently lost the first time someone edits that schedule in the dashboard.
Form submission path
The dashboard create/edit routes parse the form body with the UpsertSchedule zod schema (apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.schedules.new/route.tsx:73) and pass the result straight to UpsertTaskScheduleService. The form has no window field, so options.window is always undefined, and normalizeScheduleWindow(undefined) returns { windowDurationSeconds: null, windowPercentage: null }, which the update writes over the existing values.
This also flips scheduleHasChanged (apps/webapp/app/v3/services/upsertTaskSchedule.server.ts:172-174) and re-registers all instances, so timing changes immediately.
A fix could either keep the existing window when the caller omits the field on the dashboard path (e.g. an explicit window: undefined vs. "not provided" distinction), or surface the window in the form so it round-trips.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (let i = 0; i < count; i++) { | ||
| nextScheduledTimestamp = calculateNextScheduledTimestamp( | ||
| cron, | ||
| timezone, | ||
| nextScheduledTimestamp | ||
| ); | ||
| nextScheduledTimestamp = calculateNextNominalTimestamp(cron, timezone, nextScheduledTimestamp); | ||
|
|
There was a problem hiding this comment.
🔍 upcoming timestamps can now include past dates on a late fire
nextScheduledTimestamps now chains purely off calculateNextNominalTimestamp instead of calculateNextScheduledTimestamp, dropping the old "if the computed slot is in the past, recompute from now" fallback. For the dashboard/API callers that seed from new Date() this is identical. Inside the engine, though, upcoming is generated from exactScheduleTime (internal-packages/schedule-engine/src/engine/index.ts:538-543), which with spreading enabled is the nominal tick and can be well in the past by the time the job actually fires. ScheduledTaskPayload.upcoming may therefore contain timestamps that have already elapsed, which previously could not happen. Arguably more consistent (strict nominal chaining), but it is a user-visible payload change worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const queueWaitMs = | ||
| typeof message.message.eligibleAtMs === "number" | ||
| ? Math.max(0, Date.now() - message.message.eligibleAtMs) | ||
| : undefined; |
There was a problem hiding this comment.
🔍 Queue-wait metrics will absorb the intentional spread delay
eligibleAtMs is stamped at enqueue time (internal-packages/run-engine/src/engine/systems/enqueueSystem.ts:155 uses Date.now() unless anchorEligibilityAtQueuePosition). For a spread schedule the run is enqueued at trigger time (up to a full window before its effective time), so the new queue_wait_ms span attribute and the existing wait field in run-queue/index.ts:1018-1019 will report the deliberate spread delay as queue backlog. If these feed latency dashboards/alerts, enabling the flag will look like a queue regression.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Add the backend plumbing for cron schedule spreading/delaying.
Changes
queueTimestampfor the "effectiveAt" delayed start time, propagate it to Clickhouse TaskRun tableFlag
When switched on:
TODO: