From 91200b1a48bececf5634719a2a7b64f1a0898db6 Mon Sep 17 00:00:00 2001 From: Art Leo Date: Fri, 21 Aug 2026 11:31:15 +1000 Subject: [PATCH 1/5] Fix proxy exiting 0 on fatal startup errors main() logged fatal errors (config parse failure, sentry setup failure, ListenAndServe bind failure, proxy.Close failure) via log.Println(err) followed by a bare return, which lets the process exit normally with code 0. Integrations that gate on exit code (e.g. github/codeql-action) treated this as success even when the proxy never started, e.g. on "address already in use". Replace each of those branches with log.Fatal(err), which logs then calls os.Exit(1). This matches the existing convention already used elsewhere in this codebase for fatal setup errors (proxy.go's setCA and cache.New failures, logging.go's log file open failure). Add main_test.go covering the changed exit-code paths using the standard Go "helper process" pattern (TestMain re-execs the test binary via os.Args[0] under an env var to run the real main()): - bind-address-in-use now exits non-zero (the reported defect) - graceful SIGTERM shutdown still exits 0 (regression guard) - invalid config path exits non-zero Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- main.go | 12 ++--- main_test.go | 150 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 8 deletions(-) create mode 100644 main_test.go diff --git a/main.go b/main.go index 1d63246..7429b69 100644 --- a/main.go +++ b/main.go @@ -44,14 +44,12 @@ func main() { cfg, err := config.Parse(*configPath) if err != nil { - log.Println(err) - return + log.Fatal(err) } sentry, err := setupSentry() if err != nil { - log.Println(err) - return + log.Fatal(err) } envSettings := config.ProxyEnvSettings{ @@ -96,13 +94,11 @@ func main() { log.Printf("Listening (%s)", *addr) if err := server.ListenAndServe(); err != http.ErrServerClosed { - log.Println(err) - return + log.Fatal(err) } if err := proxy.Close(); err != nil { - log.Println(err) - return + log.Fatal(err) } } diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..fa4d33f --- /dev/null +++ b/main_test.go @@ -0,0 +1,150 @@ +package main + +import ( + "bytes" + "encoding/json" + "errors" + "net" + "os" + "os/exec" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dependabot/proxy/internal/config" +) + +// helperProcessEnv, when set to "1" in the environment, tells TestMain to run +// the real main() instead of the test suite. This lets tests re-exec the +// already-compiled test binary as a standalone proxy process and observe its +// real exit code - something that can't be done in-process once main() calls +// log.Fatal/os.Exit. This is the same "helper process" technique the Go +// standard library uses to test os.Exit/signal behavior (see os/exec and +// os/signal tests). +const helperProcessEnv = "PROXY_HELPER_PROCESS" + +func TestMain(m *testing.M) { + if os.Getenv(helperProcessEnv) == "1" { + main() + return + } + os.Exit(m.Run()) +} + +// runHelperProcess builds an *exec.Cmd that re-executes this test binary as a +// standalone proxy process (via the TestMain hook above), passing args as +// command-line flags and stdin as its stdin. +func runHelperProcess(t *testing.T, args []string, stdin string) *exec.Cmd { + t.Helper() + cmd := exec.Command(os.Args[0], args...) //nolint:gosec // args are test-controlled, not user input + cmd.Env = append(os.Environ(), helperProcessEnv+"=1") + cmd.Stdin = strings.NewReader(stdin) + return cmd +} + +// minimalConfigJSON returns a valid proxy config (with a working MITM CA) +// serialized as JSON, suitable for feeding via stdin so the helper process +// can get all the way to server.ListenAndServe(). +func minimalConfigJSON(t *testing.T) string { + t.Helper() + cfg := config.Config{CA: testCA()} + b, err := json.Marshal(cfg) + require.NoError(t, err) + return string(b) +} + +// freeAddr returns a "host:port" address that is free at the time of the +// call by briefly binding to port 0 and releasing it. +func freeAddr(t *testing.T) string { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := l.Addr().String() + require.NoError(t, l.Close()) + return addr +} + +// exitCodeFromWaitErr extracts the process exit code from the error returned +// by exec.Cmd.Wait()/Run(). A nil error means exit code 0. +func exitCodeFromWaitErr(t *testing.T, err error) int { + t.Helper() + if err == nil { + return 0 + } + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return exitErr.ExitCode() + } + t.Fatalf("helper process did not exit normally: %v", err) + return -1 +} + +// TestListenAndServe_AddressInUse_ExitsNonZero reproduces the reported +// defect: the proxy previously logged the bind error and exited 0, causing +// integrations that gate on exit code (e.g. github/codeql-action) to +// silently proceed as if the proxy were listening. +func TestListenAndServe_AddressInUse_ExitsNonZero(t *testing.T) { + l, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer func() { + require.NoError(t, l.Close()) + }() + addr := l.Addr().String() + + cmd := runHelperProcess(t, []string{"-addr=" + addr, "-config=-"}, minimalConfigJSON(t)) + var stderr bytes.Buffer + cmd.Stderr = &stderr + + runErr := cmd.Run() + + code := exitCodeFromWaitErr(t, runErr) + assert.NotEqual(t, 0, code, "expected non-zero exit code when the listen address is already in use, got 0 (output: %s)", stderr.String()) + assert.Contains(t, stderr.String(), "address already in use") +} + +// TestGracefulShutdown_ExitsZero is a regression guard: a clean shutdown via +// SIGTERM (the normal operational path) must still exit 0 after the fix. +func TestGracefulShutdown_ExitsZero(t *testing.T) { + addr := freeAddr(t) + + cmd := runHelperProcess(t, []string{"-addr=" + addr, "-config=-"}, minimalConfigJSON(t)) + var stderr bytes.Buffer + cmd.Stderr = &stderr + require.NoError(t, cmd.Start()) + + // Note: stderr is not safe to read here - it's being written to + // concurrently by the still-running subprocess, so the failure message + // below intentionally omits its contents (only safe to read after + // cmd.Wait() below). + require.Eventually(t, func() bool { + conn, dialErr := net.DialTimeout("tcp", addr, 100*time.Millisecond) + if dialErr != nil { + return false + } + _ = conn.Close() + return true + }, 5*time.Second, 50*time.Millisecond, "proxy did not start listening in time") + + require.NoError(t, cmd.Process.Signal(syscall.SIGTERM)) + + waitErr := cmd.Wait() + code := exitCodeFromWaitErr(t, waitErr) + assert.Equal(t, 0, code, "expected graceful shutdown to exit 0 (output: %s)", stderr.String()) +} + +// TestInvalidConfigPath_ExitsNonZero covers the config.Parse error branch +// using the same helper-process harness. +func TestInvalidConfigPath_ExitsNonZero(t *testing.T) { + cmd := runHelperProcess(t, []string{"-config=/nonexistent/path/definitely-missing.json"}, "") + var stderr bytes.Buffer + cmd.Stderr = &stderr + + runErr := cmd.Run() + + code := exitCodeFromWaitErr(t, runErr) + assert.NotEqual(t, 0, code, "expected non-zero exit code for an invalid config path, got 0 (output: %s)", stderr.String()) +} From f9cdb6ec403a7536ec6422e32ee331cfc8c42de1 Mon Sep 17 00:00:00 2001 From: Art Leo Date: Fri, 21 Aug 2026 11:44:23 +1000 Subject: [PATCH 2/5] Narrow fix scope to ListenAndServe only Revert the log.Fatal change for config.Parse, setupSentry, and proxy.Close - we do not know whether their silent exit-0 behavior on error was intentional, so leave them untouched. Only the server.ListenAndServe() bind-failure branch (the reported defect) now exits non-zero via log.Fatal. Also remove TestInvalidConfigPath_ExitsNonZero, which exercised the now-reverted config.Parse branch. The ListenAndServe-focused tests (address-in-use, graceful shutdown) remain. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- main.go | 9 ++++++--- main_test.go | 13 ------------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/main.go b/main.go index 7429b69..5cfa026 100644 --- a/main.go +++ b/main.go @@ -44,12 +44,14 @@ func main() { cfg, err := config.Parse(*configPath) if err != nil { - log.Fatal(err) + log.Println(err) + return } sentry, err := setupSentry() if err != nil { - log.Fatal(err) + log.Println(err) + return } envSettings := config.ProxyEnvSettings{ @@ -98,7 +100,8 @@ func main() { } if err := proxy.Close(); err != nil { - log.Fatal(err) + log.Println(err) + return } } diff --git a/main_test.go b/main_test.go index fa4d33f..6005ade 100644 --- a/main_test.go +++ b/main_test.go @@ -135,16 +135,3 @@ func TestGracefulShutdown_ExitsZero(t *testing.T) { code := exitCodeFromWaitErr(t, waitErr) assert.Equal(t, 0, code, "expected graceful shutdown to exit 0 (output: %s)", stderr.String()) } - -// TestInvalidConfigPath_ExitsNonZero covers the config.Parse error branch -// using the same helper-process harness. -func TestInvalidConfigPath_ExitsNonZero(t *testing.T) { - cmd := runHelperProcess(t, []string{"-config=/nonexistent/path/definitely-missing.json"}, "") - var stderr bytes.Buffer - cmd.Stderr = &stderr - - runErr := cmd.Run() - - code := exitCodeFromWaitErr(t, runErr) - assert.NotEqual(t, 0, code, "expected non-zero exit code for an invalid config path, got 0 (output: %s)", stderr.String()) -} From 0acbe2c1eea90a8485e9a5842341ebdebb802f79 Mon Sep 17 00:00:00 2001 From: Art Leo Date: Fri, 21 Aug 2026 11:51:46 +1000 Subject: [PATCH 3/5] Address Copilot review feedback on test portability - Assert the platform-independent "listen tcp : bind:" prefix (generated by Go's net package) instead of the OS-specific error text, which differs on Windows (e.g. "Only one usage of each socket address..."). - Skip the SIGTERM-based graceful shutdown assertion on Windows, where os.Process.Signal(syscall.SIGTERM) cannot deliver a graceful shutdown signal (it returns os.ErrProcessDone/EWINDOWS instead). The helper process is killed and reaped before skipping so it doesn't leak. - Verified main.go and main_test.go still build and the test binary still compiles when cross-compiled for GOOS=windows. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- main_test.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/main_test.go b/main_test.go index 6005ade..1c8afc2 100644 --- a/main_test.go +++ b/main_test.go @@ -7,6 +7,7 @@ import ( "net" "os" "os/exec" + "runtime" "strings" "syscall" "testing" @@ -103,7 +104,11 @@ func TestListenAndServe_AddressInUse_ExitsNonZero(t *testing.T) { code := exitCodeFromWaitErr(t, runErr) assert.NotEqual(t, 0, code, "expected non-zero exit code when the listen address is already in use, got 0 (output: %s)", stderr.String()) - assert.Contains(t, stderr.String(), "address already in use") + // The OS-level error text differs by platform (e.g. "address already in + // use" on Unix vs. "Only one usage of each socket address..." on + // Windows), but the "listen tcp : bind:" prefix is generated by + // Go's net package itself and is stable across platforms. + assert.Contains(t, stderr.String(), "listen tcp "+addr+": bind:") } // TestGracefulShutdown_ExitsZero is a regression guard: a clean shutdown via @@ -129,6 +134,18 @@ func TestGracefulShutdown_ExitsZero(t *testing.T) { return true }, 5*time.Second, 50*time.Millisecond, "proxy did not start listening in time") + if runtime.GOOS == "windows" { + // os.Process.Signal(syscall.SIGTERM) is not supported on Windows: it + // returns os.ErrProcessDone/EWINDOWS instead of delivering a graceful + // shutdown signal, so the assertion below would fail there even + // though the exit-code fix itself is correct. Terminate the helper + // process directly instead of leaking it, and skip the + // signal-based assertion on this platform. + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + t.Skip("SIGTERM cannot be delivered via os.Process.Signal on Windows; skipping graceful-shutdown assertion") + } + require.NoError(t, cmd.Process.Signal(syscall.SIGTERM)) waitErr := cmd.Wait() From 76cc2f3f59453887989b241ed9e471b98970ea2d Mon Sep 17 00:00:00 2001 From: Art Leo Date: Fri, 21 Aug 2026 12:05:27 +1000 Subject: [PATCH 4/5] Trigger CI re-run No-op commit to re-trigger workflow runs after a transient GitHub API rate-limit failure in the unrelated Smoke/e2e artifact-download step. No functional changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> From b8292186292ccb298817b9ee16d7737f433edc5d Mon Sep 17 00:00:00 2001 From: Nish Sinha Date: Fri, 21 Aug 2026 15:19:53 -0400 Subject: [PATCH 5/5] Preserve cleanup on fatal exits Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- main.go | 15 +++++++++++---- main_test.go | 24 +++++++++++++++++++----- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/main.go b/main.go index 5cfa026..8b7740d 100644 --- a/main.go +++ b/main.go @@ -31,6 +31,10 @@ var ( ) func main() { + os.Exit(run()) +} + +func run() int { flag.Parse() file := setupLogging() if file != nil { @@ -45,13 +49,13 @@ func main() { cfg, err := config.Parse(*configPath) if err != nil { log.Println(err) - return + return 1 } sentry, err := setupSentry() if err != nil { log.Println(err) - return + return 1 } envSettings := config.ProxyEnvSettings{ @@ -96,13 +100,16 @@ func main() { log.Printf("Listening (%s)", *addr) if err := server.ListenAndServe(); err != http.ErrServerClosed { - log.Fatal(err) + log.Println(err) + return 1 } if err := proxy.Close(); err != nil { log.Println(err) - return + return 1 } + + return 0 } func setupSentry() (bool, error) { diff --git a/main_test.go b/main_test.go index 1c8afc2..9b7bc3e 100644 --- a/main_test.go +++ b/main_test.go @@ -41,7 +41,7 @@ func TestMain(m *testing.M) { // command-line flags and stdin as its stdin. func runHelperProcess(t *testing.T, args []string, stdin string) *exec.Cmd { t.Helper() - cmd := exec.Command(os.Args[0], args...) //nolint:gosec // args are test-controlled, not user input + cmd := exec.CommandContext(t.Context(), os.Args[0], args...) //nolint:gosec // args are test-controlled, not user input cmd.Env = append(os.Environ(), helperProcessEnv+"=1") cmd.Stdin = strings.NewReader(stdin) return cmd @@ -62,7 +62,8 @@ func minimalConfigJSON(t *testing.T) string { // call by briefly binding to port 0 and releasing it. func freeAddr(t *testing.T) string { t.Helper() - l, err := net.Listen("tcp", "127.0.0.1:0") + var listenConfig net.ListenConfig + l, err := listenConfig.Listen(t.Context(), "tcp", "127.0.0.1:0") require.NoError(t, err) addr := l.Addr().String() require.NoError(t, l.Close()) @@ -89,7 +90,8 @@ func exitCodeFromWaitErr(t *testing.T, err error) int { // integrations that gate on exit code (e.g. github/codeql-action) to // silently proceed as if the proxy were listening. func TestListenAndServe_AddressInUse_ExitsNonZero(t *testing.T) { - l, err := net.Listen("tcp", "127.0.0.1:0") + var listenConfig net.ListenConfig + l, err := listenConfig.Listen(t.Context(), "tcp", "127.0.0.1:0") require.NoError(t, err) defer func() { require.NoError(t, l.Close()) @@ -125,8 +127,9 @@ func TestGracefulShutdown_ExitsZero(t *testing.T) { // concurrently by the still-running subprocess, so the failure message // below intentionally omits its contents (only safe to read after // cmd.Wait() below). + dialer := net.Dialer{Timeout: 100 * time.Millisecond} require.Eventually(t, func() bool { - conn, dialErr := net.DialTimeout("tcp", addr, 100*time.Millisecond) + conn, dialErr := dialer.DialContext(t.Context(), "tcp", addr) if dialErr != nil { return false } @@ -142,7 +145,7 @@ func TestGracefulShutdown_ExitsZero(t *testing.T) { // process directly instead of leaking it, and skip the // signal-based assertion on this platform. _ = cmd.Process.Kill() - _, _ = cmd.Process.Wait() + _ = cmd.Wait() t.Skip("SIGTERM cannot be delivered via os.Process.Signal on Windows; skipping graceful-shutdown assertion") } @@ -152,3 +155,14 @@ func TestGracefulShutdown_ExitsZero(t *testing.T) { code := exitCodeFromWaitErr(t, waitErr) assert.Equal(t, 0, code, "expected graceful shutdown to exit 0 (output: %s)", stderr.String()) } + +func TestInvalidConfigPath_ExitsNonZero(t *testing.T) { + cmd := runHelperProcess(t, []string{"-config=/nonexistent/path/definitely-missing.json"}, "") + var stderr bytes.Buffer + cmd.Stderr = &stderr + + runErr := cmd.Run() + + code := exitCodeFromWaitErr(t, runErr) + assert.NotEqual(t, 0, code, "expected non-zero exit code for an invalid config path, got 0 (output: %s)", stderr.String()) +}