From 550352c1b73bed291910c7e238ed34dd4011b08a Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 3 Sep 2026 01:18:18 +0000 Subject: [PATCH] =?UTF-8?q?fix(setup):=20Chromium=20bootstrap=20is=20best-?= =?UTF-8?q?effort=20and=20bounded=20=E2=80=94=20skills=20always=20register?= =?UTF-8?q?=20(#1900,=20#1901,=20#1902,=20#913,=20#2233)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setup runs under `set -e`, and the Chromium bootstrap in section 2 sat ahead of skill registration in section 4 with a bare `bunx playwright install chromium`, an unbounded download, and an explicit `exit 1` after the post-install launch probe. On an offline, proxied, or AppArmor-restricted box the user ended with ZERO skills registered and a re-run that died at the same line; a wedged download hung setup indefinitely. Every browser failure now records a reason code in _PW_FAIL_REASON and setup continues: skipped (GSTACK_SKIP_PLAYWRIGHT=1, #913), chromium-install, chromium-install-timeout (the download is bounded by the existing _wait_with_deadline helper, default 600s, env GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT, process tree killed via _kill_tree), chromium-install-locked (another setup holds the lock: this one registers skills and re-probes next time instead of exiting), windows-no-node, windows-node-modules, post-install-launch (with the GSTACK_CHROMIUM_NO_SANDBOX=1 hint for Ubuntu 24.04's userns policy, #2157). The daemon font refresh is skipped when Chromium is unavailable. The final summary names the skills that need the browser (/qa, /qa-only, /design-review, /browse, make-pdf, /pair-agent) and the fix for the recorded reason, and logs the reason code (never a path) through gstack-telemetry-log when telemetry is on. Tests: static invariants over the anchor-sliced block (no exit, every reason code, deadline helper, trap chaining, guarded refresh, summary contents) plus an integration harness that executes the real block with a stubbed probe and installer: install failure, hang killed at the deadline with the tree kill recorded, non-numeric knob fallback, live lock (continues, installer not run, lock preserved), stale lock reclaimed, post-install probe failure, and the skip flag. Credit @DavidMiserak (PR #1900) for the best-effort shape; re-implemented on the current block. Co-Authored-By: Claude Fable 5.1 --- setup | 104 ++++++++--- test/setup-playwright-best-effort.test.ts | 218 ++++++++++++++++++++++ 2 files changed, 295 insertions(+), 27 deletions(-) create mode 100644 test/setup-playwright-best-effort.test.ts diff --git a/setup b/setup index 7b9dd6764..52594e72f 100755 --- a/setup +++ b/setup @@ -776,7 +776,33 @@ if [ -f /etc/os-release ]; then fi fi -if ! ensure_playwright_browser; then +# Chromium is BEST-EFFORT (#1900, #1901, #1902, #913, #2233). Every later step +# — skill registration (# 4), Codex/Kiro installs, migrations, hooks — is +# independent of the browser, so a failed or wedged download must never abort +# setup under `set -e`. Each failure records a reason code in _PW_FAIL_REASON; +# the skills that need Chromium (/qa, /qa-only, /design-review, /browse, +# make-pdf, /pair-agent) are named in the final summary instead of the user +# discovering a half-installed gstack. Lock contention is a reason too: another +# setup is installing Chromium right now, so this run registers skills and +# re-probes next time. The download is bounded (default 600s, env +# GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT) because Playwright's own retries cover a +# flaky socket but not a wedged bunx; the killed tree is the whole process +# group via _kill_tree. Reason codes: skipped, chromium-install, +# chromium-install-timeout, chromium-install-locked, windows-no-node, +# windows-node-modules, post-install-launch. +# test/setup-playwright-best-effort.test.ts pins this block. +_PW_FAIL_REASON="" +_pw_fail() { + local code="$1"; shift + _PW_FAIL_REASON="${_PW_FAIL_REASON:+$_PW_FAIL_REASON,}$code" + echo " Chromium bootstrap: $code — $*" >&2 +} +_PW_INSTALL_TIMEOUT="${GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT:-600}" +case "$_PW_INSTALL_TIMEOUT" in ''|*[!0-9]*) _PW_INSTALL_TIMEOUT=600 ;; esac + +if [ "${GSTACK_SKIP_PLAYWRIGHT:-0}" = "1" ]; then + _pw_fail skipped "GSTACK_SKIP_PLAYWRIGHT=1 — Chromium install skipped by request (#913)" +elif ! ensure_playwright_browser; then echo "Installing Playwright Chromium..." # XProtect self-heal (#2554): the probe failure may be the OS killing the # cached Chromium, not a missing install. Clear quarantine on the Playwright @@ -806,48 +832,50 @@ if ! ensure_playwright_browser; then else bunx playwright install chromium fi - ) + ) & + _PW_RC=0 + _wait_with_deadline $! "$_PW_INSTALL_TIMEOUT" || _PW_RC=$? + if [ "$_PW_RC" -eq 124 ]; then + _pw_fail chromium-install-timeout "bunx playwright install chromium exceeded ${_PW_INSTALL_TIMEOUT}s and was killed (raise with GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT=)" + elif [ "$_PW_RC" -ne 0 ]; then + _pw_fail chromium-install "bunx playwright install chromium exited $_PW_RC (offline, proxy, or blocked download?)" + fi rm -rf "$_PW_LOCK" 2>/dev/null || true # Restore the original handler (never `trap - EXIT`, which would clear # cleanup_copied_bun for the rest of the script). trap cleanup_copied_bun EXIT else - echo " another gstack setup is already installing Chromium (lock: $_PW_LOCK)." >&2 - echo " Wait for it to finish, then re-run ./setup. If no other setup is running," >&2 - echo " remove the stale lock: rm -rf \"$_PW_LOCK\"" >&2 - exit 1 + _pw_fail chromium-install-locked "another gstack setup is installing Chromium (lock: $_PW_LOCK) — re-run ./setup after it finishes, or remove a stale lock: rm -rf \"$_PW_LOCK\"" fi - if [ "$IS_WINDOWS" -eq 1 ]; then + if [ -z "$_PW_FAIL_REASON" ] && [ "$IS_WINDOWS" -eq 1 ]; then # On Windows, Node.js launches Chromium (not Bun — see oven-sh/bun#4253). # Ensure playwright is importable by Node from the gstack directory. if ! command -v node >/dev/null 2>&1; then - echo "gstack setup failed: Node.js is required on Windows (Bun cannot launch Chromium due to a pipe bug)" >&2 - echo " Install Node.js: https://nodejs.org/" >&2 - exit 1 + _pw_fail windows-no-node "Node.js is required on Windows to launch Chromium (Bun cannot: oven-sh/bun#4253) — install from https://nodejs.org/ and re-run ./setup" + else + echo "Windows detected — verifying Node.js can load Playwright..." + if ! ( + cd "$SOURCE_GSTACK_DIR" + # Bun's node_modules already has playwright; verify Node can require it + node -e "require('playwright')" 2>/dev/null || npm install --no-save playwright + # @ngrok/ngrok is externalized in server-node.mjs and resolved at runtime. + # Verify the platform-specific native binary is installed so /pair-agent + # tunnels don't fail later with a cryptic module-not-found error. + node -e "require('@ngrok/ngrok')" 2>/dev/null || npm install --no-save @ngrok/ngrok + ); then + _pw_fail windows-node-modules "npm could not install playwright / @ngrok/ngrok for Node.js" + fi fi - echo "Windows detected — verifying Node.js can load Playwright..." - ( - cd "$SOURCE_GSTACK_DIR" - # Bun's node_modules already has playwright; verify Node can require it - node -e "require('playwright')" 2>/dev/null || npm install --no-save playwright - # @ngrok/ngrok is externalized in server-node.mjs and resolved at runtime. - # Verify the platform-specific native binary is installed so /pair-agent - # tunnels don't fail later with a cryptic module-not-found error. - node -e "require('@ngrok/ngrok')" 2>/dev/null || npm install --no-save @ngrok/ngrok - ) fi fi -if ! ensure_playwright_browser; then +if [ -z "$_PW_FAIL_REASON" ] && ! ensure_playwright_browser; then if [ "$IS_WINDOWS" -eq 1 ]; then - echo "gstack setup failed: Playwright Chromium could not be launched via Node.js" >&2 - echo " This is a known issue with Bun on Windows (oven-sh/bun#4253)." >&2 - echo " Ensure Node.js is installed and 'node -e \"require('playwright')\"' works." >&2 + _pw_fail post-install-launch "Playwright Chromium could not be launched via Node.js (oven-sh/bun#4253) — ensure 'node -e \"require('playwright')\"' works, then re-run ./setup" else - echo "gstack setup failed: Playwright Chromium could not be launched" >&2 + _pw_fail post-install-launch "Playwright Chromium installed but could not be launched — on Ubuntu 24.04+ (AppArmor blocks unprivileged user namespaces) try GSTACK_CHROMIUM_NO_SANDBOX=1 (#2157)" fi - exit 1 fi # 2b. Ensure a color-emoji font is installed so make-pdf emoji render (Linux). @@ -859,7 +887,9 @@ if ! ensure_emoji_font; then echo " Fedora: sudo dnf install google-noto-color-emoji-fonts" >&2 echo " Arch: sudo pacman -S noto-fonts-emoji" >&2 echo " Alpine: sudo apk add font-noto-emoji" >&2 -else +elif [ -z "$_PW_FAIL_REASON" ]; then + # Only when Chromium is actually usable — restarting a daemon that cannot + # launch its browser just produces a second failure line. refresh_browse_daemon_for_fonts fi @@ -2597,3 +2627,23 @@ if ! grep -q '^redact_prepush_hook:' "$_GSTACK_CFG_FILE" 2>/dev/null; then log " installs the hook automatically in every repo you ship from." fi fi + +# ─── Chromium bootstrap summary (best-effort browser, see # 2) ─────────────── +# Printed LAST so it is the thing the user sees, after every skill registered. +if [ -n "${_PW_FAIL_REASON:-}" ]; then + log "" + log "Browser unavailable: Chromium bootstrap did not complete ($_PW_FAIL_REASON)." + log " Skills that need it: /qa, /qa-only, /design-review, /browse, make-pdf, /pair-agent." + log " Everything else is installed and works. Fix the cause and re-run ./setup." + case "$_PW_FAIL_REASON" in + *chromium-install-timeout*) log " Slow link? Raise the bound: GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT=1800 ./setup" ;; + esac + case "$_PW_FAIL_REASON" in + *post-install-launch*) log " Ubuntu 24.04+ (AppArmor user namespaces): GSTACK_CHROMIUM_NO_SANDBOX=1 ./setup (#2157)" ;; + esac + # Reason code only — never a path or command line — so a support thread can + # be matched to a code. Telemetry-gated inside gstack-telemetry-log. + if [ -x "$SOURCE_GSTACK_DIR/bin/gstack-telemetry-log" ]; then + "$SOURCE_GSTACK_DIR/bin/gstack-telemetry-log" --event-type onboarding --skill _setup_playwright --outcome "$_PW_FAIL_REASON" >/dev/null 2>&1 || true + fi +fi diff --git a/test/setup-playwright-best-effort.test.ts b/test/setup-playwright-best-effort.test.ts new file mode 100644 index 000000000..a4cde9542 --- /dev/null +++ b/test/setup-playwright-best-effort.test.ts @@ -0,0 +1,218 @@ +/** + * setup: Chromium bootstrap is best-effort and bounded (#1900, #1901, #1902, + * #913, #2233). + * + * Before: `set -e` plus a bare `bunx playwright install chromium` (and an + * explicit `exit 1` after the post-install probe) sat in section "# 2", ahead + * of "# 4. Install for Claude". An offline, proxied, or AppArmor-restricted + * box ended with ZERO skills registered, and a wedged download hung setup + * forever. Now every failure records a reason code in _PW_FAIL_REASON, the + * install is deadline-bounded, lock contention is a reason (not a fatal), and + * skill registration always runs. + * + * Two layers, following test/setup-emoji-font.test.ts's convention: + * 1. static invariants over the anchor-sliced block (line-number agnostic); + * 2. an integration harness that executes the REAL block with stubbed + * probe/installer so the exit path and reason codes are exercised. + */ +import { describe, test, expect } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const SETUP_SRC = fs.readFileSync(path.join(ROOT, 'setup'), 'utf-8'); + +const BLOCK_START = "# 2. Ensure Playwright's Chromium is available"; +const BLOCK_END = '# 2b. Ensure a color-emoji font'; + +function slice(startAnchor: string, endAnchor: string): string { + const start = SETUP_SRC.indexOf(startAnchor); + const end = SETUP_SRC.indexOf(endAnchor, start); + if (start < 0 || end < 0) throw new Error(`anchor not found: ${startAnchor} .. ${endAnchor}`); + return SETUP_SRC.slice(start, end); +} + +function extractFn(name: string): string { + const start = SETUP_SRC.indexOf(`${name}() {`); + const end = SETUP_SRC.indexOf('\n}\n', start); + if (start < 0 || end < 0) throw new Error(`function not found: ${name}`); + return SETUP_SRC.slice(start, end + 2); +} + +const block = slice(BLOCK_START, BLOCK_END); +const codeLines = block.split('\n').filter((l) => !l.trim().startsWith('#')).join('\n'); + +describe('setup: Chromium bootstrap static invariants', () => { + test('no exit inside the bootstrap block (skills must always register)', () => { + expect(codeLines).not.toMatch(/\bexit 1\b/); + expect(codeLines).not.toMatch(/\bexit\b/); + }); + + test('every failure arm records a reason code', () => { + for (const code of [ + 'skipped', 'chromium-install', 'chromium-install-timeout', 'chromium-install-locked', + 'windows-no-node', 'windows-node-modules', 'post-install-launch', + ]) { + expect(codeLines).toContain(`_pw_fail ${code} `); + } + }); + + test('the download is deadline-bounded through the shared helper and env knob', () => { + expect(codeLines).toContain('_wait_with_deadline $! "$_PW_INSTALL_TIMEOUT"'); + expect(codeLines).toContain('GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT'); + // Non-numeric knob falls back to the default instead of breaking arithmetic. + expect(codeLines).toMatch(/case "\$_PW_INSTALL_TIMEOUT" in ''\|\*\[!0-9\]\*\)/); + }); + + test('lock contention is a reason code, not a fatal', () => { + const lockElse = codeLines.slice(codeLines.indexOf('else\n _pw_fail chromium-install-locked')); + expect(lockElse.length).toBeGreaterThan(0); + expect(block).toContain('GSTACK_SKIP_PLAYWRIGHT'); + }); + + test('the lock EXIT trap still chains cleanup_copied_bun and is restored', () => { + expect(codeLines).toContain("trap 'rm -rf \"$_PW_LOCK\" 2>/dev/null || true; cleanup_copied_bun' EXIT"); + expect(codeLines).toContain('trap cleanup_copied_bun EXIT'); + }); + + test('the daemon font refresh is skipped when Chromium is unavailable', () => { + const emoji = slice(BLOCK_END, '# 3. Ensure ~/.gstack global state directory exists'); + expect(emoji).toContain('elif [ -z "$_PW_FAIL_REASON" ]; then'); + expect(emoji).toContain('refresh_browse_daemon_for_fonts'); + }); + + test('the final summary names the affected skills and the reason', () => { + const tail = SETUP_SRC.slice(SETUP_SRC.indexOf('Chromium bootstrap summary')); + expect(tail).toContain('Browser unavailable'); + for (const skill of ['/qa', '/design-review', '/browse', 'make-pdf', '/pair-agent']) { + expect(tail).toContain(skill); + } + expect(tail).toContain('$_PW_FAIL_REASON'); + expect(tail).toContain('GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT=1800'); + expect(tail).toContain('GSTACK_CHROMIUM_NO_SANDBOX=1'); + }); +}); + +/** + * Integration harness: the real block + the real deadline helpers, with the + * probe and installer stubbed. `bunx` is a shell function, so the block's + * subshell inherits the stub. Emits REASON=... and REACHED_END=1 so the test + * can prove setup continued past the block. + */ +function runBlock(opts: { + probe: 'ok' | 'fail'; + bunx: string; // body of the bunx stub + env?: Record; + preLockPid?: string; // pre-create the install lock held by this pid + markKill?: boolean; // record _kill_tree invocations to $MARK +}): { stdout: string; stderr: string; status: number; elapsedMs: number; tmp: string } { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-pw-block-')); + const mark = path.join(tmp, 'mark'); + const killTree = opts.markKill + ? extractFn('_kill_tree').replace('_kill_tree() {', '_kill_tree_orig() {') + + `\n_kill_tree() { echo killed >> "${mark}"; _kill_tree_orig "$1"; }\n` + : extractFn('_kill_tree'); + if (opts.preLockPid) { + const lock = path.join(tmp, 'gstack-playwright-install.lock'); + fs.mkdirSync(lock); + fs.writeFileSync(path.join(lock, 'pid'), opts.preLockPid); + } + const script = [ + 'set -e', + 'IS_WINDOWS=0', + `SOURCE_GSTACK_DIR="${tmp}"`, + `TMPDIR="${tmp}"`, + `MARK="${mark}"`, + '_PLAYWRIGHT_PLATFORM_OVERRIDE=""', + 'cleanup_copied_bun() { :; }', + 'trap cleanup_copied_bun EXIT', + '_clear_playwright_quarantine() { :; }', + `ensure_playwright_browser() { ${opts.probe === 'ok' ? 'return 0' : 'return 1'}; }`, + `bunx() { echo bunx-called >> "$MARK"; ${opts.bunx}; }`, + killTree, + extractFn('_wait_with_deadline'), + block, + 'echo "REASON=$_PW_FAIL_REASON"', + 'echo "REACHED_END=1"', + ].join('\n'); + const scriptPath = path.join(tmp, 'block.sh'); + fs.writeFileSync(scriptPath, script); + const t0 = Date.now(); + const r = spawnSync('bash', [scriptPath], { + encoding: 'utf-8', + timeout: 60_000, + env: { PATH: process.env.PATH ?? '', HOME: tmp, ...(opts.env ?? {}) }, + }); + return { stdout: r.stdout ?? '', stderr: r.stderr ?? '', status: r.status ?? -1, elapsedMs: Date.now() - t0, tmp }; +} + +describe('setup: Chromium bootstrap block executes best-effort', () => { + test('probe ok: no reason recorded, no install attempted', () => { + const r = runBlock({ probe: 'ok', bunx: 'exit 0' }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('REASON=\n'); + expect(r.stdout).toContain('REACHED_END=1'); + expect(fs.existsSync(path.join(r.tmp, 'mark'))).toBe(false); + }); + + test('install exits non-zero: reason chromium-install, setup continues (was: exit 1 before any skill registered)', () => { + const r = runBlock({ probe: 'fail', bunx: 'exit 7' }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('REASON=chromium-install\n'); + expect(r.stdout).toContain('REACHED_END=1'); + expect(r.stderr).toContain('chromium-install'); + expect(r.stderr).toContain('exited 7'); + }); + + test('install hangs: killed at the deadline with reason chromium-install-timeout, tree kill recorded', () => { + const r = runBlock({ + probe: 'fail', bunx: 'sleep 30', markKill: true, + env: { GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT: '1' }, + }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('REASON=chromium-install-timeout\n'); + expect(r.stdout).toContain('REACHED_END=1'); + expect(r.elapsedMs).toBeLessThan(20_000); + expect(fs.readFileSync(path.join(r.tmp, 'mark'), 'utf-8')).toContain('killed'); + }); + + test('non-numeric timeout knob falls back to the default instead of erroring', () => { + const r = runBlock({ probe: 'fail', bunx: 'exit 3', env: { GSTACK_PLAYWRIGHT_INSTALL_TIMEOUT: 'soon' } }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('REASON=chromium-install\n'); + }); + + test('lock held by a live process: reason chromium-install-locked, setup continues (was: exit 1)', () => { + const r = runBlock({ probe: 'fail', bunx: 'exit 0', preLockPid: String(process.pid) }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('REASON=chromium-install-locked\n'); + expect(r.stdout).toContain('REACHED_END=1'); + // The installer must not have run under a foreign lock. + expect(fs.existsSync(path.join(r.tmp, 'mark'))).toBe(false); + // And the foreign lock is left for its owner. + expect(fs.existsSync(path.join(r.tmp, 'gstack-playwright-install.lock'))).toBe(true); + }); + + test('stale lock (dead pid) is reclaimed and the install proceeds', () => { + const r = runBlock({ probe: 'fail', bunx: 'exit 0', preLockPid: '999999' }); + expect(r.status).toBe(0); + expect(r.stderr).toContain('reclaiming stale Chromium-install lock'); + expect(fs.readFileSync(path.join(r.tmp, 'mark'), 'utf-8')).toContain('bunx-called'); + }); + + test('install succeeds but the post-install probe fails: reason post-install-launch with the userns hint', () => { + const r = runBlock({ probe: 'fail', bunx: 'exit 0' }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('REASON=post-install-launch\n'); + expect(r.stderr).toContain('GSTACK_CHROMIUM_NO_SANDBOX=1'); + }); + + test('GSTACK_SKIP_PLAYWRIGHT=1: reason skipped, installer never invoked (#913)', () => { + const r = runBlock({ probe: 'fail', bunx: 'exit 0', env: { GSTACK_SKIP_PLAYWRIGHT: '1' } }); + expect(r.status).toBe(0); + expect(r.stdout).toContain('REASON=skipped\n'); + expect(fs.existsSync(path.join(r.tmp, 'mark'))).toBe(false); + }); +});