Skip to content

fix: defer KnowledgeGraph init to first use in MCP server - #167

Closed
adv3nt3 wants to merge 1 commit into
MemPalace:developfrom
adv3nt3:fix/lazy-knowledge-graph-init
Closed

fix: defer KnowledgeGraph init to first use in MCP server#167
adv3nt3 wants to merge 1 commit into
MemPalace:developfrom
adv3nt3:fix/lazy-knowledge-graph-init

Conversation

@adv3nt3

@adv3nt3 adv3nt3 commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Summary

mcp_server.py line 34 runs _kg = KnowledgeGraph() at module level. KnowledgeGraph.__init__ creates the parent directory and opens/creates a SQLite database. This means importing the MCP server module — even to inspect its tools list or in tests — creates ~/.mempalace/knowledge_graph.sqlite3 as a side effect.

Fix

Replace _kg = KnowledgeGraph() with _kg = None and a lazy _get_kg() getter that creates the instance on first call. All 5 call sites updated from _kg.method() to _get_kg().method().

_kg = None

def _get_kg():
    global _kg
    if _kg is None:
        _kg = KnowledgeGraph()
    return _kg

Changes

1 file changed (mempalace/mcp_server.py), 14 insertions, 6 deletions.

Test plan

  • ruff check + ruff format --check pass
  • python3 -m py_compile compiles OK
  • Pyright reports 0 new diagnostics from this change
  • All 5 _kg. call sites updated to _get_kg().
  • Lazy init pattern verified via Context7 CPython docs

Refs: #159 (point 8)

@adv3nt3
adv3nt3 force-pushed the fix/lazy-knowledge-graph-init branch from f70baa7 to 953232b Compare April 7, 2026 23:29
@bgauryy

bgauryy commented Apr 8, 2026

Copy link
Copy Markdown

PR Review: fix: defer KnowledgeGraph init to first use in MCP server

Executive Summary

Aspect Value
PR Goal Lazy-init KnowledgeGraph to prevent SQLite side effects on module import
Files Changed 1
Risk Level 🟢 LOW - well-scoped refactor, no behavior change at runtime
Review Effort 1 - trivial, single-pattern change
Recommendation ✅ APPROVE

Affected Areas: mempalace/mcp_server.py — knowledge graph initialization and 5 KG tool functions

Business Impact: None at runtime. Prevents unwanted ~/.mempalace/ directory and SQLite DB creation when the module is merely imported (e.g. in tests or tool inspection).

Flow Changes: KnowledgeGraph() instantiation moves from import-time to first tool invocation. No change to steady-state behavior.

Ratings

Aspect Score
Correctness 5/5
Security 5/5
Performance 5/5
Maintainability 5/5

PR Health

  • Has clear description
  • References the root cause (mcp_server.py line 34 module-level init)
  • Appropriate size (14 added / 6 deleted, 1 file)
  • Has relevant tests — no test added, but side-effect is hard to assert without import isolation

What Changed

Before: _kg = KnowledgeGraph() at module level (line 34). Importing mcp_server triggers:

  1. Path(~/.mempalace).mkdir(parents=True, exist_ok=True)
  2. SQLite CREATE TABLE IF NOT EXISTS on knowledge_graph.sqlite3

After: _kg = None + _get_kg() lazy getter. The KnowledgeGraph is created only when a KG tool (kg_query, kg_add, kg_invalidate, kg_timeline, kg_stats) is first called.

All 5 call sites updated:

Function Old New
tool_kg_query _kg.query_entity(...) _get_kg().query_entity(...)
tool_kg_add _kg.add_triple(...) _get_kg().add_triple(...)
tool_kg_invalidate _kg.invalidate(...) _get_kg().invalidate(...)
tool_kg_timeline _kg.timeline(...) _get_kg().timeline(...)
tool_kg_stats _kg.stats() _get_kg().stats()

No missed references — remaining _kg mentions in the file are inside a prompt string, not code.

Low Priority Issues

#1: Lazy getter is not thread-safe

Location: mempalace/mcp_server.py_get_kg() | Confidence: ⚠️ MED

The check-then-set pattern (if _kg is None: _kg = KnowledgeGraph()) is subject to a race condition under concurrent threads. Two threads could both observe None and create separate instances.

Mitigating factors: MCP servers run on a single asyncio event loop, so concurrent thread access is extremely unlikely. This is a non-blocking observation.

# If thread safety ever becomes needed:
import threading
_kg_lock = threading.Lock()

def _get_kg():
    global _kg
    if _kg is None:
        with _kg_lock:
            if _kg is None:
                _kg = KnowledgeGraph()
    return _kg

Verdict

Clean, well-motivated fix. The lazy initialization pattern is standard Python, all call sites are covered, and the change eliminates a real side effect that impacts testability. No functional risk.


Created by Octocode MCP https://octocode.ai 🔍🐙

Replace module-level _kg = KnowledgeGraph() with lazy _get_kg()
that creates the instance on first call. Respects --palace flag
for custom db_path when provided. Importing mcp_server no longer
creates ~/.mempalace/knowledge_graph.sqlite3 as a side effect,
fixing test isolation and spurious database creation.
@adv3nt3
adv3nt3 force-pushed the fix/lazy-knowledge-graph-init branch from 953232b to 053fca2 Compare April 9, 2026 17:53
@adv3nt3

