fix(start): reap CDP children + fail fast on busy port (issue #25)#27
Merged
Conversation
Probe the web port before launching Chrome and fail fast with an actionable error (exit 1) when it is already bound, instead of spawning into a doomed bind and surfacing only a generic 90s timeout. Wrap the post-launch region of the --cdp-port branch in failure-cleanup that reaps the spawned Chrome, the flutter web-server (holder + child PIDs), the FIFO, and the tmp profile directory on any throw, then surfaces the original error and returns 1. Cleanup is best-effort and never masks the original error. Add a regression test that exercises defaultChromeNavigate against a fake CDP endpoint, pinning page-target selection so the -32601 fix cannot regress to the browser-level endpoint.
…#25) Add CHANGELOG [Unreleased] entries (Added: fail-fast port-in-use error; Fixed: failure-time child reaping) and a state-and-recovery skill section covering the busy-port error and post-probe reaping.
There was a problem hiding this comment.
Pull request overview
This PR hardens the start --cdp-port flow to address issue #25 by failing fast when the requested web port is already in use, and by cleaning up spawned CDP-related resources on post-launch failure. It also adds regression coverage to prevent the prior CDP navigation fix (page-target selection) from regressing, and updates release/skill documentation accordingly.
Changes:
- Add a web-port availability probe to fail fast before launching Chrome when
--portis already bound. - Add best-effort cleanup on post-launch failures to reap Chrome/flutter processes and delete the FIFO/tmp profile directory.
- Add regression tests for CDP page-target selection in
defaultChromeNavigate, plus docs and changelog updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/src/commands/start_command.dart | Adds port probe seam, fail-fast behavior on busy --port, and post-launch cleanup logic for CDP start failures. |
| test/commands/start_command_test.dart | Adds tests for busy-port fail-fast, cleanup behavior, and a regression test for CDP page-target selection. |
| skills/fluttersdk-artisan/references/state-and-recovery.md | Documents the new fail-fast behavior and updated recovery guidance for CDP start failures. |
| CHANGELOG.md | Records the new fail-fast and cleanup behaviors under [Unreleased]. |
Comments suppressed due to low confidence (1)
lib/src/commands/start_command.dart:463
- The failure-cleanup try/catch starts after log/FIFO setup, so if
logFile.parent.create,logFile.writeAsString, or_ensureFifothrows, the already-launched detached Chrome (and tmp profile dir) will not be reaped. That leaves the same kind of orphaned Chrome/tmp-profile artifacts this PR is aiming to prevent on post-launch failures.
final logFile = File('${_logDir()}/flutter-dev.log');
await logFile.parent.create(recursive: true);
await logFile.writeAsString('');
final fifoPath = '${_logDir()}/flutter-dev.fifo';
await _ensureFifo(fifoPath);
…est (PR #27 review) Widen the CDP failure-cleanup try to include log-dir + FIFO creation, so a throw there (e.g. mkfifo failure) still reaps the already-launched Chrome and tmp profile dir instead of leaking them. Add a regression test for that path. Bind the page-target regression test's fake CDP server to the 'localhost' hostname so it matches defaultChromeNavigate's http://localhost client and stays robust across IPv4/IPv6 resolution.
…#27 review) CHANGELOG: cleanup failures are ignored silently, not logged; reword to match the code. state-and-recovery: the busy-port fast-fail reports the web port (--port), not the CDP port; clarify the heading, cause, and recovery.
Contributor
Author
|
Addressed all Copilot review points in commits 51cfcd9 + ac34e21:
Full suite green (1152 tests), |
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.
Closes #25.
Context
The primary symptom in issue #25 (CDP
Page.navigatefailing with-32601, VM Service scrape timing out) was already fixed onmasterby commit 871d0a7, which rewrotedefaultChromeNavigateto select a page-type target instead of the browser-level endpoint. This PR addresses the issue's still-open secondary observation, locks the primary fix with a regression test, and confirms the whole flow end-to-end.Changes
--port._handleCdpBranchnow probeswebPortbefore launching Chrome (newcdpPortProbeseam,ServerSocket.bindthen close). A bound port exits 1 with an actionable error naming the port and suggestingfsa stopor a different--port, instead of spawning into a doomed bind and surfacing only the generic 90s timeout. Nothing is spawned on a busy port.state.jsonwrite is wrapped in failure-cleanup. On any throw (navigate failure, scrape timeout, PID-capture failure) it reaps the spawned Chrome, the flutter web-server (holder + child PIDs), the FIFO, and the tmp profile directory, then surfaces the original error and returns 1. Cleanup is best-effort and never masks the original error; no SIGKILL grace loop (fsa stopremains the deliberate full reaper).defaultChromeNavigateagainst a fake CDP HTTP + WebSocket endpoint, so the-32601fix cannot regress.[Unreleased](Added + Fixed) and astate-and-recoveryskill recovery section.Scope is the
--cdp-portbranch only; the non-CDP branch andvmServicePortare untouched.Verification
dart analyze lib/ test/ bin/clean,dart formatno diff, 1151 tests pass, projectlib/line coverage 84.53% (above the 80% gate).fsa start --cdp-port 9399 --port 3199againstexample/returned exit 0, scrapedvmServiceUri, and wrote a populatedstate.json(chromePid / cdpPort / tmpProfileDir).fsa stopthen reaped Chrome + flutter + FIFO + tmp profile cleanly (port freed, both PIDs gone).Note
_reapAfterCdpFailuretakes 6 named params (above the 5-param style threshold). Left as-is deliberately: single internal caller, so a parameter object would be premature structure. Worth revisiting if a second reaper caller appears (e.g. V1.x consolidation withStopCommand._reapChrome).