fix(cron): clean up HERMES_CRON_SESSION env var after each job run (#56771) - #57124
fix(cron): clean up HERMES_CRON_SESSION env var after each job run (#56771)#57124AlexFucuson9 wants to merge 1 commit into
Conversation
HERMES_CRON_SESSION was set via os.environ (process-global) in run_job()
but never removed in the finally block. When the cron scheduler runs
in-process via InProcessCronScheduler (same process as the gateway),
this sentinel leaked into interactive gateway sessions, causing
execute_code and terminal commands to be blocked with the cron-deny
approval error.
Add os.environ.pop('HERMES_CRON_SESSION', None) to the finally block
alongside the existing TERMINAL_CWD cleanup.
Fixes NousResearch#56771
Competing fix cluster for #56771 (all open, all different sites/mechanisms): this PR (#57124) cleans up the leaked |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the source of the leak. The current main implementation still has the process-global marker at cron/scheduler.py:2812, so the underlying report is valid.
Problems
- The added cleanup is unsafe with concurrent jobs.
cron/scheduler.py:3741-3788dispatches workdir-less jobs in parallel; when one job reaches thispop, another active cron job can lose its cron classification. The approval gates attools/approval.py:2173,2700, and3121would then no longer applyapprovals.cron_modeto that remaining job. - Cleanup only after
run_job()returns still leaves an interactive gateway request misclassified while any cron job is active: the process-global flag is set atcron/scheduler.py:2812, and_is_gateway_approval_context()prioritizes it attools/approval.py:241-245. - This diff adds no regression test for the cross-thread gateway/cron case.
Suggested changes
- Scope the cron marker per job (for example with the existing ContextVar session-state approach) and have the approval gates read that scoped value.
- Add a concurrent-thread regression test showing gateway approval remains interactive while real cron work still honors
approvals.cron_mode.
Automated hermes-sweeper review.
| os.environ.pop("TERMINAL_CWD", None) | ||
| else: | ||
| os.environ["TERMINAL_CWD"] = _prior_terminal_cwd | ||
| # Remove the process-wide cron sentinel so it does not leak into |
There was a problem hiding this comment.
This process-global cleanup is unsafe when due jobs run concurrently: a completed job can clear the marker while another cron job is still active, causing its approval checks to stop enforcing approvals.cron_mode. The cron marker needs per-job scoped state rather than a shared env-var lifetime.
Problem
When the cron scheduler runs in-process via
InProcessCronScheduler(same Python process as the gateway),run_job()setsos.environ["HERMES_CRON_SESSION"] = "1"but never removes it in thefinallyblock. Sinceos.environis process-global and shared across all threads, this sentinel leaks into interactive gateway sessions.Impact: After any cron job runs,
execute_codeand terminal commands in Telegram/gateway interactive sessions are blocked with the cron-deny approval error until the gateway process is restarted.Root Cause
cron/scheduler.py:2444— setsos.environ["HERMES_CRON_SESSION"] = "1"cron/scheduler.py:3025+—finallyblock cleans upTERMINAL_CWD, ContextVars, session DB, agent resources — but notHERMES_CRON_SESSIONtools/approval.py:2655—check_code_execution_approval()readsHERMES_CRON_SESSIONfromos.getenv()and blocks executionThe codebase already uses ContextVars for per-job session state to avoid exactly this class of cross-session pollution, but
HERMES_CRON_SESSIONwas left as a rawos.environwrite.Fix
Add
os.environ.pop("HERMES_CRON_SESSION", None)to thefinallyblock inrun_job(), alongside the existingTERMINAL_CWDcleanup. This ensures the env var is only present while a cron job is actively running.Fixes #56771