testutil: control-plane error contract and rotation-seam tests

[Kiran01bm]

Summary

Adds two AWS-seam tests to the Ministack AWS-boundary tier: a control-plane error contract test (typed RDS fault matching) and a password-rotation seam test (rotation via ModifyDBCluster lands on the running database). Both are behaviors the engine's discovery and connection code will rely on in production.

What

• ProvisionAuroraPostgres now returns an AuroraCluster handle (control-plane client, cluster/instance IDs, URL() / URLWithPassword()) so tests can drive further control-plane operations against the provisioned cluster.
• TestAuroraControlPlaneErrorContract — describing an unknown cluster and creating duplicate cluster/instance identifiers surface as the AWS SDK's typed RDS faults, matched with errors.As, never by message text.
• TestAuroraControlPlanePasswordRotation — ModifyDBCluster applies a new master password to the real database: the new password connects through pkg/dbconn, the stale one is refused with SQLSTATE 28P01 — the exact failure a mid-migration connection hits after a production rotation.
• docs/testing.md — the tier-share section now lists all three Ministack tests and their seams; the planned-growth list drops IAM-auth (not planned) and adds rotation recovery (once pkg/dbconn grows a credential-refresh hook); logical replication is called out as a data-plane concern.

Why

The tier existed with one provisioning E2E; these tests pin down the two seams the engine hits first in production — control-plane error handling during discovery, and credential rotation mid-migration. The rotation test also establishes the baseline the future credential-refresh hook must recover from.

One emulator fidelity gap found and documented: Ministack's duplicate-instance wire code is DBInstanceAlreadyExistsFault, while real AWS emits DBInstanceAlreadyExists — so the SDK cannot map it to the typed fault and that one case matches by error-code prefix (holds against both the emulator and a real endpoint). Worth contributing a fix upstream.

References

