fix(cron): reset cron-delivery ContextVars to _UNSET after a job - #82362
Open
aldoeliacim wants to merge 1 commit into
Open
fix(cron): reset cron-delivery ContextVars to _UNSET after a job#82362aldoeliacim wants to merge 1 commit into
aldoeliacim wants to merge 1 commit into
Conversation
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: NousResearch#43370 (fixes the sibling HERMES_CRON_SESSION leak via the token
mechanism; this covers the auto-delivery vars it does not touch), NousResearch#8866.
alt-glitch
added
type/bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
run_job()sets theHERMES_CRON_AUTO_DELIVER_*ContextVars while a job runs so the job's ownsend_messagecan detect a duplicate auto-delivery target. On the way out it "cleared" them by setting""— but an empty string is the explicitly cleared state that suppresses theos.environfallback inget_session_env. Correct WHILE the job runs; wrong after it: the empty value leaks into every later caller in the same thread/task (cross-run / cross-test pollution), so a subsequent consumer that should have fallen back toos.environsees "explicitly no target" instead.Fix:
reset_cron_delivery_vars()ingateway/session_context.pyreturns the vars to the_UNSETsentinel (never-set state), andrun_job()calls it in the job-teardown path instead of the""loop.Why
"", suppress fallback) and "never set" (_UNSET, allow fallback) is the documented contract ofget_session_env— teardown was landing on the wrong side of it.How to test
11 tests pass, including the new one: after
reset_cron_delivery_vars(),get_session_envfalls back toos.environagain (fails against the previous""teardown).Tested on Linux (aarch64).
Supersedes auto-closed #47089 (head fork deleted in a remote swap; GitHub can't reopen a PR whose head repo is gone). Rebased onto current main — upstream's
run_jobteardown gained cwd-lock and non-dispatcher token handling since; this change composes with both.