refactor(attach): improve attach error handling and cleanup by htoyoda18 · Pull Request #13488 · docker/compose

htoyoda18 · 2025-12-31T16:32:22Z

What I did

Improved error handling around attach output and stream/tty operations while keeping existing attach behavior.
attach: stopped ignoring the result of fmt.Fprintf and log a debug message when writing the "Attaching to ..." line fails.
doAttachContainer: ensured the restore function returned by attachContainerStreams is always called (defer restore) to avoid leaving the terminal in a broken state.
attachContainerStreams: stopped ignoring errors from terminal restore and stream closes, and added debug/warn logs for unexpected failures.
Stream copy goroutines now capture and log non-EOF copy errors.
Simplified getContainerStreams by removing unused variables and returning explicit nils.

Related issue
N/A

(not mandatory) A picture of a cute animal, if possible in relation to what you did

ndeloof · 2026-01-05T08:13:49Z

pkg/compose/attach.go


-	_, _, err = s.attachContainerStreams(ctx, id, inspect.Config.Tty, nil, wOut, wErr)
-	return err
+	restore, _, err := s.attachContainerStreams(ctx, id, inspect.Config.Tty, nil, wOut, wErr)


as attachContainerStreams is only used by doAttachContainer it could be refactored to avoid a restore function being returned (this function signature is a legacy from d4a8e92).

Thanks for the review! You're absolutely right that the restore function is legacy and no longer needed.

aad4071

I've refactored attachContainerStreams to remove the unused signature elements:

Removed stdin parameter (always passed as nil by doAttachContainer)

Changed return type from (func(), chan bool, error) to error

Removed the restore function and detached channel logic

Removed unused imports (streams, term)

Since stdin was always nil, the restore function was always a no-op and the detached channel was never used, so this change doesn't affect behavior—it just simplifies the function to match its actual usage.

ndeloof · 2026-01-06T10:39:39Z

pkg/compose/attach.go

