diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index e864669..7fc34d4 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1484,9 +1484,11 @@ needed function pointers for — so it is also what the save format stores. // Terminal is the physical device, behind an interface so tests run headless. // The real game uses term.Tcell; tests use a scripted testTerm. type Terminal interface { - Render(w *Window) // blit a window to the device - ReadChar() byte // event loop → C char codes (arrows→hjkl, ^C→quit) - Fini() // restore the device (curses endwin) on exit + Render(w *Window) // blit a window to the device + ReadChar() (byte, bool) // event loop → C char codes (arrows→hjkl, ^C→quit); + // ok is false when Interrupt woke the read + Interrupt() // wake a blocked ReadChar (signal goroutine only) + Fini() // restore the device (curses endwin) on exit } // Screen exposes the curses vocabulary the ported code speaks, over a Terminal. @@ -1518,12 +1520,46 @@ SIGWINCH — SIGTSTP is not among the signals it takes, and is dropped (§9). Signals are handled in `cmd/rogue/main.go`: one `os/signal` channel read by one goroutine that reads exactly one signal, so a second signal can never call -`os.Exit` out from under an in-flight save. SIGHUP and SIGTERM `AutoSave` on the +`os.Exit` out from under an in-flight save. SIGHUP and SIGTERM autosave on the way out (save.c `auto_save`); SIGINT and SIGQUIT restore the terminal and exit without saving, matching C — where `auto_save` is reserved for HUP/TERM and neither `leave()` nor `quit()` nor `endit()` writes a save file — and keeping a deliberate interrupt from becoming a free checkpoint. +The signal goroutine does not write the save itself. It calls +`RogueGame.AutoSaveOnSignal`, which posts a request, wakes the input read +through `Terminal.Interrupt`, and waits for the game goroutine to take it or for +a deadline to run out; the encode happens on the game goroutine, the only one +that touches game state. It answers between turns (`command`), on waking from a +blocked `readchar`, and while parked in the `!` shell escape (`runShellEscape`) +— the three places it can sit for any length of time. Before issue #24 the +handler gob-encoded the live game tree from its own goroutine after removing the +save file, which raced every mutation the game was making and left a window with +no save file at all; `saveFile` now writes a temporary file in the save's own +directory and renames it over the target, so the previous save survives any +failure, including the deadline expiring with nothing written. + +What that guarantees precisely, and what it does not: the encode runs on the one +goroutine that owns the state, so the snapshot is internally consistent and +always restorable. It is not guaranteed to be a between-commands snapshot. +`readchar` is also reached from prompts raised part-way through a command +(`--More--`, `askOverwrite`, `getStr`, the direction and pack prompts), and the +command has already mutated state by then — `fight` sets `Count`/`Quiet` and +runs `runTo` before any message, `revealXeroc` writes `Disguise` before emitting +one. A save serviced at such a prompt freezes that command half applied; +restoring re-enters `playit` at the top of `command`, so the rest of that +command never runs. The result is a coherent state one turn's worth of effects +short, which is the price of being able to save at all for a player whose line +dropped mid-prompt. + +The shell escape runs the shell on a helper goroutine so that the game goroutine +stays free to answer, but a panic out of `Terminal.ShellEscape` (which is how a +failed `Screen.Resume` is reported) is recovered there and re-raised on the game +goroutine. A panic reaching the top of a helper goroutine would kill the process +without running the main goroutine's `defer t.Fini()`, leaving the tty raw — the +failure issue #12 removed, on the one path where the terminal is already broken. +Re-raising keeps the invariant below true. + The handlers are installed immediately after `term.New()`, which is the call that puts the tty in raw mode, and before the game exists — the saver is handed over afterwards through `pendingSaver`. That ordering is what makes "every path @@ -1611,26 +1647,26 @@ Tombstone/victory screens port verbatim from rip.c. ## 6. C construct → Go construct map -| C construct | Go translation | -| ---------------------------------------------------- | ----------------------------------------------------------------------------------------------- | -| global variable | `RogueGame` field (or Player/Level/subsystem field) | -| file-scope static | unexported field on `RogueGame` or subsystem struct | -| `THING` union | `Creature` / `Object` structs | -| linked lists (`l_next/l_prev`, attach/detach) | slices + `attach(&s, x)`-style helpers (prepend, remove by identity) | -| `coord*` aliasing (t_dest) | `*Coord` pointers into live structs (kept) | -| function pointers (daemons, options, nameit) | enum IDs → handler tables on `gameData` (daemons + effect dispatch); closures (options, nameit) | -| `when`/`otherwise`/`until` macros | `case`/`default`/`for !cond` | -| `goto` (retry loops in pack.c, move.c, passages.c) | labeled loops / restructured `for` | -| char-indexed tables (`monsters[type-'A']`) | same, `monsterTable[t.Type-'A']` | -| damage strings `"3x4/1x2"` | parsed once into a `DiceSpec` (`[]DiceRoll`) by `ParseDice` at table time | -| curses stdscr/hw windows | `Window` cell buffers over tcell | -| `mvinch` screen reads | `Window.Inch` from the buffer | -| signal handlers (SIGHUP autosave, SIGTSTP, SIGINT) | `os/signal` goroutine → channel checked in ReadChar; tcell handles TSTP/resize | -| `setjmp`-free exits (`my_exit`, `exit()` everywhere) | `myExit` restores the terminal and calls `os.Exit(0)`; one game run is one process | -| `vsprintf` message building | `fmt.Sprintf` | -| XOR-encrypted binary saves (state.c) | gob snapshot (§5.6) | -| DES wizard password | `ROGUE_WIZARD=1` env check | -| `md_*` platform shims | stdlib (`os`, `os/user`, `time`) or deleted | +| C construct | Go translation | +| ---------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------- | +| global variable | `RogueGame` field (or Player/Level/subsystem field) | +| file-scope static | unexported field on `RogueGame` or subsystem struct | +| `THING` union | `Creature` / `Object` structs | +| linked lists (`l_next/l_prev`, attach/detach) | slices + `attach(&s, x)`-style helpers (prepend, remove by identity) | +| `coord*` aliasing (t_dest) | `*Coord` pointers into live structs (kept) | +| function pointers (daemons, options, nameit) | enum IDs → handler tables on `gameData` (daemons + effect dispatch); closures (options, nameit) | +| `when`/`otherwise`/`until` macros | `case`/`default`/`for !cond` | +| `goto` (retry loops in pack.c, move.c, passages.c) | labeled loops / restructured `for` | +| char-indexed tables (`monsters[type-'A']`) | same, `monsterTable[t.Type-'A']` | +| damage strings `"3x4/1x2"` | parsed once into a `DiceSpec` (`[]DiceRoll`) by `ParseDice` at table time | +| curses stdscr/hw windows | `Window` cell buffers over tcell | +| `mvinch` screen reads | `Window.Inch` from the buffer | +| signal handlers (SIGHUP autosave, SIGTSTP, SIGINT) | `os/signal` goroutine; the autosave is handed to the game goroutine and taken between turns or on waking a blocked `ReadChar`; tcell handles resize only | +| `setjmp`-free exits (`my_exit`, `exit()` everywhere) | `myExit` restores the terminal and calls `os.Exit(0)`; one game run is one process | +| `vsprintf` message building | `fmt.Sprintf` | +| XOR-encrypted binary saves (state.c) | gob snapshot (§5.6) | +| DES wizard password | `ROGUE_WIZARD=1` env check | +| `md_*` platform shims | stdlib (`os`, `os/user`, `time`) or deleted | **Exit discipline** deserves a note: the C code calls `exit()` from deep inside call chains (death, victory, save). One game run is one process, so the port @@ -1752,13 +1788,19 @@ the signal goroutine while the game goroutine may be inside `Render` or call from the signal goroutine is safe — it is a _logical_ race over the screen state: the game goroutine can redraw into a screen the handler has just suspended, or resume under a half-finished frame. Getting it right means -plumbing the signal through the input loop and handling it synchronously, a -design change well beyond a signal-safety fix. C's own wiring here is vestigial: -`tstp` is armed only by `md_tstpresume()`, which runs after a successful -`restore()`, so a freshly started C game never had a SIGTSTP handler either. -`term.Tcell.ShellEscape` (the `!` command) already covers getting to a shell and -back, doing the same suspend/resume dance synchronously on the game goroutine -where it is safe. +plumbing the signal through the input loop and handling it synchronously. Half +of that plumbing now exists — `Terminal.Interrupt` wakes a blocked `ReadChar` so +the game goroutine can act on a signal (§5.3, issue #24) — but the suspend and +the resume still have to be sequenced against the game's own drawing, which is +the part that remains a design change rather than a signal-safety fix. C's own +wiring here is vestigial: `tstp` is armed only by `md_tstpresume()`, which runs +after a successful `restore()`, so a freshly started C game never had a SIGTSTP +handler either. `term.Tcell.ShellEscape` (the `!` command) already covers +getting to a shell and back. Since issue #24 the shell itself runs on a helper +goroutine (`runShellEscape`), so a hangup arriving while the player is away in +the shell still rescues the game; the game goroutine waits there without +drawing, so the suspend/resume dance is as undisturbed as it was when the call +ran inline. **SIGINT → `quit()`.** Under `md_onsignal_autosave` C routed SIGINT into the interactive "really quit?" prompt. The port exits instead (after restoring the diff --git a/MEMORY.md b/MEMORY.md index 0ce580b..54fdf04 100644 --- a/MEMORY.md +++ b/MEMORY.md @@ -10,7 +10,36 @@ unlikely error returns through game code — e.g. write-side Close/encode failur where continuing would mean corrupt state. Return errors where a caller genuinely handles them (save-file prompts, restore validation). Reserve deliberate `_ =` discards for true best-effort paths (scorefile writes, -signal-time autosave), always with a comment saying why. +`Terminal.Interrupt`'s post to a full event queue), always with a comment saying +why. + +Signal-time autosave used to be on that list and no longer is (issue #24). It is +best effort in the sense that nothing can be reported to a player whose terminal +is already going away, but the outcome is a value, not a discard: the signal +goroutine calls `AutoSaveOnSignal`, which hands the save to the game goroutine — +the only one allowed to touch game state — and returns whether it was taken +before the deadline. The game answers between turns (`command`), while parked +waiting for a key (`readchar`), and while parked in the `!` shell escape +(`runShellEscape`). `saveFile` writes a temporary file and renames it over the +target, so a save that fails or never happens leaves the player's previous save +whole; never reintroduce a `Remove` before the write in `autoSave`, and never +encode game state from any goroutine but the game's. + +Do not upgrade that into "the snapshot is always taken between commands" — it is +not. What is true is that the encode runs on the state-owning goroutine, so the +snapshot is internally consistent and restorable. `readchar` is reached from +mid-command prompts (`--More--`, `askOverwrite`, `getStr`, direction and pack +prompts) and the command has already mutated state by then, so a save taken +there freezes that command half applied and the player loses the rest of it on +restore. That is acceptable and documented; the false stronger claim was caught +in review of PR #26 and must not come back. + +Related, and easy to reintroduce: work moved onto a helper goroutine must not be +allowed to panic there. A panic at the top of any goroutine kills the process +without running the other goroutines' defers, including `cmd/rogue/main.go`'s +`defer t.Fini()`, which is what leaves a raw tty (issue #12). `runShellEscape` +recovers its helper's panic and re-raises it on the game goroutine for exactly +that reason. C's exit() calls are not unwound: one game run is one process, so myExit (game/rip.go) restores the terminal via Terminal.Fini and calls os.Exit(0), and diff --git a/TODO.md b/TODO.md index a89a148..4561027 100644 --- a/TODO.md +++ b/TODO.md @@ -34,6 +34,69 @@ wizard commands). # Completed Steps +- 2026-08-09 Signal-time autosave moved onto the game goroutine + (`fix/autosave-race`, 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. `make test` + has run with `-race` since 2026-08-09 and was green, because no test had ever + driven the turn loop concurrently with a signal: evidence of untested, not of + safe. The handler now writes nothing itself. `AutoSaveOnSignal` posts a + request on a one-deep channel, wakes the input read, and waits up to + `signalSaveTimeout` (3s) for the game goroutine to take it; the encode happens + on the goroutine that owns the state. **The blocked-on-input case is the whole + point** — a dropped connection lands while the player is thinking, so a flag + checked only between turns would never be looked at — and it is handled by + making the read interruptible: `Terminal.ReadChar` returns `(byte, bool)` with + `ok == false` meaning "woken by `Interrupt`, no key", `term.Tcell.Interrupt` + posts a `tcell.EventInterrupt` onto tcell's own event queue to unpark + `PollEvent`, and `readchar` services the request and reads again, so no caller + sees the wake-up. The other unbounded park is the `!` shell escape, where a + hangup used to save and would otherwise have regressed to not saving: the + shell now runs on a helper goroutine and `runShellEscape` selects on {shell + finished, save request}, keeping the encode on the game goroutine while it + draws nothing. Between turns (`command`) covers a game that is busy rather + than parked. The wait is bounded so that a game goroutine wedged with no + service point can never stop a signal from getting the process out; giving up + costs nothing now that `saveFile` writes a temporary file in the save's own + directory, fsyncs it, and renames it over the target instead of truncating in + place — a failed or skipped save leaves the previous save whole. New + `game/autosave_test.go` drives the real turn loop while a second goroutine + asks for 25 saves (the interleaving that never existed before), plus the + parked-on-input case with a terminal fake that genuinely blocks, the shell + case, the deadline case (previous save byte-for-byte intact), the no-file-name + case, and the rename discipline — the last pinned by a handle opened before + the save, which still reads the old file whole after it. Each was + mutation-proved: reverting `AutoSaveOnSignal` to encode on the calling + goroutine (the pre-fix behavior) makes the turn-loop test fail under `-race` + with 113 reports, and removing each of the three service points fails exactly + the test for that park with its own message. `pendingSaver` now reads the game + out from under its mutex instead of delegating with it held, because the + delegated call blocks until the save is taken — the PR #23 review's N3 note, + load-bearing rather than hypothetical, and pinned by a test. The + SIGINT/SIGQUIT no-save decision and the single-signal-read ordering guarantee + are untouched; `savesOnSignal`'s third ground ("safety") is rewritten, since + the corruption window it weighed no longer exists. `MEMORY.md` stops listing + signal-time autosave among the deliberate `_ =` discards and states the new + discipline; `ARCHITECTURE.md` §5.3, the `Terminal` sketch, the C-to-Go mapping + row and §9's SIGTSTP paragraph are corrected to match. Two things review + caught and this entry records so they are not undone: moving the shell onto a + helper goroutine also moved `term.Tcell.ShellEscape`'s `panic` on a failed + `Screen.Resume` there, and a panic at the top of any goroutine kills the + process without running the deferred calls of the others — including + `cmd/rogue/main.go`'s `defer t.Fini()`, so the tty would have been left raw on + exactly the path where the terminal is already broken (issue #12's failure, + reintroduced on a new path). `runShellEscape` recovers the helper's panic and + re-raises it on the game goroutine, pinned by + `TestShellEscapePanicUnwindsTheGameGoroutine`. And the first version of this + work claimed in four places that nothing is half-mutated at the `readchar` + service point; that is false, since `readchar` is reached from mid-command + prompts. What is actually guaranteed is that the encode runs on the + state-owning goroutine, so the snapshot is internally consistent and + restorable, though it may freeze a command half applied. `Next Step` + deliberately not rotated: out-of-band issue work. + - 2026-08-09 Signal-time terminal restore (`sig-leave`, closes #12): the port handled only SIGHUP and SIGTERM, so SIGINT and SIGQUIT killed the process with tcell still holding the tty, leaving the user at a shell with no echo. All diff --git a/cmd/rogue/main.go b/cmd/rogue/main.go index d8ebdcc..d4d4392 100644 --- a/cmd/rogue/main.go +++ b/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() diff --git a/cmd/rogue/main_test.go b/cmd/rogue/main_test.go index 182a2ea..38c41e9 100644 --- a/cmd/rogue/main_test.go +++ b/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 +} diff --git a/game/autosave_test.go b/game/autosave_test.go new file mode 100644 index 0000000..c34dd0c --- /dev/null +++ b/game/autosave_test.go @@ -0,0 +1,511 @@ +//nolint:testpackage // white-box tests reach unexported state (approved 2026-07-07) +package game + +// Tests for the signal-triggered autosave handoff (issue #24): the signal +// goroutine must never encode game state itself, and the game goroutine +// must answer wherever it is parked. + +import ( + "io" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// autoSaveWait is the deadline the tests hand AutoSaveOnSignal when they +// expect the save to be taken. It is long enough that a loaded machine +// cannot turn a working handoff into a spurious failure, and it is never +// actually waited out on a passing run. +const autoSaveWait = 10 * time.Second + +// TestAutoSaveOnSignalRacesTurnLoop is the test issue #24 exists for: it +// drives the real turn loop on one goroutine while another asks for a +// signal-triggered autosave over and over, which is the interleaving no +// test in the suite used to produce. `make test` runs with -race, so a +// save that encodes the live game tree from the asking goroutine — what +// the old AutoSave did straight from the signal handler — is reported as +// a data race and fails this test. +// +// Non-vacuity: with AutoSaveOnSignal's body replaced by a direct +// g.autoSave() call, i.e. exactly the pre-#24 behavior, this test fails +// under -race with the encoder reading state that command() is writing. +func TestAutoSaveOnSignalRacesTurnLoop(t *testing.T) { + t.Parallel() + + // Same mix as TestTurnLoopCrashSweep: the spaces answer any --More-- + // prompt, and the script is long enough that the drive never runs it + // out. + script := []byte(strings.Repeat("h j k l y u b n s . ", 400)) + + g := New(Params{Seed: 20260809, Term: &testTerm{input: script}}) + g.FileName = filepath.Join(t.TempDir(), "rogue.save") + g.startLevel() + g.prePlay() + + const wantSaves = 25 + + var taken int + + done := make(chan struct{}) + + go func() { + defer close(done) + + for range wantSaves { + if g.AutoSaveOnSignal(autoSaveWait) { + taken++ + } + } + }() + + driveUntilDone(t, g, done) + + // The close of done orders that goroutine's writes before this read. + if taken != wantSaves { + t.Errorf("saves taken = %d, want %d", taken, wantSaves) + } + + // Every request was answered by the turn loop, so the file is the + // work of the game goroutine and must be a whole save. + assertRestorable(t, g.FileName) +} + +// driveUntilDone runs turns until the saving goroutine is finished, +// fortifying the hero each turn so no death exits the test binary. The +// turn cap keeps a broken handoff from hanging the suite instead of +// failing it. +func driveUntilDone(t *testing.T, g *RogueGame, done <-chan struct{}) { + t.Helper() + + const maxTurns = 1000 + + for range maxTurns { + select { + case <-done: + return + default: + } + + fortify(g) + g.command() + } + + t.Fatal("the turn loop ran out of turns before the saves were taken") +} + +// TestAutoSaveOnSignalWhileBlockedOnInput is the case the fix is really +// for: the connection drops while the player is staring at the screen, +// so the game goroutine is parked in ReadChar and will not reach the +// between-turns check on its own. A flag checked only between turns would +// never be looked at here. +func TestAutoSaveOnSignalWhileBlockedOnInput(t *testing.T) { + t.Parallel() + + bt := newBlockingTerm() + g := mkBlockedGame(t, bt) + + read := make(chan byte) + + go func() { read <- g.readchar() }() + + // The wake is buffered, so this is correct whether or not the reader + // has reached ReadChar yet. + if !g.AutoSaveOnSignal(autoSaveWait) { + t.Fatal("the save was not taken while the game was blocked on input") + } + + assertRestorable(t, g.FileName) + + // The interrupt must not have been mistaken for a keystroke: the + // reader is still waiting, and still returns the real key. + bt.keys <- 'x' + + if ch := <-read; ch != 'x' { + t.Errorf("readchar() = %q, want 'x'", ch) + } +} + +// TestAutoSaveOnSignalWhileInShellEscape covers the other place the game +// goroutine parks for an unbounded time: the `!` shell escape, where it +// used to sit inside the shell call with no way to answer. A dropped line +// while the player is off in a shell is as much a hangup as any other. +func TestAutoSaveOnSignalWhileInShellEscape(t *testing.T) { + t.Parallel() + + st := &shellTerm{ + blockingTerm: newBlockingTerm(), + entered: make(chan struct{}), + release: make(chan struct{}), + } + g := mkBlockedGame(t, st) + + left := make(chan struct{}) + + go func() { + defer close(left) + + g.shell() + }() + + <-st.entered + + if !g.AutoSaveOnSignal(autoSaveWait) { + t.Error("the save was not taken while the game was in the shell escape") + } + + assertRestorable(t, g.FileName) + + close(st.release) + <-left +} + +// TestShellEscapePanicUnwindsTheGameGoroutine pins the reason +// runShellEscape recovers its helper's panic. +// +// term.Tcell.ShellEscape panics when Screen.Resume fails, and the shell +// now runs on a helper goroutine. A panic reaching the top of that helper +// would kill the process without running the deferred calls of any other +// goroutine — including cmd/rogue/main.go's `defer t.Fini()`, which is +// the only thing that takes the tty back out of raw mode. That is issue +// #12's failure, and it would land on the one path where the terminal is +// already broken. +// +// So the panic has to arrive on the goroutine that runs the game, with +// that goroutine's deferred restore still on the stack. This test stands +// in for main: a Fini deferred around the g.shell() call, and the panic +// caught after it, asserting both that the restore ran and that the +// original value came through. Against the unrecovered version there is +// nothing to assert — the panic escapes a helper goroutine and takes the +// whole test binary down, which is the failure being prevented. +func TestShellEscapePanicUnwindsTheGameGoroutine(t *testing.T) { + t.Parallel() + + pt := &panickingShellTerm{blockingTerm: newBlockingTerm()} + g := mkBlockedGame(t, pt) + + caught := make(chan any, 1) + + go func() { + // Registered first, so it runs last: it sees the terminal + // already restored, exactly as the runtime would have printed + // the trace after main's Fini. + defer func() { caught <- recover() }() + + // Stands in for cmd/rogue/main.go's `defer t.Fini()`. + defer pt.Fini() + + g.shell() + }() + + got := <-caught + + if got == nil { + t.Fatal("the resume failure did not reach the game goroutine") + } + + if msg, ok := got.(string); !ok || msg != errShellResume { + t.Errorf("recovered %v, want %q", got, errShellResume) + } + + if !pt.restored { + t.Error("the terminal was not restored on the way out") + } + + // shell() must not have resumed into its InShell reset and refresh: + // there is no screen left to draw into. + if !g.InShell { + t.Error("shell() carried on drawing after the resume failed") + } +} + +// TestAutoSaveOnSignalTimesOutLeavingTheOldSave pins the backstop: a game +// goroutine that never reaches a service point must not hold the process +// open, and giving up must cost the player nothing. The old save is still +// there, byte for byte — which is the whole point of renaming over the +// target instead of removing it first. +func TestAutoSaveOnSignalTimesOutLeavingTheOldSave(t *testing.T) { + t.Parallel() + + g := mkGame(t, 77) + g.FileName = filepath.Join(t.TempDir(), "rogue.save") + + const old = "an older save nobody is allowed to destroy" + + writeErr := os.WriteFile(g.FileName, []byte(old), 0o600) + if writeErr != nil { + t.Fatal(writeErr) + } + + // Nothing drives the turn loop, so nothing will ever answer. + start := time.Now() + + if g.AutoSaveOnSignal(100 * time.Millisecond) { + t.Error("AutoSaveOnSignal reported a save that nobody took") + } + + if waited := time.Since(start); waited > time.Second { + t.Errorf("waited %v for an unanswered save, want the deadline to bound it", + waited) + } + + got, readErr := os.ReadFile(g.FileName) + if readErr != nil { + t.Fatalf("the previous save was destroyed: %v", readErr) + } + + if string(got) != old { + t.Error("the previous save was overwritten by a save that never ran") + } +} + +// TestAutoSaveOnSignalWithoutASaveFile covers the death demo's terminal +// case: a game with no file name has nothing to write, and must say so +// rather than reporting a save that did not happen. +func TestAutoSaveOnSignalWithoutASaveFile(t *testing.T) { + t.Parallel() + + g := New(Params{Seed: 5, Term: &testTerm{ + input: []byte(strings.Repeat("s . ", 200)), + }}) + g.FileName = "" + g.startLevel() + g.prePlay() + + var answered bool + + done := make(chan struct{}) + + go func() { + defer close(done) + + answered = g.AutoSaveOnSignal(autoSaveWait) + }() + + driveUntilDone(t, g, done) + + if answered { + t.Error("AutoSaveOnSignal = true with no save file name") + } +} + +// TestSaveFileReplacesTargetAtomically pins the write discipline: the new +// save arrives by rename, so the file the player already had is never +// written into, and the temporary file it came from is not left lying in +// the save directory. +// +// The load-bearing assertion is the handle opened before the save. A +// rename leaves the old file whole and merely stops it being reachable by +// name, so that handle still reads the old save; the truncate-in-place +// write this replaced would empty it under the reader — the same +// in-place write that, interrupted, left the player with a file that +// could no longer be restored. +func TestSaveFileReplacesTargetAtomically(t *testing.T) { + t.Parallel() + + g := mkGame(t, 11) + dir := t.TempDir() + path := filepath.Join(dir, "rogue.save") + + const old = "an older save" + + writeErr := os.WriteFile(path, []byte(old), 0o600) + if writeErr != nil { + t.Fatal(writeErr) + } + + held, openErr := os.Open(path) //nolint:gosec // G304: test temp path + if openErr != nil { + t.Fatal(openErr) + } + + defer func() { _ = held.Close() }() + + saveErr := g.saveFile(path) + if saveErr != nil { + t.Fatalf("saveFile: %v", saveErr) + } + + kept, readErr := io.ReadAll(held) + if readErr != nil { + t.Fatalf("reading the file that was there before the save: %v", readErr) + } + + if string(kept) != old { + t.Errorf("the previous save was written into rather than replaced: %q", + string(kept)) + } + + entries, readErr := os.ReadDir(dir) + if readErr != nil { + t.Fatal(readErr) + } + + if len(entries) != 1 || entries[0].Name() != "rogue.save" { + t.Errorf("save directory = %v, want just the save file", names(entries)) + } + + info, statErr := os.Stat(path) + if statErr != nil { + t.Fatal(statErr) + } + + if perm := info.Mode().Perm(); perm != 0o400 { + t.Errorf("save file mode = %v, want 0400", perm) + } + + assertRestorable(t, path) +} + +// TestSaveFileLeavesTargetWhenTheRenameFails is the other half of the +// same discipline: a save that cannot be completed must leave what the +// player already had. The target here is a non-empty directory, which no +// rename can replace — the one write failure that can be forced without +// depending on file permissions, and therefore on not being root. +func TestSaveFileLeavesTargetWhenTheRenameFails(t *testing.T) { + t.Parallel() + + g := mkGame(t, 12) + dir := t.TempDir() + path := filepath.Join(dir, "rogue.save") + + mkErr := os.Mkdir(path, 0o700) + if mkErr != nil { + t.Fatal(mkErr) + } + + keep := filepath.Join(path, "keep") + + writeErr := os.WriteFile(keep, []byte("still here"), 0o600) + if writeErr != nil { + t.Fatal(writeErr) + } + + saveErr := g.saveFile(path) + if saveErr == nil { + t.Error("saveFile over an unreplaceable target reported success") + } + + _, statErr := os.Stat(keep) + if statErr != nil { + t.Errorf("the target was damaged by a failed save: %v", statErr) + } + + entries, readErr := os.ReadDir(dir) + if readErr != nil { + t.Fatal(readErr) + } + + if len(entries) != 1 { + t.Errorf("save directory = %v, want no temporary file left behind", + names(entries)) + } +} + +// names lists directory entry names for a failure message. +func names(entries []os.DirEntry) []string { + out := make([]string, 0, len(entries)) + for _, e := range entries { + out = append(out, e.Name()) + } + + return out +} + +// assertRestorable checks that path holds a save this program can load, +// which is what "the save was taken" has to mean: a file of the right +// size proves nothing about a torn encode. +func assertRestorable(t *testing.T, path string) { + t.Helper() + + _, err := Restore(path, Params{Term: &testTerm{}}) + if err != nil { + t.Errorf("the saved file does not restore: %v", err) + } +} + +// mkBlockedGame builds a game with a save file name and a terminal whose +// reads block, for the tests that park the game goroutine. +func mkBlockedGame(t *testing.T, term Terminal) *RogueGame { + t.Helper() + + g := New(Params{Seed: 4242, Term: term}) + g.NewLevel() + g.FileName = filepath.Join(t.TempDir(), "rogue.save") + + return g +} + +// blockingTerm is a Terminal that genuinely blocks in ReadChar until a +// key is pushed or Interrupt wakes it — which testTerm, whose reads never +// block, cannot reproduce. +type blockingTerm struct { + keys chan byte + wake chan struct{} +} + +func newBlockingTerm() *blockingTerm { + return &blockingTerm{ + keys: make(chan byte), + // Buffered by one and posted to without blocking, the same + // contract term.Tcell.Interrupt has with tcell's event queue: an + // interrupt that arrives before the read still wakes it. + wake: make(chan struct{}, 1), + } +} + +func (t *blockingTerm) Render(*Window) {} + +func (t *blockingTerm) Fini() {} + +// Interrupt wakes a blocked ReadChar; called from the saving goroutine. +func (t *blockingTerm) Interrupt() { + select { + case t.wake <- struct{}{}: + default: + } +} + +// ReadChar blocks until a key arrives or Interrupt wakes it. +func (t *blockingTerm) ReadChar() (byte, bool) { + select { + case ch := <-t.keys: + return ch, true + case <-t.wake: + return 0, false + } +} + +// shellTerm is a blockingTerm that also offers a shell escape which stays +// in the shell until the test lets it out. +type shellTerm struct { + *blockingTerm + + entered chan struct{} + release chan struct{} +} + +// ShellEscape parks the caller in the "shell" until released. +func (t *shellTerm) ShellEscape() { + close(t.entered) + <-t.release +} + +// errShellResume is what panickingShellTerm panics with, standing in for +// the value term.Tcell.ShellEscape raises when Screen.Resume fails. +const errShellResume = "resume failed" + +// panickingShellTerm is a blockingTerm whose shell escape panics on the +// way out, the way term.Tcell.ShellEscape does when the screen cannot be +// resumed. It records whether Fini ran, which is the thing that must +// still happen. +type panickingShellTerm struct { + *blockingTerm + + restored bool +} + +func (t *panickingShellTerm) Fini() { t.restored = true } + +func (t *panickingShellTerm) ShellEscape() { panic(errShellResume) } diff --git a/game/command.go b/game/command.go index e21690b..dd1dcff 100644 --- a/game/command.go +++ b/game/command.go @@ -8,6 +8,13 @@ package game func (g *RogueGame) command() { p := &g.Player + // Between turns is the one point in the loop where the game state is + // whole, so it is where a signal-triggered autosave is answered when + // the game goroutine is busy rather than waiting for a key (issue + // #24). The other service points are readchar (io.c) and + // runShellEscape, covering the two ways this goroutine can be parked. + g.serviceAutoSaveRequest() + ntimes := 1 // number of player moves if p.On(Hasted) { ntimes++ @@ -890,7 +897,7 @@ func (g *RogueGame) shell() { if se, ok := g.scr.term.(interface{ ShellEscape() }); ok { g.InShell = true - se.ShellEscape() + g.runShellEscape(se) g.InShell = false g.refresh() @@ -898,3 +905,58 @@ func (g *RogueGame) shell() { g.msg("shell escape is not available") } } + +// runShellEscape runs the shell and returns when it exits, answering +// signal-triggered autosave requests in the meantime (issue #24). +// +// The shell blocks for as long as the player is away — minutes, or until +// they forget — and a line dropping while they are in it is exactly the +// case SIGHUP autosave exists for, so this goroutine cannot simply sit +// inside the call. The shell runs on a helper goroutine and the game +// goroutine waits here, still the only one that ever encodes game state. +// It draws nothing while it waits, so the suspend/resume dance is as +// undisturbed as it was when it ran inline (ARCHITECTURE.md section 9). +// +// A panic out of ShellEscape must not be allowed to unwind on the helper +// goroutine. term.Tcell.ShellEscape panics when Screen.Resume fails, and +// a panic reaching the top of any goroutine kills the process without +// running any *other* goroutine's deferred calls — which is where +// cmd/rogue/main.go's `defer t.Fini()` lives. Running the shell off the +// game goroutine would therefore have left the tty raw on exactly the +// path where the terminal is already broken, reintroducing issue #12 on a +// path this change created. So the helper recovers, and the value is +// re-raised below on the game goroutine, whose stack does have Fini in +// it. The recover deferral is registered after `defer close(done)` and so +// runs before it, which is what publishes panicVal to the reader. +func (g *RogueGame) runShellEscape(se interface{ ShellEscape() }) { + done := make(chan struct{}) + + var panicVal any + + go func() { + defer close(done) + + defer func() { + panicVal = recover() + }() + + se.ShellEscape() + }() + + for { + select { + case <-done: + if panicVal != nil { + // Re-raised here so the unwind passes through the game + // goroutine's deferred Fini. shell()'s InShell reset and + // refresh are skipped deliberately: there is no screen + // left to draw into. + panic(panicVal) + } + + return + case req := <-g.sigSave: + g.runAutoSaveRequest(req) + } + } +} diff --git a/game/game.go b/game/game.go index 3da4fac..5ec9f62 100644 --- a/game/game.go +++ b/game/game.go @@ -140,6 +140,13 @@ type RogueGame struct { rogueOpts string // the ROGUEOPTS string, re-parsed by playit as in C restored bool // game came from a save file; Run skips setup + // sigSave carries signal-triggered autosave requests from the signal + // goroutine to the game goroutine, which is the only one allowed to + // touch the state above (issue #24). Buffered by one: the handler + // reads exactly one signal, so there is never more than one request. + // See AutoSaveOnSignal and serviceAutoSaveRequest in save.go. + sigSave chan *autoSaveRequest + // data is the game's copy of the static tables (extern.c and friends). data *gameData } @@ -161,6 +168,7 @@ func New(params Params) *RogueGame { Depth: 1, ScorePath: params.ScorePath, LastScore: -1, + sigSave: make(chan *autoSaveRequest, 1), } g.Options = Options{ SeeFloor: true, diff --git a/game/io.go b/game/io.go index 98de025..a267a7d 100644 --- a/game/io.go +++ b/game/io.go @@ -166,15 +166,37 @@ func stepOk(ch byte) bool { // readchar reads and returns a character, checking for gross input errors // (io.c readchar). +// +// Waiting for a key is where the game spends nearly all of its wall +// clock, so it is also where a signal-triggered autosave usually finds +// it: a dropped connection lands while the player is thinking, not +// mid-turn. Terminal.Interrupt wakes the read for exactly that, and the +// save runs here, on the game goroutine, before reading again. +// +// What that buys is a snapshot taken by the goroutine that owns the +// state, so it is internally consistent and restorable. It is not +// necessarily a between-commands snapshot: readchar is also reached from +// prompts raised part-way through a command — --More--, askOverwrite, +// getStr, the direction and pack prompts — and mutation has already +// happened by then. See serviceAutoSaveRequest (save.go) for what that +// costs the player. func (g *RogueGame) readchar() byte { - ch := g.scr.term.ReadChar() - if ch == 3 { // ^C - g.quit(0) + for { + ch, ok := g.scr.term.ReadChar() + if !ok { + g.serviceAutoSaveRequest() - return 27 + continue + } + + if ch == 3 { // ^C + g.quit(0) + + return 27 + } + + return ch } - - return ch } // statusCache is the set of static shadow variables in io.c status() that diff --git a/game/save.go b/game/save.go index 8d52c67..30c7824 100644 --- a/game/save.go +++ b/game/save.go @@ -6,6 +6,8 @@ import ( "errors" "fmt" "os" + "path/filepath" + "time" ) // save.c + state.c — game persistence. The hand-written, XOR-encrypted C @@ -643,38 +645,176 @@ func (g *RogueGame) askOverwrite() saveAnswer { } } -// saveFile writes the saved game (save.c save_file). A failed write means -// a corrupt save, so the file is removed before reporting the error. +// saveFile writes the saved game (save.c save_file). +// +// The snapshot goes to a temporary file in the target's own directory and +// is renamed over the target, so there is no instant at which the player +// has no save file: until the rename the old file is whole, and after it +// the new one is. C wrote straight over the target, and this port did the +// same with a remove in front of it (AutoSave), so a write that failed — +// or a signal-time save cut short by the process dying — could leave the +// player with neither the old save nor a usable new one (issue #24). +// +// The temporary file is fsynced before the rename so its contents reach +// the disk ahead of the directory entry that will point at it. The +// directory itself is not fsynced: that would only matter for a machine +// that loses power in the same instant, and the old save survives that +// case anyway. A process killed mid-encode leaves its temporary file +// behind, which is litter next to a destroyed save file, and the dot +// prefix keeps it out of the way. func (g *RogueGame) saveFile(path string) error { - f, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o400) //nolint:gosec,lll // G304: user-chosen save path + f, err := os.CreateTemp(filepath.Dir(path), ".rogue-save-*") if err != nil { return err } - encErr := gob.NewEncoder(f).Encode(g.snapshot()) - closeErr := f.Close() + tmp := f.Name() - if encErr != nil || closeErr != nil { - _ = os.Remove(path) // don't leave a corrupt save behind + writeErr := encodeSnapshot(f, g.snapshot()) + if writeErr != nil { + _ = os.Remove(tmp) // never leave a half-written file behind - if encErr != nil { - return encErr - } - - return closeErr + return writeErr } - return os.Chmod(path, 0o400) + renErr := os.Rename(tmp, path) + if renErr != nil { + _ = os.Remove(tmp) + + return renErr + } + + return nil } -// AutoSave silently saves to the current file name; used on SIGHUP/SIGTERM -// (save.c auto_save). Best effort by design: it runs on the way out of a -// dying process. -func (g *RogueGame) AutoSave() { - if g.FileName != "" { - _ = os.Remove(g.FileName) - _ = g.saveFile(g.FileName) +// encodeSnapshot encodes the snapshot into an open temporary file and +// closes it, leaving it read-only as the C game's saves were (save.c +// save_file). It never removes the file: its caller owns the cleanup, so +// that one place decides what happens to a failed write. +func encodeSnapshot(f *os.File, st *SaveState) error { + encErr := gob.NewEncoder(f).Encode(st) + if encErr == nil { + encErr = f.Sync() } + + if encErr == nil { + encErr = f.Chmod(0o400) + } + + closeErr := f.Close() + + if encErr != nil { + return encErr + } + + return closeErr +} + +// autoSaveRequest is one signal-triggered autosave in flight: the signal +// goroutine posts it and waits, the game goroutine performs the save and +// closes done. ok is written before done is closed and read only after, +// so the close is the happens-before edge that publishes it. +type autoSaveRequest struct { + done chan struct{} + ok bool +} + +// AutoSaveOnSignal asks the game goroutine to autosave and waits up to +// timeout for it to finish, reporting whether the save actually ran +// (save.c auto_save, the SIGHUP/SIGTERM handler). It is the only entry +// point the signal goroutine may use, and it deliberately touches no game +// state: the gob encoder used to walk the live game tree from the signal +// goroutine while the game goroutine was mid-turn mutating it (issue +// #24). +// +// Blocked on input is the case that matters, since a dropped connection +// is the whole reason the handler exists: the request is posted first and +// the input read is then interrupted, so a game goroutine parked in +// ReadChar wakes, saves in readchar, and reads again. A game goroutine +// that is running turns instead picks the request up between turns, in +// command; one parked in the `!` shell escape picks it up in +// runShellEscape. +// +// The wait is bounded because the signal goroutine's job is to get the +// process out. If the game goroutine is somewhere with no service point +// at all, the deadline expires, this reports false, and the caller +// restores the terminal and exits — leaving the player's previous save +// file exactly as it was, which is the point of the rename in saveFile. +func (g *RogueGame) AutoSaveOnSignal(timeout time.Duration) bool { + req := &autoSaveRequest{done: make(chan struct{})} + + select { + case g.sigSave <- req: + default: + // A request is already queued and unserviced, or there is no + // game loop to service one; either way this one would not be + // answered either. + return false + } + + g.scr.Interrupt() + + timer := time.NewTimer(timeout) + defer timer.Stop() + + select { + case <-req.done: + return req.ok + case <-timer.C: + return false + } +} + +// serviceAutoSaveRequest performs a pending signal-triggered autosave, if +// one is waiting, and otherwise returns at once. It runs on the game +// goroutine — that is the whole design — so it must only be called where +// that goroutine is not itself inside the encode: between turns, or while +// parked waiting for input or for the shell escape. +// +// What is guaranteed, exactly: the encode runs on the one goroutine that +// owns the state, so the snapshot is internally consistent and always +// restorable. It is *not* guaranteed to be a between-commands snapshot. +// Two of the three service points are, but readchar is reached from +// prompts raised part-way through a command — --More-- on the second +// message of a turn, askOverwrite, getStr, the direction and pack prompts +// — and by then the command has already mutated state: fight sets +// g.Count and g.Quiet and runs runTo before any message, revealXeroc +// writes tp.Disguise before emitting one. A save serviced at such a +// prompt therefore freezes that command half applied; restoring re-enters +// playit at the top of command, so the rest of that command never runs +// and the player loses its remaining effects. That is a coherent game +// state, one turn's worth of effects short — strictly better than the +// torn encode this replaced, and the cost of being able to save a player +// whose line dropped mid-prompt at all. +func (g *RogueGame) serviceAutoSaveRequest() { + select { + case req := <-g.sigSave: + g.runAutoSaveRequest(req) + default: + } +} + +// runAutoSaveRequest answers one request: save, then release the waiter. +func (g *RogueGame) runAutoSaveRequest(req *autoSaveRequest) { + req.ok = g.autoSave() + + close(req.done) +} + +// autoSave silently saves to the current file name (save.c auto_save), +// reporting whether it wrote a save. Game-goroutine only — reach it +// through AutoSaveOnSignal from anywhere else. +// +// The error is not surfaced: there is no player to tell, since the +// terminal is on its way out, and nothing sensible to do about it. It is +// reported to the waiting signal goroutine as a failed save rather than +// discarded outright, which is what the old `_ =` here used to do. +func (g *RogueGame) autoSave() bool { + if g.FileName == "" { + return false + } + + return g.saveFile(g.FileName) == nil } // ErrSaveOutOfDate reports a save file from an incompatible version. @@ -746,6 +886,7 @@ func Restore(path string, params Params) (*RogueGame, error) { FileName: path, rogueOpts: params.RogueOpts, restored: true, + sigSave: make(chan *autoSaveRequest, 1), } g.scr = NewScreen(params.Term) g.Msgs.attach(g.scr, g.look, g.readchar) diff --git a/game/screen.go b/game/screen.go index a344bcf..9064988 100644 --- a/game/screen.go +++ b/game/screen.go @@ -14,8 +14,15 @@ type Terminal interface { // Render blits the window to the device. Render(w *Window) // ReadChar blocks for the next key, translated to Rogue's input bytes - // (arrows become hjkl, control keys their C0 codes). - ReadChar() byte + // (arrows become hjkl, control keys their C0 codes). ok is false when + // the read was woken by Interrupt instead of by a key, which is how a + // signal-triggered autosave reaches a game parked on input; the byte + // is meaningless then. + ReadChar() (ch byte, ok bool) + // Interrupt wakes a ReadChar that is blocked waiting for a key. It is + // the one Terminal method called from another goroutine, so an + // implementation must be safe to call concurrently with ReadChar. + Interrupt() // Fini restores the device to its pre-game state (curses endwin). The // game calls it on its way out, since one game run is one process. Fini() @@ -223,6 +230,15 @@ func (s *Screen) Fini() { } } +// Interrupt wakes a device read that is blocked waiting for a key, if +// there is a device. Called from the signal goroutine; everything else on +// Screen belongs to the game goroutine. +func (s *Screen) Interrupt() { + if s.term != nil { + s.term.Interrupt() + } +} + // RefreshWin pushes an arbitrary window to the device (curses wrefresh). func (s *Screen) RefreshWin(w *Window) { if s.term != nil { diff --git a/game/term_test.go b/game/term_test.go index ca0bcc3..951931a 100644 --- a/game/term_test.go +++ b/game/term_test.go @@ -14,18 +14,22 @@ func (t *testTerm) Render(*Window) {} func (t *testTerm) Fini() {} -func (t *testTerm) ReadChar() byte { +// Interrupt has nothing to wake: this terminal's ReadChar never blocks. +// The blocking case has its own fake, blockingTerm in autosave_test.go. +func (t *testTerm) Interrupt() {} + +func (t *testTerm) ReadChar() (byte, bool) { if t.pos < len(t.input) { c := t.input[t.pos] t.pos++ - return c + return c, true } t.tick++ if t.tick%2 == 0 { - return '\n' + return '\n', true } - return ' ' + return ' ', true } diff --git a/term/tcell.go b/term/tcell.go index 49d1a39..79d33dc 100644 --- a/term/tcell.go +++ b/term/tcell.go @@ -79,8 +79,9 @@ func (t *Tcell) Render(w *game.Window) { } // ReadChar blocks for the next key, translated to the byte codes the C -// game reads: arrows become hjkl, control keys their C0 codes. -func (t *Tcell) ReadChar() byte { +// game reads: arrows become hjkl, control keys their C0 codes. ok is +// false when Interrupt woke the read instead of a key arriving. +func (t *Tcell) ReadChar() (byte, bool) { for { ev := t.screen.PollEvent() switch ev := ev.(type) { @@ -88,14 +89,32 @@ func (t *Tcell) ReadChar() byte { if t.last != nil { t.Render(t.last) } + case *tcell.EventInterrupt: + // Interrupt posted this from the signal goroutine: hand + // control back so the game goroutine can service a pending + // autosave, then it reads again. + return 0, false case *tcell.EventKey: if b, ok := translateKey(ev); ok { - return b + return b, true } } } } +// Interrupt wakes a ReadChar parked in PollEvent by posting an interrupt +// event onto tcell's own event queue — the mechanism tcell provides for +// exactly this, and the only Tcell method called from another goroutine +// (Screen.PostEvent is a channel send, safe to call concurrently). +// +// Best effort by design: PostEvent fails only when the event queue is +// full, which means the game goroutine is not parked waiting for a key, +// and a game goroutine that is running turns reaches the between-turns +// check on its own. +func (t *Tcell) Interrupt() { + _ = t.screen.PostEvent(tcell.NewEventInterrupt(nil)) +} + // translateKey converts a key event to a game input byte; ok is false // for keys the C game does not understand. func translateKey(ev *tcell.EventKey) (byte, bool) {