Unsubscribe the ProcessExit handler on dispose

[matt-edmondson]

Fixes #311

The defect

EnsureDisposeOnExit subscribed a lambda to AppDomain.CurrentDomain.ProcessExit and kept no reference to it:

AppDomain.CurrentDomain.ProcessExit += (sender, e) => Dispose();

The lambda closes over this, and AppDomain.CurrentDomain lives for the whole process — so the instance was rooted from the moment it first queued a save. With no reference to the exact delegate there was nothing to pass to -=, so Dispose could not release it either.

Invisible for the common case of one Get() singleton. Not invisible for the multi-instance use the library explicitly supports and tests: a long-running process creating instances through LoadOrCreate(subdirectory) / LoadOrCreate(fileName) — one per save slot, profile or document — accumulates permanently rooted instances in proportion to churn, which is the thing implementing IDisposable on this type is meant to prevent.

The change

The delegate is held in a field and removed in Dispose(bool disposing).

Two decisions worth stating, since both are visible in the diff:

IsDisposeRegistered deliberately stays true after disposal. The existing TestDisposeCanBeCalledMultipleTimes already pins this ("IsDisposeRegistered should remain true after multiple disposals"), and keeping it set is also the safer behaviour: it means a later QueueSave on a disposed instance cannot re-enter the if (!IsDisposeRegistered) branch and root it again, this time with a handler whose Dispose() is a no-op. Resetting the flag would have reintroduced the leak on a narrower path.

The unsubscribe sits outside the disposing check, inside !disposedValue. It touches only AppDomain.CurrentDomain, a static that outlives every instance, so it is safe to reach from a finalizer. In practice the finalizer path is unreachable while a handler is registered — a rooted object is never finalized — but making it conditional on disposing would encode the opposite assumption for no benefit.

Test

TestDisposeReleasesTheProcessExitHandlerSoTheInstanceCanBeCollected asserts the thing that was actually wrong — reachability — rather than the shape of the fix. It queues a save (which is what registers the handler), disposes, drops the reference and collects, then asserts a WeakReference to it is dead.

A test that merely asserted a private field was null would pass against a fix that nulled the field without unsubscribing, which is the obvious way to get this wrong.

The instance is created and dropped inside a [MethodImpl(MethodImplOptions.NoInlining)] helper. Without that, it can stay alive in a local slot the JIT has not yet reported dead, and the weak reference survives for reasons unrelated to the event handler — a false pass in Debug and a flaky one in Release.

Proved failing without the fix. Reverting only AppDataStorage/AppData.cs and keeping the test:

failed TestDisposeReleasesTheProcessExitHandlerSoTheInstanceCanBeCollected (63ms)
  Assertion failed. Expected condition to be false.
  A disposed instance should not still be rooted by the ProcessExit handler.
  Assert.IsFalse(reference.IsAlive)

  total: 104   failed: 1   succeeded: 103

It fails on the instance still being reachable — the leak itself. The 103 pre-existing tests are unaffected in both directions, including the two that pin IsDisposeRegistered.

Verification

• dotnet build AppDataStorage.sln -c Release — succeeded, 0 errors
• dotnet test AppDataStorage.sln -c Release — 104 total, 104 passed, 0 failed, 0 skipped
• Same suite against reverted AppData.cs — 1 of 104 failed, as above

Run on .NET SDK 10.0.401, Linux.

The build emits 3 warnings, all System.Text.Encodings.Web / System.IO.Pipelines / System.Text.Json 10.0.12 reporting no net7.0 support for the library's net7.0 leg. They are pre-existing and unrelated — they reproduce identically with this change reverted, and concern transitive package TFM support rather than anything in this diff.

Not in this change

#310 — the interrupted-write window in WriteText/ReadText — is untouched. Same file, entirely separate fault.

🤖 Generated with Claude Code

https://claude.ai/code/session_012Pup1E96GmvMiRTNMdABat

---

Generated by Claude Code

[matt-edmondson]

Checked this one, and it stays as written — reasoning on the companion alert at line 502, which flags the second GC.Collect() in the same three-call sequence.

---

Generated by Claude Code

[matt-edmondson]

Verified, and the call sequence stays. The rule is right about the pattern and wrong about this use, and the suggested replacement would remove what the test measures.

The alert is a true positive on the pattern. cs/call-to-gc exists because forcing a collection in production code is a performance anti-pattern — you pay a full GC to get behaviour the runtime would have managed better itself. That reasoning does not reach a test whose assertion is reachability.

There is no alternative API. The test asserts that a disposed AppData<T> is no longer rooted by the ProcessExit handler — the leak this PR fixes. The only way to observe that from managed code is to force a collection and then check a WeakReference. .NET exposes nothing else: reachability is not queryable, and an object is only proven unreachable by collecting it.

The suggested fix does not work, and the comment says so itself. It offers three alternatives and disclaims each one in the same breath — GC.KeepAlive is "not appropriate ... but is unnecessary", WaitForFullGCComplete() "is not equivalent and depends on notifications" — before landing on "poll briefly with GC.WaitForPendingFinalizers() only". That last one is the problem: WaitForPendingFinalizers waits for the finalizer queue to drain, it does not collect anything. With no GC.Collect() the instance is never collected, reference.IsAlive stays true however long you poll, and the test fails — against the fixed code. It would not preserve test intent; it would invert the result.

The three calls are each load-bearing, which is why there are three rather than one:

call	why
GC.Collect()	queues the unreachable instance for finalization
GC.WaitForPendingFinalizers()	lets finalizers run, so a resurrected object settles
GC.Collect()	reclaims what finalization released

Dropping the second Collect would make the test flaky rather than wrong, since a finalizable object survives its first collection.

On suppressing it instead: [SuppressMessage] is a Roslyn mechanism and has no effect on a CodeQL alert, so adding one here would look like a fix without being one. I have not pushed one. If you would rather this stopped recurring on future PRs, the options are a // codeql[cs/call-to-gc] inline suppression or dismissing the alert in the code-scanning UI as "used in tests" — both are repo-policy calls rather than something to decide inside a bug fix, so I have left it to you.

Worth noting the gating check agrees: CodeQL itself reported success on this head, as did github-advanced-security, alongside green tests on ubuntu, windows and macos. This is an advisory inline note, not a failing signal.

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud