fix: request SYNCHRONIZE access in the Windows PID liveness check#597
Open
SHudici wants to merge 1 commit into
Open
fix: request SYNCHRONIZE access in the Windows PID liveness check#597SHudici wants to merge 1 commit into
SHudici wants to merge 1 commit into
Conversation
_pid_alive_windows opened the process with PROCESS_QUERY_LIMITED_INFORMATION only. That access right cannot be waited on, so WaitForSingleObject always returned WAIT_FAILED (ERROR_ACCESS_DENIED) — for live and dead processes alike — and the code treated any non-signaled result as "alive". Dead PIDs therefore read as alive: stale daemons were never detected and `daemon start` refused to run after a crash left a PID file behind. Three changes: - Open with PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE so the handle is waitable. - Treat WAIT_FAILED explicitly: log and presume alive (fail-safe, consistent with the ACCESS_DENIED branch) instead of conflating it with "still running". - Declare argtypes/restype for OpenProcess, WaitForSingleObject, and CloseHandle. ctypes otherwise defaults everything to c_int, which truncates 64-bit HANDLEs and returns WAIT_FAILED as -1 — never equal to the unsigned 0xFFFFFFFF constant, so the explicit WAIT_FAILED handling could never fire against the real API. The kernel32 interface stays injectable, so the tests drive the handle/wait outcomes portably; the prototypes are applied where the real DLL is constructed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
_pid_alive_windowsopens the process withPROCESS_QUERY_LIMITED_INFORMATIONonly, then callsWaitForSingleObjecton the handle. That access right does not includeSYNCHRONIZE, soWaitForSingleObjectalways returnsWAIT_FAILED(last error 5, access denied) — for live and dead processes alike. The code treated any non-WAIT_OBJECT_0result as "alive", so dead PIDs read as alive: stale daemons were never detected, andcrg daemon startrefused to start after a crash left a PID file behind.Verified with a direct Win32 probe: waiting on a handle opened without
SYNCHRONIZEfails with error 5; addingSYNCHRONIZEmakes the wait report dead processes correctly.Fix
PROCESS_QUERY_LIMITED_INFORMATION | SYNCHRONIZE.WAIT_FAILEDexplicitly: log and presume alive (fail-safe — never kill or replace a daemon we cannot inspect), instead of silently conflating it with "still running".argtypes/restype(viactypes.wintypes) forOpenProcess,WaitForSingleObject, andCloseHandle. Without them ctypes defaults every return to Cint: 64-bitHANDLEs can truncate, andWaitForSingleObject'sWAIT_FAILED(0xFFFFFFFF) comes back as-1, which never compares equal to the unsigned constant — so the explicitWAIT_FAILEDbranch above could never fire against the real API.Testing
tests/test_daemon.py: the fake kernel32 now asserts the exact access mask, and a new test covers theWAIT_FAILED→ presumed-alive path with handle cleanup. Prototypes are declared only where the real DLL is constructed, so the injected-fake seam the tests use is unchanged.test_pid_alive_for_dead_pid/test_is_daemon_running_alivepass against the real API. POSIX path untouched.One of a small series of PRs making the project fully usable on Windows contributor machines.
🤖 Generated with Claude Code