diff --git a/docs/agent-guide/038-electron-db-worker-switch-graph.md b/docs/agent-guide/038-electron-db-worker-switch-graph.md new file mode 100644 index 0000000000..eed3e7acb3 --- /dev/null +++ b/docs/agent-guide/038-electron-db-worker-switch-graph.md @@ -0,0 +1,187 @@ +# Electron Db Worker Switch Graph Implementation Plan + +Goal: Fix Electron graph switching with db-worker-node so switching to a new graph never leaves the renderer bound to a stopped runtime. + +Architecture: Keep db-worker runtime lifecycle in the `db-worker-runtime` path and window close path, and treat `setCurrentGraph` as window graph metadata synchronization only. + +Architecture: Remove or guard the duplicate release path in `setCurrentGraph` that can stop the newly started runtime after `persist-db/ persist-db/ ipc "db-worker-runtime" B. + -> main db-worker manager switches A -> B. + -> state/set-current-repo! B. + -> ipc "setCurrentGraph" B. + -> handler calls release-window! again. + -> B runtime may be stopped unexpectedly. + +Target sequence. +Renderer restore graph B. + -> persist-db/ ipc "db-worker-runtime" B. + -> main db-worker manager switches A -> B. + -> state/set-current-repo! B. + -> ipc "setCurrentGraph" B. + -> handler only updates window graph path. + -> B runtime stays available. +``` + +## Testing Plan + +I will follow `@test-driven-development` and add failing tests before each behavior change. + +I will add a failing regression test in `/Users/rcmerci/gh-repos/logseq/src/test/electron/db_worker_manager_test.cljs` that encodes the expected post-switch invariant that the new runtime remains active until explicit release on window close. + +I will add a failing test in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/persist_db_test.cljs` for the graph switch flow to assert runtime rebinding happens once per target repo and is not reinitialized by graph metadata sync. + +I will add a focused unit test file `/Users/rcmerci/gh-repos/logseq/src/test/electron/graph_switch_flow_test.cljs` for extracted pure graph-switch decision logic so the release/no-release condition is testable without Electron GUI dependencies. + +I will run focused tests after each phase and finish with `bb dev:lint-and-test`. + +I will perform manual Electron smoke checks that open graph A, switch to graph B, execute thread-api reads and writes, and then switch back to graph A. + +I will review changes against `@prompts/review.md` before merge. + +NOTE: I will write *all* tests before I add any implementation behavior. + +## Implementation plan + +### Phase 1: Add failing tests that reproduce the switch-order regression. + +1. Add a failing test in `/Users/rcmerci/gh-repos/logseq/src/test/electron/db_worker_manager_test.cljs` that simulates A -> B switch and asserts no stop is triggered for B before explicit release. +2. Add a failing test in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/persist_db_test.cljs` for the sequence ` B -> A and invoking read and write actions after each switch. +26. Validate no immediate invoke failures after switch and confirm runtime stays available for the active graph. +27. Remove temporary logs that are not needed for long-term maintainability. + +### Phase 7: Final verification and review gate. + +28. Run `bb dev:test -v 'electron.graph-switch-flow-test'`. +29. Run `bb dev:test -v 'electron.db-worker-manager-test'`. +30. Run `bb dev:test -v 'frontend.persist-db-test'`. +31. Run `bb dev:lint-and-test`. +32. Confirm each command reports `0 failures, 0 errors`. +33. Run final review checklist pass against `@prompts/review.md`. + +## Edge cases + +| Scenario | Expected behavior | +|---|---| +| Switch A -> B in one window where both graphs are local db graphs. | Runtime for B remains active and calls succeed immediately after switch. | +| Switch A -> B -> A quickly before all async handlers settle. | Late `setCurrentGraph` sync does not stop the currently active runtime. | +| Two windows share graph A and one window switches to graph B. | Graph A runtime remains alive for the other window and graph B runtime starts for the switching window. | +| Re-select current graph B from UI without actual graph change. | No runtime restart and no runtime release occurs. | +| Window closes right after switch to B. | Runtime release happens exactly once via close flow and does not leave stale window mapping. | +| Runtime ownership is external (`:owned? false`). | Graph switch does not attempt to stop external runtime unexpectedly. | +| Restore fails after runtime bind but before UI route redirect. | Failure handling does not silently stop the newly bound runtime unless explicit cleanup path runs. | + +## Verification commands and expected outputs + +```bash +bb dev:test -v 'electron.graph-switch-flow-test' +bb dev:test -v 'electron.db-worker-manager-test' +bb dev:test -v 'frontend.persist-db-test' +bb dev:lint-and-test +``` + +Each command should finish with `0 failures, 0 errors`. + +Manual Electron switch checks should show successful read and write operations immediately after each switch. + +No `db-worker invoke failed` errors should appear during normal A -> B -> A switching. + +## Testing Details + +The new tests verify behavior around event ordering and runtime lifecycle boundaries instead of implementation details. + +The manager tests validate that stale or duplicate release operations cannot terminate the active repo runtime. + +The frontend tests validate that runtime rebinding is driven by repo changes and not by metadata synchronization calls. + +Manual smoke checks validate real Electron runtime behavior that unit tests cannot fully represent. + +## Implementation Details + +- Keep `setCurrentGraph` focused on graph-path synchronization and remove lifecycle side effects from that path. +- Keep runtime start and stop orchestration centralized in `electron.db-worker` manager APIs. +- Use a small pure helper for release decision logic so regression tests do not depend on Electron runtime modules. +- Preserve existing `db-worker-runtime` IPC contract from renderer to main process. +- Keep old graph cleanup tied to explicit switch lifecycle and window close lifecycle only. +- Validate switch behavior with both single-window and multi-window tests. +- Use `@test-driven-development` for red-green implementation order. +- Follow `@prompts/review.md` checks before merging. + +## Question + +Decision: On failed graph restore, keep the newly bound runtime alive for fast retry in the same window. + +Decision: Do not add a short-lived debug flag for switch sequencing logs in development builds. + +Decision: Add an E2E scenario in `/Users/rcmerci/gh-repos/logseq/clj-e2e` for switch graph with db-worker-node and treat it as a release gate. + +--- diff --git a/src/electron/electron/db_worker.cljs b/src/electron/electron/db_worker.cljs index 67ff0087f5..3b0709bb19 100644 --- a/src/electron/electron/db_worker.cljs +++ b/src/electron/electron/db_worker.cljs @@ -42,7 +42,7 @@ [state nil] (let [remaining (disj (:windows entry) window-id) state' (cond-> state - true + (= repo (get-in state [:window->repo window-id])) (update :window->repo dissoc window-id) (seq remaining) diff --git a/src/electron/electron/graph_switch_flow.cljs b/src/electron/electron/graph_switch_flow.cljs new file mode 100644 index 0000000000..a91b405302 --- /dev/null +++ b/src/electron/electron/graph_switch_flow.cljs @@ -0,0 +1,13 @@ +(ns electron.graph-switch-flow) + +(defn release-runtime-on-set-current-graph? + "Decides whether `setCurrentGraph` should release db-worker runtime. + + Returns `false` by design. + + In Electron, `setCurrentGraph` is metadata synchronization for window graph + path only. Runtime lifecycle is handled by the `db-worker-runtime` IPC path + (switch/start) and window close path (release). Releasing here reintroduces + the stale double-release bug that can stop the newly bound runtime." + [_switch] + false) diff --git a/src/electron/electron/handler.cljs b/src/electron/electron/handler.cljs index 31daa5f093..9379140af4 100644 --- a/src/electron/electron/handler.cljs +++ b/src/electron/electron/handler.cljs @@ -27,6 +27,7 @@ [electron.state :as state] [electron.utils :as utils] [electron.window :as win] + [electron.graph-switch-flow :as graph-switch-flow] [logseq.cli.common.graph :as cli-common-graph] [logseq.common.graph :as common-graph] [logseq.db.sqlite.util :as sqlite-util] @@ -326,8 +327,11 @@ (defmethod handle :setCurrentGraph [^js window [_ graph-name]] (let [next-graph-path (when graph-name (utils/get-graph-dir graph-name)) - current-graph-path (state/get-window-graph-path window)] - (p/let [_ (when (not= current-graph-path next-graph-path) + current-graph-path (state/get-window-graph-path window) + release-runtime? (graph-switch-flow/release-runtime-on-set-current-graph? + {:previous-graph-path current-graph-path + :next-graph-path next-graph-path})] + (p/let [_ (when release-runtime? (db-worker/release-window! (.-id window)))] (if next-graph-path (set-current-graph! window next-graph-path) diff --git a/src/test/electron/db_worker_manager_test.cljs b/src/test/electron/db_worker_manager_test.cljs index 77c8650a37..34048e9daa 100644 --- a/src/test/electron/db_worker_manager_test.cljs +++ b/src/test/electron/db_worker_manager_test.cljs @@ -77,6 +77,50 @@ (is false (str "unexpected error: " e)))) (p/finally (fn [] (done))))))) +(deftest ensure-stopped-stale-repo-does-not-clear-new-window-mapping + (async done + (let [stop-calls (atom []) + manager (db-worker/create-manager + {:start-daemon! (fn [repo] (p/resolved (runtime repo))) + :stop-daemon! (fn [rt] + (swap! stop-calls conj (:repo rt)) + (p/resolved true))})] + (-> (p/let [_ (db-worker/ensure-started! manager "graph-a" :window-1) + _ (db-worker/ensure-started! manager "graph-a" :window-2) + _ (db-worker/ensure-started! manager "graph-b" :window-1) + ;; simulate late/stale release for previous repo after window-1 already moved to graph-b + _ (db-worker/ensure-stopped! manager "graph-a" :window-1) + manager-state @(:state manager)] + (is (= "graph-b" (get-in manager-state [:window->repo :window-1]))) + (is (= #{:window-2} (get-in manager-state [:repos "graph-a" :windows]))) + (is (= #{:window-1} (get-in manager-state [:repos "graph-b" :windows]))) + (is (empty? @stop-calls))) + (p/catch (fn [e] + (is false (str "unexpected error: " e)))) + (p/finally (fn [] (done))))))) + +(deftest ensure-stopped-stale-intermediate-repo-after-switch-back-keeps-current-repo + (async done + (let [stop-calls (atom []) + manager (db-worker/create-manager + {:start-daemon! (fn [repo] (p/resolved (runtime repo))) + :stop-daemon! (fn [rt] + (swap! stop-calls conj (:repo rt)) + (p/resolved true))})] + (-> (p/let [_ (db-worker/ensure-started! manager "graph-a" :window-1) + _ (db-worker/ensure-started! manager "graph-b" :window-1) + _ (db-worker/ensure-started! manager "graph-a" :window-1) + ;; stale cleanup for graph-b arrives after the window is already back on graph-a + _ (db-worker/ensure-stopped! manager "graph-b" :window-1) + manager-state @(:state manager)] + (is (= "graph-a" (get-in manager-state [:window->repo :window-1]))) + (is (= #{:window-1} (get-in manager-state [:repos "graph-a" :windows]))) + (is (nil? (get-in manager-state [:repos "graph-b"]))) + (is (= ["graph-a" "graph-b"] @stop-calls))) + (p/catch (fn [e] + (is false (str "unexpected error: " e)))) + (p/finally (fn [] (done))))))) + (deftest ensure-window-stopped-releases-active-runtime-by-window (async done (let [stop-calls (atom []) diff --git a/src/test/electron/graph_switch_flow_test.cljs b/src/test/electron/graph_switch_flow_test.cljs new file mode 100644 index 0000000000..eebeb437ac --- /dev/null +++ b/src/test/electron/graph_switch_flow_test.cljs @@ -0,0 +1,15 @@ +(ns electron.graph-switch-flow-test + (:require [cljs.test :refer [deftest is]] + [electron.graph-switch-flow :as graph-switch-flow])) + +(deftest set-current-graph-switch-does-not-release-runtime + (is (false? + (graph-switch-flow/release-runtime-on-set-current-graph? + {:previous-graph-path "graph-a" + :next-graph-path "graph-b"})))) + +(deftest set-current-graph-reselect-does-not-release-runtime + (is (false? + (graph-switch-flow/release-runtime-on-set-current-graph? + {:previous-graph-path "graph-a" + :next-graph-path "graph-a"})))) diff --git a/src/test/frontend/persist_db_test.cljs b/src/test/frontend/persist_db_test.cljs index 94538a48c3..37a74dd78d 100644 --- a/src/test/frontend/persist_db_test.cljs +++ b/src/test/frontend/persist_db_test.cljs @@ -5,6 +5,7 @@ [frontend.persist-db :as persist-db] [frontend.persist-db.protocol :as protocol] [frontend.persist-db.remote :as remote] + [frontend.storage :as storage] [frontend.state :as state] [frontend.util :as util] [promesa.core :as p])) @@ -112,6 +113,52 @@ (set! remote/stop! original-stop!) (done))))))) +(deftest electron-fetch-init-data-then-set-current-repo-does-not-rebind-runtime + (async done + (let [ipc-calls (atom []) + start-calls (atom []) + stop-calls (atom []) + wrapped-worker (fn [& _] nil) + original-state @state/state + original-electron? util/electron? + original-ipc ipc/ipc + original-start! remote/start! + original-stop! remote/stop! + original-storage-set storage/set + original-storage-remove storage/remove] + (reset-runtime-state!) + (set! util/electron? (constantly true)) + (set! ipc/ipc (fn [channel repo] + (swap! ipc-calls conj [channel repo]) + (p/resolved nil))) + (set! remote/start! (fn [{:keys [repo]}] + (swap! start-calls conj repo) + (->FakeRemote repo wrapped-worker))) + (set! remote/stop! (fn [client] + (swap! stop-calls conj (:repo client)) + (p/resolved true))) + (set! storage/set (fn [& _] nil)) + (set! storage/remove (fn [& _] nil)) + (-> (p/let [repo "logseq_db_graph_a" + _ (persist-db/