Skip to content

feat(schema): add WorkerDeployment.externalId and task_runs_v2.external_deployment_id - #4661

Open
0ski wants to merge 1 commit into
mainfrom
oskar/feat-external-id-migrations
Open

feat(schema): add WorkerDeployment.externalId and task_runs_v2.external_deployment_id#4661
0ski wants to merge 1 commit into
mainfrom
oskar/feat-external-id-migrations

Conversation

@0ski

@0ski 0ski commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Migrations only, no code reads them yet. Postgres: nullable non-unique externalId on WorkerDeployment plus a CONCURRENTLY-built (environmentId, externalId) index in its own migration file. ClickHouse: external_deployment_id String DEFAULT '' on task_runs_v2 (plain String, not LowCardinality - commit SHAs are high-cardinality). Part of task run version skew protection (TRI-12998).

@changeset-bot

changeset-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 603acad

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

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The changes add external_deployment_id to ClickHouse task runs with reversible migration support. They also add optional externalId storage to Prisma WorkerDeployment records and define an index on environmentId and externalId.

Merge Risk: 🟡 Moderate · up to 603ac

The new PostgreSQL index migration can fail when deployed through the default transactional path because concurrent index creation requires a non-transactional execution path. Merge readiness depends on adjusting the migration or deployment procedure.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the schema changes but omits the required issue reference, checklist, testing, changelog, and screenshots sections. Add the required template sections, including the issue reference, completed checklist, testing steps, changelog entry, and screenshots or a stated reason they are not applicable.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies both schema changes in the pull request.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch oskar/feat-external-id-migrations

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.

coderabbitai[bot]

This comment was marked as resolved.

@0ski
0ski marked this pull request as ready for review August 18, 2026 13:15
devin-ai-integration[bot]

This comment was marked as resolved.

@0ski
0ski force-pushed the oskar/feat-external-id-migrations branch from 7625d61 to 603acad Compare August 18, 2026 18:50
@pkg-pr-new

pkg-pr-new Bot commented Aug 18, 2026

Copy link
Copy Markdown

Open in StackBlitz

@trigger.dev/build

npm i https://pkg.pr.new/@trigger.dev/build@aad440b

trigger.dev

npm i https://pkg.pr.new/trigger.dev@aad440b

@trigger.dev/core

npm i https://pkg.pr.new/@trigger.dev/core@aad440b

@trigger.dev/python

npm i https://pkg.pr.new/@trigger.dev/python@aad440b

@trigger.dev/react-hooks

npm i https://pkg.pr.new/@trigger.dev/react-hooks@aad440b

@trigger.dev/redis-worker

npm i https://pkg.pr.new/@trigger.dev/redis-worker@aad440b

@trigger.dev/rsc

npm i https://pkg.pr.new/@trigger.dev/rsc@aad440b

@trigger.dev/schema-to-json

npm i https://pkg.pr.new/@trigger.dev/schema-to-json@aad440b

@trigger.dev/sdk

npm i https://pkg.pr.new/@trigger.dev/sdk@aad440b

commit: aad440b

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9f86bf16-4933-4e54-ad10-7f694b0ae9be

📥 Commits

Reviewing files that changed from the base of the PR and between 7625d61 and 603acad.

📒 Files selected for processing (4)
  • internal-packages/clickhouse/schema/040_add_task_runs_v2_external_deployment_id.sql
  • internal-packages/database/prisma/migrations/20260818120000_add_worker_deployment_external_id/migration.sql
  • internal-packages/database/prisma/migrations/20260818120100_add_worker_deployment_environment_id_external_id_index/migration.sql
  • internal-packages/database/prisma/schema.prisma
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal-packages/database/prisma/schema.prisma

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📜 Review details
⏰ Context from checks skipped due to timeout. (21)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 12)
  • GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 12)
  • GitHub Check: runops-guard / runops-guard
  • GitHub Check: internal / 🧪 Unit Tests: Internal
  • GitHub Check: typecheck / typecheck
  • GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp
  • GitHub Check: fk-cascade-guard / fk-cascade-guard
  • GitHub Check: code-quality / code-quality
  • GitHub Check: audit
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Build and publish previews
🧰 Additional context used
📓 Path-based instructions (2)
internal-packages/database/prisma/migrations/**/migration.sql

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

