test: drive the autosave race test to a condition, not a turn count (closes #36) #40
Reference in New Issue
Block a user
Delete Branch "fix/autosave-turn-budget-36"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.