🐛 Declare the nitrate client of the revocation watcher

The params schema of the ::revocation-watcher key named
:app.nitrate/client as a bare keyword. Inside [:map ...] a bare
keyword is not a key but a schema property, so malli resolved it
as a registry lookup. Nothing had registered that keyword, and
compiling the schema raised :malli.core/invalid-schema, which took
down the whole system build instead of one assertion.

Nothing caught it because the assertion only runs when
PENPOT_FLAGS carries enable-backend-asserts, so the suite passed
with the schema broken. With the flag the backend died before a
single test ran.

The client is declared as an explicit pair, the way
auth.oidc/schema:routes-params already does. It is optional
because init-key yields nil when the :admin-console flag is off,
and a required key would fail the assert on a normal boot.

The new test compiles the schema and pins both halves of that
contract, following the pattern already used for the three worker
schemas in backend-tests.worker-test.

AI-assisted-by: space-bunny-free
This commit is contained in:
Andrey Antukh 2026-10-08 14:07:39 +00:00
parent 9880a9471d
commit b252ab493e
5 changed files with 32 additions and 3 deletions

View File

@ -32,7 +32,9 @@
- `job` is deletion-protected at the SQL level (`deletion_protection__tgr`): a stray `DELETE FROM job` raises instead of dropping the row and its `job_event` cascade without touching the referenced `storage_object`. The only intended deleters, the jobs GC and the submit dedupe, disable the guard with `SET LOCAL rules.deletion_protection TO off` inside their transaction; tests use `th/db-force-delete`.
- A missing job-def raises (`:no-job-definition`) instead of completing. Throwing with `ex-data :type ::retry` still controls retry behavior; `:strategy ::noop` retries without incrementing retry count.
- Cron entries claim their `scheduled_task` row with `FOR UPDATE SKIP LOCKED`, disable statement/idle-in-transaction timeouts locally, submit one `job` row per entry when no active instance exists (no-overlap), and reschedule themselves in `finally` unless interrupted. Worker, dispatcher, and cron components do not start when the DB pool is read-only.
- Integrant params schemas name injected components as bare `::ns/key` entries (pool, metrics, msgbus, job defs), and a bare entry is a malli registry lookup, not `any?`: the owning namespace must `sm/register!` it at load, or the backend crashes at boot with `:malli.core/invalid-schema` instead of asserting. Unit tests never build those keys, so `backend-tests.worker-test` compiles the three worker schemas to catch it.
- Integrant params schemas name injected components as bare `::ns/key` entries (pool, metrics, msgbus, job defs), and a bare entry is a malli registry lookup, not `any?`: the owning namespace must `sm/register!` it at load, or the backend crashes at boot with `:malli.core/invalid-schema` instead of asserting. Unit tests never build those keys, so `backend-tests.worker-test` compiles the three worker schemas and `backend-tests.http-websocket-test` compiles the revocation-watcher one to catch it. The assertion only runs with `PENPOT_FLAGS=enable-backend-asserts`, so without it a broken schema reaches production and only crashes at boot.
- Whether a bare entry works has nothing to do with its position or with how many of them there are: it is a registry lookup, so it works only if that exact keyword was `sm/register!`'d. `[:map ::db/pool ::metrics/metrics]` is fine, `[:map :app.aaa/unregistered :app.ccc/unregistered]` throws. When a component is **not** registered as a malli type, spell it as an explicit pair instead of registering a type just to satisfy the lookup — `[:app.nitrate/client {:optional true} [:maybe :map]]` is how `auth.oidc/schema:routes-params` and `loggers.audit.archive-task/schema:audit-log-archive-params` declare it. `{:optional true}` matters because `init-key` yields `nil` when the owning feature flag (`:admin-console`) is off, and a required key would then fail the assert on a perfectly normal boot.
- `sm/valid?` catches every `Throwable` and returns `false`, so an uncompilable schema and a genuinely invalid value both surface as a bare `Assert failed:` with no cause. When an `assert-key` fires with no message, compile the schema by hand to see the real exception.
## Config and HTTP/session middleware

View File

