fix(agent): a group with nothing left in it is not a failed kill - #85
Merged
Conversation
The sandbox tests failed about once in twenty-five runs on macOS with "kill process group: Operation not permitted (os error 1)", taking whichever test happened to lose the race. It looked like a test problem and was not: the same flake reproduces on main with none of the recent work applied, and it can hit any sandbox test. The sequence is narrow. child_exited_unreaped confirms the child is gone, the pipes are not closed yet, so the loop kills the process group to clear any straggler, and then reports a failure if that kill did not work. By then the group can hold nothing but the unreaped zombie, and macOS answers EPERM where Linux answers ESRCH. ESRCH was already forgiven, on the correct reasoning that a group with nothing in it is the outcome the kill wanted. EPERM is the same fact with a different number, and it was not forgiven, so a command that had already succeeded failed while tidying up something that was no longer there. Both errnos now mean the same thing, behind a named predicate so the reasoning has somewhere to live. Anything else still fails the run. The forgiving direction is the dangerous one here, since it could hide a kill that genuinely did not happen, so the set stays at exactly these two and the test pins that EINVAL is still an error. Twenty-five consecutive runs of the sandbox module are clean where the same loop produced a failure before. That is consistent rather than conclusive at this sample size; the argument that should carry weight is the mechanical one, that the kill is now told the truth about a group it has already outlived. Claude-Session: https://claude.ai/code/session_01KGPVQ8wUG7h36zashWYCp4
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.
The sandbox tests fail about once in twenty-five runs on macOS:
It takes whichever sandbox test happens to lose the race. I have seen it
land on
output_below_cap_is_not_dropped_after_head_fillsand ontruncation_preserves_utf8_boundaries_and_uses_one_marker, and once on bothin the same run.
It is not a test problem and it is not new. It reproduces on
mainwithnone of the recent work applied. CI has been lucky.
The race
Sandbox::run's wait loop,zorp-agent/src/sandbox/mod.rs:child_exited_unreaped(pgid)returns true. The child is already gone.loop calls
kill_process_group(pgid)to clear it.By step 3 the group can hold nothing but the exited-but-unreaped child itself.
macOS answers
EPERMfor a group like that where Linux answersESRCH.kill_process_groupalready forgaveESRCH, on the correct reasoning that agroup with nothing in it is exactly the outcome the kill wanted. It did not
forgive
EPERM, which is the same fact reported with a different number. So acommand that had already succeeded failed while tidying up something that was
no longer there.
The fix
Both errnos now mean "there was nothing left to kill", behind a named
predicate so the reasoning has somewhere to live:
Every member of this group is a process we spawned, so we are always permitted
to signal it while it is alive. The kernel only refuses when there is nothing
signalable there.
The forgiving direction is the dangerous one, since a wider set could hide
a kill that genuinely did not happen. So the set stays at exactly these two,
and the test pins that
EINVALis still a real error.Verification
Twenty-five consecutive runs of
cargo test -p zorp-agent --lib sandbox::areclean, where the identical loop produced a failure before the change.
That is consistent, not conclusive. Twenty-five runs against a roughly 4%
base rate cannot distinguish zero from small. The argument that should carry
weight is the mechanical one: the kill is now told the truth about a group it
has already outlived. The empirical run is corroboration, not proof, and I
would rather say so than present it as one.
cargo test -p zorp-agent --lib475 passed,cargo fmt --all --checkclean,cargo clippy -p zorp-agent --all-targets --locked -- -D warningsclean.How it surfaced
Found while a subagent was rebasing another branch. It reported the failure,
did not assume the failure was its own, checked out pre-merge
main, andreproduced it there twice. That is the reason this is a PR and not a retry.
https://claude.ai/code/session_01KGPVQ8wUG7h36zashWYCp4