From ea7dafa36b2f5ddc14e20158e3b4aa526d7211b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 17:29:52 +0000 Subject: [PATCH 1/2] Make pg_isready probe TCP, which is what dependents connect to TestUploadAndRetrieve/filesystem failed 2 of 40 e2e runs, naming a different container each time -- upload-1 on run 160 (main, ff2f794d), plc-1 on run 182. That looked like a generic startup race. It is not; it is one bug with a precise mechanism, and plc-postgres's own log against plc's crash shows it: 16:55:27.593 temp server: listening on Unix socket ONLY 16:55:27.633 temp server: ready to accept connections <- healthcheck green 16:55:29.441 temp server: shut down 16:55:29.887 real server: listening on IPv4 0.0.0.0:5432 16:55:30.531 plc exits: ECONNREFUSED 172.18.0.6:5432 Without -h, pg_isready uses the Unix socket. The official postgres image runs initdb against a temporary server started with listen_addresses='', so the socket is up while TCP is refused, and the healthcheck passes during init. A dependent with condition: service_healthy then starts into a window where the port it connects to does not exist yet -- here 2.25 seconds wide, which is why it is intermittent and why the victim differs run to run. Six sites, all of them: five compose files plus smelt/pkg/generate/compose.go, whose output generated/compose/piri.yml is gitignored and regenerates. Verified the generator actually emits it, not just that the source changed. Postgres was the only class member: every other healthcheck in the tree is HTTP over localhost/127.0.0.1 or redis-cli ping, all TCP by construction, so none can have the socket-versus-port gap. Checked rather than assumed. check-pg-healthchecks.sh keeps it fixed. The rule has no list in it -- every pg_isready either names a host or is a bug -- so nothing needs updating when a service is added, and it covers Go as well as YAML because the generator builds one of these strings. It also fails when it finds nothing at all, since a check that silently guards an empty set is the failure it exists to prevent. Verified in both directions: 6 FAILs before the fix, 6 oks after. Not verified here: that e2e now passes reliably. A 5% flake cannot be shown fixed by one green run, and CI has no Docker-less way to exercise it. The mechanism is proven from the logs; the frequency is not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- .github/scripts/check-pg-healthchecks.sh | 56 ++++++++++++++++++++++++ .github/workflows/ci.yml | 2 + smelt/pkg/generate/compose.go | 2 +- smelt/systems/hilt/compose.yml | 2 +- smelt/systems/ingot/compose.yml | 2 +- smelt/systems/plc/compose.yml | 2 +- smelt/systems/swarf/compose.yml | 2 +- smelt/systems/upload/compose.yml | 2 +- 8 files changed, 64 insertions(+), 6 deletions(-) create mode 100755 .github/scripts/check-pg-healthchecks.sh diff --git a/.github/scripts/check-pg-healthchecks.sh b/.github/scripts/check-pg-healthchecks.sh new file mode 100755 index 00000000..5507c114 --- /dev/null +++ b/.github/scripts/check-pg-healthchecks.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# Every pg_isready must name a host, so the healthcheck probes TCP. +# +# Without -h, pg_isready uses the Unix socket, and the official postgres image +# runs initdb against a temporary server started with `listen_addresses=''` -- +# socket up, TCP refused. The healthcheck therefore passes *during* init, and a +# dependent with `condition: service_healthy` starts into a window where the +# port it actually connects to does not exist yet. +# +# This is not theoretical. From e2e run 35247949580, plc-postgres's own log +# against plc's crash: +# +# 16:55:27.593 temp server: listening on Unix socket ONLY +# 16:55:27.633 temp server: ready to accept connections <- healthcheck goes green +# 16:55:29.441 temp server: shut down +# 16:55:29.887 real server: listening on IPv4 0.0.0.0:5432 +# 16:55:30.531 plc exits: ECONNREFUSED 172.18.0.6:5432 +# +# The healthcheck was green 2.25 seconds before TCP existed. That is why +# TestUploadAndRetrieve/filesystem failed 2 of 40 e2e runs naming a *different* +# container each time -- whichever dependent lost the race that run. +# +# The rule has no list in it. Every pg_isready is either given a host or it is +# a bug, so nothing here needs updating when a service is added. It covers Go +# as well as YAML because smelt/pkg/generate builds one of these strings. +set -euo pipefail + +cd "$(dirname "$0")/../.." + +status=0 +checked=0 +while IFS= read -r hit; do + file="${hit%%:*}" + rest="${hit#*:}" + line="${rest%%:*}" + checked=$((checked + 1)) + case "$rest" in + *pg_isready*-h[[:space:]]*|*pg_isready*--host*) echo "ok $file:$line" ;; + *) echo "FAIL $file:$line pg_isready with no -h probes the Unix socket"; status=1 ;; + esac +done < <(grep -rn 'pg_isready' --include='*.yml' --include='*.yaml' --include='*.go' . | grep -v '^\./\.git/') + +if [ "$checked" -eq 0 ]; then + echo "No pg_isready healthchecks found -- this check has nothing to guard." + echo "That is suspicious rather than fine: it used to find several." + exit 1 +fi + +if [ "$status" -eq 0 ]; then + echo "All $checked pg_isready probes name a host." +else + echo + echo "Add -h 127.0.0.1. Without it the probe uses the Unix socket, which is" + echo "up during initdb while TCP is not, so dependents start too early." +fi +exit "$status" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 10d2bff4..b1b9c573 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,6 +56,8 @@ jobs: run: .github/scripts/check-base-images.sh - name: every pulled compose image is pinned by digest run: .github/scripts/check-stack-images.sh + - name: every pg_isready healthcheck probes TCP + run: .github/scripts/check-pg-healthchecks.sh - name: gofmt run: | unformatted=$(gofmt -s -l .) diff --git a/smelt/pkg/generate/compose.go b/smelt/pkg/generate/compose.go index ceed4140..e34771fa 100644 --- a/smelt/pkg/generate/compose.go +++ b/smelt/pkg/generate/compose.go @@ -150,7 +150,7 @@ func buildPostgresService() ComposeService { "piri-postgres-data:/var/lib/postgresql/data", }, Healthcheck: &Healthcheck{ - Test: []string{"CMD-SHELL", "pg_isready -U piri -d postgres"}, + Test: []string{"CMD-SHELL", "pg_isready -U piri -d postgres -h 127.0.0.1"}, StartInterval: "1s", Interval: "5s", Timeout: "3s", diff --git a/smelt/systems/hilt/compose.yml b/smelt/systems/hilt/compose.yml index cfdadecf..9ce179e8 100644 --- a/smelt/systems/hilt/compose.yml +++ b/smelt/systems/hilt/compose.yml @@ -136,7 +136,7 @@ services: volumes: - hilt-postgres-data:/var/lib/postgresql/data healthcheck: - test: ["CMD-SHELL", "pg_isready -U hilt -d hilt"] + test: ["CMD-SHELL", "pg_isready -U hilt -d hilt -h 127.0.0.1"] start_interval: 1s interval: 5s timeout: 3s diff --git a/smelt/systems/ingot/compose.yml b/smelt/systems/ingot/compose.yml index ebbad34a..c8f476ee 100644 --- a/smelt/systems/ingot/compose.yml +++ b/smelt/systems/ingot/compose.yml @@ -135,7 +135,7 @@ services: volumes: - ingot-postgres-data:/var/lib/postgresql/data healthcheck: - test: ["CMD-SHELL", "pg_isready -U ingot -d ingot"] + test: ["CMD-SHELL", "pg_isready -U ingot -d ingot -h 127.0.0.1"] start_interval: 1s interval: 5s timeout: 3s diff --git a/smelt/systems/plc/compose.yml b/smelt/systems/plc/compose.yml index e91ab0b4..b6dea7b5 100644 --- a/smelt/systems/plc/compose.yml +++ b/smelt/systems/plc/compose.yml @@ -48,7 +48,7 @@ services: volumes: - plc-postgres-data:/var/lib/postgresql/data healthcheck: - test: ["CMD-SHELL", "pg_isready -U plc -d plc"] + test: ["CMD-SHELL", "pg_isready -U plc -d plc -h 127.0.0.1"] start_interval: 1s interval: 5s timeout: 3s diff --git a/smelt/systems/swarf/compose.yml b/smelt/systems/swarf/compose.yml index 1a1e99a1..80ffe422 100644 --- a/smelt/systems/swarf/compose.yml +++ b/smelt/systems/swarf/compose.yml @@ -57,7 +57,7 @@ services: volumes: - swarf-postgres-data:/var/lib/postgresql/data healthcheck: - test: ["CMD-SHELL", "pg_isready -U swarf -d swarf"] + test: ["CMD-SHELL", "pg_isready -U swarf -d swarf -h 127.0.0.1"] start_interval: 1s interval: 5s timeout: 3s diff --git a/smelt/systems/upload/compose.yml b/smelt/systems/upload/compose.yml index 2df53600..2707d1f7 100644 --- a/smelt/systems/upload/compose.yml +++ b/smelt/systems/upload/compose.yml @@ -81,7 +81,7 @@ services: volumes: - upload-postgres-data:/var/lib/postgresql/data healthcheck: - test: ["CMD-SHELL", "pg_isready -U sprue -d sprue"] + test: ["CMD-SHELL", "pg_isready -U sprue -d sprue -h 127.0.0.1"] start_interval: 1s interval: 5s timeout: 3s From a8a6040b879acaf112124c8977f86f2323290235 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 17:33:52 +0000 Subject: [PATCH 2/2] Match an invocation of pg_isready, not a mention of one The guard failed CI on its own step name. ci.yml:59 reads "every pg_isready healthcheck probes TCP", the grep matched the bare word, and the check reported a workflow file as an unhosted healthcheck. A guard that cannot tell running a thing from naming it is not guarding the thing. Anchored to a preceding quote, so the name has to start a quoted command string. That keeps .github/ in scope rather than excluding it, which matters because an Actions services: block can carry --health-cmd "pg_isready ..." and excluding the directory would have made a real blind spot to dodge a false positive. The gap it does leave, named in the header rather than papered over: an unquoted invocation like `--health-cmd pg_isready`. Nothing here writes one, and the alternative -- matching every mention -- is exactly what just misfired. How I missed it: I verified the guard in both directions, then wired it into ci.yml, and did not run it again. The verification was of a tree that no longer existed by the time I pushed. Re-verified against the final tree this time, and on three cases rather than two: it still FAILs when -h is dropped from plc, it does not trip on an added prose mention, and the tree as pushed is clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt --- .github/scripts/check-pg-healthchecks.sh | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/scripts/check-pg-healthchecks.sh b/.github/scripts/check-pg-healthchecks.sh index 5507c114..8ec08ebc 100755 --- a/.github/scripts/check-pg-healthchecks.sh +++ b/.github/scripts/check-pg-healthchecks.sh @@ -23,6 +23,16 @@ # The rule has no list in it. Every pg_isready is either given a host or it is # a bug, so nothing here needs updating when a service is added. It covers Go # as well as YAML because smelt/pkg/generate builds one of these strings. +# +# It matches an *invocation*, not a mention: the name must be preceded by a +# quote, so it is the start of a quoted command string. The first version of +# this check did not, and its own step name in ci.yml -- "every pg_isready +# healthcheck probes TCP" -- failed it. A guard that cannot tell running a +# thing from naming it is not guarding the thing. +# +# The gap that leaves: an unquoted invocation, e.g. `--health-cmd pg_isready` +# in an Actions services: block. Nothing in this repository writes one, and +# the alternative -- matching every mention -- is what just misfired. set -euo pipefail cd "$(dirname "$0")/../.." @@ -38,7 +48,7 @@ while IFS= read -r hit; do *pg_isready*-h[[:space:]]*|*pg_isready*--host*) echo "ok $file:$line" ;; *) echo "FAIL $file:$line pg_isready with no -h probes the Unix socket"; status=1 ;; esac -done < <(grep -rn 'pg_isready' --include='*.yml' --include='*.yaml' --include='*.go' . | grep -v '^\./\.git/') +done < <(grep -rn '["'"'"']pg_isready' --include='*.yml' --include='*.yaml' --include='*.go' . | grep -v '^\./\.git/') if [ "$checked" -eq 0 ]; then echo "No pg_isready healthchecks found -- this check has nothing to guard."