diff --git a/.serena/memories/frontend/workspace-state-persistence-subtleties.md b/.serena/memories/frontend/workspace-state-persistence-subtleties.md index be612121e1..06bd2ae269 100644 --- a/.serena/memories/frontend/workspace-state-persistence-subtleties.md +++ b/.serena/memories/frontend/workspace-state-persistence-subtleties.md @@ -22,7 +22,7 @@ - Persistence buffers local commits: status becomes pending after about 200ms, commits are flushed after about 3s or `::force-persist`, and buffered commits are merged per file before `:update-file`. - Persistence sends revn as the max of the commit revn and locally tracked latest revn; remote commits update that revn tracker. - Persistence is skipped in version preview/read-only mode or without edit permission. -- Save failures split transient vs terminal (`transient-error?`: the repo retryable types `:network`/`:offline`/`:bad-gateway`/`:service-unavailable` plus `:invalid-save-response`; everything else is terminal). Terminal keeps the `:error` halt + `flash-persistence` path; transient enters a `:retrying` episode: the head commit stays queued and resends with backoff 2s/8s/20s (3 retries, then today's terminal path). Resends rotate the `::request-id` stamp only when the old request left `active-requests`, carry the same `:commit-id`, and never double-send while one request is in flight (`:in-flight` stays silent). Retry timers carry the episode token; a superseded token stays silent. Status stays `:retrying` through re-entries (`next-status` refuses `:pending`/`:saving` from it); waiters (`wait-persisted-or-error`) wait through it and reject only on `:error`. +- Save failures split transient vs terminal (`transient-error?`: the repo retryable types `:network`/`:offline`/`:bad-gateway`/`:service-unavailable`/`:gateway-error` plus `:invalid-save-response`; everything else is terminal). Terminal keeps the `:error` halt + `flash-persistence` path; transient enters a `:retrying` episode: the head commit stays queued and resends with backoff 2s/8s/20s (3 retries, then today's terminal path). A stamped commit whose request left `active-requests` is simply sent again under the same `:commit-id` (the backend `file_commit` table applies a repeated commit id once), so there is no unknown-outcome halt; a request still in flight is never double-sent (`:in-flight` stays silent). A new edit on an `:error` queue resumes it from the head, and permission recovery resumes an already-attempted head too. Retry timers carry the episode token; a superseded token stays silent. Status stays `:retrying` through re-entries (`next-status` refuses `:pending`/`:saving` from it); waiters (`wait-persisted-or-error`) wait through it and reject only on `:error`. - One reconnect notice per episode: sticky toast tagged `:persistence-reconnecting` (single-toast store, re-show replaces), hidden by tag on drain (`:saved`) and on terminal failure; recovery is silent. Header indicator has a `:retrying` state (`workspace.header.retrying`). - Resume triggers: backoff timer and the browser `online` event (guarded by `exists? js/window`; re-enters the runner only for a live `:retrying` episode). New local edits during `:retrying` only join the queue: the episode keeps its `:run-id`, so `append-commit` does not re-enter, and the live runner (still waiting on the head's `commit-persisted`) sends them after the head. Re-entering on edits would bypass the backoff and spend an attempt per batched commit. - Tests instant-trigger retries by stubbing `rx/timer` (recording delays to assert the schedule); dynamic bindings do not survive `await` continuations, so no dynamic var for delays. diff --git a/frontend/src/app/main/data/persistence.cljs b/frontend/src/app/main/data/persistence.cljs index a92b4e1ae3..14e6c7b7f3 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -26,6 +26,7 @@ (declare ^:private run-persistence-task) (declare ^:private persist-commit) +(declare ^:private resume-persistence) (log/set-level! :warn) @@ -35,6 +36,11 @@ (def force-persist? #(= % ::force-persist)) +(def retry-give-up-ms + "How long a failing queue keeps being sent. Matches how long the backend + remembers a commit id." + (* 24 60 60 1000)) + (def ^:private saving-stall-timeout-ms (* 5 60 1000)) (def ^:private saving-check-interval-ms 30000) (def ^:private save-wait-timeout-ms (* 2 60 1000)) @@ -114,10 +120,16 @@ (#{:error :saved} status) (dissoc :run-id :last-progress-at :stall-reported)))))))) +(def ^:private transient-codes + "Save failures a later attempt can get past: an unusable save response, and + a repeat the backend turned away while the save it repeats was running." + #{:invalid-save-response + :commit-in-progress}) + (defn transient-error? "True when a save failure is worth retrying with backoff: a transient transport failure (from the shared `repo/retryable-types` set, which says - nothing about the edits themselves) or an unusable save response. + nothing about the edits themselves) or one of `transient-codes`. Everything else (auth, validation, state) is terminal and keeps the `:error` path. Checked against both `:type` and `:cause-type` because `persistence-failed` wraps the original cause under `:type :persistence`." @@ -125,7 +137,7 @@ (boolean (or (contains? rp/retryable-types type) (contains? rp/retryable-types cause-type) - (= :invalid-save-response code)))) + (contains? transient-codes code)))) (def ^:private retry-delays-ms "Backoff delays (ms) between save retries; the count is the retry budget. @@ -139,6 +151,15 @@ save or on terminal failure." :persistence-reconnecting) +(defn- retry-window-open? + "True while a failing queue may still be sent again. Past this the backend + has forgotten the commit id, so sending it once more could apply the same + changes twice." + [pstate] + (let [since (:failing-since pstate)] + (or (nil? since) + (< (- (inst-ms (ct/now)) since) retry-give-up-ms)))) + (defn- report-stalled-persistence [now] (ptk/reify ::report-stalled-persistence @@ -209,7 +230,7 @@ (throw (ex-info "invalid state" {}))))) (update :index dissoc commit-id) (assoc :last-progress-at (inst-ms (ct/now))) - (dissoc :stall-reported + (dissoc :stall-reported :failing-since :attempts :retry-token :retry-for))))))) (defn- append-commit @@ -232,11 +253,18 @@ ptk/WatchEvent (watch [_ state _] (let [pstate (:persistence state)] - ;; A `:retrying` episode keeps its run id, so a new edit only - ;; joins the queue: the live runner sends it after the head, and - ;; resends stay on the backoff schedule and its attempt budget. - (when (and (not= :error (:status pstate)) - (= run-id (:run-id pstate))) + (cond + ;; A new edit restarts a failed queue from its head, while the + ;; backend still recognizes the head's commit id and so cannot + ;; apply it twice. + (= :error (:status pstate)) + (when (retry-window-open? pstate) + (rx/of (resume-persistence))) + + ;; A `:retrying` episode keeps its run id, so a new edit only + ;; joins the queue: the live runner sends it after the head, and + ;; resends stay on the backoff schedule and its attempt budget. + (= run-id (:run-id pstate)) (rx/of (update-status :saving) (run-persistence-task)))))))) @@ -257,6 +285,7 @@ :commit-id commit-id :hint (ex-message cause) ::errors/handled? true)) + (update :failing-since d/nilv (inst-ms (ct/now))) (dissoc :run-id :last-progress-at :stall-reported :attempts :retry-token :retry-for)))))) @@ -293,6 +322,7 @@ :retry-token (uuid/next) :retry-for commit-id :last-progress-at (inst-ms (ct/now))) + (update :failing-since d/nilv (inst-ms (ct/now))) (dissoc :stall-reported)))))) ptk/WatchEvent @@ -313,19 +343,6 @@ (->> (rx/timer delay-ms) (rx/map (fn [_] (persist-commit commit-id {:token token}))))))))))) -(defn- rotate-stalled-stamp - "Clears a previous attempt stamp when it is safe to resend: the commit is - the current retry head and its request is no longer in flight. A still - in-flight request is left alone — its own result drives the next step." - [state commit-id] - (let [commit (dm/get-in state [:persistence :index commit-id])] - (if (and (= :retrying (dm/get-in state [:persistence :status])) - (= commit-id (dm/get-in state [:persistence :retry-for])) - (::request-id commit) - (not (contains? @active-requests (::request-id commit)))) - (update-in state [:persistence :index commit-id] dissoc ::request-id) - state))) - (defn- commit-persisted [commit] (ptk/reify ::commit-persisted @@ -357,15 +374,13 @@ "Classifies what should happen with a queued commit before sending it. The attempt stamp and the send decision both read this, so a commit is only ever stamped with a request that is actually going to be sent." - [state commit-id request-id] + [state commit-id] (let [commit (dm/get-in state [:persistence :index commit-id])] (cond (= :error (dm/get-in state [:persistence :status])) :halted (nil? commit) :missing-commit (::acknowledged commit) :acknowledged (contains? @active-requests (::request-id commit)) :in-flight - (and (::request-id commit) - (not= request-id (::request-id commit))) :unknown-outcome (not (dm/get-in state [:permissions :can-edit])) :permission-denied :else :ready))) @@ -402,24 +417,22 @@ id cause))))))) (defn- persist-commit - "Sends the queued commit, stamping the attempt first. Inside a `:retrying` - episode a previous stamp is rotated when its request is no longer in - flight, so a retry resends the same commit instead of failing as - `:unknown-outcome`. Carries an optional `:token`: retry timers pass the - episode token, and a stale token (superseded episode) stays silent - instead of sending or failing." + "Sends the queued commit, stamping the attempt first. A commit whose + earlier request is no longer in flight is sent again under the same + commit id, which the backend applies only once. Carries an optional + `:token`: retry timers pass the episode token, and a stale token + (superseded episode) stays silent instead of sending or failing." [commit-id & {:keys [token]}] (let [request-id (uuid/next)] (ptk/reify ::persist-commit ptk/UpdateEvent (update [_ state] (let [token-ok? (or (nil? token) - (= token (dm/get-in state [:persistence :retry-token]))) - state (if token-ok? (rotate-stalled-stamp state commit-id) state)] + (= token (dm/get-in state [:persistence :retry-token])))] (if (and token-ok? - (= :ready (attempt-state state commit-id request-id))) - ;; Record the attempt before starting I/O. An interrupted request - ;; may have reached the server and must not be replayed blindly. + (= :ready (attempt-state state commit-id))) + ;; Stamp the attempt before any I/O, so a request in flight is + ;; never sent twice in parallel. (assoc-in state [:persistence :index commit-id ::request-id] request-id) state))) @@ -436,16 +449,12 @@ (not= token (dm/get-in state [:persistence :retry-token]))) ;; Stale retry timer: its episode was superseded. Stay silent. (rx/empty) - (case (attempt-state state commit-id request-id) + (case (attempt-state state commit-id) :halted (rx/empty) :missing-commit (fail :missing-commit "A queued save has no change data") :acknowledged (rx/of (commit-persisted commit)) ;; The replacement runner listens for the original request's result. :in-flight (rx/empty) - ;; Fail-safe for anything outside a live retry episode (retry - ;; rotation in `update` already cleared the stamp when resending - ;; is safe). Keep the attempt stamp to prevent replay. - :unknown-outcome (fail :save-outcome-unknown "An interrupted save has an unknown outcome") :permission-denied (fail :save-permission-denied "Edit permission was lost before changes could be saved") :ready (send-queued-commit request-id (:session-id state) commit)))))))) @@ -503,7 +512,7 @@ (ptk/reify ::recover-persistence ptk/WatchEvent (watch [_ state _] - (let [{:keys [queue index status error run-id]} (:persistence state) + (let [{:keys [queue index status error run-id] :as pstate} (:persistence state) commit (get index (peek queue))] (cond ;; A retrying episode owns its recovery through the backoff @@ -512,7 +521,7 @@ (not= status :retrying) (or (not= status :error) (and (= :save-permission-denied (:code error)) - (not (::request-id commit)) + (retry-window-open? pstate) (= (:file-id commit) (:current-file-id state)) (dm/get-in state [:permissions :can-edit])))) (rx/of (resume-persistence)) diff --git a/frontend/test/frontend_tests/data/persistence_retry_test.cljs b/frontend/test/frontend_tests/data/persistence_retry_test.cljs index 87b5745624..31c8ed346c 100644 --- a/frontend/test/frontend_tests/data/persistence_retry_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_retry_test.cljs @@ -218,9 +218,9 @@ (rx/subject))))))) ;; Scenario: a persist-commit arrives with a superseded episode token after -;; a transient failure. Without the token guard it would fail terminally as -;; `:save-outcome-unknown`; with it, nothing happens. Proves: stale retry -;; timers stay silent. +;; a transient failure. Without the token guard it would send outside the +;; backoff schedule; with it, nothing happens. Proves: stale retry timers +;; stay silent. (t/deftest ^:async stale-retry-token-stays-silent (await (with-persistence diff --git a/frontend/test/frontend_tests/data/persistence_test.cljs b/frontend/test/frontend_tests/data/persistence_test.cljs index 4d1b660d84..6ce657ff66 100644 --- a/frontend/test/frontend_tests/data/persistence_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_test.cljs @@ -427,8 +427,8 @@ ;; - Transport failures and malformed answers (`nil`, empty, bad revision) ;; fail the save but preserve the queue for retry. ;; - Initialization recovers gracefully: dangling runners error instead of -;; skipping, unsent commits are picked up, unknown outcomes are reported -;; without replaying. +;; skipping, unsent commits are picked up, unknown outcomes are sent again +;; under the same commit id. ;; ;; Same async pattern as the watchdog tests: triggers stay bare, every ;; assert block is preceded by `wait-for` on its leading signal (or a bare @@ -553,8 +553,9 @@ ;; retry instead, so their coverage moved to the retry tests below (and to ;; the exhaustion test for the terminal toast). -;; The transient/terminal classification is pure: transport failures and -;; unusable save responses retry with backoff, everything else stays +;; The transient/terminal classification is pure: transport failures, +;; unusable save responses and repeats turned away while the save they +;; repeat runs retry with backoff, everything else stays ;; terminal. Note the wrapped shape: `persistence-failed` records the ;; original cause under `:cause-type` with `:type :persistence`. (t/deftest transient-error-classification @@ -571,6 +572,8 @@ (t/is (dps/transient-error? {:type :persistence :cause-type :service-unavailable :code :save-failed})) (t/is (dps/transient-error? {:type :persistence :cause-type :bad-gateway :code :save-failed})) (t/is (dps/transient-error? {:type :persistence :code :invalid-save-response})) + ;; A repeat the backend turned away while the save it repeats was running. + (t/is (dps/transient-error? {:type :persistence :cause-type :validation :code :commit-in-progress})) (t/is (not (dps/transient-error? {:type :authentication}))) (t/is (not (dps/transient-error? {:type :validation}))) (t/is (not (dps/transient-error? {:type :internal}))) @@ -734,27 +737,142 @@ (t/is (empty? (get-in @store [:persistence :queue]))))))) ;; Scenario: a queued commit carries a request id but persistence has no -;; run id for it when initializing — its outcome is unknowable. It errors -;; as `:save-outcome-unknown`, stays queued, and nothing is sent. -;; Proves: unknown outcomes report without replaying (never assume persisted, -;; never resend blindly). -(t/deftest ^:async recovery-reports-an-unknown-request-outcome-without-replaying-it +;; run id for it when initializing, so its outcome is unknown. It is sent +;; again under the same commit id and saves. Proves: an unknown outcome is +;; resolved by the backend, which applies a repeated commit id only once. +(t/deftest ^:async recovery-sends-again-a-save-with-an-unknown-outcome (await (with-persistence - (^:async fn [{:keys [file-id requests store]}] + (^:async fn [{:keys [file-id response requests store]}] (let [commit (assoc @(local-commit file-id) ::dps/request-id (uuid/next)) id (:id commit)] (ptk/emit! store #(assoc % :persistence {:queue (conj #queue [] id) :index {id commit} :status :saving}) (dps/initialize-persistence)) - (await (async/wait-for #(= :save-outcome-unknown - (get-in @store [:persistence :error :code])) - "unknown outcome errors")) - (t/is (= :error (get-in @store [:persistence :status]))) - (t/is (= :save-outcome-unknown (get-in @store [:persistence :error :code]))) - (t/is (= [id] (vec (get-in @store [:persistence :queue])))) - (t/is (empty? @requests))))))) + (await (async/wait-for #(= 1 (count @requests)) "the commit is sent again")) + (t/is (= id (:commit-id (second (first @requests))))) + (rx/push! response {:revn 1}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the resent commit saves")) + (t/is (= 1 (count @requests)))))))) + +;; Scenario: a save fails terminally and a new edit arrives. The queue +;; restarts from its head: the failed commit goes out again under its own +;; commit id, then the new edit follows. Proves: an edit after a failure +;; sends everything queued, in order. +(t/deftest ^:async an-edit-after-a-failed-save-sends-the-queue-again + (let [calls (atom 0)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "the first save fails")) + (t/is (= 1 (count @requests))) + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the queue saves after the new edit")) + (let [commit-ids (mapv (comp :commit-id second) @requests)] + (t/is (= 3 (count commit-ids))) + (t/is (= (first commit-ids) (second commit-ids)) + "the failed commit is sent again under the same commit id") + (t/is (not= (second commit-ids) (nth commit-ids 2)) + "the new edit follows as its own commit"))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "Validation failed" {:type :validation})) + (rx/of {:revn @calls}))))))) + +;; Scenario: a save keeps failing while edits arrive. Inside the retry +;; window each edit sends the queue again; once the window has passed, edits +;; only pile up. Proves: a commit is never sent after the backend may have +;; forgotten its id, and the edits are kept rather than dropped. +(t/deftest ^:async an-edit-past-the-retry-window-does-not-send-the-queue-again + (let [clock (atom 0)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {ct/now (mock/stub #(ct/inst @clock))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "the first save fails")) + (t/is (= 1 (count @requests))) + + (reset! clock (quot dps/retry-give-up-ms 2)) + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 2 (count @requests)) + "inside the window an edit sends the head again")) + + (reset! clock (+ dps/retry-give-up-ms 1000)) + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 3 (count (get-in @store [:persistence :queue]))) + "the edit joins the queue")) + (await (async/settle)) + (t/is (= 2 (count @requests)) "past the window an edit sends nothing") + (t/is (= :error (get-in @store [:persistence :status])))))) + (fn [_ _] (rx/throw (ex-info "Validation failed" {:type :validation}))))))) + +;; Scenario: a save fails and a later edit saves the queue. Proves: a save +;; that lands ends the run of failures, so the retry window starts over. +(t/deftest ^:async a-landed-save-restarts-the-retry-window + (let [calls (atom 0)] + (await + (with-persistence + (^:async fn [{:keys [file-id store]}] + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "the first save fails")) + (t/is (some? (get-in @store [:persistence :failing-since]))) + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the queue saves")) + (t/is (nil? (get-in @store [:persistence :failing-since])))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "Validation failed" {:type :validation})) + (rx/of {:revn @calls}))))))) + +;; Scenario: a transient failure is retried after edit permission is lost, +;; so the commit fails with a request already sent. Restoring the permission +;; sends it again. Proves: an edit already attempted is not left behind when +;; the permission comes back. +(t/deftest ^:async permission-restoration-resumes-an-edit-already-attempted + (let [calls (atom 0) + timer-s (rx/subject)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [_] timer-s))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= :retrying (get-in @store [:persistence :status])) + "transient failure retries")) + (ptk/emit! store #(assoc-in % [:permissions :can-edit] false)) + (rx/push! timer-s :tick) + (await (async/wait-for #(= :save-permission-denied + (get-in @store [:persistence :error :code])) + "the retry fails without permission")) + (t/is (= 1 (count @requests))) + (ptk/emit! store + #(assoc-in % [:permissions :can-edit] true) + (ptk/data-event :app.main.data.common/change-team-role)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "restoration saves the attempted edit")) + (t/is (= 2 (count @requests))) + (t/is (apply = (map (comp :commit-id second) @requests))) + (rx/end! timer-s)))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/of {:revn 1}))))))) ;; Scenario: an edit is queued before persistence even initializes. On ;; initialization plus force-persist it is sent exactly once (still queued