switched back to disabled cpp exceptions

Author: gms1

binding.gyp

@@ -16,7 +16,7 @@
       "include_dirs": [
         "<!@(node -p \"require('node-addon-api').include\")"],
       "dependencies": [
-        "<!(node -p \"require('node-addon-api').targets\"):node_addon_api_except"
+        "<!(node -p \"require('node-addon-api').targets\"):node_addon_api"
       ],
       "conditions": [
         ["sqlite != 'internal'", {
@@ -55,7 +55,6 @@
         ["OS=='win'", {
           "msvs_settings": {
             "VCCLCompilerTool": {
-              "ExceptionHandling": 1,
               "BufferSecurityCheck": "true",
               "ControlFlowGuard": "Guard"
             },

memory-bank/decisionLog.md

@@ -214,17 +214,30 @@ if (locked && pending == before_pending) {
 
 ---
 
-### 2026-03-29: NAPI Exception Handling
+### 2026-04-25: NAPI Exception Handling — Switched to NAPI_DISABLE_CPP_EXCEPTIONS
 
-**Decision**: Use `node_addon_api_except` instead of `NAPI_DISABLE_CPP_EXCEPTIONS=1`
+**Decision**: Use `node_addon_api` (NAPI_DISABLE_CPP_EXCEPTIONS mode) instead of `node_addon_api_except`
 
 **Rationale**:
-- Commit 48e95e8a0d32277449c269b41fba6419acb21418 changed the build configuration
-- Using `node_addon_api_except` from node-addon-api provides proper exception handling support
-- This is the recommended approach for modern node-addon-api versions
+- The previous decision (2026-03-29) to use `node_addon_api_except` caused uncatchable C++ exceptions that crashed the process with SIGABRT
+- `TRY_CATCH_CALL`'s `try { callback.Call() } catch (Napi::Error& e) { throw; }` re-threw C++ exceptions from within async `Work_After*` callbacks where there was no C++ catch handler on the stack → `std::terminate()` → `abort()`
+- With `NAPI_DISABLE_CPP_EXCEPTIONS=1`, `Napi::Error` is never thrown as a C++ exception — it's just a JavaScript value, so all errors are catchable from JS
+- The `node_addon_api` target includes `noexcept.gypi` which defines `NODE_ADDON_API_DISABLE_CPP_EXCEPTIONS` and `-fno-exceptions`
+- No explicit `NAPI_DISABLE_CPP_EXCEPTIONS=1` define needed in `binding.gyp` — the dependency handles it
+
+**Additional fixes**:
+- Removed dead code: `TRY_CATCH_CALL` try/catch block and `throw;`, `g_env_shutting_down` mechanism
+- Initialized `retryErrors` in C++ `Backup::Backup()` constructor with `[SQLITE_BUSY, SQLITE_LOCKED]` to prevent `FATAL ERROR: Error::New napi_get_last_error_info` when `retryErrors.Value()` is called on an empty `Napi::Reference<Array>`
 
 **Files Changed**:
-- `binding.gyp`: Changed from `node_addon_api` to `node_addon_api_except` dependency
+- `binding.gyp`: Changed dependency from `node_addon_api_except` to `node_addon_api`
+- `src/macros.h`: Removed `#include <atomic>`, `g_env_shutting_down`, and try/catch from `TRY_CATCH_CALL`
+- `src/node_sqlite3.cc`: Removed `g_env_shutting_down`, `EnvCleanupHook`, `napi_add_env_cleanup_hook`
+- `src/backup.cc`: Initialize `retryErrors` in constructor
+- `test/uncatchable-exceptions.test.js`: Integration tests for both fixed scenarios
+- `test/uncatchable-scenarios/`: Child process crash reproduction scripts
+
+**Supersedes**: 2026-03-29 decision to use `node_addon_api_except`
 
 ---
 

memory-bank/issues.md

@@ -1,58 +1,49 @@
 # issues
 
-## Uncatchable Native Addon Exceptions (C++ → Napi::Error)
+## Uncatchable Native Addon Exceptions (C++ → Napi::Error) — FIXED
 
-Several operations on the native addon throw `Napi::Error` instances from C++ code that **cannot be caught by JavaScript `try/catch` or `Promise.catch()`**. These exceptions propagate as C++ exceptions through the N-API layer and terminate the process with an abort signal.
+**Status**: Fixed as of 2026-04-25. Switched from `node_addon_api_except` to `node_addon_api` (NAPI_DISABLE_CPP_EXCEPTIONS mode), removed dead `TRY_CATCH_CALL` try/catch and `g_env_shutting_down`, and initialized `retryErrors` in the C++ Backup constructor.
 
-### Known uncatchable scenarios
+### Previously uncatchable scenarios (now catchable)
 
-#### 1. `Backup.step()` after `Backup.finish()`
+#### 1. `Backup.step()` after `Backup.finish()` — FIXED
 
-Calling `step()` on a backup handle that has already been finished throws:
-```
-terminate called after throwing an instance of 'Napi::Error'
-  what(): SQLITE_MISUSE: Backup is already finished
-Aborted (core dumped)
-```
+Previously crashed with `FATAL ERROR: Error::New napi_get_last_error_info` in `GetRetryErrors()` because `retryErrors` (`Napi::Reference<Array>`) was never initialized in the C++ constructor — it was only set by the JS wrapper in `Database.prototype.backup()`. Calling `retryErrors.Value()` on an empty reference caused a N-API fatal error.
 
-**Affected code paths**:
-- `lib/promise/backup.js` line 97-99: The `if (!this._backup)` guard only checks for `null`, but after `finish()` the native handle is in a "finished" state that still exists but is invalid
-- The callback API (`lib/sqlite3-callback.js`) has the same issue — `backup.step()` after `backup.finish()` is uncatchable
+**Fix**: Initialize `retryErrors` in the C++ `Backup::Backup()` constructor with default values `[SQLITE_BUSY, SQLITE_LOCKED]`, matching what the JS wrapper sets. Now `Backup.step()` after `Backup.finish()` returns a normal JS error: `SQLITE_MISUSE: Backup is already finished`.
 
-**Workaround in promise wrapper**: The `SqliteBackup.step()` method checks `if (!this._backup)` before calling the native `step()`, but this only catches the case where `_backup` is explicitly set to `null`. After `finish()`, the `_backup` reference is still non-null but the native handle is invalid. Setting `_backup = null` in `finish()` would prevent the crash but would change the semantics of the `idle`/`completed`/`failed` getters after finish.
+**Test**: `test/uncatchable-scenarios/backup-step-after-finish.js` — exit code 0 with "OK: got expected error".
 
-**Test impact**: Cannot write integration tests for `step()` after `finish()` — the process aborts before Mocha can report the failure. Unit tests with mocks are used instead (see `test/promise.unit.test.js`).
+#### 2. JS callback throw inside Work_After* async callback — FIXED
 
-#### 2. Statement operations after `Database.close()`
+Previously, `TRY_CATCH_CALL`'s `try { callback.Call() } catch (Napi::Error& e) { throw; }` re-threw C++ exceptions from within async `Work_After*` callbacks where there was no C++ catch handler on the stack, causing `std::terminate()` → `abort()`.
 
-Calling methods on a `Statement` after its parent `Database` has been closed throws uncatchable Napi::Error:
-```
-terminate called after throwing an instance of 'Napi::Error'
-  what(): The expression evaluated to a falsy value: assert(err)
-Aborted (core dumped)
-```
+**Fix**: Switched to `NAPI_DISABLE_CPP_EXCEPTIONS=1` (via `node_addon_api` dependency). With exceptions disabled, `Napi::Error` is never thrown as a C++ exception — it's just a JavaScript value. Also removed the dead `try/catch` and `throw;` from `TRY_CATCH_CALL` macro.
 
-**Affected operations**: `Statement.map()`, `Statement.all()`, `Statement.get()`, `Statement.run()`, `Statement.each()`, `Statement.finalize()`, `Statement.reset()`, `Statement.bind()`
+**Test**: `test/uncatchable-scenarios/stmt-callback-throws.js` — exit code 1 (normal JS uncaught exception) instead of 134 (SIGABRT).
 
-**Workaround**: Always finalize all statements before closing the database. The callback API's `Database.close()` returns `SQLITE_BUSY` error if unfinalized statements exist, but if you force-close and then use a statement, the crash is uncatchable.
+#### 3. Statement operations after `Database.close()` — Already catchable
 
-**Test impact**: Cannot test `Statement.prototype.map` error path by closing the database first. Unit tests with stubs are used instead (see `test/callback-branches.test.js`).
+`Statement::Schedule()` calls `CleanQueue()` synchronously on the main JS thread, where `InstanceMethodCallbackWrapper` catches the `Napi::Error` and converts it to a JS exception. This scenario returns a normal JS error: `SQLITE_MISUSE: Statement is already finalized`.
 
-#### 3. `verbose.test.js` assertion failure after `verbose()` called
+#### 4. `verbose.test.js` assertion failure after `verbose()` called
 
-When `sqlite3.verbose()` has been called globally (setting `isVerbose = true`), the test "Should not add trace info to error when verbose is not called" in `verbose.test.js` will fail because the error stack DOES contain trace info. The `resetVerbose()` function restores original methods but does NOT reset the `isVerbose` flag. The assertion `err.stack.indexOf(invalid_sql) === -1` fails, and the Napi::Error from the assertion propagates as an uncatchable C++ exception.
+When `sqlite3.verbose()` has been called globally (setting `isVerbose = true`), the test "Should not add trace info to error when verbose is not called" in `verbose.test.js` will fail because the error stack DOES contain trace info. The `resetVerbose()` function restores original methods but does NOT reset the `isVerbose` flag.
 
 **Workaround**: The `verbose()` idempotency test was placed as the LAST test in `verbose.test.js` to avoid contaminating the "not called" test. Test ordering matters for this file.
 
-### Root cause
+### Root cause (historical)
 
-These are C++ exceptions thrown via `Napi::Error` that propagate through the N-API boundary. JavaScript `try/catch` only catches JavaScript exceptions. When a C++ exception reaches the N-API boundary without being caught by a `Napi::TryCatch` block on the C++ side, it triggers `std::terminate()` which calls `abort()`.
+With `NAPI_CPP_EXCEPTIONS` enabled, `Napi::Error` was thrown as a C++ exception. The `TRY_CATCH_CALL` macro caught it and re-threw with `throw;`, but from within async `Work_After*` callbacks where there was no C++ catch handler on the stack → `std::terminate()` → `abort()`. Additionally, `Backup::GetRetryErrors()` crashed on an uninitialized `Napi::Reference<Array>`.
 
-### Design implications
+### Changes made
 
-1. **Promise wrappers must guard against invalid states** before calling native methods, since native errors cannot be caught and converted to rejected promises
-2. **Test coverage for error paths** in native code must use mocks/stubs rather than trying to trigger actual native errors
-3. **Global state changes** (like `verbose()`) must be carefully managed in test suites to avoid cross-test contamination
+1. **`binding.gyp`**: Changed dependency from `node_addon_api_except` to `node_addon_api` (includes `noexcept.gypi` which defines `NODE_ADDON_API_DISABLE_CPP_EXCEPTIONS` and `-fno-exceptions`)
+2. **`src/macros.h`**: Removed `#include <atomic>`, `extern std::atomic<bool> g_env_shutting_down`, and the `try/catch` + `throw;` from `TRY_CATCH_CALL`
+3. **`src/node_sqlite3.cc`**: Removed `#include <atomic>`, `g_env_shutting_down` variable, `EnvCleanupHook`, and `napi_add_env_cleanup_hook` call
+4. **`src/backup.cc`**: Initialize `retryErrors` in `Backup::Backup()` constructor with `[SQLITE_BUSY, SQLITE_LOCKED]`
+5. **`test/uncatchable-exceptions.test.js`**: Integration tests that verify both scenarios now produce catchable JS errors
+6. **`test/uncatchable-scenarios/`**: Child process scripts for crash reproduction
 
 ---
 

src/backup.cc

@@ -173,6 +173,14 @@ Backup::Backup(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Backup>(info)
     baton->filenameIsDest = filenameIsDest.Value();
 
     this->db->Schedule(Work_BeginInitialize, baton);
+
+    // Initialize retryErrors with default values so GetRetryErrors() never
+    // crashes on an empty Napi::Reference, even if the JS wrapper that sets
+    // backup.retryErrors is bypassed (i.e. direct new sqlite3.Backup(...)).
+    Napi::Array defaultRetryErrors = Napi::Array::New(env, 2);
+    defaultRetryErrors.Set(static_cast<uint32_t>(0), Napi::Number::New(env, SQLITE_BUSY));
+    defaultRetryErrors.Set(static_cast<uint32_t>(1), Napi::Number::New(env, SQLITE_LOCKED));
+    retryErrors.Reset(defaultRetryErrors, 1);
 }
 
 void Backup::Work_BeginInitialize(Database::Baton* baton) {

src/macros.h

@@ -4,14 +4,10 @@
 const char* sqlite_code_string(int code);
 const char* sqlite_authorizer_string(int type);
 #include <vector>
-#include <atomic>
 
 // TODO: better way to work around StringConcat?
 #include <napi.h>
 
-// Forward declaration of shutdown flag from node_sqlite3.cc
-extern std::atomic<bool> g_env_shutting_down;
-
 inline Napi::String StringConcat(Napi::Value str1, Napi::Value str2) {
   return Napi::String::New(str1.Env(), str1.As<Napi::String>().Utf8Value() +
                     str2.As<Napi::String>().Utf8Value() );
@@ -133,21 +129,8 @@ inline bool OtherIsInt(Napi::Number source) {
     if ((argc != 0) && (passed_argv != NULL)) {\
       args.assign(passed_argv, passed_argv + argc);\
     }\
-    try {\
-        Napi::Value res = (callback).Call(Napi::Value(context), args);\
-        if (res.IsEmpty()) return __VA_ARGS__;\
-    } catch (const Napi::Error&) {\
-        /* During Node.js/Electron shutdown, when using NAPI_CPP_EXCEPTIONS,*/\
-				/* we must take care to not throw any exceptions. */\
-        /* But when napi_call_function returns napi_cannot_run_js */\
-        /* this throws a C++ exception, when NAPI_CPP_EXCEPTIONS is enabled. */\
-        if (g_env_shutting_down.load()) {\
-             /* We need to silently ignore exceptions during shutdown. */\
-            return __VA_ARGS__;\
-        }\
-        /* Real rror - re-throw to propagate it */\
-        throw;\
-    }
+    Napi::Value res = (callback).Call(Napi::Value(context), args);\
+    if (res.IsEmpty()) return __VA_ARGS__;
 
 #define WORK_DEFINITION(name)                                                  \
     Napi::Value name(const Napi::CallbackInfo& info);                          \

src/node_sqlite3.cc

@@ -2,7 +2,6 @@
 #include <sstream>
 #include <cstring>
 #include <string>
-#include <atomic>
 #include <sqlite3.h>
 
 #include "macros.h"
@@ -12,21 +11,11 @@
 
 using namespace node_sqlite3;
 
-// Global flag set when the environment is shutting down.
-std::atomic<bool> g_env_shutting_down{false};
-
-static void EnvCleanupHook(void* /*data*/) {
-    g_env_shutting_down.store(true);
-}
-
 namespace {
 
 Napi::Object RegisterModule(Napi::Env env, Napi::Object exports) {
     Napi::HandleScope scope(env);
 
-    // Register cleanup hook to detect shutdown
-    napi_add_env_cleanup_hook(env, EnvCleanupHook, nullptr);
-
     Database::Init(env, exports);
     Statement::Init(env, exports);
     Backup::Init(env, exports);

test/uncatchable-exceptions.test.js

@@ -0,0 +1,63 @@
+/**
+ * Integration tests for uncatchable native addon exceptions.
+ *
+ * These scenarios previously caused process aborts (SIGABRT) due to Napi::Error
+ * C++ exceptions escaping from async Work_After* callbacks. With
+ * NAPI_DISABLE_CPP_EXCEPTIONS=1, these are now catchable JS errors and the
+ * child processes exit normally.
+ *
+ * Each test runs the dangerous operation in a child process to isolate any
+ * potential crash from the test runner.
+ */
+
+'use strict';
+
+const assert = require('assert');
+const { execFile } = require('child_process');
+const path = require('path');
+
+const CHILD_TIMEOUT = 30000;
+
+function runChild(scriptPath) {
+    return new Promise((resolve) => {
+        execFile(
+            process.execPath,
+            [scriptPath],
+            { timeout: CHILD_TIMEOUT, maxBuffer: 1024 * 1024 },
+            (error, stdout, stderr) => {
+                resolve({
+                    exitCode: error ? (error.code || 1) : 0,
+                    signal: error ? (error.killed ? 'SIGKILL' : (error.signal || null)) : null,
+                    stdout: stdout || '',
+                    stderr: stderr || ''
+                });
+            }
+        );
+    });
+}
+
+describe('Uncatchable native addon exceptions', function() {
+    this.timeout(60000);
+
+    it('Backup.step() after Backup.finish() returns a catchable JS error', async function() {
+        const script = path.join(__dirname, 'uncatchable-scenarios', 'backup-step-after-finish.js');
+        const result = await runChild(script);
+
+        // Process should exit normally (exit code 0), not crash with SIGABRT
+        assert.strictEqual(result.signal, null, 'Process should not be killed by signal');
+        assert.strictEqual(result.exitCode, 0, 'Process should exit with code 0');
+        assert.ok(result.stdout.includes('OK: got expected error'),
+            `Expected success message in stdout, got: ${result.stdout}`);
+    });
+
+    it('JS callback throw inside Work_After* async callback is a normal JS exception', async function() {
+        const script = path.join(__dirname, 'uncatchable-scenarios', 'stmt-callback-throws.js');
+        const result = await runChild(script);
+
+        // Process should exit with code 1 (uncaught JS exception), not SIGABRT (code 134)
+        assert.strictEqual(result.signal, null, 'Process should not be killed by signal');
+        assert.strictEqual(result.exitCode, 1, 'Process should exit with code 1 (uncaught exception)');
+        assert.ok(result.stderr.includes('intentional throw from async callback'),
+            `Expected error message in stderr, got: ${result.stderr}`);
+    });
+});

test/uncatchable-scenarios/backup-step-after-finish.js

@@ -0,0 +1,53 @@
+/**
+ * Crash scenario: Backup.step() after Backup.finish()
+ * With NAPI_CPP_EXCEPTIONS this causes std::terminate() → abort()
+ */
+'use strict';
+
+const sqlite3 = require('../..');
+const path = require('path');
+const fs = require('fs');
+
+// Ensure tmp dir exists
+const tmpDir = path.join(__dirname, '..', 'tmp');
+if (!fs.existsSync(tmpDir)) fs.mkdirSync(tmpDir, { recursive: true });
+
+const db = new sqlite3.Database(':memory:', function(err) {
+    if (err) { console.error('open error:', err.message); process.exit(1); }
+
+    db.run('CREATE TABLE test (id INTEGER PRIMARY KEY, value TEXT)', function(err) {
+        if (err) { console.error('create error:', err.message); process.exit(1); }
+
+        db.run("INSERT INTO test (value) VALUES ('hello')", function(err) {
+            if (err) { console.error('insert error:', err.message); process.exit(1); }
+
+            const backup = new sqlite3.Backup(db, path.join(tmpDir, 'crash_backup.db'), 'main', 'main', true, function(err) {
+                if (err) { console.error('backup init error:', err.message); process.exit(1); }
+
+                backup.step(-1, function(err) {
+                    if (err) { console.error('step error:', err.message); process.exit(1); }
+
+                    backup.finish(function() {
+                        // Use setTimeout to call step outside the async callback context
+                        setTimeout(function() {
+                            try {
+                                backup.step(-1, function(err) {
+                                    if (err) {
+                                        console.log('OK: got expected error:', err.message);
+                                        process.exit(0);
+                                    } else {
+                                        console.log('ERROR: should have errored');
+                                        process.exit(1);
+                                    }
+                                });
+                            } catch (e) {
+                                console.log('OK: caught synchronous error:', e.message);
+                                process.exit(0);
+                            }
+                        }, 100);
+                    });
+                });
+            });
+        });
+    });
+});