feat(rivetkit): integrate websocket hibernation #3301
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: WebSocket Hibernation IntegrationI've reviewed this PR and found the implementation to be well-structured overall. Here are my findings organized by category: ✅ Strengths
🐛 Critical Issues1. Race Condition in
|
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
NathanFlurry
force-pushed
the
10-31-feat_rivetkit_integrate_websocket_hibernation
branch
from
d6e9414 to
60414c4
Compare
NathanFlurry
changed the base branch from
graphite-base/3301
to
10-31-fix_use_correct_vbare_functions_for_runner_tunnel
PR Review: WebSocket Hibernation IntegrationSummaryThis PR implements WebSocket hibernation support in RivetKit, allowing WebSocket connections to persist across actor sleep/wake cycles. This is a significant enhancement for long-lived WebSocket connections and reduces resource consumption. Code Quality & Best Practices✅ Strengths
🔍 Areas for Improvement1. Missing Index Validation (Bug Risk)// rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1604-1615
const wsIndex = this.#persist.hibernatableWebSocket.findIndex((ws) =>
arrayBuffersEqual(ws.requestId, rivetRequestIdLocal),
);
const removed = this.#persist.hibernatableWebSocket.splice(wsIndex, 1);Issue: If Recommendation: if (wsIndex !== -1) {
const removed = this.#persist.hibernatableWebSocket.splice(wsIndex, 1);
// ... log removed
} else {
this.#rLog.warn({ msg: "could not find hibernatable websocket to remove" });
}2. Race Condition in WebSocket Event Tracking// rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1551-1578
const onSocketOpened = (event: any) => {
rivetRequestId = event?.rivetRequestId;
persistedHibernatableWebSocket = this.#persist.hibernatableWebSocket.find(...)
}Issue: The WebSocket is added to Recommendation: Consider synchronization or ensure the hibernatable WS is added before any events can fire. 3. ArrayBuffer Comparison Performance// rivetkit-typescript/packages/rivetkit/src/utils.ts:252-265
export function arrayBuffersEqual(buf1: ArrayBuffer, buf2: ArrayBuffer): boolean {
if (buf1.byteLength !== buf2.byteLength) return false;
const view1 = new Uint8Array(buf1);
const view2 = new Uint8Array(buf2);
for (let i = 0; i < view1.length; i++) {
if (view1[i] !== view2[i]) return false;
}
return true;
}Issue: O(n) comparison called in multiple Recommendation: Consider using a Map keyed by base64/hex string representation of requestId for O(1) lookup. 4. Sleep Logic Inconsistency// rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1904-1905
if (conn.status === "connected" && !conn.isHibernatable)
return false;Issue: Active raw WebSockets prevent sleep (line 1912), even if they're hibernatable. This seems inconsistent. Question: Should hibernatable raw WebSockets also be exempted from the raw WebSocket check at line 1912? 5. Error Handling in canHibernatWebSocket Callback// rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:223-238
try {
const canHibernate = canHibernatWebSocket(truncatedRequest);
hibernationConfig = { enabled: canHibernate, lastMsgIndex: undefined };
} catch (error) {
logger().error({ msg: "error calling canHibernatWebSocket", error });
hibernationConfig = { enabled: false, lastMsgIndex: undefined };
}Good: Catches errors but could be more informative about which actor/path failed. Security Concerns
|
Pull Request Review: WebSocket Hibernation IntegrationI've completed a thorough review of this PR. Overall, the implementation is well-structured and follows the codebase conventions. Below are my findings: ✅ Strengths
🔍 Issues & Concerns1. Critical: Potential Memory Leak in Hibernation Config (HIGH PRIORITY)Location: // Save hibernatable WebSocket
handler.actor[PERSIST_SYMBOL].hibernatableWebSocket.push({
requestId,
lastSeenTimestamp: BigInt(Date.now()),
msgIndex: -1n,
});Problem: This
Fix: Only push if if (!existingWs) {
handler.actor[PERSIST_SYMBOL].hibernatableWebSocket.push({
requestId,
lastSeenTimestamp: BigInt(Date.now()),
msgIndex: -1n,
});
}2. Race Condition: WebSocket Cleanup (MEDIUM PRIORITY)Location: The
Suggestion: Add explicit check: if (rivetRequestId) {
const wsIndex = this.#persist.hibernatableWebSocket.findIndex((ws) =>
arrayBuffersEqual(ws.requestId, rivetRequestId)
);
if (wsIndex !== -1) {
this.#persist.hibernatableWebSocket.splice(wsIndex, 1);
this.#rLog.debug({ msg: "removed hibernatable websocket", ... });
}
} else {
this.#rLog.debug({ msg: "socket closed without rivetRequestId" });
}3. Timing Issue: Event Listener Registration (LOW PRIORITY)Location: The event listeners are registered after the websocket might already be open. In the current flow:
Potential issue: If the underlying WebSocket implementation fires Suggestion: Consider registering listeners before any WebSocket operations, or document this assumption clearly. 4. BigInt Precision Loss (LOW-MEDIUM PRIORITY)Location: lastMsgIndex: Number(existingWs.msgIndex),Converting Suggestion: Either:
5. Inconsistent Null HandlingLocation: const rivetRequestId = event?.rivetRequestId;Uses optional chaining, but later code in Suggestion: Make the undefined case more explicit earlier: if (!rivetRequestId) {
this.#rLog.debug({ msg: "websocket opened without rivetRequestId" });
return;
}🎯 Performance Considerations
🔒 Security ConsiderationsNo major security concerns identified. The implementation:
📝 Code Style & Conventions✅ Follows CLAUDE.md conventions:
🧪 Test CoverageQuestion: Are there tests for:
These would be valuable to prevent regressions. 📋 SummaryMust Fix Before Merge:
Should Fix Before Merge: Nice to Have: Overall, this is solid work! The main concern is the memory leak in Issue #1, which should be addressed before merging. The rest are lower priority but would improve robustness. Let me know if you'd like me to clarify any of these points or if you'd like help implementing the fixes! |
NathanFlurry
force-pushed
the
10-31-feat_rivetkit_integrate_websocket_hibernation
branch
from
60414c4 to
9854433
Compare
NathanFlurry
force-pushed
the
10-31-fix_use_correct_vbare_functions_for_runner_tunnel
branch
from
96f8d1f to
fc88080
Compare
PR Review: WebSocket Hibernation IntegrationSummaryThis PR implements WebSocket hibernation support for RivetKit actors, allowing WebSocket connections to persist across actor sleep/wake cycles. The implementation adds tracking for hibernatable WebSockets and integrates this with the actor persistence layer. Code Quality & ArchitectureStrengths
Issues & Concerns🔴 Critical: Missing Index ValidationLocation: const wsIndex = this.#persist.hibernatableWebSocket.findIndex((ws) =>
arrayBuffersEqual(ws.requestId, rivetRequestIdLocal),
);
const removed = this.#persist.hibernatableWebSocket.splice(wsIndex, 1);Problem: Fix: Add validation: if (wsIndex !== -1) {
const removed = this.#persist.hibernatableWebSocket.splice(wsIndex, 1);
// ... logging
} else {
this.#rLog.warn({
msg: "could not find hibernatable websocket to remove",
rivetRequestId,
});
}🟡 Potential Bug: Race Condition in Event HandlersLocation: The Concern: Depending on timing, the WebSocket might not be in the array yet when Recommendation: Add a comment explaining the expected timing or consider refactoring to make the flow more explicit. 🟡 Type Safety: Missing Null CheckLocation: persistedHibernatableWebSocket.msgIndex = BigInt(event.rivetMessageIndex);Issue: Fix: Add validation: if (event.rivetMessageIndex !== undefined) {
persistedHibernatableWebSocket.msgIndex = BigInt(event.rivetMessageIndex);
}🟡 Code Organization: Extra Blank LinesLocation: adapter._handleOpen(requestId);
// Call websocket handlerTwo blank lines are unusual for this codebase. Consider removing one. 🔵 Typo in Config Property NameLocation: canHibernatWebSocketShould be 🔵 Performance: O(n) Array ComparisonsLocation: Multiple locations using The implementation uses linear search through Current: O(n*m) where n = number of WebSockets, m = bytes in requestId This is likely premature optimization for most use cases, but worth considering if hibernatable WebSocket counts grow large. SecurityArrayBuffer Equality CheckThe State PersistenceThe persistence layer correctly tracks WebSocket state, but there's no validation that Performance
Test Coverage🔴 Major Gap: No tests found for the hibernation feature. The grep search found no test files covering:
Recommendation: Add tests covering:
Best Practices✅ Follows CLAUDE.md Guidelines
✅ Schema VersioningThe migration from v1 to v2 is clean and follows the existing versioned data pattern. ✅ Experimental FeaturesGood use of RecommendationsHigh Priority
Medium Priority
Low Priority
ConclusionThis is a solid implementation of an experimental feature. The main concern is the critical bug with Overall: Approve with required changes ✅ (after fixing the findIndex bug) |
PR Review: WebSocket Hibernation IntegrationI've reviewed this PR implementing WebSocket hibernation support. Overall, the implementation is solid and follows good practices, but I've identified several issues that should be addressed. Critical Issues1. Race Condition in WebSocket Tracking (rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:259-264)The // Save hibernatable WebSocket
handler.actor[PERSIST_SYMBOL].hibernatableWebSocket.push({
requestId,
lastSeenTimestamp: BigInt(Date.now()),
msgIndex: -1n,
});Problem: This is called before determining if hibernation is actually enabled. If Impact: Non-hibernatable WebSockets will accumulate in the Fix: Only add to 2. Duplicate WebSocket Entries on Reconnection (rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:179-192)When checking for existing WebSockets at lines 179-184: const existingWs = handler.actor[PERSIST_SYMBOL].hibernatableWebSocket.find((ws) =>
arrayBuffersEqual(ws.requestId, requestId),
);If an Impact: Each reconnection will create a duplicate entry, leading to unbounded array growth. Fix: Add early return after setting 3. msgIndex Not Updated on onSocketOpened (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1551-1578)In if (persistedHibernatableWebSocket) {
persistedHibernatableWebSocket.lastSeenTimestamp = BigInt(Date.now());
}Problem: The Impact: Message replay or loss on reconnection scenarios. Recommendation: Clarify the intended behavior. Should 4. Potential Index Out of Bounds (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1604-1615)The cleanup logic uses const wsIndex = this.#persist.hibernatableWebSocket.findIndex((ws) =>
arrayBuffersEqual(ws.requestId, rivetRequestIdLocal),
);
const removed = this.#persist.hibernatableWebSocket.splice(wsIndex, 1);Problem: If Impact: Wrong WebSocket entries get removed, corrupting state. Fix: Check High Priority Issues5. Sleep Logic Doesn't Consider Raw WebSockets (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1904-1912)The // Check for active conns. This will also cover active actions, since all actions have a connection.
for (const conn of this.#connections.values()) {
if (conn.status === "connected" && !conn.isHibernatable)
return false;
}
// Do not sleep if there are raw websockets open
if (this.#activeRawWebSockets.size > 0) return false;Problem: The second check ( Impact: Actors with hibernatable raw WebSockets will never sleep. Recommendation: Modify the logic to check if raw WebSockets are hibernatable before preventing sleep, similar to how connections are checked. 6. Missing rivetMessageIndex Validation (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1580-1598)In persistedHibernatableWebSocket.msgIndex = BigInt(event.rivetMessageIndex);Problem: No validation if Impact: Could set Fix: Add validation: Medium Priority Issues7. Log Message Typo (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1633)Line 1633 has incorrect log message: this.#rLog.debug({
msg: "actor instance onSocketMessage", // Should be "onSocketClosed"
rivetRequestId,
isHibernatable: !!persistedHibernatableWebSocket,
hibernatableWebSocketCount: this.#persist.hibernatableWebSocket.length,
});Fix: Change to 8. Type Safety Concern with PERSIST_SYMBOL (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:54-58)The new get [PERSIST_SYMBOL](): PersistedActor<S, CP, CS, I> {
return this.#persist;
}Concern: This breaks encapsulation by exposing mutable internal state. External code (like in Recommendation: Consider providing dedicated methods for managing hibernatable WebSockets instead of exposing raw state, or at least document that this is intentionally mutable. 9. Schema Migration TestingThe migration from v1 to v2 (rivetkit-typescript/packages/rivetkit/src/schemas/actor-persist/versioned.ts:23-29) looks correct: migrations.set(
1,
(v1Data: v1.PersistedActor): v2.PersistedActor => ({
...v1Data,
hibernatableWebSocket: [],
}),
);Recommendation: Ensure there are tests covering:
Minor Issues10. Inconsistent Error Handling (rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:231-240)The try-catch around } catch (error) {
logger().error({
msg: "error calling canHibernatWebSocket",
error,
});
hibernationConfig = {
enabled: false,
lastMsgIndex: undefined,
};
}Recommendation: Consider if this should throw or if there should be metrics/alerts for configuration errors. 11. TypeScript Strict Mode ConcernsMultiple uses of
Recommendation: Define proper types for these events to improve type safety, especially since you're accessing specific properties like Positive Observations✅ Good migration strategy: The v1→v2 schema migration is clean and backward compatible ✅ Proper logging: Good use of structured logging throughout for debugging ✅ Event-driven design: The event listener approach for tracking WebSocket lifecycle is clean ✅ Configuration flexibility: The ✅ Following conventions: Adheres to the codebase's error handling and logging patterns Testing RecommendationsConsider adding tests for:
SummaryThe feature implementation is well-structured, but there are critical bugs that need to be fixed before merging:
Overall direction is good - please address the critical issues and consider the recommendations for a more robust implementation. |
NathanFlurry
force-pushed
the
10-31-fix_use_correct_vbare_functions_for_runner_tunnel
branch
from
fc88080 to
479b46d
Compare
NathanFlurry
force-pushed
the
10-31-feat_rivetkit_integrate_websocket_hibernation
branch
from
9854433 to
554ce2a
Compare
PR Review: WebSocket Hibernation IntegrationI've reviewed PR #3301 which adds WebSocket hibernation support to RivetKit. This is a significant feature that enables stateful WebSocket connections to persist across actor sleep/wake cycles. Here's my detailed feedback: ✅ Strengths1. Well-Structured Schema Migration
2. Comprehensive State Tracking
3. Good Logging & Observability
🔍 Issues & ConcernsCritical: Potential Memory Leak in
|
PR Review: WebSocket Hibernation IntegrationThis PR introduces WebSocket hibernation support to RivetKit, allowing WebSocket connections to persist across actor sleep/wake cycles. Overall, the implementation is well-structured with proper schema versioning and migration. Here's my detailed feedback: ✅ Strengths
🐛 Potential IssuesCritical
Moderate
Minor
🔒 Security Considerations
⚡ Performance Considerations
📝 Test CoverageI don't see test file changes in this PR. Consider adding tests for:
📚 DocumentationThe feature is marked experimental which is good. Consider adding:
✏️ Nits
SummaryThis is a solid implementation of a complex feature. The main concerns are:
The schema versioning and migration approach is exemplary - great work on that front! |
NathanFlurry
force-pushed
the
10-31-fix_use_correct_vbare_functions_for_runner_tunnel
branch
from
479b46d to
d6216a4
Compare
Pull Request Review: WebSocket Hibernation IntegrationOverviewThis PR adds experimental WebSocket hibernation support to RivetKit, allowing actors to persist WebSocket connections across sleep/wake cycles. The implementation includes schema changes, connection state tracking, and integration with the engine runner. Code Quality & Best Practices✅ Strengths
|
Merge activity
|
NathanFlurry
force-pushed
the
10-31-feat_rivetkit_integrate_websocket_hibernation
branch
from
dcaa1af to
e0a1d01
Compare
NathanFlurry
force-pushed
the
10-31-fix_use_correct_vbare_functions_for_runner_tunnel
branch
from
7327333 to
c0672bb
Compare
PR Review: WebSocket Hibernation IntegrationThis PR implements WebSocket hibernation functionality for RivetKit, allowing WebSocket connections to be persisted and resumed across actor sleep/wake cycles. Overall, the implementation is well-structured, but there are several issues that should be addressed. Critical Issues1. Race Condition in Hibernation Config (rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:259-266)The // Save hibernatable WebSocket
handler.actor[PERSIST_SYMBOL].hibernatableWebSocket.push({
requestId,
lastSeenTimestamp: BigInt(Date.now()),
msgIndex: -1n,
});Problems:
Recommendation: Only add to the array when 2. Missing Index Bounds Check (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1602-1630)In const wsIndex = this.#persist.hibernatableWebSocket.findIndex((ws) =>
arrayBuffersEqual(ws.requestId, rivetRequestIdLocal),
);
const removed = this.#persist.hibernatableWebSocket.splice(wsIndex, 1);Problem: If Fix: if (wsIndex !== -1) {
const removed = this.#persist.hibernatableWebSocket.splice(wsIndex, 1);
// ... logging
}3. Incorrect Log Message (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1633)this.#rLog.debug({
msg: "actor instance onSocketMessage", // ← Wrong message
rivetRequestId,
isHibernatable: !!persistedHibernatableWebSocket,
hibernatableWebSocketCount: this.#persist.hibernatableWebSocket.length,
});This should be High Priority Issues4. Disabled Hibernation Sleep Check (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1904-1908)The commented-out code suggests hibernatable WebSockets should prevent actor sleep, but it's currently disabled: // TODO: Enable this when hibernation is implemented. We're waiting on support for Guard to not auto-wake the actor if it sleeps.
// if (conn.status === "connected" && !conn.isHibernatable)
// return false;
if (conn.status === "connected") return false;Concern: This means actors cannot sleep even when all WebSockets are hibernatable, defeating the purpose of hibernation. The TODO should be tracked with a GitHub issue and timeline. 5. Event Listener Cleanup Best Practices (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1640-1644)The cleanup code swallows all errors: try {
websocket.removeEventListener("open", onSocketOpened);
websocket.removeEventListener("message", onSocketMessage);
websocket.removeEventListener("close", onSocketClosed);
websocket.removeEventListener("error", onSocketClosed);
} catch {}Recommendation: Log errors during cleanup for debugging purposes rather than silently swallowing them. Medium Priority Issues6. Missing Migration PathThe new BARE schema version Questions:
7. Type Safety: BigInt Conversion (rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:191)lastMsgIndex: Number(existingWs.msgIndex),Converting 8. Experimental API SurfaceThe PR adds several
Recommendation: Add clear documentation about:
Code Quality Issues9. Inconsistent Null HandlingThe code uses optional chaining ( 10. Performance: Linear Search on Every MessageIn persistedHibernatableWebSocket = this.#persist.hibernatableWebSocket.find((ws) =>
arrayBuffersEqual(ws.requestId, rivetRequestIdLocal),
);For actors with many hibernatable WebSockets, this could impact performance. Consider using a Positive Observations✅ Good: The BARE schema is well-documented with clear field descriptions Testing Recommendations
Security ConsiderationsNo critical security issues identified, but consider:
SummaryThe PR implements a valuable feature but has several bugs that should be fixed before merging: Must Fix:
Should Fix:
Nice to Have:
Overall assessment: Needs revision before merge. |
WebSocket Hibernation Feature ReviewThis PR introduces WebSocket hibernation support, allowing actors to persist WebSocket connections across sleep/wake cycles. Overall, the implementation is well-structured and follows the codebase conventions. Here are my findings: Strengths ✅
Critical Issues 🔴1. Potential Memory Leak in
|
graphite-app
bot
deleted the
10-31-feat_rivetkit_integrate_websocket_hibernation
branch
No description provided.