From efdaa5e308cce57a4b1a0c48ef962ab4bc711616 Mon Sep 17 00:00:00 2001 From: "Graciliano M. P." Date: Tue, 11 Aug 2026 18:50:15 -0300 Subject: [PATCH] fix: DBObjectDirectoryAdapter lost objects written just before a read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_saveObject` was `async` and performed `File.writeAsString`, but both callers — `doInsert` and `doUpdate` — dropped the returned `Future`. Every reader in the adapter inspects the filesystem *synchronously* (`listSync`, `existsSync`), so there was no happens-before relationship between a `store` completing and its object being on disk. The failure is silent rather than loud: `_doSelectAllImpl` lists the directory, reads each entry, and passes the result through `resolveAllNotNull()`. A file that has not landed yet reads back as `null` and is simply discarded, so `selectAll` returns fewer objects with no error. This is what made `Pagination [objectAdapter]` flaky on CI. The evidence fits: the entries that went missing were always the most recently stored ones (the test stores in the order PG-03, PG-01, PG-05, PG-02, PG-04, and the failures dropped PG-04, or PG-04 and PG-02), and which ones varied per run. It does not reproduce on a fast local disk, so it only ever showed up on CI. Confirmed by experiment: inserting a 30ms delay before the un-awaited write reproduces that exact assertion failure locally. Fixed by making the write synchronous, consistent with every other filesystem operation in this class. Note `analysis_options.yaml` sets `discarded_futures: false`, which is why the dropped `Future` was never flagged. Adds a test asserting that a stored object is immediately readable. It only fails where the write is slow enough to lose the race, so it is a statement of the invariant rather than a sensitive guard — `Pagination [objectAdapter]` remains the test that actually catches this, and it should now be stable. Co-Authored-By: Claude Opus 5 (1M context) --- .../bones_api_entity_db_object_directory.dart | 16 ++-- test/bones_api_entity_db_directory_test.dart | 76 ++++++++++++++++++- 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/lib/src/bones_api_entity_db_object_directory.dart b/lib/src/bones_api_entity_db_object_directory.dart index 86283ea..daab98a 100644 --- a/lib/src/bones_api_entity_db_object_directory.dart +++ b/lib/src/bones_api_entity_db_object_directory.dart @@ -749,14 +749,18 @@ class DBObjectDirectoryAdapter return _finishOperation(op, id, preFinish); } - Future _saveObject( - String table, - Object? id, - Map obj, - ) async { + /// Writes synchronously, and must stay synchronous. + /// + /// Every reader in this adapter checks the filesystem synchronously + /// ([Directory.listSync], [File.existsSync]), so an asynchronous write would + /// let a store return before its object is visible: a `store` immediately + /// followed by a `selectAll`/`selectByID` could miss it, and + /// [_doSelectAllImpl] would silently drop it (a not-yet-written file reads + /// back as `null`, which `resolveAllNotNull` discards). + void _saveObject(String table, Object? id, Map obj) { var file = _resolveObjectFile(table, id); var enc = dart_convert.json.encode(obj); - await file.writeAsString(enc); + file.writeAsStringSync(enc); } Future?> _readObject(String table, Object? id) async { diff --git a/test/bones_api_entity_db_directory_test.dart b/test/bones_api_entity_db_directory_test.dart index 304a6e2..972f88b 100644 --- a/test/bones_api_entity_db_directory_test.dart +++ b/test/bones_api_entity_db_directory_test.dart @@ -1,13 +1,15 @@ @TestOn('vm') @Tags(['entities']) -@Timeout(Duration(seconds: 30)) +@Timeout(Duration(seconds: 60)) import 'dart:io'; +import 'dart:typed_data'; import 'package:bones_api/bones_api_db_directory.dart'; import 'package:bones_api/bones_api_test.dart'; import 'package:test/test.dart'; import 'bones_api_entity_db_tests_base.dart'; +import 'bones_api_test_entities.dart'; class MemoryTestConfig extends APITestConfigDBSQLMemory { MemoryTestConfig() @@ -23,6 +25,78 @@ Future main() async { await _runTest(false, false); await _runTest(true, true); await _runTest(false, true); + + _runStoreVisibilityTest(); +} + +/// REGRESSION: `DBObjectDirectoryAdapter._saveObject` used to be `async`, and +/// `doInsert`/`doUpdate` dropped its `Future`. Since every reader in that +/// adapter inspects the filesystem synchronously, a `store` could return +/// before its object was on disk — and `selectAll` would then *silently omit* +/// it, because a not-yet-written file reads back as `null` and is discarded by +/// `resolveAllNotNull`. +/// +/// That is what made `Pagination [objectAdapter]` flaky on CI: entries went +/// missing from the result, and which ones varied per run. Confirmed by adding +/// a 30ms delay before the (un-awaited) write, which reproduces that failure +/// exactly. +/// +/// Note this test only *fails* where the write is slow enough to lose the +/// race — it does not on a fast local disk. It is kept as a cheap statement of +/// the invariant; `Pagination [objectAdapter]` remains the sensitive guard. +void _runStoreVisibilityTest() { + group('DBObjectDirectoryAdapter', () { + test('a stored object is immediately visible', () async { + var tempDir = Directory.systemTemp.createTempSync( + 'bones_api_tests_object_dir_visibility', + ); + + var provider = createEntityRepositoryProvider2( + true, + (p, dbPort, dbConfig) => + DBObjectDirectoryAdapter(tempDir, parentRepositoryProvider: p), + 0, + null, + ); + + addTearDown(() { + provider.close(); + try { + tempDir.deleteSync(recursive: true); + } catch (_) {} + }); + + await provider.ensureInitialized(); + + var photoRepo = provider.photoAPIRepository; + + // Big enough to give a slow filesystem a chance to lose the race: + var data = Uint8List(512 * 1024); + + var ids = []; + + for (var i = 1; i <= 4; ++i) { + var id = 'PG-SYNC-0$i'; + ids.add(id); + + expect(await photoRepo.store(Photo.fromData(data, id: id)), equals(id)); + + expect( + await photoRepo.selectByID(id), + isNotNull, + reason: '`$id` not readable right after `store`', + ); + } + + var all = await photoRepo.selectAll(); + + expect( + all.map((e) => e.id).where(ids.contains).toList()..sort(), + equals(ids), + reason: '`selectAll` dropped a stored object', + ); + }); + }); } Future _runTest(bool useReflection, bool populateSource) {