daniel/gitdan-actions
1
0
Fork 0
Code Issues 5 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ci): only ever push a commit this run actually gated; fix vacuous scenario-9 guard

Browse Source 
## v1 could advance onto an ungated commit

`needs: selftest` gates this run's own commit, but the push targeted
origin/main's freshly-fetched tip with nothing comparing the two.
Trace: M1 merges green; M2 merges while M1's selftest is still
running; M1's release-tag job fetches tip = M2 and pushes v1 = M2,
whose own selftest may be queued, running, or red. If M2 is red, its
own job is skipped, so v1 sits on a red commit across every consuming
project until the next green merge -- with nothing red pointing at
the release itself. ci.yaml:107-109 and README.md:640-641 both
asserted this couldn't happen; ea48c03's own diff established the
precondition (tip "can be minutes stale... behind its own selftest
job") without closing it.

Fix: skip the push unless origin/main's tip IS this run's own
github.sha, checked before the existing v1-monotonicity check
(dc1e631) rather than replacing it -- the two compose (tip-mismatch
first, since it's the coarser reason to defer; ancestor-check second,
for a duplicate run whose own commit is still current). Reverted the
push target from the fetched tip back to `${{ github.sha }}`, now
that the guard makes them provably equal whenever the push fires.

## Convergence trace: does the newest commit's job still run?

Read gitea's source further at the pinned v1.27.2 tag:
PrepareToStartJobWithConcurrency (services/actions/clear_tasks.go)
calls CancelPreviousJobsByJobConcurrency on every job entering the
group, unconditionally cancelling whatever was previously
Waiting/Blocked there -- so at most one job sits queued in the group
at a time; each new arrival supersedes it. Because job-level
concurrency is only evaluated once `needs: selftest` is satisfied
(job_emitter.go re-evaluates readiness there), "arrival order" tracks
each commit's own selftest-completion time, not raw merge order -- an
older commit with a slower selftest can enter the group after a
newer one and cancel its queued slot.

That cancelled job is gone for good; it will never push. But the
commit that's genuinely current at the moment merges stop arriving is
always the one still queued when the running job finishes, because
every subsequent arrival (from every subsequent merge, not just the
"newest" one at any single instant) keeps re-superseding the queue.
So v1 always eventually catches up -- "one merge later" in the common
case, "at the next merge, whenever that happens" in the adversarial
case where a stale survivor runs, finds itself no longer current,
defers, and nothing is left queued. It cannot get stuck forever short
of the repository never receiving another merge, because every future
push re-attempts the same check against whatever's current by then.
Stated this plainly in the comment and README rather than repeating
the false "never" guarantee in softer words.

Re-derived the truth table against the new guard in a scratch
origin+clone, six cases: own commit == tip, no v1 (push); v1 already
== own commit (skip, duplicate run); tip moved past own gated commit
because a newer merge landed (skip, defers); the newer commit's own
run once nothing further has landed (push); v1 already ahead of a
now-stale gated commit (skip, tip-mismatch catches it first); tip ==
own commit but v1 independently ahead via a local-only descendant,
isolating the second (ancestor) check on its own (skip). All six
resolved as intended.

## Scenario 9's pass-3 guard was vacuous

scripts/prune-cache-selftest.sh:230 grepped 'self-clear' against
$scratch/log. summary_line() (cache-lib.sh) writes only to
$GITHUB_STEP_SUMMARY; the self-clear branch (prune-cache.sh:632)
writes 'self-clear' there and 'clearing own' to stdout (:631) --
'self-clear' never appears in $scratch/log at all, so the branch was
unreachable and the `ok` unconditional. Scenario 8 already greps the
right string against the right file (assert_log "clearing own" ...);
scenario 9 now does the same, staying on $scratch/log where it
already was -- the string was wrong, not the file.

Red-proved by capturing a real prune-cache.sh log where self-clear
genuinely fired (scenario 9's own fixture with MIN_FREE_PCT
temporarily raised to 100, in a scratch copy, reverted after) and
running both patterns against it: `grep -q 'self-clear'` -> no match
(the old check's vacuous pass, confirmed); `grep -q 'clearing own'`
-> match (the fix's correct fail). Matches the reviewer's own
measurement exactly. The real prune-cache-selftest.sh was untouched
during this experiment; only the grep string changed in the actual
commit.

bash scripts/selftest.sh: all 6 suites green (67 prune-cache
assertions, unchanged in count -- the fix corrects what scenario 9's
existing check compares, not what it asserts). shellcheck -x
--source-path=scripts scripts/*.sh: clean, as before not covering the
inline ci.yaml shell.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UkSjXXtU6JYcN2vPntWhfb
This commit is contained in:
claude
2026-09-22 14:01:28 -05:00
co-authored by Claude Sonnet 5
parent ea48c03aeb
commit 17d87b0647
3 changed files with 54 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.gitea/workflows/ci.yaml
+39 -35
View File
@@ -128,13 +128,12 @@ jobs:
# Gitea wakes exactly one Blocked job in the group and cancels the rest
# outright, with no ordering on which one it picks (no `ORDER BY` in the
# query behind CancelPreviousJobsByJobConcurrency,
# models/actions/run_job_list.go). What it buys is cheaper: every
# execution that does reach the push step targets origin/main's live tip
# (below), never its own trigger commit, so it makes no difference which
# one wins -- the survivor pushes where any of them would have, and a
# cancelled job costs nothing. This group's only job is to stop more than
# one job from pushing AT THE SAME TIME, which is wasted work, not a
# correctness risk on its own.
# models/actions/run_job_list.go). Every execution still only ever pushes
# its OWN gated commit (below), never another job's, so a job that gets
# cancelled here costs nothing beyond its own wasted run -- it was never
# going to push anyone else's commit either. This group's only job is to
# stop more than one job from pushing AT THE SAME TIME, which is wasted
# work, not a correctness risk on its own.
concurrency:
group: release-tag-v1
cancel-in-progress: false
@@ -155,37 +154,42 @@ jobs:
token: ${{ secrets.GITHUB_TOKEN }}
fetch-depth: 0
# This run's own trigger commit (${{ github.sha }}) is deliberately not
# what gets pushed: whichever job the concurrency group above lets
# through is the one that pushes, and that choice carries no relation
# to commit recency, so every execution has to converge on the SAME
# target regardless of which job it is. `origin/main`'s live tip,
# re-fetched here rather than trusted from the checkout above (which
# can be minutes stale by this point, behind its own selftest job), is
# that common target -- read fresh, every job that reaches this step
# resolves to the same commit whenever main hasn't moved between them,
# and to whatever's newest when it has.
# `needs: selftest` only gates THIS run's own commit -- it says nothing
# about whether `origin/main` has since moved on to a merge whose own
# selftest hasn't finished, is still queued, or is red. Two things
# follow, checked in this order:
#
# A live target doesn't make the push itself safe on its own: two jobs
# can still read main at genuinely different moments if it advances
# between their two fetches, so the one with the earlier reading must
# not overwrite the other's already-pushed, newer one. That's what the
# ancestor check below still guards -- not "this job's stale trigger
# commit" any more, but "this job's freshly-read tip, which another
# job's fresher read may have already superseded." `--is-ancestor`
# treats a commit as its own ancestor, so "already at" and "already
# ahead" are one case. A v1 that doesn't exist yet, or that shares no
# history with this tip, falls through to the push -- the guard is
# only ever a reason to skip, never a reason to fail.
- name: Determine origin/main's tip and whether v1 needs to move
# 1. If `origin/main`'s live tip (re-fetched here, not trusted from the
# checkout above, which can be minutes stale behind this job's own
# selftest) is no longer THIS run's own `github.sha`, some other
# merge has landed since. Pushing it would release a commit this run
# never gated -- so this run defers instead, unconditionally. The
# commit that IS the live tip has its own run, and that run's own
# guard is what releases it once ITS turn to push comes -- possibly
# only once merges pause for a moment, so v1 can land one merge
# later than the newest one in a busy stretch. It never lands on a
# commit that wasn't gated, and it always catches up once things go
# quiet, because every future merge re-attempts the same check
# against whatever is current by then.
# 2. Only once (1) confirms this IS the live tip does it matter whether
# v1 already covers it -- a duplicate run, or a manual push already
# having done this. `--is-ancestor` treats a commit as its own
# ancestor, so "already at" and "already ahead" are one case. A v1
# that doesn't exist yet, or shares no history with this commit,
# falls through to the push -- both checks are only ever a reason to
# skip, never a reason to fail.
- name: Determine whether this commit is still current and needs releasing
id: check
run: |
git fetch origin main
TIP=$(git rev-parse origin/main)
echo "tip=$TIP" >> "$GITHUB_OUTPUT"
if git rev-parse -q --verify refs/tags/v1 >/dev/null \
&& git merge-base --is-ancestor "$TIP" refs/tags/v1; then
echo "v1 already at or ahead of origin/main's tip ($TIP) -- nothing to do"
SHA="${{ github.sha }}"
if [ "$TIP" != "$SHA" ]; then
echo "origin/main's tip ($TIP) has moved past this run's own gated commit ($SHA) -- deferring to whichever run's own commit is now the live tip"
echo "skip=true" >> "$GITHUB_OUTPUT"
elif git rev-parse -q --verify refs/tags/v1 >/dev/null \
&& git merge-base --is-ancestor "$SHA" refs/tags/v1; then
echo "v1 already at or ahead of $SHA -- nothing to do"
echo "skip=true" >> "$GITHUB_OUTPUT"
else
echo "skip=false" >> "$GITHUB_OUTPUT"
@@ -194,8 +198,8 @@ jobs:
# Lightweight tag, matching what v1 already is (`git cat-file -t v1`
# reports `commit`, not `tag`) -- no identity needed to move it, only
# to push it.
- name: Force v1 to origin/main's tip
- name: Force v1 to this commit
if: steps.check.outputs.skip != 'true'
run: |
git tag -f v1 "${{ steps.check.outputs.tip }}"
git tag -f v1 "${{ github.sha }}"
git push --force origin v1
README.md
+14 -10
View File
@@ -636,22 +636,26 @@ entry, another permission — is a `v2`, not a `v1` move. Everything else moves
**Moving the tag is automatic, gated on the same build that gates a PR.** A
`release-tag` job in `.gitea/workflows/ci.yaml` runs on every push to `main`,
`needs: selftest`, and force-moves `v1` to `origin/main`'s live tip once
`needs: selftest`, and force-moves `v1` to *that run's own commit* once
selftest succeeds — a broken build never reaches it, so `v1` can't advance
onto one. It pushes with the run's built-in `GITHUB_TOKEN`; if that token
turns out not to have write access, the push step fails and the job goes red
in the Actions UI. That's a loud failure, not the silent one this replaced:
`v1` stays put, and nobody has to notice on their own that it lagged.
Two merges landing close together can start two `release-tag` jobs at once; a
job-level `concurrency` group lets only one push at a time, and every job
targets `origin/main`'s current tip rather than its own trigger commit, so it
makes no difference which one the group lets through. Before pushing, the job
also checks whether `v1` already points at that tip or a descendant of it —
covering the case where two jobs read the tip at genuinely different moments
— and skips as a normal, successful outcome rather than pushing backward. A
run whose log says "nothing to do" did its job correctly; it just found
nothing to move.
Before pushing, the job checks two things and pushes only if both hold: that
`origin/main`'s live tip is still this run's own commit (not some later merge
that landed while this job was queued behind its own selftest), and that `v1`
doesn't already point at that commit or a descendant of it. Either check can
skip the push, as a normal, successful outcome — a run whose log says
"nothing to do" did its job correctly. The first check is what a job-level
`concurrency` group alone can't guarantee: Gitea's wake-one/cancel-rest
handling applies no ordering by commit recency, so an older merge's job can
be the one that survives to run — that job now defers instead of releasing a
commit it never gated. The newer merge's own job releases it once its own
turn comes, which can land `v1` one merge behind the newest during a busy
stretch; it always catches up once merges pause, because every later run
re-checks against whatever is current by then.
This used to be a manual step, treated as a deliberate release decision taken
once, knowingly, after the merge — in practice it was still forgotten
scripts/prune-cache-selftest.sh
+1 -1
View File
@@ -227,7 +227,7 @@ PATH="$scratch/bin9:$outer_path" \
assert_kept "$root/target-$OWN" "this run's own cache directory survives a genuinely pressured sibling pass"
assert_kept "$root/target-$OWN/blob" "and its contents survive — not a recreated empty directory"
assert_gone "$root/target-$LIVE" "the sibling is evicted instead, to make the same room"
if grep -q 'self-clear' "$scratch/log"; then
if grep -q 'clearing own' "$scratch/log"; then
fail "own cache was cleared by pass 3, not genuinely spared by pass 2 — this scenario proves nothing"
fi
ok "the requirement was met by pass 2 alone; pass 3 never ran"