Skip to content

fix(gateway): prevent duplicate Photon sidecar storms - #61868

Closed
s00rz wants to merge 4 commits into
NousResearch:mainfrom
s00rz:fix/photon-multiplex-sidecar-storm
Closed

fix(gateway): prevent duplicate Photon sidecar storms#61868
s00rz wants to merge 4 commits into
NousResearch:mainfrom
s00rz:fix/photon-multiplex-sidecar-storm

Conversation

@s00rz

@s00rz s00rz commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents multiplexed gateways from launching duplicate Photon sidecars and from marking a healthy shared platform disconnected during duplicate rejection.

When gateway.multiplex_profiles is enabled, credential-backed platforms can be discovered for every served profile. The duplicate guard recognized bot-token fields but not Photon/Spectrum's _project_secret, so each profile attempted to launch a Node sidecar on the same 127.0.0.1:8789 listener. The rejection path also called disconnect() on freshly constructed adapters that had never connected; for shared-credential adapters that could mutate aggregate platform state and shut down the primary Photon sidecar.

This change makes Photon credentials participate in the existing log-safe fingerprint and treats a rejected, never-connected adapter as owning no resources to disconnect.

Related Issue

No tracking issue filed.

Companion: #54565 handles the separate Windows CREATE_NO_WINDOW hardening for Photon subprocesses. This PR intentionally does not duplicate that contributor's work.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/run.py
    • Include Photon/Spectrum project secrets in multiplex credential fingerprints.
    • Do not call disconnect() for duplicates rejected before connect().
    • Generalize the duplicate diagnostic beyond bot tokens.
  • tests/gateway/test_multiplex_adapter_registry.py
    • Verify Photon project-secret fingerprints are stable and log-safe.
    • Verify a duplicate adapter is neither connected nor disconnected and cannot disturb shared live state.

Sweeper Feedback Follow-up

The automated review correctly identified that credential fingerprints alone do not prevent two different Photon projects from binding the same sidecar endpoint. Follow-up commits preserve the original fix and additionally:

  • Claim Photon sidecar listeners independently by normalized bind + port.
  • Reject distinct credentials sharing one listener before connect() or disconnect().
  • Continue allowing distinct credentials when each profile uses a distinct sidecar port.
  • Publish claims only after a successful secondary connection, so a failed start cannot block a later valid profile.
  • Reserve retryable primary credential/listener ownership so the reconnect watcher cannot collide with a secondary sidecar.
  • Cover same credentials, distinct credentials on the default port, distinct ports, failed-secondary recovery, and retrying-primary ownership.

How to Test

  1. Configure a default profile plus secondary profiles with gateway.multiplex_profiles: true and shared Photon credentials.
  2. Start the gateway.
  3. Confirm one primary Photon adapter connects, secondary duplicates are rejected before spawning, and the primary Photon/Telegram platform states remain connected.
  4. Run:
    • scripts/run_tests.sh tests/gateway/test_multiplex_adapter_registry.py -q
    • python scripts/check-windows-footguns.py --all
    • ruff check .

Validation completed:

  • Follow-up focused Windows suite after the sweeper remediation: 16 passed.
  • Repository wrapper in an isolated Linux/CI-parity container: 12 passed.
  • Native Windows 11 focused pytest: 12 passed.
  • Windows footgun scanner: 753 files scanned, no findings.
  • Full Ruff: passed.
  • Python compilation and git diff --check: passed.
  • Live Windows 11 gateway: one Photon sidecar, zero post-fix EADDRINUSE/sidecar-exit events, Telegram + Photon + API server connected, and repeated five-minute watchdog checks returned 0 without a restart storm.

Native-Windows wrapper note: the unmodified wrapper currently fails before collection under Git Bash because env -i drops Windows home variables and the progress renderer uses CP1252. The same wrapper passed unchanged in the Linux container, and the focused tests passed directly in the native Windows environment. The advisory full-tree ty scan is also not claimed green: ty 0.0.21 panicked on an unrelated existing file and reported the repository's baseline diagnostics.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — focused wrapper/native suites passed; full matrix is delegated to CI
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11 and Linux container

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A; behavior is internal and documented inline
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Before, one restart produced ten secondary Photon sidecar exits on the shared port and duplicate cleanup left aggregate platform state disconnected. After the fix:

photon_node_count=1
startup_eaddrinuse=0
startup_sidecar_exits=0
telegram=connected
photon=connected
watchdog_result=0

@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 10, 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 addressing a real Photon multiplexing gap: current gateway/run.py:8621-8643 does not fingerprint _project_secret, while PhotonAdapter stores the resolved secret at plugins/platforms/photon/adapter.py:290-294.

Problems

  • The new fingerprint only detects equal secrets. Two profiles with different Photon credentials still bypass the claim check, but both default to 127.0.0.1:8789 (plugins/platforms/photon/adapter.py:298-301) and each connect() starts a sidecar (plugins/platforms/photon/adapter.py:414-436). That leaves a multiplexed EADDRINUSE path.
  • The added test hashes a fake secret but does not cover Photon multiplex startup or distinct credentials sharing the default sidecar port.

Suggested changes

  • Guard Photon sidecar bind/port collisions independently from credential fingerprints, or require an explicit distinct sidecar port for a secondary Photon profile.
  • Add focused coverage for same credentials, distinct credentials on the default port, and distinct ports if supported.

Automated hermes-sweeper review.

Comment thread gateway/run.py
# Photon/Spectrum authenticates with project credentials instead
# of a bot token. Including its secret keeps multiplexed profiles
# from spawning competing sidecars for the same account and port.
"_project_secret",

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.

This only catches identical secrets. Photon adapters with different project credentials still bypass this guard, but both default to the same sidecar listener (plugins/platforms/photon/adapter.py:298-301) and start a sidecar in connect(); please also guard the bind/port collision or require a distinct secondary sidecar port.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
@s00rz

s00rz commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the automated sweeper feedback with additive follow-up commits on the existing PR branch; the original commit and its same-credential/disconnect fix remain intact.

The follow-up now guards Photon sidecar bind/port collisions independently from credentials, allows distinct configured ports, releases claims after failed secondary starts, and reserves retrying-primary ownership so reconnects cannot collide with a secondary. Focused coverage now includes all requested credential/port combinations plus failure/retry lifecycle cases.

Final fast-forward-branch validation: 16 passed in tests/gateway/test_multiplex_adapter_registry.py; focused Ruff passed; py_compile and git diff --check passed; Windows footgun scan passed (753 files). No companion PR was opened and the PR remains a draft.

@s00rz
s00rz force-pushed the fix/photon-multiplex-sidecar-storm branch from 4cd8e4c to 8c69d1b Compare July 18, 2026 15:18
@s00rz

s00rz commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Rebased the four Photon commits onto current upstream main and resolved the intervening multiplex configuration/reconnect refactors. The PR is now ready for review and GitHub reports it mergeable with no conflicts. Post-rebase validation: focused multiplex suite 37 passed; adjacent gateway startup/reconnect suites 56 passed; full Ruff passed; Windows footgun scan passed (777 files); py_compile and git diff --check passed. Upstream CI was triggered but is currently action_required and needs maintainer workflow approval.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #73563 — cherry-picked (incl. your rebase follow-ups) with authorship preserved. First in the train since it was the only gateway/run.py piece.

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-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

4 participants