Test on Windows and macOS, await closing databases

.github/workflows/test.yaml

@@ -12,6 +12,7 @@ jobs:
     if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository)  
     outputs:
       dart-version: ${{ steps.dart.outputs.dart-version }}
+      pubspec-hash: ${{ steps.pubspec-hash.outputs.hash }}
     steps:
       - uses: actions/checkout@v6
       - uses: dart-lang/setup-dart@v1
@@ -20,6 +21,11 @@ jobs:
         # Make pub cache folder consistent across OSes, so that we can share the cache.
         run: "echo PUB_CACHE=.dart_tool/pub-cache/ >> $GITHUB_ENV"
         shell: bash
+      - name: "Hash pubspec.yaml"
+        id: pubspec-hash
+        run: |
+          echo "hash=${{ hashFiles('pubspec.yaml') }}" >> $GITHUB_OUTPUT
+        shell: bash
       # We need to update the cache whenever the pubspec.lock changes, as that
       # indicates a changed dependency after `pub upgrade`. However, we can't
       # include the pubspec.lock in the cache key as it's not part of the repository.
@@ -30,7 +36,7 @@ jobs:
           path: |
             ${{ env.PUB_CACHE }}
             pubspec.lock
-          key: dart-deps-${{ steps.dart.outputs.dart-version }}-${{ hashFiles('pubspec.yaml') }}
+          key: dart-deps-${{ steps.dart.outputs.dart-version }}-${{ steps.pubspec-hash.outputs.hash }}
           restore-keys:
             dart-deps-${{ steps.dart.outputs.dart-version }}
             dart-deps-
@@ -59,15 +65,19 @@ jobs:
           path: |
             ${{ env.PUB_CACHE }}
             pubspec.lock
-          key: dart-deps-${{ steps.dart.outputs.dart-version }}-${{ hashFiles('pubspec.yaml') }}
+          key: dart-deps-${{ steps.dart.outputs.dart-version }}-${{ steps.pubspec-hash.outputs.hash }}
           enableCrossOsArchive: true
 
   test:
     needs: [analyze]
-    runs-on: ubuntu-latest
-    if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository)  
+    runs-on: ${{ matrix.os }}
+    if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository)
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [ubuntu-latest, windows-latest, macos-latest]
     steps:
-      - uses: actions/checkout@v6
+      - uses: actions/checkout@v7
       - uses: dart-lang/setup-dart@v1
         with:
           sdk: ${{ needs.analyze.outputs.dart-version }}
@@ -79,7 +89,7 @@ jobs:
           path: |
             ${{ env.PUB_CACHE }}
             pubspec.lock
-          key: dart-deps-${{ needs.analyze.outputs.dart-version }}-${{ hashFiles('pubspec.yaml') }}
+          key: dart-deps-${{ needs.analyze.outputs.dart-version }}-${{ needs.analyze.outputs.pubspec-hash }}
           # Should be created by analyze run
           fail-on-cache-miss: true
           enableCrossOsArchive: true
@@ -90,21 +100,27 @@ jobs:
           dart pub global activate pana
 
       - name: Setup
+        run: dart run tool/sqlite3_wasm_download.dart
+
+      - name: Test sqlite_async on Dart VM
+        working-directory: packages/sqlite_async
         run: |
-          dart run tool/sqlite3_wasm_download.dart
-          dart compile js -O4 --no-minify -Dsqlite3.dartbigints=false -o assets/db_worker.js packages/sqlite_async/lib/src/web/worker/worker.dart
+          dart test -p vm
 
-      - name: Test sqlite_async
+      - name: Test sqlite_async on Chrome
+        if: runner.os == 'Linux'
         working-directory: packages/sqlite_async
         run: |
-          dart test -p chrome,vm --compiler dart2js,dart2wasm
+          dart compile js -O4 --no-minify -Dsqlite3.dartbigints=false -o ../../assets/db_worker.js lib/src/web/worker/worker.dart
+          dart test -p chrome --compiler dart2js,dart2wasm
           dart run build_runner test -- -p chrome
 
       - name: Test drift_sqlite_async
         working-directory: packages/drift_sqlite_async
         run: dart test
 
       - name: Pana for sqlite_async
+        if: runner.os == 'Linux'
         run: dart pub global run pana --no-warning packages/sqlite_async
         # We don't run pana on drift_sqlite_async because it can depend on unpublished changes from
         # sqlite_async, causing it to fail all the time.

packages/drift_sqlite_async/test/utils/test_utils.dart

@@ -1,12 +1,14 @@
 import 'dart:io';
 
 import 'package:sqlite_async/sqlite_async.dart';
+import 'package:test_api/scaffolding.dart';
 import 'package:test_api/src/backend/invoker.dart';
 
 Future<SqliteDatabase> setupDatabase({String? path}) async {
   final db =
       SqliteDatabase.withFactory(SqliteOpenFactory(path: path ?? dbPath()));
   await db.initialize();
+  addTearDown(db.close);
   return db;
 }
 

