Harden path identity, durable moves, scans, and root relocation - #717
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes download/import path handling so client-reported torrent paths preserve meaningful leading/trailing whitespace (important on Unix-like filesystems), while adding stricter, OS-aware validation for Listenarr-owned destination paths (root folders, add-to-library, and move operations).
Changes:
- Preserve whitespace in torrent client path mapping and remote path translation by avoiding whitespace-trimming/whitespace-only rejection in key path plumbing.
- Add OS-aware normalization/validation for user-provided destination directories, including explicit support for filesystem roots in root-folder configuration and blocking parent traversal for concrete destinations.
- Add regression tests covering whitespace-bearing torrent folders (Transmission/qBittorrent), root-folder root paths, and invalid destination rejection in library add/move workflows.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Mocks/Api/TransmissionApiMock.cs | Adds a canned Transmission torrent response with whitespace-bearing folder names for regression coverage. |
| tests/Features/Infrastructure/DownloadClients/Common/TorrentClientPathMapperTests.cs | New tests asserting whitespace preservation and rooted-child handling for torrent file mapping. |
| tests/Features/Infrastructure/DownloadClients/Common/DownloadClientAdapterTests.cs | Adds Transmission adapter regression asserting whitespace-bearing ContentPath/SourceFiles are preserved. |
| tests/Features/Domain/Utils/FileUtilsTests.cs | Adds tests for whitespace-preserving combining/normalization and new destination-path validation rules. |
| tests/Features/Application/Downloads/Common/DownloadClientGatewayTests.cs | Adds tests ensuring remote path mapping and directory expansion keep whitespace-bearing paths intact. |
| tests/Features/Application/Audiobooks/RootFolders/RootFolderServiceTests.cs | Adds coverage for allowing filesystem roots and validating/normalizing root folder paths. |
| tests/Features/Api/Features/Library/LibraryController_MoveTests.cs | Adds API test ensuring invalid move destinations are rejected with 400. |
| tests/Features/Api/Features/Library/LibraryController_AddToLibraryTests.cs | Updates custom path test to be OS-safe and adds parent-traversal/invalid-path rejection tests. |
| listenarr.infrastructure/DownloadClients/Transmission/TransmissionImportPathResolver.cs | Stops treating whitespace-bearing strings as “empty” when building content/source paths. |
| listenarr.infrastructure/DownloadClients/Qbittorrent/QbittorrentImportPathResolver.cs | Same as above for qBittorrent source file translation. |
| listenarr.infrastructure/DownloadClients/Common/TorrentClientPathMapper.cs | Reworks torrent path mapping to avoid trimming path-segment whitespace and better handle rooted-looking child paths. |
| listenarr.infrastructure/Configuration/Paths/RemotePathMappingService.cs | Adjusts empty-path detection to avoid rejecting whitespace-bearing remote paths. |
| listenarr.domain/Common/FileUtils.UserProvidedPaths.cs | New OS-aware user-provided directory path validation/normalization helper with root/traversal options. |
| listenarr.domain/Common/FileUtils.PathCombining.cs | Documents CombineWithOptionalBase’s whitespace-preserving intent and relaxes base-path emptiness check. |
| listenarr.application/Downloads/Common/DownloadClientGateway.cs | Preserves whitespace-bearing paths during mapping/translation and directory-expansion flows. |
| listenarr.application/Audiobooks/RootFolders/RootFolderService.cs | Root folder create/update now uses OS-aware normalization and allows filesystem root paths. |
| listenarr.application/Audiobooks/Contracts/ILibraryAddService.cs | Extends result contract to surface validation failures cleanly to API workflows. |
| listenarr.application/Audiobooks/Catalog/LibraryAddService.cs | Validates custom/generated destination paths with OS-aware normalization and blocks parent traversal. |
| listenarr.api/Features/Library/LibraryMoveWorkflow.cs | Validates and normalizes move destinations (blocking traversal) instead of silently normalizing invalid targets. |
| listenarr.api/Features/Library/LibraryAddWorkflow.cs | Returns 400 for validation failures and validates custom destination paths via OS-aware normalization. |
|
Temporary formatter/compiler diagnostic superseded and cleaned up. |
3 similar comments
|
Temporary formatter/compiler diagnostic superseded and cleaned up. |
|
Temporary formatter/compiler diagnostic superseded and cleaned up. |
|
Temporary formatter/compiler diagnostic superseded and cleaned up. |
|
Temporary verbose Windows test diagnostic superseded by compact output. |
Temporary Windows test diagnosticsExit code: |
Temporary Windows test diagnosticsExit code: |
Temporary Windows test diagnosticsExit code: |
Temporary Windows test diagnosticsExit code: |
…ns, root relocation) into kevin/canary-live The largest sync of the fork: 716 upstream files, 48 merge conflicts, resolved theirs-for-infrastructure / ours-for-behavior. Highlights: - Adopted upstream's DB-enforced file ownership/identity, filesystem mutation coordinator, per-book operation locks, and durable move manifests. Our descend-move, BasePath-persist, and scan cascade belts are superseded by their structurally stronger equivalents. - Local behaviors re-expressed on the new architecture: post-scan notification + Audiobookshelf scan hooks, rescan-metadata-on-scan (ForceMetadataRefresh end-to-end), organize stub-target replacement (wired through durable move target authorization), delete-time author exclusions inside the coordinator flow, enriched ASIN-conflict 409, organize sweep routed through the durable move workflow. - Kept: pre-ingest verification, matching id-guard, blocked-entry cleanup, stall reaper, all Audiobookshelf/AI-assist/verification surface, server-driven activity feed (upstream's move-progress UI adapted into it). - Migrations: upstream's duplicate ProcessExecutionLogs migration dropped in favor of our earlier identical one; both Listenarrs#717 Designers regenerated as supersets carrying all local model state (a SQLite table-rebuild would otherwise drop local columns — incl. a live-DB data-loss hazard on MoveJobs.ReplaceStubTarget); pre-existing drift in five local Designers repaired now that Designer accuracy is load-bearing. - macOS portability fixes to upstream's new filesystem code (candidate upstream PR): openat(O_CREAT) variadic-mode corruption on Apple arm64, missing Darwin branch in FileSystemSemanticsResolver, /var symlink vs O_NOFOLLOW pinned walks, host-default case-sensitivity assumption. - Architecture-rule reconciliation: 54 pre-rule local test classes exempted, 4 fork-local files allowlisted for legacy path identity, 2 files split into partials for the size cap. Gates: backend 3332/3332 (135 platform-gated skips), frontend 657/657, vue-tsc/eslint/vite build clean, zero conflict markers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#717 added This branch predated TestClasses_FollowRepositoryConventions, so the test class inherited nothing and carried neither trait. Inherit BaseTests and add the exact Name trait and a Category, which is what the architecture suite now requires.
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives.
Rework of this PR onto Listenarrs#717's rewritten file-registration flow. - FfprobeTagMetadataMapper now reads ASIN / AUDIBLE_ASIN / ISBN tags into AudioMetadata, so a scanned file's embedded identifier is available during registration. - New AudiobookFileService.IdentifierAdoption partial adopts that identifier onto a bare audiobook from INSIDE the per-audiobook operation lock (EnsureAudiobookFileCoreAsync), so the write cannot race file ownership or a concurrent audiobook update. A unanimity guard refuses to adopt when the book's linked files carry disagreeing ASINs (a sign of mis-attribution). - The upstream metadata refresh (Audible lookup) runs AFTER the lock is released -- signalled out via a StrongBox -- so the network call never holds the global filesystem lock. It fills only empty fields and never fails the scan. Restores the IAudiobookMetadataRefreshService dropped in the rebase. Tests: FfprobeTagMetadataMapper (incl. AUDIBLE_ASIN dialect + no-overwrite), AudiobookMetadataRefreshService.FillMissingFields, and the coordination test updated for the new constructor dependency. Build + suite green, no regressions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Ported onto Listenarrs#717, where BuildNamingMetadata has moved into DownloadImportService.Naming.cs and the culture-dependent parse is reintroduced there. A position arrives from Audnexus as a string and is not always a decimal: an omnibus sits at "1-4", a prequel at "0", a novella at "1.5". Squeezing it through decimal.TryParse loses the ones that do not parse, and naming then falls through to the track number and writes that into the filename as if it were the series number. The parse also used the server's culture, so "1.5" read as 15 wherever '.' is the group separator. SeriesPositionRaw keeps the original string, naming prefers it, and the remaining decimal parses are pinned to InvariantCulture. On the import side the {SeriesNumber} token is built by a small helper in DownloadImportService.Naming.cs rather than written inline. DownloadImportService.cs is already at the 500 line architecture cap, and the naming partial is where the rest of this logic lives. (cherry picked from commit b5ad3d9)
Summary
This PR began with the Unix/macOS download-folder whitespace bug in #528 and expanded into a repair of the filesystem contracts used by imports, audiobook moves, root-folder relocation, scans, file registration, organize/rename, deletion, and path-bearing persistence.
The branch now separates three concepts that were previously conflated:
Physical library moves are durable workflows instead of
BasePathpre-saves. Jobs persist endpoint identity, tracked-file manifests, lease/retry state, physical-generation proofs, database-backed mutation journals, target-directory state, cleanup authority, and durable scan handoffs so crashes, cancellation, retries, pathname replacement, and stale workers fail closed.Issues resolved
Closes #528
Closes #598
Closes #765
Closes #438
Related: #766
Platform-specific mutation contract
mkdirat, then reopened and verified without following links. Because POSIX does not atomically return a handle for the generation created bymkdirat, a newly created Unix directory is treated as retained/unowned rather than being granted destructive cleanup authority solely from the create result.Changes
Added
AudiobookDeletionIntentstate so process death between filesystem cleanup and database deletion cannot orphan files without recovery authority.ConfigRootPath/runtime/file-move-locks.Changed
BasePath; metadata changes only after durable completion or through the explicit metadata-only repair path.FileMovercopy/move/hardlink fallbacks to reject aliases and overlaps, verify publication and cleanup, preserve uncertain content, and avoid recursive copy-and-delete success claims.Retained.Pending,Failed, andNeedsAttentionstates rather than inferring repairability from human-readable error text.(2)/(3)duplicates and duplicateAudiobookFilerows.\\remains a filename character instead of being rewritten as a separator, including when the Unix root itself contains a literal backslash.Fixed
BasePathto the author folder (Library scan attributes every book by an author to whichever one is scanned, and sets the author folder as its BasePath #765); the fix/regression approach proposed in fix(scan): don't attribute every book by an author to whichever one is scanned #766 was adapted onto this PR...traversal into case-distinct Windows sibling directories.AudiobookFileIdincorrectly blocking root collision repair forever..NET LocalApplicationDatacould resolve empty when$HOME/.local/sharedid not exist; cross-process move locks now use the Listenarr-owned config runtime directory.Removed
BasePathpre-save behavior.Windows == case-insensitiveassumptions from destructive mutation authorization and durable recovery identity.Testing and current validation status
Local Windows validation of the final unchanged tree
dotnet format --verify-no-changes: passed.dotnet ef migrations has-pending-model-changes: no pending model changes.git diff --check: passed.Native GitHub validation
Exact-head
Run Testsworkflow31510036348is green forac211dba8e4710eaa9be583d035b0dc551d2eebe.Linux (
unit-tests)Windows (
backend-tests-windows)All current exact-head checks, including CodeQL and the
minorversion-label check, are green. There are currently zero unresolved GitHub Code Quality review threads.External Docker/Podman reproduction
The Docker lock-directory defect reported by @m4bard is addressed by moving cross-process locks to the Listenarr-owned config runtime root. The original unprimed-container reproduction was rerun independently against a descendant containing that fix and passed all expected import modes. The independent rerun also confirmed that the runtime lock hierarchy is created from an initially absent
runtime/directory, lock permissions remain current-user-only, and no lock artifacts are leaked into the library tree.Review status
The final unchanged tree completed two consecutive independent adversarial review passes with zero confirmed findings.
The review covered the complete authoritative remediation diff, including:
The final source tree remained unchanged after the second clean review pass and exact-head native validation.