Conversation
vladimirrott
left a comment
There was a problem hiding this comment.
Thanks for this, and for the honesty in the body about ShellCheck and the crashed local loop. Two blocking items below, both about the test rather than the fix. The polling helper itself is right, and I want to say what I verified before I say what is missing.
Head reviewed: b4b15427. maintainer screen 463 returns DO NOT EXECUTE, so everything ran in rootless podman with no network against a detached worktree.
What holds up
Your test passes as written:
$ out="$(podman run --rm --network=none -v /tmp/r21p463:/repo:ro -w /repo docker.io/library/bash:5 bash tests/release/crates-index-poll.test.sh 2>&1)"; rc=$?; echo "rc=$rc"; printf '%s\n' "$out" | grep -v level=warning | tail -2
rc=0
sysknife-daemon 0.15.0 is available on crates.io
crates index polling contract passed.
ShellCheck, the run you could not do locally, is clean over the whole sweep the scripts-lint job runs:
$ cd /tmp/r21p463 && out="$(find tests/e2e tests/release scripts assets/demo -type f -name '*.sh' -print0 | xargs -0 shellcheck --severity=warning 2>&1)"; rc=$?; echo "shellcheck rc=$rc"; printf '%s\n' "$out" | head -40
shellcheck rc=0
That is ShellCheck 0.10.0 here against 0.11.0 in CI, so treat it as strong rather than conclusive.
The status=$? in the else branch is the line I expected to find wrong and it is correct. Bash keeps the condition's status there, which I confirmed rather than assumed:
$ cat /tmp/semtest.sh
#!/usr/bin/env bash
set -e
f() { return "${1:-2}"; }
for v in 0 1 2; do
if f "$v"; then
echo "v=$v: branch=then"
else
status=$?
echo "v=$v: branch=else status=$status"
fi
done
$ bash /tmp/semtest.sh; echo "rc=$?"
v=0: branch=then
v=1: branch=else status=1
v=2: branch=else status=2
rc=0
So a 404 from the skip check reaches ((status != 1)) as 1 and falls through to publish, and a network failure reaches it as 2 and exits. That last part is a real improvement the PR body undersells: the old curl --fail collapsed "crates.io is unreachable" and "this version does not exist" into the same non-zero, so a blip during the skip check sent the job into a publish it had no basis for. Yours names the failure and stops.
Blocking 1: the test reaches no CI job
tests/release/*.test.sh scripts run in CI by name, one step each in the docs-and-hygiene job. Yours is not in that list, and on your branch it is the only release test in that position:
$ cd /tmp/r21p463 && for f in tests/release/*.test.sh; do b=$(basename "$f"); if ! grep -rqF "$b" .github/workflows/; then echo "NOT IN WORKFLOWS: $b"; fi; done
NOT IN WORKFLOWS: crates-index-poll.test.sh
The same loop on main prints nothing, so this would be the first one. scripts/ci-local.sh discovers every release test by glob and would run it on a developer's machine, and tests/release/ci-local.test.sh asserts CI ⊆ local but not local ⊆ CI, so nothing goes red today. #377 is open and adds exactly that missing direction; whichever of the two lands second will turn this red, so it is worth fixing now either way.
Add a step next to the others in .github/workflows/ci.yml:
- name: Check the release waits for crates.io to index each crate
run: bash tests/release/crates-index-poll.test.shBlocking 2: the test stays green with the fix removed
This is the one I care about. Your test asserts the helper's behaviour and asserts that sleep 30 is gone, and nothing asserts the workflow calls the helper. Delete the call and everything still passes:
$ cd /tmp/r21p463 && cp /tmp/rel463.orig .github/workflows/release.yml && sed -i '/wait_for_crate_version "\$crate" "\$VERSION"/d' .github/workflows/release.yml
$ out="$(podman run --rm --network=none -v /tmp/r21p463:/repo:ro -w /repo docker.io/library/bash:5 bash tests/release/crates-index-poll.test.sh 2>&1)"; rc=$?; echo "MUTATION-2 (workflow never calls the poller) rc=$rc"; printf '%s\n' "$out" | grep -v 'level=warning' | tail -4
MUTATION-2 (workflow never calls the poller) rc=0
sysknife-daemon 0.15.0 is available on crates.io
crates index polling contract passed.
That tree publishes six crates back to back with no wait of any kind, which is worse than the sleep 30 you replaced, and the contract test calls it a pass. The sleep 30 assertion is half an anchor: it catches the old mechanism coming back and misses the new one going away.
The restoring assertion is short. Something in the shape of:
grep -Fq 'wait_for_crate_version "$crate" "$VERSION"' "$workflow" || {
printf 'FAIL: release workflow does not wait for the index after publishing\n' >&2
exit 1
}
grep -Fq 'source scripts/crates-index-poll.sh' "$workflow" || {
printf 'FAIL: release workflow does not load the polling helper\n' >&2
exit 1
}Please run the same mutation yourself once it is in and paste the red output, the way you pasted the green.
For completeness, the two mutations that do bite, so you know which parts are already anchored. Putting sleep 30 back:
$ sed -i 's| wait_for_crate_version "$crate" "$VERSION"| sleep 30|' .github/workflows/release.yml
$ out="$(podman run --rm --network=none -v /tmp/r21p463:/repo:ro -w /repo docker.io/library/bash:5 bash tests/release/crates-index-poll.test.sh 2>&1)"; rc=$?; echo "MUTATION-1 rc=$rc"; printf '%s\n' "$out" | grep -v 'level=warning' | tail -4
MUTATION-1 rc=1
sysknife-daemon 0.15.0 is available on crates.io
FAIL: release workflow still contains fixed sleep 30
And neutering the timeout guard inside the helper:
$ sed -i 's|^ if ((elapsed >= timeout)); then| if false; then|' scripts/crates-index-poll.sh
$ out="$(timeout 120 podman run --rm --network=none -v /tmp/r21p463:/repo:ro -w /repo docker.io/library/bash:5 bash tests/release/crates-index-poll.test.sh 2>&1)"; rc=$?; echo "MUTATION-3 (timeout guard removed) rc=$rc"
MUTATION-3 (timeout guard removed) rc=124
Two optional notes
rc=124 above is my timeout 120 firing, not your test failing. With CRATES_IO_SLEEP=true the perpetual-404 case spins with nothing to stop it, so a broken timeout arithmetic hangs the job until GitHub's six-hour limit instead of going red in two seconds. Bounding the loop by iteration count as well as elapsed seconds, or wrapping the perpetual-404 case in timeout, turns that into a fast failure. Optional.
tests/release/crates-index-poll.test.sh has no newline at end of file. No gate objects (tracked-eol.test.sh checks CRLF and mixed endings only), so this is cosmetic.
Your checks, and why they are sitting there
The board shows no checks because four runs are parked at action_required:
$ gh api repos/lacs-project/sysknife/actions/runs --jq '.workflow_runs[] | select(.head_sha|startswith("b4b1542")) | "\(.id)\t\(.name)\t\(.status)\t\(.conclusion)"'
35508302296 ci completed action_required
35508302271 e2e completed action_required
35508302270 CodeQL completed action_required
35508302269 secret-scan completed action_required
I approve first-time contributors' runs as a matter of course, and I am not approving these, because this PR edits .github/workflows/release.yml. Approving a fork run that touches workflow files is the one case where that click can hand a fork the repository's token, so a human reads the diff first. Nothing about that is a judgement on you or on the change; it is the rule for the file, not for the author. I will read it and release the runs.
Scope
You kept this to the polling path and left the --no-verify half of #420 alone, which is the right call. That second bullet is a separate decision about how we publish, and it should not ride along on a fix that is already testable on its own.
Once the two blocking items are in, re-request review and I will run the same mutations against the new head.
One more thing. You are comfortable in release plumbing and you wrote a curl stub with a request counter rather than trusting a sleep, which is the instinct this repository is built around. If you run Ubuntu anywhere, SysKnife's read-only side is worth ten minutes: sysknife plan "why is my disk filling up" shows you the proposed actions without executing anything, and the approval step is where the whole design lives. Take it or leave it, and it has nothing to do with this PR landing.
Summary
Replace the fixed post-publish sleep with bounded crates.io polling so dependency crates are not published before their newly uploaded dependencies are visible in the registry.
The polling helper distinguishes:
The release workflow now waits for each newly published crate version to become available before continuing to the next dependent crate.
Related Issue
Fixes #420
Validation
Local validation completed:
bash tests/release/crates-index-poll.test.shbash -n scripts/crates-index-poll.shbash -n tests/release/crates-index-poll.test.shgit diff --checkgit diff --cached --checkShellCheck was not run locally because installing it was blocked by the unaccepted Xcode licence. A broader local release-test loop was attempted but the local session crashed, so it is not reported as passing.
Notes for Reviewers
The polling timeout defaults to 300 seconds with a 15-second interval. Tests cover delayed indexing, perpetual 404 timeout, network failure, request counting, the default timeout bound, and removal of the fixed
sleep 30.This PR keeps the change focused on the polling path described in #420.