fix(mcp): tolerate concurrent registry migrations#4234
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesIdempotent mcp_servers column migration
README link target updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/mcp_registry/store.rs (1)
136-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd logging to the genuine-failure branch.
The race/idempotent paths log via
trace!/debug!/warn!, but the real-error branch at Line 143 (Ok(_) => Err(err).context(context)— ALTER failed and the column is confirmed still absent) propagates the error silently. This is the path that surfaces an actual migration failure to the UI, so it's the most useful one to log. As per coding guidelines: "Add debug logging to entry/exit, branches, external calls, retries/timeouts, state transitions, and errors usinglog/tracing".♻️ Proposed logging on the genuine-error branch
- Ok(_) => Err(err).context(context), + Ok(_) => { + tracing::debug!( + "[mcp-registry] schema add column failed and still absent column={column} error={err}" + ); + Err(err).context(context) + }🤖 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 `@src/openhuman/mcp_registry/store.rs` around lines 136 - 150, The genuine-failure path in the mcp registry schema-add logic is missing an error log, so add a tracing log in the Ok(_) => Err(err).context(context) branch inside the mcp_servers_columns match in store.rs. Use the existing column/context variables and include the original err details before propagating so the failed ALTER COLUMN case is visible in logs, while keeping the existing debug/warn behavior for the idempotent and refresh-failure branches.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/openhuman/mcp_registry/store.rs`:
- Around line 136-150: The genuine-failure path in the mcp registry schema-add
logic is missing an error log, so add a tracing log in the Ok(_) =>
Err(err).context(context) branch inside the mcp_servers_columns match in
store.rs. Use the existing column/context variables and include the original err
details before propagating so the failed ALTER COLUMN case is visible in logs,
while keeping the existing debug/warn behavior for the idempotent and
refresh-failure branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3ff0d69-bb92-48a6-875c-9cf0a0f4acf7
📒 Files selected for processing (1)
src/openhuman/mcp_registry/store.rs
|
CodeRabbit flagged the localized README link cleanup as potentially out of scope. I kept it in this PR because the required Markdown Link Check scans these docs on this PR and was failing on pre-existing stale links, which left the MCP migration fix red. The docs changes are limited to the exact five files and link categories reported by lychee: the old native voice docs URL, Reddit's 403 response, and the Star History fragment URL. |
1 participant
Summary
mcp_serverscolumns afterALTER TABLE ADD COLUMNfailures and treats the migration as successful when the desired column is now present.PRAGMA table_infosnapshots hitting an already-addeddeployment_urlcolumn.Problem
Failed to add deployment_url column to mcp_serverson load.ALTER TABLE ADD COLUMN deployment_urlafter another connection has already added it.ALTER TABLE ADD COLUMN IF NOT EXISTS, so the duplicate-column path needs explicit handling.Solution
mcp_serverscolumn addition throughadd_mcp_servers_column_if_missing.PRAGMA table_infosnapshot.ALTER TABLEfailure, refreshes the column list; if the target column exists, logs the observed concurrent/stale-snapshot path and continues.Submission Checklist
## Related— N/A: no coverage-matrix feature ID applies to this persistence migration fix.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release manual smoke surface changed.Closes #NNNin the## RelatedsectionImpact
transport,deployment_url, andenabledadded as before.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-4194-mcp-migration-race8fd5768fc,58440321d,82c235419Validation Run
pnpm --filter openhuman-app format:check— N/A: frontend workspace not changed.pnpm typecheck— N/A: TypeScript/frontend not changed.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib additive_column_migration_tolerates_duplicate_column_from_stale_snapshotGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib mcp_registry::store::testsGGML_NATIVE=OFF cargo check --manifest-path Cargo.tomlcargo fmt --manifest-path Cargo.toml --checkgit diff --checkrg -n "tinyhumans\\.gitbook\\.io/openhuman/features/voice|reddit\\.com/r/tinyhumansai|star-history\\.com/#tinyhumansai/openhuman" docs/README.*.mdreturned no matches.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
deployment_urlmigration failure when the column already exists.Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
mcp_serversby making column additions safely idempotent during concurrent/duplicate migration attempts.deployment_urlis added correctly.