From 334f727e870599538b07f758c145520f9aa875e7 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 15 Oct 2026 08:40:39 +0000 Subject: [PATCH] :bug: Stop oversized audit events from wedging the queue Port staging c74d415 (#12252) to main, adapted to this branch. The :export-shapes modal event carried the full :exports vector, each export embedding its shape, which could reach megabytes and wedge the audit archive queue past the proxy body limit. Build a compact body instead: totals, distinct shapes and per-type counts, plus the page name. Other modals keep their props. Move the modal :origin prop to ::ev/origin so make-proto-event records it as :event-origin; the UI still reads the plain prop. Bound exception messages at the source: get-hint clamps to max-message-length (1000 chars) and the CLJS format-throwable clamps the trace message line and honors :trace-length. The Context Hint header in generate-report prints get-hint instead of the raw hint. Also fix first-line on the JVM, where cuerdas/index-of returns nil and (pos? nil) throws. Adaptations to main: skip audit-log.md (absent here), skip the generate-report huge-message test (staging async helpers are absent), and fit the errors.cljs hunk to main's generate-report. AI-assisted-by: muse-spark-1.3-contributor --- .serena/memories/clojure/idioms.md | 1 + .../frontend/routing-app-shell-subtleties.md | 3 +- common/src/app/common/exceptions.cljc | 39 ++++++++++--- common/test/common_tests/exceptions_test.cljc | 57 +++++++++++++++++++ common/test/common_tests/runner.cljc | 4 +- frontend/src/app/main/data/modal.cljs | 27 ++++++++- frontend/src/app/main/errors.cljs | 2 +- .../test/frontend_tests/data/modal_test.cljs | 55 ++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + 9 files changed, 176 insertions(+), 14 deletions(-) create mode 100644 common/test/common_tests/exceptions_test.cljc create mode 100644 frontend/test/frontend_tests/data/modal_test.cljs diff --git a/.serena/memories/clojure/idioms.md b/.serena/memories/clojure/idioms.md index 26db315487..1345fd50a6 100644 --- a/.serena/memories/clojure/idioms.md +++ b/.serena/memories/clojure/idioms.md @@ -18,3 +18,4 @@ assumption; a wrong assumption here already cost a review round. - `^:async` fns never throw synchronously (rejected promises instead); `try/catch/finally` supported; continuations are microtasks, timers and RxJS schedulers are macrotasks (FIFO). +- `cuerdas.core/index-of` returns `nil` (not `-1`) when the substring is absent, like `clojure.string/index-of`. `(pos? nil)` **throws on the JVM** but evaluates to `false` in CLJS (JS coercion), so `(if (pos? (str/index-of s "\n")) …)` works in the browser and crashes the backend. Guard with `some?` before `pos?` (this was `app.common.exceptions/first-line`). diff --git a/.serena/memories/frontend/routing-app-shell-subtleties.md b/.serena/memories/frontend/routing-app-shell-subtleties.md index d62690ec72..7181144a9f 100644 --- a/.serena/memories/frontend/routing-app-shell-subtleties.md +++ b/.serena/memories/frontend/routing-app-shell-subtleties.md @@ -16,10 +16,11 @@ - `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/common/src/app/common/exceptions.cljc b/common/src/app/common/exceptions.cljc index 588391601a..7bb36f2253 100644 --- a/common/src/app/common/exceptions.cljc +++ b/common/src/app/common/exceptions.cljc @@ -29,6 +29,22 @@ (def ^:dynamic *data-length* 8) (def ^:dynamic *data-level* 8) +(def max-message-length + "Upper bound, in characters, for an exception message surfaced as a hint or + embedded in a formatted stack trace. A message can carry a whole payload (a + serialized file, for instance) and grow to megabytes; only the head helps to + identify the failure, and the rest is noise that bloats reports and the audit + events built from them." + 1000) + +(defn- truncate + "Truncates a string to at most `n` characters, appending an ellipsis when it + had to cut. Values that are not strings pass through unchanged." + [s n] + (if (and (string? s) (> (count s) n)) + (str (subs s 0 n) "...") + s)) + (defmacro error [& {:keys [type hint] :as params}] `(ex-info ~(or hint (name type)) @@ -243,7 +259,7 @@ :cljs (defn format-throwable - [cause & {:as opts}] + [cause & {:keys [trace-length]}] (with-out-str (when-let [exdata (ex-data cause)] (when-let [hint (or (get exdata :hint) @@ -251,7 +267,7 @@ (when (str/index-of hint "\n") (println "Hint:") (println "--------------------") - (println hint) + (println (truncate hint max-message-length)) (println))) (when-let [explain (get exdata ::sm/explain)] @@ -272,14 +288,20 @@ (println)) (when-let [trace (.-stack cause)] - (println "Trace:") - (println "--------------------") - (println (.-stack cause)))))) + (let [lines (str/lines trace) + head (truncate (first lines) max-message-length) + tail (rest lines)] + (println "Trace:") + (println "--------------------") + (when head + (println head)) + (doseq [line (if trace-length (take trace-length tail) tail)] + (println line))))))) (defn first-line [s] (let [break-index (str/index-of s "\n")] - (if (pos? break-index) + (if (and (some? break-index) (pos? break-index)) (subs s 0 break-index) s))) @@ -308,5 +330,6 @@ (defn get-hint [cause] - (or (some-> (ex-data cause) (get :hint) first-line) - (some-> (ex-message cause) first-line))) + (truncate (or (some-> (ex-data cause) (get :hint) first-line) + (some-> (ex-message cause) first-line)) + max-message-length)) diff --git a/common/test/common_tests/exceptions_test.cljc b/common/test/common_tests/exceptions_test.cljc new file mode 100644 index 0000000000..6f9df8da60 --- /dev/null +++ b/common/test/common_tests/exceptions_test.cljc @@ -0,0 +1,57 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns common-tests.exceptions-test + "Exception message bounding in `app.common.exceptions`. + + A message can carry a whole payload (a serialized file, for instance) and + grow to megabytes. `get-hint` and the CLJS `format-throwable` keep a bounded + head so neither the audit `:hint` prop nor the report can explode." + (:require + #?(:cljs [cuerdas.core :as str]) + [app.common.exceptions :as ex] + [clojure.test :as t])) + +(defn- huge-string + [n] + (apply str (repeat n "x"))) + +(t/deftest first-line-keeps-a-single-line-message-intact-test + (t/is (= "boom" (ex/first-line "boom"))) + (t/is (= "boom" (ex/first-line "boom\nrest of the message")))) + +(t/deftest get-hint-returns-the-first-line-test + (t/is (= "boom" (ex/get-hint (ex-info "boom\nsecond line" {}))))) + +(t/deftest get-hint-prefers-the-ex-data-hint-test + (t/is (= "from-data" (ex/get-hint (ex-info "from-message" {:hint "from-data"}))))) + +(t/deftest get-hint-bounds-a-huge-message-test + (let [huge (huge-string (* 4 ex/max-message-length)) + hint (ex/get-hint (ex-info huge {}))] + (t/is (string? hint)) + (t/is (<= (count hint) (+ ex/max-message-length 3))) + (t/is (= (subs hint 0 32) (subs huge 0 32))))) + +(t/deftest get-hint-bounds-a-huge-ex-data-hint-test + (let [huge (huge-string (* 4 ex/max-message-length)) + hint (ex/get-hint (ex-info "short" {:hint huge}))] + (t/is (<= (count hint) (+ ex/max-message-length 3))) + (t/is (= (subs hint 0 32) (subs huge 0 32))))) + +#?(:cljs + (t/deftest format-throwable-bounds-a-huge-trace-message-test + (let [huge (huge-string (* 4 ex/max-message-length)) + out (ex/format-throwable (ex-info huge {})) + msg (some #(when (str/includes? % (subs huge 0 16)) %) (str/lines out))] + (t/is (some? msg)) + (t/is (<= (count msg) (+ ex/max-message-length 16)))))) + +#?(:cljs + (t/deftest format-throwable-honors-trace-length-test + (let [full (ex/format-throwable (ex-info "boom" {})) + short (ex/format-throwable (ex-info "boom" {}) {:trace-length 1})] + (t/is (< (count (str/lines short)) (count (str/lines full))))))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 1b55f1d1e4..1a0ba1217d 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -16,6 +16,7 @@ [common-tests.buffer-test] [common-tests.colors-test] [common-tests.data-test] + [common-tests.exceptions-test] [common-tests.files-builder-test] [common-tests.files-changes-test] [common-tests.files-migrations-0025-test] @@ -96,8 +97,9 @@ 'common-tests.buffer-test 'common-tests.colors-test 'common-tests.data-test - 'common-tests.files-changes-test + 'common-tests.exceptions-test 'common-tests.files-builder-test + 'common-tests.files-changes-test 'common-tests.files-migrations-0025-test 'common-tests.files-migrations-0026-test 'common-tests.files-migrations-test diff --git a/frontend/src/app/main/data/modal.cljs b/frontend/src/app/main/data/modal.cljs index 052080f3fb..c6351e1a8d 100644 --- a/frontend/src/app/main/data/modal.cljs +++ b/frontend/src/app/main/data/modal.cljs @@ -17,6 +17,19 @@ ;; TODO: rename `:type` to `:name` +(defn- summarize-export-shapes + "Builds a compact body for the `:export-shapes` modal event. We know the + shape of these props here, so instead of dumping every export (each one + carrying its full shape, which can be megabytes) we emit counters: total + exports, distinct shapes and one count per export type." + [props] + (let [exports (:exports props)] + (merge + {:page (:name props) + :num-shapes (count (distinct (map :object-id exports))) + :num-exports (count exports)} + (frequencies (map :type exports))))) + (defn show ([props] (show (uuid/next) (:type props) props)) @@ -26,9 +39,17 @@ (ptk/reify ::show-modal ev/Event (-data [_] - (-> props - (dissoc :type) - (assoc :name type))) + (let [origin (:origin props) + data (if (= type :export-shapes) + (assoc (summarize-export-shapes props) :name type) + (-> props + (dissoc :type) + (assoc :name type)))] + ;; The origin is event metadata, not a prop: sending it as + ;; ::ev/origin makes `make-proto-event` put it on the event context + ;; (`:event-origin`); as a plain `:origin` it only stayed as a prop. + (cond-> (dissoc data :origin) + (some? origin) (assoc ::ev/origin origin)))) ptk/UpdateEvent (update [_ state] diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index 28e6e54a9b..5be508739a 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -157,7 +157,7 @@ (println "Context:") (println "--------------------") (println "Timestamp:" (ct/format-inst (ct/now) :rfc1123)) - (println "Hint: " (or (:hint data) (ex-message cause) "--")) + (println "Hint: " (or (ex/get-hint cause) "--")) (println "Prof ID: " (str (or profile-id "--"))) (println "Team ID: " (str (or team-id "--"))) (when-let [file-id (or (:file-id data) file-id)] diff --git a/frontend/test/frontend_tests/data/modal_test.cljs b/frontend/test/frontend_tests/data/modal_test.cljs new file mode 100644 index 0000000000..e3b0fd4325 --- /dev/null +++ b/frontend/test/frontend_tests/data/modal_test.cljs @@ -0,0 +1,55 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.data.modal-test + (:require + [app.common.uuid :as uuid] + [app.main.data.event :as ev] + [app.main.data.modal :as modal] + [cljs.test :as t :include-macros true])) + +(t/deftest export-shapes-modal-event-carries-a-summary + (let [shape-a (uuid/next) + shape-b (uuid/next) + exports [{:object-id shape-a :type :png} + {:object-id shape-a :type :svg} + {:object-id shape-b :type :png}] + event (modal/show :export-shapes + {:exports exports + :origin "workspace:sidebar" + :name "Page 1"}) + data (ev/-data event)] + + (t/testing "the raw exports never leak into the event" + (t/is (not (contains? data :exports)))) + + (t/testing "the event carries counters instead" + (t/is (= 3 (:num-exports data))) + (t/is (= 2 (:num-shapes data))) + (t/is (= 2 (:png data))) + (t/is (= 1 (:svg data)))) + + (t/testing "the modal context is preserved" + (t/is (= "Page 1" (:page data))) + (t/is (= :export-shapes (:name data)))) + + (t/testing "origin is emitted as event metadata, not as a prop" + (t/is (= "workspace:sidebar" (::ev/origin data))) + (t/is (not (contains? data :origin)))))) + +(t/deftest other-modals-keep-their-props + (let [event (modal/show :some-modal {:foo "bar" :items [1 2 3]}) + data (ev/-data event)] + (t/is (= "bar" (:foo data))) + (t/is (= [1 2 3] (:items data))) + (t/is (= :some-modal (:name data))))) + +(t/deftest modal-origin-becomes-event-metadata + (let [event (modal/show :some-modal {:foo "bar" :origin "dashboard"}) + data (ev/-data event)] + (t/is (= "dashboard" (::ev/origin data))) + (t/is (not (contains? data :origin))) + (t/is (= "bar" (:foo data))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 8423e94fbd..e7a5859879 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -11,6 +11,7 @@ [frontend-tests.data.dashboard-test] [frontend-tests.data.exports-assets-test] [frontend-tests.data.fix-deleted-fonts-test] + [frontend-tests.data.modal-test] [frontend-tests.data.nitrate-test] [frontend-tests.data.persistence-retry-test] [frontend-tests.data.persistence-test] @@ -117,6 +118,7 @@ 'frontend-tests.copy-as-svg-test 'frontend-tests.data.dashboard-test 'frontend-tests.data.fix-deleted-fonts-test + 'frontend-tests.data.modal-test 'frontend-tests.data.nitrate-test 'frontend-tests.data.persistence-retry-test 'frontend-tests.data.persistence-test