fix(mcp): optimistic-lock concurrent MCP server PATCHes

[thesaadmirza]

Follow-up to #13976. In review, @erichare flagged one narrow lost-update case left in the DB-backed store: when two PATCHes hit the same server at once, both read the same row, each merges its change onto that snapshot, and the last commit wins, so the other PATCH's field is dropped.

update_server now guards a merge PATCH with the row version: UPDATE ... WHERE id = ? AND version = ?. If the row moved under us the update matches nothing, so we re-read the current row, merge onto that, and retry. Both PATCHes' fields survive. A full replace still writes unconditionally, since there the caller is sending the whole config and last-writer-wins is the expected behavior.

No schema change. The version column already shipped with #13976; this just starts using it.

Changes

• api/v2/mcp.py: version-guarded retry loop for merge PATCHes in update_server, unconditional write for full replaces, and a bounded retry budget (returns 409 if it can't settle, which only same-server concurrent PATCHes can trigger).
• tests/unit/api/v2/test_mcp_db_store.py: one test fires many concurrent PATCHes that each set a different field and checks they all survive; another runs a PATCH against a concurrent DELETE and checks it fails cleanly instead of raising a raw DB error.

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when multiple updates occur simultaneously.
  • Prevented database errors during concurrent updates and deletions.
  • Ensured merged server configurations retain all successfully applied changes.
  • Improved recovery when records are created, updated, or removed concurrently.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f5a3384c-c60e-4aba-ae9b-4a79658c7593

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

update_server now uses bounded optimistic-concurrency retries for MCP server persistence, including merge conflicts, create races, and concurrent deletes. New tests cover concurrent field-preserving merges and PATCH/delete races.

Changes

MCP server concurrency handling

Layer / File(s)	Summary
Version-guarded update retries src/backend/base/langflow/api/v2/mcp.py	update_server adds bounded retries, version-checked merge updates, recovery from create and delete races, cache clearing, and direct decrypted result reloads.
Concurrency regression coverage src/backend/tests/unit/api/v2/test_mcp_db_store.py	Tests verify concurrent merge patches preserve all fields and PATCH/delete races do not expose raw database errors.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• langflow-ai/langflow#13976: Both changes modify update_server database merge and PATCH concurrency behavior.

Suggested labels: bug, lgtm

Suggested reviewers: Cristhianzl, erichare

🚥 Pre-merge checks | ✅ 8 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (8 passed)

Check name	Status	Explanation
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.
Test Coverage For New Implementations	✅ Passed	Added backend unit tests in test_mcp_db_store.py for concurrent merge PATCHes and PATCH-vs-DELETE races, matching the new update_server logic and naming conventions.
Test Quality And Coverage	✅ Passed	Async pytest tests cover the new merge-PATCH concurrency paths and PATCH/DELETE race with real DB sessions, asserting final state and clean error handling.
Test File Naming And Structure	✅ Passed	New tests live in test_*.py under unit/api/v2, use pytest/asyncio correctly, have descriptive names, and include setup/cleanup plus positive and negative cases.
Excessive Mock Usage Warning	✅ Passed	Mocks are limited to external cache helpers; core MCP store behavior is exercised with real SQLite sessions and real update/get functions.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: optimistic locking for concurrent MCP server PATCHes.

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/backend/tests/unit/api/v2/test_mcp_db_store.py (1)

334-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

@pytest.mark.asyncio decorators are redundant.

This repo configures pytest-asyncio with asyncio_mode = "auto", so async tests are auto-detected and these decorators can be dropped. If the surrounding tests in this file already carry the decorator, keep them consistent (remove file-wide in a follow-up rather than partially).

Based on learnings: "pytest-asyncio is configured with asyncio_mode = 'auto' ... you do not need to decorate test functions or classes with pytest.mark.asyncio."

Also applies to: 364-364

🤖 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/backend/tests/unit/api/v2/test_mcp_db_store.py` at line 334, Remove the
redundant `@pytest.mark.asyncio` decorators from the affected tests in this file,
including both referenced locations, while preserving consistency with
surrounding tests; if decorators are widespread, leave them unchanged for a
separate file-wide cleanup.

Source: Learnings

src/backend/base/langflow/api/v2/mcp.py (1)

426-484: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Retry budget scales with concurrent same-server writers.

The version guard makes each conflicting writer retry roughly once per competing writer on the same (user_id, name). With _MAX_UPSERT_RETRIES = 12, more than ~12 concurrent merge PATCHes to the same server can exhaust the budget and surface a spurious 409 even though no genuine deadlock exists. For the current test load (8 writers) this is safe. If this endpoint can see higher same-key fan-out, consider a larger/adaptive budget with jittered backoff, or documenting the practical concurrency ceiling.

🤖 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/backend/base/langflow/api/v2/mcp.py` around lines 426 - 484, Update the
retry handling around the upsert loop in the MCP server persistence logic to
avoid spurious 409 responses under higher same-key concurrency. Increase or
adapt the budget beyond the fixed _MAX_UPSERT_RETRIES value, add jittered
backoff between version-conflict retries, and document the practical concurrency
limit if the budget remains bounded; preserve the existing behavior for create
races and successful updates.

🤖 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 `@src/backend/base/langflow/api/v2/mcp.py`:
- Around line 466-473: Replace the 500 status with 404 or 409 when the merge
PATCH re-read in the concurrent-delete path finds no MCPServer, updating the
HTTPException in the MCP server merge logic. Also align the delete early-path
and check_existing handling to use appropriate 404/409 statuses instead of 500
for not-found or already-existing resources.

---

Nitpick comments:
In `@src/backend/base/langflow/api/v2/mcp.py`:
- Around line 426-484: Update the retry handling around the upsert loop in the
MCP server persistence logic to avoid spurious 409 responses under higher
same-key concurrency. Increase or adapt the budget beyond the fixed
_MAX_UPSERT_RETRIES value, add jittered backoff between version-conflict
retries, and document the practical concurrency limit if the budget remains
bounded; preserve the existing behavior for create races and successful updates.

In `@src/backend/tests/unit/api/v2/test_mcp_db_store.py`:
- Line 334: Remove the redundant `@pytest.mark.asyncio` decorators from the
affected tests in this file, including both referenced locations, while
preserving consistency with surrounding tests; if decorators are widespread,
leave them unchanged for a separate file-wide cleanup.

🪄 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: c86725af-dd3b-45a6-8032-25724ae2952f

📥 Commits

Reviewing files that changed from the base of the PR and between 7ffd040 and ba43f4f.

📒 Files selected for processing (2)

• src/backend/base/langflow/api/v2/mcp.py
• src/backend/tests/unit/api/v2/test_mcp_db_store.py

[erichare]

Thanks so much @thesaadmirza . some small changes

Requested changes

• [P1] Keep versions monotonic across full replacements — mcp.py:476

  The full-replace path increments from a potentially stale ORM value, allowing a version to be reused. A later guarded PATCH can then overwrite the newer replacement using stale config. Use a DB-side increment such as version=MCPServer.version + 1 and add a mixed replace/PATCH regression test.

• [P2] Re-raise non-duplicate integrity failures — mcp.py:439

  Every IntegrityError is treated as a duplicate-create race. If no winning row exists, it retries twelve times and returns a misleading concurrency 409. After refetching, re-raise when existing is None; only retry when a conflicting row exists.

• [P2] Make the PATCH/delete test deterministic — test_mcp_db_store.py:385

  The probabilistic loop may never exercise the intended race, and the assertion accepts any HTTPException, including 500. Use a synchronization barrier and assert that DELETE succeeds and PATCH returns exactly 404.

[thesaadmirza]

Thanks @erichare, these all made sense. Fixed in 783c326fba:

P1 (version reuse on full replace). The replace now bumps the version in the UPDATE itself: .values(version=MCPServer.version + 1, ...). Still last-writer-wins on the config, but the version always comes from the current DB row, so a replace can't reuse a number a concurrent PATCH already took.

P2 (non-duplicate integrity errors). After the refetch it re-raises when there's no winning row, so only the real duplicate-name race retries. A genuine IntegrityError now surfaces instead of looping into a 409.

P2 (delete test). Dropped the probabilistic loop. It now deletes the row right between the PATCH's read and its guarded write, then asserts the PATCH returns 404 and the row is gone.

[erichare]

Hi @thesaadmirza , do you mind rebasing this onto release-1.12.0? ill then do a final review and merge it in! Thank you!

[thesaadmirza]

Hi @thesaadmirza , do you mind rebasing this onto release-1.12.0? ill then do a final review and merge it in! Thank you!

Done, thanks @erichare! Updated it onto release-1.12.0 and it's mergeable with no conflicts. Ready for your final review whenever you have a moment.

[erichare]

Thanks so much @thesaadmirza , appreciate the contribution!

[erichare]

LGTM Thanks @thesaadmirza