Skip to content

Commit 6b859fc

Browse files
committed
Close a native pane on SIGHUP instead of an ignored SIGTERM
Closing a native task pane cost 1.65-1.79 s. Phase instrumentation put essentially all of it in one place: the host signalled the shell tree with SIGTERM and waited out a 1500 ms grace window before escalating to SIGKILL. An interactive shell on a PTY ignores SIGTERM, so that window always expired and only the force-kill ever retired a pane - the graceful path reaped neither the shell nor its children. Send the shell and its foreground group SIGHUP, the hangup a PTY shell is built to honour; it exits at once and HUPs its own jobs. Descendants keep SIGTERM so a process that traps it gets the notification it got before, and the bounded SIGKILL ladder stays behind it for a shell that traps SIGHUP. Also back the exit-observation poll off from a flat 100 ms tick to 5 ms -> 100 ms, and give registry.stop an optional phase observer so the next slow teardown is attributed rather than guessed. Measured, real hosts, macOS, n=15: close p50 1662 -> 124 ms, p95 1770 -> 403 ms. Covered by graceful-teardown.bun-e2e.ts (idle, foreground-child, and trap-both-signals cases, each asserting no leaked record, token, host, shell or child) and by kill-tree.test.ts in CI.
1 parent 1eda67e commit 6b859fc

9 files changed

Lines changed: 623 additions & 11 deletions

