Skip to content

fix: disable RunQueue Worker in priority tests to prevent partial-batch race#3440

Merged
matt-aitken merged 2 commits intomainfrom
fix-priority-test-flakiness
Apr 24, 2026
Merged

fix: disable RunQueue Worker in priority tests to prevent partial-batch race#3440
matt-aitken merged 2 commits intomainfrom
fix-priority-test-flakiness

Conversation

@matt-aitken
Copy link
Copy Markdown
Member

Summary

  • The processMasterQueueForEnvironment call in the priority test was racing against background processQueueForWorkerQueue jobs scheduled 50ms after each trigger
  • With a 50ms debounce (processWorkerQueueDebounceMs: 50) and runs triggered sequentially, the RunQueue Worker could process those jobs mid-sequence, pushing partial batches to the worker queue in the wrong overall priority order
  • masterQueueConsumersDisabled: true only blocks the shard-level polling loops — it does not prevent the RunQueue's own Worker from processing these debounced jobs
  • Fix: add worker.disabled: true to the test 1 engine config, which propagates to workerOptions.disabled in the RunQueue constructor and prevents the Worker from starting

Test plan

  • Both priority tests pass: pnpm run test ./src/engine/tests/priority.test.ts --run
  • Test 1 log confirms no ✅ Starting run engine worker or worker loop messages — workers fully disabled
  • Test 2 unaffected (uses master queue consumers for automatic promotion, no disabled flag added)

🤖 Generated with Claude Code

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 24, 2026

⚠️ No Changeset found

Latest commit: 1fe1d5b

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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 923e0be8-d22b-401a-8de7-89f67c07359d

📥 Commits

Reviewing files that changed from the base of the PR and between 027b712 and 1fe1d5b.

📒 Files selected for processing (1)
  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
  • GitHub Check: sdk-compat / Cloudflare Workers
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: sdk-compat / Bun Runtime
  • GitHub Check: sdk-compat / Deno Runtime
  • GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: audit
  • GitHub Check: Analyze (python)
  • GitHub Check: Analyze (actions)
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use function declarations instead of default exports

Add crumbs as you write code using // @Crumbs comments or `// `#region` `@crumbs blocks. These are temporary debug instrumentation and must be stripped using agentcrumbs strip before merge.

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use vitest for all tests in the Trigger.dev repository

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)

**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}

📄 CodeRabbit inference engine (AGENTS.md)

Format code using Prettier before committing

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptive describe and it blocks
Tests should avoid mocks or stubs and use the helpers from @internal/testcontainers when Redis or Postgres are needed
Use vitest for running unit tests

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
internal-packages/run-engine/src/engine/tests/**/*.test.ts

📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)

