Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fbuild-python/src/lib.rs`:
- Around line 408-416: The loop currently spin-waits when the websocket is
closed because read_lines_inner returns an empty Vec and
wait_for_remote_json_rpc_response keeps polling; fix by making the poll contract
terminal-aware: change read_lines_inner (or add a thin wrapper used by
write_json_rpc) to return Option<Vec<String>> (None meaning stream terminated on
Ok(None)/Close) or a small enum, and update wait_for_remote_json_rpc_response to
treat None as a terminal condition and break/propagate a connection error
immediately instead of retrying until timeout; update call sites (the block that
calls wait_for_remote_json_rpc_response and any other callers around lines near
read_lines_inner / write_json_rpc) to handle the terminal signal accordingly.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea30befd-365d-49d5-a5a6-379dda614bfb
📒 Files selected for processing (1)
crates/fbuild-python/src/lib.rs
| if let Some(json_part) = wait_for_remote_json_rpc_response(timeout, |remaining| { | ||
| // read_lines takes &mut self but we only have &self here — | ||
| // use the raw WS read directly | ||
| let lines = self.read_lines_inner(remaining.min(1.0)); | ||
| for line in &lines { | ||
| if let Some(json_part) = line.strip_prefix("REMOTE:") { | ||
| let json_module = py.import_bound("json")?; | ||
| let result = json_module.call_method1("loads", (json_part.trim(),))?; | ||
| return Ok(result.to_object(py)); | ||
| } | ||
| } | ||
| if lines.is_empty() { | ||
| break; | ||
| } | ||
| // use the raw WS read directly. | ||
| self.read_lines_inner(remaining.min(1.0)) | ||
| }) { | ||
| let json_module = py.import_bound("json")?; | ||
| let result = json_module.call_method1("loads", (json_part.trim(),))?; | ||
| return Ok(result.to_object(py)); | ||
| } |
There was a problem hiding this comment.
Risk of tight-spin until timeout when the WebSocket is closed/errored.
The refactor correctly stops treating a single empty batch as terminal, but it also drops the only signal the outer loop had for a dead stream. read_lines_inner returns an empty Vec<String> promptly (without consuming wall time) whenever the underlying ws_read.next() yields Ok(None) or Ok(Some(Message::Close(_))) (see lines 525), i.e. after the daemon disconnects. In that scenario wait_for_remote_json_rpc_response will now re-invoke poll immediately, every iteration returning instantly with vec![], burning a core until the caller's timeout elapses — and only then surfacing PyTimeoutError instead of a prompt connection error.
The previous code at least bailed out of the loop on the first empty batch, so a severed session failed fast. The new logic needs an equivalent "no more data will ever come" exit path.
Suggested shape: let the poller distinguish "waited and got nothing" from "stream is done", e.g. return Option<Vec<String>> (None = terminal) or a small enum, and have wait_for_remote_json_rpc_response break on the terminal variant:
♻️ Sketch of a terminal-aware poll contract
-fn wait_for_remote_json_rpc_response<F>(timeout: f64, mut poll: F) -> Option<String>
-where
- F: FnMut(f64) -> Vec<String>,
-{
+fn wait_for_remote_json_rpc_response<F>(timeout: f64, mut poll: F) -> Option<String>
+where
+ F: FnMut(f64) -> Option<Vec<String>>, // None => stream terminated
+{
let deadline = std::time::Instant::now() + std::time::Duration::from_secs_f64(timeout);
while std::time::Instant::now() < deadline {
let remaining = (deadline - std::time::Instant::now()).as_secs_f64();
- let lines = poll(remaining);
+ let Some(lines) = poll(remaining) else {
+ return None; // peer gone, don't spin
+ };
if let Some(json_part) = extract_remote_json_rpc_response(&lines) {
return Some(json_part);
}
}
None
}…and have read_lines_inner (or a thin wrapper used only by write_json_rpc) report the Ok(None) / Close case as None. As a cheaper alternative, detect "poll returned faster than remaining" and insert a bounded sleep / break, but an explicit terminal signal is more robust.
Also applies to: 542-557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-python/src/lib.rs` around lines 408 - 416, The loop currently
spin-waits when the websocket is closed because read_lines_inner returns an
empty Vec and wait_for_remote_json_rpc_response keeps polling; fix by making the
poll contract terminal-aware: change read_lines_inner (or add a thin wrapper
used by write_json_rpc) to return Option<Vec<String>> (None meaning stream
terminated on Ok(None)/Close) or a small enum, and update
wait_for_remote_json_rpc_response to treat None as a terminal condition and
break/propagate a connection error immediately instead of retrying until
timeout; update call sites (the block that calls
wait_for_remote_json_rpc_response and any other callers around lines near
read_lines_inner / write_json_rpc) to handle the terminal signal accordingly.
#65) Completes the remaining async surface from #65: - __aenter__ / __aexit__ — async context-manager that opens the /ws/serial-monitor session, sends the attach handshake, stores the split sink/source halves, and cleans up on exit. - read_lines(timeout_secs) — async batch read, honors auto_reconnect (Preempted → continue; Reconnected → continue). - write(data) — async write that waits for daemon write_ack. - write_json_rpc(request, timeout_secs) — async JSON-RPC send + poll for REMOTE: response, preserving the PR #57 full-timeout guarantee by keeping polling across empty batches. Send+Sync refactor: - Split WebSocketStream via .split() into WsSink + WsSource. - Each half stored in its own Arc<tokio::sync::Mutex<Option<_>>> so concurrent read/write futures do not serialize each other and each future owns its own Arc clone. Sync SerialMonitor is untouched — additive only. Deferred follow-up: unit tests for the new async methods. The existing 11 fbuild-python tests still pass; new tests would need a more elaborate WebSocket mock than the HTTP mock used for send_op_async (PR #84). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#65) (#107) Completes the remaining async surface from #65: - __aenter__ / __aexit__ — async context-manager that opens the /ws/serial-monitor session, sends the attach handshake, stores the split sink/source halves, and cleans up on exit. - read_lines(timeout_secs) — async batch read, honors auto_reconnect (Preempted → continue; Reconnected → continue). - write(data) — async write that waits for daemon write_ack. - write_json_rpc(request, timeout_secs) — async JSON-RPC send + poll for REMOTE: response, preserving the PR #57 full-timeout guarantee by keeping polling across empty batches. Send+Sync refactor: - Split WebSocketStream via .split() into WsSink + WsSource. - Each half stored in its own Arc<tokio::sync::Mutex<Option<_>>> so concurrent read/write futures do not serialize each other and each future owns its own Arc clone. Sync SerialMonitor is untouched — additive only. Deferred follow-up: unit tests for the new async methods. The existing 11 fbuild-python tests still pass; new tests would need a more elaborate WebSocket mock than the HTTP mock used for send_op_async (PR #84). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
SerialMonitor.write_json_rpc()polling until the caller's full timeout expiresREMOTE:extraction and the empty-batch retry pathFixes #49.
Verification
cargo test -p fbuild-pythoncargo fmt --all --checkSummary by CodeRabbit
Refactor
Tests