From 275989e5b5e6c653b471eeea33df83b2d8666147 Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Thu, 24 Sep 2026 10:15:33 +0200 Subject: [PATCH] :sparkles: Keep trying a failed save at a slow pace --- .../workspace-state-persistence-subtleties.md | 2 +- frontend/src/app/main/data/persistence.cljs | 89 +++++++++---- .../data/persistence_retry_test.cljs | 121 +++++++++++++++++- 3 files changed, 178 insertions(+), 34 deletions(-) diff --git a/.serena/memories/frontend/workspace-state-persistence-subtleties.md b/.serena/memories/frontend/workspace-state-persistence-subtleties.md index 06bd2ae269..11679c62af 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`/`: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`. +- 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 the terminal path). A transient failure that exhausts the backoff arms a slow cycle (`slow-retry-delay-ms` 30s): `resume-persistence true` sets `:recovering`, so a transient failure goes straight back to `:error` (one send per cycle, no reconnect notice, no re-flash) and re-arms the cycle; `:recovering` and `:failing-since` clear when a head saves. The cycle and edit-driven resumes stop after `retry-give-up-ms` (24h, the backend redis TTL of a commit id). `:commit-in-progress` (backend turned away a repeat of a save still running) is transient. 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 14e6c7b7f3..257b98374f 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -27,6 +27,7 @@ (declare ^:private run-persistence-task) (declare ^:private persist-commit) (declare ^:private resume-persistence) +(declare ^:private slow-retry-cycle) (log/set-level! :warn) @@ -36,6 +37,11 @@ (def force-persist? #(= % ::force-persist)) +(def slow-retry-delay-ms + "Pause between the attempts of a queue whose backoff is spent: one attempt + per cycle, until it saves or the retry window closes." + 30000) + (def retry-give-up-ms "How long a failing queue keeps being sent. Matches how long the backend remembers a commit id." @@ -230,7 +236,7 @@ (throw (ex-info "invalid state" {}))))) (update :index dissoc commit-id) (assoc :last-progress-at (inst-ms (ct/now))) - (dissoc :stall-reported :failing-since + (dissoc :stall-reported :failing-since :recovering :attempts :retry-token :retry-for))))))) (defn- append-commit @@ -290,23 +296,35 @@ :attempts :retry-token :retry-for)))))) ptk/WatchEvent - (watch [_ _ _] - ;; The terminal toast supersedes the reconnect notice; hide it - ;; explicitly instead of relying on the single-toast replacement. - (rx/of (ptk/data-event ::error cause) - (ntf/hide :tag reconnecting-tag))) + (watch [_ state stream] + (rx/merge + ;; The terminal toast supersedes the reconnect notice; hide it + ;; explicitly instead of relying on the single-toast replacement. + (rx/of (ptk/data-event ::error cause) + (ntf/hide :tag reconnecting-tag)) + ;; A transport failure that outlasts the backoff can still pass, so + ;; the queue keeps trying at a slow pace. + (if (and (transient-error? (ex-data cause)) + (retry-window-open? (:persistence state))) + (slow-retry-cycle stream) + (rx/empty)))) ptk/EffectEvent - (effect [_ _ _] - ;; Report without invoking global handlers that may reload the file or - ;; navigate away before the user can recover the retained changes. - (errors/flash-persistence cause)))) + (effect [_ state _] + ;; A failed slow attempt repeats a failure the user was already warned + ;; about. Report without invoking global handlers that may reload the + ;; file or navigate away before the user can recover the retained + ;; changes. + (when-not (and (dm/get-in state [:persistence :recovering]) + (transient-error? (ex-data cause))) + (errors/flash-persistence cause))))) (defn- persistence-transient-failure "Transient save failure: the head commit stays queued and a retry is scheduled with backoff instead of parking the save in `:error`. Once the - budget (`retry-delays-ms`) is exhausted, the failure falls through to the - terminal `persistence-failed` path unchanged." + budget (`retry-delays-ms`) is exhausted, or when the failed attempt was + one of the slow cycle, the failure falls through to the terminal + `persistence-failed` path, which schedules the next slow attempt." [commit-id cause] (ptk/reify ::persistence-transient-failure ptk/UpdateEvent @@ -327,8 +345,8 @@ ptk/WatchEvent (watch [_ state _] - (let [attempts (dm/get-in state [:persistence :attempts])] - (if (> attempts (count retry-delays-ms)) + (let [{:keys [attempts recovering]} (:persistence state)] + (if (or recovering (> attempts (count retry-delays-ms))) (rx/of (persistence-failed commit-id cause)) (rx/merge ;; One notice per episode: shown on the first attempt, @@ -493,19 +511,36 @@ (ntf/hide :tag reconnecting-tag))))))) (defn- resume-persistence - [] - (ptk/reify ::resume-persistence - ptk/UpdateEvent - (update [_ state] - (update state :persistence - (fn [pstate] - (-> pstate - (dissoc :error :attempts :retry-token :retry-for) - (assoc :run-id (uuid/next) :status :saving) - (update :last-progress-at d/nilv (inst-ms (ct/now))))))) - ptk/WatchEvent - (watch [_ _ _] - (rx/of (run-persistence-task))))) + "Starts the queue again. A `recovering` resume is an attempt of the slow + cycle, which sends once instead of starting a backoff episode." + ([] (resume-persistence false)) + ([recovering] + (ptk/reify ::resume-persistence + ptk/UpdateEvent + (update [_ state] + (update state :persistence + (fn [pstate] + (-> pstate + (dissoc :error :attempts :retry-token :retry-for) + (assoc :run-id (uuid/next) + :status :saving + :recovering recovering) + (update :last-progress-at d/nilv (inst-ms (ct/now))))))) + ptk/WatchEvent + (watch [_ _ _] + (rx/of (run-persistence-task)))))) + +(defn- slow-retry-cycle + "Resumes a failed queue after `slow-retry-delay-ms`, unless the queue is + resumed some other way, persistence restarts or the workspace closes." + [stream] + (let [stopper-s (rx/merge + (rx/filter (ptk/type? ::resume-persistence) stream) + (rx/filter (ptk/type? ::initialize-persistence) stream) + (rx/filter (ptk/type? ::dw/finalize-workspace) stream))] + (->> (rx/timer slow-retry-delay-ms) + (rx/map (fn [_] (resume-persistence true))) + (rx/take-until stopper-s)))) (defn- recover-persistence [] diff --git a/frontend/test/frontend_tests/data/persistence_retry_test.cljs b/frontend/test/frontend_tests/data/persistence_retry_test.cljs index 31c8ed346c..10ba3cc0b4 100644 --- a/frontend/test/frontend_tests/data/persistence_retry_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_retry_test.cljs @@ -80,10 +80,19 @@ (rx/dispose! store) (rx/end! response)))))) +(defn- instant-backoff + "Timer double: the backoff delays fire at once and the slow cycle never + does, so a permanent failure settles in `:error`." + [ms] + (if (= ms dps/slow-retry-delay-ms) + (rx/subject) + (rx/of :tick))) + (defn- check-failed-save-response "Feeds `result` as the transport response and asserts the commit stays queued as a failed save instead of being treated as persisted. Retry - timers fire instantly (stubbed `rx/timer`), so a permanently bad answer + timers fire instantly (stubbed `rx/timer`, slow cycle held), so a + permanently bad answer exhausts the 3-retry budget and lands terminal: `:error` carrying `:invalid-save-response`, queue intact, exactly 4 sends." [result] @@ -91,7 +100,7 @@ (^:async fn [{:keys [file-id requests store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + {rx/timer (mock/stub instant-backoff)} (ptk/emit! store (local-commit file-id) ::dps/force-persist) (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) (= :invalid-save-response @@ -138,7 +147,7 @@ (^:async fn [{:keys [file-id requests store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + {rx/timer (mock/stub instant-backoff)} (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]))) @@ -166,7 +175,7 @@ (^:async fn [{:keys [file-id failures requests store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [ms] (swap! delays conj ms) (rx/of :tick)))} + {rx/timer (mock/stub (fn [ms] (swap! delays conj ms) (instant-backoff ms)))} (ptk/emit! store (local-commit file-id) ::dps/force-persist) (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) (= 4 (count @requests))) @@ -174,7 +183,8 @@ (t/is (= :error (get-in @store [:persistence :status]))) (t/is (= :save-failed (get-in @store [:persistence :error :code]))) (t/is (= 1 (count (get-in @store [:persistence :queue])))) - (t/is (= [2000 8000 20000] @delays) "the backoff schedule fires in order") + (t/is (= [2000 8000 20000 dps/slow-retry-delay-ms] @delays) + "the backoff schedule fires in order, then the slow cycle is armed") (t/is (= 1 (count @failures)) "exhaustion flashes exactly once")))) (fn [_ _] (rx/throw (ex-info "offline" {:type :offline}))))))) @@ -330,7 +340,7 @@ (^:async fn [{:keys [file-id store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + {rx/timer (mock/stub instant-backoff)} (ptk/emit! store (local-commit file-id) ::dps/force-persist) (await (async/wait-for #(= :error (get-in @store [:persistence :status])) "retries exhaust into terminal error")) @@ -485,3 +495,102 @@ (rx/dispose! store) (rx/end! ticks) (rx/end! response))))))) + +;; --------------------------------------------------------------------------- +;; slow cycle +;; --------------------------------------------------------------------------- + +(defn- slow-cycle-timer + "Timer double: the backoff delays fire at once, and every slow cycle waits + on a subject collected in `cycles`, so the test fires them one by one." + [cycles] + (fn [ms] + (if (= ms dps/slow-retry-delay-ms) + (let [cycle-s (rx/subject)] + (swap! cycles conj cycle-s) + cycle-s) + (rx/of :tick)))) + +;; Scenario: the backoff is spent while the network is down. The queue keeps +;; trying on the slow cycle, one send per cycle, without warning the user +;; again, and saves once the network is back. Proves: a session outlasting +;; the backoff still recovers on its own. +(t/deftest ^:async a-spent-backoff-keeps-trying-on-a-slow-cycle + (let [calls (atom 0) + cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id failures requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= 1 (count @cycles))) + "the spent backoff arms the slow cycle")) + (t/is (= 4 (count @requests))) + (t/is (= 1 (count @failures))) + + (rx/push! (last @cycles) :tick) + (await (async/wait-for #(= 2 (count @cycles)) + "a failed slow attempt arms the next cycle")) + (t/is (= 5 (count @requests)) "a slow cycle sends once") + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (= 1 (count @failures)) "the user is not warned again") + + (rx/push! (last @cycles) :tick) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the slow attempt saves")) + (t/is (= 6 (count @requests))) + (t/is (apply = (map (comp :commit-id second) @requests)) + "every attempt carries the same commit id") + (t/is (nil? (get-in @store [:persistence :recovering])))))) + (fn [_ _] + (if (< (swap! calls inc) 6) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/of {:revn 1}))))))) + +;; Scenario: a save fails for a reason no attempt can get past. Proves: only +;; transport failures arm the slow cycle. +(t/deftest ^:async a-terminal-failure-does-not-arm-the-slow-cycle + (let [cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "the save fails")) + (await (async/settle)) + (t/is (= 1 (count @requests))) + (t/is (empty? @cycles))))) + (fn [_ _] (rx/throw (ex-info "Validation failed" {:type :validation}))))))) + +;; Scenario: the slow cycle is still failing when the retry window closes. +;; Proves: the cycle stops once the backend may have forgotten the commit +;; id, keeping the edits queued. +(t/deftest ^:async the-slow-cycle-stops-when-the-retry-window-closes + (let [clock (atom 0) + cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles)) + ct/now (mock/stub #(ct/inst @clock))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (count @cycles)) "the slow cycle is armed")) + + (reset! clock (+ dps/retry-give-up-ms 1000)) + (rx/push! (last @cycles) :tick) + (await (async/wait-for #(= 5 (count @requests)) "the last slow attempt")) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "the attempt fails")) + (await (async/settle)) + (t/is (= 1 (count @cycles)) "no further cycle is armed") + (t/is (= 1 (count (get-in @store [:persistence :queue]))))))) + (fn [_ _] (rx/throw (ex-info "offline" {:type :offline})))))))