diff --git a/cmd/mcpproxy/logdir.go b/cmd/mcpproxy/logdir.go new file mode 100644 index 000000000..7bb4602cd --- /dev/null +++ b/cmd/mcpproxy/logdir.go @@ -0,0 +1,65 @@ +package main + +import ( + "os" + "path/filepath" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// defaultDataDirPath returns the default data directory (/.mcpproxy), +// mirroring internal/config's default resolution. It is used to decide whether +// the resolved data dir is "non-default" for log co-location. On failure to +// resolve the home dir it returns config.DefaultDataDir, which will simply not +// match any absolute data dir and leave logs at the OS-standard location. +func defaultDataDirPath() string { + home, err := os.UserHomeDir() + if err != nil { + return config.DefaultDataDir + } + return filepath.Join(home, config.DefaultDataDir) +} + +// resolveServeLogDir decides the log directory for the `serve` command. +// +// Precedence: +// 1. explicit --log-dir flag (explicitLogDir) — always wins. +// 2. a log dir already set in the loaded config (configLogDir). +// 3. for a NON-default data dir, co-locate logs under /logs. +// 4. otherwise "" — meaning the OS-standard location resolved by +// internal/logs.GetLogDir (e.g. ~/Library/Logs/mcpproxy on macOS). +// +// Step 3 is the fix for MCP-2250: Go integration tests, e2e scripts, and the +// FE/QA Playwright harnesses all run `mcpproxy serve` with a custom data dir +// (temp dirs, ./test-data, /tmp/mcpproxy-*) but, because the log dir was +// derived purely from $HOME, every one of them appended to the SAME shared +// prod log at ~/Library/Logs/mcpproxy/main.log. A reader of that file saw +// dozens of fast boots interleaved and mistook it for the core restarting +// ~every 10s. Co-locating logs with a non-default data dir keeps those runs +// self-contained and leaves the real prod log clean. The default data dir +// (~/.mcpproxy) is unchanged: it still logs to the OS-standard location so the +// tray and documented paths keep working. +func resolveServeLogDir(explicitLogDir, configLogDir, dataDir, defaultDataDir string) string { + if explicitLogDir != "" { + return explicitLogDir + } + if configLogDir != "" { + return configLogDir + } + if dataDir != "" && !sameDir(dataDir, defaultDataDir) { + return filepath.Join(dataDir, "logs") + } + return "" +} + +// sameDir reports whether two paths refer to the same directory, comparing +// their cleaned absolute forms so that relative/aliased spellings of the +// default data dir are still recognized as the default. +func sameDir(a, b string) bool { + absA, errA := filepath.Abs(a) + absB, errB := filepath.Abs(b) + if errA != nil || errB != nil { + return filepath.Clean(a) == filepath.Clean(b) + } + return absA == absB +} diff --git a/cmd/mcpproxy/logdir_test.go b/cmd/mcpproxy/logdir_test.go new file mode 100644 index 000000000..a7703a67c --- /dev/null +++ b/cmd/mcpproxy/logdir_test.go @@ -0,0 +1,91 @@ +package main + +import ( + "path/filepath" + "testing" +) + +// TestResolveServeLogDir documents the precedence that keeps non-default-data-dir +// runs (Go tests, e2e scripts, FE/QA harnesses) from polluting the shared prod +// log at ~/Library/Logs/mcpproxy/main.log — the root cause of the phantom +// "core restarts every 10s" reports in MCP-2250. +func TestResolveServeLogDir(t *testing.T) { + const defaultDataDir = "/home/u/.mcpproxy" + + cases := []struct { + name string + explicit string + configLog string + dataDir string + defaultData string + want string + }{ + { + name: "explicit --log-dir always wins", + explicit: "/custom/logs", + configLog: "/cfg/logs", + dataDir: "/tmp/test-123", + defaultData: defaultDataDir, + want: "/custom/logs", + }, + { + name: "config Logging.LogDir wins when no flag", + explicit: "", + configLog: "/cfg/logs", + dataDir: "/tmp/test-123", + defaultData: defaultDataDir, + want: "/cfg/logs", + }, + { + name: "default data dir keeps OS-standard log dir (empty => GetLogDir)", + explicit: "", + configLog: "", + dataDir: defaultDataDir, + defaultData: defaultDataDir, + want: "", + }, + { + name: "non-default data dir co-locates logs under /logs", + explicit: "", + configLog: "", + dataDir: "/tmp/mcpproxy-test-Foo", + defaultData: defaultDataDir, + want: filepath.Join("/tmp/mcpproxy-test-Foo", "logs"), + }, + { + name: "relative non-default data dir (harness ./test-data) co-locates", + explicit: "", + configLog: "", + dataDir: "./test-data", + defaultData: defaultDataDir, + want: filepath.Join("test-data", "logs"), + }, + { + name: "absolute spelling of default data dir is treated as default", + explicit: "", + configLog: "", + dataDir: "/home/u/../u/.mcpproxy", + defaultData: defaultDataDir, + want: "", + }, + { + name: "empty data dir is a no-op (OS-standard)", + explicit: "", + configLog: "", + dataDir: "", + defaultData: defaultDataDir, + want: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := resolveServeLogDir(tc.explicit, tc.configLog, tc.dataDir, tc.defaultData) + // Compare cleaned, since the helper may return a joined relative path. + if filepath.Clean(got) != filepath.Clean(tc.want) { + t.Fatalf("resolveServeLogDir(%q,%q,%q,%q) = %q, want %q", + tc.explicit, tc.configLog, tc.dataDir, tc.defaultData, got, tc.want) + } + }) + } +} diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 163e67b2c..ca6a7ab05 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -466,10 +466,12 @@ func runServer(cmd *cobra.Command, _ []string) error { } } - // Override log directory if specified - if cmdLogDir != "" { - cfg.Logging.LogDir = cmdLogDir - } + // Resolve the log directory. An explicit --log-dir wins; otherwise a + // non-default data dir co-locates logs under /logs so that + // tests/e2e/harness `serve` runs do not pollute the shared OS-standard + // prod log (root cause of the phantom "core restarts every 10s" in + // MCP-2250). The default data dir keeps the OS-standard location. + cfg.Logging.LogDir = resolveServeLogDir(cmdLogDir, cfg.Logging.LogDir, cfg.DataDir, defaultDataDirPath()) // Setup logger with new logging system logger, err := logs.SetupLogger(cfg.Logging) diff --git a/docs/logging.md b/docs/logging.md index e65381aef..a8b302e89 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -73,6 +73,13 @@ The logging system automatically selects the appropriate directory based on your - **Location**: `~/.mcpproxy/logs/` - **Used when**: OS detection fails or standard directories are inaccessible +### Custom data directory +- **Location**: `/logs/` +- **Used when**: the resolved data directory is **not** the default (`~/.mcpproxy`) — i.e. you ran with a custom `--data-dir` flag or a `data_dir` set in the config file — **and** no explicit `--log-dir` was given. +- **Rationale**: co-locating logs with a custom data directory keeps non-default runs (integration tests, e2e scripts, throwaway/QA instances) self-contained so they never write into the shared OS-standard log (`~/Library/Logs/mcpproxy/main.log`). Mixing many short-lived `serve` processes into that single shared file previously made the log unreadable and produced misleading "the core restarts every ~10s" signals. +- **Override**: an explicit `--log-dir` always takes precedence; the default data directory (`~/.mcpproxy`) is unaffected and still logs to the OS-standard location above. +- **Resolution order**: `--log-dir` flag → `logging.log_dir` in config → `/logs` (non-default data dir) → OS-standard location. + ## Configuration Options ### Log Levels @@ -383,4 +390,4 @@ The logging system follows established standards for each operating system: - **Windows**: [Application Data Guidelines](https://docs.microsoft.com/en-us/windows/win32/shell/knownfolderid) - **Linux**: [XDG Base Directory Specification](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html) -This ensures that logs are stored in the expected locations for each platform, making them easy to find and manage. \ No newline at end of file +This ensures that logs are stored in the expected locations for each platform, making them easy to find and manage. diff --git a/internal/logs/e2e_test.go b/internal/logs/e2e_test.go index 89c7a498c..38c35c04d 100644 --- a/internal/logs/e2e_test.go +++ b/internal/logs/e2e_test.go @@ -336,12 +336,21 @@ func TestE2E_MCPProxyWithLogging(t *testing.T) { // Wait for the command to finish _ = cmd.Wait() - // Check if log file was created - logFilePath, err := GetLogFilePath("mcpproxy-e2e-binary.log") - require.NoError(t, err) - + // Because the config sets a non-default data_dir (tempDir) and no + // --log-dir was given, logs are co-located under /logs rather + // than the shared OS-standard location (MCP-2250). This keeps test/e2e + // runs from polluting the real ~/Library/Logs/mcpproxy/main.log. + logFilePath := filepath.Join(tempDir, "logs", "mcpproxy-e2e-binary.log") _, err = os.Stat(logFilePath) - assert.NoError(t, err, "Log file should be created by mcpproxy binary") + assert.NoError(t, err, "Log file should be created under /logs by mcpproxy binary") + + // The shared OS-standard location must NOT receive this run's log. + osStdPath, osErr := GetLogFilePath("mcpproxy-e2e-binary.log") + require.NoError(t, osErr) + if _, statErr := os.Stat(osStdPath); statErr == nil { + t.Errorf("custom data_dir run leaked its log into the shared OS-standard dir: %s", osStdPath) + os.Remove(osStdPath) + } // Read and verify log content if err == nil { @@ -351,9 +360,6 @@ func TestE2E_MCPProxyWithLogging(t *testing.T) { contentStr := string(content) assert.Contains(t, contentStr, "Starting mcpproxy", "Log should contain startup message") assert.Contains(t, contentStr, "Log directory configured", "Log should contain directory info") - - // Clean up - os.Remove(logFilePath) } }