Improved schedule engine #2174
Improved schedule engine #2174
Conversation
|
WalkthroughThis change set introduces a major refactor and expansion of the scheduling and worker infrastructure in the codebase, centered around the introduction of a new internal package, ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (5)
internal-packages/run-engine/src/engine/types.ts (1)
128-137: 💡 Verification agent🧩 Analysis chain
Temporal field proliferation – clarify semantics of
createdAtvsqueuedAt/queueTimestamp
TriggerParamsnow exposes three very similar timestamps:
createdAtqueuedAtqueueTimestampWithout a clear contract, callers may set only one of them or confuse their meaning, causing subtle ordering bugs in metrics or deduplication.
Suggest either:
- createdAt?: Date; + /** + * Absolute creation time of the *task run* record in the DB. + * Prefer `queuedAt` for queue-ing semantics and `queueTimestamp` for + * external-system hand-off. Documented here to avoid ambiguity. + */ + createdAt?: Date;and add JSDoc for the other two, or fold them into a single canonical field.
Run this to locate all writes to these properties:
🏁 Script executed:
#!/bin/bash rg -n --pretty $'(createdAt|queuedAt|queueTimestamp)\\s*:' | headLength of output: 296
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check for queuedAt and queueTimestamp in TriggerParams type rg -n "queuedAt" internal-packages/run-engine/src/engine/types.ts || echo "no matches for queuedAt" rg -n "queueTimestamp" internal-packages/run-engine/src/engine/types.ts || echo "no matches for queueTimestamp"Length of output: 209
Enhance JSDoc for Temporal Fields in TriggerParams
The three timestamp fields in
TriggerParams—createdAt,queuedAt, andqueueTimestamp—currently lack documentation and have subtly different meanings. Please add clear JSDoc comments to each to prevent misuse:• File:
internal-packages/run-engine/src/engine/types.ts
Lines: ~108–137Suggested diff:
interface TriggerParams { + /** + * Absolute creation time of the task-run record in the database. + * Use only for DB-side ordering; not for queue sequencing. + */ createdAt?: Date; + /** + * Time when this run was enqueued in our internal scheduler. + * Use for internal queue ordering and deduplication. + */ queuedAt?: Date; + /** + * Time when the run was handed off to an external system or broker. + * Use for external-system metrics and hand-off tracing. + */ queueTimestamp?: Date; machine?: MachinePresetName; workerId?: string; runnerId?: string; releaseConcurrency?: boolean; runChainState?: RunChainState; scheduleId?: string; scheduleInstanceId?: string; }apps/webapp/app/v3/services/triggerTaskV1.server.ts (1)
367-372: 🛠️ Refactor suggestionSame validation gap for
options.queueTimestampDuplicate the sanity check here (or extract a shared util) to avoid divergent behaviours between V1 and the new engine path.
internal-packages/schedule-engine/tsconfig.test.json (1)
1-22:⚠️ Potential issue
includemisses the actual test folderIntegration tests live in
test/, but"include": ["src/**/*.test.ts"]only covers unit tests insidesrc/.
vitestwill still run the file, yet the compiler won’t type-check it, defeating the purpose of this config.- "include": ["src/**/*.test.ts"], + "include": ["src/**/*.test.ts", "test/**/*.ts"],Add the glob (or move the tests) to ensure they’re compiled.
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
46-63: 🛠️ Refactor suggestion
txparameter is now unused – drop or use
enqueue()still acceptstx?: PrismaClientOrTransactionbut never references it after the refactor tocommonWorker.-static async enqueue( - deploymentId: string, - fromStatus: string, - errorMessage: string, - runAt: Date, - tx?: PrismaClientOrTransaction -) { +static async enqueue( + deploymentId: string, + fromStatus: string, + errorMessage: string, + runAt: Date, +) {Keeping an unused parameter risks confusion and lint warnings. If transactional enqueue is still required, wire it through
commonWorker.enqueue; otherwise remove it.apps/webapp/app/v3/services/cancelDevSessionRuns.server.ts (1)
93-106:⚠️ Potential issueBreaking change: parameter order flipped
enqueue()used to be(options, tx, runAt?).
The new signature(options, runAt?, tx?)silently swapsrunAtandtx. Existing call-sites will now pass a Prisma client where aDateis expected, resulting in an invalidavailableAtand no transactional context.Either restore the original ordering or add an overload while deprecating the old one:
-static async enqueue( - options: CancelDevSessionRunsServiceOptions, - runAt?: Date, - tx?: PrismaClientOrTransaction -) { +// Preserve old order +static async enqueue( + options: CancelDevSessionRunsServiceOptions, + tx?: PrismaClientOrTransaction, + runAt?: Date, +) {Remember to prefix
_txif it remains unused.
♻️ Duplicate comments (3)
apps/webapp/app/presenters/v3/RunListPresenter.server.ts (1)
182-186: SameclampToNowduplication as in NextRunListPresenterPlease pull this helper from a shared util to keep behaviour consistent across all call-sites and cut maintenance overhead.
internal-packages/schedule-engine/tsconfig.build.json (1)
6-8: Same DOM lib concern as in tsconfig.src.jsonConsider trimming the
libarray here as well to keep the build output consistent with the runtime environment.apps/webapp/app/v3/services/triggerTaskV1.server.ts (1)
442-443: Conditionally includecreatedAtApply the same conditional spread pattern here to avoid sending explicit
undefinedto Prisma.
🧹 Nitpick comments (31)
apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts (2)
192-195: Utility duplication – extractclampToNow
clampToNowis now copy-pasted in at least two presenters. Move it to a small shared util (e.g.~/utils/date.ts) to avoid divergence and ease testing.-function clampToNow(date: Date): Date { - const now = new Date(); - return date > now ? now : date; -} +import { clampToNow } from "~/utils/date";
206-208: Minor: avoid doubleDate→number→Dateconversions
time.from/time.toareDateobjects. You callclampToNow(time.to)(returnsDate) and then immediately.getTime(). Consider inlining to keep it obvious:- to: time.to ? clampToNow(time.to).getTime() : undefined, + to: time.to ? Math.min(time.to.getTime(), Date.now()) : undefined,Purely cosmetic; feel free to skip.
apps/webapp/app/presenters/v3/RunListPresenter.server.ts (1)
291-294: Performance & readability: calculate once
clampToNow(time.to)is evaluated during SQL template construction; for large batches this inside-template computation is negligible, but calculating once improves clarity:- time.to - ? Prisma.sql`AND tr."createdAt" <= ${clampToNow(time.to).toISOString()}::timestamp` + time.to + ? Prisma.sql`AND tr."createdAt" <= ${ + clampToNow(time.to).toISOString() + }::timestamp`Even better after extracting util as suggested above.
internal-packages/schedule-engine/test/setup.ts (1)
1-4: Consider moving global vitest config tovitest.config.*instead of runtime mutation
vi.setConfig({ testTimeout: 60_000 });works, but:
- It executes after test file import order, which can be flaky if other files rely on the timeout earlier.
- A
vitest.config.ts(or<root>/vitest.setup.tsreferenced viasetupFiles) is the canonical place.If you keep this, at least prefix the filename with
setupTests.to make its intent clearer.apps/webapp/app/v3/services/triggerTask.server.ts (1)
34-36: Consider surfacing docs / validation for the new options
queueTimestampandoverrideCreatedAtare accepted here, but nothing verifies that:
queueTimestamp≤overrideCreatedAt(or vice-versa depending on intent)- both are sensible (e.g. not in the future or > now + max skew)
A short JSDoc comment or runtime guard would prevent subtle clock-skew bugs.
apps/webapp/app/v3/services/createBackgroundWorker.server.ts (1)
28-29: Import is used only for side-effectful calls – keep tree-shaking friendly
scheduleEngineis imported at module top level but only referenced insidesyncDeclarativeSchedules.
For clarity / bundler friendliness you could move the import next to its usage:-import { scheduleEngine } from "../scheduleEngine.server"; +// defer import to keep the main bundle lean +import { scheduleEngine } from "../scheduleEngine.server";(Not critical; feel free to ignore if build size is not a concern.)
internal-packages/schedule-engine/tsconfig.json (1)
1-8: Minor: missing"strict": true&rootDircan hurt DXSince this is a new package, enabling compiler strictness early saves bugs later:
"compilerOptions": { + "strict": true, "moduleResolution": "Node16", "module": "Node16", "customConditions": ["@triggerdotdev/source"] }Also add
"rootDir": "./src"to keep emitted paths tidy.internal-packages/schedule-engine/tsconfig.src.json (1)
6-8: Unnecessary DOM libs increase type-check noiseThe library list includes
DOM*libs even though the package runs exclusively in Node. This can:
- pull in thousands of irrelevant types
- mask accidental browser-only API usage
- "lib": ["ES2020", "DOM", "DOM.Iterable", "DOM.AsyncIterable"], + "lib": ["ES2020"],Unless you intentionally reference browser globals inside the engine, dropping them will speed up builds and tighten type safety.
apps/webapp/app/runEngine/services/triggerTask.server.ts (1)
311-312: PassingcreatedAtasundefinedclutters the Prisma payload
createdAtis always present in the object, even when no override is supplied. Prisma acceptsundefined, but it still bloats query text and hides intent.Consider making the property conditional:
- createdAt: options.overrideCreatedAt, + ...(options.overrideCreatedAt + ? { createdAt: options.overrideCreatedAt } + : {}),Keeps the payload minimal and future-proof if Prisma tightens strict-null-checks.
internal-packages/schedule-engine/src/engine/workerCatalog.ts (1)
3-14: Explicit typing &as constwould safeguard the catalog
scheduleWorkerCatalogis currently an untyped object literal. Down-stream code will lose IntelliSense and accidental property changes won’t be caught.-export const scheduleWorkerCatalog = { +import type { WorkerCatalog } from "@trigger.dev/worker"; // hypothetical shared type + +export const scheduleWorkerCatalog: WorkerCatalog = { "schedule.triggerScheduledTask": { schema: z.object({ instanceId: z.string(), exactScheduleTime: z.coerce.date(), }),Optionally append
as constafter the object to freeze the keys.This minor change prevents typos (e.g.,
"schedule.triggerSchedueldTask") from compiling.apps/webapp/app/services/email.server.ts (1)
95-101: Return the enqueue promise to surface failures
scheduleEmailcurrently awaits the enqueue but returnsvoid, swallowing any value the worker library might provide (job id, etc.). Returning the promise enables callers to react or log the job id.-export async function scheduleEmail(data: DeliverEmail, delay?: { seconds: number }) { +export async function scheduleEmail( + data: DeliverEmail, + delay?: { seconds: number } +): Promise<void> { const availableAt = delay ? new Date(Date.now() + delay.seconds * 1000) : undefined; - await commonWorker.enqueue({ + return commonWorker.enqueue({ job: "scheduleEmail", payload: data, availableAt, }); }Also consider validating
delay.seconds >= 0to avoid accidental past dates.internal-packages/schedule-engine/src/engine/distributedScheduling.ts (1)
21-26: Small clarity nit
Math.pow(2, 31)can be replaced with the more idiomatic2 ** 31, which is easier to read and avoids an extra function call.apps/webapp/app/v3/commonWorker.server.ts (1)
48-141: Catalog definition is getting unwieldyHard-coding dozens of job specs with identical
visibilityTimeoutMsand very similar retry policies makes maintenance painful. Consider extracting helpers:const defaultJob = <T>(schema: T, retry = 5) => ({ schema, visibilityTimeoutMs: 60_000, retry: { maxAttempts: retry }, });Then:
catalog: { "v3.resumeBatchRun": defaultJob(z.object({ batchRunId: z.string() })), ... }This trims ~150 LOC and reduces copy-paste errors.
internal-packages/schedule-engine/README.md (2)
51-58: Minor wording & brevity nit“from executing at exactly the same moment”
-Prevents thundering herd issues by distributing executions across time windows +Prevents thundering-herd issues by spreading executions across a time window🧰 Tools
🪛 LanguageTool
[style] ~51-~51: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...t all scheduled tasks from executing at exactly the same moment: ```typescript import { calcula...(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
101-105: Dangling colon renders as loose punctuationA colon at the start of a bullet occasionally shows up in some markdown renderers:
-- `redis`: Redis connection configuration ++ `redis` – Redis connection configuration🧰 Tools
🪛 LanguageTool
[uncategorized] ~101-~101: Loose punctuation mark.
Context: ...these configuration options: -prisma: PrismaClient instance -redis: Redis ...(UNLIKELY_OPENING_PUNCTUATION)
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
53-62: ConsiderenqueueOnceto avoid duplicate timeout jobsTimeouts are idempotent; using plain
enqueuecan queue duplicates ifenqueueis called multiple times before the first job is processed.- await commonWorker.enqueue({ + await commonWorker.enqueueOnce({ id: `timeoutDeployment:${deploymentId}`,The unique
idis already provided, so switching toenqueueOnceprotects against accidental duplication at no extra cost.apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts (1)
107-116: UseenqueueOncefor id-based deduplicationAs with the timeout service, the job ID is deterministic (
v3.executeTasksWaitingForDeploy:${backgroundWorkerId}).
Switching toenqueueOnceguarantees exactly-once semantics during bursts (e.g., multiple deploy hooks firing).- return await commonWorker.enqueue({ + return await commonWorker.enqueueOnce({apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts (1)
60-69: Unusedtxparameter – silence linter or drop itThe
txargument is no longer forwarded tocommonWorker.enqueue, leaving it unused and triggering the@typescript-eslint/no-unused-varsrule in most code-bases.-static async enqueue(attemptId: string, tx: PrismaClientOrTransaction, runAt?: Date) { +static async enqueue( + attemptId: string, + _tx: PrismaClientOrTransaction, // keep positional compatibility but silence linter + runAt?: Date +) {If transactions are no longer required here, consider making the parameter optional and prefixing with an underscore (or removing it entirely) across call-sites to avoid confusion.
apps/webapp/app/runEngine/services/batchTrigger.server.ts (1)
316-321:txparameter kept but never used
#enqueueBatchTaskRun()still accepts atxparameter yet the value is ignored after the switch tocommonWorker.enqueue. Either remove the parameter or rename it to_txto clarify that it is intentionally unused:-async #enqueueBatchTaskRun(options: BatchProcessingOptions, tx?: PrismaClientOrTransaction) { +async #enqueueBatchTaskRun(options: BatchProcessingOptions, _tx?: PrismaClientOrTransaction) {Cleaning this up avoids false expectations that the call will be wrapped in a transaction.
apps/webapp/app/v3/services/resumeTaskDependency.server.ts (1)
157-171: UnusedtxargumentSimilar to other services,
txis accepted but not used after the move tocommonWorker.enqueue.-static async enqueue( - dependencyId: string, - sourceTaskAttemptId: string, - tx: PrismaClientOrTransaction, - runAt?: Date -) { +static async enqueue( + dependencyId: string, + sourceTaskAttemptId: string, + _tx: PrismaClientOrTransaction, + runAt?: Date +) {This prevents linter warnings and signals that transactional support has been removed.
apps/webapp/test/distributedScheduling.test.ts (1)
57-88: Potential flakiness in uniform-distribution assertionThe “should distribute work evenly” test uses 1 000 samples and fails if fewer than 80 % of 30 buckets receive at least one hit.
While unlikely, the deterministic hash could still cluster enough samples to dip below the 24-bucket threshold, making the test flaky.Consider loosening the assertion or increasing the sample size:
-const testCount = 1000; -… -expect(bucketsWithItems).toBeGreaterThan(distributionWindow * 0.8); +const testCount = 5_000; // reduces variance +… +expect(bucketsWithItems).toBeGreaterThan(distributionWindow * 0.7);This keeps the spirit of the check while minimising spurious CI failures.
apps/webapp/app/v3/services/resumeBatchRun.server.ts (1)
2-2: Unused import side-effectSwitching to
commonWorkermakes thePrismaClientOrTransactionargument inenqueue()obsolete. Consider pruning that parameter from the method signature (and its call-sites) to avoid dead code and confusion.apps/webapp/app/v3/scheduleEngine.server.ts (1)
81-116: Error is swallowed – add structured loggingThe
catchonly returns{ success:false }but drops stack traces; production debugging will be painful.- } catch (error) { - return { - success: false, - error: error instanceof Error ? error.message : String(error), - }; + } catch (error) { + logger.error("scheduleEngine.onTriggerScheduledTask failed", { error }); + return { + success: false, + error: error instanceof Error ? error.message : String(error), + }; }apps/webapp/app/v3/services/batchTriggerV3.server.ts (2)
478-540: Inconsistent logging & IDs reference “V2”This v3 service logs under
BatchTriggerV2and builds job IDs withBatchTriggerV2Service.process.
Rename toBatchTriggerV3to avoid future grep/debug headaches.- id: `BatchTriggerV2Service.process:${options.batchId}:${options.processingId}`, + id: `BatchTriggerV3Service.process:${options.batchId}:${options.processingId}`,Same for every logger.debug message using
[BatchTriggerV2] ….
895-899:txparameter is never used
#enqueueBatchTaskRunstill acceptstxbut ignores it. Drop the argument or forward it to a transactional queue helper; leaving it dangling is misleading.internal-packages/schedule-engine/test/scheduleEngine.test.ts (1)
121-123: Avoid brittle exact-equality on Date
expect(actualNextExecution).toEqual(expectedExecutionTime)can flake by a few ms.
Use a ±1 s window ortoBeCloseTowith a tolerance instead.internal-packages/schedule-engine/src/engine/types.ts (1)
27-30: ConstrainlogLevelvia a union
logLevel?: stringaccepts invalid values at runtime. ConsiderlogLevel?: "log" | "error" | "warn" | "info" | "debug";This mirrors the enum you use elsewhere and prevents typos.
apps/webapp/app/v3/services/upsertTaskSchedule.server.ts (2)
108-128: Parallelise instance creation / registrationThe loop issues a Prisma insert then immediately waits for
scheduleEngine.registerNextTaskScheduleInstance.
With many environments this becomes O(n) sequential I/O. Consider:await Promise.all( options.environments.map(async (environmentId) => { const instance = await this._prisma.taskScheduleInstance.create({ … }); await scheduleEngine.registerNextTaskScheduleInstance({ instanceId: instance.id }); }) );This keeps semantic order while cutting latency substantially.
206-218: Typo & readability
instancesToDeleted➜instancesToDelete. Minor, but avoiding grammatical slips helps future readers.internal-packages/schedule-engine/src/engine/index.ts (1)
96-106: Side-effects in constructor hinder testabilitySpawning the worker inside the constructor (
this.worker.start()) means merely importing / instantiating the class connects to Redis and spawns threads, complicating unit tests and life-cycle management.
Expose an explicitstart()method instead; call it from your singleton wrapper.apps/webapp/app/env.server.ts (1)
686-695: Clarify overlapping concurrency env-varsThe trio
SCHEDULE_WORKER_CONCURRENCY_WORKERS,
SCHEDULE_WORKER_CONCURRENCY_TASKS_PER_WORKER, and
SCHEDULE_WORKER_CONCURRENCY_LIMIT
overlap but the codebase only readsconcurrency(workers × tasks). Document which flags are authoritative or drop the unused ones to avoid misconfiguration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreferences/hello-world/src/trigger/schedule.tsis excluded by!references/**
📒 Files selected for processing (49)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts(2 hunks)apps/webapp/app/presenters/v3/RunListPresenter.server.ts(2 hunks)apps/webapp/app/runEngine/services/batchTrigger.server.ts(2 hunks)apps/webapp/app/runEngine/services/triggerTask.server.ts(1 hunks)apps/webapp/app/services/email.server.ts(2 hunks)apps/webapp/app/services/runsRepository.server.ts(1 hunks)apps/webapp/app/services/worker.server.ts(19 hunks)apps/webapp/app/v3/commonWorker.server.ts(3 hunks)apps/webapp/app/v3/scheduleEngine.server.ts(1 hunks)apps/webapp/app/v3/services/batchTriggerV3.server.ts(3 hunks)apps/webapp/app/v3/services/cancelDevSessionRuns.server.ts(2 hunks)apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts(2 hunks)apps/webapp/app/v3/services/changeCurrentDeployment.server.ts(1 hunks)apps/webapp/app/v3/services/createBackgroundWorker.server.ts(3 hunks)apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts(1 hunks)apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts(2 hunks)apps/webapp/app/v3/services/registerNextTaskScheduleInstance.server.ts(0 hunks)apps/webapp/app/v3/services/resumeBatchRun.server.ts(2 hunks)apps/webapp/app/v3/services/resumeTaskDependency.server.ts(2 hunks)apps/webapp/app/v3/services/resumeTaskRunDependencies.server.ts(2 hunks)apps/webapp/app/v3/services/retryAttempt.server.ts(2 hunks)apps/webapp/app/v3/services/timeoutDeployment.server.ts(2 hunks)apps/webapp/app/v3/services/triggerScheduledTask.server.ts(0 hunks)apps/webapp/app/v3/services/triggerTask.server.ts(1 hunks)apps/webapp/app/v3/services/triggerTaskV1.server.ts(2 hunks)apps/webapp/app/v3/services/upsertTaskSchedule.server.ts(4 hunks)apps/webapp/app/v3/utils/distributedScheduling.server.ts(1 hunks)apps/webapp/package.json(1 hunks)apps/webapp/test/distributedScheduling.test.ts(1 hunks)internal-packages/run-engine/src/engine/index.ts(2 hunks)internal-packages/run-engine/src/engine/types.ts(1 hunks)internal-packages/schedule-engine/README.md(1 hunks)internal-packages/schedule-engine/package.json(1 hunks)internal-packages/schedule-engine/src/engine/distributedScheduling.ts(1 hunks)internal-packages/schedule-engine/src/engine/index.ts(1 hunks)internal-packages/schedule-engine/src/engine/scheduleCalculation.ts(1 hunks)internal-packages/schedule-engine/src/engine/types.ts(1 hunks)internal-packages/schedule-engine/src/engine/workerCatalog.ts(1 hunks)internal-packages/schedule-engine/src/index.ts(1 hunks)internal-packages/schedule-engine/test/scheduleEngine.test.ts(1 hunks)internal-packages/schedule-engine/test/setup.ts(1 hunks)internal-packages/schedule-engine/tsconfig.build.json(1 hunks)internal-packages/schedule-engine/tsconfig.json(1 hunks)internal-packages/schedule-engine/tsconfig.src.json(1 hunks)internal-packages/schedule-engine/tsconfig.test.json(1 hunks)internal-packages/schedule-engine/vitest.config.ts(1 hunks)packages/redis-worker/src/worker.ts(1 hunks)packages/rsc/src/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- apps/webapp/app/v3/services/registerNextTaskScheduleInstance.server.ts
- apps/webapp/app/v3/services/triggerScheduledTask.server.ts
🧰 Additional context used
🧬 Code Graph Analysis (15)
apps/webapp/app/v3/services/changeCurrentDeployment.server.ts (1)
apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts (1)
ExecuteTasksWaitingForDeployService(8-117)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts (1)
apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts (1)
ExecuteTasksWaitingForDeployService(8-117)
apps/webapp/app/v3/services/resumeTaskRunDependencies.server.ts (1)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)
apps/webapp/app/services/email.server.ts (2)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)internal-packages/emails/src/index.tsx (1)
data(80-122)
apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts (1)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)
apps/webapp/app/v3/services/timeoutDeployment.server.ts (3)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)apps/webapp/app/db.server.ts (1)
PrismaClientOrTransaction(21-21)apps/webapp/app/services/worker.server.ts (1)
workerQueue(373-373)
apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts (1)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)
packages/redis-worker/src/worker.ts (2)
internal-packages/schedule-engine/src/engine/index.ts (1)
JobHandlerParams(217-225)internal-packages/zod-worker/src/index.ts (1)
K(581-660)
apps/webapp/app/v3/services/retryAttempt.server.ts (1)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)
internal-packages/schedule-engine/src/engine/types.ts (4)
internal-packages/schedule-engine/src/index.ts (3)
TriggerScheduledTaskCallback(5-5)ScheduleEngineOptions(3-3)TriggerScheduleParams(4-4)apps/webapp/app/db.server.ts (1)
PrismaClient(228-228)internal-packages/redis/src/index.ts (1)
RedisOptions(4-4)internal-packages/tracing/src/index.ts (2)
Tracer(14-14)Meter(16-16)
apps/webapp/app/v3/services/resumeTaskDependency.server.ts (1)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)
apps/webapp/test/distributedScheduling.test.ts (1)
apps/webapp/app/v3/utils/distributedScheduling.server.ts (1)
calculateDistributedExecutionTime(6-31)
apps/webapp/app/v3/services/cancelDevSessionRuns.server.ts (2)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)apps/webapp/app/v3/services/batchTriggerV3.server.ts (1)
options(894-900)
apps/webapp/app/v3/services/resumeBatchRun.server.ts (1)
apps/webapp/app/v3/commonWorker.server.ts (1)
commonWorker(279-279)
apps/webapp/app/v3/services/createBackgroundWorker.server.ts (1)
apps/webapp/app/v3/scheduleEngine.server.ts (1)
scheduleEngine(11-11)
🪛 LanguageTool
internal-packages/schedule-engine/README.md
[style] ~51-~51: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...t all scheduled tasks from executing at exactly the same moment: ```typescript import { calcula...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
[uncategorized] ~101-~101: Loose punctuation mark.
Context: ...these configuration options: - prisma: PrismaClient instance - redis: Redis ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
packages/rsc/src/package.json (1)
1-3: Minimalpackage.jsonLGTM, but double-check directory purposeDeclaring
"type": "module"is fine and consistent with the rest of the repo.
Make sure:
- No other
package.jsonhigher in thepackages/rsctree overrides this.- Tooling that consumes this folder (ts-node, vitest, etc.) expects ESM.
If both are already covered, nothing else to do here.
apps/webapp/package.json (1)
53-57: Workspace dependency added – ensure pipeline & lockfile are updated
"@internal/schedule-engine": "workspace:*"looks correct.
Please run the package-manager install (pnpm install/npm install -w) and commit the updated lockfile so CI picks it up.apps/webapp/app/v3/services/changeCurrentDeployment.server.ts (1)
87-87: Call site now matches newenqueuesignature – all good
ExecuteTasksWaitingForDeployService.enqueuenow only accepts(backgroundWorkerId, runAt?).
This update removes the stale Prisma argument and compiles cleanly.apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts (1)
164-166: LGTM – signature change reflected correctly
ExecuteTasksWaitingForDeployService.enqueueno longer requires the Prisma client, and the call site has been updated accordingly.apps/webapp/app/v3/services/createBackgroundWorker.server.ts (1)
571-573: Error propagation unchanged but worth revisiting
scheduleEngine.registerNextTaskScheduleInstanceis awaited inside this tight loop.
If that call throws (e.g. transient Redis issue) the entire worker-creation flow aborts and rolls back the DB transaction – the same behaviour the removed service had, but now the failure surface is broader (Redis, distributed locks…).Consider wrapping in a retry helper or catching and logging non-fatal errors to avoid making deployments brittle.
Also applies to: 603-604
internal-packages/schedule-engine/vitest.config.ts (1)
11-13: Confirm the esbuild target is aligned with runtime
target: "node18"is fine today, but if the runtime matrix includes Node 20 (current LTS) the emitted code will miss out on native improvements (e.g., fetch, structuredClone). Worth double-checking the production containers.internal-packages/schedule-engine/src/index.ts (1)
1-6: Public entry-point looks goodThe minimalist barrel file is clear and follows the Node16/ESM convention that the compiler will emit
.jsfiles, so the explicit.jsextension is appropriate. No further concerns.apps/webapp/app/v3/services/resumeTaskRunDependencies.server.ts (1)
8-9: Import change looks correctSwitching to the new
commonWorkeraligns with the broader refactor.apps/webapp/app/services/email.server.ts (1)
7-8: Import update acknowledgedMigrating to
commonWorkeris consistent with the rest of the PR.internal-packages/schedule-engine/package.json (1)
8-14: Non-standard export key
"@triggerdotdev/source"insideexportsisn’t a recognised condition and may break resolution in Node ≥16. Verify that tools relying on this key actually support it; otherwise use a documented condition such as"source"or"development".apps/webapp/app/v3/commonWorker.server.ts (1)
188-195: Ensure numeric env values
pollIntervalMs,immediatePollIntervalMs, worker counts, etc., are passed straight fromenv.*. Confirm the Zod schema parses them to numbers; if they are still strings, RedisWorker will treat"5000"as a string and mis-handle arithmetic/timeouts.If parsing isn’t already done in
env.server.ts, coerce here:pollIntervalMs: Number(env.COMMON_WORKER_POLL_INTERVAL),apps/webapp/app/services/worker.server.ts (1)
238-246: ConfirmfinalAttemptsemantics against worker implementation
finalAttemptis derived fromjob.attempts === job.max_attempts.
Ingraphile-workertheattemptscounter already includes the current attempt, so the flag will betruewhile the job is still running, not after it has finally failed.
IfscheduleEngine.triggerScheduledTask()internally interpretsfinalAttemptas “no more retries will follow”, this could lead to premature deletion / alerting.await scheduleEngine.triggerScheduledTask({ instanceId: payload.instanceId, finalAttempt: job.attempts === job.max_attempts, // ← verify semantics });Please double-check the contract and adjust to
job.attempts + 1 === job.max_attempts(or similar) if the engine expects the next attempt to be the last.apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
66-69: Acknowledge both queues symmetrically
dequeueis invoked onworkerQueue, whileackis invoked oncommonWorker.
IfworkerQueue.dequeue()fails (e.g., job not found) thecommonWorker.ack()will still run, silently discarding the job in the new queue. Consider guarding with an existence check or catching errors to keep behaviour predictable during the migration window.packages/redis-worker/src/worker.ts (1)
37-48: Type extraction looks goodExtracting
JobHandlerParamsmakes handler definitions clearer and reusable; compile-time inference also improves.apps/webapp/app/v3/services/resumeBatchRun.server.ts (1)
337-344: VerifyavailableAtsemantics
commonWorker.enqueueoften expects a Unix epoch (ms) whereas you’re passing aDate | undefined.
Double-check the worker’s API – if it expects a number, wrap with.getTime(); if it acceptsDate, ignore this note.internal-packages/schedule-engine/src/engine/scheduleCalculation.ts (1)
22-30: LGTM – cron calculation is concise and correct
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/v3/commonWorker.server.ts (3)
127-154: Two almost-identical job definitions may lead to accidental drift
"v3.processBatchTaskRun"and"runengine.processBatchTaskRun"repeat the same schema/timeout/retry logic, differing only by a couple of optional fields.
If both names are required, consider:
- Creating a shared
BatchTaskRunSchemaand spreading it into both entries to avoid silent divergence later.- Verifying that downstream producers/consumers consistently pick the intended key – the similarity makes copy-paste mistakes easy.
Let me know if you’d like a quick refactor snippet.
42-49:DeliverEmailSchemais imported but never reused in the handlerThe catalog entry validates against
DeliverEmailSchema, yet the handler receivespayloaduntyped (any).
Annotating the handler parameter tightens compile-time safety:-import { DeliverEmailSchema } from "emails"; +import { DeliverEmailSchema, type DeliverEmail } from "emails"; … - scheduleEmail: async ({ payload }) => { - await sendEmail(payload); + scheduleEmail: async ({ payload }) => { + const email: DeliverEmail = payload; // satisfies schema + await sendEmail(email); },This prevents accidental misuse of the
payloadobject.Also applies to: 211-214
78-87: Constant-ify the ubiquitousvisibilityTimeoutMs: 60_000/maxAttemptsThe same magic numbers reappear in every catalog entry. Extracting small helpers (e.g.
const ONE_MINUTE = 60_000) or a factory function improves readability and makes global tuning trivial.Also applies to: 112-114, 136-138, 150-154
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/v3/commonWorker.server.ts(3 hunks)packages/redis-worker/src/queue.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/redis-worker/src/queue.ts
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- 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 (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/webapp/app/v3/commonWorker.server.ts (1)
201-205:❌ Incorrect review comment
Convert concurrency env values to numbers before passing to the worker
envvariables arestring | undefined.RedisWorker’sconcurrencyoptions expect numbers; handing it strings risks implicit coercion bugs when the library does arithmetic (e.g.Math.min). Parse once at the boundary:- concurrency: { - workers: env.COMMON_WORKER_CONCURRENCY_WORKERS, - tasksPerWorker: env.COMMON_WORKER_CONCURRENCY_TASKS_PER_WORKER, - limit: env.COMMON_WORKER_CONCURRENCY_LIMIT, - }, + concurrency: { + workers: Number(env.COMMON_WORKER_CONCURRENCY_WORKERS), + tasksPerWorker: Number(env.COMMON_WORKER_CONCURRENCY_TASKS_PER_WORKER), + limit: Number(env.COMMON_WORKER_CONCURRENCY_LIMIT), + },
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Searching for concurrency env vars in schema:" rg -n "COMMON_WORKER_CONCURRENCY_WORKERS" -n . echo "🔍 Locating where `env` is exported:" rg -n "export const env" -n .Length of output: 2457
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Checking tasksPerWorker schema definition:" rg -n "COMMON_WORKER_CONCURRENCY_TASKS_PER_WORKER" apps/webapp/app/env.server.ts echo "🔍 Checking limit schema definition:" rg -n "COMMON_WORKER_CONCURRENCY_LIMIT" apps/webapp/app/env.server.tsLength of output: 507
🏁 Script executed:
#!/bin/bash set -e echo "🔍 Showing import statements in commonWorker.server.ts:" sed -n '1,50p' apps/webapp/app/v3/commonWorker.server.ts | grep -E "^import .*env"Length of output: 248
No conversion needed: concurrency env vars are already numbers
Theenvimport comes fromEnvironmentSchema.parse(process.env), where these keys usez.coerce.number().int().default(...). That guaranteesenv.COMMON_WORKER_CONCURRENCY_*are typed and parsed asnumber, so wrapping them inNumber(…)is redundant.No changes required.
Likely an incorrect or invalid review comment.
…ter that out in the lists
ericallam
force-pushed
the
cursor-schedule-worker-redis
branch
from
68375fc to
42c3f51
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal-packages/schedule-engine/src/engine/index.ts (2)
42-42: Unsafe tracer fallback still present
CastingstartSpantoanyto extract.tracerdefeats type-safety and risks breaking when the tracing util changes. Provide the tracer viaoptions.traceror an explicitgetTracer()helper instead.
608-609: Job-ID collision risk remains
id: \scheduled-task-instance:${instanceId}`` is reused for every run; pending or stalled jobs with the same ID will cause enqueue failures or silent overwrites.
Include a unique component (timestamp or counter) in the ID.
🧹 Nitpick comments (2)
internal-packages/schedule-engine/src/engine/index.ts (1)
90-90: Hard-coded “debug” logger level for worker
The worker always uses a newLogger("ScheduleEngineWorker", "debug"), ignoring the engine’s configured log level.
Pass the chosen level (or the existing logger instance) to avoid noisy logs in production.internal-packages/schedule-engine/README.md (1)
51-57: Minor wording nitpick
“executing at exactly the same moment” → “executing simultaneously” is shorter and clearer.🧰 Tools
🪛 LanguageTool
[style] ~51-~51: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...t all scheduled tasks from executing at exactly the same moment: ```typescript import { calcula...(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreferences/hello-world/src/trigger/schedule.tsis excluded by!references/**
📒 Files selected for processing (50)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts(2 hunks)apps/webapp/app/presenters/v3/RunListPresenter.server.ts(2 hunks)apps/webapp/app/runEngine/services/batchTrigger.server.ts(2 hunks)apps/webapp/app/runEngine/services/triggerTask.server.ts(1 hunks)apps/webapp/app/services/email.server.ts(2 hunks)apps/webapp/app/services/runsRepository.server.ts(1 hunks)apps/webapp/app/services/worker.server.ts(19 hunks)apps/webapp/app/v3/commonWorker.server.ts(3 hunks)apps/webapp/app/v3/scheduleEngine.server.ts(1 hunks)apps/webapp/app/v3/services/batchTriggerV3.server.ts(3 hunks)apps/webapp/app/v3/services/cancelDevSessionRuns.server.ts(2 hunks)apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts(2 hunks)apps/webapp/app/v3/services/changeCurrentDeployment.server.ts(1 hunks)apps/webapp/app/v3/services/createBackgroundWorker.server.ts(3 hunks)apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts(1 hunks)apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts(2 hunks)apps/webapp/app/v3/services/registerNextTaskScheduleInstance.server.ts(0 hunks)apps/webapp/app/v3/services/resumeBatchRun.server.ts(2 hunks)apps/webapp/app/v3/services/resumeTaskDependency.server.ts(2 hunks)apps/webapp/app/v3/services/resumeTaskRunDependencies.server.ts(2 hunks)apps/webapp/app/v3/services/retryAttempt.server.ts(2 hunks)apps/webapp/app/v3/services/timeoutDeployment.server.ts(2 hunks)apps/webapp/app/v3/services/triggerScheduledTask.server.ts(0 hunks)apps/webapp/app/v3/services/triggerTask.server.ts(1 hunks)apps/webapp/app/v3/services/triggerTaskV1.server.ts(2 hunks)apps/webapp/app/v3/services/upsertTaskSchedule.server.ts(4 hunks)apps/webapp/app/v3/utils/distributedScheduling.server.ts(1 hunks)apps/webapp/package.json(1 hunks)apps/webapp/test/distributedScheduling.test.ts(1 hunks)internal-packages/run-engine/src/engine/index.ts(2 hunks)internal-packages/run-engine/src/engine/types.ts(1 hunks)internal-packages/schedule-engine/README.md(1 hunks)internal-packages/schedule-engine/package.json(1 hunks)internal-packages/schedule-engine/src/engine/distributedScheduling.ts(1 hunks)internal-packages/schedule-engine/src/engine/index.ts(1 hunks)internal-packages/schedule-engine/src/engine/scheduleCalculation.ts(1 hunks)internal-packages/schedule-engine/src/engine/types.ts(1 hunks)internal-packages/schedule-engine/src/engine/workerCatalog.ts(1 hunks)internal-packages/schedule-engine/src/index.ts(1 hunks)internal-packages/schedule-engine/test/scheduleEngine.test.ts(1 hunks)internal-packages/schedule-engine/test/setup.ts(1 hunks)internal-packages/schedule-engine/tsconfig.build.json(1 hunks)internal-packages/schedule-engine/tsconfig.json(1 hunks)internal-packages/schedule-engine/tsconfig.src.json(1 hunks)internal-packages/schedule-engine/tsconfig.test.json(1 hunks)internal-packages/schedule-engine/vitest.config.ts(1 hunks)packages/redis-worker/src/queue.ts(1 hunks)packages/redis-worker/src/worker.ts(1 hunks)packages/rsc/src/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- apps/webapp/app/v3/services/registerNextTaskScheduleInstance.server.ts
- apps/webapp/app/v3/services/triggerScheduledTask.server.ts
✅ Files skipped from review due to trivial changes (2)
- internal-packages/schedule-engine/src/index.ts
- internal-packages/schedule-engine/src/engine/scheduleCalculation.ts
🚧 Files skipped from review as they are similar to previous changes (44)
- packages/rsc/src/package.json
- internal-packages/run-engine/src/engine/types.ts
- internal-packages/schedule-engine/test/setup.ts
- apps/webapp/package.json
- apps/webapp/app/v3/services/changeCurrentDeployment.server.ts
- internal-packages/schedule-engine/tsconfig.json
- apps/webapp/app/v3/services/createBackgroundWorker.server.ts
- apps/webapp/app/v3/services/triggerTask.server.ts
- apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts
- packages/redis-worker/src/queue.ts
- apps/webapp/app/runEngine/services/triggerTask.server.ts
- internal-packages/schedule-engine/vitest.config.ts
- apps/webapp/app/services/runsRepository.server.ts
- internal-packages/schedule-engine/tsconfig.src.json
- apps/webapp/app/presenters/v3/NextRunListPresenter.server.ts
- internal-packages/schedule-engine/tsconfig.build.json
- apps/webapp/app/v3/services/resumeTaskRunDependencies.server.ts
- apps/webapp/app/services/email.server.ts
- internal-packages/schedule-engine/tsconfig.test.json
- apps/webapp/app/v3/services/triggerTaskV1.server.ts
- apps/webapp/app/v3/utils/distributedScheduling.server.ts
- apps/webapp/app/services/worker.server.ts
- apps/webapp/app/presenters/v3/RunListPresenter.server.ts
- apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts
- apps/webapp/app/v3/services/timeoutDeployment.server.ts
- internal-packages/schedule-engine/src/engine/workerCatalog.ts
- internal-packages/schedule-engine/package.json
- internal-packages/run-engine/src/engine/index.ts
- internal-packages/schedule-engine/src/engine/distributedScheduling.ts
- apps/webapp/app/runEngine/services/batchTrigger.server.ts
- apps/webapp/app/v3/services/resumeTaskDependency.server.ts
- apps/webapp/app/v3/commonWorker.server.ts
- apps/webapp/app/v3/services/cancelDevSessionRuns.server.ts
- apps/webapp/test/distributedScheduling.test.ts
- apps/webapp/app/v3/services/retryAttempt.server.ts
- packages/redis-worker/src/worker.ts
- apps/webapp/app/v3/services/resumeBatchRun.server.ts
- internal-packages/schedule-engine/test/scheduleEngine.test.ts
- apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts
- internal-packages/schedule-engine/src/engine/types.ts
- apps/webapp/app/v3/scheduleEngine.server.ts
- apps/webapp/app/v3/services/batchTriggerV3.server.ts
- apps/webapp/app/v3/services/upsertTaskSchedule.server.ts
- apps/webapp/app/env.server.ts
🧰 Additional context used
🪛 LanguageTool
internal-packages/schedule-engine/README.md
[style] ~51-~51: ‘exactly the same’ might be wordy. Consider a shorter alternative.
Context: ...t all scheduled tasks from executing at exactly the same moment: ```typescript import { calcula...
(EN_WORDINESS_PREMIUM_EXACTLY_THE_SAME)
[uncategorized] ~101-~101: Loose punctuation mark.
Context: ...these configuration options: - prisma: PrismaClient instance - redis: Redis ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
…ty (indexing now happens during the build image process)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsx (2)
52-55: Remove the staledeploymentIndexingIsRetryableimportThe retry-indexing UI and worker flow were removed, yet the helper is still imported.
Keeping it around adds dead code and a silent dependency that will be forgotten later.-import { deploymentIndexingIsRetryable } from "~/v3/deploymentStatus";
328-343:canRetryIndexingis now pointless and causes an empty pop-over
canRetryIndexingis still calculated but no longer drives any menu item.
When it istrue(andcanBeMadeCurrentisfalse) we render the 3-dot menu with no entries, producing a confusing blank pop-over for users.Proposed minimal cleanup:
- const canRetryIndexing = deployment.isLatest && deploymentIndexingIsRetryable(deployment); const canBeRolledBack = canBeMadeCurrent && currentDeployment?.version && compareDeploymentVersions(deployment.version, currentDeployment.version) === -1; const canBePromoted = canBeMadeCurrent && !canBeRolledBack; - if (!canBeMadeCurrent && !canRetryIndexing) { + if (!canBeMadeCurrent) { return ( <TableCell to={path} isSelected={isSelected}> {""} </TableCell> ); }…and delete the now-unused
canRetryIndexingcomputation (together with the import noted above).This avoids the UX glitch and eliminates redundant logic.
apps/webapp/app/v3/services/cancelTaskRunV1.server.ts (1)
114-121: Enqueue dependency-cancellation jobs in parallel to improve latency.Inside a tight loop we
awaiteachCancelTaskAttemptDependenciesService.enqueue, forcing N sequential round-trips to Redis:for (const attempt of attempts) { await CancelTaskAttemptDependenciesService.enqueue(attempt.id); }This can be cut from O(N) network RTTs to ~1 by batching with
Promise.all:- for (const attempt of attempts) { - await CancelTaskAttemptDependenciesService.enqueue(attempt.id); - } + await Promise.all( + attempts.map((a) => + CancelTaskAttemptDependenciesService.enqueue(a.id) + ), + );Throughput is higher and the surrounding cancellation flow finishes sooner.
♻️ Duplicate comments (2)
internal-packages/schedule-engine/src/engine/index.ts (2)
616-623: Job-ID collision risk persists – ensure uniqueness
enqueue()still uses onlyinstanceIdfor the job ID; multiple runs queued before pruning will overwrite each other or be rejected.- id: `scheduled-task-instance:${instanceId}`, + // Include the exact schedule time to guarantee uniqueness + id: `scheduled-task-instance:${instanceId}:${exactScheduleTime.getTime()}`,
352-355: Guard missing for optional dev-environment connectivity handler
isDevEnvironmentConnectedHandleris invoked unconditionally and will throw if the option is omitted.- const [devConnectedError, isConnected] = await tryCatch( - this.options.isDevEnvironmentConnectedHandler(instance.environment.id) - ); + let isConnected = false; + let devConnectedError: unknown; + if (this.options.isDevEnvironmentConnectedHandler) { + [devConnectedError, isConnected] = await tryCatch( + this.options.isDevEnvironmentConnectedHandler(instance.environment.id) + ); + } else { + shouldTrigger = false; + skipReason = "dev_connection_handler_missing"; + }
🧹 Nitpick comments (4)
internal-packages/schedule-engine/src/engine/index.ts (3)
130-139: PreferfindUniqueoverfindFirstfor primary-key lookups
idis a unique/PK column;findUniqueis faster, conveys intent, and avoids accidental surprises if the unique constraint changes.- const instance = await this.prisma.taskScheduleInstance.findFirst({ + const instance = await this.prisma.taskScheduleInstance.findUnique({ where: { id: params.instanceId }, include: { taskSchedule: true, environment: true, }, });Apply the same change in
triggerScheduledTask.Also applies to: 259-273
159-166: Usingnew Date()whenlastScheduledTimestampis null can skip earlier executionsIf a schedule was created long ago but never run, defaulting to the current time may jump over intended back-dated runs. Consider seeding with the schedule’s creation time or the earliest valid cron match instead.
98-99: Worker logger hard-codes"debug"levelThis ignores
options.logLevelsupplied by the caller and causes inconsistent logging.- logger: new Logger("ScheduleEngineWorker", "debug"), + logger: new Logger("ScheduleEngineWorker", (this.options.logLevel ?? "info") as any),apps/webapp/app/v3/commonWorker.server.ts (1)
201-204: Add defensive logging / metrics around email delivery.
sendEmailmay throw network-level errors (SMTP, SES throttling, etc.). Letting the exception propagate re-queues the job, but you lose the root-cause context. Wrap with try/catch to add structured logs (or increment a failure metric) before re-throwing:- scheduleEmail: async ({ payload }) => { - await sendEmail(payload); - }, + scheduleEmail: async ({ payload }) => { + try { + await sendEmail(payload); + } catch (err) { + logger.error("scheduleEmail failed", { payload, err }); + throw err; // preserve retry semantics + } + },This makes post-mortems faster without affecting job retries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/webapp/app/components/runs/v3/RetryDeploymentIndexingDialog.tsx(0 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsx(1 hunks)apps/webapp/app/routes/resources.$projectId.deployments.$deploymentShortCode.retry-indexing.ts(0 hunks)apps/webapp/app/services/worker.server.ts(17 hunks)apps/webapp/app/v3/commonWorker.server.ts(3 hunks)apps/webapp/app/v3/scheduleEngine.server.ts(1 hunks)apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts(2 hunks)apps/webapp/app/v3/services/cancelTaskRunV1.server.ts(1 hunks)apps/webapp/app/v3/services/indexDeployment.server.ts(0 hunks)apps/webapp/app/v3/services/resumeTaskRunDependencies.server.ts(0 hunks)apps/webapp/app/v3/services/retryDeploymentIndexing.server.ts(0 hunks)internal-packages/schedule-engine/src/engine/index.ts(1 hunks)internal-packages/schedule-engine/src/engine/types.ts(1 hunks)packages/redis-worker/src/worker.ts(3 hunks)
💤 Files with no reviewable changes (5)
- apps/webapp/app/components/runs/v3/RetryDeploymentIndexingDialog.tsx
- apps/webapp/app/routes/resources.$projectId.deployments.$deploymentShortCode.retry-indexing.ts
- apps/webapp/app/v3/services/retryDeploymentIndexing.server.ts
- apps/webapp/app/v3/services/resumeTaskRunDependencies.server.ts
- apps/webapp/app/v3/services/indexDeployment.server.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts
- internal-packages/schedule-engine/src/engine/types.ts
- packages/redis-worker/src/worker.ts
- apps/webapp/app/services/worker.server.ts
- apps/webapp/app/v3/scheduleEngine.server.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/webapp/app/v3/services/cancelTaskRunV1.server.ts (1)
apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts (1)
CancelTaskAttemptDependenciesService(7-70)
apps/webapp/app/v3/commonWorker.server.ts (10)
internal-packages/emails/src/index.tsx (1)
DeliverEmailSchema(20-32)apps/webapp/app/services/email.server.ts (1)
sendEmail(104-106)apps/webapp/app/runEngine/services/batchTrigger.server.ts (2)
payload(657-687)RunEngineBatchTriggerService(52-688)apps/webapp/app/v3/services/batchTriggerV3.server.ts (2)
payload(902-932)BatchTriggerV3Service(95-933)apps/webapp/app/v3/services/resumeTaskDependency.server.ts (1)
ResumeTaskDependencyService(8-172)apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
TimeoutDeploymentService(8-70)apps/webapp/app/v3/services/executeTasksWaitingForDeploy.ts (1)
ExecuteTasksWaitingForDeployService(8-117)apps/webapp/app/v3/services/retryAttempt.server.ts (1)
RetryAttemptService(7-36)apps/webapp/app/v3/services/cancelTaskAttemptDependencies.server.ts (1)
CancelTaskAttemptDependenciesService(7-70)apps/webapp/app/v3/services/cancelDevSessionRuns.server.ts (1)
CancelDevSessionRunsService(18-107)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
internal-packages/schedule-engine/src/engine/index.ts (1)
230-233: Retry /finalAttemptlogic is a stub
finalAttemptis alwaysfalse; failures insidetriggerScheduledTaskwill bubble and potentially spam retries without back-off. Verify the intended retry semantics and wire them up.apps/webapp/app/v3/commonWorker.server.ts (1)
191-195: Ensure env numeric values are coerced to numbers.
@trigger.dev/redis-workerexpectsworkers,tasksPerWorker, andlimitto be numbers.
Ifenvsurfaces raw strings, the worker will silently treat them asNaN, effectively disabling the concurrency limits.- workers: env.COMMON_WORKER_CONCURRENCY_WORKERS, - tasksPerWorker: env.COMMON_WORKER_CONCURRENCY_TASKS_PER_WORKER, - limit: env.COMMON_WORKER_CONCURRENCY_LIMIT, + workers: Number(env.COMMON_WORKER_CONCURRENCY_WORKERS), + tasksPerWorker: Number(env.COMMON_WORKER_CONCURRENCY_TASKS_PER_WORKER), + limit: Number(env.COMMON_WORKER_CONCURRENCY_LIMIT),Confirm that the
envhelper already returns numbers; otherwise, coerce explicitly as above.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal-packages/schedule-engine/src/engine/index.ts (2)
352-356: Guard optionalisDevEnvironmentConnectedHandlerbefore invoking
this.options.isDevEnvironmentConnectedHandleris still called unconditionally. If the option isn’t supplied, this throwsTypeError: undefined is not a function, breaking triggers in development environments.
Add a presence check or make the handler mandatory.
616-622: Job-ID collision risk remains
id: \scheduled-task-instance:${instanceId}`is reused for every run. If a previous run hasn’t been pruned,enqueue` may reject or overwrite, silently dropping executions.- id: `scheduled-task-instance:${instanceId}`, + id: `scheduled-task-instance:${instanceId}:${exactScheduleTime.getTime()}`,
🧹 Nitpick comments (1)
internal-packages/schedule-engine/src/engine/index.ts (1)
348-350: High-cardinality metric label
dev_environment_checks_totaltags every sample withenvironment_id, which can explode the cardinality of your time-series database.
Consider using low-cardinality labels (e.g.environment_type) and exposing high-cardinality data via logs instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal-packages/schedule-engine/src/engine/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
No description provided.