Add regression tests for thenable RPC proxies - #330966
Merged
Merged
Conversation
#330923 stopped the observable debugger RPC proxies and Copilot's worker RPC proxy from answering `then`. Without that guard each proxy is a thenable, so `await`ing one -- or returning it from an `async` function -- hands the promise's own resolve/reject callbacks to a remote `then` call that never invokes them and the await never settles. Cover both boundaries so the guards cannot be removed silently. Each test asserts the proxy exposes no `then` and that awaiting it resolves to the proxy itself. Verified both fail when the corresponding guard is reverted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds regression coverage ensuring RPC proxies cannot be treated as thenables and cause unresolved awaits.
Changes:
- Tests debugger request and notification proxies.
- Tests Copilot worker proxy and verifies no remote call occurs.
Show a summary per file
| File | Description |
|---|---|
src/vs/base/test/common/observableInternal/logging/debugger/rpc.test.ts |
Covers debugger RPC proxies. |
extensions/copilot/src/util/node/test/worker.spec.ts |
Covers the worker RPC proxy. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
Tyler James Leonhardt (TylerLeonhardt)
approved these changes
Aug 14, 2026
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #330923, which stopped the observable debugger RPC proxies and Copilot's worker RPC proxy from answering
then. This adds the regression coverage that was intentionally left out of that PR.Why
Those proxies synthesize a callable for any property name, so without the guard
proxy.thenis a function and the proxy is a thenable.awaiting one — or returning it from anasyncfunction — hands the promise machinery's ownresolve/rejectcallbacks to a remotethencall that never invokes them, and the await never settles. That is the deadlock originally hit in #330571.The guards are four unremarkable-looking lines. Without tests, a future reader can delete them as dead code and silently reintroduce a hang that only shows up as a spinner in the UI.
Tests
src/vs/base/test/common/observableInternal/logging/debugger/rpc.test.tsSimpleTypedRpcConnectionrequests+notificationsproxiesextensions/copilot/src/util/node/test/worker.spec.tscreateRpcProxy, exposed publicly asWorkerWithRpcProxy.proxyEach asserts the proxy exposes no
thenand that awaiting it resolves to the proxy itself. The Copilot test additionally asserts no remote call was dispatched, since the failure mode is a spuriousthencall over the wire.Validation
Confirmed these are genuine regression tests, not tautologies — each was run with its corresponding guard temporarily reverted and fails, then passes once restored:
AssertionError: + [AsyncFunction (anonymous)] - undefinedthenassertionmainThe core test needs
observableInternalinternals, so it carries aneslint-disable-next-line local/code-no-deep-import-of-internalmatching the existing convention insrc/vs/base/test/common/observables/observable.test.ts.