fix(docker): scope instance ID to data dir instead of a host-wide temp file - #1300
Merged
Merged
Conversation
…p file GetInstanceID() persisted to a single filepath.Join(os.TempDir(), ...) file shared by every process a user runs on a host, so any two concurrent mcpproxy processes (a scratch dev instance next to the main app, two separate installs) silently got the SAME instance ID. That defeats com.mcpproxy.instance container-ownership labels used by manager.go's cleanup and by the ownership filter added in PR #1298 for same-host instances (cross-host-shared-daemon cases still work). Scope the ID to the process's data dir instead (cfg.DataDir, set once via core.SetInstanceDataDir from upstream.NewManager, the common construction point for every entry point) since concurrent instances on one host already require distinct data dirs -- BBolt locks config.db. Fall back to a fresh per-process UUID when no data dir is known yet. On first run under a data dir, adopt-then-retire the legacy shared file so pre-upgrade containers stay cleanable by whichever instance starts first, without letting every future data dir keep re-adopting the same shared ID. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
ce5c2f7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c769bc02.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-infallible-shannon-ef.mcpproxy-docs.pages.dev |
…view codex gpt-5.6-sol review of the instance-ID fix (PR #1300) found that adoptLegacyInstanceID's read-then-remove let two concurrent processes both read the same legacy id before either deleted the file, recreating the very host-wide-shared-id bug the fix targets. Claim the legacy file via os.Rename to a process-unique path instead: rename atomically fails once another process has already claimed the source, so exactly one process adopts a given legacy id. Also fixes a test bug the same review caught: the subprocess helper tests never overrode legacyInstanceIDPath in the child process, so they could have adopted-and-deleted a real machine's actual pre-upgrade legacy file. Threads a scratch path through an env var instead. Adds a goroutine-race test (50x under -race) proving the claim is exclusive. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35328834519 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Windows CI failed TestAdoptLegacyInstanceIDConcurrentClaimIsExclusive (#1300 windows-amd64 build): all 8 concurrent goroutines got 0 adopters instead of exactly 1. The test spawns racers as goroutines in one OS process, so they all share one os.Getpid() and therefore compute the identical claim destination -- a collision that can't happen in real usage (real racers are always separate processes with distinct PIDs). On Windows that collision made every racer's rename fail. Add an atomic per-call sequence number alongside the PID in the claim path, so it can never collide even when called concurrently within one process. No behavior change for the real (cross-process) case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Second codex gpt-5.6-sol review round on PR #1300. opencode's Copilot quota was still exhausted, so codex gpt-5.6-sol (the documented fallback) reviewed again. Findings: 1. adoptLegacyInstanceID removed the claimed legacy file unconditionally, even when the subsequent save to the new per-data-dir location failed -- silently losing the id on the next restart. Now the claim file is only removed after a successful save; on failure it's left in place so it isn't silently destroyed. 2. The PID+sequence claim-path suffix (from the previous round's Windows fix) doesn't guarantee cross-restart uniqueness -- PIDs get reused, and the review argued a stale claim file from a crash could collide with a later process's first claim. Go's os.Rename on Windows actually uses MOVEFILE_REPLACE_EXISTING (verified against the Go 1.26 stdlib source), so this wouldn't have caused a failure the way the review described, but using a random UUID instead of PID+sequence removes any theoretical collision outright and is simpler. 3. Test coverage: TestGetInstanceIDDistinctAcrossConcurrentProcesses ran its two subprocesses sequentially despite its name; and there was no test of the ACTUAL cross-process legacy-claim race (only an in-process goroutine simulation, which artificially shares one PID across racers). Added TestGetInstanceIDCrossProcessLegacyClaimIsExclusive (two real subprocesses racing over one seeded legacy file) and TestAdoptLegacyInstanceIDPreservesClaimWhenSaveFails, and made the existing "concurrent processes" test actually start both processes concurrently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…gnature CI on PR #1300 failed to build cmd/mcpproxy on every platform: "assignment mismatch: 2 variables but loadConfig returns 3 values". Unrelated to this PR's instance-ID changes -- main itself is currently broken this way (#1299 changed loadConfig to also return a *serveConfigSaver, and the listen_flag_test.go added by #1301 wasn't updated for it). Discard the unused saver return to match the real signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 18, 2026
…ance (FR-007) Cross-model review (codex gpt-5.6-sol quota exhausted, fell back to codex exec) round 1 on PR #1284 found a BLOCKER: ownsContainer/ContainerOwnedByAny checked only the com.mcpproxy.server label and the canonical name, never the com.mcpproxy.instance label #1300 already stamps on every container. Two mcpproxy processes (distinct data dirs) that each configure a server with the same raw name would each pass the OTHER's container through the predicate — cleanupAllManagedContainers, ForceCleanupAllContainers, and every per-server core.Client stop/kill/rm path could stop, kill or rm a live sibling instance's container and log it as its own. Thread the instance label through the read/predicate chain instead: ownsContainer and ContainerOwnedByAny now take an instanceLabel and require it to equal core.GetInstanceID(); ownedContainer/ContainerRow/ managedContainer grew an Instance field populated from a new docker-ps --format column, and ContainerMutator.Owns grew a third parameter. Updated the Docker-side --filter on the per-server listing paths for the same belt-and-braces reason the server label already gets one. Test fixtures across docker_ownership_test.go, docker_mutation_reverify_test.go, docker_review_round11_test.go and manager_container_ownership_test.go that represent an OWNED container now carry this test process's own instance label (a withOwnInstance helper); added direct predicate-table cases for a missing/mismatched instance label, plus an integration case in TestSweeps_ReverifyOwnershipAtMutationTime proving both the shutdown and emergency sweep reject another live instance's same-named container. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 18, 2026
…logging, canonical container ownership (Spec 105 PR E, FR-007) (#1284) * test(scope): Spec 105 PR E red phase — log attribution, OAuth stop routing, container ownership (FR-007) Failing tests for gaps FR007-G1..G6 (tasks T048-T053), all red on HEAD by assertion; every file compiles against HEAD: - internal/logs/logger_attributed_test.go: colliding `a/b`/`a_b` writers under both encoders, child stderr/launcher text cannot forge the stamp (D8 left-to-right boundary rule), legacy unstamped + torn lines withheld, subject-evidence rule for historical container/callback records (container_owner, sanitised name is never evidence), filter-before-limit, case-only names (branches on FS case sensitivity) and forced-rotation shared history with administrator outcomes recorded. - internal/logs/logger.go: ReadUpstreamServerLogTailAttributed signature scaffold delegating to the whole-file reader (T054 fills the body). - internal/server/mcp_tail_log_scope_test.go: colliding-file fixture with real stamped writers (closers closed); scoped a_b token gets only own records with lines_returned == filtered count; SC-001 differential with and without hidden a/b; administrator whole-file pin (green). - internal/oauth/callback_stop_logger_test.go: StopCallbackServer routes the stop/dropped-waiter records through the stopped server's recorded logger in both start orders (private manager, observer per server). - internal/upstream/core/docker_ownership_test.go: sh+awk fake docker via SetWellKnownDockerPathsForTest + ResetDockerPathCacheForTest; connect, disconnect name-pattern and image-name fallback never mutate or log a foreign container; ownership matcher table a vs a-b vs a/b vs A driven through ensureNoExistingContainers. - internal/upstream/core/upstream_logger_audit_test.go (T054a, green pin): every upstreamLogger.* call passes a constant message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(scope): Spec 105 PR E — per-record log attribution, subject-bound OAuth stop, canonical container ownership (FR-007) Green phase for gaps FR007-G1..G6 (tasks T054-T059): - internal/logs/attribution.go: ReadUpstreamServerLogTailAttributed — no new field (D8); the existing `server=<raw>` stamp is the ownership signal. Console encoder: scan ` | {` boundaries left to right and accept the first whose suffix decodes as exactly one complete JSON object; JSON encoder: the whole line; every top-level `server` value must equal the requested name; subject-evidence rule for container records (container_owner required and equal, sanitised names are never evidence) and callback records naming another server; filter before the tail limit; lines with no accepted boundary withheld. Whole-file reader untouched (SC-005). - internal/server/mcp.go handleTailLog: attributed reader for scoped callers, whole file for administrators; lines_returned counts the authorized tail. - internal/oauth/config.go: stopCallbackServerLocked logs through the stopped server's recorded logger; StopCallbackServerWithLogger no longer adopts a logger as the manager logger on stop (signature kept). - internal/upstream/core/docker_ownership.go + docker.go: every cleanup path (ensureNoExistingContainers, disconnect name-pattern fallback, image-name fallback, exact-name kill) filters by label com.mcpproxy.server=<raw> AND ^mcpproxy-<sanitised>-[a-z0-9]{4}$ server-side and again in Go; foreign containers are neither mutated nor logged; every housekeeping record that names a container carries container_owner (D9). - connection_launcher.go loggerWriter: one record per line — a child write carrying a line break can no longer start a fresh line (D8 rule 2 relies on line boundaries; research.md D8 records the launcher-path finding). - Inverted pinned tests (T058): mcp_tail_log_scope_test.go fixture writes the canary through the real stamped writer plus an unstamped legacy line that scoped callers must not see; mcp_secret_redaction_test.go tail_log fixture uses stamped records in both encoder shapes for scoped and admin callers. Fixture fixes: callback_stop_logger_test waits for both stop records and judges the foreign observer by records about the stopped server; tailLogLineSignature strips the caller segment. - docs: tail_log attribution rule + retained effects (agent-tokens), container ownership rule and correct label names (docker-isolation); tasks.md Phase 5 ticked; ROADMAP.md regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E critique round 1 — over-long lines skipped, container_count evidence, audit widened, OAuth stop fields Spec 105 FR-007 critique round 1 (security + parity/tests): - internal/logs: the attributed reader no longer turns a >1 MiB line into a scoped-caller error (bufio.ErrTooLong); over-long lines are consumed as non-attributable so the response class is independent of hidden co-owners (SC-001). `container_count` is a container subject under D8 rule 3 (pre-105 sweep counts included co-owners' containers). - internal/upstream/core: the sweep's count record carries container_owner; producer audit now covers every zap level call on any receiver across core, launcher and oauth (oauthLogger tee) with two reviewed exceptions; loggerWriter newline split pinned; ownsContainer tested directly and through a filter-blind fake-docker mode. - internal/oauth: stop/dropped-waiter records no longer re-add server/bind_host/port the recorded logger already carries. - internal/server: differential gains a true absent-co-owner arm; T048 oracle justification recorded. - docs/specs: GET /api/v1/servers/{id}/logs is not an administrator reader (whole-file, agent-reachable) — documented and recorded as a gap-map §8 follow-up with the whole-file 64 KiB cap; docker-isolation documents cidfile-only tracking for user `docker run` servers and stale containers after a rename; research D8 records the over-long rule, profile-scoped administrator decision and audit scope. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E codex round 1 — first-boundary log attribution, ownership on cidfile and exact-name container paths Codex round 1 raised three findings against PR E (Spec 105 FR-007); all three reproduced red and are fixed here. 1. internal/logs: a torn foreign console record (partial final write, no terminator) followed by an appended complete `a_b` record shares one physical line. The reader rejected the fragment's ` | {` boundary and scanned on to a_b's, attributing the whole line — a/b fragment included — to a_b. The accepted boundary is now the FIRST ` | {` on the line; a first boundary that does not decode marks the line non-attributable. A `{`-prefixed line is judged as one JSON object and no longer falls through to the console scan (a torn JSON-era fragment before a console record was disclosed the same way). Trade-off pinned in ChildTextCannotForgeOwner: a launcher-pumped child line containing ` | {` under the console encoder is now withheld from its own writer too, never misattributed. Residual documented in attribution.go: a tear inside the message part leaves only message text ahead of the later record and is indistinguishable from that record's message. Regression: ConcatenatedTornFragmentWithheld (3 shapes × 2 encoders). 2. internal/upstream/core, cidfile path: the id read from --cidfile was recorded as owned, written into the per-server log with a fabricated container_owner=<requesting server>, and stopped/killed on disconnect without ownsContainer. A user-configured direct `docker run --name custom` gets a cidfile but neither the label nor a canonical name, so under D9 it is not ours. trackCidfileContainer and killDockerContainerWithContext now inspect the container (docker ps -a --filter id= with the ownership format) and apply ownsContainer before acting and before writing any record; container_owner is the label read back. A container failing ownership is left alone and never named in the per-server log. Consequence (docs/features/docker-isolation.md): MCPProxy no longer stops a user `docker run` server's container at all — use --rm. 3. internal/upstream/core, exact-name paths (cidfile name recovery and killDockerContainerByNameWithContext): filtered by label and tracked name only, never applied ownsContainer, and attributed with the requested server. Both now go through lookupOwnedContainerByName (label + anchored QuoteMeta name filter, Go-side ownsContainer) and record the label read back. Production only ever tracks the generated canonical name, so the reachable hole was nil; fixed as a contract violation. Every lookup now flows through listOwnedContainersFiltered, so no path can act on or log a row the predicate did not admit. stopOwnedContainer reports success and writes the outcome to the per-server log (keeping the records the cidfile path wrote before). cidfile poll interval/attempts are package vars so the recovery fallback is unit-testable. Fake docker gains an id= filter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E codex round 2 — one-header console records, subject-safe deprecated OAuth start, child output as a container subject Logs (attribution.go): - A console record is accepted only when the text in front of its first ` | {` boundary is exactly one record header at offset 0 (a foreign record torn inside its message left only text in front of the later record and was handed to the later writer); a suffix that is itself a JSON-encoder record (level+ts+msg) is rejected. - Line cap measured on content: exactly 1 MiB stays eligible. - Child output is a container subject when it names one: producers stamp child_output=true and the reader withholds a child-output record that mentions a container id, a canonical container name or Docker's name-conflict phrase unless container_owner matches. OAuth (config.go): the deprecated StartCallbackServer no longer resolves its logger through adoptLoggerLocked(nil) — a caller without a logger records the zap global, never the last-installed server's tee, so a server started that way cannot write its start/stop records into another server's log. Docker: the pre-spawn "Docker isolation configured" record carries no container_owner for the generated, unverified name; loggerWriter writes the launcher-pumped child line as the `message` field of a constant "launcher" record (audit exception removed); monitorStderr stamps child_output. Fixture: a/b vs hidden a-b, fake docker run answering with the daemon's conflict text naming a-b's container, asserted through the real per-server file, the attributed reader and tail_log. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(upstream): rename the fake-docker shell quoter — sandbox_linux_test.go already declares shellQuote under the linux tag Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(upstream): keep a real docker out of PATH for the fake-docker fixture Ubuntu runners ship /usr/bin/docker, and the resolver's PATH lookup won over the well-known-path seam, so the owned-container cells saw the real daemon's empty ps. The fixture now builds a PATH dir linking only sh, awk, printf and cat. Verified on ubuntu:24.04 with a decoy docker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(logs): skip the shared-file rotation fixture on Windows Two lumberjack sinks on one file cannot rotate there: the rename fails while the co-owner holds the file open, so the fixture premise never holds. The attributed reader under test is platform-neutral. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E review round 4 — sweep outcome records carry the read-back container owner The whole-manager sweeps (cleanupAllManagedContainers, ForceCleanupAllContainers) stamped container_owner only on their intent records; the stop/kill/rm outcome records — success and failure — named the container's id or name without it, and the kill loop threaded a bare id slice that dropped the owner altogether. Every record a sweep writes that names a container now carries the owner Docker reported for it at selection (Spec 105 FR-007 / research D8, D9). The kill loop iterates the selected rows directly; its guard was always true past the early return, so removing it is behaviour-preserving. Test: the fake docker gains a Running column (ps -q answers running rows) and a fail file (stop/kill/rm exit 1) so both the force-kill and the error branches fire; every main.log record whose field values carry the own container's id or name must carry container_owner, on both arms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E review round 5 — sweeps re-verify ownership at mutation time, disconnect-timeout records name the container only with evidence Codex round 5, docker chunk (Spec 105 FR-007 / D8, D9): 1. forceCleanupClient (disconnect timeout) named the tracked container id before ownership was verified and on every outcome without container_owner. core.Client.ForceRemoveTrackedContainerIfOwned now hands back the owner label it read at the rm -f; the manager's pre-verification intent names the server only, the removed / rm-failed outcomes carry the id with that owner, and a rejected or unverifiable container is never named by id. 2. The shutdown and emergency sweeps stopped, killed and removed containers on the ownership their initial docker ps established. Every mutation now goes through reverifyOwnedManagedContainer, which re-reads the name and label immediately before the stop/kill/rm and re-applies core.ContainerOwnedByAny: a container renamed or relabelled since the listing is left alone and the refusal recorded without its id or name; the container_owner on every record is the value read at mutation time. Tests: TestForceCleanupClient_NamesTheContainerOnlyWithOwnershipEvidence, TestSweeps_ReverifyOwnershipAtMutationTime (fake docker gains a post-listing fixture swap); TestForceRemoveTrackedContainerIfOwned_AppliesOwnership asserts the returned owner. Docs: research D9, docker-isolation.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E review round 6 — every Docker mutation re-verifies ownership through one helper Codex round 6 found the round-5 moment-of-mutation rule applied only in the manager sweeps: the core's image-fallback, name-pattern and pre-creation cleanups stopped / rm -f'd the rows a listing returned, the kill after a failed `docker stop` was a second mutation with no read at all, and the docker-logs monitor read the cidfile itself on its wait timeout and named that id with an executable `docker logs` command. - core.ContainerMutator.Mutate is now the single verify-then-mutate implementation: `docker ps -a --no-trunc --filter id=` immediately before the command, exact FULL-id match (every listing runs --no-trunc, so a replacement whose id extends the listed one cannot be admitted), the ownership predicate re-applied, the row read handed back as the only source of container_id/container_owner on the caller's records; a refusal names no id or name. - core: stopOwnedContainer takes an id and re-verifies before the stop AND before the escalating kill; ensureNoExistingContainers re-verifies per row (a/b vs a-b collision); the per-row listing records become a count. - monitoring: the docker-logs monitor never reads the cidfile; it names only the tracked, verified container (new Client.containerOwner) and records nothing but the timeout otherwise. - manager: the private reverify is deleted; the three sweep sites go through the same helper; the fake docker's --filter id= is a prefix match like docker's and the round-5 test gains the id-extending arm. - tests: swapFixtureAfterPs(n) + failVerbs on the core fake docker; TestDockerMutations_ReverifyOwnershipAtMutationTime (4 paths x 4 arms), TestDockerCleanup_PreCreationReverifiesEachRow_SlashVsDashCollision, TestDockerStopEscalation_ReverifiesBeforeKill, TestMonitorDockerLogs_NamesOnlyAVerifiedContainer. - docs: research D9 and docs/features/docker-isolation.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E review round 8 — cidfile refusals and container health checks name no id without ownership evidence Two MUST-FIX findings from codex round 7 (docker.go chunk; the logs chunk stayed clean and untouched): 1. trackCidfileContainer's err!=nil and !ok branches logged shortContainerID(containerID) into the main logger even though the container's ownership could not be verified or had failed the predicate — the same refusal rule mutateOwnedContainer already applies (server + reason only, never an id) was missing here. Both branches now log only the server name and the reason. 2. Manager.verifyContainerHealthy decided health from `docker inspect <stored-id>` alone. inspect answers by id regardless of name or label, so a container another Docker client relabelled or renamed after tracking still read back Running=true under the same id, and ForceReconnectAll treated it as healthy — skipping recovery for a container that was no longer canonically this server's. The check is now split into a pure verifyDockerContainerHealthy that re-establishes ownership through the same read+predicate ContainerMutator uses for every mutation (a new ContainerMutator.Verify, factored out of Mutate so both share one implementation) before trusting inspect: a container that fails the predicate now is reported unhealthy with no id in the record, and ForceReconnectAll's existing rebuild (RemoveServer + AddServer) both recovers and clears the stale tracked id/owner as a byproduct. A container ownership confirms is named, with container_owner from that same read. Tests: TestDockerCleanup_CidfileRefusal_MainLogRecordsNoID (not-owned and docker-read-failure arms) and TestVerifyDockerContainerHealthy_ReverifiesOwnershipBeforeInspect (relabelled, renamed, read-failure, unchanged-running, unchanged-stopped) via the fake docker shims, extended with an `inspect` branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E review round 9 — lifecycle log lines and diagnostics name a container only with evidence Two MUST-FIX findings from codex round 8 (Spec 105 D8): 1. Four lifecycle housekeeping log sites (Connect failure, connectStdio init failure, initialize() init failure, Disconnect by-ID/by-name) named the cached container id, or a merely GENERATED canonical name never observed from Docker, without container_owner. containerID is assigned only by trackCidfileContainer or the cidfile-timeout name-recovery fallback, both of which verify ownership via Docker's read-back before ever setting it and always pair it with containerOwner. A new dockerContainerLogFields helper is the single place all four sites build their fields from: nil when containerID is empty (so an unverified generated name never gets logged as evidence), otherwise container_id/container_name/container_owner from the tracked, already-verified state. 2. GetConnectionDiagnostics published the cached container id and inspected it by id alone, the same stale-ownership gap verifyDockerContainerHealthy closed for the manager's health path in round 8. It now routes through ContainerMutator.Verify first: a container that no longer passes the ownership predicate is reported as absent (container_running=false, no id), and only a container ownership confirms right now is published, with the owner read back at that same moment. Tests via the fake docker shim and a failing MCP transport, covering both arms at every site: TestDockerContainerLogFields (the helper), TestInitializeFailure_DockerCleanupLog, TestDisconnectWithContext_DockerCleanupLog, TestConnectStdioDirectDockerRun_ContainerEvidence (a real Connect -> connectStdio -> initialize chain for a direct `docker run` upstream), and TestGetConnectionDiagnostics_ReverifiesOwnershipBeforePublishing. All shown red on the pre-fix code. Swept every remaining container_id/container_name zap field across internal/upstream/core and manager.go; every other site already pairs with container_owner from a same-moment Docker read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(upstream): pin the direct-exec docker path in the fake-docker fixture On Linux the spawn keeps the login-shell wrap unless DOCKER_HOST or DOCKER_CONTEXT is already in the process env; the Landlock job has neither, so the fixture's poisoned SHELL was exec'd and the connection failed at start — before the initialization-failure lifecycle site the evidence test expects. Set DOCKER_HOST in the fixture so every job runs the shim through the same direct-exec path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E review round 11 — housekeeping counts and cidfile-recovery failure carry no unowned container evidence Two MUST-FIX findings from codex round 9 review (docker chunk; logs chunk clean): - Three housekeeping records (image cleanup, name-pattern cleanup, the pre-creation sweep's main-log record) published container_count without container_owner. D8 rule 3 treats a container count as container-subject evidence, so each now pairs the count with the label Docker reported on the listed rows (the pre-creation sweep's paired upstreamLogger record already did this). - The terminal cidfile-recovery failure named c.containerName — a generated name never read back from Docker — as orphaned. Under a suffix collision that name can currently belong to a colliding server, so the record now names only the server, matching the round-9 lifecycle fixes for every other generated-name-only state. Red tests first (docker_review_round11_test.go), all four failed pre-fix and pass post-fix. Full existing PR suite green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(scope): PR E review round 13 — HasDockerContainers applies canonical ownership, sweep counts are owner-grouped codex round 10 docker findings (.review-tmp/codex-r10-docker.txt), both confirmed by trace and both MUST-FIX per the maintainer's decision: 1. HasDockerContainers selected by the shared, copyable com.mcpproxy.managed / com.mcpproxy.instance labels alone, unlike the sweeps' listOwnedManagedContainers selection (label + canonical name via core.ContainerOwnedByAny). A foreign container that copies those labels, or an orphan of a server removed from config, therefore read as "still running" and drove the runtime/server shutdown path into its 15s cleanup-verification wait, a second force-clean, and a false "Some containers may still be running after force cleanup" report for a container mcpproxy neither started nor can act on (D9). Fix: readManagedContainers/listOwnedManagedContainers take an includeStopped bool (the same convention core.Client.listOwnedContainersFiltered already uses) so a caller can ask for the running-only `docker ps` real Docker defaults to. HasDockerContainers now calls listOwnedManagedContainers(ctx, false, ...) and reports len(owned) > 0 — the sweeps pass true, unchanged. 2. The shutdown sweep's "Found mcpproxy-managed containers to cleanup" and the emergency sweep's "Force removing managed containers" records published a bare aggregate count with no container_owner. A sweep can select containers belonging to more than one configured server, so the count could not be bound to a subject (D8). Fix: logOwnerGroupedCounts replaces the aggregate with one record per Docker-read owner, each carrying container_owner and its own count. Tests: TestHasDockerContainers_AppliesCanonicalOwnership (foreign-label, orphaned-owner, canonically-owned-running, canonically-owned-stopped, docker-unavailable) and TestSweepCounts_AreOwnerGrouped (two owners, two records) — both red before the fix. The manager's fake-docker shim gained `-a` handling (a plain `ps` now answers running rows only, matching real docker) to make the running/stopped distinction testable. Swept manager.go and internal/upstream/core for any other container count/ id/name field published without container_owner: none found — the one intentionally unpaired count (listOwnedManagedContainers' "Skipping containers..." Warn) counts rows that by definition have no established owner, per its existing comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(upstream): drop ownerless rejected-row count from container sweep PR E review round 14 — listOwnedManagedContainers still logged a bare "count" of rows that failed canonical ownership. Those rows' labels are untrusted (that's exactly why they were rejected), so no owner could be attributed to the tally, violating D8's rule that every container count must carry the owner it counts. Drop the log line entirely rather than fabricate an owner; nothing downstream consumes the field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scope): PR E review round 16 — container state from the Verify read, scoped log tail bounded by budget codex round 13 docker findings (.review-tmp/codex-r13-docker.txt), both confirmed by trace and both MUST-FIX per the maintainer's decision: 1. TOCTOU: GetConnectionDiagnostics and the manager's health check (verifyDockerContainerHealthy) each verified ownership with one ContainerMutator.Verify (docker ps) read, then issued a SEPARATE, later `docker inspect <id>` to decide running/status. Between the two, another Docker client could relabel or rename the container into a colliding server's namespace; the inspect then reported the NOW-FOREIGN container's state while the code kept attributing it to the original server. Fix: containerRowFormat gained a fourth field, {{.Status}} — docker ps's own human STATUS text — captured in the SAME docker ps -a read ContainerMutator.read already makes. ContainerRow gained a Status field and a Running() bool method deriving running state from Docker's own "Up" prefix convention (verified against a live daemon: docker ps --format has no .Running field at all, and .State is State.Status, not State.Running — a paused container shows State.Status="paused" but State.Running=true, and its docker ps STATUS text is "Up ... (Paused)", so the "Up" prefix carries the same information .State.Running would). Both call sites now derive running/status from the Verify read's row alone; the separate `docker inspect` calls are deleted entirely. 2. Timing-class (SC-005): ReadUpstreamServerLogTailAttributed scanned the shared log file from byte 0, filtering attributable lines, before taking the last N — so a scoped caller's response time was proportional to a hidden co-owner's entire earlier volume in the shared file, a response-time side channel the non-disclosing-refusal definition (status, body AND timing class; SC-005) forbids. Fix: the scan now Stats the file and Seeks to scopedBackwardStartOffset (at most scopedBackwardReadBudget, 16 MiB, bytes before EOF) before the unchanged filter-then-limit forward scan — readBoundedLine, recordAttributableTo and every D8 boundary/subject-evidence rule are untouched, only where the scan starts is new. Below the budget (the common case) this returns byte-identical results to before; past it, a request whose own recent records sit further back returns fewer than requested rather than reading further — bounded and fail-closed, never an error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scope): scope canonical container ownership to this mcpproxy instance (FR-007) Cross-model review (codex gpt-5.6-sol quota exhausted, fell back to codex exec) round 1 on PR #1284 found a BLOCKER: ownsContainer/ContainerOwnedByAny checked only the com.mcpproxy.server label and the canonical name, never the com.mcpproxy.instance label #1300 already stamps on every container. Two mcpproxy processes (distinct data dirs) that each configure a server with the same raw name would each pass the OTHER's container through the predicate — cleanupAllManagedContainers, ForceCleanupAllContainers, and every per-server core.Client stop/kill/rm path could stop, kill or rm a live sibling instance's container and log it as its own. Thread the instance label through the read/predicate chain instead: ownsContainer and ContainerOwnedByAny now take an instanceLabel and require it to equal core.GetInstanceID(); ownedContainer/ContainerRow/ managedContainer grew an Instance field populated from a new docker-ps --format column, and ContainerMutator.Owns grew a third parameter. Updated the Docker-side --filter on the per-server listing paths for the same belt-and-braces reason the server label already gets one. Test fixtures across docker_ownership_test.go, docker_mutation_reverify_test.go, docker_review_round11_test.go and manager_container_ownership_test.go that represent an OWNED container now carry this test process's own instance label (a withOwnInstance helper); added direct predicate-table cases for a missing/mismatched instance label, plus an integration case in TestSweeps_ReverifyOwnershipAtMutationTime proving both the shutdown and emergency sweep reject another live instance's same-named container. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scope): reject tab-corrupted docker-ps rows in ownership reads (FR-007) Cross-model review round 2 (codex, gpt-5.6-sol quota still exhausted) on the round-1 instance-scoping fix found a real HIGH: docker ps --format has no escaping for a label's own value, and the tab-delimited parsers (ContainerMutator.read, listOwnedContainersFiltered, manager's readManagedContainers) used a bounded or lenient split that would absorb an attacker-controlled trailing tab into the wrong field — a container whose Instance (or Owner) label value was literally "<real-id>\t<junk>" could read back as an exact match, with the junk silently folded into Status. All three now require the row to split into EXACTLY the expected field count; a row with an extra tab is rejected outright. Added TestContainerMutatorRead_RejectsEmbeddedTabInLabel (verified red against the prior bounded-SplitN code, green against the fix). Round 2 also repeated its round-1 BLOCKER as "not fully resolved" because Docker labels are unauthenticated metadata a Docker-capable actor can copy verbatim (no smuggling needed — they can just read the real instance id off any of this process's own containers). That is a genuine, but pre-existing and already-documented, characteristic of every label in this ownership model (ContainerOwnedByAny's own doc already says the same of the server label: "which any foreign container can copy") — an actor with Docker socket access is already host-privileged and out of FR-007's threat model, which is about canonical identification among mcpproxy's own legitimate containers, not authenticating against a host-level adversary. Documented that explicitly on containerInstanceLabel rather than attempting a cryptographic-labels redesign out of this fix's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scope): stop trimming docker-ps output before splitting into rows (FR-007) Cross-model review round 3 (codex, final round per this task's 3-round cap) on the round-2 exact-field-count fix found a real bypass: listOwnedContainersFiltered and manager's readManagedContainers both did strings.Split(strings.TrimSpace(output), "\n") before splitting each line on tabs. Instance is the LAST templated field in both formats, so a label value an attacker controls ending in its own literal tab renders as a trailing tab on the last line of output — indistinguishable from ordinary trailing whitespace, which TrimSpace (it treats \t as whitespace) silently strips, collapsing the row back to the expected field count and admitting the forged suffix as if the extra tab had never been there. Both now split the raw output directly (no TrimSpace) and drop only genuinely empty lines (docker's own trailing newline), so the attacker's tab stays exactly where they put it and the exact-count check added in the previous round still rejects the row. Round 3 also confirmed the round-1/round-2 BLOCKER debate is resolved: the reviewer agrees the "Docker labels aren't cryptographically unforgeable" concern is a pre-existing, already-documented characteristic of this ownership model (not a regression from adding instance-scoping) and should not independently block this PR. This is the reviewer's 3rd round on this PR; per the task's round cap this is the last review round taken — the fix above was verified by build, vet (both editions) and the full internal/upstream race suite, but was not re-submitted for a 4th review round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(scope): fail whole docker-ps read closed on any malformed row (FR-007) Cross-model review round 4 (codex; coordinator raised the round cap to this repo's actual 10-round policy) found a deeper bypass of the round-3 fix: a label value can contain a literal NEWLINE, not just a tab. Since docker ps renders exactly one line per container, an attacker's own container whose Owner (or Instance) label is "junk\n<forged-id>\tmcpproxy-a-wxyz\ta\t<real-instance>" splits Docker's single row into two lines — a short, malformed first fragment and a second fragment that, taken alone, looks like a complete, independently well-formed row for a container id/name/owner/instance of the attacker's choosing. The round-2/3 "exact field count" check only rejected the individual malformed line, then kept scanning and would still accept the forged sibling. Fixed by failing the WHOLE read closed the moment any line doesn't parse to the expected field count, in all three parsers (core.Client. listOwnedContainersFiltered, core.ContainerMutator.read, manager. readManagedContainers) — a single malformed line means line boundaries in this output can no longer be trusted at all, so nothing from that read is used, rather than keeping whichever rows still look well-formed. Also fixed read()'s remaining TrimSpace-before-split inconsistency the reviewer flagged (finding B), matching the other two parsers. Added TestContainerMutatorRead_RejectsNewlineSplicedRow, verified red against the round-3 "skip the bad line, keep scanning" behavior and green against this fix. Residual, bounded risk (documented, not fixed further this round): a maximally sophisticated attacker who fully controls BOTH the Owner and Instance label values of their own container (only possible on cleanupAllManagedContainers' deliberately broad, instance-unconstrained listing) can in principle balance tab-padding across both resulting fragments so neither is individually malformed. Even so, any resulting mutation still goes through a fresh, independent per-id ContainerMutator.Verify before acting, which only ever lets mcpproxy act on a container the attacker's own docker-socket access already lets them create/stop/remove directly — never an innocent third party's container. Closing this residual completely would require switching label reads to JSON-encoded output (`docker inspect --format '{{json .Config.Labels}}'`) instead of tab-delimited `--format` templates, which is a larger rewrite flagged here rather than attempted this round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Summary
GetInstanceID()persisted the mcpproxy instance ID to a singlefilepath.Join(os.TempDir(), "mcpproxy-instance-id")file, which is shared by every process a user runs on a host — so any two concurrent mcpproxy processes (a scratch dev instance alongside the main app, two separate installs) silently got the SAME instance ID.com.mcpproxy.instanceDocker container-ownership label relied on byinternal/upstream/manager.go's container cleanup, and by the ownership filter added in #1298 — for two processes on the same host, though cross-host-shared-daemon cases were unaffected.cfg.DataDir) instead, since concurrent instances on one host already require distinct data dirs (BBolt takes an exclusive lock onconfig.db).core.SetInstanceDataDir()is called once fromupstream.NewManager, the common construction point for every entry point (serve, tray,mcpproxy call,mcpproxy code). Falls back to a fresh per-process UUID if no data dir is known at labeling time.instance-idfile yet, adopts the legacy sharedos.TempDir()file if present (so pre-upgrade containers stay cleanable by whichever instance starts first), then deletes it so no other data dir can re-adopt the same ID afterward.Why
Root-caused during an opencode gpt-5.6-sol cross-review of #1298, which added the container-ownership filter but noted
GetInstanceID()'s host-wide-shared-file gap as a pre-existing, out-of-scope limitation. This PR fixes that root cause so the filter (once #1298 merges) actually distinguishes concurrent same-host instances, not just same-instance-ID-across-hosts cases.Test plan
go build ./...go test ./internal/upstream/...(incl.-raceoninternal/upstream/core)resolveInstanceID): uniqueness per data dir, persistence across restarts, no-data-dir fallback, one-shot legacy-file adoption (and non-re-adoption by a second data dir)GetInstanceID()/SetInstanceDataDir()singleton across genuinely separate OS processesgolangci-lint run --config .github/.golangci.yml ./internal/upstream/...— only 2 pre-existing findings, unrelated to these files#1298'sSourceResolver.SetInstanceIDownership filter, once merged/rebased on top of this — it callscore.GetInstanceID()unchanged, so it should pick up per-instance uniqueness automatically, but wasn't re-tested here since that PR isn't merged tomainyet🤖 Generated with Claude Code