From cccc175155417c437829421eb1595defed8536b4 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Fri, 11 Sep 2026 16:50:45 -0400 Subject: [PATCH 1/5] fix(mc): retry transport failures, stop calling them "Artifact not found" 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. --- .github/workflows/ci.yml | 12 ++ README.md | 15 ++ lib/main.py | 164 +++++++++++++++++- tests/test_mc_retry.py | 356 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 540 insertions(+), 7 deletions(-) create mode 100644 tests/test_mc_retry.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dfa037b..a1022fa 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -138,3 +138,15 @@ jobs: for os in ubuntu-latest windows-latest macos-latest ubuntu-devino; 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 diff --git a/README.md b/README.md index d938b52..17467d8 100644 --- a/README.md +++ b/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`, diff --git a/lib/main.py b/lib/main.py index ed4888a..f8ac718 100644 --- a/lib/main.py +++ b/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,95 @@ def http(req, timeout=120, retries=3): # ── mc client ──────────────────────────────────────────────────────────────── +# `mc` reports every failure on stderr with the same `mc: ...` 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"|\bunexpected eof\b" + r"|\bEOF\b" + r"|bad gateway" + r"|service unavailable" + r"|gateway time-?out" + r"|\bslow ?down\b" + r"|\binternalerror\b" + r"|\brequesttimeout\b" + r"|too many requests" + r"|\b(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"|\b(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 +328,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 +569,27 @@ 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") + 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 +628,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)) diff --git a/tests/test_mc_retry.py b/tests/test_mc_retry.py new file mode 100644 index 0000000..4e508cc --- /dev/null +++ b/tests/test_mc_retry.py @@ -0,0 +1,356 @@ +"""Unit tests for `mc` error classification and the transport retry. + +Standard library only, like lib/main.py itself: `python -m unittest discover -s tests`. +Every test mocks `subprocess.run`, so nothing here touches the network, the +storage endpoint, `mc`, or the OIDC flow. + +The transport sample is the real failure from +DevinoSolutions/stealth-chrome-devtools-mcp run 34640838095, job 103404145715, +which a passing upload reported as "Artifact not found". +""" +import contextlib +import importlib.util +import io +import os +import sys +import tempfile +import unittest +from unittest import mock + +ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +_spec = importlib.util.spec_from_file_location( + "devino_artifact_main", os.path.join(ROOT, "lib", "main.py") +) +main = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(main) + + +RESET = ( + "mc: Unable to prepare URL for copying. Get " + '"https://storage.devino.ca/gh-artifacts/?location=": read tcp ' + "10.1.0.14:44618->172.67.206.3:443: read: connection reset by peer" +) +MISSING = ( + "mc: Unable to validate source " + "`devino/gh-artifacts/o/r/1/missing.tgz`. Object does not exist." +) +DENIED = "mc: Unable to copy. Access Denied." + + +class FakeCompleted(object): + def __init__(self, returncode, stdout): + self.returncode = returncode + self.stdout = stdout + + +class FakeRun(object): + """Replays a list of (returncode, output); repeats the last entry forever.""" + + def __init__(self, *results): + self.results = list(results) + self.calls = [] + + def __call__(self, cmd, env=None, stdout=None, stderr=None, **kwargs): + self.calls.append(list(cmd)) + rc, out = self.results[min(len(self.calls) - 1, len(self.results) - 1)] + return FakeCompleted(rc, out.encode("utf-8")) + + +def make_store(): + """A Store with no __init__: no mc download, no OIDC exchange, no network.""" + s = main.Store.__new__(main.Store) + s.mc = "mc" + s.cfg = os.path.join(tempfile.gettempdir(), "devino-test-cfg") + s.env = {} + s.bucket = "gh-artifacts" + s.endpoint = "https://storage.example.invalid" + return s + + +@contextlib.contextmanager +def harness(*results): + """Patch subprocess.run and time.sleep; yield (store, runner, sleeps, out).""" + runner = FakeRun(*results) + sleeps = [] + buf = io.StringIO() + with mock.patch.object(main.subprocess, "run", runner), mock.patch.object( + main.time, "sleep", sleeps.append + ), contextlib.redirect_stdout(buf): + yield make_store(), runner, sleeps, buf + + +class TestClassify(unittest.TestCase): + def test_success_is_ok(self): + self.assertEqual(main.classify_mc_error(0, ""), "ok") + self.assertEqual(main.classify_mc_error(0, MISSING), "ok") + + def test_the_observed_reset_is_transport(self): + self.assertEqual(main.classify_mc_error(1, RESET), "transport") + + def test_other_transport_shapes(self): + for text in ( + "mc: dial tcp 1.2.3.4:443: connect: connection refused", + "mc: Get https://s/: net/http: TLS handshake timeout", + "mc: read tcp: i/o timeout", + "mc: context deadline exceeded", + "mc: Put https://s/: EOF", + "mc: 503 Service Unavailable", + "mc: 502 Bad Gateway", + "mc: Please reduce your request rate. SlowDown", + "mc: We encountered an internal error, please try again: InternalError", + "mc: write: broken pipe", + "mc: connect: network is unreachable", + ): + self.assertEqual(main.classify_mc_error(1, text), "transport", text) + + def test_missing_object_is_not_found(self): + for text in ( + MISSING, + "mc: Unable to stat. The specified key does not exist.", + "mc: NoSuchKey", + "mc: The specified bucket does not exist. NoSuchBucket", + "mc: 404 Not Found", + ): + self.assertEqual(main.classify_mc_error(1, text), "not-found", text) + + def test_credentials_are_auth_not_transport(self): + for text in ( + DENIED, + "mc: InvalidAccessKeyId", + "mc: SignatureDoesNotMatch", + "mc: ExpiredToken: the security token has expired", + "mc: 403 Forbidden", + ): + self.assertEqual(main.classify_mc_error(1, text), "auth", text) + + def test_dns_failure_is_transport_not_not_found(self): + # "no such host" contains no 404 wording, but a naive not-found rule + # that fired on "no such" would misread it. Ordering guard. + text = "mc: dial tcp: lookup storage.devino.ca: no such host" + self.assertEqual(main.classify_mc_error(1, text), "transport") + + def test_unrecognised_failure_is_unknown(self): + self.assertEqual(main.classify_mc_error(1, "mc: something new"), "unknown") + + def test_unknown_output_is_never_silently_ok(self): + self.assertNotEqual(main.classify_mc_error(1, ""), "ok") + + +class TestRetryDelay(unittest.TestCase): + def test_schedule_is_exponential(self): + rng = mock.Mock() + rng.random.return_value = 0.5 # no jitter at the midpoint + got = [main.retry_delay(i, rng=rng) for i in range(5)] + self.assertEqual(got, [2.0, 4.0, 8.0, 16.0, 32.0]) + + def test_jitter_stays_within_25_percent(self): + for i, base in enumerate(main.MC_BACKOFF): + for value in (0.0, 0.5, 1.0): + rng = mock.Mock() + rng.random.return_value = value + d = main.retry_delay(i, rng=rng) + self.assertGreaterEqual(d, base * 0.75) + self.assertLessEqual(d, base * 1.25) + + def test_jitter_actually_varies(self): + values = {round(main.retry_delay(0), 6) for _ in range(50)} + self.assertGreater(len(values), 1) + + def test_index_is_clamped(self): + rng = mock.Mock() + rng.random.return_value = 0.5 + self.assertEqual(main.retry_delay(99, rng=rng), float(main.MC_BACKOFF[-1])) + self.assertEqual(main.retry_delay(-3, rng=rng), float(main.MC_BACKOFF[0])) + + +class TestStoreRun(unittest.TestCase): + def test_success_runs_once_and_never_sleeps(self): + with harness((0, "done")) as (store, runner, sleeps, _): + rc, out = store.run("cp", "--quiet", "src", "dst") + self.assertEqual(rc, 0) + self.assertEqual(out, "done") + self.assertEqual(len(runner.calls), 1) + self.assertEqual(sleeps, []) + self.assertEqual(store.last_error_kind, "ok") + self.assertEqual(store.last_attempts, 1) + + def test_transport_error_then_success(self): + with harness((1, RESET), (1, RESET), (0, "ok")) as (store, runner, sleeps, buf): + rc, _ = store.run("cp", "--quiet", "src", "dst") + self.assertEqual(rc, 0) + self.assertEqual(len(runner.calls), 3) + self.assertEqual(len(sleeps), 2) + self.assertLess(sleeps[0], sleeps[1]) # backoff grows + self.assertGreaterEqual(sleeps[0], 2 * 0.75) + self.assertLessEqual(sleeps[1], 4 * 1.25) + self.assertEqual(store.last_attempts, 3) + self.assertIn("::warning::", buf.getvalue()) + self.assertIn("retrying", buf.getvalue()) + + def test_transport_error_exhausts_attempts_then_fails(self): + with harness((1, RESET)) as (store, runner, sleeps, buf): + with self.assertRaises(SystemExit) as cm: + store.run("cp", "--quiet", "src", "dst") + self.assertEqual(cm.exception.code, 1) + self.assertEqual(len(runner.calls), main.MC_ATTEMPTS) + self.assertEqual(len(sleeps), main.MC_ATTEMPTS - 1) + text = buf.getvalue() + self.assertIn("failed after %d attempt(s)" % main.MC_ATTEMPTS, text) + self.assertIn("transport error", text) + self.assertIn("connection reset by peer", text) # mc's text, verbatim + + def test_not_found_is_never_retried(self): + with harness((1, MISSING)) as (store, runner, sleeps, buf): + with self.assertRaises(SystemExit): + store.run("cp", "--quiet", "src", "dst") + self.assertEqual(len(runner.calls), 1) + self.assertEqual(sleeps, []) + self.assertIn("not-found error", buf.getvalue()) + + def test_auth_failure_is_never_retried(self): + with harness((1, DENIED)) as (store, runner, sleeps, _): + with self.assertRaises(SystemExit): + store.run("cp", "--quiet", "src", "dst") + self.assertEqual(len(runner.calls), 1) + self.assertEqual(sleeps, []) + self.assertEqual(store.last_error_kind, "auth") + + def test_unknown_failure_is_never_retried(self): + with harness((1, "mc: what")) as (store, runner, sleeps, _): + with self.assertRaises(SystemExit): + store.run("cp", "--quiet", "src", "dst") + self.assertEqual(len(runner.calls), 1) + self.assertEqual(sleeps, []) + + def test_check_false_returns_instead_of_exiting(self): + with harness((1, MISSING)) as (store, runner, sleeps, _): + rc, out = store.run("cp", "--quiet", "src", "dst", check=False) + self.assertEqual(rc, 1) + self.assertIn("does not exist", out) + self.assertEqual(store.last_error_kind, "not-found") + self.assertEqual(store.last_attempts, 1) + + def test_check_false_still_retries_transport_errors(self): + with harness((1, RESET)) as (store, runner, sleeps, _): + rc, _ = store.run("cp", "--quiet", "src", "dst", check=False) + self.assertEqual(rc, 1) + self.assertEqual(len(runner.calls), main.MC_ATTEMPTS) + self.assertEqual(store.last_error_kind, "transport") + + def test_attempts_override(self): + with harness((1, RESET)) as (store, runner, sleeps, _): + with self.assertRaises(SystemExit): + store.run("ls", "--json", "dst", attempts=2) + self.assertEqual(len(runner.calls), 2) + self.assertEqual(len(sleeps), 1) + + def test_listing_is_retried_too(self): + with harness((1, RESET), (0, "{}")) as (store, runner, sleeps, _): + rc, _ = store.run("ls", "--json", "dst") + self.assertEqual(rc, 0) + self.assertEqual(len(runner.calls), 2) + + +class TestUploadPath(unittest.TestCase): + """The upload path shells out through the same Store.run, so it inherits + the retry. These assert the upload's own argument shape survives it.""" + + def test_tagged_upload_retries_transport_and_succeeds(self): + args = ["cp", "--quiet", "--tags", "retention=7", "/tmp/a.tgz", "devino/gh-artifacts/o/r/1/a.tgz"] + with harness((1, RESET), (0, "")) as (store, runner, sleeps, _): + rc, _ = store.run(*args) + self.assertEqual(rc, 0) + self.assertEqual(len(runner.calls), 2) + self.assertEqual(len(sleeps), 1) + for call in runner.calls: + self.assertEqual(call[-len(args):], args) # same command each time + self.assertIn("--tags", call) + + def test_upload_not_found_bucket_is_not_retried(self): + # A bucket that does not exist is a configuration error, not a blip. + text = "mc: Unable to copy. The specified bucket does not exist." + with harness((1, text)) as (store, runner, sleeps, buf): + with self.assertRaises(SystemExit): + store.run("cp", "--quiet", "/tmp/a.tgz", "devino/nope/k.tgz") + self.assertEqual(len(runner.calls), 1) + self.assertEqual(sleeps, []) + + +class TestDownloadErrorMessage(unittest.TestCase): + def test_real_404_keeps_the_familiar_wording(self): + msg = main.download_error_message("a", "gh-artifacts", "o/r/1/a.tgz", "not-found", 1, MISSING) + self.assertTrue(msg.startswith("Artifact not found: a (s3://gh-artifacts/o/r/1/a.tgz)")) + self.assertIn("does not exist", msg) + + def test_transport_failure_does_not_claim_the_artifact_is_missing(self): + msg = main.download_error_message( + "release-evidence-install-smoke-sdist-Linux-X64", + "gh-artifacts", + "DevinoSolutions/stealth-chrome-devtools-mcp/34640838095/" + "release-evidence-install-smoke-sdist-Linux-X64.tgz", + "transport", + main.MC_ATTEMPTS, + RESET, + ) + self.assertNotIn("Artifact not found", msg) + self.assertIn("not a missing artifact", msg) + self.assertIn("connection reset by peer", msg) + self.assertIn("after %d attempt(s)" % main.MC_ATTEMPTS, msg) + + def test_auth_failure_is_labelled_as_such(self): + msg = main.download_error_message("a", "b", "k", "auth", 1, DENIED) + self.assertNotIn("Artifact not found", msg) + self.assertIn("authorization error", msg) + + def test_unknown_failure_still_shows_mc_output_first(self): + msg = main.download_error_message("a", "b", "k", "unknown", 1, "mc: weird") + self.assertNotIn("Artifact not found", msg) + self.assertIn("mc: weird", msg) + + +class TestDoDownload(unittest.TestCase): + """End to end through do_download, so the call site is covered and not just + the helper. Store is replaced with a pre-built one, so no network.""" + + def _run(self, store, results): + runner = FakeRun(*results) + sleeps = [] + buf = io.StringIO() + env = { + "GITHUB_REPOSITORY": "DevinoSolutions/stealth-chrome-devtools-mcp", + "GITHUB_RUN_ID": "34640838095", + "INPUT_NAME": "release-evidence-install-smoke-sdist-Linux-X64", + "INPUT_PATH": tempfile.mkdtemp(prefix="devino-dl-"), + } + with mock.patch.dict(os.environ, env, clear=True), mock.patch.object( + main.subprocess, "run", runner + ), mock.patch.object(main.time, "sleep", sleeps.append), mock.patch.object( + main, "Store", lambda: store + ), contextlib.redirect_stdout(buf): + with self.assertRaises(SystemExit): + main.do_download() + return runner, sleeps, buf.getvalue() + + def test_transport_error_is_reported_verbatim_and_retried(self): + store = make_store() + runner, sleeps, text = self._run(store, [(1, RESET)]) + self.assertEqual(len(runner.calls), main.MC_ATTEMPTS) + self.assertEqual(len(sleeps), main.MC_ATTEMPTS - 1) + errors = [ln for ln in text.splitlines() if ln.startswith("::error::")] + self.assertEqual(len(errors), 1) + self.assertNotIn("Artifact not found", errors[0]) + self.assertIn("not a missing artifact", errors[0]) + self.assertIn("connection reset by peer", text) + + def test_genuine_missing_artifact_keeps_the_old_message(self): + store = make_store() + runner, sleeps, text = self._run(store, [(1, MISSING)]) + self.assertEqual(len(runner.calls), 1) + self.assertEqual(sleeps, []) + errors = [ln for ln in text.splitlines() if ln.startswith("::error::")] + self.assertEqual(len(errors), 1) + self.assertTrue(errors[0].startswith("::error::Artifact not found: ")) + + +if __name__ == "__main__": + unittest.main(verbosity=2) From 79462cd1f5d73a2d2790420dcb3cfe8ca3c14cfc Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Fri, 11 Sep 2026 16:55:10 -0400 Subject: [PATCH 2/5] fix(mc): do not read digits in an artifact name as an HTTP status code 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. --- lib/main.py | 14 ++++++++++---- tests/test_mc_retry.py | 26 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/lib/main.py b/lib/main.py index f8ac718..e745b35 100644 --- a/lib/main.py +++ b/lib/main.py @@ -150,16 +150,20 @@ def http(req, timeout=120, retries=3): r"|temporary failure in name resolution" r"|network is unreachable" r"|host is unreachable" - r"|\bunexpected eof\b" - r"|\bEOF\b" + 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" - r"|\b(429|500|502|503|504)\b", + # 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( @@ -170,7 +174,9 @@ def http(req, timeout=120, retries=3): r"|invalidtoken" r"|token has expired" r"|permission denied" - r"|\b(401|403)\b", + 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( diff --git a/tests/test_mc_retry.py b/tests/test_mc_retry.py index 4e508cc..bfb6bfe 100644 --- a/tests/test_mc_retry.py +++ b/tests/test_mc_retry.py @@ -96,6 +96,8 @@ def test_other_transport_shapes(self): "mc: Put https://s/: EOF", "mc: 503 Service Unavailable", "mc: 502 Bad Gateway", + "mc: server responded with 503", + "mc: status code: 504", "mc: Please reduce your request rate. SlowDown", "mc: We encountered an internal error, please try again: InternalError", "mc: write: broken pipe", @@ -129,6 +131,30 @@ def test_dns_failure_is_transport_not_not_found(self): text = "mc: dial tcp: lookup storage.devino.ca: no such host" self.assertEqual(main.classify_mc_error(1, text), "transport") + def test_digits_in_an_artifact_name_are_not_status_codes(self): + # mc echoes the object key, so a bare numeric rule would read these as + # 5xx and retry a missing object five times under the wrong label. + for name in ("coverage-503", "shard-502", "e2e-500", "build-429", "run-504"): + text = ( + "mc: Unable to validate source " + "`devino/gh-artifacts/o/r/1/%s.tgz`. Object does not exist." % name + ) + self.assertEqual(main.classify_mc_error(1, text), "not-found", name) + + def test_digits_in_an_artifact_name_are_not_auth_codes(self): + for name in ("coverage-403", "smoke-401"): + text = ( + "mc: Unable to validate source " + "`devino/gh-artifacts/o/r/1/%s.tgz`. Object does not exist." % name + ) + self.assertEqual(main.classify_mc_error(1, text), "not-found", name) + + def test_socket_details_are_not_status_codes(self): + # The observed reset carries ports and IPs; none of them is an HTTP code. + self.assertEqual(main.classify_mc_error(1, RESET), "transport") + quiet = "mc: Unable to stat `devino/gh-artifacts/o/r/500/a.tgz`. Object does not exist." + self.assertEqual(main.classify_mc_error(1, quiet), "not-found") + def test_unrecognised_failure_is_unknown(self): self.assertEqual(main.classify_mc_error(1, "mc: something new"), "unknown") From a738f4792405b8dc712f74e2b11253ec645a81f0 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Fri, 11 Sep 2026 16:58:08 -0400 Subject: [PATCH 3/5] fix(download): keep the error message ASCII so it survives Windows stdout 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. --- lib/main.py | 7 +++++-- tests/test_mc_retry.py | 8 ++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/main.py b/lib/main.py index e745b35..5d7b6f6 100644 --- a/lib/main.py +++ b/lib/main.py @@ -590,9 +590,12 @@ def download_error_message(name, bucket, key, kind, attempts, output): "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) + "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) ) diff --git a/tests/test_mc_retry.py b/tests/test_mc_retry.py index bfb6bfe..6465a21 100644 --- a/tests/test_mc_retry.py +++ b/tests/test_mc_retry.py @@ -328,6 +328,14 @@ def test_auth_failure_is_labelled_as_such(self): self.assertNotIn("Artifact not found", msg) self.assertIn("authorization error", msg) + def test_every_message_is_ascii_encodable(self): + # fail() prints to stdout. On Windows that is cp1252 before Python 3.15, + # so a stray em-dash would raise UnicodeEncodeError inside the error + # path and replace a useful message with a traceback. + for kind in ("not-found", "transport", "auth", "unknown"): + msg = main.download_error_message("a", "b", "k", kind, 5, RESET) + msg.encode("ascii") # raises on any non-ASCII character + def test_unknown_failure_still_shows_mc_output_first(self): msg = main.download_error_message("a", "b", "k", "unknown", 1, "mc: weird") self.assertNotIn("Artifact not found", msg) From c5513a65aec3ec81e4b4a2c88eb1d6a8cfb50d4e Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Fri, 11 Sep 2026 17:41:52 -0400 Subject: [PATCH 4/5] ci: add a concurrency group so a superseded run releases its pool slot 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. --- .github/workflows/ci.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a1022fa..328b81f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,14 @@ permissions: contents: read id-token: write +# The roundtrip matrix includes ubuntu-devino, so without this every push to +# every branch left a job queued on a saturated self-hosted pool that nothing +# cancelled. 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 }}) From 3fd2497b92ab1a5be1b5258376ce2333bcdd8907 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Fri, 11 Sep 2026 20:34:01 -0400 Subject: [PATCH 5/5] ci: take this workflow off the self-hosted pool, it can never be scheduled 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. --- .github/workflows/ci.yml | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 328b81f..4d94417 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,10 +10,11 @@ permissions: contents: read id-token: write -# The roundtrip matrix includes ubuntu-devino, so without this every push to -# every branch left a job queued on a saturated self-hosted pool that nothing -# cancelled. Group name matches the org's prevailing ci.yml style; main is -# exempt from cancellation so a push there always finishes validating. +# 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' }} @@ -24,8 +25,15 @@ jobs: 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 @@ -131,7 +139,11 @@ 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 @@ -139,11 +151,11 @@ jobs: 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