chore(rivetkit): implement new hibernating ws protocol #3464
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: Hibernating WebSocket Protocol ImplementationOverviewThis PR implements a new hibernating WebSocket protocol in RivetKit. The changes refactor WebSocket lifecycle management and message acknowledgment handling. Overall the code quality is good with clear separation of concerns, but there are several issues to address before merging. Critical Issues1. Incomplete WebSocket Adapter Constructor Call (tunnel.ts:139-193)The // tunnel.ts:148-186
const adapter = new WebSocketTunnelAdapter(
requestIdStr,
hibernatable,
request,
(data: ArrayBuffer | string, isBinary: boolean) => { /* ... */ },
(code?: number, reason?: string, retry: boolean = false) => { /* ... */ },
);But the constructor signature (websocket-tunnel-adapter.ts:49-73) requires: constructor(
tunnel: Tunnel, // MISSING
actorId: string, // MISSING
requestId: string, // PROVIDED
hibernatable: boolean, // PROVIDED
request: Request, // PROVIDED
messageIndex: number, // MISSING
persistHibernatableWebSocketMessageIndex: (messageIndex: number) => void, // MISSING
sendCallback: (data: ArrayBuffer | string, isBinary: boolean) => void, // PROVIDED
closeCallback: (code?: number, reason?: string, retry?: boolean) => void, // PROVIDED
)This will cause a runtime error. The parameters are misaligned - 2. Loop Variable Declaration Error (tunnel.ts:116)for (const i = 0; i < requestIds.length; i++) {Should be: for (let i = 0; i < requestIds.length; i++) {
3. Inconsistent Property Access (websocket-tunnel-adapter.ts:97)The adapter references 4. Missing Request Parameter (tunnel.ts:120-126)When calling this.#createWebSocket(
actorId,
requestId,
idToStr(requestId),
true,
messageIndex,
); // Missing 'request' parameterThe method signature requires a High Priority Issues5. Breaking API Change Without Migration Path (mod.ts:60-66)The // Before
websocket?: (runner, actorId, ws, requestId, request) => Promise<void>;
// After
websocket: (runner, actorId, ws, requestId, request) => void;This is a breaking change that will affect all existing consumers. Consider:
6. Unused Symbols Imported (tunnel.ts:12-16)import {
SET_CAN_HIBERNATE_SYMBOL,
SET_REQUEST_SYMBOL,
WebSocketTunnelAdapter,
} from "./websocket-tunnel-adapter";
7. Missing Validation (tunnel.ts:104-134)The
8. Incomplete TODO Comments (tunnel.ts:192, websocket-tunnel-adapter.ts:517)Several critical TODOs are left in the code:
These should either be implemented or documented with follow-up issue numbers. Medium Priority Issues9. Error Handling in Message Sequence Logic (websocket-tunnel-adapter.ts:103-157)The message sequencing logic is well-designed, but consider edge cases:
10. Race Condition Potential (tunnel.ts:662-674)There's a check for duplicate WebSockets, but in a concurrent scenario: const existingAdapter = this.#actorWebSockets.get(requestIdStr);
if (existingAdapter) {
existingAdapter.__closeWithoutCallback(1000, "ws.duplicate_open");
}If two open messages arrive simultaneously, both could pass the check before either is added to the map. Consider using a lock or atomic operation. 11. Memory Leak Risk (tunnel.ts:97)The check if (!ws.canHibernate) {
ws.__closeWithoutCallback(1000, "ws.tunnel_shutdown");
}But they're immediately cleared from the map on line 101. This means the WebSocket objects remain in memory without being properly cleaned up. Should hibernatable WebSockets be persisted to storage here? Minor Issues / Code Quality12. Logging Inconsistencies
13. Magic Numbers (websocket-tunnel-adapter.ts:154)this.close(1008, closeReason);Consider defining WebSocket close codes as named constants: const WS_CLOSE_POLICY_VIOLATION = 1008;14. Type Safety (tunnel.ts:145-146)Parameters #createWebSocket(options: {
actorId: string;
requestId: RequestId;
requestIdStr: string;
hibernatable: boolean;
messageIndex: number;
request: Request;
}): WebSocketTunnelAdapter15. Commented Out Code (actor-driver.ts:81-87)Large blocks of commented code should be removed: // // WebSocket message acknowledgment debouncing for hibernatable websockets
// #hibernatableWebSocketAckQueue: Map<...> = new Map();
// #wsAckFlushInterval?: NodeJS.Timeout;If this code might be needed, reference the commit SHA in a comment instead. 16. OpenAPI Changes Not Reviewed (rivetkit-openapi/openapi.json)The PR removes 277 lines from the OpenAPI spec. Ensure:
Performance Considerations17. UUID String Conversion (tunnel.ts:807-809)
18. Missing Index Validation Before Persistence (websocket-tunnel-adapter.ts:160-163)Before persisting the message index, validate it's in the acceptable range: if (messageIndex < 0 || messageIndex > 65535) {
throw new Error(`Invalid message index: ${messageIndex}`);
}
this.messageIndex = messageIndex;Testing Recommendations
Security Considerations19. Message Index ValidationThe message index sequence validation is good, but ensure:
Documentation20. Missing JSDocKey methods lack documentation:
Positive Aspects✅ Good separation of concerns between tunnel and adapter RecommendationsBefore merging:
After merging:
SummaryThe refactoring improves the WebSocket lifecycle management significantly, but critical bugs prevent this from being merged as-is. The constructor parameter mismatch (Issue #1) will cause immediate runtime failures. After addressing the critical issues, this will be a solid improvement to the codebase. |
NathanFlurry
force-pushed
the
11-11-fix_guard_dont_send_open_msg_for_hibernating_ws_hibernating_ws_keepalive
branch
from
f45c96f to
769c3f8
Compare
NathanFlurry
force-pushed
the
11-13-chore_rivetkit_implement_new_hibernating_ws_protocol
branch
from
dbe19ca to
426a752
Compare
Code Review: Hibernating WebSocket Protocol ImplementationThis PR implements a new hibernating WebSocket protocol that significantly refactors how WebSocket connections are managed, particularly around hibernation and message acknowledgment. Here's my comprehensive review: 🎯 Overall AssessmentStrengths:
Areas of Concern:
🐛 Critical Issues1. Compilation Error in tunnel.ts:116-126for (const i = 0; i < requestIds.length; i++) {This should be Location: 2. Missing Parameters in #createWebSocketThe // Line 120-126 - Missing 'request' parameter
this.#createWebSocket(
actorId,
requestId,
idToStr(requestId),
true,
messageIndex,
);Location: 3. API Signature Changes Without MigrationThe Old: Location: 4. Incomplete WebSocketTunnelAdapter ConstructorThe constructor signature in Location: 5. Deleted Assert Function Not ReplacedIn Location:
|
| Category | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐ | Good separation of concerns |
| Code Quality | ⭐⭐⭐ | Some dead code and TODOs |
| Type Safety | ⭐⭐ | Missing parameters, incomplete refactoring |
| Security | ⭐⭐⭐⭐ | Good validation of message indices |
| Performance | ⭐⭐⭐⭐ | Efficient message handling |
| Test Coverage | ⭐⭐ | Missing tests for critical paths |
Overall: Needs Work
This PR has good architectural improvements but requires fixing critical issues before merging:
- Compilation errors
- Missing required callback implementations
- Incomplete API refactoring
- Dead code cleanup
Once these issues are addressed, this will be a solid improvement to the WebSocket hibernation system.
PR Review: chore(rivetkit): implement new hibernating ws protocolOverall, this is a solid refactor that improves the hibernating WebSocket protocol implementation. The changes simplify the architecture and improve reliability. Below are my detailed findings: ✅ Strengths
🐛 Potential Bugs
|
NathanFlurry
force-pushed
the
11-11-fix_guard_dont_send_open_msg_for_hibernating_ws_hibernating_ws_keepalive
branch
from
769c3f8 to
fc8f1fc
Compare
NathanFlurry
force-pushed
the
11-13-chore_rivetkit_implement_new_hibernating_ws_protocol
branch
from
426a752 to
04be372
Compare
NathanFlurry
force-pushed
the
11-13-chore_rivetkit_implement_new_hibernating_ws_protocol
branch
from
04be372 to
9d6c4c8
Compare
NathanFlurry
force-pushed
the
11-11-fix_guard_dont_send_open_msg_for_hibernating_ws_hibernating_ws_keepalive
branch
from
fc8f1fc to
e5ca471
Compare
PR Review: Hibernating WebSocket Protocol ImplementationCritical Issues1. Import Error in
|
Code Review: Hibernating WebSocket Protocol ImplementationSummaryThis PR implements a new hibernating WebSocket protocol for RivetKit. The changes refactor how WebSocket connections are managed, adding support for hibernatable connections that can survive actor restarts by persisting and restoring connection state. Positive ObservationsCode Quality
Implementation Strengths
Issues & RecommendationsCritical Issues1. Bug: Missing import in tunnel.ts ❌Location: tunnel.ts:7 import { wrappingLte } from "./utils";Issue: The import references 2. Bug: Unused imports in tunnel.ts
|
NathanFlurry
force-pushed
the
11-13-chore_rivetkit_implement_new_hibernating_ws_protocol
branch
from
9d6c4c8 to
449ac28
Compare
NathanFlurry
force-pushed
the
11-11-fix_guard_dont_send_open_msg_for_hibernating_ws_hibernating_ws_keepalive
branch
from
e5ca471 to
5b5466a
Compare
PR Review: Implement New Hibernating WebSocket ProtocolI've reviewed the changes and found several critical bugs that will prevent compilation, along with some recommendations for improvement. Critical Bugs (Must Fix)1. Import Name Mismatch in websocket-tunnel-adapter.tsLocation: import { wrappingLte } from "./utils";Issue: The function is named Fix: Change the import to: import { wrappingLteU16 } from "./utils";And update the usage on line 124: if (wrappingLteU16(messageIndex, previousIndex)) {2. Missing Request Parameter in tunnel.tsLocation: for (const i = 0; i < requestIds.length; i++) {
const requestId = requestIds[i];
const messageIndex = messageIndices[i];
this.#createWebSocket(
actorId,
requestId,
idToStr(requestId),
true,
messageIndex,
); // Missing 6th parameter: request
}Issue: Fix: You need to either:
3. Commented Out Field Declarations Still Being UsedLocation: // // WebSocket message acknowledgment debouncing for hibernatable websockets
// #hibernatableWebSocketAckQueue: Map<
// string,
// { requestIdBuf: ArrayBuffer; messageIndex: number }
// > = new Map();
// #wsAckFlushInterval?: NodeJS.Timeout;Issue: These fields are commented out but are being referenced throughout the file (lines 316, 339, 344, 348, 649, 653, 770, 834-836). This will cause compilation errors. Fix: Uncomment these field declarations. 4. Invalid for Loop DeclarationLocation: for (const i = 0; i < requestIds.length; i++) {Issue: Using Fix: Change to: for (let i = 0; i < requestIds.length; i++) {5. Missing Implementation in actor-driver.tsLocation: Issue: The PR changes
But the implementation in
Fix: The actor-driver.ts needs to be updated to implement the new API instead of the old one. Code Quality Issues6. Removed Return StatementLocation: // TODO: assign request to ws so we can pass this throughIssue: The Fix: Add a return statement: // Store adapter
this.#actorWebSockets.set(requestIdStr, adapter);
return adapter;7. WebSocket Constructor Signature MismatchLocation: The old constructor signature appears to be: new WebSocketTunnelAdapter(
requestIdStr,
hibernatable,
request,
// ... callbacks
)But based on the new code in websocket-tunnel-adapter.ts (lines 50-74), the signature should be: new WebSocketTunnelAdapter(
tunnel,
actorId,
requestId,
hibernatable,
request,
messageIndex,
persistHibernatableWebSocketMessageIndex,
sendCallback,
closeCallback,
)This needs to be verified and fixed. Design Concerns8. Message Index Persistence StrategyLocation: The 9. Async to Sync Change Without Clear JustificationLocation: websocket: (
runner: Runner,
actorId: string,
ws: any,
requestId: protocol.RequestId,
request: Request,
) => void; // Changed from Promise<void> to voidConcern: This breaking change from async to sync might cause issues if implementations need to perform async operations during WebSocket setup. Consider whether this is intentional and document the reasoning. Positive Aspects
Recommendations
SummaryThis PR implements an important feature but has several compilation-blocking bugs that need to be fixed before it can be merged. The core logic appears sound, but the implementation is incomplete in several areas. Recommendation: Address the 5 critical bugs before merging. The code quality issues should also be fixed but are less critical. |
PR Review: chore(rivetkit): implement new hibernating ws protocolSummaryThis PR refactors the WebSocket tunnel implementation to support a new hibernating WebSocket (HWS) protocol. The changes affect the TypeScript runner SDK and RivetKit engine driver, implementing message sequencing, hibernation support, and improved lifecycle management. Code Quality & Best Practices✅ Strengths
|
NathanFlurry
changed the base branch from
11-11-fix_guard_dont_send_open_msg_for_hibernating_ws_hibernating_ws_keepalive
to
graphite-base/3464
NathanFlurry
force-pushed
the
11-13-chore_rivetkit_implement_new_hibernating_ws_protocol
branch
from
449ac28 to
7fadc23
Compare
NathanFlurry
force-pushed
the
graphite-base/3464
branch
from
5b5466a to
450d9e5
Compare
Pull Request Review: Hibernating WebSocket Protocol ImplementationOverviewThis PR implements a new hibernating WebSocket protocol for RivetKit, focusing on improving WebSocket state management and message sequencing for actors that can hibernate and wake up. Code Quality & Best PracticesStrengths ✅
Issues & Concerns
|
Pull Request Review: Hibernating WebSocket Protocol ImplementationSummaryThis PR implements a new hibernating WebSocket protocol for RivetKit. The changes refactor the WebSocket handling to better support WebSocket hibernation with message index tracking, improved event handling, and proper sequence validation. Critical Issues1. Import Error in
|
No description provided.