Skip to content

fix(mcp): fix RawResponseEnabled hot reload not working - #2654

Merged
Han-Ya-Jun merged 1 commit into
TencentBlueKing:masterfrom
Han-Ya-Jun:fix/mcp-raw-response-hot-reload-master
Apr 21, 2026
Merged

fix(mcp): fix RawResponseEnabled hot reload not working#2654
Han-Ya-Jun merged 1 commit into
TencentBlueKing:masterfrom
Han-Ya-Jun:fix/mcp-raw-response-hot-reload-master

Conversation

@Han-Ya-Jun

Copy link
Copy Markdown
Member

Why this change was needed:
When changing RawResponseEnabled configuration for an MCP server, the setting was not taking effect without restarting the service. This was because genToolHandler captured the rawResponseEnabled bool value at handler registration time, so subsequent changes via SetRawResponseEnabled had no effect on already-registered tool handlers.

What changed:

  • Changed genToolHandler signature from rawResponseEnabled bool to rawResponseEnabledGetter func() bool to accept a getter function
  • Updated tool handler to call rawResponseEnabledGetter() dynamically instead of using captured boolean value
  • Changed call sites to pass mcpServer.RawResponseEnabled method value instead of config.RawResponseEnabled or rawResponseEnabled boolean
  • Added unit test for dynamic getter behavior in hot-reload scenario

Problem solved:
RawResponseEnabled configuration changes now take effect immediately without requiring service restart. Tool handlers dynamically read the latest value from MCPServer on each execution.

Description

Fixes # (issue)

Checklist

  • 填写 PR 描述及相关 issue (write PR description and related issue)
  • 代码风格检查通过 (code style check passed)
  • PR 中包含单元测试 (include unit test)
  • 单元测试通过 (unit test passed)
  • 本地开发联调环境验证通过 (local development environment verification passed)

Why this change was needed:
When changing RawResponseEnabled configuration for an MCP server, the
setting was not taking effect without restarting the service. This was
because genToolHandler captured the rawResponseEnabled bool value at
handler registration time, so subsequent changes via SetRawResponseEnabled
had no effect on already-registered tool handlers.

What changed:
- Changed genToolHandler signature from rawResponseEnabled bool to
  rawResponseEnabledGetter func() bool to accept a getter function
- Updated tool handler to call rawResponseEnabledGetter() dynamically
  instead of using captured boolean value
- Changed call sites to pass mcpServer.RawResponseEnabled method value
  instead of config.RawResponseEnabled or rawResponseEnabled boolean
- Added unit test for dynamic getter behavior in hot-reload scenario

Problem solved:
RawResponseEnabled configuration changes now take effect immediately
without requiring service restart. Tool handlers dynamically read the
latest value from MCPServer on each execution.

Co-authored-by: claude <noreply@anthropic.com>

@wklken wklken left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #2654 Code Review 汇总报告

由 codex-internal (gpt-5.4) + claude-internal 双模型 review,主 agent 汇总整理。

变更概述

本次 PR 修复了 MCP Proxy 中 RawResponseEnabled 配置热更新不生效的问题。

根本原因genToolHandler 在 handler 注册时对 rawResponseEnabled 进行了值快照(静态 bool),导致后续通过 SetRawResponseEnabled() 修改配置后,已注册的 handler 仍使用旧值。

修复方案:将 genToolHandler 第三个参数从 bool 改为 func() bool(getter 函数),调用方统一改为传入 mcpServer.RawResponseEnabled 方法值,使 handler 在每次实际执行时动态读取最新配置。

变更文件

  • src/mcp-proxy/pkg/infra/proxy/proxy.go:18 行变更(函数签名 + 两处调用方)
  • src/mcp-proxy/pkg/infra/proxy/server_test.go:23 行新增(getter 行为单元测试)

问题列表

🔴 Blocking

🟠 Major

