v4: implement onCancel callbacks #2022
Conversation
🦋 Changeset detectedLatest commit: bbfa96c The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update introduces a comprehensive cancellation mechanism across the task execution lifecycle. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant Worker
participant Executor
participant Task
participant LifecycleHooks
User->>WebApp: Request to cancel a running task
WebApp->>Worker: Send CANCEL IPC message (with timeout)
Worker->>Executor: AbortController aborts signal
Worker->>LifecycleHooks: Call registered onCancel hooks
LifecycleHooks-->>Task: Invoke onCancel (global/task-specific)
Executor-->>Task: Task run observes abort signal
Task-->>Executor: Task run aborts or cleans up
Executor-->>Worker: Task run completes or aborts
Worker->>WebApp: Acknowledge cancellation
Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
|
|
1 similar comment
|
|
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
|
😱 Found 3 issues. Time to roll up your sleeves! 😱 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (18)
references/hello-world/src/trigger/init.ts (1)
9-11: Good example implementation of global onCancel handlerThis demonstrates the proper usage of the new onCancel hook as a global handler. The async function signature and logging pattern matches other lifecycle hooks in the file.
Consider adding an example that shows how to perform cleanup operations during cancellation, such as releasing resources or stopping long-running operations, which would showcase the practical utility of this feature.
tasks.onCancel(async ({ ctx, payload }) => { logger.info("Hello, world from the global cancel", { ctx, payload }); + // Example: Clean up resources when tasks are cancelled + await releaseAnyHeldResources(); });references/hello-world/src/trigger/example.ts (2)
240-246:onCancelhook may hang indefinitely byawait-ingrunPromiseand adding a 10 s sleepIf the run was aborted part-way through,
runPromisemight never resolve (e.g. if the run is stuck in un-cancellable I/O), causing the hook itself to block forever.
In addition, the extraawait setTimeout(10_000)delays shutdown and defeats fast-fail semantics.Consider:
- const output = await runPromise; - ... - await setTimeout(10_000); + let output: TOutput | undefined; + try { + output = await Promise.race([runPromise, Promise.resolve(undefined)]); + } catch (_) { + /* swallow – already cancelling */ + } + // Remove the additional 10 s delay to let the worker exit promptly
210-213: Leverage full hook parameters for richer diagnostics
onCancelreceives{ ctx, signal, init, runPromise, … }.
Logging only the payload misses useful context (run id, attempt, etc.). Consider loggingctxand checkingsignal.abortedto aid debugging.packages/core/src/v3/types/tasks.ts (1)
92-114: Makingsignalmandatory is a breaking API changeAll existing user-defined
run,init,middleware,startetc. implementations must now accept a non-optionalsignal.
Ensure:
- The change is documented in release notes.
- Code-mods or compile-time errors guide users to update their signatures.
packages/core/src/v3/timeout/api.ts (2)
6-9: Clarify the no-op semantics ofabortAfterTimeout
abortAfterTimeout()inNoopTimeoutManagerignores thetimeoutInSecondsargument and simply returns a freshAbortController.
That’s perfectly fine for a noop implementation, but it can be confusing to future readers and may trigger “unused-parameter” lint warnings.- abortAfterTimeout(timeoutInSeconds?: number): AbortController { - return new AbortController(); + // Intentionally ignores `timeoutInSeconds` – this manager never aborts. + abortAfterTimeout(_timeoutInSeconds?: number): AbortController { + return new AbortController(); }
27-33: Consider providing a non-undefined fallback forsignal
signalcurrently forwards whatever the activeTimeoutManagerexposes.
If a custom manager forgets to define.signal, callers will suddenly receiveundefined, which forces extra defensive checks throughout the codebase.A tiny fallback avoids that foot-gun:
- public get signal(): AbortSignal | undefined { - return this.#getManager().signal; + public get signal(): AbortSignal | undefined { + return this.#getManager().signal ?? undefined; // <- guarantees the property exists }(Or return a dummy
AbortSignalsuch asnew AbortController().signal.)
Not critical, but it makes the API more predictable.packages/cli-v3/src/entryPoints/managed-run-worker.ts (2)
410-420: Detach the timeout listener to avoid leaking closuresThe listener that forwards
timeoutController.signal.aborttocancelController.abort()is never removed.
For a long-lived worker processing many runs, this will accumulate one redundant listener per execution.A quick fix inside the same block:
const timeoutController = timeout.abortAfterTimeout(execution.run.maxDuration); const onTimeoutAbort = () => { if (_isCancelled || cancelController.signal.aborted) { return; } cancelController.abort(timeoutController.signal.reason); }; timeoutController.signal.addEventListener("abort", onTimeoutAbort); // 🧹 remove on finish try { const { result } = await executor.execute( execution, metadata, traceContext, cancelController.signal ); ... } finally { + timeoutController.signal.removeEventListener("abort", onTimeoutAbort); }
476-484: Send an acknowledgement back to the executor after handlingCANCELThe executor currently fires a
CANCELIPC message but never receives confirmation that the worker finished flushing / calling hooks.
If the controller side awaits such a message, it may hang indefinitely.Consider replying with a lightweight
CANCEL_HANDLED(or similar) message:await flushAll(timeoutInMs); await sender.send("CANCEL_HANDLED", { id: _execution?.run.id });Even if the executor doesn’t wait today, the explicit ack future-proofs the protocol.
references/d3-chat/src/trigger/chat.ts (2)
160-183:chunksis collected but never used
chunksis populated viaonChunk, yet it’s not referenced afterwards.
If it’s only for debugging, wrap it in a conditional log; otherwise consider removing it to save memory in long streams.
230-233: Avoid the banned{}typeBiome flags
{}as “any non-nullable value”.
Useunknownor an explicit record instead:-const chunks: TextStreamPart<{}>[] = []; +const chunks: TextStreamPart<Array<Record<string, unknown>>>[] = [];Or, if you don’t care about the tool payload, simply:
TextStreamPart<unknown>[].🧰 Tools
🪛 Biome (1.9.4)
[error] 230-230: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/cli-v3/src/entryPoints/dev-run-worker.ts (3)
413-426: Potential listener leak ontimeoutController.signalA new listener is attached for every run, but never removed. After many runs the process accumulates event listeners and Node.js will warn (“Possible EventEmitter memory leak”). Remove the listener in the
finallyblock that clears_isRunning:finally { + timeoutController.signal.removeEventListener("abort", onTimeoutAbort); _execution = undefined; _isRunning = false; }(where
onTimeoutAbortis the named callback you pass toaddEventListener).
480-488: Double invocation of cancel hooks
callCancelHooks()is invoked here, but the same hooks are also executed insideTaskExecutorwhen it detectsabortSignal.aborted. Unless the manager de-duplicates, listeners will fire twice.
Consider calling the hooks only after the executor finishes, or guard insidecallOnCancelHookListenersto ensure idempotency.
501-511: Timeout does not actually cancel long-running hooks
Promise.racewithsetTimeoutonly hides late resolution; the hook Promise still runs to completion in the background. If a hook never resolves, the process keeps running. Pass anAbortSignalto the hooks or reject/cleanup after the timeout:const controller = new AbortController(); const timeout = setTimeout(timeoutInMs).then(() => controller.abort()); await lifecycleHooks.callOnCancelHookListeners(controller.signal);This lets hook implementers respect the cancellation deadline.
packages/cli-v3/src/executions/taskRunProcess.ts (2)
107-116: Error handling incancel()hides IPC failuresThe blanket
catchjust logs toconsole.error, whereas the caller may need to know that cancellation failed (e.g. child already exited). Re-throw after logging so the supervisor can decide what to do:- } catch (err) { - console.error("Error cancelling task run process", { err }); + } catch (err) { + console.error("Error cancelling task run process", { err }); + throw err; }
231-235: Grace-period before SIGKILL would be safer
kill()is invoked immediately after sending theCANCELIPC message. If the child honours the cancel quickly this is fine, but in the common case it will still be shutting down and may be hard-killed. Consider:
- Send
CANCEL.- Wait e.g.
timeoutInMs + 2 sfor the child to exit voluntarily.- Only then escalate with
SIGKILL.This mirrors the graceful-shutdown pattern used elsewhere in the codebase.
packages/core/src/v3/lifecycleHooks/index.ts (1)
264-290: Public API docs & typings should be updated for new cancel hooksGreat to see full parity for
onCancelhooks. Please remember to:
- Add JSDoc comments so SDK consumers discover the new methods.
- Export the new
AnyOnCancelHookFunctionand related types from the package root to avoid forcing deep imports.No code change required here, just documentation/exposure.
packages/core/src/v3/lifecycleHooks/types.ts (1)
246-252: Consider replacingvoidwithundefinedin union typeThe return type union includes
void, which can be confusing in TypeScript union types. Consider using onlyundefinedfor consistency and clarity.- ) => undefined | void | Promise<undefined | void>; + ) => undefined | Promise<undefined | void>;🧰 Tools
🪛 Biome (1.9.4)
[error] 252-252: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/core/src/v3/workers/taskExecutor.ts (1)
422-437: Remove debug console.log in production codeThere's a console.log statement that appears to be debugging code. This should be removed before the PR is merged.
signal.addEventListener("abort", () => { if (typeof signal.reason === "string" && signal.reason.includes("cancel")) { - console.log("abortPromise: cancel"); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.changeset/real-rats-drop.md(1 hunks)apps/webapp/app/components/runs/v3/RunIcon.tsx(1 hunks)apps/webapp/app/v3/services/cancelTaskRun.server.ts(0 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(5 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(5 hunks)packages/cli-v3/src/executions/taskRunProcess.ts(3 hunks)packages/core/src/utils.ts(1 hunks)packages/core/src/v3/lifecycle-hooks-api.ts(1 hunks)packages/core/src/v3/lifecycleHooks/index.ts(2 hunks)packages/core/src/v3/lifecycleHooks/manager.ts(3 hunks)packages/core/src/v3/lifecycleHooks/types.ts(12 hunks)packages/core/src/v3/schemas/messages.ts(1 hunks)packages/core/src/v3/timeout/api.ts(3 hunks)packages/core/src/v3/timeout/types.ts(1 hunks)packages/core/src/v3/timeout/usageTimeoutManager.ts(3 hunks)packages/core/src/v3/types/tasks.ts(3 hunks)packages/core/src/v3/usage-api.ts(1 hunks)packages/core/src/v3/usage/devUsageManager.ts(1 hunks)packages/core/src/v3/workers/taskExecutor.ts(19 hunks)packages/core/test/taskExecutor.test.ts(1 hunks)packages/trigger-sdk/src/v3/hooks.ts(3 hunks)packages/trigger-sdk/src/v3/shared.ts(2 hunks)packages/trigger-sdk/src/v3/tasks.ts(2 hunks)references/d3-chat/src/trigger/chat.ts(5 hunks)references/hello-world/src/trigger/example.ts(2 hunks)references/hello-world/src/trigger/init.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/v3/services/cancelTaskRun.server.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/core/src/v3/timeout/usageTimeoutManager.ts (1)
packages/core/src/v3/usage/devUsageManager.ts (2)
sample(18-37)sample(53-55)
packages/trigger-sdk/src/v3/shared.ts (3)
packages/core/src/v3/lifecycle-hooks-api.ts (2)
lifecycleHooks(5-5)AnyOnCancelHookFunction(37-37)packages/core/src/v3/lifecycleHooks/types.ts (1)
AnyOnCancelHookFunction(254-254)packages/trigger-sdk/src/v3/hooks.ts (1)
AnyOnCancelHookFunction(29-29)
packages/core/src/v3/timeout/api.ts (1)
packages/core/src/v3/timeout/types.ts (1)
TimeoutManager(1-4)
packages/cli-v3/src/executions/taskRunProcess.ts (1)
packages/cli-v3/src/utilities/logger.ts (1)
logger(113-113)
packages/core/src/v3/lifecycleHooks/manager.ts (3)
packages/core/src/v3/lifecycle-hooks-api.ts (4)
RegisteredHookFunction(10-10)AnyOnWaitHookFunction(22-22)TaskWait(34-34)AnyOnCancelHookFunction(37-37)packages/core/src/v3/lifecycleHooks/types.ts (6)
RegisteredHookFunction(174-178)AnyOnWaitHookFunction(71-71)TaskWait(36-53)AnyOnCancelHookFunction(254-254)RegisterHookFunctionParams(169-172)LifecycleHooksManager(256-344)packages/core/src/v3/workers/taskExecutor.ts (2)
wait(453-523)wait(607-677)
🪛 Biome (1.9.4)
references/d3-chat/src/trigger/chat.ts
[error] 230-230: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/core/src/v3/lifecycleHooks/types.ts
[error] 252-252: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
🔇 Additional comments (36)
apps/webapp/app/components/runs/v3/RunIcon.tsx (1)
100-100: Consistent addition of onCancel hook renderingThe addition of "task-hook-onCancel" to the switch statement is implemented correctly, maintaining consistency with the other non-error lifecycle hooks that render using the FunctionIcon with text-text-dimmed styling.
.changeset/real-rats-drop.md (1)
1-5: LGTM - Clear and concise changeset entryThe changeset correctly specifies a patch update to the "@trigger.dev/sdk" package with a clear description of the new functionality being added.
packages/trigger-sdk/src/v3/tasks.ts (2)
11-11: Appropriate addition of onCancel importThe onCancel hook is correctly imported from the hooks module.
99-99: Correct export of onCancel in tasks objectThe onCancel hook is properly exposed in the tasks export object, making it available for users of the SDK.
packages/core/src/v3/usage-api.ts (1)
7-7: Type exports for usage monitoringThe addition of UsageMeasurement and UsageSample type exports is appropriate for tracking resource usage during task execution, which would be relevant for cancellation scenarios where resources need to be properly accounted for.
packages/core/src/v3/lifecycle-hooks-api.ts (1)
35-37: Properly extends the lifecycle hooks API with new cancel hook types.These new exported types (
TaskCancelHookParams,OnCancelHookFunction, andAnyOnCancelHookFunction) are essential for implementing the new cancellation hook support throughout the codebase. The addition follows the existing pattern established for other lifecycle hooks.packages/core/src/v3/usage/devUsageManager.ts (1)
77-79: Added defensive check before deleting measurement.This change prevents potential errors by ensuring the measurement exists in the map before attempting to delete it. This is particularly important in cancellation scenarios when measurements might already be cleaned up by other processes.
packages/core/src/v3/schemas/messages.ts (1)
246-251: Good addition of CANCEL message type to support task cancellation.The new message type with the
timeoutInMsparameter provides a clear protocol for signaling cancellation between workers and executors. This is a key component for implementing structured cancellation support as described in the PR.packages/core/test/taskExecutor.test.ts (1)
1908-1910: Ensures consistent abort signal availability in tests.This change guarantees that the task executor always receives a valid abort signal, even if none is provided. This is crucial for proper cancellation handling and aligns with the broader implementation of cancellation hooks.
packages/core/src/v3/timeout/types.ts (1)
2-2: Improved API for AbortController managementThe change to return
AbortControllerinstead of justAbortSignaland making the timeout parameter optional provides better flexibility for timeout management. This enhances the cancellation mechanism by giving callers more control over aborting operations.packages/trigger-sdk/src/v3/shared.ts (2)
45-45: Added import for cancel hook typeThe
AnyOnCancelHookFunctionimport aligns with the implementation of the new onCancel lifecycle hook.
1642-1646: Added support for onCancel lifecycle hookThe implementation follows the same pattern as other lifecycle hooks, properly registering task-specific cancel hooks with the lifecycle hooks manager.
packages/core/src/utils.ts (1)
20-40: Added helpful promise utility for cancellation supportThe
Deferred<T>type andpromiseWithResolvers<T>()function are well-implemented utilities that will help with external promise resolution control, which is particularly useful for cancellation scenarios.packages/trigger-sdk/src/v3/hooks.ts (3)
14-14: Added import for new cancel hook typeAdding the
AnyOnCancelHookFunctiontype import supports the new onCancel hook implementation.
29-29: Exported cancel hook type for SDK consumersExporting the
AnyOnCancelHookFunctiontype allows SDK users to properly type their cancel hook implementations.
137-147: Added onCancel global hook registrationThe new
onCancelhook follows the same pattern as other lifecycle hooks, with proper overloads to support both named and unnamed hooks. This implementation enables users to register cancellation handlers that will be invoked when a task is cancelled.packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
410-435:_isCancelledmust be cleared before measuring success pathThe success branch is gated by
if (_isRunning && !_isCancelled) { … }If
_isCancelledremainedtruefrom a previous run (see issue above), the worker would never sendTASK_RUN_COMPLETED, leaking the process and leaving the parent hanging. Ensure you reset_isCancelled = falsebefore every run or, better, derive this flag purely from the current controller’sabortedstate.packages/cli-v3/src/executions/taskRunProcess.ts (1)
123-126: Early return fromcleanup()may leave child process aliveIf
cleanup()is called while_isBeingCancelledis true, the function returns without killing the child (becausekill = trueis ignored). If the previouscancel()failed for any reason the worker could remain running indefinitely.
Consider waiting foronExitwith a timeout here, or fall back tokill("SIGKILL")after a grace period.packages/core/src/v3/lifecycleHooks/types.ts (4)
10-10: Signal parameter now required across all hook interfacesMaking
signala required parameter in all lifecycle hooks ensures that cancellation state is always available, improving the robustness of task execution.
233-244: Well-structured addition of cancellation hook parametersThe
TaskCancelHookParamsinterface follows the established pattern for other lifecycle hooks while adding the importantrunPromiseparameter, which allows hooks to observe the current task run state.
334-340: Well-designed cancel hook registration methodsThe implementation of cancel hook registration and retrieval methods follows the established pattern for other lifecycle hooks, ensuring consistency in the API.
342-343: Cancel listener methods match established patternsThese listener methods maintain consistency with other hook listeners, making the API intuitive for developers who are already familiar with the other lifecycle hooks.
packages/core/src/v3/lifecycleHooks/manager.ts (6)
16-16: Consistent import of new cancel hook typeThe import of
AnyOnCancelHookFunctionaligns with the pattern used for other lifecycle hook types.
60-62: Minor reordering of wait hooks fieldsThis is just a reordering of the wait hooks declarations with no functional change, keeping related fields together for better organization.
66-69: Good implementation of cancel hook storageThe private fields for global and task-specific cancel hooks follow the established pattern for other lifecycle hooks, maintaining consistency in the implementation.
71-77: Efficient implementation of cancel hook listenersThe listener registration and calling methods correctly use
Promise.allSettledto ensure all listeners are called even if some fail, which is crucial for cleanup operations.
412-441: Well-implemented cancel hook registration methodsThe implementation of the cancel hook registration and retrieval methods follows the exact same pattern as other lifecycle hooks, ensuring consistency and predictability in the API.
445-468: Comprehensive NoopLifecycleHooksManager implementationThe cancel hook methods in the NoopLifecycleHooksManager correctly implement the interface with no-op behavior, ensuring that the interface can be used in contexts where hooks are not needed.
packages/core/src/v3/workers/taskExecutor.ts (8)
1-1: Import Context for proper span context propagationThe additional import of
Contextfrom OpenTelemetry supports proper context propagation to child spans, which is important for maintaining trace continuity during cancellation.
54-54: Added promiseWithResolvers utilityThis utility simplifies creating a promise with exposed resolve/reject functions, which is used to track task execution status and provide it to cancellation hooks.
94-94: Signal parameter now requiredMaking the AbortSignal parameter required ensures that cancellation is always supported throughout the task execution lifecycle.
156-164: Well-designed runPromise implementationThe creation of a promise with resolvers to track task execution status is a clean approach. The catch handler prevents unhandled promise rejections, which is a good defensive programming practice.
165-174: Proper cancel hook listener registrationThe implementation registers a cancel hook listener that calls the cancel functions with the runPromise, allowing hooks to observe the current execution state.
198-199: Task error properly rejects runPromiseWhen a task fails, the runPromise is rejected, ensuring that cancellation hooks see the correct execution state.
248-249: Task success properly resolves runPromiseWhen a task succeeds, the runPromise is resolved, ensuring that cancellation hooks see the correct execution state.
525-605: Well-implemented callOnCancelFunctions methodThe method to call cancellation hooks follows the same pattern as other lifecycle hook methods, with proper tracing, error handling, and hook invocation. The method is comprehensive and consistent with the rest of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
references/hello-world/src/trigger/example.ts (1)
224-227:⚠️ Potential issueAvoid registering a global
tasks.onCancelhandler inside every task run.
tasks.onCancel()attaches a global listener that lives for the lifetime of the process. Calling it insidecancelTask.run()means every invocation of the task adds yet another listener, leading to unbounded growth and memory leaks.- tasks.onCancel(async () => { - logger.info("global task onCancel hook but inside of the run function baby!"); - }); +// Move this to module scope (executed once at start-up) +tasks.onCancel(async () => { + logger.info("Global tasks.onCancel hook"); +});packages/cli-v3/src/entryPoints/managed-run-worker.ts (1)
236-236:⚠️ Potential issue
AbortControllermust be re-created per run – current design causes false early cancellations.
cancelControlleris declaredconstat module scope and is aborted when aCANCELmessage is received. Every subsequent task run will reuse the same already-aborted signal, soexecutor.execute()will immediately observesignal.aborted === trueand bail out.-const cancelController = new AbortController(); +// Must be mutable so we can reset it before each EXECUTE_TASK_RUN +let cancelController: AbortController = new AbortController();Then, at the very start of the
EXECUTE_TASK_RUNhandler (right after we ensure we're not already running):+// Fresh controller for this execution +cancelController = new AbortController(); +_isCancelled = false;references/d3-chat/src/trigger/chat.ts (1)
233-236:⚠️ Potential issue
tasks.onCancelmust be registered outside the run function.Registering
tasks.onCancelinside the run function leaks handlers across executions. Every invocation of the task adds a new global listener that's never removed.Either move it to module scope or capture and clean up the disposer:
- tasks.onCancel(async () => { - // We have access to the chunks here - logger.info("interruptible-chat: task cancelled with chunks", { chunks }); - }); + const disposer = tasks.onCancel(async () => { + // We have access to the chunks here + logger.info("interruptible-chat: task cancelled with chunks", { chunks }); + }); + + try { + // ...existing code... + } finally { + // Remove the listener when the task is done + disposer(); + }
🧹 Nitpick comments (1)
references/d3-chat/src/trigger/chat.ts (1)
230-230: Fix the empty object type notation.Don't use
{}as a type. Prefer explicitly defining the object shape or use a more appropriate type.- const chunks: TextStreamPart<{}>[] = []; + const chunks: TextStreamPart<Record<string, never>>[] = [];🧰 Tools
🪛 Biome (1.9.4)
[error] 230-230: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/cli-v3/src/entryPoints/dev-run-worker.ts(5 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(5 hunks)packages/core/src/v3/timeout/usageTimeoutManager.ts(3 hunks)packages/core/src/v3/types/tasks.ts(3 hunks)references/d3-chat/src/trigger/chat.ts(5 hunks)references/hello-world/src/trigger/example.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/v3/timeout/usageTimeoutManager.ts
- packages/core/src/v3/types/tasks.ts
- packages/cli-v3/src/entryPoints/dev-run-worker.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
references/d3-chat/src/trigger/chat.ts (3)
packages/core/src/v3/timeout/usageTimeoutManager.ts (1)
signal(13-15)packages/core/src/v3/timeout/api.ts (1)
signal(27-29)packages/trigger-sdk/src/v3/tasks.ts (2)
schemaTask(83-83)tasks(87-104)
🪛 Biome (1.9.4)
references/d3-chat/src/trigger/chat.ts
[error] 230-230: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
references/hello-world/src/trigger/example.ts (3)
210-212: Well-implemented onCancel hook for hooksTask.The onCancel hook is implemented properly alongside other lifecycle hooks, providing consistent error handling for task cancellation.
228-236: Good use of abort signal with error handling.The code properly passes the signal to setTimeout and catches any abort errors that might be thrown when the task is cancelled, demonstrating best practices for handling cancellation during async operations.
246-257: Well-implemented task-specific onCancel hook.The onCancel hook demonstrates several important capabilities:
- Accessing the task payload
- Awaiting the original run's promise to get output
- Performing asynchronous work (with timeout)
- Proper logging throughout the cancellation process
This provides a good example of how to implement cancellation handling.
packages/cli-v3/src/entryPoints/managed-run-worker.ts (4)
409-409: Good implementation of combined abort signal.Using
AbortSignal.any()to race between cancellation and timeout signals is an excellent approach. This allows cancellation to preempt execution, regardless of whether it comes from an explicit cancel request or a timeout.
413-414: Proper check for cancellation state before sending result.The code correctly verifies both
_isRunningand!_isCancelledbefore sending the task completion message, which prevents sending completion messages for cancelled tasks.
460-468: Well-structured CANCEL message handler.The handler for the CANCEL message follows a good sequence:
- Sets the cancellation flag
- Aborts the controller with a reason
- Calls cancellation hooks with a timeout
- Stops usage measurement if active
- Flushes all telemetry data
This ensures proper cleanup and reporting when a task is cancelled.
478-488: Good implementation of callCancelHooks with timeout.The
callCancelHooksfunction properly usesPromise.racewith a timeout to ensure cancellation hooks don't hang indefinitely. This is important for system stability when user-defined hooks might be problematic.references/d3-chat/src/trigger/chat.ts (2)
113-113: Well-updated task signature with signal parameter.The run method has been properly updated to accept the signal parameter, enabling cancellation support.
160-183: Good implementation of chunk collection and abort signal passing.The code collects streamed text chunks while properly passing the abort signal to streamText, enabling cancellation of the streaming operation.
| } catch (error) { | ||
| if (error instanceof Error && error.name === "AbortError") { | ||
| // streamText will throw an AbortError if the signal is aborted, so we can handle it here | ||
| } else { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Function may return undefined, violating the declared return type.
When a cancellation error is caught, the function falls through without a return value, potentially returning undefined instead of the declared string.
if (error instanceof Error && error.name === "AbortError") {
// streamText will throw an AbortError if the signal is aborted, so we can handle it here
+ return ""; // Return empty string to satisfy the return type
} else {
throw 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.
| } catch (error) { | |
| if (error instanceof Error && error.name === "AbortError") { | |
| // streamText will throw an AbortError if the signal is aborted, so we can handle it here | |
| } else { | |
| throw error; | |
| } | |
| } | |
| } catch (error) { | |
| if (error instanceof Error && error.name === "AbortError") { | |
| // streamText will throw an AbortError if the signal is aborted, so we can handle it here | |
| return ""; // Return empty string to satisfy the return type | |
| } else { | |
| throw error; | |
| } | |
| } |
This PR adds the
onCancellifecycle hook, that allows you to do work when a run is cancelled:You can use the
onCancelhook along with thesignalpassed into the run function to interrupt a call to an external service, for example using the streamText function from the AI SDK:Summary by CodeRabbit
New Features
onCancel, enabling tasks to handle cancellation events with custom logic.Bug Fixes
Documentation