2 Commits

Author	SHA1	Message	Date
James Anderson	9b24683503	fix(build): don't crash on benign floating asset-import errors (#1510) (#1846)	2026-06-08 19:36:19 +01:00
James Anderson	fc5f7a6492	fix(dev): hoist socket-error backstop to module top-level so lifecycle events can't remove it (#916) ... * debug(dev): hoist socket-error backstop to module top-level + opt-in trace Two changes intended to pin down why the process-level handler from PR #913 still doesn't catch the ECONNRESET trace some users report: 1. Move installation from inside configureServer to module top-level, guarded by Symbol.for to prevent double-install. Earlier versions tied teardown to httpServer 'close', which Vite emits on dep re-optimization, full reloads, and other lifecycle events — leaving a window where the listener is absent when a stale stream errors. Module-level install + Symbol guard removes that window entirely. 2. Add an opt-in console.warn marker behind VINEXT_DEBUG_SOCKET_ERRORS=1 that fires whenever the listener absorbs a peer-disconnect error. The visible `node:events:487 throw er; // Unhandled 'error' event` output otherwise leaves it ambiguous whether the listener never ran (install / lifecycle issue) or ran but was somehow bypassed (different async surface / dispatcher). The marker disambiguates. No behavior change for users not setting the env flag beyond the hoisting. Filter codes and re-throw shape are unchanged from #913. Refs #905. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address bonk review on dev socket-error backstop Three issues from PR #916 review: 1. Module-top-level install was wrong — it ran during 'vinext build', 'vp pack', and Vitest worker imports too, so the JSDoc 'dev-only' claim was inaccurate and a real ECONNRESET from a pre-render fetch in CI would have been silently swallowed. Move the install back into configureServer (which only fires in dev) but **without** binding teardown to httpServer 'close' — that was the original lifecycle bug PR #913 had. Symbol.for guard makes re-invocation a no-op so the listener still survives server restarts within a session. 2. Restore the middleware-mode skip dropped in the previous commit. Embedders running vinext in middleware mode (Express/Connect) keep ownership of their own process-level handlers. Gated on server.httpServer, matching the prior PR #913 behavior. 3. Document the listener-ordering implication of installing during configureServer (registers late in the queue, so Sentry / structured logging / test-runner hooks still observe non-peer-disconnect errors before vinext's sync re-throw aborts iteration), and the Symbol.for first-loaded-wins caveat for the multi-version case. Refactor: collapse the duplicated `(err as Error & { code }).code` casts into a single `peerDisconnectCode` helper that returns the matched code or undefined, so the debug log path reuses it without recasting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(dev): re-hoist socket-error backstop to module load (configureServer too late) The previous attempt to address bonk's review by moving install back into configureServer was wrong — it didn't restore working behavior: $ VINEXT_DEBUG_SOCKET_ERRORS=1 vp dlx vinext@455a61b dev ... GET /stage/oliver 200 in 1.1s node:events:487 throw er; // Unhandled 'error' event Error: read ECONNRESET ... No `[vinext] dev socket-error backstop installed` startup line. The function never ran. Cause: in vite-plus's plugin lifecycle, `server.httpServer` is null at the moment configureServer fires, so the `if (server.httpServer)` middleware-mode guard skipped install. Requests still get served (httpServer is created later) but the listener was never attached. Hoist back to module load. To address bonk's "loaded in build/test contexts too" concern, gate install on: - `process.env.VITEST === "true"` → Vitest worker imports - `process.argv` contains "build" → `vinext build` / `vp build` `process.argv` is read because the CLI entry imports index.ts before it has a chance to set an env var, and `configResolved` runs too late for module-load gating. Updated JSDoc to be honest about the install context, the listener- ordering implication, the middleware-mode embedder situation, and the Symbol.for first-loaded-wins caveat. Refs #905. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(dev): positive-gate socket backstop on argv[2]==='dev' Replace the negative VITEST/build skip list with a single positive check: install only when argv[2] === "dev". Matches `vinext dev`, `vp dev`, `vite dev`, and any CLI following the "<bin> dev" convention. Everything else — build, Vitest workers, library embedders with a custom runner — skips install, so genuine peer-disconnect errors surface normally in those contexts. Cleaner default: no listener unless we're confident this is a dev server invocation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(dev): gate socket backstop on Vite's command==='serve' Replace argv-sniffing with the canonical signal: Vite's `config()` hook receives `{ command: "serve" | "build" }` directly. Install the backstop only when `command === "serve"`. Works identically for `vinext dev`, `vp dev`, `vite dev`, and library embedders that call `createServer` themselves — anywhere Vite considers itself a dev server. `config()` runs before `configureServer` (so before httpServer matters) and before the dep-optimization or full-reload events that broke the earlier `httpServer.close`-tied teardown. Symbol.for guard keeps the install idempotent across server restarts within a session. Removes: - Module-load-time install + argv[2] === "dev" check - VITEST / build env-var negative skip list Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(dev): narrow gate to !isPreview so vite preview skips install `command === "serve"` covers both `vite dev` and `vite preview` (the post-build static server). Preview doesn't stream RSC and doesn't need this guard, so narrow to `command === "serve" && !env.isPreview`. `vinext start` runs prod-server.ts directly without Vite, so it was already correctly excluded — `config()` never fires there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: extend socket-error backstop to vinext start (Next.js parity) Next.js installs uncaughtException + unhandledRejection handlers in their router-server unconditionally — no dev/prod gate. Both the Node.js dev server and the Node.js prod server share the same process-level error guards. See vercel/next.js packages/next/src/server/lib/router-server.ts:809-810. vinext was only guarding the dev path. `vinext start` runs prod-server.ts directly as a Node HTTP server (used for self-hosted deploys; Cloudflare Workers prod doesn't load this module — the runtime owns socket lifecycle there) and had the same theoretical exposure to peer-disconnect crashes through the pipeline() / fetch() paths it streams responses through. Refactor: - Extract the install function from index.ts into a new src/server/socket-error-backstop.ts (drop the "Dev" prefix — no longer dev-only). - Keep the call from the vinext:config plugin's config() hook, gated on command === "serve" && !isPreview (covers vinext dev / vp dev / vite dev / library embedders). - Add a parallel call at the top of startProdServer() in prod-server.ts (covers vinext start). vinext is more conservative than Next.js's log-only handler — we filter strictly on peer-disconnect codes and sync re-throw the rest, so genuine bugs still surface. The parity is in *where* we install, not what we swallow. Vitest workers and `vinext build` never reach either entry point, so peer-disconnect errors in those contexts surface normally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(dev): auto-install socket-error backstop at module load The Vite config() hook gate (`command === "serve" && !isPreview`) proved unreliable in vite-plus's lifecycle — the hook didn't fire (or didn't pass command correctly), and install was silently skipped. Field reproducer: published 501dd95 had the call site in the bundle at the right spot, but `VINEXT_DEBUG_SOCKET_ERRORS=1` produced no startup marker, immediate ECONNRESET crash on first request. Move the install back to module load — invoked unconditionally as a side effect when socket-error-backstop.ts is imported. This was the last shape verified to work via the diagnostic flag. Drop the import-and-call pattern in index.ts in favor of a side-effect import (`import "./server/socket-error-backstop.js"`), which is enough to trigger the auto-install. The import order keeps the install ahead of every other vinext server code path. Skip in Vitest workers via `process.env.VITEST === "true"` so genuine peer-disconnect errors during test runs surface normally. Build runs are unaffected (short-lived, no peer-disconnect-prone streams) — matches Next.js's pattern of installing in any HTTP-serving entry without further gating. prod-server.ts still calls installSocketErrorBackstop() explicitly for `vinext start`. Idempotent via Symbol.for guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: call installSocketErrorBackstop directly in index.ts Remove the side-effect import + auto-install indirection. Just call the function explicitly at module top-level in index.ts, where it's obvious. Same observable behavior, clearer code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: gate socket backstop on NODE_ENV to keep prerender ECONNRESET fatal Bonk's PR #916 review caught a real corruption bug: the build path calls startProdServer() during prerender (build/run-prerender.ts:181, build/prerender.ts:398, :720), so the install fires twice during a build — once at index.ts module load, once inside startProdServer. User fetch() calls inside prerendered pages can hit ECONNRESET from flaky upstream APIs. With the backstop installed, those errors are silently absorbed instead of crashing the build, producing corrupt prerendered HTML/RSC output. Add NODE_ENV gating inside installSocketErrorBackstop: - Skip if NODE_ENV === "production" → covers vinext build + prerender - Skip if NODE_ENV === "test" → covers test runners that follow convention - Skip if VITEST === "true" → kept for Vitest specifically Vite sets NODE_ENV=production for the build command before plugins load, so the gate fires correctly. Trade-off: vinext start with NODE_ENV=production set in shell will also skip install — losing strict Next.js parity for that path. Acceptable: prod-server's pipeline() callbacks already handle the streaming case, and the real-world bug reports are all dev-server. Also fix the now-stale comment at prod-server.ts:817 that still referenced the reverted "Vite plugin config() hook" install path, and document the listener-ordering trade-off more honestly in the JSDoc — `index.ts` imports synchronously at the top of every user's vite.config.ts, registering vinext's listener earlier than user / tooling crash reporters. Sync re-throw on non-peer-disconnect errors still surfaces the crash but later-registered observers don't see the event; users who need crash-reporter visibility for those errors must register their handler before importing vinext. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: bypass socket backstop during prerender via VINEXT_PRERENDER Replace the install-time NODE_ENV gate (which broke vinext start parity by skipping installation entirely when NODE_ENV=production) with a fire-time VINEXT_PRERENDER check inside the listener. The listener is always installed, but during prerender it re-throws all errors unconditionally — acting as if no listener were present — so user fetch() ECONNRESETs during prerender crash the build with the original stack instead of being silently absorbed into corrupt prerendered output. The fire-time check (vs. install-time) is necessary because index.ts loads at Vite plugin import — well before prerender begins — and the Symbol.for guard then makes any later install call a no-op. A static install-time gate can't catch the prerender phase that follows. Set VINEXT_PRERENDER=1 in run-prerender.ts at the top of runPrerender() so the flag covers the entire prerender orchestration including startProdServer setup. prerender.ts already sets the same var around its actual render passes; this widens the scope. Restores Next.js-parity install for vinext start (NODE_ENV gate is gone). Test-runner skip stays install-time on VITEST / NODE_ENV=test since those contexts genuinely shouldn't have the listener. Refs PR #916 review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address bonk PR #916 follow-up review Three issues bonk flagged after the prerender fix landed: 1. Stale comment in prod-server.ts:816-820 — described the NODE_ENV self-gate that was reverted in b3e8759. Replace with one that describes the current behavior: idempotent install via Symbol.for guard, kept here for entry points that load prod-server without index.ts (Next.js parity), prerender bypass is fire-time via VINEXT_PRERENDER not install-time. 2. prerender.ts's finally blocks `delete VINEXT_PRERENDER` clobbers any value set by the caller. After prerenderApp returns inside runPrerender (which sets the flag for the whole orchestration), the var is deleted instead of restored to runPrerender's "1", leaving narrow gap windows around shared-server close where ECONNRESET would be absorbed instead of re-thrown. Save the prior value and restore it. Doesn't cause prerender output corruption (rendering already complete by these gaps) but it's the correct hygiene. 3. Document the orchestrator-induced ECONNRESET trade-off in the backstop's JSDoc — unconditional re-throw during prerender means a hung-route + orchestrator timeout surfaces the build crash as ECONNRESET rather than the route's own error. Acceptable but worth calling out so debuggers know to set VINEXT_DEBUG_SOCKET_ERRORS=1 to disambiguate. Plus minor JSDoc wording nit: the "Symbol.for guard prevents re-install" line was load-bearing in the wrong way — the relevant point is timing, not de-dup. Reworded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address bonk PR #916 follow-up review (round 2) - run-prerender.ts: save/restore VINEXT_PRERENDER to avoid leaking the env var into Vitest workers (mirrors prerender.ts pattern). - index.ts: correct stale install-site comment — install gate is Vitest-only; build/prerender bypass is fire-time via VINEXT_PRERENDER. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: focused unit test for socket-error-backstop predicate + install gate Addresses bonk PR #916 review item #3 (carried over). Extracts 'peerDisconnectCode' as an exported pure predicate so the matching logic can be tested in isolation without process-state mutation, and adds a thin 'isSocketErrorBackstopInstalled()' query so the test can verify the Vitest install-gate short-circuit fires in worker processes. Tests cover: - ECONNRESET / EPIPE / ECONNABORTED accepted, other codes rejected - non-Error / null / primitive reasons handled (unhandledRejection fires with arbitrary reason values) - Install gate skips Vitest workers (process.env.VITEST === 'true') Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: ask-bonk[bot] <ask-bonk[bot]@users.noreply.github.com>	2026-04-27 07:55:05 +01:00