Skip to content

test(cron): add coverage for pre-set stop event - #67135

Open
devxMani wants to merge 1 commit into
NousResearch:mainfrom
devxMani:main
Open

test(cron): add coverage for pre-set stop event#67135
devxMani wants to merge 1 commit into
NousResearch:mainfrom
devxMani:main

Conversation

@devxMani

Copy link
Copy Markdown

Adds a unit test covering the edge case where the in-process cron scheduler is started with an already-set stop event.

The test verifies that the scheduler exits cleanly without continuing background work when shutdown has already been requested. This improves test coverage around the scheduler lifecycle without modifying production code.

Related to #67102

  • ✅ Tests (adding or improving test coverage)

  • Added a new unit test in tests/cron/test_scheduler_provider.py.

  • Covered the scenario where InProcessCronScheduler starts with the stop event already set.

  • No production code was modified.

  1. Run the cron scheduler test suite.
  2. Verify the new test executes successfully.
  3. Confirm existing cron scheduler tests continue to pass.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have labels Jul 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this adds the pre-set stop_event scheduler-provider test requested by #67102; the broader ticker coverage request remains open.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for adding the focused lifecycle coverage. Current main dispatches cron_tick() only inside the while not stop_event.is_set() guard in cron/scheduler_provider.py:192; the new test directly protects that pre-start shutdown behavior. The existing test at tests/cron/test_scheduler_provider.py:147 verifies shutdown after the ticker has started, so this is not duplicate coverage.

This is an automated hermes-sweeper review.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Nine open PRs are associated with this 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 scheduler ownership. The fix diffs range from obsolete direct-ticker guards and startup suppression to provider-aware runtime-lock deferral; the recorded cross-issue best-fix selection is #44050, although every 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 complementary test PRs, not ownership-fix duplicates.

Suggested consolidation

Author action: rebase #44050 onto current main while preserving its built-in-only provider integration and regression coverage; this retains the recorded best-fix target without making a merge recommendation. Then close #44049, #46207, and #47358 as duplicates of #44050, and split the standalone-send timeout from #52259 and independently justified status/profile hardening from #67418 before closing their remaining ownership portions as duplicates; despite the keep_open reviews on #46207, #47358, #52259, and #67418, the first two target the obsolete direct-ticker path and the latter two contain distinct salvageable work around an overlapping ownership fix. Keep #67135 and #67198 open for their complementary scheduler-test coverage, and keep #67204 open after the review-requested comment correction.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants