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

[sterfried]

Why

MFD deploys intermittently fail when this action triggers the ArgoCD update-application.yaml workflow. The dispatch POST gets a transient HTTP 500 from GitHub and the whole deploy/migrations job dies with no retry:

Triggering workflow: workflows/update-application.yaml/dispatches
curl: (22) The requested URL returned error: 500
api failed:
response: { "message": "Failed to run workflow dispatch", "status": "500" }

Example: cloudbeds/mfd merge-to-main deploy — https://github.com/cloudbeds/mfd/actions/runs/30014504167/job/89231870820 (job Deploy stage-us2 / Migrations). Every deploy path (merge-main→stage, RC, release, migration-hotfix, to stage and prod) routes through this action, so a single transient 5xx anywhere blocks the deploy and pages the squad.

Root cause

api() already had a would-be retry branch, but it was broken two ways and both bit here:

1. Wrong match string — it only treated a failure as retryable when the body contained the literal "Server Error". GitHub's real dispatch 500 says "Failed to run workflow dispatch" (no match), so it fell straight to exit 1.
2. No actual retry even when matched — the branch just echoed "Server error - trying again" and returned; it never re-issued the request. On the dispatch call that returns empty, leaving the caller polling for a run that was never created (a latent hang).

Net effect today: zero resilience to a transient dispatch failure.

What changed

• Rewrote api() as a bounded retry-with-backoff. It captures the HTTP status via curl -w (instead of --fail-with-body) so it can distinguish failure classes:
  • retry — network/transport errors (000), 429, and 5xx, with capped exponential backoff;
  • fail fast — other 4xx (real client errors), no retry;
  • surface failure — once attempts are exhausted it still exit 1s, so a genuine outage is never masked.
• Tunable via env (sensible defaults): API_MAX_ATTEMPTS (5), API_RETRY_BASE_SECONDS (3), API_RETRY_MAX_SECONDS (30). The retry also covers the polling GETs, not just the dispatch.

Testing

Added a shell test harness (tests/) driving the real api() against a scripted curl stub, plus a CI workflow (.github/workflows/test.yaml) to run it on every PR. Coverage:

• retries transient 500 then succeeds;
• does not retry a 4xx (fails fast in 1 attempt);
• gives up after API_MAX_ATTEMPTS on a persistent 500 (failure still surfaces);
• retries a transport error;
• retry loop is clean under set -e (which production runs with).

Verified green under both bash and the runtime busybox sh inside alpine:3.15.0 (the Dockerfile base).

Rollout

This action is consumed by pinned tag. After merge:

1. Tag a new release (v1.6.6).
2. Bump the pin in cloudbeds/mfd (.github/actions/trigger-argocd/action.yaml, currently @v1.6.5) to the new tag. Happy to open that PR once the tag exists.

---

Sterling can resume the Claude Code session that produced this work with: claude --resume 1d0ac1d0-a5f8-48e2-b03e-dcebcf07306c (run from /var/www/phpprojects/mfd)

🤖 Generated with Claude Code

[unblocked]

✅ No issues found

About Unblocked

Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.

📖 Documentation — Learn more in our docs.

💬 Ask questions — Mention @unblocked to request a review or summary, or ask follow-up questions.

👍 Give feedback — React to comments with 👍 or 👎 to help us improve.

⚙️ Customize — Adjust settings in your preferences.

[greptile-apps]

Greptile Summary

This PR replaces a broken no-op retry stub in api() with a proper bounded retry-with-backoff implementation. It adds a CI test suite with a scripted curl stub that exercises six distinct scenarios including transport errors, rate-limiting, backoff capping, and set -e compatibility.

• entrypoint.sh: rewrites api() to capture the HTTP status via curl -w '%{http_code}' instead of --fail-with-body, then branches on it — retrying 000/429/5xx with capped exponential backoff and fast-failing on other 4xx; also adds a TWAW_SOURCE_ONLY guard so the script can be sourced by tests without running main.
• tests/: introduces api_retry.test.sh (six scenarios under bash), api_retry_seterr.test.sh (retry path under set -e), and lightweight curl/sleep stubs.
• .github/workflows/test.yaml: runs the test suite under both bash (ubuntu runner) and busybox sh (alpine:3.15.0 Docker image — the real production shell).

Confidence Score: 5/5

Safe to merge — the retry logic is correct, POSIX-compatible, and covered by tests that exercise the real production shell.

The rewritten api() correctly distinguishes retryable errors (transport/429/5xx) from fast-fail client errors (4xx), implements capped exponential backoff, and always surfaces failure once retries are exhausted. The set -e compatibility of every conditional construct in the retry loop was verified by the dedicated seterr test, and the full suite runs inside alpine:3.15.0 (the production Docker base) in CI. The TWAW_SOURCE_ONLY guard is a clean, non-intrusive addition that does not affect the production code path.

