Skip to content

Give the fast lane the Postgres client the full gate had, and assert both lanes keep it - #1255

Merged
bradflaugher merged 2 commits into
devfrom
claude/agents-review-fleet-promote-pw9idd
Aug 23, 2026
Merged

bradflaugher merged 2 commits into
devfrom
claude/agents-review-fleet-promote-pw9idd

Conversation

@bradflaugher

Copy link
Copy Markdown
Contributor

What changed, and why

The two loose ends flagged on promotion #1253, closed.

1. ci.yml's pg_dump fix, ported to dev. A cherry-pick of f60767e, so dev's ci.yml is now byte-identical to main's. main and dev had diverged by exactly this one fix; they no longer do. (Recap: /usr/bin/pg_dump is a symlink to postgresql-common's pg_wrapper, which dispatches on the version/cluster in ~/.postgresqlrc or /etc/postgresql-common/user_clusters — cluster configuration, not "the newest client installed" — so on a runner carrying a PostgreSQL 16 cluster the wrapper kept selecting 16 over a successful client-18 install, and the new major assertion failed with got=16.)

2. dev-ci.yml gets the same Postgres-client lane. This is the root cause of why that defect reached a promotion PR at all, and it turns out to have been costing coverage the whole time:

dev-ci.yml's Go job runs the same go test ./... against the same postgres:18 service as ci.yml's, and installed no matching client. The runner ships client 16, and TestBackupRestoreRoundTrip self-skips on a client/server major mismatch (internal/admincli/backup_test.go:155) — so the only coverage of fleet backup / fleet restore ran as a SKIP on every dev push. That is the green-but-vacuous shape this repo keeps writing post-mortems about, and it is the same hole #1250 closed on the full lane while leaving the fast lane wide open.

The second cost is the one that motivated this: a step that exists in one lane is a step the other lane cannot catch. ci.yml being the sole home of that install is precisely why its broken version survived to a dev→main PR.

Both lanes now carry the install byte-identically, including the $GITHUB_PATH append the wrapper makes necessary, and both assertions — install by absolute path in the step that performs it, PATH resolution in the following step, since $GITHUB_PATH only applies from the next step on.

3. A test so this class cannot recur. TestGoSuiteLanesInstallMatchingPgClient asserts, for both ci.yml and dev-ci.yml, that the lane installs a postgresql-client-N and appends /usr/lib/postgresql/N/bin to $GITHUB_PATH. Both halves have failed in production, one per lane — dev-ci.yml with no install at all, ci.yml with an install whose assertion could not pass — and neither failure was visible in its own lane, so "keep these in sync" as a comment is not enough. The service major itself is already covered by TestPostgresMajorAgreesAcrossCI, so this test asserts only the two things that actually broke.

The test names the two files rather than deriving them: benchmark.yml, e2e-canary.yml and ci.yml's own e2e-live job also declare a Postgres service but never run the Go suite, so demanding a pg_dump client of them would be noise — and pretending to infer "the lanes that run go test ./... against a service" out of YAML would be a worse lie than naming them.

How you verified it

The new test was mutation-tested, not just read. A test that passes unconditionally is the exact failure mode it exists to prevent, so both failure modes were induced:

  • Dropped the $GITHUB_PATH append → FAIL … installs postgresql-client-18 but never appends /usr/lib/postgresql/18/bin to $GITHUB_PATH.
  • Dropped the client install → FAIL … runs go test ./... against a Postgres service but installs no postgresql-client-N.
  • Restored → ok.

The restored coverage is live and green, measured rather than assumed. Before the ci.yml fix, pg_dump resolved to 16 on both lanes, so TestBackupRestoreRoundTrip had not actually executed in CI on either. Ran it for real against a local PostgreSQL 16 server with the matching client-16 first on PATH — the exact majors-agree condition CI now creates:

=== RUN   TestBackupRestoreRoundTrip
--- PASS: TestBackupRestoreRoundTrip (0.31s)

PASS, not SKIP — so this un-skips into working coverage, not into a new red.

Also: actionlint clean over every workflow, gofmt clean, go vet -tags fleet_host_executor ./scripts/ clean, and the full ./scripts/ suite green — including TestPostgresMajorAgreesAcrossCI and TestDuplicatedToolPinsAgree, the two neighbours this could have disturbed.

Scope and deviations

Scoped to the two loose ends and the test that keeps them fixed. No production code — two workflow files, one test file, one CHANGELOG entry.

Worth knowing about the effect on this lane: dev-ci.yml's Go job now runs TestBackupRestoreRoundTrip for real instead of skipping it, which adds the client install (~15s) and a sub-second test to a job that has been finishing in ~6 minutes against a 20-minute timeout. Deliberate — that test running is the point.

Nothing deferred, and nothing deviated from the two items as flagged.


  • CHANGELOG.md updated, if this is a user-visible change
  • A design note (docs/<FEATURE>.md) added, if this ships a feature — n/a, ships no feature
  • An ADR added or superseded in docs/adr/, if this adds, weakens or reverses an invariant — n/a, no invariant moves
  • The diff is scoped to one change (no unrelated refactors)

Generated by Claude Code

