Skip to content

fix(mcp): make paired settings/credential writes atomic - #259

Merged
Zongwei9888 merged 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:pr/mcp-pair-atomic
Oct 21, 2026
Merged

Zongwei9888 merged 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:pr/mcp-pair-atomic

Conversation

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor

Summary

An MCP upsert/remove is a paired write: the config settings entry and its OAuth credentials must land together or not at all. Today, if the credential half fails after the settings mutation, the config is left silently half-paired (settings entry updated/orphaned while credentials disagree).

This PR gives paired writes compensating-transaction semantics:

  • If OAuth credential cleanup fails after the settings mutation, the settings half is rolled back to its exact previous entry (or dropped if the server was just created).
  • If the rollback itself also fails, raise loudly naming both residual states (C1 unverified semantics) — never leave a silent half-pair behind.

Changes

  • core/application/mcp_service.py: new _restore_server_entry() helper; upsert/remove wrap credential cleanup so a failure triggers a compensating settings rollback, and a rollback failure raises ApplicationError with details carrying both credential_error and rollback_error.
  • tests/application/test_management_services.py: 3 regression tests — rollback on cleanup failure (upsert + remove), happy-path pair lands together and cleans the credential, and rollback failure raises a loud error naming both residual states.

Tests

  • 3 new tests (including negative control: reverting the fix turns the 2 rollback tests red while the happy path stays green)
  • Targeted regression after rebase onto latest main (5cd55efb): tests/application/test_management_services.py tests/test_mcp_oauth.py tests/test_mcp_server.py tests/test_mcp_cli.py → 30 passed
  • ruff check + ruff format clean (pre-commit hooks pass)

Dependency note

No new dependencies. Only existing symbols (ApplicationError, ConfigStore.mutate, McpOAuthCredentialStore.delete_endpoint) are used.

Restore the settings half of an MCP server entry when OAuth credential
cleanup fails after the config mutation, instead of leaving a silent
half-paired write. If the rollback itself also fails, raise loudly with
both residual states (C1 unverified semantics).
@Zongwei9888
Zongwei9888 merged commit 73d7846 into HKUDS:main Oct 21, 2026
22 of 24 checks passed
@Zongwei9888

Copy link
Copy Markdown
Collaborator

Merged into main on 2026-10-21 as 73d7846 ("Merge PR #259: keep an MCP server's settings and credential in step"). Thank you, and sorry this waited a week for a reply after your rebase.

The rollback is a real improvement for remove: before, a failed credential delete left the config entry gone and the credential stranded, so a retry failed with "not defined". The three tests are clear and stable (15/15 repeated runs here).

One follow-up on main in e40ac1d: the _restore_server_entry docstring no longer mentions "C1 unverified semantics", since that label isn't defined anywhere in this repo. Two optional notes if you want to touch it again: the restore writes back a previous read outside the lock, so restoring only when the entry still equals what was just written would avoid overwriting a concurrent edit; and the previous is None branch can't be reached today.

The change is listed in the README news for 2026-10-21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants