[kristofszeles]

Summary

When Terminator's window is closed, the shell sometimes fails to flush its history file (the path bash's $HISTFILE points at, defaulting to ~/.bash_history). With bash 5.2 patch 14 or later (released December 2022) the loss is frequent — typically observed on a sizeable fraction of window closes, more under load — rather than the occasional flake it used to be. The cause is two-layered:

• Immediate cause (Terminator): Terminal.close() sends SIGHUP to the shell and immediately tears down the VTE widget, closing the PTY master. This is a fire-and-forget pattern with no synchronization between the application-level signal and the kernel-level PTY hangup that follows microseconds later.
• Upstream cause (bash): a regression introduced in bash 5.2 patch 14 (commit 6647917a, 2022-12-13) added a code path in sig.c that, when a second terminating signal arrives while the shell's termsig_handler is mid-flight, short-circuits to kill_shell(sig) — terminating bash immediately, before maybe_save_shell_history() finishes writing to disk. Bash 5.2 patch 14 didn't cause the race — Terminator's close pattern has always delivered two SIGHUPs in rapid succession — it removed the implicit protection that had been making the race benign. Whether the resulting kill arrives before or after the history write completes depends on system timing, so the user-visible loss is intermittent but now common (empirically ~34% in the harness, higher under load).

This PR fixes the Terminator side by inserting a short, bounded wait so the shell finishes exiting (and the history write completes) before the PTY is destroyed. The fix is correct and complete on its own: by the time the kernel would deliver the second SIGHUP via tty hangup, bash is already gone and there is no recipient to interrupt. Terminator users on every affected bash version get correct behavior the moment this lands.

The bug

Closing the Terminator window — via the [X] button, the keybinding, or exiting the last terminal — sometimes leaves commands typed in the session out of bash's history file. The behavior is non-deterministic: identical sessions can save or drop history depending on scheduling.

Root cause — Terminator side

In terminatorlib/terminal.py, Terminal.close() used to do this:

def close(self):
    """Close ourselves"""
    dbg('close: called')
    self.cnxids.remove_widget(self.vte)
    self.emit('close-term')
    if self.pid is not None:
        try:
            dbg('close: killing %d' % self.pid)
            os.kill(self.pid, signal.SIGHUP)
        except Exception as ex:
            dbg('os.kill failed: %s' % ex)
            pass

    if self.vte:
        self.terminalbox.remove(self.vte)
        del(self.vte)

The window-destroy path (terminatorlib/window.py:294-310) calls terminal.close() for each terminal and then self.destroy() on the window. The above body has zero delay between SIGHUP and PTY teardown.

Concretely the race looks like this:

1. os.kill(self.pid, signal.SIGHUP) — bash enters its termination handler, which runs the EXIT trap and writes $HISTFILE via maybe_save_shell_history().
2. Microseconds later, self.terminalbox.remove(self.vte) and del(self.vte) finalize the VTE widget, closing the PTY master.
3. Closing the PTY master triggers a kernel-generated SIGHUP to bash's foreground process group — a second hangup arriving while bash is still flushing.
4. On last-window close, Terminator's Python process is also tearing down, so bash is being orphaned at the same moment.

Whether bash finishes writing $HISTFILE before step 2/3/4 cuts it short depends partly on kernel scheduling — but as the next section explains, the bash side of the equation also changed in late 2022 in a way that makes the race much harder to win. That is why this same Terminator code was effectively race-free for years on bash ≤ 5.2 patch 13 yet loses on bash ≥ 5.2 patch 14.

Root cause — bash side (the reason losing the race went from harmless to costly)

Bash 5.2 patch 14 (Chet Ramey, commit 6647917a, 2022-12-13, "process additional terminating signals when running the EXIT trap after a terminating signal") added a new short-circuit to termsig_sighandler in sig.c:

/* If we are currently handling a terminating signal, we have a couple of
   choices here. We can ignore this second terminating signal and let the
   shell exit from the first one, or we can exit immediately by killing
   the shell with this signal. This code implements the latter; to implement
   the former, replace the kill_shell(sig) with return. */
if (handling_termsig)
    kill_shell (sig);          /* just short-circuit now */

