fix(run): forward signals to child and propagate exit status#812
Merged
Conversation
Contributor
|
The changes in this PR will be included in the next version bump.
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
varlock-website | 2aa3ffc | Commit Preview URL Branch Preview URL |
Jun 22 2026, 07:25 PM |
commit: |
… child Review follow-ups: - don't impose a default SIGKILL deadline (SIGHUP-reload / ignored signals no longer kill the child); escalation is opt-in via _VARLOCK_FORCE_KILL_TIMEOUT_MS - skip forwarding once the child has exited, so a clean run never signals a reaped (possibly recycled) pid/process group
- register SIGTERM/SIGINT/SIGHUP/SIGQUIT handlers before spawning the child. Previously they were installed after spawn, so the child could inherit an 'ignored' SIGINT disposition (Node special-cases SIGINT) and never react to a forwarded signal — a real, timing-dependent hang surfaced under load. - gate process-group detach (setsid) on no std stream being a TTY, rather than just stdin. This keeps the daemon's tty-based session scoping of any nested varlock unchanged whenever a terminal is present, and also avoids losing the child's controlling terminal (/dev/tty, SIGWINCH) in piped-stdin terminals.
… std-stream TTYs A process can keep its controlling terminal while its std fds are pipes (e.g. turbo running interactively but piping a task's output). The daemon scopes unlock sessions on the controlling terminal (e_tdev), so detaching such a child would sever tty-based scoping of any nested varlock. Probe /dev/tty (canonical POSIX controlling-terminal check) in addition to isTTY, and only setsid when there is genuinely no terminal.
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.
What
varlock runstays resident (it pipes the child's stdout through redaction whenever output is piped/redirected — interactive terminals get raw pass-through), but it never forwarded terminating signals to the child and lost the child's signal-death status. As a containerENTRYPOINT/ PID 1 this meansdocker stop/ pod termination never reaches the app gracefully — the orchestrator waits out the grace period thenSIGKILLs.Changes
SIGTERM,SIGINT,SIGHUP,SIGQUIT) to the child instead ofSIGKILL-ing it, so it can run its own shutdown handlers. Handlers are installed before the child is spawned, so the child inherits a clean default disposition and reliably reacts to a forwarded signal (installing them after spawn let the child inherit an ignoredSIGINT, causing an intermittent hang).setsid) and signals go to the whole group, so grandchildren terminate too. When a terminal is present we stay in the shared group — preserving the child's controlling terminal (/dev/tty,SIGWINCH) and the daemon's tty-based session scoping of any nestedvarlock. "Terminal present" is detected via the std streams and a/dev/ttyprobe, so it stays correct even when a runner (e.g. turbo) keeps a controlling terminal but pipes a task's std fds.tini/dumb-init, varlock does not impose its own kill deadline (a forwarded signal isn't always terminal; e.g.SIGHUPoften means reload). Escalation toSIGKILLis opt-in via_VARLOCK_FORCE_KILL_TIMEOUT_MS.128+N(e.g.143for SIGTERM) instead of1.SIGKILLat a reaped (possibly recycled) pid/process group.Existing redaction, env injection, and normal-exit-code behavior are unchanged. Adds smoke tests covering signal forwarding,
128+Npropagation, the no-force-kill default, and opt-in escalation, plus a docs note.Daemon session-scoping impact
None. The unlock-session/peer-identity scoping keys off the
varlockprocess that connects to the enclave and its ancestry/env — all upstream of the spawned child.setsidis only applied when there's no controlling terminal (an env-anchored / process-tree context thatsetsiddoesn't perturb), and never when a terminal is present (so tty-based scoping of nestedvarlock— including turbo per-task PTYs and thevarlock → claude → varlockcase — is untouched).setsidchanges neither env vars nor parent PIDs. Verified empirically: a child is only made a session leader when no terminal exists; with a per-task PTY or an interactive terminal it stays in the shared group.Known tradeoff
In interactive (shared-group) mode the terminal delivers Ctrl-C to the child and varlock forwards it, so the child may see
SIGINTtwice. Still strictly better than the old immediateSIGKILL, and forwarding is required so a directkill -TERM <varlock-pid>isn't lost.Out of scope
waitpid(-1)API in pure Node).execve-replacing the child when redaction is off (interactive TTY, no stdout piping needed) for perfect signal transparency — a separate follow-up.