Repository navigation
fix(release): a SHA256SUMS replaced between its listing and its read is read again (flaky nightly test) - #2822
Conversation
…is read again The nightly's two runs at once replaced SHA256SUMS under each other's read: the read 404ed, counted as no SHA256SUMS, and each run uploaded it again until one used its five uploads (the flaky test_nightly_two_runs_at_once_end_at_the_newer_commit). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The one red check here (test_linux_saves_move, ubuntu part 1/2) is not this PR: it is an order-dependent bug on main in host/hos_main.c's copy_link (an absolute link listed after the entry it names keeps the old place). Fix and a stronger test: #2832. A rerun of the failed job (needs a maintainer) should pass. |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, this is a good catch and a clean fix for the flaky nightly test. Read from the diff only (I did not run anything); CI is the test evidence.
The race. uploaded() now re-lists and re-reads when the read itself fails, so a SHA256SUMS replaced under a new id between the listing and the read is picked up instead of turning into "nothing there". The retry is bounded (3 tries) and only triggers on a failed read, never on a content mismatch: a real difference between the sums and the files still comes out of cmp in finish()/check() exactly as before, so it cannot be hidden. The retry is also the only thing that changed; finish() and its 5-upload limit with the clear error are untouched.
Shell. Fine under set -euo pipefail: the read goes to a temp file, so a half-written body is never printed before a failed gh, and the [[ -n $asset ]] || break is safe. Nit, not blocking: a persistent read failure after 3 tries still answers nothing silently (as before, and finish() then reports it); a one-line stderr note there would make the logs clearer. Another nit: if assets itself fails under set -e the temp file is left behind (the other helpers use a trap or have the same shape, so I would not bother).
Test. The fake gh's race counter makes the replacement deterministic: each read of SHA256SUMS bumps the id and 404s, race: 2 needs exactly the third try, and the test checks both check and finish and that nothing uploaded SHA256SUMS. On main's script the first read 404s, || true makes it empty, so check fails and finish uploads again, as the description says. That matches the cause from the failing runs' logs (404 x3 then the five-uploads error), so it explains main's red runs too.
One residual window (non-blocking, a separate follow-up if you want it). upload() deletes the old SHA256SUMS and then POSTs the new one, so there is a short moment when no SHA256SUMS is listed at all. A run that lists in that moment still gets "none" and uploads again. This PR closes the replaced-under-a-new-id case that the CI logs show; the delete/POST gap is much narrower and is what #2779's extra check after the fifth upload would catch.
CI. ubuntu part 2/2 (where test_release_scripts runs) passes. The one red check, ubuntu part 1/2, is test_linux_saves_move, unrelated to these files; the author points at #2832 for it. Nothing here for rule 3c beyond the test failing before and passing after, which holds.
… place (#2832) `test_linux_saves_move` fails now and then on CI's ubuntu part 1/2 ("a link to an entry that moves too: last names another folder", e.g. on #2822, which only touches release scripts), and the same bug can point a player's saves link at a folder that no longer exists after the first start on Linux. **Cause.** When `user/` moves to `$XDG_DATA_HOME/hookshot`, `copy_link` (host/hos_main.c) makes each link again. A link to an entry that moves too is pointed at that entry's new place, but only when `realpath` resolves it. An absolute link (`user/last -> <game>/user/save/<id>`) listed after `save/` names nothing once `save/` has gone. `realpath` then fails, the link's text is copied unchanged, and the new link names the old place, which is removed at the end of the move. Which entry comes first depends on the file system's directory order, so the check passes on most runners and fails on some. **Fix.** The link's text is checked against the folder on the move both as `realpath` names it and as given, after `readlink` too. Links written from where they stand and links that name nothing are unchanged. **Test.** The case "a link to an entry that moves too" now makes 36 more absolute links to `save/<id>` (`last-a` ... `last-9`), so on any directory order some are listed after `save/`. Without the fix, those name the old place. The check is Linux only (it skips on Windows), so the run on this PR's ubuntu job is the proof. `python -I -B tests/run_all.py` on Windows: 419 passed; the 5 that fail (fpx_sticky, bake_portability, input_background, lift_cache, spawn_emit) fail on `main` on this PC too. **Overlap.** #2317 moves this code (`copy_link`, `move_old`/`move_new`) into the per-user data folder work and has the same `realpath`-only check. Whichever merges second should carry the change across. **Not tested in the game.** The Linux move runs once, at a first start with a `user/` beside the game, which can't be run on this PC (Windows). Wanted: someone on Linux with a `user/` folder holding an absolute symlink to a file in `user/save/` to start the game once and check that the link in `~/.local/share/hookshot` points inside that folder (`readlink`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: breakthetargets <81792782+breakthetargets@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @hckrman101 for the review and the merge. Agreed on the delete/POST window: it is narrower than the replaced-under-a-new-id case the CI logs showed, and #2779's check after the fifth upload covers it. The two nits (a stderr note after the third failed read, the temp file left when |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tests/test_release_scripts.py'stest_nightly_two_runs_at_once_end_at_the_newer_commitfails now and then on the ubuntu part 2 check (klimovich008 saw it on #2708 and #2732 today), and the same race can fail a real nightly when the Linux and Windows runs finish together:release_files.sh finishgives up with "SHA256SUMS still differs from the files after 5 uploads".Cause
From the failing runs' logs (e.g. run 43237869157): after
nightly: moved forward ...and its uploads, the run printedgh: Not Found (HTTP 404)three times, then the five-uploads error.uploaded()lists the release's files, then reads SHA256SUMS by its id; the other platform's run replaces SHA256SUMS (a new id) between those two calls, the read 404s, and|| trueturned that into "no SHA256SUMS". So the run uploads it again, which replaces it under the other run's read, and the two runs ping-pong until one has used its five uploads (in the testssleepis a no-op, so the runs retry in step).Change
uploaded()lists and reads again (up to three times) when the read fails, so a file replaced meanwhile is read under its new id instead of counting as missing. A read that keeps failing still answers nothing, as before.race: {"SHA256SUMS": n}: the next n reads of it find it replaced under a new id), andtest_sums_replaced_between_listing_and_read_are_read_againchecks thatcheckpasses andfinishuploads nothing through two such replacements. On main's scriptcheckfails (the empty read) andfinishuploads SHA256SUMS again.Overlap: #2779 (Nitnek, draft) adds one more check after the fifth upload in
finish(); this PR does not touchfinish()and the two merge in either order. Both together: this one stops the ping-pong, #2779 catches a last upload not yet visible. #2626 changes other lines offinish().Tested
python -I -B tests/run_all.pyon Windows 11 (419 passed; 5 failed, the same 5 as main on this PC: compiler headers and Python modules here).test_release_scripts.pyis skipped on Windows (it needsfcntlandjq), so its run is this PR's Linux CI.bash -n scripts/release_files.sh.🤖 Generated with Claude Code