fix(health): use forward-slash virtual paths for rclone VFS notifications

[fatbob01]

Summary

On Windows the directories handed to vfs/forget and vfs/refresh carry \ separators, so they match no VFS node and the invalidation silently does nothing.

filepath.Dir is OS-aware. These are virtual paths, and rclone's VFS is forward-slash on every platform, so they need the POSIX path package instead.

Why it fails silently

vfs/forget echoes back whatever it is given and reports success regardless of whether the path resolves:

POST /vfs/forget  dir=tv/THIS-DIRECTORY-DOES-NOT-EXIST-12345
{ "forgotten": [ "tv/THIS-DIRECTORY-DOES-NOT-EXIST-12345" ] }

So nothing surfaces as an error. From a Windows host running external rclone, before the fix:

{"level":"ERROR","msg":"Failed to notify rclone VFS to forget/refresh directories",
 "dirs":["tv\\SpongeBob.SquarePants.S17E15.Night.School.Knuckleheads.720p.HDTV.AAC2.0.x264-Slurpuff"], ...}

In one night: 18 "Successfully notified rclone VFS" entries that invalidated nothing, plus 16 outright failures.

Four sites

file	what it builds
internal/health/checker.go	NotifyRcloneVFS directory
internal/health/library_sync.go	orphaned-metadata cleanup dirs
internal/nzbfilesystem/metadata_remote_file.go	coalesced refresh after a safety-folder move
internal/importer/postprocessor/vfs_notifier.go	the refresh target and its ancestry

The first two came from #846; the last two predate it.

The vfs_notifier one also had a second defect: it walked the ancestry with filepath.Dir before normalizing, and its != "/" guards never match on Windows where the root is \, so a useless bare-root entry was appended to every batch:

"dirs":["tv/MasterChef.US.S16E15.1080p.DSNP.WEB-DL.AAC2.0.H.264-RAWR","\\tv","\\"]

Also normalized at the RC boundary

I found these four one at a time, each from a stray backslash in a log, which suggests per-site fixes are fragile: a new caller reintroduces the bug and nothing surfaces it. So RefreshDir now normalizes too, in both implementations. The caller-side fixes stay, since vfs_notifier's root guards can only be fixed at the caller, but a missed site degrades to a working call instead of a silent no-op.

ToVFSPath is a thin named wrapper over filepath.ToSlash so the intent is testable.

No behaviour change on Linux or Docker

On POSIX, os.PathSeparator is /, so filepath.ToSlash returns the string unchanged and filepath.Dir and path.Dir run the same Clean logic. Output is byte-for-byte identical. A backslash is a legal POSIX filename character and is deliberately left alone.

Measured with GOOS=windows:

input	filepath.Dir (current)	path.Dir(ToSlash(…))
tv/SpongeBob.S17E15/ep.mkv	tv\SpongeBob.S17E15	tv/SpongeBob.S17E15
movies/Film.2024/film.mkv	movies\Film.2024	movies/Film.2024
/file-at-mount-root.mkv	\	/
tv\Legacy.Row\ep.mkv (legacy row)	tv\Legacy.Row	tv/Legacy.Row

Deliberately unchanged

filepath.Dir calls that operate on real OS paths stay as they are: symlink target resolution and filesystem walking in library_sync.go (1284, 1456, 1660), and the filepath.Join(cfg.MountPath, …) for os.Stat in vfs_notifier.go.

Testing

• pkg/rclonecli/paths_test.go: forward-slash input unchanged on every platform, the platform-specific separator contract, and the boundary property that no Windows separator reaches rclone.
• go test ./... passes.
• Running in production on Windows with external rclone. Since deploying, Failed to notify rclone VFS has gone from 16 in a night to 0, and the directories now arrive correctly forward-slashed.

[fatbob01]

Added a third site in 292ee77: RepairCoalescer.EnqueueRefresh in internal/nzbfilesystem/metadata_remote_file.go also receives filepath.Dir, so the coalesced refresh after a safety-folder move has the same problem. This one predates #846.

Caught it in my own logs:

"dirs":["movies/Anything.for.Love.1993.DVDRip.x264-HANDJOB","\movies","\\"]

Note the bare "\\" for a file at the mount root, alongside a correctly-formed entry from a different code path in the same batch.

Heads-up on merge order: this touches a line in metadata_remote_file.go adjacent to #848, so whichever of the two lands second may need a trivial rebase. Happy to do that whenever you like.

[javi11]

Can you add a test?

[fatbob01]

Found a fourth site after deploying this and watching the logs: notifyVFSWith in internal/importer/postprocessor/vfs_notifier.go. Added in 6462e0c.

It normalizes the target path but walks the ancestry with filepath.Dir first, so the parent and grandparent stay backslash-separated (normalizeForRclone only trims a leading /). The != "/" guards never match either, since the Windows root is \, so a bare root entry gets appended to every batch:

"dirs":["tv/MasterChef.US.S16E15.1080p.DSNP.WEB-DL.AAC2.0.H.264-RAWR","\tv","\\"]

One good entry, two that match nothing. This one also predates #846.

Worth noting the first three sites are confirmed working in production: since deploying, Failed to notify rclone VFS has gone from 16 in a night to 0, and the primary directory now comes through correctly forward-slashed. The remaining backslashes in that line were the only clue that a fourth site existed.

filepath.Dir at line 116 of the same file builds a real OS path for os.Stat and is deliberately untouched.

[fatbob01]

Added tests in d415aa2.

Rather than only unit-testing the four inline call sites, I moved the normalization into RefreshDir as well, in both implementations. My reasoning: I found those four sites one at a time, each from a stray backslash in a production log, so per-site fixes look fragile here. A new caller reintroduces the bug and nothing surfaces it, because vfs/forget reports success for a directory that does not resolve. Normalizing at the boundary means a missed site degrades to a working call instead of a silent no-op.

