@azure/cosmos: request handler - fix 'abortSignal' and event listener memory leak#35428
Conversation
|
Thank you for your contribution @petriposio! We will review the pull request and get back to you soon. |
|
@jeremymeng I remember you've dealt with similar leaks in the past, does this fix look correct? |
|
This fix looks good to me. Thanks for your contribution @petriposio! @amanrao23 @aditishree1 please have a look |
|
Hi @petriposio. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
|
Still active. Waiting for a review. Could the code owners have a look, please? Is there anything I can do to get this merged? |
e3bece2 to
7e9ee48
Compare
Extracts the suspicious-PR detection logic from the workflow's inline
bash into a proper TypeScript module at eng/tools/detect-suspicious-pr/
with full test coverage.
Structure:
src/detect.ts – pure detection functions (checkInjection, checkCiFiles,
checkLifecyclePatch, detectSuspiciousPR)
src/run.ts – CLI entry point: reads JSON from stdin, exits 1 if
suspicious
src/index.ts – public re-exports
test/detect.test.ts – 124 vitest tests across 6 suites
The workflow now:
1. Sparse-checks out the detection tool from the base branch
2. Installs its deps with npm
3. Gathers PR metadata via gh CLI and builds a JSON payload
4. Pipes it to the TypeScript detector via tsx
5. Closes the PR if the detector exits non-zero
Tests cover:
- 35 malicious branch name patterns (injection, PS, shell, URLs, etc.)
- 15 benign branch names (including real merged community PRs)
- 12 malicious + 10 benign PR titles
- 4 commit message cases
- 9 protected CI/CD paths + 4 registry/build configs + 10 safe paths
- 10 malicious + 6 benign npm lifecycle patches
- 8 end-to-end integration scenarios (real PR #37419, #37403, #35428)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Packages impacted by this PR
@azure/cosmos
Issues associated with this PR
Describe the problem that is addressed by this PR
In
sdk/cosmosdb/cosmos/src/request/RequestHandler.tsan event listener is added to theabortSignal. The event listener is never removed. This causes a memory leak.This bug might also eventually lead to the user provided abort signal to be ignored if
events.setMaxListenersis configured in NodeJS.A reproducible example
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists