3a01283358
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. Only the check at the top of command is; the other two service points both sit inside a command call already under way. readchar is reached from mid-command prompts (--More--, askOverwrite, getStr, the direction and pack prompts) with the command's mutations already applied, and runShellEscape is reached from shell, an ordinary '!' command handler dispatched inside command, with that turn's DoDaemons(Before) and DoFuses(Before) already fired and its AFTER pass not yet. Restoring re-enters playit at the top of command, so either way the rest of that command is lost and a fresh BEFORE pass runs on top of the one in the snapshot. 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.
89 lines
5.1 KiB
Markdown
89 lines
5.1 KiB
Markdown
# Project Memory
|
|
|
|
Working notes for agents on this repo. Read this alongside TODO.md (which holds
|
|
the step queue and workflow) before starting work.
|
|
|
|
## Error handling
|
|
|
|
Panicking on bad/unexpected errors is allowed and preferred over threading
|
|
unlikely error returns through game code — e.g. write-side Close/encode failures
|
|
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,
|
|
`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. Only the check at the top of
|
|
`command` is a between-commands snapshot; the other two service points both sit
|
|
inside a `command` call already under way. `readchar` is reached from
|
|
mid-command prompts (`--More--`, `askOverwrite`, `getStr`, direction and pack
|
|
prompts) with the command's mutations already applied, and `runShellEscape` is
|
|
reached from `shell`, an ordinary `'!'` command handler, with that turn's
|
|
`DoDaemons(Before)`/`DoFuses(Before)` already fired and its AFTER pass not yet.
|
|
Restoring re-enters `playit` at the top of `command`, so either way the rest of
|
|
that command is lost and a fresh BEFORE pass runs on top of the one already in
|
|
the snapshot. That is acceptable and documented; two successive false claims —
|
|
first that `readchar` was safe, then that two of the three service points were
|
|
between-commands — were caught in review of PR #26, and neither may 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
|
|
Run() never returns. There is nothing to recover — do not write code that
|
|
expects to regain control after game-over. The testing consequence is that any
|
|
death (combat, starvation, level drain, freezing) exits the _test binary_, so
|
|
tests drive command() directly rather than Run(), and crash-sweep drives pin the
|
|
hero each turn with the fortify() helper in game/run_test.go.
|
|
|
|
## Linting
|
|
|
|
The .golangci.yml is byte-identical to the canonical shared config and must not
|
|
be edited — not even to add an exception. To disable a linter, ask sneak,
|
|
explaining what the linter does; approved exceptions are recorded as in-code
|
|
//nolint directives (file-level where a whole file is affected) carrying the
|
|
approval date, which is what keeps the config canonical. Approved so far:
|
|
testpackage, exhaustive, and mnd (2026-07-07). paralleltest was approved on
|
|
2026-07-06 but the exception is no longer in force — it was fixed instead, with
|
|
t.Parallel() in all 32 tests. The complexity linters (cyclop, gocognit, nestif)
|
|
are enabled and clean as of refactor step 7 (2026-07-07): the whole
|
|
golangci-lint run is 0 issues, so keep it that way — decompose new hot spots
|
|
rather than reaching for a nolint. Line-level //nolint with a reason is used
|
|
sparingly for C-faithfulness (e.g. the authentic "missle" message spellings) and
|
|
provably-safe gosec conversions; each needs a justifying comment.
|
|
|
|
## Faithfulness
|
|
|
|
Behavior must not change during the idiomatic-Go refactor unless a TODO step
|
|
says so. The 80x24 seed-compatible gameplay, message text (including original
|
|
typos), RNG call order, and C quirks (documented in tests like
|
|
TestHoldScrollGreedyMonsterQuirk) are contract. Doc comments keep their "(file.c
|
|
func_name)" breadcrumbs.
|
|
|
|
## Debugging
|
|
|
|
Write real, committed test files with t.Logf output and run them with the make
|
|
targets — `make test` (or `make check` for the full gate); never raw `go test`.
|
|
The target carries `-timeout 30s -race -cover` and reruns verbosely on failure,
|
|
so a raw invocation silently drops the race detector. No throwaway scratch
|
|
scripts. Successful debug probes become regression tests.
|