fix(electron): never let a failed graph save make the window unclosable - #43
fix(electron): never let a failed graph save make the window unclosable#43Lcstyle wants to merge 1 commit into
Conversation
If frontend.db/persist! rejects, electron.listener/persist-dbs! replies on the "persistent-dbs-error" channel. There was no handler for it, so three things went wrong at once and the app could not be closed again for the rest of the session: 1. electron.window/close-handler had already called (.preventDefault e) and then parked on a bare (async/<! state/persistent-dbs-chan). The confirmation only ever arrives from :persistent-dbs-saved, so the go block waited forever and (destroy-window! win) was unreachable. 2. The message fell through to (defmethod handle :default [args]), which declares one parameter while (defmulti handle (fn [_window args] ...)) dispatches with two. That raised an arity exception rather than logging, and set-ipc-handler! swallowed it - so the unhandled channel left almost no trace. The one log line it did produce printed the BrowserWindow object, not the channel name. 3. electron/core.cljs resets *win to nil right after the first close attempt and guards the retry with (when window ...), so every later click on X, Quit or Ctrl+Q silently did nothing at all. Handle :persistent-dbs-error by unblocking the channel, give the wait a 10s timeout so no future failure mode can reproduce this, and fix the :default arity so unhandled channels are actually reported. Closing without the cache is safe: the graph's markdown files on disk are the source of truth and are already written; the transit file is only a parsed-DB cache, so the cost is a slower next startup, not data loss. Refs logseq/logseq#12968
|
Context on the red check, so you don't have to dig into it: The only workflow that ran is PR Labeler, and it fails with Also worth flagging: no build or test workflow ran here at all. Since CI won't cover this, here's what I did verify locally — a On the unpatched build that exact sequence is the permanent hang. Repeating the caveat from the description so it isn't lost: I could not get the 10s timeout to fire One incidental finding while testing, in case it's useful: |
Companion to logseq/logseq#42. That PR removes one way the graph save can fail; this one makes any save
failure survivable instead of turning the app into something that can never be closed.
Refs logseq/db-test#1133.
The bug
If
frontend.db/persist!rejects,frontend.handler.repo/persist-db!catches it and callson-error, whichelectron.listener/persist-dbs!wires to(ipc/ipc "persistent-dbs-error")(
src/main/electron/listener.cljs:39).Nothing handles that channel.
grep -rn "persistent-dbs-error"over the whole tree returnsexactly one hit: the sender. Three things then go wrong at once:
1. The close handler parks forever.
electron.window/close-handler(
src/electron/electron/window.cljs:82-99) calls(.preventDefault e)first and unconditionally,sends
"persistent-dbs", then waits on a bare(async/<! state/persistent-dbs-chan). That channelis only ever fed by
:persistent-dbs-saved, so when the renderer answers on the error channelinstead the go block waits forever and
(destroy-window! win)is unreachable. There is no timeout.2. The
:defaultfallback throws instead of logging.(defmulti handle (fn [_window args] ...))dispatches with two arguments, but
(defmethod handle :default [args] ...)declares one(
handler.cljs:694). So an unhandled channel raises an arity exception, whichset-ipc-handler!swallows. The single log line it does produce prints the BrowserWindow object rather than the
channel name — a ~200-line object dump that identifies nothing.
3. Retrying does nothing.
electron/core.cljs:322-324runs(reset! *win nil)right after thefirst close attempt and guards the retry with
(when window ...). From the second click onward, X /Quit / Ctrl+Q call
preventDefaultand then do nothing at all — silently.The user-visible result is an application that ignores every quit request, with no dialog and
effectively no log trail. On macOS there is at least a transient "syncing internal status" toast; on
Linux there is no feedback whatsoever. My own instance sat like this for 2 days 16 hours. Nothing
was deadlocked — every thread was sleeping and the renderer answered
executeJavaScriptin 2 ms.I confirmed (3) on the stuck instance by replaying the close through the Electron main-process
inspector:
closefired,defaultPrevented: true, window still alive 6 s later, zero renderererrors recorded and zero new log lines.
The change
:persistent-dbs-error, which logs and unblocks the channel so the window can close.reproduce this either.
:defaultarity and log the channel name instead of the window.Closing without the cache is safe and worth saying explicitly: the graph's markdown files are the
source of truth and are already on disk. The
.transitfile is only a parsed-DB cache, so the costof closing anyway is a slower next startup, not data loss. Silently refusing to close is strictly
worse — in my case it also meant the cache silently stopped being written for eight days before I
noticed.
Verification
Built this branch with the
create_pagefix from logseq/logseq#42 deliberately left out, so the Bean bug islive and the save genuinely fails, then ran it against a real 5228-page graph:
On the unpatched build that exact sequence is the permanent hang. Here it logs a clear reason and
exits in a second.
One caveat, stated plainly: I did not manage to observe the 10 s timeout fire at runtime. Once
:persistent-dbs-erroris handled it always unblocks the channel first, and I could not induce"renderer never replies at all" from outside the app. The timeout is compiled (release build, 0
warnings) and is a standard
alts!overasync/timeout, but it is reviewed rather than exercised —worth a reviewer's eye. Happy to drop it to a separate PR if you'd prefer to take only the handler
and arity fixes, which are the ones under test.