chore(db): remove unnecessary FK constraints on TaskRunExecutionSnapshot #2533
Conversation
|
WalkthroughThis change removes foreign key constraints from public.TaskRunExecutionSnapshot via a migration and refactors the Prisma schema to decouple TaskRunExecutionSnapshot from several entities. In schema.prisma, executionSnapshots back-relations are removed from Organization, RuntimeEnvironment, Project, WorkerInstance, and BatchTaskRun. TaskRunExecutionSnapshot drops relations to those models, adds scalar projectId and organizationId, keeps environmentId as a scalar with environmentType, and introduces completedWaitpointOrder (String[]). The batchId remains as a scalar without a relation. In the run engine, the heartbeat update no longer writes to the database; it logs a heartbeat instead while retaining scheduling logic. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal-packages/database/prisma/schema.prisma (1)
912-912: completedWaitpointOrder lacks a default; create path can omit it → possible NOT NULL insert failurecreateExecutionSnapshot only sets this when completedWaitpoints is provided. Make it default to [].
- completedWaitpointOrder String[] + completedWaitpointOrder String[] @default([])Also ensure the migration sets a DB default and backfills nulls to '{}' for existing rows.
🧹 Nitpick comments (3)
internal-packages/database/prisma/schema.prisma (2)
1532-1536: Optional: name back-relations for clarity/future-proofingExplicit @relation names on runsBlocked and waitpoints help avoid ambiguity if another relation between the same models is added.
Example:
runsBlocked TaskRunWaitpoint[] @relation("BatchRunsBlocked") waitpoints Waitpoint[] @relation("BatchCompletedWaitpoints")You’d also need to mirror names on the other sides.
925-925: Deprecate lastHeartbeatAt (heartbeats no longer persisted)This field is now stale. Either remove it in a follow-up migration or mark as deprecated to prevent accidental reads.
- lastHeartbeatAt DateTime? + /// @deprecated Heartbeats are logged (not persisted) as of this change. + lastHeartbeatAt DateTime?internal-packages/database/prisma/migrations/20250919123905_remove_unnecessary_foreign_key_constraints_task_run_execution_snapshot/migration.sql (1)
1-15: Idempotent FK drops LGTM; plan for brief AccessExclusive lock on table
- IF EXISTS makes this safe across environments. Good.
- Postgres will take an AccessExclusive lock on TaskRunExecutionSnapshot; run during low traffic to avoid blocking writers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal-packages/database/prisma/migrations/20250919123905_remove_unnecessary_foreign_key_constraints_task_run_execution_snapshot/migration.sql(1 hunks)internal-packages/database/prisma/schema.prisma(2 hunks)internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
internal-packages/database/prisma/schema.prisma (1)
905-907: projectId/organizationId now scalars: consider adding indexesWithout FKs, add secondary indexes if queries filter by these fields to avoid regressions.
Suggested indexes:
model TaskRunExecutionSnapshot { // ... projectId String organizationId String // ... - @@index([runId, isValid, createdAt(sort: Desc)]) + @@index([runId, isValid, createdAt(sort: Desc)]) + @@index([projectId, createdAt(sort: Desc)]) + @@index([organizationId, createdAt(sort: Desc)]) }Verify need:
internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts (1)
377-381: Lower heartbeat log level; confirm DB heartbeat consumers
- Found lastHeartbeatAt in Prisma schema/migrations (TaskRunExecutionSnapshot & WorkerInstance), in API schema, and writes in:
- internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts
- apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts
- packages/core/src/v3/schemas/api.ts
- Confirm no internal/external consumer reads TaskRunExecutionSnapshot.lastHeartbeatAt for liveness. If any do, either keep DB writes (or emit an explicit heartbeat event/metric) and/or deprecate/remove the column.
- Tone down logging to debug and serialize the timestamp; suggested change:
- this.$.logger.info("heartbeatRun snapshot heartbeat updated", { - id: latestSnapshot.id, - runId: latestSnapshot.runId, - lastHeartbeatAt: new Date(), - }); + this.$.logger.debug("heartbeatRun snapshot heartbeat", { + id: latestSnapshot.id, + runId: latestSnapshot.runId, + lastHeartbeatAt: new Date().toISOString(), + });
No description provided.