(kyuubi) branch branch-1.12 updated: [KYUUBI #7759] Reduce the connection limit count when session creation fails

[ch...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

pan3793 pushed a commit to branch branch-1.12
in repository https://gitbox.apache.org/repos/asf/kyuubi.git


The following commit(s) were added to refs/heads/branch-1.12 by this push:
     new 903bd5de28 [KYUUBI #7759] Reduce the connection limit count when session creation fails
903bd5de28 is described below

commit 903bd5de2882c468040cf8fbb89eab29b9c30c62
Author: Jaeho Yoo <ja...@navercorp.com>
AuthorDate: Wed Oct 28 09:49:03 2026 +0800

    [KYUUBI #7759] Reduce the connection limit count when session creation fails
    
    ### Why are the changes needed?
    
    `KyuubiSessionManager.openSession` takes a count from the session limiter before the session is
    opened, and `closeSession` is the only place that gives one back.
    
    `SessionManager.openSession` calls `createSession` outside the `try` whose `catch` calls
    `closeSession`, and `AbstractSession.normalizedConf` is a `val`, so a session whose conf fails
    validation dies in the constructor and is never registered in `handleToSession`. `closeSession`
    never runs for it, and the count is kept for good.
    
    Once as many opens have failed as `kyuubi.server.limit.connections.per.user` allows, every further
    open from that user is rejected by the limiter although the user holds no session, until the
    server restarts. `kyuubi.server.limit.connections.per.ipaddress` leaks the same way.
    
    Ordinary client configuration reaches this on a server that follows the documented
    `kyuubi.session.conf.restrict.list` guidance: `spark.sql.extensions` is in the example restrict
    list in the [session configuration docs](https://kyuubi.readthedocs.io/en/master/security/session_configuration.html),
    and a client that keeps sending it locks its own account out. We hit this in production - three
    server nodes each rejected 20 opens for the restricted key, after which every open from that user
    failed with the connection-limit message while `kyuubi_connection_opened` stayed 0, and a rolling
    restart was the only way to recover.
    
    Affects master and 1.11.1. Batch sessions are not affected: `openBatchSession` takes its count
    after the session is built and inside the `try` that closes it.
    
    close #7759
    
    ### How was this patch tested?
    
    Added `KyuubiSessionManagerLimiterSuite`, which sets `kyuubi.server.limit.connections.per.user=2`
    and a restrict list, then opens `limit + 2` sessions carrying the restricted key and asserts every
    attempt is still rejected by the restrict list rather than by the limiter.
    
        build/mvn test -pl kyuubi-server -am -Dtest=none \
            -DwildcardSuites=org.apache.kyuubi.session.KyuubiSessionManagerLimiterSuite
    
    Passes with the change; with the change reverted it fails on the third attempt with
    `"Connection limit per user reached (user: user limit: 2)" did not contain
    "spark.sql.extensions is a restrict key"`, which is the production symptom.
    
    ### Was this patch assisted by generative AI tooling?
    
    Assisted-by: Claude Opus 5
    
    Closes #7760 from Chaho12/session-limiter-leak.
    
    Closes #7759
    
    ebbc2c933 [Jaeho Yoo] [KYUUBI #7759] Reduce the connection limit count when session creation fails
    
    Authored-by: Jaeho Yoo <ja...@navercorp.com>
    Signed-off-by: Cheng Pan <ch...@apache.org>
    (cherry picked from commit a4f61c4b15b784a6037c00ecfdb291d5563d0c70)
    Signed-off-by: Cheng Pan <ch...@apache.org>
---
 .../kyuubi/session/KyuubiSessionManager.scala      | 34 +++++++++----
 .../session/KyuubiSessionManagerLimiterSuite.scala | 58 ++++++++++++++++++++++
 2 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/session/KyuubiSessionManager.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/session/KyuubiSessionManager.scala
index 344da0e71e..8c8bd25c0f 100644
--- a/kyuubi-server/src/main/scala/org/apache/kyuubi/session/KyuubiSessionManager.scala
+++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/session/KyuubiSessionManager.scala
@@ -93,16 +93,30 @@ class KyuubiSessionManager private (name: String) extends SessionManager(name) {
       ipAddress: String,
       conf: Map[String, String]): Session = {
     val userConf = this.getConf.getUserDefaults(user)
-    new KyuubiSessionImpl(
-      protocol,
-      user,
-      password,
-      ipAddress,
-      conf,
-      this,
-      userConf,
-      userConf.get(ENGINE_DO_AS_ENABLED),
-      parser)
+    try {
+      new KyuubiSessionImpl(
+        protocol,
+        user,
+        password,
+        ipAddress,
+        conf,
+        this,
+        userConf,
+        userConf.get(ENGINE_DO_AS_ENABLED),
+        parser)
+    } catch {
+      case e: Throwable =>
+        // The count openSession took is given back by closeSession alone. A session that fails
+        // here - on the session conf validation in AbstractSession.normalizedConf, for one - is
+        // never registered in handleToSession, so closeSession never runs for it and the count
+        // stays taken: once as many opens have failed as the limit allows, the user and the
+        // client address are turned away by the limiter until the server restarts. A failure
+        // after setSession is already given back by the closeSession call in
+        // SessionManager.openSession, so giving it back here alone keeps it from going back
+        // twice.
+        limiter.foreach(_.decrement(UserIpAddress(user, ipAddress)))
+        throw e
+    }
   }
 
   override def openSession(
diff --git a/kyuubi-server/src/test/scala/org/apache/kyuubi/session/KyuubiSessionManagerLimiterSuite.scala b/kyuubi-server/src/test/scala/org/apache/kyuubi/session/KyuubiSessionManagerLimiterSuite.scala
new file mode 100644
index 0000000000..a74fc29407
--- /dev/null
+++ b/kyuubi-server/src/test/scala/org/apache/kyuubi/session/KyuubiSessionManagerLimiterSuite.scala
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.session
+
+import scala.collection.JavaConverters._
+
+import org.apache.kyuubi.{KyuubiFunSuite, Utils, WithKyuubiServer}
+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.operation.TClientTestUtils
+import org.apache.kyuubi.shaded.hive.service.rpc.thrift.TOpenSessionReq
+
+class KyuubiSessionManagerLimiterSuite extends WithKyuubiServer with KyuubiFunSuite {
+
+  private val perUserLimit = 2
+  private val restrictKey = "spark.sql.extensions"
+
+  override protected val conf: KyuubiConf = KyuubiConf()
+    .set(KyuubiConf.SERVER_LIMIT_CONNECTIONS_PER_USER, perUserLimit)
+    .set(KyuubiConf.SESSION_CONF_RESTRICT_LIST, Set(restrictKey))
+
+  test("connection limit count is given back when the session fails before it is registered") {
+    // A session carrying a restricted key fails while it is created, before it is put into
+    // handleToSession, so closeSession never runs for it. Unless the count openSession took is
+    // given back there, every attempt keeps one, and the attempts past the limit are turned away
+    // by the limiter rather than by the restrict list - the user stays locked out until the
+    // server restarts.
+    (1 to perUserLimit + 2).foreach { attempt =>
+      TClientTestUtils.withThriftClient(server.frontendServices.head) { client =>
+        val req = new TOpenSessionReq()
+        req.setUsername(Utils.currentUser)
+        req.setPassword("anonymous")
+        req.setConfiguration(Map(restrictKey -> "org.apache.kyuubi.NoopExtensions").asJava)
+        val errorMessage = client.OpenSession(req).getStatus.getErrorMessage
+        assert(
+          errorMessage.contains(s"$restrictKey is a restrict key"),
+          s"attempt $attempt: $errorMessage")
+        assert(
+          !errorMessage.contains("Connection limit per user reached"),
+          s"attempt $attempt: the connection limit count was not given back")
+      }
+    }
+  }
+}