Skip to content

fix(runtime): hang up a client terminal's shell when stopping it - #6116

Open
UncertaintyDeterminesYou4ndMe wants to merge 1 commit into
mainfrom
fix/terminal-close-sighup
Open

UncertaintyDeterminesYou4ndMe wants to merge 1 commit into
mainfrom
fix/terminal-close-sighup

Conversation

@UncertaintyDeterminesYou4ndMe

Copy link
Copy Markdown
Contributor

Summary

Closing a terminal took about 2 s in every client. A client terminal runs exec "$SHELL" -l in a PTY, an interactive shell, and interactive zsh and bash ignore SIGTERM. So runtime.resource.stop always waited out the 2 s kill grace (DEFAULT_PROCESS_TERMINATION_GRACE_MS) until SIGKILL ended the shell, and only then answered; a client that keeps the tab until the stop is confirmed (Desktop) shows it for those 2 s.

A client terminal's run now carries interactiveShell: true. Stopping it sends SIGTERM to the process tree as before, then SIGHUP to the shell process alone, as closing a terminal window does. The shell exits at once and, by default, hangs up its jobs; the rest of its group still gets SIGTERM first. The grace and the SIGKILL after it are unchanged, for a shell that traps HUP.

Two consequences of the shell now exiting on a hang-up rather than a SIGKILL: zsh runs ~/.zlogout, and a job in the terminal that ignores both SIGTERM and SIGHUP (for example one started with nohup and trapping TERM) outlives the stop, as it outlives a closed terminal window and as it already does for any run whose root exits on SIGTERM. Before, it was killed by the SIGKILL after the grace.

Only client terminals are hung up. Other runs, including the model's PTY runs, are unchanged: their root may be the program itself, which may treat SIGHUP as a reload. A pipe run cannot carry the flag; the manager refuses it at start.

Verification

  • runtime.resource.stop of a client terminal on a dev Host built from this branch, five runs each: 2073–2086 ms with the coordinator not setting the flag (as on main), 51–61 ms with it; both end cancelled, exit code 130.
  • The new shell-run-manager tests: with a 30 s grace the stop ends in about 90 ms, the group getting only SIGTERM and the shell SIGHUP; on main's source the test fails after 30 s with the group also getting SIGKILL. A second test checks that a job in the shell's group that ignores SIGHUP is gone after the stop. The coordinator test pins the flag on a terminal start and its absence on a command start.
  • shell-run-manager and pty-process-driver (71 tests, 4 skipped as on main) and the three runtime-resource-* files in runtime-host (32 tests) pass. Not run locally: the root npm run typecheck and the full workspace suite, because my local dependencies are older than this commit's lockfile.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code found the cause, wrote the change and its tests, and reviewed them adversarially; I reviewed the result.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally (Biome and the affected suites pass; the root typecheck was not run locally, see Verification)

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Interactive zsh and bash ignore SIGTERM, so every client terminal close
waited out the 2 s kill grace until SIGKILL ended the shell. Stopping a
client terminal still sends SIGTERM to its process tree, then hangs up the
shell process alone, as closing a terminal window does, so the shell exits
at once while the rest of its group keeps SIGTERM's graceful exit. Other
runs, including the model's PTY runs, are not hung up, because a program
may treat SIGHUP as a reload.

Generated-by: Claude Code
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Oct 15, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Reviewed range: merge-base with main..1797715 (single commit, first review, no prior findings).

The approach is sound. On POSIX, runTermination still runs the SIGTERM pass via terminateProcessTree (SIGKILL to descendants that left the root's group, then SIGTERM to -pid). Only after that does it send SIGHUP to the shell's pid alone, through node-pty's kill (process.kill(pid, 'SIGHUP')), and then it waits for the same grace and SIGKILL escalation as before. So a shell that traps HUP is still force-killed after the grace. Under job control, jobs started in an interactive terminal sit in their own process groups. The SIGTERM pass already SIGKILLs those jobs as escaped descendants, so the "nohup job outlives the stop" case in the description only applies to processes that share the shell's own group. The new code is guarded by rootExited || finalizeOnce with no await between the check and the signal, and an ESRCH is swallowed. The only remaining pid-reuse window is the gap between node-pty's native reap and the JS onExit callback. The existing SIGKILL escalation has the same window, so this is not a new class of race. On Windows, the hang-up is skipped and taskkill /t /f has already ended the tree, so nothing changes there. The pipe-mode refusal and the coordinator flag (set only when command is undefined) look right.

Findings

P2: PR-caused CI failure. The test job fails in scripts/windows-package-source-closure.test.mjs ("the recovery filter is exactly the Windows-branching closure of its tests"). The new process.platform !== 'win32' guard is the first win32 literal in packages/runtime/src/shell-run-manager.ts. That puts the file in the Windows-branching closure, but .github/workflows/windows-recovery.yml does not list it in its path filter. Main's file has no win32 and the latest main CI run is green, so this failure comes from the PR. There are two ways to fix it: add the file to the windows-recovery path filter (which schedules the Windows lane on every shell-run-manager edit), or move the platform decision into PtyProcessDriver, which already branches on win32 and is already in the filter. For example, a hangUp() that does nothing on Windows. Either works. The second also avoids killPty turning a SIGHUP into a full pty.kill() on Windows if the guard is ever dropped.

P3: The second new test ("ends a job that ignores SIGHUP in an interactive shell process group") also passes on main, because the job is ended by the group SIGTERM that predates this change. It is a reasonable guard that the hang-up does not leave group members behind, but it does not prove the fix. Only the first test fails without the change: on main it would see SIGTERM and then SIGKILL on -pid and take about 30 s. Both tests run bash -c without job control, so neither exercises a real interactive login shell. That gap is acceptable given the manual timing evidence in the description.

P3 (nit): The coordinator test comment says "hangs up the shell instead of sending SIGTERM". It is in addition to SIGTERM, as the contract doc says.

CI / mergeability: GitHub reports the PR as MERGEABLE, with merge state BLOCKED. test fails (P2 above), while label and windows_recovery pass. No protocol files are touched, so there is no epoch impact.

// the SIGKILL after the grace. Its group has had SIGTERM; signalling the
// group once the shell is gone could reach a reused process group ID.
// Windows has no hang-up, and taskkill above has already ended the tree.
if (live.mode === 'pty' && live.interactiveShell && process.platform !== 'win32') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This win32 literal adds shell-run-manager.ts to the Windows-branching closure, so windows-package-source-closure.test.mjs fails: the windows-recovery.yml path filter does not list the file. Either add it to the filter, or move the platform check into PtyProcessDriver (for example, a hangUp() that does nothing on Windows), which is already in the filter.

assert.equal(manager.livePtyCount(), 0);
});

test('ends a job that ignores SIGHUP in an interactive shell process group', {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also passes without the fix, because the group SIGTERM ends the job. It works as a guard, but only the previous test proves the change.

visibility: undefined,
cwd: '/workspace',
pty: true,
// Stopping the terminal hangs up the shell instead of sending SIGTERM.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the hang-up comes after SIGTERM, not instead of it.

This branch has not been deployed

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

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants