Skip to content

Refactor task module: rename protocol.py to requests.py and reduce redundancy#2667

Merged
jlowin merged 1 commit intomainfrom
refactor/task-module-cleanup
Dec 21, 2025
Merged

Refactor task module: rename protocol.py to requests.py and reduce redundancy#2667
jlowin merged 1 commit intomainfrom
refactor/task-module-cleanup

Conversation

@jlowin
Copy link
Copy Markdown
Member

@jlowin jlowin commented Dec 21, 2025

Improves clarity and reduces duplication in the task module by renaming protocol.pyrequests.py and consolidating Redis lookup logic.

Changes

Renamed for clarity:

  • src/fastmcp/server/tasks/protocol.pyrequests.py
  • More descriptive: these handlers process MCP task requests (tasks/get, tasks/result, tasks/cancel, tasks/list)

Consolidated constants:

  • Moved DEFAULT_POLL_INTERVAL_MS and DEFAULT_TTL_MS to config.py
  • DEFAULT_POLL_INTERVAL_MS is now derived from DEFAULT_POLL_INTERVAL instead of duplicated
  • Updated TaskConfig to reference the constant

Reduced redundancy:

  • Extracted _lookup_task_execution() helper function
  • Eliminates ~50 lines of duplicated Redis lookup code across handlers
  • Performance improvement: uses redis.mget() for single round-trip instead of 3 separate gets

All 198 task tests pass.

…dundancy

Renamed protocol.py to requests.py for clarity - it handles MCP task
request endpoints (tasks/get, tasks/result, tasks/cancel, tasks/list).

Consolidated constants to config.py as single source of truth:
- DEFAULT_POLL_INTERVAL_MS now derived from DEFAULT_POLL_INTERVAL
- TaskConfig uses constant instead of hardcoded timedelta(seconds=5)

Extracted _lookup_task_execution() helper to eliminate ~50 lines of
duplicated Redis lookup code. Uses redis.mget() for single round-trip
instead of 3 separate calls (performance improvement).
@marvin-context-protocol marvin-context-protocol Bot added the enhancement Improvement to existing functionality. For issues and smaller PR improvements. label Dec 21, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 21, 2025

Walkthrough

This pull request refactors task handler configuration and imports within the task server module. It introduces three new module-level constants in the config file—DEFAULT_POLL_INTERVAL, DEFAULT_POLL_INTERVAL_MS, and DEFAULT_TTL_MS—to centralize task defaults. A new internal helper function _lookup_task_execution is added to consolidate Redis queries for task metadata and execution. Task handlers are updated to use this helper and the new constants instead of hard-coded values. Additionally, import paths are reorganized across the tasks module, relocating task protocol handlers and configuration symbols to different source files.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description covers the main changes (rename, consolidation, redundancy reduction) and includes testing confirmation, but is missing the Contributors and Review Checklist sections required by the template. Complete the Contributors Checklist (issue reference, workflow, testing, documentation) and Review Checklist (self-review, ready for review) as specified in the template.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main refactoring work: renaming protocol.py to requests.py and consolidating redundant code in the task module.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/task-module-cleanup

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/fastmcp/server/tasks/requests.py (3)

42-46: Consider adding type hints for docket and return type.

The docket parameter and the first element of the return tuple are typed as Any. While this may be necessary to avoid circular imports, consider using string literals for forward references or importing under TYPE_CHECKING for better type safety.

🔎 Suggested improvement
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from docket import Docket
    from docket.execution import Execution

async def _lookup_task_execution(
    docket: "Docket",
    session_id: str,
    client_task_id: str,
) -> tuple["Execution", str | None, int]:

213-235: Consider using _lookup_task_execution helper here as well.

This handler still performs its own Redis lookup for task_key and execution, duplicating logic that exists in _lookup_task_execution. While the handler doesn't need created_at or poll_interval_ms, you could still use the helper and discard the unused values, or extract a smaller helper for just the task_key → execution lookup.

This is a minor consistency suggestion that could be addressed in a follow-up.


403-405: Potential ValueError if created_at is malformed.

datetime.fromisoformat() can raise ValueError if created_at contains an invalid ISO format string. While the value is stored by your own code and should be well-formed, consider defensive handling for robustness.

🔎 Suggested fix
-            createdAt=datetime.fromisoformat(created_at)
-            if created_at
-            else datetime.now(timezone.utc),
+            createdAt=(
+                datetime.fromisoformat(created_at)
+                if created_at
+                else datetime.now(timezone.utc)
+            )
+            if not created_at or _is_valid_isoformat(created_at)
+            else datetime.now(timezone.utc),

Or more simply, wrap in try/except:

try:
    created_at_dt = datetime.fromisoformat(created_at) if created_at else datetime.now(timezone.utc)
except ValueError:
    created_at_dt = datetime.now(timezone.utc)
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7ff23a0 and 798c6c5.

📒 Files selected for processing (4)
  • src/fastmcp/server/server.py (1 hunks)
  • src/fastmcp/server/tasks/config.py (2 hunks)
  • src/fastmcp/server/tasks/requests.py (8 hunks)
  • src/fastmcp/server/tasks/subscriptions.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Use Python ≥ 3.10 with full type annotations
Never use bare except - be specific with exception types

Files:

  • src/fastmcp/server/tasks/config.py
  • src/fastmcp/server/tasks/requests.py
  • src/fastmcp/server/tasks/subscriptions.py
  • src/fastmcp/server/server.py
🧠 Learnings (2)
📚 Learning: 2025-12-21T21:37:55.015Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.015Z
Learning: Applies to src/fastmcp/__init__.py : All module exports should be intentional - only re-export to `fastmcp.*` for fundamental types like `FastMCP` and `Client`, prefer users importing from specific submodules for specialized features

Applied to files:

  • src/fastmcp/server/tasks/subscriptions.py
  • src/fastmcp/server/server.py
📚 Learning: 2025-12-21T21:37:55.015Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.015Z
Learning: Applies to src/fastmcp/**/__init__.py : Core types that define a module's purpose should be exported (e.g., `Middleware` from `fastmcp.server.middleware`), while specialized features can live in submodules

Applied to files:

  • src/fastmcp/server/server.py
🧬 Code graph analysis (1)
src/fastmcp/server/tasks/requests.py (3)
src/fastmcp/server/server.py (1)
  • docket (382-387)
src/fastmcp/server/context.py (1)
  • session_id (360-409)
src/fastmcp/client/tasks.py (1)
  • status (171-199)
⏰ 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). (4)
  • GitHub Check: Run tests with lowest-direct dependencies
  • GitHub Check: Run tests: Python 3.10 on ubuntu-latest
  • GitHub Check: Run tests: Python 3.10 on windows-latest
  • GitHub Check: Run tests: Python 3.13 on ubuntu-latest
🔇 Additional comments (8)
src/fastmcp/server/tasks/subscriptions.py (1)

16-16: LGTM!

The import path update correctly reflects the module rename from protocol.py to requests.py. The DOCKET_TO_MCP_STATE symbol is now sourced from the consolidated requests module.

src/fastmcp/server/server.py (1)

609-614: LGTM!

The import path update correctly reflects the module rename from protocol.py to requests.py. The handler registration logic remains unchanged.

src/fastmcp/server/tasks/config.py (2)

18-21: Good refactor: Centralized task defaults.

The new constants consolidate task metadata defaults in a single location, improving maintainability. Deriving DEFAULT_POLL_INTERVAL_MS from DEFAULT_POLL_INTERVAL ensures consistency between the two representations.


55-55: LGTM!

Using the constant DEFAULT_POLL_INTERVAL instead of an inline timedelta(seconds=5) ensures the default is defined in one place.

src/fastmcp/server/tasks/requests.py (4)

1-5: LGTM!

The updated docstring accurately describes the module's purpose and clarifies that these handlers query/manage existing tasks.


68-72: Good optimization: Batch Redis fetch.

Using redis.mget() to fetch all three keys in a single round-trip is a good performance improvement over separate get() calls.


139-142: LGTM!

The handler now uses the centralized _lookup_task_execution helper, reducing code duplication and ensuring consistent lookup logic.


388-395: LGTM on the helper usage and cancellation logic.

The handler now uses the centralized _lookup_task_execution helper, and correctly uses execution.key for the cancellation call. This aligns with the PR objective of reducing redundant Redis lookup logic.

@jlowin jlowin merged commit 3afdb3f into main Dec 21, 2025
14 checks passed
@jlowin jlowin deleted the refactor/task-module-cleanup branch December 21, 2025 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to existing functionality. For issues and smaller PR improvements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant