celilo/lunacycle

Fork 0

chromium.launch() is unguarded — a browser-cache miss crashes health_check instead of recording a check #56

New issue

Closed

opened 2026-08-17 22:46:10 +00:00 by forgejo-admin · 1 comment

forgejo-admin commented

2026-08-17 22:46:10 +00:00

Owner

Found while re-reading runSpaSmoke for the #54 playwright discussion. Not the defect that discussion started from, and worse than it.

What

celilo/scripts/health-check.ts, runSpaSmoke:

let chromium: ChromiumLauncher;
try {
  chromium = await loadChromium();
} catch (err) {
  addCheck('smoke_spa', 'warn', `playwright-core not available on celilo CLI host: ...`);
  return;                                   // guarded
}

const browser = (await chromium.launch()) as import('playwright-core').Browser;   // NOT guarded

try {
  const result = await runSpaSmokeCheck({ ... });
  addCheck('smoke_spa', 'pass', ...);
} catch (err) {
  addCheck('smoke_spa', 'fail', ...);       // guarded
} finally { ... }

The module load is guarded. The smoke run is guarded. The launch() between them is bare. Anything it throws propagates straight out of runSpaSmoke, out of lunacycleHealthCheck, and out of the hook.

Why that matters

The hook's whole contract is to collect check items and throw one aggregated Health checks failed: <names> at the end. A bare throw from launch() bypasses that entirely:

No smoke_spa item is recorded, so the failure is not attributable to a named check.
The remaining checks after it (smoke_api, smoke_ws) never run.
The operator sees a raw playwright message — browserType.launch: Executable doesn't exist at /root/.cache/ms-playwright/chromium-1223/... — instead of a health check name.

And it takes out two things at once, because the same hook backs both: the 15m health monitor, and the deploy verification that gates VERIFIED.

This is live today for any launch failure — a missing browser, a sandbox/seccomp problem, OOM on a 512MB-class host, a stale cache after a disk sweep. It is not hypothetical or bump-specific.

Where it intersects #54

#54 aligns playwright-core ~1.55.1 → ~1.60.0. Playwright resolves its browser from a revision-keyed cache directory, and there is no executablePath anywhere in the hook. Read from each package's own browsers.json:

