feat(webapp): add skipIfActive trigger option for drop-on-conflict dedup#3433
feat(webapp): add skipIfActive trigger option for drop-on-conflict dedup#3433eni9889 wants to merge 1 commit intotriggerdotdev:mainfrom
skipIfActive trigger option for drop-on-conflict dedup#3433Conversation
…dedup `skipIfActive: boolean` is a new option on `tasks.trigger()` (and batch items). When set, and at least one `tag` is supplied, the webapp checks Postgres for an in-flight (non-terminal) TaskRun with the same `taskIdentifier` + environment + all-of the supplied tags. If one exists, the trigger is a no-op: the existing run is returned with `isCached: true` + new `wasSkipped: true` flag, and no new run is created. Why: current options don't cover the "cron scanner drops duplicates" pattern. - `idempotencyKey` caches successful completions too -> scheduled retriggers blocked until TTL expires. - `concurrencyKey` queues duplicates FIFO -> backlog grows when a sync hangs; dashboard fills with redundant QUEUED rows. - Manual `runs.list()` dedup in user code is expensive (ClickHouse `task_runs_v2 FINAL + hasAny(tags)` under load) and gets reinvented poorly in every downstream project. Implementation follows the `IdempotencyKeyConcern` pattern: - `SkipIfActiveConcern.handleTriggerRequest()` does one indexed SELECT on `"TaskRun"` (GIN `runTags_idx` + composite status/env index). - Invoked in `RunEngineTriggerTaskService` AFTER idempotency (explicit idempotency match wins) but BEFORE run creation (skipped triggers never touch the queue). - Route surfaces `wasSkipped: true` in the response. Additive — older SDK clients ignore unknown field. Scope: - Engine v2 only (the V1 `TriggerTaskServiceV1` path is not touched; new code in v1 is frozen upstream). - Per-item honor in batch trigger via the same per-item concern call. - No schema migration; reuses existing indexes. Docs: `docs/skip-if-active.mdx` compares with `idempotencyKey` / `concurrencyKey` and documents the interaction matrix. Tests: unit tests for `SkipIfActiveConcern` covering no-flag, no-tags, no-match, single-match, string-tag normalization, and the row-disappeared-mid-query race. Refs: triggerdotdev#3428 (related context for cron-scanner load patterns that motivated this option).
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughThis pull request introduces a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes DetailsKey ChangesNew Core Logic:
Service Integration:
Type & Schema Updates:
Testing & Documentation:
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
|
Hi @eni9889, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| vi.mock("~/services/logger.server", () => ({ | ||
| logger: { | ||
| debug: vi.fn(), | ||
| info: vi.fn(), | ||
| warn: vi.fn(), | ||
| error: vi.fn(), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🔴 Test file uses mocks and spies, violating mandatory repository testing rules
The test file apps/webapp/test/engine/skipIfActiveConcern.test.ts extensively uses vi.mock (line 3), vi.fn() (lines 5–9, 50–51), and hand-rolled mock objects for the Prisma client, directly violating the mandatory testing rules in ai/references/tests.md (referenced by AGENTS.md). Those rules explicitly state: "Do not mock anything", "Do not use mocks in tests", "Do not use spies in tests", "Do not use stubs in tests", "Do not use fakes in tests". The AGENTS.md further states: "Tests should avoid mocks or stubs and use the helpers from @internal/testcontainers when Redis or Postgres are needed." The test should use postgresTest from @internal/testcontainers to run against a real Postgres database, similar to how apps/webapp/test/engine/triggerTask.test.ts uses containerTest for its core assertions.
Prompt for agents
The test file apps/webapp/test/engine/skipIfActiveConcern.test.ts uses vi.mock for the logger (line 3) and vi.fn() throughout to build mock Prisma clients. This violates the mandatory repo rules in ai/references/tests.md which say 'Do not mock anything' and 'Do not use mocks in tests'.
The AGENTS.md also says: 'Tests should avoid mocks or stubs and use the helpers from @internal/testcontainers when Redis or Postgres are needed.'
To fix this, rewrite the test to use postgresTest (or containerTest) from @internal/testcontainers, which provides a real PrismaClient connected to a real Postgres instance. The test should:
1. Use postgresTest or containerTest to get a real prisma client
2. Set up real TaskRun rows in the database with appropriate statuses and tags using prisma.taskRun.create()
3. Create a SkipIfActiveConcern with the real prisma client
4. Assert behavior against real database queries
See apps/webapp/test/engine/triggerTask.test.ts for an example of how to structure tests using containerTest in this directory.
Was this helpful? React with 👍 or 👎 to provide feedback.
| * unlike `concurrencyKey` (which queues duplicates). Requires at least | ||
| * one tag to scope the check. | ||
| */ | ||
| skipIfActive: z.boolean().optional(), |
There was a problem hiding this comment.
🚩 SDK trigger functions don't yet pass skipIfActive through to the API
The skipIfActive option was added to the Zod schemas (TriggerTaskRequestBody and BatchTriggerTaskItem in packages/core/src/v3/schemas/api.ts), but the SDK's trigger_internal function in packages/trigger-sdk/src/v3/shared.ts (around line 1166) builds the request body by explicitly mapping known options. There's no code in the SDK to pass skipIfActive from TriggerOptions to the API request body. This means SDK users can't actually use this feature via tasks.trigger() yet — they'd need to use the API directly. The TriggerOptions type in packages/core/src/v3/types/tasks.ts may also need updating. This may be intentional (SDK support in a follow-up PR) but is worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Refs #3428
Closes (none — filed alongside this PR; happy to open a tracking issue if you'd prefer)
✅ Checklist
.changeset/skip-if-active.md—@trigger.dev/coreminor)Testing
Unit tests added at
apps/webapp/test/engine/skipIfActiveConcern.test.ts(no DB, mocks Prisma). Covers:false→ no-op, no query executed.truebut no tags → no-op (documented: tag scope required).true+ tags + no match → proceed to normal run creation.true+ tags + match → return existing run,wasSkipped: true, no new run.tags: "foo") normalizes to array correctly.Local end-to-end verification against prod-like Postgres (maxcare's self-hosted Trigger.dev v4 instance running this patch):
skipIfActive=truewith matching in-flight run → response{ id, isCached: true, wasSkipped: true }, no new row in"TaskRun".skipIfActive=truewith no match → new run created normally.skipIfActive=truecombined withidempotencyKey→ idempotency match wins, the skip-check never fires.skipIfActive=truewithtags: []→ no-op (proceeds to create).Bitmap Heap ScanonTaskRun_runTags_idx+TaskRun_status_runtimeEnvironmentId_createdAt_id_idx, Execution Time <100ms against a 12M-row productionTaskRuntable.No DB schema change — uses existing indexes.
Changelog
New:
skipIfActive: booleanoption ontasks.trigger()and batch items.When set, and at least one
tagis supplied, the server short-circuits the trigger if an in-flight (non-terminal) TaskRun with the sametaskIdentifier+ environment + all of the supplied tags already exists. Returns the existing run withisCached: true+ newwasSkipped: trueflag. No new run is created. No queue entry. No trace span for the would-be new run.Why a new option? Existing options each miss the "cron scanner drops duplicates" case:
idempotencyKeyconcurrencyKeyruns.list()in user codetask_runs_v2 FINAL + hasAny(tags)in ClickHouse. On our self-hosted v4 instance we observed 59+ concurrent pile-up pegging CH at 100% CPU. See issue #3426 for background.skipIfActivegives a first-class drop-on-conflict primitive that lets the server do the dedup with one indexed Postgres lookup.Implementation
Mirrors the existing
IdempotencyKeyConcernpattern end-to-end.packages/core/src/v3/schemas/api.ts): adds the optional field toTriggerTaskRequestBody.optionsandBatchTriggerTaskItem.options. Adds optionalwasSkipped: booleantoTriggerTaskResponse.apps/webapp/app/runEngine/concerns/skipIfActive.server.ts): one indexed SELECT on"TaskRun"scoped to non-terminal statuses.runTags @> ARRAY[...]::text[]for AND-match on tags. Returns{ wasSkipped: true, run }on match.apps/webapp/app/runEngine/services/triggerTask.server.ts): invoked fromRunEngineTriggerTaskService.call()after idempotency (so explicit idempotency cache-hits win) and before run creation (so skipped triggers never touch the queue).apps/webapp/app/routes/api.v1.tasks.$taskId.trigger.ts): surfaceswasSkippedin the JSON response.TriggerTaskServiceResultgains optionalwasSkipped?: boolean.Scope
TriggerTaskServiceV1is intentionally untouched — v1 is frozen.BatchTriggerTaskItemcan carry its ownskipIfActive; the same concern fires per item.TaskRun_runTags_idx+ composite status/env index.skipIfActive: truewith no tags is a documented no-op (prevents surprising global dedup).idempotencyKey/concurrencyKey/delay/ttl/batchTriggerdocumented indocs/skip-if-active.mdx.Out of scope (follow-ups)
Docs
docs/skip-if-active.mdx— compares withidempotencyKeyandconcurrencyKey, walks through the cron-scanner example, lists interaction matrix.docs/docs.jsonunder the existing idempotency entry.Screenshots
N/A — server-side feature, no UI.
💯