Memory Leak Fixer

Memory Leak Fixer

Proactively find and fix memory leaks in SkiaSharp — a thin managed wrapper over native Skia, so its recurring, high-impact class of leaks is native ownership / disposal correctness — the C# binding failing to dispose, own, pin, or root a native object correctly — not the managed view-retention leaks a pure-managed app framework worries about. This skill hunts that class of leaks and produces a validated fix.

Scope: managed C# only. Work only in the code SkiaSharp owns — the C# bindings (binding/**) and view layers (source/**). Everything under externals/skia/** is upstream Skia (including our C shim): out of scope, not buildable on a standard runner, and handled by a separate process. Every candidate must be provable and fixable from C#.

Read documentation/dev/memory-management.md first — it is the authoritative model (pointer types, owns: flag, ref-count rules, the same-instance-return contract). This skill assumes that model.

The leak catalogue is split into 11 focused references, each with a description, why it's bad, a leak→fix code example, and a leak-specific anti-pattern. Phase 1 maps the selected focus area directly to its one reference file; consult that same file when writing a fix (Phase 3).

Code samples are illustrative and trimmed to the essential lines; real wrappers must retain normal argument validation and use GC.KeepAlive where lifetime requires it.

Golden rules (non-negotiable)

1. One leak per run. Pick the single strongest candidate; do not batch.
2. Empirical confirmation before any fix. A fix is only valid if a regression test FAILS on the current tree without the fix and PASSES with it (red→green, both directions). No demonstrated red→green ⇒ no PR.
3. Never weaken, skip, mute, [Obsolete]-hide, or delete a test to make it pass. If the only thing that turns green is a mute, the fix is wrong — reject it.
4. Never edit generated files or upstream Skia. *.generated.cs and everything under externals/skia/** (including our C shim) are off-limits — this skill is managed-C# only. Every fix lives in binding/** or source/**.
5. ABI stability. Add overloads / methods; never change or remove a public signature.
6. Honest scope note. In every issue/PR, say whether it is a clear framework bug or a usage footgun the framework could harden — and what is empirically proven vs statically reasoned.
7. Finding nothing is the expected outcome. SkiaSharp is mature and heavily hardened; there is no planted/seeded bug waiting to be found. Most runs should end with no candidate. Never invent a leak, never rationalize deliberately-hardened, well-documented code as "decoys," and never lower the evidence bar to force a result. A quiet run is a first-class success — report it and emit a single noop (see Phase 5).

Mode selection

Run the phases in order. The skill has two entry points:

Phase 0 — Prepare the scan (no native download)

CI runner reality: this skill fixes managed C# only (binding/**, source/**). The native library is consumed as a pre-built package (externals-download) — you never build native code, so every candidate must be provable and fixable from C#. A leak whose only fix is in native / upstream Skia is out of scope: file an issue (Phase 4), never open a PR you cannot validate.

1. Confirm the SDK: dotnet --version.
2. Read the ownership model and select the focus-area entry. Do not restore local tools or download pre-built natives during setup, source scanning, or de-duplication. A quiet or duplicate run must end without either operation.

Phase 1 — Scan (find ONE candidate)

1.1 Choose a focus area (round-robin across runs)

A full 11-area sweep every run is wasteful and the surface is mostly hardened, so start from ONE focus area and widen only if it's exhausted.

Override first. If the run supplies an explicit focus area (a bare number 0–10 — e.g. a maintainer testing one area on demand), use that number directly as FOCUS and skip the rotation below. Otherwise rotate the starting area on a time-based round-robin so consecutive runs cover different areas. This needs only date, so it behaves identically locally and in CI — no $GITHUB_RUN_NUMBER / $RANDOM (which don't exist or aren't deterministic outside GitHub Actions):

# Round-robin: advance one focus area every hour, cycling through all 11.
DOY=$(date -u +%j); HOUR=$(date -u +%H)         # day-of-year + hour, both zero-padded
FOCUS=$(( (10#$DOY * 24 + 10#$HOUR) % 11 ))      # 10# forces base-10
echo "focus area: $FOCUS"

The 10# prefix is required: date zero-pads %j/%H, and $(( 08 )) is an invalid-octal error without it. For a targeted local run, skip the rotation and just name the focus area you want.

Every focus area is drawn from a real, historical SkiaSharp leak fix. Open the one file mapped to FOCUS, then read its Where to look line, description, why-it's-bad, leak→fix example, and per-area anti-pattern before scanning:

If the selected focus area is exhausted (its leaks are already open issues/PRs — see 1.3), advance to the next index and load only its mapped reference file.

1.2 Establish the retention/ownership path

For each candidate write the precise path with file:line citations:

• Native-handle leaks: creation site → escape path → missing Dispose/unref.
• owns: bugs: the P/Invoke name that produced the handle (_new_/_create returns an owned object; _get_/property-style returns a borrowed pointer) vs the owns: value the C# wrapper passed.
• Views retention: long-lived root → subscription/handler → transient view, and the unload path that should have detached but doesn't.

Skip if already weak/correct: uses WeakEventHandler/WeakReference/ ConditionalWeakTable, or the ownership already matches the memory-management rules.

1.3 De-dup against this project's own open issues/PRs

Before confirming, fetch and skip anything already covered. Search two ways — real SkiaSharp leak fixes are usually filed as [BUG] … (not [memory-leak] …), so the title-prefix search alone will miss them. Also search by the specific type/API name:

# 1. Prior runs of THIS workflow (our own prefix):
gh issue list --repo "$GITHUB_REPOSITORY" --search '"[memory-leak]" in:title' \
  --state open --limit 100 --json number,title,body
gh pr list --repo "$GITHUB_REPOSITORY" --search '"[memory-leak]" in:title' \
  --state open --limit 100 --json number,title,body

# 2. Human-reported coverage of the SAME api/type (the important check):
#    e.g. for the Blob.FromStream candidate below, this surfaces open PR #3473.
gh issue list --repo "$GITHUB_REPOSITORY" --search 'Blob.FromStream in:title,body' --state open --json number,title
gh pr    list --repo "$GITHUB_REPOSITORY" --search 'Blob.FromStream in:title,body' --state open --json number,title

A candidate is OUT only if an open issue/PR already covers the same handle / ownership path (by our prefix OR by the api/type name). A candidate whose only prior item is CLOSED may be re-filed. Worked example: the HarfBuzzSharp.Blob.FromStream fixed-pointer leak (area 4) is a genuine, still-present bug — but open PR #3473 "Make Blob.FromStream GC safe" already fixes it, so it is OUT: stand down, do not open a duplicate PR, emit a noop.

Pick the ONE strongest candidate. If none is convincing, stop — a quiet run is a success (the surface is hardened; the value is catching new leaks as code lands). Do not keep digging past a reasonable single pass hoping to manufacture a finding: report the quiet result and emit a noop (Phase 5).

1.4 Bootstrap one qualified candidate

Only after one managed-C# candidate has a citable ownership path and clears the Phase 1.3 open-item de-dup gate, prepare for empirical proof. Run this exact command once per run:

dotnet tool restore && dotnet cake --target=externals-download

This is the mandatory bootstrap before any source build or test, not a scan prerequisite. Do not run either command for a quiet/duplicate candidate, and do not repeat either command in later phases.

Phase 2 — Prove (empirical confirmation)

Every in-scope leak is observable from managed code, so prove it with a WeakReference + forced-GC probe that mirrors the existing memory tests. Prove it against the shipped SkiaSharp NuGet in a throwaway project — no source build, no display:

mkdir -p /tmp/leakprobe && cd /tmp/leakprobe

leakprobe.csproj referencing <PackageReference Include="SkiaSharp" Version="*" /> — the floating * resolves to the latest stable SkiaSharp on nuget.org automatically (use Version="*-*" to include the latest preview) — plus xunit + Microsoft.NET.Test.Sdk, then a single [Fact] that:

1. runs Control (correct usage), Leaky (the suspect path), Mitigation (the proposed workaround), each allocating N subjects tracked by WeakReference;
2. forces GC (for (i=0;i<6;i++){ GC.Collect(); GC.WaitForPendingFinalizers(); GC.Collect(); });
3. asserts the leaky subjects stay alive while control + mitigation are collected.

cd /tmp/leakprobe && dotnet test --logger "console;verbosity=normal"

• Passes ⇒ leak confirmed. Failed to build / assertions don't hold ⇒ hypothesis wrong; iterate once on another candidate or stop.
• For undisposed-handle leaks, prefer proving the wrapper is not collected / Dispose is never reached; the in-repo equivalent uses SKObject.GetInstance<T>(handle, out _) + CollectGarbage() (see tests/Tests/SkiaSharp/SKObjectTest.cs).

Phase 3 — Fix (red→green regression test + minimal change)

3.1 Write the failing test FIRST (red)

Add a focused regression test to the console test project (source lives under tests/Tests/, run via tests/SkiaSharp.Tests.Console). Model it on existing disposal/leak tests:

• AssertEx.EventuallyGC(weakRef, …) — GC-based (tests/Tests/Xunit/AssertEx.cs).
• SKObject.GetInstance<T>(handle, out inst) + CollectGarbage() — wrapper lifecycle (tests/Tests/SkiaSharp/SKObjectTest.cs).
• For views: the handler pattern in tests/SkiaSharp.Tests.Devices/Tests/Maui/MemoryLeakTests.cs.

Using the one bootstrap from Phase 1.4, build and confirm the test FAILS on the current tree (proves it catches the leak):

dotnet build binding/SkiaSharp/SkiaSharp.csproj
dotnet test tests/SkiaSharp.Tests.Console/SkiaSharp.Tests.Console.csproj --filter "FullyQualifiedName~<YourTestName>"

If a test you expected to be red is green, your hypothesis is wrong — go back to Phase 1.

3.2 Implement the minimal idiomatic fix

Apply the Fix (✓) in the selected focus-area reference — every reference has a worked before/after. Then re-read that reference's Watch out (❌ don't): note: it names the specific wrong fix that turns one leak into another (an unconditional Dispose, flipping owns: blind, nulling a field before disposing, a pinned GCHandle where a plain field suffices, …).

Touch only the minimal code, and keep it inside binding/** / source/**. Never change a public signature to fix ownership — add an overload or fix internals (ABI stability). If the only correct fix is in native / upstream Skia, stop and file an issue (Phase 4) — this skill does not open native PRs.

3.3 Confirm green + no regressions

Rebuild and re-run the regression test (now PASSES) plus neighbouring tests:

dotnet test tests/SkiaSharp.Tests.Console/SkiaSharp.Tests.Console.csproj --filter "FullyQualifiedName~<YourTestName>"
# then a wider relevant slice, e.g. the type's test class, to catch regressions

Enforce red→green in both directions: revert the fix ⇒ red; re-apply ⇒ green.

3.4 Self-review gate — before you open the PR

Run this checklist before committing or opening the PR, so a bad attempt is dropped now instead of pushed and reverted. If any box can't be ticked, fix it or stand down (emit a noop) — do not open the PR.

• The test genuinely goes red without the fix, green with it, both directions (§3.3) — not green because a test was muted, [Obsolete]-hidden, skipped, or weakened.
• The fix is inside binding/** / source/** only — no *.generated.cs, no externals/skia/**, no native / upstream change.
• No public signature changed — overloads / internals only (ABI stable).
• The selected reference's Watch out (❌ don't): note does not describe what you just did (no unconditional same-instance Dispose, no blind owns: flip, no field nulled before dispose, no pinned GCHandle or hand-rolled keep-alive field/List<T> where SKObject.Referenced(...) / KeepAliveObjects (or a plain field) suffices, …).
• This is a real leak with a citable path — not hardened, documented code rationalised as a "decoy," and not a finding manufactured to avoid a quiet run.
• Not already covered by an open issue/PR (§1.3).

All ticked ⇒ proceed to Phase 4 (file the finding, then open the PR). Any unticked ⇒ no PR (fix it, file the finding as an issue on its own, or noop).

Phase 4 — File the finding, then open the linked fix PR

A confirmed, managed-C#-fixable leak produces two linked safe outputs so the finding and the fix are tracked separately and the issue auto-closes when the PR merges.

Labels (both the issue and the PR): a memory leak is a performance concern, so tag both outputs with tenet/performance (the quality-tenet umbrella) and perf/memory-leak (the performance sub-type). When this skill runs from the memory-leak-fixer workflow these labels are applied automatically by its safe-outputs config; when filing by hand, add them yourself. The perf/* taxonomy is defined in the issue-triage skill (references/labels.md).

4.1 The issue — the finding

Emit a create_issue that describes the leak, not the fix. Give it a temporary_id (format aw_ + 3–8 alphanumeric characters — no underscores or other symbols — e.g. aw_leak1) so the PR can reference it before its real number exists. Body (markdown):

• AI-generated banner naming this workflow + the memory-leak-fixer skill.
• Focus area, and the retention/ownership path with file:line citations.
• Evidence: the Phase 2 proof — the probe you ran and its alive/collected counts.
• Scope note: framework bug vs footgun; empirically-proven vs statically-reasoned; ABI impact.
• Labels: tenet/performance + perf/memory-leak.

4.2 The PR — the fix

Create a feature branch (dev/memory-leak-<short-desc>), commit the test + fix, and open a draft create_pull_request. Body (markdown):

• AI-generated banner naming this workflow + skill.
• The fix: what changed and why it is the idiomatic pattern (point at the focus area's Fix ✓).
• Proof (red→green): the failing-then-passing test and the exact dotnet test commands.
• A closing keyword on its own line so merging auto-closes the finding: Fixes #<temporary_id> — e.g. Fixes #aw_leak1 (the id you gave the issue in 4.1). gh-aw rewrites it to the real issue number once the issue is created.
• Labels: tenet/performance + perf/memory-leak.

4.3 Out of scope (native / upstream only)

If the leak is real but the only correct fix lives under externals/skia/** (incl. the C shim), do not open a PR. Emit the create_issue from 4.1 alone — finding plus the proposed native fix — so nothing is lost.

Phase 5 — Report

Write a short summary: which focus area, the candidate (file:line), the proof result, and the resulting issue + PR links. Name the actual checked universe and evidence: for an exhaustive claim, name the bounded query/path and confirm that every returned result was inspected without truncation; for a sample, say it was representative and name the files or candidates actually opened. Never infer an exhaustive scan or aggregate count from a few representative reads. When run from the agentic workflow, append this to the run's step summary.

End with the right safe output(s):

• Confirmed + managed-C# fix → the issue + PR pair from Phase 4 (the PR body carries Fixes #… so merging closes the issue).
• Confirmed but native/upstream-only fix → the create-issue alone (finding + proposal).
• Quiet run (no convincing candidate) or a dry run → a single noop carrying this summary.

A noop is the correct "nothing to do / analysis only" signal — never finish with no safe output, which makes the run look incomplete.