handling_termsig is set by termsig_handler while it is running the shell's cleanup, including maybe_save_shell_history(). If a second terminating signal arrives during that window, the new branch calls kill_shell(), which restores the default disposition for the signal and kill(getpid(), sig)s the shell — bypassing whatever cleanup was in progress.

Why this lands on Terminator specifically

Terminator's close path delivers two SIGHUPs to the same bash, microseconds apart:

1. First SIGHUP — os.kill(self.pid, signal.SIGHUP) from the close path. Bash enters termsig_sighandler → termsig_handler → sets handling_termsig = SIGHUP → starts maybe_save_shell_history() which opens $HISTFILE and begins writing.
2. Second SIGHUP — Terminator destroys the VTE widget, which closes the PTY master. The kernel's tty-hangup logic delivers SIGHUP to the foreground process group of the slave side, which is the same bash. This second SIGHUP arrives a few hundred microseconds after the first.
3. The new patch-14 branch fires. termsig_sighandler sees handling_termsig != 0 and calls kill_shell(SIGHUP). Bash terminates immediately. The file write started in step 1 is interrupted partway through — $HISTFILE is truncated, empty, or missing the most recent entries depending on where bash was in the fopen / fputs / fclose sequence.

The pre-patch-14 code had no such short-circuit. A second SIGHUP went through the standard terminate_immediately check, which explicitly exempts SIGHUP, SIGTERM, SIGINT, etc. from immediate termination. The duplicate was effectively swallowed; maybe_save_shell_history() ran to completion; history saved correctly.

The bash author flagged the choice deliberately in the source comment, naming the exact one-line revision that would restore the older behavior: "to implement the former, replace the kill_shell(sig) with return."

Why this fix on the Terminator side works regardless of bash

With this PR applied, the timeline becomes:

1. First SIGHUP sent.
2. Terminator waits for the shell to exit. Bash completes maybe_save_shell_history(), runs its EXIT trap, and terminates cleanly via the kill_shell call at the end of termsig_handler (the intentional one, not the regression branch).
3. Terminator destroys the VTE widget. The PTY master closes.
4. The kernel's tty-hangup logic delivers SIGHUP — but bash is already gone and its session/process group is empty. There is no recipient to interrupt.

The race the bash patch-14 branch turns into data loss simply cannot happen, because no second SIGHUP is delivered to a live bash. The fix is durable across bash versions: it works on pre-patch-14 bash (where it would also be correct, just slightly less necessary) and on post-patch-14 bash (where it is required to avoid data loss).

Regression history

This is a two-step regression. A latent Terminator-side race was introduced in 2017 — the second SIGHUP has always been getting delivered to bash a few microseconds after the first. What changed in late 2022 was not the race itself but bash's tolerance for losing it: a bash-side change removed an implicit protection that had been silently absorbing the duplicate SIGHUP. The race outcome flipped from "swallowed, history saved" to "frequently fatal, history sometimes truncated mid-write." Bash didn't cause the bug — it stopped masking it.

2010-01-25 — 8d8681e6 (Chris Jones, "... we now specifically send a SIGHUP to the child of a terminal when that terminal is explicitly close()d"). At this point Terminal.close() was just:

def close(self):
    """Close ourselves"""
    dbg('Terminal::close: emitting close-term')
    self.emit('close-term')
    os.kill(self.pid, signal.SIGHUP)

The PTY/VTE widget was not destroyed here — that happened later, naturally interleaved with GTK's destroy chain, which gave bash plenty of time to flush history between the SIGHUP and the PTY going away. No race; history was reliably saved.

2017-02-11 — c4d372c5 (Stephen Boddy, "Fixes vte object not being released properly, and holding open hidden /tmp files.") added inline VTE teardown immediately after the SIGHUP:

     def close(self):
         """Close ourselves"""
         dbg('close: called')
-        self.cnxids.remove_signal(self.vte, 'child-exited')
+        self.cnxids.remove_widget(self.vte)
         self.emit('close-term')
         try:
             dbg('close: killing %d' % self.pid)
             …
+        if self.vte:
+            self.terminalbox.remove(self.vte)
+            del(self.vte)

That commit fixed a real bug — VTE widgets were leaking and holding open files in /tmp — by forcing the widget reference to drop synchronously inside close() instead of relying on GC during the GTK destroy chain. But pulling the teardown into close() means it now runs ~microseconds after the SIGHUP, with no synchronization between the two. That is the race this PR fixes: the shell is given a hangup signal and then has its PTY ripped out from under it before its SIGHUP handler has a chance to run maybe_save_shell_history() to completion.

2022-12-13 — bash 5.2 patch 14, commit 6647917a (Chet Ramey). Promoted bash's handling_termsig flag from a termsig_handler local to a file-scope static, and added the new if (handling_termsig) kill_shell(sig) branch in termsig_sighandler. The latent Terminator-side race had existed since 2017; this bash change is what made the race start losing reliably.

So the bug surface is:

Period	Terminal.close() behavior	Bash version	History saved on [X]?
2010 → 2017	SIGHUP, then GTK destroys the widget naturally later	any	Yes (no race)
2017 → 2022	SIGHUP, then immediately terminalbox.remove(vte) + del(vte)	bash ≤ 5.2 patch 13	Yes (race latent, swallowed by bash)
2022-12 → this PR	Same as above	bash ≥ 5.2 patch 14	Frequently lost (~34% in harness; higher under load / headless)
This PR onwards	SIGHUP, wait for shell to exit (≤ 500 ms, os.waitid + WNOWAIT), then terminalbox.remove(vte) + del	any	Yes

The fix restores the pre-2017 invariant ("shell gets time to flush before its PTY dies") without reintroducing the leak c4d372c5 solved: the wait happens between SIGHUP and the widget teardown, so the widget is still released synchronously inside close() — just a few tens of milliseconds later, after the shell has finished its work. It also closes the race in a way that does not depend on bash's behavior: it works regardless of which bash version the user is running.

Fix

Briefly wait for the shell to actually exit before destroying the VTE widget. The wait is bounded (500 ms cap, 20 ms poll interval) so a hung shell can't freeze the UI.

def close(self):
    """Close ourselves"""
    dbg('close: called')
    self.cnxids.remove_widget(self.vte)
    self.emit('close-term')
    if self.pid is not None:
        try:
            dbg('close: killing %d' % self.pid)
            os.kill(self.pid, signal.SIGHUP)
        except Exception as ex:
            # We really don't want to care if this failed. Deep OS voodoo is
            # not what we should be doing.
            dbg('os.kill failed: %s' % ex)
            pass
        # Wait for the shell to finish exiting before we tear down the
        # PTY. This is NOT a politeness hedge — removing it resurrects a
        # real data-loss bug. The mechanism:
        #
        #   1. We've just sent SIGHUP to the shell.
        #   2. If we destroy the VTE now, the PTY master closes and the
        #      kernel delivers a SECOND SIGHUP via tty hangup to the
        #      slave's foreground process group.
        #   3. On bash 5.2 patch 14 and later (commit 6647917a, Dec 2022
        #      — "process additional terminating signals when running
        #      the EXIT trap after a terminating signal"), a second
        #      SIGHUP arriving while termsig_handler is mid-flight
        #      routes through the new kill_shell() path and terminates
        #      bash immediately, before maybe_save_shell_history() can
        #      finish writing $HISTFILE. Result: silent history
        #      loss on every window close.
        #
        # Waiting until the shell has fully exited ensures the kernel's
        # post-PTY-close SIGHUP has no recipient still trying to flush.
        # The timeout caps the wait so a shell that ignores SIGHUP can't
        # hang the window close; bash's own flush completes in well
        # under 50 ms in practice.
        self._wait_for_shell_exit()

    if self.vte:
        self.terminalbox.remove(self.vte)
        del(self.vte)

