fix(build): give each worktree its own Postgres test port, and refuse to run when it is unreachable (TASK-2708) (#1253)

* fix(build): give each worktree its own Postgres test port, and refuse to run when it is unreachable (TASK-2708)

docker-compose.test.yml bound the host port to 5445, so exactly one worktree
could run make test-pg at a time. With concurrent worktrees the normal
operating mode that produced three incidents in an afternoon: a
port-already-allocated collision, a container dying mid-run under concurrent
suites, and a stack orphaned by a removed worktree blocking the port for
everyone.

The worst of the three forged a gate leg: go test exited 2 having executed NO
TESTS because the database was unreachable, and exit 2 with zero FAIL lines
reads like a pass at a glance.

Docker now assigns the host port and the Makefile reads it back with
docker compose port. Before running anything the target probes the HOST path
the tests will use, from a throwaway container, and refuses with a banner
saying no tests executed rather than letting an unreachable database look like
a result. If the suite fails and the database is gone afterwards, it says the
failures are infrastructure.

The compose project name was already per-directory, so teardown never could
reach a sibling; the orphan recovery command is now documented where someone
looking for it will be.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* docs: record that make test-pg is now safe from concurrent worktrees (TASK-2708)

The worktree section is where a reader learns what is safe to run alongside a
sibling, so it is where this belongs — including that a privately-started
container is no longer needed, and the recovery command for a stack orphaned by
a deleted worktree.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* docs(store): the mutation-harness recipe reads the port back instead of hardcoding it (TASK-2708)

A paste-ready recipe in a comment is a consumed artifact: it said 5445, and
after the ephemeral-port change pasting it would connect to whatever else is on
that port, or to nothing. Found by re-running the prose sweep with a
path-scoped exclusion — the first pass piped through 'grep -v node_modules',
which filters by LINE CONTENT and had silently eaten the hits in files whose
matching line mentions node_modules.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* docs(build): say that the banner discriminates, not the exit code (TASK-2708)

Measured while building the counterfactual matrix: make collapses every failed
recipe to exit 2, so the infrastructure refusals and an ordinary test failure
are indistinguishable by status. The banners are the only discriminator, and a
reader who assumed otherwise would build automation on a difference that does
not exist.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* fix(build): unique compose project, guarded startup, loud teardown failure (TASK-2708)

All four from codex round 1, each verified in the recipe before accepting.

Compose defaults the project name to the directory BASENAME, so two checkouts
sharing a basename share a stack and one down -v tears down the other's
database mid-run — the cross-worktree teardown this task exists to prevent,
reached through a second door. The project name is now explicit and keyed to
the absolute path. My compose comment had claimed the default was already
sufficient, in the place the next reader would believe it.

up --wait now runs inside the guarded block: a health-check timeout used to
abort the recipe before teardown, leaving the stack behind and creating exactly
the orphan this task was filed about.

A failed teardown is announced with the command to reap the stack instead of
being swallowed. It does NOT fail the build: the tests genuinely ran and their
status is honest; the leak is a separate fact and is now a loud one.

make test-pg-project prints the name so an orphan can be reaped without
re-deriving it.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* fix(build): portable readiness probe, safe project derivation, honest recovery docs (TASK-2708)

Five from codex round 2, each verified before accepting.

The readiness probe used `docker run --network host`, which is Linux-only by
default. On Docker Desktop a perfectly healthy database would have been
reported unreachable and the target would have REFUSED TO RUN where it used to
work — a guard against false greens turned into a false red. It now uses the
host's pg_isready when present and falls back to an in-container check, which
is weaker but never lies about the platform.

The project name interpolated CURDIR into shell command text, so a checkout
path containing a quote would have broken the quoting. The shell now reads its
own working directory instead.

Teardown failures on the three guard exits were silenced by >/dev/null,
contradicting the loud-teardown promise those same guards make.

Two docs were falsified by my own earlier commit in this branch: CLAUDE.md
still told the reader to reap a stack by directory name, and the mutation
recipe in the store test omitted -p entirely, which is exactly the
same-basename collision the change exists to prevent.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* fix(build): tear the stack down on interrupt; record why a post-run outage is not reported (TASK-2708)

Round 3, one finding accepted and one refused.

ACCEPTED: Ctrl-C during go test killed the recipe shell before down -v, leaving
an orphaned stack — the exact failure this task was filed about. An INT/TERM
trap set before up covers startup as well.

REFUSED, with the premise checked rather than argued: the reviewer asked for
the post-run banner's EXIT_CODE gate to be dropped so a database dying after a
passing run is reported. storetest.NewPostgres skips only when the env var is
EMPTY; a database that is gone produces t.Fatalf, not a skip. So exit 0 means
every Postgres-backed test completed against a live database, and failing the
leg because the container stopped afterwards would convert honest greens into
reds. Written into the Makefile so the next reviewer does not re-raise it.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* fix(build): probe the host-published port on every platform; interrupt reports teardown honestly (TASK-2708)

Round 4, both findings real.

The readiness fallback ran 'compose exec pg_isready', which answers whether the
server is alive INSIDE the container — a broken host port mapping passes it and
the guard is bypassed. Not a rarely-exercised path either: this box has no host
pg_isready, so the fallback is the branch that has been running all along. It
now reaches back through host.docker.internal from a throwaway container, which
is native on Docker Desktop and resolves on Linux via
--add-host=...:host-gateway. Verified against a live stack, with a negative
control on a port nothing listens on.

That is the third version of this probe. --network host was Linux-only and
would have falsely refused on Desktop; compose exec was portable but asked a
narrower question than the claim it carried.

The interrupt trap announced 'stack torn down' unconditionally, so an
interrupted run whose teardown failed reported successful cleanup. It now
reports what happened and names the command to reap the stack.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR
This commit is contained in:
xarmian
2026-09-05 10:17:47 -04:00
committed by GitHub
parent 14cb97593f
commit 5aa4bbe319
4 changed files with 194 additions and 15 deletions
+2
View File
@@ -41,6 +41,8 @@ Agent sessions take a `git worktree` per task rather than sharing the main check
- **A fresh worktree has no `web/.svelte-kit`** (gitignored, generated). Run `npx svelte-kit sync` in `web/` before any vitest/vite command — or `npm run check`, which syncs first. Without it, vitest fails with `Failed to load tsconfig '.svelte-kit/tsconfig.json': Tsconfig not found` regardless of how `node_modules` was set up. (This missing generated dir was historically misdiagnosed as a symlink problem — `npm ci` "fixed" it only because its `prepare` script runs `svelte-kit sync`.)
- **Never run `npm ci` in a worktree whose `web/node_modules` is a symlink — including via make.** `npm ci` lives in the `web` target, so every target whose dependency chain reaches it is off-limits too: currently `web`, `build`, `install`, `serve`, `web-check`, and `check` (via `web-check`). Everything else — `build-go`, `dev`, `restart`, `test`, `test-pg`, `lint`, `vuln`, `web-test`, `web-audit`, `dev-web`, `clean` — never reaches `npm ci`. `npm ci` deletes through the symlink into the shared tree, breaking every other worktree and session at once with a confusing `vitest: not found`. If you want a real, isolated `node_modules` instead of a symlink, `npm ci` in an un-symlinked `web/` is ~5s on a warm cache and regenerates `.svelte-kit` as a side effect.
- **`make test-pg` is safe to run from several worktrees at once** (TASK-2708). It used to bind the Postgres test container to a fixed host port, so a second worktree failed with `port is already allocated` and a stack orphaned by a removed worktree blocked the port for everyone. Docker now assigns the port and the Makefile reads it back, so each worktree gets its own container on its own port under its own compose project. If you have been starting a private container by hand to avoid the collision, you no longer need to. Two other things that target now does: it REFUSES to run, with a `NO TESTS EXECUTED` banner, when the database is unreachable — `go test` exiting 2 with zero FAIL lines had already been mistaken for a pass once — and it says so explicitly when the database dies mid-run, so the failures read as infrastructure rather than as evidence about the code. To reap a stack whose worktree was deleted before teardown: `docker ps --filter name=padtest-` lists them and the container name carries the compose project, so `docker compose -p <that-project> down -v` reaps it from anywhere. From inside the worktree, `make test-pg-project` prints the name and `make test-pg-down` does it for you. (The project is `padtest-<basename>-<checksum-of-the-absolute-path>` — NOT the bare directory name, which would collide between two checkouts sharing a basename.)
`web/vitest.config.ts`'s `server.fs.allow` note covers the other worktree wrinkle (symlink realpaths vs the dev-server file-serving guard) and points back at this section.
## Key Directories
+155 -9
View File
@@ -1,4 +1,4 @@
.PHONY: build test test-pg test-pg-down dev clean web dev-web serve restart lint install check vuln web-check web-test web-audit
.PHONY: build test test-pg test-pg-down test-pg-project dev clean web dev-web serve restart lint install check vuln web-check web-test web-audit
BINARY=pad
BUILD_DIR=./cmd/pad
@@ -58,16 +58,162 @@ test:
go test -timeout=45m ./... -v
# Run tests against PostgreSQL (starts a container automatically).
# Uses port 5445 to avoid conflicts with any local PostgreSQL.
test-pg:
docker compose -f docker-compose.test.yml up -d --wait
PAD_TEST_POSTGRES_URL="postgres://pad:pad@localhost:5445/pad?sslmode=disable" go test -timeout=45m ./... -v -count=1; \
EXIT_CODE=$$?; \
docker compose -f docker-compose.test.yml down -v; \
exit $$EXIT_CODE
#
# THE HOST PORT IS EPHEMERAL (TASK-2708). It was hardcoded to 5445, which let
# exactly one worktree run this target at a time; with concurrent worktrees the
# normal operating mode that produced three incidents in an afternoon, the worst
# of which forged a green-looking gate leg — `go test` exited 2 having run NO
# TESTS because the container was unreachable, and "exit 2 with zero FAIL lines"
# reads a lot like a pass to a quick glance.
#
# So: Docker assigns the port, we read it back, and we REFUSE TO RUN rather than
# let an unreachable database look like a test result. Never put a fixed host
# port back in docker-compose.test.yml or hardcode one here.
#
# AN INTERRUPT TEARS THE STACK DOWN TOO. Ctrl-C during `go test` kills this
# recipe's shell, and without the trap above `down -v` never runs — orphaning
# exactly the stack this task was filed about. Set before `up`, so an interrupt
# during startup is covered as well.
#
# WHAT THIS DELIBERATELY DOES NOT DO: report a database that dies AFTER a
# passing run. The post-run banner is gated on the tests having failed, and a
# reviewer asked for that gate to be dropped. It stays, because the premise
# does not hold — storetest.NewPostgres skips only when PAD_TEST_POSTGRES_URL
# is EMPTY, and a database that is gone produces t.Fatalf, not a skip. So under
# this target a dead database always fails the tests that touch it, and exit 0
# means every Postgres-backed test completed against a live one. Failing the
# leg on a container that stopped after the suite finished would turn honest
# greens red.
#
# THE READINESS PROBE TESTS THE HOST-PUBLISHED PORT, which is the path the
# tests take — not the container's own socket. Two ways in, because neither is
# universal: the host's pg_isready when it exists, otherwise a throwaway
# container reaching back through host.docker.internal (native on Docker
# Desktop, and `--add-host=...:host-gateway` makes it resolve on Linux too).
# An earlier version used `--network host`, which is Linux-only and would have
# made a healthy database read as unreachable on Desktop; the version after
# that fell back to `compose exec`, which answers "is the server alive inside
# the container" and would let a broken port mapping through the guard. This
# box has no host pg_isready, so the fallback is the branch that actually runs
# here — it is not a rarely-exercised path.
#
# THE BANNER IS THE DISCRIMINATOR, NOT THE EXIT CODE. make collapses every
# failed recipe to exit 2, so "the database was unreachable" and "tests failed"
# are indistinguishable by status — measured, not assumed. Do not key automation
# off the exit code expecting to tell them apart; grep the output for
# NO TESTS EXECUTED (nothing ran) or THE DATABASE DIED (it ran against a
# database that went away).
#
# Recovering an orphan (a stack whose worktree was removed before teardown):
# `docker ps --filter name=padtest-` lists them, and the container name carries
# the project. Reap one from anywhere with
# docker compose -p <project> down -v
# From inside the worktree itself, `make test-pg-project` prints the name and
# `make test-pg-down` does it for you.
# TEST_PG_PKGS narrows the run. Defaults to everything, which is what a gate
# wants; a narrower value is for checking this target's own plumbing (e.g. the
# concurrency acceptance) without two full-suite runs. A gate leg reported from
# a narrowed run is not a gate leg.
TEST_PG_PKGS ?= ./...
# An EXPLICIT project name, unique per absolute path (codex round 1, P2).
# Compose otherwise defaults it to the directory BASENAME, so two checkouts
# that happen to share a basename — /a/docapp and /b/docapp — share a stack,
# and one `down -v` tears down the other's database mid-run. That is the
# cross-worktree teardown this task exists to make impossible, reachable
# through a second door. The basename is kept in the name so an orphan is
# still identifiable by eye; the checksum of the full path is what makes it
# unique. Lowercased and punctuation-stripped because compose rejects
# anything else.
# $$PWD and pwd rather than $(CURDIR): make interpolates CURDIR into the
# command TEXT, so a checkout path containing a quote would break the quoting
# and run whatever followed it. The shell reads its own working directory
# instead, so no path text is ever parsed as command text. cksum is 32-bit and
# that is deliberate — it is portable to every platform this repo builds on,
# unlike sha1sum/shasum, and the basename is in the name too, so the checksum
# only has to separate same-named siblings rather than be cryptographic.
# `tr -d '\n'` BEFORE the -c translation, not after: `tr -c` treats the
# trailing newline basename emits as an invalid character and turns it into a
# dash, which produced `padtest-docapp-2708--1800854141`. Compose accepted it,
# so nothing failed — the doubled dash in the printed name is what showed it.
TEST_PG_PROJECT := padtest-$(shell basename "$$PWD" | tr -d '\n' | tr 'A-Z' 'a-z' | tr -c 'a-z0-9_-' '-')-$(shell pwd | cksum | cut -d' ' -f1)
COMPOSE_TEST := docker compose -p $(TEST_PG_PROJECT) -f docker-compose.test.yml
test-pg:
@trap 'echo ""; if $(COMPOSE_TEST) down -v >/dev/null 2>&1; then echo "test-pg: INTERRUPTED - stack $(TEST_PG_PROJECT) torn down."; else echo "test-pg: INTERRUPTED and TEARDOWN FAILED - reap it with: docker compose -p $(TEST_PG_PROJECT) down -v"; fi; exit 130' INT TERM; \
port=""; \
if ! $(COMPOSE_TEST) up -d --wait; then \
echo ""; \
echo "################################################################"; \
echo "# NO TESTS EXECUTED - the database container never came up #"; \
echo "# This is NOT a test result. The Postgres leg did not run. #"; \
echo "################################################################"; \
$(COMPOSE_TEST) down -v >/dev/null 2>&1 || echo "# ...and TEARDOWN ALSO FAILED: docker compose -p $(TEST_PG_PROJECT) down -v"; \
exit 1; \
fi; \
port=$$($(COMPOSE_TEST) port postgres 5432 2>/dev/null | sed 's/.*://'); \
if [ -z "$$port" ]; then \
echo ""; \
echo "################################################################"; \
echo "# NO TESTS EXECUTED - could not read the container's host port #"; \
echo "# This is NOT a test result. The Postgres leg did not run. #"; \
echo "################################################################"; \
$(COMPOSE_TEST) down -v >/dev/null 2>&1 || echo "# ...and TEARDOWN ALSO FAILED: docker compose -p $(TEST_PG_PROJECT) down -v"; \
exit 1; \
fi; \
url="postgres://pad:pad@127.0.0.1:$$port/pad?sslmode=disable"; \
if command -v pg_isready >/dev/null 2>&1; then \
pg_isready -h 127.0.0.1 -p "$$port" -U pad -q; ready=$$?; \
else \
docker run --rm --add-host=host.docker.internal:host-gateway postgres:17-alpine \
pg_isready -h host.docker.internal -p "$$port" -U pad -q; ready=$$?; \
fi; \
if [ $$ready -ne 0 ]; then \
echo ""; \
echo "################################################################"; \
echo "# NO TESTS EXECUTED - Postgres on port $$port is not ready #"; \
echo "# This is NOT a test result. The Postgres leg did not run. #"; \
echo "################################################################"; \
$(COMPOSE_TEST) down -v >/dev/null 2>&1 || echo "# ...and TEARDOWN ALSO FAILED: docker compose -p $(TEST_PG_PROJECT) down -v"; \
exit 1; \
fi; \
echo "test-pg: Postgres on 127.0.0.1:$$port (project $(TEST_PG_PROJECT))"; \
PAD_TEST_POSTGRES_URL="$$url" go test -timeout=45m $(TEST_PG_PKGS) -v -count=1; \
EXIT_CODE=$$?; \
if command -v pg_isready >/dev/null 2>&1; then \
pg_isready -h 127.0.0.1 -p "$$port" -U pad -q; still_up=$$?; \
else \
docker run --rm --add-host=host.docker.internal:host-gateway postgres:17-alpine \
pg_isready -h host.docker.internal -p "$$port" -U pad -q; still_up=$$?; \
fi; \
if [ $$EXIT_CODE -ne 0 ] && [ $$still_up -ne 0 ]; then \
echo ""; \
echo "################################################################"; \
echo "# THE DATABASE DIED DURING THE RUN. #"; \
echo "# Treat the failures above as INFRASTRUCTURE, not as evidence #"; \
echo "# about the code, and re-run before drawing any conclusion. #"; \
echo "################################################################"; \
fi; \
if ! $(COMPOSE_TEST) down -v; then \
echo ""; \
echo "################################################################"; \
echo "# TEARDOWN FAILED - the stack is still running. Reap it with: #"; \
echo "# docker compose -p $(TEST_PG_PROJECT) down -v"; \
echo "# The test status below is honest; this leak is a separate fact. #"; \
echo "################################################################"; \
fi; \
exit $$EXIT_CODE
# Tears down THIS directory's stack only: the project name is keyed to this
# directory's ABSOLUTE path, so it cannot reach a sibling's container even if
# the two directories share a basename.
test-pg-down:
docker compose -f docker-compose.test.yml down -v
$(COMPOSE_TEST) down -v
# Prints this directory's compose project name, so an orphan can be reaped
# from anywhere without re-deriving it by hand.
test-pg-project:
@echo $(TEST_PG_PROJECT)
dev: build-go
./$(BINARY) server start --host $(HOST)
+25 -4
View File
@@ -1,7 +1,26 @@
# PostgreSQL for integration tests.
# Usage: docker compose -f docker-compose.test.yml up -d
# make test-pg
# docker compose -f docker-compose.test.yml down -v
# Usage: make test-pg (starts, runs, tears down)
# make test-pg-down (tear down a stack left behind)
#
# THE HOST PORT IS EPHEMERAL AND THAT IS THE POINT (TASK-2708). It used to be
# hardcoded to 5445, which let exactly ONE worktree run the Postgres leg at a
# time: a second `make test-pg` failed with "port is already allocated", and a
# stack orphaned by a removed worktree blocked the port for everyone until
# somebody found and reaped it. With concurrent worktrees the normal operating
# mode, that produced three incidents in one afternoon — including a run that
# exited 2 having executed NO TESTS, which reads a lot like green at a glance.
#
# Docker assigns the host port; the Makefile reads it back with
# `docker compose port postgres 5432` and builds PAD_TEST_POSTGRES_URL from it.
# Do not reintroduce a fixed host port here, and do not hardcode one in a
# script — derive it, or you have rebuilt the collision.
#
# The Makefile also sets an EXPLICIT compose project name, keyed to the
# checkout's absolute path. Compose's default is the directory BASENAME, which
# is per-worktree in the usual layout but NOT unique — two checkouts sharing a
# basename would share a stack, and one `down -v` would tear down the other's
# database mid-run. An earlier version of this comment claimed the default was
# already sufficient; it is not, and the Makefile no longer relies on it.
services:
postgres:
@@ -11,7 +30,9 @@ services:
POSTGRES_PASSWORD: pad
POSTGRES_DB: pad
ports:
- "5445:5432"
# No host side: Docker picks a free port. Read it back rather than
# assuming one (see the header).
- "5432"
healthcheck:
test: ["CMD-SHELL", "pg_isready -U pad"]
interval: 2s
@@ -67,9 +67,19 @@ import (
// substantiate them, which is the point of writing down the exact edit. Re-run
// with:
//
// docker compose -f docker-compose.test.yml up -d --wait
// PAD_TEST_POSTGRES_URL="postgres://pad:pad@localhost:5445/pad?sslmode=disable" \
// P=$(make test-pg-project)
// docker compose -p $P -f docker-compose.test.yml up -d --wait
// PORT=$(docker compose -p $P -f docker-compose.test.yml port postgres 5432 | sed 's/.*://')
// PAD_TEST_POSTGRES_URL="postgres://pad:pad@127.0.0.1:$PORT/pad?sslmode=disable" \
// go test ./internal/store/ -run 'CrossWorkspaces_Opposing|JointlyExceedQuota' -count=1
// docker compose -p $P -f docker-compose.test.yml down -v
//
// The port is READ BACK and the project name is EXPLICIT (TASK-2708). This
// recipe hardcoded 5445 until that change, so pasting it now would reach
// whatever else is on that port, or nothing. The `-p` is not decoration
// either: without it compose falls back to the directory basename, and two
// checkouts sharing a basename would share this stack — which is the
// cross-worktree teardown the change exists to prevent.
//
// MUTATION A — remove the outer acquisition entirely.
//