Bug 322603 - SqlScanResultProcessor removes tracks completely after scanning for album covers
Summary: SqlScanResultProcessor removes tracks completely after scanning for album covers
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Unclassified
Component: Collections/Local (show other bugs)
Version: 2.7-git
Platform: Archlinux Packages Linux
: NOR grave (vote)
Target Milestone: 2.8
Assignee: Amarok Developers
URL:
Keywords: regression, release_blocker
Depends on:
Blocks:
 
Reported: 2013-07-20 09:43 UTC by Aaron
Modified: 2013-07-21 21:38 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.8


Attachments
Debug output showing problem as it happens (511.03 KB, application/gzip)
2013-07-20 17:11 UTC, Aaron
Details
SqlScanResultProcessor-debugging.patch (8.84 KB, patch)
2013-07-21 08:12 UTC, Matěj Laitl
Details
Debug output showing problem as it happens again (751.89 KB, application/gzip)
2013-07-21 09:22 UTC, Aaron
Details
Scan results (XML files) as requested by Matěj Laitl (783.13 KB, application/gzip)
2013-07-21 09:24 UTC, Aaron
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron 2013-07-20 09:43:32 UTC
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [SqlScanResultProcessor] removeTrack( Entry(id=9455, path="/media/Home/Muziek/Mstislav Rostropovich/Bach - Cello Suites/Disc 1 - J.S. Bach - Cello Suites 1, 4 & 5/03 - Suite no. 1 in G major - III. Courante.flac", dirId=23308, uid="amarok-sqltrackuid://a74694ab871cf29f2260dd27fb36052f") ) 

Reproducible: Always

