Repository navigation
bug(rate-limiting): three _SharedRateLimiter singletons are defined and never called, so the controls they imply do not run #16857
Description
Split out of #16845's third acceptance criterion. Cost recording (AC1/AC2) is a different control on a different surface from request-rate limiting, and carrying both on one issue would make #16845 close on partial delivery or not close at all.
Problem
autobot_shared/rate_limiter.py's shared RateLimiter is instantiated as a module-level singleton in several places. Three of those singletons are never called anywhere — they are constructed at import and nothing acquires against them, so the control each one implies is not running.
| Singleton | Defined at | Non-definition references |
|---|---|---|
user_rate_limiter |
autobot-backend/user_management/middleware/rate_limit.py:28 |
none |
conversation_rate_limiter |
autobot-backend/utils/conversation_rate_limiter.py:360 |
none |
llm_rate_limiter |
autobot-backend/llm_shared/optimization/rate_limiter.py:393 |
one, its own __all__ entry at :382 |
Each is a live object doing nothing: no request handler, middleware, or dependency acquires against it. A reader finding user_rate_limiter in the user-management middleware layer reasonably concludes per-user request limiting is enforced. It is not.
Two things this is not, checked rather than assumed
#16845 named two; there are three. llm_rate_limiter is the addition. It is easy to read as live because git grep llm_rate_limiter also matches get_llm_rate_limiter in llm_shared/base_provider.py:167 and cross_worker_rate_limiter.py — but that is a different object, LLMCrossWorkerRateLimiter (cross_worker_rate_limiter.py:252), which is genuinely wired. The _SharedRateLimiter at optimization/rate_limiter.py:393 is exported and never acquired.
ConversationRateLimiter the class is alive; the singleton is not. utils/claude_api_integration.py:135 does self.rate_limiter = ConversationRateLimiter() if self.config.enable_rate_limiting else None and calls can_make_request(). That is the in-memory payload/statistics limiter in the same module — not the module-level conversation_rate_limiter _SharedRateLimiter instance, which is the Redis-backed sliding window and is the dead one. Deleting the module on a grep of the class name would remove working code.
Working references in-tree
Two singletons built the same way are wired, so the pattern to follow already exists and does not need designing:
gateway_rate_limiter(services/gateway/message_queue.py:30) —await gateway_rate_limiter.acquire_token(platform, requests_per_second=rps, burst_size=burst_size)at:94, covered bytests/test_gateway_manager.py:503.integration_rate_limiter(integrations/rate_limiter.py:214) —await _shared_rate_limiter.acquire(...)atintegrations/github_integration.py:292andcommunication_integration.py:244, covered byintegrations/github_integration_test.py:202-208.
Impact
There is no per-user and no per-conversation request-rate cap running on AutoBot's request surfaces. The infrastructure is built, configured with tiers, and connected to Redis; it is simply never invoked. This is the same shape as #16845's cost defect — a control that exists, looks present to a reader, and does not execute.
Acceptance criteria
-
user_rate_limiteris acquired against on a real per-user request path, or the reason it cannot be is recorded on this issue. -
conversation_rate_limiterlikewise for the per-conversation path, without disturbing theConversationRateLimiterclass thatclaude_api_integration.pyuses. -
llm_rate_limiteris acquired against, or its relationship toLLMCrossWorkerRateLimiteris settled — if the cross-worker limiter already covers this surface, the redundant singleton is the finding and the resolution is to say so in the module, not to leave two limiters implying two controls. - A test per wired limiter asserts a request is actually refused when the limit is exceeded — not that
acquirewas called. A mock that returns truthy passes against an unwired limiter. - No limiter is removed to satisfy this issue. Per the repo's "cleanup means finishing the work" rule, deletion is only correct once the control is demonstrably not wanted, which is an owner decision and not assumed here.
Open question
Whether each surface should be rate limited, and at what tier, is a product decision rather than a bug fix. If the answer for any of the three is "this control is not wanted", that is a legitimate outcome — but it needs stating in the module so the next reader does not re-file this issue, rather than leaving a constructed-and-unused object as the only record.
Related
Split from #16845 (AC3). Umbrella #16835 (control-plane map). Same defect class as #16845's LLMCostTracker.record — built, plausible to a reader, never executing.
🤖 Generated with Claude Code
Activity
mrveiss commentedon Sep 17, 2026
Answering the question that should be settled before any code: for each of the three, is it dead because it was never wired, or because what it protected moved?
All three were never wired. No call site has existed for any of them at any point in the repository's history — not one that was later removed.
Method:
git log -S'<name>.' -- autobot-backendover all ofmain's history. That pickaxe matches any commit that ever added or removed an attribute access on the singleton, so a call site that existed and was later deleted would appear. All three return zero commits. The negative control is the two working limiters: the same search shape finds the live call sites forgateway_rate_limiterandintegration_rate_limiterin the tree today.user_rate_limiter0f631d884— "consolidate 5 rate limiters into sharedautobot_shared/rate_limiter.py(#4460)"conversation_rate_limiterllm_rate_limiter0af9875cd— a rename ofllm_interface_pkg→llm_sharedSo the shape of this is not five abandoned controls. The #4460 consolidation created shared-limiter singletons for several scopes and wired some of them and not others —
gateway_rate_limiterandintegration_rate_limitergot call sites and tests, these three got a definition and nothing else. It is an incomplete refactor, not a retired feature.The one that must not simply be wired
llm_rate_limiteris the exception, and wiring it would be the decorative outcome the question is guarding against. The surface it would cover is already rate-limited:llm_shared/base_provider.py:167doeswhich is
LLMCrossWorkerRateLimiter(cross_worker_rate_limiter.py:252) — Issue #8170, a Redis token shared across all uvicorn workers, with a documented allow-all fallback when Redis is down, sitting inside the GH#16527 concurrency semaphore. That is a better fit for the LLM surface than a per-process sliding window, and it post-dates the dead singleton.Adding a second limiter to that path would mean two limiters on one surface with independent budgets — the outcome where it looks enforced and the numbers mean nothing. For this one the honest resolutions are to remove it, or to leave it and say in the module that
LLMCrossWorkerRateLimiterowns this surface. That is AC3 of this issue as filed, and this is the evidence for it.The two with no substitute
user_rate_limiterandconversation_rate_limiterhave no equivalent covering their surfaces:user_management/middleware/rate_limit.pyholdsTargetedPasswordChangeRateLimiter, which uses Redis directly for fixed-attempt counters on the password-change surface only. There is no general per-user request cap — I found noRateLimitMiddleware, noslowapi, and noadd_middlewarerate-limit registration anywhere inautobot-backend.ConversationRateLimiterthe class is alive atclaude_api_integration.py:135, but it is the in-memory payload/statistics limiter in the same module, per-process and per-instance. The Redis-backed sliding window beside it — the dead singleton — has no counterpart.Worth recording, because it is the closest this came to "what it protected moved": the only historical reference to the module from outside was
claude_api_optimization_suite.py:33, retired by248d6dae1. That line importedConversationRateLimiter, the class — not the singleton. So even the retired consumer never used the dead object.What this means for the acceptance criteria
The first two ACs stand as written: these are genuinely unwired controls on genuinely unprotected surfaces, and wiring them adds real enforcement.
The third needs re-reading in light of the above — for
llm_rate_limiterthe likely correct outcome is not wiring. I am not making that call here: whether the cross-worker limiter fully covers the intent is a judgement about what the LLM surface should be limited on, and per this issue's own last criterion no limiter gets removed to satisfy it without an owner saying so.mrveiss commentedon Oct 2, 2026
Owner decision (2026-10-02), answering this issue's open question
Measured first: the three dead singletons are instances of the one shared
autobot_shared.rate_limiter.RateLimiter. There is no duplicate implementation to consolidate; the gap is wiring only.ssot_configdefines none of therate_limit_*keys, so the hard-coded tier defaults apply:authenticated60/min and 2000/h,privileged300/min.Decided by the owner:
user_rate_limiter: acquired per user on the chat message-send path only, not as middleware on every route. A route-wide 60/min cap would 429 the UI's own polling and reconnects.conversation_rate_limiter: acquired per conversation on the same chat send path. TheConversationRateLimiterclass used byclaude_api_integration.pyis untouched.llm_rate_limiter: not wired. Provider dispatch is already rate-limited byLLMCrossWorkerRateLimiter. The resolution is to verify that limiter covers every provider call and to state it in the module, rather than run two LLM limiters (this issue's AC3 option).Sibling search (open issues on these files, plus rate limit / throttle / backoff): none share these files. #17221 is
autobot_shared/rate_limit.py, the IP limiter, a different module. Nothing is folded in.mrveiss commentedon Oct 2, 2026
Decision revised (owner, 2026-10-02): no per-conversation limit
A pre-push review found that
conversation_rate_limiter, keyed by a conversation id taken from the request, would be acquired before the endpoint's ownership check. Any authenticated user could POST 60/min against another user'schat_idand lock the owner out of their own conversation with 429s. The ownership check cannot simply run first, because its log-only feature-flag mode does not refuse. The owner chose to drop the per-conversation limit: the per-user limit already bounds how fast any one caller sends to any conversation.utils/conversation_rate_limiter.pynow states in the module why the instance is intentionally unwired, which is this issue's own resolution option.So the outcome per AC:
user_rate_limiter: wired, per user, on the 7 chat message-send endpoints only.conversation_rate_limiter: not wired, reason recorded in the module (lockout, above).llm_rate_limiter: not wired, reason recorded in the module (LLMCrossWorkerRateLimiteris the authority; itsstream_completiongap is on Per-provider concurrency cap does not cover stream_completion() — only chat_completion() is bounded #16538).test(rate-limiting): pin each send endpoint's runtime name against th…
test(rate-limiting): walk api.chat with effective_routes, not .routes (…
fix(rate-limiting): enforce the per-user limit on chat sends, and rec…
mrveiss commentedon Oct 2, 2026
Closure evidence: #17874 merged as
6b3e37958b; every AC verified againstorigin/mainorigin/mainuser_rate_limiteracquired on a real per-user request pathautobot-backend/api/chat_send_rate_limit.pyenforce_chat_send_rate_limitcallsuser_rate_limiter.acquire(...)for POSTs to the 7SEND_ENDPOINTS. It is mounted on the chat router byinitialization/router_registry/route_dependencies.pythroughapp_factory.py:91(include_router(..., dependencies=deps)).conversation_rate_limiter: wired, or the reason it cannot be is recordedutils/conversation_rate_limiter.py, "NOT ACQUIRED ANYWHERE, ON PURPOSE"). Keyed by a request-supplied id ahead of an ownership check that has a non-refusing mode, it would be a cross-user lockout. TheConversationRateLimiterclass used byclaude_api_integration.pyis untouched.llm_rate_limiteracquired, or its relationship toLLMCrossWorkerRateLimitersettledllm_shared/optimization/rate_limiter.py, "NOT ACQUIRED ANYWHERE, ON PURPOSE"): provider pacing isLLMCrossWorkerRateLimiter. Itsstream_completiongap is on #16538.autobot-backend/api/chat_send_rate_limit_test.py::test_a_user_past_the_limit_is_refused_with_retry_after: the shared limiter's real Lua window on fakeredis, asserting HTTP 429 withRetry-After, not a call toacquire. Plus: the owner is not locked out by another user's flood; a non-send route is never limited; endpoint names are pinned in source and at runtime througheffective_routes().user_management/middleware/rate_limit.py:28,utils/conversation_rate_limiter.py:366,llm_shared/optimization/rate_limiter.py:398.Merge gate at head
07199b1412: 11 of 11 required contexts SUCCESS (matched as positives withcomm -12), 0 behind, 0 unresolved threads.mrveiss commentedon Oct 2, 2026
Verification: this closure is correct. The uncalled singleton is uncalled on purpose, and the reason is recorded in the code.
A duplicate/false-closure sweep flagged this issue as "closed while its defect is still live", on the grounds that
llm_rate_limiterhas no caller. It does have no caller. That is not the defect — it is the resolution, and this issue is where the decision was made.At the declaration site on
origin/main(autobot-backend/llm_shared/optimization/rate_limiter.py:388-398):The other two singletons are genuinely wired. Verified against
origin/main, following the alias rather than grepping the name:integration_rate_limiterintegrations/communication_integration.py:244,integrations/github_integration.py:292_shared_rate_limiter(:34/:35), thenawait _shared_rate_limiter.acquire(...).github_integration_test.py:202-208and:305-313assert the Redis-backed limiter is the one usedgateway_rate_limiterservices/gateway/message_queue.py:94await gateway_rate_limiter.acquire_token(...), declared:30llm_rate_limiterget_llm_rate_limiter()atbase_provider.py:182So all three named items are resolved: two wired, one deliberately not, with the supersession documented where a reader meets the symbol.
Why the sweep reached the wrong verdict
The grep was correct and complete:
llm_rate_limiterappears exactly twice in the whole tree onorigin/main— its__all__entry at:382and its declaration at:398. Nothing else references it. The absence was real and the inference from it was wrong, because an uncalled symbol can be uncalled by decision, and only the surrounding comment distinguishes that from a gap. The sweep that found it had caveated its own single-pattern grep and asked for confirmation before anything acted on it, which is why this is a comment and not a reopen.The live residual, already tracked
The same in-code note names a real remaining gap and it is not this issue: #16538 —
stream_completionacquires neither the cross-worker limiter nor the concurrency cap. Anyone arriving here looking for live rate-limiting work wants that one.One stale reference worth knowing about
#17217 lists this issue in its pattern table as
Three rate-limiter singletons | Defined, never called. Two of the three are now called, and the third is a ruled decision rather than an unexecuted control, so that row no longer describes the code. Flagging it rather than editing another umbrella table; nothing here needs reopening.No state changed, nothing reopened.