From c07cebbab72106f6d31c259491ff03e6feb954e4 Mon Sep 17 00:00:00 2001 From: voidstack <143511464+voidstackloop@users.noreply.github.com> Date: Fri, 18 Sep 2026 18:00:16 +0300 Subject: [PATCH] fix(db): close failed initialization connections (#13342) * fix(db): close failed initialization connections * docs: add changelog fragment for #13303 db handle-leak fix --------- Co-authored-by: voidstackloop Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --- .../fixes/13342-db-init-handle-leak.md | 1 + src/lib/db/core.ts | 210 ++++++++++-------- 2 files changed, 117 insertions(+), 94 deletions(-) create mode 100644 changelog.d/fixes/13342-db-init-handle-leak.md diff --git a/changelog.d/fixes/13342-db-init-handle-leak.md b/changelog.d/fixes/13342-db-init-handle-leak.md new file mode 100644 index 0000000000..0029e56806 --- /dev/null +++ b/changelog.d/fixes/13342-db-init-handle-leak.md @@ -0,0 +1 @@ +- **fix(db):** `getDbInstance()` now closes the probe and primary SQLite connections on every failed initialization path, not just the happy path, fixing a handle leak that caused `EPERM` on Windows teardown. ([#13303](https://github.com/diegosouzapw/OmniRoute/issues/13303)) diff --git a/src/lib/db/core.ts b/src/lib/db/core.ts index 4c4fdafbfa..769b3d2ad2 100644 --- a/src/lib/db/core.ts +++ b/src/lib/db/core.ts @@ -1088,8 +1088,9 @@ export function getDbInstance(): SqliteDatabase { // Detect and handle old schema format — preserve data when possible (#146) // Uses a single probe connection that becomes the real connection when possible. if (fs.existsSync(sqliteFile)) { + let probe: SqliteDatabase | null = null; try { - const probe = openSqliteDatabase(sqliteFile, { readonly: true }); + probe = openSqliteDatabase(sqliteFile, { readonly: true }); // #9934: init asymmetry — bin/cli/sqlite.mjs::openOmniRouteDb (used by // `omniroute setup`) creates storage.sqlite with only the partial inline // schema (key_value + provider_connections) and never runs migrations. @@ -1156,6 +1157,12 @@ export function getDbInstance(): SqliteDatabase { closeProbeIfSafe(probe); } } catch (e: unknown) { + try { + closeProbeIfSafe(probe); + probe = null; + } catch { + /* ignore */ + } const message = e instanceof Error ? e.message : String(e); console.warn("[DB] Could not probe existing DB:", message); @@ -1214,6 +1221,12 @@ export function getDbInstance(): SqliteDatabase { /* ok */ } } + } finally { + try { + closeProbeIfSafe(probe); + } catch { + /* ignore */ + } } } @@ -1234,40 +1247,41 @@ export function getDbInstance(): SqliteDatabase { } const db = openSqliteDatabase(sqliteFile); - // Emit the same "[DB] Driver: ..." line openDatabaseAsync() prints so the - // packaged-app smoke guard (#7592) can assert the native driver was - // selected on the server's primary DB path too, not only the backup-import - // route. - console.log(`[DB] Driver: ${db.driver} | file: ${sqliteFile}`); - // better-sqlite3 is synchronous, so a contended write parks the Node event loop for up to - // busy_timeout ms (a 0-CPU freeze that stacks under load → /health stops responding). The - // hot-path writers here (usage_history, call_logs) are best-effort and the WinUI host opens - // the same DB, so cap the block at 2s instead of 5s: normal writes complete in <1ms, and a - // contended op can no longer freeze the loop past the host watchdog's 6s liveness probe. - // - // Install the busy handler before the connection's first statement. `journal_mode = WAL` - // needs a SHARED lock, and another process closing its WAL connection briefly holds the - // file EXCLUSIVE (checkpoint + WAL delete); node:sqlite opens with busy timeout 0, so with - // the pragmas in the other order that window surfaced as `database is locked` at startup. - db.pragma("busy_timeout = 2000"); - db.pragma("journal_mode = WAL"); - db.pragma("synchronous = NORMAL"); - db.pragma(`cache_size = -${DEFAULT_DATABASE_SETTINGS.optimization.cacheSize}`); - db.pragma("temp_store = MEMORY"); - // Tables first, then the legacy-column healers, and only then the indexes: an upgraded - // database can already own call_logs/usage_history/provider_connections with an older - // column set, where the CREATE TABLE is a no-op but the indexes still reference columns - // the healers below are the ones adding. - db.exec(SCHEMA_TABLES_SQL); - ensureProviderConnectionsColumns(db); - ensureUsageHistoryColumns(db); - ensureCallLogsColumns(db); - db.exec(SCHEMA_INDEXES_SQL); + try { + // Emit the same "[DB] Driver: ..." line openDatabaseAsync() prints so the + // packaged-app smoke guard (#7592) can assert the native driver was + // selected on the server's primary DB path too, not only the backup-import + // route. + console.log(`[DB] Driver: ${db.driver} | file: ${sqliteFile}`); + // better-sqlite3 is synchronous, so a contended write parks the Node event loop for up to + // busy_timeout ms (a 0-CPU freeze that stacks under load → /health stops responding). The + // hot-path writers here (usage_history, call_logs) are best-effort and the WinUI host opens + // the same DB, so cap the block at 2s instead of 5s: normal writes complete in <1ms, and a + // contended op can no longer freeze the loop past the host watchdog's 6s liveness probe. + // + // Install the busy handler before the connection's first statement. `journal_mode = WAL` + // needs a SHARED lock, and another process closing its WAL connection briefly holds the + // file EXCLUSIVE (checkpoint + WAL delete); node:sqlite opens with busy timeout 0, so with + // the pragmas in the other order that window surfaced as `database is locked` at startup. + db.pragma("busy_timeout = 2000"); + db.pragma("journal_mode = WAL"); + db.pragma("synchronous = NORMAL"); + db.pragma(`cache_size = -${DEFAULT_DATABASE_SETTINGS.optimization.cacheSize}`); + db.pragma("temp_store = MEMORY"); + // Tables first, then the legacy-column healers, and only then the indexes: an upgraded + // database can already own call_logs/usage_history/provider_connections with an older + // column set, where the CREATE TABLE is a no-op but the indexes still reference columns + // the healers below are the ones adding. + db.exec(SCHEMA_TABLES_SQL); + ensureProviderConnectionsColumns(db); + ensureUsageHistoryColumns(db); + ensureCallLogsColumns(db); + db.exec(SCHEMA_INDEXES_SQL); - // ── Versioned Migrations ── - // Auto-seed 001 as applied (the inline SCHEMA_SQL already created these tables) - // then run any new migrations (002+) - db.exec(` + // ── Versioned Migrations ── + // Auto-seed 001 as applied (the inline SCHEMA_SQL already created these tables) + // then run any new migrations (002+) + db.exec(` CREATE TABLE IF NOT EXISTS _omniroute_migrations ( version TEXT PRIMARY KEY, name TEXT NOT NULL, @@ -1277,73 +1291,81 @@ export function getDbInstance(): SqliteDatabase { VALUES ('001', 'initial_schema'); `); - runMigrations(db, { isNewDb, databaseExistedBeforeInitialization }); - // Fresh installs need the same post-migration index guarantee as upgraded - // databases, including recovery from an interrupted migration 127 attempt. - ensureUsageHistoryAccountIndex(db); + runMigrations(db, { isNewDb, databaseExistedBeforeInitialization }); + // Fresh installs need the same post-migration index guarantee as upgraded + // databases, including recovery from an interrupted migration 127 attempt. + ensureUsageHistoryAccountIndex(db); - applyStoredDatabaseOptimizationSettings(db); + applyStoredDatabaseOptimizationSettings(db); - // Apply mmap_size from stored settings (migration 046), fallback to 256MiB - try { - const mmapRow = db - .prepare("SELECT value FROM key_value WHERE namespace = ? AND key = ?") - .get("databaseSettings", "mmapSize") as { value: string } | undefined; - const mmapSize = mmapRow ? Math.max(0, parseInt(mmapRow.value, 10) || 0) : 268435456; - if (mmapSize > 0) { - db.pragma(`mmap_size = ${mmapSize}`); - } - } catch { - // mmap_size is best-effort; not available in all runtimes (e.g. web) - } - - offloadLegacyCallLogDetails(db); - - // Auto-migrate from db.json if exists - if (jsonDbFile && fs.existsSync(jsonDbFile)) { - migrateFromJson(db, jsonDbFile); - } - - if (failedProbePath && preservedCriticalState.preservedTables.length > 0) { + // Apply mmap_size from stored settings (migration 046), fallback to 256MiB try { - const restoredTables = restoreCriticalDbState(db, preservedCriticalState); - console.log( - `[DB] Restored preserved critical DB state after probe failure: ${summarizePreservedTables( - restoredTables - )}` - ); - } catch (error: unknown) { - const message = error instanceof Error ? error.message : String(error); - try { - closeProbeIfSafe(db); - } catch { - /* ignore */ + const mmapRow = db + .prepare("SELECT value FROM key_value WHERE namespace = ? AND key = ?") + .get("databaseSettings", "mmapSize") as { value: string } | undefined; + const mmapSize = mmapRow ? Math.max(0, parseInt(mmapRow.value, 10) || 0) : 268435456; + if (mmapSize > 0) { + db.pragma(`mmap_size = ${mmapSize}`); } - cleanupRecreatedSqliteFiles(sqliteFile); - throw new Error( - `[DB] Automatic recovery aborted after probe failure. ` + - `Preserved database: ${failedProbePath}. ` + - `Restore failure: ${message}.` - ); + } catch { + // mmap_size is best-effort; not available in all runtimes (e.g. web) } + + offloadLegacyCallLogDetails(db); + + // Auto-migrate from db.json if exists + if (jsonDbFile && fs.existsSync(jsonDbFile)) { + migrateFromJson(db, jsonDbFile); + } + + if (failedProbePath && preservedCriticalState.preservedTables.length > 0) { + try { + const restoredTables = restoreCriticalDbState(db, preservedCriticalState); + console.log( + `[DB] Restored preserved critical DB state after probe failure: ${summarizePreservedTables( + restoredTables + )}` + ); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + try { + closeProbeIfSafe(db); + } catch { + /* ignore */ + } + cleanupRecreatedSqliteFiles(sqliteFile); + throw new Error( + `[DB] Automatic recovery aborted after probe failure. ` + + `Preserved database: ${failedProbePath}. ` + + `Restore failure: ${message}.` + ); + } + } + + // Store schema version + const versionStmt = db.prepare( + "INSERT OR REPLACE INTO db_meta (key, value) VALUES ('schema_version', '1')" + ); + versionStmt.run(); + + // Register the singleton BEFORE the health-check gate below: the flag read + // (isDbHealthcheckStartupDeferredEnabled, like any DB-backed feature-flag + // override) itself calls getDbInstance(), and with the singleton still + // unset at this point that call would see no existing instance and open a + // second, independent connection — which hits the very same unset-singleton + // gate on its own way through, recursing without end (each recursion opens + // its own real DB connection and re-runs migrations-check, so this is a + // real resource-exhaustion loop, not just a deep call stack). #13717 rework. + setDb(db); + } catch (error) { + try { + closeProbeIfSafe(db); + } catch { + /* ignore */ + } + throw error; } - // Store schema version - const versionStmt = db.prepare( - "INSERT OR REPLACE INTO db_meta (key, value) VALUES ('schema_version', '1')" - ); - versionStmt.run(); - - // Register the singleton BEFORE the health-check gate below: the flag read - // (isDbHealthcheckStartupDeferredEnabled, like any DB-backed feature-flag - // override) itself calls getDbInstance(), and with the singleton still - // unset at this point that call would see no existing instance and open a - // second, independent connection — which hits the very same unset-singleton - // gate on its own way through, recursing without end (each recursion opens - // its own real DB connection and re-runs migrations-check, so this is a - // real resource-exhaustion loop, not just a deep call stack). #13717 rework. - setDb(db); - if (shouldRunStartupDbHealthCheck()) { if (isDbHealthcheckStartupDeferredEnabled()) { // Opt-in (#13717): defer the check past startup via the bounded/paged,