Fix future hang when a queuing-system job dies without output#1038
Fix future hang when a queuing-system job dies without output#1038jan-janssen wants to merge 3 commits into
Conversation
FileTaskScheduler only resolved a task's future once its _o.h5 output file appeared, so a job killed by the scheduler (walltime TIMEOUT, OOM, NODE_FAIL, scancel) never wrote that file and future.result() blocked forever. _check_task_output now falls back to querying the job status via pysqa when the output file is missing and fails the future if the job is no longer known to the queuing system. The status query is throttled per task (_JOB_STATUS_CHECK_INTERVAL) and imported lazily so subprocess-only task submissions never pay the pysqa import cost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe file scheduler now queries queue status for tasks missing output, throttles repeated checks, fails futures for unknown jobs, and propagates polling state through refresh and shutdown paths. A public pysqa status helper and unit tests were added. ChangesDead job detection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant OutputChecker
participant QueueStatus
participant Future
Scheduler->>OutputChecker: inspect task output
OutputChecker->>QueueStatus: query queue_id
QueueStatus-->>OutputChecker: status or None
OutputChecker->>Future: fail when job is unknown and output is absent
Possibly related PRs
🚥 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.
🧹 Nitpick comments (2)
tests/unit/task_scheduler/file/test_backend.py (2)
266-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest throttle expiry, not only immediate suppression.
The loop on Line 275 proves one lookup for back-to-back polls, but a throttle that never expires would also pass. Mock the scheduler clock, advance it through the 30-second interval, return
Noneon the second lookup, and assert the future fails. This protects against reintroducing the original indefinite-pending behavior.🤖 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 `@tests/unit/task_scheduler/file/test_backend.py` around lines 266 - 285, The test_check_task_output_status_check_is_throttled test only verifies immediate suppression, not throttle expiry. Mock the scheduler clock used by _check_task_output, perform the initial polls, advance time beyond the 30-second interval, make the next status lookup return None, and assert that the Future from future_obj fails after the expired-throttle check.
223-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the output-arrival race.
This verifies the failure path, but not the required case where the job is absent from the queue and writes its output immediately after the status lookup. Add a test that makes the mocked lookup create a valid output file, then assert successful completion; otherwise a regression can incorrectly fail completed work.
🤖 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 `@tests/unit/task_scheduler/file/test_backend.py` around lines 223 - 246, Add a test alongside test_check_task_output_dead_job_without_output that mocks pysqa_get_status_of_job to create a valid serialized output file in cache_directory before returning no job status, then invokes _check_task_output and asserts the Future completes successfully with the expected result. This must cover the race where output appears immediately after the status lookup and preserve successful completion instead of raising RuntimeError.
🤖 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.
Nitpick comments:
In `@tests/unit/task_scheduler/file/test_backend.py`:
- Around line 266-285: The test_check_task_output_status_check_is_throttled test
only verifies immediate suppression, not throttle expiry. Mock the scheduler
clock used by _check_task_output, perform the initial polls, advance time beyond
the 30-second interval, make the next status lookup return None, and assert that
the Future from future_obj fails after the expired-throttle check.
- Around line 223-246: Add a test alongside
test_check_task_output_dead_job_without_output that mocks
pysqa_get_status_of_job to create a valid serialized output file in
cache_directory before returning no job status, then invokes _check_task_output
and asserts the Future completes successfully with the expected result. This
must cover the race where output appears immediately after the status lookup and
preserve successful completion instead of raising RuntimeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95f01696-151e-44e9-9eea-110efdd1c23f
📒 Files selected for processing (3)
src/executorlib/standalone/command_pysqa.pysrc/executorlib/task_scheduler/file/shared.pytests/unit/task_scheduler/file/test_backend.py
|
@copilot On windows I get the following errors, please skip these tests on windows: |
... Done — added |
|
@copilot skip the following tests when |
... Fixed — added a |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1038 +/- ##
==========================================
- Coverage 94.22% 94.15% -0.07%
==========================================
Files 39 39
Lines 2113 2140 +27
==========================================
+ Hits 1991 2015 +24
- Misses 122 125 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
SlurmClusterExecutor/FluxClusterExecutorfutures hung forever when the backing queuing-system job died before writing its output file (walltime TIMEOUT, OOM, NODE_FAIL, externalscancel)._check_task_outputnow falls back to querying the job status viapysqa.QueueAdapter.get_status_of_jobwhen the expected_o.h5output file is missing, and fails the future with aRuntimeErrorif the queuing system no longer knows about the job._JOB_STATUS_CHECK_INTERVAL = 30s) to avoid floodingsqueue/saccton every poll of the much fasterrefresh_rateloop, and is imported lazily so subprocess-only (non-pysqa) task submissions never pay thepysqaimport cost.Test plan
backend=None) tasks never trigger a queuing-system status query.pytest tests/unitpasses (pre-existing failures for missing optionalnetworkx/pygraphvizextras confirmed present onmaintoo, unrelated to this change).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests