Skip to content

fix(electron): never let a failed graph save make the window unclosable - #43

Open
Lcstyle wants to merge 1 commit into
logseq:version/filefrom
Lcstyle:fix/unquittable-window-on-persist-failure
Open

fix(electron): never let a failed graph save make the window unclosable#43
Lcstyle wants to merge 1 commit into
logseq:version/filefrom
Lcstyle:fix/unquittable-window-on-persist-failure

Conversation

@Lcstyle

@Lcstyle Lcstyle commented Aug 5, 2026

Copy link
Copy Markdown

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 calls
on-error, which electron.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 returns
exactly 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 channel
is only ever fed by :persistent-dbs-saved, so when the renderer answers on the error channel
instead the go block waits forever and (destroy-window! win) is unreachable. There is no timeout.

2. The :default fallback 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, which set-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-324 runs (reset! *win nil) right after the
first close attempt and guards the retry with (when window ...). From the second click onward, X /
Quit / Ctrl+Q call preventDefault and 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 executeJavaScript in 2 ms.

I confirmed (3) on the stuck instance by replaying the close through the Electron main-process
inspector: close fired, defaultPrevented: true, window still alive 6 s later, zero renderer
errors recorded and zero new log lines.

The change

  • Add :persistent-dbs-error, which logs and unblocks the channel so the window can close.
  • Give the wait a 10 s timeout, so a renderer that never answers at all (wedged, crashed) can't
    reproduce this either.
  • Fix the :default arity 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 .transit file is only a parsed-DB cache, so the cost
of 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_page fix from logseq/logseq#42 deliberately left out, so the Bean bug is
live and the save genuinely fails, then ran it against a real 5228-page graph:

1. trigger the Bean : create_page('zz-prb-test', {a:'b'})  -> created 130448
2. force a persist  : bean error present: True          <- save really is failing
3. close the window : EXITED at t+1s

09:36:20.820 > Failed to persist the graph cache. Closing anyway - your files on disk are
               unaffected, but the next startup will re-index.
09:36:39.921 > window-all-closed Quitting...

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-error is 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! over async/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.

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
@Lcstyle

Lcstyle commented Aug 5, 2026

Copy link
Copy Markdown
Author

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
HttpError: No commit found for the ref fix/unquittable-window-on-persist-failure.
That's pull_request_target resolving the head ref against the base repo, so it can't see a branch
that lives in a fork — a known limitation of TimonVS/pr-labeler-action. It's not specific to this
PR: every PR Labeler run in this repo has failed the same way, including chore/readme-og-explanation
and og-readme on 2026-07-17. Nothing in the diff causes it and there's nothing I can do from a fork.

Also worth flagging: no build or test workflow ran here at all. build.yml, db.yml, e2e.yml,
graph-parser.yml and publishing.yml are all gated on branches: [master] for both push and
pull_request, so PRs targeting version/file don't trigger any of them. pr-labeler.yml is the
only workflow without a branch filter, which is why it's the only one that ran.

Since CI won't cover this, here's what I did verify locally — a release app electron --debug build
(0 warnings) with the create_page fix from logseq/logseq#42 deliberately left out, so the save genuinely
throws and the failure path is the one actually under test:

1. trigger the Bean : create_page('zz-prb-test', {a:'b'})  -> created 130448
2. force a persist  : bean error present: True          <- the save really is failing
3. close the window : EXITED at t+1s

09:36:20.820 > Failed to persist the graph cache. Closing anyway - your files on disk are
               unaffected, but the next startup will re-index.
09:36:39.921 > window-all-closed Quitting...

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
at runtime — once :persistent-dbs-error is handled it always unblocks the channel first, and I
couldn't induce "renderer never replies at all" from outside the app. So the handler and arity fixes
are exercised; the timeout is compiled and reviewed but not. Glad to split it into its own PR if
you'd prefer to take only the tested part.

One incidental finding while testing, in case it's useful: state/persistent-dbs-chan is
(async/chan 1), so a routine successful auto-save leaves a value buffered in it. A close can then
return immediately and look like a pass. The original hang needed no successful persist since
startup — which is exactly the situation the Bean creates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant