feat: make the worker handoff write path cancellable - #1237
Open
charleswool wants to merge 1 commit into
Open
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>
charleswool
requested review from
ashnamehrotra,
pmengelbert and
sozercan
as code owners
August 25, 2026 03:31
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 38 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.
| err error | ||
| } | ||
|
|
||
| ch := make(chan opened, 1) |
| // 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) |
| // 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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.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 the two cases are separable. Worth noting Go unlinks the socket onClose, so this only matters after an unclean exit; a clean shutdown leaves nothing behind at all.Testing
Three new tests, all in the untagged file where they run against both implementations unless noted:
TestWriteImagesPipeHonoursACanceledContextTestListenRefusesToReplaceANonSocketTestListenReclaimsAStaleSocketVerified 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 — commit
ee5188fc, the head of this PRCross-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 on the same commit, 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. Happy to do it next if you'd like it.