fix: make Connection::Close() idempotent to handle failed connections (#379)

When sql.query() was called with an invalid database, the connection
would fail to open but the cleanup code would still try to close it.
This threw "Connection is not open" as an uncaught exception instead
of passing the error to the callback.

Fix: Instead of throwing when Close() is called on a connection that
was never opened, just call the callback with success (idempotent close).

Fixes #379

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

Author: TimelordUK

cpp/src/js/Connection.cpp

@@ -215,9 +215,14 @@ Napi::Value Connection::Close(const Napi::CallbackInfo& info) {
   const Napi::Env env = info.Env();
   Napi::HandleScope scope(env);
 
-  // Check if we have a connection to close
+  // If connection was never opened or already closed, just call callback with success
+  // This makes Close() idempotent and prevents crashes when closing failed connections
   if (!isConnected_) {
-    Napi::Error::New(env, "Connection is not open").ThrowAsJavaScriptException();
+    SQL_LOG_DEBUG("Connection::Close - connection not open, calling callback with success");
+    if (info.Length() > 0 && info[info.Length() - 1].IsFunction()) {
+      Napi::Function callback = info[info.Length() - 1].As<Napi::Function>();
+      callback.Call({env.Null(), Napi::Boolean::New(env, true)});
+    }
     return env.Undefined();
   }
 

package-lock.json

@@ -9,7 +9,7 @@
       "name": "Stephen James"
     }
   ],
-  "version": "5.1.2",
+  "version": "5.1.3",
   "homepage": "https://github.com/TimelordUK/node-sqlserver-v8",
   "bugs": {
     "url": "https://github.com/TimelordUK/node-sqlserver-v8/issues"

package.json

@@ -0,0 +1,132 @@
+'use strict'
+
+/**
+ * Test for Issue #379: sql.query() throws uncaught exception instead of
+ * passing error to callback when database doesn't exist.
+ */
+
+const sql = require('./lib/sql')
+
+// Get base connection string and modify to use non-existent database
+function getConnectionString() {
+  try {
+    const fs = require('fs')
+    const path = require('path')
+    const rcPath = path.join(__dirname, '.env-cmdrc')
+    if (fs.existsSync(rcPath)) {
+      const config = JSON.parse(fs.readFileSync(rcPath, 'utf8'))
+      if (config.test) {
+        const testConfig = config.test
+        if (testConfig.CONNECTION_KEY && testConfig[testConfig.CONNECTION_KEY]) {
+          return testConfig[testConfig.CONNECTION_KEY]
+        }
+      }
+    }
+  } catch (e) {
+    // ignore
+  }
+  return 'Driver={ODBC Driver 18 for SQL Server};Server=127.0.0.1,1433;Database=node;UID=sa;PWD=Password_123#;TrustServerCertificate=yes;'
+}
+
+// Modify connection string to use non-existent database
+const baseConnectionString = getConnectionString()
+const badConnectionString = baseConnectionString.replace(/Database=[^;]+/, 'Database=NonExistentDB_12345')
+
+console.log('='.repeat(70))
+console.log('Test for Issue #379: sql.query() error handling')
+console.log('='.repeat(70))
+console.log('')
+console.log('Base connection:', baseConnectionString.substring(0, 60) + '...')
+console.log('Bad connection:', badConnectionString.substring(0, 60) + '...')
+console.log('')
+
+// Track if callback was called
+let callbackCalled = false
+
+// Set up uncaught exception handler
+process.on('uncaughtException', (err) => {
+  console.error('')
+  console.error('!!! UNCAUGHT EXCEPTION !!!')
+  console.error('Error:', err.message)
+  console.error('Stack:', err.stack)
+  console.error('')
+  console.error('This is the bug - error should have gone to callback, not thrown!')
+  process.exit(1)
+})
+
+process.on('unhandledRejection', (reason, promise) => {
+  console.error('')
+  console.error('!!! UNHANDLED REJECTION !!!')
+  console.error('Reason:', reason)
+  console.error('')
+  console.error('This is a bug - rejection should have been handled!')
+  process.exit(1)
+})
+
+console.log('Test 1: sql.query() with non-existent database')
+console.log('-'.repeat(70))
+
+sql.query(badConnectionString, 'SELECT 1 as test', (err, results) => {
+  callbackCalled = true
+
+  if (err) {
+    console.log('SUCCESS: Error was passed to callback (expected behavior)')
+    console.log('Error message:', err.message)
+    console.log('')
+    runTest2()
+  } else {
+    console.log('UNEXPECTED: Query succeeded?', results)
+    runTest2()
+  }
+})
+
+// Give it a few seconds, then check if callback was called
+setTimeout(() => {
+  if (!callbackCalled) {
+    console.error('')
+    console.error('TIMEOUT: Callback was never called!')
+    console.error('This might indicate the error was swallowed or thrown elsewhere.')
+    process.exit(1)
+  }
+}, 10000)
+
+function runTest2() {
+  console.log('Test 2: sql.open() with non-existent database')
+  console.log('-'.repeat(70))
+
+  sql.open(badConnectionString, (err, conn) => {
+    if (err) {
+      console.log('SUCCESS: Error was passed to callback (expected behavior)')
+      console.log('Error message:', err.message)
+      console.log('')
+      runTest3()
+    } else {
+      console.log('UNEXPECTED: Connection succeeded?')
+      conn.close(() => runTest3())
+    }
+  })
+}
+
+function runTest3() {
+  console.log('Test 3: sql.promises.query() with non-existent database')
+  console.log('-'.repeat(70))
+
+  sql.promises.query(badConnectionString, 'SELECT 1 as test')
+    .then(results => {
+      console.log('UNEXPECTED: Query succeeded?', results)
+      finishTests()
+    })
+    .catch(err => {
+      console.log('SUCCESS: Promise rejected (expected behavior)')
+      console.log('Error message:', err.message)
+      console.log('')
+      finishTests()
+    })
+}
+
+function finishTests() {
+  console.log('='.repeat(70))
+  console.log('All tests completed without uncaught exceptions!')
+  console.log('='.repeat(70))
+  process.exit(0)
+}