Skip to content

[v4] Don’t allowed canceled delayed runs to be put into the queue #1981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 24, 2025

Conversation

matt-aitken
Copy link
Member

@matt-aitken matt-aitken commented Apr 24, 2025

If you canceled a delayed run (before the delay date) then it was "Canceled". But then it got queued and executed as normal, as if you hadn't canceled it in the first place…

Created a RunEngine test to reproduce the issue. Then fixed it.

Summary by CodeRabbit

  • New Features

    • Added the ability to prevent delayed runs from being enqueued or executed if they are cancelled before starting.
    • Introduced a status check to identify if a run is in its initial state.
  • Bug Fixes

    • Cancelling a delayed run now reliably prevents it from being queued or executed after the delay period.
  • Tests

    • Added a test to verify that cancelling a delayed run keeps it from being queued or executed after cancellation.

Copy link

changeset-bot bot commented Apr 24, 2025

⚠️ No Changeset found

Latest commit: 46493b9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Apr 24, 2025

Walkthrough

This change enhances the run engine's handling of delayed runs by integrating the DelayedRunSystem into the RunAttemptSystem. It introduces logic to prevent delayed runs from being enqueued if they are cancelled before execution. The update includes a new status helper function, adjustments to Prisma queries to access delay information, and a new test to verify that cancelling a delayed run ensures it cannot be queued or executed after cancellation.

Changes

File(s) Change Summary
src/engine/index.ts Passes the delayedRunSystem instance to the RunAttemptSystem constructor within RunEngine.
src/engine/statuses.ts Adds isInitialState function to check if a run execution status is "RUN_CREATED".
src/engine/systems/delayedRunSystem.ts Adds preventDelayedRunFromBeingEnqueued method to acknowledge and prevent a delayed run from being enqueued by the worker.
src/engine/systems/runAttemptSystem.ts Integrates DelayedRunSystem into RunAttemptSystem; updates constructor and options; modifies cancelRun and startRunAttempt to prevent enqueuing cancelled delayed runs; updates Prisma selects for delayUntil.
src/engine/tests/delays.test.ts Adds a test case verifying that cancelling a delayed run prevents it from being queued or executed after the delay period.

Sequence Diagram(s)

Loading
sequenceDiagram
    participant User
    participant RunEngine
    participant RunAttemptSystem
    participant DelayedRunSystem
    participant Worker

    User->>RunEngine: Cancel delayed run
    RunEngine->>RunAttemptSystem: cancelRun(runId)
    RunAttemptSystem->>Prisma: Fetch run (includes delayUntil)
    alt Run is in initial state and delayed
        RunAttemptSystem->>DelayedRunSystem: preventDelayedRunFromBeingEnqueued(runId)
        DelayedRunSystem->>Worker: ack(jobId)
    end
    RunAttemptSystem-->>RunEngine: Run cancelled

Possibly related PRs

  • triggerdotdev/trigger.dev#1804: Introduces and modifies the release concurrency system within the run engine, including changes to RunAttemptSystem and integration with DelayedRunSystem.

Poem

In the warren where code and logic run,
We’ve taught our system a clever new pun—
If a run’s delayed but cancelled in time,
It won’t hop to the queue, nor cross the finish line.
With checks and tests, the rabbits cheer,
No more ghostly runs will appear!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e9c6cc7 and 46493b9.

