test: add reader, connection-loss and failover checks to the AWS-boundary tier

[Kiran01bm]

Add read-only reader, connection-loss and metadata-failover checks to the AWS-boundary test tier, and move it to ministack 1.5.6-full.

Why

The AWS-boundary tier proved only that pg-sprite could reach an Aurora-shaped endpoint. It did not assert how the executor behaves when the endpoint is a read-only replica, when the connection drops mid-change, or when the cluster fails over — the three shapes an operator actually hits. Each of those already has a defined outcome in the code (terminal on SQLSTATE 25006, terminal execution-failed on connection loss) but nothing pinned it.

What

• internal/testutil/ministack.go: image 1.4.15-full → 1.5.6-full; cluster replication enabled so the cluster has a real hot-standby reader (a second member, so provisioning takes longer); one memberHostAddr helper resolves the host-published port of either member.
• internal/testutil/ministack_integration_test.go:
  • ReaderIsReadOnly — catalog preflight succeeds on the standby, a write fails with SQLSTATE 25006, and the connection layer treats it as terminal.
  • ConnectionLossDuringSchemaChange — stopping compute interrupts an in-flight DDL; the test pins the cause (backend admin_shutdown/crash_shutdown or a connection-level error — never the session's own statement_timeout), asserts the terminal execution-failed outcome, waits for every member to return, and proves the interrupted relation left no catalog trace.
  • MetadataFailoverKeepsWriterSession — FailoverDBCluster flips the API-visible writer while an established writer transaction stays usable; the emulator does not promote the standby at the data plane and the test claims nothing more.
  • RDS status literals are named constants; the two load-bearing subtest orderings are documented on the suite.
• docs/testing.md: describe the subtests, what each guarantees, the replication-driven provisioning cost, and the ordering constraints.

Before / after

Before
  make test-aws-boundary
    └─ connect to cluster endpoint ──> ok

After
  make test-aws-boundary
    ├─ connect to cluster endpoint       ──> ok
    ├─ ReaderIsReadOnly                  ──> SQLSTATE 25006 ──> terminal, not retried
    ├─ ConnectionLossDuringSchemaChange  ──> 57P01/57P02 or connection error ──> execution-failed
    │                                        restart: both members available, no relation left behind
    └─ MetadataFailoverKeepsWriterSession──> writer flips, open writer transaction still usable

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 530cde2.

Verdict: the reader and failover subtests prove what they claim; ConnectionLossDuringSchemaChange does not — both of its assertions pass for any failure of that DDL, and there is a live alternative cause inside the same subtest. Nothing here is unsafe to land — it's test-only — but that one subtest would go green on a bug it's meant to catch, so it's worth fixing before it becomes the thing someone trusts.

Findings

1. ConnectionLossDuringSchemaChange's two assertions don't discriminate the cause. executor.OutcomeCode maps everything outside the typed sentinel set to CodeExecutionFailed — its own doc says so (pkg/executor/code.go:119-141) — and dbconn.Retryable is false for every non-08, non-lock SQLSTATE. I ran the pairing rather than eyeballing it:

changeErr	OutcomeCode	Retryable	subtest verdict
plain errors.New	execution-failed	false	✅ passes
statement timeout 57014	execution-failed	false	✅ passes
undefined table 42P01	execution-failed	false	✅ passes
permission denied 42501	execution-failed	false	✅ passes

So the subtest passes if the DDL fails for any reason at all — and one is sitting right there: newClusterPool sets StatementTimeout: time.Minute while the payload is pg_sleep(300), so a slow StopDBCluster yields a server-side 57014 and the subtest still goes green while proving nothing about connection loss. The pg_stat_activity gate before the stop is the right in-flight proof and does its job; it's the post-conditions that need the teeth. A genuine connection loss cannot produce a *pgconn.PgError at all — the server is gone — so that's the discriminator.

2. The stop/start subtest mutates cluster-wide state the next subtest depends on, and only waits for half of it. After StartDBCluster it polls DescribeDBClusters until the cluster is available, then proves DDL works on the writer. FailoverDuringSchemaChange runs next and targets cluster.ReaderInstanceID — but nothing re-checks that the reader member came back after the restart. The suite comment documents exactly one ordering constraint (PasswordRotation last); this adds a second, undocumented one, and if the emulator brings members back lazily the failure lands in the failover subtest, pointing at the wrong seam.

3. instanceHostAddr is a line-for-line copy of siblingHostAddr. The two differ only in -cluster-%s → -instance-%s in the container name and the wording of three messages — ~24 duplicated lines including the Docker client lifecycle.

4. (nits) The new assertions hardcode "stopped", "starting", "failing-over", and "available" as literals while the file already keeps rdsStatusAvailable as a constant. Worth noting too that real RDS answers StopDBCluster with stopping, not stopped, so that one assertion pins an emulator divergence — fine for this tier if deliberate, but a reader will assume AWS shape from a test named for the AWS boundary. Separately, FailoverDuringSchemaChange reads like a data-plane failover mid-change; the doc comment and docs/testing.md are careful to say it isn't, so the name is the only thing overclaiming — something like MetadataFailoverKeepsWriterSession would match what it proves.

Action items

1. (Finding 1) Pin the cause: add var pgErr *pgconn.PgError; require.NotErrorAs(t, changeErr, &pgErr, "an interrupted connection must not surface as a server error") (testify v1.11.1 has NotErrorAs). Optionally raise the pool's StatementTimeout for this subtest, or shorten the pg_sleep, so the timeout can never race the stop.
2. (Finding 2) After the restart, also wait for ReaderInstanceID to report available — the same DescribeDBInstances poll the provision path already uses — and extend the TestAuroraControlPlane comment to say the stop/start subtest must precede the failover one.
3. (Finding 3) Collapse the two into one helper taking the name kind ("cluster" / "instance").
4. (optional) Use a constant for each new RDS status literal, and rename the failover subtest to what it proves.

Verified (tried to break, couldn't)

go build ./... clean and go vet -tags ministack ./internal/testutil/ clean at head; the ministack-tagged package compiles and links. ReaderIsReadOnly holds up under attack: pg_is_in_recovery() is the correct standby proof, EventuallyWithT around preflight.CheckTable is the right shape for waiting on replay rather than assuming it, and the 25006 assertion matches dbconn.Retryable's actual behavior — I read pkg/dbconn/retry.go and confirmed only lock-not-available, deadlock, serialization-failure and class 08 are retryable, so a read-only refusal is correctly terminal and the claim in the doc comment is accurate. FailoverDuringSchemaChange's writer-count loop correctly asserts exactly one writer rather than merely "the reader is now a writer," which is the stronger invariant. The docs changes match the code, including the deliberate disclaimer that Ministack does not promote the standby at the data plane — that honesty is worth more than the test it describes. I did not run the tier itself: it needs the ministackorg/ministack:1.5.6-full image and provisions real cluster members over minutes, so findings 1 and 3 are proved from source and a local executable probe, and finding 2 is reasoned from the poll conditions rather than observed.

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

[aparajon]

🤖 Two-lens product review (OSS adoption ease, then integration as an engine), same head 530cde2. Shorter than the correctness review above and separate from it on purpose.

Lens 1 — someone finds this repo and runs it against their own database

The tier gets meaningfully more honest with this change. The three shapes it now pins — reader endpoint, connection loss, failover — are exactly the ones an operator hits first, and docs/testing.md saying plainly that Ministack "does not yet promote the standby at the data plane, so the test deliberately makes no such claim" is the most valuable line in the diff: a newcomer can tell what is proven from what is merely emulated, which is usually the thing test docs hide.

One gap for that reader: MINISTACK_RDS_PG_CLUSTER_REPLICATION=1 is now set for every provision, and every run creates a second db.t3.medium member and waits for it. The tier's own docs lead with provisioning costing minutes, so it's worth a sentence saying that cost went up and roughly by how much — otherwise the first person to run make test-aws-boundary after this lands reads the longer wait as a hang.

Lens 2 — how a caller like SchemaBot consumes this

The reader subtest documents a seam that points the wrong way for a caller. It proves that catalog-only preflight succeeds against a hot standby and the refusal only arrives when the executor attempts a write. For anything driving pg-sprite programmatically that is the worst available ordering: plan and preflight report a green, the operator approves, and the change dies mid-apply as a generic execution-failed. The cheap discriminator is already in the test — pg_is_in_recovery() — so preflight could refuse a recovery-mode target up front with a typed disposition the caller can render at plan time, which is where a "you pointed at a reader" message actually helps someone. Pinning today's behavior in a test is right; I'd rather the pinned behavior were the refusal.

execution-failed as the catch-all limits what a caller can classify. Finding 1 in the correctness review is a test problem, but it rests on a product fact: connection loss, statement timeout, permission denied and a missing relation are indistinguishable to OutcomeCode. A caller deciding whether to retry, surface "target unreachable," or block the operator has to re-parse the underlying error to tell those apart — the exact string-parsing that typed outcome codes exist to avoid. A distinct code for an ambiguous connection interruption would be actionable on both sides, and this PR's subtest would then have something specific to assert.

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

[aparajon]

🤖 Approving on Armand's behalf after the adversarial correctness review above. Finding 1 is worth doing before this becomes a test someone trusts — flagging it, not gating on it.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/75, follow-up commit

Source: review comment 5533058628 (correctness) and 5533059002 (product, two lenses) at head 530cde2. Fixes are in the follow-up commit on this branch.

#	Concern	Status
1	ConnectionLossDuringSchemaChange's assertions pass for any DDL failure (OutcomeCode and Retryable don't discriminate), and a live alternative cause sits in the subtest: StatementTimeout: time.Minute vs pg_sleep(300) lets a slow stop yield 57014 and still go green	fixed — but not with the suggested require.NotErrorAs(PgError): at Ministack 1.5.6 StopDBCluster runs container.stop(timeout=5) on postgres:<major>-alpine, whose STOPSIGNAL SIGINT is a fast shutdown, so each backend does send FATAL 57P01 admin_shutdown before exiting and the interrupted statement surfaces as a *pgconn.PgError (observed in the local run). assertInterruptedByShutdown therefore pins the cause as SQLSTATE ∈ {57P01, 57P02} or a non-PgError connection-level error, and rejects everything else (57014 included). The subtest's session statement_timeout is set to 2× the payload so the race cannot exist; the payload duration and timeout are one pair of named constants. After restart the test also proves to_regclass(schema.interrupted) is NULL — the interrupted change left no catalog trace
2	Stop/start mutates cluster-wide state the next subtest depends on but waits only for the cluster status; the reader member may still be reviving when Failover… targets it, and the ordering constraint is undocumented	fixed — after StartDBCluster the test waits for the cluster, then awaitInstanceStatus(...) on both InstanceID and ReaderInstanceID; TestAuroraControlPlane's comment and docs/testing.md now state both load-bearing orderings (stop/start before failover; rotation last)
3	instanceHostAddr is a line-for-line copy of siblingHostAddr (~24 lines incl. Docker client lifecycle)	fixed — one memberHostAddr(t, ctr, kind memberKind, identifier) with typed memberKindCluster/memberKindInstance; callers and the awsAccountID comment updated
4 (nits)	RDS status literals hardcoded; "stopped" pins an emulator divergence (real RDS answers stopping); FailoverDuringSchemaChange overclaims a data-plane failover	fixed — rdsStatus* constants for every status the subtests observe; the stop assertion accepts stopping or stopped with the divergence documented on the constants; subtest and function renamed MetadataFailoverKeepsWriterSession, doc comment says it proves exactly the kept session and nothing more
product L1	Replication is now on for every provision, adding a second member and lengthening the wait; docs should say so or the first local run reads as a hang	fixed — docs/testing.md "How much of the suite runs on Ministack" now explains the replication flag, why the reader is needed, and that the price is a longer provisioning wait while both members reach available
product L2a	Preflight succeeds on a hot standby and the refusal arrives only at write time — the worst ordering for a programmatic caller; preflight should refuse a recovery-mode target (pg_is_in_recovery()) with a typed disposition	deferred — tracked as an internal follow-up (preflight refuses a recovery-mode target). Agreed on the direction; it is an engine behaviour change (new preflight check + disposition + doc/capability updates), not a test fix, so it gets its own small PR
product L2b	execution-failed is a catch-all: connection loss, timeout, permission denied and missing relation are indistinguishable to OutcomeCode; a distinct code for an interrupted write would be actionable	deferred — tracked as an internal follow-up (typed outcome code for an interrupted write). Same reasoning: a new typed outcome code touches the verdict contract and SchemaBot's adapter, so it lands separately; this PR's subtest already asserts the cause at the SQLSTATE level so it has teeth in the meantime