Skip to content

fix(gateway): classify Photon target_not_allowed as a dead target - #74145

Open
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/photon-dead-target-classification
Open

fix(gateway): classify Photon target_not_allowed as a dead target#74145
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/photon-dead-target-classification

Conversation

@pierrenode

@pierrenode pierrenode commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

PhotonAdapter._sidecar_send() preserves str(e) which includes target_not_allowed text when a shared/free-tier Photon line permanently cannot open a new outbound thread to a target. classify_send_error() didn't recognize this shape, so mark_dead() was never called and the target retried forever on every send.

classify_send_error() now recognizes target_not_allowed as a "forbidden"-class dead target — matching the existing docstring, which already describes this bucket as "lacks permission to post to the target".

Scope note (updated)

An earlier revision of this PR also carried scheduler-level wiring (cron/scheduler.py::_deliver_result() consulting/updating its own DeadTargetRegistry around the live-adapter and standalone paths, since production cron delivery calls DeliveryRouter._deliver_to_platform() directly and never went through DeliveryRouter.deliver()'s dead-target check). That wiring has been removed from this PR: it duplicates — with a narrower implementation (no media-only-job coverage, no shared registry passed into DeliveryRouter) — the fix already carried by #64915, which is intentionally the PR that owns that call site. Landing both would conflict on the same lines with divergent implementations.

This PR is back to its original, narrow scope: only the classify_send_error() classification fix.

Test plan

  • tests/gateway/test_dead_targets.py::test_photon_target_not_allowed_marks_target_deadDeliveryRouter.deliver()-level test proving the classifier recognizes the Photon error shape end-to-end (first delivery marks dead, second is skipped).
  • tests/gateway/test_send_error_classification.py — unit coverage for the new target_not_allowed"forbidden" classification.
  • Mutation-verified: reverting the gateway/platforms/base.py change breaks both of the above.
  • Ran the full neighboring suite: tests/gateway/test_dead_targets.py + tests/gateway/test_send_error_classification.py + tests/gateway/test_send_multiple_images.py (37 passed) — all green.
  • ruff check clean on all changed files.
  • Rebased onto current upstream/main, single commit.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 29, 2026

@teknium1 teknium1 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.

Thanks for tracing the Photon error classification path. The classifier gap is real: PhotonAdapter._sidecar_send() preserves str(e) (plugins/platforms/photon/adapter.py:2473-2482), and that string includes target_not_allowed (plugins/platforms/photon/adapter.py:262-282).

Problems

  • The claimed cron fix does not reach production cron delivery. cron/scheduler.py:1789-1806 calls router._deliver_to_platform(...) directly. Dead-target lookup and mark_dead() only occur in DeliveryRouter.deliver() (gateway/delivery.py:341-385), so the new branch in gateway/platforms/base.py cannot short-circuit a later cron tick.
  • The added regression test calls router.deliver() (tests/gateway/test_dead_targets.py:79-85), covering the wrapper that cron bypasses rather than the reported scheduler path.

Suggested changes

  • Route the scheduler's live and fallback delivery paths through a dead-target-aware public API, while preserving its existing topic-routing and fallback semantics.
  • Add a scheduler-level regression covering two Photon cron deliveries and asserting the second does not invoke the live adapter or standalone fallback.

Automated hermes-sweeper review.

Comment thread gateway/platforms/base.py
@@ -2328,6 +2328,7 @@ def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> s
or "not enough rights" in blob
or "have no rights" in blob
or "not a member" in blob
or "target_not_allowed" in blob

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.

Blocking: cron does not call the wrapper that consumes this classification. cron/scheduler.py:1789-1806 invokes _deliver_to_platform() directly, while mark_dead() is only reached from DeliveryRouter.deliver() at gateway/delivery.py:376-385; this match therefore will not short-circuit a later cron tick.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@pierrenode
pierrenode force-pushed the fix/photon-dead-target-classification branch from 273f3bf to 2898814 Compare August 11, 2026 07:43
@pierrenode pierrenode changed the title fix(gateway): classify Photon target_not_allowed as a dead target fix(gateway): classify Photon target_not_allowed as a dead target and wire cron delivery through dead-target tracking Aug 11, 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.
@pierrenode
pierrenode force-pushed the fix/photon-dead-target-classification branch from 2898814 to 84430d1 Compare August 13, 2026 23:39
@pierrenode pierrenode changed the title fix(gateway): classify Photon target_not_allowed as a dead target and wire cron delivery through dead-target tracking fix(gateway): classify Photon target_not_allowed as a dead target Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants