Fix the race when reading the emulator's process group - #23
Merged
Conversation
setsid(2) runs asynchronously in the child, so for a brief window after backgrounding the launcher its pid still reports the *shell's* process group. Reading the pgid exactly once loses that race intermittently: the guard then sees EMU_PGID == SELF_PGID, correctly refuses to group-kill our own step, and silently drops to the single-pid fallback -- so the process group scoping added in #21 was not reliably active. Observed on the runner in 1 of 4 boot cycles (#22's run); 0 of 4 in #21's, which is why it was missed. Consequence was mild -- 'adb emu kill' is the primary shutdown path and the fallback still terminates the launcher -- but the scoping is the whole point of that change. Poll until the pgid settles instead, giving up only if the emulator dies or after 10s. The first iteration is identical to the previous single read, so this can only ever do better. Also log the resolved pgid, so a future run shows whether isolation actually happened rather than staying silent. The failure cannot be reproduced on an unloaded dev machine (0 losses in 40 trials); forcing the timing -- a child that lingers in our process group for 300ms before setsid -- the old single read loses 10/10 and the polling version loses 0/10.
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.
The bug
setsid(2)runs asynchronously in the child, so for a brief window after backgrounding the launcher its pid still reports the shell's process group. #21 read the pgid exactly once:When that read lands too early, the guard sees
EMU_PGID == SELF_PGID, correctly refuses to group-kill our own step, and silently falls back to a single-pid kill. So the process-group scoping that #21 was meant to introduce was not reliably active.Observed on the runner in 1 of 4 boot cycles during #22's run, and 0 of 4 during #21's — which is why it slipped through. The consequence was mild (
adb emu killis the primary shutdown path, and the fallback still terminates the launcher), but the scoping is the entire point of that change.The fix
Poll until the pgid settles, giving up only if the emulator dies or after 10s. The first iteration is byte-for-byte what the old code did, so this can only ever do better — it just retries instead of surrendering.
Also logs the resolved pgid on success. Previously a successful isolation printed nothing, so the logs couldn't distinguish "worked" from "never checked" — only the failure was visible.
Verification
The race does not reproduce on an unloaded dev machine: 0 losses in 40 trials. Forcing the timing the runner exhibits — a child that lingers in the parent's process group for 300ms before
setsid— separates the two clearly:Only
emulator-preview-multirunchanges; the other five jobs parse byte-identical tomain.