Skip to content

[BugFix] Fix graceful exit handling in MySQLReadListener to preserve pre-query SQL connections#67917

Merged
stdpain merged 5 commits intomainfrom
copilot/fix-improper-graceful-exit
Jan 16, 2026
Merged

[BugFix] Fix graceful exit handling in MySQLReadListener to preserve pre-query SQL connections#67917
stdpain merged 5 commits intomainfrom
copilot/fix-improper-graceful-exit

Conversation

Copy link
Contributor

Copilot AI commented Jan 14, 2026

Why I'm doing:

During graceful exit (leader transfer), MySQLReadListener was terminating all connections after query execution, breaking JDBC clients that send initialization queries like select @@query_timeout before actual user queries.

What I'm doing:

Added isTerminated() method with graceful exit awareness:

  • Fast-path returns true if connection already terminated
  • Returns false if no statements executed yet (null executor)
  • During graceful exit, only terminates connections after non-pre-query SQLs
  • Null-safe: handles executor and parsed statement nullability

Refactored handleRequest() to use isTerminated() instead of direct terminated flag check.

private boolean isTerminated() {
    if (terminated) {
        return true;
    }
    StmtExecutor executor = connectProcessor.getExecutor();
    if (executor == null) {
        return false;
    }
    final StatementBase lastStmt = executor.getParsedStmt();
    return GracefulExitFlag.isGracefulExit() && !SqlUtils.isPreQuerySQL(lastStmt);
}

Added unit tests covering terminated flag, graceful exit scenarios, null safety, and pre-query SQL types (SET statements, connection_id(), query_timeout).

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
    • This pr needs auto generate documentation
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 4.1
    • 4.0
    • 3.5
    • 3.4

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • mirror.iscas.ac.cn
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.12/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.12/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.12 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.12/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/starrocks/starrocks/fe/fe-core org.codehaus.plexus.classworlds.launcher.Launcher test-compile -Dtest=MySQLReadListenerTest (dns block)
  • repository.cloudera.com
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.12/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.12/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.12 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.12/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/starrocks/starrocks/fe/fe-core org.codehaus.plexus.classworlds.launcher.Launcher test-compile -Dtest=MySQLReadListenerTest (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

In the StarRocks/starrocks repository, a bug exists in the MySQLReadListener.java file that causes improper handling of graceful exits. The problem lies in the method determining termination, which does not adequately check for the combination of graceful exit flags and SQL types. The current implementation directly references the terminated variable without considering additional logical conditions proposed in the isTerminated method introduced by your patch.

To resolve this issue, enhance the MySQLReadListener class with an improved termination condition and refactor the handleRequest method to use this. These changes will:

  1. Introduce a new isTerminated method to encapsulate termination logic.
  2. Ensure termination considers both the GracefulExitFlag and pre-query SQL status appropriately.

The patch includes the following key update:

  • Adds a new method isTerminated() to handle graceful exit flag checks combined with the parsed statement SQL type.
  • Refactors the handleRequest method to utilize isTerminated() instead of directly checking the terminated variable.

Here was the provided patch:

@@ -15,12 +15,15 @@
 package com.starrocks.mysql.nio;
 
 import com.starrocks.common.Config;
+import com.starrocks.common.util.SqlUtils;
 import com.starrocks.mysql.MysqlPackageDecoder;
 import com.starrocks.mysql.RequestPackage;
 import com.starrocks.mysql.ssl.SSLDecoder;
 import com.starrocks.qe.ConnectContext;
 import com.starrocks.qe.ConnectProcessor;
 import com.starrocks.rpc.RpcException;
+import com.starrocks.server.GracefulExitFlag;
+import com.starrocks.sql.ast.StatementBase;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.xnio.ChannelListener;
@@ -114,11 +117,19 @@ public class MySQLReadListener implements ChannelListener<ConduitStreamSourceCha
         ctx.cleanup();
     }