playwright-core	chromium revision	cache directory
1.55.1 (bundled today)	1193	`chromium-1193`
1.60.0 (what #54 ships)	1223	`chromium-1223`

So the bump changes which directory launch() looks in. Unless revision 1223 is already present on celilo-mgr, the first health check after that deploy throws — and because of this bug it throws uncaught rather than recording a failed smoke_spa.

Note also that nothing in the module installs a browser: celilo/scripts/package.json has no postinstall, and the root's playwright install chromium-headless-shell runs in the repo, not on celilo-mgr. Whatever chromium is on that box got there by some other route, so "it works today" is not evidence 1223 will be there tomorrow.

Worth checking before #54 lands: whether chromium-1223 exists in celilo-mgr's playwright cache. If it does not, #54 needs the browser provisioned first, in whatever form the platform-vs-module question settles on.

Fix

Independent of #54, and worth doing either way — bring launch() inside a guard so a launch failure becomes an attributable check rather than a crash:

let browser: import('playwright-core').Browser;
try {
  browser = (await chromium.launch()) as import('playwright-core').Browser;
} catch (err) {
  addCheck('smoke_spa', 'fail', `Could not launch chromium: ${...}`);
  return;
}

fail rather than warn: unlike "playwright-core is not installed at all", a browser that is present but unlaunchable is a real regression on a host that is supposed to be able to run this.

Worth a test alongside it — the loadChromium seam already makes this reachable: a stub whose launch() rejects should produce a failed smoke_spa check and let smoke_api/smoke_ws still run. #53 (PR #55) adds the harness that makes that a two-line test.

Found while re-reading `runSpaSmoke` for the #54 playwright discussion. Not the defect that discussion started from, and worse than it. ## What `celilo/scripts/health-check.ts`, `runSpaSmoke`: ```ts let chromium: ChromiumLauncher; try { chromium = await loadChromium(); } catch (err) { addCheck('smoke_spa', 'warn', `playwright-core not available on celilo CLI host: ...`); return; // guarded } const browser = (await chromium.launch()) as import('playwright-core').Browser; // NOT guarded try { const result = await runSpaSmokeCheck({ ... }); addCheck('smoke_spa', 'pass', ...); } catch (err) { addCheck('smoke_spa', 'fail', ...); // guarded } finally { ... } ``` The module *load* is guarded. The smoke *run* is guarded. The `launch()` between them is bare. Anything it throws propagates straight out of `runSpaSmoke`, out of `lunacycleHealthCheck`, and out of the hook. ## Why that matters The hook's whole contract is to collect check items and throw one aggregated `Health checks failed: <names>` at the end. A bare throw from `launch()` bypasses that entirely: - No `smoke_spa` item is recorded, so the failure is not attributable to a named check. - The remaining checks after it (`smoke_api`, `smoke_ws`) never run. - The operator sees a raw playwright message — `browserType.launch: Executable doesn't exist at /root/.cache/ms-playwright/chromium-1223/...` — instead of a health check name. And it takes out two things at once, because the same hook backs both: the 15m health monitor, and the deploy verification that gates `VERIFIED`. This is live today for any launch failure — a missing browser, a sandbox/seccomp problem, OOM on a 512MB-class host, a stale cache after a disk sweep. It is not hypothetical or bump-specific. ## Where it intersects #54 #54 aligns `playwright-core` `~1.55.1` → `~1.60.0`. Playwright resolves its browser from a **revision-keyed** cache directory, and there is no `executablePath` anywhere in the hook. Read from each package's own `browsers.json`: | playwright-core | chromium revision | cache directory | |---|---|---| | 1.55.1 (bundled today) | **1193** | `chromium-1193` | | 1.60.0 (what #54 ships) | **1223** | `chromium-1223` | So the bump changes which directory `launch()` looks in. Unless revision 1223 is already present on celilo-mgr, the first health check after that deploy throws — and because of this bug it throws *uncaught* rather than recording a failed `smoke_spa`. Note also that nothing in the module installs a browser: `celilo/scripts/package.json` has no `postinstall`, and the root's `playwright install chromium-headless-shell` runs in the repo, not on celilo-mgr. Whatever chromium is on that box got there by some other route, so "it works today" is not evidence 1223 will be there tomorrow. **Worth checking before #54 lands:** whether `chromium-1223` exists in celilo-mgr's playwright cache. If it does not, #54 needs the browser provisioned first, in whatever form the platform-vs-module question settles on. ## Fix Independent of #54, and worth doing either way — bring `launch()` inside a guard so a launch failure becomes an attributable check rather than a crash: ```ts let browser: import('playwright-core').Browser; try { browser = (await chromium.launch()) as import('playwright-core').Browser; } catch (err) { addCheck('smoke_spa', 'fail', `Could not launch chromium: ${...}`); return; } ``` `fail` rather than `warn`: unlike "playwright-core is not installed at all", a browser that is present but unlaunchable is a real regression on a host that is supposed to be able to run this. Worth a test alongside it — the `loadChromium` seam already makes this reachable: a stub whose `launch()` rejects should produce a failed `smoke_spa` check and let `smoke_api`/`smoke_ws` still run. #53 (PR #55) adds the harness that makes that a two-line test.

forgejo-admin added the

type/bug

area/deploy

priority/high

labels

2026-08-17 22:46:10 +00:00

forgejo-admin commented

2026-08-17 23:04:25 +00:00

Author

Owner

Widening this: smoke-handler.ts has the same defect in a worse form, and I only checked it because celilo/playwright-platform is writing a spec requirement against this issue.

`smoke-handler.ts` is worse than `health-check.ts`

log(`launching chromium for SPA smoke against https://${wwwHost}/`);
const { chromium } = (await import('playwright-core')) as { ... };   // NOT guarded
const browser = (await chromium.launch()) as import('playwright-core').Browser;   // NOT guarded
try {
  const result = await runSpaSmokeCheck({ ... });
  log(`SPA OK (...)`);
} finally { /* close only */ }

Three ways this is worse than the health-check version:

The dynamic import is unguarded too. health-check.ts at least wraps loadChromium() and degrades to a warn. Here, a missing playwright-core throws just like a missing browser does.
There is no addCheck at all. This is a bus handler, not a hook — there is no check list to record into, so a throw is simply a failed event-bus job. Nothing is attributable to smoke_spa, because no such item exists on this path.
max_attempts: 1 in the manifest, so there is no retry to paper over a transient launch failure.

And the same cascade: the API and WS smoke calls sit after the browser block, so a launch failure means they never run. Identical shape to the health-check case — one failure taking out the other two checks — but with even less to diagnose from.

There is also no loadChromium seam here, so unlike health-check.ts (which #53/PR #55 makes testable) this path cannot be unit-tested at all as written.

Scope correction

The issue title and body describe health-check.ts only. The fix has to cover both call sites. On the subscriber path "record a named check" is not available, so the equivalent is: catch, log with the same detail, and let the API and WS smoke still run rather than dying at the browser.

Not an instance of #57

Worth stating so nobody over-applies it: smoke-handler.ts calls runApiSmoke({ fetchImpl: fetch }) with the global fetch, and that is correct here. A bus subscriber gets no injected Fetcher — the file's own docblock explains that the hook runner's privilege model does not extend to subscriber subprocesses, which is why it shells out to celilo module config get for its config. #57 is about health-check.ts, where an injected fetcher does exist and is bypassed. This path has nothing to bypass.

Minor, fold into the same PR

smoke-handler.ts's docblock illustrates the wiring with timeout_ms: 120000. manifest.yml says 90000. Stale comment (Rule 1.5).

Widening this: `smoke-handler.ts` has the same defect in a worse form, and I only checked it because celilo/playwright-platform is writing a spec requirement against this issue. ## `smoke-handler.ts` is worse than `health-check.ts` ```ts log(`launching chromium for SPA smoke against https://${wwwHost}/`); const { chromium } = (await import('playwright-core')) as { ... }; // NOT guarded const browser = (await chromium.launch()) as import('playwright-core').Browser; // NOT guarded try { const result = await runSpaSmokeCheck({ ... }); log(`SPA OK (...)`); } finally { /* close only */ } ``` Three ways this is worse than the health-check version: 1. **The dynamic import is unguarded too.** `health-check.ts` at least wraps `loadChromium()` and degrades to a `warn`. Here, a missing `playwright-core` throws just like a missing browser does. 2. **There is no `addCheck` at all.** This is a bus handler, not a hook — there is no check list to record into, so a throw is simply a failed event-bus job. Nothing is attributable to `smoke_spa`, because no such item exists on this path. 3. **`max_attempts: 1`** in the manifest, so there is no retry to paper over a transient launch failure. And the same cascade: the API and WS smoke calls sit *after* the browser block, so a launch failure means they never run. Identical shape to the health-check case — one failure taking out the other two checks — but with even less to diagnose from. There is also no `loadChromium` seam here, so unlike `health-check.ts` (which #53/PR #55 makes testable) this path cannot be unit-tested at all as written. ## Scope correction The issue title and body describe `health-check.ts` only. The fix has to cover both call sites. On the subscriber path "record a named check" is not available, so the equivalent is: catch, log with the same detail, and let the API and WS smoke still run rather than dying at the browser. ## Not an instance of #57 Worth stating so nobody over-applies it: `smoke-handler.ts` calls `runApiSmoke({ fetchImpl: fetch })` with the global fetch, and that is **correct** here. A bus subscriber gets no injected `Fetcher` — the file's own docblock explains that the hook runner's privilege model does not extend to subscriber subprocesses, which is why it shells out to `celilo module config get` for its config. #57 is about `health-check.ts`, where an injected fetcher *does* exist and is bypassed. This path has nothing to bypass. ## Minor, fold into the same PR `smoke-handler.ts`'s docblock illustrates the wiring with `timeout_ms: 120000`. `manifest.yml` says `90000`. Stale comment (Rule 1.5).

forgejo-admin referenced this issue from a commit

2026-08-18 08:44:54 +00:00

fix(smoke): guard chromium.launch() at both sites (#56)

forgejo-admin referenced this issue from a pull request that will close it,

2026-08-18 08:45:11 +00:00

fix(smoke): guard chromium.launch() at both sites (#56) #60

forgejo-admin closed this issue

2026-08-18 15:29:39 +00:00

forgejo-admin referenced this issue from a commit

2026-08-18 15:29:40 +00:00

Merge pull request 'fix(smoke): guard chromium.launch() at both sites (#56)' (#60) from guard-browser-launch into main