Skip to content

fix(everything): honor terminal task status reached on the final poll - #4811

Open
BlueX888 wants to merge 1 commit into
modelcontextprotocol:mainfrom
BlueX888:fix/prep-everything-async-task-poll-off-by-one-timeout
Open

BlueX888 wants to merge 1 commit into
modelcontextprotocol:mainfrom
BlueX888:fix/prep-everything-async-task-poll-off-by-one-timeout

Conversation

@BlueX888

Copy link
Copy Markdown

Description

The async sampling and elicitation demo tools in the everything server report a
timeout for a task that reached a terminal status on the last poll attempt the
loop is allowed to make. This fixes the off-by-one in the timeout branch of both
tools and adds regression tests.

Server Details

  • Server: everything
  • Changes to: tools (trigger-sampling-request-async, trigger-elicitation-request-async)

Motivation and Context

Both tools poll the client's tasks/get endpoint in a bounded loop:

while (
  taskStatus !== "completed" &&
  taskStatus !== "failed" &&
  taskStatus !== "cancelled" &&
  attempts < MAX_POLL_ATTEMPTS
) { ... }

The loop makes at most MAX_POLL_ATTEMPTS polls, so the last poll it is allowed
to make can legitimately observe a terminal status. But the timeout branch
immediately after it tested only the attempt counter:

// src/everything/tools/trigger-sampling-request-async.ts:185
// src/everything/tools/trigger-elicitation-request-async.ts:189
if (attempts >= MAX_POLL_ATTEMPTS) {
  return { ... `[TIMEOUT] Task timed out after ${MAX_POLL_ATTEMPTS} poll attempts` ... };
}

When the terminal status arrives on poll number MAX_POLL_ATTEMPTS, attempts
has already reached the budget, so the tool returns [TIMEOUT] even though the
task is finished:

  • completed — the result the client already produced is discarded and
    tasks/result is never fetched, so the caller never sees it;
  • failed / cancelled — reported as a timeout instead of the real outcome,
    dropping the statusMessage the client supplied in tasks/get.

The timeout branch now mirrors the loop's own exit condition, so it only fires
when the status is still non-terminal.

How Has This Been Tested?

Regression tests were written first and run against the unpatched tree; all four
fail there and pass after the fix. The client mock keeps the task in a
non-terminal status (working / input_required) and only returns the terminal
status on the final allowed tasks/get, using vi.useFakeTimers() +
vi.runAllTimersAsync() so the run is deterministic and does not wait on real
poll intervals.

cd src/everything
../../node_modules/.bin/vitest run __tests__/tools.test.ts -t "final poll attempt"
# Test Files  1 passed (1)
#       Tests  4 passed | 62 skipped (66)

../../node_modules/.bin/vitest run --coverage
# Test Files  5 passed (5)
#       Tests  112 passed (112)

../../node_modules/.bin/tsc --noEmit
# exit 0

Cases covered:

  • sampling: completed on the final poll returns [COMPLETED] and fetches
    tasks/result; failed on the final poll returns [FAILED]; a task that
    never leaves a non-terminal status still times out after MAX_POLL_ATTEMPTS
    polls.
  • elicitation: completed on the final poll returns [COMPLETED] and fetches
    tasks/result; cancelled on the final poll returns [CANCELLED].

npm run prettier:check reports style issues in 59 pre-existing files (the repo
has no .prettierrc, while src/everything uses single quotes); the
pre-change tools.test.ts is flagged identically, and the two modified source
files pass the check on their own. Prettier is not part of the TypeScript CI
workflow, which runs npm test and npm run build.

Breaking Changes

None. The change only affects output for a task that finishes on the final poll
attempt, which previously always returned a timeout.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follow MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

No tracking issue: this was found by reading the poll loop and confirmed by
test. The same off-by-one exists in both async tools because the two files were
written from the same template, so it is fixed in both.

Unchecked checklist items: no README change is needed for a bug fix; the change
was not exercised against a live LLM client, only against a mocked client
session in unit tests; the fix adds no new error-handling path.

The poll loops in the async sampling and elicitation tools exit either on a
terminal status or when the attempt budget runs out, but the timeout branch
tested only the attempt count. A task that reached a terminal status on the
last allowed poll was reported as a timeout: `completed` threw away the result
the client had already produced (tasks/result was never fetched), and
`failed`/`cancelled` were masked by the same branch. Gate the timeout on the
status still being non-terminal, mirroring the loop's own exit condition.
Copilot AI balanced review requested due to automatic review settings September 15, 2026 14:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants