feat: run the worker handoff over Unix sockets on Windows - #1231
feat: run the worker handoff over Unix sockets on Windows#1231charleswool wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Centralizes worker handoff IPC and adds a Windows Unix-domain-socket implementation while preserving Unix FIFOs.
Changes:
- Moves collector/scanner/remover handoff logic into
pkg/utils. - Adds platform-specific IPC and cross-platform tests.
- Includes stacked Windows named-pipe CRI support.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/utils/utils.go |
Adds shared handoff wrappers and endpoint parsing. |
pkg/utils/utils_test.go |
Tests Unix and named-pipe endpoints. |
pkg/utils/platform_windows.go |
Adds Windows CRI dialing. |
pkg/utils/platform_windows_test.go |
Tests Windows platform behavior. |
pkg/utils/platform_unix.go |
Isolates Unix platform behavior. |
pkg/utils/platform_unix_test.go |
Tests Unix platform behavior. |
pkg/utils/handoff_windows.go |
Implements socket-based Windows handoff. |
pkg/utils/handoff_unix.go |
Encapsulates FIFO-based Unix handoff. |
pkg/utils/handoff_test.go |
Adds portable handoff tests. |
pkg/scanners/template/scanner_template.go |
Uses the shared completion API. |
pkg/remover/remover.go |
Uses shared read and completion APIs. |
pkg/collector/collector.go |
Uses shared write and completion APIs. |
go.mod |
Adds go-winio. |
go.sum |
Records dependency checksums. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if p.l == nil { | ||
| return nil | ||
| } | ||
| return p.l.Close() |
There was a problem hiding this comment.
Fixed in 8c925e11.
You're right, and the test was the worse half of this — it was named for a property it never exercised.
Close now clears the listener before closing it, so a repeat call takes the existing nil guard and reports success. TestCompletionPipeCloseIsIdempotentlySafe now actually closes twice.
To be accurate about severity though: no current caller closes twice. pkg/collector closes explicitly and pkg/scanners/template defers, so neither doubles up today. This was the documented contract disagreeing with the implementation, waiting for a future caller — not a live bug. The Unix side is a no-op and was already idempotent, which is exactly how the two implementations drifted apart without the test noticing.
|
|
||
| // maxSocketPath is the sun_path limit. Exceeding it fails deep inside the | ||
| // syscall with an opaque error, so it is checked up front. | ||
| const maxSocketPath = 108 |
There was a problem hiding this comment.
Fixed in 8c925e11.
Confirmed rather than assumed — I probed net.Listen("unix", ...) directly on Windows with paths of increasing length:
len=106 OK
len=107 OK
len=108 FAIL listen unix ...: bind: invalid argument
len=109 FAIL listen unix ...: bind: invalid argument
So 107 is the real ceiling, exactly as you said, and 108 produced precisely the opaque bind: invalid argument that the guard exists to replace with a useful message.
maxSocketPath is now 107, with the reasoning in the comment. Added TestSocketPathLimitBoundary (Windows-only) which asserts both sides — a 107-byte path binds, a 108-byte path is rejected by the guard — so the limit can't quietly drift back.
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
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/utils/handoff_windows.go:67
p.lis never cleared, so a secondClosecallsnet.Listener.Closeagain and returns an already-closed error on Windows. This contradicts the stated idempotent behavior (and the test named for it currently callsCloseonly once). Clear the field before closing so repeated calls are safe.
func (p *CompletionPipe) Close() error {
if p.l == nil {
return nil
}
return p.l.Close()
pkg/utils/handoff_windows.go:162
- A pathname must leave room for the NUL terminator in the 108-byte
sun_pathfield, so 107 bytes is the largest valid path. This guard allows a 108-byte path and therefore fails to provide the promised up-front validation;net.Listenwill still fail with the opaque syscall error. Rejectlen(path) >= maxSocketPath.
if len(path) > maxSocketPath {
return nil, fmt.Errorf("socket path %q is %d bytes, over the %d byte limit", path, len(path), maxSocketPath)
}
pkg/utils/handoff_windows.go:180
- The same boundary issue applies on the dialing side: a 108-byte pathname cannot be NUL-terminated in
sun_path, but this check allows it and exposes a low-level dial failure instead of the intended validation error. Rejectlen(path) >= maxSocketPath.
if len(path) > maxSocketPath {
return nil, fmt.Errorf("socket path %q is %d bytes, over the %d byte limit", path, len(path), maxSocketPath)
}
The collector, scanner and remover hand images off through four named pipes in a shared volume, but only two of the eight operations lived in pkg/utils. The rest were written out inline across three binaries, so the protocol could not be reasoned about, tested, or reimplemented in one place. Move all of them behind CreatePipe, WriteImagesPipe, ReadImagesPipe, WriteCompletionPipe and ReadCompletionPipe. ReadCollectScanPipe and WriteScanErasePipe remain as thin wrappers, since custom scanners may call them directly. No behaviour change: the same syscalls run in the same order, callers keep their own logging and exit codes, and the payload validation stays with the caller so existing handling of unexpected content is preserved. Two log messages differ slightly where an error path was merged. A side effect worth noting: because the FIFO calls now route through the per-GOOS mkfifo shim, pkg/utils, pkg/collector and pkg/scanners/template compile for GOOS=windows for the first time. They do not yet *work* there -- mkfifo returns ErrFifoUnsupported -- so Windows images should not be published until the transport lands. Only the manager still fails to build for Windows, on inotify, and it is Linux-only by design. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
Windows has no filesystem FIFO, so pkg/collector, pkg/utils and pkg/scanners/template built for GOOS=windows but died on the first mkfifo. Implement the handoff over AF_UNIX instead, which Windows has supported since Server 2019 and which Go's net package exposes there. Linux is untouched: handoff.go moves to handoff_unix.go behind a build tag, byte for byte. Unix sockets were chosen over Windows named pipes because they need no new dependency, the endpoint lives in the pod's own volume rather than a machine-global namespace, and the endpoint stays a file -- so the remover can still infer that the scanner is disabled from its absence. Responsibility inverts between the two implementations: with a FIFO the writer creates the endpoint and the reader polls for it, while with a socket the reader listens and the writer dials. The exported API hides that, so callers are unchanged. Two Windows behaviours worth recording, both found by testing rather than by reading docs: Dialing a socket that does not exist reports WSAECONNREFUSED, not ENOENT, and WSAECONNREFUSED does not match syscall.ECONNREFUSED. Neither os.IsNotExist nor errors.Is(fs.ErrNotExist) matches it. So "did the peer ever publish this endpoint?" is answered with os.Stat, and the writer's retry loop does not classify errors at all. A dial succeeds as soon as a listener exists, even with no Accept pending, so the writer is not blocked as it would be on a FIFO. The handoff still completes in order because the reader gets the buffered payload and EOF on close. The new tests are deliberately build-tag free: they exercise whichever implementation the platform selects, so the two cannot drift apart. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
Both found in review. sun_path is 108 bytes including the terminating NUL, so a pathname can use at most 107. The guard allowed 108 through to the opaque "bind: invalid argument" it exists to prevent. Confirmed on Windows: a 107-byte path binds, a 108-byte one does not. Close now clears the listener, so a second call reports success rather than net.ErrClosed. That is a real path, not a hypothetical one: the scanner defers Close while the collector also closes explicitly. The existing test only closed once despite its name, so it did not cover what it claimed. It now closes twice, and a new Windows-only test pins the 107/108 boundary so the limit cannot drift back. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
6c1e318 to
8c925e1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
pkg/scanners/template/scanner_template.go:65
- On Windows,
CreateCompletionPipeowns a live listener, but any laterChmodorReadCollectScanPipeerror returns without closing it. In particular, context cancellation leaves the scanner completion endpoint published even thoughReceiveImagesfailed, so the remover may signal a scanner that has abandoned the handoff. Close the endpoint unless initialization completes successfully.
// published up front so the remover can tell a scanner is present
cfg.completion, err = util.CreateCompletionPipe(util.EraseCompleteScanPath)
pkg/utils/handoff_unix.go:126
ReadImagesPipenever closes the FIFO on Unix. This regresses the remover path, which previously closedfafterio.ReadAll, and leaves an open descriptor for every successful call to this now-exported helper. Close the file on both read success and failure, propagating the close error when the read itself succeeded.
data, err := io.ReadAll(f)
if err != nil {
return nil, err
}
Found in review. The collector handed over the image list and only then created the endpoint it waits on. The peer can read that list and signal completion before the endpoint exists, in which case the remover fails its completion write and exits, while the collector blocks in Await forever. This is pre-existing on main and affects Linux identically: a FIFO that has not been created yet reports ENOENT from open() exactly as a missing socket reports it from stat(). The window is one syscall wide on either platform, which is why it has not been hit in practice. The scanner already publishes its completion endpoint before reading, so this makes the collector consistent with the rest of the codebase. It does change Linux ordering, which the PR description now calls out rather than continuing to claim Linux is untouched. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/utils/handoff_unix.go:126
- The opened FIFO is never closed. This regresses the remover path, which previously closed its descriptor, and leaks the descriptor on both successful reads and JSON errors. Close it immediately after
ReadAll(while still propagating close failures) before unmarshalling.
data, err := io.ReadAll(f)
if err != nil {
return nil, err
}
pkg/utils/handoff_unix.go:66
Closeleaves the FIFO published on Unix despite the API promising to release the endpoint. Consequently, after a successfulClose, recreating the same completion pipe still fails withEEXIST, unlike the Windows implementation. Remove the FIFO here while treating an already-removed path as success so the lifecycle and idempotency contract is consistent across platforms.
This issue also appears on line 123 of the same file.
func (p *CompletionPipe) Close() error {
return nil
Found in review. ReadImagesPipe never closed the file it opened -- not on success, not on a read error, not on a JSON error. The remover closed that descriptor before this refactor, so it is a regression rather than an inherited bug. Three neighbours had the same class of leak on their error paths: Await, WriteImagesPipe and WriteCompletionPipe each returned early on a failed read or write without closing. All four now close on every path and propagate the close error only when the operation itself succeeded, which preserves the previous behaviour of surfacing a failed close. The socket implementation was already clean here; only the ported FIFO code leaked. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
|
Both suppressed comments from the latest review, addressed — one accepted and extended, one declined with reasoning. 1.
|
| return net.Listen("unix", path) | ||
| } | ||
|
|
||
| // dialForever waits for the reader to start listening. Errors are not |
There was a problem hiding this comment.
instead of dialForever should this be part of a context?
|
@charleswool do you think we can add a windows CI test case? |
Note
Rebased onto
mainnow that #1229 has merged, so the diff is now just this work: 8 files, three commits.9daeb947refactor: gather the worker handoff intopkg/utils— pure move, no behavior changeed03cad6feat: run the worker handoff over Unix sockets on Windows8c925e11fix: address review — socket path limit andCloseidempotency118719d4fix: address review — publish the collector completion endpoint before the payloadWhat
Makes the collector -> scanner -> remover handoff work on Windows, which unblocks
pkg/collectorandpkg/scanners/templateforGOOS=windows.Continues the Windows work started in #1229.
Why
The handoff is a set of Unix FIFOs, and the FIFO calls were written inline in three different packages:
pkg/collectorunix.Mkfifo+ blockingopento hand images to the scanner, then a second FIFO to wait for the erasepkg/removerpkg/scanners/templateunix.Mkfifofor its own completion endpointSo
golang.org/x/sys/unixwas imported by three packages that otherwise have nothing platform-specific in them, and any Windows story required touching all three. #1229 stopped at exactly this line for that reason.How
First commit moves the FIFO code behind an API in
pkg/utilswith no behaviour change. Linux is byte-for-byte the same syscall sequence:CompletionPipeis a type rather than a function because the scanner has to publish its endpoint early and read it much later — that gap is the whole mechanism by which the remover detects that a scanner is present.Second commit adds
handoff_windows.go. Windows has no filesystem FIFO, so the transport is a Unix domain socket, supported since Windows Server 2019 and reachable through plainnet.Listen("unix", ...)/net.Dial("unix", ...).A socket preserves the three properties the FIFO implementation leans on:
open()blocks until the peer arrivesAcceptblocks; the writer dials with retryio.ReadAllreturns on closeos.Statstill answersResponsibility is inverted: with a FIFO the writer creates the endpoint and the reader polls for it; with a socket the reader listens and the writer dials.
Why not named pipes
#1229 already vendors
go-winio, so a named pipe was the obvious choice. Unix sockets won on three counts:netcovers it;go-winiostays confined to CRI dialing.\\.\pipe\; a socket is a file in the pod's shared volume, so two ImageJobs on one node cannot collide, and nothing on the node can connect to it.os.IsNotExistkeeps working.pkg/removerdistinguishes "scanner disabled" from a real failure by that check. A missing pipe reports something else entirely.The tradeoff is that the socket path is subject to the
sun_pathlimit, solistenanddialForevercheck it up front rather than failing deep inside the syscall with an opaque error. The field is 108 bytes including the terminating NUL, so a pathname can use at most 107 — verified by probingnet.Listendirectly on Windows, where 107 binds and 108 returnsbind: invalid argument.Not a breaking change
mkfifocalls, same modes, same blocking semantics; the code moved between packages without changing. The exception is118719d4, which reorders two calls in the collector to close a pre-existing race — see below.ReadCollectScanPipeandWriteScanErasePipeare kept as thin wrappers, since out-of-tree scanners may call them.One pre-existing race, fixed here
Review caught that the collector handed over the image list and only then created the endpoint it waits on:
If the peer reads the list and signals completion before that endpoint is published, the remover's completion write fails and it calls
os.Exit, while the collector blocks inAwaitforever — a hung pod with a failed container.Two things worth being precise about:
main, and it is not Windows-specific. A FIFO that has not been created yet reportsENOENTfromopen()exactly as a missing socket reports it fromstat(). The window is one syscall wide on either platform, which is why it has not been hit in practice.ReceiveImagespublishesEraseCompleteScanPathbefore reading. The collector was the odd one out, so the fix makes it consistent rather than introducing a new convention.I'd normally leave a pre-existing bug out of a port PR, but this one is four lines, sits in code this PR already rewrites, and deadlocks a pod when it fires.
Testing
Unit —
pkg/utils/handoff_test.godeliberately carries no build tag, so the same four tests run against whichever implementation the platform selects: images round-trip, completion round-trip, absent peer reportsos.IsNotExist, andCloseis safe to call twice.Cross-compile —
GOOS=windows go build ./pkg/...now succeeds forpkg/collectorandpkg/scanners/template, which it did not before.On a real node — see below.
Standalone E2E test results
Upstream has no Windows CI, so this was validated on a personal fork against a real AKS Windows Server 2022 node.
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. The collector, scanner and remover are separate containers sharing a volume, so
ipcspikeruns this PR's actualpkg/utilsAPI from two containers of one pod over anemptyDir:producer(collector-shaped) publishes its completion endpoint, hands over an image list, then waits to be told the erase finishedconsumer(remover-shaped) reads the list, checks that an endpoint nobody published is still reported asIsNotExist, then signals backEnvironment
mcr.microsoft.com/windows/nanoserver:ltsc2022runAsUserName: NT AUTHORITY\SYSTEMemptyDirmounted into both containersLive results — run against the code in
ed03cad6Cross-container handoff, two containers of one pod over an
emptyDir:The 18s on the consumer side is the deliberate 20s 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:Two observations from the run
A missing Unix socket reports connection-refused on Windows, not
ENOENT. This is worth writing down because it is not what the Unix intuition predicts, and it initially hung a test. Dialing a path that does not exist givesWSAECONNREFUSED(10061), and none of the obvious checks match it:Two consequences, both in the code:
WriteCompletionPipeusesos.Statfor the existence check rather than inferring it from the dial error, sopkg/remover'sos.IsNotExistbranch keeps working unchanged.dialForeverdoes not classify errors. There is no reliable "not yet" error to match on, so it retries unconditionally on a 1s tick — which is also what the Unix side effectively does by blocking inopen().Socket rendezvous is weaker than FIFO rendezvous.
Dialsucceeds as soon as the listener exists, without a matchingAccept, whereas a FIFOopendoes not return until the reader is actually there. In this flow it does not matter — the payload is small, it is written and the connection closed immediately, and the reader always callsAccept— but it is a real semantic difference rather than a drop-in equivalence, so I would rather state it than imply the two are identical.Follow-ups, not in this PR
pkg/utilsare still Linux-shaped (/run/eraser.sh/shared-data/...), and the manager still emits a Linux pod spec with a CRIhostPathmount. Windows needs a HostProcesssecurityContextand a Windows mount path. That is the next piece.pkg/remover's single 5-minute context coveringListImages+ListContainers+ everyDeleteImage. Measured on this node, one large Windows image takes 15–74s to delete, so the budget is exhausted by roughly four images. Flagged in feat: support Windows containerd named-pipe CRI endpoints #1229 as well.Finish()inpkg/scanners/templatereturnsnilwhen the completion payload is not the expected message. Silently succeeding on an unexpected payload looks wrong, but changing it is a behaviour change unrelated to this PR — happy to fix it separately if you agree it is a bug.One question for reviewers
ReadCollectScanPipeandWriteScanErasePipeare now one-line wrappers overReadImagesPipe/WriteImagesPipe. I kept them because they are exported and an out-of-tree scanner could be calling them. If you would rather they were deprecated or dropped, say so and I will.