fix(mc): retry transport failures, stop reporting them as "Artifact not found"

.github/workflows/ci.yml

@@ -10,14 +10,30 @@ permissions:
   contents: read
   id-token: write
 
+# Without this, every push to every branch started a full run that nothing
+# superseded: this workflow uploads and downloads a 130 MiB fixture per Linux
+# job, so redundant runs are not free even on hosted runners. Group name
+# matches the org's prevailing ci.yml style; main is exempt from cancellation
+# so a push there always finishes validating.
+concurrency:
+  group: ci-${{ github.ref }}
+  cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
+
 jobs:
   roundtrip:
     name: roundtrip (${{ matrix.os }})
     runs-on: ${{ matrix.os }}
     strategy:
       fail-fast: false
+      # GitHub-hosted only. This repository is public, and the org's only
+      # self-hosted runner group (Default) sets allows_public_repositories:
+      # false, so a job asking for `ubuntu-devino` here can never be assigned
+      # and queues forever. That restriction is deliberate and should stay:
+      # letting a public repo onto self-hosted runners exposes them to fork
+      # pull requests. The Linux pool path is exercised continuously anyway by
+      # the 51 consumer upload steps that run this action on `ubuntu-devino`.
       matrix:
-        os: [ubuntu-latest, windows-latest, macos-latest, ubuntu-devino]
+        os: [ubuntu-latest, windows-latest, macos-latest]
     steps:
       - uses: actions/checkout@v4
 
@@ -123,18 +139,34 @@ jobs:
   cross-job:
     name: cross-job download
     needs: roundtrip
-    runs-on: ubuntu-devino
+    # Was `ubuntu-devino`; moved to a hosted runner for the reason given on the
+    # roundtrip matrix above. What this job actually tests is that one job can
+    # fetch artifacts uploaded by *other* jobs on *other* operating systems,
+    # which does not depend on where this job itself runs.
+    runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
       - name: Download every single-* artifact from all OSes
         uses: ./download
         with:
           pattern: single-*
           path: all
-      - name: Verify all four
+      - name: Verify all three
         shell: bash
         run: |
           ls -la all
-          for os in ubuntu-latest windows-latest macos-latest ubuntu-devino; do
+          for os in ubuntu-latest windows-latest macos-latest; do
             test -f "all/single-$os/a.txt" || { echo "missing $os"; exit 1; }
           done
+
+  unit:
+    # Pure-python, mocks subprocess: no network, no mc, no OIDC, no runner pool.
+    name: unit tests
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Run unit tests
+        shell: bash
+        run: |
+          PY="$(command -v python3 || command -v python)"
+          "$PY" -m unittest discover -s tests -p "test_*.py" -v

README.md

@@ -51,6 +51,21 @@ up to 1/3/5/7/14/30/90 days and stored as an object tag (values above 90 are
 capped at 90). When `retention-days` is not set the artifact expires after
 14 days, which matches how the org's test reports and screenshots are used.
 
+## Transient failures
+
+Every `mc` call is retried when it fails for a transport reason — connection
+reset, refused or timed out, TLS handshake timeout, DNS failure, or a 5xx from
+the endpoint. Five attempts, exponential backoff (2, 4, 8, 16 s) with +/- 25%
+jitter. Both verbs are safe to repeat: `cp` writes a whole object under a key
+derived from the run, and `ls` is read-only.
+
+A real 404 (`NoSuchKey`, "object does not exist") and a rejected credential are
+**never** retried — they are deterministic answers, so retrying only delays the
+report. The download step also words its error after the real cause: only a
+genuine 404 says `Artifact not found`. Anything else leads with `mc`'s own
+message, so a network failure is not mistaken for an upload that never
+happened.
+
 ## Inputs
 
 `upload`: `name`, `path`, `if-no-files-found`, `retention-days`,

lib/main.py

@@ -17,6 +17,7 @@
 import os
 import pathlib
 import platform
+import random
 import re
 import shutil
 import subprocess
@@ -40,6 +41,16 @@
 RETENTION_BUCKETS = [1, 3, 5, 7, 14, 30, 90]
 GLOB_CHARS = set("*?[")
 
