From f7670cf86a1a556e0d54ea4a49102c363960a725 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 08:05:30 +0000 Subject: [PATCH] docs,save: correct a stale TODO claim and rename encodeSnapshot (closes #27) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four cleanups recorded as advisories during the PR #26 review and deliberately kept out of it. No behaviour change. The 2026-08-09 sig-leave (closes #12) TODO entry still argued in the present tense that declining to save on SIGINT/SIGQUIT was also the safe choice, "because AutoSave gob-encodes live state that the main goroutine is still mutating, after removing the old file". Both halves stopped being true with #24: the encode runs on the game goroutine and saveFile is CreateTemp/Sync/Chmod/Rename with no Remove. The paragraph is now in the past tense and marked superseded, pointing at the fix/autosave-race entry, and says the split stands on C and on semantics alone — which is what the current savesOnSignal comment says. The false claim was in the #12 entry, not the #24 one; the latter's account of the old remove-then-write is correctly historical and is untouched, as is the err113 mention in the 2026-07-06 entry. encodeSnapshot becomes writeSnapshotFile: it encodes, fsyncs, chmods 0400 and closes, and the old name claimed only the first of those. Its doc comment now names all four and why the fsync is there. TestAutoSaveOnSignalWhileInShellEscape used t.Error for a precondition, so a save that was never taken fell through into assertRestorable, which can then only report a second, derived failure. t.Fatal, matching the identical assertion in the blocked-on-input test. serviceAutoSaveRequest's doc comment carried 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 have caught it; the paragraph is rewrapped to the block's width. Next Step deliberately not rotated: out-of-band issue work. --- TODO.md | 110 +++++++++++++++++++++++++++--------------- game/autosave_test.go | 2 +- game/save.go | 23 ++++----- 3 files changed, 84 insertions(+), 51 deletions(-) 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: -- 2.49.1