From 7f3e0bb59a7697e480fcee2263642f8ed9ba70be Mon Sep 17 00:00:00 2001
From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Date: Wed, 9 Sep 2026 12:35:42 +0530
Subject: [PATCH] test(agent): the worker-start-failure test intercepts the
 callback worker again

`kwargs.get("target") is sched._run_callback` is always False (a bound method is a fresh object
per access), so the fake never returned Boom and the _dispatch failure branch went untested;
the test passed on the normal worker. Compare with == and assert the interception happened
(mutation: retiring the handle on start failure now fails the test).

The thread-count assertions sampled while per-fire workers were still live; quiesce every
handle with cancel(wait=) before sampling so the count is deterministic (AGENTS.md: timing tests
must not assume a quiet runner).

Follow-up to #106308.
---
 tests/agent/test_periodic_scheduler.py | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/tests/agent/test_periodic_scheduler.py b/tests/agent/test_periodic_scheduler.py
index 839af9b805..84fe6dfeb4 100644
--- a/tests/agent/test_periodic_scheduler.py
+++ b/tests/agent/test_periodic_scheduler.py
@@ -24,18 +24,18 @@ def test_two_intervals_fire_proportionally_and_cancel_stops_one():
 
     assert _wait_until(lambda: len(slow) >= 3)
     assert len(fast) > len(slow)  # 5x interval ratio -> clearly more fast ticks
-    # Scheduling another timer creates no persistent per-handle thread.
-    before = threading.active_count()
-    sched.schedule(lambda: None, 0.01).cancel()
-    assert threading.active_count() == before
     assert sched._thread is not None and sched._thread.is_alive()
 
-    h_fast.cancel()
+    h_fast.cancel(wait=1.0)
     n_fast = len(fast)
     time.sleep(0.1)
     assert len(fast) == n_fast, "cancelled callback kept firing"
     assert len(slow) > 3, "sibling callback stopped when another was cancelled"
-    h_slow.cancel()
+    h_slow.cancel(wait=1.0)
+    # With every handle quiesced, scheduling + cancelling adds no persistent thread.
+    before = threading.active_count()
+    sched.schedule(lambda: None, 0.01).cancel(wait=1.0)
+    assert threading.active_count() == before
 
 
 def test_raising_callback_is_rescheduled_and_does_not_kill_sibling():
@@ -81,15 +81,15 @@ def test_module_level_schedule_uses_shared_default():
     hits = []
     h = schedule(lambda: hits.append(1), 0.01)
     assert _wait_until(lambda: hits)
-    h.cancel()
+    h.cancel(wait=1.0)
     thread = periodic_scheduler._DEFAULT._thread
     assert thread is not None and thread.name == "hermes-periodic-scheduler"
-    # Scheduling more timers on the shared default adds no OS threads.
+    # Scheduling more timers on the shared default adds no persistent OS threads.
     before = threading.active_count()
     handles = [schedule(lambda: None, 0.01) for _ in range(20)]
-    assert threading.active_count() == before
     for handle in handles:
-        handle.cancel()
+        handle.cancel(wait=1.0)
+    assert threading.active_count() == before
 
 
 def test_blocked_callback_does_not_stall_due_sibling(monkeypatch):
@@ -132,7 +132,8 @@ def test_worker_start_failure_keeps_timer(monkeypatch):
     def flaky(*args, **kwargs):
         # Only this scheduler's own callback worker fails, once; a leaked handle on the shared
         # _DEFAULT scheduler must not be the one that consumes the single Boom.
-        if kwargs.get("target") is sched._run_callback and attempts["n"] == 0:
+        # Bound methods are fresh objects per access: compare with ==, never `is`.
+        if kwargs.get("target") == sched._run_callback and attempts["n"] == 0:
             attempts["n"] += 1
 
             class Boom:
@@ -148,6 +149,7 @@ def test_worker_start_failure_keeps_timer(monkeypatch):
         assert _wait_until(lambda: bool(fired), timeout=3.0), (
             "worker-start failure silently retired the timer"
         )
+        assert attempts["n"] == 1, "the fake never intercepted the callback worker"
         assert not handle.cancelled
     finally:
-        handle.cancel()
+        handle.cancel(wait=1.0)