fix: kill_holder honors safety guards and verifies SIGKILL - #3
Merged
Merged
Conversation
The library API used to skip kill_block_reason and report SIGKILL success even when the process was still alive (D-state / unkillable).
TerminateProcess + WaitForSingleObject timeout used to return success ("still shutting down"). After the grace wait, still-alive PIDs are now a failure, matching the POSIX SIGKILL path.
The still-alive test only patched os.kill, so Windows CI took _windows.kill(missing pid) and reported already gone. Cover both backends without skipping Windows.
hc-ui
marked this pull request as ready for review
August 26, 2026 10:30
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.
Library
kill_holderused to skipkill_block_reason(PID 1 / self / critical / service) and report SIGKILL success even when the process was still alive (D-state / unkillable).This is a different gap from #1 (refuse killing the parent shell) and #2 (hardlink alternate paths).
Changes
kill_block_reasonbefore sending any signal so the library API cannot bypass the same guards as the CLI._pid_alive; still-alive processes are reported as a failure instead of a fake success._windows.killused to return success withtermination requested (still shutting down)whenWaitForSingleObjecttimed out afterTerminateProcess. That is the same fake-success as the old POSIX SIGKILL path. It now re-checks_pid_existsand returns failure if the PID is still alive.Why Windows CI went red
Failed run: https://github.com/hc-ui/wholocks/actions/runs/32955462098
tests/test_core.py::TestKillSafety::test_sigkill_reports_failure_if_still_aliveonly patchedos.kill/_pid_alive. On Windowskill_holdergoes to_windows.kill(44444), which saw a missing PID and returned(True, "already gone"). Ubuntu stayed green because it takes the SIGKILL path.Tests
PYTHONPATH=src python3 -m pytest -q→ 70 passed locally.