Skip to content

fix: reap completed background jobs before reading status - #22

Merged
takeokunn merged 1 commit into
mainfrom
fix/background-wait-race
Sep 29, 2026
Merged

takeokunn merged 1 commit into
mainfrom
fix/background-wait-race

Conversation

@takeokunn

@takeokunn takeokunn commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Cause

The custom SIGCHLD handler was introduced in commit 56a98ac (fix: tokenizer, env vars, && ||, pipeline) together with nshell's own waitpid(..., 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:process objects and sb-ext:process-wait while retaining the replacement handler. SBCL's default handler calls sb-impl::get-processes-status-changes to update process-object status and cookies, but nshell's handler only set *children-changed*. Consequently, wait %1 could remain blocked after the child exited because process-wait did 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 waitpid path. The registry-based fg path 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

  • SBCL 2.6.7 target-signal.lisp:183: the default SIGCHLD handler calls get-processes-status-changes.
  • SBCL 2.6.7 run-program.lisp:267-285: process-wait waits for process status/cookie changes.
  • SBCL 2.6.7 run-program.lisp:354-399: status collection is limited to registered SBCL process objects and avoids WAIT3 because direct waitpid users could race with it.
  • nshell's wait path is 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/null workers, and an outer 2-second watchdog per iteration:

Binary Iterations Exit code 1 Timeout Other
f0d1f3c (before) 300 300 0 0
d2ccf76 (after) 300 300 0 0

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.1 timeout 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-lifecycle was skipped because PTY is unavailable in the sandbox; it was not counted as passed.
  • The four CI jobs for the rebased head have now succeeded in run 36592497820; the non-sandboxed integration suite also passed e2e-main-interactive-pty-job-control-lifecycle in 4.305s.

@takeokunn
takeokunn force-pushed the fix/background-wait-race branch from 751edd1 to d2ccf76 Compare September 29, 2026 15:15
@takeokunn
takeokunn force-pushed the fix/background-wait-race branch from d2ccf76 to 5d11502 Compare September 29, 2026 15:44
@takeokunn

Copy link
Copy Markdown
Collaborator Author

Verification update for head 5d11502:

  • Before/after stress comparison: f0d1f3c and d2ccf76, each N=300 under CPU load, running nshell -c 'false & wait %1' with a 2-second outer watchdog. Both results were exit code 1: 300, timeout: 0, other: 0.
  • Local formal gate: 2282 passed, 34 skipped, 0 failed, 0 errored (exit 0). The PTY lifecycle case was skipped locally because PTY is unavailable in the sandbox.
  • CI run 36592497820: all four jobs succeeded.
  • The non-sandboxed CI integration suite passed the PTY job-control e2e: e2e-main-interactive-pty-job-control-lifecycle passed in 4.305s. The suite reported 2320 passed, 1 skipped, 0 todo, 0 failed, 0 errored.

@takeokunn
takeokunn merged commit 396d5b1 into main Sep 29, 2026
4 checks passed
@takeokunn
takeokunn deleted the fix/background-wait-race branch September 29, 2026 16:18
@takeokunn

Copy link
Copy Markdown
Collaborator Author

Merged with squash commit \ on main.

@takeokunn

Copy link
Copy Markdown
Collaborator Author

Merged with squash commit 396d5b1 on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant