Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix library files inconsistencies #2773

Merged
merged 4 commits into from
Mar 23, 2024

Conversation

mikiher
Copy link
Contributor

@mikiher mikiher commented Mar 20, 2024

Fixes #2686

After some further investigation, it looks like the code already covers Audio files media updates for modified files, so what I ended up doing was the following:

  • Refactored library files properties in LibraryItemScanData
  • Used the refactored properties above in BookScanner and PodcastScanner to update media with modified ebook/cover library files
  • Fixed another related bug in which libraryFolderId in LibraryItem was also not properly updated when books were moved between different library folders within the same library

To test this, I did a lot of moving of directries between different folders (with audio/ebooks/covers) in the same library ) with watcher on and off (and running library scans when watcher was off). Now everything seems to be working without crashes or errors, and the database records look fine after each move. I'd like to write unit tests, but the code is so complicated that I need to wrap my head around how to do it properly.

Also, I still need to write a db migration script to fix existing corrup records, but I think it's important to check in the logic fixes first because of the nasty crashes and corruptions the current code can potentially create.

@mikiher mikiher marked this pull request as ready for review March 20, 2024 10:23
@mikiher
Copy link
Contributor Author

mikiher commented Mar 23, 2024

I also added a fix to #2767, since it touches the same areas of code.

In a nutshell, I fixed the library item matching logic, in both the watcher and the library scan code paths.

Up until now, it was only able to find an existing library item if it had the same ino as the detected change.
This logic missed moving of files from the root folder to a sub folder and vice versa (because in those moves, the library item ino changes from file ino to folder ino, or the other way round), which caused #2767.

I changed the logic to also match between an item's ino and another item's library file inos (and the other way round).

Again, I tested this extensively trying to move an audio file around, with watcher disabled and library scans, and with watcher enabled. This seems to work well now.

@mikiher
Copy link
Contributor Author

mikiher commented Mar 23, 2024

Just a couple of meta comments on all of this:

  • I find the code for handling file updates to be extremely complex and brittle. I think we really need to think of ways to simplify it.
  • The scanLibrary and scanFolderUpdates code paths should be doing roughly the same thing, and yet they are very different and share too little code. I understand the need for different approaches because of the different latency and performance constraints, but we should really strive to make them share as much code as possible.

@advplyr
Copy link
Owner

advplyr commented Mar 23, 2024

The functions that are grouping files into library item paths is something I've wanted to refactor for a while so they can share the same function.

…terate through json objects comparing inodes
Comment on lines +642 to +647
sequelize.where(sequelize.literal('(SELECT count(*) FROM json_each(libraryFiles) WHERE json_valid(json_each.value) AND json_each.value->>"$.ino" = :inode)'), {
[sequelize.Op.gt]: 0
})
], {
inode: ino
})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some file systems have inode values that are only a few digits long so I added this additional query that uses sqlite json functions. Unfortunately this is an extra query and is sqlite specific but in Sequelize v7 they are adding support for handling JSON queries.

@advplyr
Copy link
Owner

advplyr commented Mar 23, 2024

I tested a lot of situations and it is working for all the cases you solved for. I found an existing bug with the scanner that I overlooked when I switched everything to sqlite.

The bug can be reproduced by..

  1. Create a new book by putting an audio file in /Test Author/Test Book/audiofile.mp3
  2. Once book is scanned in move audiofile.mp3 to /Test Author/audiofile.mp3
  3. Note scanner ignores the update with
    [2024-03-23 14:53:10.596] WARN: [LibraryScanner] Files were modified in a parent directory of a library item "/audiobooks/Test Author/Test Title" - ignoring (LibraryScanner.js:583)

I'm not sure how to solve for that yet. This logic is getting complex as you mentioned.
There is another issue of starting the audiofile.mp3 in one folder and moving it to a subfolder. The library item folder will remain the original folder so adding other files in there will be considered the same book.

@advplyr advplyr merged commit 51ff623 into advplyr:master Mar 23, 2024
2 checks passed
@mikiher
Copy link
Contributor Author

mikiher commented Mar 23, 2024

Regarding the bug, I looked at this "first pass" code, and I must admit I don't have clear understanding of what this code is supposed to do, and why... the comment doesn't really help. I'll try to read it more carefully, but if you have a moment, I'd appreciate a short exaplanation.

@mikiher mikiher deleted the fix-library-files-inconsistencies branch March 23, 2024 22:36
@advplyr
Copy link
Owner

advplyr commented Mar 24, 2024

Yeah this is a mess just looking at it again. I'll take a stab at cleaning up that function

@iconoclasthero
Copy link

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Recurring crash
3 participants