feat: use block/mysql (driver name "block-mysql")

[morgo]

What

block/mysql is now a hard fork with its own module path, rather than a replace-target for go-sql-driver/mysql. SchemaBot imports it directly and opens its pools under the name the fork registers, block-mysql.

Upstream go-sql-driver/mysql is no longer linked by any SchemaBot package. Getting there meant reimplementing the one thing that still pulled it in, which turned out to also be the fix for a startup break.

All three dependency pins are merged revisions:

dependency	pinned to
github.com/block/mysql	ee0a93fe — master
github.com/block/spirit	10804bbe — #1221, head of main
vitess.io/vitess → block/vitess	88d15fda — #23, head of release-24.0

Why the spirit bump forces this

Spirit's pkg/dbconn moved to block/mysql and retired its own copy of the RDS certificate bundle. A tls=rds DSN out of EnhanceDSNWithTLS only resolves for a consumer using the same driver package's TLS registry — registries are per-package globals, and nothing about them travels in the DSN. On the old pairing (spirit on block/mysql, SchemaBot on upstream) every RDS MySQL connection would have failed in ParseDSN with unknown config name: rds, before any dial.

That property is the whole story below, applied one level down.

The bug this fixes

Found in review of an earlier revision of this PR, and it is the same hazard one layer in: OpenReloadable could not open a MySQL storage pool against an RDS host, so the server did not start.

ConnectionDSN injects tls=rds. Open dials through block-mysql, whose registry has that name. OpenReloadable dialled through go-mysql/hotswap-dsn-driver, which embeds upstream and cannot be pointed at the fork — so it parsed the DSN with a registry that has no rds:

ConnectionDSN out = u:p@tcp(sb.cluster-abc123.us-west-2.rds.amazonaws.com:3306)/schemabot
                    ?interpolateParams=true&parseTime=true&timeout=30s&tls=rds&writeTimeout=1m0s

block/mysql ParseDSN   err=<nil>                                     TLS!=nil=true
upstream    ParseDSN   err=invalid value / unknown config name: rds     ← the hot-swap driver's parser
OpenReloadable         err=open reloadable MySQL connection: invalid value / unknown config name: rds

OpenReloadable's only caller is openStoragePool (pkg/serve/serve.go), on the DialectMySQL arm, during startup.

CI was green and that green was doing no work. tlsModeForHost injects tls=rds only when dbconn.IsRDSHost(addr), and no CI job points storage at an *.rds.amazonaws.com address. The break was host-shaped, not code-shaped, so nothing in the suite could see it.

Two fixes were available and both were rejected:

• Mirror rds into upstream's registry. Works, and preserves verification. But it keeps two registries in sync forever, and the sync is the thing that just broke.
• Drop the tls=rds injection, since block/mysql applies the RDS trust store itself. Equivalent for Open; for the hot-swap pool it would have taken the storage pool from failing loudly to connecting in the clear, on the connection that carries every credential and lease.

The fix taken removes the second driver instead, so there is one registry and the question cannot recur.

pkg/connreload — the reload machinery, driver-independent

The hot-swap driver's job was small: re-read credentials when a dial is refused, so a rotated secret does not need a pod restart. That is now pkg/connreload. A caller supplies two functions:

• Resolve(dsn) (driver.Connector, error) — raw DSN to dialer, called once at open and once per reload, never per dial.
• Refused(error) bool — does this dial error mean the server rejected these credentials.

Everything about when to reload lives in the package: one reload in flight at a time, one reload per generation of credentials however many dials failed against it, a cooldown so a secrets-backend outage is not amplified into one resolve per refused dial, and a reload that runs detached so a hung secret resolution cannot pin a pool connection slot.

It is one implementation because that scheduling is the subtle part, and it existed twice — postgresconn had its own copy, which this deletes. Both storage pools resolve their DSN through the same secrets machinery, so a difference in how aggressively they re-resolve it would have said nothing about either engine. MySQL's half is now 60 lines; PostgreSQL's is 35.

It is not a port. Three things the replaced driver got wrong, each now pinned by a test:

	hot-swap driver	pkg/connreload
reload callback	one package-level variable — opening a second reloadable pool silently repointed the first one's reload at the second one's secret	per pool
failing secrets backend	no cooldown: one resolve per refused dial	one resolve per 30s window
hung secret resolution	pinned the electing dial, holding a pool connection slot	reload detached; the dial returns on its own context
panicking resolver	crashes the process	recovered, treated as a failed reload

pkg/mysqlerr and keeping one driver

mysqlerr.Number/Is read a MySQL error code without asserting a driver's error type. With upstream gone the second branch is deleted, but the helper stays, because the failure mode it guards is silent: two MySQLError structs in two packages are field-identical and unrelated under errors.As, so a classifier that type-asserts keeps compiling and just stops recognizing deadlocks.

Since nothing fails to compile if a second driver returns, a depguard rule now denies both import paths with the reason in the message. That is the only module-wide enforcement available for this, and it is one line of config per denied path — happy to drop it if you would rather not add the linter.

Call sites that read codes: sqlstore/error_classifier.go, mysqlerr.Reason, engine/spirit.isLockWaitTimeout. None assert a driver type.

Also here

• The tidb parser replace is dropped. Spirit no longer needs the fork; upstream 511dba1dbe17 parses and re-renders every geometry subtype and SRID.
• postgresconn.rdsRootPool takes mysql.RDSTLSConfig().RootCAs — spirit deleted GetEmbeddedRDSBundle with its bundle. Same private Amazon roots regardless of engine, and RDSTLSConfig clones per call, so nothing is aliased with the MySQL side.

Scope

Mostly mechanical: the import path in ~70 files and the driver name at ~250 sql.Open sites, the large majority in tests.

Two things deliberately not renamed, because they are not driver names:

• topo.OpenServer("mysql", …) — a Vitess topo implementation name.
• sqlx.NewDb(db, "mysql") — a bindvar-dialect tag. sqlx's registry only knows mysql/sqlite3/nrmysql/nrsqlite3; "block-mysql" resolves to UNKNOWN and silently breaks named-query binding. (SchemaBot has no such call, but GAP does — flagging it since the same sweep runs there.)

Verification

• go build ./..., go vet ./... (compiles all test packages), gofmt -l clean, golangci-lint run clean on the default, integration and e2e tag variants
• go test ./... green; -race green on connreload, mysqlconn, postgresconn, mysqlerr, storage/..., serve
• Upstream is not linked: go list -deps -test ./... contains no github.com/go-sql-driver/mysql. It remains in go.mod as // indirect because testcontainers-go/modules/mysql test-imports it; no SchemaBot package does.
• The RDS case now has a test. TestReloadablePoolReachesRDSHostWithVerifiedTLS asserts both halves: that the injected TLS name resolves in the driver the pool dials with, and that what the pool dials with verifies the server — real roots, correct ServerName, InsecureSkipVerify=false, no plaintext fallback. Both halves are needed, because block/mysql applies RDS TLS on its own, so the resolved-trust assertions alone cannot distinguish "the name resolved" from "the driver supplied TLS anyway".
• Mutations, all killed. In connreload: removing the cooldown check, removing the stale-generation arm guard, running the reload inline instead of detached, removing the stale-generation dedup, removing recover(), ignoring the Refused predicate, never advancing the generation, never clearing the cooldown on success. In mysqlconn: dropping the RDS TLS injection, and weakening the mode to PREFERRED.
• baseline gofmt -l on main confirmed clean first, so the reformatted files are import-ordering from this change and nothing else

