Repository navigation
Fix authentication provider cleanup when an extension host stops - #345127
Connor Peet (connor4312) merged 10 commits into
Conversation
Unregister authentication providers when their main-thread customer is disposed, without removing replacements registered by another extension host. Keep OAuth storage intact. Related to microsoft#343316.
Cover host replacement, cross-host isolation, late unregister calls, and registration/session event behavior. Related to microsoft#343316.
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: TylerLeonhardtMatched files:
|
There was a problem hiding this comment.
🟡 Changes recommended
A stale unregister notification can delete a newly registered authentication provider, leaving session requests to fail; the ordering also needs a regression test.
1 open issue
What changed in this PR
This PR addresses stale authentication providers after extension-host replacement, related to #343316, by tying provider cleanup to the owning host’s lifetime.
Changes:
- Unregisters host-owned providers on disposal while preserving same-ID replacements.
- Suppresses unregister notifications to the stopping host without deleting persisted OAuth data.
- Adds lifecycle tests for replacement, cross-host isolation, late unregister calls, and event behavior.
| File | Description |
|---|---|
src/vs/workbench/api/test/browser/mainThreadAuthentication.test.ts |
Adds provider lifecycle and host-replacement regression tests. |
src/vs/workbench/api/browser/mainThreadAuthentication.ts |
Adds ownership-aware provider cleanup during disposal and explicit unregister. |
Review effort: Balanced
Discard superseded registrations and notify only the host that owns the removed provider. Related to microsoft#343316.
Cover both registration orderings with the real extension-host queue and preserve normal external unregistration. Client typecheck and the official Electron suites pass (11 + 24 tests): https://github.com/wozhenbang2004/vscode/actions/runs/41013263036
Connor Peet (connor4312)
left a comment
Connor Peet (connor4312)
left a comment
There was a problem hiding this comment.
The entire MainThreadAuthentication will be disposed when the corresponding extension host stops. I think you're barking up the wrong tree here, unless you're able to actually repro something...
Updated createHost and registerDynamicProvider functions to include RPCProtocol and improve dynamic provider registration handling. Added tests for rediscovering dynamic providers after RPC and customer disposal.
Register a typed server actor rather than a DTO-shaped client proxy. The disposal test still uses production RPCProtocol cancellation.
|
Connor Peet (@connor4312) You're right that the whole The remaining registration is in I've added At This is a component-level reproduction with real RPC disposal, not a real extension-host process restart or the original Datadog/Add Folder flow. I haven't reproduced that end-to-end scenario, so I can't yet claim this proves the cause of #343316. The PR description now makes that boundary explicit. |
|
Your PR seems to add additional guards against "Another extension host may have replaced the provider registered by this host" which I think are not related to the issue; just the lack of deregistration |
Removed unused imports and simplified function signatures in mainThreadAuthentication tests.
Cover explicit unregistration, external removal followed by registration on another host, and notifications to surviving hosts.
Unregister providers from their registration disposer, preserve owner notification suppression, and release registrations after external unregistration. Remove the separate disposal override and provider identity check.
Related to #343316.
Authentication providers can remain in
AuthenticationServiceafter their owningMainThreadAuthenticationcustomer is disposed, leaving discovery to reuse a provider backed by the old RPC proxy. The customer already is disposed when its extension host stops; the missing cleanup is the service's independent provider registration.Provider deregistration now belongs to the registration's disposer, so both explicit unregistration and customer disposal use the same cleanup. External unregistration also releases the local registration, and cleanup suppresses only the owning host's unregister echo.
The focused regression tests check that disposal deregisters the provider without additional client-registration writes. The
RPCProtocoltest models the manager's teardown order, verifies cancellation through the old proxy, and checks that a fresh host can register with the same authentication service. Three additional tests cover explicit unregistration, external unregistration followed by registration on another host and teardown of the old host, and notifications to surviving hosts.Earlier validation for
ecaef8a5b708092acb0dff35fc8ea3323aa81d5fpassed in this isolated fork run: changed-file hygiene and ESLint, full client typecheck/transpilation, and the official Electron suites (12MainThreadAuthenticationtests and 24AuthenticationServicetests, plus one clean-state harness check per invocation; JUnit totals 13 and 25, no failures/errors/skips).The same RPC regression was then run with only the production
mainThreadAuthentication.tsreplaced by its unpatched version fromcd2259bdf72ec7421e70ae80999ceb46f8bd86fe. It failed at provider discovery: actualdynamic-provider, expectedundefined, after the real cancellation assertion had passed. Logs and JUnit artifact. The validation workflow stays on a separate fork branch, not in this PR.This establishes the component-level lifecycle defect, not the original live Datadog OAuth/Add Folder to Workspace scenario. I have not reproduced that end-to-end flow or confirmed this resolves all of #343316.
The official Electron suites and client typecheck have not been rerun for revision
e5886065e6ebcff0e1a712c49407c23233b71b61.