Open
Conversation
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR introduces a complete rate-limiting system for InstantDB, including CEL expression support for rate-limit rules, a Hazelcast-backed persistence layer with PostgreSQL storage, HMAC-based bucket key hashing, periodic cleanup of stale entries, and a client-side testing playground. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CEL as CEL Evaluator
participant RateLimitBkt as RateLimitBucket
participant Hazelcast
participant Postgres
participant Sweep as Periodic Sweep
Client->>CEL: Evaluate permission rule with rateLimit.<config>.limit(key)
CEL->>RateLimitBkt: Create RateLimitBucket for app/config
RateLimitBkt->>Hazelcast: Request bucket for bucket-key (HMAC-hashed)<br/>via get-bucket-with-config
Hazelcast->>Postgres: Load existing rate-limit-keys row<br/>(via MapStore.load)
Postgres-->>Hazelcast: Return key row or null
Hazelcast-->>RateLimitBkt: Return bucket (in-memory or<br/>reconstructed from Postgres)
RateLimitBkt->>RateLimitBkt: tryLimit(): consume tokens<br/>or throw exception
RateLimitBkt->>Hazelcast: Update bucket state
Hazelcast->>Postgres: Persist bucket to rate_limit_keys<br/>(async MapStore.store)
Postgres-->>Hazelcast: ACK
Hazelcast-->>CEL: Success or RateLimitedException
CEL-->>Client: Return permission result<br/>or rate-limited error with retry-after
par Async Cleanup
Sweep->>Postgres: Periodic: DELETE stale rate_limit_keys<br/>where updated_at < now - interval<br/>(skip-locked, bounded retries)
Postgres-->>Sweep: Rows deleted
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
View Vercel preview at instant-www-js-rate-limit-jsv.vercel.app. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
server/resources/migrations/104_rate_limit_keys.down.sql (1)
1-1: Consider making the down migration idempotent.Using
DROP TABLE IF EXISTSmakes rollback safer in partial/repair scenarios.🧱 Suggested change
-drop table rate_limit_keys; +drop table if exists rate_limit_keys;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/resources/migrations/104_rate_limit_keys.down.sql` at line 1, The down migration uses a hard DROP TABLE which can fail if the table is already absent; update the statement in 104_rate_limit_keys.down.sql so the rollback is idempotent by using a conditional drop (e.g., change the DROP TABLE for rate_limit_keys to use IF EXISTS) so running the down migration repeatedly or during partial/repair scenarios won't error.client/sandbox/react-nextjs/pages/play/rate-limit.tsx (1)
141-147: Add a fallback whene.messageis missing informatError.Line 146 can render
undefinedfor unknown error shapes; fallback text would keep the output readable.💡 Suggested tweak
function formatError(label: string, e: any): string { const retryAfter = e?.hint?.['retry-after']; if (retryAfter != null) { return `${label}: Try again in ${retryAfter} seconds`; } - return `${label}: ${e.message}`; + return `${label}: ${e?.message ?? 'Unknown error'}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/sandbox/react-nextjs/pages/play/rate-limit.tsx` around lines 141 - 147, The formatError function can return "undefined" when e.message is absent; update formatError to use a safe fallback for the error text (e.g., use e?.message, then String(e) or a literal like "Unknown error" when that is missing) so the returned string is always readable; modify the body of formatError (the function named formatError and its usage of e.message) to prefer e?.message ?? String(e) ?? 'Unknown error' (or equivalent) before interpolating into the returned message.server/src/instant/rate_limit.clj (1)
83-88: Inconsistent metric name instoreAll.The
storeAllmethod uses::storeas the metric/trace identifier instead of::storeAll, which could cause confusion when analyzing metrics or traces.♻️ Proposed fix
(storeAll [_ m] - (sql/do-execute! ::store + (sql/do-execute! ::storeAll (aurora/conn-pool :write) ["insert into rate_limit_keys (key, value) values (unnest(?::bytea[]), unnest(?::bytea[])) on conflict (key) do update set value = excluded.value" (with-meta (keys m) {:pgtype "bytea[]"}) (with-meta (vals m) {:pgtype "bytea[]"})]))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/rate_limit.clj` around lines 83 - 88, The metric/trace identifier passed to sql/do-execute! inside the storeAll method is incorrectly ::store; update that call so the identifier matches the method by using ::storeAll (i.e., replace ::store with ::storeAll in the sql/do-execute! invocation within the storeAll implementation) so metrics/traces correctly reflect the storeAll operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/www/components/dash/Perms.tsx`:
- Line 276: The rendered fallback string contains a leading space: change the
expression in the JSX from using the fallback string ' the rules' to 'the rules'
so the message in the Perms component ("There was an error in
{errorRes?.in?.join('->') || ' the rules'}") no longer produces "in the rules";
update the fallback to 'the rules' inside the same JSX expression where
errorRes?.in?.join('->') is used.
In `@server/src/instant/model/rule.clj`:
- Line 433: The symbol tool/def-locals is undefined and will cause a runtime
compiler error; remove this debug artifact or replace it with a proper require
of the tool namespace if that macro is actually needed. Locate the stray call to
tool/def-locals in the rule.clj namespace (the lone (tool/def-locals) line) and
either delete it or add the corresponding :require for the tool namespace and
confirm the intended symbol name; prefer removing the line if it was a debug
leftover.
In `@server/src/instant/util/exception.clj`:
- Around line 415-420: The :retry-after computation in
throw-permission-rate-limited! can be negative when retry-at is slightly past;
change the calculation to clamp to a non-negative integer by computing the
seconds as (int (max 0 (/ (.toMillis (Duration/between (Instant/now) retry-at))
1000))) or equivalent so :retry-after is always >= 0 while leaving ::hint
:retry-at unchanged; update the expression that builds ::hint in
throw-permission-rate-limited! to use this clamped value.
---
Nitpick comments:
In `@client/sandbox/react-nextjs/pages/play/rate-limit.tsx`:
- Around line 141-147: The formatError function can return "undefined" when
e.message is absent; update formatError to use a safe fallback for the error
text (e.g., use e?.message, then String(e) or a literal like "Unknown error"
when that is missing) so the returned string is always readable; modify the body
of formatError (the function named formatError and its usage of e.message) to
prefer e?.message ?? String(e) ?? 'Unknown error' (or equivalent) before
interpolating into the returned message.
In `@server/resources/migrations/104_rate_limit_keys.down.sql`:
- Line 1: The down migration uses a hard DROP TABLE which can fail if the table
is already absent; update the statement in 104_rate_limit_keys.down.sql so the
rollback is idempotent by using a conditional drop (e.g., change the DROP TABLE
for rate_limit_keys to use IF EXISTS) so running the down migration repeatedly
or during partial/repair scenarios won't error.
In `@server/src/instant/rate_limit.clj`:
- Around line 83-88: The metric/trace identifier passed to sql/do-execute!
inside the storeAll method is incorrectly ::store; update that call so the
identifier matches the method by using ::storeAll (i.e., replace ::store with
::storeAll in the sql/do-execute! invocation within the storeAll implementation)
so metrics/traces correctly reflect the storeAll operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 23d12345-97f6-4ec6-979b-613e65f81d7c
📒 Files selected for processing (12)
client/sandbox/react-nextjs/pages/play/rate-limit.tsxclient/www/components/dash/Perms.tsxserver/resources/migrations/104_rate_limit_keys.down.sqlserver/resources/migrations/104_rate_limit_keys.up.sqlserver/src/instant/core.cljserver/src/instant/db/cel.cljserver/src/instant/db/instaql.cljserver/src/instant/model/rule.cljserver/src/instant/rate_limit.cljserver/src/instant/util/exception.cljserver/test/instant/model/rule_test.cljserver/test/instant/util/hazelcast_test.clj
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/src/instant/util/exception.clj (1)
419-422:⚠️ Potential issue | 🟡 MinorUse ceiling division to avoid premature retries.
The current implementation truncates sub-second delays, so a 500ms wait becomes
:retry-after 0. This could cause clients to retry immediately and still be rate-limited. Rounding up ensures clients wait at least until the bucket refills.🛠️ Proposed fix
:retry-after (max 0 - (int (/ (.toMillis (Duration/between (Instant/now) - retry-at)) - 1000))) + (int (Math/ceil (/ (.toMillis (Duration/between (Instant/now) + retry-at)) + 1000.0))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/util/exception.clj` around lines 419 - 422, The :retry-after calculation currently truncates sub-second waits by using integer division on (.toMillis (Duration/between (Instant/now) retry-at)), causing 500ms to become 0; update the calculation in the retry-after expression to perform ceiling division (round up) instead of truncation so any non-zero millisecond delay yields at least 1 second. Locate the expression using (:retry-after (max 0 (int (/ (.toMillis (Duration/between (Instant/now) retry-at)) 1000)))) and replace the division with a ceiling approach (e.g., divide by 1000.0 and Math/ceil, or add 999 before integer division) so the computed value is rounded up to the next second and still clamped by max 0.
🧹 Nitpick comments (1)
server/src/instant/model/rule.clj (1)
373-374: Consider removing or clarifying the XXX comment.The comment states "Here is where we need to validate the rules are available" but the validation is actually being performed on the very next line by passing
rulestocel/validation-errors(which feeds into therate-limit-validator). This leftover development note may cause confusion.♻️ Proposed fix
;; create the program to see if it throws _program (cel/->program ast) - ;; XXX: Here is where we need to validate the rules are available errors (cel/validation-errors rules compiler ast)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/model/rule.clj` around lines 373 - 374, Remove or clarify the leftover "XXX" dev note: delete the comment ";; XXX: Here is where we need to validate the rules are available" or replace it with a succinct, accurate comment that describes what the next line does (e.g., "Validate provided rules with cel/validation-errors before rate-limit-validator"). Ensure the edit is made adjacent to the errors binding where cel/validation-errors is invoked so the comment no longer misleads future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/src/instant/util/exception.clj`:
- Around line 419-422: The :retry-after calculation currently truncates
sub-second waits by using integer division on (.toMillis (Duration/between
(Instant/now) retry-at)), causing 500ms to become 0; update the calculation in
the retry-after expression to perform ceiling division (round up) instead of
truncation so any non-zero millisecond delay yields at least 1 second. Locate
the expression using (:retry-after (max 0 (int (/ (.toMillis (Duration/between
(Instant/now) retry-at)) 1000)))) and replace the division with a ceiling
approach (e.g., divide by 1000.0 and Math/ceil, or add 999 before integer
division) so the computed value is rounded up to the next second and still
clamped by max 0.
---
Nitpick comments:
In `@server/src/instant/model/rule.clj`:
- Around line 373-374: Remove or clarify the leftover "XXX" dev note: delete the
comment ";; XXX: Here is where we need to validate the rules are available" or
replace it with a succinct, accurate comment that describes what the next line
does (e.g., "Validate provided rules with cel/validation-errors before
rate-limit-validator"). Ensure the edit is made adjacent to the errors binding
where cel/validation-errors is invoked so the comment no longer misleads future
readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 88d7ae6c-639f-451b-a5c5-294e623b350b
📒 Files selected for processing (3)
client/www/components/dash/Perms.tsxserver/src/instant/model/rule.cljserver/src/instant/util/exception.clj
✅ Files skipped from review due to trivial changes (1)
- client/www/components/dash/Perms.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/instant/util/hazelcast.clj (1)
337-345:⚠️ Potential issue | 🟡 MinorConsider adding error handling for unknown Hazelcast type IDs, or verify old serialized data has been cleaned up.
The magic-code serializers (type IDs 2 and 8) are already removed from
serializer-configs. While the backward-compatibility contract (lines 17–30) is well documented, there is no defensive error handling if old persisted data in the MapStore or distributed cache contains those type IDs—deserialization will fail withSerializationException. Either restore the old serializers with graceful degradation, or confirm through MapStore/persistence inspection that no old data remains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/util/hazelcast.clj` around lines 337 - 345, serializer-configs was stripped of the old "magic-code" serializers (type IDs 2 and 8) which causes deserialization to throw SerializationException if persisted maps still contain those type IDs; update the code to defensively handle unknown type IDs by either re-registering lightweight backward-compatible serializers for IDs 2 and 8 that map old payloads to a safe fallback type, or add a startup validation that scans MapStore/distributed maps for entries with type IDs 2/8 and fails fast with a clear remediation message; locate the config list named serializer-configs and the serialization registration code that originally mapped type IDs 2 and 8 to implement one of these fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/resources/config/staging.edn`:
- Around line 35-43: The EDN map is malformed and contains duplicate keys and
extra braces; fix the map by removing the stray closing brace that prematurely
ends the map, delete the duplicate :instant-config-app-id entry so the key
appears only once, ensure only one trailing closing brace remains to balance the
opening brace, and keep :database-url, :rate-limit-hmac-key and
:database-cluster-id entries intact (verify the single :instant-config-app-id
value is the intended one).
---
Outside diff comments:
In `@server/src/instant/util/hazelcast.clj`:
- Around line 337-345: serializer-configs was stripped of the old "magic-code"
serializers (type IDs 2 and 8) which causes deserialization to throw
SerializationException if persisted maps still contain those type IDs; update
the code to defensively handle unknown type IDs by either re-registering
lightweight backward-compatible serializers for IDs 2 and 8 that map old
payloads to a safe fallback type, or add a startup validation that scans
MapStore/distributed maps for entries with type IDs 2/8 and fails fast with a
clear remediation message; locate the config list named serializer-configs and
the serialization registration code that originally mapped type IDs 2 and 8 to
implement one of these fixes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb3cce3f-312e-4afd-a543-13a44445344e
📒 Files selected for processing (10)
server/resources/config/dev.ednserver/resources/config/prod.ednserver/resources/config/staging.ednserver/resources/migrations/104_rate_limit_keys.up.sqlserver/src/instant/config.cljserver/src/instant/config_edn.cljserver/src/instant/rate_limit.cljserver/src/instant/util/exception.cljserver/src/instant/util/hazelcast.cljserver/test/instant/util/hazelcast_test.clj
💤 Files with no reviewable changes (1)
- server/test/instant/util/hazelcast_test.clj
✅ Files skipped from review due to trivial changes (2)
- server/resources/config/prod.edn
- server/resources/migrations/104_rate_limit_keys.up.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/instant/util/exception.clj
- server/src/instant/rate_limit.clj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/db/cel.clj`:
- Around line 1450-1458: The member-overload "limit" with overload-id
"_rateLimit_limit_tokens" currently uses swap! with update-in and returns the
atom state (breaking the declared BOOL return and corrupting the :rate-limits
accumulator); update the impl for [_rateLimit_limit_tokens] to perform a numeric
add against the bucket key using swap! with update (not update-in with a map
argument), and ensure the function returns true (a boolean) after updating
*where-rate-limits*; reference the impl fn for member-overload "limit" and the
atom *where-rate-limits* when making this change.
- Around line 553-559: create-rate-limit-obj currently calls reduce-kv on
(get-in ctx [:rules :code "$rateLimits"]) without guarding for a missing key,
causing exceptions when "$rateLimits" is absent; update create-rate-limit-obj to
default that lookup to an empty map before calling reduce-kv (e.g., use a safe
get-in or or-default pattern) so reduce-kv always receives a map, and apply the
same guarding pattern to the similar use in rate-limit-validation-errors
(referencing the function name rate-limit-validation-errors in
server/src/instant/model/rule.clj) to prevent crashes for apps missing the
"$rateLimits" key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ad8f2582-dce4-4de2-ae50-2e8dda087b14
📒 Files selected for processing (5)
server/resources/config/staging.ednserver/src/instant/db/cel.cljserver/src/instant/db/instaql.cljserver/src/instant/rate_limit.cljserver/src/instant/util/exception.clj
🚧 Files skipped from review as they are similar to previous changes (3)
- server/resources/config/staging.edn
- server/src/instant/db/instaql.clj
- server/src/instant/rate_limit.clj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/src/instant/db/cel.clj (1)
1450-1458:⚠️ Potential issue | 🔴 CriticalMissing
truereturn in tokens overload breaks BOOL contract.The
_rateLimit_limit_tokenswhere-overload declarescel-return-type SimpleType/BOOLbutswap!returns the updated atom state (a map), not a boolean. Compare with the single-token overload (line 1449) which explicitly returnstrue.This will cause type mismatches or unexpected behavior when rules use
rateLimit.*.limit(subject, tokens)in where-clauses.Proposed fix
(member-overload "limit" [{:overload-id "_rateLimit_limit_tokens" :cel-args [rate-limit-bucket-cel-type SimpleType/DYN SimpleType/INT] :cel-return-type SimpleType/BOOL :java-args [RateLimitBucket Object Long] :impl (fn [[^RateLimitBucket b ^Object k ^Long tokens]] (swap! *where-rate-limits* update {:bucket b - :bucket-key k} (fnil + 0) tokens))}])]) + :bucket-key k} (fnil + 0) tokens) + true)}])])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/cel.clj` around lines 1450 - 1458, The tokens overload _rateLimit_limit_tokens currently returns the result of swap! (the updated *where-rate-limits* map) which violates its declared cel-return-type SimpleType/BOOL; update the impl for the overload with overload-id "_rateLimit_limit_tokens" (the fn implementing RateLimitBucket/Object/Long) so it performs the swap! exactly as it does now but then returns true (boolean) instead of the swap! result—i.e., call swap! to update *where-rate-limits* and ensure the final expression / return value of the impl function is true to satisfy the BOOL contract.
🧹 Nitpick comments (2)
server/test/instant/db/instaql_test.clj (1)
5147-5157: Assert the full page size on the successful reads.
seqwill still pass if these queries return only one row, so a partial-result/token-accounting regression could slip through. Since both calls request:limit 2, assertcountexplicitly here.Suggested tightening
(testing "first query succeeds" - (is (seq (:users (pretty-perm-q - {:app-id app-id - :current-user {:id user-id}} - {:users {:$ {:limit 2}}}))))) + (is (= 2 + (count (:users (pretty-perm-q + {:app-id app-id + :current-user {:id user-id}} + {:users {:$ {:limit 2}}})))))) (testing "second query succeeds" - (is (seq (:users (pretty-perm-q - {:app-id app-id - :current-user {:id user-id}} - {:users {:$ {:limit 2}}}))))) + (is (= 2 + (count (:users (pretty-perm-q + {:app-id app-id + :current-user {:id user-id}} + {:users {:$ {:limit 2}}}))))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/instant/db/instaql_test.clj` around lines 5147 - 5157, The tests "first query succeeds" and "second query succeeds" currently assert non-empty results with seq, which can hide partial results; update both assertions to check the returned :users vector length equals 2 by using count on the result of pretty-perm-q for the given {:app-id app-id :current-user {:id user-id}} and {:users {:$ {:limit 2}}} inputs (look for the pretty-perm-q calls in those two testing blocks) so the tests explicitly assert full page size rather than just non-empty.server/test/instant/rate_limit_test.clj (1)
54-105: Tighten invalid-config assertions to expected error shape.Lines 55-105 currently assert any
Exception. Consider asserting the specific Instant validation error type/message so these tests fail only for real contract regressions, not unrelated runtime exceptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/instant/rate_limit_test.clj` around lines 54 - 105, Replace the broad (thrown? Exception ...) assertions in tests referencing rules-rate-limit-config->limit-config and rules-rate-limit-config->bucket-config with targeted thrown-with-msg? (or thrown?) checks that assert the specific exception class and validation message (e.g. IllegalArgumentException or the concrete Instant validation exception) and a regex matching the expected error text (like "invalid.*capacity", "period.*too (short|long)", or "Instant"). Update each failing-case line to use thrown-with-msg? with a descriptive regex so tests only pass when the expected Instant/validation error is raised by rules-rate-limit-config->limit-config and rules-rate-limit-config->bucket-config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/test/instant/db/transaction_test.clj`:
- Around line 5893-5895: The test currently builds instance and cluster names
using a 6-digit random integer bound to id, which can collide; replace the
numeric id with a UUID-based string to ensure collision-resistance. In the let
that defines id and the map keys :instance-name and :cluster-name, generate a
UUID (e.g. via java.util.UUID/randomUUID) and append it to "test-instance-" and
"test-cluster-" so each test name is unique and not prone to reuse of an
existing Hazelcast instance.
---
Duplicate comments:
In `@server/src/instant/db/cel.clj`:
- Around line 1450-1458: The tokens overload _rateLimit_limit_tokens currently
returns the result of swap! (the updated *where-rate-limits* map) which violates
its declared cel-return-type SimpleType/BOOL; update the impl for the overload
with overload-id "_rateLimit_limit_tokens" (the fn implementing
RateLimitBucket/Object/Long) so it performs the swap! exactly as it does now but
then returns true (boolean) instead of the swap! result—i.e., call swap! to
update *where-rate-limits* and ensure the final expression / return value of the
impl function is true to satisfy the BOOL contract.
---
Nitpick comments:
In `@server/test/instant/db/instaql_test.clj`:
- Around line 5147-5157: The tests "first query succeeds" and "second query
succeeds" currently assert non-empty results with seq, which can hide partial
results; update both assertions to check the returned :users vector length
equals 2 by using count on the result of pretty-perm-q for the given {:app-id
app-id :current-user {:id user-id}} and {:users {:$ {:limit 2}}} inputs (look
for the pretty-perm-q calls in those two testing blocks) so the tests explicitly
assert full page size rather than just non-empty.
In `@server/test/instant/rate_limit_test.clj`:
- Around line 54-105: Replace the broad (thrown? Exception ...) assertions in
tests referencing rules-rate-limit-config->limit-config and
rules-rate-limit-config->bucket-config with targeted thrown-with-msg? (or
thrown?) checks that assert the specific exception class and validation message
(e.g. IllegalArgumentException or the concrete Instant validation exception) and
a regex matching the expected error text (like "invalid.*capacity", "period.*too
(short|long)", or "Instant"). Update each failing-case line to use
thrown-with-msg? with a descriptive regex so tests only pass when the expected
Instant/validation error is raised by rules-rate-limit-config->limit-config and
rules-rate-limit-config->bucket-config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 22235ae1-3a6c-4f11-a753-7e6bca9c4dff
📒 Files selected for processing (11)
client/packages/core/src/rulesTypes.tsclient/www/lib/permsJsonSchema.tsxserver/resources/config/staging.ednserver/src/instant/db/cel.cljserver/src/instant/db/instaql.cljserver/src/instant/rate_limit.cljserver/src/instant/util/exception.cljserver/test/instant/db/instaql_test.cljserver/test/instant/db/transaction_test.cljserver/test/instant/model/rule_test.cljserver/test/instant/rate_limit_test.clj
✅ Files skipped from review due to trivial changes (1)
- server/src/instant/rate_limit.clj
🚧 Files skipped from review as they are similar to previous changes (3)
- server/test/instant/model/rule_test.clj
- server/src/instant/db/instaql.clj
- server/src/instant/util/exception.clj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/www/lib/permsJsonSchema.tsx (1)
141-163:⚠️ Potential issue | 🔴 Critical
$rateLimitsis still matched as a generic namespace.JSON Schema applies both
properties.$rateLimitsandpatternPropertiesto the same key. Since'$rateLimits'matches'^[$a-zA-Z0-9_\\-]+$', valid rate-limit configs are also validated as namespace rule blocks and get rejected because keys likecreateThingsare treated as forbidden properties.Suggested fix
return { type: 'object', properties, patternProperties: { - '^[$a-zA-Z0-9_\\-]+$': genericNamespaceRuleBlock, + '^(?!\\$rateLimits$)[$a-zA-Z0-9_\\-]+$': genericNamespaceRuleBlock, }, additionalProperties: false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/www/lib/permsJsonSchema.tsx` around lines 141 - 163, The patternProperties currently matches keys like '$rateLimits' causing JSON Schema to apply both properties.$rateLimits and patternProperties to the same key; update the pattern in patternProperties (where genericNamespaceRuleBlock is used) to exclude the literal '$rateLimits' (e.g. change '^[$a-zA-Z0-9_\\-]+$' to a negative-lookahead variant like '^(?!\\$rateLimits$)[$a-zA-Z0-9_\\-]+$') so that the explicit properties.$rateLimits schema is the only one applied to that key.
♻️ Duplicate comments (1)
server/src/instant/db/cel.clj (1)
1451-1458:⚠️ Potential issue | 🔴 CriticalReturn a boolean from the token-consuming where overload.
This overload still returns the
swap!result, while the declared CEL return type isBOOLand the one-token overload returnstrue. That letsrateLimit.<bucket>.limit(key, tokens)feed a map into the where-evaluation path instead of a boolean.🐛 Proposed fix
(member-overload "limit" [{:overload-id "_rateLimit_limit_tokens" :cel-args [rate-limit-bucket-cel-type SimpleType/DYN SimpleType/INT] :cel-return-type SimpleType/BOOL :java-args [RateLimitBucket Object Long] :impl (fn [[^RateLimitBucket b ^Object k ^Long tokens]] (swap! *where-rate-limits* update {:bucket b :bucket-key k} (fnil + 0) tokens) + true)}])]) - update {:bucket b - :bucket-key k} (fnil + 0) tokens))}])])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/db/cel.clj` around lines 1451 - 1458, The overload "_rateLimit_limit_tokens" currently returns the result of swap! (a numeric new count) but must return a boolean per the declared :cel-return-type BOOL; update the :impl for that overload (the anonymous fn taking [^RateLimitBucket b ^Object k ^Long tokens]) to perform the swap! side-effect against *where-rate-limits* and then return true (e.g., call swap! first and then return true) so rateLimit.<bucket>.limit(key, tokens) yields a boolean rather than a map/number.
🧹 Nitpick comments (2)
client/packages/core/src/rulesTypes.ts (1)
35-47:fieldstyping is broader than the validator.
InstantRulesEntityBlocknow accepts every non-idattribute infields, but the permissions schema only allows blob attrs there. That means schema-typed callers can writefieldsrules that compile and then fail validation later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/packages/core/src/rulesTypes.ts` around lines 35 - 47, The fields typing in InstantRulesEntityBlock is too broad and should be narrowed to only blob attributes to match runtime validation; update the fields mapped type in InstantRulesEntityBlock so the key set filters Schema['entities'][EntityName]['attrs'] to only those AttrName whose attr type extends your blob attribute type (e.g. using a conditional key remapping like [AttrName in Exclude<...> as Schema['entities'][EntityName]['attrs'][AttrName] extends BlobAttr ? AttrName : never]?: string), keeping the rest of the InstantRulesEntityBlock (bind, allow) unchanged.server/test/instant/db/transaction_test.clj (1)
5953-5962: Assert the retry hint, not just the exception kind.The public contract here is more than “some
::ex/rate-limitedexception was thrown” — it also includes the retry metadata. This test would still pass ifretry-at/retry-afterregressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/instant/db/transaction_test.clj` around lines 5953 - 5962, The test currently only checks that permissioned-tx/transact! throws a rate-limited exception via rate-limit-err?; update the catch to also assert the retry hint metadata (e.g. that (ex-data e) contains a non-nil :retry-at or :retry-after and that its value is a valid timestamp/number or parsable duration). Specifically, in the testing block around permissioned-tx/transact! (using make-ctx) replace the catch-return with assertions that (rate-limit-err? e) is true and that (get (ex-data e) :retry-after) or :retry-at exists and is of the expected type (non-nil and numeric/string parseable), failing the test if the retry hint is missing or invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/packages/core/src/rulesTypes.ts`:
- Around line 73-81: The top-level index signature currently allows any string
key to be an InstantRulesRateLimits which lets arbitrary namespaces accept
$rateLimits-shaped maps; change the shape so only an explicit $rateLimits
property (or another dedicated key) can be InstantRulesRateLimits and all other
arbitrary string keys must follow the entity block shape that requires an allow
field. Concretely, update the union on the index signature in rulesTypes.ts:
remove InstantRulesRateLimits from the generic [key: string] union and add a
distinct optional property named "$rateLimits": InstantRulesRateLimits; keep the
entity block type (with bind?, allow, fields?) as the index-signature target so
entities must include allow (refer to InstantRulesRateLimits and
InstantRulesAllowBlock to wire the types correctly).
In `@server/src/instant/db/instaql.clj`:
- Around line 1867-1875: The replay loop in the doseq in instaql.clj is
destructuring rate-limit entries as {:keys [^RateLimitBucket bucket key]} but
the entries are recorded as {:bucket b :bucket-key k} (see cel.clj), so key is
nil and everyone shares the same bucket; change the destructuring to pull
:bucket-key (e.g., {:keys [^RateLimitBucket bucket bucket-key]}) and pass that
bucket-key into (.limit bucket bucket-key ...) (or bind :bucket-key to the name
you expect) so the per-user/per-IP bucket is honored.
In `@server/src/instant/rate_limit.clj`:
- Around line 247-253: The check using (.toHours refill-period) floors the
duration and lets values like "1 day 1 second" slip through; replace the
hours-based comparison with a direct Duration comparison against a 1-day
Duration (e.g. use (.compareTo refill-period (java.time.Duration/ofDays 1)) and
treat > 0 as invalid) in the block that calls ex/throw-validation-err! for the
max limit, and add a regression test asserting that a duration of "1 day 1
second" (or Duration/ofDays 1 plus Duration/ofSeconds 1) is rejected.
In `@server/test/instant/db/instaql_test.clj`:
- Around line 5159-5167: The test currently only verifies the exception type via
rate-limit-err?; update the "third query is rate limited" assertion to capture
the thrown exception (instead of returning false) and assert the rate-limit
payload fields exist and are sane: use ex-data on the caught exception from
pretty-perm-q and assert that :retry-at and :retry-after keys are present (and
numeric or parsable) in addition to keeping the rate-limit type check
(rate-limit-err?). Locate the test block using pretty-perm-q and rate-limit-err?
and add assertions against (ex-data e) for :retry-at and :retry-after.
---
Outside diff comments:
In `@client/www/lib/permsJsonSchema.tsx`:
- Around line 141-163: The patternProperties currently matches keys like
'$rateLimits' causing JSON Schema to apply both properties.$rateLimits and
patternProperties to the same key; update the pattern in patternProperties
(where genericNamespaceRuleBlock is used) to exclude the literal '$rateLimits'
(e.g. change '^[$a-zA-Z0-9_\\-]+$' to a negative-lookahead variant like
'^(?!\\$rateLimits$)[$a-zA-Z0-9_\\-]+$') so that the explicit
properties.$rateLimits schema is the only one applied to that key.
---
Duplicate comments:
In `@server/src/instant/db/cel.clj`:
- Around line 1451-1458: The overload "_rateLimit_limit_tokens" currently
returns the result of swap! (a numeric new count) but must return a boolean per
the declared :cel-return-type BOOL; update the :impl for that overload (the
anonymous fn taking [^RateLimitBucket b ^Object k ^Long tokens]) to perform the
swap! side-effect against *where-rate-limits* and then return true (e.g., call
swap! first and then return true) so rateLimit.<bucket>.limit(key, tokens)
yields a boolean rather than a map/number.
---
Nitpick comments:
In `@client/packages/core/src/rulesTypes.ts`:
- Around line 35-47: The fields typing in InstantRulesEntityBlock is too broad
and should be narrowed to only blob attributes to match runtime validation;
update the fields mapped type in InstantRulesEntityBlock so the key set filters
Schema['entities'][EntityName]['attrs'] to only those AttrName whose attr type
extends your blob attribute type (e.g. using a conditional key remapping like
[AttrName in Exclude<...> as Schema['entities'][EntityName]['attrs'][AttrName]
extends BlobAttr ? AttrName : never]?: string), keeping the rest of the
InstantRulesEntityBlock (bind, allow) unchanged.
In `@server/test/instant/db/transaction_test.clj`:
- Around line 5953-5962: The test currently only checks that
permissioned-tx/transact! throws a rate-limited exception via rate-limit-err?;
update the catch to also assert the retry hint metadata (e.g. that (ex-data e)
contains a non-nil :retry-at or :retry-after and that its value is a valid
timestamp/number or parsable duration). Specifically, in the testing block
around permissioned-tx/transact! (using make-ctx) replace the catch-return with
assertions that (rate-limit-err? e) is true and that (get (ex-data e)
:retry-after) or :retry-at exists and is of the expected type (non-nil and
numeric/string parseable), failing the test if the retry hint is missing or
invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 14f5839a-5fa5-4eb6-b787-c283933f3a21
📒 Files selected for processing (14)
client/packages/core/src/rulesTypes.tsclient/packages/core/src/utils/fetch.tsclient/www/app/docs/rate-limits/page.mdclient/www/data/docsNavigation.jsclient/www/lib/permsJsonSchema.tsxserver/resources/config/staging.ednserver/src/instant/db/cel.cljserver/src/instant/db/instaql.cljserver/src/instant/rate_limit.cljserver/src/instant/util/exception.cljserver/test/instant/db/instaql_test.cljserver/test/instant/db/transaction_test.cljserver/test/instant/model/rule_test.cljserver/test/instant/rate_limit_test.clj
✅ Files skipped from review due to trivial changes (2)
- client/www/data/docsNavigation.js
- client/packages/core/src/utils/fetch.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/instant/util/exception.clj
- server/resources/config/staging.edn
dwwoelfel
commented
Apr 16, 2026
| (def sse-message-type-id 10) | ||
| (def wal-record-type-id 11) | ||
| (def create-magic-code-key-type-id 12) | ||
| (def consume-magic-code-key-type-id 13) |
Contributor
Author
There was a problem hiding this comment.
Usually we'd have to wait for the backend to fully deploy before we removed a type, but I've updated the map name. New instances will never see this type, so it's safe to remove.
dwwoelfel
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support for rate limits in rules.
You define the rate limit configs inside of the permissions file with a "$rateLimits" key. And then you can use them by adding
rateLimit.yourKey.limit(anyValue), whereanyValuewill probably beauth.idorrequest.ip.I decided to put the rate limits in the perms file because it lets us validate that any usage of
rateLimit.someConfighas the corresponding"$rateLimits"config. That will prevent any runtime errors. If we put them in multiple files, then it would be harder for us to ensure that they were in sync. The downside is that we can't create a system table called$rateLimits, but I think that's ok.Here what the rules code looks like:
{ "myNs": { "allow": { "create": "rateLimit.createMyNs.limit(auth.id)" } }, "$rateLimits": { "createMyNs": { "limits": [{ "capacity": 10, "refill": { "period": "1 hour" } }] } } }If you exceed the limit, we'll throw a custom rate-limit error that looks like this:
{ "hint": { "retry-at": "2026-04-15T16:09:24.530786Z", "retry-after": 1, "remaining-tokens": 0 }, "traceId": "074d1463fc63c38ced71aceeda306c19", "name": "InstantAPIError", "status": 429, "body": { "type": "rate-limited", "message": "Your request exceeded the rate limit.", "hint": { "retry-at": "2026-04-15T16:09:24.530786Z", "retry-after": 1 }, "trace-id": "074d1463fc63c38ced71aceeda306c19" } }If you have some operation that consumes more than one token,
limittakes an optional 2nd "tokens" arg for the amount of tokens, e.g.rateLimit.createMyNs.limit(auth.id, 10)will consume 10 tokens (or throw if fewer than 10 are available).Notes on implementation
We use bucket4j for the rate limit buckets. It stores data in a hazelcast map which is also backed by postgres using a
MapStore. Hazelcast gives us fast in-memory updates, but can still fetch the older entries from postgres when we exceed our memory budget.The postgres store is cleaned out periodically, deleting anything that hasn't been updated in two days. We don't let you set a refill period longer than 1 day, so we can be certain that anything older than 2 days is stale.
We could put the
MapStorein an s3 directory bucket instead of postgres, but I'm hoping postgres will be enough since that will be easier to manage.Rate-limit keys
The rate-limit bucket is just a giant map. We hash all of the inputs with hmac-sha256 and then take the first 128 bits for the key. That should give us good collision resistance and make it impossible to pre-generate collisions. We put the key into a UUID format. That lets us use a little postgres trick--instead of storing the key as
bytea, we can store it as auuid.uuidhas a fixed-length, so we save a byte per row.We previously wrapped the key in a record, e.g.
CreateMagicCodeKey. Now we just use a bare UUID and include a string for each key type in the hash. That saves us a little overhead in hazelcast. I updated the map name so that we don't have to worry about backwards compatibility (has the effect of a 1-time rate-limit reset).