Skip to content

fix: retry transient GitHub API errors instead of failing the dispatch #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sterfried merged 4 commits into from
Jul 24, 2026
Merged

fix: retry transient GitHub API errors instead of failing the dispatch #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc95553
fix: retry transient GitHub API errors instead of failing the dispatch
sterfried Jul 23, 2026
c3c5c30
ci: restrict test workflow GITHUB_TOKEN to contents: read
sterfried Jul 23, 2026
d833b58
chore: address review — drop dead lets_wait, cover busybox in CI, cle…
sterfried Jul 23, 2026
173661f
test: lock in 429 + backoff behavior; note dispatch-retry idempotency…
sterfried Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions .github/workflows/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
name: Test

on:
pull_request:
push:
branches:
- main

jobs:
shell-tests:
name: entrypoint.sh tests
runs-on: ubuntu-latest
permissions:
contents: read
steps:
- name: Checkout Code
uses: actions/checkout@v4

- name: Run api() retry tests (bash)
run: |
set -e
for t in tests/*.test.sh; do
echo "== $t (bash) =="
bash "$t"
done
Comment thread
greptile-apps[bot] marked this conversation as resolved.

# Exercise the real runtime shell. The Docker entrypoint runs the script
# with busybox `sh` in alpine:3.15.0 (see Dockerfile), which relies on ash
# extensions the ubuntu runner's /bin/sh (dash) does not support, so run
# the suite inside that image rather than under the host shell.
- name: Run api() retry tests (busybox sh, alpine:3.15.0)

@sterfried sterfried Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This second run is the faithful production-shell check: the Docker entrypoint uses busybox sh in alpine:3.15.0, which relies on ash extensions (<(...), [[ ]]) that the runner's /bin/sh (dash) can't parse. Running the suite inside that exact image is the only way to exercise the real runtime shell in CI.

run: |
docker run --rm -v "$PWD":/work -w /work \
public.ecr.aws/docker/library/alpine:3.15.0 \
sh -c 'apk add --no-cache bash curl jq coreutils >/dev/null && \
set -e && \
for t in tests/*.test.sh; do echo "== $t (busybox) =="; sh "$t"; done'
91 changes: 69 additions & 22 deletions entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,31 +82,73 @@ validate_args() {
fi
}

lets_wait() {
echo "Sleeping for ${wait_interval} seconds"
sleep "$wait_interval"
}

# Perform a GitHub REST API call, retrying transient failures.
#
# GitHub occasionally returns transient errors on these endpoints - most notably
# the workflow-dispatch trigger, which intermittently 500s with the body
# {"message":"Failed to run workflow dispatch","status":"500"}. Network errors
# and HTTP 429/5xx are retried with capped exponential backoff. Other 4xx
# responses are client errors and fail fast, and once retries are exhausted the
# failure is still surfaced to the caller so a genuine outage is never masked.
#
# Tunable via environment (defaults in parentheses):
# API_MAX_ATTEMPTS total attempts before giving up (5)
# API_RETRY_BASE_SECONDS first backoff delay; doubles each retry (3)
# API_RETRY_MAX_SECONDS cap on any single backoff delay (30)
api() {
path=$1; shift
if response=$(curl --fail-with-body -sSL \
"${GITHUB_API_URL}/repos/${INPUT_OWNER}/${INPUT_REPO}/actions/$path" \
-H "Authorization: Bearer ${INPUT_GITHUB_TOKEN}" \
-H 'Accept: application/vnd.github.v3+json' \
-H 'Content-Type: application/json' \
"$@")
then
echo "$response"
else
local path=$1; shift
local max_attempts=${API_MAX_ATTEMPTS:-5}
local delay=${API_RETRY_BASE_SECONDS:-3}
local max_delay=${API_RETRY_MAX_SECONDS:-30}
local attempt=1
local body_file http_code response

while true; do
body_file=$(mktemp)
# No --fail-with-body: capture the status code via -w and branch on it,
# so we can tell a retryable 5xx apart from a fatal 4xx. A transport-level
# failure (curl exits non-zero, prints nothing) is mapped to code 000.
http_code=$(curl -sSL \
-o "$body_file" \
-w '%{http_code}' \
"${GITHUB_API_URL}/repos/${INPUT_OWNER}/${INPUT_REPO}/actions/$path" \
-H "Authorization: Bearer ${INPUT_GITHUB_TOKEN}" \
-H 'Accept: application/vnd.github.v3+json' \
-H 'Content-Type: application/json' \
"$@") || http_code=000

@sterfried sterfried Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key change vs. the old code: dropped --fail-with-body. We capture the HTTP status via -w '%{http_code}' (body goes to the temp file) and branch on the code below, so a 5xx comes back here as a normal curl success and can be told apart from a 4xx. If curl itself fails (DNS/connection/timeout → non-zero exit, no output), the || http_code=000 maps it to 000, which we treat as a retryable transport error.

response=$(cat "$body_file")
rm -f "$body_file"

# Success (2xx): return the response body.
if [ "$http_code" -ge 200 ] && [ "$http_code" -lt 300 ]; then
echo "$response"
return 0
fi

# Retry transport failures (000), rate limiting (429) and server errors (5xx).
# Retrying the non-idempotent dispatch POST assumes the failure was
# pre-creation - true for the observed "Failed to run workflow dispatch" 500.
# Even if a run were somehow created, trigger_workflow keys off newly-appeared
# runs and an ArgoCD sync is idempotent, so a duplicate is harmless.
if [ "$http_code" = "000" ] || [ "$http_code" = "429" ] || [ "$http_code" -ge 500 ]; then

@sterfried sterfried Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only transient classes retry: transport (000), rate-limit (429), and server errors (5xx). Every other status — i.e. real 4xx client errors — skips this block and falls through to the fail-fast path, so we never retry a genuinely bad request 5×.

if [ "$attempt" -lt "$max_attempts" ]; then
echo >&2 "api transient failure (HTTP ${http_code}) on ${path}; attempt ${attempt}/${max_attempts}, retrying in ${delay}s"
[ -n "$response" ] && echo >&2 "response: $response"
sleep "$delay"
attempt=$((attempt + 1))
delay=$((delay * 2))
[ "$delay" -gt "$max_delay" ] && delay=$max_delay

@sterfried sterfried Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capped exponential backoff: delay starts at API_RETRY_BASE_SECONDS and doubles each attempt, but is clamped to API_RETRY_MAX_SECONDS so it can't grow unbounded.

continue
fi
fi

# Non-retryable error, or retries exhausted: surface the failure.

@sterfried sterfried Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both terminal paths land here — a non-retryable status, or retries exhausted — and still exit 1. Intentional: the goal is to ride out transient blips, not to swallow a real failure (silently swallowing was half of the old bug).

echo >&2 "api failed:"
echo >&2 "path: $path"
echo >&2 "http_code: $http_code"
echo >&2 "response: $response"
if [[ "$response" == *'"Server Error"'* ]]; then
echo >&2 "Server error - trying again"
else
exit 1
fi
fi
exit 1
done
}

lets_wait() {
Expand Down Expand Up @@ -242,4 +284,9 @@ main() {
fi
}

main
# Allow the script to be sourced by the test harness without executing main.
# TWAW_SOURCE_ONLY is never set in production (the Docker entrypoint runs the
# script directly), so the default behaviour is unchanged.
if [ "${TWAW_SOURCE_ONLY:-}" != "1" ]; then

@sterfried sterfried Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test-only seam: lets the test harness source this file to call api() directly without running main. TWAW_SOURCE_ONLY is never set in production (the Docker entrypoint runs sh entrypoint.sh), so runtime behavior is unchanged.

main
fi
107 changes: 107 additions & 0 deletions tests/api_retry.test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
#!/usr/bin/env bash
# Tests for the transient-error retry behaviour of entrypoint.sh's api() helper.
# Runs the real api() against a scripted `curl` stub (tests/stub/curl).
set -u

HERE=$(cd "$(dirname "$0")" && pwd)
ROOT=$(cd "$HERE/.." && pwd)

# Put the curl stub first on PATH; every other tool (jq, sed, mktemp) stays real.
export PATH="$HERE/stub:$PATH"

export CURL_STUB_COUNTER
CURL_STUB_COUNTER=$(mktemp)
export CURL_STUB_SCENARIO
CURL_STUB_SCENARIO=$(mktemp)
Comment thread
greptile-apps[bot] marked this conversation as resolved.
trap 'rm -f "$CURL_STUB_COUNTER" "$CURL_STUB_SCENARIO"' EXIT

# Values api() reads to build the request URL (contents irrelevant to the stub).
export INPUT_OWNER=cloudbeds INPUT_REPO=argocd-mfd INPUT_GITHUB_TOKEN=x
# Retry knobs: keep attempts small and eliminate real sleeping so tests are fast.
export API_MAX_ATTEMPTS=4
export API_RETRY_BASE_SECONDS=0

# Source the script for its functions only; do not run main.
# shellcheck disable=SC1090
TWAW_SOURCE_ONLY=1 . "$ROOT/entrypoint.sh"
set +e # entrypoint.sh enables `set -e`; disable it so the harness controls flow.

@sterfried sterfried Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entrypoint.sh turns on set -e when sourced; we turn it back off so the harness controls its own flow. api() calls exit on hard failure, so every call is wrapped in a subshell / command substitution to contain that exit — the separate *_seterr* test then re-checks the retry loop with set -e left on, which is how production runs.


fail=0
pass() { printf 'ok - %s\n' "$1"; }
die() { printf 'FAIL - %s\n' "$1"; fail=1; }

# scenario "<printf-escaped lines>" resets the counter and writes the response script.
scenario() { : > "$CURL_STUB_COUNTER"; printf '%b' "$1" > "$CURL_STUB_SCENARIO"; }
attempts() { cat "$CURL_STUB_COUNTER"; }

S500='500\t{"message":"Failed to run workflow dispatch","status":"500"}'

# --- Test 1: transient 500s then success --------------------------------------
# GitHub's dispatch endpoint intermittently returns a 500 with this exact body
# (no "Server Error" string). api() must retry and ultimately succeed.
scenario "$S500\n$S500\n204\t\n"
# api() calls `exit` on hard failure, so always invoke it in a subshell.
out=$(api "workflows/update-application.yaml/dispatches" --data '{}' 2>/dev/null); rc=$?
if [ "$rc" -eq 0 ] && [ "$(attempts)" -eq 3 ]; then
pass "retries transient 500 then succeeds (3 attempts)"
else
die "retries transient 500 then succeeds: rc=$rc attempts=$(attempts) (want rc=0 attempts=3)"
fi

# --- Test 2: genuine 4xx fails fast, no retry ---------------------------------
scenario '404\t{"message":"Not Found"}\n'
out=$(api "workflows/missing.yaml/dispatches" --data '{}' 2>/dev/null); rc=$?
if [ "$rc" -ne 0 ] && [ "$(attempts)" -eq 1 ]; then
pass "does not retry a 4xx (fails fast in 1 attempt)"
else
die "does not retry a 4xx: rc=$rc attempts=$(attempts) (want rc!=0 attempts=1)"
fi

# --- Test 3: persistent 500 exhausts retries then fails ------------------------
scenario "$S500\n$S500\n$S500\n$S500\n$S500\n"
out=$(api "workflows/update-application.yaml/dispatches" --data '{}' 2>/dev/null); rc=$?
if [ "$rc" -ne 0 ] && [ "$(attempts)" -eq "$API_MAX_ATTEMPTS" ]; then
pass "gives up after API_MAX_ATTEMPTS on persistent 500 (surfaces failure)"
else
die "persistent 500 exhausts retries: rc=$rc attempts=$(attempts) (want rc!=0 attempts=$API_MAX_ATTEMPTS)"
fi

# --- Test 4: transport error (no HTTP response) is retried --------------------
scenario 'NETERR\n200\t{"ok":true}\n'
if out=$(api "runs/123" 2>/dev/null); then rc=0; else rc=$?; fi
if [ "$rc" -eq 0 ] && [ "$(attempts)" -eq 2 ] && printf '%s' "$out" | grep -q '"ok":true'; then
pass "retries a transport failure then succeeds (2 attempts)"
else
die "retries a transport failure: rc=$rc attempts=$(attempts) out=$out"
fi

# --- Test 5: a 429 (rate limit) is retried like a 5xx -------------------------
scenario '429\t{"message":"API rate limit exceeded"}\n204\t\n'
out=$(api "workflows/update-application.yaml/dispatches" --data '{}' 2>/dev/null); rc=$?
if [ "$rc" -eq 0 ] && [ "$(attempts)" -eq 2 ]; then
pass "retries a 429 then succeeds (2 attempts)"
else
die "retries a 429: rc=$rc attempts=$(attempts) (want rc=0 attempts=2)"
fi

# --- Test 6: backoff doubles each retry and caps at API_RETRY_MAX_SECONDS ------
# The sleep stub records each requested delay (via SLEEP_LOG) instead of really
# sleeping, so the backoff sequence can be asserted without waiting. With base 3
# and cap 10 over 5 persistent failures, the delays are 3, 6, 10 (12 capped), 10.
SLEEP_LOG=$(mktemp)
scenario "$S500\n$S500\n$S500\n$S500\n$S500\n"
out=$(
export SLEEP_LOG API_RETRY_BASE_SECONDS=3 API_RETRY_MAX_SECONDS=10 API_MAX_ATTEMPTS=5
api "workflows/update-application.yaml/dispatches" --data '{}' 2>/dev/null
); rc=$?
seq=$(tr '\n' ' ' < "$SLEEP_LOG" | sed 's/ *$//')
rm -f "$SLEEP_LOG"
if [ "$rc" -ne 0 ] && [ "$(attempts)" -eq 5 ] && [ "$seq" = "3 6 10 10" ]; then
pass "backoff doubles then caps at API_RETRY_MAX_SECONDS (delays: 3 6 10 10)"
else
die "backoff sequence: rc=$rc attempts=$(attempts) delays=[$seq] (want attempts=5 delays='3 6 10 10')"
fi

echo "----"
if [ "$fail" -eq 0 ]; then echo "ALL PASS"; else echo "SOME FAILED"; fi
exit "$fail"
39 changes: 39 additions & 0 deletions tests/api_retry_seterr.test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#!/bin/sh
# Production runs entrypoint.sh with `set -e` (see the top of the script). The
# retry loop uses `A && B` lists and arithmetic that can trip `set -e` if written
# carelessly, so this test exercises the multi-retry path with `set -e` left ON
# and asserts the loop runs to completion instead of aborting early.
set -eu

HERE=$(cd "$(dirname "$0")" && pwd)
ROOT=$(cd "$HERE/.." && pwd)
export PATH="$HERE/stub:$PATH"

CURL_STUB_COUNTER=$(mktemp); export CURL_STUB_COUNTER
CURL_STUB_SCENARIO=$(mktemp); export CURL_STUB_SCENARIO
trap 'rm -f "$CURL_STUB_COUNTER" "$CURL_STUB_SCENARIO"' EXIT
export INPUT_OWNER=cloudbeds INPUT_REPO=argocd-mfd INPUT_GITHUB_TOKEN=x
export API_MAX_ATTEMPTS=4 API_RETRY_BASE_SECONDS=0

# Sourcing re-enables `set -e`; deliberately do NOT disable it afterwards.
TWAW_SOURCE_ONLY=1 . "$ROOT/entrypoint.sh"

S500='500\t{"message":"Failed to run workflow dispatch","status":"500"}'
: > "$CURL_STUB_COUNTER"
printf '%b' "$S500\n$S500\n204\t\n" > "$CURL_STUB_SCENARIO"

# api() calls exit on hard failure and inherits `set -e`; run it in a subshell
# inside an `if` so neither its exit nor a non-zero status aborts this harness.
if ( api "workflows/update-application.yaml/dispatches" --data '{}' >/dev/null 2>&1 ); then
rc=0
else
rc=$?
fi
n=$(cat "$CURL_STUB_COUNTER")

if [ "$rc" -eq 0 ] && [ "$n" -eq 3 ]; then
echo "ok - retry loop completes cleanly under set -e (3 attempts)"
exit 0
fi
echo "FAIL - set -e retry path: rc=$rc attempts=$n (want rc=0 attempts=3)"
exit 1
58 changes: 58 additions & 0 deletions tests/stub/curl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
#!/usr/bin/env bash
# Faithful-enough `curl` stub for exercising entrypoint.sh's api() function.
#
# It understands only the flags api() actually uses (--fail-with-body, -o <file>,
# -w <fmt>) and ignores everything else (-H, -sSL, --data, the URL). Behaviour is
# driven by two files so a single test can script a sequence of responses:
#
# $CURL_STUB_SCENARIO : one line per attempt. Either "<CODE><TAB><BODY>" or the
# literal token NETERR to simulate a transport failure.
# $CURL_STUB_COUNTER : running attempt counter (created on first call).
#
# It emulates real curl closely enough that both the old (--fail-with-body, body
# on stdout) and the new (-o file + -w '%{http_code}') implementations work.
set -u

fail_with_body=0
out_file=""
w_fmt=""

while [ $# -gt 0 ]; do
case "$1" in
--fail-with-body) fail_with_body=1 ;;
-o) shift; out_file="$1" ;;
-w) shift; w_fmt="$1" ;;
esac
shift
done

n=$(cat "$CURL_STUB_COUNTER" 2>/dev/null || echo 0)
n=$((n + 1))
echo "$n" > "$CURL_STUB_COUNTER"

line=$(sed -n "${n}p" "$CURL_STUB_SCENARIO")
if [ "$line" = "NETERR" ] || [ -z "$line" ]; then
# Transport-level failure (connection reset, DNS, timeout): no HTTP response.
exit 7
fi

code=$(printf '%s' "$line" | cut -f1)
body=$(printf '%s' "$line" | cut -f2-)

# Body goes to the -o file if requested, otherwise to stdout (like real curl).
if [ -n "$out_file" ]; then
printf '%s' "$body" > "$out_file"
else
printf '%s' "$body"
fi

# The -w template is written to stdout with %{http_code} substituted.
if [ -n "$w_fmt" ]; then
printf '%s' "${w_fmt//%\{http_code\}/$code}"
fi

# --fail-with-body makes real curl exit 22 on HTTP >= 400 (still emitting body).
if [ "$fail_with_body" -eq 1 ] && [ "$code" -ge 400 ]; then
exit 22

@sterfried sterfried Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stub emulates real curl closely enough that both the old code path (--fail-with-body, body on stdout, exit 22 on ≥400) and the new one (-o file + -w status) work against it. That's what let the same tests fail against the old api() (proving they test the bug) and pass against the new one.

fi
exit 0
6 changes: 6 additions & 0 deletions tests/stub/sleep
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#!/bin/sh
# Test stub for `sleep`: records the requested duration (when SLEEP_LOG is set)
# and returns immediately instead of actually sleeping, so backoff timing can be
# asserted without real delays. When SLEEP_LOG is unset it is a silent no-op.
[ -n "${SLEEP_LOG:-}" ] && printf '%s\n' "$1" >> "$SLEEP_LOG"
exit 0
Loading