MagicBeanDev/Hookshot

Archived

Watch

Fork 0

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

Open

github-sync wants to merge 1 commit from github/pr/2779 into main

github-sync commented

2026-11-12 12:37:09 +00:00

Owner

@Nitnek opened this pull request on GitHub · 2026-11-12 12:35 UTC · original

Release jobs can exhaust the checksum-replacement budget without checking whether their last upload has become visible. This adds one delayed, read-only final validation rather than another replacement.

What changes

Preserve the existing loop with at most five logical replacement calls.
If it has not converged, wait five seconds after the last replacement, recompute checksums from a fresh asset listing and compare the exact remote SHA256SUMS bytes once more.
Do not perform a sixth replacement. Existing internal upload retries are unchanged: five logical replacements can still involve up to fifteen checksum POST attempts.
Keep persistent inconsistency, failed final listings and failed output copies as failures.
Add a deterministic regression that makes visibility advance after waits, not after a count of reads, and checks the five-replacement/five-wait bound.

Fresh local validation

Base: 51da0464b4cf0c7f56b81ebd6bb309325835e175 (current main at qualification).
Head: 90c63184c52fb1780c578390e8ba8c343f4f0b5c.
Linux, Python 3.14.9, isolated -I -B invocations; no real release API calls.

bash -n scripts/release_files.sh: exit 0.
New temporal regression on that exact base's production script, with only the candidate test fixture overlaid: RED, 1 test, exit 1, expected SHA256SUMS still differs from the files after 5 uploads.
Same regression on the candidate: GREEN, 1 test, exit 0.
python -I -B tests/test_release_scripts.py -v: the first candidate run failed only in the existing concurrent-nightly test; the unchanged follow-up passed 34/34, exit 0.
python -I -B tests/run_all.py --strict --jobs 1 release_scripts: the first run failed on the same concurrent-nightly test; the unchanged follow-up passed 1/1 checks, no skips, exit 0.
Production and test files are byte-identical to the independently reviewed snapshot, transplanted without logical adjustment. Only scripts/release_files.sh and tests/test_release_scripts.py differ from the base.

Existing intermittent failure, kept visible

This is not an all-green history. In this fresh qualification, the original, clean base's release suite failed 1 of 33 tests in test_nightly_two_runs_at_once_end_at_the_newer_commit. The candidate's first direct suite failed 1 of 34 tests in the same test; its first targeted runner also failed there (both ordering subcases), with the same checksum-exhaustion assertion and HTTP 404 diagnostics.

An isolated replay on the exact base production with the byte-identical candidate fixture reproduced that assertion immediately. The other exercised release scripts were also byte-identical. A further interleaved baseline replay passed, then the unchanged candidate runner passed. Complete stdout/stderr, native exits and commit/interpreter/file identities are retained for every attempt. Subprocesses mentioned by ResourceWarnings were checked afterwards; no active fixture child remained. This patch does not claim to solve the broader concurrent-nightly race.

Prior review and limits

The prior independent review was on base 467805032c70d21cb1e5e633faf7eb556b02935f; both changed files on current main were unchanged from that base. Its separate, uncommitted diagnostic probes covered twelve synthetic scenarios, including persistent invisible/divergent checksums, final-listing and checksum-read errors, asset changes during the final wait, nested retries, output-copy failure and fast paths. Those are historical bounded review evidence, not twelve new tests run on current main or added to this PR.

Release behavior is exercised with stand-in gh and a virtual sleep clock. No GitHub release was created, uploaded or modified, and actual GitHub propagation timing is not established.
No global repository-suite pass, Windows build or gameplay validation is claimed. This remains a draft; published-head CI has not been assumed green.
See #2626: its draft-tag preservation and newly-created-release discovery changes are distinct, but it touches both files changed here, including the same finish function. Coordination is still needed; Discord ownership/claims have not been verified. No part of #2626 is incorporated here.
No issue is auto-closed.

Assisted-by: GPT-5.6-terra and GPT-6.1-sol
Co-Authored-By: GPT-5.6-terra [email protected]
Co-Authored-By: GPT-6.1-sol [email protected]

> [@Nitnek](https://github.com/Nitnek) opened this pull request on GitHub · 2026-11-12 12:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2779) Release jobs can exhaust the checksum-replacement budget without checking whether their last upload has become visible. This adds one delayed, read-only final validation rather than another replacement. ## What changes - Preserve the existing loop with **at most five logical replacement calls**. - If it has not converged, wait five seconds after the last replacement, recompute checksums from a fresh asset listing and compare the exact remote `SHA256SUMS` bytes once more. - Do not perform a sixth replacement. Existing internal upload retries are unchanged: five logical replacements can still involve up to fifteen checksum POST attempts. - Keep persistent inconsistency, failed final listings and failed output copies as failures. - Add a deterministic regression that makes visibility advance after waits, not after a count of reads, and checks the five-replacement/five-wait bound. ## Fresh local validation Base: `51da0464b4cf0c7f56b81ebd6bb309325835e175` (current `main` at qualification). Head: `90c63184c52fb1780c578390e8ba8c343f4f0b5c`. Linux, Python 3.14.9, isolated `-I -B` invocations; no real release API calls. - `bash -n scripts/release_files.sh`: exit 0. - New temporal regression on that exact base's production script, with only the candidate test fixture overlaid: **RED**, 1 test, exit 1, expected `SHA256SUMS still differs from the files after 5 uploads`. - Same regression on the candidate: **GREEN**, 1 test, exit 0. - `python -I -B tests/test_release_scripts.py -v`: the first candidate run failed only in the existing concurrent-nightly test; the unchanged follow-up passed **34/34**, exit 0. - `python -I -B tests/run_all.py --strict --jobs 1 release_scripts`: the first run failed on the same concurrent-nightly test; the unchanged follow-up passed **1/1 checks**, no skips, exit 0. - Production and test files are byte-identical to the independently reviewed snapshot, transplanted without logical adjustment. Only `scripts/release_files.sh` and `tests/test_release_scripts.py` differ from the base. ### Existing intermittent failure, kept visible This is not an all-green history. In this fresh qualification, the original, clean base's release suite failed **1 of 33 tests** in `test_nightly_two_runs_at_once_end_at_the_newer_commit`. The candidate's first direct suite failed **1 of 34 tests** in the same test; its first targeted runner also failed there (both ordering subcases), with the same checksum-exhaustion assertion and HTTP 404 diagnostics. An isolated replay on the **exact base production** with the **byte-identical candidate fixture** reproduced that assertion immediately. The other exercised release scripts were also byte-identical. A further interleaved baseline replay passed, then the unchanged candidate runner passed. Complete stdout/stderr, native exits and commit/interpreter/file identities are retained for every attempt. Subprocesses mentioned by ResourceWarnings were checked afterwards; no active fixture child remained. This patch does **not** claim to solve the broader concurrent-nightly race. ## Prior review and limits The prior independent review was on base `467805032c70d21cb1e5e633faf7eb556b02935f`; both changed files on current main were unchanged from that base. Its separate, uncommitted diagnostic probes covered twelve synthetic scenarios, including persistent invisible/divergent checksums, final-listing and checksum-read errors, asset changes during the final wait, nested retries, output-copy failure and fast paths. Those are **historical bounded review evidence**, not twelve new tests run on current main or added to this PR. - Release behavior is exercised with stand-in `gh` and a virtual sleep clock. No GitHub release was created, uploaded or modified, and actual GitHub propagation timing is not established. - No global repository-suite pass, Windows build or gameplay validation is claimed. This remains a draft; published-head CI has not been assumed green. - See #2626: its draft-tag preservation and newly-created-release discovery changes are distinct, but it touches both files changed here, including the same `finish` function. Coordination is still needed; Discord ownership/claims have not been verified. No part of #2626 is incorporated here. - No issue is auto-closed. Assisted-by: GPT-5.6-terra and GPT-6.1-sol Co-Authored-By: GPT-5.6-terra <[email protected]> Co-Authored-By: GPT-6.1-sol <[email protected]>

github-sync added 1 commit

2026-11-12 12:37:09 +00:00

fix(release): wait before the final checksum validation 90c63184c5

After the final replacement, allow visibility to advance before
recomputing assets and comparing SHA256SUMS one last time. Keep
the five logical replacement limit and existing transport retries.

Assisted-by: GPT-5.6-terra and GPT-6.1-sol
Co-Authored-By: GPT-5.6-terra <[email protected]>
Co-Authored-By: GPT-6.1-sol <[email protected]>

github-sync referenced this pull request

2026-11-12 15:49:09 +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(release): a SHA256SUMS replaced between its listing and its read is read again (flaky nightly test) (#2822)

github-sync referenced this pull request

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

ci: label-gated hardware tests on testers' self-hosted runners, through the owner's sandbox #2807

github-sync referenced this pull request

2026-11-12 17:59:34 +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

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

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

github-sync commented

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

Author

Owner

@Nitnek commented on GitHub · 2026-11-12 18:15 UTC · original

Compatibility follow-up against main 4cf785a0b143bb34026506c9a3fe4f4bb93c797d and this Draft's unchanged head 90c63184c52fb1780c578390e8ba8c343f4f0b5c. I checked the already-merged #2822 instead of implementing a competing fix.

The two changes address different observations:

#2822 re-lists and re-reads SHA256SUMS when its asset ID was replaced between listing and reading.
#2779 delays and performs one final read after the fifth logical replacement, without a sixth replacement.

I ran both existing deterministic public regression tests under Linux, with the fixture gh storing only SYNTHETIC state. The fixture assertions and production inputs were kept unchanged except for the explicitly described local composition:

Production source	Replaced-asset-ID test from current main	Last-upload-visibility test from #2779
main `4cf785a0`	PASS / exit 0	expected FAIL / exit 1
original #2779 `90c63184`	expected FAIL / exit 1 (it lacks #2822)	already covered by this PR's original qualification
private composition: current main + this PR's exact final-read block	PASS / exit 0	PASS / exit 0

Those are five fresh test invocations, including two deliberate negative controls—not “five green tests”. The combined temporal test still asserts five logical checksum uploads and five simulated waits. The replaced-ID test asserts that its two replacements are exhausted and check/finish do not upload another checksum unnecessarily.

The composition retained main's uploaded() byte-for-byte and added only the existing #2779 final-read block to finish(). Source hashes were verified before/after, Bash syntax checks passed, and complete script outputs/mock call-state records were retained before fixture cleanup. No branch/head/rebase was changed publicly.

This is bounded compatibility evidence using two different fixture versions, not a whole-suite run, a real GitHub release/API test, hosted CI, or a measured flake rate for the concurrent-nightly test. The Draft status remains appropriate. A future integration should retain #2822 and its deterministic regression, rather than treating these changes as alternatives.

AI-assisted analysis: Hermes Agent (OpenAI GPT-6.1-sol).

> [@Nitnek](https://github.com/Nitnek) commented on GitHub · 2026-11-12 18:15 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2779#issuecomment-6832042002) Compatibility follow-up against **main `4cf785a0b143bb34026506c9a3fe4f4bb93c797d`** and this Draft's unchanged head **`90c63184c52fb1780c578390e8ba8c343f4f0b5c`**. I checked the already-merged #2822 instead of implementing a competing fix. The two changes address different observations: - #2822 re-lists and re-reads `SHA256SUMS` when its asset ID was replaced between listing and reading. - #2779 delays and performs one final read after the fifth logical replacement, without a sixth replacement. I ran both existing deterministic public regression tests under Linux, with the fixture `gh` storing only SYNTHETIC state. The fixture assertions and production inputs were kept unchanged except for the explicitly described local composition: | Production source | Replaced-asset-ID test from current main | Last-upload-visibility test from #2779 | |---|---|---| | main `4cf785a0` | PASS / exit 0 | expected FAIL / exit 1 | | original #2779 `90c63184` | expected FAIL / exit 1 (it lacks #2822) | already covered by this PR's original qualification | | private composition: current main + this PR's exact final-read block | PASS / exit 0 | PASS / exit 0 | Those are **five fresh test invocations**, including two deliberate negative controls—not “five green tests”. The combined temporal test still asserts **five logical checksum uploads and five simulated waits**. The replaced-ID test asserts that its two replacements are exhausted and `check`/`finish` do not upload another checksum unnecessarily. The composition retained main's `uploaded()` byte-for-byte and added only the existing #2779 final-read block to `finish()`. Source hashes were verified before/after, Bash syntax checks passed, and complete script outputs/mock call-state records were retained before fixture cleanup. No branch/head/rebase was changed publicly. This is bounded compatibility evidence using two different fixture versions, **not** a whole-suite run, a real GitHub release/API test, hosted CI, or a measured flake rate for the concurrent-nightly test. The Draft status remains appropriate. A future integration should retain #2822 and its deterministic regression, rather than treating these changes as alternatives. AI-assisted analysis: Hermes Agent (OpenAI GPT-6.1-sol).

Commenting is not possible because the repository is archived.

No reviewers