Always reject read-only connections

[morgo]

Upstream makes this an option (rejectReadOnly) defaulting to off. It is unconditional here, and Config.RejectReadOnly is gone.

Why it shouldn't be an option

RDS and Aurora fail over by moving DNS. A pooled connection to the demoted writer stays open and stays usable, so 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 doesn't find out until a failover, which is the worst moment to learn it.

An option whose only correct setting in the environment we deploy in is "on" isn't really an option; it's a step you can forget. Removing it removes the failure mode.

What happens to the parameter

DSN	Result
rejectReadOnly=true	Parses, does nothing — a DSN written for upstream keeps working
rejectReadOnly=false	Error. It states an expectation the driver will not meet, and quietly ignoring it is the same class of problem this change exists to remove
rejectReadOnly=yes	Error, as before — still a malformed DSN

Config.RejectReadOnly is removed rather than kept-and-ignored, so a caller setting it in Go fails to compile instead of silently having no effect. (strata's pkg/mysqlrds is the one place in Block code that sets it; the line just deletes.)

One carve-out, which upstream's own 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 replaces a usable *MySQLError with a dead transaction. TestContextBeginReadOnly failed exactly that way before the exemption:

driver_test.go:3035: expected MySQLError, got driver: bad connection

and passes unmodified with it. The flag is set in begin() and cleared on commit/rollback.

A session the application makes read-only with its own SET SESSION TRANSACTION READ ONLY is deliberately not exempt — nothing distinguishes it from a demoted writer. That is a real behaviour change for anyone relying on session state to reject writes, and it's documented in the README.

Tests

readonly_test.go (new, fork-owned) covers the DSN parameter in all three directions, the three error numbers each yielding ErrBadConn with the connection closed, an unrelated error (1062) still surfacing as itself with the connection intact, and the read-only-transaction exemption ending when the transaction does — otherwise one read-only transaction would disarm the protection for the rest of the connection's life.

Upstream's TestRejectReadOnly loses the "option off" case, 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 with -race.

Merge order

Independent of #4, but both touch the same README paragraphs; whichever lands second gets rebased.

[coveralls]

Coverage Report for CI Build 34064857257

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.2%) to 84.767%

Details

• Coverage increased (+0.2%) from the base build.
• Patch coverage: 45 of 45 lines across 4 files are fully covered (100%).
• 1 coverage regression across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
dsn.go	1	85.92%

---

Coverage Stats

Relevant Lines:	4267
Covered Lines:	3617
Line Coverage:	84.77%
Coverage Strength:	328355.4 hits per line

---

💛 - Coveralls

[Copilot]

🟢 Approval recommended

The behavior change is consistently implemented across DSN parsing, connection error handling, and transaction lifecycle, with targeted tests and documentation covering the new contract and exemption.

Pull request overview

This PR makes read-only connection rejection unconditional in this fork of the Go MySQL driver, removes the Config.RejectReadOnly API surface, and preserves DSN compatibility by accepting rejectReadOnly=true while erroring on rejectReadOnly=false. It also introduces an explicit carve-out for transactions opened with TxOptions.ReadOnly, so read-only errors in that case remain surfaced as *MySQLError rather than being converted into driver.ErrBadConn.

Changes:

• Make read-only error handling (errno 1792/1290/1836) always close the connection and return driver.ErrBadConn, except during explicit read-only transactions.
• Remove Config.RejectReadOnly and update DSN parsing so rejectReadOnly=true is accepted (no-op) and rejectReadOnly=false becomes an error.
• Add/adjust tests and documentation to pin the new behavior and the read-only-transaction exemption lifecycle.

File summaries

File	Description
connection.go	Adds inReadOnlyTx state and sets it on successful BeginTx(...ReadOnly: true) starts.
transaction.go	Clears inReadOnlyTx on commit/rollback so the exemption doesn’t outlive the transaction.
packets.go	Makes read-only error rejection unconditional (gated only by !inReadOnlyTx).
dsn.go	Removes Config.RejectReadOnly, drops DSN formatting for it, and changes DSN parsing semantics for rejectReadOnly.
driver_test.go	Updates upstream read-only rejection test to reflect “always-on” behavior and no-parameter default.
dsn_test.go	Updates DSN parsing expectations now that rejectReadOnly=true is a no-op.
readonly_test.go	Adds fork-owned unit tests for DSN parameter behavior, ErrBadConn mapping + connection closure, and the read-only-tx exemption.
README.md	Documents the fork behavior change, DSN parameter semantics, and the explicit read-only transaction carve-out.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0
• Review effort level: Lite

---

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

[aparajon]

🤖 Adversarial correctness review — 902395bf (+230/-53, 8 files)

