Fix Azure Functions HTTPS launches in VS Code - #19001
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 28d91674-6ae5-424f-9193-6cccf604e336
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19001Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19001" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Fixes VS Code HTTPS launches for .NET Azure Functions resources.
Changes:
- Builds and launches Functions from compiled output with shell-safe HTTPS arguments.
- Separates Run-mode task tracking from Debug-mode CoreCLR attachment.
- Adds lifecycle, quoting, build, and race-condition unit coverage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
extension/src/utils/cmdShim.ts |
Exports cmd argument quoting. |
extension/src/test/azureFunctionsDebugger.test.ts |
Adds Functions debugger tests. |
extension/src/test/aspireDebugSession.test.ts |
Tests already-started session tracking. |
extension/src/loc/strings.ts |
Adds localized launch errors. |
extension/src/debugger/languages/dotnet.ts |
Exports DotNetService. |
extension/src/debugger/languages/azureFunctions.ts |
Implements build, launch, quoting, and task lifecycle logic. |
extension/src/debugger/debuggerExtensions.ts |
Supports prepared, already-started sessions. |
extension/src/debugger/AspireDebugSession.ts |
Tracks and reports external session termination. |
extension/src/dcp/AspireDcpServer.ts |
Handles already-started resource sessions. |
extension/package.nls.json |
Adds localization source entries. |
extension/loc/xlf/aspire-vscode.xlf |
Updates generated localization data. |
|
I’m holding this PR until #18984 lands because both changes use |
…de-functions-https
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (3)
extension/src/debugger/languages/azureFunctions.ts:299
- All new tests stub
startFuncProcessand synthesize task events, so they do not exercise the integration that failed in #18872: the Azure Functions API joins these arguments into aShellExecution, then VS Code applies the configured task shell. A quoting regression or duplicate NoDebug launch would still pass. Add extension E2E coverage that launches an HTTPS Functions resource through DCP and verifies the host starts once and is reachable.
result = await api.startFuncProcess(buildOutputPath, quoteFuncHostArguments(args), dcpEnv);
extension/src/test/azureFunctionsDebugger.test.ts:541
- This helper supplies
{}as the DCP server, but the run-mode test tracks a session and then disposes it. Disposal resolvestermination, whose callback calls_dcpServer.sendNotification, producing an unhandledTypeErrorafter the test. Supply asendNotificationstub here.
return new AspireDebugSession(parentDebugSession, {} as any, {} as any, terminalProvider as any, () => { });
extension/src/debugger/languages/azureFunctions.ts:45
- VS Code automation profiles support shell
args, but this code ignores them when deciding how to quote. A valid cmd profile using/v:onwill expand!NAME!even inside the quoted argument;quoteCmdArgumentexplicitly assumes/v:off, so a generated certificate password can be corrupted or expanded. Read and validate the profile arguments (at minimum rejecting delayed expansion), or launch through a path that controls cmd's parsing mode.
type TerminalProfileConfiguration = {
path?: string | string[];
source?: string;
};
Always rebuild Functions projects before launching from compiled output, and reject cmd profiles that enable delayed expansion before quoting HTTPS arguments. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7e47e8f9-ec1e-4250-8fe7-5e218d032a78
Reject cmd exclamation arguments without relying on terminal profile state, and report the worker PID for already-started no-debug sessions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7e47e8f9-ec1e-4250-8fe7-5e218d032a78
|
Copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
extension/src/debugger/languages/azureFunctions.ts:117
- The “shell-safe” regexes exclude
=, which is common in flags like--port=7071,--urls=https://..., etc. Those arguments will be unnecessarily quoted, contradicting the comment about keeping ordinary flags unchanged and potentially breaking any exact-argument inspection done by the Azure Functions extension before it flattens args. Expanding the safe character set to include=(and any other universally-safe characters you intend to preserve) would avoid altering these common flags.
if (funcHostArgs.every(argument => /^[A-Za-z0-9_./:-]+$/.test(argument))) {
return funcHostArgs;
}
extension/src/debugger/languages/azureFunctions.ts:131
- The “shell-safe” regexes exclude
=, which is common in flags like--port=7071,--urls=https://..., etc. Those arguments will be unnecessarily quoted, contradicting the comment about keeping ordinary flags unchanged and potentially breaking any exact-argument inspection done by the Azure Functions extension before it flattens args. Expanding the safe character set to include=(and any other universally-safe characters you intend to preserve) would avoid altering these common flags.
const isShellSafe = shell === 'posix'
? /^[A-Za-z0-9_./:-]+$/.test(argument)
: /^[A-Za-z0-9_./:\\-]+$/.test(argument);
if (isShellSafe) {
return argument;
}
extension/src/loc/strings.ts:31
- The guidance says to configure
terminal.integrated.automationProfile, but the code actually reads platform-scoped keys liketerminal.integrated.automationProfile.windows|osx|linux(and can also consultdefaultProfile.*). To reduce user confusion, consider updating this message to reference the platform-specific setting key(s) that are actually used.
export const azureFunctionsUnsupportedTaskShell = vscode.l10n.t('The configured VS Code task shell is not supported for Azure Functions launch arguments. Configure terminal.integrated.automationProfile to use PowerShell, Command Prompt, bash, zsh, fish, or WSL.');
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7e47e8f9-ec1e-4250-8fe7-5e218d032a78
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7e47e8f9-ec1e-4250-8fe7-5e218d032a78
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 1 / 100 test projects · 2 jobs, from 19 changed files. Selected test projects (1 / 100)
Selected jobs (2)
How these were chosen — grouped by what changed📄 Job reasons
Selection computed for commit |
|
✅ No documentation update needed. Step 5 branch taken: Triggered signals (1): Why this is a false positive: These are internal debugger-launch arguments the VS Code extension previously forwarded incorrectly to the The existing docs page No docs PR was drafted. |
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
Review of the Azure Functions HTTPS launch fix. The per-shell quoting logic and its unit tests are solid, and the earlier {}-as-DCP-server leak is fixed. Seven problems below, ordered by impact.
Note on CI: CI had never run on this PR — the last completed CI run was on 97e0501e, but the last four commits were pushed with GITHUB_TOKEN by the Copilot agent workflow, which does not fire pull_request events. I closed/reopened the PR to trigger it and it is now fully green (337 passing).
However, the new azure-functions E2E shard still has not run anywhere. extension_e2e_tests in tests.yml:715 is force-skipped repo-wide via if: ${{ false && ... }} (issue #18412), so the shard added in 538096b9, the pinned Core Tools 4.12.1 / VSIX checksums, the func --version preflight and the generated Functions project are all unvalidated. Worth running it manually before merge — see findings 2 and 7.
Breakdown: 2 High, 1 Medium, 4 Low.
| extensionLogOutputChannel.info(`Captured func host task for runId ${debugConfiguration.runId}: ${execution.task.name}`); | ||
| taskExecutionsByRunId.set(debugConfiguration.runId, execution); | ||
| }; | ||
| const taskStartSubscription = vscode.tasks.onDidStartTaskProcess(event => { |
There was a problem hiding this comment.
High — failure to capture the func task is silent and breaks termination reporting.
captureFuncExecution only runs if onDidStartTaskProcess fires while taskStartSubscription is alive. In vscode-azurefunctions v1.22, startFuncProcessFromApi calls taskUtils.executeIfNotActive(funcTask) — when an equivalent func task is already active (the AF source comments explicitly cover start/stop/restart in quick succession) no new task process starts and no event fires.
If funcExecution stays undefined:
taskExecutionsByRunIdis never set, sokillFuncProcesscan never calltaskExecution.terminate()and thefunc hostleaks.- In run mode,
taskEndSubscriptioncomparesevent.execution !== funcExecution, which never matches, soterminationnever resolves and the resource stays Running in the dashboard forever after the host exits or crashes.
Nothing is logged either — the previous diff-based capture at least emitted extensionLogOutputChannel.warn when it was ambiguous.
Suggested fix — after result.success:
if (!funcExecution) {
extensionLogOutputChannel.warn(
`Did not capture a func host task for runId ${debugConfiguration.runId}; termination and cleanup will be degraded.`);
}and in run mode add a fallback termination signal (e.g. poll process.kill(workerPidNumber, 0)) so the resource state still transitions when the host dies.
| profiles: { | ||
| [projectName]: { | ||
| commandName: 'Project', | ||
| commandLineArgs: `--useHttps --cert ${certificatePath} --password ${certificatePassword}`, |
There was a problem hiding this comment.
High (coverage) — the new E2E never reaches the quoting code this PR is about.
The fixture passes --useHttps --cert <path> --password AspireE2E, Aspire appends --port <n>, and the workspace lives under mkdtempSync(tmp, 'aev-'). Every character in every argument is in [A-Za-z0-9_./:-], so quoteFuncHostArguments takes the fast path at line 115 and returns the array verbatim. getFuncHostTaskShell(), classifyFuncHostTaskShell() and quoteFuncHostArgument() are never invoked in the E2E.
That leaves the entire shell-resolution/quoting surface — precisely the part that can only be validated against a real ShellExecution and a real shell — covered by unit-test stubs only.
Suggested fix: force the quoting path in the fixture, e.g.
const certificatePassword = "Aspire E2E p@ss'\\";(space + apostrophe + backslash), and/or place the .pfx under a directory containing a space. That makes the Linux/bash quoter run end-to-end and would actually fail if quoteShellArg regressed.
| const complete = (exitCode: number): void => { | ||
| if (completed) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Medium — complete() runs cleanupRun on natural func-host exit, signalling a possibly-recycled PID.
When the task-end event fires, complete() calls cleanupRun(runId) → killFuncProcess → process.kill(workerPidNumber) on a worker that has already exited. The extension host is not the worker's parent, so it cannot reap it, and the PID may already have been reused by an unrelated process — which then receives SIGTERM.
Previously cleanupRun was only reached on the explicit stop/failure paths, so this exit-driven kill is new behaviour.
Suggested fix: distinguish the natural-exit path from the stop path — drop the tracked PID before cleanup when complete is invoked from the onDidEndTaskProcess handler (workerPidsByRunId.delete(runId)), or gate the kill on process.kill(pid, 0) succeeding.
| extensionLogOutputChannel.info(`Azure Functions worker process started (PID: ${workerPid})`); | ||
|
|
||
| // Track the worker PID for cleanup | ||
| const workerPidNumber = parseInt(workerPid, 10); |
There was a problem hiding this comment.
Low — parseInt(result.processId, 10) can produce NaN, reported to DCP as pid: null.
If the AF API returns success: true with a non-numeric processId, workerPidNumber becomes NaN. In run mode that flows into ProcessRestartedNotification.pid, and JSON.stringify serialises NaN as null — so DCP silently receives a null PID instead of the launch failing loudly. The same NaN is also stored in workerPidsByRunId and later passed to process.kill.
Suggested fix, immediately after the parse:
if (!Number.isInteger(workerPidNumber) || workerPidNumber <= 0) {
throw new Error(`Azure Functions host returned an invalid process id: ${result.processId}`);
}| } | ||
|
|
||
| if (identity.includes('git bash') || identity.includes('wsl') || identity.includes('cygwin') || identity.includes('msys') || | ||
| /(?:^|[\\/\s])(ba|z|fi|k)?sh(?:\.exe)?(?:$|\s)/.test(identity)) { |
There was a problem hiding this comment.
Low — POSIX-compatible dash/ash shells are rejected as unsupported.
The classifier regex is /(?:^|[\\/\s])(ba|z|fi|k)?sh(?:\.exe)?(?:$|\s)/, so /bin/dash and /bin/ash do not match and fall through to throwUnsupportedTaskShell(). Both use quoting semantics identical to bash/sh, so failing the launch closed is unnecessary — and dash is /bin/sh on Debian/Ubuntu, which is a plausible terminal.integrated.automationProfile.linux setting.
(csh/tcsh are correctly rejected — their quoting genuinely differs.)
Suggested fix: add da|a to the prefix alternation — /(?:^|[\\/\s])(ba|z|fi|k|da|a)?sh(?:\.exe)?(?:$|\s)/ — and add a classifier unit test for /bin/dash.
| const certificatePassword = 'AspireE2E'; | ||
| fs.mkdirSync(propertiesDirectory, { recursive: true }); | ||
| fs.writeFileSync(path.join(projectDirectory, `${projectName}.csproj`), `<Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
There was a problem hiding this comment.
Low — the E2E Functions fixture pins net8.0.
The generated project targets net8.0, but setup-dotnet installs only the global.json SDK (10.0.201). The build then needs the 8.0 reference packs from NuGet, and func host start needs an 8.0 runtime present on the runner — neither is guaranteed by the workflow, so the shard's success depends on what happens to be baked into the runner image.
Suggested fix: target net10.0 to match the SDK the job actually installs, or add an explicit dotnet-version: 8.0.x to setup-dotnet for this shard.
|
|
||
| let result: StartFuncProcessResult; | ||
| try { | ||
| result = await api.startFuncProcess(buildOutputPath, quoteFuncHostArguments(args), dcpEnv); |
There was a problem hiding this comment.
Low — argument/shell validation happens after a full dotnet build.
quoteFuncHostArguments(args) is evaluated inline in the startFuncProcess call here, i.e. after buildDotNetProject and getDotNetTargetPath. A purely static configuration error (% or ! under cmd, an unsupported shell) therefore makes the user sit through a full project build before surfacing.
Suggested fix: hoist const quotedArgs = quoteFuncHostArguments(args); above the buildDotNetProject call so it fails fast.
The branch conflicted with main, so GitHub could not build refs/pull/19133/merge and no `pull_request` workflow had ever been scheduled for the PR - `ci.yml` had zero runs on this branch while sibling PRs were running normally. Merging is what unblocks CI. microsoft#19001 landed the Azure Functions HTTPS launch path, which touches the same four places this branch does. - `.github/workflows/extension-e2e-tests.yml` and `extension/CONTRIBUTING.md`: both shards kept. The resource-debugger prose no longer lists Azure Functions among the languages whose debugger is not installed into the E2E VS Code instance, because that shard now installs it. - `extension/scripts/run-e2e.js`: the AppHost fixture emits both opt-in resources. `writeAzureFunctionsProject` runs before `writeAppHostProject` as on main, `writeNodeAppFixture` after it, and the csproj template carries both the `Aspire.Hosting.JavaScript` and `Aspire.Hosting.Azure.Functions` references. - `extension/src/debugger/languages/azureFunctions.ts`: union of both import sets. `AzureFunctionsLaunchConfiguration` is dropped from the value import because the merged body only narrows through `isAzureFunctionsLaunchConfiguration`. - `extension/src/test/azureFunctionsDebugger.test.ts`: an add/add conflict. Took main's suite, which is the far larger one, and re-added this branch's metadata coverage - adapter identity, session naming from the project file, project resolution, and the two rejection paths - as a second suite, since main covers none of it and the naming tests are what pin the localized `azureFunctionsDisplayName` / `azureFunctionsLabel` strings. The missing-extension test needed a real change rather than a straight port: main now builds the project before it resolves the extension, so the unported test failed on `spawn dotnet ENOENT` instead of reaching the lookup. It stubs `DotNetService` the way the neighbouring tests do. - `extension/loc/xlf/aspire-vscode.xlf`: regenerated with `yarn localize` from the merged `package.nls.json` instead of hand-merging generated XML. `compile-tests`, `compile-e2e`, `lint` and the unit suite (1511 passing, 0 failing) are clean on the merge result. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Filed #19138 to address the remaining comments left on this PR |
Description
Fixes #18872.
Fixes VS Code launching .NET Azure Functions HTTPS resources through the normal .NET path, which incorrectly passed
--certand--passwordtodotnet.This change:
func hostfrom its compiled output directory.