Fix: Prevent hung worker process on flush exceptions #1501
Conversation
🦋 Changeset detectedLatest commit: 1e5b400 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
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 pull request introduces several changes aimed at improving logging behavior in the application. A patch for "trigger.dev" is added to prevent specific log statements from causing worker processes to hang. Additionally, modifications are made to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/cli-v3/src/executions/taskRunProcess.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
nicktrn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
.changeset/ninety-teachers-fold.md (1)
4-5: Consider enhancing the changeset description with more details.While the current description captures the essence of the fix, it would be helpful to include:
- Which specific log statements were affected (error logging during flush operations)
- The root cause (esbuild formatting function forcing async code to run synchronously)
- The scope of impact (internal CLI logger methods only, not user-facing logging functions)
Here's a suggested enhancement:
-Prevent certain log statements from hanging deployed worker processes +Prevent worker process hangs when throwing errors during flush operations. This fixes an issue +where the internal CLI logger's esbuild formatting function was forcing async code to run +synchronously, affecting only internal error logging during usage/tracing/metadata flush operations.packages/cli-v3/src/utilities/logger.ts (3)
61-64: Deprecation notices effectively prevent process hanging issuesThe deprecation notices clearly communicate the risk of using these methods in deployed code. This aligns well with the PR's objective of preventing worker process hangs.
However, to further strengthen this implementation:
Consider:
- Adding runtime environment detection to throw errors if these methods are called outside CLI context
- Creating separate CLI-specific and worker-safe logger implementations
Example implementation:
+ const isCliEnvironment = () => process.env.TRIGGER_CLI === 'true'; + /** @deprecated **ONLY USE THIS IN THE CLI** - It will hang the process when used in deployed code (!) */ - warn = (...args: unknown[]) => this.doLog("warn", args); + warn = (...args: unknown[]) => { + if (!isCliEnvironment()) { + throw new Error('Logger.warn() can only be used in CLI environment'); + } + this.doLog("warn", args); + }; /** @deprecated **ONLY USE THIS IN THE CLI** - It will hang the process when used in deployed code (!) */ - error = (...args: unknown[]) => this.doLog("error", args); + error = (...args: unknown[]) => { + if (!isCliEnvironment()) { + throw new Error('Logger.error() can only be used in CLI environment'); + } + this.doLog("error", args); + };
Line range hint
83-96: Consider refactoring formatMessage to avoid sync formatting in non-CLI contextsThe root cause of the hanging issue lies in the synchronous formatting using
formatMessagesSync.Consider refactoring to:
- Use async formatting in worker contexts
- Implement a simpler formatting strategy for non-CLI environments
private formatMessage(level: Exclude<LoggerLevel, "none">, message: string): string { const kind = LOGGER_LEVEL_FORMAT_TYPE_MAP[level]; - if (kind) { + if (kind && isCliEnvironment()) { // Format the message using the esbuild formatter. const [firstLine, ...otherLines] = message.split("\n"); const notes = otherLines.length > 0 ? otherLines.map((text) => ({ text })) : undefined; return formatMessagesSync([{ text: firstLine, notes }], { color: true, kind, terminalWidth: this.columns, })[0]!; } else { - return message; + // Simple formatting for non-CLI environments + return `[${level.toUpperCase()}] ${message}`; } }
Line range hint
1-1: Update source attribution commentThe current comment references wrangler repo as the source, but given the significant divergence in functionality (CLI-specific limitations), this might be misleading.
- // This is a copy of the logger utility from the wrangler repo: https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/src/logger.ts + // Initially based on wrangler's logger utility, but modified for CLI-specific use: + // https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/src/logger.tspackages/cli-v3/src/executions/taskRunProcess.ts (3)
87-87: LGTM! Consider enhancing error detailsThe change from
logger.errortoconsole.errorhelps prevent worker process hangs during error flushing, which aligns with the PR objectives.Consider destructuring the error to log more specific details:
-console.error("Error flushing task run process", { err }); +console.error("Error flushing task run process", { + error: err instanceof Error ? err.message : String(err), + stack: err instanceof Error ? err.stack : undefined +});
97-97: LGTM! Consider refactoring error handlingThe change from
logger.errortoconsole.errorhelps prevent worker process hangs during error flushing, which aligns with the PR objectives.Consider extracting the flush error handling into a private method to reduce code duplication between
cancelandcleanup:+ async #handleFlushError(err: unknown) { + console.error("Error flushing task run process", { + error: err instanceof Error ? err.message : String(err), + stack: err instanceof Error ? err.stack : undefined + }); + } + async cancel() { this._isBeingCancelled = true; try { await this.#flush(); } catch (err) { - console.error("Error flushing task run process", { err }); + await this.#handleFlushError(err); } await this.kill(); } async cleanup(kill = true) { try { await this.#flush(); } catch (err) { - console.error("Error flushing task run process", { err }); + await this.#handleFlushError(err); } if (kill) { await this.kill("SIGKILL"); } }
Line range hint
192-196: Consider enhancing flush timeout handlingThe flush operation uses timeouts, but could be made more robust to prevent hangs.
Consider these improvements:
- Make timeouts configurable through constructor options
- Add a cleanup mechanism if IPC connection is broken
- Add logging for timeout occurrences
async #flush(timeoutInMs: number = 5_000) { logger.debug("flushing task run process", { pid: this.pid }); - await this._ipc?.sendWithAck("FLUSH", { timeoutInMs }, timeoutInMs + 1_000); + try { + if (!this._ipc) { + logger.debug("No IPC connection available for flush"); + return; + } + + await Promise.race([ + this._ipc.sendWithAck("FLUSH", { timeoutInMs }, timeoutInMs + 1_000), + new Promise((_, reject) => + setTimeout(() => reject(new Error("Flush timeout")), timeoutInMs + 2_000) + ) + ]); + } catch (err) { + if (err.message === "Flush timeout") { + logger.debug("Flush operation timed out"); + } + throw err; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.changeset/ninety-teachers-fold.md(1 hunks)packages/cli-v3/src/executions/taskRunProcess.ts(2 hunks)packages/cli-v3/src/utilities/logger.ts(1 hunks)
🔇 Additional comments (2)
.changeset/ninety-teachers-fold.md (1)
1-3: LGTM on the version bump!
Using a patch version is appropriate as this fixes a bug without introducing breaking changes.
packages/cli-v3/src/utilities/logger.ts (1)
61-64: Verify logger usage across the codebase
Let's ensure all current usages of these methods are in CLI-safe contexts.
@trigger.dev/build
@trigger.dev/core
@trigger.dev/react-hooks
trigger.dev
@trigger.dev/rsc
@trigger.dev/sdk
commit: |
The CLI logger uses a special esbuild formatting function that forces async code to become synchronous using worker threads. That approach is incompatible with deployed workers and could cause the worker process to hang, becoming completely unresponsive. Two affected log statements were found, both called if errors are thrown when flushing usage, tracing, and metadata.
This only affected two methods of the internal CLI logger, which is not called by any user-facing logging functions.
Summary by CodeRabbit
Bug Fixes
New Features
warnanderrormethods in the Logger class, indicating their restricted use in deployed code to prevent process hangs.Style