test: the nestedpidnskill control is load-dependent, and the comment said otherwise - #570
Merged
Merged
Conversation
…said otherwise The comment merged with #568 claimed the reverted build leaks "6/6 every single run" across "3 quiet runs and 3 fork-storm-loaded runs each". The loaded half is false. Corrected in place rather than deleted: the shape recurs — a control measured under one set of conditions, written down as though it held under all of them. Re-measured against the same reverted build, which is the flat pre-#101 topology: exec.go execs bwrap directly, no CLONE_NEWPID and no __inpidns. Dropping CLONE_NEWPID alone does not build a working snug — the verb refuses with "this process is pid N, not pid 1" — so a reader reproducing this has to revert both halves. quiet, load ~1 reverted build FAILS 6/6, both assertions loaded, load 8-10 reverted build PASSES 4/4 runs The defect is still reachable under that load: a bash-paced poll of the same predicate, trigger at 0.39s, leaks 5/5 at every extra delay from 0 to 100ms. What moves is where the poll lands relative to the window, not whether the window exists. Offset sweep of the reverted build, quiet host, 5 trials a point: 0.04 0.06 0.08 -> 0/5 0.10 0.12 0.15 -> 5/5 0.20 0.25 -> 0/5 So the window is ~100-150ms quiet and opens LATER under load while the poll — cheap /proc reads — fires at roughly the same place. What the test guarantees is therefore narrower than "it catches the regression", and the comment now says so: it never FAILS falsely, which is what the fixed offset could not manage, and it catches issue #13's residual returning on a host that is not busy. CI is such a host. The comment also says not to answer this with a sleep: no constant lands in this window on every host, which is why the poll replaced one. Comment only, no behaviour change. Test still passes on this host (6.412s, load 0.49); make gate green. Co-Authored-By: Claude Opus 5 (1M context) <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
1 participant
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.
Comment-only correction to what #568 merged.
What was wrong
The comment says the reverted build leaks "6/6 every single run" across "3 quiet runs and 3 fork-storm-loaded runs each". Loaded half is false.
Corrected in place, not deleted — the shape recurs: a control measured under one set of conditions, written down as though it held under all of them.
Re-measurement
Reverted build = flat pre-#101 topology:
exec.goexecs bwrap directly, noCLONE_NEWPID, no__inpidns. DroppingCLONE_NEWPIDalone does NOT build a working snug — the verb refuses withthis process is pid N, not pid 1— so a reader reproducing this reverts both halves. That detail was not in the tree anywhere.Defect still reachable under that load: bash-paced poll of the same predicate, trigger at 0.39s, leaks 5/5 at every extra delay 0-100ms. So what moves is where the poll lands relative to the window, not whether the window exists.
Offset sweep, reverted build, quiet host, 5 trials a point:
Window ~100-150ms quiet, opens LATER under load; the poll (cheap /proc reads) fires at roughly the same place either way.
What the test actually guarantees
Narrower than "catches the regression", and the comment now says so: never fails falsely — what the fixed offset could not manage — and catches issue #13's residual returning on a host that is not busy. CI is such a host; a workstation under a fork storm is not.
Comment also says not to answer this with a sleep: no constant lands in this window on every host, which is why the poll replaced one.
Verification
Comment only, no behaviour change.
TestSIGKILLBeforeThePayloadStartsLeavesNoInitpasses on this host (6.412s, load 0.49).make gategreen.🤖 Generated with Claude Code