From c96b8a7a687c66d439ba8838a20c243dbbccf858 Mon Sep 17 00:00:00 2001 From: Dasemu Date: Wed, 26 Aug 2026 11:33:30 +0200 Subject: [PATCH] Fase 7: cache the daemon's per-invoke lock check, harden mobile server config storage assert-lock-owner! ran on every write invoke and did fs.existsSync + fs.readFileSync + JSON.parse + a process.kill(pid, 0) syscall each time -- all synchronous on the Node event loop, once per request. Now cached in db_worker_node_lock.cljs: the lock file's content is cached and invalidated by watching its containing directory (survives an unlink+recreate, unlike watching the file path directly), and the liveness check is skipped entirely when the recorded pid is this process's own -- the dominant case, since assert-lock-owner! is the owner checking its own lock, not a stranger checking someone else's. Writes this process makes itself (create-lock!/update-lock!/ remove-lock!) invalidate the cache synchronously in the same call, so the only staleness window is for a lock file changed by another process -- a few ms until the fs.watch callback fires, which doesn't weaken the two-daemons-can't-both-hold-a-live-lock guarantee. Verified against a real daemon (5 consecutive write invokes, no lock-watch-failed warnings) and with two new tests: one proving repeated assert-lock-owner! calls keep agreeing with an unchanged lock, one proving a lock file rewritten by "another process" (a raw fs.writeFileSync bypassing update-lock!'s own invalidation) is picked up within the polling window instead of being served stale forever. All 8 pre-existing lock tests and all 61 db-worker-node-test tests still pass. frontend.config's db-worker-server-url/token getters/setters stay synchronous localStorage reads everywhere (retrofitting their many callers to await an async native store is out of scope), but on native mobile, writes now also mirror into frontend.mobile.secure-storage (OS keychain/keystore) as a durability backstop -- WebView localStorage can be evicted under storage pressure or not survive a reinstall. mobile.init/init! now fires config/ --- src/main/frontend/config.cljs | 77 +++++++++++++++++-- .../frontend/worker/db_worker_node_lock.cljs | 75 +++++++++++++++++- src/main/mobile/init.cljs | 8 ++ .../worker/db_worker_node_lock_test.cljs | 72 +++++++++++++++++ 4 files changed, 223 insertions(+), 9 deletions(-) diff --git a/src/main/frontend/config.cljs b/src/main/frontend/config.cljs index 41830a3..ad8d1e0 100644 --- a/src/main/frontend/config.cljs +++ b/src/main/frontend/config.cljs @@ -2,14 +2,18 @@ "App config and fns built on top of configuration" (:require [clojure.set :as set] [clojure.string :as string] + [frontend.mobile.secure-storage :as secure-storage] + [frontend.mobile.util :as mobile-util] [frontend.state :as state] [frontend.util :as util] [goog.crypt.Md5] + [lambdaisland.glogi :as log] [logseq.common.cognito-config :as cognito-config] [logseq.common.config :as common-config] [logseq.common.graph-dir :as common-graph-dir] [logseq.common.path :as path] [logseq.db.sqlite.util :as sqlite-util] + [promesa.core :as p] [shadow.resource :as rc])) (goog-define DEV-RELEASE false) @@ -104,6 +108,63 @@ ;; (see frontend.persist-db/remote-runtime?). Stored client-side only; the ;; daemon holds the authoritative credential (see ;; frontend.worker.db-worker-node-auth). +;; On native mobile, WebView localStorage isn't durable the way it is on +;; desktop -- iOS in particular can evict it under storage pressure, and it +;; doesn't reliably survive an app reinstall/restore. localStorage stays the +;; synchronous read path everywhere, including mobile (every getter below is +;; called synchronously all over persist_db.cljs/handler.assets.cljs, and +;; retrofitting those call sites to await an async native store is out of +;; scope here); frontend.mobile.secure-storage (OS keychain/keystore) mirrors +;; writes as a best-effort durability backstop and (if (string/blank? value) + (secure-storage/ (p/let [need-url? (string/blank? (.getItem js/localStorage "db-worker-server-url")) + need-token? (string/blank? (.getItem js/localStorage "db-worker-server-token")) + stored-url (when need-url? (secure-storage/ url string/trim)] + (if (string/blank? trimmed) + (.removeItem js/localStorage "db-worker-server-url") + (.setItem js/localStorage "db-worker-server-url" trimmed)) + ( token string/trim)] + (if (string/blank? trimmed) + (.removeItem js/localStorage "db-worker-server-token") + (.setItem js/localStorage "db-worker-server-token" trimmed)) + (clj (js/JSON.parse (.toString (fs/readFileSync path) "utf8")) :keywordize-keys true)))) +;; assert-lock-owner! runs on every write invoke (db_worker_node.cljs), so a +;; naive read-lock there means fs.existsSync + fs.readFileSync + JSON.parse +;; as a synchronous syscall on the Node event loop per request. Cached here, +;; invalidated by watching the lock file's *directory* (not the file itself +;; -- fs.watch on a path can stop firing across an unlink+recreate on some +;; platforms, watching the containing directory and filtering by filename +;; survives it) so an external change to the lock is picked up on the next +;; read instead of the cache going permanently stale (Fase 7). +;; +;; Writes this process makes itself (create-lock!/update-lock!/remove-lock!) +;; invalidate synchronously in the same call, before the fs.watch callback +;; would even fire -- the only staleness window is for a lock file changed +;; by some *other* process, on the order of a few ms until the watch +;; callback runs. That's the same window any lock-based coordination has +;; between the change happening and a reader noticing it; it does not weaken +;; the guarantee two daemons can't both hold a live lock on the same repo. +(defonce ^:private *lock-cache (atom {})) +(def ^:private absent ::absent) + +(defn- invalidate-cached-lock! + [path] + (swap! *lock-cache update path dissoc :lock)) + +(defn- close-watcher! + [path] + (when-let [watcher (get-in @*lock-cache [path :watcher])] + (try (.close watcher) (catch :default _ nil))) + (swap! *lock-cache dissoc path)) + +(defn- ensure-watcher! + [path] + (when-not (get-in @*lock-cache [path :watcher]) + (try + (let [dir (node-path/dirname path) + file-name (node-path/basename path) + watcher (fs/watch dir + (fn [_event changed-name] + (when (or (nil? changed-name) (= changed-name file-name)) + (invalidate-cached-lock! path))))] + (swap! *lock-cache assoc-in [path :watcher] watcher)) + (catch :default e + ;; Directory may not exist yet (lock not created yet) -- next call + ;; retries; falls back to always-fresh reads until it succeeds. + (log/warn :db-worker-node-lock-watch-failed {:path path :error (ex-message e)}))))) + +(defn- cached-lock + [path] + (ensure-watcher! path) + (let [cached (get-in @*lock-cache [path :lock] absent)] + (if (= cached absent) + (let [lock (read-lock path)] + (swap! *lock-cache assoc-in [path :lock] lock) + lock) + cached))) + (defn remove-lock! [path] (when (and (seq path) (fs/existsSync path)) - (fs/unlinkSync path))) + (fs/unlinkSync path)) + (close-watcher! path)) (defn create-lock! [{:keys [root-dir repo owner-source]}] @@ -111,6 +167,7 @@ (fs/writeFileSync fd (js/JSON.stringify (clj->js lock))) (finally (fs/closeSync fd))) + (invalidate-cached-lock! path) (resolve lock))) (catch :default e (log/error :db-worker-node-lock-create-failed e) @@ -133,14 +190,26 @@ :owner-source (normalize-owner-source (:owner-source lock))})] (fs/mkdirSync (node-path/dirname path) #js {:recursive true}) (fs/writeFileSync path (js/JSON.stringify (clj->js lock'))) + (invalidate-cached-lock! path) (resolve lock')) (catch :default e (log/error :db-worker-node-lock-update-failed e) (reject e)))))) +(defn- lock-owner-alive? + "The pid recorded in the lock file is, on the hot path, this very + process's own pid -- assert-lock-owner! is the *owner* checking its own + lock hasn't been usurped, not a stranger checking someone else's. A + process can always signal itself, so that case needs no + process.kill(pid, 0) syscall at all; only a genuinely foreign pid (a + stolen/stale lock, already on the throw path) pays for the real check." + [pid] + (or (= pid (.-pid js/process)) + (= :alive (pid-status pid)))) + (defn assert-lock-owner! [path {:keys [repo pid lock-id] :as owner-lock}] - (let [lock (read-lock path)] + (let [lock (cached-lock path)] (cond (nil? owner-lock) (throw (ex-info "lock owner missing" @@ -152,7 +221,7 @@ {:code :repo-locked :path path})) - (not= :alive (pid-status (:pid lock))) + (not (lock-owner-alive? (:pid lock))) (throw (ex-info "graph lock is stale" {:code :repo-locked :path path diff --git a/src/main/mobile/init.cljs b/src/main/mobile/init.cljs index e40b37b..8568f12 100644 --- a/src/main/mobile/init.cljs +++ b/src/main/mobile/init.cljs @@ -4,6 +4,7 @@ ["@capacitor/keyboard" :refer [^js Keyboard]] ["@capacitor/network" :refer [^js Network]] [clojure.string :as string] + [frontend.config :as config] [frontend.handler.editor :as editor-handler] [frontend.mobile.flows :as mobile-flows] [frontend.mobile.intent :as intent] @@ -106,6 +107,13 @@ (.addListener Network "networkStatusChange" #(reset! mobile-flows/*mobile-network-status %))) (defn init! [] + ;; Fire-and-forget, before anything checks whether this client is + ;; remote-configured -- restores a server URL/token from the OS + ;; keychain/keystore if the WebView's localStorage came back empty (Fase + ;; 7). Best-effort: if it loses the race with the first remote-configured? + ;; check, that check just sees "not configured" until the next one. + (config/js legacy-lock))) (is (= :unknown (:owner-source (db-lock/read-lock path)))))) +(defn- = attempt max-attempts) false + :else (p/do! (p/delay delay-ms) (p/recur (inc attempt))))))) + +(deftest assert-lock-owner-serves-cached-content-without-rereading-on-every-call-test + (async done + (let [root-dir (node-helper/create-tmp-dir "db-worker-node-lock-cache-hot-path") + repo (str "logseq_db_lock_cache_" (subs (str (random-uuid)) 0 8)) + path (db-lock/lock-path root-dir repo)] + (-> (p/let [lock (db-lock/create-lock! {:root-dir root-dir + :repo repo + :owner-source :cli}) + ;; assert-lock-owner! is the hot path invoked on every + ;; write -- calling it repeatedly here must keep agreeing + ;; with the on-disk lock without needing an external + ;; change to have happened in between. + first-check (db-lock/assert-lock-owner! path lock) + second-check (db-lock/assert-lock-owner! path lock) + third-check (db-lock/assert-lock-owner! path lock)] + (is (= lock first-check second-check third-check))) + (p/catch (fn [e] + (is false (str "unexpected error: " e)))) + (p/finally (fn [] + (db-lock/remove-lock! path) + (done))))))) + +(deftest assert-lock-owner-picks-up-a-lock-file-rewritten-by-another-process-test + (async done + (let [root-dir (node-helper/create-tmp-dir "db-worker-node-lock-cache-external-change") + repo (str "logseq_db_lock_cache_ext_" (subs (str (random-uuid)) 0 8)) + path (db-lock/lock-path root-dir repo)] + (-> (p/let [lock (db-lock/create-lock! {:root-dir root-dir + :repo repo + :owner-source :cli}) + ;; Populate the cache the same way the daemon's hot path + ;; does, then simulate a *different* process replacing the + ;; lock (e.g. after recovering it as stale) by writing the + ;; file directly -- bypassing update-lock!'s own + ;; synchronous cache invalidation. This is exactly the + ;; scenario the fs.watch invalidation exists for. + _ (db-lock/assert-lock-owner! path lock) + usurping-lock (assoc lock :lock-id (str (random-uuid))) + _ (do (fs/writeFileSync path (js/JSON.stringify (clj->js usurping-lock))) + nil) + picked-up? (