Fix idle timer review issues

internal/app/timers_test.go

@@ -221,3 +221,193 @@ func TestTimerOwnershipEnforced(t *testing.T) {
 		t.Fatal("expected ownership error from foreign pause")
 	}
 }
+
+// TestTimerResumeRechecksIdleAll covers the case where every watched
+// child becomes idle while an idle_all timer is paused. Without a resume
+// re-check, the timer would stay pending forever because the state
+// transitions happened during the pause window.
+func TestTimerResumeRechecksIdleAll(t *testing.T) {
+	sess, mgr, rec := newTestManager(t)
+	owner := fakeChild("p_owner")
+	a := fakeChild("p_a")
+	b := fakeChild("p_b")
+	addChild(sess, owner)
+	addChild(sess, a)
+	addChild(sess, b)
+	working := StateWorking
+	a.idleState.Store(&working)
+	b.idleState.Store(&working)
+
+	resp, err := mgr.TimerFireWhenIdleAll("p_owner", "all done", "", []string{"p_a", "p_b"}, 0)
+	if err != nil {
+		t.Fatalf("TimerFireWhenIdleAll: %v", err)
+	}
+	if resp.Status != "pending" {
+		t.Fatalf("status: got %q want pending", resp.Status)
+	}
+	if err := mgr.TimerPause("p_owner", resp.ID); err != nil {
+		t.Fatalf("Pause: %v", err)
+	}
+
+	// Both watched children become idle WHILE THE TIMER IS PAUSED, so
+	// onChildStateChanged is not consulted for this timer.
+	idle := StateIdle
+	a.idleState.Store(&idle)
+	b.idleState.Store(&idle)
+
+	if err := mgr.TimerResume("p_owner", resp.ID); err != nil {
+		t.Fatalf("Resume: %v", err)
+	}
+	got := rec.snapshot()
+	if len(got) != 1 || got[0].Body != "all done" {
+		t.Fatalf("expected fire on resume, got: %+v", got)
+	}
+}
+
+// TestTimerResumeRechecksIdleAny covers the same missed-transition shape
+// for idle_any: a non-baseline watched child going idle during pause must
+// fire on resume.
+func TestTimerResumeRechecksIdleAny(t *testing.T) {
+	sess, mgr, rec := newTestManager(t)
+	owner := fakeChild("p_owner")
+	a := fakeChild("p_a")
+	addChild(sess, owner)
+	addChild(sess, a)
+	working := StateWorking
+	a.idleState.Store(&working)
+
+	resp, err := mgr.TimerFireWhenIdleAny("p_owner", "one done", "", []string{"p_a"}, 0)
+	if err != nil {
+		t.Fatalf("TimerFireWhenIdleAny: %v", err)
+	}
+	if resp.Status != "pending" {
+		t.Fatalf("status: got %q want pending", resp.Status)
+	}
+	if err := mgr.TimerPause("p_owner", resp.ID); err != nil {
+		t.Fatalf("Pause: %v", err)
+	}
+	idle := StateIdle
+	a.idleState.Store(&idle)
+	if err := mgr.TimerResume("p_owner", resp.ID); err != nil {
+		t.Fatalf("Resume: %v", err)
+	}
+	got := rec.snapshot()
+	if len(got) != 1 || got[0].Body != "one done" {
+		t.Fatalf("expected fire on resume, got: %+v", got)
+	}
+}
+
+// TestTimerResumeIdleAnyExcludesBaselineDuringPause guards against a
+// resume re-check firing for a watcher that was idle at registration
+// (and therefore part of the baseline) — only non-baseline transitions
+// should satisfy idle_any.
+func TestTimerResumeIdleAnyExcludesBaselineDuringPause(t *testing.T) {
+	sess, mgr, rec := newTestManager(t)
+	owner := fakeChild("p_owner")
+	a := fakeChild("p_a")
+	b := fakeChild("p_b")
+	addChild(sess, owner)
+	addChild(sess, a)
+	addChild(sess, b)
+	idle := StateIdle
+	working := StateWorking
+	a.idleState.Store(&idle)    // baseline: already idle
+	b.idleState.Store(&working) // not baseline
+
+	resp, err := mgr.TimerFireWhenIdleAny("p_owner", "one done", "", []string{"p_a", "p_b"}, 0)
+	if err != nil {
+		t.Fatalf("TimerFireWhenIdleAny: %v", err)
+	}
+	if err := mgr.TimerPause("p_owner", resp.ID); err != nil {
+		t.Fatalf("Pause: %v", err)
+	}
+	// b stays working — only a is idle, and a was baseline. Resume
+	// must not fire.
+	if err := mgr.TimerResume("p_owner", resp.ID); err != nil {
+		t.Fatalf("Resume: %v", err)
+	}
+	if got := rec.snapshot(); len(got) != 0 {
+		t.Fatalf("unexpected fire on resume: %+v", got)
+	}
+}
+
+// TestTimerRecordsRemovedOnFire ensures fired delay timers don't leak
+// in the timer registry — bodies and metadata must be released.
+func TestTimerRecordsRemovedOnFire(t *testing.T) {
+	sess, mgr, rec := newTestManager(t)
+	c := fakeChild("p_owner")
+	addChild(sess, c)
+	id, err := mgr.TimerSet("p_owner", "wake up", "test", 0.05)
+	if err != nil {
+		t.Fatalf("TimerSet: %v", err)
+	}
+	deadline := time.Now().Add(time.Second)
+	for time.Now().Before(deadline) {
+		if len(rec.snapshot()) > 0 {
+			break
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+	if len(rec.snapshot()) != 1 {
+		t.Fatalf("timer didn't fire")
+	}
+	mgr.mu.Lock()
+	_, stillThere := mgr.timers[id]
+	count := len(mgr.timers)
+	mgr.mu.Unlock()
+	if stillThere {
+		t.Fatalf("fired timer %s was not removed from registry", id)
+	}
+	if count != 0 {
+		t.Fatalf("timer registry not drained: %d entries", count)
+	}
+}
+
+// TestTimerRecordsRemovedOnCancel ensures canceled timers are dropped
+// from the registry.
+func TestTimerRecordsRemovedOnCancel(t *testing.T) {
+	sess, mgr, _ := newTestManager(t)
+	c := fakeChild("p_owner")
+	addChild(sess, c)
+	id, err := mgr.TimerSet("p_owner", "x", "", 60)
+	if err != nil {
+		t.Fatalf("TimerSet: %v", err)
+	}
+	if err := mgr.TimerCancel("p_owner", id); err != nil {
+		t.Fatalf("Cancel: %v", err)
+	}
+	mgr.mu.Lock()
+	_, stillThere := mgr.timers[id]
+	mgr.mu.Unlock()
+	if stillThere {
+		t.Fatalf("canceled timer %s was not removed from registry", id)
+	}
+}
+
+// TestTimerRecordsRemovedOnIdleFire ensures idle_* timers are dropped
+// from the registry once they fire via onChildStateChanged.
+func TestTimerRecordsRemovedOnIdleFire(t *testing.T) {
+	sess, mgr, rec := newTestManager(t)
+	owner := fakeChild("p_owner")
+	a := fakeChild("p_a")
+	addChild(sess, owner)
+	addChild(sess, a)
+	working := StateWorking
+	a.idleState.Store(&working)
+	resp, err := mgr.TimerFireWhenIdleAny("p_owner", "one done", "", []string{"p_a"}, 0)
+	if err != nil {
+		t.Fatalf("TimerFireWhenIdleAny: %v", err)
+	}
+	idle := StateIdle
+	a.idleState.Store(&idle)
+	mgr.onChildStateChanged("p_a", StateIdle)
+	if got := rec.snapshot(); len(got) != 1 {
+		t.Fatalf("expected fire, got: %+v", got)
+	}
+	mgr.mu.Lock()
+	_, stillThere := mgr.timers[resp.ID]
+	mgr.mu.Unlock()
+	if stillThere {
+		t.Fatalf("fired idle timer %s was not removed from registry", resp.ID)
+	}
+}