Repository navigation
XERK-2580: Stop the one-shot kill from signalling every process - #1372
Merged
Merged
Conversation
_kill_oneshot_proc (from XERK-2545, #1339) still killpg'd proc.pid directly. A mocked Popen's pid coerces to 1, and killpg(1) is kill(-1): the exact call that took TrueNAS down. Route it through _signalable_pid like its siblings.
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.
Summary: The agent's one-shot cleanup can no longer kill every process its user owns when it is handed a fake or invalid process id. This closes the last copy of the bug that repeatedly took the TrueNAS agent down.
Part of XERK-2580
Why
What changed
SessionManager._kill_oneshot_procpasses the pid through_signalable_pid. If the pid isn't a real one, it falls back toproc.kill(), as the sibling kill sites already do.test_a_mocked_oneshot_proc_is_never_killpg_dinTestKillpgNeverHitsPidOne.Risk
Low. Only a pid that isn't a positive integer above 1 behaves differently. A real child is still group-killed. To roll back, revert this commit.
Testing
QA result: PASS. QA confirmed a bogus pid (MagicMock, 1, 0, -1, True, None) never reaches killpg. A live one-shot group (child plus grandchildren) went from 4 processes to 0 on shutdown. Four deliberate breakages of the guard were each caught by the new test.
Not tested: Windows (that branch is unchanged) and a full end-to-end
_handle_shutdownof a running manager.Test evidence
pytest tests/, 4451 passed, 26 skipped. QA reran it under a kill guard:unittest discover, 4477 OK.Follow-ups
test_a_shutdown_landing_inside_popen_still_kills_that_childchecks only the direct child's exit, not the process group (Low, pre-existing). Rolled into XERK-2584.