chore(rivetkit): buffer outbound tunnel messages #3509
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code Review: Buffer Outbound Tunnel MessagesSummaryThis PR implements message buffering for outbound tunnel messages when the WebSocket connection is not ready. The implementation follows the same pattern as the existing KV request buffering system and addresses the issue of dropped tunnel messages during connection interruptions. Positive Aspects
Issues and Concerns1. Memory Leak Risk - Unbounded Buffer Growth - HIGH PRIORITYLocation: tunnel.ts:60-65 The bufferedMessages array has no size limit. If the WebSocket remains disconnected for an extended period while tunnel messages continue to be sent, this buffer will grow unbounded, potentially causing an out-of-memory error. Recommendation: Add a maximum buffer size with appropriate handling (FIFO eviction when full). 2. Missing Buffer Cleanup on Shutdown - MEDIUM PRIORITYLocation: tunnel.ts:97-124 The shutdown() method does not clear bufferedMessages. While the buffer will be garbage collected when the object is destroyed, explicitly clearing it is a best practice. Recommendation: Add this.bufferedMessages = [] at the start of shutdown(). 3. Potential Message Ordering Issue - MEDIUM PRIORITYLocation: tunnel.ts:481-495 and tunnel.ts:79-95 When resendBufferedEvents() is called, it attempts to send all buffered messages. However, if sendMessage() encounters an actor that does not exist (line 504-511), it logs a warning and returns without sending. This means the message is lost (not re-buffered), subsequent messages for valid actors are still sent, and message ordering guarantees may be violated. Recommendation: Consider whether dropped messages should be re-buffered or if actors are guaranteed to exist when resending. Document this behavior or add re-buffering logic. 4. Race Condition Possibility - LOW PRIORITYLocation: tunnel.ts:89-93 The pattern of copying the buffer and clearing it is good. If sendMessage recursively buffers (which it does on line 493), those messages go into the new empty array and will be sent on next reconnect. This is actually correct behavior, but could use a comment explaining it. Minor Issues5. Inconsistent Access Modifier - STYLELocation: mod.ts:198 Changed from pegboardWebSocket (private) to __pegboardWebSocket (public but name-mangled). While this is necessary for the type predicate to work across classes, it is inconsistent with TypeScript privacy conventions. Consideration: This is acceptable given the type predicate requirement, but document why it is public in a comment. 6. Missing Test Coverage - TESTINGNo tests appear to cover the new buffering logic. Consider adding tests for: messages buffered when disconnected, messages sent on reconnection, buffer behavior during rapid connect/disconnect cycles, and buffer overflow scenarios. Security ConcernsNo significant security issues identified. The buffering mechanism does not introduce new attack vectors. Performance Considerations
Recommendations Summary
Approval StatusThe core implementation is solid and follows good patterns. However, I recommend addressing the unbounded buffer growth issue before merging, as it could cause production issues under poor network conditions. The other issues can be addressed in follow-up PRs if needed. Suggested action: Add a maximum buffer size limit and explicitly clear the buffer on shutdown, then this PR is ready to merge. |
Merge activity
|
graphite-app
bot
deleted the
11-20-chore_rivetkit_buffer_outbound_tunnel_messages
branch
No description provided.