microsoft / microsoft/aspire

npm CLI package: unaddressed review follow-ups from #17297 (signal exit code, supportedRids drift, cache layout)

Open
#17,767 1 comment 0 reactions 0 assignees View on GitHub
area-cli triage:bot-seen triage:needs-human
Dominant language
C#
Stars
6.3k
Forks
991
Avg merge
2d 15h
Merged PRs (30d)
196

Description

Follow-ups from @radical's final review on #17297 (the npm CLI package work) that I didn't get to before merge. Capturing them here so they don't get lost. Two are concrete correctness issues; one is a design question. All should be sorted before the npm package goes GA.

### 1. Launcher reports exit 0 when the child is signaled externally
Thread: https://github.com/microsoft/aspire/pull/17297#discussion_r3330918109 (`eng/clipack/npm/aspire.js`)

If the native CLI's child process gets a signal directly (process supervisor, OOM killer, `pkill -INT `, `kubectl delete pod` — anything targeting the child rather than the wrapper), the wrapper's own still-registered forwarding listener consumes the re-raised self-signal and the wrapper exits **0**. So CI exit-code checks, systemd `restart=on-failure`, k8s restart policies, and `aspire ... && next` chains all see a terminated run as a clean success. A long-lived `aspire run` killed by OOM falsely reports success.

Fix: remove the forwarding listeners before re-raising the signal so Node default-dies with the signal:
```js
child.on('exit', (code, signal) => {
if (signal) {
for (const s of forwardedSignals) {
process.removeAllListeners(s);
}
process.kill(process.pid, signal);
return;
}
process.exit(code === null ? 1 : code);
});
```
Add a regression test in `AspireJsLauncherTests` that signals the cached child binary directly and asserts a nonzero / `128 + signum` exit.

### 2. `$supportedRids` can silently drift from the actually-built RIDs
Thread: https://github.com/microsoft/aspire/pull/17297#discussion_r3330918113 (`eng/scripts/pack-cli-npm-package.ps1`)

The RID list lives in 5 places. `LauncherSupportsAllRidsDefinedInPackScript` only covers `$supportedRids` ↔ `launcher.detectRid`. The dangerous direction: add a clipack csproj for a new RID + update the pipeline `$expectedRids` (build + release both pass) but forget `$supportedRids` → the pointer's `optionalDependencies` and `aspire-package-map.json` silently omit it → end users on the new platform install the pointer, npm installs no RID package, `detectRid` returns the new RID, and the launcher throws "No Aspire CLI npm package is available for RID ''" at runtime. The reverse drift is also silent.

Fix: derive `$supportedRids` from the `eng/clipack/Aspire.Cli.*.csproj` filenames (same trick `Publishing.props` already uses for `_ExpectedCliRids`), and update the test to assert against that canonical csproj-derived set:
```powershell
$clipackDir = Join-Path $repoRoot 'eng\clipack'
$supportedRids = @(
Get-ChildItem -LiteralPath $clipackDir -Filter 'Aspire.Cli.*.csproj' -File |
ForEach-Object { $_.BaseName -replace '^Aspire\.Cli\.', '' } |
Sort-Object
)
if ($supportedRids.Count -eq 0) {
throw "No Aspire.Cli.*.csproj projects found under '$clipackDir'; cannot determine the npm supported RID set."
}
```

### 3. Do we actually need the `` segment in the npm cache path?
Thread: https://github.com/microsoft/aspire/pull/17297#discussion_r3330884648 (`eng/clipack/npm/aspire.js`)

npm is the only one of our install routes that adds an explicit `` segment (`~/.aspire/npm///bin/aspire`). Since `optionalDependencies` + `os`/`cpu`/`libc` selectors guarantee exactly one RID package per npm prefix, the launcher always reads the same `node_modules` entry and always `detectRid()`s to the same RID — so `~/.aspire/npm//bin/aspire` would keep the in-flight-upgrade safety (the `` segment) without committing to a layout shape we couldn't find a load-bearing use for. Decide whether to simplify before GA. Related: the broader `~/.aspire/` route-layout convention is tracked in #17762.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.