fix: take the signal-time autosave on the game goroutine (closes #24)

The SIGHUP/SIGTERM handler gob-encoded the live game tree from the signal
goroutine while the game goroutine was mid-turn mutating it, and AutoSave
removed the save file before encoding — so the failure mode was not a
stale save but a deleted one followed by a possibly torn replacement,
with a window in which the player had neither. The suite has run under
-race since 2026-08-09 and was green because nothing had ever driven the
turn loop concurrently with a signal: evidence of untested, not of safe.

The handler no longer writes anything. AutoSaveOnSignal posts a request,
wakes the input read, and waits up to signalSaveTimeout for the game
goroutine to take it; the encode runs on the goroutine that owns the
state, at the three points where that goroutine can sit: between turns
(command), on waking from a blocked readchar, and while parked in the `!`
shell escape (runShellEscape, which now runs the shell on a helper
goroutine so a hangup during it still rescues the game).

Blocked on input is the case that matters — a dropped connection lands
while the player is thinking, so a flag checked only between turns would
never be looked at. Terminal.ReadChar therefore returns (byte, bool),
with ok false meaning "woken by Interrupt, no key", and term.Tcell posts
a tcell.EventInterrupt onto tcell's own event queue to unpark PollEvent.
readchar services the request and reads again, so no caller sees it.

saveFile writes a temporary file in the save's own directory, fsyncs it
and renames it over the target instead of truncating in place, so a save
that fails — or never happens because the deadline ran out — leaves the
player's previous save whole.

The SIGINT/SIGQUIT no-save decision and the single-signal-read ordering
guarantee are untouched. pendingSaver reads the game out from under its
mutex rather than delegating with it held, because the delegated call now
blocks until the save is taken.

cmd/rogue/main.go

@@ -115,11 +115,32 @@ func loadParams() game.Params {
 // saver is the autosave half of *game.RogueGame that the signal handler
 // needs; an interface so the handler is testable headlessly.
 type saver interface {
-	// AutoSave writes the game to its save file, best effort (save.c
-	// auto_save).
-	AutoSave()
+	// AutoSaveOnSignal asks the game goroutine to write the save file and
+	// waits up to timeout for it, reporting whether the save ran (save.c
+	// auto_save). The handler never encodes anything itself; see
+	// signalSaveTimeout.
+	AutoSaveOnSignal(timeout time.Duration) bool
 }
 
+// signalSaveTimeout bounds how long the signal handler waits for the game
+// goroutine to take its autosave.
+//
+// The handler cannot encode the game itself — that was issue #24's data
+// race — so it has to hand the work to the goroutine that owns the state
+// and wait. The game answers between turns, while parked waiting for a
+// key, and while parked in the shell escape, which covers everywhere it
+// can sit for any length of time; the deadline is the backstop for a game
+// goroutine wedged somewhere with no service point, so that a signal can
+// never fail to get the process out. It is generous next to the
+// milliseconds a gob encode of one game takes, and invisible to a player
+// whose connection has already dropped.
+//
+// Giving up costs nothing now that saveFile renames over the target
+// (game/save.go): a save that does not happen leaves the previous save
+// whole, where the old remove-then-encode could leave the player with
+// neither.
+const signalSaveTimeout = 3 * time.Second
+
 // finisher is the terminal-restoring half of game.Terminal that the
 // signal handler needs (curses endwin).
 type finisher interface {
@@ -130,26 +151,35 @@ type finisher interface {
 // pendingSaver is the saver the signal handler holds from the moment the
 // terminal goes raw. The handler has to be armed before there is a game
 // to save — restoring a save file and the death demo both run with the
-// tty already raw — so AutoSave does nothing until set hands over the
-// real game. The mutex is not decoration: set runs on the main goroutine
-// and AutoSave on the signal goroutine.
+// tty already raw — so AutoSaveOnSignal does nothing until set hands over
+// the real game. The mutex is not decoration: set runs on the main
+// goroutine and AutoSaveOnSignal on the signal goroutine.
 type pendingSaver struct {
 	mu   sync.Mutex
 	game saver
 }
 
-// AutoSave saves the game if there is one yet, and otherwise does
+// AutoSaveOnSignal saves the game if there is one yet, and otherwise does
 // nothing: a signal arriving before the game is built still restores the
 // terminal, which is the part that matters.
-func (p *pendingSaver) AutoSave() {
+//
+// The lock is held only long enough to read the game, not across the
+// delegated save. That changed with issue #24: the real
+// AutoSaveOnSignal now blocks until the game goroutine takes the save or
+// the deadline expires, and holding the mutex across a wait that long
+// would stall a concurrent set — the case the PR #23 review flagged as
+// safe only for as long as set is called exactly once. This shape does
+// not depend on that.
+func (p *pendingSaver) AutoSaveOnSignal(timeout time.Duration) bool {
 	p.mu.Lock()
-	defer p.mu.Unlock()
+	g := p.game
+	p.mu.Unlock()
 
-	if p.game == nil {
-		return
+	if g == nil {
+		return false
 	}
 
-	p.game.AutoSave()
+	return g.AutoSaveOnSignal(timeout)
 }
 
 // set hands the signal handler the game to autosave, once one exists.
@@ -196,12 +226,16 @@ func handledSignals() []os.Signal {
 // would turn it into a one-keystroke undo for a bad turn: a gameplay
 // change, not a robustness fix.
 //
-// Safety: this runs on a goroutine while the main goroutine is mid-turn
-// mutating game state, and AutoSave removes the save file before
-// gob-encoding that live state. On HUP/TERM that risk is accepted
-// because the process is about to die regardless and a best-effort save
-// beats none. On INT/QUIT there is nothing to rescue, so the right
-// choice is the one with no corruption window at all.
+// Safety: this used to be the third ground, back when the handler
+// gob-encoded live game state from its own goroutine after removing the
+// save file — a data race with a window in which the player had no save
+// at all, accepted on HUP/TERM because the process was dying anyway and
+// avoided entirely on INT/QUIT. Issue #24 removed the window instead of
+// living with it: the handler now hands the save to the game goroutine
+// and waits (AutoSaveOnSignal), and the write goes to a temporary file
+// renamed over the target. The split above stands on C and on semantics,
+// which is where it always belonged; INT and QUIT do not save because
+// the player asked to stop, not because saving is dangerous.
 func savesOnSignal(sig os.Signal) bool {
 	return sig == syscall.SIGHUP || sig == syscall.SIGTERM
 }
@@ -239,14 +273,19 @@ func notifySignals() chan os.Signal {
 // leaveOnSignal waits for one signal and takes the game out.
 //
 // Exactly one goroutine reads exactly one signal, which is what makes
-// the exit safe: a second signal (a SIGINT landing while a SIGHUP's
-// AutoSave is still writing, say) stays in the buffer unread and can
-// never call exit out from under an in-flight save. The order within is
-// the same one myExit uses (game/rip.go): save if this signal saves,
-// then restore the terminal, then exit.
+// the exit safe: a second signal (a SIGINT landing while a SIGHUP's save
+// is still being written, say) stays in the buffer unread and can never
+// call exit out from under an in-flight save. The order within is the
+// same one myExit uses (game/rip.go): save if this signal saves, then
+// restore the terminal, then exit.
+//
+// AutoSaveOnSignal returns once the game goroutine has finished writing,
+// or once signalSaveTimeout has run out, so the save is complete before
+// the terminal is torn down and the process leaves — and the process
+// leaves either way.
 func leaveOnSignal(sig <-chan os.Signal, g saver, t finisher, exit func(int)) {
 	if savesOnSignal(<-sig) {
-		g.AutoSave()
+		g.AutoSaveOnSignal(signalSaveTimeout)
 	}
 
 	t.Fini()

cmd/rogue/main_test.go

@@ -11,6 +11,7 @@ import (
 	"sync"
 	"syscall"
 	"testing"
+	"time"
 )
 
 // The steps the signal handler can take, in the order signalRecorder
@@ -61,9 +62,13 @@ func newSignalRecorder() *signalRecorder {
 	return &signalRecorder{done: make(chan struct{})}
 }
 
-// AutoSave records a save attempt (the saver half).
-func (r *signalRecorder) AutoSave() {
+// AutoSaveOnSignal records a save attempt (the saver half). The real one
+// hands the work to the game goroutine and waits; the recorder stands in
+// for a game that takes it immediately.
+func (r *signalRecorder) AutoSaveOnSignal(time.Duration) bool {
 	r.record(stepSave)
+
+	return true
 }
 
 // Fini records a terminal restore (the finisher half).
@@ -231,11 +236,12 @@ type blockingSaver struct {
 	extra os.Signal
 }
 
-// AutoSave delivers the extra signal mid-save, then records the save.
-func (b *blockingSaver) AutoSave() {
+// AutoSaveOnSignal delivers the extra signal mid-save, then records the
+// save.
+func (b *blockingSaver) AutoSaveOnSignal(timeout time.Duration) bool {
 	b.queue <- b.extra
 
-	b.rec.AutoSave()
+	return b.rec.AutoSaveOnSignal(timeout)
 }
 
 // TestLeaveOnRealSignal is the deepest headless check available: it
@@ -314,10 +320,63 @@ func TestPendingSaverArmsBeforeTheGameExists(t *testing.T) {
 	// Once the game is handed over, the same saver writes it.
 	started := newSignalRecorder()
 	pending.set(started)
-	pending.AutoSave()
+
+	if !pending.AutoSaveOnSignal(signalSaveTimeout) {
+		t.Error("after set: the save was not reported as taken")
+	}
 
 	saved, _ := started.taken()
 	if want := []string{stepSave}; !slices.Equal(saved, want) {
 		t.Errorf("after set: steps = %v, want %v", saved, want)
 	}
 }
+
+// TestPendingSaverDoesNotHoldItsLockAcrossTheSave pins the reason
+// pendingSaver reads the game out from under the mutex instead of
+// delegating with it held: since issue #24 the delegated save blocks
+// until the game goroutine takes it or the deadline expires, so a mutex
+// held across it would stall whoever calls set. Nothing calls set twice
+// today, which is why the PR #23 review recorded this as a future-proof
+// note rather than a bug — this test is what stops it becoming one.
+func TestPendingSaverDoesNotHoldItsLockAcrossTheSave(t *testing.T) {
+	t.Parallel()
+
+	pending := &pendingSaver{}
+	stuck := &stuckSaver{entered: make(chan struct{}), release: make(chan struct{})}
+	pending.set(stuck)
+
+	go pending.AutoSaveOnSignal(signalSaveTimeout)
+
+	<-stuck.entered // the delegated save is in flight
+
+	done := make(chan struct{})
+
+	go func() {
+		defer close(done)
+
+		pending.set(newSignalRecorder()) // must not block on the save
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(time.Second):
+		t.Error("set blocked while a save was in flight: the lock is held across it")
+	}
+
+	close(stuck.release)
+}
+
+// stuckSaver blocks inside the delegated save until it is released,
+// standing in for a game goroutine that is slow to answer.
+type stuckSaver struct {
+	entered chan struct{}
+	release chan struct{}
+}
+
+// AutoSaveOnSignal blocks until the test releases it.
+func (s *stuckSaver) AutoSaveOnSignal(time.Duration) bool {
+	close(s.entered)
+	<-s.release
+
+	return true
+}