Fix fallback for errors in logging

src/runner/PlutoRunner/src/PlutoRunner.jl

@@ -76,4 +76,6 @@ include("./io/logging.jl")
 include("./io/stdout.jl")
 include("./precompile.jl")
 
+__init__() = redirect_original_stderr(stderr)
+
 end

src/runner/PlutoRunner/src/evaluation/deleting globals.jl

@@ -89,7 +89,7 @@ function move_vars(
                 catch ex
                     if !(ex isa UndefVarError)
                         @warn "Failed to move variable $(symbol) to new workspace:"
-                        showerror(original_stderr, ex, stacktrace(catch_backtrace()))
+                        showerror(original_stderr[], ex, stacktrace(catch_backtrace()))
                     end
                 end
             end
@@ -198,7 +198,7 @@ function try_delete_toplevel_methods(workspace::Module, (cell_id, name_parts)::T
             (val isa Function) && delete_toplevel_methods(val, cell_id)
         catch ex
             @warn "Failed to delete methods for $(name_parts)"
-            showerror(original_stderr, ex, stacktrace(catch_backtrace()))
+            showerror(original_stderr[], ex, stacktrace(catch_backtrace()))
             false
         end
     catch

src/runner/PlutoRunner/src/io/logging.jl

@@ -1,7 +1,21 @@
 import Logging
 
-const original_stdout = stdout
-const original_stderr = stderr
+const original_stderr = Ref{IO}()
+
+function redirect_original_stderr(io)
+    original_stderr[] = io
+    return nothing
+end
+
+function redirect_original_stderr(f::Function, io)
+    old_stderr = original_stderr[]
+    redirect_original_stderr(io)
+    try
+        return f()
+    finally
+        redirect_original_stderr(old_stderr)
+    end
+end
 
 const old_logger = Ref{Union{Logging.AbstractLogger,Nothing}}(nothing)
 
@@ -113,8 +127,9 @@ function Logging.handle_message(pl::PlutoCellLogger, level, msg, _module, group,
         yield()
 
     catch e
-        println(original_stderr, "Failed to relay log from PlutoRunner")
-        showerror(original_stderr, e, stacktrace(catch_backtrace()))
+        Logging.with_logger(Logging.ConsoleLogger(original_stderr[])) do
+            @error "Failed to relay log from PlutoRunner" exception=(e, catch_backtrace())
+        end
 
         nothing
     end

test/Logging.jl

@@ -66,6 +66,14 @@ using Pluto.WorkspaceManager: poll
         "show(stdout, collect(1:500))", # 24
         "show(stdout, \"text/plain\", collect(1:500))", # 25
         "display(collect(1:500))", # 26
+
+        "struct StructThatErrorsOnPrinting end", # 27
+        """
+        Base.print(::IO, ::StructThatErrorsOnPrinting) = error("Can't print this")
+        """, # 28
+        """
+        @info "" _id=StructThatErrorsOnPrinting()
+        """, # 29
     ]))
 
     @testset "Stdout" begin
@@ -201,4 +209,57 @@ using Pluto.WorkspaceManager: poll
     end
 
     cleanup(🍭, notebook)
+
+    @testset "Logging error fallback" begin
+        # This testset needs a separate server session to capture the process stderr (which
+        # is different from the notebook stderr)
+        🍍 = ServerSession()
+
+        msg = if true
+            # Alternative 1: use a remote worker and capture stderr using redirect_stderr.
+            # The reason we cannot use the same session as above is that a Malt.Worker's
+            # stderr is bound to the local stderr when the worker is instantiated, which
+            # happens at the first call to update_run! with a given session and notebook.
+            # Since update_run! has been called several times above, the existing worker's
+            # stderr is already bound to the local TTY, and redirect_stderr won't change
+            # that. We need a new session such that we can instantiate a new worker within
+            # the redirect_stderr block. (An alternative would be to move this testset to
+            # the top of the file such that it gets the first call to update_run!, but it
+            # seems like a bad idea to have tests that may pass or fail depending on the
+            # order of the testsets.)
+            #
+            # This argument is specific to Malt. Distributed relays stderr differently.
+            🍍.options.evaluation.workspace_use_distributed = true
+
+            pipe = Pipe()
+            redirect_stderr(pipe) do
+                update_run!(🍍, notebook, notebook.cells[27:29])
+            end
+            t = Threads.@spawn read(pipe, String)
+            close(pipe)
+            fetch(t)
+        else
+            # Alternative 2: use a local worker and capture stderr using
+            # PlutoRunner.redirect_original_stderr. This avoids the complications due to
+            # stderr relaying between processes and Malt implementation details, but
+            # requires using a bespoke redirection function from PlutoRunner rather than
+            # Base.redirect_stderr.
+            🍍.options.evaluation.workspace_use_distributed = false
+
+            io = IOBuffer()
+            PlutoRunner.redirect_original_stderr(io) do
+                update_run!(🍍, notebook, notebook.cells[27:29])
+            end
+            msg_ = String(take!(io))
+            close(io)
+            msg_
+        end
+
+        @test notebook.cells[27] |> noerror
+        @test notebook.cells[28] |> noerror
+        @test notebook.cells[29] |> noerror
+        @test occursin("Failed to relay log from PlutoRunner", msg)
+
+        cleanup(🍍, notebook)
+    end
 end