Skip to content

fix(logs): isolate non-default data-dir logs to <data-dir>/logs (MCP-2255)#657

Merged
Dumbris merged 1 commit into
mainfrom
fix/mcp2250-isolate-test-logs
Jun 14, 2026
Merged

fix(logs): isolate non-default data-dir logs to <data-dir>/logs (MCP-2255)#657
Dumbris merged 1 commit into
mainfrom
fix/mcp2250-isolate-test-logs

Conversation

@Dumbris

@Dumbris Dumbris commented Jun 14, 2026

Copy link
Copy Markdown
Member

Summary

When mcpproxy serve runs with a non-default --data-dir and no explicit --log-dir,
co-locate logs under <data-dir>/logs instead of the shared OS-standard location
(~/Library/Logs/mcpproxy on macOS, ~/.local/share/mcpproxy/logs on Linux).

Why

Every process that runs mcpproxy serve — Go integration tests, e2e scripts,
FE/QA Playwright harnesses, and the real production core — was appending to the
same shared main.log. In a 72-minute window, ~42 process boots were logged,
but only 2 were the real production core. The other 40 were test/harness runs.
A reader of that shared log file would see dozens of fast interleaved boots and
conclude "the core restarts every ~10s" — the false-alarm chain MCP-2207 → MCP-2249 → MCP-2250.

Resolution order

--log-dir flag → config logging.log_dir<data-dir>/logs (non-default data dir only)
→ OS-standard location.

Changes

File Change
cmd/mcpproxy/logdir.go NEWresolveServeLogDir() + helpers
cmd/mcpproxy/logdir_test.go NEW — table-driven tests for all precedence cases
cmd/mcpproxy/main.go Wire resolveServeLogDir() into runServer()
docs/logging.md Document the custom data-dir log location
internal/logs/e2e_test.go Assert logs go to <data-dir>/logs; verify no leak into shared OS dir

Testing

  • make build — passes
  • go test ./cmd/mcpproxy/... -race -count=1 — passes
  • go test ./internal/logs/... -race -count=1 — passes
  • ./scripts/run-linter.sh — 0 issues

Gates

  • Do not merge — human pre-merge gate (Gate 3)

Related: MCP-2255

The phantom "core restarts every ~10s" reports (MCP-2250, split from
MCP-2207) were a logging artifact, not a real restart loop. `serve`
enables file logging by default and derives the log dir purely from
$HOME (internal/logs.GetLogDir), ignoring --data-dir. So every
`mcpproxy serve` run — Go integration tests (temp data-dirs), e2e
scripts, FE/QA Playwright harnesses, AND the real prod core — appended
to the same ~/Library/Logs/mcpproxy/main.log. A reader of that shared
file saw dozens of fast, interleaved boots and concluded the core was
restarting every ~10s. Forensics on a 72-min window show only 2 real
prod-core boots (data_dir ~/.mcpproxy) among ~42, and every shutdown
was reason="signal" (no panics/self-restarts).

Fix: when the resolved data dir is non-default and no explicit
--log-dir was given, co-locate logs under <data-dir>/logs. The default
data dir (~/.mcpproxy) is unchanged and still logs to the OS-standard
location, so the tray and documented paths keep working. An explicit
--log-dir still wins.

- Add resolveServeLogDir helper (cmd/mcpproxy/logdir.go) with unit tests
  covering the full precedence: --log-dir > config log_dir >
  <data-dir>/logs (non-default) > OS-standard.
- Wire it into runServer (cmd/mcpproxy/main.go).
- Update TestE2E_MCPProxyWithLogging to assert the binary writes to
  <data-dir>/logs and does NOT leak into the shared OS-standard dir.
- Document the resolution order in docs/logging.md.

Related MCP-2250

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying mcpproxy-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8e61b03
Status: ✅  Deploy successful!
Preview URL: https://f1f1d7c3.mcpproxy-docs.pages.dev
Branch Preview URL: https://fix-mcp2250-isolate-test-log.mcpproxy-docs.pages.dev

View logs

@Dumbris

Dumbris commented Jun 14, 2026

Copy link
Copy Markdown
Member Author

Closing — the authorizing issue MCP-2250 was cancelled by the orchestrator as a duplicate of MCP-2249 (done) with the premise ("prod core restarts ~every 10s") invalidated: the signal was E2E/test instances polluting the shared ~/Library/Logs/mcpproxy/main.log, not a real prod restart loop. Standing down per that decision.

Note for whoever triages follow-ups: this PR does not depend on the invalidated premise. It implements the actual hygiene fix for the root phenomenon everyone observed — routing logs of a non-default data-dir to <data-dir>/logs so tests/e2e/harness serve runs stop polluting the shared prod log dir (the thing that spawned MCP-2207 → MCP-2249 → MCP-2250 confusion). It was locally green (go test ./cmd/mcpproxy/... ./internal/logs/... -race, golangci-lint v2 clean).

Branch fix/mcp2250-isolate-test-logs is preserved — reopen or revive under a fresh issue if the log-isolation hygiene improvement is wanted. Not merging on my own initiative since the owning issue is cancelled.

@Dumbris Dumbris closed this Jun 14, 2026
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 60.00000% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/mcpproxy/logdir.go 63.15% 6 Missing and 1 partial ⚠️
cmd/mcpproxy/main.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Dumbris Dumbris reopened this Jun 14, 2026
@Dumbris Dumbris changed the title fix(logs): isolate non-default data-dir logs to <data-dir>/logs (MCP-2250) fix(logs): isolate non-default data-dir logs to <data-dir>/logs (MCP-2255) Jun 14, 2026
@Dumbris

