From a8574e41b65a9dfeebb80f3f86b2e2577565c463 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 15:00:35 +0000 Subject: [PATCH 1/3] docs: record the CTS-field dispose-optional decision (keep flagging; reframe the Serilog differentiator) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A decision note from the Serilog/Npgsql oracle triage. _shutdownSignal (Serilog, a plain CancellationTokenSource Cancel()'d but not Dispose()'d) is effectively benign — no WaitHandle/CancelAfter/linking, so it holds nothing and the GC reclaims it; the same shape as the SemaphoreSlim dispose-optional exemption (#92). But a CTS dispose-optional gate is NOT worth it: CTS is our canonical disposable-field, so it would flip ~7 "must warn" assertions across 4 samples; real-world benign instances are a minority; and CTS convention is "always dispose" (unlike SemaphoreSlim). Decision: keep the conservative detector; treat plain-CTS / ReaderWriterLockSlim / managed-only field leaks as low-severity instances; the honest flagship differentiator is Npgsql _pruningTimer (a real System.Threading.Timer leak), not _shutdownSignal. A severity-tier (OS-handle/timer/linked = warning; plain-managed = info) is deferred as a future option. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- docs/notes/cts-field-dispose-optional.md | 74 ++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 docs/notes/cts-field-dispose-optional.md diff --git a/docs/notes/cts-field-dispose-optional.md b/docs/notes/cts-field-dispose-optional.md new file mode 100644 index 00000000..13b4f51a --- /dev/null +++ b/docs/notes/cts-field-dispose-optional.md @@ -0,0 +1,74 @@ +# CancellationTokenSource field leaks are *dispose-optional* — and why we still flag them + +A triage note prompted by the cross-tool oracle on Serilog (see +[`oracle.md`](oracle.md), [`real-world-mining.md`](real-world-mining.md)). It +records why one of our headline "own-only" findings is weaker than first claimed, +and the deliberate decision **not** to add a `CancellationTokenSource` (CTS) +dispose-optional exemption mirroring the SemaphoreSlim one. + +## The finding that prompted it + +The Serilog oracle's single own-only leak was +`BatchingSink._shutdownSignal` — a `CancellationTokenSource` field that is +`Cancel()`-ed in `Dispose()` but never `Dispose()`-d. We presented it as the clean +field/owner-lifetime differentiator (CodeQL is local-scoped, Infer# method-scoped; +neither flags it). On a closer read of the Serilog source that over-sells it. + +## Why `_shutdownSignal` is effectively benign + +From `BatchingSink.cs` (`dev`): `readonly CancellationTokenSource _shutdownSignal = new();` + +- `.Token.WaitHandle` is **never** read → no lazily-allocated kernel event (OS handle); +- no `CancelAfter(...)`/timer → not rooted in the timer queue; +- not a linked source (`CreateLinkedTokenSource`) → no registration on a parent token; +- the token only feeds `Task.Delay(Infinite, token)` and a channel read — registration-based cancellation, cleared on `Cancel()`. + +So once `Cancel()` runs, the abandoned CTS holds **no unmanaged resource and is rooted +nowhere**. `CancellationTokenSource` has no finalizer of its own, so a plain instance is +simply **collected by the GC**; calling `.Dispose()` on it would be a near no-op. This is +the *same shape* as the SemaphoreSlim dispose-optional exemption (PR #92): a `SemaphoreSlim` +is dispose-optional until `.AvailableWaitHandle` is read; a **plain** CTS is dispose-optional +until `.Token.WaitHandle` is read / `CancelAfter` is used / it is linked. `_shutdownSignal` +reads none of those. (The owner being process-lived — the `Log.Logger` singleton case — makes +it doubly moot, but loggers *can* churn per-scope, so the plain-CTS argument is the robust one.) + +## Why we did **not** add a CTS dispose-optional gate + +The #92 SemaphoreSlim gate was cheap because `SemaphoreSlim` is not used as a canonical +disposable anywhere else. CTS is the opposite — it is **our go-to "owned IDisposable field"** +across the test surface. A plain-CTS exemption (exempt unless `.Token.WaitHandle` / `CancelAfter` +/ linked) would flip **~7 "must warn" assertions** in 4 sample files from warn → silent: + +| sample | field | what it actually tests | +|---|---|---| +| `SemaphoreFieldSample` | `_ctsControl` | the #92 type-scope control | +| `AliasDisposeSample` | `_neverDisposed`, `_rebound`, `_refRebound`, `_scopedLeak` | alias / rebound / scoped-alias mechanics | +| `ResolvedDisposableSample` | `cts` (+ a `MemoryStream` field) | resolve-aware disposable field | +| `DisposableFieldViewModel` | `ReportViewModel._cts` | the field-detector flagship | + +None of those are *about* CTS — they use it as a convenient IDisposable. The gate would force +rewriting them onto a non-optional type, for little gain: real-world dispose-optional instances +are a **minority** (Serilog `_shutdownSignal`; Npgsql `GlobalTypeMapper._lock`, a +`ReaderWriterLockSlim` on a singleton) — a couple per repo, not a flood. And the prevailing +.NET convention for CTS is the **opposite** of SemaphoreSlim: *always dispose it* (CA2000 is +insistent precisely because the dangerous `CancelAfter`/linked omissions are common and costly). +A blanket exemption would fight that convention. + +> "Look at the scale first" earned its keep here: the same refinement that was narrow and cheap +> for SemaphoreSlim is broad and against-the-grain for CTS. + +## Decision + +- **Keep the conservative detector.** Flagging undisposed CTS fields follows the convention and + catches the dangerous (`CancelAfter`/linked/`WaitHandle`) cases; a plain-CTS instance is a + low-severity *instance*, not a reason to exempt the type. +- **Reframe the differentiator.** `_shutdownSignal` (Serilog) and `_lock` (Npgsql) are + low-severity, dispose-optional-class instances. The honest flagship for the field/owner-lifetime + capability is **Npgsql `PoolingDataSource._pruningTimer`** — a `System.Threading.Timer` holds a + live timer-queue registration and a rooted callback, a *real* leak until disposed. +- **Deferred option — severity tiers (not exemption).** If we later want the tool to encode + criticality, split the disposable-field severity: "holds an OS handle / timer / linked CTS" → + warning; "plain managed (plain CTS, `ReaderWriterLockSlim`, `MemoryStream`)" → info/hint. That + keeps recall (the instance still surfaces) without flipping any test (warn → info, not + warn → silent). It needs a curated OS-handle-vs-managed type classifier (an inverted/extended + `IsDisposeOptional`) and is only worth it if the criticality signal proves valuable on the corpus. From d5644bd6764a5270b6272f8729c83966825ebec3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 15:09:01 +0000 Subject: [PATCH 2/3] =?UTF-8?q?docs:=20fix=20CTS=20note=20=E2=80=94=20RWLS?= =?UTF-8?q?=20is=20not=20managed-only=20(Codex=20P2)=20+=20clarify=20hypot?= =?UTF-8?q?hetical/provenance=20(CodeRabbit)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Codex P2: ReaderWriterLockSlim lazily allocates kernel wait handles under contention (Dispose releases them), so it is NOT managed-only. Move it out of the info/hint tier; the deferred severity split is now three families, and RWLS sits in "lazily allocates a wait handle" with NO static gate (contention is a runtime property) -> stays warning. Separate the two benign axes: _shutdownSignal benign by TYPE (plain CTS), _lock benign only by process-lived OWNER (not a managed-only type). - CodeRabbit: clarify the plain-CTS "dispose-optional until WaitHandle/CancelAfter/ linked" describes the .NET semantics / what a gate WOULD key on, NOT current detector behaviour (IsDisposeOptional still exempts only Task/ValueTask + System.Data). - CodeRabbit: add provenance for the mined examples (serilog/serilog, npgsql/npgsql@v8.0.9; see oracle.md) — illustrative of the classes, not exhaustive. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- docs/notes/cts-field-dispose-optional.md | 44 ++++++++++++++++-------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/docs/notes/cts-field-dispose-optional.md b/docs/notes/cts-field-dispose-optional.md index 13b4f51a..5e5f96c9 100644 --- a/docs/notes/cts-field-dispose-optional.md +++ b/docs/notes/cts-field-dispose-optional.md @@ -29,8 +29,11 @@ simply **collected by the GC**; calling `.Dispose()` on it would be a near no-op the *same shape* as the SemaphoreSlim dispose-optional exemption (PR #92): a `SemaphoreSlim` is dispose-optional until `.AvailableWaitHandle` is read; a **plain** CTS is dispose-optional until `.Token.WaitHandle` is read / `CancelAfter` is used / it is linked. `_shutdownSignal` -reads none of those. (The owner being process-lived — the `Log.Logger` singleton case — makes -it doubly moot, but loggers *can* churn per-scope, so the plain-CTS argument is the robust one.) +reads none of those. (This is the .NET runtime *semantics* — and exactly what a SemaphoreSlim-style +gate WOULD key on — **not** current detector behaviour: today `IsDisposeOptional` exempts only +`Task`/`ValueTask` + `System.Data`, so the detector flags CTS fields regardless. Keeping it that +way is the deliberate choice below.) The owner being process-lived — the `Log.Logger` singleton +case — makes it doubly moot, but loggers *can* churn per-scope, so the plain-CTS argument is the robust one. ## Why we did **not** add a CTS dispose-optional gate @@ -47,9 +50,15 @@ across the test surface. A plain-CTS exemption (exempt unless `.Token.WaitHandle | `DisposableFieldViewModel` | `ReportViewModel._cts` | the field-detector flagship | None of those are *about* CTS — they use it as a convenient IDisposable. The gate would force -rewriting them onto a non-optional type, for little gain: real-world dispose-optional instances -are a **minority** (Serilog `_shutdownSignal`; Npgsql `GlobalTypeMapper._lock`, a -`ReaderWriterLockSlim` on a singleton) — a couple per repo, not a flood. And the prevailing +rewriting them onto a non-optional type, for little gain: real-world benign instances are a +**minority**, and they split across two *different* axes (both mined by our cross-tool oracle +runs — `serilog/serilog` and `npgsql/npgsql@v8.0.9`; see [`oracle.md`](oracle.md) — illustrative +of the classes, not an exhaustive census). Serilog `_shutdownSignal` is benign by **type** (a +plain CTS — statically gateable, like SemaphoreSlim). Npgsql `GlobalTypeMapper._lock` is benign +only by **owner lifetime** (a process-lived singleton): a `ReaderWriterLockSlim` is **not** +managed-only — it lazily allocates kernel wait handles under contention, which `Dispose()` +releases, so a *churned, contended* lock would genuinely leak them. A couple of benign instances +per repo, not a flood. And the prevailing .NET convention for CTS is the **opposite** of SemaphoreSlim: *always dispose it* (CA2000 is insistent precisely because the dangerous `CancelAfter`/linked omissions are common and costly). A blanket exemption would fight that convention. @@ -62,13 +71,20 @@ A blanket exemption would fight that convention. - **Keep the conservative detector.** Flagging undisposed CTS fields follows the convention and catches the dangerous (`CancelAfter`/linked/`WaitHandle`) cases; a plain-CTS instance is a low-severity *instance*, not a reason to exempt the type. -- **Reframe the differentiator.** `_shutdownSignal` (Serilog) and `_lock` (Npgsql) are - low-severity, dispose-optional-class instances. The honest flagship for the field/owner-lifetime - capability is **Npgsql `PoolingDataSource._pruningTimer`** — a `System.Threading.Timer` holds a - live timer-queue registration and a rooted callback, a *real* leak until disposed. +- **Reframe the differentiator.** Both `_shutdownSignal` (Serilog — plain CTS, dispose-optional by + *type*) and `_lock` (Npgsql — a `ReaderWriterLockSlim` benign only because its owner is a + process-lived singleton, **not** because the type is managed-only) are low-severity *instances*. + The honest flagship for the field/owner-lifetime capability is **Npgsql + `PoolingDataSource._pruningTimer`** — a `System.Threading.Timer` holds a live timer-queue + registration and a rooted callback, a *real* leak until disposed. - **Deferred option — severity tiers (not exemption).** If we later want the tool to encode - criticality, split the disposable-field severity: "holds an OS handle / timer / linked CTS" → - warning; "plain managed (plain CTS, `ReaderWriterLockSlim`, `MemoryStream`)" → info/hint. That - keeps recall (the instance still surfaces) without flipping any test (warn → info, not - warn → silent). It needs a curated OS-handle-vs-managed type classifier (an inverted/extended - `IsDisposeOptional`) and is only worth it if the criticality signal proves valuable on the corpus. + criticality, split the disposable-field severity into three families: (1) **always holds an OS + handle / timer / linked or `CancelAfter` CTS** (FileStream, Socket, `System.Threading.Timer`) → + warning; (2) **lazily allocates a wait handle** (`SemaphoreSlim`, plain CTS, `ManualResetEventSlim`, + and `ReaderWriterLockSlim` *under contention*) → warning *unless* a syntactic gate proves the + handle is never allocated (`.AvailableWaitHandle` / `.Token.WaitHandle` unread) — note RWLS has + **no** such gate (contention is a runtime property), so it stays warning; (3) **truly managed-only** + (`MemoryStream`, `StringWriter`, `Task`, `DataTable`) → info/hint. That keeps recall (the instance + still surfaces) without flipping any test (warn → info, not warn → silent). It needs a curated + classifier (an extended `IsDisposeOptional`) and is only worth it if the criticality signal proves + valuable on the corpus. From 45968006d33f6fd5a5c9f8f617f0ce900edd8a89 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 15:20:30 +0000 Subject: [PATCH 3/3] =?UTF-8?q?docs:=20reframe=20=E2=80=94=20criticality?= =?UTF-8?q?=20is=20a=20spectrum=20=C3=97=20churn,=20not=20a=20flagship=20(?= =?UTF-8?q?re-examine=20=5FpruningTimer)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applying the same rigor to _pruningTimer that the Serilog _shutdownSignal triage used: it IS genuinely never disposed (a true TP), and its worst case is heavier than a plain CTS — while scheduled, the static TimerQueue roots its state (the data source), so an abandoned source can't be GC'd and the callback keeps firing, dragging the open connection pool. But it self-disables when idle (a collectable cycle) and is benign for the dominant process-lived NpgsqlDataSource singleton — the SAME lifetime caveat as _shutdownSignal. So _pruningTimer is not a clean "critical flagship" either. Replace the "flagship = _pruningTimer" framing with the honest conclusion: criticality is a spectrum = (self-root / OS-handle / live-work) × (owner churn), and a static analyzer cannot observe churn — which is exactly the case for the deferred severity tiers. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- docs/notes/cts-field-dispose-optional.md | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/docs/notes/cts-field-dispose-optional.md b/docs/notes/cts-field-dispose-optional.md index 5e5f96c9..c7007422 100644 --- a/docs/notes/cts-field-dispose-optional.md +++ b/docs/notes/cts-field-dispose-optional.md @@ -71,12 +71,22 @@ A blanket exemption would fight that convention. - **Keep the conservative detector.** Flagging undisposed CTS fields follows the convention and catches the dangerous (`CancelAfter`/linked/`WaitHandle`) cases; a plain-CTS instance is a low-severity *instance*, not a reason to exempt the type. -- **Reframe the differentiator.** Both `_shutdownSignal` (Serilog — plain CTS, dispose-optional by - *type*) and `_lock` (Npgsql — a `ReaderWriterLockSlim` benign only because its owner is a - process-lived singleton, **not** because the type is managed-only) are low-severity *instances*. - The honest flagship for the field/owner-lifetime capability is **Npgsql - `PoolingDataSource._pruningTimer`** — a `System.Threading.Timer` holds a live timer-queue - registration and a rooted callback, a *real* leak until disposed. +- **Criticality is a spectrum, not a flagship.** All of these field leaks are *conditional*: + `_shutdownSignal` (Serilog — plain CTS, dispose-optional by *type*), `_lock` (Npgsql — a + `ReaderWriterLockSlim` benign only because its owner is a process-lived singleton, **not** + because the type is managed-only), and even `_pruningTimer` (Npgsql `PoolingDataSource` — a + `System.Threading.Timer` that is genuinely never disposed). The Timer's *worst* case is heavier + than a CTS's: while it is scheduled, the static `TimerQueue` roots its `state` (the data source), + so an abandoned source cannot be GC'd and the callback keeps firing — dragging the open + connection pool with it. But it self-disables (`Change(Infinite, Infinite)`) when there is + nothing to prune, becoming a collectable cycle, and for the dominant process-lived + `NpgsqlDataSource` singleton it is benign at shutdown — the *same* lifetime caveat as + `_shutdownSignal`. So there is **no clean, unconditionally-critical flagship** here: criticality + is a *spectrum* = (does the type self-root / hold an OS handle / keep doing work) × (does the + owner *churn*), and a static analyzer **cannot observe churn**. The genuinely severe field leak + is a live OS handle (FileStream / Socket / an un-returned connection) on a *churned* owner — + whose severity we can only ever *worst-case*, never prove. That is exactly the case for the + severity tiers below. - **Deferred option — severity tiers (not exemption).** If we later want the tool to encode criticality, split the disposable-field severity into three families: (1) **always holds an OS handle / timer / linked or `CancelAfter` CTS** (FileStream, Socket, `System.Threading.Timer`) →