fix(mcp): bound repeated tool rediscovery - #2989
Conversation
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
Blocking: this bounds only notifications that arrive while the same tools/list request is still in flight; it does not bound the response-followed-by-notification loop described in #2981.
supersededAttempts is local to refreshToolLoop(). Once a refresh completes, refreshState is cleared and the next notification creates a new loop with the counter reset to zero. A server that sends list_changed just after every tools/list response can therefore continue rediscovery indefinitely.
The regression fixture currently sends and awaits the notification before returning the response:
That guarantees the favorable state.pending === true ordering and does not reproduce the issue's "after every response" timing. I changed only the fixture to schedule sendToolListChanged() after returning the response; the test then observed 127 tools/list calls instead of 3 in about 675 ms.
The test also enables the hostile behavior only after the initial sync() has settled, and tests removal only after the retry-limit error has already occurred. It therefore does not verify the two other contracts requested in #2981: initial sync settling and configuration removal preempting an active rediscovery storm.
Could we keep the retry/suppression state at the connection-generation level instead of inside one refresh promise? After reaching the limit, further notifications for that generation should remain suppressed until a defined recovery boundary, such as a quiet period, explicit refresh, or reconnect. The regression test should schedule the notification after each response and attempt removal while rediscovery is still active.
Generated-by: Codex Signed-off-by: Oniel Alejo Feliz <197416079+XonkelX@users.noreply.github.com>
Generated-by: Codex Signed-off-by: Oniel Alejo Feliz <197416079+XonkelX@users.noreply.github.com>
909a2e8 to
f6d9cdd
Compare
There was a problem hiding this comment.
Thanks for moving the suppression state to the connection-generation lifetime. That addresses the reset identified in the previous review, but the bound still uses notification timing and count rather than actual rediscovery passes.
A slow server can still bypass the bound: if each tools/list takes longer than the one-second quiet period and then emits list_changed, every notification resets consecutiveSignals. With a 1.2-second response delay, rediscovery continued indefinitely without producing the frequency error.
The opposite timing also causes a regression. MCP notification handlers are dispatched concurrently, so three notifications arriving during one in-flight refresh increment the counter three times. The third marks the generation as suppressed, and refreshToolLoop() then discards the successful definitions it already received. In a reproduction where the server changed from echo to replacement, the manager reported changed too frequently and permanently retained the stale echo snapshot.
Could we count actual rediscovery passes at the per-generation scheduler instead? Notifications received during an active request should coalesce into one pending pass, and the quiet period should begin after a refresh completes. A delayed-response regression and a three-notification burst regression would cover both sides of the invariant.
AI-assisted review disclosure: Codex coordinated the source and lifecycle analysis, and Claude Opus performed an independent adversarial review. The reported behavior was reproduced against the exact PR head; the human reviewer retains responsibility for the final review decision.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed against #2981.
The 5ms notify-after-list flood does stop at three tools/list calls, and disconnect does not wait on an in-flight refresh. Removal can still settle. Keeping the previous snapshot is the right call; disconnecting would open a reconnect churn path.
Two holes remain, so I am not approving yet.
A third list_changed inside the 1s window is discarded without starting a refresh (refreshToolsAfterNotification, consecutiveSignals >= 3). A legitimate server that notifies three times during startup batch registration loses that last change and stays on changed too frequently until a >1s quiet gap or a manual refreshTools. refreshTools has no production caller.
The quiet window is measured from the last notification, not from a finished rediscovery. If tools/list takes longer than 1s (the list timeout is 15s) and then emits list_changed, consecutiveSignals resets every time. That is the original loop with a longer period. The new tests only pin the 5ms path.
Also: this branch currently reports no CI checks.
AI-assisted review: Grok 4.6, opencode-go/deepseek-v4-flash:max, and ark-coding-plan/glm-5.3 each reviewed the PR independently. I walked refreshToolsAfterNotification against a 5ms flood, a 100ms legitimate triple, and a 1100ms slow loop. Unverified by me: I did not run the MCP suite.
|
Thanks for tackling the rediscovery loop — the direction is right and test 1 is a genuine regression test. I ran the suite and probed two edge cases the tests don't cover; both surfaced issues worth resolving before merge. Conclusion: PASS with two mandatory-handling P2s (both verified by reproduction). P2-1 — the bound can be bypassed by a slow hostile server; the rediscovery loop continues indefinitely. P2-2 — three legitimate notifications within 1s are misclassified as hostile: spurious error and discarded refresh results. Optional nits (P3): the suppression error sticks until the next notification (no auto-recovery while the server is quiet); test 2 ("config removal preempts in-flight rediscovery") already passes on AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS（两个 P2 必须处理或明确延后）。两个 P2 均已实证复现：① 慢速 hostile server 可绕过 bound——计数仅当相邻通知 ≤1s 才累计，1.2s 延迟 + notify-per-list 时计数每次重置为 1、永远达不到 3，rediscovery 循环以每 RTT 一次的速率无限持续，PR 声称修复的核心场景只对快速通知成立；② 误伤——1 秒内 3 次合法通知被判 hostile，产生虚假 error 且三次有效变更全部被丢弃（工具列表冻结在变更前状态），计数 3 与窗口 1s 无任何协议或工程依据。建议改用 trailing debounce（限速而非判定 hostile，约 15 行 vs 当前 160+ 行）。P3 可选：suppression error 粘滞不自愈、test 2 在 main 上本已通过（checklist 声明不实）、suppressed 后仍逐通知 emit。 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughWhat this PR solvesMCP servers can emit The manager now:
Design assessmentThis change extends the existing The added state coordinates concurrent refreshes, counts superseded attempts, retains the last valid snapshot, and resets state during connection lifecycle changes. This complexity is necessary for the requested behavior. The solution remains coherent, but the state and race handling increase review cost. The expanded tests cover remote and stdio transports, refresh coalescing, cancellation, stale bindings, reconnect and close races, schema validation, error handling, and configuration reconciliation. No safe deletion or simplification is evident without weakening regression coverage. Validation and risksReported validation includes MCP tests, lint, formatting, notices, and targeted Biome checks. The affected MCP workspace passes its test command. The root build remains blocked by an unrelated existing The review identified two concrete behavioral risks:
Additional risks include persistent suppression errors, status updates and emissions from suppressed notifications, and a public Review-relevant risksThe current diff changes user-visible tool availability and diagnostic behavior by suppressing refreshes and retaining the previous callable snapshot. Material changes in this area require independent human review under repository policy. The current diff changes MCP synchronization, reconnection, transport-closure handling, and server removal behavior. Material changes in integration and operational behavior require independent human review under repository policy. No exported declaration changes are reported. No licensing, release, or governance effect was identified in the current diff. Required checks remain unverified unless confirmed directly. The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughMCP tool discovery now bounds repeated ChangesMCP tool discovery refresh
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to Concurrent refresh requests may reset the safeguard against repeated tool rediscovery, allowing a hostile server notification pattern to continue longer than intended and causing excess refresh work. The PR is mergeable with explicit owner awareness or follow-up on preserving the bound for concurrent callers. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/mcp/src/index.ts (1)
287-294: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
refreshToolsclears the suppression bound unconditionally.Every public
refreshToolscall replacesrefreshNotificationStatewithrefreshPasses: 0, suppressed: false. A refresh that is already in flight for the same client and generation keeps running throughrefreshToolLoop, which then re-reads the new state object and restarts the budget from zero. A caller that pollsrefreshToolswhile a hostile server notifies removes the bound.If the reset is intended only as an operator escape hatch, restrict it to the case where no refresh is active:
♻️ Restrict the reset to idle connections
- entry.refreshNotificationState = { - client, - connectionGeneration, - refreshPasses: 0, - suppressed: false, - }; + const active = + entry.refreshState?.client === client && + entry.refreshState.connectionGeneration === connectionGeneration; + if (!active) { + entry.refreshNotificationState = { + client, + connectionGeneration, + refreshPasses: 0, + suppressed: false, + }; + } return this.startToolRefresh(serverId, entry, client, connectionGeneration);packages/mcp/src/__tests__/manager.test.ts (1)
917-925: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth transports repeat the same
beforeToolListclosure.Lines 917-924 and Lines 940-947 are identical. Hoist one closure above
createServerand pass it to bothcreateProtocolServercalls.♻️ Share one closure
let toolListClockAdvance = () => {}; + const beforeToolList = async () => { + toolListClockAdvance(); + const gate = nextToolListGate; + if (!gate) return; + nextToolListGate = undefined; + gate.markStarted(); + await gate.waitForRelease; + };Then use
beforeToolList,at both call sites.As per path instructions: "Flag concrete cases where code can be deleted or simplified."
Also applies to: 940-948
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c83fdb9-2fa2-4a3c-8f72-5fe4332142e8
📒 Files selected for processing (2)
packages/mcp/src/__tests__/manager.test.tspackages/mcp/src/index.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Generated-by: Codex Signed-off-by: Oniel Alejo Feliz <197416079+XonkelX@users.noreply.github.com>
|
Thanks for the follow-up — the two P2s from the earlier review are addressed, and the new tests are solid. I re-verified on the updated head. Conclusion: the blockers are resolved. Ready to merge once CI runs green (it currently needs approval to run). Verified on the updated head (
Remaining observations (non-blocking):
Nice addition: the error-sanitization tests (no secret leakage, control-character stripping in status errors) are a good hardening beyond the original scope. AI-assisted review disclosure: this follow-up was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：两个 P2 阻塞项已解决，可以合并（需先批准并跑通 CI）。已修复：① 慢速 hostile 绕过——删除了 quiet-period 重置逻辑，refreshPasses 只增不减，测试改为在 tools/list 响应后推进时钟（1.1s 间隔）仍断言 bound；② 有效结果被丢弃——refreshToolLoop 现在先应用 snapshot 再抛 frequency error，新测试验证 suppressed 时最新有效工具列表仍被发布；③ burst 合并——新增测试验证 in-flight 刷新期间到达的通知被合并、无 error。本地跑工作区测试 62/62 通过（比之前多 3 个）。残余（不阻塞）：合法 server 做 4 次独立完成的刷新仍会触发频率 error（后果已减轻——最新快照被应用，工具列表保持最新），如需彻底关闭可改用 trailing debounce 限速而非计数；另外分支 CI 处于 action_required 状态，需批准运行。加分项：新增的错误消息脱敏测试（不泄露 secret、控制字符清理）。 |
|
LGTM. Both P2s from review are resolved, the new tests cover the slow-notifier and snapshot-preservation cases, and all CI checks are green (test_workspaces, typecheck, test, windows_recovery, windows_baseline). Merging. AI-assisted review disclosure: the review was AI-assisted (pi subagent on ollama-cloud/deepseek-v4-flash); the merge decision is the human maintainer's. |
3 participants
Summary
Bound consecutive MCP Tool rediscovery attempts when a server emits
tools/list_changedfor everytools/list. The manager now stops after three superseded attempts, keeps the previous callable snapshot, reports the hostile notification pattern, and remains removable through config sync.Fixes #2981
Verification
npm --workspace @maka/mcp test— 58 passednpm run lintnpm run format:checknpm run check:third-party-noticesnpx biome check packages/mcp/src/index.ts packages/mcp/src/__tests__/manager.test.tsnpm run buildreaches an unrelated existingpackages/runtime-host/src/server/session-transcript-pager.ts:249TS7006 error onmain; the affected MCP workspace builds and typechecks in its test command.Checklist
Does this PR entail a change in behavior?
Generated with Codex; reviewed and submitted by the human contributor of record.