refactor: extract instrumentation lazy init to server/instrumentation-runtime.ts

[NathanDrake2406]

Testing Kimi2.6

Summary

Extracts the 25-line __ensureInstrumentation block from the generated App Router RSC entry into a normal, typed, unit-testable module: server/instrumentation-runtime.ts.

Principle: Codegen should describe the app shape; normal modules should implement behavior.

What changed

Before

entries/app-rsc-entry.ts emitted ~25 lines of inline generated code for every App Router project that uses instrumentation.ts:

• Module-level mutable state (__instrumentationInitialized, __instrumentationInitPromise)
• An idempotent async function __ensureInstrumentation()
• register() and onRequestError wiring
• VINEXT_PRERENDER short-circuit

This is the canonical "imperative shell with mutable bookkeeping" pattern. Embedding it in a template string makes it untestable, hard to review, and easy to drift between dev/prod paths.

After

1. New module: packages/vinext/src/server/instrumentation-runtime.ts exports ensureInstrumentationRegistered(instrumentationModule).
2. Generated entry: imports the helper and calls await ensureInstrumentationRegistered(_instrumentation) — two lines instead of twenty-five.
3. Tests: Added focused unit tests for idempotency, concurrent racing, prerender skipping, and graceful handling of modules without register/onRequestError.

Why this matters

• Testability: The lazy-init semantics (shared promise, dedup, env-gate) are now covered by Vitest unit tests instead of only by integration tests.
• Entry thinness: The generated RSC entry stays focused on route-specific imports, manifests, and thin closures — exactly what codegen should own.
• Dev/prod parity: Since both dev and prod use the same ensureInstrumentationRegistered import, fixes or additions to instrumentation behavior automatically apply everywhere.

Next.js references

Next.js supports instrumentation.ts via a register() hook called once at server startup, plus an optional onRequestError() handler. The semantics we preserve:

• Lazy init on first request (not top-level await) to avoid blocking V8 isolate startup — see how Next.js defers instrumentation registration in the dev server:
  • packages/next/src/server/dev/next-dev-server.ts
• Global wiring for onRequestError so the handler is reachable across module graphs — Next.js stores the hook on a shared instrumentation instance:
  • packages/next/src/server/lib/instrumentation/internal-lifecycle.ts
• Official docs:
  • https://nextjs.org/docs/app/building-your-application/optimizing/instrumentation

Test plan

# New unit tests for the extracted helper
pnpm test tests/instrumentation.test.ts

# Snapshot update for generated entry templates
pnpm test tests/entry-templates.test.ts

# App Router integration (covers instrumentation in dev + prod)
pnpm test tests/app-router.test.ts

All pass locally.

[pkg-pr-new]

Open in StackBlitz

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

commit: 40384c9

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[james-elicx]

/bigbonk review

[ask-bonk]

Good refactor — the extraction is clean and the tests cover the right cases. The generated entry is meaningfully thinner now and the behavioral semantics are preserved exactly. Two issues worth addressing before merge:

1. Bug: register() rejection poisons the shared promise permanently. If register() throws, the rejected promise is cached in initPromise and initialized never becomes true. Every subsequent call returns the same rejected promise — the handler is permanently broken with no recovery path. The original inline code had the same bug, but now that this is a proper module with tests, it's worth fixing. Either catch inside the IIFE and still set initialized = true (so onRequestError still gets wired), or null out initPromise on failure to allow retry.

2. Minor: isOnRequestErrorHandler type guard is unnecessary indirection. It's a single-use function that just wraps typeof value === "function" — the same check used for register two lines above. The asymmetry makes the reader wonder if there's a deeper validation happening. Inlining it (with a cast) would be more consistent.

[ask-bonk]

If register() rejects, the rejected promise is stored in initPromise and initialized never flips to true. Every subsequent call to ensureInstrumentationRegistered returns the cached rejected promise — the handler is permanently poisoned with no recovery.

The original inline code had the same flaw, but since this refactor is explicitly about making the behavior testable and correct, it's worth fixing here. Two options:

Option A — fail open, still wire onRequestError:

