diff --git a/TODO.md b/TODO.md index ff3c8af..c2568dc 100644 --- a/TODO.md +++ b/TODO.md @@ -34,6 +34,32 @@ wizard commands). # Completed Steps +- 2026-08-09 Cleanups deferred from the PR #26 review (`cleanup/pr26-followups`, + closes #27): four items, no behaviour change. (1) The `sig-leave` entry below + still argued, in the present tense, that declining to save on SIGINT/SIGQUIT + was the safe choice because `AutoSave` encodes live state after removing the + file — both halves untrue since #24, and the entry read as a claim about how + the code works now rather than a record of what was weighed then. It is in the + past tense and marked superseded, pointing at the `fix/autosave-race` entry. + Nothing else in the file was touched — in particular the `err113` linter name + in the 2026-07-06 entry, which a `grep` for `113` still matches, and the "over + a hundred reports" wording the #26 rework had already corrected. Note for + anyone chasing this class of bug: the false claim was in the #12 entry, not + the #24 one, whose account of the old remove-then-write is correctly past + tense — find these by content, since `make fmt` reflows the file and cited + line numbers rot. (2) `encodeSnapshot` is `writeSnapshotFile`: it encodes, + fsyncs, chmods 0400 and closes, and the old name claimed only the first of + those. One call site (`saveFile`), and the doc comment now lists what it does + and why the fsync is there. (3) `TestAutoSaveOnSignalWhileInShellEscape` used + `t.Error` for its precondition, so a save that was never taken fell through + into `assertRestorable`, which can then only report a second, derived failure; + it is `t.Fatal`, matching the identical assertion in the blocked-on-input + test. (4) `serviceAutoSaveRequest`'s doc comment had a 24-column stub line + ("The result is still a") left by an earlier edit — `gofmt` does not rewrap + comments, so `fmt-check` was legitimately green and nothing would ever have + caught it. Rewrapped to the block's width. `Next Step` deliberately not + rotated: out-of-band issue work. + - 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 @@ -115,45 +141,51 @@ wizard commands). HUP/TERM — and the semantics agree: HUP/TERM are involuntary teardown worth rescuing a game from, while INT/QUIT are a deliberate "stop now" that must not become a one-keystroke checkpoint against a save discipline built to be - anti-save-scum. It is also the safe choice: `AutoSave` gob-encodes live state - that the main goroutine is still mutating, after removing the old file, so on - the signals with nothing to rescue the port takes the option with no - corruption window. The single-reader design closes the window the issue warned - about: a second signal arriving mid-save stays unread in the buffer instead of - exiting out from under the writer (`TestLeaveOnSignalIgnoresLaterSignals` - reproduces exactly that interleaving). New `cmd/rogue/main_test.go` pins the - membership of `handledSignals()` itself (`TestHandledSignalsSet` — without it - the rest of the file, which iterates that set, would pass against a set that - had silently lost SIGINT and SIGQUIT again), and covers the ordering for each - signal, the save/no-save split against `savesOnSignal`, the - mid-save-second-signal case, the pre-game `pendingSaver` window, and real - SIGINT/SIGQUIT/SIGHUP/SIGTERM delivered to the test process through the same - `notifySignals` wiring the game uses; the tty leaving raw mode is the one step - not checkable headlessly (it needs a controlling terminal), and - `term.Tcell.Fini` is a direct pass-through to tcell's `Screen.Fini` that - `myExit` already depends on. Two premises in the issue turned out to be wrong - and are recorded in ARCHITECTURE.md: `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`/`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`, a SIGINT to the process group while the - `!` shell escape has the screen suspended, and the window **after** - `term.New()`: nothing is raw before it, and the handlers used to be installed - only once the game existed, leaving the restore path and `-d`'s `DeathDemo()` - — which never returns, blocking in `waitFor` inside `death()` — running raw - with no handler at all. The handlers are therefore installed immediately after - `term.New()`, with the game handed to them afterwards via `pendingSaver`; a - signal before the game exists restores the terminal and exits with nothing to - save, and the SIGHUP/SIGTERM autosave behavior on the play path is unchanged. - ARCHITECTURE.md §9 gained rows for SIGTSTP/`tstp()` (dropped: raw mode means - Ctrl-Z cannot reach us, a suspend from the signal goroutine would race the - drawing goroutine, and C armed `tstp` only after a `restore()`; the `!` shell - escape covers the need), for SIGINT not routing to the interactive `quit()` - prompt, and for `auto_save` on the fault signals; §5.3's claim that tcell - handles SIGTSTP was false — tcell registers only SIGWINCH — and is corrected. - `Next Step` deliberately not rotated: out-of-band issue work. + anti-save-scum. A third ground was weighed at the time and has since been + superseded: back then `AutoSave` gob-encoded live state that the main + goroutine was still mutating, after removing the old file, so declining to + save on the signals with nothing to rescue was also the option with no + corruption window. That window is gone as of the `fix/autosave-race` entry + above (#24) — the encode now runs on the game goroutine and `saveFile` renames + a temporary file into place — so nothing here should be read as a statement + about how saving works now; the split stands on C and on semantics alone, as + the current `savesOnSignal` comment says. The single-reader design closes the + window the issue warned about: a second signal arriving mid-save stays unread + in the buffer instead of exiting out from under the writer + (`TestLeaveOnSignalIgnoresLaterSignals` reproduces exactly that interleaving). + New `cmd/rogue/main_test.go` pins the membership of `handledSignals()` itself + (`TestHandledSignalsSet` — without it the rest of the file, which iterates + that set, would pass against a set that had silently lost SIGINT and SIGQUIT + again), and covers the ordering for each signal, the save/no-save split + against `savesOnSignal`, the mid-save-second-signal case, the pre-game + `pendingSaver` window, and real SIGINT/SIGQUIT/SIGHUP/SIGTERM delivered to the + test process through the same `notifySignals` wiring the game uses; the tty + leaving raw mode is the one step not checkable headlessly (it needs a + controlling terminal), and `term.Tcell.Fini` is a direct pass-through to + tcell's `Screen.Fini` that `myExit` already depends on. Two premises in the + issue turned out to be wrong and are recorded in ARCHITECTURE.md: `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`/`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`, a SIGINT to the + process group while the `!` shell escape has the screen suspended, and the + window **after** `term.New()`: nothing is raw before it, and the handlers used + to be installed only once the game existed, leaving the restore path and + `-d`'s `DeathDemo()` — which never returns, blocking in `waitFor` inside + `death()` — running raw with no handler at all. The handlers are therefore + installed immediately after `term.New()`, with the game handed to them + afterwards via `pendingSaver`; a signal before the game exists restores the + terminal and exits with nothing to save, and the SIGHUP/SIGTERM autosave + behavior on the play path is unchanged. ARCHITECTURE.md §9 gained rows for + SIGTSTP/`tstp()` (dropped: raw mode means Ctrl-Z cannot reach us, a suspend + from the signal goroutine would race the drawing goroutine, and C armed `tstp` + only after a `restore()`; the `!` shell escape covers the need), for SIGINT + not routing to the interactive `quit()` prompt, and for `auto_save` on the + fault signals; §5.3's claim that tcell handles SIGTSTP was false — tcell + registers only SIGWINCH — and is corrected. `Next Step` deliberately not + rotated: out-of-band issue work. - 2026-08-09 Wizard-create bounds fix (`fix/wizard-which-bounds`, closes #10): `createObj` stored the raw `0-f` nibble as `Object.Which` with no bounds diff --git a/game/autosave_test.go b/game/autosave_test.go index c34dd0c..5f94942 100644 --- a/game/autosave_test.go +++ b/game/autosave_test.go @@ -152,7 +152,7 @@ func TestAutoSaveOnSignalWhileInShellEscape(t *testing.T) { <-st.entered if !g.AutoSaveOnSignal(autoSaveWait) { - t.Error("the save was not taken while the game was in the shell escape") + t.Fatal("the save was not taken while the game was in the shell escape") } assertRestorable(t, g.FileName) diff --git a/game/save.go b/game/save.go index 0c8ce96..70d7bf7 100644 --- a/game/save.go +++ b/game/save.go @@ -670,7 +670,7 @@ func (g *RogueGame) saveFile(path string) error { tmp := f.Name() - writeErr := encodeSnapshot(f, g.snapshot()) + writeErr := writeSnapshotFile(f, g.snapshot()) if writeErr != nil { _ = os.Remove(tmp) // never leave a half-written file behind @@ -687,11 +687,13 @@ func (g *RogueGame) saveFile(path string) error { return nil } -// 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 { +// writeSnapshotFile writes the snapshot into an open temporary file: it +// encodes, fsyncs so the bytes reach the disk before the caller renames +// the file into place, chmods it read-only as the C game's saves were +// (save.c save_file), and closes it. It never removes the file: its +// caller owns the cleanup, so that one place decides what happens to a +// failed write. +func writeSnapshotFile(f *os.File, st *SaveState) error { encErr := gob.NewEncoder(f).Encode(st) if encErr == nil { encErr = f.Sync() @@ -812,11 +814,10 @@ func (g *RogueGame) AutoSaveOnSignal(timeout time.Duration) bool { // handler that clears After before prompting, as identifyTrapCommand // does ahead of promptDirection. // -// The result is still a -// coherent game state, one turn's worth of effects off — strictly better -// than the torn encode this replaced, and the cost of being able to save -// a player whose line dropped mid-prompt, or who is away in a shell, at -// all. +// The result is still a coherent game state, one turn's worth of effects +// off — strictly better than the torn encode this replaced, and the cost +// of being able to save a player whose line dropped mid-prompt, or who +// is away in a shell, at all. func (g *RogueGame) serviceAutoSaveRequest() { select { case req := <-g.sigSave: