fix(dev): hoist socket-error backstop to module top-level so lifecycle events can't remove it

[james-elicx]

Summary

Real fix for the residual #905 crashes that #913 didn't catch. Confirmed via the opt-in trace below.

#913 installed the uncaughtException handler inside configureServer and tore it down on httpServer 'close'. Vite emits close on dep re-optimization, full reloads, and other lifecycle events — leaving a window where the listener is absent when a stale stream errors. The exact pattern users were hitting:

GET /page 200 in 109ms
node:events:487
      throw er; // Unhandled 'error' event
Error: read ECONNRESET

Reproducer confirmed with the diagnostic flag from this PR:

$ VINEXT_DEBUG_SOCKET_ERRORS=1 vp dlx https://pkg.pr.new/vinext@916 dev
...
[vinext] absorbed uncaughtException ECONNRESET

The marker firing where the crash used to happen confirms the listener is now in place when the error fires.

Changes

1. Hoist install from configureServer to module top-level, guarded by Symbol.for("vinext.devSocketErrorBackstop") to prevent double-install (e.g. when vinext is loaded by both a project and a sibling workspace package). No teardown — the listener lives for the process.

2. Add opt-in VINEXT_DEBUG_SOCKET_ERRORS=1 trace that logs when the listener absorbs a peer-disconnect. Default off; useful for confirming the listener fires when users still see crashes in the field.

Filter codes (ECONNRESET / EPIPE / ECONNABORTED) and synchronous re-throw of non-peer-disconnect errors are unchanged from #913.

Refs

• Refs Dev server crashes with Unhandled 'error' event (ECONNRESET) when editing files #905 (residual crashes after fix(dev): swallow socket errors so peer disconnects don't crash dev server #911 and fix(dev): backstop uncaughtException for socket errors pipe() misses #913)
• Closes fix(dev): patch EventEmitter.emit to swallow peer-disconnect errors #915 (the EventEmitter.prototype.emit patch was the wrong direction — domain can replace prototype emit on its load, so that approach was fragile)

Test plan

• vp check passes (1 pre-existing benchmarks Cannot find module 'vinext' error is unrelated)
• Reproducer from Dev server crashes with Unhandled 'error' event (ECONNRESET) when editing files #905: VINEXT_DEBUG_SOCKET_ERRORS=1 vp dev no longer crashes; [vinext] absorbed uncaughtException ECONNRESET printed where the crash used to be
• Genuine programming errors still crash dev server with full stack (sync re-throw preserves Node default crash semantics)

🤖 Generated with Claude Code

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@916

commit: 8fa627a

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original

[james-elicx]

/bigbonk review

[ask-bonk]

Reviewing the move to a top-level install. The core insight is sound: httpServer.once('close', ...) does fire on dep re-optimization and other lifecycle events, leaving windows where the listener is absent. The repro trace in the description (absorbed uncaughtException ECONNRESET printed where the crash used to be) is good evidence the new placement actually solves the residual #905 case. A few things worth flagging before merge.

1. The "dev-only" framing in the JSDoc is incomplete

The comment says:

Dev-only by virtue of being unused by the production prod-server entry, which runs as a Cloudflare Worker where socket lifecycle is owned by the runtime.

That's accurate for the Workers runtime, but packages/vinext/src/index.ts is also loaded in two other Node contexts that aren't the dev server:

• vinext build / vp pack — the plugin module is evaluated for every build run, including CI builds that prerender or fetch external resources. A real ECONNRESET from a pre-render fetch would now be silently swallowed instead of failing the build.
• vp test workers — multiple test files import ../packages/vinext/src/index.js directly (tests/shims.test.ts:6, tests/tsconfig-paths-vite8.test.ts:6, tests/tsconfig-path-alias-build.test.ts:7). Every Vitest worker that imports those tests installs the process-level handlers.

This is probably fine in practice — peer-disconnect codes are rare in build/test contexts — but the JSDoc claim that the listener is "dev-only" is doing more work than it should. Worth either narrowing the install to command === 'serve' (gated inside config() or configResolved() so we still beat configureServer lifecycle), or updating the JSDoc to acknowledge the install runs whenever the plugin module is loaded and explain why that's still acceptable.

2. Removing the middleware-mode skip is a behavior change

The previous code had:

// Skipped in middleware mode (httpServer is null): the embedding host
// owns process-level handlers, and we have no reliable teardown hook
// to remove ours, so installation would leak.
if (server.httpServer) { ... }

The new top-level install drops that guard. Embedders running vinext in middleware mode (Express/Connect/etc. integrations) now get vinext's process-level uncaughtException and unhandledRejection listeners installed unconditionally, with no opt-out. The original comment correctly identified this as a footgun ("the embedding host owns process-level handlers"). Either restore the gate (check process.env or a vinext option) or, at minimum, drop a note in the JSDoc that middleware-mode hosts now share these listeners.

3. Listener-ordering side effect on non-peer-disconnect errors

Synchronously throwing from an uncaughtException listener aborts iteration of the remaining listeners — the comment from the deleted block called this out for the loop case, but the ordering implication is new with this PR. Quick sanity check:

process.on('uncaughtException', (e) => { console.log('FIRST'); });    // earlier loader
process.on('uncaughtException', (e) => { if (e.code === 'X') return; throw e; }); // vinext
process.on('uncaughtException', (e) => { console.log('LAST'); });     // later loader
setImmediate(() => { const e = new Error('boom'); e.code = 'OTHER'; throw e; });
// Output: FIRST  (then crash, "LAST" never runs)

By hoisting to module top level, vinext's listener is now registered before most user/tooling listeners (test runner debug hooks, crash reporters, Sentry's onUncaughtException, etc.). For peer-disconnect codes the early-return is fine. For non-peer-disconnect errors the sync re-throw still crashes the process with the correct stack — Node's default handler runs because no listener fully handled it — but later-registered listeners that wanted to observe the crash (telemetry, structured logging) never see it.

That's a real regression vs. the prior configureServer install, where vinext's listener was registered after Vite/user setup and was at the end of the queue. Probably acceptable for the dev server (where users mostly want the crash to surface and don't have telemetry attached), but combined with #1 this also affects build runs and test workers where third-party observability is more common. Worth at least documenting; ideally the listener uses process.nextTick(() => { throw err; }) and accepts the re-entry consideration with a guard, or schedules via queueMicrotask. The deleted comment block specifically rejected nextTick because it would re-enter the same listener — but a one-shot guard (e.g., if (rethrowing) process.exit(1)) would let earlier listeners surface the error first while still preserving crash semantics.

4. Symbol.for guard caveat

Symbol.for("vinext.devSocketErrorBackstop") shares state across module realms, so if two different vinext versions are loaded in the same process the first to evaluate wins and the second's filtering rules silently don't apply. Not a bug, just worth a one-liner in the JSDoc — "first-loaded version wins" — so future debugging doesn't get confused if peer disconnect handling changes between versions.

Smaller things

• Line 530 logs "dev socket-error backstop installed" only when debug is set, but the listener is installed unconditionally. That's intended, but the log line itself is a minor leak of internal terminology to user stderr — fine since it's gated behind the env var.
• The (err as Error & { code?: string }).code cast is repeated three times. isPeerDisconnect already extracts code; consider returning the code from isPeerDisconnect (or inlining a tiny getCode helper) so debug logs reuse it without recasting.
• unhandledRejection callback signature: Node passes (reason, promise). The reason: unknown typing is correct, but for completeness the signature could include the promise — not actionable, just noting.

Bottom line

The mechanism is correct and the diagnostic trace is a nice addition. My main asks before merge:

1. Either gate the install on command === 'serve' (preferred) or update the JSDoc to be honest that it runs in build/test contexts too and explain why that's OK.
2. Decide explicitly whether middleware-mode embedders should keep getting process-level listeners installed, and document the answer.
3. Briefly acknowledge the listener-ordering side effect in the comment, since it's newly relevant once we run before everyone else.

[ask-bonk]

Posted the review on PR #916. Summary of the main points:

1. JSDoc "dev-only" framing is incomplete — the plugin module is also loaded during vinext build, vp pack, and Vitest worker processes (multiple test files import index.ts directly). The listener installs in all those contexts.

2. Removing the middleware-mode skip is a silent behavior change — the previous code intentionally skipped install when httpServer was null because embedding hosts (Express/Connect) own their own process handlers. The new code installs unconditionally with no opt-out.

3. Listener-ordering side effect — by hoisting to module top-level, vinext's listener now registers before most user/tooling handlers. The synchronous re-throw on non-peer-disconnect errors aborts iteration of later listeners (Sentry, structured logging, test runner hooks). Process still crashes with the right stack, but downstream observers don't see the event. This is a regression vs. the prior configureServer install.

4. Symbol.for first-loaded-wins caveat worth documenting.

Plus a few smaller code-quality notes (repeated casts, signature pedantry).

github run

[james-elicx]

Thanks — all four addressed in 455a61b.

1 + 2 (dev-only framing & middleware-mode skip): Moved the install back into configureServer so it genuinely only fires in dev (build / Vitest worker imports of index.ts skip it entirely), and restored the if (server.httpServer) middleware-mode skip. The lifecycle bug from #913 was specifically the httpServer.once('close', ...) teardown — dropping that and relying on the Symbol.for guard for idempotency keeps the listener alive across the session without any teardown surface. Verified via the diagnostic flag that [vinext] absorbed uncaughtException ECONNRESET still fires after the previously-failing reproducer.

3 (listener ordering): Documented in the JSDoc — by going back to configureServer, vinext's listener registers late in the queue again (after most user / tooling setup), so Sentry / structured logging observers still see non-peer-disconnect errors before our sync re-throw aborts iteration. I considered the nextTick re-entry-guard variant but kept sync throw to match #913's earlier review reasoning and because the regression bonk was concerned about is largely a top-level-install artifact that no longer applies.

4 (Symbol.for caveat): Added the "first-loaded-wins" line to the JSDoc.

Smaller items:

• Repeated code cast — collapsed into a peerDisconnectCode(err) helper that returns the matched code or undefined, so the debug logs reuse it without recasting.
• unhandledRejection signature — left as (reason: unknown); the promise arg isn't used, and Node accepts the narrower signature.

[james-elicx]

You were right — that broke it. Pushed ffc5684 re-hoisting back to module load.

Root cause of the configureServer regression: in vite-plus's plugin lifecycle, server.httpServer is null when configureServer fires (it gets populated later). The if (server.httpServer) middleware-mode guard caused install to be skipped entirely. Requests are still served (httpServer is created and listens after) but our listener was never attached. Confirmed by the missing [vinext] dev socket-error backstop installed startup line in your last run.

Bonk's concerns re-addressed without going back into the broken hook:

• build / test workers — gate at module load on process.env.VITEST === "true" and process.argv containing "build". process.argv rather than an env var because cli.ts imports index.ts before it could set one, and configResolved runs too late for module-load gating.
• middleware-mode embedders — there's no module-load-time signal for middleware mode. JSDoc now acknowledges they get the listeners and points them at VINEXT_DEBUG_SOCKET_ERRORS to detect interception + a workaround (lazy import).
• listener ordering — JSDoc now honest that we register early at module load, and explains the trade-off. Sync re-throw on non-peer-disconnect kept (later observers don't see the event, but the crash still surfaces with the right stack).
• Symbol.for caveat — kept.

Worth re-running once pkg.pr.new rebuilds:

VINEXT_DEBUG_SOCKET_ERRORS=1 vp dlx https://pkg.pr.new/vinext@916 dev

The startup [vinext] dev socket-error backstop installed line should be back, and the absorb marker should fire on disconnect like the first working iteration.

[james-elicx]

Pushed 501dd95 — extends coverage to vinext start for parity with Next.js.

What Next.js does: router-server.ts:809-810 installs process.on('uncaughtException') and process.on('unhandledRejection') unconditionally — same handler runs for both their dev and prod Node.js servers. Per-request req.on('error') / socket.on('error') swallows at lines 289-294 and 823-830 are also unconditional. They install everywhere their Node server runs.

What vinext now does to match:

Context	Layer 1 (per-connection)	Layer 2 (process-level)
vinext dev / vp dev	✓ in configureServer	✓ via config() hook
vinext start (self-hosted Node prod)	already none	✓ via startProdServer()
Cloudflare Workers prod	n/a	n/a (runtime owns sockets)
vinext build	n/a	skipped
Vitest workers	n/a	skipped

Refactor:

• Extracted install function into packages/vinext/src/server/socket-error-backstop.ts, exporting installSocketErrorBackstop() (renamed — no longer "Dev"-prefixed).
• Plugin's config() hook calls it gated on command === "serve" && !isPreview.
• startProdServer() calls it at startup (matching Next.js's router-server pattern of guarding any Node HTTP-serving entry).

Where vinext intentionally diverges from Next.js: their logError is log-only — they swallow every uncaught exception silently (with a stderr log). vinext keeps the strict filter on peer-disconnect codes (ECONNRESET / EPIPE / ECONNABORTED) and synchronously re-throws everything else, so genuine bugs still surface with full stacks. The parity is in where we install, not what we swallow.

Comments + docstring at the call sites and in the new module updated to match.

[james-elicx]

/bigbonk review

[ask-bonk]

Reviewing the latest iteration after 501dd95 + 295099c7 + 90f74295. The author has done a good job iterating through the previous review cycle, and most of bonk's earlier concerns are now either resolved or honestly documented in the JSDoc. A few things still worth flagging.

1. Build-time prerender now silently swallows real ECONNRESET errors

This is the headline concern. The JSDoc explicitly justifies the lack of a build-time gate by saying:

Build runs (vinext build) also import index.ts but the listener is harmless there — the build process is short-lived and doesn't stream peer-disconnect-prone responses.

That claim is incorrect. packages/vinext/src/build/run-prerender.ts:181 and packages/vinext/src/build/prerender.ts:398 and :720 all call startProdServer() during the build to spin up a real HTTP server and prerender pages against it. Page / RSC code rendered during prerender frequently makes external fetch() calls (data fetching is the whole point of getStaticProps / RSC fetch), and those fetch calls are exactly the peer-disconnect-prone surfaces the backstop is designed to absorb.

Concrete failure mode: an upstream API the user prerenders against drops a TCP connection mid-response → ECONNRESET → backstop absorbs it → the affected fetch() either returns truncated data or hangs (depending on whether the error was on the response stream or the connection). Build appears to succeed but produces an incorrect prerendered HTML/RSC bundle. Previously this would have crashed the build with a clear stack — now it's silent corruption.

This is the same concern the previous review's #1 raised about vinext build contexts, and the JSDoc rebuttal doesn't hold once you trace through prerender. Two ways to fix:

• Gate installSocketErrorBackstop() in index.ts on process.env.NODE_ENV !== "production" or some other dev-only signal that prerender doesn't trip. The earlier-rejected command === "serve" gate was rejected because the config() hook didn't fire reliably in vite-plus, but a process-env-based gate at module load doesn't have that timing problem. (Note: prerender runs startProdServer with NODE_ENV=production typically, so NODE_ENV actually works here.)
• Or, accept the trade-off and update the JSDoc to be honest: "during prerender, peer-disconnect errors from user fetch() calls are absorbed; if you're seeing missing/truncated data in prerendered output, set VINEXT_DEBUG_SOCKET_ERRORS=1 to confirm."

The first option is preferable — a flaky upstream taking down the build is the correct behavior, not something to swallow.

2. Stale comment in prod-server.ts:817

// Process-level peer-disconnect backstop. Matches the install from
// vinext's Vite plugin config() hook for dev — covers any Node
// HTTP-serving entry, mirroring Next.js's router-server pattern.
installSocketErrorBackstop();

The "vinext's Vite plugin config() hook for dev" reference is from an earlier iteration that was reverted (see 295099c7 "auto-install socket-error backstop at module load"). Current install in index.ts is module-top-level, not a config() hook. Trivial fix but the reference will confuse future readers.

3. Listener registers extremely early — earlier than even the JSDoc admits

The JSDoc says the listener "registers early at module load" but understates how early. index.ts is the vinext plugin entry, imported synchronously at the top of every user's vite.config.ts. That means by the time any of the following has a chance to run, vinext's listener is already in the queue:

• Other Vite plugins that wrap process events (e.g., @sentry/vite-plugin, vite-plugin-node integrations)
• The user's own vite.config.ts instrumentation (if they wire up Sentry/OpenTelemetry there)
• Test runners launched via vite build && some-runner (although Vitest is gated out)

For peer-disconnect codes this is fine. For the synchronous re-throw path, vinext's listener fires first, throws, and aborts iteration of any later-registered observer. The JSDoc acknowledges this in passing but doesn't make the trade-off explicit. Suggestion: explicitly call out that "uncaught errors that aren't peer-disconnect codes will crash the process with the original stack, but listeners registered after vinext (most user/tooling crash reporters in dev) won't observe them. To observe non-peer-disconnect errors, register your handler before importing vinext."

This combined with #1 is the strongest argument for narrowing the gate: if the install genuinely runs only in dev, the impact on Sentry/OpenTelemetry visibility is contained to the dev-server use case where most users don't have crash reporters wired in anyway.

4. VITEST === "true" skip is the right default but coupled to one runner

process.env.VITEST is a Vitest-specific signal. If the codebase or downstream consumers ever run vinext code under another test runner (e.g., a Playwright test that imports a fixture's vite.config.ts), the listener installs. Probably acceptable — the concern is small — but worth mentioning that the gate is runner-coupled.

5. No focused test for the new module

packages/vinext/src/server/socket-error-backstop.ts is 85 lines of subtle process-state logic with non-obvious branching:

• Symbol.for idempotency
• VITEST short-circuit
• Peer-disconnect code filter
• Sync re-throw on non-peer-disconnect

None of this has a direct unit test. The PR description's evidence is one repro under VINEXT_DEBUG_SOCKET_ERRORS=1, which only exercises the absorb path. Per AGENTS.md guidance on "testing guidance for entry refactors" — focused helper tests are preferred when behavior moves into a typed module. Suggestion: add a small tests/socket-error-backstop.test.ts that verifies:

• Idempotent installation (call twice, only one listener added)
• VITEST env skip
• ECONNRESET / EPIPE / ECONNABORTED absorbed
• Other errors re-thrown synchronously (use a manual process.emit("uncaughtException", ...) or test the predicate in isolation by exporting it)

The predicate peerDisconnectCode is a pure function and trivial to test in isolation — splitting it into an exported helper would let the test file skip the process-state mutation entirely.

6. Smaller things

• Symbol.for guard interaction with startProdServer re-entry during prerender hybrid builds. run-prerender.ts:181 calls startProdServer once for hybrid builds; prerender.ts:398 and :720 may call it again in non-hybrid paths. The Symbol.for guard handles this correctly (second call returns early) but the JSDoc doesn't mention this is the dominant repeat-call pattern in practice. Worth a one-liner.
• installSocketErrorBackstop returns void — no way to query whether it ran. If a user reports that peer-disconnect errors aren't being absorbed, they can set VINEXT_DEBUG_SOCKET_ERRORS=1 to see the marker. That's enough, but exposing a isSocketErrorBackstopInstalled() predicate would help test code and make debugging more precise.
• (err as { code?: string } | null)?.code — the | null in the cast is defensive against null/undefined errors from unhandledRejection's reason. Good. Minor: a tiny helper function getCode(v: unknown): string | undefined would let peerDisconnectCode read getCode(err) === "ECONNRESET" || ... and avoid the as repetition.

Bottom line

The mechanism is correct and the iteration through bonk's prior review was thorough. The Next.js parity argument from 501dd95 (router-server installs in any Node HTTP-serving entry) is sound and well-cited.

The one thing that needs to change before merge is #1: prerender does stream peer-disconnect-prone fetches and the JSDoc's rebuttal is wrong. Either gate the install (preferred — NODE_ENV would work even though command === "serve" didn't) or fix the JSDoc to acknowledge the silent-corruption risk during prerender.

Items 2-6 are nice-to-have and don't block merge.

[ask-bonk]

Posted the review on PR #916. Summary of the main points:

1. JSDoc claim that build is "harmless" is incorrect — prerender.ts and run-prerender.ts call startProdServer() to spin up a real HTTP server during build, and user fetch() calls inside prerendered pages can hit ECONNRESET. Now silently absorbed instead of crashing the build, producing potentially corrupt prerendered output. This is the headline blocker — recommend gating on NODE_ENV !== "production" (which command === "serve" couldn't accomplish due to the vite-plus lifecycle issue, but NODE_ENV works at module load).

2. Stale comment in prod-server.ts:817 referencing "vinext's Vite plugin config() hook" — that path was reverted in 295099c7 but the comment didn't follow.

3. Listener-ordering regression understated — registers in index.ts top-level synchronously when the user's vite.config.ts imports vinext, which is earlier than the JSDoc admits. Sync re-throw aborts later observers (Sentry, OpenTelemetry).

4. VITEST === "true" is runner-coupled — fine as default but worth noting.

5. No focused unit test for the new 85-line module — bug-prone process-state logic deserves a test file per AGENTS.md guidance.

6. Smaller code-quality notes (repeated cast, no isInstalled() predicate, undocumented Symbol.for re-entry pattern in hybrid prerender builds).

Bottom line: only #1 blocks merge; everything else is nice-to-have.

github run

[james-elicx]

Pushed b3e8759 addressing bonk's blocker. The fix:

• Replace the install-time NODE_ENV gate 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.
• Fire-time (vs install-time) is necessary because index.ts loads at Vite plugin import, well before prerender begins. The Symbol.for guard then makes any later install call a no-op, so a static install-time gate can't catch the prerender phase that follows.
• Set VINEXT_PRERENDER=1 at the top of runPrerender() in build/run-prerender.ts so the flag covers the whole prerender orchestration including startProdServer setup. prerender.ts already sets the same var around its render passes; this widens the scope.
• Drops the NODE_ENV === "production" gate. Restores Next.js-parity install for vinext start. Test-runner skip stays install-time on VITEST / NODE_ENV === "test" since those contexts genuinely shouldn't have the listener.

Net behavior:

Context	Listener installed?	Behavior
vinext dev	yes	filters peer-disconnect ✓
vinext start	yes	filters peer-disconnect ✓ (Next.js parity)
vinext build (no prerender phase)	yes	listener present but no peer-disconnect-prone code runs
Prerender (within vinext build)	yes	re-throws everything via VINEXT_PRERENDER check ✓
Vitest workers	no	normal Node behavior ✓

[james-elicx]

/bigbonk re-review for issues

[ask-bonk]

Reviewing b3e8759 after the fix-up for the prerender silent-corruption blocker. The new design is sound and bonk's blocker is genuinely addressed. A couple of follow-ups worth flagging.

1. Stale comment in prod-server.ts:816-820

The b3e8759 commit rewrote the socket-error-backstop JSDoc to drop the NODE_ENV gate in favor of a fire-time VINEXT_PRERENDER check, but the comment at the second install site wasn't updated:

// Process-level peer-disconnect backstop. installSocketErrorBackstop
// self-gates on NODE_ENV: this call is a no-op during prerender
// (NODE_ENV=production) so user fetch() calls hit by ECONNRESET
// still crash the build instead of producing corrupt output. Fires
// for vinext start only when NODE_ENV is unset.
installSocketErrorBackstop();

Both claims here are now wrong:

• "self-gates on NODE_ENV" — the install no longer gates on NODE_ENV at all. The current install-time gate is VITEST === "true" || NODE_ENV === "test".
• "no-op during prerender (NODE_ENV=production)" — install does happen during prerender now; the prerender bypass is fire-time via VINEXT_PRERENDER inside the listener.
• "Fires for vinext start only when NODE_ENV is unset" — fires for vinext start regardless of NODE_ENV.

The end-user behavior is correct (Next.js parity for vinext start is restored, prerender still re-throws), but the comment now describes an intermediate iteration that was reverted in the same commit. Suggest replacing with something like:

// Process-level peer-disconnect backstop. Idempotent via the
// Symbol.for guard inside installSocketErrorBackstop, so this
// call is a no-op when index.ts has already installed it. Kept
// here for the rare case where prod-server is loaded without
// index.ts also being loaded (none of the current entry points
// hit this, but it preserves Next.js's "install everywhere a
// Node HTTP server runs" parity). Prerender bypass is fire-time
// via VINEXT_PRERENDER, not install-time.

2. prerender.ts's delete VINEXT_PRERENDER clobbers runPrerender's set

Tracing through the hybrid build path (run-prerender.ts:133 → prerenderApp → prerenderPages):

1. runPrerender:133 sets VINEXT_PRERENDER=1 (no try/finally — never restored).
2. prerenderApp enters its try, re-sets VINEXT_PRERENDER=1 at prerender.ts:690. Its finally deletes the var at prerender.ts:1116.
3. After prerenderApp returns, the var is deleted (not restored to the value runPrerender set).
4. prerenderPages runs next, re-sets VINEXT_PRERENDER=1 at prerender.ts:373, finally deletes at line 645.
5. After runPrerender's try block completes, VINEXT_PRERENDER is deleted, but sharedProdServer.close() runs in the outer finally at run-prerender.ts:271.

The gap windows where VINEXT_PRERENDER is unset during a prerender run:

• Between prerenderApp returning and prerenderPages setting the var. Synchronous, near-zero in practice but technically a window where ECONNRESET escaping during shared-server idle would be absorbed instead of crashing.
• During sharedProdServer.close() in run-prerender.ts:268-273. TCP half-close can asynchronously surface ECONNRESET on the server-side socket; with the var unset, the backstop absorbs.

Neither window can produce corrupt prerendered output (rendering is already complete by the time these fire), so this isn't a #1-style silent-corruption blocker. But it's worth fixing for hygiene: prerender.ts's set/delete should save = process.env.VINEXT_PRERENDER; ... ; if (save === undefined) delete process.env.VINEXT_PRERENDER; else process.env.VINEXT_PRERENDER = save; so callers' set isn't clobbered.

Trivial change inside prerender.ts; doesn't block merge.

3. Orchestrator-induced ECONNRESET during prerender now crashes the build

The current behavior:

process.on("uncaughtException", (err: Error) => {
  if (process.env.VINEXT_PRERENDER === "1") throw err;  // unconditional re-throw
  const code = peerDisconnectCode(err);
  ...
});

During prerender, any uncaught error — including an ECONNRESET on the local 127.0.0.1 prod server caused by the orchestrator aborting/timing out a route fetch — crashes the build. This is the right tradeoff for ECONNRESETs that originate from user fetch() calls (silent-corruption avoidance), but it conflates them with ECONNRESETs that originate from the orchestrator's own HTTP client behavior.

Concrete failure mode: runPrerender orchestrator fetches /some/slow/route, an upstream issue inside the prod server makes that response take >30s, the orchestrator's HTTP client aborts (or the route render hangs and the orchestrator times out), ECONNRESET surfaces on the prod server's socket, escapes to uncaughtException, build crashes with an opaque "Error: read ECONNRESET" rather than the actual route error.

This is rare and probably acceptable — a hung prerender route is itself a build problem worth surfacing — but the tradeoff isn't documented. The JSDoc currently says only that user fetch() calls during prerender hit external APIs that can drop connections. Worth adding a one-liner: "Note: this also makes the prod server's own incoming-connection ECONNRESETs (orchestrator client aborts) fatal during prerender. Acceptable because such errors usually indicate a stuck route render, but the resulting crash will surface as ECONNRESET rather than the underlying cause."

A more nuanced approach would re-throw only when peerDisconnectCode(err) is set during prerender (so non-peer-disconnect errors crash with their original stack as before, but peer-disconnects also crash with ECONNRESET). The current throw err unconditional path achieves the same effect for peer-disconnects but is needlessly broad — non-peer-disconnect errors would crash anyway via the second throw err later in the listener. Simplification: keep unconditional re-throw but note in code that it's defense in depth, not behavior change for non-peer-disconnects.

4. JSDoc claim about index.ts load timing is slightly imprecise

 * Doing the check at install time wouldn't work: `index.ts` loads at
 * Vite plugin import (well before prerender starts), so by the time
 * `VINEXT_PRERENDER` is set the listener would already be installed
 * via the Symbol.for guard.

Strictly: the Symbol.for guard would prevent re-install, but the install was always a one-shot anyway — so the relevant point is "by the time VINEXT_PRERENDER is set, the listener is already installed (period); we cannot uninstall and re-install it." The Symbol.for is doing the de-dup work, not the timing work. Minor wording nit; the intent is clear.

5. (Pre-existing, not introduced by this PR) prod-server.ts:999-1000 comment

// Forward to RSC handler — the endpoint is implemented there and has
// access to the in-process map. VINEXT_PRERENDER=1 must be set (it is,
// since this server is only started during the prerender phase).
// Fall through to the RSC handler below.

"This server is only started during the prerender phase" is wrong — startProdServer is also the entry point for vinext start self-hosted Node deploys, where VINEXT_PRERENDER is unset and the prerender endpoints correctly 403. Not introduced by this PR; fix opportunistically if you're touching the file.

Bottom line

The fix is correct and bonk's prior #1 is addressed. The headline issue is the stale comment in prod-server.ts:816-820 which now contradicts the code behavior — that should be updated before merge to avoid future-debugger confusion. Items 2-5 are nice-to-haves and don't block.

Verifying the fix end-to-end:

Context	installSocketErrorBackstop runs?	VINEXT_PRERENDER at fire time	Net behavior
vinext dev	yes (index.ts top-level)	unset	filters peer-disconnect ✓
vinext start	yes (index.ts + prod-server, second is no-op)	unset	filters peer-disconnect ✓ (Next.js parity)
vinext build non-prerender phase	yes (index.ts)	unset	filters peer-disconnect (acceptable; build doesn't open prerender-prone sockets)
Prerender (within vinext build)	yes (already installed)	"1"	re-throws unconditionally ✓
Vitest workers	no	n/a	normal Node behavior ✓

This matches the table in the PR comment. The mechanism holds up.

[ask-bonk]

Review posted on PR #916. Summary of the main points:

1. Stale comment in prod-server.ts:816-820 — describes the old NODE_ENV self-gate that was reverted in b3e8759. Comment now contradicts the code (claims "no-op during prerender (NODE_ENV=production)" but install actually does fire and the prerender bypass is fire-time via VINEXT_PRERENDER). The headline cleanup item.

2. prerender.ts's delete VINEXT_PRERENDER clobbers runPrerender's set — prerenderApp/prerenderPages blindly delete in their finally blocks rather than restoring the prior value, leaving narrow gap windows around shared-server close where ECONNRESET would be absorbed. Doesn't cause prerender corruption (rendering already complete), but worth fixing.

3. Orchestrator-induced ECONNRESET during prerender now crashes — the unconditional throw err during prerender means a hung-route + orchestrator timeout will surface as opaque ECONNRESET rather than the underlying cause. Acceptable tradeoff but undocumented.

4. JSDoc wording nit about Symbol.for vs install timing.

5. Pre-existing wrong comment in prod-server.ts:999-1000 (not introduced by this PR; opportunistic fix).

Verified the end-to-end matrix matches the author's claim — the fix design holds up. Bonk's prior #1 (silent-corruption blocker) is genuinely resolved.

github run

[james-elicx]

Pushed 7894608 addressing the follow-up.

#1 Stale comment in prod-server.ts — replaced. Now describes the current behavior: idempotent install via the Symbol.for guard, kept at this site 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 clobbering runPrerender's set — both prerenderApp and prerenderPages finally blocks now save previousPrerenderFlag = process.env.VINEXT_PRERENDER before setting and restore it (delete only when the prior value was undefined). Closes the gap windows around shared-server close where ECONNRESET would have been absorbed instead of re-thrown.

#3 Orchestrator-induced ECONNRESET docs — added a paragraph to the backstop JSDoc calling out that an orchestrator client abort during prerender will surface the build crash as Error: read ECONNRESET rather than the underlying route failure, and that VINEXT_DEBUG_SOCKET_ERRORS=1 can disambiguate.

#4 JSDoc wording nit — reworded "by the time VINEXT_PRERENDER is set the listener would already be installed via the Symbol.for guard" to "the listener has already been installed (we cannot uninstall and re-install per phase)". The Symbol.for is doing de-dup, not timing — your phrasing is right.

#5 Pre-existing prod-server.ts:999-1000 comment — left for now; happy to fix opportunistically in a follow-up if useful but not touching this PR's scope.

[james-elicx]

/bigbonk re-review for issues

[ask-bonk]

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk re-review for genuine issues

[ask-bonk]

Reviewing 7894608 after the follow-up. The previous-review blockers are all addressed cleanly. Verified end-to-end:

• prod-server.ts:809-823 — comment now accurately describes idempotent install + Symbol.for guard + fire-time VINEXT_PRERENDER bypass. ✓
• prerender.ts:373-374 / 646-647 and :695-696 / 1122-1123 — save/restore pattern around VINEXT_PRERENDER correctly preserves runPrerender's outer set across the inner prerenderApp / prerenderPages finally blocks. ✓
• socket-error-backstop.ts:46-54 — orchestrator-ECONNRESET tradeoff documented. ✓
• socket-error-backstop.ts:41-44 — JSDoc wording corrected; Symbol.for is doing de-dup, not timing. ✓

Nothing here blocks merge. A few small things worth noting before sign-off, none of which are regressions vs. the prior iteration:

1. runPrerender sets VINEXT_PRERENDER without a save/restore wrapper

packages/vinext/src/build/run-prerender.ts:133 sets the var unconditionally and never restores it. For the CLI build path this is fine (process exits after). For tests calling runPrerender directly, VINEXT_PRERENDER is left set in the worker after the function returns.

This doesn't currently cause test failures because the backstop's install gate (VITEST === "true") skips installation in Vitest workers, so the dirty env var doesn't trip the prerender bypass on a listener that doesn't exist. But it's an environmental side-effect that future test code or a runtime that doesn't set VITEST could trip on.

Suggest mirroring the same save/restore pattern used in prerender.ts:

const previousPrerenderFlag = process.env.VINEXT_PRERENDER;
process.env.VINEXT_PRERENDER = "1";
try {
  // ... existing body
} finally {
  if (previousPrerenderFlag === undefined) delete process.env.VINEXT_PRERENDER;
  else process.env.VINEXT_PRERENDER = previousPrerenderFlag;
}

Trivial and keeps the contract symmetric with prerender.ts's helpers.

2. index.ts:118-126 install-site comment is slightly imprecise

// Self-gates on NODE_ENV / VITEST so build (incl. prerender) and tests
// skip cleanly. See socket-error-backstop.ts for full rationale.
installSocketErrorBackstop();

The "build (incl. prerender) ... skip cleanly" framing is wrong. The install is not skipped during build — it runs at module load (vinext build imports index.ts). Prerender isn't skipped at install either; prerender bypass is fire-time via VINEXT_PRERENDER inside the listener. The install gate is only VITEST === "true" || NODE_ENV === "test" (per socket-error-backstop.ts:86).

This is the same kind of stale-iteration comment that prod-server.ts:816-820 was carrying before 7894608 fixed it. Since the JSDoc inside socket-error-backstop.ts explains the actual behavior at length, the call-site comment can be much shorter and just point at the module:

// Install the process-level peer-disconnect backstop at module load.
// Skips Vitest workers via env-var gate; bypasses during prerender via
// fire-time VINEXT_PRERENDER check. See socket-error-backstop.ts.
installSocketErrorBackstop();

3. No focused unit test for socket-error-backstop.ts — carried over from prior review

This was item #5 in the previous review and was acknowledged as nice-to-have. Still applies. The module now has 113 lines of subtle process-state logic with multiple branching conditions:

• Symbol.for idempotency (line 85)
• Test-runner short-circuit (line 86)
• VINEXT_PRERENDER re-throw (lines 96, 105)
• Peer-disconnect filter (lines 97-101, 106-110)
• Sync re-throw on non-peer-disconnect (lines 102, 111)

Per AGENTS.md "testing guidance for entry refactors": when behavior moves into a typed module, focused helper tests are preferred. The pure predicate peerDisconnectCode is trivial to export-and-test in isolation; the install logic can be tested by clearing the Symbol.for guard between calls and asserting process.listeners("uncaughtException").length.

Not a merge blocker, but the module would benefit from a test file to prevent regressions on the next iteration of this code (e.g., if someone adds another env-var bypass or filter case).

4. unhandledRejection's throw reason with non-Error rejections

When reason is a non-Error (e.g., a string, null, or a plain object without .code), peerDisconnectCode(reason) returns undefined and the listener falls through to throw reason. Node converts that to an uncaughtException, which re-enters vinext's uncaughtException listener. That second pass either (a) hits VINEXT_PRERENDER and re-throws, or (b) peerDisconnectCode returns undefined and re-throws.

The terminal behavior is correct (process crashes), but the crash surface for non-Error rejections is now layered — Node's default unhandled-rejection diagnostics (the Promise { ... } reference, the node --unhandled-rejections=warn switch) are bypassed because we converted it into an uncaught exception. Strictly: this is the same behavior as process.on("unhandledRejection", (r) => { throw r; }) without vinext, but worth a comment noting that vinext does not preserve Node's unhandled-rejection surface for non-peer-disconnect non-Error rejections.

Minor; not actionable unless the surface matters in practice.

Bottom line

Ready to merge from my side. Item #1 is the only one I'd suggest fixing in this PR (trivial save/restore symmetry); items #2-#4 are documentation/test-coverage hygiene that can land in a follow-up.

[ask-bonk]

The review was posted successfully as a PR review (not as an issue comment, which is why it didn't show under comments). Posted the review on PR #916.

Summary of the main points:

1. Previous-review blockers all resolved — prod-server.ts stale comment fixed, prerender.ts save/restore correctly preserves outer set, orchestrator-ECONNRESET tradeoff documented, JSDoc wording corrected.

2. runPrerender sets VINEXT_PRERENDER without save/restore — fine for CLI builds (process exits) but leaks the env var into Vitest workers after runPrerender() returns. Currently harmless because the install gate skips backstop in tests, but should mirror the symmetric save/restore pattern from prerender.ts for hygiene.

3. Stale index.ts:118-126 install-site comment — claims build (incl. prerender) "skip cleanly" via the install gate, but the gate is only for test runners. Build and prerender bypass via fire-time VINEXT_PRERENDER, not install-time. Same kind of stale-iteration comment that was just fixed in prod-server.ts.

4. No focused unit test — carried over from prior review fix: inline source content in sourcemaps to prevent missing source warnings #5; still applies, still nice-to-have.

5. unhandledRejection with non-Error reasons — re-throwing converts unhandled rejections into uncaught exceptions, bypassing Node's default unhandled-rejection diagnostics. Minor surface concern, not actionable.

Bottom line: only #1 is worth fixing in this PR; #2-#4 can land in a follow-up.

github run

[james-elicx]

/bigbonk re-review for genuine issues

[ask-bonk]

Committed d6a9d5b. Summary of what was addressed:

Round 3 follow-up addressing bonk's most recent review:

The latest commit on the branch (3fa5be1) had already resolved bonk's items #1 (runPrerender save/restore) and #2 (index.ts:118-126 stale install-site comment) before I started. The remaining items from that review were #3 (no focused unit test) and #4 (unhandledRejection non-Error reasons surface concern).

This commit (d6a9d5b) addresses #3:

• Extracted peerDisconnectCode as a named export so the pure predicate is testable in isolation without mutating process state.
• Added isSocketErrorBackstopInstalled() query so the test can assert the Vitest install-gate short-circuit (VITEST === "true") actually fires in worker processes.
• Added tests/socket-error-backstop.test.ts (4 tests, ~270ms) covering:
  • ECONNRESET / EPIPE / ECONNABORTED matched
  • Other codes (ENOENT, EHOSTUNREACH, no code) rejected
  • Non-Error / null / primitive reasons handled gracefully (this also documents the predicate's behavior on the surface that ci: Bonk should fail faster #4 was about — non-Error rejections fall through to re-throw because the predicate returns undefined)
  • Install gate skips Vitest workers

Item #4 (non-Error unhandledRejection bypassing Node's unhandled-rejection diagnostics) was already noted in the bonk review as "minor; not actionable unless the surface matters in practice" — leaving as-is since it's the same behavior as the equivalent stock Node process.on("unhandledRejection", r => { throw r; }) pattern.

vp check passes (1 pre-existing benchmarks Cannot find module 'vinext' error unrelated to this PR, acknowledged in the PR description).

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Reviewing 8fa627a after the round-3 follow-up adding the focused unit test. Verified prior items end-to-end:

• run-prerender.ts:133-134, 274-275 — save/restore wrapper around VINEXT_PRERENDER matches the symmetric pattern used in prerender.ts. ✓
• index.ts:118-124 — install-site comment now accurately describes module-load install + Vitest gate + fire-time prerender bypass. ✓
• socket-error-backstop.ts:88-91 — peerDisconnectCode extracted as a pure exported predicate, no process-state mutation. ✓
• tests/socket-error-backstop.test.ts — 3 predicate tests + 1 gate-skip test, ~270ms. ✓

Nothing here blocks merge. A few small observations, all carry-overs or hygiene notes that don't justify another iteration on this PR:

1. Test coverage is narrower than the commit message claims

The commit message lists coverage as:

• ECONNRESET / EPIPE / ECONNABORTED matched
• Other codes (ENOENT, EHOSTUNREACH, no code) rejected
• Non-Error / null / primitive reasons handled gracefully
• Install gate skips Vitest workers

Strictly accurate for the predicate, but the listener behavior (absorb vs re-throw, VINEXT_PRERENDER bypass, debug log emission) isn't exercised. Idempotency-via-Symbol.for is also untested — the gate-skip test short-circuits before it reaches the Symbol.for guard, so the guard's de-dup behavior gets no coverage. Neither is realistically testable inside a Vitest worker without forking a child process, so the practical answer is "not worth it" — but the commit message could be honest that this is predicate coverage, not full listener coverage.

2. isSocketErrorBackstopInstalled is "test-only" but exported through the module's public surface

The JSDoc says "Not part of the public API." but it's a normal export. Anything that imports socket-error-backstop.ts can call it. Cosmetic — convention is to use _ prefix or push to a __test__ re-export — but the module isn't part of the package's public entry, so realistic blast radius is zero.

3. Gate-skip test is somewhat self-validating

expect(process.env.VITEST).toBe("true");
expect(isSocketErrorBackstopInstalled()).toBe(false);

If the gate ever regressed (e.g. VITEST !== "true" flipped), this test would still need someone to also import index.ts for the install path to be tripped, which the test file doesn't do. Today the test passes because installSocketErrorBackstop() is never called inside the worker — same outcome as a properly-gated install. To genuinely test the gate, the test would need to call installSocketErrorBackstop() directly and assert no listener was added. Trivial to strengthen:

const before = process.listenerCount("uncaughtException");
installSocketErrorBackstop();
expect(process.listenerCount("uncaughtException")).toBe(before);
expect(isSocketErrorBackstopInstalled()).toBe(false);

Worth doing in a follow-up; the assertion as-shipped passes vacuously if either gate or install is broken.

4. Carry-overs that remain unaddressed (acceptable)

• unhandledRejection with non-Error reasons bypasses Node's default unhandled-rejection diagnostics — author correctly noted this as minor and acknowledged in PR comments.
• No test for the VINEXT_PRERENDER re-throw path. Same forking-worker constraint as #1.

Bottom line

Merge-ready. The mechanism is solid, prior blockers are properly addressed, and the test coverage that was added is real coverage of the pure predicate. Items above are hygiene-only and don't justify another iteration on this PR.

[ask-bonk]

Posted the re-review on PR #916.

Summary of findings:

No genuine blockers remaining. All prior review items are properly addressed:

• ✅ run-prerender.ts save/restore wrapper for VINEXT_PRERENDER
• ✅ index.ts install-site comment accurately describes current behavior
• ✅ peerDisconnectCode extracted as pure exported predicate
• ✅ Unit tests added (4 tests, ~270ms)

Minor observations (none blocking):

1. Test coverage is narrower than commit message claims — covers the pure predicate but not the listener absorb/re-throw behavior or the VINEXT_PRERENDER bypass path. Worker-scoped tests can't realistically cover those without forking.
2. The gate-skip test is self-validating — passes vacuously since the test file doesn't import index.ts. Could be strengthened by directly calling installSocketErrorBackstop() and asserting no listener was added.
3. isSocketErrorBackstopInstalled JSDoc says "test-only" but it's a normal export — cosmetic.
4. Carry-overs (unhandledRejection non-Error surface, prerender bypass test) remain unaddressed but acknowledged.

PR is merge-ready.

github run