test(cron): cover ticker inter-tick interval pacing (#67102) - #67204
test(cron): cover ticker inter-tick interval pacing (#67102)#67204xxiaoxiong wants to merge 1 commit into
Conversation
The cron ticker characterization suite in tests/cron/test_scheduler_provider.py covers stop_event semantics, BaseException survival, heartbeat recording, and drain-pause gating — but every existing test drives the loop with interval=0 to keep it tight. That means the production pacing contract — InProcessCronScheduler.start() parking on stop_event.wait(interval) between iterations, NOT busy-looping — is unverified. This adds one targeted test using interval=0.2 that asserts the second call to cron.scheduler.tick arrives no sooner than ~0.2s after the first, while still exiting cleanly when stop_event is set. Verified RED→GREEN: when stop_event.wait(interval) is intentionally broken to stop_event.wait(0) the test fails (gap=0.000s < 0.2s); restored code passes. Issue NousResearch#67102 noted the cron ticker lacked dedicated unit tests; this fills the one piece of contract not already exercised. The other three test names suggested in NousResearch#67102 (test_cron_ticker_stop_event, test_cron_ticker_with_adapter, test_cron_ticker_handles_adapter_error) are conceptually already covered by test_inprocess_provider_ticks_and_stops, test_inprocess_provider_skips_dispatch_ while_draining, and test_ticker_survives_baseexception_from_tick respectively, so this PR is intentionally a focused one-test gap-fill, not a duplicate suite.
alt-glitch
added
type/test
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused characterization test. It exercises the production pacing operation at cron/scheduler_provider.py:219 without changing runtime behavior.
Problems
tests/cron/test_scheduler_provider.py:246says the test allows a 1.5×-interval jitter margin, but the assertion attests/cron/test_scheduler_provider.py:249is exactlygap >= interval; no such margin exists.
Suggested changes
- Update or remove the inaccurate comment so the documented contract matches the assertion.
Automated hermes-sweeper review.
| assert not t.is_alive(), "ticker did not exit after stop_event was set" | ||
| assert len(calls) >= 2, "ticker did not fire at least two ticks" | ||
| gap = calls[1] - calls[0] | ||
| # Allow a small scheduling-jitter margin (default = 1.5x interval) so this |
There was a problem hiding this comment.
The assertion below is exactly gap >= interval; it does not implement a 1.5× jitter margin. Please update or remove this comment so it matches the executable contract.
teknium1
added
sweeper:risk-automation
SummaryNine PRs are associated with this issue complex: #44049, #44050, #46207, #47358, #52259, and #67418 address the Desktop-versus-Gateway cron race, while #67135, #67198, and #67204 add scheduler-provider tests without changing ownership or delivery behavior. The fix diffs range from obsolete direct-ticker guards and lifecycle suppression to provider-aware runtime-lock deferral; the recorded cross-issue best-fix verdict selects #44050, although each PR header records verify_verdikt=n/a. Related pull requests
Duplicates#44049 and #44050 implement substantially the same runtime-lock-backed scheduler deferral; #46207 and #47358 are obsolete direct-ticker variants, while the ownership portions of #52259 and #67418 address the same race through broader lifecycle, timeout, or status-probe changes. #67135, #67198, and #67204 are independent test PRs, not ownership-fix duplicates. Suggested consolidationAuthor action: rebase #44050 onto current main, retaining its built-in-only provider integration and regression coverage; this preserves the recorded best-fix verdict without recommending a merge. Close #44049, #46207, and #47358 as duplicates of #44050; for #52259, split out the standalone-send timeout before closing its ownership portion as duplicate, and for #67418, split independently justified status/profile hardening before closing its ownership gate as duplicate. These closure recommendations explicitly differ from the keep_open reviews on #46207, #47358, #52259, and #67418 because the first two target the obsolete direct-ticker path, while the latter two combine overlapping ownership behavior with distinct salvageable work. Keep #67135 and #67198 open with their complementary #67102 coverage as the salvage path, and keep #67204 open with its real-time pacing test after correcting the inaccurate jitter-margin comment. Cross-PR triage: Reviewed 9 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 114 kB of PR diffs, 42 kB of issue/PR text, 15 kB of discussion (22 comments), 25 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Closing stale PR — superseded by upstream work / no longer relevant. |
4 participants
What
Adds one targeted characterization test to
tests/cron/test_scheduler_provider.pylocking the inter-tick interval pacing ofInProcessCronScheduler.start()— the only piece of the ticker contract not already covered.Why
Issue #67102 asks for dedicated cron-ticker unit tests. Audit showed the suite is already broad (38 tests), but every existing test drives the loop with
interval=0to keep it tight. The production pacing contract —stop_event.wait(interval)parking the ticker for ~60s between iterations, NOT busy-looping — was not actually exercised by any test.This fills that one gap with
interval=0.2.Mapping to #67102 issue's suggested test names
test_cron_ticker_fires_at_intervaltest_inprocess_provider_sleeps_for_interval_between_tickstest_cron_ticker_stop_eventtest_inprocess_provider_ticks_and_stopstest_cron_ticker_with_adaptertest_inprocess_provider_skips_dispatch_while_draining(covers the can_dispatch adapter gate path)test_cron_ticker_handles_adapter_errortest_ticker_survives_baseexception_from_tickSo this PR is intentionally a focused one-test gap-fill, not a duplicate suite.
RED → GREEN verification
To prove the new test actually locks the timing contract (not just incidentally passes), I verified bidirectionally:
cron/scheduler_provider.pyto callstop_event.wait(0)instead ofstop_event.wait(interval)— simulating a busy-loop regression. The new test fails as expected:The other 38 tests remain green in both directions.
Notes for maintainer
_wait_untilpolling helper (the suite already uses it to avoid fixed-sleep flakiness under loaded CI).gap >= intervalcleanly with no jitter margin (interval=0.2 is small enough that scheduling jitter is negligible); a busy-loop regression would land at gap≈0, well below the threshold.mainis up-to-date with upstream.