Repository navigation
fix(mcp): every tool shares one persistent session per endpoint (#451) - #452
Merged
Merged
Conversation
Most aimdb-mcp tools opened a new AimX connection per call. Over serial that either failed (the tty is already open) or sent a new hello, which reset the board's session and discarded the drain cursor drain_record reads. - All tools and the records resource go through ConnectionPool::get_client (renamed from get_drain_client) and hold the per-client lock for one call. - get_connection, invalidate_connection, invalidate_drain_client and the per-tool fallbacks are gone, so no path opens a second connection. - Each endpoint has one connect in flight: parallel first calls wait for it instead of racing, where the loser found the tty busy. Tests: serial_session calls every tool between opening a ring's drain cursor and draining it, and checks the queued events and exactly one hello. New tests cover parallel first calls and the records resource. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A persistent client such as aimdb-mcp can occupy the only slot, and instance discovery's separate connection then fails. The default limit of 16 is not affected. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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
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.
Description
Most
aimdb-mcptools opened a new AimX connection on every call. Over serial that breaks the sessionget_recordanddrain_recordshare. A UART never closes, so a board learns of a new client only from ahello, and ahelloresets its session. Each call either failed (the tty was already open) or reset the board and discarded the drain cursor. #450 fixed this forget_recordanddrain_record; this PR finishes it for everything else.aimdb-mcplist_records,set_record,get_instance_info,graph_nodes,graph_edges,graph_topo_order,get_buffer_metrics,reset_buffer_metrics,get_stage_profiling,reset_stage_profiling, and the records resource. Each takes the per-client lock for one call.get_connection(),invalidate_connection(),invalidate_drain_client()and the per-tool fallbacks are removed. A tool called before the pool is initialized now returns an error, asget_recordanddrain_recordalready did.get_drain_client()is nowget_client(), since it is no longer drain-specific.Not in the issue: parallel first calls
get_drain_client()released its lock while connecting, so two first calls on one endpoint both connected. On serial the loser found the tty busy, which becomes more likely once every tool shares this path. Each endpoint now has one connect in flight and the others wait for it, without holding up other endpoints.aimdb-uds-connectormax_connections(1). A persistent client such asaimdb-mcpcan occupy the only slot, and instance discovery's separate connection then fails. The default limit of 16 is not affected. Checked with a throwaway program and a counting logger: one warning at a limit of 1 (through the builder orfrom_config), none at 2 or the default. There is no regression test in the repo.Not in this PR
get_buffer_metricsmatches only the Rust type name, although its parameter isrecord_key;get_stage_profilingmatches the name or the key. This is existing behaviour, and the new test passes a type name for that reason.Tests
tools/aimdb-mcp/tests/serial_session.rs:every_tool_shares_the_drain_session_over_serialSerialServerboard behind a pty. Open a ring's drain cursor, publish events, call each tool once, including error replies, then drain. Every call succeeds, the drain returns the queued events, and the board receives exactly onehello.serial_session.rs:concurrent_first_calls_open_one_sessionhello.tools/aimdb-mcp/tests/persistent_client.rs:the_records_resource_uses_the_poolserial_session.rsruns on Linux only, so it compiles to zero tests on macOS. CI runs it throughmake test.Each test fails when its bug is put back (Linux container, Rust 1.98):
list_recordsopens its own connectiondrain_recorddrops its client on an error replyRelated Issue
Checklist
make check). Left to CI. Run locally:aimdb-mcptests on macOS, and fmt and clippy foraimdb-mcpwithtransport-serialon macOS and Linux;🤖 Generated with Claude Code