The argument for making this unconditional is right, and the part I'd have pushed back on is already handled: the read-only-transaction exemption is the correct carve-out, for the correct reason (database/sql doesn't retry inside a transaction, so rejecting would trade a usable *MySQLError for a dead one), and refusing rejectReadOnly=false rather than ignoring it is the right call — a silent no-op is the failure mode the whole change exists to remove. The README rewrite is unusually honest for a behaviour change; it names both consequences rather than only the win.

My findings are about the two silences that are left.

#	Sev	Where	What
1	med	connection.go:175, transaction.go:27,44	The exemption flag's lifecycle has no test. Setting it unconditionally — which exempts every transaction and silently turns the whole feature off — passes the entire suite, as does deleting either clear
2	low-med	packets.go:629	The server's error is discarded with nothing logged, so on a persistently read-only target the caller gets driver.ErrBadConn and no trace of "read-only" anywhere — the same silence this change exists to remove, moved rather than closed
3	low	README, rejectReadOnly	The non-read-only 1290 triggers aren't named, and a deployment deliberately pointed at a reader endpoint now has no escape hatch at all

---

1 — the flag that disarms the protection is never driven by a test (med)

Four mutations against the new logic, full suite each time (TestLoadData excluded — it fails identically at master, local_infile isn't set on my server):

Mutation	Result
remove the !mc.inReadOnlyTx exemption entirely	killed — TestReadOnlyTxIsExempt, TestContextBeginReadOnly
mc.inReadOnlyTx = readOnly → = true in begin	survives
Commit stops clearing the flag	survives
Rollback stops clearing the flag	survives

The second one is the one that matters. = true means every transaction is exempt, so any connection that has ever run a BeginTx — read-write included — stops rejecting read-only errors for the rest of its life in the pool. That is the feature turned off, silently, and the suite is green.

What makes it worth a finding rather than a coverage note is that TestReadOnlyTxIsExempt already states the invariant:

	// 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

The comment describes exactly the mutation that survives, and then the test satisfies it by assigning the field itself. Nothing in the suite ever calls begin, Commit or Rollback and observes the flag — which is the only place the lifecycle can actually go wrong, and the reason the field exists at all. As written the test proves handleErrorPacket reads the flag correctly; it can't distinguish that from a flag nobody ever clears.

newRWMockConn plus a scripted OK packet gets there without a server: mc.begin(true), assert inReadOnlyTx, tx.Commit(), assert it's cleared, then re-run the 1792 packet and require ErrBadConn. That's one test that kills all three survivors. The integration-side alternative is extending TestContextBeginReadOnly past the commit, which also pins that a read-write BeginTx does not set the flag.

While that's open: clearing inReadOnlyTx in ResetSession would make the invariant per-checkout instead of per-Commit-path. I couldn't find a way to strand it true through database/sql — Tx always ends in Commit or Rollback, and a failed Commit still clears — so this is hardening, not a bug. But it's a flag whose stuck-true direction silently disables a safety default, and ResetSession is where a pooled connection's assumptions get re-established anyway.

2 — the original error is dropped on the floor, and nothing logs it (low-med)

	mc.Close()
	return driver.ErrBadConn

The case for this change is that a demoted-writer connection fails "with no error that says the connection is the problem." That's true, and it's fixed for the failover case, because database/sql retries onto a healthy connection and the caller never sees anything.

It is not fixed for the case where the target stays read-only — a reader endpoint, a cluster with no writer, super_read_only left on after maintenance. There database/sql burns its retry budget (two cached-conn attempts, then one forced-new) and hands the caller driver.ErrBadConn, which says nothing at all. Before this change the caller got Error 1290: The MySQL server is running with the --read-only option so it cannot execute this statement — the string that names the actual problem. After it, that string is constructed nowhere: the error number is read, matched, and discarded before me.Message is ever assembled.

So the silence didn't close, it moved: from "a write that mysteriously fails forever" to "a write that mysteriously fails forever and churns three connections per attempt." The README's "the caller sees driver.ErrBadConn rather than the original error" states this accurately, which is good, but stating it isn't the same as leaving a trace.

One line fixes it, using the logger already wired into mysqlConn and used two files over for closing bad idle connection:

mc.log("closing read-only connection, errno ", errno, ": ", string(data[3:]))
mc.Close()
return driver.ErrBadConn

Now the failover case logs once and moves on, and the persistent case leaves a repeating line that names the condition. That is the whole diagnostic gap, and it costs nothing on the path that matters.

3 — who loses the option, and what 1290 actually means (low)

Two things I'd add to the rejectReadOnly section, since it's now the reference for a behaviour nobody can turn off.

Name the other 1290s. "ERROR 1290 is also raised for some conditions unrelated to read-only mode" is right but not actionable. The ones an operator actually meets 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 are persistent rather than transient, so all land in the finding-2 shape: retried, churned, surfaced as ErrBadConn. Naming them turns "some conditions" into something a reader can match against their own symptom.

Say what a reader-endpoint deployment should do. Upstream's removed text told you not to enable this if the database is intentionally read-only; that advice no longer has anywhere to go, because Config.RejectReadOnly is gone and rejectReadOnly=false is a parse error. An application pointed at an Aurora reader endpoint that attempts a write — by bug, by a stray migration, by a metrics writer — now gets the churn-plus-ErrBadConn behaviour with no way back. That may well be the right trade for this fork's population, and the read-only-transaction exemption is a partial answer, but it doesn't cover autocommit and the README currently doesn't acknowledge the case at all. One paragraph saying "if your target is deliberately read-only, use sql.TxOptions{ReadOnly: true} or don't use this driver for that connection" would close it honestly.

---

Verified — the suite, and three attacks that dissolved

Local. go build ./..., go vet ./..., gofmt -l clean. Full suite against MySQL 8.0 over TCP: identical failing set at 902395bf and at origin/master — TestLoadData and its three subtests, from local_infile being off on my server, not from this diff. CI green on all ten matrix jobs plus the OSS check.

Attack that dissolved: the exemption being set before the statement that could trip it. mc.inReadOnlyTx = readOnly sits inside the if err == nil arm, so a failed START TRANSACTION READ ONLY leaves the flag alone, and the BEGIN itself is evaluated with the flag still false. The COMMIT of a read-only transaction is correctly still inside the exemption, since the clear happens after exec.

Attack that dissolved: the flag surviving into the next borrower of a pooled connection. Every path out of a sql.Tx runs Commit or Rollback, both of which clear unconditionally — including when exec returns an error — and the two early returns are guarded on tx.mc == nil or a closed connection, where the connection is discarded anyway. driver.ConnBeginTx is implemented, so database/sql never falls back to the path that would bypass begin(readOnly). Reachable only by mutation, which is finding 1.

Attack that dissolved: rejectReadOnly becoming a compatibility trap. Accepting true and refusing false is the asymmetry that makes an upstream DSN keep working without letting it lie, and TestRejectReadOnlyDSN covers all three cases including the non-boolean. Worth noting for whoever consumes this: deleting the Config.RejectReadOnly field is a compile break for anyone setting it in Go rather than in a DSN — loud, and there is no equivalent for the DSN path, which is why refusing false there matters.

Leak check: clean. Nothing internal in the diff or the README additions; RDS and Aurora are the vendor's own product names and the failover behaviour described is public.

Every mutation was restored from backup; the worktree is clean at 902395bf.

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

[aparajon]

🤖 Approving — making it unconditional is the right call and the read-only-transaction carve-out is correct for the right reason. Findings in the review comment above: the exemption flag's lifecycle is untested (setting it unconditionally turns the whole feature off with the suite green), and the discarded server error leaves a persistently read-only target with no trace of read-only anywhere — one mc.log closes it.

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

[morgo]

🤖 All three addressed in d987085.

1 — the lifecycle is now driven. TestReadOnlyTxLifecycle runs begin against a scripted mock server and observes the flag through Commit and Rollback. I re-ran your three survivors:

Mutation	Before	After
mc.inReadOnlyTx = readOnly → = true	survives	killed — a read-write transaction does not
Commit stops clearing	survives	killed — Commit clears it
Rollback stops clearing	survives	killed — Rollback clears it

Each subtest ends by feeding a real 1792 packet back through handleErrorPacket and requiring ErrBadConn, so it pins the consequence rather than just the field.

I also took the ResetSession hardening. Agreed it is unreachable through database/sql today — but as you say, the stuck direction is the one that silently disables the default, and that is where a pooled connection's assumptions get re-established.

2 — logged. mc.log("closing read-only connection, errno ", errno, ": ", …) before mc.Close(). One detail beyond your snippet: data[3:] still carries the #HY000 SQL-state marker, so it strips that the same way the normal path does, and the line reads as the server's own message.

3 — README. Named the four (secure_file_priv, super_read_only, innodb_read_only, --skip-grant-tables) and pointed at the new log line, so the persistent case is now identifiable rather than just accurately described.

The reader-endpoint paragraph is in, and it is worth flagging that this is not hypothetical: block/spirit is a live instance. dbconn.DBConfig.RejectReadOnly is public and defaults true, and pkg/datasync/runner.go:261 sets it false on purpose for a read-only VStream/PlanetScale source, with a comment giving your exact rationale. pkg/dbconn/conn.go:341 assigns it to cfg.RejectReadOnly, which this PR deletes — so spirit will not compile against this, and its documented opt-out has nowhere to go. The read-only-transaction exemption does not cover it, since that path is not wrapped in a ReadOnly transaction. Raised with Morgan; spirit will not be bumped past this until it is settled.

Verification: full suite at d987085 has the same 105 failures as the branch point — all from no local MySQL on :3306 — so this adds none. go vet and gofmt clean.