From 384e316317ecb3fc90b22f377166dae10bea8b53 Mon Sep 17 00:00:00 2001 From: tamnd <1218621+tamnd@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:17:11 +0700 Subject: [PATCH] Keep the manifests an index is made of The tidy up in the image workflow deleted the per architecture manifests that every published image is built out of, which left fifteen indexes resolving and then pointing at nothing. Every tag in the package pulls as manifest unknown right now, which is why gdb and tier1 fail on #125. The retention rule kept three kinds of version: the newest few hundred, anything tagged, and anything a release named. A manifest inside an index is none of those. It never gets a tag, because the tag goes on the index, and it is never in the lockfile, because the lockfile records index digests. The build is fully cached against a fixed commit so the same manifest keeps the same digest week after week and its age never moves, and one Monday it falls past the floor and goes. So close the protected set over membership before comparing anything against it. cpybuild expand starts from every tagged digest and every digest a release named, asks the registry what each one lists, and walks until nothing new turns up. It refuses to print at all if it found no parts anywhere, because with fifteen indexes in the package that means Docker is missing rather than that there is nothing to keep. The content never changed and the layer blobs are still there, so the rebuild this commit triggers re pushes the same manifests under the same digests and repairs the pinned index in place. Closes #126 --- .github/workflows/cpython-images.yml | 18 +++++- tools/cpybuild/src/cpybuild/cli.py | 44 +++++++++++++++ tools/cpybuild/src/cpybuild/images.py | 56 +++++++++++++++++++ tools/cpybuild/src/cpybuild/retention.py | 48 +++++++++++++++- tools/cpybuild/tests/test_cpybuild_cli.py | 42 +++++++++++++- tools/cpybuild/tests/test_cpybuild_images.py | 47 ++++++++++++++++ .../cpybuild/tests/test_cpybuild_retention.py | 45 ++++++++++++++- 7 files changed, 292 insertions(+), 8 deletions(-) diff --git a/.github/workflows/cpython-images.yml b/.github/workflows/cpython-images.yml index d79886d..de4762d 100644 --- a/.github/workflows/cpython-images.yml +++ b/.github/workflows/cpython-images.yml @@ -264,6 +264,13 @@ jobs: enable-cache: true - run: uv sync --all-packages + # Asking what an index is made of needs a pull, and the parts are what #126 was about. + - uses: docker/login-action@v3 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + - name: Delete images nothing points at any more env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -273,11 +280,18 @@ jobs: KEEP: "120" run: | set -euo pipefail - uv run cpybuild protected > /tmp/protected.txt - wc -l < /tmp/protected.txt | xargs echo "digests a release or the lockfile points at:" + uv run cpybuild protected > /tmp/named.txt + wc -l < /tmp/named.txt | xargs echo "digests a release or the lockfile points at:" gh api --paginate \ "/users/${{ github.repository_owner }}/packages/container/cpython-internals%2Fcpython/versions" \ > /tmp/versions.json + # An index is a list of manifests and those manifests are versions in their own + # right, never tagged and never in the lockfile. Deleting one leaves an index that + # resolves and then points at nothing, so they have to be kept too. Issue #126. + uv run cpybuild expand \ + --versions /tmp/versions.json \ + --protected /tmp/named.txt \ + > /tmp/protected.txt uv run cpybuild prune \ --versions /tmp/versions.json \ --protected /tmp/protected.txt \ diff --git a/tools/cpybuild/src/cpybuild/cli.py b/tools/cpybuild/src/cpybuild/cli.py index 80bea75..460ee57 100644 --- a/tools/cpybuild/src/cpybuild/cli.py +++ b/tools/cpybuild/src/cpybuild/cli.py @@ -18,6 +18,7 @@ cpybuild record-index debug sha256:... cpybuild devcontainer --write point the devcontainer at the lockfile's debug image cpybuild protected digests the tidy up is not allowed to delete + cpybuild expand --versions v.json --protected p.txt those digests and their parts cpybuild prune --versions v.json --protected p.txt version ids that can be deleted """ @@ -36,10 +37,12 @@ DEVCONTAINER, DIGEST, LOCKFILE, + REGISTRY, Broken, Lock, devcontainer_problems, digests, + members_of, problems, protected, retarget, @@ -206,6 +209,41 @@ def _protected(args: argparse.Namespace) -> int: return 0 +def _expand(args: argparse.Namespace) -> int: + """Print the protected digests and everything they are made of, one per line. + + This is the answer to #126. What we call an image is an index naming an amd64 manifest, an + arm64 manifest and an attestation for each, and those parts are versions the registry will + happily delete on their own. Deleting one leaves an index pointing at nothing, which is + `manifest unknown` for everybody pulling it. + + Refuses to print anything if the walk found no parts at all. Fifteen indexes have parts, so + zero means Docker is not there or the login did not take, and the useful thing to do with + an answer that is probably wrong is not hand it to something that deletes images. + """ + body = json.loads(Path(args.versions).read_text(encoding="utf-8")) + versions = retention.read(body) + named = { + one.strip() + for one in Path(args.protected).read_text(encoding="utf-8").splitlines() + if one.strip() + } + roots = retention.anchors(versions, named) + found = retention.reachable(roots, members_of(args.registry)) + parts = found - roots + if not parts: + print( + f"{len(roots)} digests to keep and not one of them is made of anything, " + "which cannot be right, so nothing is safe to delete", + file=sys.stderr, + ) + return 1 + print(f"{len(roots)} kept outright, and {len(parts)} more they are made of", file=sys.stderr) + for one in sorted(found): + print(one) + return 0 + + def _prune(args: argparse.Namespace) -> int: """Print the id of every package version that can be deleted, one per line. @@ -310,6 +348,12 @@ def build() -> argparse.ArgumentParser: safe = subs.add_parser("protected", help="digests the tidy up must not delete") safe.set_defaults(handler=_protected) + wider = subs.add_parser("expand", help="protected digests plus the parts they are made of") + wider.add_argument("--versions", required=True, help="the registry's version list as JSON") + wider.add_argument("--protected", required=True, help="digests to keep, one per line") + wider.add_argument("--registry", default=REGISTRY, help="where to ask what an index lists") + wider.set_defaults(handler=_expand) + gone = subs.add_parser("prune", help="version ids that can be deleted") gone.add_argument("--versions", required=True, help="the registry's version list as JSON") gone.add_argument("--protected", required=True, help="digests to keep, one per line") diff --git a/tools/cpybuild/src/cpybuild/images.py b/tools/cpybuild/src/cpybuild/images.py index 3a61cfc..ba24fbb 100644 --- a/tools/cpybuild/src/cpybuild/images.py +++ b/tools/cpybuild/src/cpybuild/images.py @@ -15,6 +15,7 @@ import json import re import subprocess +import sys from collections.abc import Callable, Iterable from dataclasses import dataclass, field from datetime import date @@ -160,6 +161,61 @@ def digests(lock: Lock) -> set[str]: return found | {one.digest for one in lock.indexes.values()} +class Unreadable(Broken): + """The registry would not say what an image is made of.""" + + +def members_in(text: str) -> list[str]: + """The digests an index lists, or nothing at all for a plain manifest. + + A plain manifest has `layers` and no `manifests`, and an index has the other way round, so + the absent key is the answer rather than a case to detect. + """ + body = json.loads(text) + listed = body.get("manifests") or [] + return [str(one["digest"]) for one in listed if one.get("digest")] + + +def _inspect(reference: str) -> str: + """What the registry has under a reference, as it stored it. + + `--raw` rather than the pretty output, because the pretty output resolves an index into a + table of platforms and drops the attestation rows, and the attestations are versions the + tidy up would otherwise delete. + """ + done = subprocess.run( + ["docker", "buildx", "imagetools", "inspect", "--raw", reference], + capture_output=True, + text=True, + check=False, + ) + if done.returncode != 0: + raise Unreadable(f"{reference}: {done.stderr.strip() or 'inspect failed'}") + return done.stdout + + +def members_of( + registry: str = REGISTRY, + inspect: Callable[[str], str] = _inspect, +) -> Callable[[str], list[str]]: + """A lookup from one digest to the digests inside it, for the tidy up to walk. + + A digest that will not resolve is reported and treated as holding nothing, because some of + them do not resolve any more and refusing to run would mean the tidy up never runs again. + The caller is the one that decides whether too many failed to be worth continuing, and + `cpybuild expand` does exactly that. + """ + + def look(digest: str) -> list[str]: + try: + return members_in(inspect(f"{registry}@{digest}")) + except (Unreadable, ValueError, TypeError, KeyError) as error: + print(f"cannot read {digest[:19]}, treating it as empty: {error}", file=sys.stderr) + return [] + + return look + + def _from_git(revision: str, path: str = str(LOCKFILE)) -> str | None: """The contents of a file as of some revision, or None if it was not there yet.""" done = subprocess.run( diff --git a/tools/cpybuild/src/cpybuild/retention.py b/tools/cpybuild/src/cpybuild/retention.py index fc9e86d..7caa44c 100644 --- a/tools/cpybuild/src/cpybuild/retention.py +++ b/tools/cpybuild/src/cpybuild/retention.py @@ -7,7 +7,7 @@ Deleting a container image is not reversible and the thing it breaks is somebody else's afternoon a year from now, so this module is written to be timid. It takes the list the registry gave us and returns ids, and the workflow does the deleting. Everything it refuses to -delete, it refuses for a stated reason, and there are three of them: +delete, it refuses for a stated reason, and there are four of them: Anything a release points at stays, however old. A reader who checks out `v0.2.0` and runs the experiments has to get the interpreter that version was written against. @@ -15,14 +15,24 @@ Anything with a tag on it stays. A tag on a version means it is the current image for one of the five builds, and the whole point of the lockfile is that those keep resolving. +Anything one of those is made of stays. This one is issue #126 and it cost the whole package. +What we call an image is an index: a short list naming an amd64 manifest, an arm64 manifest and +an attestation for each. Those parts are versions in their own right, and they are never tagged +and never in the lockfile, so the first two rules do not see them. The build is fully cached +against a fixed commit, so a part keeps the same digest week after week and its age never moves, +and one Monday it falls past the floor and is deleted. The index survives and lists two halves +that are not there any more, which reads as `manifest unknown` to everybody pulling it. So the +protected set has to be closed over membership before anything is compared against it. + The newest few hundred stay regardless. This is the crude rule and it is the one that catches -what the other two miss: an image published an hour ago that the lockfile pull request has not +what the other three miss: an image published an hour ago that the lockfile pull request has not been merged for yet is not protected by anything else, and deleting it would be the tidy up undoing the run it followed. """ from __future__ import annotations +from collections.abc import Callable, Iterable from dataclasses import dataclass @@ -50,6 +60,37 @@ def read(body: list[dict]) -> list[Version]: return [Version.from_dict(one) for one in body] +def anchors(versions: list[Version], protected: Iterable[str]) -> set[str]: + """Where the keeping starts: every tagged version, and everything a release named. + + Separate from `reachable` because working out the starting points needs no registry and + following the membership does, and the two being separate is what lets the interesting + half be tested against a dictionary. + """ + return {one.digest for one in versions if one.tags} | set(protected) + + +def reachable(roots: Iterable[str], children: Callable[[str], Iterable[str]]) -> set[str]: + """Those digests and everything they are made of, all the way down. + + A breadth first walk rather than one pass, because an index can name an index. Today it + does not, but the shape that produced #126 was somebody reasonably assuming a fixed depth, + and a walk costs one `while` loop. + + `children` is passed in rather than reached for, so the tidy up can be tested without a + registry and so the one place that talks to Docker stays in the command line module. + """ + seen: set[str] = set() + queue = list(roots) + while queue: + one = queue.pop() + if one in seen: + continue + seen.add(one) + queue.extend(children(one)) + return seen + + def doomed(versions: list[Version], protected: set[str], keep: int) -> list[Version]: """The versions that can go, newest first, and nothing else. @@ -68,5 +109,6 @@ def why(versions: list[Version], protected: set[str], keep: int) -> str: pinned = sum(1 for one in versions if one.digest in protected) return ( f"{len(versions)} versions, deleting {len(going)}: " - f"{keep} newest kept, {tagged} tagged, {pinned} pointed at by a release" + f"{keep} newest kept, {tagged} tagged, " + f"{pinned} named by a release or part of something named" ) diff --git a/tools/cpybuild/tests/test_cpybuild_cli.py b/tools/cpybuild/tests/test_cpybuild_cli.py index afd5475..63d9519 100644 --- a/tools/cpybuild/tests/test_cpybuild_cli.py +++ b/tools/cpybuild/tests/test_cpybuild_cli.py @@ -192,7 +192,47 @@ def test_prune_prints_ids_on_stdout_and_the_reasoning_on_stderr(tmp_path, capsys ) said = capsys.readouterr() assert said.out.split() == ["2"] - assert "1 pointed at by a release" in said.err + assert "1 named by a release or part of something named" in said.err + + +def test_expand_adds_the_manifests_an_index_is_made_of(tmp_path, capsys, monkeypatch): + """Issue #126. The parts are versions the registry will delete on their own.""" + part = "sha256:" + "1" * 64 + versions = tmp_path / "versions.json" + versions.write_text( + json.dumps( + [ + { + "id": 1, + "name": TWO, + "created_at": "2026-08-01T00:00:00Z", + "metadata": {"container": {"tags": ["debug"]}}, + } + ] + ) + ) + safe = tmp_path / "protected.txt" + safe.write_text(f"{ONE}\n") + inside = {ONE: [part]} + monkeypatch.setattr("cpybuild.cli.members_of", lambda registry: lambda one: inside.get(one, [])) + assert main(["expand", "--versions", str(versions), "--protected", str(safe)]) == 0 + assert sorted(capsys.readouterr().out.split()) == sorted([ONE, TWO, part]) + + +def test_expand_refuses_when_nothing_turned_out_to_be_made_of_anything( + tmp_path, capsys, monkeypatch +): + """Fifteen indexes have parts, so zero means Docker is missing or the login did not take, + and an answer that is probably wrong should not be handed to something that deletes.""" + versions = tmp_path / "versions.json" + versions.write_text(json.dumps([])) + safe = tmp_path / "protected.txt" + safe.write_text(f"{ONE}\n") + monkeypatch.setattr("cpybuild.cli.members_of", lambda registry: lambda digest: []) + assert main(["expand", "--versions", str(versions), "--protected", str(safe)]) == 1 + said = capsys.readouterr() + assert said.out == "" + assert "nothing is safe to delete" in said.err def test_prune_reads_a_protected_file_with_blank_lines_in_it(tmp_path, capsys): diff --git a/tools/cpybuild/tests/test_cpybuild_images.py b/tools/cpybuild/tests/test_cpybuild_images.py index ec4275d..1aa7d62 100644 --- a/tools/cpybuild/tests/test_cpybuild_images.py +++ b/tools/cpybuild/tests/test_cpybuild_images.py @@ -268,3 +268,50 @@ def test_the_committed_devcontainer_pulls_the_committed_debug_image(): lock = Lock.load(images.LOCKFILE) said = images.DEVCONTAINER.read_text(encoding="utf-8") assert images.devcontainer_problems(lock, said) == [] + + +INDEX = """{ + "mediaType": "application/vnd.oci.image.index.v1+json", + "manifests": [ + {"digest": "sha256:aa", "platform": {"architecture": "amd64"}}, + {"digest": "sha256:bb", "platform": {"architecture": "arm64"}} + ] +}""" + +MANIFEST = """{ + "mediaType": "application/vnd.oci.image.manifest.v1+json", + "config": {"digest": "sha256:cc"}, + "layers": [{"digest": "sha256:dd"}] +}""" + + +def test_an_index_says_which_manifests_it_is_made_of(): + assert images.members_in(INDEX) == ["sha256:aa", "sha256:bb"] + + +def test_a_plain_manifest_is_made_of_no_other_manifests(): + """It has layers, and a layer is a blob rather than a version the tidy up could delete.""" + assert images.members_in(MANIFEST) == [] + + +def test_a_digest_the_registry_will_not_talk_about_reads_as_holding_nothing(capsys): + """Some of them genuinely do not resolve any more, and refusing to run would mean the tidy + up never runs again. The refusal lives in `cpybuild expand`, which can see how many failed.""" + + def refuse(reference: str) -> str: + raise images.Unreadable(f"{reference}: manifest unknown") + + assert images.members_of("somewhere", refuse)(ONE) == [] + assert "treating it as empty" in capsys.readouterr().err + + +def test_the_lookup_asks_about_a_digest_and_never_about_a_tag(): + """A tag can move between the walk and the delete, and a digest cannot.""" + asked = [] + + def remember(reference: str) -> str: + asked.append(reference) + return INDEX + + images.members_of("somewhere", remember)(ONE) + assert asked == [f"somewhere@{ONE}"] diff --git a/tools/cpybuild/tests/test_cpybuild_retention.py b/tools/cpybuild/tests/test_cpybuild_retention.py index 0391fcd..0c48ae3 100644 --- a/tools/cpybuild/tests/test_cpybuild_retention.py +++ b/tools/cpybuild/tests/test_cpybuild_retention.py @@ -9,10 +9,12 @@ from __future__ import annotations from cpybuild import retention -from cpybuild.retention import Version, doomed, why +from cpybuild.retention import Version, anchors, doomed, reachable, why SAFE = "sha256:" + "a" * 64 LOOSE = "sha256:" + "b" * 64 +PART = "sha256:" + "c" * 64 +DEEPER = "sha256:" + "d" * 64 def version(id: int, day: int, digest: str = LOOSE, tags: tuple[str, ...] = ()) -> Version: @@ -87,4 +89,43 @@ def test_the_log_line_says_why_things_were_kept_and_not_only_how_many(): versions = [version(1, day=1, digest=SAFE), version(2, day=2, tags=("debug",))] said = why(versions, {SAFE}, keep=0) assert "1 tagged" in said - assert "1 pointed at by a release" in said + assert "1 named by a release or part of something named" in said + + +def test_the_keeping_starts_from_the_tagged_versions_as_well_as_the_named_ones(): + versions = [version(1, day=1, tags=("debug",)), version(2, day=2, digest=PART)] + assert anchors(versions, {SAFE}) == {SAFE, LOOSE} + + +def test_a_manifest_inside_a_kept_index_is_kept_too(): + """Issue #126. This is the one that broke every image in the package at once.""" + inside = {SAFE: [PART, DEEPER]} + assert reachable({SAFE}, lambda one: inside.get(one, [])) == {SAFE, PART, DEEPER} + + +def test_an_index_inside_an_index_is_followed_all_the_way_down(): + """Nothing produces this today. The bug was somebody assuming a fixed depth.""" + inside = {SAFE: [PART], PART: [DEEPER]} + assert reachable({SAFE}, lambda one: inside.get(one, [])) == {SAFE, PART, DEEPER} + + +def test_two_indexes_sharing_a_manifest_do_not_send_the_walk_round_forever(): + """Two runs of a cached build push the same halves, so sharing is the normal case.""" + inside = {SAFE: [PART], LOOSE: [PART], PART: [SAFE]} + assert reachable({SAFE, LOOSE}, lambda one: inside.get(one, [])) == {SAFE, LOOSE, PART} + + +def test_a_part_of_a_kept_index_survives_a_tidy_up_that_would_otherwise_take_it(): + """The whole story at once: an old untagged manifest that only a tagged index points at. + + Before #126 this returned both 3 and 1, and deleting 1 left the `debug` tag resolving to an + index whose amd64 half was gone. + """ + versions = [ + version(1, day=1, digest=PART), + version(2, day=2, digest=SAFE, tags=("debug",)), + version(3, day=3), + ] + inside = {SAFE: [PART]} + safe = reachable(anchors(versions, set()), lambda one: inside.get(one, [])) + assert [one.id for one in doomed(versions, safe, keep=0)] == [3]