[hirehamir]

- What I did

Fixes #5802

- How I did it

I kept the write end of the pipe alive for the duration of the test with

defer w.Close()

- How to verify it

# without the changes from this branch: fails most runs
# with the changes from this branch: 0 failures
GOGC=1 GOMEMLIMIT=8MiB go test ./cli/command/container/ -run TestContainerPrunePromptTermination -count=40 -v

	GC forced (GOGC=1 GOMEMLIMIT=8MiB)	GC off (GOGC=off)
without this change	33 of 40 fail	0 of 40 fail
with this change	0 of 40 fail	0 of 40 fail

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/test/cmd.go	0.00%	5 Missing ⚠️

📢 Thoughts on this report? Let us know!

[GitNimay]

I can confirm this fixes the flake, which also hit CI on #7408 (TestSystemPrunePromptTermination). The discarded write end of the pipe is garbage-collected, its finalizer closes it, and the prompt reads EOF (treated as "no"), so the command returns "prune has been cancelled" instead of prompt.ErrTerminated.

Reproduce on master with:

GOGC=1 go test -race -count=200 -run PromptTermination ./cli/command/system ./cli/command/image

That gave 11 failures on master, and 0 with the write end kept open.

Two things you may want to add here:

• cmd/docker-trust/internal/test/cmd.go has an identical copy of TerminatePrompt with the same bug.
• Under -race, the cli.OutBuffer().Len() check in the helper races with the prompt that is still writing to the buffer. The writer hook already guarantees a write happened, so that check can be dropped.

This is the flake tracked in #5802.

[hirehamir]

That makes sense. I'll add those changes right now.

[hirehamir]

@Benehiko anything else you'd like to see here?

[Benehiko]

Thanks for tracking this down! The root cause checks out: the discarded write end gets finalized by the GC, and the prompt reads EOF and treats it as "no". Dropping the OutBuffer().Len() check is also safe, since the writer hook only fires after the write to the buffer has completed.

Before this can be merged:

• The commits need to be signed off for the DCO check; none of them currently has a Signed-off-by trailer.
• Please squash the three commits into one. They are all part of the same fix, and the last one ("fix identical issue in internal/test/cmd.go") is misleading since that file was already changed in the first commit. A single commit whose message explains the cause (the unreferenced write end is closed by its GC finalizer, so the prompt reads EOF) will be much more useful in git log.

You can do both at once with git rebase -i --signoff master (mark the later commits as squash), then force-push.

[Benehiko]

r is never closed, so every call to TerminatePrompt leaks a file descriptor. Could we register cleanup for both ends of the pipe with t.Cleanup instead of deferring only w.Close()?

Suggested change

	r, w, err := os.Pipe()
	assert.NilError(t, err)
	// Keep the write end alive for the duration of the test.
	defer w.Close()
	r, w, err := os.Pipe()
	assert.NilError(t, err)
	t.Cleanup(func() {
	_ = w.Close()
	_ = r.Close()
	})

[Benehiko]

Same as in internal/test/cmd.go: r is never closed here either. Could we clean up both ends with t.Cleanup?

Suggested change

	r, w, err := os.Pipe()
	assert.NilError(t, err)
	// Keep the write end alive for the duration of the test.
	defer w.Close()
	r, w, err := os.Pipe()
	assert.NilError(t, err)
	t.Cleanup(func() {
	_ = w.Close()
	_ = r.Close()
	})

[hirehamir]

You're welcome! I'll make those changes right now. Thanks for including the git rebase -i --signoff command.