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

@@ -702,6 +702,21 @@ TrackPointer TrackDAO::addTracksAddFile(const QFileInfo& fileInfo, bool unremove
         qDebug() << "TrackDAO::addTracksAddFile:"
                 << "Track has already been added to the database"
                 << oldTrackId;
+        DEBUG_ASSERT(pTrack->getDateAdded().isValid());
+        const auto trackLocation = pTrack->getLocation();
+        // TODO: These duplicates are only detected by chance when
+        // the other track is currently cached. Instead file aliasing
+        // must be detected reliably in any situation.
+        if (fileInfo.absoluteFilePath() != trackLocation) {
+            kLogger.warning()
+                    << "Both track locations"
+                    << fileInfo.absoluteFilePath()
+                    << "and"
+                    << trackLocation
+                    << "are referencing the same file"
+                    << fileInfo.canonicalFilePath();
+            return TrackPointer();
+        }
         return pTrack;
     }
     // Keep the GlobalTrackCache locked until the id of the Track
@@ -1222,20 +1237,32 @@ TrackPointer TrackDAO::getTrackFromDB(TrackId trackId) const {
     const QString trackLocation(queryRecord.value(0).toString());
 
     GlobalTrackCacheResolver cacheResolver(QFileInfo(trackLocation), trackId);
-    TrackPointer pTrack = cacheResolver.getTrack();
-    VERIFY_OR_DEBUG_ASSERT(pTrack) {
-        // Just to be safe, but this should never happen!!
-        return pTrack;
-    }
-    DEBUG_ASSERT(pTrack->getId() == trackId);
-    if (cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::HIT) {
+    pTrack = cacheResolver.getTrack();
+    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
         // the operation and simply return the already cached Track
         // object which is up-to-date.
+        DEBUG_ASSERT(pTrack);
+        return pTrack;
+    }
+    if (cacheResolver.getLookupResult() ==
+            GlobalTrackCacheLookupResult::ConflictCanonicalLocation) {
+        // Reject requests that would otherwise cause a caching caching conflict
+        // by accessing the same, physical file from multiple tracks concurrently.
+        DEBUG_ASSERT(!pTrack);
+        DEBUG_ASSERT(cacheResolver.getTrackRef().hasId());
+        DEBUG_ASSERT(cacheResolver.getTrackRef().hasCanonicalLocation());
+        kLogger.warning()
+                << "Failed to load track with id"
+                << trackId
+                << "that is referencing the same file"
+                << cacheResolver.getTrackRef().getCanonicalLocation()
+                << "as the cached track with id"
+                << cacheResolver.getTrackRef().getId();
         return pTrack;
     }
-    DEBUG_ASSERT(cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::MISS);
+    DEBUG_ASSERT(cacheResolver.getLookupResult() == GlobalTrackCacheLookupResult::Miss);
     // The cache will immediately be unlocked to reduce lock contention!
     cacheResolver.unlockCache();
 

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(
@@ -148,7 +175,7 @@ TrackPointer GlobalTrackCacheLocker::lookupTrackByRef(
 GlobalTrackCacheResolver::GlobalTrackCacheResolver(
         QFileInfo fileInfo,
         SecurityTokenPointer pSecurityToken)
-        : m_lookupResult(GlobalTrackCacheLookupResult::NONE) {
+        : m_lookupResult(GlobalTrackCacheLookupResult::None) {
     DEBUG_ASSERT(m_pInstance);
     m_pInstance->resolve(this, std::move(fileInfo), TrackId(), std::move(pSecurityToken));
 }
@@ -157,7 +184,7 @@ GlobalTrackCacheResolver::GlobalTrackCacheResolver(
         QFileInfo fileInfo,
         TrackId trackId,
         SecurityTokenPointer pSecurityToken)
-        : m_lookupResult(GlobalTrackCacheLookupResult::NONE) {
+        : m_lookupResult(GlobalTrackCacheLookupResult::None) {
     DEBUG_ASSERT(m_pInstance);
     m_pInstance->resolve(this, std::move(fileInfo), std::move(trackId), std::move(pSecurityToken));
 }
@@ -167,7 +194,7 @@ void GlobalTrackCacheResolver::initLookupResult(
         TrackPointer&& strongPtr,
         TrackRef&& trackRef) {
     DEBUG_ASSERT(m_pInstance);
-    DEBUG_ASSERT(GlobalTrackCacheLookupResult::NONE == m_lookupResult);
+    DEBUG_ASSERT(GlobalTrackCacheLookupResult::None == m_lookupResult);
     DEBUG_ASSERT(!m_strongPtr);
     m_lookupResult = lookupResult;
     m_strongPtr = std::move(strongPtr);
@@ -176,7 +203,7 @@ void GlobalTrackCacheResolver::initLookupResult(
 
 void GlobalTrackCacheResolver::initTrackIdAndUnlockCache(TrackId trackId) {
     DEBUG_ASSERT(m_pInstance);
-    DEBUG_ASSERT(GlobalTrackCacheLookupResult::NONE != m_lookupResult);
+    DEBUG_ASSERT(GlobalTrackCacheLookupResult::None != m_lookupResult);
     DEBUG_ASSERT(m_strongPtr);
     DEBUG_ASSERT(trackId.isValid());
     if (m_trackRef.getId().isValid()) {
@@ -391,8 +418,32 @@ 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) {
+            const auto cachedTrackRef =
+                    validateAndCanonicalizeRequestedTrackRef(trackRef, *trackPtr);
+            // Multiple tracks may reference the same physical file on disk
+            if (!trackRef.hasId() || trackRef.getId() == cachedTrackRef.getId()) {
+                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 +453,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(
@@ -499,7 +548,7 @@ void GlobalTrackCache::resolve(
             }
             TrackRef trackRef = createTrackRef(*strongPtr);
             pCacheResolver->initLookupResult(
-                    GlobalTrackCacheLookupResult::HIT,
+                    GlobalTrackCacheLookupResult::Hit,
                     std::move(strongPtr),
                     std::move(trackRef));
             return;
@@ -515,7 +564,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,10 +574,21 @@ void GlobalTrackCache::resolve(
                         << trackRef.getCanonicalLocation()
                         << strongPtr.get();
             }
-            pCacheResolver->initLookupResult(
-                    GlobalTrackCacheLookupResult::HIT,
-                    std::move(strongPtr),
-                    std::move(trackRef));
+            auto cachedTrackRef = validateAndCanonicalizeRequestedTrackRef(
+                    trackRef,
+                    *strongPtr);
+            // Multiple tracks may reference the same physical file on disk
+            if (!trackRef.hasId() || trackRef.getId() == cachedTrackRef.getId()) {
+                pCacheResolver->initLookupResult(
+                        GlobalTrackCacheLookupResult::Hit,
+                        std::move(strongPtr),
+                        std::move(trackRef));
+            } else {
+                pCacheResolver->initLookupResult(
+                        GlobalTrackCacheLookupResult::ConflictCanonicalLocation,
+                        TrackPointer(),
+                        std::move(cachedTrackRef));
+            }
             return;
         }
     }
@@ -591,7 +652,7 @@ void GlobalTrackCache::resolve(
     savingPtr->moveToThread(QApplication::instance()->thread());
 
     pCacheResolver->initLookupResult(
-            GlobalTrackCacheLookupResult::MISS,
+            GlobalTrackCacheLookupResult::Miss,
             std::move(savingPtr),
             std::move(trackRef));
 }

src/track/globaltrackcache.h

@@ -12,9 +12,10 @@
 class GlobalTrackCache;
 
 enum class GlobalTrackCacheLookupResult {
-    NONE,
-    HIT,
-    MISS
+    None,
+    Hit,
+    Miss,
+    ConflictCanonicalLocation
 };
 
 // Find the updated location of a track in the database when
@@ -228,6 +229,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()
+            << '}';
 }