From ee5188fce5bb32108b431f0c489191d0fc9281c0 Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Tue, 25 Aug 2026 13:24:24 +1000 Subject: [PATCH 01/11] feat: make the worker handoff write path cancellable Follow-up to #1231, addressing both review threads left open there. Cancellation. WriteImagesPipe and WriteCompletionPipe now take a context. The collector and remover derive theirs from SIGTERM, so a terminating pod no longer leaves a worker blocked forever on a peer that is never going to arrive, and the scanner passes the context it already has. The Unix rendezvous is deliberately untouched. I had proposed O_NONBLOCK plus polling, but that changes the syscall every existing deployment depends on, and a non-blocking descriptor then has to handle EAGAIN on payloads larger than the pipe buffer. Instead the blocking open runs on a goroutine that hands the file back if the caller is still waiting and closes it if not. Linux keeps the exact open it has always used; only the waiting becomes interruptible. WriteCompletionPipe stats the path before opening, so an absent scanner is still reported as ENOENT even when the context is already done. Left to the select, that case would have been decided at random, which would have made "scanner disabled" indistinguishable from "we are shutting down". WriteScanErasePipe keeps its signature for out-of-tree scanners and waits indefinitely, as before. Endpoint safety. listen removed whatever sat at the endpoint path before binding. A socket left behind by an unclean exit does have to go, or a crashed worker would poison the endpoint for every retry, but anything else there is not ours to delete: the worker runs as NT AUTHORITY\SYSTEM and shares the volume with a scanner image we do not control. Lstat reports ModeSocket on Windows, so the two cases are separable. Signed-off-by: Charles Wu --- pkg/collector/collector.go | 10 ++++- pkg/remover/remover.go | 14 +++--- pkg/scanners/template/scanner_template.go | 2 +- pkg/utils/handoff_test.go | 31 +++++++++++-- pkg/utils/handoff_unix.go | 52 +++++++++++++++++++--- pkg/utils/handoff_windows.go | 53 +++++++++++++++-------- pkg/utils/platform_windows_test.go | 43 ++++++++++++++++++ pkg/utils/utils.go | 5 ++- 8 files changed, 173 insertions(+), 37 deletions(-) diff --git a/pkg/collector/collector.go b/pkg/collector/collector.go index 16646f84f6..8ea21ef59d 100644 --- a/pkg/collector/collector.go +++ b/pkg/collector/collector.go @@ -1,11 +1,14 @@ package main import ( + "context" "flag" "fmt" "net/http" _ "net/http/pprof" "os" + "os/signal" + "syscall" "time" "github.com/eraser-dev/eraser/pkg/cri" @@ -29,6 +32,11 @@ var ( func main() { flag.Parse() + // A terminating pod should not leave the worker blocked on a peer that is + // never going to arrive. The stop func is discarded rather than deferred + // because every exit path here is os.Exit, which would skip it anyway. + ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + if *enableProfile { go func() { server := &http.Server{ @@ -85,7 +93,7 @@ func main() { os.Exit(1) } - if err := util.WriteImagesPipe(path, finalImages); err != nil { + if err := util.WriteImagesPipe(ctx, path, finalImages); err != nil { log.Error(err, "failed to send images", "pipeFile", path) os.Exit(1) } diff --git a/pkg/remover/remover.go b/pkg/remover/remover.go index 5db5bb2dd5..f9f637547d 100644 --- a/pkg/remover/remover.go +++ b/pkg/remover/remover.go @@ -39,6 +39,11 @@ const ( func main() { flag.Parse() + // A terminating pod should not leave the worker blocked on a peer that is + // never going to arrive. The stop func is discarded rather than deferred + // because every exit path here is os.Exit, which would skip it anyway. + ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + if *enableProfile { go func() { server := &http.Server{ @@ -74,7 +79,7 @@ func main() { } if *imageListPtr == "" { - nonCompliantImages, err := util.ReadImagesPipe(context.Background(), util.ScanErasePath) + nonCompliantImages, err := util.ReadImagesPipe(ctx, util.ScanErasePath) if err != nil { log.Error(err, "error reading non-compliant images") os.Exit(generalErr) @@ -114,8 +119,6 @@ func main() { if os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" { // record metrics - ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) - exporter, reader, provider := metrics.ConfigureMetrics(ctx, log, os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT")) otel.SetMeterProvider(provider) @@ -123,16 +126,15 @@ func main() { log.Error(err, "error recording metrics") } metrics.ExportMetrics(log, exporter, reader) - cancel() } if *imageListPtr == "" { - if err := util.WriteCompletionPipe(util.EraseCompleteCollectPath); err != nil { + if err := util.WriteCompletionPipe(ctx, util.EraseCompleteCollectPath); err != nil { log.Error(err, "unable to signal completion", "pipeFile", util.EraseCompleteCollectPath) os.Exit(generalErr) } - err := util.WriteCompletionPipe(util.EraseCompleteScanPath) + err := util.WriteCompletionPipe(ctx, util.EraseCompleteScanPath) // if the scanner is disabled if os.IsNotExist(err) { return diff --git a/pkg/scanners/template/scanner_template.go b/pkg/scanners/template/scanner_template.go index 9f12dcfa21..ba66200059 100644 --- a/pkg/scanners/template/scanner_template.go +++ b/pkg/scanners/template/scanner_template.go @@ -88,7 +88,7 @@ func (cfg *config) SendImages(nonCompliantImages, failedImages []unversioned.Ima nonCompliantImages = append(nonCompliantImages, failedImages...) } - if err := util.WriteScanErasePipe(nonCompliantImages); err != nil { + if err := util.WriteImagesPipe(cfg.ctx, util.ScanErasePath, nonCompliantImages); err != nil { cfg.log.Error(err, "unable to write non-compliant images to scan erase pipe") return err } diff --git a/pkg/utils/handoff_test.go b/pkg/utils/handoff_test.go index 0d1e45a227..b30476e603 100644 --- a/pkg/utils/handoff_test.go +++ b/pkg/utils/handoff_test.go @@ -2,9 +2,11 @@ package utils import ( "context" + "errors" "os" "path/filepath" "testing" + "time" "github.com/eraser-dev/eraser/api/unversioned" ) @@ -36,7 +38,7 @@ func TestImagesHandoffRoundTrip(t *testing.T) { } errCh := make(chan error, 1) - go func() { errCh <- WriteImagesPipe(path, want) }() + go func() { errCh <- WriteImagesPipe(context.Background(), path, want) }() got, err := ReadImagesPipe(context.Background(), path) if err != nil { @@ -66,7 +68,7 @@ func TestCompletionHandoffRoundTrip(t *testing.T) { defer func() { _ = pipe.Close() }() errCh := make(chan error, 1) - go func() { errCh <- WriteCompletionPipe(path) }() + go func() { errCh <- WriteCompletionPipe(context.Background(), path) }() data, err := pipe.Await() if err != nil { @@ -86,7 +88,7 @@ func TestCompletionHandoffRoundTrip(t *testing.T) { func TestWriteCompletionPipeAbsentPeerIsNotExist(t *testing.T) { path := filepath.Join(shortTempDir(t), "no-such-peer") - err := WriteCompletionPipe(path) + err := WriteCompletionPipe(context.Background(), path) if err == nil { t.Fatal("expected an error writing to an endpoint nobody published") } @@ -95,6 +97,29 @@ func TestWriteCompletionPipeAbsentPeerIsNotExist(t *testing.T) { } } +// The whole point of taking a context: a worker whose peer never arrives has to +// be able to give up, on either platform. +func TestWriteImagesPipeHonoursACanceledContext(t *testing.T) { + path := filepath.Join(shortTempDir(t), "never-read") + + ctx, cancel := context.WithCancel(context.Background()) + + errCh := make(chan error, 1) + go func() { errCh <- WriteImagesPipe(ctx, path, []unversioned.Image{{ImageID: "sha256:aaaa"}}) }() + + // nothing ever reads the endpoint, so the write is still waiting + cancel() + + select { + case err := <-errCh: + if !errors.Is(err, context.Canceled) { + t.Errorf("WriteImagesPipe = %v, want context.Canceled", err) + } + case <-time.After(30 * time.Second): + t.Fatal("WriteImagesPipe ignored the canceled context") + } +} + func TestCompletionPipeCloseIsIdempotentlySafe(t *testing.T) { path := filepath.Join(shortTempDir(t), "closed") diff --git a/pkg/utils/handoff_unix.go b/pkg/utils/handoff_unix.go index 49ae8dd35e..bd36cd4b01 100644 --- a/pkg/utils/handoff_unix.go +++ b/pkg/utils/handoff_unix.go @@ -68,8 +68,9 @@ func (p *CompletionPipe) Close() error { return nil } -// WriteImagesPipe publishes the endpoint and blocks until the reader connects. -func WriteImagesPipe(path string, images []unversioned.Image) error { +// WriteImagesPipe publishes the endpoint and blocks until the reader connects, +// or until ctx is done. +func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Image) error { data, err := json.Marshal(images) if err != nil { return err @@ -79,8 +80,7 @@ func WriteImagesPipe(path string, images []unversioned.Image) error { return err } - //nolint:gosec // G304: Opening pipe file is intended functionality - file, err := os.OpenFile(path, os.O_WRONLY, 0) + file, err := openForWrite(ctx, path) if err != nil { return err } @@ -93,6 +93,38 @@ func WriteImagesPipe(path string, images []unversioned.Image) error { return err } +// openForWrite opens a FIFO for writing, which blocks in the kernel until a +// reader arrives. The open itself is left untouched -- the rendezvous, and the +// behavior every existing deployment depends on, is exactly as before. Only +// the waiting is made interruptible, by doing it on a goroutine that hands the +// file over if anyone is still listening and closes it if not. +func openForWrite(ctx context.Context, path string) (*os.File, error) { + type opened struct { + file *os.File + err error + } + + ch := make(chan opened, 1) + go func() { + //nolint:gosec // G304: Opening pipe file is intended functionality + file, err := os.OpenFile(path, os.O_WRONLY, 0) + select { + case ch <- opened{file: file, err: err}: + default: + if file != nil { + _ = file.Close() + } + } + }() + + select { + case <-ctx.Done(): + return nil, ctx.Err() + case o := <-ch: + return o.file, o.err + } +} + // ReadImagesPipe waits for the endpoint to appear, then reads until the writer // finishes. It returns ctx.Err() if the context is canceled while waiting. func ReadImagesPipe(ctx context.Context, path string) ([]unversioned.Image, error) { @@ -143,9 +175,15 @@ func ReadImagesPipe(ctx context.Context, path string) ([]unversioned.Image, erro // WriteCompletionPipe signals a peer that this stage is done. The returned error // satisfies os.IsNotExist when the peer never published the endpoint, which is // how an absent scanner is detected. -func WriteCompletionPipe(path string) error { - //nolint:gosec // G304: Opening pipe file is intended functionality - file, err := os.OpenFile(path, os.O_WRONLY, 0) +func WriteCompletionPipe(ctx context.Context, path string) error { + // Checked before the open so that an absent peer is reported as such even + // when ctx is already done; otherwise a terminating remover could mistake a + // disabled scanner for a cancellation, and vice versa. + if _, err := os.Stat(path); err != nil { + return err + } + + file, err := openForWrite(ctx, path) if err != nil { return err } diff --git a/pkg/utils/handoff_windows.go b/pkg/utils/handoff_windows.go index 0323da79dc..df8d3c1d83 100644 --- a/pkg/utils/handoff_windows.go +++ b/pkg/utils/handoff_windows.go @@ -73,16 +73,15 @@ func (p *CompletionPipe) Close() error { return l.Close() } -// WriteImagesPipe blocks until the reader is listening, then sends the list. -// The unbounded retry mirrors the Unix implementation, where opening a FIFO for -// writing blocks until a reader arrives. -func WriteImagesPipe(path string, images []unversioned.Image) error { +// WriteImagesPipe blocks until the reader is listening, then sends the list, or +// returns ctx.Err() if the context is done first. +func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Image) error { data, err := json.Marshal(images) if err != nil { return err } - conn, err := dialForever(path) + conn, err := dial(ctx, path) if err != nil { return err } @@ -141,7 +140,7 @@ func ReadImagesPipe(ctx context.Context, path string) ([]unversioned.Image, erro // WriteCompletionPipe signals a peer that this stage is done. The returned error // satisfies os.IsNotExist when the peer never published the endpoint, which is // how an absent scanner is detected. -func WriteCompletionPipe(path string) error { +func WriteCompletionPipe(ctx context.Context, path string) error { // Dialing a socket that is not there reports connection-refused on Windows // rather than ENOENT, so the filesystem is the only reliable way to tell // "never published" from "published but gone". @@ -149,7 +148,8 @@ func WriteCompletionPipe(path string) error { return err } - conn, err := net.Dial("unix", path) + var d net.Dialer + conn, err := d.DialContext(ctx, "unix", path) if err != nil { return err } @@ -167,29 +167,48 @@ func listen(path string) (net.Listener, error) { return nil, fmt.Errorf("socket path %q is %d bytes, over the %d byte limit", path, len(path), maxSocketPath) } - // a socket left behind by a previous run would fail the bind - if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + // A socket left behind by an unclean exit would fail the bind, so it has to + // go. Anything else at this path is not ours to delete: the worker runs as + // SYSTEM and shares the volume with a scanner image we do not control. + switch fi, err := os.Lstat(path); { + case errors.Is(err, fs.ErrNotExist): + case err != nil: return nil, err + case fi.Mode()&os.ModeSocket == 0: + return nil, fmt.Errorf("refusing to replace %q: it exists and is not a socket", path) + default: + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, err + } } return net.Listen("unix", path) } -// dialForever waits for the reader to start listening. Errors are not -// classified: Windows reports a missing socket as connection-refused, so there -// is no reliable "not yet" error to match on. Retrying unconditionally mirrors -// the Unix implementation, where opening a FIFO for writing blocks until a -// reader arrives. -func dialForever(path string) (net.Conn, error) { +// dial waits for the reader to start listening. Errors are not classified: +// Windows reports a missing socket as connection-refused, so there is no +// reliable "not yet" error to match on. Retrying on a tick mirrors the Unix +// implementation, where opening a FIFO for writing blocks until a reader +// arrives. +func dial(ctx context.Context, path string) (net.Conn, error) { if len(path) > maxSocketPath { return nil, fmt.Errorf("socket path %q is %d bytes, over the %d byte limit", path, len(path), maxSocketPath) } + ticker := time.NewTicker(time.Second) + defer ticker.Stop() + + var d net.Dialer for { - conn, err := net.Dial("unix", path) + conn, err := d.DialContext(ctx, "unix", path) if err == nil { return conn, nil } - time.Sleep(time.Second) + + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-ticker.C: + } } } diff --git a/pkg/utils/platform_windows_test.go b/pkg/utils/platform_windows_test.go index 4b4a911adb..11244a2b0a 100644 --- a/pkg/utils/platform_windows_test.go +++ b/pkg/utils/platform_windows_test.go @@ -6,6 +6,7 @@ import ( "context" "errors" "fmt" + "net" "os" "path/filepath" "strings" @@ -79,6 +80,48 @@ func TestSocketPathLimitBoundary(t *testing.T) { } } +// The worker runs as SYSTEM and shares the volume with a scanner image we do +// not control, so an occupied endpoint path is a reason to stop rather than to +// start deleting. +func TestListenRefusesToReplaceANonSocket(t *testing.T) { + dir := shortTempDir(t) + path := filepath.Join(dir, "occupied") + + if err := os.WriteFile(path, []byte("not a socket"), 0o600); err != nil { + t.Fatal(err) + } + + if l, err := listen(path); err == nil { + _ = l.Close() + t.Fatal("listen replaced a regular file, want an error") + } + + if _, err := os.Stat(path); err != nil { + t.Errorf("the file was removed anyway: %v", err) + } +} + +// A socket the previous run failed to clean up must still be reclaimable, +// otherwise a crashed worker would poison the endpoint for every retry. +func TestListenReclaimsAStaleSocket(t *testing.T) { + dir := shortTempDir(t) + path := filepath.Join(dir, "stale") + + stale, err := net.Listen("unix", path) + if err != nil { + t.Fatal(err) + } + // leaks the endpoint on purpose: Close would unlink it and remove the case + // under test + t.Cleanup(func() { _ = stale.Close() }) + + l, err := listen(path) + if err != nil { + t.Fatalf("listen over a stale socket: %v", err) + } + _ = l.Close() +} + func TestMkfifoUnsupported(t *testing.T) { if err := mkfifo("ignored", PipeMode); !errors.Is(err, ErrFifoUnsupported) { t.Errorf("mkfifo on windows = %v, want ErrFifoUnsupported", err) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index f3d4521738..4de028ddef 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -334,9 +334,10 @@ func ReadCollectScanPipe(ctx context.Context) ([]unversioned.Image, error) { } // WriteScanErasePipe is the scanner-facing spelling of WriteImagesPipe, kept -// because custom scanners may call it directly. +// because custom scanners may call it directly. It waits indefinitely; reach +// for WriteImagesPipe when the wait needs to be cancellable. func WriteScanErasePipe(vulnerableImages []unversioned.Image) error { - return WriteImagesPipe(ScanErasePath, vulnerableImages) + return WriteImagesPipe(context.Background(), ScanErasePath, vulnerableImages) } func ProcessRepoDigests(repoDigests []string) ([]string, []error) { From 83eb67db82f6cffa1ab2aeb9ccd2e8c9f05622cd Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Wed, 26 Aug 2026 11:31:56 +1000 Subject: [PATCH 02/11] fix: do not swallow SIGTERM, and do not orphan the opened FIFO All three found in review. The buffered channel in openForWrite defeated its own cleanup. With one slot free the send always succeeded, so the default case never ran: if the context won and a reader arrived later, the goroutine handed the file into a buffer nobody would ever read, leaking the descriptor and leaving the FIFO with a writer that never closes. Making the channel unbuffered is not enough on its own, because the open can win the race to that select before the caller reaches its own, and the default case would then close a file the caller was about to ask for. The channel is now unbuffered and paired with an explicit abandoned signal, so the goroutine blocks until the caller has either taken the file or given up on it. Registering signal notification also suppresses Go's default SIGTERM exit, and neither worker observed the context everywhere it mattered. removeImages built its five-minute timeout from context.Background, so a SIGTERM during deletion was ignored until the work finished or the kubelet escalated to SIGKILL; it now derives from the caller's context. In the collector the gap is after the write, where Await deliberately has no context, so notification is stopped before that wait and SIGTERM regains its default effect. Signed-off-by: Charles Wu --- pkg/collector/collector.go | 10 ++++++---- pkg/remover/helpers.go | 7 +++++-- pkg/remover/remover.go | 2 +- pkg/remover/remover_test.go | 3 ++- pkg/utils/handoff_unix.go | 11 +++++++++-- 5 files changed, 23 insertions(+), 10 deletions(-) diff --git a/pkg/collector/collector.go b/pkg/collector/collector.go index 8ea21ef59d..b02bce9306 100644 --- a/pkg/collector/collector.go +++ b/pkg/collector/collector.go @@ -32,10 +32,11 @@ var ( func main() { flag.Parse() - // A terminating pod should not leave the worker blocked on a peer that is - // never going to arrive. The stop func is discarded rather than deferred - // because every exit path here is os.Exit, which would skip it anyway. - ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + // Scoped to the cancellable write below and stopped before Await, which has + // no context: registering the handler suppresses the default SIGTERM exit, so + // holding it across an uncancellable wait would turn a terminating pod into a + // SIGKILL instead of a clean one. + ctx, stopSignals := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) if *enableProfile { go func() { @@ -97,6 +98,7 @@ func main() { log.Error(err, "failed to send images", "pipeFile", path) os.Exit(1) } + stopSignals() data, err := completion.Await() if err != nil { diff --git a/pkg/remover/helpers.go b/pkg/remover/helpers.go index 9d16e1d7d6..eb203e9e14 100644 --- a/pkg/remover/helpers.go +++ b/pkg/remover/helpers.go @@ -8,10 +8,13 @@ import ( util "github.com/eraser-dev/eraser/pkg/utils" ) -func removeImages(c cri.Remover, targetImages []string) (int, error) { +func removeImages(ctx context.Context, c cri.Remover, targetImages []string) (int, error) { removed := 0 - backgroundContext, cancel := context.WithTimeout(context.Background(), timeout) + // Derived from the caller's context, not Background: signal notification is + // registered for the whole process, so nothing would observe a SIGTERM during + // the deletion loop otherwise. + backgroundContext, cancel := context.WithTimeout(ctx, timeout) defer cancel() images, err := c.ListImages(backgroundContext) diff --git a/pkg/remover/remover.go b/pkg/remover/remover.go index f9f637547d..299eea9327 100644 --- a/pkg/remover/remover.go +++ b/pkg/remover/remover.go @@ -111,7 +111,7 @@ func main() { log.Info("no images to exclude") } - removed, err := removeImages(client, imagelist) + removed, err := removeImages(ctx, client, imagelist) if err != nil { log.Error(err, "failed to remove images") os.Exit(generalErr) diff --git a/pkg/remover/remover_test.go b/pkg/remover/remover_test.go index d6ca179578..4b3974a4ed 100644 --- a/pkg/remover/remover_test.go +++ b/pkg/remover/remover_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "testing" v1 "k8s.io/cri-api/pkg/apis/runtime/v1" @@ -50,7 +51,7 @@ func TestRemoveImages(t *testing.T) { } } - _, err := removeImages(client, tc.remove) + _, err := removeImages(context.Background(), client, tc.remove) if tc.shouldErr && err == nil { t.Fatal("expected error, got none") } diff --git a/pkg/utils/handoff_unix.go b/pkg/utils/handoff_unix.go index bd36cd4b01..d43cc6fa62 100644 --- a/pkg/utils/handoff_unix.go +++ b/pkg/utils/handoff_unix.go @@ -104,13 +104,19 @@ func openForWrite(ctx context.Context, path string) (*os.File, error) { err error } - ch := make(chan opened, 1) + // Unbuffered, and paired with abandoned rather than a default case. A + // buffered channel would accept the file after the caller had already + // returned, orphaning the descriptor; a default case would close a file the + // caller was about to ask for, if the open won the race to this select. + ch := make(chan opened) + abandoned := make(chan struct{}) + go func() { //nolint:gosec // G304: Opening pipe file is intended functionality file, err := os.OpenFile(path, os.O_WRONLY, 0) select { case ch <- opened{file: file, err: err}: - default: + case <-abandoned: if file != nil { _ = file.Close() } @@ -119,6 +125,7 @@ func openForWrite(ctx context.Context, path string) (*os.File, error) { select { case <-ctx.Done(): + close(abandoned) return nil, ctx.Err() case o := <-ch: return o.file, o.err From 12469c307ec6e0856e38628a0303751337bdbd3a Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Wed, 26 Aug 2026 16:03:47 +1000 Subject: [PATCH 03/11] fix: make the write itself cancellable, and stop consuming SIGTERM Four more from review. Only the rendezvous was cancellable, not the write. Once the socket or pipe buffer fills, a peer that connects and then stops draining blocks the worker indefinitely, so "the write path is cancellable" was not true for a large image list. Both platforms now watch the context and close the endpoint to unblock the write, and report ctx.Err() rather than the close-induced write error. The collector's signal handler covered far more than the one call that observes it. getImages builds its own timeout from context.Background, so registering the handler at the top of main meant a blocked CRI listing ignored SIGTERM for up to five minutes; the handler now starts immediately before the write. Stopping it afterwards also left a lost-signal window: a SIGTERM landing between the write returning and the handler stopping was consumed rather than killing the process, and the collector walked into Await and waited for SIGKILL. The context is checked once the handler is stopped. The absent-peer test only ever ran with a live context, so the precedence the pre-open Stat exists to guarantee was untested. A missing endpoint must report IsNotExist even when the context is already canceled, otherwise "the scanner is disabled" and "we are terminating" become indistinguishable. Signed-off-by: Charles Wu --- pkg/collector/collector.go | 20 +++++++++++++------ pkg/utils/handoff_test.go | 19 ++++++++++++++++++ pkg/utils/handoff_unix.go | 36 ++++++++++++++++++++++++++++------- pkg/utils/handoff_windows.go | 37 ++++++++++++++++++++++++++++-------- 4 files changed, 91 insertions(+), 21 deletions(-) diff --git a/pkg/collector/collector.go b/pkg/collector/collector.go index b02bce9306..8350d9c3ae 100644 --- a/pkg/collector/collector.go +++ b/pkg/collector/collector.go @@ -32,12 +32,6 @@ var ( func main() { flag.Parse() - // Scoped to the cancellable write below and stopped before Await, which has - // no context: registering the handler suppresses the default SIGTERM exit, so - // holding it across an uncancellable wait would turn a terminating pod into a - // SIGKILL instead of a clean one. - ctx, stopSignals := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) - if *enableProfile { go func() { server := &http.Server{ @@ -94,12 +88,26 @@ func main() { os.Exit(1) } + // Registering the handler suppresses the default SIGTERM exit, so it covers + // exactly the one call that observes ctx. Everything above builds its own + // timeouts from Background, and Await below has no context at all; holding + // the handler across either would swallow the signal. + ctx, stopSignals := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + if err := util.WriteImagesPipe(ctx, path, finalImages); err != nil { + stopSignals() log.Error(err, "failed to send images", "pipeFile", path) os.Exit(1) } stopSignals() + // A signal that landed after the write completed was consumed by the handler + // rather than killing the process, so it has to be acted on here. + if err := ctx.Err(); err != nil { + log.Error(err, "terminating before waiting for completion") + os.Exit(1) + } + data, err := completion.Await() if err != nil { log.Error(err, "failed to read pipe", "pipeFile", util.EraseCompleteCollectPath) diff --git a/pkg/utils/handoff_test.go b/pkg/utils/handoff_test.go index b30476e603..a965370971 100644 --- a/pkg/utils/handoff_test.go +++ b/pkg/utils/handoff_test.go @@ -97,6 +97,25 @@ func TestWriteCompletionPipeAbsentPeerIsNotExist(t *testing.T) { } } +// The pre-open Stat exists so this precedence holds: an endpoint nobody +// published reports IsNotExist even when the caller is already shutting down. +// Left to a select, the two would race and "the scanner is disabled" would +// become indistinguishable from "we are terminating". +func TestWriteCompletionPipeAbsentPeerBeatsACanceledContext(t *testing.T) { + path := filepath.Join(shortTempDir(t), "no-such-peer") + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := WriteCompletionPipe(ctx, path) + if err == nil { + t.Fatal("expected an error writing to an endpoint nobody published") + } + if !os.IsNotExist(err) { + t.Errorf("os.IsNotExist(%v) = false, want true", err) + } +} + // The whole point of taking a context: a worker whose peer never arrives has to // be able to give up, on either platform. func TestWriteImagesPipeHonoursACanceledContext(t *testing.T) { diff --git a/pkg/utils/handoff_unix.go b/pkg/utils/handoff_unix.go index d43cc6fa62..da31a31d6a 100644 --- a/pkg/utils/handoff_unix.go +++ b/pkg/utils/handoff_unix.go @@ -85,7 +85,34 @@ func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Imag return err } - _, err = file.Write(data) + return writeAndClose(ctx, file, data) +} + +// writeAndClose writes the payload and closes, which is what frames the message +// for the reader. The open is not the only place this can block: once the pipe +// buffer fills, a reader that stops draining blocks the write too, so the +// watcher closes the file to unblock it. +func writeAndClose(ctx context.Context, file *os.File, payload []byte) error { + done := make(chan struct{}) + defer close(done) + + go func() { + select { + case <-ctx.Done(): + _ = file.Close() + case <-done: + } + }() + + _, err := file.Write(payload) + + // The watcher may already have closed the file, which is what surfaced as the + // write error, so the context is checked before the error is trusted. + if ctxErr := ctx.Err(); ctxErr != nil { + _ = file.Close() + return ctxErr + } + if closeErr := file.Close(); closeErr != nil && err == nil { err = closeErr } @@ -195,10 +222,5 @@ func WriteCompletionPipe(ctx context.Context, path string) error { return err } - _, err = file.WriteString(EraseCompleteMessage) - if closeErr := file.Close(); closeErr != nil && err == nil { - err = closeErr - } - - return err + return writeAndClose(ctx, file, []byte(EraseCompleteMessage)) } diff --git a/pkg/utils/handoff_windows.go b/pkg/utils/handoff_windows.go index df8d3c1d83..24489dbb39 100644 --- a/pkg/utils/handoff_windows.go +++ b/pkg/utils/handoff_windows.go @@ -86,12 +86,38 @@ func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Imag return err } - if _, err := conn.Write(data); err != nil { + return sendAndClose(ctx, conn, data) +} + +// sendAndClose writes the payload and closes, which is what frames the message +// for the reader. DialContext only makes connecting cancellable, so a peer that +// connects and then stops reading would block the write itself; closing the +// connection from the watcher is what unblocks it. +func sendAndClose(ctx context.Context, conn net.Conn, payload []byte) error { + done := make(chan struct{}) + defer close(done) + + go func() { + select { + case <-ctx.Done(): + _ = conn.Close() + case <-done: + } + }() + + _, err := conn.Write(payload) + + // The watcher may already have closed the connection, which is what surfaced + // as the write error, so the context is checked before the error is trusted. + if ctxErr := ctx.Err(); ctxErr != nil { + _ = conn.Close() + return ctxErr + } + if err != nil { _ = conn.Close() return err } - // closing is what signals end-of-message to the reader return conn.Close() } @@ -154,12 +180,7 @@ func WriteCompletionPipe(ctx context.Context, path string) error { return err } - if _, err := conn.Write([]byte(EraseCompleteMessage)); err != nil { - _ = conn.Close() - return err - } - - return conn.Close() + return sendAndClose(ctx, conn, []byte(EraseCompleteMessage)) } func listen(path string) (net.Listener, error) { From f18303cc6887c6d745e884ae0ef7f5e407b11e64 Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Wed, 26 Aug 2026 16:38:59 +1000 Subject: [PATCH 04/11] fix: read ctx.Err before stopping signal delivery stopSignals cancels the context returned by NotifyContext, so checking ctx.Err afterwards always reported Canceled and the collector exited on every successful run instead of waiting for the erase to finish. The E2E suite caught it: collector_pipeline hung on all four Kubernetes versions while every other test passed, because the remover was left blocking on a completion endpoint whose reader had already exited. Signed-off-by: Charles Wu --- pkg/collector/collector.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/collector/collector.go b/pkg/collector/collector.go index 8350d9c3ae..17190fb7c2 100644 --- a/pkg/collector/collector.go +++ b/pkg/collector/collector.go @@ -99,12 +99,17 @@ func main() { log.Error(err, "failed to send images", "pipeFile", path) os.Exit(1) } + + // Read before stopping, because stopSignals cancels this context itself: + // checked afterwards it would always report Canceled, and the collector + // would exit on every successful run instead of waiting for the erase. + sigErr := ctx.Err() stopSignals() - // A signal that landed after the write completed was consumed by the handler - // rather than killing the process, so it has to be acted on here. - if err := ctx.Err(); err != nil { - log.Error(err, "terminating before waiting for completion") + // A signal that landed while the handler was registered was consumed rather + // than killing the process, so it has to be acted on here. + if sigErr != nil { + log.Error(sigErr, "terminating before waiting for completion") os.Exit(1) } From 287bbbe13176faaddc16390fd4c0ce864192c817 Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Thu, 27 Aug 2026 11:33:04 +1000 Subject: [PATCH 05/11] test: cover cancellation while the write itself is blocked The existing cancellation test never starts a peer, so it only exercises the rendezvous and never reaches writeAndClose or sendAndClose. The blocked-payload path those were added for was untested. The new test attaches a reader that never drains, then cancels once the writer has moved past the open and into Write with a payload far larger than the 64 KiB pipe buffer. It is Unix-only, which is a finding rather than an omission. A single large Write does not block on a Windows Unix domain socket: 64 MiB to a peer that never reads completed in 11ms, because the OS accepts the whole overlapped send regardless of size. The watcher in sendAndClose is kept anyway, since that is an observation about one OS and Go version rather than a documented guarantee, and the two platforms should not offer different contracts. That reasoning is now recorded on the function. Signed-off-by: Charles Wu --- pkg/utils/handoff_windows.go | 12 ++++-- pkg/utils/platform_unix_test.go | 69 +++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/pkg/utils/handoff_windows.go b/pkg/utils/handoff_windows.go index 24489dbb39..28cb3d5820 100644 --- a/pkg/utils/handoff_windows.go +++ b/pkg/utils/handoff_windows.go @@ -90,9 +90,15 @@ func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Imag } // sendAndClose writes the payload and closes, which is what frames the message -// for the reader. DialContext only makes connecting cancellable, so a peer that -// connects and then stops reading would block the write itself; closing the -// connection from the watcher is what unblocks it. +// for the reader. DialContext only makes connecting cancellable, so the watcher +// covers the write itself. +// +// A single large Write does not appear to block here in practice -- 64 MiB to a +// peer that never reads completed in 11ms, because Windows accepts the whole +// overlapped send regardless of size. The watcher is kept anyway: that is an +// observation about one OS and Go version, not a documented guarantee, and the +// Unix implementation genuinely does block once the pipe buffer fills. The +// contract should not differ between the two. func sendAndClose(ctx context.Context, conn net.Conn, payload []byte) error { done := make(chan struct{}) defer close(done) diff --git a/pkg/utils/platform_unix_test.go b/pkg/utils/platform_unix_test.go index 9d156dc774..98062993e3 100644 --- a/pkg/utils/platform_unix_test.go +++ b/pkg/utils/platform_unix_test.go @@ -10,8 +10,77 @@ import ( "os" "path/filepath" "testing" + "time" + + "github.com/eraser-dev/eraser/api/unversioned" ) +// The rendezvous is not the only place a write can block: once the 64 KiB pipe +// buffer fills, a reader that has opened the FIFO and then stopped draining +// holds the writer in Write, which no open deadline covers. +// +// This is Unix-only on purpose. The same scenario is not reachable through the +// socket implementation: a single Write of 64 MiB to a stalled peer was measured +// completing in 11ms on Windows, because the OS accepts the whole overlapped +// send regardless of size. +func TestWriteImagesPipeHonoursCancellationWhileBlockedOnAStalledReader(t *testing.T) { + path := filepath.Join(shortTempDir(t), "stalled") + + // well past the 64 KiB pipe buffer, so the write cannot simply complete + images := make([]unversioned.Image, 120000) + for i := range images { + images[i] = unversioned.Image{ImageID: fmt.Sprintf("sha256:%060d", i)} + } + + ctx, cancel := context.WithCancel(context.Background()) + + errCh := make(chan error, 1) + go func() { errCh <- WriteImagesPipe(ctx, path, images) }() + + stallReader(t, path) + + // The reader is attached, so the open has returned and the writer has moved + // on to filling the buffer. There is no way to observe "blocked in Write" + // directly, so give it a moment to get there -- otherwise this degrades into + // the rendezvous case already covered in handoff_test.go. + time.Sleep(500 * time.Millisecond) + cancel() + + select { + case err := <-errCh: + if !errors.Is(err, context.Canceled) { + t.Errorf("WriteImagesPipe = %v, want context.Canceled", err) + } + case <-time.After(30 * time.Second): + t.Fatal("WriteImagesPipe ignored cancellation while blocked on a stalled reader") + } +} + +// stallReader opens the FIFO for reading and never reads from it. Opening is +// what releases the writer's blocked open, so the writer proceeds into Write and +// stops once the pipe buffer is full. +func stallReader(t *testing.T, path string) { + t.Helper() + + // the writer creates the FIFO, so it may not exist yet + deadline := time.Now().Add(10 * time.Second) + for { + //nolint:gosec // G304: opening the test's own pipe is the point + f, err := os.OpenFile(path, os.O_RDONLY, 0) + if err == nil { + t.Cleanup(func() { _ = f.Close() }) + return + } + if !os.IsNotExist(err) { + t.Fatalf("open fifo for reading: %v", err) + } + if time.Now().After(deadline) { + t.Fatal("the writer never created the fifo") + } + time.Sleep(10 * time.Millisecond) + } +} + func TestGetAddressAndDialer(t *testing.T) { testCases := []struct { endpoint string From 85102512c47a5383008e4fcbaa49ba4363e8d6fe Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Thu, 27 Aug 2026 13:18:09 +1000 Subject: [PATCH 06/11] test: give the handoff round trips a deadline Both halves of a handoff block until the peer arrives, and the round trips passed context.Background to both. That is fine while they pass. When they do not -- one side failing to reach the rendezvous -- the other side waits forever, and the first sign of trouble is the package-wide ten minute timeout with no indication of which test is stuck. A run here did exactly that. A 30 second deadline turns the hang into a failure in the test that caused it. The cancellation tests already had this guard; the round trips were the gap. Await takes no context, so the completion round trip enforces the deadline with a select instead. That asymmetry is a fair argument for giving Await a context, which is still open from earlier review. Signed-off-by: Charles Wu --- pkg/utils/handoff_test.go | 50 ++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 8 deletions(-) diff --git a/pkg/utils/handoff_test.go b/pkg/utils/handoff_test.go index a965370971..48610fca20 100644 --- a/pkg/utils/handoff_test.go +++ b/pkg/utils/handoff_test.go @@ -29,8 +29,23 @@ func shortTempDir(t *testing.T) string { return dir } +// testContext bounds the round trips. Both halves of a handoff block until the +// peer shows up, so on context.Background a rendezvous that never completes +// hangs until the package-wide test timeout -- ten minutes of nothing, with no +// indication of which test is stuck. A deadline turns that into a failure in +// the test that caused it. +func testContext(t *testing.T) context.Context { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + + return ctx +} + func TestImagesHandoffRoundTrip(t *testing.T) { path := filepath.Join(shortTempDir(t), "images") + ctx := testContext(t) want := []unversioned.Image{ {ImageID: "sha256:aaaa", Names: []string{"repo/one:v1"}}, @@ -38,9 +53,9 @@ func TestImagesHandoffRoundTrip(t *testing.T) { } errCh := make(chan error, 1) - go func() { errCh <- WriteImagesPipe(context.Background(), path, want) }() + go func() { errCh <- WriteImagesPipe(ctx, path, want) }() - got, err := ReadImagesPipe(context.Background(), path) + got, err := ReadImagesPipe(ctx, path) if err != nil { t.Fatalf("ReadImagesPipe: %v", err) } @@ -60,6 +75,7 @@ func TestImagesHandoffRoundTrip(t *testing.T) { func TestCompletionHandoffRoundTrip(t *testing.T) { path := filepath.Join(shortTempDir(t), "complete") + ctx := testContext(t) pipe, err := CreateCompletionPipe(path) if err != nil { @@ -68,18 +84,36 @@ func TestCompletionHandoffRoundTrip(t *testing.T) { defer func() { _ = pipe.Close() }() errCh := make(chan error, 1) - go func() { errCh <- WriteCompletionPipe(context.Background(), path) }() + go func() { errCh <- WriteCompletionPipe(ctx, path) }() - data, err := pipe.Await() - if err != nil { - t.Fatalf("Await: %v", err) + type awaited struct { + data []byte + err error + } + + // Await takes no context, so it cannot be given the deadline directly; the + // select is what enforces it. + awaitCh := make(chan awaited, 1) + go func() { + data, err := pipe.Await() + awaitCh <- awaited{data: data, err: err} + }() + + var got awaited + select { + case got = <-awaitCh: + case <-ctx.Done(): + t.Fatalf("Await did not return within the deadline: %v", ctx.Err()) + } + if got.err != nil { + t.Fatalf("Await: %v", got.err) } if err := <-errCh; err != nil { t.Fatalf("WriteCompletionPipe: %v", err) } - if string(data) != EraseCompleteMessage { - t.Errorf("payload = %q, want %q", string(data), EraseCompleteMessage) + if string(got.data) != EraseCompleteMessage { + t.Errorf("payload = %q, want %q", string(got.data), EraseCompleteMessage) } } From 822f166ce119015080be1f084c57adcd8b38ff1e Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Thu, 27 Aug 2026 13:38:36 +1000 Subject: [PATCH 07/11] fix: stop suppressing SIGTERM across the uncancellable read Found in review. Registering the signal handler at the top of main disabled the default SIGTERM exit for the whole process, the wait for the scanner included. That wait is only interruptible while the endpoint is absent: once the peer publishes, the read blocks in os.OpenFile on the FIFO or io.ReadAll on the accepted socket, and the context reaches neither. A peer that published and then stalled left the remover ignoring SIGTERM until the kubelet's SIGKILL, where before this branch it died immediately -- the opposite of what the branch is for. Upstream registered the handler only after the read, and the collector already spells out the rule: cover exactly the calls that observe ctx. The remover now follows it too. Cancellation also has to be reported. Both delete branches log and continue, so removeImages returns nil, and with --imagelist there is no completion write afterwards to surface it -- an interrupted run exited 0 having removed nothing. ctx.Err is checked before the removal counts as a success. Signed-off-by: Charles Wu --- pkg/remover/remover.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/pkg/remover/remover.go b/pkg/remover/remover.go index 299eea9327..2dfda19433 100644 --- a/pkg/remover/remover.go +++ b/pkg/remover/remover.go @@ -39,11 +39,6 @@ const ( func main() { flag.Parse() - // A terminating pod should not leave the worker blocked on a peer that is - // never going to arrive. The stop func is discarded rather than deferred - // because every exit path here is os.Exit, which would skip it anyway. - ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) - if *enableProfile { go func() { server := &http.Server{ @@ -79,7 +74,7 @@ func main() { } if *imageListPtr == "" { - nonCompliantImages, err := util.ReadImagesPipe(ctx, util.ScanErasePath) + nonCompliantImages, err := util.ReadImagesPipe(context.Background(), util.ScanErasePath) if err != nil { log.Error(err, "error reading non-compliant images") os.Exit(generalErr) @@ -111,12 +106,27 @@ func main() { log.Info("no images to exclude") } + // Registering the handler suppresses the default SIGTERM exit, so it starts + // only here: once the peer publishes its endpoint the read above blocks in a + // call no context can interrupt, and covering it would swallow the signal + // until SIGKILL. The stop func is discarded rather than deferred because + // every exit path below is os.Exit, which would skip it anyway. + ctx, _ := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + removed, err := removeImages(ctx, client, imagelist) if err != nil { log.Error(err, "failed to remove images") os.Exit(generalErr) } + // A signal that landed during removal was consumed rather than killing the + // process, and with --imagelist there is no completion write below to report + // it, so an interrupted run would otherwise exit 0 having removed nothing. + if err := ctx.Err(); err != nil { + log.Error(err, "terminating before removal finished", "removed", removed) + os.Exit(generalErr) + } + if os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" { // record metrics exporter, reader, provider := metrics.ConfigureMetrics(ctx, log, os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT")) From 5e7080b20f41f4db2cae592cae126c81e88b7cfe Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Thu, 27 Aug 2026 13:45:12 +1000 Subject: [PATCH 08/11] fix: close the two cancellation races review found in the handoff Both were reported against code that had not changed since the previous round. The write watcher outlived the context check. Cancellation landing between that check and the function's own Close left the two goroutines racing to close the same handle, so a write that had already succeeded could return os.ErrClosed instead of nil -- a delivered handoff reported as a failure. The watcher is now joined before the handle is touched again and reports whether it did the closing, so the outcome is known rather than inferred from the error. Both platforms had it. listen could not tell a stale endpoint from a live one: the mode is ModeSocket either way, so a listener that was still serving would be unlinked and rebound over, silently stranding its peer. The endpoint is probed with a connect first, and answering means live, and live means refuse. TestListenReclaimsAStaleSocket had been asserting the old behavior -- it kept its listener open, so the case it covered was a live socket, not a stale one. It now uses SetUnlinkOnClose to leave a genuinely abandoned endpoint, and a second test covers the live case; without the probe that one fails with "listen replaced a live socket". The probe narrows the Lstat/Remove window rather than closing it. There is no atomic unlink-if-socket to reach for, and exploiting what is left needs the scanner to plant a file inside the window, where the worst outcome is deleting that file. Signed-off-by: Charles Wu --- pkg/utils/handoff_unix.go | 15 +++++++----- pkg/utils/handoff_windows.go | 28 +++++++++++++++++----- pkg/utils/platform_windows_test.go | 37 ++++++++++++++++++++++++++---- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/pkg/utils/handoff_unix.go b/pkg/utils/handoff_unix.go index da31a31d6a..7d5b36c5ed 100644 --- a/pkg/utils/handoff_unix.go +++ b/pkg/utils/handoff_unix.go @@ -94,23 +94,26 @@ func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Imag // watcher closes the file to unblock it. func writeAndClose(ctx context.Context, file *os.File, payload []byte) error { done := make(chan struct{}) - defer close(done) + closedByWatcher := make(chan bool, 1) go func() { select { case <-ctx.Done(): _ = file.Close() + closedByWatcher <- true case <-done: + closedByWatcher <- false } }() _, err := file.Write(payload) - // The watcher may already have closed the file, which is what surfaced as the - // write error, so the context is checked before the error is trusted. - if ctxErr := ctx.Err(); ctxErr != nil { - _ = file.Close() - return ctxErr + // Joining the watcher before touching the file again is what makes the rest + // unambiguous: once it has reported, no cancellation close can still land, + // and whoever closed the file is known rather than guessed from the error. + close(done) + if <-closedByWatcher { + return ctx.Err() } if closeErr := file.Close(); closeErr != nil && err == nil { diff --git a/pkg/utils/handoff_windows.go b/pkg/utils/handoff_windows.go index 28cb3d5820..2ef4fcd275 100644 --- a/pkg/utils/handoff_windows.go +++ b/pkg/utils/handoff_windows.go @@ -32,6 +32,10 @@ import ( // checked up front. const maxSocketPath = 107 +// stalenessProbe bounds the connect used to tell a stale endpoint from a live +// one. Both ends are local, so a listener that exists answers immediately. +const stalenessProbe = time.Second + // CompletionPipe is an endpoint a peer can observe before anything is read from // it. The scanner creates one early precisely so the remover can tell a scanner // is present, which means the listener has to outlive its creation. @@ -101,24 +105,28 @@ func WriteImagesPipe(ctx context.Context, path string, images []unversioned.Imag // contract should not differ between the two. func sendAndClose(ctx context.Context, conn net.Conn, payload []byte) error { done := make(chan struct{}) - defer close(done) + closedByWatcher := make(chan bool, 1) go func() { select { case <-ctx.Done(): _ = conn.Close() + closedByWatcher <- true case <-done: + closedByWatcher <- false } }() _, err := conn.Write(payload) - // The watcher may already have closed the connection, which is what surfaced - // as the write error, so the context is checked before the error is trusted. - if ctxErr := ctx.Err(); ctxErr != nil { - _ = conn.Close() - return ctxErr + // Joining the watcher before touching the connection again is what makes the + // rest unambiguous: once it has reported, no cancellation close can still + // land, and whoever closed it is known rather than guessed from the error. + close(done) + if <-closedByWatcher { + return ctx.Err() } + if err != nil { _ = conn.Close() return err @@ -204,6 +212,14 @@ func listen(path string) (net.Listener, error) { case fi.Mode()&os.ModeSocket == 0: return nil, fmt.Errorf("refusing to replace %q: it exists and is not a socket", path) default: + // The mode says socket, not stale socket -- a live listener looks + // identical on disk. Connecting is the only way to tell, and stranding a + // peer that is still listening is worse than refusing to start. + if conn, err := net.DialTimeout("unix", path, stalenessProbe); err == nil { + _ = conn.Close() + return nil, fmt.Errorf("refusing to replace %q: something is still listening on it", path) + } + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { return nil, err } diff --git a/pkg/utils/platform_windows_test.go b/pkg/utils/platform_windows_test.go index 11244a2b0a..e112bdbf0c 100644 --- a/pkg/utils/platform_windows_test.go +++ b/pkg/utils/platform_windows_test.go @@ -107,13 +107,20 @@ func TestListenReclaimsAStaleSocket(t *testing.T) { dir := shortTempDir(t) path := filepath.Join(dir, "stale") - stale, err := net.Listen("unix", path) + // Go unlinks the socket on Close, so the only endpoint left on disk is one + // nobody closed. SetUnlinkOnClose reproduces that without crashing a + // process: the file stays, the listener does not. + stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) if err != nil { t.Fatal(err) } - // leaks the endpoint on purpose: Close would unlink it and remove the case - // under test - t.Cleanup(func() { _ = stale.Close() }) + stale.SetUnlinkOnClose(false) + if err := stale.Close(); err != nil { + t.Fatal(err) + } + if _, err := os.Lstat(path); err != nil { + t.Fatalf("the endpoint should still be on disk: %v", err) + } l, err := listen(path) if err != nil { @@ -122,6 +129,28 @@ func TestListenReclaimsAStaleSocket(t *testing.T) { _ = l.Close() } +// The case above is indistinguishable from this one by mode alone, and taking +// the path from a peer that is still listening would strand it silently. +func TestListenRefusesALiveSocket(t *testing.T) { + dir := shortTempDir(t) + path := filepath.Join(dir, "live") + + live, err := net.Listen("unix", path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = live.Close() }() + + if l, err := listen(path); err == nil { + _ = l.Close() + t.Fatal("listen replaced a live socket, want an error") + } + + if _, err := os.Lstat(path); err != nil { + t.Errorf("the live endpoint was removed anyway: %v", err) + } +} + func TestMkfifoUnsupported(t *testing.T) { if err := mkfifo("ignored", PipeMode); !errors.Is(err, ErrFifoUnsupported) { t.Errorf("mkfifo on windows = %v, want ErrFifoUnsupported", err) From 080a917a0f5660b2fc21632af611bba8ba807b75 Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Thu, 27 Aug 2026 14:54:17 +1000 Subject: [PATCH 09/11] fix: stop the staleness probe from being taken for the peer Found in review, and introduced by the previous round's fix. listen probes an existing endpoint to tell a live socket from a stale one. That probe is a connection, and these endpoints serve exactly one: the listener it had just declined to evict would accept the probe, read nothing and be left with no peer still to come. Refusing to unlink the socket was right and stranded the worker anyway. Await and ReadImagesPipe now keep accepting until something actually sends a payload. Nothing legitimate sends an empty one -- the image list is JSON, the completion message is a constant -- and the volume is shared, so the peer was never the only thing that could knock. This covers stray connects generally rather than the probe specifically. TestAwaitIgnoresAConnectThatSaysNothing queues a connect ahead of the peer exactly as listen does; without the skip it fails with payload = "", want "complete". The last two dials in tests that still passed context.Background now take the bounded one. A package where every wait is bounded fails in the test that stalled, rather than hanging until the go test timeout kills the binary with no indication of which test was stuck -- which is how both stalls seen here have presented. Signed-off-by: Charles Wu --- pkg/utils/handoff_windows.go | 67 ++++++++++++++++++++---------- pkg/utils/platform_unix_test.go | 2 +- pkg/utils/platform_windows_test.go | 58 +++++++++++++++++++++++++- 3 files changed, 102 insertions(+), 25 deletions(-) diff --git a/pkg/utils/handoff_windows.go b/pkg/utils/handoff_windows.go index 2ef4fcd275..be57e2f2cb 100644 --- a/pkg/utils/handoff_windows.go +++ b/pkg/utils/handoff_windows.go @@ -56,13 +56,29 @@ func CreateCompletionPipe(path string) (*CompletionPipe, error) { // Await blocks until a peer signals completion. The payload is returned // unvalidated so callers keep their existing handling of unexpected content. func (p *CompletionPipe) Await() ([]byte, error) { - conn, err := p.l.Accept() - if err != nil { - return nil, err - } - defer func() { _ = conn.Close() }() + for { + conn, err := p.l.Accept() + if err != nil { + return nil, err + } + + data, err := io.ReadAll(conn) + _ = conn.Close() + if err != nil { + return nil, err + } + + // A connect that says nothing is not the peer: listen probes this + // endpoint to tell a live socket from a stale one, and the volume is + // shared, so the peer is not the only thing that can knock. Accepting + // one of those as the signal would strand the worker that meant to send + // it. + if len(data) == 0 { + continue + } - return io.ReadAll(conn) + return data, nil + } } // Close releases the endpoint, which also unpublishes it. Callers both defer @@ -155,26 +171,33 @@ func ReadImagesPipe(ctx context.Context, path string) ([]unversioned.Image, erro } }() - conn, err := l.Accept() - if err != nil { - if ctxErr := ctx.Err(); ctxErr != nil { - return nil, ctxErr + for { + conn, err := l.Accept() + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + return nil, err } - return nil, err - } - defer func() { _ = conn.Close() }() - data, err := io.ReadAll(conn) - if err != nil { - return nil, err - } + data, err := io.ReadAll(conn) + _ = conn.Close() + if err != nil { + return nil, err + } - images := []unversioned.Image{} - if err := json.Unmarshal(data, &images); err != nil { - return nil, err - } + // see Await: a connect that says nothing is not the writer + if len(data) == 0 { + continue + } - return images, nil + images := []unversioned.Image{} + if err := json.Unmarshal(data, &images); err != nil { + return nil, err + } + + return images, nil + } } // WriteCompletionPipe signals a peer that this stage is done. The returned error diff --git a/pkg/utils/platform_unix_test.go b/pkg/utils/platform_unix_test.go index 98062993e3..352aa3ed59 100644 --- a/pkg/utils/platform_unix_test.go +++ b/pkg/utils/platform_unix_test.go @@ -143,7 +143,7 @@ func TestUnixDialerConnects(t *testing.T) { t.Fatalf("getAddressAndDialer: %v", err) } - conn, err := dialer(context.Background(), addr) + conn, err := dialer(testContext(t), addr) if err != nil { t.Fatalf("dial %q: %v", addr, err) } diff --git a/pkg/utils/platform_windows_test.go b/pkg/utils/platform_windows_test.go index e112bdbf0c..c4a51ed988 100644 --- a/pkg/utils/platform_windows_test.go +++ b/pkg/utils/platform_windows_test.go @@ -3,7 +3,6 @@ package utils import ( - "context" "errors" "fmt" "net" @@ -151,6 +150,61 @@ func TestListenRefusesALiveSocket(t *testing.T) { } } +// Refusing to unlink a live endpoint is not enough on its own: the probe that +// establishes it is live is itself a connection, and these endpoints serve one. +// If the probe were mistaken for the peer, the listener it just declined to +// evict would be handed an empty payload and left with nothing to wait for. +func TestAwaitIgnoresAConnectThatSaysNothing(t *testing.T) { + path := filepath.Join(shortTempDir(t), "complete") + ctx := testContext(t) + + pipe, err := CreateCompletionPipe(path) + if err != nil { + t.Fatalf("CreateCompletionPipe: %v", err) + } + defer func() { _ = pipe.Close() }() + + // the listener is already published, so this is queued ahead of the peer -- + // exactly what listen's staleness probe does to a live endpoint + probe, err := net.DialTimeout("unix", path, stalenessProbe) + if err != nil { + t.Fatalf("probing the endpoint: %v", err) + } + if err := probe.Close(); err != nil { + t.Fatal(err) + } + + errCh := make(chan error, 1) + go func() { errCh <- WriteCompletionPipe(ctx, path) }() + + type awaited struct { + data []byte + err error + } + awaitCh := make(chan awaited, 1) + go func() { + data, err := pipe.Await() + awaitCh <- awaited{data: data, err: err} + }() + + var got awaited + select { + case got = <-awaitCh: + case <-ctx.Done(): + t.Fatalf("Await did not return within the deadline: %v", ctx.Err()) + } + if got.err != nil { + t.Fatalf("Await: %v", got.err) + } + if err := <-errCh; err != nil { + t.Fatalf("WriteCompletionPipe: %v", err) + } + + if string(got.data) != EraseCompleteMessage { + t.Errorf("payload = %q, want %q -- the probe was taken for the peer", string(got.data), EraseCompleteMessage) + } +} + func TestMkfifoUnsupported(t *testing.T) { if err := mkfifo("ignored", PipeMode); !errors.Is(err, ErrFifoUnsupported) { t.Errorf("mkfifo on windows = %v, want ErrFifoUnsupported", err) @@ -176,7 +230,7 @@ func TestNpipeDialerConnects(t *testing.T) { t.Fatalf("getAddressAndDialer: %v", err) } - conn, err := dialer(context.Background(), addr) + conn, err := dialer(testContext(t), addr) if err != nil { t.Fatalf("dial %q: %v", addr, err) } From 768d5bfce148eefd96cbe14ef7477dc29332e48e Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Thu, 27 Aug 2026 14:54:17 +1000 Subject: [PATCH 10/11] test: pin the context removeImages hands the runtime Found in review. removeImages derives its deadline from the caller rather than Background so that process-wide signal notification actually reaches the runtime, and nothing covered it: every existing case passes a context that is never done, and the fake CRI client discarded the argument entirely. Reverting the propagation left the suite green. The fake now observes its context, as a real client does, and a caller that has already gone must not get images deleted on its behalf. Against context.Background the new test reports removed = 1, want 0. Signed-off-by: Charles Wu --- pkg/remover/remover_test.go | 23 +++++++++++++++++++++++ pkg/remover/test_client_test.go | 6 +++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/pkg/remover/remover_test.go b/pkg/remover/remover_test.go index 4b3974a4ed..efa80da72b 100644 --- a/pkg/remover/remover_test.go +++ b/pkg/remover/remover_test.go @@ -86,3 +86,26 @@ func TestRemoveImages(t *testing.T) { }) } } + +// removeImages builds its deadline from the caller rather than Background, so +// that a SIGTERM registered process-wide actually reaches the runtime. Nothing +// above notices if that regresses -- every case passes a context that is never +// done -- so this pins it: a caller who has already gone must not get images +// deleted on their behalf. +func TestRemoveImagesPassesTheCallersContextToTheRuntime(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + client := &testClient{t: t, images: []*v1.Image{{Id: "sha256:aaaa"}}} + + removed, err := removeImages(ctx, client, []string{"sha256:aaaa"}) + if err != nil { + t.Fatalf("removeImages: %v", err) + } + if removed != 0 { + t.Errorf("removed = %d, want 0", removed) + } + if len(client.images) != 1 { + t.Error("the image was deleted for a caller that was already gone") + } +} diff --git a/pkg/remover/test_client_test.go b/pkg/remover/test_client_test.go index 799c93e6a4..7fa6e47b73 100644 --- a/pkg/remover/test_client_test.go +++ b/pkg/remover/test_client_test.go @@ -87,8 +87,12 @@ func (c *testClient) removeImageFromSlice(index int) { c.images = s } -func (c *testClient) DeleteImage(_ context.Context, image string) (err error) { +func (c *testClient) DeleteImage(ctx context.Context, image string) (err error) { c.logf("DeleteImage: %s", image) + // a real CRI client fails the call rather than deleting anyway + if err := ctx.Err(); err != nil { + return err + } if image == "" { return errImageEmpty } From b833fd95f736f7c87643aac3f2479ba03aee5ced Mon Sep 17 00:00:00 2001 From: Charles Wu Date: Thu, 27 Aug 2026 16:49:34 +1000 Subject: [PATCH 11/11] fix: refuse an occupied endpoint instead of reclaiming it Found in review, on the third attempt at the same question. listen removed whatever socket it found, on the theory that a crashed worker would otherwise poison the endpoint for every retry. Review has now broken that theory three ways: mode cannot tell a live socket from an abandoned one, the connect added to tell them apart is itself a connection these single-accept endpoints will consume, and a refused connect is not proof of staleness either, since a live listener with a full backlog refuses too. Each fix created the next defect, which is the sign the premise was wrong rather than the code. The premise was wrong. Nothing of ours outlives the pod at these paths: the shared volume is an emptyDir created with it, and restartPolicy is Never, so a worker that dies is replaced by a new pod with a new volume rather than restarted onto this one. There is no retry that inherits a socket, so there is nothing to reclaim -- and Unix has never tried to, because CreateCompletionPipe calls mkfifo and lets EEXIST through. So Windows now fails the same way. Whatever is at the path, it is not a previous run of ours, and the volume is shared with a scanner image we do not control, so it is refused rather than deleted. This drops the connect probe and the Remove with it, which is also the end of the Lstat/Remove race: the window closes because nothing is unlinked at all, rather than being narrowed. The three listen tests collapse into the two cases that still differ: something there that is not a socket, and something there that is. The latter covers both an endpoint left by a dead listener and one still being served, because they are the same case now. Signed-off-by: Charles Wu --- pkg/utils/handoff_windows.go | 29 +++------ pkg/utils/platform_windows_test.go | 97 ++++++++++++++---------------- 2 files changed, 54 insertions(+), 72 deletions(-) diff --git a/pkg/utils/handoff_windows.go b/pkg/utils/handoff_windows.go index be57e2f2cb..4bb83baf13 100644 --- a/pkg/utils/handoff_windows.go +++ b/pkg/utils/handoff_windows.go @@ -32,10 +32,6 @@ import ( // checked up front. const maxSocketPath = 107 -// stalenessProbe bounds the connect used to tell a stale endpoint from a live -// one. Both ends are local, so a listener that exists answers immediately. -const stalenessProbe = time.Second - // CompletionPipe is an endpoint a peer can observe before anything is read from // it. The scanner creates one early precisely so the remover can tell a scanner // is present, which means the listener has to outlive its creation. @@ -225,27 +221,18 @@ func listen(path string) (net.Listener, error) { return nil, fmt.Errorf("socket path %q is %d bytes, over the %d byte limit", path, len(path), maxSocketPath) } - // A socket left behind by an unclean exit would fail the bind, so it has to - // go. Anything else at this path is not ours to delete: the worker runs as - // SYSTEM and shares the volume with a scanner image we do not control. - switch fi, err := os.Lstat(path); { + // Nothing of ours outlives the pod here: the shared volume is an emptyDir + // created with it, and restartPolicy is Never, so a worker that dies is + // replaced by a new pod with a new volume rather than restarted onto this + // one. Whatever is at this path is therefore not a previous run to clean up, + // and the volume is shared with a scanner image we do not control. Unix + // refuses the same way, because mkfifo returns EEXIST. + switch _, err := os.Lstat(path); { case errors.Is(err, fs.ErrNotExist): case err != nil: return nil, err - case fi.Mode()&os.ModeSocket == 0: - return nil, fmt.Errorf("refusing to replace %q: it exists and is not a socket", path) default: - // The mode says socket, not stale socket -- a live listener looks - // identical on disk. Connecting is the only way to tell, and stranding a - // peer that is still listening is worse than refusing to start. - if conn, err := net.DialTimeout("unix", path, stalenessProbe); err == nil { - _ = conn.Close() - return nil, fmt.Errorf("refusing to replace %q: something is still listening on it", path) - } - - if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { - return nil, err - } + return nil, fmt.Errorf("refusing to bind %q: something already exists at that path", path) } return net.Listen("unix", path) diff --git a/pkg/utils/platform_windows_test.go b/pkg/utils/platform_windows_test.go index c4a51ed988..5fd6943b9d 100644 --- a/pkg/utils/platform_windows_test.go +++ b/pkg/utils/platform_windows_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/Microsoft/go-winio" ) @@ -90,70 +91,65 @@ func TestListenRefusesToReplaceANonSocket(t *testing.T) { t.Fatal(err) } - if l, err := listen(path); err == nil { - _ = l.Close() - t.Fatal("listen replaced a regular file, want an error") - } - - if _, err := os.Stat(path); err != nil { - t.Errorf("the file was removed anyway: %v", err) - } + assertListenRefuses(t, path) } -// A socket the previous run failed to clean up must still be reclaimable, -// otherwise a crashed worker would poison the endpoint for every retry. -func TestListenReclaimsAStaleSocket(t *testing.T) { - dir := shortTempDir(t) - path := filepath.Join(dir, "stale") +// Nothing of ours outlives the pod at these paths, so both cases below are +// anomalies rather than something to tidy up -- and they are indistinguishable +// on disk anyway, which is why the distinction is no longer attempted. +func TestListenRefusesAnEndpointThatAlreadyExists(t *testing.T) { + t.Run("left behind by a dead listener", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "stale") + + // Go unlinks the socket on Close, so the only endpoint left on disk is + // one nobody closed. SetUnlinkOnClose reproduces that without having to + // crash a process: the file stays, the listener does not. + stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + if err != nil { + t.Fatal(err) + } + stale.SetUnlinkOnClose(false) + if err := stale.Close(); err != nil { + t.Fatal(err) + } - // Go unlinks the socket on Close, so the only endpoint left on disk is one - // nobody closed. SetUnlinkOnClose reproduces that without crashing a - // process: the file stays, the listener does not. - stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) - if err != nil { - t.Fatal(err) - } - stale.SetUnlinkOnClose(false) - if err := stale.Close(); err != nil { - t.Fatal(err) - } - if _, err := os.Lstat(path); err != nil { - t.Fatalf("the endpoint should still be on disk: %v", err) - } + assertListenRefuses(t, path) + }) - l, err := listen(path) - if err != nil { - t.Fatalf("listen over a stale socket: %v", err) - } - _ = l.Close() + t.Run("still being served", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "live") + + live, err := net.Listen("unix", path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = live.Close() }() + + assertListenRefuses(t, path) + }) } -// The case above is indistinguishable from this one by mode alone, and taking -// the path from a peer that is still listening would strand it silently. -func TestListenRefusesALiveSocket(t *testing.T) { - dir := shortTempDir(t) - path := filepath.Join(dir, "live") +func assertListenRefuses(t *testing.T, path string) { + t.Helper() - live, err := net.Listen("unix", path) - if err != nil { - t.Fatal(err) + if _, err := os.Lstat(path); err != nil { + t.Fatalf("the endpoint should be on disk before the call: %v", err) } - defer func() { _ = live.Close() }() if l, err := listen(path); err == nil { _ = l.Close() - t.Fatal("listen replaced a live socket, want an error") + t.Fatal("listen bound over an existing endpoint, want an error") } if _, err := os.Lstat(path); err != nil { - t.Errorf("the live endpoint was removed anyway: %v", err) + t.Errorf("the endpoint was removed anyway: %v", err) } } -// Refusing to unlink a live endpoint is not enough on its own: the probe that -// establishes it is live is itself a connection, and these endpoints serve one. -// If the probe were mistaken for the peer, the listener it just declined to -// evict would be handed an empty payload and left with nothing to wait for. +// The endpoints here serve exactly one connection, and the volume is shared +// with a scanner image we do not control, so a connect that says nothing must +// not be mistaken for the peer -- doing so would hand the listener an empty +// payload and leave the real worker with nothing waiting for it. func TestAwaitIgnoresAConnectThatSaysNothing(t *testing.T) { path := filepath.Join(shortTempDir(t), "complete") ctx := testContext(t) @@ -164,11 +160,10 @@ func TestAwaitIgnoresAConnectThatSaysNothing(t *testing.T) { } defer func() { _ = pipe.Close() }() - // the listener is already published, so this is queued ahead of the peer -- - // exactly what listen's staleness probe does to a live endpoint - probe, err := net.DialTimeout("unix", path, stalenessProbe) + // the listener is already published, so this is queued ahead of the peer + probe, err := net.DialTimeout("unix", path, time.Second) if err != nil { - t.Fatalf("probing the endpoint: %v", err) + t.Fatalf("connecting to the endpoint: %v", err) } if err := probe.Close(); err != nil { t.Fatal(err)