Skip to content

fix(gateway): remove stale foreign pid file during cleanup #14153

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.

Sign up for GitHub

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

Closed
Pytonballoon810 wants to merge 1 commit into from
Closed

fix(gateway): remove stale foreign pid file during cleanup #14153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions gateway/status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,11 @@ def _cleanup_invalid_pid_path(pid_path: Path, *, cleanup_stale: bool) -> None:
if not cleanup_stale:
return
try:
if pid_path == _get_pid_path():
remove_pid_file()
else:
pid_path.unlink(missing_ok=True)
# Invalid/stale PID records must be removed unconditionally.
# Do NOT call remove_pid_file() here: that helper intentionally keeps
# PID files that belong to a different process during graceful
# handoffs, which is the opposite of what stale cleanup needs.
pid_path.unlink(missing_ok=True)
Comment on lines +219 to +223

Copilot AI Apr 22, 2026

Copy link

Choose a reason for hiding this comment

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

get_running_pid() treats PermissionError from os.kill(pid, 0) the same as ProcessLookupError and calls _cleanup_invalid_pid_path(). With this change, that path now unlinks the PID file unconditionally, which can delete a PID file for a process that is actually still running (EPERM means “process exists but you don’t have permission”). That can allow a second gateway instance to start under the same PID path when the original process is owned by another user / has restricted permissions. Consider handling PermissionError separately (e.g., treat it as “running” and return the PID, or at least skip PID-file deletion on PermissionError and leave the file in place).

Copilot uses AI. Check for mistakes.
except Exception:
pass

Expand Down
20 changes: 20 additions & 0 deletions tests/gateway/test_status.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,26 @@ def test_get_running_pid_rejects_live_non_gateway_pid(self, tmp_path, monkeypatc
assert status.get_running_pid() is None
assert not pid_path.exists()

def test_get_running_pid_removes_stale_pid_from_different_process(self, tmp_path, monkeypatch):
"""Regression: stale PID file owned by a dead *different* PID must be removed.

Previously, stale cleanup called remove_pid_file() for the default PID path,
but that helper intentionally refuses to delete files that don't belong to
the current process. Result: a dead foreign PID could leave gateway.pid
behind forever and block startup with a PID-file race.
"""
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
pid_path = tmp_path / "gateway.pid"
pid_path.write_text(json.dumps({"pid": 99999}))

def _dead_pid(pid, sig):
raise ProcessLookupError

monkeypatch.setattr(status.os, "kill", _dead_pid)

assert status.get_running_pid() is None
assert not pid_path.exists()

def test_get_running_pid_accepts_gateway_metadata_when_cmdline_unavailable(self, tmp_path, monkeypatch):
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
pid_path = tmp_path / "gateway.pid"
Expand Down
Loading