fix(memory): harden atomic reindex cleanup

steipete · steipete · commit 53a97fe0a76a · 2026-05-09T14:43:32.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -158,6 +158,7 @@ Docs: https://docs.openclaw.ai
 
 ### Fixes
 
+- Memory: close temp SQLite handles before failed atomic reindex cleanup and retry Windows EBUSY/EPERM/EACCES temp file removals, so `memory index --force` does not abort or leave temp sidecars on locked filesystems. Fixes #79708. Thanks @LobsterFarmerAmp and @hclsys.
 - Agents/CLI: handle resumed CLI JSONL output and bound supervisor output buffering so resumed runs stay readable without letting noisy child output grow unbounded.
 - Agents/sandbox: include the container workspace path hint in sandbox-root escape errors while preserving shortened host workspace roots. Fixes #79712. Thanks @haumanto and @hclsys.
 - Image generation: honor configured web-fetch SSRF policy across OpenAI, Google, MiniMax, OpenRouter, and Vydra provider requests so RFC2544 fake-IP proxy opt-ins reach generation calls. Fixes #79716. (#79765) Thanks @hclsys.
diff --git a/extensions/memory-core/src/memory/manager-atomic-reindex.ts b/extensions/memory-core/src/memory/manager-atomic-reindex.ts
@@ -8,30 +8,48 @@ type MemoryIndexFileOps = {
   wait: (ms: number) => Promise<void>;
 };
 
