From 3e0c417be5b521f849c12d482fd737bf5415aa9e Mon Sep 17 00:00:00 2001 From: Laszlo Sisa Date: Wed, 29 Jul 2026 10:52:48 +0200 Subject: [PATCH 1/3] Fix keep-alive deadlock: wait for a signal instead of bare select {} After warm-up, `block()` parked the main goroutine on a bare `select {}` to keep the container alive when `-exit-after-warmup` is false. This is not a run-forever primitive: once warm-up completes every other goroutine winds down, leaving the empty select as the only goroutine. Go's runtime treats that as a deadlock and aborts the process with "all goroutines are asleep - deadlock!" the moment it goes idle, crash-looping the container after it has already written its probe files. The crash surfaces when the binary is built with newer Go toolchains; older ones only lengthened the fuse. Block on SIGINT/SIGTERM instead. A signal receive is wakeable, so the deadlock detector never fires, and the process now shuts down gracefully when the orchestrator sends a termination signal. `-exit-after-warmup` behaviour is unchanged. Adds tests covering the exit-after-warmup short-circuit and that the keep-alive wait unblocks on a signal. Fixes #366 Co-Authored-By: Claude Opus 4.8 --- cmd/root.go | 26 +++++++++++++-- cmd/root_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 cmd/root_test.go diff --git a/cmd/root.go b/cmd/root.go index 9d9b0b9..dbfc3e2 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -22,6 +22,8 @@ import ( "mittens/internal/pkg/safe" "mittens/internal/pkg/warmup" "os" + "os/signal" + "syscall" "time" ) @@ -135,11 +137,29 @@ func Min(x, y int) int { return x } -// block blocks forever unless `-exit-after-warmup` is set to true +// shutdownSignals are the signals block() waits for to keep the process alive +// after warm-up. Kept as a single source of truth so block() and its tests +// cannot drift on which signals trigger shutdown. +var shutdownSignals = []os.Signal{syscall.SIGINT, syscall.SIGTERM} + +// block keeps the process alive until a shutdown signal so the container stays +// up after warm-up and shuts down gracefully. Waiting on a signal (rather than +// a bare `select {}`) avoids tripping Go's deadlock detector once the process +// is idle (see #366). Returns immediately when `-exit-after-warmup` is set. func block() { - if !opts.ExitAfterWarmup { - select {} + if opts.ExitAfterWarmup { + return } + sig := make(chan os.Signal, 1) + signal.Notify(sig, shutdownSignals...) + defer signal.Stop(sig) + blockUntilSignal(sig) +} + +// blockUntilSignal blocks until a signal is received. It is kept separate from +// block so the waiting behaviour can be tested without sending real signals. +func blockUntilSignal(sig <-chan os.Signal) { + <-sig } // postProcess includes steps that run once the warmup finishes. diff --git a/cmd/root_test.go b/cmd/root_test.go new file mode 100644 index 0000000..08dc755 --- /dev/null +++ b/cmd/root_test.go @@ -0,0 +1,83 @@ +//Copyright 2019 Expedia, Inc. +// +//Licensed under the Apache License, Version 2.0 (the "License"); +//you may not use this file except in compliance with the License. +//You may obtain a copy of the License at +// +//http://www.apache.org/licenses/LICENSE-2.0 +// +//Unless required by applicable law or agreed to in writing, software +//distributed under the License is distributed on an "AS IS" BASIS, +//WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +//See the License for the specific language governing permissions and +//limitations under the License. + +package cmd + +import ( + "mittens/cmd/flags" + "os" + "syscall" + "testing" + "time" +) + +// TestBlockReturnsWhenExitAfterWarmup verifies that block() is a no-op when +// -exit-after-warmup is set, so the process can exit right after warm-up. +func TestBlockReturnsWhenExitAfterWarmup(t *testing.T) { + prev := opts + t.Cleanup(func() { opts = prev }) + + opts = &flags.Root{} + opts.ExitAfterWarmup = true + + done := make(chan struct{}) + go func() { + block() + close(done) + }() + + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("block did not return with -exit-after-warmup set") + } +} + +// TestShutdownSignalsAreTermination pins the signal set block() registers for, +// so swapping in the wrong signal is caught even though os/signal offers no way +// to introspect a channel's registration. +func TestShutdownSignalsAreTermination(t *testing.T) { + want := []os.Signal{syscall.SIGINT, syscall.SIGTERM} + + if len(shutdownSignals) != len(want) { + t.Fatalf("shutdownSignals = %v, want %v", shutdownSignals, want) + } + for i, s := range want { + if shutdownSignals[i] != s { + t.Errorf("shutdownSignals[%d] = %v, want %v", i, shutdownSignals[i], s) + } + } +} + +// TestBlockUntilSignalUnblocksOnSignal verifies the keep-alive wait is +// wakeable: it returns once a termination signal is delivered. This guards +// against regressing to a bare `select {}`, which is unwakeable and trips +// Go's deadlock detector when the process goes idle (see #366). +func TestBlockUntilSignalUnblocksOnSignal(t *testing.T) { + sig := make(chan os.Signal, 1) + + done := make(chan struct{}) + go func() { + blockUntilSignal(sig) + close(done) + }() + + sig <- syscall.SIGTERM + + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("blockUntilSignal did not return after receiving a signal") + } +} From 1ebb539f2f9e4e634445faee5ab116c18aafda7a Mon Sep 17 00:00:00 2001 From: Laszlo Sisa Date: Wed, 29 Jul 2026 13:13:58 +0200 Subject: [PATCH 2/3] Address review: move keep-alive test to an integration test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the cmd-package unit tests (which reached unexported helpers) with a black-box test in test/root_test.go that builds and runs the binary, confirms it stays alive once idle, and exits cleanly on SIGTERM — the signal Kubernetes sends on pod termination. Simplify block() back to the inline signal wait now that the test seam is no longer needed. Co-Authored-By: Claude Opus 4.8 --- cmd/root.go | 21 +++--------- cmd/root_test.go | 83 ----------------------------------------------- test/root_test.go | 63 +++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 99 deletions(-) delete mode 100644 cmd/root_test.go diff --git a/cmd/root.go b/cmd/root.go index dbfc3e2..769bc65 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -137,28 +137,17 @@ func Min(x, y int) int { return x } -// shutdownSignals are the signals block() waits for to keep the process alive -// after warm-up. Kept as a single source of truth so block() and its tests -// cannot drift on which signals trigger shutdown. -var shutdownSignals = []os.Signal{syscall.SIGINT, syscall.SIGTERM} - -// block keeps the process alive until a shutdown signal so the container stays -// up after warm-up and shuts down gracefully. Waiting on a signal (rather than -// a bare `select {}`) avoids tripping Go's deadlock detector once the process -// is idle (see #366). Returns immediately when `-exit-after-warmup` is set. +// block keeps the process alive until SIGINT/SIGTERM so the container stays up +// after warm-up and shuts down gracefully. Waiting on a signal (rather than a +// bare `select {}`) avoids tripping Go's deadlock detector once the process is +// idle (see #366). Returns immediately when `-exit-after-warmup` is set. func block() { if opts.ExitAfterWarmup { return } sig := make(chan os.Signal, 1) - signal.Notify(sig, shutdownSignals...) + signal.Notify(sig, syscall.SIGINT, syscall.SIGTERM) defer signal.Stop(sig) - blockUntilSignal(sig) -} - -// blockUntilSignal blocks until a signal is received. It is kept separate from -// block so the waiting behaviour can be tested without sending real signals. -func blockUntilSignal(sig <-chan os.Signal) { <-sig } diff --git a/cmd/root_test.go b/cmd/root_test.go deleted file mode 100644 index 08dc755..0000000 --- a/cmd/root_test.go +++ /dev/null @@ -1,83 +0,0 @@ -//Copyright 2019 Expedia, Inc. -// -//Licensed under the Apache License, Version 2.0 (the "License"); -//you may not use this file except in compliance with the License. -//You may obtain a copy of the License at -// -//http://www.apache.org/licenses/LICENSE-2.0 -// -//Unless required by applicable law or agreed to in writing, software -//distributed under the License is distributed on an "AS IS" BASIS, -//WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -//See the License for the specific language governing permissions and -//limitations under the License. - -package cmd - -import ( - "mittens/cmd/flags" - "os" - "syscall" - "testing" - "time" -) - -// TestBlockReturnsWhenExitAfterWarmup verifies that block() is a no-op when -// -exit-after-warmup is set, so the process can exit right after warm-up. -func TestBlockReturnsWhenExitAfterWarmup(t *testing.T) { - prev := opts - t.Cleanup(func() { opts = prev }) - - opts = &flags.Root{} - opts.ExitAfterWarmup = true - - done := make(chan struct{}) - go func() { - block() - close(done) - }() - - select { - case <-done: - case <-time.After(time.Second): - t.Fatal("block did not return with -exit-after-warmup set") - } -} - -// TestShutdownSignalsAreTermination pins the signal set block() registers for, -// so swapping in the wrong signal is caught even though os/signal offers no way -// to introspect a channel's registration. -func TestShutdownSignalsAreTermination(t *testing.T) { - want := []os.Signal{syscall.SIGINT, syscall.SIGTERM} - - if len(shutdownSignals) != len(want) { - t.Fatalf("shutdownSignals = %v, want %v", shutdownSignals, want) - } - for i, s := range want { - if shutdownSignals[i] != s { - t.Errorf("shutdownSignals[%d] = %v, want %v", i, shutdownSignals[i], s) - } - } -} - -// TestBlockUntilSignalUnblocksOnSignal verifies the keep-alive wait is -// wakeable: it returns once a termination signal is delivered. This guards -// against regressing to a bare `select {}`, which is unwakeable and trips -// Go's deadlock detector when the process goes idle (see #366). -func TestBlockUntilSignalUnblocksOnSignal(t *testing.T) { - sig := make(chan os.Signal, 1) - - done := make(chan struct{}) - go func() { - blockUntilSignal(sig) - close(done) - }() - - sig <- syscall.SIGTERM - - select { - case <-done: - case <-time.After(time.Second): - t.Fatal("blockUntilSignal did not return after receiving a signal") - } -} diff --git a/test/root_test.go b/test/root_test.go index 98ce26e..6c6a68a 100644 --- a/test/root_test.go +++ b/test/root_test.go @@ -15,6 +15,7 @@ package test import ( + "bytes" "compress/gzip" "context" "fmt" @@ -24,6 +25,9 @@ import ( "mittens/internal/pkg/probe" "net/http" "os" + "os/exec" + "path/filepath" + "syscall" "testing" "time" @@ -273,6 +277,65 @@ func TestCompressWithGZip(t *testing.T) { assert.Equal(t, requestBody, decompressedBody, "Assert that server-side decompressed body is equal to client request body") } +// TestKeepAliveExitsGracefullyOnSigterm runs the built binary the way a +// Kubernetes sidecar does: warm-up finishes (here the target never becomes +// ready, so no requests are sent) and the process is left idle with +// -exit-after-warmup unset. A bare `select {}` keep-alive would abort with +// "all goroutines are asleep - deadlock!" once idle; the signal-based wait must +// instead stay up and then exit cleanly on SIGTERM — the signal kubelet +// delivers when it terminates a pod. +func TestKeepAliveExitsGracefullyOnSigterm(t *testing.T) { + bin := filepath.Join(t.TempDir(), "mittens") + build := exec.Command("go", "build", "-o", bin, ".") + build.Dir = ".." + build.Env = append(os.Environ(), "CGO_ENABLED=0") + if out, err := build.CombinedOutput(); err != nil { + t.Fatalf("failed to build binary: %v\n%s", err, out) + } + + // Point readiness at a port nothing listens on so the target never becomes + // ready: warm-up gives up quickly and the process goes idle. + proc := exec.Command(bin, + "-http-requests=get:/hello-world", + "-target-readiness-port=9999", + "-target-readiness-http-path=/non-existent", + "-max-readiness-wait-seconds=1", + "-max-duration-seconds=2", + ) + var out bytes.Buffer + proc.Stdout = &out + proc.Stderr = &out + if err := proc.Start(); err != nil { + t.Fatalf("failed to start binary: %v", err) + } + + waitErr := make(chan error, 1) + go func() { waitErr <- proc.Wait() }() + + // Once idle the process must stay alive — a bare select{} would already have + // deadlocked and exited by now. + select { + case err := <-waitErr: + t.Fatalf("process exited before receiving a signal (deadlock regression?): %v\n%s", err, out.String()) + case <-time.After(3 * time.Second): + } + + // Kubernetes terminates a pod with SIGTERM; the container must exit promptly + // and cleanly (status 0). + if err := proc.Process.Signal(syscall.SIGTERM); err != nil { + t.Fatalf("failed to send SIGTERM: %v", err) + } + select { + case err := <-waitErr: + if err != nil { + t.Fatalf("process did not exit cleanly on SIGTERM: %v\n%s", err, out.String()) + } + case <-time.After(5 * time.Second): + _ = proc.Process.Kill() + t.Fatalf("process did not exit within 5s of SIGTERM\n%s", out.String()) + } +} + func setup() { fmt.Println("Starting up http server") mockHttpServer, mockHttpServerPort = fixture.StartHttpTargetTestServer([]fixture.PathResponseHandler{ From 331013cc27be7eab9085f26a6a43e737a4c5e771 Mon Sep 17 00:00:00 2001 From: Laszlo Sisa Date: Wed, 29 Jul 2026 13:20:51 +0200 Subject: [PATCH 3/3] ci: re-trigger checks Previous run hit an unrelated flaky test (gRPC reflection); no code change. Co-Authored-By: Claude Opus 4.8