Release 3.3.5#337
Merged
Merged
Conversation
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…315) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
MountCheck had no tests. Pins current behaviour before optimising the mount lookups it performs: the null-mount and null-MountOptions guards (MountOptions is always null on Windows, since DriveInfoMount is built without them), the DistinctBy on root directory, and the reported path ordering.
GetMount() re-enumerated the entire OS mount table on every call. On Linux that reads /proc/mounts and stats every mount point, so on a host with many network mounts it is hundreds of syscalls per call. MountCheck calls GetMount() once per movie path, which made Check Health take over 24 hours on a 264k scene library. DiskTransferService also pays this twice per file transfer. Cache the mount list for 15 seconds. That collapses a health check run from hundreds of thousands of enumerations to roughly one every 15s, while staying short enough that a newly mounted share still shows up in the folder browser without any invalidation hooks. GetMount() stays per-path: MountCheck resolves mounts from movie paths specifically so symlinks and junctions are handled by the provider, so the number of lookups is unchanged, only their cost. Only topology is cached. Free space is unaffected, since IMount reads it live from the underlying DriveInfo/UnixDriveInfo on each property access. Fixes Whisparr/Whisparr#1102
RootFolderCheck and DiskSpaceService called GetBestRootFolderPath without passing the root folders. The result is cached per path, but on a miss it falls back to querying every root folder from the database, so a cold cache meant one query per movie. RootFolderCheck runs on startup, when the cache is always cold, which on a 264k scene library is 264k queries. Pass the root folders in via the existing overload, which is there for exactly this.
GetMount tested every mount with IsParentPath, which walks the path back up to the root each time. That walk only depends on the path, so it was being repeated identically for every mount on the system. Hoist it: build the ancestors once per lookup and compare each mount root against them. PathEquals is untouched, and the comparison is the same OsPath equality IsParentPath performed, so paths resolve exactly as before, including relative segments that PathEquals resolves via CleanFilePath. Measured over 130 mounts, roughly 160us -> 62us per lookup, or ~42s -> ~16s across a 264k scene library.
None of these sit on lines the mount check perf work touches; they are
pre-existing findings inherited from upstream. Keeping them in their own
commit so that PR stays reviewable and the divergence is easy to drop.
- Pass the caught exception to the logger in FolderWritable (S6667). The
NLog layout renders ${exception:format=ToString} on any call carrying
one, so the inline copy of e.Message goes away rather than being
printed twice. Net gain is the exception type, which the bare message
did not carry.
- Rename IsFileLocked's parameter to path, matching IDiskProvider (S927).
No caller passes it by name.
- Drop RemoveReadOnlyFolder, which has no callers (S1144).
- Make GetDriveInfoMounts static (S2325). It was never virtual, so no
subclass could have overridden it.
- Make SetWritePermissionsInternal static in the Mono test fixture (S2325).
The index status bar showed a generic "Downloaded" instead of the file's quality. Every index view (table, poster, overview, for both scenes and movies) already renders movieFile.quality.quality.name and falls back to "Downloaded" when it is absent -- the paged API simply never sent it. PagedBuilder already LEFT JOINs MovieFiles, but QueryJoined builds its SELECT from its type parameters, so PagedQuery's <Movie, MovieMetadata> emitted only Movies.* and MovieMetadata.*. Movie.MovieFile is a plain property rather than LazyLoaded, so nothing back-filled it and ToResource's null-conditional dropped the file. GetPaged now maps MovieFile via its own query. Two constraints shaped it: - PagedQuery is shared with MoviesWithoutFiles and MoviesWhereCutoffUnmet, which GROUP BY Movies.Id. Selecting a non-aggregated file column under that GROUP BY errors on Postgres, so those keep using PagedQuery and only GetPaged changes. Neither needs the file: their rows fetch it separately. - MovieFileResource.ToResource dereferences Movie.QualityProfile to compute QualityCutoffNotMet, so hydrating MovieFile without a profile would throw. The profile is hydrated from the repository, falling back the way All() does when a movie points at a profile that no longer exists. MediaInfo is left out of the query: it stores the full ffprobe dump (~7KB a row, measured) that no index view renders, and it would otherwise be deserialized for every row on a page of up to 1000. Fixes Whisparr/Whisparr#1104
POST /movie/paged took request.PageSize verbatim, so a client could ask for any number of records while the index page size options cap at 1000. The tag-filter branch of this same endpoint, StudioController and PerformerController all already guard their page size. Uses <= 1000 rather than the < 1000 those three use: the page size options allow 1000 inclusive, so the exclusive form would quietly serve 10 records to anyone who picked the documented maximum. Out-of-range values fall back to 10, matching the existing guards. This is a behavior change for any client that passes a page size above 1000.
Fixes Whisparr/Whisparr#1102 `CheckHealthCommand` runs for 24+ hours on a 264k scene library (~130 NFS/ZFS mounts), pinning CPU throughout, then re-triggers 6 hours later. Health checks run serially on the command executor thread, so this blocks every other scheduled task. Note: Radarr `develop` is byte-identical in `MountCheck.cs` and `DiskProviderBase.cs`, so this is not Whisparr specific and there was no upstream fix to mirror. Worth upstreaming. ## The bug `MountCheck` calls `GetMount()` once per movie path, and `GetMount()` re-enumerated the entire OS mount table on **every** call, with no caching. On Linux that means reading `/proc/mounts` and stat'ing every mount point, so at 264k paths it did 264k full mount table enumerations, each hundreds of syscalls (and network round trips, on NFS). ## The changes Each is a separate commit and reviewable on its own. **1. Cache the mount list (the actual fix).** `GetAllMounts()` becomes a private caching wrapper with a 15s absolute TTL over a new `protected virtual FetchAllMounts()`, which Mono overrides. Private so a subclass cannot accidentally bypass the cache, and so a stray `override GetAllMounts` fails loudly at compile time rather than silently doing nothing. `GetMount()` deliberately stays per-path. `MountCheck` resolves mounts from movie paths specifically so the provider handles symlinks and junctions, so the number of lookups is unchanged, only their cost. Only topology is cached. Free space is unaffected: `DriveInfoMount` and `ProcMount` read it live from the underlying `DriveInfo`/`UnixDriveInfo` on each property access. 15s is short enough that a newly mounted share still turns up in the folder browser without needing any invalidation hooks, while still collapsing a health check run from hundreds of thousands of enumerations to roughly one per 15s. This also speeds up imports, since `DiskTransferService` pays two lookups per file transfer. **2. Fix a `RootFolderCheck` N+1 (independent bug found on the way).** `RootFolderCheck` and `DiskSpaceService` called `GetBestRootFolderPath` without passing the root folders. The result is cached per path, but on a miss it falls back to querying every root folder from the database. `RootFolderCheck` runs on startup, when the cache is always cold, so that was one query per movie. Fixed via the existing overload, which exists for exactly this. **3. Cheapen the per-mount scan.** `GetMount` tested every mount with `IsParentPath`, which walks the path back up to the root each time. That walk only depends on the path, so it was repeated identically for all ~130 mounts. Hoisted to once per lookup. I originally planned to also hoist `PathEquals`'s normalization, but benchmarking showed that assumption was wrong: normalization was only ~14% of the cost, while the ancestor walk was the bulk. So `PathEquals` is left completely untouched, which avoids precomputing `CleanFilePath` per mount (it can throw, which would have changed when exceptions fire on the import hot path). Measured over 130 mounts: ~160us -> ~62us per lookup, or ~42s -> ~16s across 264k paths. ## Testing `MountCheck` had no coverage at all, so the first commit adds a fixture pinning current behaviour before anything changed: the null-mount and null-`MountOptions` guards (`MountOptions` is always null on Windows, since `DriveInfoMount` is constructed without them), the `DistinctBy` on root directory, and the reported path ordering. Both regression tests were confirmed to fail without their fix: - mount cache: *"Expected invocation on the mock once, but was 3 times"* - root folder N+1: *"should never have been performed, but was 10 times"* (10 movies) New `GetMount` semantics tests (longest matching mount wins, exact mount root, paths with relative segments) were confirmed to **pass against the old code** first, so they pin existing behaviour rather than the new implementation. I also ran a differential check of old vs new resolution across 34 real paths against this machine's 11 real mounts, including a nested pair (`/System/Volumes/Data` and `/System/Volumes/Data/home`), `..` segments and trailing slashes. Identical results. Green: `Common.Test`, `Mono.Test`, `Host.Test` (`ContainerFixture` resolves the whole DI graph, which covers the constructor plumbing), and the 144 `Core.Test` health check tests. Two `HttpClientFixture` Cloudflare tests fail, but they fail identically on `eros-develop` without these changes; they hit a live network endpoint and are unrelated.
) Fixes Whisparr/Whisparr#1104 The index status bar showed a generic "Downloaded" instead of the file's quality. Radarr shows the quality, and this used to as well. ## What was actually wrong The issue suspected an expensive `movie.moviefileid.quality.id` relationship walk needing an in-memory movie-file cache. It turned out to be neither expensive nor a frontend gap: - **The frontend was already complete.** Table, poster and overview — for both scenes and movies — already render `movieFile.quality.quality.name` with `?? translate('Downloaded')` as the fallback. That fallback *is* the reported bug. No frontend changes here. - **The JOIN already existed.** `PagedBuilder` already LEFT JOINs `MovieFiles`. But `QueryJoined` builds its SELECT from its *type parameters*, so `PagedQuery`'s `<Movie, MovieMetadata>` emitted only `Movies.*` and `MovieMetadata.*` — the file was joined and then discarded. `Movie.MovieFile` is a plain property rather than `LazyLoaded`, so nothing back-filled it and `ToResource`'s null-conditional dropped it. ## The change `GetPaged` now maps `MovieFile` through its own query. Two constraints shaped it, and both are worth knowing before simplifying this: 1. **`PagedQuery` is deliberately left alone.** It is shared with `MoviesWithoutFiles` and `MoviesWhereCutoffUnmet`, which `GROUP BY Movies.Id`. Selecting a non-aggregated file column under that GROUP BY errors on **PostgreSQL** (`column "MovieFiles.Id" must appear in the GROUP BY clause`) — SQLite tolerates it, so this would not surface in local testing. Neither Wanted page needs the file: their rows already fetch it via `useSingleMovieFile`. 2. **`QualityProfile` must be hydrated.** `MovieFileResource.ToResource` dereferences `movie.QualityProfile` unconditionally to compute `QualityCutoffNotMet`, and `UpgradableSpecification.QualityCutoffNotMet` dereferences `profile.UpgradeAllowed` on its first line. Hydrating `MovieFile` without a profile throws `NullReferenceException` (verified) — latent today only because `MovieFile` is always null. The profile comes from the repository with a fallback, the way `All()` already handles movies pointing at a profile that no longer exists. `MediaInfo` is excluded from the query. Measured against a real library it stores the full ffprobe dump at ~7KB a row, ~90% of it `rawStreamData`, which `MediaInfoResource` discards anyway and no index view renders. Including it would deserialize ~7MB per request on a page of 1000. Consequence: `/movie/paged` returns `movieFile.mediaInfo = null` while `/movie` still populates it. The column list is derived from the mapper rather than hardcoded, so a new `MovieFiles` column can't silently go missing, and `Id` is forced to lead the segment because Dapper splits on the first `Id` column. ## Second commit `POST /movie/paged` took `request.PageSize` verbatim, so a client could request any number of records. The tag-filter branch of this same endpoint, `StudioController` and `PerformerController` all already guard theirs. Kept as a separate commit — it's a pre-existing gap, not part of the quality fix. Uses `<= 1000` rather than the `< 1000` those three use: the index page size options allow 1000 inclusive, so the exclusive form would quietly serve 10 records to anyone picking the documented maximum. **Behavior change** for any client passing a page size above 1000 (now falls back to 10, matching the existing guards). ## Testing - 7 repository tests covering quality hydration, profile hydration, the missing-profile fallback, `MediaInfo` exclusion, and null-file. 4 of them fail without the fix. - A `MoviesWithoutFiles` test pinning constraint 1 — that path had no coverage at all. - 4 `MovieResource` mapping tests covering constraint 2, which the repository tests can't reach. - Verified end-to-end against a real library: `/movie/paged` returns the file's quality with `qualityCutoffNotMet` computed and `mediaInfo: null`; Wanted/Missing and Cutoff Unmet still return records; 1000 records served in ~48ms. Quality confirmed rendering on the index in the UI. Not verified: the Postgres GROUP BY hazard is reasoned from the shared-`PagedQuery` structure and avoided by construction rather than reproduced — SQLite won't surface it.
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
….29.7 (#298) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
sampulsar
approved these changes
Jul 20, 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
5 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.
Database Migration
NO
Description
Rolling the 3.3.5 development cycle into a stable release.
Replaces #330, which was headed by
eros-developand therefore kept absorbingpost-3.3.5 commits as they landed. This one is headed by the frozen
release-3.3.5branch, pinned to 39b1db6 — the exact commit carrying thev3.3.5marker tag — so the release contents can't drift.Commits after the tag (#331, #334, #335) are deliberately left on
eros-developfor the next cycle.Merge with "Create a merge commit" — not squash, not rebase. The build
derives its base version from the
v3.3.5tag's commit SHA being reachablefrom
eros; squashing collapses that SHA and rebasing rewrites it, either ofwhich makes the release build as 3.3.4.
Screenshot(s) (if UI related)
Details
Todos
Issues Fixed or Closed by this PR