Export ErrBadConnNoWrite so Close() errors are identifiable

[htoyoda18]

Description

Conn.Close() can return errBadConnNoWrite as-is: when the peer has already
closed the connection before COM_QUIT could be sent, writePacket() returns
this sentinel, and Close() does not run it through markBadConn() (Close is
not a retryable operation, so converting it to driver.ErrBadConn would be
meaningless there).

Being unexported, this value cannot be identified by callers except by
matching the error string "bad connection", which is fragile since it
depends on an internal implementation detail. This is a real-world problem
when apps want to distinguish this specific, benign, unpreventable condition
(closing an already-dead idle pooled connection, e.g. during graceful
shutdown) from other, more meaningful close errors.

This PR simply exports errBadConnNoWrite as ErrBadConnNoWrite. It is a
pure rename: the error value, its message, and every existing code path
(including markBadConn's conversion to driver.ErrBadConn for the
retry-safe call sites: Begin/BeginTx/Exec/ExecContext/Prepare/
PrepareContext/Query/QueryContext) are unchanged. Callers can now write
errors.Is(err, mysql.ErrBadConnNoWrite) instead of matching on the error
string.

Related history

• Introduced unexported in fix driver.ErrBadConn behavior, stop repeated queries #302.
• How can I use errBadConnNoWrite? #1303 shows the assumption at the time: this error is "internal" and
  should never reach a caller un-converted, because markBadConn always
  turns it into driver.ErrBadConn first. Close() is the one path where
  that assumption doesn't hold, since it intentionally skips markBadConn.
• Improve ErrBadConn #1582/remove errBadConnNoWrite and markBadConn #1583 proposed removing this sentinel (and markBadConn) entirely.
  That PR was split into Reduce "busy buffer" logs #1641/stmt.Close() returns nil when double close #1642, which trimmed the other use sites
  (busy-buffer cases) while intentionally keeping the single remaining one
  in writePacket(), precisely because removing it entirely would break
  retry-safety signaling for Query/Exec when the first write fails with
  zero bytes sent. This PR does not touch that retained logic at all — it
  only makes the existing sentinel usable via errors.Is by external
  callers.

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary
• Added myself / the copyright holder to the AUTHORS file

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 24813054-323f-4c27-a5fc-a74b152e24f6

📥 Commits

Reviewing files that changed from the base of the PR and between 1d9c421 and 17cf7cf.

📒 Files selected for processing (5)

• AUTHORS
• connection.go
• connection_test.go
• errors.go
• packets.go

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

Walkthrough

The change exports ErrBadConnNoWrite, updates packet and connection error handling to use it, documents its Conn.Close behavior, and adds a test for error identification. The AUTHORS list adds Hiroto Toyoda.

Changes

Bad connection sentinel

Layer / File(s)	Summary
Export sentinel and update write handling errors.go, packets.go, AUTHORS	The package exports ErrBadConnNoWrite. Its documentation covers Conn.Close, and writePacket returns the exported sentinel. The AUTHORS list adds Hiroto Toyoda.
Propagate and validate close errors connection.go, connection_test.go	markBadConn checks ErrBadConnNoWrite. TestCloseErrBadConnNoWrite verifies that Conn.Close returns an error matching the sentinel.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 17cf7

This PR only exposes the existing bad-connection sentinel without changing connection behavior or error handling, so no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely states the main change: exporting ErrBadConnNoWrite so callers can identify Close() errors.
Description check	✅ Passed	The description directly explains the exported sentinel, the Conn.Close() behavior, the errors.Is usage, and the unchanged retry behavior.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[methane]

errBadConnNoWrite is an error I strongly dislike. I once tried to remove it, but gave up partway through. I may make another attempt to remove it in the future.

This error is intended to be something users neither need to use nor even need to know about. Why do you want to detect it with errors.Is?
Is your use case common enough to justify making this part of the public API and maintaining its backward compatibility indefinitely?

[htoyoda18]

@methane
Fair question. Our use case: during graceful shutdown, sql.DB.Close() logs a continuous stream of "bad connection" errors. This is exactly the Close() → writeCommandPacket(comQuit) → errBadConnNoWrite path — the peer already closed the idle socket, the error is not actionable, and local resources are freed either way. We wanted to log this specific case at a lower severity, but today the only way to detect it is strings.Contains(err.Error(), "bad connection").

I don't have evidence this is common beyond our case, other than #1303 where someone else also wanted to detect it and was told it's internal — so I can't claim strong demand for a permanent public API commitment.

Given that, would you prefer a narrower fix instead: since Close() already discards local resources unconditionally and this "0 bytes written, peer already gone" case is never actionable, could Close() just swallow it and return nil? That solves the underlying noisy-log problem without adding public API surface. Happy to send that instead, or drop this PR if you'd rather handle it differently later.

[methane]

Why are you shutting down the database before gracefully shutting down the application? I think this error is useful because it reveals that the shutdown order may be unsafe.

[htoyoda18]

Not a shutdown-ordering issue on our side. This happens on idle connections that the peer (MySQL server's wait_timeout, or a proxy/LB in front of it) had already closed independently, before Close() ever runs. sql.DB can't detect a dead idle connection until it tries to use it — here, via Close()'s COM_QUIT — so this can fire whenever the pool closes a connection the server side already dropped, whether during graceful shutdown or routine idle-connection eviction (ConnMaxIdleTime). It's a routine TCP-level race, not evidence our app closes the DB too early.

[methane]

ja: その場合、たまたまエラーが発生したのがCloseだったから問題にならなかっただけで危険な構成であること自体には変わりありません。あなたのアプリケーションは長時間のidleのあと、たまたまサーバーから接続がcloseされるのと同時にクエリを実行した場合にリトライ不可能なエラーを起こす可能性があります。SetConnMaxLifetime などを使ってサーバー側のタイムアウトよりも明確に短いタイムアウトをクライアントに持たせるべきです。これはMySQLだけでなくHTTPでも同じです。サーバーから接続をcloseすることは危険で、クライアントはサーバーから接続をcloseされるよりも早いidle timeoutやlifetimeを持つべきです。

en:
In that case, the fact that the error happened to occur during Close merely prevented it from causing a problem this time; the configuration itself is still unsafe.

After a long idle period, your application could issue a query at exactly the same time the server happens to close the connection, resulting in a non-retryable error. You should configure the client with a timeout that is clearly shorter than the server-side timeout, for example by using SetConnMaxLifetime.

This is not specific to MySQL; the same principle applies to HTTP as well. Having the server close a connection is inherently risky. The client should use an idle timeout or connection lifetime that expires before the server closes the connection.