feat: Unify worker manager #954
Conversation
📝 WalkthroughWalkthroughThe provided changes represent a comprehensive refactoring of the worker management and runtime systems within the Deno and Substantial runtime environments. The primary focus is on replacing the existing Changes
Sequence DiagramsequenceDiagram
participant Client
participant WorkerManager
participant DenoWorker
participant WorkerRuntime
Client->>WorkerManager: callFunction(modulePath, functionName, args)
WorkerManager->>DenoWorker: create worker
DenoWorker->>WorkerRuntime: send message with function details
WorkerRuntime->>DenoWorker: execute function
alt Function Successful
DenoWorker->>WorkerManager: return result (SUCCESS)
else Function Failed
DenoWorker->>WorkerManager: return error (FAIL/ERROR)
end
WorkerManager->>Client: resolve/reject promise
This sequence diagram illustrates the new workflow for executing functions in the Deno runtime using the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (1)
Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #954 +/- ##
==========================================
+ Coverage 77.72% 77.76% +0.04%
==========================================
Files 154 154
Lines 19154 19019 -135
Branches 1930 1918 -12
==========================================
- Hits 14888 14791 -97
+ Misses 4243 4205 -38
Partials 23 23 ☔ View full report in Codecov by Sentry. |
Sure 👉 |
Natoandro
requested review from
luckasRanarison,
michael-0acf4,
zifeo and
Yohe-Am
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (21)
src/typegate/src/runtimes/deno/worker.ts (1)
47-49: Handle unknown message types by sending a FAILURE messageInstead of throwing an error for unknown message types, consider sending a
FAILUREmessage back to the main thread to gracefully handle unexpected input.Apply this diff to handle unknown message types:
default: - // unreachable - throw new Error(`Unknown message type: ${type}`); + self.postMessage({ + type: "FAILURE", + error: `Unknown message type: ${type}`, + }); + return;src/typegate/src/runtimes/patterns/worker_manager/deno.ts (1)
46-47: Make the 'onerror' handler asynchronousThe
onerrorhandler should be marked asasyncto maintain consistency with theonmessagehandler and ensure that any asynchronous operations withinhandlerFnare properly awaited.Apply this diff:
this.#worker.onerror = /*async*/ (event) => + this.#worker.onerror = async (event) =>src/typegate/src/runtimes/deno/worker_manager.ts (1)
64-64: Wrap error messages in Error objects when rejecting PromisesTo ensure consistent error handling, wrap error messages in
Errorobjects when rejecting Promises. This allows downstream code to handle errors effectively using standard error handling mechanisms.Apply this diff:
case "FAILURE": - reject(event.exception ?? event.error); + if (event.exception) { + reject(event.exception); + } else { + reject(new Error(event.error)); + }src/typegate/src/runtimes/substantial/workflow_worker_manager.ts (1)
88-99: Avoid duplication ofmodulePathin worker creation and message dataIn
triggerStart(), you're passingmodulePathboth when creating the worker and in the message data sent to the worker. If the worker can accessmodulePathfrom its task specification, consider removing the duplication to simplify the code and reduce potential inconsistencies.Apply this diff to eliminate the duplication:
this.createWorker(name, runId, { modulePath: workflowModPath, }); this.sendMessage(runId, { type: "START", data: { - modulePath: workflowModPath, functionName: name, run: storedRun, schedule, internal: internalTCtx, }, });src/typegate/src/runtimes/patterns/worker_manager/mod.ts (1)
74-74: Address the TODO comment increateWorkermethodThere's a TODO comment
// TODO inlinein thecreateWorker()method. To ensure code clarity and maintainability, please address this TODO by either implementing the intended functionality or removing the comment if it's no longer relevant.Do you want me to help implement the required functionality or open a new GitHub issue to track this task?
src/typegate/src/runtimes/substantial/agent.ts (11)
49-49: Trailing Comma in ConstructorThere's an unnecessary trailing comma in the constructor parameters after
config.Apply this diff to remove the trailing comma:
constructor( private backend: Backend, private queue: string, - private config: AgentConfig, + private config: AgentConfig ) {
101-105: Concatenate Workflow Names ProperlyThe logging statement in the
startmethod uses unnecessary concatenation. The template literal can be simplified.Apply this diff to simplify the logging statement:
this.logger.warn( - `Initializing agent to handle ${ - workflows - .map(({ name }) => name) - .join(", ") - }`, + `Initializing agent to handle ${workflows.map(({ name }) => name).join(", ")}` );
144-144: Comparison Using Strict EqualityIn the filter function, use strict equality (
===) for string comparison to ensure type safety.Apply this diff to update the comparison:
const requests = replayRequests.filter( - ({ run_id }) => getTaskNameFromId(run_id) == workflow.name, + ({ run_id }) => getTaskNameFromId(run_id) === workflow.name, );
201-201: Typographical Error in Log MessageThere's a minor grammatical error in the log message: "skip triggering" should be "Skipping triggering" for clarity.
Apply this diff to correct the log message:
this.logger.warn( - `skip triggering ${next.run_id} for the current tick as it is still ongoing`, + `Skipping triggering ${next.run_id} for the current tick as it is still ongoing`, );
248-252: Improve Clarity in Log Message FormattingThe log message uses nested template literals and JSON.stringify, which can be simplified for better readability.
Apply this diff to simplify the log message:
this.logger.warn( - `First item in the operation list is not a Start, got "${ - JSON.stringify( - first, - ) - }" instead. Closing the underlying schedule.`, + `First item in the operation list is not a Start, got "${JSON.stringify(first)}" instead. Closing the underlying schedule.` );
289-292: Return Type Annotation for Event HandlerThe method
#eventResultHandlerForshould explicitly state its return type to ensure type safety and readability.Apply this diff to add the return type annotation:
#eventResultHandlerFor( workflowName: string, runId: string, + ): EventHandler<WorkflowEvent> {
330-330: Typographical Error in Log MessageThe log message includes an extra quote at the end. Remove the unnecessary quote for correctness.
Apply this diff to correct the log message:
this.logger.debug(`Interrupt "${workflowName}": ${interrupt}"`); - this.logger.debug(`Interrupt "${workflowName}": ${interrupt}"`); + this.logger.debug(`Interrupt "${workflowName}": ${interrupt}`);
334-334: Redundant Log MessageThe log message repeats the interrupt information already logged previously. Consider removing or combining it to reduce verbosity.
Apply this diff to update the log message:
this.logger.info(`Persist records for "${workflowName}": ${interrupt}"`); - this.logger.info(`Persist records for "${workflowName}": ${interrupt}"`); + this.logger.info(`Persisting records for "${workflowName}": ${interrupt}`);
378-379: Remove Debugging StatementsThe
console.log({ event });statement appears to be left over from debugging. Consider removing it or replacing it with a proper logging statement.Apply this diff to remove the debugging statement:
this.workerManager.destroyWorker(workflowName, runId); - console.log({ event });Or, if logging is necessary, use the logger:
+ this.logger.debug(`Event: ${JSON.stringify(event)}`);
439-443: Use of Deprecated Logger MethodIn the
checkIfRunHasStoppedfunction,logger.erroris used to log a JSON string. Ensure that the logger method supports JSON objects or stringify the message properly.Apply this diff to ensure proper logging:
logger.error( `bad logs: ${ - JSON.stringify( - run.operations.map(({ event }) => event.type), - ) + run.operations.map(({ event }) => event.type).join(", ") }`, );
456-460: Duplicate Code BlockThe block of code logging bad logs appears twice in the function. Consider refactoring to avoid duplication.
src/typegate/src/runtimes/patterns/worker_manager/types.ts (1)
6-8: BaseMessage Interface Should Be ExtensibleThe
BaseMessageinterface currently only includes atypeproperty. Consider adding an index signature to allow for additional properties in extended interfaces.Apply this diff to make
BaseMessageextensible:export interface BaseMessage { type: string; + [key: string]: unknown; }src/typegate/src/runtimes/deno/types.ts (1)
19-21: OptionalexceptionProperty inDenoEventIn the
DenoEventtype, theexceptionproperty is marked asError | undefined. Consider usingErroronly, and when there's no exception, omit the property, to simplify the type definition.Apply this diff to adjust the type:
export type DenoEvent = | { type: "SUCCESS"; result: unknown } - | { type: "FAILURE"; error: string; exception: Error | undefined }; + | { type: "FAILURE"; error: string; exception?: Error };src/typegate/src/typegate/artifacts/mod.ts (1)
198-217: Consider adding input validation and file permissions.While the implementation is solid, consider these improvements:
- Validate input parameters (empty strings, null values)
- Set appropriate file permissions for security
async getInlineArtifact( tgName: string, code: string, ext: string, transform = (code: string) => code, ) { + if (!tgName?.trim() || !code?.trim() || !ext?.trim()) { + throw new Error('Invalid input: tgName, code, and ext must not be empty'); + } const hash = await sha256(code); const path = resolve( this.persistence.dirs.cache, "inline", tgName, hash + ext, ); if (await exists(path)) { return path; } await Deno.mkdir(dirname(path), { recursive: true }); - await Deno.writeTextFile(path, transform(code)); + await Deno.writeTextFile(path, transform(code), { mode: 0o644 }); return path; }tests/runtimes/deno/deno_test.ts (1)
42-63: Improve documentation for removed test.The comment "no worker reuse" could be more descriptive about why this functionality was removed and its implications.
- // -- no worker reuse... + // Test removed: Worker reuse is no longer supported in the new unified worker management system. + // Each workflow execution now uses a fresh worker instance to ensure isolation.tests/runtimes/substantial/common.ts (1)
Line range hint
626-667: Add comments explaining the filter expression logic.While the filter implementation is correct and type-safe, the complex nested OR/AND conditions would benefit from documentation explaining:
- The purpose of each condition
- The expected matching behavior
- Why these specific test cases were chosen
filter: { or: [ { + // Match completed runs containing "substantial" and: [ { status: { contains: JSON.stringify("COMPL") } }, { contains: JSON.stringify("substantial") }, ], }, { + // Match exact value "Bump typegraph v3 => v4" using double negation not: { not: { eq: JSON.stringify("Bump typegraph v3 => v4") }, }, }, ], } satisfies Expr,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
src/typegate/src/runtimes/deno/deno.ts(8 hunks)src/typegate/src/runtimes/deno/deno_messenger.ts(0 hunks)src/typegate/src/runtimes/deno/types.ts(1 hunks)src/typegate/src/runtimes/deno/worker.ts(1 hunks)src/typegate/src/runtimes/deno/worker_manager.ts(1 hunks)src/typegate/src/runtimes/patterns/messenger/async_messenger.ts(0 hunks)src/typegate/src/runtimes/patterns/messenger/lazy_async_messenger.ts(0 hunks)src/typegate/src/runtimes/patterns/messenger/types.ts(0 hunks)src/typegate/src/runtimes/patterns/worker_manager/deno.ts(1 hunks)src/typegate/src/runtimes/patterns/worker_manager/mod.ts(1 hunks)src/typegate/src/runtimes/patterns/worker_manager/types.ts(1 hunks)src/typegate/src/runtimes/substantial.ts(2 hunks)src/typegate/src/runtimes/substantial/agent.ts(15 hunks)src/typegate/src/runtimes/substantial/types.ts(1 hunks)src/typegate/src/runtimes/substantial/worker.ts(3 hunks)src/typegate/src/runtimes/substantial/workflow_worker_manager.ts(3 hunks)src/typegate/src/typegate/artifacts/mod.ts(1 hunks)tests/runtimes/deno/deno_test.ts(1 hunks)tests/runtimes/substantial/common.ts(7 hunks)
💤 Files with no reviewable changes (4)
- src/typegate/src/runtimes/patterns/messenger/lazy_async_messenger.ts
- src/typegate/src/runtimes/deno/deno_messenger.ts
- src/typegate/src/runtimes/patterns/messenger/types.ts
- src/typegate/src/runtimes/patterns/messenger/async_messenger.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (20)
src/typegate/src/runtimes/substantial/agent.ts (9)
267-267: Ensure TaskContext is Passed CorrectlyWhen triggering the start in
workerManager, ensure thattaskContextis correctly initialized and passed to avoid runtime errors.
375-375: Consistent Naming: 'Graceful' Spelling CorrectionThe word "Gracefull" is misspelled. It should be "Graceful" for consistency and correctness.
[typo]
Apply this diff to correct the spelling:
async #workflowHandleGracefullCompletion( + async #workflowHandleGracefulCompletion(Also, update any references to this method accordingly.
383-385: Sensitive Data in LogsLogging the entire
resultobject might expose sensitive data. Consider sanitizing the output or reducing the verbosity.[security]
Apply this diff to modify the logging:
this.logger.info( - `gracefull completion of "${runId}" (${event.type}): ${JSON.stringify(result)} started at "${startedAt}"`, + `Graceful completion of "${runId}" (${event.type}) started at "${startedAt}"` );
404-404: Typographical Error in Log MessageThere's an extra quotation mark at the end of the log message. Remove it for correctness.
[typo]
Apply this diff to correct the log message:
this.logger.info( - `Persist finalized records for "${workflowName}": ${result}" and closing everything..`, + `Persist finalized records for "${workflowName}": ${result} and closing everything..`, );
447-447: Typographical Error in Error MessageThere's a typo in the error message: "occured" should be "occurred".
[typo]
Apply this diff to correct the typo:
throw new Error( - `"${run.run_id}" has potentially corrupted logs, another run occured yet previous has not stopped`, + `"${run.run_id}" has potentially corrupted logs, another run occurred yet the previous has not stopped`, );
272-274:⚠️ Potential issueEvent Handler Return Type Mismatch
In
this.#eventResultHandlerFor, ensure that the returned function matches the expectedEventHandler<WorkflowEvent>signature.Apply this diff to ensure the function matches the expected signature:
this.workerManager.listen( next.run_id, - this.#eventResultHandlerFor(workflowName, next.run_id), + this.#eventResultHandlerFor(workflow.name, next.run_id) );[Note] Replace
workflowNamewithworkflow.nameto use the correct variable.Likely invalid or redundant comment.
68-68:⚠️ Potential issueString Template Syntax
The template string in the
catchblock might not interpolate variables correctly due to the usage of double quotes and backticks.Apply this diff to fix the string interpolation:
this.logger.warn( - `Failed writing log metadata for schedule "${schedule}" (${runId}), skipping it: ${err}`, + `Failed writing log metadata for schedule "${schedule}" (${runId}), skipping it: ${err}` );Likely invalid or redundant comment.
13-14: Import Statement: Ensure Correct Import PathThe import path for
getTaskNameFromIdandEventHandlermay need verification. Ensure that the module../patterns/worker_manager/mod.tsexports these entities correctly, and the path is accurate.Run the following script to verify the existence of the imported entities:
✅ Verification successful
Import Paths Verified: No Issues Found
The import statements are correct. Both
getTaskNameFromIdandEventHandlerare properly exported from their respective modules, and the relative paths accurately resolve to these locations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that getTaskNameFromId and EventHandler are correctly exported. # Search for the export statements in the codebase rg 'export.*(getTaskNameFromId|EventHandler)' --type tsLength of output: 277
17-19: Update Import References after Type ChangesThe imports
InterruptEvent,WorkflowCompletionEvent, andWorkflowEventhave been added, replacing previous types. Ensure that all references to the old types (Interrupt,Result,WorkerData,WorkflowResult) have been updated accordingly throughout the file.Run the following script to check for any remaining references to the old types:
src/typegate/src/runtimes/patterns/worker_manager/types.ts (2)
4-4: Type Alias for ConsistencyThe
TaskIdtype is defined as a string. Ensure that this type alias is used consistently throughout the codebase to represent task identifiers.
10-12: Generic EventHandler TypeThe
EventHandlertype is properly defined as a generic function. Ensure that all event handlers comply with this signature for consistency.src/typegate/src/runtimes/deno/types.ts (1)
6-9: ClarifyTaskSpecType UsageThe
TaskSpectype includesmodulePathandfunctionName. Ensure that paths and names used comply with the expected formats, and consider validating them to prevent errors during runtime.src/typegate/src/runtimes/substantial/types.ts (3)
14-23: LGTM! Well-structured message type.The WorkflowMessage type provides a clear contract for workflow execution with proper type discrimination and all necessary fields.
25-38: LGTM! Comprehensive completion event type.The WorkflowCompletionEvent type effectively handles both success and failure cases with proper error context.
40-44: LGTM! Well-designed event hierarchy.The event types (InterruptEvent and WorkflowEvent) form a clear and comprehensive event handling system.
Also applies to: 47-54
src/typegate/src/runtimes/substantial/worker.ts (2)
24-29: LGTM! Clear error handling for missing functions.The error message structure is clear and type-safe with the satisfies keyword.
48-71: Address the "How??" comment and consider improving exception handling.The exception handling logic is correct, but the comment suggests uncertainty about the implementation. Consider documenting why this approach was chosen or if there's a better way to handle non-Error exceptions.
- // How?? + // Note: We only pass Error instances to maintain structured error handling. + // Other types of exceptions are intentionally undefined to avoid serialization issues.✅ Verification successful
The "How??" comment should be replaced with clear documentation about serialization safety.
The current exception handling approach is correct and follows established patterns in the codebase. The suggested comment explains the rationale:
- // How?? + // Note: We only pass Error instances to maintain structured error handling. + // Other types of exceptions are intentionally undefined to avoid serialization issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find errorToString implementation ast-grep --pattern 'function errorToString' # Look for similar exception handling patterns rg "instanceof Error" -A 2 -B 2 # Search for any comments or patterns related to error serialization rg "serialization|serialize" --type tsLength of output: 16754
src/typegate/src/runtimes/substantial.ts (1)
23-28: LGTM! Centralized task ID generation.The change from
Agent.nextIdtocreateTaskIdaligns well with the PR's objective of unifying worker management, providing a consistent approach to task ID generation across different runtimes.Also applies to: 255-255
tests/runtimes/substantial/common.ts (2)
29-38: LGTM! Well-structured type definitions.The new type definitions enhance type safety and reusability:
TestTemplateOptionsprovides a clean base interfaceTestTemplateOptionsXadds flexible delay configurations using generics
45-46: LGTM! Consistent test template updates.The addition of the
onlyparameter with a default value offalseacross all test templates provides:
- Selective test execution capability
- Backward compatibility
- Consistent interface across templates
Also applies to: 174-175, 350-351, 517-518, 676-677
src/typegate/src/runtimes/substantial/workflow_worker_manager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/typegate/src/runtimes/patterns/worker_manager/mod.ts (1)
123-127:⚠️ Potential issueMemory leak in destroyWorker persists
The
startedAtrecord for the task is still not deleted after the worker is destroyed, which could lead to a memory leak.Apply this diff to fix the memory leak:
task.worker.destroy(); this.#activeTasks.delete(taskId); this.#tasksByName.get(name)!.delete(taskId); - // startedAt records are not deleted + this.#startedAt.delete(taskId);
🧹 Nitpick comments (3)
src/typegate/src/runtimes/deno/deno.ts (1)
263-267: Consider cachingentryModulePathfor performance optimizationThe TODO comment at line 263 suggests caching
entryModulePath. Implementing caching can reduce redundant file system operations, improve performance, and prevent unnecessary disk access when retrieving module paths.src/typegate/src/runtimes/patterns/worker_manager/mod.ts (2)
9-18: Well-designed abstract worker interface!The
BaseWorkerabstract class provides a clean interface for worker implementations with proper type safety through generics. The abstract methods cover all essential operations: listening, sending messages, destroying workers, and ID access.Consider adding:
- A
statusproperty to track worker lifecycle states- Error handling methods for worker failures
- Health check mechanism for long-running workers
61-69: Improve error handling in getInitialTimeStartedAtThe error message could be more helpful by including the available task IDs for debugging.
Apply this diff to enhance the error message:
if (!startedAt) { + const availableTaskIds = Array.from(this.#startedAt.keys()).join(', '); throw new Error( - `Invalid state: cannot find initial time for task "${taskId}"`, + `Invalid state: cannot find initial time for task "${taskId}". Available tasks: [${availableTaskIds}]`, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/typegate/src/runtimes/deno/deno.ts(8 hunks)src/typegate/src/runtimes/deno/worker.ts(1 hunks)src/typegate/src/runtimes/patterns/worker_manager/mod.ts(1 hunks)src/typegate/src/runtimes/substantial/workflow_worker_manager.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (8)
src/typegate/src/runtimes/deno/deno.ts (2)
158-158: Ensure proper resource cleanup by callingworkerManager.deinit()The
deinit()method currently hasawait this.workerManager.deinit();commented out. To prevent potential resource leaks and ensure proper cleanup of workers, consider uncommenting this line or implementing the necessary cleanup logic within theWorkerManager.
369-373: Review security implications when generating code inexportInlineFunction()The
exportInlineFunction()function appends code without sanitizing the inputcode. Ifcodecontains malicious content, it could introduce security vulnerabilities. Ensure that thecodeparameter is properly sanitized or originates from a trusted source to prevent code injection attacks.src/typegate/src/runtimes/deno/worker.ts (1)
40-44: Avoid including raw exception object in failure messageIncluding the raw exception object
ein the failure message may lead to serialization errors or unintended exposure of sensitive information. It's safer to include only the error message or a sanitized version of the error.src/typegate/src/runtimes/substantial/workflow_worker_manager.ts (1)
Line range hint
22-99: Refactoring enhances modularity and maintainabilityExtending
BaseWorkerManagerand refactoring methods likedestroyWorker,isOngoing, andlistenimproves code reuse and readability. The changes streamline worker management and align with best practices.src/typegate/src/runtimes/patterns/worker_manager/mod.ts (4)
1-8: LGTM! Proper license header and imports.The file includes the appropriate MPL-2.0 license header and necessary imports for logging and type definitions.
74-74: Remove or implement the TODO commentThe TODO comment "TODO inline" needs to be addressed. If inlining is necessary, it should be implemented; if not, the comment should be removed.
Would you like me to help implement the inlining of the
addWorkercall?
145-149: LGTM! Task ID creation with proper sanitizationThe implementation now correctly sanitizes the name by replacing the delimiter pattern, addressing the previous concern about task ID parsing errors.
151-159: Verify "unreachable" comment and improve error handlingThe comment states this is "unreachable", but we should verify this claim and improve error handling.
Let's verify this by checking all task ID creation points:
Consider adding validation in createTaskId to ensure this truly is unreachable:
export function createTaskId(name: string) { + if (!name) { + throw new Error("Task name cannot be empty"); + } const uuid = crypto.randomUUID(); const sanitizedName = name.replace(/_::_/g, "__"); return `${sanitizedName}_::_${uuid}`; }✅ Verification successful
"unreachable" comment is correct, but error message could be improved
The comment is accurate as all TaskIds are created through
createTaskId, which ensures proper format by sanitizing the name and always providing a UUID. However, consider updating the error message to:throw new Error(`Internal error: malformed task ID "${taskId}". Please report this as a bug.`);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all createTaskId calls to verify task ID format echo "Searching for createTaskId calls:" rg "createTaskId\(" -A 2 # Search for direct TaskId assignments to catch any manual creation echo "Searching for TaskId type usage:" ast-grep --pattern 'const $var: TaskId = $value'Length of output: 1025
Script:
#!/bin/bash # Find TaskId type definition and any other potential creation points echo "Searching for TaskId type definition:" rg "type.*TaskId" echo -e "\nSearching for string literals with _::_:" rg '"[^"]*_::_[^"]*"' echo -e "\nChecking if TaskId is exported/imported:" rg "export.*TaskId|import.*TaskId"Length of output: 1449
<!-- Pull requests are squashed and merged using: - their title as the commit message - their description as the commit body Having a good title and description is important for the users to get readable changelog. --> <!-- 1. Explain WHAT the change is about --> - Solves [MET-667](https://linear.app/metatypedev/issue/MET-667/gate-unify-the-worker-manager-between-workflows-and-runtime) - [x] `BaseWorkerManager` - [x] Use in Deno runtime - [ ] ~Use in Python runtime~ _(followup PR)_ - [ ] ~Use in Rust runtime~ _(followup PR)_ - [ ] ~Worker pooling~ _(followup PR)_ <!-- 2. Explain WHY the change cannot be made simpler --> <!-- 3. Explain HOW users should update their code --> --- - [ ] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> Based on the comprehensive summary, here are the updated release notes: - **New Features** - Enhanced worker management system with improved task tracking and execution. - Introduced new `WorkerManager` for more robust Deno runtime operations. - Added support for inline artifact generation and management. - New asynchronous method `getInlineArtifact` in the `ArtifactStore` class. - **Improvements** - Streamlined messaging and event handling across different runtime components. - Improved error reporting and task lifecycle management. - Refined type definitions for better type safety. - **Breaking Changes** - Removed `DenoMessenger` and `LazyAsyncMessenger` classes. - Restructured workflow event and message handling. - Updated task ID generation mechanism. - **Performance** - Optimized worker initialization and task execution. - Introduced more efficient task tracking and resource management. - **Bug Fixes** - Improved error handling in worker and runtime environments. - Enhanced message communication between workers and main thread. - Removed outdated test cases to focus on relevant functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
BaseWorkerManagerUse in Python runtime(followup PR)Use in Rust runtime(followup PR)Worker pooling(followup PR)Migration notes
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
WorkerManagerfor more robust Deno runtime operations.getInlineArtifactin theArtifactStoreclass.Improvements
Breaking Changes
DenoMessengerandLazyAsyncMessengerclasses.Performance
Bug Fixes