Skip to content

fix(cron): reset cron-delivery ContextVars to _UNSET after a job - #47089

Closed
aldoeliacim wants to merge 1 commit into
NousResearch:mainfrom
aldoeliacim:fix/cron-auto-deliver-contextvar-leak
Closed

fix(cron): reset cron-delivery ContextVars to _UNSET after a job#47089
aldoeliacim wants to merge 1 commit into
NousResearch:mainfrom
aldoeliacim:fix/cron-auto-deliver-contextvar-leak

Conversation

@aldoeliacim

@aldoeliacim aldoeliacim commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

What & why

run_job() sets the HERMES_CRON_AUTO_DELIVER_* ContextVars while a job runs
so the job's own send_message can detect a duplicate auto-delivery target.
The cleanup in the finally block reset them with var.set("").

The problem: in gateway/session_context.py, "" is the explicitly-cleared
state that intentionally suppresses the os.environ fallback in
get_session_env — it is not the same as the _UNSET "never set" sentinel.
So once a cron job finishes, the three auto-delivery vars stay at "" for the
lifetime of that thread/task, and every later caller in the same process reads
an empty cron-delivery value instead of falling back to os.environ.

Consequences:

  • Production: in a long-lived scheduler process, a finished cron job can
    shadow a later same-thread caller's auto-delivery environment.
  • Tests: it manifests as cross-file pollution — after
    tests/cron/test_scheduler.py runs, the duplicate-target detection in
    tests/tools/test_send_message_tool.py::test_cron_duplicate_target_is_skipped_and_explained
    fails with KeyError: 'skipped' because it reads the leaked empty ContextVar
    instead of the env it set. The test passes in isolation, which is the
    classic ordering-dependent symptom.

The fix

  • Add session_context.reset_cron_delivery_vars(), which restores the three
    HERMES_CRON_AUTO_DELIVER_* vars to the _UNSET sentinel (re-enabling the
    os.environ fallback for later callers).
  • Call it in run_job()'s finally cleanup instead of the set("") loop.

The per-job set("") at job start is deliberately unchanged: each job
should begin from a clean explicitly-empty state before its delivery target is
resolved, so a job that resolves no target can't inherit the previous job's.
Only the post-job cleanup is corrected.

How to test

# The fix in isolation:
pytest tests/gateway/test_session_env.py -q

# The ordering-dependent failure this resolves (red before, green after):
pytest tests/cron/test_scheduler.py \
       tests/tools/test_send_message_tool.py::TestSendMessageTool::test_cron_duplicate_target_is_skipped_and_explained -q

New regression tests in tests/gateway/test_session_env.py assert that
reset_cron_delivery_vars() returns the vars to _UNSET and that
get_session_env's os.environ fallback works again after a reset.

Local run: tests/cron/test_scheduler.py (136) + tests/gateway/test_session_env.py
(15, incl. 2 new) + the previously order-dependent send_message test all green
(152 passed together).

Platforms tested

Linux (Python 3.11). The change is pure ContextVar lifecycle management with no
OS-specific behavior.

Related


Rebased onto current main (2026-06-26). Conflict in tests/gateway/test_session_env.py: main added new executor tests (..._survives_default_executor_shutdown, ..._refuses_resurrection_after_shutdown) in the same region as this PR's reset_cron_delivery_vars tests; resolved by keeping both blocks. cron/scheduler.py + gateway/session_context.py auto-merged clean. Verified: session_env + cron suites = 594 passed.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists labels Jun 16, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: #43370 (cron session-context isolation for the approval-mode/HERMES_CRON_SESSION vars) and #8866 (cron delivery-target reliability). Same _UNSET-vs-explicit-empty ContextVar bug class as the merged #17747 defensive clearing, applied to the auto-delivery path. Not a duplicate -- different vars, complementary fix.

@aldoeliacim
aldoeliacim force-pushed the fix/cron-auto-deliver-contextvar-leak branch 2 times, most recently from 9c76a8a to 8f91e1d Compare June 29, 2026 10:34
Copilot AI review requested due to automatic review settings July 1, 2026 20:58
@aldoeliacim
aldoeliacim force-pushed the fix/cron-auto-deliver-contextvar-leak branch from 8f91e1d to f1d2ae0 Compare July 1, 2026 20:58

Copilot AI 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.

Pull request overview

This PR fixes a cron scheduler ContextVar lifecycle bug where post-job cleanup reset the cron auto-delivery ContextVars to "" (explicitly-cleared), unintentionally suppressing get_session_env’s os.environ fallback for later callers running in the same thread/task. The change restores the correct “never set” sentinel (_UNSET) after each job, preventing cross-run and cross-test pollution.

Changes:

  • Add reset_cron_delivery_vars() in gateway/session_context.py to restore HERMES_CRON_AUTO_DELIVER_* ContextVars to _UNSET.
  • Update cron/scheduler.py::run_job() to call reset_cron_delivery_vars() in finally instead of setting those ContextVars to "".
  • Add regression tests in tests/gateway/test_session_env.py verifying _UNSET restoration and os.environ fallback behavior after reset.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
gateway/session_context.py Adds a dedicated reset helper to restore cron auto-delivery ContextVars to _UNSET (re-enabling env fallback).
cron/scheduler.py Switches post-job cleanup to use the new reset helper to avoid leaking "" into later callers.
tests/gateway/test_session_env.py Adds regression coverage for the sentinel vs empty-string behavior and env fallback restoration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cron/scheduler.py
Comment on lines 2446 to 2449
# Use ContextVars for per-job session/delivery state so parallel jobs
# don't clobber each other's targets (os.environ is process-global).
from gateway.session_context import set_session_vars, clear_session_vars, _VAR_MAP
from gateway.session_context import set_session_vars, clear_session_vars, reset_cron_delivery_vars, _VAR_MAP

Comment on lines +411 to +415
_CRON_DELIVERY_VARS = (
"HERMES_CRON_AUTO_DELIVER_PLATFORM",
"HERMES_CRON_AUTO_DELIVER_CHAT_ID",
"HERMES_CRON_AUTO_DELIVER_THREAD_ID",
)
@aldoeliacim
aldoeliacim force-pushed the fix/cron-auto-deliver-contextvar-leak branch 2 times, most recently from b771187 to 42e9530 Compare July 4, 2026 19:58
run_job() sets the HERMES_CRON_AUTO_DELIVER_* ContextVars while a job runs so
the job's own send_message can detect a duplicate auto-delivery target. The
cleanup in the finally block set them back to "" — but "" is the
"explicitly cleared" state that suppresses the os.environ fallback in
get_session_env, NOT the "never set" (_UNSET) sentinel. So after any cron run,
every later caller in the same thread/task reads an empty cron-delivery value
instead of falling back to os.environ.

In a long-lived scheduler process this lets a finished cron job shadow a later
same-thread caller's auto-delivery env. It also surfaces as cross-test
pollution: after tests/cron/test_scheduler.py runs, the duplicate-target
detection in tests/tools/test_send_message_tool.py reads the leaked empty
ContextVar instead of the env the test set.

Fix: add session_context.reset_cron_delivery_vars(), which restores the three
cron-delivery vars to _UNSET, and call it in run_job's finally cleanup instead
of set(""). The per-job set("") at job START is unchanged (correct: each job
starts from a clean explicitly-empty state before its target is resolved).

Adds regression tests in tests/gateway/test_session_env.py asserting the vars
return to _UNSET and that get_session_env's os.environ fallback works again
after a reset.

Related: #43370 (fixes the sibling HERMES_CRON_SESSION leak via the token
mechanism; this covers the auto-delivery vars it does not touch), #8866.
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 comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants