Conversation
ContainerNodeNameMap.merge() dropped containerListIncomplete. When a cloud failing with ContainersRetrievalException was followed by a healthy cloud, the merged map looked complete and cleanUpSuperfluousComputer() could remove agents of the unreachable cloud. Refs jenkinsci#1365 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…issing When a cloud's credentialsId could not be resolved, toSSlConfig() returned null and a client without TLS was built and cached. The cache key ignores the credential's content and the watchdog and provisioner keep the entry alive, so the client kept failing with "400 Client sent an HTTP request to an HTTPS server" long after the credentials were created. toSSlConfig() now throws, so nothing is cached and the first call made once the credentials exist builds a TLS client. The watchdog skips the affected cloud and marks the container list as incomplete instead of aborting the run. Fixes jenkinsci#1365 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Narrow the IllegalStateException handling so unrelated failures are not suppressed.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes Docker client caching when Docker server credentials are temporarily unavailable.
Changes:
- Rejects unresolved credentials before client caching.
- Preserves incomplete watchdog state across cloud merges.
- Adds regression tests for credentials, watchdog handling, and map merging.
| File | Description |
|---|---|
src/test/java/io/jenkins/docker/client/DockerAPITest.java |
Tests credential resolution and client caching. |
src/test/java/com/nirima/jenkins/plugins/docker/DockerContainerWatchdogTest.java |
Tests watchdog failure handling and recovery. |
src/test/java/com/nirima/jenkins/plugins/docker/ContainerNodeNameMapTest.java |
Tests merge behavior and completeness state. |
src/main/java/io/jenkins/docker/client/DockerAPI.java |
Rejects missing credentials before caching. |
src/main/java/com/nirima/jenkins/plugins/docker/DockerContainerWatchdog.java |
Handles affected clouds while continuing processing. |
src/main/java/com/nirima/jenkins/plugins/docker/ContainerNodeNameMap.java |
Preserves incomplete-list state during merges. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The IllegalStateException catch in the watchdog covered the whole try-with-resources block, so an unrelated IllegalStateException raised after the client was obtained (e.g. closing the client) was swallowed, logged as a connection failure and marked the container list incomplete. Catch it around getClient() only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getClient() can also throw IllegalStateException from the client cache bookkeeping (UsageTrackingCache.cacheAndIncrementUsage), which the watchdog would have logged as an unreachable cloud and skipped. Throw a MissingDockerServerCredentialsException instead, a subclass of IllegalStateException for compatibility, and catch only that. Only resolve the credentials for tcp endpoints: docker-java ignores the SSL configuration for unix and npipe sockets, so a leftover credentials id there must not turn a working cloud into a failing one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
krisstern
approved these changes
Sep 30, 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
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.
Fixes #1365
When a cloud's
credentialsIdcannot be resolved (typically for a few seconds at startup, while JCasC replaces the credentials store and a startup job recreates them),DockerAPI.toSSlConfig()returnednulland a client without TLS was built and cached. The cache key ignores the credential's content, and the watchdog and the provisioner keep the entry alive, so every call kept failing with400 Client sent an HTTP request to an HTTPS serverlong after the credentials existed.Four commits:
ContainerNodeNameMap.merge()dropped thecontainerListIncompleteflag. When a failing cloud was followed by a healthy one, the merged map looked complete andcleanUpSuperfluousComputer()could remove agents of the unreachable cloud. This is a prerequisite for the second commit.toSSlConfig()now throws when the credentials are missing, so nothing is cached and the first call made once they exist builds a TLS client. The watchdog skips the affected cloud and marks the container list as incomplete instead of aborting its run.getClient()only, so an unrelatedIllegalStateExceptionraised once the client is obtained still propagates.MissingDockerServerCredentialsException(a subclass ofIllegalStateException), and the watchdog catches only that type, so otherIllegalStateExceptions thrown bygetClient(), such as aUsageTrackingCacheinvariant failure, propagate. Credentials are only resolved fortcpendpoints: docker-java ignores the SSL config forunixandnpipesockets, so a leftover credentials id there keeps working as before.Behaviour change
This is intended: for a
tcpendpoint whosecredentialsIddoes not resolve to global Docker Server Credentials,getClient()now fails loudly instead of silently dropping TLS. This also applies to a stale credentials id on a plain-HTTP daemon (:2375), and to credentials stored in a folder, whichtoSSlConfig()never looked up. Endpoints without a credentials id, andunix/npipeendpoints, are unaffected.Testing done
DockerAPITest,DockerContainerWatchdogTestandContainerNodeNameMapTest. They fail onmasterand pass with the fix, except three that guard against regressions rather than reproduce the bug:testOtherIllegalStateExceptionFromGetClientIsNotSwallowed,testMissingCredentialsExceptionAfterObtainingClientIsNotSwallowed(both fail if the catch is widened again) andgetClientIgnoresAnUnresolvableCredentialsIdOnAUnixSocket(fails without thetcpguard).mvn verifypasses (201 tests, SpotBugs clean).AI disclosure: the investigation, the patch and its tests were produced with the help of an AI assistant, Claude Opus 5.5 (
claude-opus-5-5) by Anthropic, used through Claude Code. I reviewed the changes and take responsibility for them.Submitter checklist
🤖 Generated with Claude Code