✨ Wait previous fail state before retry

.serena/memories/frontend/workspace-state-persistence-subtleties.md

@@ -24,7 +24,7 @@
 - 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`.
 - 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, the browser `online` event (guarded by `exists? js/window`; re-enters the runner only for a live `:retrying` episode), and new local edits (`append-commit` re-enters the runner during `:retrying`; the emission retires the stuck runner via its stopper).
+- 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.
 - Undo transactions can stay open only temporarily; timed-out pending transactions are force-committed after about 20s. Undo entries are capped at 50.
 - Undo/redo are ignored while a normal editor/drawing interaction is active, except grid-layout edition handles undo through this path.

frontend/src/app/main/data/persistence.cljs

@@ -232,14 +232,11 @@
       ptk/WatchEvent
       (watch [_ state _]
         (let [pstate (:persistence state)]
-          ;; A new edit during `:retrying` re-enters the runner even though
-          ;; the run id no longer matches: the previous runner is stuck
-          ;; behind the failed head, and the emission itself retires it
-          ;; through its stopper. Concurrent re-entries collapse to a
-          ;; single send via the in-flight guard in `attempt-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))
-                     (or (= run-id (:run-id pstate))
-                         (= :retrying (:status pstate))))
+                     (= run-id (:run-id pstate)))
             (rx/of (update-status :saving)
                    (run-persistence-task))))))))
 

frontend/src/app/main/errors.cljs

@@ -400,6 +400,12 @@
     (dom/trigger-download-uri "report" "text/plain" uri)
     (ts/schedule-on-idle #(wapi/revoke-uri uri))))
 
+(defn- report-format
+  "Payload format for `cause`: compact for environment failures, full
+  otherwise."
+  [cause]
+  (if (environment-error? cause) :compact :full))
+
 (defn- emit-flash-report!
   "Reserves, generates and emits the flash report. Returns the generated
   report string, or nil when nothing is emitted (non-exception cause,
@@ -410,7 +416,7 @@
                             :handled "handled-exception"
                             :unhandled "unhandled-exception"
                             :silent nil)]
-      (let [format      (if (environment-error? cause) :compact :full)
+      (let [format      (report-format cause)
             report-hint (ex/get-hint cause)]
         (when (and (string? report-hint) (not (str/empty? report-hint)))
           (when-let [{:keys [occurrences]} (reserve! event-name cause)]
@@ -432,6 +438,9 @@
 
   The report is reserved before being generated, so repeated errors that
   fall inside the governor window do not pay the report-building cost.
+  With `:report-link?` the toast always carries a download link: when no
+  report is emitted (suppressed repeat, empty hint, `:silent`), one is
+  generated for the link alone, without emitting it.
 
   The whole body (report pipeline first, toast after) runs inside a single
   `ts/schedule` callback: nothing report- or toast-related executes
@@ -457,19 +466,23 @@
    (fn [resolve _reject]
      (ts/schedule
       (fn []
-        (let [report (try (emit-flash-report! type cause)
-                          (catch :default err
-                            (.error js/console "error on emitting report" err)
-                            nil))]
+        (let [report      (try (emit-flash-report! type cause)
+                               (catch :default err
+                                 (.error js/console "error on emitting report" err)
+                                 nil))
+              link-report (when report-link?
+                            (or report
+                                (when (ex/exception? cause)
+                                  (generate-report cause {:format (report-format cause)}))))]
           (try (st/emit!
                 (ntf/show
                  (cond-> {:content (or ^boolean hint (tr "errors.generic"))
                           :type :toast
                           :level :error
                           :timeout timeout}
-                   (and report-link? report)
+                   link-report
                    (assoc :links [{:label (tr "labels.download" "report.txt")
-                                   :callback (partial download-report! report)}]))))
+                                   :callback (partial download-report! link-report)}]))))
                (catch :default err
                  (.error js/console "error on emitting toast" err)))
           (resolve report)))))))

frontend/test/frontend_tests/data/persistence_retry_test.cljs

@@ -179,9 +179,9 @@
        (fn [_ _] (rx/throw (ex-info "offline" {:type :offline})))))))
 
 ;; Scenario: a retry timer fires while the replacement request is still in
-;; flight. The timer is driven by hand: fail the first send, queue a second
-;; edit so its run resends (hanging), then fire the pending timer. Proves:
-;; the firing is skipped instead of double-sending the same commit.
+;; flight. The timer is driven by hand: fail the first send, let the
+;; online signal resend the head (hanging), then fire the pending timer.
+;; Proves: the firing is skipped instead of double-sending the same commit.
 (t/deftest ^:async retry-skips-resend-while-previous-request-is-in-flight
   (let [calls   (atom 0)
         delays  (atom [])
@@ -198,11 +198,10 @@
             (await (async/wait-for #(= :retrying (get-in @store [:persistence :status]))
                                    "transient failure retries"))
             (t/is (= 1 (count @requests)))
-            ;; Phase 2 — a new edit re-enters the runner under the live
-            ;; episode (status stays :retrying) and resends the head,
-            ;; hanging in flight.
-            (ptk/emit! store (local-commit file-id) ::dps/force-persist)
-            (await (async/wait-for #(= 2 (count @requests)) "second edit resends"))
+            ;; Phase 2 — the online signal resends the head under the live
+            ;; episode (status stays :retrying), hanging in flight.
+            (ptk/emit! store (#'dps/resume-on-online))
+            (await (async/wait-for #(= 2 (count @requests)) "online resends"))
             (t/is (= :retrying (get-in @store [:persistence :status])))
             (t/is (= 1 (get-in @store [:persistence :attempts])))
             ;; Phase 3 — the pending timer fires into the in-flight request:
@@ -211,7 +210,7 @@
             (await (async/settle))
             (t/is (= 2 (count @requests)) "no double-send while in flight")
             (t/is (= :retrying (get-in @store [:persistence :status])))
-            (t/is (= 2 (count (get-in @store [:persistence :queue]))))
+            (t/is (= 1 (count (get-in @store [:persistence :queue]))))
             (rx/end! timer-s))))
        (fn [_ _]
          (if (= 1 (swap! calls inc))
@@ -242,6 +241,51 @@
           (t/is (nil? (get-in @store [:persistence :error]))))))
      (fn [_ _] (rx/throw (ex-info "offline" {:type :offline}))))))
 
+;; Scenario: a new edit lands while the episode waits on its backoff timer.
+;; The edit only joins the queue: nothing is sent and no attempt is spent
+;; until the timer fires. The retry then saves the head and the runner
+;; sends the new edit after it. Proves: edits during an episode never
+;; bypass the backoff nor consume the retry budget.
+(t/deftest ^:async new-edit-during-retry-waits-for-the-backoff
+  (let [calls   (atom 0)
+        delays  (atom [])
+        timer-s (rx/subject)]
+    (await
+     (with-persistence
+       (^:async fn [{:keys [file-id requests store]}]
+         (await
+          (mock/with-mocks*
+            {rx/timer (mock/stub (fn [ms] (swap! delays conj ms) timer-s))}
+            ;; Phase 1 — first send fails transiently; the retry pends on
+            ;; the hand-fired timer.
+            (ptk/emit! store (local-commit file-id) ::dps/force-persist)
+            (await (async/wait-for #(= :retrying (get-in @store [:persistence :status]))
+                                   "transient failure retries"))
+            (t/is (= 1 (count @requests)))
+            ;; Phase 2 — a new edit joins the queue without resending.
+            (ptk/emit! store (local-commit file-id) ::dps/force-persist)
+            (await (async/wait-for #(= 2 (count (get-in @store [:persistence :queue])))
+                                   "second edit is queued"))
+            (await (async/settle))
+            (t/is (= 1 (count @requests)) "the edit does not resend the head")
+            (t/is (= 1 (get-in @store [:persistence :attempts])) "no attempt is spent")
+            (t/is (= [2000] @delays) "the pending backoff is kept")
+            (t/is (= :retrying (get-in @store [:persistence :status])))
+            ;; Phase 3 — the timer fires: the head saves, then the new edit.
+            (rx/push! timer-s :tick)
+            (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status]))
+                                         (empty? (get-in @store [:persistence :queue])))
+                                   "both edits save"))
+            (t/is (= 3 (count @requests)) "failed send, retry, then the new edit")
+            (let [[first-id retry-id edit-id] (map (comp :commit-id second) @requests)]
+              (t/is (= first-id retry-id) "the retry resends the head")
+              (t/is (not= retry-id edit-id) "the new edit is sent after the head"))
+            (rx/end! timer-s))))
+       (fn [_ _]
+         (if (= 1 (swap! calls inc))
+           (rx/throw (ex-info "offline" {:type :offline}))
+           (rx/of {:revn @calls})))))))
+
 ;; Scenario: a transient failure raises the reconnect notice; the retry
 ;; then succeeds. The timer is driven by hand so the test observes the
 ;; episode mid-flight: one visible notice (the store holds a single toast,
@@ -342,11 +386,11 @@
   (filter #(= ::ev/event (ptk/type %)) events))
 
 ;; Scenario: the `online` event arrives while a retry request is already in
-;; flight. The first send fails transiently, a new edit re-enters and
-;; resends (hanging), then connectivity reports back mid-flight. Proves:
-;; the online entry point honors the in-flight guard instead of
-;; double-sending — the same guard as the retry-timer path, through
-;; `resume-on-online` -> `run-persistence-task`.
+;; flight. The first send fails transiently, the retry timer resends
+;; (hanging), then connectivity reports back mid-flight. Proves: the online
+;; entry point honors the in-flight guard instead of double-sending — the
+;; same guard as the retry-timer path, through `resume-on-online` ->
+;; `run-persistence-task`.
 (t/deftest ^:async online-event-does-not-resend-an-in-flight-request
   (let [calls   (atom 0)
         timer-s (rx/subject)]
@@ -362,10 +406,10 @@
             (await (async/wait-for #(= :retrying (get-in @store [:persistence :status]))
                                    "transient failure retries"))
             (t/is (= 1 (count @requests)))
-            ;; Phase 2 — a new edit re-enters the runner under the live
-            ;; episode and resends the head, hanging in flight.
-            (ptk/emit! store (local-commit file-id) ::dps/force-persist)
-            (await (async/wait-for #(= 2 (count @requests)) "second edit resends"))
+            ;; Phase 2 — the retry timer fires and resends the head,
+            ;; hanging in flight.
+            (rx/push! timer-s :tick)
+            (await (async/wait-for #(= 2 (count @requests)) "retry resends"))
             (t/is (= :retrying (get-in @store [:persistence :status])))
             ;; Phase 3 — online arrives mid-flight: silent, never a third send.
             (ptk/emit! store (#'dps/resume-on-online))

frontend/test/frontend_tests/errors_governor_test.cljs

@@ -357,3 +357,47 @@
         tm/schedule         (mock/stub (fn [f] (mock/asap f)))}
        (await (errors/flash :cause cause :type :handled))
        (t/is (= 1 (count @events)) "only the toast survives a reporting failure")))))
+
+(defn- toast-links
+  "The download links of every toast collected through the `st/emit!`
+  double, one entry per toast."
+  [events]
+  (->> events
+       (remove #(= ::ev/event (ptk/type %)))
+       (map #(get-in (ptk/update % {}) [:notification :links]))))
+
+(t/deftest ^:async repeated-flash-keeps-the-report-download-link
+  ;; Scenario: the same save failure flashes twice inside the governor
+  ;; window with a report link. The repeat is not emitted, but its toast
+  ;; still offers the report. Proves: the governor bounds emission, never
+  ;; the user's download.
+  (let [generated (atom 0)
+        cause     (error-cause :type :validation :hint "save failed")
+        events    (atom [])]
+    (await
+     (mock/with-mocks*
+       {st/format-last-events (mock/stub (fn [& _] (swap! generated inc) "report"))
+        st/emit!              (mock/stub (fn [& emitted] (swap! events into emitted)))
+        rt/get-current-href   (constantly "https://penpot.example.com/#/workspace")
+        tm/schedule           (mock/stub (fn [f] (mock/asap f)))}
+       (dotimes [_ 2]
+         (await (errors/flash :cause cause :type :handled :report-link? true)))
+       (t/is (= 1 (count (report-events @events))) "the repeat is not emitted")
+       (t/is (= [1 1] (map count (toast-links @events))) "both toasts carry the link")
+       (t/is (= 2 @generated) "the repeat builds its report for the link only")))))
+
+(t/deftest ^:async unreportable-flash-keeps-the-report-download-link
+  ;; Scenario: a failure with an empty hint flashes with a report link. It
+  ;; cannot be emitted, but its toast still offers the report. Proves: the
+  ;; link does not depend on emission.
+  (let [cause  (ex-info "" {:type :validation})
+        events (atom [])]
+    (await
+     (mock/with-mocks*
+       {st/format-last-events (mock/stub (fn [& _] "report"))
+        st/emit!              (mock/stub (fn [& emitted] (swap! events into emitted)))
+        rt/get-current-href   (constantly "https://penpot.example.com/#/workspace")
+        tm/schedule           (mock/stub (fn [f] (mock/asap f)))}
+       (await (errors/flash :cause cause :type :handled :report-link? true))
+       (t/is (empty? (report-events @events)) "nothing is emitted")
+       (t/is (= [1] (map count (toast-links @events))) "the toast carries the link")))))