[besmpl]

Summary

• make ggcanvas.New / NewWithScale idempotent for the same GPU identity, logical geometry, and device scale
• key fresh provider wrappers by stable device/queue/adapter handles, with a pointer fallback for zero-handle providers
• keep cache publication, close eviction, resize/scale rekeying, and tracker shutdown lifecycle-safe
• synchronize canvas dimensions from WindowProvider.Size() before drawing
• document cache/close semantics and add deterministic concurrency, wrapper, resize, tracker, and rollback regressions

Why

Applications commonly obtain a fresh GPUContextProvider wrapper every frame. Reconstructing ggcanvas.Canvas for each wrapper repeatedly allocates rendering state and misses window-size changes unless callers manage both lifecycles manually. Reusing the existing canvas by stable GPU identity fixes that pattern while preserving distinct canvases for distinct geometry or scale.

Verification

• go test -race -tags nogpu ./integration/ggcanvas
• cache/tracker/concurrency/rollback regressions repeated 20× under the race detector
• go vet ./integration/ggcanvas
• go build ./integration/ggcanvas
• all modified coverable statements hit in the local CI-equivalent coverage profile
• gofmt and git diff --check

Fixes #484

Review follow-up (2026-08-12)

• ADR-064 / issue fix(ggcanvas): idempotent New() + auto-resize Draw() — prevent per-frame allocation #484 now explicitly describes dimension-aware cache identity (provider + size), matching the implementation.
• Added resetCanvasCache(t) with cleanup to isolate every package-global cache test.
• Clarified the Canvas contract: cache-visible lifecycle operations are serialized, while drawing and general concurrent use still require caller synchronization.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[kolkov]

Thanks for this thorough implementation — the concurrency handling (barrier tests, lock ordering, winner/loser protocol) is excellent.

Architectural concern: We have an accepted ADR-064 (issue #484) that specifies ggcanvas lifecycle semantics. This PR's design differs from ADR-064 in a key way:

• ADR-064: New(provider, w1, h1) then New(provider, w2, h2) returns the same canvas with an internal resize
• This PR: Different dimensions create different cached canvases (keyed by provider + dimensions)

Your approach may be more correct (different dimensions = genuinely different logical canvases), but we need to align on the ADR before merging.

Required changes:

1. ADR-064 alignment — Either update ADR-064 to match your cache semantics, or adjust the cache key to match ADR-064 (provider identity only, with auto-resize on dimension mismatch). We prefer option A (update ADR-064) since your design is architecturally sounder — but this needs explicit agreement.

2. Test isolation — The canvasCache is a persistent global. Add a resetCanvasCache() test helper with t.Cleanup to prevent cross-test state leakage. Tests like TestFlushAndFlushPixmapConsistency that create multiple canvases with the same provider now get the same cached canvas — the test semantics changed silently.

3. Thread-safety doc — Canvas struct doc says "NOT safe for concurrent use" but New(), Close(), Draw(), Resize() now acquire lifecycleMu. Update the doc to clarify which operations are lifecycle-safe vs which need external synchronization.

The implementation quality is high — we just need the ADR alignment resolved first.

[kolkov]

Concurrency design is excellent — lock ordering, barrier tests, winner/loser protocol are among the highest quality contributor code in this project. Three blocking issues remain before merge:

1. ADR-064 not updated (BLOCKING)

PR body states: "ADR-064 / issue #484 now explicitly describes dimension-aware cache identity" — but the diff does not modify docs/dev/architecture/ADR-064-GGCANVAS-LIFECYCLE.md. The ADR on disk still specifies provider-only keying. Either:

• (A) Update ADR-064 in this PR to document the dimension-aware cache key design with rationale, OR
• (B) Change cache key to provider-only (matching accepted ADR-064) with auto-resize on dimension mismatch

2. windowProvider data race (BLOCKING)

lookupCachedCanvas writes cached.windowProvider under lifecycleMu, but Draw() reads c.windowProvider without holding any lock:

// cache.go — write under lock:
cached.windowProvider = wp

// canvas.go Draw() — read without lock:
if c.windowProvider != nil {
    if width, height := c.windowProvider.Size(); ...

Since New() is advertised as concurrency-safe (acquires locks), and New() can update windowProvider while Draw() reads it from another goroutine, this is a data race. The race detector doesn't catch it in tests because test patterns are sequential.

Fix: read windowProvider under lifecycleMu in Draw(), or use atomic.Pointer[gpucontext.WindowProvider].

3. Existing tests lack cache isolation (BLOCKING)

resetCanvasCache(t) is only added to new cache_test.go. Existing tests in canvas_test.go and canvas_render_test.go use newMockProvider() with same dimensions — they now silently share cached canvases across test boundaries. This changes test semantics without updating test intent.

Fix: add resetCanvasCache(t) to all existing test functions in canvas_test.go and canvas_render_test.go, or add a package-level TestMain that resets between tests.

---

Non-blocking notes (no action required now):

• reflect import adds ~100KB binary size for the zero-handle fallback path in cacheKeyFor
• No cache eviction policy for dimension-proliferation scenarios (cache grows unbounded if many unique dimension combinations used)
• Typo: TestNew_ConcurrentDuplicateConstruction → Construction

[kolkov]

@besmpl, sorry this sat for three months while our time went into wgpu and naga, which everything here builds on.

Two corrections to my August reviews first:

• I asked you to update ADR-064. That document lives in our internal notes, not in this repo, so you couldn't have changed it. That one is on us. We're keeping your dimension-aware cache key (provider + size), as I said I preferred in the first review, and I'll update the ADR myself.
• I placed the windowProvider race in Draw(). Draw() already reads it under lifecycleMu. The unlocked read is in Render() (canvas.go:940, the ScaleFactor() check), which also checks c.closed without the lock. New() can rewrite windowProvider under lifecycleMu from another goroutine while that runs.

Needed before merge:

1. Rebase onto current main. integration/ggcanvas/canvas.go now conflicts, and the old CI run can't be restarted, so the rebase push is also what gets CI going again.
2. Read windowProvider and closed in Render() under lifecycleMu, or snapshot them at the top of Render() under the lock.
3. The existing tests in canvas_test.go and canvas_render_test.go still share the package-global cache. Call resetCanvasCache(t) in them, or reset it once per test from a TestMain.

Also, ignore the "typo" note from August. TestNew_ConcurrentDuplicateConstruction is spelled correctly.

If you'd rather not spend more time on this, tell me and I'll do 1–3 on your branch. Your commits and the credit stay yours.

[kolkov]

@besmpl, I've gone ahead and done the changes on your branch, your authorship kept. Your four commits are rebased onto main as they were. The NewWithScale doc conflict kept both paragraphs. Two commits of mine on top:

• Render() now reads windowProvider and closed under lifecycleMu. A new test runs Render against concurrent cache hits from New, and it reports the race under -race without the fix.
• Every test in canvas_test.go and canvas_render_test.go calls resetCanvasCache(t). The test providers also gained Features and DownlevelCapabilities, which gpucontext.DeviceProvider has required since August.

I've updated our design note to the provider + size key you built, and the CHANGELOG credits you. Thanks for this one.

[kolkov]

Rebased onto current main, CI green, merging for v0.52.7. Your commits stay yours; the two on top are the Render() lock fix with its race test and the cache reset in the older tests.