From 1ec5a04e88fee288a84449834872ef67a6d866ac Mon Sep 17 00:00:00 2001 From: revtex Date: Fri, 28 Aug 2026 19:33:23 -0400 Subject: [PATCH] Stop looking up a recording that has been thrown away Resume Spotify on a song a fraction of a second before it ends - or start a new one from a stopped player, where the media session reports the previous track for a few hundred milliseconds - and Offstream starts a recording it discards a moment later for being under the minimum length. The metadata lookup that recording started carried on regardless. Nothing joined it: FinaliseAsync returns on the too-short and silent branches before AwaitEnrichmentAsync, deliberately, so a fragment never waits on a provider. Nothing stopped it either. So it spent the next several seconds asking Spotify about a song that had already finished, against a rate limit shared with the recording that replaced it, and then announced "had no metadata" for a file that was never written - which reads as the track matching having failed when it was working exactly as intended. Worse in the case where the answer carries no track at all: that lookup can run the thirty-attempt no-track budget out and stand the session's advertisement handling down, on the evidence of a track that had already ended. The lookup now belongs to the recording rather than to the session. The session creates a cancellation source per recording, linked to its own token, and hands it to the TrackRecorder with the task; the recorder cancels it on the branches where it decides no file will exist. It cannot hang off the track boundary instead. Every normal recording is finalised after its song has ended - that is when the tags are joined - so cancelling in StopCurrentRecorder would strip the tags off everything. Two smaller things travel with it: Cover art the lookup had already fetched is deleted when the recording is discarded. The lookup usually finishes before a short recording is decided, so the image was already on disk with nothing left to reference it, and only the already-recorded branch ever cleaned one up. CoverArtFetcher also deletes a half-written image when the fetch is cancelled mid-write, because the path it would be found by is lost with the exception - cancellation only became reachable there with this change. CancelEnrichment swallows ObjectDisposedException. A session torn down while a recording is still finalising disposes the recorder from one thread while FinaliseAsync decides on the other, and cancelling an already-disposed source would surface as "Recording X failed" on a recording that was merely being tidied away. Not fixed here, and the reason the fragment existed at all: the media session reports the previous track's properties with IsPlaying already true, so the song that actually started loses its first few hundred milliseconds to the discarded recorder's Prime(). That is a change to what audio a recording contains and wants deciding on its own. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 18 ++++ .../Metadata/CoverArtFetcher.cs | 28 +++++- src/Offstream.Core/Metadata/TrackEnricher.cs | 5 +- .../Recording/RecordingSession.cs | 20 +++- src/Offstream.Core/Recording/TrackRecorder.cs | 99 ++++++++++++++++++- .../Recording/RecordingSessionTests.cs | 51 +++++++++- .../Recording/TrackRecorderTests.cs | 93 ++++++++++++++++- 7 files changed, 302 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da48717..219ea20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,24 @@ phase plan these entries follow. ## [Unreleased] +### Fixed + +- **A recording that is thrown away no longer keeps looking its track up.** Resume Spotify on a + song a fraction of a second before it ends — or start a new one from a stopped player, where the + media session reports the previous track for a few hundred milliseconds — and Offstream starts a + recording it discards a moment later for being under the minimum length. The metadata lookup for + that fragment carried on regardless, because nothing joins a lookup for a recording that will + never exist and nothing used to stop one either: it spent several seconds asking Spotify about a + song that had already finished, against a rate limit shared with the recording that replaced it, + and then announced "had no metadata" for a file that was never written — which reads as the track + matching having failed when it was working exactly as intended. It is now cancelled the moment + the recording is discarded, and again when the session stops. +- **Cover art fetched for a discarded recording is deleted rather than left in the temp + directory.** The lookup usually finishes before a short recording is decided, so the image was + already on disk with nothing left to reference it; only the "kept the file already on disk" + branch ever cleaned one up. A half-written image from a fetch cancelled mid-download is removed + too, since the path it would be found by is lost with the cancellation. + ## [0.1.0] - 2026-08-15 The first release. Everything below is the whole of Offstream rather than a change to it: diff --git a/src/Offstream.Core/Metadata/CoverArtFetcher.cs b/src/Offstream.Core/Metadata/CoverArtFetcher.cs index 4e748fb..0585b70 100644 --- a/src/Offstream.Core/Metadata/CoverArtFetcher.cs +++ b/src/Offstream.Core/Metadata/CoverArtFetcher.cs @@ -81,7 +81,20 @@ public CoverArtFetcher(HttpClient httpClient, IFileSystem fileSystem) } var path = TempFileFor(uri); - await _fileSystem.File.WriteAllBytesAsync(path, image, cancellationToken); + + try + { + await _fileSystem.File.WriteAllBytesAsync(path, image, cancellationToken); + } + catch (OperationCanceledException) + { + // The path is about to be lost with the exception, so nothing downstream can ever + // delete what was written of it. Cancellation reaches here whenever the recording + // this art belongs to is discarded mid-fetch, which is common enough at a track + // boundary to be worth not leaving half an image in the temp directory each time. + TryDelete(path); + throw; + } return path; } @@ -99,6 +112,19 @@ public CoverArtFetcher(HttpClient httpClient, IFileSystem fileSystem) } } + /// Removes a file this fetch had started writing, if it got that far. + private void TryDelete(string path) + { + try + { + if (_fileSystem.File.Exists(path)) _fileSystem.File.Delete(path); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // A stray temp file is not worth failing a cancellation over. + } + } + /// A scratch path in the temp directory, keeping the URL's image extension. private string TempFileFor(Uri uri) { diff --git a/src/Offstream.Core/Metadata/TrackEnricher.cs b/src/Offstream.Core/Metadata/TrackEnricher.cs index 527f8fb..9ba6dfe 100644 --- a/src/Offstream.Core/Metadata/TrackEnricher.cs +++ b/src/Offstream.Core/Metadata/TrackEnricher.cs @@ -125,7 +125,10 @@ public async Task EnrichAsync(Track track, CancellationToken ca } catch (OperationCanceledException) { - // The session is stopping. Not worth a line of its own. + // The caller gave up on this lookup: the session is stopping, or the recording it + // belongs to has been discarded and there is nothing left to tag. Neither is worth a + // line of its own — and the second must not print "had no metadata", which would + // report a missing tag on a file that was never written. return TrackEnrichment.None; } #pragma warning disable CA1031 // A provider fault must not reach the recording; that is this class's job. diff --git a/src/Offstream.Core/Recording/RecordingSession.cs b/src/Offstream.Core/Recording/RecordingSession.cs index b007a5a..6dc0456 100644 --- a/src/Offstream.Core/Recording/RecordingSession.cs +++ b/src/Offstream.Core/Recording/RecordingSession.cs @@ -549,17 +549,33 @@ private void Consider(Track track) /// and the track plays for minutes, so overlapping them makes it free; doing it after the /// recording would add that second to every track before its file appears. /// + /// + /// And it is the recorder's to cancel. The lookup outlives the track it belongs to on + /// purpose — a finished recording is tagged after the song has ended — so it cannot be tied to + /// the track boundary. What it must not outlive is the recording itself: a fragment that is + /// discarded has nothing left to tag, and its lookup is stopped by + /// at the moment that is decided. The source is linked to the + /// session's own token so a teardown still ends it, and handed over with the task. + /// /// private void StartRecorder(Track detected, OutputPaths paths) { if (_buffer is null) return; var track = new Track(detected); - var enrichment = _enricher?.EnrichAsync(track, _stopping.Token); + + var enrichmentCancellation = _enricher is null + ? null + : CancellationTokenSource.CreateLinkedTokenSource(_stopping.Token); + + var enrichment = enrichmentCancellation is null + ? null + : _enricher!.EnrichAsync(track, enrichmentCancellation.Token); AnnounceWhenEnriched(track, paths, enrichment); - var recorder = new TrackRecorder(_buffer, _settings, track, paths, _fileSystem, enrichment); + var recorder = new TrackRecorder( + _buffer, _settings, track, paths, _fileSystem, enrichment, enrichmentCancellation); // Now, on the poll loop, not when the recording task gets scheduled: everything captured // after this instant belongs to the new track. diff --git a/src/Offstream.Core/Recording/TrackRecorder.cs b/src/Offstream.Core/Recording/TrackRecorder.cs index 520bbc3..912816c 100644 --- a/src/Offstream.Core/Recording/TrackRecorder.cs +++ b/src/Offstream.Core/Recording/TrackRecorder.cs @@ -74,6 +74,7 @@ public sealed class TrackRecorder : IDisposable private readonly OutputPaths _paths; private readonly Track _track; private readonly Task? _enrichment; + private readonly CancellationTokenSource? _enrichmentCancellation; private readonly CancellationTokenSource _stopping = new(); private readonly TaskCompletionSource _bufferDrained = @@ -81,6 +82,7 @@ public sealed class TrackRecorder : IDisposable private long _bytesWritten; private bool _primed; + private bool _disposed; /// The shared capture buffer this recording drains. /// The session's settings. @@ -93,13 +95,18 @@ public sealed class TrackRecorder : IDisposable /// just before the encode request is built — the last moment at which album, track number and /// cover art can still reach the file. /// + /// + /// The lookup's own cancellation, handed over with it. Cancelled by this recorder the moment + /// the recording is thrown away — see — and disposed with it. + /// public TrackRecorder( AudioCaptureBuffer buffer, RecordingSettings settings, Track track, OutputPaths paths, IFileSystem fileSystem, - Task? enrichment = null) + Task? enrichment = null, + CancellationTokenSource? enrichmentCancellation = null) { ArgumentNullException.ThrowIfNull(buffer); ArgumentNullException.ThrowIfNull(settings); @@ -113,6 +120,7 @@ public TrackRecorder( _paths = paths; _fileSystem = fileSystem; _enrichment = enrichment; + _enrichmentCancellation = enrichmentCancellation; } /// The track this recorder is capturing. @@ -224,6 +232,7 @@ public async Task RunAsync(CancellationToken cancellationToken = if (cancellationToken.IsCancellationRequested) { + AbandonEnrichment(); _paths.DeleteFile(tempWavePath); return new TrackRecording(RecordingOutcome.Cancelled, _track, Elapsed); } @@ -231,8 +240,90 @@ public async Task RunAsync(CancellationToken cancellationToken = return await FinaliseAsync(tempWavePath); } - /// Releases the stop signal. Recording itself ends with . - public void Dispose() => _stopping.Dispose(); + /// + /// Releases the stop signal and the lookup's cancellation. Recording itself ends with + /// . + /// + /// + /// The lookup is cancelled before its source is disposed, always. Disposing a + /// that has not been cancelled throws + /// out of the next Register on its token — which + /// is inside whatever HTTP call the lookup is making — while disposing one that has been + /// cancelled is the ordinary pattern. Every path that reaches a recording worth keeping has + /// already joined the lookup by the time this runs, so this only ever bites a recording that + /// is being abandoned. + /// + public void Dispose() + { + if (_disposed) return; + _disposed = true; + + _stopping.Dispose(); + + if (_enrichmentCancellation is null) return; + + CancelEnrichment(); + _enrichmentCancellation.Dispose(); + } + + /// + /// Stops the metadata lookup for a recording that is not going to exist, and tidies up + /// anything it fetched before it noticed. + /// + /// + /// + /// A discarded recording's lookup used to run to its own conclusion. Nothing joined + /// it — returns on the too-short and silent branches before + /// , deliberately, so a fragment never waits on a + /// provider — and nothing stopped it either. A track resumed a fraction of a second before it + /// ended therefore spent the next several seconds asking Spotify about a song that had + /// already finished, spending calls against a rate limit shared with the recording that + /// replaced it, and announcing "had no metadata" for a file that was never written. Worse in + /// the case where the answer carries no track at all: that lookup can run the no-track budget + /// out and stand this session's advertisement handling down, on the evidence of a track that + /// had already ended. + /// + /// + /// Cover art is deleted rather than left. The fetch may well have finished before the + /// cancellation reached it, and its temp file is referenced by nothing once this recording is + /// gone — only the already-recorded branch used to delete one, so every discarded recording + /// that had enriched in time leaked an image into the temp directory. + /// + /// + private void AbandonEnrichment() + { + CancelEnrichment(); + + if (_enrichment is null) return; + + _ = _enrichment.ContinueWith( + completed => TryDelete(completed.Result.CoverArtPath), + CancellationToken.None, + TaskContinuationOptions.OnlyOnRanToCompletion | TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + } + + /// + /// Ends the lookup, whether or not this recorder has already been disposed. + /// + /// + /// The two callers can arrive in either order: a session torn down while a recording is still + /// finalising disposes this recorder from one thread while is + /// deciding on the other. Cancelling a source that is already cancelled is a no-op, but + /// cancelling a disposed one throws — and that exception would come out of the recording task + /// as a failure the user is told about, on a recording that was merely being tidied away. + /// + private void CancelEnrichment() + { + try + { + _enrichmentCancellation?.Cancel(); + } + catch (ObjectDisposedException) + { + // Disposal got there first, and it cancels on the way past. Nothing left to do. + } + } private async Task CaptureAsync(WaveFileWriter writer, byte[] chunk, CancellationToken stopping) { @@ -288,12 +379,14 @@ private async Task FinaliseAsync(string tempWavePath) { // Not a failure of ours: Spotify is playing to an endpoint this session is not // capturing, which the shell reports as such rather than as an error. + AbandonEnrichment(); _paths.DeleteFile(tempWavePath); return new TrackRecording(RecordingOutcome.Silent, _track, duration); } if (duration.TotalSeconds < _settings.MinimumRecordedLengthSeconds) { + AbandonEnrichment(); _paths.DeleteFile(tempWavePath); return new TrackRecording(RecordingOutcome.TooShort, _track, duration); } diff --git a/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs b/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs index 238335f..50f8c48 100644 --- a/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs +++ b/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs @@ -115,14 +115,30 @@ private sealed class FakeEnricher : ITrackEnricher /// Which tracks were looked up, in order. public List Tracks { get; } = []; + /// Whether the lookup hangs, as a real one still chasing a provider would. + public bool NeverAnswers { get; set; } + + /// The token the session gave the most recent lookup. + public CancellationToken Token { get; private set; } + public Task EnrichAsync(Track track, CancellationToken cancellationToken = default) { Calls++; Tracks.Add(track.Title); + Token = cancellationToken; Apply?.Invoke(track); - return Task.FromResult(new TrackEnrichment(Updated: true, CoverArtPath)); + return NeverAnswers + ? NeverAsync(cancellationToken) + : Task.FromResult(new TrackEnrichment(Updated: true, CoverArtPath)); + } + + private static async Task NeverAsync(CancellationToken cancellationToken) + { + await Task.Delay(Timeout.Infinite, cancellationToken); + + return TrackEnrichment.None; } } @@ -559,6 +575,39 @@ await WaitFor( Assert.Empty(harness.Saved); } + /// + /// The lookup belongs to the recording rather than to the session, and it ends with it. + /// A discarded fragment has nothing left to tag, so the lookup it started is stopped instead + /// of being left to spend the next several seconds asking about a track that has already + /// ended — and the track that replaced it keeps a lookup of its own, which is what makes this + /// a per-recording cancellation rather than the session's. + /// + [Fact] + public async Task Session_DiscardingAShortRecording_CancelsOnlyThatTracksLookup() + { + var enricher = new FakeEnricher { NeverAnswers = true }; + + await using var harness = new Harness(s => s.MinimumRecordedLengthSeconds = 30, enricher: enricher); + + harness.Session.Start(); + + await RecordTrackAsync(harness, Harness.Playing("Artist", "Title"), bytes: 300); + + var discarded = enricher.Token; + + harness.Play(Harness.Playing("Artist", "Next")); + + await WaitFor( + () => harness.Recorded.Any(r => r.Outcome == RecordingOutcome.TooShort), + "the short recording to be discarded"); + + await WaitFor( + () => discarded.IsCancellationRequested, + "the discarded recording's lookup to be cancelled"); + + Assert.False(enricher.Token.IsCancellationRequested); + } + /// /// Nothing captured means Spotify is playing to a device this session is not recording — /// worth saying plainly, because the fix is a settings change rather than a retry. diff --git a/tests/Offstream.Core.Tests/Recording/TrackRecorderTests.cs b/tests/Offstream.Core.Tests/Recording/TrackRecorderTests.cs index 338ade3..2838c4b 100644 --- a/tests/Offstream.Core.Tests/Recording/TrackRecorderTests.cs +++ b/tests/Offstream.Core.Tests/Recording/TrackRecorderTests.cs @@ -55,7 +55,7 @@ public Harness( Track? track = null, ExistingFilePolicy policy = ExistingFilePolicy.Overwrite, string? template = null, - Func>? enrich = null) + Func>? enrich = null) { FileSystem = new MockFileSystem(); FileSystem.Directory.CreateDirectory(MusicRoot); @@ -68,7 +68,17 @@ public Harness( Track = track ?? SampleTrack(); Paths = new OutputPaths(Settings, Track, FileSystem, new DateTime(2026, 8, 12, 10, 0, 0, DateTimeKind.Utc)); - Recorder = new TrackRecorder(Buffer, Settings, Track, Paths, FileSystem, enrich?.Invoke(Track)); + // Created before the lookup so the lookup can observe it, exactly as the session does. + EnrichmentCancellation = enrich is null ? null : new CancellationTokenSource(); + + Recorder = new TrackRecorder( + Buffer, + Settings, + Track, + Paths, + FileSystem, + enrich?.Invoke(Track, EnrichmentCancellation!.Token), + EnrichmentCancellation); } public MockFileSystem FileSystem { get; } @@ -83,6 +93,9 @@ public Harness( public TrackRecorder Recorder { get; } + /// The lookup's cancellation, as the session hands it to the recorder. + public CancellationTokenSource? EnrichmentCancellation { get; } + public void Dispose() => Recorder.Dispose(); /// Feeds audio, then stops the recorder once it has all been consumed. @@ -181,7 +194,7 @@ public async Task Record_WithAnEnrichedTemplate_KeepsTheFileAlreadyOnDisk() using var harness = new Harness( policy: ExistingFilePolicy.Skip, template: Template, - enrich: track => + enrich: (track, _) => { track.Album = "Album"; track.Year = 1983; @@ -211,7 +224,7 @@ public async Task Record_WithAnEnrichedTemplateAndNothingOnDisk_Records() using var harness = new Harness( policy: ExistingFilePolicy.Skip, template: @"{artist}\({year}) {album}\{track:00} {title}", - enrich: track => + enrich: (track, _) => { track.Album = "Album"; track.Year = 1983; @@ -277,6 +290,78 @@ public async Task Record_WithNoAudioAtAll_ReportsSilentAndLeavesNothingBehind() Assert.DoesNotContain(harness.FileSystem.AllFiles, f => f.EndsWith(".tmp", StringComparison.Ordinal)); } + /// + /// A recording that is thrown away has nothing left to tag, so its lookup is stopped rather + /// than left to finish. It used to run on for seconds against a track that had already ended + /// — spending calls on a rate limit shared with the recording that replaced it, and reporting + /// a missing tag on a file that was never written. + /// + [Theory] + [InlineData(30, 500)] // Discarded as too short. + [InlineData(2, 0)] // Discarded as silent. + public async Task Record_Discarded_StopsTheMetadataLookup(int minimumSeconds, int bytes) + { + using var harness = new Harness( + minimumSeconds: minimumSeconds, + enrich: async (_, token) => + { + // A lookup still chasing a provider when the recording is decided. + await Task.Delay(Timeout.Infinite, token); + return TrackEnrichment.None; + }); + + await harness.RecordAsync(bytes); + + Assert.True(harness.EnrichmentCancellation!.IsCancellationRequested); + } + + /// + /// The lookup often finishes before the recording is decided, and its cover art is a temp + /// file that nothing else will ever reference once the recording is gone. Only the + /// already-recorded branch used to delete one, so every discarded recording that had enriched + /// in time left an image behind. + /// + [Fact] + public async Task Record_ShorterThanTheMinimum_DeletesCoverArtTheLookupAlreadyFetched() + { + const string CoverArt = @"C:\art\cover.jpg"; + + using var harness = new Harness( + minimumSeconds: 30, + enrich: (_, _) => Task.FromResult(new TrackEnrichment(Updated: true, CoverArt))); + + harness.FileSystem.Directory.CreateDirectory(@"C:\art"); + harness.FileSystem.File.WriteAllBytes(CoverArt, [1, 2, 3]); + + var recording = await harness.RecordAsync(bytes: 500); + + Assert.Equal(RecordingOutcome.TooShort, recording.Outcome); + Assert.False(harness.FileSystem.File.Exists(CoverArt)); + } + + /// + /// The other half of the rule: a recording that is kept still needs its art, and its lookup + /// must not be cancelled on the way to the encode request. + /// + [Fact] + public async Task Record_Captured_KeepsTheCoverArtAndTheLookup() + { + const string CoverArt = @"C:\art\cover.jpg"; + + using var harness = new Harness( + enrich: (_, _) => Task.FromResult(new TrackEnrichment(Updated: true, CoverArt))); + + harness.FileSystem.Directory.CreateDirectory(@"C:\art"); + harness.FileSystem.File.WriteAllBytes(CoverArt, [1, 2, 3]); + + var recording = await harness.RecordAsync(bytes: 500); + + Assert.Equal(RecordingOutcome.Captured, recording.Outcome); + Assert.Equal(CoverArt, recording.Encode!.CoverArtPath); + Assert.True(harness.FileSystem.File.Exists(CoverArt)); + Assert.False(harness.EnrichmentCancellation!.IsCancellationRequested); + } + /// /// Whatever is buffered when a track starts is the tail of the previous one; keeping it is /// how a recording opens with the last second of the song before it.