Skip to content

fix(container): set Detach true in ExecStart to prevent blocking on command execution - #1683

Merged
nicholas-fedor merged 1 commit into
mainfrom
fix/1678-lifecycle-hook-exec-detach
May 27, 2026
Merged

fix(container): set Detach true in ExecStart to prevent blocking on command execution#1683
nicholas-fedor merged 1 commit into
mainfrom
fix/1678-lifecycle-hook-exec-detach

Conversation

@nicholas-fedor

@nicholas-fedor nicholas-fedor commented May 27, 2026

Copy link
Copy Markdown
Owner

This PR fixes ExecStart blocking lifecycle hooks by setting Detach: true and preventing false exit code 126 on Docker Engine v29+.

Problem

Under Docker Engine v29 (moby/moby v29+), ExecStart with Detach=false (the default) blocks synchronously until the command finishes. When watchtower subsequently calls ExecAttach to capture output, the exec instance is already complete, and the daemon responds with "exec command is already running". This error text gets captured as the command output, and ExecInspect reports exit code 126, causing the lifecycle hook to be marked as failed despite the command having actually succeeded. Every container with a pre-update lifecycle hook fails its update.

Solution

Set Detach: true in ExecStartOptions so that ExecStart returns immediately after launching the command in the background. The subsequent ExecAttach can then properly stream output while the command runs, and waitForExecOrTimeout correctly retrieves the exit code via ExecInspect. This matches the standard Docker API pattern for non-blocking exec with output capture.

Changes

  • pkg/container/client.go:1025 — Added Detach: true to ExecStartOptions so ExecStart does not block until command completion
  • pkg/container/client_test.go — Added test verifying ExecStart sends Detach: true and updated mock to expect it

Summary by CodeRabbit

Bug Fixes

  • Fixed command execution in containers to eliminate blocking behavior and prevent subsequent attachment failures.

Review Change Stack

…ommand execution

- Add `Detach: true` to `ExecStartOptions` so the daemon does not block until the command finishes
- Update test mock handler to verify `Detach: true` is sent in the `ExecStart` request
- Add explicit test case documenting the `Detach: true` requirement for `ExecStart`
@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c930cc2b-6377-44d1-8959-f44dfa98000d

📥 Commits

Reviewing files that changed from the base of the PR and between b22cf73 and de88b10.

📒 Files selected for processing (2)
  • pkg/container/client.go
  • pkg/container/client_test.go

Walkthrough

ExecuteCommand is updated to start Docker exec instances with Detach: true while keeping TTY: true. Tests now explicitly verify this behavior, and the shared mock handler is tightened to enforce the detached flag in request verification.

Changes

Detach Flag in Exec Start

Layer / File(s) Summary
ExecuteCommand Detach flag implementation
pkg/container/client.go
ExecuteCommand changes ExecStartOptions to include Detach: true alongside TTY: true when calling c.api.ExecStart.
Detach behavior test verification
pkg/container/client_test.go
A new test asserts ExecuteCommand uses ExecStart with Detach: true to avoid blocking and attach failures; the setupExecMockHandlers helper is tightened to require Detach: true in JSON verification alongside Tty: true.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related issues

  • #1678: Addresses the blocking and "already running" conflicts by setting Detach: true in ExecuteCommand as described in the issue.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: setting Detach to true in ExecStart to prevent blocking during command execution, which is the core fix in this PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 5 duplication

Metric Results
Complexity 0
Duplication 5

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@nicholas-fedor
nicholas-fedor merged commit c1dfc34 into main May 27, 2026
18 checks passed
@nicholas-fedor
nicholas-fedor deleted the fix/1678-lifecycle-hook-exec-detach branch May 27, 2026 18:13
@codecov

codecov Bot commented May 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1683      +/-   ##
==========================================
+ Coverage   74.60%   74.80%   +0.20%     
==========================================
  Files          59       59              
  Lines        9839     9948     +109     
==========================================
+ Hits         7340     7442     +102     
- Misses       2237     2241       +4     
- Partials      262      265       +3     
Files with missing lines Coverage Δ
pkg/container/client.go 63.55% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: pre-update lifecycle hook fails — ExecStart blocks then ExecAttach captures "is already running" (exit 126) on 1.17.x / Docker v29

1 participant