adv3nt3 commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

@bgauryy Recreated this branch from current main to resolve merge conflicts — upstream added --palace flag support, WAL logging, and input sanitization to mcp_server.py since the original PR. The lazy _get_kg() pattern is the same, now updated to respect the --palace conditional for custom db_path. The diff tab may show unrelated changes from main, but the only new code is the _get_kg() function and the 5 call-site replacements.

@guybary-wix guybary-wix left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Checked the diff — confirms your claim. Only _get_kg() + the 5 call-site replacements, no stale upstream noise. --palace path logic carried over correctly. Approving.

@guybary-wix guybary-wix left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Previous approval was submitted in error by an automated tool. Withdrawing approval.

@adv3nt3

adv3nt3 commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

@guybary-wix any concerns?

@web3guru888 web3guru888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔧 Review of #167fix: defer KnowledgeGraph init to first use in MCP server

Scope: +18/−9 · 1 file(s) · touches core

  • ⚠️ mempalace/mcp_server.py (modified: +18/−9)

Issues

  • ⚠️ Touches mempalace/mcp_server.py — Core MCP server — maintainer guards this closely

🟡 Needs attention — touches guarded files and has items to address.


🏛️ Reviewed by MemPalace-AGI · Autonomous research system with perfect memory · Showcase: Truth Palace of Atlantis

@bensig
bensig changed the base branch from main to develop April 11, 2026 22:23
@bensig

bensig commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

closing — KG initialization was reworked in #647 and #667 (both merged). thanks @adv3nt3!

@bensig bensig closed this Apr 12, 2026
mvalentsev added a commit to mvalentsev/mempalace that referenced this pull request Apr 26, 2026
TestKGLazyCache covers the scenarios behind the lazy per-path refactor:

- test_lazy_init_no_import_side_effect: a fresh subprocess import does
  not create ~/.mempalace/knowledge_graph.sqlite3 (what closed PR MemPalace#167
  was aiming at).
- test_get_kg_returns_same_instance: two _get_kg() calls under the same
  resolved path return the same object, cache has one entry.
- test_get_kg_different_paths_different_instances: rotating env var
  produces distinct KGs.
- test_multi_tenant_env_switch: the exact scenario from MemPalace#1136 — write
  under path A, query under path B returns empty, switching back to A
  sees the fact.
- test_cache_thread_safe: 16 threads racing _get_kg() end up with one
  shared instance and one cache entry.
mvalentsev added a commit to mvalentsev/mempalace that referenced this pull request May 1, 2026
TestKGLazyCache covers the scenarios behind the lazy per-path refactor:

- test_lazy_init_no_import_side_effect: a fresh subprocess import does
  not create ~/.mempalace/knowledge_graph.sqlite3 (what closed PR MemPalace#167
  was aiming at).
- test_get_kg_returns_same_instance: two _get_kg() calls under the same
  resolved path return the same object, cache has one entry.
- test_get_kg_different_paths_different_instances: rotating env var
  produces distinct KGs.
- test_multi_tenant_env_switch: the exact scenario from MemPalace#1136 — write
  under path A, query under path B returns empty, switching back to A
  sees the fact.
- test_cache_thread_safe: 16 threads racing _get_kg() end up with one
  shared instance and one cache entry.
mvalentsev added a commit to mvalentsev/mempalace that referenced this pull request May 2, 2026
TestKGLazyCache covers the scenarios behind the lazy per-path refactor:

- test_lazy_init_no_import_side_effect: a fresh subprocess import does
  not create ~/.mempalace/knowledge_graph.sqlite3 (what closed PR MemPalace#167
  was aiming at).
- test_get_kg_returns_same_instance: two _get_kg() calls under the same
  resolved path return the same object, cache has one entry.
- test_get_kg_different_paths_different_instances: rotating env var
  produces distinct KGs.
- test_multi_tenant_env_switch: the exact scenario from MemPalace#1136 — write
  under path A, query under path B returns empty, switching back to A
  sees the fact.
- test_cache_thread_safe: 16 threads racing _get_kg() end up with one
  shared instance and one cache entry.
mvalentsev added a commit to mvalentsev/mempalace that referenced this pull request May 3, 2026
TestKGLazyCache covers the scenarios behind the lazy per-path refactor:

- test_lazy_init_no_import_side_effect: a fresh subprocess import does
  not create ~/.mempalace/knowledge_graph.sqlite3 (what closed PR MemPalace#167
  was aiming at).
- test_get_kg_returns_same_instance: two _get_kg() calls under the same
  resolved path return the same object, cache has one entry.
- test_get_kg_different_paths_different_instances: rotating env var
  produces distinct KGs.
- test_multi_tenant_env_switch: the exact scenario from MemPalace#1136 — write
  under path A, query under path B returns empty, switching back to A
  sees the fact.
- test_cache_thread_safe: 16 threads racing _get_kg() end up with one
  shared instance and one cache entry.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants