fix(frontend): fence session reset from active requests - #26497
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking: the new request/lifecycle fence is bypassed by the legacy MigrateConnFrom actions.
In RoutineManager.MigrateConnectionFromWithContext, MigrateConnFromSkipUserLevelLockRelease reads the current session user-lock state before routine.migrateConnectionFromWithContext acquires mc. A reachable ordering is:
- A SQL request owns
requestInProgressbut has not acquired its user lock yet. - The legacy migration RPC checks
UserLevelLocksForMigrationand observes no locks, then blocks inbeginOperationWithContext. - The SQL request completes
GET_LOCKand releases the request lease. - Migration resumes, sets
userLevelLocksMigrated = true, and succeeds without rechecking the lock state.
That defeats the safety check and can let a connection migrate with a held lock; close then takes the migrated-lock discard path instead of releasing it. The MigrateConnFromEnableUserLevelLockRelease branch is also completely outside lifecycle admission and can mutate an old session while reset/close owns it. These compatibility actions remain reachable during rolling upgrades.
Please move both action-specific session reads/mutations behind the same lifecycle admission (with the check performed after admission), and add request-first/reset-first interleaving tests for these two actions.
|
@XuPeng-SH Addressed on Both legacy
Regression coverage calls the production Validation:
The PR remains Ready for Review. New exact-head CI is pending. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
The previous lifecycle-fence blocker is resolved on e70c5a45ca7617ab668d09150184814f98218078.
Both legacy MigrateConnFrom actions now acquire the same request/lifecycle admission before reading or mutating the session. In particular, the skip action rechecks user-level locks on the admitted current session, and the enable action only changes that same session generation. The new request-first and reset-first tests cover both actions and would expose the stale/pre-admission access from the prior head.
I also rechecked the surrounding ownership paths: reset remains fail-fast while a request owns the session; migration waits with caller cancellation; close seals admission and cancels active work while preserving its existing non-waiting request-close behavior. I found no remaining correctness or lifecycle blocker in this diff.
Local exact-head validation passed:
- all newly added focused tests
- the same focused set under
-race - full
pkg/frontend - direct consumer
pkg/cnservice - compile-only checks and
go vetfor both packages git diff --check
Approve.
Merge Queue Status
This pull request spent 1 hour 28 minutes 23 seconds in the queue, including 45 minutes 57 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #20022
What this PR does / why we need it:
Proxy can observe a terminal MySQL response and start
ResetSessionwhile theoriginal CN
RoutineManager.Handleris still completing response accountingand defers. The old lifecycle controller serialized reset/migration/close, but
did not serialize ordinary requests, so reset could close the old Session,
process, and transaction handler while Handler still owned them.
This change:
ResetSessionfail fast with the existing busy error while a requestis active, so Proxy discards that backend and reconnects;
existing cancel-without-request-wait behavior;
the routine;
ExecRequestpanic recovery safely handle a nil transaction handler.Proxy cache/generation/in-flight production logic is unchanged.
Regression evidence
4959b7cdd853923f60b168d9ef69e242a1cadc37TestRoutineManagerResetSessionRejectsRequestAfterResponseWritefailed:reset returned
nilafter the terminal response write while Handler wasdeliberately blocked before returning.
af17db6b3db4b19cf5b4a604395574ce21a4a602Session/proc/TxnHandler and SessionManager entry, then succeeds once after
Handler exits and serves the next
COM_PING;rejected Proxy cache entry, and nil-TxnHandler recovery paths pass.
Validation
.agents/skills/mo-dev/scripts/mo-cgo-test ./pkg/frontend -count=1: PASS.agents/skills/mo-dev/scripts/mo-cgo-test ./pkg/proxy -count=1: PASSpkg/frontendandpkg/proxywith-race -count=1: PASS-race -count=100: PASSgo build -mod=readonly ./pkg/frontend ./pkg/proxy: PASSgo vet -mod=readonly ./pkg/frontend ./pkg/proxy: PASSgit diff --check: PASSPASS review=PASS validation=PENDINGProxy Compose BVT, pessimistic launch BVT, and coverage.
Local deployment evidence
The local topology was launched with the real Proxy path
(
mo-service -with-proxy), 2 CNs, and true new client connections.isolated 5 s context deadline; all Proxy/CN probes passed;
0 failures, all 36 endpoint probes passed, and backend sessions converged
from 119 to 17/18 rather than growing with total connections;
wait active, orcreateTxnOpUnsafewas observed in these valid runs.A malformed first result was observed in two cold-start runs immediately after
ports opened. A later diagnostic run without s2c batching passed 5,984 queries,
but it ran after the cluster was fully warm and therefore does not prove a
buffering defect. This cold-start observation is tracked as unresolved
acceptance evidence and is not used to justify an unrelated Proxy code change.
The complete 10-minute local soak is still pending: the initial 100
connections/s setup exhausts the macOS loopback ephemeral-port range, while
the later 50 connections/s rerun was interrupted by the local disk safety
threshold. TKE validation remains pending.
QA / BVT
independent true short connections. Deterministic Frontend/Proxy concurrency
tests provide the in-repo regression coverage.
connections for 10 minutes at a host-safe connection rate;
no panic/restart/continuous login failure, and bounded backend sessions.
phase/testingforAriznawlllto runthe deployment matrix; link
matrixorigin/mo-nightly-regression#835for therecurring short-connection coverage.
Current-head CR follow-up (2026-08-05)
Head
e70c5a45ca7617ab668d09150184814f98218078also routes both rolling-upgradeMigrateConnFromcompatibility actions through the Routine lifecycle admissionbefore reading or mutating Session user-lock state. Skip now checks the current
admitted Session after a request/reset owner exits; Enable mutates only that
admitted Session generation.
against
1bc7d0021fb1fbcbd00ce4af3b703711a90d9e38.T=0.21s,B=30s,N=100PASS;reset-first
T=0.22s,B=30s,N=100PASS.pkg/frontend: list/build/vet/test and full-package race PASS.pkg/cnservice: list/build/vet/test PASS.git diff --check: PASS.PASS review=PASS validation=PENDINGat exact heade70c5a45ca7617ab668d09150184814f98218078.MigrateConnFromaction values areselected by old Proxy versions and cannot be invoked through SQL BVT. The
deterministic
RoutineManager.MigrateConnectionFromWithContextinterleavingtests exercise the production RPC entry and assert the final lock/Session
state. New exact-head CI is pending.