Fix memory leaks, error handling, and type-safety issues in N-API bindings (#1422)

### Memory leaks (Critical)

Added missing `free(header)` on `TAKE_FAILED` early-return paths where `malloc`'d `rmw_request_id_t*` was never freed.
- `src/rcl_action_server_bindings.cpp` — `ActionTakeResultRequest`, `ActionTakeGoalRequest`, `ActionTakeCancelRequest`
- `src/rcl_service_bindings.cpp` — `RclTakeRequest`

### Error handling misordering (High)

Captured `rcl_get_error_string()` before calling `rcl_reset_error()` to avoid passing empty error messages to JavaScript.
- `src/rcl_action_client_bindings.cpp` — `ActionTakeFeedback`, `ActionTakeStatus`
- `src/rcl_action_server_bindings.cpp` — `ActionTakeGoalResponse`, `ActionTakeCancelResponse`, `ActionTakeResultResponse`

### Sequence number truncation (High)

Changed `int32_t`/`uint32_t` → `double` casts for 64-bit sequence numbers to avoid silent truncation above 2^31.
- `src/rcl_action_client_bindings.cpp`, `src/rcl_action_server_bindings.cpp`, `src/rcl_client_bindings.cpp`

### Exception-safe cleanup (Medium)

Used `RCPPUTILS_SCOPE_EXIT` in `src/rcl_subscription_bindings.cpp` for `RclTakeRaw` and `GetContentFilter` to ensure resource finalization even if N-API calls throw. Both scope-exit blocks check the fini return value and call `rcl_reset_error()` on failure to prevent stale error state from leaking into subsequent calls. Reordered `free(subscription)` before `ThrowAsJavaScriptException()` in `CreateSubscription`.

### Context rollback on init failure (Medium)

Wrapped post-init steps in `index.js` with `try/catch` that calls `context.tryShutdown()` if `generator.generateAll()` fails, preventing a half-initialized context from blocking re-initialization. The catch block normalizes non-Error exceptions before appending rollback failure details. Added corresponding test in `test/test-init-shutdown.js` with `try/finally` to guarantee sinon stub restoration even on assertion failure.

Fix: #1420

Author: minggangw

index.js

@@ -387,23 +387,35 @@ let rcl = {
 
     rclnodejs.init(context.handle, argv, context._domainId);
 
-    if (_rosVersionChecked) {
-      // no further processing required
-      return;
-    }
+    try {
+      if (_rosVersionChecked) {
+        // no further processing required
+        return;
+      }
 
-    const version = await getCurrentGeneratorVersion();
-    const forced =
-      version === null || compareVersions(version, generator.version(), '<');
-    if (forced) {
-      debug(
-        'The generator will begin to create JavaScript code from ROS IDL files...'
-      );
-    }
+      const version = await getCurrentGeneratorVersion();
+      const forced =
+        version === null || compareVersions(version, generator.version(), '<');
+      if (forced) {
+        debug(
+          'The generator will begin to create JavaScript code from ROS IDL files...'
+        );
+      }
 
-    await generator.generateAll(forced);
-    // TODO determine if tsd generateAll() should be here
-    _rosVersionChecked = true;
+      await generator.generateAll(forced);
+      // TODO determine if tsd generateAll() should be here
+      _rosVersionChecked = true;
+    } catch (error) {
+      try {
+        context.tryShutdown();
+      } catch (shutdownError) {
+        const initError =
+          error instanceof Error ? error : new Error(String(error));
+        initError.message += ` (rollback also failed: ${shutdownError.message})`;
+        throw initError;
+      }
+      throw error;
+    }
   },
 
   /**

src/rcl_action_client_bindings.cpp

@@ -132,7 +132,7 @@ Napi::Value ActionSendGoalRequest(const Napi::CallbackInfo& info) {
       rcl_action_send_goal_request(action_client, buffer, &sequence_number),
       RCL_RET_OK, rcl_get_error_string().str);
 
-  return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
+  return Napi::Number::New(env, static_cast<double>(sequence_number));
 }
 
 Napi::Value ActionSendResultRequest(const Napi::CallbackInfo& info) {
@@ -149,7 +149,7 @@ Napi::Value ActionSendResultRequest(const Napi::CallbackInfo& info) {
       rcl_action_send_result_request(action_client, buffer, &sequence_number),
       RCL_RET_OK, rcl_get_error_string().str);
 
-  return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
+  return Napi::Number::New(env, static_cast<double>(sequence_number));
 }
 
 Napi::Value ActionTakeFeedback(const Napi::CallbackInfo& info) {
@@ -163,9 +163,9 @@ Napi::Value ActionTakeFeedback(const Napi::CallbackInfo& info) {
 
   rcl_ret_t ret = rcl_action_take_feedback(action_client, buffer);
   if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
-    Napi::Error::New(env, rcl_get_error_string().str)
-        .ThrowAsJavaScriptException();
+    std::string error_msg = rcl_get_error_string().str;
     rcl_reset_error();
+    Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
     return Napi::Boolean::New(env, false);
   }
 
@@ -186,9 +186,9 @@ Napi::Value ActionTakeStatus(const Napi::CallbackInfo& info) {
 
   rcl_ret_t ret = rcl_action_take_status(action_client, buffer);
   if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
+    std::string error_msg = rcl_get_error_string().str;
     rcl_reset_error();
-    Napi::Error::New(env, rcl_get_error_string().str)
-        .ThrowAsJavaScriptException();
+    Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
     return Napi::Boolean::New(env, false);
   }
 
@@ -247,7 +247,7 @@ Napi::Value ActionSendCancelRequest(const Napi::CallbackInfo& info) {
       rcl_action_send_cancel_request(action_client, buffer, &sequence_number),
       RCL_RET_OK, rcl_get_error_string().str);
 
-  return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
+  return Napi::Number::New(env, static_cast<double>(sequence_number));
 }
 
 #if ROS_VERSION >= 2505  // ROS2 >= Kilted

src/rcl_action_server_bindings.cpp

@@ -126,6 +126,7 @@ Napi::Value ActionTakeResultRequest(const Napi::CallbackInfo& info) {
     return js_obj;
   }
 
+  free(header);
   return env.Undefined();
 }
 
@@ -148,6 +149,7 @@ Napi::Value ActionTakeGoalRequest(const Napi::CallbackInfo& info) {
     return js_obj;
   }
 
+  free(header);
   return env.Undefined();
 }
 
@@ -221,14 +223,14 @@ Napi::Value ActionTakeGoalResponse(const Napi::CallbackInfo& info) {
   free(header);
 
   if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
+    std::string error_msg = rcl_get_error_string().str;
     rcl_reset_error();
-    Napi::Error::New(env, rcl_get_error_string().str)
-        .ThrowAsJavaScriptException();
+    Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
     return env.Undefined();
   }
 
   if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
-    return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
+    return Napi::Number::New(env, static_cast<double>(sequence_number));
   }
   return env.Undefined();
 }
@@ -250,14 +252,14 @@ Napi::Value ActionTakeCancelResponse(const Napi::CallbackInfo& info) {
   free(header);
 
   if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
+    std::string error_msg = rcl_get_error_string().str;
     rcl_reset_error();
-    Napi::Error::New(env, rcl_get_error_string().str)
-        .ThrowAsJavaScriptException();
+    Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
     return env.Undefined();
   }
 
   if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
-    return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
+    return Napi::Number::New(env, static_cast<double>(sequence_number));
   }
   return env.Undefined();
 }
@@ -279,14 +281,14 @@ Napi::Value ActionTakeResultResponse(const Napi::CallbackInfo& info) {
   free(header);
 
   if (ret != RCL_RET_OK && ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
+    std::string error_msg = rcl_get_error_string().str;
     rcl_reset_error();
-    Napi::Error::New(env, rcl_get_error_string().str)
-        .ThrowAsJavaScriptException();
+    Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
     return env.Undefined();
   }
 
   if (ret != RCL_RET_ACTION_CLIENT_TAKE_FAILED) {
-    return Napi::Number::New(env, static_cast<int32_t>(sequence_number));
+    return Napi::Number::New(env, static_cast<double>(sequence_number));
   }
   return env.Undefined();
 }
@@ -463,6 +465,7 @@ Napi::Value ActionTakeCancelRequest(const Napi::CallbackInfo& info) {
     return js_obj;
   }
 
+  free(header);
   return env.Undefined();
 }
 

src/rcl_client_bindings.cpp

@@ -88,7 +88,7 @@ Napi::Value SendRequest(const Napi::CallbackInfo& info) {
   THROW_ERROR_IF_NOT_EQUAL(rcl_send_request(client, buffer, &sequence_number),
                            RCL_RET_OK, rcl_get_error_string().str);
 
-  return Napi::Number::New(env, static_cast<uint32_t>(sequence_number));
+  return Napi::Number::New(env, static_cast<double>(sequence_number));
 }
 
 Napi::Value RclTakeResponse(const Napi::CallbackInfo& info) {
@@ -104,7 +104,7 @@ Napi::Value RclTakeResponse(const Napi::CallbackInfo& info) {
   int64_t sequence_number = header.request_id.sequence_number;
 
   if (ret == RCL_RET_OK) {
-    return Napi::Number::New(env, static_cast<uint32_t>(sequence_number));
+    return Napi::Number::New(env, static_cast<double>(sequence_number));
   }
 
   rcl_reset_error();

src/rcl_service_bindings.cpp

@@ -96,6 +96,7 @@ Napi::Value RclTakeRequest(const Napi::CallbackInfo& info) {
     return js_obj;
   }
 
+  free(header);
   return env.Undefined();
 }
 

src/rcl_subscription_bindings.cpp

@@ -19,6 +19,8 @@
 
 #include <cstdio>
 #include <memory>
+#include <rcpputils/scope_exit.hpp>
+// NOLINTNEXTLINE
 #include <string>
 
 #include "macros.h"
@@ -108,12 +110,6 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
       rcl_ret_t ret = rcl_subscription_options_set_content_filter_options(
           expression.c_str(), argc, (const char**)argv, &subscription_ops);
 
-      if (ret != RCL_RET_OK) {
-        std::string error_string = rcl_get_error_string().str;
-        rcl_reset_error();
-        Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
-      }
-
       if (argc) {
         for (int i = 0; i < argc; i++) {
           free(argv[i]);
@@ -122,7 +118,10 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
       }
 
       if (ret != RCL_RET_OK) {
+        std::string error_string = rcl_get_error_string().str;
+        rcl_reset_error();
         free(subscription);
+        Napi::Error::New(env, error_string).ThrowAsJavaScriptException();
         return env.Undefined();
       }
     }
@@ -137,8 +136,8 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
     if (ret != RCL_RET_OK) {
       std::string error_msg = rcl_get_error_string().str;
       rcl_reset_error();
-      Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
       free(subscription);
+      Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
       return env.Undefined();
     }
 
@@ -157,9 +156,9 @@ Napi::Value CreateSubscription(const Napi::CallbackInfo& info) {
 
     return js_obj;
   } else {
-    Napi::Error::New(env, GetErrorMessageAndClear())
-        .ThrowAsJavaScriptException();
+    std::string error_msg = GetErrorMessageAndClear();
     free(subscription);
+    Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
     return env.Undefined();
   }
 }
@@ -194,10 +193,16 @@ Napi::Value RclTakeRaw(const Napi::CallbackInfo& info) {
     return env.Undefined();
   }
 
+  RCPPUTILS_SCOPE_EXIT({
+    rcl_ret_t fini_ret = rmw_serialized_message_fini(&msg);
+    if (fini_ret != RCL_RET_OK) {
+      rcl_reset_error();
+    }
+  });
+
   Napi::Buffer<char> buffer = Napi::Buffer<char>::Copy(
       env, reinterpret_cast<char*>(msg.buffer), msg.buffer_length);
-  THROW_ERROR_IF_NOT_EQUAL(rmw_serialized_message_fini(&msg), RCL_RET_OK,
-                           "Failed to deallocate message buffer");
+
   return buffer;
 }
 
@@ -369,6 +374,14 @@ Napi::Value GetContentFilter(const Napi::CallbackInfo& info) {
     return env.Undefined();
   }
 
+  RCPPUTILS_SCOPE_EXIT({
+    rcl_ret_t fini_ret =
+        rcl_subscription_content_filter_options_fini(subscription, &options);
+    if (fini_ret != RCL_RET_OK) {
+      rcl_reset_error();
+    }
+  });
+
   // Create result object
   Napi::Object result = Napi::Object::New(env);
   result.Set(
@@ -387,16 +400,6 @@ Napi::Value GetContentFilter(const Napi::CallbackInfo& info) {
   }
   result.Set("parameters", parameters);
 
-  // Cleanup
-  rcl_ret_t fini_ret =
-      rcl_subscription_content_filter_options_fini(subscription, &options);
-  if (fini_ret != RCL_RET_OK) {
-    std::string error_msg = rcl_get_error_string().str;
-    rcl_reset_error();
-    Napi::Error::New(env, error_msg).ThrowAsJavaScriptException();
-    return env.Undefined();
-  }
-
   return result;
 }
 

test/test-init-shutdown.js

@@ -15,7 +15,9 @@
 'use strict';
 
 const assert = require('assert');
+const sinon = require('sinon');
 const rclnodejs = require('../index.js');
+const generator = require('../rosidl_gen/index.js');
 
 describe('rclnodejs init and shutdown test suite', function () {
   this.timeout(60 * 1000);
@@ -99,6 +101,40 @@ describe('rclnodejs init and shutdown test suite', function () {
     }, 'shutting rclnodejs down twice should not cause an error to be thrown');
   });
 
+  it('rolls back context when generator init step fails', async function () {
+    rclnodejs.shutdownAll();
+
+    const generateAllStub = sinon
+      .stub(generator, 'generateAll')
+      .rejects(new Error('generator failed'));
+
+    try {
+      const indexPath = require.resolve('../index.js');
+      delete require.cache[indexPath];
+      const freshRclnodejs = require('../index.js');
+
+      const failedContext = new freshRclnodejs.Context();
+
+      await assert.rejects(async () => {
+        await freshRclnodejs.init(failedContext);
+      }, /generator failed/);
+      assert.ok(freshRclnodejs.isShutdown(failedContext));
+
+      generateAllStub.restore();
+
+      const recoveredContext = new freshRclnodejs.Context();
+      await assert.doesNotReject(async () => {
+        await freshRclnodejs.init(recoveredContext);
+      });
+      freshRclnodejs.shutdown(recoveredContext);
+      freshRclnodejs.removeSignalHandlers();
+    } finally {
+      if (generateAllStub.restore) {
+        generateAllStub.restore();
+      }
+    }
+  });
+
   it('rclnodejs create node without init should fail', async function () {
     assert.throws(
       () => {