ci: prevent orphaned DeepSeek V4 engines on test interruption - #86
ci: prevent orphaned DeepSeek V4 engines on test interruption#86luohuan19 wants to merge 3 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds signal-aware DeepSeek V4 subprocess teardown, bounded process-group cleanup, and tests for termination behavior. CI guard timeouts increase for Qwen3 and DeepSeek V4, while orphaned ChangesCI guard resilience
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Runner
participant DeepSeekTest
participant DeepSeekServer
participant ProcessGroup
Runner->>DeepSeekTest: deliver SIGTERM
DeepSeekTest->>DeepSeekTest: raise TerminatedBySignal
DeepSeekTest->>ProcessGroup: stop with SIGTERM grace
ProcessGroup->>DeepSeekServer: send SIGTERM
ProcessGroup->>DeepSeekServer: escalate to SIGKILL if needed
DeepSeekTest-->>Runner: complete teardown
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces robust signal handling and process termination mechanisms to prevent orphaned engine processes during test runs, especially when terminated by a CI runner. It adds a context manager to translate termination signals into a custom exception, uses PR_SET_PDEATHSIG to ensure child processes are killed if the parent dies, and adjusts termination grace periods based on whether the run was interrupted. The review feedback suggests loading libc at the module level to avoid unsafe dlopen calls after fork, utilizing this pre-loaded instance in _set_pdeathsig to maintain async-signal safety, and replacing the fragile use of sys.exc_info() in the finally block with an explicit except TerminatedBySignal block to set an interruption flag.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_deepseek_v4_accuracy.py (1)
291-306: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPost-SIGKILL reap wait ignores
term_grace, undermining the interrupted-path budget.
_stop_process_groupcorrectly bounds the first wait (line 291) and the descendant sweep (line 311) byterm_grace, but the fallbackprocess.wait(timeout=10)at line 298 (reached after escalating to SIGKILL) is still hardcoded. On the interrupted path (term_grace=SIGNAL_TERM_GRACE_SECONDS=1.0), if the process doesn't die within 1s of SIGTERM, this can add up to 10 more seconds — blowing well past the "~3s" budget the header comment (lines 47-51) says the whole teardown must fit inside, and letting the runner's own SIGKILL hit pytest before the final descendant-kill sweep (lines 312-321) ever runs. That's exactly the orphan scenario this PR sets out to prevent, and it's most likely to bite precisely when a process is stuck on NPU/device I/O and slow to die.
test_stop_process_group_honours_term_grace(430-449) doesn't catch this: it only assertswaits[0], not the secondwait()call.🔧 Proposed fix: bound the reap-wait by term_grace too
+SIGKILL_REAP_SECONDS = 10.0 + def _stop_process_group( process: subprocess.Popen, term_grace: float = NORMAL_TERM_GRACE_SECONDS ) -> None: ... try: - process.wait(timeout=10) + process.wait(timeout=min(SIGKILL_REAP_SECONDS, term_grace)) except subprocess.TimeoutExpired:And extend the test to catch a regression:
_stop_process_group(StuckProcess(), term_grace=SIGNAL_TERM_GRACE_SECONDS) assert waits[0] == SIGNAL_TERM_GRACE_SECONDS + assert waits[1] == SIGNAL_TERM_GRACE_SECONDS assert signals[:2] == [signal.SIGTERM, signal.SIGKILL]Also applies to: 430-449
🤖 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/test_deepseek_v4_accuracy.py` around lines 291 - 306, Update _stop_process_group so the post-SIGKILL process.wait call uses term_grace instead of the hardcoded 10-second timeout, preserving the teardown budget and allowing the descendant sweep to run promptly. Extend test_stop_process_group_honours_term_grace to assert the second wait also receives term_grace.
🤖 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 @.github/workflows/ci.yml:
- Around line 277-279: Update the orphan-task cleanup loop around task-submit
--kill so n increments only when the kill command succeeds; preserve the warning
output for failed terminations and ensure the final killed-task summary reflects
successful kills only.
---
Outside diff comments:
In `@tests/test_deepseek_v4_accuracy.py`:
- Around line 291-306: Update _stop_process_group so the post-SIGKILL
process.wait call uses term_grace instead of the hardcoded 10-second timeout,
preserving the teardown budget and allowing the descendant sweep to run
promptly. Extend test_stop_process_group_honours_term_grace to assert the second
wait also receives term_grace.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be7a1303-f72a-4d34-a501-8d89333e1a9d
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/test_deepseek_v4_accuracy.py
a85fd20 to
ede9e36
Compare
The DeepSeek V4 accuracy guard left ~800 GiB of orphaned engines behind whenever the CI runner interrupted it: a bare SIGTERM takes CPython down at SIG_DFL without unwinding the stack, so the `finally` that reaps the server (started in its own session via start_new_session) never ran, and the engine lived on holding its NPU cards and shared memory. Test hardening: - _raise_on_termination(): trap SIGTERM/SIGINT/SIGHUP and re-raise as TerminatedBySignal (a KeyboardInterrupt) so `finally` teardown runs; deafen the signals first so the runner's escalation can't abort cleanup mid-flight. - _set_pdeathsig() preexec hook: PR_SET_PDEATHSIG backstops the one signal we cannot catch (SIGKILL of pytest), closing the fork/prctl race. - _stop_process_group(term_grace): escalate to SIGKILL within ~1s on the interrupted path (the runner SIGKILLs us ~3s after SIGTERM) and stay patient (20s) on the normal path. - Cover the new paths with unit tests. CI workflow: - Switch the orphan-cleanup step from `task-submit --list` (truncates the command column to 77 chars, so the marker grep can silently miss) to `--find`, which matches the untruncated command and prints bare task-ids; report how many tasks were killed. - Raise task-submit --timeout 1200 -> 3600 to tolerate longer device acquisition.
- Load libc.so.6 at module import instead of inside the preexec_fn, so the child does not dlopen after fork (unsafe if another thread held the loader lock across the fork); _set_pdeathsig() now uses the pre-loaded handle. - Replace the fragile sys.exc_info() probe in the finally block with an explicit `except TerminatedBySignal: interrupted = True; raise`.
Increment the killed counter inside the success branch so the "killed N orphaned task(s)" summary matches the warnings and never overcounts a kill that actually failed.
| ) | ||
|
|
||
|
|
||
| class TerminatedBySignal(KeyboardInterrupt): |
There was a problem hiding this comment.
非特定模型相关的部分不太适合放这个文件，是否单独提出去
| assert "still alive after SIGKILL" in capsys.readouterr().out | ||
|
|
||
|
|
||
| def test_stop_process_group_honours_term_grace(monkeypatch) -> None: |
2 participants
Background
The DeepSeek V4 accuracy guard left ~800 GiB of orphaned engines behind whenever the CI runner interrupted it. A bare SIGTERM takes CPython down at SIG_DFL without unwinding the stack, so the
finallythat reaps the server (started in its own session viastart_new_session) never ran. The server survives the signal, is inherited by init, and keeps holding its NPU cards and shared memory.Test hardening
_raise_on_termination(): trap SIGTERM/SIGINT/SIGHUP and re-raise asTerminatedBySignal(aKeyboardInterruptsubclass) sofinallyteardown runs; deafen the signals first (SIG_IGN) so the runner's escalation cannot abort cleanup mid-flight._set_pdeathsig()preexec hook:PR_SET_PDEATHSIGbackstops the one signal we cannot catch (SIGKILL of pytest) and closes the fork/prctl race._stop_process_group(term_grace): escalate to SIGKILL within ~1s on the interrupted path (the runner SIGKILLs us ~3s after SIGTERM) and stay patient (20s) on the normal path.CI workflow
task-submit --list(truncates the command column to 77 chars, so the marker grep can silently miss and leave tasks holding devices) to--find, which matches the untruncated command and prints bare task-ids; also report how many tasks were killed.task-submit --timeout1200 -> 3600 to tolerate longer device acquisition.Testing
pytest tests/test_deepseek_v4_accuracy.py -k "not matches_expected_text"-> 7 passed.Known non-blocking notes (from review)
process.wait(timeout=10)after SIGKILL in_stop_process_groupis hardcoded and not bounded byterm_grace; on the interrupted path a direct child in D state could exceed the ~3s window (the group SIGKILL has already been delivered, so no orphan results)._set_pdeathsigcallsctypes.CDLL("libc.so.6")inside the preexec_fn (dlopen-after-fork); could be moved to import time.task-submit --findexits 0 when there is no match — worth confirming so a clean run is not reported as a failure.