Repository navigation
fix: single config commit section with per-server generations (Spec 116 PR1; #1558, #1560) - #1590
Merged
Merged
Conversation
…16 PR1) Every writer of the server set or a server's fields goes through Runtime.Commit: one configCommitMu section that gates, prepares generations, writes disk and one bbolt transaction, restamps retained instances and publishes. SaveConfiguration persists the live config instead of rebuilding it from storage; REST PATCH and MCP update/patch resolve inside the commit; the MCP quarantine path purges the index (B1); discovery checks eligibility and writes the index under the approval lock (D15) and reads approval policy from storage (D16). Adds scripts/check-upstream-write-sites.sh to CI.
MutateConfig (PATCH /api/v1/config, profile writes) built its candidate from the desired config read before the commit lock, so its apply could revert a removal or quarantine committed in between. The read, mutation, guard and pin move now run inside the apply commit.
Deploying mcpproxy-docs with
|
| Latest commit: |
ea6832d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fcf5bad3.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-116-pr1-config-commit.mcpproxy-docs.pages.dev |
… D15 hold, reconnect predicate) - A commit that cannot read the stored view aborts with nothing written (the boot commit continues without storage, as main's boot sync did). - The restart refresh imports the server from the file without rewriting it. - The no-op skip path records the caller's desired config, so a reload that cancels a pending restart-gated change is not undone by the next write. - Every published generation is written to meta.config_commit_generation, global-only commits included, so generations stay monotonic across restarts. - upstream.Manager.AddServerConfigAt replaces an instance exactly when config.ConnectionEquivalent is false (isolation, OAuth, working_dir and init timeout patches reconnect again). - A registry URL edit copies only the probe-derived fields onto the live entry. - Compensation restores the captured file mode exactly (umask-independent, 0000 included). - D15: a discovery pass's last-good snapshot and StateView publish run in the approval-lock hold after its eligibility/generation check; the sweep publishes per server. - Tests: failing-first regressions for each fix, an in-transaction rollback case for ApplyServerCommit, a connected-upstream retained-instance test, Windows-safe mode assertion. plan.md and quickstart.md amended.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ation-checked review capture and empty stamp, once-per-commit events) - REST PATCH validates max_concurrent_requests/queue_size/queue_timeout inside the commit against the live server_concurrency_defaults (Config.ValidateServerConcurrency); a value the next start would refuse is HTTP 400 with live, storage and file unchanged. - FR-008a: the review capture of an ineligible server and the empty-result ToolsDiscovered stamp run in one approval-lock hold behind a row/generation check, so a stale tools/list cannot become the review snapshot or mark discovery complete after a connection-changing commit. The inspection capture binds to the instance generation taken before its list. - FR-028: CommitTx.EventExtra carries the caller's fields on the commit's one servers.changed; QuarantineServer, EnableServer and BulkEnableServers no longer send a second copy (or a second review.changed). - check-upstream-write-sites.sh covers the AsyncManager queue writers, with planted self-tests. - T1.21 checks the operator's noncanonical bytes before and after each boot. - Spec: FR-008a, contracts/surfaces.md (PATCH 400, additive event payloads), quickstart L1.4 reads generations from config.db, createMu note in plan/tasks.
…uth clear in the row transaction, UpdateConfig path published with the commit, failed-reload recovery docs)
…proval lock, path-only UpdateConfig writes the new path, AddUpstream in the write-site check, compensation and rename docs)
… every branch, deep-copied commit working copy, step 4 lock set and reload disk/compensation docs)
…ide the commit, MCP update reports only the committed instance's connection, D5/plan Prepare and lock-set docs)
… the current instance, unchanged MCP update waits for the committed instance, write-site check catches method values, admission step docs)
…he commit lock, startup admission commit keeps the empty-set GC guard, write-site check catches split selectors, unique FR-009a/FR-010a)
…nts instead of skipping lines starting with *, contract documents PATCH /api/v1/config and no PUT server route)
… comments between the dot and the method name, research.md names POST /api/v1/config/apply instead of PUT/apply)
… r11) plan.md said the goroutine-id re-entry check in Runtime.Commit was compiled only under a commitlockcheck build tag. No such tag exists; the check runs unconditionally via runtime.Stack and returns errCommitReentry.
… commit generation order, UnderApprovalLock lock-hold and hook-error tests, write-site check exits 2 on scan failures, D5 and data-model docs match the conditional disk step and commitOwner guard)
Conflicts in internal/server/server.go and internal/runtime/lifecycle.go (#1561 PATCH quarantine purge) resolved to the commit-based UpdateServer. The commit's index purge now also covers an explicit quarantine decision on an entry the admission gate had quarantined implicitly, so main's TestUpdateServer_QuarantineTrue_PurgesIndexedTools passes without the separate PurgeQuarantinedServerFromIndex seam.
…es an explicit PATCH quarantine of a gate-quarantined server UpdateConfig (no production caller left) schedules no manager work, as before Spec 116: its connect worker raced the supervisor's reconnect of the same server and left the replacement instance disconnected, failing TestCaptureQuarantinedToolDefinitions_RelistsAfterClientReplacement about 1 run in 10 under -race. The approve-with-blocks fixture configures the counting upstream's transport, so the unquarantine commit's connect worker re-adds a server that can connect (TestApproveWithBlockNeverDispatchesBlockedTool). An UpdateServer/SetQuarantined that makes an explicit quarantine decision marks the server for the index purge even when the admission gate had already quarantined it implicitly (#1561, merged from main); whole-config replacements do not count as decisions.
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 38239075255 --repo smart-mcp-proxy/mcpproxy-go
|
…t quarantine Sol round 12 (#1061, #1561): an UpdateServer or SetQuarantined that leaves an entry explicitly quarantined now always marks it for the commit's index purge, also when the entry already was an explicit quarantine. A commit whose only effect is such a purge (an identical PATCH {"quarantined":true}, POST .../quarantine or MCP quarantine_server) no longer takes the skip rule: it takes the server's approval lock, purges the global index and rebuilds the affected profile indexes, and publishes nothing. Main's Server.UpdateServer purged on every explicit true request. FR-001a documents the exception.
…iting the published snapshot in place TestE2E_ToolResponseModeToggle and TestE2E_DirectDeferredSelfHealingInvalidParams relied on pre-Spec-116 aliasing: normalizeHarnessConfigForApply wrote tools_limit and call_tool_timeout into Config() in place, which on main happened to be the same object GET /api/v1/config serves (desired config). Under the commit design the desired config is a separate copy, so every /config/apply failed validation on tools_limit. Leg A of the toggle test also wrote the compact config file before UpdateConfig registered the path; a path-adopting commit writes the committed config at the new path (spec edge case "Path change"), clobbering it. The helper now commits a caller-owned copy through CommitConfig, the toggle test uses it and registers the path before the hand edit. New TestE2E_HarnessNormalizeReachesRESTConfig fails on the old helper.
Dumbris
added a commit
that referenced
this pull request
Oct 12, 2026
…ct engine (RT-01..RT-07) Adds the TPA attack lab (Docker-isolated malicious fixtures, red-team harness, experiment suite) and fixes RT-01..RT-07: structural input-schema changes and safety-annotation flips are held for review in every trust mode (versioned hash migration keeps existing approvals); trust_mode scan never auto-approves a changed contract; quarantined-but-connected servers have contract changes verified without indexing; scans cover the contracts discovery ingested (incl. output schemas) and fail closed on lookup errors; detect engine hardening on top of SEC-01 (destination matching, data-sink parameter shapes, schema-text scanning); scanners read a proxy-owned source copy so symlinks cannot redirect the snapshot. Fresh Docker suite: EXP-1..EXP-6c all HOLD (EXP-1 dangerous, EXP-3 dangerous); baseline runner dangerous 93/100; scan-eval recall 1.0 fp 0.0. OpenCode gpt-6.1-sol: SHIP at round 11, rounds 12-21 P2-only (cap extended by the maintainer); remaining P2s tracked in #1601 and #1602. Failing checks: E2E TestE2E_DisableServerRemovesToolsFromSearch also fails on main since #1590 (being fixed separately); CodeQL go/path-injection on operator-configured working_dir paths (non-required, reviewed).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
Spec 116 PR1 only (
specs/116-config-commit-consistency/plan.md§PR split, tasks T001-T034). This PR also ships the spec directory, so the spec PR #1583 can be closed.Every writer of the server set or of a server's fields now goes through one primitive,
Runtime.Commit(reason, fn)(internal/runtime/commit.go). It holdsconfigCommitMufrom reading the live config until the new config is published, and runs one pipeline:fnmutates the copy throughCommitTx(CreateServer,UpdateServer,SetEnabled,SetQuarantined,RemoveServer,ReplaceConfig,ReconcileLive,MutateGlobal,ExpectGeneration/ExpectIncarnation,UnderApprovalLock,Bootstrap,ForceDisk).configsvc.Prepareassigns the commit generation G and the per-server generations (commitview.ServerGen{Gen, Incarnation}), and computes the change set: Created, Updated (with Reconnect), Removed, or Replaced for a same-commit remove and re-create. It also computes the projection diff against storage.UnderApprovalLockhooks. A change toquarantine_enabledtakes every server's lock (FR-008c).ApplyServerCommit). It writes rows with their prepared stamps verbatim, the deletions (row and call history) andmeta.config_commit_generation/quarantine_enabled. Then the index purge for removed, newly quarantined and newly disabled servers (B1/security: quarantining a server via the config file leaves its tools in the search index, so retrieve_tools still discloses their descriptions #1061), the OAuth-config clear and the orphan GC. On a storage failure the captured file is restored byte for byte and nothing is published.Manager.RestampRetainedgives every retained instance its new config and generation as one atomic pair (managed.Client.SetConfigAt, forward only).configsvc.Publishand the legacyr.cfg/desiredCfgswap.After the lock is released, the commit unregisters removed servers, spawns connect workers stamped with the generation for created, replaced and reconnecting servers, requests discovery for restamped connected servers, and sends its events once.
Converted writers:
ApplyConfig,ReloadConfiguration(one pinned publish; the raw file is never published), the boot reconcile (ReconcileLive+Bootstrap),gateInitialConfig,UpdateConfig,UpdateListenAddress,SaveConfiguration(persists the live config and no longer rebuilds it from storage),EnableServer,BulkEnableServers,QuarantineServer, theRestartServerdisk refresh andSetServerShared. REST add, update, PATCH and remove, MCPupstream_serversadd, update, patch and remove, and MCPquarantine_security quarantine_serverare converted too, along with registry source add, edit and remove and the config funnel (MutateConfig).D15/D16 from the design reviews:
meta.quarantine_enabled), not from the published config.scripts/check-upstream-write-sites.sh(with a red/green self-test) runs in CI. It fails on anyupstreamsrow write outsideinternal/storageandinternal/runtime/commit*.go.Not in this PR (as the plan says): the full purge set and incarnation-bound approvals (PR2), generation checks in workers and the supervisor (PR3), and dial gates and retirement (PR4).
approveBaselineToolsForServerAt(name, inc)is the hook PR2 fills.Design decisions and deviations from the spec
MutateConfig(PATCH/api/v1/config, profile writes) read the desired config before the lock and applied a whole copy of it. That resurrected removed servers and reverted quarantines even with every other writer converted. Its read, mutation, binding guard and pin move now run inside the apply commit (applyConfigWith). The regression test isTestCommit_FunnelMutationCannotResurrectRemoval, which fails on the previous commit. Lock order:bindingWriteMu→configCommitMu.AdvanceGenerations(PR3) moves this into step 7.meta.config_commit_generationexists. Before that (unit-test fixtures that never commit), the published config is used, as onmain. In the daemon the startup admission commit writes the key before anything else runs.ValidateDetailedover the whole config.ApplyConfigand the funnel validate the submitted config, and the REST and MCP doors validate their inputs as before. Validating every commit would let one invalid unrelated entry block all writes.supervisor.Start(). The boot commit then runs withBootstrap(). Rows that match the file keep their stored stamps (Prepareseed). Neither commit rewrites the operator's file, andReloadConfigurationnever writes it either.TestHandleAddUpstream_DuplicateNameIsRefusedWithoutSideEffects).resolveServerPatch, which runs inside the commit via the new optionalhttpapi.ServerPatcherinterface (Server.PatchServer). Test doubles that lack it use the old path. The server'sUpdateServernow also appliesexpose_prompts,max_concurrent_requests,queue_sizeandqueue_timeout. The PATCH handler already resolved these fields, but onmainthey were dropped before persisting.MCPProxyServerexists only in package tests. Its storage writer lives inbare_upstream_writer_test.go, so production has no write path outside the commit.LoadConfiguredServers(cfg)is deleted (D14). The runtime package's tests get a test-only shim, andinternal/servertests callReconcileServers/CommitConfiginstead. Oneconfigsvctest drops the removed pre-publish hook.server_sharing_testreplaces the removed admission re-publication with a commit. The quarantine_security list_quarantined returns env/headers/oauth secrets in plaintext, unauthenticated #1148 door-scan tables get two rows (prepareLocked,mergeUpstreamPatch). No assertion of the fix: make server add create-only across REST, MCP and Web UI (UX-01) #1570, fix: serialize per-server tool-approval writes so baselines and approvals are complete (UX-02) #1571, security: servers loaded from mcp_config.json bypass the trust-mode admission gate entirely #937 or security: quarantining a server via the config file leaves its tools in the search index, so retrieve_tools still discloses their descriptions #1061 tests changed.create_only_add_testcallsremoveServerFromConfiginstead of the deletedremoveServerStorage.Commiton the same goroutine returns an invariant error instead of deadlocking. It uses a goroutine-id check, always on rather than behind a build tag.CopyServerConfignow copiesToonOutput, with a reflection-style test that every serialized field survives the copy.Lock order (normative, plan §Lock order)
configCommitMu→ server approval locks (sorted; released before step 7) →storage.Manager.mu/bbolt, and → bleve deletes.configCommitMu→upstream.Manager.captureMu(W) →Manager.muin step 7.configCommitMu→configsvc.updateMu→Runtime.mu.bindingWriteMu→configCommitMu(funnel). Discovery:captureSnapMu→ approval lock(one) → storage, index. Nothing that holds an inner lock callsCommit.TestLockOrder_PR1covers re-entry, commit vs discovery, and commit vs a capture section.Compatibility
test-api-e2e.shpins.UpstreamRecord.generation/incarnation, meta keys); no migration.Verification
Failing-first on the parent (seams and tests only, commit 3313465): T1.1 (both variants), T1.2, T1.3, T1.4, T1.5, T1.6, T1.7 and T1.12 all fail on
mainand pass here.New tests: T1.9, T1.11, T1.13-T1.21,
TestLockOrder_PR1, the configsvc generation table, theApplyServerCommitall-or-nothing and stamp tests,TestCommitviewIsLeaf, and the funnel resurrection test.go test -race -count=20of the runtime commit tests atGOMAXPROCS=1and4, and-count=10of the server T1 tests: green.go test -race -tags server -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint"over runtime, server, httpapi, storage, oauth, config, commitview and upstream: green. The full non-race./internal/... ./cmd/...run is green.golangci-lint v2, bare and
--build-tags server: no finding in a changed file../scripts/test-api-e2e.shon a scratch port: 68 passed, 0 failed.Live L1 on scratch qa070 daemons with the big profile: 40 concurrent REST and MCP writers, removals, quarantines, a 50x DELETE→POST cycle, and config PATCHes.
mainconfig.json== API ==config.db, after settle and after restartretrieve_toolshits right after MCPquarantine_servertrusted-libraryreconnects within about 2 s of a restart.Not run live: the forward-header revoke on a connected HTTP server (L1 step 4). T1.19 covers it at instance level.
Fixes #1558
Fixes #1560
Refs #1559
Supersedes #1583 (spec)
🤖 Generated with Claude Code