Fix future hang when a queuing-system job dies without output - #1038
Merged
jan-janssen merged 7 commits intoAug 7, 2026
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>
Contributor
|
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:
📝 WalkthroughWalkthroughThe file scheduler now queries queue status for tasks missing output, throttles repeated checks, fails futures for absent or errored jobs, and propagates validation state through refresh and shutdown paths. PySQA status helpers, scheduler wiring, and regression tests were added. ChangesDead job detection
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant OutputChecker
participant PySQA
participant Future
Scheduler->>OutputChecker: inspect task output
OutputChecker->>PySQA: query queue_id when output is absent
PySQA-->>OutputChecker: return status or None
OutputChecker->>Future: set RuntimeError for dead or errored job
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Member
Author
|
@copilot On windows I get the following errors, please skip these tests on windows: |
Contributor
... Done — added |
Member
Author
|
@copilot skip the following tests when |
Contributor
... Fixed — added a |
jan-janssen
marked this pull request as draft
July 22, 2026 08:53
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1038 +/- ##
==========================================
- Coverage 94.22% 94.19% -0.03%
==========================================
Files 39 39
Lines 2113 2137 +24
==========================================
+ Hits 1991 2013 +22
- Misses 122 124 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jan-janssen
marked this pull request as ready for review
August 6, 2026 15:36
Contributor
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 `@tests/unit/executor/test_flux_cluster.py`:
- Around line 248-264: Use a unique test-specific cache directory in the
FluxClusterExecutor test, and pass that same directory to both the executor’s
cache_directory parameter and get_cache_data(). Ensure queue_id discovery only
scans entries created by this test, avoiding interference from unrelated jobs.
In `@tests/unit/task_scheduler/file/test_backend.py`:
- Line 266: Update the serialize_funct assignment in the test to bind the unused
second return value to "_" instead of data_dict, while preserving task_key
usage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61bc90f4-c599-4374-b8cd-0060a176b415
📒 Files selected for processing (3)
src/executorlib/task_scheduler/file/shared.pytests/unit/executor/test_flux_cluster.pytests/unit/task_scheduler/file/test_backend.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/executorlib/task_scheduler/file/shared.py
Comment on lines
+248
to
+264
| with FluxClusterExecutor( | ||
| resource_dict={"cores": 1, "cwd": "executorlib_cache"}, | ||
| block_allocation=False, | ||
| cache_directory="executorlib_cache", | ||
| pmi_mode=pmi, | ||
| ) as exe: | ||
| cloudpickle_register(ind=1) | ||
| future = exe.submit(long_running_function, 1) | ||
|
|
||
| queue_id = None | ||
| for _ in range(200): | ||
| for entry in get_cache_data(cache_directory="executorlib_cache"): | ||
| if entry.get("queue_id") is not None: | ||
| queue_id = entry["queue_id"] | ||
| if queue_id is not None: | ||
| break | ||
| sleep(0.1) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a test-specific cache directory.
get_cache_data() scans all entries in executorlib_cache. If another entry has a queue_id, this test can cancel that unrelated job and leave future running. Create a unique cache directory for this test and use it for both FluxClusterExecutor and get_cache_data().
🤖 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/executor/test_flux_cluster.py` around lines 248 - 264, Use a
unique test-specific cache directory in the FluxClusterExecutor test, and pass
that same directory to both the executor’s cache_directory parameter and
get_cache_data(). Ensure queue_id discovery only scans entries created by this
test, avoiding interference from unrelated jobs.
| # https://github.com/pyiron/executorlib/issues/1037. | ||
| cache_directory = os.path.abspath("executorlib_cache") | ||
| os.makedirs(cache_directory, exist_ok=True) | ||
| task_key, data_dict = serialize_funct(fn=my_funct, fn_args=[1], fn_kwargs={"b": 2}) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused data_dict binding.
This assignment triggers Ruff RUF059. Replace data_dict with _.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 266-266: Unpacked variable data_dict is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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` at line 266, Update the
serialize_funct assignment in the test to bind the unused second return value to
"_" instead of data_dict, while preserving task_key usage.
Source: Linters/SAST tools
…-slurm-cluster-executor-job-timeout-hang
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/executorlib/task_scheduler/file/shared.py (1)
412-421: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPass
validate_functionto_check_task_output.This call omits
validate_function._check_task_outputtherefore always receives its defaultNonevalue and skips queue-status validation. After the pending-Future fix, dead queue jobs will remain unresolved instead of failing as required.Proposed fix
backend=backend, + validate_function=validate_function, status_check_dict=status_check_dict,🤖 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/executorlib/task_scheduler/file/shared.py` around lines 412 - 421, Update the `_check_task_output` call in the task-scheduling flow to pass the available `validate_function` argument, preserving its use for queue-status validation and dead-job failure handling. Keep the existing arguments and behavior unchanged otherwise.
🤖 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/executorlib/standalone/command_pysqa.py`:
- Around line 45-49: Update the status-check throttling around status_check_dict
and last_checked so tasks without an existing task_key entry always perform
their first status query. Apply the interval check only when a prior check
timestamp is present, while preserving the existing throttling behavior for
previously checked tasks.
In `@src/executorlib/task_scheduler/file/shared.py`:
- Around line 246-247: Update the validation-unavailable branch in the task
scheduling flow to return the task’s existing pending Future instead of False.
Preserve the Future contract expected by the refresh logic, including its
.done() method, so pending subprocess tasks remain scheduled until validation
becomes available.
---
Outside diff comments:
In `@src/executorlib/task_scheduler/file/shared.py`:
- Around line 412-421: Update the `_check_task_output` call in the
task-scheduling flow to pass the available `validate_function` argument,
preserving its use for queue-status validation and dead-job failure handling.
Keep the existing arguments and behavior unchanged otherwise.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1adf576c-3b13-4c7c-9493-451530bc2b3f
📒 Files selected for processing (3)
src/executorlib/standalone/command_pysqa.pysrc/executorlib/task_scheduler/file/shared.pysrc/executorlib/task_scheduler/file/task_scheduler.py
Comment on lines
+45
to
+49
| last_checked = ( | ||
| status_check_dict.get(task_key, 0.0) if status_check_dict is not None else 0.0 | ||
| ) | ||
| if now - last_checked < job_status_check_interval: | ||
| return False |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not throttle the first status check.
A task that has no status_check_dict entry uses 0.0 as last_checked. If monotonic() is below 30 seconds, the first status query is skipped. Only throttle tasks that have an actual prior check time.
Proposed fix
- last_checked = (
- status_check_dict.get(task_key, 0.0) if status_check_dict is not None else 0.0
- )
- if now - last_checked < job_status_check_interval:
+ last_checked = status_check_dict.get(task_key) if status_check_dict is not None else None
+ if (
+ last_checked is not None
+ and now - last_checked < job_status_check_interval
+ ):
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| last_checked = ( | |
| status_check_dict.get(task_key, 0.0) if status_check_dict is not None else 0.0 | |
| ) | |
| if now - last_checked < job_status_check_interval: | |
| return False | |
| last_checked = status_check_dict.get(task_key) if status_check_dict is not None else None | |
| if ( | |
| last_checked is not None | |
| and now - last_checked < job_status_check_interval | |
| ): | |
| return False |
🤖 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/executorlib/standalone/command_pysqa.py` around lines 45 - 49, Update the
status-check throttling around status_check_dict and last_checked so tasks
without an existing task_key entry always perform their first status query.
Apply the interval check only when a prior check timestamp is present, while
preserving the existing throttling behavior for previously checked tasks.
jan-janssen
force-pushed
the
fix/1037-slurm-cluster-executor-job-timeout-hang
branch
from
August 7, 2026 04:13
5546e3c to
e23aee1
Compare
* Move job validation to pysqa module * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fixes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * mypy fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix tests * fix --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
jan-janssen
deleted the
fix/1037-slurm-cluster-executor-job-timeout-hang
branch
August 7, 2026 06:51
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
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
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
New Features
Bug Fixes
Tests