Repository navigation
Conversation
0b9f65a to
ae14f3f
Compare
|
I have cut this PR down. #901 covers half of what it was doing, and covers it better than I did. This originally fixed two things behind the same #901 fixes the size. It opens the metadata path and takes the stream length unconditionally. Mine read the length through the lease's generation-bound stream and then fell back to Worth adding that my size half had no test of its own here either. Both test files on this branch cover the scan boundary, not the size. So the size change is gone and the two files it touched are back at canary. What is leftOne production change, in The scan passed the lease's metadata path as both the byte source and the media identity. Those are deliberately separate things: the lease exists to give stable byte access while the public path stays the file's real identity, and the single-path overload collapses the two. On Linux the metadata path is an extensionless That is a different symptom from a wrong size. A file with this problem has no embedded metadata read at all, rather than metadata plus a bad length, so #901 landing does not make it go away. ChecksFull suite unfiltered: 3039 passed, 0 failed, 127 skipped, which is the canary baseline. Both builds clean at 0 warnings. The test changes here modify existing scan tests rather than adding new ones, so the count does not move. Control: reverting Test-merged against canary, against #901 and against #902: all three clean. I have not compared my scan change against anything in #902 beyond the merge, so if it overlaps in intent rather than in text, say so and I will look again. |
… rules CI on ubuntu-24.04 ran the backend suite for the first time: 3 failures out of 3264, all from merged PRs that had not been through upstream review. - Conform AudiobookMetadataRefreshServiceTests (Listenarrs#781), FfprobeTagMetadataMapperTests (Listenarrs#781) and SabnzbdResponseMapperTests (Listenarrs#840) to TestClasses_FollowRepositoryConventions. - Split the claim diagnostics helpers out of AudiobookFileService.cs, which Listenarrs#849 and Listenarrs#781 together pushed to 503 lines against the 500-line cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The registration lease deliberately separates stable byte access (ReadPath)
from public media identity (PublicPath). On Linux the lease's metadata path
is a /proc/{pid}/fd/{fd} descriptor link, and two consumers treat it as if
it were the file.
The scan's embedded-metadata pass called the single-path overload of
ExtractFileMetadataAsync, which builds MetadataFileSource(path, path). The
probe guard tests the public half for an audio extension, a descriptor link
has none, and so the candidate was rejected before ffprobe ran. That pass is
the fallback for candidates path attribution could not claim, so on Linux a
correctly tagged file in an unrecognised folder shape could never be claimed
by any route.
The registered length was stat'ed from the same descriptor path. Stat on the
link reports the length of the link rather than of its target, a constant 64
bytes, so every registered file on Linux recorded Size = 64. Reading the
length from the pinned handle keeps the lease's generation guarantee, since
it never consults the visible path.
Refs Listenarrs#818
Both of these mocked only the single-path overload of ExtractFileMetadataAsync, so once the scan routes through MetadataFileSource the strict mock saw no matching setup, the extractor returned nothing, and every file read as unreadable. That turned a passing suite into two failures that looked like behaviour regressions and were not. ScanAsync_CaseDistinctMetadataFolders_RemainConflicting just needed the overload. ScanAsync_MetadataReplacementAndRestore_ReadsPinnedFileGeneration needed the overload plus a decision about which half of the source its callback reads. It reads ReadPath, because the point of the test is that the scan sees the original generation even while the visible file is swapped underneath it. It now also asserts the other half. PublicPath must still be the candidate as a person sees it, extension included, because on Linux ReadPath is a /proc descriptor link with no extension and anything deriving media identity from it loses the extension entirely. Confirmed load-bearing by collapsing both halves onto the descriptor path: the assertion fails with the real path expected and /proc/<pid>/fd/<fd> observed, which is the defect this branch exists to fix, previously only demonstrable against a running container.
…rrs#901 This PR fixed two things behind the same /proc descriptor link: the probe's extension guard rejecting the candidate, and FileInfo reporting the link's own 64 bytes instead of the file's length. Listenarrs#901 fixes the size half, and fixes it better. It opens the metadata path and takes the stream length unconditionally. The version here read the length through the lease's generation-bound stream but fell back to FileInfo on the metadata path when the lease did not expose one, which on Linux is the original 64-byte bug again. That half also had no test of its own here; both test files in this branch cover the scan boundary. So the size change is dropped and the two shared files go back to canary. What remains is the half Listenarrs#901 does not touch: the scan passed the lease's metadata path as both the byte source and the media identity, and on Linux that path is an extensionless /proc link, so the audio-extension guard rejected the candidate before ffprobe ever ran. Passing the candidate as the identity alongside the metadata path as the byte source keeps the guard working on the real filename. The two PRs no longer touch a file in common.
2af63b9 to
0d4e045
Compare
Summary
On Linux a registration lease's metadata path is a
/proc/{pid}/fd/{fd}descriptor link rather than a path to the file, and two separate readers treated it as though it were one. This fixes the first: the probe's audio-extension guard rejects a candidate whose public half has no extension, so the embedded-metadata pass never runs.The second reader is
FileInfoon the link, which reports the length of the link rather than of the file. That is #821 and it is not fixed here; see the note below.Fixes #818. Rebased onto
03958c15as promised on #818 once #819 landed.This originally claimed #821 as well and no longer does. The size-site half was split out of this branch and is not in the current diff, so leaving the claim in place would have auto-closed that issue on merge with nothing here addressing it. The size-site half is covered by #901, which handles both sites rather than the one this branch used to touch.
The mechanism was worked out jointly with @kevinroberts on that thread rather than by me alone. He reproduced it independently on unpatched canary, withdrew his own suggested validation case once it turned out not to exercise the pass it was meant to test, and rewrote the issue around the root cause. The second of those is the reason the check below varies which agreements break instead of asserting one construction.
Changes
Fixed
AudiobookScanService.Metadata.cspassesnew MetadataFileSource(pinnedMetadataFile.MetadataPath, candidate)instead of collapsing both halves onto the descriptor path. The read path stays the pinned generation; the public path keeps the real filename, so the guard sees an audio extension and ffprobe runs.The pinning guarantee is unchanged. The read path stays the pinned generation, which is the property the lease exists to provide.
Testing
Two existing tests needed to follow the read onto the two-part source. Both mocked only the single-path overload, so a strict mock saw no matching setup once the scan routed through
MetadataFileSource, the extractor returned nothing, and every file read as unreadable.ScanAsync_MetadataReplacementAndRestore_ReadsPinnedFileGenerationneeded a decision rather than a swap. Its callback readsReadPath, because the point of the test is that the scan sees the original generation while the visible file is swapped underneath it. It now also assertsPublicPathis still the candidate with its extension intact, and that assertion is load-bearing: collapsing both halves onto the descriptor path fails it with the real path expected and/proc/<pid>/fd/<fd>observed. That is this defect, in a unit test, where before it was only reachable through a running container.Full suite: 3,029 passed, 0 failed, 125 skipped, which is the baseline on
03958c15unchanged, since this modifies two existing tests rather than adding any.Reproduced before and after against
ghcr.io/listenarrs/listenarr:canaryand a build of this branch, with a public check that varies which agreements are broken rather than asserting a single case:Both agreements have to break before anything reaches the pass, which is why the two single-mismatch rows are controls rather than filler: each rescues the other, so a check that only broke the filename would never exercise the path it was written to test.
What this does not fix
Worth being explicit, since the branch touches the neighbourhood of three other issues.
#542 is untouched, and out of scope here now that this branch changes no size value. The book-level total still reads "not set" on both builds. #901 corrects the per-file rows; whatever should sum them is a separate defect and I have not looked at it here.
#822 is only partly addressed, and not by this. #819 added an early return recording
MetadataEnrichmentSkippedLimitedStorage, which covers the limited-storage case. A probe refusal on storage that passes the generation check still commitsCompletedwith nothing recorded and no diagnostic. That is what the matrix rows above show as UNCLAIMED with no diagnostic, and it stays true after this change for any other reason a probe might refuse.I verified #819's early return does not shadow this: it returns at
:25and the patched call is at:73, so on storage with durable generation proof execution still reaches it.On the scope
This asked earlier whether you would rather have the two issues as two PRs. They are two PRs now: this one is the metadata-fallback path, and the size sites are #901's. So there is nothing to split and no answer needed.