Skip to content

fix(cdk-assets-lib): pLimit.dispose() doesn't stop new jobs from starting - #1877

Open
Adityaj0 wants to merge 1 commit into
aws:mainfrom
Adityaj0:fix/p-limit-dispose-race
Open

fix(cdk-assets-lib): pLimit.dispose() doesn't stop new jobs from starting#1877
Adityaj0 wants to merge 1 commit into
aws:mainfrom
Adityaj0:fix/p-limit-dispose-race

Conversation

@Adityaj0

Copy link
Copy Markdown
Contributor

Closes #1876

Reason for this change

pLimit()'s dispose() method is documented and tested to stop new jobs from starting, but dispatch() (called every time a new task is submitted) never checked the stopped flag - only resumeNext() (which runs when an already-active job finishes) did. So a task submitted after dispose() would start immediately whenever a concurrency slot happened to be free at submission time, silently violating dispose()'s contract.

Description of changes

  • dispatch() now returns immediately if stopped is true, so no new job starts after dispose().
  • ret() now rejects immediately with 'Task has been cancelled' if a task is submitted after dispose(), so callers don't hang waiting on a promise that would otherwise never settle (queued-but-never-dispatched).
  • Added a regression test to test/private/p-limit.test.ts that fails before the fix (job runs despite dispose()) and passes after.

Description of how you validated changes

Ran the full @aws-cdk/cdk-assets-lib test suite locally - all 104 tests across 14 suites pass, including the new regression test and the existing p-limit.test.ts tests.

Test Suites: 14 passed, 14 total
Tests:       104 passed, 104 total

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…ting

dispatch() never checked the `stopped` flag, only resumeNext() (called
when an already-active job finishes) did. So calling dispose() and then
submitting a new task via the limiter, while a concurrency slot happened
to be free, would start that task immediately instead of rejecting it -
silently violating dispose()'s documented/tested contract that "new jobs
aren't started after dispose is called".

Guard dispatch() with the stopped flag, and reject immediately when a
task is submitted after dispose() so callers don't hang waiting on a
promise that will never settle.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.10%. Comparing base (7ff50e7) to head (49611cf).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1877   +/-   ##
=======================================
  Coverage   91.10%   91.10%           
=======================================
  Files          80       80           
  Lines       12205    12205           
  Branches     1742     1742           
=======================================
  Hits        11119    11119           
  Misses       1050     1050           
  Partials       36       36           
Flag Coverage Δ
suite.unit 91.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cdk-assets-lib: pLimit.dispose() does not stop new jobs from starting when a concurrency slot is free

2 participants