From 302dbadcb82a5fea2d64b565a9d111d81984da36 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 1 Oct 2026 11:36:55 -0700 Subject: [PATCH] fix(doctor): batch the NOCOW open-file check and close agent databases before the rewrite (#162919) --- .../integrity-and-recovery.md | 7 +- src/commands/doctor-maintenance-state.ts | 4 + src/commands/doctor-sqlite-nocow.test.ts | 114 +++++++++++++++++- src/commands/doctor-sqlite-nocow.ts | 63 +++++++--- 4 files changed, 163 insertions(+), 25 deletions(-) diff --git a/docs/reference/database-schemas/integrity-and-recovery.md b/docs/reference/database-schemas/integrity-and-recovery.md index 329442ff0fb4..e09756a33598 100644 --- a/docs/reference/database-schemas/integrity-and-recovery.md +++ b/docs/reference/database-schemas/integrity-and-recovery.md @@ -463,8 +463,11 @@ place. This updater-to-Doctor environment contract lets the updater account for physical identity changes separately from schema migration. Operator runs outside a managed update keep the normal explicit repair behavior. -Doctor drains its database handles and awaits its inspection workers before the -rewrite. A refusal saying `store files are open (pids: …)` names the processes +Doctor drains its database handles, including pooled auth-profile readers for +all agent stores under the active state directory, and awaits its inspection +workers before the rewrite. It checks every regular file in each store directory +with bounded `fuser` batches so large directories fit the operating system's +argument limit. A refusal saying `store files are open (pids: …)` names the processes reported by `fuser`. A refusal saying `fuser could not establish that all handles are closed` includes the inspection error; check that `fuser` is installed and can inspect processes through `/proc`. Both refusals leave the original store diff --git a/src/commands/doctor-maintenance-state.ts b/src/commands/doctor-maintenance-state.ts index 23b4e96cb48d..be3435606d1c 100644 --- a/src/commands/doctor-maintenance-state.ts +++ b/src/commands/doctor-maintenance-state.ts @@ -43,6 +43,10 @@ export function createDoctorMaintenanceState(options: { const closeResources = async () => { await resources?.close(); await inspections?.close(); + // Auth inspection readers are pooled separately from canonical agent handles. + const { closeAuthProfileReadPool } = + await import("../agents/auth-profiles/sqlite-read-pool.js"); + closeAuthProfileReadPool({ kind: "root", rootPath: resolveStateDir(selectedEnv) }); resources = undefined; inspections = undefined; }; diff --git a/src/commands/doctor-sqlite-nocow.test.ts b/src/commands/doctor-sqlite-nocow.test.ts index 1de48bd16ebe..47cac0212856 100644 --- a/src/commands/doctor-sqlite-nocow.test.ts +++ b/src/commands/doctor-sqlite-nocow.test.ts @@ -3,11 +3,14 @@ import fs from "node:fs"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; +import { loadPersistedAuthProfileStore } from "../agents/auth-profiles/persisted.js"; import { openNodeSqliteDatabase } from "../infra/node-sqlite.js"; +import { readSqliteReaderDiagnosticsForPath } from "../infra/sqlite-reader-lifecycle.js"; import { isSqlitePathOnBtrfs, setSqliteDirectoryNoCow } from "../infra/sqlite-wal-filesystem.js"; import { openOpenClawAgentDatabase } from "../state/openclaw-agent-db.js"; import { openOpenClawStateDatabase } from "../state/openclaw-state-db.js"; import { withOpenClawTestState } from "../test-utils/openclaw-test-state.js"; +import { prepareDoctorDatabasePreflight } from "./doctor-database-preflight.js"; import { beginDoctorMaintenance } from "./doctor-maintenance.js"; import { inspectDoctorSqliteNoCow, repairDoctorSqliteNoCow } from "./doctor-sqlite-nocow.js"; import { @@ -96,19 +99,41 @@ describe("Doctor btrfs NOCOW", () => { try { const paths = await maintenance!.run(async () => { const state = openOpenClawStateDatabase(); - const agent = openOpenClawAgentDatabase({ agentId: "main" }); - for (const database of [state, agent]) { + const agents = ["main", "secondary", "unconfigured"].map((agentId) => + openOpenClawAgentDatabase({ agentId }), + ); + for (const database of [state, ...agents]) { database.db.exec( "CREATE TABLE nocow_payload(value TEXT); INSERT INTO nocow_payload VALUES ('preserved');", ); } - return inspectDoctorSqliteNoCow([state.path, agent.path]).paths; + for (const agent of agents) { + loadPersistedAuthProfileStore(path.dirname(agent.path)); + } + const preflight = await prepareDoctorDatabasePreflight({ + cfg: { agents: { list: [{ id: "main" }, { id: "secondary" }] } }, + }); + expect(preflight.agentDatabaseMigrationDiscovery?.discovery.targets).toHaveLength(3); + return inspectDoctorSqliteNoCow([state.path, ...agents.map((agent) => agent.path)]) + .paths; + }); + expect(paths).toHaveLength(4); + const toolRunner = vi.mocked(spawnSync).getMockImplementation()!; + const handleCounts: number[] = []; + vi.mocked(spawnSync).mockImplementation((command, args, options) => { + if (command === "fuser") { + for (const pathname of paths) { + handleCounts.push(readSqliteReaderDiagnosticsForPath(pathname).connectionCount); + } + } + return toolRunner(command, args, options); }); - expect(paths).toHaveLength(2); const originals = paths.map((pathname) => fs.statSync(pathname).ino); await maintenance!.repairSqliteNoCow(paths); expect(maintenance!.warnings).toEqual([]); - expect(fixture.exchanges).toBe(2); + expect(fixture.exchanges).toBe(4); + expect(handleCounts.length).toBeGreaterThan(0); + expect(handleCounts.every((count) => count === 0)).toBe(true); for (const [index, pathname] of paths.entries()) { expect(fs.statSync(pathname).ino).not.toBe(originals[index]); const storeDir = path.dirname(pathname); @@ -131,7 +156,7 @@ describe("Doctor btrfs NOCOW", () => { } expect( log.mock.calls.filter(([line]) => line.startsWith("Rewrote SQLite")), - ).toHaveLength(2); + ).toHaveLength(4); } finally { await maintenance?.release(); } @@ -270,6 +295,83 @@ describe("Doctor btrfs NOCOW", () => { }, ); + it.each([ + { name: "many sibling files", count: 2_001, suffix: "", outcome: "holders" }, + { name: "long UTF-8 paths", count: 320, suffix: "界".repeat(60), outcome: "closed" }, + { + name: "an inspection failure in a later batch", + count: 320, + suffix: "界".repeat(60), + outcome: "inspection-error", + }, + ])( + "inspects every file in bounded fuser batches with $name", + async ({ count, suffix, outcome }) => { + seedDatabase(); + const nested = path.join(directory, "workshop-skills"); + fs.mkdirSync(nested); + const files = [ + sqlitePath, + `${sqlitePath}-wal`, + `${sqlitePath}-shm`, + path.join(directory, "sibling.txt"), + ]; + for (let index = 0; index < count; index++) { + const pathname = path.join(nested, `${index}-${suffix}.txt`); + fs.writeFileSync(pathname, "preserved sibling"); + files.push(pathname); + } + const original = fs.statSync(sqlitePath); + const tool = vi.mocked(spawnSync).getMockImplementation()!; + const batches: string[][] = []; + vi.mocked(spawnSync).mockImplementation((command, args, options) => { + if (command !== "fuser") { + return tool(command, args, options); + } + const argv = [...(args ?? [])]; + batches.push(argv); + const result = { status: 1, stdout: "", stderr: "", pid: 0, output: [], signal: null }; + if ( + argv.length > 2_000 || + argv.reduce((bytes, pathname) => bytes + Buffer.byteLength(pathname, "utf8") + 1, 0) > + 64 * 1024 + ) { + return { + ...result, + status: null, + error: Object.assign(new Error("spawnSync fuser E2BIG"), { code: "E2BIG" }), + }; + } + if (outcome === "holders") { + return { ...result, status: 0, stdout: batches.length === 1 ? "12345 12345" : "67890" }; + } + if (outcome === "inspection-error" && batches.length > 1) { + return { ...result, stderr: "Cannot stat file /proc/123/fd/4: Permission denied" }; + } + return result; + }); + + const notes = (await repair()).join("\n"); + expect(notes).not.toContain("E2BIG"); + expect(batches.length).toBeGreaterThan(1); + expect(batches.flat().slice(0, files.length).toSorted()).toEqual(files.toSorted()); + if (outcome === "closed") { + expect(notes).toContain("Rewrote SQLite store directory with NOCOW"); + expect(fixture.exchanges).toBe(1); + expect(fs.readFileSync(files.at(-1)!, "utf8")).toBe("preserved sibling"); + expect(batches.flat().filter((pathname) => pathname === sqlitePath)).toHaveLength(2); + } else { + expect(notes).toContain( + outcome === "holders" + ? "store files are open (pids: 12345, 67890)" + : "fuser could not establish that all handles are closed: Cannot stat file /proc/123/fd/4: Permission denied", + ); + expect(fixture.exchanges).toBe(0); + expect(fs.statSync(sqlitePath).ino).toBe(original.ino); + } + }, + ); + it.each([ { name: "open handles", diff --git a/src/commands/doctor-sqlite-nocow.ts b/src/commands/doctor-sqlite-nocow.ts index d964c38af3f6..3d37ce43f1dc 100644 --- a/src/commands/doctor-sqlite-nocow.ts +++ b/src/commands/doctor-sqlite-nocow.ts @@ -16,6 +16,9 @@ import { DoctorMaintenanceRefusalError } from "../infra/update-doctor-result.js" import { assertDoctorSqliteMaintenancePathsNotAliased } from "./doctor-sqlite-maintenance-lock.js"; const TOOL_TIMEOUT_MS = 2_000; +const FUSER_MAX_PATHS = 2_000; +// Leave room for the inherited environment and argv pointers under Linux ARG_MAX. +const FUSER_MAX_PATH_BYTES = 64 * 1024; function hasNoCow(pathname: string): boolean { const result = spawnSync("lsattr", ["-d", "--", pathname], { @@ -147,29 +150,55 @@ async function quickCheck(pathname: string) { } function assertNoOpenFiles(paths: readonly string[]) { - const result = spawnSync( - "fuser", - paths.map((pathname) => path.resolve(pathname)), - { + const batches: string[][] = []; + let batch: string[] = []; + let batchBytes = 0; + for (const pathname of paths) { + const absolute = path.resolve(pathname); + const bytes = Buffer.byteLength(absolute, "utf8") + 1; + if ( + batch.length > 0 && + (batch.length >= FUSER_MAX_PATHS || batchBytes + bytes > FUSER_MAX_PATH_BYTES) + ) { + batches.push(batch); + batch = []; + batchBytes = 0; + } + batch.push(absolute); + batchBytes += bytes; + } + if (batch.length > 0) { + batches.push(batch); + } + + const pids = new Set(); + let inspectionError: string | undefined; + for (const args of batches) { + const result = spawnSync("fuser", args, { encoding: "utf8", timeout: TOOL_TIMEOUT_MS, - }, - ); - const stdout = result.stdout?.trim() ?? ""; - const stderr = result.stderr?.trim() ?? ""; - if (result.status === 0 && /^\d+(?:\s+\d+)*$/u.test(stdout)) { - const pids = [...new Set(stdout.split(/\s+/u))].join(", "); + }); + const stdout = result.stdout?.trim() ?? ""; + const stderr = result.stderr?.trim() ?? ""; + if (result.status === 0 && /^\d+(?:\s+\d+)*$/u.test(stdout)) { + for (const pid of stdout.split(/\s+/u)) { + pids.add(pid); + } + } else if (result.error || result.status !== 1 || stdout || stderr) { + inspectionError ??= + stderr || + result.error?.message || + (result.signal ? `signal ${result.signal}` : stdout || `exit status ${result.status}`); + } + } + if (pids.size > 0) { throw new Error( - `store files are open (pids: ${pids}); stop processes using this store before retrying`, + `store files are open (pids: ${[...pids].join(", ")}); stop processes using this store before retrying`, ); } - if (result.error || result.status !== 1 || stdout || stderr) { - const detail = - stderr || - result.error?.message || - (result.signal ? `signal ${result.signal}` : stdout || `exit status ${result.status}`); + if (inspectionError) { throw new Error( - `fuser could not establish that all handles are closed: ${detail}; ensure fuser is installed and can inspect processes using this store`, + `fuser could not establish that all handles are closed: ${inspectionError}; ensure fuser is installed and can inspect processes using this store`, ); } }