File tree

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Short: Closing a native pane is instant
2+
3+
Closing a pane on the native terminal backend took about 1.7 seconds: the host asked the shell to stop with SIGTERM, which an interactive shell on a PTY ignores, so every close waited out the full grace window and then force-killed. The host now sends SIGHUP, the hangup a terminal shell actually honours, and a pane closes in about 0.12 seconds with its foreground processes properly reaped.
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
# 193 — A native pane closes on SIGHUP, because an interactive PTY shell ignores SIGTERM
2+
3+
## Context
4+
5+
Closing a native task pane took 1.65–1.79 s (seq 1382's measurement, reproduced here at
6+
p50 1662 ms / p95 1770 ms). Decision 192 had already cleared the read/split/layout path,
7+
so the close was the last multi-second action left on the native backend.
8+
9+
## Investigation
10+
11+
`scripts/measure-native-pane-close.ts` attributes one close to named phases
12+
(`classify` / `handshake` / `exitWait` / `forceTerm` / `forceKill`) via a new optional
13+
`onPhase` observer on `registry.stop`. Baseline, real hosts, macOS, idle, n=7:
14+
15+
| phase | p50 |
16+
|---|---|
17+
| classify | 13.5 ms |
18+
| handshake | 2.1 ms |
19+
| **exitWait** | **1720.8 ms** |
20+
21+
The graceful handshake was never slow — it answered in 2 ms. Everything was spent
22+
watching for a shell that was not going to exit.
23+
24+
`host.ts shutdown()` signalled the shell tree with **SIGTERM**, then waited
25+
`Promise.race([proc.exited, delay(1500)])` before escalating to SIGKILL. An
26+
**interactive shell on a PTY ignores SIGTERM**, so that race always lost. Measured
27+
directly against a real zsh:
28+
29+
| shell | signal | exits? | foreground child reaped? |
30+
|---|---|---|---|
31+
| pipes (non-PTY) | SIGTERM | yes, 1 ms ||
32+
| PTY | SIGTERM | **no**, still alive at 2001 ms | **no** |
33+
| PTY | SIGKILL | yes, 1 ms ||
34+
| PTY | **SIGHUP** | **yes, 1 ms** | **yes** |
35+
36+
So the graceful path cleaned up neither the shell nor its children; only the SIGKILL
37+
escalation ever retired a pane, and every close paid the full grace window to get there.
38+
The non-PTY row is why this was never caught: every context that spawns the shell on
39+
pipes sees SIGTERM work perfectly.
40+
41+
## Decision
42+
43+
- `host.ts shutdown()` sends **SIGHUP** to the shell and its foreground process group —
44+
the hangup a PTY shell is built to honour, and what a terminal emulator sends when its
45+
window closes. `killTree` takes the shell signal and the descendant signal separately;
46+
descendants keep **SIGTERM**, so a server that traps it gets exactly the notification it
47+
got before.
48+
- The bounded ladder is unchanged: 1500 ms grace, then SIGKILL, then a 1000 ms settle.
49+
A shell that traps SIGHUP still stops, just via the fallback.
50+
- `registry.stop`'s exit-observation poll backs off 5 ms → 100 ms instead of a flat
51+
100 ms tick. Same deadline, same exit condition; the common case no longer pays a whole
52+
tick to notice an already-dead process. A/B at n=15: close p50 150.6 → 124.0 ms,
53+
p95 446.5 → 402.7 ms.
54+
- `registry.stop` accepts an optional `onPhase` observer. Off by default and free when
55+
absent — it exists so the next slow teardown is attributed instead of guessed.
56+
57+
Measured after (same harness, n=15): **close p50 1662 → 124 ms, p95 1770 → 403 ms**.
58+
`exitWait` p50 1720.8 → 80.4 ms, and what remains is genuine host exit work (server
59+
stop, parser flush, journal stop, state removal), not poll granularity.
60+
61+
`graceful-teardown.bun-e2e.ts` locks it in: idle shell and foreground-child cases must
62+
stop inside 800 ms with record, token, host PID, shell PID and the child all gone; the
63+
trap-both-signals case must still stop, must stay inside the 4000 ms fallback bound, and
64+
must exceed the graceful budget — proving force is still the floor and never the first
65+
move. The test was verified to fail on the pre-change code (1640 ms vs the 800 ms budget).
66+
67+
## Risks
68+
69+
- Descendants now have far less wall-clock before the shell goes away, because the shell
70+
no longer lingers for 1500 ms. That lingering was an accident of the bug, not a
71+
contract, and a real terminal gives no such window either — but a pane hosting a
72+
process that relied on a slow SIGTERM death will now be cut short.
73+
- SIGHUP and SIGTERM have the same default disposition (terminate), so only processes
74+
that explicitly handle one of them can observe the difference at all.
75+
- Windows/ConPTY is untouched: that branch returns before `killTree` and tears down
76+
through the token-named Job Object exactly as before. The e2e skips on `win32`, so the
77+
POSIX budgets are not asserted against a platform that does not use this path.
78+
- The 5 ms poll floor raises the syscall rate at the very start of a stop. Bounded — it
79+
doubles to the same 100 ms ceiling within five iterations.
80+
81+
## Alternatives considered
82+
83+
- **Shorten the 1500 ms grace window.** Treats the symptom: the shell would still ignore
84+
the signal, and every close would still end in SIGKILL — just sooner. It trades the
85+
latency for a strictly less graceful teardown.
86+
- **Send SIGHUP to descendants too.** Simpler signature, but it silently changes what a
87+
trapped-SIGTERM process in the pane receives. Keeping SIGTERM there costs one parameter
88+
and preserves existing behaviour exactly.
89+
- **Write `exit\r` into the terminal**, as the Windows branch does. Fragile on POSIX: with
90+
a foreground process running, the text is delivered to that process instead of the shell.
91+
- **Unconditional SIGKILL.** Fastest and wrong — it removes the graceful path the pane
92+
contract depends on, and would strand descendants that clean up on a signal.

