Skip to content

fix(rate-limit): warn once when the key ceiling binds; correct eviction policy docs - #314

Open
claudegoogl-sudo wants to merge 1 commit into
masterfrom
chore/rate-limit-eviction-observability
Open

fix(rate-limit): warn once when the key ceiling binds; correct eviction policy docs#314
claudegoogl-sudo wants to merge 1 commit into
masterfrom
chore/rate-limit-eviction-observability

Conversation

@claudegoogl-sudo

@claudegoogl-sudo claudegoogl-sudo commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Follows CONTRIBUTING.md PR template — sections present: Thinking Path, Linked Issues or Issue Description (path B, enhancement labels), What Changed, Verification, Risks, Model Used, Checklist.

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • The server rate-limits untrusted keys (anonymous webhook sources, plugin wake targets) through a shared sliding-window store
  • The store bounds memory with a hard ceiling on live keys. When the ceiling binds, it evicts the first-inserted live key
  • Two gaps existed. The doc comment claimed the policy "drops the coldest keys, not the hottest" — an LRU claim. The store is FIFO by first insertion, so a hot key inserted early is evicted before a cold key inserted later. Eviction was also silent, so an operator could not see the ceiling bind
  • This pull request corrects the comment and adds one warning per store instance on the first ceiling eviction
  • The benefit is a truthful policy document and an observable ceiling, with zero change to eviction or limiting behaviour

Linked Issues or Issue Description

No public GitHub issue exists for this internal hardening follow-up. Framed as an Enhancement per .github/ISSUE_TEMPLATE/enhancement.yml:

Pre-submission checklist

  • This improves existing behaviour — it adds no new command, endpoint, or concept.
  • No existing open or closed proposal covers this change.

What existing behavior does this improve?

The sliding-window rate-limit store (server/src/services/sliding-window-rate-limit-store.ts): the accuracy of its eviction-policy documentation, and observability when the maxKeys ceiling binds.

Subsystem affected

server/ — REST API & orchestration services

Current behavior

When a new key pushes the live-key map over maxKeys, the store evicts the first-inserted live key and logs nothing. The module comment claims the policy "drops the coldest keys, not the hottest". That is wrong: the store is FIFO by first insertion, not LRU. A long-lived hot key inserted before a flood is evicted before cold keys inserted after it.

Proposed behavior

The comment states FIFO-by-first-insertion accurately. The first ceiling eviction per store instance emits exactly one warning naming the limiter and the ceiling:

pino context: {"limiter":"plugin-webhook:ip","maxKeys":10000}
message: "sliding-window rate-limit key ceiling bound; evicting first-inserted (FIFO) keys"

Repeat evictions stay silent, so a key flood cannot become a log flood.

Reason and benefit

A binding ceiling means the live working set no longer fits. That is the one signal that the ceiling is mis-tuned or that a key-flood attack is in flight. Without a log line that state is invisible. The corrected comment stops future maintainers from reasoning about the store as an LRU cache.

Breaking changes

None. Eviction order, limiting decisions, and the store API shape are unchanged. The new name option is optional and defaults to a generic store name.

What Changed

  • server/src/services/sliding-window-rate-limit-store.ts: corrected the FIFO claims in the module header, the DEFAULT_SLIDING_WINDOW_MAX_KEYS doc, and the record() contract. Added an optional name option. Added a once-per-instance logger.warn on the first ceiling eviction with { limiter, maxKeys } context.
  • server/src/services/plugin-webhook-rate-limit.ts: names its three stores plugin-webhook:endpoint, plugin-webhook:ip, and plugin-webhook:verified-endpoint.
  • server/src/services/plugin-wake-rate-limit.ts: names its store plugin-wake.
  • server/src/__tests__/sliding-window-rate-limit-store.test.ts: new "eviction observability" tests. Two evictions produce exactly one warning. No warning when the ceiling never binds. A fresh store instance warns again.

Verification

  • node_modules/.bin/vitest run server/src/__tests__/sliding-window-rate-limit-store.test.ts server/src/__tests__/plugin-webhook-rate-limit.test.ts server/src/__tests__/plugin-wake-rate-limit.test.ts — 3 test files passed, 64 tests passed (21 store, 43 webhook limiter, wake limiter included).
  • pnpm --filter @paperclipai/server typecheck (tsc --noEmit) — exit 0.
  • Manual: the new tests assert warn was called exactly once after two separate ceiling evictions, assert the warning context equals { limiter, maxKeys }, and assert no warning before any eviction.

Risks

Low risk. The only runtime change is one log line per store instance, plus a logger import into the store module. Eviction order, map-size behaviour, and limiting decisions are untouched. All pre-existing rate-limit tests pass without modification.

Model Used

  • Claude (Anthropic), via Claude Code / claude_local adapter, extended thinking + tool use (shell, file edit, test execution).

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have not referenced internal/instance-local Paperclip issues or links (only public GitHub #NNN / github.com/paperclipai/paperclip URLs)
  • My branch name describes the change (e.g. docs/..., fix/...) and contains no internal Paperclip ticket id or instance-derived details
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • All Paperclip CI gates are green
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups (Greptile is not installed on this fork, so no bot review exists; no open findings to address)
  • I will address all Greptile and reviewer comments before requesting merge

…iction

The store comment claimed the oldest-first eviction policy "drops the
coldest keys, not the hottest". The store evicts FIFO by first insertion:
a long-lived hot key inserted before a flood is evicted before cold keys
inserted after it. The module header, the DEFAULT_SLIDING_WINDOW_MAX_KEYS
doc, and the record() contract now say exactly that.

Ceiling eviction was also silent. It now emits a single logger.warn per
store instance on the first eviction, naming the limiter and the ceiling
via a new optional store `name`. Repeat evictions stay silent so a key
flood cannot become a log flood. The webhook limiter names its three
bucket stores (endpoint, ip, verified-endpoint) and the wake limiter
names its store. No change to eviction or limiting behaviour.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
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.

1 participant