-type MoveMemoryIndexFilesOptions = {
+type MemoryIndexFileOptions = {
   fileOps?: MemoryIndexFileOps;
   maxRenameAttempts?: number;
   renameRetryDelayMs?: number;
+  maxRemoveAttempts?: number;
+  removeRetryDelayMs?: number;
 };
 
+type ResolvedMemoryIndexFileOptions = Required<MemoryIndexFileOptions>;
+
 const defaultFileOps: MemoryIndexFileOps = {
   rename: fs.rename,
   rm: fs.rm,
   wait: sleep,
 };
 
-const transientRenameErrorCodes = new Set(["EBUSY", "EPERM", "EACCES"]);
+const transientFileErrorCodes = new Set(["EBUSY", "EPERM", "EACCES"]);
 const defaultMaxRenameAttempts = 6;
 const defaultRenameRetryDelayMs = 25;
+const defaultMaxRemoveAttempts = 10;
+const defaultRemoveRetryDelayMs = 50;
+
+function isTransientFileError(err: unknown): boolean {
+  return transientFileErrorCodes.has((err as NodeJS.ErrnoException).code ?? "");
+}
 
-function isTransientRenameError(err: unknown): boolean {
-  return transientRenameErrorCodes.has((err as NodeJS.ErrnoException).code ?? "");
+function resolveMemoryIndexFileOptions(
+  options: MemoryIndexFileOptions = {},
+): ResolvedMemoryIndexFileOptions {
+  return {
+    fileOps: options.fileOps ?? defaultFileOps,
+    maxRenameAttempts: Math.max(1, options.maxRenameAttempts ?? defaultMaxRenameAttempts),
+    renameRetryDelayMs: options.renameRetryDelayMs ?? defaultRenameRetryDelayMs,
+    maxRemoveAttempts: Math.max(1, options.maxRemoveAttempts ?? defaultMaxRemoveAttempts),
+    removeRetryDelayMs: options.removeRetryDelayMs ?? defaultRemoveRetryDelayMs,
+  };
 }
 
 async function renameWithRetry(
   source: string,
   target: string,
-  options: Required<MoveMemoryIndexFilesOptions>,
+  options: ResolvedMemoryIndexFileOptions,
 ): Promise<void> {
   for (let attempt = 1; attempt <= options.maxRenameAttempts; attempt++) {
     try {
@@ -41,7 +59,7 @@ async function renameWithRetry(
       if ((err as NodeJS.ErrnoException).code === "ENOENT") {
         return;
       }
-      if (!isTransientRenameError(err) || attempt === options.maxRenameAttempts) {
+      if (!isTransientFileError(err) || attempt === options.maxRenameAttempts) {
         throw err;
       }
       await options.fileOps.wait(options.renameRetryDelayMs * attempt);
@@ -53,13 +71,9 @@ async function renameWithRetry(
 export async function moveMemoryIndexFiles(
   sourceBase: string,
   targetBase: string,
-  options: MoveMemoryIndexFilesOptions = {},
+  options: MemoryIndexFileOptions = {},
 ): Promise<void> {
-  const resolvedOptions: Required<MoveMemoryIndexFilesOptions> = {
-    fileOps: options.fileOps ?? defaultFileOps,
-    maxRenameAttempts: Math.max(1, options.maxRenameAttempts ?? defaultMaxRenameAttempts),
-    renameRetryDelayMs: options.renameRetryDelayMs ?? defaultRenameRetryDelayMs,
-  };
+  const resolvedOptions = resolveMemoryIndexFileOptions(options);
   const suffixes = ["", "-wal", "-shm"];
   for (const suffix of suffixes) {
     const source = `${sourceBase}${suffix}`;
@@ -68,12 +82,33 @@ export async function moveMemoryIndexFiles(
   }
 }
 
-async function removeMemoryIndexFiles(
+async function rmWithRetry(path: string, options: ResolvedMemoryIndexFileOptions): Promise<void> {
+  for (let attempt = 1; attempt <= options.maxRemoveAttempts; attempt++) {
+    try {
+      await options.fileOps.rm(path, { force: true });
+      return;
+    } catch (err) {
+      if ((err as NodeJS.ErrnoException).code === "ENOENT") {
+        return;
+      }
+      if (!isTransientFileError(err) || attempt === options.maxRemoveAttempts) {
+        throw err;
+      }
+      await options.fileOps.wait(options.removeRetryDelayMs * attempt);
+    }
+  }
+  throw new Error("rm retry loop exited unexpectedly");
+}
+
+export async function removeMemoryIndexFiles(
   basePath: string,
-  fileOps: MemoryIndexFileOps = defaultFileOps,
+  options: MemoryIndexFileOptions = {},
 ): Promise<void> {
+  const resolvedOptions = resolveMemoryIndexFileOptions(options);
   const suffixes = ["", "-wal", "-shm"];
-  await Promise.all(suffixes.map((suffix) => fileOps.rm(`${basePath}${suffix}`, { force: true })));
+  for (const suffix of suffixes) {
+    await rmWithRetry(`${basePath}${suffix}`, resolvedOptions);
+  }
 }
 
 async function swapMemoryIndexFiles(targetPath: string, tempPath: string): Promise<void> {
@@ -92,13 +127,23 @@ export async function runMemoryAtomicReindex<T>(params: {
   targetPath: string;
   tempPath: string;
   build: () => Promise<T>;
+  beforeTempCleanup?: () => Promise<void> | void;
+  fileOptions?: MemoryIndexFileOptions;
 }): Promise<T> {
   try {
     const result = await params.build();
     await swapMemoryIndexFiles(params.targetPath, params.tempPath);
     return result;
   } catch (err) {
-    await removeMemoryIndexFiles(params.tempPath);
+    try {
+      await params.beforeTempCleanup?.();
+      await removeMemoryIndexFiles(params.tempPath, params.fileOptions);
+    } catch (cleanupErr) {
+      throw new AggregateError(
+        [err, cleanupErr],
+        "memory atomic reindex failed and temp cleanup failed",
+      );
+    }
     throw err;
   }
 }
diff --git a/extensions/memory-core/src/memory/manager-sync-ops.ts b/extensions/memory-core/src/memory/manager-sync-ops.ts
@@ -1178,6 +1178,7 @@ export abstract class MemoryManagerSyncOps {
     const tempDb = openMemoryDatabaseAtPath(tempDbPath, this.settings.store.vector.enabled);
 
     const originalDb = this.db;
+    let tempDbClosed = false;
     let originalDbClosed = false;
     const originalState = {
       ftsAvailable: this.fts.available,
@@ -1216,6 +1217,12 @@ export abstract class MemoryManagerSyncOps {
       nextMeta = await runMemoryAtomicReindex({
         targetPath: dbPath,
         tempPath: tempDbPath,
+        beforeTempCleanup: () => {
+          if (!tempDbClosed) {
+            closeMemoryDatabase(tempDb);
+            tempDbClosed = true;
+          }
+        },
         build: async () => {
           await this.seedEmbeddingCache(originalDb);
           const shouldSyncMemory = this.sources.has("memory");
@@ -1265,7 +1272,8 @@ export abstract class MemoryManagerSyncOps {
           this.writeMeta(meta);
           this.pruneEmbeddingCacheIfNeeded?.();
 
-          closeMemoryDatabase(this.db);
+          closeMemoryDatabase(tempDb);
+          tempDbClosed = true;
           closeMemoryDatabase(originalDb);
           originalDbClosed = true;
           return meta;
@@ -1278,7 +1286,10 @@ export abstract class MemoryManagerSyncOps {
       this.vector.dims = nextMeta?.vectorDims;
     } catch (err) {
       try {
-        closeMemoryDatabase(this.db);
+        if (!tempDbClosed && this.db === tempDb) {
+          closeMemoryDatabase(tempDb);
+          tempDbClosed = true;
+        }
       } catch {}
       restoreOriginalState();
       throw err;
diff --git a/extensions/memory-core/src/memory/manager.atomic-reindex.test.ts b/extensions/memory-core/src/memory/manager.atomic-reindex.test.ts
@@ -3,7 +3,11 @@ import os from "node:os";
 import path from "node:path";
 import { DatabaseSync } from "node:sqlite";
 import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
-import { moveMemoryIndexFiles, runMemoryAtomicReindex } from "./manager-atomic-reindex.js";
+import {
+  moveMemoryIndexFiles,
+  removeMemoryIndexFiles,
+  runMemoryAtomicReindex,
+} from "./manager-atomic-reindex.js";
 
 async function expectPathMissing(targetPath: string): Promise<void> {
   await expect(fs.access(targetPath)).rejects.toMatchObject({ code: "ENOENT" });
@@ -133,6 +137,101 @@ describe("memory manager atomic reindex", () => {
     expect(rename).toHaveBeenCalledTimes(1);
     expect(wait).not.toHaveBeenCalled();
   });
+
+  it.each(["EBUSY", "EPERM", "EACCES"] as const)(
+    "retries transient %s rm failures during index file cleanup",
+    async (code) => {
+      const calls: string[] = [];
+      const rm = vi.fn(async (filePath: string) => {
+        calls.push(filePath);
+        if (calls.length === 1) {
+          throw Object.assign(new Error("busy"), { code });
+        }
+      });
+      const wait = vi.fn().mockResolvedValue(undefined);
+
+      await removeMemoryIndexFiles("index.sqlite.tmp", {
+        fileOps: { rename: fs.rename, rm, wait },
+        maxRemoveAttempts: 3,
+        removeRetryDelayMs: 10,
+      });
+
+      expect(calls).toEqual([
+        "index.sqlite.tmp",
+        "index.sqlite.tmp",
+        "index.sqlite.tmp-wal",
+        "index.sqlite.tmp-shm",
+      ]);
+      expect(wait).toHaveBeenCalledTimes(1);
+      expect(wait).toHaveBeenCalledWith(10);
+    },
+  );
+
+  it("throws after exhausting transient rm retries", async () => {
+    const rm = vi.fn().mockRejectedValue(Object.assign(new Error("busy"), { code: "EBUSY" }));
+    const wait = vi.fn().mockResolvedValue(undefined);
+
+    await expect(
+      removeMemoryIndexFiles("index.sqlite.tmp", {
+        fileOps: { rename: fs.rename, rm, wait },
+        maxRemoveAttempts: 3,
+        removeRetryDelayMs: 10,
+      }),
+    ).rejects.toMatchObject({ code: "EBUSY" });
+
+    expect(rm).toHaveBeenCalledTimes(3);
+    expect(wait).toHaveBeenCalledTimes(2);
+    expect(wait).toHaveBeenNthCalledWith(1, 10);
+    expect(wait).toHaveBeenNthCalledWith(2, 20);
+  });
+
+  it("does not retry non-transient rm failures", async () => {
+    const rm = vi.fn().mockRejectedValue(Object.assign(new Error("invalid"), { code: "EINVAL" }));
+    const wait = vi.fn().mockResolvedValue(undefined);
+
+    await expect(
+      removeMemoryIndexFiles("index.sqlite.tmp", {
+        fileOps: { rename: fs.rename, rm, wait },
+        maxRemoveAttempts: 3,
+        removeRetryDelayMs: 10,
+      }),
+    ).rejects.toMatchObject({ code: "EINVAL" });
+
+    expect(rm).toHaveBeenCalledTimes(1);
+    expect(wait).not.toHaveBeenCalled();
+  });
+
+  it("closes temp resources before removing temp files after build failure", async () => {
+    const events: string[] = [];
+    let tempClosed = false;
+    const rm = vi.fn(async (filePath: string) => {
+      events.push(tempClosed ? `rm:${filePath}:closed` : `rm:${filePath}:open`);
+    });
+
+    await expect(
+      runMemoryAtomicReindex({
+        targetPath: "index.sqlite",
+        tempPath: "index.sqlite.tmp",
+        beforeTempCleanup: async () => {
+          events.push("close-temp");
+          tempClosed = true;
+        },
+        fileOptions: {
+          fileOps: { rename: fs.rename, rm, wait: vi.fn().mockResolvedValue(undefined) },
+        },
+        build: async () => {
+          throw new Error("embedding failure");
+        },
+      }),
+    ).rejects.toThrow("embedding failure");
+
+    expect(events).toEqual([
+      "close-temp",
+      "rm:index.sqlite.tmp:closed",
+      "rm:index.sqlite.tmp-wal:closed",
+      "rm:index.sqlite.tmp-shm:closed",
+    ]);
+  });
 });
 
 function writeChunkMarker(dbPath: string, marker: string): void {
