Fix/shim sigterm exit window - #29
Merged
Merged
Conversation
The parent and the child arm a timer off the same req.Deadline, so when it fires the executor SIGTERMs the process group at the same instant the child's own context deadline cancels the handler. A cooperative handler returns ctx.Err(), the shim writes its Result frame and settles on exit 0, and then mainExitCode's deferred signal.Stop puts SIGTERM back on its default disposition while the process is still alive and still a target. A signal landing in that gap kills a child that had already reported, and the parent reads back Signal 15 for a handler that shut down cleanly. Ignoring SIGTERM for the rest of the exit path closes it. The defer is registered before the Stop so that it runs after it, and that ordering is the whole point: get it backwards and Stop has the last word and nothing changes. Only the real child does this. An in-process caller is not about to exit and would carry an ignored SIGTERM for the life of the test binary, so Main passes true and the tests pass false. TestKillLadderClassifiesACooperativeTimeoutCorrectly failed 8 times in 120 runs under -race with the CPU loaded, and 0 in 120 after. Putting the same call in Main rather than mainExitCode only got it to 2 in 120, because the remaining defers and the return are themselves wide enough to lose the race.
Nine of them, all paths gosec cannot see are already contained. The six G703 hits in localfs.go are removes and renames of temp files that os.CreateTemp made inside a directory resolve had already confined to fs.root, and resolve does the real work: it rejects an absolute key, rejects one that cleans to the root itself, and Rel-checks the joined path so nothing climbs out. The seventh is a WalkDir over req.OutputDir, which the parent picks and hands across the request fd, never something the handler supplies. The two G115 hits convert a descriptor to uintptr, and fdFromEnv already falls back to its default on a negative, with both defaults positive. Annotated at each site with its reason instead of excluded wholesale in .golangci.yml, matching the #nosec G304 already sitting on LocalFS.Open. Suppressing the first batch surfaced more that the taint analysis had been hiding behind them, so this took three passes to reach a clean run.
TestKillLadderKillsTheWholeProcessGroup checked the grandchild pid once, the instant Run returned, and failed on ubuntu CI with the grandchild "still alive". It was not alive. terminate sends the group its SIGKILL and returns without waiting for anything to actually die, so Run can come back while the grandchild is a zombie: killed, but not yet reaped. syscall.Kill reports success rather than ESRCH for that state, because the process table entry is still sitting there. The grandchild is forked by the fixture rather than by the test, so once the fixture dies it gets reparented and reaped on whatever schedule the subreaper feels like, which is nothing this test synchronises with. Polling for up to two seconds is the same treatment TestKillLadderReapsAHelperAfterACooperativeLeaderExits already got, in this file, for the identical flake. longSleep is five minutes, so the bound is nowhere near it and a grandchild that genuinely survived the group kill still fails this exactly as before. Only reproduces on Linux. 200 runs of the old assertion under load on darwin came back clean on both this branch and the commit it forks from, which is also why the first CI run caught it and local runs did not.
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.
No description provided.