fix(mcp-router): make ExtProcServer.RoutingConfig swap race-free by SarthakB11 · Pull Request #925 · Kuadrant/mcp-gateway

SarthakB11 · 2026-05-10T10:55:36Z

Summary

Same lock-discipline class as the recent broker race fixes (#860, #872, #888, #918, #921, #911), this one on the router side.

ExtProcServer.OnConfigChange in internal/mcp-router/server.go mutates s.RoutingConfig with a single bare assignment. It is registered as a config.Observer and fires from viper's fsnotify-driven goroutine, while Process is invoked once per concurrent ext_proc gRPC stream and the handlers it dispatches read s.RoutingConfig from at least five sites in request_handlers.go (MCPGatewayExternalHostname, GetServerConfigByName, MCPGatewayInternalHostname, RouterAPIKey). The pointer write is not atomic in Go, the race detector fires, and a config reload that lands during a tools/call can leave a goroutine reading a stale MCPServersConfig under the new pointer. In practice that surfaces as wrong-server routing, or 401 if RouterAPIKey rotated.

Convert ExtProcServer.RoutingConfig to atomic.Pointer[config.MCPServersConfig]. OnConfigChange becomes Store, readers become Load. initializeMCPSeverSession reads both MCPGatewayInternalHostname and RouterAPIKey for the same logical operation, so it captures the snapshot once via routingConfig := s.RoutingConfig.Load(). That keeps the host and the API key from the same configuration generation rather than re-Loading twice and risking a torn read across a reload.

setUpRouter seeds the initial pointer so Load() is non-nil before the first OnConfigChange fires. It also panics with a clear message if mcpConfig is unexpectedly nil at construction time, which is a new failure mode but a loud one (better than a deferred nil deref deep in a hot path).

Test fixtures in server_test.go and request_handlers_test.go updated to use the atomic.Pointer accessor.

Verification

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

Summary by CodeRabbit

Bug Fixes
- Improved routing configuration handling to be race-free, preventing requests from using nil or partially-updated routing state.
- Request and response flows now consistently use the active routing configuration for :authority rewriting and backend target/server resolution during token elicitation.
Tests
- Updated routing-config initialization in tests to apply configuration after server creation.
- Added a race-focused concurrency test to verify safe routing config updates while request handling runs concurrently.

coderabbitai · 2026-05-10T10:55:43Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

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: 2383498c-df00-4827-9122-493bbf1b3d81

📥 Commits

Reviewing files that changed from the base of the PR and between 58297fd and 9d18344.

📒 Files selected for processing (1)

internal/mcp-router/server_test.go

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

internal/mcp-router/server_test.go

📝 Walkthrough

Walkthrough

This PR changes ExtProcServer.RoutingConfig to an atomic pointer, updates readers and writers to use Load() and Store(), seeds router config after construction, and adjusts tests for the new initialization and concurrent access pattern.

Changes

Atomic RoutingConfig Concurrency Fix

Layer / File(s)	Summary
Core atomic pointer field and store semantics `internal/mcp-router/server.go`	`ExtProcServer.RoutingConfig` changes to `atomic.Pointer[config.MCPServersConfig]`, and `OnConfigChange` now publishes updates with `Store()`.
Router initialization with seeded config `cmd/mcp-broker-router/router.go`	`createRouter` removes inline `RoutingConfig` initialization, checks for nil config, and seeds the router with `Store(a.mcpConfig)` after construction.
Request and response handler Load() patterns `internal/mcp-router/request_handlers.go`, `internal/mcp-router/response_handlers.go`	Request and response paths read routing data via `Load()`, and session initialization snapshots the config once before backend lookup and client init.
Test construction pattern updates `internal/mcp-router/request_handlers_test.go`, `internal/mcp-router/response_handlers_test.go`, `internal/mcp-router/server_test.go`	Affected tests switch from struct-literal routing config setup to post-construction `Store()` calls.
Concurrent config swap race test `internal/mcp-router/server_test.go`	A new race-focused test runs `HandleRequestHeaders` and `OnConfigChange` concurrently against the shared routing config.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Data race on RoutingConfig in ext_proc server #1081 — Addresses the same concurrent access race on RoutingConfig by converting to atomic pointer semantics.

Possibly related PRs

Kuadrant/mcp-gateway#976 — Both PRs touch initializeMCPServerSession; this one snapshots routing config while that one adds singleflight session control.
Kuadrant/mcp-gateway#1116 — Both PRs modify HandleRequestHeaders header rewriting behavior.
Kuadrant/mcp-gateway#1156 — Both PRs alter internal routing/session initialization paths in internal/mcp-router.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 21.43% 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 clearly matches the main change: making ExtProcServer.RoutingConfig updates race-free in mcp-router.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.}

SarthakB11 · 2026-05-10T18:56:25Z

@coderabbitai review

coderabbitai · 2026-05-10T18:56:31Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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

Rebased onto main against the SSRF/backend-init JWT changes that landed in this file. Reader sites are down to four now (one path moved off RouterAPIKey via GenerateBackendInitToken), all converted to Load(). Test fixtures updated to use the atomic.Pointer accessor. Build + race + lint clean locally.

david-martin · 2026-05-20T10:03:42Z

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.

Patryk-Stefanski

Good race fix. atomic.Pointer is the right tool here — channels would add latency on the hot path, and this is a classic read-heavy config-swap pattern.

Blocker: missed RoutingConfig access site

response_handlers.go:62 still reads s.RoutingConfig directly:

serverInfo, cfgErr := s.RoutingConfig.GetServerConfigByName(req.serverName)

This needs s.RoutingConfig.Load().GetServerConfigByName(...) too — same race window as the request handler sites you already fixed.

Nit: snapshot capture in initializeMCPSeverSession

The PR body mentions capturing the config snapshot once to avoid torn reads. Verify that both GetServerConfigByName (line ~675) and MCPGatewayInternalHostname (line ~756) use the same snapshot variable rather than calling .Load() twice.

Missing trailing newlines

Per project style, files must end with a newline.

The overall approach is solid — just needs that one missed site in response_handlers.go.

jasonmadigan

needs a rebase -- merge conflicts with main.

SarthakB11 · 2026-06-13T06:18:17Z

Thanks @Patryk-Stefanski + @jasonmadigan . Pushed 71b95b9.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/mcp-router/server_test.go`:
- Around line 797-832: The test currently uses time.Sleep(200ms) to try to
overlap goroutines but doesn't guarantee either goroutine executed; replace the
sleep with a start barrier (e.g., sync.WaitGroup or a start channel) to ensure
both goroutines begin at the same time and use a bounded loop count (fixed
number of iterations) in each goroutine instead of an infinite loop so they
actually perform concurrent calls to server.HandleRequestHeaders and
server.OnConfigChange (which exercise RoutingConfig.Load/Store) before closing
the stop signal; also remove the now-unused time import. Ensure you reference
the existing goroutine bodies that call
server.HandleRequestHeaders(context.Background(), nil) and
server.OnConfigChange(context.Background(), &config.MCPServersConfig{...}) and
change their loops to wait on the shared start barrier, run N iterations, then
return so wg.Wait() deterministically observes concurrent access.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e262f86e-d893-4db0-aaab-8c7f3da54320

📥 Commits

Reviewing files that changed from the base of the PR and between e99c0a6 and 71b95b9.

📒 Files selected for processing (7)

cmd/mcp-broker-router/router.go
internal/mcp-router/request_handlers.go
internal/mcp-router/request_handlers_test.go
internal/mcp-router/response_handlers.go
internal/mcp-router/response_handlers_test.go
internal/mcp-router/server.go
internal/mcp-router/server_test.go

SarthakB11 · 2026-06-16T21:22:22Z

Hey @Patryk-Stefanski, @jasonmadigan. Pushed 71b95b9 Friday with the CodeRabbit fixes: replaced time.Sleep(200ms) with a start channel + sync.WaitGroup + bounded loops, dropped the unused time import. CI is all green now. Could you take another look when you have a moment?

jasonmadigan

rebased onto main, all conflicts resolved. tests pass, race detector clean.

atomic.Pointer is the right call for this read-heavy pattern. snapshot in initializeMCPServerSession prevents torn reads. race test is deterministic (channel barrier + 5000 iterations, no time.Sleep).

nits (author's call):

router.go constructor comment is verbose for project style
server.go RoutingConfig field comment is wordy
race test uses blank _ for *testing.T

OnConfigChange fires from the viper fsnotify goroutine and did a single bare assignment to s.RoutingConfig, while Process is invoked once per concurrent ext_proc gRPC stream and its handlers read RoutingConfig on every request. The pointer write is not atomic in Go and the race detector fires; a config reload landing during a request can leave a goroutine reading a stale MCPServersConfig under the new pointer. Convert ExtProcServer.RoutingConfig to atomic.Pointer[config.MCPServersConfig]. OnConfigChange becomes Store, and every reader site loads through .Load(). Seed the initial pointer in createRouter (cmd/mcp-broker-router/router.go) so Load is non-nil before the first OnConfigChange fires, and panic with a clear message if mcpConfig is unexpectedly nil at construction time. Address review on the rebased branch: - response_handlers.go: the 401 elicitation path was added to main after this PR was authored. Route it through s.RoutingConfig.Load() like the other handler sites so it builds against the atomic.Pointer field. - request_handlers.go: capture the routing config snapshot once at the top of initializeMCPServerSession and reuse the local for both GetServerConfigByName and MCPGatewayInternalHostname. Avoids torn reads if a Store lands between the two calls. Update test fixtures across server_test.go, request_handlers_test.go, and response_handlers_test.go to use the atomic.Pointer accessor. Add TestExtProcServer_OnConfigChange_DataRace exercising HandleRequestHeaders concurrently with OnConfigChange. Run with go test -race ./internal/mcp-router/... Rebased onto upstream/main e99c0a6. Same lock-discipline class as the recent merged concurrency fixes (Kuadrant#860, Kuadrant#872, Kuadrant#888, Kuadrant#911, Kuadrant#918, Kuadrant#921, Kuadrant#922). Signed-off-by: SarthakB11 <[email protected]>

Address CodeRabbit review on the new TestExtProcServer_OnConfigChange_DataRace. The original used time.Sleep(200ms) to overlap goroutines, which is probabilistic; one goroutine could exit early without exercising RoutingConfig.Load/Store at all. Replace with: - a start channel as a wait barrier so both goroutines begin simultaneously - bounded 5000-iteration loops in each goroutine for a known concurrent workload that the race detector can sample against - close(start) + wg.Wait() for deterministic termination Removes the now-unused time import. Verified: go test -race -run TestExtProcServer_OnConfigChange_DataRace ./internal/mcp-router/ runs in ~1.1s with the race detector clean. Signed-off-by: SarthakB11 <[email protected]>

jasonmadigan · 2026-06-23T10:03:18Z

thanks @SarthakB11

re-reviewed

SarthakB11 · 2026-06-23T18:37:35Z

Thanks for the rebase and the architecture sign-off, jasonmadigan. Happy to push the three nit cleanups if useful, otherwise good to merge as-is.

Address jasonmadigan review feedback: - router.go: shorten constructor seed-config comment - server.go: condense RoutingConfig field comment to one line - server_test.go: name *testing.T param t instead of _ Signed-off-by: SarthakB11 <[email protected]>

SarthakB11 · 2026-06-24T08:21:46Z

Pushed 58297fd with the 3 nit cleanups: router.go constructor comment trimmed, server.go RoutingConfig field comment shortened, race test uses named t instead of _. CI green locally (build + tests + race + vet).

The prior nit cleanup renamed _ *testing.T to t *testing.T but left the parameter unused, which golangci-lint (revive) flags. Capture HandleRequestHeaders errors in the reader goroutine and surface them via t.Errorf so the parameter is genuinely used and a real handler error during the race exercise no longer goes silent. Signed-off-by: SarthakB11 <[email protected]>

SarthakB11 · 2026-06-29T20:00:30Z

Pushed 9d18344. Fix for the revive unused-parameter failure my prior nit cleanup introduced: surface HandleRequestHeaders errors via t.Errorf instead of discarding them, so the test param is actually used and a real handler error during the race exercise no longer goes silent. Race detector still clean locally.

The conformance-tests failure on the same run looks unrelated to this branch (conformance-server connection refused during the readiness window, then becomes Ready 60+s later, but the conformance check has already timed out). Probably flaky infra rather than this PR; happy to debug further if it reproduces.

SarthakB11 force-pushed the sarth/routing-config-atomic branch from 28b5fbd to ed62bbe Compare May 14, 2026 11:34

Patryk-Stefanski previously requested changes Jun 12, 2026

View reviewed changes

jasonmadigan requested changes Jun 12, 2026

View reviewed changes

SarthakB11 force-pushed the sarth/routing-config-atomic branch from ed62bbe to 71b95b9 Compare June 13, 2026 06:16

coderabbitai Bot added review-effort/medium Medium review effort (3): few files, moderate logic high-risk Touches concurrency, auth, sessions, CRDs, ext_proc, or routing labels Jun 13, 2026

coderabbitai Bot reviewed Jun 13, 2026

View reviewed changes

Comment thread internal/mcp-router/server_test.go Resolved

SarthakB11 force-pushed the sarth/routing-config-atomic branch from 5db7433 to f558656 Compare June 13, 2026 08:44

david-martin added the triage/needs-issue PR needs a linked issue label Jun 22, 2026

jasonmadigan force-pushed the sarth/routing-config-atomic branch from f558656 to 985db05 Compare June 23, 2026 09:57

jasonmadigan mentioned this pull request Jun 23, 2026

fix: use atomic.Pointer for RoutingConfig to eliminate data race #1090

Closed

4 tasks done

jasonmadigan previously approved these changes Jun 23, 2026

View reviewed changes

SarthakB11 added 2 commits June 23, 2026 11:01

jasonmadigan dismissed their stale review via f22cbdd June 23, 2026 10:02

jasonmadigan force-pushed the sarth/routing-config-atomic branch from 985db05 to f22cbdd Compare June 23, 2026 10:02

jasonmadigan enabled auto-merge (squash) June 23, 2026 10:03

auto-merge was automatically disabled June 24, 2026 08:21
Head branch was pushed to by a user without write access

Patryk-Stefanski approved these changes Jun 30, 2026

View reviewed changes

Patryk-Stefanski merged commit f50087d into Kuadrant:main Jun 30, 2026
12 checks passed

coderabbitai Bot mentioned this pull request Jul 13, 2026

Isolated router #1269

Merged

dependabot Bot mentioned this pull request Sep 28, 2026

deps(deps): Bump github.com/onsi/gomega from 1.43.1 to 1.44.0 #1564

Merged

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!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

SarthakB11 commented May 10, 2026

Uh oh!

coderabbitai Bot commented May 10, 2026

Uh oh!

SarthakB11 commented May 14, 2026

Uh oh!

david-martin commented May 20, 2026

Uh oh!

Patryk-Stefanski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmadigan left a comment

Choose a reason for hiding this comment

Uh oh!

SarthakB11 commented Jun 13, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SarthakB11 commented Jun 16, 2026

Uh oh!

jasonmadigan left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmadigan commented Jun 23, 2026

Uh oh!

SarthakB11 commented Jun 23, 2026

Uh oh!

SarthakB11 commented Jun 24, 2026

Uh oh!

SarthakB11 commented Jun 29, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SarthakB11 commented May 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 10, 2026 •

edited

Loading