Suggested change

	initPromise = (async () => {
	if (typeof instrumentationModule.register === "function") {
	await instrumentationModule.register();
	}
	// Store the onRequestError handler on globalThis so it is visible to
	// reportRequestError() regardless of which Vite environment module graph
	// it is called from. With @vitejs/plugin-rsc the RSC and SSR environments
	// run in the same Node.js process and share globalThis. With
	// @cloudflare/vite-plugin everything runs inside the Worker so globalThis
	// is the Worker's global — also correct.
	if (isOnRequestErrorHandler(instrumentationModule.onRequestError)) {
	globalThis.__VINEXT_onRequestErrorHandler__ = instrumentationModule.onRequestError;
	}
	initialized = true;
	})();
	initPromise = (async () => {
	try {
	if (typeof instrumentationModule.register === "function") {
	await instrumentationModule.register();
	}
	} catch (err) {
	console.error(
	"[vinext] instrumentation.register() threw:",
	err instanceof Error ? err.message : String(err),
	);
	}
	// Store the onRequestError handler on globalThis so it is visible to
	// reportRequestError() regardless of which Vite environment module graph
	// it is called from. With @vitejs/plugin-rsc the RSC and SSR environments
	// run in the same Node.js process and share globalThis. With
	// @cloudflare/vite-plugin everything runs inside the Worker so globalThis
	// is the Worker's global — also correct.
	if (isOnRequestErrorHandler(instrumentationModule.onRequestError)) {
	globalThis.__VINEXT_onRequestErrorHandler__ = instrumentationModule.onRequestError;
	}
	initialized = true;
	})();

This matches how runInstrumentation (Pages Router path) already handles errors — it catches and logs, then continues. Keeps dev/prod parity.

Option B — allow retry by clearing initPromise on failure. Less preferred because it could cause register() side effects to run multiple times on a flaky init.

Either way, a test for register() rejection behavior would be good to add.

[ask-bonk]

Nit: this type guard is a single-use wrapper around typeof value === "function" — the exact same check used for register on line 58. The asymmetry makes the reader pause to check if there's deeper validation. Consider inlining it:

Suggested change

	function isOnRequestErrorHandler(value: unknown): value is OnRequestErrorHandler {
	return typeof value === "function";
	}

and on line 68:

if (typeof instrumentationModule.onRequestError === "function") {
  globalThis.__VINEXT_onRequestErrorHandler__ = instrumentationModule.onRequestError as OnRequestErrorHandler;
}

Or alternatively, if you prefer the type guard style, use it for both checks (register and onRequestError) so the pattern is consistent.

[ask-bonk]

Missing test: what happens when register() throws? Currently, a rejection from register() poisons initPromise permanently — every subsequent call re-throws the same error. Worth adding a test that verifies the desired behavior (whether that's fail-open, retry, or intentional propagation) so the contract is explicit.

Something like:

it("handles register() rejection gracefully", async () => {
  const register = vi.fn().mockRejectedValue(new Error("init failed"));
  const onRequestError = vi.fn();

  // Should not throw (or should throw — depends on desired semantics)
  await ensureInstrumentationRegistered({ register, onRequestError });

  // onRequestError should still be wired despite register() failure
  await reportRequestError(new Error("boom"), sampleRequest, sampleContext);
  expect(onRequestError).toHaveBeenCalledOnce();
});

[ask-bonk]

Review posted. Here's a summary of the feedback:

Overall: Clean refactor that correctly extracts instrumentation lazy-init from codegen into a testable module. The generated entry is meaningfully thinner and the behavioral semantics are preserved.

Key findings:

1. Bug — register() rejection poisons the shared promise permanently. If register() throws, initPromise caches the rejected promise and initialized never flips to true. Every subsequent request re-throws. The fix is to add a try/catch inside the IIFE (matching how runInstrumentation in the Pages Router path already handles errors), and a test for this case.

2. Nit — isOnRequestErrorHandler type guard is unnecessary indirection. It wraps a bare typeof === "function" check but register uses the raw typeof check directly. The asymmetry is distracting. Suggested either inlining it or using the guard for both checks consistently.

3. Missing test — register() rejection behavior. Added a suggestion for a test that exercises the error path and verifies onRequestError still gets wired.

github run

[james-elicx]

Pre-existing bug - will merge refactor.