Skip to content

refactor: move task attribute to function-based variants only [SEP-1686]#2560

Merged
jlowin merged 2 commits intomainfrom
refactor/task-attr-function-variants
Dec 6, 2025
Merged

refactor: move task attribute to function-based variants only [SEP-1686]#2560
jlowin merged 2 commits intomainfrom
refactor/task-attr-function-variants

Conversation

@jlowin
Copy link
Copy Markdown
Member

@jlowin jlowin commented Dec 6, 2025

The task attribute for SEP-1686 background task support was defined on base classes (Tool, Prompt, Resource, ResourceTemplate) but only function-based variants can actually be registered with Docket since they're the only ones with an fn callable. This led to hasattr(obj, "fn") guards in _docket_lifespan().

This PR moves task to the function-based subclasses only (FunctionTool, FunctionPrompt, FunctionResource, FunctionResourceTemplate) and uses proper isinstance() checks:

for tool in self._tool_manager._tools.values():
    if isinstance(tool, FunctionTool) and tool.task:
        docket.register(tool.fn)

Also moves create_resource() from ResourceTemplate to FunctionResourceTemplate since it depends on task.

@jlowin jlowin changed the title refactor: move task attribute to function-based variants only refactor: move task attribute to function-based variants only [SEP-1686] Dec 6, 2025
@jlowin jlowin added this to the MCP 11/25/25 milestone Dec 6, 2025
@marvin-context-protocol marvin-context-protocol Bot added enhancement Improvement to existing functionality. For issues and smaller PR improvements. server Related to FastMCP server implementation or server-side functionality. labels Dec 6, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 6, 2025

Walkthrough

The refactoring consolidates the task field (indicating background task execution support) from base classes to their function-specific variants: FunctionTool, FunctionPrompt, FunctionResource, and FunctionResourceTemplate. Background task handling is now routed through these derived classes instead of the base types. Server routing logic is updated to use isinstance checks instead of hasattr checks for determining background task support. The read and existing core behaviors remain unchanged across all modified modules.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The PR description is incomplete; it lacks required contributors and review checklists specified in the template, though it provides clear context about the refactoring changes. Complete the PR description by adding the contributors checklist with issue reference and confirmation of testing/documentation, plus the review checklist items.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main refactoring: moving the task attribute from base classes to function-based variants only, with a reference to the relevant SEP.
✨ 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-attr-function-variants

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.

@jlowin jlowin merged commit 1ed976b into main Dec 6, 2025
10 of 11 checks passed
@jlowin jlowin deleted the refactor/task-attr-function-variants branch December 6, 2025 02:05
@marvin-context-protocol
Copy link
Copy Markdown
Contributor

Test Failure Analysis

Summary: The test_task_cancellation_workflow test is failing because the cancellation handler doesn't wait for the state to propagate before returning.

Root Cause: In src/fastmcp/server/tasks/protocol.py, the tasks_cancel_handler (line 274-355) calls await docket.cancel(task_key) at line 342 and immediately returns a CancelTaskResult with status="cancelled". However, it doesn't verify that the cancellation has actually taken effect in Docket's execution state.

When the test then calls task.status(), which invokes tasks_get_handler, that handler syncs the execution state from Redis (line 110) and checks the actual Docket execution state. If the cancellation hasn't fully propagated through Redis yet, the execution might still be in RUNNING state, which maps to "working" status.

This is a race condition that's more likely to occur under certain timing conditions (like Python 3.10 in CI vs Python 3.12 locally).

Suggested Solution: After calling docket.cancel(), the handler should:

  1. Retrieve the execution object
  2. Call await execution.sync() to ensure the cancellation has propagated
  3. Verify the state is actually CANCELLED before returning

Specifically, modify tasks_cancel_handler around line 341-342:

# Cancel via Docket (now sets CANCELLED state natively)
await docket.cancel(task_key)

# Get execution and sync to ensure cancellation has propagated
execution = await docket.get_execution(task_key)
if execution:
    await execution.sync()
    # Verify it's actually cancelled
    mcp_state = DOCKET_TO_MCP_STATE.get(execution.state, "cancelled")
else:
    mcp_state = "cancelled"

Then use mcp_state in the return statement instead of hardcoding "cancelled".

Detailed Analysis

Failure Log:

FAILED tests/server/tasks/test_task_methods.py::test_task_cancellation_workflow - AssertionError: assert 'working' == 'cancelled'

Test Code (tests/server/tasks/test_task_methods.py:157-174):

async def test_task_cancellation_workflow(endpoint_server):
    async with Client(endpoint_server) as client:
        task = await client.call_tool("slow_tool", {}, task=True)
        await asyncio.sleep(0.1)
        await task.cancel()  # Calls tasks_cancel_handler
        await asyncio.sleep(0.1)
        status = await task.status()  # Calls tasks_get_handler
        assert status.status == "cancelled"  # FAILS: status is "working"

The test expects that after cancellation and a brief wait, the status should be "cancelled". However, tasks_get_handler syncs from Redis and returns the actual Docket execution state, which hasn't been updated yet.

Comparison with tasks_get_handler:
The tasks_get_handler (lines 45-139) properly syncs the execution state before returning:

execution = await docket.get_execution(task_key)
await execution.sync()  # Line 110
mcp_state = DOCKET_TO_MCP_STATE.get(execution.state, "failed")  # Line 113

The tasks_cancel_handler should follow the same pattern.

Related Files
  • src/fastmcp/server/tasks/protocol.py:274-355 - The tasks_cancel_handler that needs fixing
  • src/fastmcp/server/tasks/protocol.py:45-139 - The tasks_get_handler that shows the correct pattern (sync before reading state)
  • tests/server/tasks/test_task_methods.py:157-174 - The failing test

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. server Related to FastMCP server implementation or server-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant