Add FastMCP server and startup path

[Atharva-Kanherkar]

Summary

• add a thin mcp_server/server.py adapter over the shared runtime and MCP schemas
• register the locked v1 MCP tool surface with stdio and mounted streamable-http startup paths
• add focused server tests plus minimal README and dependency updates for the new entrypoint

Testing

• python -m py_compile mcp_server/server.py mcp_server/main.py tests/test_mcp_server.py

Notes

• Issue MCP-1D: Implement the FastMCP server and transport startup #61 lock comment: MCP-1D: Implement the FastMCP server and transport startup #61 (comment)
• The local environment here does not have the project dependencies installed, so I could not run pytest or dependency-backed integration tests.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agentic-memory	Ready	Preview, Comment	Mar 30, 2026 10:17am

[Atharva-Kanherkar]

@claude Please review this PR throughly., If any manual tests or anything else is needed please ask. Please do not leave any decisison tree or dry runs. go to eeach line of code, do a dry run, make a dependency tree, dry run those codes, find bugs and tell me. Even if you think this might be not an issue, but dry run is telling yo,u, tell me,. USe a real productin case where you will make a test suite, dry run it, see the procvess, report if it breaks, and tell me again,

[claude]

Claude finished @Atharva-Kanherkar's task in 4m 36s —— View job

---

PR Review: Add FastMCP server and startup path

Tasks

• Read CLAUDE.md for repo guidelines
• Read all changed files line by line
• Build dependency tree
• Dry-run server.py logic
• Dry-run test_mcp_server.py
• Identify bugs and report findings

---

Dependency Tree

python -m mcp_server
  └── mcp_server/__main__.py
        └── mcp_server/server.py :: main()

mcp_server/server.py
  ├── config.py                         (CHROMA_DB_PATH, MEDIA_STORAGE_PATH)
  ├── mcp_server/schemas.py
  │   ├── forgetting/contradiction.py   (ContradictionCandidate)
  │   ├── forgetting/service.py         (ForgettingReport)
  │   ├── models/base.py                (MemoryRecord)
  │   ├── models/episodic.py            (EpisodicMemory)
  │   ├── models/procedural.py          (ProceduralMemory)
  │   ├── models/semantic.py            (SemanticMemory)
  │   ├── retrieval/ranking.py          (RankedResult)
  │   ├── runtime.py                    (MemoryRuntime)
  │   └── stores/procedural_store.py    (ProceduralMatch)
  ├── models/base.py                    (normalize_modality)
  ├── runtime.py                        (MemoryRuntime, build_runtime)
  ├── stores/episodic_store.py          (EpisodicStoreError, MediaTooLargeError)
  ├── stores/media_store.py             (MediaStore)
  └── utils/embeddings.py              (EmbeddingProviderError, TextEmbedder)

tests/test_mcp_server.py
  ├── mcp_server/server.py              (MemoryMCPServer, ServerConfig)
  ├── mcp_server/schemas.py             (4 request models)
  └── tests/helpers.py                  (HashingEmbedder, cleanup_dir, make_temp_chroma_dir)

---

Bug Report

🔴 CRITICAL — Temp File Leak on MediaStore.resolve_media_type Failure

File: mcp_server/server.py, lines 441–454

Dry-run trace (inline content path):

_prepare_media(media=InlineContentModel, requested_media_type=None)
  → payload = base64.b64decode(...)         # OK
  → suffix = Path("diagram.png").suffix     # ".png"
  → handle = NamedTemporaryFile(..., delete=False)   # /tmp/agentic_memory_mcp_XXXXX.png
  → handle.write(payload)
  → handle.close()  (in finally)
  → path = Path("/tmp/agentic_memory_mcp_XXXXX.png")
  → media_type = MediaStore.resolve_media_type(path, None)   # ← RAISES?
     # If it raises, PreparedMedia is never created.
     # Exception propagates up to remember_fact/episode/procedure.
     # Their finally: _cleanup_prepared_media(prepared_media)  runs with prepared_media=None
     # → no cleanup. Temp file is permanently leaked on disk.