packages/sqlite_async/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.14.5
+
+- Make `close()` wait for the database to actually be closed.
+
 ## 0.14.4
 
 - Native: Add the `NativeSqliteOpenFactory.beforeOpen` method, which can be overridden to configure

packages/sqlite_async/lib/src/native/database/native_sqlite_database.dart

@@ -40,7 +40,7 @@ final class NativeSqliteDatabaseImpl extends SqliteDatabaseImpl {
 
     return pool;
   });
-  bool _isClosed = false;
+  Future<void>? _closing;
   final _lockGuard = Object();
 
   @override
@@ -66,7 +66,7 @@ final class NativeSqliteDatabaseImpl extends SqliteDatabaseImpl {
 
   @override
   bool get closed {
-    return _isClosed;
+    return _closing != null;
   }
 
   /// Returns true if the _write_ connection is in auto-commit mode
@@ -84,14 +84,20 @@ final class NativeSqliteDatabaseImpl extends SqliteDatabaseImpl {
   }
 
   @override
-  Future<void> close() async {
-    _isClosed = true;
-    final pool = await _pool;
-    pool.close();
+  Future<void> close() {
+    _checkNotLocked('close');
 
-    while (_workers.isNotEmpty) {
-      _workers.removeFirst().close();
-    }
+    return _closing ??= Future.sync(() async {
+      final pool = await _pool;
+
+      // Acquire all connections to ensure this doesn't race with any leased
+      // connection.
+      final allConnections = await pool.exclusiveAccess();
+      pool.close(); // Prevent subsequent pool requests.
+
+      await _workers.map((e) => e.close()).wait;
+      allConnections.close();
+    });
   }
 
   @override
@@ -224,11 +230,7 @@ final class NativeSqliteDatabaseImpl extends SqliteDatabaseImpl {
   }
 
   void _returnIsolateWorker(IsolateWorker worker) {
-    if (_isClosed) {
-      worker.close();
-    } else {
-      _workers.addLast(worker);
-    }
+    _workers.addLast(worker);
   }
 
   void _checkNotLocked(String? debugContext) {

packages/sqlite_async/lib/src/native/database/worker.dart

@@ -11,14 +11,30 @@ import 'package:async/async.dart';
 final class IsolateWorker {
   final Isolate _isolate;
 
-  final ReceivePort _receiveResponses = ReceivePort('isolate worker');
+  final ReceivePort _receiveResponses;
   final SendPort _sendCommands;
 
   final Map<int, Completer<Object?>> _outstandingWorkItems = {};
   int _nextWorkItem = 0;
+  var _closeRequested = false;
+  late final Future<void> _closeAcknowledged;
+
+  IsolateWorker._(this._isolate, this._sendCommands, this._receiveResponses) {
+    _closeAcknowledged = _receiveResponses.listen((Object? message) {
+      if (message == null) {
+        // Null message is the exit listener registered on the isolate.
+
+        for (final pending in _outstandingWorkItems.values) {
+          // This really shouldn't happen, but it's better than not having a future
+          // that doesn't complete.
+          pending.completeError(StateError('Worker closed'));
+        }
+        _outstandingWorkItems.clear();
+
+        _receiveResponses.close();
+        return;
+      }
 
-  IsolateWorker._(this._isolate, this._sendCommands) {
-    _receiveResponses.listen((Object? message) {
       final WorkResult(:id, :result) = message as WorkResult;
       if (_outstandingWorkItems.remove(id) case final completer?) {
         switch (result) {
@@ -28,34 +44,38 @@ final class IsolateWorker {
             completer.completeError(error, stackTrace);
         }
       }
-    });
+    }).asFuture();
   }
 
   Future<T> run<T>(FutureOr<T> Function() task) async {
+    if (_closeRequested) throw StateError('Isolate worker closed');
+
     final id = _nextWorkItem++;
     final completer = _outstandingWorkItems[id] = Completer();
 
     _sendCommands.send(WorkItem(id, _receiveResponses.sendPort, task));
     return (await completer.future) as T;
   }
 
-  void close() {
+  Future<void> close() {
+    _closeRequested = true;
     _isolate.kill();
-    for (final pending in _outstandingWorkItems.values) {
-      // This really shouldn't happen, but it's better than not having a future
-      // that doesn't complete.
-      pending.completeError(StateError('Worker closed'));
-    }
-    _outstandingWorkItems.clear();
-    _receiveResponses.close();
+
+    return _closeAcknowledged;
   }
 
   static Future<IsolateWorker> spawn() async {
     final receiveSendPort = ReceivePort();
-    final isolate = await Isolate.spawn(_entrypoint, receiveSendPort.sendPort);
+    final receiveResponses = ReceivePort('isolate worker');
+    final isolate = await Isolate.spawn(
+      _entrypoint,
+      receiveSendPort.sendPort,
+      onExit: receiveResponses.sendPort,
+    );
+
     final port = (await receiveSendPort.first) as SendPort;
 
-    return IsolateWorker._(isolate, port);
+    return IsolateWorker._(isolate, port, receiveResponses);
   }
 
   static void _entrypoint(SendPort sendPort) async {

packages/sqlite_async/pubspec.yaml

@@ -1,6 +1,6 @@
 name: sqlite_async
 description: High-performance asynchronous interface for SQLite on Dart and Flutter.
-version: 0.14.4
+version: 0.14.5
 resolution: workspace
 repository: https://github.com/powersync-ja/sqlite_async.dart
 environment:

packages/sqlite_async/test/basic_test.dart

@@ -303,7 +303,7 @@ void main() {
       final hasLock = Completer<void>();
 
       final db = await testUtils.setupDatabase(path: path);
-      db.withAllConnections((writer, readers) async {
+      final allConnections = db.withAllConnections((writer, readers) async {
         hasLock.complete();
         await releaseLock.future;
       });
@@ -318,6 +318,9 @@ void main() {
         db.abortableWriteLock((_) async {}, abortTrigger: Future.value(null)),
         throwsAbortException,
       );
+
+      releaseLock.complete();
+      await allConnections;
     });
 
     test('execute single statement with RETURNING populates ResultSet',
@@ -445,6 +448,7 @@ void main() {
         await testUtils.testFactory(
             path: path, options: SqliteOptions(maxReaders: maxReaders)),
       );
+      addTearDown(db.close);
       await db.initialize();
       await createTables(db);
 
@@ -512,6 +516,17 @@ void main() {
         expect(row.values.map((e) => e.runtimeType), [int, double]);
       });
     }, skip: identical(0, 0.0) ? 'Requires 64-bit ints' : false);
+
+    test('cannot use closed databases', () async {
+      final db = await testUtils.setupDatabase(path: path);
+
+      await db.initialize();
+      await db.close();
+      expect(db.closed, isTrue);
+
+      await expectLater(db.execute('SELECT 1'), throwsA(anything));
+      await expectLater(db.get('SELECT 1'), throwsA(anything));
+    });
   });
 }
 

packages/sqlite_async/test/native/basic_test.dart

@@ -58,6 +58,7 @@ void main() {
         await testUtils.testFactory(
             path: path, options: SqliteOptions(maxReaders: 3)),
       );
+      addTearDown(db.close);
       await db.initialize();
       await createTables(db);
 
@@ -90,6 +91,7 @@ void main() {
         await testUtils.testFactory(
             path: path, options: SqliteOptions(maxReaders: 3)),
       );
+      addTearDown(db.close);
       await db.initialize();
       await createTables(db);
 
@@ -366,6 +368,7 @@ void main() {
     test('invokes beforeOpen callback on factories', () async {
       final factoy = _BeforeSetupHook(path: path);
       final db = SqliteDatabase.withFactory(factoy);
+      addTearDown(db.close);
       expect(factoy.didCallBeforeOpen, isFalse);
       await db.initialize();
 

packages/sqlite_async/test/native/watch_test.dart

@@ -41,6 +41,7 @@ void main() {
           (await testUtils.testFactory(path: path)) as NativeSqliteOpenFactory;
       final db = factory.openNativeConnection(
           SqliteOpenOptions(primaryConnection: true, readOnly: false));
+      addTearDown(db.close);
 
       db.execute('CREATE TABLE a (bar INTEGER);');
       db.execute('CREATE TABLE b (bar INTEGER);');
@@ -148,8 +149,8 @@ void main() {
       final reads = StreamQueue(db.updates);
       final first = reads.next;
 
-      db.writeLock((ctx) async {
-        await ctx.execute('INSERT INTO customer(name) VALUES (?)', ['test']);
+      await db.writeLock((ctx) async {
+        await ctx.execute('INSERT INTO customers(name) VALUES (?)', ['test']);
         // Because we're not in a transaction, this should emit an update. We
         // shouldn't just collect updates at the end of writeLock to avoid
         // long-running writers never emitting updates.

packages/sqlite_async/test/utils/abstract_test_utils.dart

@@ -1,5 +1,6 @@
 import 'package:sqlite3/common.dart';
 import 'package:sqlite_async/sqlite_async.dart';
+import 'package:test/scaffolding.dart';
 
 abstract class AbstractTestUtils {
   String dbPath();
@@ -17,7 +18,10 @@ abstract class AbstractTestUtils {
     SqliteOptions options = defaultTestOptions,
   }) async {
     final factory = await testFactory(path: path, options: options);
-    return SqliteDatabase.withFactory(factory);
+    final db = SqliteDatabase.withFactory(factory);
+
+    addTearDown(db.close);
+    return db;
   }
 
   /// Deletes any DB data