Fix browser debug targets, empty webRoot, and resource stop ordering - #19145
Fix browser debug targets, empty webRoot, and resource stop ordering#19145Adam Ratzman (adamint) wants to merge 2 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19145Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19145" |
There was a problem hiding this comment.
Pull request overview
Fixes VS Code debugging configuration and shutdown behavior.
Changes:
- Validates browser adapters and omits empty web roots.
- Stops resource sessions before AppHost and parent sessions.
- Localizes Azure Functions debugger labels and adds tests.
Show a summary per file
| File | Description |
|---|---|
extension/src/test/browserDebugger.test.ts |
Tests browser configuration and validation. |
extension/src/test/azureFunctionsDebugger.test.ts |
Tests Azure Functions debugger metadata. |
extension/src/test/aspireDebugSession.test.ts |
Tests debugger shutdown ordering. |
extension/src/loc/strings.ts |
Adds localized debugger strings. |
extension/src/debugger/languages/browser.ts |
Validates browsers and handles web roots. |
extension/src/debugger/languages/azureFunctions.ts |
Localizes session names. |
extension/src/debugger/AspireDebugSession.ts |
Reorders session shutdown. |
extension/package.nls.json |
Registers localization strings. |
extension/loc/xlf/aspire-vscode.xlf |
Updates generated localization data. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- 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. |
|
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. |
Four fixes to the VS Code resource debugger, extracted from microsoft#19133 so they are not gated on an E2E harness. `WithBrowserDebugger(browser)` takes an arbitrary string and `browser.ts` forwarded it as `pwa-<value>`. js-debug only contributes `pwa-chrome` and `pwa-msedge`, so anything else failed inside VS Code with "Configured debug type is not supported" after the session had already started - no resource name, no indication of which browsers do work. Unknown values now fail up front with the supported list. The allowlist is a `Map` rather than an object literal because the key is caller-supplied: a literal inherits `Object.prototype`, so a resource named `toString` or `__proto__` would resolve to an inherited member and assign a function to `debugConfiguration.type`. The hosting side sends `web_root: ""` when a browser resource has no web root, and js-debug treats an empty `webRoot` as a real path when it resolves source maps. Only forward a value the AppHost actually configured. `stopDebugging()` stopped the AppHost first and left resource debug sessions to `dispose()`. A resource running under a debugger can hold the AppHost shutdown open until its own session exits, so the AppHost stop waited on a process whose debugger had not been told to stop yet. Resource sessions now stop first, and a failure there is rethrown only after the AppHost and the synthetic Aspire parent have been stopped, so one bad adapter cannot strand the rest. The Azure Functions debug-session names were plain template strings, so they were never extracted for localization despite being user-visible session names. They use `vscode.l10n.t` now, like the adjacent browser labels. Adds `browserDebugger.test.ts`, extends the `stopDebugging` ordering test, and adds a metadata suite to `azureFunctionsDebugger.test.ts` that pins the localized names. 1473 unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`Promise.all` settles on the first rejection, so a resource adapter that failed to stop released `stopDebugging()` while the remaining resource stops were still in flight and the AppHost stop began underneath them. That is the ordering this change exists to enforce, broken on precisely the path where a resource is most likely to be orphaned. `Promise.allSettled` now waits for every adapter; a single failure is rethrown unchanged after the AppHost and the synthetic Aspire parent have been stopped, and multiple failures are rethrown as an `AggregateError` so no reason is discarded the way `Promise.all` discarded them. `if (launchConfig.web_root)` let a whitespace-only value through because `" "` is truthy, so js-debug still received an invalid source-map root - the same outcome the empty-string guard was added to prevent. The trimmed value is now both the thing tested and the thing forwarded, so what was validated is what js-debug gets. `stopDebugging waits for every resource stop to settle before stopping the AppHost` fails against the old `Promise.all` with the AppHost and parent stops recorded ahead of `slow-resource-session-settled`. The blank web root cases (`""`, `" "`, `"\t"`, `"\n"`, `" \t\r\n "`) plus the trimmed-forwarding case give 5 failures against the untrimmed guard. 1480 unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (2)
extension/src/debugger/AspireDebugSession.ts:136
- This lifecycle fix is covered only with a stubbed
vscode.debug.stopDebugging, so it cannot detect the real VS Code termination/disposal race or verify that the resource process is gone before AppHost teardown. The linked E2E coverage in #19133 is still an open draft, meaning this fix can merge without a regression test for the orphaned-process symptom. Add equivalent coverage underextension/src/test-e2e/that starts a resource, stops the Aspire session, and verifies the resource process exits.
const resourceStopResults = await Promise.allSettled(resourceDebugSessions.map(session => session.stopSession()));
extension/src/debugger/AspireDebugSession.ts:161
- An AppHost or parent stop rejection exits this
try/finallybefore the resource failures below are examined. When teardown failures coincide, the resource failure promised by this method is silently lost (and a parent failure also masks an AppHost failure). Capture each stop error, then preserve a single reason or throw one aggregate after every stop has been attempted.
try {
await this._appHostDebugSession?.stopSession();
}
finally {
await this.stopParentDebugSessionOnce();
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
70f1562 to
6d2f67a
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
extension/src/debugger/AspireDebugSession.ts:161
- When an AppHost stop rejects, control exits after the
finally, so the resource failures collected above are never rethrown; a parent-stop rejection can similarly replace both earlier failures. This loses the shutdown diagnostics this change promises whenever failures occur at more than one level. Capture AppHost and parent failures too, finish every stop, then throw the sole failure or an aggregate of all failures.
try {
await this._appHostDebugSession?.stopSession();
}
finally {
await this.stopParentDebugSessionOnce();
}
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Description
Three user-visible fixes to VS Code extension debugging, split out of #19133 so they are not blocked on the E2E harness in that PR.
1. Browser resources fail with an opaque adapter error.
browser.tsbuilt the debug type aspwa-${browser}, so any value other thanchrome/msedgeproducedConfigured debug type '<x>' is not supportedfrom VS Code, with no indication of which resource or which value caused it. Browser debug targets now resolve through aReadonlyMapallowlist and an unsupported value throws a named error identifying the resource and the accepted values.A
Mapis used rather than an object literal so that inputs such astoString,constructorand__proto__cannot reachObject.prototypeand assign a non-string value todebugConfiguration.type. The hosting API accepts arbitrary strings here, so the lookup has to be safe for any input.2. Empty
web_rootproduced a broken source map root. An empty string was assigned straight todebugConfiguration.webRoot, which js-debug resolves against the workspace and then fails to map sources. Empty and whitespace-only values are now left unset.3.
stopDebugging()stopped the AppHost before its resources. Stopping the AppHost first tears down the orchestrator that owns the resource processes, so resource debug sessions could be left orphaned or report spurious termination errors. Resource sessions are now stopped first (in parallel, excluding the AppHost session), then the AppHost, then the parent session. A resource stop failure is rethrown after the remaining sessions have been stopped, so one failure cannot strand the rest.Related
switchthat adds Firefox support and a permissivepwa-${x}passthrough default, and Show install hint when a language debug extension is missing #19131, which is adding a general "requested debugger is not available" failure path. See the discussion on those PRs for which mechanism should win; this PR fixes the immediate opaque-failure symptom.Validation
From
extension/:Checklist