feat(engine): Improve execution stalls troubleshooting, align dev and prod behavior, adding heartbeats.yield utility #2489
Conversation
… prod behavior, adding heartbeats.yield utility
|
WalkthroughThis PR implements a heartbeat system across core, CLI, workers, and SDK: adds HeartbeatsAPI, HeartbeatsManager, and StandardHeartbeatsManager; exposes a v3 heartbeats API and SDK helpers (yield and getLastHeartbeat); and replaces interval-based worker heartbeats with event-driven heartbeats. It updates run-engine stall handling (environment-aware messages, retry control, and runStatusFromError now takes environmentType), adds guards against operating on FINISHED snapshots, switches permanently-fail flows to use latest snapshots, raises default run execution timeouts to 10 minutes, and adds troubleshooting docs for stalled executions. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks (2 warnings, 1 inconclusive)❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/v3/errors.ts (1)
306-310: Typo in error code literal: “TASK_HAS_N0_EXECUTION_SNAPSHOT”.The “0” digit will never match the real code (“NO”), breaking classification and falling into
assertExhaustive. Fix the literal.Apply:
- case "TASK_HAS_N0_EXECUTION_SNAPSHOT": + case "TASK_HAS_NO_EXECUTION_SNAPSHOT":internal-packages/run-engine/src/engine/errors.ts (1)
55-55: Typo in error code literal: “TASK_HAS_N0_EXECUTION_SNAPSHOT”.Same issue as in core errors; will never match the real code.
Apply:
- case "TASK_HAS_N0_EXECUTION_SNAPSHOT": + case "TASK_HAS_NO_EXECUTION_SNAPSHOT":
🧹 Nitpick comments (19)
apps/webapp/app/env.server.ts (1)
522-523: 10-minute stall timeout defaults: verify operational alignmentConfirm dashboards/alarms and docs consistently reflect 10 minutes, and that staging/preview have overrides if needed.
packages/cli-v3/src/executions/taskRunProcess.ts (3)
112-116: Don't silently swallow cancel errorsSwallowing here hides unexpected failures to deliver CANCEL. Log at debug.
try { await this.#cancel(); - } catch (err) {} + } catch (err) { + logger.debug("cancel: failed to send CANCEL to child", { err, pid: this.pid }); + }
126-127: Capture and log flush errors during cleanupKeep cleanup resilient but log failures for diagnosis.
- await tryCatch(this.#flush()); + { + const [err] = await tryCatch(this.#flush()); + if (err) logger.debug("cleanup: flush failed", { err, pid: this.pid }); + }
442-443: Capture and log flush errors during suspendSame rationale as cleanup.
- await tryCatch(this.#flush()); + { + const [err] = await tryCatch(this.#flush()); + if (err) logger.debug("suspend: flush failed", { err, pid: this.pid }); + }docs/troubleshooting.mdx (1)
184-215: Tiny enhancement: mention env knobs for the 10‑min timeoutConsider adding: “You can configure the stall timeout via RUN_ENGINE_TIMEOUT_EXECUTING and RUN_ENGINE_TIMEOUT_EXECUTING_WITH_WAITPOINTS.”
packages/cli-v3/src/entryPoints/dev-run-controller.ts (2)
360-369: FINISHED handling: detach handlers before suspend (reduce race window)Small window where a heartbeat could still fire. Detach first.
- await this.exitTaskRunProcessWithoutFailingRun({ + this.taskRunProcess?.unsafeDetachEvtHandlers(); + await this.exitTaskRunProcessWithoutFailingRun({ flush: true, reason: "already-finished", });
620-643: Don’t await heartbeat I/O in the event handlerFire-and-forget to avoid backpressure if multiple heartbeats cluster.
- const response = await this.httpClient.dev.heartbeatRun( + void this.httpClient.dev.heartbeatRun( this.runFriendlyId, this.snapshotFriendlyId, { cpu: 0, memory: 0, } - ); - - if (!response.success) { - logger.debug("[DevRunController] Heartbeat failed", { error: response.error }); - } + ).then((response) => { + if (!response.success) { + logger.debug("[DevRunController] Heartbeat failed", { error: response.error }); + } + });packages/core/src/v3/heartbeats/types.ts (1)
1-8: Prefertypealias overinterfaceper repo guidelines.Functionally equivalent here and aligns with “we usually use types over interfaces”.
Apply:
-export interface HeartbeatsManager { - startHeartbeat(id: string): void; - stopHeartbeat(): void; - yield(): Promise<void>; - reset(): void; - - get lastHeartbeat(): Date | undefined; -} +export type HeartbeatsManager = { + startHeartbeat(id: string): void; + stopHeartbeat(): void; + yield(): Promise<void>; + reset(): void; + readonly lastHeartbeat: Date | undefined; +};internal-packages/run-engine/src/engine/errors.ts (1)
29-34: Avoid magic string comparison for environment.Use the exported constant if available, or a typed constant in-file to prevent typos.
Apply:
+const DEV: RuntimeEnvironmentType = "DEVELOPMENT"; - if (environmentType === "DEVELOPMENT") { + if (environmentType === DEV) { return "CANCELED"; }packages/trigger-sdk/src/v3/heartbeats.ts (1)
37-39: Naming nit: preferlastHeartbeatgetter to mirror coreTo keep SDK and core consistent, consider exporting a getter-style property rather than a
getLastHeartbeat()function, e.g.,lastHeartbeat(): Date | undefinedor re-exporting the value via a getter. Not critical.-export const heartbeats = { - yield: heartbeatsYield, - getLastHeartbeat: heartbeatsGetLastHeartbeat, -}; +export const heartbeats = { + yield: heartbeatsYield, + get lastHeartbeat() { + return heartbeatsGetLastHeartbeat(); + }, +};packages/cli-v3/src/entryPoints/managed-run-worker.ts (1)
33-34: ImportattemptKeyto align heartbeat IDs with dev workerIf we standardize on attempt-level IDs, bring in
attemptKey.traceContext, + attemptKey, heartbeats, } from "@trigger.dev/core/v3";internal-packages/run-engine/src/engine/index.ts (2)
216-217: Use Logger instead of console.log for startup messagePrefer the project Logger for consistency and log routing.
- console.log("✅ Starting run engine worker"); + this.logger.info("✅ Starting run engine worker");
1253-1265: Avoid hardcoding “sent every 30s” in stall messagesHeartbeat cadence may vary across environments. Remove the fixed interval or derive it from config.
- ? `Run timed out after ${timeoutDuration} due to missing heartbeats (sent every 30s). Check if your \`trigger.dev dev\` CLI is still running, or if CPU-heavy work is blocking the main thread.` - : `Run timed out after ${timeoutDuration} due to missing heartbeats (sent every 30s). This typically happens when CPU-heavy work blocks the main thread.`; + ? `Run timed out after ${timeoutDuration} due to missing heartbeats. Check if your \`trigger.dev dev\` CLI is still running, or if CPU-heavy work is blocking the main thread.` + : `Run timed out after ${timeoutDuration} due to missing heartbeats. This typically happens when CPU-heavy work blocks the main thread.`;packages/core/src/v3/heartbeats/api.ts (2)
41-43: Allow optional override when registering a global managerUseful for tests/dev hot-reload; keeps API explicit.
- public setGlobalManager(manager: HeartbeatsManager): boolean { - return registerGlobal(API_NAME, manager); - } + public setGlobalManager(manager: HeartbeatsManager, allowOverride = false): boolean { + return registerGlobal(API_NAME as any, manager, allowOverride); + }
58-65: Annotate return types and avoid returning values for void methodsClarifies API shape and prevents accidental Promise propagation if implementations change.
- public startHeartbeat(id: string) { - return this.#getManager().startHeartbeat(id); - } + public startHeartbeat(id: string): void { + this.#getManager().startHeartbeat(id); + } - public stopHeartbeat() { - return this.#getManager().stopHeartbeat(); - } + public stopHeartbeat(): void { + this.#getManager().stopHeartbeat(); + }packages/core/src/v3/heartbeats/manager.ts (4)
22-28: Yield without adding artificial latency24ms adds avoidable delay; a 0ms timeout is sufficient to yield cooperatively.
- // Only call setImmediate if we haven't yielded in the last interval + // Only yield if we haven't yielded in the last interval if (Date.now() - this.lastHeartbeatYieldTime >= this.intervalInMs) { - // await setImmediate(); - await setTimeout(24); + await new Promise<void>((r) => setTimeout(r, 0)); this.lastHeartbeatYieldTime = Date.now(); }
45-50: Only mark lastHeartbeat on success and preserve error detailsKeep lastHeartbeat meaningful (successful send) and log the error object to retain stack/cause.
- const [error] = await tryCatch(this.listener(id)); - this.lastHeartbeatDate = new Date(); - - if (error) { - console.error("Failed to send HEARTBEAT message", { error: String(error) }); - } + const [error] = await tryCatch(this.listener(id)); + if (error) { + console.error("Failed to send HEARTBEAT message", { error }); + } else { + this.lastHeartbeatDate = new Date(); + }Would you like lastHeartbeat to represent “last attempted” instead? If so, keep the original semantics and just adjust the logger.
59-61: Clear state on stop to avoid stale controller and gatingOptional, but makes stopHeartbeat fully idempotent and resets yield throttling until restarted.
stopHeartbeat(): void { - this.currentAbortController?.abort(); + this.currentAbortController?.abort(); + this.currentAbortController = undefined; + this.lastHeartbeatYieldTime = undefined; }
11-16: Consider returning an unsubscribe from registerListenerLow-priority: returning a disposer helps tests swap listeners safely, even if you keep a single global callback at runtime.
- registerListener(callback: (id: string) => Promise<void>) { - this.listener = callback; - } + registerListener(callback: (id: string) => Promise<void>): () => void { + this.listener = callback; + return () => { + if (this.listener === callback) this.listener = undefined; + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
references/hello-world/src/trigger/cpuHeavy.tsis excluded by!references/**
📒 Files selected for processing (20)
apps/webapp/app/env.server.ts(1 hunks)docs/troubleshooting.mdx(1 hunks)internal-packages/run-engine/src/engine/errors.ts(2 hunks)internal-packages/run-engine/src/engine/index.ts(4 hunks)internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts(7 hunks)packages/cli-v3/src/entryPoints/dev-run-controller.ts(8 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(7 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(6 hunks)packages/cli-v3/src/executions/taskRunProcess.ts(3 hunks)packages/core/src/v3/errors.ts(2 hunks)packages/core/src/v3/heartbeats-api.ts(1 hunks)packages/core/src/v3/heartbeats/api.ts(1 hunks)packages/core/src/v3/heartbeats/manager.ts(1 hunks)packages/core/src/v3/heartbeats/types.ts(1 hunks)packages/core/src/v3/index.ts(1 hunks)packages/core/src/v3/links.ts(1 hunks)packages/core/src/v3/utils/globals.ts(2 hunks)packages/core/src/v3/workers/index.ts(1 hunks)packages/trigger-sdk/src/v3/heartbeats.ts(1 hunks)packages/trigger-sdk/src/v3/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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:
packages/core/src/v3/heartbeats-api.tspackages/core/src/v3/index.tspackages/core/src/v3/workers/index.tspackages/core/src/v3/errors.tsapps/webapp/app/env.server.tspackages/core/src/v3/utils/globals.tspackages/trigger-sdk/src/v3/heartbeats.tspackages/core/src/v3/links.tspackages/cli-v3/src/executions/taskRunProcess.tspackages/core/src/v3/heartbeats/types.tspackages/trigger-sdk/src/v3/index.tspackages/cli-v3/src/entryPoints/dev-run-worker.tsinternal-packages/run-engine/src/engine/index.tspackages/core/src/v3/heartbeats/api.tsinternal-packages/run-engine/src/engine/errors.tspackages/core/src/v3/heartbeats/manager.tsinternal-packages/run-engine/src/engine/systems/runAttemptSystem.tspackages/cli-v3/src/entryPoints/managed-run-worker.tspackages/cli-v3/src/entryPoints/dev-run-controller.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/heartbeats-api.tspackages/core/src/v3/index.tspackages/core/src/v3/workers/index.tspackages/core/src/v3/errors.tsapps/webapp/app/env.server.tspackages/core/src/v3/utils/globals.tspackages/core/src/v3/links.tspackages/core/src/v3/heartbeats/types.tspackages/core/src/v3/heartbeats/api.tspackages/core/src/v3/heartbeats/manager.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/env.server.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/env.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/env.server.ts
🧠 Learnings (6)
📚 Learning: 2024-10-18T15:41:52.352Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1418
File: packages/core/src/v3/errors.ts:364-371
Timestamp: 2024-10-18T15:41:52.352Z
Learning: In `packages/core/src/v3/errors.ts`, within the `taskRunErrorEnhancer` function, `error.message` is always defined, so it's safe to directly call `error.message.includes("SIGTERM")` without additional checks.
Applied to files:
packages/core/src/v3/errors.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Import Trigger.dev APIs from "trigger.dev/sdk/v3" when writing tasks or related utilities
Applied to files:
packages/trigger-sdk/src/v3/heartbeats.tspackages/trigger-sdk/src/v3/index.tspackages/cli-v3/src/entryPoints/dev-run-worker.tspackages/cli-v3/src/entryPoints/managed-run-worker.ts
📚 Learning: 2025-08-29T10:06:49.293Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.293Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from trigger.dev/core’s package.json
Applied to files:
packages/trigger-sdk/src/v3/index.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Define tasks using task({ id, run, ... }) with a unique id per project
Applied to files:
docs/troubleshooting.mdx
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : For idempotent child-task invocations, create and pass idempotencyKey (and optional TTL) when calling trigger()/batchTrigger() from tasks
Applied to files:
docs/troubleshooting.mdx
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use triggerAndWait() only from within a task context (not from generic app code) and handle result.ok or use unwrap() with error handling
Applied to files:
docs/troubleshooting.mdx
🧬 Code graph analysis (14)
packages/core/src/v3/heartbeats-api.ts (2)
packages/trigger-sdk/src/v3/heartbeats.ts (1)
heartbeats(41-44)packages/core/src/v3/heartbeats/api.ts (1)
HeartbeatsAPI(28-73)
packages/core/src/v3/errors.ts (1)
packages/core/src/v3/links.ts (1)
links(1-33)
packages/core/src/v3/utils/globals.ts (2)
packages/core/src/v3/heartbeats/api.ts (1)
HeartbeatsManager(70-72)packages/core/src/v3/heartbeats/types.ts (1)
HeartbeatsManager(1-8)
packages/trigger-sdk/src/v3/heartbeats.ts (1)
packages/core/src/v3/heartbeats-api.ts (1)
heartbeats(5-5)
packages/cli-v3/src/executions/taskRunProcess.ts (1)
packages/core/src/utils.ts (1)
tryCatch(5-18)
packages/core/src/v3/heartbeats/types.ts (2)
packages/core/src/v3/heartbeats/api.ts (1)
HeartbeatsManager(70-72)internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
id(1582-1589)
packages/cli-v3/src/entryPoints/dev-run-worker.ts (3)
packages/core/src/v3/heartbeats/manager.ts (1)
StandardHeartbeatsManager(5-77)packages/core/src/v3/heartbeats-api.ts (1)
heartbeats(5-5)packages/core/src/v3/idempotencyKeys.ts (1)
attemptKey(132-134)
internal-packages/run-engine/src/engine/index.ts (1)
packages/core/src/v3/index.ts (1)
formatDurationMilliseconds(35-35)
packages/core/src/v3/heartbeats/api.ts (2)
packages/core/src/v3/heartbeats/types.ts (1)
HeartbeatsManager(1-8)packages/core/src/v3/utils/globals.ts (3)
registerGlobal(21-37)unregisterGlobal(45-51)getGlobal(39-43)
internal-packages/run-engine/src/engine/errors.ts (1)
packages/core/src/v3/schemas/common.ts (4)
TaskRunError(200-205)TaskRunError(207-207)RuntimeEnvironmentType(547-552)RuntimeEnvironmentType(554-555)
packages/core/src/v3/heartbeats/manager.ts (3)
packages/core/src/v3/heartbeats/api.ts (1)
HeartbeatsManager(70-72)packages/core/src/v3/heartbeats/types.ts (1)
HeartbeatsManager(1-8)internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
id(1582-1589)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (2)
internal-packages/run-engine/src/engine/errors.ts (2)
ServiceValidationError(69-78)runStatusFromError(5-67)internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts (2)
EnhancedExecutionSnapshot(20-25)status(428-449)
packages/cli-v3/src/entryPoints/managed-run-worker.ts (4)
packages/core/src/v3/heartbeats/manager.ts (1)
StandardHeartbeatsManager(5-77)packages/core/src/v3/heartbeats-api.ts (1)
heartbeats(5-5)packages/trigger-sdk/src/v3/heartbeats.ts (1)
heartbeats(41-44)internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
id(1582-1589)
packages/cli-v3/src/entryPoints/dev-run-controller.ts (1)
packages/core/src/v3/errors.ts (1)
SuspendedProcessError(527-533)
⏰ 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). (22)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
🔇 Additional comments (24)
packages/core/src/v3/links.ts (1)
17-18: LGTM: new troubleshooting linkMatches the new doc anchor.
packages/cli-v3/src/entryPoints/dev-run-controller.ts (5)
7-7: Import of SuspendedProcessErrorOK; used to special-case suspend exits.
528-533: Suspend path handling looks correctGracefully exits without failing the run on suspend.
598-599: Run attempt lifecycle flag initializationGood reset before execution.
670-671: Marking completion state before returning processAppropriate to guard double-completion flows.
792-822: Idempotent shutdown helper: good additionCentralizes teardown; prevents duplicate onFinished calls.
packages/core/src/v3/workers/index.ts (1)
32-32: Good re-export; keeps workers surface coherent.No issues. This enables CLI workers to import the standard manager from a stable path.
packages/trigger-sdk/src/v3/index.ts (1)
18-18: SDK now surfaces heartbeats — looks correct.Additive, no breaking changes. Please ensure docs/examples reference
import { heartbeats } from "trigger.dev/sdk/v3".If helpful, I can open a docs follow-up PR to add usage examples. Want me to draft it?
packages/core/src/v3/heartbeats-api.ts (1)
1-5: Singleton entrypoint is fine and side-effect free when unused.Naming aligns with other v3 “-api” modules; no default export; tree-shakeable. LGTM.
packages/core/src/v3/index.ts (1)
21-21: Publicly exposing heartbeats API via v3 index is correct.Additive export; no ordering hazards observed.
packages/core/src/v3/utils/globals.ts (1)
3-3: Key name consistency verified
TheAPI_NAMEconstant ("heartbeats") inheartbeats/api.tsmatches the["heartbeats"]slot inglobals.ts. No changes needed.packages/core/src/v3/errors.ts (1)
285-347: Confirm stalled-executing errors are intended to be retriable
internal-packages/run-engine/src/engine/retrying.ts and apps/webapp/app/v3/services/completeAttempt.server.ts both treatTASK_RUN_STALLED_EXECUTING(and its wait-points variant) as retriable—mapping to CANCELED in DEV and triggering retries. Verify this behavior is desired to avoid excessive churn in DEV.internal-packages/run-engine/src/engine/errors.ts (1)
5-8: Verified no stale 1-arg calls remain. All invocations of runStatusFromError now pass two arguments.packages/trigger-sdk/src/v3/heartbeats.ts (2)
30-32: Good wrapper: awaitable yield is correctly delegatedThe wrapper cleanly forwards to the core API and keeps the public surface small.
41-44: Public API surface looks goodMinimal, intuitive, aligns with docs.
packages/cli-v3/src/entryPoints/dev-run-worker.ts (2)
563-564: Heartbeat lifecycle closed correctlyStopping the heartbeat in
finallyensures no leaks between runs.
698-700: Listener wiring is correct and awaits IPC sendThe await prevents message pile-up; manager already guards/logs failures internally.
packages/cli-v3/src/entryPoints/managed-run-worker.ts (2)
299-300: Reset covers heartbeats as wellGood inclusion to ensure clean state before next run.
735-737: Listener registration LGTMHeartbeats emit via IPC on schedule; matches core manager contract.
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (4)
694-696: Good guard: prevent succeeding an already finished runThis avoids double-finishing edge cases under races.
851-853: Good guard: prevent failing an already finished runSame rationale; this is a safe, defensive check.
933-941: Passing latestSnapshot object through failure path is cleaner and correctUsing EnhancedExecutionSnapshot ensures correct previousSnapshotId and environment-aware status mapping.
Also applies to: 1452-1476, 1522-1522
873-914: Confirm forceRequeue-driven runAttemptFailed emission semantics
forceRequeue always emits a runAttemptFailed event (runAttemptSystem.ts L873–914) even if the final outcome is fail_run or cancel_run; verify downstream analytics/UX won’t double-report or confuse terminal failure signals.packages/core/src/v3/heartbeats/manager.ts (1)
1-78: Node-only contexts confirmed;node:timers/promisesimports are safe. StandardHeartbeatsManager is only instantiated in CLI entry points and server-side (.server.ts) code with no client-side TSX or browser bundles, so no bundler breakage will occur.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
packages/core/src/v3/heartbeats/api.ts (1)
6-24: Resolved: Noop manager matches the interface nowstartHeartbeat/stopHeartbeat are sync and yield returns Promise. Looks good.
packages/cli-v3/src/entryPoints/managed-run-worker.ts (2)
533-534: Prefer attempt-level heartbeat IDs for parity and cleaner lifecycleUse attemptKey(execution) if available so heartbeats tie to attempts, not runs.
- standardHeartbeatsManager.startHeartbeat(_execution.run.id); + // Prefer attempt-level ID to avoid collisions across retries + standardHeartbeatsManager.startHeartbeat(attemptKey(execution));To locate attemptKey in the repo (and confirm import path), run:
#!/bin/bash rg -nP -C2 '\battemptKey\s*\(' --type=ts
139-143: Sanitize HEARTBEAT_INTERVAL_MS to avoid 0/NaN tight loopsClamp and default the interval before constructing the manager.
-const standardHeartbeatsManager = new StandardHeartbeatsManager( - parseInt(heartbeatIntervalMs ?? "30000", 10) -); +const intervalMs = (() => { + const n = Number(heartbeatIntervalMs); + return Number.isFinite(n) && n > 0 ? Math.max(250, Math.floor(n)) : 30_000; +})(); +const standardHeartbeatsManager = new StandardHeartbeatsManager(intervalMs); heartbeats.setGlobalManager(standardHeartbeatsManager);packages/core/src/v3/heartbeats/manager.ts (2)
1-4: Avoid Node-only timers in core; make timers isomorphicnode:timers/promises breaks web/edge bundling. Replace with an abortable sleep helper and an async while loop.
import { tryCatch } from "../tryCatch.js"; import { HeartbeatsManager } from "./types.js"; -import { setInterval, setImmediate, setTimeout } from "node:timers/promises"; + +// Isomorphic helpers (work in Node, browser, and edge) +const delay = (ms: number) => + new Promise<void>((resolve) => globalThis.setTimeout(resolve, ms)); + +function abortableSleep(ms: number, signal: AbortSignal): Promise<void> { + return new Promise<void>((resolve, reject) => { + if (signal.aborted) return reject(Object.assign(new Error("Aborted"), { name: "AbortError" })); + const id = globalThis.setTimeout(() => { + signal.removeEventListener("abort", onAbort); + resolve(); + }, ms); + function onAbort() { + globalThis.clearTimeout(id); + reject(Object.assign(new Error("Aborted"), { name: "AbortError" })); + } + signal.addEventListener("abort", onAbort, { once: true }); + }); +}
40-58: Refactor loop to isomorphic, abortable while-loop and only update lastHeartbeat on successRemoves Node dependency and avoids reporting lastHeartbeat when the send failed.
- private async startHeartbeatLoop(id: string, signal: AbortSignal) { - try { - for await (const _ of setInterval(this.intervalInMs, undefined, { - signal, - })) { - if (this.listener) { - const [error] = await tryCatch(this.listener(id)); - this.lastHeartbeatDate = new Date(); - - if (error) { - console.error("Failed to send HEARTBEAT message", { error: String(error) }); - } - } - } - } catch (error) { - // Ignore errors as we expect them to be thrown when the heartbeat is stopped - // And since we tryCatch inside the loop, we don't need to handle any other errors here - } - } + private async startHeartbeatLoop(id: string, signal: AbortSignal) { + try { + while (!signal.aborted) { + await abortableSleep(this.intervalInMs, signal); + if (signal.aborted) break; + if (this.listener) { + const [error] = await tryCatch(this.listener(id)); + if (error) { + console.error("Failed to send HEARTBEAT message", { error: String(error) }); + } else { + this.lastHeartbeatDate = new Date(); + } + } + } + } catch { + // Expected on abort; no-op. + } + }packages/cli-v3/src/entryPoints/dev-run-controller.ts (1)
53-55: SIGTERM handler uses unstable reference; off() won’t remove it and this-binding is fragileRegistering with
this.sigterm.bind(this)creates a new function, butoff("SIGTERM", this.sigterm)uses a different reference → leak and potential wrongthis. Use a stable field.Apply:
@@ - private isCompletingRun = false; - private isShuttingDown = false; + private isCompletingRun = false; + private isShuttingDown = false; + private readonly onSigterm = () => { void this.sigterm(); }; @@ - process.on("SIGTERM", this.sigterm.bind(this)); + process.on("SIGTERM", this.onSigterm); @@ - process.off("SIGTERM", this.sigterm); + process.off("SIGTERM", this.onSigterm);Also applies to: 126-127, 859-860
🧹 Nitpick comments (3)
packages/core/src/v3/heartbeats/api.ts (2)
41-47: Expose allowOverride for controlled re-registrationSome runtimes may need to swap managers (dev ↔ prod). Consider threading allowOverride through to registerGlobal.
- public setGlobalManager(manager: HeartbeatsManager): boolean { - return registerGlobal(API_NAME, manager); - } + public setGlobalManager( + manager: HeartbeatsManager, + { allowOverride = false }: { allowOverride?: boolean } = {} + ): boolean { + return registerGlobal(API_NAME, manager, allowOverride); + }
49-56: Clarify reset semantics (resets then disables)reset() currently clears the underlying manager and unregisters the API. That’s a bit surprising for a method named reset. Either document the behavior or split into reset() and resetAndDisable().
+ /** + * Resets the current manager state and then disables the global API. + * NOTE: Call setGlobalManager(...) again to re-enable. + */ public reset() { this.#getManager().reset(); this.disable(); }packages/cli-v3/src/entryPoints/managed-run-worker.ts (1)
719-724: Also sanitize usage heartbeat interval (minor)Mirror the clamp/default logic to avoid overly chatty usage heartbeats in misconfigurations.
- const prodUsageManager = new ProdUsageManager(devUsageManager, { - heartbeatIntervalMs: usageIntervalMs ? parseInt(usageIntervalMs, 10) : undefined, + const prodUsageManager = new ProdUsageManager(devUsageManager, { + heartbeatIntervalMs: (() => { + if (!usageIntervalMs) return undefined; + const n = Number(usageIntervalMs); + return Number.isFinite(n) && n > 0 ? Math.max(1_000, Math.floor(n)) : undefined; + })(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/cli-v3/src/entryPoints/dev-run-controller.ts(9 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(7 hunks)packages/core/src/v3/heartbeats/api.ts(1 hunks)packages/core/src/v3/heartbeats/manager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
packages/cli-v3/src/entryPoints/dev-run-controller.tspackages/core/src/v3/heartbeats/manager.tspackages/core/src/v3/heartbeats/api.tspackages/cli-v3/src/entryPoints/managed-run-worker.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/heartbeats/manager.tspackages/core/src/v3/heartbeats/api.ts
🧠 Learnings (2)
📚 Learning: 2024-10-18T15:41:52.352Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1418
File: packages/core/src/v3/errors.ts:364-371
Timestamp: 2024-10-18T15:41:52.352Z
Learning: In `packages/core/src/v3/errors.ts`, within the `taskRunErrorEnhancer` function, `error.message` is always defined, so it's safe to directly call `error.message.includes("SIGTERM")` without additional checks.
Applied to files:
packages/cli-v3/src/entryPoints/dev-run-controller.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Import Trigger.dev APIs from "trigger.dev/sdk/v3" when writing tasks or related utilities
Applied to files:
packages/cli-v3/src/entryPoints/managed-run-worker.ts
🧬 Code graph analysis (4)
packages/cli-v3/src/entryPoints/dev-run-controller.ts (1)
packages/core/src/v3/errors.ts (1)
SuspendedProcessError(527-533)
packages/core/src/v3/heartbeats/manager.ts (2)
packages/core/src/v3/heartbeats/api.ts (1)
HeartbeatsManager(70-72)packages/core/src/v3/heartbeats/types.ts (1)
HeartbeatsManager(1-8)
packages/core/src/v3/heartbeats/api.ts (2)
packages/core/src/v3/heartbeats/types.ts (1)
HeartbeatsManager(1-8)packages/core/src/v3/utils/globals.ts (3)
registerGlobal(21-37)unregisterGlobal(45-51)getGlobal(39-43)
packages/cli-v3/src/entryPoints/managed-run-worker.ts (5)
packages/core/src/v3/heartbeats/manager.ts (1)
StandardHeartbeatsManager(5-78)packages/core/src/v3/workers/index.ts (1)
StandardHeartbeatsManager(32-32)packages/core/src/v3/heartbeats-api.ts (1)
heartbeats(5-5)packages/trigger-sdk/src/v3/heartbeats.ts (1)
heartbeats(41-44)internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
id(1582-1589)
⏰ 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 (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 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 (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- 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 (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
packages/cli-v3/src/entryPoints/managed-run-worker.ts (3)
561-564: Good: always stop heartbeat in finallyPrevents leaks if execution throws or is cancelled.
296-301: Good: reset heartbeats with the rest of the environmentEnsures no stale timers or state across runs.
737-739: LGTM: event-driven heartbeat wiringListener sends TASK_HEARTBEAT on each tick; aligns with the new manager.
packages/cli-v3/src/entryPoints/dev-run-controller.ts (3)
7-7: Importing SuspendedProcessError — good usageUsing SuspendedProcessError to distinguish suspend vs. fail paths makes the error flow clearer.
598-599: isCompletingRun flags — appropriate gatingSetting/resetting around execute/complete provides a simple guard against double-finish races.
Also applies to: 670-671
528-533: SuspendedProcessError branch — appropriate fast-exitTreating a suspension as a non-error path and finishing without completion submission is correct.
| if (this.isCompletingRun) { | ||
| logger.debug("Run is finished but we're completing it, skipping"); | ||
| return; | ||
| } | ||
|
|
||
| await this.exitTaskRunProcessWithoutFailingRun({ | ||
| flush: true, | ||
| reason: "already-finished", | ||
| }); | ||
| return; |
There was a problem hiding this comment.
Process not returned to pool on FINISHED → resource leak and pool starvation
On FINISHED (when not already completing), you suspend and call shutdown, but shutdownExecution doesn’t return the process to the pool or unsubscribe. This leaks the borrowed process and leaves the SIGTERM listener installed.
Apply:
@@
- await this.exitTaskRunProcessWithoutFailingRun({
+ await this.exitTaskRunProcessWithoutFailingRun({
flush: true,
reason: "already-finished",
});
@@
- private async exitTaskRunProcessWithoutFailingRun({
+ private async exitTaskRunProcessWithoutFailingRun({
flush,
reason,
}: {
flush: boolean;
reason: string;
}) {
- await this.taskRunProcess?.suspend({ flush });
-
- // No services should be left running after this line - let's make sure of it
- this.shutdownExecution(`exitTaskRunProcessWithoutFailingRun: ${reason}`);
+ try {
+ await this.taskRunProcess?.suspend({ flush });
+ } catch (error) {
+ logger.debug("Failed to suspend task run process", { error });
+ }
+ // No services should be left running after this line - let's make sure of it
+ await this.shutdownExecution(`exitTaskRunProcessWithoutFailingRun: ${reason}`);
}
@@
- private shutdownExecution(reason: string) {
+ private async shutdownExecution(reason: string) {
if (this.isShuttingDown) {
logger.debug(`[shutdown] ${reason} (already shutting down)`, {
newReason: reason,
});
return;
}
logger.debug(`[shutdown] ${reason}`);
this.isShuttingDown = true;
+ // Detach signal handler early
+ process.off("SIGTERM", this.onSigterm);
+
this.snapshotPoller.stop();
- this.opts.onFinished();
- this.taskRunProcess?.unsafeDetachEvtHandlers();
+ // Unsubscribe from run notifications if currently in RUN
+ if (this.state.phase === "RUN") {
+ try {
+ this.unsubscribeFromRunNotifications(this.state);
+ } catch (error) {
+ logger.debug("[shutdown] Failed to unsubscribe from run notifications", { error });
+ }
+ this.state = { phase: "IDLE" };
+ }
+
+ // Return process to pool if we still hold it
+ if (this.taskRunProcess) {
+ try {
+ const version = this.opts.worker.serverWorker?.version || "unknown";
+ await this.opts.taskRunProcessPool.returnProcess(this.taskRunProcess, version);
+ } catch (error) {
+ logger.debug("[shutdown] Failed to return task run process to pool", { error });
+ } finally {
+ this.taskRunProcess = undefined;
+ }
+ }
+
+ this.opts.onFinished();
}Also applies to: 792-804, 805-821
🤖 Prompt for AI Agents
In packages/cli-v3/src/entryPoints/dev-run-controller.ts around lines 360-369,
when handling FINISHED you call exitTaskRunProcessWithoutFailingRun but do not
return the borrowed process to the pool or unsubscribe the SIGTERM listener,
leaking the process and leaving the listener installed; fix by ensuring the
borrowed process is released and listeners removed before returning — either
update shutdownExecution/exitTaskRunProcessWithoutFailingRun to call the
existing releaseProcessToPool/unsubscribeSignalHandlers (or equivalent) on
success, or explicitly call this.releaseProcessToPool() and remove the SIGTERM
listener here before the return; apply the same change to the other two spots
noted (lines ~792-804 and ~805-821).
| taskRunProcess.unsafeDetachEvtHandlers(); | ||
|
|
||
| taskRunProcess.onTaskRunHeartbeat.attach(async (runId) => { | ||
| if (!this.runFriendlyId || !this.snapshotFriendlyId) { | ||
| logger.debug("[DevRunController] Skipping heartbeat, no run ID or snapshot ID"); | ||
| return; | ||
| } | ||
|
|
||
| logger.debug("[DevRunController] Sending heartbeat"); | ||
|
|
||
| const response = await this.httpClient.dev.heartbeatRun( | ||
| this.runFriendlyId, | ||
| this.snapshotFriendlyId, | ||
| { | ||
| cpu: 0, | ||
| memory: 0, | ||
| } | ||
| ); | ||
|
|
||
| if (!response.success) { | ||
| logger.debug("[DevRunController] Heartbeat failed", { error: response.error }); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap heartbeat calls in try/catch to avoid unhandled rejections
External call may throw (network, cancellation). Unhandled promise rejections can crash Node or degrade reliability.
Apply:
- taskRunProcess.onTaskRunHeartbeat.attach(async (runId) => {
- if (!this.runFriendlyId || !this.snapshotFriendlyId) {
- logger.debug("[DevRunController] Skipping heartbeat, no run ID or snapshot ID");
- return;
- }
-
- logger.debug("[DevRunController] Sending heartbeat");
-
- const response = await this.httpClient.dev.heartbeatRun(
- this.runFriendlyId,
- this.snapshotFriendlyId,
- {
- cpu: 0,
- memory: 0,
- }
- );
-
- if (!response.success) {
- logger.debug("[DevRunController] Heartbeat failed", { error: response.error });
- }
- });
+ taskRunProcess.onTaskRunHeartbeat.attach(async (runId) => {
+ try {
+ if (!this.runFriendlyId || !this.snapshotFriendlyId) {
+ logger.debug("[DevRunController] Skipping heartbeat, no run ID or snapshot ID");
+ return;
+ }
+ logger.debug("[DevRunController] Sending heartbeat");
+ const response = await this.httpClient.dev.heartbeatRun(
+ this.runFriendlyId,
+ this.snapshotFriendlyId,
+ { cpu: 0, memory: 0 }
+ );
+ if (!response.success) {
+ logger.debug("[DevRunController] Heartbeat failed", { error: response.error });
+ }
+ } catch (error) {
+ logger.debug("[DevRunController] Heartbeat threw", { error });
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| taskRunProcess.unsafeDetachEvtHandlers(); | |
| taskRunProcess.onTaskRunHeartbeat.attach(async (runId) => { | |
| if (!this.runFriendlyId || !this.snapshotFriendlyId) { | |
| logger.debug("[DevRunController] Skipping heartbeat, no run ID or snapshot ID"); | |
| return; | |
| } | |
| logger.debug("[DevRunController] Sending heartbeat"); | |
| const response = await this.httpClient.dev.heartbeatRun( | |
| this.runFriendlyId, | |
| this.snapshotFriendlyId, | |
| { | |
| cpu: 0, | |
| memory: 0, | |
| } | |
| ); | |
| if (!response.success) { | |
| logger.debug("[DevRunController] Heartbeat failed", { error: response.error }); | |
| } | |
| }); | |
| taskRunProcess.unsafeDetachEvtHandlers(); | |
| taskRunProcess.onTaskRunHeartbeat.attach(async (runId) => { | |
| try { | |
| if (!this.runFriendlyId || !this.snapshotFriendlyId) { | |
| logger.debug("[DevRunController] Skipping heartbeat, no run ID or snapshot ID"); | |
| return; | |
| } | |
| logger.debug("[DevRunController] Sending heartbeat"); | |
| const response = await this.httpClient.dev.heartbeatRun( | |
| this.runFriendlyId, | |
| this.snapshotFriendlyId, | |
| { cpu: 0, memory: 0 } | |
| ); | |
| if (!response.success) { | |
| logger.debug("[DevRunController] Heartbeat failed", { error: response.error }); | |
| } | |
| } catch (error) { | |
| logger.debug("[DevRunController] Heartbeat threw", { error }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/cli-v3/src/entryPoints/dev-run-controller.ts around lines 620 to
643, the async heartbeatRun call is not wrapped in try/catch so
network/cancellation errors can produce unhandled promise rejections; wrap the
await this.httpClient.dev.heartbeatRun(...) in a try/catch, log the caught error
(with context like run/snapshot IDs) and handle it similarly to non-success
responses (e.g., debug/error log and early return), ensuring the event handler
never throws.
| constructor(private readonly intervalInMs: number) {} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp interval and avoid parameter property for sanitization
Ensure a sane minimum interval and integer value.
-export class StandardHeartbeatsManager implements HeartbeatsManager {
+export class StandardHeartbeatsManager implements HeartbeatsManager {
@@
- constructor(private readonly intervalInMs: number) {}
+ private readonly intervalInMs: number;
+
+ constructor(intervalInMs: number) {
+ const n = Math.floor(Number(intervalInMs));
+ this.intervalInMs = Number.isFinite(n) && n > 0 ? Math.max(250, n) : 1_000;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(private readonly intervalInMs: number) {} | |
| export class StandardHeartbeatsManager implements HeartbeatsManager { | |
| private readonly intervalInMs: number; | |
| constructor(intervalInMs: number) { | |
| const n = Math.floor(Number(intervalInMs)); | |
| // Use a sane minimum (250ms) and default to 1000ms on invalid input | |
| this.intervalInMs = Number.isFinite(n) && n > 0 | |
| ? Math.max(250, n) | |
| : 1_000; | |
| } | |
| // …rest of class… | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/v3/heartbeats/manager.ts around lines 11-12, the
constructor currently uses a parameter property and does not sanitize the
interval; change it to accept the raw value, parse it to an integer (e.g.
Number/parseInt then Math.floor), default to a sane value if NaN, and clamp it
to a minimum (e.g. const MIN_INTERVAL_MS = 50) before assigning to a private
readonly field; do not use the parameter property for direct assignment so the
stored interval is always an integer >= MIN_INTERVAL_MS.
| async yield(): Promise<void> { | ||
| if (!this.lastHeartbeatYieldTime) { | ||
| return; | ||
| } | ||
|
|
||
| // Only call setImmediate if we haven't yielded in the last interval | ||
| if (Date.now() - this.lastHeartbeatYieldTime >= this.intervalInMs) { | ||
| // await setImmediate(); | ||
| await setTimeout(24); | ||
|
|
||
| this.lastHeartbeatYieldTime = Date.now(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make yield cross-platform (no node:timers/promises)
Use a tiny setTimeout-based yield and keep the throttle.
async yield(): Promise<void> {
if (!this.lastHeartbeatYieldTime) {
return;
}
- // Only call setImmediate if we haven't yielded in the last interval
+ // Only yield if we haven't yielded in the last interval
if (Date.now() - this.lastHeartbeatYieldTime >= this.intervalInMs) {
- // await setImmediate();
- await setTimeout(24);
+ await delay(24);
this.lastHeartbeatYieldTime = Date.now();
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/core/src/v3/heartbeats/manager.ts around lines 17 to 29, replace the
node-specific await setTimeout(24) with a tiny cross-platform promise-based
yield; keep the existing throttle logic intact. Concretely, change the awaited
call to await new Promise(resolve => setTimeout(resolve, 0)) (or a small
configurable ms if you prefer), ensuring lastHeartbeatYieldTime is still updated
only when the throttle condition passes and no node:timers/promises imports are
required.
No description provided.