TrackDAO/GlobalTrackCache: Handle file aliasing

src/library/dao/trackdao.cpp

@@ -1227,7 +1227,20 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const {
         // Just to be safe, but this should never happen!!
         return pTrack;
     }
-    DEBUG_ASSERT(pTrack->getId() == trackId);
+    if (pTrack->getId() != trackId) {
+        // This happens if two different tracks are referencing
+        // the same (physical) file on the file system!! Due to
+        // symbolic links different locations may resolve to the
+        // same canonical location. We can only load each file
+        // once at a time.
+        kLogger.warning()
+                << "Returning already loaded track"
+                << pTrack->getId()
+                << "instead of"
+                << trackId
+                << "with the same canonical file location"
+                << pTrack->getFileInfo().canonicalFilePath();
+    }
     if (cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::HIT) {
         // Due to race conditions the track might have been reloaded
         // from the database in the meantime. In this case we abort

src/track/globaltrackcache.cpp

@@ -391,8 +391,34 @@ bool GlobalTrackCache::isEmpty() const {
     return m_tracksById.empty() && m_tracksByCanonicalLocation.empty();
 }
 
+TrackPointer GlobalTrackCache::lookupByRef(
+        const TrackRef& trackRef) {
+    TrackPointer trackPtr;
+    if (trackRef.hasId()) {
+        trackPtr = lookupById(trackRef.getId());
+        if (trackPtr) {
+            return trackPtr;
+        }
+    }
+    if (trackRef.hasCanonicalLocation()) {
+        trackPtr = lookupByCanonicalLocation(trackRef.getCanonicalLocation());
+        if (trackPtr) {
+            if (trackRef.hasId() &&
+                    trackRef.getId() != trackPtr->getId()) {
+                kLogger.warning()
+                        << "Found a different track with the same canonical location:"
+                        << "expected =" << trackRef
+                        << "actual =" << createTrackRef(*trackPtr);
+            }
+            return trackPtr;
+        }
+    }
+    return trackPtr;
+}
+
 TrackPointer GlobalTrackCache::lookupById(
         const TrackId& trackId) {
+    TrackPointer trackPtr;
     const auto trackById(m_tracksById.find(trackId));
     if (m_tracksById.end() != trackById) {
         // Cache hit
@@ -402,45 +428,43 @@ TrackPointer GlobalTrackCache::lookupById(
                     << trackId
                     << trackById->second->getPlainPtr();
         }
-        return revive(trackById->second);
+        trackPtr = revive(trackById->second);
+        DEBUG_ASSERT(trackPtr);
     } else {
         // Cache miss
         if (traceLogEnabled()) {
             kLogger.trace()
                     << "Cache miss for"
                     << trackId;
         }
-        return TrackPointer();
     }
+    return trackPtr;
 }
 
-TrackPointer GlobalTrackCache::lookupByRef(
-        const TrackRef& trackRef) {
-    if (trackRef.hasId()) {
-        return lookupById(trackRef.getId());
+TrackPointer GlobalTrackCache::lookupByCanonicalLocation(
+        const QString& canonicalLocation) {
+    TrackPointer trackPtr;
+    const auto trackByCanonicalLocation(
+            m_tracksByCanonicalLocation.find(canonicalLocation));
+    if (m_tracksByCanonicalLocation.end() != trackByCanonicalLocation) {
+        // Cache hit
+        if (traceLogEnabled()) {
+            kLogger.trace()
+                    << "Cache hit for"
+                    << canonicalLocation
+                    << trackByCanonicalLocation->second->getPlainPtr();
+        }
+        trackPtr = revive(trackByCanonicalLocation->second);
+        DEBUG_ASSERT(trackPtr);
     } else {
-        const auto canonicalLocation = trackRef.getCanonicalLocation();
-        const auto trackByCanonicalLocation(
-                m_tracksByCanonicalLocation.find(canonicalLocation));
-        if (m_tracksByCanonicalLocation.end() != trackByCanonicalLocation) {
-            // Cache hit
-            if (traceLogEnabled()) {
-                kLogger.trace()
-                        << "Cache hit for"
-                        << canonicalLocation
-                        << trackByCanonicalLocation->second->getPlainPtr();
-            }
-            return revive(trackByCanonicalLocation->second);
-        } else {
-            // Cache miss
-            if (traceLogEnabled()) {
-                kLogger.trace()
-                        << "Cache miss for"
-                        << canonicalLocation;
-            }
-            return TrackPointer();
+        // Cache miss
+        if (traceLogEnabled()) {
+            kLogger.trace()
+                    << "Cache miss for"
+                    << canonicalLocation;
         }
     }
+    return trackPtr;
 }
 
 TrackPointer GlobalTrackCache::revive(
@@ -515,7 +539,8 @@ void GlobalTrackCache::resolve(
                     << "Resolving track by canonical location"
                     << trackRef.getCanonicalLocation();
         }
-        auto strongPtr = lookupByRef(trackRef);
+        auto strongPtr = lookupByCanonicalLocation(
+                trackRef.getCanonicalLocation());
         if (strongPtr) {
             // Cache hit
             if (debugLogEnabled()) {

src/track/globaltrackcache.h

@@ -226,10 +226,12 @@ private slots:
     void relocateTracks(
             GlobalTrackCacheRelocator* /*nullable*/ pRelocator);
 
-    TrackPointer lookupById(
-            const TrackId& trackId);
     TrackPointer lookupByRef(
             const TrackRef& trackRef);
+    TrackPointer lookupById(
+            const TrackId& trackId);
+    TrackPointer lookupByCanonicalLocation(
+            const QString& canonicalLocation);
 
     TrackPointer revive(GlobalTrackCacheEntryPointer entryPtr);
 

src/track/trackref.cpp

@@ -1,5 +1,6 @@
 #include "track/trackref.h"
 
+#include <QDebugStateSaver>
 
 bool TrackRef::verifyConsistency() const {
     // Class invariant: The location can only be set together with
@@ -16,17 +17,25 @@ bool TrackRef::verifyConsistency() const {
 }
 
 std::ostream& operator<<(std::ostream& os, const TrackRef& trackRef) {
-    return os << '[' << trackRef.getLocation().toStdString()
-            << " | " << trackRef.getCanonicalLocation().toStdString()
-            << " | " << trackRef.getId()
-            << ']';
-
+    return os
+            << "TrackRef{"
+            << trackRef.getLocation().toStdString()
+            << ','
+            << trackRef.getCanonicalLocation().toStdString()
+            << ','
+            << trackRef.getId()
+            << '}';
 }
 
-QDebug operator<<(QDebug debug, const TrackRef& trackRef) {
-    debug.nospace() << '[' << trackRef.getLocation()
-                    << " | " << trackRef.getCanonicalLocation()
-                    << " | " << trackRef.getId()
-                    << ']';
-    return debug.space();
+QDebug operator<<(QDebug dbg, const TrackRef& trackRef) {
+    const QDebugStateSaver saver(dbg);
+    dbg = dbg.maybeSpace() << "TrackRef";
+    return dbg.nospace()
+            << '{'
+            << trackRef.getLocation()
+            << ','
+            << trackRef.getCanonicalLocation()
+            << ','
+            << trackRef.getId()
+            << '}';
 }