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.