Merged
Conversation
…capture
Two daemon-mode network bugs fixed:
1. Performance API fallback eval timeout (network --limit):
- drain_daemon_events now handles interleaved Firefox messages (consoleAPICall
push events) that could arrive before the daemon's drain response
- evaluate_js_async now skips consoleAPICall/pageError push events that share
the console actor's `from` field, preventing misidentification as eval ack
2. Navigate --with-network captures too few events:
- New drain_network_events_timed() uses total elapsed time instead of per-read
idle timeout, capturing events that arrive in bursts during page navigation
- --network-timeout now means total collection time, not idle gap
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces per-read idle timeouts with a wall-clock total timeout for network-event draining, and adds message-filtering/looping to avoid consuming forwarded Firefox events before the daemon's responses; also updates help text and related calling sites to use the new timed drain. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI/navigate
participant Transport
participant Drain as drain_network_events_timed
participant Daemon as Daemon/Firefox
CLI->>Daemon: navigateTo (request)
Note over Daemon: Page loads → emits resource events (bursts)
Daemon-->>Transport: resource-available / resource-updated messages
CLI->>Drain: start drain(total_timeout)
Drain->>Transport: set_read_timeout(500ms)
loop while now < deadline
Transport-->>Drain: message or Timeout
alt message is daemon/resource
Drain->>Drain: parse & accumulate
else unsolicited/other
Drain->>Drain: ignore / continue
end
end
Drain-->>CLI: (Vec<NetworkResource>, Vec<NetworkResourceUpdate>)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Fixes daemon-mode network command reliability by hardening message draining / response matching and revising navigate --with-network collection to be wall-clock bounded rather than idle-gap bounded.
Changes:
- Make daemon drain and console eval handling robust to interleaved Firefox push events in daemon mode.
- Add
drain_network_events_timed()and switchnavigate --with-networkto use a total elapsed collection window. - Update CLI help text and add KB research/iteration writeups documenting the root causes and fixes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| kb/research/network-daemon-issues.md | Documents root causes and fixes for daemon-mode network issues. |
| kb/iterations/iteration-37-network-daemon-fix.md | Iteration plan/results for the daemon-mode network fixes. |
| crates/ff-rdp-core/src/actors/console.rs | Makes evaluate_js_async skip console push events so they aren’t misread as eval acks/results. |
| crates/ff-rdp-cli/src/daemon/client.rs | Makes drain_daemon_events resilient to interleaved non-daemon frames. |
| crates/ff-rdp-cli/src/commands/network_events.rs | Adds drain_network_events_timed() for total-time-bounded event collection. |
| crates/ff-rdp-cli/src/commands/navigate.rs | Switches navigate --with-network to the timed drain approach. |
| crates/ff-rdp-cli/src/cli/args.rs | Updates --network-timeout help text to reflect total-time semantics. |
Comments suppressed due to low confidence (1)
crates/ff-rdp-cli/src/commands/navigate.rs:26
- The
restore_timeoutdoc comment still refers to being called afterdrain_network_events, butnavigate --with-networknow usesdrain_network_events_timed. Updating this comment will keep the guidance accurate for future changes.
/// Restore the socket read timeout to the value established at connect time.
///
/// Called after `drain_network_events` completes so that subsequent RDP
/// round-trips (e.g. unwatch, wait condition polling) use the original timeout.
/// Failures are logged and swallowed — the drain has already completed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+83
to
+87
| Err(ProtocolError::Timeout) => { | ||
| // Per-read timeout — check if total elapsed time has exceeded the limit. | ||
| if start.elapsed() >= total_timeout { | ||
| break; | ||
| } |
There was a problem hiding this comment.
drain_network_events_timed only checks start.elapsed() when transport.recv() returns ProtocolError::Timeout. If messages keep arriving more frequently than the 500ms poll interval, recv() will never time out and this loop can run well past total_timeout, contradicting the CLI help text/semantics. Consider enforcing the deadline on every iteration (or by dynamically setting the read timeout to the remaining time) so the function stops after the requested wall-clock duration even under continuous traffic.
Comment on lines
+62
to
+94
|
|
||
| // Set a short read timeout for responsive polling. | ||
| transport.set_read_timeout(Some(poll_interval))?; | ||
|
|
||
| let mut all_resources = Vec::new(); | ||
| let mut all_updates = Vec::new(); | ||
|
|
||
| loop { | ||
| match transport.recv() { | ||
| Ok(msg) => { | ||
| let msg_type = msg.get("type").and_then(Value::as_str).unwrap_or_default(); | ||
| match msg_type { | ||
| "resources-available-array" => { | ||
| all_resources.extend(parse_network_resources(&msg)); | ||
| } | ||
| "resources-updated-array" => { | ||
| all_updates.extend(parse_network_resource_updates(&msg)); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| Err(ProtocolError::Timeout) => { | ||
| // Per-read timeout — check if total elapsed time has exceeded the limit. | ||
| if start.elapsed() >= total_timeout { | ||
| break; | ||
| } | ||
| // Otherwise keep polling. | ||
| } | ||
| Err(e) => return Err(e), | ||
| } | ||
| } | ||
|
|
||
| Ok((all_resources, all_updates)) |
There was a problem hiding this comment.
drain_network_events_timed changes the transport's read timeout (set_read_timeout(Some(poll_interval))) but does not restore the previous value. Right now callers restore manually, but this side effect is easy to miss and can break subsequent RDP round-trips if a future caller forgets. Consider saving the previous timeout and restoring it before returning (or documenting prominently that callers must restore it).
Suggested change
| // Set a short read timeout for responsive polling. | |
| transport.set_read_timeout(Some(poll_interval))?; | |
| let mut all_resources = Vec::new(); | |
| let mut all_updates = Vec::new(); | |
| loop { | |
| match transport.recv() { | |
| Ok(msg) => { | |
| let msg_type = msg.get("type").and_then(Value::as_str).unwrap_or_default(); | |
| match msg_type { | |
| "resources-available-array" => { | |
| all_resources.extend(parse_network_resources(&msg)); | |
| } | |
| "resources-updated-array" => { | |
| all_updates.extend(parse_network_resource_updates(&msg)); | |
| } | |
| _ => {} | |
| } | |
| } | |
| Err(ProtocolError::Timeout) => { | |
| // Per-read timeout — check if total elapsed time has exceeded the limit. | |
| if start.elapsed() >= total_timeout { | |
| break; | |
| } | |
| // Otherwise keep polling. | |
| } | |
| Err(e) => return Err(e), | |
| } | |
| } | |
| Ok((all_resources, all_updates)) | |
| let previous_read_timeout = transport.read_timeout()?; | |
| // Set a short read timeout for responsive polling. | |
| transport.set_read_timeout(Some(poll_interval))?; | |
| let result = { | |
| let mut all_resources = Vec::new(); | |
| let mut all_updates = Vec::new(); | |
| loop { | |
| match transport.recv() { | |
| Ok(msg) => { | |
| let msg_type = msg.get("type").and_then(Value::as_str).unwrap_or_default(); | |
| match msg_type { | |
| "resources-available-array" => { | |
| all_resources.extend(parse_network_resources(&msg)); | |
| } | |
| "resources-updated-array" => { | |
| all_updates.extend(parse_network_resource_updates(&msg)); | |
| } | |
| _ => {} | |
| } | |
| } | |
| Err(ProtocolError::Timeout) => { | |
| // Per-read timeout — check if total elapsed time has exceeded the limit. | |
| if start.elapsed() >= total_timeout { | |
| break; | |
| } | |
| // Otherwise keep polling. | |
| } | |
| Err(e) => break Err(e), | |
| } | |
| } | |
| Ok((all_resources, all_updates)) | |
| }; | |
| transport.set_read_timeout(previous_read_timeout)?; | |
| result |
…raffic Move the elapsed-time check to the top of the loop so the function stops even when messages arrive faster than the 500ms poll interval, matching the documented total-time-limit semantics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
network --limitin daemon mode):drain_daemon_eventsnow handles interleaved Firefox push events that could arrive before the daemon's drain response.evaluate_js_asyncnow skipsconsoleAPICall/pageErrorpush events that share the console actor'sfromfield, preventing misidentification as the eval acknowledgement.navigate --with-network --network-timeout): Newdrain_network_events_timed()uses total elapsed wall-clock time instead of a per-read idle timeout. This captures network events that arrive in bursts during page navigation (e.g., after a 1-2s gap while the page loads).--network-timeoutnow means total collection time, not idle gap between events.network --followandnetwork(snapshot mode) are unchanged — onlynavigate --with-networkuses the new timed drain.Test plan
cargo fmt— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace— all tests passff-rdp network --limit 20in daemon mode returns Performance API resultsff-rdp navigate --with-network --network-timeout 5000 <url>captures full page loadff-rdp network --followstill streams events in real time🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
navigate --with-network --network-timeoutnow counts total elapsed time (captures events across short idle gaps) instead of stopping on idle readsDocumentation