Steps to Reproduce:
1. Restore working backup (made a few weeks ago, on Amarok 2.7.1)
2. Wipe the images table (get a duplicate key error otherwise; will file a report for this separately)
3. Start Amarok (in debug mode)
4. Hit 'Update collection'
5. Amarok finds all tracks (#322415 solved my problem with this)
6. Put all tracks in playlist
7. Amarok will now search for album art. It actually finds album art, but proceeds to remove every track in the background.
Actual Results:  
Amarok will search for album art and actually finds album art, but proceeds to remove every track in the background.

Expected Results:  
Just don't remove the tracks. They're there, seeing as they've just been added by the collection scanner.

Example debug output for one album:

amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [Playlist::Model] Metadata updated for album "Bach: The Cello Suites" 
amarok:     [SqlScanResultProcessor] removeTrack( Entry(id=9455, path="/home/aaron/Music/Mstislav Rostropovich/Bach - Cello Suites/Disc 1 - J.S. Bach - Cello Suites 1, 4 & 5/03 - Suite no. 1 in G major - III. Courante.flac", dirId=23308, uid="amarok-sqltrackuid://a74694ab871cf29f2260dd27fb36052f") )
Comment 1 Myriam Schweingruber 2013-07-20 10:41:55 UTC
I very much doubt it removes anything, did you try a full collection rescan?

Also please make sure you did install Amarok correctly, using "make install", don't use "make build"

@Devs: This is the third report by an Arch user btw.
Comment 2 Aaron 2013-07-20 12:36:35 UTC
I checked in the database, the tracks are definitely gone from there. Of course, they remain in the file system and a full rescan will re-index them, but most statistics are gone, which is rather unfortunate. About half of my tracks has their stats written to file; while Amarok picks up the rating, it apparently ignores the saved play counts.
Comment 3 Aaron 2013-07-20 13:07:05 UTC
In an attempt to further analyse the problem, I just dropped all tables from the database and restarted Amarok. It created blank tables again and I hit "full rescan". After a few minutes, the entire database is filled with tracks.

Amarok now starts searching for album covers. The GUI freezes for some time. After that, I notice the database is nearly empty again: it's only kept the album I was just playing (with the appropriate album cover, no less), but all the rest is gone again.

Looks like there's definitely a bug, to me.
Comment 4 Aaron 2013-07-20 13:23:30 UTC
One more detail: I just tried whether disabling automatic gathering of cover art has any positive effect on this. It does not; as soon as an album with cover art is encountered (a cover.jpg in the directory, in this case), the entire removal process is started again and I am gradually left with but one album's worth of tracks in my database.

Back to 2.7.1 for me, for now. I really hope this gets resolved before 2.8's release.
Comment 5 Myriam Schweingruber 2013-07-20 14:09:57 UTC
Just a few questions: did you actually use the latest git build in your test (current is v2.7.90-47-g9414f13) or was this with Amarok 2.8 beta aka 2.7.90? How exactly did you build?

What is your database setup? The default internal one or an external MySQL server? Did you check your MariaDB installation is correct? There were problems before on Arch with the database...
Comment 6 Aaron 2013-07-20 14:56:41 UTC
Indeed, I should've mentioned this was a build of 4b3292ef57c07a62e75d210b02e4e5729eb82f2d (the commit that fixes bug #322415).

To elaborate on the build process, here's the relevant part of the script I used to make my Arch package:

build() {
  mkdir build
  cd build
  cmake ../$_appname-$_pkgver \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_INSTALL_PREFIX=/usr \
    -DKDE4_BUILD_TESTS=OFF
  make
}
 
package(){
  cd build
  make DESTDIR="$pkgdir" install
}

As for what database setup I am using: an external MariaDB database, version 5.5.32 (mostly because my Amarok database is backed up daily, like the rest of the dbs).

With regards to whether the install is correct... well, I assume the Arch packager did the regular unit tests before shipping it. It seems to be working as expected for other projects I am using and developing locally.
Comment 7 Konrad Zemek 2013-07-20 15:29:52 UTC
Would you kindly submit the full debug output? It would help with figuring out what's going on.
Also, do you have libmariadbclient installed?
Comment 8 Myriam Schweingruber 2013-07-20 16:04:45 UTC
(In reply to comment #6)
> Indeed, I should've mentioned this was a build of
> 4b3292ef57c07a62e75d210b02e4e5729eb82f2d (the commit that fixes bug #322415).

which is 10 commits behind current git btw, but I guess this doesn't change much as there were no other commits to the database code.
 
> As for what database setup I am using: an external MariaDB database, version
> 5.5.32 (mostly because my Amarok database is backed up daily, like the rest
> of the dbs).

Why external? There is no reason why you can't use the embedded database, it is stored in $HOME/.kde/share/apps/amarok/ and quite easy to backup. The only real reason to use an external database is if you want to share it with somebody else/another computer.

> 
> With regards to whether the install is correct... well, I assume the Arch
> packager did the regular unit tests before shipping it. It seems to be
> working as expected for other projects I am using and developing locally.

There you assume wrongly, as the database mess earlier this year was simply due to users not reading the mandatory upgrade messages (like this one: https://www.archlinux.org/news/mariadb-replaces-mysql-in-repositories/). There was no automatic migration supplied by the packagers. So yes, please check your MariaDB installation, as suggested by Konrad as well.
Comment 9 Konrad Zemek 2013-07-20 16:20:03 UTC
In addition to what Myriam and I wrote above, please try to reproduce the bug using an embedded database.
Comment 10 Aaron 2013-07-20 17:11:25 UTC
Created attachment 81224 [details]
Debug output showing problem as it happens

> Why external? There is no reason why you can't use the embedded database, it is stored in $HOME/.kde/share/apps/amarok/ and quite easy to backup. The only real reason to use an external database is if you want to share it with somebody else/another computer.

My computer is used mostly for development purposes, so I already have a MySQL/MariaDB server running to do get my work done. Why run another MySQL instance while one is already running? To me, this seems to contradict the UNIX philosophy (one daemon for each task).

Anyway, let me iterate this once more: it's not like I'm running a MariaDB server just for Amarok. I use it for development purposes nearly every day.

Added benefits for me are being able to easily access my Amarok database from external applications (I once wrote a statistics script, for example) and my backup script backs it up just like the other twenty-odd databases on my local MariaDB server.

> There you assume wrongly, as the database mess earlier this year was simply due to users not reading the mandatory upgrade messages (like this one: https://www.archlinux.org/news/mariadb-replaces-mysql-in-repositories/). There was no automatic migration supplied by the packagers. So yes, please check your MariaDB installation, as suggested by Konrad as well.

I migrated from the MySQL packages months ago, a day or two after it was released. As I've been saying, the database server is working just fine — I've been using it for other projects than my music library. If you need any concrete information, I'll be happy to provide it, but please don't imply I don't know how to use a package manager.

> Would you kindly submit the full debug output? It would help with figuring out what's going on.

> In addition to what Myriam and I wrote above, please try to reproduce the bug using an embedded database.

I'll do you one better — attached is the debug output of the exact same problem, using the embedded database. The process I used is the following:

1) Set use_server to false in ~/.kde4/share/config/amarokrc
2) `rm -r ~/.kde4/share/apps/amarok/mysqle`
3) `amarok --debug` and capture output
4) Amarok has started with an empty database
5) Do a full rescan of the local collection
6) Drag entire collection to playlist
7) Let Amarok collect album art
8) Debug output shows the same pattern as earlier
9) After a while, I now have *three* tracks in my database.

