feat: make RateLimitRegistry an injectable dependency (#26) - #42
feat: make RateLimitRegistry an injectable dependency (#26)#42rcbevans wants to merge 22 commits into
Conversation
b30d92f to
995c303
Compare
| return explicit | ||
| if di_registry.has_provider(RateLimitRegistry): | ||
| entry = di_registry.get(RateLimitRegistry) | ||
| if entry.kind != "value": |
There was a problem hiding this comment.
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_registryreturns early onhas_provider, so the LOOP registration never happensLoopScope.bootstraponly resolvesscope is Scope.LOOPdispatch.py:246gets None_consumer.py:298_needs_acquireis 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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| pg_pool = getattr(request.app.state, "pg_pool", None) | ||
|
|
||
| from taskq.ratelimit.registry import registry as rl_registry | ||
| from taskq.settings import WorkerSettings |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a702a68 to
509cc5f
Compare
… semantics in bootstrap
…eclared collection pass
…min_state fallback
…ge for actor-declared primitives
509cc5f to
ab02e4b
Compare
…f module singleton
d8667e2 to
758ac78
Compare
Closes #26.
What
RateLimitRegistrywas a module-level singleton consumed by import: callers could not substitute it, tests could not isolate it structurally, and nothing inworker_mainorcreate_routershowed 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 beforevalidate()runs. Same name + same config is a no-op, same name + different config is aValueErrorat startup.worker_main/_main/create_routertake an optionalrate_limit_registry. Resolution order: explicit argument, then aRateLimitRegistryvalue provider in the DI registry, then the module singleton. Factory/class providers and explicit+DI co-presence raiseTypeErrorat bootstrap, before any pool I/O, because both would split bootstrap and dispatch onto different instances.Depends(get_rl_registry)fromapp.state, falling back to the singleton, so hand-assembled setups keep working.RateLimitRegistry.clear()test aid that resets all six state fields including the opportunistic-eviction scan timestamps. conftest uses it instead of poking private dicts.Fully backwards compatible. Import-time
.register()plusworker_main(...)with no argument behaves exactly as before, and there is a regression test for it.Also in the diff:
required-versionfor uv is now a range (>=0.11.19,<0.12) instead of an exact pin, so current uv releases work.Test plan
clear(), instance normalization andKeyError, Phase 2b instance validation, non-leader sweep eviction, admin owned/singleton/reset paths, bootstrap conflict and queue-prefixValueErrors, end-to-end dispatch with an actor-declared instance