I think this is still ready. The new commits add DNSCluster clustering, PHX_HOST handling and more accurate env-var detection, and nothing in them should block.
DNSCluster. I checked the new start command against the runtime:
/bin/sh -c (controllers/sandbox/sandbox.go), so ${RELEASE_NODE:-…@$(hostname -i)} gets expanded.web.app.miren returns one A record per sandbox in the same app (handleAppMirenQuery in pkg/dns/dns.go), so DNSCluster can find each replica.setupHosts maps the sandbox hostname to the sandbox IP in /etc/hosts. So hostname -i gives the same address the peers resolve, and each node's release@IP name matches what the others connect to.Clustering only turns on when the lock has dns_cluster and the code reads DNS_CLUSTER_QUERY. That keeps it away from apps with their own discovery setup. The test that replaces the get_env call with nil shows the fallback to a plain start.
Env detection. The elixirCode anchor stops phx.new's commented-out System.get_env examples from being reported. The optional/required split handles the forms phx.new actually uses:
|| raise counts as required, including when the raise is on the next line.|| "default", get_env/2 and in ~w(...) count as optional.Reads that only appear in dev/test config are dropped. deps/ and _build/ are no longer scanned; that change applies to every stack, but I can't see it hurting any of them. PHX_HOST shows up in miren init under "must be configured manually" and doesn't block, which matches the guide.
Still open:
docker manifest inspect once over every entry.mix phx.new → deploy. That matters a bit more now, because the default Phoenix app starts through a shell $(hostname -i) expansion that no test runs; TestElixirDetect only compares the command string. One deploy with two replicas would check the node naming, the cluster join and the endpoint startup in one go.You've accepted the releases: releases() limitation, so I'm leaving that out.
evanphx's changes-requested review is still on the PR. He left it before pushing his commits, and it's his to clear once he's happy with what landed.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1271,
"verdict": "ready",
"event": "approve",
"summary": "I think this is still ready. The new commits add DNSCluster clustering, `PHX_HOST` handling and more accurate env-var detection, and nothing in them should block.\n\n**DNSCluster.** I checked the new start command against the runtime:\n- Sandbox commands run through `/bin/sh -c` (`controllers/sandbox/sandbox.go`), so `${RELEASE_NODE:-…@$(hostname -i)}` gets expanded.\n- `web.app.miren` returns one A record per sandbox in the same app (`handleAppMirenQuery` in `pkg/dns/dns.go`), so DNSCluster can find each replica.\n- `setupHosts` maps the sandbox hostname to the sandbox IP in `/etc/hosts`. So `hostname -i` gives the same address the peers resolve, and each node's `release@IP` name matches what the others connect to.\n\nClustering only turns on when the lock has `dns_cluster` **and** the code reads `DNS_CLUSTER_QUERY`. That keeps it away from apps with their own discovery setup. The test that replaces the `get_env` call with `nil` shows the fallback to a plain `start`.\n\n**Env detection.** The `elixirCode` anchor stops phx.new's commented-out `System.get_env` examples from being reported. The optional/required split handles the forms phx.new actually uses:\n- `|| raise` counts as required, including when the raise is on the next line.\n- `|| \"default\"`, `get_env/2` and `in ~w(...)` count as optional.\n\nReads that only appear in dev/test config are dropped. `deps/` and `_build/` are no longer scanned; that change applies to every stack, but I can't see it hurting any of them. `PHX_HOST` shows up in `miren init` under \"must be configured manually\" and doesn't block, which matches the guide.\n\n**Still open:**\n- **Hexpm tags:** only the default tag from the version table has been pulled. It's worth running `docker manifest inspect` once over every entry.\n- **No live deploy yet:** no one has run a real `mix phx.new` → deploy. That matters a bit more now, because the default Phoenix app starts through a shell `$(hostname -i)` expansion that no test runs; `TestElixirDetect` only compares the command string. One deploy with two replicas would check the node naming, the cluster join and the endpoint startup in one go.\n\nYou've accepted the `releases: releases()` limitation, so I'm leaving that out.\n\nevanphx's changes-requested review is still on the PR. He left it before pushing his commits, and it's his to clear once he's happy with what landed.",
"posted_to_pr": true,
"head_sha": "fa31454a7c83357c07efe216e1edaaac800bccd0"
}I think this is ready for human reviewers. The new work is version selection: the imagerefs.ElixirReleases table, and elixir_version.go reading .tool-versions and mise.toml. It works the way the guide describes. build.version wins over a pin, and a pin wins over the default. A value containing -erlang- is passed through as a full tag, and the bookworm check in GenerateLLB now only matters for that path, which is the only way a non-bookworm tag can get in.
The OTP fallback is kept narrow. It only applies when the Elixir minor is supported and the pinned OTP isn't. An unknown minor, or an explicit -otp-N suffix that isn't in the table, still errors and names the file it came from. servers/build/buildkit.go collects stack.Events() after GenerateLLB, so both the fallback event and the "Building on hexpm/elixir:…" event reach the build result. That's what the guide means by "the build output says exactly which versions it used." TestElixirBuildVersion and TestElixirPinnedVersion cover the suffix forms, mise list and table entries, the precedence rules and the error paths.
Two things are worth a look, neither blocking:
releases: is written inline (elixir.go:54). If a project writes releases: releases(), mixReleasesRe doesn't match, and the stack falls back to the OTP app name. It then runs mix release with no argument. With a single release defined, Mix builds that release under its own name. So the copy from _build/prod/rel/<app> fails, and WebCommand points at the wrong binary. Details in the inline comment.20260918 rebuild, and the Docker tests only pull the default (1.19.6 / OTP 28.5.0.7). If hexpm skipped a pairing in that rebuild, anyone who picks it gets a pull error. Running docker manifest inspect once over every entry before merging would rule that out.One point from my earlier review still stands: I'd still do one real mix phx.new → deploy before merging, since no test runs assets.deploy or checks that PHX_SERVER starts the endpoint.
releases: keyword list written inline. For releases: releases(), which is a common way to write it, namedRelease stays false and a non-umbrella app falls back to app:. Then mix release runs with no name. When there's exactly one release, Mix builds it under its own name, so the copy from _build/prod/rel/<app> fails with a not-found error, and WebCommand points at a binary that doesn't exist. One option: when mix.exs has releases: and this regex doesn't match, emit an event that tells the user to inline the block or set default_release:. Another is to copy whatever single directory ends up under _build/prod/rel/. Either would turn a confusing copy failure into something the user can act on.Verdict: ready
Both problems from my last review are fixed, and each fix has a test, so I think this can go to human reviewers.
elixir.go now uses slices.Sorted(maps.Keys(opts.EnvVars)). The comment on it says the order is part of the cache key, which should stop anyone from turning it back into a plain map range later. The deps, compile and release steps now get the same digest from one build to the next.hasPhoenix now checks the root mix.lock through hasLockedDep, so umbrella apps get PHX_SERVER=true and a generated SECRET_KEY_BASE. For assets under apps/, you chose to emit an umbrella-assets detection event and document the gap instead of building them. I think that's a reasonable scope for this PR. The documented onbuild workaround works: applyOnBuild runs each command through /bin/sh -c in /app, and the builder already has MIX_ENV=prod set, so cd apps/my_app_web && mix assets.deploy does what the guide says. TestElixirUmbrellaPhoenix checks the lock-based detection, the release name, SECRET_KEY_BASE and the warning event.The aptInstall/installNpm switch to locked cache mounts touches every stack, and I think it's a needed fix. apt takes an exclusive lock and fails right away if someone else holds it. With this change, the Elixir builder and runtime stages install packages in parallel against the same cache IDs. Every step lists the mounts in the same order (lists, then archives), so serialising them can't deadlock.
My earlier point about a real run still stands. No Docker test runs assets.deploy or checks that PHX_SERVER actually starts the endpoint, so I'd still do one real mix phx.new → deploy before merging.
Posted to the PR's comment threads when you submit.
slices.Sorted(maps.Keys(opts.EnvVars)), and the comment explains that env order is part of every later step's cache key. The deps, compile and release steps now get the same digest on every build.hasPhoenix now comes from hasLockedDep("phoenix", lock), so an umbrella gets PHX_SERVER and SECRET_KEY_BASE. The assets gap now shows up as an umbrella-assets event, and the guide documents an onbuild workaround. That workaround works because applyOnBuild runs each command through /bin/sh -c in /app. TestElixirUmbrellaPhoenix covers all of it. Documenting the gap is fine for this PR.Verdict: ready
I'd hand this to human reviewers, with two problems to fix or consciously accept first. All four issues from my last review are fixed in the code (replies on each thread). TestElixirWithoutConfigUsesBuildEnv removes config/ and calls System.fetch_env!("BUILD_TOKEN") at compile time, so one Docker build covers both of the fixes that mattered most.
Build env vars are added in random order, so the dependency cache misses on almost every build (elixir.go:218). opts.EnvVars is a Go map, and ranging over a map gives a different order each time. BuildKit's EnvList keeps env vars in the order they were added, and that order goes into the exec op's digest. So with two or more vars, the digest of every step after this loop changes from one build to the next. That includes local.hex, deps.get/deps.compile, compile and release. A typical Phoenix app has SECRET_KEY_BASE, PHX_HOST and DATABASE_URL set, so nearly every deploy would recompile all its deps from scratch. Ruby and Node use the same loop, but Ruby's copy runs after bundle install, so only assets:precompile pays for it. Moving the vars ahead of the mix steps was my suggestion, and it's what makes the order matter this much here. Sorting the keys first fixes it. applyOnBuild will then keep that order, because re-adding an existing key leaves it where it is.
Umbrella Phoenix apps build, but the endpoint never starts (elixir.go:122). The guide says umbrellas are supported once they have a releases: block. But hasPhoenix, hasAssets and assetsNpm only look at the root mix.exs and assets/. In a mix phx.new --umbrella project, {:phoenix, …} and the assets.deploy alias live in apps/<name>_web/mix.exs, and the assets live under that app too. So none of the three flags get set: PHX_SERVER is never exported, assets are never built, and SECRET_KEY_BASE isn't detected. The generated runtime.exs only sets server: true when PHX_SERVER is set, so the release boots without listening on $PORT. At minimum, detect Phoenix from mix.lock. hasLockedDep already does this, and the root lock includes phoenix. Then either handle the web app's assets, or say in the guide that umbrella Phoenix isn't supported yet.
One earlier point still stands: assets.deploy and PHX_SERVER don't run in any Docker test, so I'd still do one real mix phx.new → deploy before merge.
opts.EnvVars adds the vars in random order. BuildKit's EnvList keeps insertion order, and that order ends up in the exec op digest. So with two or more vars, every step from mix local.hex through mix release gets a different digest on each build, and the deps layer is rebuilt nearly every deploy. Sort the keys first (slices.Sorted(maps.Keys(opts.EnvVars))) and add them in that order.hasDep only reads the root mix.exs. In a mix phx.new --umbrella project, phoenix is declared in apps/<name>_web/mix.exs, so hasPhoenix stays false. That means PHX_SERVER=true is never set and the endpoint doesn't start. hasAssets and assetsNpm miss for the same reason: the alias and assets/ live in the web app. The guide says umbrellas work if they have releases:, so this setup is supported on paper. At minimum, use s.hasLockedDep("phoenix", lock), since the root mix.lock includes phoenix. Then either build the web app's assets or document that umbrella Phoenix isn't supported yet.Posted to the PR's comment threads when you submit.
copyDepInputs only adds config to the copy sources when s.hasDir("config") is true. TestElixirWithoutConfigUsesBuildEnv removes config/ from the fixture and builds it for real, so this is covered.builder right after MIX_ENV/LANG, before mix local.hex and every mix step after it. The new test's fetch_env! module attribute would fail the build if they arrived too late. I've opened a separate comment on this loop about the random map order, which now affects the deps cache.GenerateLLB now rejects any tag without -debian-bookworm-, and the error shows the default tag and links the Versions section. The test covers 1.18, a trixie tag and an alpine tag.assets/node_modules, and the comment explains why a root node_modules is left in place for the JS augmentations. That matches how skipJSInstall expects things to work.Verdict: caveats
This isn't ready for human review yet. The layering is sound: deps are resolved from mix.exs/mix.lock/config alone, mix compile runs before assets.deploy, and only the release ships on bookworm-slim. But two things in GenerateLLB will break real apps: plain Mix apps with no config/ directory, and apps that expect their configured env vars to be set while the build runs.
Must fix
config/ breaks the build (elixir.go:269). AllowEmptyWildcard only helps when the source actually contains a wildcard. That's why you wrote mix.lock*, and why bundleInstall copies .ruby-version* ("matched via wildcard so a missing file is not an error"). config has no wildcard, and mix new hasn't generated a config/ directory since Elixir 1.9, so a plain Plug/Bandit app without one should fail at this copy. Both fixtures include config/, so the tests don't cover it.elixir.go:250). BuildOptions.EnvVars is documented for "onBuild commands, asset precompilation". Ruby adds them before assets:precompile, and Node adds them before next build. Here they only arrive through applyOnBuild, which runs after mix compile and mix assets.deploy but before mix release. That has two effects:
config/prod.exs or the asset pipeline that reads an env var at build time won't see it.config/*.exs gets evaluated by both mix compile and mix release, so a value read through Application.compile_env can differ between them. Releases check for that mismatch at boot and refuse to start.Worth doing now
[build] version is passed straight through (elixir.go:149). The comment on ElixirDefaultTag explains why the builder has to be bookworm, yet a -debian-trixie- or -alpine- tag is accepted. The build succeeds, and the release then crashes at boot on bookworm-slim. A short value like 1.18, the way other stacks take versions, turns into a pull error that doesn't mention the tag format. Checking for -debian-bookworm- would give both cases a clear error.node_modules conflicts with skipJSInstall (elixir.go:174). When a root package.json sits next to a local node_modules, DetectAugmentations skips the npm install because it expects copyApp to bring the vendored tree in. This stack excludes node_modules from the context, so neither path installs those root JS deps.hello fixture. The phoenix fixture is only used for detection and env vars, so assets.deploy, the assets/ npm install and PHX_SERVER never run in a test. The old guide said it had been validated on a live cluster. The new guide makes the same kind of promise for the automatic build, so it's worth running mix phx.new → miren init → miren deploy once before merge.Locking the apt cache mounts is a real fix. The builder and runtime stages both run aptInstall, and without the lock they'd fight over apt's lock.
config has no wildcard, so AllowEmptyWildcard doesn't apply to it, and BuildKit should fail the copy when the directory is missing. mix new stopped generating config/ in Elixir 1.9, so a plain Plug/Bandit app without one would fail here. That's the same reason bundleInstall uses .ruby-version* and this line uses mix.lock*. I'd only copy config when s.hasDir("config") is true, and add a fixture with no config/ so the test covers it.opts.EnvVars only reaches the builder here, after mix compile (line 225) and mix assets.deploy (line 243) have already run. Ruby adds them before assets:precompile, and Node adds them before next build, as the BuildOptions.EnvVars doc describes. So build-time reads in config/prod.exs or the asset pipeline won't see the user's values. On top of that, mix release below re-evaluates the config with the vars set. Any Application.compile_env value that depends on one of them then differs between compile and release, and the release refuses to boot on that mismatch. I'd add the env vars to builder right after the MIX_ENV/LANG AddEnv calls, before any mix step runs.ElixirDefaultTag explains why the builder has to be bookworm. With a trixie or alpine tag, the build succeeds and the release crashes at boot on bookworm-slim, which is the hardest version of this failure to debug. A short value like 1.18 (how [build] version works for other stacks) gives a pull error that doesn't mention the tag format. I'd reject tags that don't contain -debian-bookworm-, with a message pointing at the Versions section of the Elixir guide.node_modules here clashes with the npm/yarn/bun augmentations. When a root package.json sits next to a local node_modules, DetectAugmentations sets skipJSInstall because it expects copyApp to bring that tree in (see the doc comment on applyAugmentations). This exclude keeps it out, so the root JS deps never get installed. Either drop node_modules from the excludes, or pass false for skipInstall in this stack.Verdict: not_ready