TestAutoSaveOnSignalRacesTurnLoop failed intermittently under load. The
diagnosis captured on the issue is confirmed: it is not a data race, it is
the test's own drive loop running out of a fixed 1000-turn budget. The budget is
removed rather than widened.
No non-test code changed. game/autosave_test.go and TODO.md only.
The finding, verified
The assertion is driveUntilDone's t.Fatal("the turn loop ran out of turns before the saves were taken"), and no WARNING: DATA RACE appears anywhere.
I did not take that on trust. Instrumenting the loop to report the turns it
actually consumed, and running the suite under varying scheduling pressure,
showed the count tracking contention and nothing else:
Condition (48-core host, load ~57)
Turns used by 25 saves, cap 1000
ambient, 48 procs
58, 77, 97, 102, 110, 118
GOMAXPROCS=4
105, 418
GOMAXPROCS=2
539, and one run that exhausted 1000
GOMAXPROCS=1
655
The GOMAXPROCS=2 run reproduced the reported assertion exactly, with zero DATA RACE reports in the log. That is the diagnosis, independently reproduced.
The reason is mechanical: the turns this loop spends between one save being
answered and the next request arriving are not work, they are the saving
goroutine's wake-up latency. So "1000 turns" is a wall-clock assumption in
disguise, and raising it would have hidden the flake rather than removed it.
Review measured each old-code failure at 0.12 seconds — 1000 turns burned in
a tenth of a second — which is the sharpest statement of that: the cap was never
a wall-clock allowance at all, and that is why every attempt to reproduce it by
loading the host failed.
The fix
driveUntilDone now drives until the saving goroutine finishes, and nothing
else — no turn count, no timeout of its own.
Termination is not lost; it moves to where it belongs, the contract of the code
under test. Every AutoSaveOnSignal returns within the timeout it is handed, so
the saving goroutine always finishes and done always closes. A handoff that
has stopped answering costs one autoSaveWait in total, however many saves were
asked for: g.sigSave is one deep, so an unserviced request stays in the
channel and every later call finds it full and fails immediately. What fails is
then the real assertion — saves taken = 0, want 25 — which says far more than
"out of turns" ever did.
That is not the worst case, and the comment in the test now says so. A handoff
that still drains every request but takes longer than autoSaveWait to do it
costs one timeout per save: wantSaves * autoSaveWait = 250s, which would run
past the 30s package timeout instead of reaching the assertion. It needs about
ten seconds of scheduler starvation per save against the 0.12s-per-1000-turns
regime above, so it is remote, and the 1000-turn cap did not bound it either — a
turn count is not a time bound. go test -timeout 30s is the backstop under all
of it.
A second assumption underneath the first
Removing the cap exposed one, and this is why the diff is not one line.
testTerm answers space and newline for ever once its script is exhausted.
Neither key takes a turn, and command() loops until the player consumes one
(if !g.After { ntimes++ }), so past the end of the script command()never
returns. The old cap was silently sized to the script — 4000 characters, two
consumed per turn, against 1000 turns. My first uncapped attempt duly wedged
inside a single command() call and hit the 30s package timeout; the goroutine
dump showed the drive still inside command() with the saving goroutine already
gone.
The two drive tests therefore take a new driveTerm, a headless terminal whose
script repeats. Repeating is necessary and not sufficient, and the comment
on driveTerm now names the property that is actually load-bearing. Most keys
take a turn only conditionally: ' ' is the "legal" illegal command and clears After outright (game/tables.go:804-805), and all eight movement keys clear
it on a refused step — wall or map edge (game/move.go:74), illegal diagonal
(game/move.go:136), confused step landing in place (game/move.go:31). A
script of only those keys wedges exactly the way testTerm's tail does.
What makes the wedge impossible is that the cycle always contains at least one unconditional turn-taker, and these scripts contain two: '.', whose
handler is empty (game/tables.go:801-803), and 's' / search, which writes After on no path (game/tables.go:770, game/command.go:521-551). Neither
can be refused — not blocked in all eight directions, not Held, not a bear
trap, and not NoCommand > 0, where playTurn skips executeCommand
altogether and After is simply left true. Trim both out and the wedge comes
straight back.
Proof the test is still a real guard
Both mutations were run through make test in this worktree and reverted
afterwards. .golangci.yml is byte-identical (sha256 021cc83f...46bcb) and
nothing under game/testdata/ was touched.
Mutation
Result against the new test
AutoSaveOnSignal body replaced by a direct g.autoSave() — the pre-fix behavior from #24
--- FAIL: TestAutoSaveOnSignalRacesTurnLoop, 139WARNING: DATA RACE reports here, 62-110 on the reviewer's machine
serviceAutoSaveRequest removed from command()
--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.16s): saves taken = 0, want 25
The race trace under the first mutation, trimmed, naming the new drive loop:
WARNING: DATA RACE
Read at 0x00c00058a9e0 by goroutine 318:
game.(*RogueGame).snapshotHeader() game/save.go:260
game.(*RogueGame).snapshot() game/save.go:183
game.(*RogueGame).saveFile() game/save.go:673
game.(*RogueGame).autoSave() game/save.go:852
game.(*RogueGame).AutoSaveOnSignal() game/save.go:746
game.TestAutoSaveOnSignalRacesTurnLoop.func1() game/autosave_test.go:66
Previous write at 0x00c00058a9e0 by goroutine 8:
game.(*RogueGame).executeCommand() game/command.go:163
game.(*RogueGame).playTurn() game/command.go:74
game.(*RogueGame).command() game/command.go:27
game.driveUntilDone() game/autosave_test.go:134
The count is machine-dependent — 139 here, 73/96/113 in #26, 62-110 in review — and the property is
what is pinned, not the number. Review's top trace also showed the racing write
coming from Window.AddCh / look / turnUpkeep rather than executeCommand;
both appear, same class. The second mutation is strictly better than before: it
used to fail by exhausting turns, it now fails in 10s with the count of saves
actually taken, and the package finishes in 10.4s rather than hitting the 30s
timeout.
Under load
The reliable trigger turned out not to be raw host load — with the old code, 24
concurrent unconstrained runs at load ~120 were all green, which matches the
"could not reproduce" observation on the issue. The trigger is Go-scheduler
starvation of the saving goroutine plus the doubled load of the verbose rerun.
So the A/B was run under exactly that: GOMAXPROCS=2, 48-core host at load ~150
(40 spinners), with an unrelated deliberate failure planted in the tree so that every run took the verbose rerun.
Tree
Runs
TestAutoSaveOnSignalRacesTurnLoop failures
main @ 13caec4 (1000-turn cap)
8
8 of 8, all "ran out of turns"
this branch
8
0 of 8 (only the planted failure)
Review reproduced the A/B independently at reduced scale: main 4 of 4, this
branch 0 of 4.
Further runs on this branch, all green, all GOFLAGS=-count=1:
24 concurrent make test at host load ~120 on 48 cores;
10 sequential alongside a spinner load;
5 each at GOMAXPROCS 1, 2 and 4;
8 at GOMAXPROCS=2 under the forced verbose rerun at load ~155.
Verification
make fmt, then make check green: fmt-check + lint (0 issues) + test. Every lint run used a private empty GOLANGCI_LINT_CACHE inside this
worktree, reported no parallel golangci-lint is running, and named no path
outside the worktree. The gomodguard deprecation warning
(#29) is the only warning and is not from
this change.
TODO.md gains a Completed Steps entry in the same commit. Next Step is not
rotated — it is the release step, which needs a human.
Closes https://git.eeqj.de/sneak/rgoue/issues/36.
`TestAutoSaveOnSignalRacesTurnLoop` failed intermittently under load. The
diagnosis captured on the issue is confirmed: it is **not** a data race, it is
the test's own drive loop running out of a fixed 1000-turn budget. The budget is
removed rather than widened.
No non-test code changed. `game/autosave_test.go` and `TODO.md` only.
## The finding, verified
The assertion is `driveUntilDone`'s
`t.Fatal("the turn loop ran out of turns before the saves were taken")`, and no
`WARNING: DATA RACE` appears anywhere.
I did not take that on trust. Instrumenting the loop to report the turns it
actually consumed, and running the suite under varying scheduling pressure,
showed the count tracking contention and nothing else:
| Condition (48-core host, load ~57) | Turns used by 25 saves, cap 1000 |
| --- | --- |
| ambient, 48 procs | 58, 77, 97, 102, 110, 118 |
| `GOMAXPROCS=4` | 105, 418 |
| `GOMAXPROCS=2` | 539, **and one run that exhausted 1000** |
| `GOMAXPROCS=1` | 655 |
The `GOMAXPROCS=2` run reproduced the reported assertion exactly, with zero
`DATA RACE` reports in the log. That is the diagnosis, independently reproduced.
The reason is mechanical: the turns this loop spends between one save being
answered and the next request arriving are not work, they are the saving
goroutine's wake-up latency. So "1000 turns" is a wall-clock assumption in
disguise, and raising it would have hidden the flake rather than removed it.
Review measured each old-code failure at **0.12 seconds** — 1000 turns burned in
a tenth of a second — which is the sharpest statement of that: the cap was never
a wall-clock allowance at all, and that is why every attempt to reproduce it by
loading the host failed.
## The fix
`driveUntilDone` now drives until the saving goroutine finishes, and nothing
else — no turn count, no timeout of its own.
Termination is not lost; it moves to where it belongs, the contract of the code
under test. Every `AutoSaveOnSignal` returns within the timeout it is handed, so
the saving goroutine always finishes and `done` always closes. A handoff that
has stopped answering costs one `autoSaveWait` in total, however many saves were
asked for: `g.sigSave` is one deep, so an unserviced request stays in the
channel and every later call finds it full and fails immediately. What fails is
then the real assertion — `saves taken = 0, want 25` — which says far more than
"out of turns" ever did.
That is not the worst case, and the comment in the test now says so. A handoff
that still drains every request but takes longer than `autoSaveWait` to do it
costs one timeout per save: `wantSaves * autoSaveWait` = 250s, which would run
past the 30s package timeout instead of reaching the assertion. It needs about
ten seconds of scheduler starvation per save against the 0.12s-per-1000-turns
regime above, so it is remote, and the 1000-turn cap did not bound it either — a
turn count is not a time bound. `go test -timeout 30s` is the backstop under all
of it.
## A second assumption underneath the first
Removing the cap exposed one, and this is why the diff is not one line.
`testTerm` answers space and newline for ever once its script is exhausted.
Neither key takes a turn, and `command()` loops until the player consumes one
(`if !g.After { ntimes++ }`), so past the end of the script `command()` **never
returns**. The old cap was silently sized to the script — 4000 characters, two
consumed per turn, against 1000 turns. My first uncapped attempt duly wedged
inside a single `command()` call and hit the 30s package timeout; the goroutine
dump showed the drive still inside `command()` with the saving goroutine already
gone.
The two drive tests therefore take a new `driveTerm`, a headless terminal whose
script repeats. Repeating is necessary and **not** sufficient, and the comment
on `driveTerm` now names the property that is actually load-bearing. Most keys
take a turn only conditionally: `' '` is the "legal" illegal command and clears
`After` outright (`game/tables.go:804-805`), and all eight movement keys clear
it on a refused step — wall or map edge (`game/move.go:74`), illegal diagonal
(`game/move.go:136`), confused step landing in place (`game/move.go:31`). A
script of only those keys wedges exactly the way `testTerm`'s tail does.
What makes the wedge impossible is that the cycle always contains at least one
**unconditional** turn-taker, and these scripts contain two: `'.'`, whose
handler is empty (`game/tables.go:801-803`), and `'s'` / `search`, which writes
`After` on no path (`game/tables.go:770`, `game/command.go:521-551`). Neither
can be refused — not blocked in all eight directions, not `Held`, not a bear
trap, and not `NoCommand > 0`, where `playTurn` skips `executeCommand`
altogether and `After` is simply left true. Trim both out and the wedge comes
straight back.
## Proof the test is still a real guard
Both mutations were run through `make test` in this worktree and reverted
afterwards. `.golangci.yml` is byte-identical (sha256 `021cc83f...46bcb`) and
nothing under `game/testdata/` was touched.
| Mutation | Result against the new test |
| --- | --- |
| `AutoSaveOnSignal` body replaced by a direct `g.autoSave()` — the pre-fix behavior from https://git.eeqj.de/sneak/rgoue/issues/24 | `--- FAIL: TestAutoSaveOnSignalRacesTurnLoop`, **139** `WARNING: DATA RACE` reports here, 62-110 on the reviewer's machine |
| `serviceAutoSaveRequest` removed from `command()` | `--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.16s): saves taken = 0, want 25` |
The race trace under the first mutation, trimmed, naming the new drive loop:
```
WARNING: DATA RACE
Read at 0x00c00058a9e0 by goroutine 318:
game.(*RogueGame).snapshotHeader() game/save.go:260
game.(*RogueGame).snapshot() game/save.go:183
game.(*RogueGame).saveFile() game/save.go:673
game.(*RogueGame).autoSave() game/save.go:852
game.(*RogueGame).AutoSaveOnSignal() game/save.go:746
game.TestAutoSaveOnSignalRacesTurnLoop.func1() game/autosave_test.go:66
Previous write at 0x00c00058a9e0 by goroutine 8:
game.(*RogueGame).executeCommand() game/command.go:163
game.(*RogueGame).playTurn() game/command.go:74
game.(*RogueGame).command() game/command.go:27
game.driveUntilDone() game/autosave_test.go:134
```
The count is machine-dependent — 139 here, 73/96/113 in
https://git.eeqj.de/sneak/rgoue/pulls/26, 62-110 in review — and the property is
what is pinned, not the number. Review's top trace also showed the racing write
coming from `Window.AddCh` / `look` / `turnUpkeep` rather than `executeCommand`;
both appear, same class. The second mutation is strictly better than before: it
used to fail by exhausting turns, it now fails in 10s with the count of saves
actually taken, and the package finishes in 10.4s rather than hitting the 30s
timeout.
## Under load
The reliable trigger turned out not to be raw host load — with the old code, 24
concurrent unconstrained runs at load ~120 were all green, which matches the
"could not reproduce" observation on the issue. The trigger is Go-scheduler
starvation of the saving goroutine plus the doubled load of the verbose rerun.
So the A/B was run under exactly that: `GOMAXPROCS=2`, 48-core host at load ~150
(40 spinners), with an unrelated deliberate failure planted in the tree so that
**every** run took the verbose rerun.
| Tree | Runs | `TestAutoSaveOnSignalRacesTurnLoop` failures |
| --- | --- | --- |
| `main` @ `13caec4` (1000-turn cap) | 8 | **8 of 8**, all "ran out of turns" |
| this branch | 8 | **0 of 8** (only the planted failure) |
Review reproduced the A/B independently at reduced scale: `main` 4 of 4, this
branch 0 of 4.
Further runs on this branch, all green, all `GOFLAGS=-count=1`:
- 24 concurrent `make test` at host load ~120 on 48 cores;
- 10 sequential alongside a spinner load;
- 5 each at `GOMAXPROCS` 1, 2 and 4;
- 8 at `GOMAXPROCS=2` under the forced verbose rerun at load ~155.
## Verification
`make fmt`, then `make check` green: `fmt-check` + `lint` (**0 issues**) +
`test`. Every lint run used a private empty `GOLANGCI_LINT_CACHE` inside this
worktree, reported no `parallel golangci-lint is running`, and named no path
outside the worktree. The `gomodguard` deprecation warning
(https://git.eeqj.de/sneak/rgoue/issues/29) is the only warning and is not from
this change.
`TODO.md` gains a Completed Steps entry in the same commit. `Next Step` is not
rotated — it is the release step, which needs a human.
TestAutoSaveOnSignalRacesTurnLoop failed intermittently under load. The
captured failure text settles what it was: driveUntilDone's
t.Fatal("the turn loop ran out of turns before the saves were taken")
with no WARNING: DATA RACE anywhere in the log. The handoff is fine; the
test's own drive loop ran out of its fixed 1000-turn budget first.
Confirmed by instrumenting the loop to report the turns it actually
used. The count tracks scheduling pressure and nothing else: about
60-120 turns at host load ~57 with the whole machine to spread over, 418
at GOMAXPROCS=4, 539 and 655 at 2 and 1, and past 1000 under the doubled
load of the verbose rerun the test target performs after a failure. The
turns spent between one save being answered and the next request
arriving are not work, they are the saving goroutine's wake-up latency,
so a fixed turn count is a wall-clock assumption in disguise. Raising it
would hide the flake, not fix it.
So the budget is gone rather than larger. driveUntilDone drives until
the saving goroutine finishes and nothing else. Termination still holds,
it just belongs to the code under test: every AutoSaveOnSignal returns
within the timeout it is handed, and g.sigSave is one deep, so an
unserviced request stays in the channel and every later call finds it
full and fails at once. A dead handoff therefore releases the saving
goroutine after one autoSaveWait however many saves were asked for, and
what fails is the real assertion - "saves taken = 0, want 25" - rather
than "out of turns".
Removing the cap exposed a second assumption underneath it. testTerm
answers space and newline for ever once its script is exhausted, and
neither key takes a turn, so command(), which loops until the player
consumes one, never returns; the old cap was silently sized to the
script. An uncapped drive wedged inside a single command() call. The two
drive tests now use driveTerm, a headless terminal whose script repeats,
so every key it hands out takes a turn.
The guard is undiminished, shown by mutation and reverted afterwards.
Reverting the fix from the earlier signal-autosave work - AutoSaveOnSignal
replaced by a direct g.autoSave(), encoding on the calling goroutine -
still fails the test with 139 DATA RACE reports, snapshotHeader reading
what executeCommand writes. Removing serviceAutoSaveRequest from
command() still fails it too, now in 10s with "saves taken = 0, want 25"
instead of by hanging.
Under load: at GOMAXPROCS=2 on a 48-core host at load ~150, with an
unrelated deliberate failure in the tree so every run took the verbose
rerun, the old code failed 8 of 8 runs and the new code 0 of 8. Also
green across 24 concurrent unconstrained runs at load ~120, 10 runs
alongside a spinner load, and 5 runs each at GOMAXPROCS 1, 2 and 4.
No non-test code changed. make check green, lint 0 issues, .golangci.yml
byte-identical.
The captured finding on #36 holds, and I
reproduced it rather than assuming it.
Instrumenting driveUntilDone to report the turns it actually consumed, then
varying scheduling pressure, gave:
Condition (48-core host, load ~57)
Turns used by 25 saves, cap 1000
ambient, 48 procs
58, 77, 97, 102, 110, 118
GOMAXPROCS=4
105, 418
GOMAXPROCS=2
539, and one run that hit the cap
GOMAXPROCS=1
655
The GOMAXPROCS=2 run that hit the cap produced exactly autosave_test.go:63: the turn loop ran out of turns before the saves were taken
with zeroWARNING: DATA RACE in the log. Not a race, and no residual race
found behind it — so nothing to report against #24 and no non-test code was touched.
One correction to the brief I was given, which is worth recording: it framed the autoSaveWait = 10s / fixed-turn-count split as the asymmetry to fix, implying a
wall-clock bound on the drive was the answer. A wall-clock bound is the wrong
shape here. AutoSaveOnSignal already gives up after autoSaveWait, so a drive
watchdog set to the same value is a photo finish with it: I built that version
first, and which of the two fired — the watchdog's message or the real saves taken assertion — came out a coin flip between runs. That is a new flake
traded for the old one. The drive needs no time bound of its own at all; see
below.
The fix
driveUntilDone drives until the saving goroutine finishes, and nothing else —
no turn count, no timeout.
Termination moves to where it belongs, the contract of the code under test. Every AutoSaveOnSignal returns within the timeout it is handed, and g.sigSave is one
deep, so an unserviced request stays in the channel and every later call finds it
full and fails at once. A dead handoff releases the saving goroutine after one autoSaveWait however many saves were asked for, done closes, and what fails is
the real assertion — saves taken = 0, want 25. go test -timeout 30s is the
backstop under that.
The second assumption underneath the cap
Worth flagging for review because it is not obvious. testTerm answers space and
newline for ever once its script is exhausted; neither takes a turn, and command() loops until the player consumes one (if !g.After { ntimes++ }), so
past the end of the script command() never returns. The 1000-turn cap was
silently sized to the 4000-character script. My first uncapped attempt wedged
inside a single command() call and hit the 30s package timeout — the goroutine
dump showed the drive still inside command() with the saving goroutine already
gone. The two drive tests now use driveTerm, whose script repeats, so every key
takes a turn.
Proof the test still catches the reverted fix
Both mutations run through make test and reverted afterwards; .golangci.yml
byte-identical (sha256 021cc83f...46bcb), nothing under game/testdata/
touched.
Mutation
Result against the new test
AutoSaveOnSignal body replaced by a direct g.autoSave() — encode on the calling goroutine, the pre-fix behavior
--- FAIL: TestAutoSaveOnSignalRacesTurnLoop, 139WARNING: DATA RACE reports, snapshotHeader reading what executeCommand writes
serviceAutoSaveRequest removed from command()
--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.16s): saves taken = 0, want 25
139 sits in the 73-113-96 band #26 measured;
the property is pinned, not the number. The second mutation is strictly better
reported than before: it used to fail by exhausting turns, it now names the count
of saves actually taken, and the package finishes in 10.4s instead of timing out.
Under load
Raw host load alone does not do it — with the old code, 24 concurrent
unconstrained runs at load ~120 were all green, matching the
"could not reproduce" observation on the issue. The trigger is Go-scheduler
starvation of the saving goroutine, plus the doubled load of the verbose rerun.
A/B under exactly that: GOMAXPROCS=2, 48-core host at load ~150, with an
unrelated deliberate failure planted so every run took the verbose rerun.
Tree
Runs
TestAutoSaveOnSignalRacesTurnLoop failures
main @ 13caec4 (1000-turn cap)
8
8 of 8, all "ran out of turns"
this branch
8
0 of 8 (only the planted failure)
Further runs on this branch, all green, all GOFLAGS=-count=1: 24 concurrent make test at load ~120; 10 sequential alongside a spinner load; 5 each at GOMAXPROCS 1, 2 and 4.
Gate
make fmt, then make check green — fmt-check + lint (0 issues) + test. Lint ran against a private empty GOLANGCI_LINT_CACHE inside this
worktree, reported no parallel golangci-lint is running, and named no path
outside the worktree. The gomodguard deprecation warning
(#29) is the only warning and is not mine.
## Diagnosis confirmation
The captured finding on https://git.eeqj.de/sneak/rgoue/issues/36 holds, and I
reproduced it rather than assuming it.
Instrumenting `driveUntilDone` to report the turns it actually consumed, then
varying scheduling pressure, gave:
| Condition (48-core host, load ~57) | Turns used by 25 saves, cap 1000 |
| --- | --- |
| ambient, 48 procs | 58, 77, 97, 102, 110, 118 |
| `GOMAXPROCS=4` | 105, 418 |
| `GOMAXPROCS=2` | 539, and one run that hit the cap |
| `GOMAXPROCS=1` | 655 |
The `GOMAXPROCS=2` run that hit the cap produced exactly
`autosave_test.go:63: the turn loop ran out of turns before the saves were taken`
with **zero** `WARNING: DATA RACE` in the log. Not a race, and no residual race
found behind it — so nothing to report against
https://git.eeqj.de/sneak/rgoue/issues/24 and no non-test code was touched.
One correction to the brief I was given, which is worth recording: it framed the
`autoSaveWait = 10s` / fixed-turn-count split as the asymmetry to fix, implying a
wall-clock bound on the drive was the answer. A wall-clock bound is the wrong
shape here. `AutoSaveOnSignal` already gives up after `autoSaveWait`, so a drive
watchdog set to the same value is a photo finish with it: I built that version
first, and which of the two fired — the watchdog's message or the real
`saves taken` assertion — came out a coin flip between runs. That is a new flake
traded for the old one. The drive needs no time bound of its own at all; see
below.
## The fix
`driveUntilDone` drives until the saving goroutine finishes, and nothing else —
no turn count, no timeout.
Termination moves to where it belongs, the contract of the code under test. Every
`AutoSaveOnSignal` returns within the timeout it is handed, and `g.sigSave` is one
deep, so an unserviced request stays in the channel and every later call finds it
full and fails at once. A dead handoff releases the saving goroutine after one
`autoSaveWait` however many saves were asked for, `done` closes, and what fails is
the real assertion — `saves taken = 0, want 25`. `go test -timeout 30s` is the
backstop under that.
## The second assumption underneath the cap
Worth flagging for review because it is not obvious. `testTerm` answers space and
newline for ever once its script is exhausted; neither takes a turn, and
`command()` loops until the player consumes one (`if !g.After { ntimes++ }`), so
past the end of the script `command()` never returns. The 1000-turn cap was
silently sized to the 4000-character script. My first uncapped attempt wedged
inside a single `command()` call and hit the 30s package timeout — the goroutine
dump showed the drive still inside `command()` with the saving goroutine already
gone. The two drive tests now use `driveTerm`, whose script repeats, so every key
takes a turn.
## Proof the test still catches the reverted fix
Both mutations run through `make test` and reverted afterwards; `.golangci.yml`
byte-identical (sha256 `021cc83f...46bcb`), nothing under `game/testdata/`
touched.
| Mutation | Result against the new test |
| --- | --- |
| `AutoSaveOnSignal` body replaced by a direct `g.autoSave()` — encode on the calling goroutine, the pre-fix behavior | `--- FAIL: TestAutoSaveOnSignalRacesTurnLoop`, **139** `WARNING: DATA RACE` reports, `snapshotHeader` reading what `executeCommand` writes |
| `serviceAutoSaveRequest` removed from `command()` | `--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.16s): saves taken = 0, want 25` |
139 sits in the 73-113-96 band https://git.eeqj.de/sneak/rgoue/pulls/26 measured;
the property is pinned, not the number. The second mutation is strictly better
reported than before: it used to fail by exhausting turns, it now names the count
of saves actually taken, and the package finishes in 10.4s instead of timing out.
## Under load
Raw host load alone does not do it — with the old code, 24 concurrent
unconstrained runs at load ~120 were all green, matching the
"could not reproduce" observation on the issue. The trigger is Go-scheduler
starvation of the saving goroutine, plus the doubled load of the verbose rerun.
A/B under exactly that: `GOMAXPROCS=2`, 48-core host at load ~150, with an
unrelated deliberate failure planted so every run took the verbose rerun.
| Tree | Runs | `TestAutoSaveOnSignalRacesTurnLoop` failures |
| --- | --- | --- |
| `main` @ `13caec4` (1000-turn cap) | 8 | **8 of 8**, all "ran out of turns" |
| this branch | 8 | **0 of 8** (only the planted failure) |
Further runs on this branch, all green, all `GOFLAGS=-count=1`: 24 concurrent
`make test` at load ~120; 10 sequential alongside a spinner load; 5 each at
`GOMAXPROCS` 1, 2 and 4.
## Gate
`make fmt`, then `make check` green — `fmt-check` + `lint` (**0 issues**) +
`test`. Lint ran against a private empty `GOLANGCI_LINT_CACHE` inside this
worktree, reported no `parallel golangci-lint is running`, and named no path
outside the worktree. The `gomodguard` deprecation warning
(https://git.eeqj.de/sneak/rgoue/issues/29) is the only warning and is not mine.
Verified at head cb7ddfd against #36. No non-test code changed: game/autosave_test.go and TODO.md only.
Reproduced
Mutation A (AutoSaveOnSignal body replaced by a direct g.autoSave()): --- FAIL: TestAutoSaveOnSignalRacesTurnLoop, 62WARNING: DATA RACE in
the first run (110 across run plus verbose rerun), traces snapshotHeader / snapshot / saveFile / autoSave / AutoSaveOnSignal
at autosave_test.go:57 against the drive at autosave_test.go:116. My count
is not 139; #26 measured 73/96/113 and
the number is machine-dependent — the property is what is pinned. One nuance
on the PR body's wording: in my top trace the racing write comes from Window.AddCh / look / turnUpkeep, not executeCommand; both appear,
same class.
Mutation B (serviceAutoSaveRequest removed from command()): --- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.02s): saves taken = 0, want 25,
plus the saved file does not restore, package finishing in 10.057s. Exactly
as reported — a diagnosable assertion, not a timeout.
A/B under load, scaled down: GOMAXPROCS=2, 20 spinners, host load ~35 on
48 cores, an unrelated planted failing test forcing the verbose rerun on every
run, 4 runs per tree, alternating. main @ 13caec4: 4 of 4 failed with
"ran out of turns". This branch: 0 of 4 (only the planted failure). Each
old failure took 0.12s — 1000 turns burned in a tenth of a second, which
states the diagnosis more sharply than "too slow under load" does: the budget
was never a wall-clock allowance at all.
Findings
Both non-blocking, and both about stated reasoning rather than behavior.
1. game/autosave_test.go:38-39, :464-471, and the commit message: "every
key it hands out takes a turn" is false. The guarantee rests on two keys, not on
the script repeating.
Half the shipped script is spaces, and ' ' is explicitly g.After = false
(game/tables.go:804-805, the "legal illegal command"). All eight movement keys
clear After as well when the step is refused: wall or off-map
(game/move.go:74), illegal diagonal (game/move.go:136), a confused step
landing in place (game/move.go:31). What actually makes the wedge impossible
is that the cycle contains two unconditional turn-takers — '.' (empty
handler, game/tables.go:801-803) and 's' / search
(game/command.go:521-551, no After write on any path).
I checked those two against the states you asked about: blocked in all eight
directions; Held (game/move.go:152-156 refuses the move without clearing After); bear trap (game/move.go:20-25); NoCommand > 0, where playTurn
skips executeCommand entirely (game/command.go:62-75) so After stays true,
and fortify zeroes NoCommand/NoMove every iteration anyway; maze and
passage squares; and a pending prompt, where --More-- drains through waitForSpace, which the spaces answer. readCommand also cannot stop reading
from the terminal, because Running, ToDeath and Count are only ever set by
keys absent from this script (game/move.go:8, game/command.go:359, game/command.go:180).
Probe: I set the script to " " — still repeating — and it wedged exactly the
way testTerm's tail does, panic: test timed out after 30s with the drive
inside command() / readCommand. So "the script repeats" is not the property
that saves it; "the cycle contains an unconditional turn-taker" is. The
conclusion in this PR is correct, but the reason written into the test is not,
and this comment is now where a future reader will go to learn why the drive
terminates — which is precisely the class of latent assumption the change exists
to remove.
Related, same paragraph: driveTerm's script contains no newline, where testTerm's tail supplied '\n' on every other read. Nothing reachable from
this script needs one — waitFor('\n') sits on the death and score paths
(game/rip.go:69, game/score.go:106) that fortify prevents, and getStr's endsInput is reached only from commands not in the mix — so it is not a defect
today. It is an undocumented precondition of the new fake.
2. game/autosave_test.go:91-101 and the commit message: "a dead handoff
releases the saving goroutine after one autoSaveWait however many saves were
asked for" holds only for a permanently dead handoff.
Enumerating the termination paths: AutoSaveOnSignal itself always returns —
the post is non-blocking (game/save.go:748-755), Interrupt is a no-op on driveTerm (game/autosave_test.go:485), and the wait is a select on req.done against a timer (game/save.go:759-767). A fully dead handoff costs
one autoSaveWait then 24 instant failures, confirmed empirically at 10.02s by
mutation B above. But a live-but-slow handoff — one that does drain each
request, only after more than autoSaveWait — lets each call time out and the
next post succeed, for a worst case of wantSaves * autoSaveWait = 250s, i.e. driveUntilDone waiting out the 30s package timeout. That needs roughly 10s of
scheduler starvation per save against the 0.12s-per-1000-turns regime measured
above, so it is remote, and it is not a regression: the 1000-turn cap was not a
time bound either. It is the one gap in the "no bound of its own" argument, and
the comment states the bound more strongly than it holds.
The rejection of the wall-clock watchdog is sound on its own terms; a drive
deadline equal to autoSaveWait really is a photo finish with AutoSaveOnSignal's own timer, and I would not have accepted that version
either.
Checked and clean
Diff is game/autosave_test.go plus TODO.md only; t.Parallel() and the //nolint:testpackage header intact with no new nolints; TODO.md gains a
Completed Steps entry and "Next Step" is still the release step; .golangci.yml
sha256 021cc83f...46bcb and absent from the diff; nothing under game/testdata/; no Dockerfile, CI or script/; no Claude or Anthropic
reference and no attribution trailer anywhere in the diff or commit; commit
title ends (closes #36); git diff --check clean; fast-forwardable onto origin/main; make check green — fmt-check, lint 0 issues (private
empty GOLANGCI_LINT_CACHE, no parallel golangci-lint is running, no path
outside my worktree, gomodguard deprecation only), tests pass — plus two
further clean GOFLAGS=-count=1 make test runs and the 8 full-suite runs from
the A/B, all race-clean. This repo has no CI by design, so the head commit
carries no statuses, as main does not. driveTerm is used only by the two
drive tests and holds no shared state, so it cannot starve anything else in the
package.
Both mutations, the space-script probe and the planted A/B failure were run in a
throwaway worktree and reverted; the shared clone is untouched.
## Independent review — PASS
Verified at head `cb7ddfd` against
https://git.eeqj.de/sneak/rgoue/issues/36. No non-test code changed:
`game/autosave_test.go` and `TODO.md` only.
## Reproduced
- **Mutation A** (`AutoSaveOnSignal` body replaced by a direct `g.autoSave()`):
`--- FAIL: TestAutoSaveOnSignalRacesTurnLoop`, **62** `WARNING: DATA RACE` in
the first run (110 across run plus verbose rerun), traces
`snapshotHeader` / `snapshot` / `saveFile` / `autoSave` / `AutoSaveOnSignal`
at `autosave_test.go:57` against the drive at `autosave_test.go:116`. My count
is not 139; https://git.eeqj.de/sneak/rgoue/pulls/26 measured 73/96/113 and
the number is machine-dependent — the property is what is pinned. One nuance
on the PR body's wording: in my top trace the racing write comes from
`Window.AddCh` / `look` / `turnUpkeep`, not `executeCommand`; both appear,
same class.
- **Mutation B** (`serviceAutoSaveRequest` removed from `command()`):
`--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.02s): saves taken = 0, want 25`,
plus `the saved file does not restore`, package finishing in 10.057s. Exactly
as reported — a diagnosable assertion, not a timeout.
- **A/B under load**, scaled down: `GOMAXPROCS=2`, 20 spinners, host load ~35 on
48 cores, an unrelated planted failing test forcing the verbose rerun on every
run, 4 runs per tree, alternating. `main` @ `13caec4`: **4 of 4** failed with
"ran out of turns". This branch: **0 of 4** (only the planted failure). Each
old failure took **0.12s** — 1000 turns burned in a tenth of a second, which
states the diagnosis more sharply than "too slow under load" does: the budget
was never a wall-clock allowance at all.
## Findings
Both non-blocking, and both about stated reasoning rather than behavior.
**1. `game/autosave_test.go:38-39`, `:464-471`, and the commit message: "every
key it hands out takes a turn" is false. The guarantee rests on two keys, not on
the script repeating.**
Half the shipped script is spaces, and `' '` is explicitly `g.After = false`
(`game/tables.go:804-805`, the "legal illegal command"). All eight movement keys
clear `After` as well when the step is refused: wall or off-map
(`game/move.go:74`), illegal diagonal (`game/move.go:136`), a confused step
landing in place (`game/move.go:31`). What actually makes the wedge impossible
is that the cycle contains two *unconditional* turn-takers — `'.'` (empty
handler, `game/tables.go:801-803`) and `'s'` / `search`
(`game/command.go:521-551`, no `After` write on any path).
I checked those two against the states you asked about: blocked in all eight
directions; `Held` (`game/move.go:152-156` refuses the move without clearing
`After`); bear trap (`game/move.go:20-25`); `NoCommand > 0`, where `playTurn`
skips `executeCommand` entirely (`game/command.go:62-75`) so `After` stays true,
and `fortify` zeroes `NoCommand`/`NoMove` every iteration anyway; maze and
passage squares; and a pending prompt, where `--More--` drains through
`waitForSpace`, which the spaces answer. `readCommand` also cannot stop reading
from the terminal, because `Running`, `ToDeath` and `Count` are only ever set by
keys absent from this script (`game/move.go:8`, `game/command.go:359`,
`game/command.go:180`).
Probe: I set the script to `" "` — still repeating — and it wedged exactly the
way `testTerm`'s tail does, `panic: test timed out after 30s` with the drive
inside `command()` / `readCommand`. So "the script repeats" is not the property
that saves it; "the cycle contains an unconditional turn-taker" is. The
conclusion in this PR is correct, but the reason written into the test is not,
and this comment is now where a future reader will go to learn why the drive
terminates — which is precisely the class of latent assumption the change exists
to remove.
Related, same paragraph: `driveTerm`'s script contains no newline, where
`testTerm`'s tail supplied `'\n'` on every other read. Nothing reachable from
this script needs one — `waitFor('\n')` sits on the death and score paths
(`game/rip.go:69`, `game/score.go:106`) that `fortify` prevents, and `getStr`'s
`endsInput` is reached only from commands not in the mix — so it is not a defect
today. It is an undocumented precondition of the new fake.
**2. `game/autosave_test.go:91-101` and the commit message: "a dead handoff
releases the saving goroutine after one `autoSaveWait` however many saves were
asked for" holds only for a *permanently* dead handoff.**
Enumerating the termination paths: `AutoSaveOnSignal` itself always returns —
the post is non-blocking (`game/save.go:748-755`), `Interrupt` is a no-op on
`driveTerm` (`game/autosave_test.go:485`), and the wait is a `select` on
`req.done` against a timer (`game/save.go:759-767`). A fully dead handoff costs
one `autoSaveWait` then 24 instant failures, confirmed empirically at 10.02s by
mutation B above. But a *live-but-slow* handoff — one that does drain each
request, only after more than `autoSaveWait` — lets each call time out and the
next post succeed, for a worst case of `wantSaves * autoSaveWait` = 250s, i.e.
`driveUntilDone` waiting out the 30s package timeout. That needs roughly 10s of
scheduler starvation per save against the 0.12s-per-1000-turns regime measured
above, so it is remote, and it is not a regression: the 1000-turn cap was not a
time bound either. It is the one gap in the "no bound of its own" argument, and
the comment states the bound more strongly than it holds.
The rejection of the wall-clock watchdog is sound on its own terms; a drive
deadline equal to `autoSaveWait` really is a photo finish with
`AutoSaveOnSignal`'s own timer, and I would not have accepted that version
either.
## Checked and clean
Diff is `game/autosave_test.go` plus `TODO.md` only; `t.Parallel()` and the
`//nolint:testpackage` header intact with no new nolints; `TODO.md` gains a
Completed Steps entry and "Next Step" is still the release step; `.golangci.yml`
sha256 `021cc83f...46bcb` and absent from the diff; nothing under
`game/testdata/`; no Dockerfile, CI or `script/`; no Claude or Anthropic
reference and no attribution trailer anywhere in the diff or commit; commit
title ends ` (closes #36)`; `git diff --check` clean; fast-forwardable onto
`origin/main`; `make check` green — `fmt-check`, lint **0 issues** (private
empty `GOLANGCI_LINT_CACHE`, no `parallel golangci-lint is running`, no path
outside my worktree, `gomodguard` deprecation only), tests pass — plus two
further clean `GOFLAGS=-count=1 make test` runs and the 8 full-suite runs from
the A/B, all race-clean. This repo has no CI by design, so the head commit
carries no statuses, as `main` does not. `driveTerm` is used only by the two
drive tests and holds no shared state, so it cannot starve anything else in the
package.
Both mutations, the space-script probe and the planted A/B failure were run in a
throwaway worktree and reverted; the shared clone is untouched.
Manager notes (the review is in its own comment above).
I am overruling the reviewer's "land it as-is" and sending this back for a
comment-only rework. The finding is right; I disagree with its weight, and I
want to explain why rather than just applying a label.
The defect: a correct conclusion resting on a false stated reason. game/autosave_test.go:38-39, :464-471 and the commit message say the
repeating script works because "every key it hands out takes a turn". That is
false, and the reviewer proved it: half the script is spaces, ' ' is
explicitly g.After = false (tables.go:804-805), and all eight movement
keys clear After on a refused step (move.go:74, :136, :31). What
actually makes the wedge impossible is much narrower — the cycle contains two unconditional turn-takers, '.' and 's', verified to survive blocked-all-
directions, Held, bear trap, NoCommand>0, maze, and a pending --More--.
Why that is blocking rather than a follow-up. This comment is the safety
argument for the fix. Someone later trimming the script to movement keys —
entirely reasonable if you believe "every key takes a turn" — reintroduces the
exact 30s wedge this PR exists to remove. The reviewer even demonstrated it:
setting the script to " " wedges precisely like testTerm did, panic: test timed out after 30s.
So the false sentence is not decoration; it is a trap aimed at the next
person, in the file whose whole subject is a trap that caught five sessions.
And consistency matters here. I blocked PR #26 five times, PR #37, and
PR #38 on exactly this class: a claim recorded as fact that nobody had
verified. Waiving it now, on a PR that is otherwise the best work in this
backlog, would say the standard applies to weak work and bends for strong
work. It is a two-comment edit.
Second item, same class: the unbounded-wait comment states the bound more
strongly than it holds. A live-but-slow handoff (draining each request but
slower than autoSaveWait) gives a worst case of wantSaves × autoSaveWait =
250s, not one autoSaveWait. Remote — it needs ~10s of starvation per save
against a measured 0.12s per 1000 turns — and not a regression, but the
comment should say what is true.
None of this touches the substance, which is excellent and is not being
re-litigated:
The guard is intact: reverting the #24 fix still produces 62-110 DATA RACE
reports (the reviewer's count differs from the author's 139 and PR #26's
73/96/113 — machine-dependent, and the property is what is pinned, not the
number).
Removing the service point fails in 10.02s with a diagnosable saves taken = 0, want 25 instead of a timeout — the fix converted an
unreadable hang into an assertion.
The A/B reproduced independently at reduced scale: main 4/4 "ran out of
turns", branch 0/4.
One number in the review strengthens the diagnosis beyond how the author
stated it: each old failure took 0.12 seconds — 1000 turns in a tenth of
a second. The cap was never a wall-clock allowance at all, which is why every
earlier attempt to reproduce it by loading the host failed. That belongs in
the record.
Rework brief: correct the two comment claims and the commit message. Nothing
else.
Manager notes (the review is in its own comment above).
**I am overruling the reviewer's "land it as-is" and sending this back for a
comment-only rework.** The finding is right; I disagree with its weight, and I
want to explain why rather than just applying a label.
**The defect: a correct conclusion resting on a false stated reason.**
`game/autosave_test.go:38-39`, `:464-471` and the commit message say the
repeating script works because "every key it hands out takes a turn". That is
false, and the reviewer proved it: half the script is spaces, `' '` is
explicitly `g.After = false` (`tables.go:804-805`), and all eight movement
keys clear `After` on a refused step (`move.go:74`, `:136`, `:31`). What
actually makes the wedge impossible is much narrower — the cycle contains two
**unconditional** turn-takers, `'.'` and `'s'`, verified to survive blocked-all-
directions, `Held`, bear trap, `NoCommand>0`, maze, and a pending `--More--`.
**Why that is blocking rather than a follow-up.** This comment is the safety
argument for the fix. Someone later trimming the script to movement keys —
entirely reasonable if you believe "every key takes a turn" — reintroduces the
exact 30s wedge this PR exists to remove. The reviewer even demonstrated it:
setting the script to `" "` wedges precisely like `testTerm` did,
`panic: test timed out after 30s`.
So the false sentence is not decoration; it is a trap aimed at the next
person, in the file whose whole subject is a trap that caught five sessions.
**And consistency matters here.** I blocked PR #26 five times, PR #37, and
PR #38 on exactly this class: a claim recorded as fact that nobody had
verified. Waiving it now, on a PR that is otherwise the best work in this
backlog, would say the standard applies to weak work and bends for strong
work. It is a two-comment edit.
**Second item, same class:** the unbounded-wait comment states the bound more
strongly than it holds. A live-but-slow handoff (draining each request but
slower than `autoSaveWait`) gives a worst case of `wantSaves × autoSaveWait` =
250s, not one `autoSaveWait`. Remote — it needs ~10s of starvation per save
against a measured 0.12s per 1000 turns — and not a regression, but the
comment should say what is true.
**None of this touches the substance, which is excellent and is not being
re-litigated:**
- The guard is intact: reverting the #24 fix still produces 62-110 `DATA RACE`
reports (the reviewer's count differs from the author's 139 and PR #26's
73/96/113 — machine-dependent, and the *property* is what is pinned, not the
number).
- Removing the service point fails in 10.02s with a diagnosable
`saves taken = 0, want 25` instead of a timeout — the fix converted an
unreadable hang into an assertion.
- The A/B reproduced independently at reduced scale: `main` 4/4 "ran out of
turns", branch 0/4.
**One number in the review strengthens the diagnosis beyond how the author
stated it:** each old failure took **0.12 seconds** — 1000 turns in a tenth of
a second. The cap was never a wall-clock allowance at all, which is why every
earlier attempt to reproduce it by loading the host failed. That belongs in
the record.
Rework brief: correct the two comment claims and the commit message. Nothing
else.
Both findings accepted, neither rebutted. Both were the same defect — a claim
recorded as fact — and both now say what the source actually does. No test logic
and no game code changed: against cb7ddfd the diff is comments, the commit
message, TODO.md prose, and this PR's body. make check re-run whole after
the edits.
Verified against the source before writing a word
I checked every claim myself rather than transcribing the brief. All of it held;
I found nothing wrong in it. What I read, and what it says:
the eight movement keys clear After on a refused step
game/move.go:74, :136, :31
wall or off-map in moveResolve, illegal diagonal in moveTarget, confused step landing in place in moveHero. Confirmed — and game/move.go:143 is a fourth, a run into the hero's own square.
'.' is an unconditional turn-taker
game/tables.go:801-803
handler body is empty. dispatchKey (game/command.go:221-226) finds it in commandHandlers, calls it and returns, so nothing downstream can clear After. Confirmed.
's' is an unconditional turn-taker
game/tables.go:770, game/command.go:521-551
's': (*RogueGame).search; neither search, nor searchSpot, nor look writes After on any path. Confirmed by reading every g.After = site in the package — they are in armor.go, things.go, potions.go, move.go, options.go, tables.go and command.go, none of them in the search chain.
NoCommand > 0 leaves After true
game/command.go:62-75, :117
playTurn takes the if g.NoCommand != 0 arm and skips executeCommand entirely; After was set true in turnUpkeep and nothing clears it. Confirmed.
a bear trap and Held do not clear it either
game/move.go:20-25, :156-159
both return without touching After. Confirmed.
driveTerm supplies no newline, and waitFor('\n') would wedge on it
waitFor('\n') loops on readchar until '\n' or '\r', which this script never yields. Both call sites are the death and score paths, which fortify (game/run_test.go:14-22) prevents reaching. Confirmed unreachable, and now documented.
F1: the safety argument, rewritten
The false sentence is gone from all four places it appeared — the test's script
comment, the driveTerm doc, the commit message and the PR body. driveTerm's
doc now carries the argument, and it is the one that survives checking:
> Repeating the script is necessary but nowhere near sufficient, and the
> difference is what anyone editing one of these scripts has to know. Most keys
> take a turn only conditionally. ' ' is the "legal" illegal command and
> clears After outright (tables.go). All eight movement keys clear it whenever
> the step is refused: a wall or the map edge (move.go moveResolve), an
> illegal diagonal (moveTarget), or a confused step that lands back in place
> (moveHero). A script of nothing but those keys wedges exactly the way
> testTerm's tail does, repetition or no repetition — with the script set to
> just " " this drive hits the 30s package timeout inside command().
>
> What actually makes the wedge impossible is that the cycle always contains at
> least one unconditional turn-taker, and the scripts here carry two: '.',
> the rest command, whose handler is empty, and 's', search, which writes
> After on no path. Nothing refuses either one — not being blocked in all
> eight directions, not Held, not stuck in a bear trap, and not
> NoCommand > 0, where playTurn skips executeCommand altogether and
> After is simply left true. Trim both out and the wedge this test exists to
> remove comes straight back.
The " " probe result is the reviewer's, credited as such in my report rather
than claimed as my own run; everything else in that paragraph I read out of the
files listed above.
The script line in TestAutoSaveOnSignalRacesTurnLoop now points at it instead
of asserting the false general rule:
> The '.' and the 's' are what make an unbounded drive safe, and at least
> one of the two has to stay in the cycle: see driveTerm.
One deliberate difference from the brief's phrasing: "removing both would
reintroduce the wedge" is what I wrote, not "removing either". Removing one of
the two is survivable — one unconditional turn-taker is enough — and the test's
other drive script ("s . ", in TestAutoSaveOnSignalWithoutASaveFile) has
both as well. The invariant is "at least one", and that is what both comments
state.
F1, folded in: the missing newline
Same paragraph, as an explicit precondition of the fake rather than an accident:
> One further precondition, from what this fake does not supply: testTerm's
> tail answered a newline every other read and this does not. Nothing reachable
> from these scripts asks for one — waitFor('\n') sits on the death and score
> paths (rip.go, score.go), which fortify prevents from ever being reached —
> but a script that could reach them would park in waitFor for ever.
F2: the bound stated as it actually holds
driveUntilDone's doc no longer claims one autoSaveWait covers every case:
> Dropping it costs no termination guarantee, because the bound belongs to the
> code under test and not to this loop: each AutoSaveOnSignal call returns
> within the timeout the caller hands it, so the saving goroutine always
> finishes and done always closes. That bound is worth stating exactly,
> because it is not one autoSaveWait.
>
> A handoff that has stopped answering altogether costs one, in total, however
> many saves were asked for. g.sigSave is one deep, so the unserviced request
> stays in the channel and every later call finds it full and reports failure
> immediately — measured at 10.0s for 25 saves with the service point deleted
> from command(). What fails is then the caller's own assertion, the count of
> saves actually taken, which says far more than "out of turns" ever did.
>
> A handoff that still drains every request but takes longer than
> autoSaveWait to do it is the worst case, and costs one timeout per save:
> wantSaves * autoSaveWait, 250s at these constants, which would run past the
> package timeout rather than reach the assertion. It takes about ten seconds of
> scheduler starvation per save to get there, against a regime measured at 0.12s
> per 1000 turns, so it is remote — and the 1000-turn cap did not bound it
> either, a turn count being no kind of time bound. go test -timeout 30s is
> the backstop under all of it.
I re-derived the 250s rather than taking it: the post is non-blocking and only
fails when sigSave is already full, so a handoff that eventually drains each
request leaves the channel empty for the next call, which then gets its own full autoSaveWait. 25 x 10s. The 10.0s figure is the reviewer's measurement of
mutation B, credited; my own run of the same mutation was 10.16s.
The autoSaveWait const comment no longer implies a single wait either — it now
points at driveUntilDone "for what that bound comes to".
Also corrected in the record
Commit message: both claims rewritten to match the above, and the 0.12s
figure added — 1000 turns burned in a tenth of a second, which states the
diagnosis better than "too slow under load" and explains why loading the host
never reproduced it. The DATA RACE count is now given as a range across
machines (139 here, 62-110 in review, 73/96/113 in #26) with the property named as the thing
pinned, rather than as a single number.
TODO.md: the Completed Steps entry repeated both false claims; both
paragraphs are corrected in the same commit. The entry is kept and "Next Step"
is untouched — still the release step.
This PR's body: same two passages corrected, plus the reviewer's trace
nuance (their top trace shows the racing write from Window.AddCh / look / turnUpkeep, not executeCommand; both appear, same class) and their
independent 4/4-vs-0/4 A/B. Strictly speaking outside "comments and the commit
message", but it carried the identical false sentence and it is my own text;
flagging it here so it is a visible choice rather than a silent one.
Gate
make fmt, then make check re-run in full after the last edit — it
short-circuits, so nothing was carried over from the earlier run.
prettier --tab-width 4 --prose-wrap always --check ...
All matched files use Prettier code style!
golangci-lint run ./...
level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) ..."
0 issues.
ok git.eeqj.de/sneak/rgoue/cmd/rogue (cached) coverage: 29.7% of statements
ok git.eeqj.de/sneak/rgoue/game 2.491s coverage: 64.0% of statements
The game package — the only one this touches — ran for real at 2.491s rather
than reporting cached. Lint used a private empty GOLANGCI_LINT_CACHE under my
own throwaway worktree, reported no parallel golangci-lint is running, and
named no path outside it; the gomodguard deprecation
(#29) is the only warning and predates
this branch.
.golangci.yml still sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb and absent
from the diff; nothing under game/testdata/; no Dockerfile, CI or script/; git diff --check clean; still fast-forwardable onto origin/main @ 13caec4;
still one commit whose title ends (closes #36), amended and force-pushed with --force-with-lease. Work was done in a throwaway worktree and the shared clone
is untouched on main.
## Rework: F1 and F2 corrected, head now `3061931`
Both findings accepted, neither rebutted. Both were the same defect — a claim
recorded as fact — and both now say what the source actually does. No test logic
and no game code changed: against `cb7ddfd` the diff is comments, the commit
message, `TODO.md` prose, and this PR's body. `make check` re-run whole after
the edits.
## Verified against the source before writing a word
I checked every claim myself rather than transcribing the brief. All of it held;
I found nothing wrong in it. What I read, and what it says:
| Claim | Where | What the source says |
| --- | --- | --- |
| `' '` does not take a turn | `game/tables.go:804-805` | `' ': func(g *RogueGame) { g.After = false }`, carrying the comment `// "legal" illegal command`. Confirmed. |
| the eight movement keys clear `After` on a refused step | `game/move.go:74`, `:136`, `:31` | wall or off-map in `moveResolve`, illegal diagonal in `moveTarget`, confused step landing in place in `moveHero`. Confirmed — and `game/move.go:143` is a fourth, a run into the hero's own square. |
| `'.'` is an unconditional turn-taker | `game/tables.go:801-803` | handler body is empty. `dispatchKey` (`game/command.go:221-226`) finds it in `commandHandlers`, calls it and returns, so nothing downstream can clear `After`. Confirmed. |
| `'s'` is an unconditional turn-taker | `game/tables.go:770`, `game/command.go:521-551` | `'s': (*RogueGame).search`; neither `search`, nor `searchSpot`, nor `look` writes `After` on any path. Confirmed by reading every `g.After =` site in the package — they are in `armor.go`, `things.go`, `potions.go`, `move.go`, `options.go`, `tables.go` and `command.go`, none of them in the search chain. |
| `NoCommand > 0` leaves `After` true | `game/command.go:62-75`, `:117` | `playTurn` takes the `if g.NoCommand != 0` arm and skips `executeCommand` entirely; `After` was set true in `turnUpkeep` and nothing clears it. Confirmed. |
| a bear trap and `Held` do not clear it either | `game/move.go:20-25`, `:156-159` | both return without touching `After`. Confirmed. |
| `driveTerm` supplies no newline, and `waitFor('\n')` would wedge on it | `game/io.go:286-292`, `game/rip.go:69`, `game/score.go:106` | `waitFor('\n')` loops on `readchar` until `'\n'` or `'\r'`, which this script never yields. Both call sites are the death and score paths, which `fortify` (`game/run_test.go:14-22`) prevents reaching. Confirmed unreachable, and now documented. |
## F1: the safety argument, rewritten
The false sentence is gone from all four places it appeared — the test's script
comment, the `driveTerm` doc, the commit message and the PR body. `driveTerm`'s
doc now carries the argument, and it is the one that survives checking:
> Repeating the script is necessary but nowhere near sufficient, and the
> difference is what anyone editing one of these scripts has to know. Most keys
> take a turn only conditionally. `' '` is the "legal" illegal command and
> clears `After` outright (tables.go). All eight movement keys clear it whenever
> the step is refused: a wall or the map edge (move.go `moveResolve`), an
> illegal diagonal (`moveTarget`), or a confused step that lands back in place
> (`moveHero`). A script of nothing but those keys wedges exactly the way
> `testTerm`'s tail does, repetition or no repetition — with the script set to
> just `" "` this drive hits the 30s package timeout inside `command()`.
>
> What actually makes the wedge impossible is that the cycle always contains at
> least one _unconditional_ turn-taker, and the scripts here carry two: `'.'`,
> the rest command, whose handler is empty, and `'s'`, search, which writes
> `After` on no path. Nothing refuses either one — not being blocked in all
> eight directions, not `Held`, not stuck in a bear trap, and not
> `NoCommand > 0`, where `playTurn` skips `executeCommand` altogether and
> `After` is simply left true. Trim both out and the wedge this test exists to
> remove comes straight back.
The `" "` probe result is the reviewer's, credited as such in my report rather
than claimed as my own run; everything else in that paragraph I read out of the
files listed above.
The script line in `TestAutoSaveOnSignalRacesTurnLoop` now points at it instead
of asserting the false general rule:
> The `'.'` and the `'s'` are what make an unbounded drive safe, and at least
> one of the two has to stay in the cycle: see `driveTerm`.
One deliberate difference from the brief's phrasing: "removing both would
reintroduce the wedge" is what I wrote, not "removing either". Removing one of
the two is survivable — one unconditional turn-taker is enough — and the test's
other drive script (`"s . "`, in `TestAutoSaveOnSignalWithoutASaveFile`) has
both as well. The invariant is "at least one", and that is what both comments
state.
## F1, folded in: the missing newline
Same paragraph, as an explicit precondition of the fake rather than an accident:
> One further precondition, from what this fake does not supply: `testTerm`'s
> tail answered a newline every other read and this does not. Nothing reachable
> from these scripts asks for one — `waitFor('\n')` sits on the death and score
> paths (rip.go, score.go), which `fortify` prevents from ever being reached —
> but a script that could reach them would park in `waitFor` for ever.
## F2: the bound stated as it actually holds
`driveUntilDone`'s doc no longer claims one `autoSaveWait` covers every case:
> Dropping it costs no termination guarantee, because the bound belongs to the
> code under test and not to this loop: each `AutoSaveOnSignal` call returns
> within the timeout the caller hands it, so the saving goroutine always
> finishes and `done` always closes. That bound is worth stating exactly,
> because it is not one `autoSaveWait`.
>
> A handoff that has stopped answering altogether costs one, in total, however
> many saves were asked for. `g.sigSave` is one deep, so the unserviced request
> stays in the channel and every later call finds it full and reports failure
> immediately — measured at 10.0s for 25 saves with the service point deleted
> from `command()`. What fails is then the caller's own assertion, the count of
> saves actually taken, which says far more than "out of turns" ever did.
>
> A handoff that still drains every request but takes longer than
> `autoSaveWait` to do it is the worst case, and costs one timeout per save:
> `wantSaves * autoSaveWait`, 250s at these constants, which would run past the
> package timeout rather than reach the assertion. It takes about ten seconds of
> scheduler starvation per save to get there, against a regime measured at 0.12s
> per 1000 turns, so it is remote — and the 1000-turn cap did not bound it
> either, a turn count being no kind of time bound. `go test -timeout 30s` is
> the backstop under all of it.
I re-derived the 250s rather than taking it: the post is non-blocking and only
fails when `sigSave` is already full, so a handoff that eventually drains each
request leaves the channel empty for the next call, which then gets its own full
`autoSaveWait`. 25 x 10s. The 10.0s figure is the reviewer's measurement of
mutation B, credited; my own run of the same mutation was 10.16s.
The `autoSaveWait` const comment no longer implies a single wait either — it now
points at `driveUntilDone` "for what that bound comes to".
## Also corrected in the record
- **Commit message**: both claims rewritten to match the above, and the 0.12s
figure added — 1000 turns burned in a tenth of a second, which states the
diagnosis better than "too slow under load" and explains why loading the host
never reproduced it. The `DATA RACE` count is now given as a range across
machines (139 here, 62-110 in review, 73/96/113 in
https://git.eeqj.de/sneak/rgoue/pulls/26) with the property named as the thing
pinned, rather than as a single number.
- **`TODO.md`**: the Completed Steps entry repeated both false claims; both
paragraphs are corrected in the same commit. The entry is kept and "Next Step"
is untouched — still the release step.
- **This PR's body**: same two passages corrected, plus the reviewer's trace
nuance (their top trace shows the racing write from `Window.AddCh` / `look` /
`turnUpkeep`, not `executeCommand`; both appear, same class) and their
independent 4/4-vs-0/4 A/B. Strictly speaking outside "comments and the commit
message", but it carried the identical false sentence and it is my own text;
flagging it here so it is a visible choice rather than a silent one.
## Gate
`make fmt`, then `make check` re-run in full after the last edit — it
short-circuits, so nothing was carried over from the earlier run.
```
prettier --tab-width 4 --prose-wrap always --check ...
All matched files use Prettier code style!
golangci-lint run ./...
level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) ..."
0 issues.
ok git.eeqj.de/sneak/rgoue/cmd/rogue (cached) coverage: 29.7% of statements
ok git.eeqj.de/sneak/rgoue/game 2.491s coverage: 64.0% of statements
```
The `game` package — the only one this touches — ran for real at 2.491s rather
than reporting cached. Lint used a private empty `GOLANGCI_LINT_CACHE` under my
own throwaway worktree, reported no `parallel golangci-lint is running`, and
named no path outside it; the `gomodguard` deprecation
(https://git.eeqj.de/sneak/rgoue/issues/29) is the only warning and predates
this branch.
`.golangci.yml` still sha256
`021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` and absent
from the diff; nothing under `game/testdata/`; no Dockerfile, CI or `script/`;
`git diff --check` clean; still fast-forwardable onto `origin/main` @ `13caec4`;
still one commit whose title ends ` (closes #36)`, amended and force-pushed with
`--force-with-lease`. Work was done in a throwaway worktree and the shared clone
is untouched on `main`.
Verified at head 3061931 against #36.
Scope as directed: the corrected prose, re-checked against the source rather than
against the text, plus the gate and one mutation. The substance that passed at cb7ddfd was not re-litigated.
The delta is comment-only.git diff cb7ddfd 3061931 touches TODO.md prose
and game/autosave_test.go comments only. The single non-comment line inside the
changed hunks — term := &driveTerm{...} at game/autosave_test.go:43 — is
unchanged context. No test logic, no game code.
'.' — game/tables.go:801-803, empty handler; dispatchKey
(game/command.go:222-226) finds it in commandHandlers, calls it and returns (0, false), so nothing downstream can clear After. Unconditional.
's' — game/tables.go:770 → search (game/command.go:521-552) → searchSpot (:557-595), searchFloor (:599-), look (game/misc.go:22). misc.go has no After assignment at all, and command.go's are at
117/239/273/288/342/357/377/416/891/926 — none inside 521-620. Unconditional.
All four movement sites confirmed, and there is no fifth. game/move.go:31 (confused step landing in place, moveHero), :74 (wall or
map edge — offMap returns ' ' at :132 and falls into that arm of moveResolve), :136 (illegal diagonal, moveTarget), :143 (running into
the hero's own square, moveTarget). The eight keys dispatch straight to moveHero with no wrapper (game/tables.go:700-707), and the rest of that
call tree — moveEnter, springTrap (move.go:307), moveOnto, fight.go, pickUp (pack.go:307-338) — writes After nowhere. Every remaining site in
the package sits in a handler for a key absent from these scripts (wield, wear, takeOff, dropIt, doZap, option, quaffHaste, promptPackItem
at pack.go:403-); move.go:9 is startRun, reachable only from run keys,
also absent.
Refusal-proof — Held (move.go:156-159) and bear trap (move.go:20-25)
both return without touching After; NoCommand > 0 takes the playTurn
else-arm (command.go:62-75) and skips executeCommand, leaving After true
from turnUpkeep (command.go:117). Confirmed.
Both scripts carry both keys: "h j k l y u b n s . " (:43) and "s . "
(:306).
"both" vs "either" — the reworker is right and the brief was wrong
One unconditional turn-taker suffices. Drop '.' from "h j k l y u b n s . " and 's' still terminates the drive; drop 's' and '.' still does. So "removing both would reintroduce the wedge" is true, and
"removing either" would have been false — it would have told a reader that 's'
alone cannot carry the drive, which is exactly the wrong conclusion. The
invariant is "at least one", and that is what game/autosave_test.go:41-42 and :498-505 state.
F2 — both bounds follow from the code
AutoSaveOnSignal (game/save.go:745-768) posts non-blocking (:748-755) then
selects on req.done against a timer (:762-767); driveTerm.Interrupt is a
no-op (:525), so the call is bounded unconditionally.
Permanently dead handoff: the unserviced request sits in the one-deep sigSave, so calls 2-25 fail at once. One autoSaveWait in total — measured
below at 10.11s.
Live-but-slow: each request is eventually drained, so the next post succeeds
and buys its own full wait. wantSaves * autoSaveWait = 25 x 10s = 250s
(:50, :20) against go test -timeout 30s (Makefile:37).
The autoSaveWait const comment (:17-21) now defers to driveUntilDone "for
what that bound comes to" and no longer implies a single wait.
The newline precondition
waitFor('\n') (game/io.go:286-294) loops on readchar until '\n'/'\r',
which driveTerm never yields. Its only two call sites are game/rip.go:69
(death) and game/score.go:106 (score). Unreachability verified, not
accepted: fortify (game/run_test.go:14-22) pins HP/MaxHP/Exp/FoodLeft at
30000 and zeroes NoCommand/NoMove before every command(), closing every death caller (fight.go, move.go:393/:425, sticks.go:508-510, daemons.go:140 starvation, rip.go:276); score's other callers need quit
(game.go:343), totalWinner (command.go:783) or the -s option
(score.go:259), none of which this script's keys can reach.
Disclosed as a judgement call rather than passed silently: the comment credits
that unreachability to fortify alone. fortify closes the death path; the
quit/win/-s routes into score are closed by the script's key set instead. The
operative clause — "nothing reachable from these scripts asks for one" — is true
as written, so I am not filing it, but the attribution is narrower than the
conclusion it carries.
The false sentence
Gone from the working tree, the commit message and the PR body. The only
surviving "takes a turn" strings are TODO.md:76 ("neither key takes a turn",
about testTerm's space/newline tail — true: '\n' falls to the dispatchKey
default at command.go:238-239, which sets After = false and calls illcom)
and game/autosave_test.go:489 ("Most keys take a turn only conditionally"),
which is the correction itself.
Also checked
DATA RACE is given as a cross-machine range with the property named as what is
pinned. 0.12s appears at TODO.md:70, game/autosave_test.go:113 and in the
commit message, always as "per 1000 turns", not overstated. TODO.md keeps its
Completed Steps entry at :38, and "Next Step" (:30-34, the release step) is
absent from the diff.
Mutation re-run — serviceAutoSaveRequest deleted from command()
(game/command.go:16; the readchar service point at game/io.go:189 fires
only on !ok, which driveTerm.ReadChar never returns): --- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.11s), autosave_test.go:70: saves taken = 0, want 25, package 10.293s, zero DATA RACE. The comment edits disturbed nothing. Reverted afterwards; the work
was done in a throwaway worktree and the shared clone is untouched on main.
Gate — make check green: fmt-check clean, lint 0 issues (private
empty GOLANGCI_LINT_CACHE, no parallel golangci-lint is running, no path
outside my worktree; the gomodguard deprecation, #29, is the only warning), tests real at
2.797s rather than (cached). Two further GOFLAGS=-count=1 make test runs,
both green and race-clean (2.454s, 2.367s). .golangci.yml sha256 021cc83f...46bcb and absent from the diff; nothing under game/testdata/; no
Dockerfile, CI or script/; no Claude or Anthropic reference and no attribution
trailer anywhere; single commit whose title ends (closes #36); git diff --check clean; fast-forwardable onto main @ 13caec4. This repo has
no CI by design, so the head carries no statuses, as main does not.
One inaccuracy in the record rather than in the change, stated for completeness:
the rework comment lists the package's g.After = sites as being "in armor.go, things.go, potions.go, move.go, options.go, tables.go and command.go". They are also in pack.go:423/:433, sticks.go:22 and weapons.go:123/:130/:136. The conclusion that enumeration supports — none
in the search chain — is true, and none of the omitted sites are reachable from
these scripts either, so nothing shipped is wrong.
## Independent confirmation review — PASS
Verified at head `3061931` against https://git.eeqj.de/sneak/rgoue/issues/36.
Scope as directed: the corrected prose, re-checked against the source rather than
against the text, plus the gate and one mutation. The substance that passed at
`cb7ddfd` was not re-litigated.
**The delta is comment-only.** `git diff cb7ddfd 3061931` touches `TODO.md` prose
and `game/autosave_test.go` comments only. The single non-comment line inside the
changed hunks — `term := &driveTerm{...}` at `game/autosave_test.go:43` — is
unchanged context. No test logic, no game code.
## F1, clause by clause
- `' '` — `game/tables.go:804-805`, `g.After = false`, carrying the
`// "legal" illegal command` comment. Confirmed.
- `'.'` — `game/tables.go:801-803`, empty handler; `dispatchKey`
(`game/command.go:222-226`) finds it in `commandHandlers`, calls it and returns
`(0, false)`, so nothing downstream can clear `After`. Unconditional.
- `'s'` — `game/tables.go:770` → `search` (`game/command.go:521-552`) →
`searchSpot` (`:557-595`), `searchFloor` (`:599-`), `look` (`game/misc.go:22`).
`misc.go` has no `After` assignment at all, and `command.go`'s are at
117/239/273/288/342/357/377/416/891/926 — none inside 521-620. Unconditional.
- **All four movement sites confirmed, and there is no fifth.**
`game/move.go:31` (confused step landing in place, `moveHero`), `:74` (wall or
map edge — `offMap` returns `' '` at `:132` and falls into that arm of
`moveResolve`), `:136` (illegal diagonal, `moveTarget`), `:143` (running into
the hero's own square, `moveTarget`). The eight keys dispatch straight to
`moveHero` with no wrapper (`game/tables.go:700-707`), and the rest of that
call tree — `moveEnter`, `springTrap` (`move.go:307`), `moveOnto`, `fight.go`,
`pickUp` (`pack.go:307-338`) — writes `After` nowhere. Every remaining site in
the package sits in a handler for a key absent from these scripts (`wield`,
`wear`, `takeOff`, `dropIt`, `doZap`, `option`, `quaffHaste`, `promptPackItem`
at `pack.go:403-`); `move.go:9` is `startRun`, reachable only from run keys,
also absent.
- Refusal-proof — `Held` (`move.go:156-159`) and bear trap (`move.go:20-25`)
both return without touching `After`; `NoCommand > 0` takes the `playTurn`
else-arm (`command.go:62-75`) and skips `executeCommand`, leaving `After` true
from `turnUpkeep` (`command.go:117`). Confirmed.
Both scripts carry both keys: `"h j k l y u b n s . "` (`:43`) and `"s . "`
(`:306`).
## "both" vs "either" — the reworker is right and the brief was wrong
One unconditional turn-taker suffices. Drop `'.'` from
`"h j k l y u b n s . "` and `'s'` still terminates the drive; drop `'s'` and
`'.'` still does. So "removing **both** would reintroduce the wedge" is true, and
"removing either" would have been false — it would have told a reader that `'s'`
alone cannot carry the drive, which is exactly the wrong conclusion. The
invariant is "at least one", and that is what `game/autosave_test.go:41-42` and
`:498-505` state.
## F2 — both bounds follow from the code
`AutoSaveOnSignal` (`game/save.go:745-768`) posts non-blocking (`:748-755`) then
selects on `req.done` against a timer (`:762-767`); `driveTerm.Interrupt` is a
no-op (`:525`), so the call is bounded unconditionally.
- Permanently dead handoff: the unserviced request sits in the one-deep
`sigSave`, so calls 2-25 fail at once. One `autoSaveWait` in total — measured
below at 10.11s.
- Live-but-slow: each request is eventually drained, so the next post succeeds
and buys its own full wait. `wantSaves * autoSaveWait` = 25 x 10s = 250s
(`:50`, `:20`) against `go test -timeout 30s` (`Makefile:37`).
The `autoSaveWait` const comment (`:17-21`) now defers to `driveUntilDone` "for
what that bound comes to" and no longer implies a single wait.
## The newline precondition
`waitFor('\n')` (`game/io.go:286-294`) loops on `readchar` until `'\n'`/`'\r'`,
which `driveTerm` never yields. Its only two call sites are `game/rip.go:69`
(`death`) and `game/score.go:106` (`score`). Unreachability verified, not
accepted: `fortify` (`game/run_test.go:14-22`) pins HP/MaxHP/Exp/FoodLeft at
30000 and zeroes `NoCommand`/`NoMove` before every `command()`, closing every
`death` caller (`fight.go`, `move.go:393`/`:425`, `sticks.go:508-510`,
`daemons.go:140` starvation, `rip.go:276`); `score`'s other callers need `quit`
(`game.go:343`), `totalWinner` (`command.go:783`) or the `-s` option
(`score.go:259`), none of which this script's keys can reach.
Disclosed as a judgement call rather than passed silently: the comment credits
that unreachability to `fortify` alone. `fortify` closes the death path; the
quit/win/`-s` routes into `score` are closed by the script's key set instead. The
operative clause — "nothing reachable from these scripts asks for one" — is true
as written, so I am not filing it, but the attribution is narrower than the
conclusion it carries.
## The false sentence
Gone from the working tree, the commit message and the PR body. The only
surviving "takes a turn" strings are `TODO.md:76` ("neither key takes a turn",
about `testTerm`'s space/newline tail — true: `'\n'` falls to the `dispatchKey`
default at `command.go:238-239`, which sets `After = false` and calls `illcom`)
and `game/autosave_test.go:489` ("Most keys take a turn only conditionally"),
which is the correction itself.
## Also checked
`DATA RACE` is given as a cross-machine range with the property named as what is
pinned. 0.12s appears at `TODO.md:70`, `game/autosave_test.go:113` and in the
commit message, always as "per 1000 turns", not overstated. `TODO.md` keeps its
Completed Steps entry at `:38`, and "Next Step" (`:30-34`, the release step) is
absent from the diff.
**Mutation re-run** — `serviceAutoSaveRequest` deleted from `command()`
(`game/command.go:16`; the `readchar` service point at `game/io.go:189` fires
only on `!ok`, which `driveTerm.ReadChar` never returns):
`--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.11s)`,
`autosave_test.go:70: saves taken = 0, want 25`, package 10.293s, zero
`DATA RACE`. The comment edits disturbed nothing. Reverted afterwards; the work
was done in a throwaway worktree and the shared clone is untouched on `main`.
**Gate** — `make check` green: `fmt-check` clean, lint **0 issues** (private
empty `GOLANGCI_LINT_CACHE`, no `parallel golangci-lint is running`, no path
outside my worktree; the `gomodguard` deprecation,
https://git.eeqj.de/sneak/rgoue/issues/29, is the only warning), tests real at
2.797s rather than `(cached)`. Two further `GOFLAGS=-count=1 make test` runs,
both green and race-clean (2.454s, 2.367s). `.golangci.yml` sha256
`021cc83f...46bcb` and absent from the diff; nothing under `game/testdata/`; no
Dockerfile, CI or `script/`; no Claude or Anthropic reference and no attribution
trailer anywhere; single commit whose title ends ` (closes #36)`;
`git diff --check` clean; fast-forwardable onto `main` @ `13caec4`. This repo has
no CI by design, so the head carries no statuses, as `main` does not.
One inaccuracy in the record rather than in the change, stated for completeness:
the rework comment lists the package's `g.After =` sites as being "in `armor.go`,
`things.go`, `potions.go`, `move.go`, `options.go`, `tables.go` and
`command.go`". They are also in `pack.go:423`/`:433`, `sticks.go:22` and
`weapons.go:123`/`:130`/`:136`. The conclusion that enumeration supports — none
in the search chain — is true, and none of the omitted sites are reachable from
these scripts either, so nothing shipped is wrong.
Manager notes (the review is in its own comment above).
Verdict accepted: PASS. Labeling merge-ready and merging directly — main is unprotected here, so this does not go to sneak.
The adjudication went against me, correctly. I told the reworker to warn
against removing "either" unconditional turn-taker; they used "both" and were
right. One suffices, so the invariant is "at least one" — and my phrasing
would have told a reader that 's' alone cannot carry the drive. That is the
wrong conclusion in the exact direction this rework existed to prevent: I
sent the PR back for a comment that misled about what keeps the loop alive,
and my correction to it would have misled about the same thing the other way.
Good instinct by the reworker to deviate and flag it rather than comply.
Blocking this on a comment was the right call, and the review shows why.
The corrected invariant is not a tidier way of saying the old one — it is a
different, narrower claim, and the difference is load-bearing. ' ' clears After; all four movement-refusal sites clear it (move.go:31, :74, :136, and :143, which the reworker found and my brief missed); the
reviewer then confirmed there is no fifth by checking that the eight keys call moveHero with no wrapper and that moveEnter, springTrap, moveOnto, fight.go and pickUp write After nowhere. What actually holds the drive up
is '.' and 's', and now the comment says so.
Both F2 bounds check out — one autoSaveWait (measured 10.11s) for a dead
handoff, 25 × 10s = 250s for a live-but-slow one against a 30s package
timeout — and the autoSaveWait const comment no longer implies a single wait.
Two disclosures I want on the record, neither shipped wrong:
The newline-precondition comment credits fortify alone for closing waitFor('\n'). fortify closes the death path; the quit / totalWinner / -s routes are closed by the script's key set instead. The operative clause
("nothing reachable from these scripts asks for one") is true as written, so
the attribution is narrower than the conclusion — worth knowing, not worth a
round.
The rework comment's list of files containing g.After = omits pack.go, sticks.go, weapons.go. The conclusion it supports (none in the search
chain) is true and none are script-reachable. A record inaccuracy in a PR
comment, not in the tree.
Reporting both rather than only the verdict is what makes the verdict worth
something.
What this PR actually fixed, now settled: a guard test for a
save-destroying data race was failing intermittently, and every earlier attempt
to reproduce it by loading the host failed — because the trigger was never
wall-clock at all. Each old failure burned 1000 turns in 0.12 seconds; the
cap was a scheduler-starvation detector wearing a turn-budget costume. Five
sessions watched it fire and filtered away the evidence; changing the
instruction from "capture it" to "write the full output to a file and read it"
cracked it in one pass.
With this merged, the repo's Next Step — tag a release once a full game
completes without defects (#8) — is the only thing left in the queue, and it
needs a human at an 80x24 terminal.
Manager notes (the review is in its own comment above).
**Verdict accepted: PASS. Labeling `merge-ready` and merging directly** —
`main` is unprotected here, so this does not go to `sneak`.
**The adjudication went against me, correctly.** I told the reworker to warn
against removing "either" unconditional turn-taker; they used "both" and were
right. One suffices, so the invariant is "at least one" — and my phrasing
would have told a reader that `'s'` alone cannot carry the drive. That is the
wrong conclusion **in the exact direction this rework existed to prevent**: I
sent the PR back for a comment that misled about what keeps the loop alive,
and my correction to it would have misled about the same thing the other way.
Good instinct by the reworker to deviate and flag it rather than comply.
**Blocking this on a comment was the right call, and the review shows why.**
The corrected invariant is not a tidier way of saying the old one — it is a
different, narrower claim, and the difference is load-bearing. `' '` clears
`After`; all **four** movement-refusal sites clear it (`move.go:31`, `:74`,
`:136`, and `:143`, which the reworker found and my brief missed); the
reviewer then confirmed there is no fifth by checking that the eight keys call
`moveHero` with no wrapper and that `moveEnter`, `springTrap`, `moveOnto`,
`fight.go` and `pickUp` write `After` nowhere. What actually holds the drive up
is `'.'` and `'s'`, and now the comment says so.
**Both F2 bounds check out** — one `autoSaveWait` (measured 10.11s) for a dead
handoff, `25 × 10s = 250s` for a live-but-slow one against a 30s package
timeout — and the `autoSaveWait` const comment no longer implies a single wait.
**Two disclosures I want on the record, neither shipped wrong:**
- The newline-precondition comment credits `fortify` alone for closing
`waitFor('\n')`. `fortify` closes the death path; the quit / `totalWinner` /
`-s` routes are closed by the script's key set instead. The operative clause
("nothing reachable from these scripts asks for one") is true as written, so
the attribution is narrower than the conclusion — worth knowing, not worth a
round.
- The rework comment's list of files containing `g.After =` omits `pack.go`,
`sticks.go`, `weapons.go`. The conclusion it supports (none in the search
chain) is true and none are script-reachable. A record inaccuracy in a PR
comment, not in the tree.
Reporting both rather than only the verdict is what makes the verdict worth
something.
**What this PR actually fixed**, now settled: a guard test for a
save-destroying data race was failing intermittently, and every earlier attempt
to reproduce it by loading the host failed — because the trigger was never
wall-clock at all. Each old failure burned 1000 turns in **0.12 seconds**; the
cap was a scheduler-starvation detector wearing a turn-budget costume. Five
sessions watched it fire and filtered away the evidence; changing the
instruction from "capture it" to "write the full output to a file and read it"
cracked it in one pass.
With this merged, the repo's `Next Step` — tag a release once a full game
completes without defects (#8) — is the only thing left in the queue, and it
needs a human at an 80x24 terminal.
clawbot
merged commit bde4eae450 into main2026-08-09 19:18:56 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #36.
TestAutoSaveOnSignalRacesTurnLoopfailed intermittently under load. Thediagnosis captured on the issue is confirmed: it is not a data race, it is
the test's own drive loop running out of a fixed 1000-turn budget. The budget is
removed rather than widened.
No non-test code changed.
game/autosave_test.goandTODO.mdonly.The finding, verified
The assertion is
driveUntilDone'st.Fatal("the turn loop ran out of turns before the saves were taken"), and noWARNING: DATA RACEappears anywhere.I did not take that on trust. Instrumenting the loop to report the turns it
actually consumed, and running the suite under varying scheduling pressure,
showed the count tracking contention and nothing else:
GOMAXPROCS=4GOMAXPROCS=2GOMAXPROCS=1The
GOMAXPROCS=2run reproduced the reported assertion exactly, with zeroDATA RACEreports in the log. That is the diagnosis, independently reproduced.The reason is mechanical: the turns this loop spends between one save being
answered and the next request arriving are not work, they are the saving
goroutine's wake-up latency. So "1000 turns" is a wall-clock assumption in
disguise, and raising it would have hidden the flake rather than removed it.
Review measured each old-code failure at 0.12 seconds — 1000 turns burned in
a tenth of a second — which is the sharpest statement of that: the cap was never
a wall-clock allowance at all, and that is why every attempt to reproduce it by
loading the host failed.
The fix
driveUntilDonenow drives until the saving goroutine finishes, and nothingelse — no turn count, no timeout of its own.
Termination is not lost; it moves to where it belongs, the contract of the code
under test. Every
AutoSaveOnSignalreturns within the timeout it is handed, sothe saving goroutine always finishes and
donealways closes. A handoff thathas stopped answering costs one
autoSaveWaitin total, however many saves wereasked for:
g.sigSaveis one deep, so an unserviced request stays in thechannel and every later call finds it full and fails immediately. What fails is
then the real assertion —
saves taken = 0, want 25— which says far more than"out of turns" ever did.
That is not the worst case, and the comment in the test now says so. A handoff
that still drains every request but takes longer than
autoSaveWaitto do itcosts one timeout per save:
wantSaves * autoSaveWait= 250s, which would runpast the 30s package timeout instead of reaching the assertion. It needs about
ten seconds of scheduler starvation per save against the 0.12s-per-1000-turns
regime above, so it is remote, and the 1000-turn cap did not bound it either — a
turn count is not a time bound.
go test -timeout 30sis the backstop under allof it.
A second assumption underneath the first
Removing the cap exposed one, and this is why the diff is not one line.
testTermanswers space and newline for ever once its script is exhausted.Neither key takes a turn, and
command()loops until the player consumes one(
if !g.After { ntimes++ }), so past the end of the scriptcommand()neverreturns. The old cap was silently sized to the script — 4000 characters, two
consumed per turn, against 1000 turns. My first uncapped attempt duly wedged
inside a single
command()call and hit the 30s package timeout; the goroutinedump showed the drive still inside
command()with the saving goroutine alreadygone.
The two drive tests therefore take a new
driveTerm, a headless terminal whosescript repeats. Repeating is necessary and not sufficient, and the comment
on
driveTermnow names the property that is actually load-bearing. Most keystake a turn only conditionally:
' 'is the "legal" illegal command and clearsAfteroutright (game/tables.go:804-805), and all eight movement keys clearit on a refused step — wall or map edge (
game/move.go:74), illegal diagonal(
game/move.go:136), confused step landing in place (game/move.go:31). Ascript of only those keys wedges exactly the way
testTerm's tail does.What makes the wedge impossible is that the cycle always contains at least one
unconditional turn-taker, and these scripts contain two:
'.', whosehandler is empty (
game/tables.go:801-803), and's'/search, which writesAfteron no path (game/tables.go:770,game/command.go:521-551). Neithercan be refused — not blocked in all eight directions, not
Held, not a beartrap, and not
NoCommand > 0, whereplayTurnskipsexecuteCommandaltogether and
Afteris simply left true. Trim both out and the wedge comesstraight back.
Proof the test is still a real guard
Both mutations were run through
make testin this worktree and revertedafterwards.
.golangci.ymlis byte-identical (sha256021cc83f...46bcb) andnothing under
game/testdata/was touched.AutoSaveOnSignalbody replaced by a directg.autoSave()— the pre-fix behavior from #24--- FAIL: TestAutoSaveOnSignalRacesTurnLoop, 139WARNING: DATA RACEreports here, 62-110 on the reviewer's machineserviceAutoSaveRequestremoved fromcommand()--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.16s): saves taken = 0, want 25The race trace under the first mutation, trimmed, naming the new drive loop:
The count is machine-dependent — 139 here, 73/96/113 in
#26, 62-110 in review — and the property is
what is pinned, not the number. Review's top trace also showed the racing write
coming from
Window.AddCh/look/turnUpkeeprather thanexecuteCommand;both appear, same class. The second mutation is strictly better than before: it
used to fail by exhausting turns, it now fails in 10s with the count of saves
actually taken, and the package finishes in 10.4s rather than hitting the 30s
timeout.
Under load
The reliable trigger turned out not to be raw host load — with the old code, 24
concurrent unconstrained runs at load ~120 were all green, which matches the
"could not reproduce" observation on the issue. The trigger is Go-scheduler
starvation of the saving goroutine plus the doubled load of the verbose rerun.
So the A/B was run under exactly that:
GOMAXPROCS=2, 48-core host at load ~150(40 spinners), with an unrelated deliberate failure planted in the tree so that
every run took the verbose rerun.
TestAutoSaveOnSignalRacesTurnLoopfailuresmain@13caec4(1000-turn cap)Review reproduced the A/B independently at reduced scale:
main4 of 4, thisbranch 0 of 4.
Further runs on this branch, all green, all
GOFLAGS=-count=1:make testat host load ~120 on 48 cores;GOMAXPROCS1, 2 and 4;GOMAXPROCS=2under the forced verbose rerun at load ~155.Verification
make fmt, thenmake checkgreen:fmt-check+lint(0 issues) +test. Every lint run used a private emptyGOLANGCI_LINT_CACHEinside thisworktree, reported no
parallel golangci-lint is running, and named no pathoutside the worktree. The
gomodguarddeprecation warning(#29) is the only warning and is not from
this change.
TODO.mdgains a Completed Steps entry in the same commit.Next Stepis notrotated — it is the release step, which needs a human.
TestAutoSaveOnSignalRacesTurnLoop failed intermittently under load. The captured failure text settles what it was: driveUntilDone's t.Fatal("the turn loop ran out of turns before the saves were taken") with no WARNING: DATA RACE anywhere in the log. The handoff is fine; the test's own drive loop ran out of its fixed 1000-turn budget first. Confirmed by instrumenting the loop to report the turns it actually used. The count tracks scheduling pressure and nothing else: about 60-120 turns at host load ~57 with the whole machine to spread over, 418 at GOMAXPROCS=4, 539 and 655 at 2 and 1, and past 1000 under the doubled load of the verbose rerun the test target performs after a failure. The turns spent between one save being answered and the next request arriving are not work, they are the saving goroutine's wake-up latency, so a fixed turn count is a wall-clock assumption in disguise. Raising it would hide the flake, not fix it. So the budget is gone rather than larger. driveUntilDone drives until the saving goroutine finishes and nothing else. Termination still holds, it just belongs to the code under test: every AutoSaveOnSignal returns within the timeout it is handed, and g.sigSave is one deep, so an unserviced request stays in the channel and every later call finds it full and fails at once. A dead handoff therefore releases the saving goroutine after one autoSaveWait however many saves were asked for, and what fails is the real assertion - "saves taken = 0, want 25" - rather than "out of turns". Removing the cap exposed a second assumption underneath it. testTerm answers space and newline for ever once its script is exhausted, and neither key takes a turn, so command(), which loops until the player consumes one, never returns; the old cap was silently sized to the script. An uncapped drive wedged inside a single command() call. The two drive tests now use driveTerm, a headless terminal whose script repeats, so every key it hands out takes a turn. The guard is undiminished, shown by mutation and reverted afterwards. Reverting the fix from the earlier signal-autosave work - AutoSaveOnSignal replaced by a direct g.autoSave(), encoding on the calling goroutine - still fails the test with 139 DATA RACE reports, snapshotHeader reading what executeCommand writes. Removing serviceAutoSaveRequest from command() still fails it too, now in 10s with "saves taken = 0, want 25" instead of by hanging. Under load: at GOMAXPROCS=2 on a 48-core host at load ~150, with an unrelated deliberate failure in the tree so every run took the verbose rerun, the old code failed 8 of 8 runs and the new code 0 of 8. Also green across 24 concurrent unconstrained runs at load ~120, 10 runs alongside a spinner load, and 5 runs each at GOMAXPROCS 1, 2 and 4. No non-test code changed. make check green, lint 0 issues, .golangci.yml byte-identical.Diagnosis confirmation
The captured finding on #36 holds, and I
reproduced it rather than assuming it.
Instrumenting
driveUntilDoneto report the turns it actually consumed, thenvarying scheduling pressure, gave:
GOMAXPROCS=4GOMAXPROCS=2GOMAXPROCS=1The
GOMAXPROCS=2run that hit the cap produced exactlyautosave_test.go:63: the turn loop ran out of turns before the saves were takenwith zero
WARNING: DATA RACEin the log. Not a race, and no residual racefound behind it — so nothing to report against
#24 and no non-test code was touched.
One correction to the brief I was given, which is worth recording: it framed the
autoSaveWait = 10s/ fixed-turn-count split as the asymmetry to fix, implying awall-clock bound on the drive was the answer. A wall-clock bound is the wrong
shape here.
AutoSaveOnSignalalready gives up afterautoSaveWait, so a drivewatchdog set to the same value is a photo finish with it: I built that version
first, and which of the two fired — the watchdog's message or the real
saves takenassertion — came out a coin flip between runs. That is a new flaketraded for the old one. The drive needs no time bound of its own at all; see
below.
The fix
driveUntilDonedrives until the saving goroutine finishes, and nothing else —no turn count, no timeout.
Termination moves to where it belongs, the contract of the code under test. Every
AutoSaveOnSignalreturns within the timeout it is handed, andg.sigSaveis onedeep, so an unserviced request stays in the channel and every later call finds it
full and fails at once. A dead handoff releases the saving goroutine after one
autoSaveWaithowever many saves were asked for,donecloses, and what fails isthe real assertion —
saves taken = 0, want 25.go test -timeout 30sis thebackstop under that.
The second assumption underneath the cap
Worth flagging for review because it is not obvious.
testTermanswers space andnewline for ever once its script is exhausted; neither takes a turn, and
command()loops until the player consumes one (if !g.After { ntimes++ }), sopast the end of the script
command()never returns. The 1000-turn cap wassilently sized to the 4000-character script. My first uncapped attempt wedged
inside a single
command()call and hit the 30s package timeout — the goroutinedump showed the drive still inside
command()with the saving goroutine alreadygone. The two drive tests now use
driveTerm, whose script repeats, so every keytakes a turn.
Proof the test still catches the reverted fix
Both mutations run through
make testand reverted afterwards;.golangci.ymlbyte-identical (sha256
021cc83f...46bcb), nothing undergame/testdata/touched.
AutoSaveOnSignalbody replaced by a directg.autoSave()— encode on the calling goroutine, the pre-fix behavior--- FAIL: TestAutoSaveOnSignalRacesTurnLoop, 139WARNING: DATA RACEreports,snapshotHeaderreading whatexecuteCommandwritesserviceAutoSaveRequestremoved fromcommand()--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.16s): saves taken = 0, want 25139 sits in the 73-113-96 band #26 measured;
the property is pinned, not the number. The second mutation is strictly better
reported than before: it used to fail by exhausting turns, it now names the count
of saves actually taken, and the package finishes in 10.4s instead of timing out.
Under load
Raw host load alone does not do it — with the old code, 24 concurrent
unconstrained runs at load ~120 were all green, matching the
"could not reproduce" observation on the issue. The trigger is Go-scheduler
starvation of the saving goroutine, plus the doubled load of the verbose rerun.
A/B under exactly that:
GOMAXPROCS=2, 48-core host at load ~150, with anunrelated deliberate failure planted so every run took the verbose rerun.
TestAutoSaveOnSignalRacesTurnLoopfailuresmain@13caec4(1000-turn cap)Further runs on this branch, all green, all
GOFLAGS=-count=1: 24 concurrentmake testat load ~120; 10 sequential alongside a spinner load; 5 each atGOMAXPROCS1, 2 and 4.Gate
make fmt, thenmake checkgreen —fmt-check+lint(0 issues) +test. Lint ran against a private emptyGOLANGCI_LINT_CACHEinside thisworktree, reported no
parallel golangci-lint is running, and named no pathoutside the worktree. The
gomodguarddeprecation warning(#29) is the only warning and is not mine.
Independent review — PASS
Verified at head
cb7ddfdagainst#36. No non-test code changed:
game/autosave_test.goandTODO.mdonly.Reproduced
AutoSaveOnSignalbody replaced by a directg.autoSave()):--- FAIL: TestAutoSaveOnSignalRacesTurnLoop, 62WARNING: DATA RACEinthe first run (110 across run plus verbose rerun), traces
snapshotHeader/snapshot/saveFile/autoSave/AutoSaveOnSignalat
autosave_test.go:57against the drive atautosave_test.go:116. My countis not 139; #26 measured 73/96/113 and
the number is machine-dependent — the property is what is pinned. One nuance
on the PR body's wording: in my top trace the racing write comes from
Window.AddCh/look/turnUpkeep, notexecuteCommand; both appear,same class.
serviceAutoSaveRequestremoved fromcommand()):--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.02s): saves taken = 0, want 25,plus
the saved file does not restore, package finishing in 10.057s. Exactlyas reported — a diagnosable assertion, not a timeout.
GOMAXPROCS=2, 20 spinners, host load ~35 on48 cores, an unrelated planted failing test forcing the verbose rerun on every
run, 4 runs per tree, alternating.
main@13caec4: 4 of 4 failed with"ran out of turns". This branch: 0 of 4 (only the planted failure). Each
old failure took 0.12s — 1000 turns burned in a tenth of a second, which
states the diagnosis more sharply than "too slow under load" does: the budget
was never a wall-clock allowance at all.
Findings
Both non-blocking, and both about stated reasoning rather than behavior.
1.
game/autosave_test.go:38-39,:464-471, and the commit message: "everykey it hands out takes a turn" is false. The guarantee rests on two keys, not on
the script repeating.
Half the shipped script is spaces, and
' 'is explicitlyg.After = false(
game/tables.go:804-805, the "legal illegal command"). All eight movement keysclear
Afteras well when the step is refused: wall or off-map(
game/move.go:74), illegal diagonal (game/move.go:136), a confused steplanding in place (
game/move.go:31). What actually makes the wedge impossibleis that the cycle contains two unconditional turn-takers —
'.'(emptyhandler,
game/tables.go:801-803) and's'/search(
game/command.go:521-551, noAfterwrite on any path).I checked those two against the states you asked about: blocked in all eight
directions;
Held(game/move.go:152-156refuses the move without clearingAfter); bear trap (game/move.go:20-25);NoCommand > 0, whereplayTurnskips
executeCommandentirely (game/command.go:62-75) soAfterstays true,and
fortifyzeroesNoCommand/NoMoveevery iteration anyway; maze andpassage squares; and a pending prompt, where
--More--drains throughwaitForSpace, which the spaces answer.readCommandalso cannot stop readingfrom the terminal, because
Running,ToDeathandCountare only ever set bykeys absent from this script (
game/move.go:8,game/command.go:359,game/command.go:180).Probe: I set the script to
" "— still repeating — and it wedged exactly theway
testTerm's tail does,panic: test timed out after 30swith the driveinside
command()/readCommand. So "the script repeats" is not the propertythat saves it; "the cycle contains an unconditional turn-taker" is. The
conclusion in this PR is correct, but the reason written into the test is not,
and this comment is now where a future reader will go to learn why the drive
terminates — which is precisely the class of latent assumption the change exists
to remove.
Related, same paragraph:
driveTerm's script contains no newline, wheretestTerm's tail supplied'\n'on every other read. Nothing reachable fromthis script needs one —
waitFor('\n')sits on the death and score paths(
game/rip.go:69,game/score.go:106) thatfortifyprevents, andgetStr'sendsInputis reached only from commands not in the mix — so it is not a defecttoday. It is an undocumented precondition of the new fake.
2.
game/autosave_test.go:91-101and the commit message: "a dead handoffreleases the saving goroutine after one
autoSaveWaithowever many saves wereasked for" holds only for a permanently dead handoff.
Enumerating the termination paths:
AutoSaveOnSignalitself always returns —the post is non-blocking (
game/save.go:748-755),Interruptis a no-op ondriveTerm(game/autosave_test.go:485), and the wait is aselectonreq.doneagainst a timer (game/save.go:759-767). A fully dead handoff costsone
autoSaveWaitthen 24 instant failures, confirmed empirically at 10.02s bymutation B above. But a live-but-slow handoff — one that does drain each
request, only after more than
autoSaveWait— lets each call time out and thenext post succeed, for a worst case of
wantSaves * autoSaveWait= 250s, i.e.driveUntilDonewaiting out the 30s package timeout. That needs roughly 10s ofscheduler starvation per save against the 0.12s-per-1000-turns regime measured
above, so it is remote, and it is not a regression: the 1000-turn cap was not a
time bound either. It is the one gap in the "no bound of its own" argument, and
the comment states the bound more strongly than it holds.
The rejection of the wall-clock watchdog is sound on its own terms; a drive
deadline equal to
autoSaveWaitreally is a photo finish withAutoSaveOnSignal's own timer, and I would not have accepted that versioneither.
Checked and clean
Diff is
game/autosave_test.goplusTODO.mdonly;t.Parallel()and the//nolint:testpackageheader intact with no new nolints;TODO.mdgains aCompleted Steps entry and "Next Step" is still the release step;
.golangci.ymlsha256
021cc83f...46bcband absent from the diff; nothing undergame/testdata/; no Dockerfile, CI orscript/; no Claude or Anthropicreference and no attribution trailer anywhere in the diff or commit; commit
title ends
(closes #36);git diff --checkclean; fast-forwardable ontoorigin/main;make checkgreen —fmt-check, lint 0 issues (privateempty
GOLANGCI_LINT_CACHE, noparallel golangci-lint is running, no pathoutside my worktree,
gomodguarddeprecation only), tests pass — plus twofurther clean
GOFLAGS=-count=1 make testruns and the 8 full-suite runs fromthe A/B, all race-clean. This repo has no CI by design, so the head commit
carries no statuses, as
maindoes not.driveTermis used only by the twodrive tests and holds no shared state, so it cannot starve anything else in the
package.
Both mutations, the space-script probe and the planted A/B failure were run in a
throwaway worktree and reverted; the shared clone is untouched.
Manager notes (the review is in its own comment above).
I am overruling the reviewer's "land it as-is" and sending this back for a
comment-only rework. The finding is right; I disagree with its weight, and I
want to explain why rather than just applying a label.
The defect: a correct conclusion resting on a false stated reason.
game/autosave_test.go:38-39,:464-471and the commit message say therepeating script works because "every key it hands out takes a turn". That is
false, and the reviewer proved it: half the script is spaces,
' 'isexplicitly
g.After = false(tables.go:804-805), and all eight movementkeys clear
Afteron a refused step (move.go:74,:136,:31). Whatactually makes the wedge impossible is much narrower — the cycle contains two
unconditional turn-takers,
'.'and's', verified to survive blocked-all-directions,
Held, bear trap,NoCommand>0, maze, and a pending--More--.Why that is blocking rather than a follow-up. This comment is the safety
argument for the fix. Someone later trimming the script to movement keys —
entirely reasonable if you believe "every key takes a turn" — reintroduces the
exact 30s wedge this PR exists to remove. The reviewer even demonstrated it:
setting the script to
" "wedges precisely liketestTermdid,panic: test timed out after 30s.So the false sentence is not decoration; it is a trap aimed at the next
person, in the file whose whole subject is a trap that caught five sessions.
And consistency matters here. I blocked PR #26 five times, PR #37, and
PR #38 on exactly this class: a claim recorded as fact that nobody had
verified. Waiving it now, on a PR that is otherwise the best work in this
backlog, would say the standard applies to weak work and bends for strong
work. It is a two-comment edit.
Second item, same class: the unbounded-wait comment states the bound more
strongly than it holds. A live-but-slow handoff (draining each request but
slower than
autoSaveWait) gives a worst case ofwantSaves × autoSaveWait=250s, not one
autoSaveWait. Remote — it needs ~10s of starvation per saveagainst a measured 0.12s per 1000 turns — and not a regression, but the
comment should say what is true.
None of this touches the substance, which is excellent and is not being
re-litigated:
DATA RACEreports (the reviewer's count differs from the author's 139 and PR #26's
73/96/113 — machine-dependent, and the property is what is pinned, not the
number).
saves taken = 0, want 25instead of a timeout — the fix converted anunreadable hang into an assertion.
main4/4 "ran out ofturns", branch 0/4.
One number in the review strengthens the diagnosis beyond how the author
stated it: each old failure took 0.12 seconds — 1000 turns in a tenth of
a second. The cap was never a wall-clock allowance at all, which is why every
earlier attempt to reproduce it by loading the host failed. That belongs in
the record.
Rework brief: correct the two comment claims and the commit message. Nothing
else.
cb7ddfdb67to3061931291Rework: F1 and F2 corrected, head now
3061931Both findings accepted, neither rebutted. Both were the same defect — a claim
recorded as fact — and both now say what the source actually does. No test logic
and no game code changed: against
cb7ddfdthe diff is comments, the commitmessage,
TODO.mdprose, and this PR's body.make checkre-run whole afterthe edits.
Verified against the source before writing a word
I checked every claim myself rather than transcribing the brief. All of it held;
I found nothing wrong in it. What I read, and what it says:
' 'does not take a turngame/tables.go:804-805' ': func(g *RogueGame) { g.After = false }, carrying the comment// "legal" illegal command. Confirmed.Afteron a refused stepgame/move.go:74,:136,:31moveResolve, illegal diagonal inmoveTarget, confused step landing in place inmoveHero. Confirmed — andgame/move.go:143is a fourth, a run into the hero's own square.'.'is an unconditional turn-takergame/tables.go:801-803dispatchKey(game/command.go:221-226) finds it incommandHandlers, calls it and returns, so nothing downstream can clearAfter. Confirmed.'s'is an unconditional turn-takergame/tables.go:770,game/command.go:521-551's': (*RogueGame).search; neithersearch, norsearchSpot, norlookwritesAfteron any path. Confirmed by reading everyg.After =site in the package — they are inarmor.go,things.go,potions.go,move.go,options.go,tables.goandcommand.go, none of them in the search chain.NoCommand > 0leavesAftertruegame/command.go:62-75,:117playTurntakes theif g.NoCommand != 0arm and skipsexecuteCommandentirely;Afterwas set true inturnUpkeepand nothing clears it. Confirmed.Helddo not clear it eithergame/move.go:20-25,:156-159After. Confirmed.driveTermsupplies no newline, andwaitFor('\n')would wedge on itgame/io.go:286-292,game/rip.go:69,game/score.go:106waitFor('\n')loops onreadcharuntil'\n'or'\r', which this script never yields. Both call sites are the death and score paths, whichfortify(game/run_test.go:14-22) prevents reaching. Confirmed unreachable, and now documented.F1: the safety argument, rewritten
The false sentence is gone from all four places it appeared — the test's script
comment, the
driveTermdoc, the commit message and the PR body.driveTerm'sdoc now carries the argument, and it is the one that survives checking:
> Repeating the script is necessary but nowhere near sufficient, and the
> difference is what anyone editing one of these scripts has to know. Most keys
> take a turn only conditionally.
' 'is the "legal" illegal command and> clears
Afteroutright (tables.go). All eight movement keys clear it whenever> the step is refused: a wall or the map edge (move.go
moveResolve), an> illegal diagonal (
moveTarget), or a confused step that lands back in place> (
moveHero). A script of nothing but those keys wedges exactly the way>
testTerm's tail does, repetition or no repetition — with the script set to> just
" "this drive hits the 30s package timeout insidecommand().>
> What actually makes the wedge impossible is that the cycle always contains at
> least one unconditional turn-taker, and the scripts here carry two:
'.',> the rest command, whose handler is empty, and
's', search, which writes>
Afteron no path. Nothing refuses either one — not being blocked in all> eight directions, not
Held, not stuck in a bear trap, and not>
NoCommand > 0, whereplayTurnskipsexecuteCommandaltogether and>
Afteris simply left true. Trim both out and the wedge this test exists to> remove comes straight back.
The
" "probe result is the reviewer's, credited as such in my report ratherthan claimed as my own run; everything else in that paragraph I read out of the
files listed above.
The script line in
TestAutoSaveOnSignalRacesTurnLoopnow points at it insteadof asserting the false general rule:
> The
'.'and the's'are what make an unbounded drive safe, and at least> one of the two has to stay in the cycle: see
driveTerm.One deliberate difference from the brief's phrasing: "removing both would
reintroduce the wedge" is what I wrote, not "removing either". Removing one of
the two is survivable — one unconditional turn-taker is enough — and the test's
other drive script (
"s . ", inTestAutoSaveOnSignalWithoutASaveFile) hasboth as well. The invariant is "at least one", and that is what both comments
state.
F1, folded in: the missing newline
Same paragraph, as an explicit precondition of the fake rather than an accident:
> One further precondition, from what this fake does not supply:
testTerm's> tail answered a newline every other read and this does not. Nothing reachable
> from these scripts asks for one —
waitFor('\n')sits on the death and score> paths (rip.go, score.go), which
fortifyprevents from ever being reached —> but a script that could reach them would park in
waitForfor ever.F2: the bound stated as it actually holds
driveUntilDone's doc no longer claims oneautoSaveWaitcovers every case:> Dropping it costs no termination guarantee, because the bound belongs to the
> code under test and not to this loop: each
AutoSaveOnSignalcall returns> within the timeout the caller hands it, so the saving goroutine always
> finishes and
donealways closes. That bound is worth stating exactly,> because it is not one
autoSaveWait.>
> A handoff that has stopped answering altogether costs one, in total, however
> many saves were asked for.
g.sigSaveis one deep, so the unserviced request> stays in the channel and every later call finds it full and reports failure
> immediately — measured at 10.0s for 25 saves with the service point deleted
> from
command(). What fails is then the caller's own assertion, the count of> saves actually taken, which says far more than "out of turns" ever did.
>
> A handoff that still drains every request but takes longer than
>
autoSaveWaitto do it is the worst case, and costs one timeout per save:>
wantSaves * autoSaveWait, 250s at these constants, which would run past the> package timeout rather than reach the assertion. It takes about ten seconds of
> scheduler starvation per save to get there, against a regime measured at 0.12s
> per 1000 turns, so it is remote — and the 1000-turn cap did not bound it
> either, a turn count being no kind of time bound.
go test -timeout 30sis> the backstop under all of it.
I re-derived the 250s rather than taking it: the post is non-blocking and only
fails when
sigSaveis already full, so a handoff that eventually drains eachrequest leaves the channel empty for the next call, which then gets its own full
autoSaveWait. 25 x 10s. The 10.0s figure is the reviewer's measurement ofmutation B, credited; my own run of the same mutation was 10.16s.
The
autoSaveWaitconst comment no longer implies a single wait either — it nowpoints at
driveUntilDone"for what that bound comes to".Also corrected in the record
figure added — 1000 turns burned in a tenth of a second, which states the
diagnosis better than "too slow under load" and explains why loading the host
never reproduced it. The
DATA RACEcount is now given as a range acrossmachines (139 here, 62-110 in review, 73/96/113 in
#26) with the property named as the thing
pinned, rather than as a single number.
TODO.md: the Completed Steps entry repeated both false claims; bothparagraphs are corrected in the same commit. The entry is kept and "Next Step"
is untouched — still the release step.
nuance (their top trace shows the racing write from
Window.AddCh/look/turnUpkeep, notexecuteCommand; both appear, same class) and theirindependent 4/4-vs-0/4 A/B. Strictly speaking outside "comments and the commit
message", but it carried the identical false sentence and it is my own text;
flagging it here so it is a visible choice rather than a silent one.
Gate
make fmt, thenmake checkre-run in full after the last edit — itshort-circuits, so nothing was carried over from the earlier run.
The
gamepackage — the only one this touches — ran for real at 2.491s ratherthan reporting cached. Lint used a private empty
GOLANGCI_LINT_CACHEunder myown throwaway worktree, reported no
parallel golangci-lint is running, andnamed no path outside it; the
gomodguarddeprecation(#29) is the only warning and predates
this branch.
.golangci.ymlstill sha256021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcband absentfrom the diff; nothing under
game/testdata/; no Dockerfile, CI orscript/;git diff --checkclean; still fast-forwardable ontoorigin/main@13caec4;still one commit whose title ends
(closes #36), amended and force-pushed with--force-with-lease. Work was done in a throwaway worktree and the shared cloneis untouched on
main.Independent confirmation review — PASS
Verified at head
3061931against #36.Scope as directed: the corrected prose, re-checked against the source rather than
against the text, plus the gate and one mutation. The substance that passed at
cb7ddfdwas not re-litigated.The delta is comment-only.
git diff cb7ddfd 3061931touchesTODO.mdproseand
game/autosave_test.gocomments only. The single non-comment line inside thechanged hunks —
term := &driveTerm{...}atgame/autosave_test.go:43— isunchanged context. No test logic, no game code.
F1, clause by clause
' '—game/tables.go:804-805,g.After = false, carrying the// "legal" illegal commandcomment. Confirmed.'.'—game/tables.go:801-803, empty handler;dispatchKey(
game/command.go:222-226) finds it incommandHandlers, calls it and returns(0, false), so nothing downstream can clearAfter. Unconditional.'s'—game/tables.go:770→search(game/command.go:521-552) →searchSpot(:557-595),searchFloor(:599-),look(game/misc.go:22).misc.gohas noAfterassignment at all, andcommand.go's are at117/239/273/288/342/357/377/416/891/926 — none inside 521-620. Unconditional.
game/move.go:31(confused step landing in place,moveHero),:74(wall ormap edge —
offMapreturns' 'at:132and falls into that arm ofmoveResolve),:136(illegal diagonal,moveTarget),:143(running intothe hero's own square,
moveTarget). The eight keys dispatch straight tomoveHerowith no wrapper (game/tables.go:700-707), and the rest of thatcall tree —
moveEnter,springTrap(move.go:307),moveOnto,fight.go,pickUp(pack.go:307-338) — writesAfternowhere. Every remaining site inthe package sits in a handler for a key absent from these scripts (
wield,wear,takeOff,dropIt,doZap,option,quaffHaste,promptPackItemat
pack.go:403-);move.go:9isstartRun, reachable only from run keys,also absent.
Held(move.go:156-159) and bear trap (move.go:20-25)both return without touching
After;NoCommand > 0takes theplayTurnelse-arm (
command.go:62-75) and skipsexecuteCommand, leavingAftertruefrom
turnUpkeep(command.go:117). Confirmed.Both scripts carry both keys:
"h j k l y u b n s . "(:43) and"s . "(
:306)."both" vs "either" — the reworker is right and the brief was wrong
One unconditional turn-taker suffices. Drop
'.'from"h j k l y u b n s . "and's'still terminates the drive; drop's'and'.'still does. So "removing both would reintroduce the wedge" is true, and"removing either" would have been false — it would have told a reader that
's'alone cannot carry the drive, which is exactly the wrong conclusion. The
invariant is "at least one", and that is what
game/autosave_test.go:41-42and:498-505state.F2 — both bounds follow from the code
AutoSaveOnSignal(game/save.go:745-768) posts non-blocking (:748-755) thenselects on
req.doneagainst a timer (:762-767);driveTerm.Interruptis ano-op (
:525), so the call is bounded unconditionally.sigSave, so calls 2-25 fail at once. OneautoSaveWaitin total — measuredbelow at 10.11s.
and buys its own full wait.
wantSaves * autoSaveWait= 25 x 10s = 250s(
:50,:20) againstgo test -timeout 30s(Makefile:37).The
autoSaveWaitconst comment (:17-21) now defers todriveUntilDone"forwhat that bound comes to" and no longer implies a single wait.
The newline precondition
waitFor('\n')(game/io.go:286-294) loops onreadcharuntil'\n'/'\r',which
driveTermnever yields. Its only two call sites aregame/rip.go:69(
death) andgame/score.go:106(score). Unreachability verified, notaccepted:
fortify(game/run_test.go:14-22) pins HP/MaxHP/Exp/FoodLeft at30000 and zeroes
NoCommand/NoMovebefore everycommand(), closing everydeathcaller (fight.go,move.go:393/:425,sticks.go:508-510,daemons.go:140starvation,rip.go:276);score's other callers needquit(
game.go:343),totalWinner(command.go:783) or the-soption(
score.go:259), none of which this script's keys can reach.Disclosed as a judgement call rather than passed silently: the comment credits
that unreachability to
fortifyalone.fortifycloses the death path; thequit/win/
-sroutes intoscoreare closed by the script's key set instead. Theoperative clause — "nothing reachable from these scripts asks for one" — is true
as written, so I am not filing it, but the attribution is narrower than the
conclusion it carries.
The false sentence
Gone from the working tree, the commit message and the PR body. The only
surviving "takes a turn" strings are
TODO.md:76("neither key takes a turn",about
testTerm's space/newline tail — true:'\n'falls to thedispatchKeydefault at
command.go:238-239, which setsAfter = falseand callsillcom)and
game/autosave_test.go:489("Most keys take a turn only conditionally"),which is the correction itself.
Also checked
DATA RACEis given as a cross-machine range with the property named as what ispinned. 0.12s appears at
TODO.md:70,game/autosave_test.go:113and in thecommit message, always as "per 1000 turns", not overstated.
TODO.mdkeeps itsCompleted Steps entry at
:38, and "Next Step" (:30-34, the release step) isabsent from the diff.
Mutation re-run —
serviceAutoSaveRequestdeleted fromcommand()(
game/command.go:16; thereadcharservice point atgame/io.go:189firesonly on
!ok, whichdriveTerm.ReadCharnever returns):--- FAIL: TestAutoSaveOnSignalRacesTurnLoop (10.11s),autosave_test.go:70: saves taken = 0, want 25, package 10.293s, zeroDATA RACE. The comment edits disturbed nothing. Reverted afterwards; the workwas done in a throwaway worktree and the shared clone is untouched on
main.Gate —
make checkgreen:fmt-checkclean, lint 0 issues (privateempty
GOLANGCI_LINT_CACHE, noparallel golangci-lint is running, no pathoutside my worktree; the
gomodguarddeprecation,#29, is the only warning), tests real at
2.797s rather than
(cached). Two furtherGOFLAGS=-count=1 make testruns,both green and race-clean (2.454s, 2.367s).
.golangci.ymlsha256021cc83f...46bcband absent from the diff; nothing undergame/testdata/; noDockerfile, CI or
script/; no Claude or Anthropic reference and no attributiontrailer anywhere; single commit whose title ends
(closes #36);git diff --checkclean; fast-forwardable ontomain@13caec4. This repo hasno CI by design, so the head carries no statuses, as
maindoes not.One inaccuracy in the record rather than in the change, stated for completeness:
the rework comment lists the package's
g.After =sites as being "inarmor.go,things.go,potions.go,move.go,options.go,tables.goandcommand.go". They are also inpack.go:423/:433,sticks.go:22andweapons.go:123/:130/:136. The conclusion that enumeration supports — nonein the search chain — is true, and none of the omitted sites are reachable from
these scripts either, so nothing shipped is wrong.
Manager notes (the review is in its own comment above).
Verdict accepted: PASS. Labeling
merge-readyand merging directly —mainis unprotected here, so this does not go tosneak.The adjudication went against me, correctly. I told the reworker to warn
against removing "either" unconditional turn-taker; they used "both" and were
right. One suffices, so the invariant is "at least one" — and my phrasing
would have told a reader that
's'alone cannot carry the drive. That is thewrong conclusion in the exact direction this rework existed to prevent: I
sent the PR back for a comment that misled about what keeps the loop alive,
and my correction to it would have misled about the same thing the other way.
Good instinct by the reworker to deviate and flag it rather than comply.
Blocking this on a comment was the right call, and the review shows why.
The corrected invariant is not a tidier way of saying the old one — it is a
different, narrower claim, and the difference is load-bearing.
' 'clearsAfter; all four movement-refusal sites clear it (move.go:31,:74,:136, and:143, which the reworker found and my brief missed); thereviewer then confirmed there is no fifth by checking that the eight keys call
moveHerowith no wrapper and thatmoveEnter,springTrap,moveOnto,fight.goandpickUpwriteAfternowhere. What actually holds the drive upis
'.'and's', and now the comment says so.Both F2 bounds check out — one
autoSaveWait(measured 10.11s) for a deadhandoff,
25 × 10s = 250sfor a live-but-slow one against a 30s packagetimeout — and the
autoSaveWaitconst comment no longer implies a single wait.Two disclosures I want on the record, neither shipped wrong:
fortifyalone for closingwaitFor('\n').fortifycloses the death path; the quit /totalWinner/-sroutes are closed by the script's key set instead. The operative clause("nothing reachable from these scripts asks for one") is true as written, so
the attribution is narrower than the conclusion — worth knowing, not worth a
round.
g.After =omitspack.go,sticks.go,weapons.go. The conclusion it supports (none in the searchchain) is true and none are script-reachable. A record inaccuracy in a PR
comment, not in the tree.
Reporting both rather than only the verdict is what makes the verdict worth
something.
What this PR actually fixed, now settled: a guard test for a
save-destroying data race was failing intermittently, and every earlier attempt
to reproduce it by loading the host failed — because the trigger was never
wall-clock at all. Each old failure burned 1000 turns in 0.12 seconds; the
cap was a scheduler-starvation detector wearing a turn-budget costume. Five
sessions watched it fire and filtered away the evidence; changing the
instruction from "capture it" to "write the full output to a file and read it"
cracked it in one pass.
With this merged, the repo's
Next Step— tag a release once a full gamecompletes without defects (#8) — is the only thing left in the queue, and it
needs a human at an 80x24 terminal.