The sadscan:disable annotations cover false positives: obviously-fake localhost test DSNs, a doc-comment URI shape with literal user:pass placeholders, and prose comments containing the word "PlanetScale". The scanner reports every finding in a file once the file is touched, which is why untouched lines in postgresconn_test.go acquired annotations.

Related

• block/spirit#1221 — the retirement that makes this necessary
• block/vitess#23 / squareup/strata#424 — same retirement in the other two consumers

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Several test cleanups run SQL using t.Context() inside t.Cleanup, which is typically canceled at cleanup time and can silently skip cleanup and leave databases/tables behind.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates SchemaBot to use the github.com/block/mysql fork directly (registered as the database/sql driver name block-mysql) while still allowing the upstream go-sql-driver/mysql to remain linked via the hot-swap DSN driver. To avoid silent retry-classification regressions caused by two non-interchangeable MySQLError types, it routes MySQL error-code inspection through a new shared helper.

Changes:

• Switches MySQL driver imports and sql.Open calls across the codebase from "mysql" to "block-mysql" where SchemaBot expects to use the fork.
• Introduces pkg/mysqlerr.Number / pkg/mysqlerr.Is to read MySQL error codes from either linked driver’s error type and updates retry/lock-wait classification call sites accordingly.
• Updates Postgres RDS CA handling to source roots from block/mysql now that Spirit no longer embeds/exposes its own bundle; bumps block/spirit and block/vitess pins accordingly.

File summaries

File	Description
pkg/webhook/webhook_misc_integration_test.go	Use block-mysql for target DB setup in webhook integration tests.
pkg/webhook/webhook_integration_test.go	Switch test DB opens to block-mysql; includes MySQL container + service setup helpers.
pkg/webhook/vschema_only_check_integration_test.go	Use block-mysql for schemabot storage DB in vschema-only check tests.
pkg/webhook/terminal_apply_head_publish_test.go	Use block-mysql for schemabot storage DB in terminal publish test.
pkg/webhook/rollback_integration_test.go	Use block-mysql and forked mysql package in rollback integration tests.
pkg/webhook/plan_integration_test.go	Use block-mysql for target + storage DB access in plan integration tests.
pkg/webhook/plan_drift_integration_test.go	Use forked mysql package and block-mysql in drift integration tests.
pkg/webhook/plan_comment_retire_integration_test.go	Use block-mysql for schemabot storage DB in plan comment retirement tests.
pkg/webhook/plan_change_ownership_integration_test.go	Use block-mysql in drift + storage DB access during ownership tests.
pkg/webhook/fanout_two_deployment_integration_test.go	Use block-mysql for target + storage DB access in fanout tests.
pkg/webhook/failure_logs_integration_test.go	Use block-mysql for schemabot storage DB in failure-log summary tests.
pkg/webhook/direct_gate_integration_test.go	Use block-mysql for drift DB access when asserting gate behavior.
pkg/webhook/copy_discard_gate_integration_test.go	Use block-mysql for drift DB access in copy/discard gate scenarios.
pkg/webhook/control_integration_test.go	Register forked driver and use block-mysql for control-operation tests.
pkg/webhook/comment_authority_integration_test.go	Use block-mysql for schemabot storage DB in comment authority tests.
pkg/webhook/check_records_stopped_test.go	Use block-mysql for schemabot storage DB in check-record tests.
pkg/webhook/check_records_rollback_test.go	Use block-mysql for schemabot storage DB in rollback check-record tests.
pkg/webhook/check_records_refused_plan_test.go	Use block-mysql for schemabot storage DB in refused-plan tests.
pkg/webhook/blocked_gate_integration_test.go	Use block-mysql for drift DB access in blocked gate tests.
pkg/webhook/auto_plan_integration_test.go	Use forked mysql package + block-mysql in auto-plan integration tests.
pkg/webhook/apply_integration_test.go	Use block-mysql for target DB in apply integration tests.
pkg/webhook/apply_comment_integration_test.go	Use block-mysql for schemabot storage DB in apply-comment lifecycle tests.
pkg/webhook/apply_check_records_integration_test.go	Use block-mysql for schemabot storage DB in apply check-record tests.
pkg/testutil/mysql.go	Register forked driver for MySQL readiness probes in tests.
pkg/tern/shard_writethrough_integration_test.go	Use block-mysql for lease stamping DB in tern integration tests.
pkg/tern/local_resume_engine_logging_integration_test.go	Use block-mysql for local client DB operations in tern tests.
pkg/tern/local_dispatch_shard_integration_test.go	Use block-mysql for local shard dispatch DB operations.
pkg/tern/local_dispatch_attach_integration_test.go	Use block-mysql for attach dispatch DB operations.
pkg/tern/local_control_multiop_resume_integration_test.go	Use block-mysql for lease DB in multi-op resume fixture.
pkg/tern/local_control_cancel_settle_integration_test.go	Use block-mysql for control cancel/settle DB operations.
pkg/tern/local_client.go	Switch driver import to forked mysql package in tern local client.
pkg/tern/local_client_integration_test.go	Use forked mysql package + block-mysql across tern local client tests.
pkg/tern/local_apply_adopt_integration_test.go	Use block-mysql for local adopt apply fixture DB operations.
pkg/tern/grpc_retryable_pause_integration_test.go	Use forked mysql package + block-mysql for control-plane storage setup.
pkg/tern/grpc_control_rejection_integration_test.go	Use block-mysql for direct DB writes used in gRPC rejection tests.
pkg/storage/internal/sqlstore/webhook_events_test.go	Register forked driver; use block-mysql for storage tests.
pkg/storage/internal/sqlstore/retry_test.go	Switch mysql error type import to forked mysql package.
pkg/storage/internal/sqlstore/parity_test.go	Register forked driver and use block-mysql for parity harness.
pkg/storage/internal/sqlstore/mysql_test.go	Register forked driver and open test DB via block-mysql.
pkg/storage/internal/sqlstore/locks_test.go	Use block-mysql for concurrent lock-store test pools.
pkg/storage/internal/sqlstore/error_classifier.go	Route MySQL retry classification through pkg/mysqlerr.Is (no driver-type asserts).
pkg/storage/internal/sqlstore/error_classifier_test.go	Switch mysql error type import to forked mysql package.
pkg/storage/internal/sqlstore/checks_test.go	Use block-mysql for changed-rows store DB creation.
pkg/storage/internal/sqlstore/apply_operations_test.go	Use block-mysql for concurrent-driver pools and DB-error cases.
pkg/storage/internal/sqlstore/applies_test.go	Use block-mysql for concurrent-driver pools and DB-error cases.
pkg/serve/serve.go	Register forked driver for server runtime.
pkg/serve/serve_close_test.go	Register forked driver; open lazy handle via block-mysql in close-path test.
pkg/postgresconn/postgresconn.go	Source RDS root CA pool from block/mysql TLS config.
pkg/pendingdrops/cleaner_integration_test.go	Register forked driver and use block-mysql in pending-drops cleaner tests.
pkg/namedlock/namedlock_integration_test.go	Register forked driver in namedlock integration tests.
pkg/mysqlerr/number.go	New helper to extract MySQL error codes from either linked driver’s error type.
pkg/mysqlerr/number_test.go	Tests for dual-driver error-code extraction and non-interchangeable error types.
pkg/mysqlerr/mysqlerr.go	Update Reason to use mysqlerr.Number rather than a single driver type assertion.
pkg/mysqlerr/mysqlerr_test.go	Switch mysql error type import to forked mysql package.
pkg/mysqlconn/mysqlconn.go	Use block-mysql as the default driver name; document split with hot-swap driver.
pkg/mysqlconn/mysqlconn_test.go	Update expectation to block-mysql and parse DSN with forked mysql package.
pkg/localscale/tls_integration_test.go	Use forked mysql package + block-mysql for localscale TLS tests.
pkg/localscale/server.go	Register forked driver and use block-mysql for vtgate/vtcombo DB pools.
pkg/localscale/server_integration_test.go	Register forked driver and use block-mysql for localscale integration DB access.
pkg/localscale/server_deploy_integration_test.go	Register forked driver and use block-mysql for localscale deploy tests.
pkg/localscale/proxy.go	Use block-mysql for upstream DB opened per proxy client connection.
pkg/localscale/planetscale_recovery_integration_test.go	Switch mysql package import to forked mysql package.
pkg/localscale/managed.go	Use block-mysql for managed-cluster mysqld connections.
pkg/localscale/helpers.go	Use block-mysql for branch DB + backend mysqld connections.
pkg/localscale/handlers_branches.go	Use block-mysql when snapshotting branch schemas via mysqld.
pkg/inventory/static.go	Switch mysql DSN parsing to forked mysql package.
pkg/inventory/static_test.go	Switch mysql DSN parsing to forked mysql package; add sadscan annotations on Postgres DSNs.
pkg/inventory/connection_assembler.go	Switch mysql DSN parsing to forked mysql package; add sadscan annotation on PlanetScale token-name constant comment.
pkg/inventory/connection_assembler_test.go	Switch mysql DSN parsing to forked mysql package; add sadscan annotation on Postgres DSN assertion.
pkg/etre/resolver_test.go	Switch mysql DSN parsing to forked mysql package.
pkg/engine/spirit/spirit_integration_test.go	Switch MySQL test DB opens to block-mysql and mysql package to fork.
pkg/engine/spirit/helpers.go	Switch mysql package import to forked mysql package.
pkg/engine/spirit/failure_reason_test.go	Switch mysql package import to forked mysql package.
pkg/engine/spirit/existing_copy.go	Switch mysql package import to forked mysql package.
pkg/engine/spirit/direct.go	Switch mysql import to forked mysql and use mysqlerr.Is for lock-wait timeout detection.
pkg/engine/spirit/control.go	Switch mysql package import to forked mysql package.
pkg/engine/planetscale/tls.go	Switch mysql package import to forked mysql package for TLS config registration.
pkg/engine/planetscale/planetscale.go	Switch mysql package import to forked mysql package; open vtgate via block-mysql; add sadscan annotation on PlanetScale lint-error comment.
pkg/engine/planetscale/planetscale_test.go	Switch mysql package import to forked mysql package.
pkg/engine/planetscale/branch.go	Switch mysql package import to forked mysql package; open keyspace DBs via block-mysql.
pkg/engine/planetscale/apply.go	Switch mysql package import to forked mysql package; open branch connection via block-mysql.
pkg/api/telemetry_integration_test.go	Register forked driver and use block-mysql for storage DB in telemetry tests.
pkg/api/service_integration_test.go	Register forked driver and use block-mysql for storage DB in service integration tests.
pkg/api/rollback_plan_integration_test.go	Register forked driver and use block-mysql for storage DB in rollback-plan integration tests.
pkg/api/pending_drops_cleaner_integration_test.go	Register forked driver for pending-drops cleaner API integration tests.
pkg/api/operator_multi_operation_integration_test.go	Register forked driver for operator multi-operation API integration tests.
pkg/api/mysql_shared_integration_test.go	Switch mysql package import to fork; open shared MySQL via block-mysql.
pkg/api/ensure_schema_integration_test.go	Register forked driver for ensure-schema integration tests.
pkg/api/enqueue_authorized_apply_integration_test.go	Register forked driver and use block-mysql for storage DB in enqueue tests.
pkg/api/config.go	Switch mysql DSN parsing import to forked mysql package.
pkg/api/config_test.go	Switch mysql DSN parsing import to forked mysql package.
integration/workflow_test.go	Use block-mysql for integration workflow DB setup and verification queries.
integration/status_cli_test.go	Use block-mysql for schemabot storage DB in status CLI integration tests.
integration/setup_test.go	Switch mysql package import to fork; use block-mysql for container schema init and tern storage opens.
integration/serve_boot_retry_test.go	Switch mysql package import to forked mysql package.
integration/resolve_apply_id_test.go	Use block-mysql for schemabot + target DB access in remote apply-id integration tests.
integration/operator_test.go	Use block-mysql for schemabot storage + target DB access in operator integration tests.
integration/hybrid_mode_test.go	Switch mysql package import to fork; use block-mysql in hybrid-mode integration tests.
integration/grpc_integration_test.go	Switch mysql package import to fork; use block-mysql for schemabot + target DB access.
integration/cli_test.go	Switch mysql package import to fork; use block-mysql for CLI integration test DB operations.
go.mod	Add github.com/block/mysql; bump github.com/block/spirit; update Vitess replace to block/vitess pin.
go.sum	Add checksum entries for block/mysql; update checksums for new Spirit/Vitess pins.
e2e/testutil/db.go	Register forked driver; use block-mysql for MySQL test utility helpers.
e2e/local/local_test.go	Register forked driver and use block-mysql in local E2E tests.
e2e/local/helpers_test.go	Use block-mysql for local E2E test helpers (open/cleanup/fixtures).
e2e/local/apply_wait_test.go	Use block-mysql for diagnostics DB access in apply-wait E2E test.
e2e/k8s/k8s_test.go	Switch mysql import to fork; use block-mysql for k8s E2E verification/cleanup.
e2e/k8s/dataplane_progress_ownership_test.go	Use block-mysql for index existence verification in dataplane progress tests.
e2e/grpc/multideploy_test.go	Switch mysql import to fork; use block-mysql for multi-deploy gRPC E2E DB ops.
e2e/grpc/helpers_test.go	Register forked driver; use block-mysql for gRPC E2E helpers and cleanup.
e2e/grpc/grpc_test.go	Register forked driver; use block-mysql for gRPC E2E setup/cleanup DB access.

