Skip to content

Dispose the source info the processing creates itself - #7

Merged
Kentarohakase merged 1 commit into
mainfrom
fix/source-info-subscription-leak
Aug 12, 2026
Merged

Dispose the source info the processing creates itself#7
Kentarohakase merged 1 commit into
mainfrom
fix/source-info-subscription-leak

Conversation

@Kentarohakase

Copy link
Copy Markdown
Owner

Stacked on #6. Not from the improvement plan — this came out of the survey that produced it, and it is a real leak rather than a cleanup.

The leak

AudioInfo subscribes to LocalizationService.Instance.PropertyChanged in its constructor so its display strings follow a language change, and it unsubscribes only in Dispose():

public AudioInfo()
{
    LocalizationService.Instance.PropertyChanged += OnLocalizationChanged;
}

LocalizationService.Instance is a process-lifetime singleton, so an AudioInfo that nobody disposes stays reachable through that subscription until the app closes.

AudioProcessingService.ProcessAsync analyses the source itself when the caller supplies none:

var sourceInfo = options.SourceInfo;
if (sourceInfo is null)
{
    var analysis = await _ffprobeService.AnalyzeAsync(...);
    ...
    sourceInfo = analysis.Value;   // assigned to a local, never disposed
}

Every run down that path left one behind. They are not merely retained: each one keeps receiving OnLocalizationChanged for every later language switch, so the cost grows with the number of runs.

The fix

The remainder of ProcessAsync moves into RenderAsync, which lets the created instance be held in a using without re-indenting ninety lines of an untested method:

var sourceInfo = options.SourceInfo;
if (sourceInfo is not null)
{
    // Supplied by the caller, so it belongs to the queue item and is disposed with it.
    return await RenderAsync(options, sourceInfo, progress, cancellationToken);
}

...
using var ownedSourceInfo = analysis.Value;
return await RenderAsync(options, ownedSourceInfo, progress, cancellationToken);

Ownership is the point of the split: an AudioInfo handed in through the options belongs to the BatchProcessingItem and must not be disposed here, so only the one created locally is.

ProcessAsync had no tests, so the extraction was verified to be a pure move — git diff -w shows nothing but the new wrapper, with the entire body from EnsureSufficientDiskSpace onward untouched.

Scope check

I looked for the same pattern elsewhere. FFprobeService and AudioDiagnosticsService create instances and hand ownership to their caller, which is correct. AudioInfo.WithSelectedAudioStream returns a new instance into BatchProcessingItem.SetAudioInfo, which disposes the previous one and is guarded by a ReferenceEquals check, so the no-streams case that returns this cannot dispose and store the same instance at once. That guard is now covered by a test, since it is load-bearing and easy to remove by accident.

A note on the new test

Dispose_StopsListeningForLanguageChanges passed in isolation but failed in the full run: it assumed the process was not already on English, and assigning the same culture raises no change at all. It now establishes a known starting culture first. Worth mentioning because it is the same class of cross-test coupling that #4 addressed — that one was a race, this one was an ordering assumption.

Verification

dotnet format .\AudioQualityEnhancer.slnx --verify-no-changes   -> clean
dotnet build  .\AudioQualityEnhancer.slnx -c Release            -> 0 warnings, 0 errors
dotnet test   .\AudioQualityEnhancer.slnx -c Release            -> 267 passed, 0 failed

No changelog entry

The leak is invisible until a long session with many runs, and no behaviour a user can point at changes.

AudioInfo subscribes to the localization service in its constructor so its
display strings follow a language change, and it unsubscribes only when it is
disposed. That service lives for the whole session, so an instance nobody
disposes stays reachable through it until the app closes.

ProcessAsync analyses the source itself when the caller supplies none, and that
instance was assigned to a local and then dropped. Every run down that path left
one behind, and each one kept getting notified on every later language change.

The rest of the method moves into RenderAsync so the created instance can be
held in a using without re-indenting ninety lines. The caller-supplied instance
takes the same path but is not disposed there, because it belongs to the queue
item that hands it in.

The new test pins the contract that makes disposal matter: a disposed AudioInfo
no longer reacts to a language change.
@Kentarohakase
Kentarohakase changed the base branch from refactor/shared-audio-thresholds to main August 12, 2026 01:10
@Kentarohakase
Kentarohakase force-pushed the fix/source-info-subscription-leak branch from aba512b to 2329e41 Compare August 12, 2026 01:10
@Kentarohakase
Kentarohakase merged commit 60efebe into main Aug 12, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant