fix: exhaust Forgejo timeline pagination #254
3 changed files with 143 additions and 19 deletions
3
changelog.d/240.md
Normal file
3
changelog.d/240.md
Normal file
|
|
@ -0,0 +1,3 @@
|
||||||
|
### Fixed
|
||||||
|
|
||||||
|
- Read Forgejo timelines to exhaustion so busy issues retain their newest label events despite dishonest total-count headers (#240).
|
||||||
|
|
@ -84,7 +84,7 @@ forgejo_page_url() {
|
||||||
esac
|
esac
|
||||||
}
|
}
|
||||||
|
|
||||||
# forge_api [--paginate] <endpoint> [--jq <expr>]
|
# forge_api [--paginate | --paginate-exhaustive] <endpoint> [--jq <expr>]
|
||||||
#
|
#
|
||||||
# --paginate walks page= until a short page, then PROVES the walk was
|
# --paginate walks page= until a short page, then PROVES the walk was
|
||||||
# complete by comparing what it collected against the server's declared
|
# complete by comparing what it collected against the server's declared
|
||||||
|
|
@ -94,11 +94,26 @@ forgejo_page_url() {
|
||||||
# make the completeness check compare null to a number — the guard itself
|
# make the completeness check compare null to a number — the guard itself
|
||||||
# degrading silently, which is the failure class re-entering through the
|
# degrading silently, which is the failure class re-entering through the
|
||||||
# door built to stop it.
|
# door built to stop it.
|
||||||
|
#
|
||||||
|
# --paginate-exhaustive is the narrow alternative for an endpoint whose
|
||||||
|
# x-total-count is known not to describe the collection. It proves completion
|
||||||
|
# by reading through the first short page and never consults that header.
|
||||||
forge_api() {
|
forge_api() {
|
||||||
local paginate=false endpoint="" jqexpr="" have_jq=false
|
local paginate=false paginate_exhaustive=false method=GET endpoint="" jqexpr="" have_jq=false
|
||||||
while [ $# -gt 0 ]; do
|
while [ $# -gt 0 ]; do
|
||||||
case "$1" in
|
case "$1" in
|
||||||
--paginate) paginate=true ;;
|
--paginate) paginate=true ;;
|
||||||
|
--paginate-exhaustive) paginate_exhaustive=true ;;
|
||||||
|
-X | --method)
|
||||||
|
[ "$#" -ge 2 ] || { echo "forge_api: $1 requires a value" >&2; return 1; }
|
||||||
|
method="$2"
|
||||||
|
shift
|
||||||
|
;;
|
||||||
|
-X?*) method="${1#-X}" ;;
|
||||||
|
--method=*)
|
||||||
|
method="${1#*=}"
|
||||||
|
[ -n "$method" ] || { echo "forge_api: --method requires a value" >&2; return 1; }
|
||||||
|
;;
|
||||||
--jq) jqexpr="$2"; have_jq=true; shift ;;
|
--jq) jqexpr="$2"; have_jq=true; shift ;;
|
||||||
-*) ;;
|
-*) ;;
|
||||||
*) [ -n "$endpoint" ] || endpoint="$1" ;;
|
*) [ -n "$endpoint" ] || endpoint="$1" ;;
|
||||||
|
|
@ -106,6 +121,14 @@ forge_api() {
|
||||||
shift
|
shift
|
||||||
done
|
done
|
||||||
[ -n "$endpoint" ] || { echo "forge_api: endpoint required" >&2; return 1; }
|
[ -n "$endpoint" ] || { echo "forge_api: endpoint required" >&2; return 1; }
|
||||||
|
if [ "$paginate" = true ] && [ "$paginate_exhaustive" = true ]; then
|
||||||
|
echo "forge_api: --paginate and --paginate-exhaustive are mutually exclusive" >&2
|
||||||
|
return 1
|
||||||
|
fi
|
||||||
|
if { [ "$paginate" = true ] || [ "$paginate_exhaustive" = true ]; } && [ "$method" != GET ]; then
|
||||||
|
echo "forge_api: pagination is available only for GET requests" >&2
|
||||||
|
return 1
|
||||||
|
fi
|
||||||
|
|
||||||
local base token
|
local base token
|
||||||
base="$(forgejo_api_base)" || return 1
|
base="$(forgejo_api_base)" || return 1
|
||||||
|
|
@ -116,7 +139,7 @@ forge_api() {
|
||||||
# shellcheck disable=SC2064 # the paths are fixed at trap time on purpose
|
# shellcheck disable=SC2064 # the paths are fixed at trap time on purpose
|
||||||
trap "rm -f '$hdr' '$body'" RETURN
|
trap "rm -f '$hdr' '$body'" RETURN
|
||||||
|
|
||||||
if [ "$paginate" = false ]; then
|
if [ "$paginate" = false ] && [ "$paginate_exhaustive" = false ]; then
|
||||||
if ! curl -sS -D "$hdr" -o "$body" \
|
if ! curl -sS -D "$hdr" -o "$body" \
|
||||||
-H "Authorization: token $token" -H 'Accept: application/json' \
|
-H "Authorization: token $token" -H 'Accept: application/json' \
|
||||||
"$base/$endpoint"; then
|
"$base/$endpoint"; then
|
||||||
|
|
@ -142,21 +165,23 @@ forge_api() {
|
||||||
fi
|
fi
|
||||||
forgejo_http_ok "$hdr" "GET $endpoint" || return 1
|
forgejo_http_ok "$hdr" "GET $endpoint" || return 1
|
||||||
|
|
||||||
# Re-read on EVERY page, not once (#4712). A board that changes size
|
if [ "$paginate_exhaustive" = false ]; then
|
||||||
# under the walk was invisible: page 1 declaring 4 and page 2 declaring
|
# Re-read on EVERY page, not once (#4712). A board that changes size
|
||||||
# 9 stopped at 4 believing itself whole. A moving total means the read
|
# under the walk was invisible: page 1 declaring 4 and page 2 declaring
|
||||||
# cannot have been atomic, so it is refused rather than reconciled.
|
# 9 stopped at 4 believing itself whole. A moving total means the read
|
||||||
local page_total
|
# cannot have been atomic, so it is refused rather than reconciled.
|
||||||
page_total="$(forgejo_total_count "$hdr")" || return 1
|
local page_total
|
||||||
if [ -z "$total" ]; then
|
page_total="$(forgejo_total_count "$hdr")" || return 1
|
||||||
total="$page_total"
|
if [ -z "$total" ]; then
|
||||||
elif [ "$page_total" != "$total" ]; then
|
total="$page_total"
|
||||||
cat >&2 <<EOF
|
elif [ "$page_total" != "$total" ]; then
|
||||||
|
cat >&2 <<EOF
|
||||||
forge_api: the declared total for '$endpoint' changed between pages — $total then $page_total (#188).
|
forge_api: the declared total for '$endpoint' changed between pages — $total then $page_total (#188).
|
||||||
The collection moved under the walk, so no page set can be proven whole.
|
The collection moved under the walk, so no page set can be proven whole.
|
||||||
Refusing rather than reconciling a board that is already out of date.
|
Refusing rather than reconciling a board that is already out of date.
|
||||||
EOF
|
EOF
|
||||||
return 1
|
return 1
|
||||||
|
fi
|
||||||
fi
|
fi
|
||||||
|
|
||||||
pagejson="$(cat "$body")"
|
pagejson="$(cat "$body")"
|
||||||
|
|
@ -175,13 +200,17 @@ EOF
|
||||||
[ "$n" -gt 0 ] || break
|
[ "$n" -gt 0 ] || break
|
||||||
all="$(jq -s '.[0] + .[1]' <<<"$all"$'\n'"$pagejson")"
|
all="$(jq -s '.[0] + .[1]' <<<"$all"$'\n'"$pagejson")"
|
||||||
got=$((got + n))
|
got=$((got + n))
|
||||||
[ "$got" -lt "$total" ] || break
|
if [ "$paginate_exhaustive" = true ]; then
|
||||||
|
[ "$n" -eq 50 ] || break
|
||||||
|
else
|
||||||
|
[ "$got" -lt "$total" ] || break
|
||||||
|
fi
|
||||||
page=$((page + 1))
|
page=$((page + 1))
|
||||||
done
|
done
|
||||||
|
|
||||||
# The assert. A short read here is the silent-truncation bug arriving by
|
# The assert. A short read here is the silent-truncation bug arriving by
|
||||||
# another route, so it is fatal rather than a warning.
|
# another route, so it is fatal rather than a warning.
|
||||||
if [ "$got" -ne "$total" ]; then
|
if [ "$paginate_exhaustive" = false ] && [ "$got" -ne "$total" ]; then
|
||||||
cat >&2 <<EOF
|
cat >&2 <<EOF
|
||||||
forge_api: incomplete gather for '$endpoint' — collected $got of $total declared (#188).
|
forge_api: incomplete gather for '$endpoint' — collected $got of $total declared (#188).
|
||||||
Refusing rather than reconciling a partial board: a sweep over part of the
|
Refusing rather than reconciling a partial board: a sweep over part of the
|
||||||
|
|
@ -561,7 +590,13 @@ forge_request_reviewer() {
|
||||||
# empty timeline — the two states the ruling ladder must tell apart (#4853).
|
# empty timeline — the two states the ruling ladder must tell apart (#4853).
|
||||||
forge_timeline() {
|
forge_timeline() {
|
||||||
local n="${1:?forge_timeline: number required}" raw
|
local n="${1:?forge_timeline: number required}" raw
|
||||||
raw="$(forge_api --paginate "repos/$REPO/issues/$n/timeline")" || return 1
|
# Measured on this instance: limit=10 reports x-total-count=10 and limit=50
|
||||||
|
# reports 50, while crew!96 held 151 events and strict pagination returned
|
||||||
|
# only its first 50. No other measured endpoint echoes its page size this
|
||||||
|
# way. Timelines are append-only, so exhaustion can include concurrent new
|
||||||
|
# events but cannot create a deletion hole; that is why only this call site
|
||||||
|
# may bypass the header-bound completeness proof (#240).
|
||||||
|
raw="$(forge_api --paginate-exhaustive "repos/$REPO/issues/$n/timeline")" || return 1
|
||||||
jq '
|
jq '
|
||||||
[.[]
|
[.[]
|
||||||
| select(.type == "label")
|
| select(.type == "label")
|
||||||
|
|
|
||||||
|
|
@ -746,6 +746,7 @@ check "no api base refuses" 1 "cannot reach the forge" \
|
||||||
# .user.login -> .actor.login. Mutation-verified: collapsing add/remove or
|
# .user.login -> .actor.login. Mutation-verified: collapsing add/remove or
|
||||||
# emitting .user instead of .actor each reds its own case (#4853).
|
# emitting .user instead of .actor each reds its own case (#4853).
|
||||||
timeline_stub() {
|
timeline_stub() {
|
||||||
|
: >"$timeline_calls"
|
||||||
# shellcheck disable=SC2317 # invoked indirectly, by forge_api
|
# shellcheck disable=SC2317 # invoked indirectly, by forge_api
|
||||||
curl() {
|
curl() {
|
||||||
local hdr="" out="" url=""
|
local hdr="" out="" url=""
|
||||||
|
|
@ -753,14 +754,44 @@ timeline_stub() {
|
||||||
case "$1" in -D) hdr="$2"; shift ;; -o) out="$2"; shift ;; -H) shift ;; *) url="$1" ;; esac
|
case "$1" in -D) hdr="$2"; shift ;; -o) out="$2"; shift ;; -H) shift ;; *) url="$1" ;; esac
|
||||||
shift
|
shift
|
||||||
done
|
done
|
||||||
printf 'HTTP/1.1 200 OK\r\nX-Total-Count: %s\r\n\r\n' "${FAKE_TL_N:-2}" >"$hdr"
|
printf '%s\n' "$url" >>"$timeline_calls"
|
||||||
|
local page=1 page_body="$FAKE_TIMELINE" page_total="${FAKE_TL_N:-2}"
|
||||||
|
case "$url" in *page=*) page="${url##*page=}"; page="${page%%&*}" ;; esac
|
||||||
|
if [ "${#FAKE_TL_PAGES[@]}" -gt 0 ]; then
|
||||||
|
if [ "$page" -le "${#FAKE_TL_PAGES[@]}" ]; then
|
||||||
|
page_body="${FAKE_TL_PAGES[$((page - 1))]}"
|
||||||
|
else
|
||||||
|
page_body='[]'
|
||||||
|
fi
|
||||||
|
page_total="$(jq 'length' <<<"$page_body")"
|
||||||
|
fi
|
||||||
|
{
|
||||||
|
printf 'HTTP/1.1 200 OK\r\n'
|
||||||
|
[ "${FAKE_TL_HEADERS:-yes}" = no ] || printf 'X-Total-Count: %s\r\n' "$page_total"
|
||||||
|
printf '\r\n'
|
||||||
|
} >"$hdr"
|
||||||
case "$url" in
|
case "$url" in
|
||||||
*timeline*) printf '%s' "$FAKE_TIMELINE" >"$out" ;;
|
*timeline*) printf '%s' "$page_body" >"$out" ;;
|
||||||
*) printf '[]' >"$out" ;;
|
*) printf '[]' >"$out" ;;
|
||||||
esac
|
esac
|
||||||
return 0
|
return 0
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
timeline_page() {
|
||||||
|
jq -nc --argjson first "$1" --argjson count "$2" '
|
||||||
|
[range($first; $first + $count)
|
||||||
|
| {
|
||||||
|
type: "label",
|
||||||
|
body: "1",
|
||||||
|
user: {login: "setter"},
|
||||||
|
label: {name: "needs-ruling"},
|
||||||
|
created_at: ("event-" + tostring)
|
||||||
|
}]
|
||||||
|
'
|
||||||
|
}
|
||||||
|
timeline_calls="$TMP/timeline_calls"
|
||||||
|
FAKE_TL_PAGES=()
|
||||||
|
FAKE_TL_HEADERS=yes
|
||||||
FAKE_TIMELINE='[
|
FAKE_TIMELINE='[
|
||||||
{"type":"label","body":"1","user":{"login":"setter"},"label":{"name":"needs-ruling"},"created_at":"2026-08-02T14:58:13Z"},
|
{"type":"label","body":"1","user":{"login":"setter"},"label":{"name":"needs-ruling"},"created_at":"2026-08-02T14:58:13Z"},
|
||||||
{"type":"label","body":"","user":{"login":"setter"},"label":{"name":"needs-ruling"},"created_at":"2026-08-02T15:22:22Z"},
|
{"type":"label","body":"","user":{"login":"setter"},"label":{"name":"needs-ruling"},"created_at":"2026-08-02T15:22:22Z"},
|
||||||
|
|
@ -777,6 +808,61 @@ check "forge_timeline drops non-label events" 0 "" \
|
||||||
test "$(jq '[.[] | select(.event == null or .event == "")] | length' <<<"$tl")" = 0
|
test "$(jq '[.[] | select(.event == null or .event == "")] | length' <<<"$tl")" = 0
|
||||||
check "forge_timeline uses .actor.login, not a bare .user" 0 "" \
|
check "forge_timeline uses .actor.login, not a bare .user" 0 "" \
|
||||||
jq -e 'all(.[]; has("actor") and (.user|not))' <<<"$tl" >/dev/null
|
jq -e 'all(.[]; has("actor") and (.user|not))' <<<"$tl" >/dev/null
|
||||||
|
|
||||||
|
# Forgejo's timeline endpoint lies consistently: x-total-count echoes the
|
||||||
|
# current page size. With 151 events its pages declare 50, 50, 50 and 1, so
|
||||||
|
# strict pagination stops successfully after page 1 and drops the newest 101
|
||||||
|
# events. Exhaustion is safe only here because timelines are append-only.
|
||||||
|
FAKE_TL_PAGES=(
|
||||||
|
"$(timeline_page 1 50)"
|
||||||
|
"$(timeline_page 51 50)"
|
||||||
|
"$(timeline_page 101 50)"
|
||||||
|
"$(timeline_page 151 1)"
|
||||||
|
)
|
||||||
|
timeline_stub
|
||||||
|
tl="$(REPO=o/r forge_timeline 188)"
|
||||||
|
check "forge_timeline exhausts all pages despite per-page total headers" 0 "" \
|
||||||
|
test "$(jq 'length' <<<"$tl")" = 151
|
||||||
|
check "forge_timeline retains the newest event beyond page one" 0 "" \
|
||||||
|
jq -e 'any(.[]; .created_at == "event-151")' <<<"$tl" >/dev/null
|
||||||
|
check "a short final page terminates without an extra empty-page read" 0 "" \
|
||||||
|
test "$(wc -l <"$timeline_calls")" = 4
|
||||||
|
|
||||||
|
# A collection exactly divisible by the page size needs one final empty read;
|
||||||
|
# stopping after the second full page cannot prove exhaustion.
|
||||||
|
FAKE_TL_PAGES=("$(timeline_page 1 50)" "$(timeline_page 51 50)")
|
||||||
|
timeline_stub
|
||||||
|
exhaustive_count="$(REPO=o/r forge_api --paginate-exhaustive 'repos/o/r/issues/188/timeline' --jq 'length')"
|
||||||
|
check "exhaustive pagination terminates after an empty page" 0 "" \
|
||||||
|
test "$exhaustive_count" = 100
|
||||||
|
check "an exactly-full exhaustive gather reads the empty third page" 0 "" \
|
||||||
|
test "$(wc -l <"$timeline_calls")" = 3
|
||||||
|
|
||||||
|
# The exhaustive path's completeness proof is the short page itself; it must
|
||||||
|
# never consult the endpoint's missing or dishonest total header.
|
||||||
|
FAKE_TL_PAGES=("$(timeline_page 1 50)" "$(timeline_page 51 1)")
|
||||||
|
FAKE_TL_HEADERS=no
|
||||||
|
timeline_stub
|
||||||
|
check "exhaustive pagination needs no x-total-count header" 0 "" \
|
||||||
|
eq 51 forge_api --paginate-exhaustive 'repos/o/r/issues/188/timeline' --jq 'length'
|
||||||
|
check "the same missing-header fixture is still refused by strict pagination" 1 \
|
||||||
|
"did not send x-total-count" \
|
||||||
|
forge_api --paginate 'repos/o/r/issues/188/timeline' --jq 'length'
|
||||||
|
FAKE_TL_HEADERS=yes
|
||||||
|
|
||||||
|
check "strict and exhaustive pagination are mutually exclusive" 1 "mutually exclusive" \
|
||||||
|
forge_api --paginate --paginate-exhaustive 'repos/o/r/issues/188/timeline'
|
||||||
|
check "exhaustive pagination refuses a non-GET method" 1 "GET" \
|
||||||
|
forge_api --paginate-exhaustive -X POST 'repos/o/r/issues/188/timeline'
|
||||||
|
check "exhaustive pagination refuses compact -XPOST too" 1 "GET" \
|
||||||
|
forge_api --paginate-exhaustive -XPOST 'repos/o/r/issues/188/timeline'
|
||||||
|
check "exhaustive pagination refuses --method=POST too" 1 "GET" \
|
||||||
|
forge_api --paginate-exhaustive --method=POST 'repos/o/r/issues/188/timeline'
|
||||||
|
check "the exhaustive flag has exactly one production call site" 0 "" \
|
||||||
|
test "$(grep -c 'paginate-exhaustive' "$ROOT/lib/forge-forgejo.sh")" = 5
|
||||||
|
check "only forge_timeline invokes exhaustive pagination" 0 "" \
|
||||||
|
test "$(grep -c 'forge_api --paginate-exhaustive' "$ROOT/lib/forge-forgejo.sh")" = 1
|
||||||
|
|
||||||
# Unreadable: curl fails. Status must surface through forge_timeline itself
|
# Unreadable: curl fails. Status must surface through forge_timeline itself
|
||||||
# (not a later jq), or the ruling ladder invents a verdict on a half-read.
|
# (not a later jq), or the ruling ladder invents a verdict on a half-read.
|
||||||
# shellcheck disable=SC2317
|
# shellcheck disable=SC2317
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue