Skip to content

refactor(abuse)!: make adapters immutable - #14056

Open
ChiragAgg5k wants to merge 16 commits into
mainfrom
refactor/abuse-immutable
Open

ChiragAgg5k wants to merge 16 commits into
mainfrom
refactor/abuse-immutable

Conversation

@ChiragAgg5k

Copy link
Copy Markdown
Member

Closes #14046

Summary

utopia-php/abuse adapters were stateful. setParam() mutated them, parseKey() overwrote the key pattern so later params were ignored, and TimeLimit cached its count and window start on the instance, so a reused adapter counted into an expired window. The adapters are now immutable, matching packages/client, circuit-breaker, schedule and smtp. This is a breaking change, released as abuse 3.0.0.

$limit = new TimeLimit\Redis('url:{url},ip:{ip}', 10, 60, $redis)
    ->withParams(['{url}' => $url, '{ip}' => $ip]); // returns a clone

$result = $limit->check(); // Result { limited, limit, remaining, reset }
$limit->peek();            // read-only, no hit
$limit->reset();

Changes

Before After
setParam() mutates and returns $this withParams() / withParam() return a clone; the key is resolved per call with strtr()
check(): bool, plus remaining(), limit(), time() check(): Result and peek(): Result
Cached $count and $timestamp on the instance Window derived from the clock on every call
Mutable protected properties readonly classes with promoted constructor properties; concrete adapters are final
Utopia\Abuse\Abuse wrapper Removed; call the adapter directly
Utopia\Abuse\Adapters\* Utopia\Abuse\Adapter\*
ReCaptcha extends Adapter, raw curl Standalone final readonly ReCaptcha::verify() on utopia-php/client
SlidingWindow/TokenBucket fail open on a bad Lua reply Throw RuntimeException, like TimeLimit
44-entry PHPStan baseline Level max is clean; baseline deleted

Appwrite call sites migrated: shared/api.php (init + shutdown), init/resources.php, realtime.php, Mqtt/Handler.php, Functions/Create.php, Workers/Deletes.php.

X-RateLimit-* headers are unchanged. The old remaining was read before the hit as limit - (count + 1), which equals the post-hit remaining that check() now returns. When _APP_OPTIONS_ABUSE is disabled, headers come from peek(), so no hit is recorded (same as before).

Test plan

Check Result
abuse unit 35 passed
abuse e2e (Redis, RedisCluster, RedisPool, RedisPoolCluster, MySQL, SlidingWindow ×3, TokenBucket ×3) 139 tests, 0 failures, 14 skipped (TablesDB, needs APPWRITE_ENDPOINT)
Regression: reused adapter + withParams, and window rollover on the same instance Fails on main, passes here
abuse PHPStan level max, Rector, Pint Clean
Root composer lint, analyze, refactor:check Pass
Appwrite e2e abuseEnabled group CI

Follow-up (Cloud, after 3.0.0 is tagged)

  • Port the Pool/None subclasses of TimeLimit, SlidingWindow and TokenBucket to the new hit/count/set contract; the breaker fallback must cover the new throw-on-bad-reply behaviour.
  • app/http.php: (new Abuse($timeLimit))->check() → $timeLimit->check()->limited.
  • app/realtime.php getTimelimit(): stop caching one adapter per coroutine; build it, or call withParams(), per request.

…rams

Adapters cached the window timestamp and count at construction and mutated
their key via setParam, so a reused instance kept counting into a stale
window and leaked params between callers. Adapter is now readonly with
withParams clones, check/peek return a Result, the window is derived from
now() on every call, and the Redis family hits through one atomic Lua script.
Adds the readonly Adapter with withParams/key, the Result value object, the
TimeLimit base with a now() clock seam, and the Redis/RedisCluster/RedisPool/None
adapters behind a shared RedisBase that hits through one atomic Lua script.
Unit tests drive the base through an in-memory adapter with a movable clock.
…utable contract

hit() now returns the pre-hit count and only writes below the limit, count() is a read-only lookup, and nothing is cached on the instance so withParams() clones stay correct across windows.
Window and elapsed fraction are derived from a now() seam on every call, so a reused instance never carries stale window state; the Lua script returns the pre-hit estimate so Result can be built uniformly across algorithms.
Adapters no longer cache the timestamp or bucket state, so a reused or cloned instance always reads a fresh refill. check() returns a Result, and both Lua scripts return the integer tokens used before the call, with every key passed in KEYS for Dragonfly. The refillRate guard now applies only when tokens > 0, so limit 0 (unlimited) can be built from limit / interval.
ReCaptcha never fit the rate-limit Adapter contract (reset/getLogs/cleanup all threw), so it becomes a final readonly verifier with an injectable PSR-18 client, defaulting to utopia-php/client over cURL. The package now targets PHP 8.5 and the README examples follow the immutable withParams()/Result API.
Cover remaining, peek, reset time, withParams isolation and window rollover on a single instance; align assertions to window starts so clock-sensitive checks cannot straddle a boundary.
…ch to immutable API

Reuse one adapter instance across windows and refills so the suites catch state frozen at construction, and cover withParams clones counting independently.
Callers now pass params through withParams() clones and read check()/peek() Results, so headers come from one atomic round trip instead of separate remaining()/limit()/time() reads, and key/API-key paths emit headers via peek() without recording a hit.
…TokenBucket

