From c700d964645f8a102a96bf43aeb76efecfe00cd5 Mon Sep 17 00:00:00 2001
From: M Bussonnier <bussonniermatthias@gmail.com>
Date: Tue, 17 Dec 2024 09:54:47 +0100
Subject: [PATCH 1/3] Better error message when starting kernel for session.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This catches error when starting a kernel for a session (for example if
one switches the kernel for an existing notebook), and propagate the
error. Otherwise the frontend will just show "Error: uncaught
exception".

In addition this setup ruff G001 – do not use string formatting in
logging, see some of the reasons to not do that:

https://docs.astral.sh/ruff/rules/logging-string-format/

And fixes the few instances where jupyter-server does it.
---
 examples/simple/simple_ext1/handlers.py      |  3 +-
 jupyter_server/gateway/managers.py           | 14 +++++----
 jupyter_server/services/contents/handlers.py |  7 ++---
 jupyter_server/services/sessions/handlers.py | 32 +++++++++++++++-----
 pyproject.toml                               |  1 +
 5 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/examples/simple/simple_ext1/handlers.py b/examples/simple/simple_ext1/handlers.py
index 72f54a8bfd..6ac99f64a4 100644
--- a/examples/simple/simple_ext1/handlers.py
+++ b/examples/simple/simple_ext1/handlers.py
@@ -18,7 +18,8 @@ def get(self):
         self.log.info(f"Extension Name in {self.name} Default Handler: {self.name}")
         # A method for getting the url to static files (prefixed with /static/<name>).
         self.log.info(
-            "Static URL for / in simple_ext1 Default Handler: {}".format(self.static_url(path="/"))
+            "Static URL for / in simple_ext1 Default Handler: %s",
+            self.static_url(path="/"),
         )
         self.write("<h1>Hello Simple 1 - I am the default...</h1>")
         self.write(f"Config in {self.name} Default Handler: {self.config}")
diff --git a/jupyter_server/gateway/managers.py b/jupyter_server/gateway/managers.py
index 0ac47f8f57..d7b0997ca4 100644
--- a/jupyter_server/gateway/managers.py
+++ b/jupyter_server/gateway/managers.py
@@ -632,9 +632,10 @@ async def get_msg(self, *args: Any, **kwargs: Any) -> dict[str, Any]:
         timeout = kwargs.get("timeout", 1)
         msg = await self._async_get(timeout=timeout)
         self.log.debug(
-            "Received message on channel: {}, msg_id: {}, msg_type: {}".format(
-                self.channel_name, msg["msg_id"], msg["msg_type"] if msg else "null"
-            )
+            "Received message on channel: {}, msg_id: {}, msg_type: {}",
+            self.channel_name,
+            msg["msg_id"],
+            msg["msg_type"] if msg else "null",
         )
         self.task_done()
         return cast("dict[str, Any]", msg)
@@ -643,9 +644,10 @@ def send(self, msg: dict[str, Any]) -> None:
         """Send a message to the queue."""
         message = json.dumps(msg, default=ChannelQueue.serialize_datetime).replace("</", "<\\/")
         self.log.debug(
-            "Sending message on channel: {}, msg_id: {}, msg_type: {}".format(
-                self.channel_name, msg["msg_id"], msg["msg_type"] if msg else "null"
-            )
+            "Sending message on channel: %s, msg_id: %s, msg_type: %s",
+            self.channel_name,
+            msg["msg_id"],
+            msg["msg_type"] if msg else "null",
         )
         self.channel_socket.send(message)
 
diff --git a/jupyter_server/services/contents/handlers.py b/jupyter_server/services/contents/handlers.py
index 13e987809b..5836dd5dda 100644
--- a/jupyter_server/services/contents/handlers.py
+++ b/jupyter_server/services/contents/handlers.py
@@ -210,10 +210,9 @@ async def patch(self, path=""):
     async def _copy(self, copy_from, copy_to=None):
         """Copy a file, optionally specifying a target directory."""
         self.log.info(
-            "Copying {copy_from} to {copy_to}".format(
-                copy_from=copy_from,
-                copy_to=copy_to or "",
-            )
+            "Copying %r to %r",
+            copy_from,
+            copy_to or "",
         )
         model = await ensure_async(self.contents_manager.copy(copy_from, copy_to))
         self.set_status(201)
