fix(broker): take RLock in findServerByName and use MCPName() in log by SarthakB11 · Pull Request #923 · Kuadrant/mcp-gateway

SarthakB11 · 2026-05-10T10:38:12Z

Summary

Found another concurrency hole in the broker, in the same family as the recent mcpServers / session-cache fixes (#860, #872, #888, #918, #921).

findServerByName in internal/broker/filtered_tools_handler.go iterates broker.mcpServers without taking broker.mcpLock. OnConfigChange mutates the same map under the write lock at broker.go:175,190,197, so a tools/list request that lands during a config reload races on the iteration. The existing helpers ToolAnnotations and GetServerInfo already RLock around the same map, so this fix just brings findServerByName into line.

While I was in the file I also fixed an adjacent log line in filterToolsByServerMap. It passed the method value upstream.MCPName instead of calling it (MCPName()), so slog rendered a method-value formatter string in place of the actual server name. Happy to split that into a separate PR if preferred.

TestFilterTools_DataRace exercises FilterTools and a mutator that mirrors OnConfigChange under broker.mcpLock from two goroutines. Pattern matches manager_test.go::TestMCPManager_status_DataRace. Run with go test -race ./internal/broker/....

Verification

go build ./...
go vet ./...
go test -race -count=1 -timeout 60s ./internal/broker/...
make lint

Summary by CodeRabbit

Bug Fixes
- Fixed race condition in tool filtering that could occur during concurrent configuration updates.
- Corrected server identification logging in tool filtering operations.
Tests
- Added a concurrency test to validate thread safety during simultaneous tool filtering and configuration mutations.

coderabbitai · 2026-05-10T10:38:25Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cf9a65da-d69a-4a10-bbdf-712f4062cb65

📥 Commits

Reviewing files that changed from the base of the PR and between e446ebd and 03c1124.

📒 Files selected for processing (2)

internal/broker/filtered_tools_handler.go
internal/broker/filtered_tools_handler_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

internal/broker/filtered_tools_handler.go
internal/broker/filtered_tools_handler_test.go

📝 Walkthrough

Walkthrough

This PR adds RLock protection to findServerByName, corrects a logging call to upstream.MCPName(), and introduces TestFilterTools_DataRace that concurrently exercises FilterTools and mcpServers mutations.

Changes

Synchronization and Race Condition Prevention

Layer / File(s)	Summary
Lock Protection for Server Lookup `internal/broker/filtered_tools_handler.go`	`findServerByName` wraps `broker.mcpServers` access in `mcpLock.RLock()`/`RUnlock()` to prevent concurrent modification races.
Logging Correction `internal/broker/filtered_tools_handler.go`	Debug log for missing tools now calls `upstream.MCPName()` as a method instead of referencing the field.
Data Race Test `internal/broker/filtered_tools_handler_test.go`	Adds `sync` import and `TestFilterTools_DataRace` that runs a reader goroutine calling `FilterTools` and a writer goroutine swapping `mcpServers[serverID]` under `mcpLock` for ~200ms.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

bug: data race in findServerByName - missing lock on mcpServersbug #947 — Similar fix adding mcpLock RLock/RUnlock around broker.mcpServers access and a concurrency test.

Possibly related PRs

Kuadrant/mcp-gateway#943 — Modifies the same findServerByName area (related to upstream server selection and concurrent safety).

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: acquiring RLock in findServerByName and fixing the MCPName() method call in logging.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly

Built for teams:

Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

david-martin · 2026-05-14T09:38:16Z

This will need a rebase against main. A few related PRs have merged that touch overlapping files:

Refactor/manager channel event loop #943 refactored the manager to a channel-based event loop and modified filtered_tools_handler
fix: serialize concurrent manage calls in MCPManager #936 added manage() serialization

The RLock fix and MCPName() logging fix in this PR should still apply, but please verify after rebasing.

findServerByName iterates broker.mcpServers without taking broker.mcpLock, while OnConfigChange mutates the same map under the write lock. A tools/list call concurrent with a config reload can therefore data-race on the iteration. Mirror the locking pattern used by ToolAnnotations and GetServerInfo: take an RLock for the duration of the loop. Also fix the adjacent log line in filterToolsByServerMap which passed the method value upstream.MCPName instead of calling it. The slog encoder rendered a method-value formatter string instead of the actual server name, destroying the diagnostic value of the line. Add TestFilterTools_DataRace exercising FilterTools concurrently with a mutator that mirrors OnConfigChange under broker.mcpLock. Run with go test -race ./internal/broker/... Signed-off-by: SarthakB11 <[email protected]>

SarthakB11 · 2026-05-14T11:34:15Z

Rebased onto main. RLock fix and MCPName() log fix both survived #943's channel-event-loop refactor; mcpLock is still in place at broker.go:66 and OnConfigChange still takes the write lock, so the race was real. Test fixture updated to use upstream.NewActiveForTesting(...) for the new ActiveMCPServer interface. Build + race + lint clean locally.

david-martin · 2026-05-20T10:04:09Z

Hey, thanks for the contribution. Just a heads up: reviews are currently being prioritised around the v0.7 milestone. PRs tied to v0.7 issues will be reviewed first. We'll get to this one, but it may take a bit longer than usual.

jasonmadigan · 2026-06-11T15:54:01Z

thanks @SarthakB11, closing -- this was already fixed on main by 904f2b7 (@Aman-Cool).

david-martin mentioned this pull request May 14, 2026

fix: add missing mcpLock.RLock in findServerByName to prevent data race #951

Closed

SarthakB11 force-pushed the sarth/filtered-tools-race branch from e446ebd to 03c1124 Compare May 14, 2026 11:34

jasonmadigan closed this Jun 11, 2026

Navigation Menu

Repository navigation

fix(broker): take RLock in findServerByName and use MCPName() in log - #923

fix(broker): take RLock in findServerByName and use MCPName() in log#923
SarthakB11 wants to merge 1 commit into
Kuadrant:mainfrom
SarthakB11:sarth/filtered-tools-race

SarthakB11 commented May 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 10, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

david-martin commented May 14, 2026

Uh oh!

SarthakB11 commented May 14, 2026

Uh oh!

david-martin commented May 20, 2026

Uh oh!

jasonmadigan commented Jun 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

SarthakB11 commented May 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

david-martin commented May 14, 2026

Uh oh!

SarthakB11 commented May 14, 2026

Uh oh!

david-martin commented May 20, 2026

Uh oh!

jasonmadigan commented Jun 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SarthakB11 commented May 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 10, 2026 •

edited

Loading