fix(gate): task timeout management #957
Conversation
📝 WalkthroughWalkthroughThe changes involve significant modifications to the Changes
Sequence DiagramsequenceDiagram
participant AsyncMessenger
participant Task
participant Broker
AsyncMessenger->>Task: execute()
AsyncMessenger->>Task: startTaskTimer()
activate Task
Task-->>AsyncMessenger: timeoutId
AsyncMessenger-->>Task: monitor pulse
alt Timeout occurs
AsyncMessenger->>Task: receive()
else Task completes
Task->>AsyncMessenger: resolve promise
end
deactivate Task
The sequence diagram illustrates the new task execution and timeout management flow, showing how the 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 (
|
![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/messenger/async_messenger.ts (2)
Line range hint
33-33: Remove unused timer property.The
#timerproperty appears to be unused after the refactoring. It can be safely removed.- #timer?: ReturnType<typeof setInterval>;
110-114: Document resource destruction behavior.The resource destruction behavior on timeout is significant and should be documented. Consider adding a JSDoc comment explaining when and why resources are destroyed.
+ /** + * Destroys and recreates the broker when a task times out if timer_destroy_resources is enabled. + * This helps prevent resource leaks and reset the broker to a clean state. + */ if (this.config.timer_destroy_resources) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts(4 hunks)src/typegate/src/runtimes/patterns/messenger/types.ts(1 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 (3)
src/typegate/src/runtimes/patterns/messenger/types.ts (1)
14-15: LGTM! Clean formatting of union types.The separation of union types into distinct lines improves code readability.
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (2)
23-23: LGTM! Clear type definition.The config type clearly specifies the required timer-related configuration fields.
72-74: LGTM! Proper timeout initialization.The timeout is now properly initialized and stored with the task data.
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts
Outdated
Show resolved
Hide resolved
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (2)
104-108: Consider sanitizing the error message.The current implementation includes the entire task object in the error message, which might expose sensitive information. Consider creating a sanitized version with only necessary fields.
-const data = JSON.stringify(task, null, 2); +const data = JSON.stringify({ + id: task.id, + op: task.op, + remainingPulseCount: task.remainingPulseCount +}, null, 2);
92-117: Add documentation for the recursive timeout strategy.The recursive timeout implementation is non-trivial. Consider adding JSDoc comments explaining:
- The purpose of remainingPulseCount
- The recursive timeout mechanism
- Resource cleanup behavior
+/** + * Manages task timeouts using a recursive strategy. + * @param task The message task to monitor + * @returns The timeout ID for the current check + * + * The method implements a pulse-based monitoring system where: + * - Each pulse decrements remainingPulseCount + * - Tasks with remaining pulses are rescheduled + * - Tasks with no remaining pulses trigger timeout handling + */ private startTaskTimer(task: Message<M>) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (1)
Learnt from: luckasRanarison
PR: metatypedev/metatype#957
File: src/typegate/src/runtimes/patterns/messenger/async_messenger.ts:92-117
Timestamp: 2025-01-09T07:12:37.705Z
Learning: In AsyncMessenger class, task IDs are incremented sequentially and tasks are removed upon completion, ensuring no timeout collisions. Previous timeouts don't need to be cleared as tasks with same IDs won't exist simultaneously due to the increment and cleanup mechanism.
⏰ 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: test-full
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: pre-commit
🔇 Additional comments (4)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (4)
23-23: LGTM: Clear and well-defined configuration type.The type definition correctly specifies the required configuration properties for timeout management.
52-57: LGTM: Proper cleanup of tasks and timeouts.The implementation correctly handles task cleanup by clearing timeouts and resolving promises before termination.
72-74: LGTM: Clean timeout initialization and storage.The implementation properly initializes and stores the timeout ID for each task, maintaining the task lifecycle.
110-114: Verify the resource cleanup strategy.The current implementation destroys and recreates the broker on timeout. This might be too aggressive if multiple tasks are running concurrently.
Consider:
- Adding a grace period before broker reset
- Logging additional context about concurrent tasks
✅ Verification successful
The broker reset implementation is appropriate and safe
The current implementation is well-designed with proper task management:
- Tasks are tracked independently of the broker state
- The reset only occurs for timed-out tasks and is configurable
- Concurrent tasks are properly managed through the task tracking system
- The terminate() method ensures clean shutdown by waiting for all tasks
The aggressive reset strategy is intentional and safe as it only affects tasks that have already timed out, while maintaining the integrity of other running tasks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for broker usage patterns to understand impact rg -A 3 "this\.broker = this\.#start" .Length of output: 795
Script:
#!/bin/bash # Check broker implementation and task management ast-grep --pattern 'class $_Messenger { $$$ #start($_) { $$$ } $$$ }' echo "---" # Look for task tracking or queue management rg -B 2 -A 5 "task|queue|concurrent" src/typegate/src/runtimes/patterns/messenger/Length of output: 6097
|
Ouch, I missed something |
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts
Outdated
Show resolved
Hide resolved
luckasRanarison
dismissed stale reviews from michael-0acf4 and Natoandro
via
e54b954
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (1)
96-125: Consider enhancing error handling and documentationThe implementation of task timeout management is solid, but could benefit from these improvements:
- Structure the error message as JSON for better parsing
- Add JSDoc comments explaining the recursive timeout strategy and pulse count mechanism
private startTaskTimer(message: Message<M>) { + /** + * Manages task timeouts with support for pulse counting. + * When pulse count > 0, the timeout is reset and decremented, + * allowing long-running tasks to stay alive by "pulsing". + * @param message The task message containing ID and pulse count + * @returns Timeout ID for cleanup + */ return setTimeout(() => { const task = this.#tasks.get(message.id); if (!task) return; if ( message.remainingPulseCount !== undefined && message.remainingPulseCount > 0 ) { // check again next time if unterminated message.remainingPulseCount -= 1; task.timeoutId = this.startTaskTimer(message); return; } // default behavior or 0 pulse left - const data = JSON.stringify(message, null, 2); this.receive({ id: message.id, - error: `${this.#timeoutSecs}s timeout exceeded: ${data}`, + error: JSON.stringify({ + code: 'TASK_TIMEOUT', + message: `Task timeout after ${this.#timeoutSecs}s`, + details: message + }) }); if (this.config.timer_destroy_resources) { this.#stop(this.broker); logger.info("reset broker after timeout"); this.broker = this.#start(this.receive.bind(this)); } }, this.config.timer_max_timeout_ms); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts(3 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: luckasRanarison
PR: metatypedev/metatype#957
File: src/typegate/src/runtimes/patterns/messenger/async_messenger.ts:92-117
Timestamp: 2025-01-09T07:12:37.705Z
Learning: In AsyncMessenger class, task IDs are incremented sequentially and tasks are removed upon completion, ensuring no timeout collisions. Previous timeouts don't need to be cleared as tasks with same IDs won't exist simultaneously due to the increment and cleanup mechanism.
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (1)
Learnt from: luckasRanarison
PR: metatypedev/metatype#957
File: src/typegate/src/runtimes/patterns/messenger/async_messenger.ts:92-117
Timestamp: 2025-01-09T07:12:37.705Z
Learning: In AsyncMessenger class, task IDs are incremented sequentially and tasks are removed upon completion, ensuring no timeout collisions. Previous timeouts don't need to be cleared as tasks with same IDs won't exist simultaneously due to the increment and cleanup mechanism.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (4)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (4)
23-23: LGTM: Config type updateThe addition of
timer_destroy_resourcestoAsyncMessengerConfigtype is well-defined and properly marked as readonly.
52-57: LGTM: Proper cleanup of task timeoutsThe terminate method now correctly cleans up task-specific timeouts before resolving promises, ensuring no memory leaks.
82-84: LGTM: Proper cleanup in receiveThe receive method correctly cleans up the task's timeout before processing the response, maintaining clean task lifecycle management.
72-74: LGTM: Task execution with timeout managementThe execute method properly initializes and stores task-specific timeouts. The implementation aligns with the sequential task ID mechanism mentioned in the learnings, preventing timeout collisions.
Let's verify the task ID increment mechanism:
✅ Verification successful
✓ Verified: Task ID generation is correctly implemented
The implementation uses a private counter that generates sequential IDs and properly wraps around using modulo operation, ensuring unique task IDs for timeout management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that task IDs are properly incremented and wrapped ast-grep --pattern 'nextId(): number { $$$ this.#counter += 1; this.#counter %= $_ $$$ }'Length of output: 99
Script:
#!/bin/bash # Search for ID generation methods with more flexible pattern ast-grep --pattern 'private #counter' # Find any method that might handle ID generation ast-grep --pattern 'getId(): number { $$$ }' # Backup search with ripgrep for counter-related code rg "#counter" -A 3 -B 3Length of output: 1932
| clearTimeout(t.timeoutId); | ||
| return t.promise; | ||
| }), | ||
| ); | ||
| logger.info(`close worker ${this.constructor.name}`); | ||
| await this.#stop(this.broker); | ||
| clearInterval(this.#timer); |
There was a problem hiding this comment.
Oh, you're right
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #957 +/- ##
==========================================
- Coverage 77.72% 77.69% -0.04%
==========================================
Files 154 154
Lines 19154 19144 -10
Branches 1930 1928 -2
==========================================
- Hits 14888 14873 -15
- Misses 4243 4248 +5
Partials 23 23 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (1)
94-108: Consider implementing backoff strategy for pulse retriesThe current implementation recursively creates new timeouts without any backoff strategy, which could lead to memory pressure under certain conditions.
Consider implementing an exponential backoff strategy for pulse retries:
if ( message.remainingPulseCount !== undefined && message.remainingPulseCount > 0 ) { // check again next time if unterminated message.remainingPulseCount -= 1; - task.timeoutId = this.startTaskTimer(message); + const backoffFactor = Math.pow(2, message.initialPulseCount - message.remainingPulseCount); + const nextTimeout = Math.min(this.config.timer_max_timeout_ms * backoffFactor, MAX_TIMEOUT); + task.timeoutId = setTimeout(() => this.checkTaskTimeout(message), nextTimeout); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts(3 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: luckasRanarison
PR: metatypedev/metatype#957
File: src/typegate/src/runtimes/patterns/messenger/async_messenger.ts:92-117
Timestamp: 2025-01-09T07:12:37.705Z
Learning: In AsyncMessenger class, task IDs are incremented sequentially and tasks are removed upon completion, ensuring no timeout collisions. Previous timeouts don't need to be cleared as tasks with same IDs won't exist simultaneously due to the increment and cleanup mechanism.
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (1)
Learnt from: luckasRanarison
PR: metatypedev/metatype#957
File: src/typegate/src/runtimes/patterns/messenger/async_messenger.ts:92-117
Timestamp: 2025-01-09T07:12:37.705Z
Learning: In AsyncMessenger class, task IDs are incremented sequentially and tasks are removed upon completion, ensuring no timeout collisions. Previous timeouts don't need to be cleared as tasks with same IDs won't exist simultaneously due to the increment and cleanup mechanism.
⏰ 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: test-full
🔇 Additional comments (4)
src/typegate/src/runtimes/patterns/messenger/async_messenger.ts (4)
51-56: LGTM: Improved resource cleanupThe changes properly clean up timeouts before resolving promises, preventing potential memory leaks.
80-82: LGTM: Proper timeout cleanup on task completionThe code correctly cleans up the timeout when a task completes, preventing unnecessary timer executions.
23-23: Breaking change: Update configuration typeThe addition of
timer_destroy_resourcestoTypegateConfigBaserequires users to update their configuration. This should be documented in migration notes.
70-72: Verify task timeout race condition handlingWhile the timeout initialization looks good, there's a potential edge case where a task could complete right after
startTaskTimerbut beforethis.#tasks.set. Consider moving the timeout initialization after setting the task.- const timeoutId = this.startTaskTimer(message); - this.#tasks.set(id, { promise, hooks, timeoutId }); + this.#tasks.set(id, { promise, hooks, timeoutId: null }); + const timeoutId = this.startTaskTimer(message); + this.#tasks.get(id)!.timeoutId = timeoutId;
|
Closing because of #954 |
Migration notes
Summary by CodeRabbit
New Features
Improvements