Repository navigation
fix: atomic server removal with full purge and per-server incarnation (Spec 116 PR2; #1559, #1562) - #1600
Merged
Conversation
…rnation-bound approvals T2.1/T2.10 (every removal path purges the whole purge set and the last-good snapshot), T2.2 (a same-name re-add through REST and MCP inherits nothing), T2.4/T2.17/T2.19 (approvals and the unquarantine baseline bound to the incarnation), T2.5 (scanner approval across a remove and re-add), T2.6 (exact OAuth key match, B2), T2.12 (late identity registration), T046 (toolhash key format), FR-017 (payload generation/incarnation), plus the PR2 test seams (nil in production). T2.7 is regression coverage of PR1's D15 check.
… the commit transaction (Spec 116 PR2) ApplyServerCommit now deletes the purge set of every removed or replaced server in the row's bbolt transaction: tool and prompt approvals, OAuth tokens and DCR client, OAuth completion events, scan reports and jobs (with the job index), the integrity baseline, tool-call history and server identities, and tool hashes (data-model PurgeSet). ApplyServerCommitWithReport returns a PurgeReport per name. ClearOAuthState and the purge match OAuth keys exactly: the bare name or name_ plus 16 lowercase hex characters, so removing foo no longer deletes the tokens of foo_bar (B2). Incarnation-conditional writers for state an asynchronous writer can produce after a removal: RegisterServerIdentityIfIncarnation, RecordToolCallIfIncarnation (code_execution exempt), SaveScanJobIfIncarnation and SaveScanReportIfIncarnation check the stored row in the same transaction. A startup commit that loads no server keeps the trust state (RowOnlyRemoves), as the boot-time orphan GC does. commitview gains ErrServerReplaced and ServerReplacedError so the scanner and the HTTP layer can match it without importing the runtime.
…writers (Spec 116 PR2; #1559, #1562) The commit records CommitResult.Purged, drops the last-good tool snapshot and capture stamp of a purged server under its approval lock, and after the lock cancels the OAuth refresh schedule, revokes an inspection exemption, evicts dead signature-cache entries and notifies purge observers. Connect workers register the server identity only while the row keeps the incarnation they were started for; replayed calls record history the same way. Every approval writer is bound to an incarnation (FR-014): it captures it when it starts (or takes the caller's expected_incarnation), and under the server's approval lock - the lock the removal purges under - it re-checks the stored row and writes nothing (ErrServerReplaced) when the server was removed or re-added since. Covered: ApproveTools, ApproveAllTools, BlockTools, BlockAllTools, SetToolEnabled, SetAllToolsEnabled (with *At variants), WithServerApprovalLockAt for writers outside the runtime, and QuarantineServerAt (ExpectIncarnation in the commit). The baseline promotion after an unquarantine is bound to the incarnation of the commit that unquarantined, and promotes nothing when the row moved or is quarantined again (review R9.5). Server payloads and the review payload carry generation and incarnation; removal activity records carry the purge summary.
…d scanner approvals, expected_incarnation on REST and the Web UI (Spec 116 PR2)
REST and MCP removal use the commit's purge and drop their ad hoc cleanup
(the REST OAuth clear with its prefix match, the refresh-schedule call and
the tool-approval delete). MCP removal records the purge summary.
Prompt approvals (checkPromptApprovals, ApprovePrompt, ApproveAllPrompts)
read and write a server's records under its approval lock after the
incarnation check; RefreshPrompts captures the incarnations before it
lists, so a pass that straddles a remove and re-add records nothing for the
replacement (FR-016).
Scanner approval captures the incarnation, saves the baseline and blocks
under the approval lock only while the row keeps it, and unquarantines
through a commit that expects it (FR-015); a remove and re-add in between
fails it with server_replaced and leaves the replacement quarantined. Scan
jobs and reports record their incarnation and are saved conditionally, an
unforced approval ignores a report of another incarnation, and a purge
drops the cached verdict (FR-014a). Tool-call history from proxied and
code_execution calls is bound to the incarnation the call started on.
REST: generation and incarnation on server payloads; optional
expected_incarnation on tools/approve, tools/block, tools/{tool}/enabled,
enable_all, disable_all, unquarantine and security/approve; 409
server_replaced and 404 for an unknown server. The Web UI sends the
incarnation of the review or server it rendered. A qa_hooks-only
'debug purge-report' / 'debug seed-purge-set' command supports the live
checks.
REST API (removal and incarnation, expected_incarnation, 409 server_replaced), architecture (purge set in the commit, incarnation-bound approvals), security quarantine (re-add starts clean), CLAUDE.md recent changes, and the Spec 116 plan and tasks marking what PR2 ships and what moves to a PR2b (the OAuth credential binding).
Deploying mcpproxy-docs with
|
| Latest commit: |
a6591ab
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8135eaa5.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-116-pr2-atomic-removal.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 38312413118 --repo smart-mcp-proxy/mcpproxy-go
|
…ool listings, unbound writes refused after commit, scanner incarnation carry-through - GetServerReview and GetAllServers (StateView and legacy paths) read the displayed config and the generation/incarnation from one committed snapshot. - Replay captures the incarnation before resolving the client and rejects a server with no committed identity. - Storage refuses unbound (incarnation 0) tool-call, scan-job and scan-report writes once a config commit exists. - Scanner: Pass 2 and its failed placeholder carry the originating job's incarnation (skipped when no longer current); StartScan refuses a server the committed view does not know (REST 404); every save goes through the conditional writer; the purge observer detaches an obsolete active job. - REST tool listings carry the incarnation read before their rows (incarnation, server_incarnation); the Web UI binds global Tools batch actions and Server Detail decisions to the rows on screen and refuses stale rows; the review screen shows failed per-tool decisions on every server. - Spec: #1562 qualified (at-rest half in PR2, in-flight OAuth saves in PR2b).
… stored row, in-commit refresh/exemption clears, identity-backed call history, scan registration and stale-job cleanup bound to the incarnation, Web UI choices and selections bound to the incarnation
…dexes, incarnation check on no-op bulk toggles, incarnation-bound scan summary cache and observer predicate, OAuth PR2b limitation disclosed
…gate's record, removed server's active scan not inherited by a same-name replacement, unbound unquarantine bound at arrival, missing-server 404 on unquarantine and security/approve
…te identity-checked once the original call's server was removed, late toggle rollbacks bound to the review on screen
…record (same-config re-add reuses the identity hash), tool listings serve the StateView only at the committed incarnation and reconcile never carries tools across an incarnation
…rom a lagging view (404 / omitted), tool listings return rows only when the incarnation did not move across the read, removal docs qualify re-add behavior by admission policy
…w only at the bound incarnation, the global listing applies the quarantine exclusion to the replacement it served, the Web UI joins hold evidence only across matching incarnations, the purge oracle finds orphaned history buckets
…rge also clears tool_baseline_decisions marker; incarnation-bound CommitServerApprovalAt)
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 PR2 (
specs/116-config-commit-consistency, plan.md §PR2, tasks.md PR2): removal is atomic and complete, and a same-name re-add starts clean. It builds on PR1 (#1590), which already put every removal path on one synchronous commit, so nothing can resurrect a removed server.internal/storage/purge.go).ApplyServerCommitdeletes the whole purge set of every removed or replaced server in the bbolt transaction that deletes its row. The set is tool and prompt approvals, OAuth tokens and DCR client, OAuth completion events, scan reports and jobs (with the job index), the integrity baseline, tool-call history and server identities, and tool hashes.PurgeReportper name goes toCommitResult.Purged.ClearOAuthStateand the purge delete onlynameandname_followed by 16 lowercase hex characters. Removing or logging outfoono longer deletesfoo_bar's tokens.RemoveServerand MCPupstream_servers removelose their ad hoc cleanup: the REST prefix OAuth clear, the refresh-schedule call and the approval delete. The MCP removal activity record carriesmetadata.purged.*Atvariants), prompt approvals andcheckPromptApprovals, and scanner baseline+blocks each capture the incarnation (or takeexpected_incarnation). Under the server's approval lock, which the removal purges under, they re-check the stored row and returnErrServerReplacedwith nothing written. Scanner approval unquarantines throughCommit(ExpectIncarnation)(FR-015). The unquarantine baseline promotion is bound to the incarnation of its own commit and requires the row to still be unquarantined (R9.5).generationandincarnationare on server payloads (REST and SSE embed via management) and on the review payload. These routes take an optionalexpected_incarnation:tools/approve,tools/block,tools/{tool}/enabled,enable_all,disable_all,unquarantineandsecurity/approve. A mismatch returns 409server_replaced: server '<name>' was removed or replaced since it was reviewed; an unknown server returns 404. The Web UI sends the incarnation of the review or server it rendered.oas/swagger.yamlis regenerated anddocs/api/rest-api.mdis updated.mcpproxy debug purge-report/seed-purge-set(build tagqa_hooks, never shipped) for the live checks.Design decisions and deviations from the spec
ServerGenthreaded through theupstream.Manager→managed→core→oauthconstructors, and PR3 (bug: stale connect/disconnect workers and supervisor actions re-install old server configs after a newer commit #1563) rewrites those constructors (AddServerConfigAtand friends). Landing it first would make both PRs edit the same constructors.SaveTokenhas no incarnation check, so an in-flight OAuth write from the old instance (a code exchange, refresh or DCR save already past its network round-trip) can land after the purge, or after an OAuth-config clear that keeps the name and URL.tasks.mdandplan.md.ServerCommitBatch.RowOnlyRemoves). This mirrors PR1's boot-time orphan-GC guard, so a config that came up empty cannot wipe every approval and credential. An explicit removal, or anApplyConfigto an empty set, still purges (T2.1apply_empty_set).ApplyServerCommitrefuses zero stamps.ErrServerNotFound(REST 404) instead of silently doing nothing. Otherwise it could write a record no purge will ever see.ErrServerReplacedmoved tocommitview. It is aliased in runtime, with aServerReplacedErrortype, so the scanner and httpapi can match it without importing the runtime.internal/storage/purgetest.Lock order
Unchanged (plan.md §Lock order). Approval writers take L1 for one server (sorted only inside
Commit) and then L2. Prompt and scanner writers reach L1 only throughRuntime.WithServerApprovalLockAt, never while holding L0 or L3. The in-memory purge takeslastGoodToolsMu(a leaf) under L1, as D15 discovery already does. Refresh-schedule cancellation, exemption revocation and purge observers run after L0 is released. Conditional async writes are single bbolt transactions and add no edge.Compatibility
generationandincarnationon server and review payloads,incarnationon scan jobs and reports. Additive optional request fieldexpected_incarnation.-tags qa_hooks.Verification
main43c5dc8): storage T2.1, T2.6, T2.17 and T046; runtime T2.1, T2.4, T2.12, T2.17 and T2.19; server T2.2, T054, T2.5 and FR-017. All fail on the parent. T2.7 passes on the parent as regression coverage. Tests that use APIs PR2 adds (*At,Purged, conditional writes, T2.8, the httpapi 409 mapping, vitest) were written with the implementation.go test -race -tags server -timeout 30m -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/...: exit 0.go test ./internal/runtime/... ./internal/server/...(CI skip regex) pass. Frontend vitest passes 2601/2601 andvue-tscis clean..github/.golangci.yml) reports nothing in changed files, both bare and with--build-tags server.scripts/check-upstream-write-sites.shpasses../scripts/test-api-e2e.shon a scratch port (LISTEN_PORT=19003): 70/70.make buildOK. The Web smoke sweep (scratch port 19005): 111 passed, 3 failed, all on the Profiles and Clients screens this PR does not touch (1100px layout, /clients contrast).mainbinary against the PR2 binary, same script. The evidence lives outside the repo:docs/qa/0.70.0-ux-2026-10-10/remediation/evidence/116pr2/EVIDENCE.md.acmestill holds 2 prompt approvals, an integrity baseline, scan jobs and reports, and an OAuth completion. The replacement shows the old scan'scleanverdict (report_id scan-acme-qa). A staleapprove_allapproves the replacement's 12 tools (200), and a stalesecurity/approveunquarantines it (200). Servers removed through MCP (foo) and a config edit (bravo) keep their OAuth tokens, prompt approvals, baselines and scans.not_scanned. The stale approve and the stale security approve both return 409server_replaced, and nothing changes.fooandbravohave zero purge-set keys.foo_barkeeps its token and stays connected. In a 30-iteration concurrent remove/re-add vs bound-approve loop, every approval either ran before the removal or answered 409; the finalacmehas no approved tool.config.json, the API and storage agree, before and after a restart, with no resurrection.Not run here: the cross-model review rounds, which the lead runs.
Fixes #1559
Part of #1562 (the credential half closes with PR2b, T2.9)
🤖 Generated with Claude Code