Repository navigation
fix(browser): prevent open hangs and reduce startup overhead - #964
Conversation
Agentflare-Branch: task/773-browser-open-hangs Agentflare-Item: 773 Agentflare-Session: d4fb427d-cddf-4f95-946e-ab30a8a4660c
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 24 minutes. View limit details
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shared process and pipe helpers crates/flare-process/Cargo.toml, crates/flare-process/src/lib.rs, crates/flare-process/src/pipe.rs, src/agent_launch.rs |
The process crate adds platform-specific pipe-readiness helpers and a standard-handle inheritance helper. Agent launch imports the shared output-readiness helper. |
Bounded subprocess output handling crates/flare-browser/src/lib.rs |
run_blocking prevents standard-handle inheritance, starts children with null stdin and piped output, and reads both output streams while waiting for child exit. Tests cover inherited pipes, output limits, and redacted failure reporting. |
Browser open benchmark
| Layer / File(s) | Summary |
|---|---|
Backend selection and benchmark run crates/flare-browser/examples/bench_open.rs |
The example selects a backend and runs five timed samples each for cold opens, warm opens, snapshots, and URL reads. It checks non-snapshot output and closes the session. |
Priority:
Estimated code review effort: 3 (Moderate) | ~30 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant BrowserCaller
participant run_blocking
participant prevent_stdio_inheritance
participant ChildProcess
participant StdoutReader
participant StderrReader
BrowserCaller->>run_blocking: start command
run_blocking->>prevent_stdio_inheritance: clear standard-handle inheritance
run_blocking->>ChildProcess: spawn with null stdin and piped output
run_blocking->>StdoutReader: drain available stdout bytes
run_blocking->>StderrReader: drain available stderr bytes
ChildProcess-->>run_blocking: exit
run_blocking->>StdoutReader: signal bounded post-exit drain
run_blocking->>StderrReader: signal bounded post-exit drain
Merge Risk: 
1ea7f
-
The change fixes browser commands hanging on inherited output pipes. No merge-blocking issue remains in the reviewed changes. A possible Windows shim-selection limitation existed before this PR and is not made worse by it.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the browser fix and the startup overhead reduction, and it matches the summarized changes: non-hanging run_blocking with bounded drain, stdio inheritance prevention, and pipe-readiness… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description clearly explains the browser hang fix, Windows process and PATH behavior, caller stdin policy, regression coverage, benchmark status, and validation results. It does not use the templa… Full details: Title check
-
Explanation
The title names the browser fix and the startup overhead reduction, and it matches the summarized changes: non-hanging run_blocking with bounded drain, stdio inheritance prevention, and pipe-readiness reuse. The 'reduce startup overhead' part is partly supported by the benchmark and the shim-priority change, but the description says the final startup timings were not rerun, so that half is less certain. The title is specific and follows the conventional-commit form, and it is not misleading.
Full details: Description check
-
Explanation
The description clearly explains the browser hang fix, Windows process and PATH behavior, caller stdin policy, regression coverage, benchmark status, and validation results. It does not use the template headings or checkbox format, and it omits an explicit Notes for reviewers section, but it includes the required testing details, risk limitations, and compatibility context.
-
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Agentflare-Branch: task/773-browser-open-hangs Agentflare-Item: 773 Agentflare-Session: d4fb427d-cddf-4f95-946e-ab30a8a4660c
Agentflare-Branch: task/773-browser-open-hangs Agentflare-Item: 773 Agentflare-Session: 5c89e410-2f92-4ef3-ad03-44c57dce1a55
|
@coderabbitai review |
|
|
@coderabbitai review |
|
Browser commands could finish navigating yet hang indefinitely because the persistent daemon and Chrome retained inherited output-pipe handles. Return when the command exits, drain pending output within the existing memory cap, and prevent the caller's original Windows stdio from leaking into redirected children.
Reuse the existing pipe-readiness checks for CLI and MCP calls. On Windows, preserve normal PATH priority and use the mise shim when it comes first, so cached install paths cannot override the project's selected version. Add regression coverage and a repeatable startup benchmark.
Pass stdin policy explicitly at the caller: the CLI inherits input for piped scripts such as
eval -- --stdin, while both MCP paths and the benchmark use null input. Scripts stay on stdin, including those larger than Windows' command-line limit.Validation:
mbx test -p flare-browser -p flare-process -- --test-threads=1: 21 passed; 7 subprocess fixtures intentionally ignored.mbx build --bin agentflarepassed. Rebuilt Windows debug CLI returned2for piped1+1and65536for a piped 64 KiB string-length script in an isolated browser session.cargo fmt --all -- --check,git diff --check, and independent review passed.The original hang fix was smoke-tested and benchmarked with a Windows debug CLI. Startup timings have not been rerun after restoring shim priority; the previously reported shortcut timings do not describe the final version-selection path.
Linux/macOS, first-use installation downloads, and the full workspace suite were not run locally.
Opened by
codexon i-flare-c997d745ae66 (flared:c997d745ae66) for item #773 via agentflare.Summary by CodeRabbit