[dave-365]

[dave-365]

Problem Background

During application hot reload, pending AsyncProgressWorker operations can cause Node.js environment teardown issues, potentially resulting in event-loop hangs or application crashes.

This is particularly relevant to developers using node-addon-api in applications with hot-reload workflows.

Related discussion: [Issue #1109](#1109)

Proposed Solution

This PR introduces environment cleanup handling for AsyncProgressWorkerBase to ensure that pending asynchronous operations are properly handled during environment teardown.

The intention is to prevent event-loop hangs and improve application stability during hot reload.

Impact

This fix may benefit developers using AsyncProgressWorker in applications that frequently reload or recreate their Node.js environments.

Review Request

Could the maintainers please review the proposed cleanup approach and provide feedback on any lifecycle or thread-safety concerns?

[KevinEady]

Hi @dave-365 ,

Is there any way to include a test in this PR? We have some tests already that check for various behavior in terminating environments, e.g. https://github.com/nodejs/node-addon-api/blob/main/test/error_terminating_environment.js . Maybe you can follow this pattern?

[dave-365]

Hi @dave-365 ,

Is there any way to include a test in this PR? We have some tests already that check for various behavior in terminating environments, e.g. https://github.com/nodejs/node-addon-api/blob/main/test/error_terminating_environment.js . Maybe you can follow this pattern?

@KevinEady Okay I will include test in this PR.

[dave-365]

Hi @dave-365 ,

Is there any way to include a test in this PR? We have some tests already that check for various behavior in terminating environments, e.g. https://github.com/nodejs/node-addon-api/blob/main/test/error_terminating_environment.js . Maybe you can follow this pattern?

Hi @KevinEady

I have addressed the review feedback and added automated tests following the pattern in test/error_terminating_environment.js:

1. Automated Test Coverage: Added test/async_progress_worker_terminating_environment.js testing both:
   • Environment termination while the worker thread is actively executing and sending progress.
   • Normal completion prior to environment teardown (verifying cleanup hook removal and no use-after-free).
   • Both scenarios are tested against C++ exceptions (binding.node) and no-exceptions (binding_noexcept.node) configurations.
2. Lifecycle & UAF Prevention:
   • In ~AsyncProgressWorkerBase(), _cleanup_hook.Remove(Env()) safely removes the hook when the worker completes normally.
   • If OnEnvironmentTeardown() runs, _cleanup_hook is reset to empty to avoid duplicate removal.
   • If the environment is terminating, status is marked napi_cancelled so JavaScript callbacks (OnOK/OnError) are skipped during teardown, cleanly routing to destruction.
3. Linting and Full Test Suite: All tests pass (node test), and formatting complies with clang-format and eslint.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 34.61538% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.27%. Comparing base (25c6b21) to head (76e2ad0).

Files with missing lines	Patch %	Lines
napi-inl.h	34.61%	11 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1759      +/-   ##
==========================================
- Coverage   63.68%   63.27%   -0.41%     
==========================================
  Files           3        3              
  Lines        2065     2091      +26     
  Branches      731      744      +13     
==========================================
+ Hits         1315     1323       +8     
- Misses        162      173      +11     
- Partials      588      595       +7     

☔ 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.

[dave-365]

"Hi @KevinEady , I have pushed a fix for the Windows CI failure in commit f4d293c (properly awaiting worker termination and removing script evaluation from the finalizer). Could you please approve the workflow run when you have a moment? Thank you!"

[KevinEady]

Not sure if the failure was due to the code, since it looks like it was failing while building, but re-ran anyway

[dave-365]

Not sure if the failure was due to the code, since it looks like it was failing while building, but re-ran anyway
@KevinEady Thought it was building issue failed now again Unix failed I will check on local.

[dave-365]

@KevinEady

the test case was execution times was low for that window that is the root causse in the test file i fixed and i tested fully download visual studio in local now it's npm run pretest working fix no error.
Could you please approve the workflow run when you have a moment? Thank you!"