FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[api] Answer nested requests on the sync connection in stack order by cplieger · Pull Request #64639 · microsoft/TypeScript · GitHub

[api] Answer nested requests on the sync connection in stack order - #64639

Open
Christopher Plieger (cplieger) wants to merge 1 commit into
microsoft:mainfrom
cplieger:fix-api-sync-conn-stack-order
Open

Christopher Plieger (cplieger) wants to merge 1 commit into
microsoft:mainfrom
cplieger:fix-api-sync-conn-stack-order

Conversation

Christopher Plieger (cplieger) commented Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

While SyncConn.Call handles a nested request from a client callback, it releases its lock, so another goroutine's call can start inside the client's pending nested request. Responses are matched by method name, so the nested answer and both callback answers then reach the wrong requests.

Impact: a resolver callback that delegates to another resolver, the pattern the API's own test shows, binds most imports to the wrong module once the loader uses more than one thread. Every type and diagnostic the tool reads from that program is then wrong, and no error says so. In the reproduction, 873 of 900 nested answers went to another request.

The connection now keeps a stack of the exchanges in flight. A new call waits while another call is innermost, and a request is answered only once it is innermost again, which is the order a synchronous client handles them in.

A call made from a notification handler while another call is waiting now blocks. No HandleNotification in the tree makes a call today.

TestSyncConnNestedRequestsAnswerInStackOrder drives two overlapping callbacks against a fake synchronous client under testing/synctest. It fails on each of 20 runs without the change. With a tsc built from this branch, the reproduction in the issue gives 900 right answers of 900. TestSyncConnRunReturnsResponseWritePanic keeps a panic inside WriteResponse returning an error from Run, as it does on main.

I met this while building deadset-ts, the TypeScript analyzer of deadset, on the TypeScript 7 API, as part of a small set of defects that work hit, which is why there are a few related reports from me.

An AI coding agent wrote this patch. I have read, built and tested it and will handle the review.

  • There is an associated issue in the Backlog milestone (required)
  • Code is up-to-date with the main branch
  • You've successfully run npx hereby test
  • You've successfully run npx hereby lint
  • You've successfully run npx hereby check:format
  • There are new or updated tests validating the change

Fixes #64631

Copilot AI balanced review requested due to automatic review settings October 5, 2026 10:28
typescript-automation Bot added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 5, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Copilot review overview

🔵 Needs a closer look

The bidirectional synchronization and panic-recovery behavior warrant final human review despite no concrete blocking findings.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes #64631 by coordinating synchronous IPC exchanges in stack order so overlapping callbacks receive the correct responses.

Changes:

  • Tracks in-flight exchanges and waits before starting calls or answering requests.
  • Adds regression tests for nested response ordering and response-write panic handling.
File Description
tsc/​internal/​ipc/​conn_sync.go Adds stack-based coordination for calls and responses.
tsc/​internal/​ipc/​conn_sync_test.go Tests overlapping nested requests and panic propagation.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

This branch has not been deployed

No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Status: Not started

Development

Successfully merging this pull request may close these issues.

[api] A nested request from a resolveModuleName callback gets another request's answer

2 participants


Back | FazBrowse Home | New Git URL