diff --git a/jupyter_server/services/sessions/handlers.py b/jupyter_server/services/sessions/handlers.py
index 3e013c0335..a71f16d421 100644
--- a/jupyter_server/services/sessions/handlers.py
+++ b/jupyter_server/services/sessions/handlers.py
@@ -161,14 +161,30 @@ async def patch(self, session_id):
                 changes["kernel_id"] = kernel_id
             elif model["kernel"].get("name") is not None:
                 kernel_name = model["kernel"]["name"]
-                kernel_id = await sm.start_kernel_for_session(
-                    session_id,
-                    kernel_name=kernel_name,
-                    name=before["name"],
-                    path=before["path"],
-                    type=before["type"],
-                )
-                changes["kernel_id"] = kernel_id
+
+                try:
+                    kernel_id = await sm.start_kernel_for_session(
+                        session_id,
+                        kernel_name=kernel_name,
+                        name=before["name"],
+                        path=before["path"],
+                        type=before["type"],
+                    )
+                    changes["kernel_id"] = kernel_id
+                except Exception as e:
+                    # the error message may contain sensitive information, so we want to
+                    # be careful with it, thus we only give the short repr of the exception
+                    # and the full traceback.
+                    # this should be fine as we are exposing here the same info as when we strt a new kernel
+                    msg = "The '%s' kernel could not be started: %s" % (
+                        kernel_name,
+                        repr(str(e)),
+                    )
+                    status_msg = "Error starting kernel %s" % kernel_name
+                    self.log.error("Error starting kernel: %s", kernel_name)
+                    self.set_status(501)
+                    self.finish(json.dumps({"message": msg, "short_message": status_msg}))
+                    return
 
         await sm.update_session(session_id, **changes)
         s_model = await sm.get_session(session_id=session_id)
diff --git a/pyproject.toml b/pyproject.toml
index 9bfcc74eef..31b5d24f38 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -142,6 +142,7 @@ extend-select = [
   "B",  # flake8-bugbear
   "I",  # isort
   "UP",  # pyupgrade
+  "G001", # no % or f formatting in logs, prevents sttructured logging
 ]
 unfixable = [
   # Don't touch print statements

From 5266697806cb244b45595493dead7d43bc353369 Mon Sep 17 00:00:00 2001
From: M Bussonnier <bussonniermatthias@gmail.com>
Date: Tue, 17 Dec 2024 02:38:29 -0800
Subject: [PATCH 2/3] Apply suggestions from code review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
---
 jupyter_server/gateway/managers.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/jupyter_server/gateway/managers.py b/jupyter_server/gateway/managers.py
index d7b0997ca4..daa6f99213 100644
--- a/jupyter_server/gateway/managers.py
+++ b/jupyter_server/gateway/managers.py
@@ -632,7 +632,7 @@ async def get_msg(self, *args: Any, **kwargs: Any) -> dict[str, Any]:
         timeout = kwargs.get("timeout", 1)
         msg = await self._async_get(timeout=timeout)
         self.log.debug(
-            "Received message on channel: {}, msg_id: {}, msg_type: {}",
+            "Received message on channel: %s, msg_id: %s, msg_type: %s",
             self.channel_name,
             msg["msg_id"],
             msg["msg_type"] if msg else "null",

From fa14577c9f5b438e9687f98c9fd5a8c7d10b4331 Mon Sep 17 00:00:00 2001
From: M Bussonnier <bussonniermatthias@gmail.com>
Date: Tue, 17 Dec 2024 03:54:32 -0800
Subject: [PATCH 3/3] Update jupyter_server/services/sessions/handlers.py

Co-authored-by: David Brochart <david.brochart@gmail.com>
---
 jupyter_server/services/sessions/handlers.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/jupyter_server/services/sessions/handlers.py b/jupyter_server/services/sessions/handlers.py
index a71f16d421..2d62c21b5a 100644
--- a/jupyter_server/services/sessions/handlers.py
+++ b/jupyter_server/services/sessions/handlers.py
@@ -175,7 +175,7 @@ async def patch(self, session_id):
                     # the error message may contain sensitive information, so we want to
                     # be careful with it, thus we only give the short repr of the exception
                     # and the full traceback.
-                    # this should be fine as we are exposing here the same info as when we strt a new kernel
+                    # this should be fine as we are exposing here the same info as when we start a new kernel
                     msg = "The '%s' kernel could not be started: %s" % (
                         kernel_name,
                         repr(str(e)),