fix(cron): wire confirmed-dead delivery targets into the live-adapter send path - #64915
Open
pierrenode wants to merge 1 commit into
Open
fix(cron): wire confirmed-dead delivery targets into the live-adapter send path#64915pierrenode wants to merge 1 commit into
pierrenode wants to merge 1 commit into
Conversation
Contributor
|
Thanks for tracing the live-adapter bypass. The premise is confirmed on current main: cron calls Problems
Suggested changes
The changed production file is byte-identical between this PR's base and current |
pierrenode
force-pushed
the
fix/cron-delivery-dead-target-wiring
branch
from
July 27, 2026 00:36
f7d4b55 to
0119333
Compare
… send path gateway/delivery.py's DeliveryRouter.deliver() checks DeadTargetRegistry before every send and marks/clears it around success or failure — but cron's live-adapter path calls the private _deliver_to_platform() directly, bypassing deliver() entirely (it needs to skip deliver()'s private-chat reply-anchor requirement, which cron sends have no inbound anchor to satisfy). The only caller of the public deliver() in the whole codebase is the test suite, so a target confirmed dead (deleted group, blocked/kicked bot, deactivated user) was retried on every single cron tick, on every platform, forever — the exact waste dead_targets.py exists to prevent. Share one DeadTargetRegistry instance across a delivery pass: - Skip a target already marked dead before attempting any send (live or standalone), both at the top of the per-target loop and again right before the standalone fallback so a target that just got marked dead this same tick isn't sent to twice. - On a real send failure from _deliver_to_platform(), classify it via gateway/delivery.py's existing _classify_dead_from_error_text() and mark the target dead when it's a whole-chat death. - On a successful (or assumed-delivered) live send, clear any stale dead flag — self-healing, mirroring deliver()'s own behavior. Extends the same classification to the live-media send path: _send_media_via_adapter() previously swallowed every per-file failure internally (just a logger.warning), giving the caller no way to know a send failed. It now returns the list of failure messages, and the caller feeds each through the same _classify_dead_from_error_text() used for text sends. This matters specifically for media-only jobs (no text_to_send at all): adapter_ok stays at its vacuous default of True in that case — nothing ever flips it False — so a media-only send against a confirmed-dead target both never got marked dead AND had any stale dead flag immediately self-healed away by the very next line. The self-healing clear() is now guarded on is_dead() so it doesn't erase a mark the media-failure branch just set earlier in the same pass. Adds direct test coverage for both the media-only forbidden-send-marks- dead case and the transient-media-error-does-not-mark-dead negative case, mirroring the existing text-send test pair.
pierrenode
force-pushed
the
fix/cron-delivery-dead-target-wiring
branch
from
July 29, 2026 14:43
0119333 to
3301344
Compare
pierrenode
added a commit
to pierrenode/hermes-agent
that referenced
this pull request
Aug 13, 2026
classify_send_error() didn't recognize Photon's target_not_allowed error text (shared/free-tier lines permanently rejecting a new outbound thread), so DeliveryRouter.deliver() never called mark_dead() for it and every delivery attempt kept re-sending to a target the sidecar had already rejected — wasting a send against flood control on each try. Add the 'target_not_allowed' substring to the existing 'forbidden' bucket (already documented as covering 'lacks permission to post to the target'). No new error kind, no change to DeadTargetRegistry._DEAD_ERROR_KINDS — mirrors the existing pattern for other permanently-rejected targets. The scheduler-level wiring this PR originally also carried (cron's _deliver_result() calling DeliveryRouter._deliver_to_platform() directly, bypassing deliver()'s dead-target check) is intentionally left to NousResearch#64915, which already lands a broader fix for that same call site (including the media-only-job case this PR's version didn't cover) — landing both would conflict on the same lines with divergent implementations.
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.
Summary
gateway/delivery.py::DeliveryRouter.deliver()is the only place that consultsgateway/dead_targets.py'sDeadTargetRegistry: it skips a target already confirmed dead (deleted group, blocked/kicked bot, deactivated user), and marks/clears the flag around a send's failure/success.cron/scheduler.py's live-adapter delivery path does not calldeliver(). It deliberately calls the privateDeliveryRouter._deliver_to_platform()directly instead, becausedeliver()'s private-chat topic detection demands a reply anchor (thread_id/message_thread_idin metadata) that cron sends — which have no inbound message to reply to — can't supply; routing through_deliver_to_platform()withthread_idpassed via the target/metadata bypasses that check (see the existing comment at the call site,#22773/#52060).Checked every call site in the codebase:
deliver()'s only caller anywhere is the test suite.gateway/run.pyconstructsself.delivery_routerand keeps.adapterssynced but never calls.deliver()either. The practical effect: a cron job's target that has been definitively dead for weeks is retried — full send attempt, full failure, full log line — on every single tick, on every platform, forever. This is exactly the wastedead_targets.py's own docstring says it exists to prevent ("re-sending to it on every cron tick... wastes a send attempt against the platform's flood-control envelope and spams the logs").Fix
cron/scheduler.py::_deliver_result()now shares oneDeadTargetRegistryinstance across a delivery pass:DeliveryRouter(config, adapters, dead_targets=...)so the live-adapter path's failure handling and my checks stay consistent within one run._deliver_to_platform()raises a real send error, classify it via the existinggateway/delivery.py::_classify_dead_from_error_text()(the same classifierdeliver()itself uses) and mark the target dead if it's a whole-chat death.deliver()'s own post-success behavior.The standalone (non-live-adapter) send path itself (
tools/send_message_tool.py::_send_to_platform) is intentionally left untouched — extending dead-target awareness there is a separate, independent surface I haven't audited to the same depth, and is out of scope for this PR. The pre-loop check (item 1) already protects standalone-only ticks (gateway not live) from repeat waste once a target is marked dead via any live-adapter tick.Test plan
test_already_dead_target_is_skipped_without_calling_adapter: pre-marks a target dead, assertsadapter.sendis never called and the standalone fallback is never called either.test_forbidden_send_error_marks_target_dead: adapter returns a "Forbidden: bot was blocked by the user" failure; asserts a freshDeadTargetRegistry()instance sees it marked dead afterward (proves persistence, not just an in-memory object the test happens to hold), and that the standalone fallback is skipped on the same tick.test_transient_send_error_does_not_mark_target_dead: a generic "Connection reset by peer" failure must NOT be classified as dead, and the standalone fallback must still run — guards against over-broad classification.test_successful_send_clears_dead_flag: a successful send callsDeadTargetRegistry.clear()with the right platform/chat_id (spied viapatch.object(..., autospec=True)).cron/scheduler.pyand confirmed 3 of the 4 new tests fail against pre-fix code (the 4th — the transient-error precision guard — correctly passes either way, since "not marking dead" is also true with zero dead-target logic at all; it only has teeth combined with the other three).tests/cron/test_scheduler.py(220 tests),tests/gateway/test_dead_targets.py,tests/gateway/test_delivery.py,tests/gateway/test_delivery_silence_filter.py, andtests/cron/test_jobs.py— 443 tests total — all pass.ruff checkclean.gateway/dead_targets.py's retry-window logic itself, nevercron/scheduler.pyorgateway/delivery.py— doesn't address this wiring gap. feat(cron): redirect stale-target deliveries to parent/home channel #54598 ("redirect stale-target deliveries to parent/home channel") does touchcron/scheduler.py's standalone-fallback region, but implements a completely independent mechanism (its ownis_definitive_delivery_failure()classifier redirecting to a fallback channel) with no reference toDeadTargetRegistry— diffed line-by-line, no functional overlap with this change.