Fix SMB playback and library scan reliability

CHANGELOG.md

@@ -23,6 +23,8 @@
 - **ISO completion diagnostics** — strict per-packet validation now records a bounded snapshot of packet lengths, transfer slots, and the selected wire format on the first mismatch, without accepting packets more loosely. This is diagnostic groundwork for #354, not a compatibility workaround.
 
 ### Network & SMB
+- **SMB sync resilience** — an unreadable or vanished child folder no longer aborts the whole library scan; directory loops are depth-capped and duplicate paths are skipped. Folder art larger than the server's single-READ limit now loads through the pipelined reader.
+- **SMB playback robustness** — a failing progress callback can no longer cancel a shared transfer or start a duplicate download, cache accounting failures no longer discard a completed file, a disappearing cache entry is treated as a miss, a changed remote file invalidates its cached bytes on resync, and a failed next-track download reports cleanly instead of throwing in the background.
 - **Atomic SMB downloads** — playback, next-track prefetch, and gapless preparation for the same file share one transfer. Downloads publish only after the native completion event, a consistent byte count, and a matching on-disk length; short, cancelled, and incomplete transfers delete their staging file and stay retryable.
 - **Cache safety** — SMB entries move to a new `smb-v2:` key so files left by older builds are bypassed without clearing app data, and staging and `.part` files are excluded from lookup, eviction, and the cache cap.
 - **Privacy-safe failures** — SMB errors resolve to allowlisted stage/code pairs and localized copy; hostnames, account names, shares, and paths are never echoed into logs or banners. URL parsing decodes escapes once and rejects embedded credentials, unsupported schemes, query/fragment data, and out-of-range ports; Test Connection probes the configured root.

docs/SMB_VALIDATION.md

@@ -7,13 +7,26 @@ guest fallback, or change to signing/encryption policy. Test Connection now open
 the share and lists the configured root folder once; it does not guarantee read
 permission for every file or successful decoding/output.
 
+Library sync treats the configured root as mandatory but skips an unreadable or
+vanished child directory (common on Windows shares: `System Volume
+Information`, per-user folders, or a folder deleted mid-scan) instead of
+aborting the whole sync. Directory identities are de-duplicated and recursion is
+depth-capped so junction/DFS loops cannot run forever. Cover-art reads use the
+pipelined reader, so images larger than the server's single-READ limit still
+load.
+
 Downloads use a same-directory hidden staging file, verify the server's byte
 count, then close and atomically rename it. Dart also requires the completion
 event and checks the published file length. Playback/prefetch for the same
-server/file share one download. Older SMB cache entries are bypassed using a new
-SMB-only cache key, without clearing app data or other protocols' cache entries.
-Normal errors/cancellation clean up staging files; force-killing the process can
-leave an unused hidden staging file, which is never considered playable.
+server/file share one download. A resync invalidates cached bytes when the
+listed file size changes, so a replaced file cannot keep playing from the old
+cache. Older SMB cache entries are bypassed using a new SMB-only cache key,
+without clearing app data or other protocols' cache entries. Normal
+errors/cancellation clean up staging files; force-killing the process can leave
+an unused hidden staging file, which is never considered playable. Progress
+listeners are isolated from the transfer, so a failing UI callback cannot cancel
+a download other callers are waiting on, and the shared-transfer entry is
+released when the transfer finishes rather than when its first waiter leaves.
 
 Listing-only songs can still show `00:00` before their first download. After a
 successful download, metadata extraction asynchronously fills missing duration,
@@ -33,7 +46,8 @@ flutter test
 Tests cover interrupted/short downloads and retry, absent/invalid completion,
 on-disk length validation, same-key concurrency with progress, original bytes,
 legacy-cache bypass, LRU accounting, staging-file exclusion, URL encoding,
-configured-root probing, safe diagnostics and metadata preservation.
+configured-root probing, safe diagnostics, metadata preservation, partial
+directory walks, and progress-listener isolation.
 
 ## Real-device checklist (not replaced by unit tests)
 
@@ -47,7 +61,9 @@ posting addresses, share names or credentials.
    in their separate fields, never in the URL.
 2. Confirm correct credentials work. Wrong credentials, denied shares, missing
    roots and denied file reads should fail at the corresponding stage. A readable
-   root with a denied track can legitimately pass Test Connection.
+   root with a denied track can legitimately pass Test Connection. A denied
+   child folder should be skipped during sync while sibling folders still
+   import; a denied configured root must still fail the sync.
 3. Play uncached FLAC/MP3 and a large hi-res/DSD original supported by the selected
    output. Compare the same file played locally in Flick. Confirm duration/format
    update after caching, and remain after library sync.

lib/services/network_cache_service.dart

