Support VS Code browser debug session termination - #18626
Adam Ratzman (adamint) wants to merge 50 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18626Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18626" |
There was a problem hiding this comment.
Pull request overview
This PR implements a focused slice of browser debugging support for WithBrowserDebugger() in the Aspire VS Code extension. When DCP asks the extension to launch a browser resource, it now configures a js-debug (pwa-chrome/pwa-msedge) launch with a stable isolated user-data directory and clean-launch runtime flags, and reports sessionTerminated back to DCP when the browser debug session ends. This prevents closed browser debug resources from appearing as orphaned run sessions. WASM managed debugging via the C# debug bridge is explicitly left as follow-up work.
Changes:
- Rewrote the browser launch configuration builder to normalize the browser name to a js-debug adapter type, merge/deduplicate clean-launch runtime args, use a per-run user-data directory, and require a URL.
- Added
sendSessionTerminatedplus a one-shot terminate listener inAspireDebugSessionthat emits the DCPsessionTerminatednotification (with de-duplication) when the browser session ends, driven by two new optional config fields. - Added unit tests covering adapter selection, runtime-arg merging, user-data-dir, and the terminate-to-notification flow.
Show a summary per file
| File | Description |
|---|---|
| extension/src/debugger/languages/browser.ts | Builds the js-debug browser launch config: adapter mapping, runtime-arg merge, isolated user-data dir, and session-terminated flags. |
| extension/src/debugger/AspireDebugSession.ts | Adds sendSessionTerminated (de-duplicated) and wires a terminate listener to emit the DCP notification on browser session end. |
| extension/src/dcp/types.ts | Adds optional sessionTerminatedDcpId and sendSessionTerminatedOnDebugSessionEnd fields to the extended debug configuration. |
| extension/src/test/browserDebugger.test.ts | New tests for adapter selection, runtime-arg preservation, user-data-dir, and the terminate→sessionTerminated flow. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Wait for VS Code to settle browser debug shutdown before resolving a disposed-after-start launch as failed, so run cleanup cannot delete the browser profile while js-debug is still stopping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c235246b-d021-4d8e-b041-cc4984674ebe
There was a problem hiding this comment.
Review details
Suppressed comments (4)
extension/src/test/browserDebugger.test.ts:17
- These tests stub every
vscode.debuglifecycle API, so they cannot catch regressions in real Extension Host adapter selection or session event ordering. The PR description claims abrowserDebugger.e2e.test.jsshard and workflow rows, but neither the E2E source nor a browser-debugger row exists in the current tree. Add the claimed extension E2E coverage and workflow entries so this debugger flow runs in CI.
test('configures Chromium for an isolated browser that exits with the debug session', async () => {
extension/src/debugger/languages/browser.ts:31
BrowserLaunchConfiguration.urlis optional andisBrowserLaunchConfiguration()only checkstype, so this path can still emit a js-debug launch withurl: undefined. Validate a non-empty URL before selecting the adapter; otherwise malformed DCP input reaches VS Code instead of failing as an invalid launch configuration.
debugConfiguration.type = getBrowserDebugAdapter(launchConfig.browser);
extension/src/debugger/languages/browser.ts:59
- This does not implement the profile ownership model stated in the PR description and security checklist. The branch delegates profile creation and cleanup to js-debug via
userDataDir = true; there is no Aspiremkdtemp,realpathcontainment check, or registered recursive cleanup. Either restore the described security-sensitive implementation and its tests, or update the description/checklist and validation claims to match the actual threat and cleanup model.
// Let js-debug create and clean up the isolated profile. A workspace-provided profile
// path or command-line override would make Aspire stop a session without necessarily
// owning the browser instance that was launched.
debugConfiguration.userDataDir = true;
debugConfiguration.runtimeArgs = mergeRuntimeArgs(debugConfiguration.runtimeArgs);
extension/src/debugger/AspireDebugSession.ts:659
- For process-backed sessions, this is now the only successful-stop path that calls
cleanupRun, but no listener performs the same bookkeeping when the debug session ends naturally.adapterTracker.ts:102-124sends DCP termination on adapter exit without cleanup, so handlers such as the Azure Functions host cleanup registered atlanguages/azureFunctions.ts:241remain leaked after a normal exit. Observe root-session termination for non-browser resources too (without duplicating the adapter's DCP notification) and run cleanup there.
stopSessionPromise = Promise.resolve(vscode.debug.stopDebugging(session)).then(
() => {
// Run cleanup only after VS Code confirms the session stopped. A failed stop keeps
// both the DCP run and its resource-specific cleanup available for a retry.
cleanupRun(debugConfig.runId);
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c235246b-d021-4d8e-b041-cc4984674ebe
There was a problem hiding this comment.
Review details
Suppressed comments (3)
extension/src/debugger/languages/browser.ts:75
- There is no browser debugger test under
extension/src/test-e2e; all new adapter-selection and missing-Firefox tests stub VS Code APIs. Restore extension-host coverage that calls the browser resource configuration path and verifies Edge/Chrome/Firefox adapter selection and the missing-adapter error. This can remain configuration-only and does not need to launch a real browser.
if (debugType === firefoxDebugAdapterType && !isFirefoxDebuggerInstalled()) {
promptToInstallFirefoxDebugger();
throw new Error(firefoxDebuggerNotInstalled);
extension/src/debugger/browserDebugSessionTermination.ts:44
- A stop that reaches the outer 10-second deadline is not actually retryable.
resetStopSessionAttemptclears only the wrapper inAspireDebugSession; this class keeps_stopPromisepointing at the original never-settlingstopCore(), so the next attempt reuses it and times out without issuing anothervscode.debug.stopDebuggingcall. Expose a way to clear the browser stop attempt and invoke it from each timeout path (including DCP teardown) before allowing a retry.
this._stopPromise ??= this.stopCore();
return this._stopPromise;
extension/src/debugger/languages/browser.ts:60
- The supported-browser path still accepts a missing or whitespace-only
url: the type guard only checkstype, so Firefox (and the existing Chromium entries) reachesstartDebuggingwithurl: undefinedand fails inside the adapter. Keep the unsupported-browser diagnostic as-is, then explicitly reject a supported browser whose URL is absent with a missing-URL launch-configuration error before assigningdebugConfiguration.url.
extensionLogOutputChannel.warn(`No supported debug adapter is registered for browser '${browser}'.`);
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
| this._terminationListener.dispose(); | ||
|
|
||
| if (this._dcpId) { | ||
| this._sendSessionTerminated(this._runId, this._dcpId); |
There was a problem hiding this comment.
finish() marks the lifecycle complete, then sends the DCP notification before cleanupRun. If that synchronous callback throws, the browser run cleanup is skipped even though the listener is already disposed. This helper owns terminal cleanup, so I think cleanup should run in a finally; I'll add a throwing-callback test.
| // can run concurrently without conflicting | ||
| debugConfiguration.userDataDir = true; | ||
|
|
||
| if (debugConfiguration.type === firefoxDebugAdapterType) { |
There was a problem hiding this comment.
The Firefox adapter defaults every launch without port to 6000. Two Aspire Firefox browser resources can therefore collide or connect the second adapter to the first process, and the adapter doesn't support port: 0 or another race-free reservation mechanism. Firefox support was added during this PR, but it isn't safe for Aspire's multi-resource model yet. I'm going to remove Firefox from this PR and leave Chromium termination as the supported path rather than ship a hidden single-session constraint.
| return merged; | ||
| } | ||
|
|
||
| function isUserDataDirArg(arg: string): boolean { |
There was a problem hiding this comment.
There's still a Windows hole in this fix: isUserDataDirArg compares the switch name case-sensitively, while Chromium normalizes switch names on Windows. --User-Data-Dir=... therefore survives and can replace js-debug's isolated profile. I'll make this comparison case-insensitive and cover both combined and split forms.
| this._finished = true; | ||
| this._terminationListener.dispose(); | ||
|
|
||
| if (this._dcpId) { |
There was a problem hiding this comment.
The warning we said we'd keep here was lost in the later simplification. A null DCP ID now silently skips the terminal notification. Normal DCP starts should always have one, but this should still log the run-specific warning so the invalid state is diagnosable. I'll restore that with coverage.
| return Promise.resolve(); | ||
| } | ||
|
|
||
| this._stopPromise ??= this.stopCore(); |
There was a problem hiding this comment.
Timeouts still break the retry guarantee here. The outer stop promise is cleared when the budget expires, but BrowserDebugSessionTermination._stopPromise and the DCP teardownPromise keep the original pending operation. Every later DELETE then reuses the same hung stop. I'll make resets identity/generation-based across the three layers and cover a permanently hung first attempt followed by a successful retry.
Two conflicts, both additive collisions where the branch and main added different code at the same place: - capabilities.ts: the branch's Firefox debugger detection and main's Java detection landed adjacently. Union of both. - aspireDebugSession.test.ts: import lists. Main's set plus the branch's cleanupRun. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 522a7395-2ea1-4d1a-91b3-40050853b9f4
…r-wasm-debug-session Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> # Conflicts: # extension/loc/xlf/aspire-vscode.xlf
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
extension/src/dcp/AspireDcpServer.ts — The stop timeout does not cover this await. If browser startup never settles (for example,… |
 |
extension/src/test-e2e/browserDebugger.e2e.test.ts — This test confirms that Firefox is still rejected, but the PR description explicitly says Firefox… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
 |
extension/src/debugger/browserDebugSessionTermination.ts — A stop that exceeds Aspire's shutdown budget leaves this promise pending forever.… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
extension/src/debugger/languages/browser.ts — This user-visible debugger path has only stubbed unit coverage; there is currently no browser… View resolved comment |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Two Copilot findings still need cleanup on the current head: the browser stop budget needs to cover a startup that never settles, and the PR description still advertises Firefox after the implementation intentionally narrowed to Chromium. I'm fixing both before calling this ready. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7c01c030-226e-41c5-81ca-a6483a1856df
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
extension/src/debugger/AspireDebugSession.ts — This browser lifecycle registration is skipped by the isShuttingDown early return above. That… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
extension/src/debugger/browserDebugSessionTermination.ts — A stop that exceeds Aspire's shutdown budget leaves this promise pending forever.… View comment |
|
extension/src/test-e2e/browserDebugger.e2e.test.ts — This test confirms that Firefox is still rejected, but the PR description explicitly says Firefox… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
extension/src/dcp/AspireDcpServer.ts — The stop timeout does not cover this await. If browser startup never settles (for example,… View resolved comment |
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
extension/src/debugger/browserDebugSessionTermination.ts:71
- This method is invoked through
_disposablesonly after ordered shutdown succeeded or during irreversiblefinalizeForExtensionShutdown(), yet a genuine stop rejection leaves the global termination listener registered indefinitely. That contradicts finalization's listener-release contract and retains this session object after its owner is gone. Dispose_terminationListenerin the rejection handler; a root termination that races the rejection already makesstopCore()return successfully via_finished.
stopAndDisposeOnFailure(): void {
// A failed explicit stop can still be followed by a natural root-session termination.
// Keep observing that event while handling the rejection so disposal cannot create an
// unhandled promise or suppress the eventual DCP notification and cleanup.
void this.stop().catch(() => { });
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
One current lifecycle gap remains: a browser session created during shutdown is stopped on the late-session path, but DCP loses the session handle and can report termination before that stop is confirmed. I’m keeping the late browser session reachable until its stop result is known, while preserving the existing shutdown ordering and retry semantics. |
…llowup Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7c01c030-226e-41c5-81ca-a6483a1856df
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7c01c030-226e-41c5-81ca-a6483a1856df
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The lifecycle changes are coherent and comprehensively covered by focused race, retry, shutdown, and configuration tests.
Review tier: Balanced
Findings: 1
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
extension/src/test-e2e/browserDebugger.e2e.test.ts — This test confirms that Firefox is still rejected, but the PR description explicitly says Firefox… View comment |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
extension/src/debugger/AspireDebugSession.ts — This browser lifecycle registration is skipped by the isShuttingDown early return above. That… View resolved comment |
|
extension/src/debugger/browserDebugSessionTermination.ts — A stop that exceeds Aspire's shutdown budget leaves this promise pending forever.… View resolved comment |
Description
Adds the VS Code extension lifecycle needed for
WithBrowserDebugger()browser resources.The implementation is intentionally Chromium-only. The Firefox adapter defaults sessions without an explicit port to port 6000 and does not support a race-free dynamic port reservation, so it cannot safely support multiple Aspire browser resources. Managed WASM debugging through the C# debug bridge remains separate work under #17797.
Relationship to #19125
#19125 owns the general adapter-backed DCP stop/retention lifecycle. This PR preserves the browser-specific confirmed-stop rule: DCP must not report a browser run as terminated until VS Code confirms the root browser session ended. If #19125 lands first, this branch should retain that distinction while rebasing its overlapping DCP code.
Validation
Part of #17797
Checklist