TrackDAO/GlobalTrackCache: Handle file aliasing #3027
Conversation
|
I'll try to review this tomorrow. Can you add a changelog entry? |
|
CI failures are unrelated. Rescanning a library with a single, physical file and multiple aliases (= symbolic links, both for (sub-)directories and files) now works flawlessly. The files are added one after another by subsequent library scans as expected. We cannot store canonical file paths in the database, because those are not supposed to be stable and persistent. Mount points may change over time. This is the reason why aliased files could not reliably be detected as duplicates without accessing all existing files individually. |
There was a problem hiding this comment.
I am afraid we need to fail the lookup in this case, as if the track is not existing.
This case is well tested in the using code, but returning a track with a different ID is not considered and will lead to hard to explain follow up issue.
The ASSERT was there for a good reason, it is deeply assumed everywhere.
For example in AutoDJProcessor::getNextTrackFromQueue() the track is loaded with a different ID.
AutoDJProcessor::removeTrackFromTopOfQueue considered this track as manually loaded and does not remove the track. Result, the same track is played over and over again.
The track will never be anlyzed but the result is not stored in AnalysisFeature::analyzeTracks
The play counter of the Track will not be increase, the preview button will not light up the BPM lock will be bypassed ..... to be continued
I actually think that this situation is a faulty state, we should inform him about this bad state and just fail.
Mixxx is not ready to graceful deal with this situation.
|
Not possible. If we fail the lookup a different track will be loaded for the same file resulting in potential file corruption when exporting tracks. Caching the 2nd track would also fail, because we cannot cache 2 different tracks with the same canonical location. |
src/library/dao/trackdao.cpp
Outdated
| << "instead of" | ||
| << trackId | ||
| << "with the same canonical file location" | ||
| << pTrack->getFileInfo().canonicalFilePath(); |
There was a problem hiding this comment.
I think we can safely return return TrackPointer(); here. All using code need to handle that anyway.
I have found three issue we need urgently fixed anyway:
AutoDJFeature::slotAddRandomTrack() has a debug assert that is wrong
void TrackExportWorker::run(), CrateFeature::slotExportTrackFiles() and SetlogFeature::slotJoinWithPrevious() are also missing the null check.
I mean returning a null track as if the track is not available.
Yes, that's why I consider the library as faulty. No need to workaround in Mixxx, just fail. |
|
For testing purposes: dumb_music_folder.tar.gz |
|
@daschuer Ping |
Replaces #3026
Topic on Zulip: DEBUG_ASSDERT in GlobalTrackCache::resolve
Two tracks with different locations but the same canonical locations cause a debug assertion. This is possible by creating a symbolic link to an existing track in the music folder and rescanning the library. Both tracks are now shown in the library, but they refer to the same file.
Only one of those tracks could be loaded at the same time!! This might cause some inconvenience and inconsistencies in the UI when editing the track properties, but it prevents file corruption when exporting file tags.
Not sure if this is a critical bug. Backport to 2.2? I did not check if this is feasible.