Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions openviking/utils/process_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import atexit
import os
import subprocess
import sys
import threading

Expand Down Expand Up @@ -78,6 +79,28 @@ def _is_pid_alive(pid: int) -> bool:
except OSError:
# /proc not available or process exited between kill and open
pass
elif sys.platform == "darwin":
try:
proc = subprocess.run(
["ps", "-p", str(pid), "-o", "command="],
capture_output=True,
text=True,
timeout=2,
)
if proc.returncode != 0:
return False
cmdline = (proc.stdout or "").lower()
if "openviking" not in cmdline and "python" not in cmdline:
logger.info(
"PID %d is alive but not an OpenViking process (cmdline: %.100s). "
"Assuming stale lock from recycled PID.",
pid,
cmdline[:100],
)
return False
except (OSError, subprocess.SubprocessError):
# Keep the existing liveness result if process inspection is unavailable.
pass

return True

Expand Down
51 changes: 37 additions & 14 deletions tests/unit/test_process_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import os
from pathlib import Path
from types import SimpleNamespace

import pytest

Expand Down Expand Up @@ -78,9 +79,9 @@ def test_current_pid_is_alive(self):
current_pid = os.getpid()
assert _is_pid_alive(current_pid) is True

def test_pid_1_is_alive(self):
"""Test that PID 1 (init) is typically alive."""
# PID 1 is usually init process on Linux
def test_pid_1_is_alive_without_platform_identity_check(self, monkeypatch):
"""Test that a live PID is detected on platforms without identity checks."""
monkeypatch.setattr(process_lock_module.sys, "platform", "freebsd")
assert _is_pid_alive(1) is True

def test_nonexistent_pid_not_alive(self):
Expand Down Expand Up @@ -119,6 +120,25 @@ def _raise_system_error(_pid: int, _sig: int) -> None:
with pytest.raises(SystemError):
_is_pid_alive(12345)

def test_darwin_recycled_pid_with_unrelated_process_is_stale(self, monkeypatch):
"""macOS should not keep a stale lock when the PID was reused."""

def _pid_exists(_pid: int, _sig: int) -> None:
return None

def _sysctl_process(_args, **_kwargs):
return SimpleNamespace(
returncode=0,
stdout="mdwrite\0/System/Library/Spotlight/mdwrite",
stderr="",
)

monkeypatch.setattr(process_lock_module.sys, "platform", "darwin")
monkeypatch.setattr(process_lock_module.os, "kill", _pid_exists)
monkeypatch.setattr(process_lock_module.subprocess, "run", _sysctl_process)

assert _is_pid_alive(857) is False


class TestAcquireDataDirLock:
"""Test acquire_data_dir_lock function."""
Expand Down Expand Up @@ -155,16 +175,16 @@ def test_acquire_with_stale_lock_succeeds(self, tmp_path: Path):
lock_path = acquire_data_dir_lock(str(tmp_path))
assert lock_path == str(tmp_path / LOCK_FILENAME)

def test_acquire_with_live_process_raises(self, tmp_path: Path):
def test_acquire_with_live_process_raises(self, tmp_path: Path, monkeypatch):
"""Test acquiring lock with live process raises DataDirectoryLocked."""
# Use PID 1 (init) which is typically alive
(tmp_path / LOCK_FILENAME).write_text("1")
(tmp_path / LOCK_FILENAME).write_text("12345")
monkeypatch.setattr(process_lock_module, "_is_pid_alive", lambda _pid: True)

with pytest.raises(DataDirectoryLocked) as exc_info:
acquire_data_dir_lock(str(tmp_path))

assert "Another OpenViking process" in str(exc_info.value)
assert "PID 1" in str(exc_info.value)
assert "PID 12345" in str(exc_info.value)

def test_acquire_creates_directory(self, tmp_path: Path):
"""Test acquiring lock creates directory if it doesn't exist."""
Expand All @@ -188,9 +208,10 @@ def _deny_makedirs(*_args, **_kwargs) -> None:

assert not (workspace / LOCK_FILENAME).exists()

def test_error_message_suggests_http_server(self, tmp_path: Path):
def test_error_message_suggests_http_server(self, tmp_path: Path, monkeypatch):
"""Test error message suggests using one HTTP server."""
(tmp_path / LOCK_FILENAME).write_text("1")
(tmp_path / LOCK_FILENAME).write_text("12345")
monkeypatch.setattr(process_lock_module, "_is_pid_alive", lambda _pid: True)

with pytest.raises(DataDirectoryLocked) as exc_info:
acquire_data_dir_lock(str(tmp_path))
Expand All @@ -199,19 +220,21 @@ def test_error_message_suggests_http_server(self, tmp_path: Path):
assert "OpenViking server" in error_msg
assert "connect clients over HTTP" in error_msg

def test_error_message_shows_pid(self, tmp_path: Path):
def test_error_message_shows_pid(self, tmp_path: Path, monkeypatch):
"""Test error message shows conflicting PID."""
(tmp_path / LOCK_FILENAME).write_text("1")
(tmp_path / LOCK_FILENAME).write_text("12345")
monkeypatch.setattr(process_lock_module, "_is_pid_alive", lambda _pid: True)

with pytest.raises(DataDirectoryLocked) as exc_info:
acquire_data_dir_lock(str(tmp_path))

error_msg = str(exc_info.value)
assert "PID 1" in error_msg
assert "PID 12345" in error_msg

def test_error_message_shows_directory(self, tmp_path: Path):
def test_error_message_shows_directory(self, tmp_path: Path, monkeypatch):
"""Test error message shows directory path."""
(tmp_path / LOCK_FILENAME).write_text("1")
(tmp_path / LOCK_FILENAME).write_text("12345")
monkeypatch.setattr(process_lock_module, "_is_pid_alive", lambda _pid: True)

with pytest.raises(DataDirectoryLocked) as exc_info:
acquire_data_dir_lock(str(tmp_path))
Expand Down