sneak/rgoue
1
0
Fork 0
Code Issues 10 Pull Requests Actions Packages Projects Releases Wiki Activity

SIGHUP/SIGTERM autosave is a data race: gob-encodes live state from a second goroutine #24

New Issue
Closed
opened 2026-08-09 07:48:54 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 07:48:54 +02:00
Collaborator
Copy Link
Copy Source

Problem

The SIGHUP/SIGTERM handler calls AutoSave from a signal goroutine while the
game goroutine is still mutating state. The gob encoder walks the live game
tree with no synchronisation, so the save can be written from a torn,
half-updated snapshot.

This is pre-existing, not introduced by PR #23. The implementer of #12
found it, correctly left it exactly as found, and confined their change to
making sure the new SIGINT/SIGQUIT handling does not make it worse.

Worse, AutoSave removes the save file before encoding. So the failure
mode is not "a slightly stale save" — it is: delete the player's existing
save, then write a possibly-corrupt replacement. There is a window where the
player has neither.

Why -race has not caught it

The race detector only reports races it actually observes, and no test drives
the turn loop concurrently with a signal. make test runs with -race and is
green — that is evidence of untested, not of safe. Any fix here must come
with a test that genuinely provokes the interleaving, or it proves nothing.

Definition of done

  1. The autosave path no longer encodes state concurrently with the game
    goroutine. The expected shape (open to a better design if you find one):
    the signal handler sets a flag; the command loop checks it between turns
    and performs the save synchronously on the game goroutine.
  2. Signal-triggered saves still work when the game is blocked on input —
    which is the common case for a dropped connection, and the whole reason
    the handler exists. A between-turns flag check that never runs because the
    process is parked in PollEvent would be a regression, not a fix. Say
    explicitly how you handle this.
  3. A test drives the turn loop while signalling and stays clean under
    -race across repeated runs (GOFLAGS=-count=1). Demonstrate the test is
    non-vacuous: show it failing against the current unsynchronised code.
  4. The delete-then-write ordering is made safe — write to a temporary file and
    rename over the target, so a crash mid-encode cannot leave the player with
    no save at all.
  5. MEMORY.md's "signal-time autosave" note (currently listed among the
    deliberate best-effort _ = discard paths) is updated to match whatever
    the new discipline is.
  6. make check fully green.
  7. TODO.md updated in the same commit — Completed Steps entry, and do not
    rotate "Next Step".
  8. Commit title ends with (closes #N).

Implementation requirements

  • Touches cmd/rogue/main.go, game/command.go, and game/save.go.
  • Do NOT change the SIGINT/SIGQUIT no-save semantics settled in PR #23, and do
    NOT weaken its single-signal-read ordering guarantee. Read that PR's
    savesOnSignal doc comment before touching the handler.
  • make targets only. Do NOT modify .golangci.yml. No Dockerfile/CI/script/.
  • Leave c-master and modern-rogue alone; read C only via git show.
  • Do NOT regenerate goldens under game/testdata/.
  • Never mention Claude or Anthropic anywhere.

Depends on

PR #23 (#12) — land that first so the handler shape is settled.

Priority

Medium-high. It is a real correctness bug that can destroy a save file, but
it needs an involuntary signal to trigger, so it is rarer than a crash.

## Problem The SIGHUP/SIGTERM handler calls `AutoSave` from a signal goroutine while the game goroutine is still mutating state. The gob encoder walks the live game tree with no synchronisation, so the save can be written from a torn, half-updated snapshot. This is **pre-existing**, not introduced by PR #23. The implementer of #12 found it, correctly left it exactly as found, and confined their change to making sure the new SIGINT/SIGQUIT handling does not make it worse. Worse, `AutoSave` **removes the save file before** encoding. So the failure mode is not "a slightly stale save" — it is: delete the player's existing save, then write a possibly-corrupt replacement. There is a window where the player has neither. ## Why `-race` has not caught it The race detector only reports races it actually observes, and no test drives the turn loop concurrently with a signal. `make test` runs with `-race` and is green — that is evidence of *untested*, not of *safe*. Any fix here must come with a test that genuinely provokes the interleaving, or it proves nothing. ## Definition of done 1. The autosave path no longer encodes state concurrently with the game goroutine. The expected shape (open to a better design if you find one): the signal handler sets a flag; the command loop checks it between turns and performs the save **synchronously on the game goroutine**. 2. Signal-triggered saves still work when the game is blocked on input — which is the common case for a dropped connection, and the whole reason the handler exists. A between-turns flag check that never runs because the process is parked in `PollEvent` would be a regression, not a fix. Say explicitly how you handle this. 3. A test drives the turn loop **while signalling** and stays clean under `-race` across repeated runs (`GOFLAGS=-count=1`). Demonstrate the test is non-vacuous: show it failing against the current unsynchronised code. 4. The delete-then-write ordering is made safe — write to a temporary file and rename over the target, so a crash mid-encode cannot leave the player with no save at all. 5. `MEMORY.md`'s "signal-time autosave" note (currently listed among the deliberate best-effort `_ =` discard paths) is updated to match whatever the new discipline is. 6. `make check` fully green. 7. `TODO.md` updated in the same commit — Completed Steps entry, and do **not** rotate "Next Step". 8. Commit title ends with ` (closes #N)`. ## Implementation requirements - Touches `cmd/rogue/main.go`, `game/command.go`, and `game/save.go`. - Do NOT change the SIGINT/SIGQUIT no-save semantics settled in PR #23, and do NOT weaken its single-signal-read ordering guarantee. Read that PR's `savesOnSignal` doc comment before touching the handler. - `make` targets only. Do NOT modify `.golangci.yml`. No Dockerfile/CI/`script/`. - Leave `c-master` and `modern-rogue` alone; read C only via `git show`. - Do NOT regenerate goldens under `game/testdata/`. - Never mention Claude or Anthropic anywhere. ## Depends on PR #23 (#12) — land that first so the handler shape is settled. ## Priority Medium-high. It is a real correctness bug that can destroy a save file, but it needs an involuntary signal to trigger, so it is rarer than a crash.
clawbot commented 2026-08-09 08:27:33 +02:00
Author
Collaborator
Copy Link
Copy Source

Implementation plan

Read first: the issue, TODO.md, MEMORY.md, ARCHITECTURE.md §5.3/§9, and the
whole PR #23 thread (all six comments) including the savesOnSignal /
pendingSaver / leaveOnSignal code and doc comments.

Design: request-and-wait handoff, save on the game goroutine

The signal goroutine stops writing anything. It posts a request and waits.

  1. RogueGame gains an unexported sigSave chan *autoSaveRequest (buffered 1),
    initialised in New and Restore. A request carries a done chan struct{}
    and an ok field written before done is closed.
  2. New game-side entry point for the signal goroutine:
    AutoSaveOnSignal(timeout time.Duration) bool — posts the request, wakes a
    blocked input read, waits for done or the deadline, reports whether the save
    ran. It never touches game state itself.
  3. The game goroutine services requests in serviceAutoSaveRequest (non-blocking
    receive) and performs the encode synchronously, at two points:
    • top of command() (game/command.go) — between turns, which also covers
      resting/NoCommand turns and running;
    • inside readchar() (game/io.go) whenever the terminal read is woken by an
      interrupt rather than a key.
  4. AutoSave() (exported, callable from anywhere) goes away as the signal entry
    point; the actual write becomes unexported autoSave(), reachable only from
    the game goroutine.

The blocked-on-input case (definition of done #2), explicitly

A between-turns flag check alone is exactly the regression the issue names, so
the input read has to be interruptible. Mechanism:

  • Terminal gains Interrupt() and ReadChar becomes ReadChar() (byte, bool),
    where ok == false means "the read was woken by Interrupt, no key".
  • term.Tcell.Interrupt posts tcell.NewEventInterrupt(nil) via
    Screen.PostEvent, which is exactly what tcell provides to wake a goroutine
    parked in PollEvent. Tcell.ReadChar's existing event loop gains an
    *tcell.EventInterrupt case that returns (0, false).
  • readchar() loops: on ok == false it services the pending autosave and reads
    again, so no caller sees the wake-up.

Saving from inside a nested prompt (really quit?, an inventory prompt) is
deliberate and safe: the game goroutine is parked, nothing is mid-mutation at
that instant, and the snapshot is the state as of the start of that command —
the same as the player never having answered the prompt.

Second blocking case, the ! shell escape (game goroutine parked in
cmd.Run, screen suspended): today a SIGHUP there does save, so leaving it
uncovered would be a regression. shell() will run ShellEscape on a helper
goroutine and select on {shell finished, save request}, so the encode still
runs on the goroutine that owns game state while that goroutine is otherwise
idle. No concurrent Render — it is parked in the select — so §9's
suspend/resume safety argument still holds; §9 gets updated to match.

Bounded wait: AutoSaveOnSignal gives up after a deadline (const in
cmd/rogue/main.go) and reports false, so a game goroutine that is wedged
somewhere with no service point can never hang the process on its way out. On
that path nothing is written and the previous save file is left intact — which
is only acceptable because of the atomic-write change below.

Atomic write (definition of done #4)

saveFile stops truncating the target in place and autoSave stops removing it:
os.CreateTemp in the same directory, encode, Sync, Close, Chmod 0400,
os.Rename over the target, with the temp removed on every failure path. There
is then no instant at which the player has no save file, and a crash mid-encode
leaves the previous save untouched.

pendingSaver (stated reason for the one change)

Locking otherwise untouched, but the delegated call now blocks for up to the save
deadline, so it will read the saver under p.mu into a local and call it after
unlocking. Holding the mutex across a blocking delegate would stall a concurrent
set — the future-proofing note N3 from the PR #23 review, now load-bearing
rather than hypothetical. savesOnSignal's decision and leaveOnSignal's
single-signal-read ordering guarantee are not touched.

Tests (definition of done #3, non-vacuous)

In game/, all with t.Parallel():

  1. TestAutoSaveOnSignalRacesTurnLoop — drives command() in a loop on the game
    goroutine (with fortify() so no death exits the test binary) while another
    goroutine calls AutoSaveOnSignal repeatedly. This is the one that must fail
    first: I will demonstrate it under -race against an AutoSaveOnSignal that
    encodes on the calling goroutine (i.e. today's AutoSave), and report it
    rather than claim success if I cannot make it fail.
  2. TestAutoSaveOnSignalWhileBlockedOnInput — a terminal fake whose ReadChar
    genuinely blocks until Interrupt is called; the game goroutine sits in
    readchar() and the save still lands. This is the DoD #2 test.
  3. TestAutoSaveOnSignalWhileInShellEscape — same, for the shell window.
  4. TestAutoSaveOnSignalTimesOut — no service point; returns false inside the
    deadline and the existing save file is byte-for-byte untouched.
  5. saveFile tests: replaces an existing file atomically, leaves no temp behind,
    final mode 0400; and a forced-failure case leaving the previous target in
    place.
  6. cmd/rogue/main_test.go updated for the renamed saver method; the existing
    ordering/split/pendingSaver tests keep their meaning.

Docs

MEMORY.md's error-handling note stops listing signal-time autosave among the
_ = best-effort discards and describes the new discipline. ARCHITECTURE.md
§5.3 and the §9 signal rows are corrected (including the mapping-table row that
already claims "channel checked in ReadChar", which is only true after this
change). TODO.md gets a Completed Steps entry in the same commit; Next Step
is not rotated.

Out of scope, untouched

chooseSeed/SEED (#25), the SIGINT/SIGQUIT no-save decision, .golangci.yml,
game/testdata/ goldens, Dockerfile/CI/script/.

Verification will be make fmt then make check under the retry protocol —
discarding any run that says parallel golangci-lint is running or names paths
outside my worktree — plus repeated GOFLAGS=-count=1 make test runs for the
race tests.

## Implementation plan Read first: the issue, `TODO.md`, `MEMORY.md`, `ARCHITECTURE.md` §5.3/§9, and the whole PR #23 thread (all six comments) including the `savesOnSignal` / `pendingSaver` / `leaveOnSignal` code and doc comments. ### Design: request-and-wait handoff, save on the game goroutine The signal goroutine stops writing anything. It posts a request and waits. 1. `RogueGame` gains an unexported `sigSave chan *autoSaveRequest` (buffered 1), initialised in `New` and `Restore`. A request carries a `done chan struct{}` and an `ok` field written before `done` is closed. 2. New game-side entry point for the signal goroutine: `AutoSaveOnSignal(timeout time.Duration) bool` — posts the request, wakes a blocked input read, waits for `done` or the deadline, reports whether the save ran. It never touches game state itself. 3. The game goroutine services requests in `serviceAutoSaveRequest` (non-blocking receive) and performs the encode synchronously, at two points: - top of `command()` (`game/command.go`) — between turns, which also covers resting/`NoCommand` turns and running; - inside `readchar()` (`game/io.go`) whenever the terminal read is woken by an interrupt rather than a key. 4. `AutoSave()` (exported, callable from anywhere) goes away as the signal entry point; the actual write becomes unexported `autoSave()`, reachable only from the game goroutine. ### The blocked-on-input case (definition of done #2), explicitly A between-turns flag check alone is exactly the regression the issue names, so the input read has to be interruptible. Mechanism: - `Terminal` gains `Interrupt()` and `ReadChar` becomes `ReadChar() (byte, bool)`, where `ok == false` means "the read was woken by `Interrupt`, no key". - `term.Tcell.Interrupt` posts `tcell.NewEventInterrupt(nil)` via `Screen.PostEvent`, which is exactly what tcell provides to wake a goroutine parked in `PollEvent`. `Tcell.ReadChar`'s existing event loop gains an `*tcell.EventInterrupt` case that returns `(0, false)`. - `readchar()` loops: on `ok == false` it services the pending autosave and reads again, so no caller sees the wake-up. Saving from inside a nested prompt (`really quit?`, an inventory prompt) is deliberate and safe: the game goroutine is parked, nothing is mid-mutation at that instant, and the snapshot is the state as of the start of that command — the same as the player never having answered the prompt. Second blocking case, the `!` shell escape (game goroutine parked in `cmd.Run`, screen suspended): today a SIGHUP there does save, so leaving it uncovered would be a regression. `shell()` will run `ShellEscape` on a helper goroutine and `select` on {shell finished, save request}, so the encode still runs on the goroutine that owns game state while that goroutine is otherwise idle. No concurrent `Render` — it is parked in the select — so §9's suspend/resume safety argument still holds; §9 gets updated to match. Bounded wait: `AutoSaveOnSignal` gives up after a deadline (const in `cmd/rogue/main.go`) and reports `false`, so a game goroutine that is wedged somewhere with no service point can never hang the process on its way out. On that path nothing is written and the previous save file is left intact — which is only acceptable because of the atomic-write change below. ### Atomic write (definition of done #4) `saveFile` stops truncating the target in place and `autoSave` stops removing it: `os.CreateTemp` in the same directory, encode, `Sync`, `Close`, `Chmod 0400`, `os.Rename` over the target, with the temp removed on every failure path. There is then no instant at which the player has no save file, and a crash mid-encode leaves the previous save untouched. ### `pendingSaver` (stated reason for the one change) Locking otherwise untouched, but the delegated call now blocks for up to the save deadline, so it will read the saver under `p.mu` into a local and call it after unlocking. Holding the mutex across a blocking delegate would stall a concurrent `set` — the future-proofing note N3 from the PR #23 review, now load-bearing rather than hypothetical. `savesOnSignal`'s decision and `leaveOnSignal`'s single-signal-read ordering guarantee are not touched. ### Tests (definition of done #3, non-vacuous) In `game/`, all with `t.Parallel()`: 1. `TestAutoSaveOnSignalRacesTurnLoop` — drives `command()` in a loop on the game goroutine (with `fortify()` so no death exits the test binary) while another goroutine calls `AutoSaveOnSignal` repeatedly. This is the one that must fail first: I will demonstrate it under `-race` against an `AutoSaveOnSignal` that encodes on the calling goroutine (i.e. today's `AutoSave`), and report it rather than claim success if I cannot make it fail. 2. `TestAutoSaveOnSignalWhileBlockedOnInput` — a terminal fake whose `ReadChar` genuinely blocks until `Interrupt` is called; the game goroutine sits in `readchar()` and the save still lands. This is the DoD #2 test. 3. `TestAutoSaveOnSignalWhileInShellEscape` — same, for the shell window. 4. `TestAutoSaveOnSignalTimesOut` — no service point; returns `false` inside the deadline and the existing save file is byte-for-byte untouched. 5. `saveFile` tests: replaces an existing file atomically, leaves no temp behind, final mode `0400`; and a forced-failure case leaving the previous target in place. 6. `cmd/rogue/main_test.go` updated for the renamed saver method; the existing ordering/split/`pendingSaver` tests keep their meaning. ### Docs `MEMORY.md`'s error-handling note stops listing signal-time autosave among the `_ =` best-effort discards and describes the new discipline. `ARCHITECTURE.md` §5.3 and the §9 signal rows are corrected (including the mapping-table row that already claims "channel checked in ReadChar", which is only true after this change). `TODO.md` gets a Completed Steps entry in the same commit; `Next Step` is not rotated. ### Out of scope, untouched `chooseSeed`/`SEED` (#25), the SIGINT/SIGQUIT no-save decision, `.golangci.yml`, `game/testdata/` goldens, Dockerfile/CI/`script/`. Verification will be `make fmt` then `make check` under the retry protocol — discarding any run that says `parallel golangci-lint is running` or names paths outside my worktree — plus repeated `GOFLAGS=-count=1 make test` runs for the race tests.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
Review passed and gate is green; ready to merge
 needs-checks
make check is failing on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found problems; author must rework
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/rgoue#24