Skip to content

fix: Check Health taking 24h+ on large libraries - #324

Merged
plz12345 merged 5 commits into
eros-developfrom
fix/1102-mount-check-performance
Jul 17, 2026
Merged

fix: Check Health taking 24h+ on large libraries#324
plz12345 merged 5 commits into
eros-developfrom
fix/1102-mount-check-performance

Conversation

@plz12345

@plz12345 plz12345 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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.

Caveat

I could not reproduce a 264k library on ~130 NFS mounts locally, so the headline timing is extrapolated from benchmarks. Merging this auto-closes Whisparr/Whisparr#1102, so it may be worth having the reporter confirm the improvement on their library first.

Follow-up (not in this PR)

RemovedMovieCheck materializes 264k Movie + MovieMetadata + alt-title rows just to read one enum per row. Not an N+1, but a large allocation spike. Proper fix is a status filtered repo query.

@plz12345 plz12345 changed the title Fix Check Health taking 24h+ on large libraries Fix: Check Health taking 24h+ on large libraries Jul 15, 2026
@plz12345 plz12345 changed the title Fix: Check Health taking 24h+ on large libraries fix: Check Health taking 24h+ on large libraries Jul 15, 2026
plz12345 added 4 commits July 15, 2026 15:56
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.
@plz12345
plz12345 force-pushed the fix/1102-mount-check-performance branch from 1aca315 to 09f0744 Compare July 15, 2026 19:56
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).
Comment thread src/NzbDrone.Common/Disk/DiskProviderBase.cs Dismissed
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@plz12345
plz12345 merged commit 1ef2ed2 into eros-develop Jul 17, 2026
38 checks passed
@plz12345
plz12345 deleted the fix/1102-mount-check-performance branch July 17, 2026 21:52
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.

[V3] CheckHealthCommand takes hours on large libraries — MountCheck calls GetAllMounts() uncached per movie path

4 participants