From 4fb373466eeba0327a3d5cf68ac7156cccbf464d Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 31 Mar 2026 15:01:01 +0100 Subject: [PATCH] refactor: simplify memory recovery and test setup --- .../src/memory/manager.read-file.test.ts | 68 +++---- .../memory/manager.readonly-recovery.test.ts | 39 ++-- extensions/memory-core/src/memory/manager.ts | 190 +++++++++++------- src/infra/path-env.test.ts | 29 +-- src/infra/provider-usage.fetch.shared.test.ts | 11 +- src/media-understanding/image.test.ts | 46 ++--- 6 files changed, 189 insertions(+), 194 deletions(-) diff --git a/extensions/memory-core/src/memory/manager.read-file.test.ts b/extensions/memory-core/src/memory/manager.read-file.test.ts index 98b161a9935..af97a079a83 100644 --- a/extensions/memory-core/src/memory/manager.read-file.test.ts +++ b/extensions/memory-core/src/memory/manager.read-file.test.ts @@ -1,51 +1,17 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import type { OpenClawConfig } from "openclaw/plugin-sdk/memory-core-host-engine-foundation"; +import { readMemoryFile } from "openclaw/plugin-sdk/memory-core-host-engine-storage"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; -import { resetEmbeddingMocks } from "./embedding.test-mocks.js"; -import type { MemoryIndexManager } from "./index.js"; -import { getRequiredMemoryIndexManager } from "./test-manager-helpers.js"; - -function createMemorySearchCfg(options: { - workspaceDir: string; - indexPath: string; -}): OpenClawConfig { - return { - agents: { - defaults: { - workspace: options.workspaceDir, - memorySearch: { - provider: "openai", - model: "mock-embed", - store: { path: options.indexPath, vector: { enabled: false } }, - cache: { enabled: false }, - query: { minScore: 0, hybrid: { enabled: false } }, - sync: { watch: false, onSessionStart: false, onSearch: false }, - }, - }, - list: [{ id: "main", default: true }], - }, - } as OpenClawConfig; -} describe("MemoryIndexManager.readFile", () => { let workspaceDir: string; - let indexPath: string; let memoryDir: string; - let manager: MemoryIndexManager | null = null; beforeAll(async () => { - resetEmbeddingMocks(); workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-mem-read-")); - indexPath = path.join(workspaceDir, "index.sqlite"); memoryDir = path.join(workspaceDir, "memory"); await fs.mkdir(memoryDir, { recursive: true }); - manager = await getRequiredMemoryIndexManager({ - cfg: createMemorySearchCfg({ workspaceDir, indexPath }), - agentId: "main", - purpose: "status", - }); }); afterEach(async () => { @@ -58,16 +24,16 @@ describe("MemoryIndexManager.readFile", () => { }); afterAll(async () => { - if (manager) { - await manager.close(); - manager = null; - } await fs.rm(workspaceDir, { recursive: true, force: true }); }); it("returns empty text when the requested file does not exist", async () => { const relPath = "memory/2099-01-01.md"; - const result = await manager!.readFile({ relPath }); + const result = await readMemoryFile({ + workspaceDir, + extraPaths: [], + relPath, + }); expect(result).toEqual({ text: "", path: relPath }); }); @@ -77,7 +43,13 @@ describe("MemoryIndexManager.readFile", () => { await fs.mkdir(path.dirname(absPath), { recursive: true }); await fs.writeFile(absPath, ["line 1", "line 2", "line 3"].join("\n"), "utf-8"); - const result = await manager!.readFile({ relPath, from: 2, lines: 1 }); + const result = await readMemoryFile({ + workspaceDir, + extraPaths: [], + relPath, + from: 2, + lines: 1, + }); expect(result).toEqual({ text: "line 2", path: relPath }); }); @@ -87,7 +59,13 @@ describe("MemoryIndexManager.readFile", () => { await fs.mkdir(path.dirname(absPath), { recursive: true }); await fs.writeFile(absPath, ["alpha", "beta"].join("\n"), "utf-8"); - const result = await manager!.readFile({ relPath, from: 10, lines: 5 }); + const result = await readMemoryFile({ + workspaceDir, + extraPaths: [], + relPath, + from: 10, + lines: 5, + }); expect(result).toEqual({ text: "", path: relPath }); }); @@ -113,7 +91,11 @@ describe("MemoryIndexManager.readFile", () => { }); try { - const result = await manager!.readFile({ relPath }); + const result = await readMemoryFile({ + workspaceDir, + extraPaths: [], + relPath, + }); expect(result).toEqual({ text: "", path: relPath }); } finally { readSpy.mockRestore(); diff --git a/extensions/memory-core/src/memory/manager.readonly-recovery.test.ts b/extensions/memory-core/src/memory/manager.readonly-recovery.test.ts index a0b601c47a9..1da15dc6adf 100644 --- a/extensions/memory-core/src/memory/manager.readonly-recovery.test.ts +++ b/extensions/memory-core/src/memory/manager.readonly-recovery.test.ts @@ -4,9 +4,8 @@ import path from "node:path"; import type { DatabaseSync } from "node:sqlite"; import type { OpenClawConfig } from "openclaw/plugin-sdk/memory-core-host-engine-foundation"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { resetEmbeddingMocks } from "./embedding.test-mocks.js"; -import { MemoryIndexManager } from "./manager.js"; -import { getRequiredMemoryIndexManager } from "./test-manager-helpers.js"; +import { openMemoryDatabaseAtPath } from "./manager-sync-ops.js"; +import { runMemorySyncWithReadonlyRecovery } from "./manager.js"; type ReadonlyRecoveryHarness = { closed: boolean; @@ -36,7 +35,6 @@ type ReadonlyRecoveryHarness = { describe("memory manager readonly recovery", () => { let workspaceDir = ""; let indexPath = ""; - let manager: MemoryIndexManager | null = null; function createMemoryConfig(): OpenClawConfig { return { @@ -57,15 +55,6 @@ describe("memory manager readonly recovery", () => { } as OpenClawConfig; } - async function createRealManager() { - manager = await getRequiredMemoryIndexManager({ - cfg: createMemoryConfig(), - agentId: "main", - purpose: "status", - }); - return manager; - } - function createReadonlyRecoveryHarness() { const reopenedClose = vi.fn(); const initialClose = vi.fn(); @@ -95,7 +84,6 @@ describe("memory manager readonly recovery", () => { ensureSchema: vi.fn(), readMeta: vi.fn(() => undefined), }; - Object.setPrototypeOf(harness, MemoryIndexManager.prototype); return { harness, initialDb, @@ -105,6 +93,16 @@ describe("memory manager readonly recovery", () => { }; } + async function runSyncWithReadonlyRecovery( + harness: ReadonlyRecoveryHarness, + params?: { reason?: string; force?: boolean; sessionFiles?: string[] }, + ) { + return await runMemorySyncWithReadonlyRecovery( + harness as unknown as Parameters[0], + params, + ); + } + function expectReadonlyRecoveryStatus( instance: { readonlyRecoveryAttempts: number; @@ -131,7 +129,7 @@ describe("memory manager readonly recovery", () => { const { harness, initialClose } = createReadonlyRecoveryHarness(); harness.runSync.mockRejectedValueOnce(params.firstError).mockResolvedValueOnce(undefined); - await MemoryIndexManager.prototype.sync.call(harness as unknown as MemoryIndexManager, { + await runSyncWithReadonlyRecovery(harness, { reason: "test", }); @@ -142,7 +140,6 @@ describe("memory manager readonly recovery", () => { } beforeEach(async () => { - resetEmbeddingMocks(); workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-mem-readonly-")); indexPath = path.join(workspaceDir, "index.sqlite"); await fs.mkdir(path.join(workspaceDir, "memory"), { recursive: true }); @@ -151,10 +148,6 @@ describe("memory manager readonly recovery", () => { afterEach(async () => { vi.restoreAllMocks(); - if (manager) { - await manager.close(); - manager = null; - } await fs.rm(workspaceDir, { recursive: true, force: true }); }); @@ -177,7 +170,7 @@ describe("memory manager readonly recovery", () => { harness.runSync.mockRejectedValueOnce(new Error("embedding timeout")); await expect( - MemoryIndexManager.prototype.sync.call(harness as unknown as MemoryIndexManager, { + runSyncWithReadonlyRecovery(harness, { reason: "test", }), ).rejects.toThrow("embedding timeout"); @@ -187,12 +180,12 @@ describe("memory manager readonly recovery", () => { }); it("sets busy_timeout on memory sqlite connections", async () => { - const currentManager = await createRealManager(); - const db = (currentManager as unknown as { db: DatabaseSync }).db; + const db = openMemoryDatabaseAtPath(indexPath, false); const row = db.prepare("PRAGMA busy_timeout").get() as | { busy_timeout?: number; timeout?: number } | undefined; const busyTimeout = row?.busy_timeout ?? row?.timeout; expect(busyTimeout).toBe(5000); + db.close(); }); }); diff --git a/extensions/memory-core/src/memory/manager.ts b/extensions/memory-core/src/memory/manager.ts index b65c085474d..41db96f03cb 100644 --- a/extensions/memory-core/src/memory/manager.ts +++ b/extensions/memory-core/src/memory/manager.ts @@ -58,6 +58,121 @@ const log = createSubsystemLogger("memory"); const { indexCache: INDEX_CACHE, indexCachePending: INDEX_CACHE_PENDING } = getMemoryIndexManagerCacheStore(); +type MemoryReadonlyRecoveryState = { + closed: boolean; + db: DatabaseSync; + vectorReady: Promise | null; + vector: { + enabled: boolean; + available: boolean | null; + extensionPath?: string; + loadError?: string; + dims?: number; + }; + readonlyRecoveryAttempts: number; + readonlyRecoverySuccesses: number; + readonlyRecoveryFailures: number; + readonlyRecoveryLastError?: string; + runSync: (params?: { + reason?: string; + force?: boolean; + sessionFiles?: string[]; + progress?: (update: MemorySyncProgressUpdate) => void; + }) => Promise; + openDatabase: () => DatabaseSync; + ensureSchema: () => void; + readMeta: () => { vectorDims?: number } | undefined; +}; + +export function isMemoryReadonlyDbError(err: unknown): boolean { + const readonlyPattern = + /attempt to write a readonly database|database is read-only|SQLITE_READONLY/i; + const messages = new Set(); + + const pushValue = (value: unknown): void => { + if (typeof value !== "string") { + return; + } + const normalized = value.trim(); + if (!normalized) { + return; + } + messages.add(normalized); + }; + + pushValue(err instanceof Error ? err.message : String(err)); + if (err && typeof err === "object") { + const record = err as Record; + pushValue(record.message); + pushValue(record.code); + pushValue(record.name); + if (record.cause && typeof record.cause === "object") { + const cause = record.cause as Record; + pushValue(cause.message); + pushValue(cause.code); + pushValue(cause.name); + } + } + + return [...messages].some((value) => readonlyPattern.test(value)); +} + +export function extractMemoryErrorReason(err: unknown): string { + if (err instanceof Error && err.message.trim()) { + return err.message; + } + if (err && typeof err === "object") { + const record = err as Record; + if (typeof record.message === "string" && record.message.trim()) { + return record.message; + } + if (typeof record.code === "string" && record.code.trim()) { + return record.code; + } + } + return String(err); +} + +export async function runMemorySyncWithReadonlyRecovery( + state: MemoryReadonlyRecoveryState, + params?: { + reason?: string; + force?: boolean; + sessionFiles?: string[]; + progress?: (update: MemorySyncProgressUpdate) => void; + }, +): Promise { + try { + await state.runSync(params); + return; + } catch (err) { + if (!isMemoryReadonlyDbError(err) || state.closed) { + throw err; + } + const reason = extractMemoryErrorReason(err); + state.readonlyRecoveryAttempts += 1; + state.readonlyRecoveryLastError = reason; + log.warn(`memory sync readonly handle detected; reopening sqlite connection`, { reason }); + try { + state.db.close(); + } catch {} + state.db = state.openDatabase(); + state.vectorReady = null; + state.vector.available = null; + state.vector.loadError = undefined; + state.ensureSchema(); + const meta = state.readMeta(); + state.vector.dims = meta?.vectorDims; + try { + await state.runSync(params); + state.readonlyRecoverySuccesses += 1; + } catch (retryErr) { + state.readonlyRecoveryFailures += 1; + throw retryErr; + } + } +} + export async function closeAllMemoryIndexManagers(): Promise { const pending = Array.from(INDEX_CACHE_PENDING.values()); if (pending.length > 0) { @@ -598,52 +713,11 @@ export class MemoryIndexManager extends MemoryManagerEmbeddingOps implements Mem } private isReadonlyDbError(err: unknown): boolean { - const readonlyPattern = - /attempt to write a readonly database|database is read-only|SQLITE_READONLY/i; - const messages = new Set(); - - const pushValue = (value: unknown): void => { - if (typeof value !== "string") { - return; - } - const normalized = value.trim(); - if (!normalized) { - return; - } - messages.add(normalized); - }; - - pushValue(err instanceof Error ? err.message : String(err)); - if (err && typeof err === "object") { - const record = err as Record; - pushValue(record.message); - pushValue(record.code); - pushValue(record.name); - if (record.cause && typeof record.cause === "object") { - const cause = record.cause as Record; - pushValue(cause.message); - pushValue(cause.code); - pushValue(cause.name); - } - } - - return [...messages].some((value) => readonlyPattern.test(value)); + return isMemoryReadonlyDbError(err); } private extractErrorReason(err: unknown): string { - if (err instanceof Error && err.message.trim()) { - return err.message; - } - if (err && typeof err === "object") { - const record = err as Record; - if (typeof record.message === "string" && record.message.trim()) { - return record.message; - } - if (typeof record.code === "string" && record.code.trim()) { - return record.code; - } - } - return String(err); + return extractMemoryErrorReason(err); } private async runSyncWithReadonlyRecovery(params?: { @@ -652,35 +726,7 @@ export class MemoryIndexManager extends MemoryManagerEmbeddingOps implements Mem sessionFiles?: string[]; progress?: (update: MemorySyncProgressUpdate) => void; }): Promise { - try { - await this.runSync(params); - return; - } catch (err) { - if (!this.isReadonlyDbError(err) || this.closed) { - throw err; - } - const reason = this.extractErrorReason(err); - this.readonlyRecoveryAttempts += 1; - this.readonlyRecoveryLastError = reason; - log.warn(`memory sync readonly handle detected; reopening sqlite connection`, { reason }); - try { - this.db.close(); - } catch {} - this.db = this.openDatabase(); - this.vectorReady = null; - this.vector.available = null; - this.vector.loadError = undefined; - this.ensureSchema(); - const meta = this.readMeta(); - this.vector.dims = meta?.vectorDims; - try { - await this.runSync(params); - this.readonlyRecoverySuccesses += 1; - } catch (retryErr) { - this.readonlyRecoveryFailures += 1; - throw retryErr; - } - } + await runMemorySyncWithReadonlyRecovery(this, params); } async readFile(params: { diff --git a/src/infra/path-env.test.ts b/src/infra/path-env.test.ts index 3188ff3b912..617897c45ce 100644 --- a/src/infra/path-env.test.ts +++ b/src/infra/path-env.test.ts @@ -1,5 +1,6 @@ import path from "node:path"; -import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { ensureOpenClawCliOnPath } from "./path-env.js"; const state = vi.hoisted(() => ({ dirs: new Set(), @@ -19,15 +20,21 @@ vi.mock("node:fs", async (importOriginal) => { ...actual, constants: { ...actual.constants, X_OK: actual.constants.X_OK ?? 1 }, accessSync: (p: string, mode?: number) => { - // `mode` is ignored in tests; we only model "is executable" or "not". - if (!state.executables.has(absInMock(p))) { - throw new Error(`EACCES: permission denied, access '${p}' (mode=${mode ?? 0})`); + const resolved = absInMock(p); + if (state.executables.has(resolved)) { + return; } + actual.accessSync(p, mode); + }, + statSync: (p: string) => { + const resolved = absInMock(p); + if (state.dirs.has(resolved)) { + return { + isDirectory: () => true, + }; + } + return actual.statSync(p); }, - statSync: (p: string) => ({ - // Avoid throws for non-existent paths; the code under test only cares about isDirectory(). - isDirectory: () => state.dirs.has(absInMock(p)), - }), }; return { ...wrapped, default: wrapped }; @@ -37,8 +44,6 @@ vi.mock("./env.js", () => ({ isTruthyEnvValue: (value?: string) => value === "1" || value === "true", })); -let ensureOpenClawCliOnPath: typeof import("./path-env.js").ensureOpenClawCliOnPath; - describe("ensureOpenClawCliOnPath", () => { const envKeys = [ "PATH", @@ -51,10 +56,6 @@ describe("ensureOpenClawCliOnPath", () => { ] as const; let envSnapshot: Record<(typeof envKeys)[number], string | undefined>; - beforeAll(async () => { - ({ ensureOpenClawCliOnPath } = await import("./path-env.js")); - }); - beforeEach(() => { envSnapshot = Object.fromEntries(envKeys.map((k) => [k, process.env[k]])) as typeof envSnapshot; state.dirs.clear(); diff --git a/src/infra/provider-usage.fetch.shared.test.ts b/src/infra/provider-usage.fetch.shared.test.ts index b287f1fad04..cfda93e39e9 100644 --- a/src/infra/provider-usage.fetch.shared.test.ts +++ b/src/infra/provider-usage.fetch.shared.test.ts @@ -9,7 +9,6 @@ import { describe("provider usage fetch shared helpers", () => { afterEach(() => { - vi.useRealTimers(); vi.restoreAllMocks(); }); @@ -31,7 +30,6 @@ describe("provider usage fetch shared helpers", () => { }); it("forwards request init and clears the timeout on success", async () => { - vi.useFakeTimers(); const clearTimeoutSpy = vi.spyOn(globalThis, "clearTimeout"); const fetchFnMock = vi.fn( async (_input: URL | RequestInfo, init?: RequestInit) => @@ -62,7 +60,6 @@ describe("provider usage fetch shared helpers", () => { }); it("aborts timed out requests and clears the timer on rejection", async () => { - vi.useFakeTimers(); const clearTimeoutSpy = vi.spyOn(globalThis, "clearTimeout"); const fetchFnMock = vi.fn( (_input: URL | RequestInfo, init?: RequestInit) => @@ -74,11 +71,9 @@ describe("provider usage fetch shared helpers", () => { ); const fetchFn = withFetchPreconnect(fetchFnMock); - const request = fetchJson("https://example.com/usage", {}, 50, fetchFn); - const rejection = expect(request).rejects.toThrow("aborted by timeout"); - await vi.advanceTimersByTimeAsync(50); - - await rejection; + await expect(fetchJson("https://example.com/usage", {}, 10, fetchFn)).rejects.toThrow( + "aborted by timeout", + ); expect(clearTimeoutSpy).toHaveBeenCalledTimes(1); }); diff --git a/src/media-understanding/image.test.ts b/src/media-understanding/image.test.ts index 096b1824482..11c6c07bfef 100644 --- a/src/media-understanding/image.test.ts +++ b/src/media-understanding/image.test.ts @@ -37,7 +37,8 @@ vi.mock("@mariozechner/pi-ai", async (importOriginal) => { }; }); -vi.mock("../agents/models-config.js", () => ({ +vi.mock("../agents/models-config.js", async (importOriginal) => ({ + ...(await importOriginal()), ensureOpenClawModelsJson: ensureOpenClawModelsJsonMock, })); @@ -54,7 +55,7 @@ vi.mock("../agents/pi-model-discovery-runtime.js", () => ({ discoverModels: discoverModelsMock, })); -let describeImageWithModel: typeof import("./image.js").describeImageWithModel; +const { describeImageWithModel } = await import("./image.js"); describe("describeImageWithModel", () => { afterEach(() => { @@ -62,31 +63,8 @@ describe("describeImageWithModel", () => { vi.restoreAllMocks(); }); - beforeEach(async () => { - vi.resetModules(); + beforeEach(() => { vi.stubGlobal("fetch", fetchMock); - vi.doMock("@mariozechner/pi-ai", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - complete: completeMock, - }; - }); - vi.doMock("../agents/models-config.js", () => ({ - ensureOpenClawModelsJson: ensureOpenClawModelsJsonMock, - })); - vi.doMock("../agents/model-auth.js", () => ({ - getApiKeyForModel: getApiKeyForModelMock, - resolveApiKeyForProvider: resolveApiKeyForProviderMock, - requireApiKey: requireApiKeyMock, - })); - vi.doMock("../agents/pi-model-discovery-runtime.js", () => ({ - discoverAuthStorage: () => ({ - setRuntimeApiKey: setRuntimeApiKeyMock, - }), - discoverModels: discoverModelsMock, - })); - ({ describeImageWithModel } = await import("./image.js")); vi.clearAllMocks(); fetchMock.mockResolvedValue({ ok: true, @@ -248,10 +226,10 @@ describe("describeImageWithModel", () => { it("normalizes deprecated google flash ids before lookup and keeps profile auth selection", async () => { const findMock = vi.fn((provider: string, modelId: string) => { expect(provider).toBe("google"); - expect(modelId).toBe("gemini-3-flash-preview"); + expect(modelId).toBe("gemini-3.1-flash-preview"); return { provider: "google", - id: "gemini-3-flash-preview", + id: "gemini-3.1-flash-preview", input: ["text", "image"], baseUrl: "https://generativelanguage.googleapis.com/v1beta", }; @@ -261,7 +239,7 @@ describe("describeImageWithModel", () => { role: "assistant", api: "google-generative-ai", provider: "google", - model: "gemini-3-flash-preview", + model: "gemini-3.1-flash-preview", stopReason: "stop", timestamp: Date.now(), content: [{ type: "text", text: "flash ok" }], @@ -282,7 +260,7 @@ describe("describeImageWithModel", () => { expect(result).toEqual({ text: "flash ok", - model: "gemini-3-flash-preview", + model: "gemini-3.1-flash-preview", }); expect(findMock).toHaveBeenCalledOnce(); expect(getApiKeyForModelMock).toHaveBeenCalledWith( @@ -296,10 +274,10 @@ describe("describeImageWithModel", () => { it("normalizes gemini 3.1 flash-lite ids before lookup and keeps profile auth selection", async () => { const findMock = vi.fn((provider: string, modelId: string) => { expect(provider).toBe("google"); - expect(modelId).toBe("gemini-3.1-flash-lite-preview"); + expect(modelId).toBe("gemini-3.1-flash-lite"); return { provider: "google", - id: "gemini-3.1-flash-lite-preview", + id: "gemini-3.1-flash-lite", input: ["text", "image"], baseUrl: "https://generativelanguage.googleapis.com/v1beta", }; @@ -309,7 +287,7 @@ describe("describeImageWithModel", () => { role: "assistant", api: "google-generative-ai", provider: "google", - model: "gemini-3.1-flash-lite-preview", + model: "gemini-3.1-flash-lite", stopReason: "stop", timestamp: Date.now(), content: [{ type: "text", text: "flash lite ok" }], @@ -330,7 +308,7 @@ describe("describeImageWithModel", () => { expect(result).toEqual({ text: "flash lite ok", - model: "gemini-3.1-flash-lite-preview", + model: "gemini-3.1-flash-lite", }); expect(findMock).toHaveBeenCalledOnce(); expect(getApiKeyForModelMock).toHaveBeenCalledWith(