📒 Files selected for processing (5)
  • internal-packages/run-engine/src/engine/index.ts (1 hunks)
  • internal-packages/run-engine/src/engine/statuses.ts (1 hunks)
  • internal-packages/run-engine/src/engine/systems/delayedRunSystem.ts (1 hunks)
  • internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (5 hunks)
  • internal-packages/run-engine/src/engine/tests/delays.test.ts (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal-packages/run-engine/src/engine/statuses.ts (1)
packages/core/src/v3/schemas/runEngine.ts (2)
  • TaskRunExecutionStatus (6-16)
  • TaskRunExecutionStatus (18-19)
internal-packages/run-engine/src/engine/systems/delayedRunSystem.ts (2)
internal-packages/run-engine/src/engine/index.ts (1)
  • runId (1160-1314)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (2)
  • runId (1085-1194)
  • runId (1208-1229)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (3)
internal-packages/run-engine/src/engine/systems/systems.ts (1)
  • SystemResources (9-17)
internal-packages/run-engine/src/engine/systems/delayedRunSystem.ts (1)
  • DelayedRunSystem (14-138)
internal-packages/run-engine/src/engine/statuses.ts (1)
  • isInitialState (39-42)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: units / 🧪 Unit Tests
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
internal-packages/run-engine/src/engine/statuses.ts (1)

39-42: Concise and well-structured helper function.

This helper function follows the existing pattern in the file and serves a clear purpose of identifying runs in their initial state. This is essential for the new feature to prevent cancelled delayed runs from being enqueued.

internal-packages/run-engine/src/engine/systems/delayedRunSystem.ts (1)

135-137: Critical feature for handling cancelled delayed runs.

This method prevents delayed runs from being enqueued by acknowledging the job that would have scheduled them. The implementation is simple but effective, using the worker's ack method with a consistent job ID format.

internal-packages/run-engine/src/engine/index.ts (1)

306-306: Good integration of DelayedRunSystem into RunAttemptSystem.

This change enables the integration of the delayed run cancellation feature by providing the RunAttemptSystem with access to the DelayedRunSystem instance, allowing it to prevent delayed runs from being enqueued after cancellation.

internal-packages/run-engine/src/engine/tests/delays.test.ts (1)

294-403: Excellent test coverage for cancelled delayed runs.

This test thoroughly verifies that cancelling a delayed run prevents it from being enqueued or executed after its delay expires. The test:

  1. Creates a run with a delay
  2. Verifies it's in DELAYED status
  3. Cancels the run
  4. Verifies it's properly cancelled
  5. Waits for the original delay to expire
  6. Confirms the run is still cancelled and not enqueued
  7. Attempts to dequeue and verifies nothing is dequeued
  8. Final verification of cancelled state

This comprehensive test ensures the new feature works as expected across all involved systems.

internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (7)

25-25: Import required for delayed run cancellation feature.

The additional import of isInitialState is used to check if a run is in its initial state before preventing delayed runs from being enqueued.


35-35: Import DelayedRunSystem for integration.

Adding the DelayedRunSystem import is necessary for the integration of delayed run handling within the RunAttemptSystem.


42-42: Add DelayedRunSystem dependency to options interface.

Properly extends the RunAttemptSystemOptions interface to include the delayedRunSystem as a required dependency.


52-52: Add private field for DelayedRunSystem.

This private field will store the instance of DelayedRunSystem injected via the constructor.


59-59: Initialize DelayedRunSystem in constructor.

Correctly initializes the delayedRunSystem field from the options provided to the constructor.


975-975: Include delayUntil in TaskRun data selection.

Including the delayUntil field in the select query is necessary to check if a cancelled run is delayed and needs to be prevented from executing.


994-998:

✅ Verification successful

Core fix: Prevent cancelled delayed runs from being enqueued.

This is the key implementation that addresses the issue where cancelled delayed runs were still being executed. The code checks if:

  1. The run is in its initial state (using the isInitialState helper)
  2. The run has a scheduled delay time

If both conditions are met, it prevents the delayed run from being enqueued, effectively fixing the issue described in the PR.


🏁 Script executed:

#!/bin/bash
# Verify that the isInitialState function is being used correctly
ast-grep --pattern $'export function isInitialState(status: TaskRunExecutionStatus): boolean {
  $$$
  return $$$;
}'

Length of output: 526


🏁 Script executed:

#!/bin/bash
# Display lines around the snippet in runAttemptSystem.ts for better context
sed -n '900,1050p' internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts

Length of output: 5233


Prevent canceled delayed runs from being enqueued

The new check in internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts ensures that if a run is still in its initial state (RUN_CREATED) and has a delayUntil timestamp, we invoke

await this.delayedRunSystem.preventDelayedRunFromBeingEnqueued({ runId });

to stop any canceled delayed runs from entering the queue.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Sorry, something went wrong.

@matt-aitken matt-aitken merged commit bb606a1 into main Apr 24, 2025
12 checks passed
@matt-aitken matt-aitken deleted the fix-delayed-canceled-still-queued branch April 24, 2025 14:10
Sign up for free to 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.

None yet

2 participants