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
|
NathanFlurry
force-pushed
the
11-13-chore_rivetkit_implement_new_hibernating_ws_protocol
branch
from
7fadc23 to
afc0cf5
Compare
Pull Request Review: Hibernating WebSocket Protocol ImplementationI've reviewed this PR implementing the new hibernating WebSocket protocol. This is a substantial refactoring with 2397 additions and 2149 deletions across 25 files. Overall, the implementation is well-structured with good error handling and comprehensive test coverage for the wrapping arithmetic utilities. Strengths1. Excellent Test Coverage for Utilities
2. Good Documentation
3. Robust Error Handling
4. Clean Architecture
Issues & ConcernsCritical Issues1. Incomplete Implementation - Will Break at Runtime The throw "TODO";
// TODO:
// const connPersist: PersistedConn<CP, CS> = {
// id: crypto.randomUUID(),
// parameters: params,
// state: connState as CS,
// ...This is a blocker - the code cannot function without this implementation. The commented-out code needs to be completed or the feature isn't usable. 2. Missing Error Handler Reference removePersisted: this.#hwsRemovePersisted.bind(this),This method is referenced but doesn't appear to be implemented in the diff. This will cause a runtime error when the runner tries to call it. 3. Unsafe String Throwing throw "TODO";Per JavaScript/TypeScript best practices, always throw Error objects, not strings: throw new Error("TODO: Implement hibernatable connection persistence");Design Concerns4. Async Consistency Issue The comment says "NOTE: This method is safe to be async" but the websocket handler signature changed from async to sync: // Before (async):
websocket?: (runner, actorId, ws, requestId, request) => Promise<void>;
// After (sync):
websocket: (runner, actorId, ws, requestId, request) => void;While the comment in
5. Missing Null Safety
6. Race Condition Risk When shutting down, the code only closes non-hibernatable websockets: if (!ws[HIBERNATABLE_SYMBOL]) {
ws._closeWithoutCallback(1000, "ws.tunnel_shutdown");
}But what happens to hibernatable WebSockets during shutdown? They're left in the map but the tunnel is destroyed. Consider explicitly documenting the shutdown semantics for hibernatable connections. Code Quality Issues7. Inconsistent Commenting Style // mARK: - Internal StateTypo: should be 8. Magic Numbers const GC_INTERVAL = 60000; // 60 seconds
const MESSAGE_ACK_TIMEOUT = 5000; // 5 secondsConsider if these should be configurable or at least documented why these specific values were chosen. What happens if a persist takes longer than 5 seconds? 9. Verbose Nested Ternaries The get ephemeralData(): EphemeralConn<CP, CS> {
if ("hibernatable" in this.#data) {
return this.#data.hibernatable.data;
} else if ("ephemeral" in this.#data) {
return this.#data.ephemeral.data;
} else {
return assertUnreachable(this.#data);
}
}Since get ephemeralData(): EphemeralConn<CP, CS> {
return "hibernatable" in this.#data
? this.#data.hibernatable.data
: this.#data.ephemeral.data;
}10. Unused Import import assert from "node:assert"This import is added but I don't see it being used in the file. If the custom assert was removed, verify this import is actually needed. Performance Concerns11. Linear Search in Hot Path findHibernatableConn(requestIdBuf: ArrayBuffer): Conn<...> | undefined {
return Array.from(this.#connections.values()).find((conn) => {
const connStateManager = conn[CONN_STATE_MANAGER_SYMBOL];
const connRequestId = connStateManager.hibernatableDataRaw?.hibernatableRequestId;
return connRequestId && arrayBuffersEqual(connRequestId, requestIdBuf);
});
}This performs a linear O(n) search through all connections on every hibernatable WebSocket restoration. Consider maintaining a separate 12. Potential Memory Leak #hibernatableWebSocketAckQueue = new Map<
string,
{ actorId: string; requestId: RequestId; messageIndex: number }
>();This queue accumulates ack requests. Is there cleanup logic? What happens if persist fails or is delayed indefinitely? Consider adding:
Documentation Gaps13. Incomplete Sequence Diagrams Several sections are marked as
These should be completed before merging or removed if not applicable. 14. Missing Error Recovery Documentation
Testing Concerns15. No Integration Tests
16. No Error Path Tests
Security Considerations17. Request Validation The
18. Message Index Manipulation
Ensure the gateway validates indices before sending to prevent this attack vector. RecommendationsMust Fix Before Merge
Should Fix Before Merge
Nice to Have
SummaryThis is a solid implementation of a complex feature, but it has critical blockers that prevent it from being merged:
The architecture and design are sound, test coverage for utilities is excellent, and the documentation (while incomplete) shows good intent. Once the blockers are addressed and the recommended fixes applied, this will be a high-quality addition to the codebase. Recommendation: Request Changes - Address critical issues before merge. |
No description provided.