Make JSI SQLite setup and reset failures reject instead of crashing

CHANGELOG-Unreleased.md

@@ -26,6 +26,13 @@
 - [Android] Generate `BuildConfig` explicitly (AGP 9 no longer does for libraries) and stop applying the unused Kotlin plugin, which collides with AGP 9's built-in Kotlin support
 - [SQLite] [iOS] Database setup failures on the legacy (non-JSI) bridge (database cannot be opened, schema setup or migration fails) now reject the promise and reach `onSetUpError` instead of raising an `NSException` that crashes the app
 - [SQLite] [iOS] [Android] After a failed schema setup or migration on the legacy (non-JSI) bridge, operations queued during setup are rejected with the setup error and the driver stays connected, so the app can recover with `database.unsafeResetDatabase()` instead of every call hanging (iOS) or being rejected with "No driver with tag N available" (Android). The Node.js SQLite bridge (`sqlite-node`) behaves the same way
+- [SQLite] [JSI] Database setup failures on the JSI path (database file cannot be opened, schema setup or migration fails) now reject the setup promise and reach `onSetUpError` instead of crashing the app
+- [SQLite] [JSI] A failed `unsafeResetDatabase()` now rejects instead of crashing the app, and leaves the (empty, consistent) database open so the reset can be retried. Other calls on a database whose setup or reset failed are rejected instead of failing an assertion
+- `database.unsafeResetDatabase()` that fails now restores the adapter (and clears record caches, since data may already be erased), so the reset can be retried instead of every later call failing with "Cannot call database.adapter.X while the database is being reset"
+- Adapters now clear their cached record IDs at the start of `unsafeResetDatabase()`, so a reset that fails early no longer leaves later fetches failing with "Record ID ... was sent over the bridge, but it's not cached"
+- [SQLite] [JSI] Calling a native method with the wrong number of arguments now throws a JS error on Android too, instead of crashing the app
+- [SQLite] [JSI] Empty or comment-only raw SQL queries, and record queries (`Q.unsafeSqlQuery`) whose first column is not `id`, now reject instead of failing a native assertion
+- [SQLite] [JSI] The prepared statement cache is now bounded (256 statements, least recently used are finalized first), so one-off queries no longer keep a prepared statement alive for the lifetime of the database
 
 ### Performance
 

native/android/src/main/java/com/nozbe/watermelondb/WMDatabaseDriver.java

