diff --git a/code_review_graph/daemon.py b/code_review_graph/daemon.py index cdbdd543..36db7a84 100644 --- a/code_review_graph/daemon.py +++ b/code_review_graph/daemon.py @@ -316,8 +316,10 @@ def clear_pid(path: Path | None = None) -> None: # Win32 constants for the OpenProcess-based liveness check (#511). _PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 +_SYNCHRONIZE = 0x00100000 _ERROR_ACCESS_DENIED = 5 _WAIT_OBJECT_0 = 0x0 +_WAIT_FAILED = 0xFFFFFFFF def _pid_alive_windows( @@ -327,6 +329,11 @@ def _pid_alive_windows( ) -> bool: """Win32 PID liveness check via OpenProcess/WaitForSingleObject. + The access mask must include SYNCHRONIZE: a handle opened with only + PROCESS_QUERY_LIMITED_INFORMATION cannot be waited on, so + WaitForSingleObject returns WAIT_FAILED (ERROR_ACCESS_DENIED) and + every exited process reads as alive. + The kernel32 interface is injected so tests can drive handle/wait outcomes on non-Windows platforms. *get_last_error* defaults to ``kernel32.GetLastError``; the real caller passes @@ -334,14 +341,24 @@ def _pid_alive_windows( """ if get_last_error is None: get_last_error = kernel32.GetLastError - handle = kernel32.OpenProcess(_PROCESS_QUERY_LIMITED_INFORMATION, False, pid) + handle = kernel32.OpenProcess(_PROCESS_QUERY_LIMITED_INFORMATION | _SYNCHRONIZE, False, pid) if not handle: # NULL handle: process is dead, or we lack access. ACCESS_DENIED # means it exists but is owned by another user — treat as alive. return get_last_error() == _ERROR_ACCESS_DENIED try: + result = kernel32.WaitForSingleObject(handle, 0) + if result == _WAIT_FAILED: + # The wait itself errored — we cannot prove the process dead, + # so err alive, consistent with the ACCESS_DENIED branch. + logger.debug( + "WaitForSingleObject on PID %d failed (error %d); presuming alive", + pid, + get_last_error(), + ) + return True # WAIT_OBJECT_0 means the process handle is signaled (it exited). - return kernel32.WaitForSingleObject(handle, 0) != _WAIT_OBJECT_0 + return result != _WAIT_OBJECT_0 finally: kernel32.CloseHandle(handle) @@ -355,8 +372,18 @@ def pid_alive(pid: int) -> bool: """ if sys.platform == "win32": import ctypes + from ctypes import wintypes kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + # Explicit prototypes: ctypes otherwise defaults every argument and + # return value to c_int, which truncates 64-bit HANDLEs and returns + # WAIT_FAILED as -1 — never equal to the unsigned 0xFFFFFFFF constant. + kernel32.OpenProcess.argtypes = (wintypes.DWORD, wintypes.BOOL, wintypes.DWORD) + kernel32.OpenProcess.restype = wintypes.HANDLE + kernel32.WaitForSingleObject.argtypes = (wintypes.HANDLE, wintypes.DWORD) + kernel32.WaitForSingleObject.restype = wintypes.DWORD + kernel32.CloseHandle.argtypes = (wintypes.HANDLE,) + kernel32.CloseHandle.restype = wintypes.BOOL return _pid_alive_windows(pid, kernel32, ctypes.get_last_error) try: os.kill(pid, 0) # signal 0 = existence check diff --git a/tests/test_daemon.py b/tests/test_daemon.py index 61f5139b..0c3a19fb 100644 --- a/tests/test_daemon.py +++ b/tests/test_daemon.py @@ -379,8 +379,8 @@ def test_alive_when_wait_times_out(self): kernel32 = _FakeKernel32(handle=1234, wait_result=0x102) assert _pid_alive_windows(4242, kernel32) is True - # PROCESS_QUERY_LIMITED_INFORMATION, no inherit, the pid - assert kernel32.open_calls == [(0x1000, False, 4242)] + # PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE, no inherit, the pid + assert kernel32.open_calls == [(0x1000 | 0x00100000, False, 4242)] assert kernel32.wait_calls == [(1234, 0)] # The handle must always be closed assert kernel32.closed == [1234] @@ -393,6 +393,19 @@ def test_dead_when_handle_is_signaled(self): assert _pid_alive_windows(4242, kernel32) is False assert kernel32.closed == [1234] + def test_alive_when_wait_fails(self): + """WAIT_FAILED (0xFFFFFFFF) cannot prove death — presume alive. + + Regression guard: with a QUERY-only access mask the wait always + failed, so dead daemons were reported as running forever. + """ + from code_review_graph.daemon import _pid_alive_windows + + kernel32 = _FakeKernel32(handle=1234, wait_result=0xFFFFFFFF) + assert _pid_alive_windows(4242, kernel32) is True + # The handle must still be closed on this path + assert kernel32.closed == [1234] + def test_alive_on_access_denied(self): """NULL handle + ERROR_ACCESS_DENIED (5) means alive (other user).""" from code_review_graph.daemon import _pid_alive_windows