feat(storage): add postgresconn package with rotation-surviving pool

[Kiran01bm]

Adds pkg/postgresconn — the PostgreSQL counterpart of pkg/mysqlconn: centralized DSN normalization plus a storage pool whose credentials survive secret rotation. Groundwork for the storage factory; no call sites adopt it yet.

Why

pgx has no equivalent of the MySQL hot-swap DSN driver, so rotation survival needs a custom database/sql/driver.Connector. Reload fires only after an authentication failure — never per connection — because the storage DSN may resolve through a remote secrets backend that must not be hit on every dial, and a failed reload arms a short cooldown so a backend outage costs one resolve attempt per window instead of one per rejected dial.

before (raw sql.Open("pgx", dsn)):
  secret rotated ──► new conn ──► 28P01 ──► pool errors until restart

after (postgresconn.OpenReloadable):
  secret rotated ──► new conn ──► 28P01
                                    │
                                    ▼
                     reload: re-resolve DSN (re-read secret)
                                    │ (on error: keep current config,
                                    │  cooldown before the next attempt)
                                    ▼
                     retry with fresh credentials ──► success

What

• Open normalizes the DSN before opening a pgx-stdlib pool: an RDS host with no explicit sslmode gets sslmode=require (the counterpart of the TLS mode mysqlconn injects); an explicit sslmode — including disable, with or without whitespace around = — always wins. Both URL and keyword DSN forms are handled via proper parsing.
• OpenReloadable wraps the pgx connector so a fresh dial rejected as unauthenticated (SQLSTATE 28P01/28000 — the signature of a rotated password) reloads the DSN and retries once. A reload error keeps the current credentials and arms a 30s cooldown; a generation counter collapses concurrent failed dials into a single reload.
• WithConnectTimeout applies on open and on the credential-reload path alike, and a reloaded raw DSN gets the same RDS normalization as the boot DSN.
• DSN parse errors never echo the DSN: a *url.Error in the chain reproduces the full URL — password included — so only its underlying cause is kept.

Integration tests prove a live ALTER ROLE ... PASSWORD rotation is transparent to the pool.

[Copilot]

Pull request overview

Adds a new pkg/postgresconn package that centralizes PostgreSQL DSN normalization (notably RDS sslmode=require defaults) and introduces a reloadable database/sql connector intended to let a long-lived storage pool survive password rotation by re-resolving credentials only after authentication failures.

Changes:

• Introduces Open and OpenReloadable helpers for pgx-stdlib pooling, including WithConnectTimeout options.
• Implements DSN normalization for both URL and keyword/value DSN forms, injecting sslmode=require only for RDS hosts when sslmode is absent.
• Adds unit + integration tests, including a live password rotation test using ALTER ROLE ... PASSWORD.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
pkg/postgresconn/postgresconn.go	New Postgres connection helper package with DSN normalization and a rotation-surviving reloadable connector.
pkg/postgresconn/postgresconn_test.go	Unit tests for DSN normalization, connect timeout option, and reloadable connector behavior (including concurrency dedupe).
pkg/postgresconn/postgresconn_integration_test.go	Integration tests validating pool functionality against a real Postgres container and transparent password rotation.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

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

[aparajon]

🤖 Strong PR — DSN normalization is careful, the generation-based reload dedup is a nice design, and the credential-redaction work is thorough. Three things worth addressing before the storage factory adopts this; none are live today since nothing calls it yet.

1. The reload cooldown doesn't cover a persistently-rejected credential

reloadCooldown arms lastReloadFail only when reload() errors (or the reloaded DSN fails to parse). When reload() succeeds but returns credentials the server still rejects, refresh bumps gen and clears lastReloadFail. The next Connect snapshots the new gen, so its auth failure passes the gen != failedGen dedup and reloads again — one secrets-backend resolve per new physical connection, with nothing to bound it.

This isn't only the transient rotation-lag window. It persists for any condition that keeps returning class-28 while the secret store keeps handing back the same value:

• secret sync is broken or lagging, so the mounted secret stays stale
• the role was dropped or renamed (28P01)
• no pg_hba.conf entry for the host/user/database, or pg_hba requires SSL and the connection isn't (both 28000, which isAuthError deliberately matches)

In those states every failed query costs one resolve, indefinitely — sustained load on the secrets backend at exactly the moment the storage DB is already unreachable and callers are retrying hardest. That's the amplification the cooldown was added for, and the package doc states the intent explicitly: "an outage there must not turn every rejected dial into a resolve call." Right now that holds only when the backend errors, not when it answers with a credential that doesn't work.

Suggested fix: arm the cooldown when the post-reload retry also fails authentication — Connect already has that result in hand:

conn, err = connectConfig(ctx, *fresh)
if err != nil && isAuthError(err) {
    c.armReloadCooldown() // reloaded credentials are no better; back off
}
return conn, err

Comparing the reloaded password against the one that just failed would work too, but keying off the retry result covers the dropped-role and pg_hba cases as well.

