Skip to content

[Bug] Single-sbatch termination handling#971

Merged
podkidyshev merged 1 commit into
release-v171from
ipod/ssbatch-sigterm
Jul 21, 2026
Merged

[Bug] Single-sbatch termination handling#971
podkidyshev merged 1 commit into
release-v171from
ipod/ssbatch-sigterm

Conversation

@podkidyshev

Copy link
Copy Markdown
Contributor

Summary

Single-sbatch cancellation didn't cancel the slurm job

Test Plan

  • Automated CI
  • Manual runs

Additional Notes

@podkidyshev podkidyshev added the bug Something isn't working label Jul 20, 2026
@podkidyshev podkidyshev self-assigned this Jul 20, 2026
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

SingleSbatchRunner.run now tracks submitted jobs, kills active jobs during shutdown, and always clears job tracking. Tests cover shutdown during monitoring or submission and normal completion without killing completed jobs.

Changes

Slurm job lifecycle

Layer / File(s) Summary
Track and terminate submitted jobs
src/cloudai/systems/slurm/single_sbatch_runner.py
Submitted jobs are tracked, killed when shutdown occurs during monitoring, and removed from tracking in a finally block.
Validate lifecycle cleanup
tests/test_single_sbatch_runner.py
Tests cover shutdown during monitoring or submission and confirm completed jobs are not killed while tracking is cleared.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: jeffnvidia

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title matches the main change by describing single-sbatch termination handling, though it's a bit broader than the exact cancellation fix.
Description check ✅ Passed The description is directly about single-sbatch cancellation not canceling the Slurm job, which matches the PR changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ipod/ssbatch-sigterm

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/cloudai/systems/slurm/single_sbatch_runner.py`:
- Around line 190-193: The job registration and shutdown check in
SingleSbatchRunner must prevent duplicate cancellation when shutdown occurs
between them. In src/cloudai/systems/slurm/single_sbatch_runner.py lines
190-193, synchronize registration with shutdown or make cancellation idempotent
so each job is killed exactly once. In tests/test_single_sbatch_runner.py lines
558-575, add coverage that triggers shutdown from jobs.append() and asserts
exactly one kill.

In `@tests/test_single_sbatch_runner.py`:
- Around line 558-575: Extend the shutdown regression coverage in
test_run_cancels_job_when_shutdown_occurs_during_submission by replacing or
augmenting the jobs collection with a custom append implementation that calls
runner.shutdown() after the submitted job is registered. Run the submission flow
and assert SlurmSystem.kill is called exactly once for the job, covering
shutdown cancellation after registration and preventing duplicate cancellation.
🪄 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: ASSERTIVE

Plan: Enterprise

Run ID: 35b2fe29-bf0f-4f89-9990-4955a2d6b237

📥 Commits

Reviewing files that changed from the base of the PR and between 09f728c and 4061184.

📒 Files selected for processing (2)
  • src/cloudai/systems/slurm/single_sbatch_runner.py
  • tests/test_single_sbatch_runner.py

Comment thread src/cloudai/systems/slurm/single_sbatch_runner.py
Comment thread tests/test_single_sbatch_runner.py
@podkidyshev
podkidyshev changed the base branch from main to release-v171 July 20, 2026 11:20
Comment thread src/cloudai/systems/slurm/single_sbatch_runner.py

@jj10306 jj10306 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

@podkidyshev
podkidyshev merged commit f597e38 into release-v171 Jul 21, 2026
6 checks passed
@podkidyshev
podkidyshev deleted the ipod/ssbatch-sigterm branch July 21, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants