Repository navigation
Keep topaz's own output off the terminal during a gemdb run - #132
Conversation
Ctrl+C ended `gemdb app.py` with `[Sending soft break]`, a line number in gemdb-run.tpz, `ERROR 3004 , a ExitClientError occurred` and `Logging out session 1.` around the KeyboardInterrupt, and the ERROR line read as a failure. The wrapper now runs the driver under -P with an -I init file that pushes topaz's output to a per-run log, so the terminal shows only what the program printed. The program's output is untouched: it goes to GsFile stdout/stderr, the process's own descriptors. The log is shown, less topaz's echo of the driver, when no status came back: a refused login or a session ended mid-run is reported by topaz alone. Under -P topaz reads stdin at the end of the script, so the driver now ends with `exit`; without it, a refused login with input piped in ran that input as topaz commands. -I also stops any .topazini being read. Fixes #131. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| GEMDB_STATUS_FILE="$(mktemp "\${TMPDIR:-/tmp}/gemdb-status-XXXXXX")" | ||
| export GEMDB_STATUS_FILE | ||
| topaz -L -q -S "$ROOT/bin/gemdb-run.tpz" ${TOPAZ_TUNING} -- "$@" | ||
| GEMDB_TOPAZ_LOG="$(mktemp "\${TMPDIR:-/tmp}/gemdb-topaz-XXXXXX")" |
There was a problem hiding this comment.
💭 Should we clean these temp files up with a trap '…' EXIT instead of relying on the rm -f further down?
Under -P the log also gets topaz's echo of set user … pass … (which is what the redaction assertion in the new integration test guards against on stderr), so the log now holds the password at rest — for an external database, that's the real one. If the wrapper never reaches the rm -f (e.g. the user closes the terminal tab mid-run → SIGHUP, or the process gets a SIGTERM), the log stays behind in $TMPDIR.
It's 0600 from mktemp, same as the driver, so it's not readable by other users — it would just be one more copy of the password that nobody ever removes. The status file and the -c code file have the same gap today, so a single trap would cover all three. (I checked that bash runs an EXIT trap on both HUP and TERM.)
There was a problem hiding this comment.
Done in eb047d1: one EXIT trap, armed before the first mktemp, now removes the status file, the topaz log and a -c run's code file. It replaces the rm -f lines at the end. A function rather than an array, because macOS's /bin/bash 3.2 treats an empty array as unbound under set -u. I measured that 3.2 runs the trap on TERM, HUP and INT, including when the whole process group gets the signal, and that the status passed to exit is unchanged.
One correction to the premise: the log doesn't hold the password. Topaz redacts it in the echo. The log line reads set user gemdb pass ******************************** (measured with the real 32-character password, and with a wrong one). The new refused-login test still checks that stderr carries nothing beyond GemStone's report. Either way the trap was worth doing, since the status and code files had the same gap.
New integration test: it starts gemdb forever.py in its own process group, sends SIGHUP to the group the way closing a terminal tab does, and requires $TMPDIR to be empty. It fails against the previous wrapper.
That test turned up one more thing: topaz survives the hangup and keeps running the program with its session. That was already true on main under -S, so it isn't from this PR. The test ends topaz with SIGTERM so the next test can stop the stone. On a real terminal the next write to the dead tty probably ends it, but a program that never prints would run on. I'll raise that separately.
| expect(ran.stderr).toContain('Login failed'); | ||
| expect(ran.stderr).not.toContain('not-the-password'); | ||
| expect(ran.stdout + ran.stderr).not.toContain('LEAKED'); | ||
| expect(ran.stdout).not.toContain('hello from'); | ||
| expect(ran.code).not.toBe(0); |
There was a problem hiding this comment.
💭 Is it worth pinning what the user actually sees here, the way the Ctrl-C test now does with toEqual([...])?
As written, toContain('Login failed') plus the password check only cover the second sed expression (prompt lines). If the run … % range expression stopped matching — say topaz's prompt or echo format changes in the next alpha — the whole driver's Smalltalk would be dumped on stderr and this test would still pass. And since topaz "carries on through the script" after the refused login, whatever it says about the run block with no session also ends up here, which is exactly the kind of noise this PR is about.
Even a cheaper expect(ran.stderr).not.toContain('commandLineArguments') would catch the driver leaking. Similarly, could not.toBe(0) be the exact code the wrapper falls back to, so a change in that case branch is noticed?
There was a problem hiding this comment.
Yes. In eb047d1 the test pins stderr line by line to GemStone's report and nothing else:
-----------------------------------------------------
GemStone: Error Fatal
Login failed: the userId/password combination is invalid or expired.
Error Category: … Number: 4051 …
Arg 1: nil
So the driver leaking through, if the run … % range ever stops matching, fails it. So would anything topaz says about the run block it goes on through with no session; today it says nothing that reaches stderr (captured before pinning). Only the dashes and the 4051 line are matched loosely, so the category numbers can change in a future alpha.
The exit code is now toBe(1). That's topaz's own status, which the case passes on when there's no status file.
stdout is deliberately not pinned. The gem sometimes writes stone ibSocket read failed … SSL routines::wrong version number there after a failed login. That's the gem's own output, not topaz's, and it doesn't appear every time. stdout is still checked for the piped LEAKED line and for the program's output.
The status file, the topaz log and a -c run's code file were removed by lines at the end of the wrapper, so a run whose terminal was closed, or that was sent SIGTERM, left them in $TMPDIR. One EXIT trap, armed before the first is made, now covers all three; bash runs it on SIGHUP and SIGTERM as well (measured with macOS's /bin/bash 3.2). The refused-login test now pins stderr to GemStone's report alone and the exit status to 1, so the driver leaking through the log filter, or topaz saying anything about the run block, fails it. A new test closes a run's "terminal" mid-run and finds TMPDIR empty; against the previous wrapper it fails. topaz itself outlives the hangup, as it did before, so the test ends it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #131.
What a reader sees
Stopping
gemdb app.pywith Ctrl+C, before:After (the
^Cis the terminal's own echo, as withpython3):The exit status is still 130.
How
Everything topaz itself prints now goes to a per-run log instead of the terminal. The program's output is unaffected because it never goes through topaz:
print()and the driver's own reports write toGsFile stdout/stderr, which in linked topaz are the process's real descriptors.Three pieces, each needed (measured on 4.0.0.a4):
-Pinstead of-S. Under-S,OUTPUT PUSH … ONLYhas no effect on stdout. Topaz's help says so, andoutput push /dev/null onlychanged nothing.-Iinit file (bin/gemdb-run-init.tpz).-Pechoes script lines until the push takes effect.-idoesn't stop that echo./dev/null. A refused login (session limit, space threshold, an external database's password) or a session the stone ends mid-run is reported only by topaz. When no status came back, the wrapper prints the log, minus topaz's echo of the driver.Two consequences:
exit. Under-P, topaz reads commands from stdin at the end of the script. Without theexit, a refused login with input piped in ran that input as topaz commands; the new integration test catches that if theexitis removed..topaziniis read any more, whether from home or from the current directory. The driver'ssetlines already overrode one for the login.Approaches tried that didn't work:
System logoutinstead of theExitClientError(worse: a fatal-error block, then stranded attopaz>), andGsFile _getStdFile: 3(only descriptors 0–2 are accepted).Tests
cli.test.ts(integration): the Ctrl-C test now requires the terminal to show exactlyserving,cleaned up,KeyboardInterrupt. New test: a refused login is reported on stderr, the password stays redacted, and input piped in is not run.cli.test.ts(unit): the old "noexitline" assertion is inverted, with checks on the init file and the wrapper.input()waits, a double Ctrl-C,input()andsys.exit(3), an uncaught exception, and a refused login on a terminal.🤖 Generated with Claude Code