From 7477ee694b45f23698687b0ce22123d17f09a810 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Sun, 6 Sep 2026 14:35:21 -0600 Subject: [PATCH 1/2] Always reject read-only connections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream makes this an option (rejectReadOnly) defaulting to off. It is unconditional here, and Config.RejectReadOnly is gone. The failure it prevents is silent, and the mistake that causes it is invisible. RDS and Aurora fail over by moving DNS: a pooled connection to the demoted writer stays open and stays usable, and every write on it fails for as long as the pool keeps it — potentially until the process restarts. Nothing in the DSN or in the error says the connection is the problem, so a deployment that left the option off does not find out until a failover, which is the worst possible moment to learn it. An option whose only correct setting in the environment we deploy in is "on" is not really an option; it is a step you can forget. What happens to the parameter: rejectReadOnly=true still parses and does nothing, so a DSN written for upstream keeps working. rejectReadOnly=false is an error rather than a silent no-op — it states an expectation the driver will not meet, and quietly ignoring it is the same class of problem this change exists to remove. One carve-out, which upstream's own test suite found: a transaction opened with driver.TxOptions.ReadOnly is exempt. There the read-only error is the answer the caller asked for, and database/sql does not retry inside a transaction, so rejecting would replace a usable *MySQLError with a dead transaction — TestContextBeginReadOnly failed exactly that way before the exemption, and passes unmodified with it. A session the application makes read-only with its own SET is deliberately not exempt: nothing distinguishes it from a demoted writer. Upstream's TestRejectReadOnly loses the case where the option is off, since that is no longer a state the driver can be in; the first case now covers a DSN that says nothing, which is where the old default did the wrong thing. Full suite passes against MySQL 8.0.44, race enabled. --- README.md | 81 +++++++++++++++++++++------------ connection.go | 8 ++++ driver_test.go | 23 ++++++---- dsn.go | 21 +++++---- dsn_test.go | 4 +- packets.go | 27 +++++++++-- readonly_test.go | 116 +++++++++++++++++++++++++++++++++++++++++++++++ transaction.go | 2 + 8 files changed, 230 insertions(+), 52 deletions(-) create mode 100644 readonly_test.go diff --git a/README.md b/README.md index 48d12bfa9..86330bd65 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,8 @@ partition's own bundle to verify them. ## What this fork changes -Two things, both for packaging reasons only. Neither alters protocol behaviour. +Three things. The first two are packaging; neither alters protocol +behaviour. The third changes a default, deliberately. **The module path is `github.com/block/mysql`.** Upstream's path plus a `replace` directive would work for a binary, but `replace` is not inherited @@ -66,6 +67,14 @@ connections with: db, err := sql.Open("block-mysql", dsn) ``` +**Read-only connections are always rejected.** Upstream's `rejectReadOnly` +option defaults to off; here the behaviour is unconditional and +`Config.RejectReadOnly` is gone. RDS and Aurora fail over by moving DNS, so a +pooled connection to the demoted writer stays open and every write on it fails +until the process restarts — with nothing in the DSN or the error to say the +connection is the problem. See the `rejectReadOnly` parameter below for what +happens to a DSN that still sets it. + The DSN format, `Config`, and the rest of the API are upstream's. ## Linking both drivers @@ -99,9 +108,13 @@ git merge upstream/master Edits to upstream files are confined to three things: the module path and driver name (`go.mod`, `driver.go`, plus doc comments and test call sites that -spell either one out), the CI matrix (see below), and a one-line call in -`Config.normalize` that hands off to `rds.go`. The capabilities above live in -files upstream does not have, which is what keeps merges near-mechanical. +spell either one out), the CI matrix (see below), a one-line call in +`Config.normalize` that hands off to `rds.go`, and the read-only rejection (one +condition in `packets.go`, the parameter in `dsn.go`, and the +read-only-transaction flag in `connection.go`/`transaction.go`). The +capabilities above live in files upstream does not have, which is what keeps +merges near-mechanical. + Additions are cheapest when they follow the same shape: new files, or new methods on existing types, in preference to reworking an upstream code path. @@ -522,30 +535,42 @@ I/O read timeout. The value must be a decimal number with a unit suffix (*"ms"*, ``` Type: bool -Valid Values: true, false -Default: false -``` - - -`rejectReadOnly=true` causes the driver to reject read-only connections. This -is for a possible race condition during an automatic failover, where the mysql -client gets connected to a read-only replica after the failover. - -Note that this should be a fairly rare case, as an automatic failover normally -happens when the primary is down, and the race condition shouldn't happen -unless it comes back up online as soon as the failover is kicked off. On the -other hand, when this happens, a MySQL application can get stuck on a -read-only connection until restarted. It is however fairly easy to reproduce, -for example, using a manual failover on AWS Aurora's MySQL-compatible cluster. - -If you are not relying on read-only transactions to reject writes that aren't -supposed to happen, setting this on some MySQL providers (such as AWS Aurora) -is safer for failovers. - -Note that ERROR 1290 can be returned for a `read-only` server and this option will -cause a retry for that error. However the same error number is used for some -other cases. You should ensure your application will never cause an ERROR 1290 -except for `read-only` mode when enabling this option. +Valid Values: true +Default: (always on; see below) +``` + +**Changed in this fork.** Upstream makes this an option, defaulting to off. +Here the driver always rejects read-only connections, `Config.RejectReadOnly` +is gone, and the parameter survives only so a DSN written for upstream keeps +parsing: `rejectReadOnly=true` is accepted and does nothing, while +`rejectReadOnly=false` is an error rather than a silent no-op, because it +states an expectation the driver will not meet. + +Rejecting means that when a statement fails with a read-only error (1792, 1290 +or 1836), the driver closes that connection and returns `driver.ErrBadConn`, so +`database/sql` retries the statement on a new one. + +It is not optional because the failure it prevents is silent and the mistake +that causes it is invisible. RDS and Aurora fail over by moving DNS: a pooled +connection to the demoted writer stays open and stays usable, and every write +on it fails for as long as the pool keeps it — until the process restarts. +Nothing in the DSN or in the error says the connection is the problem, and a +deployment that forgot the option does not find out until a failover. + +One exception: a transaction opened with `sql.TxOptions{ReadOnly: true}` is +exempt. There the read-only error is the answer the caller asked for, and +`database/sql` does not retry inside a transaction anyway, so rejecting would +replace a usable `*MySQLError` with a dead transaction. + +Two consequences worth knowing: + +* A session made read-only by the application's own `SET SESSION TRANSACTION + READ ONLY` is *not* exempt — nothing distinguishes it from a demoted writer. + Writes on such a session are retried on a new connection instead of failing. + Use privileges, or the `ReadOnly` transaction option, to express that intent. +* ERROR 1290 is also raised for some conditions unrelated to read-only mode. + Those are now retried too, and if the condition persists the caller sees + `driver.ErrBadConn` rather than the original error. ##### `serverPubKey` diff --git a/connection.go b/connection.go index 35d669ca7..3d38df951 100644 --- a/connection.go +++ b/connection.go @@ -41,6 +41,13 @@ type mysqlConn struct { parseTime bool compress bool + // inReadOnlyTx is set while a transaction the caller explicitly opened + // with driver.TxOptions.ReadOnly is in flight. Fork addition: it is the + // one case where a read-only error is the answer the caller asked for + // rather than a sign of a demoted writer, so handleErrorPacket must not + // turn it into ErrBadConn. See packets.go. + inReadOnlyTx bool + // for context support (Go 1.8+) watching bool watcher chan<- context.Context @@ -165,6 +172,7 @@ func (mc *mysqlConn) begin(readOnly bool) (driver.Tx, error) { } err := mc.exec(q) if err == nil { + mc.inReadOnlyTx = readOnly return &mysqlTx{mc}, err } return nil, mc.markBadConn(err) diff --git a/driver_test.go b/driver_test.go index 8430f3850..65a197588 100644 --- a/driver_test.go +++ b/driver_test.go @@ -2293,26 +2293,29 @@ func TestColumnsReusesSlice(t *testing.T) { } } +// TestRejectReadOnly exercises the read-only rejection, which this fork +// applies unconditionally. Upstream's version of this test also asserted the +// behaviour with the option off; that is no longer a state the driver can be +// in, so the first case now covers a DSN that says nothing. func TestRejectReadOnly(t *testing.T) { + // No parameter: the rejection happens anyway. This is the case upstream's + // default gets wrong. runTests(t, dsn, func(dbt *DBTest) { // Create Table dbt.mustExec("CREATE TABLE test (value BOOL)") - // Set the session to read-only. We didn't set the `rejectReadOnly` - // option, so any writes after this should fail. + // Set the session to read only. Any writes after this should error on + // a driver.ErrBadConn, and cause `database/sql` to initiate a new + // connection. _, err := dbt.db.Exec("SET SESSION TRANSACTION READ ONLY") // Error 1193: Unknown system variable 'TRANSACTION' => skip test, // MySQL server version is too old maybeSkip(t, err, 1193) - if _, err := dbt.db.Exec("DROP TABLE test"); err == nil { - t.Fatalf("writing to DB in read-only session without " + - "rejectReadOnly did not error") - } - // Set the session back to read-write so runTests() can properly clean - // up the table `test`. - dbt.mustExec("SET SESSION TRANSACTION READ WRITE") + // This would error, but `database/sql` should automatically retry on a + // new connection which is not read-only, and eventually succeed. + dbt.mustExec("DROP TABLE test") }) - // Enable the `rejectReadOnly` option. + // rejectReadOnly=true still parses, for a DSN written against upstream. runTests(t, dsn+"&rejectReadOnly=true", func(dbt *DBTest) { // Create Table dbt.mustExec("CREATE TABLE test (value BOOL)") diff --git a/dsn.go b/dsn.go index c3ea608d1..ca8d62d72 100644 --- a/dsn.go +++ b/dsn.go @@ -72,7 +72,6 @@ type Config struct { InterpolateParams bool // Interpolate placeholders into query string MultiStatements bool // Allow multiple statements in one query ParseTime bool // Parse time values to time.Time - RejectReadOnly bool // Reject read-only connections // unexported fields. new options should be come here. // boolean first. alphabetical order. @@ -407,10 +406,6 @@ func (cfg *Config) FormatDSN() string { writeDSNParam(&buf, &hasParam, "readTimeout", cfg.ReadTimeout.String()) } - if cfg.RejectReadOnly { - writeDSNParam(&buf, &hasParam, "rejectReadOnly", "true") - } - if len(cfg.ServerPubKey) > 0 { writeDSNParam(&buf, &hasParam, "serverPubKey", url.QueryEscape(cfg.ServerPubKey)) } @@ -677,13 +672,23 @@ func parseDSNParams(cfg *Config, params string) (err error) { return } - // Reject read-only connections + // Reject read-only connections. + // + // Fork change: this is unconditional here, so the parameter carries no + // information. It is still accepted, because a DSN written for upstream + // should not stop parsing — but only in the direction that agrees with + // what the driver does. rejectReadOnly=false is refused rather than + // ignored: it states an expectation the driver will not meet, and a + // silent no-op is exactly the kind of quiet disagreement this change + // exists to remove. See packets.go. case "rejectReadOnly": - var isBool bool - cfg.RejectReadOnly, isBool = readBool(value) + on, isBool := readBool(value) if !isBool { return errors.New("invalid bool value: " + value) } + if !on { + return errors.New("rejectReadOnly=false: this driver always rejects read-only connections; the option cannot be disabled") + } // Server public key case "serverPubKey": diff --git a/dsn_test.go b/dsn_test.go index 120550cf4..f060801f0 100644 --- a/dsn_test.go +++ b/dsn_test.go @@ -129,7 +129,9 @@ var testDSNs = []struct { cfg.AllowOldPasswords = true cfg.ClientFoundRows = true cfg.ParseTime = true - cfg.RejectReadOnly = true + // rejectReadOnly=true is still in the DSN above: it parses and + // sets nothing, because this driver always rejects read-only + // connections. See dsn.go. }), }, { diff --git a/packets.go b/packets.go index 88f318e33..8deab175b 100644 --- a/packets.go +++ b/packets.go @@ -598,12 +598,29 @@ func (mc *mysqlConn) handleErrorPacket(data []byte) error { // 1792: ER_CANT_EXECUTE_IN_READ_ONLY_TRANSACTION // 1290: ER_OPTION_PREVENTS_STATEMENT (returned by Aurora during failover) // 1836: ER_READ_ONLY_MODE - if (errno == 1792 || errno == 1290 || errno == 1836) && mc.cfg.RejectReadOnly { + if (errno == 1792 || errno == 1290 || errno == 1836) && !mc.inReadOnlyTx { // Oops; we are connected to a read-only connection, and won't be able - // to issue any write statements. Since RejectReadOnly is configured, - // we throw away this connection hoping this one would have write - // permission. This is specifically for a possible race condition - // during failover (e.g. on AWS Aurora). See README.md for more. + // to issue any write statements. We throw away this connection hoping + // the next one would have write permission. This is specifically for a + // possible race condition during failover (e.g. on AWS Aurora). See + // README.md for more. + // + // Fork change: upstream gates this on the rejectReadOnly option, which + // defaults to off. It is unconditional here. On a provider that fails + // over by moving DNS — RDS and Aurora both do — an application that + // leaves it off keeps a pooled connection to the demoted writer and + // every write on it fails, indefinitely, with no error that says the + // connection is the problem. Nothing about the DSN reveals the + // omission, so it is not a mistake a deployment discovers except + // during a failover. See dsn.go for what happens to the option. + // + // The one exception is a transaction the caller opened with + // driver.TxOptions.ReadOnly: there the read-only error is the answer + // they asked for, and database/sql does not retry inside a + // transaction anyway, so rejecting would replace a usable + // *MySQLError with a dead transaction. A session made read-only by + // the application's own SET is not exempt — nothing distinguishes it + // from a demoted writer. // // We explicitly close the connection before returning // driver.ErrBadConn to ensure that `database/sql` purges this diff --git a/readonly_test.go b/readonly_test.go new file mode 100644 index 000000000..95cf9b448 --- /dev/null +++ b/readonly_test.go @@ -0,0 +1,116 @@ +// Go MySQL Driver - A MySQL-Driver for Go's database/sql package +// +// Copyright 2026 The Go-MySQL-Driver Authors. All rights reserved. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +package mysql + +import ( + "database/sql/driver" + "encoding/binary" + "errors" + "strings" + "testing" +) + +// TestRejectReadOnlyDSN covers what the rejectReadOnly parameter does now that +// the behaviour it used to control is unconditional. It is accepted in the +// direction that agrees with the driver and refused in the direction that does +// not, so a DSN carrying the upstream default is a loud failure rather than a +// silent disagreement. +func TestRejectReadOnlyDSN(t *testing.T) { + for _, value := range []string{"true", "1", "TRUE"} { + if _, err := ParseDSN("user:pass@tcp(127.0.0.1:3306)/db?rejectReadOnly=" + value); err != nil { + t.Errorf("rejectReadOnly=%s: %v (a DSN written for upstream should still parse)", value, err) + } + } + + for _, value := range []string{"false", "0", "FALSE"} { + _, err := ParseDSN("user:pass@tcp(127.0.0.1:3306)/db?rejectReadOnly=" + value) + if err == nil { + t.Errorf("rejectReadOnly=%s parsed without error; it states an expectation the driver will not meet", value) + continue + } + if !strings.Contains(err.Error(), "cannot be disabled") { + t.Errorf("rejectReadOnly=%s: error %q does not explain that the option is gone", value, err) + } + } + + if _, err := ParseDSN("user:pass@tcp(127.0.0.1:3306)/db?rejectReadOnly=yes"); err == nil { + t.Error("rejectReadOnly=yes parsed without error; a non-boolean value is still a malformed DSN") + } +} + +// TestReadOnlyErrorsAreBadConn pins the rejection itself: the three read-only +// error numbers must yield driver.ErrBadConn — which is what makes +// database/sql discard the connection and retry — with no configuration +// involved. Upstream gates this on an option that defaults to off. +func TestReadOnlyErrorsAreBadConn(t *testing.T) { + // 1792: ER_CANT_EXECUTE_IN_READ_ONLY_TRANSACTION + // 1290: ER_OPTION_PREVENTS_STATEMENT (returned by Aurora during failover) + // 1836: ER_READ_ONLY_MODE + for _, errno := range []uint16{1792, 1290, 1836} { + _, mc := newRWMockConn(0) + err := mc.handleErrorPacket(errPacket(errno, "read-only")) + if !errors.Is(err, driver.ErrBadConn) { + t.Errorf("errno %d returned %v, want driver.ErrBadConn", errno, err) + } + if !mc.closed.Load() { + t.Errorf("errno %d did not close the connection; database/sql would hand it back out", errno) + } + } + + // An unrelated error must still surface as itself. Widening the rejection + // to errors that are not about read-only would turn a real failure into a + // silent retry loop. + _, mc := newRWMockConn(0) + err := mc.handleErrorPacket(errPacket(1062, "Duplicate entry")) + var myErr *MySQLError + if !errors.As(err, &myErr) || myErr.Number != 1062 { + t.Errorf("errno 1062 returned %v, want a *MySQLError with Number 1062", err) + } + if mc.closed.Load() { + t.Error("errno 1062 closed the connection") + } +} + +// TestReadOnlyTxIsExempt covers the one case that must NOT be rejected: a +// transaction the caller opened with driver.TxOptions.ReadOnly. There the +// read-only error is the answer they asked for, and database/sql does not +// retry inside a transaction — rejecting would replace a usable *MySQLError +// with a dead transaction. (Upstream's TestContextBeginReadOnly is the +// end-to-end version of this and passes unmodified.) +func TestReadOnlyTxIsExempt(t *testing.T) { + _, mc := newRWMockConn(0) + mc.inReadOnlyTx = true + + err := mc.handleErrorPacket(errPacket(1792, "Cannot execute statement in a READ ONLY transaction")) + var myErr *MySQLError + if !errors.As(err, &myErr) || myErr.Number != 1792 { + t.Errorf("inside an explicit read-only transaction, errno 1792 returned %v, want a *MySQLError", err) + } + if mc.closed.Load() { + t.Error("the connection was closed; the caller's own read-only transaction is not a demoted writer") + } + + // Once the transaction ends the exemption must end with it, or one + // read-only transaction disarms the protection for the rest of the + // connection's life. + mc.inReadOnlyTx = false + if err := mc.handleErrorPacket(errPacket(1792, "read-only")); !errors.Is(err, driver.ErrBadConn) { + t.Errorf("after the transaction, errno 1792 returned %v, want driver.ErrBadConn", err) + } +} + +// errPacket builds the ERR packet body handleErrorPacket expects: the 0xff +// marker, the error number, then a SQL state block and the message. +func errPacket(errno uint16, message string) []byte { + data := []byte{iERR, 0, 0} + binary.LittleEndian.PutUint16(data[1:3], errno) + data = append(data, '#') + data = append(data, []byte("HY000")...) + return append(data, []byte(message)...) +} diff --git a/transaction.go b/transaction.go index 8c502f49e..ad41ea2c4 100644 --- a/transaction.go +++ b/transaction.go @@ -24,6 +24,7 @@ func (tx *mysqlTx) Commit() (err error) { return } err = tx.mc.exec("COMMIT") + tx.mc.inReadOnlyTx = false tx.mc = nil return } @@ -40,6 +41,7 @@ func (tx *mysqlTx) Rollback() (err error) { return } err = tx.mc.exec("ROLLBACK") + tx.mc.inReadOnlyTx = false tx.mc = nil return } From adddf85e7dba0d832f5c0ef4e60b86242eb30cf5 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Sun, 6 Sep 2026 16:40:48 -0600 Subject: [PATCH 2/2] readonly: test the exemption lifecycle, log the discarded error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes from review. The exemption flag had no test driving it. TestReadOnlyTxIsExempt proved handleErrorPacket reads it, but set the field itself, so three mutations survived the full suite: begin setting it unconditionally, and either of Commit or Rollback failing to clear it. The first is the worst — every transaction exempt means any connection that has ever run a BeginTx stops rejecting read-only errors for the rest of its life in the pool, the feature silently off. TestReadOnlyTxLifecycle drives begin/Commit/Rollback against a mock server and kills all three. ResetSession now clears the flag too. Every sql.Tx ends in Commit or Rollback so this should be unreachable, but the stuck direction is the unsafe one and a pooled connection's assumptions belong there. The server's error was discarded with nothing logged. For the failover this is written for that is fine — database/sql retries and the caller sees nothing. For a target that stays read-only it is not: the retry budget burns, the caller gets a bare driver.ErrBadConn, and the one string that named the problem was assembled nowhere. Log it before closing. README: name the 1290s that are not failover (secure_file_priv, super_read_only, innodb_read_only, --skip-grant-tables), and say plainly what a deliberately read-only deployment should do now that the option is gone. --- README.md | 22 +++++++++-- connection.go | 8 ++++ packets.go | 14 +++++++ readonly_test.go | 100 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 86330bd65..e57aaf744 100644 --- a/README.md +++ b/README.md @@ -106,7 +106,7 @@ git fetch upstream git merge upstream/master ``` -Edits to upstream files are confined to three things: the module path and +Edits to upstream files are confined to four things: the module path and driver name (`go.mod`, `driver.go`, plus doc comments and test call sites that spell either one out), the CI matrix (see below), a one-line call in `Config.normalize` that hands off to `rds.go`, and the read-only rejection (one @@ -568,9 +568,23 @@ Two consequences worth knowing: READ ONLY` is *not* exempt — nothing distinguishes it from a demoted writer. Writes on such a session are retried on a new connection instead of failing. Use privileges, or the `ReadOnly` transaction option, to express that intent. -* ERROR 1290 is also raised for some conditions unrelated to read-only mode. - Those are now retried too, and if the condition persists the caller sees - `driver.ErrBadConn` rather than the original error. +* ERROR 1290 is also raised for conditions unrelated to failover — the ones you + are likely to meet are `secure_file_priv` (a `SELECT … INTO OUTFILE` or `LOAD + DATA` outside the permitted directory), `super_read_only`, `innodb_read_only` + and `--skip-grant-tables`. All of these persist rather than clear, so the + statement is retried, the connection churned, and the caller finally sees + `driver.ErrBadConn` rather than the message that named the problem. The + driver logs the server's own error before discarding it, so the condition is + still identifiable — look for `closing read-only connection` in the log. + +**If your target is deliberately read-only** — an Aurora reader endpoint, a +replica, a source you only ever read from — this fork is a poor fit for that +connection, and there is no longer an option to turn it off. Wrap reads in +`sql.TxOptions{ReadOnly: true}` and they are exempt; anything on autocommit +that the server rejects will still be retried and churned. If that is not +workable, use a driver that lets you disable the behaviour for that connection. +The trade is deliberate: the population this fork serves writes to RDS +primaries, where the silent failure is the more expensive one. ##### `serverPubKey` diff --git a/connection.go b/connection.go index 3d38df951..dd3329251 100644 --- a/connection.go +++ b/connection.go @@ -803,6 +803,14 @@ func (mc *mysqlConn) ResetSession(ctx context.Context) error { return driver.ErrBadConn } + // Fork addition: re-establish the read-only-transaction exemption for the + // new borrower. Commit and Rollback both clear it and every sql.Tx ends in + // one of them, so this is hardening rather than a fix — but the direction + // it can get stuck in, true, silently disables the read-only rejection for + // the rest of this connection's life, and a pooled connection's + // assumptions belong here. See packets.go. + mc.inReadOnlyTx = false + // Perform a stale connection check. We only perform this check for // the first query on a connection that has been checked out of the // connection pool: a fresh connection from the pool is more likely diff --git a/packets.go b/packets.go index 8deab175b..a9617f4d4 100644 --- a/packets.go +++ b/packets.go @@ -622,6 +622,20 @@ func (mc *mysqlConn) handleErrorPacket(data []byte) error { // the application's own SET is not exempt — nothing distinguishes it // from a demoted writer. // + // Log before discarding it. On the failover this is written for the + // caller never sees anything — database/sql retries onto a healthy + // connection — so one line is the whole trace. On a target that is + // *persistently* read-only (a reader endpoint, a cluster with no + // writer, super_read_only left on after maintenance) database/sql + // burns its retry budget and hands the caller a bare + // driver.ErrBadConn; without this, the server's own message — the only + // text that names the actual problem — is assembled nowhere. + msg := data[3:] + if len(msg) > 6 && msg[0] == 0x23 { + msg = msg[6:] // skip the "#HY000" SQL-state marker, as below + } + mc.log("closing read-only connection, errno ", errno, ": ", string(msg)) + // We explicitly close the connection before returning // driver.ErrBadConn to ensure that `database/sql` purges this // connection and initiates a new one for next statement next time. diff --git a/readonly_test.go b/readonly_test.go index 95cf9b448..16925d2c5 100644 --- a/readonly_test.go +++ b/readonly_test.go @@ -9,6 +9,7 @@ package mysql import ( + "context" "database/sql/driver" "encoding/binary" "errors" @@ -105,6 +106,105 @@ func TestReadOnlyTxIsExempt(t *testing.T) { } } +// TestReadOnlyTxLifecycle drives the exemption flag through begin, Commit, +// Rollback and ResetSession, which is the only place it can actually go wrong +// and the reason the field exists. +// +// TestReadOnlyTxIsExempt above proves handleErrorPacket *reads* the flag +// correctly, but it sets the field itself, so it cannot tell a flag that is +// cleared from one that nobody ever clears. A flag stuck true silently +// disables the read-only rejection for the rest of the connection's life in +// the pool — the whole feature off, with no symptom. +func TestReadOnlyTxLifecycle(t *testing.T) { + // beginTx runs a scripted START TRANSACTION against a mock server and + // returns the connection with the flag in whatever state begin left it. + beginTx := func(t *testing.T, readOnly bool) (*mockConn, *mysqlConn, driver.Tx) { + t.Helper() + conn, mc := newRWMockConn(0) + conn.queuedReplies = [][]byte{okPacket()} + tx, err := mc.begin(readOnly) + if err != nil { + t.Fatalf("begin(%v): %v", readOnly, err) + } + return conn, mc, tx + } + + // rejects reports what a read-only error does on this connection now. + rejects := func(mc *mysqlConn) bool { + return errors.Is(mc.handleErrorPacket(errPacket(1792, "read-only")), driver.ErrBadConn) + } + + t.Run("a read-only transaction sets it", func(t *testing.T) { + _, mc, _ := beginTx(t, true) + if !mc.inReadOnlyTx { + t.Error("begin(true) did not set inReadOnlyTx; the caller's own read-only transaction would be rejected") + } + }) + + t.Run("a read-write transaction does not", func(t *testing.T) { + // This is the direction that disarms the feature: if begin set the + // flag unconditionally, any connection that had ever run a BeginTx + // would stop rejecting read-only errors. + _, mc, _ := beginTx(t, false) + if mc.inReadOnlyTx { + t.Error("begin(false) set inReadOnlyTx; an ordinary transaction would exempt the connection from read-only rejection") + } + if !rejects(mc) { + t.Error("a read-only error inside a read-write transaction was not rejected") + } + }) + + t.Run("Commit clears it", func(t *testing.T) { + conn, mc, tx := beginTx(t, true) + conn.queuedReplies = [][]byte{okPacket()} + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + if mc.inReadOnlyTx { + t.Fatal("Commit left inReadOnlyTx set") + } + if !rejects(mc) { + t.Error("after Commit a read-only error was still exempt; one read-only transaction disarmed the connection for good") + } + }) + + t.Run("Rollback clears it", func(t *testing.T) { + conn, mc, tx := beginTx(t, true) + conn.queuedReplies = [][]byte{okPacket()} + if err := tx.Rollback(); err != nil { + t.Fatalf("Rollback: %v", err) + } + if mc.inReadOnlyTx { + t.Fatal("Rollback left inReadOnlyTx set") + } + if !rejects(mc) { + t.Error("after Rollback a read-only error was still exempt") + } + }) + + t.Run("ResetSession clears it", func(t *testing.T) { + // Belt and braces: every sql.Tx ends in Commit or Rollback, so this + // should be unreachable — but it is the point where a pooled + // connection's assumptions are re-established for a new borrower, and + // the stuck direction of this flag is the unsafe one. + _, mc, _ := beginTx(t, true) + if err := mc.ResetSession(context.Background()); err != nil { + t.Fatalf("ResetSession: %v", err) + } + if mc.inReadOnlyTx { + t.Error("ResetSession left inReadOnlyTx set; the next borrower inherits the exemption") + } + }) +} + +// okPacket builds a minimal OK packet: header (length, sequence 1), then the +// 0x00 marker, zero affected rows, zero last-insert-id, autocommit status and +// no warnings. Sequence 1 is what the server answers a freshly written command +// with, which is what mc.exec expects back. +func okPacket() []byte { + return []byte{7, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0} +} + // errPacket builds the ERR packet body handleErrorPacket expects: the 0xff // marker, the error number, then a SQL state block and the message. func errPacket(errno uint16, message string) []byte {