From cf1675a18bad92c9e38e43512f54d44723e525a4 Mon Sep 17 00:00:00 2001 From: blooop Date: Wed, 9 Sep 2026 10:30:43 +0100 Subject: [PATCH] fix: the prebuild tag moved for files the image cannot see Opening this repository with `dl` pulls a published container instead of building one, and it had gone back to building. The prebuild tag is a hash of the build context as well as of the build config, and a Dockerfile with no COPY leaves devpod's `includeFiles` empty -- at which point it hashes the *whole* context directory, keyed by path and CRC32, with only its own feature-staging folder excluded (pkg/util/hash.DirectoryHash, from config.CalculatePrebuildHash). There was no `.dockerignore` inside `.devcontainer/`, so the context half of the hash was "every byte in this directory". Four things in there cannot reach the image: the feature's README, its troubleshooting guide, the host-side `initializeCommand`, and this manifest's own comments -- and the manifest is mostly comments. Measured on devpod 0.26.1, amd64, at 4db3427: appending one comment line to `claude-code/README.md` and one to `claude-code/init-host.sh` moved the tag from devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7, which CI had published and every launch pulled, to devpod-90e9641b9d3661f681e640eeedb3a410, which nothing had ever built. Several minutes per launch, on every branch carrying the edit, and the symptom is only that opening a container is slow again -- devpod treats an unmatched tag as a cache miss and builds locally without complaint. `.devcontainer/.dockerignore` narrows the hash to the files the image is built from. Excluding `init-host.sh` is what makes the two halves of the hash agree rather than a new judgement call: `normalizeConfigForHash` already clears `DevContainerActions`, where `initializeCommand` lives, out of the config half, and the script was then hashed straight back in as context. The manifest goes the same way and for the same reason -- devpod read it to get here and does not read it out of the context, and the config half already carries every build input it declares. Checked in both directions, because an exclusion too wide is the worse bug: a launch that pulls an image built from a Dockerfile the branch no longer has comes up fine and is the wrong container. Prose, the host hook and comment-only manifest edits no longer move the tag; the Dockerfile, `claude-code/install.sh`, `claude-code/devcontainer-feature.json`, a changed `build.cacheFrom` and an added feature all still do. The resulting image was diffed against one built from the same tree without the `.dockerignore`: same 19 layers, same env, same metadata labels, `claude` and `dockerd` both present. Five guards, four of them because the premise can move under this file. test_the_dockerfile_copies_nothing_out_of_the_build_context is the first one to read -- add a COPY and the excluded files stop being invisible to the image and start being missing from it. The classification test makes a new file under `.devcontainer/` a failure until somebody says which side of the line it is on, since the silent default is the expensive one. One rebuild is owed: the `.dockerignore` is itself part of the context, so this commit moves the tag once, and launches build locally until devcontainer-prebuild.yml republishes on main. --- .devcontainer/.dockerignore | 53 +++++ .devcontainer/devcontainer.json | 24 ++- .github/workflows/devcontainer-prebuild.yml | 19 +- CHANGELOG.md | 34 ++++ docs/development.md | 28 ++- test/unit/test_devcontainer_manifest.py | 208 ++++++++++++++++++++ 6 files changed, 344 insertions(+), 22 deletions(-) create mode 100644 .devcontainer/.dockerignore diff --git a/.devcontainer/.dockerignore b/.devcontainer/.dockerignore new file mode 100644 index 00000000..adf7cc00 --- /dev/null +++ b/.devcontainer/.dockerignore @@ -0,0 +1,53 @@ +# What the prebuild tag is allowed to notice. +# +# devpod hashes the build context as well as the build config, and with a +# Dockerfile that COPYs nothing it hashes the *whole* directory: every file +# under the context, keyed by path and CRC32, with only its own feature-staging +# folder excluded (pkg/util/hash.DirectoryHash, reached from +# pkg/devcontainer/config.CalculatePrebuildHash). There was no .dockerignore in +# here, so the context half of the hash was "every byte in .devcontainer/", +# which is a wider claim than the tag is meant to make: the tag is supposed to +# move when the image would differ, and three kinds of file in here cannot +# change the image at all. +# +# Measured on devpod 0.26.1, amd64, against this repository's `.devcontainer/` +# at 4db3427: appending one comment line to claude-code/README.md and one to +# claude-code/init-host.sh moved the tag from +# devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7 -- which CI had published, so it +# pulled -- to devpod-90e9641b9d3661f681e640eeedb3a410, which nothing has ever +# published. That is a multi-minute local build, per launch, on every branch +# carrying the edit, bought with a prose change. +# +# Nothing here is excluded from the *build* in any meaningful sense, which is +# what makes this safe rather than a trade: the Dockerfile has no COPY and no +# ADD, so the context tar is not read at all. The one guard against that +# changing is a test -- test_the_dockerfile_copies_nothing_out_of_the_context. + +# Prose. The feature's README and troubleshooting guide are here because the +# devcontainer Features spec wants them beside the feature they document, not +# because anything reads them at build time. +**/*.md + +# The host-side hook. `initializeCommand` runs on the developer's machine +# before the container exists, and devpod already agrees it is not a build +# input: normalizeConfigForHash clears DevContainerActions, which is where the +# command lives, out of the config half of the hash. It then hashed the script +# itself straight back in as context. Excluding it here is what makes the two +# halves say the same thing. +claude-code/init-host.sh + +# The manifest itself, for the same reason and with the most to gain from it. +# devpod does not read devcontainer.json out of the context -- it read the file +# to get here -- and the config half of the hash already carries every build +# input it declares: `name`, `features`, `overrideFeatureInstallOrder`, `image`, +# `dockerfile`, `context` and the whole of `build`. Hashing the raw bytes on top +# of that adds nothing except the comments, and this file is mostly comments: +# `abf6d9b` moved the tag with ~30 lines of them beside a one-line mount change. +# +# It also stops the raw bytes overruling devpod on the fields it deliberately +# drops -- `mounts`, `containerEnv`, `postCreateCommand`, `customizations` -- +# all of which devpod applies when it creates the container, so a prebuilt image +# plus a changed one of them is correct and was being rebuilt for nothing. +# Verified both directions: a comment-only edit leaves the tag alone, while +# changing `build.cacheFrom` or adding a feature still moves it. +devcontainer.json diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 4db3dcef..f4cec7cf 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -9,16 +9,22 @@ // is a hash *of the build context* as well as of the config // (pkg/devcontainer/config/prebuild.go). With the root as context that // hash moved on every commit to any file in the repository, so a - // prebuilt image could never match one. Scoped here it moves when - // `.devcontainer/**` moves, which is exactly when the image has to be - // rebuilt -- this Dockerfile and the local `claude-code` feature's - // scripts included, since they sit inside the context and are hashed - // with it. + // prebuilt image could never match one. // - // The root `.dockerignore` is no longer read for this build (docker - // looks for one inside the context); its patterns existed to keep - // `.pixi/` and `rust/target/` out of a root-sized context, and nothing - // of that shape is in here. + // Scoping it here narrowed that to "every commit under + // `.devcontainer/**`", which is close to right but not the same claim as + // "every commit that changes the image", and the gap cost real launches. + // A Dockerfile with no COPY leaves devpod's includeFiles empty, and it + // then hashes the whole directory -- so this file's own comments, the + // feature's README, and the host-side `initializeCommand` were all + // hashed into the tag, none of which the image can see. + // `.dockerignore`, below, is what closes it; read that file for the + // measurement and the reasoning. + // + // The root `.dockerignore` is not read for this build -- docker looks + // for one inside the context, and there is one there now. The root + // file's patterns existed to keep `.pixi/` and `rust/target/` out of a + // root-sized context, and nothing of that shape is in here. "context": ".", // Best-effort layer cache for builders that know nothing about devpod diff --git a/.github/workflows/devcontainer-prebuild.yml b/.github/workflows/devcontainer-prebuild.yml index 9195b125..3dfc90cf 100644 --- a/.github/workflows/devcontainer-prebuild.yml +++ b/.github/workflows/devcontainer-prebuild.yml @@ -29,11 +29,20 @@ name: Devcontainer prebuild on: push: branches: [ "main" ] - # Exactly the inputs to the hash. `build.context` is `.devcontainer`, so the - # hashed context is this directory and nothing else -- which is what makes a - # path filter honest here rather than an optimisation that silently ships a - # stale image. A commit that changes `devlaunch/`, `rust/` or `pyproject.toml` - # cannot change the tag, so there is nothing for this workflow to do. + # A superset of the inputs to the hash, which is the safe direction to err in. + # `build.context` is `.devcontainer`, so nothing outside this directory can + # move the tag -- a commit to `devlaunch/`, `rust/` or `pyproject.toml` has + # nothing for this workflow to do. Inside it, `.devcontainer/.dockerignore` + # narrows the hash further, to the files the image is actually built from, so + # some commits this filter catches cannot move the tag either. Those cost a + # registry lookup and nothing else: `devpod build` finds the prebuild already + # published and returns without rebuilding or repushing. + # + # Narrowing the filter to match the hash exactly is the change not to make. + # It would mean restating `.dockerignore`'s patterns here, in a second place, + # inverted -- and the failure mode of getting that wrong is a commit that + # moved the tag with no image published at it, which is a silent return to + # local builds for everyone on `main`. # # The workflow file itself is in the list because a change to how the image # is built or tagged should produce one. diff --git a/CHANGELOG.md b/CHANGELOG.md index b6d6652c..1d6682dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Editing prose under `.devcontainer/` no longer throws away the prebuilt + container.** Opening this repository with `dl` pulls a published image instead + of building one, and it had gone back to building. The prebuild tag is a hash + of the build context, and with a Dockerfile that `COPY`s nothing devpod hashes + the *whole* context directory -- so the feature's `README.md`, its + `TROUBLESHOOTING.md`, the host-side `initializeCommand`, and this manifest's + own comments were all hashed into a tag that is supposed to move only when the + image would differ. None of the four can reach the image. + + Measured on devpod 0.26.1, amd64, at `4db3427`: one comment line appended to + `claude-code/README.md` and one to `claude-code/init-host.sh` moved the tag + from `devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7`, which CI had published and + every launch pulled, to `devpod-90e9641b9d3661f681e640eeedb3a410`, which + nothing had ever built. The cost was several minutes per launch, on every + branch carrying the edit, and the only symptom was that opening a container + was slow again -- devpod treats an unmatched tag as a cache miss and builds + locally without complaint. + + `.devcontainer/.dockerignore` is the fix, and the narrowing is checked in both + directions rather than only the useful one. Prose, the host hook and + `devcontainer.json` stop moving the tag; the Dockerfile, + `claude-code/install.sh`, `claude-code/devcontainer-feature.json` and the + build inputs the manifest declares still do -- `build.cacheFrom` and an added + feature were both confirmed to move it, since devpod hashes those from the + parsed config rather than from the file. An exclusion too wide would be the + worse bug: a launch pulling an image built from a Dockerfile the branch no + longer has, coming up fine and being the wrong container. + + **One rebuild is still owed.** The `.dockerignore` is itself part of the + context, so this commit moves the tag once; launches build locally until + `devcontainer-prebuild.yml` republishes on `main`. + ## [0.35.0] - 2026-09-09 ### Added diff --git a/docs/development.md b/docs/development.md index 0a85a029..91b8947b 100644 --- a/docs/development.md +++ b/docs/development.md @@ -396,14 +396,26 @@ Two consequences worth knowing: what makes the tag usable. The Dockerfile copies nothing out of the context, so the root was never needed, but it was hashed, which meant a different tag on every commit to any file and a prebuilt image that never matched one. - Scoped to `.devcontainer`, the tag moves when `.devcontainer/**` moves, the - Dockerfile and the local feature's scripts included. -- **A commit whose `.devcontainer/` differs from the last prebuild builds - locally.** That is the correct answer rather than a gap: the alternative is a - container built from something other than what the branch asks for. The pull - comes back once the change is on `main`. What the tag does not promise is the - converse, that one `.devcontainer/` tree always yields one image; see "What - the prebuild tag does not promise" below. +- **Inside `.devcontainer/`, only the files the image is built from are + hashed**, and `.devcontainer/.dockerignore` is what draws that line. Scoping + the context to this directory was not enough on its own: a Dockerfile with no + `COPY` leaves devpod's `includeFiles` empty and it hashes the whole directory, + so the feature's `README.md`, its `TROUBLESHOOTING.md`, the host-side + `initializeCommand` and `devcontainer.json`'s own comments all moved the tag + without changing the image. Measured on devpod 0.26.1 at `4db3427`: one + comment line appended to `claude-code/README.md` and one to + `claude-code/init-host.sh` moved the tag off the published + `devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7` and onto a tag nothing had ever + built. What still moves it: the Dockerfile, `claude-code/install.sh`, + `claude-code/devcontainer-feature.json`, and the build inputs + `devcontainer.json` declares -- `build`, `features`, `image`, `name` -- which + devpod hashes from the parsed config rather than from the file. +- **A commit that changes what the image is built from builds locally until it + reaches `main`.** That is the correct answer rather than a gap: the + alternative is a container built from something other than what the branch + asks for. The pull comes back once the change is on `main`. What the tag does + not promise is the converse, that one `.devcontainer/` tree always yields one + image; see "What the prebuild tag does not promise" below. `.github/workflows/devcontainer-prebuild.yml` publishes it, on pushes to `main` that touch `.devcontainer/**` and on manual dispatch. Its path filter is exactly diff --git a/test/unit/test_devcontainer_manifest.py b/test/unit/test_devcontainer_manifest.py index 5dea4911..070b4d57 100644 --- a/test/unit/test_devcontainer_manifest.py +++ b/test/unit/test_devcontainer_manifest.py @@ -46,6 +46,45 @@ PIXI_GLOBAL_INSTALL = "pixi global install " DOCKERFILE = REPO_ROOT / ".devcontainer" / "Dockerfile" +CONTEXT_DIR = REPO_ROOT / ".devcontainer" +CONTEXT_DOCKERIGNORE = CONTEXT_DIR / ".dockerignore" + +# Every file under the build context, split by whether the *image* can see it. +# +# devpod hashes the context as well as the config, and with includeFiles empty -- +# which is what a Dockerfile with no COPY leaves it -- it hashes the whole +# directory (pkg/util/hash.DirectoryHash). So anything in here that the build +# cannot read is a file whose edits throw the published prebuild away and buy +# nothing, and `.dockerignore` is what keeps it out of the hash. +# +# Split by hand rather than derived, because "can the image see this" is not a +# property of the path: `install.sh` and `devcontainer-feature.json` are the +# local feature devpod bakes in, while `init-host.sh` sits beside them and runs +# on the host. The two sets together have to account for every file present, so +# a file added here is a test failure until somebody says which side it is on. +CONTEXT_BUILD_INPUTS = frozenset( + { + "Dockerfile", + "claude-code/install.sh", + "claude-code/devcontainer-feature.json", + } +) +CONTEXT_NON_BUILD_INPUTS = frozenset( + { + # Prose, here because the Features spec wants it beside its feature. + "claude-code/README.md", + "claude-code/TROUBLESHOOTING.md", + # `initializeCommand`, which runs on the host before the container + # exists. devpod clears DevContainerActions out of the config half of + # the hash for exactly that reason, then hashed the script back in as + # context. + "claude-code/init-host.sh", + # devpod read this file to get here; it does not read it out of the + # context, and the config half of the hash already carries every build + # input it declares. + "devcontainer.json", + } +) LOCKFILE = REPO_ROOT / "pixi.lock" #: Lock-file format version -> the lowest pixi release that can read it. #: @@ -202,6 +241,42 @@ def test_a_readonly_spelling_the_membership_test_misreads_is_refused(flag): parse_mount(f"source=/a,target=/b,type=bind,{flag}") +def dockerignore_patterns() -> list: + """The `.dockerignore` lines devpod's pattern matcher will actually see. + + Comments and blank lines are dropped, which is what `ignorefile.ReadAll` + does before the patterns reach `patternmatcher`. Measured on devpod 0.26.1: + with this file's three patterns the debug line reports `excludeCount=4`, the + fourth being devpod's own feature-staging folder, which it appends itself. + """ + lines = CONTEXT_DOCKERIGNORE.read_text().splitlines() + return [line.strip() for line in lines if line.strip() and not line.startswith("#")] + + +def excluded_from_the_context_hash(rel_path: str) -> bool: + """Whether `.dockerignore` keeps `rel_path` out of the context hash. + + A deliberately small matcher, covering only the two pattern forms this + repository's `.dockerignore` uses: `**/*.ext`, which docker matches at any + depth including the top level, and an exact context-relative path. Any other + form raises rather than returning False, so this cannot quietly report "not + excluded" about a pattern it does not understand -- which is the one way a + guard like this fails without saying so. + """ + for pattern in dockerignore_patterns(): + if pattern.startswith("**/*."): + if rel_path.endswith(pattern[len("**/*") :]): + return True + elif "*" in pattern or "?" in pattern or "[" in pattern: + raise AssertionError( + f"{CONTEXT_DOCKERIGNORE.name} pattern {pattern!r} is a glob form this " + "test cannot evaluate; teach it the form or express the exclusion as a path" + ) + elif rel_path == pattern or rel_path.startswith(pattern + "/"): + return True + return False + + def test_devcontainer_manifest_is_this_repos_and_parses(devcontainer): """Comments stripped, this file is JSON, and it is the manifest we mean. @@ -866,3 +941,136 @@ def test_the_host_hook_heals_the_socket_it_is_mounted_from(devcontainer, mounts) f"init-host.sh does not heal a stale mount at ${variable}, which is what " "the manifest binds the agent socket from" ) + + +def test_the_dockerfile_copies_nothing_out_of_the_build_context(): + """The premise every exclusion below rests on. + + `.dockerignore` is safe here only because the build never reads the context: + no `COPY`, no `ADD`, so the tar devpod assembles is opened by nothing. Add + one and the excluded files stop being invisible to the image and start being + *missing* from it -- a build that fails, or worse, a stale prebuild that + matches a context it no longer describes. + + So this is the test to read first when `.dockerignore` looks wrong: it is the + claim, and the exclusions are downstream of it. + """ + instructions = [ + line.split(maxsplit=1)[0].upper() + for line in DOCKERFILE.read_text().splitlines() + if line.strip() and not line.lstrip().startswith("#") + ] + assert not {"COPY", "ADD"} & set(instructions), ( + f"{DOCKERFILE.name} now reads the build context, which .dockerignore is filtering; " + "either the copied paths have to be un-excluded or the exclusions have to go" + ) + + +def test_every_file_in_the_build_context_is_classified_as_a_build_input_or_not(): + """A file added under `.devcontainer/` has to be put on one side or the other. + + Left unclassified it defaults to the expensive answer -- hashed, so its edits + move the prebuild tag -- and defaults to it silently, because nothing about a + launch changes except that it takes minutes again. Failing here is the cheap + version of that conversation. + + `.dockerignore` itself is neither: it is what draws the line, and it is + hashed, correctly, since changing what the hash can see should move the tag. + """ + present = { + str(path.relative_to(CONTEXT_DIR)) + for path in CONTEXT_DIR.rglob("*") + if path.is_file() and path.name != ".dockerignore" + } + classified = CONTEXT_BUILD_INPUTS | CONTEXT_NON_BUILD_INPUTS + assert present == classified, ( + f"the build context holds {sorted(present - classified)} that no test has " + f"classified, and expects {sorted(classified - present)} that are not there; " + "add each to CONTEXT_BUILD_INPUTS or CONTEXT_NON_BUILD_INPUTS" + ) + + +def test_the_prebuild_tag_cannot_move_for_a_file_the_image_never_sees(): + """The fix for a prebuild that stopped being pulled. + + Measured on devpod 0.26.1, amd64, at 4db3427: appending a comment line to + `claude-code/README.md` and another to `claude-code/init-host.sh` moved the + tag from `devpod-5bf7be3e3e7e1b3f4fbb01a9b3ab88e7`, which CI had published + and every launch pulled, to `devpod-90e9641b9d3661f681e640eeedb3a410`, which + nothing had ever published. Neither file can reach the image. The cost was a + full local build per launch, on every branch carrying the edit, and the only + symptom was that opening a container was slow again. + """ + for rel_path in sorted(CONTEXT_NON_BUILD_INPUTS): + assert excluded_from_the_context_hash(rel_path), ( + f"{rel_path} cannot reach the image but is still hashed into the prebuild " + f"tag; {CONTEXT_DOCKERIGNORE.name} has to exclude it or editing it throws " + "the published prebuild away" + ) + + +def test_the_prebuild_tag_still_moves_for_every_file_the_image_is_built_from(): + """The other half, and the one that makes the exclusions worth having. + + An exclusion too wide is worse than none: the tag stops moving when the image + would genuinely differ, so a launch pulls an image built from a Dockerfile or + a feature installer that is no longer the one on the branch. That failure is + quieter than a slow build and much harder to attribute -- the container comes + up, it is simply not the container the branch asks for. + """ + for rel_path in sorted(CONTEXT_BUILD_INPUTS): + assert not excluded_from_the_context_hash(rel_path), ( + f"{rel_path} is built into the image but {CONTEXT_DOCKERIGNORE.name} keeps it " + "out of the prebuild tag, so a launch can pull an image that predates it" + ) + + +def test_the_excluded_manifest_leaves_its_build_inputs_in_the_hash(devcontainer): + """Excluding `devcontainer.json` is only safe while devpod hashes what it declares. + + `normalizeConfigForHash` keeps `name`, `features`, `overrideFeatureInstallOrder`, + `image`, `dockerfile`, `context` and the whole of `build`, and clears the rest -- + `mounts`, `containerEnv`, `postCreateCommand`, `customizations` -- because devpod + applies those when it creates the container rather than when it builds the image. + So the config half of the hash already carries every build input this file has, + and hashing its raw bytes on top added only the comments, which this manifest is + mostly made of. + + What this test holds is the boundary: every key in here is either one devpod + hashes or one it deliberately applies at create time. A key that is neither + would be a build input the tag could not see, which is the stale-image failure + the test above describes, reached from the other direction. + """ + assert "devcontainer.json" in dockerignore_patterns(), ( + "this test is about the manifest being excluded from the context hash, and it " + "is not excluded; either restore the exclusion or drop this test" + ) + hashed = { + "name", + "features", + "overrideFeatureInstallOrder", + "image", + "dockerfile", + "context", + "build", + } + applied_at_create = { + "mounts", + "containerEnv", + "postCreateCommand", + "customizations", + "initializeCommand", + "runArgs", + "workspaceFolder", + "workspaceMount", + "remoteUser", + "remoteEnv", + "privileged", + "init", + } + unaccounted = set(devcontainer) - hashed - applied_at_create + assert not unaccounted, ( + f"{DEVCONTAINER_JSON.name} declares {sorted(unaccounted)}, which this test cannot " + "say devpod hashes or applies at create time; if any of them affects the image, the " + "manifest can no longer be excluded from the context hash" + )