No files require special attention.

Important Files Changed

Filename	Overview
entrypoint.sh	Core fix: api() rewritten with correct bounded retry-with-backoff; POSIX-compatible arithmetic and conditional forms are used throughout; the duplicate lets_wait() present in an earlier iteration has been removed; TWAW_SOURCE_ONLY guard is safe and correctly scoped.
tests/api_retry.test.sh	Six scenarios cover success-after-retry, fast-fail on 4xx, exhausted retries, transport error recovery, 429 retry, and backoff capping; temp files cleaned via trap; assertion logic is clear and correct.
tests/api_retry_seterr.test.sh	Dedicated set -e compatibility test for the retry loop; correctly leaves set -e active after sourcing and wraps api() in an if-subshell to prevent harness abort; trap added for temp file cleanup.
tests/stub/curl	Faithful curl stub that handles both the old (--fail-with-body) and new (-o / -w) calling conventions; bash-specific parameter substitution is fine since the stub shebang is #!/usr/bin/env bash and bash is installed in the alpine CI step.
.github/workflows/test.yaml	Two-step CI: bash run on ubuntu-latest followed by busybox sh run inside alpine:3.15.0 via Docker, which is the actual production shell; apk installs bash so the curl stub shebang resolves correctly inside the container.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([api called]) --> B[mktemp body_file]
    B --> C["curl -sSL -o body_file -w '%{http_code}'"]
    C -- "curl exits 0" --> D{http_code?}
    C -- "curl exits non-zero" --> E[http_code = 000]
    E --> D
    D -- "2xx" --> F[echo response\nreturn 0]
    D -- "000 / 429 / 5xx" --> G{attempt < max_attempts?}
    D -- "other 4xx" --> H[log error\nexit 1]
    G -- "yes" --> I["log warning\nsleep delay\nattempt++\ndelay = min(delay*2, max_delay)\ncontinue"]
    I --> B
    G -- "no (exhausted)" --> H

Loading

_{Reviews (3): Last reviewed commit: "test: lock in 429 + backoff behavior; no..." | Re-trigger Greptile}

[sterfried]

Thanks for the review @greptile-apps — all three addressed in d833b58:

1. Dead lets_wait() — removed (it was pre-existing; the second definition already overrode it).
2. busybox coverage — added a CI step that runs the full suite under busybox sh in alpine:3.15.0 via Docker. I didn't use the ubuntu runner's sh because that's dash, which can't parse entrypoint.sh's existing <(...)/[[ ]] — details in the inline thread.
3. Temp file cleanup — added EXIT traps in both test harnesses.

Verified green under both bash and busybox locally.

[greptile-apps]

All three issues are cleanly addressed. The implementation looks solid:

• Dead lets_wait(): removed the first definition at line 85, leaving only the active one.
• busybox CI step: runs the full suite inside alpine:3.15.0 via Docker with the correct rational (dash can't parse the ash/bash extensions already in entrypoint.sh). Pulling from public.ecr.aws is a nice touch to avoid Docker Hub rate limits in CI.
• Temp file cleanup: trap 'rm -f "$CURL_STUB_COUNTER" "$CURL_STUB_SCENARIO"' EXIT in both test harnesses is exactly right — the EXIT trap fires on any exit path including signals.

One minor thing worth a look: the curl stub uses ${w_fmt//%\{http_code\}/$code} (a bash parameter substitution) but the stub's shebang is #!/usr/bin/env bash, so that's fine. When the tests run under busybox sh the stub is still invoked as a subprocess with its own shebang, not sourced, so no compatibility issue there.

LGTM — this is ready to merge.

[sterfried]

Author walk-through — a few inline notes on the non-obvious bits to make review easier.

[sterfried]

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.

[sterfried]

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×.

[sterfried]

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.

[sterfried]

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).

[sterfried]

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.

[sterfried]

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.

[sterfried]

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.

[sterfried]

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.

[sterfried]

Addressed the review follow-ups in 173661f (all were non-blocking niceties):

• Double-dispatch note — added an inline comment at the retry branch: retrying the non-idempotent dispatch POST assumes a pre-creation failure (true for the observed "Failed to run workflow dispatch" 500), and even a duplicate would be harmless since trigger_workflow keys off newly-appeared runs and an ArgoCD sync is idempotent.
• 429 test — added an explicit 429-then-success scenario to lock that path in.
• Backoff assertion — added a sleep stub that records delays instead of sleeping, so the test now asserts the exact sequence (3 6 10 10 = doubling then capped at API_RETRY_MAX_SECONDS) without real waits.

Suite is now 6 assertions + the set -e test, green under both bash and busybox (alpine:3.15.0) in CI.