fix(startup): normalize non-Error throws + tolerate closed DB in instrumentation bootstrap (#6560) (#6622)

An update/restart could crash the whole server at boot with
TypeError: Cannot create property 'message' on string 'Database closed',
masking the real failure. driverFactory.ts's preInitSqlJs() cached its sql.js
WASM adapter per file path but never checked whether it had since been
closed by a racing gracefulShutdown/resetDbInstance; reusing the dead
handle made the next query throw sql.js's own raw string "Database closed"
straight out of instrumentation-node.ts's previously-unguarded
ensureDbInitialized() call. Next.js's registerInstrumentation() wrapper
unconditionally does err.message = ... on whatever register() rejects
with, and assigning .message on a primitive string throws in strict mode
-- that secondary TypeError is what actually crashed the process.

Fixed in two parts: preInitSqlJs() now evicts a closed cached adapter
instead of returning it, and a new ensureDbReadyForBoot() normalizes any
non-Error throw and retries once for a transient "database closed"
message before re-throwing anything else as a real Error.
This commit is contained in:
Diego Rodrigues de Sa e Souza
2026-07-08 00:18:33 -03:00
committed by GitHub
parent fb3892b52e
commit 48e902c9d0
4 changed files with 181 additions and 2 deletions

View File

@@ -33,6 +33,7 @@ _Living section — bullets land here as PRs merge into `release/v3.8.47` (paral
- **fix(resilience):** a bare, unrecognized `403` from a no-credential (`authType:"none"`) provider like mimocode or theoldllm no longer permanently bans the connection ([#6315](https://github.com/diegosouzapw/OmniRoute/issues/6315), [#6345](https://github.com/diegosouzapw/OmniRoute/issues/6345)) — `classifyProviderError()`'s 403 branch only exempted `apikey` providers from the terminal `FORBIDDEN` classification, so these free/stateless proxies (no real account/credential to revoke) fell through to `FORBIDDEN` on the first unmatched 403 and got `isActive:false, testStatus:"banned"` with no cooldown or retry. The exemption now also covers `authType:"none"` providers, returning `null` (recoverable) so the existing connection-cooldown/retry layer handles it. Regression guard: `tests/unit/errorClassifier-noauth-403-6315.test.ts`.
- **fix(providers):** the **Auggie (Augment CLI)** executor no longer fails on Windows with `spawn EINVAL` ([#6304](https://github.com/diegosouzapw/OmniRoute/issues/6304)) — the global-npm install exposes `auggie` as a `.cmd` shim, which Node's `child_process.spawn` cannot launch on win32 without `shell: true`. Both spawn sites (streaming + the `auggie --version` test) now go through a shared `buildAuggieSpawnOptions()` that sets `shell: process.platform === "win32"`; the argv (built by `buildAuggieArgs()` with a registry-validated `model` and a trailing `--` end-of-options marker) is unchanged, so the argument-injection surface stays closed on non-Windows. Regression guard: `tests/unit/auggie-win32-spawn-6304.test.ts`.
- **fix(api):** the dashboard **"Test model"** action is now a clean connection test ([#6240](https://github.com/diegosouzapw/OmniRoute/issues/6240)) — `modelTestRunner` sent its probe request without an explicit compression override, so whenever the operator's global `compression.enabled` flag was on the test call inherited compression (and any Output-Styles system prompt), polluting the result. The internal test requests now send `X-OmniRoute-Compression: off`, and `chatCore` honors an explicit `off` header even when `compression.enabled` is globally true. Regression guards: `tests/unit/model-test-runner-compression-off-6240.test.ts`, `tests/integration/test-model-compression-off-6240.test.ts`.
- **fix(startup):** an update/restart could crash the whole server at boot with `TypeError: Cannot create property 'message' on string 'Database closed'`, masking the real failure and 500-ing every request until manually restarted ([#6560](https://github.com/diegosouzapw/OmniRoute/issues/6560), plausibly the root cause of #6594's post-upgrade 500) — `driverFactory.ts::preInitSqlJs()` cached its sql.js WASM adapter per file path in a `globalThis`-backed map for idempotency, but never checked whether the cached adapter had since been closed (e.g. by `gracefulShutdown`/`resetDbInstance` racing a reload); reusing that dead handle made the very next query throw sql.js's own bare string `"Database closed"` (not an `Error`) straight out of `instrumentation-node.ts`'s previously-unguarded `ensureDbInitialized()` call, and Next.js's internal `registerInstrumentation()` wrapper unconditionally does `err.message = ...` on whatever `register()` rejects with — assigning `.message` on a primitive string throws in strict mode, so the secondary `TypeError` is what actually crashed the process. Fixed in two parts: `preInitSqlJs()` now evicts a closed cached adapter and creates a fresh one instead of returning it; a new `ensureDbReadyForBoot()` wraps the DB-init call, normalizes any non-Error throw via `normalizeBootError()`, and retries once specifically for a transient "database closed" message (now succeeding against the fresh adapter) before re-throwing anything else as a real `Error`. Regression guard: `tests/unit/instrumentation-database-closed-6560.test.ts`.
### 📝 Maintenance

View File

@@ -35,6 +35,67 @@ export function renameProcessTitle(currentTitle: string): string {
return `omniroute${currentTitle.slice("next-server".length)}`;
}
/**
* Normalize any thrown/rejected value into a real `Error` instance.
*
* Next.js's own `registerInstrumentation()` wrapper (see
* `node_modules/next/dist/server/lib/router-utils/instrumentation-globals.external.js`)
* unconditionally does `err.message = \`...${err.message}\`` on whatever our
* `register()` export rejects with, assuming it is always an `Error`. If a raw
* non-Error primitive bubbles up instead (e.g. sql.js's WASM adapter throws the
* bare string `"Database closed"` — see `./lib/db/adapters/sqljsAdapter.ts`),
* that assignment throws `TypeError: Cannot create property 'message' on
* string '...'` in strict mode, masking the original error and crashing the
* whole server on every boot (#6560). Normalizing before it leaves our code
* guarantees Next always receives something `.message`-assignable.
*/
export function normalizeBootError(err: unknown): Error {
return err instanceof Error ? err : new Error(String(err));
}
// Matches sql.js's raw `throw "Database closed"` (and similarly-worded
// variants) thrown when a query runs against an already-closed WASM handle —
// typically a stale globalThis-cached adapter left over by a prior
// close/reload racing with this boot (#6560).
const TRANSIENT_DB_CLOSED_RE = /database\s*(connection\s*)?(is\s*)?closed/i;
/**
* Initialize the SQLite singleton for boot, tolerating one transient
* "database closed" failure (#6560) by retrying once — the driverFactory
* cache-eviction fix (`preInitSqlJs`) makes the retry create a fresh adapter
* instead of reusing the dead one. Any other failure (or a second consecutive
* "database closed") is re-thrown as a real `Error` via `normalizeBootError`
* so it can never crash instrumentation with a masking TypeError — the caller
* (`registerNodejs`) still surfaces it as a real boot failure.
*
* `ensureDbInitializedFn` is only for tests to inject a fake without
* module-mocking (`node:test` does not support `mock.module` reliably here).
*/
export async function ensureDbReadyForBoot(
ensureDbInitializedFn?: () => Promise<void>
): Promise<void> {
const ensureDbInitialized =
ensureDbInitializedFn ?? (await import("@/lib/db/core")).ensureDbInitialized;
try {
await ensureDbInitialized();
} catch (err: unknown) {
const normalized = normalizeBootError(err);
if (!TRANSIENT_DB_CLOSED_RE.test(normalized.message)) {
throw normalized;
}
console.warn(
"[STARTUP] Database was closed by a prior reload/shutdown — retrying with a fresh connection (#6560):",
normalized.message
);
try {
await ensureDbInitialized();
} catch (retryErr: unknown) {
throw normalizeBootError(retryErr);
}
}
}
function isBackgroundServicesDisabled(): boolean {
const raw = process.env.OMNIROUTE_DISABLE_BACKGROUND_SERVICES;
if (!raw) return false;
@@ -271,7 +332,7 @@ export async function registerNodejs(): Promise<void> {
console.warn("[COMPLIANCE] Could not initialize audit log:", msg);
}
await import("@/lib/db/core").then(({ ensureDbInitialized }) => ensureDbInitialized());
await ensureDbReadyForBoot();
// Storage-configured scheduled VACUUM (#4437): registers the timer from
// Settings > System & Storage and persists lastVacuumAt for the UI.

View File

@@ -64,7 +64,16 @@ export function tryOpenSync(
export async function preInitSqlJs(filePath: string): Promise<SqliteAdapter> {
const cache = getSqlJsCache();
const existing = cache.get(filePath);
if (existing) return existing;
if (existing) {
if (existing.open) return existing;
// Stale handle left over by a prior close/reload (e.g. gracefulShutdown or
// resetDbInstance closed the underlying WASM db but this globalThis-backed
// cache — deliberately shared across re-invocations for idempotency — still
// holds the reference). Reusing it would make every subsequent query throw
// the raw string "Database closed" straight from sql.js (#6560). Evict and
// recreate instead of returning a dead connection.
cache.delete(filePath);
}
const { createSqlJsAdapter } = await import("./sqljsAdapter");
const adapter = await createSqlJsAdapter(filePath);

View File

@@ -0,0 +1,108 @@
import test from "node:test";
import assert from "node:assert/strict";
import {
normalizeBootError,
ensureDbReadyForBoot,
} from "../../src/instrumentation-node";
// Regression guard for #6560: during an update/restart, sql.js's WASM adapter
// can throw the bare string `"Database closed"` (not an Error instance) when a
// stale, already-closed handle is reused. Next.js's own
// `registerInstrumentation()` wrapper unconditionally does
// `err.message = ...` on whatever `register()` rejects with — if that value is
// a primitive string, the assignment itself throws
// `TypeError: Cannot create property 'message' on string 'Database closed'`,
// masking the real error and crashing the whole server on every boot.
test("normalizeBootError wraps a raw thrown string into a real Error", () => {
const normalized = normalizeBootError("Database closed");
assert.ok(normalized instanceof Error, "must be a real Error instance");
assert.equal(normalized.message, "Database closed");
// The exact crash from #6560: assigning `.message` on the original raw
// string throws; on the normalized Error it must be a no-op assignment.
assert.throws(() => {
// @ts-expect-error - deliberately mutating a primitive to reproduce the bug
("Database closed").message = "mutated";
}, TypeError);
assert.doesNotThrow(() => {
normalized.message = `An error occurred while loading instrumentation hook: ${normalized.message}`;
});
assert.equal(
normalized.message,
"An error occurred while loading instrumentation hook: Database closed"
);
});
test("normalizeBootError passes real Error instances through unchanged (identity)", () => {
const original = new Error("boom");
const normalized = normalizeBootError(original);
assert.equal(normalized, original);
});
test("normalizeBootError wraps non-string, non-Error throws (null/undefined/object)", () => {
assert.equal(normalizeBootError(null).message, "null");
assert.equal(normalizeBootError(undefined).message, "undefined");
assert.equal(normalizeBootError({ code: "X" }).message, "[object Object]");
});
test("ensureDbReadyForBoot retries once and succeeds when the first attempt throws the raw 'Database closed' string (RED before fix, GREEN after)", async () => {
let calls = 0;
const fakeEnsureDbInitialized = async (): Promise<void> => {
calls += 1;
if (calls === 1) {
throw "Database closed";
}
// second attempt (post-retry) succeeds, as it would once the caller gets a
// fresh, non-stale adapter.
};
await assert.doesNotReject(ensureDbReadyForBoot(fakeEnsureDbInitialized));
assert.equal(calls, 2, "must retry exactly once after a transient 'Database closed' failure");
});
test("ensureDbReadyForBoot re-throws (normalized) when the retry also fails", async () => {
let calls = 0;
const fakeEnsureDbInitialized = async (): Promise<void> => {
calls += 1;
throw "Database closed";
};
await assert.rejects(
ensureDbReadyForBoot(fakeEnsureDbInitialized),
(err: unknown) => {
assert.ok(err instanceof Error, "rejection must be a real Error, never a raw string");
assert.equal((err as Error).message, "Database closed");
return true;
}
);
assert.equal(calls, 2);
});
test("ensureDbReadyForBoot does not retry and re-throws (normalized) unrelated failures", async () => {
let calls = 0;
const fakeEnsureDbInitialized = async (): Promise<void> => {
calls += 1;
throw new Error("disk full");
};
await assert.rejects(
ensureDbReadyForBoot(fakeEnsureDbInitialized),
(err: unknown) => {
assert.ok(err instanceof Error);
assert.equal((err as Error).message, "disk full");
return true;
}
);
assert.equal(calls, 1, "unrelated errors must not trigger the closed-DB retry");
});
test("ensureDbReadyForBoot resolves without retrying when there is no failure", async () => {
let calls = 0;
const fakeEnsureDbInitialized = async (): Promise<void> => {
calls += 1;
};
await assert.doesNotReject(ensureDbReadyForBoot(fakeEnsureDbInitialized));
assert.equal(calls, 1);
});