From 37bff572435208d09b5b7403edf638c88871cc14 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 11 Nov 2026 23:20:14 -0500 Subject: [PATCH 1/3] fix(memory): release held lock files on exit and SIGINT/SIGTERM withFileLock releases its lock in a finally, which never runs when a signal ends the process, so a stopped server stranded memory.jsonl.lock and the next writer waited for it to go stale (the full 30 s from another container). Track held locks and remove those still ours synchronously on exit; SIGINT and SIGTERM handlers exit with the default status so the exit hook runs. The Dockerfile stops with SIGINT, as everything's does. Closes #5485 Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .changeset/memory-lock-exit-cleanup.md | 5 + src/memory/Dockerfile | 6 + src/memory/README.md | 1 + src/memory/__tests__/file-lock.test.ts | 167 ++++++++++++++++++++++++- src/memory/index.ts | 64 +++++++++- 5 files changed, 239 insertions(+), 4 deletions(-) create mode 100644 .changeset/memory-lock-exit-cleanup.md diff --git a/.changeset/memory-lock-exit-cleanup.md b/.changeset/memory-lock-exit-cleanup.md new file mode 100644 index 0000000000..54e857fdb8 --- /dev/null +++ b/.changeset/memory-lock-exit-cleanup.md @@ -0,0 +1,5 @@ +--- +"@modelcontextprotocol/server-memory": patch +--- + +Remove the memory file lock the server holds when it is stopped by SIGINT or SIGTERM, or exits, so the next writer does not wait for it to go stale. diff --git a/src/memory/Dockerfile b/src/memory/Dockerfile index 27eca1f8b6..b024edb016 100644 --- a/src/memory/Dockerfile +++ b/src/memory/Dockerfile @@ -72,4 +72,10 @@ WORKDIR /app # not root, and refuses to start the container under `runAsNonRoot`. USER 1000:1000 +# The server handles SIGTERM itself, removing any lock it holds before it +# exits (#5485). Node running as PID 1 would otherwise ignore `docker stop`'s +# SIGTERM, leaving it to wait out its timeout and SIGKILL the server. SIGINT +# is handled the same way; it is the stop signal everything's image uses. +STOPSIGNAL SIGINT + ENTRYPOINT ["node", "dist/index.js"] diff --git a/src/memory/README.md b/src/memory/README.md index d519f14f3e..43e7e952bb 100644 --- a/src/memory/README.md +++ b/src/memory/README.md @@ -267,6 +267,7 @@ Saves are atomic. Every write tool writes the whole graph to a temporary file in Each MCP client starts its own server process, so two clients using the same `MEMORY_FILE_PATH` (or both using the default) are two processes writing one file. Every write tool takes an exclusive lock file, `.lock`, next to the memory file while it reads, changes and saves the graph, so writes from different processes take turns and none is lost. The lock file exists only while a write is in progress, so the directory holding the memory file must be writable (it already must be, for the atomic save). +- A server stopped mid-write by `SIGINT` or `SIGTERM` (Ctrl-C, `docker stop`, a client shutting it down), or exiting normally, removes the lock it holds before it exits, so the next writer does not wait on it. - A lock left behind by a server that crashed mid-write is removed automatically: at once if its process is known to have exited on the same machine, otherwise once it has gone 30 seconds without being refreshed (a server holding the lock refreshes it every 15 seconds while it works). Only one waiting server breaks a stale lock at a time, guarded by a momentary `.lock.break` file. - A write that cannot get the lock within 60 seconds fails with a tool error rather than overwriting the file. - The lock relies on exclusive file creation, which local filesystems provide; a network filesystem that does not honour it cannot be shared safely this way. diff --git a/src/memory/__tests__/file-lock.test.ts b/src/memory/__tests__/file-lock.test.ts index db55503c9c..f1570e729e 100644 --- a/src/memory/__tests__/file-lock.test.ts +++ b/src/memory/__tests__/file-lock.test.ts @@ -1,12 +1,20 @@ // Unit tests for withFileLock, the lock file that orders mutations from // separate server processes sharing one graph file (#4797): mutual exclusion, -// release, breaking an abandoned lock, and giving up on a held one. +// release, breaking an abandoned lock, and giving up on a held one; and the +// exit-time release of the locks this process still holds (#5485). import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { promises as fs } from "fs"; import os from "os"; import path from "path"; -import { spawnSync } from "child_process"; -import { KnowledgeGraphManager, withFileLock } from "../index.js"; +import { spawn, spawnSync } from "child_process"; +import { fileURLToPath, pathToFileURL } from "url"; +import { + KnowledgeGraphManager, + heldLockPaths, + installLockCleanup, + releaseHeldLocks, + withFileLock, +} from "../index.js"; import { makeTempGraph } from "./helpers.js"; const fast = { staleMs: 60_000, timeoutMs: 100, retryMs: 5 }; @@ -401,3 +409,156 @@ describe("withFileLock", () => { expect(await fs.readdir(dir)).toEqual(["memory.jsonl"]); }); }); + +describe("releasing held locks on exit (#5485)", () => { + let dir: string; + let cleanup: () => Promise; + + beforeEach(async () => { + ({ dir, cleanup } = await makeTempGraph()); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await cleanup(); + }); + + async function exists(file: string): Promise { + return fs.access(file).then( + () => true, + () => false, + ); + } + + it("tracks a lock only while it is held", async () => { + const lockPath = path.join(dir, "memory.jsonl.lock"); + await withFileLock(lockPath, async () => { + expect(heldLockPaths()).toEqual([lockPath]); + }); + expect(heldLockPaths()).toEqual([]); + }); + + it("removes each lock still held, and only those still ours", async () => { + const ours = path.join(dir, "ours.lock"); + const taken = path.join(dir, "taken.lock"); + const gone = path.join(dir, "gone.lock"); + await withFileLock(ours, () => + withFileLock(taken, () => + withFileLock(gone, async () => { + // Another process broke and re-took one lock, and another vanished. + await fs.writeFile(taken, "someone else"); + await fs.unlink(gone); + + releaseHeldLocks(); + + expect(heldLockPaths()).toEqual([]); + expect(await exists(ours)).toBe(false); + expect(await fs.readFile(taken, "utf-8")).toBe("someone else"); + }), + ), + ); + expect(await fs.readFile(taken, "utf-8")).toBe("someone else"); + }); + + it("keeps tracking a lock the same process re-took after breaking it", async () => { + const lockPath = path.join(dir, "memory.jsonl.lock"); + let releaseFirst!: () => void; + const firstHolds = new Promise((resolve) => (releaseFirst = resolve)); + let firstStarted!: () => void; + const started = new Promise((resolve) => (firstStarted = resolve)); + vi.spyOn(console, "error").mockImplementation(() => {}); + + const first = withFileLock( + lockPath, + async () => { + firstStarted(); + await firstHolds; + }, + { staleMs: 60_000 }, + ); + await started; + // Age the first holder's lock past a short staleMs, so a second holder in + // this process breaks it and takes it over. + const old = new Date(Date.now() - 10_000); + await fs.utimes(lockPath, old, old); + await withFileLock( + lockPath, + async () => { + releaseFirst(); + await first; + // The first holder's release left the second's lock, and its entry. + expect(heldLockPaths()).toEqual([lockPath]); + expect(await exists(lockPath)).toBe(true); + }, + { staleMs: 1_000, timeoutMs: 1_000, retryMs: 5 }, + ); + expect(heldLockPaths()).toEqual([]); + }); + + it("installs an exit hook and SIGINT and SIGTERM handlers, once", () => { + const handlers = new Map void>(); + const on = vi.spyOn(process, "on").mockImplementation((event, listener) => { + handlers.set(event, listener as () => void); + return process; + }); + const exit = vi + .spyOn(process, "exit") + .mockImplementation((() => undefined) as typeof process.exit); + + installLockCleanup(); + installLockCleanup(); + + expect(on).toHaveBeenCalledTimes(3); + expect(handlers.get("exit")).toBe(releaseHeldLocks); + handlers.get("SIGINT")?.(); + expect(exit).toHaveBeenLastCalledWith(130); + handlers.get("SIGTERM")?.(); + expect(exit).toHaveBeenLastCalledWith(143); + }); + + // A real signal, sent to a real process holding a lock mid-operation. + // Windows has no SIGTERM to deliver: process.kill() there ends the process + // outright, so no handler can run. + it.skipIf(process.platform === "win32")( + "removes the lock when SIGTERM stops a process mid-mutation", + async () => { + const lockPath = path.join(dir, "memory.jsonl.lock"); + const distIndex = pathToFileURL( + path.join( + path.dirname(fileURLToPath(import.meta.url)), + "..", + "dist", + "index.js", + ), + ).href; + const script = ` + const { installLockCleanup, withFileLock } = await import(${JSON.stringify(distIndex)}); + installLockCleanup(); + await withFileLock(${JSON.stringify(lockPath)}, () => { + console.log("held"); + return new Promise(() => setInterval(() => {}, 1000)); + }); + `; + const child = spawn( + process.execPath, + ["--input-type=module", "-e", script], + { stdio: ["ignore", "pipe", "inherit"] }, + ); + const exited = new Promise((resolve) => + child.on("exit", (code) => resolve(code)), + ); + await new Promise((resolve, reject) => { + child.stdout.on("data", (data: Buffer) => { + if (data.toString().includes("held")) resolve(); + }); + child.on("exit", () => reject(new Error("exited before holding"))); + }); + expect(await exists(lockPath)).toBe(true); + + child.kill("SIGTERM"); + + expect(await exited).toBe(143); + expect(await exists(lockPath)).toBe(false); + }, + ); +}); diff --git a/src/memory/index.ts b/src/memory/index.ts index e57bf12ed3..421f64aa44 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -15,7 +15,7 @@ import { import { McpServer } from "@modelcontextprotocol/server"; import type { Transport } from "@modelcontextprotocol/server"; import { z } from "zod"; -import { promises as fs, realpathSync } from "fs"; +import { promises as fs, readFileSync, realpathSync, unlinkSync } from "fs"; import type { FileHandle } from "fs/promises"; import path from "path"; import os from "os"; @@ -276,6 +276,60 @@ async function breakStaleLock( } } +// The lock files this process holds right now, each mapped to the token it +// wrote into it. withFileLock releases its lock in a `finally`, which never +// runs when a signal ends the process, so the exit hook releaseHeldLocks() +// removes them instead (#5485). Without it, a lock left by a stopped server +// stalls the next writer until it goes stale, which takes the full staleMs +// when that writer is on another host (another container) and cannot see +// that this process has gone. +const heldLocks = new Map(); + +// The lock paths this process holds right now, for tests. +export function heldLockPaths(): string[] { + return [...heldLocks.keys()]; +} + +// Synchronously remove every lock this process still holds, as the process +// exits: an `exit` listener cannot wait on a promise. As in withFileLock's own +// release, a lock is removed only while it still holds this process's token, +// so a lock broken as stale and re-taken by another process stays in place. +export function releaseHeldLocks(): void { + for (const [lockPath, token] of heldLocks) { + try { + if (readFileSync(lockPath, "utf-8") === token) { + unlinkSync(lockPath); + } + } catch { + // Already gone, or unreadable: there is nothing of ours to remove. + } + } + heldLocks.clear(); +} + +// The exit codes a shell reports for a process its signal ended: 128 + the +// signal's number. +const SIGNAL_EXIT_CODES = { SIGINT: 130, SIGTERM: 143 } as const; + +let lockCleanupInstalled = false; + +// Release held locks however the server process ends (#5485): on a normal +// exit or process.exit() through the `exit` event, and on SIGINT or SIGTERM, +// whose default action would end the process without emitting `exit`. A +// signal handler replaces that default, so it exits itself, with the status +// the default would have given. Installed once, by main(): only the server +// process should take over its signals, not a test that imports this module. +export function installLockCleanup(): void { + if (lockCleanupInstalled) { + return; + } + lockCleanupInstalled = true; + process.on("exit", releaseHeldLocks); + for (const [signal, code] of Object.entries(SIGNAL_EXIT_CODES)) { + process.on(signal, () => process.exit(code)); + } +} + // Run operation while holding an exclusive lock file at lockPath, so that // mutations from separate server processes sharing one graph file take turns // and each one loads the graph the previous one saved (#4797). The lock is a @@ -370,11 +424,18 @@ export async function withFileLock( .catch(ignoreError); }, staleMs / 2); heartbeat.unref(); + heldLocks.set(lockPath, token); try { return await operation(); } finally { clearInterval(heartbeat); + // Stop tracking the lock before releasing it. A lock this same process + // broke as stale and re-took in another withFileLock call carries that + // call's token, and stays tracked for it. + if (heldLocks.get(lockPath) === token) { + heldLocks.delete(lockPath); + } // Release only our own lock: if it was broken as stale while we held it, // the file may now belong to another process. const current = await fs.readFile(lockPath, "utf-8").catch(() => null); @@ -1435,6 +1496,7 @@ export async function main(transport?: Transport): Promise { if (locationWarning) { console.error(locationWarning); } + installLockCleanup(); const handle = serveStdio(({ era }) => createServer(memoryFilePath, era), { transport, // serveStdio reports a transport that fails, including one that cannot From 72d35dbb27a692078e7aba4fa6063e07bf6dc6a0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 11 Nov 2026 23:34:49 -0500 Subject: [PATCH 2/3] fix(memory): track a lock from its create until its release completes Copilot round 1 on #5530: a signal during the token write or during the release found no tracked entry and left the lock. Track it as soon as the exclusive open returns, treat a file holding a prefix of our token as ours at exit, and untrack only after the release's unlink. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/memory/__tests__/file-lock.test.ts | 45 ++++++++++++++++++++++++++ src/memory/index.ts | 24 +++++++++----- 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/src/memory/__tests__/file-lock.test.ts b/src/memory/__tests__/file-lock.test.ts index f1570e729e..4773fa5b8b 100644 --- a/src/memory/__tests__/file-lock.test.ts +++ b/src/memory/__tests__/file-lock.test.ts @@ -460,6 +460,51 @@ describe("releasing held locks on exit (#5485)", () => { expect(await fs.readFile(taken, "utf-8")).toBe("someone else"); }); + it("removes a lock whose token was still being written at exit", async () => { + const lockPath = path.join(dir, "memory.jsonl.lock"); + const realOpen = fs.open; + let finishWrite!: () => void; + let writing!: () => void; + const writeStarted = new Promise((resolve) => (writing = resolve)); + vi.spyOn(fs, "open").mockImplementationOnce(async (...args) => { + const handle = await realOpen(...args); + const realWrite = handle.writeFile.bind(handle); + // Hold the token write open, with nothing written yet. + handle.writeFile = async (data) => { + writing(); + await new Promise((resolve) => (finishWrite = resolve)); + return realWrite(data); + }; + return handle; + }); + + const held = withFileLock(lockPath, async () => "ran"); + await writeStarted; + expect(await fs.readFile(lockPath, "utf-8")).toBe(""); + + releaseHeldLocks(); + + expect(await exists(lockPath)).toBe(false); + finishWrite(); + expect(await held).toBe("ran"); + }); + + it("still tracks a lock while its release is in progress", async () => { + const lockPath = path.join(dir, "memory.jsonl.lock"); + const realUnlink = fs.unlink; + let trackedDuringRelease: string[] = []; + await withFileLock(lockPath, async () => { + // The release's removal of the lock is the next unlink. + vi.spyOn(fs, "unlink").mockImplementationOnce(async (file) => { + trackedDuringRelease = heldLockPaths(); + return realUnlink(file); + }); + }); + expect(trackedDuringRelease).toEqual([lockPath]); + expect(heldLockPaths()).toEqual([]); + expect(await exists(lockPath)).toBe(false); + }); + it("keeps tracking a lock the same process re-took after breaking it", async () => { const lockPath = path.join(dir, "memory.jsonl.lock"); let releaseFirst!: () => void; diff --git a/src/memory/index.ts b/src/memory/index.ts index 421f64aa44..1c7be3f1b6 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -294,10 +294,13 @@ export function heldLockPaths(): string[] { // exits: an `exit` listener cannot wait on a promise. As in withFileLock's own // release, a lock is removed only while it still holds this process's token, // so a lock broken as stale and re-taken by another process stays in place. +// A lock is tracked from the moment its exclusive create returns, before its +// token is written, so a file holding only the start of the token (or none of +// it) is ours too: the exit interrupted the write. export function releaseHeldLocks(): void { for (const [lockPath, token] of heldLocks) { try { - if (readFileSync(lockPath, "utf-8") === token) { + if (token.startsWith(readFileSync(lockPath, "utf-8"))) { unlinkSync(lockPath); } } catch { @@ -359,6 +362,10 @@ export async function withFileLock( } } if (handle) { + // Track the lock before anything else is awaited: a signal is handled + // only between tasks, never between the open's completion and this + // line, so from here on the exit hook can remove it (#5485). + heldLocks.set(lockPath, token); try { await handle.writeFile(token); await handle.close(); @@ -367,6 +374,7 @@ export async function withFileLock( // every other writer waiting for it to go stale. await handle.close().catch(ignoreError); await fs.unlink(lockPath).catch(ignoreError); + heldLocks.delete(lockPath); throw error; } break; @@ -424,24 +432,24 @@ export async function withFileLock( .catch(ignoreError); }, staleMs / 2); heartbeat.unref(); - heldLocks.set(lockPath, token); try { return await operation(); } finally { clearInterval(heartbeat); - // Stop tracking the lock before releasing it. A lock this same process - // broke as stale and re-took in another withFileLock call carries that - // call's token, and stays tracked for it. - if (heldLocks.get(lockPath) === token) { - heldLocks.delete(lockPath); - } // Release only our own lock: if it was broken as stale while we held it, // the file may now belong to another process. const current = await fs.readFile(lockPath, "utf-8").catch(() => null); if (current === token) { await fs.unlink(lockPath).catch(ignoreError); } + // Stop tracking the lock only once the release is done, so an exit during + // it still removes the lock. A lock this same process broke as stale and + // re-took in another withFileLock call carries that call's token, and + // stays tracked for it. + if (heldLocks.get(lockPath) === token) { + heldLocks.delete(lockPath); + } } } From 4efde404fe4757fe2c825f5801a64c565a0ac38d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 11 Nov 2026 23:49:08 -0500 Subject: [PATCH 3/3] fix(memory): match a written lock's token exactly at exit Copilot round 2 on #5530: once our token is written, a prefix match also matched another writer's empty replacement lock, letting exit cleanup remove it. Record whether the token write finished; only an unwritten lock, which cannot yet have been judged stale and replaced, takes the prefix match. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- src/memory/__tests__/file-lock.test.ts | 13 +++++++++++++ src/memory/index.ts | 27 ++++++++++++++++++-------- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/memory/__tests__/file-lock.test.ts b/src/memory/__tests__/file-lock.test.ts index 4773fa5b8b..a436e6708f 100644 --- a/src/memory/__tests__/file-lock.test.ts +++ b/src/memory/__tests__/file-lock.test.ts @@ -489,6 +489,19 @@ describe("releasing held locks on exit (#5485)", () => { expect(await held).toBe("ran"); }); + it("leaves another writer's empty replacement of a written lock in place", async () => { + const lockPath = path.join(dir, "memory.jsonl.lock"); + await withFileLock(lockPath, async () => { + // Our lock was broken as stale, and a new writer has created its own + // but not yet written its token. + await fs.writeFile(lockPath, ""); + + releaseHeldLocks(); + + expect(await exists(lockPath)).toBe(true); + }); + }); + it("still tracks a lock while its release is in progress", async () => { const lockPath = path.join(dir, "memory.jsonl.lock"); const realUnlink = fs.unlink; diff --git a/src/memory/index.ts b/src/memory/index.ts index 1c7be3f1b6..a94fd94a08 100644 --- a/src/memory/index.ts +++ b/src/memory/index.ts @@ -277,13 +277,17 @@ async function breakStaleLock( } // The lock files this process holds right now, each mapped to the token it -// wrote into it. withFileLock releases its lock in a `finally`, which never +// writes into it and whether that write has finished. withFileLock releases its lock in a `finally`, which never // runs when a signal ends the process, so the exit hook releaseHeldLocks() // removes them instead (#5485). Without it, a lock left by a stopped server // stalls the next writer until it goes stale, which takes the full staleMs // when that writer is on another host (another container) and cannot see // that this process has gone. -const heldLocks = new Map(); +interface HeldLock { + token: string; + written: boolean; +} +const heldLocks = new Map(); // The lock paths this process holds right now, for tests. export function heldLockPaths(): string[] { @@ -295,12 +299,17 @@ export function heldLockPaths(): string[] { // release, a lock is removed only while it still holds this process's token, // so a lock broken as stale and re-taken by another process stays in place. // A lock is tracked from the moment its exclusive create returns, before its -// token is written, so a file holding only the start of the token (or none of -// it) is ours too: the exit interrupted the write. +// token is written. Until that write finishes, a file holding only the start +// of the token (or none of it) is ours too: the exit interrupted the write. +// Nothing else can have replaced it by then, since a lock that new, with a +// live owner, is never judged stale. Once the token is written only an exact +// match is ours, so another writer's replacement, empty while its own token +// is being written, is left alone. export function releaseHeldLocks(): void { - for (const [lockPath, token] of heldLocks) { + for (const [lockPath, { token, written }] of heldLocks) { try { - if (token.startsWith(readFileSync(lockPath, "utf-8"))) { + const contents = readFileSync(lockPath, "utf-8"); + if (written ? contents === token : token.startsWith(contents)) { unlinkSync(lockPath); } } catch { @@ -365,9 +374,11 @@ export async function withFileLock( // Track the lock before anything else is awaited: a signal is handled // only between tasks, never between the open's completion and this // line, so from here on the exit hook can remove it (#5485). - heldLocks.set(lockPath, token); + const held: HeldLock = { token, written: false }; + heldLocks.set(lockPath, held); try { await handle.writeFile(token); + held.written = true; await handle.close(); } catch (error) { // The lock was created but not written: remove it rather than leave @@ -447,7 +458,7 @@ export async function withFileLock( // it still removes the lock. A lock this same process broke as stale and // re-took in another withFileLock call carries that call's token, and // stays tracked for it. - if (heldLocks.get(lockPath) === token) { + if (heldLocks.get(lockPath)?.token === token) { heldLocks.delete(lockPath); } }