internal-packages/database/prisma/migrations/**/migration.sql: When adding indexes to existing tables, use CREATE INDEX CONCURRENTLY IF NOT EXISTS to avoid production table locks.
Keep each CONCURRENTLY index in its own separate migration file, and add only one index per migration file.
Indexes on newly created tables may be created without CONCURRENTLY in the same migration as CREATE TABLE.
When adding an index for a new column on an existing table, use two migrations: first ALTER TABLE ... ADD COLUMN IF NOT EXISTS ..., then a separate migration containing CREATE INDEX CONCURRENTLY IF NOT EXISTS ....

Files:

  • internal-packages/database/prisma/migrations/20260818120000_add_worker_deployment_external_id/migration.sql
  • internal-packages/database/prisma/migrations/20260818120100_add_worker_deployment_environment_id_external_id_index/migration.sql
internal-packages/clickhouse/schema/[0-9][0-9][0-9]_*.sql

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

internal-packages/clickhouse/schema/[0-9][0-9][0-9]_*.sql: Migration file numbering: name files as 0(N+1)_descriptive_name.sql where N is the largest existing migration number in schema/; rebase and renumber if main adds migrations before opening a PR
DDL in migrations must be idempotent: use ALTER TABLE ... ADD COLUMN IF NOT EXISTS, CREATE TABLE IF NOT EXISTS, DROP TABLE IF EXISTS, ADD INDEX IF NOT EXISTS, DROP INDEX IF EXISTS, and CREATE MATERIALIZED VIEW IF NOT EXISTS forms to allow out-of-order and retry-safe application

Files:

  • internal-packages/clickhouse/schema/040_add_task_runs_v2_external_deployment_id.sql
🧠 Learnings (2)
📚 Learning: 2026-02-03T18:48:31.790Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: internal-packages/database/prisma/migrations/20260129162810_add_integration_deployment/migration.sql:14-18
Timestamp: 2026-02-03T18:48:31.790Z
Learning: For Prisma migrations targeting PostgreSQL: - When adding indexes to existing tables, create the index in a separate migration file and include CONCURRENTLY to avoid locking the table. - For indexes on newly created tables (in CREATE TABLE statements), you can create the index in the same migration file without CONCURRENTLY. This reduces rollout complexity for new objects while protecting uptime for existing structures.

Applied to files:

  • internal-packages/database/prisma/migrations/20260818120000_add_worker_deployment_external_id/migration.sql
  • internal-packages/database/prisma/migrations/20260818120100_add_worker_deployment_environment_id_external_id_index/migration.sql
📚 Learning: 2026-03-22T13:49:20.068Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: internal-packages/database/prisma/migrations/20260318114244_add_prompt_friendly_id/migration.sql:5-5
Timestamp: 2026-03-22T13:49:20.068Z
Learning: For Prisma migration SQL files under `internal-packages/database/prisma/migrations/`, it is acceptable to create indexes with `CREATE INDEX` / `CREATE UNIQUE INDEX` (i.e., without `CONCURRENTLY`) when the parent table is introduced in the same PR and has no existing production rows yet. Only require `CREATE INDEX CONCURRENTLY` (or otherwise account for existing production data/locks) when the table already exists in production with data.

Applied to files:

  • internal-packages/database/prisma/migrations/20260818120000_add_worker_deployment_external_id/migration.sql
  • internal-packages/database/prisma/migrations/20260818120100_add_worker_deployment_environment_id_external_id_index/migration.sql
🔇 Additional comments (2)
internal-packages/clickhouse/schema/040_add_task_runs_v2_external_deployment_id.sql (1)

1-7: LGTM!

internal-packages/database/prisma/migrations/20260818120000_add_worker_deployment_external_id/migration.sql (1)

1-1: LGTM!

…al_deployment_id

Migrations only, no code reads them yet. Postgres: nullable non-unique externalId on WorkerDeployment plus a CONCURRENTLY-built (environmentId, externalId) index in its own migration file. ClickHouse: external_deployment_id String DEFAULT '' on task_runs_v2 (plain String, not LowCardinality - commit SHAs are high-cardinality). Part of task run version skew protection (TRI-12998).
@0ski
0ski force-pushed the oskar/feat-external-id-migrations branch from 603acad to aad440b Compare August 19, 2026 10:37
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