Skip to content

feat(mcp): add a reload_server meta-tool - #176

Open
JYPochez wants to merge 1 commit into
mixelpixx:mainfrom
JYPochez:up/reload-server
Open

feat(mcp): add a reload_server meta-tool#176
JYPochez wants to merge 1 commit into
mixelpixx:mainfrom
JYPochez:up/reload-server

Conversation

@JYPochez

Copy link
Copy Markdown
Contributor

Changing Konnect's source and rebuilding does nothing until the MCP client is restarted, because the client spawns the server and holds it for the session. Replacing the binary underneath a running server just kills the connection.

A stdio server cannot restart itself by exiting — the client owns the process lifecycle and does not respawn it mid-session. So reload_server execs into the binary on disk instead: that replaces the process image while keeping the PID and the inherited stdin/stdout pipes, so the connection is never broken and the client goes on talking to the new build.

exec is a one-way door, so the new binary is run once (--version) and checked before the switch. A half-written copy, a failed link, or an unsigned binary macOS would kill becomes a refused call naming the reason rather than a server that is simply gone. confirm=true is required so a stray call cannot restart mid-task, and the reply is written before the switch since exec never returns on success. Windows gets a clear unsupported error — no exec equivalent keeps the pipes.

Router state does not survive; the new image starts at the starter kit. That is self-healing: a call to a previously loaded tool returns toolset_not_loaded naming its toolset, so recovery is one hop.

Also pins the meta-tool count in a test. It is quoted in DEV.md, README.md and tool-directory.md, so adding one now forces those to be updated in the same commit instead of drifting. Meta-tools 6 -> 7, docs updated to 194 total.

Changing Konnect's source and rebuilding does nothing until the MCP client is
restarted, because the client spawns the server and holds it for the session.
Replacing the binary underneath a running server just kills the connection.

A stdio server cannot restart itself by exiting — the client owns the process
lifecycle and does not respawn it mid-session. So reload_server execs into the
binary on disk instead: that replaces the process image while keeping the PID
and the inherited stdin/stdout pipes, so the connection is never broken and the
client goes on talking to the new build.

exec is a one-way door, so the new binary is run once (--version) and checked
before the switch. A half-written copy, a failed link, or an unsigned binary
macOS would kill becomes a refused call naming the reason rather than a server
that is simply gone. confirm=true is required so a stray call cannot restart
mid-task, and the reply is written before the switch since exec never returns
on success. Windows gets a clear unsupported error — no exec equivalent keeps
the pipes.

Router state does not survive; the new image starts at the starter kit. That is
self-healing: a call to a previously loaded tool returns toolset_not_loaded
naming its toolset, so recovery is one hop.

Also pins the meta-tool count in a test. It is quoted in DEV.md, README.md and
tool-directory.md, so adding one now forces those to be updated in the same
commit instead of drifting. Meta-tools 6 -> 7, docs updated to 194 total.

433 tests pass (3 added); clippy --workspace -D warnings clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mixelpixx

Copy link
Copy Markdown
Owner

Good problem, and the core insight — that a stdio server can't restart itself by exiting, because the client owns the process lifecycle — is right. But there are enough sharp edges that I'd like changes before it lands.

argv is lost. Command::new(&exe).exec() passes argv[0] and nothing else. main.rs parses --config <path> from argv, and the comment there notes KiCAD launches the server exactly that way. After a reload the server falls back to Config::load(), potentially with a different transport, ipc_address, project_dir or jlcpcb_db_path — a silent reconfiguration presented to the user as a successful reload. Re-exec with std::env::args_os().skip(1).

No transport guard. The whole "the connection survives" rationale is stdio-only. Under TransportMode::Http or Both, axum genuinely serves concurrently and the listening socket is O_CLOEXEC, so exec drops it along with every in-flight request. Gate on TransportMode::Stdio.

The 250 ms window is a real one. stdio.rs is a strictly serial loop, so nothing is running when the handler returns — good. But the handler spawns the timer and returns, the loop flushes and goes straight back to read_line, so within those 250 ms the client can send another request and the server will start handling it. exec then fires mid-write. write_atomic_unlocked guards its temp file with a ScratchGuard, and exec runs no destructors, so that leaves an orphaned scratch file beside the user's .kicad_sch. The target file itself survives — rename is atomic — but a transaction journal (#122) caught mid-commit needs the konnect transaction recovery path. Set a shutdown flag so the read loop stops accepting before the timer fires, and start the timer after the transport flush rather than inside the handler.

The --version check helps but doesn't close the hole. It's TOCTOU: the check and the exec are 250 ms apart, and the entire reason to call this tool is that a cargo build just wrote the binary — one still linking can pass --version and be replaced before exec. It also doesn't compare versions, so a no-op or a downgrade reloads silently.

Windows. The #[cfg(not(unix))] arm returns a clear error, which is right — but meta_tool_descriptions() has no cfg gate, so on this repo's primary dev platform the model sees a seventh meta-tool that is guaranteed to fail and pays its description tokens on every tools/list. Gate the description too, and then the 6→7 / 193→194 doc churn only applies where the tool exists.

Note main now has a test pinning the documented tool counts (crates/konnect/tests/doc_tool_counts.rs), so a meta-tool count change has to move DEV.md with it — that will catch you rather than the reviewer, which is the point.

Deferring to the next round rather than closing; ping me when you've reworked it.

@mixelpixx

Copy link
Copy Markdown
Owner

Checking in — this has been waiting on the rework since 2026-08-14, and I would like to either land it or close it rather than leave it open indefinitely.

The three things from the review still stand:

  1. exec() drops --config from argv, so a server started with an explicit config reloads without one and silently picks up different settings. That is the blocking one.
  2. No TransportMode guard. Re-execing is coherent for stdio; for HTTP it drops live connections with no handshake.
  3. Advertised on Windows, where it always fails. exec has no Windows equivalent, so the tool appears in the catalogue and returns an error on every call there. Either gate the registration by platform or implement a spawn-and-exit path.

(3) also has a cost you may not have noticed: adding a meta-tool takes the count from 6 to 7, and doc_tool_counts.rs enforces the meta-tool count in DEV.md, so that needs updating in the same PR.

No pressure on timing — say the word if you would rather I take it over, or if you would prefer to close it and revisit later. It is a reasonable feature and I do not want it to rot silently.

@mixelpixx

Copy link
Copy Markdown
Owner

Status check at Round 12: no commits since the rework request — all four items stand (argv preserved via env::args_os().skip(1); a TransportMode guard so HTTP/Both don't lose their socket to exec's O_CLOEXEC; gating the registration on unsupported platforms, not just the runtime error; stop accepting work before re-exec to close the ScratchGuard race). The branch is also now CONFLICTING and its counts predate five releases. Happy to review the moment those land; closing is also fine if you'd rather not carry it.

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.

2 participants