fix(storage): keep connection metadata and secrets consistent on OS store failures (#276)

lib/core/storage/local_db.dart

@@ -359,23 +359,51 @@ class LocalDb {
 
   /// Inserts a row and returns the SQLite row id.
   /// Password and connection string are stored in the OS secure store, not in SQLite.
+  ///
+  /// If writing secrets fails, the SQLite row is rolled back and the error is
+  /// rethrown so callers can surface a Keychain / libsecret failure.
   Future<int> addConnection(ConnectionRow row) async {
     final db = await _open();
     final id = await db.insert('connections', row.toPersistenceMap());
-    await ConnectionSecretsStore.writeForConnection(
-      id,
-      password: row.password,
-      connectionString: row.connectionString,
-    );
+    try {
+      await ConnectionSecretsStore.writeForConnection(
+        id,
+        password: row.password,
+        connectionString: row.connectionString,
+      );
+    } catch (e) {
+      try {
+        await ConnectionSecretsStore.deleteForConnection(id);
+      } catch (_) {
+        // Best-effort cleanup of any partial secret writes.
+      }
+      await db.delete('connections', where: 'id = ?', whereArgs: [id]);
+      rethrow;
+    }
     return id;
   }
 
-  /// Atomically updates an existing connection row in SQLite and its secrets in the secure store.
+  /// Updates an existing connection row in SQLite and its secrets in the secure store.
+  ///
+  /// If writing secrets fails, the previous SQLite row and previous secrets are
+  /// restored (best effort) and the error is rethrown.
   Future<void> updateConnection(ConnectionRow row) async {
     if (row.id == null) {
       throw ArgumentError('ConnectionRow.id cannot be null when calling updateConnection');
     }
     final db = await _open();
+    final previousMaps = await db.query(
+      'connections',
+      where: 'id = ?',
+      whereArgs: [row.id],
+    );
+    if (previousMaps.isEmpty) {
+      throw ArgumentError('No connection found with id ${row.id}');
+    }
+    final previousRow = ConnectionRow.fromMap(previousMaps.first);
+    final previousSecrets =
+        await ConnectionSecretsStore.readForConnection(row.id!);
+
     await db.transaction((txn) async {
       final count = await txn.update(
         'connections',
@@ -387,15 +415,41 @@ class LocalDb {
         throw ArgumentError('No connection found with id ${row.id}');
       }
     });
-    await ConnectionSecretsStore.writeForConnection(
-      row.id!,
-      password: row.password,
-      connectionString: row.connectionString,
-    );
+
+    try {
+      await ConnectionSecretsStore.writeForConnection(
+        row.id!,
+        password: row.password,
+        connectionString: row.connectionString,
+      );
+    } catch (e) {
+      await db.update(
+        'connections',
+        previousRow.toPersistenceMap(),
+        where: 'id = ?',
+        whereArgs: [row.id],
+      );
+      try {
+        await ConnectionSecretsStore.writeForConnection(
+          row.id!,
+          password: previousSecrets.password,
+          connectionString: previousSecrets.connectionString,
+        );
+      } catch (_) {
+        // Best-effort restore of previous secrets; surface the original error.
+      }
+      rethrow;
+    }
   }
 
+  /// Deletes a connection. SQLite deletion always proceeds even if the secure
+  /// store delete fails (e.g. missing key or unavailable libsecret daemon).
   Future<void> removeConnection(int id) async {
-    await ConnectionSecretsStore.deleteForConnection(id);
+    try {
+      await ConnectionSecretsStore.deleteForConnection(id);
+    } catch (_) {
+      // Do not block removing the connection metadata when the OS store fails.
+    }
     final db = await _open();
     await db.delete('connections', where: 'id = ?', whereArgs: [id]);
   }

test/core/storage/local_db_secrets_test.dart

@@ -155,5 +155,81 @@ void main() {
       expect(secrets.password, 'new-secret-password');
       expect(secrets.connectionString, 'postgres://root:new-secret-password@db.example.com:5433/mydb');
     });
+
+    test('removeConnection still deletes SQLite row when secure-store delete fails', () async {
+      const row = ConnectionRow(
+        type: 'redis',
+        name: 'R3',
+        host: '127.0.0.1',
+        port: 6379,
+        password: 'x',
+        createdAt: '2026-01-01T00:00:00Z',
+      );
+      final id = await LocalDb.instance.addConnection(row);
+      testMemorySecrets.failNextDelete = StateError('libsecret unavailable');
+
+      await LocalDb.instance.removeConnection(id);
+
+      final list = await LocalDb.instance.getConnections();
+      expect(list.where((c) => c.id == id), isEmpty);
+    });
+
+    test('addConnection rolls back SQLite row when secure-store write fails', () async {
+      testMemorySecrets.failNextWrite = StateError('keychain write failed');
+      const row = ConnectionRow(
+        type: 'redis',
+        name: 'R4',
+        host: '127.0.0.1',
+        port: 6379,
+        password: 'secret',
+        createdAt: '2026-01-01T00:00:00Z',
+      );
+
+      await expectLater(
+        LocalDb.instance.addConnection(row),
+        throwsA(isA<StateError>()),
+      );
+
+      final list = await LocalDb.instance.getConnections();
+      expect(list.where((c) => c.name == 'R4'), isEmpty);
+    });
+
+    test('updateConnection rolls back SQLite and secrets when secure-store write fails', () async {
+      const initialRow = ConnectionRow(
+        type: 'postgres',
+        name: 'PG_Before',
+        host: 'localhost',
+        port: 5432,
+        username: 'admin',
+        password: 'old-password',
+        createdAt: '2026-01-01T00:00:00Z',
+      );
+      final id = await LocalDb.instance.addConnection(initialRow);
+
+      testMemorySecrets.failNextWrite = StateError('keychain write failed');
+      final updatedRow = ConnectionRow(
+        id: id,
+        type: 'postgres',
+        name: 'PG_After',
+        host: 'db.example.com',
+        port: 5433,
+        username: 'root',
+        password: 'new-password',
+        createdAt: '2026-01-01T00:00:00Z',
+      );
+
+      await expectLater(
+        LocalDb.instance.updateConnection(updatedRow),
+        throwsA(isA<StateError>()),
+      );
+
+      final list = await LocalDb.instance.getConnections();
+      final loaded = list.singleWhere((c) => c.id == id);
+      expect(loaded.name, 'PG_Before');
+      expect(loaded.host, 'localhost');
+      expect(loaded.port, 5432);
+      expect(loaded.username, 'admin');
+      expect(loaded.password, 'old-password');
+    });
   });
 }

test/memory_secrets_backend.dart

@@ -8,11 +8,22 @@ final MemorySecretsStorageBackend testMemorySecrets =
 class MemorySecretsStorageBackend implements SecretsStorageBackend {
   final Map<String, String> _values = {};
 
+  /// When non-null, the next [write] throws this error (then clears the flag).
+  Object? failNextWrite;
+
+  /// When non-null, the next [delete] throws this error (then clears the flag).
+  Object? failNextDelete;
+
   @override
   Future<String?> read(String key) async => _values[key];
 
   @override
   Future<void> write(String key, String? value) async {
+    final fail = failNextWrite;
+    if (fail != null) {
+      failNextWrite = null;
+      throw fail;
+    }
     if (value == null || value.isEmpty) {
       _values.remove(key);
     } else {
@@ -22,8 +33,17 @@ class MemorySecretsStorageBackend implements SecretsStorageBackend {
 
   @override
   Future<void> delete(String key) async {
+    final fail = failNextDelete;
+    if (fail != null) {
+      failNextDelete = null;
+      throw fail;
+    }
     _values.remove(key);
   }
 
-  void clear() => _values.clear();
+  void clear() {
+    _values.clear();
+    failNextWrite = null;
+    failNextDelete = null;
+  }
 }