fix(mcp): route stdio clients through single-writer daemon - #1976
fix(mcp): route stdio clients through single-writer daemon#1976fatkobra wants to merge 8 commits into
Conversation
|
/gemini review |
|
@fatkobra on Windows this breaks the default entry point: CPython never got The daemon lifetime also changes the writer story for everything that isn't an MCP client: there's no idle shutdown, and mcp_server takes its server-lifetime writer lease, so once a daemon exists, hook-spawned And honestly, the framing here isn't ok: To be clear, I'm not hunting your PRs. I'd dug deep into #1924 and #1920 myself earlier, so I had the context loaded when yours showed up, and after those two I read this one too and couldn't walk past the Windows thing. Nothing personal. |
|
Thanks for the detailed review @mvalentsev . I agree these are serious design concerns, not just CI/test issues. The PR currently makes I also agree the daemon lifetime/writer-lease interaction needs a maintainer decision. This PR consolidates MCP stdio clients, but CLI mines and hook-spawned mines do not route through the daemon, so it does not make the single-writer story true end-to-end. In some setups it may actually starve those external writers harder. I also agree that I’ll step back from this implementation and, if maintainers want help, I’m happy to coordinate on #1270 or on a smaller opt-in/cross-platform slice with explicit tests. |
|
Pushed a rework. The PR no longer introduces a separate MCP-specific daemon. It now reuses the existing Main changes:
This still contributes to #1963 rather than closing it. Full closure still needs hook/CLI rollout so all writer paths consistently use the daemon-backed route. |
|
@mvalentsev thanks again for the review. I reworked the PR around your concerns. The new version no longer starts a separate MCP-specific Unix-socket daemon. It reuses the existing That changes the shape materially:
It still does not claim to complete the full Tier 3 rollout by itself; hook/CLI configuration still needs to consistently route through daemon-backed paths for the whole #1963 cluster to close. |
|
I tested the current head (
I prepared and independently reviewed a narrow patch based directly on this head:
The core normalization is: received_backend = str(identity.get("backend") or "").strip().lower()
if not received_backend:
received_backend = expected["backend"]Validation on the PR head:
I have the clean tested patch ready and can provide it as a cherry-pick/stacked branch if useful. One separate non-blocking follow-up is env-only backend selection (for example |
|
Thanks, @Timofa — this is excellent validation, and I appreciate you taking the time to reproduce both failures against the real daemon/client path, prepare the narrow fix, and independently verify it. I confirmed both blockers in the current head:
Please share the commit hash or stacked branch. I’m happy to cherry-pick your tested patch, including the real loopback HTTP regression test, so your authorship is preserved in the commit history. I also agree that environment-only backend selection, such as Thank you again for finding, reproducing, and preparing fixes for these two blockers. |
|
Thanks for confirming both issues. I've pushed the patch as a single commit stacked directly on the current PR head (
To cherry-pick: git fetch https://github.com/Timofa/mempalace.git codex/fix-1976-mcp-bridge
git cherry-pick 43a1f9d0ba01fddea9f8200c7c09dd415d7185d0The commit is limited to the Validation on this commit:
The environment-only |
|
Cherry-picked @Timofa’s tested fix for the Credit to @Timofa for reproducing both failures against a real loopback daemon/client path, preparing the focused patch, and adding the integration and identity regression coverage. The cherry-picked commit preserves @Timofa as the author. |
|
Pushed a scope cleanup removing the obsolete standalone-daemon documentation. I initially attempted to remove the earlier formatting-only change to |
|
Pushed the effective-identity and installed-command E2E follow-up. Effective identity changes:
E2E coverage:
Because the normal CI jobs install the editable package and run the full test suite on Linux, macOS, and Windows, this installed-command E2E is exercised across all three platforms. |
|
I tested the current pull request head against the two-client topology discussed in #1963. Tested commit: Topology tested
Result
Neither client emitted standard error, and both exited successfully. One useful migration detail observed during the test: the entry point mapping now makes One scope caveat: this validates the bridge, daemon writer gate, concurrent two-client coordination path, and basic restart durability for a small write set. It does not exercise the ChromaDB Within that scope, this pull request looks aligned with the desired shared-agent topology: one daemon owner process, one ChromaDB handle, standard input/output clients bridged into that owner, and serialized writes without direct peer-writer conflicts. |
|
Thank you for running this independent two-client integration test and documenting the topology and results so clearly. This validates the core behavior #1976 is intended to provide:
I agree with the scope caveat. A two-drawer test validates the gateway topology and basic restart durability, but it does not reproduce the A larger threshold-crossing scenario would be useful as a separate stress/integration validation if maintainers want it, but I will not broaden the claims of this PR based on the current test. Thanks @henderlabs again for the careful verification. |
|
Thanks for this contribution, and apologies for the slow turnaround.
If you'd rather not pick it back up, no problem at all — just say so and I'll close it out, and thanks either way for taking the time to send it. |
|
@igorls I will work on it and rebase it and address it and make it ready. |
1b02477 to
e8df7bd
Compare
|
@igorls rebased this branch onto current The rebase preserves current upstream behavior, including:
It also retains the PR-specific MCP bridge behavior:
@Timofa Timofa's Git skipped the old formatting-only I also extracted writer-lease acquisition into a small helper so the merged The rebased history was pushed with |
|
Disclosure: produced by Claude Code on my machine — it ran the measurements and drafted this, I reviewed and I'm posting it. Strong +1 on this direction, from a Windows install that is currently in exactly the state the PR describes. Plus one measurement that I think is worth considering for the writer gate. Why this PR matters for a normal desktop setupmempalace 3.7.1 / chromadb 1.5.9, Result: the daemon takes for the entire life of that session. Following the recommended supervised configuration is precisely what makes every agent read-only, and since That is the loop this PR breaks: the bridge makes the supervised daemon the correct configuration rather than the thing that locks everyone out, and it keeps the "no external service by default" property from AGENTS.md. Measurement: serializing reads behind writes has a visible costI couldn't test the stdio bridge against my production palace, so I measured the closest available shape of "one owner, N clients": Concurrent mutations — works, as the earlier two-client test in this thread also found: Long write vs. concurrent read:
In the HTTP transport this is If they do, one agent's transcript ingest stalls every other agent's recall for the duration of the mine. On my machine transcripts run up to 5.3 MB, so worst case is tens of seconds of blocked reads across all sessions — and recall latency is the thing users notice immediately. Under the pre-bridge topology those reads came from separate read-only processes and were never blocked by a mine, so this would be a visible regression for a setup that otherwise gets strictly better. Two shapes that would avoid it, both with precedent in the codebase:
Not a blocker for landing it — just flagging it before the topology becomes the default, since after that every session's recall latency depends on the answer. The read/write split you need already exists in
|
|
@stefano-scarpone-bluvacanze thank you for the detailed testing and for separating direct observations from inferences. I checked both points against the current PR head. Read serializationYour read-latency concern applies to the current implementation.
The existing I will treat this as a performance follow-up unless maintainers want read concurrency included in this PR’s scope. Knowledge-graph pathThe KG concern also applies.
That behavior exists on current I do not think the internal Current architecture overlapThere is one additional development since this branch was last rebased: current The branch now conflicts because these are overlapping gateway implementations, not just because of incidental code drift. @igorls before I rebase again, could you confirm the intended 3.8 direction?
I do not want to resolve the entry-point conflict by silently replacing the newer upstream design. |
|
Thanks for the careful review and for surfacing the lifecycle and identity concerns. We’ve made a directional decision since this PR was opened: the long-term architecture is a central, long-lived This is especially important now that the replicated palace and logstream are becoming core infrastructure. We need one visible, well-configured, stable service owning the palace locally, with clear health and identity information. Because of that, we do not want to rebase or merge the daemon bridge and lifecycle implementation in this PR. That path is superseded by the hub/proxy architecture. However, the identity-validation concern here is still important. A caller must not silently connect to a hub that does not match its effective palace and configuration. We would welcome a focused follow-up PR against current The next lifecycle step on our side is safe “connect or start” behavior: if the correct hub is absent, one caller may start it under a cross-process lock, while other callers wait for readiness and connect. The running hub should remain visible and inspectable to the user. We will handle the database-path consistency and existing split-store migration as a separate correctness fix before making this topology the default. Thank you again—your review helped identify the safeguards we need to preserve as we move to the unified hub model. |
|
Thanks @igorls — understood. I will not rebase or further update #1976. I accept the architectural decision that the long-term owner is one visible, long-lived I’m happy to extract the still-relevant identity work into a focused PR against current
I will leave the connect-or-start lifecycle work to the maintainer-side implementation you described. |
What does this PR do?
Contributes to #1963.
This reworks #1976 into a package-level complement to the daemon + bridge direction in #1270.
Instead of introducing a second MCP-specific daemon, this PR reuses the existing
mempalace.daemonqueue/server as the local owner and adds MCP stdio bridging into that daemon.The previous default
mempalace-mcpentry point started a full MCP server per client session. Each process could open its own local ChromaDBPersistentClient, which is one source of the concurrent-writer HNSW divergence cluster tracked in #1963.This PR changes the MCP stdio topology:
mempalace-mcpbecomes a lightweight stdio bridge./mcpendpoint.mempalace.mcp_server.handle_request()lazily.tools/callrequests are serialized through the daemon writer gate.tools/callrequests share one in-process writer lock.initialize,ping, and notifications stay outside the writer lock.mempalace-mcp-stdio.MEMPALACE_MCP_DISABLE_DAEMON=1provides an emergency rollback path.Important scope note
This PR is one implementation slice within #1963.
#1963 is a tracking epic for the broader single-writer architecture. This PR makes the MCP stdio path use the existing daemon owner and ties MCP writes into the same daemon serialization point as daemon jobs.
The full Tier 3 rollout still needs installer/config follow-through so hook and CLI workflows consistently use daemon-backed paths rather than direct ChromaDB writers.
Why this complements #1270
#1270 proposed the durable daemon + bridge architecture: one long-lived owner and thin clients.
This PR adds production pieces around that direction:
mempalace.daemoninstead of a second daemon implementation;/mcpendpoint;Why this does not touch
mine_palace_lock()The low-level palace lock remains fail-fast for direct external writers.
Earlier attempts to make the lock wait by default caused existing lock tests to fail or hang because direct Chroma/MCP writers are expected to raise
MineAlreadyRunningwhen another process holds the palace.This PR keeps that safety contract intact and moves queueing/serialization into the daemon path instead.
Compatibility
Existing MCP configs that call:
continue to work, but now go through the daemon bridge.
Raw stdio fallback:
Emergency rollback:
How to test
Focused tests:
Related safety tests:
Full CI shape:
References
Contributes to the Tier 3 daemon/bridge direction in #1963.
Complements the daemon + bridge architecture from #1270.
Related to #1229, #1888, and #1966.
Checklist
python -m pytest tests/ -v)ruff check .)