fix(server): give docker wait a timeout the container can outlive (#1482)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: FelixTJDietrich

.changeset/sandbox-streaming-deadline.md

@@ -0,0 +1,10 @@
+---
+---
+
+No user-facing or operator-facing effect. Waiting for an agent container to exit shared a Docker
+connection with ordinary requests, and that connection carried a socket timeout — so a review still
+working after thirty minutes had its wait torn down and was reported as failed. The wait now runs on
+its own connection whose timeout sits above the longest run a sandbox is allowed, which cannot cut a
+legitimate wait short but still reclaims the connection if the daemon stops answering. None of this
+reached a release: agent sandboxes have never shipped, so there is no version an operator can be
+upgrading from where a long review failed this way.

server/src/main/java/de/tum/cit/aet/hephaestus/agent/sandbox/docker/DockerClientOperations.java

@@ -17,6 +17,7 @@
 import de.tum.cit.aet.hephaestus.agent.sandbox.spi.SandboxInfrastructureException;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
+import java.time.Duration;
 import java.time.Instant;
 import java.util.List;
 import java.util.Map;
@@ -37,6 +38,9 @@ public class DockerClientOperations
     private static final Logger log = LoggerFactory.getLogger(DockerClientOperations.class);
     private static final int LOG_COLLECTION_TIMEOUT_SECONDS = 30;
 
+    /** A pull that has not finished by here is treated as failed; the RPC client's idle timeout sits above it. */
+    static final Duration IMAGE_PULL_TIMEOUT = Duration.ofMinutes(5);
+
     /**
      * Maximum log output to collect from a container (prevents OOM from runaway output). This is
      * the <em>collection</em> limit; see {@link DockerSandboxAdapter#MAX_LOG_EVENT_BYTES} for the
@@ -46,8 +50,12 @@ public class DockerClientOperations
 
     private final DockerClient dockerClient;
 
-    public DockerClientOperations(DockerClient dockerClient) {
+    /** `docker wait` is silent until the container exits, so it needs a socket timeout no RPC call wants. */
+    private final DockerClient streamingClient;
+
+    public DockerClientOperations(DockerClient dockerClient, DockerClient streamingClient) {
         this.dockerClient = dockerClient;
+        this.streamingClient = streamingClient;
     }
 
     @Override
@@ -83,10 +91,13 @@ public boolean pullImage(String image) {
         try {
             log.info("Pulling Docker image: {}", image);
             long startMs = System.currentTimeMillis();
-            boolean completed = dockerClient.pullImageCmd(image).start().awaitCompletion(5, TimeUnit.MINUTES);
+            boolean completed = dockerClient
+                .pullImageCmd(image)
+                .start()
+                .awaitCompletion(IMAGE_PULL_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS);
             long durationMs = System.currentTimeMillis() - startMs;
             if (!completed) {
-                log.warn("Docker pull timed out after 5 min: image={}", image);
+                log.warn("Docker pull timed out after {}: image={}", IMAGE_PULL_TIMEOUT, image);
                 return false;
             }
             log.info("Pulled Docker image in {} ms: {}", durationMs, image);
@@ -249,7 +260,7 @@ public void startContainer(String containerId) {
     @Override
     public DockerOperations.WaitResult waitContainer(String containerId) {
         try {
-            var callback = dockerClient.waitContainerCmd(containerId).start();
+            var callback = streamingClient.waitContainerCmd(containerId).start();
             try {
                 int exitCode = callback.awaitStatusCode();
                 log.debug("Container {} exited with code {}", containerId, exitCode);

server/src/main/java/de/tum/cit/aet/hephaestus/agent/sandbox/docker/DockerSandboxConfiguration.java

@@ -14,6 +14,7 @@
 import de.tum.cit.aet.hephaestus.agent.sandbox.docker.interactive.InteractiveSandboxRegistry;
 import de.tum.cit.aet.hephaestus.agent.sandbox.docker.interactive.StdinWriteWatchdog;
 import de.tum.cit.aet.hephaestus.agent.sandbox.spi.InteractiveSandboxService;
+import de.tum.cit.aet.hephaestus.agent.sandbox.spi.ResourceLimits;
 import de.tum.cit.aet.hephaestus.agent.sandbox.spi.SandboxException;
 import de.tum.cit.aet.hephaestus.agent.sandbox.spi.SandboxManager;
 import de.tum.cit.aet.hephaestus.core.runtime.RuntimeRole;
@@ -27,6 +28,7 @@
 import java.util.concurrent.ThreadPoolExecutor;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
@@ -55,16 +57,55 @@ public class DockerSandboxConfiguration {
 
     private static final Logger log = LoggerFactory.getLogger(DockerSandboxConfiguration.class);
 
-    /** Connections per container: create/start, wait, logs/copy. */
-    private static final int CONNECTIONS_PER_CONTAINER = 3;
+    /** RPC connections per container: create/start, logs, and a copy-out lease held while it is read. */
+    private static final int RPC_CONNECTIONS_PER_CONTAINER = 3;
 
     private static final Duration HTTP_CONNECTION_TIMEOUT = Duration.ofSeconds(5);
 
-    /** docker wait/logs can block for the full container lifetime. */
-    private static final Duration HTTP_RESPONSE_TIMEOUT = Duration.ofMinutes(30);
+    /**
+     * Idle timeout, not a deadline: Apache installs responseTimeout as the socket timeout for the
+     * whole exchange, so it bounds the gap between reads and a call that keeps producing bytes runs
+     * as long as it likes. Every RPC call carries its own budget, so this only has to reclaim the
+     * connection when the daemon goes silent — generous enough that a slow image layer or a large
+     * archive upload never trips it.
+     */
+    static final Duration HTTP_RESPONSE_TIMEOUT = Duration.ofMinutes(30);
+
+    /**
+     * `docker wait` sends nothing until the container exits, so for it the idle timeout is a ceiling
+     * on the container's life. Sitting above {@link ResourceLimits#MAX_RUNTIME} it can never cut a
+     * legitimate wait short, while still reclaiming a connection the daemon has abandoned —
+     * docker-java's reader thread only ever gets an interrupt, which a blocking read ignores.
+     */
+    static final Duration HTTP_STREAMING_RESPONSE_TIMEOUT = ResourceLimits.MAX_RUNTIME.plusMinutes(10);
+
+    /** Calls whose response body is the stream. One wait per container, and nothing else. */
+    @Bean(name = "dockerStreamingClient", destroyMethod = "close")
+    public DockerClient dockerStreamingClient(SandboxProperties properties) {
+        return buildClient(
+            properties,
+            HTTP_STREAMING_RESPONSE_TIMEOUT,
+            properties.maxConcurrentContainers(),
+            "streaming"
+        );
+    }
 
     @Bean(destroyMethod = "close")
     public DockerClient dockerClient(SandboxProperties properties) {
+        return buildClient(
+            properties,
+            HTTP_RESPONSE_TIMEOUT,
+            properties.maxConcurrentContainers() * RPC_CONNECTIONS_PER_CONTAINER,
+            "rpc"
+        );
+    }
+
+    private DockerClient buildClient(
+        SandboxProperties properties,
+        Duration responseTimeout,
+        int maxConnections,
+        String kind
+    ) {
         var configBuilder = DefaultDockerClientConfig.createDefaultConfigBuilder()
             .withDockerHost(properties.dockerHost())
             .withDockerTlsVerify(properties.tlsVerify());
@@ -78,24 +119,30 @@ public DockerClient dockerClient(SandboxProperties properties) {
         var httpClient = new ApacheDockerHttpClient.Builder()
             .dockerHost(config.getDockerHost())
             .sslConfig(config.getSSLConfig())
-            .maxConnections(properties.maxConcurrentContainers() * CONNECTIONS_PER_CONTAINER)
+            .maxConnections(maxConnections)
             .connectionTimeout(HTTP_CONNECTION_TIMEOUT)
-            .responseTimeout(HTTP_RESPONSE_TIMEOUT)
+            .responseTimeout(responseTimeout)
             .build();
 
         DockerClient client = DockerClientImpl.getInstance(config, httpClient);
         log.info(
-            "Docker sandbox client configured: host={}, tlsVerify={}",
+            "Docker sandbox client configured: kind={}, host={}, tlsVerify={}, responseTimeout={}, maxConnections={}",
+            kind,
             properties.dockerHost(),
-            properties.tlsVerify()
+            properties.tlsVerify(),
+            responseTimeout,
+            maxConnections
         );
 
         return client;
     }
 
     @Bean
-    public DockerClientOperations dockerClientOperations(DockerClient dockerClient) {
-        return new DockerClientOperations(dockerClient);
+    public DockerClientOperations dockerClientOperations(
+        DockerClient dockerClient,
+        @Qualifier("dockerStreamingClient") DockerClient dockerStreamingClient
+    ) {
+        return new DockerClientOperations(dockerClient, dockerStreamingClient);
     }
 
     @Bean

server/src/test/java/de/tum/cit/aet/hephaestus/agent/sandbox/docker/DockerClientOperationsTest.java

@@ -9,6 +9,7 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
 
 import com.github.dockerjava.api.DockerClient;
@@ -52,11 +53,14 @@ class DockerClientOperationsTest extends BaseUnitTest {
     @Mock
     private DockerClient dockerClient;
 
+    @Mock
+    private DockerClient streamingClient;
+
     private DockerClientOperations ops;
 
     @BeforeEach
     void setUp() {
-        ops = new DockerClientOperations(dockerClient);
+        ops = new DockerClientOperations(dockerClient, streamingClient);
     }
 
     @Nested
@@ -498,14 +502,27 @@ class WaitContainer {
         void shouldReturnExitCode() {
             WaitContainerCmd cmd = mock(WaitContainerCmd.class);
             WaitContainerResultCallback callback = mock(WaitContainerResultCallback.class);
-            when(dockerClient.waitContainerCmd("ctr-1")).thenReturn(cmd);
+            when(streamingClient.waitContainerCmd("ctr-1")).thenReturn(cmd);
             when(cmd.start()).thenReturn(callback);
             when(callback.awaitStatusCode()).thenReturn(42);
 
             DockerOperations.WaitResult result = ops.waitContainer("ctr-1");
 
             assertThat(result.exitCode()).isEqualTo(42);
         }
+
+        @Test
+        void shouldNotUseTheRpcClientWhenWaitingForAContainer() {
+            WaitContainerCmd cmd = mock(WaitContainerCmd.class);
+            WaitContainerResultCallback callback = mock(WaitContainerResultCallback.class);
+            when(streamingClient.waitContainerCmd("ctr-1")).thenReturn(cmd);
+            when(cmd.start()).thenReturn(callback);
+            when(callback.awaitStatusCode()).thenReturn(0);
+
+            ops.waitContainer("ctr-1");
+
+            verifyNoInteractions(dockerClient);
+        }
     }
 
     @Nested

server/src/test/java/de/tum/cit/aet/hephaestus/agent/sandbox/docker/DockerSandboxConfigurationTest.java

@@ -0,0 +1,27 @@
+package de.tum.cit.aet.hephaestus.agent.sandbox.docker;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import de.tum.cit.aet.hephaestus.agent.sandbox.spi.ResourceLimits;
+import de.tum.cit.aet.hephaestus.testconfig.BaseUnitTest;
+import org.junit.jupiter.api.Test;
+
+class DockerSandboxConfigurationTest extends BaseUnitTest {
+
+    @Test
+    void shouldOutliveTheLongestSandboxWhenWaitingForAContainerToExit() {
+        // `docker wait` is silent until the container exits, so this timeout is a ceiling on the
+        // container's life. At or below MAX_RUNTIME it would cut a legitimate wait short.
+        assertThat(DockerSandboxConfiguration.HTTP_STREAMING_RESPONSE_TIMEOUT).isGreaterThan(
+            ResourceLimits.MAX_RUNTIME
+        );
+    }
+
+    @Test
+    void shouldGiveOrdinaryRequestsMoreThanTheLongestBudgetedCallCanTake() {
+        // pullImage budgets itself 5 minutes; a shorter idle timeout would pre-empt that budget.
+        assertThat(DockerSandboxConfiguration.HTTP_RESPONSE_TIMEOUT).isGreaterThan(
+            DockerClientOperations.IMAGE_PULL_TIMEOUT
+        );
+    }
+}

server/src/test/java/de/tum/cit/aet/hephaestus/agent/sandbox/docker/DockerSandboxLiveTest.java

@@ -78,7 +78,7 @@ void setUp() {
             new ApacheDockerHttpClient.Builder().dockerHost(URI.create("unix:///var/run/docker.sock")).build()
         );
 
-        dockerOps = new DockerClientOperations(dockerClient);
+        dockerOps = new DockerClientOperations(dockerClient, dockerClient);
         dockerWaitExecutor = Executors.newCachedThreadPool();
         containerManager = new SandboxContainerManager(dockerOps, image -> {}, properties, dockerWaitExecutor);
         networkManager = new SandboxNetworkManager(dockerOps, properties);

server/src/test/java/de/tum/cit/aet/hephaestus/agent/sandbox/docker/RepositoryTreeStagingLiveTest.java

@@ -98,7 +98,7 @@ void setUp() {
             DefaultDockerClientConfig.createDefaultConfigBuilder().build(),
             new ApacheDockerHttpClient.Builder().dockerHost(URI.create("unix:///var/run/docker.sock")).build()
         );
-        DockerClientOperations dockerOps = new DockerClientOperations(dockerClient);
+        DockerClientOperations dockerOps = new DockerClientOperations(dockerClient, dockerClient);
         dockerWaitExecutor = Executors.newCachedThreadPool();
         containerManager = new SandboxContainerManager(dockerOps, image -> {}, properties, dockerWaitExecutor);
         networkManager = new SandboxNetworkManager(dockerOps, properties);

server/src/test/java/de/tum/cit/aet/hephaestus/agent/sandbox/docker/interactive/DockerInteractiveSandboxLiveTest.java

@@ -124,7 +124,7 @@ void setUp() throws Exception {
             new ApacheDockerHttpClient.Builder().dockerHost(URI.create("unix:///var/run/docker.sock")).build()
         );
 
-        dockerOps = new DockerClientOperations(dockerClient);
+        dockerOps = new DockerClientOperations(dockerClient, dockerClient);
         dockerWaitExecutor = Executors.newCachedThreadPool();
         containerManager = new SandboxContainerManager(dockerOps, image -> {}, sandboxProperties, dockerWaitExecutor);
         networkManager = new SandboxNetworkManager(dockerOps, sandboxProperties);