fix(mcp): reload the target table so a stub change needs no broker restart - #4321
fix(mcp): reload the target table so a stub change needs no broker restart#4321chenmingwei23 wants to merge 1 commit into
Conversation
GPT 5.6 Review — 🔴 changes requested (blocking)GPT 5.6 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- src/kiro_crew/slack/gateway.py:6282 -- Target-table write adds an awaited gateway boot step
BLOCKING -- src/kiro_crew/mcp_gateway/target_table.py:58 -- Custom socket paths bypass the target-table write gate
[BLOCK-MERGE] c845b16 |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound root-cause fix; but it converts a spawn-time-only trust anchor into a runtime-mutable exec-command file, and the security spec wasn't updated with it. Watch
[DESIGN-REVIEWED] c845b16 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBased on my analysis of the diff and the actual code, let me finalize my assessment of each candidate. Candidate 1 (concurrent Candidate 2 (custom Candidate 3 (respawn + prewarm skip pre-resolve reload): Confirmed both call sites omit No new grounded defect surfaced in Step 2 — the No findings. [OPUS-REVIEWED] c845b16 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSAdvisory premise-level review of First-Principles-Verdict: PASS Cause-level fix: the immutable spawn environment was the real constraint, and every piece added — file, reload, gate, fail-closed report — names its harm. What this change shipsIntent: make flipping one MCP server's stub switch take effect without freezing the dashboard behind a full broker teardown (#4317) — a FIX.
WatchOne sibling of the rewrite-requires-restart coupling remains: [FIRST-PRINCIPLES-REVIEWED] c845b16 |
9cee014 to
869d99d
Compare
|
Dispositions for the two blocking findings on
|
869d99d to
b38a7a6
Compare
|
Disposition for the blocking finding on
Also on this head: rebased onto |
b38a7a6 to
af54007
Compare
|
Dispositions for the two blocking findings on Both hold, and taken together they say the mechanism was wrong rather than under-guarded. This is the third round of blocking findings in this span, and each round closed the instance it was handed and received a sibling: a gate keyed on process start, then a publish/reload ordering gap, now a stale-hit path and a non-monotonic stamp. All four share one root -- freshness was being INFERRED from a number, so every patch added another place the inference could be wrong. So this round removes the inference instead of guarding it, and the diff gets smaller rather than larger.
Net effect on the design: no generation, no clock, no process-start gate, no background refresh task, no publish flag. What remains is publish-on-build, read-before-spawn, per-table precedence, environment as the fallback floor, and the owner-only trust check. Verification on the new head: 7 mutants applied and all killed, including the two that pin these fixes -- reverting to a miss-only reload fails |
…start Toggling one server's stub froze the MCP Management page for seconds to tens of seconds with every other switch disabled, because the apply tore the daemon down and respawned it. The respawn existed for one reason: the daemon resolved a stubbed server's real launch command from its own environment, which a live process cannot change. So the routing table was immutable for the daemon's lifetime, and a one-bit change to one server had to drain every pooled backend and every in-flight call to take effect -- while buying nothing, since an open session's MCP toolset is fixed at session/new and cannot pick up a new stub set either way. The rewriter already computed that whole mapping. It is now published beside the gateway socket as an owner-only targets.json whenever it is built, so an apply is a rewrite plus one atomic file write and the broker keeps serving. Publishing on EVERY build, including the one that precedes a broker start, is what keeps this simple. A published table can never be older than the environment of a daemon that could read it, so the daemon needs no freshness arithmetic to decide whether its copy is current -- no generation, no clock, no process-start comparison, and so nothing for a clock step, a VM restore or a supervisor respawn to skew. The reload happens off the event loop immediately before a backend is spawned, so a backend is spawned from the mapping on disk at that moment. It runs before resolving rather than only after a miss, because a stale SUCCESS is the harder case: a server whose target command changed would otherwise keep resolving to the previous command for as long as the cached copy survived. Exactness also matters for a miss, because the stub treats an unknown target as terminal and deliberately does not fall back to a per-session exec, so that a genuinely broken backend cannot crash-loop per session -- a server stubbed moments earlier would otherwise be reported unknown and lost for the whole life of the session that asked for it. A spawn already forks a process, so one stat there is not a cost worth optimising against correctness. Precedence is per table, never per key: a table that loads is the whole answer, so a server the operator just unstubbed stops resolving even though the daemon's environment still names it. Merging key-by-key would have made unstubbing a no-op until the next restart. The environment stays as the floor -- a missing, foreign-owned, group-writable or unparseable table falls back to it -- and a publish that does not land fails closed rather than letting a broker serve routing nobody asked for. Closes #4317
af54007 to
c845b16
Compare
|
Disposition for the Design Review concern on
|
|
Disposition for the First Principles concerns on Both watch items were correct, and both were mine: the design was restructured and the CHANGELOG and commit message were updated with it, but these two were not.
No code behaviour changed for either item; the first is prose, the second a comment. Full local floor re-run green on the new head (56456 pytest passed, the nine host failures pre-existing). |
Stopping to re-decide the premise, not patching round fiveBoth findings on The two current findings, and why each is entangled1. Correct, and it is a direct consequence of the mitigation I added last round. I noticed the gap while implementing it and judged it acceptable because every entry in that registry is home-anchored; that judgement was wrong to leave unstated. The proposed fix -- derive the table from a fixed write-protected runtime directory, independent of 2. Also correct, and verified rather than taken on faith: This one is a recurrence. The same boot-path publish was raised in round one and I dispositioned it as moot. It has come back as blocking, which means my round-one reasoning was not sound and I did not revisit it. The proposed fix -- move publication past readiness -- reintroduces a window in which a daemon can start against an unpublished table. Publishing on every build, boot included, is precisely what let the round-three restructure delete the generation, the clock and the process-start gate: a published table can never be older than the environment of any daemon that could read it. Moving the publish past readiness gives that reasoning back. The whole span, for the record
Four rounds of fixes held. What did not hold is the premise underneath them: publish a file next to the socket and have the daemon re-read it. Each round closed a real hole and opened the next one, and the last two cannot both be closed without giving back an earlier guarantee. What I think is actually rightThe reported bug is that the Servers page freezes for up to 20s with every control disabled. It is not that the broker restarts. The restart only had to leave the request path -- and the file table was never load-bearing for the fix, only for making the restart unnecessary. The smaller shape: keep the broker restart, take it off the request path. What that concedes, stated plainly: a short window after the toggle where a newly created session still gets the old stub set. That window is far less visible than it sounds -- a session's MCP toolset is fixed at The alternative worth considering, if in-place re-routing is genuinely wanted, is a control verb on the daemon's own socket rather than a file: the socket is already owner-only and already the daemon's input surface, so it needs no new trust check, no gate entry and no boot publish. I checked -- no such surface exists today (the socket serves MCP connections only; the signal handlers are shutdown-only), so that is a design round, not an edit. I would rather ship the small version and file the in-place re-routing separately than take a fifth swing at this one. Flagging for a decision instead of choosing unilaterally, since collapsing the approach is a bigger call than any single finding here. |
Problem / Motivation
On Developer -> MCP Management -> Servers, flipping one server's stub switch freezes the page for seconds to tens of seconds, and every other switch on the page -- including the global sharing switch -- is disabled for the whole time, with no progress indication. It reads as a hung UI.
POST /api/mcp-gateway/servers/stubpersists the allowlist and then awaits_apply_mcp_stub(), which stopped the broker, re-ran the rewriter over every agent spec, respawned the daemon, and rebuilt the warm pool -- all inside the request. The stop alone has a 20s upper bound derived from the daemon's own drain budget (DRAIN_SECS 10 + POOL_SHUTDOWN_SECS 5 + SIGNAL_MARGIN_SECS 5), and its real cost scales with the live fleet, because every pooled backend and in-flight connection has to be torn down.Why it matters
Changing one server's stub bit is a one-bit configuration edit, and it costs a full broker cycle: every pooled backend drained, every in-flight tool call cancelled, the warm pool rebuilt. The respawn also bought nothing for the sessions the operator has open -- a session's MCP toolset is fixed at
session/new, so an already-running session cannot pick up a new stub set no matter what the broker does, and a new session connects to the daemon fresh either way. The whole cycle was paid and discarded.What changed (motivation -> approach -> change)
Root cause. The daemon resolved a stubbed server's real launch command from its own process environment (
env_target_resolverreadingKIROCREW_MCP_TARGET_<SERVER>). A live process's environment cannot be changed, so the routing table was immutable for the daemon's lifetime and the only way to change it was to respawn.Approach. The resolution seam was already pluggable --
gatewaydcallsresolver(pool_key)and its own error text advertisesor pass a target_resolver, so reading the environment was a default implementation, not a constraint. The rewriter already computes the whole mapping. So the mapping is published beside the gateway socket as an owner-onlytargets.json, and the daemon reads it instead. An apply becomes a rewrite plus one atomic file write, with the broker left serving.The shipped design, and the two properties that make it simple:
_acquire_backend. Before resolving rather than only after a miss, because a stale SUCCESS is the harder case: a server whose target command changed would otherwise keep resolving to the previous command for as long as the cached copy survived. Exactness matters for a miss too, because the stub treats an unknown target as terminal and deliberately does not fall back to a per-session exec (so a broken backend cannot crash-loop per session) -- a server stubbed moments earlier would otherwise be reported unknown and lost for the whole life of the session that asked for it. A spawn already forks a process, so one stat there is not a cost worth trading against correctness.Precedence is per table, never per key. A table that loads is the whole answer, so a server the operator just unstubbed stops resolving even though the daemon's environment still names it. Merging the two sources key-by-key would have let that entry keep resolving it, making unstubbing a no-op until the next restart.
Failure handling. The environment remains the floor: a missing, foreign-owned, group-writable, unparseable or wrong-version table falls back to it, so the worst case is the previous behaviour rather than a failed spawn. A publish that does not land returns
None, so neither a broker start nor anapplied: truereport proceeds on routing the broker never saw.Trust.
targets.jsonmaps a server name to a command the broker execs, and the broker does not run under the agent's sandbox. The reader refuses a table owned by another account or writable beyond its owner, but those checks cannot refuse a same-uid write -- so the path is also registered insecurity._WRITE_PROTECTED_HOME_PATHSandsecurity._WRITE_PROTECTED_BASH_LEAVES, the same registry that already covers the browse launch config and the on-call schedule as inputs to a security or authorization decision. Kiro Crew publishes the file directly and does not route through that gate, so its own writes still work. The table carries command lines only; backend environment keeps its own separate assembly path.Tests
test/test_mcp_target_table.py(new, 21 tests) andtest/test_mcp_stub_apply.py(extended). Mutation-verified -- 9 mutants applied across the design, all killed:{}) is a real "nothing stubbed" state rather than a fallback;0o600; the path is write-gated on both the file-edit and shell paths, with the gated path derived fromdefault_target_table_pathso renaming the runtime directory cannot silently move the file out from under the gate;Path.stat/Path.read_textraise);None; a failed rewrite never publishes;Two existing tests pinned the old restart contract (
test_mcp_stub_apply.py,test_slack_gateway_coverage.py) and were retargeted to the republish contract, keeping the reason each existed -- the rewriter must still re-run.Manual verification
N/A -- unit coverage sufficient. The change is backend-only with no rendered delta, and each load-bearing property (per-table precedence, reload-before-resolve including the stale-success case, fail-closed publish, the write gate on both paths) is pinned by a mutation-verified test rather than a click-through.
Full local floor green:
isort,flake8,mypy(987 files), andpytestat 56456 passed. Nine failures on this host are pre-existing and reproduce identically on a clean checkout of the base (they depend on the host's/tmplayout, a realghonPATH, and a live flock holder), so they are environmental rather than introduced here.Why no screenshot: backend-only change; no frontend path is touched and nothing rendered changes. The UI's page-wide disable during the apply is deliberately left alone here and tracked separately.
Related Issues
Closes #4317
Checklist