Repository navigation
Read omitted argument defaults for per-argument Cooldown, Debounce, RateLimit, and ConcurrencyLimit - #585
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f13fc35f73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def get_argument(self, parameter: str) -> Any: | ||
| signature = get_signature(self.function) | ||
| bound_args = signature.bind(*self.args, **self.kwargs) | ||
| bound_args.apply_defaults() |
There was a problem hiding this comment.
When the named parameter is omitted and backed by a dependency (for example, customer_id: int = Depends(resolve_customer)), apply_defaults() inserts the dependency marker rather than the value that dependency resolution later passes to the task. Both legacy and annotated ConcurrencyLimit paths call get_argument(), so executions whose dependencies resolve to different customer IDs are keyed on the same marker and incorrectly share one concurrency bucket; concurrency needs to consume the already-resolved parameter value instead of the raw signature default.
Useful? React with 👍 / 👎.
…ateLimit, and ConcurrencyLimit A per-argument Cooldown, Debounce, or RateLimit keyed on None when the caller left out an argument that has a default, so a call with the default and a call passing the same value explicitly landed in different windows. A per-argument ConcurrencyLimit failed the task with a ValueError in the same situation. The resolver now falls back to the parameter's declared default, and Execution.get_argument applies defaults before looking the argument up. Closes chrisguidry#496
f13fc35 to
4a1c7f0
Compare
…limits docket-rs's per-field Cooldown, Debounce, and RateLimit keyed a field that the message left out as null, and ConcurrencyLimit failed the task, even when the task's type gives the field a serde default and the handler runs with it. This is the same gap that pydocket closes for omitted arguments with defaults (chrisguidry#496). The worker now reads each message's arguments through the task's type: a field the message left out takes the value the handler runs with, and the fields the message wrote stay as written, so pydocket and docket-rs key them the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P7t1QdrNx5qwTkqb6CVkT4
The `rust/v0.4.1` release needs the manifest at 0.4.1, because the publish job refuses a tag that does not match `rust/Cargo.toml`. The release reads a task type's serde defaults for the fields a message leaves out in the per-argument behaviors, from #585, and picks up the crate updates from #576 and #586. Both are fixes, so it bumps the patch version. The exact pin on docket-rs-macros changes with it, and the README's "0.4" install lines already cover 0.4.1. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01P7t1QdrNx5qwTkqb6CVkT4 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Closes #496
When a caller leaves out an argument that has a default,
signature.bind()doesn't include it, so the per-argument dependencies never saw the value the task actually runs with:Cooldown,Debounce, andRateLimitkeyed onNone, soadd(task)(1)andadd(task)()landed in different windows even though both run withcustomer_id=1.ConcurrencyLimit(both theAnnotatedform and the parameter-default form) raisedValueError: ConcurrencyLimit argument 'customer_id' not foundand the task failed without running.Changes:
resolved_dependencies, the annotation loop now falls back to the parameter's declared default afterprovidedandarguments. I didn't callapply_defaults()in_provided_arguments, since (as the issue notes) that puts dependency defaults intoprovidedand skips resolving task-level limits.Execution.get_argumentcallsapply_defaults()before the lookup. It only returns the one named argument, so this doesn't affect dependency resolution.Tests: added the reproductions from the issue to
test_cooldown.py,test_debounce.py,test_ratelimit.py, andconcurrency_limits/test_annotated.py(plus one for theConcurrencyLimit("customer_id", ...)default form). All five failed before the change and pass after.REDIS_VERSION=memory uv run pytestpasses (1145 passed, 94 skipped; the changed source files are fully covered), anduv run prek runon the touched files passes. I didn't run the Docker Redis matrix locally.