-
Notifications
You must be signed in to change notification settings - Fork 52.2k
fix(tui): reap orphaned slash_worker processes on gateway startup (Fixes #48643) #48656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Nigmat-future
wants to merge
3
commits into
NousResearch:main
Choose a base branch
from
Nigmat-future:fix/slash-worker-orphan-reap-48643
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
a28d39f
fix(tui): reap orphaned slash_worker processes on gateway startup
Nigmat-future 75dbba7
fix(tui): call Process.ppid() for psutil 7.x compatibility
Nigmat-future 7c980a7
fix(tui): break slash_worker out of parent job for kill-on-close binding
Nigmat-future File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,199 @@ | ||
| """Tests for slash_worker orphan reaping and lifecycle helpers.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import psutil | ||
| import pytest | ||
|
|
||
| from tui_gateway import slash_worker_lifecycle as lifecycle | ||
|
|
||
|
|
||
| def test_is_slash_worker_cmdline(): | ||
| assert lifecycle.is_slash_worker_cmdline( | ||
| ["/usr/bin/python", "-m", "tui_gateway.slash_worker", "--session-key", "k"] | ||
| ) | ||
| assert not lifecycle.is_slash_worker_cmdline(["python", "-m", "pytest"]) | ||
|
|
||
|
|
||
| def test_has_live_gateway_owner_true_for_direct_child(monkeypatch): | ||
| my_pid = 4242 | ||
|
|
||
| class FakeParent: | ||
| ppid = my_pid | ||
|
|
||
| def cmdline(self): | ||
| return [] | ||
|
|
||
| monkeypatch.setattr(lifecycle.psutil, "Process", lambda pid: FakeParent()) | ||
| assert lifecycle.has_live_gateway_owner(9001, my_pid=my_pid) is True | ||
|
|
||
|
|
||
| def test_has_live_gateway_owner_true_for_gateway_ancestor(monkeypatch): | ||
| gateway_pid = 5000 | ||
| worker_pid = 9001 | ||
|
|
||
| class GatewayProc: | ||
| ppid = 1 | ||
|
|
||
| def cmdline(self): | ||
| return ["python", "-m", "tui_gateway.entry"] | ||
|
|
||
| def status(self): | ||
| return psutil.STATUS_RUNNING | ||
|
|
||
| class WorkerParent: | ||
| ppid = gateway_pid | ||
|
|
||
| def cmdline(self): | ||
| return ["conhost.exe"] | ||
|
|
||
| def status(self): | ||
| return psutil.STATUS_RUNNING | ||
|
|
||
| def fake_process(pid): | ||
| if pid == worker_pid: | ||
| return WorkerParent() | ||
| if pid == gateway_pid: | ||
| return GatewayProc() | ||
| raise psutil.NoSuchProcess(pid) | ||
|
|
||
| monkeypatch.setattr(lifecycle.psutil, "Process", fake_process) | ||
| assert lifecycle.has_live_gateway_owner(worker_pid, my_pid=4242) is True | ||
|
|
||
|
|
||
| def test_has_live_gateway_owner_false_when_chain_breaks(monkeypatch): | ||
| worker_pid = 9001 | ||
|
|
||
| class WorkerParent: | ||
| ppid = 7777 | ||
|
|
||
| def cmdline(self): | ||
| return ["bash"] | ||
|
|
||
| def status(self): | ||
| return psutil.STATUS_RUNNING | ||
|
|
||
| def fake_process(pid): | ||
| if pid == worker_pid: | ||
| return WorkerParent() | ||
| raise psutil.NoSuchProcess(pid) | ||
|
|
||
| monkeypatch.setattr(lifecycle.psutil, "Process", fake_process) | ||
| assert lifecycle.has_live_gateway_owner(worker_pid, my_pid=4242) is False | ||
|
|
||
|
|
||
| def test_has_live_gateway_owner_supports_psutil7_ppid_method(monkeypatch): | ||
| my_pid = 4242 | ||
| worker_pid = 9001 | ||
| gateway_pid = 5000 | ||
|
|
||
| class GatewayProc: | ||
| def ppid(self): | ||
| return 1 | ||
|
|
||
| def cmdline(self): | ||
| return ["python", "-m", "tui_gateway.entry"] | ||
|
|
||
| def status(self): | ||
| return psutil.STATUS_RUNNING | ||
|
|
||
| class WorkerParent: | ||
| def ppid(self): | ||
| return gateway_pid | ||
|
|
||
| def cmdline(self): | ||
| return ["conhost.exe"] | ||
|
|
||
| def status(self): | ||
| return psutil.STATUS_RUNNING | ||
|
|
||
| def fake_process(pid): | ||
| if pid == worker_pid: | ||
| return WorkerParent() | ||
| if pid == gateway_pid: | ||
| return GatewayProc() | ||
| raise psutil.NoSuchProcess(pid) | ||
|
|
||
| monkeypatch.setattr(lifecycle.psutil, "Process", fake_process) | ||
| assert lifecycle.has_live_gateway_owner(worker_pid, my_pid=my_pid) is True | ||
|
|
||
|
|
||
| def test_reap_orphan_slash_workers_terminates_unowned(monkeypatch): | ||
| terminated: list[int] = [] | ||
|
|
||
| class FakeProcInfo: | ||
| def __init__(self, pid, cmdline): | ||
| self.info = {"pid": pid, "cmdline": cmdline} | ||
|
|
||
| rows = [ | ||
| FakeProcInfo(100, ["python", "-m", "tui_gateway.slash_worker", "--session-key", "k1"]), | ||
| FakeProcInfo(101, ["python", "-m", "pytest"]), | ||
| ] | ||
|
|
||
| monkeypatch.setattr( | ||
| lifecycle.psutil, | ||
| "process_iter", | ||
| lambda attrs: iter(rows), | ||
| ) | ||
| monkeypatch.setattr( | ||
| lifecycle, | ||
| "has_live_gateway_owner", | ||
| lambda pid, my_pid: False, | ||
| ) | ||
| monkeypatch.setattr( | ||
| lifecycle, | ||
| "_terminate_pid", | ||
| lambda pid: terminated.append(pid), | ||
| ) | ||
|
|
||
| count = lifecycle.reap_orphan_slash_workers(my_pid=4242) | ||
| assert count == 1 | ||
| assert terminated == [100] | ||
|
|
||
|
|
||
| def test_reap_orphan_slash_workers_skips_owned_workers(monkeypatch): | ||
| terminated: list[int] = [] | ||
|
|
||
| class FakeProcInfo: | ||
| def __init__(self, pid, cmdline): | ||
| self.info = {"pid": pid, "cmdline": cmdline} | ||
|
|
||
| rows = [ | ||
| FakeProcInfo(100, ["python", "-m", "tui_gateway.slash_worker", "--session-key", "k1"]), | ||
| ] | ||
|
|
||
| monkeypatch.setattr( | ||
| lifecycle.psutil, | ||
| "process_iter", | ||
| lambda attrs: iter(rows), | ||
| ) | ||
| monkeypatch.setattr( | ||
| lifecycle, | ||
| "has_live_gateway_owner", | ||
| lambda pid, my_pid: True, | ||
| ) | ||
| monkeypatch.setattr( | ||
| lifecycle, | ||
| "_terminate_pid", | ||
| lambda pid: terminated.append(pid), | ||
| ) | ||
|
|
||
| count = lifecycle.reap_orphan_slash_workers(my_pid=4242) | ||
| assert count == 0 | ||
| assert terminated == [] | ||
|
|
||
|
|
||
| def test_maybe_reap_orphan_slash_workers_on_startup_runs_once(monkeypatch): | ||
| lifecycle._reaper_ran = False | ||
| calls: list[int] = [] | ||
|
|
||
| monkeypatch.setattr( | ||
| lifecycle, | ||
| "reap_orphan_slash_workers", | ||
| lambda **kwargs: calls.append(1) or 2, | ||
| ) | ||
|
|
||
| lifecycle.maybe_reap_orphan_slash_workers_on_startup() | ||
| lifecycle.maybe_reap_orphan_slash_workers_on_startup() | ||
|
|
||
| assert calls == [1] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| """Windows-only end-to-end checks for slash_worker lifecycle helpers.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import subprocess | ||
| import sys | ||
| import time | ||
|
|
||
| import psutil | ||
| import pytest | ||
|
|
||
| pytestmark = pytest.mark.skipif(sys.platform != "win32", reason="Windows job-object e2e") | ||
|
|
||
|
|
||
| def test_kill_on_close_job_terminates_child_when_spawner_exits(): | ||
| script = """ | ||
| import subprocess, sys | ||
| from tui_gateway.slash_worker_lifecycle import attach_slash_worker_kill_job | ||
|
|
||
| BREAKAWAY = 0x01000000 | ||
| proc = subprocess.Popen( | ||
| [sys.executable, "-c", "import time; time.sleep(120)"], | ||
| creationflags=BREAKAWAY, | ||
| ) | ||
| job = attach_slash_worker_kill_job(proc) | ||
| if job is None: | ||
| raise SystemExit("job attach failed") | ||
| # Keep job handle alive until this process exits. | ||
| _KEEP_JOB = job | ||
| print(proc.pid, flush=True) | ||
| """ | ||
| spawner = subprocess.Popen( | ||
| [sys.executable, "-c", script], | ||
| stdout=subprocess.PIPE, | ||
| text=True, | ||
| ) | ||
| out, _ = spawner.communicate(timeout=15) | ||
| worker_pid = int(out.strip()) | ||
| assert spawner.returncode == 0 | ||
| time.sleep(2) | ||
| assert not psutil.pid_exists(worker_pid), "child should die when job handle closes" | ||
|
|
||
|
|
||
| def test_reaper_kills_orphan_slash_worker_after_launcher_exits(): | ||
| launcher = """ | ||
| import subprocess, sys, time | ||
| BREAKAWAY = 0x01000000 | ||
| proc = subprocess.Popen( | ||
| [sys.executable, "-m", "tui_gateway.slash_worker", "--session-key", "e2e-reap"], | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| creationflags=BREAKAWAY, | ||
| ) | ||
| print(proc.pid, flush=True) | ||
| time.sleep(0.5) | ||
| """ | ||
| launcher_proc = subprocess.Popen( | ||
| [sys.executable, "-c", launcher], | ||
| stdout=subprocess.PIPE, | ||
| text=True, | ||
| ) | ||
| out, _ = launcher_proc.communicate(timeout=20) | ||
| worker_pid = int(out.strip()) | ||
| # Give watchdog a moment; if still alive, reaper must kill it. | ||
| time.sleep(1) | ||
| if not psutil.pid_exists(worker_pid): | ||
| pytest.skip("watchdog already reaped worker before reaper ran") | ||
|
|
||
| from tui_gateway.slash_worker_lifecycle import reap_orphan_slash_workers | ||
|
|
||
| reaped = reap_orphan_slash_workers(my_pid=__import__("os").getpid()) | ||
| time.sleep(1) | ||
| try: | ||
| assert reaped >= 1 | ||
| assert not psutil.pid_exists(worker_pid) | ||
| finally: | ||
| if psutil.pid_exists(worker_pid): | ||
| psutil.Process(worker_pid).kill() |
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current main already passes
creationflags=windows_hide_flags()in this samePopencall (tui_gateway/server.py:323). Salvage this by composing the breakaway bit with that helper; adding a secondcreationflagskwarg conflicts, while replacing it loses the existing no-console-window behavior.