TrackDAO/GlobalTrackCache: Handle file aliasing

CHANGELOG

@@ -3,6 +3,7 @@
 * Add controller mapping for Hercules DJControl Inpulse 200 #2542
 * Add controller mapping for Hercules DJControl Jogvision #2370
 * Fix missing manual in deb package lp:1889776
+* Fix caching of duplicate tracks that reference the same file #3027
 
 ==== 2.2.4 2020-05-10 ====
 

src/library/dao/trackdao.cpp

@@ -1227,7 +1227,22 @@ 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) {
+        // The cacheResolver() failed to resolve the track by it's trackId,
+        // but has found a different track that has already been loaded and
+        // is stored in the cache. That track references 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 and
+        // access each file once at a time to prevent corruption due to
+        // concurrent, non-exclusive (write) access.
+        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

@@ -34,6 +34,33 @@ TrackRef createTrackRef(const Track& track) {
     return TrackRef::fromFileInfo(track.getFileInfo(), track.getId());
 }
 
+TrackRef validateAndCanonicalizeRequestedTrackRef(
+        const TrackRef requestedTrackRef,
+        const Track& cachedTrack) {
+    const auto cachedTrackRef = createTrackRef(cachedTrack);
+    // If an id has been provided the caller expects that if a track
+    // is found it is supposed to have the exact same id. This cannot
+    // be guaranteed due to file system aliasing.
+    // The found track may or may not have a valid id.
+    if (requestedTrackRef.hasId() &&
+            requestedTrackRef.getId() != cachedTrackRef.getId()) {
+        DEBUG_ASSERT(
+                requestedTrackRef.getLocation() !=
+                cachedTrackRef.getLocation());
+        DEBUG_ASSERT(
+                requestedTrackRef.getCanonicalLocation() ==
+                cachedTrackRef.getCanonicalLocation());
+        kLogger.warning()
+                << "Found a different track for the same canonical location:"
+                << "requested =" << requestedTrackRef
+                << "cached =" << cachedTrackRef;
+        return cachedTrackRef;
+    } else {
+        // Regular case, i.e. no aliasing
+        return requestedTrackRef;
+    }
+}
+
 class EvictAndSaveFunctor {
   public:
     explicit EvictAndSaveFunctor(
@@ -391,8 +418,28 @@ 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) {
+            validateAndCanonicalizeRequestedTrackRef(trackRef, *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 +449,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 +560,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()) {
@@ -524,6 +570,10 @@ void GlobalTrackCache::resolve(
                         << trackRef.getCanonicalLocation()
                         << strongPtr.get();
             }
+            // Replace requested with cached TrackRef to prevent inconcistencies
+            trackRef = validateAndCanonicalizeRequestedTrackRef(
+                    trackRef,
+                    *strongPtr);
             pCacheResolver->initLookupResult(
                     GlobalTrackCacheLookupResult::HIT,
                     std::move(strongPtr),

src/track/globaltrackcache.h

@@ -228,6 +228,13 @@ private slots:
 
     TrackPointer lookupById(
             const TrackId& trackId);
+    TrackPointer lookupByCanonicalLocation(
+            const QString& canonicalLocation);
+
+    /// Lookup the track either by id (primary) or by
+    /// canonical location (secondary). The id of the
+    /// returned track might differ from the requested
+    /// id due to file system aliasing!!
     TrackPointer lookupByRef(
             const TrackRef& trackRef);
 

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()
+            << '}';
 }