fix(e2e): clean up NPU async FFN process trees - #279
Conversation
jiangkuaixue123
left a comment
jiangkuaixue123
left a comment
There was a problem hiding this comment.
The run/role tagging and bounded procfs cleanup are well scoped, and the related 148 unit tests pass locally. I found one process-identity race that should be addressed before merge.
| ): | ||
| for pid in matching_pids: | ||
| try: | ||
| os.kill(pid, signal.SIGKILL) |
There was a problem hiding this comment.
[P2] Please avoid signaling a recycled PID. find_processes_matching_environment() closes the procfs files and returns bare numeric PIDs; while this loop is running, a matched worker can exit and its PID can be reused before os.kill(). In that case teardown can SIGKILL an unrelated process that never carried these markers—the preceding group SIGKILL makes this race especially relevant. Please keep a stable process identity across matching and signaling (for example, open a pidfd before reading /proc/<pid>/environ and signal that pidfd, or verify the process start time immediately before signaling), and add a test covering the exit/reuse window.
There was a problem hiding this comment.
Fixed in 45c4ccf: the scanner opens a pidfd before reading environ and SIGKILL is sent through that stable handle. Added a regression test for exit and immediate numeric-PID reuse.
NPU async FFN workers can outlive or leave their launcher process group while a pending CAM receive blocks teardown. Tag each managed process tree with a unique run id and role, then find and SIGKILL all matching FFN processes through procfs after normal group cleanup. Delegate only the FFN group liveness check to one bounded 120-second procfs cleanup phase, and extend the outer cancellation window to cover nested cleanup. Keep focused coverage for exact environment matching and role isolation. Fixes vllm-project#251 Signed-off-by: ShwStone <HaowenShi@outlook.com>
fd3bac6 to
45c4ccf
Compare
2 participants
Summary
Testing
Fixes #251