Skip to content

fix(mcp): prevent lost updates during concurrent OAuth token saves - #300

Open
li-xiu-qi wants to merge 1 commit into
stepfun-ai:mainfrom
li-xiu-qi:fix/mcp-oauth-concurrent-token-saves
Open

li-xiu-qi wants to merge 1 commit into
stepfun-ai:mainfrom
li-xiu-qi:fix/mcp-oauth-concurrent-token-saves

Conversation

@li-xiu-qi

@li-xiu-qi li-xiu-qi commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #146

Problem

When two Step processes simultaneously save OAuth tokens for different MCP servers, both read the full .credentials.json, modify their own keys, and overwrite the file. If both processes read the same stale snapshot before writing, the later writer overwrites the earlier writer's keys, causing entries that belonged to other servers to disappear from the file. The correct behavior requires that when one process saves its own key, all keys already written by other processes must also be preserved.

Changes

  • Merged two saveTokens call sites in packages/coding-agent/src/step/mcp-oauth.ts into a single saveCredentialTokens implementation.
  • Wrapped the read-modify-write cycle in a cross-process lock using the existing proper-lockfile dependency. Token saves acquire the lock asynchronously with a 30-second bounded wait and a 60-second stale window, so slow writes do not block the UI and a live lock cannot be stolen during that wait. Lock acquisition failure still fails closed without writing. The same lock covers logout, preventing deletion and save from overwriting each other.
  • Added randomUUID to the temporary file used when publishing .credentials.json to eliminate collision, and clean up that file on write or rename failure while preserving the original error.
  • Zero new dependencies introduced.

Behavior and compatibility

  • saveTokens now returns a Promise, which the MCP SDK already awaits. logoutMcpServer keeps its synchronous signature, while the JSON data format, configuration keys, and persisted credential shape are unchanged.
  • Writes are serialized; reads do not require holding the lock and continue reading atomic snapshots published via rename. No data migration is required.

Validation

  • New concurrency tests: packages/coding-agent/test/step-mcp-oauth.test.ts and packages/coding-agent/test/fixtures/mcp-oauth-save-tokens-child.ts. A deliberate file barrier forces two real operating system processes to read the identical stale snapshot before writing, verifying that tokens for both target servers survive the race.
  • Regression evidence: The new test reproduces the lost update on the unpatched main. With this PR applied, all 14 tests pass, including interactive-login locking, lock-acquisition failure, failed-publication cleanup, and a live-lock hold longer than the default 10-second stale window.
    • Tests run from packages/coding-agent using: NODE_ENV=development npx -y pnpm@9.15.9 exec vitest --run test/step-mcp-oauth.test.ts
  • Repository checks: CI=true NODE_ENV=development npx -y pnpm@9.15.9 run check ran from the repository root: passed.
  • Stress reproduction: Using a 2,090,073-byte synthetic credentials file on Windows (Node v24.21.0), the unpatched code lost one entry in 3/3 rounds. The patched version preserved both entries across 200/200 rounds, with sequential verification passing 3/3.
  • Full repository validation: build-check-test passed on commit ab638295536ab0a78a35b4d24e163f91fe1ef95b.

Limitations

None.

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.

Concurrent MCP OAuth token saves can lose credentials

1 participant