Skip to content

fix(mcp): answer a tool call whose response never arrives - #690

Merged
ducnmm merged 1 commit into
devfrom
wyner/walm-328-issue-sse-bridge-idle-close-orphans-in-flight-remember-calls
Aug 19, 2026
Merged

fix(mcp): answer a tool call whose response never arrives#690
ducnmm merged 1 commit into
devfrom
wyner/walm-328-issue-sse-bridge-idle-close-orphans-in-flight-remember-calls

Conversation

@HoangDucBach

@HoangDucBach HoangDucBach commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Ticket

WALM-328 — the MCP bridge leaves a tool call hanging when its response is lost.

What changed?

All in packages/mcp/src/bridge.ts.

  • A deadline per in-flight request. Each entry is stamped with startedAt and a sweeper closes out the expired ones. The stamp is never refreshed, not even by a replay, or a reconnect loop would keep pushing the deadline out and reintroduce the same hang.
  • A truthful error. Expired calls go through the existing failRequest, which already adds the id to closedOutIds so a late genuine reply is dropped instead of becoming a second response for that id. failRequest gains optional wording: its "relayer unavailable" text would be wrong here, since the relayer is fine and only one reply went missing.
  • MEMWAL_MCP_CALL_TIMEOUT_MS, derived in code from the slowest server-side tool deadline (analyze) plus headroom, so it moves with that tool instead of being pinned. The sweep period is capped at 5s so a long deadline is honoured closely.
  • Richer reconnect logging. In-flight ids and methods, not just the count.

Why is this needed?

The bridge had no deadline of its own. Its only timer watches for silence on the SSE stream, and the relayer's 3s keepalive keeps that stream looking healthy — so a reply lost while the connection is still up trips nothing: no watchdog, therefore no reconnect, therefore no replay. The request sat in inFlight forever and the caller could only report a bare timeout.

Two of the three options in the issue were already implemented: the sidecar keepalive (services/server/scripts/mcp/index.ts:164-184) and in-flight replay (bridge.ts:832-857). The third, an explicit orphaned-call error, is what this adds.

Scope

The bridge's own timers. No transport, protocol or relayer change.

Out of scope

  • notifications/progress, which the MCP SDK honours via resetTimeoutOnProgress and would stop the client giving up at its own 60s default. The protocol's proper answer for long calls; worth its own ticket.
  • Replay can double-write: inFlight.set runs before the send branch, so a call the relayer already executed is re-POSTed verbatim, and no idempotency key crosses the bridge.

How was this tested?

  • Unit tests
  • Integration tests
  • End-to-end tests
  • Manual testing
  • Not applicable

New test/orphaned-call.test.mjs reproduces the exact gap: the mock relayer keeps heartbeating throughout, answers initialize, then swallows the memwal_remember reply. It asserts the call is closed out with a retryable error, that getSseGetCount() === 1 so the watchdog demonstrably did not rescue it, and that a late genuine reply is dropped rather than delivered twice.

Verified the test earns its place: with the sweeper disabled it fails with timed out waiting for message, the reported symptom. Suite is 19/19, tsc clean.

One caveat worth stating: a single unattributed failure appeared in one run and did not reproduce across 18 further runs on this branch or 8 on origin/dev. Most likely machine load, since these tests spawn child processes on real timers, but it is unexplained.

How can the reviewer verify it?

cd packages/mcp && npm test

# watch it catch the bug: disable the sweeper, then re-run
# (src/bridge.ts, in the orphanSweeper loop)
#   if (elapsedMs <= callTimeoutMs) continue;   ->   if (true) continue;
node --test test/orphaned-call.test.mjs

Risks and dependencies

  • SLOWEST_SERVER_TOOL_MS duplicates a value from another package. analyze's 180s lives in services/server/scripts/mcp/tools/, which this package cannot import from. If that tool is granted longer and this is not raised with it, the bridge will start declaring healthy requests orphaned. The constant carries that warning.
  • A call is answered at most one sweep (5s) after its deadline, so the effective timeout is 240–245s rather than exactly 240s.
  • The default only errs toward waiting longer. A short override via the env var is the way to exercise the path; values below 1s are ignored.

Author checklist

  • This pull request maps to one ticket and one logical outcome.
  • I reviewed the complete diff myself.
  • I removed unrelated, debug, and temporary changes.
  • I ran the relevant tests.
  • CI is green.
  • The branch is up to date with its target branch.
  • I added or updated tests where appropriate.
  • I documented any important risk, dependency, rollout, or follow-up.
  • I provided clear verification steps.
  • The pull request is ready for review and is no longer a Draft.

@jessiemongeon1

jessiemongeon1 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Style Guide Audit

All 1 file(s) pass the style guide audit.

The bridge had no deadline of its own for a request. Its only timer watches
for silence on the SSE stream, and the relayer's 3s keepalive keeps that
stream looking healthy, so a reply lost while the connection is still up
trips nothing: no watchdog, therefore no reconnect, therefore no replay. The
request sat in `inFlight` forever and the caller could only report a bare
timeout with nothing to act on.

Stamp each in-flight entry with `startedAt` and sweep for expired ones. The
stamp is never refreshed, not even by a replay, since a reconnect loop would
otherwise keep pushing the deadline out and reintroduce the same indefinite
wait. The sweep period is capped at 5s so the deadline is honoured closely
rather than drifting by a quarter of itself.

Expired calls close out through the existing `failRequest` path, which
already adds the id to `closedOutIds` so a late genuine reply is dropped
rather than becoming a second response for that id. `failRequest` gains
optional wording because its "relayer unavailable" text would be wrong here:
the relayer is fine, one reply just went missing, and saying otherwise sends
whoever debugs it next in the wrong direction.

The default of 240s clears the slowest server-side tool deadline (`analyze`
at 180s) so a healthy request is never called orphaned while the relayer is
still working on it. `MEMWAL_MCP_CALL_TIMEOUT_MS` overrides it.

Also log the in-flight ids and methods at reconnect rather than just the
count. The reported `replayCount: 0` could not be reproduced from the code —
all three `inFlight.delete` sites require an actual response frame, and
`inFlight.clear()` only runs on an account switch — so the next occurrence
needs to show whether the map was empty or the entry was removed early.

Refs WALM-328
@HoangDucBach
HoangDucBach force-pushed the wyner/walm-328-issue-sse-bridge-idle-close-orphans-in-flight-remember-calls branch from b82f5ae to 4d6c87d Compare August 18, 2026 23:55
@HoangDucBach
HoangDucBach requested a review from ducnmm August 18, 2026 23:59
@HoangDucBach

Copy link
Copy Markdown
Collaborator Author

Style Guide Audit

Audited 1 file(s) against the Sui Documentation Style Guide.

2 violation(s) found. All must be fixed before merge.

docs/reference/environment-variables.md (2 violation(s))

2 violation(s) (1 regex, 1 claude)

  • Line 73 — Use "might" not "may"

    • Current: may
    • Fix: might
  • Line 73 — word-preference

    • Current: a single request may wait
    • Fix: a single request might wait

Automated audit using the Sui Documentation Style Guide.

Fixed

@ducnmm ducnmm 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.

LGTM! The per-call deadline handling, sweeper with unref/clear, late response suppression in closedOutIds, and regression test in orphaned-call.test.mjs look solid.

@ducnmm
ducnmm merged commit 6b4cfd7 into dev Aug 19, 2026
12 checks passed
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.

3 participants