fix: defer SetClientTools until after DB persistence to prevent runtime/DB state drift - #5972
Merged
Conversation
|
|
This was referenced Aug 8, 2026
Merged
Collaborator
Author
This was referenced Aug 8, 2026
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughHeader and token-exchange verification now activate MCP clients before tool persistence. Runtime tools are applied only after database persistence succeeds. Persistence errors report aligned state and support clean retries. ChangesMCP tool synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 8, 2026
Collaborator
Author
Merge activity
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 `@transports/bifrost-http/handlers/mcp.go`:
- Around line 496-516: Reconcile persisted tools after UpdateMCPClientConfig
errors in both verification paths at
transports/bifrost-http/handlers/mcp.go:496-516 and
transports/bifrost-http/handlers/mcp.go:670-690: reload the client, and when
persisted tools and DiscoveredToolNameMapping exist, pass them to SetClientTools
before completing the error flow. In the header replay-success branch, hydrate
runtime tools before returning. Do not rely on UpdateClient to copy discovered
tools.
🪄 Autofix
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 Plus
Run ID: 0fd298bc-d2ba-48ff-9090-bf6b55c51585
📒 Files selected for processing (1)
transports/bifrost-http/handlers/mcp.go
Comment on lines
496
to
+516
| if err := h.store.ConfigStore.UpdateMCPClientConfig(ctx, clientConfig.ID, updateReq); err != nil { | ||
| // NOTE: Partial success; the runtime client is connected and serving | ||
| // but the DB row still has no discovered tools, so runtime and | ||
| // database state have drifted. The replay guard reads DiscoveredTools | ||
| // from the DB, so retrying this endpoint re-runs verification and | ||
| // reconverges the two; a restart re-parks the client in | ||
| // pending_verification. | ||
| // NOTE: Partial success; the runtime client is activated (Healthy) | ||
| // but — since SetClientTools runs below, after this succeeds — has | ||
| // no tools live either, so runtime and DB agree (both empty) rather | ||
| // than drifting. The replay guard reads DiscoveredTools from the | ||
| // DB, so retrying this endpoint re-runs verification cleanly; a | ||
| // restart re-parks the client in pending_verification. | ||
| logger.Error(fmt.Sprintf( | ||
| "[PARTIAL SUCCESS] MCP client %s was activated after headers verification but persisting discovered tools failed: %v. "+ | ||
| "Runtime and database state have drifted: the client is connected now but will return to pending_verification on restart. "+ | ||
| "Retry this endpoint to re-verify and persist.", | ||
| "The client is connected but has no tools live yet; retry this endpoint to re-run verification and persist.", | ||
| clientConfig.ID, err, | ||
| )) | ||
| SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Client activated but discovered tools could not be persisted, so runtime and database state have drifted until re-verified. Retry this endpoint to re-run verification and persist: %v", err)) | ||
| SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Client activated but discovered tools could not be persisted. Retry this endpoint to re-run verification and persist: %v", err)) | ||
| return | ||
| } | ||
| // Set discovered tools on the client after persistence succeeds — not | ||
| // before. SetClientTools may propagate this update to other observers | ||
| // of the DB row (deployment-specific), so it must run strictly after | ||
| // the write above lands — otherwise a propagated update could be read | ||
| // back before the row it depends on was actually written. | ||
| h.mcpManager.SetClientTools(clientConfig.ID, tools, toolNameMapping) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -i 'mcp|config.*store|store' . | head -200
printf '%s\n' '--- relevant symbols ---'
rg -n --glob '*.go' 'UpdateMCPClientConfig|SetClientTools|DiscoveredTools|pending_verification|replay' .
printf '%s\n' '--- handler outline ---'
ast-grep outline transports/bifrost-http/handlers/mcp.goRepository: maximhq/bifrost
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- handler sections ---'
sed -n '400,560p' transports/bifrost-http/handlers/mcp.go
sed -n '600,730p' transports/bifrost-http/handlers/mcp.go
printf '%s\n' '--- persistence implementations and interfaces ---'
rg -n -C 8 --glob '*.go' 'func .*UpdateMCPClientConfig|UpdateMCPClientConfig\(' .
printf '%s\n' '--- runtime tool implementations ---'
rg -n -C 10 --glob '*.go' 'func .*SetClientTools|SetClientTools\(' .
printf '%s\n' '--- replay branches ---'
rg -n -C 15 --glob '*.go' 'DiscoveredTools != nil|DiscoveredTools == nil|DiscoveredTools' transports/bifrost-http/handlersRepository: maximhq/bifrost
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '400,560p' transports/bifrost-http/handlers/mcp.go
sed -n '600,730p' transports/bifrost-http/handlers/mcp.go
rg -n -C 8 --glob '*.go' 'func .*UpdateMCPClientConfig|UpdateMCPClientConfig\(' .
rg -n -C 10 --glob '*.go' 'func .*SetClientTools|SetClientTools\(' .
rg -n -C 15 --glob '*.go' 'DiscoveredTools != nil|DiscoveredTools == nil|DiscoveredTools' transports/bifrost-http/handlersRepository: maximhq/bifrost
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- UpdateMCPClientConfig implementation ---'
sed -n '2241,2355p' framework/configstore/rdb.go
printf '%s\n' '--- MCP client handler entry and replay guards ---'
sed -n '250,430p' transports/bifrost-http/handlers/mcp.go
printf '%s\n' '--- runtime activation/update helper references ---'
rg -n -C 12 --glob '*.go' 'func .*updateMCPClientWithRetry|updateMCPClientWithRetry|func .*UpdateClient|func .*AddClient' transports/bifrost-http/handlers/mcp.go core/mcp/clientmanager.go
printf '%s\n' '--- SetClientTools implementation ---'
sed -n '1020,1095p' core/mcp/clientmanager.go
printf '%s\n' '--- focused tests about persistence failures/retries ---'
rg -n -C 12 --glob '*_test.go' 'persist|persistence|UpdateMCPClientConfig|verify-headers|verify-exchange|SetClientTools|pending_verification' transports/bifrost-http/handlers framework/configstore core/mcp | head -500Repository: maximhq/bifrost
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '2241,2355p' framework/configstore/rdb.go
sed -n '250,430p' transports/bifrost-http/handlers/mcp.go
rg -n -C 12 --glob '*.go' 'func .*updateMCPClientWithRetry|updateMCPClientWithRetry|func .*UpdateClient|func .*AddClient' transports/bifrost-http/handlers/mcp.go core/mcp/clientmanager.go
sed -n '1020,1095p' core/mcp/clientmanager.go
rg -n -C 12 --glob '*_test.go' 'persist|persistence|UpdateMCPClientConfig|verify-headers|verify-exchange|SetClientTools|pending_verification' transports/bifrost-http/handlers framework/configstore core/mcp | head -500Repository: maximhq/bifrost
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remainder of database transaction ---'
sed -n '2350,2415p' framework/configstore/rdb.go
printf '%s\n' '--- UpdateClient complete body ---'
sed -n '1329,1465p' core/mcp/clientmanager.go
printf '%s\n' '--- AddClient tool/state initialization ---'
sed -n '461,690p' core/mcp/clientmanager.go
printf '%s\n' '--- exact handler replay and activation lines ---'
nl -ba transports/bifrost-http/handlers/mcp.go | sed -n '425,525p'
nl -ba transports/bifrost-http/handlers/mcp.go | sed -n '615,705p'
printf '%s\n' '--- focused persistence tests ---'
rg -n --glob '*_test.go' 'UpdateMCPClientConfig|DiscoveredTools|verifyMCPClient|verify-headers|verify-exchange' framework/configstore/rdb_mcp_tools_test.go framework/configstore/rdb_mcp_admin_auth_mode_test.go transports/bifrost-http/handlers core/mcp | head -250Repository: maximhq/bifrost
Length of output: 22574
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '2350,2415p' framework/configstore/rdb.go
sed -n '1329,1465p' core/mcp/clientmanager.go
sed -n '461,690p' core/mcp/clientmanager.go
nl -ba transports/bifrost-http/handlers/mcp.go | sed -n '425,525p'
nl -ba transports/bifrost-http/handlers/mcp.go | sed -n '615,705p'
rg -n --glob '*_test.go' 'UpdateMCPClientConfig|DiscoveredTools|verifyMCPClient|verify-headers|verify-exchange' framework/configstore/rdb_mcp_tools_test.go framework/configstore/rdb_mcp_admin_auth_mode_test.go transports/bifrost-http/handlers core/mcp | head -250Repository: maximhq/bifrost
Length of output: 22367
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '2400,2475p' framework/configstore/rdb.go
sed -n '1400,1555p' core/mcp/clientmanager.go
sed -n '430,530p' transports/bifrost-http/handlers/mcp.go
sed -n '620,705p' transports/bifrost-http/handlers/mcp.go
python3 - <<'PY'
from pathlib import Path
handler = Path("transports/bifrost-http/handlers/mcp.go").read_text()
manager = Path("core/mcp/clientmanager.go").read_text()
rdb = Path("framework/configstore/rdb.go").read_text()
checks = {
"update_client_does_not_copy_discovered_tools": "DiscoveredTools:" not in manager[manager.index("func (m *MCPManager) UpdateClient"):manager.index("func (m *MCPManager) UpdateClientCredentials")],
"set_tools_sets_healthy": "client.State = schemas.MCPConnectionStateHealthy" in manager[manager.index("func (m *MCPManager) SetClientTools"):manager.index("func (m *MCPManager) RemoveClient")],
"rdb_transaction": "return s.DB().Transaction(func(tx *gorm.DB) error" in rdb[rdb.index("func (s *RDBConfigStore) UpdateMCPClientConfig"):],
}
for name, value in checks.items():
print(f"{name}={value}")
PYRepository: maximhq/bifrost
Length of output: 21646
Reconcile the database result after UpdateMCPClientConfig errors.
A returned transaction error does not prove that the write rolled back. Reload the client and call SetClientTools with persisted tools and DiscoveredToolNameMapping when they exist. Apply this to both verification paths. In the header replay-success branch, hydrate the runtime tools before returning.
UpdateClient does not copy discovered tools, so the runtime can remain without tools while the database contains them.
📍 Affects 1 file
transports/bifrost-http/handlers/mcp.go#L496-L516(this comment)transports/bifrost-http/handlers/mcp.go#L670-L690
🤖 Prompt for 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.
In `@transports/bifrost-http/handlers/mcp.go` around lines 496 - 516, Reconcile
persisted tools after UpdateMCPClientConfig errors in both verification paths at
transports/bifrost-http/handlers/mcp.go:496-516 and
transports/bifrost-http/handlers/mcp.go:670-690: reload the client, and when
persisted tools and DiscoveredToolNameMapping exist, pass them to SetClientTools
before completing the error flow. In the header replay-success branch, hydrate
runtime tools before returning. Do not rely on UpdateClient to copy discovered
tools.
Source: Path instructions
Pratham-Mishra04
changed the base branch from
08-07-feat_persist_and_resync_mcp_tool_discoveries_uniformly_across_all_client_types_via_a_hash-gated_core_callback
to
graphite-base/5972
August 8, 2026 11:39
…n verify-headers/verify-exchange to close a stale-read race
Pratham-Mishra04
force-pushed
the
08-08-fix_persist_discovered_tools_before_triggering_cluster_propagation_in_verify-headers_verify-exchange_to_close_a_stale-read_race
branch
from
August 8, 2026 11:42
cbcaf6b to
1341ebc
Compare
Pratham-Mishra04
deleted the
08-08-fix_persist_discovered_tools_before_triggering_cluster_propagation_in_verify-headers_verify-exchange_to_close_a_stale-read_race
branch
August 8, 2026 11:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SetClientToolswas being called before the discovered tools were persisted to the database. This created a window where the runtime had tools live but the DB row did not, causing state drift between the two. If persistence then failed, the runtime would be serving tools that the DB had no record of, making retries and restarts inconsistent.Changes
SetClientToolsto run strictly afterUpdateMCPClientConfigsucceeds in bothverifyMCPClientHeadersandverifyMCPClientExchangeSetClientToolsmay propagate updates to other observers of the DB row; it must run after the write lands to avoid a propagated read racing ahead of the writeType of change
Affected areas
How to test
Trigger MCP client verification via both the headers and exchange endpoints. Simulate a DB persistence failure after activation and confirm that the runtime client has no tools live (matching the empty DB row) rather than serving tools that were never persisted. Confirm that retrying the endpoint re-runs verification and correctly persists and activates tools.
go test ./...Breaking changes
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines