Skip to content

feat: make RateLimitRegistry an injectable dependency (#26) - #42

Open
rcbevans wants to merge 22 commits into
mainfrom
fix/26-injectable-ratelimit-registry
Open

feat: make RateLimitRegistry an injectable dependency (#26)#42
rcbevans wants to merge 22 commits into
mainfrom
fix/26-injectable-ratelimit-registry

Conversation

@rcbevans

Copy link
Copy Markdown
Contributor

Closes #26.

What

RateLimitRegistry was a module-level singleton consumed by import: callers could not substitute it, tests could not isolate it structurally, and nothing in worker_main or create_router showed the dependency. This makes it an owned, injectable object while keeping the singleton as the default everywhere.

  • @actor(rate_limits=[...], reservations=[...]) accepts primitive instances (TokenBucket, SlidingWindow, ConcurrencyReservation) alongside names and keyed refs. The worker collects them at bootstrap and registers them into its registry before validate() runs. Same name + same config is a no-op, same name + different config is a ValueError at startup.
  • worker_main / _main / create_router take an optional rate_limit_registry. Resolution order: explicit argument, then a RateLimitRegistry value provider in the DI registry, then the module singleton. Factory/class providers and explicit+DI co-presence raise TypeError at bootstrap, before any pool I/O, because both would split bootstrap and dispatch onto different instances.
  • Keyed idle eviction moved out of the leader gate. Eviction is process-local bookkeeping, so every worker now sweeps its own registry each 30s tick. With the singleton default this changes nothing; it also fixes non-leader processes never sweeping.
  • Admin handlers resolve the registry via Depends(get_rl_registry) from app.state, falling back to the singleton, so hand-assembled setups keep working.
  • New RateLimitRegistry.clear() test aid that resets all six state fields including the opportunistic-eviction scan timestamps. conftest uses it instead of poking private dicts.
  • Docs rewritten around the ownership model: declare on the actor by default, own an instance for shared or non-job use, singleton as the convenience case. Testing section documents the fresh-instance pattern.

Fully backwards compatible. Import-time .register() plus worker_main(...) with no argument behaves exactly as before, and there is a regression test for it.

Also in the diff: required-version for uv is now a range (>=0.11.19,<0.12) instead of an exact pin, so current uv releases work.

Test plan

  • 3842 unit tests pass; ruff check, ruff format, pyright (src and tests) clean
  • New integration tests against real Postgres (testcontainers): actor-declared collection pass, DI value provider resolution, singleton backwards-compat regression
  • New unit coverage: resolution order matrix, clear(), instance normalization and KeyError, Phase 2b instance validation, non-leader sweep eviction, admin owned/singleton/reset paths, bootstrap conflict and queue-prefix ValueErrors, end-to-end dispatch with an actor-declared instance

@XBeg9
XBeg9 self-requested a review July 28, 2026 17:58
@rcbevans
rcbevans requested review from clinzy and kjw-azx July 28, 2026 18:17
@rcbevans rcbevans self-assigned this Jul 28, 2026
@rcbevans
rcbevans force-pushed the fix/26-injectable-ratelimit-registry branch from b30d92f to 995c303 Compare July 28, 2026 19:03

@XBeg9 XBeg9 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.

comments inline

return explicit
if di_registry.has_provider(RateLimitRegistry):
entry = di_registry.get(RateLimitRegistry)
if entry.kind != "value":

@XBeg9 XBeg9 Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

entry.kind is checked, entry.scope isn't. Register a value provider at PROCESS scope and rate limiting silently stops working.

  • guard passes, you return entry.impl
  • register_rate_limit_registry returns early on has_provider, so the LOOP registration never happens
  • LoopScope.bootstrap only resolves scope is Scope.LOOP
  • dispatch.py:246 gets None
  • _consumer.py:298 _needs_acquire is False

Bootstrap, validate and bucket-sync all run against the injected registry, so nothing looks broken.

Test: actor with TokenBucket(capacity=1), frozen clock, registry at PROCESS scope. Both jobs ran, zero snoozes, tokens_remaining=1. The bucket was never touched, so acquire didn't run at all. Same harness at Scope.LOOP denies the second job.

Factory/class providers already get a TypeError for this exact reason: "bootstrap using one instance while LOOP-scope dispatch resolution produced another". Scope has the same failure mode and no guard.

The has_provider early-return isn't new. What's new is DI injection being a documented resolution path, which puts this behind the public API.

Reject non-LOOP with the same TypeError, or re-register the resolved instance at LOOP. I have the red test if it's useful — in-process, includes the LOOP control.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verified the chain end to end and went with rejection.

_resolve_rl_registry now checks scope next to kind: a value provider at anything other than Scope.LOOP raises TypeError before pools open.

Rejection over re-registering at LOOP: ProviderRegistry is keyed by type only, one entry per type, so re-registering would mean mutating the caller's declared scope. Non-LOOP is never correct here because dispatch resolves the registry from the LOOP cache only (dispatch.py:246). PROCESS scope is worse than the factory case in one respect: actor injection of RateLimitRegistry would resolve from the PROCESS cache while automatic acquire around dispatch stays off, so the failure is even less visible.

Tests: parametrized unit test over PROCESS/THREAD/TRANSIENT (every non-LOOP member of the Scope enum) asserting TypeError, plus a _main test proving the raise precedes open_worker_deps. The LOOP control already exists in test_di_value_provider_wins_over_singleton. 3609 non-integration tests pass; ruff and pyright clean.

Covered without the red test: the parametrized guard tests hit the same gap, and the frozen-clock scenario now fails at bootstrap instead of reaching dispatch. The rate-limiting guide's ownership section also documents the DI value-at-LOOP path now.

provider is already registered — a user pre-registered value provider
wins (bootstrap resolution rule 2); after the bootstrap
``kind == "value"`` check the DI-cached instance and the bootstrap
instance are provably the same object. When the module singleton is

@XBeg9 XBeg9 Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only true at LOOP scope. At PROCESS scope there is no DI-cached instance, so bootstrap holds one object and dispatch holds None.

This line currently asserts the invariant that's broken. Worth rewording whenever the guard lands.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reworded in d28e099 alongside the guard: it now says a pre-registered LOOP-scope value provider wins and cites the bootstrap kind/scope checks. With non-LOOP rejected in _resolve_rl_registry, the invariant this line asserts holds again.

Comment thread pyproject.toml Outdated
# while <0.12 keeps CI and local on the same validated minor line.
# Trade-off vs. an exact pin: setup-uv must consult the versions manifest to
# resolve the range (the fetch an exact pin skips); accepted per review.
# Version range: setup-uv in CI resolves versions without fetching the

@XBeg9 XBeg9 Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is backwards. From setup-uv v9 src/version/resolve.ts:

ExactVersionResolver.resolve() returns parsedSpecifier.normalized and stops. RangeVersionResolver.resolve() calls findHighestSatisfyingVersion(), which fetches the manifest. >=0.11.19,<0.12 is kind === "range", so it takes the second path.

The range is what triggers the raw.githubusercontent.com fetch. v7.5.0 release notes say the same: "latest and exact versions such as 0.5.0 skipped version resolution entirely".

The value didn't change in this diff either, so a comment edit can't be what fixed the CI failures it mentions.

An exact pin doesn't get you off that host entirely — getArtifact() reads the same manifest on a tool-cache miss. One less fetch, not zero.

The previous comment had it right.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Checked against setup-uv resolve.ts: ExactVersionResolver returns the normalized version without a fetch, RangeVersionResolver goes through findHighestSatisfyingVersion and the manifest. >=0.11.19,<0.12 takes the range path, so the comment had it backwards. Pin removed in a702a68 rather than reworded: it hard-failed local uv on any other version, and the manifest is still read on a tool-cache miss, so the protection was partial.

assert len(acquired) == 1


async def test_di_value_provider_wins_over_singleton_end_to_end(pg_dsn: str, schema: str) -> None:

@XBeg9 XBeg9 Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mutated dispatch to resolve None, which is the end state of the scope bug, then ran the full integration tier. 969 passed. Nothing in this file noticed. The three that caught it are in test_dispatch_one_job.py, which actually dispatch a job.

Two things this is not:

It's not a vacuous test. Mutating bootstrap to ignore the injected provider does fail here, with MissingProvider at validate(). So it pins which registry bootstrap resolved. It just says nothing about which one dispatch ends up with.

And it's not an untested feature — the dispatch unit tests cover that.

The real gap is narrow: every register_value(RateLimitRegistry, ...) in the suite uses Scope.LOOP, so nothing exercises the config that breaks. One test that injects at PROCESS scope and then dispatches would close this and the _bootstrap.py issue together.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Closed by the rejection route: a non-LOOP value provider raises TypeError at bootstrap, so injecting at PROCESS scope and then dispatching is unreachable config. Covered by test_di_value_provider_non_loop_scope_raises_typeerror parametrized over PROCESS/THREAD/TRANSIENT, plus test_main_raises_typeerror_for_process_scope_provider_before_opening_pools, which proves the raise precedes open_worker_deps.

Comment thread examples/app.py
pg_pool = getattr(request.app.state, "pg_pool", None)

from taskq.ratelimit.registry import registry as rl_registry
from taskq.settings import WorkerSettings

@XBeg9 XBeg9 Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Local import is gone, so rl_registry on 457 now resolves to the module-level one from :135 via LEGB. Works, but it quietly changed which registry this endpoint reads.

Separate thing in the same file: this builds its own registry and re-registers primitives that examples/actors/ratelimit.py puts on the singleton. Under backend="memory" those are two independent states. Examples get copied into real deployments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both deliberate. peek_rate_limits reads the same owned registry passed to create_router, so the endpoint and /taskq agree. The import-time register() calls in actors/ratelimit.py serve examples/worker.py, which calls worker_main without rate_limit_registry and resolves the singleton; app.py is a separate process, so under backend="memory" its state is independent of the worker's either way. The duplicate in the app process is dead weight, noted at app.py:128-134. If you want the examples to demonstrate a single pattern, the clean version is worker.py taking an owned registry with actor-declared instances and dropping the singleton registration; can do it here or as a follow-up.

@rcbevans
rcbevans requested a review from XBeg9 July 28, 2026 21:26
@rcbevans
rcbevans force-pushed the fix/26-injectable-ratelimit-registry branch from a702a68 to 509cc5f Compare July 29, 2026 04:23
rcbevans added 21 commits July 29, 2026 21:44
@rcbevans
rcbevans force-pushed the fix/26-injectable-ratelimit-registry branch from 509cc5f to ab02e4b Compare July 30, 2026 04:58
@rcbevans
rcbevans force-pushed the fix/26-injectable-ratelimit-registry branch from d8667e2 to 758ac78 Compare July 30, 2026 05:33
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 RateLimitRegistry an injectable dependency rather than a module-level singleton

2 participants