@@ -58,9 +58,16 @@ class NetworkCacheService {
     final file = File(p.join(serverDir.path, fileName));
     if (!await file.exists()) return null;
 
+    // The file can be evicted (or removed by Android's cache manager) between
+    // the existence check and the LRU touch. Treat that as a miss so callers
+    // re-download instead of handing playback a path that no longer exists.
     final now = DateTime.now();
-    if (file.lastModifiedSync() != now) {
-      await file.setLastModified(now);
+    try {
+      if (file.lastModifiedSync() != now) {
+        await file.setLastModified(now);
+      }
+    } on FileSystemException {
+      return null;
     }
     return file.path;
   }
@@ -123,6 +130,28 @@ class NetworkCacheService {
   Future<void> completeDownload(String path) =>
       _evictIfOverCap(protect: File(path));
 
+  /// Drop cached copies for (server, remoteId), across all extensions. Used
+  /// when a remote file changes so the next play cannot reuse stale bytes.
+  /// Staging files are left alone: they belong to an in-flight downloader and
+  /// are never lookup candidates.
+  Future<void> remove(int remoteServerId, String remoteId) async {
+    final serverDir = Directory(
+      p.join((await _root()).path, '$remoteServerId'),
+    );
+    if (!await serverDir.exists()) return;
+
+    final prefix = '${_hash(remoteServerId, remoteId)}.';
+    await for (final entry in serverDir.list()) {
+      if (entry is! File) continue;
+      if (!p.basename(entry.path).startsWith(prefix)) continue;
+      try {
+        await entry.delete();
+      } on FileSystemException {
+        /* Another caller may have removed or locked it; best effort. */
+      }
+    }
+  }
+
   // ponytail: full O(n) size scan per eviction; track running totals only
   // if a server with a huge cache ever shows up on profile.
   Future<void> _evictIfOverCap({File? protect}) async {

lib/services/player_service.dart

@@ -5606,7 +5606,7 @@ class PlayerService {
       }
     }
 
-    final nextPath = await _resolveRustPath(nextSong);
+    final nextPath = await _resolveGaplessNextPath(nextSong);
     if (nextPath != null) {
       await _rustAudioService.setReplayGainDefault(
         await _computeReplayGainDbFor(nextSong),
@@ -5630,6 +5630,26 @@ class PlayerService {
     }
   }
 
+  /// Resolve the next local path without letting an unawaited gapless
+  /// preparation task throw. Network sources (notably SMB) download on this
+  /// path; a failed transfer must leave the current track untouched and be
+  /// reported like any other SMB playback failure.
+  Future<String?> _resolveGaplessNextPath(Song nextSong) async {
+    try {
+      return await _resolveRustPath(nextSong);
+    } on SmbFailure catch (failure) {
+      _debugLog(
+        '[crossfade] next-track SMB prepare failed: '
+        '${failure.stage}/${failure.code}',
+      );
+      _reportSmbPlaybackFailure(failure);
+      return null;
+    } catch (e) {
+      _debugLog('[crossfade] next-track path resolve failed: $e');
+      return null;
+    }
+  }
+
   /// Resolves the next song for the just_audio crossfade engine. Mirrors the
   /// linear-next rule used by the Rust gapless path (shuffle is honoured
   /// because [_playlist] is pre-shuffled at play time). Returns null when there

lib/services/sources/smb_service.dart

@@ -276,13 +276,33 @@ class SmbService implements NetworkSourceService {
     final key = (server.id, remoteId);
     final job = _downloads.putIfAbsent(key, () => _SmbDownload());
     if (onProgress != null) job.listeners.add(onProgress);
-    job.future ??= _download(server, remoteId, extension, job);
+    // Tie map cleanup to the transfer itself, not to whichever caller happens
+    // to stop waiting first. A caller can bail out (e.g. a UI progress
+    // callback throws, or the screen is disposed) while the shared transfer is
+    // still running; dropping the map entry there would let the next caller
+    // start a second download for the same file.
+    job.future ??= _download(server, remoteId, extension, job).whenComplete(() {
+      if (identical(_downloads[key], job)) _downloads.remove(key);
+    });
     try {
-      if (onProgress != null && job.progress != null) onProgress(job.progress!);
+      final progress = job.progress;
+      if (onProgress != null && progress != null) {
+        _notifyProgress(onProgress, progress);
+      }
       return await job.future!;
     } finally {
       if (onProgress != null) job.listeners.remove(onProgress);
-      if (identical(_downloads[key], job)) _downloads.remove(key);
+    }
+  }
+
+  /// Progress callbacks belong to the UI, never to the transfer. Letting one
+  /// throw would abort the shared download and delete a file other callers are
+  /// waiting on.
+  void _notifyProgress(void Function(double) listener, double progress) {
+    try {
+      listener(progress);
+    } catch (_) {
+      /* A broken progress listener must not fail playback. */
     }
   }
 
@@ -295,6 +315,9 @@ class SmbService implements NetworkSourceService {
     // Old versions wrote interrupted transfers to the final filename. A new
     // SMB-only key bypasses those entries without wiping other protocols/data.
     final cacheId = 'smb-v2:$remoteId';
+    // Callers may omit the extension (prefetch) or pass a different case.
+    // Derive it from the remote name so every caller resolves the same file.
+    final cacheExtension = _cacheExtension(extension, remoteId);
     String? destPath;
     final clock = Stopwatch()..start();
     var received = 0;
@@ -303,11 +326,15 @@ class SmbService implements NetworkSourceService {
       final cached = await _cache.getPath(
         server.id,
         cacheId,
-        extension: extension,
+        extension: cacheExtension,
       );
       if (cached != null) return cached;
       final s = _parseServer(server);
-      destPath = await _cache.pathFor(server.id, cacheId, extension: extension);
+      destPath = await _cache.pathFor(
+        server.id,
+        cacheId,
+        extension: cacheExtension,
+      );
       var complete = false;
       await for (final p in _downloader(
         host: s.host,
@@ -327,14 +354,21 @@ class SmbService implements NetworkSourceService {
         if (total > 0) {
           job.progress = received / total;
           for (final listener in List.of(job.listeners)) {
-            listener(job.progress!);
+            _notifyProgress(listener, job.progress!);
           }
         }
       }
       if (!complete || await File(destPath).length() != total) {
         throw const SmbFailure('download', 'incomplete');
       }
-      await _cache.completeDownload(destPath);
+      // The download is already complete and playable. Cache accounting is
+      // best-effort: a failure to evict old entries must not turn a successful
+      // transfer into a failed playback.
+      try {
+        await _cache.completeDownload(destPath);
+      } catch (_) {
+        AppLog.instance.add('SMB stage=cache code=storage result=failed');
+      }
       AppLog.instance.add(
         'SMB stage=download result=ok received=$received total=$total elapsed_ms=${clock.elapsedMilliseconds}',
       );
@@ -373,6 +407,15 @@ class SmbService implements NetworkSourceService {
     final user = server.username ?? '';
     final pass = _password(server);
 
+    // Remember the sizes of already-listed rows so a replaced file invalidates
+    // its cached bytes as well as its metadata. SMB listings expose size only.
+    final previousSizes = <String, int>{};
+    for (final song in await _repo.getSongsByRemoteServer(server.id)) {
+      final remoteId = song.remoteId;
+      final size = song.fileSize;
+      if (remoteId != null && size != null) previousSizes[remoteId] = size;
+    }
+
     // Collect file entries grouped by parent directory (for per-folder
     // cover/audio classification), yielding progress as the walk streams in.
     final byDir = <String, List<SmbEntry>>{};
@@ -430,6 +473,20 @@ class SmbService implements NetworkSourceService {
             ),
           )
           .toList();
+      // Drop bytes cached for a file whose size changed so playback cannot
+      // serve the previous version. Do this before persisting the new size:
+      // if the process stops in between, the next sync still sees the old
+      // listing size and repeats the cleanup instead of accepting stale bytes.
+      for (final e in songEntries) {
+        final previous = previousSizes[e.path];
+        if (previous != null && previous != e.size.toInt()) {
+          try {
+            await _cache.remove(server.id, 'smb-v2:${e.path}');
+          } catch (_) {
+            AppLog.instance.add('SMB stage=cache code=storage result=failed');
+          }
+        }
+      }
       await _repo.upsertSmbSongs(entities);
       syncedRemoteIds.addAll(entities.map((e) => e.remoteId!));
       songsFound += entities.length;
@@ -498,10 +555,34 @@ class SmbService implements NetworkSourceService {
 
   String? _extension(String name) {
     final dot = name.lastIndexOf('.');
-    if (dot <= 0) return null;
+    if (dot <= 0 || dot == name.length - 1) return null;
     return name.substring(dot + 1).toLowerCase();
   }
 
+  /// Prefer the caller's extension, but normalize it and fall back to the
+  /// remote filename. This keeps `.bin` out of the cache when a caller omits
+  /// the hint, and lets callers that pass `FLAC` share the file cached as
+  /// `flac` instead of downloading it twice.
+  String? _cacheExtension(String? extension, String remoteId) {
+    String? clean(String? value) {
+      var candidate = value?.trim().toLowerCase();
+      if (candidate == null) return null;
+      candidate = candidate.replaceFirst(RegExp(r'^\.+'), '');
+      if (candidate.isEmpty ||
+          candidate == 'unknown' ||
+          !RegExp(r'^[a-z0-9]+$').hasMatch(candidate)) {
+        return null;
+      }
+      return candidate;
+    }
+
+    final provided = clean(extension);
+    if (provided != null) return provided;
+    final slash = remoteId.lastIndexOf('/');
+    final name = slash >= 0 ? remoteId.substring(slash + 1) : remoteId;
+    return clean(_extension(name));
+  }
+
   String _lastPathSegment(String path) {
     var p = path;
     while (p.endsWith('/') && p.length > 1) {