Match TimeLimit\RedisBase so a broken eval surfaces instead of silently allowing the request; fail-open stays the job of subclasses that override eval (e.g. Cloud Pool breakers).
The pool suites prefix keys in getAdapter, so hard-coded keys failed RedisPoolTest and RedisPoolClusterTest.
The immutable rewrite typed the scan and log replies that the 44 baseline entries covered.
Each check now reads the clock, so three database round trips in a one-second window could straddle a second boundary on a cold MySQL and flake testStaticKey.
@ChiragAgg5k ChiragAgg5k self-assigned this Oct 1, 2026
@hansi-codes

hansi-codes Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

🟢 Tier S · Ready to merge

The incremental test change preserves coverage of the submitted form values without depending on a particular serialization.

Refactors the abuse package to immutable adapters with parameter-copy methods and a shared Result value object, and migrates Appwrite callers to the new API. Separates ReCaptcha verification from rate limiting and updates package configuration, documentation, and regression coverage, including fractional token-bucket reset calculations. The newest change checks decoded ReCaptcha form fields instead of an exact serialized request body.

Latest changes: Updates the ReCaptcha request test to decode the form body and assert field values rather than requiring one exact URL-encoded string.

Verdict New comments Fixed Still open
✅ Approved 0 0 0
📂 Walkthrough · 19
File Change
app/controllers/shared/api.php Migrates abuse checks, response headers, and resets to immutable parameters and Result fields.
app/init/resources.php Constructs the renamed Redis TimeLimit adapter within the pool lease.
app/realtime.php Migrates connection and message rate limiting to direct adapter checks.
src/Appwrite/Mqtt/Handler.php Migrates MQTT CONNECT limiting to immutable parameters and Result.
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php Uses Result for function-creation limits and response headers.
src/Appwrite/Platform/Workers/Deletes.php Updates the abuse database adapter import.
packages/abuse/README.md Documents immutable adapters, Result, and standalone ReCaptcha verification.
packages/abuse/composer.json Updates PHP requirements and HTTP-client dependencies.
packages/abuse/phpstan.neon, packages/abuse/phpstan-baseline.neon, packages/abuse/rector.php Removes the analysis baseline and updates analysis and refactoring configuration.
packages/abuse/src/Adapter.php, packages/abuse/src/Result.php Introduces immutable parameter copies and the shared rate-limit result contract.
packages/abuse/src/Adapter/TimeLimit.php, packages/abuse/src/Adapter/TimeLimit/** Replaces cached state with live windows and storage operations.
packages/abuse/src/Adapter/SlidingWindow.php, packages/abuse/src/Adapter/SlidingWindow/** Ports sliding-window implementations to immutable adapters and Result.
packages/abuse/src/Adapter/TokenBucket.php, packages/abuse/src/Adapter/TokenBucket/** Ports token buckets to immutable adapters and preserves fractional balances for reset calculations.
packages/abuse/src/ReCaptcha.php Adds standalone verification through an injectable HTTP client.
packages/abuse/src/Abuse.php, packages/abuse/src/Adapters/** Removes the wrapper and old adapter implementations.
packages/abuse/tests/ReCaptchaTest.php Tests verification verdicts and outgoing requests, now asserting decoded form values.
packages/abuse/tests/** Migrates unit, integration, and benchmark coverage and adds reuse, rollover, and fractional-reset regressions.
rfc/monorepo.md Updates the abuse package’s static-analysis status.
packages/abuse/rector.php Updates adapter paths and removes obsolete rule exclusions.

Reviewed the commits since 3d7852a · Details · Comment @hansi-codes review to re-run, or mention @hansi-codes with a question.

@hansi-codes hansi-codes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Tier A · See the inline comments. Summary

Comment thread packages/abuse/src/Adapter/TokenBucket.php Outdated
@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

✨ Benchmark results

Comparing main (before) → refactor/abuse-immutable (after).

Metric Before After Change
🚀 Requests/sec 193.08 187.91 ⚪ -2.7%
⏱️ Latency P50 88.77 ms 90.7 ms ⚪ +2.2%
⏱️ Latency P95 214.89 ms 218.29 ms ⚪ +1.6%
Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario P50 (ms) P95 (ms) Requests RPS Δ P95 (ms)
API total 90.7 218.29 11,742 187.91 +3.4
Account 177.08 339.55 618 10.23 -2.03
TablesDB 85.83 165.25 6,386 104.36 -8.3
Storage 85.97 185.84 3,090 52.25 +9.26
Functions 131.22 274.08 1,648 28.42 +11.09

Top API waits (after)

API request Max wait (ms)
account.name.update 540.97
functions.create 435
functions.variables.update 432.1
functions.variables.create 424.35
account.prefs.update 414.18

The Redis scripts floored the balance before returning, so reset overstated the time until the bucket refills, and peek() reported the post-consume refill time. Adapters now return the refilled balance and TokenBucket subtracts the consumed token only on check().

@hansi-codes hansi-codes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Tier S · Looks good to merge. Summary

This branch has not been deployed

No deployments
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.

Make the abuse library immutable

1 participant