• Ministack
• docs/testing.md — "How much of the suite runs on Ministack"

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Review requested by Armand and performed by his agent — same two lenses used across this stack (#11, #10, #15, #14, #9, #8, #7): pg-sprite as an OSS-first, best-in-class Postgres DDL tool, and pg-sprite as a clean integration target for an orchestrator. Reviewed at head 9125d94, stacked on #11. An adversarial correctness pass is posted separately, and it closes this stack.

Two things here are better practice than the code they're attached to. First, the emulator divergence was found by running the thing rather than by reading its docs, and it's written down at the point of use with the real-AWS behavior spelled out beside it — most people would have quietly changed the assertion until it passed and left the next reader to rediscover why. Second, returning an AuroraCluster handle instead of a bare URL is the right refactor at the right time: it's what lets a test drive a second control-plane operation without the harness growing a parameter per scenario, and URLWithPassword keeps the rotation case from needing its own URL builder. The docs edit is honest in the same way as #11's — narrowing the reader/writer item to "metadata-level: every Ministack endpoint resolves to one shared container" is the author marking down their own future test's value before anyone asks.

The findings are about which contract these tests actually pin, and it's mostly not pg-sprite's.

OSS lens

1. A documented divergence with no upstream link becomes a permanent local workaround. The PR body says the Ministack wire-code bug is "worth contributing a fix upstream", and it is — but nothing in the repo links an issue, and the assertion is a prefix match that will keep passing after the bug is fixed. So the most likely outcome is that this comment is still in the tree in two years describing an emulator that stopped behaving that way. File the issue, link it from the code comment, and make the assertion fail loudly when the emulator changes (see adversarial finding 1). That's the difference between "known divergence, tracked" and "we worked around it".

2. Dropping IAM auth from planned growth narrows this tier's case considerably. testutil: AWS-boundary test tier via Ministack RDS/Aurora #11's list had reader/writer topology, Secrets Manager, and RDS IAM tokens; this PR removes IAM ("not planned") and adds rotation recovery. IAM auth was the item where an emulator genuinely earns a merge gate — token construction, expiry, and BeforeConnect refresh are real pg-sprite logic with timing-dependent failure modes. What's left is Secrets Manager resolution (a fetch-and-format step testable against any fake secret source) and rotation recovery (which, per the adversarial comment, is mostly pkg/dbconn dial-time logic testable against a plain container). Worth re-asking the question from testutil: AWS-boundary test tier via Ministack RDS/Aurora #11 directly: what is the first thing this tier will catch that the data-plane tier can't, and should it be a required merge gate before then?

3. The rotation scenario is a genuinely good idea and deserves to be a documented behavior, not only a test. "What happens to an in-flight schema change when the database password rotates" is a question every operator of a long-running DDL tool eventually asks, and pg-sprite is in a position to answer it precisely. The answer (adversarial finding 2) is more interesting than the test currently captures, and it belongs in docs/ next to the credential-refresh hook design — it's the kind of thing people choose a tool for.

Integration lens

4. The error contract that matters to an orchestrator isn't the RDS SDK's — it's what pkg/dbconn does with the failure. pg-sprite still makes no AWS API calls, so types.DBClusterNotFoundFault and friends are the SDK's contract with Ministack. The contract an integrator actually needs pinned is one line away and already correct: Retryable(&pgconn.PgError{Code: "28P01"}) returns false, so a credential rotation produces one clean failure rather than a retry storm against an auth-failing endpoint. I verified it (28P01 and 28000 false; 08006 and 55P03 true) — that's pg-sprite's own behavior, it's the property that would actually hurt if it regressed, and the rotation test is the natural place to assert it.

5. Rotation intersects executor: native CREATE INDEX CONCURRENTLY with fail-closed invalid-index recovery #15's executor in a way worth designing for now. BuildIndexConcurrently acquires a second connection after a failed build, for the catalog verdict — and that acquire happens potentially hours after the first one, on a fresh dial. A rotation during a long index build therefore turns a provable verdict into an unprovable one, and per executor: native CREATE INDEX CONCURRENTLY with fail-closed invalid-index recovery #15's fail-closed rule that surfaces as an InvalidIndexError telling an operator to drop an index nobody inspected. That's the concrete reason the credential-refresh hook is a safety feature rather than a convenience, and it's a good argument for BeforeConnect resolving credentials per-dial rather than the pool holding a password captured at construction.

6. AuroraCluster.URL() goes stale after a rotation, silently. The rotation test depends on exactly that — it's how the stale-password check works — but the handle is now shared API, and the next test that rotates will hand a stale URL() to something that expects it to connect. A Rotate(t, password) helper that updates the handle's current password (and leaves URLWithPassword for the deliberately-stale case) makes the intent explicit instead of implicit in call order.

Verified solid

28P01 is matched as a SQLSTATE on a typed *pgconn.PgError, not by message — the same discipline the rest of the repo applies, and the one place in this PR where a string match would have been genuinely tempting. The rotation poll uses named local deadline constants rather than inline durations, matching the harness convention. require.ErrorAs on the two faults the SDK does map correctly is the right assertion shape, and the duplicate-cluster case is a real round trip (the cluster genuinely exists, so the fault is genuine rather than synthesized). ApplyImmediately: true on ModifyDBCluster is necessary here and correctly set. The docs table and the coverage table were both updated in the same PR rather than drifting. CGO_ENABLED=0 go build ./... passes at this head.

This review was generated by Claude Code (claude-fable-5).

[aparajon]

🤖 Adversarial correctness review requested by Armand and performed by his agent — separate from the two-lens pass, and the last of this stack. Method: same as #11 — for a test, the attack is "what would still pass if the thing under test were removed", plus "does the scenario it names match the failure it reproduces". Verified against a real PostgreSQL 16 at head 9125d94; reproductions are in the collapsed block at the end.

Findings, most severe first

1. The error-contract test writes an emulator defect into the contract, and instructs engine code to copy it. The comment is explicit about what future production code should do:

// Match the code prefix so the assertion holds against both the
// emulator and a real endpoint — engine code consuming this error must
// do the same.
assert.True(t, strings.HasPrefix(apiErr.ErrorCode(), "DBInstanceAlreadyExists"), …)

But engine code running against real AWS must not do the same: real AWS emits DBInstanceAlreadyExists, which the SDK maps to types.DBInstanceAlreadyExistsFault, so the correct production match is the same errors.As the two assertions immediately above use. The prefix match is a workaround for a bug in one emulator, promoted to a documented instruction for code that will never talk to that emulator. It's also the exact thing the test's own doc comment forbids two lines earlier — "matchable with errors.As — never by message text" — and a wire-code prefix is the same category of match, just shorter.

The permanence is the worse half. strings.HasPrefix passes against DBInstanceAlreadyExists and DBInstanceAlreadyExistsFault, so when Ministack fixes the code the test keeps passing green and nothing ever prompts anyone to remove the workaround. If the divergence is worth documenting, it's worth asserting exactly: match types.DBInstanceAlreadyExistsFault as the real contract, and pin the emulator's wrong code with an exact-match assertion and a link to the upstream issue, so the day it's fixed the test fails and says why. A workaround that can't fail is a workaround that never leaves.

2. The rotation test doesn't reproduce the failure it names. The PR body and the test comment both call the stale-password refusal "the exact failure a mid-migration connection hits after a production rotation". It isn't — PostgreSQL doesn't re-authenticate an established session, so a connection that is already open sails straight through a rotation:

in-flight session after rotation: err=<nil> (result=2)
first dial after the pool recycled: SQLSTATE 28P01

The failure lands on the next dial, not on the rotation: the pool growing past its idle set, a MaxConnLifetime recycle, a reconnect after a network blip. So a schema change mid-flight when the password rotates does not fail — it keeps running, possibly for hours, and then fails at some later moment with no causal link to the rotation an operator will remember. That is a materially harder failure to diagnose than an immediate one, and it's the actual design input for the credential-refresh hook this test is meant to establish a baseline for: the hook has to fire per-dial (BeforeConnect), and the recovery test has to reproduce a recycle, not a fresh NewPool.

It also has a direct consequence for #15. BuildIndexConcurrently acquires a second connection after the build fails, to run the catalog verdict — a fresh dial, potentially hours after the first. A rotation during a long index build therefore converts a provable verdict into an unprovable one, which per that executor's fail-closed rule surfaces as an InvalidIndexError naming a DROP INDEX CONCURRENTLY for an index nothing inspected. Worth having the rotation test cover the shape that actually produces that.

3. Neither test's load-bearing assertion needs the control plane. The rotation test's outcome reproduces in 1.4 seconds against a plain container with ALTER ROLE … PASSWORD, identical SQLSTATE:

--- PASS: TestAdvRotationAssertionsWithoutControlPlane (1.42s)
    stale password after rotation: SQLSTATE 28P01

And the error-contract test asserts the AWS SDK's mapping of Ministack's wire codes — pg-sprite still makes no AWS API call anywhere outside internal/testutil. This is the same structural point as #11's finding 1, now with two more tests on it, and it matters more here because the tier is a required merge gate: three tests, none of which fails when pg-sprite breaks.

The constructive version is short. The assertion this test is one line from making is about pg-sprite, and it's the one that would hurt if it regressed: a rotation must produce one clean failure, not a retry storm against an auth-failing endpoint. I checked the classifier directly —

Retryable(SQLSTATE 28P01) = false
Retryable(SQLSTATE 28000) = false
Retryable(SQLSTATE 08006) = true
Retryable(SQLSTATE 55P03) = true

— and it's correct today. assert.False(t, dbconn.Retryable(err)) beside the existing SQLSTATE assertion turns the rotation test from a PostgreSQL demonstration into a pg-sprite regression test, at no cost.

4. The tier's provisioning cost triples, on a required gate. All three tests call ProvisionAuroraPostgres independently, so a CI run starts three Ministack containers and three sibling PostgreSQL containers, each under its own five-minute available deadline — and again inside the test job, which runs ./... (documented in docs/testing.md, so this is cost rather than surprise). Sharing one provisioned cluster across the three tests would cut it to one; the rotation test mutates the master password, so it would need to rotate back or be ordered last, which is a small price. Given that the tier is a merge gate and the freePort race from #11 has a five-minute detection cost, tripling the number of chances to hit it is worth weighing deliberately.

5. URL() is silently stale after a rotation. AuroraCluster.URL() always formats fixturePassword, so once a test rotates, the handle's most obvious accessor returns credentials the database no longer accepts. The rotation test relies on this deliberately — it's how the stale check works — but the handle is now shared harness API, and the next test that rotates leaves the trap armed for whatever runs after it. A Rotate helper that updates the handle's current password, leaving URLWithPassword for the deliberately-stale case, makes the intent explicit rather than encoded in call order.

Probed and held

28P01 is asserted on a typed *pgconn.PgError, not matched in a message — the correct discipline, and the tempting shortcut was right there. The duplicate-cluster and unknown-cluster assertions are genuine round trips against real state (the cluster exists; the identifier doesn't) rather than synthesized errors, so they'd actually catch an SDK mapping change. The rotation poll is bounded by named local constants and fails the test on expiry rather than continuing. ApplyImmediately: true is required for ModifyDBCluster to land without a maintenance window and is correctly set. The AuroraCluster refactor doesn't change the provisioning path — TestAuroraControlPlaneProvisionAndConnect's assertions are unchanged apart from the handle accessor. dbconn.Retryable correctly treats class 08 as transient and class 28 as terminal. CGO_ENABLED=0 go build ./... passes at this head.

Reproduction tests

internal/testutil/adv12_integration_test.go — findings 2 and 3, against PostgreSQL 16

package testutil_test

import (
	"errors"
	"fmt"
	"testing"
	"time"

	"github.com/jackc/pgx/v5/pgconn"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/internal/testutil"
	"github.com/block/pg-sprite/pkg/dbconn"
)

// The rotation test's assertions with the control plane removed: does the
// 28P01 outcome depend on ModifyDBCluster, or only on PostgreSQL?
func TestAdvRotationAssertionsWithoutControlPlane(t *testing.T) {
	url := testutil.StartPostgres(t)
	admin, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url})
	require.NoError(t, err)
	t.Cleanup(admin.Close)

	_, err = admin.Exec(t.Context(),
		"CREATE ROLE rotating LOGIN PASSWORD 'test-password-do-not-use'")
	require.NoError(t, err)

	stale := userURL(t, url, "test-password-do-not-use")
	rotated := userURL(t, url, "test-password-rotated-do-not-use")

	p, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: stale, LockTimeout: 300 * time.Millisecond})
	require.NoError(t, err, "the original password connects")
	p.Close()

	_, err = admin.Exec(t.Context(),
		"ALTER ROLE rotating PASSWORD 'test-password-rotated-do-not-use'")
	require.NoError(t, err)

	p2, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: rotated, LockTimeout: 300 * time.Millisecond})
	require.NoError(t, err, "the rotated password connects")
	p2.Close()

	_, err = dbconn.NewPool(t.Context(), dbconn.Config{URL: stale, LockTimeout: 300 * time.Millisecond})
	var pgErr *pgconn.PgError
	require.ErrorAs(t, err, &pgErr)
	assert.Equal(t, "28P01", pgErr.Code)
	t.Logf("stale password after rotation: SQLSTATE %s", pgErr.Code)
}

