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. Running the shell on a helper goroutine would also have moved term.Tcell.ShellEscape's panic on a failed Screen.Resume onto it, and a panic at the top of any goroutine terminates the process without running the deferred calls of the others — including cmd/rogue/main.go's `defer t.Fini()`. The tty would have been left raw on precisely the path where the terminal is already broken, which is issue #12's failure on a path this change created. runShellEscape therefore recovers the helper's panic and re-raises it on the game goroutine, whose stack has the restore in it, so "every path restores the terminal via Terminal.Fini before exiting" stays true. 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. What the handoff guarantees is stated exactly rather than flatteringly: the encode runs on the state-owning goroutine, so the snapshot is internally consistent and restorable, but it is not necessarily taken between commands. readchar is reached from mid-command prompts (--More--, askOverwrite, getStr, the direction and pack prompts) and the command has already mutated state by then, so a save taken there freezes that command half applied and restoring loses the rest of it. 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.
This commit is contained in:
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user