Skip to content

fix(cron): enforce a single scheduler owner - #66162

Closed
opita04 wants to merge 18 commits into
NousResearch:mainfrom
opita04:fix/cron-single-owner-secret-refresh-20260717
Closed

fix(cron): enforce a single scheduler owner#66162
opita04 wants to merge 18 commits into
NousResearch:mainfrom
opita04:fix/cron-single-owner-secret-refresh-20260717

Conversation

@opita04

@opita04 opita04 commented Jul 17, 2026

Copy link
Copy Markdown

Summary

  • enforce one scheduler owner per canonical HERMES_HOME with a full-lifetime kernel lease and dynamic gateway/Desktop handoff
  • fence callbacks to the active generation and retain ownership until providers, jobs, callback reservations, and actual worker threads drain
  • refresh external secrets atomically before relevant no_agent subprocesses and capture an immutable sanitized child environment
  • strictly parse, expand, and deep-merge user/managed secret configuration; fail closed on malformed, unresolved, duplicate, or failed refresh state
  • keep external scheduling providers gateway-only while preserving automatic Desktop-only scheduling
  • sanitize lifecycle failures so backend or secret-bearing exception text cannot escape
  • pin dashboard and gateway fires to the reserved canonical home and synchronously start authoritative worker threads before returning 202

Problem

Gateway and Desktop could both dispatch one shared cron registry. The file lock serialized individual ticks but allowed either process to win, so jobs alternated between processes with different managed-secret bootstrap contexts. no_agent execution also constructed subprocess environments before managed-secret refresh.

Safety properties

  • gateway credentials remain isolated from Desktop
  • ownership handoff never unlinks the kernel lock file
  • stale callback generations fail closed
  • cancellation cannot release a reservation while synchronous work is still running
  • environment precedence remains dotenv → external sources → managed .env
  • configured bootstrap aliases and active OP_SESSION_* credentials are stripped from children
  • raw provider, reservation, thread/task-start, backend, and secret-source exception text is not returned or logged

Validation

Exact reviewed head: eda3bc24502cbdf6a106cd824607fb91cf0d8a53

  • 843 passed in the final affected upstream matrix
  • independent final specification review: PASS
  • independent adversarial security/concurrency review: APPROVED
  • additional reviewer matrices: 815, 232, 120, 112, 91, and 65 tests passed
  • Ruff, Python compilation, and git diff --check passed
  • exact worktree clean

No secret values are included in this change.

Gate gateway and Desktop cron startup through one fail-closed owner policy and refresh configured external secrets before script-only jobs spawn.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/cli CLI entry point, hermes_cli/, setup wizard comp/dashboard Web dashboard / control panel UI (dashboard/, landing) area/config Config system, migrations, profiles needs-decision Awaiting maintainer decision before any implementation 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 labels Jul 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this implements a configuration-enforced single-owner policy for the desktop/gateway cron scheduler cluster (#43965, #44049, #52259). Its policy is broader than the existing live-gateway deferral approaches; maintainers should choose the ownership semantics.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment

Enforces a single scheduler owner for cron jobs to prevent duplicate job dispatch. Adds scheduler_owner config key and thread-safe secret refresh. 10 files, 27K diff. The pattern is sound and uses proper locking. Recommend human review for the full scope of the config/loader changes.


Reviewed by Hermes Agent

@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 the substantial concurrency and secret-handling work. One blocking cross-process regression needs resolution.

Problems

  • cron/scheduler.py:3876-3880 now calls on_jobs_changed() only through reserved_active_scheduler_provider(). That registry is process-local (cron/scheduler_runtime.py:21-33), so a separate hermes cron / tool process has no active entry and silently skips reconciliation. This conflicts with the external-provider contract in cron/scheduler_provider.py:78-82, which requires create/update/remove/pause/resume to notify the provider.
  • tests/cron/test_jobs_changed_notify.py:38-49 mocks a local reservation, so it does not cover that cross-process owner case.

Suggested changes

  • Route mutations to the active owner through a durable or authenticated cross-process mechanism, then add an integration test for an independent mutation process while an external owner is active.

Automated hermes-sweeper review.

Comment thread cron/scheduler.py
resolve_cron_scheduler().on_jobs_changed()
from cron.scheduler_runtime import reserved_active_scheduler_provider

with reserved_active_scheduler_provider() as reservation:

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.

reserved_active_scheduler_provider() is process-local. A CLI/tool process that mutates jobs while another process owns an external scheduler will receive None here, so on_jobs_changed() never runs and the external registry is not reconciled. Please retain a cross-process notification path to the owner and cover that separate-process case.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 18, 2026
@opita04

opita04 commented Jul 23, 2026

Copy link
Copy Markdown
Author

Superseded by #70286, which is rebased on current main and contains only the scheduler-ownership/lifecycle fix. The older secret-refresh and cron-isolation work is intentionally not carried forward.

@opita04 opita04 closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles 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) comp/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists 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 sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants