fix(cron): run manual jobs without blocking the caller - #53395
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Good fix for cron manual jobs blocking the caller. The new _submit_job_with_running_guard function correctly checks for duplicate running jobs and releases the running set in a finally block. The contextvars.copy_context() pattern is correct for thread pool isolation.
Looks Good
- Clean separation of guard logic into its own function
- Running-set membership always released via finally block
- Context isolation with copy_context is appropriate
- No security concerns
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the caller-blocking manual-run path; current main still invokes run_one_job() inline from tools/cronjob_tools.py:622-641, so the underlying issue remains valid.
Problems
- The proposed extracted submission helper does not carry forward current main's shutdown protections in
cron/scheduler.py:3663-3702: preflight detection of interpreter finalization plus cleanup whenpool.submit()races shutdown. Those protections were added in8aab8be50. - The proposed claim-loss branch is older than the current behavior at
tools/cronjob_tools.py:625-637, which distinguishes paused/disabled/missing jobs from a held claim (7ecc822e1). Preserve that distinction during salvage.
Suggested changes
- Extract the current guarded submission logic, including both shutdown paths, into the reusable helper.
- Preserve the current claim-loss reason selection and add regression coverage for it after changing the dispatch model.
This is an automated hermes-sweeper review.
Extract current main's guarded scheduler submission path so manual runs reuse workdir-aware pools, running-job dedupe, execution-ledger ownership, ContextVar isolation, and both interpreter-shutdown protections. Preserve paused, disabled, missing, and held-claim result reasons while returning immediately after successful background dispatch. Tested: scripts/run_tests.sh -j 4 tests/cron tests/tools/test_cronjob_run_immediate.py tests/hermes_cli/test_cron.py (756 passed) Refs: NousResearch#52705
5b09dfa to
dbed6e1
Compare
|
Addressed in dbed6e1. Extracted current main’s complete guarded scheduler submission path into a reusable module-level helper and routed both ticker and manual dispatch through it, preserving the interpreter-shutdown preflight, submit-race cleanup, running-job dedupe, execution-ledger ownership, ContextVar isolation, and workdir-aware pool selection. Manual cronjob(action='run') now claims and queues the job immediately, returning execution_pending=true without waiting for run_one_job() to finish. I also preserved the distinct paused/disabled, missing, and genuinely held-claim responses, and added regressions covering both shutdown paths, claim-loss reason propagation, and the real non-blocking execution flow. |
Salvaged from PR #53395 by @izumi0uu: the fire claim's 300s TTL is routinely outlived by real cron jobs, so claim_job_for_fire alone cannot stop a manual cronjob(action='run') from double-firing a job the ticker (or another manual run) is still executing. Extract the ticker's _submit_with_guard running-set check into shared module-level helpers (try_register_running_job / release_running_job) and register manual runs through the same set — one dedupe owner, no drift. Manual runs also become visible to get_running_job_ids (the gateway shutdown drain, #60432) and mark_running_jobs_interrupted, which previously could not see them. The background dispatch path pre-checks the running set so a mid-run job reports 'already running' in the tool response immediately instead of as a delayed error completion event; the authoritative atomic check remains in _run_claimed_job on the worker. Co-authored-by: izumi0uu <izumi0uu@gmail.com>
Salvaged from PR #53395 by @izumi0uu: the fire claim's 300s TTL is routinely outlived by real cron jobs, so claim_job_for_fire alone cannot stop a manual cronjob(action='run') from double-firing a job the ticker (or another manual run) is still executing. Extract the ticker's _submit_with_guard running-set check into shared module-level helpers (try_register_running_job / release_running_job) and register manual runs through the same set — one dedupe owner, no drift. Manual runs also become visible to get_running_job_ids (the gateway shutdown drain, #60432) and mark_running_jobs_interrupted, which previously could not see them. The background dispatch path pre-checks the running set so a mid-run job reports 'already running' in the tool response immediately instead of as a delayed error completion event; the authoritative atomic check remains in _run_claimed_job on the worker. Co-authored-by: izumi0uu <izumi0uu@gmail.com>
|
Thanks @izumi0uu — the strongest idea in this PR outlived the PR itself: your observation that manual runs and the ticker need ONE shared in-flight dedupe owner (the fire claim's 300s TTL can't prevent double-fires on long jobs) was salvaged into #80807, now on main. The ticker's The non-blocking half went a different way than the fire-and-forget submit here: #80807 routes manual runs through the async-delegation rail so the outcome re-enters the conversation as a completion event rather than requiring status polling. Closing with thanks — the dedupe guard is your contribution in the tree. |
Salvaged from PR NousResearch#53395 by @izumi0uu: the fire claim's 300s TTL is routinely outlived by real cron jobs, so claim_job_for_fire alone cannot stop a manual cronjob(action='run') from double-firing a job the ticker (or another manual run) is still executing. Extract the ticker's _submit_with_guard running-set check into shared module-level helpers (try_register_running_job / release_running_job) and register manual runs through the same set — one dedupe owner, no drift. Manual runs also become visible to get_running_job_ids (the gateway shutdown drain, NousResearch#60432) and mark_running_jobs_interrupted, which previously could not see them. The background dispatch path pre-checks the running set so a mid-run job reports 'already running' in the tool response immediately instead of as a delayed error completion event; the authoritative atomic check remains in _run_claimed_job on the worker. Co-authored-by: izumi0uu <izumi0uu@gmail.com>
Salvaged from PR NousResearch#53395 by @izumi0uu: the fire claim's 300s TTL is routinely outlived by real cron jobs, so claim_job_for_fire alone cannot stop a manual cronjob(action='run') from double-firing a job the ticker (or another manual run) is still executing. Extract the ticker's _submit_with_guard running-set check into shared module-level helpers (try_register_running_job / release_running_job) and register manual runs through the same set — one dedupe owner, no drift. Manual runs also become visible to get_running_job_ids (the gateway shutdown drain, NousResearch#60432) and mark_running_jobs_interrupted, which previously could not see them. The background dispatch path pre-checks the running set so a mid-run job reports 'already running' in the tool response immediately instead of as a delayed error completion event; the authoritative atomic check remains in _run_claimed_job on the worker. Co-authored-by: izumi0uu <izumi0uu@gmail.com>
What does this PR do?
Fixes the synchronous
cronjob(action='run')path so a manual cron run starts immediately without blocking the caller until the job finishes.Before this change, the model tool claimed the job and then called
run_one_job()inline. That kept the current agent/tool turn open for the full runtime of the cron task. This patch reuses the scheduler's persistent pools for one-off manual dispatch, so manual runs still execute right away but now return after the job is queued.Related Issue
Fixes #52705
Type of Change
Changes Made
cron.scheduler.dispatch_job()as the single-job, non-blocking counterpart totick(sync=False), reusing the existing running-job guard and workdir-aware pool selection.tools.cronjob_tools._execute_job_now()to claim the job and dispatch it through the scheduler pool instead of callingrun_one_job()inline./cron runmessaging to describe immediate background execution instead of “next scheduler tick”.How to Test
/Users/idah/.hermes/hermes-agent-2/.venv/bin/pytest -q tests/tools/test_cronjob_run_immediate.py/Users/idah/.hermes/hermes-agent-2/.venv/bin/pytest -q tests/hermes_cli/test_cron.py/Users/idah/.hermes/hermes-agent-2/.venv/bin/pytest -q tests/cron/test_parallel_pool.pyChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A