fix(gateway): remove stale foreign pid file during cleanup - #14153
fix(gateway): remove stale foreign pid file during cleanup#14153Pytonballoon810 wants to merge 1 commit into
Conversation
Stale PID cleanup in get_running_pid() incorrectly called remove_pid_file() for the default gateway PID path. remove_pid_file() intentionally preserves PID files owned by other processes, so stale foreign records could survive indefinitely and cause repeated startup failures with: 'PID file race lost to another gateway instance'.\n\nFix by unlinking invalid/stale PID paths directly in _cleanup_invalid_pid_path().\n\nAdd regression test covering stale foreign PID removal.
There was a problem hiding this comment.
Pull request overview
This PR fixes gateway startup/restart deadlocks caused by stale PID files by ensuring invalid/stale PID records are unlinked directly (rather than delegating to remove_pid_file(), which intentionally preserves PID files owned by other processes during graceful handoffs).
Changes:
- Update stale/invalid PID cleanup to always
unlink()the PID file instead of callingremove_pid_file()for the default PID path. - Add a regression test asserting that a stale PID file containing a dead “foreign” PID is removed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
gateway/status.py |
Changes stale/invalid PID cleanup to unconditionally unlink the PID file. |
tests/gateway/test_status.py |
Adds regression coverage for removing a stale PID file referencing a dead non-current PID. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 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) |
There was a problem hiding this comment.
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).
|
Likely duplicate of #13709. |
|
Thanks for the contribution, @Pytonballoon810! This is a well-diagnosed bug report with a correct fix. However, the identical change has already landed on
As @alt-glitch noted, this is also related to the cluster of issues #13709, #13947, #14002, and #13713 — all fixed by the same upstream change. This is an automated hermes-sweeper review. |
Summary
~/.hermes/gateway.pidby unlinking invalid/stale PID files directly.remove_pid_file()in the stale-cleanup path, because that helper intentionally preserves PID files owned by other processes.Root cause
get_running_pid()delegates invalid/stale PID cleanup to_cleanup_invalid_pid_path(). For the default PID path, cleanup previously calledremove_pid_file(), which refuses to delete files not owned by the current process. A stale foreign PID record could therefore persist and repeatedly block startup with:PID file race lost to another gateway instance.Validation
python -m pytest tests/gateway/test_status.py -q28 passedFiles changed
gateway/status.pytests/gateway/test_status.pyImpact
Prevents restart/startup flap loops caused by stale foreign PID records after unexpected shutdowns or interrupted replacements.