claude added 2 commits August 23, 2026 01:57
The assertion added in this range failed on its first real outing — got=16
against server 18 — over an install that had plainly succeeded
(`Setting up postgresql-client-18 (18.6-1.pgdg24.04+2)`). Installing the
package is not what decides the answer: /usr/bin/pg_dump is a symlink to
postgresql-common's pg_wrapper, and the wrapper's own header states the
rule — it calls the client "with the version, cluster and default database
specified in ~/.postgresqlrc or /etc/postgresql-common/user_clusters". That
is cluster configuration, not "the newest client installed", so on a runner
image carrying a PostgreSQL 16 cluster, adding a client package alongside it
changes nothing about the dispatch.

Verified rather than reasoned about: this container has the same shape as
the runner (/usr/bin/pg_dump -> ../share/postgresql-common/pg_wrapper, the
versioned tree at /usr/lib/postgresql/16/bin), and there the wrapper
likewise reports 16 while prepending the versioned bin dir resolves
pg_dump to it directly.

So the versioned bin dir goes first on $GITHUB_PATH. That is also what the
test needs: internal/admincli/backup.go execs `pg_dump` off PATH, with no
versioned path of its own, so PATH resolution — not package presence — is
the thing that decides whether backup_test.go's major-mismatch t.Skipf
turns the only coverage of `fleet backup`/`fleet restore` off.

Asserted in two places, because $GITHUB_PATH takes effect only from the
next step: the install by absolute path in the step that performs it, and
PATH resolution in a step of its own. The second one is the assertion that
mirrors what the test actually invokes, so the two cannot drift apart again
without going red.

The CHANGELOG entry claiming the step "now asserts the major" is corrected
in the same commit — the assertion was there and could not pass, which is
the kind of half-true the honesty-in-docs invariant is about.

Verified: actionlint clean over every workflow; go test ./scripts/ passes,
including TestPostgresMajorAgreesAcrossCI, which requires every postgres
major named anywhere in .github/workflows to agree.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018eFXcF6BM7bxFVEQH67iuk
dev-ci.yml's Go job runs the same `go test ./...` against the same server-18
service as ci.yml's, and installed no matching client. The runner ships
client 16, and TestBackupRestoreRoundTrip self-skips on a client/server
major mismatch, so the ONLY coverage of `fleet backup` / `fleet restore`
ran as a SKIP on every dev push — the green-but-vacuous shape this repo
keeps writing post-mortems about.

It had a second cost, which is why this is worth more than the coverage:
ci.yml being the sole home of that step is exactly why a broken version of
it could not surface until a dev→main promotion PR. A step that exists in
one lane is a step the other lane cannot catch.

So the install is now in both lanes, byte-identical, including the
$GITHUB_PATH append the wrapper makes necessary and the two assertions
(install by absolute path, PATH resolution in the following step, since
$GITHUB_PATH only applies from the next step on).

TestGoSuiteLanesInstallMatchingPgClient asserts both halves for both lanes.
Both halves have failed in production, one per lane — dev-ci.yml with no
install at all, ci.yml with an install whose assertion could not pass — and
neither failure was visible in its own lane, so a comment saying "keep
these in sync" is not enough. The test names the two files rather than
inferring them: benchmark.yml, e2e-canary.yml and ci.yml's e2e-live job also
declare a Postgres service but never run the Go suite, and pretending to
derive "the lanes that run go test ./... with a service" from YAML would be
a worse lie than naming them.

Verified:
- The new test fails on each failure mode and passes when restored — checked
  by mutation, not by reading it: dropping the $GITHUB_PATH line and
  dropping the client install each produce the intended error.
- TestBackupRestoreRoundTrip PASSES (not skips) once the majors agree. Ran
  it for real against a local PostgreSQL 16 server with the matching
  client-16 on PATH: `--- PASS: TestBackupRestoreRoundTrip (0.31s)`. Before
  the ci.yml fix that preceded this, pg_dump resolved to 16 on BOTH lanes,
  so this test had not actually executed in CI on either.
- actionlint clean over every workflow; gofmt, go vet and the full
  ./scripts/ suite clean.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018eFXcF6BM7bxFVEQH67iuk
@bradflaugher
bradflaugher merged commit 8df0305 into dev Aug 23, 2026
15 checks passed
bradflaugher added a commit that referenced this pull request Aug 23, 2026
…keeps both lanes honest (#1256)

Carries #1255. dev-ci.yml's Go job runs the same `go test ./...` against the
same server-18 service as the full gate and had no matching client, so
TestBackupRestoreRoundTrip hit its major-mismatch t.Skipf and the only
coverage of `fleet backup` / `fleet restore` ran as a SKIP on every dev push.
It also left ci.yml as the sole home of that step, which is why a broken
version of it could not surface until promotion #1253.

TestGoSuiteLanesInstallMatchingPgClient asserts both halves — the client
install and the $GITHUB_PATH append — for both lanes. Each has failed once in
production, one per lane, neither visibly in its own lane.

ci.yml is absent from this diff by design: #1255 cherry-picked its pg_dump fix
onto dev, producing content identical to main's, so the branches no longer
diverge there.

This promotion merged with zero conflicts, against twenty on #1253#1253
went in as a merge commit rather than a squash, so the ancestry link survived.
Merged as a merge commit for the same reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants