From 2240d80c1fc1e39e0e4f42f47ae1374954cb7f4d Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Thu, 20 Aug 2026 11:11:31 -0500 Subject: [PATCH] fix(import): actually embed the ASIN after an import Two independent faults stop the post-import ASIN write from reaching the file. The import reports success either way, because the enrichment step is deliberately non-fatal, so neither is visible without reading the destination file's tags. The stream is write-only. TagLibAudioTagWriter's file abstraction hands TagLib the lease's metadata write stream, which reaches OpenIndependentWriteStream and opens O_WRONLY on Unix, or NtCreateFile without GenericRead on Windows. Mpeg4.File.Save() parses the existing box headers through that same stream before it writes, so it throws NotSupportedException mid-parse. Open read+write instead, via a new UnixOpenFlags.OpenReadWriteNoFollow() alongside the existing OpenWriteNoFollow(). The tag lookup never matches on MPEG-4. ApplyAsinTag tests `file.Tag is AppleTag`, but an MPEG-4 file's Tag is a CombinedTag wrapping the Apple tag, so the branch is never taken. The Id3v2 and Xiph branches do not match an m4b either, so Save() rewrites an unchanged file and the writer logs success. Ask for the tag by type instead. Only MPEG-4 answers to Apple, so mp3 and flac fall through as before. The second fault matters for how the first is judged: fixing only the stream turns a logged failure into a silent one. That was observed on a build carrying just the stream change, not predicted. Pinning is unaffected. The handle is still opened relative to the pinned parent, still refuses to follow a link, and is still checked against the validated file object. Widening the access mode does not widen what the lease will open. Rebuilt on top of #828, which replaced the hardcoded open-flag constants with UnixOpenFlags. The new method inherits that commit's per-architecture noFollow detection rather than reintroducing a literal. --- .../PinnedAudiobookFileRegistrationLease.cs | 4 ++- ...PinnedDirectoryCreation.FileMove.Native.cs | 7 +++-- ...PinnedDirectoryCreation.FileOpenWindows.cs | 6 +++-- .../PinnedDirectoryCreation.FileStreams.cs | 15 ++++++++--- .../FileSystem/UnixOpenFlags.cs | 14 ++++++++++ .../Library/Files/TagLibAudioTagWriter.cs | 6 ++++- ...nnedAudiobookFileRegistrationLeaseTests.cs | 26 +++++++++++++++++++ .../FileSystem/UnixOpenFlagsTests.cs | 15 +++++++++++ 8 files changed, 83 insertions(+), 10 deletions(-) diff --git a/listenarr.infrastructure/FileSystem/PinnedAudiobookFileRegistrationLease.cs b/listenarr.infrastructure/FileSystem/PinnedAudiobookFileRegistrationLease.cs index e217793e0..1a91653fb 100644 --- a/listenarr.infrastructure/FileSystem/PinnedAudiobookFileRegistrationLease.cs +++ b/listenarr.infrastructure/FileSystem/PinnedAudiobookFileRegistrationLease.cs @@ -65,7 +65,9 @@ public Stream OpenMetadataWriteStream() "Pinned path-only registration leases do not authorize metadata writes."); } - return _file.OpenIndependentWriteStream( + // Read+write, because the only consumer is a tag library that parses the container it + // is about to rewrite through this same stream. + return _file.OpenIndependentReadWriteStream( bufferSize: 128 * 1024, asynchronous: false); } diff --git a/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileMove.Native.cs b/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileMove.Native.cs index 40c265ff9..a0f2e5cce 100644 --- a/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileMove.Native.cs +++ b/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileMove.Native.cs @@ -101,12 +101,15 @@ private static SafeFileHandle OpenRelativeFileUnix( private static SafeFileHandle OpenRelativeFileForWriteUnix( SafeFileHandle parentHandle, string fileName, - string fullPath) + string fullPath, + bool readable = false) { var fd = OpenAt( parentHandle.DangerousGetHandle().ToInt32(), fileName, - UnixOpenFlags.OpenWriteNoFollow(), + readable + ? UnixOpenFlags.OpenReadWriteNoFollow() + : UnixOpenFlags.OpenWriteNoFollow(), mode: 0); if (fd >= 0) { diff --git a/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileOpenWindows.cs b/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileOpenWindows.cs index e0b4a7644..d3acdc071 100644 --- a/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileOpenWindows.cs +++ b/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileOpenWindows.cs @@ -259,7 +259,8 @@ private static SafeFileHandle OpenRelativeFileStableDeleteWindows( private static SafeFileHandle OpenRelativeFileForWriteWindows( SafeFileHandle parentHandle, string fileName, - string fullPath) + string fullPath, + bool readable = false) { var nameBuffer = Marshal.StringToHGlobalUni(fileName); var unicodeStringPointer = IntPtr.Zero; @@ -281,7 +282,8 @@ private static SafeFileHandle OpenRelativeFileForWriteWindows( }; var status = NtCreateFile( out var rawHandle, - GenericWrite | FileReadAttributes | Synchronize, + (readable ? GenericRead : 0u) + | GenericWrite | FileReadAttributes | Synchronize, ref attributes, out _, IntPtr.Zero, diff --git a/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileStreams.cs b/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileStreams.cs index ff5b2cb4d..f154d01a4 100644 --- a/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileStreams.cs +++ b/listenarr.infrastructure/FileSystem/PinnedDirectoryCreation.FileStreams.cs @@ -23,21 +23,28 @@ internal FileStream OpenIndependentReadStream(int bufferSize, bool asynchronous) asynchronous); } - internal FileStream OpenIndependentWriteStream(int bufferSize, bool asynchronous) + // Tag libraries rewrite a container in place: they parse the existing box or frame + // structure through the same stream they then write back to, so a write-only handle + // fails while still reading. The pinning is unaffected by the wider access mode — the + // handle is still opened relative to the pinned parent, still refuses to follow a + // link, and is still checked against the validated file object below. + internal FileStream OpenIndependentReadWriteStream(int bufferSize, bool asynchronous) { ThrowIfDisposed(); var handle = OperatingSystem.IsWindows() ? OpenRelativeFileForWriteWindows( _parentHandle, _fileName, - FullPath) + FullPath, + readable: true) : OpenRelativeFileForWriteUnix( _parentHandle, _fileName, - FullPath); + FullPath, + readable: true); return OpenVerifiedIndependentStream( handle, - FileAccess.Write, + FileAccess.ReadWrite, bufferSize, asynchronous); } diff --git a/listenarr.infrastructure/FileSystem/UnixOpenFlags.cs b/listenarr.infrastructure/FileSystem/UnixOpenFlags.cs index b43ac61e5..d63764dd2 100644 --- a/listenarr.infrastructure/FileSystem/UnixOpenFlags.cs +++ b/listenarr.infrastructure/FileSystem/UnixOpenFlags.cs @@ -76,6 +76,20 @@ internal static int OpenWriteNoFollow() return LinuxWriteOnly | noFollow | LinuxCloseOnExec; } + // For a caller that has to read the file back through the same descriptor it writes to. + // Tag libraries do this: they parse the existing container through the stream they are + // about to rewrite, so a write-only descriptor fails partway through the parse. + internal static int OpenReadWriteNoFollow() + { + if (IsMacOSHost()) + { + return MacReadWrite | MacNoFollow | MacCloseOnExec; + } + + var (_, noFollow) = GetCurrentLinuxDirectorySafetyFlags(); + return LinuxReadWrite | noFollow | LinuxCloseOnExec; + } + internal static int CreateWriteExclusiveNoFollow() { if (IsMacOSHost()) diff --git a/listenarr.infrastructure/Library/Files/TagLibAudioTagWriter.cs b/listenarr.infrastructure/Library/Files/TagLibAudioTagWriter.cs index 5ae3d6d64..2b1dc5515 100644 --- a/listenarr.infrastructure/Library/Files/TagLibAudioTagWriter.cs +++ b/listenarr.infrastructure/Library/Files/TagLibAudioTagWriter.cs @@ -90,7 +90,11 @@ public Task WriteAsinTagAsync( private static void ApplyAsinTag(TagLib.File file, string asin) { - if (file.Tag is TagLib.Mpeg4.AppleTag appleTag) + // An MPEG-4 file's Tag is a CombinedTag wrapping the Apple tag, never the AppleTag + // itself, so a type test on file.Tag matches nothing here and the save that follows + // writes an unchanged file while still reporting success. The tag has to be asked + // for by type. Only MPEG-4 answers to Apple, so mp3 and flac fall through as before. + if (file.GetTag(TagLib.TagTypes.Apple, create: true) is TagLib.Mpeg4.AppleTag appleTag) appleTag.SetDashBox("com.apple.iTunes", "ASIN", asin); else if (file.GetTag(TagLib.TagTypes.Id3v2) is TagLib.Id3v2.Tag id3Tag) { diff --git a/tests/Features/Infrastructure/FileSystem/PinnedAudiobookFileRegistrationLeaseTests.cs b/tests/Features/Infrastructure/FileSystem/PinnedAudiobookFileRegistrationLeaseTests.cs index 43617c61f..712dd0477 100644 --- a/tests/Features/Infrastructure/FileSystem/PinnedAudiobookFileRegistrationLeaseTests.cs +++ b/tests/Features/Infrastructure/FileSystem/PinnedAudiobookFileRegistrationLeaseTests.cs @@ -93,6 +93,32 @@ public async Task OpenMetadataWriteStream_PublicPathReplaced_DoesNotOpenReplacem await File.ReadAllTextAsync(displacedPath)); } + [Fact] + public async Task OpenMetadataWriteStream_IsReadableSoATagLibraryCanParseWhatItRewrites() + { + var parent = FileService.GetTempDirectory( + "registration-lease-metadata-write-access"); + var publicPath = await FileService.GetFileAsync( + parent, + "book.m4b", + "original generation"); + using var lease = PinnedAudiobookFileRegistrationLease.Open(publicPath); + + using var stream = lease.OpenMetadataWriteStream(); + + // TagLib reads the existing box structure back through the write stream before it + // saves, so a write-only handle throws NotSupportedException mid-parse and the tag is + // never written. The import still reports success, which is why this needs asserting + // rather than observing. + Assert.True(stream.CanWrite); + Assert.True(stream.CanRead); + Assert.True(stream.CanSeek); + + var buffer = new byte[stream.Length]; + Assert.Equal(buffer.Length, stream.Read(buffer, 0, buffer.Length)); + Assert.Equal("original generation", System.Text.Encoding.UTF8.GetString(buffer)); + } + [WindowsFact] public async Task StableRegistrationLease_BlocksPublicPathReplacementUntilDisposed() { diff --git a/tests/Features/Infrastructure/FileSystem/UnixOpenFlagsTests.cs b/tests/Features/Infrastructure/FileSystem/UnixOpenFlagsTests.cs index 28334d037..dec74cfae 100644 --- a/tests/Features/Infrastructure/FileSystem/UnixOpenFlagsTests.cs +++ b/tests/Features/Infrastructure/FileSystem/UnixOpenFlagsTests.cs @@ -48,6 +48,21 @@ public void GetLinuxDirectorySafetyFlags_RejectsArchitecturesListenarrDoesNotRel UnixOpenFlags.GetLinuxDirectorySafetyFlags(architecture)); } + [Fact] + public void OpenReadWriteNoFollow_AsksForReadAccessWhereOpenWriteDoesNot() + { + // Given / When + var writeOnly = UnixOpenFlags.OpenWriteNoFollow(); + var readWrite = UnixOpenFlags.OpenReadWriteNoFollow(); + + // Then: the access mode is the low two bits, and only that differs. Asserting the + // difference rather than a literal keeps this honest on both released architectures, + // since the noFollow bit is not the same value on arm64 and x64. + Assert.Equal(1, writeOnly & 0x3); + Assert.Equal(2, readWrite & 0x3); + Assert.Equal(writeOnly & ~0x3, readWrite & ~0x3); + } + [Fact] public void EnsureMacOSArchitectureSupported_AcceptsReleasedX64Target() {