feat(gateway): websocket hibernation #3400
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
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: |
Code Review: WebSocket Hibernation FeatureSummaryThis PR implements WebSocket hibernation functionality for the pegboard gateway, allowing WebSocket connections to sleep when actors are unavailable and wake up when they become ready. Overall, the implementation is well-structured with good separation of concerns. ✅ Strengths
🐛 Potential Bugs & IssuesCritical Issues1. Missing Peek Consumption After Non-Message Events (pegboard-gateway/src/lib.rs:586-611) When Ping, Pong, or Frame messages are encountered in the hibernate_ws function, they are peeked but never consumed from the stream. This creates an infinite loop - the same message will be peeked repeatedly. The code currently has: match msg {
Message::Binary(_) | Message::Text(_) => return Ok(HibernationResult::Message),
Message::Close(_) => return Ok(HibernationResult::Close),
// Ignore rest
_ => {} // BUG: Peeked message not consumed\!
}Fix: Call pinned.try_next().await? before continuing the loop for ignored message types. 2. Unreachable Code Path (pegboard-gateway/src/lib.rs:595-596) The bail!("should be unreachable") will never execute since try_next().await? already propagates the error. Either remove the bail! or replace it with unreachable!(). Moderate Issues3. Race Condition in Hibernation (pegboard-gateway/src/lib.rs:561-575) The tokio::select! will cancel the losing branch. If a message arrives at the exact moment the actor becomes ready, there's potential for edge cases. This may be acceptable behavior, but worth documenting. 4. TypeScript Typo Fixed (rivetkit-typescript/packages/rivetkit/src/actor/config.ts:74) Good catch fixing canHibernatWebSocket → canHibernateWebSocket. Verify that documentation and examples using this API are also updated if needed. ⚡ Performance ConsiderationsLock Contention: The receiver is wrapped in Arc<Mutex<>>, and during hibernation, the mutex will be held for potentially long periods while waiting for messages. This is likely acceptable since WebSocket handling is typically single-threaded per connection. Busy Loop Potential: Due to bug #1, ignored message types will cause a tight busy loop. Once fixed, the loop will properly block on peek(). 🔒 Security Concerns1. Close Frame Handling During Hibernation When a client sends a Close frame during hibernation, the connection terminates gracefully. Ensure that:
2. Timeout Handling I don't see an explicit timeout for the hibernation period. Consider:
🧪 Test CoverageMissing Test Coverage (concerning):
Recommendations:
📝 Code Quality & Best PracticesImport Style (guard/src/routing/api_public.rs:4) The code uses Consider changing to use anyhow::*; for consistency. Formatting & Logging: Good adherence to project standards with hard tabs and structured logging. 🎯 Summary of Action ItemsMust Fix (Blocking):
Should Fix (High Priority):
Nice to Have:
Overall AssessmentThis is a well-architected feature with good separation of concerns. The main concerns are:
Once these issues are addressed, this will be a solid addition to the codebase. Great work on improving the semantics and structure of the WebSocket retry/hibernation logic! |
MasterPtato
force-pushed
the
11-05-feat_gateway_websocket_hibernation
branch
from
096085e to
e79c2c9
Compare
PR Review: WebSocket Hibernation FeatureSummaryThis PR implements WebSocket hibernation functionality, allowing WebSocket connections to pause when the downstream actor is not ready and resume when it becomes available. This is a significant architectural improvement for handling actor lifecycle management. ✅ Strengths1. Clean Architecture
2. Smart Peekable ImplementationThe use of pub type WebSocketReceiver =
Peekable<futures_util::stream::SplitStream<WebSocketStream<TokioIo<Upgraded>>>>;This allows peeking at messages during hibernation without consuming them, which is critical for the hibernation behavior. 3. Proper Error HandlingThe
4. TypeScript IntegrationThe changes to 🐛 Potential Issues1. Logic Bug in
|
MasterPtato
force-pushed
the
11-05-feat_gateway_websocket_hibernation
branch
from
e79c2c9 to
6335bf0
Compare
WebSocket Hibernation PR ReviewThis PR implements WebSocket hibernation, allowing actors to sleep while maintaining WebSocket connections by pausing message processing until the actor becomes ready again. Overall, the implementation is well-structured and follows good practices. Here are my findings: ✅ Strengths
🐛 Potential Issues
🔍 Code Style & ConventionsGood adherence to repository conventions:
Minor observations:
🎯 Performance Considerations
🔒 Security ConsiderationsNo security concerns identified. The hibernation mechanism properly:
✨ Suggestions for Future Enhancements
📊 Test Coverage AssessmentThe PR modifies critical WebSocket handling logic but doesn't add new tests. Consider adding:
SummaryThis is a solid implementation of an important feature. The code is clean, well-structured, and follows repository conventions. The main issues are minor (grammar, unreachable code comment) and easily addressed. Recommendation: Approve with minor suggested fixes. Great work! 🚀 |
MasterPtato
force-pushed
the
11-05-feat_gateway_websocket_hibernation
branch
from
6335bf0 to
3a226de
Compare
MasterPtato
force-pushed
the
11-05-feat_gateway_websocket_hibernation
branch
from
3a226de to
a63afd8
Compare
PR Review: WebSocket Hibernation ImplementationThank you for implementing WebSocket hibernation! This is a well-structured feature that enables actors to sleep while maintaining WebSocket connections. Here's my detailed review: ✅ Strengths
🔍 Issues & ConcernsCritical: anyhow Glob Imports Still PresentWhile CLAUDE.md was updated to prohibit glob imports from anyhow, several files still use // Found in these files:
engine/packages/guard-core/src/websocket_handle.rs:1
engine/packages/guard-core/src/custom_serve.rs:1
engine/packages/guard-core/src/server.rs:10Recommendation: Update these to explicit imports like Potential Race Condition in hibernate_wsIn _ => {
pinned.try_next().await?; // Line 607
}Issue: If the stream is Recommendation: Consider whether the logic should:
Missing Test CoverageThe test in async fn handle_websocket_hibernation(&self, _websocket: WebSocketHandle) -> Result<bool> {
todo!();
}Issue: This will panic if the test path exercises hibernation. Recommendation: Either:
TypeScript TODO CommentIn // TODO: When WS hibernation is ready, update this to only count non-hibernatable websocketsRecommendation: Since hibernation is now implemented (line 2072 checks 🎯 Minor Suggestions
🔒 Security & Performance
📊 Overall AssessmentThis is a solid implementation of a complex feature. The core logic is sound and the integration across Rust and TypeScript layers is well-executed. The main concerns are:
The feature appears production-ready once the anyhow imports are cleaned up and test coverage is verified. Recommendation: Approve with minor changes requested (fix anyhow imports and test stub). |
MasterPtato
force-pushed
the
11-05-feat_gateway_websocket_hibernation
branch
from
a63afd8 to
b8bfd5a
Compare
Code Review: WebSocket Hibernation FeatureThis PR implements WebSocket hibernation to allow actors to sleep while maintaining client connections. Overall, the implementation is solid with good attention to detail. Here are my findings: ✅ Strengths
🐛 Potential Issues1. Missing Mutex Lock Semantics (High Priority)In async fn hibernate_ws(ws_rx: Arc<Mutex<WebSocketReceiver>>) -> Result<HibernationResult> {
let mut guard = ws_rx.lock().await;
let mut pinned = std::pin::Pin::new(&mut *guard);
loop {
if let Some(msg) = pinned.as_mut().peek().await {
// ...Issue: The mutex lock is held for the entire duration of hibernation (potentially indefinitely). If Fix: Consider releasing the lock between peek operations or documenting that concurrent calls are not supported. 2. Incomplete Test Coverage (High Priority)The test file async fn handle_websocket_hibernation(&self, _websocket: WebSocketHandle) -> Result<bool> {
todo!();
}Issue: The test implementation uses Recommendation: Either implement a proper test or mark the test as 3. Race Condition Potential (Medium Priority)In let close = tokio::select! {
_ = ready_sub.next() => { false }
hibernation_res = hibernate_ws(client_ws.recv()) => { /* ... */ }
};Issue: If the actor becomes ready while a message is being peeked, there's a small window where the message could be lost or processed out of order. Recommendation: Consider whether message ordering guarantees are maintained when waking from hibernation. 4. Attempt Counter Reset Logic (Low Priority)In if ws_hibernate {
attempts = 0;
} else {
attempts += 1;
}Observation: Hibernation resets the attempt counter. This is probably correct, but ensure this doesn't allow infinite retries if hibernation repeatedly fails. 🔍 Code Quality
🔒 Security Considerations
⚡ Performance Considerations
📝 Minor Suggestions
🧪 Test CoverageMissing:
Recommendation: Add at least one integration test demonstrating the hibernation flow works end-to-end. SummaryThis is a well-implemented feature that follows the codebase conventions. The main concerns are:
The code is production-ready after addressing the high-priority issues above. |
| export const chatRoom = actor({ | ||
| options: { | ||
| sleepTimeout: 2000, | ||
| canHibernateWebSocket: true, |
There was a problem hiding this comment.
not needed – this is for onWebSocket
There was a problem hiding this comment.
i removed this line but i still dont know what you mean
|
|
||
| false | ||
| } | ||
| hibernation_res = hibernate_ws(client_ws.recv()) => { |
There was a problem hiding this comment.
won't this consume the message? or does this get written to the buffer + replayed? either way, needs a comment on what happens to the message.
There was a problem hiding this comment.
none of the logic related to websocket messages is present here, and in the hibernate_ws fn it uses .peek() which implies its not consuming the message
|
|
||
| if attempts > max_attempts | ||
| || (!is_retryable_ws_error(&err) && !ws_retry) | ||
| if ws_hibernate { |
There was a problem hiding this comment.
how do we handle failure cases if we fail to re-wake the actor when the client disconnects?
There was a problem hiding this comment.
We don't do anything if the client disconnects. When we add waking and if it fails to wake the only thing we can do is log the error
MasterPtato
force-pushed
the
11-05-feat_gateway_websocket_hibernation
branch
from
b8bfd5a to
8cef849
Compare
PR Review: WebSocket Hibernation FeatureThis PR implements WebSocket hibernation support, allowing actors to sleep when there are hibernatable WebSocket connections. Overall, the implementation is well-structured and follows good patterns. Here's my detailed feedback: ✅ Strengths
🔍 Issues & ConcernsCritical Issues
Medium Priority Issues
Minor Issues
🎯 Suggested Improvements
📊 Performance Considerations
🔒 Security Considerations
📝 Documentation UpdatesThe PR includes extensive documentation updates in ✅ Test Coverage AssessmentCurrent Coverage: Basic happy path is tested
Recommendation: Add at least 2-3 more test cases covering error scenarios. 🎉 ConclusionThis is a well-implemented feature that follows the codebase conventions. The main concerns are:
Recommendation: Address the critical issues and consider the suggested improvements before merging. The core implementation is solid. Generated with Claude Code |
MasterPtato
force-pushed
the
11-05-feat_gateway_websocket_hibernation
branch
from
8cef849 to
7239ed4
Compare
Code Review: WebSocket Hibernation FeatureThis PR implements WebSocket hibernation, allowing actors to sleep while maintaining WebSocket connections. Overall, this is a well-implemented feature with good test coverage. Here's my detailed feedback: Strengths ✅
Issues & Concerns 🔍1. Potential Race Condition in
|
No description provided.