fix(release): a SHA256SUMS replaced between its listing and its read is read again (flaky nightly test)

github-sync commented

2026-11-12 15:49:09 +00:00

Owner

@nullifyshot opened this pull request on GitHub · 2026-11-12 15:30 UTC · original
Merged on GitHub in 7610111146

tests/test_release_scripts.py's test_nightly_two_runs_at_once_end_at_the_newer_commit fails 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 finish gives 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 printed gh: 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 || true turned 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 tests sleep is 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.
The stand-in gh can replay that race (race: {"SHA256SUMS": n}: the next n reads of it find it replaced under a new id), and test_sums_replaced_between_listing_and_read_are_read_again checks that check passes and finish uploads nothing through two such replacements. On main's script check fails (the empty read) and finish uploads SHA256SUMS again.

Overlap: #2779 (Nitnek, draft) adds one more check after the fifth upload in finish(); this PR does not touch finish() 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 of finish().

Tested

python -I -B tests/run_all.py on Windows 11 (419 passed; 5 failed, the same 5 as main on this PC: compiler headers and Python modules here). test_release_scripts.py is skipped on Windows (it needs fcntl and jq), so its run is this PR's Linux CI.
bash -n scripts/release_files.sh.
Not tested: the flake's own rate. It is a race; the new test pins the mechanism deterministically, and the ubuntu part 2 check on later PRs is where it would show again.

🤖 Generated with Claude Code

> [@nullifyshot](https://github.com/nullifyshot) opened this pull request on GitHub · 2026-11-12 15:30 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2822) > **Merged on GitHub** in https://git.magicbean.dev/MagicBeanDev/Hookshot/commit/7610111146a72bfd9e30b75e0bbcccdc91297f70 `tests/test_release_scripts.py`'s `test_nightly_two_runs_at_once_end_at_the_newer_commit` fails 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 finish` gives 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 printed `gh: 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 `|| true` turned 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 tests `sleep` is 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. - The stand-in gh can replay that race (`race: {"SHA256SUMS": n}`: the next n reads of it find it replaced under a new id), and `test_sums_replaced_between_listing_and_read_are_read_again` checks that `check` passes and `finish` uploads nothing through two such replacements. On main's script `check` fails (the empty read) and `finish` uploads SHA256SUMS again. Overlap: #2779 (Nitnek, draft) adds one more check after the fifth upload in `finish()`; this PR does not touch `finish()` 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 of `finish()`. ## Tested - `python -I -B tests/run_all.py` on Windows 11 (419 passed; 5 failed, the same 5 as main on this PC: compiler headers and Python modules here). `test_release_scripts.py` is skipped on Windows (it needs `fcntl` and `jq`), so its run is this PR's Linux CI. - `bash -n scripts/release_files.sh`. - Not tested: the flake's own rate. It is a race; the new test pins the mechanism deterministically, and the ubuntu part 2 check on later PRs is where it would show again. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

github-sync added 1 commit

2026-11-12 15:49:09 +00:00

fix(release): a SHA256SUMS replaced between its listing and its read is read again e32b159b8e

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 <[email protected]>

github-sync referenced this pull request

2026-11-12 15:49:15 +00:00

fix(saves): a link to an entry that moved first follows it to the new place #2832

github-sync commented

2026-11-12 15:54:04 +00:00

Author

Owner

@nullifyshot commented on GitHub · 2026-11-12 15:48 UTC · original

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.

> [@nullifyshot](https://github.com/nullifyshot) commented on GitHub · 2026-11-12 15:48 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2822#issuecomment-6828389831) 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.

github-sync referenced this pull request from a commit

2026-11-12 17:57:20 +00:00

fix(release): a SHA256SUMS replaced between its listing and its read is read again (flaky nightly test) (#2822)

github-sync referenced this pull request from a commit

2026-11-12 17:57:20 +00:00

fix(saves): a link to an entry that moved first follows it to the new place (#2832)

github-sync referenced this pull request from a commit

2026-11-12 17:57:27 +00:00

Merge main (65a4d874) for #2822's fix of the flaky nightly test

github-sync closed this pull request

2026-11-12 17:59:33 +00:00

github-sync commented

2026-11-12 17:59:34 +00:00

Author

Owner

@hckrman101 approved on GitHub · 2026-11-12 17:14 UTC · original

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.

> [@hckrman101](https://github.com/hckrman101) approved on GitHub · 2026-11-12 17:14 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2822#pullrequestreview-5878837756) 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.

github-sync commented

2026-11-12 17:59:34 +00:00

Author

Owner

@nullifyshot commented on GitHub · 2026-11-12 17:28 UTC · original

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 assets fails) are fair; I'll keep them for a small follow-up if the nightly test flakes again rather than open one for logs alone.

> [@nullifyshot](https://github.com/nullifyshot) commented on GitHub · 2026-11-12 17:28 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2822#issuecomment-6830933777) 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 `assets` fails) are fair; I'll keep them for a small follow-up if the nightly test flakes again rather than open one for logs alone.

github-sync referenced this pull request

2026-11-12 19:00:54 +00:00

test(vk): execute startup pool placement and fallback checks #2877

github-sync referenced this pull request

2026-11-12 19:02:16 +00:00

fix(release): wait before the final checksum validation #2779

Rows
Columns

fix(release): a SHA256SUMS replaced between its listing and its read is read again (flaky nightly test) #2822

Cause

Change

Tested