No Window.Close teardown: animation pumper goroutine survives window close and keeps calling RequestRedraw; animPumper.Stop can silently miss its goroutine

[AnyCPU]

Depends on: #171 — Window.Close calls overlay.Stack.Clear(), which that patch introduces (that patch notes Clear "has no in-tree caller yet"; this one wires the intended caller). The diff below is generated against main (a961458) with that patch applied — apply it first.

Summary

app.Window owns a long-lived background goroutine — the animation pumper, a time.Ticker loop holding a captured gpucontext.WindowProvider reference — but has no Close method and no teardown path of any kind. The only thing that ever stops the pumper is an idle-frame heuristic inside Window.Frame(), and Frame() stops being called the moment the window closes. Close a window while an animation is running (a spinner, an in-progress transition) and the pumper goroutine survives forever: the ticker keeps firing and keeps calling wp.RequestRedraw() against a window context that no longer exists. Every close-while-animating cycle leaks a goroutine + ticker, and the stray RequestRedraw calls land on a dead window.

The window's overlay stack has the same problem: overlays are window-owned and unreachable from the root widget tree, so nothing unmounts open overlays (and releases their signal bindings) when the window goes away.

While fixing this, a latent race in animPumper.Stop() surfaced: it performed a non-blocking send on an unbuffered channel (select/default), which silently does nothing if the goroutine isn't parked on its select at that exact instant (e.g. it is inside wp.RequestRedraw()). That was already flaky for the repeated idle-heuristic call it was written for; for a one-shot deterministic Close() it is a correctness bug.

Root cause

All references are to main (v0.1.44-era, a961458) with the overlay-lifecycle patch applied; verified by reading:

• app/window.go has no Close method. The only animToken.Stop() call in the file sits inside Frame()'s idle heuristic (app/window.go:612-630, stop at :627): the pumper stops only after 3 consecutive idle frames — a code path that requires the window to still be alive and pumping frames.
• The pumper goroutine (app/window.go:1618-1633, newAnimPumperWithInterval) loops on select { case <-p.stop; case <-ticker.C: wp.RequestRedraw() }. Once started, nothing outside Frame() can reach it.
• animPumper.Stop() (app/window.go:1635-1640) is a non-blocking send guarded by select/default. If the goroutine is between select iterations — mid-RequestRedraw is the common case, since that is where it spends its non-parked time — the send hits default and is silently dropped, and the pumper never stops.
• Two call sites lazily re-create the pumper whenever animToken == nil: the ScheduleAnimationFrame callback (app/window.go:244-249) and Frame()'s active branch (app/window.go:618-620). Any close implementation that merely stops the pumper can therefore have it resurrected by an animation frame scheduled immediately after close.
• desktop.Run's gogpuApp.OnClose (desktop/desktop.go:73-79) tears down GPU resources (boundary textures, accelerator, canvas) but performs no app.Window teardown at all.
• Open overlays: overlay.Stack is owned by the window (app/window.go:58), not reachable from the root tree, so window teardown is the only place they can be unmounted — and no such place existed.

Fix

A new idempotent Window.Close() (app/window.go), wired into desktop.Run's gogpuApp.OnClose before GPU teardown:

• Stops the pumper deterministically and nils animToken.
• Clears the overlay stack via overlay.Stack.Clear(), unmounting every open overlay and releasing its signal bindings (dropdowns/dialogs/popovers open at close time no longer outlive the window).
• Sets a closed guard checked by both lazy-recreate sites, so a ScheduleAnimationFrame or Frame() call racing in after Close() cannot strand a fresh pumper goroutine. Repeated Close() calls are no-ops.
• Close deliberately does not unmount the root tree — that remains SetRoot's job; the method's doc comment spells this out.

animPumper.Stop() is rewritten to close(p.stop) guarded by a per-instance sync.Once: a channel close is observed by the goroutine's select-receive regardless of timing (no lost-wakeup window), and the Once makes repeated/concurrent Stop() calls safe — necessary now that both the idle heuristic and Close() may call it. Since newAnimPumperWithInterval always allocates a fresh struct (fresh channel, fresh Once), one instance's close can never affect another.

The design rule this enforces: every long-lived goroutine owned by a window must be stopped at a deterministic, observable lifecycle event — the idle heuristic remains as the steady-state optimization, but it is no longer the only stop path.

Verification

