Repository navigation
fix(cron): bind the profile secret scope for the dispatch-failure notice - #130431
Closed
skyeyesec333 wants to merge 1 commit into
Closed
skyeyesec333 wants to merge 1 commit into
skyeyesec333 wants to merge 1 commit into
Conversation
run_one_job's pre-handoff dispatch-failure branch (NousResearch#124013) calls _deliver_crash_failure before _run_one_job_body, which is where the firing profile's secret scope is installed for delivery. Under a multiplexed gateway the notice then resolves the home channel through get_secret with no scope, raises UnscopedSecretError, and never leaves. Wrap the notice in _install_fire_secret_scope/_reset_fire_secret_scope, the pair _run_one_job_body and record_unknown_worker_outcome already use.
12 of 13 tasks
kshitijk4poor
added a commit
that referenced
this pull request
Oct 3, 2026
kshitijk4poor
added a commit
that referenced
this pull request
Oct 3, 2026
…h-notice exit _run_one_job_body's except branch sends _deliver_crash_failure when anything in the body raises, but the firing profile's secret scope was only installed after claim_dispatch and mark_execution_running. A raise from either sent the crash notice with no scope: under multiplex get_secret fails closed (UnscopedSecretError, "Hermes could not read this profile's TELEGRAM_HOME_CHANNEL") and the notice never left. Same class as the dispatch-failure branch fixed for #131585 (and called out in #130431's body). Install the scope first thing in the try so every exit through the except is scoped; the outer finally already resets it. The handoff path stays outside it (#107692), and the external-worker payload path and the post-handoff notice already run scoped. Extends the #130431 test with a claim_dispatch-raising case. Refs #131585 Co-authored-by: skyeyesec333 <skyeyesec333@users.noreply.github.com>
|
Thanks @skyeyesec333 — this landed in #132446 (merged, 8d15ca4) with your commit kept as authored (c8815a5). We also moved the body's scope install ahead of |
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
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.
Fixes #131585
Resubmission of #127310. It closed unmerged on 2026-09-30 when I deleted the fork it came from by mistake, and GitHub can't reopen it. Rebased onto
mainat bd32c71. Since #126982 merged, the fix uses its_install_fire_secret_scope()helper instead of building the scope by hand. Earlier review and test notes are on #127310.What does this PR do?
The failure notice for a restart-safe cron worker dispatch failure (added in #124013 for #123401) can't be delivered under a multiplexed gateway. It fails with the same plain-language error the notice is meant to replace:
Impact: this only affects gateways serving more than one profile (the default once a second profile exists), but there it undoes #124013 in the case it was added for. #123401 was a 9.5-hour worker outage with 106 failed runs and no notices; on a multiplexed gateway the same outage today still sends none for jobs that deliver to a home channel. The incident row and
last_errorare still recorded, so nothing is lost; it just isn't pushed to anyone.run_one_jobcalls_deliver_crash_failurefrom the dispatch-failure branch, which returns before_run_one_job_bodyever runs._run_one_job_bodyis where the owning profile's secret scope gets installed for delivery. So the notice runs with no scope, and_home_env_lookup/_env_home_target_chat_idread the home channel throughget_secret, which raisesUnscopedSecretErrorwhen multiplex is on and no scope is set. The incident row is still written, but the notice itself never goes out, so the outage #123401 wanted to surface is still silent for any job that delivers to a home channel.I hit this on a multiplexed gateway during the worker
ModuleNotFoundError: ruameloutage (#125820 and friends): three dispatch failures in a row on a 5-minute job, each followed by the delivery error above.The fix wraps the
_deliver_crash_failurecall in_install_fire_secret_scope()/_reset_fire_secret_scope(), the pair_run_one_job_bodyandrecord_unknown_worker_outcome(the post-handoff notice) already use. The post-handoff branch is unchanged.Related Issue
Follow-up to #123401 / #124013. #126982 (merged 09-30) added the routed-fire helpers this version now uses; it didn't touch this branch. On
main,record_unknown_worker_outcomealready wraps the post-handoff notice the same way.One narrower case this PR leaves alone:
_run_one_job_body's ownexceptpath also calls_deliver_crash_failure, and an exception raised before that function installs its scope (claim_dispatch,mark_execution_running) would hit the same unscoped read. I haven't seen that in practice, so I kept this PR to the branch that fails in production.Type of Change
Changes Made
cron/scheduler.py: inrun_one_job's pre-handoff dispatch-failure branch, wrap_deliver_crash_failurein_install_fire_secret_scope()/_reset_fire_secret_scope().tests/cron/test_restart_safe_worker.py:test_dispatch_failure_notice_resolves_the_owning_profiles_home_channelruns a failed handoff with multiplex active and the home channel only in the profile's.env, and checks the notice resolves it, the incident is recorded, the outcome isdelivered, and no scope is left installed afterwards.How to Test
scripts/run_tests.sh tests/cron/test_restart_safe_worker.pyHermes could not read this profile's TELEGRAM_HOME_CHANNELline as production; with this change the file passes (35 passed).current_secret_scope()is still set after the fire). Each control fails only the new test.I also ran all of
tests/cronplus the secret-scope tests: 163 files, 1645 passed, 3 skipped. I did not run the fullpytest tests/suite.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (targeted suites only, see above)Documentation & 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