fix: KG path mismatch between MCP server and CLI

[duha887b]

Summary

• KG path fix: MCP server now uses palace_path/knowledge.db unconditionally, matching the CLI. Previously, running without --palace (default for plugin installs) caused KG writes to go to ~/.mempalace/knowledge_graph.sqlite3 while CLI reads from ~/.mempalace/palace/knowledge.db — silent data loss.
• atexit shutdown hook: Checkpoints SQLite WAL and releases ChromaDB client before process exit, preventing data loss when stdio transport closes.

Problem

KG facts written via MCP tools (mempalace_kg_add) were invisible to CLI commands and new MCP sessions. Three different paths were involved:

Path	Used by
~/.mempalace/knowledge_graph.sqlite3	KnowledgeGraph() default (no-arg fallback)
palace_path/knowledge_graph.sqlite3	MCP server with --palace
palace_path/knowledge.db	CLI, mempalace search, mempalace_kg_query

Fix

One-line change: always derive KG path from MempalaceConfig.palace_path + "knowledge.db".

# Before (lines 59-63):
_config = MempalaceConfig()
if _args.palace:
    _kg = KnowledgeGraph(db_path=os.path.join(_config.palace_path, "knowledge_graph.sqlite3"))
else:
    _kg = KnowledgeGraph()

# After:
_config = MempalaceConfig()
_kg = KnowledgeGraph(db_path=os.path.join(_config.palace_path, "knowledge.db"))

Verified: KnowledgeGraph(db_path=...) auto-creates the file and tables via _init_db() — no issue with fresh palaces.

Test plan

• Fresh palace: KnowledgeGraph(db_path="/tmp/fresh/knowledge.db") creates file + tables
• MCP mempalace_kg_add writes visible to CLI mempalace search
• MCP mempalace_kg_query reads same data as CLI
• No regression on existing drawer operations

Related

• Bug: MCP Tools diary_write and kg_add Return Internal Error -32000 #526 — MCP write failures on Windows (encoding, separate fix)
• Bug: MCP server fails with surrogate error on Windows when writing CJK content #503 — Unicode errors on Windows

🤖 Generated with Claude Code

[web3guru888]

This is the fix for exactly the bug described in #538 — and it's clean.

KG path fix (): Correct. The conditional branch was the source of the silent data loss — depending on whether --palace was passed, MCP was writing to a different path than CLI was reading from. Unconditionally deriving from palace_path is the right invariant.

_shutdown() atexit handler: Good coverage of the common case (SIGTERM, SIGINT, normal exit). The SIGKILL caveat is clearly documented in the docstring — that's the right tradeoff acknowledgment rather than overclaiming. One thought: if downstream adopters need SIGKILL safety, PRAGMA journal_mode=DELETE at init is the lever, and it's worth noting that in the PR description (you've already included it in the function docstring, so readers will find it).

Minor nit: the import atexit at module bottom rather than the top is the only style thing — won't affect correctness but most linters will flag it. Easy to move in a fixup.

This directly fixes a critical data corruption path that many CLI+MCP combo users would have hit silently. +1 to merge alongside (or after) any open MCP PRs.

[web3guru888]

Good fix — we confirmed the root cause on #538, happy to see this land.

A few notes from reviewing the diff:

Path unification ✅ — The conditional is exactly the problem we traced on #538. Collapsing to palace_path/knowledge.db unconditionally is the right call. One minor heads-up: the old no-arg path was knowledge_graph.sqlite3 but the new one is knowledge.db — existing no-arg installs will start with a blank KG on upgrade. Worth a migration note in the changelog (or a one-time mv hint).

WAL checkpoint ✅ — PRAGMA wal_checkpoint(TRUNCATE) on clean exit is solid. We do the same in our integration and it eliminates the partial-write on SIGTERM. The SIGKILL caveat in the docstring is honest and correct.

ChromaDB release — del _client_cache removes the reference but doesn't actually close the client; the background persist thread may still be running at that point. If PersistentClient exposes a .close() or .stop() method in the ChromaDB version you're targeting, calling it explicitly before the del would be safer. The WAL checkpoint on the SQLite side is the critical piece here anyway.

import atexit placement — Works fine at module bottom, though PEP 8 prefers top-of-file. Low priority but easy to move.

Overall this is a surgical, minimal fix for a real data-loss bug. Happy to see it merged. 🟢

[igorls]

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)