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 2 commits into
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
If you can tell me how to collect the info you need I'll report here. |
Here's a compact set of things to ask the reporter to run. The goal is to (a) confirm what stdin actually is in their environment, and (b) capture a goroutine dump showing where compose publish --yes is parked.
Show what stdin is wired to in the same shell contextls -la /proc/self/fd/0 or, for the actual compose invocation, prefix it:ls -la /proc/self/fd/0 && docker compose publish --yes ... What to look for: a. /dev/null --> closed/EOF on read Also useful: what is the exact CI/runner (GitHub Actions step, GitLab, Jenkins, cron, ssh -T, nohup, systemd unit, docker exec without -i, etc.). The runner often dictates whether stdin is /dev/null (EOF) vs an idle pipe.
PID=$(pgrep -nf "compose publish") If strace shows it parked in read(0, ...) and /proc/$PID/fd/0 is a pipe, that's the smoking gun for "open idle pipe, not EOF."
Ask them to capture stderr (2> compose.stacks) and paste the dump. That will show the exact goroutine blocked on stdin and the call chain that led there — usually more useful than a gdb stack for Go. If they want a non-fatal dump (keep the process alive), they can attach delve:
docker compose publish --yes ... < /dev/null If < /dev/null does not hang but their real invocation does, then their stdin is an open-idle pipe and your --yes short-circuit is the correct fix; the contrast is good evidence to put in the PR. That's the package: fd inspection + strace + SIGQUIT goroutine dump + the /dev/null vs /dev/zero contrast. |
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