Repository navigation
fix: bind upstream workers and supervisor actions to the server generation (Spec 116 PR3; #1563) - #1608
Merged
Conversation
…or actions (#1563) Adds the PR3 test seams (beforeWorkerAct/afterWorkerAct in the post-commit and RestartServer workers, the supervisor beforeExecuteAction hook) and T3.1-T3.7 and T3.9, which fail on main: a parked worker, restart worker or supervisor action re-installs an old config, removes a re-enabled instance or disconnects a replacement after a newer commit.
…ation (Spec 116 PR3; #1563) The config commit's step 7 now calls upstream.Manager.AdvanceGenerations, which records every server's committed generation in the manager (a tombstone for a removed one), unregisters the instance of every removed, replaced or reconnecting server before the snapshot publishes, and restamps retained instances (replacing PR1's RestampRetained). Every asynchronous registry change or connect is bound to the generation it was scheduled for and re-checked under captureMu + Manager.mu in the same critical section: the commit's connect/disconnect workers (AddServerAt, RemoveServerAt), supervisor actions (generation from their planning snapshot; ActionDisconnect also bound to the observed InstanceID), RestartServer, ForceReconnectAll and Docker recovery (ReplaceInstanceAt at the same generation), ConnectAll and the OAuth logout disconnect. A stale call returns ErrStaleGeneration and does nothing. Instances leave the registry only through unregisterLocked, which marks them unregistered so they never connect again; workers connect the exact instance they installed. Instances are stamped with their generation at construction (NewClientAt), D15 drops the generation-0 exemption, and last-good tool snapshots carry the instance generation so an approval reindex re-lists after an endpoint change instead of reapplying old tools. The write-site check forbids unbound manager mutations in internal/runtime and internal/server and any m.clients write outside unregisterLocked/installLocked. qa_hooks adds the L3 park points.
…nder the server name The Disconnect of an instance a commit or replacement unregistered runs after the commit lock is released, concurrently with the replacement's connect, and its Disconnected notification (keyed by server name) could overwrite the replacement's Connected status. Superseded instances are now silent; seen live as connected=false for a connected server after a same-generation restart raced an endpoint edit.
…thod values and multiline registry mutations; align the L3 recipe (Spec 116 PR3 Sol round 1) - managed.Client.ConfigAt returns (config, generation) from one atomic load. ForceReconnectAll uses it, and when only a restamp moved the generation of the same registered instance it re-reads the pair and retries, so Docker recovery never installs the pre-restamp config at the new generation. - check-upstream-write-sites.sh: the FR-020 scan flags legacy manager method values, calls split across lines and multiline m.clients deletes and assignments; planted cases added to the self-test. - quickstart L3 now names the servers, park target, release file and PID assertions the shipped live script uses.
Deploying mcpproxy-docs with
|
| Latest commit: |
1b69a14
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://efa28463.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-116-pr3-stale-workers.mcpproxy-docs.pages.dev |
…et and comment-hidden registry writes (Spec 116 PR3 Sol round 2) Every registry mutation takes Manager.limiterPubMu before releasing mu and holds it across the Spec 093 limiter publication, so a stale action that validated its generation and lost the CPU after the unlock can no longer retire or overwrite the limiter a newer commit published. The FR-020 scanner finds assignments by operator (multi-target LHS included), matches clear(m.clients), and tracks the enclosing function on lexed code from column-0 declarations only.
…es and select receive cases (Spec 116 PR3 Sol round 3)
…ndex expressions split across lines (Spec 116 PR3 Sol round 4)
…l inside an index keeps the m.clients LHS (Spec 116 PR3 Sol round 5)
…he FR-020 scan (parenthesized, alias, method expression) (Spec 116 PR3 Sol round 6)
…ear in the FR-020 scan (parenthesized, dereferenced, alias) (Spec 116 PR3 Sol round 7)
…ge-level func literal after installLocked/unregisterLocked is scanned (Spec 116 PR3 Sol round 8)
… with Spec 117 PR-2/PR-3 and held-change review UX)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…he current instance in the inventory stress test (Spec 116 PR3 resync) Under Spec 116 PR3 a commit that changes Quarantined unregisters the instance under captureMu (retiring it) and the commit's connect worker installs the instance of the new generation asynchronously. The #1609 stress test required GetClient to succeed synchronously after the final unquarantine, which fails whenever the last toggle left the server quarantined (5/20 under GOMAXPROCS=2). Wait for the current instance to be registered and connected; the final capture and the index/approval consistency check are unchanged.
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 38347737392 --repo smart-mcp-proxy/mcpproxy-go
|
Dumbris
added a commit
that referenced
this pull request
Oct 13, 2026
…ows PR Build timeout) (#1613) ## Problem `internal/runtime` took ~472s in the PR Build `Build Binaries (windows-latest, windows, amd64, …)` job, whose `go test` runs with the default 10m timeout. It timed out once on #1608 and passed on rerun. Timings from run 38347737392 (Windows job 115142245158): | Test | Windows | |---|---| | `TestConfigCommit_CommitPathsConverge` | 92.0s | | `TestReviewPayload_MasksSecretsInsideShellCommandArgs` | 11.6s | | `TestUnquarantineViaConfigReload_LeavesIndexAlone` | 8.2s | | … 78 more tests ≥1s | 277.7s total for tests ≥1s | The package is I/O-bound rather than CPU-bound: on macOS it ran 263s at 18% CPU. Nearly every test builds a `Runtime`, and each one writes the config file (temp file + fsync + rename), commits to `config.db` (bbolt fsync per commit) and creates a Bleve index (scorch `root.bolt`). `FlushFileBuffers` is very expensive on the Windows runner. `TestConfigCommit_CommitPathsConverge` runs 200 rounds of five fsynced commit paths. ## Fix - `config.DisableWriteSyncForTesting()` makes `atomicWriteFileMode` skip the fsync before the rename. - `storage.DisableSyncForTesting()` makes `NewBoltDB` open with bbolt `NoSync`. - A `TestMain` in `internal/runtime` calls both and also sets `bbolt.DefaultOptions.NoSync`, which Bleve's scorch copies for `root.bolt`. A second `TestMain` in `internal/storage` (~147s on Windows) calls `DisableSyncForTesting`. Production code paths are unchanged: both switches default off, and only `_test.go` files call them. No test is skipped, shortened or retimed, and no timeout is raised. Writes are still visible in-process to every reader; only crash durability is dropped, and no test in these packages simulates a crash. ## Local results (macOS) | | Before | After | |---|---|---| | `internal/runtime` (no race) | 263s, 18% CPU | 121s | | `TestConfigCommit_CommitPathsConverge` | 16.3s | 0.5s | | `go test -race` runtime/storage/config | — | all pass (runtime 259s) | ## Windows results (PR Build, windows-latest amd64) | | Before (run 38347737392) | After (run 38359460609) | |---|---|---| | Job duration | 13m45s | 7m04s | | `internal/runtime` | 471.7s | 166.5s | | `internal/storage` | 147.4s | 9.6s | | `TestConfigCommit_CommitPathsConverge` | 92.0s | 1.0s | No test failures in the job. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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 PR3 only (
specs/116-config-commit-consistency/plan.md§PR3,tasks.mdT060-T077): every connect/disconnect worker, supervisor action,RestartServer,ForceReconnectAll/Docker recovery andConnectAllis bound to the server's commit generation (and, for an action on an existing instance, to that instance) and re-checks it inside the manager's lock before acting. A stale one is inert, so it can no longer re-install an old config, re-create a removed server, unquarantine a quarantined one or disconnect a replacement after a newer commit (#1563).Not in this PR: the dial gate / retirement barrier incl. OAuth (PR4, #1564) and the OAuth credential binding (#1607).
Design
upstream.Manager.AdvanceGenerations(replacing PR1'sRestampRetained) undercaptureMuW +Manager.mu: it records every configured server's committed generation in the manager'sdesiredview (a tombstone for a removed name), unregisters the instance of every removed or replaced server and of every server whose connection settings,enabledorquarantinedchanged, and restamps retained instances. This happens before the snapshot is published; the unregistered instances are disconnected after the commit lock is released.AddServerConfigAt/AddServerAt,RemoveServerAt,ConnectServerAt(generation),DisconnectServerAt(name, gen, InstanceID)andReplaceInstanceAt(name, gen, expected, cfg)(generation + exact instance). Each returnsErrStaleGeneration(=commitview.ErrStaleGeneration) and does nothing unless the check passes in the same critical section as the change. Callers log it at debug level.managed.Client.InstanceID()(process-wide, never reused). Instances are stamped with their generation at construction (managed.NewClientAt). Every removal fromManager.clientsgoes throughunregisterLocked(and every insert throughinstallLocked).connectWorker/disconnectWorker) use the generation of their commit and connect the exact instance they installed. The supervisor binds each action to the generation of the snapshot it was planned from;ActionDisconnectalso binds to theInstanceIDit observed (ActorPoolSimpleno longer resolves by name).RestartServer,ForceReconnectAlland Docker recovery replace the instance at the same generation throughReplaceInstanceAt.ConnectAllskips an instance that is not at its committed generation. The OAuth logout disconnect is bound to the instance it marked logged out (the hazard recorded on bug: stale connect/disconnect workers and supervisor actions re-install old server configs after a newer commit #1563); the token clear stays name-keyed (Spec 116 PR2b: bind OAuth token reads/writes to the server incarnation #1607).captureMufor writing, so it waits for a capture section in progress (T3.5).scripts/check-upstream-write-sites.shalso fails on an unbound manager mutation (AddServerConfig/AddServer/RemoveServer/DisconnectServeron the manager) ininternal/runtimeorinternal/server, on an internal legacy call (m.X(...)) ininternal/upstream, and on anym.clientswrite outsideunregisterLocked/installLocked(self-test extended).internal/qahooks(-tags qa_hooksonly; nil in release builds):MCPPROXY_QA_PARK_WORKER,MCPPROXY_QA_PARK_SUPERVISOR_DISCONNECT,MCPPROXY_QA_RELEASE_DIR.Deviations from the spec text (documented in
tasks.mdPR3 implementation note)unregisterLockedalso marks the instance unregistered:Connecton it returnsmanaged.ErrInstanceUnregistered, a dial in flight when it was unregistered discards its result, and its state transitions are no longer notified under the server name. Without this a stale holder (a worker, the reconnect loop,ConnectAll) could reconnect an instance the manager no longer serves and leak its process (the baseline live run shows such leaks), and a superseded instance's lateDisconnectednotification could overwrite the replacement'sConnectedstatus (seen live, fixed in the last commit). This is a connect-level latch only; the byte-levelRetirebarrier remains PR4.AdvanceGenerationsreceives the whole committed server set (plus removals), not only the changes, sodesiredis complete after the boot and admission-gate commits. It returnsAdvanceResult{Unregistered, Restamped}.ReplaceInstanceAttakes an optional config and returns the new instance, which the caller connects (Manager.ConnectInstance).ActionReconnecthas no disconnect leg since fix: disabling a server removes its tools from search again (regression from #1590) #1603, so onlyActionDisconnectcarries anInstanceID.ActionRemoveusesRemoveServerAt(name, 0), which acts only while the committed configuration has no such name.AddServerConfig/AddServer/RemoveServerstay for tests and CLI-only managers; for a name a commit recorded they install at the committed generation and never move it. The daemon never calls them (CI grep).actor_pool_complex_reference.go(unreferenced, unbound manager calls) is deleted. The test-only discovery wrappers moved to a_test.gofile and bind to the committed generation.Lock order
Unchanged from plan.md §Lock order: L0
configCommitMu→ L1 approval locks (released before step 7) → L2 storage; L0 → L3captureMu(W) → L4Manager.muin step 7 (AdvanceGenerations). The bound methods take L3 W → L4 for registry changes and L4 R for checks; none holds L3/L4 while connecting or disconnecting, and none calls into the runtime. Capture sections (L3 R → L1) are unchanged; the commit still never holds L1 and L3 together.TestLockOrder_*pass at-race -count=20,GOMAXPROCS=1and4.Compatibility
No config, storage, REST or CLI change. Behaviour visible to users: an edit that changes a server's connection settings,
enabledorquarantinedunregisters the old instance when the commit publishes (the commit's worker installs the new one), and a restart replaces the instance atomically. The manager API gains the bound methods; the unbound ones remain for tests/CLI.Verification
190e9b4d6, seams only): T3.1 (remove/replace/quarantine), T3.2, T3.3 (realActorPoolSimple+upstream.Manager; remove/replace/quarantine), T3.4, T3.5b, T3.6, T3.7, T3.9 all fail on main. T064/T3.8/T3.10 (internal/upstream/manager_generation_test.go) use the new API.go test -race -tags server -timeout 30m -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/...: all packages ok, no data race.TestLockOrder*+ T1.19 at-race -count=20,GOMAXPROCS=1and4: ok.TestE2E_DisableServerRemovesToolsFromSearch -count=20: 20/20.--build-tags server(--new-from-rev=origin/main): 0 issues.check-upstream-write-sites.test.sh: 39/39. Both editions and-tags qa_hooksbuild.make build+./scripts/test-api-e2e.shon scratch ports 19005/19006: 70/70.-tags qa_hooks), baseline main vs PR3:Evidence:
docs/qa/0.70.0-ux-2026-10-10/remediation/evidence/116pr3/EVIDENCE.md(not committed).Fixes #1563
🤖 Generated with Claude Code