Implement tests for RunEngine in src/engine/tests/ using testcontainers for Redis and PostgreSQL containerization

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.{ts,tsx}: Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
Place test files next to source files with naming pattern MyService.ts -> MyService.test.ts.
For Redis/PostgreSQL tests in vitest, use testcontainers helpers: redisTest, postgresTest, or containerTest imported from @internal/testcontainers.

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
**/*.ts{,x}

📄 CodeRabbit inference engine (CLAUDE.md)

Always import from @trigger.dev/sdk when writing Trigger.dev tasks. Never use @trigger.dev/sdk/v3 or deprecated client.defineJob.

Files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
🧠 Learnings (25)
📓 Common learnings
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: internal-packages/database/prisma/schema.prisma:666-666
Timestamp: 2026-04-16T14:21:18.496Z
Learning: In `triggerdotdev/trigger.dev`, the `BackgroundWorkerTask` covering index on `(runtimeEnvironmentId, slug, triggerSource)` lives in `internal-packages/database/prisma/migrations/20260413000000_add_bwt_covering_index/migration.sql` as a `CREATE INDEX CONCURRENTLY IF NOT EXISTS`, intentionally in its own migration file separate from the `TaskIdentifier` table migration. Do not flag this index as missing from the schema migrations in future reviews.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3299
File: internal-packages/run-engine/src/run-queue/index.ts:683-699
Timestamp: 2026-03-31T06:25:47.761Z
Learning: In `internal-packages/run-engine/src/run-queue/index.ts`, the fast-path enqueue (when `enableFastPath` is true) intentionally skips the TTL sorted set. The `expireRun` worker job is scheduled independently *before* `enqueueMessage` is called (in `engine.trigger()`), so TTL expiry is handled regardless of which path the message takes. The slow-path dequeue Lua removes from the TTL set on dequeue (not on enqueue), making the fast-path skip equivalent. Do not suggest adding a `!ttlInfo` guard to the fast-path check — it would prevent nearly all production runs from using the fast path since dev environments default to a 10m TTL.
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/{app/v3/services/triggerTask.server.ts,app/v3/services/batchTriggerV3.server.ts} : In `triggerTask.server.ts` and `batchTriggerV3.server.ts`, do NOT add database queries. Task defaults (TTL, etc.) are resolved via `backgroundWorkerTask.findFirst()` in the queue concern (`queues.server.ts`). Piggyback on the existing query instead of adding new ones
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/app/v3/services/queues.server.ts : If adding a new task-level default, add it to the existing `select` clause in the `backgroundWorkerTask.findFirst()` query in `queues.server.ts` — do NOT add a second query. If the default doesn't need to be known at trigger time, resolve it at dequeue time instead
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3166
File: internal-packages/run-engine/src/batch-queue/tests/index.test.ts:711-713
Timestamp: 2026-03-03T13:07:33.177Z
Learning: In `internal-packages/run-engine/src/batch-queue/tests/index.test.ts`, test assertions for rate limiter stubs can use `toBeGreaterThanOrEqual` rather than exact equality (`toBe`) because the consumer loop may call the rate limiter during empty pops in addition to actual item processing, and this over-calling is acceptable in integration tests.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Focus on Run Engine 2.0 code in `internal/run-engine` for new development; legacy run engine code is being deprecated
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: For V1/V2 branching in services within `apps/webapp/app/v3/`, only modify V2 code paths. All new work uses Run Engine 2.0 (`internal/run-engine`) and redis-worker, not legacy V1 code (MarQS queue, triggerTaskV1, cancelTaskRunV1, etc.).
📚 Learning: 2026-03-02T12:43:25.254Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/tests/**/*.test.ts : Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-03T13:07:33.177Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3166
File: internal-packages/run-engine/src/batch-queue/tests/index.test.ts:711-713
Timestamp: 2026-03-03T13:07:33.177Z
Learning: In `internal-packages/run-engine/src/batch-queue/tests/index.test.ts`, test assertions for rate limiter stubs can use `toBeGreaterThanOrEqual` rather than exact equality (`toBe`) because the consumer loop may call the rate limiter during empty pops in addition to actual item processing, and this over-calling is acceptable in integration tests.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-31T06:25:47.761Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3299
File: internal-packages/run-engine/src/run-queue/index.ts:683-699
Timestamp: 2026-03-31T06:25:47.761Z
Learning: In `internal-packages/run-engine/src/run-queue/index.ts`, the fast-path enqueue (when `enableFastPath` is true) intentionally skips the TTL sorted set. The `expireRun` worker job is scheduled independently *before* `enqueueMessage` is called (in `engine.trigger()`), so TTL expiry is handled regardless of which path the message takes. The slow-path dequeue Lua removes from the TTL set on dequeue (not on enqueue), making the fast-path skip equivalent. Do not suggest adding a `!ttlInfo` guard to the fast-path check — it would prevent nearly all production runs from using the fast path since dev environments default to a 10m TTL.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-04-15T15:39:31.575Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2026-04-15T15:39:31.575Z
Learning: Focus on Run Engine 2.0 code in `internal/run-engine` for new development; legacy run engine code is being deprecated

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-02T12:43:25.254Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/systems/**/*.ts : Integrate OpenTelemetry tracer and meter instrumentation in RunEngine systems for observability

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-04-16T13:45:22.317Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/test/engine/taskIdentifierRegistry.test.ts:3-19
Timestamp: 2026-04-16T13:45:22.317Z
Learning: In `apps/webapp/test/engine/taskIdentifierRegistry.test.ts`, the `vi.mock` calls for `~/services/taskIdentifierCache.server` (stubbing `getTaskIdentifiersFromCache` and `populateTaskIdentifierCache`), `~/models/task.server` (stubbing `getAllTaskIdentifiers`), and `~/db.server` (stubbing `prisma` and `$replica`) are intentional. The suite uses real Postgres via testcontainers for all `TaskIdentifier` DB operations, but isolates the Redis cache layer and legacy query fallback as separate concerns not exercised in this test file. Do not flag these mocks as violations of the no-mocks policy in future reviews.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2025-11-27T16:26:44.496Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/executing-commands.mdc:0-0
Timestamp: 2025-11-27T16:26:44.496Z
Learning: For running tests, navigate into the package directory and run `pnpm run test --run` to enable single-file test execution (e.g., `pnpm run test ./src/engine/tests/ttl.test.ts --run`)

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-04-07T14:12:18.946Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/test/engine/batchPayloads.test.ts:5-24
Timestamp: 2026-04-07T14:12:18.946Z
Learning: In `apps/webapp/test/engine/batchPayloads.test.ts`, using `vi.mock` for `~/v3/objectStore.server` (stubbing `hasObjectStoreClient` and `uploadPacketToObjectStore`), `~/env.server` (overriding offload thresholds), and `~/v3/tracer.server` (stubbing `startActiveSpan`) is intentional and acceptable. Simulating controlled transient upload failures (e.g., fail N times then succeed) to verify `p-retry` behavior cannot be reproduced with real services or testcontainers. This file is an explicit exception to the repo's general no-mocks policy.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/app/v3/services/{cancelTaskRun,batchTriggerV3}.server.ts : When editing services that branch on `RunEngineVersion` to support both V1 and V2 (e.g., `cancelTaskRun.server.ts`, `batchTriggerV3.server.ts`), only modify V2 code paths

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-31T06:25:47.761Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3299
File: internal-packages/run-engine/src/run-queue/index.ts:683-699
Timestamp: 2026-03-31T06:25:47.761Z
Learning: In `internal-packages/run-engine/src/run-queue/index.ts`, message scores passed to `enqueueMessage` are always <= now (computed as `(queueTimestamp || createdAt) - priorityMs`). Future-scored messages (nack retries) never go through `enqueueMessage`; they go directly through `nackMessage` → ZADD to the queue sorted set. Do not flag fast-path enqueue for allowing future-timestamped messages.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-02T12:43:43.173Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/redis-worker/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:43.173Z
Learning: Applies to packages/redis-worker/**/redis-worker/src/worker.ts : Worker loop and job processing should implement concurrency control in src/worker.ts

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-04-13T21:44:00.032Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/services/taskIdentifierRegistry.server.ts:24-67
Timestamp: 2026-04-13T21:44:00.032Z
Learning: In `apps/webapp/app/services/taskIdentifierRegistry.server.ts`, the sequential upsert/updateMany/findMany writes in `syncTaskIdentifiers` are intentionally NOT wrapped in a Prisma transaction. This function runs only during deployment-change events (low-concurrency path), and any partial `isInLatestDeployment` state is acceptable because it self-corrects on the next deployment. Do not flag this as a missing-transaction/atomicity issue in future reviews.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-03T13:08:03.862Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3166
File: packages/redis-worker/src/fair-queue/index.ts:1114-1121
Timestamp: 2026-03-03T13:08:03.862Z
Learning: In packages/redis-worker/src/fair-queue/index.ts, it's acceptable for the worker queue depth cap check to allow overshooting by up to batchClaimSize messages per iteration, as the next iteration will recheck and prevent sustained growth beyond the limit.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-22T19:27:29.014Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts:104-112
Timestamp: 2026-03-22T19:27:29.014Z
Learning: In `apps/webapp/app/v3/services/alerts/createAlertChannel.server.ts`, the `#scheduleErrorAlertEvaluation` helper intentionally uses the same job id (`evaluateErrorAlerts:${projectId}`) as the evaluator's periodic self-chain. The deduplication is desired: if a future run is already queued, the immediate enqueue becomes a no-op, preventing two evaluations firing in quick succession. Do not flag this as a bug or suggest a unique/timestamped id.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-14T10:24:37.515Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3219
File: internal-packages/run-engine/src/run-queue/index.ts:774-777
Timestamp: 2026-03-14T10:24:37.515Z
Learning: In `internal-packages/run-engine/src/run-queue/index.ts`, checking `message.concurrencyKey` to branch CK vs non-CK logic is safe and intentional. The `concurrencyKey` field and the `:ck:` suffix in `message.queue` are always set atomically during the same enqueue call (`queueKey = this.keys.queueKey(env, message.queue, concurrencyKey)`). In `enqueueMessage`, `concurrencyKey` is the source of truth that creates the `:ck:` queue name (checking the queue name would be circular). In `#callAcknowledgeMessage`, `#callNackMessage`, and `#callMoveToDeadLetterQueue`, the `OutputPayload` is a single serialized JSON blob containing both fields — a message with `:ck:` in the queue name but a missing `concurrencyKey` cannot exist. Do not suggest replacing `message.concurrencyKey` checks with queue-name-derived predicates.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-04-16T14:19:16.330Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: apps/webapp/CLAUDE.md:0-0
Timestamp: 2026-04-16T14:19:16.330Z
Learning: Applies to apps/webapp/app/v3/services/queues.server.ts : If adding a new task-level default, add it to the existing `select` clause in the `backgroundWorkerTask.findFirst()` query in `queues.server.ts` — do NOT add a second query. If the default doesn't need to be known at trigger time, resolve it at dequeue time instead

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-02T12:43:43.173Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/redis-worker/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:43.173Z
Learning: Applies to packages/redis-worker/**/redis-worker/src/queue.ts : Job queue abstraction should be Redis-backed in src/queue.ts

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-02T12:43:17.177Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/database/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:17.177Z
Learning: New code should always target Prisma RunEngineVersion V2 (run-engine + redis-worker), not V1 (legacy MarQS + Graphile)

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-02T12:43:43.173Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/redis-worker/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:43.173Z
Learning: Applies to packages/redis-worker/**/*@(job|queue|worker|background).{ts,tsx} : Use trigger.dev/redis-worker for all new background job implementations, replacing graphile-worker and zodworker

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-02T12:43:25.254Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Use Redis for distributed locks and queues in the RunEngine system via `RunQueue` and `RunLocker` utilities

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-04-15T15:39:06.868Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: For V1/V2 branching in services within `apps/webapp/app/v3/`, only modify V2 code paths. All new work uses Run Engine 2.0 (`internal/run-engine`) and redis-worker, not legacy V1 code (MarQS queue, triggerTaskV1, cancelTaskRunV1, etc.).

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.

