TrackDAO/GlobalTrackCache: Handle file aliasing #3026
Conversation
I don't consider this as critical, because we need many exceptional cases at once to actually loose data. |
|
The backport requires a few, minor changes. I will substitute this branch. After merging we need to resolve some conflicts when merging into 2.3. |
|
I will open a new PR to trigger the CI builds. |
| if (trackRef.hasCanonicalLocation()) { | ||
| trackPtr = lookupByCanonicalLocation(trackRef.getCanonicalLocation()); | ||
| if (trackPtr) { | ||
| if (trackRef.hasId() && |
There was a problem hiding this comment.
Can this ever be true because of the check in line 426?
There was a problem hiding this comment.
If another track with a different id but the same canonical location has already been stored in the cache we will get here. That's part of the problem.
There was a problem hiding this comment.
But we might check that the found track also already has a valid id. Otherwise, this is a rare, intermediate edge case.
| @@ -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 | |||
There was a problem hiding this comment.
// The cacheResolver() failed to resolve the track by it's trackId, but has found a different track listed in the database, referencing ...
It took me a bit to remind me how everything is connected. So I think I think this comment helps here.
There was a problem hiding this comment.
The issue is that the using code keeps has the old track ID already on stack and keeps using it.
So we either need to inform the using code about the changed track ID or fail here.
There was a problem hiding this comment.
I am not able to solve all downstream issues and also haven't spotted any serious issues at first sight.
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.