diff --git a/README.md b/README.md index d7295a30..aa1abd6e 100644 --- a/README.md +++ b/README.md @@ -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-фронтира; она не +> должна отставать от того, что мы реально увидели в чужом коде. + --- ## Структура diff --git a/docs/notes/field-notes-patterns.md b/docs/notes/field-notes-patterns.md new file mode 100644 index 00000000..9d88d511 --- /dev/null +++ b/docs/notes/field-notes-patterns.md @@ -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. diff --git a/docs/notes/oracle.md b/docs/notes/oracle.md index 1c1bf547..25c3ac26 100644 --- a/docs/notes/oracle.md +++ b/docs/notes/oracle.md @@ -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 @@ -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. + ## Honest gaps (v1) - **No tool versions pinned in the report yet.** `microsoft/infersharpaction@v1.5` diff --git a/scripts/oracle_compare.py b/scripts/oracle_compare.py index 621ae063..108c4794 100644 --- a/scripts/oracle_compare.py +++ b/scripts/oracle_compare.py @@ -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 @@ -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).