Skip to content

fix(ai): Pathfinder MCP client hangs then aborts (10s timeout) - #112

Merged
NathanTarbert merged 4 commits into
mainfrom
fix/pathfinder-streamable-http
Jul 21, 2026
Merged

NathanTarbert merged 4 commits into
mainfrom
fix/pathfinder-streamable-http

Conversation

@NathanTarbert

Copy link
Copy Markdown
Collaborator

Problem

The worker's Pathfinder retrieval always timed out — logs showed [Pathfinder] searchDocs failed: This operation was aborted, so every AI response fell back to no-docs retrieval and came back LOW confidence.

Root cause

PathfinderClient.connect() did GET {mcpUrl}/sse then await response.text(). /sse is a long-lived text/event-stream that never closes, so .text() blocked until the 10s AbortController fired → This operation was aborted. Retrieval was never actually used.

The existing tests mocked .text() as instantly-resolving, which masked the infinite-stream hang — the bug shipped green.

Fix

Switch to the MCP Streamable HTTP transport the same server already supports (POST {mcpUrl}/mcp):

  • initialize → capture Mcp-Session-Id response header → best-effort notifications/initializedtools/call with the session header. Finite responses, no hanging stream.
  • parseJsonRpc handles plain JSON and SSE-framed (data: ...) replies.
  • parseSearchResults now parses the current copilotkit-docs-mcp SNIPPET/TITLE/SOURCE/CONTENT text format (legacy JSON-array kept for back-compat); synthesizes a descending rank score since the text format carries no numeric score (results are already server-filtered by min_score).
  • AbortError → clear MCP request timed out message instead of the opaque abort.

