Skip to content

Report asynchronous JSON-RPC body write failures with message context - #1946

Open
Shobhit Gupta (shobhit26-09) wants to merge 2 commits into
microsoft:mainfrom
shobhit26-09:fix/payload-write-error-context
Open

Shobhit Gupta (shobhit26-09) wants to merge 2 commits into
microsoft:mainfrom
shobhit26-09:fix/payload-write-error-context

Conversation

@shobhit26-09

Copy link
Copy Markdown

Summary

Await the message-body write inside WriteableStreamMessageWriter.doWrite so its asynchronous rejection reaches the existing catch block and handleError(error, msg).

The header write is awaited, but the body write currently returns its promise directly. If the header succeeds and the body rejects, the caller still receives the rejection, but the writer skips the message-associated error event and error-count increment. A transport-level error event may still fire without that context.

This makes body failure reporting consistent with header failure reporting without changing framing, write ordering, or the rejected error object.

Tests

Added two deterministic tests using real Node Writable streams: fail the header callback, or let the header succeed and fail the body callback. Both assert rejection with the original error and an onError event carrying the original message and count 1. The body test fails before the change; the header test is a passing control. Streams and writers are cleaned up in finally.

Validation on Linux / Node 22.23.4:

  • npm run compile:jsonrpc: passes.
  • npm --prefix jsonrpc run test:node: all 81 tests pass.
  • npm --prefix jsonrpc run lint: passes (existing ESLint option-deprecation notice only).
  • JSON-RPC browser test bundle builds; all 13 existing browser tests pass in Chromium 149.0.7827.55. Used the repository's browser test page with a local Playwright driver, a separate port, and the installed Chromium executable.
  • git diff --check: passes.
  • Both files on this branch match the tested local files byte for byte.

Related investigation: #1919 discusses missing context on transport errors and restart decisions. This PR does not fix or close that broader issue: transport-level count-less error forwarding and language-client restart policy are unchanged. It also does not change the separate request-rejection cleanup fixed by #1855.

AI assistance was used for investigation, implementation, and testing. Full monorepo/editor integration tests, Firefox, Safari, macOS and Windows were not run. Hosted CI has not yet run.

Copilot AI balanced review requested due to automatic review settings November 15, 2026 09:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues block the JSON-RPC body-write error-handling change.

No open issues 🎉

What changed in this PR

Ensures asynchronous JSON-RPC body-write failures reach the writer’s existing error handler, preserving message context and incrementing the error count. Framing, write ordering, and the original rejection error remain unchanged; broader transport-error and restart behavior is outside this PR’s scope.

Changes:

  • Await body writes so rejected promises reach handleError(error, msg).
  • Add deterministic header- and body-failure tests checking error identity, message context, and count.
File Description
jsonrpc/​src/​node/​test/​messages.test.ts Adds header- and body-write failure regression tests with stream cleanup.
jsonrpc/​src/​common/​messageWriter.ts Awaits the body write inside the existing error-handling block.

Review effort: Balanced

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