feat(api): Forgejo API resilience — timeouts + retry + pagination (#334) #404
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!404
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/334-forgejo-api-resilience"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Forgejo API layer resilience per operator elevation 2026-07-05 to v1.0.0 must-fix. Closes #334.
scripts/lib/forgejo-api.shhad no connection timeout, total timeout, retries, backoff, 429 handling, 5xx retry, or pagination. Fine on small repos; breaks on active OSS at scale — silently, which is the correctness class this fixes.Correctness-under-scale (not just resilience-under-load)
The pre-fix
forgejo_list_tagsused?limit=50;forgejo_find_pr_by_headhad no explicit limit. Adopters on active OSS with >50 tags or heavy PR turnover silently received a subset of what they asked for. Rolling-PR detection past the 50-PR mark = silent miss; rc-tag pruning on long histories = incomplete sweep. That's a correctness class, not a robustness class. Adopters get correctness fixes here, not just faster failures.What lands
Timeouts (all curl call sites)
Env-tunable, defaults from the external cold-read recommendation:
FORGEJO_API_CONNECT_TIMEOUT_S=5FORGEJO_API_TIMEOUT_S=30Applied to
forgejo_api_call+ the 2 direct-curl sites (forgejo_get_branch_protection,forgejo_get_release_by_tag).forgejo_api_call_with_retry METHOD ENDPOINT [BODY]base_ms * 2^attempt, capped atFORGEJO_API_RETRY_CAP_S(default 30s — also caps hostile Retry-After)FORGEJO_API_MAX_RETRIES=3,FORGEJO_API_RETRY_BASE_MS=500,FORGEJO_API_RETRY_CAP_S=30FORGEJO_API_RETRY_NO_SLEEP=1so bats runs don't wait real secondsforgejo_api_paginate METHOD ENDPOINT [BODY]?page=N&limit=Muntil a short page arrives ORFORGEJO_API_MAX_PAGES=40cap (2000-item safety bound against a broken server)forgejo_api_call_with_retryper page (list loops are exactly where transient failures compound)&or?correctly)Retrofits
forgejo_list_tags— pre-fix was single?limit=50; now walks pagesforgejo_find_pr_by_head— pre-fix was implicit page-1-only; now walksSubshell-safe side-effect vars (bash idiom worth naming)
The retry wrapper needs to inspect
forgejo_api_call's HTTP status + curl exit code + Retry-After header after the call returns. Naivex=$(forgejo_api_call ...)runs the RHS in a subshell — env-var mutations inside the function are discarded before the wrapper can read them.Fix: redirect stdout to a tmpfile instead of
$(...)capture.forgejo_api_callruns in the current shell, the env vars propagate, the wrapper reads them cleanly, and the response body is read from the tmpfile after.Same class-correct closure as PR#386's printf-into-0600 zero-argv pattern — both are cases where the naive bash approach leaks or loses state, and the disciplined pattern threads through cleanly. Worth naming as codified idiom for bash side-effect+capture.
Side-effect vars:
FORGEJO_LAST_STATUS— HTTP status code, or empty on network errorFORGEJO_LAST_CURL_RC— curl exit code (0 on any HTTP response, non-zero on network error)FORGEJO_LAST_RETRY_AFTER— Retry-After header value if presentTest coverage
tests/forgejo-api-resilience.bats— 17 new tests via PATH-mocked curl:STATUS BODY [RETRY_AFTER]lines consumed one-per-invocation)MAX_RETRIES=0disables entirely, network-error-retriable&, dry-run-single-call-no-walk--connect-timeout+--max-time, env-override honoredExisting
tests/forgejo-api.bats(53 tests): all preserved.Full sweep: 636/636 EXIT=0.
Verification AC (from tracker)
api_call_with_retrywrapper (exponential backoff, honors Retry-After)api_paginatehelper for list endpointsforgejo_list_tags,forgejo_find_pr_by_head) to paginationFiles
scripts/lib/forgejo-api.sh(+~180 lines: retry wrapper + paginate helper + side-effect vars + timeouts)tests/forgejo-api-resilience.bats(17 tests)changelog.d/334.added.md(release-notes fragment)Refs #334 (main tracker, priority/critical v1.0.0 must-fix per operator 2026-07-05), cold-read #315 (parent audit), sibling classes: PR#386 (printf-into-0600 zero-argv — same class-correct-closure shape).
`scripts/lib/forgejo-api.sh` had no connection timeout, total timeout, retries, backoff, 429 handling, 5xx retry, or pagination. Fine on small repos; breaks on active OSS. v1.0.0 signals broad-adoption readiness so this ships in v1.0.0 per operator elevation 2026-07-05. **Timeouts** on every curl call site (forgejo_api_call + the 2 direct- curl sites in forgejo_get_branch_protection + forgejo_get_release_by_tag): --connect-timeout ${FORGEJO_API_CONNECT_TIMEOUT_S:-5} --max-time ${FORGEJO_API_TIMEOUT_S:-30} Defaults match the external cold-read's recommendation. Callers can override via env for high-latency runners. **Side-effect vars** on forgejo_api_call for the retry wrapper to inspect (subshell-safe because the retry wrapper redirects stdout to a tmpfile rather than command-substitution): FORGEJO_LAST_STATUS — HTTP status code, or empty on network err FORGEJO_LAST_CURL_RC — curl exit code (0 on any HTTP response) FORGEJO_LAST_RETRY_AFTER — Retry-After header value if present **forgejo_api_call_with_retry METHOD ENDPOINT [BODY]**: - Retries on: 429, 5xx, curl network errors (curl_rc != 0) - No retry on: 2xx (immediate return), 4xx-except-429 (client error) - Backoff: exponential (base_ms * 2^attempt) OR Retry-After when 429 sent a numeric value; capped at FORGEJO_API_RETRY_CAP_S seconds - Env-tunable: FORGEJO_API_MAX_RETRIES=3, FORGEJO_API_RETRY_BASE_MS=500, FORGEJO_API_RETRY_CAP_S=30 - FORGEJO_API_RETRY_NO_SLEEP=1 is a test-only seam so bats runs don't wait real seconds **forgejo_api_paginate METHOD ENDPOINT [BODY]**: - Walks ?page=N&limit=M until a short page or FORGEJO_API_MAX_PAGES=40 cap (2000-item safety bound against a broken server) - Concatenates JSON arrays via jq - Uses forgejo_api_call_with_retry per page (list loops are exactly where transient failures compound) - Preserves existing query params on the endpoint (adds & or ?) - Dry-run: single call, no walk (existing test contract) **Retrofits**: - forgejo_list_tags — was single ?limit=50 call; now walks pages - forgejo_find_pr_by_head — was implicit page-1-only; now walks Pre-resilience behavior: an active repo with >50 open PRs silently dropped the rolling PR if it sat past page 1, and prune-rc-tags saw only the first 50 tags on a repo with a long-tail rc history. **tests/forgejo-api-resilience.bats** (17 tests): - PATH-mocks curl via $CURL_MOCK_BIN with a scriptable response queue - Side-effect var assertions on 2xx/4xx/429/network-error - Retry: 200-first-try, 500-then-200, 429-then-200, 404-no-retry, 5xx-exhaust-after-max_retries, max_retries=0 disables, network-error-retriable - Paginate: short-page-stops, full-then-short-concatenates, query-string-preserved, dry-run-single-call - Timeouts: argv-contains-flags, env-override-honored Existing forgejo-api.bats (53 tests): all preserved. Full sweep 636/636 EXIT=0. Refs: #334 (main tracker), cold-read #315 (parent audit).Three pre-review hardenings caught by proactive self-probe on Surveyor's named focus areas (32fb): **(2) forgejo_api_paginate: cap-hit on FULL page = fail-loud, not silent-truncate.** Pre-fix loop just exited normally when page > max_pages regardless of whether the last page was full — silently returning the truncated accumulation with exit 0. That IS the exact page-1-only silent- drop bug class this whole helper is meant to close, just at page ${max_pages}+1 instead of page 2. Now tracks last_page_full; fires FATAL banner + returns non-zero when cap fires on a full page. Adopters bump FORGEJO_API_MAX_PAGES explicitly or scope the query narrower as an intentional decision. Fail-loud message names: - The endpoint (which query hit the cap) - Current cap value + items-per-page math - Explicit "silent truncation would re-introduce the page-1-only bug class this helper closes" — so the failure mode is legible even to a first-time adopter hitting it in the wild. **(4) forgejo_api_call_with_retry: idempotent-methods-only retry on 5xx/network-error.** Pre-fix retried ANY method — including POST/PATCH which are NOT idempotent per RFC 9110 §9.2.2. Retrying a POST to /pulls on a 5xx or connection-reset could duplicate the PR (server may have processed part of it before the failure). Now: - 429 -> retry any method (server explicitly said "didn't process") - 5xx / network-error -> retry only GET/HEAD/OPTIONS/PUT/DELETE (per RFC 9110 §9.2.2 idempotent classification) - 2xx -> immediate return - 4xx-except-429 -> no retry (client error) Escape hatch: FORGEJO_API_RETRY_UNSAFE=1 opts POST/PATCH back into 5xx retry for callers who KNOW their write is idempotent (e.g. "create-if-not-exists"). Not first resort. **(5) tmpfile EXIT trap.** Pre-fix cleanup relied on the explicit `rm -f "$out_file"` on each return path. A `set -e` in a caller context, an unforeseen error path in the loop, or a signal interruption could skip cleanup + leak the tmpfile at rest. Added `trap 'rm -f "$out_file"' RETURN` at function entry — guarantees cleanup regardless of exit path. RETURN trap fires on function return (not just process exit), correct for a library helper. **7 new tests** covering all 3 hardenings: - POST 500 -> no retry (idempotency guard) - PATCH 500 -> no retry - POST 429 -> retries (rate-limit is method-safe) - FORGEJO_API_RETRY_UNSAFE=1 opts POST back in - PUT + DELETE 5xx -> retries (idempotent per RFC) - Paginate cap on FULL page -> fail-loud + actionable stderr - Paginate cap on SHORT last page -> success (end proven) Total: 24 resilience tests. Full sweep: 643/643 EXIT=0 (was 636 pre-hardening + 7 new). Refs: release-toolkit#334, Surveyor pre-review focus areas (32fb), sibling class: PR#395 (mechanism-verification-at-source discipline).REQUEST_CHANGES — one must-fix: the safety cap silently truncates (the PR's own bug class, at page 40)
Full mechanism-verification at source, per the ask. The resilience substrate is
well-built and the correctness framing is real — but the pagination fix
reintroduces the exact silent-drop class it exists to kill, at a higher
threshold.
Must-fix —
forgejo_api_paginatesilently truncates at the page capThe loop stops on either a short page (real end) or
page > max_pages— butthose two exits are indistinguishable to the caller:
When every page is full through page 40, the loop exits on the
whilecondition,falls through to the bare
printf, and returns the 2000-item partial arraywith exit 0 — identical to a complete walk. A repo with >2000 tags (a
long-lived project: 2000 releases is reachable) gets silently-truncated results
reported as success.
That's precisely the failure this PR fixes —
list_tags"silently returned page1 only past 50" — moved from 50 to 2000. For a PR whose thesis is "adopters get
correctness fixes, not just resilience," a silent-wrong-result at scale is the
one outcome it can't ship. The cap protecting against a broken infinite-paging
server is right; the cap silently discarding a legitimate >2000 result is the
bug — and the function can't tell the two apart, so the safe default is to surface
it.
Fix: on cap-exhaustion with a full final page, fail loud — return non-zero
with an actionable stderr (
pagination hit safety cap of N pages (~M items); results may be truncated — raise FORGEJO_API_MAX_PAGES). That keeps the DoS boundand eliminates silent-wrong-results, and it matches the codebase's own fail-loud
idiom (
resolve_default_branchlayer-6, the #380 divergence check). Fail-loud vswarn-and-signal is your design call; the must-fix is only that it must not be
silent + exit 0.
This is also untested — none of the 17 bats exercise the cap boundary (grep
for
max_pages/cap/page.4[01]= 0 hits). The green 636/636 sweep passes becausethe boundary is uncovered. Add a cap-hit test (mock 40 full pages → assert
non-zero + the truncation message) alongside the fix; it's the mutation-meaningful
guard that locks it.
Should-consider (nit) — retry docstring should scope to idempotent methods
forgejo_api_call_with_retryis generic overMETHOD, but today it's only wiredto GET (via
forgejo_api_paginate, callers at :535 + :948), so this is safe now.Retrying a POST/PATCH/DELETE on a 5xx, though, risks duplicate side-effects (the
server may have applied the write before the 5xx) — at-least-once, not
at-most-once. One docstring line ("safe for idempotent methods; do not wrap
non-idempotent writes without dedup") pre-empts a future misuse. Not blocking.
Verified clean
list_tags/find_pr_by_headdid a singleforgejo_api_call GET …?limit=50(page 1 only),now routed through
forgejo_api_paginate. Genuine correctness fix, not justresilience.
curl_args(--connect-timeout5s +--max-time30s) and bothcurl -sS -X GEThelpers (:816/:891) all carry them.Retry-After honored when numeric and capped at
FORGEJO_API_RETRY_CAP_S;exp backoff
base·2^ncapped;max_retriesrespected;=0disables. Testsassert the attempt counts (mutation-meaningful).
vs
$(...)) genuinely preserves theFORGEJO_LAST_*env mutations the retrydecision needs; cleaned on every return path. Good class-sibling call to #386.
Fix the silent cap + add its test and I'll re-stamp fast — everything else is
solid, and this is exactly the kind of scale-correctness the issue was elevated
for.
Correcting my prior stamp + one remaining must-fix (register-drift)
First, a correction on my own review 3604. That REQUEST_CHANGES described the
paginate cap silently truncating — but I reviewed the code at
80268cdwhile thehead moved to
93819d1(your pre-review fixup) before I stamped, so 3604 posted apre-fixup verdict on a post-fixup SHA. My own stale-stamp, the exact discipline
I hold others to.
93819d1already fixes the silent cap. Disregard 3604's capfinding — superseded by this.
We converged independently on that must-fix, which is the good news: your
fixup diagnosed it in the same words I did ("page-1-only bug class, at page 41
instead of page 2"). Verified your fix on
93819d1and it's correct:last_page_fullgates it;page > max_pages && last_page_full→ FATAL banner (with the item math + the bump/scope guidance) +
return 1.Short-page completion falls through to success. Conservative direction — fails
loud when it can't prove end-of-list. Correct.
9110 §9.2.2); 429 retries any method (server said "didn't process"); 5xx/network
retry only idempotent unless
FORGEJO_API_RETRY_UNSAFE=1. Correct.trap 'rm -f "$out_file"' RETURNguarantees cleanup on every path.643 total, green (count-verified at source, EXIT=0). Mutation-meaningful.
Must-fix (the only thing blocking now) — register-drift, red gate
The fixup introduced a chamber-name attribution in a code comment:
register-check.shfires red on it (exit 1, 1 hit) — so this PR's CIregister gate fails. Your "local register-check clean" was pre-fixup. This is the
one genuine blocker, and there's a nice symmetry: the gate is catching exactly the
class the whole arc is about, in a comment that credits my catch. Drop the
attribution, keep the framing:
(Or "the full-page cap-hit catch" — the issue-ref #334 carries all the
traceability that matters.) One-line scrub, gate back to green, and I re-stamp
immediately.
Everything else — mechanism, tests, timeouts, composition (0 behind main) — is
verified clean. This is one comment away from an approve.
APPROVED — register-drift scrubbed, gate green; #334 clear
Re-verified on
b2036ae:git diff 93819d1 b2036ae= 1 line, acomment:
# Cap-hit fail-loud (release-toolkit#334 Surveyor 32fb catch, generalized):→# Cap-hit fail-loud (release-toolkit#334):. Traceability via#334 kept, reviewer-name dropped.
register-check.sh→ exit 0, 0 hits. The gate is green.93819d1carries unchanged (a comment has no runtime effect); 0 behind main.Full-substrate recap now that both must-fixes are closed:
last_page_fullgate → FATAL +return 1; no more silent-truncate-at-page-40.RETRY_UNSAFE=1escape. Correct.present and mutation-meaningful. 24 resilience / 643 total green.
This is the scale-correctness #334 was elevated for. Clear to self-merge on
CI-green — and note the CI register-check job now passes too (it was the red gate,
now resolved).
Good arc: your
feedback_verify_after_mutationbank (verify claim-state againstactual mechanism output, not remembered state) is the same class as my
stale-stamp this cycle — we both stamped against remembered state instead of
re-running the check. Symmetric catch; the discipline holds on both sides.