@ -8,7 +8,8 @@ JVM `clojure.test` (kaocha runner) under `backend/test/backend_tests/`.
- **Preferred way to run them: `scripts/ci <module> --test`**, which runs lint, tests and format per module and writes a log per task under `.ci-logs/`. It exports `PENPOT_FLAGS` with `enable-backend-asserts` appended, so use it rather than calling `clojure -M:dev:test` by hand.
- **Why the flag matters:** `clojure.core/assert` is elided at compile time when `*assert*` is false, and `app.config` only sets that var from the flags when it loads. The test fixture redefines the flags after that, so they never reach it: without the flag the suite runs with every assertion compiled out, and a test that depends on an assert only means something with it set. GitHub Actions sets it too. When running the suite directly, the command is `PENPOT_FLAGS="enable-backend-asserts" clojure -M:dev:test`.
- Coverage: if code is added or modified in `src/`, corresponding tests in `test/backend_tests/` must be added or updated.
- Isolated run: `clojure -M:dev:test --focus backend-tests.my-ns-test` for a specific test namespace, or `clojure -M:dev:test --focus backend-tests.my-ns-test/my-test-var` for a specific test var.
- Isolated run: `clojure -M:dev:test --focus backend-tests.my-ns-test` for a specific test namespace, or `clojure -M:dev:test --focus backend-tests.my-ns-test/my-test-var` for a specific test var. Keep the `PENPOT_FLAGS="enable-backend-asserts"` prefix on these too; a focused run without it hides assert-dependent failures exactly like a full one.
- A **whole-suite** crash reported as `Execution error ... Assert failed: ...` with no `FAIL in` line is not a test failure: an `ig/assert-key` ran while the test system was being built, which happens outside `clojure.test`, so kaocha reports it as a load/build error and the summary has no counts. Fix the offending schema (see `mem:backend/subtleties`) rather than hunting for a test.
- Multiple focus symbols are supported by REPEATING the flag (kaocha accumulates them, last-wins does not apply): `clojure -M:dev:test --focus backend-tests.ns-one --focus backend-tests.ns-two` — preferred over a shell loop of isolated runs.
- Regression run: `clojure -M:dev:test` to ensure no regressions in related functional areas.
- If you need to filter output, tee to a temp file first: `clojure -M:dev:test 2>&1 | tee /tmp/penpot-test-output.txt`.

View File

@ -34,6 +34,8 @@ Which mutations announce: `delete-team-member`, `leave-team`, `update-team-membe
`backend/test/backend_tests/http_websocket_test.clj` drives the real Redis-backed msgbus, so the revocation tests are end-to-end: register the connection in the real registry, run the real command, wait for the subscription to drop, then assert no content arrives. A connection built without `register-connection` is invisible to the watcher, so the watcher-based tests must go through `open-registered-connection`. Interval tests shorten the config via `with-redefs` on `cf/get` rather than waiting minutes. `with-clean-registry` runs as an `:each` fixture, so no test can leak a live relay loop into the next one. `captured-events` forwards the keyword args through to the real `mbus/pub!` — rebuilding them positionally publishes to the topic `:message` with a `nil` payload and silently untests every other notice. Interval tests take access away with a direct row delete, never with a command: a command announces, so the test would pass even with the interval removed. The resubscribe-counting test is the one that proves finished relays stop polling.
`revocation-watcher-schema-resolves-every-ref` covers the `::revocation-watcher` params schema, which no other test compiles. It existed broken for a while: the three injected keys were written as bare keywords, which is a valid malli *property* position and not a key, so the schema raised `:malli.core/invalid-schema` — invisible unless `PENPOT_FLAGS=enable-backend-asserts` was set, and a crash of the whole system build rather than one test (see `mem:backend/testing` on the flag).
## See also
- `mem:backend/auth-permissions-product-domains` for the permission predicates and the not-found semantics that the re-checks reuse.

View File

@ -530,7 +530,7 @@
[:map
::mbus/msgbus
::db/pool
:app.nitrate/client])
[:app.nitrate/client {:optional true} [:maybe :map]]])
(defmethod ig/assert-key ::revocation-watcher
[_ params]

View File

@ -6,6 +6,7 @@
(ns backend-tests.http-websocket-test
(:require
[app.common.schema :as sm]
[app.common.time :as ct]
[app.common.uuid :as uuid]
[app.config :as cf]
@ -1082,6 +1083,29 @@
(t/testing "the file subscription on the other team survives"
(t/is (some? (subscribed-file conn)))))))
;; The revocation watcher is the only integrant key here, so it is the
;; only one that compiles this schema at boot: a ref with no registered
;; schema crashes the boot with :malli.core/invalid-schema instead of
;; asserting. The test system builds the component, but assert-key only
;; runs its check with *assert* set, so pin the compilation here.
(t/deftest revocation-watcher-schema-resolves-every-ref
(t/testing "the schema compiles"
(t/is (sm/schema? (sm/schema @#'ws/schema:revocation-watcher))))
(t/testing "every ref the key injects is registered"
(t/is (= #{::db/pool ::mbus/msgbus}
(disj (set (sm/keys @#'ws/schema:revocation-watcher))
:app.nitrate/client))))
(t/testing "the nitrate client is optional, so a nil one passes"
(t/is (sm/valid? @#'ws/schema:revocation-watcher
{::db/pool (:app.db/pool th/*system*)
::mbus/msgbus (:app.msgbus/msgbus th/*system*)})))
(t/testing "a non-map nitrate client is rejected"
(t/is (not (sm/valid? @#'ws/schema:revocation-watcher
{::db/pool (:app.db/pool th/*system*)
::mbus/msgbus (:app.msgbus/msgbus th/*system*)
:app.nitrate/client "not-a-client"})))))
(t/deftest organization-owner-keeps-read-only-access
(let [{:keys [owner editor team file]} (editor-in-team! 1 2)]