diff --git a/.serena/memories/clojure/idioms.md b/.serena/memories/clojure/idioms.md index c8672aa647..74a25326d9 100644 --- a/.serena/memories/clojure/idioms.md +++ b/.serena/memories/clojure/idioms.md @@ -1,10 +1,10 @@ # Clojure Idioms (verified) -Behaviors confirmed against the language/stdlib — do not re-derive from -assumption; a wrong assumption here already cost a review round. +Behaviors confirmed against the language/stdlib — do not re-derive from assumption; a wrong assumption here already cost a review round. -- `int?` is NOT 32-bit-only: true for `Long`, `Integer`, `Short`, - `Byte` (fixed-precision integers). Clojure integer literals are - `Long`, so `(int? 5000)` is true. -- `integer?` is the general integer predicate; prefer it when any - integer kind must match, `int?` only when fixed precision is meant. +- `int?` is NOT 32-bit-only: true for `Long`, `Integer`, `Short`, `Byte` (fixed-precision integers). Clojure integer literals are `Long`, so `(int? 5000)` is true. +- `integer?` is the general integer predicate; prefer it when any integer kind must match, `int?` only when fixed precision is meant. +- `await` is a `cljs.core` macro asserting `(:async &env)`: it fails at compile time outside an `^:async` context, never silently. +- The analyzer reads `:async` only from the fn name meta and the `fn` operator meta; list-level meta is ignored (would make a MetaFn). `(fn ^:async [] …)` puts the meta on argv (pre/post only, NOT async). +- Valid: `(defn ^:async f)`, `(defn- ^:async f)`, `(^:async fn [] …)` (the latter is what stock `t/async` generates itself). +- `^:async` fns never throw synchronously (rejected promises instead); `try/catch/finally` supported; continuations are microtasks, timers and RxJS schedulers are macrotasks (FIFO). diff --git a/.serena/memories/frontend/routing-app-shell-subtleties.md b/.serena/memories/frontend/routing-app-shell-subtleties.md index 537c9591a9..ea3e98243d 100644 --- a/.serena/memories/frontend/routing-app-shell-subtleties.md +++ b/.serena/memories/frontend/routing-app-shell-subtleties.md @@ -12,7 +12,7 @@ - `app.main.errors/submit-report` is governed by a dedup governor: each report carries a fingerprint (`report-name|type|code|hint|first stack frame`; the report name is part of it so a handled report never coalesces with an unhandled/exception-page one), the first occurrence is always emitted, repeats within 2 minutes are counted and included in the next emitted report as `:occurrences`, and the fingerprint cache is bounded (first-inserted entry evicted, FIFO, via `:order` queue) so memory stays fixed. It applies to `handled-exception`, `unhandled-exception` and `exception-page`; a report without an exception cause is ignored and does not consume a reservation. - Errors caused by the environment (`environment-error-types`: `:network`, `:offline`, `:bad-gateway`, `:service-unavailable`, `:nitrate-unavailable`, `:nitrate-not-configured`) are not application defects: they are reported as audit-only `handled-exception` (never `unhandled-exception`/`exception-page`, so they skip internal reports and alerts), with a compact report and a fingerprint that drops the stack frame. `:offline` has its own handler and no longer falls through to `:default`; `:network`/`:offline` show the `errors.connection-error` toast. - `generate-report` accepts an explicit `{:format :compact|:full}` (default `:full`) chosen by its caller (`flash`, `exception-section*`): `:compact` keeps the context header plus type/code/uri, and skips the stack, the `ex-data` dump and the last-events list. It is total: if formatting fails it returns a minimal fallback string instead of nil, so an already reserved emission is never dropped. -- `flash` reserves the report before generating it, so suppressed occurrences do not pay the `generate-report` cost; the toast is unchanged. `flash` derives only the payload format from the cause (`environment-error?` → `:compact`); the audit event name is the canonical one requested by `:type` (`handled-exception`/`unhandled-exception`) and is never reclassified, because external tools filter on those names. `exception-section*` picks `handled-exception`/`exception-page` explicitly per cause; `flash-persistence` only adds the toast hint. +- `flash` runs its whole body (report pipeline first, toast after) inside a single `ts/schedule` callback: nothing executes synchronously on the error handler's stack. The report is reserved before being generated, so suppressed occurrences do not pay the `generate-report` cost; a reporting or notification failure is logged to the console and never propagates, and the toast is still attempted. `flash` returns a total promise (never rejects) resolving with the generated report, or nil when nothing is emitted, once the callback completes; production callers ignore it, tests await it. `flash` derives only the payload format from the cause (`environment-error?` → `:compact`); the audit event name is the canonical one requested by `:type` (`handled-exception`/`unhandled-exception`) and is never reclassified, because external tools filter on those names. `exception-section*` picks `handled-exception`/`exception-page` explicitly per cause; `flash-persistence` only adds the toast hint. ## Store and websocket diff --git a/.serena/memories/frontend/testing.md b/.serena/memories/frontend/testing.md index d657214ea5..7c6bfec777 100644 --- a/.serena/memories/frontend/testing.md +++ b/.serena/memories/frontend/testing.md @@ -6,7 +6,39 @@ Frontend validation: CLJS + React/Rumext + RxJS/Potok; SCSS modules; shared CLJC READ `mem:testing` FIRST — it defines the execution discipline (no piping, tee to file, preferred commands) that applies to all CLJS/JS test runs. -Frontend unit tests live under `frontend/test/frontend_tests/` and use `cljs.test`. They should be deterministic, avoid DOM/UI integration where possible, and mock side effects such as RPC, storage, timers, or network access. Mock through `frontend-tests.helpers.mock`: prefer `mock/with-mocks` (installs with `set!`, so it survives async boundaries) over `with-redefs`. The `:esm` test build dispatches calls to multi-arity vars as `cljs$core$IFn$_invoke$arity$N`, so stub multi-arity vars with `mock/stub` (arities 0-6); for variadic call sites with more than 6 args use a plain variadic `fn` instead. A mock must not call the mocked var again (self-delegation inside a multi-arity function recurses). Async tests wrap the body in `t/async` and thread its `done` into `mock/with-mocks` as the outer callback; `done'` must be called exactly once (calling it twice only prints a warning; not calling it stalls the run and leaks the mocks). +Frontend unit tests live under `frontend/test/frontend_tests/` and use `cljs.test`. They should be deterministic, avoid DOM/UI integration where possible, and mock side effects such as RPC, storage, timers, or network access. + +### Async-first stance + +Frontend testing is async-first: everything essentially asynchronous is modeled with a test reproducing the asynchrony, even when the test could be written "synchronously". Sync-passing tests prove nothing about async behavior and rot as soon as an async boundary appears downstream. Consequences: mock through `frontend-tests.helpers.mock`, never `with-redefs`, except unit tests of purely synchronous functions; transport doubles deliver asynchronously (`observe-on :async`) while the test keeps scenario timing (explicit pushes); assertions always follow quiescence (`wait-for` on presence, bare `settle` tick for absence-only blocks), never a trigger. + +### Primitives + +- `mock/with-mocks` (callback style, legacy compat): installs with `set!` so mocks survive async boundaries; bodies run deferred past the current tick via `asap`, so only done-chained (`t/async`) contexts are allowed; `done'` restores and completes exactly once (twice only warns; never calling it stalls the run and leaks the mocks). Prefer `mock/with-mocks*` for new tests. +- `mock/with-mocks*` (direction): body forms wrapped in a generated `^:async` fn, evaluates to a promise — `await` it, `await` nested scopes too, no `done` in test code. Rejections and non-promise returns report as `:error` via `run-mocked`. +- `mock/stub` wraps fns for arities 0-6 (the `:esm` test build dispatches multi-arity vars as `cljs$core$IFn$_invoke$arity$N`); when the mocked var is variadic-defined (or called with more than 6 args), the call compiles to variadic dispatch, so use a plain variadic `fn` — the stub does not forward variadic. A mock must not call the mocked var again (self-delegation inside a multi-arity function recurses). +- Helpers in `frontend-tests.helpers.async`: `->promise` (single-value observable → promise; beicon has no `to-promise`), `await-response` (subscribe→push→await, atomic), `settle`, `wait-for` (immediate check + bounded poll, fails instead of hanging), `observe` (stream → termination promise; asserts provided, timeout rejects). +- Fixtures return promises (`with-watchdog`, `with-persistence`); `await` them from `^:async` tests. `main_errors` and `fonts` are fully migrated (no legacy `with-mocks` left). +- Valid `^:async` placements: `mem:clojure/idioms`. + +### Observing event streams + +To assert over emitted event sequences, observe termination: subscribe through `observe` (async delivery forced even for sync sources), `await` its promise, then assert the collected values. Never branch on nil (`when-let` skipping observation lets setup bugs pass as "empty"): producers answer refusals with empty streams, never nil, so every path subscribes uniformly. Observed termination is exact quiescence — no manual `settle` after it. Errors reject unless `:on-error` handles them. + +### Runner and library facts (verified: CLJS 1.12.145, beicon2 `df7058a`) + +- `cljs.test` keeps its env in a `set!` var: assertions inside deferred ticks count. `run-block`: double `done` only warns; missing `done` stalls. +- `t/async` discards the body promise — completion signals ONLY via `done`. `t/deftest ^:async` adds auto async-context + auto `done`, but awaiting stays the author's job; without it the test passes empty. +- `take 1` is per-subscription on a hot subject: subscribe-before-push or hang. `end!`/`.error` with pending takes only forwards valueless completion. + +### Traps that bit + +- `^:async` tests require map-style fixtures (`(t/use-fixtures :each {:before f})`): function-style fixtures abort the whole run ("Async tests require fixtures to be specified as maps"). +- `st/emit!` doubles collect heterogeneous events: audit `DataEvent`s deref, toast reify-objects do not — discriminate with `ptk/type` (total, never throws), never blind `deref`. +- Subscription order decides delivery order: never resolve settlement from a pre-subscribed branch racing the pipeline. +- Auto-answering mocks lose deadline expressiveness (can't time answers), need teardown timer-cancellation, and post-teardown deliveries hit real implementations — explicit pushes + async delivery + `wait-for` won on every axis. +- Teardown belongs to the terminal continuation, never to `finally`-around-triggers (it would dispose in-flight flows). +- A body that awaits must be `(^:async fn …)` even if the rest is sync; sync sequences are atomic vs the event loop. From `frontend/`: - Full unit test run (always builds, suppressed output): `pnpm run test:quiet`. diff --git a/.serena/memories/frontend/workspace-state-persistence-subtleties.md b/.serena/memories/frontend/workspace-state-persistence-subtleties.md index 70f0be8f86..b07fee51d3 100644 --- a/.serena/memories/frontend/workspace-state-persistence-subtleties.md +++ b/.serena/memories/frontend/workspace-state-persistence-subtleties.md @@ -22,6 +22,10 @@ - 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`. +- 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). +- 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. - After local commits and when render-wasm is active, text shapes get derived `:position-data` recomputed in a separate commit tagged `#{:position-data}`; that tag is excluded from the position-data watcher to avoid loops. diff --git a/frontend/src/app/main/data/changes.cljs b/frontend/src/app/main/data/changes.cljs index cc2bc548eb..865779e5dc 100644 --- a/frontend/src/app/main/data/changes.cljs +++ b/frontend/src/app/main/data/changes.cljs @@ -247,22 +247,26 @@ ;; Historical previews must not create edits to the live file. Check ;; this when creating commits so previously queued edits can still save. - (when (and (:can-edit permissions) - (not (dm/get-in state [:workspace-global :preview-id]))) - (log/trace :hint "commit-changes" :redo-changes redo-changes) - (let [selected (dm/get-in state [:workspace-local :selected])] - (rx/of (-> params - (assoc :undo-group undo-group) - (assoc :features features) - (assoc :tags tags) - (assoc :stack-undo? stack-undo?) - (assoc :save-undo? save-undo?) - (assoc :file-id file-id) - (assoc :file-revn (resolve-file-revn state file-id)) - (assoc :file-vern (resolve-file-vern state file-id)) - (assoc :undo-changes uchg) - (assoc :redo-changes rchg) - (assoc :selected-before selected) - (assoc :translation? translation?) - (assoc :skip-component-sync? skip-component-sync?) - (commit))))))))) + ;; Refusals answer with an empty stream (never nil) so callers can + ;; uniformly subscribe and observe termination. + (if (and (:can-edit permissions) + (not (dm/get-in state [:workspace-global :preview-id]))) + (do + (log/trace :hint "commit-changes" :redo-changes redo-changes) + (let [selected (dm/get-in state [:workspace-local :selected])] + (rx/of (-> params + (assoc :undo-group undo-group) + (assoc :features features) + (assoc :tags tags) + (assoc :stack-undo? stack-undo?) + (assoc :save-undo? save-undo?) + (assoc :file-id file-id) + (assoc :file-revn (resolve-file-revn state file-id)) + (assoc :file-vern (resolve-file-vern state file-id)) + (assoc :undo-changes uchg) + (assoc :redo-changes rchg) + (assoc :selected-before selected) + (assoc :translation? translation?) + (assoc :skip-component-sync? skip-component-sync?) + (commit))))) + (rx/empty)))))) diff --git a/frontend/src/app/main/data/persistence.cljs b/frontend/src/app/main/data/persistence.cljs index 709a88a51f..f2c6cfafeb 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -8,20 +8,24 @@ (:require [app.common.data :as d] [app.common.data.macros :as dm] + [app.common.exceptions :as ex] [app.common.logging :as log] [app.common.time :as ct] [app.common.uuid :as uuid] [app.main.data.changes :as dch] [app.main.data.common :as-alias dc] [app.main.data.helpers :as dsh] + [app.main.data.notifications :as ntf] [app.main.data.workspace :as-alias dw] [app.main.errors :as errors] [app.main.refs :as refs] [app.main.repo :as rp] + [app.util.i18n :refer [tr]] [beicon.v2.core :as rx] [potok.v2.core :as ptk])) (declare ^:private run-persistence-task) +(declare ^:private persist-commit) (log/set-level! :warn) @@ -35,6 +39,16 @@ (def ^:private saving-check-interval-ms 30000) (def ^:private save-wait-timeout-ms (* 2 60 1000)) +(defn terminal-status? + "True when a persistence snapshot releases waiters: a failed save, or + a settled (`nil` / `:saved`) empty queue. Anything else — `:pending`, + `:saving` and the `:retrying` episode — keeps waiting." + [{:keys [status queue]}] + (boolean + (or (= status :error) + (and (empty? queue) + (or (nil? status) (= status :saved)))))) + (defn wait-persisted-or-error "Returns an observable that emits the first terminal persistence status (nil | :saved) and completes. Raises when the queue has failed and, with @@ -42,10 +56,7 @@ ([] (wait-persisted-or-error save-wait-timeout-ms)) ([timeout-ms] (let [base (->> (rx/from-atom refs/persistence {:emit-current-value? true}) - (rx/filter (fn [{:keys [status queue]}] - (or (= status :error) - (and (empty? queue) - (or (nil? status) (= status :saved)))))) + (rx/filter terminal-status?) (rx/take 1) (rx/mapcat (fn [{:keys [status error]}] (if (= status :error) @@ -74,12 +85,15 @@ (rx/concat (rx/of ::force-persist) (wait-persisted timeout-ms)))) (defn- next-status - "Refuses downgrades: a save in progress stays :saving, and a failed save - stays :error until persistence is resumed." + "Refuses downgrades: a save in progress stays :saving, a failed save + stays :error until persistence is resumed, and a retrying episode stays + :retrying until it saves or errors (re-entries send under the episode + instead of resetting it)." [from to] (cond (and (= to :pending) (= from :saving)) from (and (= from :error) (#{:pending :saving} to)) from + (and (= from :retrying) (#{:pending :saving} to)) from :else to)) (defn- update-status @@ -98,14 +112,39 @@ (update :last-progress-at d/nilv (inst-ms (ct/now))) (#{:error :saved} status) - (dissoc :run-id :last-progress-at :stall-reported?)))))))) + (dissoc :run-id :last-progress-at :stall-reported)))))))) + +(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. + 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`." + [{:keys [type cause-type code]}] + (boolean + (or (contains? rp/retryable-types type) + (contains? rp/retryable-types cause-type) + (= :invalid-save-response code)))) + +(def ^:private retry-delays-ms + "Backoff delays (ms) between save retries; the count is the retry budget. + A plain value (not a dynamic var): dynamic bindings do not survive `await` + continuations, so tests instant-trigger retries by stubbing `rx/timer`." + [2000 8000 20000]) + +(def ^:private reconnecting-tag + "Tag of the single reconnect notice: re-showing replaces it by + construction (the store holds one toast), and it is hidden by tag on + save or on terminal failure." + :persistence-reconnecting) (defn- report-stalled-persistence [now] (ptk/reify ::report-stalled-persistence ptk/UpdateEvent (update [_ state] - (assoc-in state [:persistence :stall-reported?] true)) + (assoc-in state [:persistence :stall-reported] true)) ptk/EffectEvent (effect [_ state _] @@ -113,8 +152,8 @@ commit-id (peek queue) commit (get index commit-id) hint "File saving has made no progress for more than five minutes" - cause (ex-info hint - {:type :persistence + cause (ex/error :type :persistence + :hint hint :code :saving-stalled :file-id (or (:file-id commit) (:current-file-id state)) :commit-id commit-id @@ -123,12 +162,12 @@ :queued-commits (count queue) :elapsed-ms (- now last-progress-at) :can-edit (dm/get-in state [:permissions :can-edit]) - :read-only? (dm/get-in state [:workspace-global :read-only?]) + :read-only (dm/get-in state [:workspace-global :read-only?]) :preview-id (dm/get-in state [:workspace-global :preview-id]) - :render-context-lost? (dm/get-in state [:render-state :lost])})] + :render-context-lost? (dm/get-in state [:render-state :lost]))] (errors/submit-report :event-name "handled-exception" :hint hint - :report (errors/generate-report cause) + :report (fn [] (errors/generate-report cause)) :cause cause))))) (defn- check-persistence @@ -136,11 +175,12 @@ (ptk/reify ::check-persistence ptk/WatchEvent (watch [_ state _] - (let [{:keys [status last-progress-at stall-reported?]} (:persistence state) + (let [{:keys [status queue last-progress-at stall-reported]} (:persistence state) now (inst-ms (ct/now))] - (when (and (#{:pending :saving} status) + (when (and (#{:pending :saving :retrying} status) + (seq queue) last-progress-at - (not stall-reported?) + (not stall-reported) (> (- now last-progress-at) saving-stall-timeout-ms)) (rx/of (report-stalled-persistence now))))))) @@ -169,7 +209,8 @@ (throw (ex-info "invalid state" {}))))) (update :index dissoc commit-id) (assoc :last-progress-at (inst-ms (ct/now))) - (dissoc :stall-reported?))))))) + (dissoc :stall-reported + :attempts :retry-token :retry-for))))))) (defn- append-commit "Event used internally to append the current change to the @@ -191,8 +232,14 @@ 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`. (when (and (not= :error (:status pstate)) - (= run-id (:run-id pstate))) + (or (= run-id (:run-id pstate)) + (= :retrying (:status pstate)))) (rx/of (update-status :saving) (run-persistence-task)))))))) @@ -213,11 +260,15 @@ :commit-id commit-id :hint (ex-message cause) ::errors/handled? true)) - (dissoc :run-id :last-progress-at :stall-reported?)))))) + (dissoc :run-id :last-progress-at :stall-reported + :attempts :retry-token :retry-for)))))) ptk/WatchEvent (watch [_ _ _] - (rx/of (ptk/data-event ::error cause))) + ;; 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))) ptk/EffectEvent (effect [_ _ _] @@ -225,6 +276,59 @@ ;; navigate away before the user can recover the retained changes. (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." + [commit-id cause] + (ptk/reify ::persistence-transient-failure + ptk/UpdateEvent + (update [_ state] + ;; Always counts (even past the budget): the watch routes on the + ;; stored count, so it must read — never recompute — attempts. + (let [attempts (inc (dm/get-in state [:persistence :attempts] 0))] + (update state :persistence + (fn [pstate] + (-> pstate + (assoc :status :retrying + :attempts attempts + :retry-token (uuid/next) + :retry-for commit-id + :last-progress-at (inst-ms (ct/now))) + (dissoc :stall-reported)))))) + + ptk/WatchEvent + (watch [_ state _] + (let [attempts (dm/get-in state [:persistence :attempts])] + (if (> attempts (count retry-delays-ms)) + (rx/of (persistence-failed commit-id cause)) + (rx/merge + ;; One notice per episode: shown on the first attempt, + ;; re-showing would only replace the identical toast. + (when (= 1 attempts) + (rx/of (ntf/show {:content (tr "errors.save-retrying") + :type :toast + :level :warning + :tag reconnecting-tag}))) + (let [delay-ms (nth retry-delays-ms (dec attempts)) + token (dm/get-in state [:persistence :retry-token])] + (->> (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 @@ -235,7 +339,7 @@ (update [_ state] ;; Keep the acknowledgment even if the queue runner has stopped. (d/update-in-when state [:persistence :index (:id commit)] - assoc ::acknowledged? true)))) + assoc ::acknowledged true)))) (defn- update-file-request "Issues the `update-file` request, tracked as active for its lifetime." @@ -261,7 +365,7 @@ (cond (= :error (dm/get-in state [:persistence :status])) :halted (nil? commit) :missing-commit - (::acknowledged? commit) :acknowledged + (::acknowledged commit) :acknowledged (contains? @active-requests (::request-id commit)) :in-flight (and (::request-id commit) (not= request-id (::request-id commit))) :unknown-outcome @@ -295,19 +399,32 @@ :code :invalid-save-response :file-id file-id}))))) (rx/catch (fn [cause] - (rx/of (persistence-failed id cause))))))) + (rx/of ((if (transient-error? (ex-data cause)) + persistence-transient-failure + persistence-failed) + id cause))))))) (defn- persist-commit - [commit-id] + "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." + [commit-id & {:keys [token]}] (let [request-id (uuid/next)] (ptk/reify ::persist-commit ptk/UpdateEvent (update [_ state] - (if (= :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. - (assoc-in state [:persistence :index commit-id ::request-id] request-id) - 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)] + (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. + (assoc-in state [:persistence :index commit-id ::request-id] request-id) + state))) ptk/WatchEvent (watch [_ state _] @@ -318,17 +435,22 @@ :code code :commit-id commit-id :file-id (:file-id commit)}))))] - (case (attempt-state state commit-id request-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) - ;; Even :network and :offline do not prove that the server - ;; skipped the write. 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))))))) + (if (and (some? token) + (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) + :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)))))))) (defn- run-persistence-task @@ -361,7 +483,8 @@ (rx/take-until stoper-s))) :else - (rx/of (update-status :saved))))))) + (rx/of (update-status :saved) + (ntf/hide :tag reconnecting-tag))))))) (defn- resume-persistence [] @@ -371,7 +494,7 @@ (update state :persistence (fn [pstate] (-> pstate - (dissoc :error) + (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 @@ -386,7 +509,10 @@ (let [{:keys [queue index status error run-id]} (:persistence state) commit (get index (peek queue))] (cond + ;; A retrying episode owns its recovery through the backoff + ;; scheduler; resuming here would bypass the attempt budget. (and (seq queue) + (not= status :retrying) (or (not= status :error) (and (= :save-permission-denied (:code error)) (not (::request-id commit)) @@ -416,6 +542,19 @@ (assoc :redo-changes rchg) (assoc :changes rchg))))))) +(defn- resume-on-online + "Re-enters the runner for a retrying episode when the browser reports + connectivity back, instead of waiting out the backoff. Terminal failures + stay terminal: only a live `:retrying` episode resumes." + [] + (ptk/reify ::resume-on-online + ptk/WatchEvent + (watch [_ state _] + (let [{:keys [status queue]} (:persistence state)] + (if (and (seq queue) (= :retrying status)) + (rx/of (run-persistence-task)) + (rx/empty)))))) + (defn initialize-persistence [] (ptk/reify ::initialize-persistence @@ -453,6 +592,15 @@ (rx/map (fn [_] (check-persistence))) (rx/take-until stoper-s)) + ;; The browser knows when connectivity returns: re-enter the + ;; runner for a retrying episode instead of waiting out the + ;; backoff. No `window` outside the browser (tests, SSR). + (or (when (exists? js/window) + (->> (rx/from-event js/window "online") + (rx/map (fn [_] (resume-on-online))) + (rx/take-until stoper-s))) + (rx/empty)) + (->> notifier-s (rx/map #(ptk/data-event ::persistence-notification)) (rx/take-until stoper-s)) diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index c4f109f6f7..ceda6f90db 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -16,6 +16,7 @@ [app.main.data.nitrate :as dnt] [app.main.data.notifications :as ntf] [app.main.data.workspace :as-alias dw] + [app.main.repo :as rp] [app.main.router :as rt] [app.main.store :as st] [app.main.worker] @@ -136,13 +137,10 @@ (def environment-error-types "Error types produced by the environment rather than by an application - defect." - #{:network - :offline - :bad-gateway - :service-unavailable - :nitrate-unavailable - :nitrate-not-configured}) + defect: the shared transient transport set (`repo/retryable-types`) plus + the nitrate-specific failures." + (into rp/retryable-types #{:nitrate-unavailable + :nitrate-not-configured})) (defn environment-error? [cause] @@ -232,7 +230,9 @@ ;; ;; The fingerprint cache is bounded: when it is full, the fingerprint ;; inserted first is evicted (FIFO order), so memory cannot grow without -;; limit. +;; limit. Eviction drops the evicted fingerprint's pending counter with +;; it, so its next occurrence emits as a fresh report (`:occurrences` +;; 1): a bounded precision trade-off, not an accounting bug. (def report-window-ms "Minimum time between two reports with the same fingerprint." @@ -281,9 +281,16 @@ (str/prune hint 120))] (if (environment-error? cause) base - (let [;; A JS stack string starts with "Error: "; the first - ;; actual frame is the second line. - frame (or (some-> (.-stack cause) (str/lines) (second)) "")] + (let [;; A JS stack string starts with "Error: ". The first + ;; actual frame is the first subsequent line shaped like a + ;; frame: skipping the message line and matching on `(` tolerates + ;; wrapper-prepended stacks instead of trusting the position. + frame (or (some->> (.-stack cause) + (str/lines) + (drop 1) + (filter #(str/includes? % "(")) + (first)) + "")] (str base "|" (str/prune frame 120)))))) (defn- evict-oldest @@ -340,6 +347,17 @@ [fingerprint now] (swap! report-governor reserve-report* fingerprint now)) +(defn- reserve! + "Common reservation step shared by `submit-report` and the `flash` + pipeline: fingerprint the cause and ask the governor. Returns the + occurrence count when this report must be emitted, nil when the + governor suppresses it." + [event-name cause] + (let [state (reserve-report! (error-fingerprint event-name cause) + (inst-ms (ct/now)))] + (when (::emit state) + {:occurrences (::occurrences state)}))) + (defn- emit-report! "Emit the audit event for a report that is already reserved by the governor." @@ -357,17 +375,22 @@ `cause` must be the exception the report describes: a report without a cause is ignored (and does not consume a governor reservation), so every - report shares the same fingerprint format." + report shares the same fingerprint format. + + `report` is either the report string or a zero-arg function building + it: the function runs only when the governor grants emission, so + suppressed occurrences never pay the report-building cost." [& {:keys [event-name report hint cause] :or {event-name "unhandled-exception"}}] (when (and (ex/exception? cause) (not (str/empty? hint)) - (string? report) + (or (string? report) (fn? report)) (string? event-name)) - (let [state (reserve-report! (error-fingerprint event-name cause) - (inst-ms (ct/now)))] - (when (::emit state) - (emit-report! event-name report hint (::occurrences state)))))) + (when-let [{:keys [occurrences]} (reserve! event-name cause)] + (emit-report! event-name + (if (fn? report) (report) report) + hint + occurrences)))) (defn- download-report! [report event] @@ -377,6 +400,27 @@ (dom/trigger-download-uri "report" "text/plain" uri) (ts/schedule-on-idle #(wapi/revoke-uri uri)))) +(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, + `:silent` type, empty hint or denied governor reservation)." + [type cause] + (when (ex/exception? cause) + (when-let [event-name (case type + :handled "handled-exception" + :unhandled "unhandled-exception" + :silent nil)] + (let [format (if (environment-error? cause) :compact :full) + report-hint (ex/get-hint cause)] + (when (and (string? report-hint) (not (str/empty? report-hint))) + (when-let [{:keys [occurrences]} (reserve! event-name cause)] + (let [generated (generate-report cause {:format format})] + (emit-report! event-name + generated + report-hint + occurrences) + generated))))))) + (defn flash "Show error notification banner and emit error report. A nil timeout keeps the notification visible until dismissed or replaced. @@ -389,7 +433,19 @@ The report is reserved before being generated, so repeated errors that fall inside the governor window do not pay the report-building cost. - The notification is scheduled asynchronously (via tm/schedule) to + The whole body (report pipeline first, toast after) runs inside a single + `ts/schedule` callback: nothing report- or toast-related executes + synchronously on the error handler's stack. A failure while reporting or + notifying is logged to the console and never propagates; the toast is + still attempted. + + Returns a promise resolving with the generated report (or nil when + nothing is emitted) once the scheduled callback completes. The promise + is total: it never rejects. Production callers ignore it + (fire-and-forget); it exists so tests can await completion instead of + reasoning about timer order. + + The notification is scheduled asynchronously (via `ts/schedule`) to avoid pushing a new event into the potok store while the store's own error-handling pipeline is still on the call stack. Emitting synchronously from inside an error handler creates a re-entrant @@ -397,33 +453,26 @@ (RangeError: Maximum call stack size exceeded)." [& {:keys [type hint cause timeout report-link?] :or {type :handled timeout 5000}}] - (let [report (when (ex/exception? cause) - (when-let [event-name (case type - :handled "handled-exception" - :unhandled "unhandled-exception" - :silent nil)] - (let [format (if (environment-error? cause) :compact :full) - report-hint (ex/get-hint cause)] - (when (and (string? report-hint) (not (str/empty? report-hint))) - (let [state (reserve-report! (error-fingerprint event-name cause) (inst-ms (ct/now)))] - (when (::emit state) - (let [generated (generate-report cause {:format format})] - (emit-report! event-name - generated - report-hint - (::occurrences state)) - generated)))))))] - - (ts/schedule - #(st/emit! - (ntf/show - (cond-> {:content (or ^boolean hint (tr "errors.generic")) - :type :toast - :level :error - :timeout timeout} - (and report-link? report) - (assoc :links [{:label (tr "labels.download" "report.txt") - :callback (partial download-report! report)}]))))))) + (js/Promise. + (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))] + (try (st/emit! + (ntf/show + (cond-> {:content (or ^boolean hint (tr "errors.generic")) + :type :toast + :level :error + :timeout timeout} + (and report-link? report) + (assoc :links [{:label (tr "labels.download" "report.txt") + :callback (partial download-report! report)}])))) + (catch :default err + (.error js/console "error on emitting toast" err))) + (resolve report))))))) (defn- handle-connectivity-error "Report a failure caused by the user's connectivity. These are audit-only diff --git a/frontend/src/app/main/repo.cljs b/frontend/src/app/main/repo.cljs index 9f6703f6bf..01ce9e4c01 100644 --- a/frontend/src/app/main/repo.cljs +++ b/frontend/src/app/main/repo.cljs @@ -23,9 +23,12 @@ ;; -- Retry helpers ----------------------------------------------------------- -(def ^:private retryable-types +(def retryable-types "Set of error types that are considered transient and safe to retry - for idempotent (GET) requests." + for idempotent (GET) requests. Also the single source of truth for the + transient transport classification consumed by `persistence/transient-error?` + and `errors/environment-error-types`: extend this set (never a copy) + when a new retryable transport failure appears." #{:network ; js/fetch network-level failure :bad-gateway ; 502 :service-unavailable ; 503 diff --git a/frontend/src/app/main/ui/workspace/left_header.cljs b/frontend/src/app/main/ui/workspace/left_header.cljs index 1d9fb8b25f..37e6247b87 100644 --- a/frontend/src/app/main/ui/workspace/left_header.cljs +++ b/frontend/src/app/main/ui/workspace/left_header.cljs @@ -115,18 +115,21 @@ :pending (stl/css :status-notification :pending-status) :saving (stl/css :status-notification :saving-status) :saved (stl/css :status-notification :saved-status) + :retrying (stl/css :status-notification :retrying-status) :error (stl/css :status-notification :error-status) (stl/css :status-notification)) :title (case persistence-status :pending (tr "workspace.header.saving") :saving (tr "workspace.header.saving") :saved (tr "workspace.header.saved") + :retrying (tr "workspace.header.retrying") :error (tr "workspace.header.save-error") nil)} (case persistence-status :pending deprecated-icon/status-alert :saving deprecated-icon/status-alert :saved deprecated-icon/status-tick + :retrying deprecated-icon/status-alert :error deprecated-icon/status-wrong nil)] [:div {:class (stl/css :file-name-label)} file-name]])] diff --git a/frontend/src/app/main/ui/workspace/left_header.scss b/frontend/src/app/main/ui/workspace/left_header.scss index 536eb66326..663c7465b2 100644 --- a/frontend/src/app/main/ui/workspace/left_header.scss +++ b/frontend/src/app/main/ui/workspace/left_header.scss @@ -117,6 +117,10 @@ animation: jump 0.3s ease-out; } + &.retrying-status { + background-color: var(--status-widget-background-color-warning); + } + &.error-status { background-color: var(--status-widget-background-color-error); } diff --git a/frontend/test/frontend_tests/data/persistence_retry_test.cljs b/frontend/test/frontend_tests/data/persistence_retry_test.cljs new file mode 100644 index 0000000000..5da357242f --- /dev/null +++ b/frontend/test/frontend_tests/data/persistence_retry_test.cljs @@ -0,0 +1,443 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.data.persistence-retry-test + "Retry-episode tests for the file persistence save pipeline + (`app.main.data.persistence`). + + Covers the transient-failure retry contract: transient failures resend + the head commit with backoff, the budget bounds the episode, guards + (in-flight, stale token, online resume) prevent double-sends, and hung + resends across governor windows still earn their own stall report." + (:require + [app.common.time :as ct] + [app.common.uuid :as uuid] + [app.main.data.changes :as dch] + [app.main.data.event :as ev] + [app.main.data.persistence :as dps] + [app.main.errors :as errors] + [app.main.repo :as rp] + [app.main.router :as rt] + [app.main.store :as st] + [app.util.i18n :as i18n] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] + [frontend-tests.helpers.mock :as mock] + [potok.v2.core :as ptk])) + +(defn- local-commit + "Builds a synthetic local commit event for `file-id`: a page edit as redo, + no undo, from the `:local` source." + [file-id] + (ptk/data-event ::dch/commit + {:id (uuid/next) + :file-id file-id + :file-revn 0 + :file-vern 0 + :source :local + :features #{} + :redo-changes [{:type :mod-page :id (uuid/next) :name "Edited"}] + :undo-changes []})) + +(defn- with-persistence + "Async fixture for the persistence tests: mocks the transport and flash, and + runs `f` with persistence initialized. + + Evaluates to a promise resolving once `f` settles and teardown completes; + `await` it from an `^:async` test. + + By default the transport responds through the `:response` subject. Pass a + `respond` function (`(fn [cmd params] observable)`) to drive the transport + response directly instead. Delivery through the subject is asynchronous + (`observe-on :async`): await each effect — via `wait-for` — before + asserting it." + [f & [respond]] + (let [file-id (uuid/next) + response (rx/subject) + failures (atom []) + requests (atom []) + store (ptk/store {:state {:current-file-id file-id + :permissions {:can-edit true} + :files {file-id {:id file-id :revn 0}}} + :on-error #(t/is false (str %))})] + (mock/with-mocks* + {rp/cmd! (mock/stub (fn [cmd params] + (swap! requests conj [cmd params]) + (if respond + (respond cmd params) + (->> response (rx/take 1) (rx/observe-on :async))))) + errors/flash (fn [& {:keys [cause]}] + (swap! failures conj cause))} + (try + (ptk/emit! store (dps/initialize-persistence)) + (await (f {:file-id file-id :response response :failures failures + :requests requests :store store})) + (finally + (rx/dispose! store) + (rx/end! response)))))) + +(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 + exhausts the 3-retry budget and lands terminal: `:error` carrying + `:invalid-save-response`, queue intact, exactly 4 sends." + [result] + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= :invalid-save-response + (get-in @store [:persistence :error :code])) + (= 1 (count (get-in @store [:persistence :queue])))) + "invalid response fails the save")) + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (= :invalid-save-response (get-in @store [:persistence :error :code]))) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + (t/is (= 4 (count @requests)) "initial send plus 3 retries")))) + (fn [_ _] result))) + +;; Variant: an empty answer. +(t/deftest ^:async empty-save-response-preserves-the-queue-as-failed + (await (check-failed-save-response (rx/empty)))) + +;; Variant: a nil answer. +(t/deftest ^:async nil-save-response-preserves-the-queue-as-failed + (await (check-failed-save-response (rx/of nil)))) + +;; Variant: a negative revision answer. +(t/deftest ^:async invalid-revision-save-response-preserves-the-queue-as-failed + (await (check-failed-save-response (rx/of {:revn -1})))) + +;; Retry tests. +;; +;; Production contract under test: a transient save failure keeps the head +;; commit queued under `:retrying` and resends it with backoff (2s / 8s / +;; 20s, then terminal). Retry timers are stubbed — instant when the test +;; drives completion, manually fired when it scripts the race — and every +;; stub records its delays so the schedule itself is asserted. +;; +;; Same async pattern as above: triggers stay bare, every assert block is +;; preceded by `wait-for` on its leading signal. + +;; Scenario: the first send fails transiently, the retry succeeds. Both +;; sends carry the same `:commit-id`; the save lands with an empty queue +;; and the episode metadata is cleared. Proves: one transient failure +;; retries the same commit instead of erroring. +(t/deftest ^:async transient-failure-retries-and-saves + (let [calls (atom 0)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + (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]))) + "retry saves the file")) + (t/is (= :saved (get-in @store [:persistence :status]))) + (t/is (empty? (get-in @store [:persistence :queue]))) + (t/is (= 2 (count @requests)) "failed send plus one retry") + (t/is (apply = (map (comp :commit-id second) @requests)) + "both sends carry the same commit id") + (t/is (nil? (get-in @store [:persistence :attempts])) + "the episode metadata is cleared on success")))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/of {:revn 1}))))))) + +;; Scenario: every send fails transiently. The 2s / 8s / 20s retries fire +;; and then the failure falls through to the exact terminal path: `:error` +;; carrying the cause, queue intact, one flash. Proves: the budget bounds +;; the episode (4 sends) and exhaustion is today's terminal behavior. +(t/deftest ^:async retry-exhaustion-goes-terminal + (let [delays (atom [])] + (await + (with-persistence + (^: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)))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= 4 (count @requests))) + "retries exhaust into terminal error")) + (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 (= 1 (count @failures)) "exhaustion flashes exactly once")))) + (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. +(t/deftest ^:async retry-skips-resend-while-previous-request-is-in-flight + (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 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")) + (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: + ;; skipped, never a third send. + (rx/push! timer-s :tick) + (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])))) + (rx/end! timer-s)))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + (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. +(t/deftest ^:async stale-retry-token-stays-silent + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [_] (rx/subject)))} + (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))) + (ptk/emit! store (#'dps/persist-commit + (peek (get-in @store [:persistence :queue])) + {:token (uuid/next)})) + (await (async/settle)) + (t/is (= 1 (count @requests)) "stale token sends nothing") + (t/is (= :retrying (get-in @store [:persistence :status]))) + (t/is (nil? (get-in @store [:persistence :error])))))) + (fn [_ _] (rx/throw (ex-info "offline" {:type :offline})))))) + +;; 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, +;; so episodes never stack), gone once the save lands. Proves: exactly one +;; notice per episode, hidden on recovery. +(t/deftest ^:async retry-shows-a-single-reconnect-notice-until-saved + (let [calls (atom 0) + timer-s (rx/subject)] + (await + (with-persistence + (^:async fn [{:keys [file-id store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [_] timer-s)) + i18n/tr (mock/stub #(str "translated:" %))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (get-in @store [:persistence :attempts])) + "first attempt fails into retrying")) + (let [notice (get @store :notification)] + (t/is (map? notice) "a single notice is visible, never stacked")) + (t/is (= :persistence-reconnecting (get-in @store [:notification :tag]))) + (t/is (= "translated:errors.save-retrying" + (get-in @store [:notification :content]))) + (rx/push! timer-s :tick) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (nil? (get @store :notification))) + "save hides the notice")) + (t/is (= :saved (get-in @store [:persistence :status]))) + (t/is (nil? (get @store :notification)) "recovery is silent") + (rx/end! timer-s)))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/of {:revn 1}))))))) + +;; Scenario: every send fails transiently with instant timers. Exhaustion +;; takes the terminal path, which hides the reconnect notice explicitly +;; before flashing. Proves: no stale notice survives a terminal failure. +(t/deftest ^:async retry-exhaustion-hides-the-reconnect-notice + (await + (with-persistence + (^:async fn [{:keys [file-id store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + (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")) + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (nil? (get @store :notification)) + "the terminal path hides the reconnect notice")))) + (fn [_ _] (rx/throw (ex-info "offline" {:type :offline})))))) + +;; Scenario: a retrying episode waits on its backoff timer when the browser +;; reports connectivity back. The pending timer never fires, so only the +;; online signal can resume: the head resends under the live episode and, +;; once answered, the file saves. A second online signal with an empty +;; queue sends nothing. Proves: reconnect resumes promptly without +;; terminal failures staying terminal. +(t/deftest ^:async online-event-resumes-a-retrying-episode + (let [calls (atom 0) + timer-s (rx/subject) + second-s (atom nil)] + (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")) + (t/is (= 1 (count @requests))) + (ptk/emit! store (#'dps/resume-on-online)) + (await (async/wait-for #(= 2 (count @requests)) "online resends")) + (t/is (= 2 (count @requests))) + (t/is (= :retrying (get-in @store [:persistence :status]))) + (t/is (= 1 (get-in @store [:persistence :attempts]))) + (rx/push! @second-s {:revn 1}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "resumed save lands")) + (t/is (= :saved (get-in @store [:persistence :status]))) + (ptk/emit! store (#'dps/resume-on-online)) + (await (async/settle)) + (t/is (= 2 (count @requests)) "online with an empty queue sends nothing") + (rx/end! timer-s)))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + (let [s (rx/subject)] (reset! second-s s) s))))))) + +(defn- audit-events + "The audit events (report emissions) out of everything collected through + the `st/emit!` double. Discriminates by `ptk/type`, which is total (never + throws)." + [events] + (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`. +(t/deftest ^:async online-event-does-not-resend-an-in-flight-request + (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))} + ;; 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 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")) + (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)) + (await (async/settle)) + (t/is (= 2 (count @requests)) "online sends nothing while in flight") + (t/is (= :retrying (get-in @store [:persistence :status]))) + (rx/end! timer-s)))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/subject))))))) + +;; Scenario: the same file stalls twice, more than a governor window apart +;; (scripted clock). Each stall emits its own report: the second is a new +;; episode, not a suppressed duplicate of the first. Proves: identical +;; stall causes coalesce only inside the window. +;; +;; Note: unlike `with-watchdog`, the real `submit-report` runs here (only +;; `generate-report` stays doubled), so the test exercises the governor, +;; the thunk contract and the emit path end to end. +(t/deftest ^:async repeated-stalls-across-windows-emit-each-report + (let [clock (atom 0) + ticks (rx/subject) + response (rx/subject) + requests (atom []) + causes (atom []) + events (atom []) + file-id (uuid/next) + store (ptk/store {:state {:current-file-id file-id + :permissions {:can-edit true} + :files {file-id {:id file-id :revn 0}}} + :on-error #(t/is false (str %))})] + (errors/reset-report-governor!) + (await + (mock/with-mocks* + {ct/now (mock/stub #(ct/inst @clock)) + rx/interval (mock/stub (fn [_] ticks)) + rp/cmd! (mock/stub (fn [cmd params] + (swap! requests conj {:cmd cmd :params params}) + (->> response (rx/take 1) (rx/observe-on :async)))) + st/state store + errors/generate-report (fn [cause & _] + (swap! causes conj cause) + "report") + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + rt/get-current-href (constantly "https://penpot.example.com/#/workspace")} + (try + (ptk/emit! store (dps/initialize-persistence)) + ;; Phase 1 — the first stall reports once. + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (reset! clock 300001) + (rx/push! ticks :tick) + (await (async/wait-for #(= 1 (count (audit-events @events))) + "first stall reports")) + (t/is (= 1 (count @requests))) + ;; Phase 2 — the save lands, resetting the stall clock; a new + ;; edit stalls again, over a governor window later. + (rx/push! response {:revn 1}) + (await (async/wait-for #(= :saved (get-in @store [:persistence :status])) + "first save lands")) + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 2 (count @requests)) "second edit is sent")) + (reset! clock (+ 300001 errors/report-window-ms 300001)) + (rx/push! ticks :tick) + (await (async/wait-for #(= 2 (count (audit-events @events))) + "second stall reports")) + (let [audits (audit-events @events)] + (t/is (= 2 (count audits))) + (t/is (= 2 (count @causes)) "each granted stall builds its report") + (t/is (every? #(= 1 (:occurrences (deref %))) audits) + "each stall is a fresh emission, never a coalesced repeat")) + (finally + (rx/dispose! store) + (rx/end! ticks) + (rx/end! response))))))) diff --git a/frontend/test/frontend_tests/data/persistence_test.cljs b/frontend/test/frontend_tests/data/persistence_test.cljs index 9a6fe53f10..4d1b660d84 100644 --- a/frontend/test/frontend_tests/data/persistence_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_test.cljs @@ -5,6 +5,17 @@ ;; Copyright (c) KALEIDOS SUBSIDIARY SL (ns frontend-tests.data.persistence-test + "Tests for the file persistence save pipeline (`app.main.data.persistence`): + read-only saves, the stalled-save watchdog, the core save pipeline + (queueing, failures, recovery), and the transient/terminal failure + classification. Retry-episode behavior lives in + `frontend-tests.data.persistence-retry-test`. + + The watchdog and read-only tests are fully async: `mock/with-mocks*` + installs the doubles, `^:async` bodies `await` observable effects (never + assert straight after a trigger), and completion propagates through + promises — test code threads no `done`. The remaining tests still use + `mock/with-mocks` with an explicit `done` chain." (:require [app.common.time :as ct] [app.common.uuid :as uuid] @@ -17,10 +28,13 @@ [app.util.i18n :as i18n] [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] [frontend-tests.helpers.mock :as mock] [potok.v2.core :as ptk])) (defn- local-commit + "Builds a synthetic local commit event for `file-id`: a page edit as redo, + no undo, from the `:local` source." [file-id] (ptk/data-event ::dch/commit {:id (uuid/next) @@ -32,46 +46,77 @@ :redo-changes [{:type :mod-page :id (uuid/next) :name "Edited"}] :undo-changes []})) -(t/deftest queued-edits-save-during-temporary-read-only-mode - (t/async done - (doseq [read-only-event [(drw/context-lost) - #(assoc-in % [:workspace-global :read-only?] true) - #(assoc % :workspace-global {:read-only? true - :preview-id (uuid/next)})]] - (let [file-id (uuid/next) - response (rx/subject) - errors (atom []) - store (ptk/store {:state {:permissions {:can-edit true} - :files {file-id {:id file-id :revn 0}}} - :on-error #(swap! errors conj %)})] - (mock/with-mocks - {rp/cmd! (mock/stub (fn [_ _] (rx/take 1 response)))} - (fn [section-done] - (try - (ptk/emit! store (dps/initialize-persistence) - (local-commit file-id) - read-only-event - ::dps/force-persist) - (rx/push! response {:revn 1}) - (t/is (= :saved (get-in @store [:persistence :status]))) - (t/is (empty? (get-in @store [:persistence :queue]))) +(defn- ^:async run-read-only-phases + "Drives the read-only save scenario presuming asynchronous APIs: every + phase emits its triggers, awaits the transport round-trip, and only + then asserts the observed state." + [store response requests file-id read-only-event errors] + (ptk/emit! store (dps/initialize-persistence) + (local-commit file-id) + read-only-event + ::dps/force-persist) + (await (async/await-response requests response {:revn 1})) + (t/is (= :saved (get-in @store [:persistence :status]))) + (t/is (empty? (get-in @store [:persistence :queue]))) - (ptk/emit! store (drw/context-restored) - #(assoc-in % [:workspace-global :read-only?] false) - (local-commit file-id) - ::dps/force-persist) - (rx/push! response {:revn 2}) - (t/is (= :saved (get-in @store [:persistence :status]))) - (t/is (empty? (get-in @store [:persistence :queue]))) - (t/is (empty? @errors)) - (finally - (rx/dispose! store) - (rx/end! response) - (section-done)))) - (fn [])))) - (done))) + (ptk/emit! store (drw/context-restored) + #(assoc-in % [:workspace-global :read-only?] false) + (local-commit file-id) + ::dps/force-persist) + (await (async/await-response requests response {:revn 2})) + (t/is (= :saved (get-in @store [:persistence :status]))) + (t/is (empty? (get-in @store [:persistence :queue]))) + (t/is (empty? @errors))) -(t/deftest historical-preview-cannot-create-local-commits +(defn- ^:async check-read-only-save + "Saves a queued edit while the file is read-only and verifies that the edit + is persisted once the restriction is lifted. + + Scenario: with persistence initialized, queue an edit, switch the file to + read-only (`read-only-event`), and force persistence — the edit goes out + and is acknowledged. Then restore an editable context, queue another edit + and persist again. Both saves land in order with no reported errors. + Proves: read-only defers writes without dropping them, and resumption + picks up where it left off." + [read-only-event] + (let [file-id (uuid/next) + response (rx/subject) + requests (atom []) + errors (atom []) + store (ptk/store {:state {:permissions {:can-edit true} + :files {file-id {:id file-id :revn 0}}} + :on-error #(swap! errors conj %)})] + + (mock/with-mocks* + {rp/cmd! (mock/stub (fn [cmd params] + (let [req (->> response (rx/take 1) (rx/observe-on :async))] + (swap! requests conj {:cmd cmd :params params :req req}) + req)))} + (try + (await (run-read-only-phases store response requests file-id read-only-event errors)) + (finally + (rx/dispose! store) + (rx/end! response)))))) + +;; Variant: the file goes read-only through render context loss. +(t/deftest ^:async queued-edits-save-after-context-loss + (await (check-read-only-save (drw/context-lost)))) + +;; Variant: the file goes read-only through the workspace flag. +(t/deftest ^:async queued-edits-save-in-read-only-mode + (await (check-read-only-save #(assoc-in % [:workspace-global :read-only?] true)))) + +;; Variant: the file goes read-only through preview mode. +(t/deftest ^:async queued-edits-save-in-read-only-preview-mode + (await (check-read-only-save #(assoc % :workspace-global {:read-only? true + :preview-id (uuid/next)})))) + +;; Scenario: the open file is a historical preview (read-only with a preview +;; id). Attempting a commit there must produce nothing observable: the gate +;; answers refusals with an empty stream (never nil), so the test observes +;; termination instead of branching on nil. Previews cannot create local +;; commits. Proves: the read-only preview gate holds at the watch level. +(t/deftest ^:async historical-preview-cannot-create-local-commits (let [file-id (uuid/next) output (atom []) state {:current-file-id file-id @@ -79,22 +124,50 @@ :files {file-id {:id file-id :revn 0 :vern 0}} :workspace-global {:read-only? true :preview-id (uuid/next)}} event (dch/commit-changes {:redo-changes [] :undo-changes []})] - (when-let [result (ptk/watch event state (rx/empty))] - (->> result (rx/subs! #(swap! output conj %)))) + (await (async/observe (ptk/watch event state (rx/empty)) + :on-next #(swap! output conj %))) (t/is (empty? @output)))) +;; Watchdog: stalled-save detection. +;; +;; Production contract under test (`app.main.data.persistence`, +;; `saving-stall-timeout-ms` = 5 minutes): +;; - A request unanswered past the deadline reports once as +;; `:saving-stalled` (a `handled-exception` audit event) and never +;; repeats for the same stall (`:stall-reported`). +;; - Any completed save resets the clock: a later stall reports again. +;; - Edits queued behind a stall are preserved; an already failed save is +;; not re-reported as a stall. +;; +;; Emulation model — the fixture freezes the three inputs the watchdog +;; reads, so each test scripts time by hand: +;; - clock (`ct/now`): an atom. The test sets the time; time never flows. +;; - ticks (`rx/interval`): a subject. Each push runs one watchdog pass. +;; - network (`rp/cmd!`): answered when the test pushes into `:response` +;; (scenario timing stays in test hands); delivery is asynchronous, so +;; every phase awaits its effects before asserting them. +;; - reports (`errors/generate-report`/`submit-report`): recorded into +;; atoms for assertions; nothing leaves the test. Thunk reports are +;; evaluated through the `generate-report` double, mirroring production +;; (granted reports build, suppressed ones never exist here). +;; +;; Reading the tests below: every assert block is preceded by quiescence — +;; `wait-for` on its presence-conditions, or a bare `settle` tick when it +;; asserts only absence. (defn- with-watchdog "Async fixture for the persistence watchdog tests: mocks the clock, timers and RPC transport, and runs `f` with persistence initialized. - Mocks are installed through `mock/with-mocks` and restored when the test - completes; `done'` chains to the `t/async` `done` and is always called - exactly once after teardown, so a failing body can neither leak the mocks - nor stall the test run." - [f done] + Evaluates to a promise resolving once `f` settles and teardown completes; + `await` it from an `^:async` test. The transport answers when the test + pushes into `:response` (scenario timing stays in test hands); delivery + is asynchronous, so await each effect — via `wait-for` — before asserting + it. `f` is awaited (usually an `^:async` fn)." + [f] (let [clock (atom 0) ticks (rx/subject) response (rx/subject) + requests (atom []) reports (atom []) causes (atom []) file-id (uuid/next) @@ -102,137 +175,277 @@ :permissions {:can-edit true} :files {file-id {:id file-id :revn 0}}} :on-error #(t/is false (str %))})] - (mock/with-mocks + (mock/with-mocks* {ct/now (mock/stub #(ct/inst @clock)) rx/interval (mock/stub (fn [_] ticks)) - rp/cmd! (mock/stub (fn [_ _] (rx/take 1 response))) + rp/cmd! (mock/stub (fn [cmd params] + (swap! requests conj {:cmd cmd :params params}) + (->> response (rx/take 1) (rx/observe-on :async)))) st/state store errors/generate-report (fn [cause & _] (swap! causes conj cause) "report") errors/submit-report (fn [& params] - (swap! reports conj (apply hash-map params)))} - (fn [done'] - (try - (ptk/emit! store (dps/initialize-persistence)) - (f {:clock clock :ticks ticks :response response :causes causes - :reports reports :store store :file-id file-id}) - (finally - (rx/dispose! store) - (rx/end! ticks) - (rx/end! response) - (done')))) - done))) + ;; Mirrors production: a granted report builds + ;; its payload through `generate-report`, so a + ;; thunk report is evaluated here while a + ;; string report is recorded as it arrives. + (let [m (apply hash-map params)] + (swap! reports conj m) + (when (fn? (:report m)) + ((:report m)))))} + (try + (ptk/emit! store (dps/initialize-persistence)) + (await (f {:clock clock :ticks ticks :response response :requests requests + :causes causes :reports reports :store store :file-id file-id})) + (finally + (rx/dispose! store) + (rx/end! ticks) + (rx/end! response)))))) -(t/deftest stalled-request-is-reported-once-without-discarding-edits - (t/async done - (with-watchdog - (fn [{:keys [clock ticks reports causes store file-id]}] - (ptk/emit! store (local-commit file-id) ::dps/force-persist) - (reset! clock 300000) - (rx/push! ticks :tick) - (t/is (empty? @reports) "Five minutes must elapse before reporting") +;; Scenario: persistence sits in a non-terminal status with an empty +;; queue past the deadline (a status transition racing the queue drain). +;; The watchdog must stay silent: with nothing queued there is nothing +;; stalled to report. Proves: no false-positive stall reports on an +;; empty queue. +(t/deftest ^:async empty-queue-never-reports-a-stall + (await + (with-watchdog + (^:async fn [{:keys [clock ticks reports store]}] + (ptk/emit! store (#'dps/update-status :pending)) + (reset! clock 300001) + (rx/push! ticks :tick) + (await (async/settle)) + (t/is (empty? @reports) "an empty queue is never a stall"))))) - ;; More local edits must not reset the stalled request's clock. - (reset! clock 300001) - (ptk/emit! store (drw/context-lost) - (local-commit file-id) ::dps/force-persist) - (rx/push! ticks :tick) - (t/is (= 1 (count @reports))) - (t/is (= "handled-exception" (:event-name (first @reports)))) - (let [data (ex-data (first @causes))] - (t/is (= :saving-stalled (:code data))) - (t/is (= file-id (:file-id data))) - (t/is (true? (:render-context-lost? data)))) +;; Scenario: the network hangs. One save goes out and is never answered. +;; Five minutes pass with no report (the deadline is exclusive); one second +;; later, with more edits piled behind the stalled request, the tick reports +;; the stall exactly once — as `handled-exception`, carrying the file id and +;; the lost render context — and later ticks repeat nothing while both edits +;; stay queued. Proves: one report per stall, no lost edits. +(t/deftest ^:async stalled-request-is-reported-once-without-discarding-edits + (await + (with-watchdog + (^:async fn [{:keys [clock ticks reports causes store file-id]}] + ;; Phase 1 — at exactly five minutes the deadline has not elapsed: silence. + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (reset! clock 300000) + (rx/push! ticks :tick) + (await (async/settle)) + (t/is (empty? @reports) "Five minutes must elapse before reporting") - (reset! clock 900000) - (rx/push! ticks :tick) - (t/is (= 1 (count @reports)) "Do not repeat a report for the same stall") - (t/is (= :saving (get-in @store [:persistence :status]))) - (t/is (= 2 (count (get-in @store [:persistence :queue]))))) - done))) + ;; Phase 2 — one second past the deadline the stall reports exactly once. + ;; More local edits must not reset the stalled request's clock. + (reset! clock 300001) + (ptk/emit! store (drw/context-lost) + (local-commit file-id) ::dps/force-persist) + (rx/push! ticks :tick) + (await (async/wait-for #(= 1 (count @reports)) "stall report")) + (t/is (= 1 (count @reports))) + (t/is (= "handled-exception" (:event-name (first @reports)))) + (let [data (ex-data (first @causes))] + (t/is (= :saving-stalled (:code data))) + (t/is (= file-id (:file-id data))) + (t/is (true? (:render-context-lost? data)))) -(t/deftest successful-saves-reset-the-stall-clock-and-allow-a-new-report - (t/async done - (with-watchdog - (fn [{:keys [clock ticks response reports store file-id]}] - (ptk/emit! store (local-commit file-id) ::dps/force-persist - (local-commit file-id) ::dps/force-persist) - (reset! clock 290000) - (rx/push! response {:revn 1}) - (reset! clock 300001) - (rx/push! ticks :tick) - (t/is (empty? @reports) "The queue is making progress") + ;; Phase 3 — long after: no repeat, both edits still queued. + (reset! clock 900000) + (rx/push! ticks :tick) + (await (async/settle)) + (t/is (= 1 (count @reports)) "Do not repeat a report for the same stall") + (t/is (= :saving (get-in @store [:persistence :status]))) + (t/is (= 2 (count (get-in @store [:persistence :queue])))))))) - (reset! clock 590001) - (rx/push! ticks :tick) - (t/is (= 1 (count @reports)) "The second request has now stalled") +;; Scenario: answers arrive, then stop. Two commits share the first request; +;; awaiting its landing advances the queue and starts the second request, so +;; the tick finds progress and stays silent. Left hanging past a fresh +;; deadline it reports once; answering it saves the file and silences the +;; watchdog; a later hang reports again. Proves: progress resets the clock, +;; and every new stall earns its own report. +;; +;; Note the awaits: a push only schedules delivery, so each tick must +;; observe the completed save — awaiting after the push is what separates +;; "answered" from "completed". +(t/deftest ^:async successful-saves-reset-the-stall-clock-and-allow-a-new-report + (await + (with-watchdog + (^:async fn [{:keys [clock ticks reports response store file-id]}] + (ptk/emit! store (local-commit file-id) ::dps/force-persist + (local-commit file-id) ::dps/force-persist) + ;; Phase 1 — two commits share the first request; its landing starts the second. + (reset! clock 290000) + (rx/push! response {:revn 1}) + (await (async/wait-for #(= 1 (count (get-in @store [:persistence :queue]))) + "first save lands, second request starts")) + (reset! clock 300001) + (rx/push! ticks :tick) + (await (async/settle)) + (t/is (empty? @reports) "The queue is making progress") - (rx/push! response {:revn 2}) - (t/is (= :saved (get-in @store [:persistence :status]))) - (reset! clock 1000000) - (rx/push! ticks :tick) - (t/is (= 1 (count @reports)) "A saved file must not be reported") + ;; Phase 2 — the second request hangs past its own fresh deadline. + (reset! clock 590001) + (rx/push! ticks :tick) + (await (async/wait-for #(= 1 (count @reports)) "second request stalls")) + (t/is (= 1 (count @reports)) "The second request has now stalled") - (ptk/emit! store (local-commit file-id) ::dps/force-persist) - (reset! clock 1300001) - (rx/push! ticks :tick) - (t/is (= 2 (count @reports)) "A later stall gets its own report")) - done))) + (rx/push! response {:revn 2}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "second save lands")) + (t/is (= :saved (get-in @store [:persistence :status]))) + ;; Phase 3 — answering saves the file: the tick stays silent. + (reset! clock 1000000) + (rx/push! ticks :tick) + (t/is (= 1 (count @reports)) "A saved file must not be reported") -(t/deftest pending-edits-are-monitored-without-extending-the-deadline - (t/async done - (with-watchdog - (fn [{:keys [clock ticks reports store]}] - (rx/push! ticks :tick) - (t/is (empty? @reports) "An idle file must not be reported") - (ptk/emit! store (#'dps/update-status :pending)) - (reset! clock 300001) - (ptk/emit! store (#'dps/update-status :pending)) - (rx/push! ticks :tick) - (t/is (= 1 (count @reports))) - (ptk/emit! store (#'dps/update-status :error)) - (reset! clock 900000) - (rx/push! ticks :tick) - (t/is (= 1 (count @reports)) "Do not report an already failed save")) - done))) + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + ;; Phase 4 — a later hang is a new stall with its own report. + (reset! clock 1300001) + (rx/push! ticks :tick) + (await (async/wait-for #(= 2 (count @reports)) "later stall reports again")) + (t/is (= 2 (count @reports)) "A later stall gets its own report"))))) -(t/deftest reinitializing-persistence-replaces-the-watchdog - (t/async done - (let [active-timers (atom 0) - ticks (rx/subject) - store (ptk/store {:state {} :on-error #(t/is false (str %))})] - (mock/with-mocks - {rx/interval (mock/stub - (fn [_] - (rx/create - (fn [subscriber] - (swap! active-timers inc) - (let [subscription (.subscribe ticks subscriber)] - (fn [] - (rx/dispose! subscription) - (swap! active-timers dec)))))))} - (fn [section-done] - (try - (ptk/emit! store (dps/initialize-persistence)) - (t/is (= 1 @active-timers)) - (ptk/emit! store (dps/initialize-persistence)) - (t/is (= 1 @active-timers)) - (finally - (rx/dispose! store) - (rx/end! ticks) - (t/is (zero? @active-timers)) - (section-done)))) - done)))) +;; Scenario: no network request exists at all — an edit sits `:pending` +;; locally without being sent. The watchdog still tracks it against the +;; original deadline (re-marking `:pending` does not push it): an idle +;; file never reports, an aged pending edit reports once, and once the +;; save fails outright the watchdog stands down — failures belong to the +;; save path, never twice. Proves: pending edits are watched without +;; extending deadlines, and failures are not re-reported as stalls. +(t/deftest ^:async pending-edits-are-monitored-without-extending-the-deadline + (await + (with-watchdog + (^:async fn [{:keys [clock ticks reports store file-id]}] + ;; Phase 1 — idle: nothing queued, nothing reported. + (rx/push! ticks :tick) + (await (async/settle)) + (t/is (empty? @reports) "An idle file must not be reported") + ;; Phase 2 — a queued (yet unsent) edit ages into one report, and + ;; re-marking pending does not extend its deadline. + (let [id (uuid/next)] + (ptk/emit! store + #(assoc % :persistence {:queue (conj #queue [] id) + :index {id {:id id :file-id file-id}}}))) + (ptk/emit! store (#'dps/update-status :pending)) + (reset! clock 300001) + (ptk/emit! store (#'dps/update-status :pending)) + (rx/push! ticks :tick) + (await (async/wait-for #(= 1 (count @reports)) "pending edit ages into a report")) + (t/is (= 1 (count @reports))) + ;; Phase 3 — an outright failure is not a stall: no second report. + (ptk/emit! store (#'dps/update-status :error)) + (reset! clock 900000) + (rx/push! ticks :tick) + (await (async/settle)) + (t/is (= 1 (count @reports)) "Do not report an already failed save"))))) +;; Scenario: initializing twice must replace the watchdog instead of stacking +;; it — a single active timer at all times, zero after teardown. Proves: +;; reinitialization swaps the timer subscription instead of leaking it. +;; (Fully synchronous bodies need no awaits; the promise shell still +;; guarantees restore and completion.) +(t/deftest ^:async reinitializing-persistence-replaces-the-watchdog + (await + (let [active-timers (atom 0) + ticks (rx/subject) + store (ptk/store {:state {} :on-error #(t/is false (str %))})] + (mock/with-mocks* + {rx/interval (mock/stub + (fn [_] + (rx/create + (fn [subscriber] + (swap! active-timers inc) + (let [subscription (.subscribe ticks subscriber)] + (fn [] + (rx/dispose! subscription) + (swap! active-timers dec)))))))} + (try + (ptk/emit! store (dps/initialize-persistence)) + (t/is (= 1 @active-timers)) + (ptk/emit! store (dps/initialize-persistence)) + (t/is (= 1 @active-timers)) + (finally + (rx/dispose! store) + (rx/end! ticks) + (t/is (zero? @active-timers)))))))) + +;; Scenario: the first send fails transiently and the retry resend hangs +;; forever inside the `:retrying` episode. Past the 5-minute deadline the +;; watchdog must still report the stall exactly once — and never repeat +;; it — while the queued edit is preserved. Proves: the watchdog sees +;; hung resends; a `:retrying` episode cannot deadlock silently. +(t/deftest ^:async hung-retry-resend-is-reported-as-a-stall + (let [calls (atom 0) + timer-s (rx/subject)] + (await + (with-watchdog + (^:async fn [{:keys [clock ticks reports causes requests store file-id]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [_] timer-s)) + rp/cmd! (mock/stub (fn [cmd params] + (swap! requests conj {:cmd cmd :params params}) + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + ;; The resend hangs forever: the subject + ;; is never answered nor failed. + (rx/subject))))} + ;; Phase 1 — first send fails transiently; the episode parks + ;; 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 — the timer fires and the resend goes out, hanging. + (rx/push! timer-s :tick) + (await (async/wait-for #(= 2 (count @requests)) "retry resends")) + (t/is (= :retrying (get-in @store [:persistence :status]))) + ;; Phase 3 — past the deadline the watchdog reports the stall + ;; once, keeping the queued edit. + (reset! clock 300001) + (rx/push! ticks :tick) + (await (async/wait-for #(= 1 (count @reports)) "hung resend stalls")) + (t/is (= 1 (count @reports))) + (t/is (= "handled-exception" (:event-name (first @reports)))) + (t/is (= :saving-stalled (:code (ex-data (first @causes))))) + (t/is (= :retrying (get-in @store [:persistence :status]))) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + ;; Phase 4 — later ticks repeat nothing for the same stall. + (reset! clock 900000) + (rx/push! ticks :tick) + (await (async/settle)) + (t/is (= 1 (count @reports)) "Do not repeat a report for the same stall") + (rx/end! timer-s)))))))) + +;; Save pipeline tests. +;; +;; Production contract under test (`app.main.data.persistence`): +;; - Commits queue locally and flush in order; every request carries the +;; queued changes exactly once (no resends of active requests, no drops). +;; - Without edit permission nothing is sent and the failure flashes. +;; - 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. +;; +;; 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 +;; `settle` tick when it asserts only absence). (defn- with-persistence "Async fixture for the persistence tests: mocks the transport and flash, and runs `f` with persistence initialized. - Like `with-watchdog`, `done'` chains to the `t/async` `done` and is called - exactly once after teardown. Inside a `doseq`, pass a no-op completion - (`(fn [])`) and call the test's `done` once at the end." - [f done] + Evaluates to a promise resolving once `f` settles and teardown completes; + `await` it from an `^:async` test. + + By default the transport responds through the `:response` subject. Pass a + `respond` function (`(fn [cmd params] observable)`) to drive the transport + response directly instead. Delivery through the subject is asynchronous + (`observe-on :async`): await each effect — via `wait-for` — before + asserting it." + [f & [respond]] (let [file-id (uuid/next) response (rx/subject) failures (atom []) @@ -241,214 +454,340 @@ :permissions {:can-edit true} :files {file-id {:id file-id :revn 0}}} :on-error #(t/is false (str %))})] - (mock/with-mocks + (mock/with-mocks* {rp/cmd! (mock/stub (fn [cmd params] (swap! requests conj [cmd params]) - (rx/take 1 response))) + (if respond + (respond cmd params) + (->> response (rx/take 1) (rx/observe-on :async))))) errors/flash (fn [& {:keys [cause]}] (swap! failures conj cause))} - (fn [done'] - (try - (ptk/emit! store (dps/initialize-persistence)) - (f {:file-id file-id :response response :failures failures - :requests requests :store store}) - (finally - (rx/dispose! store) - (rx/end! response) - (done')))) - done))) - -(t/deftest permission-loss-fails-without-discarding-queued-edits - (t/async done - (with-persistence - (fn [{:keys [file-id requests failures store]}] - (ptk/emit! store (local-commit file-id) - #(assoc-in % [:permissions :can-edit] false) - ::dps/force-persist) - (t/is (= :error (get-in @store [:persistence :status]))) - (t/is (= 1 (count (get-in @store [:persistence :queue])))) - (t/is (empty? @requests)) - (t/is (= 1 (count @failures))) - (ptk/emit! store (local-commit file-id) ::dps/force-persist - (#'dps/update-status :pending)) - (t/is (= :error (get-in @store [:persistence :status]))) - (t/is (= 2 (count (get-in @store [:persistence :queue]))))) - done))) - -(t/deftest failed-request-retains-the-queue-and-is-not-retried-on-initialization - (t/async done - (with-persistence - (fn [{:keys [file-id response requests store]}] - (ptk/emit! store (local-commit file-id) ::dps/force-persist - (local-commit file-id) ::dps/force-persist) - (.error response (ex-info "Connection lost" {:type :network})) + (try (ptk/emit! store (dps/initialize-persistence)) - (t/is (= :error (get-in @store [:persistence :status]))) - (t/is (= 2 (count (get-in @store [:persistence :queue])))) - (t/is (= 1 (count @requests)))) - done))) + (await (f {:file-id file-id :response response :failures failures + :requests requests :store store})) + (finally + (rx/dispose! store) + (rx/end! response)))))) -(t/deftest save-failures-use-a-translated-warning-except-for-authentication - (t/async done - (doseq [cause-type [:network :offline :authentication]] - (with-persistence - (fn [{:keys [file-id response store]}] - (let [notifications (atom [])] - (mock/with-mocks - {errors/flash (fn [& params] - (swap! notifications conj (apply hash-map params))) - i18n/tr (mock/stub #(str "translated:" %))} - (fn [section-done] - (ptk/emit! store (local-commit file-id) ::dps/force-persist) - (.error response (ex-info "Raw transport details" {:type cause-type})) - (t/is (= :error (get-in @store [:persistence :status]))) - (let [data (get-in @store [:persistence :error])] - (ptk/handle-error (assoc data ::errors/instance (ex-info "Save failed" data)))) - (if (= cause-type :authentication) - (t/is (empty? @notifications)) - (t/is (= ["translated:errors.save-failed"] - (mapv :hint @notifications)))) - (section-done)) - (fn [])))) - (fn []))) - (done))) +;; Scenario: edit permission is revoked with an edit queued. The save fails +;; without sending anything, the queue keeps the edit, and the user is +;; notified once; further edits keep queueing behind the failure instead of +;; replacing it. Proves: permission loss fails safe — no send, no discard, +;; one flash. +(t/deftest ^:async permission-loss-fails-without-discarding-queued-edits + (await + (with-persistence + (^:async fn [{:keys [file-id requests failures store]}] + (ptk/emit! store (local-commit file-id) + #(assoc-in % [:permissions :can-edit] false) + ::dps/force-persist) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= 1 (count (get-in @store [:persistence :queue])))) + "permission loss fails the save")) + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + (t/is (empty? @requests)) + (t/is (= 1 (count @failures))) + (ptk/emit! store (local-commit file-id) ::dps/force-persist + (#'dps/update-status :pending)) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= 2 (count (get-in @store [:persistence :queue])))) + "second edit queues behind the failure")) + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (= 2 (count (get-in @store [:persistence :queue])))))))) -(t/deftest missing-commit-is-an-error-instead-of-skipping-changes - (t/async done - (with-persistence - (fn [{:keys [requests store]}] - (let [id (uuid/next)] - (ptk/emit! store - #(assoc % :persistence {:queue (conj #queue [] id) - :index {} :run-id (uuid/next) - :status :saving}) - (dps/initialize-persistence)) - (t/is (= :error (get-in @store [:persistence :status]))) - (t/is (= [id] (vec (get-in @store [:persistence :queue])))) - (t/is (empty? @requests)))) - done))) +;; Scenario: a request with two queued commits fails terminally at the +;; transport. Both edits stay queued as failed; reinitializing persistence +;; must not replay the dead request. Proves: failed requests preserve the +;; queue and are never retried on initialization. +(t/deftest ^:async failed-request-retains-the-queue-and-is-not-retried-on-initialization + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (ptk/emit! store (local-commit file-id) ::dps/force-persist + (local-commit file-id) ::dps/force-persist) + (.error response (ex-info "Validation failed" {:type :validation})) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "failed request errors the save")) + (ptk/emit! store (dps/initialize-persistence)) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= 2 (count (get-in @store [:persistence :queue])))) + "initialization preserves the failed queue")) + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (= 2 (count (get-in @store [:persistence :queue])))) + (t/is (= 1 (count @requests))))))) -(t/deftest initialization-recovers-an-unsent-commit-with-a-dangling-run-id - (t/async done - (with-persistence - (fn [{:keys [file-id response requests store]}] - (let [commit (assoc @(local-commit file-id) :changes []) - id (:id commit)] - (ptk/emit! store - #(assoc % :persistence {:queue (conj #queue [] id) - :index {id commit} :run-id (uuid/next) - :status :saving}) - (dps/initialize-persistence)) - (t/is (= 1 (count @requests))) - (rx/push! response {:revn 1}) - (t/is (= :saved (get-in @store [:persistence :status]))) - (t/is (empty? (get-in @store [:persistence :queue]))))) - done))) +(defn- check-save-failure-warning + "Fails a queued save with `cause-type` and asserts the hint shown to the + user; authentication failures have their own UI and must stay silent." + [cause-type] + (with-persistence + (^:async fn [{:keys [file-id response store]}] + (let [notifications (atom [])] + (await + (mock/with-mocks* + {errors/flash (fn [& params] + (swap! notifications conj (apply hash-map params))) + i18n/tr (mock/stub #(str "translated:" %))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (.error response (ex-info "Raw transport details" {:type cause-type})) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "failed request errors the save")) + (t/is (= :error (get-in @store [:persistence :status]))) + (let [data (get-in @store [:persistence :error])] + (ptk/handle-error (assoc data ::errors/instance (ex-info "Save failed" data)))) + (if (= cause-type :authentication) + (do (await (async/settle)) + (t/is (empty? @notifications))) + (do (await (async/wait-for #(seq @notifications) "save-failure toast")) + (t/is (= ["translated:errors.save-failed"] + (mapv :hint @notifications))))))))))) -(t/deftest an-active-request-is-never-sent-twice - (t/async done - (doseq [interrupt [[(dps/initialize-persistence)] - [(ptk/data-event ::dps/error)]]] - (with-persistence - (fn [{:keys [file-id response requests store]}] - (apply ptk/emit! store (local-commit file-id) ::dps/force-persist interrupt) - (ptk/emit! store (dps/initialize-persistence)) - (t/is (= 1 (count @requests))) - (rx/push! response {:revn 1}) - (t/is (= :saved (get-in @store [:persistence :status]))) - (t/is (empty? (get-in @store [:persistence :queue])))) - (fn []))) - (done))) +;; Variant: an authentication failure stays silent (own UI flow). +(t/deftest ^:async authentication-save-failure-shows-no-warning + (await (check-save-failure-warning :authentication))) -(t/deftest permission-restoration-resumes-only-unsent-edits - (t/async done - (with-persistence - (fn [{:keys [file-id response requests store]}] - (ptk/emit! store (local-commit file-id) ::dps/force-persist - (local-commit file-id) ::dps/force-persist - #(assoc-in % [:permissions :can-edit] false)) - (rx/push! response {:revn 1}) - (t/is (= :error (get-in @store [:persistence :status]))) - (t/is (= 1 (count (get-in @store [:persistence :queue])))) - (ptk/emit! store - #(assoc-in % [:permissions :can-edit] true) - (ptk/data-event :app.main.data.common/change-team-role)) - (t/is (= 2 (count @requests))) - (rx/push! response {:revn 2}) - (t/is (= :saved (get-in @store [:persistence :status]))) - (t/is (empty? (get-in @store [:persistence :queue])))) - done))) +;; NOTE: the `:network` and `:offline` warning variants lived here while +;; transport failures went straight to `:error`. Transient failures now +;; retry instead, so their coverage moved to the retry tests below (and to +;; the exhaustion test for the terminal toast). -(t/deftest recovery-keeps-an-acknowledgment-received-without-a-runner - (t/async done - (with-persistence - (fn [{:keys [file-id response requests store]}] - (ptk/emit! store (local-commit file-id) ::dps/force-persist - (ptk/data-event ::dps/error)) - (rx/push! response {:revn 1}) - (t/is (= 1 (count (get-in @store [:persistence :queue])))) - (ptk/emit! store (dps/initialize-persistence)) - (t/is (= 1 (count @requests)) "The acknowledged changes must not be sent again") - (t/is (= :saved (get-in @store [:persistence :status]))) - (t/is (empty? (get-in @store [:persistence :queue])))) - done))) +;; The transient/terminal classification is pure: transport failures and +;; unusable save responses 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 + (t/is (dps/transient-error? {:type :network})) + (t/is (dps/transient-error? {:type :offline})) + (t/is (dps/transient-error? {:type :bad-gateway})) + (t/is (dps/transient-error? {:type :service-unavailable})) + (t/is (dps/transient-error? {:type :persistence :cause-type :network})) + (t/is (dps/transient-error? {:type :persistence :cause-type :offline})) + (t/is (dps/transient-error? {:type :internal :cause-type :bad-gateway})) + (t/is (dps/transient-error? {:type :persistence :cause-type :service-unavailable})) + ;; The wrapped shape stays transient with an explicit non-retry code too: + ;; the cause-type carries the transport verdict, not the wrapper's code. + (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})) + (t/is (not (dps/transient-error? {:type :authentication}))) + (t/is (not (dps/transient-error? {:type :validation}))) + (t/is (not (dps/transient-error? {:type :internal}))) + (t/is (not (dps/transient-error? {:type :persistence :cause-type :authentication}))) + (t/is (not (dps/transient-error? {:type :persistence :code :missing-commit}))) + (t/is (not (dps/transient-error? {:type :persistence :code :save-permission-denied}))) + (t/is (not (dps/transient-error? {})))) -(t/deftest recovery-reports-an-unknown-request-outcome-without-replaying-it - (t/async done - (with-persistence - (fn [{:keys [file-id 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)) - (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)))) - done))) +;; The transient set is owned by `repo/retryable-types`: every transport +;; type retryable at the HTTP layer must also be transient for the save +;; pipeline, in both the direct and the wrapped (`persistence-failed`) +;; positions. Proves: the two classifications cannot drift apart. +(t/deftest transient-classification-follows-repo-retryable-types + (doseq [transport-type rp/retryable-types] + (t/testing (str "transport type " transport-type " is transient in both positions") + (t/is (dps/transient-error? {:type transport-type})) + (t/is (dps/transient-error? {:type :persistence :cause-type transport-type}))))) -(t/deftest initialization-flushes-buffered-edits-without-duplicating-them - (t/async done - (with-persistence - (fn [{:keys [file-id response requests store]}] - (ptk/emit! store (local-commit file-id) - (dps/initialize-persistence) - ::dps/force-persist) - (t/is (= 1 (count @requests))) - (t/is (= 1 (count (get-in @store [:persistence :queue])))) - (rx/push! response {:revn 1}) - (t/is (= :saved (get-in @store [:persistence :status])))) - done))) +;; Scenario: a save fails transiently and the episode parks on its backoff +;; timer. A waiter started mid-episode must not resolve on the `:retrying` +;; transition — only the terminal save releases it. Proves: `:retrying` is +;; non-terminal for waiters. +(t/deftest waiter-release-rule + ;; The waiter release rule is pure: only a failed save or a settled + ;; (`nil` / `:saved`) empty queue releases waiters. In particular, the + ;; `:retrying` episode — queue non-empty, status non-terminal — never + ;; does. Proves: `:retrying` is non-terminal for waiters, a document of + ;; intent over the `wait-persisted-or-error` filter. + ;; + ;; Note: this intentionally avoids scripting the global store atom — it + ;; is shared with the whole suite, so any test reading it observes + ;; other namespaces' leftovers depending on run order. + (t/is (dps/terminal-status? {:status :error :queue [1]})) + (t/is (dps/terminal-status? {:status :saved :queue []})) + (t/is (dps/terminal-status? {:status nil :queue []})) + (t/is (dps/terminal-status? {:status nil :queue nil})) + (t/is (not (dps/terminal-status? {:status :retrying :queue [1]}))) + (t/is (not (dps/terminal-status? {:status :saving :queue [1]}))) + (t/is (not (dps/terminal-status? {:status :pending :queue [1]}))) + (t/is (not (dps/terminal-status? {:status :saved :queue [1]})))) -(t/deftest synchronous-save-results-do-not-leave-a-dangling-runner - (t/async done - (with-persistence - (fn [{:keys [file-id store]}] - (mock/with-mocks - {rp/cmd! (mock/stub (fn [_ _] (rx/of {:revn 1})))} - (fn [section-done] - (ptk/emit! store (local-commit file-id) ::dps/force-persist) - (t/is (= :saved (get-in @store [:persistence :status]))) - (t/is (empty? (get-in @store [:persistence :queue]))) - (section-done)) - (fn []))) - done))) +;; Scenario: the queue references a commit id with no matching commit +;; (plus a dangling run id) when persistence initializes. It must error +;; instead of silently skipping the unknown entry, keeping it queued and +;; sending nothing. Proves: dangling queue entries fail loudly, never skip. +(t/deftest ^:async missing-commit-is-an-error-instead-of-skipping-changes + (await + (with-persistence + (^:async fn [{:keys [requests store]}] + (let [id (uuid/next)] + (ptk/emit! store + #(assoc % :persistence {:queue (conj #queue [] id) + :index {} :run-id (uuid/next) + :status :saving}) + (dps/initialize-persistence)) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "dangling commit errors instead of skipping")) + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (= [id] (vec (get-in @store [:persistence :queue])))) + (t/is (empty? @requests))))))) -(t/deftest empty-or-invalid-save-responses-preserve-the-queue-as-failed - (t/async done - (doseq [result [(rx/empty) (rx/of nil) (rx/of {:revn -1})]] - (with-persistence - (fn [{:keys [file-id store]}] - (mock/with-mocks - {rp/cmd! (mock/stub (fn [_ _] result))} - (fn [section-done] - (ptk/emit! store (local-commit file-id) ::dps/force-persist) - (t/is (= :error (get-in @store [:persistence :status]))) - (t/is (= :invalid-save-response (get-in @store [:persistence :error :code]))) - (t/is (= 1 (count (get-in @store [:persistence :queue])))) - (section-done)) - (fn []))) - (fn []))) - (done))) +;; Scenario: a valid but unsent commit sits queued with a dangling run id +;; when persistence initializes. It is sent exactly once and, once answered, +;; the file saves with an empty queue. Proves: unsent commits survive a +;; stale runner — neither stuck nor duplicated. +(t/deftest ^:async initialization-recovers-an-unsent-commit-with-a-dangling-run-id + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (let [commit (assoc @(local-commit file-id) :changes []) + id (:id commit)] + (ptk/emit! store + #(assoc % :persistence {:queue (conj #queue [] id) + :index {id commit} :run-id (uuid/next) + :status :saving}) + (dps/initialize-persistence)) + (await (async/wait-for #(= 1 (count @requests)) "dangling commit is sent")) + (t/is (= 1 (count @requests))) + (rx/push! response {:revn 1}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "recovered save lands")) + (t/is (= :saved (get-in @store [:persistence :status]))) + (t/is (empty? (get-in @store [:persistence :queue])))))))) + +(defn- check-active-request-not-resent + "Sends a commit, interrupts the active request with `interrupt` and + reinitializes persistence: the pending request must not be sent again." + [interrupt] + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (apply ptk/emit! store (local-commit file-id) ::dps/force-persist interrupt) + (ptk/emit! store (dps/initialize-persistence)) + (await (async/wait-for #(= 1 (count @requests)) "only the active request is sent")) + (t/is (= 1 (count @requests))) + (rx/push! response {:revn 1}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "active request completes")) + (t/is (= :saved (get-in @store [:persistence :status]))) + (t/is (empty? (get-in @store [:persistence :queue])))))) + +;; Variant: the interrupt is a reinitialization. +(t/deftest ^:async reinitializing-does-not-resend-an-active-request + (await (check-active-request-not-resent [(dps/initialize-persistence)]))) + +;; Variant: the interrupt is a save error event. +(t/deftest ^:async save-error-does-not-resend-an-active-request + (await (check-active-request-not-resent [(ptk/data-event ::dps/error)]))) + +;; Scenario: two commits go out, permission is lost mid-flight (first request +;; fails, one edit stays queued as failed), then permission is restored with +;; a team-role change. Only the unsent edit is resent; once answered, the +;; file saves with an empty queue. Proves: restoration resumes exactly the +;; unsent edits. +(t/deftest ^:async permission-restoration-resumes-only-unsent-edits + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (ptk/emit! store (local-commit file-id) ::dps/force-persist + (local-commit file-id) ::dps/force-persist + #(assoc-in % [:permissions :can-edit] false)) + (rx/push! response {:revn 1}) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= 1 (count (get-in @store [:persistence :queue])))) + "permission loss fails the save")) + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + (ptk/emit! store + #(assoc-in % [:permissions :can-edit] true) + (ptk/data-event :app.main.data.common/change-team-role)) + (await (async/wait-for #(= 2 (count @requests)) "restoration resends")) + (t/is (= 2 (count @requests))) + (rx/push! response {:revn 2}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "resumed save lands")) + (t/is (= :saved (get-in @store [:persistence :status]))) + (t/is (empty? (get-in @store [:persistence :queue]))))))) + +;; Scenario: the answer arrives but no runner is tracking the request +;; anymore. The acknowledged commit stays queued instead of being dropped; +;; on initialization it is settled as saved without resending. Proves: late +;; acknowledgments are neither lost nor replayed. +(t/deftest ^:async recovery-keeps-an-acknowledgment-received-without-a-runner + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (ptk/emit! store (local-commit file-id) ::dps/force-persist + (ptk/data-event ::dps/error)) + (rx/push! response {:revn 1}) + (await (async/wait-for #(= 1 (count (get-in @store [:persistence :queue]))) + "acknowledged commit stays queued")) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + (ptk/emit! store (dps/initialize-persistence)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "recovery completes without resending")) + (t/is (= 1 (count @requests)) "The acknowledged changes must not be sent again") + (t/is (= :saved (get-in @store [:persistence :status]))) + (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 + (await + (with-persistence + (^:async fn [{:keys [file-id 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))))))) + +;; Scenario: an edit is queued before persistence even initializes. On +;; initialization plus force-persist it is sent exactly once (still queued +;; until answered); once answered, the file saves. Proves: pre-init buffered +;; edits flush exactly once, without duplicating. +(t/deftest ^:async initialization-flushes-buffered-edits-without-duplicating-them + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (ptk/emit! store (local-commit file-id) + (dps/initialize-persistence) + ::dps/force-persist) + (await (async/wait-for #(and (= 1 (count @requests)) + (= 1 (count (get-in @store [:persistence :queue])))) + "buffered edit is sent once")) + (t/is (= 1 (count @requests))) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + (rx/push! response {:revn 1}) + (await (async/wait-for #(= :saved (get-in @store [:persistence :status])) + "buffered edit saves")) + (t/is (= :saved (get-in @store [:persistence :status]))))))) + +;; Scenario: the transport answers synchronously (immediate observable). +;; The save completes, the queue empties, and no runner is left dangling. +;; Proves: synchronous transport responses settle cleanly. +(t/deftest ^:async synchronous-save-results-do-not-leave-a-dangling-runner + (await + (with-persistence + (^:async fn [{:keys [file-id store]}] + (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]))) + "synchronous save completes")) + (t/is (= :saved (get-in @store [:persistence :status]))) + (t/is (empty? (get-in @store [:persistence :queue])))) + (fn [_ _] (rx/of {:revn 1}))))) diff --git a/frontend/test/frontend_tests/errors_governor_test.cljs b/frontend/test/frontend_tests/errors_governor_test.cljs new file mode 100644 index 0000000000..add35d862c --- /dev/null +++ b/frontend/test/frontend_tests/errors_governor_test.cljs @@ -0,0 +1,359 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.errors-governor-test + "Unit tests for the error report governor (`app.main.errors`). + + Tests cover: + - error fingerprinting – stable identity incl. wrapper stacks + - reserve-report* – pure window/count/eviction decisions + - submit-report – governed emission, thunk reports + - flash report pipeline – reserve-before-generate, totality" + (:require + [app.main.data.event :as ev] + [app.main.errors :as errors] + [app.main.router :as rt] + [app.main.store :as st] + [app.util.timers :as tm] + [cljs.test :as t :include-macros true] + [cuerdas.core :as str] + [frontend-tests.helpers.mock :as mock] + [potok.v2.core :as ptk])) + +;; --------------------------------------------------------------------------- +;; Error report governor +;; +;; The governor deduplicates report by fingerprint: the first occurrence is +;; always emitted, repeats inside a 2 minute window are counted, and the +;; fingerprint cache is bounded (the oldest entry is evicted when full). +;; --------------------------------------------------------------------------- + +(t/use-fixtures :each {:before #(errors/reset-report-governor!)}) + +(defn- error-cause + [& {:keys [type code hint]}] + (ex-info (or hint "boom") + (cond-> {} + (some? type) (assoc :type type) + (some? code) (assoc :code code) + (some? hint) (assoc :hint hint)))) + +(defn- capture-reports! + "Run `f` capturing the events emitted through `st/emit!`." + [f] + (let [events (atom [])] + (with-redefs [st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + rt/get-current-href (constantly "https://penpot.example.com/#/workspace")] + (f) + @events))) + +(t/deftest fingerprint-is-stable-for-equivalent-errors + (let [cause-a (error-cause :type :network :code :fetch-failed :hint "unable to perform fetch operation") + cause-b (error-cause :type :network :code :fetch-failed :hint "unable to perform fetch operation")] + (t/is (= (errors/error-fingerprint "handled-exception" cause-a) + (errors/error-fingerprint "handled-exception" cause-b))))) + +(t/deftest fingerprint-changes-with-error-identity + (let [base (error-cause :type :network :code :fetch-failed :hint "boom")] + (t/is (not= (errors/error-fingerprint "handled-exception" base) + (errors/error-fingerprint "handled-exception" + (error-cause :type :network :code :fetch-failed :hint "other")))) + (t/is (not= (errors/error-fingerprint "handled-exception" base) + (errors/error-fingerprint "handled-exception" + (error-cause :type :validation :code :fetch-failed :hint "boom")))) + (t/is (not= (errors/error-fingerprint "handled-exception" base) + (errors/error-fingerprint "handled-exception" + (error-cause :type :network :code :other :hint "boom")))))) + +(t/deftest fingerprint-includes-the-report-name + (let [cause (error-cause :type :network :hint "boom")] + (t/is (not= (errors/error-fingerprint "handled-exception" cause) + (errors/error-fingerprint "unhandled-exception" cause))) + (t/is (not= (errors/error-fingerprint "handled-exception" cause) + (errors/error-fingerprint "exception-page" cause))))) + +(t/deftest fingerprint-handles-missing-type-and-code + (let [fingerprint (errors/error-fingerprint "handled-exception" (js/Error. "plain failure"))] + (t/is (string? fingerprint)) + (t/is (str/starts-with? fingerprint "handled-exception|unknown|unknown|")))) + +(t/deftest fingerprint-skips-non-frame-preamble-lines + ;; Scenario: a bundler/polyfill wrapper prepends extra non-frame lines + ;; before the real frames. Positional indexing would take the preamble + ;; as the frame and fragment the grouping; matching the first frame + ;; line keeps the identity equal to the plain equivalent. Proves: + ;; grouping survives wrapper-prepended stacks. + (let [plain (doto (ex-info "boom" {:type :internal :hint "boom"}) + (unchecked-set "stack" "Error: boom\n at call-site-a (app.js:1)")) + wrapped (doto (ex-info "boom" {:type :internal :hint "boom"}) + (unchecked-set "stack" "Error: boom\nWrapped by polyfill\n at call-site-a (app.js:1)"))] + (t/is (= (errors/error-fingerprint "handled-exception" plain) + (errors/error-fingerprint "handled-exception" wrapped))))) + +(t/deftest environment-fingerprints-ignore-the-stack-frame + (let [cause-a (doto (ex-info "http error" {:type :offline :hint "http error"}) + (unchecked-set "stack" "Error: http error\n at call-site-a (app.js:1)")) + cause-b (doto (ex-info "http error" {:type :offline :hint "http error"}) + (unchecked-set "stack" "Error: http error\n at call-site-b (app.js:2)")) + defect-a (doto (ex-info "boom" {:type :internal :hint "boom"}) + (unchecked-set "stack" "Error: boom\n at call-site-a (app.js:1)")) + defect-b (doto (ex-info "boom" {:type :internal :hint "boom"}) + (unchecked-set "stack" "Error: boom\n at call-site-b (app.js:2)"))] + (t/testing "environment failures group across internal call sites" + (t/is (= (errors/error-fingerprint "handled-exception" cause-a) + (errors/error-fingerprint "handled-exception" cause-b)))) + (t/testing "application defects keep the stack frame in their identity" + (t/is (not= (errors/error-fingerprint "handled-exception" defect-a) + (errors/error-fingerprint "handled-exception" defect-b)))))) + +(t/deftest governor-emits-first-occurrence-and-suppresses-repeats + (let [d1 (errors/reserve-report* (errors/initial-report-state) "fp" 1000) + d2 (errors/reserve-report* d1 "fp" 2000) + d3 (errors/reserve-report* d2 "fp" 3000) + d4 (errors/reserve-report* d3 "fp" (+ 1000 errors/report-window-ms))] + (t/is (true? (::errors/emit d1))) + (t/is (= 1 (::errors/occurrences d1))) + (t/is (false? (::errors/emit d2))) + (t/is (nil? (::errors/occurrences d2))) + (t/is (false? (::errors/emit d3))) + (t/is (nil? (::errors/occurrences d3))) + (t/is (true? (::errors/emit d4))) + (t/is (= 3 (::errors/occurrences d4))))) + +(t/deftest governor-evicts-oldest-entry-when-cache-is-full + (let [base (reduce (fn [state i] + (errors/reserve-report* state (str "fp-" i) (* 1000 i))) + (errors/initial-report-state) + (range errors/max-tracked-fingerprints)) + state (errors/reserve-report* base + "fp-new" + (* 1000 errors/max-tracked-fingerprints))] + (t/is (= errors/max-tracked-fingerprints (count (:entries state)))) + (t/is (= errors/max-tracked-fingerprints (count (:order state)))) + (t/is (true? (::errors/emit state))) + (t/is (nil? (get-in state [:entries "fp-0"]))) + (t/is (= "fp-1" (peek (:order state)))) + (t/is (some? (get-in state [:entries "fp-new"]))))) + +(t/deftest governor-evicts-by-insertion-order-not-by-last-emission + (let [base (reduce (fn [state i] + (errors/reserve-report* state (str "fp-" i) (* 1000 i))) + (errors/initial-report-state) + (range errors/max-tracked-fingerprints)) + ;; fp-0 re-emits after the window, so its :emitted-at becomes the + ;; most recent one, but it keeps its insertion position. + re-emitted (errors/reserve-report* base + "fp-0" + (+ (* 1000 errors/max-tracked-fingerprints) + errors/report-window-ms)) + state (errors/reserve-report* re-emitted + "fp-new" + (+ (* 1000 errors/max-tracked-fingerprints) + errors/report-window-ms + 1000))] + (t/is (true? (::errors/emit state))) + ;; FIFO: the first inserted one goes, even though it was the last + ;; emitted and fp-1 is the oldest by :emitted-at. + (t/is (nil? (get-in state [:entries "fp-0"]))) + (t/is (some? (get-in state [:entries "fp-1"]))) + (t/is (= errors/max-tracked-fingerprints (count (:entries state)))) + (t/is (= errors/max-tracked-fingerprints (count (:order state)))))) + +(t/deftest submit-report-is-governed-and-reports-occurrences + (let [cause (error-cause :type :network :code :fetch-failed :hint "boom") + events (capture-reports! + (fn [] + (dotimes [_ 5] + (errors/submit-report :event-name "handled-exception" + :report "report" + :hint "boom" + :cause cause))))] + (t/is (= 1 (count events))) + (t/is (= 1 (:occurrences (deref (first events))))))) + +(t/deftest invalid-report-does-not-consume-a-reservation + (let [cause (error-cause :type :network :hint "boom") + events (capture-reports! + (fn [] + (errors/submit-report :event-name "handled-exception" + :report nil :hint "boom" :cause cause) + (errors/submit-report :event-name "handled-exception" + :report "report" :hint "boom" :cause cause)))] + (t/is (= 1 (count events))) + (t/is (= 1 (:occurrences (deref (first events))))))) + +(t/deftest submit-report-evaluates-thunk-reports-lazily + ;; Scenario: the same failure is submitted twice with the report as a + ;; thunk. The first occurrence is granted and builds the report; the + ;; second is suppressed and must never run the thunk. Proves: suppressed + ;; occurrences skip the report-building cost entirely. + (let [calls (atom 0) + report (fn [] (swap! calls inc) "report") + cause (error-cause :type :network :hint "boom") + events (capture-reports! + (fn [] + (errors/submit-report :event-name "handled-exception" + :report report + :hint "boom" + :cause cause) + (errors/submit-report :event-name "handled-exception" + :report report + :hint "boom" + :cause cause)))] + (t/is (= 1 (count events))) + (t/is (= 1 @calls) "only the granted occurrence builds the report") + (t/is (= "report" (:report (deref (first events))))))) + +(t/deftest governor-applies-to-every-report-name + (let [events (capture-reports! + (fn [] + (doseq [event-name ["handled-exception" "unhandled-exception" "exception-page"]] + (let [cause (error-cause :type :internal :hint event-name)] + (errors/submit-report :event-name event-name + :report "report" + :hint event-name + :cause cause) + (errors/submit-report :event-name event-name + :report "report" + :hint event-name + :cause cause)))))] + (t/is (= 3 (count events))))) + +(t/deftest submit-report-without-cause-is-ignored + (let [events (capture-reports! + (fn [] + (errors/submit-report :event-name "exception-page" :report "report" :hint "boom") + (errors/submit-report :event-name "exception-page" + :report "report" + :hint "boom" + :cause (error-cause :type :internal :hint "boom"))))] + ;; The cause-less call is ignored and must not consume the reservation + ;; of the cause-based report. + (t/is (= 1 (count events))) + (t/is (= 1 (:occurrences (deref (first events))))))) + +(t/deftest governor-bounds-an-incident-like-loop + (let [cause (error-cause :type :network :hint "unable to perform fetch operation") + events (capture-reports! + (fn [] + (dotimes [_ 10000] + (errors/submit-report :event-name "handled-exception" + :report "report" + :hint "unable to perform fetch operation" + :cause cause))))] + (t/is (= 1 (count events))))) + +(defn- report-events + "The audit events (report emissions) out of everything collected through + the `st/emit!` double. Toasts are ungoverned by design — one per `flash` + call — so only the events typed as audit events count for emission + bounds. Discriminates by `ptk/type`, which is total (never throws), + because toasts are not derefable." + [events] + (filter #(= ::ev/event (ptk/type %)) events)) + +(t/deftest ^:async flash-suppressed-occurrence-does-not-build-a-report + ;; Scenario: the same failure flashes 3 times inside the governor window. + ;; Only the first occurrence reserves emission; the suppressed ones skip + ;; generation entirely. Proves: one audit event and a single report build + ;; for the whole burst; each `flash` is awaited to completion, so no + ;; timer reasoning is involved. + (let [generated (atom 0) + cause (error-cause :type :internal :hint "unable to perform fetch operation") + 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 [_ 3] + (await (errors/flash :cause cause :type :handled))) + (t/is (= 1 (count (report-events @events)))) + (t/is (= 1 @generated)))))) + +(t/deftest ^:async flash-bounds-an-incident-like-loop + ;; Scenario: 10 000 identical flashes, replaying the incident loop. Every + ;; flash is awaited (all 10 000 scheduled at once, one wait for all of + ;; them), but the governor emits only the first occurrence. Proves: the + ;; burst produces exactly one audit event and one report build. + (let [generated (atom 0) + cause (error-cause :type :internal :hint "unable to perform fetch operation") + 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)))} + (await (js/Promise.all + (into-array + (map (fn [_] (errors/flash :cause cause :type :handled)) + (range 10000))))) + (t/is (= 1 (count (report-events @events)))) + (t/is (= 1 @generated)))))) + +(t/deftest generate-report-is-total-when-formatting-fails + (with-redefs [st/format-last-events (mock/stub (fn [& _] (throw (ex-info "formatting failed" {}))))] + (let [report (errors/generate-report (error-cause :type :network :hint "boom"))] + (t/is (string? report))))) + +(t/deftest ^:async flash-emits-a-fallback-report-when-generation-fails + ;; Scenario: the full-report formatter throws. `generate-report` stays + ;; total and the fallback string is what gets emitted. Proves: a + ;; formatting failure still produces exactly one audit event carrying a + ;; string report, with the `flash` awaited to completion. + (let [events (atom [])] + (await + (mock/with-mocks* + {st/format-last-events (mock/stub (fn [& _] (throw (ex-info "formatting failed" {})))) + 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 (error-cause :type :internal :hint "boom") :type :handled)) + (let [reports (report-events @events)] + (t/is (= 1 (count reports))) + (t/is (string? (:report (deref (first reports)))))))))) + +(t/deftest ^:async flash-emits-nothing-synchronously + ;; Scenario: a single handled failure. Nothing report- or toast-related + ;; may run on the error handler's stack: the whole `flash` body executes + ;; inside one scheduled callback. Proves: zero events right after the + ;; call returns; report + toast once the returned promise completes. + (errors/reset-report-governor!) + (let [events (atom []) + cause (error-cause :type :internal :hint "async flash probe")] + (await + (mock/with-mocks* + {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)))} + (let [completed (errors/flash :cause cause :type :handled)] + (t/is (empty? @events) "nothing emitted synchronously") + (await completed) + (t/is (= 2 (count @events)) "report + toast once completed")))))) + +(t/deftest ^:async flash-report-failure-neither-propagates-nor-kills-the-toast + ;; Scenario: the report emission itself throws (audit sink down). The + ;; failure is logged and swallowed at the schedule boundary while the + ;; toast is still attempted, so the user keeps the banner. Proves: + ;; `flash` never throws, its promise still completes, and a reporting + ;; failure still leaves the toast emitted. + (errors/reset-report-governor!) + (let [events (atom []) + calls (atom 0) + cause (error-cause :type :internal :hint "emit failure probe")] + (await + (mock/with-mocks* + {st/emit! (mock/stub (fn [& emitted] + (if (zero? @calls) + (do (swap! calls inc) + (throw (ex-info "sink down" {}))) + (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)) + (t/is (= 1 (count @events)) "only the toast survives a reporting failure"))))) diff --git a/frontend/test/frontend_tests/fonts_test.cljs b/frontend/test/frontend_tests/fonts_test.cljs index 284b77635a..11ba4d9573 100644 --- a/frontend/test/frontend_tests/fonts_test.cljs +++ b/frontend/test/frontend_tests/fonts_test.cljs @@ -11,6 +11,7 @@ [app.util.http :as http] [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] [frontend-tests.helpers.mock :as mock])) (def sample-font @@ -139,104 +140,107 @@ (t/use-fixtures :each - (fn [test-fn] - (reset! fonts/preview-sprite {:status :idle :ids #{} :node nil :refs 0}) - (test-fn))) + {:before #(reset! fonts/preview-sprite {:status :idle :ids #{} :node nil :refs 0})}) (defn- fake-node "A minimal DOM-like node exposing only what the sprite attach/detach touches." [] #js {:remove (fn [] nil)}) -(t/deftest attach-preview-sprite-returns-nil-while-sprite-is-not-ready - (mock/with-mocks - {globals/browser? (mock/stub (constantly true))} - (fn [done] - (reset! fonts/preview-sprite {:status :loading :ids #{} :node nil :refs 0}) - (t/is (nil? (fonts/attach-preview-sprite!))) - (t/is (= 0 (:refs @fonts/preview-sprite))) +(t/deftest ^:async attach-preview-sprite-returns-nil-while-sprite-is-not-ready + (await + (mock/with-mocks* + {globals/browser? (mock/stub (constantly true))} + (reset! fonts/preview-sprite {:status :loading :ids #{} :node nil :refs 0}) + (t/is (nil? (fonts/attach-preview-sprite!))) + (t/is (= 0 (:refs @fonts/preview-sprite))) - (reset! fonts/preview-sprite {:status :error :ids #{} :node nil :refs 0}) - (t/is (nil? (fonts/attach-preview-sprite!))) - (t/is (= 0 (:refs @fonts/preview-sprite))) - (done)) - (fn [] nil))) + (reset! fonts/preview-sprite {:status :error :ids #{} :node nil :refs 0}) + (t/is (nil? (fonts/attach-preview-sprite!))) + (t/is (= 0 (:refs @fonts/preview-sprite))) + ;; Trailing settle: the body is synchronous, but `with-mocks*` + ;; evaluates to a promise, so the body must settle one. + (await (async/settle))))) -(t/deftest attach-preview-sprite-increments-refs-and-returns-the-node - (mock/with-mocks - {globals/browser? (mock/stub (constantly true))} - (fn [done] - (let [node (fake-node)] - (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0}) - (t/is (identical? node (fonts/attach-preview-sprite!))) - (t/is (= 1 (:refs @fonts/preview-sprite))) - (t/is (identical? node (fonts/attach-preview-sprite!))) - (t/is (= 2 (:refs @fonts/preview-sprite))) - (done))) - (fn [] nil))) +(t/deftest ^:async attach-preview-sprite-increments-refs-and-returns-the-node + (await + (mock/with-mocks* + {globals/browser? (mock/stub (constantly true))} + (let [node (fake-node)] + (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0}) + (t/is (identical? node (fonts/attach-preview-sprite!))) + (t/is (= 1 (:refs @fonts/preview-sprite))) + (t/is (identical? node (fonts/attach-preview-sprite!))) + (t/is (= 2 (:refs @fonts/preview-sprite))) + ;; Trailing settle: the body is synchronous, but `with-mocks*` + ;; evaluates to a promise, so the body must settle one. + (await (async/settle)))))) -(t/deftest detach-preview-sprite-removes-node-only-when-last-reference-drops - (mock/with-mocks - {globals/browser? (mock/stub (constantly true))} - (fn [done] - (let [removed? (volatile! false) - node #js {:remove (fn [] (vreset! removed? true))}] - (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0}) - (fonts/attach-preview-sprite!) - (fonts/attach-preview-sprite!) +(t/deftest ^:async detach-preview-sprite-removes-node-only-when-last-reference-drops + (await + (mock/with-mocks* + {globals/browser? (mock/stub (constantly true))} + (let [removed? (volatile! false) + node #js {:remove (fn [] (vreset! removed? true))}] + (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0}) + (fonts/attach-preview-sprite!) + (fonts/attach-preview-sprite!) - ;; First detach keeps the node: another dropdown is still open. - (fonts/detach-preview-sprite! node) - (t/is (= 1 (:refs @fonts/preview-sprite))) - (t/is (false? @removed?)) + ;; First detach keeps the node: another dropdown is still open. + (fonts/detach-preview-sprite! node) + (t/is (= 1 (:refs @fonts/preview-sprite))) + (t/is (false? @removed?)) - ;; Second detach reaches zero refs, so the node is removed from the DOM. - (fonts/detach-preview-sprite! node) - (t/is (= 0 (:refs @fonts/preview-sprite))) - (t/is (true? @removed?)) - (done))) - (fn [] nil))) + ;; Second detach reaches zero refs, so the node is removed from the DOM. + (fonts/detach-preview-sprite! node) + (t/is (= 0 (:refs @fonts/preview-sprite))) + (t/is (true? @removed?)) + ;; Trailing settle: the body is synchronous, but `with-mocks*` + ;; evaluates to a promise, so the body must settle one. + (await (async/settle)))))) -(t/deftest detach-preview-sprite-clamps-refs-at-zero - (mock/with-mocks - {globals/browser? (mock/stub (constantly true))} - (fn [done] - (let [removed? (volatile! false) - node #js {:remove (fn [] (vreset! removed? true))}] - (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0}) - (fonts/detach-preview-sprite! node) - (t/is (= 0 (:refs @fonts/preview-sprite))) - (t/is (true? @removed?)) - (done))) - (fn [] nil))) +(t/deftest ^:async detach-preview-sprite-clamps-refs-at-zero + (await + (mock/with-mocks* + {globals/browser? (mock/stub (constantly true))} + (let [removed? (volatile! false) + node #js {:remove (fn [] (vreset! removed? true))}] + (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node node :refs 0}) + (fonts/detach-preview-sprite! node) + (t/is (= 0 (:refs @fonts/preview-sprite))) + (t/is (true? @removed?)) + ;; Trailing settle: the body is synchronous, but `with-mocks*` + ;; evaluates to a promise, so the body must settle one. + (await (async/settle)))))) -(t/deftest prefetch-preview-sprite-fetches-only-from-idle-or-error +(t/deftest ^:async prefetch-preview-sprite-fetches-only-from-idle-or-error (let [calls (volatile! 0) fetch (mock/stub (fn [& _] (vswap! calls inc) (rx/empty)))] - (mock/with-mocks - {globals/browser? (mock/stub (constantly true)) - http/fetch fetch} - (fn [done] - ;; :ready → no refetch - (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node (fake-node) :refs 0}) - (fonts/prefetch-preview-sprite!) - (t/is (= 0 @calls)) + (await + (mock/with-mocks* + {globals/browser? (mock/stub (constantly true)) + http/fetch fetch} + ;; :ready → no refetch + (reset! fonts/preview-sprite {:status :ready :ids #{"a"} :node (fake-node) :refs 0}) + (fonts/prefetch-preview-sprite!) + (t/is (= 0 @calls)) - ;; :loading → no refetch (an earlier request is in flight) - (reset! fonts/preview-sprite {:status :loading :ids #{} :node nil :refs 0}) - (fonts/prefetch-preview-sprite!) - (t/is (= 0 @calls)) + ;; :loading → no refetch (an earlier request is in flight) + (reset! fonts/preview-sprite {:status :loading :ids #{} :node nil :refs 0}) + (fonts/prefetch-preview-sprite!) + (t/is (= 0 @calls)) - ;; :error → retries - (reset! fonts/preview-sprite {:status :error :ids #{} :node nil :refs 0}) - (fonts/prefetch-preview-sprite!) - (t/is (= 1 @calls)) + ;; :error → retries + (reset! fonts/preview-sprite {:status :error :ids #{} :node nil :refs 0}) + (fonts/prefetch-preview-sprite!) + (t/is (= 1 @calls)) - ;; :idle → first fetch - (reset! fonts/preview-sprite {:status :idle :ids #{} :node nil :refs 0}) - (fonts/prefetch-preview-sprite!) - (t/is (= 2 @calls)) - (done)) - (fn [] nil)))) + ;; :idle → first fetch + (reset! fonts/preview-sprite {:status :idle :ids #{} :node nil :refs 0}) + (fonts/prefetch-preview-sprite!) + (t/is (= 2 @calls)) + ;; Trailing settle: the body is synchronous, but `with-mocks*` + ;; evaluates to a promise, so the body must settle one. + (await (async/settle)))))) diff --git a/frontend/test/frontend_tests/helpers/async.cljs b/frontend/test/frontend_tests/helpers/async.cljs new file mode 100644 index 0000000000..36bbaed769 --- /dev/null +++ b/frontend/test/frontend_tests/helpers/async.cljs @@ -0,0 +1,92 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.helpers.async + "Async-first plumbing for ClojureScript tests. + + Lets a test read sequentially while presupposing asynchronous APIs: + triggers run, effects are awaited as promises, and `done` always runs + at the end. Relies on two platform properties: + + - `cljs.test` keeps its env in a `set!` var, so assertions inside + deferred ticks are still counted. + - RxJS delivers `observe-on :async` notifications in scheduling (FIFO) + order, so awaiting the last scheduled delivery observes every effect + scheduled before it." + (:require + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true])) + +(defn ->promise + "Coerces a single-value observable into a js/Promise that resolves + with its first value (or rejects on error)." + [ob] + (js/Promise. (fn [resolve reject] (rx/subs! resolve reject ob)))) + +(defn ^:async await-response + "Pushes `value` into the `response` subject and resolves once the last + recorded request completes, delivery included. + + The promise subscribes BEFORE pushing: subscribing after the push + would never resolve. Request entries are maps holding the request + observable under `:req`." + [requests response value] + (let [p (->promise (:req (last @requests)))] + (rx/push! response value) + (await p))) + +(defn settle + "Resolves on the next macrotask, letting scheduled deliveries land. + Await it before asserting absence: silence is only meaningful once the + queue had a chance to deliver." + [] + (js/Promise. (fn [resolve] (js/setTimeout resolve 0)))) + +(defn observe + "Subscribes to `ob` forcing asynchronous delivery through `observe-on + :async` — even when the source is synchronous — and returns a promise + resolving once the stream terminates. + + Keyword args: `:on-next` per value, `:on-error` (handled errors resolve; + unhandled reject), `:on-complete` on clean termination, `:timeout-ms` + (default 2000). Asserts the stream is provided: resolving nil would let + a test pass without observing anything. Rejects on timeout instead of + hanging." + [ob & {:keys [on-next on-error on-complete timeout-ms] + :or {timeout-ms 2000}}] + (assert (some? ob) "observe requires an observable stream") + (js/Promise. + (fn [resolve reject] + (let [timer (js/setTimeout + #(reject (ex-info "Stream did not terminate in time" {})) + timeout-ms) + settle (fn [f v] + (js/clearTimeout timer) + (f v))] + (->> ob + (rx/observe-on :async) + (rx/subs! + (fn [v] (when on-next (on-next v))) + (fn [e] + (if on-error + (do (on-error e) (settle resolve nil)) + (settle reject e))) + (fn [] + (when on-complete (on-complete)) + (settle resolve nil)))))))) + +(defn ^:async wait-for + "Resolves once `pred` holds, checking immediately and then once per + macrotask (bounded: records a failure instead of hanging). Await it + before asserting effects of triggers — it holds whether the effect + lands synchronously or not." + [pred msg & [{:keys [max-ticks] :or {max-ticks 50}}]] + (if (pred) + nil + (if (<= max-ticks 0) + (t/is false (str "Timed out waiting for: " msg)) + (do (await (settle)) + (await (wait-for pred msg {:max-ticks (dec max-ticks)})))))) diff --git a/frontend/test/frontend_tests/helpers/mock.cljc b/frontend/test/frontend_tests/helpers/mock.cljc index d342c80002..b55b8e24bd 100644 --- a/frontend/test/frontend_tests/helpers/mock.cljc +++ b/frontend/test/frontend_tests/helpers/mock.cljc @@ -19,13 +19,20 @@ The `with-mocks` helper wraps the lifecycle: 1. Reset recording atoms 2. Save original var values, install mocks via `set!` - 3. Execute `(test-fn inner-done)` + 3. Defer `(test-fn inner-done)` past the current tick via `asap`, + so the body provably runs with mocks that survived an async + boundary 4. `inner-done` restores originals and calls `outer-done` - (typically `cljs.test/async`'s done). + (typically `cljs.test/async`'s done). A throw inside the deferred + body is reported as an `:error`, then restores and completes. + + Requires a done-chained context (usually `t/async`): a synchronous + test completes before its deferred body runs. Usage: `(with-mocks {ns/sym mock-fn, ...} test-fn done)`" #?(:cljs (:require - [beicon.v2.core :as rx])) + [beicon.v2.core :as rx] + [cljs.test :as t])) #?(:cljs (:require-macros [frontend-tests.helpers.mock]))) ;; ═══════════════════════════════════════════════════════════════ @@ -34,15 +41,19 @@ #?(:clj (defmacro with-mocks - "Resets recording atoms, installs `mocks` via `set!`, then - calls `(test-fn inner-done)`. Original var values are restored - when `inner-done` is called. + "Resets recording atoms, installs `mocks` via `set!`, then defers + `(test-fn inner-done)` past the current tick via `asap`. `mocks` is a map of sym → mock-fn (e.g. `{app.main.repo/cmd! mock-fn}`). `inner-done` restores the originals and calls `outer-done` (the - `cljs.test/async` `done` callback). + `cljs.test/async` `done` callback). A throw inside the deferred body + is reported as an `:error` before restoring and completing, so a + failing body can neither leak the mocks nor stall the run. + + Requires a done-chained context (usually `t/async`): a synchronous + caller completes before its deferred body runs. Example: @@ -55,7 +66,8 @@ (rx/subs! (fn [v] ...) (fn [err] (done')) - (fn [] (done'))))))))" + (fn [] (done'))))) + done)))" [mocks test-fn outer-done] (let [entries (map identity mocks) gen-pairs (mapv (fn [[qsym _mock]] @@ -76,13 +88,67 @@ (fn [{:keys [qsym osym]}] `(set! ~qsym ~osym)) gen-pairs)] + `(let [test-fn# ~test-fn] + (frontend-tests.helpers.mock/reset-state!) + (let ~let-bindings + ~@install-exprs + (frontend-tests.helpers.mock/asap + (fn [] + (try + (test-fn# (fn [] + ~@restore-exprs + (~outer-done))) + (catch :default e# + ~@restore-exprs + (cljs.test/report {:type :error + :message "Uncaught exception, not in assertion." + :expected nil + :actual e#}) + (~outer-done)))))))))) + +#?(:clj + (defmacro with-mocks* + "Installs `mocks` via `set!`, then evaluates `body` inside a generated + `^:async` fn awaited via `run-mocked`: when the body settles, the + originals are restored — always. A rejection is reported as an `:error`. + + Evaluates to a promise resolving once the body settles and the mocks + are restored. `await` it, either directly or by returning it from an + `^:async` fn; nested scopes must be awaited too. + + Example: + + (t/deftest ^:async my-async-test + (await (mock/with-mocks* + {app.main.repo/cmd! mock/rpc-cmd-mock} + (await (some-async-flow)) + ...)))" + [mocks & body] + (let [entries (map identity mocks) + gen-pairs (mapv (fn [[qsym _mock]] + {:qsym qsym + :osym (gensym "orig-")}) + entries) + let-bindings (vec (mapcat + (fn [{:keys [qsym osym]}] + [osym qsym]) + gen-pairs)) + install-exprs (mapv + (fn [[_qsym mock-fn] {:keys [qsym]}] + `(set! ~qsym ~mock-fn)) + entries + gen-pairs) + restore-exprs (mapv + (fn [{:keys [qsym osym]}] + `(set! ~qsym ~osym)) + gen-pairs)] `(do (frontend-tests.helpers.mock/reset-state!) (let ~let-bindings ~@install-exprs - (~test-fn (fn inner-done# [] - ~@restore-exprs - (~outer-done)))))))) + (frontend-tests.helpers.mock/run-mocked + (^:async fn [] ~@body) + (fn [] ~@restore-exprs))))))) ;; ═══════════════════════════════════════════════════════════════ ;; Runtime (ClojureScript only) @@ -164,6 +230,41 @@ ([a b c d e] (f a b c d e)) ([a b c d e g] (f a b c d e g)))) + ;; Scheduling + ;; ═══════════════════════════════════════════════════════════════ + + (defn asap + "Runs `f` on the next macrotask (`js/setTimeout` 0) and returns + the timer id. Used by `with-mocks` to defer the test body past + the current tick, so installed mocks provably survive async + boundaries." + [f] + (js/setTimeout f 0)) + + (defn ^:async run-mocked + "Awaits the async zero-arg `thunk`, then runs `restore` — always, in + that order — and resolves. A rejection is reported as an `:error` + (uncaught-exception semantics, like `test-var-block*`); a non-promise + return is reported as an `:error` too." + [thunk restore] + (let [p (thunk)] + (if (and (some? p) (fn? (.-then p))) + (try + (await p) + (catch :default e + (t/report {:type :error + :message "Uncaught exception, not in assertion." + :expected nil + :actual e})) + (finally + (restore))) + (do + (restore) + (t/report {:type :error + :message "with-mocks* body did not return a promise." + :expected nil + :actual p}))))) + ;; Lifecycle ;; ═══════════════════════════════════════════════════════════════ diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs index cdb8b84ec9..a964620c15 100644 --- a/frontend/test/frontend_tests/main_errors_test.cljs +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -11,7 +11,8 @@ - stale-asset-error? – pure predicate - exception->error-data – pure transformer - on-error re-entrancy guard – prevents recursive invocations - - flash schedules async emit – ntf/show is not emitted synchronously + - flash schedules async report and toast – neither the report nor + ntf/show is emitted synchronously - organization SSO recovery – expired SSO sessions go back to the provider - invalid-sso-config handler – requires :organization-id to promote to :sso-error - save failure notification – sticky toast carrying the error report @@ -33,6 +34,7 @@ [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] [cuerdas.core :as str] + [frontend-tests.helpers.async :as async] [frontend-tests.helpers.mock :as mock] [potok.v2.core :as ptk])) @@ -99,14 +101,8 @@ data (errors/exception->error-data err)] (t/is (= "fallback message" (:hint data)))))) -;; --------------------------------------------------------------------------- -;; Error report governor -;; -;; The governor deduplicates report by fingerprint: the first occurrence is -;; always emitted, repeats inside a 2 minute window are counted, and the -;; fingerprint cache is bounded (the oldest entry is evicted when full). -;; --------------------------------------------------------------------------- - +;; Shared report-test helpers (copied from the governor suite so each +;; namespace stays self-contained). (t/use-fixtures :each {:before #(errors/reset-report-governor!)}) (defn- error-cause @@ -126,215 +122,14 @@ (f) @events))) -(t/deftest fingerprint-is-stable-for-equivalent-errors - (let [cause-a (error-cause :type :network :code :fetch-failed :hint "unable to perform fetch operation") - cause-b (error-cause :type :network :code :fetch-failed :hint "unable to perform fetch operation")] - (t/is (= (errors/error-fingerprint "handled-exception" cause-a) - (errors/error-fingerprint "handled-exception" cause-b))))) - -(t/deftest fingerprint-changes-with-error-identity - (let [base (error-cause :type :network :code :fetch-failed :hint "boom")] - (t/is (not= (errors/error-fingerprint "handled-exception" base) - (errors/error-fingerprint "handled-exception" - (error-cause :type :network :code :fetch-failed :hint "other")))) - (t/is (not= (errors/error-fingerprint "handled-exception" base) - (errors/error-fingerprint "handled-exception" - (error-cause :type :validation :code :fetch-failed :hint "boom")))) - (t/is (not= (errors/error-fingerprint "handled-exception" base) - (errors/error-fingerprint "handled-exception" - (error-cause :type :network :code :other :hint "boom")))))) - -(t/deftest fingerprint-includes-the-report-name - (let [cause (error-cause :type :network :hint "boom")] - (t/is (not= (errors/error-fingerprint "handled-exception" cause) - (errors/error-fingerprint "unhandled-exception" cause))) - (t/is (not= (errors/error-fingerprint "handled-exception" cause) - (errors/error-fingerprint "exception-page" cause))))) - -(t/deftest fingerprint-handles-missing-type-and-code - (let [fingerprint (errors/error-fingerprint "handled-exception" (js/Error. "plain failure"))] - (t/is (string? fingerprint)) - (t/is (str/starts-with? fingerprint "handled-exception|unknown|unknown|")))) - -(t/deftest environment-fingerprints-ignore-the-stack-frame - (let [cause-a (doto (ex-info "http error" {:type :offline :hint "http error"}) - (unchecked-set "stack" "Error: http error\n at call-site-a (app.js:1)")) - cause-b (doto (ex-info "http error" {:type :offline :hint "http error"}) - (unchecked-set "stack" "Error: http error\n at call-site-b (app.js:2)")) - defect-a (doto (ex-info "boom" {:type :internal :hint "boom"}) - (unchecked-set "stack" "Error: boom\n at call-site-a (app.js:1)")) - defect-b (doto (ex-info "boom" {:type :internal :hint "boom"}) - (unchecked-set "stack" "Error: boom\n at call-site-b (app.js:2)"))] - (t/testing "environment failures group across internal call sites" - (t/is (= (errors/error-fingerprint "handled-exception" cause-a) - (errors/error-fingerprint "handled-exception" cause-b)))) - (t/testing "application defects keep the stack frame in their identity" - (t/is (not= (errors/error-fingerprint "handled-exception" defect-a) - (errors/error-fingerprint "handled-exception" defect-b)))))) - -(t/deftest governor-emits-first-occurrence-and-suppresses-repeats - (let [d1 (errors/reserve-report* (errors/initial-report-state) "fp" 1000) - d2 (errors/reserve-report* d1 "fp" 2000) - d3 (errors/reserve-report* d2 "fp" 3000) - d4 (errors/reserve-report* d3 "fp" (+ 1000 errors/report-window-ms))] - (t/is (true? (::errors/emit d1))) - (t/is (= 1 (::errors/occurrences d1))) - (t/is (false? (::errors/emit d2))) - (t/is (nil? (::errors/occurrences d2))) - (t/is (false? (::errors/emit d3))) - (t/is (nil? (::errors/occurrences d3))) - (t/is (true? (::errors/emit d4))) - (t/is (= 3 (::errors/occurrences d4))))) - -(t/deftest governor-evicts-oldest-entry-when-cache-is-full - (let [base (reduce (fn [state i] - (errors/reserve-report* state (str "fp-" i) (* 1000 i))) - (errors/initial-report-state) - (range errors/max-tracked-fingerprints)) - state (errors/reserve-report* base - "fp-new" - (* 1000 errors/max-tracked-fingerprints))] - (t/is (= errors/max-tracked-fingerprints (count (:entries state)))) - (t/is (= errors/max-tracked-fingerprints (count (:order state)))) - (t/is (true? (::errors/emit state))) - (t/is (nil? (get-in state [:entries "fp-0"]))) - (t/is (= "fp-1" (peek (:order state)))) - (t/is (some? (get-in state [:entries "fp-new"]))))) - -(t/deftest governor-evicts-by-insertion-order-not-by-last-emission - (let [base (reduce (fn [state i] - (errors/reserve-report* state (str "fp-" i) (* 1000 i))) - (errors/initial-report-state) - (range errors/max-tracked-fingerprints)) - ;; fp-0 re-emits after the window, so its :emitted-at becomes the - ;; most recent one, but it keeps its insertion position. - re-emitted (errors/reserve-report* base - "fp-0" - (+ (* 1000 errors/max-tracked-fingerprints) - errors/report-window-ms)) - state (errors/reserve-report* re-emitted - "fp-new" - (+ (* 1000 errors/max-tracked-fingerprints) - errors/report-window-ms - 1000))] - (t/is (true? (::errors/emit state))) - ;; FIFO: the first inserted one goes, even though it was the last - ;; emitted and fp-1 is the oldest by :emitted-at. - (t/is (nil? (get-in state [:entries "fp-0"]))) - (t/is (some? (get-in state [:entries "fp-1"]))) - (t/is (= errors/max-tracked-fingerprints (count (:entries state)))) - (t/is (= errors/max-tracked-fingerprints (count (:order state)))))) - -(t/deftest submit-report-is-governed-and-reports-occurrences - (let [cause (error-cause :type :network :code :fetch-failed :hint "boom") - events (capture-reports! - (fn [] - (dotimes [_ 5] - (errors/submit-report :event-name "handled-exception" - :report "report" - :hint "boom" - :cause cause))))] - (t/is (= 1 (count events))) - (t/is (= 1 (:occurrences (deref (first events))))))) - -(t/deftest invalid-report-does-not-consume-a-reservation - (let [cause (error-cause :type :network :hint "boom") - events (capture-reports! - (fn [] - (errors/submit-report :event-name "handled-exception" - :report nil :hint "boom" :cause cause) - (errors/submit-report :event-name "handled-exception" - :report "report" :hint "boom" :cause cause)))] - (t/is (= 1 (count events))) - (t/is (= 1 (:occurrences (deref (first events))))))) - -(t/deftest governor-applies-to-every-report-name - (let [events (capture-reports! - (fn [] - (doseq [event-name ["handled-exception" "unhandled-exception" "exception-page"]] - (let [cause (error-cause :type :internal :hint event-name)] - (errors/submit-report :event-name event-name - :report "report" - :hint event-name - :cause cause) - (errors/submit-report :event-name event-name - :report "report" - :hint event-name - :cause cause)))))] - (t/is (= 3 (count events))))) - -(t/deftest submit-report-without-cause-is-ignored - (let [events (capture-reports! - (fn [] - (errors/submit-report :event-name "exception-page" :report "report" :hint "boom") - (errors/submit-report :event-name "exception-page" - :report "report" - :hint "boom" - :cause (error-cause :type :internal :hint "boom"))))] - ;; The cause-less call is ignored and must not consume the reservation - ;; of the cause-based report. - (t/is (= 1 (count events))) - (t/is (= 1 (:occurrences (deref (first events))))))) - -(t/deftest governor-bounds-an-incident-like-loop - (let [cause (error-cause :type :network :hint "unable to perform fetch operation") - events (capture-reports! - (fn [] - (dotimes [_ 10000] - (errors/submit-report :event-name "handled-exception" - :report "report" - :hint "unable to perform fetch operation" - :cause cause))))] - (t/is (= 1 (count events))))) - -(t/deftest flash-suppressed-occurrence-does-not-build-a-report - (let [generated (atom 0) - cause (error-cause :type :internal :hint "unable to perform fetch operation") - events (atom [])] - (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/noop} - (fn [done'] - (dotimes [_ 3] - (errors/flash :cause cause :type :handled)) - (t/is (= 1 (count @events))) - (t/is (= 1 @generated)) - (done')) - (fn [])))) - -(t/deftest flash-bounds-an-incident-like-loop - (let [generated (atom 0) - cause (error-cause :type :internal :hint "unable to perform fetch operation") - events (atom [])] - (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/noop} - (fn [done'] - (dotimes [_ 10000] - (errors/flash :cause cause :type :handled)) - (t/is (= 1 (count @events))) - (t/is (= 1 @generated)) - (done')) - (fn [])))) - -(t/deftest generate-report-is-total-when-formatting-fails - (with-redefs [st/format-last-events (mock/stub (fn [& _] (throw (ex-info "formatting failed" {}))))] - (let [report (errors/generate-report (error-cause :type :network :hint "boom"))] - (t/is (string? report))))) - -(t/deftest flash-emits-a-fallback-report-when-generation-fails - (let [events (atom [])] - (with-redefs [st/format-last-events (mock/stub (fn [& _] (throw (ex-info "formatting failed" {})))) - st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) - rt/get-current-href (constantly "https://penpot.example.com/#/workspace") - tm/schedule mock/noop] - (errors/flash :cause (error-cause :type :internal :hint "boom") :type :handled) - (t/is (= 1 (count @events))) - (t/is (string? (:report (deref (first @events)))))))) +(defn- report-events + "The audit events (report emissions) out of everything collected through + the `st/emit!` double. Toasts are ungoverned by design — one per `flash` + call — so only the events typed as audit events count for emission + bounds. Discriminates by `ptk/type`, which is total (never throws), + because toasts are not derefable." + [events] + (filter #(= ::ev/event (ptk/type %)) events)) ;; --------------------------------------------------------------------------- ;; Environment failures @@ -357,64 +152,75 @@ (t/is (false? (errors/environment-error? (js/Error. "plain failure")))) (t/is (false? (errors/environment-error? nil))))) -(t/deftest generate-report-compact-omits-stack-data-and-last-events - (mock/with-mocks - {st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {})))) - rt/get-current-href (constantly "https://penpot.example.com/#/workspace")} - (fn [done'] - (let [cause (ex-info "http error" {:type :offline - :hint "http error" - :uri "/api/rpc/command/update-file" - :headers {"x-session-id" "secret"}}) - report (errors/generate-report cause {:format :compact})] - (t/is (string? report)) - (t/is (str/includes? report "Hint:")) - (t/is (str/includes? report "http error")) - (t/is (str/includes? report ":offline")) - (t/is (str/includes? report "/api/rpc/command/update-file")) - (t/is (not (str/includes? report "Last events:"))) - (t/is (not (str/includes? report "Data:"))) - (t/is (not (str/includes? report "===="))) - (t/is (not (str/includes? report "secret")))) - (done')) - (fn []))) +(t/deftest ^:async generate-report-compact-omits-stack-data-and-last-events + (await + (mock/with-mocks* + {st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {})))) + rt/get-current-href (constantly "https://penpot.example.com/#/workspace")} + (let [cause (ex-info "http error" {:type :offline + :hint "http error" + :uri "/api/rpc/command/update-file" + :headers {"x-session-id" "secret"}}) + report (errors/generate-report cause {:format :compact})] + (t/is (string? report)) + (t/is (str/includes? report "Hint:")) + (t/is (str/includes? report "http error")) + (t/is (str/includes? report ":offline")) + (t/is (str/includes? report "/api/rpc/command/update-file")) + (t/is (not (str/includes? report "Last events:"))) + (t/is (not (str/includes? report "Data:"))) + (t/is (not (str/includes? report "===="))) + (t/is (not (str/includes? report "secret"))) + ;; Trailing settle: the body is synchronous, but `with-mocks*` + ;; evaluates to a promise, so the body must settle one. + (await (async/settle)))))) -(t/deftest generate-report-defaults-to-the-full-format - (mock/with-mocks - {st/format-last-events (mock/stub (fn [& _] "(stub last events)")) - rt/get-current-href (constantly "https://penpot.example.com/#/workspace")} - (fn [done'] - (let [report (errors/generate-report (error-cause :type :internal :hint "boom"))] - (t/is (str/includes? report "Last events:")) - (t/is (str/includes? report "(stub last events)"))) - (done')) - (fn []))) +(t/deftest ^:async generate-report-defaults-to-the-full-format + (await + (mock/with-mocks* + {st/format-last-events (mock/stub (fn [& _] "(stub last events)")) + rt/get-current-href (constantly "https://penpot.example.com/#/workspace")} + (let [report (errors/generate-report (error-cause :type :internal :hint "boom"))] + (t/is (str/includes? report "Last events:")) + (t/is (str/includes? report "(stub last events)")) + ;; Trailing settle: the body is synchronous, but `with-mocks*` + ;; evaluates to a promise, so the body must settle one. + (await (async/settle)))))) -(t/deftest connectivity-handlers-report-governed-compact-audit-events +(t/deftest ^:async connectivity-handlers-report-governed-compact-audit-events + ;; Scenario: a network failure and an offline status through the global + ;; handler. Each is reported as a governed compact audit event plus its + ;; connection toast. Proves: per type, report first and toast right after, + ;; with no stack in the report and the dedicated toast message. (doseq [type [:network :offline]] (errors/reset-report-governor!) (let [events (atom []) cause (ex-info "http error" {:type type :hint "http error"})] - (mock/with-mocks - {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] (f))) - st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {}))))} - (fn [done'] - (errors/on-error cause) - ;; `flash` emits the report first and schedules the toast right after. - (t/is (= 2 (count @events)) (str "unexpected event count for " type)) - (let [report-event (first @events) - toast-event (second @events) - props (deref report-event)] - (t/is (= "handled-exception" (::ev/name props))) - (t/is (not (str/includes? (:report props) "Last events:"))) - (t/is (= (i18n/tr "errors.connection-error") - (get-in (ptk/update toast-event {}) [:notification :content])))) - (done')) - (fn []))))) + (await + (mock/with-mocks* + {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] (f))) + st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {}))))} + (errors/on-error cause) + (await (async/settle)) + ;; `flash` emits the report first and schedules the toast right after. + (t/is (= 2 (count @events)) (str "unexpected event count for " type)) + (let [report-event (first @events) + toast-event (second @events) + props (deref report-event)] + (t/is (= "handled-exception" (::ev/name props))) + (t/is (not (str/includes? (:report props) "Last events:"))) + (t/is (= (i18n/tr "errors.connection-error") + (get-in (ptk/update toast-event {}) [:notification :content]))))))))) -(t/deftest flash-keeps-the-canonical-event-name-and-derives-the-format +(t/deftest ^:async flash-keeps-the-canonical-event-name-and-derives-the-format + ;; Scenario: an environment failure flashed as `:handled` and as + ;; `:unhandled`. The audit event name is the canonical one requested by + ;; the caller while only the payload format derives from the cause + ;; (compact, with no stack and no header dump). Proves: per type, one + ;; compact audit event under the requested name, with the `flash` + ;; awaited to completion. (let [cause (ex-info "http error" {:type :network :hint "http error" :headers {"x-session-id" "secret"}})] @@ -422,58 +228,65 @@ [:unhandled "unhandled-exception"]]] (errors/reset-report-governor!) (let [events (atom [])] - (mock/with-mocks - {st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) - rt/get-current-href (constantly "https://penpot.example.com/#/workspace") - tm/schedule mock/noop - st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {}))))} - (fn [done'] - ;; The event name is the canonical one requested by the caller; - ;; only the payload format is derived from the cause. - (errors/flash :cause cause :type type) - (t/is (= 1 (count @events)) (str "unexpected event count for " type)) - (let [props (deref (first @events)) - report (:report props)] - (t/is (= event-name (::ev/name props)) (str "event name for " type)) - (t/is (not (str/includes? report "Last events:"))) - (t/is (not (str/includes? report "secret")))) - (done')) - (fn [])))))) + (await + (mock/with-mocks* + {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))) + st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {}))))} + ;; The event name is the canonical one requested by the caller; + ;; only the payload format is derived from the cause. + (await (errors/flash :cause cause :type type)) + (let [reports (report-events @events)] + (t/is (= 1 (count reports)) (str "unexpected event count for " type)) + (let [props (deref (first reports)) + report (:report props)] + (t/is (= event-name (::ev/name props)) (str "event name for " type)) + (t/is (not (str/includes? report "Last events:"))) + (t/is (not (str/includes? report "secret"))))))))))) -(t/deftest offline-loop-is-governed-and-never-unhandled +(t/deftest ^:async offline-loop-is-governed-and-never-unhandled + ;; Scenario: 10 000 offline errors through the global handler, replaying + ;; a connectivity outage. Every occurrence is handled (never unhandled) + ;; and the governor emits only the first. Proves: one `handled-exception` + ;; audit event for the whole loop, observed after timers drain. (let [events (atom []) cause (ex-info "http error" {:type :offline :hint "http error"})] - (mock/with-mocks - {st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) - rt/get-current-href (constantly "https://penpot.example.com/#/workspace") - tm/schedule mock/noop - st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {}))))} - (fn [done'] - (dotimes [_ 10000] - (errors/on-error cause)) - (t/is (= 1 (count @events))) - (t/is (= "handled-exception" (::ev/name (deref (first @events))))) - (done')) - (fn [])))) + (await + (mock/with-mocks* + {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))) + st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {}))))} + (dotimes [_ 10000] + (errors/on-error cause)) + (await (async/settle)) + (t/is (= 1 (count (report-events @events)))) + (t/is (= "handled-exception" (::ev/name (deref (first (report-events @events)))))))))) -(t/deftest flash-persistence-uses-compact-reports-for-environment-failures +(t/deftest ^:async flash-persistence-uses-compact-reports-for-environment-failures + ;; Scenario: a save failure caused by the environment. `flash-persistence` + ;; delegates to `flash`, so the audit event carries a compact report + ;; (context only: no stack, no header dump). Proves: one + ;; `handled-exception` with a compact payload, observed after timers + ;; drain. (let [events (atom [])] - (mock/with-mocks - {st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) - rt/get-current-href (constantly "https://penpot.example.com/#/workspace") - tm/schedule mock/noop - st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {}))))} - (fn [done'] - (errors/flash-persistence (ex-info "http error" {:type :offline - :hint "http error" - :headers {"x-session-id" "secret"}})) - (t/is (= 1 (count @events))) - (let [props (deref (first @events))] - (t/is (= "handled-exception" (::ev/name props))) - (t/is (not (str/includes? (:report props) "Last events:"))) - (t/is (not (str/includes? (:report props) "secret")))) - (done')) - (fn [])))) + (await + (mock/with-mocks* + {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))) + st/format-last-events (mock/stub (fn [& _] (throw (ex-info "must not be called" {}))))} + (errors/flash-persistence (ex-info "http error" {:type :offline + :hint "http error" + :headers {"x-session-id" "secret"}})) + (await (async/settle)) + (let [reports (report-events @events)] + (t/is (= 1 (count reports))) + (let [props (deref (first reports))] + (t/is (= "handled-exception" (::ev/name props))) + (t/is (not (str/includes? (:report props) "Last events:"))) + (t/is (not (str/includes? (:report props) "secret"))))))))) (t/deftest exception-page-reports-dedup-by-cause (let [cause-a (error-cause :type :internal :code :unable-to-process-repository-response :hint "boom") @@ -576,152 +389,136 @@ :organization-id organization-id :team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9"}) -(t/deftest expired-organization-sso-navigates-to-identity-provider - (t/async done - (t/testing "the browser is sent to the identity provider instead of an error page" - (let [events (atom [])] - (mock/with-mocks - {rp/cmd! (mock/stub - (fn [_command _params] - (rx/of {:authorized false - :redirect-uri "https://idp.example.com/authorize"}))) - rt/get-current-href (constantly workspace-href) - st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} - (fn [done'] - (errors/on-error (sso-required-error)) - (t/is (= [::rt/nav-raw] (mapv ptk/type @events))) - (done')) - done))))) +(t/deftest ^:async expired-organization-sso-navigates-to-identity-provider + (t/testing "the browser is sent to the identity provider instead of an error page" + (let [events (atom [])] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized false + :redirect-uri "https://idp.example.com/authorize"}))) + rt/get-current-href (constantly workspace-href) + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} + (errors/on-error (sso-required-error)) + (await (async/settle)) + (t/is (= [::rt/nav-raw] (mapv ptk/type @events)))))))) -(t/deftest expired-organization-sso-comes-back-to-the-current-location - (t/async done - (t/testing "the SSO check asks the provider to return the user where they were" - (let [rpc-calls (atom [])] - (mock/with-mocks - {rp/cmd! (mock/stub - (fn [command params] - (swap! rpc-calls conj {:command command :params params}) - (rx/of {:authorized false - :redirect-uri "https://idp.example.com/authorize"}))) - rt/get-current-href (constantly workspace-href) - st/emit! mock/noop} - (fn [done'] - (errors/on-error (sso-required-error)) - (t/is (= [{:command :check-nitrate-sso - :params {:team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9" - :organization-id organization-id - :url workspace-href}}] - @rpc-calls)) - (done')) - done))))) +(t/deftest ^:async expired-organization-sso-comes-back-to-the-current-location + (t/testing "the SSO check asks the provider to return the user where they were" + (let [rpc-calls (atom [])] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [command params] + (swap! rpc-calls conj {:command command :params params}) + (rx/of {:authorized false + :redirect-uri "https://idp.example.com/authorize"}))) + rt/get-current-href (constantly workspace-href) + st/emit! mock/noop} + (errors/on-error (sso-required-error)) + (await (async/settle)) + (t/is (= [{:command :check-nitrate-sso + :params {:team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9" + :organization-id organization-id + :url workspace-href}}] + @rpc-calls))))))) -(t/deftest already-satisfied-organization-sso-retries-the-location - (t/async done - (t/testing "a session renewed meanwhile (e.g. in another tab) reloads instead of erroring" - (let [events (atom [])] - (mock/with-mocks - {rp/cmd! (mock/stub - (fn [_command _params] - (rx/of {:authorized true :reason :sso-satisfied}))) - rt/get-current-href (constantly workspace-href) - st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} - (fn [done'] - (errors/on-error (sso-required-error)) - (t/is (= [::rt/reload] (mapv ptk/type @events))) - (done')) - done))))) +(t/deftest ^:async already-satisfied-organization-sso-retries-the-location + (t/testing "a session renewed meanwhile (e.g. in another tab) reloads instead of erroring" + (let [events (atom [])] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized true :reason :sso-satisfied}))) + rt/get-current-href (constantly workspace-href) + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} + (errors/on-error (sso-required-error)) + (await (async/settle)) + (t/is (= [::rt/reload] (mapv ptk/type @events)))))))) -(t/deftest organization-sso-without-usable-provider-shows-the-sso-error-dialog - (t/async done - (t/testing "SSO is required but there is nowhere to go: offer a retry, not a permission error" - (let [assigned* (atom nil)] - (mock/with-mocks - {rp/cmd! (mock/stub - (fn [_command _params] - (rx/of {:authorized false :redirect-uri nil}))) - rt/get-current-href (constantly workspace-href) - rt/assign-exception (fn [error] - (reset! assigned* error) - (ptk/data-event ::assigned error))} - (fn [done'] - (errors/on-error (sso-required-error)) - (t/is (= :sso-error (:type @assigned*))) - (t/is (= organization-id (:organization-id @assigned*))) - (t/is (true? (:is-workspace @assigned*))) - (done')) - done))))) +(t/deftest ^:async organization-sso-without-usable-provider-shows-the-sso-error-dialog + (t/testing "SSO is required but there is nowhere to go: offer a retry, not a permission error" + (let [assigned* (atom nil)] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized false :redirect-uri nil}))) + rt/get-current-href (constantly workspace-href) + rt/assign-exception (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))} + (errors/on-error (sso-required-error)) + (await (async/settle)) + (t/is (= :sso-error (:type @assigned*))) + (t/is (= organization-id (:organization-id @assigned*))) + (t/is (true? (:is-workspace @assigned*)))))))) -(t/deftest organization-sso-without-team-access-reports-a-permission-failure - (t/async done - (t/testing "a user who cannot reach the team keeps getting the authentication error" - (let [assigned* (atom nil)] - (mock/with-mocks - {rp/cmd! (mock/stub - (fn [_command _params] - (rx/of {:authorized true :reason :no-team-access}))) - rt/get-current-href (constantly workspace-href) - rt/assign-exception (fn [error] - (reset! assigned* error) - (ptk/data-event ::assigned error))} - (fn [done'] - (errors/on-error (sso-required-error)) - (t/is (= :authentication (:type @assigned*))) - (t/is (= :nitrate-sso-required (:code @assigned*))) - (done')) - done))))) +(t/deftest ^:async organization-sso-without-team-access-reports-a-permission-failure + (t/testing "a user who cannot reach the team keeps getting the authentication error" + (let [assigned* (atom nil)] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized true :reason :no-team-access}))) + rt/get-current-href (constantly workspace-href) + rt/assign-exception (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))} + (errors/on-error (sso-required-error)) + (await (async/settle)) + (t/is (= :authentication (:type @assigned*))) + (t/is (= :nitrate-sso-required (:code @assigned*)))))))) -(t/deftest organization-sso-does-not-retry-on-an-unexplained-authorization - (t/async done - (t/testing "reloading on an answer we don't understand would spin on the same rejection" - (let [events (atom [])] - (mock/with-mocks - {rp/cmd! (mock/stub (fn [_command _params] (rx/of {:authorized true}))) - rt/get-current-href (constantly workspace-href) - rt/assign-exception (fn [error] (ptk/data-event ::assigned error)) - st/async-emit! (fn [& emitted] (swap! events into emitted))} - (fn [done'] - (errors/on-error (sso-required-error)) - (t/is (= [::assigned] (mapv ptk/type @events))) - (done')) - done))))) +(t/deftest ^:async organization-sso-does-not-retry-on-an-unexplained-authorization + (t/testing "reloading on an answer we don't understand would spin on the same rejection" + (let [events (atom [])] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub (fn [_command _params] (rx/of {:authorized true}))) + rt/get-current-href (constantly workspace-href) + rt/assign-exception (fn [error] (ptk/data-event ::assigned error)) + st/async-emit! (fn [& emitted] (swap! events into emitted))} + (errors/on-error (sso-required-error)) + (await (async/settle)) + (t/is (= [::assigned] (mapv ptk/type @events)))))))) -(t/deftest organization-sso-error-without-context-is-reported-as-it-arrives - (t/async done - (t/testing "with no organization and no team there is nothing to check" - (let [rpc-calls (atom 0) - assigned* (atom nil)] - (mock/with-mocks - {rp/cmd! (mock/stub (fn [_command _params] - (swap! rpc-calls inc) - (rx/empty))) - rt/get-current-href (constantly workspace-href) - rt/assign-exception (fn [error] - (reset! assigned* error) - (ptk/data-event ::assigned error))} - (fn [done'] - (errors/on-error {:type :authentication - :code :nitrate-sso-required}) - (t/is (zero? @rpc-calls)) - (t/is (= :nitrate-sso-required (:code @assigned*))) - (done')) - done))))) +(t/deftest ^:async organization-sso-error-without-context-is-reported-as-it-arrives + (t/testing "with no organization and no team there is nothing to check" + (let [rpc-calls (atom 0) + assigned* (atom nil)] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub (fn [_command _params] + (swap! rpc-calls inc) + (rx/empty))) + rt/get-current-href (constantly workspace-href) + rt/assign-exception (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))} + (errors/on-error {:type :authentication + :code :nitrate-sso-required}) + (await (async/settle)) + (t/is (zero? @rpc-calls)) + (t/is (= :nitrate-sso-required (:code @assigned*)))))))) -(t/deftest a-resultless-organization-sso-check-does-not-wedge-later-rejections - (t/async done - (t/testing "the one-in-flight guard is released even when no answer arrives" - (let [rpc-calls (atom 0)] - (mock/with-mocks - {rp/cmd! (mock/stub (fn [_command _params] - (swap! rpc-calls inc) - (rx/empty))) - rt/get-current-href (constantly workspace-href) - st/emit! mock/noop} - (fn [done'] - (errors/on-error (sso-required-error)) - (errors/on-error (sso-required-error)) - (t/is (= 2 @rpc-calls)) - (done')) - done))))) +(t/deftest ^:async a-resultless-organization-sso-check-does-not-wedge-later-rejections + (t/testing "the one-in-flight guard is released even when no answer arrives" + (let [rpc-calls (atom 0)] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub (fn [_command _params] + (swap! rpc-calls inc) + (rx/empty))) + rt/get-current-href (constantly workspace-href) + st/emit! mock/noop} + (errors/on-error (sso-required-error)) + (errors/on-error (sso-required-error)) + (await (async/settle)) + (t/is (= 2 @rpc-calls))))))) ;; A failing check must stay a failing check: the generic handling turns it ;; into a toast, whereas swallowing it would show a permission error for @@ -735,35 +532,31 @@ [error] (swap! check-failures conj error)) -(t/deftest failing-organization-sso-check-is-not-reported-as-missing-access - (t/async done +(t/deftest ^:async failing-organization-sso-check-is-not-reported-as-missing-access + (t/testing "the SSO check fails like a real request (later tick) and the failure stays a failure" (reset! check-failures []) (let [assigned* (atom nil)] - (mock/with-mocks - {rp/cmd! - (mock/stub - (fn [_command _params] - (->> (rx/timer 0) - (rx/mapcat (fn [_] - (rx/throw (ex-info "boom" {:type ::test-check-failure}))))))) + (await + (mock/with-mocks* + {rp/cmd! + (mock/stub + (fn [_command _params] + (->> (rx/timer 0) + (rx/mapcat (fn [_] + (rx/throw (ex-info "boom" {:type ::test-check-failure}))))))) - rt/get-current-href - (constantly workspace-href) + rt/get-current-href + (constantly workspace-href) - rt/assign-exception - (fn [error] - (reset! assigned* error) - (ptk/data-event ::assigned error))} - - (fn [done'] - (errors/on-error (sso-required-error)) - (tm/schedule - 50 - (fn [] - (t/is (= [::test-check-failure] (mapv :type @check-failures))) - (t/is (nil? @assigned*)) - (done')))) - done)))) + rt/assign-exception + (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))} + (errors/on-error (sso-required-error)) + (await (async/wait-for #(= 1 (count @check-failures)) + "sso check failure observed")) + (t/is (= [::test-check-failure] (mapv :type @check-failures))) + (t/is (nil? @assigned*))))))) ;; --------------------------------------------------------------------------- ;; :validation / :invalid-sso-config @@ -825,7 +618,7 @@ (t/is (= timeout (get-in state [:notification :timeout]))) (t/is (= :visible (get-in state [:notification :status])))))))) -(t/deftest persistence-notifications-include-an-error-report-download +(t/deftest ^:async persistence-notifications-include-an-error-report-download (let [scheduled (atom []) idle-callbacks (atom []) events (atom []) @@ -833,68 +626,82 @@ revoked (atom []) report "generated error report" cause (ex-info "Save failed" {:type :validation})] - (with-redefs [dom/prevent-default (fn [_]) - dom/trigger-download-uri (fn [& params] - (swap! downloads conj params)) - errors/generate-report (fn [_] report) - errors/submit-report (fn [& _]) - ;; `tr` is called with one and with two arguments, and its - ;; two-argument arity is variadic: the stub exposes both - ;; shapes so the compiled static calls resolve. - i18n/tr (fn ([key] (str key ":")) - ([key & args] - (str key ":" (first args)))) - st/emit! (mock/stub (fn [& emitted] - (swap! events into emitted))) - tm/schedule (mock/stub (fn [callback] - (swap! scheduled conj callback))) - tm/schedule-on-idle (mock/stub (fn [callback] - (swap! idle-callbacks conj callback))) - wapi/create-blob (mock/stub (fn [content media-type] - {:content content :media-type media-type})) - wapi/create-uri (fn [_] "blob:report") - wapi/revoke-uri (fn [uri] - (swap! revoked conj uri))] - (errors/flash-persistence cause) - (doseq [callback @scheduled] (callback)) - (let [state (ptk/update (first @events) {}) - download (get-in state [:notification :links 0])] - (t/is (= "labels.download:report.txt" (:label download))) - ((:callback download) nil) - (t/is (= [["report" "text/plain" "blob:report"]] @downloads)) - (doseq [callback @idle-callbacks] (callback)) - (t/is (= ["blob:report"] @revoked)))))) + (await + (mock/with-mocks* + {dom/prevent-default (fn [_]) + dom/trigger-download-uri (fn [& params] + (swap! downloads conj params)) + errors/generate-report (fn [_ & _] report) + errors/submit-report (fn [& _]) + ;; `tr` is called with one and with two arguments, and its + ;; two-argument arity is variadic: the stub exposes both + ;; shapes so the compiled static calls resolve. + i18n/tr (fn ([key] (str key ":")) + ([key & args] + (str key ":" (first args)))) + st/emit! (mock/stub (fn [& emitted] + (swap! events into emitted))) + tm/schedule (mock/stub (fn [callback] + (swap! scheduled conj callback))) + tm/schedule-on-idle (mock/stub (fn [callback] + (swap! idle-callbacks conj callback))) + wapi/create-blob (mock/stub (fn [content media-type] + {:content content :media-type media-type})) + wapi/create-uri (fn [_] "blob:report") + wapi/revoke-uri (fn [uri] + (swap! revoked conj uri))} + (errors/flash-persistence cause) + (doseq [callback @scheduled] (callback)) + (await (async/settle)) + ;; The report is emitted first and the toast right after, so the + ;; toast carrying the download link is the last event collected. + (let [state (ptk/update (last @events) {}) + download (get-in state [:notification :links 0])] + (t/is (= "labels.download:report.txt" (:label download))) + ((:callback download) nil) + (t/is (= [["report" "text/plain" "blob:report"]] @downloads)) + (doseq [callback @idle-callbacks] (callback)) + (t/is (= ["blob:report"] @revoked))))))) -(t/deftest persistence-waiters-do-not-report-an-already-handled-failure +(t/deftest ^:async persistence-waiters-do-not-report-an-already-handled-failure (let [reports (atom []) rejected (atom []) pstate (atom {:status :saving}) store (ptk/store {:state {} :on-error errors/on-error}) cause (ex-info "Save failed" {:type :network})] - (with-redefs [refs/persistence pstate - st/emit! (mock/stub (fn [& emitted] (swap! reports into emitted))) - tm/schedule (mock/stub (fn [_]))] - (try - (ptk/emit! store (#'dps/persistence-failed (uuid/next) cause)) - (reset! pstate (:persistence @store)) - (dotimes [_ 2] - (->> (dps/wait-persisted-or-error) - (rx/subs! (fn [_] (t/is false "A failed save must still reject")) - (fn [error] - (swap! rejected conj error) - (errors/on-error error))))) - (t/is (= 2 (count @rejected))) - (t/is (= 1 (count @reports))) - ;; A standalone timeout and a later save failure are new incidents. - ;; The later failure carries a distinct signature: repeating the same - ;; one inside the governor window is coalesced by design. - (errors/on-error (ex-info "Save timed out" {:type :persistence :code :save-timeout})) - (t/is (= 2 (count @reports))) - (ptk/emit! store (#'dps/persistence-failed (uuid/next) - (ex-info "Save failed again" {:type :network}))) - (t/is (= 3 (count @reports))) - (finally - (rx/dispose! store)))))) + (await + (mock/with-mocks* + {refs/persistence pstate + st/emit! (mock/stub (fn [& emitted] + (swap! reports into + (report-events emitted)))) + ;; Run the scheduled `flash` body synchronously: the whole flow + ;; under test is synchronous except for the deferral. + tm/schedule (mock/stub (fn [f] (f)))} + (try + (ptk/emit! store (#'dps/persistence-failed (uuid/next) cause)) + (reset! pstate (:persistence @store)) + (dotimes [_ 2] + (->> (dps/wait-persisted-or-error) + (rx/subs! (fn [_] (t/is false "A failed save must still reject")) + (fn [error] + (swap! rejected conj error) + (errors/on-error error))))) + (await (async/settle)) + (t/is (= 2 (count @rejected))) + (t/is (= 1 (count @reports))) + ;; A standalone timeout and a later save failure are new incidents. + ;; The later failure carries a distinct signature: repeating the same + ;; one inside the governor window is coalesced by design. + (errors/on-error (ex-info "Save timed out" {:type :persistence :code :save-timeout})) + (await (async/settle)) + (t/is (= 2 (count @reports))) + (ptk/emit! store (#'dps/persistence-failed (uuid/next) + (ex-info "Save failed again" {:type :network}))) + (await (async/settle)) + (t/is (= 3 (count @reports))) + (finally + (rx/dispose! store))))))) ;; --------------------------------------------------------------------------- ;; Save failures that belong to their own handler @@ -917,22 +724,20 @@ (t/is (= :authentication (:type @assigned))) (t/is (empty? @scheduled) "An expired session shows no save notification"))))) -(t/deftest expired-organization-sso-during-save-renews-the-session - (t/async done - (t/testing "an SSO-guarded save failure goes back through the identity provider" - (let [events (atom [])] - (mock/with-mocks - {rp/cmd! (mock/stub - (fn [_command _params] - (rx/of {:authorized false - :redirect-uri "https://idp.example.com/authorize"}))) - rt/get-current-href (constantly workspace-href) - st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} - (fn [done'] - (errors/flash-persistence (ex-info "SSO required" (sso-required-error))) - (t/is (= [::rt/nav-raw] (mapv ptk/type @events))) - (done')) - done))))) +(t/deftest ^:async expired-organization-sso-during-save-renews-the-session + (t/testing "an SSO-guarded save failure goes back through the identity provider" + (let [events (atom [])] + (await + (mock/with-mocks* + {rp/cmd! (mock/stub + (fn [_command _params] + (rx/of {:authorized false + :redirect-uri "https://idp.example.com/authorize"}))) + rt/get-current-href (constantly workspace-href) + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted)))} + (errors/flash-persistence (ex-info "SSO required" (sso-required-error))) + (await (async/settle)) + (t/is (= [::rt/nav-raw] (mapv ptk/type @events)))))))) (t/deftest a-deleted-file-during-save-shows-the-exception-page (t/testing "a file that no longer exists shows its page, not the save notification" @@ -959,20 +764,25 @@ (t/is (= [::dw/reload-current-file] (mapv ptk/type @events))) (t/is (empty? @scheduled) "A restored version shows no save notification"))))) -(t/deftest other-validation-failures-during-save-keep-the-notification +(t/deftest ^:async other-validation-failures-during-save-keep-the-notification (t/testing "a validation failure without a recovery of its own is still notified" (let [events (atom []) scheduled (atom [])] - (with-redefs [errors/generate-report (fn [_] "generated error report") - errors/submit-report (fn [& _]) - st/emit! (mock/stub (fn [& emitted] - (swap! events into emitted))) - tm/schedule (mock/stub (fn [callback] - (swap! scheduled conj callback)))] - (errors/flash-persistence (ex-info "Invalid data" {:type :validation - :code :invalid-data})) - (doseq [callback @scheduled] (callback)) - (t/is (= 1 (count @events))) - (let [state (ptk/update (first @events) {})] - (t/is (nil? (get-in state [:notification :timeout]))) - (t/is (some? (get-in state [:notification :links 0])))))))) + (await + (mock/with-mocks* + {errors/generate-report (fn [_ & _] "generated error report") + errors/submit-report (fn [& _]) + st/emit! (mock/stub (fn [& emitted] + (swap! events into emitted))) + tm/schedule (mock/stub (fn [callback] + (swap! scheduled conj callback)))} + (errors/flash-persistence (ex-info "Invalid data" {:type :validation + :code :invalid-data})) + (doseq [callback @scheduled] (callback)) + (await (async/settle)) + ;; The report is emitted first and the toast right after: one + ;; audit event plus the sticky toast carrying the report link. + (t/is (= 1 (count (report-events @events)))) + (let [state (ptk/update (last @events) {})] + (t/is (nil? (get-in state [:notification :timeout]))) + (t/is (some? (get-in state [:notification :links 0]))))))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 51690bbb32..560ec71bce 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -12,6 +12,7 @@ [frontend-tests.data.dashboard-test] [frontend-tests.data.exports-assets-test] [frontend-tests.data.nitrate-test] + [frontend-tests.data.persistence-retry-test] [frontend-tests.data.persistence-test] [frontend-tests.data.profile-test] [frontend-tests.data.repo-test] @@ -31,6 +32,7 @@ [frontend-tests.data.workspace-texts-test] [frontend-tests.data.workspace-thumbnails-test] [frontend-tests.data.workspace-versions-test] + [frontend-tests.errors-governor-test] [frontend-tests.errors-test] [frontend-tests.fonts-test] [frontend-tests.helpers-shapes-test] @@ -130,6 +132,7 @@ 'frontend-tests.data.comments-filters-test 'frontend-tests.data.dashboard-test 'frontend-tests.data.nitrate-test + 'frontend-tests.data.persistence-retry-test 'frontend-tests.data.persistence-test 'frontend-tests.data.profile-test 'frontend-tests.data.repo-test @@ -150,6 +153,7 @@ 'frontend-tests.data.workspace-texts-test 'frontend-tests.data.workspace-thumbnails-test 'frontend-tests.data.workspace-versions-test + 'frontend-tests.errors-governor-test 'frontend-tests.errors-test 'frontend-tests.fonts-test 'frontend-tests.helpers-shapes-test diff --git a/frontend/translations/en.po b/frontend/translations/en.po index ff228ae248..70083f25ea 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -1915,6 +1915,10 @@ msgid "errors.save-failed" msgstr "" "Autosave is not working due to an error. Contact support to report the error and reload to continue from your last saved version." +#: src/app/main/data/persistence.cljs:308 +msgid "errors.save-retrying" +msgstr "Connection lost. Retrying to save your changes." + #: src/app/main/errors.cljs:442 msgid "errors.svg-parser.invalid-svg" msgstr "SVG is invalid or malformed" @@ -7564,6 +7568,10 @@ msgstr "Reset" msgid "workspace.header.save-error" msgstr "Error on saving" +#: src/app/main/ui/workspace/left_header.cljs:125 +msgid "workspace.header.retrying" +msgstr "Retrying to save" + #: src/app/main/ui/workspace/left_header.cljs:123 msgid "workspace.header.saved" msgstr "Saved"