fix(mc): retry transport failures, stop reporting them as "Artifact not found" - #2
Merged
Merged
Conversation
…und" A download from storage.devino.ca hit a TCP reset mid-run and the action reported it as "Artifact not found", failing a required check while the producing job had passed (stealth-chrome-devtools-mcp run 34640838095, job 103404145715). Store.run() was the single mc wrapper and called subprocess.run exactly once, so any blip against the endpoint failed the step outright; the download path then passed check=False and asserted "Artifact not found" for every non-zero exit, whatever the cause. - classify_mc_error() reads mc's message: transport / auth / not-found / unknown. Order matters, so a DNS failure is not read as a missing object. - Store.run() retries transport failures only: 5 attempts, 2/4/8/16 s with +/- 25% jitter. A 404 or a rejected credential is never retried. Both verbs are safe to repeat (cp writes a whole object, ls is read-only), so upload, download and listing all inherit this. - download_error_message() keeps "Artifact not found" for a real 404 and leads with mc's own text otherwise, printed verbatim. - 30 unit tests, subprocess mocked, no network. New ci.yml "unit" job. The OIDC/STS flow is untouched.
mc echoes the object key in its error text, so the bare
\b(429|500|502|503|504)\b alternation would read a genuinely missing
"coverage-503" as a 5xx: five pointless retries, then the wrong label on
the error. A bare code now only counts when something says it is one
("status:", "code:", "responded with", "returned"); the reason phrases
(bad gateway, service unavailable, gateway timeout, too many requests)
already carried the real cases on their own.
Same for 401/403 on the auth side, with "forbidden"/"unauthorized" added
so mc's bare "403 Forbidden" is still classified.
Bare EOF is now anchored to Go's ": EOF" shape for the same reason.
3 new tests (33 total): five 5xx-looking and two 4xx-looking artifact
names that do not exist must classify as not-found, and the observed
reset with its ports and IPs must still classify as transport.
…dout The em-dash I had put in the new download error would have raised UnicodeEncodeError on a Windows runner: fail() prints to stdout, and before Python 3.15 that is the console code page (cp1252), which cannot encode it. The error path would have replaced a useful message with a traceback, on exactly the failure this PR exists to report well. The three pre-existing non-ASCII strings in the file are all summary() text, which append_file() writes with encoding="utf-8", so they are unaffected and unchanged. One test (34 total): every download_error_message() variant must encode as ASCII.
The roundtrip matrix includes ubuntu-devino, and this workflow had no
concurrency key, so every push to every branch queued a job on the
saturated self-hosted pool that nothing ever cancelled. Three pushes on
this PR left three queued pool jobs competing with each other, two of
them testing stale commits.
group: ci-${{ github.ref }} matches the prevailing style across the org
(31 of the 257 workflow-level concurrency blocks in DevinoSolutions use
exactly that group name). main is exempt from cancellation so a push
there always finishes validating; everywhere else the newest commit wins.
…duled DevinoSolutions/artifact is public, and the org's only self-hosted runner group (Default) has allows_public_repositories: false, so every job here asking for `ubuntu-devino` queues forever instead of failing. On PR #2 the roundtrip matrix cell sat queued for over four hours across four runs and never started once. That restriction should stay: putting a public repo on self-hosted runners exposes them to fork pull requests. So the workflow moves instead. - roundtrip matrix drops ubuntu-devino, keeping the three hosted platforms. - cross-job download moves from ubuntu-devino to ubuntu-latest. What it tests is that one job can fetch artifacts uploaded by other jobs on other operating systems, which does not depend on where it runs itself; its verification loop drops to the three OSes that now upload. Nothing is lost in coverage: the Linux-on-pool path is exercised continuously by the 51 consumer upload steps that call this action on ubuntu-devino from private repositories. Correction to the record: the two ubuntu-devino runs that passed on 2026-09-02 were NOT from a time when the repo was private. Its PublicEvent is 2026-09-02T05:45:04Z, the same instant as created_at, and those runs started at 06:17Z and 06:18Z. The repo has been public since creation, so it is the runner-group setting that was tightened afterwards.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failure this fixes
DevinoSolutions/stealth-chrome-devtools-mcprun34640838095, job103404145715(release-gate / release-evidence). Four artifacts downloadedcleanly, then the connection to the endpoint was reset and the action announced
a missing artifact — while the job that produced it,
install-smoke (sdist Linux/X64), had passed:Two separate defects in one line of output. The headline names the wrong cause,
and a single TCP reset was enough to red the check.
Why it matters beyond that one run
51 upload steps across 14 repositories now route through this action, 45 of
them on the
ubuntu-devinopool, and several of them feed required checks. Onecomposite action against one endpoint currently has no damping at all: any
transient reset becomes a red required check, in any migrated repository.
What the code did
Store.run()is the onlymcwrapper in the action — upload and download,cpandlsall go through it — and it invokedsubprocess.runexactly once,with no retry:
The download loop then passed
check=False, which suppresses that accuratemessage, and asserted "Artifact not found" for any non-zero exit — reset,
DNS failure, expired credentials or a real missing object alike. The true cause
was appended after a newline, so on a GitHub annotation it lands on the second
line and the headline is wrong.
What it does now
classify_mc_error(rc, output)readsmc's message rather than its exitcode, which is
1for everything:transport/auth/not-found/unknown. Order is deliberate — a DNS failure says "no such host" and mustnot be read as a missing object, and a 503 must not be read as a permission
problem.
Store.run()retriestransportonly: 5 attempts with exponentialbackoff, 2 / 4 / 8 / 16 s, ±25% jitter so a fleet-wide blip does not retry in
lockstep. A real 404 and a rejected credential are never retried — they are
deterministic answers, and retrying only delays the report by half a minute.
An unrecognised failure is not retried either.
Store.run()is the single wrapper, upload, download and listingall inherit this — that is the whole of the "same fix on the upload path".
Both verbs are safe to repeat:
cpwrites a whole object under a key derivedfrom the run, and
lsis read-only.download_error_message()keeps the familiarArtifact not foundfor agenuine 404 and otherwise leads with
mc's own text, printed verbatim, withthe attempt count and an explicit "not a missing artifact".
::warning::withmc's first line, so a run that recoversstill shows what happened.
Nothing on a healthy run changes: success on the first attempt returns exactly
as before, with no sleep and no extra output.
The OIDC / STS credential flow is untouched. No credential is printed; the
existing
::add-mask::calls are unchanged.Tests
tests/test_mc_retry.py, 30 tests, standard libraryunittestonly, matchingthe module's own no-dependency rule.
subprocess.runandtime.sleeparemocked, so nothing touches the network,
mc, the endpoint or the OIDC flow.The transport sample is the exact
mcline from the log above.Covered: the observed reset and ten other transport shapes; four missing-object
shapes; five credential shapes; the DNS-versus-404 ordering guard; the backoff
schedule, jitter bounds and index clamping; first-attempt success taking no
retry and no sleep; transport-then-success; exhaustion after exactly
MC_ATTEMPTScalls andMC_ATTEMPTS - 1sleeps; 404, auth and unknown neverretried;
check=Falsereturning instead of exiting while still retryingtransport; the tagged upload command retrying with its arguments unchanged; and
do_download()end to end, asserting that a reset no longer produces anArtifact not foundannotation while a real 404 still does.Mutation-checked rather than asserted: reverting the download message, removing
the retry, and retrying every error class each make the suite fail (1, 7 and 6
failing tests respectively).
A new
unitjob inci.ymlruns them onubuntu-latest. It needs no OIDC, nomcand no self-hosted runner, so it adds no load to the pool.Checks
.github/workflows/ci.ymlalready exercises the real round trip onubuntu-latest,windows-latest,macos-latestandubuntu-devino, plus across-job download, so the happy path is covered against live MinIO by this
PR's own checks.
LF only, 0 CR bytes in all four files (counted on raw bytes).