Skip to content

Commit 4aa40a2

Browse files
vincentkocvincentkoc
committed
fix(memory): harden stale reindex cleanup
1 parent 85158a7 commit 4aa40a2

3 files changed

Lines changed: 145 additions & 33 deletions

File tree

‎extensions/memory-core/src/memory/manager-db-probe.test.ts‎

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1+
import fsSync from "node:fs";
12
import fs from "node:fs/promises";
23
import os from "node:os";
34
import path from "node:path";
45
import { DatabaseSync } from "node:sqlite";
5-
import { afterAll, beforeAll, describe, expect, it } from "vitest";
6+
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
67
import { openMemoryDatabaseAtPath } from "./manager-db.js";
78

89
async function expectPathMissing(targetPath: string): Promise<void> {
@@ -21,6 +22,10 @@ describe("openMemoryDatabaseAtPath readOnly probe", () => {
2122
await fs.rm(fixtureRoot, { recursive: true, force: true });
2223
});
2324

25+
afterEach(() => {
26+
vi.restoreAllMocks();
27+
});
28+
2429
it("allows opening when the database file exists", async () => {
2530
const dbPath = path.join(fixtureRoot, `case-${caseId++}`, "index.sqlite");
2631
const dir = path.dirname(dbPath);
@@ -164,4 +169,73 @@ describe("openMemoryDatabaseAtPath readOnly probe", () => {
164169
await expectPathMissing(`${orphanBase}-shm`);
165170
await expectPathMissing(`${orphanBase}.lock`);
166171
});
172+
173+
it("removes an aged lock-only reindex orphan", async () => {
174+
const dbPath = path.join(fixtureRoot, `case-${caseId++}`, "index.sqlite");
175+
const dir = path.dirname(dbPath);
176+
await fs.mkdir(dir, { recursive: true });
177+
const seed = new DatabaseSync(dbPath);
178+
seed.close();
179+
180+
const orphanLock = `${dbPath}.tmp-87654321-aaaa-bbbb-cccc-123456789abc.lock`;
181+
await fs.writeFile(orphanLock, '{"pid":999999999}');
182+
const old = new Date(Date.now() - 60 * 60_000);
183+
await fs.utimes(orphanLock, old, old);
184+
185+
const db = openMemoryDatabaseAtPath(dbPath, false);
186+
db.close();
187+
188+
await expectPathMissing(orphanLock);
189+
});
190+
191+
it("keeps aged reindex temp files while the live database is absent", async () => {
192+
const dbPath = path.join(fixtureRoot, `case-${caseId++}`, "index.sqlite");
193+
await fs.mkdir(path.dirname(dbPath), { recursive: true });
194+
const orphanBase = `${dbPath}.tmp-abcdef12-aaaa-bbbb-cccc-123456789abc`;
195+
await fs.writeFile(orphanBase, "recovery candidate");
196+
const old = new Date(Date.now() - 48 * 60 * 60_000);
197+
await fs.utimes(orphanBase, old, old);
198+
199+
const db = openMemoryDatabaseAtPath(dbPath, false, true);
200+
db.close();
201+
202+
await expect(fs.access(orphanBase)).resolves.toBeUndefined();
203+
});
204+
205+
it("keeps aged reindex temp files when owner liveness is uncertain", async () => {
206+
const dbPath = path.join(fixtureRoot, `case-${caseId++}`, "index.sqlite");
207+
const dir = path.dirname(dbPath);
208+
await fs.mkdir(dir, { recursive: true });
209+
const seed = new DatabaseSync(dbPath);
210+
seed.close();
211+
212+
const activeBase = `${dbPath}.tmp-fedcba98-aaaa-bbbb-cccc-123456789abc`;
213+
await fs.writeFile(activeBase, "active");
214+
await fs.writeFile(`${activeBase}.lock`, '{"pid":12345}');
215+
const old = new Date(Date.now() - 60 * 60_000);
216+
await fs.utimes(activeBase, old, old);
217+
await fs.utimes(`${activeBase}.lock`, old, old);
218+
vi.spyOn(process, "kill").mockImplementation(() => {
219+
throw Object.assign(new Error("unknown process state"), { code: "EACCES" });
220+
});
221+
222+
const db = openMemoryDatabaseAtPath(dbPath, false);
223+
db.close();
224+
225+
await expect(fs.access(activeBase)).resolves.toBeUndefined();
226+
await expect(fs.access(`${activeBase}.lock`)).resolves.toBeUndefined();
227+
});
228+
229+
it("does not block database startup when orphan discovery fails", async () => {
230+
const dbPath = path.join(fixtureRoot, `case-${caseId++}`, "index.sqlite");
231+
await fs.mkdir(path.dirname(dbPath), { recursive: true });
232+
const seed = new DatabaseSync(dbPath);
233+
seed.close();
234+
vi.spyOn(fsSync, "readdirSync").mockImplementationOnce(() => {
235+
throw Object.assign(new Error("scan failed"), { code: "EACCES" });
236+
});
237+
238+
const db = openMemoryDatabaseAtPath(dbPath, false);
239+
db.close();
240+
});
167241
});

‎extensions/memory-core/src/memory/manager-db.ts‎

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@ const reindexTempFileMinAgeMs = 10 * 60_000;
1616
const reindexTempFileWithoutLockMinAgeMs = 24 * 60 * 60_000;
1717
const reindexTempUuidPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/;
1818
const memoryIndexFileSuffixes = ["", "-wal", "-shm"] as const;
19+
const reindexTempEntrySuffixes = [".lock", "-wal", "-shm", ""] as const;
1920

2021
function resolveReindexTempBaseName(dbBaseName: string, entryName: string): string | undefined {
21-
for (const suffix of memoryIndexFileSuffixes) {
22+
for (const suffix of reindexTempEntrySuffixes) {
2223
if (!entryName.endsWith(suffix)) {
2324
continue;
2425
}
@@ -48,20 +49,39 @@ function readReindexTempLockPid(lockPath: string): number | undefined {
4849
}
4950
}
5051

51-
function isProcessRunning(pid: number): boolean {
52+
function isProcessDefinitelyDead(pid: number): boolean {
5253
try {
5354
process.kill(pid, 0);
54-
return true;
55+
return false;
5556
} catch (err) {
56-
return (err as NodeJS.ErrnoException).code === "EPERM";
57+
return (err as NodeJS.ErrnoException).code === "ESRCH";
58+
}
59+
}
60+
61+
function isRegularFile(filePath: string): boolean {
62+
try {
63+
return fs.statSync(filePath).isFile();
64+
} catch {
65+
return false;
5766
}
5867
}
5968

6069
function cleanupAgedReindexTempFiles(dbPath: string, nowMs = Date.now()): void {
70+
// A missing live database can be the brief Windows swap window. Never delete
71+
// the only complete temp candidate while the canonical path is absent.
72+
if (!isRegularFile(dbPath)) {
73+
return;
74+
}
6175
const dir = path.dirname(dbPath);
6276
const dbBaseName = path.basename(dbPath);
6377
const tempBaseNames = new Set<string>();
64-
for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
78+
let entries: fs.Dirent[];
79+
try {
80+
entries = fs.readdirSync(dir, { withFileTypes: true });
81+
} catch {
82+
return;
83+
}
84+
for (const entry of entries) {
6585
if (!entry.isFile()) {
6686
continue;
6787
}
@@ -72,24 +92,34 @@ function cleanupAgedReindexTempFiles(dbPath: string, nowMs = Date.now()): void {
7292
}
7393

7494
for (const tempBaseName of tempBaseNames) {
95+
if (!isRegularFile(dbPath)) {
96+
return;
97+
}
7598
const lockPath = path.join(dir, `${tempBaseName}.lock`);
7699
const lockPid = readReindexTempLockPid(lockPath);
77-
if (lockPid && isProcessRunning(lockPid)) {
100+
// Cleanup is destructive, so an uncertain owner state is treated as live.
101+
if (lockPid && !isProcessDefinitelyDead(lockPid)) {
78102
continue;
79103
}
80104
const filePaths = [
81105
...memoryIndexFileSuffixes.map((suffix) => path.join(dir, `${tempBaseName}${suffix}`)),
82106
lockPath,
83107
];
84-
const stats = filePaths
85-
.map((filePath) => {
86-
try {
87-
return fs.statSync(filePath);
88-
} catch {
89-
return undefined;
108+
const stats: fs.Stats[] = [];
109+
let hasUnknownFileState = false;
110+
for (const filePath of filePaths) {
111+
try {
112+
stats.push(fs.statSync(filePath));
113+
} catch (err) {
114+
if ((err as NodeJS.ErrnoException).code !== "ENOENT") {
115+
hasUnknownFileState = true;
116+
break;
90117
}
91-
})
92-
.filter((stat): stat is fs.Stats => stat !== undefined);
118+
}
119+
}
120+
if (hasUnknownFileState) {
121+
continue;
122+
}
93123
if (stats.length === 0) {
94124
continue;
95125
}

‎extensions/memory-core/src/memory/manager-sync-ops.ts‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ import {
4545
type EmbeddingProviderId,
4646
type EmbeddingProviderRuntime,
4747
} from "./embeddings.js";
48-
import { runMemoryAtomicReindex } from "./manager-atomic-reindex.js";
48+
import { removeMemoryIndexFiles, runMemoryAtomicReindex } from "./manager-atomic-reindex.js";
4949
import { closeMemoryDatabase, openMemoryDatabaseAtPath } from "./manager-db.js";
5050
import { isMemoryEmbeddingOperationError } from "./manager-embedding-errors.js";
5151
import {
@@ -2306,11 +2306,9 @@ export abstract class MemoryManagerSyncOps {
23062306
const dbPath = resolveUserPath(this.settings.store.path);
23072307
const tempDbPath = `${dbPath}.tmp-${randomUUID()}`;
23082308
const tempLockPath = `${tempDbPath}.lock`;
2309-
fsSync.mkdirSync(path.dirname(tempLockPath), { recursive: true });
2310-
fsSync.writeFileSync(tempLockPath, JSON.stringify({ pid: process.pid, createdAt: Date.now() }));
2311-
const tempDb = openMemoryDatabaseAtPath(tempDbPath, this.settings.store.vector.enabled);
23122309

23132310
const originalDb = this.db;
2311+
let tempDb: DatabaseSync | undefined;
23142312
let tempDbClosed = false;
23152313
let originalDbClosed = false;
23162314
const originalState = {
@@ -2338,22 +2336,27 @@ export abstract class MemoryManagerSyncOps {
23382336
this.vectorReady = originalDbClosed ? null : originalState.vectorReady;
23392337
};
23402338

2341-
this.db = tempDb;
2342-
this.lastMetaSerialized = null;
2343-
this.resetVectorState();
2344-
this.fts.available = false;
2345-
this.fts.loadError = undefined;
2346-
this.ensureSchema();
2347-
2348-
let nextMeta: MemoryIndexMeta | null;
2349-
23502339
try {
2351-
nextMeta = await runMemoryAtomicReindex({
2340+
fsSync.mkdirSync(path.dirname(tempLockPath), { recursive: true });
2341+
fsSync.writeFileSync(
2342+
tempLockPath,
2343+
JSON.stringify({ pid: process.pid, createdAt: Date.now() }),
2344+
);
2345+
tempDb = openMemoryDatabaseAtPath(tempDbPath, this.settings.store.vector.enabled);
2346+
const openedTempDb = tempDb;
2347+
this.db = openedTempDb;
2348+
this.lastMetaSerialized = null;
2349+
this.resetVectorState();
2350+
this.fts.available = false;
2351+
this.fts.loadError = undefined;
2352+
this.ensureSchema();
2353+
2354+
const nextMeta = await runMemoryAtomicReindex({
23522355
targetPath: dbPath,
23532356
tempPath: tempDbPath,
23542357
beforeTempCleanup: () => {
23552358
if (!tempDbClosed) {
2356-
closeMemoryDatabase(tempDb);
2359+
closeMemoryDatabase(openedTempDb);
23572360
tempDbClosed = true;
23582361
}
23592362
},
@@ -2429,7 +2432,7 @@ export abstract class MemoryManagerSyncOps {
24292432
this.writeMeta(meta);
24302433
this.pruneEmbeddingCacheIfNeeded?.();
24312434

2432-
closeMemoryDatabase(tempDb);
2435+
closeMemoryDatabase(openedTempDb);
24332436
tempDbClosed = true;
24342437
closeMemoryDatabase(originalDb);
24352438
originalDbClosed = true;
@@ -2443,15 +2446,20 @@ export abstract class MemoryManagerSyncOps {
24432446
this.vector.dims = nextMeta?.vectorDims;
24442447
} catch (err) {
24452448
try {
2446-
if (!tempDbClosed && this.db === tempDb) {
2449+
if (tempDb && !tempDbClosed && this.db === tempDb) {
24472450
closeMemoryDatabase(tempDb);
24482451
tempDbClosed = true;
24492452
}
24502453
} catch {}
2454+
try {
2455+
await removeMemoryIndexFiles(tempDbPath);
2456+
} catch {}
24512457
restoreOriginalState();
24522458
throw err;
24532459
} finally {
2454-
await fs.rm(tempLockPath, { force: true });
2460+
try {
2461+
await fs.rm(tempLockPath, { force: true });
2462+
} catch {}
24552463
}
24562464
}
24572465

0 commit comments

Comments
 (0)