Dumbris commented Jun 14, 2026

Copy link
Copy Markdown
Member Author

Re-opened after revival from closed PR #657 (previously linked to MCP-2250, now under MCP-2255). Branch fix/mcp2250-isolate-test-logs at commit 8e61b030. All local checks pass. CI re-running.

@github-actions

Copy link
Copy Markdown

📦 Build Artifacts

Workflow Run: View Run
Branch: fix/mcp2250-isolate-test-logs

Available Artifacts

  • archive-darwin-amd64 (28 MB)
  • archive-darwin-arm64 (25 MB)
  • archive-linux-amd64 (16 MB)
  • archive-linux-arm64 (14 MB)
  • archive-windows-amd64 (28 MB)
  • archive-windows-arm64 (24 MB)
  • frontend-dist-pr (0 MB)
  • installer-dmg-darwin-amd64 (21 MB)
  • installer-dmg-darwin-arm64 (19 MB)

How to Download

Option 1: GitHub Web UI (easiest)

  1. Go to the workflow run page linked above
  2. Scroll to the bottom "Artifacts" section
  3. Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 27490919768 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

@Dumbris

Dumbris commented Jun 14, 2026

Copy link
Copy Markdown
Member Author

CEO Code Review — MCP-2265

LGTM. Clean, well-scoped fix for MCP-2255.

  • logdir.go — Pure resolveServeLogDir function with clear 4-step precedence. sameDir using filepath.Abs correctly handles relative/aliased paths.
  • logdir_test.go — 7 cases cover the full matrix including edge cases (relative path, ../ aliasing, empty data dir). Solid coverage.
  • main.go — Minimal one-line swap. Correct.
  • e2e_test.go — Negative assertion (prod log must NOT receive the run) is exactly right to prevent regression.
  • docs/logging.md — Resolution order is accurate and the rationale is clear.

No issues found. Ready to merge. (Self-approval blocked by GitHub; reviewer should merge.)

@Dumbris

Dumbris commented Jun 14, 2026

Copy link
Copy Markdown
Member Author

CEO Review (MCP-2274) — APPROVED

Log-dir isolation logic is correct and well-tested. The fix cleanly addresses the root cause of MCP-2250/MCP-2255.

What's good:

  • resolveServeLogDir has clear 4-level precedence: explicit flag → config → non-default data dir co-location → OS standard
  • sameDir correctly normalises via filepath.Abs — the aliased-path edge case is covered
  • 7 table-driven unit tests cover all branches including path aliasing
  • e2e test now asserts both the new location (<data-dir>/logs) AND that the shared OS-standard path was NOT written to
  • Docs updated with resolution-order table and rationale

No issues found. Ready to merge.

@Dumbris

Dumbris commented Jun 14, 2026

Copy link
Copy Markdown
Member Author

Review: ACCEPT ✅

Reviewed by BackendEngineer (CodexReviewer was unavailable — usage limits). Verdict: ACCEPT.

Summary

Clean, well-scoped fix for MCP-2250: non-default data_dir serve runs (Go integration tests, e2e scripts, FE/QA Playwright harnesses) were all appending to the shared OS-standard prod log (~/Library/Logs/mcpproxy/main.log), which made the file look like the core was restarting ~every 10s. The fix co-locates logs under <data-dir>/logs for non-default data dirs.

Correctness

  • Resolution precedence (resolveServeLogDir) is sound: --log-dir flag → logging.log_dir config → <data-dir>/logs (non-default) → "" (OS-standard via GetLogDir).
  • Default path preserved: --data-dir flag default is "", which the loader resolves to ~/.mcpproxy (absolute). defaultDataDirPath() produces the same filepath.Join(home, ".mcpproxy"), so sameDir matches → default invocations return "" → OS-standard location unchanged. The tray and documented log paths keep working.
  • Edge cases covered: aliased/relative spellings of the default dir (/home/u/../u/.mcpproxy) normalize via filepath.Abs; empty dataDir is a no-op; relative harness dirs (./test-data) co-locate correctly.
  • sameDir falls back to filepath.Clean comparison if Abs fails — graceful.

Tests (ran locally at head 8e61b030)

  • go test ./cmd/mcpproxy/ -run TestResolveServeLogDirPASS (all 7 precedence cases).
  • go test ./internal/logs/ -run TestE2E_MCPProxyWithLogging (built binary) → PASS. The updated e2e asserts the log lands under <data-dir>/logs and that the shared OS-standard path does not receive the run's log — a strong regression guard for the exact MCP-2250 leak.

Docs

docs/logging.md updated with the new "Custom data directory" location, rationale, override behavior, and full resolution order. Meets ENG-9.

CI

All required checks green, including qa-gate (SUCCESS), Unit Tests, Cross-Platform Logging Tests (ubuntu/macos/windows), E2E, Lint, CodeQL.

Note (non-blocking)

This is an intentional behavior change for real users who set a custom data_dir in config: their logs now go to <data-dir>/logs instead of the OS-standard location. This is documented and arguably more intuitive (self-contained data dir). No action needed.

On ACCEPT the Gatekeeper sweep will approve. Human merges at the pre-merge gate.

@mcpproxy-gatekeeper mcpproxy-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gatekeeper approval — Codex review verdict: ACCEPT.

This approval is posted automatically by the MCPProxy Gatekeeper App on behalf of the Codex reviewer (verdict of record lives in the Paperclip review thread). Author≠approver satisfied; QA + CI gates enforced separately.

Auto-approved per Model B (MCP-1249).

@Dumbris Dumbris merged commit 8f39779 into main Jun 14, 2026
73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants