feat: execute bounded WebSocket sessions - #23
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds public WebSocket execution and cancellation APIs. It implements bounded frame and byte accounting, ordered actions, message expectations, close handling, deadline causes, cancellation, and persisted terminal observations. Schema validation stops after 64 failures. ChangesWebSocket execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Plan
participant execute_websocket
participant WebSocketConnection
participant Peer
participant ObservationStore
Plan->>execute_websocket: execute plan with options
execute_websocket->>WebSocketConnection: connect with cancellation
WebSocketConnection->>Peer: perform WebSocket upgrade
execute_websocket->>WebSocketConnection: execute ordered actions
Peer-->>WebSocketConnection: return frames and close events
WebSocketConnection->>ObservationStore: persist terminal observation
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/kahea-exec/src/websocket.rs (1)
2029-2032: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winApply the cancellation retry loop to
flush.
remaining()caps the returned duration at 25 ms whenevercancellationis set.readandwriteabsorb that cap with a retry loop and re-check the real deadline.flushdoes not. A cancellable session whose socket buffer stalls for more than 25 ms therefore fails withio::ErrorKind::TimedOutbefore the action, idle, or total deadline elapses.socket_errorthen reports a timeout cause for a session that had budget left.
flushruns after ping and close handling at lines 810, 855, and 918, so cancellable sessions reach this path.🐛 Proposed fix to retry `flush` until the real deadline elapses
fn flush(&mut self) -> io::Result<()> { - self.stream.set_write_timeout(Some(self.remaining()?))?; - self.stream.flush().map_err(normalize_timeout) + loop { + self.stream.set_write_timeout(Some(self.remaining()?))?; + match self.stream.flush().map_err(normalize_timeout) { + Ok(()) => return Ok(()), + Err(error) + if error.kind() == io::ErrorKind::TimedOut && self.cancellation.is_some() => + { + self.remaining()?; + } + Err(error) => return Err(error), + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/kahea-exec/src/websocket.rs` around lines 2029 - 2032, Update the flush method to retry after cancellation-induced timeout errors, rechecking the actual remaining deadline on each attempt like the existing read and write loops. Preserve immediate propagation of non-timeout errors and return a timeout only when the real deadline is exhausted.
🧹 Nitpick comments (1)
crates/kahea-exec/src/websocket.rs (1)
2885-2929: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
max_frame_bytesand invalid UTF-8.This test covers
max_inbound_frames. Two enforcement paths from the linked issue stay untested:
max_frame_bytes:WireParser::observerejects a frame whose declared payload exceedslimits.max_frame_bytesat line 137. No test drives that branch, so the length-prefix parsing for the 126 and 127 extended-length forms is unverified.- Invalid UTF-8 in a text frame:
socket_errormapsWebSocketError::Utf8toProtocolViolationat line 1115. No test drives that mapping.Both cases are cheap to add with the existing raw-
Frametest server pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/kahea-exec/src/websocket.rs` around lines 2885 - 2929, Extend the websocket enforcement tests around the existing max_inbound_frames scenario to add raw-Frame coverage for max_frame_bytes, including payload lengths using both 126- and 127-byte extended-length encodings, and assert the resulting budget/terminal behavior. Add a separate raw text-frame case containing invalid UTF-8 and assert socket_error maps it to WebSocketTerminalCause::ProtocolViolation, reusing the existing test-server and observation patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/kahea-exec/src/websocket.rs`:
- Around line 815-851: Update the ExpectPong and ExpectBinary branches in the
websocket action handling to map decode_sealed_base64 failures to the same
ExecError::InvalidSeal result used by SendBinary and Ping. Remove the
ProtocolViolation mapping for these local sealed-payload decode failures while
preserving successful payload comparisons and expectation-failed behavior.
---
Outside diff comments:
In `@crates/kahea-exec/src/websocket.rs`:
- Around line 2029-2032: Update the flush method to retry after
cancellation-induced timeout errors, rechecking the actual remaining deadline on
each attempt like the existing read and write loops. Preserve immediate
propagation of non-timeout errors and return a timeout only when the real
deadline is exhausted.
---
Nitpick comments:
In `@crates/kahea-exec/src/websocket.rs`:
- Around line 2885-2929: Extend the websocket enforcement tests around the
existing max_inbound_frames scenario to add raw-Frame coverage for
max_frame_bytes, including payload lengths using both 126- and 127-byte
extended-length encodings, and assert the resulting budget/terminal behavior.
Add a separate raw text-frame case containing invalid UTF-8 and assert
socket_error maps it to WebSocketTerminalCause::ProtocolViolation, reusing the
existing test-server and observation patterns.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3313c17-87e6-4522-9c08-eb474a7ab068
📒 Files selected for processing (2)
crates/kahea-exec/src/lib.rscrates/kahea-exec/src/websocket.rs
1 participant
Summary
Validation
Closes #12
Summary by CodeRabbit
New Features
Bug Fixes