Review details

Suppressed comments (3)

pkg/webhook/webhook_integration_test.go:1022

• This t.Cleanup cleanup executes DROP DATABASE statements with t.Context(), which is typically canceled by the time cleanup runs; that can leave staging/production databases behind. Use a non-canceled cleanup context (optionally with a timeout) for the DROP statements.
  pkg/webhook/plan_integration_test.go:815
• This t.Cleanup cleanup executes DROP DATABASE with t.Context(), which is usually canceled by the time cleanup runs; that can leave the test database behind. Use a non-canceled cleanup context (optionally with a timeout) for the DROP.
  e2e/k8s/k8s_test.go:532
• This cleanup uses t.Context() inside t.Cleanup; by the time cleanup runs the test context is typically canceled, so the DROP TABLE can be skipped and leave objects behind for later tests. Use a non-canceled cleanup context (optionally with a timeout) for the DROP.

	t.Cleanup(func() {
		db, err := sql.Open("block-mysql", dsn)
		if err != nil {
			return
		}
		defer utils.CloseAndLog(db)
		_, _ = db.ExecContext(t.Context(), "DROP TABLE IF EXISTS `"+tableName+"`")
	})

• Files reviewed: 110/111 changed files
• Comments generated: 2
• Review effort level: Lite

---

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

[morgo]

🤖 Half right, and the half that is right is out of scope for this branch.

The t.Context()-in-t.Cleanup problem is real. Verified rather than assumed: inside a t.Cleanup function, t.Context().Err() is already context canceled, because t.Context() is cancelled just before cleanups run. Any SQL issued with it fails, and it fails silently wherever the return is discarded. It is 152 sites across 42 files and predates this branch — usetesting with context-background: true is enabled here, which is what steers people to t.Context() in the first place — so it is tracked separately rather than folded into a driver rename.

The syntax claim is a misread. This file has no DROP DATABASE IF NOT EXISTS. Line 58 is DROP DATABASE IF EXISTS; the IF NOT EXISTS you matched is on the CREATE DATABASE two lines below it, at :51.

[morgo]

🤖 Real, and tracked separately. t.Context() is cancelled just before t.Cleanup functions run — verified, ctx.Err() reads context canceled inside the cleanup — so the DROP DATABASE cannot succeed, and silently so because the return is discarded.

Not fixed here: it is 152 sites across 42 files, none of them introduced by this branch, and usetesting with context-background: true is enabled in .golangci.yaml, which is what pushes new tests toward t.Context(). Folding a repo-wide test-context sweep into a driver rename would make both harder to review. It needs a cleanup context with its own timeout, applied everywhere at once.

[morgo]

🤖 CI is fixed, and on the review: one finding is a real pre-existing bug, the other is a misread.

CI — one root cause, not ten. Every red check came from e2e/consumermodule, schemabot's second module, whose go.mod I had not regenerated after the parent's graph changed. go test -race -run '^$' ./... there refused with "updates to go.mod needed". Unit Tests actually passed five minutes of real tests before dying on that same step, and Lint was cancelled behind the Build failure rather than finding anything.

Its vitess replace was also on an older block/vitess SHA than the parent's, which the comment immediately above it forbids ("Mirror the parent module's replace directives; replaces do not propagate across module boundaries"). Both now match, block/mysql is picked up, and spirit is on the parent's pin.

DROP DATABASE IF NOT EXISTS — not there. integration/workflow_test.go:58 reads DROP DATABASE IF EXISTS. The IF NOT EXISTS is on the CREATE seven lines above, at :51. No syntax bug.

t.Context() in t.Cleanup — correct, and worse than one file. I verified it rather than reasoning about it:

ctx := t.Context()  →  ctx.Err() inside t.Cleanup = context canceled

Go cancels t.Context() just before cleanup functions run, so those DROP DATABASE calls never execute — and because they discard both returns (_, _ =), they fail silently. Real bug.

