From 8941f7c54733b118f2efe55dd6f92c16fc1e9176 Mon Sep 17 00:00:00 2001 From: Jens Hausherr Date: Thu, 23 Jul 2026 14:55:43 +0200 Subject: [PATCH 1/4] fix(mysql): add lock_wait_timeout to DSN to stop statement pile-up MySQL/Percona could become unresponsive under a growing backlog of pending ALTER USER (and other) statements issued by provider-sql. Root cause: go-sql-driver/mysql cancels a context by closing the TCP socket without sending KILL QUERY (intentional, all versions incl. the latest v1.10.0). With MySQL's default lock_wait_timeout of 1 year, a statement blocked on a metadata/ACL lock (e.g. behind a backup or long transaction) stays pending server-side even after the 60s reconcile deadline makes the provider give up. Every reconcile retry issues another blocked statement; account-management statements additionally serialize on a single global ACL lock, so they pile up until max_connections is exhausted or ACL contention wedges the server. Append lock_wait_timeout=30 and a dial timeout=10s to the MySQL DSN so blocked statements fail fast server-side and release instead of accumulating. This covers every MySQL controller (User, Grant, Database; cluster and namespaced) since they share this client. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Jens Hausherr --- pkg/clients/mysql/mysql.go | 45 ++++++++++++++++++++++++++------- pkg/clients/mysql/mysql_test.go | 33 +++++++++++++++++++++--- 2 files changed, 66 insertions(+), 12 deletions(-) diff --git a/pkg/clients/mysql/mysql.go b/pkg/clients/mysql/mysql.go index 4ab55902..029020ac 100644 --- a/pkg/clients/mysql/mysql.go +++ b/pkg/clients/mysql/mysql.go @@ -16,6 +16,27 @@ import ( const ( errNotSupported = "%s not supported by mysql client" + + // lockWaitTimeoutSeconds bounds how long a statement waits for a metadata + // lock before failing server-side. MySQL's default lock_wait_timeout is + // 31536000s (1 year), so a statement queued behind a lock — e.g. an + // ALTER USER / GRANT / DROP DATABASE waiting behind a backup, a long + // transaction, or concurrent DDL — stays pending on the server + // effectively forever. This is dangerous here because the go-sql-driver + // cancels a context by closing the TCP socket WITHOUT issuing KILL, so + // when the reconcile deadline fires the provider abandons the statement + // but the server thread (and its connection) keeps waiting. Every retry + // then issues another statement that also blocks, and account-management + // statements additionally serialize on a single global ACL lock, so + // blocked statements pile up until the server becomes unresponsive. A + // short lock_wait_timeout makes such statements fail fast and release + // instead of accumulating. + // + // See docs/mysql-driver-context-cancellation.md for the full analysis. + lockWaitTimeoutSeconds = 30 + // dialTimeout bounds TCP connection establishment so a reconcile does not + // block on an unreachable endpoint. + dialTimeout = "10s" ) type mySQLDB struct { @@ -50,21 +71,27 @@ func DSN(username, password, endpoint, port, tls string, binlog *bool) string { // Use net/url UserPassword to encode the username and password // This will ensure that any special characters in the username or password // are percent-encoded for use in the user info portion of the DSN URL + + // lock_wait_timeout and timeout are appended to every connection so that a + // statement blocked on a metadata lock, or a connection to an unreachable + // endpoint, fails fast server-side instead of piling up (see the const + // docs above). lock_wait_timeout is an unrecognised driver param and is + // therefore issued as `SET lock_wait_timeout = ` by go-sql-driver on + // connect; timeout is the driver's dial timeout. + params := fmt.Sprintf("tls=%s&lock_wait_timeout=%d&timeout=%s", + tls, + lockWaitTimeoutSeconds, + dialTimeout, + ) if binlog != nil { - return fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s&sql_log_bin=%s", - username, - password, - endpoint, - port, - tls, - strconv.FormatBool(*binlog)) + params += fmt.Sprintf("&sql_log_bin=%s", strconv.FormatBool(*binlog)) } - return fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s", + return fmt.Sprintf("%s:%s@tcp(%s:%s)/?%s", username, password, endpoint, port, - tls) + params) } // ExecTx is unsupported in MySQL. diff --git a/pkg/clients/mysql/mysql_test.go b/pkg/clients/mysql/mysql_test.go index 13aacdc6..d79c24fc 100644 --- a/pkg/clients/mysql/mysql_test.go +++ b/pkg/clients/mysql/mysql_test.go @@ -4,6 +4,8 @@ import ( "fmt" "strconv" "testing" + + mysqldriver "github.com/go-sql-driver/mysql" ) func TestDSNURLEscaping(t *testing.T) { @@ -14,12 +16,14 @@ func TestDSNURLEscaping(t *testing.T) { tls := "true" binlog := false dsn := DSN(user, rawPass, endpoint, port, tls, &binlog) - if dsn != fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s&sql_log_bin=%s", + if dsn != fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s&lock_wait_timeout=%d&timeout=%s&sql_log_bin=%s", user, rawPass, endpoint, port, tls, + lockWaitTimeoutSeconds, + dialTimeout, strconv.FormatBool(binlog)) { t.Errorf("DSN string did not match expected output with URL encoded and binlog") } @@ -32,12 +36,35 @@ func TestDSNURLEscapingWithoutBinLog(t *testing.T) { rawPass := "password^" tls := "true" dsn := DSN(user, rawPass, endpoint, port, tls, nil) - if dsn != fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s", + if dsn != fmt.Sprintf("%s:%s@tcp(%s:%s)/?tls=%s&lock_wait_timeout=%d&timeout=%s", user, rawPass, endpoint, port, - tls) { + tls, + lockWaitTimeoutSeconds, + dialTimeout) { t.Errorf("DSN string did not match expected output with URL encoded") } } + +// TestDSNParsesWithLockWaitTimeout guards against a typo in the DSN param +// names: it verifies the go-sql-driver actually parses the DSN we build and +// that lock_wait_timeout is carried as a session variable (issued as +// `SET lock_wait_timeout = ` on connect) and timeout as the dial timeout. +func TestDSNParsesWithLockWaitTimeout(t *testing.T) { + dsn := DSN("username", "password", "endpoint", "3306", "preferred", nil) + + cfg, err := mysqldriver.ParseDSN(dsn) + if err != nil { + t.Fatalf("go-sql-driver could not parse the DSN we build: %v", err) + } + + want := strconv.Itoa(lockWaitTimeoutSeconds) + if got := cfg.Params["lock_wait_timeout"]; got != want { + t.Errorf("lock_wait_timeout: got %q, want %q", got, want) + } + if cfg.Timeout == 0 { + t.Errorf("dial timeout was not parsed from the DSN") + } +} From 63dbc477c128cf0ef691c2419a9db7483a0c17cf Mon Sep 17 00:00:00 2001 From: Jens Hausherr Date: Thu, 23 Jul 2026 14:56:08 +0200 Subject: [PATCH 2/4] docs: document the mysql driver's context-cancellation behavior Records why go-sql-driver/mysql cancelling a context by closing the socket (rather than issuing KILL QUERY) matters operationally, the full incident chain it can cause, the lock_wait_timeout mitigation, what it does not cover, and follow-up ideas. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Jens Hausherr --- docs/mysql-driver-context-cancellation.md | 144 ++++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 docs/mysql-driver-context-cancellation.md diff --git a/docs/mysql-driver-context-cancellation.md b/docs/mysql-driver-context-cancellation.md new file mode 100644 index 00000000..cb441f36 --- /dev/null +++ b/docs/mysql-driver-context-cancellation.md @@ -0,0 +1,144 @@ +# MySQL driver context cancellation does not stop server-side statements + +This document records a behavior of `github.com/go-sql-driver/mysql` that has +operational consequences for provider-sql, why it caused a production incident +(MySQL/Percona becoming unresponsive with many `ALTER USER` statements pending), +and how the provider mitigates it. + +## TL;DR + +- When a Go `context` is cancelled (including on deadline), the MySQL driver + **closes the TCP socket but does not send `KILL QUERY`**. The statement keeps + running / waiting on the server. +- This is **intentional, longstanding driver behavior** and is present in every + release up to and including the current latest, `v1.10.0`. **Upgrading the + driver does not fix it** — it is not a bug to be patched away. +- Combined with MySQL's default `lock_wait_timeout` of `31536000` seconds + (1 year), a statement blocked on a metadata / ACL lock stays pending forever + even after the provider gives up, and every reconcile retry adds another one. + They accumulate until the server runs out of connections or its ACL subsystem + is wedged. +- Mitigation: provider-sql now appends `lock_wait_timeout` (and a dial + `timeout`) to the MySQL DSN so blocked statements **fail fast server-side and + release**, instead of piling up. See `pkg/clients/mysql/mysql.go`. + +## The driver behavior (verified in v1.10.0) + +On query/exec the driver arms a watcher goroutine +(`connection.go` `startWatcher`). When `ctx.Done()` fires it calls +`mc.cancel(err)` → `mc.cleanup()`, and `cleanup()` simply does +`close(mc.closech)` and `conn.Close()` on the raw network connection. There is +no code path that opens a side connection and issues `KILL QUERY `. + +From Go's point of view the query is cancelled and the call returns +`context.DeadlineExceeded`. From MySQL's point of view the session's statement +continues: for a statement blocked in `Waiting for metadata lock`, the server +thread does **not** poll socket liveness during the lock wait, so it keeps +waiting until it acquires the lock (then notices the dead client) or is killed +out of band. + +The driver maintainers' recommended way to actually cancel server-side is the +application's job: check out a dedicated connection, read its +`CONNECTION_ID()`, and issue `KILL QUERY ` from a **separate** connection +on cancel. provider-sql does not do this (see "What this does not cover"). + +References: +- — query keeps running after context cancel +- — context cancellation semantics +- — DeadlineExceeded closes the network connection +- — the app-level `KILL QUERY` pattern + +## Why it caused an incident here + +1. **Every write goes through one path.** All MySQL controllers issue writes via + `mysql.ExecWrapper → c.db.Exec`, which opens a fresh connection per call + (`pkg/clients/mysql/mysql.go`). This covers, in the User controller, + `CREATE USER`, `ALTER USER ... WITH `, + `ALTER USER ... IDENTIFIED WITH `, `ALTER USER ... IDENTIFIED BY`, + `DROP USER`; in Grant, `GRANT` / `REVOKE`; in Database, + `CREATE/ALTER/DROP DATABASE`. Password rotation is only the *most frequent* + trigger — any of these statements can block. + +2. **The reconcile deadline is 60s.** crossplane-runtime applies a default + `reconcileTimeout` of 1 minute (provider-sql does not override it via + `managed.WithTimeout`). So the provider goroutine is *not* hung forever — it + is cancelled at ~60s. + +3. **But cancellation only closes the socket (see above).** The abandoned + `ALTER USER` / `GRANT` keeps waiting on the server because of the 1-year + default `lock_wait_timeout`. + +4. **Retries re-issue the statement.** The 60s cancellation surfaces as an + error, crossplane requeues with backoff, and the next reconcile opens a new + connection and issues the statement again — which also blocks. + +5. **The ACL global lock amplifies it.** MySQL account-management statements + (`CREATE/ALTER/DROP USER`, `GRANT`, `REVOKE`) serialize on a single global + ACL lock. One stuck statement stalls *every* account-management statement + server-wide, not just ones touching the same object. + +6. **A feedback loop keeps it going.** Controllers persist observed state only + *after* a successful statement. While the statement keeps failing, observed + state never updates, so the next reconcile re-diffs and re-issues the same + blocked statement every poll cycle. + +Net effect: blocked statements and their connections accumulate until +`max_connections` is exhausted and/or ACL contention wedges account management +— the database appears unresponsive. + +A common real-world trigger for the initial block is a backup +(e.g. Percona XtraBackup / `FLUSH TABLES WITH READ LOCK`) or a long-running +transaction holding a lock that account-management DDL must wait behind. + +## Mitigation implemented + +`DSN()` in `pkg/clients/mysql/mysql.go` now appends: + +- `lock_wait_timeout=30` — an unrecognised driver parameter, so go-sql-driver + issues `SET lock_wait_timeout = 30` on connect. A statement that cannot get + its metadata / ACL lock within 30s **fails server-side and releases** instead + of waiting up to a year. The provider then retries on the normal reconcile + backoff once the lock clears. +- `timeout=10s` — the driver's dial timeout, so a reconcile does not block on + an unreachable endpoint. + +This directly breaks the pile-up chain for the lock-wait case that caused the +incident, and it applies to every MySQL controller (User, Grant, Database, both +cluster- and namespaced-scoped) because they all share this client. + +## What this does *not* cover + +`lock_wait_timeout` only bounds waits for metadata / ACL locks. A statement +hung for a *non-lock* reason — a dead network path, or a server wedged +mid-execution — is still only socket-closed-without-`KILL` at the 60s reconcile +deadline, and can continue running server-side. The only remedy for that class +is the application-level `KILL QUERY` pattern described above. + +## Recommended follow-ups (not in this change) + +- **Application-level `KILL QUERY` on cancel** for the non-lock hang case + (dedicated connection + `CONNECTION_ID()` + out-of-band `KILL`). +- **Bounded shared connection pool.** The client currently calls `sql.Open` per + query and closes it immediately (no reuse, no `SetMaxOpenConns` / + `SetConnMaxLifetime`). A shared, capped `*sql.DB` would hard-limit total + connections and remove per-query connect/auth overhead, but requires adding a + `Close()` to the `xsql.DB` interface and wiring it through `Disconnect` for + all three drivers (MySQL, PostgreSQL, MSSQL). +- **Harden the reconcile feedback loop** so a persistently failing statement + does not re-fire every poll cycle. +- **Make `lock_wait_timeout` configurable** (e.g. via `ProviderConfig`) for + environments that legitimately need longer or shorter waits. + +## Reproducing / verifying + +1. Create a MySQL `User` with a `PasswordSecretRef`. +2. In a separate MySQL session, hold a lock the DDL must wait behind + (e.g. `LOCK TABLES mysql.user WRITE`, or an open transaction). +3. Rotate the password so the provider issues `ALTER USER ... IDENTIFIED BY`. + +- **Before the fix:** `SHOW PROCESSLIST` accumulates + `ALTER USER ... | Waiting for metadata lock` threads and the connection count + climbs with every reconcile. +- **After the fix:** the `ALTER USER` fails with a lock-wait-timeout error + within ~30s and is retried on backoff; `SHOW PROCESSLIST` stays clean and the + connection count stays flat. From b067c97019e7882993bcb70408edbbc278c11e10 Mon Sep 17 00:00:00 2001 From: Jens Hausherr Date: Wed, 22 Jul 2026 10:19:00 +0200 Subject: [PATCH 3/4] Reduce comments to be more concise Signed-off-by: Jens Hausherr --- pkg/clients/mysql/mysql.go | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/pkg/clients/mysql/mysql.go b/pkg/clients/mysql/mysql.go index 029020ac..8d90e078 100644 --- a/pkg/clients/mysql/mysql.go +++ b/pkg/clients/mysql/mysql.go @@ -17,21 +17,7 @@ import ( const ( errNotSupported = "%s not supported by mysql client" - // lockWaitTimeoutSeconds bounds how long a statement waits for a metadata - // lock before failing server-side. MySQL's default lock_wait_timeout is - // 31536000s (1 year), so a statement queued behind a lock — e.g. an - // ALTER USER / GRANT / DROP DATABASE waiting behind a backup, a long - // transaction, or concurrent DDL — stays pending on the server - // effectively forever. This is dangerous here because the go-sql-driver - // cancels a context by closing the TCP socket WITHOUT issuing KILL, so - // when the reconcile deadline fires the provider abandons the statement - // but the server thread (and its connection) keeps waiting. Every retry - // then issues another statement that also blocks, and account-management - // statements additionally serialize on a single global ACL lock, so - // blocked statements pile up until the server becomes unresponsive. A - // short lock_wait_timeout makes such statements fail fast and release - // instead of accumulating. - // + // prevent statements hanging indefintely server-side if timeout occurs waiting for a lock // See docs/mysql-driver-context-cancellation.md for the full analysis. lockWaitTimeoutSeconds = 30 // dialTimeout bounds TCP connection establishment so a reconcile does not @@ -68,16 +54,7 @@ func New(creds map[string][]byte, tls *string, binlog *bool) xsql.DB { // DSN returns the DSN URL func DSN(username, password, endpoint, port, tls string, binlog *bool) string { - // Use net/url UserPassword to encode the username and password - // This will ensure that any special characters in the username or password - // are percent-encoded for use in the user info portion of the DSN URL - - // lock_wait_timeout and timeout are appended to every connection so that a - // statement blocked on a metadata lock, or a connection to an unreachable - // endpoint, fails fast server-side instead of piling up (see the const - // docs above). lock_wait_timeout is an unrecognised driver param and is - // therefore issued as `SET lock_wait_timeout = ` by go-sql-driver on - // connect; timeout is the driver's dial timeout. + // add timeouts to prevent orphaned statements and excessive waits on connection dial params := fmt.Sprintf("tls=%s&lock_wait_timeout=%d&timeout=%s", tls, lockWaitTimeoutSeconds, From bb48a74902c362a532151cd40b63fac6c10b8ddf Mon Sep 17 00:00:00 2001 From: Jens Hausherr Date: Tue, 18 Aug 2026 14:42:33 +0200 Subject: [PATCH 4/4] docs: mark connection-pool follow-up as addressed, cross-link #434 The "bounded shared connection pool" follow-up listed here landed as its own change; point at docs/connection-pool.md instead of leaving it as an open TODO. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Jens Hausherr --- docs/mysql-driver-context-cancellation.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/mysql-driver-context-cancellation.md b/docs/mysql-driver-context-cancellation.md index cb441f36..b45034df 100644 --- a/docs/mysql-driver-context-cancellation.md +++ b/docs/mysql-driver-context-cancellation.md @@ -51,8 +51,9 @@ References: ## Why it caused an incident here 1. **Every write goes through one path.** All MySQL controllers issue writes via - `mysql.ExecWrapper → c.db.Exec`, which opens a fresh connection per call - (`pkg/clients/mysql/mysql.go`). This covers, in the User controller, + `mysql.ExecWrapper → c.db.Exec`. At the time of the incident this opened a + fresh connection per call (`pkg/clients/mysql/mysql.go`); see the connection + pool follow-up below. This covers, in the User controller, `CREATE USER`, `ALTER USER ... WITH `, `ALTER USER ... IDENTIFIED WITH `, `ALTER USER ... IDENTIFIED BY`, `DROP USER`; in Grant, `GRANT` / `REVOKE`; in Database, @@ -118,12 +119,11 @@ is the application-level `KILL QUERY` pattern described above. - **Application-level `KILL QUERY` on cancel** for the non-lock hang case (dedicated connection + `CONNECTION_ID()` + out-of-band `KILL`). -- **Bounded shared connection pool.** The client currently calls `sql.Open` per - query and closes it immediately (no reuse, no `SetMaxOpenConns` / - `SetConnMaxLifetime`). A shared, capped `*sql.DB` would hard-limit total - connections and remove per-query connect/auth overhead, but requires adding a - `Close()` to the `xsql.DB` interface and wiring it through `Disconnect` for - all three drivers (MySQL, PostgreSQL, MSSQL). +- **Bounded shared connection pool.** Addressed separately — see + `docs/connection-pool.md` and #195/#434, which introduce a shared, DSN-keyed + `*sql.DB` pool (with `MaxOpenConns` / `MaxIdleConns` / `ConnMaxLifetime` / + `ConnMaxIdleTime`) across all three drivers instead of opening a new + connection per query. - **Harden the reconcile feedback loop** so a persistently failing statement does not re-fire every poll cycle. - **Make `lock_wait_timeout` configurable** (e.g. via `ProviderConfig`) for