fix(import): reject a forced import onto an occupied destination instead of throwing - #251
Open
jordanfelle wants to merge 10 commits into
Open
jordanfelle wants to merge 10 commits into
jordanfelle wants to merge 10 commits into
Conversation
A book group's files are imported one at a time, but the "which existing files does this replace" query is re-run from the database for every file in the group, and the old rows are not removed until the whole group is committed. When the new release writes its parts to the same destination paths as the old release (the normal case for a multi-part audiobook), the second file of the group saw the first file's freshly written destination as an old file to displace, renamed it to a .chaptarr-upgrade~ backup, and FinalizePendingFileCommits then recycled it. For a 173-part set only the last part survived. Track the destination paths written and the rows displaced by the batch in progress, exclude those from replacement selection, and never stage a path the same batch already imported into.
A file whose destination path was already tracked took one of two paths in ResolveBookFilePathConflict: queue that row for the group's atomic swap, or adopt it in place. Both lost the row when another file of the same batch had already queued it for replacement, because the group-level ReplaceMany then deleted the very row the adoption had just repointed at the new file. The file stayed on disk with no BookFiles row, and history recorded an 'Upgrade' deletion for a path the batch had just imported. Always reuse the row found at the destination, keep the batch's adopted row ids out of GetDatabaseRowsToReplace, and keep the pending commit registered so its staged backups are still recycled and are never rolled back once the row is persisted.
…ing files ImportApprovedBooks.ImportFile built filesToReplace from every existing file of the matched edition and, for an automatic import (replaceExisting: true from DownloadedBooksImportService), staged, replaced and recycled them whenever the new copy was not a strictly lower *revision* (ImportApprovedBooks.cs:1104-1109). A second copy of an already-imported release therefore churned the library: on the live host 2,525 editions were re-imported in the same format as "Upgrade". An automatic import (not IsManualImport, not DownloadForced) now only displaces existing files when the shared upgrade rule says the release is a real upgrade for the edition's quality profile: IUpgradableSpecification.IsUpgradable plus IsUpgradeAllowed, which is the same profile-preference comparison (quality rank, revision, custom-format score, UpgradeAllowed) that search, queue and history use. Equal or worse against any file it would replace is reported as "Already imported: this edition already has files of equal or better quality" and nothing is moved, deleted or recycled. Rows whose file is missing on disk are excluded, so stale/aborted-import rows and the converted-destination reuse path are unaffected. Manual imports and forced grabs keep their existing explicit-replace behaviour untouched. The rejection surfaces through the normal "nothing imported" path: the tracked download ends up importBlocked with that message. No queue-completion mechanism was added - marking it Imported would fabricate history rows, and there is no established "recognised duplicate, leave the queue" state in this codebase.
…ready-imported duplicate The equal-quality guard compared each incoming file with the edition's surviving files only. An edition that lost parts (its rows still declare PartCount N but fewer than N files remain, as after the same-path upgrade deletion) was therefore rejected as 'already has files of equal or better quality' when the complete set arrived, so the automatic import could no longer repair it. When the surviving rows declare more parts than are present, let the import proceed and replace the partial set. Also adds a regression test that a multi-file upgrade parses each stored file's custom formats once per batch.
…rowing An automatic import whose computed destination path already held a file that filesToReplace did not cover reached CopyBookFile/MoveBookFile, where ClearTargetPath throws DestinationAlreadyExistsException and the queue item ends as IMPORT_EXCEPTION. filesToReplace only covers rows of the same edition at a different path, so a destination occupied by an untracked file or by a row tracked for another edition of the same book was never caught. ImportFile now checks the computed destination before any staging or transfer, for automatic imports only: a row tracked for this edition gives the existing AlreadyImportedRejectionReason, a row for another edition or an untracked file gives a clear rejection naming the destination. Nothing is moved or deleted. Manual imports, forced downloads, relocations and genuine upgrades (whose destination row is in filesToReplace) are unaffected.
…not exist DiskProvider.FileExists on a missing path falls back to enumerating each directory along the path for a case/normalisation match. The occupied-destination pre-check ran that on every automatic import, including the ordinary case of a new book whose folder does not exist yet, where nothing can occupy the destination. Check the parent folder first and only probe the file when it exists; the transfer keeps its own check. Adds a regression test that records FileExists probes.
…tion instead of throwing A manual import with replaceExisting=false skipped the occupied-destination check that automatic imports get, so an untracked file or a file tracked for another edition at the managed destination reached the transfer, which threw DestinationAlreadyExistsException after staging began and was logged as an Error with a stack for every retry. Apply the same pre-check when the import will not replace files; replacing manual imports are unchanged.
…ile at the destination The occupied-destination check exempted every manual import that replaces files, but replacing only stages aside tracked rows this import replaces (they are exempt from the check) and the transfer never overwrites. A replacing manual import onto an untracked file or a file tracked for another edition therefore still threw DestinationAlreadyExistsException after staging began. Apply the check to every import except relocations and forced downloads.
The up-front destination check skipped forced downloads, but a forced import overrides quality and duplicate rejections, not the fact that the transfer never overwrites a file. A forced import onto an untracked file, or one tracked for another edition, therefore still threw DestinationAlreadyExistsException after staging began. Apply the same classification to forced imports and add how to proceed to the message; a tracked file that the import replaces is still exempt.
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
The 'remove or track that file first' hint is wrong advice for a destination already tracked for this edition, and the appended text ran on without a separator after the path. Add it (after a full stop) only for the untracked and other-edition classes, and pin the same-edition forced case.
jordanfelle
added a commit
to jordanfelle/chaptarr
that referenced
this pull request
Sep 27, 2026
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
2 participants
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.
Fixes #239
Depends on #217: the diff includes #217's commits until it merges (this PR is the last commit only).
Problem
The up-front destination check added in #216/#217 excludes forced downloads (
downloadForced). A forced import overrides quality and duplicate rejections, but the transfer never overwrites an existing file, so a forced import onto an untracked file, or one tracked for another edition, still threwDestinationAlreadyExistsExceptionafter staging began instead of being rejected with a reason.Fix
Apply the same three-way classification (tracked for the same edition / tracked for another edition / untracked) to forced imports. The rejection message says why and how to proceed ("A forced import cannot overwrite it: remove or track that file first, or replace the tracked file."). Relocations keep their exemption, and a tracked file that the import replaces is still staged aside first, so a forced replacing import still works.
Verification