Prefix Redis keys with docket name for ACL isolation (2.x backport)#2812
Prefix Redis keys with docket name for ACL isolation (2.x backport)#2812jlowin merged 1 commit intorelease/2.xfrom
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: 31569f8ea9
ℹ️ 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" | ||
| ) |
There was a problem hiding this comment.
Add fallback for legacy task metadata keys
Using docket.key(...) for task metadata lookups means tasks/get, tasks/result, and tasks/cancel will no longer find tasks created by pre-5935d0c instances because those were stored under the unprefixed fastmcp:task:... keys. During a rolling upgrade or immediately after deploying this change, any in-flight tasks created by older servers will surface as “task not found,” even though the execution still exists in Docket. Consider a migration or a fallback lookup to the legacy key to keep upgrades non-disruptive.
Useful? React with 👍 / 👎.
Test Failure AnalysisSummary: The Windows test suite times out in after approximately 5 seconds. Root Cause: Test timeout at - specifically around line 920 during OAuth proxy testing. The pytest timeout is set to 5 seconds globally (visible in test output: Context: This PR changes how Redis keys are prefixed by using Suggested Solution: The test failure is in Possible causes:
Recommended actions:
Detailed AnalysisLog excerpt showing timeout: The test was running at approximately 35% completion when it timed out (running for ~2 minutes total, timing out after the 9th passing test in the file). Files changed in this PR that affect Redis:
Key changes example: # Before: fastmcp:task:{session_id}:{server_task_id}
# After: {docket_name}:fastmcp:task:{session_id}:{server_task_id}
task_meta_key = docket.key(f"fastmcp:task:{session_id}:{server_task_id}")Related FilesFiles relevant to the failure:
Files modified in this PR:
|
Test Failure AnalysisSummary: The Windows test suite times out in Root Cause: Test timeout at Context: This PR changes how Redis keys are prefixed by using Suggested Solution: The test failure is in Possible causes:
Recommended actions:
Detailed AnalysisLog excerpt showing timeout: The test was running at approximately 35% completion when it timed out (running for ~2 minutes total, timing out after the 9th passing test in the file). Files changed in this PR that affect Redis:
Key changes example: # Before: fastmcp:task:{session_id}:{server_task_id}
# After: {docket_name}:fastmcp:task:{session_id}:{server_task_id}
task_meta_key = docket.key(f"fastmcp:task:{session_id}:{server_task_id}")Related FilesFiles relevant to the failure:
Files modified in this PR:
|
WalkthroughThe pull request systematically refactors Redis key generation across three task-related modules. In Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/fastmcp/server/tasks/subscriptions.py (1)
177-179: Consistent key prefixing applied.The change mirrors the pattern in
_send_status_notificationabove. Both functions now usedocket.key()for the created_at key construction.💡 Optional: Consider extracting the key pattern to reduce duplication
The key pattern
f"fastmcp:task:{session_id}:{task_id}:created_at"appears in both functions. While the current implementation works correctly, you could optionally extract this to a helper function for consistency:def _build_created_at_key(docket: Docket, session_id: str, task_id: str) -> str: return docket.key(f"fastmcp:task:{session_id}:{task_id}:created_at")This would centralize the key format definition, though given the limited scope, the current approach is acceptable.
src/fastmcp/server/tasks/handlers.py (1)
292-301: Consistent key prefixing for resource tasks.The changes complete the pattern across all three task handlers. All task metadata keys are now prefixed via
docket.key().💡 Optional: Consider extracting the key building pattern
The key pattern appears identically in all three handlers (lines 75-78, 186-189, 292-295). You could optionally extract this to a helper function:
def _build_task_redis_keys( docket: Docket, session_id: str, task_id: str ) -> tuple[str, str]: """Build Redis keys for task metadata storage.""" task_meta_key = docket.key(f"fastmcp:task:{session_id}:{task_id}") created_at_key = docket.key(f"fastmcp:task:{session_id}:{task_id}:created_at") return task_meta_key, created_at_keyThis would centralize the key format, though the current implementation is clear and works correctly.
src/fastmcp/server/tasks/protocol.py (1)
80-86: Consider migration strategy for in-flight tasks during deployment.This PR changes the Redis key structure from
fastmcp:task:...to{docket_name}:fastmcp:task:.... Tasks created before this deployment will not be accessible after the upgrade since their keys won't match the new prefixed format.For production deployments, consider:
- Documenting that in-flight tasks will be lost during the upgrade
- Timing the deployment during low-traffic periods
- Or implementing a temporary migration layer that checks both old and new key formats during a transition period
The ACL isolation benefit likely justifies this breaking change, but it's worth documenting the upgrade impact.
Also applies to: 181-183, 314-320
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pyproject.tomlis excluded by none and included by noneuv.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (3)
src/fastmcp/server/tasks/handlers.pysrc/fastmcp/server/tasks/protocol.pysrc/fastmcp/server/tasks/subscriptions.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/server/tasks/handlers.pysrc/fastmcp/server/tasks/protocol.pysrc/fastmcp/server/tasks/subscriptions.py
🧬 Code graph analysis (3)
src/fastmcp/server/tasks/handlers.py (4)
src/fastmcp/server/server.py (1)
docket(378-383)src/fastmcp/resources/resource.py (1)
key(149-156)src/fastmcp/resources/template.py (1)
key(221-228)src/fastmcp/server/context.py (1)
session_id(360-409)
src/fastmcp/server/tasks/protocol.py (4)
src/fastmcp/server/server.py (1)
docket(378-383)src/fastmcp/resources/resource.py (1)
key(149-156)src/fastmcp/resources/template.py (1)
key(221-228)src/fastmcp/server/context.py (1)
session_id(360-409)
src/fastmcp/server/tasks/subscriptions.py (2)
src/fastmcp/resources/resource.py (1)
key(149-156)src/fastmcp/resources/template.py (1)
key(221-228)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: label-issue-or-pr
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (7)
src/fastmcp/server/tasks/subscriptions.py (1)
104-106: LGTM - Consistent key prefixing for ACL isolation.The change correctly wraps the Redis key with
docket.key()to enable docket name prefixing. This aligns with the same pattern applied in handlers.py and protocol.py for task metadata keys.src/fastmcp/server/tasks/handlers.py (2)
75-84: LGTM - Task metadata keys correctly prefixed.The changes properly wrap both
task_meta_keyandcreated_at_keywithdocket.key(), ensuring they'll match the lookups in protocol.py. The TTL calculation and Redis storage semantics remain unchanged.
186-195: Consistent key prefixing for prompt tasks.The changes mirror the pattern in
handle_tool_as_task, correctly applyingdocket.key()wrapping to both metadata keys.src/fastmcp/server/tasks/protocol.py (4)
80-86: Correct key prefixing for task status lookup.The changes properly wrap both lookup keys with
docket.key(), ensuring they match the prefixed keys stored by handlers.py. Bothtask_meta_keyandcreated_at_keyare needed for returning complete task status per SEP-1686.
181-183: Correct key prefixing for task result lookup.The change correctly wraps the task metadata key lookup. Note that
created_at_keyis not needed here sincetasks/resultonly requires the task execution key, not the creation timestamp.
314-320: Consistent key prefixing for task cancellation.The changes correctly wrap both lookup keys with
docket.key(), matching the pattern intasks_get_handler. Both keys are needed since cancellation returns full task status includingcreatedAt.
1-10: All concerns verified. The pydocket requirement is correctly updated to >=0.16.4 in pyproject.toml, and thekey()method is available in pydocket 0.16.4+ as used throughout the task handlers and protocol code for prefixing Redis keys with the docket name for ACL isolation.
Backport of #2811 for the 2.x release branch.
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.🤖 Generated with Claude Code