Prefix Redis keys with docket name for ACL isolation#2811
Conversation
The Redis keys fastmcp uses directly (not via docket) weren't prefixed with the docket name, making ACL-based isolation impossible. Now uses docket.key() to prefix all task metadata keys consistently. Bumps pydocket requirement to >=0.16.4 for the new key() method. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccae21ab5f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| task_meta_key = docket.key(f"fastmcp:task:{session_id}:{client_task_id}") | ||
| created_at_key = docket.key( | ||
| f"fastmcp:task:{session_id}:{client_task_id}:created_at" | ||
| ) | ||
| poll_interval_key = docket.key( |
There was a problem hiding this comment.
Preserve access to pre-upgrade task metadata keys
Because _lookup_task_execution now builds task_meta_key via docket.key(...), lookups only hit the new {docket_name}:fastmcp:task:... namespace. Tasks submitted before this change stored metadata under the old fastmcp:task:... keys; after a rolling upgrade, tasks/get, tasks/result, and tasks/cancel will return “not found” until those tasks expire. Consider checking the legacy key as a fallback or migrating existing keys on startup to avoid breaking in-flight tasks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yep that's accurate, but I think this is an acceptable risk because most folks would be using the memory:// broker, and also these are fairly transient keys.
WalkthroughThe changes replace direct Redis key string construction with docket.key-wrapped keys across three files in the tasks module. In handlers.py, task metadata keys for created_at and poll_interval are updated to use docket.key generation. In requests.py, Redis key construction is reworked for task metadata lookups and MGET calls. In subscriptions.py, created_at timestamp retrieval uses docket.key-wrapped keys. The control flow and logic remain unchanged; only the key construction mechanism is updated. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The Redis keys fastmcp uses directly (not via docket) weren't prefixed with the docket name, making ACL-based isolation impossible. Now uses
docket.key()to prefix all task metadata keys consistently.Keys now look like
{docket_name}:fastmcp:task:{session_id}:{task_id}instead offastmcp:task:{session_id}:{task_id}.Bumps pydocket requirement to >=0.16.4 for the new
key()method.See also: #2812 (2.x backport)
🤖 Generated with Claude Code