package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
"test:pane-e2e": "bun --preload ./src/bun/__tests__/pane-exit-e2e-preload.ts src/bun/__tests__/pane-exit-e2e.ts",
2222
"test:proto-e2e": "bun src/bun/prototypes/detached-pty/__tests__/lifecycle.bun-e2e.ts",
2323
"test:native-registry-e2e": "bun src/bun/native-terminal-registry/__tests__/lifecycle.bun-e2e.ts",
24+
"test:native-teardown-e2e": "bun src/bun/native-terminal-registry/__tests__/graceful-teardown.bun-e2e.ts",
2425
"test:native-crash-e2e": "bun src/bun/native-terminal-registry/__tests__/crash-recovery.bun-e2e.ts",
2526
"test:native-multi-client-e2e": "bun src/bun/native-terminal-registry/__tests__/multi-client.bun-e2e.ts",
2627
"test:native-app-restart-e2e": "bun src/bun/native-terminal-registry/__tests__/app-restart.bun-e2e.ts",
@@ -33,6 +34,7 @@
3334
"test:native-live-parser-e2e": "bun src/bun/native-terminal-registry/__tests__/live-parser.bun-e2e.ts",
3435
"test:native-host-images-e2e": "bun src/bun/native-terminal-registry/host-images/__tests__/lifecycle.bun-e2e.ts",
3536
"measure:native-pane-latency": "bun scripts/measure-native-pane-latency.ts",
37+
"measure:native-pane-close": "bun scripts/measure-native-pane-close.ts",
3638
"test:native-soak": "bun src/bun/native-terminal-soak/run-soak.ts",
3739
"test:native-host-image": "bunx vitest run --config vitest.config.bun.ts src/bun/native-terminal-registry/host-images/__tests__/artifact-manifest.test.ts src/bun/native-terminal-registry/host-images/__tests__/artifact-manifest-cli.test.ts src/bun/native-terminal-registry/host-images/__tests__/packaged-image-manifest.test.ts src/bun/native-terminal-registry/host-images/__tests__/packaged-image.test.ts",
3840
"package:win-archive": "bun scripts/generate-build-info.ts && bun scripts/generate-changelog.ts && vite build && bun run build:cli && electrobun build --env=canary",
Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
#!/usr/bin/env bun
2+
/**
3+
* Backend latency of CLOSING a native task pane, against REAL hosts (seq 1387).
4+
*
5+
* The sibling script `measure-native-pane-latency.ts` covers read/split/layout. This
6+
* one answers the question that one left open: where do the ~1.7 s of a pane close
7+
* actually go? It attributes every close to named phases instead of a single total:
8+
*
9+
* classify — the ownership verdict that gates signalling at all
10+
* handshake — connect + requestStop against the pane host
11+
* exitWait — the 100 ms registry poll until host + shell are observably gone
12+
* forceTerm — SIGTERM escalation, only when the handshake itself failed
13+
* forceKill — SIGKILL escalation, only when exitWait ran out
14+
* coordinator— the remainder of closeNativeTaskPane: layout reconcile + journal write
15+
*
16+
* Run: `bun run measure:native-pane-close` (JSON to stdout, progress to stderr).
17+
* Everything lands in a tmpdir, so `~/.dev3.0/` is never touched.
18+
*/
19+
20+
import { mkdirSync, mkdtempSync, rmSync } from "node:fs";
21+
import { tmpdir } from "node:os";
22+
import { join } from "node:path";
23+
import { NATIVE_MULTIPANE_DIR_ENV } from "../src/bun/native-terminal-multipane/paths";
24+
import type { StopPhase } from "../src/bun/native-terminal-registry/registry";
25+
26+
/** Fixed id so repeated runs address the same deterministic session id. */
27+
const TASK_ID = "00000000-0000-4000-8000-000000013870";
28+
const REPS = Number(process.env.CLOSE_REPS) || 7;
29+
30+
interface Stats {
31+
n: number;
32+
min: number;
33+
p50: number;
34+
p95: number;
35+
max: number;
36+
}
37+
38+
function stats(samples: number[]): Stats {
39+
const sorted = [...samples].sort((a, b) => a - b);
40+
const at = (q: number): number => sorted[Math.min(sorted.length - 1, Math.floor(sorted.length * q))]!;
41+
const round = (value: number): number => Math.round(value * 10) / 10;
42+
return {
43+
n: sorted.length,
44+
min: round(sorted[0]!),
45+
p50: round(at(0.5)),
46+
p95: round(at(0.95)),
47+
max: round(sorted[sorted.length - 1]!),
48+
};
49+
}
50+
51+
function summarise(byPhase: Map<string, number[]>): Record<string, Stats> {
52+
const out: Record<string, Stats> = {};
53+
for (const [name, samples] of [...byPhase].sort(([a], [b]) => a.localeCompare(b))) {
54+
if (samples.length) out[name] = stats(samples);
55+
}
56+
return out;
57+
}
58+
59+
async function main(): Promise<void> {
60+
const root = mkdtempSync(join(tmpdir(), "dev3-native-pane-close-"));
61+
const work = join(root, "work");
62+
mkdirSync(work, { recursive: true });
63+
process.env.DEV3_NATIVE_SESSIONS_DIR = join(root, "native-sessions");
64+
process.env[NATIVE_MULTIPANE_DIR_ENV] = join(root, "native-multipane");
65+
process.env.DEV3_NATIVE_HOST_IMAGES_DIR = join(root, "host-images");
66+
process.env.DEV3_LOG_DIR = join(root, "logs");
67+
68+
// Imported after the redirects: the path modules read these on first use.
69+
const panes = await import("../src/bun/native-task-panes");
70+
const registry = await import("../src/bun/native-terminal-registry/registry");
71+
const { paneSessionId } = await import("../src/bun/native-terminal-multipane/paths");
72+
const { nativeTaskSessionId } = await import("../src/bun/task-terminal-backend");
73+
const { defaultNativeShellLaunchSpec } = await import("../src/bun/native-terminal-registry/shell-launch");
74+
75+
const defaults = defaultNativeShellLaunchSpec({ platform: process.platform, cwd: work, env: process.env });
76+
const launch = { executable: defaults.executable, argv: [...defaults.argv] };
77+
const spawnOpts = { cwd: work, env: {}, launch };
78+
79+
const closeTotals: number[] = [];
80+
const stopTotals: number[] = [];
81+
const byPhase = new Map<string, number[]>();
82+
const push = (name: string, ms: number): void => {
83+
const bucket = byPhase.get(name) ?? [];
84+
bucket.push(ms);
85+
byPhase.set(name, bucket);
86+
};
87+
88+
await panes.startNativeTaskPanes({ taskId: TASK_ID, cwd: work, env: {}, launch, cols: 120, rows: 40 });
89+
90+
try {
91+
for (let rep = 0; rep < REPS; rep++) {
92+
// Grow to two panes so the close reconciles a layout instead of tearing down.
93+
const before = (await panes.nativeTaskPanesState(TASK_ID))!;
94+
const created = await panes.splitNativeTaskPane(TASK_ID, before.panes[0]!.paneId, "vertical", spawnOpts);
95+
96+
const mark = performance.now();
97+
await panes.closeNativeTaskPane(TASK_ID, created.paneId);
98+
closeTotals.push(performance.now() - mark);
99+
console.error(` close rep=${rep + 1}/${REPS}`);
100+
}
101+
102+
// Second pass: the same teardown one level down, so the registry phases are
103+
// attributed directly rather than inferred from the end-to-end total.
104+
for (let rep = 0; rep < REPS; rep++) {
105+
const before = (await panes.nativeTaskPanesState(TASK_ID))!;
106+
const created = await panes.splitNativeTaskPane(TASK_ID, before.panes[0]!.paneId, "vertical", spawnOpts);
107+
const sessionId = paneSessionId(nativeTaskSessionId(TASK_ID), created.paneId);
108+
109+
const mark = performance.now();
110+
await registry.stop(sessionId, {
111+
onPhase: (phase: StopPhase, ms: number) => push(phase, ms),
112+
});
113+
stopTotals.push(performance.now() - mark);
114+
115+
// Let the coordinator reconcile the now-dead pane out of the tree.
116+
await panes.closeNativeTaskPane(TASK_ID, created.paneId).catch(() => {});
117+
console.error(` stop rep=${rep + 1}/${REPS}`);
118+
}
119+
120+
const phases = summarise(byPhase);
121+
const closeStats = stats(closeTotals);
122+
const stopStats = stats(stopTotals);
123+
const phaseSum = Object.values(phases).reduce((acc, s) => acc + s.p50, 0);
124+
console.log(
125+
JSON.stringify(
126+
{
127+
platform: process.platform,
128+
bun: Bun.version,
129+
shell: launch.executable,
130+
closeEndToEnd: closeStats,
131+
registryStop: stopStats,
132+
phases,
133+
/** What closeNativeTaskPane costs on top of registry.stop, at p50. */
134+
coordinatorOverheadP50Ms: Math.round((closeStats.p50 - stopStats.p50) * 10) / 10,
135+
phaseSumP50Ms: Math.round(phaseSum * 10) / 10,
136+
},
137+
null,
138+
2,
139+
),
140+
);
141+
} finally {
142+
await panes.stopNativeTaskPanes(TASK_ID).catch(() => {});
143+
rmSync(root, { recursive: true, force: true });
144+
}
145+
// The registry holds host sockets open; nothing else keeps this process useful.
146+
process.exit(0);
147+
}
148+
149+
main().catch((err) => {
150+
console.error(err);
151+
process.exit(1);
152+
});

0 commit comments

Comments
 (0)