fix(jobs): prevent Docker job scheduling races under queue pressure - #615
Merged
Conversation
ironcommit
changed the title
ironcommit
changed the title
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/core/jobs/src/nmp/core/jobs/controllers/scheduler.py (1)
126-169: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard error-path status writes.
_update_step_status_with_timing()re-raises on failure, so theResourceAllocationErrorand genericExceptionbranches can still escape the per-step try and stop the rest of the batch. Wrap those calls like the success path’s stale-409 handling.🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/scheduler.py` around lines 126 - 169, The error-path status updates in the scheduler can still raise and break the per-step loop because `_update_step_status_with_timing()` is called directly in both the `ResourceAllocationError` and generic `Exception` handlers. Update the `Scheduler` logic to wrap those calls in the same stale-409 handling pattern used on the success path, likely within the `except ResourceAllocationError` and `except Exception` branches of the scheduling loop, so failures to persist status do not stop processing remaining steps.
🧹 Nitpick comments (2)
services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded worker count, no env override.
DOCKER_CONTAINER_START_WORKERS = 10is a fixed constant. Given this PR's goal is tuning concurrency under queue pressure, consider making it configurable via env var so it can be tuned per-deployment without a code change.♻️ Proposed fix
-DOCKER_CONTAINER_START_WORKERS = 10 +DOCKER_CONTAINER_START_WORKERS = int(os.environ.get("DOCKER_CONTAINER_START_WORKERS", "10"))🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py` at line 92, The worker count in DOCKER_CONTAINER_START_WORKERS is hardcoded, so make it configurable via an environment variable instead of a fixed 10. Update the docker backend configuration in the module that defines DOCKER_CONTAINER_START_WORKERS to read from env with a sensible default, and ensure the code paths that use this constant continue to reference the same symbol so deployments can tune concurrency without code changes.services/core/jobs/src/nmp/core/jobs/controllers/reconciler.py (1)
121-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate status-update-with-timing helper across scheduler and reconciler.
_update_step_status_with_timinghere mirrors the same-named helper added toscheduler.py(per graph context), differing only in the identifying kwargs (provider/profilevsphase) and both wrappingupdate_statuswith monotonic timing + success/failure debug/warning logs. Consider extracting a shared utility (e.g., a free function or mixin taking genericextracontext) to avoid maintaining two near-identical implementations.🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/reconciler.py` around lines 121 - 221, The _update_step_status_with_timing helper in reconciler duplicates the same timing-and-logging wrapper already present in scheduler, so consolidate both into a shared utility or mixin that wraps update_status once and accepts generic extra context. Refactor the reconciler’s _update_step_status_with_timing and the scheduler counterpart to call the shared helper, preserving the existing success/failure logging and timing behavior while only passing their specific context fields (provider/profile or phase).
🤖 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.
Outside diff comments:
In `@services/core/jobs/src/nmp/core/jobs/controllers/scheduler.py`:
- Around line 126-169: The error-path status updates in the scheduler can still
raise and break the per-step loop because `_update_step_status_with_timing()` is
called directly in both the `ResourceAllocationError` and generic `Exception`
handlers. Update the `Scheduler` logic to wrap those calls in the same stale-409
handling pattern used on the success path, likely within the `except
ResourceAllocationError` and `except Exception` branches of the scheduling loop,
so failures to persist status do not stop processing remaining steps.
---
Nitpick comments:
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py`:
- Line 92: The worker count in DOCKER_CONTAINER_START_WORKERS is hardcoded, so
make it configurable via an environment variable instead of a fixed 10. Update
the docker backend configuration in the module that defines
DOCKER_CONTAINER_START_WORKERS to read from env with a sensible default, and
ensure the code paths that use this constant continue to reference the same
symbol so deployments can tune concurrency without code changes.
In `@services/core/jobs/src/nmp/core/jobs/controllers/reconciler.py`:
- Around line 121-221: The _update_step_status_with_timing helper in reconciler
duplicates the same timing-and-logging wrapper already present in scheduler, so
consolidate both into a shared utility or mixin that wraps update_status once
and accepts generic extra context. Refactor the reconciler’s
_update_step_status_with_timing and the scheduler counterpart to call the shared
helper, preserving the existing success/failure logging and timing behavior
while only passing their specific context fields (provider/profile or phase).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d7a3109b-2bd4-429e-bfe2-e3d27cc2afe2
📒 Files selected for processing (10)
services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/exceptions.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/reconciler.pyservices/core/jobs/src/nmp/core/jobs/controllers/scheduler.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.pyservices/core/jobs/tests/controllers/test_scheduler.pyservices/core/jobs/tests/controllers/test_subprocess_backend.pyservices/core/jobs/tests/controllers/test_volcano_backend.py
💤 Files with no reviewable changes (1)
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py
Contributor
|
mckornfield
approved these changes
Jul 8, 2026
ironcommit
force-pushed
the
jobs-race/rsadler
branch
from
July 9, 2026 04:04
ce8a399 to
5e6487f
Compare
Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
ironcommit
force-pushed
the
jobs-race/rsadler
branch
from
July 9, 2026 17:27
5e6487f to
ca85989
Compare
ironcommit
enabled auto-merge
July 9, 2026 17:27
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py (1)
684-714: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up resources on cancellation exits.
Line 702 allocates task/config volumes before cancellation checks. Lines 867 and 1046 can return without cleanup; Line 1046 can also leave a just-created
createdcontainer, whichcleanup_steps()never scans.Proposed direction
def _run_container_in_thread(self, step: PlatformJobStepWithContext, container_args: dict): status_details = {} status = PlatformJobStatus.PENDING.value + task_id = container_args.get("labels", {}).get(JOB_TASK_ID_LABEL) + created_container = False @@ if self.cancel_scheduling(step): + if task_id: + self.cleanup_task_storage_volumes(step.workspace, step.job, task_id) logger.debug( @@ - container = self._client.containers.create(**container_args) + container = self._client.containers.create(**container_args) + created_container = True @@ - container = self._client.containers.create(**container_args) + container = self._client.containers.create(**container_args) + created_container = True @@ if self.cancel_scheduling(step): + if created_container: + self.cleanup_container(container) + if task_id: + self.cleanup_task_storage_volumes(step.workspace, step.job, task_id) logger.debug(Also applies to: 866-876, 1045-1056, 1503-1515
🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py` around lines 684 - 714, Ensure cancellation/early-exit paths in the Docker backend clean up any resources already created by _prepare_container_args_for_start and related launch flow. In the container startup path, add cleanup before returning on cancellation at the points that currently exit from the main start logic, and make sure a just-created container is removed/destroyed even if it never reaches the normal cleanup scan used by cleanup_steps(). Use the existing helpers around _prepare_container_args_for_start, ensure_job_storage, and the created container handling to centralize the cleanup so task/config volumes and orphaned containers are not left behind.
🤖 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.
Outside diff comments:
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py`:
- Around line 684-714: Ensure cancellation/early-exit paths in the Docker
backend clean up any resources already created by
_prepare_container_args_for_start and related launch flow. In the container
startup path, add cleanup before returning on cancellation at the points that
currently exit from the main start logic, and make sure a just-created container
is removed/destroyed even if it never reaches the normal cleanup scan used by
cleanup_steps(). Use the existing helpers around
_prepare_container_args_for_start, ensure_job_storage, and the created container
handling to centralize the cleanup so task/config volumes and orphaned
containers are not left behind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 653a9954-504b-4d5c-a115-454458e85a8f
📒 Files selected for processing (10)
services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/exceptions.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/reconciler.pyservices/core/jobs/src/nmp/core/jobs/controllers/scheduler.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.pyservices/core/jobs/tests/controllers/test_scheduler.pyservices/core/jobs/tests/controllers/test_subprocess_backend.pyservices/core/jobs/tests/controllers/test_volcano_backend.py
💤 Files with no reviewable changes (1)
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py
🚧 Files skipped from review as they are similar to previous changes (8)
- services/core/jobs/src/nmp/core/jobs/controllers/backends/exceptions.py
- services/core/jobs/tests/controllers/test_scheduler.py
- services/core/jobs/tests/controllers/test_kubernetes_backend.py
- services/core/jobs/tests/controllers/test_subprocess_backend.py
- services/core/jobs/tests/controllers/test_volcano_backend.py
- services/core/jobs/src/nmp/core/jobs/controllers/scheduler.py
- services/core/jobs/src/nmp/core/jobs/controllers/reconciler.py
- services/core/jobs/tests/controllers/test_docker_backend.py
This was referenced Jul 9, 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.
Summary
Fixes job scheduling reliability under queue pressure by introducing admission control to the Docker backend, moving TTL enforcement out of individual backends into the scheduler/reconciler, and adding comprehensive debug logging across the scheduling pipeline.
Changes
Admission control for Docker container starts
BoundedSemaphore-based admission gate (_container_start_admission) that limits concurrent container start operations toDOCKER_CONTAINER_START_WORKERS(10)SchedulingDeferredinstead of blocking — the step stays in PENDING and is retried on the next scheduler tick_prepare_container_args_for_start) is extracted and runs inside the admission gate, so storage setup and image pulls don't block unrelated stepsfinallyblock inrun_container, ensuring slots are always freed even on failureTTL enforcement moved to scheduler/reconciler
ttl_seconds_before_activechecks fromDockerJobBackend.schedule()andSubprocessJobBackend.schedule()— these were racy because the backend could time out a step that the scheduler would then re-queuesync_pendingin the reconciler, preventing the TTL path from killing steps that are actually making progressScheduling order fix
-created_at(newest first) tocreated_at(oldest first), so older queued steps are scheduled before newer onesDebug logging
logger.debugtiming spans across the entire scheduling and reconciliation pipeline: fetch, backend sync, status updates, container create, image pull, storage setup, launcher lookup, cancellation checks, and cleanupdocker_state_debug_fields()helper that extracts Docker container state (exit code, OOM, timestamps, auto-remove config, cleanup TTL) into a dict for structured loggingparse_docker_timestamp()to safely handle Docker's zero-time sentinel and parse errorsupdate_statuscalls in_update_step_status_with_timinghelpers in both scheduler and reconciler for consistent timing and error loggingCleanup improvements
docker_state_debug_fields()for consistent state extraction and logs detailed context for each cleanup decision (immediate removal, TTL retention, skip due to non-terminal state)parse_docker_timestamphandles Docker's0001-01-01T00:00:00Zsentinel without crashingTests
_prepare_container_args_for_startextractionSchedulingDeferredhandling and sort order changeSchedulingDeferredto Kubernetes and Volcano backend test mocksTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests