feat: add automation scheduler timers - #984
Conversation
Astro-Han
added
enhancement
|
Warning Review limit reached
More reviews will be available in 19 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a pluggable AutomationScheduler with a Clock/TaskRuntime, extends run helpers to accept explicit timing and track runIDs, wires the scheduler into instance/server/tool lifecycle paths, and adds comprehensive FakeClock-driven tests for one-shot, recurring, missed, and cancellation behaviors. ChangesAutomation Scheduler Implementation
Sequence DiagramsequenceDiagram
participant Clock
participant Scheduler as AutomationScheduler
participant Automation as Automation Runtime
participant Executor as Run Executor
Clock->>Scheduler: sleep finishes / timer fires
Scheduler->>Automation: evaluate schedule & active run
alt Active run awaiting input
Scheduler->>Automation: recordStoppedRun(previous_run_awaiting_input)
Automation-->>Scheduler: stopped run recorded
else No active run
Scheduler->>Executor: fork unattended execution (AbortSignal)
Executor->>Automation: execute run and publish terminal state
Automation-->>Scheduler: run finished event
end
alt Recurring and eligible
Scheduler->>Scheduler: computeNextFireAt and schedule next via Clock.sleep
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an AutomationScheduler to manage and execute scheduled automation runs (both one-shot and recurring interval-based), integrating it with the existing automation routes and adding comprehensive unit tests. The review feedback highlights three key issues in the scheduler implementation: first, using triggeredAt instead of clock.now() for scheduling the next run could cause a rapid cascade of timers if the system falls behind; second, because running is initialized to true, the start() method's early return guard prevents existing automations from being scheduled on startup; and third, one-shot automations lack a completion state, which may lead to duplicate executions if the scheduler is restarted or updated after they have already fired.
fd9c3b8 to
3527751
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/src/automation/scheduler.ts (2)
60-81: ⚡ Quick winAdd defensive error handling for deleted automations.
The
fire()function callsAutomation.runNowExecuting(automationID, ...)at line 67 without catching potential errors. If the automation is deleted between the timer firing (line 61) and therunNowExecutingcall,Automation.get(id)(called withinrunNowat line 571 of index.ts) will throwNotFoundError, resulting in an unhandled promise rejection.While the proper deletion flow calls
scheduler.cancel(automationID)before deletion, a narrow race window exists where the timer callback executes after deletion completes.Wrap the
runNowExecutingcall in a try-catch block to gracefully handle this edge case:🛡️ Defensive error handling
const fire = (automationID: string, triggeredAt: number) => { timers.delete(automationID) if (Automation.hasActiveRun(automationID)) { const stopped = Automation.recordStoppedRun(automationID, "previous_run_awaiting_input", { now: triggeredAt }) void Automation.publishRunUpdated(stopped) return } + try { Automation.runNowExecuting(automationID, { executor: async (input) => { try { return await executor(input) } finally { const latest = Automation.get(input.definition.id) if (latest.kind === "recurring" && latest.rhythm.kind === "interval" && !latest.paused) { schedule(latest, clock.now() + latest.rhythm.everyMs) } } }, attendance: "unattended", now: triggeredAt, }) + } catch (error) { + // Automation was deleted; timer already removed, nothing to do + } }🤖 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 `@packages/opencode/src/automation/scheduler.ts` around lines 60 - 81, The timer callback fire should defensively handle the case where the automation was deleted between timers.delete and invoking Automation.runNowExecuting: wrap the call to Automation.runNowExecuting(automationID, ...) in a try/catch, catch NotFoundError (or the error thrown by Automation.get) and silently return (or log at debug) instead of letting the promise reject; keep the existing finally behavior inside the executor intact and only suppress the specific deletion-related error while rethrowing or propagating other unexpected errors.
40-45: 💤 Low valueConsider renaming for clarity.
The function name
nextFireAtcomputes "next fire time from a given point," but for interval automations it simply returnsfrom + everyMswithout considering the definition'snextFireAtfield. While the usage is correct (callers pass completion time orclock.now()asfrom), the name might mislead readers into expecting the function to return the pre-computeddefinition.nextFireAtfor recurring automations.Consider renaming to
computeNextFireAtorcalculateFireTimeto better convey that this is a calculation helper rather than a field accessor.🤖 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 `@packages/opencode/src/automation/scheduler.ts` around lines 40 - 45, Rename the helper function nextFireAt to a clearer verb-named function like computeNextFireAt (or calculateFireTime) to indicate it computes a value rather than returning a stored field; update the exported function name export function nextFireAt(definition: Automation.Definition, from: number): number | null to export function computeNextFireAt(...) and update all call sites to use computeNextFireAt, leaving logic unchanged (still handling definition.paused, oneshot via definition.fireAt, interval via from + definition.rhythm.everyMs) and keeping references to definition.nextFireAt intact where callers intentionally use the stored field.
🤖 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.
Nitpick comments:
In `@packages/opencode/src/automation/scheduler.ts`:
- Around line 60-81: The timer callback fire should defensively handle the case
where the automation was deleted between timers.delete and invoking
Automation.runNowExecuting: wrap the call to
Automation.runNowExecuting(automationID, ...) in a try/catch, catch
NotFoundError (or the error thrown by Automation.get) and silently return (or
log at debug) instead of letting the promise reject; keep the existing finally
behavior inside the executor intact and only suppress the specific
deletion-related error while rethrowing or propagating other unexpected errors.
- Around line 40-45: Rename the helper function nextFireAt to a clearer
verb-named function like computeNextFireAt (or calculateFireTime) to indicate it
computes a value rather than returning a stored field; update the exported
function name export function nextFireAt(definition: Automation.Definition,
from: number): number | null to export function computeNextFireAt(...) and
update all call sites to use computeNextFireAt, leaving logic unchanged (still
handling definition.paused, oneshot via definition.fireAt, interval via from +
definition.rhythm.everyMs) and keeping references to definition.nextFireAt
intact where callers intentionally use the stored field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de879331-b65d-4e15-b598-bdf7c7843167
📒 Files selected for processing (5)
packages/opencode/src/automation/index.tspackages/opencode/src/automation/scheduler.tspackages/opencode/src/server/instance/automation.tspackages/opencode/test/server/automation-routes.test.tspackages/opencode/test/server/automation-scheduler.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/src/automation/scheduler.ts (1)
171-179: ⚡ Quick win
waitUntilloop ignores the abort signal.The wait loop only checks
clock.now() < fireAt. Whensignalis aborted (cancel/reschedule),liveClock.sleepresolves immediately, so the loop keeps re-sleeping and correctness depends entirely on theTaskRuntimealso interrupting the fiber. A pluggable runtime that only aborts the signal (theTaskRuntimecontract only guaranteesinterrupt()) would spin tightly. Separately, the post-loop guardtasks.has(automationID)can match a newly scheduled task after a reschedule, letting a stale fiber callfire. Gate onsignal.abortedto make abort authoritative.🔒️ Proposed fix
Effect.gen(function* () { while (clock.now() < fireAt) { + if (signal.aborted) return const delayMs = Math.max(0, fireAt - clock.now()) yield* Effect.promise(() => clock.sleep(Math.min(delayMs, MAX_TIMER_DELAY_MS), signal)) } - if (!running || !tasks.has(automationID)) return + if (!running || signal.aborted || !tasks.has(automationID)) return fire(automationID, fireAt) })🤖 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 `@packages/opencode/src/automation/scheduler.ts` around lines 171 - 179, The waitUntil loop ignores the AbortSignal so a cancelled/rescheduled task can spin or let a stale fiber call fire; update waitUntil (and its loop condition) to abort early by checking signal.aborted in the while condition and after waking, and before invoking fire(automationID, fireAt) also verify the signal is not aborted and that the currently scheduled task still matches this timer (e.g. tasks.has(automationID) AND the task's scheduled time equals fireAt) so a rescheduled task won't be overwritten; adjust references in waitUntil, clock.sleep, running, tasks.has/get, and fire accordingly to make the AbortSignal authoritative.
🤖 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 `@packages/opencode/src/project/instance.ts`:
- Line 130: The call to stop helpers like stopCurrentOwnedRuns (via scheduler())
can throw synchronously and should not abort disposal; wrap the stop invocation
in a best-effort try/catch so teardown still proceeds and errors are logged.
Locate the three sites where scheduler().stopCurrentOwnedRuns,
scheduler().stopDirectoryOwnedRuns, and scheduler().stopAllOwnedRuns are invoked
(e.g., inside disposeInstance/disposeDirectory/disposeAllLoadedInstances) and
change each to call the stop helper inside a try { await (or call) scheduler();
/*call stop helper*/ } catch (err) { logger.error(...) } (ensure you use the
existing logger/processLogger and continue disposal) so any synchronous throw is
caught and cleanup always runs.
---
Nitpick comments:
In `@packages/opencode/src/automation/scheduler.ts`:
- Around line 171-179: The waitUntil loop ignores the AbortSignal so a
cancelled/rescheduled task can spin or let a stale fiber call fire; update
waitUntil (and its loop condition) to abort early by checking signal.aborted in
the while condition and after waking, and before invoking fire(automationID,
fireAt) also verify the signal is not aborted and that the currently scheduled
task still matches this timer (e.g. tasks.has(automationID) AND the task's
scheduled time equals fireAt) so a rescheduled task won't be overwritten; adjust
references in waitUntil, clock.sleep, running, tasks.has/get, and fire
accordingly to make the AbortSignal authoritative.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 3a4a12da-14b7-4af2-9b99-cc999a901030
📒 Files selected for processing (7)
packages/opencode/src/automation/index.tspackages/opencode/src/automation/scheduler.tspackages/opencode/src/project/instance.tspackages/opencode/src/server/instance/automation.tspackages/opencode/src/tool/automate.tspackages/opencode/test/server/automation-routes.test.tspackages/opencode/test/server/automation-scheduler.test.ts
Closes the backend gaps left after PR1-5 so the PR6/PR7 frontend slice can
build on a complete, stable automation contract.
Goal / change boundary:
- model { providerID, modelID } now required on every AutomationDefinition;
runner passes it through so runs no longer depend on the runtime model
fallback chain (which drifts across restarts).
- variant? optional reasoning/effort, validated against the live provider
catalog on create/update; invalid variant returns 422 instead of failing
late in the run.
- stop.kind === "condition" rejected at create/update with structured
{ field: "stop", message: "unsupported_stop_condition" } (scheduler never
schedules condition stops, so accepting them only leaked dead UI surface).
- Derived fields nextFireAt / nextFires / failureStreak populated at
create/update and refreshed after every terminal run; scheduler re-publishes
automation.definition.updated with a bumped revision for global-sync.
- cron validation consolidated into src/automation/cron.ts; scheduler and
derived both consume it.
- automate tool schema picks up model/variant + Provider-backed create-time
validation.
- Migration 20260601100000 drops pre-release rows so model can be NOT NULL.
Verification:
- CI green on 5e177ba (unit-opencode flake on an unrelated VCS-routes
20s timeout cleared on rerun).
- Local: 129 automation tests (routes/runner/scheduler/event-fixtures/tool/
cron) + 35 session processor tests pass.
Review follow-ups addressed:
- codex: scheduler self-loop guard keyed by id:revision (not id alone).
- P2: recordRunOutcome retries on ConflictError instead of silently dropping
the run outcome.
- CodeRabbit: 422 create test uses a guaranteed-invalid model; fixed sleeps
replaced with terminal-state polling.
- Test seam for the ConflictError path moved off the public Automation API
into an internal __test_hooks module.
Residual risk / deferred (tracked, not blocking PR6):
- needs_user_input / loop_gate run-error codes remain reserved; producing
them needs prompt-loop semantics changes.
- Session.automationID reverse lookup deferred (session-contract migration).
- stop=condition kept in the schema layer for SDK shape parity but rejected
at validate time; collapses once a condition evaluator lands.
Linked: issue #950; follows PR #960 #983 #984 #998 #1004; unblocks PR6/PR7.
1 participant
Summary
Adds the PR3 backend scheduler slice for issue #950: a per-instance in-memory automation scheduler service for one-shot and interval-after-completion definitions, scheduled unattended execution, and no-overlap skip recording.
The scheduler now owns one interruptible Effect-backed task per scheduled automation, tracks the unattended runs it starts so shutdown can cancel them, and applies recurring stop rules that are safe for this in-memory slice. Native timers are contained behind the scheduler clock/sleep adapter instead of being the service architecture.
Why
PR1 froze the automation contract and PR2 made manual
runNowexecute safely. This PR adds background firing with the smallest useful runtime behavior while keeping the scheduler shape aligned with the broader supervised backend service design. Persistence, cron, durable locks, worktree placement, frontend, and tool-unhide remain separate later slices.Related Issue
Closes part of #950: PR3 scheduler timers + ownership + unattended policy.
Human Review Status
Pending
Review Focus
Please focus on the scheduler ownership boundary, interruptible wait/run lifecycle, long-delay waiting, completed one-shot rescheduling, recurring no-overlap behavior, recurring stop=count handling, condition-stop non-scheduling until an evaluator exists, and recurring interval anchoring after either scheduled or manual run completion.
Risk Notes
The scheduler is intentionally in-memory and per app instance for this slice. It is now modeled as a supervised scheduler service with cancelable per-definition tasks and cancelable scheduler-owned unattended runs, while the actual waiting remains process-local. Durable cross-window locking, persistence, cron, missed-run scan, restart reconciliation, and durable
nextFireAt/nextFiresstate are deferred to later slices per the frozen plan.For this PR,
nextFireAt/nextFiresremain contract fields and are not a source of truth for the in-memory scheduler queue. The real scheduler ownership lives inside the per-instance scheduler.Condition-based recurring stops are accepted by the frozen contract, but PR3 does not include a condition evaluator. To avoid turning an unevaluated condition into an infinite unattended loop, condition-stop definitions are not scheduled by this in-memory scheduler until that evaluator slice exists.
Skipped checklist items:
How To Verify
Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes
Tests