From 84efc742ff99d7bc8ce8c4641f07eb6aea9e814f Mon Sep 17 00:00:00 2001 From: James Foster Date: Thu, 15 Oct 2026 18:47:19 -0300 Subject: [PATCH] End a gemdb run when its terminal closes Closing the terminal of `gemdb app.py` ended the bash wrapper but not topaz, which ignores SIGHUP: it carried on running the program with its session (and a web app's port), out of sight. A program that kept printing was not ended by its writes to the dead terminal either, and its finally blocks never ran. Measured on 4.0.0.a4 by closing a pty under it. The wrapper's EXIT trap, which bash runs on a hangup, now sends a topaz that is still running SIGINT, what Ctrl-C sends: the program's finally blocks run and topaz logs out and exits, within a quarter of a second in the measurement. SIGTERM follows if it has not gone in ten seconds. The temporary files are removed only after topaz has gone, since the driver writes the status file as it unwinds. The integration test now closes a real pty, as a terminal tab does, and requires topaz to end on its own with the finally block run. Against the previous wrapper it fails in 15 s rather than timing out: it waits on expect's exit, not its pipes, which a surviving topaz holds open. Fixes #136. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 ++ docs/grail.md | 12 ++++++ src/__integration__/cli.test.ts | 68 +++++++++++++++++++++------------ src/__tests__/cli.test.ts | 11 ++++-- src/cli.ts | 45 ++++++++++++++++++---- 5 files changed, 105 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c6bef1..b91ca8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 now prints only `KeyboardInterrupt`, as Python does. GemStone's own messages from a `gemdb` run are shown only when the program could not run to the end, such as a refused login, and a `.topazini` file is no longer read. +- **Closing the terminal ends a `gemdb` run.** Closing the terminal tab of `gemdb app.py`, or of + **Run Python File in GemDB**, left the program running out of sight, holding a database session + and, for a web app, its port. It is now stopped as Ctrl+C would stop it: its `finally` blocks run + and its session is given back. ## [1.6.0] - 2026-10-07 diff --git a/docs/grail.md b/docs/grail.md index cd6429a..8fd1429 100644 --- a/docs/grail.md +++ b/docs/grail.md @@ -85,6 +85,18 @@ commands from stdin, and a refused login with input piped in ran that input as topaz commands (measured, and covered by `cli.test.ts`). Under `-S` the same `exit` was ignored, and on a tty it printed four lines saying so. +**Topaz ignores SIGHUP, so the wrapper ends it.** Measured on 4.0.0.a4 by +closing a pty under topaz: the wrapper exits, but topaz carries on running +the program with its session, whether the program is quiet or keeps printing +to the dead terminal, and its `finally` blocks never run (#136). The +wrapper's `EXIT` trap, which bash runs on a hangup, therefore sends a topaz +still running SIGINT, which is what Ctrl-C sends. The driver turns that into +`KeyboardInterrupt`, the `finally` blocks run, and topaz logs out and exits, +within a quarter of a second in the measurement. A topaz still running after +ten seconds gets SIGTERM, which skips `finally`. The temporary files are +removed only after topaz has gone, because the driver writes the status file +as it unwinds and would otherwise recreate one the trap had just removed. + Errors in the driver are caught as `AbstractException`, not `Error`: Grail's Python exceptions live outside the `Error` branch, which is why grail.tpz's own file mode exits 0 on a Python diff --git a/src/__integration__/cli.test.ts b/src/__integration__/cli.test.ts index 06c247b..81eae7b 100644 --- a/src/__integration__/cli.test.ts +++ b/src/__integration__/cli.test.ts @@ -311,35 +311,53 @@ exit [lindex $result 3] expect(ran.code).toBe(1); }); - it('leaves no temporary files behind, even when its terminal closes mid-run', async () => { + it('ends the program, finally blocks and all, when its terminal closes', async () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gemdb-cli-tmp-')); const env = { ...process.env, TMPDIR: tmp }; + fs.rmSync(path.join(workDir, 'hung-up.txt'), { force: true }); fs.writeFileSync( - path.join(workDir, 'forever.py'), - 'import time\nprint("serving", flush=True)\nwhile True:\n time.sleep(0.2)\n', + path.join(workDir, 'quiet.py'), + 'import time\nprint("serving", flush=True)\ntry:\n while True:\n time.sleep(0.2)\n' + + 'finally:\n open("hung-up.txt", "w").write("finally ran")\n', ); await new Promise((resolve) => execFile('bash', [cliPath(), '-c', 'print(1)'], { cwd: workDir, env }, resolve), ); - // Its own process group, so the hangup reaches the wrapper and topaz - // together, as closing a terminal tab does. - const child = spawn('bash', [cliPath(), 'forever.py'], { cwd: workDir, env, detached: true }); - const exited = new Promise((resolve) => child.on('exit', resolve)); - await new Promise((resolve) => { - child.stdout.on('data', (chunk: Buffer) => { - if (chunk.toString().includes('serving')) resolve(); - }); - }); - - process.kill(-(child.pid ?? 0), 'SIGHUP'); - await exited; + // Closing expect's side of the pty is what closing a terminal tab does: + // the kernel hangs up the foreground process group, wrapper and topaz. + const scriptPath = path.join(workDir, 'hangup.exp'); + fs.writeFileSync( + scriptPath, + `set timeout 90 +spawn bash ${cliPath()} quiet.py +expect { + -ex "serving" {} + timeout { puts stderr "TIMEOUT waiting to serve"; exit 9 } +} +puts "WRAPPER=[exp_pid] TOPAZ=[exec pgrep -P [exp_pid] -x topaz]" +close +`, + ); + // On 'exit', not when its output closes: a topaz left running would hold + // expect's pipes open, and the test would time out instead of failing. + const expectRun = spawn('/usr/bin/expect', ['-f', scriptPath], { cwd: workDir, env }); + let transcript = ''; + expectRun.stdout.on('data', (chunk: Buffer) => (transcript += chunk.toString())); + await new Promise((resolve) => expectRun.on('exit', resolve)); + const topaz = Number(/TOPAZ=(\d+)/.exec(transcript)?.[1]); + const wrapper = Number(/WRAPPER=(\d+)/.exec(transcript)?.[1]); + + // topaz ignores SIGHUP, so this is the wrapper's doing: what Ctrl-C sends. + // The wrapper goes last, once it has seen topaz go and removed its files. + const ended = await waitForProcessToEnd(topaz, 15_000); + await waitForProcessToEnd(wrapper, 15_000); const left = fs.readdirSync(tmp); - // topaz outlives a hangup, as it did before the wrapper had a trap, so it - // is ended here: left running it holds a session the next test needs gone. - process.kill(-(child.pid ?? 0), 'SIGTERM'); - await waitForGroupToEnd(child.pid ?? 0); + if (!ended) process.kill(topaz, 'SIGTERM'); fs.rmSync(tmp, { recursive: true, force: true }); + expect(topaz).toBeGreaterThan(0); + expect(ended).toBe(true); + expect(fs.readFileSync(path.join(workDir, 'hung-up.txt'), 'utf8')).toBe('finally ran'); expect(left).toEqual([]); }); @@ -386,15 +404,15 @@ function canMakeFixture(): boolean { return probe !== undefined; } -/** Resolves once no process in the group is left, polling as `kill -0` does. */ -async function waitForGroupToEnd(pgid: number): Promise { - for (let i = 0; i < 100; i++) { +/** Whether `pid` has exited within `ms`, polling as `kill -0` does. */ +async function waitForProcessToEnd(pid: number, ms: number): Promise { + for (let waited = 0; waited < ms; waited += 100) { try { - process.kill(-pgid, 0); + process.kill(pid, 0); } catch { - return; + return true; } await new Promise((resolve) => setTimeout(resolve, 100)); } - throw new Error(`process group ${pgid} still running after 10 s`); + return false; } diff --git a/src/__tests__/cli.test.ts b/src/__tests__/cli.test.ts index 99e8a13..df3bf30 100644 --- a/src/__tests__/cli.test.ts +++ b/src/__tests__/cli.test.ts @@ -123,16 +123,21 @@ describe('writeCliScripts', () => { expect(script).toMatch(/if \[ -z "\$STATUS" \]; then\n\s+sed .* "\$GEMDB_TOPAZ_LOG" >&2\n/); }); - it('removes its temporary files however the run ends', () => { + it('tidies up after the run however it ends, topaz included', () => { writeCliScripts(ext); const script = fs.readFileSync(cliPath(), 'utf8'); // Armed before the first file is made, so no exit path — a hangup // included — can skip it, and nothing after the run removes them itself. - expect(script.indexOf('trap remove_temp_files EXIT')).toBeGreaterThan(-1); - expect(script.indexOf('trap remove_temp_files EXIT')).toBeLessThan(script.indexOf('mktemp')); + expect(script.indexOf('trap end_run EXIT')).toBeGreaterThan(-1); + expect(script.indexOf('trap end_run EXIT')).toBeLessThan(script.indexOf('mktemp')); expect(script).not.toMatch(/^rm -f "\$GEMDB_STATUS_FILE"/m); + // topaz ignores SIGHUP, so a wrapper ending first sends it Ctrl-C — but + // only while topaz is running, so a finished run looks for nothing. + expect(script).toMatch(/TOPAZ_RUNNING=yes\ntopaz -L /); + expect(script).toMatch(/\nRC=\$\?\nTOPAZ_RUNNING=""\n/); + expect(script).toContain('kill -INT "$pid"'); }); it('never leaves topaz reading commands from stdin', () => { diff --git a/src/cli.ts b/src/cli.ts index 7f7da97..40bb8a9 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -624,21 +624,50 @@ fi fi fi -# The run's temporary files, removed however it ends: bash runs an EXIT trap -# on a normal exit, and on SIGHUP (a terminal tab closed mid-run) and SIGTERM -# too — measured with macOS's /bin/bash 3.2, the oldest this runs under. A -# function rather than an array, because 3.2 calls an empty array unbound -# under set -u. +# However the run ends, end_run tidies up after it: bash runs an EXIT trap on +# a normal exit, and on SIGHUP (a terminal tab closed mid-run) and SIGTERM +# too — measured with macOS's /bin/bash 3.2, the oldest this runs under. +# +# A hangup ends this script but not topaz, which ignores SIGHUP: closing the +# terminal left it running the program, holding a session (and a web app its +# port) with nobody to see it, and a program that kept printing was not ended +# by its writes failing either (#136). So if topaz is still running, it is sent +# what Ctrl-C sends — measured after closing the terminal, the program's +# finally blocks ran and topaz logged out and exited within a quarter of a +# second — and SIGTERM, which skips finally, if it has not gone in ten +# seconds. Without pgrep there is no topaz to find, and the run is left as it +# was before. The files go last, because the driver writes the status file as +# it unwinds. They are listed in a loop rather than kept in an array, because +# 3.2 calls an empty array unbound under set -u. CODE_FILE="" GEMDB_STATUS_FILE="" GEMDB_TOPAZ_LOG="" -remove_temp_files() { +TOPAZ_RUNNING="" +wait_for_exit() { + local tries=0 + while kill -0 "$1" 2>/dev/null && [ "$tries" -lt "$2" ]; do + sleep 0.1 + tries=$((tries + 1)) + done + ! kill -0 "$1" 2>/dev/null +} +stop_topaz() { + local pid + pid="$(pgrep -P $$ -x topaz 2>/dev/null | head -n 1)" + [ -n "$pid" ] || return 0 + kill -INT "$pid" 2>/dev/null + wait_for_exit "$pid" 100 && return 0 + kill -TERM "$pid" 2>/dev/null + wait_for_exit "$pid" 50 || true +} +end_run() { + if [ -n "$TOPAZ_RUNNING" ]; then stop_topaz; fi local f for f in "$CODE_FILE" "\${CODE_FILE%.py}" "$GEMDB_STATUS_FILE" "$GEMDB_TOPAZ_LOG"; do if [ -n "$f" ]; then rm -f "$f"; fi done } -trap remove_temp_files EXIT +trap end_run EXIT # -c 'code': materialise the code as a file, like CPython's -c but visibly so. # mktemp is given an explicit template — BSD and GNU disagree about -t. @@ -667,8 +696,10 @@ fi GEMDB_STATUS_FILE="$(mktemp "\${TMPDIR:-/tmp}/gemdb-status-XXXXXX")" GEMDB_TOPAZ_LOG="$(mktemp "\${TMPDIR:-/tmp}/gemdb-topaz-XXXXXX")" export GEMDB_STATUS_FILE GEMDB_TOPAZ_LOG +TOPAZ_RUNNING=yes topaz -L -q -I "$ROOT/bin/gemdb-run-init.tpz" -P "$ROOT/bin/gemdb-run.tpz" ${TOPAZ_TUNING} -- "$@" RC=$? +TOPAZ_RUNNING="" STATUS="$(cat "$GEMDB_STATUS_FILE" 2>/dev/null || true)" if [ -z "$STATUS" ]; then sed -e '/^topaz[^>]*> run$/,/^%$/d' -e '/^topaz[^>]*> /d' "$GEMDB_TOPAZ_LOG" >&2