From 19f5bd0a1882e02f80f1e7efefc72199391897c6 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Fri, 14 Aug 2026 10:10:33 +0200 Subject: [PATCH 1/2] fix(bundler): keep the failure that caused the ownership error Staging preview has been returning 500 with [ext-bundler-esbuild] Cannot own an esbuild service started outside the module-wide adapter and the message is all there is: whatever actually failed is discarded before anyone can read it. `invokeEsbuild` records the latch up front, with no cause: const ownershipError = recordOwnershipError(); // latch, no cause return result.then( () => { throw ownershipError; }, (cause) => { throw recordOwnershipError(cause); } // ??= -> cause dropped ); Because the latch is set on the line above, the `??=` in `recordOwnershipError` returns the existing causeless error and throws the real one away. The latch is permanent, so every later operation in the process reports a lifecycle problem that may not be what went wrong, and the underlying failure is never visible anywhere. Record the latch only once the operation settles, and let a cause arriving later attach to an error created without one. Fold the cause into the message too: callers log `error.message`, so a `cause` chain alone would still not be printed. This does not fix the staging failure. It makes it possible to see it. --- .../src/esbuild-bundler.test.ts | 53 ++++++++++++++++++- .../src/esbuild-bundler.ts | 53 ++++++++++++++++--- 2 files changed, 98 insertions(+), 8 deletions(-) diff --git a/extensions/ext-bundler-esbuild/src/esbuild-bundler.test.ts b/extensions/ext-bundler-esbuild/src/esbuild-bundler.test.ts index d827d9e52b..c554c43f7e 100644 --- a/extensions/ext-bundler-esbuild/src/esbuild-bundler.test.ts +++ b/extensions/ext-bundler-esbuild/src/esbuild-bundler.test.ts @@ -6,10 +6,15 @@ */ import { assertEquals, assertExists, assertRejects, assertStringIncludes } from "@std/assert"; -import { describe, it } from "@std/testing/bdd"; +import { afterEach, describe, it } from "@std/testing/bdd"; import { createRequire } from "node:module"; -import { EsbuildBundler, isLiveEsbuildServiceProcess } from "./esbuild-bundler.ts"; +import { + __recordOwnershipErrorForTests, + __resetOwnershipErrorForTests, + EsbuildBundler, + isLiveEsbuildServiceProcess, +} from "./esbuild-bundler.ts"; import { rebuildContextWithSignal } from "./context-build-lifecycle.ts"; const childProcess = createRequire(import.meta.url)("node:child_process") as { @@ -811,6 +816,50 @@ describe("EsbuildBundler.bundle", () => { }); }); +describe("ownership error cause", () => { + afterEach(() => { + __resetOwnershipErrorForTests(); + }); + + it("adopts the underlying failure when the latch was set before it surfaced", () => { + // The latch is created before the operation settles, so it starts without a + // cause. Discarding the cause that arrives afterwards is what made every + // real esbuild failure surface as a lifecycle problem instead. + __resetOwnershipErrorForTests(); + __recordOwnershipErrorForTests(); + const error = __recordOwnershipErrorForTests(new Error("spawn ENOENT esbuild")); + + assertStringIncludes(error.message, "module-wide adapter"); + assertStringIncludes(error.message, "spawn ENOENT esbuild"); + assertEquals((error.cause as Error).message, "spawn ENOENT esbuild"); + }); + + it("reports a cause supplied on the first record", () => { + __resetOwnershipErrorForTests(); + const error = __recordOwnershipErrorForTests(new Error("binary missing")); + + assertStringIncludes(error.message, "binary missing"); + assertEquals((error.cause as Error).message, "binary missing"); + }); + + it("keeps the first cause rather than overwriting it", () => { + __resetOwnershipErrorForTests(); + __recordOwnershipErrorForTests(new Error("first failure")); + const error = __recordOwnershipErrorForTests(new Error("second failure")); + + assertEquals((error.cause as Error).message, "first failure"); + assertStringIncludes(error.message, "first failure"); + }); + + it("stays usable when there is no underlying failure", () => { + __resetOwnershipErrorForTests(); + const error = __recordOwnershipErrorForTests(); + + assertStringIncludes(error.message, "module-wide adapter"); + assertEquals(error.cause, undefined); + }); +}); + describe("EsbuildBundler unsupported lifecycle ownership", () => { it("rejects shutdown after a raw service generation replaces the managed one", async () => { const observation = observeEsbuildServices(); diff --git a/extensions/ext-bundler-esbuild/src/esbuild-bundler.ts b/extensions/ext-bundler-esbuild/src/esbuild-bundler.ts index 1092a02dca..508c55fcf5 100644 --- a/extensions/ext-bundler-esbuild/src/esbuild-bundler.ts +++ b/extensions/ext-bundler-esbuild/src/esbuild-bundler.ts @@ -63,11 +63,40 @@ let activeOperationsIdle: Promise = Promise.resolve(); let resolveActiveOperationsIdle: (() => void) | null = null; const operationScopes = new AsyncLocalStorage(); +const OWNERSHIP_ERROR_MESSAGE = + "[ext-bundler-esbuild] Cannot own an esbuild service started outside the module-wide adapter; restart the process and use only the Bundler contract"; + +function describeCause(cause: unknown): string { + if (cause === undefined) return ""; + const detail = cause instanceof Error ? cause.message : String(cause); + return detail ? ` (underlying failure: ${detail})` : ""; +} + +/** + * Latch the ownership failure, keeping whatever really went wrong. + * + * The latch is permanent, so the first call decides the error every later + * operation sees. That call is often made before the operation settles, when no + * cause is known yet; adopting the cause afterwards is what keeps the real + * failure visible instead of reporting a lifecycle problem that may not be the + * actual one. The cause is folded into the message because callers log + * `error.message` and would otherwise never print the chain. + */ function recordOwnershipError(cause?: unknown): Error { - const message = - "[ext-bundler-esbuild] Cannot own an esbuild service started outside the module-wide adapter; restart the process and use only the Bundler contract"; - esbuildOwnershipError ??= new Error(message, cause === undefined ? undefined : { cause }); - return esbuildOwnershipError; + const existing = esbuildOwnershipError; + if (!existing) { + esbuildOwnershipError = new Error( + `${OWNERSHIP_ERROR_MESSAGE}${describeCause(cause)}`, + cause === undefined ? undefined : { cause }, + ); + return esbuildOwnershipError; + } + + if (cause !== undefined && existing.cause === undefined) { + existing.cause = cause; + existing.message = `${OWNERSHIP_ERROR_MESSAGE}${describeCause(cause)}`; + } + return existing; } async function getEsbuild(): Promise { @@ -220,6 +249,16 @@ function isEsbuildServiceSpawn(spawnArgs: unknown[]): boolean { * Native Node represents an active child with `null` exit fields. Some * compatible runtimes leave those fields undefined until the child exits. */ +/** The ownership latch is module-wide; tests must clear it between cases. */ +export function __resetOwnershipErrorForTests(): void { + esbuildOwnershipError = null; +} + +/** Exercise the latch without starting a real esbuild service. */ +export function __recordOwnershipErrorForTests(cause?: unknown): Error { + return recordOwnershipError(cause); +} + export function isLiveEsbuildServiceProcess( child: Pick, ): boolean { @@ -270,10 +309,12 @@ function invokeEsbuild>(operation: () => T): T { const ownedService = capturedService ?? esbuildService; if (!ownedService || !isLiveService(ownedService)) { - const ownershipError = recordOwnershipError(); + // Latch only once the operation settles. Recording up front would fix a + // causeless error in place and the rejection below could no longer attach + // what actually failed. return result.then( () => { - throw ownershipError; + throw recordOwnershipError(); }, (cause) => { throw recordOwnershipError(cause); From d31c5952f9cd45a09ac25a59ead360154de1a228 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Fri, 14 Aug 2026 11:57:42 +0200 Subject: [PATCH 2/2] fix(bundler): latch synchronously and redact the reported cause Review follow-ups. Deferring the latch until the operation settled left a window where a concurrent transform passed the admission check in runBundlerOperation and drove esbuild while ownership was already known to be invalid. Latch synchronously again; the cause still arrives, because the latch is created without one and recordOwnershipError adopts the first cause offered afterwards. The cause is folded into a message that callers log, so it must not carry a machine's filesystem layout: a compiled runtime resolves esbuild under a temp directory and spawn errors quote that path verbatim. Reduce absolute paths to their basename, keep only the first line so a stack never reaches the message, and bound the length. spawn /tmp/veryfront-esbuild-0.28.1-c3fd/esbuild ENOENT -> spawn esbuild ENOENT Also import the BDD helpers from the repo module rather than @std. --- .../src/esbuild-bundler.test.ts | 26 +++++++++++++++- .../src/esbuild-bundler.ts | 31 ++++++++++++++++--- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/extensions/ext-bundler-esbuild/src/esbuild-bundler.test.ts b/extensions/ext-bundler-esbuild/src/esbuild-bundler.test.ts index c554c43f7e..26071e440a 100644 --- a/extensions/ext-bundler-esbuild/src/esbuild-bundler.test.ts +++ b/extensions/ext-bundler-esbuild/src/esbuild-bundler.test.ts @@ -6,7 +6,7 @@ */ import { assertEquals, assertExists, assertRejects, assertStringIncludes } from "@std/assert"; -import { afterEach, describe, it } from "@std/testing/bdd"; +import { afterEach, describe, it } from "#veryfront/testing/bdd.ts"; import { createRequire } from "node:module"; import { @@ -851,6 +851,30 @@ describe("ownership error cause", () => { assertStringIncludes(error.message, "first failure"); }); + it("keeps the filesystem layout out of the reported cause", () => { + // A compiled runtime resolves esbuild under a temp directory, and spawn + // errors quote that path. The message is logged, so it must carry the + // failure without the machine's layout. + __resetOwnershipErrorForTests(); + const error = __recordOwnershipErrorForTests( + new Error("spawn /tmp/veryfront-esbuild-0.28.1-c3fd/esbuild ENOENT"), + ); + + assertStringIncludes(error.message, "spawn esbuild ENOENT"); + assertEquals(error.message.includes("/tmp/"), false); + }); + + it("keeps a stack out of the reported cause", () => { + __resetOwnershipErrorForTests(); + const error = __recordOwnershipErrorForTests( + new Error("boom\n at Object.create (file:///tmp/deno-compile/src/errors/types.ts:111:14)"), + ); + + assertStringIncludes(error.message, "boom"); + assertEquals(error.message.includes("types.ts"), false); + assertEquals(error.message.includes(" at "), false); + }); + it("stays usable when there is no underlying failure", () => { __resetOwnershipErrorForTests(); const error = __recordOwnershipErrorForTests(); diff --git a/extensions/ext-bundler-esbuild/src/esbuild-bundler.ts b/extensions/ext-bundler-esbuild/src/esbuild-bundler.ts index 508c55fcf5..152e065fb3 100644 --- a/extensions/ext-bundler-esbuild/src/esbuild-bundler.ts +++ b/extensions/ext-bundler-esbuild/src/esbuild-bundler.ts @@ -66,9 +66,29 @@ const operationScopes = new AsyncLocalStorage(); const OWNERSHIP_ERROR_MESSAGE = "[ext-bundler-esbuild] Cannot own an esbuild service started outside the module-wide adapter; restart the process and use only the Bundler contract"; +const MAX_CAUSE_DETAIL_LENGTH = 200; +/** Absolute POSIX and Windows paths, reduced to a basename below. */ +const ABSOLUTE_PATH_PATTERN = /(?:[A-Za-z]:)?(?:\/|\\\\)[^\s"']*/g; + +/** + * Reduce a cause to a single redacted line. + * + * The message is logged, so it must not carry a machine's filesystem layout: + * a compiled runtime resolves esbuild under a temp directory, and spawn errors + * quote that path verbatim. Keeping only the basename preserves what the reader + * needs -- which binary or module failed -- without the surrounding layout. The + * first line only, so a stack never reaches the message, and bounded so a large + * esbuild diagnostic cannot dominate the log line. + */ function describeCause(cause: unknown): string { if (cause === undefined) return ""; - const detail = cause instanceof Error ? cause.message : String(cause); + const raw = cause instanceof Error ? cause.message : String(cause); + const firstLine = raw.split("\n", 1)[0] ?? ""; + const withoutPaths = firstLine.replace(ABSOLUTE_PATH_PATTERN, (match) => { + const parts = match.split(/[\/\\]/).filter(Boolean); + return parts.length > 0 ? parts[parts.length - 1]! : match; + }); + const detail = withoutPaths.trim().slice(0, MAX_CAUSE_DETAIL_LENGTH); return detail ? ` (underlying failure: ${detail})` : ""; } @@ -309,9 +329,12 @@ function invokeEsbuild>(operation: () => T): T { const ownedService = capturedService ?? esbuildService; if (!ownedService || !isLiveService(ownedService)) { - // Latch only once the operation settles. Recording up front would fix a - // causeless error in place and the rejection below could no longer attach - // what actually failed. + // Latch synchronously so a concurrent operation cannot pass the admission + // check in runBundlerOperation and drive esbuild while ownership is already + // known to be invalid. The rejection handler still supplies the cause: the + // latch is created without one here, and recordOwnershipError adopts the + // first cause offered afterwards. + recordOwnershipError(); return result.then( () => { throw recordOwnershipError();