Warm the browser binary before the containment probes - #14
Merged
Conversation
node / windows-latest failed two of four CI runs on the real-Chrome containment probe, always with a timed-out DevTools endpoint on the first of its four launches, about eleven seconds in. probeHostedBrowserContainment gives the browser DEVTOOLS_ENDPOINT_TIMEOUT_MS (10s) to write DevToolsActivePort. That is a supervision bound for an already-warm appliance, not a launch-latency budget for a cold CI host, where a first Chrome start pays for binary paging and profile creation. Runs the browser once and waits for it to exit before the probe loop, so the probes do not spend their bound on first-launch cost. The bound itself is unchanged, every assertion still runs against a real browser, and the probes remain the thing being measured. Warm-up failures are ignored on purpose: it is a cache warm-up, not an assertion. The harness timeout rises from 60s to 180s to cover the warm-up plus four real launches. That is the budget for the whole test, not the bound each probe is measured against. This cannot be reproduced locally, where Chrome is already warm and the test completes in about two seconds. Verified only that it still passes on both platforms; whether it removes the CI failure is what the next run establishes. Left open: if a cold Chrome start exceeds ten seconds on a GitHub Windows runner, it may also exceed it on a Raspberry Pi 5 booting from an SD card. That is a question about the supervision bound on target hardware, not about this test, and it is not answered here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe hosted browser supervisor test now warms Chrome before probes, bounds warm-up cleanup, ignores warm-up failures, and allows 180 seconds for the real-browser test. ChangesHosted browser test
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 2
🤖 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 `@scripts/hosted-browser-supervisor.test.ts`:
- Around line 99-108: Update the browser spawn arguments in warmBrowserBinary to
include --remote-debugging-port=0 and an isolated temporary --user-data-dir,
matching the DevTools startup configuration expected by the probes while keeping
the existing warm-up behavior unchanged.
- Around line 111-114: Update the timeout handling around the warm-up process
timer to wait for the child’s exit event before resolving. Add bounded,
platform-specific cleanup for the browser process tree on timed-out warm-ups,
rather than relying on detached process-group cleanup, while preserving the
existing timeout behavior for normal completion.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6101e508-c2c7-445d-9030-7900a7c967b4
📒 Files selected for processing (1)
scripts/hosted-browser-supervisor.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: e2e / windows-latest
- GitHub Check: native / windows-latest
- GitHub Check: node / windows-latest
- GitHub Check: e2e / ubuntu-latest
- GitHub Check: node / ubuntu-latest
🔇 Additional comments (2)
scripts/hosted-browser-supervisor.test.ts (2)
2-2: LGTM!
876-881: LGTM!
Comment on lines
+99
to
+108
| const child = spawn( | ||
| browserPath, | ||
| [ | ||
| "--headless=new", | ||
| "--disable-gpu", | ||
| "--no-first-run", | ||
| "--no-default-browser-check", | ||
| "--dump-dom", | ||
| "about:blank", | ||
| ], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'DEVTOOLS_ENDPOINT_TIMEOUT_MS|DevToolsActivePort|remote-debugging|user-data-dir|probeHostedBrowserContainment|spawn\(' \
scriptsRepository: Randroids-Dojo/VCG-Console
Length of output: 33771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- warm-up implementation and test call site ---'
sed -n '77,125p;865,925p' scripts/hosted-browser-supervisor.test.ts
printf '%s\n' '--- production argument builder and endpoint wait ---'
sed -n '414,438p;1641,1688p' scripts/hosted-browser-supervisor.ts
printf '%s\n' '--- focused argument comparison ---'
python3 - <<'PY'
from pathlib import Path
import re
test = Path("scripts/hosted-browser-supervisor.test.ts").read_text()
source = Path("scripts/hosted-browser-supervisor.ts").read_text()
warm = re.search(r'const child = spawn\(\s*browserPath,\s*\[(.*?)\],\s*\{', test, re.S)
probe = re.search(r'const child = spawn\(\s*browserPath,\s*\[\s*\.\.\.buildHostedBrowserArguments\(profilePath\),(.*?)\],\s*\{', source, re.S)
builder = re.search(r'return Object\.freeze\(\[(.*?)\]\);', source, re.S)
def strings(body):
return re.findall(r'"([^"]+)"|`([^`]+)`', body)
print("warm-up literal arguments:", [a or b for a,b in strings(warm.group(1))])
print("probe extra literal arguments:", [a or b for a,b in strings(probe.group(1))])
print("builder arguments:", [a or b for a,b in strings(builder.group(1))])
PYRepository: Randroids-Dojo/VCG-Console
Length of output: 6416
Exercise the DevTools startup path in the warm-up.
warmBrowserBinary omits --remote-debugging-port=0 and --user-data-dir, while the probes wait for DevToolsActivePort in fresh profiles. Use equivalent DevTools flags and an isolated temporary profile in the warm-up.
🤖 Prompt for 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.
In `@scripts/hosted-browser-supervisor.test.ts` around lines 99 - 108, Update the
browser spawn arguments in warmBrowserBinary to include
--remote-debugging-port=0 and an isolated temporary --user-data-dir, matching
the DevTools startup configuration expected by the probes while keeping the
existing warm-up behavior unchanged.
Comment on lines
+111
to
+114
| const timer = setTimeout(() => { | ||
| child.kill(); | ||
| resolve(); | ||
| }, 30_000); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'warmBrowserBinary|child\.kill|taskkill|process\.kill|close|exit' \
scripts/hosted-browser-supervisor.test.ts scripts/hosted-browser-supervisor.tsRepository: Randroids-Dojo/VCG-Console
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- imports and warm-up helper ---'
sed -n '1,125p' scripts/hosted-browser-supervisor.test.ts
printf '%s\n' '--- process-stop implementation and callers ---'
rg -n -C 18 \
'function stopBrowserProcess|const stopBrowserProcess|stopBrowserProcess\(' \
scripts/hosted-browser-supervisor.ts
printf '%s\n' '--- test sequencing and cleanup ---'
sed -n '870,930p' scripts/hosted-browser-supervisor.test.tsRepository: Randroids-Dojo/VCG-Console
Length of output: 14607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1690,1755p' scripts/hosted-browser-supervisor.ts
printf '%s\n' '--- all process-tree cleanup references ---'
rg -n 'taskkill|kill\(-|kill\(|detached|windowsHide|process\.platform|SIGKILL|SIGTERM' \
scripts package.json .github 2>/dev/null || trueRepository: Randroids-Dojo/VCG-Console
Length of output: 8191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1750,1815p' scripts/hosted-browser-supervisor.ts
printf '%s\n' '--- launch options around every production browser spawn ---'
sed -n '500,535p;850,880p;960,980p;1045,1065p' scripts/hosted-browser-supervisor.tsRepository: Randroids-Dojo/VCG-Console
Length of output: 5110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const { EventEmitter } = require('node:events');
const child = new EventEmitter();
child.kill = () => {
setTimeout(() => child.emit('exit', null, 'SIGTERM'), 50);
return true;
};
let exited = false;
child.once('exit', () => { exited = true; });
const settled = new Promise((resolve) => {
const timer = setTimeout(() => {
child.kill();
resolve();
}, 1);
child.once('exit', () => {
clearTimeout(timer);
resolve();
});
});
(async () => {
await settled;
console.log(JSON.stringify({ settledBeforeExit: !exited }));
await new Promise((resolve) => setTimeout(resolve, 75));
console.log(JSON.stringify({ exitedAfterward: exited }));
})();
JSRepository: Randroids-Dojo/VCG-Console
Length of output: 216
Wait for the timed-out browser process to terminate before resolving.
The timeout path resolves before the exit event. The warm-up process is not detached, so it cannot use the production process-group cleanup on POSIX. Wait for exit and add bounded platform-specific browser-tree cleanup for timed-out warm-ups.
🤖 Prompt for 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.
In `@scripts/hosted-browser-supervisor.test.ts` around lines 111 - 114, Update the
timeout handling around the warm-up process timer to wait for the child’s exit
event before resolving. Add bounded, platform-specific cleanup for the browser
process tree on timed-out warm-ups, rather than relying on detached
process-group cleanup, while preserving the existing timeout behavior for normal
completion.
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
1 participant
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.
node / windows-latestfailed two of four CI runs on the real-Chrome containment probe, always with a timed-out DevTools endpoint on the first of its four launches, about eleven seconds in. It is unrelated to #13 and predates CI existing.Diagnosis
probeHostedBrowserContainmentgives the browserDEVTOOLS_ENDPOINT_TIMEOUT_MS(10s) to writeDevToolsActivePort. That is a supervision bound for an already-warm appliance, not a launch-latency budget for a cold CI host, where a first Chrome start pays for binary paging and profile creation.Only
windows-latestis affected in practice because it ships Chrome, so the test actually runs there. The mocked subtests in the same file pass; only the real-browser one fails.Fix
Run the browser once and wait for it to exit before the probe loop, so the probes do not spend their bound on first-launch cost.
hosted-browser-supervisor.tsis pinned by nine plans; the test file is not, so this changes no evidence bindings. Confirmed still 551/551 current.Honest limits
This cannot be reproduced locally. Chrome is already warm on both my machines and the test completes in about two seconds. I verified only that it still passes on Linux and Windows — whether it removes the CI failure is what this run establishes. If it fails again, the answer is to measure the actual cold-start time rather than keep guessing.
Left open: if a cold Chrome start exceeds ten seconds on a GitHub Windows runner, it may also exceed it on a Raspberry Pi 5 booting from an SD card. That is a question about the supervision bound on target hardware, not about this test, and it is not answered here.
🤖 Generated with Claude Code