The caller-side fixes stay, since vfs_notifiers != "/" root guards can only be corrected at the caller.

ToVFSPath is a thin named wrapper over filepath.ToSlash so the intent has something to test. Three tests: forward-slash input unchanged on every platform, the platform-specific separator contract (backslash is a separator on Windows, a legal filename character on POSIX), and the boundary property that no Windows separator can reach rclone.

Also tidied the PR description. It previously carried a suggestion about using forget-only instead of forget+refresh for deletions. I have since instrumented the machine properly and the CPU spin I was attributing to refresh cost happens with zero files open and rclone doing no work, hours away from any repair, so that suggestion was not supported by the evidence and I have dropped it.

[javi11]

YOu add a new code but you didn't test the other paths

[fatbob01]

Sorry, here you go! Added tests for the changed paths in 184417b.

The importer refresh ancestry was an inline closure with no way to test it, so it is now refreshDirsFor, with a table test covering a nested release dir, a file one level under the mount root, a file at the root, no leading slash, and spaces. The nzbfilesystem one drives updateFileHealthOnError through a repair with a real coalescer and asserts the enqueued directory. The existing harness passes a nil coalescer, so that value was never observable.

checker.go is already covered by the test that came with #846, TestHealthChecker_NotifyRcloneVFS, which asserts a forward-slash directory reaches RefreshDir.

I left library_sync.go uncovered. That call sits inside SyncLibrary behind the orphan-ratio gate and the two-sync confirmation, so isolating it means standing up a repo and two sync passes for one path.Dir. It goes through the RefreshDir boundary test instead. Happy to add it if you would rather have it.

[javi11]

No CRITICAL or HIGH issues. Not blocking — safe to merge. Reviewed PR #849 at 184417b8 (matches PR head OID exactly).

Verification I ran

Check	Result
go vet (4 affected pkgs)	clean
go test -race -count=1 (4 pkgs)	all pass
GOOS=windows go build ./...	fails on pre-existing cgo dep (nntppool → rapidyenc), not this PR
POSIX byte-identity claim	independently confirmed — reimplemented old vs. new refreshDirsFor and ran 13 inputs (incl. /, "", ., double slashes, trailing slash, spaces, drive letters): output identical on every one

Security: nothing. No secrets, no injection surface, no new traversal exposure — ToVFSPath only rewrites the separator, and these strings already reached the same authenticated local RC endpoint unchanged. Function lengths, file sizes, nesting depth, error handling all within limits.

Findings

MEDIUM — the Windows behavior this PR fixes has zero CI coverage (.github/workflows/pull-request.yml:17, runs-on: ubuntu-latest only). Three tests were added; on the CI runner:

• vfs_notifier_test.go:79 — t.Skip (non-Windows)
• paths_test.go:57 — t.Skip (non-Windows)
• health_disabled_test.go:283 — t.Skip on Windows

So the separator property is never asserted anywhere. Compounding it: GOOS=windows go vet ./pkg/rclonecli fails locally on the pre-existing rapidyenc cgo dep, so it can't be checked by hand either. Fix: make it platform-independent — extract a pure toSlashSep(s string, sep byte) string and drive it with '\\' on all platforms — or add a windows-latest matrix entry to the test job.

LOW — duplicated normalization loop. pkg/rclonecli/client.go:437-443 and pkg/rclonecli/vfs_client.go:115-122 are the same 8 lines plus the same comment. Extract ToVFSPaths(dirs []string) []string into paths.go alongside ToVFSPath.

LOW — ToVFSPath isn't used by the four callers it exists for. All four sites hand-roll filepath.ToSlash; I confirmed all four already import pkg/rclonecli, so path.Dir(rclonecli.ToVFSPath(p)) costs no new dependency and puts the rule in one place — which is the stated point of the wrapper.

LOW — refreshDirsFor doc comment is inaccurate (vfs_notifier.go:110): "returns the directory and its nearest ancestors." dirs[0] is the path itself, which for a single-file import is a file (tv/Show/ep.mkv), not a directory. Pre-existing behavior, correctly preserved — just the new comment overstates it.

LOW (pre-existing) — normalize strips exactly one leading slash. strings.TrimPrefix(…, "/") at vfs_notifier.go:126 leaves //tv//Show//ep.mkv → /tv//Show//ep.mkv, i.e. a leading slash still reaches rclone; trailing slashes also survive (/tv/Show/ → tv/Show/). stdpath.Clean before the trim fixes both. Optional — it is a real behavior change for those inputs, and the PR is already the natural place to make it.

One aside: Coordinator.NotifyVFS (vfs_notifier.go:17) has no callers anywhere in the repo. Pre-existing dead code, out of scope, mentioned only because the PR touches that file.

[fatbob01]

Thanks for the detailed pass, all five addressed in 68cda44.

Windows coverage. Took your suggestion: toSlashSep(s string, sep byte) is now pure, so the tests drive it with '\' directly and the separator property is asserted on the ubuntu runner rather than skipped. ToVFSPathis that bound toos.PathSeparator`.

Duplicated loop. Now ToVFSPaths(dirs) in both RefreshDir bodies.

Callers hand-rolling it. All four use rclonecli.ToVFSPath now. That left path/filepath referenced only from a comment in checker.go, so that import is gone.

Doc comment. Corrected, it no longer calls the first entry a directory.

Leading slash. Cleaned. Worth flagging that this one had a trap: cleaning inside normalize while still walking the raw path makes Dir("/tv/Show/") equal "/tv/Show", so the first two entries came out identical. The new trailing-slash test caught it. Cleaning once before the walk fixes it, and both cases are covered.

Left Coordinator.NotifyVFS alone as out of scope, agreed.