Applied to files:

  • internal-packages/run-engine/src/engine/tests/priority.test.ts
🔇 Additional comments (2)
internal-packages/run-engine/src/engine/tests/priority.test.ts (2)

20-50: LGTM — targeted fix for the priority ordering race.

Setting worker.disabled: true here cleanly addresses the race: combined with masterQueueConsumersDisabled: true and processWorkerQueueDebounceMs: 50, the RunQueue Worker will no longer pick up the 50 ms‑debounced processQueueForWorkerQueue jobs mid‑sequence, so the test can drive promotion deterministically via the explicit processMasterQueueForEnvironment call on line 80. This propagates to workerOptions.disabled in the RunQueue constructor and also prevents the engine worker from starting, matching the intended behavior.


130-158: Confirm disabled: true in Test 2 is intentional — contradicts PR description.

The PR objectives state the fix only sets worker.disabled: true in the Test 1 engine config and that "Test 2 is unchanged and continues to use master queue consumers for automatic promotion." However, Test 2 here also received disabled: true. Functionally this should still be fine: masterQueueConsumersDisabled is a separate option and defaults to enabled here, so master queue consumer shards keep polling and auto‑promotion should still occur during the setTimeout(500) wait. That said, disabling the RunQueue's internal worker removes one of the paths that can drive processQueueForWorkerQueue, so please confirm Test 2 still reliably dequeues all 5 items via the master queue consumer loop alone (no flakiness under load).

If the change to Test 2 was intentional, consider updating the PR description; if not, revert this line to keep Test 2 exercising the full worker path.


Walkthrough

Both test cases in the priority test suite now construct RunEngine with worker.disabled: true, changing the runtime worker setup used during those tests. The rest of each test's logic—run triggering, dequeuing loops, timestamp/priority ordering assertions, and queue processing calls—remains unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix—disabling the RunQueue Worker in priority tests to prevent a race condition causing partial-batch ordering issues.
Description check ✅ Passed The PR description provides detailed explanation of the race condition, root cause analysis, and the fix applied, though some required template sections (Checklist, Testing, Changelog) are incomplete or missing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-priority-test-flakiness

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

Comment @coderabbitai help to get the list of available commands and usage tips.

devin-ai-integration[bot]

This comment was marked as resolved.

@matt-aitken matt-aitken merged commit f7aefb7 into main Apr 24, 2026
41 checks passed
@matt-aitken matt-aitken deleted the fix-priority-test-flakiness branch April 24, 2026 11:03
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.

2 participants