Skip to content

feat(ratelimit): make rate-limit thresholds runtime-configurable - #756

Open
FenjuFu wants to merge 2 commits into
mainfrom
feat/configurable-rate-limits
Open

feat(ratelimit): make rate-limit thresholds runtime-configurable#756
FenjuFu wants to merge 2 commits into
mainfrom
feat/configurable-rate-limits

Conversation

@FenjuFu

@FenjuFu FenjuFu commented Aug 25, 2026

Copy link
Copy Markdown
Member

What

Make the per-endpoint rate-limit thresholds configurable at runtime, with a global on/off switch — closes #726.

Why

The quotas were compile-time constants baked into the @RateLimit annotation on each endpoint (search 60/20, download 120/30, publish 10, the various auth flows, etc.). Operators couldn't tune them for their traffic or turn limiting off in a trusted/internal deployment without rebuilding the server.

How

  • New RateLimitProperties (skillhub.ratelimit, component-scanned like DownloadRateLimitProperties):
    • enabled — master switch; false makes the interceptor skip all quota checks.
    • categories.<category>.{authenticated, anonymous, windowSeconds} — per-category overrides, each field optional.
  • RateLimitInterceptor now resolves the effective authenticated/anonymous limit and window from config, falling back to the annotation values when nothing is configured.
  • application.yml wires enabled: ${SKILLHUB_RATELIMIT_ENABLED:true} and documents the override keys with a commented example.

Everything is reachable via SKILLHUB_RATELIMIT_* env vars, e.g.:

SKILLHUB_RATELIMIT_ENABLED=false
SKILLHUB_RATELIMIT_CATEGORIES_SEARCH_AUTHENTICATED=120
SKILLHUB_RATELIMIT_CATEGORIES_PUBLISH_WINDOW_SECONDS=3600

Behavior / compatibility

  • Zero behavior change by default: with no config, every endpoint uses its existing annotation values. I deliberately did not seed default category values, because the same category can carry different limits in different controllers (e.g. download is 120/30 in the portal/CLI controllers but 60/20 in ClawHubCompatController) — seeding one value would silently change the compat endpoints. An override, when set, applies to every endpoint sharing that category, which is the intended "tune this category" semantic.
  • Added RateLimitPropertiesTest covering the enabled default, annotation fallback, partial-field override, and per-category isolation.

Closes #726

The per-endpoint quotas were compile-time constants in the @ratelimit
annotation, so operators could not tune them or turn limiting off without
rebuilding (#726).

Add RateLimitProperties (skillhub.ratelimit) with a global `enabled` toggle
and per-category threshold overrides, and have RateLimitInterceptor resolve
the effective authenticated/anonymous limit and window from config, falling
back to the annotation defaults. Unset categories keep the built-in values,
so behavior is unchanged until an override is provided; `enabled=false`
disables quota checks entirely. All configurable via SKILLHUB_RATELIMIT_*
environment variables.

Closes #726

Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
@XiaoSeS

XiaoSeS commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR. The direction is useful and the implementation is close, but I think this needs one more binding-level check before merge.

The PR says the per-category overrides are configurable through SKILLHUB_RATELIMIT_* environment variables, and application.yml documents examples such as:

SKILLHUB_RATELIMIT_CATEGORIES_SEARCH_AUTHENTICATED=120
SKILLHUB_RATELIMIT_CATEGORIES_PUBLISH_WINDOW_SECONDS=3600

I tried to validate that path on this PR head plus current main with a Spring Boot ApplicationContextRunner / @EnableConfigurationProperties test. The environment lookup can see the key, but RateLimitProperties.categories remains empty, so the interceptor would still use the annotation defaults.

Could you please add a test that proves the documented environment-variable override path actually binds into RateLimitProperties and is used by RateLimitInterceptor? If the map shape is too brittle for env vars, I’d suggest switching the public env interface to explicit built-in category keys, for example:

SKILLHUB_RATELIMIT_PUBLISH_AUTHENTICATED=10
SKILLHUB_RATELIMIT_PUBLISH_WINDOW_SECONDS=60
SKILLHUB_RATELIMIT_SEARCH_AUTHENTICATED=60

Keeping the map for YAML-style config is fine, but the env examples should be backed by tests because this feature is mainly for self-hosted operators.

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
@XiaoSeS

XiaoSeS commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Follow-up correction on the binding concern above:

My original ApplicationContextRunner reproduction used a generic property-source name, so Spring Boot did not apply its system-environment variable name adaptation. Spring Boot applies that adaptation to systemEnvironment and property sources whose names end in -systemEnvironment. With the fixture corrected, SKILLHUB_RATELIMIT_CATEGORIES_SEARCH_AUTHENTICATED=120 binds as intended.

I pushed 80a7e83a, which adds a binding-level regression test using SystemEnvironmentPropertySource. I also verified the real container path with SKILLHUB_RATELIMIT_CATEGORIES_SEARCH_ANONYMOUS=2: the first two anonymous searches returned 200 and the third returned 429. With SKILLHUB_RATELIMIT_ENABLED=false, five consecutive searches returned 200.

One deployment note: Compose users must explicitly pass category variables into server.environment (for example with a Compose override); putting an arbitrary category key only in the host .env file does not automatically inject it into the container. The application-side implementation itself does not need the proposed redesign.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] 希望可以用环境变量自定义 API 精细限流阈值

2 participants