Skip to content

feat(cron): add daemon_alive() two-level heartbeat check - #70020

Open
yubingz wants to merge 4 commits into
NousResearch:mainfrom
yubingz:feat/cron-daemon-alive-v2
Open

feat(cron): add daemon_alive() two-level heartbeat check#70020
yubingz wants to merge 4 commits into
NousResearch:mainfrom
yubingz:feat/cron-daemon-alive-v2

Conversation

@yubingz

@yubingz yubingz commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements a two-level daemon liveness check for the cron subsystem:

  1. PID check: Verify PID file exists and process is actually running
  2. Heartbeat check: Verify heartbeat file was updated within the configured interval

Returns True only if both checks pass. Falls back gracefully when either file is missing.

Also includes the complete daemon lifecycle management:

  • _daemonize(): Fork to background with proper PID file handling
  • run_ticker(): Periodic execution loop with heartbeat updates
  • stop_daemon(): Clean shutdown via SIGTERM
  • Signal handling (SIGTERM/SIGINT) + atexit cleanup

Closes #69976

Implements a two-level daemon liveness check:
1. Check PID file existence and process validity
2. Check heartbeat file freshness (configurable interval)

Also includes full daemon lifecycle: _daemonize(), run_ticker(),
stop_daemon(), signal handling, and atexit cleanup.

Closes NousResearch#69976
@alt-glitch alt-glitch added type/feature New feature or request comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #43864 and #69976. This standalone cron.daemon design calls the scheduler directly, while #43864 wires daemon mode through the CLI; these are competing contracts rather than duplicates.

@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 addressing cron liveness. The heartbeat work on current main has moved into the provider lifecycle, so this needs substantial re-scoping before it can be safely salvaged.

Problems

  • cron/daemon.py:64 catches PermissionError via OSError, deletes the PID file, and reports the daemon dead. os.kill(pid, 0) raising PermissionError indicates that the process exists but is not signalable; the linked issue explicitly calls for this case to return true.
  • cron/daemon.py:118 calls cron.scheduler.tick() directly. Current execution resolves and starts the configured scheduler provider in gateway/run.py:25491-25542; cron/scheduler_provider.py:122-159 supports external providers, while the built-in provider owns heartbeat and profile-aware lifecycle behavior.
  • The PR contains no tests for the specified PID/heartbeat cases.

Suggested changes

  • Resolve the competing standalone-daemon contract with #43864, then integrate through the selected hermes cron / provider lifecycle.
  • Preserve a live PID on PermissionError and add the listed liveness and integration tests.

Automated hermes-sweeper review.

Comment thread cron/daemon.py
# Check if process exists (signal 0)
os.kill(pid, 0) # windows-footgun: ok — POSIX process check; daemon is Unix-only
return True
except (ValueError, OSError, ProcessLookupError):

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.

PermissionError is an OSError, but for os.kill(pid, 0) it means the PID exists and is not signalable by this user. Do not unlink the PID file or return false for that case; handle it separately as a live daemon.

Comment thread cron/daemon.py
try:
while True:
try:
tick(verbose=False)

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.

This bypasses the current CronScheduler provider lifecycle. Please resolve the configured provider and use its start/stop contract so external providers, profile scoping, and the built-in heartbeat behavior are not silently skipped.

@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: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 30, 2026
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 needs-decision Awaiting maintainer decision before any implementation 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 sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(cron): 新增 cron/daemon.py,补齐 daemon_alive() 两级检测(独立 daemon + gateway in-process)

3 participants