fix: don't hang forever in Pipe.Confirm when stdin has no data#13746
fix: don't hang forever in Pipe.Confirm when stdin has no data#13746ssam18 wants to merge 1 commit intodocker:mainfrom
Conversation
d9ea627 to
f50f9ed
Compare
glours
left a comment
There was a problem hiding this comment.
Hi @ssam18, thanks for the contribution. I took a close look and I have some concerns before we can move forward.
1. The change doesn't match the reported bug
Issue #13722 is about -y not being respected. But in cmd/compose/publish.go:
if opts.assumeYes {
backendOptions.Options = append(backendOptions.Options, compose.WithPrompt(compose.AlwaysOkPrompt()))
}When -y is passed, the prompt is replaced with AlwaysOkPrompt, which returns (true, nil) without ever reading stdin. Pipe.Confirm is not reached. So the code you changed isn't on the -y path, and your PR description saying "causing fmt.Fscanln to block indefinitely even when -y is
passed" doesn't line up with what the code actually does.
Could you share a stack trace or reproducer showing Pipe.Confirm being hit with -y? That would help us confirm whether the real bug is somewhere else (for example, option ordering, a missed prompt path, or a different confirmation like confirmRemoteIncludes).
2. The fix may not actually prevent the hang
Your description says "stdin is often an open pipe that never sends data". In that case fmt.Fscanln blocks waiting for input and never returns an error, so the new if err != nil branch is never reached. The change only alters behavior when Fscanln returns an error (EOF or read error), and in
that case the previous code already returned (false, nil) via StringToBool(""). For the preChecks callers (which pass defaultValue=false), the runtime behavior is unchanged.
If Pipe.Confirm genuinely needs to not hang on an idle pipe, the fix likely needs to be non-blocking — e.g., a deadlined read on the underlying fd, or detecting upfront that stdin is /dev/null/closed, not an error check after Fscanln returns.
3. The test doesn't exercise the changed code
Test_preChecks_bind_mount_skipped_with_assume_yes uses AlwaysOkPrompt(), which doesn't touch Pipe.Confirm. It lives in a different package than your fix and would pass identically with or without the change to prompt.go. It's not a regression test for the behavior you're claiming to fix.
Suggested path forward
Before we change Pipe.Confirm, could you:
- Confirm with the issue reporter whether Azure DevOps closes stdin (EOF) or leaves it open — that determines where the hang actually is.
- Add logs/trace showing which code path is reached when
-yis passed. - If the root cause really is in
Pipe.Confirm, provide a test that drivesPipe.Confirmdirectly (with a blocking/EOF reader) and demonstrates the hang and its fix.
|
@glours You are right. I was wrong to change Pipe.Confirm. With -y set, AlwaysOkPrompt replaces s.prompt entirely so Pipe.Confirm is never in the picture, and even if it were, an open idle pipe never returns an error so the if err != nil branch wouldn't trigger. I will revert that change. The only thing left in the PR is the test, which verifies that AlwaysOkPrompt (the prompt wired up by --yes) lets preChecks pass through the bind-mount check without blocking, a regression guard rather than a fix. I don't have access to reproduce the issue on an Azure DevOps agent, so I cannot confirm the real hang path. Based on your hints, the next step is probably to ask the reporter whether stdin is actually closed (EOF) or just an open idle pipe, and whether there's any stack trace from a debug build. Happy to dig further once there's more info from the reporter. |
Signed-off-by: Samaresh Kumar Singh <ssam3003@gmail.com>
f50f9ed to
5481ea6
Compare
When running
docker compose publishin a CI environment like Azure DevOps, stdin is often an open pipe that never sends data, causingfmt.Fscanlnto block indefinitely even when-yis passed. This fix makesPipe.Confirmreturn the default value immediately when stdin returns an error (e.g. EOF or closed pipe) instead of hanging. Added a test to cover the--yespath so a prompt with bind mounts doesn't block. Fixes #13722