Skip to content

fix: report immediate cron run status for depleted repeat jobs - #57689

Open
kibooncoll wants to merge 1 commit into
NousResearch:mainfrom
kibooncoll:fix/cron-repeat1-immediate-run-status
Open

fix: report immediate cron run status for depleted repeat jobs#57689
kibooncoll wants to merge 1 commit into
NousResearch:mainfrom
kibooncoll:fix/cron-repeat1-immediate-run-status

Conversation

@kibooncoll

Copy link
Copy Markdown

Summary

  • Fixes manual cronjob run status reporting for repeat-limited jobs that are removed after execution.
  • Adds an opt-in structured run_one_job(..., return_result=True) status snapshot for immediate-run callers.
  • Preserves the default bool return behavior for scheduler callers.

Root cause

_execute_job_now previously computed execution_success by re-reading last_status from the job store after run_one_job completed.

For finite repeat jobs, mark_job_run can remove the job record as soon as the repeat limit is depleted. In the repeat=1 case, a successful run could therefore lose its post-run job record before _execute_job_now re-read it, causing the tool response to report execution_success: false even though the run output artifact showed success.

Fix

  • Extend cron.scheduler.run_one_job with return_result=True to return a structured snapshot containing processed, success, status, error, and delivery_error.
  • Keep the historical default bool return when return_result is omitted.
  • Update tools.cronjob_tools._execute_job_now to use the structured snapshot when available.
  • Retain the old post-run get_job() path as a back-compat fallback.

Tests

Command run:

  • scripts/run_tests.sh tests/tools/test_cronjob_run_immediate.py tests/cron/test_run_one_job.py -v

Result:

  • 16 tests passed
  • 0 failed

Additional syntax check:

  • python -m py_compile tools/cronjob_tools.py cron/scheduler.py tests/tools/test_cronjob_run_immediate.py tests/cron/test_run_one_job.py
  • py_compile_ok

Runtime verification

Fresh patched-runtime verification covered the repeat=1 no-agent cases:

  • success job 011c67e09485 -> execution_success: true
  • fail job ad625b6487e7 -> execution_success: false
  • empty stdout job 05102215b538 -> execution_success: true

Output artifacts:

  • /home/deco/.hermes/cron/output/011c67e09485/2026-07-03_20-10-57.md
  • /home/deco/.hermes/cron/output/ad625b6487e7/2026-07-03_20-10-57.md
  • /home/deco/.hermes/cron/output/05102215b538/2026-07-03_20-10-57.md

Caveats

  • Already-running Hermes processes may need restart/module reload before patched behavior appears in the in-process cronjob tool.
  • The active session's already-loaded tool process previously showed stale false mapping, while a fresh Python process validated the patched source behavior.

Non-goals

  • No scheduler persistence redesign.
  • No repeat-limit semantic changes.
  • No output saving or delivery flow changes.
  • No recurring cron job creation.
  • No CI auto-fix or merge in this task.

cronjob run computed execution_success by re-reading last_status after run_one_job.

For repeat-limited jobs, mark_job_run can remove the job record once the repeat limit is depleted, so successful repeat=1 runs could report execution_success: false even though their output artifact showed success.

Add an opt-in structured result snapshot to run_one_job(..., return_result=True) for immediate-run callers, while preserving the default bool return for scheduler callers.

_execute_job_now now uses that snapshot when available and keeps the old job-store re-read path as a fallback.

Add regression coverage for depleted repeat=1 success, failure, and silent no_agent runs, plus scheduler tests for the structured result snapshot.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists labels Jul 3, 2026
@natehale

Copy link
Copy Markdown

Independent rediscovery of this bug — came across it during live testing of #18565 and opened #63570 before finding yours. Closed #63570 as a duplicate.

Your approach is the better fix — capturing structured status at the source is more robust than inferring from job absence (which is what our simpler fix did). Happy to help get this merged if a review is needed.

@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 tracing this to the status read after terminal-job removal. The current main still has that exact sequence: tools/cronjob_tools.py:639-647 calls run_one_job() and then derives success from get_job(), while cron/jobs.py:1524-1529 removes a finite-repeat record before that read.

Problems

  • tests/tools/test_cronjob_run_immediate.py:62 mocks both run_one_job and get_job, so the new cases do not exercise the real mark_job_run() deletion path that triggers the bug.

Suggested changes

  • Add a temp-HERMES_HOME integration regression using a real repeat=1 job and the actual run_one_job/mark_job_run chain; assert a removed successful job reports execution_success: true.

Automated hermes-sweeper review.

with patch("tools.cronjob_tools.resolve_job_ref", return_value=dict(_JOB)), \
patch("tools.cronjob_tools.claim_job_for_fire", return_value=True), \
patch("cron.scheduler.run_one_job", return_value=run_result), \
patch("tools.cronjob_tools.get_job", return_value=None):

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 mocks both run_one_job and the post-run get_job() absence, so it cannot verify the actual mark_job_run() auto-deletion path that caused the regression. Please add one temp-HERMES_HOME integration case that leaves run_one_job, mark_job_run, and get_job real.

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 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants