github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 14:00:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

✨ Keep trying a failed save at a slow pace

Browse Source
This commit is contained in:
alonso.torres 2026-09-24 10:15:33 +02:00
parent e31f0fff11
commit 275989e5b5
3 changed files with 178 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.serena/memories/frontend/workspace-state-persistence-subtleties.md
View File

@ -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.

89
frontend/src/app/main/data/persistence.cljs
View File

@ -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
[]

121
frontend/test/frontend_tests/data/persistence_retry_test.cljs
View File

@ -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})))))))