Skip to content

fix(memory): release held lock files on exit and SIGINT/SIGTERM - #5530

Merged
cliffhall merged 3 commits into
v2/mainfrom
v2/fix/5485-memory-lock-cleanup-on-exit
Nov 12, 2026
Merged

cliffhall merged 3 commits into
v2/mainfrom
v2/fix/5485-memory-lock-cleanup-on-exit

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #5485

Description

withFileLock releases its lock in a finally, which never runs when a signal ends the process. A memory server stopped mid-write (Ctrl-C, docker stop, a client shutting it down) therefore left memory.jsonl.lock behind, and the next writer waited for it to go stale: the full 30 s when that writer is in another container and cannot see that the owner's PID is gone.

  • withFileLock now records each lock it holds (path → its token) for as long as it holds it.
  • releaseHeldLocks() removes, synchronously, every held lock that still carries this process's token, so a lock another process broke and re-took is left alone, as the normal release already does.
  • installLockCleanup(), called once from main() (not at import, so tests that import the module keep their own signals), runs that on exit, and handles SIGINT/SIGTERM by exiting with the status the default action would have given (130/143), which fires the exit hook.
  • src/memory/Dockerfile sets STOPSIGNAL SIGINT, matching src/everything/Dockerfile. Node as PID 1 ignores a signal it has no handler for; with the handlers above both signals now work, and SIGINT keeps the two images consistent.

Server Details

  • Server: memory (@modelcontextprotocol/server-memory)
  • Changes to: process lifecycle (the lock file around each mutation), the Dockerfile, the README's locking section. No tool, resource or schema changes.

Motivation and Context

See #5485. The fix follows the prompt the reporter supplied there (credit to @Faris-as for the report and prototype), written fresh against v2/main.

How Has This Been Tested?

Unit tests (src/memory/__tests__/file-lock.test.ts, new describe block):

  • a lock is tracked only while held;
  • releaseHeldLocks() removes a lock still ours, leaves one another process re-took, and is quiet about one already gone;
  • a lock this same process broke and re-took stays tracked for the new holder when the old one releases;
  • installLockCleanup() registers exit, SIGINT and SIGTERM once, and the signal handlers exit 130 / 143;
  • a real child process holding a lock mid-operation is sent SIGTERM: it exits 143 and the lock is gone (skipped on Windows, which has no deliverable SIGTERM). Without installLockCleanup() the same script exits 143 and leaves the lock, so the test discriminates.

Inspector CLI (2.12.0), both eras, before/after. To stop the real bin mid-write, MEMORY_FILE_PATH is a FIFO, so create_entities blocks in its load while holding the lock; the server's PID is read from the lock file and signalled:

Inspector --cli node <dist>/index.js -e MEMORY_FILE_PATH=<dir>/memory.jsonl (a FIFO) \
  --protocol-era <legacy|modern> --method tools/call --tool-name create_entities \
  --tool-args-json '{"entities":[{"name":"Alice","entityType":"person","observations":[]}]}'
# while blocked: kill -<TERM|INT> <pid from memory.jsonl.lock>; ls <dir>
Build legacy, SIGTERM legacy, SIGINT modern, SIGTERM modern, SIGINT
Published 2.1.0 (before) memory.jsonl memory.jsonl.lock memory.jsonl memory.jsonl.lock memory.jsonl memory.jsonl.lock memory.jsonl memory.jsonl.lock
This branch (after) memory.jsonl memory.jsonl memory.jsonl memory.jsonl

A normal create_entities still succeeds in both eras (exit 0; the modern result carries io.modelcontextprotocol/serverInfo memory-server 2.1.1) and leaves only memory.jsonl.

