Repository navigation
MCP server: replies SDK clients accept, cancelling that works under load, safe captures - #554
Merged
Merged
Conversation
Every JSON-RPC reply carried both members, one of them null. JSON-RPC 2.0 says the other member must not exist, and the official MCP TypeScript SDK validates replies strictly, so it dropped every reply netscli sent. A client built on the SDK never received the initialize answer and timed out (seen with SDK 1.17.5 and 1.30.1). result and error are now skipped when absent. id is not: a reply to a request whose id was null or unreadable must still carry "id": null. The tests fail against the old shape.
The Rust tests check the server's replies against what the server itself expects, so a reply shape the official TypeScript SDK rejects passed all of them for months. scripts/mcp-smoke connects with the SDK's StdioClientTransport, lists the tools, calls list_network_interfaces (read-only, no network) and checks that an error reply arrives too. It fails with the SDK's own reason when it drops a message, and after 30 s overall. The SDK is pinned to 1.32.1 with a lockfile. A new CI job builds netscli on ubuntu-latest and runs the script, and the CI Gate waits for it. Against the 0.3.4 binary it fails at once with "Unrecognized key: error". Against this branch it connects and lists 9 tools.
The read loop took a slot before reading the next line, so with sixteen requests running nothing more was read: not notifications/cancelled, which is how a client frees a slot, and not EOF. Cancelling failed exactly when the server was busiest, and a client that closed stdin left every request running. A request that finds every slot taken now waits in its own task, where a cancel can reach it, and the loop goes back to reading. The backpressure that waiting gave is replaced by a bound: past 32 requests running or waiting, a request is refused at once with -32000 instead of being held in memory with its params. The 15-minute ceiling now starts when a request gets its slot. Two smaller transport fixes ride along: - An over-long line's tail stayed in the buffer and was read as the next message. The rest of the line is now discarded. - In-flight tracking was keyed by id alone, so a request finishing just as the client reused its id removed the new request's entry and left it impossible to cancel. An entry is now removed only by its own task. serve() takes the transport and the handler as parameters so the tests can hold every slot with requests that never finish. Line framing moves to lines.rs, which server.rs needed to stay under the size guard.
ping is MCP's liveness check and may come at any time, before initialize too. It was answered -32601 (or -32002 before initialize), so a client that pings took a working server for a broken one. It now gets an empty result. initialize echoed whatever protocolVersion the client sent, claiming support for versions this server has never seen. It now agrees the client's version when it is one of 2025-06-18, 2025-03-26 or 2024-11-05, and otherwise answers with the newest of those, as the MCP lifecycle asks. A client that names no version still gets 2024-11-05. The SDK smoke test pings too.
A capture runs on a blocking thread that dropping its future does not reach. A cancel, the 15-minute request ceiling and EOF all drop the future, and the capture carried on regardless, holding the interface and writing its file for up to an hour. Its capture slot went with the future, so cancelling and restarting got past the four-capture limit with the old captures still running. - Each capture gets a cancel token, cancelled when its future is dropped. libpcap's read times out every 250 ms, so the loop stops within that. - The slot is held by a task that lives until the capture has returned. - At EOF the server also stops background jobs, which belong to no request and ran on after the client had gone. - A blocking capture_pcap with only maxPackets fell through to core's one-hour ceiling, past the two-minute clamp. It now gets the 10 s default the tool advertises. The output file is now created with create_new before libpcap opens it. libpcap truncates whatever is at the name, and capture_pcap always wrote capture.pcap in the server's working directory, usually the user's project. It replaced any file of that name, and a capture.pcap symlink committed to a repository sent the capture through the link, as root if the server ran elevated. create_new refuses anything at the name, a dangling symlink included. An unnamed blocking capture now gets a new timestamped name, and a capture that cannot start removes the empty file it was given. start_pcap_capture and the job status now report outputFile, so a client knows where an unnamed job's capture goes before it ends. The pcap job tests move to dispatch/pcap_tests.rs, because the lifecycle test now waits for its job to end and dispatch/tests.rs is at the size guard. The symlink test is Unix-only and runs in CI.
A result over 1 MiB that was an object, not a list, was replaced whole
by {error, limit_bytes, size_bytes}. A capture too large to return lost
its file path and packet count with its packets, and a capture job's
result lost its jobId and status, so for an unnamed capture the model
could not even say where the file was.
An object over the cap now keeps every field except its largest list,
which is cut to fit the way an oversized array already was, with
truncated, returned, total and a note naming the list beside them. For
capture_pcap that list is packets, for get_pcap_capture_result
result.packets, and for inspect_host ports.
The tests move to limits/tests.rs, because limits.rs passed the size
guard.
The remote-text cap reached banner, hex_preview and info only. The Server header was cut to 256 characters in banner and returned whole beside it in http.headers, and an HTTP status line, mDNS service and host names and TXT properties were not capped at all. Every string under http and properties is now capped like a banner, and so are hostname and full_name. A real DNS name is at most 253 characters, so a valid one is never cut.
mdns-sd's daemon is a thread with multicast sockets on every interface, and it never stops by itself: it holds a sender to its own command channel. MdnsEngine::discover shut it down on its normal exit and when a browse task failed, and nowhere else. A service type without its final dot returned early through ?, and a dropped future (an MCP cancel or the request ceiling) never reached the end. Both left the thread running, re-sending every browse it had started, until the process exited. The daemon is now owned by a guard that shuts it down when dropped, and service types are checked before any daemon starts. browse_on takes the daemon so the tests can keep a handle and see that it stops.
service_types had no length limit. Each entry is its own multicast browse, repeated for the whole window (up to 30 s), so a long list was a burst of mDNS queries on the LAN, and a repeated type was browsed twice. Duplicates are now dropped and more than 32 types is refused with -32602. The tool schema says maxItems 32.
The rows were read through (*table).Table.as_ptr(), which borrows the one-row array the struct declares and then reads NumEntries rows through it. Under Rust's aliasing model that borrow covers only the first row, so reading the rest is undefined behaviour (Miri flags it), even though it is not known to miscompile today. The pointer now comes from the table pointer with addr_of!, for both GetIpNetTable2 and GetUnicastIpAddressTable. SendARP's buffer was a [u8; 8] where the API documents an array of ULONG. It is now [u32; 2], so it also has a ULONG's alignment. A one-off local run resolved three live neighbours with SendARP and got the same MACs the table holds.
Discovery re-checks each neighbour-table-only host with a blocking SendARP, and every check started at once, each timed from that moment. With more candidates than tokio's blocking pool has threads, the later checks spent their 2.5 s queued and real neighbours were dropped. The checks now run 64 at a time, each timed from when it starts. The table's size probably keeps this rare, which is unmeasured.
The probe sent the target's bare address as the Host header, so an IPv6 target got Host: fe80::1, which is malformed, and strict servers answered 400 instead of the headers the probe reads. probe_http now takes the address and brackets an IPv6 one, as in a URL.
Only a refused connect (closed) and a timeout (filtered) were mapped. A connect answered with an ICMP host, network or administratively prohibited unreachable came back as error, so a host behind a firewall that rejects rather than drops returned an error row for every port, and the MCP server, which keeps error ports, returned all of them. Those now read as filtered, as nmap reports them, and keep the error text so the reason is still there. A timeout stays filtered with no text, as before.
The OUI loader tried data/oui.min.json.gz and data/oui.json in the working directory before the embedded copy, and any JSON that was not an object parsed to an empty map that still won. A stray file in whatever directory netscli ran from replaced vendor names, or removed them, with nothing to say so. The working-directory candidates are gone (NETSCLI_OUI_PATH and a copy next to the executable still work, as the README says), and JSON that is not an object is skipped.
When the preferred interface's network is wider than /16, the default narrows to the /24 around this host. That host address came from detect_default_ipv4_addr, which picks an interface by a different rule: the first one up, unsorted, which may be a tunnel, and with no IPv4 address on it the default fell back to 192.168.1.0/24. The address now comes from the same interface as the network. On Windows it is the adapter's own address inside the chosen prefix. detect_default_ipv4_addr is unchanged for its other caller. No test: which interface wins depends on the machine's adapters, and nothing here can stand in for them.
raw_icmp is compiled only off Windows (ping.rs gates it), so its cfg(windows) setsockopt branch was unsafe code that no build ever compiled or checked.
The example iterated the Result that scan_host returns and read .port on the Vec inside it, so it did not compile. It now awaits with ?, and compiles as written (checked as a throwaway example). The features table also lists mdns and ts.
The two sample replies showed "error": null beside the result, which is the shape the official SDK rejects. Both are re-captured from a real session against this branch. The page also says what changed for a client: replies over 1 MiB keep their other fields, captures stop on cancel and EOF, cancelling works when every slot is busy, calls past 32 are refused, ping is answered, and capture files are never replaced or followed through a symlink.
Contributor
|
Site preview: https://pr-554.netscli-site-preview.pages.dev Built from 422219f with Production is unaffected: netscli.com is served from GitHub Pages via |
# Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit fixes for the MCP server and the core findings in
areas/rust-core-mcp.md(local only). The DNS fallback is in #551.The headline fix
Clients built on the official MCP TypeScript SDK can connect.
resultanderror, one of them null. The SDK drops such replies, so its clients never received theinitializeanswer."id": nullis still written where JSON-RPC requires it.mcp-smoke) drivesnetscli servewith the real SDK, pinned at 1.32.1 inscripts/mcp-smoke. It fails at once on the 0.3.4 binary ("Unrecognized key: error") and passes here.Also fixed
capture_pcapwith onlymaxPacketsdefaults to 10 s.truncated,returnedandtotal. Job status and start reportoutputFile.pingis answered, and only real protocol versions are agreedmcp.mdsamples were re-captured from a real session.Checked (by the fixing agent, in its own target dir)
cargo fmt --all --check.cargo clippyon netscli-core, netscli-mcp and netscli, with and without--features pcap,-D warnings.cargo test:mcp_stdiotestsDecision for you
start_pcap_capture, the job-style tool meant for long captures, is still clamped to 120 s. The original design said the job tool is for the hour-long case. It is left as it was.Not in this PR
libcdependency. It will be a small follow-up.netscli serveexit promptly at EOF while a probe is still running is in the CLI PR.