+# `mc` does not retry, and this action is now on the critical path of required
+# checks in every repository that moved off the GitHub artifact sink. A single
+# TCP reset against the endpoint therefore reds a gate fleet-wide, so transport
+# failures are retried with exponential backoff. MC_ATTEMPTS attempts means
+# MC_ATTEMPTS - 1 waits: 2, 4, 8 and 16 s by default. The 32 s step is only
+# reached if MC_ATTEMPTS is raised.
+MC_ATTEMPTS = 5
+MC_BACKOFF = [2, 4, 8, 16, 32]
+MC_JITTER = 0.25  # +/- 25%, so a fleet-wide blip does not retry in lockstep
+
 
 # ── GitHub Actions helpers ───────────────────────────────────────────────────
 def log(msg):
@@ -121,6 +132,101 @@ def http(req, timeout=120, retries=3):
 
 
 # ── mc client ────────────────────────────────────────────────────────────────
+# `mc` reports every failure on stderr with the same `mc: <ERROR> ...` shape and
+# exits 1, so the exit code alone cannot tell a missing object from a dead
+# socket. These patterns read the message instead. They are ordered: a DNS
+# failure says "no such host" and must not be read as a missing object, and a
+# 503 must not be read as a permission problem.
+MC_TRANSPORT_RE = re.compile(
+    r"connection reset"
+    r"|connection refused"
+    r"|connection timed out"
+    r"|broken pipe"
+    r"|i/o timeout"
+    r"|tls handshake timeout"
+    r"|context deadline exceeded"
+    r"|\bno such host\b"
+    r"|server misbehaving"
+    r"|temporary failure in name resolution"
+    r"|network is unreachable"
+    r"|host is unreachable"
+    r"|unexpected eof"
+    r"|:\s*EOF\b"
+    r"|bad gateway"
+    r"|service unavailable"
+    r"|gateway time-?out"
+    r"|internal server error"
+    r"|\bslow ?down\b"
+    r"|\binternalerror\b"
+    r"|\brequesttimeout\b"
+    r"|too many requests"
+    # A bare status code is only a status code when something says so. The
+    # object key is echoed in mc's error text, so an artifact literally named
+    # "coverage-503" must not be read as a 503.
+    r"|(?:status|code|responded with|returned)\s*[:=]?\s*(?:429|500|502|503|504)\b",
+    re.IGNORECASE,
+)
+MC_AUTH_RE = re.compile(
+    r"\baccess ?denied\b"
+    r"|invalidaccesskeyid"
+    r"|signaturedoesnotmatch"
+    r"|expiredtoken"
+    r"|invalidtoken"
+    r"|token has expired"
+    r"|permission denied"
+    r"|\bforbidden\b"
+    r"|\bunauthorized\b"
+    r"|(?:status|code|responded with|returned)\s*[:=]?\s*(?:401|403)\b",
+    re.IGNORECASE,
+)
+MC_NOT_FOUND_RE = re.compile(
+    r"nosuchkey"
+    r"|nosuchbucket"
+    r"|nosuchversion"
+    r"|no such object"
+    r"|does not exist"
+    r"|\bnot found\b"
+    r"|\b404\b",
+    re.IGNORECASE,
+)
+
+
+def first_line(text):
+    for line in (text or "").splitlines():
+        line = line.strip()
+        if line:
+            return line[:400]
+    return ""
+
+
+def classify_mc_error(returncode, output):
+    """Classify one `mc` invocation.
+
+    Returns "ok", "transport", "auth", "not-found" or "unknown".
+
+    Only "transport" is retried. A missing object or a rejected credential is a
+    deterministic answer: retrying it just delays the report by half a minute
+    and hides the real cause behind four more identical lines.
+    """
+    if returncode == 0:
+        return "ok"
+    text = output or ""
+    if MC_TRANSPORT_RE.search(text):
+        return "transport"
+    if MC_AUTH_RE.search(text):
+        return "auth"
+    if MC_NOT_FOUND_RE.search(text):
+        return "not-found"
+    return "unknown"
+
+
+def retry_delay(attempt, rng=None):
+    """Seconds to wait after a failed attempt (0-based). Jittered +/- MC_JITTER."""
+    base = MC_BACKOFF[min(max(attempt, 0), len(MC_BACKOFF) - 1)]
+    r = rng if rng is not None else random
+    return base * (1.0 - MC_JITTER + 2.0 * MC_JITTER * r.random())
+
+
 def ensure_mc(endpoint):
     osn = os.environ.get("RUNNER_OS") or platform.system()
     arch = os.environ.get("RUNNER_ARCH") or platform.machine()
