Skip to content

fix(schedules): locking schedules to prevent double runs - #1854

Merged
icecrasher321 merged 3 commits into
stagingfrom
fix/schedules-locking
Nov 8, 2025
Merged

fix(schedules): locking schedules to prevent double runs#1854
icecrasher321 merged 3 commits into
stagingfrom
fix/schedules-locking

Conversation

@icecrasher321

Copy link
Copy Markdown
Collaborator

Summary

Schedules queuing and execution should be in a locked section.

Type of Change

  • Bug fix

Testing

Tested manually by running cron

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercelBot commented Nov 8, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentPreviewCommentsUpdated (UTC)
docsSkippedSkippedNov 8, 2025 3:39am

@greptile-appsgreptile-appsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR implements optimistic locking for schedule execution to prevent double runs. The approach uses a new lastQueuedAt timestamp column that gets set when schedules are queued (line 30 in route.ts), and the query condition ensures only schedules where lastQueuedAt < nextRunAt or lastQueuedAt IS NULL are selected (lines 38-40).

Critical Issue Found:

  • The lock (lastQueuedAt) is never released after execution completes, which will cause all schedules to stop running after their first execution
  • Missing lastQueuedAt: null in three places: successful execution (line 650), failed execution (line 682), and error catch block (line 727)

Positive Changes:

  • Major code refactoring improves maintainability with well-named helper functions
  • Proper use of UPDATE...RETURNING pattern provides atomic locking at database level
  • Schema change is clean and appropriate

Confidence Score: 0/5

  • Critical bug will break all schedule functionality after first execution
  • The locking mechanism is incomplete - lastQueuedAt is set during queuing but never cleared after execution completes. This means every schedule will run exactly once and then become permanently locked, breaking the entire scheduling system. This is a production-breaking bug that must be fixed before merge.
  • apps/sim/background/schedule-execution.ts requires immediate attention - three missing lastQueuedAt: null updates will break all schedules

Important Files Changed

File Analysis

FilenameScoreOverview
apps/sim/app/api/schedules/execute/route.ts3/5Implements optimistic locking via UPDATE with RETURNING for schedule queueing, but has a critical issue where lock is never released in execution phase
apps/sim/background/schedule-execution.ts2/5Major refactor adds helper functions and lock release logic, but missing lock release on successful execution path creates deadlock potential
packages/db/schema.ts5/5Added lastQueuedAt column to support optimistic locking mechanism for schedule execution

Sequence Diagram

sequenceDiagram
participant Cron
participant RouteAPI as Execute Route
participant DB as Database
participant Executor as Schedule Executor
Cron->>RouteAPI: GET /api/schedules/execute
RouteAPI->>DB: UPDATE workflow_schedule<br/>SET lastQueuedAt = now<br/>WHERE nextRunAt <= now<br/>AND (lastQueuedAt IS NULL OR lastQueuedAt < nextRunAt)<br/>RETURNING *
DB-->>RouteAPI: Return locked schedules
Note over RouteAPI,DB: Optimistic locking:<br/>Only schedules not already queued
loop For each schedule
RouteAPI->>Executor: Queue execution job
Executor->>Executor: Check rate limits & usage
Executor->>Executor: Load workflow & execute
alt Success
Executor->>DB: UPDATE schedule<br/>SET lastRanAt = now,<br/>nextRunAt = calculated,<br/>failedCount = 0<br/>❌ MISSING: lastQueuedAt = null
Note over Executor,DB: BUG: Lock never released!
else Failure
Executor->>DB: UPDATE schedule<br/>SET failedCount++,<br/>nextRunAt = calculated<br/>❌ MISSING: lastQueuedAt = null
else Error
Executor->>DB: UPDATE schedule<br/>SET failedCount++<br/>❌ MISSING: lastQueuedAt = null
end
end
RouteAPI-->>Cron: Return success
Loading

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment threadapps/sim/background/schedule-execution.ts
Comment threadapps/sim/background/schedule-execution.ts
Comment threadapps/sim/background/schedule-execution.ts
@icecrasher321
icecrasher321 merged commit 7a8d47a into stagingNov 8, 2025
9 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/schedules-locking branch November 8, 2025 23:27
waleedlatif1 pushed a commit that referenced this pull request Nov 9, 2025
* fix(schedules): locking schedules to prevent double runs
* add migration file
* fix
waleedlatif1 pushed a commit that referenced this pull request Nov 12, 2025
* fix(schedules): locking schedules to prevent double runs
* add migration file
* fix
@waleedlatif1waleedlatif1 mentioned this pull request Nov 12, 2025
10 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@icecrasher321