fix: restore the terminal on SIGINT/SIGQUIT (closes #12)
The port installed handlers for SIGHUP and SIGTERM only, so SIGINT and SIGQUIT killed the process with tcell still holding the tty and dropped the user into a shell with no echo and a scrambled screen. All four signals now go to one os/signal channel read by one goroutine, and every path calls Terminal.Fini before os.Exit(0) -- C's leave(), "leave quickly but curteously" (main.c). The save decision, written into the savesOnSignal comment: SIGHUP and SIGTERM keep autosaving; SIGINT and SIGQUIT restore and exit without saving. No path in C saves on INT or QUIT (leave() is endwin-and-exit, quit() confirms/scores/exits, endit() goes through fatal(), and save.c auto_save is reserved for HUP/TERM), the semantics agree (involuntary teardown is worth rescuing a game from; a deliberate "stop now" must not become a one-keystroke checkpoint against an anti-save-scum save discipline), and it is the safe choice, since AutoSave gob-encodes live state the main goroutine is still mutating after removing the old file. One reader of one signal is also what closes the corruption window: a second signal arriving while a SIGHUP's AutoSave is mid-write stays unread in the buffer instead of exiting out from under the writer. cmd/rogue/main_test.go covers the ordering per signal, the save/no-save split, the mid-save second-signal interleaving, and real SIGINT/SIGQUIT/SIGHUP/SIGTERM delivered to the test process through the same notifySignals wiring the game uses. Two premises behind the report were wrong and are recorded rather than silently fixed: leave() is not installed on SIGINT/SIGQUIT during play (the wiring is in mdport.c; the shipped build calls md_onsignal_default and installs nothing, and leave() appears only in the endgame paths of rip.c and main.c), and Ctrl-C never generated SIGINT here anyway, since tcell's raw mode clears ISIG and the key arrives as byte 0x03 -- as it did in C, whose setup() calls curses raw(). The real exposure is kill -INT / kill -QUIT and the window before term.New(). ARCHITECTURE.md section 9 gains rows for SIGTSTP/tstp() (deliberately dropped: raw mode means Ctrl-Z cannot reach the process, suspending the screen from the signal goroutine would race the drawing goroutine, and C armed tstp only after a successful restore(); the ! shell escape covers the need), for SIGINT not routing to the interactive quit() prompt, and for auto_save on the fault signals. Section 5.3's claim that tcell handles SIGTSTP was false -- tcell registers only SIGWINCH -- and is corrected.
This commit is contained in:
222
cmd/rogue/main_test.go
Normal file
222
cmd/rogue/main_test.go
Normal file
@@ -0,0 +1,222 @@
|
||||
package main
|
||||
|
||||
// White-box tests for the signal plumbing. Unlike the game package's test
|
||||
// files this one carries no //nolint:testpackage directive: testpackage
|
||||
// exempts package main, so nolintlint rejects the directive as unused.
|
||||
|
||||
import (
|
||||
"os"
|
||||
"os/signal"
|
||||
"slices"
|
||||
"strings"
|
||||
"sync"
|
||||
"syscall"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// signalRecorder stands in for the game and the terminal in the signal
|
||||
// handler, recording the order of the steps the handler takes. The mutex
|
||||
// matters: the handler runs on its own goroutine, so an unguarded slice
|
||||
// would be a data race under -race, which is exactly what these tests
|
||||
// are meant to rule out.
|
||||
type signalRecorder struct {
|
||||
mu sync.Mutex
|
||||
steps []string
|
||||
code int
|
||||
done chan struct{}
|
||||
}
|
||||
|
||||
func newSignalRecorder() *signalRecorder {
|
||||
return &signalRecorder{done: make(chan struct{})}
|
||||
}
|
||||
|
||||
// AutoSave records a save attempt (the saver half).
|
||||
func (r *signalRecorder) AutoSave() {
|
||||
r.record("save")
|
||||
}
|
||||
|
||||
// Fini records a terminal restore (the finisher half).
|
||||
func (r *signalRecorder) Fini() {
|
||||
r.record("fini")
|
||||
}
|
||||
|
||||
// exit records the process exit that ends the handler and releases any
|
||||
// waiter. It stands in for os.Exit, which cannot be called in a test.
|
||||
func (r *signalRecorder) exit(code int) {
|
||||
r.mu.Lock()
|
||||
r.code = code
|
||||
r.steps = append(r.steps, "exit")
|
||||
r.mu.Unlock()
|
||||
|
||||
close(r.done)
|
||||
}
|
||||
|
||||
// record appends one step.
|
||||
func (r *signalRecorder) record(step string) {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
|
||||
r.steps = append(r.steps, step)
|
||||
}
|
||||
|
||||
// taken returns the recorded steps and the exit code.
|
||||
func (r *signalRecorder) taken() ([]string, int) {
|
||||
r.mu.Lock()
|
||||
defer r.mu.Unlock()
|
||||
|
||||
return slices.Clone(r.steps), r.code
|
||||
}
|
||||
|
||||
// TestLeaveOnSignalRestoresTerminalBeforeExit is the core of issue #12:
|
||||
// whatever the signal, the terminal is restored before the process ends,
|
||||
// so the player is never dropped into a shell with the tty still in raw
|
||||
// mode.
|
||||
func TestLeaveOnSignalRestoresTerminalBeforeExit(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
for _, sig := range handledSignals() {
|
||||
rec := newSignalRecorder()
|
||||
|
||||
ch := make(chan os.Signal, 1)
|
||||
ch <- sig
|
||||
|
||||
leaveOnSignal(ch, rec, rec, rec.exit)
|
||||
|
||||
steps, code := rec.taken()
|
||||
|
||||
fini := slices.Index(steps, "fini")
|
||||
exit := slices.Index(steps, "exit")
|
||||
|
||||
if fini < 0 || exit < 0 || fini > exit {
|
||||
t.Errorf("%v: want the terminal restored before exit, got %v",
|
||||
sig, steps)
|
||||
}
|
||||
|
||||
if code != 0 {
|
||||
t.Errorf("%v: exit code = %d, want 0", sig, code)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestLeaveOnSignalSaveSplit pins the decision recorded on savesOnSignal:
|
||||
// SIGHUP/SIGTERM (involuntary teardown) save on the way out, SIGINT and
|
||||
// SIGQUIT (a deliberate "stop now" from the player) do not, matching C,
|
||||
// where auto_save is reserved for HUP/TERM and neither leave() nor quit()
|
||||
// nor endit() writes a save file.
|
||||
func TestLeaveOnSignalSaveSplit(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
want := map[os.Signal][]string{
|
||||
syscall.SIGHUP: {"save", "fini", "exit"},
|
||||
syscall.SIGTERM: {"save", "fini", "exit"},
|
||||
syscall.SIGINT: {"fini", "exit"},
|
||||
syscall.SIGQUIT: {"fini", "exit"},
|
||||
}
|
||||
|
||||
for _, sig := range handledSignals() {
|
||||
rec := newSignalRecorder()
|
||||
|
||||
ch := make(chan os.Signal, 1)
|
||||
ch <- sig
|
||||
|
||||
leaveOnSignal(ch, rec, rec, rec.exit)
|
||||
|
||||
steps, _ := rec.taken()
|
||||
if !slices.Equal(steps, want[sig]) {
|
||||
t.Errorf("%v: steps = %v, want %v", sig, steps, want[sig])
|
||||
}
|
||||
|
||||
if saved := slices.Contains(steps, "save"); saved != savesOnSignal(sig) {
|
||||
t.Errorf("%v: saved = %v, savesOnSignal = %v",
|
||||
sig, saved, savesOnSignal(sig))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestLeaveOnSignalIgnoresLaterSignals covers the ordering guarantee in
|
||||
// leaveOnSignal's comment: only the first signal is read, so a second one
|
||||
// arriving mid-save cannot exit out from under the save and truncate the
|
||||
// player's file. The saver here blocks until a second signal has been
|
||||
// queued, reproducing that window.
|
||||
func TestLeaveOnSignalIgnoresLaterSignals(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
rec := newSignalRecorder()
|
||||
|
||||
ch := make(chan os.Signal, 2)
|
||||
ch <- syscall.SIGHUP
|
||||
|
||||
blocker := &blockingSaver{rec: rec, queue: ch, extra: syscall.SIGINT}
|
||||
|
||||
leaveOnSignal(ch, blocker, rec, rec.exit)
|
||||
|
||||
steps, _ := rec.taken()
|
||||
if !slices.Equal(steps, []string{"save", "fini", "exit"}) {
|
||||
t.Errorf("steps = %v, want one save, one fini, one exit", steps)
|
||||
}
|
||||
|
||||
if len(ch) != 1 {
|
||||
t.Errorf("queued signals left unread = %d, want 1", len(ch))
|
||||
}
|
||||
}
|
||||
|
||||
// blockingSaver queues another signal while the save is in flight, the
|
||||
// race window leaveOnSignal is built to close.
|
||||
type blockingSaver struct {
|
||||
rec *signalRecorder
|
||||
queue chan os.Signal
|
||||
extra os.Signal
|
||||
}
|
||||
|
||||
// AutoSave delivers the extra signal mid-save, then records the save.
|
||||
func (b *blockingSaver) AutoSave() {
|
||||
b.queue <- b.extra
|
||||
|
||||
b.rec.AutoSave()
|
||||
}
|
||||
|
||||
// TestLeaveOnRealSignal is the deepest headless check available: it
|
||||
// delivers real SIGINT/SIGQUIT/SIGHUP/SIGTERM to this process through
|
||||
// os/signal, exactly as notifySignals wires them in the game, and
|
||||
// verifies each one reaches the handler and restores the terminal before
|
||||
// exiting.
|
||||
//
|
||||
// What cannot be checked here is the tty itself coming back out of raw
|
||||
// mode: that needs a controlling terminal and a live tcell screen, which
|
||||
// a headless test run does not have. This test covers everything up to
|
||||
// the Terminal.Fini call; term.Tcell.Fini is a direct pass-through to
|
||||
// tcell's Screen.Fini, which is the same call myExit already relies on.
|
||||
func TestLeaveOnRealSignal(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ch := notifySignals()
|
||||
defer signal.Stop(ch)
|
||||
|
||||
for _, sig := range handledSignals() {
|
||||
rec := newSignalRecorder()
|
||||
|
||||
go leaveOnSignal(ch, rec, rec, rec.exit)
|
||||
|
||||
unix, ok := sig.(syscall.Signal)
|
||||
if !ok {
|
||||
t.Fatalf("%v is not a unix signal", sig)
|
||||
}
|
||||
|
||||
err := syscall.Kill(os.Getpid(), unix)
|
||||
if err != nil {
|
||||
t.Fatalf("kill(%v): %v", sig, err)
|
||||
}
|
||||
|
||||
<-rec.done
|
||||
|
||||
steps, code := rec.taken()
|
||||
if !strings.HasSuffix(strings.Join(steps, ","), "fini,exit") {
|
||||
t.Errorf("%v: steps = %v, want the terminal restored before exit",
|
||||
sig, steps)
|
||||
}
|
||||
|
||||
if code != 0 {
|
||||
t.Errorf("%v: exit code = %d, want 0", sig, code)
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user