Skip to content

fix(rmcp): drive the MCP handshake once at worker startup - #110

Open
harsh04044 wants to merge 1 commit into
ContextVM:mainfrom
harsh04044:fix/rmcp-startup-handshake
Open

fix(rmcp): drive the MCP handshake once at worker startup#110
harsh04044 wants to merge 1 commit into
ContextVM:mainfrom
harsh04044:fix/rmcp-startup-handshake

Conversation

@harsh04044

Copy link
Copy Markdown

Standalone availability fix in the rmcp server worker. No payments code in it.

The problem - one inbound event from anyone permanently kills the server for everyone. All Nostr clients are multiplexed through a single rmcp service, and rmcp's pre-service handshake only accepts an initialize request as its first message. Anything else (a notifications/initialized, a response, an error response) returns ExpectedInitializeRequest, which drops the transport, cancels the worker through its drop guard, and closes everything. Nothing brings it back, so every client after that just hangs. No auth needed: the default allowlist is empty, so a stranger's event gets parsed and dispatched like any other. No attacker needed either. Nostr does not order delivery, so an honest client whose notifications/initialized lands before its own initialize kills the server it is talking to.

The fix - the worker satisfies the handshake itself at startup, sending the synthetic initialize + initialized pair to the handler right after take_message_receiver() and before the loop drains anything. The handler channel is single-producer and FIFO, so it is always ahead of every inbound event, for every peer and every arrival order. Race-free by construction rather than lucky, and it relies on rmcp tolerating nothing, so an upstream upgrade cannot undo it. If those sends fail we close the transport before returning, otherwise the event loop, cleanup and subscription tasks leak for the process lifetime.

Deletions - that makes the per-client bootstrap dead: should_inject_stateless_bootstrap (it only fired for a Request, which is exactly why a first notification or response walked past it into the handshake), initialized_clients, and is_synthetic_initialize_message with its id-rewrite guard, so the event-id rewrite is now unconditional. Checked every reader twice; they were all in this file. Kept on purpose: both synthetic builders and both sentinel response drops in forward_server_internal, which are not leftovers but now run on every startup, since the startup initialize comes back carrying the sentinel id and has to be dropped before it can reach a real client.

One behavior change - no client's initialize runs rmcp's version negotiation any more, so every client is answered with the handler's declared protocolVersion instead of an echo of the version it asked for. Clients after the first already worked this way. Measured: a first client requesting 2024-11-05 against a handler declaring 2025-11-25 got 2024-11-05 before, gets 2025-11-25 now. Left a comment on this one. Announcements are byte-identical for a handler that does not override the version, and everything else on the wire is untouched.

Tests - new tests/rmcp_handshake_survival.rs, 8 tests over the mock relay with no network, 7 of them red before the fix: serve() resolving with zero inbound traffic (it never resolved before), the three hostile first-message shapes each followed by a fresh client that must still connect and list tools, the honest reorder with no attacker, the same kill with an inbound middleware registered so dispatch runs on detached tasks, and one test per wire behavior above. Ordering-sensitive tests run multi_thread, since on current_thread the reorder cannot fire and they would stay green while proving nothing. Three mutations (drop the startup drive, swap the two sends, move it after the loop starts draining) all die. 836 tests passing under --all-features, 671 under --no-default-features.

Three follow-ups filed - all pre-existing, none introduced here. Before this fix they were masked, because the same traffic killed the whole server instead, which is worse; now they are reachable rather than fatal. Inbound ids are rewritten to the public Nostr event id, so a stranger who reads a victim's event id can cancel that victim's in-flight request. Server-initiated request ids come from a counter starting at zero and any matching inbound response satisfies them, so a stranger can answer a pending list_roots with a forged payload. And a post-handshake initialize from anyone overwrites the peer_info every client shares. All three come from one service serving every client; one service per client pubkey closes all of them and deletes the synthetic handshake machinery too, but that is a worker rewrite, not this PR.

The client worker needs nothing: it rejects any event not authored by the configured server pubkey, and rmcp's client-side init wait skips unknown notifications instead of failing.

@harsh04044

Copy link
Copy Markdown
Author

@ContextVM-org one behavior change in here I'd like a call on before this merges.

What changed - negotiate_protocol_version only runs inside rmcp's pre-service handshake, and the worker now consumes that handshake at startup, so no client's initialize is negotiated any more. Measured with a first client requesting 2024-11-05 against a handler declaring 2025-11-25: it got 2024-11-05 before, gets 2025-11-25 now.

Why it's not new for most clients - clients after the first already got the un-negotiated version, since their initialize reached the running service rather than the handshake. So this makes every client uniform, just in the less conformant direction: the MCP lifecycle spec expects the server to answer with the requested version when it supports it.

Why I didn't fix it here - one service can't hold a different negotiated version per peer, so per-client negotiation needs one service per client pubkey. That's the same change that closes the cross-client cancellation, response spoofing and shared peer_info items I filed, and it's a worker rewrite rather than something to bolt onto an availability fix.

The ask - ship as is and pick this up with the topology change, or do you want the adapter to echo the requested version itself in the meantime? The second is doable, since the worker sees the requested version on the way in and the response on the way out and could correlate them on the event id, but it adds per-request state and fakes a negotiation the service never actually did. I'd rather not, but it's your call.

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.

1 participant