// What actually happens to a schema change that is mid-flight when the
// password rotates? The test under review only probes a fresh connect.
func TestAdvRotationDoesNotBreakLiveConnections(t *testing.T) {
	url := testutil.StartPostgres(t)
	admin, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url})
	require.NoError(t, err)
	t.Cleanup(admin.Close)
	_, err = admin.Exec(t.Context(),
		"CREATE ROLE rotating LOGIN PASSWORD 'test-password-do-not-use'; GRANT ALL ON SCHEMA public TO rotating")
	require.NoError(t, err)

	work, err := dbconn.NewPool(t.Context(), dbconn.Config{
		URL: userURL(t, url, "test-password-do-not-use"), MinConns: 1, MaxConns: 2,
	})
	require.NoError(t, err)
	t.Cleanup(work.Close)

	// One live connection, checked out and working — a schema change in flight.
	held, err := work.Acquire(t.Context())
	require.NoError(t, err)
	var one int
	require.NoError(t, held.QueryRow(t.Context(), "SELECT 1").Scan(&one))

	_, err = admin.Exec(t.Context(), "ALTER ROLE rotating PASSWORD 'rotated-do-not-use'")
	require.NoError(t, err)

	// The established session: does it notice?
	err = held.QueryRow(t.Context(), "SELECT 2").Scan(&one)
	t.Logf("in-flight session after rotation: err=%v (result=%d)", err, one)
	held.Release()

	// Now the pool recycles its connections — MaxConnLifetime expiry, a
	// network blip, or explicit Reset. The next dial re-authenticates.
	work.Reset()
	_, err = work.Acquire(t.Context())
	var pgErr *pgconn.PgError
	if errors.As(err, &pgErr) {
		t.Logf("first dial after the pool recycled: SQLSTATE %s", pgErr.Code)
	} else {
		t.Logf("first dial after the pool recycled: err=%v", err)
	}
}

