From 3df2cdbc2a113616513cb52f6462339c2afbf56e Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Sun, 26 Jul 2026 11:50:19 -0400 Subject: [PATCH] fix(ci): retry the Packagist update on transient upstream failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every package in the split matrix reaches the notify step within the same second, so a release fires ~92 update-package calls at Packagist at once and reliably trips its limits. On 0.8.5 eight of them came back HTTP 500 at the same instant. The single un-retried curl turned that into eight failed jobs, and because the failure landed after "Split and push", the tags were already correct — the only real damage was a stale Packagist index, marko/core among it, which every other package resolves against. Re-running the failed jobs was enough to fix it, which is the definition of a retry we should not be doing by hand. Wrap the update call in five attempts with jittered exponential backoff. Only the transient classes retry (5xx, 429, and curl's 000 transport failure); 404 still falls through to the create-package self-heal, and 401/403 fail immediately since no retry fixes a bad token. The jitter matters as much as the backoff: all 92 jobs fail on the same second, so a fixed delay would just line their retries up on the same second too. Retrying means the curl can no longer be allowed to abort the step, so the final status is now matched against the 2xx class rather than tested with `-ge 400` — a transport failure arrives as 000, which is numerically zero and would otherwise be waved through as success, leaving the package stale behind a green job. Verified behaviourally against a stub Packagist rather than only by reading: recovery after two 500s (the 0.8.5 case), failure once five attempts are exhausted, 429 retry, the 404 registration path, fail-fast on 401, and connection-refused exiting non-zero. That harness also turned up a smaller wart now fixed here — curl leaves the response file untouched when it cannot connect, so a failed attempt printed the previous attempt's body as its own. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/split.yml | 83 +++++++++++++++++++++++++++++++------ tests/SplitWorkflowTest.php | 37 +++++++++++++++++ 2 files changed, 107 insertions(+), 13 deletions(-) diff --git a/.github/workflows/split.yml b/.github/workflows/split.yml index ffbcdef9..54cd12ee 100644 --- a/.github/workflows/split.yml +++ b/.github/workflows/split.yml @@ -90,6 +90,57 @@ jobs: PACKAGIST_TOKEN: ${{ secrets.PACKAGIST_TOKEN }} run: | PKG_URL="https://github.com/${SPLIT_ORG}/marko-${{ matrix.package }}" + RESP_FILE=/tmp/packagist_resp + + # Every package in the matrix reaches this step within the same second, + # so a release fires ~92 update-package calls at Packagist at once and + # reliably trips its limits. On 0.8.5 that returned HTTP 500 for eight + # packages; the tags had already pushed, so the job failed loudly while + # the only real damage was a stale Packagist index — including + # marko/core, which everything else resolves against. + # + # Retry the transient classes and let the definitive ones fall through + # to the caller: 404 means "not registered" and drives the create-package + # self-heal below, and 401/403 mean a bad token, which no retry fixes. + # HTTP is global rather than echoed so the log lines below can't be + # captured as part of the status code. + HTTP="" + update_with_retry() { + local attempt delay + for attempt in 1 2 3 4 5; do + # curl leaves the response file untouched when it cannot connect, + # so clear it first — otherwise a transport failure reports the + # previous attempt's body as if it were this one's. + : > "$RESP_FILE" + + # `|| true` keeps a transport failure from aborting the step under + # `bash -e` so it can be retried; the status is validated below. + HTTP=$(curl -s -o "$RESP_FILE" -w "%{http_code}" \ + --connect-timeout 10 --max-time 60 \ + -X POST https://packagist.org/api/update-package \ + -H "Content-Type: application/json" \ + -H "Authorization: Bearer markshust:${PACKAGIST_TOKEN}" \ + -d "{\"repository\":\"${PKG_URL}\"}") || true + + case "$HTTP" in + 000|429|5??) ;; # transient — worth another attempt + *) return 0 ;; # definitive — caller decides what it means + esac + + echo "Packagist update returned HTTP ${HTTP} (attempt ${attempt}/5)" + head -c 500 "$RESP_FILE" 2>/dev/null || true + echo + + # Jittered backoff. Without the jitter every job would have failed + # on the same second and would retry on the same second too. + if [[ "$attempt" -lt 5 ]]; then + delay=$(( 2 ** attempt + RANDOM % 5 )) + echo "Retrying in ${delay}s..." + sleep "$delay" + fi + done + return 0 + } # Try to update first. If Packagist returns 404 the package isn't # registered yet — self-heal by calling create-package, then retry @@ -97,11 +148,7 @@ jobs: # the manual `register-packagist.sh` step from the new-package # workflow: contributors can add `packages/foo/` via PR and the # first release tag will register + publish it automatically. - HTTP=$(curl -s -o /tmp/packagist_resp -w "%{http_code}" \ - -X POST https://packagist.org/api/update-package \ - -H "Content-Type: application/json" \ - -H "Authorization: Bearer markshust:${PACKAGIST_TOKEN}" \ - -d "{\"repository\":\"${PKG_URL}\"}") + update_with_retry if [[ "$HTTP" == "404" ]]; then echo "marko/${{ matrix.package }} not registered on Packagist — registering now..." @@ -110,12 +157,22 @@ jobs: -H "Content-Type: application/json" \ -d "{\"repository\":{\"url\":\"${PKG_URL}\"}}" echo "Registered. Re-running update so the tag gets indexed..." - curl -sf -X POST https://packagist.org/api/update-package \ - -H "Content-Type: application/json" \ - -H "Authorization: Bearer markshust:${PACKAGIST_TOKEN}" \ - -d "{\"repository\":\"${PKG_URL}\"}" - elif [[ "$HTTP" -ge 400 ]]; then - echo "Packagist update failed with HTTP ${HTTP}:" - cat /tmp/packagist_resp - exit 1 + update_with_retry fi + + # Match on the success class rather than testing `-ge 400`. The curl + # above no longer aborts the step on a transport failure, so that case + # now reaches here as 000 — numerically zero, and therefore something + # a `-ge 400` test would wave through as success, leaving the package + # stale behind a green job. + case "$HTTP" in + 2??) + echo "marko/${{ matrix.package }} updated on Packagist (HTTP ${HTTP})" + ;; + *) + echo "Packagist update failed with HTTP ${HTTP} after 5 attempts:" + cat "$RESP_FILE" 2>/dev/null || true + echo + exit 1 + ;; + esac diff --git a/tests/SplitWorkflowTest.php b/tests/SplitWorkflowTest.php index 74746116..cc2b75a7 100644 --- a/tests/SplitWorkflowTest.php +++ b/tests/SplitWorkflowTest.php @@ -43,3 +43,40 @@ it('configures the target organization as an environment variable for easy changes', function () use ($workflowContent): void { expect($workflowContent)->toContain('SPLIT_ORG: marko-php'); }); + +it('retries the Packagist update on transient upstream failures', function () use ($workflowContent): void { + // All 92 split jobs POST to Packagist within the same second, so a release + // reliably trips its rate limits. 0.8.5 lost 8 packages to a burst of 500s + // that the single un-retried curl surfaced as a hard job failure. + expect($workflowContent)->toContain('update_with_retry'); +}); + +it('treats 5xx, 429, and curl transport errors as retryable', function () use ($workflowContent): void { + expect($workflowContent)->toContain('000|429|5??)'); +}); + +it('backs off between Packagist retries with jitter so they do not re-collide', function () use ($workflowContent): void { + // Every job fails at the same instant, so a fixed backoff would just line + // the retries up again on the same second. + expect($workflowContent)->toContain('RANDOM') + ->toContain('sleep "$delay"'); +}); + +it('still self-heals an unregistered package via create-package on 404', function () use ($workflowContent): void { + expect($workflowContent)->toContain('api/create-package') + ->toContain('"$HTTP" == "404"'); +}); + +it('fails the job on any non-2xx Packagist response once retries are exhausted', function () use ($workflowContent): void { + // Retrying means the curl can no longer abort the step itself, so a + // transport failure arrives here as 000 — numerically zero, and waved + // through by a `-ge 400` test as if it had succeeded. + expect($workflowContent)->toContain('2??)') + ->toContain('exit 1'); +}); + +it('clears the response file between attempts', function () use ($workflowContent): void { + // curl leaves it untouched when it cannot connect, which otherwise reports + // the previous attempt's body as though it belonged to this one. + expect($workflowContent)->toContain(': > "$RESP_FILE"'); +});