Skip to content

Commit 98d02eb

Browse files
committed
fix(upstream): make managed.Client config access atomic (MCP-770)
The first MCP-770 fix routed Manager-side reads (DiscoverTools, GetStats, GetTotalToolCount, ListServers) through GetConfig(), but the underlying hazard is broader: managed.Client.Config is a pointer field that SetConfig swaps off m.mu (reconcile add path), while it is read concurrently from many sites that do NOT hold the client's mc.mu — including Client.Connect's unlocked phase (client.go:197) and detached state-change callback goroutines (onStateChange, spawned by StateManager.SetError). The CI -race detector flagged the Connect-vs-SetConfig variant on PR #555's macOS unit job after the first fix was cherry-picked. A mutex-guarded accessor can't cover this: most internal readers run while mc.mu is already held, so routing them through an RLock GetConfig() would deadlock (sync.RWMutex is not reentrant). Instead make the field itself lock-free and data-race-free: - Replace the exported `Config *config.ServerConfig` field with an unexported `cfg atomic.Pointer[config.ServerConfig]`. - GetConfig() returns cfg.Load(); SetConfig() does cfg.Store(). Both are lock-free and safe whether or not mc.mu is held. - Route every reader through GetConfig() across the managed client, the upstream Manager, and the supervisor actor pool. Add TestConnect_ConfigRace: drives AddServerConfig (SetConfig) against Client.Connect concurrently; trips -race on the old field, green after. Verified: go test -race ./internal/upstream/... ./internal/runtime/... and the original CI victims TestHandleUpstreamServers_AddFromRegistry_HappyPath / _AdminAllowed_WriteOps pass x5 under -race; build + linter clean. Related: MCP-770
1 parent 59efb9c commit 98d02eb

8 files changed

Lines changed: 192 additions & 133 deletions

File tree

internal/runtime/supervisor/actor_pool.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -211,12 +211,12 @@ func (p *ActorPoolSimple) GetAllStates() map[string]*ServerState {
211211

212212
state := &ServerState{
213213
Name: name,
214-
Config: client.Config,
215-
Enabled: client.Config.Enabled,
214+
Config: client.GetConfig(),
215+
Enabled: client.GetConfig().Enabled,
216216
Connected: connected,
217217
}
218218

219-
if client.Config.Quarantined {
219+
if client.GetConfig().Quarantined {
220220
state.Quarantined = true
221221
}
222222

internal/runtime/supervisor/actor_pool_complex_reference.go

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@ import (
1616
// ActorPool manages the lifecycle of server actors and provides stats for Supervisor.
1717
// This replaces UpstreamAdapter with direct Actor integration (Phase 7.2).
1818
type ActorPool struct {
19-
actors map[string]*actor.Actor
20-
mu sync.RWMutex
21-
logger *zap.Logger
22-
manager *upstream.Manager // Use existing manager for client creation
19+
actors map[string]*actor.Actor
20+
mu sync.RWMutex
21+
logger *zap.Logger
22+
manager *upstream.Manager // Use existing manager for client creation
2323

2424
// Event aggregation
2525
eventCh chan Event
@@ -218,12 +218,12 @@ func (p *ActorPool) GetServerState(name string) (*ServerState, error) {
218218

219219
state := &ServerState{
220220
Name: name,
221-
Config: client.Config,
222-
Enabled: client.Config.Enabled,
221+
Config: client.GetConfig(),
222+
Enabled: client.GetConfig().Enabled,
223223
Connected: client.IsConnected(),
224224
}
225225

226-
if client.Config.Quarantined {
226+
if client.GetConfig().Quarantined {
227227
state.Quarantined = true
228228
}
229229

@@ -258,12 +258,12 @@ func (p *ActorPool) GetAllStates() map[string]*ServerState {
258258
connected := client.IsConnected()
259259
state := &ServerState{
260260
Name: name,
261-
Config: client.Config,
262-
Enabled: client.Config.Enabled,
261+
Config: client.GetConfig(),
262+
Enabled: client.GetConfig().Enabled,
263263
Connected: connected,
264264
}
265265

266-
if client.Config.Quarantined {
266+
if client.GetConfig().Quarantined {
267267
state.Quarantined = true
268268
}
269269

@@ -328,9 +328,9 @@ func (p *ActorPool) forwardActorEvents(name string, a *actor.Actor) {
328328
ServerName: name,
329329
Timestamp: event.Timestamp,
330330
Payload: map[string]interface{}{
331-
"connected": event.State == actor.StateConnected,
332-
"state": string(event.State),
333-
"actor_event": string(event.Type),
331+
"connected": event.State == actor.StateConnected,
332+
"state": string(event.State),
333+
"actor_event": string(event.Type),
334334
},
335335
})
336336
}

internal/upstream/client_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ func TestClient_Headers_Support(t *testing.T) {
309309
require.NotNil(t, client)
310310

311311
// Test that headers are stored in config
312-
assert.Equal(t, tt.headers, client.Config.Headers)
312+
assert.Equal(t, tt.headers, client.GetConfig().Headers)
313313

314314
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
315315
defer cancel()

0 commit comments

Comments
 (0)