2. refresh() holds the mutex across reload(), so a slow reload stalls every new connection and ignores context deadlines

refresh takes c.mu for the whole call, including c.reload(). Every concurrent Connect blocks in snapshot() before it even dials — including connections whose current credentials are perfectly good. Because that's a plain sync.Mutex, the ctx passed to Connect can't interrupt the wait, so a hung reload blocks new storage connections past their deadlines instead of failing fast.

Holding the lock is what makes the gen dedup work, so this is a deliberate trade rather than an oversight — but WithConnectTimeout bounds the dial, not reload(), and the doc contemplates resolving "through a remote secrets backend." Combined with #1, a persistently-bad credential means the lock is being taken constantly, so a slow backend degrades toward blocking the pool's entire new-connection path.

Worth deciding now, while the reload callback is still being written: either run the reload single-flight style so waiters can select on ctx.Done(), bound reload() with a timeout, or document that the callback must be fast and non-blocking so the storage factory is built to that contract.

3. The TLS assertion in the reload test passes without the injection

In TestReloadableConnectorReloadReappliesOptionsAndNormalization:

assert.NotNil(t, fresh.TLSConfig, "a reloaded RDS DSN must get sslmode=require injected")

pgx defaults to sslmode=prefer when none is specified, and prefer yields []*tls.Config{tlsConfig, nil} — a TLS primary plus a plaintext fallback. So TLSConfig is non-nil under prefer too, and this assertion would still pass if the sslmode=require injection were dropped entirely. What actually distinguishes require is the absence of the plaintext fallback (require returns []*tls.Config{tlsConfig}).

Asserting fresh.Fallbacks is empty — or asserting on the normalized DSN string directly — would pin the behavior the test name claims. (Repo convention: assert on specific values, not just existence.)

Optional

The multi-host limitation is documented on ConnectionDSN (RDS detection considers only the first host, so an RDS fallback host gets no injection) but isn't pinned by a test. Since that's the security-relevant direction of the edge, a case in the TestConnectionDSN table would keep it from drifting silently.

Verified / cleared

• TLS parity with mysqlconn holds. I went looking for a gap here and there isn't one: Spirit's REQUIRED mode is RootCAs + InsecureSkipVerify: true, i.e. encryption without certificate verification, which is the same posture Postgres sslmode=require gives. The "counterpart of the TLS mode mysqlconn injects" claim is accurate. (Whether both should be verifying is a separate, pre-existing platform question, not this PR's.)
• The keyword-DSN regex fails safe. A quoted value containing " sslmode=" would suppress injection, but the resulting connection falls back to pgx's prefer default — still an encrypted connection to an RDS host, not cleartext. sslmode = disable / sslmode =disable (whitespace around =) are correctly treated as explicit and left alone.
• No data race on the shared config. c.cfg is published under the lock and never mutated after publication (refresh assigns a fresh pointer), so dereferencing it outside the lock in Connect is safe.
• gen dedup is correct. Concurrent dials that failed on the same generation collapse to one reload; a dial that failed against a superseded generation reuses the swapped config instead of reloading. Retry happens exactly once per Connect — no spin.
• Credential redaction is real. dsnParseError strips the *url.Error layer that reproduces the full URL, covered end-to-end with a sentinel password across ConnectionDSN / Open / refresh.
• All five Copilot threads are genuinely addressed in the head commit, not just replied to.

Unit tests pass locally, go vet is clean, CI is green.

Reviewed by Armand's AI agent (Claude Opus 5).

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent

ps: feedback addressed in #981 - 5476a8c

All three findings and the optional test gap are addressed — findings 1/3/optional in a new commit and finding 2 by the already-open stacked follow-up PR (branch kiran01bm/pg-reload-outside-lock).

#	Finding	Status	Explanation
1	Cooldown doesn't cover a persistently-rejected credential (successful reload clears it, gen bump defeats dedup)	fixed	Connect now arms the cooldown when the retry with reloaded credentials also fails auth — exactly the suggested shape; covers stale sync, dropped role, and pg_hba cases. Test pins one resolve per window.
2	refresh holds the mutex across reload(); a hung reload blocks healthy dials and ignores ctx	fixed	Restructured single-flight in the stacked follow-up PR: the reload runs outside the lock behind an in-flight guard, snapshot() never blocks, waiters select on their dial context. Deterministic hung-reload tests included.
3	assert.NotNil(TLSConfig) also passes under prefer; doesn't pin the require injection	fixed	Now also asserts Fallbacks is empty — the plaintext fallback's absence is what distinguishes require from prefer.
opt	Multi-host first-host-only RDS detection documented but not pinned	fixed	Added a TestConnectionDSN case: a multi-host DSN with an RDS fallback host gets no injection.

The "Verified / cleared" section confirms parity/redaction/race/dedup behavior — no action.

Source: #976 review 4891704470, posted by Armand's AI agent (Claude Opus 5).