-			}
-		}()
+func (s *composeService) attachContainerStreams(ctx context.Context, container string, tty bool, stdout, stderr io.WriteCloser) error {
+	_, streamOut, err := s.getContainerStreams(ctx, container)


it seems to me getContainerStreams is only used from here, so there's another opportunity to simplify/cleanup code by removing useless attachment to stdin

Thanks! Simplified getContainerStreams to remove the unused stdin:

1036fbb

Changed signature to return only the output stream: (io.ReadCloser, error)

Set Stdin: false in ContainerAttach (was true but discarded by caller)

Removed DetachKeys (not needed without stdin)

The function now matches its actual read-only usage.

Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>

htoyoda18 · 2026-01-12T12:44:30Z

@ndeloof
Would you mind re-running or approving the CI workflow when you have a moment?
I found a small diff in go.mod and pushed the tidy result.

Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>

htoyoda18 · 2026-01-18T12:33:53Z

Hi @ndeloof, just following up on this.

I previously pushed a small go.mod tidy fix, so the CI should be clean now.
Whenever you have a moment, could you please re-run or approve the CI workflow?

CC @glours in case you have time to take a look as well.
Thanks!

thaJeztah · 2026-01-21T10:03:21Z

pkg/compose/attach.go

-		Stdout:     true,
-		Stderr:     true,
-		Logs:       false,
-		DetachKeys: s.configFile().DetachKeys,


Curious; wouldn't we need the detach-keys? i.e., does this mean it's no longer possible to detach from a container, or is this not used for interactive use?

only used to get output logs. For interactive mode, we rely on docker/cli RunExec and RunAttach

thaJeztah · 2026-01-21T10:06:32Z

pkg/compose/attach.go

-	_, _ = fmt.Fprintf(s.stdout(), "Attaching to %s\n", strings.Join(names, ", "))
+	_, err = fmt.Fprintf(s.stdout(), "Attaching to %s\n", strings.Join(names, ", "))
+	if err != nil {
+		logrus.Debugf("failed to write attach message: %v", err)


Slightly wondering how useful these are, other than "please the linter"; logging here would try to write to stderr, which may also fail

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [docker/compose](https://github.com/docker/compose) | patch | `v5.0.1` → `v5.0.2` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>docker/compose (docker/compose)</summary> ### [`v5.0.2`](https://github.com/docker/compose/releases/tag/v5.0.2) [Compare Source](docker/compose@v5.0.1...v5.0.2) #### What's Changed ##### 🐛 Fixes - Fixed progress UI to adapt to terminal width by [@ndeloof](https://github.com/ndeloof) in [#13519](docker/compose#13519) - Removed warning when no explicit build has been requested. by [@ndeloof](https://github.com/ndeloof) in [#13493](docker/compose#13493) - Restored `runtime_flags` support in `models` by [@ilopezluna](https://github.com/ilopezluna) in [#13460](docker/compose#13460) - Added service name completion to `down` command by [@bmo-at](https://github.com/bmo-at) in [#13470](docker/compose#13470) - Fixed tilde in `--env-file` paths expanded to user home directory by [@tensorworkerr](https://github.com/tensorworkerr) in [#13510](docker/compose#13510) - Handle `healthcheck.disable: true` by [@stavros-k](https://github.com/stavros-k) in [#13494](docker/compose#13494) - Fixed shutdown and error handling for large file change batches in `watch` by [@amyssnippet](https://github.com/amyssnippet) in [#13525](docker/compose#13525) ##### 🔧 Internal - Added unit test for upOptions.OnExit method by [@htoyoda18](https://github.com/htoyoda18) in [#13489](docker/compose#13489) - clean up temporary compose files after conversion by [@htoyoda18](https://github.com/htoyoda18) in [#13483](docker/compose#13483) - Fixed typo in isSwarmEnabled method name by [@htoyoda18](https://github.com/htoyoda18) in [#13481](docker/compose#13481) - Fixed incorrect usage of errgroup.WithContext by [@htoyoda18](https://github.com/htoyoda18) in [#13480](docker/compose#13480) - Fixed timeout initialization when waitTimeout is zero by [@htoyoda18](https://github.com/htoyoda18) in [#13471](docker/compose#13471) - Extracted API version constants to dedicated file by [@htoyoda18](https://github.com/htoyoda18) in [#13503](docker/compose#13503) - Replace tabbed indentation in sdk.md by [@pkqk](https://github.com/pkqk) in [#13505](docker/compose#13505) - Improved attach error handling and cleanup by [@htoyoda18](https://github.com/htoyoda18) in [#13488](docker/compose#13488) - Modernize tests by [@dgageot](https://github.com/dgageot) in [#13531](docker/compose#13531) - set fsnotify build tag when building for OSX by [@ndeloof](https://github.com/ndeloof) in [#13532](docker/compose#13532) ##### ⚙️ Dependencies - build(deps): bump github.com/klauspost/compress to v1.18.2 by [@thaJeztah](https://github.com/thaJeztah) in [#13499](docker/compose#13499) - go.mod: remove exclude rules by [@thaJeztah](https://github.com/thaJeztah) in [#13498](docker/compose#13498) - build(deps): bump github.com/containerd/containerd/v2 to v2.2.1 by [@thaJeztah](https://github.com/thaJeztah) in [#13497](docker/compose#13497) - build(deps): bump golang.org/x/sys from 0.39.0 to 0.40.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13502](docker/compose#13502) - build(deps): bump google.golang.org/grpc from 1.77.0 to 1.78.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13475](docker/compose#13475) - build(deps): bump github.com/go-viper/mapstructure/v2 from 2.4.0 to 2.5.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13506](docker/compose#13506) - build(deps): bump github.com/sirupsen/logrus v1.9.4 by [@thaJeztah](https://github.com/thaJeztah) in [#13518](docker/compose#13518) - Bump compose to v2.10.1 by [@ndeloof](https://github.com/ndeloof) in [#13528](docker/compose#13528) #### New Contributors - [@bmo-at](https://github.com/bmo-at) made their first contribution in [#13470](docker/compose#13470) - [@pkqk](https://github.com/pkqk) made their first contribution in [#13505](docker/compose#13505) - [@tensorworkerr](https://github.com/tensorworkerr) made their first contribution in [#13510](docker/compose#13510) - [@stavros-k](https://github.com/stavros-k) made their first contribution in [#13494](docker/compose#13494) - [@amyssnippet](https://github.com/amyssnippet) made their first contribution in [#13525](docker/compose#13525) - [@dgageot](https://github.com/dgageot) made their first contribution in [#13531](docker/compose#13531) **Full Changelog**: <docker/compose@v5.0.1...v5.0.2> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

htoyoda18 marked this pull request as ready for review December 31, 2025 16:36

htoyoda18 requested a review from a team as a code owner December 31, 2025 16:36

htoyoda18 requested review from glours and ndeloof December 31, 2025 16:36

ndeloof reviewed Jan 5, 2026

View reviewed changes

ndeloof enabled auto-merge (rebase) January 5, 2026 08:48

auto-merge was automatically disabled January 5, 2026 15:02
Head branch was pushed to by a user without write access

htoyoda18 force-pushed the fix/attach-error-handling branch from 35f9138 to 27089c6 Compare January 5, 2026 15:05

ndeloof reviewed Jan 6, 2026

View reviewed changes

htoyoda18 force-pushed the fix/attach-error-handling branch from 2d87249 to 50bafa0 Compare January 6, 2026 12:07

htoyoda18 added 4 commits January 6, 2026 21:10

Improve error handling in attach.go

8da3c30

Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>

refactor(attach): remove unused detach watcher and keep attach behavior

5825bcb

Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>

refactor(attach): simplify attachContainerStreams signature

aad4071

Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>

refactor(attach): remove unused stdin from getContainerStream

1036fbb

Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>

htoyoda18 force-pushed the fix/attach-error-handling branch from 50bafa0 to 1036fbb Compare January 6, 2026 12:10

Merge branch 'main' into fix/attach-error-handling

e83b48b

htoyoda18 requested a review from ndeloof January 7, 2026 11:54

ndeloof previously approved these changes Jan 8, 2026

View reviewed changes

ndeloof enabled auto-merge (rebase) January 8, 2026 06:08

htoyoda18 added 2 commits January 9, 2026 11:40

Merge branch 'main' into fix/attach-error-handling

317e170

Merge branch 'main' into fix/attach-error-handling

32cd647

fix: update github.com/moby/term to indirect dependency

8af7749

Signed-off-by: hiroto.toyoda <hiroto.toyoda@dena.com>

auto-merge was automatically disabled January 12, 2026 12:46
Head branch was pushed to by a user without write access

htoyoda18 dismissed ndeloof’s stale review via 8af7749 January 12, 2026 12:46

htoyoda18 requested a review from ndeloof January 13, 2026 11:58

Merge branch 'main' into fix/attach-error-handling

0d78f61

Merge branch 'main' into fix/attach-error-handling

2ddc252

ndeloof approved these changes Jan 19, 2026

View reviewed changes

ndeloof enabled auto-merge (rebase) January 19, 2026 16:37

ndeloof merged commit 06e1287 into docker:main Jan 19, 2026
24 checks passed

htoyoda18 deleted the fix/attach-error-handling branch January 20, 2026 03:04

BrewTestBot mentioned this pull request Jan 21, 2026

docker-compose 5.0.2 Homebrew/homebrew-core#263803

Merged

rancherbot mentioned this pull request Jan 21, 2026

rddepman: bump dockerCompose from 5.0.1 to 5.0.2 rancher-sandbox/rancher-desktop#9778

Merged

thaJeztah reviewed Jan 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(attach): improve attach error handling and cleanup#13488

refactor(attach): improve attach error handling and cleanup#13488
ndeloof merged 10 commits intodocker:mainfrom
htoyoda18:fix/attach-error-handling

htoyoda18 commented Dec 31, 2025 •

edited

Loading

Uh oh!

ndeloof Jan 5, 2026

Uh oh!

htoyoda18 Jan 5, 2026 •

edited

Loading

Uh oh!

ndeloof Jan 6, 2026

Uh oh!

htoyoda18 Jan 6, 2026

Uh oh!

htoyoda18 commented Jan 12, 2026 •

edited

Loading

Uh oh!

htoyoda18 commented Jan 18, 2026

Uh oh!

Uh oh!

thaJeztah Jan 21, 2026

Uh oh!

ndeloof Jan 21, 2026

Uh oh!

thaJeztah Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

htoyoda18 commented Dec 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndeloof Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

htoyoda18 Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndeloof Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

htoyoda18 Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

htoyoda18 commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htoyoda18 commented Jan 18, 2026

Uh oh!

Uh oh!

thaJeztah Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

ndeloof Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

htoyoda18 commented Dec 31, 2025 •

edited

Loading

htoyoda18 Jan 5, 2026 •

edited

Loading

htoyoda18 commented Jan 12, 2026 •

edited

Loading