diff --git a/CHANGELOG.md b/CHANGELOG.md index 4152548f12..746f98aab4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/src/instrumentation-node.ts b/src/instrumentation-node.ts index ad132b51f8..20d96240a4 100755 --- a/src/instrumentation-node.ts +++ b/src/instrumentation-node.ts @@ -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 +): Promise { + 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 { 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. diff --git a/src/lib/db/adapters/driverFactory.ts b/src/lib/db/adapters/driverFactory.ts index 4975714e02..df0c9e0856 100644 --- a/src/lib/db/adapters/driverFactory.ts +++ b/src/lib/db/adapters/driverFactory.ts @@ -64,7 +64,16 @@ export function tryOpenSync( export async function preInitSqlJs(filePath: string): Promise { 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); diff --git a/tests/unit/instrumentation-database-closed-6560.test.ts b/tests/unit/instrumentation-database-closed-6560.test.ts new file mode 100644 index 0000000000..a693dd361c --- /dev/null +++ b/tests/unit/instrumentation-database-closed-6560.test.ts @@ -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 => { + 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 => { + 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 => { + 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 => { + calls += 1; + }; + + await assert.doesNotReject(ensureDbReadyForBoot(fakeEnsureDbInitialized)); + assert.equal(calls, 1); +});