+    private boolean isTerminated() {
+        if (terminated) {
+            return true;
+        }
+        final StatementBase lastStmt = connectProcessor.getExecutor().getParsedStmt();
+        return GracefulExitFlag.isGracefulExit() && !SqlUtils.isPreQuerySQL(lastStmt);
+    }
+
     private synchronized void handleRequest(RequestPackage req) {
         ctx.setThreadLocalInfo();
         try {
             connectProcessor.processOnce(req);
-            if (ctx.isKilled() || terminated) {
+            if (ctx.isKilled() || isTerminated()) {
                 ctx.stopAcceptQuery();
                 ctx.cleanup();
             }

Please review the details provided and test the implementation for correctness, especially focusing on ensuring all edge cases are well-covered.

This pull request was created from Copilot chat.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.


Note

Improves connection termination behavior during leader transfer to avoid dropping JDBC initialization queries.

  • Adds isTerminated() that returns true if terminated, false if executor is null, and during GracefulExitFlag.isGracefulExit() only terminates when last stmt is not pre-query via SqlUtils.isPreQuerySQL
  • Updates handleRequest() to use isTerminated() instead of checking terminated directly
  • Introduces unit tests for terminated flag, graceful exit with pre-query vs non-pre-query SQL, null executor/parsed stmt, and common pre-query cases (select @@query_timeout, set query_timeout=..., select connection_id())

Written by Cursor Bugbot for commit d484e78. This will update automatically on new commits. Configure here.

Copilot AI and others added 3 commits January 14, 2026 10:40
Co-authored-by: stdpain <34912776+stdpain@users.noreply.github.com>
Co-authored-by: stdpain <34912776+stdpain@users.noreply.github.com>
Co-authored-by: stdpain <34912776+stdpain@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix improper handling of graceful exits in MySQLReadListener [BugFix] Fix graceful exit handling in MySQLReadListener to preserve pre-query SQL connections Jan 14, 2026
Copilot AI requested a review from stdpain January 14, 2026 10:47
@stdpain stdpain marked this pull request as ready for review January 14, 2026 10:53
@stdpain stdpain requested a review from a team as a code owner January 14, 2026 10:53
@alvin-celerdata
Copy link
Contributor

@cursor review

@github-actions github-actions bot removed the 4.1 label Jan 14, 2026
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no bugs!

gengjun-git
gengjun-git previously approved these changes Jan 15, 2026
@github-actions github-actions bot removed the 3.5 label Jan 15, 2026
@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

[Java-Extensions Incremental Coverage Report]

pass : 0 / 0 (0%)

@github-actions
Copy link
Contributor

[FE Incremental Coverage Report]

pass : 8 / 8 (100.00%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/mysql/nio/MySQLReadListener.java 8 8 100.00% []

@github-actions
Copy link
Contributor

[BE Incremental Coverage Report]

pass : 0 / 0 (0%)

@stdpain stdpain merged commit 0f74cc8 into main Jan 16, 2026
58 of 59 checks passed
@stdpain stdpain deleted the copilot/fix-improper-graceful-exit branch January 16, 2026 09:26
@github-actions
Copy link
Contributor

@Mergifyio backport branch-4.0

@github-actions
Copy link
Contributor

@Mergifyio backport branch-4.1

@mergify
Copy link
Contributor

mergify bot commented Jan 16, 2026

backport branch-4.0

✅ Backports have been created

Details

@mergify
Copy link
Contributor

mergify bot commented Jan 16, 2026

backport branch-4.1

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Jan 16, 2026
…pre-query SQL connections (#67917)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: stdpain <34912776+stdpain@users.noreply.github.com>
Co-authored-by: stdpain <drfeng08@gmail.com>
(cherry picked from commit 0f74cc8)
mergify bot pushed a commit that referenced this pull request Jan 16, 2026
…pre-query SQL connections (#67917)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: stdpain <34912776+stdpain@users.noreply.github.com>
Co-authored-by: stdpain <drfeng08@gmail.com>
(cherry picked from commit 0f74cc8)
wanpengfei-git pushed a commit that referenced this pull request Jan 17, 2026
…pre-query SQL connections (backport #67917) (#68037)

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: stdpain <34912776+stdpain@users.noreply.github.com>
Co-authored-by: stdpain <drfeng08@gmail.com>
wanpengfei-git pushed a commit that referenced this pull request Jan 17, 2026
…pre-query SQL connections (backport #67917) (#68038)

Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: stdpain <34912776+stdpain@users.noreply.github.com>
Co-authored-by: stdpain <drfeng08@gmail.com>
farhad-celo pushed a commit to farhad-celo/starrocks that referenced this pull request Jan 20, 2026
…pre-query SQL connections (StarRocks#67917)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: stdpain <34912776+stdpain@users.noreply.github.com>
Co-authored-by: stdpain <drfeng08@gmail.com>
Signed-off-by: Farhad Shahmohammadi <f.shahmohammadi@celonis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants