✨ Send a failed save again under its own commit id

This commit is contained in:
alonso.torres 2026-09-24 10:10:02 +02:00
parent 67466d7406
commit e31f0fff11
4 changed files with 190 additions and 63 deletions

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

View File

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

View File

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

View File

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