feat: wasm worker manager #966
Conversation
# Conflicts: # .ghjk/deno.lock # deno.lock
📝 WalkthroughWalkthroughThis pull request introduces significant architectural changes to the TypeGate runtime system, focusing on enhancing worker management, WebAssembly (Wasm) runtime interactions, and refactoring various runtime components. The changes primarily involve decoupling the existing WitWire messaging system, introducing a new worker management approach, and restructuring how different runtime environments handle operations and worker lifecycles. Key modifications include removing the Changes
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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 (
|
[skip ci]
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/typegate/src/runtimes/wasm/worker_manager.ts (1)
86-88: Structured, consistent logging.Using
logger.infofor normal flow andlogger.debugfor more detailed insights is a good practice. Consider adding an optional context string or request ID to logs for better traceability across multiple tasks.src/typegate/src/runtimes/wasm_wire.ts (1)
140-150: Asynchronous call to the worker.
- You log the operation, call
workerManager.callWitOp, and return the result once available.- The additional logging for both success and debug details helps users pinpoint potential failures or track performance.
Ensure that large responses don’t congest logs; consider a truncation or separate debug-level logs for very large payloads.
src/typegate/src/runtimes/wit_wire/mod.ts (2)
14-33: Consider retaining thehostcallfunction reference in the class.
Thehostcallfunction is passed intoinit()but isn't stored in the constructed instance. If future usage demands callinghostcallon the instance, consider storing it. Otherwise, clarifying why it's not stored may prevent confusion.
Line range hint
133-159: Throwing objects instead of nativeErrormay impact debugging.
Here, we throw a plain object containingcode,message, and optionalcause. Throwing a nativeErrorpreserves the stack trace and aligns with typical error-handling patterns. Consider adoptingErrorbuilds for consistency.src/typegate/src/runtimes/wasm/types.ts (1)
18-22: Consider normalizing error reporting inWasmEvent.
Storing a rawErrorobject might be tricky when crossing worker boundaries. You may want to serialize relevant error fields or create a standardized error response shape for reliability.src/typegate/src/runtimes/wasm/worker.ts (1)
24-58: Enhance error handling and add resource cleanup.The message handler could benefit from more robust error handling and resource management.
Consider these improvements:
self.onmessage = async function (event: MessageEvent<WasmMessage>) { const { type } = event.data; switch (type) { case "CALL": { - const { id } = event.data; + const { id, operation } = event.data; + console.debug(`Processing ${operation} for instance ${id}`); if (!witWireInstances.has(id)) { const handle = await WitWireHandle.init({ ...event.data, hostcall, }); witWireInstances.set(id, handle); + console.debug(`Created new WitWireHandle instance ${id}`); } try { self.postMessage({ type: "SUCCESS", result: await handleWitOp(event.data), }); } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + console.error(`Operation failed for instance ${id}: ${errorMessage}`); self.postMessage({ type: "FAILURE", - error: errorToString(error), + error: errorMessage, exception: error, }); + } finally { + // Clean up terminated instances + if (event.data.operation === "terminate") { + witWireInstances.delete(id); + console.debug(`Cleaned up instance ${id}`); + } } break; } default: - throw new Error(`Unknown message type: ${type}`); + const error = `Unknown message type: ${type}`; + console.error(error); + self.postMessage({ + type: "FAILURE", + error, + }); } };src/typegate/src/runtimes/patterns/worker_manager/pooling.ts (2)
24-24: Add JSDoc comment for the clear method.Consider adding documentation to explain the purpose and behavior of the clear method.
+ /** Clears any pending tasks in the queue and releases associated resources. */ clear: () => void;
81-89: Enhance clear method implementation.The clear method should also handle pending queue items.
clear() { if (this.#timerId != null) { clearTimeout(this.#timerId); + this.#timerId = null; } + // Cancel all pending items in the queue + while (this.#queue.length > 0) { + this.#cancelNextEntry(); + } } [Symbol.dispose]() { this.clear(); }src/typegate/src/types.ts (1)
53-59: Add JSDoc documentation for the WitOpArgs interface.The interface structure looks good and aligns well with WebAssembly worker management needs. However, it would benefit from JSDoc documentation explaining the purpose and usage of each property.
Consider adding documentation like this:
+/** + * Interface for WebAssembly operation arguments used in worker management. + */ export interface WitOpArgs { + /** Name of the WebAssembly operation to execute */ opName: string; + /** Resolver arguments containing parent, variables, context, and other metadata */ args: ResolverArgs; + /** Unique identifier for the operation */ id: string; + /** Path to the WebAssembly component */ componentPath: string; + /** Array of WitWire materialization information for the operations */ ops: WitWireMatInfo[]; }
📜 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 (22)
src/typegate/engine/00_runtime.js(0 hunks)src/typegate/engine/runtime.d.ts(0 hunks)src/typegate/engine/src/ext.rs(0 hunks)src/typegate/engine/src/runtimes/wit_wire.rs(2 hunks)src/typegate/src/config/types.ts(2 hunks)src/typegate/src/runtimes/deno/deno.ts(6 hunks)src/typegate/src/runtimes/patterns/worker_manager/deno.ts(2 hunks)src/typegate/src/runtimes/patterns/worker_manager/mod.ts(3 hunks)src/typegate/src/runtimes/patterns/worker_manager/pooling.ts(4 hunks)src/typegate/src/runtimes/patterns/worker_manager/wasm.ts(1 hunks)src/typegate/src/runtimes/python.ts(4 hunks)src/typegate/src/runtimes/substantial.ts(4 hunks)src/typegate/src/runtimes/substantial/agent.ts(9 hunks)src/typegate/src/runtimes/wasm/types.ts(1 hunks)src/typegate/src/runtimes/wasm/worker.ts(1 hunks)src/typegate/src/runtimes/wasm/worker_manager.ts(1 hunks)src/typegate/src/runtimes/wasm_wire.ts(4 hunks)src/typegate/src/runtimes/wit_wire/mod.ts(4 hunks)src/typegate/src/types.ts(2 hunks)tests/runtimes/python/python_sync_test.ts(4 hunks)tests/runtimes/python/python_test.ts(10 hunks)tests/runtimes/wasm_wire/rust/fdk.rs(1 hunks)
💤 Files with no reviewable changes (3)
- src/typegate/engine/runtime.d.ts
- src/typegate/engine/src/ext.rs
- src/typegate/engine/00_runtime.js
✅ Files skipped from review due to trivial changes (4)
- tests/runtimes/wasm_wire/rust/fdk.rs
- tests/runtimes/python/python_test.ts
- src/typegate/src/runtimes/patterns/worker_manager/mod.ts
- src/typegate/src/runtimes/patterns/worker_manager/deno.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- 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 (39)
src/typegate/src/runtimes/patterns/worker_manager/wasm.ts (6)
10-12: Good use of private fields for worker encapsulation.Declaring
#workerand#workerIdensures these properties remain private and inaccessible outside theWasmWorkerclass. This helps maintain a clear separation of concerns and avoids accidental external mutations.
13-14: Constructor clarity.The constructor properly captures and stores the
workerId, which is essential for identifying this worker instance. Good job also callingsuper()to ensure the base class is properly initialized.
46-48: Message posting is straightforward.The
sendmethod correctly posts messages to the worker’s queue. This is a standard approach to decouple message preparation from the core worker logic.
50-52: Graceful termination.Terminating the worker prevents resource leaks. Ensure that you consistently call
destroy()in higher-level code when the worker is no longer needed.
54-56: Convenient accessor for worker ID.The
idgetter is a simple, handy way to retrieve the worker’s identifier. This is especially useful for logging or debugging purposes.
15-31: Evaluate permissions for security and functionality.A “FIXME” note suggests uncertainty about the correct permissions. Currently, most permissions are disabled, except for network communication (“net”), read inheritance, system calls, and environment variables shared with workers. Ensure these minimal permission settings still allow all necessary operations without opening unnecessary attack vectors.
You can run a safety check or dynamic tests that validate the worker's behavior under these restrictive permissions:
✅ Verification successful
Current permission settings are secure and appropriate ✅
The permission configuration follows the principle of least privilege:
- Enabled permissions (
net,read,sys,env) are necessary for worker functionality- High-risk permissions (
run,write,ffi) are correctly disabled to prevent security vulnerabilities- Environment variables are selectively shared through
envSharedWithWorkers🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Attempt to run the new WasmWorker with typical tasks and detect any permission issues # This script checks for potential runtime errors or permission denials. rg "new WasmWorker" -A 10Length of output: 830
Script:
#!/bin/bash # Find worker implementation and permission-related content echo "=== Worker Implementation ===" rg -t typescript "class WasmWorker" -A 20 echo -e "\n=== Permission Related Errors/Issues ===" rg -i "permission|denied" --type typescript echo -e "\n=== Security Documentation ===" fd "README|SECURITY" --extension md --exec cat {}Length of output: 6896
Script:
#!/bin/bash # Find and analyze worker implementation with correct file extensions echo "=== TypeScript Files ===" fd -e ts -e tsx . src/typegate/src/runtimes/ echo -e "\n=== Worker Implementation ===" fd -e ts -e tsx . src/typegate/src/runtimes/ --exec cat {} echo -e "\n=== Permission Usage Patterns ===" rg -l "permissions.*\{" --type-add 'typescript:*.{ts,tsx}' -t typescriptLength of output: 68267
src/typegate/src/runtimes/wasm/worker_manager.ts (4)
19-23: Class structure is logically consistent.
WorkerManagerinherits fromBaseWorkerManagerand follows a cohesive pattern for managing tasks, messages, and events. The generics (TaskSpec, WasmMessage, WasmEvent) ensure type safety.
24-38: Evaluate the lazy initialization of the pool.
#getPool()stores the pool in a static variable to avoid multiple instantiations. Make sure to handle cleanup scenarios. For instance, if the server restarts or if workers need to be forcibly reinitialized, consider whether a re-init or a pool reset is required.
40-42: Injecting host call context is well-designed.Passing
hostcallCtxto the constructor allows the manager to handle environment-specific logic. This ensures a single source of truth for host call data.
44-84: Robust asynchronous workflow with event-driven messaging.
callWitOpmanages the entire lifecycle of a task.- It delegates, sends a “CALL” message, and returns a promise that resolves or rejects based on the worker’s response.
- The
handlerfunction accommodates special “HOSTCALL” messages, bridging the worker environment withhostcall.Overall, this design is clear and maintainable.
src/typegate/src/runtimes/wasm_wire.ts (6)
13-13: Decouple internal vs. external import.Importing
WitWireMatInfofrom a.jsfile might lead to confusion if other types are TS-based. Confirm that this.jsmodule is intended, or consider unifying the import style to.tsfor consistency.
25-28: Constructor argument structure is coherent.The addition of
componentPath,wireMat, andworkerManagerclarifies the runtime’s dependencies. This fosters better management of WASM runtime resources and worker interactions.
57-67: Local artifact path retrieval.
componentPathis derived from the artifact store, andwireMatis mapped from the provided materializers. This approach ensures the runtime is fully aware of all relevant metadata for the WASM artifact.
68-75: Host call context set up.You maintain the auth token and references to the typegate, as well as the
typegraphUrl. This is critical for bridging function calls back to the host.
76-82: WorkerManager instantiation.Instantiating
WorkerManagerwith the newly createdhostcallCtxis straightforward. This keeps the worker environment self-contained, with minimal knowledge of the outside world.
86-87: Methodical teardown process.
deinit()callsworkerManager.deinit()to properly shut down the manager. This helps prevent resource leaks or orphaned workers.src/typegate/src/runtimes/wit_wire/mod.ts (3)
6-6: All good on the import statement.
No issues spotted with the import ofResolverArgsandWitOpArgs.
52-53: Constructor usage looks fine.
No issues with the constructor arguments or assignments.
55-123: Handle JSON parse failures gracefully.
When parsingres.Ok, a malformed JSON string would trigger a runtime error. If the expected data source is under your control, this may not be an issue. Otherwise, consider a try/catch aroundJSON.parseto provide a clearer error.src/typegate/src/runtimes/python.ts (5)
15-15: Import statement verified.
Import ofWorkerManageris consistent with the newly introduced architecture.
24-26: Constructor signature changes look coherent.
AddingwireMatandworkerManagerclarifies responsibilities for Python-based operations.
126-134: Ensure authorized usage of the hostcall context.
CreatinghostcallCtxwith an internal token is a good approach for internal requests. Verify that the token usage matches your security guidelines and is invalidated where necessary.
144-148: Properly de-initializing worker resources.
Explicitly callingthis.workerManager.deinit()ensures WASM workers are shut down. Good resource hygiene.
203-217: Delegate method logic appears consistent.
RenamingopNameand routing calls throughthis.workerManageris aligned with the new design. The logging statements are helpful for debugging.src/typegate/src/runtimes/wasm/types.ts (3)
6-9: TaskSpec definition is clear.
No issues spotted; it effectively documents the required shape of the task.
11-11: WasmCallMessage typed well.
Merging theWitOpArgswith a{ type: "CALL" }discriminates call messages nicely.
13-16: Union type forWasmMessageis concise.
This union covers calls, hostcall results, and shutdown messages well.src/typegate/src/runtimes/wasm/worker.ts (1)
1-8: LGTM!The imports and Map initialization are well-structured and properly organized.
src/typegate/src/config/types.ts (1)
95-95: LGTM!The error message formatting change improves readability while maintaining the same information.
src/typegate/src/runtimes/deno/deno.ts (2)
37-40: LGTM!The function formatting changes improve code readability while maintaining the same functionality.
Also applies to: 46-51, 73-73
191-191: Properly await async cleanup.The addition of await is crucial for proper resource cleanup.
Let's verify that all deinit calls are properly awaited:
✅ Verification successful
Async cleanup properly handled across the codebase
The addition of
awaitforworkerManager.deinit()is correct. All async cleanup operations in the codebase are properly awaited, and synchronous operations (likeredis.close()andclient.close()) are correctly handled without await as indicated by thedeno-lint-ignore require-awaitdirectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for async deinit methods that might be called without await rg -g '*.ts' 'async\s+deinit.*\{' -A 5Length of output: 5733
Script:
#!/bin/bash # Check if redis.close() and client.close() are async rg -g '*.ts' '(redis|client)\.close\(' -B 5Length of output: 1402
tests/runtimes/python/python_sync_test.ts (2)
192-193: Great addition of pre-warming step!The pre-warming step helps eliminate cold-start bias from the performance measurements by executing the first 10 test cases before starting the timer.
310-310: LGTM: Improved test names readability.The test names are now more consistent and readable.
Also applies to: 388-388, 422-422
src/typegate/src/runtimes/substantial/agent.ts (1)
115-116: Important: Method is now async to properly await worker cleanup.The
stopmethod has been correctly updated to be async, ensuring proper cleanup by awaiting the worker manager's deinitialization.src/typegate/src/runtimes/substantial.ts (1)
151-153: Good: Method is now async to properly await agent cleanup.The
deinitmethod has been correctly updated to be async, ensuring proper cleanup by awaiting the agent's stop operation. This change aligns with the async changes inagent.ts.src/typegate/engine/src/runtimes/wit_wire.rs (3)
99-99: Good: Added automatic cleanup with Drop implementation.The Instance struct now includes an id field and implements Drop trait for automatic cleanup, which is a better approach than manual cleanup. The Drop implementation ensures that preopened directories are properly cleaned up when instances are dropped.
Also applies to: 105-129
411-413: LGTM: Instance creation now properly stores the id.The instance creation has been updated to store the instance id, which is necessary for the new cleanup mechanism.
112-112: Consider reviewing the async cleanup necessity.The FIXME comment questions whether the cleanup really needs to be async. This is a valid concern as file system operations could potentially be done synchronously during cleanup.
Run this script to analyze the impact of directory removal operations:
✅ Verification successful
The async cleanup is appropriate and should be retained
The async implementation is the correct approach here as this is a cleanup operation in a
Dropimplementation. Using synchronous file system operations would block the destructor, while the current fire-and-forget async approach allows the cleanup to proceed in the background without impacting performance. The FIXME comment can be safely removed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar cleanup patterns in the codebase rg -A 5 "remove_dir_all"Length of output: 2647
Script:
#!/bin/bash # Get broader context around the cleanup operation rg -B 10 -A 10 "FIXME: Does it really have to be async" src/typegate/engine/src/runtimes/wit_wire.rsLength of output: 826
src/typegate/src/types.ts (1)
17-17: Verify the import path and type existence.Let's ensure the imported type exists and the path is correct.
✅ Verification successful
Import statement is correct and properly used
The
WitWireMatInfotype is correctly defined in the target file and is consistently used across the codebase for WebAssembly runtime interactions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the import path and WitWireMatInfo type existence # Check if the file exists fd -e ts -e d.ts "runtime.d.ts" src/ # Check for WitWireMatInfo type definition rg -t typescript "export.*WitWireMatInfo" src/Length of output: 153
Script:
#!/bin/bash # Check for WitWireMatInfo type definition without file type restriction rg "export.*WitWireMatInfo" src/typegate/engine/runtime.d.ts # Also check for any usage of WitWireMatInfo to understand the context rg "WitWireMatInfo" src/Length of output: 1628
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/typegate/src/runtimes/patterns/worker_manager/wasm.ts (1)
41-43: 🛠️ Refactor suggestionEnhance error handling with logging and recovery strategy.
The current error handling simply throws the error, which could lead to unhandled worker failures. Consider adding logging and implementing a recovery strategy.
- this.#worker.onerror = (event) => { - throw event.error; - }; + this.#worker.onerror = (event) => { + console.error(`[WasmWorker ${this.#workerId}] Worker error:`, event.error); + // Consider implementing a recovery strategy: + // - Attempt to restart the worker + // - Notify the worker manager + // - Clean up resources + throw event.error; + };
🧹 Nitpick comments (4)
docs/metatype.dev/docs/reference/typegate/index.mdx (1)
90-91: Consider adding WASM_WORKER_WAIT_TIMEOUT_MS configuration.For consistency with other worker configurations (e.g., DENO_WORKER_WAIT_TIMEOUT_MS), consider adding documentation for the WASM worker timeout configuration.
Add the following entry to the table:
| MAX_WASM_WORKERS | Maximal number of available wasm workers | 8 | 16 | +| WASM_WORKER_WAIT_TIMEOUT_MS | Timeout for waiting for a free wasm worker | 5000 | 2000 | | SUBSTANTIAL_WORKER_WAIT_TIMEOUT_MS | Timeout for waiting for a free substantial worker | 15000 | 2000 |src/typegate/src/runtimes/patterns/worker_manager/wasm.ts (3)
5-6: Consider combining imports from the same module.The imports from
../../wasm/types.tscan be combined into a single import statement for better maintainability.-import { WasmEvent } from "../../wasm/types.ts"; -import { WasmMessage } from "../../wasm/types.ts"; +import { WasmEvent, WasmMessage } from "../../wasm/types.ts";
50-52: Enhance worker cleanup in destroy method.The
destroymethod terminates the worker but doesn't ensure proper cleanup of resources or handle pending operations.destroy() { + // Clean up any pending operations + try { this.#worker.terminate(); + } catch (error) { + console.error(`[WasmWorker ${this.#workerId}] Error during termination:`, error); + } + // Reset internal state + this.#worker = null!; }
46-48: Add error handling to send method.The
sendmethod should handle potential errors that could occur duringpostMessage, especially for large messages or if the worker is terminated.send(msg: WasmMessage) { - this.#worker.postMessage(msg); + try { + this.#worker.postMessage(msg); + } catch (error) { + console.error(`[WasmWorker ${this.#workerId}] Error sending message:`, error); + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/metatype.dev/docs/reference/typegate/index.mdx(1 hunks)src/typegate/engine/src/runtimes/wit_wire.rs(2 hunks)src/typegate/src/runtimes/patterns/worker_manager/pooling.ts(2 hunks)src/typegate/src/runtimes/patterns/worker_manager/wasm.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- 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 (7)
docs/metatype.dev/docs/reference/typegate/index.mdx (1)
90-91: LGTM! Documentation is clear and consistent.The new WASM worker configuration variables are well-documented and follow the same pattern as other worker configurations in the system.
src/typegate/engine/src/runtimes/wit_wire.rs (2)
99-99: LGTM! Good addition of instance identifier.The addition of the
idfield to theInstancestruct enhances worker management by enabling proper tracking and cleanup of instances.
Line range hint
413-419: LGTM! Proper instance initialization with ID.The instance initialization correctly clones and stores the instance ID, which is necessary for both the map key and Instance struct.
src/typegate/src/runtimes/patterns/worker_manager/pooling.ts (3)
92-93: LGTM!The timeout calculation reformatting improves readability while maintaining the same logic.
Line range hint
1-224: Consider WASM-specific worker lifecycle management.Given that this PR focuses on WASM worker management, consider if WASM workers require special handling in the worker pool, such as:
- Initialization time for WASM module loading
- Memory management considerations
- Specific cleanup requirements
Let's check for WASM-specific worker implementations:
217-221: Handle potential race condition in worker destruction.The current implementation might clear the workers array before all workers are fully destroyed if
worker.destroy()is asynchronous.Let's verify if
worker.destroy()is asynchronous:If
destroy()is asynchronous, consider this enhancement:clear() { logger.warn( `destroying idle workers: ${this.#idleWorkers .map((w) => `"${w.id}"`) .join(", ")}`, ); - this.#idleWorkers.forEach((worker) => worker.destroy()); - this.#idleWorkers = []; + const workers = this.#idleWorkers; + this.#idleWorkers = []; + return Promise.all(workers.map((worker) => worker.destroy())); }src/typegate/src/runtimes/patterns/worker_manager/wasm.ts (1)
19-30: Verify worker permissions configuration.The FIXME comment suggests uncertainty about the correct permissions. Let's verify:
- Network access is enabled (
net: true)- File read and system permissions are inherited
- Security-critical permissions (run, write, ffi) are correctly disabled
- Environment variables are restricted to a shared subset
Let's analyze the permission configuration:
✅ Verification successful
Remove FIXME comment - permissions are correctly configured
The current permission configuration is secure and consistent across worker implementations. It properly implements the principle of least privilege by:
- Enabling only necessary permissions (net, inherited read/sys)
- Explicitly disabling security-critical operations (run, write, ffi)
- Restricting environment variables to a controlled subset
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any security-related discussions or documentation rg -l "worker.*permission" --type md rg -l "deno.*permission" --type md # Check for similar worker permission patterns ast-grep --pattern 'new Worker($_, { $$$ permissions: { $$$ } $$$ })'Length of output: 159
Script:
#!/bin/bash # Look for Worker-related code and configurations rg -l "new Worker\(" rg "permissions.*{" -A 10 # Check for security documentation fd "SECURITY.md|security.md" cat README.md 2>/dev/null | grep -i "security\|permission" # Look for Deno worker patterns rg "Deno\..*Worker"Length of output: 612
Script:
#!/bin/bash # Read security documentation cat SECURITY.md # Check all worker implementations cat src/typegate/src/runtimes/patterns/worker_manager/wasm.ts cat src/typegate/src/runtimes/patterns/worker_manager/deno.ts cat src/typegate/src/runtimes/deno/hooks/mod.ts # Look for permission patterns in these specific files rg "permissions" src/typegate/src/runtimes/patterns/worker_manager/wasm.ts src/typegate/src/runtimes/patterns/worker_manager/deno.ts src/typegate/src/runtimes/deno/hooks/mod.tsLength of output: 6410
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/typegate/src/runtimes/patterns/worker_manager/pooling.ts (1)
81-85: 🛠️ Refactor suggestionEnhance queue cleanup to prevent memory leaks.
The current implementation only clears the timer but doesn't clear the queue entries. This could lead to memory leaks from lingering consumer and cancellation handler references.
Consider enhancing the cleanup:
clear() { if (this.#timerId != null) { clearTimeout(this.#timerId); } + // Clear queue and invoke cancellation handlers + while (this.#queue.length > 0) { + this.#cancelNextEntry(); + } }
🧹 Nitpick comments (8)
src/typegate/src/runtimes/wit_wire/mod.ts (5)
14-20: Use a dedicated interface for initialization parameters
Accepting a single object for initialization improves readability. Consider defining a named interface or type forparamsto enhance type clarity and consistency across the codebase.
21-31: Remove the unused_resvariable or use it meaningfully
The_resvariable fromMeta.wit_wire.initis stored but never used. Eliminate dead code or utilize the response to prevent confusion.const _res = await Meta.wit_wire.init( componentPath, id, { expected_ops: ops, metatype_version: METATYPE_VERSION, }, hostcall, ); - return new WitWireHandle(id, componentPath, ops); + // If _res isn't needed, remove it or use it meaningfully. return new WitWireHandle(id, componentPath, ops);
52-52: Constructor is empty
The empty constructor is valid, but consider removing it if no custom initialization is required. A default constructor is implicit.- ) {} + ) { + // If no custom initialization is needed, omit the body entirely. + }
77-77: Consider a more explicit check
Relying ontypeof res === 'string'works but could mask unexpected conditions. A more robust approach is to validate ifresindeed holds expected fields.
154-159: Refine error code assignment
Wrapping error details in a dynamic object is flexible but might raise maintainability concerns. Consider introducing a well-defined error code enumeration for clarity.tests/runtimes/python/python_test.ts (3)
13-13: Check path consistency
This relative import is valid but ensure consistency with any barrel files or aliases used in the project.
49-52: Type safety for resolver arguments
Casting toResolverArgsviaas unknownmay bypass type checks. If possible, provide a properly typed object to catch errors at compile time.
87-87: Same typing issue
As with the previous lambda test, ensure stronger type safety by avoidingas unknown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/typegate/src/runtimes/deno/deno.ts(5 hunks)src/typegate/src/runtimes/patterns/worker_manager/pooling.ts(4 hunks)src/typegate/src/runtimes/wasm/worker.ts(1 hunks)src/typegate/src/runtimes/wit_wire/mod.ts(8 hunks)tests/runtimes/python/python_test.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/typegate/src/runtimes/wasm/worker.ts
- src/typegate/src/runtimes/deno/deno.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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/arm64, custom-arm)
- GitHub Check: test-full
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: pre-commit
🔇 Additional comments (31)
src/typegate/src/runtimes/wit_wire/mod.ts (9)
33-33: Initialization return is clear
Returning the newWitWireHandleinstance is straightforward and aligns with the static factory pattern.
56-56: Destructuring fields is concise
Accessingid,componentPath, andopsthrough destructuring is neat and improves readability.
60-60: Ensure JSON.stringify doesn't throw
IfinJsoncan contain circular references,JSON.stringifywill throw. Consider adding checks or a safer serialization approach.
71-71: Detailed cause information
IncludingcomponentPathin the error cause is helpful for debugging. Good practice.
85-86: Ops and component are captured
Storingopsandcomponentin the error cause fosters better debugging context.
95-95: Consistent error cause
Good consistency in addingcomponentto the error’s cause here and in preceding blocks.
108-108: Logging the component path
Continuing the same debugging pattern for any error scenario is commendable.
119-119: Consistent error structure
Retainingcomponentwithin the final fallback error clarifies debugging.
133-133: Exported hostcall
Exportinghostcallfor external usage expands flexibility. Ensure that inputs are validated or sanitized ifjsoncan come from untrusted sources.tests/runtimes/python/python_test.ts (18)
6-10: New WitWire imports
Switching from a monolithic import to individual exports clarifies usage. Ensure references align with the new runtime structure.
15-27: Inline WitWire initialization is well-encapsulated
Defining a helper function (initInlineWitWire) keeps the setup code DRY and maintainable.
31-31: Correct usage of initInlineWitWire
The usage here is straightforward, properly passing parameters for initialization.
69-69: Reusing the helper
Re-initializing withinitInlineWitWirefor a different test scenario maintains consistency.
108-108: Engine initialization for tests
Spawning an engine with a specific Python runtime is correct for the test’s scope.
179-179: Inline chain usage
Chaining.on(e)config in your test queries is a succinct approach and is consistent with the overall test style.
182-184: Pre-warming approach is beneficial
This technique helps measure subsequent test performance more accurately after any cold starts.
276-276: Python engine usage
Using.engine("runtimes/python/python.py")again is consistent.
351-351: Reload engine
Re-initializing the engine withpython.tsis valid for verifying reload behaviors.
361-361: Descriptive test name
Explicitly mentioning "no artifacts in sync mode" clarifies the test objective.
365-365: Testing no-artifact scenario
Creating an engine with"python_no_artifact.py"ensures coverage of minimal artifact usage.
371-374: GQL query structure
The minimal query is appropriate for verifying lambda calls in a no-artifact scenario.
396-404: Multiple fields in a single query
VerifyingidentityDefandidentityLambdain one query is an efficient approach to cover multiple resolvers.
406-406: Empty line
No issues.
425-425: Clear test labeling
Identifies the focus on duplicate artifact uploads.
429-429: Python duplicate artifact test
Spanning a separate.pyfile ensures coverage of multi-artifact scenarios.
435-439: GraphQL request for duplicates
Checking two similarly structured queries in one request ensures the duplication logic is validated.
462-472: Extensive field checks
IncludingidentityModandidentityModDuplicatewithin the same query helps verify correct artifact resolution.src/typegate/src/runtimes/patterns/worker_manager/pooling.ts (4)
41-41: LGTM! Empty implementation is appropriate.The empty
clear()implementation is correct since the queue array is function-scoped and will be garbage collected naturally.
223-223: LGTM! Using forEach is more appropriate.Good change from
maptoforEachsince we're not using the return value of the iteration.
24-24: LGTM! Note the breaking change.The addition of the
clear()method to theWaitQueueinterface is appropriate for resource cleanup. Be aware that this is a breaking change requiring all implementations to provide this method.Let's verify if there are any other implementations of
WaitQueuethat need to be updated:✅ Verification successful
Breaking change is properly implemented
The addition of
clear()method to theWaitQueueinterface is safe. All existing implementations already provide this method:
createSimpleWaitQueue: implements emptyclear()WaitQueueWithTimeout: implementsclear()with proper resource cleanup (usesusingkeyword in tests)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other WaitQueue implementations ast-grep --pattern 'class $_ implements WaitQueue'Length of output: 52
Script:
#!/bin/bash # First, find files containing WaitQueue rg "WaitQueue" --type ts -l # Then look for implementations with different patterns ast-grep --pattern 'implements WaitQueue' ast-grep --pattern 'class $_ implements WaitQueue' ast-grep --pattern 'const $_ = createSimpleWaitQueue'Length of output: 287
Script:
#!/bin/bash # Check content of both files echo "=== pooling.ts ===" rg "WaitQueue" -C 5 src/typegate/src/runtimes/patterns/worker_manager/pooling.ts echo -e "\n=== pooling_test.ts ===" rg "WaitQueue" -C 5 tests/patterns/worker_manager/pooling_test.tsLength of output: 2504
225-225: Verify race condition handling during cleanup.While adding wait queue cleanup is good, verify that there's no race condition between destroying workers and clearing the wait queue. Consider if we need to handle in-flight worker requests.
Let's analyze the worker lifecycle handling:
✅ Verification successful
Implementation is safe from race conditions
The worker lifecycle implementation safely handles cleanup by:
- Only destroying idle workers during clear operation
- Maintaining separate tracking of busy workers
- Protecting in-flight requests through the worker borrowing mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for worker lifecycle management patterns rg -A 5 "worker\.(destroy|terminate|close)" --type tsLength of output: 2419
Script:
#!/bin/bash # Search for wait queue implementation and worker state management echo "=== Wait Queue Implementation ===" ast-grep --pattern 'class WaitQueue { $$$ }' echo -e "\n=== Worker State Management ===" rg -B 5 -A 10 "class PoolingWorkerManager" --type ts echo -e "\n=== Request Handling ===" rg -B 2 -A 5 "execute\(" --type tsLength of output: 7526
Script:
#!/bin/bash # Get the complete implementation of pooling worker manager echo "=== Pooling Worker Manager Implementation ===" cat src/typegate/src/runtimes/patterns/worker_manager/pooling.ts echo -e "\n=== Worker Interface ===" rg -B 2 -A 5 "interface Worker" --type tsLength of output: 6338
Possible race condition so it is not guaranteed that there will be a shifted item.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #966 +/- ##
==========================================
- Coverage 78.04% 77.77% -0.27%
==========================================
Files 157 160 +3
Lines 19283 19657 +374
Branches 1945 1965 +20
==========================================
+ Hits 15049 15289 +240
- Misses 4211 4347 +136
+ Partials 23 21 -2 ☔ View full report in Codecov by Sentry. |
Migration notes
Summary by CodeRabbit
Release Notes
New Features
WasmWorkerclass for managing WebAssembly workers.Configuration Changes
substantial_worker_wait_timeout_mswith more granular worker configuration options.MIN_WASM_WORKERSandMAX_WASM_WORKERS.Runtime Improvements
destroyoperations in some runtime components.Dependency Updates
0.5.1-rc.0.Performance Optimizations
Code Quality