🟡 Minor

  1. 热更新窗口期存在短暂不一致 [codex]

    UpdateMCPServerFromOpenApiSpec() 的热更新路径中,SetRawResponseEnabled(rawResponseEnabled) 是在工具重新注册之后才更新共享状态。这意味着在新 handler 暴露到路由、旧 handler 退出的短暂窗口内,若有请求并发进来,读到的仍是旧配置,返回旧的响应包裹格式。

    建议将 SetRawResponseEnabled() 调用提前至 handler 注册之前,或为整个更新过程提供原子性保证(视线上对配置切换一致性的要求决定优先级)。

  2. 测试只覆盖了 getter 机制本身,缺少热更新行为回归测试 [codex + claude]

    现有新增测试仅证明 Go method value 能动态读取 receiver 的最新字段值,未验证:

    • genToolHandler 在实际 tool 调用中使用了 getter(而非静态值)
    • UpdateMCPServerFromOpenApiSpec() 热更新后,tool 响应结构确实发生了切换

    若后续某个注册点误传回静态 bool,现有测试仍会通过,无法感知回归。建议补充一条面向行为的集成/回归测试,直接断言切换 raw_response_enabled 前后的实际 tool 响应结构。

  3. 函数签名变更无文档说明 [claude]

    genToolHandler 签名变更(boolfunc() bool)未在函数注释中体现。建议更新注释,明确说明参数类型含义及变更原因,便于后续维护。

💬 Nit

  1. 函数注释中的变量名未同步更新 [claude]

    genToolHandler 内部注释仍使用旧的 rawResponseEnabled 变量名,实际参数已改为 rawResponseEnabledGetter。建议同步修正。

  2. 测试用例命名可更具体 [claude]

    当前测试名 "should dynamically update value for tool handler getter" 较为通用。建议改为 "should reflect raw_response_enabled hot updates in tool handler",更直接表达测试意图。


优点

  • 修复方向准确:将运行时配置从"注册时快照"改为"调用时读取",正确定位了根因。
  • 设计合理:使用 getter 函数符合依赖注入原则,避免引入全局状态。
  • 并发安全RawResponseEnabled() / SetRawResponseEnabled() 已有读写锁保护,本次复用现有 accessor,未引入额外数据竞争风险。
  • 变更面极小:仅修改函数签名和两处调用方,回归面可控。
  • 有配套测试:新增了针对 getter 机制的单元测试。

综合建议

整体方案合理,代码质量良好,可以批准合并。

合并前建议优先处理:

  1. 补充热更新行为回归测试(Minor #2)—— 这是最重要的改进点,确保修复的行为有测试保护
  2. 更新函数注释(Minor #3 + Nit #1)—— 维护成本低,建议顺手处理

热更新窗口期问题(Minor #1)在当前场景下影响较小,可视线上需求决定是否在本次处理。


由 codex-internal (gpt-5.4) + claude-internal 双模型 review,主 agent 汇总 | [from openclaw-internal]

@Han-Ya-Jun
Han-Ya-Jun merged commit 2e58c8b into TencentBlueKing:master Apr 21, 2026
3 checks passed
Han-Ya-Jun added a commit to Han-Ya-Jun/blueking-apigateway that referenced this pull request May 18, 2026
…ing#2654)

Why this change was needed:
When changing RawResponseEnabled configuration for an MCP server, the
setting was not taking effect without restarting the service. This was
because genToolHandler captured the rawResponseEnabled bool value at
handler registration time, so subsequent changes via SetRawResponseEnabled
had no effect on already-registered tool handlers.

What changed:
- Changed genToolHandler signature from rawResponseEnabled bool to
  rawResponseEnabledGetter func() bool to accept a getter function
- Updated tool handler to call rawResponseEnabledGetter() dynamically
  instead of using captured boolean value
- Changed call sites to pass mcpServer.RawResponseEnabled method value
  instead of config.RawResponseEnabled or rawResponseEnabled boolean
- Added unit test for dynamic getter behavior in hot-reload scenario

Problem solved:
RawResponseEnabled configuration changes now take effect immediately
without requiring service restart. Tool handlers dynamically read the
latest value from MCPServer on each execution.

Co-authored-by: claude <noreply@anthropic.com>
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