Fix: approved imports silently delete higher-quality files of the same book - #92
Open
sebclark wants to merge 1 commit into
Open
Fix: approved imports silently delete higher-quality files of the same book#92sebclark wants to merge 1 commit into
sebclark wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Every quality protection lives in the grab-side decision engine (
UpgradeDiskSpecification,QueueSpecification,HistorySpecification) - they stop worse releases being grabbed. The import side has no quality comparison at all: once a file is matched and approved for import (a completed download matching a different edition, a manual import, a retry pass),UpgradeMediaFileService.UpgradeBookFileunconditionally deletes every existing file of the book before moving the new one in.The edition-switch path makes this destructive in practice: an incoming MP3 that matches a different edition of a book than the one whose M4B you hold sails past the grab-side specs (they compared within the grabbed edition), reaches
UpgradeBookFile, and the M4B is deleted with reason "Upgrade". With no recycle bin configured (the default), the file is gone permanently.I verified the behaviour on pristine develop with a probe test - existing
M4B-quality file on the book, incomingMP3- and it deletes the M4B without any comparison. On my install a batch of MP3-edition downloads destroyed a run of M4B files this way (recovered most from still-seeding source data; the rest were permanent losses).The fix
A last-line guard in
UpgradeBookFile: before deleting existing files, compare each against the incoming file usingQualityModelComparerover the author's per-media quality profile. If any existing file outranks the incoming one, throw instead of deleting - the import surfaces as a visible failure rather than a silent downgrade. Genuine upgrades (MP3 -> M4B) and same-rank replacements behave exactly as before; grab-side selection logic is untouched.A dedicated decision-layer fix (comparing quality across editions before approving the switch) might be the nicer long-term shape - happy to rework in that direction if you prefer; this guard is the minimal stop-the-data-loss version and is running in production on my install.
Tests
Two new tests in
UpgradeMediaFileServiceFixture: refuse-downgrade (M4B on disk, MP3 incoming -> throws, nothing deleted) and upgrade-still-works (MP3 on disk, M4B incoming -> replaced as before). A literal fails-first run isn't possible for the refusal test since it needs the widened constructor (newIQualityProfileServicedependency) - the pristine destructive behaviour was verified with a separate probe test instead, as described above. Full Core.Test suite green: 2854/2854.