Skip to content

fix(cron): yield desktop ticker to running gateway - #44050

Open
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/cron-owner-desktop-ticker-43965
Open

fix(cron): yield desktop ticker to running gateway#44050
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/cron-owner-desktop-ticker-43965

Conversation

@izumi0uu

@izumi0uu izumi0uu commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents the Desktop dashboard cron ticker from executing profile cron jobs when a same-profile gateway already owns scheduler execution.

The fix now uses the gateway runtime lock as the scheduler-owner signal. Desktop backends still keep a fallback ticker for profiles with no live gateway, but each desktop tick explicitly opts into scheduler-level deferral whenever the gateway owner is active.

Related Issue

Fixes #43965

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • Added _gateway_scheduler_owner_active() in cron/scheduler.py to delegate scheduler-ownership checks to gateway.status.is_gateway_runtime_lock_active().
  • Extended cron.scheduler.tick(...) with defer_to_gateway_owner=False so non-gateway tickers can opt into owner-aware deferral without changing gateway behavior.
  • Updated hermes_cli/web_server.py so the desktop dashboard ticker calls cron_tick(..., defer_to_gateway_owner=True) instead of duplicating a separate gateway-liveness check.
  • Added tests/cron/test_scheduler_ownership.py to cover owner detection, fail-open behavior, early deferral before lock acquisition, and the opt-in boundary.
  • Updated tests/hermes_cli/test_web_server.py to assert that the desktop ticker requests gateway-owner deferral on every tick.

How to Test

  1. Run ./.venv/bin/pytest -q tests/cron/test_scheduler_ownership.py.
  2. Run ./.venv/bin/pytest -q tests/hermes_cli/test_web_server.py -k 'TestDesktopCronTicker'.
  3. Run ./.venv/bin/pytest -q tests/gateway/test_status.py -k 'runtime_lock or get_running_pid'.
  4. Optionally run ./.venv/bin/pytest -q tests/cron/test_parallel_pool.py tests/cron/test_scheduler_ownership.py to confirm the new tick parameter does not disturb existing scheduler dispatch behavior.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.5.1 via focused pytest coverage in the upstream worktree

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@izumi0uu
izumi0uu marked this pull request as ready for review June 11, 2026 06:21
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists labels Jun 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #44049 — both fix #43965 with the same approach (_gateway_scheduler_owner_active() + defer_to_gateway_owner param in cron/scheduler.py, desktop ticker opt-in in web_server.py). #44049 is the earlier of the pair.

@alt-glitch alt-glitch added the comp/dashboard Web dashboard / control panel UI (dashboard/, landing) label Jun 26, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the scheduler-ownership race; the underlying defect is present on current main.

Problems

  • The PR edits the old direct cron_tick(...) desktop call (hermes_cli/web_server.py:126 on PR head). Current main instead resolves and starts a provider at hermes_cli/web_server.py:150-154; InProcessCronScheduler.start() calls tick() without an owner-defer option at cron/scheduler_provider.py:166-180. As written, this patch would not govern the active desktop ticker.
  • Current provider characterization coverage only asserts desktop sync=False behavior (tests/cron/test_scheduler_provider.py:74-101); it does not prove owner-aware deferral through the provider boundary.

Suggested changes

  • Thread the narrow owner-deferral option through the current provider start contract, pass it from the desktop ticker, and retain the gateway default path at gateway/run.py:20839-20846.
  • Add provider-path tests for gateway-owner deferral, no-owner execution, and the gateway's non-deferring behavior.

Automated hermes-sweeper review.

Comment thread hermes_cli/web_server.py Outdated
@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Desktop dashboard backends must yield cron execution to a live same-profile gateway so TCC/FDA-sensitive jobs retain gateway process ancestry. The cross-process tick lock prevents duplicate firing but cannot choose the correct execution owner.

Thread the opt-in through InProcessCronScheduler while preserving the gateway default and keeping built-in-only policy out of external provider contracts. Cover owner deferral, no-owner execution, gateway non-deferral, and the desktop provider boundary.

Fixes NousResearch#43965
@izumi0uu
izumi0uu force-pushed the fix/cron-owner-desktop-ticker-43965 branch from f25a82e to 02a34bf Compare July 22, 2026 14:50
@izumi0uu

Copy link
Copy Markdown
Contributor Author

Addressed in 02a34bf. The desktop cron ownership guard now follows the active scheduler-provider path instead of the removed direct cron_tick(...) call. InProcessCronScheduler forwards the narrow owner-deferral option, the desktop opts in only for the built-in provider, and the gateway retains its non-deferring default without changing external provider contracts. I added provider-path regressions for gateway-owner deferral, no-owner execution, gateway behavior, and external providers.

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management comp/dashboard Web dashboard / control panel UI (dashboard/, landing) duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop dashboard cron ticker can execute jobs instead of launchd gateway, breaking macOS TCC/FDA provenance

3 participants