Repository navigation
Conversation
…f silently dropping it evaluate disables an exhausted COUNT/UNTIL rule in the same tick that enqueues its final occurrence, and that UPDATE reliably lands before QStash delivers the dispatch. The dispatch handler then bailed on enabled=false with a 200, so the final slot of every bounded automation vanished: no run row, no workspaces.create, no retry, no failure callback — indistinguishable from a cleanly finished series. FREQ=DAILY;COUNT=1 could never fire at all. - evaluate now computes the next occurrence before enqueueing and marks the payload terminal when the rule is exhausted - dispatch lets a terminal payload through the disabled guard when its slot still matches next_run_at (stale replays can't match) - any delivery still dropped on the disabled guard persists a dispatch_failed run row instead of disappearing Fixes #6128
Contributor
📝 WalkthroughWalkthroughThe evaluator now marks exhausted bounded-rule occurrences as terminal before enqueueing them. The dispatch route permits matching terminal deliveries after disablement and records other disabled deliveries as failed runs. Tests cover terminal, non-terminal, invalid, stale, and disabled cases. ChangesBounded automation dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EvaluatorRoute
participant AutomationDatabase
participant QStash
participant DispatchRoute
EvaluatorRoute->>AutomationDatabase: read due automations
EvaluatorRoute->>EvaluatorRoute: compute occurrence and terminal status
EvaluatorRoute->>AutomationDatabase: disable exhausted bounded automation
EvaluatorRoute->>QStash: enqueue payload with terminal flag
QStash->>DispatchRoute: deliver scheduled payload
DispatchRoute->>AutomationDatabase: read automation and nextRunAt
DispatchRoute->>DispatchRoute: match terminal slot or record disabled skip
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plans/6128-scheduled-dispatch-loss.md`:
- Line 1: Move the shipped plan document 6128-scheduled-dispatch-loss.md from
plans/ to plans/done/, preserving its filename and contents.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b94e989-2788-4e17-81a4-9890a4458414
📒 Files selected for processing (5)
apps/api/src/app/api/automations/dispatch/[id]/route.test.tsapps/api/src/app/api/automations/dispatch/[id]/route.tsapps/api/src/app/api/automations/evaluate/route.test.tsapps/api/src/app/api/automations/evaluate/route.tsplans/6128-scheduled-dispatch-loss.md
| @@ -0,0 +1,77 @@ | |||
| # #6128: scheduled dispatch silently lost on the terminal slot of bounded rules | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move this shipped plan to plans/done/.
This document states that the fix shipped in this branch. Move it to plans/done/6128-scheduled-dispatch-loss.md.
As per coding guidelines, “move shipped plans to plans/done/.”
🤖 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 `@plans/6128-scheduled-dispatch-loss.md` at line 1, Move the shipped plan
document 6128-scheduled-dispatch-loss.md from plans/ to plans/done/, preserving
its filename and contents.
Source: Coding guidelines
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/app/api/automations/dispatch/[id]/route.ts">
<violation number="1" location="apps/api/src/app/api/automations/dispatch/[id]/route.ts:82">
P1: Pausing a bounded automation after its final job is enqueued can still launch that job, because user-driven disable leaves `nextRunAt` at the matching terminal slot. Persist distinct exhaustion state or invalidate the terminal slot when disabling so this bypass applies only to evaluate’s automatic disable.</violation>
</file>
<file name="apps/api/src/app/api/automations/dispatch/[id]/route.test.ts">
<violation number="1" location="apps/api/src/app/api/automations/dispatch/[id]/route.test.ts:14">
P3: The QStash signature gate is hard-mocked to `async () => true`, so the route's 401 security paths (missing signature, verify() returning false) and the 400 invalid-payload path have zero coverage. Since this endpoint's entire trust model is signature verification, add a test that makes the verify mock resolve `false` (and one for a missing header) asserting a 401 and that `dispatchAutomation` is not called.</violation>
<violation number="2" location="apps/api/src/app/api/automations/dispatch/[id]/route.test.ts:126">
P3: The stale-terminal-replay test only asserts `insertedRuns` has length 1, not that the row is `dispatch_failed` with the expected `scheduledFor`/`error`. A regression that persisted a `dispatched`, `running`, or wrong-slot row would still pass this test. Assert the row content like the disabled-skip test does.</violation>
</file>
<file name="plans/6128-scheduled-dispatch-loss.md">
<violation number="1" location="plans/6128-scheduled-dispatch-loss.md:34">
P3: The stated safety rationale is inaccurate: pausing an automation does not move next_run_at. In setEnabled (packages/trpc/src/router/automation/automation.ts) the nextRunAt patch is applied only on resume; disabling an already-exhausted automation leaves next_run_at at the terminal occurrence, so a stale terminal replay still passes the bucketToMinute(nextRunAt)==scheduledFor guard and re-runs dispatchAutomation. The real protection against double-dispatch is the run-row uniqueness constraint (onConflictDoNothing), not this guard as documented — correct the rationale so maintainers don't rely on the guard alone.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Cron as Scheduler (cron)
participant Ev as EvaluateRoute
participant DB as Database
participant QS as QStash
participant Dp as DispatchRoute
participant DA as dispatchAutomation (TRPC)
participant Relay as Relay (external)
Note over Ev,DB: Evaluate tick: fetch due automations
Cron->>Ev: POST /api/automations/evaluate
Ev->>DB: Fetch due automations (nextRunAt <= now)
DB-->>Ev: Due automation rows
loop Process each due automation
Ev->>Ev: Compute next occurrence (try/catch)
alt computeError (unparseable rrule)
Ev->>QS: Enqueue job with terminal=false (NEW: always non-terminal)
Ev->>Ev: Log advance failure (no DB update)
else next === null (exhausted bounded rule)
Ev->>QS: Enqueue job with terminal=true (NEW: terminal flag)
Ev->>DB: Update automation set enabled=false (CHANGED: still disables, but terminal flag communicates exhaustion)
else next exists (normal advance)
Ev->>QS: Enqueue job with terminal=false (non-terminal)
Ev->>DB: Update automation set nextRunAt=next
end
end
Note over QS,Dp: QStash delivers scheduled job (may be before or after DB update)
QS->>Dp: POST /api/automations/dispatch/{id} (payload: terminal, scheduledFor)
Dp->>DB: Load automation by id
DB-->>Dp: Automation row (enabled, nextRunAt)
alt automation.enabled
Dp->>DA: dispatchAutomation(automation, scheduledFor)
DA->>Relay: Trigger workspace create (part of automation)
DA->>DB: Insert run row (success)
else automation disabled
alt terminal == true AND bucketToMinute(nextRunAt) == scheduledFor
Note over Dp,DA: NEW: terminal slot matches next_run_at → exhaustion, not user pause
Dp->>DA: dispatchAutomation(automation, scheduledFor) (CHANGED: allow even if disabled)
DA->>Relay: Trigger workspace create
DA->>DB: Insert run row (success)
else
Note over Dp,DB: NEW: drop on disabled guard now records a failed run
Dp->>DB: Insert dispatch_failed run row ("automation disabled before dispatch delivery")
Dp-->>QS: Return 200 {ok:true, skipped:"disabled"}
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // next_run_at moves on any other transition (advance, pause/resume | ||
| // recompute), so a stale terminal replay can't match. | ||
| const isTerminalSlot = | ||
| parsed.data.terminal && |
Contributor
There was a problem hiding this comment.
P1: Pausing a bounded automation after its final job is enqueued can still launch that job, because user-driven disable leaves nextRunAt at the matching terminal slot. Persist distinct exhaustion state or invalidate the terminal slot when disabling so this bypass applies only to evaluate’s automatic disable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/app/api/automations/dispatch/[id]/route.ts, line 82:
<comment>Pausing a bounded automation after its final job is enqueued can still launch that job, because user-driven disable leaves `nextRunAt` at the matching terminal slot. Persist distinct exhaustion state or invalidate the terminal slot when disabling so this bypass applies only to evaluate’s automatic disable.</comment>
<file context>
@@ -55,15 +69,54 @@ export async function POST(
+ // next_run_at moves on any other transition (advance, pause/resume
+ // recompute), so a stale terminal replay can't match.
+ const isTerminalSlot =
+ parsed.data.terminal &&
+ bucketToMinute(automation.nextRunAt).getTime() === scheduledFor.getTime();
+ if (!isTerminalSlot) {
</file context>
|
|
||
| mock.module("@upstash/qstash", () => ({ | ||
| Receiver: class { | ||
| verify = mock(async () => true); |
Contributor
There was a problem hiding this comment.
P3: The QStash signature gate is hard-mocked to async () => true, so the route's 401 security paths (missing signature, verify() returning false) and the 400 invalid-payload path have zero coverage. Since this endpoint's entire trust model is signature verification, add a test that makes the verify mock resolve false (and one for a missing header) asserting a 401 and that dispatchAutomation is not called.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/app/api/automations/dispatch/[id]/route.test.ts, line 14:
<comment>The QStash signature gate is hard-mocked to `async () => true`, so the route's 401 security paths (missing signature, verify() returning false) and the 400 invalid-payload path have zero coverage. Since this endpoint's entire trust model is signature verification, add a test that makes the verify mock resolve `false` (and one for a missing header) asserting a 401 and that `dispatchAutomation` is not called.</comment>
<file context>
@@ -0,0 +1,152 @@
+
+mock.module("@upstash/qstash", () => ({
+ Receiver: class {
+ verify = mock(async () => true);
+ },
+}));
</file context>
| const json = (await response.json()) as { skipped?: string }; | ||
| expect(json.skipped).toBe("disabled"); | ||
| expect(dispatchAutomation).not.toHaveBeenCalled(); | ||
| expect(insertedRuns).toHaveLength(1); |
Contributor
There was a problem hiding this comment.
P3: The stale-terminal-replay test only asserts insertedRuns has length 1, not that the row is dispatch_failed with the expected scheduledFor/error. A regression that persisted a dispatched, running, or wrong-slot row would still pass this test. Assert the row content like the disabled-skip test does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/app/api/automations/dispatch/[id]/route.test.ts, line 126:
<comment>The stale-terminal-replay test only asserts `insertedRuns` has length 1, not that the row is `dispatch_failed` with the expected `scheduledFor`/`error`. A regression that persisted a `dispatched`, `running`, or wrong-slot row would still pass this test. Assert the row content like the disabled-skip test does.</comment>
<file context>
@@ -0,0 +1,152 @@
+ const json = (await response.json()) as { skipped?: string };
+ expect(json.skipped).toBe("disabled");
+ expect(dispatchAutomation).not.toHaveBeenCalled();
+ expect(insertedRuns).toHaveLength(1);
+ expect(insertedRuns[0]).toMatchObject({
+ automationId: AUTOMATION_ID,
</file context>
| - `dispatch` lets a disabled automation through when the payload is terminal | ||
| AND `bucketToMinute(next_run_at)` still equals `scheduledFor` (the disable | ||
| path leaves `next_run_at` at the terminal occurrence; any other transition | ||
| moves it, so stale replays can't match). |
Contributor
There was a problem hiding this comment.
P3: The stated safety rationale is inaccurate: pausing an automation does not move next_run_at. In setEnabled (packages/trpc/src/router/automation/automation.ts) the nextRunAt patch is applied only on resume; disabling an already-exhausted automation leaves next_run_at at the terminal occurrence, so a stale terminal replay still passes the bucketToMinute(nextRunAt)==scheduledFor guard and re-runs dispatchAutomation. The real protection against double-dispatch is the run-row uniqueness constraint (onConflictDoNothing), not this guard as documented — correct the rationale so maintainers don't rely on the guard alone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/6128-scheduled-dispatch-loss.md, line 34:
<comment>The stated safety rationale is inaccurate: pausing an automation does not move next_run_at. In setEnabled (packages/trpc/src/router/automation/automation.ts) the nextRunAt patch is applied only on resume; disabling an already-exhausted automation leaves next_run_at at the terminal occurrence, so a stale terminal replay still passes the bucketToMinute(nextRunAt)==scheduledFor guard and re-runs dispatchAutomation. The real protection against double-dispatch is the run-row uniqueness constraint (onConflictDoNothing), not this guard as documented — correct the rationale so maintainers don't rely on the guard alone.</comment>
<file context>
@@ -0,0 +1,77 @@
+- `dispatch` lets a disabled automation through when the payload is terminal
+ AND `bucketToMinute(next_run_at)` still equals `scheduledFor` (the disable
+ path leaves `next_run_at` at the terminal occurrence; any other transition
+ moves it, so stale replays can't match).
+- Any delivery still dropped on the disabled guard now persists a
+ `dispatch_failed` run row (`automation disabled before dispatch delivery`)
</file context>
Contributor
|
This PR has had no activity for 53 days and will close in 7 days unless there is new activity. If this contribution is still important or relevant, please leave a comment to keep it open. If it closes, you are welcome to reopen it or ask a maintainer to reopen it. Thank you for contributing! |
Contributor
|
This PR was automatically closed due to inactivity to help keep the review queue manageable. This does not mean the contribution was rejected. If this contribution is still important or relevant, please reopen it or leave a comment asking a maintainer to reopen it. Thank you for contributing! |
This branch was previously deployed
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
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.
Summary
Fixes #6128 — the terminal occurrence of every bounded (
COUNT/UNTIL) automation was silently lost:evaluateflipsenabled:falsefor an exhausted rule in the same tick that enqueues its final dispatch, the disable UPDATE beats QStash's HTTP delivery, and the dispatch handler bailed onenabled=falsewith a 200. Result: no run row, noworkspaces.create, no retry/failure callback, and an automation that reads like a cleanly finished series.FREQ=DAILY;COUNT=1could never fire. Run-now was unaffected becauserunNowcallsdispatchAutomationdirectly with no enabled check.Changes
evaluatecomputes the next occurrence before enqueueing and setsterminal: trueon the payload when the rule is exhausted (unparseable rrules keep today's behavior: enqueue non-terminal + log the advance failure).dispatch/[id]lets a disabled automation through when the payload is terminal andbucketToMinute(next_run_at)still equalsscheduledFor— exhaustion leavesnext_run_atat the terminal occurrence while every other transition moves it, so stale replays can't match.dispatch_failedrun row (automation disabled before dispatch delivery) instead of vanishing.Verification
next dev+ isolated Neon branch, real enqueue→signed delivery):COUNT=1: handler hit, 200skipped:"disabled", 0 run rows,enabled:false— bug reproduced;COUNT=1: terminal slot persists a run row, then clean disable;COUNT=2(MINUTELY, two ticks): both slots persist rows incl. the terminal one;dispatch_failedrow persisted.bun run lintandbun run typecheckexit 0.Not exercised e2e: relay→
workspaces.createhappy path (no live host locally); that'sdispatchAutomation, shared byte-for-byte with the proven manual path and unchanged here.Remaining product decisions (status enum instead of overloading
enabled, retry/backfill for missed slots, relay presence accuracy #6014/#6160) are written up inplans/6128-scheduled-dispatch-loss.md.Summary by cubic
Fixes #6128 by ensuring the terminal slot of bounded (
COUNT/UNTIL) automations is dispatched instead of being dropped, and by recording a run when a disabled guard stops delivery. This restores expected behavior for cases likeFREQ=DAILY;COUNT=1.terminal: truewhen the rule is exhausted; unparseable rrules still enqueue withterminal: falseand log the advance failure. Uses@upstash/qstashbatch enqueue.bucketToMinute(next_run_at)equalsscheduledFor; otherwise persist adispatch_failedrun row (“automation disabled before dispatch delivery”) instead of silently dropping. Calls@superset/trpc/automation-dispatch.Written for commit 73d281c. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation