Skip to content

Conversation

@ericallam
Copy link
Member

@ericallam ericallam commented Jan 21, 2026

Summary

  • Adds IF NOT EXISTS to the migration that adds idempotencyKeyOptions column to prevent errors if the column already exists

Migration Checksum Fix

If you've already applied the previous version of this migration, you'll need to update the checksum in your _prisma_migrations table to match the new migration file.

Previous checksum: f8876e274e3f7735312275eb24a9c4b40f512ac12a286b2de3add47f66df5b27
New checksum: 0620a914ddbaf01279576274432e51c41f41502cd4c8de38621625380750e397

Fix instructions

Run this SQL command against your database:

UPDATE "_prisma_migrations"
SET checksum = '0620a914ddbaf01279576274432e51c41f41502cd4c8de38621625380750e397'
WHERE migration_name = '20260116154810_add_idempotency_key_options_to_task_run';

This updates the stored checksum to match the modified migration file, allowing future migrations to proceed without checksum mismatch errors.

Test plan

  • Verified migration applies cleanly on fresh database
  • Verified checksum update works on database with previous migration applied

🤖 Generated with Claude Code

@changeset-bot
Copy link

changeset-bot bot commented Jan 21, 2026

⚠️ No Changeset found

Latest commit: d28a951

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
Contributor

coderabbitai bot commented Jan 21, 2026

Walkthrough

This pull request contains two changes: a modification to a Prisma database migration that changes the ADD COLUMN syntax to ADD COLUMN IF NOT EXISTS for an idempotencyKeyOptions JSONB column on the TaskRun table, and a new bash script that implements a batch concurrency cleaner for Redis-backed batch processing. The script accepts Redis URLs and optional flags, uses Lua scripting to atomically identify stale concurrency entries across Redis shards, and provides both reporting and cleanup capabilities with a dry-run mode.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete compared to the required template. Critical sections like a proper issue reference, contributing guide acknowledgment checklist, testing section, changelog, and screenshots are missing or incomplete. Follow the provided template more closely: add issue reference with 'Closes #', complete the checklist items, add a proper Testing section describing test steps, include a Changelog section with concise change summary, and add Screenshots section if applicable.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding IF NOT EXISTS to a migration file. It is specific, concise, and directly related to the primary change in the changeset.

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

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
scripts/batch-concurrency-cleaner.sh (6)

14-14: Consider adding bash strict mode for safer execution.

Adding set -u (or set -eu) would catch unset variable references, which is particularly important for a script that performs destructive operations on Redis data.

♻️ Suggested improvement
-set -e
+set -eu
+set -o pipefail

65-71: Suppressing stderr may hide connection failures.

Redirecting 2>/dev/null will mask authentication errors, connection timeouts, and TLS issues. Consider logging stderr to a file or only suppressing specific warnings.

♻️ Alternative approach
 rcli_read() {
-  redis-cli -u "$READ_REDIS" --no-auth-warning "$@" 2>/dev/null
+  redis-cli -u "$READ_REDIS" --no-auth-warning "$@"
 }

 rcli_write() {
-  redis-cli -u "$WRITE_REDIS" --no-auth-warning "$@" 2>/dev/null
+  redis-cli -u "$WRITE_REDIS" --no-auth-warning "$@"
 }

If specific warnings need suppression, redirect stderr to a log file for debugging: 2>>/tmp/batch-cleaner-errors.log


101-105: Shard count is hardcoded in multiple places.

The script assumes exactly 12 shards (0-11), which is embedded in both the Lua script (checking KEYS[2] through KEYS[13]) and this key construction. If the shard count changes, both locations would need updating.

Consider parameterizing shard count or adding a comment documenting this assumption.


112-122: Arithmetic operations assume valid numeric responses from Redis.

If rcli_read fails or returns unexpected output (e.g., error message, nil), the arithmetic expansion will either fail or produce incorrect results. Consider validating the response.

♻️ Defensive pattern
   master_total=0
   for i in 0 1 2 3 4 5 6 7 8 9 10 11; do
-    count=$(rcli_read ZCARD "engine:batch:master:$i")
+    count=$(rcli_read ZCARD "engine:batch:master:$i") || count=0
+    [[ "$count" =~ ^[0-9]+$ ]] || count=0
     master_total=$((master_total + count))
   done

160-163: SREM calls could be batched for better efficiency.

SREM supports removing multiple members in a single call. When there are many stale entries, individual calls create unnecessary network round-trips.

♻️ Batch SREM approach
         else
-          # Remove each stale entry individually with SREM (idempotent, safe)
-          for sid in "${stale_array[@]}"; do
-            rcli_write SREM "$conc_key" "$sid" >/dev/null
-          done
+          # Remove all stale entries in a single SREM call (idempotent, safe)
+          rcli_write SREM "$conc_key" "${stale_array[@]}" >/dev/null
           echo "[$ts] CLEANED: $tenant ($stale_count stale entries removed)"
           cleaned_tenants=$((cleaned_tenants + 1))
         fi

33-35: DELAY argument is not validated as a numeric value.

A non-numeric or negative value for --delay would cause sleep to fail. Consider adding validation after argument parsing.

♻️ Add validation
# After argument parsing, validate DELAY
if ! [[ "$DELAY" =~ ^[0-9]+$ ]] || [[ "$DELAY" -lt 1 ]]; then
  echo "Error: --delay must be a positive integer"
  exit 1
fi
📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd2f536 and d28a951.

📒 Files selected for processing (2)
  • internal-packages/database/prisma/migrations/20260116154810_add_idempotency_key_options_to_task_run/migration.sql
  • scripts/batch-concurrency-cleaner.sh
🧰 Additional context used
📓 Path-based instructions (1)
internal-packages/database/prisma/migrations/**/*.sql

📄 CodeRabbit inference engine (CLAUDE.md)

internal-packages/database/prisma/migrations/**/*.sql: When editing the Prisma schema, remove extraneous migration lines related to specific tables: _BackgroundWorkerToBackgroundWorkerFile, _BackgroundWorkerToTaskQueue, _TaskRunToTaskRunTag, _WaitpointRunConnections, _completedWaitpoints, SecretStore_key_idx, and unrelated TaskRun indexes
Database indexes must use CONCURRENTLY to avoid table locks and must be in their own separate migration file

Files:

  • internal-packages/database/prisma/migrations/20260116154810_add_idempotency_key_options_to_task_run/migration.sql
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : When editing the Prisma schema, remove extraneous migration lines related to specific tables: `_BackgroundWorkerToBackgroundWorkerFile`, `_BackgroundWorkerToTaskQueue`, `_TaskRunToTaskRunTag`, `_WaitpointRunConnections`, `_completedWaitpoints`, `SecretStore_key_idx`, and unrelated `TaskRun` indexes
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : Database indexes must use CONCURRENTLY to avoid table locks and must be in their own separate migration file
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : When editing the Prisma schema, remove extraneous migration lines related to specific tables: `_BackgroundWorkerToBackgroundWorkerFile`, `_BackgroundWorkerToTaskQueue`, `_TaskRunToTaskRunTag`, `_WaitpointRunConnections`, `_completedWaitpoints`, `SecretStore_key_idx`, and unrelated `TaskRun` indexes

Applied to files:

  • internal-packages/database/prisma/migrations/20260116154810_add_idempotency_key_options_to_task_run/migration.sql
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : Database indexes must use CONCURRENTLY to avoid table locks and must be in their own separate migration file

Applied to files:

  • internal-packages/database/prisma/migrations/20260116154810_add_idempotency_key_options_to_task_run/migration.sql
⏰ 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). (21)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 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 (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
internal-packages/database/prisma/migrations/20260116154810_add_idempotency_key_options_to_task_run/migration.sql (1)

1-2: LGTM!

The IF NOT EXISTS clause makes this migration idempotent, which is a good defensive pattern for column additions. This ensures the migration can be safely re-run without failing if the column already exists.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

@vibe-kanban-cloud
Copy link

Review Complete

Your review story is ready!

View Story

Comment !reviewfast on this PR to re-generate the story.

@ericallam ericallam marked this pull request as ready for review January 21, 2026 14:04
@ericallam ericallam merged commit bd449f7 into main Jan 21, 2026
32 checks passed
@ericallam ericallam deleted the ea-branch-114 branch January 21, 2026 14:43
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.

3 participants