New test file app/window_close_test.go (included in the patch; reuses the Lifecycle-aware overlay content helper from the overlay-lifecycle patch's test file):

• TestWindow_Close_StopsAnimPumperAndClearsOverlays — starts a pumper the way a real spinner does (ctx.ScheduleAnimationFrame()), pushes an overlay, calls Close(), and asserts: animToken is nil, OverlayCount() is 0, the overlay content's Unmount ran exactly once, a second Close() is a no-op, and a post-Close ScheduleAnimationFrame() does not resurrect the pumper. On the unpatched base this fails to compile (*Window has no Close), and each assertion targets one specific piece of the wiring (stop / clear / guard), so a partial fix fails loudly.
• TestAnimPumper_StopIsRaceFree — 50 goroutines call Stop() concurrently, then once more from the test goroutine: must not panic (close of closed channel) and the pumper must actually stop ticking (RequestRedraw count stable across a 20 ms window at a 1 ms tick interval). Passes repeatedly under go test -race -count=3.
• The shared test mock's redrawCount becomes an atomic.Int32 (with the one existing read updated), because RequestRedraw is called from the pumper's background goroutine while tests read it — the old plain-int counter was itself a data race under -race.

go build ./..., full go test ./..., go vet, gofmt -l, and golangci-lint run are all clean, on this patch alone and with the overlay-lifecycle patch stacked. The fix has been running in a production tree since 2026-07-06 with no regressions in animation start/stop behavior (the idle-heuristic path is untouched).

Patch

Base: main (a961458) plus the overlay-lifecycle patch (see "Depends on" above).

git apply-able against main + the overlay-lifecycle patch from #171 (269 lines) — Close() clears the overlay stack via Stack.Clear, which that patch introduces.

window-close-anim-pumper-teardown.diff

diff --git a/app/app_test.go b/app/app_test.go
index a8b4456..1e0db71 100644
--- a/app/app_test.go
+++ b/app/app_test.go
@@ -1,6 +1,7 @@
 package app
 
 import (
+	"sync/atomic"
 	"testing"
 
 	"github.com/gogpu/gpucontext"
@@ -44,10 +45,15 @@ func (m *mockWidget) Event(_ widget.Context, e event.Event) bool {
 }
 
 // mockWindowProvider implements gpucontext.WindowProvider for testing.
+//
+// redrawCount is an atomic.Int32 (not a plain int): RequestRedraw is
+// called from the animPumper's background goroutine (app/window.go), so
+// tests that read redrawCount from the test goroutine while a pumper is
+// still running need race-safe access.
 type mockWindowProvider struct {
 	width, height int
 	scale         float64
-	redrawCount   int
+	redrawCount   atomic.Int32
 }
 
 func (m *mockWindowProvider) Size() (int, int) {
@@ -59,7 +65,7 @@ func (m *mockWindowProvider) ScaleFactor() float64 {
 }
 
 func (m *mockWindowProvider) RequestRedraw() {
-	m.redrawCount++
+	m.redrawCount.Add(1)
 }
 
 // mockPlatformProvider implements gpucontext.PlatformProvider for testing.
diff --git a/app/window.go b/app/window.go
index 1bc1285..c9f6458 100644
--- a/app/window.go
+++ b/app/window.go
@@ -3,6 +3,7 @@ package app
 import (
 	"context"
 	"log/slog"
+	"sync"
 	"time"
 
 	"github.com/gogpu/gpucontext"
@@ -69,6 +70,12 @@ type Window struct {
 	// Used to stop the animation pumper after animations complete.
 	animIdleFrames int
 
+	// closed is set by Close() and prevents the animation pumper from being
+	// resurrected afterward: ScheduleAnimationFrame and the Frame() idle
+	// heuristic both lazily recreate animToken when nil, so without this
+	// guard a call racing after Close() could strand a new pumper goroutine.
+	closed bool
+
 	// needsAnimationFrame is set by ScheduleAnimationFrame (during Draw)
 	// and persists across ClearAfterPaint. Checked by desktop.draw frame
 	// skip to ensure animated boundary frames are not skipped.
@@ -244,7 +251,7 @@ func newWindow(
 	ctx.SetOnScheduleAnimation(func() {
 		w.animIdleFrames = 0
 		w.needsAnimationFrame = true
-		if w.animToken == nil && w.wp != nil {
+		if w.animToken == nil && w.wp != nil && !w.closed {
 			w.animToken = newAnimPumper(w.wp)
 		}
 	})
@@ -372,6 +379,29 @@ func (w *Window) Context() *widget.ContextImpl {
 	return w.ctx
 }
 
+// Close releases window-owned resources that outlive the widget tree:
+// it stops the animation pumper goroutine and unmounts every overlay,
+// releasing their signal bindings. Close is idempotent — safe to call
+// more than once — and prevents the pumper from being resurrected by a
+// later ScheduleAnimationFrame or Frame() call.
+//
+// Close does not unmount the root widget tree; callers that also want
+// that should call SetRoot(nil) (or rely on the tree being unreferenced
+// after the Window itself is discarded).
+func (w *Window) Close() {
+	if w.closed {
+		return
+	}
+	w.closed = true
+	if w.animToken != nil {
+		w.animToken.Stop()
+		w.animToken = nil
+	}
+	if w.overlays != nil {
+		w.overlays.Clear()
+	}
+}
+
 // Theme returns the window's current theme.
 func (w *Window) Theme() *theme.Theme {
 	return w.theme
@@ -615,7 +645,7 @@ func (w *Window) Frame() {
 	// and prevent start/stop thrashing from periodic data updates.
 	if w.ctx.IsInvalidated() || !w.ctx.InvalidatedRect().IsEmpty() || w.needsAnimationFrame {
 		w.animIdleFrames = 0
-		if w.animToken == nil && w.wp != nil {
+		if w.animToken == nil && w.wp != nil && !w.closed {
 			w.animToken = newAnimPumper(w.wp)
 		}
 	} else if w.animToken != nil {
@@ -1603,6 +1633,14 @@ func (w *Window) HasDirtyBoundariesOrNeedsRedraw() bool {
 // Stopped when animation completes (3 consecutive idle frames).
 type animPumper struct {
 	stop chan struct{}
+	// stopOnce guards close(stop): Stop() may be called more than once
+	// (idle heuristic + a deterministic Window.Close), and the pumper
+	// goroutine's own select also observes stop. sync.Once makes the
+	// close idempotent and race-free. It is a per-instance field —
+	// newAnimPumperWithInterval always allocates a fresh animPumper (and
+	// therefore a fresh Once and a fresh channel), so closing one
+	// instance's channel can never affect or panic a different instance.
+	stopOnce sync.Once
 }
 
 // defaultAnimPumpInterval controls the animation frame pump rate.
@@ -1632,9 +1670,14 @@ func newAnimPumperWithInterval(wp gpucontext.WindowProvider, interval time.Durat
 	return p
 }
 
+// Stop terminates the pumper goroutine. It is safe to call more than
+// once: a non-blocking send on an unbuffered channel could previously
+// miss the goroutine if it was between select iterations (e.g. inside
+// wp.RequestRedraw()), stranding it even in the steady-state idle path.
+// close() is observed by a select-receive regardless of timing, and
+// sync.Once makes repeated/concurrent Stop() calls race-free.
 func (p *animPumper) Stop() {
-	select {
-	case p.stop <- struct{}{}:
-	default:
-	}
+	p.stopOnce.Do(func() {
+		close(p.stop)
+	})
 }
diff --git a/app/window_close_test.go b/app/window_close_test.go
new file mode 100644
index 0000000..18b289c
--- /dev/null
+++ b/app/window_close_test.go
@@ -0,0 +1,90 @@
+package app
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/gogpu/ui/state"
+)
+
+// Window.Close / animPumper teardown regression tests.
+//
+// These tests are written so that removing the Window.Close wiring, the
+// closed guard, or the sync.Once-based animPumper.Stop makes an assertion
+// FAIL — not vacuously pass.
+
+// TestWindow_Close_StopsAnimPumperAndClearsOverlays covers the pumper stop
+// and the Window.Close overlay teardown wiring.
+func TestWindow_Close_StopsAnimPumperAndClearsOverlays(t *testing.T) {
+	wp := &mockWindowProvider{width: 400, height: 300, scale: 1.0}
+	uiApp := New(WithWindowProvider(wp))
+	win := uiApp.Window()
+	win.SetRoot(newMockWidget())
+
+	sig := state.NewSignal(0)
+	mgr := &windowOverlayManager{window: win}
+	content := newLifecycleOverlayContent(sig)
+	mgr.PushOverlay(content, nil)
+	if win.OverlayCount() != 1 {
+		t.Fatalf("OverlayCount = %d, want 1 before Close", win.OverlayCount())
+	}
+
+	// Start the animation pumper the same way a spinner would.
+	win.Context().ScheduleAnimationFrame()
+	if win.animToken == nil {
+		t.Fatal("animToken should be non-nil after ScheduleAnimationFrame")
+	}
+
+	win.Close()
+
+	if win.animToken != nil {
+		t.Error("animToken should be nil after Close")
+	}
+	if win.OverlayCount() != 0 {
+		t.Errorf("OverlayCount = %d after Close, want 0", win.OverlayCount())
+	}
+	if content.unmountCalled != 1 {
+		t.Errorf("unmountCalled = %d after Close, want 1", content.unmountCalled)
+	}
+
+	// Close must be idempotent.
+	win.Close()
+
+	// A pumper must not be resurrected after Close.
+	win.Context().ScheduleAnimationFrame()
+	if win.animToken != nil {
+		t.Error("animToken should stay nil after Close even if ScheduleAnimationFrame is called again")
+	}
+}
+
+// TestAnimPumper_StopIsRaceFree targets a latent race in the original
+// Stop(): it was a non-blocking send on an unbuffered channel guarded by
+// select/default, which both (a) could silently fail to signal the
+// goroutine if it wasn't parked on the select at that instant, and (b) is
+// unsafe to call concurrently/more than once in the close-vs-close sense
+// once converted to a channel close. This test calls Stop() many times
+// concurrently and must not panic ("close of closed channel") and must
+// actually stop the goroutine.
+func TestAnimPumper_StopIsRaceFree(t *testing.T) {
+	wp := &mockWindowProvider{width: 100, height: 100, scale: 1.0}
+	p := newAnimPumperWithInterval(wp, time.Millisecond)
+
+	var wg sync.WaitGroup
+	for range 50 {
+		wg.Go(p.Stop)
+	}
+	wg.Wait() // must not panic
+
+	// Stop again from the test goroutine — still must not panic.
+	p.Stop()
+
+	// Give the goroutine a moment to observe the closed channel and exit,
+	// then confirm no further redraws are requested.
+	time.Sleep(20 * time.Millisecond)
+	before := wp.redrawCount.Load()
+	time.Sleep(20 * time.Millisecond)
+	if after := wp.redrawCount.Load(); after != before {
+		t.Errorf("pumper kept ticking after Stop: redrawCount %d -> %d", before, after)
+	}
+}
diff --git a/app/window_test.go b/app/window_test.go
index 530bc9a..975dbc6 100644
--- a/app/window_test.go
+++ b/app/window_test.go
@@ -1489,7 +1489,7 @@ func TestWindow_AnimPumper_StartsOnInvalidation(t *testing.T) {
 	}
 
 	// Verify RequestRedraw was called (from the invalidation callback).
-	if wp.redrawCount == 0 {
+	if wp.redrawCount.Load() == 0 {
 		t.Error("WindowProvider.RequestRedraw should have been called")
 	}
 }
diff --git a/desktop/desktop.go b/desktop/desktop.go
index 1b2e28d..253bf43 100644
--- a/desktop/desktop.go
+++ b/desktop/desktop.go
@@ -71,6 +71,10 @@ func Run(gogpuApp *gogpu.App, uiApp *app.App) error {
 	gogpuApp.OnDraw(rl.draw)
 
 	gogpuApp.OnClose(func() {
+		// Stop the animation pumper and unmount overlays before tearing
+		// down GPU resources, so no stray RequestRedraw lands on a closing
+		// window and no overlay binding outlives it.
+		rl.uiApp.Window().Close()
 		rl.releaseBoundaryTextures()
 		gg.CloseAccelerator()
 		if rl.canvas != nil {

Environment

• gogpu/ui v0.1.44 (current main) + the overlay-lifecycle patch
• Go 1.26.x, CGO_ENABLED=0
• Platform-independent (pure Go goroutine-lifecycle issue; reproduced headless in unit tests). The severity is highest wherever RequestRedraw on a destroyed native window is unsafe.

Possible follow-up (not in this patch)

A natural companion change is having Window.SetRoot dismiss active overlays when the root is
replaced — overlays belong to the UI that spawned them, and a dropdown surviving a root swap is
arguably stale. We run that variant downstream, but it is a behavior change rather than a
leak fix, so it's deliberately not part of this patch. Happy to send it separately if you agree
replacing the root should clear open overlays.

---

Part of the fix registry: #170

[kolkov]

Confirmed — the animation pumper goroutine outlives the window because there is no Close() teardown on Window. The pumper stops only after 3 consecutive idle frames, but if widgets keep dirtying (or overlay bindings leak per #171), it runs indefinitely.

Your analysis of the animPumper.Stop race (goroutine may not have started yet when Stop is called) is correct. This depends on #171 (overlay Mount/Unmount cleanup) — we need to clean up overlay widget bindings before tearing down the window. We'll address #171 first, then #175.

[kolkov]

Fixed in v0.1.46. Added Window.Close() — stops animation pumper, unmounts overlays + root.