From f844bde086fb6215037c345c42a09be3c1b3a2a5 Mon Sep 17 00:00:00 2001 From: paulieb89 Date: Fri, 4 Sep 2026 10:35:38 +0100 Subject: [PATCH] ci: retry a failed Fly deploy, and fail loudly when the two apps disagree On the v1.15.1 release the PyPI publish and the propertydata deploy succeeded while the property-shared deploy failed twice, five minutes apart, on Depot builder timeouts: error releasing builder: deadline_exceeded: context deadline exceeded timed out connecting to machine: failed to list workers: Unavailable: ... authentication handshake failed: EOF The two deploys are independent leaf jobs, so one failing while the other succeeds is a NORMAL terminal state of this graph. Nothing observed the pair. The apps ran different versions for about an hour, with the app that needed a critical hotfix still serving the broken build, until a human noticed and deployed manually with `--depot=false`. Two changes, addressing the cause and the silence separately. scripts/deploy_fly.py -- attempt with Depot (the default and faster path; one intermittent incident is not a reason to discard it), then fall back twice to `--depot=false`, which is the one causally relevant difference from the manual recovery. Each attempt is bounded by its own 900 s timeout: both observed failures were timeouts, so without a per-attempt bound one hang consumes the job budget and the fallback never runs, making the retry decorative. `--app` and `--config` are now always explicit; CI previously relied on cwd discovery of fly.toml, so the command that ran was not the command anyone had written. scripts/verify_release.py -- a reconcile job needing BOTH deploys, which polls each app's /.well-known/mcp/server-card.json until every target reports the released version. `if: !cancelled()`, so it runs precisely when a deploy has failed; not `always()`, so a cancelled run is not reported as drift. Both apps expose the same version field from the same installed distribution (verified live 2026-09-04); propertydata's /health carries no version and cannot serve. It separates two failures that need different responses: a tag/pyproject mismatch fails immediately with its own message, because the version is baked in at build time and no amount of polling can fix it, while release drift is reported with both observed versions. It requires two CONSECUTIVE agreeing rounds, because Fly's proxy may front more than one Machine and a single 200 cannot distinguish "propagated" from "hit the updated Machine". Each app runs exactly one Machine today, which is an observation rather than a guarantee, so the defence stays. Both are stdlib-only operator scripts loaded by path in tests and driven through injected runners, following scripts/fly_observability_snapshot.py -- the suite never shells out and never needs a Fly token. Retry logic in workflow YAML cannot be tested at all, and the `if: failure()` two-step alternative needs continue-on-error, renders a failed first attempt as a misleading green, and writes the flyctl arguments twice; two copies drifting apart is the failure class this change exists to close. setup-flyctl pinned from @master to ed8efb3 (tag 1.6). That job holds a production deploy token and @master runs whatever is on the branch at that moment. The SHA is what @master already resolved to, so the pin changes nothing but reproducibility. The flyctl binary stays unpinned: it talks to a moving server API, so pinning it would turn a rare flake into certain rot. test_the_release_workflow_still_deploys_both_apps asserted the literal inline `flyctl deploy --remote-only --ha=false`, which has moved into the script. The fact it guards -- every release still deploys propertydata, which the Stage 1 runbook's honesty depends on -- is unchanged, so the assertions were moved to where each half now lives rather than relaxed: the app/config pairing is pinned in the workflow, the flags in the script. Note this cannot be verified end to end without a real release; the ladder and the reconcile logic are covered by unit tests, the workflow wiring is exercised on the next release. ./scripts/validate.sh: 2040 passed, 27 skipped. --- .github/workflows/release.yml | 46 ++++++- .gitignore | 2 + scripts/deploy_fly.py | 127 ++++++++++++++++++ scripts/verify_release.py | 173 ++++++++++++++++++++++++ tests/snapshot/test_stage1_shadow.py | 15 ++- tests/test_deploy_fly.py | 153 +++++++++++++++++++++ tests/test_verify_release.py | 194 +++++++++++++++++++++++++++ 7 files changed, 704 insertions(+), 6 deletions(-) create mode 100644 scripts/deploy_fly.py create mode 100644 scripts/verify_release.py create mode 100644 tests/test_deploy_fly.py create mode 100644 tests/test_verify_release.py diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c4cb978..02394a3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -28,10 +28,22 @@ jobs: name: Deploy property-shared to Fly runs-on: ubuntu-latest needs: publish + timeout-minutes: 45 steps: - uses: actions/checkout@v4 - - uses: superfly/flyctl-actions/setup-flyctl@master - - run: flyctl deploy --remote-only --ha=false + # Pinned by SHA rather than @master: this job holds a production deploy + # token, and @master runs whatever is on that branch at that moment. The + # SHA below is tag 1.6 / v1, which is what @master already resolved to + # when this was pinned, so the pin changes nothing except reproducibility. + # The flyctl BINARY is deliberately unpinned -- it talks to a moving + # server API, so pinning it turns a rare flake into certain rot. + - uses: superfly/flyctl-actions/setup-flyctl@ed8efb33836e8b2096c7fd3ba1c8afe303ebbff1 # 1.6 + # One step, not a `run:` plus an `if: failure()` retry step: a failed step + # fails the job even when a later one succeeds, so that shape needs + # continue-on-error and then reports a misleading green. The ladder lives + # in a script so it can be tested -- see tests/test_deploy_fly.py. + - name: Deploy (Depot, then --depot=false fallback) + run: python3 scripts/deploy_fly.py --app property-shared --config fly.toml env: FLY_API_TOKEN: ${{ secrets.FLY_API_TOKEN }} @@ -39,9 +51,35 @@ jobs: name: Deploy propertydata to Fly runs-on: ubuntu-latest needs: publish + timeout-minutes: 45 steps: - uses: actions/checkout@v4 - - uses: superfly/flyctl-actions/setup-flyctl@master - - run: flyctl deploy --config fly.app.toml --remote-only --ha=false + - uses: superfly/flyctl-actions/setup-flyctl@ed8efb33836e8b2096c7fd3ba1c8afe303ebbff1 # 1.6 + - name: Deploy (Depot, then --depot=false fallback) + run: python3 scripts/deploy_fly.py --app propertydata --config fly.app.toml env: FLY_API_TOKEN: ${{ secrets.FLY_API_TOKEN_PROPERTYDATA }} + + reconcile: + name: Both apps must serve the released version + runs-on: ubuntu-latest + needs: [deploy, deploy-propertydata] + # `!cancelled()`, not the default. On v1.15.1 one deploy failed and one + # succeeded -- a normal terminal state of this graph -- so the job that + # would have noticed must run precisely when a deploy has failed. Not + # `always()`, so a cancelled run is not reported as version drift. + # + # If `publish` fails both deploys skip and this still fails, because neither + # app serves the tag. That is correct: a release that shipped nothing must + # not show green. + if: ${{ !cancelled() }} + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + - name: Reconcile deployed versions against the release tag + run: | + python3 scripts/verify_release.py \ + --expect "${{ github.event.release.tag_name }}" \ + --target property-shared=https://property-shared.fly.dev \ + --target propertydata=https://propertydata.fly.dev \ + --attempts 20 --interval 15 --require-consecutive 2 diff --git a/.gitignore b/.gitignore index b852df7..946923e 100644 --- a/.gitignore +++ b/.gitignore @@ -57,3 +57,5 @@ scripts/* !scripts/rightmove_address_dump.py !scripts/fly_observability_snapshot.py !scripts/validate.sh +!scripts/deploy_fly.py +!scripts/verify_release.py diff --git a/scripts/deploy_fly.py b/scripts/deploy_fly.py new file mode 100644 index 0000000..5faec7b --- /dev/null +++ b/scripts/deploy_fly.py @@ -0,0 +1,127 @@ +#!/usr/bin/env python3 +"""Deploy one Fly app, retrying past a transient builder failure. + +On the v1.15.1 release, `publish` and the `propertydata` deploy both succeeded +while the `property-shared` deploy failed twice, five minutes apart: + + Waiting for depot builder... + error releasing builder: deadline_exceeded: context deadline exceeded + Error: failed to fetch an image or build from source: error building: + timed out connecting to machine: failed to list workers: + Unavailable: ... authentication handshake failed: EOF + +Fly reported no incident, the same token had built the app five hours earlier, +and `propertydata` obtained a builder on the same run. It was treated as +transient Depot unavailability. Recovery was a manual `fly deploy` with +`--depot=false`, which is the one causally relevant difference from CI. + +So: attempt with Depot (the default, and the faster path -- one intermittent +incident is not a reason to discard it permanently), then fall back to +`--depot=false`. + +**The per-attempt timeout is load-bearing.** Both observed failures were +timeouts. Without a bound, one hung attempt consumes the whole job budget and +the fallback never runs, which would make this retry decorative. + +Why a script and not YAML: retry logic in a workflow cannot be tested, and the +`if: failure()` two-step form needs `continue-on-error` (so a failed first +attempt renders as a misleading green) and writes the flyctl arguments twice -- +and two copies drifting apart is the failure class this change exists to close. + +Duplicate-release hazard, stated rather than assumed: if an attempt released the +app and then timed out waiting, the next attempt releases again. Both apps are +stateless, run `--ha=false`, and have no migration step, so a duplicate release +is benign here. That is a property of these two apps, not of `fly deploy`. + +Stdlib only: the deploy job installs flyctl, not the project. +""" + +from __future__ import annotations + +import argparse +import subprocess +import sys +import time +from typing import Callable, Sequence + +#: Depot first, then two non-Depot attempts. +ATTEMPTS: tuple[tuple[str, ...], ...] = ((), ("--depot=false",), ("--depot=false",)) + +ATTEMPT_TIMEOUT_SECONDS = 900.0 +SLEEP_BETWEEN_ATTEMPTS = 30.0 + + +def build_argv(app: str, config: str, extra: Sequence[str] = ()) -> list[str]: + """The deploy command for one attempt. + + `--app` and `--config` are always explicit. CI previously relied on cwd + discovery of `fly.toml` for property-shared, so the command that ran was not + the command anyone had written down; the successful manual recovery named + both. + """ + return [ + "flyctl", "deploy", + "--app", app, + "--config", config, + "--remote-only", + "--ha=false", + *extra, + ] + + +def deploy( + app: str, + config: str, + *, + runner: Callable[..., subprocess.CompletedProcess] = subprocess.run, + sleep: Callable[[float], None] = time.sleep, + attempts: Sequence[Sequence[str]] = ATTEMPTS, + timeout: float = ATTEMPT_TIMEOUT_SECONDS, +) -> int: + """Run the attempt ladder. Returns a process exit code.""" + failures: list[str] = [] + + for index, extra in enumerate(attempts, start=1): + argv = build_argv(app, config, extra) + label = "depot" if not extra else "depot=false" + print(f"::group::deploy attempt {index}/{len(attempts)} ({label})", flush=True) + print(f"$ {' '.join(argv)}", flush=True) + try: + completed = runner(argv, timeout=timeout) + code = completed.returncode + except subprocess.TimeoutExpired: + code = None + failures.append(f"attempt {index} ({label}): timed out after {timeout:.0f}s") + print(f"attempt {index} timed out after {timeout:.0f}s", flush=True) + except FileNotFoundError as exc: + print("::endgroup::", flush=True) + print(f"::error::flyctl is not installed: {exc}", flush=True) + return 1 + else: + if code == 0: + print("::endgroup::", flush=True) + print(f"deployed {app} on attempt {index} ({label})", flush=True) + return 0 + failures.append(f"attempt {index} ({label}): exit {code}") + print(f"attempt {index} failed with exit {code}", flush=True) + print("::endgroup::", flush=True) + + if index < len(attempts): + sleep(SLEEP_BETWEEN_ATTEMPTS) + + print(f"::error::every deploy attempt for {app} failed", flush=True) + for line in failures: + print(f" {line}", flush=True) + return 1 + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--app", required=True) + parser.add_argument("--config", required=True) + args = parser.parse_args(argv) + return deploy(args.app, args.config) + + +if __name__ == "__main__": # pragma: no cover + sys.exit(main()) diff --git a/scripts/verify_release.py b/scripts/verify_release.py new file mode 100644 index 0000000..3bfc633 --- /dev/null +++ b/scripts/verify_release.py @@ -0,0 +1,173 @@ +#!/usr/bin/env python3 +"""Assert every app actually serves the released version. + +`release.yml` fans out to two independent leaf deploy jobs. One failing while +the other succeeds is a *normal terminal state* of that graph, which is exactly +what happened on v1.15.1: PyPI and `propertydata` succeeded, `property-shared` +failed twice, and nothing noticed. The two apps ran different versions for +roughly an hour, with the app that needed a critical hotfix still on the broken +build. This script is the job that would have noticed. + +Both apps expose the same version field (verified live 2026-09-04): + + property-shared /.well-known/mcp/server-card.json -> serverInfo.version + propertydata /.well-known/mcp/server-card.json -> serverInfo.version + +`serverInfo.name` differs (`property-data` vs `property-app`) but `version` +comes from the same installed `property-shared` distribution in both, so the +comparison is meaningful rather than coincidental. `propertydata`'s `/health` +carries no version and cannot be used for this. + +Two distinct failures, deliberately reported differently: + + * **tag/pyproject mismatch** -- the version is baked into the image at build + time, so tagging v1.18.3 without bumping pyproject.toml deploys two apps + that both honestly report 1.18.2. No amount of polling fixes that, so it is + caught up front and reported as its own thing rather than as drift. + * **release drift** -- an app is not serving the released version. + +Polling, not a single request: a completed deploy does not mean the new version +is instantly serving. And `--require-consecutive` rounds, because Fly's proxy +may front more than one Machine; a single 200 cannot distinguish "propagated" +from "you happened to hit the updated Machine". At the time of writing each app +runs exactly one Machine, which is a point-in-time observation and not a +guarantee, so the defence stays. + +Stdlib only, so the reconcile job needs no project install. +""" + +from __future__ import annotations + +import argparse +import json +import sys +import time +import tomllib +import urllib.error +import urllib.request +from pathlib import Path +from typing import Callable, Sequence + +CARD_PATH = "/.well-known/mcp/server-card.json" +REQUEST_TIMEOUT_SECONDS = 10.0 + + +def normalise_tag(tag: str) -> str: + """Strip a single leading v. + + Deliberately not `lstrip("v")`, which strips every leading v and turns the + nonsense tag `vv1.0.0` into a plausible `1.0.0`. + """ + tag = tag.strip() + return tag[1:] if tag[:1] in ("v", "V") else tag + + +def project_version(pyproject: Path) -> str: + with pyproject.open("rb") as handle: + return tomllib.load(handle)["project"]["version"] + + +def _fetch(url: str) -> str: + with urllib.request.urlopen(url, timeout=REQUEST_TIMEOUT_SECONDS) as response: + return response.read().decode("utf-8") + + +def observed_version(base_url: str, fetch: Callable[[str], str]) -> tuple[str | None, str | None]: + """Return (version, error). Exactly one is None.""" + try: + body = fetch(f"{base_url.rstrip('/')}{CARD_PATH}") + except (urllib.error.URLError, OSError, TimeoutError) as exc: + return None, f"{type(exc).__name__}: {exc}" + try: + return json.loads(body)["serverInfo"]["version"], None + except (ValueError, KeyError, TypeError) as exc: + return None, f"unreadable server card: {type(exc).__name__}: {exc}" + + +def verify( + expected: str, + targets: dict[str, str], + *, + attempts: int = 20, + interval: float = 15.0, + require_consecutive: int = 2, + fetch: Callable[[str], str] = _fetch, + sleep: Callable[[float], None] = time.sleep, +) -> int: + consecutive = 0 + last: dict[str, str] = {} + + for attempt in range(1, attempts + 1): + round_state: dict[str, str] = {} + for name, base_url in targets.items(): + version, error = observed_version(base_url, fetch) + # An unreachable target is a failure, not an unknown. Treating it as + # "not yet" would let a permanently dead app pass by timing out. + round_state[name] = version if version is not None else f"<{error}>" + last = round_state + + agreed = all(value == expected for value in round_state.values()) + consecutive = consecutive + 1 if agreed else 0 + summary = " ".join(f"{name}={value}" for name, value in sorted(round_state.items())) + print( + f"attempt {attempt}/{attempts}: {summary}" + f" (agreed {consecutive}/{require_consecutive})", + flush=True, + ) + + if consecutive >= require_consecutive: + print(f"all targets serve {expected}", flush=True) + return 0 + if attempt < attempts: + sleep(interval) + + print(f"::error::RELEASE DRIFT: expected {expected} — " + " ".join( + f"{name}={value}" for name, value in sorted(last.items()) + ), flush=True) + for name, value in sorted(last.items()): + print(f" {name:20} {value}", flush=True) + return 1 + + +def parse_target(raw: str) -> tuple[str, str]: + name, _, url = raw.partition("=") + if not name or not url: + raise argparse.ArgumentTypeError(f"expected name=url, got {raw!r}") + return name, url + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--expect", required=True, help="release tag, e.g. v1.18.2") + parser.add_argument("--target", action="append", required=True, type=parse_target) + parser.add_argument("--attempts", type=int, default=20) + parser.add_argument("--interval", type=float, default=15.0) + parser.add_argument("--require-consecutive", type=int, default=2) + parser.add_argument("--pyproject", default=str(Path(__file__).resolve().parents[1] / "pyproject.toml")) + args = parser.parse_args(argv) + + expected = normalise_tag(args.expect) + + declared = project_version(Path(args.pyproject)) + if declared != expected: + print( + f"::error::tag/pyproject mismatch: tag {args.expect!r} normalises to " + f"{expected!r} but pyproject.toml declares {declared!r}. The version is " + f"baked into the image at build time, so both apps would report " + f"{declared!r} however many times they are polled. This is a release " + f"preparation error, not a partial deploy.", + flush=True, + ) + return 1 + + return verify( + expected, + dict(args.target), + attempts=args.attempts, + interval=args.interval, + require_consecutive=args.require_consecutive, + ) + + +if __name__ == "__main__": # pragma: no cover + sys.exit(main()) diff --git a/tests/snapshot/test_stage1_shadow.py b/tests/snapshot/test_stage1_shadow.py index e1bdc10..432ed7a 100644 --- a/tests/snapshot/test_stage1_shadow.py +++ b/tests/snapshot/test_stage1_shadow.py @@ -2010,10 +2010,21 @@ def test_the_release_workflow_still_deploys_both_apps(): would become misleading in the other direction. """ workflow = (REPO / ".github" / "workflows" / "release.yml").read_text() - assert "flyctl deploy --remote-only --ha=false" in workflow - assert "--config fly.app.toml" in workflow assert "release:" in workflow and "published" in workflow + # Both apps are named explicitly in the workflow. This previously asserted + # the literal `flyctl deploy --remote-only --ha=false`, which lived inline; + # the retry ladder moved that invocation into scripts/deploy_fly.py, so the + # flags are pinned there instead (below) and the app/config pairing is + # pinned here. The fact being guarded is unchanged: every release still + # deploys propertydata as well as property-shared. + assert "--app property-shared --config fly.toml" in workflow + assert "--app propertydata --config fly.app.toml" in workflow + + deploy_script = (REPO / "scripts" / "deploy_fly.py").read_text() + assert '"--remote-only"' in deploy_script + assert '"--ha=false"' in deploy_script + def test_the_runbook_documents_the_search_level_containment_rule(): body = RUNBOOK.read_text() diff --git a/tests/test_deploy_fly.py b/tests/test_deploy_fly.py new file mode 100644 index 0000000..3e52403 --- /dev/null +++ b/tests/test_deploy_fly.py @@ -0,0 +1,153 @@ +"""Tests for the Fly deploy retry ladder. + +Loaded by path and driven through an injected runner, following +`tests/test_fly_observability_snapshot.py` -- the suite never shells out and +never needs a Fly token. + +The point of putting this ladder in a script rather than in workflow YAML is +that it can be tested at all. The `if: failure()` two-step form these tests +would not have caught is the one that reports success as failure. +""" + +from __future__ import annotations + +import importlib.util +import subprocess +import sys +from pathlib import Path +from typing import Any + +import pytest + +_MODULE_PATH = Path(__file__).resolve().parents[1] / "scripts" / "deploy_fly.py" + + +def _load_module() -> Any: + spec = importlib.util.spec_from_file_location("deploy_fly", _MODULE_PATH) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +df = _load_module() + +FAKE_TOKEN = "fm2_lJPECAAAAAAAtesttokenvalue0000000000000xyz" + + +class Runner: + """Records argv per call and returns queued exit codes (or raises).""" + + def __init__(self, outcomes: list[object]): + self.outcomes = list(outcomes) + self.calls: list[list[str]] = [] + + def __call__(self, argv, timeout=None, **kwargs): + self.calls.append(list(argv)) + outcome = self.outcomes.pop(0) + if isinstance(outcome, BaseException): + raise outcome + return subprocess.CompletedProcess(argv, outcome) + + +@pytest.fixture +def sleeps() -> list[float]: + return [] + + +@pytest.fixture +def sleep(sleeps): + return sleeps.append + + +def test_first_attempt_uses_depot_and_no_fallback_runs_on_success(sleep, sleeps): + runner = Runner([0]) + assert df.deploy("property-shared", "fly.toml", runner=runner, sleep=sleep) == 0 + assert len(runner.calls) == 1, "a successful deploy must not be repeated" + assert "--depot=false" not in runner.calls[0], "Depot is the default and faster path" + assert sleeps == [], "no backoff when nothing failed" + + +def test_depot_failure_falls_back_to_depot_false(sleep): + runner = Runner([1, 0]) + assert df.deploy("property-shared", "fly.toml", runner=runner, sleep=sleep) == 0 + assert "--depot=false" not in runner.calls[0] + assert "--depot=false" in runner.calls[1], "the flag that made manual recovery work" + + +def test_exit_status_is_zero_when_the_fallback_succeeds(sleep): + """The defect an `if: failure()` two-step form would have. + + A failed first step fails the job even when a later step succeeds, so that + shape needs continue-on-error and then reports a misleading green. + """ + runner = Runner([1, 0]) + assert df.deploy("propertydata", "fly.app.toml", runner=runner, sleep=sleep) == 0 + + +def test_exit_status_is_one_when_every_attempt_fails(sleep): + runner = Runner([1, 1, 1]) + assert df.deploy("property-shared", "fly.toml", runner=runner, sleep=sleep) == 1 + assert len(runner.calls) == 3 + + +def test_a_hung_attempt_is_bounded_and_the_fallback_still_runs(sleep): + """Both observed failures were timeouts. + + Without a per-attempt bound the first hang eats the job budget and the + fallback never runs, which makes the whole ladder decorative. + """ + runner = Runner([subprocess.TimeoutExpired(cmd="flyctl", timeout=900), 0]) + assert df.deploy("property-shared", "fly.toml", runner=runner, sleep=sleep) == 0 + assert len(runner.calls) == 2 + assert "--depot=false" in runner.calls[1] + + +def test_every_attempt_is_given_a_timeout(sleep): + seen: list[float | None] = [] + + def runner(argv, timeout=None, **kwargs): + seen.append(timeout) + return subprocess.CompletedProcess(argv, 1) + + df.deploy("property-shared", "fly.toml", runner=runner, sleep=sleep) + assert seen and all(t is not None and t > 0 for t in seen), ( + "an attempt with no timeout can hang forever" + ) + + +@pytest.mark.parametrize( + ("app", "config"), + [("property-shared", "fly.toml"), ("propertydata", "fly.app.toml")], +) +def test_app_and_config_are_always_explicit_in_argv(app, config, sleep): + """CI relied on cwd discovery of fly.toml; the command that ran was not the + command anyone had written down.""" + runner = Runner([0]) + df.deploy(app, config, runner=runner, sleep=sleep) + argv = runner.calls[0] + assert argv[:2] == ["flyctl", "deploy"] + assert argv[argv.index("--app") + 1] == app + assert argv[argv.index("--config") + 1] == config + assert "--remote-only" in argv and "--ha=false" in argv + + +def test_a_missing_flyctl_fails_immediately_without_retrying(sleep): + runner = Runner([FileNotFoundError("flyctl")]) + assert df.deploy("property-shared", "fly.toml", runner=runner, sleep=sleep) == 1 + assert len(runner.calls) == 1, "retrying a missing binary cannot help" + + +def test_it_backs_off_between_attempts_but_not_after_the_last(sleep, sleeps): + runner = Runner([1, 1, 1]) + df.deploy("property-shared", "fly.toml", runner=runner, sleep=sleep) + assert len(sleeps) == 2, "three attempts means two gaps" + + +def test_the_token_never_appears_in_output(capsys, sleep, monkeypatch): + monkeypatch.setenv("FLY_API_TOKEN", FAKE_TOKEN) + runner = Runner([1, 1, 1]) + df.deploy("property-shared", "fly.toml", runner=runner, sleep=sleep) + captured = capsys.readouterr() + assert FAKE_TOKEN not in captured.out + captured.err diff --git a/tests/test_verify_release.py b/tests/test_verify_release.py new file mode 100644 index 0000000..5e2f918 --- /dev/null +++ b/tests/test_verify_release.py @@ -0,0 +1,194 @@ +"""Tests for the post-deploy release reconciliation. + +This job exists because of one concrete incident: on v1.15.1 the two Fly deploys +are independent leaf jobs, one failed and one succeeded, and that combination is +a normal terminal state of the workflow graph. The two apps ran different +versions with the broken build still live on the app that needed the fix. + +`test_one_target_behind_fails_and_names_both_versions` replays that exactly. +""" + +from __future__ import annotations + +import importlib.util +import json +import sys +from pathlib import Path +from typing import Any + +import pytest + +_MODULE_PATH = Path(__file__).resolve().parents[1] / "scripts" / "verify_release.py" + + +def _load_module() -> Any: + spec = importlib.util.spec_from_file_location("verify_release", _MODULE_PATH) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +vr = _load_module() + +TARGETS = { + "property-shared": "https://property-shared.fly.dev", + "propertydata": "https://propertydata.fly.dev", +} + + +def card(version: str, name: str = "property-data") -> str: + return json.dumps({"serverInfo": {"name": name, "version": version}}) + + +class Fetcher: + """Serves a queue of per-round responses keyed by host substring.""" + + def __init__(self, rounds: list[dict[str, object]]): + self.rounds = list(rounds) + self.calls: list[str] = [] + self._served = 0 + + def __call__(self, url: str) -> str: + self.calls.append(url) + # One round is consumed per full sweep of the targets. + index = min(self._served // len(TARGETS), len(self.rounds) - 1) + self._served += 1 + for key, value in self.rounds[index].items(): + if key in url: + if isinstance(value, BaseException): + raise value + return str(value) + raise AssertionError(f"unexpected url {url}") + + +@pytest.fixture +def sleeps() -> list[float]: + return [] + + +@pytest.fixture +def sleep(sleeps): + return sleeps.append + + +# --- tag normalisation --- + + +@pytest.mark.parametrize( + ("tag", "expected"), + [("v1.18.2", "1.18.2"), ("1.18.2", "1.18.2"), ("V1.18.2", "1.18.2"), ("vv1.0.0", "v1.0.0")], +) +def test_v_prefix_is_stripped_exactly_once(tag, expected): + """`lstrip("v")` would turn the nonsense tag vv1.0.0 into a plausible 1.0.0.""" + assert vr.normalise_tag(tag) == expected + + +# --- the happy path, and the incident --- + + +def test_both_targets_on_the_released_version_passes(sleep): + fetch = Fetcher([{"property-shared": card("1.18.2"), "propertydata": card("1.18.2")}]) + assert vr.verify("1.18.2", TARGETS, fetch=fetch, sleep=sleep, require_consecutive=2) == 0 + + +def test_one_target_behind_fails_and_names_both_versions(capsys, sleep): + """The v1.15.1 incident, replayed.""" + fetch = Fetcher([{"property-shared": card("1.15.0"), "propertydata": card("1.15.1")}]) + assert vr.verify("1.15.1", TARGETS, attempts=2, fetch=fetch, sleep=sleep) == 1 + out = capsys.readouterr().out + assert "RELEASE DRIFT" in out + assert "1.15.0" in out and "1.15.1" in out, "the operator must see both sides" + + +def test_an_unreachable_target_is_a_failure_not_a_pass(capsys, sleep): + fetch = Fetcher([ + {"property-shared": ConnectionError("refused"), "propertydata": card("1.18.2")} + ]) + assert vr.verify("1.18.2", TARGETS, attempts=2, fetch=fetch, sleep=sleep) == 1 + assert "RELEASE DRIFT" in capsys.readouterr().out + + +def test_an_unreadable_card_is_a_failure_not_a_pass(sleep): + fetch = Fetcher([{"property-shared": "not json", "propertydata": card("1.18.2")}]) + assert vr.verify("1.18.2", TARGETS, attempts=2, fetch=fetch, sleep=sleep) == 1 + + +# --- propagation --- + + +def test_it_waits_for_a_late_target_rather_than_failing_on_the_first_round(sleep): + """A completed deploy does not mean the new version is instantly serving.""" + fetch = Fetcher([ + {"property-shared": card("1.18.1"), "propertydata": card("1.18.2")}, + {"property-shared": card("1.18.2"), "propertydata": card("1.18.2")}, + {"property-shared": card("1.18.2"), "propertydata": card("1.18.2")}, + ]) + assert vr.verify("1.18.2", TARGETS, attempts=5, fetch=fetch, sleep=sleep) == 0 + + +def test_a_version_that_flaps_between_rounds_does_not_pass(sleep): + """Defends against a multi-Machine app round-robining old and new. + + A single 200 cannot distinguish "propagated" from "you happened to hit the + updated Machine". + """ + fetch = Fetcher([ + {"property-shared": card("1.18.2"), "propertydata": card("1.18.2")}, + {"property-shared": card("1.18.1"), "propertydata": card("1.18.2")}, + {"property-shared": card("1.18.2"), "propertydata": card("1.18.2")}, + ]) + assert vr.verify("1.18.2", TARGETS, attempts=3, fetch=fetch, sleep=sleep, + require_consecutive=3) == 1 + + +def test_it_stops_polling_as_soon_as_the_targets_agree(sleeps, sleep): + fetch = Fetcher([{"property-shared": card("1.18.2"), "propertydata": card("1.18.2")}]) + vr.verify("1.18.2", TARGETS, attempts=20, fetch=fetch, sleep=sleep, require_consecutive=1) + assert sleeps == [], "no reason to keep polling once the answer is in" + + +# --- the two failures must stay distinguishable --- + + +def test_a_pyproject_tag_mismatch_fails_immediately_without_polling(tmp_path, capsys): + """No amount of polling can fix a version that was never built. + + Distinct from drift, and reported as such -- otherwise an operator chases a + partial deploy that did not happen. + """ + pyproject = tmp_path / "pyproject.toml" + pyproject.write_text('[project]\nname = "property-shared"\nversion = "1.18.2"\n') + + called: list[str] = [] + original = vr._fetch + vr._fetch = lambda url: called.append(url) or "" # type: ignore[assignment] + try: + code = vr.main([ + "--expect", "v1.18.3", + "--target", "property-shared=https://example.invalid", + "--pyproject", str(pyproject), + ]) + finally: + vr._fetch = original # type: ignore[assignment] + + assert code == 1 + assert called == [], "a mismatch must not spend twenty polling rounds" + out = capsys.readouterr().out + assert "tag/pyproject mismatch" in out + assert "RELEASE DRIFT" not in out, "two different failures must read differently" + + +def test_the_real_pyproject_version_is_readable(): + """Pins the field path this depends on.""" + root = Path(__file__).resolve().parents[1] + assert vr.project_version(root / "pyproject.toml").count(".") >= 2 + + +@pytest.mark.parametrize("raw", ["noequals", "=https://x", "name="]) +def test_a_malformed_target_is_rejected(raw): + import argparse + + with pytest.raises(argparse.ArgumentTypeError): + vr.parse_target(raw)