diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt index f2f1469e..f7bcb526 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeDownloadFinalizer.kt @@ -41,15 +41,13 @@ object NativeDownloadFinalizer { // Keep this schema contract in sync with Dart HistoryDatabase before bumping either side. const val HISTORY_SCHEMA_VERSION = 12 internal val activeFFmpegSessionIds = mutableSetOf() - internal val nativeFFmpegSessionIds = mutableSetOf() + internal val nativeFFmpegSessionIds = BoundedRegistry(maxEntries = 256) internal val activeFFmpegSessionLock = Any() internal val ffmpegCompleteCallbackLock = Any() - internal val qualityVariantNameLocks = java.util.concurrent.ConcurrentHashMap() + internal val qualityVariantNameLocks = KeyedLockPool() internal var forwardedFFmpegCompleteCallback: FFmpegSessionCompleteCallback? = null internal val nativeFilteringFFmpegCompleteCallback = FFmpegSessionCompleteCallback { session -> - val isNativeSession = synchronized(activeFFmpegSessionLock) { - nativeFFmpegSessionIds.contains(session.sessionId) - } + val isNativeSession = nativeFFmpegSessionIds.consume(session.sessionId) if (!isNativeSession) { val delegate = synchronized(ffmpegCompleteCallbackLock) { forwardedFFmpegCompleteCallback diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerFFmpeg.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerFFmpeg.kt index 90a6e023..e586a56b 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerFFmpeg.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerFFmpeg.kt @@ -104,8 +104,8 @@ internal fun NativeDownloadFinalizer.runFFmpeg(command: String, shouldCancel: () val sessionId = session.sessionId synchronized(activeFFmpegSessionLock) { activeFFmpegSessionIds.add(sessionId) - nativeFFmpegSessionIds.add(sessionId) } + nativeFFmpegSessionIds.add(sessionId) FFmpegKitConfig.asyncFFmpegExecute(session) try { var cancelRequested = false diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerMedia.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerMedia.kt index f5e5f204..766afcc7 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerMedia.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeFinalizerMedia.kt @@ -124,8 +124,7 @@ internal fun NativeDownloadFinalizer.finalizeQualityVariantFilename( val cleanTarget = File(source.parentFile, cleanName) val lockTarget = if (collisionOnly) cleanTarget else File(source.parentFile, preferredName) val lockKey = lockTarget.absolutePath.lowercase(Locale.ROOT) - val lock = qualityVariantNameLocks.computeIfAbsent(lockKey) { Any() } - synchronized(lock) { + qualityVariantNameLocks.withLock(lockKey) { val selectedName = if (collisionOnly) { resolveQualityVariantFilename( fileName = logicalFileName, @@ -138,11 +137,11 @@ internal fun NativeDownloadFinalizer.finalizeQualityVariantFilename( preferredName } input.result.put("quality_variant_file_name", selectedName) - if (selectedName == state.fileName) return@synchronized + if (selectedName == state.fileName) return@withLock val target = uniqueLocalFile(source.parentFile, selectedName) if (!source.renameTo(target)) { Log.w(TAG, "Could not rename quality variant output: ${source.absolutePath}") - return@synchronized + return@withLock } state.filePath = target.absolutePath state.fileName = target.name diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/NativeResourcePools.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeResourcePools.kt new file mode 100644 index 00000000..3c6ec716 --- /dev/null +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/NativeResourcePools.kt @@ -0,0 +1,63 @@ +package com.zarz.spotiflac + +import java.util.LinkedHashSet + +/** + * Serializes work per key while allowing idle key entries to be reclaimed. + * References are counted before a caller waits on the monitor, so a waiter can + * never race with removal and end up using a different monitor for the same key. + */ +internal class KeyedLockPool { + private class Entry { + val monitor = Any() + var references = 0 + } + + private val registryLock = Any() + private val entries = HashMap() + + fun withLock(key: K, block: () -> T): T { + val entry = synchronized(registryLock) { + entries.getOrPut(key) { Entry() }.also { it.references++ } + } + try { + return synchronized(entry.monitor) { block() } + } finally { + synchronized(registryLock) { + entry.references-- + if (entry.references == 0 && entries[key] === entry) { + entries.remove(key) + } + } + } + } + + internal fun activeKeyCount(): Int = synchronized(registryLock) { + entries.size + } +} + +/** A small insertion-ordered registry that consumes completed IDs. */ +internal class BoundedRegistry(private val maxEntries: Int) { + private val lock = Any() + private val values = LinkedHashSet() + + init { + require(maxEntries > 0) { "maxEntries must be greater than zero" } + } + + fun add(value: T) = synchronized(lock) { + values.remove(value) + values.add(value) + while (values.size > maxEntries) { + val oldest = values.iterator().next() + values.remove(oldest) + } + } + + fun consume(value: T): Boolean = synchronized(lock) { + values.remove(value) + } + + internal fun size(): Int = synchronized(lock) { values.size } +} diff --git a/android/app/src/main/kotlin/com/zarz/spotiflac/SafDownloadHandler.kt b/android/app/src/main/kotlin/com/zarz/spotiflac/SafDownloadHandler.kt index c322a54e..433107f7 100644 --- a/android/app/src/main/kotlin/com/zarz/spotiflac/SafDownloadHandler.kt +++ b/android/app/src/main/kotlin/com/zarz/spotiflac/SafDownloadHandler.kt @@ -24,7 +24,7 @@ object SafDownloadHandler { // cannot interleave writes into one document. Different names keep // downloading in parallel; the second same-name caller blocks, then hits // the exists check and reports already_exists. - private val safNameLocks = java.util.concurrent.ConcurrentHashMap() + private val safNameLocks = KeyedLockPool() data class UniqueWriteResult(val uri: String, val fileName: String) data class ExistingAwareWriteResult( @@ -40,8 +40,7 @@ object SafDownloadHandler { block: () -> T ): T { val key = "$treeUriStr|$relativeDir|${fileName.lowercase(Locale.ROOT)}" - val lock = safNameLocks.computeIfAbsent(key) { Any() } - return synchronized(lock) { block() } + return safNameLocks.withLock(key, block) } /** diff --git a/android/app/src/test/kotlin/com/zarz/spotiflac/NativeResourcePoolsTest.kt b/android/app/src/test/kotlin/com/zarz/spotiflac/NativeResourcePoolsTest.kt new file mode 100644 index 00000000..9375900d --- /dev/null +++ b/android/app/src/test/kotlin/com/zarz/spotiflac/NativeResourcePoolsTest.kt @@ -0,0 +1,62 @@ +package com.zarz.spotiflac + +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import kotlin.concurrent.thread +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class NativeResourcePoolsTest { + @Test + fun keyedLockPoolReclaimsAnIdleKey() { + val pool = KeyedLockPool() + + assertEquals("done", pool.withLock("track.flac") { "done" }) + assertEquals(0, pool.activeKeyCount()) + } + + @Test + fun keyedLockPoolKeepsOneMonitorWhileAnotherCallerWaits() { + val pool = KeyedLockPool() + val firstEntered = CountDownLatch(1) + val releaseFirst = CountDownLatch(1) + val secondEntered = CountDownLatch(1) + + val first = thread { + pool.withLock("same-name.flac") { + firstEntered.countDown() + releaseFirst.await(2, TimeUnit.SECONDS) + } + } + assertTrue(firstEntered.await(2, TimeUnit.SECONDS)) + + val second = thread { + pool.withLock("same-name.flac") { + secondEntered.countDown() + } + } + assertFalse(secondEntered.await(100, TimeUnit.MILLISECONDS)) + assertEquals(1, pool.activeKeyCount()) + + releaseFirst.countDown() + first.join(2_000) + second.join(2_000) + assertTrue(secondEntered.await(100, TimeUnit.MILLISECONDS)) + assertEquals(0, pool.activeKeyCount()) + } + + @Test + fun boundedRegistryConsumesIdsAndEvictsOldestEntries() { + val registry = BoundedRegistry(maxEntries = 2) + registry.add(1) + registry.add(2) + registry.add(3) + + assertFalse(registry.consume(1)) + assertTrue(registry.consume(2)) + assertTrue(registry.consume(3)) + assertEquals(0, registry.size()) + } +} diff --git a/go_backend/exports_extensions.go b/go_backend/exports_extensions.go index cf899722..b6e0daec 100644 --- a/go_backend/exports_extensions.go +++ b/go_backend/exports_extensions.go @@ -992,7 +992,7 @@ func callExtensionFunctionJSONWithRequestID(extensionID, functionName string, ti perf.recordJS(time.Since(jsStartedAt)) if err != nil { if IsRuntimeUnsafeError(err) { - quarantineRuntimeLocked(ext, vm) + quarantineRuntimeLocked(ext, vm, err) } if isExtensionRequestCancelled(requestID) || errors.Is(err, ErrExtensionRequestCancelled) { return "", ErrExtensionRequestCancelled diff --git a/go_backend/extension_manager.go b/go_backend/extension_manager.go index 64226eba..23c1b08c 100644 --- a/go_backend/extension_manager.go +++ b/go_backend/extension_manager.go @@ -29,6 +29,9 @@ type loadedExtension struct { isolatedPoolMu sync.Mutex isolatedPool []*isolatedRuntimeHandle + + quarantineMu sync.Mutex + quarantinedRuntimes int } type isolatedRuntimeHandle struct { @@ -53,6 +56,11 @@ func getExtensionInitSettings(extensionID string) map[string]any { } func ensureRuntimeReadyLocked(ext *loadedExtension, applyStoredSettings bool) error { + if hasQuarantinedRuntime(ext) { + err := fmt.Errorf("extension runtime is still stopping after an unresponsive request") + ext.Error = err.Error() + return err + } // Gate enabling too, so a package installed with a failed gate cannot be // switched on anyway. if err := validateManifestGates(ext.Manifest); err != nil { @@ -945,7 +953,7 @@ func (m *extensionManager) InvokeAction(extensionID string, actionName string) ( result, err := RunWithTimeoutAndRecover(vm, script, DefaultJSTimeout) if err != nil { if IsRuntimeUnsafeError(err) { - quarantineRuntimeLocked(ext, vm) + quarantineRuntimeLocked(ext, vm, err) } GoLog("[Extension] InvokeAction error for %s.%s: %v\n", extensionID, actionName, err) return nil, fmt.Errorf("action failed: %v", err) diff --git a/go_backend/extension_manager_runtime.go b/go_backend/extension_manager_runtime.go index afbe67ef..798bc6b6 100644 --- a/go_backend/extension_manager_runtime.go +++ b/go_backend/extension_manager_runtime.go @@ -151,6 +151,9 @@ const maxIdleIsolatedRuntimes = 1 // acquireIsolatedExtensionRuntime pops an idle pooled runtime or builds one. func acquireIsolatedExtensionRuntime(ext *loadedExtension) (*goja.Runtime, *extensionRuntime, error) { + if hasQuarantinedRuntime(ext) { + return nil, nil, fmt.Errorf("extension runtime is still stopping after an unresponsive request") + } ext.isolatedPoolMu.Lock() if n := len(ext.isolatedPool); n > 0 { handle := ext.isolatedPool[n-1] @@ -162,13 +165,27 @@ func acquireIsolatedExtensionRuntime(ext *loadedExtension) (*goja.Runtime, *exte ext.VMMu.Lock() defer ext.VMMu.Unlock() + if hasQuarantinedRuntime(ext) { + return nil, nil, fmt.Errorf("extension runtime is still stopping after an unresponsive request") + } return newIsolatedExtensionRuntime(ext) } // releaseIsolatedExtensionRuntime pools a healthy runtime for reuse or tears // it down. Pass healthy=false after an interrupt/timeout/script error, whose // VM state can't be trusted for reuse. -func releaseIsolatedExtensionRuntime(ext *loadedExtension, vm *goja.Runtime, runtime *extensionRuntime, healthy, cleanupSafe bool) { +func releaseIsolatedExtensionRuntime( + ext *loadedExtension, + vm *goja.Runtime, + runtime *extensionRuntime, + healthy, cleanupSafe bool, + unsafeDone <-chan struct{}, +) { + if !cleanupSafe { + registerQuarantinedRuntime(ext, runtime, unsafeDone) + return + } + if runtime != nil { if err := runtime.flushStorageNow(); err != nil { GoLog("[Extension:%s] isolated download storage flush failed: %v\n", ext.ID, err) @@ -195,17 +212,57 @@ func releaseIsolatedExtensionRuntime(ext *loadedExtension, vm *goja.Runtime, run } } +func hasQuarantinedRuntime(ext *loadedExtension) bool { + if ext == nil { + return false + } + ext.quarantineMu.Lock() + defer ext.quarantineMu.Unlock() + return ext.quarantinedRuntimes > 0 +} + +// registerQuarantinedRuntime keeps the extension gated until the interrupted +// goroutine actually exits. A Go goroutine cannot be killed safely, so allowing +// a replacement VM immediately would let a broken extension accumulate an +// unbounded number of runtimes. The runtime is touched only after completion. +func registerQuarantinedRuntime(ext *loadedExtension, runtime *extensionRuntime, done <-chan struct{}) { + if ext == nil { + return + } + ext.quarantineMu.Lock() + ext.quarantinedRuntimes++ + ext.quarantineMu.Unlock() + + if done == nil { + GoLog("[Extension:%s] quarantined runtime has no completion signal; keeping extension gated\n", ext.ID) + return + } + go func() { + <-done + if runtime != nil { + runtime.closeStorageFlusher() + } + ext.quarantineMu.Lock() + if ext.quarantinedRuntimes > 0 { + ext.quarantinedRuntimes-- + } + ext.quarantineMu.Unlock() + }() +} + // quarantineRuntimeLocked detaches a VM that remained busy after interrupt. // The caller holds VMMu. Touching or cleaning up that VM would race its stuck -// goroutine; a later call will build a fresh runtime from indexProgram. -func quarantineRuntimeLocked(ext *loadedExtension, vm *goja.Runtime) { +// goroutine; replacement calls remain gated until that goroutine exits. +func quarantineRuntimeLocked(ext *loadedExtension, vm *goja.Runtime, err error) { if ext == nil || ext.VM != vm { return } + runtime := ext.runtime ext.VM = nil ext.runtime = nil ext.initialized = false ext.Error = "extension runtime was quarantined after an unresponsive script" + registerQuarantinedRuntime(ext, runtime, runtimeCompletion(err)) } // drainIsolatedRuntimePool tears down idle isolated runtimes. Called on diff --git a/go_backend/extension_provider_wrapper.go b/go_backend/extension_provider_wrapper.go index ff2e8d03..c33333b3 100644 --- a/go_backend/extension_provider_wrapper.go +++ b/go_backend/extension_provider_wrapper.go @@ -107,7 +107,7 @@ func callExtension[T any](p *extensionProviderWrapper, opts extCallOpts, parse f perf.recordPayload(result) if err != nil { if IsRuntimeUnsafeError(err) { - quarantineRuntimeLocked(p.extension, p.vm) + quarantineRuntimeLocked(p.extension, p.vm, err) } if opts.requestID != "" && isExtensionRequestCancelled(opts.requestID) { return zero, ErrExtensionRequestCancelled @@ -476,7 +476,7 @@ func (p *extensionProviderWrapper) EnrichTrackForItemID(track *ExtTrackMetadata, perf.recordPayload(result) if err != nil { if IsRuntimeUnsafeError(err) { - quarantineRuntimeLocked(p.extension, p.vm) + quarantineRuntimeLocked(p.extension, p.vm, err) } if isDownloadCancelled(itemID) { return track, ErrDownloadCancelled @@ -605,8 +605,16 @@ func (p *extensionProviderWrapper) DownloadPrepared( } vmHealthy := false cleanupSafe := true + var unsafeDone <-chan struct{} defer func() { - releaseIsolatedExtensionRuntime(p.extension, vm, runtime, vmHealthy, cleanupSafe) + releaseIsolatedExtensionRuntime( + p.extension, + vm, + runtime, + vmHealthy, + cleanupSafe, + unsafeDone, + ) }() if runtime != nil { runtime.setActiveDownloadItemID(itemID) @@ -660,6 +668,7 @@ func (p *extensionProviderWrapper) DownloadPrepared( perf.recordPayload(result) vmHealthy = err == nil cleanupSafe = !IsRuntimeUnsafeError(err) + unsafeDone = runtimeCompletion(err) if err != nil { errMsg := err.Error() errType := "script_error" diff --git a/go_backend/extension_timeout.go b/go_backend/extension_timeout.go index 1521c13f..1c28268b 100644 --- a/go_backend/extension_timeout.go +++ b/go_backend/extension_timeout.go @@ -15,6 +15,7 @@ type JSExecutionError struct { IsTimeout bool RuntimeUnsafe bool Cause error + runtimeDone <-chan struct{} } func (e *JSExecutionError) Error() string { @@ -56,11 +57,13 @@ func runGojaCallWithTimeoutContext(ctx context.Context, vm *goja.Runtime, call f err error } resultCh := make(chan result, 1) + executionDone := make(chan struct{}) var interrupted bool var interruptMu sync.Mutex go func() { + defer close(executionDone) defer func() { if r := recover(); r != nil { interruptMu.Lock() @@ -115,7 +118,7 @@ func runGojaCallWithTimeoutContext(ctx context.Context, vm *goja.Runtime, call f case <-time.After(jsInterruptGracePeriod): // Goroutine is truly stuck (e.g. HTTP read with no timeout). // Log a warning — the VM should NOT be reused after this. - GoLog("[extensionRuntime] WARNING: JS goroutine did not exit within 60s after interrupt, VM may be unsafe\n") + GoLog("[extensionRuntime] WARNING: JS goroutine did not exit within %s after interrupt, VM may be unsafe\n", jsInterruptGracePeriod) message := "execution timeout exceeded (runtime quarantined)" var cause error if cancelled { @@ -127,6 +130,7 @@ func runGojaCallWithTimeoutContext(ctx context.Context, vm *goja.Runtime, call f IsTimeout: !cancelled, RuntimeUnsafe: true, Cause: cause, + runtimeDone: executionDone, } } } @@ -167,6 +171,13 @@ func IsRuntimeUnsafeError(err error) bool { return ok && jsErr.RuntimeUnsafe } +func runtimeCompletion(err error) <-chan struct{} { + if jsErr, ok := err.(*JSExecutionError); ok && jsErr.RuntimeUnsafe { + return jsErr.runtimeDone + } + return nil +} + func IsTimeoutError(err error) bool { if jsErr, ok := err.(*JSExecutionError); ok { return jsErr.IsTimeout diff --git a/go_backend/log_progress_timeout_supplement_test.go b/go_backend/log_progress_timeout_supplement_test.go index 5429288c..95e024c2 100644 --- a/go_backend/log_progress_timeout_supplement_test.go +++ b/go_backend/log_progress_timeout_supplement_test.go @@ -163,7 +163,23 @@ func TestRunWithTimeoutQuarantinesUnresponsiveRuntime(t *testing.T) { close(release) t.Fatalf("expected unsafe runtime error, got %v", err) } + done := runtimeCompletion(err) + if done == nil { + close(release) + t.Fatal("unsafe runtime error should expose a completion signal") + } + select { + case <-done: + close(release) + t.Fatal("completion signal closed while the JS goroutine was blocked") + default: + } close(release) + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("completion signal did not close after the JS goroutine exited") + } } func TestRunWithTimeoutQuarantinesUnresponsiveCancelledRuntime(t *testing.T) { @@ -193,5 +209,59 @@ func TestRunWithTimeoutQuarantinesUnresponsiveCancelledRuntime(t *testing.T) { close(release) t.Fatalf("expected unsafe cancellation error, got %v", err) } + done := runtimeCompletion(err) + if done == nil { + close(release) + t.Fatal("unsafe cancellation should expose a completion signal") + } close(release) + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("cancelled runtime did not report completion") + } +} + +func TestQuarantinedRuntimeBlocksReplacementUntilExecutionStops(t *testing.T) { + vm := goja.New() + runtime := &extensionRuntime{} + ext := &loadedExtension{ + ID: "quarantine-test", + VM: vm, + runtime: runtime, + initialized: true, + } + done := make(chan struct{}) + err := &JSExecutionError{ + Message: "runtime quarantined", + RuntimeUnsafe: true, + runtimeDone: done, + } + + quarantineRuntimeLocked(ext, vm, err) + if ext.VM != nil || ext.runtime != nil || ext.initialized { + t.Fatal("quarantine should detach the unsafe runtime") + } + if !hasQuarantinedRuntime(ext) { + t.Fatal("extension should remain gated while execution is still running") + } + if err := ensureRuntimeReadyLocked(ext, false); err == nil || + !strings.Contains(err.Error(), "still stopping") { + t.Fatalf("replacement runtime should be blocked, got %v", err) + } + + close(done) + deadline := time.Now().Add(time.Second) + for hasQuarantinedRuntime(ext) && time.Now().Before(deadline) { + time.Sleep(time.Millisecond) + } + if hasQuarantinedRuntime(ext) { + t.Fatal("extension remained gated after the old execution stopped") + } + runtime.storageMu.RLock() + closed := runtime.storageClosed + runtime.storageMu.RUnlock() + if !closed { + t.Fatal("quarantined runtime resources were not closed after completion") + } } diff --git a/lib/screens/album_screen.dart b/lib/screens/album_screen.dart index 96199672..529d20de 100644 --- a/lib/screens/album_screen.dart +++ b/lib/screens/album_screen.dart @@ -32,7 +32,10 @@ import 'package:spotiflac_android/widgets/selection_action_button.dart'; import 'package:spotiflac_android/widgets/selection_bottom_bar.dart'; class _AlbumCache { - static final _cache = TtlCache>(const Duration(minutes: 10)); + static final _cache = TtlCache>( + const Duration(minutes: 10), + maxEntries: 40, + ); static List? get(String albumId) => _cache.get(albumId); diff --git a/lib/screens/artist_screen.dart b/lib/screens/artist_screen.dart index bda39145..f4e57b76 100644 --- a/lib/screens/artist_screen.dart +++ b/lib/screens/artist_screen.dart @@ -39,7 +39,10 @@ import 'package:spotiflac_android/widgets/view_queue_snackbar_action.dart'; part 'artist_screen_widgets.dart'; class _ArtistCache { - static final _cache = TtlCache<_CacheEntry>(const Duration(minutes: 10)); + static final _cache = TtlCache<_CacheEntry>( + const Duration(minutes: 10), + maxEntries: 24, + ); static _CacheEntry? get(String artistId) => _cache.get(artistId); diff --git a/lib/services/downloaded_embedded_cover_resolver.dart b/lib/services/downloaded_embedded_cover_resolver.dart index 9efb5b66..e5a4ceac 100644 --- a/lib/services/downloaded_embedded_cover_resolver.dart +++ b/lib/services/downloaded_embedded_cover_resolver.dart @@ -21,13 +21,14 @@ class _EmbeddedCoverCacheEntry { /// when the source file changed. class DownloadedEmbeddedCoverResolver { static const int _maxCacheEntries = 180; + static const int _maxFailedExtractEntries = 360; static final LinkedHashMap _cache = LinkedHashMap(); static final Set _pendingExtract = {}; static final Set _pendingRefresh = {}; static final Set _pendingPreviewValidation = {}; - static final Set _failedExtract = {}; + static final LinkedHashSet _failedExtract = LinkedHashSet(); static String cleanFilePath(String? filePath) { if (filePath == null) return ''; @@ -132,6 +133,15 @@ class DownloadedEmbeddedCoverResolver { } } + static void _rememberFailedExtract(String cleanPath) { + _failedExtract + ..remove(cleanPath) + ..add(cleanPath); + while (_failedExtract.length > _maxFailedExtractEntries) { + _failedExtract.remove(_failedExtract.first); + } + } + static void _validateCachedPreviewAsync( String cleanPath, _EmbeddedCoverCacheEntry entry, { @@ -186,7 +196,7 @@ class DownloadedEmbeddedCoverResolver { final hasCover = result['error'] == null && await File(outputPath).exists(); if (!hasCover) { - _failedExtract.add(cleanPath); + _rememberFailedExtract(cleanPath); _scheduleTempCoverCleanup(outputPath); return; } @@ -205,7 +215,7 @@ class DownloadedEmbeddedCoverResolver { } onChanged?.call(); } catch (_) { - _failedExtract.add(cleanPath); + _rememberFailedExtract(cleanPath); _scheduleTempCoverCleanup(outputPath); } finally { _pendingExtract.remove(cleanPath); diff --git a/lib/utils/ttl_cache.dart b/lib/utils/ttl_cache.dart index 199c8f16..361bc2b9 100644 --- a/lib/utils/ttl_cache.dart +++ b/lib/utils/ttl_cache.dart @@ -1,22 +1,56 @@ -/// Simple in-memory cache where each entry expires after [ttl]. +import 'dart:collection'; + +/// Bounded in-memory LRU cache where each entry expires after [ttl]. class TtlCache { final Duration ttl; - final Map> _entries = {}; + final int maxEntries; + final LinkedHashMap> _entries = + LinkedHashMap>(); - TtlCache(this.ttl); + TtlCache(this.ttl, {this.maxEntries = 100}) { + if (maxEntries <= 0) { + throw ArgumentError.value( + maxEntries, + 'maxEntries', + 'must be greater than zero', + ); + } + } T? get(String key) { + _removeExpired(); final entry = _entries[key]; if (entry == null) return null; - if (DateTime.now().isAfter(entry.expiresAt)) { - _entries.remove(key); - return null; - } + // LinkedHashMap preserves insertion order, so reinserting makes this the + // most recently used entry. + _entries + ..remove(key) + ..[key] = entry; return entry.value; } void set(String key, T value) { + _removeExpired(); + _entries.remove(key); _entries[key] = _TtlEntry(value, DateTime.now().add(ttl)); + while (_entries.length > maxEntries) { + _entries.remove(_entries.keys.first); + } + } + + void remove(String key) => _entries.remove(key); + + void clear() => _entries.clear(); + + int get length { + _removeExpired(); + return _entries.length; + } + + void _removeExpired() { + if (_entries.isEmpty) return; + final now = DateTime.now(); + _entries.removeWhere((_, entry) => !now.isBefore(entry.expiresAt)); } } diff --git a/lib/widgets/duplicate_review_sheet.dart b/lib/widgets/duplicate_review_sheet.dart index e056b184..5686d1eb 100644 --- a/lib/widgets/duplicate_review_sheet.dart +++ b/lib/widgets/duplicate_review_sheet.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter/material.dart'; import 'package:spotiflac_android/widgets/app_bottom_sheet.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; @@ -36,19 +38,21 @@ class DuplicateReviewSheet extends ConsumerStatefulWidget { } class _DuplicateReviewSheetState extends ConsumerState { + late final LocalLibraryNotifier _localLibraryNotifier; late Future> _groupsFuture; bool _deletedLocalRows = false; @override void initState() { super.initState(); + _localLibraryNotifier = ref.read(localLibraryProvider.notifier); _groupsFuture = LibraryDatabase.instance.findIsrcDuplicateGroups(); } @override void dispose() { if (_deletedLocalRows) { - ref.read(localLibraryProvider.notifier).reloadFromStorage(); + unawaited(_localLibraryNotifier.reloadFromStorage()); } super.dispose(); } diff --git a/test/ttl_cache_test.dart b/test/ttl_cache_test.dart new file mode 100644 index 00000000..2c764624 --- /dev/null +++ b/test/ttl_cache_test.dart @@ -0,0 +1,37 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:spotiflac_android/utils/ttl_cache.dart'; + +void main() { + test('TtlCache evicts the least recently used entry at its bound', () { + final cache = TtlCache(const Duration(minutes: 1), maxEntries: 2); + + cache.set('first', 1); + cache.set('second', 2); + expect(cache.get('first'), 1); + cache.set('third', 3); + + expect(cache.get('first'), 1); + expect(cache.get('second'), isNull); + expect(cache.get('third'), 3); + expect(cache.length, 2); + }); + + test('TtlCache sweeps expired keys even when another key is accessed', () { + final cache = TtlCache(Duration.zero, maxEntries: 3); + cache.set('expired', 1); + + expect(cache.get('different-key'), isNull); + expect(cache.length, 0); + }); + + test('TtlCache rejects a non-positive bound', () { + expect( + () => TtlCache(const Duration(minutes: 1), maxEntries: 0), + throwsArgumentError, + ); + expect( + () => TtlCache(const Duration(minutes: 1), maxEntries: -1), + throwsArgumentError, + ); + }); +}