Skip to content

feat(sharing): opt-in per-user rate limiting on sharing operations#24

Closed
doctatortot wants to merge 1 commit into
ourmem:mainfrom
doctatortot:feat/share-rate-limit
Closed

feat(sharing): opt-in per-user rate limiting on sharing operations#24
doctatortot wants to merge 1 commit into
ourmem:mainfrom
doctatortot:feat/share-rate-limit

Conversation

@doctatortot
Copy link
Copy Markdown
Contributor

Problem

Sharing has no per-user/space rate limit — one user with write access can fire thousands of share operations in rapid succession. OmemError::RateLimited (→ 429) existed but was never returned.

Change

Add an opt-in per-user token-bucket limiter (api/rate_limit.rs):

  • OMEM_SHARE_RATE_PER_MIN tokens; refill per_min / 60 per second (bursts up to the per-minute value allowed, then steady).
  • 0 = disabled (default) → no behavior change.
  • Enforced at the top of all eight sharing handlers (share, pull, reshare, batch-share, share-all, share-to-user, share-all-to-user, org/publish): state.share_rate_limiter.check(&auth.tenant_id)?. An empty bucket returns the existing 429.

State is process-local (Mutex<HashMap>); a multi-replica deployment would need a shared store (noted in the module docs).

Tests

rate_limit unit tests with injected time (no sleeps): burst-then-block, refill-over-time, per-key isolation, disabled. Full suite green (392 passed); fmt + clippy clean (no new warnings).

Docs

.env.example + docs/SHARING.md (the "No rate limiting" entry now reads "opt-in").

Sharing had no rate limit -- one user could fire thousands of share ops in rapid
succession. OmemError::RateLimited (429) existed but was never returned.

Add an in-memory per-user token-bucket limiter (api/rate_limit.rs):
OMEM_SHARE_RATE_PER_MIN tokens, refilling at per_min/60 per second (bursts up to
the per-minute value allowed, then a steady rate); 0 = disabled (default, so no
behavior change). Enforced at the top of all eight sharing handlers (share, pull,
reshare, batch-share, share-all, share-to-user, share-all-to-user, org/publish)
via state.share_rate_limiter.check(&auth.tenant_id)?; an empty bucket returns 429.

State is process-local (Mutex<HashMap>); multi-replica deployments would need a
shared store.

Tests: rate_limit unit tests (burst-then-block, refill-over-time, per-key
isolation, disabled) with injected time. Full suite green (392). fmt + clippy clean.

Docs: .env.example + docs/SHARING.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@yhyyz
Copy link
Copy Markdown
Contributor

yhyyz commented May 21, 2026

Merge conflict after #23 landed (likely config.rs or sharing.rs). Could you rebase onto main? Will merge immediately after. 🙏

yhyyz added a commit that referenced this pull request May 21, 2026
- feat(sharing): opt-in per-user rate limiting (PR #24)
  Token-bucket limiter on all 8 sharing handlers.
  OMEM_SHARE_RATE_PER_MIN=0 (default, disabled).

- feat(sharing): opt-in lazy auto-refresh stale shares on read (PR #21)
  Extracts refresh_shared_copy() helper, reused by reshare endpoint
  and new auto-refresh-on-read path in get_memory.
  OMEM_AUTO_REFRESH_SHARES=false (default, disabled).

Co-authored-by: doctatortot <doctatortot@users.noreply.github.com>
@yhyyz
Copy link
Copy Markdown
Contributor

yhyyz commented May 21, 2026

Applied in 4db9c5e — resolved the conflict on our side since the merge order was our fault. All 394 tests pass. Thanks @doctatortot! 🙏

@yhyyz yhyyz closed this May 21, 2026
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.

2 participants