Programming exercises: Stop cancelling build jobs during docker image pulls - #13311
Conversation
… pulls The stale build job watchdog cancels a job when it has no Docker container. A job that is pulling its image has no container yet, so a pull that takes longer than the grace period gets the job force-cancelled and requeued, which throws away the partial pull and repeats the same failure on the next agent. The grace period was also skipped whenever the job could not be found in the distributed processing map, dropping the budget from ~150 seconds to ~30 seconds. This happens exactly when another agent force-cancelled and requeued the job while this agent was picking it up, so the retry is more likely to be killed than the original attempt was. Track which jobs are pulling an image and skip stale detection for them, and skip it as well when the build start date is unknown rather than treating the job as stale. Jobs that are genuinely stuck without a container are still caught by the orphan cross-check. Since the watchdog was the only thing bounding a pull, bound it explicitly instead: awaitCompletion() was called without a timeout and would wait forever if the pull stopped making progress. Image pulls now have their own timeout, separate from the build timeout, because how long a pull takes depends on the image size and on the registry and network rather than on the exercise. Users saw this as "Could not pull Docker image" even though the image existed and the pull was simply slow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Programming exercises: Stop cancelling build jobs during Docker image pulls
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDocker image pulls now track per-job progress and enforce configurable overall and no-progress timeouts. Active pulls are excluded from build and stale-job timeout accounting. Missing build start times are handled without stale detection. ChangesDocker pull lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BuildJobManagementService
participant BuildAgentDockerService
participant MyPullImageResultCallback
participant BuildResultFuture
BuildJobManagementService->>BuildResultFuture: poll for build result
BuildJobManagementService->>BuildAgentDockerService: query image pull status
BuildAgentDockerService->>MyPullImageResultCallback: await pull with timeout
MyPullImageResultCallback-->>BuildAgentDockerService: progress or completion
BuildAgentDockerService-->>BuildJobManagementService: pull active or inactive
BuildJobManagementService-->>BuildResultFuture: continue waiting or enforce build timeout
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.java (1)
137-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that a timed-out pull closes its callback.
The test currently proves logging and failure, but not the abort operation. A regression removing
callback.close()would leave Docker pulling in the background while this test still passes.Proposed fix
+import java.io.IOException; - void testPullDockerImageFailsWhenPullExceedsTimeout() throws InterruptedException { + void testPullDockerImageFailsWhenPullExceedsTimeout() throws InterruptedException, IOException { var build = mockPendingImagePull(); // Simulate a pull that never finishes: the timed await reports that the image did not arrive within the timeout. when(pullImageCallback.awaitCompletion(anyLong(), any(TimeUnit.class))).thenReturn(false); try { assertThatThrownBy(() -> buildAgentDockerService.pullDockerImage(build, buildLogsMap)).isInstanceOf(LocalCIException.class); + verify(pullImageCallback).close(); assertThat(buildLogsMap.getAndTruncateBuildLogs(build.id())).anyMatch(logEntry -> logEntry.log().contains("timed out after"));As per PR objectives, expired pull callbacks must be aborted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.java` around lines 137 - 150, Extend testPullDockerImageFailsWhenPullExceedsTimeout to verify that pullImageCallback.close() is invoked when pullDockerImage times out. Keep the existing exception, log, and in-progress assertions, and place the close verification within the existing cleanup-safe test flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java`:
- Around line 137-138: Update BuildAgentDockerService to constructor-inject
imagePullTimeoutSeconds instead of field injection, and validate the value
during bean construction so zero or negative values are rejected immediately.
Preserve the existing default while ensuring the constructor stores only a
strictly positive timeout for pull operations.
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java`:
- Around line 498-503: Update the unknown-buildStartDate branch in
SharedQueueProcessingService to clear the job’s existing consecutive
stale-detection counter before logging and continuing. Preserve the current
behavior of skipping stale detection for jobs without a known start date, while
ensuring prior detections cannot trigger cancellation when the distributed entry
later reappears.
---
Nitpick comments:
In
`@src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.java`:
- Around line 137-150: Extend testPullDockerImageFailsWhenPullExceedsTimeout to
verify that pullImageCallback.close() is invoked when pullDockerImage times out.
Keep the existing exception, log, and in-progress assertions, and place the
close verification within the existing cleanup-safe test flow.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 920edfb9-0ebf-419c-b626-1d69f916915c
⛔ Files ignored due to path filters (1)
src/main/resources/config/application-buildagent.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.javasrc/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/localci/service/DockerClientTestService.java
End-to-End Test Results
❌ Failed Tests
Test Strategy: Running all tests (configuration or infrastructure changes detected) Overall: ❌ E2E: real (non-flaky) test failure 🔗 Workflow Run · 📊 Test Report |
Reject a non-positive image pull timeout at startup. awaitCompletion returns immediately for such a value, so every pull would be reported as timed out and no build could run. Reset the consecutive stale detection counter when the build start date is unknown. A job could otherwise carry earlier detections into that skip branch and be force-cancelled on the first check after its distributed entry reappears, which is the opposite of what skipping is for. The image pull branch already resets the counter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docker-java's awaitCompletion(timeout, unit) already closes the callback in a finally block, and close() is guarded against running twice, so the explicit close was dead code and the comment justifying it was wrong. Document instead what the timed variant guarantees: it still calls throwFirstError(), so a pull that fails rather than stalls propagates its exception exactly as the untimed call did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-image-pull-watchdog
…-image-pull-watchdog
The pull had a single wall-clock budget of 900 seconds, which is far longer than a whole build used to take and holds a build thread and an agent slot for the entire time. Lower the default to 300 seconds. A single budget also cannot tell the two failures apart. Track how many updates the daemon reports for a pull and judge it on movement as well as on total time: a large image over a slow link keeps reporting progress and may use the full budget, while a pull that reports nothing at all is abandoned after image-pull-stall-timeout-seconds, 60 by default. That is what an unreachable registry looks like from the agent when a firewall drops the packets rather than refusing the connection, and there is no reason to wait out the full budget for it. The two cases now fail with different messages, so the build log says whether the registry was slow or silent. Both close the callback, so the pull is really abandoned rather than left running in the background.
70fced8
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java`:
- Around line 141-142: Update the imagePullTimeoutSeconds configuration default
from 300 to the specified 900 seconds, preserving the existing property key and
explicit-configuration behavior.
- Around line 465-486: Set image-pull-timeout-seconds to 900 in both
configuration locations currently defining it as 300. In awaitPullCompletion,
calculate the remaining stall and overall deadline durations before each
callback wait, and pass the shorter positive duration to
callback.awaitCompletion instead of the fixed
PULL_PROGRESS_POLL_INTERVAL_SECONDS timeout, while preserving progress tracking
and abort behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 34714e31-86c6-4445-a7c8-59d3c26e88dc
⛔ Files ignored due to path filters (1)
src/main/resources/config/application-buildagent.ymlis excluded by!**/*.yml
📒 Files selected for processing (6)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.javasrc/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.javasrc/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/localci/service/DockerClientTestService.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementServiceTest.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java
- src/test/java/de/tum/cit/aet/artemis/localci/service/DockerClientTestService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java
- src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.java
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@krusche The exact head correctly separates image-pull and build-timeout budgets, resets stale counters on both skip paths, and bounds silent pulls while cleaning up callbacks and tracking state. The snapshot contains no completed relevant CI failure and declares no required checks.
The docker client mock is built once per class in the base class and is never reset, so whichever test stubbed the pull first kept deciding how the pull behaved for every test after it. testPullDockerImage does not stub the pull at all and only passed because another test had already left a pull stub behind, which made it depend on the execution order. Re-establish a pull that simply succeeds before each test, so every test starts from the same state whatever ran before it, and let the tests that care about a failing pull override it as they already do. The same test also verified that the pull command was issued exactly once, although the service retries a failed pull. Assert that a pull was attempted instead, which is what the test is actually about.
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@krusche The exact head keeps the pull and build timeout budgets separated, applies independent progress and overall pull bounds, and resets stale counters on both new skip paths. The latest test setup restores a successful callback per method, but one verification can still false-pass because the shared Docker mock retains invocation history; I left that low-severity issue inline. The snapshot declares no required checks and contains no completed relevant CI failure.
The waiter slept a fixed five seconds per slice, so a stall or overall timeout configured below that was only noticed once the slice ended, up to four seconds late. Shorten the slice to whatever is left until the nearest of the two deadlines. The defaults are far above the poll interval and therefore behave as before; short timeouts, which is what the tests and a tightly configured agent use, now fire on time. Also scope the pull verification in the Docker service test to the test itself. The Docker client mock lives for the whole class, so an earlier pull could satisfy the assertion even if this test stopped pulling.
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@krusche The exact head correctly separates image-pull and build-timeout budgets, clamps each pull wait to the nearest stall or overall deadline, and reliably clears pull and stale-detection state. The triggered test-isolation fix is present, and the snapshot contains no completed relevant CI failure or required pending check.
Programming exercises: Stop cancelling build jobs during Docker image pullsProgramming exercises: Stop cancelling build jobs during docker image pulls
Summary
The stale build job watchdog cancels build jobs while they are still pulling their Docker image. Users see
Could not pull Docker image <image>even though the image exists and the pull is simply slow. This PR makes the watchdog aware of in-flight image pulls, fixes a case where it silently skipped its own grace period, and bounds image pulls with an explicit timeout of their own.Checklist
General
Server
Motivation and Context
detectAndCleanupStaleBuildJobs()treats a running job without a Docker container as stale. A job that is pulling its image has no container yet by definition, so a slow pull is indistinguishable from a stuck job. Two things then go wrong:A slow pull gets the job killed. After the grace period the job is force-cancelled and requeued. The partial pull is discarded, the retry lands on another agent with an equally cold cache, and the same failure repeats.
The grace period is skipped entirely in the worst case. The
STALE_DETECTION_MIN_JOB_AGE_SECONDScheck only ran when the job was found in the distributed processing map:When the lookup returned nothing, execution fell through to the stale counter and the budget dropped from ~150 s to ~30 s. That lookup fails precisely when another agent has just force-cancelled and requeued the job, since that removes it from the map. A requeued retry is therefore more likely to be killed than the original attempt, which is what turns a single slow pull into a cancel-and-requeue loop.
Observed on the production build agents:
The job was killed 26 s after it started, while the pull it was waiting for needed roughly 190 s.
Reported in #13306, #13308 and #13310. The infrastructure trigger there (a registry mirror unreachable from the build agents, adding a ~32 s dial timeout to every Docker Hub request) is being fixed separately. This PR addresses the Artemis-side behaviour, which turns slow pulls into failed builds regardless of why a pull is slow.
Description
Do not treat jobs that are pulling an image as stale.
BuildAgentDockerServicenow tracks which build jobs are insidepullDockerImage, and exposesisImagePullInProgress(String). The registration covers the whole method, including the time a job spends waiting for the pull lock while another job downloads the same image, since those jobs have no container either.SharedQueueProcessingServiceskips stale detection for such jobs.Fail safe when the job is unknown. If the build start date cannot be determined, stale detection is skipped instead of counting the job as stale. Genuinely orphaned jobs are still cleaned up by the existing cross-check further down the same method, which removes jobs that are in the distributed map but not running locally.
Bound image pulls explicitly.
awaitCompletion()was called with no timeout, so a pull that stopped making progress would block the build thread forever. In practice the watchdog was the only thing bounding it, and that backstop is now removed for pulls, so the bound has to be real: pulls useawaitCompletion(timeout, SECONDS), and on expiry the callback is closed to abort the pull and a clear message is written to the build log.Reject a non-positive pull timeout at startup.
awaitCompletion(0, SECONDS)returns immediately, so a misconfigured value would report every pull as timed out and no build could run. The service fails fast in@PostConstructwith a message naming the property, matching the existing validation ofbuild-agent.short-nameinSharedQueueProcessingService.init().Should the pull timeout be separate from the build timeout?
Yes, and this PR adds it as
artemis.continuous-integration.image-pull-timeout-seconds(default 300) rather than reusing the build timeout:build-timeout-seconds.max(240 s by default). A cold pull of a large image legitimately exceeds that cap, so folding the two together would either fail slow pulls or force every exercise timeout to absorb infrastructure latency.The default of 300 s is generous compared to what a pull was effectively allowed before, while still being shorter than the whole build timeout used to be (120 to 240 s for pull plus build together). A stuck pull holds a slot in the shared queue, so a much longer default would let one agent hold up the cluster.
A second property,
artemis.continuous-integration.image-pull-stall-timeout-seconds(default 60), separates the two failure modes so the slow case does not need the long timeout:Agents on a genuinely slow link can still raise
image-pull-timeout-secondsexplicitly.Steps for Testing
Prerequisites:
docker rmi ls1tum/artemis-maven-template:java17-25registry-mirrorsentry to/etc/docker/daemon.jsonand restarting Docker, which adds a dial timeout to every Docker Hub request.Could not pull Docker image, and the agent log must not containForce-cancelling and requeuingfor this job.Job ... is currently pulling its Docker image, skipping stale detection.artemis.continuous-integration.image-pull-timeout-seconds: 5and repeat with a large uncached image. The build fails withPulling docker image ... timed out after 5 secondsin the build log, rather than hanging.Exam Mode Testing
Build jobs from exams go through the same build agent path with the same queue and the same watchdog, so exam programming exercises benefit identically. There are no UI changes.
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Note: Some tests in the Test job did not pass (
failure). Coverage below may be partial.Server
Last updated: 2026-08-11 18:35:01 UTC
Screenshots
Not applicable, server-side only.
🤖 Generated with Claude Code
Summary by CodeRabbit