Repository navigation
fix: reap completed background jobs before reading status - #22
Merged
Merged
Conversation
takeokunn
force-pushed
the
fix/background-wait-race
branch
from
September 29, 2026 15:15
751edd1 to
d2ccf76
Compare
takeokunn
force-pushed
the
fix/background-wait-race
branch
from
September 29, 2026 15:44
d2ccf76 to
5d11502
Compare
Collaborator
Author
|
Verification update for head
|
Collaborator
Author
|
Merged with squash commit \ on main. |
Collaborator
Author
|
Merged with squash commit 396d5b1 on main. |
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.
Cause
The custom SIGCHLD handler was introduced in commit 56a98ac (
fix: tokenizer, env vars, && ||, pipeline) together with nshell's ownwaitpid(..., WUNTRACED)job-control path. The design intentionally set a flag in the handler and reaped outside it, so stopped and continued job state would be processed synchronously by nshell.Later refactoring added background
sb-ext:processobjects andsb-ext:process-waitwhile retaining the replacement handler. SBCL's default handler callssb-impl::get-processes-status-changesto update process-object status and cookies, but nshell's handler only set*children-changed*. Consequently,wait %1could remain blocked after the child exited becauseprocess-waitdid not observe the status transition.Fix and compatibility with the original handler design
The handler now calls SBCL's process-status refresh before recording nshell's notification flag. This does not replace nshell's job-control event handling: SBCL refreshes children represented by SBCL process objects, while nshell's foreground pipeline fallback continues to use its own process-group
waitpidpath. The registry-basedfgpath uses process objects, so the two paths do not collect the same child twice. The background reaper also explicitly waits for each completed process object before reading its exit status.SBCL source evidence
target-signal.lisp:183: the default SIGCHLD handler callsget-processes-status-changes.run-program.lisp:267-285:process-waitwaits for process status/cookie changes.run-program.lisp:354-399: status collection is limited to registered SBCL process objects and avoidsWAIT3because directwaitpidusers could race with it.builtin-jobs.lisp:110-134->manage-job-wait.lisp:86-136->process-wait.git log -S'shell-sigchld-handler'identifies commit 56a98ac as the introduction point.Before/after reproduction
Using separately built binaries, CPU load from four
yes >/dev/nullworkers, and an outer 2-second watchdog per iteration:f0d1f3c(before)d2ccf76(after)The command was
nshell -c 'false & wait %1'. No iteration hung, so there was no process to sample. This stress run did not reproduce the race; the source-level SBCL evidence and the regression tests remain the primary evidence for the fix.Timeout investigation
The
cl-host-kit v0.3.1timeout implementation does enforce its timeout: it polls a monotonic deadline, terminates the process group with SIGTERM/SIGKILL, reaps the direct child, and bounds stdout/stderr reader cleanup. Therefore changing the e2e helper from 120 to 60 seconds was not a root fix and was removed. The observed 1800-second gate stall was inside the nshell subprocess execution path; the SIGCHLD/process-object fix addresses that path. The exact reason the observed wall-clock stall exceeded the helper's nominal 120-second deadline is not fully isolated, so this PR does not claim that the host-kit timeout itself was broken.Verification
nix build .#checks.aarch64-darwin.default -L: 2282 passed, 34 skipped, 0 failed, 0 errored, exit 0.e2e-main-interactive-pty-job-control-lifecyclewas skipped because PTY is unavailable in the sandbox; it was not counted as passed.