LLM client: Claude Code 2.1.322 (claude -p --strict-mcp-config, the branch's dist/index.js); the era cannot be selected from this client. Prompt: "Using the memory server, create an entity named 'Ada' of type 'person' with the observation 'wrote the first program', then read the graph and reply with just the entity's observations." → - wrote the first program. The graph file held the entity and, after the client shut the server down, no lock file remained.

Gate: npm run local:gate exit 0.

Breaking Changes

None. Client configuration is unchanged. The server process now exits with status 130 on SIGINT and 143 on SIGTERM, which is what the default signal action reported to a shell before.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follow MCP security best practices
  • I have updated the server's README accordingly
  • I have added a changeset (npm run changeset) if this changes what a TypeScript server publishes
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options (not applicable: none added)

Additional context

SIGHUP is not handled: the issue asked for SIGINT/SIGTERM, and a stdio server whose client goes away sees its stdin close rather than a hangup. A hard kill (SIGKILL, OOM) still cannot run any handler; that case remains covered by the existing stale-lock breaking.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall cliffhall added the v2 label Nov 12, 2026
@cliffhall cliffhall linked an issue Nov 12, 2026 that may be closed by this pull request
2 tasks done
@changeset-bot

changeset-bot Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4efde40

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@modelcontextprotocol/server-memory Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cliffhall
cliffhall requested a balanced review from Copilot November 12, 2026 04:27
@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

memory: MCP interface diff

Base: 1a42b3b877dc (Merge pull request #5528 from modelcontextprotocol/v2/chore/5463-mcp-interface-diff-rename-step1)
Head: the build in this checkout, at 424333fab3ae (Merge 4efde40 into 1f8badb)

Diffed because it was named (--server).

✅ No interface changes detected.

Tools Prompts Resources Resource templates
Base 9 0 1 0
Head 9 0 1 0

Updated by this run for 4efde404fe4757fe2c825f5801a64c565a0ac38d.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Lock tracking gaps during acquisition and release can leave stale locks behind if SIGINT or SIGTERM arrives during filesystem operations, undermining the intended cleanup.

2 open issues
What changed in this PR

This PR addresses #5485 by adding shutdown cleanup for the memory server’s lock files, aiming to prevent stale-lock delays after termination.

Changes:

  • Tracks held lock tokens and installs synchronous cleanup on exit, SIGINT, and SIGTERM.
  • Sets Docker’s stop signal to SIGINT and documents shutdown behavior.
  • Adds lock ownership and signal-handling tests, plus a patch changeset.
File Description
src/​memory/​README.md Documents lock cleanup during shutdown.
src/​memory/​index.ts Adds held-lock tracking and process cleanup handlers.
src/​memory/​Dockerfile Sets SIGINT as the container stop signal.
src/​memory/​__tests__/​file-lock.test.ts Tests tracking, ownership preservation, and signal cleanup.
.changeset/​memory-lock-exit-cleanup.md Records the memory server patch release change.

Review effort: Balanced

Comment thread src/memory/index.ts Resolved
Comment thread src/memory/index.ts Resolved
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 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 1: two findings, both fixed in 72d35db (each is a signal window in what this PR added).

  • Register before acquisition can be interrupted: the lock is now tracked as soon as the exclusive open returns, and at exit a file holding a prefix of our token (or nothing) counts as ours. New test: a lock whose token write is pending is removed by releaseHeldLocks().
  • Keep the lock registered until release completes: untracking now follows the release's unlink. New test: the lock is still tracked during the release's unlink.

Gate re-run: exit 0. Requesting round 2.

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

fetch: MCP interface diff

Base: 1a42b3b877dc (Merge pull request #5528 from modelcontextprotocol/v2/chore/5463-mcp-interface-diff-rename-step1)
Head: the build in this checkout, at 424333fab3ae (Merge 4efde40 into 1f8badb)

Diffed because it was named (--server).

✅ No interface changes detected.

Tools Prompts Resources Resource templates
Base 1 1 0 0
Head 1 1 0 0

Updated by this run for 4efde404fe4757fe2c825f5801a64c565a0ac38d.

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

everything: MCP interface diff

Base: 1a42b3b877dc (Merge pull request #5528 from modelcontextprotocol/v2/chore/5463-mcp-interface-diff-rename-step1)
Head: the build in this checkout, at 424333fab3ae (Merge 4efde40 into 1f8badb)

Diffed because it was named (--server).

✅ No interface changes detected.

Tools Prompts Resources Resource templates
Base 22 4 8 5
Head 22 4 8 5

Updated by this run for 4efde404fe4757fe2c825f5801a64c565a0ac38d.

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

git: MCP interface diff

Base: 1a42b3b877dc (Merge pull request #5528 from modelcontextprotocol/v2/chore/5463-mcp-interface-diff-rename-step1)
Head: the build in this checkout, at 424333fab3ae (Merge 4efde40 into 1f8badb)

Diffed because it was named (--server).

⚠️ Interface changes detected in 1 part(s) of the interface. Review them below to confirm they are intended.

Tools Prompts Resources Resource templates
Base 14 2 0 0
Head 14 2 0 0

tools

--- base/tools.json
+++ target/tools.json

- tools[git_remote].description: "Lists the repository's remotes with their fetch and push URLs, like `git remote -v`. Credentials emb..."
+ tools[git_remote].description: "Lists the repository's remotes with their fetch and push URLs, like `git remote -v`. Credentials emb..."

Updated by this run for 4efde404fe4757fe2c825f5801a64c565a0ac38d.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Exit cleanup can delete another writer’s empty replacement lock, allowing concurrent writers and lost graph updates.

2 open issues
1 resolved since last review

Review effort: Balanced

Comment thread src/memory/index.ts Resolved
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 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 2: one finding, fixed in 4efde40.

  • Exit cleanup could delete another writer's empty replacement lock: the prefix match now applies only while our own token write is in flight. A written lock needs an exact match. New regression test covers an empty replacement of a written lock. A file-identity check was not added, for the reason in the thread.

Gate re-run: exit 0. Requesting round 3.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues remain for the memory server’s shutdown lock-cleanup change.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 3 (on 4efde40): clean, no findings. Review loop converged.

@cliffhall
cliffhall merged commit 1e0c7ba into v2/main Nov 12, 2026
52 checks passed
@cliffhall
cliffhall deleted the v2/fix/5485-memory-lock-cleanup-on-exit branch November 12, 2026 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

memory: clean up active lock files on exit and termination signals

2 participants