It is not confined to the two files flagged: 152 sites across 42 test files use t.Context() inside a t.Cleanup. So it is not something to fix inside a 111-file mechanical driver rename — my diff in both flagged files is only sql.Open("mysql" → sql.Open("block-mysql", and the cleanup logic predates it. Tracked separately so it gets a sweep and a lint rule rather than two spot fixes.

Still a draft: the spirit and block/vitess pins are branch pins until block/spirit#1221 and block/vitess#23 merge.

[aparajon]

🤖 Adversarial correctness review — 48558c8c (100 files, +506/−337)

This is the consumer end of the retirement wave I looked at in block/spirit#1221 and block/vitess#23, and the parts I went in expecting to be wrong are right. mysqlerr.Number is the one I'd have written this review to ask for: two drivers really are linked, their MySQLError types really are unrelated under errors.As, and routing every code read through one helper is what keeps a retry classifier from depending on which pool an error came from. TestDriverErrorTypesAreNotInterchangeable pinning the premise is the right instinct — that is the assumption that would rot silently.

There is one bug, and it is the specific hazard the wave has been circling: the DSN still names a TLS config that only one of the two linked drivers knows about, and the storage pool is opened with the other one.

#	Sev	Where	What
1	high	pkg/mysqlconn/mysqlconn.go:113	OpenReloadable cannot open a MySQL storage pool on an RDS host. ConnectionDSN injects tls=rds, which is registered in block/mysql's registry; the hot-swap driver parses with upstream, which rejects it. Startup fails
2	low	mysqlconn_test.go:195, :207	The two tests over this code are each blind on the axis the other covers, which is why finding 1 ships green

---

1 — the storage pool cannot open against RDS (high)

ConnectionDSN sends every RDS-resolved host through dbconn.EnhanceDSNWithTLS, which writes tls=rds into the DSN. tls=rds is a name, and a name only means something inside the registry of the driver package that registered it. Spirit registers it in block/mysql's registry — that is what #1221 settled, and its own doc says so: "a consumer must open them with [DriverName]. That is a requirement, not a reassurance."

Open honours that requirement. OpenReloadable cannot: the hot-swap driver embeds upstream go-sql-driver/mysql and, as this PR's own comment correctly says, "cannot be pointed at the fork."

                        ConnectionDSN(dsn)
                                │
                    injects  tls=rds   (registered in block/mysql)
                                │
              ┌─────────────────┴─────────────────┐
              ▼                                   ▼
   Open → "block-mysql"                OpenReloadable → "mysql-hotswap-dsn"
   registry HAS "rds"                  wraps upstream; registry has NO "rds"
              │                                   │
              ▼                                   ▼
      TLS to the RDS trust store          ParseDSN: invalid value /
      ServerName = host, verified         unknown config name: rds
              │                                   │
              ▼                                   ▼
        target pools OK                    STORAGE POOL FAILS TO OPEN

Measured, against the DSN ConnectionDSN actually produces:

ConnectionDSN out = u:p@tcp(sb.cluster-abc123.us-west-2.rds.amazonaws.com:3306)/schemabot
                    ?interpolateParams=true&parseTime=true&timeout=30s&tls=rds&writeTimeout=1m0s

block/mysql ParseDSN  err=<nil>                                   TLSConfig="rds"  TLS!=nil=true
upstream    ParseDSN  err=invalid value / unknown config name: rds   ← the hot-swap driver's parser
OpenReloadable        err=open reloadable MySQL connection: invalid value / unknown config name: rds

OpenReloadable has exactly one caller — openStoragePool at pkg/serve/serve.go:535, on the schema.DialectMySQL arm — and that runs during server startup, before traffic. So on a deployment with MySQL storage whose host resolves as RDS, the server does not come up. Open is unaffected, and PlanetScale and LocalScale are consistent throughout (they register into block/mysql and open with "block-mysql"), so this is isolated to the one pool that structurally cannot use the fork.

Why 41/41 is green. tlsModeForHost injects tls=rds only when dbconn.IsRDSHost(addr), and no CI job points storage at an *.rds.amazonaws.com address. The break is host-shaped, not code-shaped, so there is no compile-time or test-time canary anywhere in the suite. That is worth stating plainly in the PR, because "green" is doing no work here.

On the fix. The tempting one — drop the tls=rds injection, since block/mysql now applies the RDS trust store itself in normalize() — is only half right, and the wrong half is the dangerous one. For Open it is genuinely equivalent: the fork would supply the same roots and the same ServerName. For the hot-swap pool it is not, because upstream has no auto-TLS, so removing the injection would take the storage pool from failing loudly to connecting in the clear — the same loud-to-silent shape I flagged on block/vitess#23, on the connection that carries every credential and lease.

What does work is mirroring the config into the registry that has to resolve it. Both drivers are linked either way — that is already this PR's stated position in number.go — so one registration keeps the two paths honest. Verified:

// in mysqlconn, once at init
_ = upstreammysql.RegisterTLSConfig("rds", blockmysql.RDSTLSConfig())

upstream ParseDSN  err=<nil>
  TLS!=nil=true  ServerName="sb.cluster-abc123.us-west-2.rds.amazonaws.com"
  roots!=nil=true  skipVerify=false
OpenReloadable     err=<nil>

Verification is preserved — real roots, correct ServerName, InsecureSkipVerify=false. Whatever shape you land on, the property to pin is that the storage pool reaches an RDS host with TLS, not merely that it opens.

2 — neither test can see finding 1 (low)

The two tests over this code are individually reasonable and collectively blind, in a way worth fixing alongside the bug:

• TestOpenNormalizesRDSDSNBeforeOpening uses an RDS host, so it builds the DSN that breaks — but it stubs openSQL, so nothing ever parses the DSN for real, and its assert.Equal(t, "rds", cfg.TLSConfig) goes through mysql.ParseDSN, which this PR repoints to the fork: the one parser that accepts rds. The updated comment is right that opening under "mysql" "would silently bypass the fork rather than fail" — the test just cannot show that the converse also breaks.
• TestOpenReloadableUsesHotswapDriver exercises the hot-swap path for real, but against 127.0.0.1, so tlsModeForHost returns not-ok and no tls= is ever injected.

So the RDS-host case never meets the hot-swap driver in any test. A single case that calls OpenReloadable with an RDS host and asserts the pool opens would have failed on this branch, and is the test that keeps this from regressing the next time either driver moves.

---

Verified — the pins, the dropped parser replace, the error-code helper, and the sweep

Both repointed pins are the merged commits, on the right branches. block/spirit 10804bbe247c is "dbconn: take RDS TLS from the driver, and drop rejectReadOnly (#1221)" and is currently head of main; block/vitess 88d15fda31ea is "mysqltopo: retire the local RDS TLS wiring in favour of the driver (#23)" and is head of release-24.0, its correct base. That closes the do-not-merge branch-pin condition the wave carried.

Dropping the tidb parser replace does not regress spatial support — I checked rather than assumed, because the deleted comment made an explicit claim about it. The fork does diverge from upstream in 15 files, including parser.y, keywords.go, types/field_type.go (GeometryType and its subtypes) and types/etc.go (TypeToStr gaining a geometry parameter), so "one keyword" would have been the wrong summary. But upstream 511dba1dbe17 handles all of it. Parse, through statement.ParseCreateTable / statement.New:

geometry point linestring polygon multipoint multilinestring multipolygon
geometrycollection    all parse OK
point SRID 4326 (both bare and /*!80003 … */ forms)    parse OK
SPATIAL KEY / SPATIAL INDEX / ADD SPATIAL KEY          parse OK

And re-rendering, which is where a lost subtype would actually bite — a flattened type is a phantom diff, and Canonicalize returns its input on parse failure, so a "nothing changed" check proves nothing on its own. Printing the output instead:

ADD COLUMN `g` polygon NOT NULL             → ADD COLUMN `g` POLYGON NOT NULL
ADD COLUMN `g` point NOT NULL SRID 4326     → ADD COLUMN `g` POINT NOT NULL /*!80003 SRID 4326 */
ADD COLUMN `g` multilinestring NOT NULL     → ADD COLUMN `g` MULTILINESTRING NOT NULL
ADD COLUMN `g` geometrycollection NOT NULL  → ADD COLUMN `g` GEOMETRYCOLLECTION NOT NULL

Subtypes and SRID survive the round trip, so the replace is genuinely dead weight and removing it is a real simplification.

The driver-name sweep is complete in production code. No sql.Open with a bare "mysql" survives; 68 files import github.com/block/mysql; the only two files still importing upstream are pkg/mysqlerr/number.go and its test, which is the point of that file. No raw *MySQLError type assertion survives outside pkg/mysqlerr — all three classifier call sites (error_classifier.go:47, :51, spirit/direct.go:275) go through mysqlerr.Is. RegisterTLSConfig appears twice, in PlanetScale and a LocalScale test, and both register into block/mysql and open with "block-mysql".

Mutations. driverName → "mysql" is killed by TestOpenNormalizesRDSDSNBeforeOpening. Making Number's upstream branch return not-found is killed by TestNumberReadsBothDrivers/upstream_go-sql-driver, /wrapped, and TestIsMatchesAnyCode — so the helper's second branch is protected, not decorative.

Consumer module. e2e/consumermodule builds and tests clean against the new graph, with block/mysql correctly // indirect and the tidb replace dropped in lockstep with the parent — the module-boundary case AGENTS.md calls out for dependency bumps.

go build ./... clean, gofmt -l pkg/ empty, go vet clean on the changed packages, and ./pkg/mysqlconn/ ./pkg/mysqlerr/ ./pkg/ddl/ ./pkg/storage/internal/sqlstore/ all pass. Probes removed; worktree clean at 48558c8c. The RDS hostnames above are invented.

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

[aparajon]

🤖 Reviewed 48558c8c. The driver switch itself is clean and the sweep is complete — but please read finding 1 before merging: on this branch the MySQL storage pool cannot open against an RDS host, so the server does not start.

ConnectionDSN injects tls=rds, which lives in block/mysql's registry; OpenReloadable goes through the hot-swap driver, which wraps upstream and rejects the name. Measured: invalid value / unknown config name: rds. Its only caller is openStoragePool on the startup path. CI is green because the break is host-shaped — no job points storage at an RDS address, so nothing in the suite can see it. Note that dropping the tls=rds injection is the wrong fix: it would take the storage pool from failing loudly to connecting in the clear. Mirroring the config into upstream's registry works and keeps verification intact (verified).

mysqlerr.Number is the right answer to the two-linked-drivers problem, and pinning the premise with TestDriverErrorTypesAreNotInterchangeable is the part I'd have asked for. I also checked the dropped tidb parser replace rather than trusting the deleted comment — upstream parses and re-renders every geometry subtype and SRID, so it is genuinely dead weight. Both repointed pins are the merged commits on their correct branches.

Full detail: my review comment.

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

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1320, 48558c8.

Verdict: 8 findings — 1 blocking (RDS storage pool cannot open), 6 non-blocking, 1 suggestion. The Go sweep itself is clean: zero sql.Open("mysql", …) remains in any .go file and the dual-driver error classification is correctly built. The blocking issue is a registry split the rename creates between the fork and the hot-swap driver.

Blocking

1. SchemaBot cannot open its storage pool against any RDS/Aurora MySQL after this PR. ConnectionDSN routes RDS hosts through spirit's EnhanceDSNWithTLS, which now registers the rds TLS config into block/mysql's package-global registry and stamps tls=rds into the DSN. mysqlconn.go:121 then hands that DSN to mysql-hotswap-dsn, which is pinned to upstream go-sql-driver v1.10.0 and whose OpenConnector calls upstream ParseDSN — returning invalid value / unknown config name: rds eagerly from sql.Open (the DriverContext branch), before any dial. So serve.go:535 fails at startup. This worked at the merge base, where spirit and schemabot both used upstream and the registration landed in the driver that reads it; the credential-reload callback is broken the same way, since it re-parses the returned tls=rds DSN with upstream. CI misses it because the only test that opens the reloadable path uses a non-RDS host with a stubbed openSQL, while the test asserting tls=rds never opens a pool.

Non-blocking

2. AGENTS.md now prescribes the driver this PR removed, and nothing gates it. AGENTS.md:321 sanctions raw sql.Open("mysql", …) for "LocalScale" and "PlanetScale/Vitess mTLS" — precisely the two paths this PR migrated, so the rule is contradicted by every call site it names; :325 still points ParseDSN/FormatDSN at go-sql-driver and :143 defines "the Go MySQL driver" as upstream. The PR touched zero markdown files. .golangci.yaml has no depguard or forbidigo, so a future sql.Open("mysql", …) compiles, runs, and passes CI — the durable fix is the lint rule, not the prose.

3. connectionLost breaks the dual-driver premise Number() establishes fifteen lines above it. mysqlerr.go:173 compares against block/mysql's ErrInvalidConn only, but Reason() opens by stating "a target error can arrive from either linked driver" and routes code lookups through the dual-handling Number(). ErrInvalidConn is a plain errors.New, so errors.Is is pointer identity and upstream's sentinel returns false. Latent today (spirit is fully on the fork), but either the premise is true and this is a hole, or it is false and Number()'s second arm is dead weight.

4. A persistently read-only target now reports "connection lost" instead of its error code. block/mysql converts errno 1290/1792/1836 to ErrBadConn unconditionally — upstream gated this on rejectReadOnly, which defaulted off. On an Aurora reader endpoint or a cluster with super_read_only stuck on, database/sql burns its retries and hands back a bare driver.ErrBadConn; Number() misses, connectionLost matches, and the operator's PR comment says the connection was lost rather than naming error 1290.

5. The RDS root-CA guard is now dead code. postgresconn.go:413 checks pool == nil, but RDSTLSConfig().RootCAs is always non-nil and block/mysql deliberately discards AppendCertsFromPEM's return ("the result is an empty pool"). Replacing the source with an empty x509.NewCertPool() leaves the whole suite green — so the deleted if !pool.AppendCertsFromPEM(…) invariant is not re-established anywhere in schemabot.

6. The comment that justified the sweep is scoped wrong, and it is why the doc surface went unchecked. pkg/testutil/mysql.go:17 says "Nothing here registers mysql any more, so the old literal failed the wait strategy with unknown driver". Registration is process-global, and upstream is linked into every server binary via mysqlconn → hotswap-dsn-driver — so outside that one test binary a stale literal opens a working upstream pool silently. That is the exact inference that let the loud misses be found and the silent ones be missed.

7. Undocumented DSN break: rejectReadOnly no longer round-trips. ConnectionDSN reformats every DSN through block/mysql, which hard-errors on rejectReadOnly=false and has no Config.RejectReadOnly field at all — so rejectReadOnly=true parses but is silently dropped from the reassembled DSN, and for OpenReloadable that DSN then goes to the upstream-backed hot-swap driver where the option defaults off. No DSN in the repo sets it today, so this is latent.

General suggestions

8. Four unpinned driver-name surfaces. The "block-mysql" literal is hardcoded at ~17 production sql.Open sites; reverting two of them to "mysql" leaves the full suite green, and an integration run would not catch it either since both drivers connect identically. TestOpenReloadableUsesHotswapDriver asserts hotswapDriverName against itself, so that name is entirely unpinned — mutating it to "block-mysql" stays green, though at runtime the credential-reloading pool would never open. error_classifier_test.go constructs only fork errors, so deleting Number()'s upstream arm leaves pkg/storage green even though its pool is the one producing upstream errors. And pkg/localscale/README.md:169 still teaches a copy-paste template using sql.Open("mysql", dsn).

The one thing that could have broken, verified

Error-type split-brain on the storage retry path — the mechanism most likely to silently break, and it is sound. error_classifier.go routes both IsRetryableConflict and IsDuplicateKey through mysqlerr.Is rather than asserting either concrete type, and number.go uses errors.As against both *blockmysql.MySQLError and *upstreammysql.MySQLError, so wrapped errors unwrap and deadlock/lock-wait retries survive regardless of which pool produced them. Every mutant against it died: breaking either arm, replacing errors.As with a bare assertion, dropping the lock-wait code, disabling the typed duplicate-key arm, and negating slices.Contains.

Verified correct

• Zero sql.Open("mysql", …) remains in any .go file; every residual "mysql" string is the engine-type discriminator or a CLI flag, not a driver name.
• All three fork pins are real merged revisions: block/spirit and block/mysql compare identical to their default branches; the vitess pin is the head of release-24.0, matching the required v0.24.1.
• Dropping the tidb parser replace is safe — spirit v0.17 carries its own in-tree pkg/parser, which retains the SPATIAL table-element rule upstream lacks.
• e2e/consumermodule/go.mod mirrors the root module on every driver-relevant line and drops the same replace in lockstep.
• pkg/mysqlconn is a correct centralization: both driver names are constants and mysqlconn_test.go:201 asserts the "block-mysql" literal, so drift on that one name fails a unit test.
• No invariant-registry breakage — no *Enforced:* line names a symbol this PR moved, and every driver occurrence in docs/invariants.md is the lease-holding-worker sense the terminology rule mandates.

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

[morgo]

🤖 Addressed in 012bed8b. Finding 1 was real and the fix went further than either option you named: upstream go-sql-driver is no longer linked by any SchemaBot package, so there is one TLS registry rather than two to keep in sync.

Your diagnosis was exactly right, including that the tempting fix is the dangerous one. Mirroring rds into upstream's registry works, and I did not take it for the reason your own diagram implies — the sync between two registries is the thing that just broke, so adding a second registration keeps the failure mode alive and only moves the day it fires. Dropping the injection is worse, for the loud-to-silent reason you gave.

So go-mysql/hotswap-dsn-driver is gone, reimplemented as pkg/connreload. Its job was small — re-read credentials when a dial is refused — and everything driver-specific reduces to two functions: Resolve(dsn) (driver.Connector, error) and Refused(error) bool. MySQL supplies 60 lines, PostgreSQL 35.

The reimplementation is not a port, and postgresconn is where that shows: it already had this machinery, so the extraction deletes a duplicate rather than adding a layer. Three things the replaced driver got wrong, each now pinned:

• the reload callback lived in a package-level variable, so opening a second reloadable pool silently repointed the first pool's reload at the second pool's secret
• no cooldown — a secrets-backend outage cost one resolve per refused dial
• the electing dial was pinned for the duration of the reload, holding a pool connection slot while a hung secret resolution ran

Finding 2 is the test I added, and your framing of the property to pin — with TLS, not merely opens — is what made it non-vacuous. My first version was not: I asserted the resolved trust and it survived a mutation that dropped the tls=rds injection entirely. block/mysql applies RDS TLS on its own for an RDS address, so TLS != nil holds either way and the resolved-trust assertions cannot tell "the name resolved" from "the driver supplied TLS anyway". TestReloadablePoolReachesRDSHostWithVerifiedTLS now asserts both halves and kills both mutations — dropping the injection, and weakening the mode to PREFERRED.

Two things worth your eye on the re-review:

1. I added depguard to .golangci.yaml denying both go-sql-driver/mysql and the hot-swap driver. Nothing fails to compile if a second driver returns and the breakage is silent, so a comment felt insufficient — but it is a new linter in the config, and it is yours to veto.
2. go-sql-driver/mysql stays in go.mod as // indirect, because testcontainers-go/modules/mysql test-imports it. No SchemaBot package links it (go list -deps -test ./...), which is the property that matters, but the go.mod line will look like the retirement is incomplete.

I also took your "green is doing no work here" point into the PR description verbatim in substance — the host-shaped-not-code-shaped distinction is worth a reader seeing.

On the Copilot findings: t.Context() inside t.Cleanup is real (verified: ctx.Err() is already context canceled there, because t.Context() is cancelled just before cleanups run), but it is 152 sites across 42 files and predates this branch — usetesting with context-background: true is what steers people to it. Tracked separately rather than folded into a driver rename. Copilot's integration/workflow_test.go:49 claim of DROP DATABASE IF NOT EXISTS is a misread: line 58 is DROP DATABASE IF EXISTS, and the IF NOT EXISTS is on the CREATE two lines up.

[aparajon]

🤖 Re-review — 48558c8c → 012bed8b (17 files, +1505/−899)

The high finding is fixed, and fixed at a level I didn't propose. I suggested mirroring tls=rds into upstream's registry so both linked drivers could resolve the name; you removed the second driver instead. OpenReloadable now builds its pool with sql.OpenDB(connector) over an in-repo pkg/connreload connector, so there is no driver-name registry lookup on that path at all — the DSN is parsed by mysql.ParseDSN inside resolveConnector, which is the fork's parser, which is the registry that has rds in it. The class of bug is gone rather than patched: the failure needed two linked MySQL drivers, and there is now one.

Measured against the DSN ConnectionDSN actually produces, on the RDS host that broke before:

ConnectionDSN out = u:p@tcp(sb.cluster-abc123.us-west-2.rds.amazonaws.com:3306)/schemabot
                    ?interpolateParams=true&timeout=30s&tls=rds&writeTimeout=1m0s
block/mysql ParseDSN  ok   TLSConfig="rds"  TLS!=nil=true
OpenReloadable        err=<nil>              ← was: unknown config name: rds
pool driver           mysql.MySQLDriver      ← was: mysql-hotswap-dsn

And the two things I asked for beyond the fix are both here. The depguard rule is the compile-time canary I said the repo had no version of, and its desc carries the actual reasoning rather than a bare ban — I checked it fires rather than assuming:

pkg/mysqlconn/zz_canary.go:4  import 'github.com/go-mysql/hotswap-dsn-driver/hotswap' is not allowed ...
pkg/mysqlconn/zz_canary.go:5  import 'github.com/go-sql-driver/mysql' is not allowed ... (depguard)
2 issues

It flags test files too, so a test reaching for upstream is caught on the same footing as production code. TestReloadablePoolReachesRDSHostWithVerifiedTLS is the test for the property I named — and it pins the mechanism as well as the outcome, with a comment explaining exactly why both halves are needed: the fork applies RDS TLS on its own, so the trust assertions pass either way and cannot distinguish "the injected name resolved" from "the driver quietly did it instead." That is the trap I warned about, closed deliberately.

Two low findings, both about the new scheduler rather than the fix.

#	Sev	Where	What
1	low	pkg/connreload/connreload.go:217	refresh's loop has no bound; termination is emergent from three separate invariants, and breaking any one of them spins inside Connect rather than failing
2	low	pkg/connreload/connreload.go:51, :145	A rotation whose secret sync lags costs up to a full cooldown of failed dials, because "reload succeeded but was refused" arms the same 30s window as "reload failed"

---

1 — the retry loop's termination is emergent, and the failure shape is a spin (low)

refresh is a for {} whose exits are: the generation advanced, or the cooldown is armed, or the dial context ended. The current code is correct — runReload's defer always either advances gen or sets lastReloadFail, so one of the first two always holds after <-done. My concern is that the guarantee lives in a different function from the loop, and the cost of losing it is not a wrong answer.

I found this by mutation rather than by reading, and the pattern is what makes it worth a line. Of the four mutations I made to the invariants this loop depends on, three produce an infinite loop, and the suite catches them only as a test timeout:

refresh drops the stale-generation short-circuit   -> hang (killed at 600s)
runReload never advances the generation            -> hang (killed at 60s)
cooldown window comparison inverted                -> hang (killed at 60s)
armCooldown drops the stale-generation guard       -> FAIL TestStaleRefusalDoesNotArmCooldown

The mutations aren't the point; where they land is. This loop runs inside database/sql's Connect, and this package's own doc explains why that matters — "database/sql counts a dial against the pool's connection budget before Connect runs, so a pinned dial would hold a pool slot for as long as the reload hangs." A spin there is worse than the hang it replaces: it holds the slot and burns a core and generates resolve traffic, on the pool that carries every lease.

Since Connect only ever wants one reload and one retry, the loop can say so directly — bound it to two passes, or track "I already waited for a reload" and surface the dial error on the second pass. Then termination is local to the loop and no future edit to the generation or cooldown logic can turn a dial into a spin. The three hanging mutations would become clean failures, which is also a better signal for whoever makes that edit.

2 — a lagging secret sync costs a full cooldown of failed dials (low)

DefaultCooldown is armed by two different conditions that the doc treats as one:

• Reload failed — the secrets backend is down or erroring. Retrying sooner than 30s is pure amplification; the window is exactly right.
• Reload succeeded, but the dial with the reloaded credentials was also refused (Connect, line 145). The doc's examples for this are steady-state faults — "dropped grant, a user the rotation renamed" — where 30s is also right.

But the same branch catches the transient case: the rotation has happened on the server and the secret store has not caught up yet. Then the reload legitimately returns the old password, the retry is legitimately refused, and the window is armed. If the store syncs two seconds later, new dials keep failing with 1045 for the remaining ~28s without attempting a reload, because the cooldown check at line 225 runs before anything else. Established connections are unaffected, so this is degraded rather than down — but it is a self-inflicted availability window on the storage pool, triggered by the exact event the package exists to make transparent.

The two conditions have different expected recovery times, so they could reasonably have different windows: a short first arm for "credentials refused after a successful reload" (a sync lag clears in seconds) and the full DefaultCooldown for "reload failed" and for repeats. Escalating on consecutive refusals would get both properties. If you'd rather keep one window, the tradeoff is worth stating in DefaultCooldown's doc, since it currently reads as being only about backend outages and this case is the one an operator will actually hit during a rotation.

---

Verified — the extraction, the coverage that moved, the driver graph, and the mutation ledger

The pkg/connreload extraction is symmetric and complete. The seven reloadableConnector methods left pkg/postgresconn and both engines now supply the same three things — Resolve, Refused, Reload — plus a driver instance and a pool name. Nothing engine-specific leaked into the shared package, and the MySQL side's contribution really is just resolveConnector and isAccessDenied. erAccessDenied = 1045 with 1698 deliberately excluded is the right call and the reasoning on it is correct: an account authenticating by something other than the password sent is a grant shape no rotation of the secret changes, so reloading could only re-resolve the same credential.

No test coverage was lost, which a −621-line test diff does not make obvious. All 16 TestReloadableConnector* cases left postgresconn_test.go, and every one has a counterpart in connreload_test.go — cooldown, hung reload not blocking Connect or snapshot, non-auth errors ignored, credentials kept when reload fails, concurrent refresh, dedupe, electing-dial and waiter context handling, waiter observing leader failure, panic unblocking waiters, stale rejection not arming the cooldown, retry failure surfaced. Seven cases are new: TestNewRequiresCallbacks, TestNewRejectsUnresolvableDSN, TestRejectsUnresolvableReloadedDSN, TestConnectSurfacesResolveErrorShape, TestCooldownOverride, TestDriverIsReported, TestPoolsDoNotShareReloadState, TestUnnamedPoolLogsWithoutPanicking. The engine-specific residue that stayed behind is the right residue — TestIsAccessDenied, DSN normalization, and the RDS TLS test.

The driver graph is what the change claims. go-sql-driver/mysql and hotswap-dsn-driver are both out of the direct requires. Upstream survives as // indirect through exactly one path — testcontainers-go/modules/mysql, reached only from pkg/api.test — and go list -deps ./cmd/... links it zero times, so the server binary has one MySQL driver in it. That is the property mysqlerr.Number and the depguard rule together now keep true, and it is why dropping Number's upstream branch is safe rather than a re-introduction of the silent miss. e2e/consumermodule was tidied in lockstep — both drivers gone from its indirect set — and builds and tests clean.

On the history in those comments. mysqlerr.Number, the classifier, and the depguard desc all explain the two-types failure by referring to the arrangement that produced it. That reads as rationale, not changelog, and I think it is the right call rather than a brush with the no-bug-references rule: a helper that exists to prevent a failure mode nobody can see in the current code otherwise looks like superstition, and the next person to "simplify" it needs the reason. Worth saying out loud so a future docs pass doesn't strip it.

Attack that dissolved — parseTime vanishing from the DSN. My round-1 measurement showed parseTime=true in ConnectionDSN's output and this round's did not, which would have broken every time.Time scan in the storage layer. It was my probe's input, not the code: parseTime is caller-supplied and preserved, which mysqlconn_test.go's wantParseTime cases pin and which I confirmed directly —

in  = ...rds.amazonaws.com:3306)/schemabot?parseTime=true
out = ...?interpolateParams=true&parseTime=true&timeout=30s&tls=rds&writeTimeout=1m0s

Mutation ledger — 7 run, 7 killed (3 of them only by hang; see finding 1). Killed cleanly: dropping the RDS TLS injection (5 tests, including the new RDS TLS test — so the mechanism assertion is load-bearing); erAccessDenied 1045 → 1698; armCooldown's stale-generation guard; Connect's armCooldown after a refused retry.

Concurrency. refresh releases the connector mutex before waiting on the reload, runReload publishes its outcome under it, and close(done) happens while the mutex is still held by the deferred unlock — so a woken waiter cannot observe a half-published swap. No lock is held across Reload or Resolve. sql.OpenDB over a connector also means no global driver registration, so two pools cannot share reload state, which TestPoolsDoNotShareReloadState pins.

Local checks. go build ./..., go vet and gofmt -l pkg/ clean; ./pkg/connreload/ ./pkg/mysqlconn/ ./pkg/mysqlerr/ ./pkg/postgresconn/ ./pkg/storage/internal/sqlstore/ all pass; consumer module clean. statement_timeout and the Postgres runtime params are untouched by this delta, so the extraction is behaviour-preserving on the PG TLS/budget axis. CI is green except two E2E K8s legs still pending at review time. Leak-checked: no internal identifiers, and every RDS hostname in the diff is an invented placeholder. Probes and the depguard canary removed; worktree clean at 012bed8b.

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

[aparajon]

🤖 Approving at 012bed8b. The high finding from round 1 is fixed at the root rather than patched — dropping the second MySQL driver and building the reloadable pool with sql.OpenDB over an in-repo connector means there is no registry lookup to get wrong, so the failure class is gone rather than worked around. Measured: OpenReloadable now opens against an RDS host with verified TLS, through mysql.MySQLDriver.

Both follow-ups I asked for are here and I checked each one bites rather than assuming: the depguard rule fires on both banned imports (test files included), and TestReloadablePoolReachesRDSHostWithVerifiedTLS pins the mechanism as well as the trust chain, which is what keeps a later switch to the driver's own auto-TLS a deliberate decision. No coverage was lost in the −621-line test diff — all 16 reloadable cases moved to pkg/connreload and gained seven more.

Two low findings on the new scheduler, neither blocking: refresh's loop is unbounded and its termination depends on invariants maintained elsewhere (three of four mutations there spin rather than fail, inside Connect, which holds a pool slot), and the cooldown treats a lagging secret sync the same as a secrets-backend outage, costing up to 30s of failed dials during the rotation it exists to make transparent.

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