The try/finally in the calling methods (remember_fact, remember_episode, remember_procedure) calls _cleanup_prepared_media(prepared_media). But prepared_media is the return value of _prepare_media. If _prepare_media raises (after creating the temp file), prepared_media is never assigned — it stays None from the outer call — and the temp file is never deleted.

Reproduction: Pass an inline media object with a mime_type that MediaStore.resolve_media_type rejects. Repeat many times → disk fills with orphaned temp files.

Fix this →

---

🔴 CRITICAL — Sync Call Blocks asyncio Event Loop

File: mcp_server/server.py, lines 396–407

async def _run_forgetting_cycle(self, *, request, dry_run):
    if self._forgetting_lock.locked():
        return ForgettingReportModel(status="already_running", dry_run=dry_run)
    async with self._forgetting_lock:
        report = self.runtime().forgetting_service.run_cycle(dry_run=dry_run)  # ← SYNC
        return serialise_forgetting_report(report, max_decisions=request.max_decisions)

forgetting_service.run_cycle() is synchronous. Called inside an async def without await asyncio.get_event_loop().run_in_executor(...), it blocks the entire asyncio event loop for the duration of the forgetting cycle.

Dry-run production case: Imagine 500 memory records. run_cycle scans all of them. During those 2–5 seconds, every other in-flight MCP request (recall, remember, etc.) is frozen. The server appears to hang. For the stdio transport this is less visible (single client), but for streamable-http with multiple clients it's a deadlock scenario.

Fix this →

---

🟡 MEDIUM — overview() Accesses Private _collection Attributes

File: mcp_server/server.py, lines 221–223

"semantic_count": runtime.semantic_store._collection.count(),
"episodic_count": runtime.episodic_store._collection.count(),
"procedural_count": runtime.procedural_store._collection.count(),

Direct access to ._collection on the store objects. If any store refactors its internal ChromaDB collection reference (rename, lazy-load, None-check), this raises AttributeError in production with no graceful fallback. The get_memory_overview MCP tool catches the exception and returns a ToolErrorModel, so the server won't crash — but the tool silently fails.

No stores expose a public count() method so the stores layer would need to be updated to expose one, or this method needs a try/except AttributeError.

---

🟡 MEDIUM — recall_episodes Tool: Unknown mode Produces Confusing Error

File: mcp_server/server.py, lines 669–689

payload: dict[str, Any] = {"mode": mode}
if mode == "recent":
    payload["limit"] = limit
elif mode == "session":
    payload["session_id"] = session_id
elif mode == "time_range":
    payload["start"] = start
    payload["end"] = end
# No else — unknown mode falls through silently
return server.recall_episodes(payload)

Dry-run with mode="invalid":

payload = {"mode": "invalid"}
server.recall_episodes({"mode": "invalid"})
  → RecallEpisodesRequestAdapter.validate_python({"mode": "invalid"})
  → Pydantic ValidationError: discriminator 'mode' — no match for "invalid"
  → _map_exception catches → ToolErrorModel(code="invalid_request", message="<pydantic internal>")

The error is caught, so no crash, but the Pydantic discriminator error message is unreadable. A simple if mode not in {"recent", "session", "time_range"}: raise ValueError(...) before the payload building would give a clean message.

Also: mode="session" with session_id=None → payload["session_id"] = None → Pydantic min_length=1 error → again caught but confusing.

---

🟡 MEDIUM — streamable_http_path Hardcoded in FastMCP Constructor

File: mcp_server/server.py, lines 512–517

mcp = FastMCP(
    _MCP_SERVER_NAME,
    json_response=True,
    streamable_http_path="/",     # ← hardcoded, ignores ServerConfig
)

ServerConfig.streamable_http_path defaults to "/mcp" and is validated, logged in /health, and user-configurable via MEMORY_MCP_PATH. But the FastMCP object is always created with streamable_http_path="/".

