test/testenv: fix leaking buildx dial-stdio processes on interrupt#984
Merged
cpuguy83 merged 1 commit intoproject-dalec:mainfrom Mar 11, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the integration test harness’ handling of docker buildx dial-stdio lifecycle so buildx/dial-stdio processes don’t leak when the test suite is interrupted or when the dial command fails early. It updates the Buildx dialer implementation and ensures testEnv.Close() runs on interrupt.
Changes:
- Reworks
dial-stdiowiring to avoidcmd.Wait()hangs when the command exits immediately (e.g., bad args), usingStdinPipe()+ an explicit copy loop. - Adds a connection wrapper that can surface underlying command failures instead of generic pipe errors.
- Updates
TestMainto close the test environment on interrupt using async.OnceFunc.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
test/testenv/buildx.go |
Refactors the buildx dial-stdio exec/stdio handling and introduces process-group configuration. |
test/main_test.go |
Ensures test environment cleanup is triggered on Ctrl+C (interrupt) exactly once. |
invidian
force-pushed
the
fix-leaking-dial-stdio
branch
from
41b1c39 to
7bc6a11
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
test/main_test.go:93
- Typo in comment:
uninteruptableshould beuninterruptible.
// This _should_ trigger builds to cancel naturally and exit the program,
// but in some cases it may not (due to timing, bugs in buildkit, uninteruptable operations, etc.).
// Cancel our signal handler so the normal handler takes over from here.
7bc6a11 to
970cc92
Compare
invidian
force-pushed
the
fix-leaking-dial-stdio
branch
from
970cc92 to
2206bd9
Compare
Existing implementation was functional only with modified buildx binary and missed few details. With this commit, we gracefully handle: - buildx binary not starting at all, e.g. caused by bad parameter. - automated connection issues without explicit confirmation. - interruption of test suite gracefully shuts down docker CLI and buildx processes. - Docker daemon restarting or crashing. Closes project-dalec#974 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
invidian
force-pushed
the
fix-leaking-dial-stdio
branch
from
2206bd9 to
efb4869
Compare
cpuguy83
approved these changes
Mar 11, 2026
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.
What this PR does / why we need it:
Existing implementation was functional only with modified buildx binary and missed few details.
With this commit, we gracefully handle:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Closes #974
Special notes for your reviewer: