Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,15 @@ back-edge); мы воспроизводим **region-based** модель, пр
поверх mining — `scripts/oracle_compare.py` + workflow **oracle (cross-tool)**,
подробности в [`docs/notes/oracle.md`](docs/notes/oracle.md).

> **Требование (convention).** Каждый прогон оракула несёт обязательство по
> учёту: разобрать `oracle-only`-находки, и любой кейс, оказавшийся ФП оракула
> **или** нашим осознанным by-design скипом, — записать как паттерн в
> [`docs/notes/field-notes-patterns.md`](docs/notes/field-notes-patterns.md)
> (источник-файл + analyzer-angle: почему наивный детектор шумит, а наш
> escape/transfer-aware чекер корректно молчит). Тетрадка — и учебник по
> C#-идиомам владения/времени жизни, и живая карта precision-фронтира; она не
> должна отставать от того, что мы реально увидели в чужом коде.

---

## Структура
Expand Down
207 changes: 207 additions & 0 deletions docs/notes/field-notes-patterns.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
# Field notes: resource & lifetime patterns from the wild

A running, curated collection of real C# idioms spotted while pointing the
cross-tool oracle ([`oracle.md`](oracle.md)) at open-source repos. Two reasons
to keep it:

1. **They're worth learning from.** Mature libraries (Polly, Dapper, …) encode
battle-tested ways to own, share, pool, and scope disposables and lifetimes.
2. **They're Own.NET's precision frontier.** The recurring theme below is *code
that is correct but that naive leak detectors misread* — exactly the cases
where staying silent is the right verdict. Every entry notes how the pattern
interacts with leak/lifetime analysis (and where Infer#/CodeQL over-report).

Each entry: the idiom, why it exists, a code sketch, and the **analyzer angle**.
Sources are pinned to the file we actually read; line numbers drift, so treat
them as "around here". New finds get appended — this is a notebook, not a spec.

---

## 1. Ownership transfer via factory return

**Seen in:** Polly `src/Polly/Bulkhead/BulkheadSemaphoreFactory.cs` →
`BulkheadPolicy.cs`; Dapper `Dapper/SqlMapper.cs` (`DbWrappedReader.Create`).

A factory **creates** a disposable and **returns** it; the *caller* becomes the
owner and is responsible for disposal. The disposable is intentionally not
disposed at the creation site.

```csharp
// factory: creates, hands ownership out
internal static (SemaphoreSlim Parallel, SemaphoreSlim Queue)
CreateBulkheadSemaphores(int maxParallelization, int maxQueueingActions) { … }

// holder: stores in fields, disposes in its own Dispose()
private readonly SemaphoreSlim _maxParallelizationSemaphore;
private readonly SemaphoreSlim _maxQueuedActionsSemaphore;
public void Dispose()
{
_maxParallelizationSemaphore.Dispose();
_maxQueuedActionsSemaphore.Dispose();
}
```

**Why:** separates *construction* (sizing/validation logic) from *ownership*
(the policy lives long, the factory doesn't). Classic "owned handle".

**Analyzer angle:** "created but not disposed *here*" ≠ leak — disposal moved
with the reference. CodeQL's `cs/local-not-disposed` flags the factory line (it
can't follow tuple-return ownership) → **false positive**. Own.NET treats
return/escape as ownership transfer and stays silent. The same shape is Dapper's
`DbWrappedReader` (returned from `ExecuteReader*`, the caller disposes), where
Infer# over-reports a `PULSE_RESOURCE_LEAK`. **Rule of thumb: follow the
reference — whoever ends up holding it owns the dispose.**

## 2. Deferred disposal via a lifecycle callback

**Seen in:** Polly `src/Polly.Extensions/Registry/ConfigureBuilderContextExtensions.cs`.

A disposable outlives the method that creates it, so disposal is *wired to a
future event* instead of a local `using`.

```csharp
#pragma warning disable CA2000 // disposal deferred to pipeline teardown
var source = new CancellationTokenSource();
context.AddReloadToken(source.Token);
context.OnPipelineDisposed(() => source.Dispose()); // disposed later, on teardown
```

**Why:** the token must stay alive for the lifetime of the pipeline, not the
configuration call. You can't `using` it — you hang its disposal off the owning
object's lifecycle.

**Analyzer angle:** disposal exists, just not lexically — it's inside a lambda
registered with a lifecycle hook. Flow-insensitive "is there a Dispose on every
path?" checks miss it → CodeQL **false positive**. Note the deliberate
`#pragma warning disable CA2000`: the authors *know* and suppress the analyzer.
**A suppressed analyzer warning next to a callback registration is a strong
"this is intentional deferred-dispose" signal.**

## 3. Pooled disposable — rent & return, don't own

**Seen in:** Polly `src/Polly.Core/Timeout/TimeoutResilienceStrategy.cs`
(`_cancellationTokenSourcePool.Get(timeout)` / `.Return(cts)`).

Hot-path disposables (here `CancellationTokenSource`) are **rented from a pool**
and **returned**, not newed-and-disposed each call.

```csharp
var cts = _cancellationTokenSourcePool.Get(timeout);
try
{
// … use cts.Token …
}
finally
{
_cancellationTokenSourcePool.Return(cts); // recycled, not Dispose()d
}
```

**Why:** `CancellationTokenSource` allocates timer/registration state; on a
per-call resilience path that churn matters. A pool amortizes it. (Polly's
`CancellationTokenSourcePool` is itself worth reading — it resets and re-arms
CTSs safely for reuse.)

**Analyzer angle:** the object is never "leaked" — it's recycled. `Return` is the
moral equivalent of dispose. A detector that only recognises `Dispose()`/`using`
sees an undisposed CTS and may flag the throw-path (CodeQL
`cs/dispose-not-called-on-throw`) — but the worst case is "object not returned to
the pool", which the GC reclaims anyway. **Pool `Get`/`Return` is an
ownership-protocol the analyzer must learn, or it over-reports.**

## 4. struct-based scoped lock (`using` over a value type)

**Seen in:** Polly `src/Polly/Utilities/TimedLock.cs` (+ its callers in
`CircuitBreaker/*`).

A `struct` that implements `IDisposable` to give a zero-allocation, RAII-style
lock **with a timeout** (so a stuck lock throws instead of hanging forever).

```csharp
using (TimedLock.Lock(_syncObject)) // acquires Monitor with a timeout
{
// critical section
} // Dispose() releases the Monitor
```

**Why:** `lock(x){}` can deadlock forever; `TimedLock` bounds the wait and can be
told to detect/raise on timeout. Being a `struct` means no heap allocation per
critical section — important on hot paths.

**Analyzer angle:** the disposable is a **value type**, disposed by the `using`.
Infer#'s Pulse engine reports each of these as `PULSE_RESOURCE_LEAK` "allocated
indirectly via `TimedLock.Lock` … not closed" — a systematic **over-report on the
struct-`using` pattern** (12 of them on Polly). Own.NET's silence is correct.
**A value-type `IDisposable` used in a `using` is disposed deterministically;
don't treat the `.Lock()` factory call as an escaping allocation.**

## 5. Bulkhead = two semaphores (bounded concurrency + bounded queue)

**Seen in:** Polly `src/Polly/Bulkhead/*` (the pair from entry 1).

The Bulkhead resilience pattern (isolate a dependency so its slowness can't drain
a shared pool) is implemented with **two** `SemaphoreSlim`s, not one:

```
maxParallelization -> how many calls run at once (the bulkhead "compartment")
maxQueueingActions -> how many may wait for a slot (cap the queue itself)
```

**Why:** one semaphore bounds concurrency, but an *unbounded* wait queue is just a
new way to exhaust memory/threads. The second semaphore (capacity
`maxQueueingActions + maxParallelization`) bounds the queue and fails fast
(`BulkheadRejectedException`) past it. Isolation **and** backpressure.

**Analyzer angle:** not a leak case — a design pattern. Filed here because it's the
canonical "bound *every* shared resource, including the queue you added to bound
the first one" lesson. (See the chat thread for the long-form explanation.)

## 6. Wrapper/adapter that forwards `Dispose` to a borrowed inner

**Seen in:** Dapper `Dapper/SqlMapper.IDataReader.cs` / `WrappedReader.cs`
(`WrappedBasicReader`).

A wrapper holds someone else's disposable and **forwards** `Dispose` to it —
because the *holder* of the wrapper, not the wrapper's creator, decides lifetime.

```csharp
public void Dispose() => _reader.Dispose(); // forward to the wrapped reader
```

**Why:** Dapper hands a reader back to the caller; the wrapper exists precisely so
the **caller** disposes it (and through it, the underlying reader). Disposing it
internally would close the caller's reader out from under them.

**Analyzer angle:** the wrapper "allocates" a reader it never disposes locally —
Infer# reads that as a leak. But it's the ownership-transfer case again (entry 1)
viewed through an adapter. **When a type wraps a disposable it doesn't own, the
correct behaviour is to forward disposal, not perform it — and an analyzer must
not count the wrapped allocation against the wrapper.**

---

## The through-line

Five of six entries are the *same lesson from different angles*: **disposal
responsibility travels with the reference** — out of a factory (1, 6), forward in
time via a callback (2), into a pool (3), or down a `using` on a value type (4).
Naive "every disposable needs a lexical `using`/`Dispose` on every path" checks
misread all of them, which is why Infer#/CodeQL over-report here and a
transfer/escape-aware checker (Own.NET) correctly stays quiet. Worth learning as
C#; worth pinning as the precision frontier.

## Maintaining this notebook (a repo requirement)

This log is **required upkeep**, not a nice-to-have (see [`oracle.md`](oracle.md)
§ Maintenance requirement and the README convention note). The rule:

> After every oracle run, triage the `oracle-only` findings. Any that turn out to
> be an oracle **false positive** or our deliberate **by-design** skip almost
> always hide an idiom like the ones above — **append it here**, with the source
> file and the analyzer angle. A run that surfaces a new FP/by-design idiom and
> doesn't record it is an incomplete run.

Keep entries source-pinned and honest about confidence (note when a judgement
rests on a decompiled/fetched excerpt rather than the full source). The point is a
collection we can *trust* — both to learn C# ownership idioms from and to navigate
Own.NET's precision frontier by.
69 changes: 69 additions & 0 deletions docs/notes/oracle.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,19 @@ Two classes sit **outside** the three-way diff and are reported separately:
same-named files in different dirs can theoretically collide (rare — the line
disambiguates). The file-level overlap is the most robust signal.

## Maintenance requirement (not optional)

**Every oracle run carries a bookkeeping obligation.** After reading a report,
triage the `oracle-only` set, and for any finding that turns out to be an oracle
**false positive** *or* our deliberate **by-design** skip, record the underlying
idiom in [`field-notes-patterns.md`](field-notes-patterns.md) — with the source
file and the *analyzer angle* (why the naive detector over-reports and why a
transfer/escape-aware checker correctly stays silent). The field-notes collection
must not fall behind what we've actually observed in real code: it is both a C#
ownership/lifetime learning resource and the living map of Own.NET's precision
frontier. A run that surfaces a new FP/by-design idiom and *doesn't* append it is
an incomplete run.

## Run it

In CI (no local Infer#/CodeQL/Docker needed) — Actions tab → **oracle
Expand Down Expand Up @@ -141,6 +154,62 @@ precision held (0 FPs; we look *more* correct than Infer# on ownership transfer)
and the recall gap is two named, roadmapped classes (interprocedural escape
tracking, exception-path disposal).

## A second worked example: Polly (ownership transfer, again)

A blind run on **App-vNext/Polly** (`42307a6e`, product code; `--exclude-tests`
dropped 145) gave **Own.NET 0 · Infer# 12 · CodeQL 26**, `agree 0`, **`own-only 0`
(still no false positives)** — but a *large* `oracle-only 38`. The point of this
example is that the headline number is honest only after decomposition; 38 is **not**
38 real misses:

- **Infer# ×12 — all `Polly.Utilities.TimedLock`.** `TimedLock` is a `struct` used as
`using (TimedLock.Lock(obj)) { … }` — it *is* disposed by the `using`. Infer#'s Pulse
reports it `PULSE_RESOURCE_LEAK` "allocated indirectly via `TimedLock.Lock` … not
closed". These look like Infer# **over-reports on the struct-using lock pattern** (the
same flavour as its `DbWrappedReader` over-reports on Dapper); Own.NET's `0` is the
right verdict, not a recall gap.
- **CodeQL ×22 — `src/Snippets/Docs/*`.** Polly's documentation snippet tree: example
code that creates `HttpResponseMessage`/`HttpClient`/rate-limiters and intentionally
never disposes them. Not product code. The comparator's `--exclude-tests` predicate
(`_is_test_path`) was widened to treat `doc`/`docs`/`snippet(s)` segments as non-product
for exactly this reason — without it, illustrative code inflates the recall gap. (This
is the slice that drops on re-run: `oracle-only` falls 38 → 16, CodeQL leak 26 → 4.)
- **The remaining 4 product CodeQL findings (3 sites — the Bulkhead factory is two lines)
all resolve to FP-or-by-design:**
- `Bulkhead/BulkheadSemaphoreFactory.cs:8,11` — the factory **returns** two
`SemaphoreSlim` as a tuple; the caller `BulkheadPolicy` stores them in `readonly`
fields and disposes them in `Dispose()`. Textbook **ownership transfer / owned
handle** — the exact case Own.NET treats as *not* a leak. CodeQL's
`cs/local-not-disposed` can't follow tuple-return ownership, so its flag at the
factory is a **false positive**, and Own.NET's silence is *more* correct (the second
live ownership-transfer precision win after Dapper's `DbWrappedReader`).
- `Registry/ConfigureBuilderContextExtensions.cs:40` — a `CancellationTokenSource`
whose disposal is deferred and wired via `context.OnPipelineDisposed(() =>
source.Dispose())` (the authors even `#pragma warning disable CA2000`). Disposed at
pipeline teardown — CodeQL just can't follow callback/deferred disposal. **FP.**
- `Timeout/TimeoutResilienceStrategy.cs:67` — `cs/dispose-not-called-on-throw` on a
`CancellationTokenRegistration` (struct, disposed on the normal path) guarding a
**pooled** CTS (`_cancellationTokenSourcePool.Get`/`Return`, not owned), with the
await wrapped in a catch that funnels exceptions into the `Outcome`. The exceptional
CFG class we don't model **by design** — and here benign (at worst a pool object not
returned, which the GC reclaims).

Net: on Polly's product code Own.NET's genuine recall gap is **≈ zero**. The whole
`oracle-only 38` reconciles as **12 Infer# struct-`using` over-reports + 22 CodeQL doc
snippets (now excluded) + 4 product CodeQL findings** — and those 4 are three false
positives (`BulkheadSemaphoreFactory:8`, `:11`, `ConfigureBuilderContextExtensions:40`)
plus one by-design exceptional-path skip (`TimeoutResilienceStrategy:67`).
Combined with `own-only 0`, this is the double signal the oracle exists to produce:
precision holds, and the "miss" pile is oracle noise, not our blind spot. (Honest caveat,
same as Dapper: "0 real misses *here*" is partly the luck of the finding mix — Polly is
async/interprocedural code we under-analyse, so this is not proof of zero recall gaps in
general.)

> The idioms these runs surfaced — ownership transfer, deferred/callback disposal, pooled
> disposables, struct-`using` locks — are collected as teachable patterns in
> [`field-notes-patterns.md`](field-notes-patterns.md). Most `oracle-only` findings that
> turn out FP-or-by-design hide one of them.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
## Honest gaps (v1)

- **No tool versions pinned in the report yet.** `microsoft/infersharpaction@v1.5`
Expand Down
31 changes: 23 additions & 8 deletions scripts/oracle_compare.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,12 +370,21 @@ def to_json(result: dict[str, Any], target: str, commit: str) -> dict[str, Any]:


def _is_test_path(path: str) -> bool:
"""True if a finding path lives under a test/benchmark/sample/example tree —
non-product code. `--exclude-tests` drops these so the three tools are diffed
on the product code only (Infer# builds just the product project, so without
this own-check/CodeQL would also count test/benchmark leaks the others can't)."""
"""True if a finding path lives under a non-product tree — tests, benchmarks,
samples, examples, or documentation snippets. `--exclude-tests` drops these so
the three tools are diffed on product code only (Infer# builds just the product
project, so without this own-check/CodeQL would also count leaks the others
can't). `doc`/`snippet` trees carry intentionally-undisposed illustrative code
(e.g. Polly's `src/Snippets/Docs/*`, where ~20 `HttpResponseMessage`/`HttpClient`
examples are never disposed by design) — counting them as product leaks inflates
the oracle-only recall gap with example code that was never meant to dispose."""
for seg in path.lower().split("/"):
if seg in ("test", "tests") or seg.startswith(("benchmark", "sample", "example")):
# Exact match for short, collision-prone names — a `snippet` *prefix* would
# wrongly drop product dirs like `SnippetEngine`/`SnippetService` (Polly's
# `src/Snippets/Docs/*` is caught by the exact `snippets`/`docs` segments
# anyway). Prefix match only for the long, unambiguous plural-able ones.
if (seg in ("test", "tests", "doc", "docs", "snippet", "snippets")
or seg.startswith(("benchmark", "sample", "example"))):
return True
return False

Expand Down Expand Up @@ -545,10 +554,16 @@ def _selftest() -> int:
if [g.cls for g in pulse] != ["leak"]:
fails.append(f"PULSE_RESOURCE_LEAK not classed as leak: {[g.cls for g in pulse]}")
# --exclude-tests predicate: matches non-product trees, not product code.
# Doc/snippet trees (Polly's src/Snippets/Docs/*) are non-product illustrative
# code — intentionally-undisposed examples that would otherwise inflate oracle-only.
if not all(_is_test_path(p) for p in
("tests/Foo/Bar.cs", "benchmarks/X/Y.cs", "src/Test/Z.cs")):
fails.append("_is_test_path should match test/benchmark trees")
if any(_is_test_path(p) for p in ("Dapper/SqlMapper.cs", "src/Lib/A.cs")):
("tests/Foo/Bar.cs", "benchmarks/X/Y.cs", "src/Test/Z.cs",
"src/Snippets/Docs/Fallback.cs", "src/MyLib/docs/Example.cs")):
fails.append("_is_test_path should match test/benchmark/doc/snippet trees")
# ...but a product dir whose name merely *starts with* a marker word is NOT
# excluded — exact match for snippet/doc guards against dropping real code.
if any(_is_test_path(p) for p in ("Dapper/SqlMapper.cs", "src/Lib/A.cs",
"src/SnippetEngine/Foo.cs", "src/Documentation/Api.cs")):
fails.append("_is_test_path should not match product paths")
# the scope note is gated on mode, not count: a product-only run that excluded
# nothing must still say so (else it reads like a full-scope run).
Expand Down
Loading