func userURL(t *testing.T, base, password string) string {
	t.Helper()
	cfg, err := pgconn.ParseConfig(base)
	require.NoError(t, err)
	return fmt.Sprintf("postgres://rotating:%s@%s:%d/%s?sslmode=disable",
		password, cfg.Host, cfg.Port, cfg.Database)
}

Observed:

--- PASS: TestAdvRotationAssertionsWithoutControlPlane (1.42s)
    stale password after rotation: SQLSTATE 28P01

--- PASS: TestAdvRotationDoesNotBreakLiveConnections (1.34s)
    in-flight session after rotation: err=<nil> (result=2)
    first dial after the pool recycled: SQLSTATE 28P01

internal/testutil/adv12b_test.go — the pg-sprite assertion the rotation test is one line from making (no database needed)

// What does pg-sprite's own retry classifier do with the rotation failure?
func TestAdvRetryableOnAuthFailure(t *testing.T) {
	for _, code := range []string{"28P01", "28000", "08006", "55P03"} {
		t.Logf("Retryable(SQLSTATE %s) = %v", code, dbconn.Retryable(&pgconn.PgError{Code: code}))
	}
}

Retryable(SQLSTATE 28P01) = false
Retryable(SQLSTATE 28000) = false
Retryable(SQLSTATE 08006) = true
Retryable(SQLSTATE 55P03) = true

This review was generated by Claude Code (claude-fable-5). Findings 2 and 3 were reproduced against a live PostgreSQL 16 with the tests above; findings 1, 4, and 5 are static analysis of the harness and workflow.

[aparajon]

🤖 Approving on Armand's behalf. My two-lens review and adversarial correctness pass are posted above — the findings there are for follow-up, not fix-before-merge blockers.

This approval was submitted by Claude Code (claude-fable-5) at Armand's direction.