Skip to content

fix(cli): lazy-load @puppeteer/browsers to prevent debug package crash#1185

Merged
miguel-heygen merged 2 commits into
mainfrom
worktree-fix+cli-puppeteer-debug-crash
Jun 4, 2026
Merged

fix(cli): lazy-load @puppeteer/browsers to prevent debug package crash#1185
miguel-heygen merged 2 commits into
mainfrom
worktree-fix+cli-puppeteer-debug-crash

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen commented Jun 4, 2026

Summary

  • Convert static import { ... } from "@puppeteer/browsers" to dynamic import() inside async functions, so non-browser commands (init, lint, docs, help) never load @puppeteer/browsers or its debug transitive dep
  • Add debug as a direct dependency to guarantee installation even when transitive resolution fails
  • isLinuxArm() becomes async (only 2 callers, both already in async contexts)

Problem

@puppeteer/browsers is marked as external in the tsup bundle config, which means it stays as a bare ESM import in dist/cli.js. At parse time, Node resolves it → @puppeteer/browsers imports debug. When debug is missing or corrupted (npm cache corruption, npx stale state, CI ephemeral environments), every CLI command crashes — including commands that don't need a browser at all.

Reproduction

# Find the debug package's entry file (location varies by package manager)
DEBUG_FILE=$(find node_modules -path "*/debug@*/debug/src/index.js" 2>/dev/null | head -1)

# Hide it to simulate a corrupted/missing install
mv "$DEBUG_FILE" "${DEBUG_FILE}.bak"

# ON MAIN — every command crashes:
node packages/cli/dist/cli.js --version
# Error: Cannot find package '.../debug/src/index.js' imported from .../Cache.js
# Exit: 1

node packages/cli/dist/cli.js --help
# Same crash — even help doesn't work

node packages/cli/dist/cli.js lint --help
# Same crash

# ON FIX BRANCH — non-browser commands work fine:
node packages/cli/dist/cli.js --version
# 0.6.69

node packages/cli/dist/cli.js --help
# (full help output)

node packages/cli/dist/cli.js lint --help
# (lint help output)

# Restore
mv "${DEBUG_FILE}.bak" "$DEBUG_FILE"

Fix

The dynamic import wraps @puppeteer/browsers loading with a try/catch that provides a clear diagnostic instead of a cryptic Node module resolution error. The bundle size dropped from 6.20 MB to 6.17 MB because the static import tree is smaller.

Test plan

  • Build passes (bun run build)
  • All pre-commit hooks pass (lint, format, typecheck, fallow)
  • With debug hidden: --version, --help, lint --help all work on fix branch, all crash on main
  • No static from "@puppeteer/browsers" in dist/cli.js (verified with grep)
  • hyperframes browser ensure still downloads Chrome correctly
  • Verify crash rate drops after deploy

Convert the static `import { ... } from "@puppeteer/browsers"` in
browser/manager.ts to dynamic imports inside the async functions that
use them. This eliminates a module-load-time crash when the transitive
`debug` dependency is missing or corrupted.

Previously, every CLI command (including init, lint, docs, help) would
crash with "Cannot find package debug" if the debug package was absent —
even though only browser-related commands need @puppeteer/browsers.

Also add `debug` as a direct dependency so npm/bun always installs it
explicitly rather than relying on transitive resolution.

PostHog data: ~3,955 total-CLI-crash occurrences since May 29.
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — Lazy-load @puppeteer/browsers + pin debug as direct dep

Right shape: avoid crashing the CLI at module-import time when the user's command (e.g. hyperframes --help, hyperframes preview) doesn't actually need Puppeteer. The crash was a top-of-file import that resolved during bootstrap regardless of subcommand.

What I verified

  1. debug as a direct dep is the correct fix. @puppeteer/browsers and puppeteer-core both import debug at module top. When the bun cache deduplicates or hoists it badly, the transitive resolution fails. Pinning it as a direct dep guarantees a node_modules entry the resolver always sees. The .fallowrc.jsonc comment block already documents the rationale. ✓
  2. Every isLinuxArm() call site is awaited. I grepped the diff for both call sites:
    • packages/cli/src/browser/manager.ts:318if (await isLinuxArm())
    • packages/cli/src/commands/browser.ts:27if (await isLinuxArm())
    • The function signature flipped boolean → Promise<boolean>, so TypeScript would have caught any unawaited caller. Worth a final repo-wide grep on green CI to confirm no if (isLinuxArm()) strings survived in test files / aws-lambda / studio.
  3. Dynamic import() is cached by Node's ESM loader. Calling loadPuppeteerBrowsers() from three sites in manager.ts re-resolves the module reference but doesn't re-execute the module body — Node's ESM cache returns the same instance. No perf regression from the extra calls.
  4. Error-message scope is slightly broad. The try { return await import(...) } catch { throw new Error("...likely missing transitive dependency \\"debug\\"...") } will swallow any import failure (e.g. an actual bug inside @puppeteer/browsers, a corrupted file in the cache, a permissions issue) and report it as "missing debug". The "likely" softens it, but a real upstream import error gets misattributed. Cheap upgrade: surface the underlying error message — throw new Error('Failed to load @puppeteer/browsers (likely missing transitive dependency "debug"): ' + String(err) + '\nFix: run npm install or bun install to restore missing packages, then retry.') — keeps the suggested-fix language but also gives the user the actual error text for the unusual failure modes. Non-blocking.
  5. Version bump 0.6.51 → 0.6.69 across every package. Mass version bump is expected for a release; just flagging that the diff scope is "fix + bump", so the version-bump hunks aren't part of the security review surface. ✓

No blockers. The remaining ask is the optional grep on green CI to confirm zero un-awaited isLinuxArm() sites repo-wide.

Review by Jerrai (hyperframes specialist)

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix targeting a real, well-quantified problem. 3,955 CLI crashes from a transitive dep resolution failure is an unambiguous production signal and the defense-in-depth approach here (direct dep + lazy load) is the right architecture. Approving with two items I'd want to see followed up in a subsequent PR.


Important — isLinuxArm() defeats the isolation it's supposed to enforce

isLinuxArm() now eagerly loads @puppeteer/browsers — making it async and wrapping it in loadPuppeteerBrowsers(). But the function's entire purpose is to gate heavy puppeteer access. The implementation shouldn't require the thing it guards.

detectBrowserPlatform() from @puppeteer/browsers is a pure wrapper over os.platform() + os.arch(). You can replace the whole function without touching puppeteer at all:

export function isLinuxArm(): boolean {
  return process.platform === "linux" && process.arch === "arm64";
}

That drops the async ripple in both callers (browser.ts line 27 and manager.ts line 328), removes the eager puppeteer load on every browser ensure invocation on ARM64, and keeps the function signature in line with what callers would naturally expect from a platform predicate.

As written, browser.ts's runEnsure() calls await isLinuxArm() before findBrowser(), so on any ARM64 machine the @puppeteer/browsers module loads unconditionally — which partially undercuts the fix for that code path.


Important — loadPuppeteerBrowsers swallows the original error and hard-codes the cause

} catch {
  throw new Error(
    `Failed to load @puppeteer/browsers (likely missing transitive dependency "debug").
` + ...
  );
}

If the failure is anything other than a missing debug dep (corrupted @puppeteer/browsers itself, a different missing transitive dep, a real module-init bug), the diagnostic actively misleads the user and hides the actual Node error. At minimum:

} catch (err) {
  throw new Error(
    `Failed to load @puppeteer/browsers: ${err instanceof Error ? err.message : String(err)}.
` +
      `If the error mentions a missing package, run \`npm install\` or \`bun install\` to restore dependencies.`,
    { cause: err }
  );
}

This preserves the useful hint while surfacing the real error message.


Nit — unchecked test plan items

Both unchecked items in the test plan (hyperframes browser ensure end-to-end, crash-rate post-deploy) are worth splitting: the former is testable locally before merge and should probably be checked before this lands. The latter is a post-deploy metric which legitimately can't be pre-verified.


The core isolation approach — converting the static import to a dynamic one inside the async functions that actually need it — is correct. findFromPuppeteerCache, findFromSystem, findFromEnv, and the new PUPPETEER_CACHE_DIR scan path all work fine with no puppeteer load, which means --version, --help, lint, docs, and init all stay clean. Nice work diagnosing and shipping this quickly.

— Vai

…oad error

isLinuxArm() was async only to call detectBrowserPlatform() from
@puppeteer/browsers, but that function just checks process.platform +
process.arch under the hood. Replace with a direct inline check and make
the function sync — no behavioral change, removes an unnecessary async
boundary and an eager load of the package we're trying to lazy-load.

Also surface the real error from loadPuppeteerBrowsers() catch block instead
of hard-coding 'likely missing transitive dependency "debug"' — the actual
cause could be anything (missing package, corrupt install, wrong Node ABI).
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — commit c3b9d94252

Both my point-4 (catch-block hard-coding) and Vai's isLinuxArm self-defeat addressed. ✓

Verified:

  • isLinuxArm() is now a synchronous inline check: process.platform === "linux" && process.arch === "arm64". Matches detectBrowserPlatform()'s linux_arm mapping with zero @puppeteer/browsers import. ✓
  • Both call sites de-async'd: manager.ts:328 and commands/browser.ts:27 are back to if (isLinuxArm()). ✓
  • loadPuppeteerBrowsers catch-block now interpolates the underlying error: Failed to load @puppeteer/browsers: ${cause} — keeps the actionable suggestion line while surfacing the real failure. ✓

Ready from my side, pending stamp authorization.

Review by Jerrai (hyperframes specialist)

@miguel-heygen miguel-heygen merged commit 8c6faa4 into main Jun 4, 2026
39 checks passed
@miguel-heygen miguel-heygen deleted the worktree-fix+cli-puppeteer-debug-crash branch June 4, 2026 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants