Skip to content

fix(photon): recover sidecar after outbound transport drops (salvaged v2) - #65392

Closed
Jeffgithub0029 wants to merge 1 commit into
NousResearch:mainfrom
Jeffgithub0029:fix/photon-outbound-recovery-v2
Closed

fix(photon): recover sidecar after outbound transport drops (salvaged v2)#65392
Jeffgithub0029 wants to merge 1 commit into
NousResearch:mainfrom
Jeffgithub0029:fix/photon-outbound-recovery-v2

Conversation

@Jeffgithub0029

Copy link
Copy Markdown
Contributor

Summary

Salvaged the direct outbound transport-drop restart/retry path from original PR #49653 (aacbd5b7f) into current main, while dropping the stale inbound fatal hooks that conflict with main's already-merged broader degraded-stream recovery.

What this PR keeps (value-add on top of main)

  1. PhotonAdapter.send() — now detects upstream_connection_dropped (via error_code from sidecar and plain-text fallback), restarts the sidecar once, retries the send. Covers cron live-delivery, send_message tool, agent replies — all call adapter.send() directly, bypassing _send_with_retry.
  2. _send_with_retry() — same sidecar-restart-before-retry logic for the gateway response path.
  3. _sidecar_call() — propagates error_code from sidecar HTTP responses into exception messages so Python can detect the drop without regex guesswork.
  4. Sidecar classifyRecoverableOutboundError() — emits error_code: upstream_connection_dropped on [upstream] Connection dropped so the adapter sees a machine-readable signal.
  5. Tests — 5 new pytest cases + 1 sidecar unit test covering direct send, plain-text fallback, and retry path.

What this PR drops (stale vs. main)

  • index.mjs console-error-only fatal hook (isFatalInboundStreamError + scheduleFatalInboundExit + FATAL_INBOUND_EXIT_CODE=75) — main already has _monitor_sidecar_health() (06cbc3b) + dual console.error/console.log interception (7f1c278) with broader coverage.
  • fatal-errors.mjs isFatalInboundStreamError — same reason, inbound path is covered by main's health monitor.

Testing

$ pytest tests/plugins/test_photon_adapter_outbound_recovery.py tests/plugins/test_photon_adapter_sidecar_restart.py -v
# 5 passed

$ pytest -k photon -v --timeout=60
# 119 passed, 2 skipped

$ cd plugins/platforms/photon/sidecar && npm test
# 1 passed (classifyRecoverableOutboundError)

Follow-up

  • _ensure_sidecar_running() added to inbound loop; it's a lightweight guard before reconnecting the /inbound stream. Main's supervisor already triggers fatal error on sidecar crash — this just avoids a race where the stream reconnects before the sidecar is back up. Can be removed if deemed redundant.

… v2)

- Add _sidecar_restart_lock to serialize restarts
- Add _ensure_sidecar_running() called before inbound reconnect
- Add _restart_sidecar_for_outbound_error() + _is_recoverable_outbound_drop()
- Hook into adapter.send() (direct cron/tools/agent path) and _send_with_retry()
- Propagate sidecar error_code in _sidecar_call() exceptions
- Remove stale inbound fatal hook (isFatalInboundStreamError, FATAL_INBOUND_EXIT_CODE)
- Keep only outbound classifier in fatal-errors.mjs
- Add test coverage for direct send, send_with_retry, and sidecar restart
@Jeffgithub0029
Jeffgithub0029 requested a review from a team July 16, 2026 05:35
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 16, 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 preserving the direct adapter.send() recovery target; that path is real on current main (tools/send_message_tool.py:727, plugins/platforms/photon/adapter.py:1083-1090).

Problems

  • The new classifyRecoverableOutboundError() is not imported or called by the sidecar. Current /send failures reach serverError() through plugins/platforms/photon/sidecar/index.mjs:844-851; that helper intentionally returns only "internal sidecar error" (index.mjs:600-606). Thus the PR's error_code=upstream_connection_dropped and raw-text restart branches cannot be reached by an actual sidecar response.
  • tests/plugins/test_photon_adapter_outbound_recovery.py constructs the proposed error code in mocks, so it does not detect the missing sidecar wiring.
  • The .gitignore hunk duplicates current desktop patterns and re-adds apps/desktop/src/**/*.d.ts after existing negated exceptions at .gitignore:77-78.

Suggested changes

  • Emit a stable, non-sensitive outbound error code from the actual /send failure path while retaining the generic error response.
  • Test that response path through the adapter restart/retry behavior, and remove the unrelated .gitignore change.

Automated hermes-sweeper review.

if (!parts.length) return String(error ?? "");
return parts.join("\n");
}

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 classifier is never imported or invoked by index.mjs, so no live /send response can receive this classification. The current /send catch calls serverError(), which returns only the generic internal sidecar error; wire this into that real response path before adapter.py depends on error_code.

if calls["send"] == 1:
return SendResult(
success=False,
error='Photon sidecar /send returned 500 error_code=upstream_connection_dropped: {"ok":false,"error":"internal sidecar error","error_code":"upstream_connection_dropped"}',

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 test injects an error_code that the current sidecar does not emit. Add coverage of the actual sidecar /send error construction as well, otherwise this passes while the production restart branch remains unreachable.

Comment thread .gitignore
apps/desktop/src/**/*.js
apps/desktop/src/**/*.js.map
apps/desktop/src/**/*.d.ts

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.

Please drop this unrelated duplicate pattern. Current .gitignore already ignores this glob, then explicitly unignores global.d.ts and vite-env.d.ts; re-adding the glob here after those exceptions changes their precedence.

@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 16, 2026
@Jeffgithub0029

Jeffgithub0029 commented Jul 16, 2026 via email

Copy link
Copy Markdown
Contributor Author

@teknium1

Copy link
Copy Markdown
Contributor

Closing — the structured-error direction landed via #73563 (salvaging #51193 with rework): the sidecar now emits real error classes, which was the missing piece that made this PR's classifyRecoverableOutboundError unreachable. The direct-send-bypasses-retry premise you identified was real and is covered by the fatal-class returns. Thanks for the v2 effort and for acknowledging the wiring gap.

@teknium1 teknium1 closed this Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have 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.

3 participants