Skip to content

bug: LoadConfiguredServers reconciles from a stale config snapshot outside the commit lock (deletes new servers, prunes approvals, reverts quarantine) #1558

Closed

Description

@Dumbris

Problem

Runtime.LoadConfiguredServers(cfg) reconciles storage, the search index and upstream connections from a captured config snapshot, and does so outside configCommitMu. It is scheduled asynchronously by ApplyConfig (internal/runtime/runtime.go:2092-2093). When the snapshot is older than a concurrent commit, every phase acts on stale data:

  • Orphan GC uses the snapshot's name set: PruneOrphanToolApprovals (internal/runtime/lifecycle.go:1461) and PruneOrphanToolCalls (:1472) delete the approval records, explicit tool blocks and call history of a server added after the snapshot.
  • Removal (:1600-1640) treats any stored row missing from the snapshot as "no longer in config" and calls DeleteUpstreamServer (:1627), deleting a server that was just created through REST/MCP. A following SaveConfiguration then drops it from runtime and disk.
  • Storage save loop (:1557, SaveUpstreamServer(serverCfg)) and the connect goroutines (:1573-1574) write the stale snapshot over a server that was removed and re-created (new args/env/quarantine/blocks), or revert a newer explicit quarantine decision.
  • ApplyConfig publishes the new config before it updates storage; any later commit that rebuilds the server list from storage (SaveConfiguration, lifecycle.go:1667) restores the stale quarantine/disable state of server A when an unrelated server B is toggled before the queued reconcile runs.
  • Same class on the add path: Server.AddServer builds its config snapshot from s.runtime.Config() before UpdateConfig takes the commit lock (internal/server/server.go:1995-1996; MCP internal/server/mcp.go:5978), so a concurrent ApplyConfig between the two is overwritten (can undo profile restrictions or other security settings).

Failure scenario

  1. ApplyConfig (config A) schedules reconcile; the worker is delayed.
  2. A server B is added via REST. It is approved, one tool is blocked, a call is recorded.
  3. The delayed reconcile runs with the pre-B snapshot: B's approvals/blocks/history are pruned and B's row is deleted. On an auto-trust server, rediscovery recreates the blocked tool as approved+enabled.

Suggested direction

Make reconcile a commit: take configCommitMu, re-read the live config inside it and reconcile against that (or discard the snapshot when the live pointer moved), and make ApplyConfig publish + storage update one commit. Never delete a row/records unless the name is absent from the current live config. Cover with deterministic hook-gated -race tests (pause before storage read / before save loop / before prune).

Where Sol found it

ux01-r2.txt #1, ux01-r3.txt #1, ux01-r4.txt #1, ux01-r5b.txt #1 (+ ux01-r5a.txt), ux01-r9-B.txt #1, ux01-r1.txt #1 (add-path snapshot).


Found by OpenCode Sol 6.1 cross-model review of the (now split) oversized PRs #1553 (UX-01) and #1555 (UX-02); verified still present on origin/main (704795c) by reading the cited sites. Pre-existing, deliberately out of scope of the minimal core PRs. Review transcripts: docs/qa/0.70.0-ux-2026-10-10/remediation/reviews/.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/storageBBolt database, config, and persistencekind/bugSomething isn't workingpriority/highShould be addressed in the current or next release

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions