diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index bb4ac77466..996b2c19d2 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -61,6 +61,10 @@ - `app.metrics/run!` is safe by default: a recording failure never throws (a metrics bug must not change the behavior of the measured operation). The first failure per metric id logs at `warn`, later ones at `debug`. The `instance` precondition is a plain assert (the backend enables `:backend-asserts`), and the collector lookup sits outside the recording guard, so a missing instance fails hard even when asserts are disabled. `::mtx/metrics` is required by the storage, s3-backend, and db-pool schemas; `app.db` wires the prometheus `MetricsTrackerFactory` unconditionally. Storage-specific metric contracts: `mem:backend/storage`. +## Log-derived metrics + +- `app.loggers.metrics/reporter` counts the log records the application emits and publishes them as `penpot_log_events_total{logger, level}` (counter), so the error panels do not have to count text in Loki. It is a component that hooks `add-watch` on `app.common.logging/log-record`, like `app.loggers.database`, but with no thread and no channel: the increment is in memory, so it goes straight in the watcher. Only `warn`, `error` and `fatal` are counted, and only the records that go through `app.common.logging` (the application's own logs; a library writing straight to log4j2 is not counted, and today that is one line in a thousand). The body of the watcher is wrapped in a `try/catch` that swallows the error on purpose: the watcher fires from the `swap!` of the log record, inside the logging path. With `::l/sync?` (`app.binfile.v1`, `app.msgbus`) an exception there would reach whoever was logging; on the async path it escapes to the logging executor worker, whose trace goes to the thread's uncaught handler -- outside the application's logging -- and the pool replaces the dead worker, so logging continues and that line is lost. The catch also makes a publication that fails every time invisible: `penpot_log_events_total` stays flat and nothing is written about it. The published `level` values are lowercase (`warn`), unlike the `level` label of the Loki logs (`ERROR`), so a panel that moves from one source to the other has to change it. Cardinality is `logger` (fixed names taken from the code) times `level`; a logger built from a dynamic value would break that. + ## Storage and media - Storage abstraction, backend configuration, logical buckets, object lifecycle, deduplication, access rules, and garbage collection: `mem:backend/storage`. diff --git a/.serena/memories/frontend/routing-app-shell-subtleties.md b/.serena/memories/frontend/routing-app-shell-subtleties.md index f98b7f626d..d033919683 100644 --- a/.serena/memories/frontend/routing-app-shell-subtleties.md +++ b/.serena/memories/frontend/routing-app-shell-subtleties.md @@ -14,14 +14,18 @@ - Frontend error handling treats stale cross-build JS chunk failures specially: messages containing `$cljs$cst$` or `$cljs$core$I` plus undefined/null/not-a-function signatures trigger throttled reload. - Plugin-originated uncaught errors are identified through the plugin runtime hook and logged rather than turning into the global exception page. - `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. +- Errors caused by the environment (`environment-error-types`: `:network`, `:offline`, `:bad-gateway`, `:service-unavailable`, `:gateway-error`, `: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`/`:gateway-error` show the `errors.connection-error` toast. +- `repo/handle-response` classifies responses the backend never wrote (body is not a transit map) by status: 504 and Cloudflare 520–524 → `:gateway-error` (retryable, so GETs and saves retry); 429 → `:rate-limit`; any other ≥400 → `:unexpected-response` (both toast-only via `handle-transport-error`, never the exception page). `:internal :unable-to-process-repository-response` is left for a <400 response that does not decode. Uninterpretable bodies are capped at 500 chars in `:data`. - `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. -- Error messages are bounded at the source: `app.common.exceptions/get-hint` clamps to `max-message-length` (1000 chars) and the CLJS `format-throwable` clamps the message line of the stack trace and honors `:trace-length`. A message can embed a whole payload (a serialized file, for example) and reach megabytes; the bound keeps the audit `:hint` prop and the `:report` string small. The `Context: Hint:` header in `report-context` prints `get-hint` (not the raw `:hint` from `ex-data`) for the same reason. - `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. +- `flash` accepts a `:links` vector of `{:label :callback}` actions, appended after the download link that `:report-link?` produces. `flash-persistence` passes a reload link, so a terminal save failure hands the user both the report and the only action that restarts persistence for the session. `labels.reload-page` plus `js/location.reload` is the established full-restart pattern; the link cannot be invoked under the test runner because `js/location` does not exist there, so it is covered by its label and by being callable. +- `flash-persistence` only delegates to another handler for `:authentication`, `:not-found` and `:code :vern-conflict`. Everything else it reports itself, and the report type is derived from the cause: `#{:validation :server-error :internal}` are application defects and report as `unhandled-exception` so they reach the internal reports and the alerts; the rest stay `handled-exception`. `:cause-type` wins over `:type` when classifying, because `persistence-failed` wraps the original cause under `:type :persistence`. Environment failures are deliberately kept handled: they land here after the retry budget is spent and the user's connection is not our bug. `:assertion` is also kept handled, matching its own handler. +- The report governor is global and stateful across tests: two reports with the same fingerprint inside `report-window-ms` collapse. Call `errors/reset-report-governor!` between assertions that expect an emission. +- Error messages are bounded at the source: `app.common.exceptions/get-hint` clamps to `max-message-length` (1000 chars) and the CLJS `format-throwable` clamps the message line of the stack trace and honors `:trace-length`. A message can embed a whole payload (a serialized file, for example) and reach megabytes; the bound keeps the audit `:hint` prop and the `:report` string small. The `Context: Hint:` header in `generate-report` prints `get-hint` (not the raw `:hint` from `ex-data`) for the same reason. ## Store and websocket For general store mechanics such as `emit!`, `last-events`, persistence, and undo, read `mem:frontend/workspace-state-persistence-subtleties`. - Websocket initialization uses `cf/public-uri` joined with `ws/notifications`, converting `http/https` to `ws/wss`, and includes the current `session-id` as query param. -- Reinitializing or finalizing websocket stops the previous receive stream. Incoming websocket payloads become Potok data events under `app.main.data.websocket/message`. \ No newline at end of file +- Reinitializing or finalizing websocket stops the previous receive stream. Incoming websocket payloads become Potok data events under `app.main.data.websocket/message`. diff --git a/.serena/memories/frontend/workspace-state-persistence-subtleties.md b/.serena/memories/frontend/workspace-state-persistence-subtleties.md index be612121e1..57094e1994 100644 --- a/.serena/memories/frontend/workspace-state-persistence-subtleties.md +++ b/.serena/memories/frontend/workspace-state-persistence-subtleties.md @@ -11,21 +11,27 @@ ## Repo calls - `app.main.repo/send!` uses GET only when the RPC name starts with `get-`, when all params are query params, or for configured special cases. Only GET requests are retried. -- GET retry is limited to transient `:network`, `:bad-gateway`, `:service-unavailable`, and `:offline` errors with exponential backoff. Mutations are not retried. +- `repo/retryable-types` is public and is the single source of truth for the transient transport classification. `persistence/transient-error?` and `errors/environment-error-types` both derive from it; extend that set, never copy it. +- GET retry is limited to transient `:network`, `:bad-gateway`, `:service-unavailable`, `:gateway-error` (504, Cloudflare 52x), and `:offline` errors with exponential backoff. Mutations are not retried. - A server SSE response is only accepted when the command is configured `:stream?`; otherwise it raises an unexpected-response assertion. ## Commits, undo, persistence -- `commit-changes` refuses to create commits unless `:permissions :can-edit` is true. It captures file revn/vern, selected-before, features, tags, undo group, and translation flag into a `::commit` event. +- `commit-changes` refuses to create commits unless `:permissions :can-edit` is true. It captures file revn/vern, selected-before, features, tags, undo group, and translation flag into a `::commit` event. A refusal answers with an empty stream, never nil, so callers subscribe uniformly instead of branching on nil. - Applying a remote commit first rolls back pending local commits, applies the remote changes, then replays pending local redo changes. Index updates are emitted for undo, remote redo, and replayed redo paths. - Local commits are independently consumed by undo, persistence, WASM model updates, thumbnail/library watchers, and text position-data recalculation. - 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`. +- A terminal failure halts the queue on purpose: the edits stay queued so `wait-persisted-or-error` keeps rejecting, and `workspace/versions.cljs` relies on that to avoid snapshotting a file whose edits were never saved. `recover-persistence` lifts the halt when a file is opened again or the role changes. A transport failure still inside its retry window (`slow-retry-pending?`) is resent once (`resume-persistence true`), never released; any other halt emits `release-failed-file` (drops every queued commit of the file that failed) then `resume-persistence`. Opening a file also replays the queued edits of its version onto the loaded data (`pending-file-changes` → `restore-pending-changes` in `bundle-fetched`); a replay that throws keeps the server data and the queue. That is what unblocks the editor without a reload; because the queue is shared, dropping the failed file is also what lets other files' pending edits drain. `release-failed-file` runs at recovery time, never at failure time — clearing the error eagerly would make waiters resolve. +- The split between `initialize-persistence-state` and `recover-persistence` is deliberate: initialization deliberately does NOT lift a halt, so undeliverable edits are released when the user acts, not when the view mounts. +- **Every non-transport terminal save failure is permanent.** There is no "stale but valid" category: permission lost, file gone, and server-rejected changes all fail again on a resend. Losing edit permission is not temporary — the backend raises `:not-found` for it (`check-edition-permissions!` → `perms/make-check-fn`), and `:not-found` routes to the exception page. So `:save-permission-denied` (a client-side `can-edit` check) must never be resumed: replaying the save to discover that routes the user to the exception page. Release it instead. +- Save failures split transient vs terminal (`transient-error?`: the repo retryable types `:network`/`:offline`/`:bad-gateway`/`:service-unavailable`/`:gateway-error` plus `:invalid-save-response`; everything else is terminal). Terminal keeps the `:error` halt + `flash-persistence` path; transient enters a `:retrying` episode: the head commit stays queued and resends with backoff 2s/8s/20s (3 retries, then the `:error` path). A transient failure that outlasts the backoff arms `slow-retry-cycle`: one silent resend (`:recovering`) every `slow-retry-delay-ms` (30s) until a save lands, `retry-give-up-ms` (24h since `:failing-since`) passes, or the head commit collects `max-slow-failures` (3) attempts that failed after `slow-failure-ms` (90s) or more (`:slow-failures`, counted in `persistence-transient-failure` from the attempt's elapsed time): a gateway timeout may leave the backend applying the save, so a save that keeps failing slowly must not be replayed for a day, while fast failures (offline) never count; the cycle stops on `::resume-persistence`, `::initialize-persistence` and `finalize-workspace`. Leaving the workspace also emits `suspend-retries` (`:suspended` in `:persistence`, drops `:retry-token` so a pending backoff timer goes stale): no backoff, slow cycle or `online` resend runs outside the workspace; only `workspace-initialized` (not a role change) lifts it via `recover-persistence` (`opened-file?`), which also resends a `:retrying` episode whose backoff was cut short. Its toast is `errors.save-still-retrying` without a reload link; a landing save hides the failure toast (`errors/persistence-failed-tag`). Replaying a save is safe because changes are idempotent, so there is no unknown-outcome guard: a commit whose earlier request left `active-requests` is simply sent again under the same `:commit-id`; only a request still in flight blocks a resend (`:in-flight` stays silent). Tests hold the slow cycle by stubbing `rx/timer` to never fire for `slow-retry-delay-ms`, or instant timers loop forever. Retry timers carry the episode token; a superseded token stays silent. Status stays `:retrying` through re-entries (`next-status` refuses `:pending`/`:saving` from it); waiters (`wait-persisted-or-error`) wait through it and reject only on `:error`. - One reconnect notice per episode: sticky toast tagged `:persistence-reconnecting` (single-toast store, re-show replaces), hidden by tag on drain (`:saved`) and on terminal failure; recovery is silent. Header indicator has a `:retrying` state (`workspace.header.retrying`). - Resume triggers: backoff timer and the browser `online` event (guarded by `exists? js/window`; re-enters the runner only for a live `:retrying` episode). New local edits during `:retrying` only join the queue: the episode keeps its `:run-id`, so `append-commit` does not re-enter, and the live runner (still waiting on the head's `commit-persisted`) sends them after the head. Re-entering on edits would bypass the backoff and spend an attempt per batched commit. - Tests instant-trigger retries by stubbing `rx/timer` (recording delays to assert the schedule); dynamic bindings do not survive `await` continuations, so no dynamic var for delays. +- `save-request-timeout-ms` is 7 minutes and wraps the `update-file` request in `rx/timeout`, failing it as a transient `:network` error so the retry path resends instead of blocking the queue forever. It is deliberately longer than `saving-stall-timeout-ms` (5 min) so the stalled report fires before the retry. +- `sync-unload-guard` keeps `:persistence` in `st/ongoing-tasks` while edits are buffered or queued, which is what holds the browser leave-page warning on. It syncs on `::append-commit`, `::discard-commit` and `::release-failed-file` (every queue change). It is the one effect that must stay in `store.cljs`'s `omitset`, and `store.cljs`'s `ongoing-tasks` watcher is guarded by `exists? js/window` because tests run without a window. - 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/.serena/memories/prod-infra/grafana.md b/.serena/memories/prod-infra/grafana.md index fbc40ec6e5..10af277db2 100644 --- a/.serena/memories/prod-infra/grafana.md +++ b/.serena/memories/prod-infra/grafana.md @@ -6,15 +6,15 @@ Keep this memory free of internal names: no host names, no deployed versions, no ## Datasources -- Metrics — **Prometheus**, uid `xkRmyFBGk`. PromQL. Metric families: `penpot_*` (RPC, task timings, CLIMIT, HTTP, websocket, storage), `hikaricp_*`, `jvm_*` and `process_*` on the backend (`job="app"`); `pg_*` from the Postgres exporter (`job="postgres"`); `redis_*` from the Redis exporter (`job="redis"`); `node_*` and `container_*` from node-exporter and cAdvisor. Backend histograms are in **milliseconds**. -- Logs — **Loki**, uid `h3CGKhEMk`. LogQL, and filter by the structured metadata `logger`/`level` (`{job="penpot"} | logger="..." | level="ERROR"`). Signals that only exist as logs: the profile behind a CLIMIT queue (`logger="app.rpc.climit"`, whose `id` carries the key). CLIMIT rejections have their own counters (`penpot_rpc_climit_rejected_total`, by `name` and `reason`), and their `hint="rejected"` line goes out at WARN only when something stays queued, so counting the lines is not counting the rejections. +- Metrics — **Prometheus**, uid `xkRmyFBGk`. PromQL. Metric families: `penpot_*` (RPC, task timings, CLIMIT, HTTP, websocket, storage, and the application log events of the backend), `hikaricp_*`, `jvm_*` and `process_*` on the backend (`job="app"`); `pg_*` from the Postgres exporter (`job="postgres"`); `redis_*` from the Redis exporter (`job="redis"`); `node_*` and `container_*` from node-exporter and cAdvisor. Backend histograms are in **milliseconds**. +- Logs — **Loki**, uid `h3CGKhEMk`. LogQL, and filter by the structured metadata `logger`/`level` (`{job="penpot"} | logger="..." | level="ERROR"`). Two things to remember when counting from it: the CLIMIT rejections have their own counters (`penpot_rpc_climit_rejected_total`, by `name` and `reason`) and their `hint="rejected"` line goes out at WARN only when something is left queued behind the call (a lone caller that times out is written at TRACE), so counting lines is not counting rejections -- the same goes for `hint="enqueued"` and `hint="acquired"`; and the only signal that exists nowhere else is the profile behind a queue (`logger="app.rpc.climit"`, whose `id` carries the key), chased there on demand. - SQL — **Postgres**, uid `y8ea9tfMk`. Panels need `rawQuery: true`, `editorMode: "code"` and `format: "table"`. The `task` table is the only truth about the queue depth. It is a **read-only replica**: it rejects any write, `CREATE TEMP TABLE` included, so queries through it must be plain `SELECT`s. It also cuts long-running queries (the MCP path has a short timeout), so bound them (e.g. a list of ids); an empty answer there is not proof that there is no data. - **Postgres (telemetry)**, uid `9YAUuM_Gz`. - `$__rate_interval` and `$__interval` are substituted by Grafana; use them instead of fixed windows in graphs. ## Main dashboard -- `Production status`, uid `penpot-production-status`. Sections: general state and deploy, RPC and CLIMIT, PostgreSQL and pool, Redis/msgbus, tasks, JVM/process. Variables `$env` and `$host` (custom), no auto refresh. +- `Production status`, uid `penpot-production-status`. Sections: general state and deploy, `RPC y CLIMIT`, PostgreSQL and pool, Redis/msgbus, tasks, JVM/process. Variables `$env` and `$host` (custom), no auto refresh. Every section opens collapsed except `RPC y CLIMIT`, which is the one read on a normal day. - Per-host investigation dashboards are separate, and the alert rules deeplink into them. ## Conventions @@ -26,6 +26,20 @@ Keep this memory free of internal names: no host names, no deployed versions, no - Cron "did it run" comes from the task metrics, not from the cron registry table (it keeps rows from old versions). - Dashboard thresholds must match the alert rule thresholds. +## Editing dashboards through the API + +- Saving through the API normalizes the dashboard: submit the native form, rows with `panels: []` and their panels top-level with contiguous `y`. A row that has to start collapsed (or open) is sent holding its children in `panels` with `collapsed` set, and the top-level `y` are recomputed by hand: one unit per collapsed row, with the expanded block kept contiguous. Grafana accepts it and returns the same shape. +- A `remove` operation on a panel leaves its `gridPos` hole behind: Grafana does not refill the layout (the UI behaves the same), so a neighbouring panel has to be moved by hand when the gap matters. +- Check `list_dashboard_versions` before writing and prefer `operations` patches: the user edits dashboards by hand and a full-JSON overwrite silently reverts that work. + +## Reading the API by hand + +- The MCP authenticates with a service account token (`GRAFANA_SERVICE_ACCOUNT_TOKEN`, set in the environment of the MCP server), and a service account has no user behind it: a 403 is never the token being bad. On `/api/user` and `/api/user/orgs` it is normal; on a single dashboard (`Access denied to this dashboard`, which is not a 404) it means that identity cannot see it, a permission on that dashboard or its folder. Restarting the MCP or rotating the token changes nothing, and `X-Grafana-Org-Id` does not help when the identity belongs to another organization. +- Listing is reliable, filtering is not: a dashboard search with a text query answers empty in this instance (through the tool and through `/api/search?query=...` alike), and `list_prometheus_metric_names` also lists names that survive only in the retained series of a metric that stopped being published. Use the listing without a query, and `count()` to tell whether a metric is alive today. +- The MCP Prometheus tool can answer empty for every query, `up` included, while the datasource itself is fine: run PromQL by hand with `grafana_api_request` against `/api/datasources/uid//resources/api/v1/query?query=...`. +- `curl` and `python` from the shell cannot reach the Grafana host of the deployment: Cloudflare blocks them (`error code: 1010`, which looks like a 403 and has nothing to do with Grafana). Probe the API through the MCP (`grafana_api_request`), which is what the deployment lets through. +- A Loki query by hand goes through `GET /api/datasources/proxy/uid//loki/api/v1/query_range?query=&start=&end=&step=` (the range comes back as `resultType: matrix`, one series per group). For Loki the `/api/datasources/uid//resources/...` route answers 500, and `/api/ds/query` is not reachable from `grafana_api_request` at all (404), even though that is the path the dashboards use. + ## Gotchas - PromQL literal dots need `\\.`; simpler to use `[.]` or a plain dot. @@ -34,9 +48,5 @@ Keep this memory free of internal names: no host names, no deployed versions, no - Prometheus: `label_replace(, "host", "$1", "instance", "([^.]+)[.][^.]+.*")` plus `legendFormat: "... {{host}}"`. - Loki metric query: `| label_format host="{{ regexReplaceAll "[.][^.]*[.][^.]*[.][^.]*$" .instance "" }}"` plus `legendFormat: "{{host}}"`. Quotes inside the template must be escaped as `\"`, or LogQL closes the string early (`not a valid duration string`). - Tables from a Prometheus raw selector carry a `__name__` column; hide it with `organize`. -- Saving through the API normalizes the dashboard (rows may be flattened, `y` recomputed): submit the native form (rows with `panels: []`, panels top-level, contiguous `y`). - Panel images cannot be rendered (no image renderer installed) and there is no browser here: verify a dashboard by running its queries with the variables substituted. - Signals that exist only as logs are counted with Loki, and the count is then cross-checked against the nginx access log for the same window and the same status code (`job="frontend"`, e.g. a `429`): two independent sources must agree before a number is trusted. -- The app writes its own logs at WARN only when something is left queued behind the call and at TRACE otherwise, so production Loki holds just the under-pressure lines: a count of `hint="enqueued"` or `hint="acquired"` is not the number of calls, and neither is a count of `hint="rejected"` lines, because a rejection that leaves the queue empty (a lone caller that times out) is written at TRACE. The rejection counters are the real number. -- The MCP Prometheus tool can answer empty for every query, `up` included, while the datasource itself is fine: an empty result from it is not proof that a panel has no data, and blaming the datasource for it is wrong. Run PromQL by hand with `grafana_api_request` against `/api/datasources/uid//resources/api/v1/query?query=...` (or `POST /api/ds/query`, the path dashboards use), and check when a series is expected but the tool says nothing. -- **Check `list_dashboard_versions` before writing.** The user edits dashboards by hand and a full-JSON overwrite silently reverts that work; prefer `operations` patches per panel. diff --git a/backend/src/app/loggers/metrics.clj b/backend/src/app/loggers/metrics.clj new file mode 100644 index 0000000000..07c97debd1 --- /dev/null +++ b/backend/src/app/loggers/metrics.clj @@ -0,0 +1,46 @@ +;; 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 INC Sucursal en España SL + +(ns app.loggers.metrics + "A reporter that counts the log records of this application, by logger and + level, and publishes them as a counter." + (:require + [app.common.logging :as l] + [app.metrics :as mtx] + [integrant.core :as ig])) + +(def ^:private counted-levels + "The levels worth counting: `info` is emitted often and says nothing about + the health of the application." + #{:warn :error :fatal}) + +(defmethod ig/assert-key ::reporter + [_ params] + (assert (mtx/metrics? (::mtx/metrics params)) "expect valid metrics instance")) + +(defmethod ig/init-key ::reporter + [_ {::mtx/keys [metrics]}] + (add-watch l/log-record ::reporter + (fn [_ _ _ record] + ;; This runs inside the logging path: the watcher fires from + ;; the `swap!` of the log record, so an exception here would + ;; reach whoever was logging on the synchronous path, and on + ;; the async one it would die in the executor's uncaught + ;; handler, taking that log line out of Loki. Swallowed on + ;; purpose either way. + (try + (let [level (::l/level record)] + (when (contains? counted-levels level) + (mtx/run! metrics + :id :log-events + :inc 1 + :labels (into-array String [(::l/logger record) (name level)])))) + (catch Throwable _ nil)))) + {::mtx/metrics metrics}) + +(defmethod ig/halt-key! ::reporter + [_ _] + (remove-watch l/log-record ::reporter)) diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index c8898f90e6..9fc93eb26e 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -52,7 +52,13 @@ (:gen-class)) (def default-metrics - {:update-file-changes + {:log-events + {::mdef/name "penpot_log_events_total" + ::mdef/help "Log records at WARN, ERROR and FATAL emitted by the application, by logger and level." + ::mdef/labels ["logger" "level"] + ::mdef/type :counter} + + :update-file-changes {::mdef/name "penpot_rpc_update_file_changes_total" ::mdef/help "A total number of changes submitted to update-file." ::mdef/type :counter} @@ -590,6 +596,9 @@ :app.loggers.database/reporter {::db/pool (ig/ref ::db/pool)} + :app.loggers.metrics/reporter + {::mtx/metrics (ig/ref ::mtx/metrics)} + ::sto/storage {::db/pool (ig/ref ::db/pool) ::mtx/metrics (ig/ref ::mtx/metrics) diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 0335369709..4882827b65 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -133,6 +133,7 @@ :app.notifications/handler :app.loggers.mattermost/reporter :app.loggers.database/reporter + :app.loggers.metrics/reporter :app.worker/cron :app.worker/dispatcher [:app.main/default :app.worker/runner] diff --git a/backend/test/backend_tests/loggers_metrics_test.clj b/backend/test/backend_tests/loggers_metrics_test.clj new file mode 100644 index 0000000000..984ad3b0f8 --- /dev/null +++ b/backend/test/backend_tests/loggers_metrics_test.clj @@ -0,0 +1,135 @@ +;; 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 INC Sucursal en España SL + +(ns backend-tests.loggers-metrics-test + "The reporter that turns log records into a counter." + (:require + [app.common.logging :as l] + [app.loggers.metrics] ;; registers the integrant methods of the reporter key + [app.main :as main] + [app.metrics :as mtx] + [clojure.test :as t] + [integrant.core :as ig]) + (:import + io.prometheus.client.CollectorRegistry)) + +(def ^:private logger + "The logger of a record is the namespace of the code that logs, and that is + this namespace." + (str *ns*)) + +(defn- init-metrics + [] + (ig/init-key :app.metrics/metrics {:default main/default-metrics})) + +(defn- init-reporter + [metrics] + (ig/init-key :app.loggers.metrics/reporter {::mtx/metrics metrics})) + +(defn- halt-reporter! + [reporter] + (ig/halt-key! :app.loggers.metrics/reporter reporter)) + +(defn- sample + "Value of the counter for one logger and level, read from the registry. Nil + when that combination has never been published." + [metrics logger level] + (.getSampleValue ^CollectorRegistry (mtx/get-registry metrics) + "penpot_log_events_total" + (into-array String ["logger" "level"]) + (into-array String [logger level]))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; COUNTING +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest warn-and-error-are-counted-by-logger-and-level + (let [metrics (init-metrics) + reporter (init-reporter metrics)] + (try + ;; `:sync?` so the record is emitted on this thread and the assertion + ;; does not depend on the logging executor. + (l/warn :hint "una prueba" ::l/sync? true) + (l/error :hint "otra prueba" ::l/sync? true) + + (t/is (= 1.0 (sample metrics logger "warn"))) + (t/is (= 1.0 (sample metrics logger "error"))) + + (l/error :hint "y otra mas" ::l/sync? true) + (t/is (= 2.0 (sample metrics logger "error"))) + + (finally + (halt-reporter! reporter))))) + +(t/deftest info-is-not-counted + (let [metrics (init-metrics) + reporter (init-reporter metrics)] + (try + (l/info :hint "un mensaje normal" ::l/sync? true) + (t/is (nil? (sample metrics logger "info"))) + + (finally + (halt-reporter! reporter))))) + +(t/deftest fatal-is-counted + (let [metrics (init-metrics) + reporter (init-reporter metrics)] + (try + ;; No macro of the logging emits `:fatal`; this is the only way a record + ;; gets that level. + (l/log :fatal :hint "algo muy malo" ::l/sync? true) + (t/is (= 1.0 (sample metrics logger "fatal"))) + + (finally + (halt-reporter! reporter))))) + +(t/deftest halting-the-reporter-stops-counting + (let [metrics (init-metrics) + reporter (init-reporter metrics)] + + (halt-reporter! reporter) + + (l/error :hint "despues de parar" ::l/sync? true) + (t/is (nil? (sample metrics logger "error"))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; SAFETY +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest a-failing-publication-does-not-break-the-logging + ;; The watcher fires from the `swap!` of the log record, inside the logging + ;; path. With `::l/sync?` an exception here would reach whoever was logging; + ;; on the async path it would end in the executor's uncaught handler, which + ;; is outside the application's logging, taking that log line out of Loki and + ;; out of this counter. + (let [metrics (init-metrics) + reporter (init-reporter metrics)] + (try + (with-redefs [mtx/run! (fn [& _] (throw (ex-info "publicando mal" {})))] + (t/is (nil? (l/error :hint "no debe propagar" ::l/sync? true)))) + + (finally + (halt-reporter! reporter))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; DEFINITION +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest the-log-events-metric-is-defined + (let [metrics (init-metrics)] + (t/is (= ["logger" "level"] + (:app.metrics.definition/labels (mtx/get-collector metrics :log-events)))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; WIRING +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(t/deftest the-system-declares-the-reporter + ;; The reporter hooks itself to the logging, so declaring it in the system is + ;; all the wiring there is. The tests of the suite remove it, so this is the + ;; only place that pins it. + (t/is (= {::mtx/metrics (ig/ref ::mtx/metrics)} + (:app.loggers.metrics/reporter main/system-config)))) diff --git a/frontend/playwright/ui/specs/text-editor-v3.spec.js b/frontend/playwright/ui/specs/text-editor-v3.spec.js index 04efe9f73c..fa49a0704a 100644 --- a/frontend/playwright/ui/specs/text-editor-v3.spec.js +++ b/frontend/playwright/ui/specs/text-editor-v3.spec.js @@ -408,10 +408,13 @@ test.describe("BUG 10934 - Double-clicking a text side handle sets auto-size", ( await workspace.goToWorkspace(); await workspace.waitForFirstRender(); - // Select the text and zoom to fit, so it is fully visible in the viewport + // Select the text and zoom to fit, so it is fully visible in the viewport. + // Wait for the zoom label to change so the handles are at their final spot. await workspace.clickLeafLayer("Fixed text"); + const zoomLabel = page.getByTitle("Zoom", { exact: true }); + const initialZoom = await zoomLabel.innerText(); await page.keyboard.press("Shift+1"); - await workspace.waitForIdle(); + await expect(zoomLabel).not.toHaveText(initialZoom); return workspace; } diff --git a/frontend/src/app/main/data/persistence.cljs b/frontend/src/app/main/data/persistence.cljs index 6dd69e5c7c..99e708d5bd 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -27,6 +27,8 @@ (declare ^:private run-persistence-task) (declare ^:private persist-commit) +(declare ^:private resume-persistence) +(declare ^:private slow-retry-cycle) (log/set-level! :warn) @@ -36,6 +38,25 @@ (def force-persist? #(= % ::force-persist)) +(def slow-retry-delay-ms + "Pause between the attempts of a queue whose backoff is spent: one attempt + per cycle, until it saves or the retry window closes." + 30000) + +(def retry-give-up-ms + "How long a queue that keeps failing on transport errors is sent again." + (* 24 60 60 1000)) + +(def slow-failure-ms + "An attempt that fails after this long may have kept the backend busy, + as a gateway timeout does, so it counts towards `max-slow-failures`." + (* 90 1000)) + +(def max-slow-failures + "Slow failures a queued commit may collect before the slow cycle stops + sending it. Fast failures, such as being offline, never count." + 3) + (def ^:private saving-stall-timeout-ms (* 5 60 1000)) (def ^:private saving-check-interval-ms 30000) (def ^:private save-wait-timeout-ms (* 2 60 1000)) @@ -142,6 +163,23 @@ save or on terminal failure." :persistence-reconnecting) +(defn- retry-window-open? + "True while a failing queue may still be sent again." + [pstate] + (let [since (:failing-since pstate)] + (or (nil? since) + (< (- (inst-ms (ct/now)) since) retry-give-up-ms)))) + +(defn- slow-retry-pending? + "True when the queue is halted by a transport failure that the slow retry + cycle keeps sending. A commit whose attempts keep failing slowly is not + sent again: each attempt may cost the backend the whole save." + [{:keys [status error slow-failures] :as pstate}] + (and (= :error status) + (transient-error? error) + (retry-window-open? pstate) + (< (or slow-failures 0) max-slow-failures))) + (defn- report-stalled-persistence [now] (ptk/reify ::report-stalled-persistence @@ -212,8 +250,9 @@ (throw (ex-info "invalid state" {}))))) (update :index dissoc commit-id) (assoc :last-progress-at (inst-ms (ct/now))) - (dissoc :stall-reported - :attempts :retry-token :retry-for))))))) + (dissoc :stall-reported :failing-since :recovering + :slow-failures :attempts :retry-token + :retry-for))))))) (defn- append-commit "Event used internally to append the current change to the @@ -243,6 +282,23 @@ (rx/of (update-status :saving) (run-persistence-task)))))))) +(defn- drop-file-commits + "Removes every queued commit belonging to `file-id`, together with its + index entry." + [pstate file-id] + (if-not file-id + pstate + (let [dropped (into #{} + (keep (fn [commit-id] + (when (= file-id (get-in pstate [:index commit-id :file-id])) + commit-id))) + (:queue pstate))] + (if (empty? dropped) + pstate + (-> pstate + (update :queue #(into (empty %) (remove dropped) %)) + (update :index #(apply dissoc % dropped))))))) + (defn- persistence-failed [commit-id cause] (ptk/reify ::persistence-failed @@ -260,28 +316,45 @@ :commit-id commit-id :hint (ex-message cause) ::errors/handled? true)) + (update :failing-since d/nilv (inst-ms (ct/now))) (dissoc :run-id :last-progress-at :stall-reported :attempts :retry-token :retry-for)))))) ptk/WatchEvent - (watch [_ _ _] - ;; The terminal toast supersedes the reconnect notice; hide it - ;; explicitly instead of relying on the single-toast replacement. - (rx/of (ptk/data-event ::error cause) - (ntf/hide :tag reconnecting-tag))) + (watch [_ state stream] + (rx/merge + ;; The terminal toast supersedes the reconnect notice; hide it + ;; explicitly instead of relying on the single-toast replacement. + (rx/of (ptk/data-event ::error cause) + (ntf/hide :tag reconnecting-tag)) + ;; A transport failure that outlasts the backoff can still pass, so + ;; the queue keeps trying at a slow pace. + (let [pstate (:persistence state)] + (if (and (not (:suspended pstate)) + (slow-retry-pending? pstate)) + (slow-retry-cycle stream) + (rx/empty))))) ptk/EffectEvent - (effect [_ _ _] - ;; Report without invoking global handlers that may reload the file or - ;; navigate away before the user can recover the retained changes. - (errors/flash-persistence cause)))) + (effect [_ state _] + ;; A failed slow attempt repeats a failure the user was already warned + ;; about. Report without invoking global handlers that may reload the + ;; file or navigate away before the user can recover the retained + ;; changes. + (let [pstate (:persistence state)] + (when-not (and (:recovering pstate) + (slow-retry-pending? pstate)) + (errors/flash-persistence cause :retrying (slow-retry-pending? pstate))))))) (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] + budget (`retry-delays-ms`) is exhausted, or when the failed attempt was + one of the slow cycle, the failure falls through to the `persistence-failed` + path, which schedules the next slow attempt. While retries are suspended + nothing is scheduled: the episode waits for a file to be opened again. + `elapsed-ms` is how long the failed attempt took." + [commit-id cause elapsed-ms] (ptk/reify ::persistence-transient-failure ptk/UpdateEvent (update [_ state] @@ -296,13 +369,22 @@ :retry-token (uuid/next) :retry-for commit-id :last-progress-at (inst-ms (ct/now))) + (update :failing-since d/nilv (inst-ms (ct/now))) + (cond-> (>= elapsed-ms slow-failure-ms) + (update :slow-failures (fnil inc 0))) (dissoc :stall-reported)))))) ptk/WatchEvent (watch [_ state _] - (let [attempts (dm/get-in state [:persistence :attempts])] - (if (> attempts (count retry-delays-ms)) + (let [{:keys [attempts recovering suspended]} (:persistence state)] + (cond + (or recovering (> attempts (count retry-delays-ms))) (rx/of (persistence-failed commit-id cause)) + + suspended + (rx/empty) + + :else (rx/merge ;; One notice per episode: shown on the first attempt, ;; re-showing would only replace the identical toast. @@ -316,19 +398,6 @@ (->> (rx/timer delay-ms) (rx/map (fn [_] (persist-commit commit-id {:token token}))))))))))) -(defn- rotate-stalled-stamp - "Clears a previous attempt stamp when it is safe to resend: the commit is - the current retry head and its request is no longer in flight. A still - in-flight request is left alone — its own result drives the next step." - [state commit-id] - (let [commit (dm/get-in state [:persistence :index commit-id])] - (if (and (= :retrying (dm/get-in state [:persistence :status])) - (= commit-id (dm/get-in state [:persistence :retry-for])) - (::request-id commit) - (not (contains? @active-requests (::request-id commit)))) - (update-in state [:persistence :index commit-id] dissoc ::request-id) - state))) - (defn- commit-persisted [commit] (ptk/reify ::commit-persisted @@ -360,30 +429,29 @@ "Classifies what should happen with a queued commit before sending it. The attempt stamp and the send decision both read this, so a commit is only ever stamped with a request that is actually going to be sent." - [state commit-id request-id] + [state commit-id] (let [commit (dm/get-in state [:persistence :index commit-id])] (cond (= :error (dm/get-in state [:persistence :status])) :halted (nil? commit) :missing-commit (::acknowledged commit) :acknowledged (contains? @active-requests (::request-id commit)) :in-flight - (and (::request-id commit) - (not= request-id (::request-id commit))) :unknown-outcome (not (dm/get-in state [:permissions :can-edit])) :permission-denied :else :ready))) (defn- send-queued-commit "Sends one queued commit and maps its outcome to persistence events." [request-id session-id {:keys [id file-id file-revn file-vern changes features] :as commit}] - (let [params {:id file-id - :revn (max file-revn (get @revn-data file-id 0)) - :vern file-vern - :session-id session-id - :origin (:origin commit) - :created-at (:created-at commit) - :commit-id id - :changes (vec changes) - :features features}] + (let [sent-at (inst-ms (ct/now)) + params {:id file-id + :revn (max file-revn (get @revn-data file-id 0)) + :vern file-vern + :session-id session-id + :origin (:origin commit) + :created-at (:created-at commit) + :commit-id id + :changes (vec changes) + :features features}] ;; UI read-only mode does not invalidate already queued edits. (->> (update-file-request request-id params) ;; Fails an unanswered request as a transient network error, so the @@ -406,30 +474,29 @@ :code :invalid-save-response :file-id file-id}))))) (rx/catch (fn [cause] - (rx/of ((if (transient-error? (ex-data cause)) - persistence-transient-failure - persistence-failed) - id cause))))))) + (rx/of (if (transient-error? (ex-data cause)) + (persistence-transient-failure + id cause (- (inst-ms (ct/now)) sent-at)) + (persistence-failed id cause)))))))) (defn- persist-commit - "Sends the queued commit, stamping the attempt first. Inside a `:retrying` - episode a previous stamp is rotated when its request is no longer in - flight, so a retry resends the same commit instead of failing as - `:unknown-outcome`. Carries an optional `:token`: retry timers pass the - episode token, and a stale token (superseded episode) stays silent - instead of sending or failing." + "Sends the queued commit, stamping the attempt first. A commit whose + earlier request is no longer in flight is sent again: changes are + idempotent, so a save the backend already applied can be applied again. + Carries an optional `:token`: retry timers pass the episode token, and a + stale token (superseded episode) stays silent instead of sending or + failing." [commit-id & {:keys [token]}] (let [request-id (uuid/next)] (ptk/reify ::persist-commit ptk/UpdateEvent (update [_ state] (let [token-ok? (or (nil? token) - (= token (dm/get-in state [:persistence :retry-token]))) - state (if token-ok? (rotate-stalled-stamp state commit-id) state)] + (= token (dm/get-in state [:persistence :retry-token])))] (if (and token-ok? - (= :ready (attempt-state state commit-id request-id))) - ;; Record the attempt before starting I/O. An interrupted request - ;; may have reached the server and must not be replayed blindly. + (= :ready (attempt-state state commit-id))) + ;; Stamp the attempt before any I/O, so a request in flight is + ;; never sent twice in parallel. (assoc-in state [:persistence :index commit-id ::request-id] request-id) state))) @@ -446,16 +513,12 @@ (not= token (dm/get-in state [:persistence :retry-token]))) ;; Stale retry timer: its episode was superseded. Stay silent. (rx/empty) - (case (attempt-state state commit-id request-id) + (case (attempt-state state commit-id) :halted (rx/empty) :missing-commit (fail :missing-commit "A queued save has no change data") :acknowledged (rx/of (commit-persisted commit)) ;; The replacement runner listens for the original request's result. :in-flight (rx/empty) - ;; Fail-safe for anything outside a live retry episode (retry - ;; rotation in `update` already cleared the stamp when resending - ;; is safe). Keep the attempt stamp to prevent replay. - :unknown-outcome (fail :save-outcome-unknown "An interrupted save has an unknown outcome") :permission-denied (fail :save-permission-denied "Edit permission was lost before changes could be saved") :ready (send-queued-commit request-id (:session-id state) commit)))))))) @@ -484,7 +547,10 @@ (rx/filter #(= commit-id (:id %))) (rx/take 1) (rx/mapcat (fn [_] + ;; A save that lands shows the connection + ;; is back: the failure warning goes away. (rx/of (discard-commit commit-id) + (ntf/hide :tag errors/persistence-failed-tag) (run-persistence-task))))) (rx/of (persist-commit commit-id))) (rx/take-until stoper-s))) @@ -494,43 +560,186 @@ (ntf/hide :tag reconnecting-tag))))))) (defn- resume-persistence + "Starts the queue again. A `recovering` resume is an attempt of the slow + cycle, which sends once instead of starting a backoff episode." + ([] (resume-persistence false)) + ([recovering] + (ptk/reify ::resume-persistence + ptk/UpdateEvent + (update [_ state] + (update state :persistence + (fn [pstate] + (-> pstate + (dissoc :error :attempts :retry-token :retry-for) + (assoc :run-id (uuid/next) + :status :saving + :recovering recovering) + (update :last-progress-at d/nilv (inst-ms (ct/now))))))) + ptk/WatchEvent + (watch [_ _ _] + (rx/of (run-persistence-task)))))) + +(defn- slow-retry-cycle + "Resumes a failed queue after `slow-retry-delay-ms`, unless the queue is + resumed some other way, persistence restarts or the workspace closes." + [stream] + (let [stopper-s (rx/merge + (rx/filter (ptk/type? ::resume-persistence) stream) + (rx/filter (ptk/type? ::initialize-persistence) stream) + (rx/filter (ptk/type? ::dw/finalize-workspace) stream))] + (->> (rx/timer slow-retry-delay-ms) + (rx/map (fn [_] (resume-persistence true))) + (rx/take-until stopper-s)))) + +(defn- suspend-retries + "Stops retrying when the workspace closes. A pending backoff timer goes + stale, and no backoff or slow cycle is armed until a file is opened + again. Queued edits stay queued and a request in flight still lands." [] - (ptk/reify ::resume-persistence + (ptk/reify ::suspend-retries ptk/UpdateEvent (update [_ state] (update state :persistence - (fn [pstate] - (-> pstate - (dissoc :error :attempts :retry-token :retry-for) - (assoc :run-id (uuid/next) :status :saving) - (update :last-progress-at d/nilv (inst-ms (ct/now))))))) - ptk/WatchEvent - (watch [_ _ _] - (rx/of (run-persistence-task))))) + #(-> % + (assoc :suspended true) + (dissoc :retry-token)))))) -(defn- recover-persistence +(defn- restore-retries + "Lets failed saves be retried again once a file is opened." [] - (ptk/reify ::recover-persistence + (ptk/reify ::restore-retries + ptk/UpdateEvent + (update [_ state] + (update state :persistence dissoc :suspended)))) + +(defn- release-failed-file + "Drops the queued commits of the file whose save failed terminally. + + This runs when the user opens a file again, not at failure time. The + failure itself stays visible, because `wait-persisted-or-error` must keep + rejecting: callers use it to avoid snapshotting a file whose edits were + never saved. What must not survive is the halt. Left in place, the failed + commits sit at the head of the queue forever and, because the queue is + shared, they also block the pending edits of every other file." + [] + (ptk/reify ::release-failed-file + ptk/UpdateEvent + (update [_ state] + (let [commit-id (get-in state [:persistence :error :commit-id]) + file-id (get-in state [:persistence :index commit-id :file-id])] + (update state :persistence + #(-> % + (drop-file-commits file-id) + (dissoc :failing-since :slow-failures))))))) + +(defn- queue-needs-a-runner? + "True when the queue still holds commits that nobody is sending. + + The runner does not survive a subscription teardown, so both + initialization and recovery hit this. A halted queue is excluded: the + halt is lifted by `recover-persistence`, which has its own triggers. A + retrying episode is excluded too: its backoff timer owns the recovery, + or `recover-persistence` resumes it when retries were suspended." + [{:keys [queue status]}] + (and (seq queue) + (not= :error status) + (not= :retrying status))) + +(defn pending-file-changes + "Queued edits to restore into a freshly loaded file, in queue order. + Only edits of the loaded version belong in its data. A permanently + failed file is excluded because recovery releases its edits." + [{:keys [persistence]} {:keys [id vern]}] + (let [{:keys [queue index status error]} persistence + failed-file-id (get-in index [(:commit-id error) :file-id])] + (if (and (= :error status) + (= id failed-file-id) + (not (slow-retry-pending? persistence))) + [] + (into [] + (comp (map #(get index %)) + (filter #(and (= id (:file-id %)) + (= vern (:file-vern %)))) + (mapcat :redo-changes)) + queue)))) + +(defn- initialize-persistence-state + "Brings persistence to a coherent starting shape when the workspace + mounts. + + Deliberately leaves a halted or retrying queue alone. Lifting a halt is + `recover-persistence`'s job, and it waits for the user to open a file, so + doing it here would release edits the moment the view mounts rather than + when the user acts on them." + [] + (ptk/reify ::initialize-persistence-state ptk/WatchEvent (watch [_ state _] - (let [{:keys [queue index status error run-id]} (:persistence state) - commit (get index (peek queue))] + (let [{:keys [queue status run-id]} (:persistence state)] (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)) - (= (:file-id commit) (:current-file-id state)) - (dm/get-in state [:permissions :can-edit])))) + ;; A commit left queued by a previous session, with no runner. + (queue-needs-a-runner? (:persistence state)) (rx/of (resume-persistence)) + ;; Nothing queued and nothing in flight: there is nothing to save. (and (empty? queue) (not= status :error) (or run-id (#{:pending :saving} status))) - (rx/of (update-status :saved))))))) + (rx/of (update-status :saved)) + + :else + (rx/empty)))))) + +(defn- recover-persistence + "Lifts a halted queue when a file is opened again or the role changes. + + Leaving the workspace suspends retries (`suspend-retries`), so opening a + file (`opened-file?`) restores them, resends a retrying episode whose + backoff was cut short, and sends a failure the slow cycle still covers + (`slow-retry-pending?`) once more instead of dropping it. + + Every other terminal failure is permanent for that file: permission lost, + file gone, or changes the server refuses. None of them can succeed on a + resend, so opening a file releases them instead of replaying them. + Retaining them until here is deliberate: it is what keeps + `wait-persisted-or-error` rejecting, which is what stops + `workspace/versions.cljs` from building a snapshot out of unsaved edits. + + A lost permission is not a temporary condition. The backend raises + `:not-found` for it, and replaying the save to find that out routes the + user to the exception page." + [opened-file?] + (ptk/reify ::recover-persistence + ptk/WatchEvent + (watch [_ state _] + (let [{:keys [queue status run-id suspended]} (:persistence state)] + (rx/concat + (if (and opened-file? suspended) + (rx/of (restore-retries)) + (rx/empty)) + (cond + ;; A live retry episode owns its recovery through the backoff + ;; scheduler; resuming here would bypass the attempt budget. + ;; A suspended one has no scheduler left. + (= :retrying status) + (if (and opened-file? suspended) + (rx/of (resume-persistence)) + (rx/empty)) + + (slow-retry-pending? (:persistence state)) + (rx/of (resume-persistence true)) + + (= :error status) + (rx/of (release-failed-file) (resume-persistence)) + + (queue-needs-a-runner? (:persistence state)) + (rx/of (resume-persistence)) + + (and (empty? queue) (or run-id (#{:pending :saving} status))) + (rx/of (update-status :saved)) + + :else + (rx/empty))))))) (def ^:private xf-mapcat-undo (mapcat :undo-changes)) @@ -552,13 +761,14 @@ (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." + stay terminal: only a live `:retrying` episode resumes, never one + suspended outside the workspace." [] (ptk/reify ::resume-on-online ptk/WatchEvent (watch [_ state _] - (let [{:keys [status queue]} (:persistence state)] - (if (and (seq queue) (= :retrying status)) + (let [{:keys [status queue suspended]} (:persistence state)] + (if (and (seq queue) (= :retrying status) (not suspended)) (rx/of (run-persistence-task)) (rx/empty)))))) @@ -601,12 +811,17 @@ (rx/filter #(= % ::force-persist))))] (rx/merge - (rx/of (recover-persistence)) + (rx/of (initialize-persistence-state)) + + (->> stream + (rx/filter (ptk/type? ::dw/finalize-workspace)) + (rx/map (fn [_] (suspend-retries))) + (rx/take-until stoper-s)) (->> stream (rx/filter #(or (ptk/type? ::dc/change-team-role %) (ptk/type? ::dw/workspace-initialized %))) - (rx/map (fn [_] (recover-persistence))) + (rx/map #(recover-persistence (ptk/type? ::dw/workspace-initialized %))) (rx/take-until stoper-s)) (->> (rx/interval saving-check-interval-ms) @@ -627,14 +842,16 @@ (rx/take-until stoper-s)) ;; Syncs the leave-page warning on every local commit and on every - ;; queue change (append after a flush, discard after a save). + ;; queue change (append after a flush, discard after a save, + ;; release of a failed file). (->> (rx/merge (rx/of false) (->> local-commits-s (rx/map (fn [_] (vreset! buffered? true)))) (->> stream (rx/filter #(or (ptk/type? ::append-commit %) - (ptk/type? ::discard-commit %))) + (ptk/type? ::discard-commit %) + (ptk/type? ::release-failed-file %))) (rx/map (fn [_] @buffered?)))) (rx/map sync-unload-guard) (rx/take-until stoper-s)) diff --git a/frontend/src/app/main/data/workspace.cljs b/frontend/src/app/main/data/workspace.cljs index b04bf5394b..8390a3ca37 100644 --- a/frontend/src/app/main/data/workspace.cljs +++ b/frontend/src/app/main/data/workspace.cljs @@ -9,6 +9,7 @@ [app.common.attrs :as attrs] [app.common.data :as d] [app.common.data.macros :as dm] + [app.common.files.changes :as cpc] [app.common.files.changes-builder :as pcb] [app.common.files.helpers :as cfh] [app.common.files.variant :as cfv] @@ -322,6 +323,22 @@ :file-id file-id :team-id team-id))))))) +(defn- restore-pending-changes + "Applies the edits still waiting to be saved to a freshly loaded `file`. + An edit that no longer applies to the loaded data leaves the file as the + server sent it; the save queue keeps the edits either way." + [state file] + (let [changes (dps/pending-file-changes state file)] + (if (empty? changes) + file + (try + (update file :data cpc/process-changes changes false) + (catch :default cause + (log/wrn :hint "unable to restore pending changes" + :file-id (dm/str (:id file)) + :cause cause) + file))))) + (defn- bundle-fetched [{:keys [file file-id thumbnails] :as bundle}] (ptk/reify ::bundle-fetched @@ -331,9 +348,10 @@ ptk/UpdateEvent (update [_ state] - (-> state - (assoc :thumbnails (d/update-vals thumbnails (fn [uri] {:uri uri :rendered-at nil}))) - (update :files assoc file-id file))))) + (let [file (restore-pending-changes state file)] + (-> state + (assoc :thumbnails (d/update-vals thumbnails (fn [uri] {:uri uri :rendered-at nil}))) + (update :files assoc file-id file)))))) (defn zoom-to-frame [] diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 3eb4496bfb..54401c94af 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -490,6 +490,7 @@ (defn flash "Show error notification banner and emit error report. A nil timeout keeps the notification visible until dismissed or replaced. + A `:tag` lets the notification be hidden later by that tag. The payload format is derived from the cause: environment failures get a compact report. The audit event name is the canonical one requested by @@ -520,7 +521,7 @@ synchronously from inside an error handler creates a re-entrant event-processing cycle that can exhaust the JS call stack (RangeError: Maximum call stack size exceeded)." - [& {:keys [type hint cause timeout report-link?] + [& {:keys [type hint cause timeout tag report-link? links] :or {type :handled timeout 5000}}] (js/Promise. (fn [resolve _reject] @@ -533,16 +534,26 @@ link-report (when report-link? (or report (when (ex/exception? cause) - (generate-report cause {:format (report-format cause)}))))] + (generate-report cause {:format (report-format cause)})))) + ;; The download link comes first because `:report-link?` is + ;; what puts the report on the toast at all; caller links are + ;; the recovery actions and follow it. + all-links (cond-> [] + link-report + (conj {:label (tr "labels.download" "report.txt") + :callback (partial download-report! link-report)}) + (seq links) (into links))] (try (st/emit! (ntf/show (cond-> {:content (or ^boolean hint (tr "errors.generic")) :type :toast :level :error :timeout timeout} - link-report - (assoc :links [{:label (tr "labels.download" "report.txt") - :callback (partial download-report! link-report)}])))) + (some? tag) + (assoc :tag tag) + + (seq all-links) + (assoc :links all-links)))) (catch :default err (.error js/console "error on emitting toast" err))) (resolve report))))))) @@ -558,6 +569,15 @@ :type :handled :hint (tr "errors.connection-error"))) +(defn- handle-transport-error + "Reports a failure whose answer did not come from the backend. These affect + one request and pass on their own, so the user keeps the page and gets a + toast." + [error prefix] + (when-let [cause (::instance error)] + (ex/print-throwable cause :prefix prefix)) + (flash :cause (::instance error) :type :handled)) + (defmethod ptk/handle-error :network [error] ;; Transient network errors (e.g. lost connectivity, DNS failure) @@ -571,6 +591,18 @@ ;; that would report it as an unhandled application error. (handle-connectivity-error error "Offline Error")) +(defmethod ptk/handle-error :gateway-error + [error] + (handle-connectivity-error error "Gateway Error")) + +(defmethod ptk/handle-error :rate-limit + [error] + (handle-transport-error error "Rate Limit Error")) + +(defmethod ptk/handle-error :unexpected-response + [error] + (handle-transport-error error "Unexpected Response")) + (def ^:private delegated-persistence-types "Save failure causes routed to their own error handler: retaining the changes cannot resolve them." @@ -583,18 +615,72 @@ ;; The retained changes no longer apply to the restored version. (= :vern-conflict code))) +(def ^:private unhandled-persistence-types + "Save failure causes that are application defects, not expected + outcomes: the client asked for something the server refused, or the + server or our own code broke. They must reach the internal reports and + the alerts, so they are reported as unhandled. + Environment failures are deliberately absent: the user's connection is + not our bug, and they land here once the retry budget is spent. + `:assertion` is absent on purpose too, because its own handler already + reports it as handled." + #{:validation :server-error :internal}) + +(defn- persistence-report-type + "The audit event name a save failure gets. `persistence-failed` wraps the + original cause under `:type :persistence` and keeps the real type in + `:cause-type`, so `:cause-type` wins: reading `:type` alone would match the + wrapper and never the cause." + [{:keys [type cause-type]}] + (if (contains? unhandled-persistence-types (or cause-type type)) + :unhandled + :handled)) + +(defn- reload-page-link + "Recovery action for a save failure: reload the window. + + Persistence stops for the session once a save fails terminally, and + reloading is the only way to get a fresh one without the user having to + know they can also leave the file and come back. `location.reload` is the + pattern used elsewhere in the app for a full restart." + [] + {:label (tr "labels.reload-page") + :callback (fn [event] + (dom/prevent-default event) + (js/location.reload))}) + +(def persistence-failed-tag + "Tag of the save failure notification, hidden once a save lands." + :persistence-failed) + (defn flash-persistence - [cause] + "Notifies a save failure. With `:retrying` the failure is transient and + the queue keeps trying in the background, so the toast says the changes + are kept and offers no reload, which would lose them." + [cause & {:keys [retrying]}] (let [data (ex-data cause)] - (if (delegated-persistence-failure? data) + (cond + (delegated-persistence-failure? data) ;; The persistence state wraps the failure and records the original ;; type under :cause-type; dispatch on it to reach the cause's handler. (on-error (-> (exception->error-data cause) (assoc :type (or (:cause-type data) (:type data))))) + + retrying (flash :cause cause - :type :handled + :type (persistence-report-type data) :timeout nil + :tag persistence-failed-tag :report-link? true + :hint (tr "errors.save-still-retrying")) + + :else + (flash :cause cause + :type (persistence-report-type data) + :timeout nil + :tag persistence-failed-tag + :report-link? true + :links [(reload-page-link)] :hint (tr "errors.save-failed"))))) (defmethod ptk/handle-error :persistence diff --git a/frontend/src/app/main/repo.cljs b/frontend/src/app/main/repo.cljs index 01ce9e4c01..8a9e832429 100644 --- a/frontend/src/app/main/repo.cljs +++ b/frontend/src/app/main/repo.cljs @@ -32,6 +32,7 @@ #{:network ; js/fetch network-level failure :bad-gateway ; 502 :service-unavailable ; 503 + :gateway-error ; 504 and the Cloudflare 52x family :offline}) ; status 0 (browser offline) (defn retryable-error? @@ -79,6 +80,28 @@ ;; -- Response handling ------------------------------------------------------- +(def ^:private gateway-statuses + "Statuses raised by an edge proxy when the request never reached the + backend or its answer never came back." + #{504 ; gateway timeout + 520 ; cloudflare: unknown origin error + 521 ; cloudflare: origin is down + 522 ; cloudflare: connection timed out + 523 ; cloudflare: origin unreachable + 524}) ; cloudflare: origin timed out + +(def ^:private max-body-excerpt + "Number of characters kept from a body that we could not interpret." + 500) + +(defn- body-excerpt + "Keeps the head of an uninterpretable body. Error pages from proxies run + into tens of kilobytes and the excerpt is enough to recognize them." + [body] + (if (and (string? body) (> (count body) max-body-excerpt)) + (str (subs body 0 max-body-excerpt) "...") + body)) + (defn handle-response [{:keys [status body headers uri] :as response}] (cond @@ -121,6 +144,35 @@ (and (>= status 400) (map? body)) (rx/throw (ex-info "http error" (assoc body :uri uri :status status))) + ;; The branches below handle responses the backend never wrote: the + ;; body carries no error code, so the status is all we can read. + + (contains? gateway-statuses status) + (rx/throw + (ex/error :type :gateway-error + :code :gateway-error + :hint "the request did not reach the backend" + :uri uri + :status status)) + + (= 429 status) + (rx/throw + (ex/error :type :rate-limit + :code :rate-limited + :hint "too many requests" + :uri uri + :status status)) + + (>= status 400) + (rx/throw + (ex/error :type :unexpected-response + :code :unexpected-error-response + :hint "unexpected error response" + :uri uri + :status status + :headers headers + :data (body-excerpt body))) + :else (rx/throw (ex/error :type :internal @@ -129,7 +181,7 @@ :uri uri :status status :headers headers - :data body)))) + :data (body-excerpt body))))) (def default-options {:update-file {:query-params [:id]} diff --git a/frontend/test/frontend_tests/data/fix_deleted_fonts_test.cljs b/frontend/test/frontend_tests/data/fix_deleted_fonts_test.cljs index 4592c90ab3..b2b5c2e083 100644 --- a/frontend/test/frontend_tests/data/fix_deleted_fonts_test.cljs +++ b/frontend/test/frontend_tests/data/fix_deleted_fonts_test.cljs @@ -2,7 +2,7 @@ ;; 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 +;; Copyright (c) KALEIDOS INC Sucursal en España SL (ns frontend-tests.data.fix-deleted-fonts-test (:require diff --git a/frontend/test/frontend_tests/data/persistence_retry_test.cljs b/frontend/test/frontend_tests/data/persistence_retry_test.cljs index c2ddab917e..6bbfbedf47 100644 --- a/frontend/test/frontend_tests/data/persistence_retry_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_retry_test.cljs @@ -2,7 +2,7 @@ ;; 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 +;; Copyright (c) KALEIDOS INC Sucursal en España SL (ns frontend-tests.data.persistence-retry-test "Retry-episode tests for the file persistence save pipeline @@ -83,18 +83,26 @@ (rx/dispose! store) (rx/end! response)))))) +(defn- instant-backoff + "Timer double: the backoff delays fire at once and the slow cycle never + does, so a failure that outlasts the backoff settles in `:error`." + [ms] + (if (= ms dps/slow-retry-delay-ms) + (rx/subject) + (rx/of :tick))) + (defn- check-failed-save-response "Feeds `result` as the transport response and asserts the commit stays queued as a failed save instead of being treated as persisted. Retry - timers fire instantly (stubbed `rx/timer`), so a permanently bad answer - exhausts the 3-retry budget and lands terminal: `:error` carrying + timers fire instantly (stubbed `rx/timer`, slow cycle held), so a + permanently bad answer exhausts the 3-retry budget and lands terminal: `:error` carrying `:invalid-save-response`, queue intact, exactly 4 sends." [result] (with-persistence (^:async fn [{:keys [file-id requests store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + {rx/timer (mock/stub instant-backoff)} (ptk/emit! store (local-commit file-id) ::dps/force-persist) (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) (= :invalid-save-response @@ -141,7 +149,7 @@ (^:async fn [{:keys [file-id requests store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + {rx/timer (mock/stub instant-backoff)} (ptk/emit! store (local-commit file-id) ::dps/force-persist) (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) (empty? (get-in @store [:persistence :queue]))) @@ -158,6 +166,39 @@ (rx/throw (ex-info "offline" {:type :offline})) (rx/of {:revn 1}))))))) +(defn- check-gateway-response-retries + "Answers the first send with a proxy error page for `status`, built by the + real `rp/handle-response`, and the retry with a valid revision. The save + must take the retry path and land, never halt in `:error`." + [status] + (let [calls (atom 0)] + (with-persistence + (^:async fn [{:keys [file-id failures requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub instant-backoff)} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + (str "a " status " is retried and the save lands"))) + (t/is (= 2 (count @requests)) "the proxy error plus one retry") + (t/is (empty? @failures) "a retried proxy error never reaches the failure toast")))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rp/handle-response {:status status + :headers {"content-type" "text/html"} + :body "Gateway Timeout" + :uri "https://penpot.example.com/api/main/methods/update-file"}) + (rx/of {:revn 1})))))) + +;; Variant: the proxy in front of the backend timed out. +(t/deftest ^:async gateway-timeout-save-is-retried + (await (check-gateway-response-retries 504))) + +;; Variant: Cloudflare gave up waiting for the origin. +(t/deftest ^:async cloudflare-origin-timeout-save-is-retried + (await (check-gateway-response-retries 524))) + ;; 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 @@ -169,7 +210,7 @@ (^:async fn [{:keys [file-id failures requests store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [ms] (swap! delays conj ms) (rx/of :tick)))} + {rx/timer (mock/stub (fn [ms] (swap! delays conj ms) (instant-backoff ms)))} (ptk/emit! store (local-commit file-id) ::dps/force-persist) (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) (= 4 (count @requests))) @@ -177,7 +218,8 @@ (t/is (= :error (get-in @store [:persistence :status]))) (t/is (= :save-failed (get-in @store [:persistence :error :code]))) (t/is (= 1 (count (get-in @store [:persistence :queue])))) - (t/is (= [2000 8000 20000] @delays) "the backoff schedule fires in order") + (t/is (= [2000 8000 20000 dps/slow-retry-delay-ms] @delays) + "the backoff schedule fires in order, then the slow cycle is armed") (t/is (= 1 (count @failures)) "exhaustion flashes exactly once")))) (fn [_ _] (rx/throw (ex-info "offline" {:type :offline}))))))) @@ -221,9 +263,9 @@ (rx/subject))))))) ;; Scenario: a persist-commit arrives with a superseded episode token after -;; a transient failure. Without the token guard it would fail terminally as -;; `:save-outcome-unknown`; with it, nothing happens. Proves: stale retry -;; timers stay silent. +;; a transient failure. Without the token guard it would send outside the +;; backoff schedule; with it, nothing happens. Proves: stale retry timers +;; stay silent. (t/deftest ^:async stale-retry-token-stays-silent (await (with-persistence @@ -333,7 +375,7 @@ (^:async fn [{:keys [file-id store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [_] (rx/of :tick)))} + {rx/timer (mock/stub instant-backoff)} (ptk/emit! store (local-commit file-id) ::dps/force-persist) (await (async/wait-for #(= :error (get-in @store [:persistence :status])) "retries exhaust into terminal error")) @@ -503,7 +545,7 @@ (^:async fn [{:keys [file-id requests store]}] (await (mock/with-mocks* - {rx/timer (mock/stub (fn [_] (rx/of :tick))) + {rx/timer (mock/stub instant-backoff) rx/timeout (mock/stub (fn [ms with ob] (swap! timeouts conj ms) (timeout 0 with ob)))} @@ -519,3 +561,324 @@ (if (= 1 (swap! calls inc)) (rx/subject) (rx/of {:revn 1}))))))) + +;; --------------------------------------------------------------------------- +;; slow cycle +;; --------------------------------------------------------------------------- + +(defn- slow-cycle-timer + "Timer double: the backoff delays fire at once, and every slow cycle waits + on a subject collected in `cycles`, so the test fires them one by one." + [cycles] + (fn [ms] + (if (= ms dps/slow-retry-delay-ms) + (let [cycle-s (rx/subject)] + (swap! cycles conj cycle-s) + cycle-s) + (rx/of :tick)))) + +;; Scenario: the backoff is spent while the network is down. The queue keeps +;; trying on the slow cycle, one send per cycle, without warning the user +;; again, and saves once the network is back. Proves: a session outlasting +;; the backoff still recovers on its own. +(t/deftest ^:async a-spent-backoff-keeps-trying-on-a-slow-cycle + (let [calls (atom 0) + cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id failures requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= 1 (count @cycles))) + "the spent backoff arms the slow cycle")) + (t/is (= 4 (count @requests))) + (t/is (= 1 (count @failures))) + + (rx/push! (last @cycles) :tick) + (await (async/wait-for #(= 2 (count @cycles)) + "a failed slow attempt arms the next cycle")) + (t/is (= 5 (count @requests)) "a slow cycle sends once") + (t/is (= :error (get-in @store [:persistence :status]))) + (t/is (= 1 (count @failures)) "the user is not warned again") + + ;; The warning the first failure left on screen. + (ptk/emit! store #(assoc % :notification {:tag errors/persistence-failed-tag + :level :error})) + (rx/push! (last @cycles) :tick) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the slow attempt saves")) + (t/is (= 6 (count @requests))) + (t/is (apply = (map (comp :commit-id second) @requests)) + "every attempt carries the same commit id") + (t/is (nil? (get-in @store [:persistence :recovering]))) + (t/is (nil? (get-in @store [:persistence :failing-since]))) + (t/is (nil? (get @store :notification)) + "the failure warning goes away once the save lands")))) + (fn [_ _] + (if (< (swap! calls inc) 6) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/of {:revn 1}))))))) + +;; Scenario: a gateway error outlasts the backoff, then the proxy recovers. +;; Proves: each gateway status takes the slow cycle and saves the held edit. +(t/deftest ^:async a-gateway-error-arms-the-slow-cycle + (doseq [status [504 520 521 522 523 524]] + (let [calls (atom 0) + cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (count @cycles)) + (str status " arms the slow cycle"))) + (t/is (= 4 (count @requests))) + (t/is (= :gateway-error (get-in @store [:persistence :error :cause-type]))) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + + (rx/push! (last @cycles) :tick) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + (str status " recovers on the slow cycle"))) + (t/is (= 5 (count @requests))) + (t/is (apply = (map (comp :commit-id second) @requests)) + "the slow cycle saves the held commit")))) + (fn [_ _] + (if (<= (swap! calls inc) 4) + (rp/handle-response {:status status :body "Gateway error" :uri "/api"}) + (rx/of {:revn 1})))))))) + +;; Scenario: a save fails for a reason no attempt can get past. Proves: only +;; transport failures arm the slow cycle. +(t/deftest ^:async a-terminal-failure-does-not-arm-the-slow-cycle + (let [cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "the save fails")) + (await (async/settle)) + (t/is (= 1 (count @requests))) + (t/is (empty? @cycles))))) + (fn [_ _] (rx/throw (ex-info "Validation failed" {:type :validation}))))))) + +;; Scenario: a gateway answers 504 only after its own timeout, while the +;; backend may still be applying the save. Proves: slow failures below the +;; limit still arm the slow cycle; at the limit the save stops being sent, +;; and opening a file releases it instead of sending it again. +(t/deftest ^:async slow-failures-stop-the-slow-cycle-at-the-limit + (doseq [[slow-count armed?] [[(dec dps/max-slow-failures) true] + [dps/max-slow-failures false]]] + (let [clock (atom 0) + calls (atom 0) + cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles)) + ct/now (mock/stub #(ct/inst @clock))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + (str slow-count " slow failures spend the backoff"))) + (await (async/settle)) + (t/is (= 4 (count @requests))) + (t/is (= slow-count (get-in @store [:persistence :slow-failures]))) + (t/is (= (if armed? 1 0) (count @cycles)) + (str slow-count " slow failures " (if armed? "arm" "do not arm") " the slow cycle")) + + (when-not armed? + (ptk/emit! store (ptk/data-event :app.main.data.common/change-team-role)) + (await (async/wait-for #(empty? (get-in @store [:persistence :queue])) + "recovery releases the held edit")) + (t/is (= 4 (count @requests)) "the released edit is not sent again"))))) + (fn [_ _] + (when (<= (swap! calls inc) slow-count) + (swap! clock + dps/slow-failure-ms)) + (rp/handle-response {:status 504 :body "Gateway timeout" :uri "/api"}))))))) + +;; Scenario: the slow cycle is still failing when the retry window closes. +;; Proves: the cycle gives up, keeping the edits queued. +(t/deftest ^:async the-slow-cycle-stops-when-the-retry-window-closes + (let [clock (atom 0) + cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles)) + ct/now (mock/stub #(ct/inst @clock))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (count @cycles)) "the slow cycle is armed")) + + (reset! clock (+ dps/retry-give-up-ms 1000)) + (rx/push! (last @cycles) :tick) + (await (async/wait-for #(= 5 (count @requests)) "the last slow attempt")) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "the attempt fails")) + (await (async/settle)) + (t/is (= 1 (count @cycles)) "no further cycle is armed") + (t/is (= 1 (count (get-in @store [:persistence :queue]))))))) + (fn [_ _] (rx/throw (ex-info "offline" {:type :offline}))))))) + +;; Scenario: the backoff is spent and the user leaves the workspace, which +;; stops the slow cycle, then opens a file again. Recovery sends the queue +;; once more instead of releasing it, and it saves. Proves: edits held by a +;; transport failure are never dropped by recovery. +(t/deftest ^:async recovery-resends-a-transport-failure-instead-of-releasing-it + (let [calls (atom 0) + cycles (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (count @cycles)) "the slow cycle is armed")) + (ptk/emit! store (ptk/data-event :app.main.data.workspace/finalize-workspace)) + (ptk/emit! store (ptk/data-event :app.main.data.common/change-team-role)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "recovery saves the held edit")) + (t/is (= 5 (count @requests))) + (t/is (apply = (map (comp :commit-id second) @requests)) + "recovery sends the same commit, not a released queue")))) + (fn [_ _] + (if (< (swap! calls inc) 5) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/of {:revn 1}))))))) + +;; Leaving the workspace tests. + +;; Scenario: a save fails on a transport error and the user leaves the +;; workspace before the backoff fires. The pending retry stays silent, and +;; opening a file sends the held edit again. Proves: the backoff does not +;; run outside the workspace and resumes once a file is opened. +(t/deftest ^:async leaving-the-workspace-stops-the-backoff + (let [calls (atom 0) + timers (atom [])] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [_] + (let [timer-s (rx/subject)] + (swap! timers conj timer-s) + timer-s)))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (count @timers)) "the failure schedules a retry")) + + (ptk/emit! store (ptk/data-event :app.main.data.workspace/finalize-workspace)) + (await (async/wait-for #(get-in @store [:persistence :suspended]) + "leaving the workspace suspends retries")) + (rx/push! (first @timers) :tick) + (await (async/settle)) + (t/is (= 1 (count @requests)) "the pending retry stays silent") + (t/is (= :retrying (get-in @store [:persistence :status]))) + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + + (ptk/emit! store (ptk/data-event :app.main.data.workspace/workspace-initialized)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "opening a file saves the held edit")) + (t/is (= 2 (count @requests))) + (t/is (apply = (map (comp :commit-id second) @requests)) + "the same commit is sent again") + (t/is (nil? (get-in @store [:persistence :suspended])))))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + (rx/throw (ex-info "offline" {:type :offline})) + (rx/of {:revn 1}))))))) + +;; Scenario: the user leaves the workspace while a save is in flight, and the +;; save then fails on a transport error. No backoff is scheduled, the online +;; event does not resend, and opening a file does. Proves: a failure that +;; lands outside the workspace waits for the user instead of retrying there. +(t/deftest ^:async a-failure-after-leaving-the-workspace-arms-no-retry + (let [calls (atom 0) + timers (atom []) + first-s (rx/subject)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [ms] (swap! timers conj ms) (rx/subject)))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (count @requests)) "the edit is sent")) + + (ptk/emit! store (ptk/data-event :app.main.data.workspace/finalize-workspace)) + (await (async/wait-for #(get-in @store [:persistence :suspended]) + "leaving the workspace suspends retries")) + (rx/error! first-s (ex-info "offline" {:type :offline})) + (await (async/wait-for #(= :retrying (get-in @store [:persistence :status])) + "the failure keeps the edit queued")) + (await (async/settle)) + (t/is (empty? @timers) "no backoff is scheduled") + + (ptk/emit! store (#'dps/resume-on-online)) + (await (async/settle)) + (t/is (= 1 (count @requests)) "the online event does not resend") + + (ptk/emit! store (ptk/data-event :app.main.data.workspace/workspace-initialized)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "opening a file saves the held edit")) + (t/is (= 2 (count @requests)))))) + (fn [_ _] + (if (= 1 (swap! calls inc)) + first-s + (rx/of {:revn 1}))))))) + +;; Scenario: the last backoff attempt is in flight when the user leaves the +;; workspace, and it fails. The queue halts without arming the slow cycle, +;; and opening a file sends it once more. Proves: the slow cycle never +;; starts outside the workspace. +(t/deftest ^:async a-spent-backoff-outside-the-workspace-arms-no-slow-cycle + (let [calls (atom 0) + cycles (atom []) + last-s (rx/subject)] + (await + (with-persistence + (^:async fn [{:keys [file-id requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (slow-cycle-timer cycles))} + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 4 (count @requests)) "the last retry is sent")) + + (ptk/emit! store (ptk/data-event :app.main.data.workspace/finalize-workspace)) + (await (async/wait-for #(get-in @store [:persistence :suspended]) + "leaving the workspace suspends retries")) + (rx/error! last-s (ex-info "offline" {:type :offline})) + (await (async/wait-for #(= :error (get-in @store [:persistence :status])) + "the spent backoff halts the queue")) + (await (async/settle)) + (t/is (empty? @cycles) "no slow cycle is armed") + (t/is (= 1 (count (get-in @store [:persistence :queue])))) + + (ptk/emit! store (ptk/data-event :app.main.data.workspace/workspace-initialized)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "opening a file saves the held edit")) + (t/is (= 5 (count @requests)))))) + (fn [_ _] + (let [n (swap! calls inc)] + (cond + (< n 4) (rx/throw (ex-info "offline" {:type :offline})) + (= n 4) last-s + :else (rx/of {:revn 1})))))))) diff --git a/frontend/test/frontend_tests/data/persistence_test.cljs b/frontend/test/frontend_tests/data/persistence_test.cljs index 0f72a59025..7fa453aaf4 100644 --- a/frontend/test/frontend_tests/data/persistence_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_test.cljs @@ -18,10 +18,12 @@ `mock/with-mocks` with an explicit `done` chain." (:require [app.common.time :as ct] + [app.common.types.shape :as cts] [app.common.uuid :as uuid] [app.main.data.changes :as dch] [app.main.data.persistence :as dps] [app.main.data.render-wasm :as drw] + [app.main.data.workspace :as dw] [app.main.errors :as errors] [app.main.repo :as rp] [app.main.store :as st] @@ -32,6 +34,74 @@ [frontend-tests.helpers.mock :as mock] [potok.v2.core :as ptk])) +(t/deftest reopening-restores-only-the-retained-edits-of-the-loaded-file + (doseq [[status cause-type vern restored?] + [[:retrying nil 0 true] + [:saving nil 0 true] + [:error :offline 0 true] + [:error :validation 0 false] + [:retrying nil 1 false]]] + (t/testing (str status " / " cause-type " / version " vern) + (let [file-id (uuid/next) + page-id (uuid/next) + shape-id (uuid/next) + first-id (uuid/next) + next-id (uuid/next) + other-id (uuid/next) + shape (cts/setup-shape {:id shape-id :type :rect :x 0 :y 0}) + file {:id file-id + :vern vern + :data {:pages [page-id] + :pages-index {page-id {:id page-id :name "Page" + :objects {uuid/zero + (cts/setup-shape {:id uuid/zero :type :frame})}}}}} + rename (fn [name] + {:type :mod-obj :id shape-id :page-id page-id + :operations [{:type :set :attr :name :val name}]}) + pstate {:status status + :error {:cause-type cause-type :commit-id first-id} + :queue (into #queue [] [first-id next-id other-id]) + :index {first-id {:file-id file-id :file-vern 0 + :redo-changes [{:type :add-obj :id shape-id :obj shape + :page-id page-id :parent-id uuid/zero + :frame-id uuid/zero}]} + next-id {:file-id file-id :file-vern 0 + :redo-changes [(rename "Retained rectangle")]} + other-id {:file-id (uuid/next) :file-vern 0 + :redo-changes [(rename "Another file")]}}} + state {:persistence pstate} + loaded (ptk/update (#'dw/bundle-fetched {:file file :file-id file-id :thumbnails {}}) + state) + restored (get-in loaded [:files file-id :data :pages-index page-id :objects shape-id])] + (t/is (= restored? (some? restored))) + (when restored? + (t/is (= "Retained rectangle" (:name restored)) + "queued edits apply in order and exclude other files")) + (t/is (identical? pstate (:persistence loaded)) + "restoring the editor must preserve the save queue"))))) + +;; Scenario: a queued edit no longer applies to the data the server sent, so +;; replaying it throws. Proves: the file still opens, as the server sent it, +;; and the edits stay queued. +(t/deftest reopening-with-an-edit-that-no-longer-applies-keeps-the-server-file + (let [file-id (uuid/next) + page-id (uuid/next) + commit-id (uuid/next) + data {:pages [page-id] + :pages-index {page-id {:id page-id :name "Page" + :objects {uuid/zero + (cts/setup-shape {:id uuid/zero :type :frame})}}}} + file {:id file-id :vern 0 :data data} + pstate {:status :saving + :queue (into #queue [] [commit-id]) + :index {commit-id {:file-id file-id :file-vern 0 + :redo-changes [{:type :mod-obj :id uuid/zero :page-id page-id + :operations [{:type :no-longer-supported}]}]}}} + loaded (ptk/update (#'dw/bundle-fetched {:file file :file-id file-id :thumbnails {}}) + {:persistence pstate})] + (t/is (identical? data (get-in loaded [:files file-id :data]))) + (t/is (identical? pstate (:persistence loaded))))) + (defn- local-commit "Builds a synthetic local commit event for `file-id`: a page edit as redo, no undo, from the `:local` source." @@ -430,8 +500,8 @@ ;; - Transport failures and malformed answers (`nil`, empty, bad revision) ;; fail the save but preserve the queue for retry. ;; - Initialization recovers gracefully: dangling runners error instead of -;; skipping, unsent commits are picked up, unknown outcomes are reported -;; without replaying. +;; skipping, unsent commits are picked up, unknown outcomes are sent again +;; under the same commit id. ;; ;; Same async pattern as the watchdog tests: triggers stay bare, every ;; assert block is preceded by `wait-for` on its leading signal (or a bare @@ -477,11 +547,10 @@ (rx/end! response)))))) ;; 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 +;; without sending anything, the queue keeps the edit and the user is +;; notified once, so `wait-persisted-or-error` still rejects and nothing is +;; snapshotted from unsaved state. Proves: permission loss fails safe. +(t/deftest ^:async permission-loss-retains-the-queue-and-notifies-once (await (with-persistence (^:async fn [{:keys [file-id requests failures store]}] @@ -492,37 +561,76 @@ (= 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 (= 1 (count (get-in @store [:persistence :queue]))) + "the undeliverable edit is retained, so waiters keep rejecting") (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/is (= 1 (count @failures))))))) -;; 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 +;; Scenario: the same terminal failure, then the user opens a file again. +;; Opening a file releases the edit that can never be saved and settles the +;; editor, instead of leaving it halted until a reload. Proves: the halt does +;; not outlive the visit that caused it. +(t/deftest ^:async opening-a-file-again-releases-a-terminal-failure (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)) + (^: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])) - (= 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))))))) + (= 1 (count (get-in @store [:persistence :queue])))) + "the save fails terminally")) + ;; The user acts again: a team-role change is one of the two real + ;; recovery triggers (the other is opening a file, whose event does + ;; real work and is not usable as a bare signal here). + (ptk/emit! store (ptk/data-event :app.main.data.common/change-team-role)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "re-entering the file settles the queue")) + (t/is (nil? (get-in @store [:persistence :error])) + "the halt is cleared once the user comes back") + (t/is (empty? @requests) + "the edit that cannot be saved is not replayed on re-entry") + (t/is (= 1 (count @failures)) "the user is still notified exactly once"))))) + +;; Scenario: two files have pending edits and the first send fails +;; terminally. Opening a file again releases the failed file, the other file +;; still saves, and the editor settles saved instead of staying halted. +;; Proves: one file that cannot be saved does not block every other file. +(t/deftest ^:async terminal-failure-does-not-block-other-files + (let [files (into [] (repeatedly 2 uuid/next)) + sends (atom 0)] + (await + (with-persistence + (^:async fn [{:keys [requests store]}] + (ptk/emit! store + #(assoc % :files (into (:files %) + (map (fn [id] [id {:id id :revn 0}])) + files)) + (local-commit (nth files 0)) + (local-commit (nth files 1)) + ::dps/force-persist) + (await (async/wait-for #(seq @requests) "the first file is sent")) + (let [failed-file (get-in (second (first @requests)) [:id])] + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (not (empty? (get-in @store [:persistence :queue])))) + "the terminal failure retains the queue")) + ;; The user acts again: recovery releases the failed file and drains + ;; the rest. + (ptk/emit! store (ptk/data-event :app.main.data.common/change-team-role)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the released file frees the rest of the queue")) + (t/is (= 2 (count @requests)) + "the other file was saved after the failed one was released") + (t/is (= 1 (count (filter #(= failed-file (get-in (second %) [:id])) @requests))) + "the failed file is sent once, never replayed"))) + (fn [_ _] + ;; Fail the first send terminally and answer the rest inline, so no + ;; hot-subject race can swallow the answer. + (if (= 1 (swap! sends inc)) + (rx/throw (ex-info "rejected" {:type :validation :code :invalid-data})) + (rx/of {:revn 1}))))))) (defn- check-save-failure-warning "Fails a queued save with `cause-type` and asserts the hint shown to the @@ -687,35 +795,53 @@ (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 +;; Scenario: permission is lost mid-flight, so the save fails terminally. +;; Even if the permission comes back, recovery releases the undeliverable +;; edits instead of replaying them. The backend raises `:not-found` for a +;; lost edit permission, so a resend would only route the user to the +;; exception page. Proves: permission loss is treated as permanent, like +;; every other terminal failure. +(t/deftest ^:async permission-loss-is-released-not-resumed (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}) + (^:async fn [{:keys [file-id requests 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])))) - (ptk/emit! store - #(assoc-in % [:permissions :can-edit] true) + ;; Permission comes back and a file is opened again. + (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]))))))) + "recovery releases the undeliverable edit")) + (t/is (empty? @requests) + "the undeliverable edit is never replayed, not even once the + permission is back") + (t/is (not= :error (get-in @store [:persistence :status])) + "the editor is not left halted"))))) + +;; Scenario: a terminal failure is waiting, and persistence is re-initialized. +;; Initialization must not lift the halt: only opening a file does. Otherwise +;; the edits would be released the moment the workspace view mounts, instead +;; of when the user acts on them. Proves: the init/recovery split holds. +(t/deftest ^:async initialization-does-not-lift-a-halt + (await + (with-persistence + (^:async fn [{:keys [file-id response store]}] + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (.error response (ex-info "Validation failed" {:type :validation})) + (await (async/wait-for #(and (= :error (get-in @store [:persistence :status])) + (= 1 (count (get-in @store [:persistence :queue])))) + "the save fails terminally")) + (ptk/emit! store (dps/initialize-persistence)) + (await (async/settle)) + (t/is (= :error (get-in @store [:persistence :status])) + "initialization leaves the halt alone") + (t/is (= 1 (count (get-in @store [:persistence :queue]))) + "and keeps the undeliverable edits queued until the user acts"))))) ;; Scenario: the answer arrives but no runner is tracking the request ;; anymore. The acknowledged commit stays queued instead of being dropped; @@ -740,27 +866,26 @@ (t/is (empty? (get-in @store [:persistence :queue]))))))) ;; Scenario: a queued commit carries a request id but persistence has no -;; run id for it when initializing — its outcome is unknowable. It errors -;; as `:save-outcome-unknown`, stays queued, and nothing is sent. -;; Proves: unknown outcomes report without replaying (never assume persisted, -;; never resend blindly). -(t/deftest ^:async recovery-reports-an-unknown-request-outcome-without-replaying-it +;; run id for it when initializing, so its outcome is unknown. It is sent +;; again under the same commit id and saves. Proves: an unknown outcome is +;; resent, since changes are idempotent. +(t/deftest ^:async recovery-sends-again-a-save-with-an-unknown-outcome (await (with-persistence - (^:async fn [{:keys [file-id requests store]}] + (^:async fn [{:keys [file-id response requests store]}] (let [commit (assoc @(local-commit file-id) ::dps/request-id (uuid/next)) id (:id commit)] (ptk/emit! store #(assoc % :persistence {:queue (conj #queue [] id) :index {id commit} :status :saving}) (dps/initialize-persistence)) - (await (async/wait-for #(= :save-outcome-unknown - (get-in @store [:persistence :error :code])) - "unknown outcome errors")) - (t/is (= :error (get-in @store [:persistence :status]))) - (t/is (= :save-outcome-unknown (get-in @store [:persistence :error :code]))) - (t/is (= [id] (vec (get-in @store [:persistence :queue])))) - (t/is (empty? @requests))))))) + (await (async/wait-for #(= 1 (count @requests)) "the commit is sent again")) + (t/is (= id (:commit-id (second (first @requests))))) + (rx/push! response {:revn 1}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the resent commit saves")) + (t/is (= 1 (count @requests)))))))) ;; Scenario: an edit is queued before persistence even initializes. On ;; initialization plus force-persist it is sent exactly once (still queued @@ -842,15 +967,24 @@ "the second save turns the guard off")) (t/is (empty? (get-in @store [:persistence :queue]))))))) -;; Scenario: the save fails terminally. The edit stays queued, and so does -;; the guard. Proves: retained edits are not dropped silently on leave. -(t/deftest ^:async failed-save-keeps-the-leave-page-guard +;; Scenario: a save fails terminally, then recovery releases the edit. The +;; guard stays on while the failed edit is retained, and turns off once it +;; is released. Proves: a release that empties the queue also turns the +;; leave-page warning off. +(t/deftest ^:async releasing-a-failed-edit-turns-the-leave-page-guard-off (await (with-persistence (^:async fn [{:keys [file-id store tasks]}] - (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (ptk/emit! store (local-commit file-id) + #(assoc-in % [:permissions :can-edit] false) + ::dps/force-persist) (await (async/wait-for #(= :error (get-in @store [:persistence :status])) - "the save fails")) - (t/is (= 1 (count (get-in @store [:persistence :queue])))) - (t/is (contains? @tasks :persistence) "the guard stays on for retained edits")) - (fn [_ _] (rx/throw (ex-info "invalid" {:type :validation})))))) + "the save fails terminally")) + (await (async/settle)) + (t/is (contains? @tasks :persistence) "the retained edit keeps the guard on") + (ptk/emit! store (ptk/data-event :app.main.data.common/change-team-role)) + (await (async/wait-for #(empty? (get-in @store [:persistence :queue])) + "recovery releases the edit")) + (await (async/wait-for #(not (contains? @tasks :persistence)) + "the release turns the guard off")) + (t/is (= :saved (get-in @store [:persistence :status]))))))) diff --git a/frontend/test/frontend_tests/data/repo_test.cljs b/frontend/test/frontend_tests/data/repo_test.cljs index 13066fbe9e..5334f1b719 100644 --- a/frontend/test/frontend_tests/data/repo_test.cljs +++ b/frontend/test/frontend_tests/data/repo_test.cljs @@ -8,7 +8,8 @@ (:require [app.main.repo :as repo] [beicon.v2.core :as rx] - [cljs.test :as t :include-macros true])) + [cljs.test :as t :include-macros true] + [cuerdas.core :as str])) ;; --------------------------------------------------------------------------- ;; retryable-error? tests (synchronous) @@ -29,6 +30,11 @@ (let [err (ex-info "service unavailable" {:type :service-unavailable})] (t/is (true? (repo/retryable-error? err)))))) +(t/deftest retryable-error-gateway-error + (t/testing "504 and the Cloudflare 52x family are retryable" + (let [err (ex-info "http error" {:type :gateway-error})] + (t/is (true? (repo/retryable-error? err)))))) + (t/deftest retryable-error-offline (t/testing "offline (status 0) is retryable" (let [err (ex-info "offline" {:type :offline})] @@ -215,3 +221,123 @@ (t/is (= 2 @call-count)) (t/is (= :authentication (:type (ex-data err)))) (done)))))))) + +;; --------------------------------------------------------------------------- +;; handle-response classification tests +;; --------------------------------------------------------------------------- + +(defn- capture-error + "Subscribes to `handle-response` for `response` and calls `k` with the + `ex-data` of the raised error." + [response k] + (->> (repo/handle-response response) + (rx/subs! + (fn [_] (t/is false "expected an error response") (k nil)) + (fn [cause] (k (ex-data cause)))))) + +(def ^:private html-error-page + "Web Page Blockedblocked") + +(t/deftest handle-response-gateway-statuses + (t/testing "every edge status that never reached the backend is a gateway error" + (t/async done + (let [statuses [504 520 521 522 523 524] + pending (atom (count statuses))] + (doseq [status statuses] + (capture-error {:status status + :body html-error-page + :headers {"content-type" "text/html"} + :uri "https://design.penpot.app/api/main/methods/get-teams"} + (fn [data] + (t/is (= :gateway-error (:type data)) + (str "status " status " should be a gateway error")) + (t/is (= status (:status data)) + (str "status " status " should be carried through")) + (when (zero? (swap! pending dec)) + (done))))))))) + +(t/deftest handle-response-rate-limit + (t/testing "a 429 with no interpretable body is a rate-limit error" + (t/async done + (capture-error {:status 429 + :body "" + :headers {} + :uri "https://design.penpot.app/api/main/methods/get-teams"} + (fn [data] + (t/is (= :rate-limit (:type data))) + (t/is (= 429 (:status data))) + (done)))))) + +(t/deftest handle-response-intercepted-by-proxy + (t/testing "an error page from a filtering proxy is not an internal error" + (t/async done + (capture-error {:status 403 + :body html-error-page + :headers {"content-type" "text/html" + "server" "Zscaler/6.2"} + :uri "https://design.penpot.app/api/main/methods/update-file"} + (fn [data] + (t/is (= :unexpected-response (:type data))) + (t/is (= 403 (:status data))) + (done)))))) + +(t/deftest handle-response-keeps-body-excerpt + (t/testing "a body too long to keep is cut short and marked as cut" + (t/async done + (capture-error {:status 403 + :body (apply str (repeat 5000 "x")) + :headers {"content-type" "text/html"} + :uri "https://design.penpot.app/api/main/methods/update-file"} + (fn [data] + (t/is (= 503 (count (:data data))) "500 characters and an ellipsis") + (t/is (str/ends-with? (:data data) "...")) + (done)))))) + +(t/deftest handle-response-keeps-a-short-body-whole + (t/testing "a body short enough to read is passed through untouched" + (t/async done + (capture-error {:status 403 + :body html-error-page + :headers {"content-type" "text/html"} + :uri "https://design.penpot.app/api/main/methods/update-file"} + (fn [data] + (t/is (= html-error-page (:data data))) + (done)))))) + +(t/deftest handle-response-challenge-still-authorization + (t/testing "a Cloudflare challenge keeps its own type" + (t/async done + (capture-error {:status 403 + :body html-error-page + :headers {"server" "cloudflare" + "cf-mitigated" "challenge"} + :uri "https://design.penpot.app/api/main/methods/get-teams"} + (fn [data] + (t/is (= :authorization (:type data))) + (t/is (= :challenge-required (:code data))) + (done)))))) + +(t/deftest handle-response-keeps-backend-error + (t/testing "an error the backend itself reported is passed through untouched" + (t/async done + (capture-error {:status 400 + :body {:type :validation :code :missing-param} + :headers {"content-type" "application/transit+json"} + :uri "https://design.penpot.app/api/main/methods/update-file"} + (fn [data] + (t/is (= :validation (:type data))) + (t/is (= :missing-param (:code data))) + (t/is (= 400 (:status data))) + (done)))))) + +(t/deftest handle-response-undecodable-non-error-is-internal + (t/testing "a non-error response we cannot interpret is still an internal error" + (t/async done + (capture-error {:status 301 + :body html-error-page + :headers {"content-type" "text/html"} + :uri "https://design.penpot.app/api/main/methods/get-teams"} + (fn [data] + (t/is (= :internal (:type data))) + (t/is (= :unable-to-process-repository-response (:code data))) + (done)))))) diff --git a/frontend/test/frontend_tests/errors_governor_test.cljs b/frontend/test/frontend_tests/errors_governor_test.cljs index c0998a7280..a51f775a43 100644 --- a/frontend/test/frontend_tests/errors_governor_test.cljs +++ b/frontend/test/frontend_tests/errors_governor_test.cljs @@ -2,7 +2,7 @@ ;; 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 +;; Copyright (c) KALEIDOS INC Sucursal en España SL (ns frontend-tests.errors-governor-test "Unit tests for the error report governor (`app.main.errors`). diff --git a/frontend/test/frontend_tests/helpers/async.cljs b/frontend/test/frontend_tests/helpers/async.cljs index 36bbaed769..1e52a49c25 100644 --- a/frontend/test/frontend_tests/helpers/async.cljs +++ b/frontend/test/frontend_tests/helpers/async.cljs @@ -2,7 +2,7 @@ ;; 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 +;; Copyright (c) KALEIDOS INC Sucursal en España SL (ns frontend-tests.helpers.async "Async-first plumbing for ClojureScript tests. diff --git a/frontend/test/frontend_tests/helpers/mock.cljc b/frontend/test/frontend_tests/helpers/mock.cljc index b55b8e24bd..c5a2d8eb49 100644 --- a/frontend/test/frontend_tests/helpers/mock.cljc +++ b/frontend/test/frontend_tests/helpers/mock.cljc @@ -29,7 +29,11 @@ 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)`" + Usage: `(with-mocks {ns/sym mock-fn, ...} test-fn done)`. + + `with-mocks*` is the preferred form for new tests: it wraps the body in a + generated `^:async` fn and evaluates to a promise, so there is no `done` to + thread. See [[run-mocked]] for the restore-on-settle machinery." #?(:cljs (:require [beicon.v2.core :as rx] [cljs.test :as t])) @@ -150,6 +154,50 @@ (^:async fn [] ~@body) (fn [] ~@restore-exprs))))))) +#?(: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 + (frontend-tests.helpers.mock/run-mocked + (^:async fn [] ~@body) + (fn [] ~@restore-exprs))))))) + ;; ═══════════════════════════════════════════════════════════════ ;; Runtime (ClojureScript only) ;; ═══════════════════════════════════════════════════════════════ diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs index fd9cfe6450..5954eab457 100644 --- a/frontend/test/frontend_tests/main_errors_test.cljs +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -135,6 +135,33 @@ [events] (filter #(= ::ev/event (ptk/type %)) events)) +(defn- ^:async report-names + "Drives `flash-persistence` with an `ex-data` map of `cause-data` and + returns the audit event names it emitted, in order. + + The report generator is stubbed and the scheduler runs its callback + inline, so the whole flash body completes before returning. The emitter + is the only observable." + [cause-data] + (let [events (atom []) + cause (ex-info "Save failed" (assoc cause-data :hint "Save failed"))] + ;; The governor is global and remembers fingerprints for two minutes, so + ;; a second identical report would be suppressed. Clear it per case. + (errors/reset-report-governor!) + (await + (mock/with-mocks* + {errors/generate-report (fn [_ & _] "generated error report") + st/emit! (mock/stub (fn [& emitted] + (swap! events into emitted))) + ;; `schedule` is multi-arity, so the mock needs every arity the + ;; flash body may call it with. + tm/schedule (mock/stub (fn + ([] nil) + ([callback] (callback)) + ([_ms callback] (callback))))} + (errors/flash-persistence cause))) + (mapv #(-> % deref ::ev/name) (report-events @events)))) + ;; --------------------------------------------------------------------------- ;; Environment failures ;; @@ -145,7 +172,7 @@ (t/deftest environment-error-classification (t/testing "environment failures are recognised" (doseq [type [:network :offline :bad-gateway :service-unavailable - :nitrate-unavailable :nitrate-not-configured]] + :gateway-error :nitrate-unavailable :nitrate-not-configured]] (t/is (true? (errors/environment-error? (error-cause :type type))) (str "expected environment error: " type)))) (t/testing "application defects are not environment failures" @@ -213,7 +240,7 @@ ;; 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]] + (doseq [type [:network :offline :gateway-error]] (errors/reset-report-governor!) (let [events (atom []) cause (ex-info "http error" {:type type :hint "http error"})] @@ -820,7 +847,67 @@ ((:callback download) nil) (t/is (= [["report" "text/plain" "blob:report"]] @downloads)) (doseq [callback @idle-callbacks] (callback)) - (t/is (= ["blob:report"] @revoked))))))) + (t/is (= ["blob:report"] @revoked)) + ;; Autosave is stopped for the whole session, so the toast also + ;; offers the only action that restarts it without the user having + ;; to know they could leave the file and come back. + (let [reload (get-in state [:notification :links 1])] + (t/is (= "labels.reload-page:" (:label reload)) + "the `tr` stub renders a one-argument key with a trailing colon") + (t/is (fn? (:callback reload)) "the reload link has a callback"))))))) + +;; Scenario: a save fails terminally. The toast offers the report download +;; and a reload. Proves: the recovery action reaches the user. +;; +;; The reload callback itself is not invoked here: `js/location` does not +;; exist under the test runner, so the link is verified by its label and by +;; being a callable, not by calling it. +(t/deftest ^:async save-failure-toast-offers-a-reload-action + (let [events (atom [])] + (await + (mock/with-mocks* + {errors/generate-report (fn [_ & _] "generated error report") + st/emit! (mock/stub (fn [& emitted] + (swap! events into emitted))) + tm/schedule (mock/stub (fn [callback] (callback)))} + (errors/flash-persistence (ex-info "Save failed" + {:type :validation :hint "Save failed"})) + (await (async/settle)) + ;; The report is emitted first and the toast right after, so the toast + ;; is the last event collected. + (let [state (ptk/update (last @events) {}) + links (get-in state [:notification :links])] + ;; `tr` is not stubbed here, and the test runner does not load the + ;; catalogue, so a key comes back verbatim without its arguments. + (t/is (= ["labels.download" "labels.reload-page"] (mapv :label links)) + "the toast carries the report and a reload, download first") + (t/is (every? #(fn? (:callback %)) links) + "both actions are callable") + (t/is (nil? (get-in state [:notification :timeout])) + "the toast stays until the user acts on it")))))) + +;; Scenario: a save fails on the transport and the queue keeps trying on +;; the slow cycle. The toast says the changes are kept and offers no reload, +;; which would lose them. Proves: the retrying variant tells the truth. +(t/deftest ^:async retrying-save-failure-toast-offers-no-reload + (let [events (atom [])] + (await + (mock/with-mocks* + {errors/generate-report (fn [_ & _] "generated error report") + st/emit! (mock/stub (fn [& emitted] + (swap! events into emitted))) + tm/schedule (mock/stub (fn [callback] (callback)))} + (errors/flash-persistence (ex-info "Save failed" + {:type :gateway-error :hint "Save failed"}) + :retrying true) + (await (async/settle)) + (let [state (ptk/update (last @events) {})] + (t/is (= "errors.save-still-retrying" (get-in state [:notification :content]))) + (t/is (= ["labels.download"] (mapv :label (get-in state [:notification :links]))) + "the toast carries the report and no reload") + (t/is (= errors/persistence-failed-tag (get-in state [:notification :tag])) + "a save that lands can hide it") + (t/is (nil? (get-in state [:notification :timeout])))))))) (t/deftest ^:async persistence-waiters-do-not-report-an-already-handled-failure (let [reports (atom []) @@ -923,25 +1010,86 @@ (t/is (= [::dw/reload-current-file] (mapv ptk/type @events))) (t/is (empty? @scheduled) "A restored version shows no save 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 [])] +(t/deftest ^:async save-failure-report-type-follows-the-cause + ;; The user still gets the same sticky toast and report link; only the audit + ;; event name changes, and that is what decides whether the incident reaches + ;; the internal reports and the alerts. + (t/testing "a validation failure is a defect, so it reports unhandled" + (t/is (= ["unhandled-exception"] + (await (report-names {:type :validation :code :invalid-data}))))) + + (t/testing "the wrapped shape reads the cause, not the wrapper" + ;; This is the shape that actually reaches flash-persistence: + ;; persistence-failed sets :type :persistence and keeps the real type + ;; under :cause-type. Reading :type alone would match the wrapper and + ;; leave the cause as handled. + (t/is (= ["unhandled-exception"] + (await (report-names {:type :persistence + :cause-type :validation + :code :invalid-data}))))) + + (t/testing "environment failures stay handled after the retry budget" + ;; They land here once the backoff is spent, and the user's connection is + ;; not our bug, so they must not alert. + (doseq [cause-type [:network :offline :bad-gateway :service-unavailable + :gateway-error]] + (t/is (= ["handled-exception"] + (await (report-names {:type :persistence + :cause-type cause-type}))) + (str cause-type " is an environment failure, not a defect")))) + + (t/testing "responses the backend never wrote stay handled" + (doseq [cause-type [:rate-limit :unexpected-response]] + (t/is (= ["handled-exception"] + (await (report-names {:type :persistence + :cause-type cause-type}))) + (str cause-type " comes from a proxy, not from our code")))) + + (t/testing "an unclassified failure keeps the handled path" + (t/is (= ["handled-exception"] + (await (report-names {:type :persistence + :cause-type :unknown-thing}))))) + + (t/testing "a bare :type still classifies, without a wrapper" + (t/is (= ["unhandled-exception"] + (await (report-names {:type :validation}))))) + + (t/testing "a server-side or internal defect reports unhandled" + (doseq [cause-type [:server-error :internal]] + (t/is (= ["unhandled-exception"] + (await (report-names {:type :persistence + :cause-type cause-type}))) + (str cause-type " is our bug"))))) + +;; --------------------------------------------------------------------------- +;; transport errors +;; --------------------------------------------------------------------------- + +(t/deftest ^:async transport-errors-are-handled-without-replacing-the-page + ;; Scenario: a rate limit and an error page the backend did not write, + ;; through the global handler. Proves: neither replaces the page, each is + ;; a handled report followed by one toast. + (doseq [type [:rate-limit :unexpected-response]] + (errors/reset-report-governor!) + (let [events (atom []) + assigned (atom []) + cause (ex-info "http error" + {:type type + :hint "http error" + :status 429 + :uri "https://design.penpot.app/api/main/methods/get-teams"})] (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)) + {st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + rt/get-current-href (constantly "https://penpot.example.com/#/workspace") + rt/assign-exception (fn [error] (swap! assigned conj error) error) + tm/schedule (mock/stub (fn [f] (f))) + st/format-last-events (mock/stub (fn [& _] "(stub last events)"))} + (errors/on-error cause) (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]))))))))) + (t/is (empty? @assigned) + (str type " must not replace the page with an error screen")) + (t/is (= ["handled-exception"] + (map (comp ::ev/name deref) (report-events @events))) + (str type " is reported as handled, not unhandled")) + (t/is (= 2 (count @events)) (str type " must show one notification"))))))) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 5c10b8d23b..2c7108dbe4 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -1532,7 +1532,7 @@ msgstr "There was an error with the comment" msgid "errors.connection-error" msgstr "Cannot reach the server. Check your connection and try again." -#: src/app/main/errors.cljs:851 +#: src/app/main/errors.cljs:414 msgid "errors.deprecated" msgstr "" "Sorry! This is an old file that uses a deprecated type of Penpot assets and " @@ -1758,8 +1758,16 @@ msgstr "The registration is currently disabled." #: src/app/main/errors.cljs:538 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." +"Autosave has stopped for this session, so your changes since the last successful save cannot be saved. Download the report to help us fix it, then reload the page, or leave this file and open it again, to keep editing." + +#: src/app/main/data/persistence.cljs:310 +msgid "errors.save-retrying" +msgstr "Connection lost. Retrying to save your changes." + +#: src/app/main/errors.cljs +msgid "errors.save-still-retrying" +msgstr "" +"Your changes are not saved yet because Penpot cannot reach the server. They are kept, and Penpot will keep trying to save them. Keep this tab open until they are saved." #: src/app/main/data/persistence.cljs:307 msgid "errors.save-retrying" @@ -6628,7 +6636,7 @@ msgstr "Reset" msgid "workspace.header.retrying" msgstr "Retrying to save" -#: src/app/main/ui/workspace/left_header.cljs:126 +#: src/app/main/ui/workspace/left_header.cljs:123 msgid "workspace.header.save-error" msgstr "Error on saving" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index 4207845025..a069f620eb 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -1778,9 +1778,12 @@ msgstr "El registro está actualmente desactivado." #: src/app/main/errors.cljs:538 msgid "errors.save-failed" msgstr "" -"El autoguardado se ha detenido debido a un error. Contacta soporte para " -"informar del error y recarga para continuar desde los últimos cambios " -"guardados." +"El autoguardado se ha detenido en esta sesión, así que los cambios desde el último guardado correcto no se podrán guardar. Descarga el informe para ayudarnos a arreglarlo y recarga la página, o sal de este fichero y vuelve a abrirlo, para seguir editando." + +#: src/app/main/errors.cljs +msgid "errors.save-still-retrying" +msgstr "" +"Tus cambios aún no se han guardado porque Penpot no puede conectar con el servidor. Se conservan y Penpot seguirá intentando guardarlos. Mantén esta pestaña abierta hasta que se guarden." #: src/app/main/errors.cljs:773 msgid "errors.svg-parser.invalid-svg"