def _wait_for_shell_exit(self, timeout=0.5, poll_interval=0.02):
    """Wait briefly for the spawned shell to exit before we tear
    down the PTY. Required to avoid silent bash history loss
    caused by bash 5.2 patch 14 reacting to the second SIGHUP that
    the kernel delivers when we close the PTY master; see the
    caller in Terminal.close() for the full mechanism.

    WNOWAIT leaves the zombie in place so VTE's GLib child-watch
    can still reap it and fire child-exited normally — we are only
    observing the exit state here, not consuming it.
    """
    deadline = time.monotonic() + timeout
    flags = os.WEXITED | os.WNOWAIT | os.WNOHANG
    while time.monotonic() < deadline:
        try:
            if os.waitid(os.P_PID, self.pid, flags):
                return
        except OSError:
            return
        time.sleep(poll_interval)

Why os.waitid with WNOWAIT

VTE installs its own GLib child watch on the shell PID, which internally relies on SIGCHLD + waitpid to reap and report the exit status via the child-exited signal. If close() called plain os.waitpid(), we would reap the child first; VTE's watch would then get ECHILD and its bookkeeping would diverge.

os.waitid(P_PID, pid, WEXITED | WNOWAIT | WNOHANG):

• WEXITED — match a process that has terminated.
• WNOHANG — return immediately when the child is still running, so we can poll without blocking the GTK main loop indefinitely.
• WNOWAIT — do not reap the child. It stays as a zombie so VTE's child watch can collect it normally.

This lets us detect "the shell has finished" without taking the status away from VTE.

Behavior after the fix

• Common case (bash exits quickly): the loop exits after one or two poll cycles — a ~20–40 ms delay on window close, imperceptible to the user. $HISTFILE is written reliably.
• Slow shell: bounded at 500 ms total wait, then teardown proceeds as before. Worst case is identical to the prior behavior.
• Already-reaped child (e.g., exit_action=close triggered by the shell exiting on its own): os.waitid raises OSError (ECHILD) and we return immediately.

Reproducing the bug

The race is intermittent in normal operation, which makes ad-hoc reproduction frustrating: a single window close usually saves history fine. Two things were needed to drive reproduction:

1. A debug shim added temporarily to Terminal.close() that drives the close path into the pre-fix behavior (and optionally guarantees loss with SIGKILL for a 100%-deterministic check).
2. An automated harness that spawns Terminator instances, seeds a unique marker into the shell's in-memory history, and closes each window via wmctrl (the same code path as clicking [X]).

The debug shim (development-only, not shipped)

The shim is gated on an env var so it stays dormant unless explicitly enabled. It is not in the shipping patch — it was inserted into a local copy of terminatorlib/terminal.py during development to drive the reproduction script. To re-run the reproduction below, paste the shim back into Terminal.close() (replacing the SIGHUP block) for the duration of testing:

if self.pid is not None:
    debug_race = os.environ.get('TERMINATOR_DEBUG_FORCE_HISTORY_RACE')
    if debug_race:
        # Drive Terminal.close() into the pre-fix code path on demand.
        # Skip the post-SIGHUP wait so PTY teardown immediately follows
        # the kill — exactly like the buggy original.
        #
        # Env var value selects the signal:
        #   'hup' / 'sighup' / '1'   -> SIGHUP (default). Real-bug
        #                               fidelity; loss is INTERMITTENT.
        #   'kill' / 'sigkill' / '9' -> SIGKILL. Bash can't catch it,
        #                               so loss is DETERMINISTIC.
        sig = signal.SIGHUP
        if debug_race.lower() in ('kill', 'sigkill', '9'):
            sig = signal.SIGKILL
        try:
            os.kill(self.pid, sig)
        except Exception as ex:
            dbg('debug kill failed: %s' % ex)
    else:
        try:
            os.kill(self.pid, signal.SIGHUP)
        except Exception as ex:
            dbg('os.kill failed: %s' % ex)
        self._wait_for_shell_exit()

Why the shim was needed:

• hup mode reproduces the bug exactly: same signal, same lack of wait. But because the race is real, loss is intermittent — typically a handful out of 50 closes on a modern machine.
• kill mode sends SIGKILL instead. Bash can't install a handler for it, so the EXIT trap and maybe_save_shell_history() are never reached. Loss is 100% deterministic, which is the right mode for binary "fix works / doesn't" CI checks.
• off mode (env var absent) takes the shipping path and is used to confirm the fix actually fixes the symptom.

The reproduction harness