For the HTTP path this currently works because Starlette's Mount strips the prefix before passing requests to FastMCP's ASGI app. But if FastMCP uses its own streamable_http_path internally (for SSE endpoint construction, redirect generation, or OpenAPI docs), there will be a path mismatch when MEMORY_MCP_PATH != "/mcp".

---

🟡 MEDIUM — validate_startup() Called Three Times

File: mcp_server/server.py, lines 779, 752, 765

# main():
server.validate_startup()          # call 1 — OK, initializes runtime

# create_http_app → lifespan():
server.validate_startup()          # call 2 — no-op (runtime already built)

# create_http_app → health():
server.validate_startup()          # call 3 — on every /health request

The /health endpoint calls validate_startup() on every request. Since validate_startup() calls self.runtime() which does build_runtime() on first call and returns cached on subsequent calls — it's safe but adds a method call on every health check. More importantly: if someone runs main() without validate_startup() first (e.g. in a test harness that calls create_http_app directly), they'd still get initialization via lifespan. The extra call in main() before create_http_app is harmless but redundant.

---

🟡 MEDIUM — mimetypes.guess_extension Is Platform-Dependent

File: mcp_server/server.py, line 440

suffix = mimetypes.guess_extension(inline.mime_type) or ""

Dry-run on different platforms:

• Linux (with full mime.types): guess_extension("image/png") → ".png" or ".pnm" (depends on /etc/mime.types ordering)
• macOS: ".png"
• Minimal container: None → suffix = ""

If suffix = "", the temp file has no extension, and MediaStore.resolve_media_type must rely entirely on requested_media_type. If that's also None, the store may misidentify the file type. On a Docker-deployed server with a minimal mime DB, inline images without filenames will create extensionless temp files.

---

🟠 TEST — test_server_forgetting_cycle_is_single_flight Tests the Wrong Scenario

File: tests/test_mcp_server.py, lines 86–98

async def test_server_forgetting_cycle_is_single_flight():
    server = make_server()
    try:
        await server._forgetting_lock.acquire()   # manually pre-locks
        try:
            second = await server.preview_forgetting_cycle(...)  # sees locked() == True
        finally:
            server._forgetting_lock.release()
        assert second.status == "already_running"

This test works, but it only tests a manually pre-locked scenario. It does NOT test the real concurrent case: two asyncio.create_task() calls both calling preview_forgetting_cycle simultaneously. The real race is:

Task A: locked() == False → passes
Task B: locked() == False → passes (no await between check and acquire)
Task A: acquires lock, runs run_cycle() (sync, blocks event loop)
Task B: blocks on `async with self._forgetting_lock` — never returns "already_running"

With asyncio's cooperative scheduler and the sync run_cycle() blocking the loop, Task B would only run after Task A completes — at which point the lock is free and B runs the cycle a second time. The "already_running" guard works in the manual-lock test but not for real concurrent calls when run_cycle is sync (Bug #2 above compounds this).

---

🔵 MINOR — Test Fixture Uses make_temp_chroma_dir for media_root

File: tests/test_mcp_server.py, line 25

resolved_media_root = media_root or make_temp_chroma_dir("mcp_media_")

make_temp_chroma_dir creates a temp dir prefixed mcp_media_. The name implies "chroma" but it's being used for media. This is a naming inconsistency that could confuse future test writers. Not a runtime bug.

---

🔵 MINOR — __main__.py Only Guards if __name__ == "__main__"

File: mcp_server/__main__.py

from .server import main

if __name__ == "__main__":
    main()

python -m mcp_server works. But python mcp_server/__main__.py would fail with a relative import error (from .server import main requires package context). This is expected Python packaging behavior, not a bug, but worth noting in the README.

---

Test Coverage Gaps

The PR has 5 test cases for ~800 lines of logic. Missing coverage:

Missing Test	Risk
recall_episodes with all 3 modes	mode routing could silently break
recall_memories / recall_procedures	no coverage of retriever integration
record_procedure_outcome (success + not-found)	double-get race is untested
create_mcp_server / create_http_app	FastMCP tool registration untested
_map_exception branches	all 5 exception types uncovered
_prepare_media with file_path branch	file validation path untested
_prepare_media with empty base64	ValueError("Cannot store empty media payload") uncovered
resolve_contradiction	no test
HTTP /health endpoint	no test
Invalid transport / port ≤ 0	ServerConfig.validate paths uncovered

---

Summary

Severity	Count	Items
🔴 Critical	2	Temp file leak on exception; event loop blocking
🟡 Medium	5	Private _collection access; recall_episodes validation; streamable_http_path disconnect; redundant validate_startup; platform-dependent mime extension
🟠 Test Gap	1	Concurrent forgetting cycle test doesn't reflect real production race
🔵 Minor	2	Naming in test fixture; __main__.py invocation note

The two critical bugs need to be fixed before merge. The event loop blocking bug (#2) in particular will cause observable hangs in any production HTTP deployment once the memory store grows beyond a few hundred records.

[Atharva-Kanherkar]

@claude re review. i added patches to only real blockers/

[claude]

Claude finished @Atharva-Kanherkar's task in 1m 21s —— View job

---

Re-review: Patches for Critical Blockers

Tasks

• Read current mcp_server/server.py to check patches
• Verify fix for 🔴 Temp file leak (_prepare_media)
• Verify fix for 🔴 Event loop blocking (_run_forgetting_cycle)
• Check updated concurrent forgetting cycle test
• Report findings

---

🔴 Critical #1 — Temp File Leak: ✅ Fixed

server.py lines 459–464:

path = Path(handle.name)
try:
    media_type = MediaStore.resolve_media_type(path, requested_media_type)
except Exception:
    path.unlink(missing_ok=True)
    raise
return PreparedMedia(path=path, media_type=media_type, cleanup=True)

The try/except wrapping resolve_media_type is correct — if it raises, the temp file is deleted before re-raising. The companion test (test_prepare_media_cleans_up_temp_file_when_media_type_resolution_fails) snapshots /tmp/agentic_memory_mcp_* before and after and asserts they're equal. Solid. ✅

One note: functools.partial is imported at the top of the file — confirm it's used here (it is, for the forgetting fix below). No issue.

---

🔴 Critical #2 — Event Loop Blocking: ✅ Fixed

server.py lines 407–413:

report = await asyncio.get_running_loop().run_in_executor(
    None,
    functools.partial(
        self.runtime().forgetting_service.run_cycle,
        dry_run=dry_run,
    ),
)

Correctly offloads the sync run_cycle to a thread pool executor. Using get_running_loop() is also the right modern choice (vs the deprecated get_event_loop() I mentioned in the first review). ✅

---

🟠 Test Gap — Concurrent Forgetting Cycle: ✅ Fixed

The new test (test_server_forgetting_cycle_is_single_flight) replaces the manual-lock approach with a real concurrent test:

def slow_run_cycle(*, dry_run: bool):
    started.set()
    future = asyncio.run_coroutine_threadsafe(release.wait(), loop)
    future.result(timeout=2)   # blocks the executor thread
    return original(dry_run=dry_run)

• slow_run_cycle pauses inside the executor thread using run_coroutine_threadsafe to bridge back to the asyncio loop
• first_task = asyncio.create_task(...) runs concurrently
• After started.wait() confirms the first task is inside run_cycle, the second call is made
• Second call correctly sees _forgetting_lock.locked() == True → returns "already_running"
• release.set() unblocks the first task

This tests the actual production race condition. ✅

---

Summary

Bug	Severity	Status
Temp file leak on resolve_media_type failure	🔴 Critical	✅ Fixed + tested
Sync run_cycle blocking event loop	🔴 Critical	✅ Fixed + tested
Concurrent forgetting cycle test didn't reflect real race	🟠 Test gap	✅ Fixed

Both blockers are correctly patched. The remaining medium/minor items from the first review (private _collection access, recall_episodes mode validation, streamable_http_path disconnect, redundant validate_startup, platform-dependent mime extension) are non-blocking and can be addressed in follow-up PRs.

This PR is unblocked from the two critical issues. Ready to merge once CI passes.