@@ -228,13 +334,42 @@ def credentials(self):
         mask(creds.get("SessionToken", ""))
         return creds["AccessKeyId"], creds["SecretAccessKey"], creds.get("SessionToken", "")
 
-    def run(self, *args, check=True):
+    # Set by run(); read by callers that need to word an error correctly.
+    last_error_kind = "ok"
+    last_attempts = 0
+
+    def run(self, *args, check=True, attempts=None):
+        """Run one `mc` command, retrying transport failures only.
+
+        Both verbs this action uses are safe to repeat: `mc cp` writes a whole
+        object under a key derived from the run, and `mc ls` is read-only.
+        """
+        attempts = MC_ATTEMPTS if attempts is None else max(1, int(attempts))
         cmd = [self.mc, "--config-dir", self.cfg, "--no-color", "--disable-pager"] + list(args)
-        p = subprocess.run(cmd, env=self.env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
-        out = p.stdout.decode("utf-8", "replace")
-        if check and p.returncode != 0:
-            fail("mc %s failed (exit %d):\n%s" % (args[0], p.returncode, out.strip()))
-        return p.returncode, out
+        rc, out, kind = 1, "", "unknown"
+        for attempt in range(attempts):
+            p = subprocess.run(cmd, env=self.env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+            rc = p.returncode
+            out = p.stdout.decode("utf-8", "replace")
+            kind = classify_mc_error(rc, out)
+            self.last_error_kind = kind
+            self.last_attempts = attempt + 1
+            if kind == "ok":
+                return rc, out
+            if kind != "transport" or attempt == attempts - 1:
+                break
+            delay = retry_delay(attempt)
+            warn(
+                "mc %s: transport error, retrying in %.1fs (attempt %d of %d): %s"
+                % (args[0], delay, attempt + 2, attempts, first_line(out))
+            )
+            time.sleep(delay)
+        if check:
+            fail(
+                "mc %s failed after %d attempt(s) (exit %d, %s error):\n%s"
+                % (args[0], self.last_attempts, rc, kind, out.strip())
+            )
+        return rc, out
 
     def target(self, key):
         return "devino/%s/%s" % (self.bucket, key)
@@ -440,6 +575,30 @@ def safe_extract(archive, dest):
     return len(members)
 
 
+def download_error_message(name, bucket, key, kind, attempts, output):
+    """Word a failed download after its real cause.
+
+    A genuine 404 keeps the familiar "Artifact not found". Anything else leads
+    with `mc`'s own text, because announcing a TCP reset as a missing artifact
+    sends whoever reads the annotation looking for an upload that exists and
+    succeeded.
+    """
+    detail = (output or "").strip()
+    if kind == "not-found":
+        return "Artifact not found: %s (s3://%s/%s)\n%s" % (name, bucket, key, detail)
+    label = {
+        "transport": "transport error reaching the storage endpoint",
+        "auth": "authorization error",
+    }.get(kind, "mc error")
+    # ASCII only: fail() prints to stdout, and on Windows that is the console
+    # code page (cp1252 before Python 3.15), so a non-ASCII character here would
+    # raise UnicodeEncodeError inside the error path itself.
+    return (
+        "Could not download artifact %s (s3://%s/%s) after %d attempt(s): %s, "
+        "not a missing artifact.\n%s" % (name, bucket, key, attempts, label, detail)
+    )
+
+
 def do_download():
     name = inp("name", "").strip()
     pattern = inp("pattern", "").strip()
@@ -478,7 +637,7 @@ def do_download():
         local = os.path.join(work, n + ".tgz")
         rc, out = store.run("cp", "--quiet", store.target(key), local, check=False)
         if rc != 0:
-            fail("Artifact not found: %s (s3://%s/%s)\n%s" % (n, store.bucket, key, out.strip()))
+            fail(download_error_message(n, store.bucket, key, store.last_error_kind, store.last_attempts, out))
         target = dest if (name or merge) else dest / n
         count = safe_extract(local, target)
         log("Downloaded %s (%s, %d entries) to %s" % (n, human(os.path.getsize(local)), count, target))