Content-address challenge image tags, update --prune-old - #89
Merged
Conversation
generateBuilds returned early for fully-built challenges before checking the challenge directory for drift, so converging a schema after a source change silently kept stale images. Run the drift check first and log a warning in the already-built case; the error behavior for pending builds is unchanged.
Tag images as {challenge}:s{seed}-{checksum:x}-{host} instead of
{challenge}:{buildid}-{host}. The checksum (new builds.checksum column,
stamped at build time) is a CRC-32 over the challenge source checksum and
flag format, so together with the explicit seed the tag identifies the
image content: a source change now produces a new tag instead of mutating
the old one, and identical content builds to the same tag regardless of
which cmgr database built it (build ids are local autoincrements).
Since identical (challenge, seed, format, checksum) tuples in different
schemas now share tags, image removal is refcount-guarded via
contentReferenced. Images also gain cmgr.managed/cmgr.challenge labels
for future discovery. A migration backfills the column for existing rows
from the owning challenge''s current source checksum and retags their
local images so deployments keep launching without a rebuild.
Builds now record the generation their last rebuild displaced
(builds.prevchecksum, rotated atomically with the checksum in
finalizeBuild) and its images are retained as a rollback target: a future
rollback operation can swap the pair, re-extract /challenge from the
retained image, and restart instances; pointer comments mark those seams.
cmgr update --prune-old (API: UpdateOptions.PruneOldImages) untags the
generation that falls out of retention, keeping {current, previous} per
build. It is off by default because a tag may be referenced outside this
cmgr database (e.g. another cmgr on the same docker daemon).
contentReferenced now also treats a rollback reference as live, and
destroying a build retires both of its retained generations
independently.
Addresses correctness findings from the pre-PR review of content-addressed
image tagging:
- Migration resumability: the builds.checksum backfill is now data-driven
(keyed on checksum=0) and runs every start until complete, rather than
being gated by column existence. Each row's images are retagged before its
checksum is stamped, so a crash or transient docker error mid-migration is
retried instead of permanently skipping rows (which left builds tagged
s{seed}-0-{host} and unlaunchable with no self-heal). The retag is
idempotent and aborts only on a genuine daemon error; rows that cannot be
resolved are surfaced with a warning.
- Rollback-generation safety: the validateBuild-failure cleanup no longer
untags a generation the surviving row still retains as its rollback target
(guards the A->B->A source-revert case via PrevChecksum != Checksum). The
extraction container is removed first so the non-forced untag can actually
free the failed build image instead of silently leaking it.
- Concurrency: builds are stamped with their content checksum at openBuild so
an in-flight build is visible to reference checks, and the
check-then-remove sections (executeBuild cleanup, pruneReplacedImages,
destroyImages) are serialized with imageMu so a concurrent remover cannot
delete a tag a finishing build depends on.
- Cross-daemon safety: destroyImages removes with Force:false and treats
failures as non-fatal (matching pruneReplacedImages) so a shared tag still
referenced by another cmgr instance's container is leaked (recoverable)
rather than force-deleted out from under it.
- contentReferenced keys on (challenge, seed, checksum) only; format is
dropped since it influences a tag solely through the checksum.
- Extract and unit-test the retention predicates (rotatedPrevChecksum,
displacedPruneCandidate, incl. the flip-flop guard); add tests for
finalizeBuild checksum persistence, openBuild stamping, contentReferenced
fail-safe and format-agnostic matching, and migration resumability.
- Label solver images cmgr.managed/cmgr.challenge like the other image
classes; document that content identity omits the per-type Dockerfile and
add omitempty to the checksum/prev_checksum JSON fields.
- Reword --prune-old (CLI flag, `update` usage text, and UpdateOptions doc) to state it untags only the generation each rebuild displaces in this run, not a sweep of every older generation; fix the "insances" / "and existing" garble in the usage text while it is being edited. - swagger: document BuildMetadata.checksum/prev_checksum, which cmgrd now always emits, and note that prev_checksum is omitted when there is no retained rollback generation. - Begin the UpdateWithOptions doc comment with the symbol name (godoc convention), matching the sibling UpdateOptions/PruneOldImages additions.
There was a problem hiding this comment.
Pull request overview
This PR changes cmgr’s Docker image tagging for challenge builds from local build-id tags to content-addressed tags derived from (seed, content checksum, host), and adds database support for persisting the current and previous content generations to enable safe retention and optional pruning of superseded image generations.
Changes:
- Introduces content-addressed challenge image tags (
s{seed}-{checksum}-{host}) and persistschecksum/prevchecksumon builds for generation tracking. - Adds migration/backfill logic to populate
builds.checksumand retag legacy images, designed to be resumable/idempotent. - Adds
update --prune-oldsupport to untag generations displaced from retention, and labels cmgr-built images for future orphan discovery.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmgr/types.go | Adds checksum/prevchecksum fields to build metadata and introduces an image untagging mutex on the manager. |
| cmgr/solver_framework.go | Labels solver images for managed/orphan-discovery tracking. |
| cmgr/retention_test.go | Adds tests for retention rotation, prune predicates, checksum stamping/persistence, and migration resumability. |
| cmgr/docker.go | Implements content checksum derivation, content-addressed dockerId, checksum migration + legacy retagging, and refcount-guarded prune/destroy behavior. |
| cmgr/database.go | Extends the builds schema with checksum/prevchecksum and runs a startup backfill migration. |
| cmgr/database_operations_test.go | Updates tests for updated challenge update API signature and legacy schema stubs used in initDatabase migration tests. |
| cmgr/database_challenges.go | Adds retention rotation + prune-candidate helpers and wires rebuild/prune flow into updateChallenges. |
| cmgr/database_builds.go | Stamps checksum during openBuild, persists checksum/prevchecksum in finalizeBuild, and adds contentReferenced refcount guard logic. |
| cmgr/content_tag_test.go | Adds tests for checksum determinism, docker tag identity, contentReferenced semantics, and legacy DB migration. |
| cmgr/api.go | Adds UpdateOptions and UpdateWithOptions to support --prune-old. |
| cmd/cmgrd/swagger.yaml | Documents new BuildMetadata fields checksum and prev_checksum. |
| cmd/cmgr/main.go | Updates CLI help text to document update --prune-old and --dry-run. |
| cmd/cmgr/challenges.go | Adds --prune-old flag wiring to UpdateWithOptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
contentChecksum used a raw CRC-32, which can legitimately be 0x00000000
(e.g. sourceChecksum 0x05f16712 with format "flag{%s}"). Because 0 is the
sentinel for unset/unmigrated (builds.checksum/prevchecksum default 0, the
migration keys on checksum=0, and prevchecksum=0 means no rollback
generation), a real generation hashing to 0 would be perpetually re-migrated
and mishandled by the retention logic as 'none'. Reserve 0 by mapping a
computed 0 to 1; regression test pins the known zero-preimage input.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Docker image tags currently embed the local build id (
{challenge}:{id}-{host}), a sqlite rowid that means nothing across cmgr instances and gets mutated in place on every rebuild. This switches tags to content identity —{challenge}:s{seed}-{checksum}-{host}— so the same challenge content resolves to the same tag on any host, and a source change produces a new tag instead of silently repointing the old one. This is the groundwork for pushing challenge images to a registry.Key changes
checksum = CRC-32(source checksum + flag format); seed carried explicitly in the tag (sprefix keeps negative seeds valid). Build id no longer appears in any tag.builds.checksum+builds.prevchecksumcolumns.prevchecksumretains the generation displaced by the last rebuild as a rollback target (therollbackverb itself is not implemented — just the retained state + pointers).cmgr update --prune-old(default off): untags the generation each rebuild displaces from retention. Off by default because another cmgr on the same daemon may share a content tag, which this database can't see. Removal everywhere is refcount-guarded and best-effort.cmgr.managed,cmgr.challenge) for future orphan discovery.Migration & compatibility
builds.checksumbackfill runs and retags existing local images from the legacy form. It is resumable (data-driven, idempotent) so an interrupted upgrade retries rather than stranding builds.checksum/prev_checksumonBuildMetadataare additive (omitempty, documented in swagger).Testing
Unit suite green (incl. new tests for the retention predicates, checksum persistence/stamping, refcount fail-safe, and migration resumability). Full build → tag/label → prune → destroy lifecycle verified live against Docker.