0bef53bd113010a0356bf1fac3ec67a645638422
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
clawbot
|
3061931291 |
test: drive the autosave race test to a condition, not a turn count (closes #36)
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. Each old-code failure took 0.12
seconds - 1000 turns burned in a tenth of a second - which is why every
attempt to reproduce this by loading the host failed: the cap was never
a wall-clock allowance at all.
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, so the saving goroutine always
finishes. A handoff that has stopped answering costs one autoSaveWait in
total, because g.sigSave is one deep and an unserviced request stays in
the channel for every later call to find full and fail on at once; what
fails is then the real assertion, "saves taken = 0, want 25", rather
than "out of turns". That is not the worst case, and the comment states
the bound that actually holds: a handoff that drains each request but
slower than autoSaveWait costs one timeout per save, wantSaves *
autoSaveWait = 250s, which would run past the 30s package timeout. It
needs about ten seconds of scheduler starvation per save against the
0.12s-per-1000-turns regime above, so it is remote, and a turn cap did
not bound it either.
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.
Repeating is necessary and not sufficient, and the comment on driveTerm
says which property is load-bearing: ' ' clears After outright and all
eight movement keys clear it on a refused step, so a script of only
those keys wedges exactly as testTerm's tail did - with the script set
to " " the drive hits the 30s timeout inside command(). What makes the
wedge impossible is that the cycle always contains an unconditional
turn-taker, and these scripts contain two, '.' and 's', neither of which
can be refused by being blocked in all directions, Held, in a bear trap,
or under NoCommand > 0. Trimming both out would bring the wedge back.
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 a flood of DATA RACE reports (139 here, 62-110
on another machine; the property is what is pinned, not the number),
the encoder reading what the turn loop 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.
|
||
| 1142f43aed |
Restore three lost C behaviors: schtick message, forced redraw, greeting (closes #13)
Three behaviors from 5.4.4 that the port dropped silently. Each is a few
lines; grouped because they are all "restore something C did".
1. sticks.c 237: the "otherwise" arm closing do_zap's switch printed
"what a bizarre schtick!", and doZap had turned it into doing nothing.
The arm is under #ifdef MASTER, not under a runtime wizard test, so in
the MASTER build this port is it printed for every player and must not
be gated on g.Wizard. WS_NOP is a case of that switch in its own right
("when WS_NOP: break;"), so "no handler ran" cannot be the trigger:
the wand of nothing does nothing quietly. C's switch covers all 14 WS_
values, so its otherwise is reachable only for an o_which outside the
table, which is what Object.hasValidWhich already screens for. All
three arms fall through to obj.Charges--, as C's do.
2. command.c 288-291: CTRL('R') is "after = FALSE; clearok(curscr, TRUE);
wrefresh(curscr);" — a forced full repaint. The port called
g.refresh(), the ordinary diffing blit, which cannot fix the only
situation the command exists for: a screen corrupted by another
program's output leaves the game's record of it still correct, so the
diff sends nothing. New Terminal.Repaint (tcell Screen.Sync, which
discards tcell's record of the terminal rather than diffing against
it), Screen.Repaint and g.repaint(), implemented in term.Tcell and in
both headless test terminals. Named for the curses operation: the
interface is the game's abstraction, not tcell's. It repaints what was
last rendered — C repainted curscr, not stdscr — so it takes no
window.
3. main.c 107-113: the startup greeting existed nowhere in the tree. New
game.Greeting, printed on stdout by cmd/rogue/main.go before
term.New(), the port's initscr(). Only the wizard wording is #ifdef
MASTER; the other is unconditional. The %d is dnum, which main.c has
just assigned to seed, so it is Params.Seed. Neither wording ends in a
newline. Two placement details the tests pin: the printf sits after
parse_opts, so a ROGUEOPTS name= is what the player is greeted by; and
it sits after the -s/-d handling and after restore(), which never
returns, so a resumed game does not announce that a dungeon is being
dug (digsNewDungeon).
Greeting parses ROGUEOPTS into a throwaway game built the way New builds
the real one, tables and home directory included: ParseOpts handles every
option, not just the one the greeting reads, and inven= is matched
against inv_t_name[], which lives on the game.
All three message strings verified byte-for-byte against origin/c-master
sticks.c and main.c. No RNG call is added on any path and nothing under
game/testdata/ changed; TestSeedCompatItemTables is green against the
untouched golden.
Mutation-proved, each behavior removed in turn with only its own test
failing: dropping the message arm fails
TestZapUnhandledWandSaysBizarreSchtick; extending the message to WS_NOP
fails TestZapWandOfNothingIsSilent; putting g.refresh() back fails
TestRedrawCommandForcesFullRepaint; swapping the two wordings, and
ignoring the ROGUEOPTS name, both fail TestGreeting; greeting on the
restore path fails TestDigsNewDungeon.
ARCHITECTURE.md 5.3 gains Repaint and why a blit cannot substitute for
it; nothing here is deliberately dropped, so section 9 is unchanged.
TODO.md gets a Completed Steps entry; Next Step deliberately not rotated,
this being out-of-band issue work.
|
|||
| f7670cf86a |
docs,save: correct a stale TODO claim and rename encodeSnapshot (closes #27)
Four cleanups recorded as advisories during the PR #26 review and deliberately kept out of it. No behaviour change. The 2026-08-09 sig-leave (closes #12) TODO entry still argued in the present tense that declining to save on SIGINT/SIGQUIT was also the safe choice, "because AutoSave gob-encodes live state that the main goroutine is still mutating, after removing the old file". Both halves stopped being true with #24: the encode runs on the game goroutine and saveFile is CreateTemp/Sync/Chmod/Rename with no Remove. The paragraph is now in the past tense and marked superseded, pointing at the fix/autosave-race entry, and says the split stands on C and on semantics alone — which is what the current savesOnSignal comment says. The false claim was in the #12 entry, not the #24 one; the latter's account of the old remove-then-write is correctly historical and is untouched, as is the err113 mention in the 2026-07-06 entry. encodeSnapshot becomes writeSnapshotFile: it encodes, fsyncs, chmods 0400 and closes, and the old name claimed only the first of those. Its doc comment now names all four and why the fsync is there. TestAutoSaveOnSignalWhileInShellEscape used t.Error for a precondition, so a save that was never taken fell through into assertRestorable, which can then only report a second, derived failure. t.Fatal, matching the identical assertion in the blocked-on-input test. serviceAutoSaveRequest's doc comment carried a 24-column stub line ("The result is still a") left by an earlier edit. gofmt does not rewrap comments, so fmt-check was legitimately green and nothing would have caught it; the paragraph is rewrapped to the block's width. Next Step deliberately not rotated: out-of-band issue work. |
|||
|
clawbot
|
3a01283358 |
fix: take the signal-time autosave on the game goroutine (closes #24)
The SIGHUP/SIGTERM handler gob-encoded the live game tree from the signal goroutine while the game goroutine was mid-turn mutating it, and AutoSave removed the save file before encoding — so the failure mode was not a stale save but a deleted one followed by a possibly torn replacement, with a window in which the player had neither. The suite has run under -race since 2026-08-09 and was green because nothing had ever driven the turn loop concurrently with a signal: evidence of untested, not of safe. The handler no longer writes anything. AutoSaveOnSignal posts a request, wakes the input read, and waits up to signalSaveTimeout for the game goroutine to take it; the encode runs on the goroutine that owns the state, at the three points where that goroutine can sit: between turns (command), on waking from a blocked readchar, and while parked in the `!` shell escape (runShellEscape, which now runs the shell on a helper goroutine so a hangup during it still rescues the game). Blocked on input is the case that matters — a dropped connection lands while the player is thinking, so a flag checked only between turns would never be looked at. Terminal.ReadChar therefore returns (byte, bool), with ok false meaning "woken by Interrupt, no key", and term.Tcell posts a tcell.EventInterrupt onto tcell's own event queue to unpark PollEvent. readchar services the request and reads again, so no caller sees it. Running the shell on a helper goroutine would also have moved term.Tcell.ShellEscape's panic on a failed Screen.Resume onto it, and a panic at the top of any goroutine terminates the process without running the deferred calls of the others — including cmd/rogue/main.go's `defer t.Fini()`. The tty would have been left raw on precisely the path where the terminal is already broken, which is issue #12's failure on a path this change created. runShellEscape therefore recovers the helper's panic and re-raises it on the game goroutine, whose stack has the restore in it, so "every path restores the terminal via Terminal.Fini before exiting" stays true. saveFile writes a temporary file in the save's own directory, fsyncs it and renames it over the target instead of truncating in place, so a save that fails — or never happens because the deadline ran out — leaves the player's previous save whole. What the handoff guarantees is stated exactly rather than flatteringly: the encode runs on the state-owning goroutine, so the snapshot is internally consistent and restorable, but it is not necessarily taken between commands. Only the check at the top of command is; the other two service points both sit inside a command call already under way. readchar is reached from mid-command prompts (--More--, askOverwrite, getStr, the direction and pack prompts) with the command's mutations already applied, and runShellEscape is reached from shell, an ordinary '!' command handler dispatched inside command, with that turn's DoDaemons(Before) and DoFuses(Before) already fired and its AFTER pass not yet. Restoring re-enters playit at the top of command, so either way the rest of that command is lost and a fresh BEFORE pass runs on top of the one in the snapshot. The SIGINT/SIGQUIT no-save decision and the single-signal-read ordering guarantee are untouched. pendingSaver reads the game out from under its mutex rather than delegating with it held, because the delegated call now blocks until the save is taken. |