The harness used to drive the numbers in the Results table below is not committed as a tracked file in this PR; it is documented inline so anyone reading this can paste it into a local script and reproduce the same measurements. Save the script below as /tmp/repro.sh, chmod +x, and invoke it from the repo root.

#!/usr/bin/env bash
#
# Reproduce / regression-test the SIGHUP-vs-PTY-teardown race in
# Terminator.close() that silently drops bash history entries (the file
# bash writes via $HISTFILE) when a window is closed.
#
# Each iteration:
#   1. Launch a fresh Terminator (-u = no D-Bus single-instance routing)
#      under TERMINATOR_DEBUG_FORCE_HISTORY_RACE so the close path can
#      be driven into the pre-fix code path on demand.
#   2. The shell uses a controlled rcfile: clean HISTFILE, no
#      PROMPT_COMMAND='history -a' (which would defeat the test by
#      flushing per-command), and a unique marker pre-seeded into
#      in-memory history via `history -s`.
#   3. Wait for the window to appear, then close it with `wmctrl -c`
#      (sends WM_DELETE_WINDOW — same as clicking [X]).
#   4. The marker should land in HISTFILE iff bash's SIGHUP handler
#      managed to flush before the PTY was torn down.
#
# Modes:
#   DEBUG_MODE=hup   intermittent loss (matches real bug exactly)
#   DEBUG_MODE=kill  deterministic 100% loss
#   DEBUG_MODE=off   verifies the fix (should report 0 lost)
#
# NOTE: The 'hup' and 'kill' modes require the debug shim documented
# above to be temporarily added back to terminatorlib/terminal.py
# Terminal.close(). It is intentionally NOT in the shipping code. With
# the shim absent, all modes behave like 'off'.

set -u

ITERATIONS=${1:-50}
DEBUG_MODE=${DEBUG_MODE:-hup}

REPRO_DIR=$(mktemp -d -t terminator-histrepro.XXXXXX)
HISTFILE=$REPRO_DIR/hist
RCFILE=$REPRO_DIR/bashrc
LOGFILE=$REPRO_DIR/log

cleanup() {
    rm -rf "$REPRO_DIR"
}
trap cleanup EXIT

if ! command -v wmctrl >/dev/null; then
    echo "error: wmctrl is required (sudo apt install wmctrl)" >&2
    exit 2
fi

# Launch the in-tree terminator (with the debug shim re-applied), not
# the system one (which won't have the shim wired up). Set TERMINATOR_BIN
# to override; defaults to ./terminator in the current working directory.
TERMINATOR_BIN=${TERMINATOR_BIN:-$(pwd)/terminator}

if [ ! -x "$TERMINATOR_BIN" ]; then
    echo "error: $TERMINATOR_BIN not found or not executable" >&2
    exit 2
fi

# Kill any stale instances so -u definitely gives us a fresh process.
pkill -f "$TERMINATOR_BIN" 2>/dev/null || true
sleep 0.3

echo "Repro dir: $REPRO_DIR"
echo "Mode:      DEBUG_MODE=$DEBUG_MODE"
echo "Running $ITERATIONS iterations..."
echo

env_args=()
case "$DEBUG_MODE" in
    off|"") ;;
    *) env_args=(env "TERMINATOR_DEBUG_FORCE_HISTORY_RACE=$DEBUG_MODE") ;;
esac

EXPECTED=0
for i in $(seq 1 "$ITERATIONS"); do
    MARKER="marker-$i-$$-$RANDOM"
    TITLE="histrepro-$i-$$"

    cat > "$RCFILE" <<EOF
