Skip to content

Widen DeleteQueuedJobPreventsRun/DeleteRunningJobCancelsWorker timeouts - #1877

Open
ramakrishnap-nv wants to merge 1 commit into
mainfrom
fix-grpc-delete-job-flake
Open

Widen DeleteQueuedJobPreventsRun/DeleteRunningJobCancelsWorker timeouts#1877
ramakrishnap-nv wants to merge 1 commit into
mainfrom
fix-grpc-delete-job-flake

Conversation

@ramakrishnap-nv

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1814. DefaultServerTests.DeleteQueuedJobPreventsRun and DefaultServerTests.DeleteRunningJobCancelsWorker (GRPC_INTEGRATION_TEST) have flaked together across at least 3 CI runs on different platforms (see #1814 for prior evidence; latest occurrence: PR #1804, job 102618657558, 12.2.2, 3.11, amd64, rockylinux8, v100, earliest-driver, oldest-deps).

Root cause

Traced the full server-side job/worker lifecycle: cancel_job, delete_job, claim_job_slot, reset_job_slot, mark_worker_jobs_failed, worker_monitor_thread. Found no logic bug -- a cancelled queued-job slot is correctly skipped and freed by the next worker that claims it, and a dead worker's in-flight job slot is correctly reset by mark_worker_jobs_failed once waitpid reaps it.

Both tests share a single worker across the entire DefaultServerTests suite (SetUpTestSuite starts one cuopt_grpc_server --workers 1 for all 24 tests). Both failing tests' immediately preceding steps kill that worker via cancel_job (SIGKILL), so the replacement worker has to pay for a fresh CUDA context init (init_worker_cuda_environment) before it can claim the next job -- on top of whatever the actual solve takes. Both observed failures landed right at their timeout boundary rather than hanging indefinitely or failing near-instantly:

  • DeleteRunningJobCancelsWorker: "Job never reached PROCESSING before delete" after 10107ms, against a 10s (40 × 250ms) budget.
  • DeleteQueuedJobPreventsRun: probe still QUEUED after the full test took 62425ms, against a 60s wait_for_job_done budget.

That's the signature of a tight timeout margin under CI GPU contention, not a deadlock -- consistent with flaking specifically on earliest-driver/oldest-deps, the slowest CUDA-init combination in the matrix.

Fix

Widen both budgets to match this same file's own established headroom for a worker respawn (DeleteRunningJobCancelsWorker already waits "up to ~30s for the replacement worker" a few lines later, for the probe job):

  • DeleteRunningJobCancelsWorker's "wait for PROCESSING" budget: 10s → 30s (40 → 120 iterations of the same 250ms poll).
  • DeleteQueuedJobPreventsRun's probe wait_for_job_done: 60s → 90s.

Test plan

  • Reviewed the full worker lifecycle for an actual logic bug; found none -- this is a timeout-headroom fix, not a behavior change.
  • Minimal, mechanical diff (two timeout constants + explanatory comments); clang-format and pre-commit pass.
  • Could not build/run the test locally -- this dev environment's installed rmm is version-skewed against the source tree (cuda_stream_view has no member get/sync, in files unrelated to this change), a pre-existing environment issue. CI's pinned environment should build and run this cleanly.

Both tests share a single worker across the whole DefaultServerTests
suite. Their preceding steps kill the worker via cancel_job
(SIGKILL), and the replacement worker pays for a fresh CUDA context
init before it can claim the next job. Traced the full lifecycle
(cancel_job, delete_job, claim_job_slot, reset_job_slot,
mark_worker_jobs_failed, worker_monitor_thread) and found no logic
bug -- cancelled/dead-worker slot cleanup is correct. Both observed
failures landed right at their timeout boundary (10107ms against a
10s budget; 62425ms total against a 60s probe wait), which is the
signature of a tight timeout margin under CI GPU contention, not a
deadlock -- the CI failure that prompted this was specifically on the
earliest-driver/oldest-deps matrix combo, where CUDA context init is
slowest.

Bump DeleteRunningJobCancelsWorker's "wait for PROCESSING" budget
from 10s to 30s, matching this file's own established headroom for a
worker respawn ("up to ~30s for the replacement worker", used a few
lines later in the same test). Bump DeleteQueuedJobPreventsRun's
probe wait from 60s to 90s for the same reason.

Fixes #1814.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ramakrishnap-nv
ramakrishnap-nv requested a review from a team as a code owner September 9, 2026 20:52
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6f495475-d60c-4463-96be-046a1560e7c4

📥 Commits

Reviewing files that changed from the base of the PR and between 02dfb9e and 5598b25.

📒 Files selected for processing (1)
  • cpp/tests/linear_programming/grpc/grpc_integration_test.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The GRPC integration tests now allow more time for worker respawn and CUDA initialization. The queued-job completion wait increases to 90 seconds. The running-job processing poll increases to approximately 30 seconds.

Changes

GRPC integration test stability

Layer / File(s) Summary
Job deletion timeout windows
cpp/tests/linear_programming/grpc/grpc_integration_test.cpp
The queued-job test increases its completion wait from 60 to 90 seconds. The running-job test increases its processing poll from approximately 10 to 30 seconds. Comments document worker respawn and CUDA initialization delays.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 5598b

The GRPC integration tests now wait longer for worker replacement and CUDA initialization while preserving their existing completion and processing-state checks. No merge-blocking risk is evident.

Suggested reviewers: mlubin

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the two affected tests and the timeout changes, which matches the primary purpose of the pull request.
Description check ✅ Passed The description directly explains the flaky tests, the worker respawn and CUDA initialization delays, the timeout changes, and the testing status.
Linked Issues check ✅ Passed The changes address issue [#1814] by increasing the PROCESSING polling budget from 10 seconds to 30 seconds and the queued probe completion timeout from 60 seconds to 90 seconds. These changes directl…
Out of Scope Changes check ✅ Passed The changes are limited to the two affected integration tests and include only timeout adjustments with explanatory comments. No unrelated code changes are present.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-grpc-delete-job-flake

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

CI Test Summary

✅ All 31 test job(s) passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky: DefaultServerTests.DeleteQueuedJobPreventsRun / DeleteRunningJobCancelsWorker in GRPC_INTEGRATION_TEST

1 participant