Skip to content

fix(http): settle failing custom route streams - #360

Merged
punkpeye merged 2 commits into
punkpeye:mainfrom
pacocartones:fix/custom-route-stream-error-settlement
Sep 7, 2026
Merged

fix(http): settle failing custom route streams#360
punkpeye merged 2 commits into
punkpeye:mainfrom
pacocartones:fix/custom-route-stream-error-settlement

Conversation

@pacocartones

@pacocartones pacocartones commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • treat a custom route as handled once its response has started
  • log and end the response if streaming fails after headers are sent
  • avoid falling through to MCP transport routing and attempting a second response
  • add a regression for a stream that errors after its first chunk

A Hono response body can reject asynchronously after the first chunk. At that point respondWithCustomRoute() cannot send an error response, but returning false tells the outer handler that no custom route matched. The request then falls through and can hang or attempt to write a second response.

Validation

  • regression test times out on the parent commit and passes with this change
  • 23 route tests pass
  • full lint, type checking and JSR publish dry-run pass

@punkpeye punkpeye left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Stops the fall-through correctly. One change needed: settle with destroy(), not end().

Verified with res.end():

  • chunked body: client gets the truncated body with a clean terminator, so it looks complete
  • Content-Length set: client keeps waiting for the missing bytes, so the hang moves client-side

Replace the if (!res.writableEnded) { res.end(); } block with:

// Drop the connection so the client sees the failure. end() writes nothing
// after destroy() but sets writableEnded, which mcp-proxy checks before
// running its own fallback response.
res.destroy();
res.end();

Test:

await expect(
  fetch(`http://localhost:${port}/failing-stream`).then((r) => r.text()),
).rejects.toThrow();

Checked locally: both cases reject in ms, 24/24 route tests pass, no unhandled rejections.

@punkpeye

punkpeye commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Pushed the destroy() change myself to land this.

  • res.destroy(); res.end() — client now sees a socket error instead of a truncated body that looks complete
  • test asserts rejection, and covers the Content-Length case too (that one hung the client)

Thanks for the fall-through fix — that part was right.

@punkpeye punkpeye left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

destroy() fix pushed and green.

@punkpeye
punkpeye merged commit 85fa2a7 into punkpeye:main Sep 7, 2026
2 checks passed
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 4.20.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants