From 9a8e7aefd209742377e7ba4a840ba82dab5e2e09 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Mon, 9 Nov 2026 08:37:02 +0100 Subject: [PATCH] =?UTF-8?q?Fix=20flaky=20image=5Fstore=5Fio=5Ftest=20"with?= =?UTF-8?q?out=20picker=20cop=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Todo: 2961b5ff --- lib/images/image_cache_store.dart | 14 ++-- test/attachments/image_store_io_test.dart | 16 +++-- .../images/image_cache_store_vanish_test.dart | 69 +++++++++++++++++++ 3 files changed, 91 insertions(+), 8 deletions(-) create mode 100644 test/images/image_cache_store_vanish_test.dart diff --git a/lib/images/image_cache_store.dart b/lib/images/image_cache_store.dart index 5abf4d9..d9bfd97 100644 --- a/lib/images/image_cache_store.dart +++ b/lib/images/image_cache_store.dart @@ -132,10 +132,16 @@ class ImageCacheStore { for (final ref in pending()) { final file = fileFor(ref); if (!file.existsSync()) continue; - final length = await file.length(); - if (await client.upload(ref, file.openRead(), length)) { - await _marker(ref).delete(); - uploaded++; + try { + final length = await file.length(); + if (await client.upload(ref, file.openRead(), length)) { + await _marker(ref).delete(); + uploaded++; + } + } on FileSystemException { + // Deleted mid-pass (note removed, cache cleared): nothing left to + // upload. A surviving marker is skipped by the check above next time. + continue; } } for (final ref in pass.referenced) { diff --git a/test/attachments/image_store_io_test.dart b/test/attachments/image_store_io_test.dart index 72ee4a8..4ed71e6 100644 --- a/test/attachments/image_store_io_test.dart +++ b/test/attachments/image_store_io_test.dart @@ -36,14 +36,19 @@ void main() { installFakeSecureStorage(initial: {'dufsImagesPassword': 'pw'}); store = make(); }); - tearDown(() => root.deleteSync(recursive: true)); + tearDown(() async { + // attach/attachFile start an upload pass they do not await; let it end + // before its directory goes, or it races the delete. + await store.reconcile(const []); + root.deleteSync(recursive: true); + }); test('attach stores, notifies and uploads straight away', () async { final ref = (await store.attach('n1', Uint8List.fromList([0xFF, 1])))!; expect(ref.noteId, 'n1'); expect(store.sourceFor(ref), isA()); expect(notified, greaterThanOrEqualTo(1)); - await pumpEventQueue(); + await store.reconcile(const []); // drains the pass attach queued expect(dufs.files[ref.relativePath], [0xFF, 1]); expect(store.isPending(ref), isFalse); }); @@ -67,7 +72,7 @@ void main() { test('without a password attach queues offline', () async { installFakeSecureStorage(); final ref = (await store.attach('n1', Uint8List.fromList([0xFF])))!; - await pumpEventQueue(); + await store.reconcile(const []); // drains the pass attach queued expect(dufs.requests, isEmpty); expect(store.isPending(ref), isTrue); expect((await store.status()).configured, isFalse); @@ -114,7 +119,7 @@ void main() { expect(a.isImage, isFalse); expect(notified, greaterThanOrEqualTo(1)); expect(await store.sizeOf(a.ref), 2); - await pumpEventQueue(); + await store.reconcile(const []); // drains the pass attach queued expect(dufs.files[a.ref.relativePath], [1, 2]); expect(File(p.join(elsewhere.path, 'r.pdf')).existsSync(), isTrue); }); @@ -149,6 +154,9 @@ void main() { final picked = file(picks, 'kept.txt', [1]); await plain.attachFile('n1', picked); expect(File(picked.path).existsSync(), isTrue); + // Drain the upload pass attachFile started, so teardown cannot delete + // the root underneath it. + await plain.reconcile(const []); }); }); diff --git a/test/images/image_cache_store_vanish_test.dart b/test/images/image_cache_store_vanish_test.dart new file mode 100644 index 0000000..e0d9ffd --- /dev/null +++ b/test/images/image_cache_store_vanish_test.dart @@ -0,0 +1,69 @@ +import 'dart:io'; +import 'dart:typed_data'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:todo/images/attachment_ref.dart'; +import 'package:todo/images/image_cache_store.dart'; + +import 'fake_dufs.dart'; + +/// A file that `existsSync()` still sees but that is gone by the time its +/// length is read: the window a teardown or a note deletion can hit while an +/// upload pass is between the two calls. +class _VanishingFile implements File { + _VanishingFile(this.path); + + @override + final String path; + + @override + bool existsSync() => true; + + @override + Future length() async => + throw PathNotFoundException(path, const OSError('gone', 2)); + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +/// Hands out a [_VanishingFile] for exactly one path, real files otherwise. +final class _VanishOverrides extends IOOverrides { + _VanishOverrides(this.vanishing); + + final String vanishing; + + @override + File createFile(String path) => + path == vanishing ? _VanishingFile(path) : super.createFile(path); +} + +void main() { + late Directory root; + late ImageCacheStore store; + late FakeDufs dufs; + + final a = AttachmentRef.tryParse('n1/a.jpg')!; + final b = AttachmentRef.tryParse('n2/b.jpg')!; + final bytes = Uint8List.fromList([1, 2, 3]); + + setUp(() { + root = Directory.systemTemp.createTempSync('image_cache_vanish'); + store = ImageCacheStore(root); + dufs = FakeDufs(); + }); + tearDown(() => root.deleteSync(recursive: true)); + + test('a file vanishing mid-pass is skipped; the rest still upload', () async { + await store.put(a, bytes); + await store.put(b, bytes); + final report = await IOOverrides.runWithIOOverrides( + () => store.reconcile({}, dufs.client()), + _VanishOverrides(store.fileFor(a).path), + ); + expect(report.uploaded, 1); + expect(dufs.files.keys, ['n2/b.jpg']); + expect(store.isPending(a), isTrue); + expect(store.isPending(b), isFalse); + }); +}