Config is unchanged — PATHFINDER_MCP_URL default (https://mcp.copilotkit.ai) is correct; the bug was purely the client transport.

Verification

  • Live smoke test against mcp.copilotkit.ai: 8 results in ~770ms (previously 10s → abort → 0 results).
  • Tests rewritten for the new transport + SNIPPET/SSE parsing (connect/session/timeout/fallback/legacy-JSON).
  • 774 @copilotkit/outpost tests pass; typecheck clean.

Impact

Retrieval works again → AI responses get real docs → confidence scores reflect actual grounding instead of always LOW.

Root cause: connect() did GET {mcpUrl}/sse then await response.text().
/sse is a long-lived text/event-stream that never closes, so .text()
blocked until the 10s AbortController fired -> 'This operation was
aborted'. Every searchDocs aborted and fell back; retrieval was never
actually used (LOW-confidence answers). The old tests mocked .text() as
instantly-resolving, masking the infinite-stream hang.

Fix: switch to the MCP Streamable HTTP transport the same server
supports (POST {mcpUrl}/mcp):
- initialize -> capture Mcp-Session-Id response header -> best-effort
  notifications/initialized -> tools/call with the session header.
  Finite responses, no hanging stream.
- parseJsonRpc handles plain JSON and SSE-framed (data: ...) replies.
- parseSearchResults now parses the current copilotkit-docs-mcp
  SNIPPET/TITLE/SOURCE/CONTENT text format (JSON-array kept for
  back-compat); synthesizes a descending rank score since the text
  format carries no numeric score.
- AbortError -> clear 'MCP request timed out' message.

Config unchanged (PATHFINDER_MCP_URL default is correct).

Verified: live smoke test against mcp.copilotkit.ai returns 8 results in
~770ms (was 10s abort). Tests rewritten for the new transport +
SNIPPET/SSE parsing. 774 outpost tests pass; typecheck clean.

@jerelvelarde jerelvelarde left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice fix — the diagnosis is spot-on (reading a never-closing text/event-stream to completion → 10s abort → silent no-docs fallback → always-LOW), and moving to the Streamable HTTP transport is the right call. The handshake, the Accept: application/json, text/event-stream header, and the "why not SSE" doc comment are all correct, and the test rewrite now asserts the transport shape instead of instant-resolving .text() (which is what let the original bug ship green). 👍

Blocking (let's resolve before merge)

1. Clear the session before re-initializing. connect() enters doConnect() when the session is expired, but this.sessionId is still non-null at that point, so post() attaches a stale Mcp-Session-Id to the initialize request (pathfinder.ts L109-110). That's semantically off — initialize is what mints a session — and per the MCP spec a server MAY answer a terminated session id with 404. If that re-init POST throws, nothing resets sessionId (the resets only run after a successful POST), so the client can get stuck re-sending the dead session id and falling back forever — the same silent-permanent-fallback failure mode this PR is fixing. One-liner:

private async doConnect(): Promise<void> {
    this.reset(); // no stale Mcp-Session-Id on (re-)initialize; failure leaves clean state
    const { body, sessionId } = await this.post({ /* initialize */ });

The smoke test wouldn't have caught this — it exercises a fresh connect (sessionId null), not the ~25-min expiry-refresh path the worker will actually hit.

2. Confirm the --- split can't clip content. parseSnippets splits on /\n-{3,}\n/ and drops blocks without TITLE: (pathfinder.ts L252-254). If a snippet's CONTENT can itself contain a --- (markdown thematic break / frontmatter — common in docs), the tail gets silently dropped and the grounding text is truncated. Can you check one real mcp.copilotkit.ai payload? If CONTENT never contains ---, ignore this. If it can, splitting on the record marker is safer:

const blocks = text.split(/\n(?=SNIPPET \d+\b)/).map((b) => b.trim()).filter((b) => /TITLE:/i.test(b));

Non-blocking (filed as follow-up issues)

  • #122 — heuristic-confidence calibration from the synthetic rank scores
  • #123 — SSE multi-data: line join
  • #124 — remove the now-unused exploreDocs / queryKnowledgeBase

Approving as soon as #1 lands and #2 is answered.

…t snippet split

Blocking review items from #112:

1. doConnect() now calls this.reset() before the initialize POST, so a
   re-init after session expiry can't attach a stale Mcp-Session-Id (a
   terminated id may 404), and a throwing re-init leaves clean state instead
   of a dead session that falls back forever. Adds a regression test that
   the post-expiry re-initialize carries no session header.

2. parseSnippets() splits on the structural "SNIPPET <n>" marker instead of
   the "---" separator (doc content commonly contains a --- horizontal rule),
   and strips only the trailing separator. Covered by the internal-rule test.
@NathanTarbert

Copy link
Copy Markdown
Collaborator Author

Addressed both blocking items in 2f80731:

1. Stale session on re-initdoConnect() now calls this.reset() before the initialize POST, so a post-expiry re-init never attaches an old Mcp-Session-Id (which a terminated session may answer with 404), and a throwing re-init leaves clean state instead of a dead id that falls back forever. Added a regression test on the expiry-refresh path (the one the worker actually hits at ~25 min) asserting the second initialize POST carries no session header — it fails without the reset().

2. --- split clipping content — switched parseSnippets to split on the structural SNIPPET <n> record marker instead of the --- separator (doc CONTENT commonly contains a --- horizontal rule / frontmatter), and it now strips only the trailing separator. Covered by the "preserves snippet content that contains an internal --- horizontal rule" test.

ai package: 13 pathfinder tests pass, typecheck clean.

@NathanTarbert

Copy link
Copy Markdown
Collaborator Author

@jerelvelarde, ready for a quick review of the changes.

@jerelvelarde jerelvelarde left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Both blocking items resolved and verified: (1) doConnect() resets before re-initialize, so no stale Mcp-Session-Id on re-init and a failed re-init leaves clean state; (2) parseSnippets splits on the SNIPPET marker, preserving content with internal --- rules. CI green (lint/typecheck/test). Non-blocking follow-ups in #122/#123/#124; recommend also landing a test that bounds a hanging body-read by the timeout. Approving.

@NathanTarbert
NathanTarbert merged commit 862b85e into main Jul 21, 2026
2 checks passed
@NathanTarbert
NathanTarbert deleted the fix/pathfinder-streamable-http branch July 21, 2026 18:10
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