fix(763): page both /statuses/{sha} reads to a validated terminator #868
@@ -723,6 +723,115 @@ jobs:
|
||||
done
|
||||
return 0
|
||||
}
|
||||
# PAGE THE PER-POST STATUS HISTORY TO A VALIDATED TERMINATOR (ersatztv#763).
|
||||
#
|
||||
# Both callers below want the WHOLE history, and a single `?limit=100` read cannot give it:
|
||||
# `limit` clamps to the server-wide `MAX_RESPONSE_ITEMS`, measured at 50.
|
||||
#
|
||||
# BE PRECISE ABOUT WHAT THAT COST, because the issue and an earlier version of this comment
|
||||
# both overstated it. Under the server default (`created_unix DESC`) page 1 holds the NEWEST
|
||||
# rows, and ids are monotonic with `created_at` (measured). So page 1 already carried the
|
||||
# true maximum id, AND every row newer than the mark — which is the only thing the
|
||||
# post-write check selects on. A single-page read therefore missed a raced verdict only if
|
||||
# more than 50 rows were created INSIDE the write window, not merely "on a head with more
|
||||
# than 50 rows". What actually removed #761's stall is deleting #751's page-2 "assume raced"
|
||||
# probe, further down; this walk is not what fixed it.
|
||||
#
|
||||
# THE WALK IS STILL WORTH HAVING, for a different reason than the issue gives: it removes
|
||||
# the gate's one fail-toward-SUCCESS path from depending on an UNDOCUMENTED ordering that
|
||||
# the server honours only coarsely. Page 1 came back `114,112,113,111,110` — newest-first,
|
||||
# but not strictly, and nothing in the API contract promises even that. Selecting by id over
|
||||
# a list known to be complete does not care how the server ordered it.
|
||||
#
|
||||
# CORRECTNESS DOES NOT DEPEND ON THE NUMBER 50. The walk stops at a validated EMPTY page, so
|
||||
# whatever page size the server actually applies is paged correctly; `limit=50` only asks
|
||||
# for what this instance will really give. The distinction matters because #751 shipped a
|
||||
# completeness check written against a hardcoded `limit=100` that was DEAD CODE precisely
|
||||
# because the cap is 50. A guard against a hardcoded cap is the thing to avoid; a request
|
||||
# that names one, whose correctness does not turn on it, is not the same mistake.
|
||||
#
|
||||
# THE TERMINATOR IS MEASURED PER ENDPOINT — this server is not consistent between them and
|
||||
# guessing has been wrong twice. Measured at 1.27.1 on 2026-08-28 against PR #761's head
|
||||
# (114 rows): pages 1 and 2 return 50, page 3 returns 14, page 4 is the two bytes `[]`.
|
||||
# `/issues/{n}/timeline` returns bare `null` past the end and `/commits/{sha}/status`
|
||||
# returns `{"statuses": null}` — three distinct empty shapes on one instance, which is why
|
||||
# `count_retargets` above and this walk deliberately do NOT share a terminator.
|
||||
#
|
||||
# A SHORT PAGE IS NOT THE END. Page 3 above carried 14 rows and was still followed by a real
|
||||
# page 4, and more fundamentally a short page is indistinguishable from a truncated
|
||||
# response. Only an empty page is evidence, which costs exactly one extra round-trip.
|
||||
#
|
||||
# AN EMPTY PAGE 1 IS LEGITIMATE HERE, unlike `count_retargets`. A head nothing has posted to
|
||||
# yet genuinely has no statuses (and a bogus sha returns `[]` too — measured), so an empty
|
||||
# first page is an ordinary answer rather than the anomaly it is on a PR timeline. It yields
|
||||
# `ph_ok=yes` over zero rows. Copying the timeline walk's "one real page required" rule here
|
||||
# would withhold the mark on every first run.
|
||||
#
|
||||
# RETRY ONCE, THEN REFUSE. `ph_ok=no` is consumed below as "could not establish", which on
|
||||
# the post-write path repairs to the STICKY sentinel — so one transient blip would cost that
|
||||
# head its exemption until a human clears it by hand. A single retry absorbs the blip;
|
||||
# anything persistent still refuses, because on that path uncertainty must never resolve to
|
||||
# green. The retry covers a non-array body too (a 502 HTML page reads exactly like one).
|
||||
page_statuses() {
|
||||
ph_rows='[]'
|
||||
ph_ok=no
|
||||
local page=1 raw kind n acc='[]' try
|
||||
# EVERY exit publishes what WAS read, not just the complete ones. `ph_ok` alone says
|
||||
# whether the list is whole; a caller that can still use a partial list must be able to
|
||||
# reach it, and the high-water mark below is exactly such a caller — see the fail-open it
|
||||
# otherwise creates (ersatztv#763, round 2).
|
||||
while [ "$page" -le 20 ]; do
|
||||
raw=""
|
||||
kind=""
|
||||
# `if`/`then`, never `cmd && var=yes`: under `set -e` a bare `A && B` whose test fails is
|
||||
# itself a failing command, and as the last command of a loop body it would kill the job.
|
||||
for try in 1 2; do
|
||||
raw=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=50&page=${page}") || raw=""
|
||||
if [ -n "${raw//[[:space:]]/}" ]; then
|
||||
# Read the type as a VALUE, not through `jq -e`, for the reason recorded at
|
||||
# `count_retargets`: `jq -e` reports the truthiness of its last output, so it cannot
|
||||
# separate "the body is null" from "the predicate is false".
|
||||
kind=$(printf '%s' "$raw" | jq -r 'type' 2>/dev/null) || kind=""
|
||||
fi
|
||||
if [ "$kind" = array ] || [ "$kind" = null ]; then break; fi
|
||||
# A SECOND IMMEDIATE REQUEST BARELY COUNTS AS A RETRY. One second does not survive a
|
||||
# Gitea restart either, and saying so is the point: this absorbs a momentary blip, not
|
||||
# an outage. An outage still lands on `ph_ok=no`, which is the safe direction.
|
||||
# Only BETWEEN attempts — sleeping after the last one delays a failure nobody is
|
||||
# waiting on, once per walk and so twice per job run.
|
||||
if [ "$try" -eq 1 ]; then sleep 1; fi
|
||||
done
|
||||
# `null` COUNTS AS AN EMPTY PAGE, though this endpoint returns `[]` today. An
|
||||
# array-only gate is the exact shape of #751: `count_retargets` had one, the timeline
|
||||
# really did return `null` past the end, so the walk never reached a validated empty
|
||||
# page and the fence withheld EVERY exemption. Re-adopting that narrowing on a second
|
||||
# endpoint would be worse, because this walk's failure is the STICKY sentinel — every
|
||||
# exempt PR would need a human verdict, per head. It cannot misread `[]` either.
|
||||
#
|
||||
# It is NOT free, though, and the cost lands on one caller: an empty page 1 accepted as
|
||||
# "complete, zero rows" is correct BEFORE the write and impossible AFTER it. The
|
||||
# post-write caller therefore rejects an empty result itself, below — this walk stays
|
||||
# caller-agnostic because the pre-write caller genuinely needs the empty answer.
|
||||
case "$kind" in
|
||||
null) ph_ok=yes; ph_rows=$acc; return 0 ;;
|
||||
array) ;;
|
||||
*) ph_rows=$acc; return 0 ;;
|
||||
esac
|
||||
n=$(printf '%s' "$raw" | jq -r 'length' 2>/dev/null) || n=""
|
||||
case "$n" in ''|*[!0-9]*) ph_rows=$acc; return 0 ;; esac
|
||||
if [ "$n" -eq 0 ]; then ph_ok=yes; ph_rows=$acc; return 0; fi
|
||||
acc=$(printf '%s\n%s' "$acc" "$raw" | jq -s -c 'add' 2>/dev/null) || acc=""
|
||||
if [ -z "$acc" ]; then return 0; fi
|
||||
ph_rows=$acc
|
||||
page=$(( page + 1 ))
|
||||
done
|
||||
# THE PAGE CAP IS NOT EXHAUSTION. Falling out of the loop leaves `ph_ok=no`, so an
|
||||
# over-long history is "could not establish" rather than a silent partial list — the same
|
||||
# treatment as an unreadable page, and for the same reason. The bound is 950 rows, not
|
||||
# 1000: the 20th request has to be the EMPTY terminator for the walk to be validated, so
|
||||
# only 19 full pages can be confirmed.
|
||||
return 0
|
||||
}
|
||||
|
||||
ex_repair=no
|
||||
count_retargets
|
||||
@@ -1005,9 +1114,45 @@ jobs:
|
||||
# `.[].id` hard-errors under `set -e` if the array ever holds a non-object, which would kill
|
||||
# the job before it posts and strand an ordinary PR with no status at all.
|
||||
max_id_before=-1
|
||||
hist_before=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100") || hist_before=""
|
||||
if [ -n "${hist_before//[[:space:]]/}" ] && printf '%s' "$hist_before" | jq -e 'type == "array"' >/dev/null 2>&1; then
|
||||
mark=$(printf '%s' "$hist_before" | jq -r '[.[] | .id? // 0] | max // 0' 2>/dev/null || true)
|
||||
# PAGED (ersatztv#763). This was one `?limit=100` read, so on a head carrying more rows than
|
||||
# the clamp the "maximum" was only the maximum of page 1. That direction is safe — a mark
|
||||
# that is too low makes pre-existing rows look newer and repairs an exemption that nobody
|
||||
# raced — but it is a stall with no diagnosis attached, and it is the same read the
|
||||
# post-write check depends on being complete.
|
||||
page_statuses
|
||||
# A PARTIAL LIST STILL YIELDS A USABLE MARK, and refusing one is a FAIL-OPEN (ersatztv#763).
|
||||
# This depends on the DESC default above: the newest row — the one carrying the maximum id,
|
||||
# ids being monotonic with `created_at` (measured) — is on page 1, so a walk that fails
|
||||
# later still saw it. Under an ASCENDING sort this reasoning inverts and the fallback
|
||||
# becomes a spurious-repair engine; see the withdrawal note at `page_statuses`.
|
||||
# Skipping the mark skips the post-write race check entirely, so a human rejection landing
|
||||
# in the write window is neither detected nor repaired and the exemption stands green over
|
||||
# it. Before paging existed only a failure of the single page-1 request could reach that;
|
||||
# requiring a COMPLETE walk here would newly route a page-2 hiccup, an over-cap history or
|
||||
# one malformed id on a later page into the same hole — turning a paging fix into a wider
|
||||
# fail-open than the bug it closes.
|
||||
#
|
||||
# A mark taken over fewer rows can only be LOWER than the true maximum, and low is the safe
|
||||
# direction: it makes the post-write check MORE eager (it may repair over a pre-existing
|
||||
# row), never blinder. That is the same trade the pre-paging code made every time, since a
|
||||
# single page was all it ever read.
|
||||
ph_len=$(printf '%s' "$ph_rows" | jq -r 'if type == "array" then length else 0 end' 2>/dev/null) || ph_len=""
|
||||
case "$ph_len" in ''|*[!0-9]*) ph_len=0 ;; esac
|
||||
if [ "$ph_ok" != yes ] && [ "$ph_len" -gt 0 ]; then
|
||||
echo "::warning::Could not page the whole status history for ${SHA:0:7}; taking the high-water mark over the ${ph_len} row(s) that were read. The mark may be below the true maximum, which makes the post-write race check more eager, never blinder."
|
||||
fi
|
||||
if [ "$ph_ok" = yes ] || [ "$ph_len" -gt 0 ]; then
|
||||
# NUMBERS ONLY. jq orders strings above every number, so a single `"id": "12"` from a
|
||||
# schema-corrupt row becomes the max, passes the `*[!0-9]*` gate as `12`, and — with a
|
||||
# larger string — silently inflates the mark until nothing looks newer than it. That is a
|
||||
# fail-open, so ids that are not numbers are excluded rather than coerced.
|
||||
# AN EMPTY HISTORY AND AN UNUSABLE ONE ARE DIFFERENT ANSWERS. A head nothing has posted
|
||||
# to legitimately has no rows, and its mark is 0 — every later row is newer, which is
|
||||
# correct. But a NON-empty history in which no row carries a numeric id is a schema we
|
||||
# cannot read, and collapsing that to 0 makes every pre-existing row look newer than the
|
||||
# mark and repairs the exemption away. So the two are reported separately and only the
|
||||
# first yields a number; `none` falls through the numeric gate below and skips the check.
|
||||
mark=$(printf '%s' "$ph_rows" | jq -r 'if length == 0 then 0 else ([.[] | select(type == "object") | .id | numbers] | max // "none") end' 2>/dev/null || true)
|
||||
case "$mark" in
|
||||
''|*[!0-9]*)
|
||||
# SKIP the check rather than treat everything as raced. A mark of 0 would make every
|
||||
@@ -1142,21 +1287,31 @@ jobs:
|
||||
# live head), `/statuses/{sha}` a BARE ARRAY (24 rows on the same head) — hence the different
|
||||
# `type == "array"` guard here.
|
||||
#
|
||||
# ORDER IS NOT RELIED ON *WITHIN* A PAGE — the check selects by id against the high-water
|
||||
# mark rather than inspecting the top of the list, so a verdict older than the mark is
|
||||
# invisible to it no matter where it sits. But this read is ONE PAGE, and `limit=100` clamps
|
||||
# to the server-wide `MAX_RESPONSE_ITEMS`, measured at 50 on this instance (ersatztv#751).
|
||||
# An earlier version of this comment claimed order-independence flatly; that is false the
|
||||
# moment a head carries more rows than the clamp, because a raced row can then sit on a page
|
||||
# this never reads. Measured: a probe head reached 33 rows after ~5 runs, and the ordering is
|
||||
# only coarsely newest-first (ids came back `33,32,31,30,28,29,27,…`), so >50 is ordinarily
|
||||
# reachable on a PR with a few CI reruns.
|
||||
# ORDER IS NOT RELIED ON *WITHIN* THE RESULT — the check selects by id against the
|
||||
# high-water mark rather than inspecting the top of the list, so a verdict older than the
|
||||
# mark is invisible to it no matter where it sits. The read is now the whole list rather
|
||||
# than one clamped page.
|
||||
#
|
||||
# "THE WHOLE LIST" MEANS WHAT WAS PRESENT WHEN EACH PAGE WAS FETCHED, not a snapshot. These
|
||||
# are independent offset-paginated GETs with no snapshot token, so a row INSERTED MID-WALK
|
||||
# can be missed: under the server default (`created_unix DESC`, measured) a new row lands at
|
||||
# position 0 — on a page already read — while everything else shifts down, so the walk
|
||||
# re-reads a duplicate and never sees the newcomer. That residual is ACCEPTED, and it is
|
||||
# bounded: a row arriving after this job's POST is not one this job overwrote, and being
|
||||
# newest it wins on the combined endpoint that branch protection actually reads.
|
||||
#
|
||||
# `sort=highestindex` WAS TRIED AND WITHDRAWN. It returns `index` ASCENDING (measured), which
|
||||
# does close the mid-walk-insert gap — an insert appends at the end, onto a page not yet
|
||||
# fetched. But ASC puts the OLDEST rows on page 1, and that inverts the safety of the
|
||||
# partial-mark fallback below: an incomplete walk would then take its maximum over the
|
||||
# oldest rows, leaving every pre-existing row above the mark and read as "raced" — the
|
||||
# spurious STICKY repair this change exists to stop. Under the server default the newest row
|
||||
# is on page 1 by construction, so a partial mark is at or very near the true maximum.
|
||||
# Closing a bounded gap by re-opening the one that caused #761 is the wrong trade.
|
||||
#
|
||||
# That matters more here than anywhere else in this job, because this is the ONE path whose
|
||||
# failure direction is toward SUCCESS: missing a raced human `failure` leaves a forged green
|
||||
# standing over a rejection. So the page-2 probe below treats "there are rows I did not read"
|
||||
# as "assume raced" — the conservative direction — rather than as "no race found".
|
||||
# Full paging of this endpoint, including for the high-water mark above, is ersatztv#763.
|
||||
# standing over a rejection. Everything below therefore resolves uncertainty to `pending`.
|
||||
#
|
||||
# The repair is `pending`, NEVER a copy of the human's state. Re-posting their `failure`
|
||||
# would attribute a human verdict to this job — the exact provenance confusion the
|
||||
@@ -1167,11 +1322,57 @@ jobs:
|
||||
#
|
||||
# A read failure here does NOT fail the job: the status is already posted, so `exit 1` would
|
||||
# change nothing about the gate's state while turning a routine API hiccup into a red run.
|
||||
# It is reported loudly and left alone — the residual is the read/POST gap either way.
|
||||
# It is reported loudly and REPAIRED (ersatztv#763) — the old treatment left it alone, which
|
||||
# is the wrong direction on this path; see below.
|
||||
if [ "$state" = "success" ] && [ "$max_id_before" -ge 0 ]; then
|
||||
post_hist=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100") || post_hist=""
|
||||
if [ -z "${post_hist//[[:space:]]/}" ] || ! printf '%s' "$post_hist" | jq -e 'type == "array"' >/dev/null 2>&1; then
|
||||
echo "::warning::Could not re-read the status history for ${SHA:0:7} after posting, so a human verdict landing during the write window would not be detected. The exemption ${CONTEXT}=success stands."
|
||||
page_statuses
|
||||
# AN EMPTY HISTORY IS IMPOSSIBLE HERE, so it is not evidence. This job has just POSTed to
|
||||
# this sha, and `/statuses/{sha}` returns one row per POST, so a well-formed response
|
||||
# saying "no statuses exist" contradicts a write that succeeded. Accepting it yields
|
||||
# `raced=0` — a green concluded from a list that cannot be real — on the one path whose
|
||||
# failure direction is toward SUCCESS, and silently, since the walk itself reports
|
||||
# success. The predecessor was not better in the way it first appears: its
|
||||
# `jq -e 'type == "array"'` gate ACCEPTED `[]` and, of the two empty shapes, warned only
|
||||
# on `null` (an empty body and a non-array object warned too), so the `[]` half
|
||||
# of this fail-open was already unlogged on `main`. What is new is that the paged walk
|
||||
# additionally reports such a read as a SUCCESS, which is why the rejection lives here.
|
||||
#
|
||||
# THIS IS NOT THE WITHDRAWN CURRENCY WITNESS, and the difference is what makes it safe.
|
||||
# That mechanism asked "is there ANY row above the mark", which an unrelated newer row
|
||||
# satisfied while the rejection stayed hidden, and which fired on schema-valid staleness.
|
||||
# This asks only whether the list is EMPTY — a state no unrelated row can produce and no
|
||||
# ordering can disguise. It carries neither defect.
|
||||
ph_len_after=$(printf '%s' "$ph_rows" | jq -r 'if type == "array" then length else 0 end' 2>/dev/null) || ph_len_after=0
|
||||
case "$ph_len_after" in ''|*[!0-9]*) ph_len_after=0 ;; esac
|
||||
# WHY the repair fired, so the log can stop asserting something it does not know. The
|
||||
# sentinel DESCRIPTION stays generic on purpose — the classification recognises it as a
|
||||
# fixed point, so its wording is load-bearing — but the `::error::` beside it need not be.
|
||||
#
|
||||
# THE EMPTY CASE GETS ITS OWN REASON rather than borrowing the unreadable one. Routing it
|
||||
# through "could not be read completely" would print that beside a walk that DID complete,
|
||||
# on a validated terminator — the answer was impossible, not unreadable, and an operator
|
||||
# reading a sticky sentinel needs to know which.
|
||||
raced_why=human
|
||||
if [ "$ph_ok" = yes ] && [ "$ph_len_after" -eq 0 ]; then
|
||||
echo "::warning::The status history for ${SHA:0:7} came back empty after this job posted to it, which cannot be true, so a raced verdict could not be ruled out. Repairing ${CONTEXT} to pending."
|
||||
raced_why="the status history came back empty after this job posted to it"
|
||||
raced=1
|
||||
elif [ "$ph_ok" != yes ]; then
|
||||
# FAIL CLOSED, AND THAT IS A BEHAVIOUR CHANGE (ersatztv#763). This used to warn and leave
|
||||
# the exemption green, while #751's page-2 probe (since deleted) repaired on
|
||||
# exactly the same uncertainty — the two halves of one check disagreed about which way
|
||||
# "I could not read it" resolves. On the one path whose failure direction is toward
|
||||
# SUCCESS it has to be `pending`: an unreadable history is not evidence that nothing
|
||||
# raced the write.
|
||||
#
|
||||
# This is affordable ONLY because the walk above is real paging. Under the old probe the
|
||||
# common trigger was an ordinary head carrying more than 50 rows, so failing closed then
|
||||
# would have cost the exemption on every busy PR. Now "could not establish" means a
|
||||
# genuine API failure that survived a retry, or a history past the page cap (950 rows,
|
||||
# since the twentieth request must be the empty terminator).
|
||||
echo "::warning::Could not establish a complete status history for ${SHA:0:7} after posting, so a human verdict landing during the write window could not be ruled out. Repairing ${CONTEXT} to pending rather than leaving an exemption green on an unverified head."
|
||||
raced_why="the status history could not be read completely"
|
||||
raced=1
|
||||
else
|
||||
# `.id > $since` is what confines this to the write window. Our OWN row is excluded twice
|
||||
# over — it carries `creator: null` (an Actions-token POST, measured; see the provenance
|
||||
@@ -1215,75 +1416,91 @@ jobs:
|
||||
# It cannot false-fire. A sentinel that already existed would have been seen at the FIRST
|
||||
# read, forcing the pending path, and this block only runs after a `success` — so a
|
||||
# sentinel ABOVE the mark can only have been written by another run mid-flight.
|
||||
raced=$(printf '%s' "$post_hist" | jq -r --arg c "$CONTEXT" --argjson since "$max_id_before" --arg rd "$REPAIR_DESC" \
|
||||
# `.creator` IS TYPE-TESTED BEFORE IT IS INDEXED, and the substitution is guarded.
|
||||
# `.creator != null and .creator.login` hard-errors ("Cannot index number with string")
|
||||
# on a row whose `creator` is any non-object, jq exits 5, and under `set -e` the
|
||||
# assignment takes the step down — AFTER the exemption `success` is already posted and
|
||||
# with the repair never attempted, leaving a forged green on a head that may carry a
|
||||
# rejection. Reproduced (ersatztv#763). `(.creator | type) == "object"` short-circuits jq's
|
||||
# `and` before the index, so a malformed row is dropped from the count instead of
|
||||
# killing the job, and a well-formed verdict beside it is still counted.
|
||||
raced=$(printf '%s' "$ph_rows" | jq -r --arg c "$CONTEXT" --argjson since "$max_id_before" --arg rd "$REPAIR_DESC" \
|
||||
'[.[] | select(type == "object")
|
||||
| select(.context? == $c)
|
||||
| select((.id? // 0) > $since)
|
||||
| select(((.id | numbers) // 0) > $since)
|
||||
| select(
|
||||
((.creator != null and .creator.login != null and .creator.login != "")
|
||||
(((.creator | type) == "object") and ((.creator.login // "") != "")
|
||||
and (((.description // "") | startswith("Review-verdict:"))))
|
||||
or ((.creator == null) and ((.description // "") == $rd))
|
||||
)] | length')
|
||||
# "NO RACE FOUND ON PAGE 1" IS NOT "NO RACE" (ersatztv#751). If page 1 was not the whole
|
||||
# list, a raced row can be beyond it, so an unread page is treated as a race rather than
|
||||
# as absence. Only checked when page 1 looked clean — a race already found needs no
|
||||
# further evidence. Unreadable page 2 also counts as raced: this is the fail-toward-
|
||||
# SUCCESS path, so uncertainty must resolve to `pending`, never to leaving green.
|
||||
)] | length') || raced=""
|
||||
# NO "CURRENCY WITNESS" HERE, AND THAT IS A DELIBERATE WITHDRAWAL (ersatztv#763).
|
||||
# A draft of this change also asserted that the post-write read must show at least one
|
||||
# row above the mark, on the grounds that reaching a validated empty page proves the
|
||||
# walk finished but not that it saw the POST just made. Two independent defects came out
|
||||
# of that one mechanism, which is the signal to remove it rather than patch it twice:
|
||||
#
|
||||
# BE HONEST ABOUT THE COST: a repair here is STICKY, not a one-run stall. It writes
|
||||
# `$REPAIR_DESC`, and the classification above refuses to grant an exemption over that
|
||||
# sentinel and re-writes it as a fixed point on every later run — so a SPURIOUS repair
|
||||
# (a head with more status rows than the page cap and no actual race) removes that head's
|
||||
# exemption permanently, and only a human `post-review-verdict.sh` clears it. That is
|
||||
# still the right direction, because the alternative is a forged green over a human
|
||||
# rejection. But it is a per-sha loss of the exemption, not an inconvenience, which is
|
||||
# the argument for replacing this with real paging (#763) rather than living with it.
|
||||
if [ "$raced" = "0" ]; then
|
||||
more_hist=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100&page=2") || more_hist="UNREADABLE"
|
||||
if [ "$more_hist" = "UNREADABLE" ]; then
|
||||
echo "::warning::Could not read page 2 of the status history for ${SHA:0:7}; treating this as a possible raced verdict and repairing to pending rather than leaving an exemption green on an unverified head."
|
||||
raced=1
|
||||
else
|
||||
mh_kind=$(printf '%s' "$more_hist" | jq -r 'type' 2>/dev/null) || mh_kind=""
|
||||
mh_len=$(printf '%s' "$more_hist" | jq -r 'if type == "array" then length else 0 end' 2>/dev/null) || mh_len=""
|
||||
case "${mh_kind}:${mh_len}" in
|
||||
null:*|array:0) ;;
|
||||
array:*)
|
||||
echo "::warning::The status history for ${SHA:0:7} runs past page 1 (${mh_len} more row(s)), so a raced human verdict could be on a page this job did not read. Repairing to pending rather than leaving the exemption green."
|
||||
raced=1 ;;
|
||||
*)
|
||||
echo "::warning::Page 2 of the status history for ${SHA:0:7} was '${mh_kind:-unparseable}'; treating as a possible raced verdict and repairing to pending."
|
||||
raced=1 ;;
|
||||
esac
|
||||
# * it did not witness what it claimed. Counting ANY row above the mark accepts an
|
||||
# unrelated newer row, so a stale-but-valid snapshot carrying that row and not the
|
||||
# human rejection passed the check and left the exemption green — the exact
|
||||
# fail-toward-SUCCESS it was added to prevent.
|
||||
# * it converted a single transient anomaly into a PERMANENT sentinel. A schema-valid
|
||||
# stale read is not retried (only transport failures and non-array bodies are), so
|
||||
# one such response cost that head its exemption until a human cleared it.
|
||||
#
|
||||
# The hazard it addressed is also not one this deployment plausibly has: Gitea here is a
|
||||
# single instance with no read replicas, so "the read does not reflect the write" has no
|
||||
# mechanism behind it. Removing it restores exactly the pre-change exposure on this
|
||||
# path — a non-regression — while the paging above closes the hole #763 is actually
|
||||
# about. If a real instance of a stale read ever appears, it needs a witness that
|
||||
# identifies THIS job's own row, plus a re-read before repairing.
|
||||
fi
|
||||
case "$raced" in
|
||||
''|*[!0-9]*)
|
||||
# FAILS CLOSED (ersatztv#763). "Not acting on it" left the exemption green on a head whose
|
||||
# write window could not be inspected at all — the same uncertainty every other branch
|
||||
# here resolves to `pending`, resolved the opposite way purely because it arrived as a
|
||||
# malformed count rather than a failed read.
|
||||
#
|
||||
# NO FIXTURE REACHES THIS BRANCH, and that is stated rather than implied. With the
|
||||
# type-safe filter above, `raced` is a number for every input the stub can pose, so
|
||||
# this arm is defense-in-depth against a future jq or schema change and is proved only
|
||||
# in combination (disarming it alone reddens nothing — measured, not assumed).
|
||||
echo "::warning::Post-write verification for ${SHA:0:7} returned '${raced}' instead of a count, so a raced verdict could not be ruled out; repairing ${CONTEXT} to pending."
|
||||
raced_why="the post-write verification did not return a count"
|
||||
raced=1
|
||||
;;
|
||||
esac
|
||||
# NORMALISE FIRST, THEN ACT ONCE. Setting `raced=1` inside a `case` arm and expecting the
|
||||
# repair to happen in a SIBLING arm does not work — the case has already dispatched. That
|
||||
# is why the unusable-count branch above sets the flag and the decision lives out here,
|
||||
# where every path that concluded "raced" reaches the same single writer.
|
||||
if [ "$raced" -gt 0 ]; then
|
||||
# The last-moment re-read found no human verdict, so any row present now was
|
||||
# written during the window and has just been masked by the exemption above.
|
||||
# DO NOT ASSERT AN OVERWRITE THAT MAY NOT HAVE HAPPENED (ersatztv#763). This
|
||||
# message used to be unconditional, so when the old page-2 probe repaired on mere
|
||||
# uncertainty it reported a human verdict as overwritten when none existed — which
|
||||
# is what made the live PR #761 stall undiagnosable from the status alone.
|
||||
if [ "$raced_why" = human ]; then
|
||||
echo "::error::A human ${CONTEXT} verdict landed on ${SHA:0:7} while this job was writing its exemption, and was overwritten. Downgrading to 'pending' so an explicit human decision cannot be silently green. Re-post it with: scripts/post-review-verdict.sh ${PR} <VERDICT>"
|
||||
else
|
||||
echo "::error::Could not verify that no human ${CONTEXT} verdict raced this exemption write on ${SHA:0:7} — ${raced_why}. Downgrading to 'pending' rather than leaving an unverified exemption green; no verdict was necessarily overwritten. Clear it with: scripts/post-review-verdict.sh ${PR} <VERDICT>"
|
||||
fi
|
||||
repair=$(jq -n --arg c "$CONTEXT" --arg u "$PR_URL" --arg d "$REPAIR_DESC" \
|
||||
'{state:"pending", context:$c, description:$d, target_url:$u}')
|
||||
# A failure HERE leaves the forged green standing, so it is retried once and then
|
||||
# screams. `set -e` would otherwise kill the job silently, after the success was
|
||||
# written and with nothing left to re-attempt.
|
||||
if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \
|
||||
"$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then
|
||||
if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \
|
||||
"$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then
|
||||
echo "::error::COULD NOT REPAIR ${CONTEXT} on ${SHA:0:7}. An exemption 'success' is standing on a head whose write window could not be cleared (${raced_why}). Post a verdict immediately: scripts/post-review-verdict.sh ${PR} <VERDICT>"
|
||||
exit 1
|
||||
fi
|
||||
fi
|
||||
case "$raced" in
|
||||
''|*[!0-9]*)
|
||||
echo "::warning::Post-write verification for ${SHA:0:7} returned '${raced}' instead of a count; not acting on it."
|
||||
;;
|
||||
*)
|
||||
if [ "$raced" -gt 0 ]; then
|
||||
# The last-moment re-read found no human verdict, so any row present now was
|
||||
# written during the window and has just been masked by the exemption above.
|
||||
echo "::error::A human ${CONTEXT} verdict landed on ${SHA:0:7} while this job was writing its exemption, and was overwritten. Downgrading to 'pending' so an explicit human decision cannot be silently green. Re-post it with: scripts/post-review-verdict.sh ${PR} <VERDICT>"
|
||||
repair=$(jq -n --arg c "$CONTEXT" --arg u "$PR_URL" --arg d "$REPAIR_DESC" \
|
||||
'{state:"pending", context:$c, description:$d, target_url:$u}')
|
||||
# A failure HERE leaves the forged green standing, so it is retried once and then
|
||||
# screams. `set -e` would otherwise kill the job silently, after the success was
|
||||
# written and with nothing left to re-attempt.
|
||||
if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \
|
||||
"$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then
|
||||
if ! gh -X POST -H 'Content-Type: application/json' -d "$repair" \
|
||||
"$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null 2>&1; then
|
||||
echo "::error::COULD NOT REPAIR ${CONTEXT} on ${SHA:0:7}. An exemption 'success' is standing on a head whose human verdict was overwritten. Post the verdict again immediately: scripts/post-review-verdict.sh ${PR} <VERDICT>"
|
||||
exit 1
|
||||
fi
|
||||
fi
|
||||
echo "Repaired ${CONTEXT} to pending on ${SHA:0:7}."
|
||||
state=pending
|
||||
fi
|
||||
;;
|
||||
esac
|
||||
echo "Repaired ${CONTEXT} to pending on ${SHA:0:7}."
|
||||
state=pending
|
||||
fi
|
||||
fi
|
||||
|
||||
|
||||
@@ -1574,6 +1574,86 @@ never be silently turned green" is too strong and is retracted: a generic `pendi
|
||||
verified, and a rejection it masks is re-derived green by a later run (ersatztv#849). The repair is
|
||||
`pending`, never a copy of the human's verdict, which would attribute a human decision to the job.
|
||||
|
||||
**Both `/statuses/{sha}` reads are PAGED** (ersatztv#763). The history is read twice — before the write
|
||||
for the high-water mark, after it for the race check — and `limit` clamps to `MAX_RESPONSE_ITEMS`
|
||||
(measured 50), so a single read of a busy head returned a partial list.
|
||||
|
||||
**Be precise about what that cost**, because ersatztv#763's own framing overstated it. Under the server
|
||||
default (`created_unix DESC`) page 1 holds the *newest* rows and ids are monotonic with `created_at`,
|
||||
so page 1 already carried both the true maximum id and every row newer than the mark — the only rows
|
||||
the post-write check selects on. A single-page read therefore missed a raced verdict only if **more
|
||||
than 50 rows were created inside the write window**, not merely on "a head with more than 50 rows".
|
||||
What actually removed PR #761's stall is the deletion of the page-2 probe below, not the paging.
|
||||
|
||||
The walk earns its place for a different reason: it removes the gate's one fail-toward-SUCCESS path
|
||||
from depending on an **undocumented ordering** the server honours only coarsely (page 1 came back
|
||||
`114,112,113,111,110` — newest-first, but not strictly, and nothing in the API contract promises even
|
||||
that). Both reads walk to a **validated empty page**, never terminating on a short one, retrying each
|
||||
page once. The termination rule is page-size independent; the *capacity* is not — 20 requests of 50
|
||||
validate at most 950 rows, since the twentieth must be the empty terminator. The terminator is measured
|
||||
per endpoint and they differ: `/statuses/{sha}` returns `[]`, `/issues/{n}/timeline` a bare `null`,
|
||||
`/commits/{sha}/status` an object with `statuses: null`.
|
||||
|
||||
**The page-2 probe is gone.** ersatztv#751 treated "there are rows I did not read" as "assume raced".
|
||||
That fired on Renovate PR #761: a head that grew past one page over ordinary CI re-runs had its
|
||||
exemption repaired away, with an `::error::` asserting a human verdict was overwritten when the head
|
||||
carried none, and the sticky sentinel then refused re-exemption on every later run.
|
||||
|
||||
The two directions are **not** symmetric, and the asymmetry is deliberate:
|
||||
|
||||
- **Post-write, uncertainty fails closed.** An unreadable history, an over-cap history, or a count that
|
||||
does not come back as a number all repair to `pending`. Previously an unreadable history warned and
|
||||
left the exemption green while the page-2 probe repaired on the same uncertainty — one check
|
||||
disagreeing with itself.
|
||||
- **Pre-write, a partial list still yields a mark.** The mark gates the post-write check entirely, so
|
||||
refusing one *skips* the check and is itself a fail-open. This rests on the DESC default above: the
|
||||
newest row, carrying the maximum id, is on page 1, so a walk that fails later still saw it. A
|
||||
*validated* empty history is a different case and is not abandoned: it yields a mark of 0, which is
|
||||
correct, since every later row is newer — that is the normal first run. What abandons the mark is a
|
||||
read that both **failed** and returned nothing, the pre-existing gap tracked as ersatztv#849 and
|
||||
unchanged here. A non-empty history carrying no numeric id is reported as unusable rather than
|
||||
collapsed to 0.
|
||||
|
||||
Two id comparisons are **numeric-only**, closing the *string* half of a fail-open (a type guard, not
|
||||
a value bound — a corrupt but genuinely numeric id would still inflate the mark, which is not
|
||||
attacker-controllable since ids are server-assigned): jq orders strings above every number, so one
|
||||
`"id": "99999"` would inflate the high-water mark until nothing looked newer, and
|
||||
`.id > $since` would read any string id as newer than any mark — making a pre-existing base-mismatched
|
||||
verdict look raced on every run, a permanent stall. `.creator` is likewise type-tested before it is
|
||||
indexed, because `.creator.login` on a non-object exits jq 5 and, under `set -e`, took the step down
|
||||
after the green was posted and before the repair.
|
||||
|
||||
**Rejected alternatives**, recorded so they are not re-adopted:
|
||||
|
||||
- **A currency witness** (require a row above the mark, proving the read reflects the POST just made).
|
||||
Counting *any* row above the mark does not witness this job's write, so a stale-but-valid snapshot
|
||||
carrying an unrelated newer row still passes; and a schema-valid stale read is not retried, so one
|
||||
such response turns a transient anomaly into a permanent sentinel. Gitea here is a single instance
|
||||
with no read replicas, so the hazard has no mechanism behind it.
|
||||
- **`sort=highestindex`** (index ASC, measured), which would close the mid-walk-insert gap below. ASC
|
||||
puts the *oldest* rows on page 1, which inverts the partial-mark fallback: the incomplete walk's
|
||||
maximum becomes the max of the oldest rows, leaving every pre-existing row above the mark and read as
|
||||
raced. That is the #761 failure, re-introduced to close a smaller one. A test asserts the request
|
||||
carries no sort parameter.
|
||||
|
||||
**Accepted residual: a row inserted mid-walk can be missed.** These are independent offset-paginated
|
||||
GETs with no snapshot token, so under DESC a new row lands at position 0 — on a page already read —
|
||||
while everything else shifts down. It is bounded: a row arriving after this job's POST is not one the
|
||||
job overwrote, and being newest it wins on the combined endpoint branch protection reads.
|
||||
|
||||
**An empty post-write history is rejected.** The walk terminates on an empty page — correct before the
|
||||
write, impossible after it, since this job has just POSTed and the endpoint returns one row per POST. A
|
||||
well-formed "no statuses exist" is not retried (the body is valid), so accepting it would conclude
|
||||
`raced=0` from a list that cannot be real, silently. This is *not* the withdrawn currency witness: that
|
||||
asked whether ANY row sat above the mark, which an unrelated newer row satisfied; this asks only whether
|
||||
the list is empty, which no unrelated row can produce.
|
||||
|
||||
The `::error::` names its own cause, and there are three: a verdict actually **found**; a read that
|
||||
could not be **completed**; and a read that completed but returned an **impossible** answer. The third
|
||||
is not a variety of the second — an operator holding a sticky sentinel needs to know whether to go
|
||||
looking for an API failure that never happened. The sentinel *description* stays generic, because the
|
||||
classification recognises it as a fixed point.
|
||||
|
||||
Three properties of this workflow are security-relevant and are **structurally** asserted by tests in
|
||||
`scripts/tests/test_pr_changed_files.py` — those tests pin the workflow's shape, which is not the same
|
||||
as establishing that the gate cannot be forged (see the residual below, and ersatztv#697/#698):
|
||||
|
||||
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
@@ -1013,24 +1013,53 @@ if "/timeline" in url:
|
||||
if "/statuses/" in url:
|
||||
mode = os.environ.get("STUB_HISTORY_MODE", "none")
|
||||
|
||||
# PAGE, honoured. THE REASON IS THE TERMINATOR, NOT THE COUNTERS — and that distinction was itself
|
||||
# a finding (#751 round 5). An earlier version of this comment said the guard sits ahead of the
|
||||
# read-counting modes "so the page-2 probe cannot shift 'raced row appears on read N'", by analogy
|
||||
# with the combined endpoint. Measured: moving this guard AFTER the counter modes reddens NOTHING,
|
||||
# because no history mode that counts reads ever issues a page-2 request — `raced=1` on page 1
|
||||
# short-circuits the probe. So that half of the rationale read as a checked reason and was not,
|
||||
# which is the precise class this branch exists to retire.
|
||||
# PAGES FAITHFULLY (ersatztv#763). The job walks this endpoint to a validated empty page instead
|
||||
# of reading one clamped page, so a stub describing page 1 only no longer describes anything the
|
||||
# job does. The snapshot is a LIST OF PAGES, not a flat list, because two modes below need a page
|
||||
# boundary the uniform 50-row slicing cannot express.
|
||||
#
|
||||
# What IS load-bearing: page 2 must terminate for every mode that describes page 1 only, or the job
|
||||
# re-reads page 1's rows as a second page and concludes the list is longer than it is. Dropping
|
||||
# just the `print("[]")` below reddens `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`.
|
||||
# (The ordering guard on the COMBINED endpoint is a different story and genuinely is counter-
|
||||
# related — moving that one reddens three mid-run-race tests.)
|
||||
# 1. PAGE SIZE AND TERMINATOR match the real endpoint. Measured at 1.27.1 on 2026-08-28 against
|
||||
# PR #761's 114-row head: pages 1 and 2 return 50, page 3 returns 14, page 4 is `[]`. So the
|
||||
# slice width is 50 and past the end is `[]` — NOT `null`, which is what
|
||||
# `/issues/{n}/timeline` returns and what `/commits/{sha}/status` spells `{"statuses": null}`.
|
||||
# Three distinct empty shapes on one server; each stub owes its own measurement.
|
||||
#
|
||||
# PAST THE END THIS ENDPOINT RETURNS `[]`, NOT `null` — measured at 1.27.1:
|
||||
# `/statuses/{sha}?limit=100&page=99` is the two bytes `[]`, while `/commits/{sha}/status` past the
|
||||
# end returns `{"statuses": null}`. A THIRD distinct empty shape on one server; the job tolerates
|
||||
# both here precisely because guessing per endpoint has been wrong twice.
|
||||
# 2. ONE LOGICAL READ IS ONE SNAPSHOT. The counter modes resolve their rows from how many times
|
||||
# the job has LOOKED, and the job now looks two or three times per logical read. Recomputing
|
||||
# on page 2 from an advanced counter would describe a different history than page 1 and
|
||||
# surface `human-after-post`'s verdict halfway through the PRE-write read, inverting the race
|
||||
# being modelled. So the pages are resolved once, on page 1, and cached.
|
||||
#
|
||||
# This is a MODELLING CHOICE, not a fidelity claim, and the difference matters. These are
|
||||
# independent offset-paginated GETs with no snapshot token, so the REAL history can change
|
||||
# between page requests — overlapping rows and shifting offsets are all reachable live. The
|
||||
# cache deliberately suppresses that, because the tests here are about the job's paging
|
||||
# logic, not about mid-walk mutation. Mid-walk mutation is untested, and saying so is the
|
||||
# honest form of the claim.
|
||||
#
|
||||
# DELIBERATELY ORDERING-BLIND, and this bounds what the paging tests prove. The real endpoint
|
||||
# serves `created_unix DESC`, so a row created DURING the write window is among the NEWEST and
|
||||
# lands on page 1 — a genuinely raced verdict would not sit on page 2 at all. These pages are
|
||||
# served in insertion order regardless, so a fixture can place a row beyond page 1 and observe
|
||||
# whether the walk reaches it.
|
||||
#
|
||||
# So `verdict-on-page-2` and `verdict-after-short-page` prove WALK COMPLETENESS — that the job
|
||||
# reads past page 1, and past a short page, to a validated empty one. They do NOT prove that a
|
||||
# real raced verdict would otherwise be missed; under the server's ordering it would not be. That
|
||||
# is the honest scope, and the workflow comment says the same thing: the walk exists so the one
|
||||
# fail-toward-SUCCESS path stops depending on an undocumented ordering, not because the ordering
|
||||
# is currently wrong.
|
||||
#
|
||||
# Counter branches are confined to the page-1 arm below, so a later page can never advance them.
|
||||
HIST_PAGE_SIZE = 50
|
||||
# ORDER-FAITHFUL MODES HONOUR `sort`. Most modes here are ordering-blind on purpose (see above):
|
||||
# they test walk COMPLETENESS, which is an order-independent property, and serving insertion order
|
||||
# lets a fixture place a row beyond page 1. But the partial-mark fallback's safety is a claim ABOUT
|
||||
# the ordering, so the mode that pins it serves the real arrangement — DESC by default, ASC when
|
||||
# the request asks for `sort=highestindex`. That is what lets re-adding the withdrawn parameter be
|
||||
# caught by BEHAVIOUR rather than only by a structural assertion on the request line.
|
||||
want_asc = "sort=highestindex" in url
|
||||
order_faithful = False
|
||||
hist_page = 1
|
||||
for part in url.split("?", 1)[-1].split("&"):
|
||||
if part.startswith("page="):
|
||||
@@ -1038,55 +1067,249 @@ if "/statuses/" in url:
|
||||
hist_page = int(part.split("=", 1)[1])
|
||||
except ValueError:
|
||||
hist_page = 1
|
||||
|
||||
ordinary = [{"id": 7000 + i, "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"} for i in range(60)]
|
||||
raced_row = {"id": 9000, "context": "review-verdict/h10", "status": "failure",
|
||||
"creator": {"login": "timothy"},
|
||||
"description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"}
|
||||
|
||||
snapshot = out / "history_snapshot.json"
|
||||
logical = out / "history_logical_reads.txt"
|
||||
if hist_page == 1:
|
||||
n_logical = (int(logical.read_text()) if logical.exists() else 0) + 1
|
||||
logical.write_text(str(n_logical))
|
||||
rows = []
|
||||
pages = None
|
||||
if mode.startswith("human-after-post"):
|
||||
# The raced verdict: absent when the high-water mark is taken, present afterwards. Its id
|
||||
# is ABOVE the mark, which is what makes it detectable.
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen > 0:
|
||||
# Configurable creator (ersatztv#742). The RACED test must stay broader than the
|
||||
# INHERITANCE test: a verdict from an account off `$H10_REVIEWERS` still counts as
|
||||
# "something human landed while we were writing", because narrowing it here would
|
||||
# leave the exemption green over that row instead of repairing to pending.
|
||||
rows = [dict(raced_row, id=5000,
|
||||
creator={"login": os.environ.get("STUB_HISTORY_CREATOR", "timothy")})]
|
||||
elif mode == "sentinel-after-post":
|
||||
# Another overlapping run repaired mid-flight: its SENTINEL lands above this run's mark,
|
||||
# while the human row it records sits BELOW the mark and is therefore invisible here.
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen > 0:
|
||||
rows = [{"id": 6000, "context": "review-verdict/h10", "status": "pending",
|
||||
"creator": None,
|
||||
"description": "Human verdict raced this exemption write — re-post the verdict"}]
|
||||
elif mode == "stale-human-already-present":
|
||||
# A human verdict that was ALREADY in the history before this run — e.g. one whose
|
||||
# recorded base did not match, which `read_existing_verdict` deliberately declines to
|
||||
# honour. It must NOT be mistaken for a raced write, or every later run of that PR would
|
||||
# repair its own exemption to pending forever.
|
||||
rows = [{"id": 10, "context": "review-verdict/h10", "status": "success",
|
||||
"creator": {"login": "timothy"},
|
||||
"description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"}]
|
||||
elif mode in ("second-page", "verdict-on-page-2", "premark-page2-error",
|
||||
"flaky-page2", "premark-page1-error"):
|
||||
# A history that genuinely runs past one page: 60 ORDINARY rows, no verdict and no
|
||||
# sentinel. Under the pre-#763 page-2 probe the mere existence of these rows forced a
|
||||
# repair; now they are simply read, and `second-page` asserts the exemption STANDS.
|
||||
rows = list(ordinary)
|
||||
if mode in ("verdict-on-page-2", "premark-page2-error"):
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen > 0:
|
||||
# THE RACED VERDICT, PLACED BEYOND THE FIRST PAGE. Its id is above every ordinary
|
||||
# row, so it is above the high-water mark too. A job that reads only page 1 cannot
|
||||
# see it — the fail-toward-SUCCESS hole #763 closes.
|
||||
rows.insert(55, dict(raced_row))
|
||||
elif mode == "premark-page2-error-desc":
|
||||
# THE PARTIAL-MARK FALLBACK, MEASURED UNDER THE REAL ORDERING. 60 rows, one of them a
|
||||
# PRE-EXISTING base-mismatched human verdict at id 7055 — genuinely older than the newest
|
||||
# row and therefore not something that raced this write.
|
||||
#
|
||||
# Under the server default (DESC) page 1 carries 7059..7010, so a walk that fails on page 2
|
||||
# still saw the true maximum: the mark is 7059, the verdict at 7055 is below it, and the
|
||||
# exemption correctly STANDS. Under ASC page 1 carries 7000..7049, the salvaged mark is
|
||||
# 7049, and that same pre-existing verdict tests as NEWER than the mark — a spurious sticky
|
||||
# repair on a head nothing raced, which is the #761 failure. This fixture is what makes
|
||||
# that difference observable.
|
||||
order_faithful = True
|
||||
rows = [{"id": 7000 + i, "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"}
|
||||
for i in range(60) if 7000 + i != 7055]
|
||||
rows.append({"id": 7055, "context": "review-verdict/h10", "status": "success",
|
||||
"creator": {"login": "timothy"},
|
||||
"description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"})
|
||||
rows.sort(key=lambda r: r["id"])
|
||||
elif mode == "null-page1-after-post":
|
||||
# The post-write read answers "no statuses exist" for a sha this job has just POSTed to.
|
||||
# That is impossible — the endpoint returns one row per POST — but it is well-formed, so
|
||||
# nothing retries it and the walk reports SUCCESS over it.
|
||||
#
|
||||
# NO RACED VERDICT IS PLANTED, deliberately. An earlier shape appended one to dramatise
|
||||
# the stakes, and it was inert twice over: unconditionally it also joined the PRE-write
|
||||
# read, which set the mark to its OWN id — the strict `> $since` then excluded it — and
|
||||
# gated to the post-write read it is never served at all, because page 1 answers `null`
|
||||
# before any row reaches the wire. A row the test
|
||||
# cannot observe is decoration that reads as coverage. What is actually under test is
|
||||
# narrower and sufficient: a response asserting an empty history for a sha this job wrote
|
||||
# to must not be accepted as proof that nothing raced.
|
||||
rows = [{"id": 7000 + i, "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"} for i in range(3)]
|
||||
elif mode == "null-terminator":
|
||||
# This endpoint returns `[]` past the end today. `null` is what `/issues/{n}/timeline`
|
||||
# returns, and an array-only gate on THAT endpoint was ersatztv#751 — the walk never
|
||||
# reached a validated empty page and every exemption was withheld. Same shape, second
|
||||
# endpoint, worse blast radius (the sticky sentinel), so `null` is tolerated and this
|
||||
# fixture proves it rather than asserting it.
|
||||
rows = [{"id": 7000 + i, "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"} for i in range(60)]
|
||||
elif mode == "string-id-preexisting-row":
|
||||
# THE TWIN of `string-id-inflates-mark`, on the COMPARISON rather than the maximum. A
|
||||
# PRE-EXISTING row (a base-mismatched verdict `read_existing_verdict` declines to honour)
|
||||
# carries a string id. jq orders strings above every number, so `.id > $since` reads it as
|
||||
# newer than any mark and counts it as having raced this write — a sticky sentinel and a
|
||||
# false "was overwritten" on every later run of that PR, forever.
|
||||
rows = [{"id": 9999, "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"},
|
||||
{"id": "3", "context": "review-verdict/h10", "status": "success",
|
||||
"creator": {"login": "timothy"},
|
||||
"description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"}]
|
||||
elif mode == "no-numeric-ids":
|
||||
# A NON-EMPTY history in which no row carries a numeric id. Distinct from an EMPTY history,
|
||||
# whose mark of 0 is correct; collapsing this case to 0 too would be a schema we cannot
|
||||
# read reported as one we can.
|
||||
rows = [{"id": "abc", "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"}]
|
||||
elif mode == "over-cap":
|
||||
# Longer than the 20-page cap (1050 rows), so the walk can never reach a validated empty
|
||||
# page. That is "could not establish", not exhaustion.
|
||||
rows = [{"id": 20000 + i, "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"} for i in range(1050)]
|
||||
elif mode == "string-id-inflates-mark":
|
||||
# A STRING ID. jq orders strings above every number, so `max` over raw ids returns
|
||||
# `"99999"` — which then passes the numeric gate as a plain `99999` and sets a high-water
|
||||
# mark far above anything real. Every later row looks OLDER than the mark, so the raced
|
||||
# verdict below is invisible and the exemption stands over it. One corrupt row is enough.
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
rows = [{"id": "99999", "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"},
|
||||
{"id": 10, "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"}]
|
||||
if seen > 0:
|
||||
rows.append(dict(raced_row))
|
||||
elif mode == "malformed-creator-beside-verdict":
|
||||
# A SCHEMA-CORRUPT ROW NEXT TO A REAL ONE. `creator` is a number, so `.creator.login`
|
||||
# hard-errors in jq ("Cannot index number with string"), jq exits 5, and an unguarded
|
||||
# `raced=$(...)` takes the whole step down under `set -e` — after the exemption `success`
|
||||
# is posted and with the repair never attempted. The genuine verdict beside it is what
|
||||
# makes the consequence visible: the correct behaviour is to drop the malformed row and
|
||||
# still repair.
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen > 0:
|
||||
rows = [{"id": 8500, "context": "review-verdict/h10", "status": "failure",
|
||||
"creator": 7, "description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"},
|
||||
dict(raced_row)]
|
||||
elif mode == "verdict-after-short-page":
|
||||
# DELIBERATELY UNFAITHFUL, and that is the point. Page 2 is SHORT (10 rows) and yet page 3
|
||||
# still carries rows — a shape the measured server does not produce, but exactly what a
|
||||
# truncated or partially-served response looks like. It is the only way to observe the
|
||||
# rule "terminate ONLY on a validated EMPTY page, never on a short one": against a
|
||||
# faithful double a short page is always the last one, so an implementation that stops
|
||||
# there is indistinguishable from a correct one.
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
pages = [ordinary[:50], ordinary[50:], [dict(raced_row)] if seen > 0 else []]
|
||||
if order_faithful and not want_asc:
|
||||
rows = list(reversed(rows))
|
||||
if pages is None:
|
||||
pages = [rows[i:i + HIST_PAGE_SIZE] for i in range(0, len(rows), HIST_PAGE_SIZE)] or [[]]
|
||||
|
||||
# THE JOB'S OWN WRITES APPEAR IN THE HISTORY. `/statuses/{sha}` returns one row per POST, so
|
||||
# once this job has posted its exemption the next read MUST show it. The stub did not model
|
||||
# that at all — the history was whatever the mode described, before and after the write alike,
|
||||
# so every ordinary run looked like a head nothing had ever been posted to. `creator: null` is
|
||||
# measured (an Actions-token POST records no creator), which is also what keeps our own row
|
||||
# out of the raced count; the id is one above everything present, mirroring the real
|
||||
# endpoint's per-head monotonic ids (verified live: 114 rows, ids strictly increasing with
|
||||
# `created_at`, no duplicates).
|
||||
posted_log = out / "posted_all.jsonl"
|
||||
if posted_log.exists():
|
||||
for line in posted_log.read_text().splitlines():
|
||||
if not line.strip():
|
||||
continue
|
||||
body = json.loads(line)
|
||||
# NUMERIC IDS ONLY when computing the next one. A fixture may deliberately carry a
|
||||
# schema-corrupt id (see `string-id-inflates-mark`), and `max()` over a str beside an
|
||||
# int raises TypeError — which fails the whole stub request, makes the walk look
|
||||
# unreadable, and produces a repair for a reason the test was not asking about. That
|
||||
# is a test passing for the wrong reason: the string-id mutation stayed green because
|
||||
# the double crashed rather than because the mark was computed correctly.
|
||||
nxt = max([r.get("id") for pg in pages for r in pg
|
||||
if isinstance(r.get("id"), int)] or [0]) + 1
|
||||
own = {"id": nxt, "context": body.get("context"),
|
||||
"status": body.get("state"), "creator": None,
|
||||
"description": body.get("description", "")}
|
||||
# WHERE the newest row goes depends on the served ordering. Under DESC it is FIRST;
|
||||
# appending it last would put the job's own POST on the OLDEST page, the arrangement
|
||||
# the order-faithful fixture exists to rule out. Ordering-blind modes keep insertion
|
||||
# order, which is what lets them place a row beyond page 1.
|
||||
if order_faithful and not want_asc:
|
||||
pages[0].insert(0, own)
|
||||
else:
|
||||
pages[-1].append(own)
|
||||
snapshot.write_text(json.dumps(pages))
|
||||
else:
|
||||
n_logical = int(logical.read_text()) if logical.exists() else 1
|
||||
pages = json.loads(snapshot.read_text()) if snapshot.exists() else [[]]
|
||||
|
||||
if hist_page == 1 and mode == "premark-page1-error":
|
||||
# PAGE 1 ITSELF FAILS, for the whole first logical read (both attempts), so the walk returns
|
||||
# nothing at all and the mark must be abandoned. Counted in its own file because the logical
|
||||
# read counter below counts page-1 REQUESTS, retries included — not logical reads.
|
||||
ctr = out / "p1_attempts.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen < 2:
|
||||
sys.exit(22)
|
||||
if hist_page > 1 and mode == "flaky-page2":
|
||||
# TRANSIENT: fails the first attempt of each logical read and succeeds on the retry. This is
|
||||
# the only fixture that exercises the retry at all — the other error modes fail every attempt,
|
||||
# so against them a one-shot walk and a retrying walk are indistinguishable.
|
||||
ctr = out / "page2_attempts.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen % 2 == 0:
|
||||
sys.exit(22)
|
||||
if hist_page > 1 and mode == "second-page-garbage":
|
||||
print("<html>502 Bad Gateway</html>")
|
||||
sys.exit(0)
|
||||
if hist_page > 1 and mode == "second-page-error":
|
||||
sys.exit(22)
|
||||
if hist_page > 1 and mode not in ("second-page", "second-page-garbage", "second-page-error"):
|
||||
print("[]")
|
||||
if hist_page == 1 and mode == "null-page1-after-post" and n_logical >= 2:
|
||||
print("null")
|
||||
sys.exit(0)
|
||||
if mode == "second-page":
|
||||
# The history runs past page 1, so "no raced row on page 1" does not establish "no race". The
|
||||
# rows are deliberately ORDINARY (no verdict, no sentinel): the point is that unread rows EXIST,
|
||||
# not that a verdict was found, so the repair must fire on uncertainty alone.
|
||||
print(json.dumps([{"id": 7000 + i, "context": "ci/other", "status": "success",
|
||||
"creator": None, "description": "unrelated"} for i in range(3)]))
|
||||
if hist_page > 1 and mode == "null-terminator" and hist_page > len(pages):
|
||||
print("null")
|
||||
sys.exit(0)
|
||||
rows = []
|
||||
if mode.startswith("human-after-post"):
|
||||
# The raced verdict: absent when the high-water mark is taken, present afterwards. Its id is
|
||||
# ABOVE the mark, which is what makes it detectable.
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen > 0:
|
||||
# Configurable creator (ersatztv#742). The RACED test must stay broader than the
|
||||
# INHERITANCE test: a verdict from an account off `$H10_REVIEWERS` still counts as
|
||||
# "something human landed while we were writing", because narrowing it here would leave
|
||||
# the exemption green over that row instead of repairing to pending.
|
||||
rows = [{"id": 5000, "context": "review-verdict/h10", "status": "failure",
|
||||
"creator": {"login": os.environ.get("STUB_HISTORY_CREATOR", "timothy")},
|
||||
"description": "Review-verdict: BLOCKED @ a9e3e23 (base: main)"}]
|
||||
elif mode == "sentinel-after-post":
|
||||
# Another overlapping run repaired mid-flight: its SENTINEL lands above this run's mark, while
|
||||
# the human row it records sits BELOW the mark and is therefore invisible here (round 5).
|
||||
ctr = out / "history_reads.txt"
|
||||
seen = int(ctr.read_text()) if ctr.exists() else 0
|
||||
ctr.write_text(str(seen + 1))
|
||||
if seen > 0:
|
||||
rows = [{"id": 6000, "context": "review-verdict/h10", "status": "pending",
|
||||
"creator": None,
|
||||
"description": "Human verdict raced this exemption write — re-post the verdict"}]
|
||||
elif mode == "stale-human-already-present":
|
||||
# A human verdict that was ALREADY in the history before this run — e.g. one whose recorded
|
||||
# base did not match, which `read_existing_verdict` deliberately declines to honour. It must
|
||||
# NOT be mistaken for a raced write, or every later run of that PR would repair its own
|
||||
# exemption to pending forever.
|
||||
rows = [{"id": 10, "context": "review-verdict/h10", "status": "success",
|
||||
"creator": {"login": "timothy"},
|
||||
"description": "Review-verdict: MERGEABLE @ a9e3e23 (base: other)"}]
|
||||
print(json.dumps(rows))
|
||||
if hist_page > 1 and mode in ("premark-page2-error", "premark-page2-error-desc") and n_logical == 1:
|
||||
# Fails ONLY on the PRE-write read, so the high-water mark must be salvaged from the partial
|
||||
# list; the post-write read then pages cleanly and must still catch the raced verdict.
|
||||
# `n_logical` is incremented on every page-1 REQUEST, including retried ones — which is fine
|
||||
# here because page 1 never fails in this mode, but it counts requests, not logical reads.
|
||||
sys.exit(22)
|
||||
|
||||
print(json.dumps(pages[hist_page - 1] if hist_page - 1 < len(pages) else []))
|
||||
sys.exit(0)
|
||||
|
||||
if "/status" in url:
|
||||
@@ -2670,7 +2893,16 @@ def test_the_high_water_MARK_is_captured_BEFORE_the_last_moment_re_read():
|
||||
# Assert on BOTH the init and the FETCH: keying only on the init line would let a refactor that
|
||||
# splits initialisation from the read slide the actual round-trip back past the re-read while this
|
||||
# test stayed green (round-3 review nit).
|
||||
mark = max(src.index("max_id_before=-1"), src.index('hist_before=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA'))
|
||||
# The FETCH is now the first bare `page_statuses` CALL (ersatztv#763 replaced the single
|
||||
# `hist_before=$(gh …)` read with the paged walk). Anchoring on the call and not on the function
|
||||
# definition matters: the definition sits with the other helpers near the top of the step, so
|
||||
# keying on it would place the "fetch" far earlier than the round-trip actually happens and this
|
||||
# assertion would hold vacuously.
|
||||
assert "\npage_statuses\n" in src, (
|
||||
"no bare `page_statuses` call site found; the mark is no longer fetched where this test "
|
||||
"believes it is, and the ordering assertion below would be vacuous"
|
||||
)
|
||||
mark = max(src.index("max_id_before=-1"), src.index("\npage_statuses\n"))
|
||||
# The LAST-MOMENT re-read is the second bare `read_existing_verdict` call.
|
||||
calls = [i for i in range(len(src)) if src.startswith("read_existing_verdict\n", i)]
|
||||
assert len(calls) >= 2, f"expected two read_existing_verdict call sites, found {len(calls)}"
|
||||
@@ -3513,25 +3745,62 @@ def test_an_UNREADABLE_page_2_refuses_to_conclude_that_no_verdict_exists(tmp_pat
|
||||
)
|
||||
|
||||
|
||||
def test_a_status_history_RUNNING_PAST_PAGE_1_repairs_rather_than_leaving_green(tmp_path):
|
||||
"""The post-write race check reads ONE page of `/statuses/{sha}`, and `limit=100` clamps to the
|
||||
server-wide cap (measured 50). So "no raced row on page 1" does not establish "no race" — a raced
|
||||
human `failure` can sit on a page this job never reads.
|
||||
def test_a_status_history_RUNNING_PAST_PAGE_1_is_PAGED_and_the_exemption_STANDS(tmp_path):
|
||||
"""THE INVERSION (ersatztv#763). This test asserted the opposite until real paging existed, and
|
||||
the change of expectation IS the fix.
|
||||
|
||||
This is the ONE path in the design whose failure direction is toward SUCCESS: missing a raced
|
||||
verdict leaves a forged green over a human rejection, permanently. So unread rows are treated as a
|
||||
race and the exemption is repaired to `pending`, which is the conservative direction — a stall a
|
||||
reviewer can clear, rather than a rejection silently turned green.
|
||||
Before: the post-write check read one clamped page, so "no raced row on page 1" did not establish
|
||||
"no race", and #751's conservative probe treated any rows on page 2 as a race. A head with more
|
||||
status rows than the cap therefore lost its exemption to a STICKY sentinel with no race anywhere
|
||||
near it. That is not hypothetical — it fired on Renovate PR #761, whose head accumulated rows over
|
||||
ordinary CI re-runs until the gate started reporting a human verdict as overwritten when none
|
||||
existed.
|
||||
|
||||
The stub's second page carries ORDINARY rows, no verdict and no sentinel: what must trigger the
|
||||
repair is the mere existence of rows this job did not read, not the discovery of a verdict.
|
||||
After: the walk pages to a validated empty page, so rows beyond the first page are simply READ.
|
||||
The stub's history is 60 ORDINARY rows — no verdict, no sentinel — and the correct answer is that
|
||||
nothing raced this write, so the exemption must STAND.
|
||||
|
||||
This is the discriminating half of the pair: it is the assertion that goes red against the old
|
||||
single-page read (which would repair here), while
|
||||
`test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired` goes red against a walk that pages but
|
||||
stops early. Neither test alone can tell "pages correctly" from "repairs on everything".
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="second-page")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 1, (
|
||||
"the exemption was repaired away even though the whole history was readable and carried no "
|
||||
f"verdict — the page-2 'assume raced' probe is still in force. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}"
|
||||
# The false `::error::` from the old probe is the thing PR #761 could not be diagnosed from, so
|
||||
# pin its absence too — a run that stands green but still shouts about an overwritten verdict
|
||||
# would satisfy the count assertion above while leaving the log as misleading as before.
|
||||
assert "was overwritten" not in (r.stdout + r.stderr), (
|
||||
f"the exemption stood, but the log still claims a verdict was overwritten:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_raced_verdict_on_PAGE_2_is_detected_and_repaired(tmp_path):
|
||||
"""The hole #763 exists to close, and the half a paging change can still get wrong.
|
||||
|
||||
A verdict sits BEYOND the first page — 60 ordinary rows with it inserted at index 55 — so only a
|
||||
walk that continues past page 1 can find it. Under the pre-#763 code the exemption would still
|
||||
have been repaired here, but for the wrong reason (rows merely EXISTED on page 2), which is why
|
||||
this test is paired with the one above rather than standing alone: together they separate
|
||||
"actually read page 2" from "repaired on any uncertainty".
|
||||
|
||||
SCOPE, stated because the double is ordering-blind: this proves WALK COMPLETENESS, not that a real
|
||||
raced verdict would otherwise be missed. The live endpoint serves `created_unix DESC`, so a row
|
||||
created inside the write window is among the newest and lands on page 1. The walk exists so that
|
||||
the one fail-toward-SUCCESS path does not rest on that undocumented ordering.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="verdict-on-page-2")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"the exemption was posted and left standing even though the status history ran past page 1, so "
|
||||
f"a raced verdict beyond it would be buried. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
"a human verdict sitting on page 2 of the status history was not detected, so the exemption "
|
||||
f"stands over a rejection nobody read. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
@@ -3539,6 +3808,388 @@ def test_a_status_history_RUNNING_PAST_PAGE_1_repairs_rather_than_leaving_green(
|
||||
assert seq[1]["description"] == "Human verdict raced this exemption write — re-post the verdict", (
|
||||
f"repaired, but not to the sentinel, so the fixed point is broken: {seq[1]}"
|
||||
)
|
||||
# It must be reported as a REAL overwrite, not as uncertainty — the two now say different things
|
||||
# in the log, and a verdict genuinely found on page 2 is the case that earns the strong wording.
|
||||
assert "was overwritten" in (r.stdout + r.stderr), (
|
||||
f"repaired, but the log did not report an overwritten verdict:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_partial_mark_is_SAFE_because_the_newest_rows_are_on_page_1(tmp_path):
|
||||
"""The partial-mark fallback's safety is a claim ABOUT THE ORDERING, so it is measured under the
|
||||
real one — this is the only order-faithful fixture here.
|
||||
|
||||
The fallback exists because refusing a mark skips the post-write check entirely, which is a
|
||||
fail-open. It is safe only because the server serves `created_unix DESC` and ids are monotonic
|
||||
with `created_at`: page 1 therefore carries the true maximum, so a walk that fails on page 2 has
|
||||
still seen it.
|
||||
|
||||
The history holds a PRE-EXISTING base-mismatched verdict at id 7055 — older than the newest row,
|
||||
so nothing it does raced this write. Under DESC the salvaged mark is 7059 and that row is below
|
||||
it: the exemption correctly STANDS. Under ASC (`sort=highestindex`, withdrawn in round 4) page 1
|
||||
would be 7000..7049, the mark 7049, and that same untouched row would test as NEWER than the mark
|
||||
— a sticky repair on a head nothing raced, which is exactly #761.
|
||||
|
||||
So re-adding the sort parameter reddens this test by BEHAVIOUR, not merely by the structural
|
||||
assertion in `test_the_walk_does_not_request_a_SORT_order`.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="premark-page2-error-desc")
|
||||
assert r.returncode == 0, r.stderr
|
||||
log = r.stdout + r.stderr
|
||||
# This pins that the walk really was INCOMPLETE — it does not prove the fallback ran, because the
|
||||
# warning is emitted by a different condition than the one that salvages the mark. Deleting the
|
||||
# fallback leaves this line green; the sibling test
|
||||
# `test_a_PRE_WRITE_paging_failure_still_yields_a_usable_high_water_mark` is what reddens then.
|
||||
assert "taking the high-water mark over the" in log, (
|
||||
f"the pre-write walk completed, so this test is not exercising a partial mark at all:\n{r.stdout[-900:]}"
|
||||
)
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 1, (
|
||||
"a PRE-EXISTING verdict was read as newer than the salvaged mark, so the exemption was "
|
||||
f"repaired away on a head nothing raced. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}"
|
||||
assert "was overwritten" not in log, (
|
||||
f"nothing raced this write, but the log claims a verdict was overwritten:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_an_EMPTY_post_write_history_is_not_evidence_that_nothing_raced(tmp_path):
|
||||
"""A well-formed response that cannot be true must not be trusted, and this one is free to reach.
|
||||
|
||||
The walk terminates on an empty page — correct BEFORE the write, where a head nothing has posted
|
||||
to genuinely has no statuses. AFTER the write it is impossible: this job has just POSTed, and
|
||||
`/statuses/{sha}` returns one row per POST, so "no statuses exist" contradicts a write that
|
||||
succeeded. Nothing retries it either, because the body is valid JSON of the right type.
|
||||
|
||||
Accepting it yields `raced=0` from a list that cannot be real, on the one path whose failure
|
||||
direction is toward SUCCESS — and silently, since the walk itself reports success. Whether a
|
||||
verdict actually raced this particular write is not the point and is not modelled here: the
|
||||
response is not evidence either way, so concluding "nothing raced" from it is unsound regardless.
|
||||
|
||||
This is NOT the withdrawn currency witness. That asked "is there ANY row above the mark", which an
|
||||
unrelated newer row satisfied while the rejection stayed hidden. This asks only whether the list is
|
||||
EMPTY — a state no unrelated row can produce and no ordering can disguise.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="null-page1-after-post")
|
||||
assert r.returncode == 0, r.stderr
|
||||
log = r.stdout + r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"an empty post-write history was accepted as proof that nothing raced this write, so an "
|
||||
f"exemption stands on a head whose write window was never inspected. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
)
|
||||
assert "came back empty" in log, (
|
||||
f"repaired, but the impossible empty read was not reported as the reason:\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert "was overwritten" not in log, (
|
||||
f"an unverifiable read was reported as an overwritten human verdict:\n{r.stdout[-900:]}"
|
||||
)
|
||||
# THE REASON IS PART OF THE FIX, so it is asserted POSITIVELY. An absence assertion alone leaves
|
||||
# `raced_why` unpinned — any string at all, including a placeholder, satisfies it while the
|
||||
# operator gets a sticky sentinel explained by nothing. The em-dash and full stop discriminate the
|
||||
# `::error::` reason from the `::warning::` text, which continues "…, which cannot be true".
|
||||
assert "— the status history came back empty after this job posted to it." in log, (
|
||||
"the repair did not report its own reason; the ::error:: carries whatever `raced_why` held "
|
||||
f"and nothing pins it:\n{r.stdout[-900:]}"
|
||||
)
|
||||
# The walk COMPLETED here, on a validated terminator — the answer was impossible, not unreadable.
|
||||
# Borrowing the unreadable-history wording would send an operator staring at a sticky sentinel
|
||||
# looking for an API failure that never happened.
|
||||
assert "could not be read completely" not in log, (
|
||||
"an empty-but-complete read was reported as an incomplete one, which points at the wrong "
|
||||
f"cause:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_NULL_page_terminates_the_walk_as_an_empty_one(tmp_path):
|
||||
"""`/statuses/{sha}` returns `[]` past the end today; `/issues/{n}/timeline` returns bare `null`.
|
||||
|
||||
An array-only gate on the timeline was ersatztv#751: the walk never reached a validated empty
|
||||
page, `rt_ok` was never `yes` for any PR, and the fence withheld EVERY exemption `success` from
|
||||
the day it shipped. Re-adopting that narrowing on this endpoint would be worse, because this
|
||||
walk's failure mode is the STICKY sentinel — every exempt PR would need a human verdict, per head,
|
||||
clearable only by hand.
|
||||
|
||||
Tolerating `null` cannot misread `[]`, so it costs nothing. This asserts it rather than trusting
|
||||
that the endpoint will never change its empty shape.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="null-terminator")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 1, (
|
||||
"a `null` page past the end was read as unreadable rather than as exhaustion, so the walk "
|
||||
f"never validated and the exemption was repaired away. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}"
|
||||
|
||||
|
||||
def test_a_STRING_id_on_a_PRE_EXISTING_row_is_not_read_as_raced(tmp_path):
|
||||
"""The twin of `test_a_STRING_id_cannot_inflate_the_high_water_mark`, on the COMPARISON rather
|
||||
than the maximum — and the twin was live on `main`, one expression away from the fix.
|
||||
|
||||
jq orders strings above every number, so `.id > $since` reads a string id as newer than ANY mark.
|
||||
A pre-existing row carrying one — such as a base-mismatched verdict `read_existing_verdict`
|
||||
deliberately declines to honour — is therefore counted as having raced this write on EVERY run of
|
||||
that PR. Each run posts the sticky sentinel and a false "was overwritten", so the exemption is
|
||||
lost permanently and no re-trigger clears it.
|
||||
|
||||
The row here is genuinely older than the mark (`"3"` against a mark of 9999) and must be invisible.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="string-id-preexisting-row")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 1, (
|
||||
"a PRE-EXISTING row with a string id was read as newer than the high-water mark and treated "
|
||||
f"as a raced verdict, so this PR loses its exemption permanently. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}"
|
||||
assert "was overwritten" not in (r.stdout + r.stderr), (
|
||||
f"nothing raced this write, but the log claims a verdict was overwritten:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_history_with_NO_numeric_ids_SKIPS_the_check_instead_of_marking_zero(tmp_path):
|
||||
"""An empty history and an unreadable one are different answers, and only one of them is 0.
|
||||
|
||||
A head nothing has posted to has no rows and its mark is 0 — correct, every later row is newer.
|
||||
A NON-empty history in which no row carries a numeric id is a schema this job cannot read.
|
||||
Reporting that as 0 claims a mark was established when none was, and every pre-existing row then
|
||||
tests as newer than it.
|
||||
|
||||
The observable difference is the log: the unusable case says so and skips the post-write check.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="no-numeric-ids")
|
||||
assert r.returncode == 0, r.stderr
|
||||
log = r.stdout + r.stderr
|
||||
assert "was not numeric" in log, (
|
||||
"a history carrying no numeric id at all was silently treated as a mark of 0, so the "
|
||||
f"degradation is invisible in the log:\n{r.stdout[-900:]}"
|
||||
)
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 1 and seq[0]["state"] == "success", (
|
||||
f"expected the exemption to be posted with the race check skipped; got {seq}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_history_LONGER_THAN_THE_PAGE_CAP_is_not_treated_as_exhausted(tmp_path):
|
||||
"""The page cap is a refusal, not a terminator.
|
||||
|
||||
Twenty requests of fifty rows cannot validate a history of 1050: the walk never reaches an empty
|
||||
page, so it has NOT established that it saw everything. Treating the cap as exhaustion would hand
|
||||
the post-write check a list it knows is partial and let it conclude "nothing raced" from it — on
|
||||
the one path whose failure direction is toward SUCCESS.
|
||||
|
||||
Only 950 rows can ever be validated, incidentally, because the twentieth request has to be the
|
||||
empty terminator.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="over-cap")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"a history longer than the page cap was treated as fully read, so a raced verdict beyond it "
|
||||
f"would have been concluded absent. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
)
|
||||
assert "could not be read completely" in (r.stdout + r.stderr), (
|
||||
f"repaired, but not reported as an incomplete read:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_the_walk_does_not_request_a_SORT_order(tmp_path):
|
||||
"""A regression guard on a WITHDRAWAL, which otherwise nothing mechanical protects.
|
||||
|
||||
`sort=highestindex` returns index ASCENDING, which would put the OLDEST rows on page 1. The
|
||||
partial-mark fallback then takes its maximum over the oldest rows, leaving every pre-existing row
|
||||
above the mark and read as raced — a spurious STICKY repair, which is the #761 failure this change
|
||||
exists to remove. It was added, measured, and withdrawn for exactly that reason.
|
||||
|
||||
Nothing about the code's behaviour reveals a re-added sort parameter (the tests' double is
|
||||
ordering-blind by design), so the guard is structural: the request must not carry one.
|
||||
"""
|
||||
src = _classify_step()["run"]
|
||||
# The REQUEST LINES ONLY — comments are excluded deliberately, because the withdrawal note inside
|
||||
# `page_statuses` names the parameter in order to explain why it is not used. A guard that reads
|
||||
# prose would fire on its own documentation.
|
||||
requests = [line for line in src.splitlines() if "statuses/$SHA?" in line and not line.lstrip().startswith("#")]
|
||||
assert requests, (
|
||||
"no `/statuses/{sha}` request line found; the walk has been reshaped, so re-point this guard "
|
||||
"rather than letting the sort-order assertion below pass vacuously"
|
||||
)
|
||||
offenders = [line.strip() for line in requests if "sort=" in line]
|
||||
assert not offenders, (
|
||||
"the status-history request carries a sort order again. ASC ordering puts the OLDEST rows on "
|
||||
"page 1, which inverts the partial-mark fallback into a spurious sticky repair — the #761 "
|
||||
f"failure this change removes. See the withdrawal note in `page_statuses`. Offending: {offenders}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_STRING_id_cannot_inflate_the_high_water_mark(tmp_path):
|
||||
"""jq sorts strings above every number, so one schema-corrupt id silently blinds the race check.
|
||||
|
||||
`max` over raw ids returns `"99999"` rather than the largest real id. `jq -r` then renders it as
|
||||
`99999`, which sails through the `*[!0-9]*` numeric gate, and the mark is set far above anything
|
||||
that exists. Every subsequent row — including a genuine human rejection racing the write — tests
|
||||
as OLDER than the mark and is invisible, so the exemption stands over it.
|
||||
|
||||
It fails toward SUCCESS and needs no attacker: one corrupt row is enough. So the mark is taken
|
||||
over numeric ids only, and a non-numeric id is excluded rather than coerced.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="string-id-inflates-mark")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"a string id inflated the high-water mark, so the raced verdict tested as older than it and "
|
||||
f"the exemption stands over a human rejection. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_TRANSIENT_page_failure_is_retried_and_the_walk_still_completes(tmp_path):
|
||||
"""The retry clause, which was unproven code until this fixture existed.
|
||||
|
||||
Every other error mode here fails on EVERY attempt, so a one-shot walk and a retrying walk behave
|
||||
identically against them — mutating `for try in 1 2` to `for try in 1` reddened nothing. That
|
||||
matters because the retry is the stated reason failing closed is affordable: without it, one
|
||||
momentary blip costs the head its exemption permanently, since the repair sentinel is sticky.
|
||||
|
||||
Here page 2 fails the first attempt of each logical read and succeeds on the retry. The history
|
||||
carries no verdict, so the correct outcome is a completed walk and a STANDING exemption.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="flaky-page2")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 1, (
|
||||
"a transient page failure was not retried, so the walk gave up and the exemption was repaired "
|
||||
f"away on a head that nothing raced. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success", f"expected the exemption to stand; got {seq}"
|
||||
|
||||
|
||||
def test_a_PRE_WRITE_read_that_returns_NOTHING_abandons_the_mark_and_says_so(tmp_path):
|
||||
"""The boundary of the partial-list fallback, pinned so the remaining fail-open is explicit.
|
||||
|
||||
A partial list still yields a usable mark. A read that returns NO rows at all cannot: there is
|
||||
nothing to take a maximum over, so `max_id_before` stays -1 and the post-write race check is
|
||||
skipped entirely — the exemption is posted with nothing verifying it afterwards.
|
||||
|
||||
That is the pre-existing gap tracked as ersatztv#849, deliberately unchanged here, and it is
|
||||
asserted rather than left implicit so the degradation is visible in the log and a future change
|
||||
that widens or closes it has to come past this test.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="premark-page1-error")
|
||||
assert r.returncode == 0, r.stderr
|
||||
log = r.stdout + r.stderr
|
||||
assert "Could not establish a status high-water mark" in log, (
|
||||
f"the mark was abandoned silently, with no record of the degradation:\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert "taking the high-water mark over the" not in log, (
|
||||
f"a mark was salvaged from an empty read, which there is nothing to compute:\n{r.stdout[-900:]}"
|
||||
)
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 1 and seq[0]["state"] == "success", (
|
||||
f"expected the exemption to be posted with the race check skipped; got {seq}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_MALFORMED_creator_row_does_not_kill_the_job_after_the_green_is_posted(tmp_path):
|
||||
"""A row whose `creator` is not an object used to be fatal, at the worst possible moment.
|
||||
|
||||
`.creator != null and .creator.login` hard-errors in jq on any non-object creator; jq exits 5 and
|
||||
under `set -euo pipefail` the assignment takes the step down. That happens AFTER the exemption
|
||||
`success` has been posted and BEFORE the repair is attempted, so one schema-corrupt row leaves a
|
||||
green standing on a head that carries a genuine human rejection — and the job reports failure in a
|
||||
way that looks like an unrelated infrastructure error.
|
||||
|
||||
The fix type-tests `creator` before indexing it, so the malformed row is dropped from the count
|
||||
while the real verdict beside it is still counted and still repaired.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="malformed-creator-beside-verdict")
|
||||
assert r.returncode == 0, f"the step died instead of skipping a malformed row: {r.stderr[-1200:]}"
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"a malformed `creator` row suppressed the repair, leaving the exemption green over the real "
|
||||
f"verdict beside it. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
)
|
||||
# THE REASON, not just the outcome — this is what makes the type guard individually provable.
|
||||
# Three clauses can each rescue this fixture (the type test, the `|| raced=""` guard, and the
|
||||
# fail-closed unusable-count branch), so the repair alone cannot tell them apart. Only the type
|
||||
# test lets the REAL verdict beside the malformed row be counted as a genuine human race; without
|
||||
# it the count comes back empty and the repair is reported as an unusable count instead.
|
||||
assert "was overwritten" in (r.stdout + r.stderr), (
|
||||
"the malformed row suppressed the real verdict beside it — repaired, but as an unverifiable "
|
||||
f"read rather than as the human rejection it is:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_verdict_BEYOND_A_SHORT_PAGE_is_still_found(tmp_path):
|
||||
""" "Terminate only on a validated EMPTY page, never on a short one" — the rule that is invisible
|
||||
against a faithful double.
|
||||
|
||||
On the real endpoint a short page IS the last page (measured: 50, 50, 14, `[]`), so an
|
||||
implementation that stops at the first short page is indistinguishable from a correct one, and
|
||||
every other test here would pass against it. The only way to observe the property is to model what
|
||||
it actually guards: a TRUNCATED response. This stub serves 50 rows, then a short page of 10, then
|
||||
a third page carrying the raced verdict.
|
||||
|
||||
A walk that treats the short page as exhaustion never reads page 3, misses the verdict, and leaves
|
||||
the exemption green over a human rejection.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="verdict-after-short-page")
|
||||
assert r.returncode == 0, r.stderr
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"a raced verdict beyond a SHORT page was not found, so the walk stopped on a short page "
|
||||
f"instead of on a validated empty one. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
)
|
||||
assert "was overwritten" in (r.stdout + r.stderr), (
|
||||
f"repaired, but not reported as a found verdict:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
def test_a_PRE_WRITE_paging_failure_still_yields_a_usable_high_water_mark(tmp_path):
|
||||
"""The fail-open a paging change can introduce while fixing one (ersatztv#763).
|
||||
|
||||
The high-water mark gates the post-write race check entirely: `max_id_before=-1` skips it, so a
|
||||
human rejection landing in the write window is neither detected nor repaired. Before paging, only
|
||||
a failure of the single page-1 request could reach that. Requiring a COMPLETE walk for the mark
|
||||
would newly route a page-2 hiccup, an over-cap history, or one malformed id on a later page into
|
||||
the same hole — a WIDER fail-open than the bug being fixed.
|
||||
|
||||
So a partial list still yields a mark. It can only be LOWER than the true maximum, which makes the
|
||||
check more eager, never blinder.
|
||||
|
||||
Here page 2 fails on the PRE-write read only; the post-write read pages cleanly and carries a raced
|
||||
verdict. With the mark salvaged from page 1 the verdict is above it and the exemption is repaired.
|
||||
With `max_id_before=-1` the check never runs and the rejection stays green.
|
||||
"""
|
||||
posted, r = _run_classify(tmp_path, _emitting("docs/a.md"), history_mode="premark-page2-error")
|
||||
assert r.returncode == 0, r.stderr
|
||||
log = r.stdout + r.stderr
|
||||
assert "taking the high-water mark over the" in log, (
|
||||
f"the partial-list fallback did not run, so the mark was not salvaged:\n{r.stdout[-900:]}"
|
||||
)
|
||||
seq = _posted_sequence(tmp_path)
|
||||
assert len(seq) == 2, (
|
||||
"the pre-write read could not be paged completely, the mark was abandoned, and the post-write "
|
||||
f"race check was skipped — leaving a raced rejection green. Posts: {seq}\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
@@ -3567,6 +4218,17 @@ def test_an_UNREADABLE_history_page_2_also_repairs_rather_than_leaving_green(tmp
|
||||
assert seq[0]["state"] == "success" and seq[1]["state"] == "pending", (
|
||||
f"expected an exemption then a repair to pending; got {seq}"
|
||||
)
|
||||
# THE REASON IS PART OF THE FIX, so it is asserted. #761's stall was undiagnosable precisely
|
||||
# because an uncertainty repair reported itself as an overwritten human verdict. Without these
|
||||
# two the `raced_why` branch is unpinned — forcing it back to `human` reddens nothing.
|
||||
log = r.stdout + r.stderr
|
||||
assert "could not be read completely" in log, (
|
||||
f"repaired, but not reported as an incomplete read:\n{r.stdout[-900:]}"
|
||||
)
|
||||
assert "was overwritten" not in log, (
|
||||
"an uncertainty repair is reported as an overwritten human verdict — the false ::error:: that "
|
||||
f"made PR #761's stall undiagnosable:\n{r.stdout[-900:]}"
|
||||
)
|
||||
|
||||
|
||||
# --- Workflow token scope (ersatztv#748) -------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user