From c939e37d54b483c84460abb3679c4429a1f0209a Mon Sep 17 00:00:00 2001 From: Jhin Lee Date: Sun, 8 Nov 2026 21:03:03 -0500 Subject: [PATCH 1/3] Pin Windows pending-delete removal for remove and prune (#1837) --- .../io/model_download_manager_io.dart | 7 +++- ..._download_manager_io_concurrency_test.dart | 5 ++- ...odel_download_manager_io_removal_test.dart | 36 ++++++++++++++++++- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/lib/src/platform/io/model_download_manager_io.dart b/lib/src/platform/io/model_download_manager_io.dart index 1376513ce..8ae472782 100644 --- a/lib/src/platform/io/model_download_manager_io.dart +++ b/lib/src/platform/io/model_download_manager_io.dart @@ -798,6 +798,9 @@ class DefaultModelDownloadManager implements ModelDownloadManager { } } + // A path whose resolution is denied (errno 5) and whose type also reads as + // `notFound` is walked past like a missing one, so a parent ACL denial + // surfaces from the later operation instead of as `LlamaStateException`. String _cacheLockKey(Directory cacheDirectory) { var ancestor = path.normalize(path.absolute(cacheDirectory.path)); final missing = []; @@ -1861,7 +1864,9 @@ enum _Removal { removed, inUse, skipped } /// with `ERROR_ACCESS_DENIED`, and so does the attribute query behind /// [FileSystemEntity.type], which reports that failure as /// [FileSystemEntityType.notFound]; callers tell it from a real denial by -/// that type. +/// that type. A different ACL denial (for example on the entry's parent) can +/// read as `notFound` too; callers then treat the path as missing and the +/// operation still fails later, with a different error type than the denial. bool _mayBeWindowsDeletePending(FileSystemException error) => Platform.isWindows && error.osError?.errorCode == 5; diff --git a/test/unit/platform/io/model_download_manager_io_concurrency_test.dart b/test/unit/platform/io/model_download_manager_io_concurrency_test.dart index 99022b5ed..b2930f12c 100644 --- a/test/unit/platform/io/model_download_manager_io_concurrency_test.dart +++ b/test/unit/platform/io/model_download_manager_io_concurrency_test.dart @@ -35,7 +35,10 @@ void main() { }); setUp(() async { - root = await Directory.systemTemp.createTemp('llamadart_cache_lock_'); + final created = await Directory.systemTemp.createTemp( + 'llamadart_cache_lock_', + ); + root = Directory(await created.resolveSymbolicLinks()); addTearDown(() => root.delete(recursive: true)); server = await _HoldingServer.start(); addTearDown(server.close); diff --git a/test/unit/platform/io/model_download_manager_io_removal_test.dart b/test/unit/platform/io/model_download_manager_io_removal_test.dart index 8f866ee6d..e3ee6b5b0 100644 --- a/test/unit/platform/io/model_download_manager_io_removal_test.dart +++ b/test/unit/platform/io/model_download_manager_io_removal_test.dart @@ -168,10 +168,19 @@ void main() { release(); await clearing.then((_) {}, onError: (Object _) {}); }); - await waiting.future; + await waiting.future.timeout( + const Duration(seconds: 10), + onTimeout: () => throw StateError( + 'clear never reached the .removal.lock: it no longer takes the ' + 'lock this test holds it at.', + ), + ); return (clearing: clearing, proceed: release); } + Future withOverrides(Future Function() body) => + IOOverrides.runWithIOOverrides(body, overrides); + void expectOnlyLocksLeftOnceDeleted(void Function() finishDelete) { finishDelete(); expect( @@ -212,6 +221,31 @@ void main() { expectOnlyLocksLeftOnceDeleted(finishDelete); }); + test('is skipped by remove when the deletion starts after it finds the ' + 'entry', () async { + final cacheKey = (await manager.list()).single.cacheKey; + void Function()? finishDelete; + overrides.afterFirstCheck = () async => + finishDelete = markPendingDelete(); + + await withOverrides(() => manager.remove(cacheKey)); + + expect(finishDelete, isNotNull); + expectOnlyLocksLeftOnceDeleted(finishDelete!); + }); + + test('is skipped by prune when the deletion starts after it finds the ' + 'entry', () async { + void Function()? finishDelete; + overrides.afterFirstCheck = () async => + finishDelete = markPendingDelete(); + + await withOverrides(() => manager.prune(maxAge: Duration.zero)); + + expect(finishDelete, isNotNull); + expectOnlyLocksLeftOnceDeleted(finishDelete!); + }); + test('is not skipped when the entry still reads as a directory: clear ' 'reports the access denial', () async { markPendingDelete(); From 7d5b2743f448434d5656821898d4167e9f5ed493 Mon Sep 17 00:00:00 2001 From: Jhin Lee Date: Sun, 8 Nov 2026 21:14:56 -0500 Subject: [PATCH 2/3] Hold remove and prune at the removal lock in the pending-delete tests --- ...odel_download_manager_io_removal_test.dart | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/test/unit/platform/io/model_download_manager_io_removal_test.dart b/test/unit/platform/io/model_download_manager_io_removal_test.dart index e3ee6b5b0..912edffc9 100644 --- a/test/unit/platform/io/model_download_manager_io_removal_test.dart +++ b/test/unit/platform/io/model_download_manager_io_removal_test.dart @@ -136,13 +136,15 @@ void main() { return finishDelete; } - Future clear() => - IOOverrides.runWithIOOverrides(manager.clear, overrides); + Future withOverrides(Future Function() body) => + IOOverrides.runWithIOOverrides(body, overrides); - /// Starts [clear] and holds it at the removal lock, after it has checked - /// the entry, until the returned function runs. + Future clear() => withOverrides(manager.clear); + + /// Starts [operation] and holds it at the removal lock, after it has + /// checked the entry, until the returned function runs. Future<({Future clearing, void Function() proceed})> - clearHeldAtRemovalLock() async { + heldAtRemovalLock(Future Function() operation) async { final waiting = Completer(); final proceed = Completer(); modelCacheEntryLocker = (lockPath, {required throwIfCancelled}) async { @@ -163,7 +165,7 @@ void main() { } } - final clearing = clear(); + final clearing = withOverrides(operation); addTearDown(() async { release(); await clearing.then((_) {}, onError: (Object _) {}); @@ -171,15 +173,15 @@ void main() { await waiting.future.timeout( const Duration(seconds: 10), onTimeout: () => throw StateError( - 'clear never reached the .removal.lock: it no longer takes the ' - 'lock this test holds it at.', + 'The operation never reached the .removal.lock: it no longer takes ' + 'the lock this test holds it at.', ), ); return (clearing: clearing, proceed: release); } - Future withOverrides(Future Function() body) => - IOOverrides.runWithIOOverrides(body, overrides); + Future<({Future clearing, void Function() proceed})> + clearHeldAtRemovalLock() => heldAtRemovalLock(manager.clear); void expectOnlyLocksLeftOnceDeleted(void Function() finishDelete) { finishDelete(); @@ -221,29 +223,27 @@ void main() { expectOnlyLocksLeftOnceDeleted(finishDelete); }); - test('is skipped by remove when the deletion starts after it finds the ' - 'entry', () async { + test('is skipped by remove when the deletion happens while it waits for ' + 'the removal lock', () async { final cacheKey = (await manager.list()).single.cacheKey; - void Function()? finishDelete; - overrides.afterFirstCheck = () async => - finishDelete = markPendingDelete(); - - await withOverrides(() => manager.remove(cacheKey)); + final held = await heldAtRemovalLock(() => manager.remove(cacheKey)); + final finishDelete = markPendingDelete(); + held.proceed(); - expect(finishDelete, isNotNull); - expectOnlyLocksLeftOnceDeleted(finishDelete!); + await held.clearing; + expectOnlyLocksLeftOnceDeleted(finishDelete); }); - test('is skipped by prune when the deletion starts after it finds the ' - 'entry', () async { - void Function()? finishDelete; - overrides.afterFirstCheck = () async => - finishDelete = markPendingDelete(); - - await withOverrides(() => manager.prune(maxAge: Duration.zero)); + test('is skipped by prune when the deletion happens while it waits for ' + 'the removal lock', () async { + final held = await heldAtRemovalLock( + () => manager.prune(maxAge: Duration.zero), + ); + final finishDelete = markPendingDelete(); + held.proceed(); - expect(finishDelete, isNotNull); - expectOnlyLocksLeftOnceDeleted(finishDelete!); + await held.clearing; + expectOnlyLocksLeftOnceDeleted(finishDelete); }); test('is not skipped when the entry still reads as a directory: clear ' From 403835bb120abce840b06c979d17f0cc95997a51 Mon Sep 17 00:00:00 2001 From: Jhin Lee Date: Sun, 8 Nov 2026 22:06:44 -0500 Subject: [PATCH 3/3] Clarify the Windows ACL-denial comments --- lib/src/platform/io/model_download_manager_io.dart | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/src/platform/io/model_download_manager_io.dart b/lib/src/platform/io/model_download_manager_io.dart index 8ae472782..c5eb430ba 100644 --- a/lib/src/platform/io/model_download_manager_io.dart +++ b/lib/src/platform/io/model_download_manager_io.dart @@ -799,8 +799,9 @@ class DefaultModelDownloadManager implements ModelDownloadManager { } // A path whose resolution is denied (errno 5) and whose type also reads as - // `notFound` is walked past like a missing one, so a parent ACL denial - // surfaces from the later operation instead of as `LlamaStateException`. + // `notFound` (for example a parent ACL denial) is walked past like a missing + // one. The operation then fails later, as an access denial on create or + // write, or a cache miss for cacheOnly; it never reports success. String _cacheLockKey(Directory cacheDirectory) { var ancestor = path.normalize(path.absolute(cacheDirectory.path)); final missing = []; @@ -1864,9 +1865,8 @@ enum _Removal { removed, inUse, skipped } /// with `ERROR_ACCESS_DENIED`, and so does the attribute query behind /// [FileSystemEntity.type], which reports that failure as /// [FileSystemEntityType.notFound]; callers tell it from a real denial by -/// that type. A different ACL denial (for example on the entry's parent) can -/// read as `notFound` too; callers then treat the path as missing and the -/// operation still fails later, with a different error type than the denial. +/// that type. A different ACL denial can read as `notFound` too; see +/// `_cacheLockKey` for what happens then. bool _mayBeWindowsDeletePending(FileSystemException error) => Platform.isWindows && error.osError?.errorCode == 5;