> Also, do you have libmariadbclient installed?

Yes, I do.
Comment 11 Myriam Schweingruber 2013-07-20 18:04:24 UTC
Thank you for the feedback.

FWIW: I didn't imply anything, i just wanted to be sure this was not an installation issue, because we had a lot of reports for that lately, wrongly blaming Amarok for database issues that were migration problems, so just trying to rule out the possibility. If you had told all this immediately we wouldn't have had to ask.
Comment 12 Matěj Laitl 2013-07-21 08:12:27 UTC
Created attachment 81233 [details]
SqlScanResultProcessor-debugging.patch
Comment 13 Matěj Laitl 2013-07-21 08:15:30 UTC
(In reply to comment #12)
> Created attachment 81233 [details]
> SqlScanResultProcessor-debugging.patch

Hi Aaron,
please checkout Amarok master again, apply this patch, restore the 2.7.1 database from backup, start Amarok in debugging mode, let it forget the tracks again and attach the full debugging output. Also please save the batch scan .xml files that are mentioned in the debugging output for later reference.

            Thanks!
Comment 14 Aaron 2013-07-21 09:22:36 UTC
Created attachment 81236 [details]
Debug output showing problem as it happens again
Comment 15 Aaron 2013-07-21 09:24:39 UTC
Created attachment 81237 [details]
Scan results (XML files) as requested by Matěj Laitl

> please checkout Amarok master again, apply this patch, restore the 2.7.1 database from backup, start Amarok in debugging mode, let it forget the tracks again and attach the full debugging output. Also please save the batch scan .xml files that are mentioned in the debugging output for later reference.

Both are attached to this report now. I hope it helps get to the bottom of the issue!
Comment 16 Matěj Laitl 2013-07-21 10:58:06 UTC
Git commit d1d8a6fd2c81a17a9089e8bb81ddece0c5426904 by Matěj Laitl.
Committed on 21/07/2013 at 10:16.
Pushed by laitl into branch 'master'.

SqlScanResultProcessor: fix stupid data-loss bug caused by my commit ca88c8d3

Commit ca88c8d3 introduced some extra logic and a member variable in
order to fix bug 311078. However, I forgot to add the variable into the
cleanupMembers() method, which resulted in state leaking from one
scanner to another.

What happened:
 1. User performed full rescan. For every directory in the collection,
    deleteDeletedTracksAndSubdirs() was called, which essentially
    inserted all existing directory ids to m_scannedDirectoryIds.
 2. All member variables were cleared except m_scannedDirectoryIds,
    which leaked into the next run.
 3. A file was changed, which triggered partial update scan run with
    only a couple of changed directories.
 4. Its deleteDeletedDirectories() method was called, which called
    deletedDirectories(), which used m_scannedDirectoryIds as a basis,
    removed just the newly scanned dirs from it and returned all the
    rest.
 5. deleteDeletedDirectories() deleted all but newly scanned
    directories.
 6. strohel feels ashamed.

The fix is to simply clear m_scannedDirectoryIds in cleanupMembers().

In addition to fixing bug 322603, this should also minimise changes of
reproducing bug 322474 (but not fixing the actual bug)
Related: bug 322474
FIXED-IN: 2.8

M  +2    -0    ChangeLog
M  +1    -0    src/core-impl/collections/db/sql/SqlScanResultProcessor.cpp

http://commits.kde.org/amarok/d1d8a6fd2c81a17a9089e8bb81ddece0c5426904
Comment 17 Matěj Laitl 2013-07-21 11:00:56 UTC
(In reply to comment #15)
> Both are attached to this report now. I hope it helps get to the bottom of
> the issue!

Hey, they did! Please try the latest master and reported whether it really fixed this problem.
Comment 18 Aaron 2013-07-21 21:38:40 UTC
That seems to have fixed it! Thanks very much.