@@ -222,8 +222,10 @@ public void unsafeResetDatabase(Schema schema) {
         if (log != null) {
             log.info("Unsafe reset database");
         }
-        database.unsafeDestroyEverything();
+        // NOTE: Clear cached record IDs before anything that can fail: JS clears its record cache even if the
+        // reset fails, and IDs left cached here would make later fetches return records JS no longer has
         cachedRecords.clear();
+        database.unsafeDestroyEverything();
         database.transaction(() -> {
             database.unsafeExecuteStatements(schema.sql);
             database.setUserVersion(schema.version);

native/ios/WatermelonDB/objc/WMDatabaseDriver.m

@@ -234,10 +234,12 @@ - (NSString *) getLocal:(NSString *)key error:(NSError **)errorPtr
 
 - (BOOL) unsafeResetDatabaseWithSchema:(NSString *)sql schemaVersion:(long)version error:(NSError **)errorPtr
 {
+    // NOTE: Clear cached record IDs before anything that can fail: JS clears its record cache even if the
+    // reset fails, and IDs left cached here would make later fetches return records JS no longer has
+    _cachedRecords = [NSMutableDictionary dictionary];
     if (![_db unsafeDestroyEverything:errorPtr]) {
         return NO;
     }
-    _cachedRecords = [NSMutableDictionary dictionary];
 
     __block WMDatabase *db = _db;
     BOOL txnResult = [db inTransaction:^BOOL(NSError **innerErrorPtr) {

native/shared/Database-batch.cpp

@@ -87,8 +87,8 @@ void Database::batchJSON(jsi::String &&jsiJson) {
                     sql = (std::string_view) field;
                 } else if (fieldIdx == 3) {
                     ondemand::array argsBatches = field;
-                    auto stmt = prepareQuery(sql);
-                    SqliteStatement statement(stmt);
+                    auto statement = prepareQuery(sql);
+                    sqlite3_stmt *stmt = statement.stmt;
 
                     for (ondemand::array args : argsBatches) {
                         // NOTE: We must capture the ID once first parsed

native/shared/Database-query.cpp

@@ -6,6 +6,15 @@ namespace watermelondb {
 using platform::consoleError;
 using platform::consoleLog;
 
+// Record queries may come from the app (Q.unsafeSqlQuery), so their shape is checked, not asserted:
+// the first column must be the record ID
+void Database::requireIdColumn(sqlite3_stmt *statement) {
+    const char *column = sqlite3_column_count(statement) > 0 ? sqlite3_column_name(statement, 0) : nullptr;
+    if (!column || std::string(column) != "id") {
+        throw jsi::JSError(getRt(), "Invalid query - the first selected column must be the record id");
+    }
+}
+
 jsi::Value Database::find(jsi::String &tableName, jsi::String &id) {
     auto &rt = getRt();
     const std::lock_guard<std::mutex> lock(mutex_);
@@ -40,7 +49,7 @@ jsi::Value Database::query(jsi::String &tableName, jsi::String &sql, jsi::Array
             break;
         }
 
-        assert(std::string(sqlite3_column_name(statement.stmt, 0)) == "id");
+        requireIdColumn(statement.stmt);
 
         const char *id = (const char *)sqlite3_column_text(statement.stmt, 0);
         if (!id) {
@@ -72,7 +81,7 @@ jsi::Value Database::queryAsArray(jsi::String &tableName, jsi::String &sql, jsi:
             break;
         }
 
-        assert(std::string(sqlite3_column_name(statement.stmt, 0)) == "id");
+        requireIdColumn(statement.stmt);
 
         const char *id = (const char *)sqlite3_column_text(statement.stmt, 0);
         if (!id) {
@@ -109,7 +118,7 @@ jsi::Array Database::queryIds(jsi::String &sql, jsi::Array &arguments) {
             break;
         }
 
-        assert(std::string(sqlite3_column_name(statement.stmt, 0)) == "id");
+        requireIdColumn(statement.stmt);
 
         const char *idText = (const char *)sqlite3_column_text(statement.stmt, 0);
         if (!idText) {
@@ -149,7 +158,10 @@ jsi::Value Database::count(jsi::String &sql, jsi::Array &arguments) {
     auto statement = executeQuery(sql.utf8(rt), arguments);
     getRow(statement.stmt);
 
-    assert(sqlite3_data_count(statement.stmt) == 1);
+    // NOTE: The query may come from the app (Q.unsafeSqlQuery), so this is checked, not asserted
+    if (sqlite3_data_count(statement.stmt) < 1) {
+        throw jsi::JSError(rt, "Count query did not return a count");
+    }
     int count = sqlite3_column_int(statement.stmt, 0);
     return jsi::Value(count);
 }

native/shared/Database-sqlite.cpp

@@ -8,26 +8,54 @@ namespace watermelondb {
 using platform::consoleError;
 using platform::consoleLog;
 
-sqlite3_stmt* Database::prepareQuery(std::string sql) {
-    sqlite3_stmt *statement = cachedStatements_[sql];
+SqliteStatement Database::prepareQuery(std::string sql) {
+    auto cached = cachedStatements_.find(sql);
+
+    if (cached != cachedStatements_.end()) {
+        // most recently used entries go to the front, so the least recently used ones are evicted first
+        statementCache_.splice(statementCache_.begin(), statementCache_, cached->second);
+        CachedStatement &entry = *cached->second;
+        assert(entry.stmt != nullptr);
+        // in theory, this shouldn't be necessary, since statements ought to be reset *after* use, not before use
+        // but still this might prevent some crashes if this is not done right
+        // TODO: Remove this later - should not be necessary, and it wastes time
+        sqlite3_reset(entry.stmt);
+        return SqliteStatement(entry.stmt, &entry.pinCount);
+    }
+
+    sqlite3_stmt *statement = nullptr;
+    int resultPrepare = sqlite3_prepare_v2(db_->sqlite, sql.c_str(), -1, &statement, nullptr);
 
+    if (resultPrepare != SQLITE_OK) {
+        sqlite3_finalize(statement);
+        throw dbError("Failed to prepare query statement");
+    }
     if (statement == nullptr) {
-        int resultPrepare = sqlite3_prepare_v2(db_->sqlite, sql.c_str(), -1, &statement, nullptr);
+        // NOTE: sqlite3_prepare_v2 succeeds without a statement if the SQL is empty or only a comment
+        throw jsi::JSError(getRt(), "Failed to prepare query statement - the query is empty");
+    }
 
-        if (resultPrepare != SQLITE_OK) {
-            sqlite3_finalize(statement);
-            throw dbError("Failed to prepare query statement");
-        }
+    evictCachedStatements();
+    statementCache_.push_front({ sql, statement, 0 });
+    cachedStatements_[sql] = statementCache_.begin();
 
-        cachedStatements_[sql] = statement;
-    } else {
-        // in theory, this shouldn't be necessary, since statements ought to be reset *after* use, not before use
-        // but still this might prevent some crashes if this is not done right
-        // TODO: Remove this later - should not be necessary, and it wastes time
-        sqlite3_reset(statement);
+    CachedStatement &entry = statementCache_.front();
+    return SqliteStatement(entry.stmt, &entry.pinCount);
+}
+
+// Finalizes least recently used statements until there is room for one more in the cache.
+// Statements pinned by a live SqliteStatement (i.e. still in use by a caller) are skipped.
+void Database::evictCachedStatements() {
+    auto it = statementCache_.end();
+    while (statementCache_.size() >= maxCachedStatements_ && it != statementCache_.begin()) {
+        --it;
+        if (it->pinCount > 0) {
+            continue;
+        }
+        sqlite3_finalize(it->stmt);
+        cachedStatements_.erase(it->sql);
+        it = statementCache_.erase(it);
     }
-    assert(statement != nullptr);
-    return statement;
 }
 
 void Database::bindArgs(sqlite3_stmt *statement, jsi::Array &arguments) {
@@ -111,8 +139,8 @@ std::string Database::bindArgsAndReturnId(sqlite3_stmt *statement, simdjson::ond
 
 SqliteStatement Database::executeQuery(std::string sql, jsi::Array &arguments) {
     auto statement = prepareQuery(sql);
-    bindArgs(statement, arguments);
-    return SqliteStatement(statement);
+    bindArgs(statement.stmt, arguments);
+    return statement;
 }
 
 void Database::executeUpdate(sqlite3_stmt *statement) {
@@ -124,16 +152,14 @@ void Database::executeUpdate(sqlite3_stmt *statement) {
 }
 
 void Database::executeUpdate(std::string sql, jsi::Array &args) {
-    auto stmt = prepareQuery(sql);
-    bindArgs(stmt, args);
-    SqliteStatement statement(stmt);
-    executeUpdate(stmt);
+    auto statement = prepareQuery(sql);
+    bindArgs(statement.stmt, args);
+    executeUpdate(statement.stmt);
 }
 
 void Database::executeUpdate(std::string sql) {
-    auto stmt = prepareQuery(sql);
-    SqliteStatement statement(stmt);
-    executeUpdate(stmt);
+    auto statement = prepareQuery(sql);
+    executeUpdate(statement.stmt);
 }
 
 void Database::getRow(sqlite3_stmt *stmt) {

native/shared/Database-turboSync.cpp

@@ -116,8 +116,8 @@ jsi::Value Database::unsafeLoadFromSync(int jsonId, jsi::Object &schema, std::st
                         auto tableSchemaArray = tableSchemas.first;
                         auto tableSchema = tableSchemas.second;
 
-                        sqlite3_stmt *stmt = prepareQuery(insertSqlFor(rt, tableName, tableSchemaArray));
-                        SqliteStatement statement(stmt);
+                        auto statement = prepareQuery(insertSqlFor(rt, tableName, tableSchemaArray));
+                        sqlite3_stmt *stmt = statement.stmt;
 
                         for (ondemand::object record : records) {
                             // TODO: It would be much more natural to iterate over schema, and then get json's field

native/shared/Database.cpp

@@ -5,7 +5,7 @@ namespace watermelondb {
 using platform::consoleError;
 using platform::consoleLog;
 
-Database::Database(jsi::Runtime *runtime, std::string path, bool usesExclusiveLocking) : runtime_(runtime), mutex_() {
+Database::Database(jsi::Runtime *runtime, std::string path, bool usesExclusiveLocking) : initialized_(false), isDestroyed_(false), mutex_(), runtime_(runtime) {
     db_ = std::make_unique<SqliteDb>(path);
 
     std::string initSql = "";
@@ -46,18 +46,26 @@ void Database::destroy() {
         return;
     }
     isDestroyed_ = true;
-    for (auto const &cachedStatement : cachedStatements_) {
-        sqlite3_stmt *statement = cachedStatement.second;
-        sqlite3_finalize(statement);
+    for (auto const &cachedStatement : statementCache_) {
+        sqlite3_finalize(cachedStatement.stmt);
     }
     cachedStatements_ = {};
+    statementCache_ = {};
     db_->destroy();
 }
 
 Database::~Database() {
     destroy();
 }
 
+// Throws (i.e. rejects the call in JS) if the database was not successfully set up, or was closed -
+// e.g. if the app keeps using it after a failed setup or reset instead of resetting it first
+void Database::requireInitialized() {
+    if (!initialized_) {
+        throw jsi::JSError(getRt(), "Database is not set up - its setup or reset failed, or it was closed");
+    }
+}
+
 bool Database::isCached(std::string cacheKey) {
     return cachedRecords_.find(cacheKey) != cachedRecords_.end();
 }
@@ -77,20 +85,28 @@ void Database::unsafeResetDatabase(jsi::String &schema, int schemaVersion) {
     // They seem to be enabling "defensive" config. So we use another obscure method to clear the database
     // https://www.sqlite.org/c3ref/c_dbconfig_defensive.html#sqlitedbconfigresetdatabase
 
+    // NOTE: Clear cached record IDs before anything that can fail: JS clears its record cache even if the
+    // reset fails, and IDs left cached here would make later fetches return records JS no longer has
+    cachedRecords_ = {};
+
     if (sqlite3_db_config(db_->sqlite, SQLITE_DBCONFIG_RESET_DATABASE, 1, 0) != SQLITE_OK) {
         throw jsi::JSError(rt, "Failed to enable reset database mode");
     }
     // NOTE: We can't VACUUM in a transaction
-    executeMultiple("vacuum");
+    try {
+        executeMultiple("vacuum");
+    } catch (...) {
+        // NOTE: Reset mode must not stay enabled on a connection that keeps being used
+        sqlite3_db_config(db_->sqlite, SQLITE_DBCONFIG_RESET_DATABASE, 0, 0);
+        throw;
+    }
 
     if (sqlite3_db_config(db_->sqlite, SQLITE_DBCONFIG_RESET_DATABASE, 0, 0) != SQLITE_OK) {
         throw jsi::JSError(rt, "Failed to disable reset database mode");
     }
 
     beginTransaction();
     try {
-        cachedRecords_ = {};
-
         // Reinitialize schema
         executeMultiple(schema.utf8(rt));
         setUserVersion(schemaVersion);

native/shared/Database.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <jsi/jsi.h>
+#include <list>
 #include <unordered_map>
 #include <unordered_set>
 #include <mutex>
@@ -49,13 +50,29 @@ class Database : public jsi::HostObject {
     std::mutex mutex_;
     jsi::Runtime *runtime_; // TODO: std::shared_ptr would be better, but I don't know how to make it from void* in RCTCxxBridge
     std::unique_ptr<SqliteDb> db_;
-    std::unordered_map<std::string, sqlite3_stmt *> cachedStatements_; // NOTE: may contain null pointers!
+
+    // Prepared statements are cached by their SQL string. The cache is bounded: once it holds
+    // maxCachedStatements_ entries, the least recently used statement is finalized to make room,
+    // so one-off queries (e.g. with ids inlined into the SQL) don't accumulate for the lifetime of
+    // the database, while the statements batch/sync/find reuse all the time stay cached.
+    // Entries pinned by a live SqliteStatement are never evicted.
+    struct CachedStatement {
+        std::string sql;
+        sqlite3_stmt *stmt;
+        unsigned pinCount;
+    };
+    static constexpr size_t maxCachedStatements_ = 256;
+    std::list<CachedStatement> statementCache_; // most recently used first
+    std::unordered_map<std::string, std::list<CachedStatement>::iterator> cachedStatements_;
     std::unordered_set<std::string> cachedRecords_;
 
     jsi::Runtime &getRt();
     jsi::JSError dbError(std::string description);
+    void requireInitialized();
+    void requireIdColumn(sqlite3_stmt *statement);
 
-    sqlite3_stmt* prepareQuery(std::string sql);
+    SqliteStatement prepareQuery(std::string sql);
+    void evictCachedStatements();
     void bindArgs(sqlite3_stmt *statement, jsi::Array &arguments);
     std::string bindArgsAndReturnId(sqlite3_stmt *statement, simdjson::ondemand::array &args);
     SqliteStatement executeQuery(std::string sql, jsi::Array &arguments);