nixl_ep: Handle planned SIGTERM in elastic test - #1500
Merged
Merged
Conversation
elastic.py intentionally sends SIGTERM to simulate rank failure during the elastic test, but PR ai-dynamo#1427 treats those expected exits as worker errors. Ignore planned SIGTERM exits when checking worker exit codes after the test completes.
|
👋 Hi itayalroy! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Contributor
Author
|
/build |
itayalroy
enabled auto-merge (squash)
April 7, 2026 11:16
itayalroy
disabled auto-merge
April 7, 2026 11:16
itayalroy
enabled auto-merge (squash)
April 7, 2026 11:16
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced the Python-level Changes
Sequence Diagram(s)sequenceDiagram
participant OS as OS Signal Layer
participant Handler as Signal Handler\n(handle_sigterm)
participant Process as Current Process
participant Main as main()\n(manager)
participant Worker as Worker Process
rect rgba(244, 67, 54, 0.5)
Note over OS,Handler: Old behavior
OS->>Handler: SIGTERM
Handler->>Handler: sys.exit(1)
Handler->>Process: Process exits (code 1)
Main->>Main: Inspect worker exitcode (!= 0)
Main->>Main: Mark worker as failure
end
rect rgba(33, 150, 243, 0.5)
Note over OS,Handler: New behavior
OS->>Handler: SIGTERM
Handler->>Handler: signal.signal(signum, SIG_DFL)
Handler->>Handler: signal.raise_signal(signum)
OS->>Process: Deliver default SIGTERM (terminate)
Process->>Main: Worker exitcode == -signal.SIGTERM
Main->>Main: Treat as non-failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/device/ep/tests/elastic/elastic.py`:
- Around line 652-653: The current check treats any -signal.SIGTERM as expected;
instead only ignore SIGTERM for workers we intentionally self-killed: add a
marker (e.g., a set like self_terminated or flag on the process) when you
perform the explicit self-kill (where handle_sigterm()-related self-kill is
invoked), then change the failure condition around p.exitcode in the loop to
treat -signal.SIGTERM as acceptable only if that process/index is in the
self_terminated marker; otherwise record it as a failure. Ensure
handle_sigterm() still re-raises for unexpected SIGTERMs and do not alter its
unconditional behavior for external signals.
🪄 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: Pro
Run ID: e3db93bd-6c3c-4cf4-b1a8-e401f35ba64b
📒 Files selected for processing (1)
examples/device/ep/tests/elastic/elastic.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
examples/device/ep/tests/elastic/elastic.py (1)
70-71: 🧹 Nitpick | 🔵 TrivialDocument why the handler re-sends the signal.
Lines 70-71 are doing the right cleanup, but restoring
SIG_DFLand re-issuingSIGTERMis not obvious at a glance. A short inline comment here would make it clear that the process is meant to terminate with the original signal-derived exit status, not via Python-levelSystemExit.In Python's `signal` module, what is the effect of restoring `signal.SIG_DFL` for `SIGTERM` and then calling `os.kill(os.getpid(), signal.SIGTERM)` from inside the signal handler?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/device/ep/tests/elastic/elastic.py` around lines 70 - 71, Add a short inline comment just above the signal restoration and resend (the lines calling signal.signal(signum, signal.SIG_DFL) and os.kill(os.getpid(), signum)) that explains that restoring SIG_DFL and re-issuing the original signal causes the process to terminate using the default OS signal handling and exit status (rather than returning to Python and raising SystemExit), ensuring the process exits with the original signal-derived status; reference the signum variable and the use of os.kill/os.getpid in the comment so readers can immediately understand intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/device/ep/tests/elastic/elastic.py`:
- Around line 70-71: Add a short inline comment just above the signal
restoration and resend (the lines calling signal.signal(signum, signal.SIG_DFL)
and os.kill(os.getpid(), signum)) that explains that restoring SIG_DFL and
re-issuing the original signal causes the process to terminate using the default
OS signal handling and exit status (rather than returning to Python and raising
SystemExit), ensuring the process exits with the original signal-derived status;
reference the signum variable and the use of os.kill/os.getpid in the comment so
readers can immediately understand intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c678001b-2c65-44a8-acfd-ba9a85c65a05
📒 Files selected for processing (1)
examples/device/ep/tests/elastic/elastic.py
Contributor
Author
|
/build |
guy-ealey-morag
previously approved these changes
Apr 10, 2026
ofirfarjun7
reviewed
Apr 12, 2026
Contributor
Author
|
/build |
ofirfarjun7
approved these changes
Apr 12, 2026
guy-ealey-morag
approved these changes
Apr 13, 2026
This was referenced Jul 16, 2026
Merged
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
3 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.
elastic.py intentionally sends SIGTERM to simulate rank failure during the elastic test, but PR #1427 treats those expected exits as worker errors. Ignore planned SIGTERM exits when checking worker exit codes after the test completes.
Summary by CodeRabbit