feat: make the worker handoff write path cancellable - #1237
Conversation
Follow-up to eraser-dev#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 <yuewu2@microsoft.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 37 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Makes worker handoff writes context-aware and protects Windows socket paths from replacing non-socket files.
Changes:
- Adds cancellable image and completion writes.
- Handles SIGTERM in collector/remover.
- Adds Windows endpoint safety and cancellation tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/collector/collector.go |
Passes a signal-aware context to handoff writes. |
pkg/remover/remover.go |
Applies cancellation to reads and completion writes. |
pkg/scanners/template/scanner_template.go |
Uses the configured context when sending images. |
pkg/utils/handoff_unix.go |
Adds interruptible FIFO opening. |
pkg/utils/handoff_windows.go |
Adds context-aware dialing and safer socket replacement. |
pkg/utils/handoff_test.go |
Tests canceled handoff writes. |
pkg/utils/platform_windows_test.go |
Tests occupied and stale socket handling. |
pkg/utils/utils.go |
Preserves the legacy indefinite-write wrapper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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 <yuewu2@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/utils/handoff_test.go:91
- This still calls
WriteCompletionPipewith a live context, so it does not cover the newly documented precedence rule that a missing endpoint must returnos.IsNotExisteven when the context is already canceled. That distinction drives remover control flow and is the reason for the new pre-openStat; exercise it here so both platform implementations cannot regress to returningcontext.Cancelednondeterministically.
err := WriteCompletionPipe(context.Background(), path)
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 <yuewu2@microsoft.com>
|
@ashnamehrotra one for you rather than something I want to decide unilaterally. Copilot's remaining comment on The mechanism. Cancelling Why I haven't just fixed it. Three things:
So the question is which you'd prefer:
I'd pick (a) now and (c) later if a caller ever needs to survive cancellation, but this is a judgement about how much Linux behaviour change you want in a Windows port, and that's yours to make rather than mine. Happy to do any of the three. |
|
Answering the suppressed comment on Good catch, and the more useful kind: the test existed and looked like it covered this, which is worse than having no test. The pre-open Added |
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 <yuewu2@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
pkg/utils/handoff_unix.go:103
os.File.Closedoes not interrupt a FIFOwrite(2)that is already blocked in the Linux kernel. If this payload fills the pipe while the reader remains open but stops draining,file.Writestays blocked afterctxis canceled, so the write path is still not cancellable as promised. This needs a nonblocking/pollable write that handles partial writes andEAGAIN, or another mechanism that can actually interrupt and join the pending write; closing the same descriptor from this watcher is insufficient.
case <-ctx.Done():
_ = file.Close()
case <-done:
pkg/remover/remover.go:45
- Registering signal notification here suppresses the default SIGTERM action during
cri.NewRemoverClient, but that constructor performs CRI Version RPCs withcontext.Background()(pkg/cri/client.go:36-44,82-87). If the runtime accepts the connection without answering, startup can block indefinitely and termination now waits for SIGKILL. MoveNotifyContextuntil after client creation, or propagate this context into the constructor as well.
// 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)
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 <yuewu2@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
pkg/utils/handoff_windows.go:120
- The cancellation watcher is still running after this context check. If
ctxis canceled after line 118 but before the return expression finishes, the watcher andconn.Close()race: success or a closed-connection error is returned depending on which close wins, rather thanctx.Err(). Signal completion to the watcher and join it immediately afterWrite, then inspect the context and close the connection, so no cancellation close can occur after the check.
// 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
pkg/utils/handoff_unix.go:113
- The cancellation watcher remains active after this context check. If cancellation occurs after line 111 but before
file.Close()completes, the watcher and the main goroutine race to close the file, so this can return success oros.ErrClosedinstead ofctx.Err()depending on scheduling. Close the done signal and join the watcher immediately afterWritebefore checking the context and closing the file.
// 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
pkg/utils/handoff_windows.go:208
- The type check does not show that this is a stale socket or that it belongs to this worker. A live listener also has
ModeSocket—in fact,TestListenReclaimsAStaleSocketkeeps its first listener open while callinglistenagain—so this removes a reachable endpoint and rebinds over it. Because the directory is shared with an untrusted scanner, theLstat/Removegap also lets that process replace the checked socket with another entry before SYSTEM deletes it. Refuse live sockets and reclaim only an endpoint proven stale using a race-safe strategy.
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
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 <yuewu2@microsoft.com>
| // 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) |
There was a problem hiding this comment.
Correct. Fixed in 822f166c.
Deriving the deletion timeout from the signal context is what created it: before this branch removeImages built its own budget from context.Background(), so cancellation could not reach it and could not be misreported by it. Once it can, nil from removeImages stops meaning "the images are gone".
main now refuses to treat the removal as successful without checking:
// 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)
}I took your fallback rather than the first suggestion deliberately, because the two are at different layers and this PR only owns one of them. Returning the context error from inside the delete branches also stops the loop early, which is a behavior change to the removal loop itself; the stacked #1239 makes it, with the tests for it, because that PR is what gives each deletion its own budget and therefore has to distinguish "this image ran out of time, carry on" from "the caller is gone, stop". Here the only defect is the exit status, so that is all that changes: an interrupted run still walks the remaining list logging instant failures, but it can no longer exit 0 while doing it.
| // 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) |
There was a problem hiding this comment.
Right, and it inverts the point of the PR. Fixed in 822f166c.
The read is only interruptible while the endpoint is missing. Once the peer publishes it, os.OpenFile blocks in the kernel on the FIFO, and on Windows the watcher closes the listener — which does nothing for a connection already accepted and sitting in io.ReadAll. So a peer that published and then stalled left the remover ignoring SIGTERM until the kubelet's SIGKILL, where before this branch it died immediately. A PR whose stated purpose is "a terminating pod shouldn't wait for the kill" made that case strictly worse.
Taking your first option — registration now starts after the read:
// 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)Everything after that point — removeImages, the metrics calls, both WriteCompletionPipe calls — takes ctx and observes it, so the handler covers exactly the calls that can act on it. That is also where main had it before this branch; moving it to the top was my own overreach and nothing required it.
Not the second option, at least not here. Making the read genuinely cancellable means giving up the blocking FIFO open, and that open is the rendezvous every existing deployment depends on — the same trade I argued against for the write side in the description, with the same problem that I cannot execute the Unix path locally to check it. The default disposition is also better than partial cancellation: it terminates immediately, with none of my code in the path.
Worth noting the collector already had the rule written down, and I simply failed to apply it to the remover:
// 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.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 <yuewu2@microsoft.com>
| // 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) |
There was a problem hiding this comment.
Correct — the coverage was theatre. Fixed in 768d5bfc.
Every case in TestRemoveImages passes a context that is never done, and the fake discarded the argument outright, so reverting context.WithTimeout(ctx, timeout) to context.WithTimeout(context.Background(), timeout) left the suite green. That is the one thing this hunk exists to do.
The fake now observes its context, which is what a real client does anyway, and the propagation is pinned by behavior rather than by inspection:
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"})
...
}Against context.Background() it reports removed = 1, want 0 and the image was deleted for a caller that was already gone.
I went with an already-cancelled caller rather than the blocking fake you suggested, because a blocking fake asserts that cancellation unblocks removeImages, and here nothing blocks — DeleteImage is a call, not a wait, and the loop has no guard in this PR. The question this hunk raises is narrower: does the runtime see the caller's context or a detached one. An already-dead context answers exactly that, without inventing a blocking behavior the real CRI client doesn't have.
Worth flagging for when you look at the stacked #1239: it adds a guard that returns before the loop reaches the runtime at all, which makes this test assert the wrong thing there, so it is removed in the commit that introduces the guard. The propagation stays covered there by TestRemoveImagesSurfacesCancellationDuringTheFinalDeletion, which cancels during a deletion and so still requires the runtime to be holding the caller's context.
| 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) { |
There was a problem hiding this comment.
The race is real and I said so when I added the probe, but I don't think it reaches the impact described, so I'd rather argue it than implement handle-based deletion on a guess.
Two things bound it:
The path is a compile-time constant. There is no traversal component and nothing attacker-influenced in it, so the entry that gets deleted is always inside the shared volume.
os.Remove does not follow reparse points. It is DeleteFileW then RemoveDirectoryW, both of which act on the link rather than its target. Swapping the socket for a symlink or junction gets the link deleted, not whatever it points at — so this cannot be turned into "make SYSTEM delete a file outside the volume", which is the version that would actually matter.
What's left is that a scanner which wins a sub-millisecond race can cause SYSTEM to delete a file in a directory the scanner already has write access to, and could therefore delete itself. That is a correctness wart, not a privilege boundary: the stated guarantee is weaker than I implied, but nothing crosses a trust boundary when it fails.
That said, the guarantee is weaker than advertised, and I'd rather the code say what it does. Two options, and I'm happy with either:
- Narrow the claim in the comment to what the check actually provides — a same-process-view guard against replacing a non-socket, not an atomic one.
- Do the identity-checked delete: open with
FILE_FLAG_OPEN_REPARSE_POINT, confirm the tag isIO_REPARSE_TAG_AF_UNIXthroughGetFileInformationByHandleEx, then delete viaFileDispositionInfoon that handle.
I'd want to do (2) on a real Windows node rather than from reasoning about the API, since it is x/sys/windows surface with no coverage in this repo today, and it is orthogonal to what this PR is about. If you'd like it, I'd rather it were its own PR with its own validation. Tell me which you prefer and I'll do it.
There was a problem hiding this comment.
This is resolved rather than narrowed now, and not by the handle-based delete I offered.
listen no longer deletes anything at all — the reclaim it existed to serve was defending a case that cannot occur, because the shared volume is an emptyDir created with the pod and restartPolicy is Never, so no retry inherits a socket. Details in b833fd95 and the thread on line 243.
With no os.Remove, there is no Lstat/Remove window to race. My earlier answer argued the residual race had no privilege impact; that argument is now moot, which is a better outcome than being right about it.
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 <yuewu2@microsoft.com>
The suppressed comments were right tooBoth rounds hid findings behind The write watcher outlived the context check ( The check ran while the watcher goroutine was still armed, so a cancellation landing between the check and the function's own The suggestion to join the watcher first is what I took, with one addition: the watcher now reports whether it did the closing, so the caller stops inferring that from the error. go func() {
select {
case <-ctx.Done():
_ = file.Close()
closedByWatcher <- true
case <-done:
closedByWatcher <- false
}
}()
_, err := file.Write(payload)
close(done)
if <-closedByWatcher {
return ctx.Err()
}After the join no cancellation close can still land, so "who closed this handle" is answered rather than guessed. Same shape on both platforms.
This is the one I'd have argued with if the test hadn't given it away. And you're right that
// 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)
}A local connect answers immediately if anything is bound, including when no The test now builds a genuinely abandoned endpoint with On the Verified: |
| 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) |
There was a problem hiding this comment.
Right, and it's a defect I introduced last round rather than one that was already there. Fixed in 080a917a.
The probe is a connection, and these endpoints serve exactly one. So listen would correctly decline to unlink a live socket, and the listener it had just protected would accept the probe, read zero bytes, and be left with nothing still to come — the worker stranded either way, only now with a confusing empty payload instead of a missing endpoint.
Await and ReadImagesPipe now keep accepting until something actually says something:
data, err := io.ReadAll(conn)
_ = conn.Close()
if err != nil {
return nil, err
}
// A connect that says nothing is not the peer [...]
if len(data) == 0 {
continue
}I took the accept-side option rather than "avoid connecting", because I don't think a non-connecting liveness check exists here that is worth the complexity: the mode is identical, the bind fails either way, and anything file-based reintroduces its own staleness question. The accept side is also where this belonged regardless of the probe — the volume is shared with a scanner image we don't control, so the peer was never the only thing that could connect, and a single stray connect could already have consumed the handoff before this PR existed. Distinguishing by payload is what makes that safe, and nothing legitimate sends an empty one: the image list is JSON, the completion message is a constant.
TestAwaitIgnoresAConnectThatSaysNothing queues a connect ahead of the peer exactly the way listen does, against a listener that is already published so the ordering is deterministic. Without the skip it fails with:
payload = "", want "complete" -- the probe was taken for the peer
There was a problem hiding this comment.
Following up: the probe this thread was about is gone entirely as of b833fd95 — a later review round showed that a failed probe doesn't prove staleness either (a live listener with a full backlog refuses), and the reclaim it served was defending a case that cannot occur. See the thread on line 243.
The accept-side fix from this thread stays, and it's worth saying why now that its original motivation has been deleted: the volume is shared with a scanner image we don't control, so the peer was never the only thing that could connect. A stray connect could consume the handoff independently of anything listen did. TestAwaitIgnoresAConnectThatSaysNothing keeps that pinned; only its comment changed, to stop citing the probe as the reason.
|
The one red check here —
A real regression from these changes would fail across all four versions — that is exactly how the earlier Happy to push a no-op to re-trigger if you'd rather see it green than take my word for it. |
1 similar comment
|
The one red check here —
A real regression from these changes would fail across all four versions — that is exactly how the earlier Happy to push a no-op to re-trigger if you'd rather see it green than take my word for it. |
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 <yuewu2@microsoft.com>
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 <yuewu2@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/utils/handoff_unix.go:117
- The watcher can still lose a cancellation due to
selectscheduling. Ifctxis canceled after this goroutine is started but before it reaches theselect, and the write then closesdone, both cases are ready and Go may choosedone; this helper reports success even though cancellation happened before the message was framed. Recheckctx.Err()after joining the watcher and before closing/reporting success.
close(done)
if <-closedByWatcher {
return ctx.Err()
pkg/utils/handoff_windows.go:143
- The watcher can still lose a cancellation due to
selectscheduling. Ifctxis canceled before this goroutine reaches itsselect, a fast write can closedonefirst from the goroutine's perspective; with both cases ready, Go may choosedoneand this returns success after cancellation. Recheckctx.Err()after joining the watcher and before closing/reporting success.
close(done)
if <-closedByWatcher {
return ctx.Err()
pkg/utils/handoff_test.go:62
- Passing
ctxhere does not actually enforce the new 30-second deadline once the Unix reader entersOpenFile/ReadAll, or once the Windows reader entersReadAll; those phases do not observe the context. A regression that publishes/connects but never finishes can therefore still hang this test until the package timeout. Run the read in a goroutine and select onctx, and similarly bound the writer-result receive.
got, err := ReadImagesPipe(ctx, path)
if err != nil {
t.Fatalf("ReadImagesPipe: %v", err)
}
if err := <-errCh; err != nil {
A pre-existing hang in
|
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 <yuewu2@microsoft.com>
| // 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 { |
There was a problem hiding this comment.
Could an empty connection return a protocol error instead of waiting for another connection? listen no longer creates probe connections, so an empty read now means the writer exited or was canceled before sending. Since it will not reconnect, could this leave the peer blocked until job cleanup?
There was a problem hiding this comment.
Good catch and the justification for that loop expired earlier in this PR without my noticing, fixed now
| // 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() |
There was a problem hiding this comment.
Could SIGTERM be lost between this ctx.Err() call and stopSignals()? If it arrives in that window, notification can consume it after sigErr captured nil, and the collector then enters uncancellable Await(). Could we remove that race or make the wait cancellable?
There was a problem hiding this comment.
Sure we can, updated to remove the race.
On the second option, I think making Await take a context is the better state and. However, I'd like to do in a follow up pr if that is okay to you because it changes the handoff API on both platforms, and on the Unix side the read carries the same partial-cancellation caveat as the write
| go func() { | ||
| select { | ||
| case <-ctx.Done(): | ||
| _ = file.Close() |
There was a problem hiding this comment.
Is this intended to support every !windows target or only Linux? On Darwin, closing the file here does not wake the blocked Write, and the added test fails after 30 seconds. Should this use nonblocking I/O or be scoped to supported platforms?
There was a problem hiding this comment.
Thanks for actually running it on Darwin — I couldn't have caught that from here. Scoped in 247f10e.
The honest answer to "every !windows target or only Linux" is: the rendezvous half of cancellation works anywhere, and the mid-write half is Linux-only. Closing a FIFO wakes a blocked write on Linux; it doesn't on Darwin, so the test sat there for its full 30 seconds asserting something the platform doesn't provide.
I've kept the file at !windows rather than narrowing to linux, because restricting it would stop the package building on a developer's Mac for no gain — the code is correct there, just weaker. The comment now says which half is guaranteed where, and the test skips off Linux instead of quietly failing:
if runtime.GOOS != "linux" {
t.Skipf("closing a blocked FIFO write is only guaranteed to unblock it on linux, not %s", runtime.GOOS)
}I'd rather not reach for nonblocking I/O to make it uniform. That means giving up the blocking open that is the rendezvous every existing deployment depends on, for a platform Eraser's workers don't run on — the nodes are Linux and Windows. If that trade looks wrong to you I'll take another run at it, but it felt like a lot of risk to buy Darwin parity in a test.
|
|
||
| go func() { | ||
| //nolint:gosec // G304: Opening pipe file is intended functionality | ||
| file, err := os.OpenFile(path, os.O_WRONLY, 0) |
There was a problem hiding this comment.
Could cancellation also terminate this blocked open? The caller returns, but this goroutine remains in OpenFile until a reader arrives, at which point that reader receives EOF from the abandoned opener. Would a nonblocking open loop avoid the goroutine and OS-thread leak?
There was a problem hiding this comment.
The leak is real and I'm not going to claim otherwise — but I'd like to argue for leaving it, because I think the consequence is smaller than it looks and the fix is riskier than it looks.
Why it's bounded. Every caller of this is a worker whose next move after a cancelled write is os.Exit. The goroutine and its OS thread live until the process does, which is milliseconds later. There's no accumulation: it is one goroutine, once, at the end of a run that is already terminating.
On the spurious EOF. That needs a reader to arrive at the same endpoint after the writer gave up. Within a pod there is exactly one reader per endpoint, and it is either already blocked in open — in which case the rendezvous completed and this path was never taken — or it never arrives at all. Across pods it can't happen: the volume is an emptyDir created with the pod, 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.
Why I'm wary of the nonblocking loop. O_WRONLY|O_NONBLOCK on a FIFO returns ENXIO until a reader is present, so a poll loop is semantically equivalent and genuinely cancellable — you're right that it would work. What it changes is the syscall every existing Linux deployment currently rendezvouses on, in a PR that is already eight commits deep, to remove a goroutine that outlives its process by milliseconds. I've argued against that trade twice in this PR for the payload write and I don't think it gets better here.
Where I'd change my mind: if Await grows a context — which came up on the collector thread too — the read side would need the same treatment, and at that point doing both together in one focused PR is clearly better than doing this half here. I'd rather that than bolt it on now.
Happy to do it either way if you'd prefer it in this PR; I just don't want to make that call unilaterally.
| // 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) |
There was a problem hiding this comment.
Should context.Canceled stop the deletion loop immediately? At present it is handled like an ordinary per-image error, so every remaining DeleteImage call is attempted before main notices cancellation. Could we return the context error here and in the prune loop?
There was a problem hiding this comment.
Agreed, and it exists — in the stacked #1239, with tests. Two commits there do exactly what you describe:
085dc1cadds the guard to the top of both loops, so a cancelled caller stops rather than walking the rest of the list588635dhandles the case the guard can't see: cancellation during the last deletion, where there is no later iteration to reach the guard
The split was deliberate but I'm now the only one who thinks so, which is usually the sign it was wrong. My reasoning was that returning the context error from inside the delete branches changes the loop's behaviour, and #1239 is the PR that has to distinguish "this image ran out of its own budget, carry on" from "the caller is gone, stop" — because it's the one that gives each deletion a budget. Here, without per-image budgets, the only defect is the exit status, so that's all I changed: main checks ctx.Err() after removeImages and exits non-zero, which stops an interrupted run reporting success even though it still walks the remaining list.
You're the second reviewer to raise it against this PR, so if you'd rather #1237 not merge without the early stop, say so and I'll move both hunks down here and rebase #1239 on top. It's a contained change; I just didn't want to duplicate it across two open PRs on my own initiative.
| go func() { errCh <- WriteImagesPipe(ctx, path, want) }() | ||
|
|
||
| got, err := ReadImagesPipe(context.Background(), path) | ||
| got, err := ReadImagesPipe(ctx, path) |
There was a problem hiding this comment.
Could this test enforce the deadline around ReadImagesPipe itself? Once the FIFO opens or the socket accepts, ReadAll does not observe ctx, so this call can still hang until the package timeout. Would running it in a goroutine and selecting on the deadline make the bound effective?
There was a problem hiding this comment.
Good catch and the deadline was only binding on the write half, fixed.
Worth noting the underlying point survives the test fix: the read genuinely is uncancellable past the rendezvous. The test now fails instead of hanging, but that is just a diagnosis improvement, the real answer is Await and the read path taking a context, which is the thread on the collector.
Found in review. The accept side skipped a connection that sent nothing and went back to waiting. That was added when listen probed the endpoint to tell a live socket from a stale one, so an empty connect really was noise. listen no longer probes, and the peer never reconnects, so the only thing an empty read can mean now is that the writer died or was canceled before sending -- and waiting for a second connection waits for one that is not coming. The reader sat there until job cleanup. Await and ReadImagesPipe return ErrEmptyHandoff instead. Nothing legitimate sends an empty payload: the image list is JSON and the completion message is a constant. The loop was also the only thing standing between a stray connect on the shared volume and a silently truncated handoff. Failing the run covers that at least as well as hanging did. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
Found in review. The deadline was only enforced on the write half. ReadImagesPipe ran on the test goroutine, and once the FIFO opens or the socket accepts, the payload read stops observing ctx -- so a peer that connected and then stalled hung here until the package timeout, which is the failure mode the deadline was added to remove. The read moves onto its own goroutine with a select, the same shape the completion round trip already needed for Await. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
Found in review, and confirmed on Darwin by the reviewer. The watcher unblocks a stalled write by closing the file, which is Linux behaviour: closing a FIFO does not wake a write that is already blocked on Darwin, so the test sat there for its full 30 seconds instead of failing fast. The file stays built for every non-Windows target, because restricting it to Linux would stop the package building on a developer's Mac for no gain -- the rendezvous half of cancellation works there, only the mid-write unblock does not. The comment now says which half is guaranteed where, and the test skips off Linux rather than quietly asserting something the platform does not provide. Nonblocking I/O would make it uniform, but that means giving up the blocking open that is the rendezvous every existing deployment depends on, which is a bigger change than this PR should carry. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
… down Found in review. The collector read ctx.Err, then stopped signal delivery. A signal arriving between the two was consumed by the still-registered handler after the check had already recorded nil, so it neither killed the process nor was acted on -- and the collector then entered Await, which takes no context and cannot be interrupted. The window is small and the result is a worker that hangs until job cleanup. The check could not simply move after the stop, because NotifyContext's stop func cancels the same context a signal would, so afterwards the two are indistinguishable. signal.Notify keeps them apart: deregister, then join the watcher, and a signal has either been recorded or is still in the buffer, with no third outcome. Making Await take a context would remove the need for any of this, and I think that is the better end state. It is a change to the handoff API on both platforms, with the same partial cancellation caveats the write side has, so it does not belong in this PR. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
pkg/utils/handoff_windows.go:224
- The PR description still says stale sockets are probed and reclaimed, and lists
TestListenReclaimsAStaleSocket, but this implementation now deliberately refuses every existing endpoint and the test asserts that behavior. Please update the description and testing table to reflect that stale endpoints are not reclaimed; otherwise reviewers and release notes describe the opposite lifecycle behavior from the code.
// 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.
| signal.Stop(sigCh) | ||
| cancel() | ||
| <-watching |
Follow-up to #1229 / #1231. Closes the two threads @ashnamehrotra and Copilot left open on #1231.
1. The write path can now be cancelled
ReadImagesPipealready took a context; the write side didn't, so a worker whose peer never arrived waited forever with no way out.WriteImagesPipeandWriteCompletionPipenow take one, and the collector and remover derive theirs fromSIGTERM— a terminating pod actually unblocks the worker instead of waiting for the kill.I changed my mind about how to do this on Linux, and it's worth explaining. On #1231 I proposed
O_NONBLOCK+ polling. Having written it, I don't think that's the right trade:open()syscall every existing deployment depends on for rendezvousEAGAINon any payload larger than the pipe buffer, which a large image list will exceedSo the blocking open is untouched. It runs on a goroutine that hands the file back if the caller is still waiting and closes it if not:
Linux keeps the exact syscall and the exact rendezvous it has always had. Only the waiting became interruptible. On Windows,
dialForeverbecomesdial(ctx, …)and usesDialContext.One subtlety worth flagging.
WriteCompletionPipenowstats the path before opening. Left to theselect, a peer that was never published and an already-done context would race, and Go would pick a winner at random — making "scanner disabled" indistinguishable from "we're shutting down". That's the signalpkg/removeruses to decide whether a scanner exists, so it can't be left to chance. This also makes the two implementations symmetric, since Windows already had tostatfirst.Where the handler gets registered matters, and review caught me getting it wrong.
signal.NotifyContextsuppresses the default SIGTERM exit, so it is now registered only across the calls that actually observe the context. The remover's wait for the scanner is not one of them: once the peer publishes its endpoint, the read blocks inos.OpenFileon the FIFO or inio.ReadAllon an already-accepted socket, and no context reaches either. Registering at the top ofmaintherefore traded an immediate default exit for a wait until SIGKILL — precisely the failure this PR claims to remove. Making the read itself cancellable means giving up the blocking open, which is the rendezvous; that is a separate change, and not one to make while unable to run the Unix path.WriteScanErasePipekeeps its signature and waits indefinitely, so out-of-tree scanners are unaffected.2.
listenno longer deletes things that aren't oursIt removed whatever sat at the endpoint path before binding. A socket left by an unclean exit does have to go — otherwise a crashed worker poisons the endpoint for every retry — but anything else there isn't ours to delete: the worker runs as
NT AUTHORITY\SYSTEMand shares the volume with a scanner image we don't control.LstatreportsModeSocketon Windows, so a regular file is refused outright. That alone isn't enough, and review caught the gap: a live socket has exactly the same mode, so the first version would still have unlinked a listener that was serving and rebound over it, stranding its peer with no error anywhere. The endpoint is now probed with a connect first — anything that answers is live, and live means refuse. Only an endpoint nobody is bound to gets reclaimed.Go unlinks the socket on
Close, so a genuinely stale endpoint only exists after an unclean exit; a clean shutdown leaves nothing behind at all.Testing
Six new tests. Those in the untagged file run against both implementations:
TestWriteImagesPipeHonoursACanceledContextTestWriteImagesPipeHonoursCancellationWhileBlockedOnAStalledReaderTestWriteCompletionPipeAbsentPeerBeatsACanceledContextTestListenRefusesToReplaceANonSocketTestListenReclaimsAStaleSocketTestListenRefusesALiveSocketThe two round trips also gained a 30 second deadline. They passed
context.Background()to both halves, and both halves block until the peer arrives — so a rendezvous that failed to complete hung until the package-wide ten minute timeout, with no indication of which test was stuck. A local run did exactly that. The cancellation tests already had the guard; the round trips were the gap.Verified locally:
GOOS=linuxandGOOS=windowsbuild + vet clean,golangci-lintclean on both, fullgo test ./pkg/...green natively on Windows.Standalone E2E test results
Upstream has no Windows CI, so this was validated on a personal fork and against a real AKS Windows Server 2022 node, same harness as #1231.
Harness — where the tooling lives
hack/ipcspike.github/workflows/windows-ci.yamlhack/windows-e2e.ps1Unit tests exercise the handoff inside one process, which is not the question that matters for a rendezvous change.
ipcspikeruns this PR's actualpkg/utilsAPI from two containers of one pod over anemptyDir: the producer publishes its completion endpoint, hands over an image list and waits; the consumer reads the list, checks that an unpublished endpoint is still reported asIsNotExist, then signals back.Environment
aksnpwin000004, Windows Server 2022 Datacenter, build 10.0.20348.5386mcr.microsoft.com/windows/nanoserver:ltsc2022runAsUserName: NT AUTHORITY\SYSTEMemptyDirmounted into both containersLive results — cross-container run on
ee5188fc, the first commit here; fork CI has been re-run on every commit sinceCross-container handoff, two containers of one pod over an
emptyDir:The 18s on the consumer side is the deliberate stagger in the producer container's command; it is the listener waiting, not latency.
The package's own tests, cross-compiled for
windows/amd64and run on the same node:Fork CI, latest run — commit
5e7080b2, all six jobs:Two observations from the run
The Linux half is verified by CI, not by me. I develop on Windows, so
handoff_unix.gocompiles and vets locally but never executes here — and the goroutine-based open is precisely the half I cannot run. Thelinux unaffectedjob runsgo build ./...plusgo test ./pkg/... ./api/... ./controllers/...on Ubuntu, so the new cancellation test did execute against the FIFO implementation:The signature change reaches out-of-tree callers. The fork's cross-container harness calls these functions directly and stopped compiling when the context parameter was added — caught by CI, not by anything local. Nothing in this PR needed changing, but it is the concrete argument for leaving
WriteScanErasePipealone: anything outside this repo calling it keeps working untouched.Still open, deliberately
CompletionPipe.Awaittakes no context and blocks the same way. It's the read side rather than the write side @ashnamehrotra asked about, and it needs the same care, so I've left it out rather than growing this PR. The collector already works around it by stopping signal delivery before the wait, and the completion round-trip test has to enforce its deadline with aselectfor the same reason — both are arguments for doing it. Happy to do it next if you'd like it.