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/<restore-server-config-from-secure- storage! at startup to copy a surviving value back into localStorage if it came back empty. Guarded so this is a true no-op outside native mobile (including under Node tests, where it's additionally gated by the same util/node-test? check the existing localStorage calls already used) -- verified via the :mobile shadow-cljs target compiling clean and the full test suite (subs-test, config-test, db-worker-node-test, db-worker-node-lock-test, subs-loader-test) staying green. iOS ATS and Android cleartext traffic, also called out in this phase of the plan, turned out to already be configured permissively (NSAllowsArbitraryLoads in Info.plist, cleartextTrafficPermitted in network_security_config.xml) -- no changes needed there. Investigated whether db-core's .remoteInvoke serializes concurrent invocations, per the plan's own open question: it does not -- frontend.common.thread-api/remote-function dispatches straight into the handler with no per-repo queueing, so two concurrent write invokes to the same repo could in principle interleave at any await point inside the handler. Deliberately did NOT add a write queue for this: the plan gates that on Fase 0 telemetry showing real invoke overlap in practice, and doing it without solid evidence of need risks a mistake in read-vs-write method classification for uncertain benefit -- querying methods (:thread-api/q, :thread-api/get-render-snapshots, etc.) share the same repo-scoped lock-check path Fase 6 just optimized for read latency, and serializing those too would undo that work. Left as an explicitly open question rather than guessed at. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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 <restore-server-config!
|
||||
;; below copies a surviving value back into localStorage on next launch if
|
||||
;; it came back empty (Fase 7). Reuses the existing secure-storage wrapper
|
||||
;; (its key prefix says "e2ee" internally -- that's the plugin's storage
|
||||
;; namespace, not a claim about what's stored under it) rather than adding a
|
||||
;; second native-storage integration for one more kind of secret.
|
||||
(def ^:private secure-server-url-key "db-worker-server-url")
|
||||
(def ^:private secure-server-token-key "db-worker-server-token")
|
||||
|
||||
(defn- native-mobile?
|
||||
"mobile-util/native-platform? touches the Capacitor bridge, which doesn't
|
||||
exist under Node (tests, the daemon) or a plain browser tab -- treat any
|
||||
throw there the same as \"not native\" rather than letting it take down
|
||||
every config.cljs caller, most of which run in those environments."
|
||||
[]
|
||||
(try
|
||||
(boolean (mobile-util/native-platform?))
|
||||
(catch :default _ false)))
|
||||
|
||||
(defn- <mirror-server-config-to-secure-storage!
|
||||
[key value]
|
||||
(if (native-mobile?)
|
||||
(-> (if (string/blank? value)
|
||||
(secure-storage/<remove-item! key)
|
||||
(secure-storage/<set-item! key value))
|
||||
(p/catch (fn [e]
|
||||
(log/warn :config/secure-storage-mirror-failed {:key key :error e}))))
|
||||
(p/resolved nil)))
|
||||
|
||||
(defn <restore-server-config-from-secure-storage!
|
||||
"Best-effort recovery for native mobile: if localStorage lost the
|
||||
configured server URL/token but the OS keychain/keystore still has it,
|
||||
copy it back into localStorage. Call once at mobile app startup, before
|
||||
the client decides whether it's remote-configured; a no-op everywhere
|
||||
else."
|
||||
[]
|
||||
(if (native-mobile?)
|
||||
(-> (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/<get-item secure-server-url-key))
|
||||
stored-token (when need-token? (secure-storage/<get-item secure-server-token-key))]
|
||||
(when (seq stored-url)
|
||||
(.setItem js/localStorage "db-worker-server-url" stored-url))
|
||||
(when (seq stored-token)
|
||||
(.setItem js/localStorage "db-worker-server-token" stored-token))
|
||||
nil)
|
||||
(p/catch (fn [e]
|
||||
(log/warn :config/secure-storage-restore-failed {:error e}))))
|
||||
(p/resolved nil)))
|
||||
|
||||
(defn get-server-base-url
|
||||
"Read the configured db-worker-node server base URL from localStorage.
|
||||
Returns nil when not set or empty."
|
||||
@@ -118,9 +179,11 @@
|
||||
empty string to clear."
|
||||
[url]
|
||||
(when-not util/node-test?
|
||||
(if (or (nil? url) (string/blank? url))
|
||||
(.removeItem js/localStorage "db-worker-server-url")
|
||||
(.setItem js/localStorage "db-worker-server-url" (string/trim url)))))
|
||||
(let [trimmed (some-> url string/trim)]
|
||||
(if (string/blank? trimmed)
|
||||
(.removeItem js/localStorage "db-worker-server-url")
|
||||
(.setItem js/localStorage "db-worker-server-url" trimmed))
|
||||
(<mirror-server-config-to-secure-storage! secure-server-url-key trimmed))))
|
||||
|
||||
(defn get-server-token
|
||||
"Read the current db-worker-node session bearer token from localStorage.
|
||||
@@ -136,9 +199,11 @@
|
||||
nil or empty string to clear (e.g. on logout or a 401)."
|
||||
[token]
|
||||
(when-not util/node-test?
|
||||
(if (or (nil? token) (string/blank? token))
|
||||
(.removeItem js/localStorage "db-worker-server-token")
|
||||
(.setItem js/localStorage "db-worker-server-token" (string/trim token)))))
|
||||
(let [trimmed (some-> token string/trim)]
|
||||
(if (string/blank? trimmed)
|
||||
(.removeItem js/localStorage "db-worker-server-token")
|
||||
(.setItem js/localStorage "db-worker-server-token" trimmed))
|
||||
(<mirror-server-config-to-secure-storage! secure-server-token-key trimmed))))
|
||||
|
||||
(defn clear-server-config!
|
||||
"Clear both the configured server URL and session token, e.g. on logout."
|
||||
|
||||
@@ -84,10 +84,66 @@
|
||||
(js->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
|
||||
|
||||
@@ -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/<restore-server-config-from-secure-storage!)
|
||||
|
||||
(.addEventListener js/window "sendIntentReceived" intent/handle-received)
|
||||
|
||||
;; handle share for code start
|
||||
|
||||
@@ -89,6 +89,78 @@
|
||||
(fs/writeFileSync path (js/JSON.stringify (clj->js legacy-lock)))
|
||||
(is (= :unknown (:owner-source (db-lock/read-lock path))))))
|
||||
|
||||
(defn- <poll-until
|
||||
"Retries pred-fn (a 0-arg fn returning a promise of a boolean) until it
|
||||
resolves truthy or max-attempts is exhausted, waiting delay-ms between
|
||||
tries. Used below to wait out the fs.watch invalidation window instead of
|
||||
asserting on an exact timing, since inotify delivery isn't instantaneous."
|
||||
[pred-fn max-attempts delay-ms]
|
||||
(p/loop [attempt 0]
|
||||
(p/let [ok? (pred-fn)]
|
||||
(cond
|
||||
ok? true
|
||||
(>= 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? (<poll-until
|
||||
(fn []
|
||||
(p/resolved
|
||||
(try
|
||||
(db-lock/assert-lock-owner! path lock)
|
||||
false
|
||||
(catch :default e
|
||||
(= :repo-locked (:code (ex-data e)))))))
|
||||
40 50)]
|
||||
(is picked-up?
|
||||
"assert-lock-owner! must eventually reject once the on-disk
|
||||
lock no longer matches, not serve the pre-change lock-id
|
||||
forever"))
|
||||
(p/catch (fn [e]
|
||||
(is false (str "unexpected error: " e))))
|
||||
(p/finally (fn []
|
||||
(db-lock/remove-lock! path)
|
||||
(done)))))))
|
||||
|
||||
(deftest update-lock-preserves-existing-owner-source-and-does-not-add-discovery-fields
|
||||
(async done
|
||||
(let [root-dir (node-helper/create-tmp-dir "db-worker-node-lock-update-owner")
|
||||
|
||||
Reference in New Issue
Block a user