Skip to content

fix(npm): do not report signal-terminated subprocesses as successful - #3928

Open
Youyuan Zhang (youyuan116) wants to merge 1 commit into
microsoft:mainfrom
youyuan116:fix/npm-signal-exit-status
Open

Youyuan Zhang (youyuan116) wants to merge 1 commit into
microsoft:mainfrom
youyuan116:fix/npm-signal-exit-status

Conversation

@youyuan116

@youyuan116 Youyuan Zhang (youyuan116) commented Oct 13, 2026 •

Copy link
Copy Markdown

What does this PR do?

Fix a false-success exit status in the shared npm platform launcher. When a subprocess is terminated by a signal, the child-process exit event supplies code = null; resolve(code || 0) incorrectly converts that to success. Use resolve(code ?? 1) to preserve numeric exit codes, including zero, and return failure when no numeric status is available.

Add 14 focused regression cases using the real launcher and local Node subprocesses, plus Azure/Fabric changelog entries. Include npm test discovery in the existing Windows/Linux/macOS analysis jobs so the new cases are executed in CI.

Pre-submission validation

  • Submission base: 9fbb8d4ce3b901d8eb4703be6e1a2b8c10277b66.
  • Local Windows, Node v24.11.1: 28 npm tests, 19 passed, zero failures, 9 POSIX-specific skips (including existing wrapper cases).
  • git apply --check and git diff --check passed.
  • The supplied historical Linux x86_64/Node v22.16.0 audit on an older base recorded 8 passes/6 failures before the fix and 14 passes/zero failures after it. This is historical evidence, not a Linux execution of this submission.
  • A local full .NET build was unavailable because the required SDK is absent. Current-revision Linux/macOS execution and hosted platform build results are now documented below.

Hosted CI verification (logs checked 2026-10-16)

Azure Pipelines build 6949712 completed successfully on 2026-10-13 for source commit 86d74b9b83d762f9cf9c136258f608768fe1c0ea. This was PR merge validation (refs/pull/3928/merge); the source SHA in the build metadata matches this branch. The hosted pipeline, including source analysis and platform build jobs, passed.

Agent Node.js Tests Passed Failed Skipped npm task log
Linux (Ubuntu 24.04) 24.21.0 28 27 0 1 TAP log
macOS 24.19.0 28 27 0 1 TAP log
Windows 24.21.0 28 19 0 9 TAP log

Counts were read from the actual Run npm launcher and wrapper tests TAP output, rather than inferred from the green job status. No npm tests were cancelled. All 29 currently reported GitHub checks, including license/cla, passed.

The six SIGTERM/SIGINT/SIGKILL regression cases (CLI and module paths) executed and passed on both Linux and macOS. Their only skipped test is Windows-specific LOCALAPPDATA behavior. The nine Windows skips cover POSIX signals and permission semantics.

GitHub issue number?

No tracking issue has been opened for this fix. This PR presents the reproduction, proposed change, and validation for maintainer review.

Pre-merge Checklist

  • Required for All PRs
    • Read contribution guidelines
    • PR title clearly describes the change
    • Commit history is clean with descriptive messages (cleanup guide)
    • Added comprehensive tests for new/modified functionality
    • Created a changelog entry if the change falls among the following: new feature, bug fix, UI/UX update, breaking change, or updated dependencies. Follow the changelog entry guide
  • For MCP tool changes:
    • One tool per PR: This PR adds or modifies only one MCP tool for faster review cycles
    • Updated servers/Azure.Mcp.Server/README.md and/or servers/Fabric.Mcp.Server/README.md documentation
    • Validate README.md changes running the script ./eng/scripts/Process-PackageReadMe.ps1. See Package README
    • For new or modified tool descriptions (excluding Fabric MCP tools), ran ToolDescriptionEvaluator and obtained a score of 0.4 or more and a top 3 ranking for all related test prompts
    • For tools with new names, including new tools or renamed tools, update consolidated-tools.json
    • For renamed tools, follow the Tool Rename Checklist and tag the PR with the breaking-change label
    • For new tools associated with Azure services or publicly available tools/APIs/products, add URL to documentation in the PR description
  • Extra steps for Azure MCP Server tool changes:
    • Updated command list in servers/Azure.Mcp.Server/docs/azmcp-commands.md
    • Ran ./eng/scripts/Update-AzCommandsMetadata.ps1 to update tool metadata in azmcp-commands.md (required for CI)
    • Updated test prompts in servers/Azure.Mcp.Server/docs/e2eTestPrompts.md
    • 👉 For Community (non-Microsoft team member) PRs:
      • Security review: Reviewed code for security vulnerabilities, malicious code, or suspicious activities before running tests (crypto mining, spam, data exfiltration, etc.)
      • Manual tests run: added comment /azp run mcp - pullrequest - live to run Live Test Pipeline

MCP tool and Azure tool-specific checklist sections are not applicable: this changes shared npm launcher infrastructure, without adding tools, modifying descriptions, or performing Azure/Fabric service operations.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution Youyuan Zhang (@youyuan116)! We will review the pull request and get back to you soon.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fix is focused, correctly preserves numeric exit codes, and has comprehensive cross-platform CI coverage.

0 open findings

What changed in this PR

Fixes npm launcher signal termination being incorrectly reported as success.

Changes:

  • Maps signal-terminated subprocesses to exit code 1.
  • Adds cross-platform regression tests and CI discovery.
  • Adds Azure and Fabric changelog entries.
File Description
eng/​npm/​platform/​index.js Corrects signal exit handling.
eng/​npm/​test/​platform.test.js Adds launcher regression coverage.
eng/​pipelines/​templates/​jobs/​analyze.yml Runs all npm tests across platforms.
servers/​Azure.Mcp.Server/​changelog-entries/​npm-fix-signal-exit-status.yaml Documents the Azure fix.
servers/​Fabric.Mcp.Server/​changelog-entries/​npm-fix-signal-exit-status.yaml Documents the Fabric fix.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@youyuan116

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@youyuan116

Copy link
Copy Markdown
Author

Could a maintainer review this exit-status fix when convenient? The PR pipeline passed, and I have updated the description with verified Linux, macOS, and Windows test counts and direct CI log links. The six SIGTERM/SIGINT/SIGKILL regression cases ran and passed on both Linux and macOS. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants