fix(agent-server): make DB sidecars reachable by name, isolate DEV/PROD data - #24
Conversation
The outer image installs podman with --no-install-recommends, which drops
the CNI dnsname plugin, so sibling containers on a user network were never
resolvable by name and the deploy-app skill's multi-container section could
not work at all. Install the plugin explicitly.
The skill now mandates per-environment DB sidecars (<project>-db-{dev,prod})
with per-environment data volumes (<project>-db-{dev,prod}-data), forbids
removing data volumes outside project deletion, and requires migrations
instead of drop-and-recreate. Drift-guard tests pin the conventions, and
container-smoke.sh gains a sidecar step (network DNS by name + volume
durability) plus delete-reap checks for the labelled network and volume.
|
TLDR; wdyt @alexanderkreidich Proposal: for now, keep the DB inside the DEV and PROD app containers instead of spinning it out as separate sidecars. Two containers, two volumes, no user-defined network, no cross-container DNS — which drops the dnsname dependency from the critical path, removes the CNI-persistence problem entirely, and keeps the DEV/PROD isolation this PR is actually after (it comes from the two volumes, not from the two containers). Simpler and more reproducible, which is what we want while the deploy loop is still an LLM following prose. |
|
I don’t think limiting this to SQLite or an embedded database will scale. The agent needs the flexibility to choose the Podman services each project requires. I propose implementing a code-level |
neuromaxer
left a comment
There was a problem hiding this comment.
Review — 6 findings
Verified the core fix first: golang-github-containernetworking-plugin-dnsname has dnsmasq-base as a hard Depends (not a Recommends), so --no-install-recommends still pulls the dnsmasq binary the plugin execs — the Dockerfile change genuinely works. And src/runtime/appContainers.ts really does remove labelled networks and volumes, so the new step-9 assertions are backed by real code.
Four findings are inline. Two land in files this PR doesn't touch, so they're here:
CNI network definitions are not persisted across an image upgrade — container/run-outer.sh:39 (medium)
This PR makes user-defined networks load-bearing for real user data, but rootless podman keeps CNI network definitions in $HOME/.config/cni/net.d, and only /workspace and /home/builder/.local/share/containers are named volumes — /home/builder/.config is a baked image layer (Dockerfile:120–125). run-outer.sh does docker rm -f + docker run on every image upgrade, so afterwards the containers and data volumes survive in the storage volume while every <project> network is gone. The entrypoint's podman start --all then fails for those containers with "network not found" and the user's app stays down until a manual redeploy.
The smoke test can't catch this because step 8 uses docker restart (writable layer preserved), not recreate. Fix: mount /home/builder/.config/cni as a volume, or point podman's network_config_dir into the already-persisted containers volume.
Name fallback omits the new -db-dev/-db-prod sidecars — src/runtime/appContainers.ts:38 (low)
The PR pins deterministic sidecar names (<project>-db-{dev,prod}) and volume names (<project>-db-{dev,prod}-data), but APP_ENVIRONMENTS/appContainersByName still only fall back to <project>-app-{dev,prod}. The module's own doc comment concedes labelling is "prose instruction to an LLM, not an enforceable invariant" — so when the agent omits --label on a sidecar, project deletion leaves the DB container running and holding its volume, and under APP_CONTAINER_RUNTIME=docker the subsequent volume rm -f of the labelled volume fails because it's in use. Both leak, with the user's data. Now that the names are a fixed convention, the fallback should cover them.
Bigger question: should we split the DB into sidecars at all?
The DEV/PROD DB split is the trickiest part of this PR, and four of the six findings are downstream of it — the prose-only --network, the missing per-env DB host, the non-persisted CNI networks, the reaper name fallback. Every one of them is a way for the agent to produce a deploy that looks green while the app has quietly lost or crossed its database.
Proposal: for now, keep the DB inside the DEV and PROD app containers instead of spinning it out as separate sidecars. Two containers, two volumes, no user-defined network, no cross-container DNS — which drops the dnsname dependency from the critical path, removes the CNI-persistence problem entirely, and keeps the DEV/PROD isolation this PR is actually after (it comes from the two volumes, not from the two containers). Simpler and more reproducible, which is what we want while the deploy loop is still an LLM following prose.
One correction on the premise, though: app container data is not on a volume today. The §2 and §3 command blocks run the app containers with only --label and -p — no -v at all. Only the sidecars get named volumes. Two separate things are true and easy to conflate:
- Inner container filesystems do survive an outer restart, because
builder-podman-storagepersists/home/builder/.local/share/containers. - They do not survive the iterate loop, which is
podman rm -f <project>-app-devon every redeploy and promote. That destroys the writable layer.
So baking the DB in isn't free — it needs the app containers to mount a named per-env volume (e.g. -v <project>-data-dev:/var/lib/postgresql/data), labelled like everything else, and the skill must state that a redeploy replaces the container but reuses that volume. Without the mount, baking the DB in makes data loss worse than the sidecar design: the user's data would die on every single redeploy rather than only on delete.
Worth pricing in before committing to it: a single container needs an entrypoint that starts the DB and then the app (or SQLite on the mounted volume, which is the genuinely simplest version of this), and a redeploy restarts the DB along with the app. Both seem like a fair trade for removing four failure modes, but it's your call. Happy to write up whichever direction you pick.
| --network <project> -v <project>-db-dev-data:/var/lib/postgresql/data <image> | ||
| ``` | ||
|
|
||
| and the same for PROD with `<project>-db-prod` + `<project>-db-prod-data`. Run |
There was a problem hiding this comment.
medium — --network <project> is prose only, so the next redeploy drops the app off it.
This sentence is the only place the app is told to join the project network. The canonical command blocks don't do it: deploy/redeploy (lines 72–77) and promote (lines 88–92) still run the app with just --label and -p, and the contract bullet at line 32 still says "Give each app container its own network (--network <project>), or the default one".
An agent that follows §2 literally on the next iterate loop — rm -f <project>-app-dev then re-run — puts the app back on the default podman network, where <project>-db-dev does not resolve. The app silently loses its database after a redeploy that otherwise reports success, and the health check in §4 won't catch it because the app still answers on its port.
Put --network <project> in the §2 and §3 command blocks themselves, and drop the "its own network / or the default one" wording at line 32 — it now contradicts this section.
| shared `<project>` network — **one sidecar per environment**, mirroring the | ||
| app's DEV/PROD split: `<project>-db-dev` and `<project>-db-prod`. **Only the | ||
| app containers publish host ports**, in the same two-number form as above; | ||
| siblings are reached by container name over the shared network (the DEV app |
There was a problem hiding this comment.
medium — no per-env DB host mechanism, so the promoted PROD container talks to the DEV database.
Per-environment sidecars require a per-environment DB host at run time. But the contract mandates "dev = prod, one Dockerfile, one build target" and "DEV and PROD differ only by image tag, container name, and host port" — and the skill shows no -e or env mechanism anywhere. The only mention of -e is the prohibition two bullets down ("Never pass secrets into app containers... into run with -e"), which reads as "don't use -e" if you're an LLM skimming for the rule.
The likely agent behaviour is baking <project>-db-dev into the image or app config. Promote then runs that same source as <project>-app-prod, which connects to the DEV database — exactly the cross-environment corruption this PR exists to prevent, and now with PROD writing to DEV's volume.
Add an explicit per-environment example (-e DATABASE_URL=postgres://...@<project>-db-dev:5432/app for DEV, -db-prod for PROD) and state plainly that app config does go in via -e — it's LLM keys specifically that must not.
| outer_exec podman run --rm --network "${PROJECT}" "${PROJECT}-app:dev" \ | ||
| sh -c "for i in 1 2 3 4 5; do wget -q -O /dev/null http://${PROJECT}-db-dev:${APP_PORT}/ && exit 0; sleep 1; done; exit 1" | ||
| # Redeploy rule: replacing the container must not touch the data volume. | ||
| check "volume data survives sidecar replacement" \ |
There was a problem hiding this comment.
low/medium — this leaves the network empty and the volume unused, so the new step-9 checks pass trivially.
The durability check podman rm -fs ${PROJECT}-db-dev and never re-creates it. From here on nothing is attached to the labelled network and nothing holds the labelled volume, with two consequences:
- Step 8's restart no longer covers a sidecar surviving an outer restart (the sidecar is gone by then).
- The step-9 checks at lines 257–260 can't fail for the reason the comment claims. It says the reaper must remove them "in dependency order: containers → networks → volumes" — but a regression that removed networks/volumes before containers, which is the real
network has active endpoints/volume is in usefailure, still goes green here, because there is no container to conflict with.
Re-running the sidecar after the durability check (a one-liner, image already built) makes DELETE reap a network with a live endpoint and a volume that's in use, which is what step 9 is meant to prove.
| --network "${PROJECT}" -v "${PROJECT}-db-dev-data:/data" "${PROJECT}-app:dev" | ||
| check "sibling on the network reaches the sidecar BY NAME (dnsname)" \ | ||
| outer_exec podman run --rm --network "${PROJECT}" "${PROJECT}-app:dev" \ | ||
| sh -c "for i in 1 2 3 4 5; do wget -q -O /dev/null http://${PROJECT}-db-dev:${APP_PORT}/ && exit 0; sleep 1; done; exit 1" |
There was a problem hiding this comment.
low — busybox wget with no timeout turns the DNS failure this step guards into a 30-minute CI hang.
There's no -T here, and the check helper has no timeout wrapper — every other network check in this script bounds itself with curl --max-time/--retry. busybox wget defaults to a 900 s network timeout.
So if the CNI bridge is misconfigured such that packets are blackholed rather than refused — precisely the failure class this step exists to catch — each of the 5 attempts blocks for 900 s. The job hits the workflow's timeout-minutes: 30 and dies with no FAIL line and no log tail, instead of reporting the failing check. Silent kill on the exact regression being guarded.
wget -T 3 fixes it; a bound on the enclosing podman run wouldn't hurt either.
Problem
The deploy-app skill's Multi-container apps section tells the agent that sidecars (
<project>-dbetc.) are "reached by container name over the shared network" — but that never worked:container/Dockerfileinstalls podman with--no-install-recommends, which drops the CNIdnsnameplugin (it is only a Recommends of podman on Ubuntu 24.04). podman 4.9.3 runs the CNI network backend, and withoutdnsnamecontainer-name resolution on apodman network createnetwork does not exist.podman network create+ a postgres sidecar succeed, but a sibling getsRESOLVE-FAILED/nc: bad address 'testproj-db'./usr/lib/cni/contains 15 plugins, nodnsname.network createor sibling DNS, so this was never caught.On top of that, the skill ran one shared
<project>-dbfor both environments — a bad DEV migration could corrupt PROD's real data — and said nothing about volume lifetime on redeploy (the deferred Durable app data item indocs/plans/builder-containers-plan.md).Changes
container/Dockerfile— installgolang-github-containernetworking-plugin-dnsnameexplicitly, with a comment (and a note that podman ≥ 5 drops CNI → switch to netavark + aardvark-dns then). No security-flag or recipe change.deploy-app/SKILL.md— per-environment sidecars<project>-db-{dev,prod}with per-environment volumes<project>-db-{dev,prod}-data; new Persistent data rules: DEV and PROD never share a database or volume, redeploy replaces containers but never removes data volumes (reaper deletes them by label on project delete), schema changes are migrations, not resets.test/deploySkill.test.ts— drift-guard tests pinning the per-env naming, the isolation rule, the never-volume rmrule, and "migrations, not resets".scripts/container-smoke.sh— new step [7b]: labellednetwork create+ per-envvolume create, run a sidecar, reach it by name from a sibling (guards the dnsname fix), and prove volume data survives container replacement; step [9] now asserts DELETE also reaps the labelled network and volume. Reuses the already-built app image, so the step pulls nothing new.@appx-org/agent-server.Validation
npm test→ 165/165 pass;npm run typecheckandnpm run checkclean;bash -non the smoke script.run-outer.shsecurity flags:DNS-AND-HTTP-OK,VOLUME-DURABLE-OK,NETWORK-REAPED-OK,VOLUME-REAPED-OK(postgres:16-alpine sidecar also verified end-to-end: resolves + accepts connections on 5432).container-smoke.shneeds a Linux VM (local Docker Desktop kernel cannot do native rootless overlay); thecontainer-smokeworkflow covers it — please trigger it on this branch viaworkflow_dispatch.