Task bridge #6456
Task bridge #6456
Conversation
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for your contribution! I've reviewed the TaskBridge implementation and found several issues that need attention. The core functionality looks solid, but there are some critical error handling and concurrency issues that should be addressed.
|
|
||
| this.reconnectTimeout = setTimeout(() => { | ||
| this.reconnectTimeout = null | ||
| this.initialize() |
There was a problem hiding this comment.
Missing error handling here could lead to unhandled promise rejections. Consider wrapping this call in a try-catch block and implementing proper error recovery or logging.
| }, this.config.reconnectDelay) | ||
| } | ||
|
|
||
| private setupTaskEventListeners(task: Task) { |
There was a problem hiding this comment.
Potential memory leak: if this method fails partway through setting up event listeners, some listeners may remain attached without being tracked in taskEventHandlers. Consider implementing cleanup logic or using a transaction-like approach.
| this.config = { url, namespace, reconnectOnError, maxReconnectAttempts, reconnectDelay } | ||
| } | ||
|
|
||
| public static getInstance(config?: TaskBridgeConfig) { |
There was a problem hiding this comment.
Race condition: multiple concurrent calls to getInstance() could create multiple instances if called before the first instance is fully constructed. Consider using a lock or making the constructor async to prevent this.
| console.log(`Received task_event event for task: ${taskId}`, message.payload) | ||
| break | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
Is this error handling intentional? Message parsing errors are only logged to console but don't trigger any recovery mechanism. Should this be more robust for production use?
| await this.subscriber.unsubscribe(this.serverChannel(taskId)) | ||
| } | ||
|
|
||
| public async publish(taskId: string, type: TaskBridgeMessageType, payload: TaskBridgeMessagePayload) { |
There was a problem hiding this comment.
The connection check here only validates isConnected flag but doesn't verify if the Redis clients are actually ready to publish. Consider checking client.status === 'ready' as well.
src/services/task-bridge/index.ts
Outdated
| export { | ||
| type TaskBridgeConfig, | ||
| type TaskBridgeMessage, | ||
| type QueuedMessage, |
There was a problem hiding this comment.
The QueuedMessage interface is exported but doesn't seem to be used in the main service implementation. Is this intended for future use or should it be documented?
|
|
||
| // TODO: Figure out when to enable task bridge. | ||
| // eslint-disable-next-line no-constant-condition | ||
| if (true) { |
There was a problem hiding this comment.
TODO comment with hard-coded true condition - is there a plan for when this should be conditionally enabled? This might be worth addressing before merging.
hannesrudolph
added
the
Issue/PR - Triage
No description provided.