feat: worker pooling #962
feat: worker pooling #962
Conversation
📝 WalkthroughWalkthroughThis pull request encompasses multiple changes across various files in the TypeGate runtime system, focusing on worker management, error handling, and lifecycle management. The modifications primarily involve updating methods for worker allocation, deallocation, and initialization, introducing more robust error handling, and refining the control flow for task delegation and worker management. The changes span several runtime components, including Deno, substantial workflows, and worker pooling mechanisms. Changes
Sequence DiagramsequenceDiagram
participant WorkerManager
participant WaitQueue
participant Worker
WorkerManager->>WaitQueue: Push task
WaitQueue-->>WorkerManager: Confirm task queued
WorkerManager->>Worker: Delegate task
Worker-->>WorkerManager: Task result
WorkerManager->>WaitQueue: Shift next task
WorkerManager->>Worker: Deallocate worker
Possibly related PRs
Suggested reviewers
📜 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 (6)
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 (
|
Natoandro
requested review from
michael-0acf4,
Yohe-Am,
zifeo and
luckasRanarison
and removed request for
michael-0acf4 and
Yohe-Am
# Conflicts: # .ghjk/deno.lock # deno.lock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/typegate/src/runtimes/patterns/worker_manager/mod.ts (1)
Line range hint
179-229: RefactordeallocateWorkerfor improved readabilityThe
deallocateWorkermethod contains complex logic handling multiple scenarios. Consider refactoring the method into smaller helper functions to enhance readability and maintainability.tests/patterns/worker_manager/pooling_test.ts (1)
33-78: LGTM! Robust timeout testing with edge cases.The test suite effectively covers timeout scenarios:
- Synchronous execution
- Small delay handling
- Timeout error cases
Consider adding tests for:
- Concurrent queue operations
- Multiple timeouts
- Queue cleanup after timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.ghjk/deno.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
src/typegate/engine/src/runtimes/wit_wire.rs(1 hunks)src/typegate/src/runtimes/deno/deno.ts(1 hunks)src/typegate/src/runtimes/deno/worker_manager.ts(2 hunks)src/typegate/src/runtimes/patterns/worker_manager/deno.ts(1 hunks)src/typegate/src/runtimes/patterns/worker_manager/mod.ts(5 hunks)src/typegate/src/runtimes/patterns/worker_manager/pooling.ts(1 hunks)src/typegate/src/runtimes/substantial/agent.ts(4 hunks)src/typegate/src/runtimes/substantial/types.ts(2 hunks)src/typegate/src/runtimes/substantial/worker.ts(0 hunks)src/typegate/src/runtimes/substantial/workflow_worker_manager.ts(2 hunks)tests/patterns/worker_manager/pooling_test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/typegate/src/runtimes/substantial/worker.ts
✅ Files skipped from review due to trivial changes (1)
- src/typegate/engine/src/runtimes/wit_wire.rs
🧰 Additional context used
🪛 Biome (1.9.4)
src/typegate/src/runtimes/substantial/types.ts
[error] 89-89: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ 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: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (22)
src/typegate/src/runtimes/patterns/worker_manager/pooling.ts (4)
4-8: Well-definedPoolConfigenhances worker pool configurabilityThe
PoolConfigtype is well-structured, allowing flexible configuration of the worker pool with optional parameters for maximum workers, minimum workers, and wait timeout.
12-15: ClearWaitQueueinterface definitionThe
WaitQueue<W>interface is clearly defined withpushandshiftmethods, facilitating consistent implementation of wait queue mechanisms.
17-32: Efficient implementation of simple wait queueThe
createSimpleWaitQueuefunction provides an efficient FIFO queue without timeout handling, suitable for scenarios where tasks do not require timeouts.
34-102: RobustWaitQueueWithTimeoutmanages timeouts effectivelyThe
WaitQueueWithTimeoutclass effectively handles consumer timeouts, cancelling consumers as needed and managing the queue with precise timing.src/typegate/src/runtimes/patterns/worker_manager/mod.ts (8)
26-32:DeallocateOptionsprovides flexible worker deallocationIntroducing the
DeallocateOptionstype enhances control over worker deallocation, allowing conditional destruction of workers and ensuring minimum worker requirements are met.
39-67: Improved worker manager initialization with configuration supportThe updated constructor accepts a
nameandPoolConfig, enabling better identification and configuration of the worker manager. The unique worker IDs generated byworkerFactoryenhance debugging and monitoring.
119-130: Proper implementation of asynchronous worker waitingThe
#waitForWorker()method correctly returns a promise that resolves when a worker becomes available, ensuring tasks are queued appropriately when the maximum number of workers is reached.
131-148: AsynchronousdelegateTaskmethod enhances task delegationUpdating
delegateTaskto be asynchronous ensures that worker allocation is properly awaited, preventing potential race conditions and improving reliability.
149-161: Efficient deallocation of all workers with optionsThe
deallocateAllWorkersmethod allows for flexible deallocation of workers usingDeallocateOptions, providing control over worker destruction and minimum worker requirements.
165-177: Correct deallocation of workers by nameThe
deallocateWorkersByNamemethod effectively deallocates all workers associated with a specific task name, aiding in targeted resource cleanup.
240-255: Comprehensive resource cleanup indeinitmethodThe
deinitmethod ensures that all workers, both active and idle, are properly destroyed, effectively releasing resources during shutdown.
105-116: Verify maximum worker limit enforcementThe
#nextWorker()method checksthis.#activeTasks.sizeagainstmaxWorkersbefore creating new workers. Ensure that this logic correctly enforces the maximum worker limit, including idle workers in the count.Consider updating the condition to include idle workers by comparing
this.#workerCountwithmaxWorkers:- if (this.#activeTasks.size < this.#poolConfig.maxWorkers) + if (this.#workerCount < this.#poolConfig.maxWorkers)src/typegate/src/runtimes/deno/worker_manager.ts (2)
24-27: Enhanced worker identification in constructorPassing
"deno runtime"as the name to theBaseWorkerManagerconstructor improves worker identification and logging for the Deno runtime environment.
Line range hint
31-68: AsynchronouscallFunctionimproves worker managementMaking
callFunctionasynchronous and usingawait this.delegateTaskensures proper worker allocation before task execution. Replacingthis.destroyWorkerwiththis.deallocateWorkeraligns with the updated worker pooling strategy, promoting efficient resource management.src/typegate/src/runtimes/substantial/workflow_worker_manager.ts (2)
25-28: LGTM! Improved worker identification.The constructor now provides a descriptive name "substantial workflows" to the worker pool, enhancing observability and debugging capabilities.
Line range hint
64-85: LGTM! Enhanced worker management with async task delegation.The method now properly awaits task delegation, ensuring better resource management and preventing potential race conditions.
src/typegate/src/runtimes/substantial/types.ts (2)
69-76: LGTM! Improved type safety with const assertion.Using a const-asserted array for valid interrupts ensures type safety and centralizes the interrupt type definitions.
89-93: Enhanced error handling with descriptive messages.The getTypeOf method now provides better error handling by validating interrupts against the known list and throwing descriptive error messages.
Let's verify that all interrupt types used in the codebase are valid:
✅ Verification successful
Interrupt handling implementation verified successfully
The implementation correctly validates interrupts against a well-defined set of values (SLEEP, SAVE_RETRY, WAIT_RECEIVE_EVENT, WAIT_HANDLE_EVENT, WAIT_ENSURE_VALUE) with both compile-time type checking and runtime validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of Interrupt.Variant to ensure valid interrupt types ast-grep --pattern 'Interrupt.Variant("$_"'Length of output: 45
Script:
#!/bin/bash # Find interrupt type definitions and usages echo "=== Searching for interrupt type definitions ===" rg -l "type.*Interrupt" --type typescript echo "=== Searching for validInterrupts ===" rg "validInterrupts.*=" --type typescript echo "=== Searching for interrupt string literals ===" rg "interrupt.*[\"']" --type typescriptLength of output: 533
Script:
#!/bin/bash # Search in the specific directory for interrupt-related code echo "=== Searching for type definitions ===" rg "type.*Interrupt|interface.*Interrupt" src/typegate/src/runtimes/ echo "=== Searching for validInterrupts ===" rg "validInterrupts" src/typegate/src/runtimes/ echo "=== Searching for interrupt constants or enums ===" rg "const.*interrupt|enum.*interrupt" -i src/typegate/src/runtimes/Length of output: 1226
Script:
#!/bin/bash # Get the content of validInterrupts array echo "=== Showing validInterrupts definition ===" rg -A 5 "const validInterrupts = \[" src/typegate/src/runtimes/substantial/types.tsLength of output: 303
🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
tests/patterns/worker_manager/pooling_test.ts (1)
11-31: LGTM! Comprehensive testing of basic queue operations.The test thoroughly covers the simple wait queue functionality including:
- Empty queue behavior
- Push and shift operations
- History tracking
src/typegate/src/runtimes/substantial/agent.ts (2)
377-377: Add error handling for worker deallocation.Similar to the interrupt handler, worker deallocation here should include error handling.
118-118: Consider awaiting worker deinitialization.The
deinitcall should be awaited to ensure proper cleanup of worker resources before the agent stops completely.src/typegate/src/runtimes/patterns/worker_manager/deno.ts (1)
46-53: LGTM! Improved error handling implementation.The changes properly handle asynchronous error handling and provide a clearer error object structure.
| @@ -182,7 +182,7 @@ export class DenoRuntime extends Runtime { | |||
| } | |||
|
|
|||
| async deinit(): Promise<void> { | |||
| // await this.workerManager.deinit(); | |||
| this.workerManager.deinit(); | |||
There was a problem hiding this comment.
Consider awaiting worker manager deinitialization.
Not awaiting workerManager.deinit() could lead to resource leaks or race conditions during shutdown. Consider maintaining the async await to ensure proper cleanup.
Apply this diff to ensure proper resource cleanup:
- this.workerManager.deinit();
+ await this.workerManager.deinit();Committable suggestion skipped: line range outside the PR's diff.
| @@ -325,7 +325,7 @@ export class Agent { | |||
| runId: string, | |||
| { interrupt, schedule, run }: InterruptEvent, | |||
| ) { | |||
| this.workerManager.destroyWorker(workflowName, runId); // ! | |||
| this.workerManager.deallocateWorker(workflowName, runId); // ! | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for worker deallocation.
The worker deallocation is a critical operation (as indicated by the "!!" comment) but lacks error handling. Consider wrapping it in a try-catch block to handle potential failures gracefully.
- this.workerManager.deallocateWorker(workflowName, runId); // !
+ try {
+ await this.workerManager.deallocateWorker(workflowName, runId);
+ } catch (error) {
+ this.logger.error(`Failed to deallocate worker for ${workflowName}:${runId}`, error);
+ // Consider whether to rethrow or handle gracefully
+ }Committable suggestion skipped: line range outside the PR's diff.
| ).then(() => { | ||
| this.workerManager.listen( | ||
| next.run_id, | ||
| this.#eventResultHandlerFor(workflow.name, next.run_id), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use async/await pattern instead of Promise chaining.
The current implementation could lead to:
- Race conditions between worker initialization and event listening
- Swallowed errors in the promise chain
- More complex error handling
Consider refactoring to:
- ).then(() => {
- this.workerManager.listen(
- next.run_id,
- this.#eventResultHandlerFor(workflow.name, next.run_id),
- );
- });
+ );
+ await this.workerManager.listen(
+ next.run_id,
+ this.#eventResultHandlerFor(workflow.name, next.run_id),
+ );Committable suggestion skipped: line range outside the PR's diff.
| @@ -60,7 +60,6 @@ self.onmessage = async function (event) { | |||
| { | |||
| type: "FAIL", | |||
| error: errorToString(wfException), | |||
| // How?? | |||
| exception: wfException instanceof Error | |||
| ? wfException | |||
| : undefined, | |||
There was a problem hiding this comment.
Would it not better to throw an unreachable error here?
There was a problem hiding this comment.
Wouldn't that still be reachable since users can throw arbitrary values?
But I suppose, instead, we can throw something that says it is not allowed, or simply wrap the stringified value thrown inside an Error instance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tools/tasks/lock.ts (1)
Line range hint
77-82: Remove unnecessary non-null assertion.The non-null assertion (
!) onlatestPreReleaseis redundant since the if condition already ensures it's not null.if (latestPreRelease) { assert( - semver.greaterThan(currentVersion, latestPreRelease!), + semver.greaterThan(currentVersion, latestPreRelease), "expected current version to be greater than latest pre-release version", ); }examples/templates/node/compose.yml (1)
Line range hint
11-12: Security: Avoid hardcoded secrets in templates.While these values are marked for development use, consider:
- Using placeholder values with clear instructions for production deployment
- Adding a warning comment about generating secure values
- Providing documentation links for security best practices
environment: - # only for dev, generate secure values for production + # WARNING: Development credentials only! + # For production: + # 1. Generate a secure random TG_SECRET using a cryptographically secure method + # 2. Set a strong TG_ADMIN_PASSWORD + # See: <link-to-security-docs> TG_SECRET: "a4lNi0PbEItlFZbus1oeH/+wyIxi9uH6TpL8AIqIaMBNvp7SESmuUBbfUwC0prxhGhZqHw8vMDYZAGMhSZ4fLw==" TG_ADMIN_PASSWORD: password
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.locktests/metagen/__snapshots__/metagen_test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
Cargo.toml(1 hunks)examples/templates/deno/api/example.ts(1 hunks)examples/templates/deno/compose.yml(1 hunks)examples/templates/node/compose.yml(1 hunks)examples/templates/node/package.json(1 hunks)examples/templates/python/compose.yml(1 hunks)examples/templates/python/pyproject.toml(1 hunks)pyproject.toml(1 hunks)src/pyrt_wit_wire/pyproject.toml(1 hunks)src/typegate/src/runtimes/wit_wire/mod.ts(1 hunks)src/typegraph/core/Cargo.toml(1 hunks)src/typegraph/core/src/global_store.rs(1 hunks)src/typegraph/deno/deno.json(1 hunks)src/typegraph/python/pyproject.toml(1 hunks)src/typegraph/python/typegraph/__init__.py(1 hunks)src/xtask/Cargo.toml(1 hunks)tests/e2e/published/sdk_test.ts(0 hunks)tests/metagen/typegraphs/sample/rs/Cargo.toml(1 hunks)tests/metagen/typegraphs/sample/rs_upload/Cargo.toml(1 hunks)tests/runtimes/wasm_reflected/rust/Cargo.toml(1 hunks)tools/consts.ts(1 hunks)tools/tasks/lock.ts(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/published/sdk_test.ts
✅ Files skipped from review due to trivial changes (16)
- tests/metagen/typegraphs/sample/rs_upload/Cargo.toml
- src/typegraph/python/pyproject.toml
- tests/runtimes/wasm_reflected/rust/Cargo.toml
- src/xtask/Cargo.toml
- examples/templates/deno/compose.yml
- tests/metagen/typegraphs/sample/rs/Cargo.toml
- src/typegraph/python/typegraph/init.py
- src/typegraph/deno/deno.json
- pyproject.toml
- src/typegate/src/runtimes/wit_wire/mod.ts
- src/typegraph/core/src/global_store.rs
- src/typegraph/core/Cargo.toml
- src/pyrt_wit_wire/pyproject.toml
- examples/templates/python/compose.yml
- Cargo.toml
- examples/templates/deno/api/example.ts
🔇 Additional comments (6)
tools/consts.ts (1)
4-6: LGTM! Version changes follow semantic versioning.The version updates are consistent:
- Current version increment to 0.5.1-rc.0 for new release candidate
- Latest release version updated to 0.5.0
- Pre-release version correctly set to null for new RC cycle
examples/templates/python/pyproject.toml (1)
3-3: LGTM! Version updates are consistent.Both the project version and typegraph dependency are correctly updated to "0.5.1-rc.0", maintaining consistency with the main version changes.
Also applies to: 9-9
tools/tasks/lock.ts (2)
68-71: LGTM! Improved null safety in version validation.The addition of the null check for
LATEST_PRE_RELEASE_VERSIONmakes the code more robust by preventing potential null reference errors.
Line range hint
1-1: Verify the scope of changes in this PR.The PR objectives mention "worker pooling" feature (MET-806), but the files under review only contain version-related changes. Please verify if:
- There are additional files that implement the worker pooling feature, or
- The PR description needs to be updated to reflect the actual changes.
✅ Verification successful
PR scope is aligned with its objectives
The PR implements the worker pooling feature (MET-806) in the
src/typegate/src/runtimes/patterns/worker_manager/directory, along with necessary dependency updates. The version-related changes appear to be supporting modifications for this feature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for worker-related changes in the codebase echo "Searching for worker-related changes..." rg -l 'worker|pool|thread' --type ts --type rust --type python # Check if there are other changed files in the PR echo "Checking for other changed files in the PR..." gh pr view 962 --json files -q '.files[].path'Length of output: 1771
examples/templates/node/package.json (1)
9-9: Consider stability implications of using RC version in templates.Using a release candidate version (
^0.5.1-rc.0) in template files might expose new users to potentially unstable features. Consider whether it would be more appropriate to wait for the stable release before updating the template.Let's check version consistency across the project:
examples/templates/node/compose.yml (1)
3-3: Verify image version consistency.The typegate image version has been updated to
v0.5.1-rc.0, which matches the SDK version update. This maintains version consistency across the template.Let's verify version consistency across Docker-related files:
✅ Verification successful
Docker image version is consistently set to v0.5.1-rc.0
All Docker-related configurations are using the same typegate image version (v0.5.1-rc.0). No version inconsistencies were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any other typegate image version references rg -g '*.{yml,yaml}' 'ghcr.io/metatypedev/typegate:v[0-9]+\.[0-9]+\.[0-9]+' --no-filenameLength of output: 247
Script:
#!/bin/bash # Check SDK version in package.json files rg -g 'package*.json' '@metatype/typegate.*"v?0\.5\.' --json | jq -r '.data.lines.text'Length of output: 92
Script:
#!/bin/bash # Broader search for typegate SDK dependency rg -g 'package*.json' '@metatype/typegate' -A 1Length of output: 47
| cancellationHandler: () => void; | ||
| addedAt: number; // timestamp | ||
| }> = []; |
There was a problem hiding this comment.
addedAt + Millis/Ms?
| const { maxWorkers } = this.#poolConfig; | ||
| // how?? xD | ||
| // We might add "urgent" tasks in the future; | ||
| // in this case the worker count might exceed `maxWorkers`. | ||
| if (maxWorkers != null && this.#workerCount >= maxWorkers) { | ||
| task.worker.destroy(); | ||
| } else { | ||
| this.#idleWorkers.push(task.worker); |
There was a problem hiding this comment.
I feel like we should never exceed max but instead ensure it is big enough at start. Not sure how that would fit here but a design idea is to allocate as much as needed with a small room for newer tasks: allocate minAlloc, then if that is not enough and then task is urgent then allocate minAlloc+5, (minAlloc+5)+5, ..., min(minAlloc + 5 + 5 + ..., maxAlloc) etc. 5 is completely arbitrary btw.
| if (timeoutMs <= 0) { | ||
| this.#cancelNextEntry(); | ||
| this.#updateTimer(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Wouldn't this be an infinite loop if Date.now() is greater? Unless I am missing something.. Or is addedAt updated somewhere?
There was a problem hiding this comment.
Yes. That is the goal. It always checks the front of the queue. If it has not changed, we are sure that timeoutMs is non positive, because the timeout has elapsed (#updateTimer is only called when we add an item on an empty queue or the timeout has elapsed). If the front of the queue has changed, then we have a different addedAt or nothing.
Note that queue items are added in a chronological order...
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/typegate/src/runtimes/patterns/worker_manager/pooling.ts (2)
120-120: Address the TODO for auto-removal of idle workersThere's a TODO comment about implementing auto-removal of idle workers after a certain time. Implementing this feature can improve resource management and prevent unnecessary resource consumption.
Would you like assistance in implementing the idle worker auto-removal functionality?
184-186: Remove informal comments to maintain professionalismThe comment
"how?? xD"is informal and may not be appropriate for production code. Consider rephrasing or removing it to maintain code professionalism.Apply this diff to revise the comment:
- // how?? xD - // We might add "urgent" tasks in the future; - // in this case the worker count might exceed `maxWorkers`. + // In the future, adding "urgent" tasks might cause the worker count to exceed `maxWorkers`.src/typegate/src/runtimes/substantial/workflow_worker_manager.ts (2)
15-15: Clarify or remove the TODO comment.The "TODO lazy" comment lacks context. Please either remove it or provide more details about what needs to be implemented.
35-35: Remove outdated TODO comment.The comment "TODO load from config" can be removed as the configuration is already being loaded from
globalConfig.src/typegate/src/runtimes/patterns/worker_manager/mod.ts (1)
163-167: Consider awaiting worker cleanup.The
deinitmethod could be improved by:
- Making it async
- Awaiting the worker cleanup process
- Ensuring all workers are properly destroyed before resolving
- deinit() { + async deinit() { this.deallocateAllWorkers({ destroy: true, shutdown: true }); - this.#pool.clear(); // this will be called more than once, but that is ok - return Promise.resolve(); + await this.#pool.clear(); }
📜 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 (6)
src/typegate/src/config/types.ts(2 hunks)src/typegate/src/runtimes/deno/worker_manager.ts(3 hunks)src/typegate/src/runtimes/patterns/worker_manager/deno.ts(4 hunks)src/typegate/src/runtimes/patterns/worker_manager/mod.ts(6 hunks)src/typegate/src/runtimes/patterns/worker_manager/pooling.ts(1 hunks)src/typegate/src/runtimes/substantial/workflow_worker_manager.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/typegate/src/runtimes/patterns/worker_manager/deno.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: test-full
🔇 Additional comments (11)
src/typegate/src/runtimes/patterns/worker_manager/pooling.ts (2)
46-48: Clarify units inaddedAttimestampConsider renaming the
addedAtproperty toaddedAtMsortimestampMsto make it clear that the timestamp is in milliseconds.
84-92: Potential infinite recursion in#updateTimermethodThere may be a potential for infinite recursion if
timeoutMsremains non-positive, as#updateTimercalls itself recursively without a base case whentimeoutMs <= 0. Ensure thataddedAtis updated appropriately, or modify the logic to prevent infinite recursion.src/typegate/src/runtimes/deno/worker_manager.ts (2)
25-38: Ensure thread-safe initialization of static#poolThe
#getPool()static method initializes the static#poolproperty without synchronization. IfWorkerManagerinstances are created concurrently, this could lead to race conditions and multiple pool instances. Consider implementing synchronization to ensure thread-safe initialization, such as using a mutex or double-checked locking pattern.
Line range hint
44-75: Verify proper handling of async operations and resource deallocationWith
callFunctionnow beingasyncand usingawait this.delegateTask(...), ensure that all promises are correctly handled and any exceptions are caught to prevent unhandled promise rejections. Also, confirm that replacingthis.destroyWorkerwiththis.deallocateWorkercorrectly manages worker resources and does not introduce memory leaks or dangling workers.src/typegate/src/runtimes/substantial/workflow_worker_manager.ts (3)
47-48: LGTM!The constructor correctly initializes the worker manager with the singleton pool instance.
Line range hint
85-105: LGTM!The async implementation correctly delegates task creation to the worker pool while maintaining the existing message structure.
37-39: Verify configuration values for worker pool.Please ensure that the configuration values are appropriate for your use case:
minWorkers: Minimum number of workers to maintainmaxWorkers: Maximum number of workers allowedwaitTimeoutMs: Timeout for worker allocationRun this script to check the configuration values:
✅ Verification successful
Worker pool configuration values are properly defined and reasonable ✅
The configuration is well-structured with appropriate default values:
minWorkers: 2 (prevents cold starts)maxWorkers: 8 (prevents resource exhaustion)waitTimeoutMs: 15000 (15s timeout for worker allocation)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check configuration values in the codebase rg -A 2 "min_substantial_workers|max_substantial_workers|substantial_worker_wait_timeout_ms" --type tsLength of output: 1110
src/typegate/src/runtimes/patterns/worker_manager/mod.ts (4)
21-25: LGTM!The
DeallocateOptionstype is well-documented and provides good control over worker lifecycle management.
74-89: LGTM!The
delegateTaskmethod correctly integrates with the worker pool while maintaining proper task tracking.
92-119: LGTM!The deallocation methods provide good control over worker lifecycle with appropriate logging.
Line range hint
122-149: Consider implementing a more gradual worker allocation strategy.While the current implementation is functional, consider implementing a more sophisticated worker allocation strategy as suggested in previous discussions. This could involve:
- Gradual allocation with small increments (e.g., minAlloc + 5)
- Predictive allocation based on task patterns
- Better handling of worker pool exhaustion
| min_deno_workers: z.coerce.number().positive().default(2), | ||
| max_deno_workers: z.coerce.number().positive().default(8), | ||
| deno_worker_wait_timeout_ms: z.coerce.number().positive().default(5000), | ||
| // deno_idle_worker_timeout_ms: z.coerce.number().positive().optional(), // auto remove idle workers | ||
| min_substantial_workers: z.coerce.number().positive().default(2), | ||
| max_substantial_workers: z.coerce.number().positive().default(8), | ||
| substantial_worker_wait_timeout_ms: z.coerce.number().positive().default( | ||
| 15000, | ||
| ), | ||
| // substantial_idle_worker_timeout_ms: z.coerce.number().positive().optional(), // auto remove idle workers |
There was a problem hiding this comment.
Allow zero values for worker configuration parameters
The use of .positive() in z.coerce.number().positive() disallows zero values. However, in the worker pool configuration, a value of zero for maxWorkers or waitTimeoutMs may be used to indicate "no limit" or "no timeout". Consider using .nonnegative() instead to allow zero values where appropriate.
Apply this diff to allow zero values:
- min_deno_workers: z.coerce.number().positive().default(2),
- max_deno_workers: z.coerce.number().positive().default(8),
- deno_worker_wait_timeout_ms: z.coerce.number().positive().default(5000),
- min_substantial_workers: z.coerce.number().positive().default(2),
- max_substantial_workers: z.coerce.number().positive().default(8),
- substantial_worker_wait_timeout_ms: z.coerce.number().positive().default(
- 15000,
- ),
+ min_deno_workers: z.coerce.number().nonnegative().default(2),
+ max_deno_workers: z.coerce.number().nonnegative().default(8),
+ deno_worker_wait_timeout_ms: z.coerce.number().nonnegative().default(5000),
+ min_substantial_workers: z.coerce.number().nonnegative().default(2),
+ max_substantial_workers: z.coerce.number().nonnegative().default(8),
+ substantial_worker_wait_timeout_ms: z.coerce.number().nonnegative().default(
+ 15000,
+ ),📝 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.
| min_deno_workers: z.coerce.number().positive().default(2), | |
| max_deno_workers: z.coerce.number().positive().default(8), | |
| deno_worker_wait_timeout_ms: z.coerce.number().positive().default(5000), | |
| // deno_idle_worker_timeout_ms: z.coerce.number().positive().optional(), // auto remove idle workers | |
| min_substantial_workers: z.coerce.number().positive().default(2), | |
| max_substantial_workers: z.coerce.number().positive().default(8), | |
| substantial_worker_wait_timeout_ms: z.coerce.number().positive().default( | |
| 15000, | |
| ), | |
| // substantial_idle_worker_timeout_ms: z.coerce.number().positive().optional(), // auto remove idle workers | |
| min_deno_workers: z.coerce.number().nonnegative().default(2), | |
| max_deno_workers: z.coerce.number().nonnegative().default(8), | |
| deno_worker_wait_timeout_ms: z.coerce.number().nonnegative().default(5000), | |
| // deno_idle_worker_timeout_ms: z.coerce.number().positive().optional(), // auto remove idle workers | |
| min_substantial_workers: z.coerce.number().nonnegative().default(2), | |
| max_substantial_workers: z.coerce.number().nonnegative().default(8), | |
| substantial_worker_wait_timeout_ms: z.coerce.number().nonnegative().default( | |
| 15000, | |
| ), | |
| // substantial_idle_worker_timeout_ms: z.coerce.number().positive().optional(), // auto remove idle workers |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/metatype.dev/docs/reference/typegate/index.mdx (1)
85-90: Enhance documentation with configuration guidelines.Consider adding the following improvements to make the documentation more helpful:
- Add a note explaining the relationship between min and max workers (e.g.,
MIN_*_WORKERSmust be less than or equal toMAX_*_WORKERS).- Provide guidance on choosing appropriate values based on workload characteristics.
- Explain why Substantial workers have a longer default timeout than Deno workers.
- Review the example values to ensure they demonstrate best practices (current examples show shorter timeouts than defaults).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/metatype.dev/docs/reference/typegate/index.mdx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (1)
docs/metatype.dev/docs/reference/typegate/index.mdx (1)
85-90: LGTM! Documentation follows the established format.The new environment variables are well-organized and consistently documented, maintaining the table structure and providing essential information for each configuration option.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/metatype.dev/docs/reference/typegate/index.mdx (3)
71-71: Fix grammar in the description.The description has incorrect grammar. Change "The rate of transactions be sent to Sentry" to "The rate of transactions to be sent to Sentry".
-| SENTRY_TRACES_SAMPLE_RATE | The rate of transactions be sent to Sentry (between 0.0 and 1.0) | 1.0 | 0.2 | +| SENTRY_TRACES_SAMPLE_RATE | The rate of transactions to be sent to Sentry (between 0.0 and 1.0) | 1.0 | 0.2 |🧰 Tools
🪛 LanguageTool
[grammar] ~71-~71: Make sure the noun ‘rate’ is in agreement with the verb ‘be’. Beware that some collective nouns (like ‘police’ or ‘team’) can be treated as both singular and plural.
Context: ...ATE | The rate of transactions be sent to Sentry (between 0.0 and 1.0) ...(DT_NN_OF_NNS_VB)
85-90: Consider adding section headers in the environment variables table.To improve readability, consider adding section headers or comments to group related environment variables. For example, add headers for "Worker Pool Configuration" before these new variables.
85-90: Enhance documentation with additional technical details.The worker pool configuration would benefit from additional clarification:
- Explain the relationship between min and max workers (e.g., when does scaling occur?)
- Clarify why substantial worker timeout (15s) is significantly higher than deno worker timeout (5s)
- Document the behavior when worker wait timeouts are reached
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/metatype.dev/docs/reference/typegate/index.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/metatype.dev/docs/reference/typegate/index.mdx
[grammar] ~71-~71: Make sure the noun ‘rate’ is in agreement with the verb ‘be’. Beware that some collective nouns (like ‘police’ or ‘team’) can be treated as both singular and plural.
Context: ...ATE | The rate of transactions be sent to Sentry (between 0.0 and 1.0) ...
(DT_NN_OF_NNS_VB)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (1)
docs/metatype.dev/docs/reference/typegate/index.mdx (1)
85-90: Documentation aligns well with worker pooling feature.The added environment variables provide comprehensive configuration options for both Deno and Substantial worker pools, which aligns perfectly with the PR's objective of implementing worker pooling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/published/typegate_upgrade_test.ts (1)
36-37: Avoid permanently ignoring testsMarking the entire test as
ignore: truemight hide regressions and reduce test coverage. Consider conditionally skipping only for the problematic scenario or addressing the underlying issue so that the test can remain enabled.
📜 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 (3)
examples/metatype.yaml(1 hunks)examples/typegraphs/metagen/rs/fdk.rs(1 hunks)tests/e2e/published/typegate_upgrade_test.ts(2 hunks)
⏰ 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: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (5)
tests/e2e/published/typegate_upgrade_test.ts (2)
102-117: Ensure sensitive data is handled securelyWhen passing environment variables like
TG_SECRETandTG_ADMIN_PASSWORD, confirm that logs or error messages do not inadvertently expose these secrets. It might also be preferable to supply them at runtime or via secure configuration management rather than in source code.
124-124: Confirm the updated log pattern aligns with actual outputYou changed the readiness check to look for
typegate ready on :${port}. Verify that this string is correct and consistent across all relevant logs and references, preventing false positives or negatives in test detection.examples/typegraphs/metagen/rs/fdk.rs (1)
112-112: Proactively verify version usage across the codebase.This update from
"0.5.0"to"0.5.1-rc.0"looks consistent here. However, ensure there are no lingering references to the old version in other files or documentation.Run this script to verify no references to
0.5.0remain elsewhere:✅ Verification successful
Version references are correctly maintained across the codebase
The version update to "0.5.1-rc.0" in the FDK is correct, while keeping "0.5.0" as the latest stable release version in tools/consts.ts is intentional for testing and upgrade purposes. Other occurrences of "0.5.0" are in third-party dependencies and historical changelog entries, which are expected to remain unchanged.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for lingering references to 0.5.0 across the repository rg "0\.5\.0" -A 3Length of output: 10654
Script:
#!/bin/bash # Search for usages of LATEST_RELEASE_VERSION rg "LATEST_RELEASE_VERSION" -A 3Length of output: 2139
examples/metatype.yaml (2)
166-166: Verify the impact of excluding the scripts directory.The addition of
typegraphs/scripts/to the TypeScript exclusion patterns could affect worker initialization if any worker-related scripts are present in this directory.Let's verify the contents and usage of the excluded directory:
✅ Verification successful
Exclusion of typegraphs/scripts is verified and correct ✅
The directory contains build/publish utility scripts that don't require TypeScript processing, as evidenced by its usage in SDK publishing tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the contents and usage of the excluded scripts directory # Check what files exist in the excluded directory echo "Files in typegraphs/scripts/:" fd . "typegraphs/scripts" # Check for any imports from the excluded directory echo -e "\nImports from typegraphs/scripts/:" rg -g '*.ts' -g '*.js' "from ['\"]\.\./scripts/" "typegraphs/"Length of output: 400
Script:
#!/bin/bash # Search for any mentions of the scripts directory pattern echo "Searching for mentions of typegraphs/scripts:" rg "typegraphs/scripts" --type yaml --type md --type ts --type js echo -e "\nChecking for similar directory patterns:" fd scripts echo -e "\nChecking documentation for context:" fd -e md . | xargs rg -l "scripts" || trueLength of output: 1065
164-168: Ensure documentation reflects the TypeScript exclusion changes.The TypeScript exclusion patterns have been updated, but we should verify that the documentation has been updated to reflect these changes, especially if this affects the worker pooling feature.
Let's check if the documentation has been updated:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #962 +/- ##
==========================================
- Coverage 78.04% 77.62% -0.42%
==========================================
Files 157 158 +1
Lines 19283 19529 +246
Branches 1945 1961 +16
==========================================
+ Hits 15049 15160 +111
- Misses 4211 4345 +134
- Partials 23 24 +1 ☔ View full report in Codecov by Sentry. |
Migration notes
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
Tests
Chores