From ea346f4361ef9be249fb631d48560499fcaf6a78 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Tue, 16 Jun 2026 02:11:11 +0200 Subject: [PATCH] fix(sqlite): close databases after failed initialization --- .../src/memory/manager-db-probe.test.ts | 24 +++++ .../memory-core/src/memory/manager-db.ts | 17 +++- .../open-file-descriptors.test-support.ts | 13 +++ src/proxy-capture/store.sqlite.test.ts | 13 +++ src/proxy-capture/store.sqlite.ts | 95 ++++++++++--------- src/state/openclaw-agent-db.test.ts | 16 ++++ src/state/openclaw-agent-db.ts | 32 ++++--- src/state/openclaw-state-db.test.ts | 11 +++ src/state/openclaw-state-db.ts | 32 ++++--- 9 files changed, 176 insertions(+), 77 deletions(-) create mode 100644 src/infra/open-file-descriptors.test-support.ts diff --git a/extensions/memory-core/src/memory/manager-db-probe.test.ts b/extensions/memory-core/src/memory/manager-db-probe.test.ts index 2de332069546..47dbb0accf8b 100644 --- a/extensions/memory-core/src/memory/manager-db-probe.test.ts +++ b/extensions/memory-core/src/memory/manager-db-probe.test.ts @@ -19,6 +19,17 @@ async function expectPathMissing(targetPath: string): Promise { await expect(fs.access(targetPath)).rejects.toThrow("ENOENT"); } +function listOpenFileDescriptorsForPath(targetPath: string): string[] { + return fsSync.readdirSync("/proc/self/fd").flatMap((fd) => { + try { + const descriptorPath = fsSync.readlinkSync(`/proc/self/fd/${fd}`); + return descriptorPath.startsWith(targetPath) ? [descriptorPath] : []; + } catch { + return []; + } + }); +} + describe("openMemoryDatabaseAtPath readOnly probe", () => { let fixtureRoot = ""; let caseId = 0; @@ -59,6 +70,19 @@ describe("openMemoryDatabaseAtPath readOnly probe", () => { expect(stat.size).toBeGreaterThan(0); }); + it.skipIf(process.platform !== "linux")( + "closes the database when SQLite maintenance configuration fails", + async () => { + const dbPath = path.join(fixtureRoot, `case-${caseId++}`, "malformed-index.sqlite"); + await fs.mkdir(path.dirname(dbPath), { recursive: true }); + await fs.writeFile(dbPath, "not a sqlite database"); + + expect(() => openMemoryDatabaseAtPath(dbPath, false, false)).toThrow(/not a database/); + + expect(listOpenFileDescriptorsForPath(dbPath)).toEqual([]); + }, + ); + it("refuses to create a missing live database while a safe reindex holds the lock", async () => { const dbPath = path.join(fixtureRoot, `case-${caseId++}`, "index.sqlite"); await fs.mkdir(path.dirname(dbPath), { recursive: true }); diff --git a/extensions/memory-core/src/memory/manager-db.ts b/extensions/memory-core/src/memory/manager-db.ts index 6c7c90217b0b..0c426d694eb2 100644 --- a/extensions/memory-core/src/memory/manager-db.ts +++ b/extensions/memory-core/src/memory/manager-db.ts @@ -127,11 +127,18 @@ export function cleanupAgedMemoryReindexTempFiles(dbPath: string, nowMs = Date.n function openConfiguredMemoryDatabaseAtPath(dbPath: string, allowExtension: boolean): DatabaseSync { const { DatabaseSync } = requireNodeSqlite(); const db = new DatabaseSync(dbPath, { allowExtension }); - configureMemorySqliteWalMaintenance(db, { - busyTimeoutMs: 5000, - databasePath: dbPath, - }); - return db; + try { + configureMemorySqliteWalMaintenance(db, { + busyTimeoutMs: 5000, + databasePath: dbPath, + }); + return db; + } catch (err) { + try { + db.close(); + } catch {} + throw err; + } } type ExistingMemoryDatabaseOpenResult = diff --git a/src/infra/open-file-descriptors.test-support.ts b/src/infra/open-file-descriptors.test-support.ts new file mode 100644 index 000000000000..8ba8d5353256 --- /dev/null +++ b/src/infra/open-file-descriptors.test-support.ts @@ -0,0 +1,13 @@ +// Linux-only test support for proving failed opens release their file handles. +import fs from "node:fs"; + +export function listOpenFileDescriptorsForPath(targetPath: string): string[] { + return fs.readdirSync("/proc/self/fd").flatMap((fd) => { + try { + const descriptorPath = fs.readlinkSync(`/proc/self/fd/${fd}`); + return descriptorPath.startsWith(targetPath) ? [descriptorPath] : []; + } catch { + return []; + } + }); +} diff --git a/src/proxy-capture/store.sqlite.test.ts b/src/proxy-capture/store.sqlite.test.ts index 570f9a70d587..fa085231f553 100644 --- a/src/proxy-capture/store.sqlite.test.ts +++ b/src/proxy-capture/store.sqlite.test.ts @@ -3,6 +3,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { listOpenFileDescriptorsForPath } from "../infra/open-file-descriptors.test-support.js"; import { acquireDebugProxyCaptureStore, closeDebugProxyCaptureStore, @@ -31,6 +32,18 @@ function makeStore() { } describe("DebugProxyCaptureStore", () => { + it.runIf(process.platform === "linux")("closes the database when initialization fails", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-proxy-capture-failed-open-")); + cleanupDirs.push(root); + const dbPath = path.join(root, "capture.sqlite"); + fs.writeFileSync(dbPath, "not a sqlite database"); + + expect(() => new DebugProxyCaptureStore(dbPath, path.join(root, "blobs"))).toThrow( + "file is not a database", + ); + expect(listOpenFileDescriptorsForPath(dbPath)).toEqual([]); + }); + it("keeps the cached store open until the last lease releases", () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-proxy-capture-lease-")); cleanupDirs.push(root); diff --git a/src/proxy-capture/store.sqlite.ts b/src/proxy-capture/store.sqlite.ts index 71f7d376e0ea..27915ae2cc28 100644 --- a/src/proxy-capture/store.sqlite.ts +++ b/src/proxy-capture/store.sqlite.ts @@ -36,50 +36,57 @@ function openDatabase(dbPath: string): OpenedDatabase { ensureParentDir(dbPath); const { DatabaseSync } = requireNodeSqlite(); const db = new DatabaseSync(dbPath); - const walMaintenance = configureSqliteConnectionPragmas(db, { - busyTimeoutMs: 5000, - databaseLabel: "debug-proxy-capture", - databasePath: dbPath, - }); - db.exec(` - CREATE TABLE IF NOT EXISTS capture_sessions ( - id TEXT PRIMARY KEY, - started_at INTEGER NOT NULL, - ended_at INTEGER, - mode TEXT NOT NULL, - source_scope TEXT NOT NULL, - source_process TEXT NOT NULL, - proxy_url TEXT, - db_path TEXT NOT NULL, - blob_dir TEXT NOT NULL - ); - CREATE TABLE IF NOT EXISTS capture_events ( - id INTEGER PRIMARY KEY, - session_id TEXT NOT NULL, - ts INTEGER NOT NULL, - source_scope TEXT NOT NULL, - source_process TEXT NOT NULL, - protocol TEXT NOT NULL, - direction TEXT NOT NULL, - kind TEXT NOT NULL, - flow_id TEXT NOT NULL, - method TEXT, - host TEXT, - path TEXT, - status INTEGER, - close_code INTEGER, - content_type TEXT, - headers_json TEXT, - data_text TEXT, - data_blob_id TEXT, - data_sha256 TEXT, - error_text TEXT, - meta_json TEXT - ); - CREATE INDEX IF NOT EXISTS capture_events_session_ts_idx ON capture_events(session_id, ts); - CREATE INDEX IF NOT EXISTS capture_events_flow_idx ON capture_events(flow_id, ts); - `); - return { db, walMaintenance }; + let walMaintenance: SqliteWalMaintenance | undefined; + try { + walMaintenance = configureSqliteConnectionPragmas(db, { + busyTimeoutMs: 5000, + databaseLabel: "debug-proxy-capture", + databasePath: dbPath, + }); + db.exec(` + CREATE TABLE IF NOT EXISTS capture_sessions ( + id TEXT PRIMARY KEY, + started_at INTEGER NOT NULL, + ended_at INTEGER, + mode TEXT NOT NULL, + source_scope TEXT NOT NULL, + source_process TEXT NOT NULL, + proxy_url TEXT, + db_path TEXT NOT NULL, + blob_dir TEXT NOT NULL + ); + CREATE TABLE IF NOT EXISTS capture_events ( + id INTEGER PRIMARY KEY, + session_id TEXT NOT NULL, + ts INTEGER NOT NULL, + source_scope TEXT NOT NULL, + source_process TEXT NOT NULL, + protocol TEXT NOT NULL, + direction TEXT NOT NULL, + kind TEXT NOT NULL, + flow_id TEXT NOT NULL, + method TEXT, + host TEXT, + path TEXT, + status INTEGER, + close_code INTEGER, + content_type TEXT, + headers_json TEXT, + data_text TEXT, + data_blob_id TEXT, + data_sha256 TEXT, + error_text TEXT, + meta_json TEXT + ); + CREATE INDEX IF NOT EXISTS capture_events_session_ts_idx ON capture_events(session_id, ts); + CREATE INDEX IF NOT EXISTS capture_events_flow_idx ON capture_events(flow_id, ts); + `); + return { db, walMaintenance }; + } catch (err) { + walMaintenance?.close(); + db.close(); + throw err; + } } function serializeJson(value: unknown): string | null { diff --git a/src/state/openclaw-agent-db.test.ts b/src/state/openclaw-agent-db.test.ts index 4c5f333ee372..3b9a14cc78d1 100644 --- a/src/state/openclaw-agent-db.test.ts +++ b/src/state/openclaw-agent-db.test.ts @@ -6,6 +6,7 @@ import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { executeSqliteQueryTakeFirstSync, getNodeSqliteKysely } from "../infra/kysely-sync.js"; import { requireNodeSqlite } from "../infra/node-sqlite.js"; +import { listOpenFileDescriptorsForPath } from "../infra/open-file-descriptors.test-support.js"; import { readSqliteNumberPragma } from "../infra/sqlite-pragma.test-support.js"; import type { DB as OpenClawAgentKyselyDatabase } from "./openclaw-agent-db.generated.js"; import { @@ -95,6 +96,21 @@ describe("openclaw agent database", () => { expect(registered?.sizeBytes).toBeGreaterThan(0); }); + it.runIf(process.platform === "linux")("closes the database when initialization fails", () => { + const stateDir = createTempStateDir(); + const databasePath = path.join(stateDir, "agent.sqlite"); + fs.writeFileSync(databasePath, "not a sqlite database"); + + expect(() => + openOpenClawAgentDatabase({ + agentId: "worker-1", + env: { OPENCLAW_STATE_DIR: stateDir }, + path: databasePath, + }), + ).toThrow("file is not a database"); + expect(listOpenFileDescriptorsForPath(databasePath)).toEqual([]); + }); + it("keeps multiple registered paths for the same agent", () => { const stateDir = createTempStateDir(); const env = { OPENCLAW_STATE_DIR: stateDir }; diff --git a/src/state/openclaw-agent-db.ts b/src/state/openclaw-agent-db.ts index 868e4f299645..f51a6fc00370 100644 --- a/src/state/openclaw-agent-db.ts +++ b/src/state/openclaw-agent-db.ts @@ -268,20 +268,24 @@ export function openOpenClawAgentDatabase( ensureOpenClawAgentDatabasePermissions(pathname, databaseOptions); const sqlite = requireNodeSqlite(); const db = new sqlite.DatabaseSync(pathname); - const walMaintenance = configureSqliteConnectionPragmas(db, { - busyTimeoutMs: OPENCLAW_SQLITE_BUSY_TIMEOUT_MS, - databaseLabel: `openclaw-agent:${agentId}`, - databasePath: pathname, - foreignKeys: true, - synchronous: "NORMAL", - }); - try { - ensureAgentSchema(db, agentId, pathname); - } catch (err) { - walMaintenance.close(); - db.close(); - throw err; - } + const walMaintenance = (() => { + let maintenance: SqliteWalMaintenance | undefined; + try { + maintenance = configureSqliteConnectionPragmas(db, { + busyTimeoutMs: OPENCLAW_SQLITE_BUSY_TIMEOUT_MS, + databaseLabel: `openclaw-agent:${agentId}`, + databasePath: pathname, + foreignKeys: true, + synchronous: "NORMAL", + }); + ensureAgentSchema(db, agentId, pathname); + return maintenance; + } catch (err) { + maintenance?.close(); + db.close(); + throw err; + } + })(); ensureOpenClawAgentDatabasePermissions(pathname, databaseOptions); const database = { agentId, db, path: pathname, walMaintenance }; cachedDatabases.set(pathname, database); diff --git a/src/state/openclaw-state-db.test.ts b/src/state/openclaw-state-db.test.ts index 4c20f4e8de68..c433b1cf7adf 100644 --- a/src/state/openclaw-state-db.test.ts +++ b/src/state/openclaw-state-db.test.ts @@ -11,6 +11,7 @@ import { getNodeSqliteKysely, } from "../infra/kysely-sync.js"; import { requireNodeSqlite } from "../infra/node-sqlite.js"; +import { listOpenFileDescriptorsForPath } from "../infra/open-file-descriptors.test-support.js"; import { readSqliteNumberPragma } from "../infra/sqlite-pragma.test-support.js"; import type { DB as OpenClawStateKyselyDatabase } from "./openclaw-state-db.generated.js"; import { @@ -79,6 +80,16 @@ describe("openclaw state database", () => { expect(database.path).toBe(path.join(stateDir, "state", "openclaw.sqlite")); }); + it.runIf(process.platform === "linux")("closes the database when initialization fails", () => { + const databasePath = path.join(createTempStateDir(), "openclaw.sqlite"); + fs.writeFileSync(databasePath, "not a sqlite database"); + + expect(() => openOpenClawStateDatabase({ path: databasePath })).toThrow( + "file is not a database", + ); + expect(listOpenFileDescriptorsForPath(databasePath)).toEqual([]); + }); + it("migrates requester and executor attribution for existing cross-agent tasks", () => { const stateDir = createTempStateDir(); const database = openOpenClawStateDatabase({ diff --git a/src/state/openclaw-state-db.ts b/src/state/openclaw-state-db.ts index 67ed7279f305..3792874e7194 100644 --- a/src/state/openclaw-state-db.ts +++ b/src/state/openclaw-state-db.ts @@ -977,20 +977,24 @@ export function openOpenClawStateDatabase( ensureOpenClawStatePermissions(pathname, env); const sqlite = requireNodeSqlite(); const db = new sqlite.DatabaseSync(pathname); - const walMaintenance = configureSqliteConnectionPragmas(db, { - busyTimeoutMs: OPENCLAW_SQLITE_BUSY_TIMEOUT_MS, - databaseLabel: "openclaw-state", - databasePath: pathname, - foreignKeys: true, - synchronous: "NORMAL", - }); - try { - ensureSchema(db, pathname); - } catch (err) { - walMaintenance.close(); - db.close(); - throw err; - } + const walMaintenance = (() => { + let maintenance: SqliteWalMaintenance | undefined; + try { + maintenance = configureSqliteConnectionPragmas(db, { + busyTimeoutMs: OPENCLAW_SQLITE_BUSY_TIMEOUT_MS, + databaseLabel: "openclaw-state", + databasePath: pathname, + foreignKeys: true, + synchronous: "NORMAL", + }); + ensureSchema(db, pathname); + return maintenance; + } catch (err) { + maintenance?.close(); + db.close(); + throw err; + } + })(); ensureOpenClawStatePermissions(pathname, env); const database = { db, path: pathname, walMaintenance }; cachedDatabases.set(pathname, database);