fix(storage): narrow the pinned-connection discard escape

[Kiran01bm]

Summary

Follow-up to #950 review feedback. The exported rebindConn.Raw(func(driverConn any) error) gave any caller open-ended access to the underlying driver connection — an unnecessary binder-bypass surface on a wrapper whose whole job is to guarantee every statement rebinds exactly once. This narrows the wrapper so lifecycle control cannot execute SQL.

What

• Replace exported rebindConn.Raw with unexported discardBadConn(): it injects driver.ErrBadConn internally so the pool destroys the session, swallows the sentinel as the success signal, and exposes no driver connection to callers.
• Rename raw() to lockerConn() so the sole sanctioned binder bypass reads as what it is: the advisory-lock boundary that must run engine-native placeholders on the pinned session.
• closeApplyTargetLockConn treats sql.ErrConnDone as success, since a preceding discard retires the session immediately.
• Tests: discardBadConn destroys the driver connection, executes no SQL, never touches the binder, and a later Close reports sql.ErrConnDone; transaction isolation levels (RepeatableRead / ReadCommitted) survive the wrapper types to the driver while still rebinding exactly once.

Why

After this change the wrapper's surface tells the full story: lockerConn() is the only way SQL can bypass the binder, and it exists solely for the advisory locker. Discarding a suspect session is deliberately incapable of running statements, so no future caller can quietly route store SQL around the rebind guarantee.

Before / after

Before:
  rebindConn.Raw(f)  ──▶ f(driverConn)   any caller, arbitrary driver access
  rebindConn.raw()   ──▶ *sql.Conn       advisory locker (binder bypass)

After:
  rebindConn.discardBadConn() ──▶ retire session only, no SQL possible
  rebindConn.lockerConn()     ──▶ *sql.Conn  advisory locker (sole binder bypass)

[Copilot]

Pull request overview

This PR tightens the rebindConn wrapper API in pkg/storage/internal/sqlstore to remove the exported driver-connection escape hatch, while preserving the one explicit binder bypass needed for advisory-locking on a pinned session. It reduces the surface area for bypassing placeholder rebinding and makes “retire this session” impossible to misuse for executing SQL outside the binder boundary.

Changes:

• Replace exported rebindConn.Raw(...) with an internal discardBadConn() that retires the pinned session via driver.ErrBadConn without exposing the underlying driver connection.
• Rename the advisory-lock escape from raw() to lockerConn() to make the binder-bypass intent explicit and narrowly scoped.
• Update lock-connection cleanup to treat sql.ErrConnDone as a successful close after a discard, and add/extend unit tests around discard behavior and Tx isolation propagation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/storage/internal/sqlstore/db.go	Removes exported driver-conn access; adds discardBadConn() and renames the lock-only binder bypass to lockerConn().
pkg/storage/internal/sqlstore/db_test.go	Adds coverage ensuring discard retires sessions without SQL/binder access and verifies Tx isolation options reach the driver through wrappers.
pkg/storage/internal/sqlstore/apply_operations.go	Switches advisory-lock calls from raw() to lockerConn() for the named-lock boundary.
pkg/storage/internal/sqlstore/apply_operations_test.go	Updates tests to use lockerConn() for named-lock Acquire/Release.
pkg/storage/internal/sqlstore/applies.go	Routes discard through discardBadConn(), switches named-lock usage to lockerConn(), and treats sql.ErrConnDone as a successful close after discard.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 2cce1fc.

Verdict: correct, and exactly the right shape for this wrapper — no findings. The escape surface now says what it means: lockerConn() is the sole binder bypass and reads as one, and lifecycle control is structurally incapable of executing SQL. One CI note: the Integration Tests job on this head was cancelled by the 10-minute job timeout with every package passing in the log (the runner was just slow — pkg/tern alone took 107s); I re-ran the job rather than treating it as a real failure, and it passed in 7m13s — CI is now fully green.

Findings

None.

Verified (tried to break, couldn't)

discardBadConn preserves the old discard's exact semantics (the injected driver.ErrBadConn sentinel is swallowed as the success signal, anything else still surfaces to the caller's warn log, and a double discard warns with sql.ErrConnDone rather than silently passing); the ErrConnDone tolerance in closeApplyTargetLockConn matches a real sequence — releaseApplyTargetLockConn discards on release-failure/not-held and then unconditionally closes, so before this change every discard path logged a spurious "failed to close" warning, and after it only genuine close failures warn; the surface narrowing is complete (the only remaining Conn.Raw call in the package is inside discardBadConn itself, and lockerConn()'s callers are exactly the two advisory-lock boundaries plus their test); the new isolation-level test closes a real gap — the previous fake driver silently ignored TxOptions via the legacy Begin() path, so nothing proved LevelRepeatableRead/LevelReadCommitted survived the wrapper types until now — and the discard test pins all three properties (driver connection destroyed, no SQL executed, binder untouched); go build ./..., go test -race ./pkg/storage/..., and the sqlstore integration suite (testcontainers) all pass locally at head.

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

[aparajon]

🤖 Approving on Armand's behalf after the adversarial correctness review above (no findings). This stamp was left by Claude Code (claude-fable-5).