fix: return an error when docker host cannot be retrieved#3613
Merged
mdelapenya merged 1 commit intotestcontainers:mainfrom Apr 9, 2026
Merged
fix: return an error when docker host cannot be retrieved#3613mdelapenya merged 1 commit intotestcontainers:mainfrom
mdelapenya merged 1 commit intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughDocker host extraction was changed from panic-on-failure to explicit error-returning behavior: a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Core as core.ExtractDockerHost
participant Client as NewClient / NewDockerProvider / DockerClient
Caller->>Client: initialize (ctx)
Client->>Core: ExtractDockerHost(ctx)
Core-->>Client: (host, nil)
alt host present
Client->>Client: use host in options/logs
Client-->>Caller: return initialized client
else error
Core-->>Client: ("" , error)
Client-->>Caller: return nil, error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker_client.go`:
- Around line 80-83: The cache flag dockerInfoSet is being set before host
resolution, so if core.ExtractDockerHost(ctx) fails the function returns an
error while leaving dockerInfoSet true and causing future Info() calls to return
a cached success; update Info() so dockerInfoSet is only set after
ExtractDockerHost succeeds (and any subsequent logging/processing completes) —
e.g., move the assignment that flips dockerInfoSet (or the code that writes
dockerInfo) to after the successful call to core.ExtractDockerHost(ctx) and any
error checks, or explicitly clear/unset dockerInfoSet on error paths of
ExtractDockerHost to ensure the cache is only marked initialized on success.
In `@internal/core/docker_host.go`:
- Around line 96-101: The code uses dockerHostOnce to cache both success and
failure in ExtractDockerHost, causing dockerHostErrCache to be sticky; change
the logic so dockerHostOnce (and dockerHostCache) only records on successful
extraction: call extractDockerHost inside a retryable path and only assign to
dockerHostCache and mark initialization (or call dockerHostOnce.Do) when
extractDockerHost returns nil error; if extractDockerHost returns an error,
return it directly without setting dockerHostErrCache so future calls can retry.
Update the related variables dockerHostOnce, dockerHostCache, dockerHostErrCache
and the call site of extractDockerHost to follow this pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64d4242b-1b52-498c-9c55-6f8f05a93aa9
📒 Files selected for processing (4)
docker_client.gointernal/core/client.gointernal/core/docker_host.goprovider.go
Comment on lines
+96
to
101
| func ExtractDockerHost(ctx context.Context) (string, error) { | ||
| dockerHostOnce.Do(func() { | ||
| dockerHostCache, dockerHostErrCache = extractDockerHost(ctx) | ||
| }) | ||
|
|
||
| return dockerHostCache | ||
| return dockerHostCache, dockerHostErrCache | ||
| } |
There was a problem hiding this comment.
Do not cache Docker host extraction errors process-wide.
At Line 97, sync.Once stores the first failure forever (dockerHostErrCache). A transient startup failure becomes unrecoverable for the lifetime of the process, even if Docker/environment is fixed later.
Suggested fix (cache success only, allow retry on errors)
var (
- dockerHostCache string
- dockerHostErrCache error
- dockerHostOnce sync.Once
+ dockerHostCache string
+ dockerHostCached bool
+ dockerHostMu sync.Mutex
)
@@
func ExtractDockerHost(ctx context.Context) (string, error) {
- dockerHostOnce.Do(func() {
- dockerHostCache, dockerHostErrCache = extractDockerHost(ctx)
- })
- return dockerHostCache, dockerHostErrCache
+ dockerHostMu.Lock()
+ defer dockerHostMu.Unlock()
+
+ if dockerHostCached {
+ return dockerHostCache, nil
+ }
+
+ host, err := extractDockerHost(ctx)
+ if err != nil {
+ return "", err
+ }
+
+ dockerHostCache = host
+ dockerHostCached = true
+ return dockerHostCache, nil
}Also applies to: 34-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/docker_host.go` around lines 96 - 101, The code uses
dockerHostOnce to cache both success and failure in ExtractDockerHost, causing
dockerHostErrCache to be sticky; change the logic so dockerHostOnce (and
dockerHostCache) only records on successful extraction: call extractDockerHost
inside a retryable path and only assign to dockerHostCache and mark
initialization (or call dockerHostOnce.Do) when extractDockerHost returns nil
error; if extractDockerHost returns an error, return it directly without setting
dockerHostErrCache so future calls can retry. Update the related variables
dockerHostOnce, dockerHostCache, dockerHostErrCache and the call site of
extractDockerHost to follow this pattern.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docker_client.go (1)
76-86:⚠️ Potential issue | 🟠 MajorDefer cache finalization until host resolution succeeds.
dockerInfoSetis set at Line 52 before the new failure path at Lines 76-79. If host extraction fails once, laterInfo()calls can incorrectly return cached success from Line 44 and mask the error this PR is trying to propagate.Suggested fix
@@ - dockerInfo = res - dockerInfoSet = true + dockerInfo = res @@ host, err := core.ExtractDockerHost(ctx) if err != nil { - return dockerInfo, err + return res, err } @@ + dockerInfoSet = true return dockerInfo, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker_client.go` around lines 76 - 86, The code sets dockerInfoSet before resolving the host, which can cache a successful state even when core.ExtractDockerHost(ctx) fails; update the flow so that dockerInfoSet is only set after host resolution succeeds: move or defer the assignment/update of dockerInfoSet until after core.ExtractDockerHost(ctx) returns without error (the success path used before logging), and ensure the Info() path checks for the absence of dockerInfoSet when host extraction fails so it does not return a cached success; touch the dockerInfoSet assignment and any early-return in Info()/Docker client initialization and the call to core.ExtractDockerHost to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docker_client.go`:
- Around line 76-86: The code sets dockerInfoSet before resolving the host,
which can cache a successful state even when core.ExtractDockerHost(ctx) fails;
update the flow so that dockerInfoSet is only set after host resolution
succeeds: move or defer the assignment/update of dockerInfoSet until after
core.ExtractDockerHost(ctx) returns without error (the success path used before
logging), and ensure the Info() path checks for the absence of dockerInfoSet
when host extraction fails so it does not return a cached success; touch the
dockerInfoSet assignment and any early-return in Info()/Docker client
initialization and the call to core.ExtractDockerHost to implement this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdec2d4a-9aaf-443d-823c-0d951e5a18d8
📒 Files selected for processing (4)
docker_client.gointernal/core/client.gointernal/core/docker_host.goprovider.go
✅ Files skipped from review due to trivial changes (1)
- internal/core/docker_host.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/client.go
- provider.go
This was referenced Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
When I run some of my tests without Docker running, they panic. I'd like them to just return an error from
testcontainers.Run()(and everything that is built on top).Why is it important?
It's not useful to get a panic when just a normal error is enough. Panic should be reserved for programming errors/etc - "Docker is not running" is not that.
Related issues