Repository navigation
Ask Chromium which port it bound, and say why when it will not start - #17
Merged
Merged
Conversation
CI failed on main with "Chromium did not expose a debugging port in time", which was the only thing that message could say. stderr went to /dev/null, neither `error` nor `exit` was listened for, and the loop polled a possibly-dead process until a 20s deadline — so a binary that could not start, a port already taken, and a genuinely slow boot were indistinguishable. The port is no longer ours to pick. `freePort()` opened a socket, closed it, and passed the number on, which is only true until something else takes it — and on CI that gap straddles a Supabase stack coming up. Chromium now binds port 0 and reports the result in DevToolsActivePort, so the race is gone rather than narrowed. Failures now carry Chromium's own stderr, and a process that dies is reported when it dies instead of at the deadline. All three paths were checked by pointing CHROME_PATH at a binary that exits, one that does not exist, and a script that starts and never binds: they report "exited early (code 125)", "spawn ENOENT", and the deadline with the stderr attached, respectively. The deadline goes to 60s. Twenty was tight for a cold runner, and the useful signal now comes from the process rather than from waiting. Also fixes a flake this uncovered, which is not the launcher's doing. `an unknown route falls back to the list` read location.hash straight after goto, but the `*` route lives inside RequireAccount, so nothing redirects until the gate mounts the routes — and goto returns on `.shell`, which the gate's own "Loading…" screen is one of. A probe caught it on 1 of 12 navigations, stale hash and [data-loading] present together every time. It waits for the redirect now. Not fixed globally by tightening SHELL_READY, though that would cover the whole family: `says nothing about being empty until the boards have been read` deliberately samples whichever side of the read the navigation lands on, and a stricter goto would leave it passing while testing nothing. actions/checkout and actions/setup-node go to v5. Both were being forced onto Node 24 with a deprecation warning on every run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6pCyUrdjterihUB8XQopQ
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe test harness now lets Chromium select its debugging port, verifies readiness through ChangesChromium test harness
CI action versions
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/helpers/chrome.js`:
- Around line 104-116: Update the launchChrome polling loop around the DevTools
probe to check the deadline before calling fetch, and pass an
AbortSignal.timeout based on the remaining deadline to the fetch for
/json/version. Ensure timeout or JSON parsing failures continue through the
existing retry path so the deadline can terminate Chromium.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74b2dd26-eae8-489a-9984-1f844afbae36
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/deploy.ymltest/browser/shell.test.jstest/helpers/chrome.jstest/run.js
CodeRabbit caught this, and it defeated the point of the change it was reviewing. `fetch` has no timeout of its own, so a port that binds and then says nothing — accepting the connection and never answering — parks the loop on an await nothing settles. The deadline sat below that await and was never reached, so the very failure this file was rewritten to report clearly would instead have hung the run with no message at all. Reproduced before fixing: a stub that binds a socket, writes its port to DevToolsActivePort and then stays silent ran past twelve seconds against a two-second deadline. With the probe bounded it throws at 2056ms. The deadline check moves above the probe, and the request carries an AbortSignal capped by what is left of it, so a single attempt can never outlive the deadline. Floored at 250ms so the last attempt before giving up is still a real attempt rather than one that aborts on arrival. The other three paths were re-checked and are unchanged: exits, failures to spawn, and a process that starts without ever binding all still report their own reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6pCyUrdjterihUB8XQopQ
boubou666
force-pushed
the
worktree-steadier-ci
branch
from
August 7, 2026 11:46
5e6a7a8 to
dd0e42d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/helpers/chrome.js`:
- Around line 119-120: Update the probe timeout in launchChrome to use the
remaining left duration capped only at 2,000 ms, without imposing a 250 ms
minimum. Also bound or skip the retry delay so launchChrome never sleeps beyond
deadline when no time remains.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b09c0045-d783-4e21-877a-bf7ff13692e3
📒 Files selected for processing (1)
test/helpers/chrome.js
Two of the three points here are taken. The retry sleep is bounded by what is left, since the next thing the loop does is decide whether to give up and there is no reason to spend 50ms first. And the message now reports time actually spent rather than the deadline that was asked for. The 250ms floor on the probe stays, which is the point that is declined. A probe handed the 3ms that happen to remain aborts on arrival and answers nothing, and a last attempt that cannot succeed is not a last attempt — running 250ms past a 60s deadline is the better trade. What that costs is an overshoot, and the fix for an overshoot you have chosen to accept is to report it rather than to round it away, which is what the elapsed-time message now does. Measured on the silent-port stub: throws at 2005ms saying "did not answer within 2003ms", where before the bounded sleep it was 2056ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6pCyUrdjterihUB8XQopQ
boubou666
pushed a commit
that referenced
this pull request
Aug 7, 2026
Five findings, and three of them are one mistake made three times: a diagnostic that can fail is a diagnostic that can replace the failure it was written to explain. I guarded `whoIsHere` against exactly this and then left the same hole in both of the other places. The browser context read now races a two-second timer. A page busy enough to miss a deadline is precisely the page whose next evaluate may never come back, and a hanging diagnostic turns a failing test into one that never finishes — the same shape as the unbounded DevTools probe fixed in #17, written again a few hours later. `undefined` also survives as a word: JSON.stringify hands back undefined rather than a string for it, which printed as though no context had been asked for. The repository read is allowed to fail without taking the refusal with it, so "save was refused" survives whatever goes wrong while explaining it. `whoIsHere` guards per participant rather than around the lot: one client that cannot be described should cost its own line rather than everybody else's. And the store snapshot now carries text as well as ids. Ids answer "did it arrive" and not "did the right thing arrive", which is what several of these waits are really about — one waits for `get('c1').text === 'third'`, against which a report saying c1 is present is no help whatever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H6pCyUrdjterihUB8XQopQ
boubou666
added a commit
that referenced
this pull request
Aug 7, 2026
The node and supabase suites flaked on CI and passed on re-run three times in a day, and nothing came of any of them — not because the failures were mysterious, but because none of the three failure paths kept its evidence. `timed out waiting for the op to arrive` says a thing did not happen and nothing about the state it did not happen in; the repository suite asserted `false !== true`, because save() answers false and discards the reason. A sync timeout now names every client the test joined: subscription status, whether it believes it is the writer, and what its store holds including text. Collected by registration in join() rather than an argument at each call site, because the wait that flakes is never the one anybody thought to annotate. `browser.js`'s waitFor takes an optional context expression read only after the deadline. A refused save reports the row as the database has it, fetched on the way to failing since the reason cannot be reconstructed from a run nobody can repeat. Verified rather than assumed: a temporary commit forced a timeout in CI, which printed timed out waiting for something that never happens — alice on sbxi8msj56djv: status=SUBSCRIBED, writer=true, objects=[c1]; bob on sbxi8msj56djv: status=SUBSCRIBED, writer=false, objects=[c1] and was then removed. Against the flake this exists for, that line separates "bob never joined" from "bob joined and the op never crossed". Review caught three instances of one mistake: a diagnostic that can fail is a diagnostic that can replace the failure it was written to explain. The browser context read was an unbounded async call after a timeout — the same shape as the DevTools probe fixed in #17 — and now races a timer; the repository read no longer takes the refusal with it; whoIsHere guards per participant rather than around the lot. This fixes no flake. It is the precondition for fixing one.
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.
CI failed on
mainwithChromium did not expose a debugging port in time— which was the only thing that message could ever say. stderr went to/dev/null, neithererrornorexitwas listened for, and the loop polled a possibly-dead process until a 20s deadline. A binary that could not start, a port already taken, and a genuinely slow boot all arrived identically.The race, removed rather than narrowed
freePort()opened a socket, closed it, and passed the number to Chromium — true only until something else takes it, and on CI that gap straddles a Supabase stack coming up. Chromium now binds--remote-debugging-port=0and reports the result inDevToolsActivePort, so the number is an answer instead of a guess.freePort()existed only for this and is gone.Diagnostics
Failures carry Chromium's own stderr, and a process that dies is reported when it dies rather than at the deadline. All three paths were exercised by pointing
CHROME_PATHat a binary that exits, one that does not exist, and a script that starts and never binds:Chromium exited before it was ready. / chromium exited early (code 125, signal null) / chromium said: …Chromium could not be started. / spawning /nonexistent/chromium failed: spawn … ENOENTChromium did not answer within 1500ms. / chromium said: pretending to startThe deadline goes to 60s; 20 was tight for a cold runner, and the useful signal now comes from the process rather than from waiting.
A flake this uncovered, which is not the launcher's doing
an unknown route falls back to the listreadlocation.hashimmediately aftergoto. But the*route lives insideRequireAccount(App.jsx), so nothing redirects until the gate mounts the routes — andgotoreturns on.shell, which the gate's own "Loading…" screen is one of. A probe caught it on 1 of 12 navigations, with the stale hash and[data-loading]present together every time. It waits for the redirect now.Deliberately not fixed by tightening
SHELL_READY, though that would cover the whole family in one line:says nothing about being empty until the boards have been readsamples whichever side of the read the navigation lands on, and a strictergotowould leave it passing while testing nothing. The hazard is real and worth a separate look.Also
actions/checkoutandactions/setup-nodeto v5 — both were being forced onto Node 24 with a deprecation warning on every run.Full suite run four times locally: 441/441.
🤖 Generated with Claude Code
https://claude.ai/code/session_01H6pCyUrdjterihUB8XQopQ
Summary by CodeRabbit
Bug Fixes
Chores