Skip to content

Make JSI SQLite setup and reset failures reject instead of crashing - #1983

Open
joker-777 wants to merge 1 commit into
Nozbe:masterfrom
kenhub:fix/jsi-setup-errors
Open

joker-777 wants to merge 1 commit into
Nozbe:masterfrom
kenhub:fix/jsi-setup-errors

Conversation

@joker-777

Copy link
Copy Markdown

Fixes the JSI (C++) path of SQLiteAdapter (jsi: true), so setup and reset failures reject and reach onSetUpError instead of killing the process. Related issue: #1945. This is the JSI counterpart of #1981, which does the same for the legacy (non-JSI) bridges. The two PRs are independent and can be merged in either order.

Changes

  • Schema setup, migration and unsafeResetDatabase() failures reject the promise and reach onSetUpError, instead of calling std::abort()
  • After a failed setup or reset, the connection stays open, so the app can recover with database.unsafeResetDatabase(). Until a reset succeeds, other calls on it reject instead of failing an assertion
  • A failed database.unsafeResetDatabase() restores the adapter and clears the record caches, so the reset can be retried through the public API. This applies to every adapter
  • Every adapter clears its cached record IDs at the start of a reset, so a reset that fails early does not leave the adapter and JS record caches out of sync
  • A database file that cannot be opened is reported through the rejected setup promise. Before, a thrown pointer that no handler catches terminated the process
  • Calling a native method with the wrong number of arguments throws a JS error on Android too
  • Raw SQL the bridge cannot run is rejected instead of failing a native assertion. That covers empty or comment-only SQL, and record queries that don't select id first
  • The prepared statement cache is bounded to 256 statements with least-recently-used eviction. Statements in use by a caller are pinned and never evicted mid-use

Left as is: the std::abort() in configureJNI (DatabasePlatformAndroid.cpp) when GetJavaVM fails. It runs while the JSI binding is installed, before any adapter or promise exists, and a failing GetJavaVM means the JNI environment itself is broken.

Tests

  • The shared adapter test errors when migration fails now checks, on JSI, that a database whose migration failed rejects later calls and can then be reset and used again
  • New shared test can retry a failed reset (SQLite) breaks the schema so the reset fails, then retries it on the same adapter
  • New shared test rejects raw SQL queries it cannot run (JSI)
  • New unit tests for the JSI dispatcher (makeDispatcher/test.js) and for Database.unsafeResetDatabase() failing

Verification

  • On this branch: flow check is clean, and ESLint is clean on the changed files. Jest passes: 80 suites / 641 tests on the host. The sqlite-node suite (src/adapters/sqlite/test.js) was run separately in a Node 22 container, because better-sqlite3 doesn't build on the host's Node; all 123 tests pass there
  • The same change was built and tested on CI in our fork (on a branch that also carries React Native 0.88 build fixes). There, the Android job built the C++ and ran the shared adapter suite on an emulator in async and JSI mode. The iOS job built it and ran the native integration tests on an iPhone 16 Pro simulator with iOS 18.5. Both passed
  • The native code was not built against this repository's master directly. The C++ files applied without conflicts

🤖 Generated with Claude Code

- Schema setup, migration and unsafeResetDatabase() failures on the JSI path reject and reach onSetUpError instead of aborting the app
- An unopenable database file, a wrong argument count, and raw SQL the bridge cannot run reject instead of crashing
- A failed reset leaves the database open and can be retried, including through database.unsafeResetDatabase()
- Adapters clear their cached record IDs before a reset can fail, so JS and adapter caches stay consistent
- The JSI prepared statement cache is bounded to 256 statements with LRU eviction

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@StasDoskalenko

Copy link
Copy Markdown

Hi @joker-777, thank you for this! Crashing at launch instead of reaching onSetUpError is a real pain, and it's great to see it handled properly.

I maintain NitromelonDB, a maintained fork of WatermelonDB (New Architecture, Nitro Modules). This one applies there too: our Nitro module still calls std::abort() when setUpWithSchema or unsafeResetDatabase fails (native/nitro/HybridNitromelonDatabase.cpp), and we have an open issue for exactly this: StasDoskalenko#61. The shared C++ under native/shared/ is still close to upstream, so most of the change should carry over.

Our fork was rewritten in TypeScript (strict mode) to reduce the chance of bugs like this one slipping through, so I think the community would really benefit if you opened your PRs against it too. A couple of notes in case you do:

  • native calls go through a Nitro HybridObject instead of DatabaseBridge.cpp
  • we already size the prepared statement cache to the schema and clear it on memory warnings, so the LRU part may need a look against that

CI runs the shared adapter suite on real iOS and Android devices, so it would get tested on both platforms.

If you don't have the time or would rather not, no worries at all, I'm happy to port it myself. Just let me know. Thanks a lot!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants