Stop looking up a recording that has been thrown away

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:

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)
         }
     }
 
+    /// <summary>Removes a file this fetch had started writing, if it got that far.</summary>
+    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.
+        }
+    }
+
     /// <summary>A scratch path in the temp directory, keeping the URL's image extension.</summary>
     private string TempFileFor(Uri uri)
     {

src/Offstream.Core/Metadata/TrackEnricher.cs

@@ -125,7 +125,10 @@ public async Task<TrackEnrichment> 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.

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.
     /// </para>
+    /// <para>
+    /// <b>And it is the recorder's to cancel.</b> 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
+    /// <see cref="TrackRecorder"/> 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.
+    /// </para>
     /// </remarks>
     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.

src/Offstream.Core/Recording/TrackRecorder.cs

@@ -74,13 +74,15 @@ public sealed class TrackRecorder : IDisposable
     private readonly OutputPaths _paths;
     private readonly Track _track;
     private readonly Task<TrackEnrichment>? _enrichment;
+    private readonly CancellationTokenSource? _enrichmentCancellation;
 
     private readonly CancellationTokenSource _stopping = new();
     private readonly TaskCompletionSource _bufferDrained =
         new(TaskCreationOptions.RunContinuationsAsynchronously);
 
     private long _bytesWritten;
     private bool _primed;
+    private bool _disposed;
 
     /// <param name="buffer">The shared capture buffer this recording drains.</param>
     /// <param name="settings">The session's settings.</param>
@@ -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.
     /// </param>
+    /// <param name="enrichmentCancellation">
+    /// The lookup's own cancellation, handed over with it. Cancelled by this recorder the moment
+    /// the recording is thrown away — see <see cref="AbandonEnrichment"/> — and disposed with it.
+    /// </param>
     public TrackRecorder(
         AudioCaptureBuffer buffer,
         RecordingSettings settings,
         Track track,
         OutputPaths paths,
         IFileSystem fileSystem,
-        Task<TrackEnrichment>? enrichment = null)
+        Task<TrackEnrichment>? 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;
     }
 
     /// <summary>The track this recorder is capturing.</summary>
@@ -224,15 +232,98 @@ public async Task<TrackRecording> RunAsync(CancellationToken cancellationToken =
 
         if (cancellationToken.IsCancellationRequested)
         {
+            AbandonEnrichment();
             _paths.DeleteFile(tempWavePath);
             return new TrackRecording(RecordingOutcome.Cancelled, _track, Elapsed);
         }
 
         return await FinaliseAsync(tempWavePath);
     }
 
-    /// <summary>Releases the stop signal. Recording itself ends with <see cref="Stop"/>.</summary>
-    public void Dispose() => _stopping.Dispose();
+    /// <summary>
+    /// Releases the stop signal and the lookup's cancellation. Recording itself ends with
+    /// <see cref="Stop"/>.
+    /// </summary>
+    /// <remarks>
+    /// <b>The lookup is cancelled before its source is disposed, always.</b> Disposing a
+    /// <see cref="CancellationTokenSource"/> that has not been cancelled throws
+    /// <see cref="ObjectDisposedException"/> out of the next <c>Register</c> 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.
+    /// </remarks>
+    public void Dispose()
+    {
+        if (_disposed) return;
+        _disposed = true;
+
+        _stopping.Dispose();
+
+        if (_enrichmentCancellation is null) return;
+
+        CancelEnrichment();
+        _enrichmentCancellation.Dispose();
+    }
+
+    /// <summary>
+    /// Stops the metadata lookup for a recording that is not going to exist, and tidies up
+    /// anything it fetched before it noticed.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// <b>A discarded recording's lookup used to run to its own conclusion.</b> Nothing joined
+    /// it — <see cref="FinaliseAsync"/> returns on the too-short and silent branches before
+    /// <see cref="AwaitEnrichmentAsync"/>, 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.
+    /// </para>
+    /// <para>
+    /// <b>Cover art is deleted rather than left.</b> 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.
+    /// </para>
+    /// </remarks>
+    private void AbandonEnrichment()
+    {
+        CancelEnrichment();
+
+        if (_enrichment is null) return;
+
+        _ = _enrichment.ContinueWith(
+            completed => TryDelete(completed.Result.CoverArtPath),
+            CancellationToken.None,
+            TaskContinuationOptions.OnlyOnRanToCompletion | TaskContinuationOptions.ExecuteSynchronously,
+            TaskScheduler.Default);
+    }
+
+    /// <summary>
+    /// Ends the lookup, whether or not this recorder has already been disposed.
+    /// </summary>
+    /// <remarks>
+    /// 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 <see cref="FinaliseAsync"/> 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.
+    /// </remarks>
+    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<TrackRecording> 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);
         }

tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs

@@ -115,14 +115,30 @@ private sealed class FakeEnricher : ITrackEnricher
         /// <summary>Which tracks were looked up, in order.</summary>
         public List<string?> Tracks { get; } = [];
 
+        /// <summary>Whether the lookup hangs, as a real one still chasing a provider would.</summary>
+        public bool NeverAnswers { get; set; }
+
+        /// <summary>The token the session gave the most recent lookup.</summary>
+        public CancellationToken Token { get; private set; }
+
         public Task<TrackEnrichment> 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<TrackEnrichment> NeverAsync(CancellationToken cancellationToken)
+        {
+            await Task.Delay(Timeout.Infinite, cancellationToken);
+
+            return TrackEnrichment.None;
         }
     }
 
@@ -559,6 +575,39 @@ await WaitFor(
         Assert.Empty(harness.Saved);
     }
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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);
+    }
+
     /// <summary>
     /// 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.