HISTFILE=$HISTFILE
HISTSIZE=10000
HISTFILESIZE=10000
HISTCONTROL=
shopt -s histappend
unset PROMPT_COMMAND
history -c
# Pre-load the marker into in-memory history so we don't need to type
# anything. If bash's SIGHUP-time flush works, it lands in \$HISTFILE.
history -s "echo $MARKER"
PS1='\$ '
EOF

    "${env_args[@]}" "$TERMINATOR_BIN" -u -T "$TITLE" \
        -x bash --rcfile "$RCFILE" -i \
        >>"$LOGFILE" 2>&1 &
    TPID=$!

    # Wait for the window to actually exist.
    for _ in $(seq 1 30); do
        if wmctrl -l 2>/dev/null | grep -qF " $TITLE"; then
            break
        fi
        sleep 0.1
    done

    # Tiny settle so the rcfile has definitely been read.
    sleep 0.15

    # Close the window — this is what trips the bug.
    if ! wmctrl -c "$TITLE" 2>/dev/null; then
        echo "  [$i] WARN: window '$TITLE' never appeared" >&2
    fi

    EXPECTED=$((EXPECTED + 1))
    sleep 0.25
    wait "$TPID" 2>/dev/null || true
done

ACTUAL=0
if [ -f "$HISTFILE" ]; then
    ACTUAL=$(grep -c "^echo marker-" "$HISTFILE" 2>/dev/null || true)
fi
LOST=$((EXPECTED - ACTUAL))

echo
echo "===================================="
echo "Expected markers: $EXPECTED"
echo "Found in HISTFILE: $ACTUAL"
echo "Lost:              $LOST"
echo "===================================="
if [ "$LOST" -gt 0 ]; then
    echo "REPRODUCED: $LOST markers were dropped."
    exit 1
else
    echo "No loss observed."
    exit 0
fi

Each iteration:

1. Writes an rcfile with a clean HISTFILE, unset PROMPT_COMMAND (so distros that flush per-command via history -a can't mask the test), and a unique marker-N-pid-rand pre-seeded into in-memory history with history -s.
2. Launches ./terminator -u … (the in-tree binary, so the shim is actually loaded; -u bypasses D-Bus single-instance routing which would silently drop the env var).
3. Polls wmctrl -l until the new window appears, then calls wmctrl -c <title> — this sends WM_DELETE_WINDOW, the same event clicking [X] produces and the only path that triggers Terminal.close() (kill -HUP $PPID and friends route through the unrelated exit_action handler instead).
4. After the loop, counts how many markers ended up in HISTFILE versus how many were attempted. The exit code is non-zero if any were dropped.

Usage (from the repo root, after pasting the script into /tmp/repro.sh and chmod +x):

/tmp/repro.sh 50               # default: DEBUG_MODE=hup
DEBUG_MODE=kill /tmp/repro.sh 50
DEBUG_MODE=off  /tmp/repro.sh 50

Avoidance traps the harness handles automatically:

• D-Bus single-instance routing (pkill + -u).
• PROMPT_COMMAND='history -a' (overridden in the rcfile).
• Distinguishing the buggy path (Terminal.close() on WM_DELETE) from the unrelated exit-action path (bash exits voluntarily).
• Using the in-tree ./terminator rather than the system binary, so the debug shim is actually loaded.

Results (50 iterations each)

With the debug shim temporarily reinstated for the hup and kill rows:

Mode	Expected	Found in HISTFILE	Lost	Outcome
hup	50	33	17	Real-bug fidelity: intermittent loss confirmed.
kill	50	0	50	Deterministic-loss control: confirms the harness can observe loss.
off	50	50	0	Shipping path with the fix: zero loss across 50 closes.

Raw output:

$ /tmp/repro.sh 50
Mode:      DEBUG_MODE=hup
Expected markers: 50
Found in HISTFILE: 33
Lost:              17
REPRODUCED: 17 markers were dropped.

$ DEBUG_MODE=kill /tmp/repro.sh 50
Mode:      DEBUG_MODE=kill
Expected markers: 50
Found in HISTFILE: 0
Lost:              50
REPRODUCED: 50 markers were dropped.

$ DEBUG_MODE=off /tmp/repro.sh 50
Mode:      DEBUG_MODE=off
Expected markers: 50
Found in HISTFILE: 50
Lost:              0
No loss observed.

The hup row demonstrates that the bug is real and reproducible in the pre-fix code path. The kill row is a sanity check on the harness itself (if kill didn't lose anything, the test would be broken). The off row is the proof that the fix closes the race: across 50 window closes through the shipping Terminal.close(), every single marker landed in HISTFILE.

[mattrose]

To any future submitters. Please don't feel you need to write a 4000 word description for a 10-line change.