fix(photon): recover inbound after half-open ("zombie") gRPC stream - #45580
fix(photon): recover inbound after half-open ("zombie") gRPC stream#45580vaibhavjnf wants to merge 1 commit into
Conversation
spectrum-ts's live-stream consumer (consumeLive in spectrum-ts 3.x) only
reconnects when its inbound async iterator throws or ends. A half-open
("zombie") gRPC socket — where the TCP connection stays ESTABLISHED but the
peer is gone (NAT idle-timeout, network blip, laptop sleep) — makes the
iterator hang forever: no error, no end. The SDK exposes no gRPC keepalive
knob (createClient takes only {address, tls, token}; grpc.keepalive_time_ms
defaults to -1 = pings off), so the inbound stream silently dies and stays
dead until the gateway is restarted. Symptom: the agent's iMessage line goes
"online but deaf" — Photon's cloud-side fallback answers users with "the agent
isn't online right now" and inbound never reaches the gateway.
Fix, entirely in the code we own (no SDK fork):
- Sidecar gains a POST /probe endpoint that drives a cheap unary read
(space.getMessage on a synthetic id) over the SAME gRPC channel the inbound
stream uses. A live channel round-trips in ms (server returns not-found,
which is success for liveness); a zombie hangs. It sends nothing to any user
and creates no chat (space.get is local in shared/dedicated mode; only the
message read touches the wire).
- The adapter runs a presence watchdog: it probes on an interval, skips the
probe when natural inbound traffic already proved liveness within the
window, and after N consecutive failed probes respawns the sidecar — a fresh
Spectrum() re-subscribes the stream and re-registers presence. Successful
probes double as application-level keepalive, helping prevent the zombie
from forming at all. Respawn is lock-guarded against double-spawn and the
watchdog is torn down cleanly on disconnect.
Behavioural settings live in config.yaml (extra), bridged to env per the
.env-is-secrets-only convention:
probe_interval_seconds (60), probe_timeout_seconds (10),
probe_max_failures (3). A non-positive interval disables the watchdog.
Tests: tests/plugins/platforms/photon/test_presence_watchdog.py covers config
resolution, the disable switch, probe alive/dead(500)/timeout/no-client, the
core N-failures->one-respawn detection, success-resets-failures, stop-then-
start respawn ordering, and lock-guarding — all without spawning Node or
hitting the network.
Contributed by Vaibhav Sharma (X: @vabbyshabby).
|
LGTM. This addresses a real "agent goes deaf" failure: spectrum-ts only recovers from a half-open/zombie gRPC socket when the async iterator throws or ends, not when it silently hangs (NAT idle-timeout, network blip, laptop sleep), so the inbound |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Photon adapter now includes a presence watchdog that recovers from half-open zombie gRPC streams. The watchdog drives periodic probes to the sidecar /probe endpoint and respawns the sidecar after consecutive failures, keeping the inbound stream alive. The implementation is well-designed: uses _first_set (not or) to honor explicit zero values, disables via non-positive interval, and is gated behind config.yaml options. Test coverage added for the watchdog behavior. No issues found.
Reviewed by Hermes Agent
|
Yay |
|
Cant wait to become an OSS contributor |
|
? |
|
@alt-glitch Heyyyy |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tackling a real resilience gap. Current main already recovers streams that report degradation, but the stated fully silent half-open case is not covered: app.messages only changes health on an iteration, end/error, or recognized SDK telemetry (plugins/platforms/photon/sidecar/index.mjs:526-558), while the adapter acts only on explicit degraded health (plugins/platforms/photon/adapter.py:540-581).
Problems
- The new
/probecatches everyspace.getMessage()rejection and returns{ alive: true }(plugins/platforms/photon/sidecar/index.mjs, PR right line 532). That makes an immediate transport/auth/API rejection indistinguishable from the intended not-found response, so the watchdog can miss a broken channel. - The change is based on spectrum-ts 3.1.0, whereas current main pins 8.0.0 (
plugins/platforms/photon/sidecar/package.json:12) and now routes health failures through the adapter fatal-error/reconnect lifecycle (plugins/platforms/photon/adapter.py:540-581).
Suggested changes
- Port the probe into the current stream-health/reconnect design and verify the spectrum-ts 8 API behavior.
- Accept only the verified not-found error as a successful probe; add a Node-level regression test for not-found versus transport/API failure.
Automated hermes-sweeper review.
| const probeId = PROBE_MSG_PREFIX + Date.now() + "-" + Math.random().toString(36).slice(2); | ||
| try { | ||
| await space.getMessage(probeId); | ||
| } catch { |
There was a problem hiding this comment.
This catch treats every completed getMessage failure as liveness. Please distinguish the expected not-found error from transport, authentication, and SDK failures; otherwise a failed unary call can return 200 and indefinitely reset the watchdog.
…ict probe semantics Maintainer rework of #45580 (issue #54036) on top of the contributor's cherry-pick, which targeted spectrum-ts 3.1.0 while main pins 8.0.0: Sidecar (primary detection, new): - stream-staleness.mjs: pure decision rules, executable under node. * classifyProbeRejection: only a not-found-shaped rejection of the synthetic-id read counts as a completed round-trip (ALIVE); any other rejection is INCONCLUSIVE — never alive. The original /probe treated ANY rejection as alive, which was too loose. * shouldProbe: probe only after 10+ min of stream silence (configurable via PHOTON_STREAM_SILENCE_PROBE_MS; <=0 disables) with a cooldown. * isZombieSuspect: zombie only on silence past threshold AND a probe-proven live channel. Silence alone NEVER degrades (shared lines can be quiet for hours); inconclusive probes NEVER degrade (network may be down — the iterator will throw and the re-subscribe loop recovers on its own). - index.mjs: track last inbound-iterator yield (noteInboundYield), run a 30s watchdog tick, and on a confirmed zombie feed markStreamDegraded -> the existing exit-75 restart path. /healthz gains a stream.staleness block (silentForMs, threshold, lastProbeOutcome, zombieSuspected). /probe reworked to strict semantics: 200 only on a proven round-trip, 503 with outcome hung|inconclusive otherwise. Adapter (second layer, reworked): - _probe_once returns tri-state alive|hung|inconclusive; only a hung sidecar HTTP call counts toward the respawn counter — inconclusive resets nothing and triggers nothing. - default probe_interval_seconds 60 -> 600 (conservative; avoid restart storms on quiet lines). - _monitor_sidecar_health surfaces zombieSuspected from /healthz as a warning; the fatal UPSTREAM_STREAM_DEGRADED path is unchanged and fires when the sidecar escalates. Tests: test_zombie_stream_watchdog.py executes the real node decision module and drives the adapter against mocked /healthz responses; test_presence_watchdog.py updated for the tri-state probe. Also adds contributor mappings for nickkarhan (#53283) and vaibhavjnf (#45580).
…ict probe semantics Maintainer rework of #45580 (issue #54036) on top of the contributor's cherry-pick, which targeted spectrum-ts 3.1.0 while main pins 8.0.0: Sidecar (primary detection, new): - stream-staleness.mjs: pure decision rules, executable under node. * classifyProbeRejection: only a not-found-shaped rejection of the synthetic-id read counts as a completed round-trip (ALIVE); any other rejection is INCONCLUSIVE — never alive. The original /probe treated ANY rejection as alive, which was too loose. * shouldProbe: probe only after 10+ min of stream silence (configurable via PHOTON_STREAM_SILENCE_PROBE_MS; <=0 disables) with a cooldown. * isZombieSuspect: zombie only on silence past threshold AND a probe-proven live channel. Silence alone NEVER degrades (shared lines can be quiet for hours); inconclusive probes NEVER degrade (network may be down — the iterator will throw and the re-subscribe loop recovers on its own). - index.mjs: track last inbound-iterator yield (noteInboundYield), run a 30s watchdog tick, and on a confirmed zombie feed markStreamDegraded -> the existing exit-75 restart path. /healthz gains a stream.staleness block (silentForMs, threshold, lastProbeOutcome, zombieSuspected). /probe reworked to strict semantics: 200 only on a proven round-trip, 503 with outcome hung|inconclusive otherwise. Adapter (second layer, reworked): - _probe_once returns tri-state alive|hung|inconclusive; only a hung sidecar HTTP call counts toward the respawn counter — inconclusive resets nothing and triggers nothing. - default probe_interval_seconds 60 -> 600 (conservative; avoid restart storms on quiet lines). - _monitor_sidecar_health surfaces zombieSuspected from /healthz as a warning; the fatal UPSTREAM_STREAM_DEGRADED path is unchanged and fires when the sidecar escalates. Tests: test_zombie_stream_watchdog.py executes the real node decision module and drives the adapter against mocked /healthz responses; test_presence_watchdog.py updated for the tri-state probe. Also adds contributor mappings for nickkarhan (#53283) and vaibhavjnf (#45580).
…ict probe semantics Maintainer rework of #45580 (issue #54036) on top of the contributor's cherry-pick, which targeted spectrum-ts 3.1.0 while main pins 8.0.0: Sidecar (primary detection, new): - stream-staleness.mjs: pure decision rules, executable under node. * classifyProbeRejection: only a not-found-shaped rejection of the synthetic-id read counts as a completed round-trip (ALIVE); any other rejection is INCONCLUSIVE — never alive. The original /probe treated ANY rejection as alive, which was too loose. * shouldProbe: probe only after 10+ min of stream silence (configurable via PHOTON_STREAM_SILENCE_PROBE_MS; <=0 disables) with a cooldown. * isZombieSuspect: zombie only on silence past threshold AND a probe-proven live channel. Silence alone NEVER degrades (shared lines can be quiet for hours); inconclusive probes NEVER degrade (network may be down — the iterator will throw and the re-subscribe loop recovers on its own). - index.mjs: track last inbound-iterator yield (noteInboundYield), run a 30s watchdog tick, and on a confirmed zombie feed markStreamDegraded -> the existing exit-75 restart path. /healthz gains a stream.staleness block (silentForMs, threshold, lastProbeOutcome, zombieSuspected). /probe reworked to strict semantics: 200 only on a proven round-trip, 503 with outcome hung|inconclusive otherwise. Adapter (second layer, reworked): - _probe_once returns tri-state alive|hung|inconclusive; only a hung sidecar HTTP call counts toward the respawn counter — inconclusive resets nothing and triggers nothing. - default probe_interval_seconds 60 -> 600 (conservative; avoid restart storms on quiet lines). - _monitor_sidecar_health surfaces zombieSuspected from /healthz as a warning; the fatal UPSTREAM_STREAM_DEGRADED path is unchanged and fires when the sidecar escalates. Tests: test_zombie_stream_watchdog.py executes the real node decision module and drives the adapter against mocked /healthz responses; test_presence_watchdog.py updated for the tri-state probe. Also adds contributor mappings for nickkarhan (#53283) and vaibhavjnf (#45580).
…ict probe semantics Maintainer rework of #45580 (issue #54036) on top of the contributor's cherry-pick, which targeted spectrum-ts 3.1.0 while main pins 8.0.0: Sidecar (primary detection, new): - stream-staleness.mjs: pure decision rules, executable under node. * classifyProbeRejection: only a not-found-shaped rejection of the synthetic-id read counts as a completed round-trip (ALIVE); any other rejection is INCONCLUSIVE — never alive. The original /probe treated ANY rejection as alive, which was too loose. * shouldProbe: probe only after 10+ min of stream silence (configurable via PHOTON_STREAM_SILENCE_PROBE_MS; <=0 disables) with a cooldown. * isZombieSuspect: zombie only on silence past threshold AND a probe-proven live channel. Silence alone NEVER degrades (shared lines can be quiet for hours); inconclusive probes NEVER degrade (network may be down — the iterator will throw and the re-subscribe loop recovers on its own). - index.mjs: track last inbound-iterator yield (noteInboundYield), run a 30s watchdog tick, and on a confirmed zombie feed markStreamDegraded -> the existing exit-75 restart path. /healthz gains a stream.staleness block (silentForMs, threshold, lastProbeOutcome, zombieSuspected). /probe reworked to strict semantics: 200 only on a proven round-trip, 503 with outcome hung|inconclusive otherwise. Adapter (second layer, reworked): - _probe_once returns tri-state alive|hung|inconclusive; only a hung sidecar HTTP call counts toward the respawn counter — inconclusive resets nothing and triggers nothing. - default probe_interval_seconds 60 -> 600 (conservative; avoid restart storms on quiet lines). - _monitor_sidecar_health surfaces zombieSuspected from /healthz as a warning; the fatal UPSTREAM_STREAM_DEGRADED path is unchanged and fires when the sidecar escalates. Tests: test_zombie_stream_watchdog.py executes the real node decision module and drives the adapter against mocked /healthz responses; test_presence_watchdog.py updated for the tri-state probe. Also adds contributor mappings for nickkarhan (#53283) and vaibhavjnf (#45580).
…ict probe semantics Maintainer rework of #45580 (issue #54036) on top of the contributor's cherry-pick, which targeted spectrum-ts 3.1.0 while main pins 8.0.0: Sidecar (primary detection, new): - stream-staleness.mjs: pure decision rules, executable under node. * classifyProbeRejection: only a not-found-shaped rejection of the synthetic-id read counts as a completed round-trip (ALIVE); any other rejection is INCONCLUSIVE — never alive. The original /probe treated ANY rejection as alive, which was too loose. * shouldProbe: probe only after 10+ min of stream silence (configurable via PHOTON_STREAM_SILENCE_PROBE_MS; <=0 disables) with a cooldown. * isZombieSuspect: zombie only on silence past threshold AND a probe-proven live channel. Silence alone NEVER degrades (shared lines can be quiet for hours); inconclusive probes NEVER degrade (network may be down — the iterator will throw and the re-subscribe loop recovers on its own). - index.mjs: track last inbound-iterator yield (noteInboundYield), run a 30s watchdog tick, and on a confirmed zombie feed markStreamDegraded -> the existing exit-75 restart path. /healthz gains a stream.staleness block (silentForMs, threshold, lastProbeOutcome, zombieSuspected). /probe reworked to strict semantics: 200 only on a proven round-trip, 503 with outcome hung|inconclusive otherwise. Adapter (second layer, reworked): - _probe_once returns tri-state alive|hung|inconclusive; only a hung sidecar HTTP call counts toward the respawn counter — inconclusive resets nothing and triggers nothing. - default probe_interval_seconds 60 -> 600 (conservative; avoid restart storms on quiet lines). - _monitor_sidecar_health surfaces zombieSuspected from /healthz as a warning; the fatal UPSTREAM_STREAM_DEGRADED path is unchanged and fires when the sidecar escalates. Tests: test_zombie_stream_watchdog.py executes the real node decision module and drives the adapter against mocked /healthz responses; test_presence_watchdog.py updated for the tri-state probe. Also adds contributor mappings for nickkarhan (#53283) and vaibhavjnf (#45580).
|
Merged via #73615 — cherry-picked with authorship preserved, plus a maintainer rework porting the watchdog to the spectrum-ts 8 surface with strict probe semantics (degraded only when a probe PROVES connectivity while the stream is silent — never on silence alone, so quiet shared lines don't restart-storm). You were the only one who went after the silent-cold case in #54036; thanks for the foundation. |
…ict probe semantics Maintainer rework of NousResearch#45580 (issue NousResearch#54036) on top of the contributor's cherry-pick, which targeted spectrum-ts 3.1.0 while main pins 8.0.0: Sidecar (primary detection, new): - stream-staleness.mjs: pure decision rules, executable under node. * classifyProbeRejection: only a not-found-shaped rejection of the synthetic-id read counts as a completed round-trip (ALIVE); any other rejection is INCONCLUSIVE — never alive. The original /probe treated ANY rejection as alive, which was too loose. * shouldProbe: probe only after 10+ min of stream silence (configurable via PHOTON_STREAM_SILENCE_PROBE_MS; <=0 disables) with a cooldown. * isZombieSuspect: zombie only on silence past threshold AND a probe-proven live channel. Silence alone NEVER degrades (shared lines can be quiet for hours); inconclusive probes NEVER degrade (network may be down — the iterator will throw and the re-subscribe loop recovers on its own). - index.mjs: track last inbound-iterator yield (noteInboundYield), run a 30s watchdog tick, and on a confirmed zombie feed markStreamDegraded -> the existing exit-75 restart path. /healthz gains a stream.staleness block (silentForMs, threshold, lastProbeOutcome, zombieSuspected). /probe reworked to strict semantics: 200 only on a proven round-trip, 503 with outcome hung|inconclusive otherwise. Adapter (second layer, reworked): - _probe_once returns tri-state alive|hung|inconclusive; only a hung sidecar HTTP call counts toward the respawn counter — inconclusive resets nothing and triggers nothing. - default probe_interval_seconds 60 -> 600 (conservative; avoid restart storms on quiet lines). - _monitor_sidecar_health surfaces zombieSuspected from /healthz as a warning; the fatal UPSTREAM_STREAM_DEGRADED path is unchanged and fires when the sidecar escalates. Tests: test_zombie_stream_watchdog.py executes the real node decision module and drives the adapter against mocked /healthz responses; test_presence_watchdog.py updated for the tri-state probe. Also adds contributor mappings for nickkarhan (NousResearch#53283) and vaibhavjnf (NousResearch#45580).
…ict probe semantics Maintainer rework of NousResearch#45580 (issue NousResearch#54036) on top of the contributor's cherry-pick, which targeted spectrum-ts 3.1.0 while main pins 8.0.0: Sidecar (primary detection, new): - stream-staleness.mjs: pure decision rules, executable under node. * classifyProbeRejection: only a not-found-shaped rejection of the synthetic-id read counts as a completed round-trip (ALIVE); any other rejection is INCONCLUSIVE — never alive. The original /probe treated ANY rejection as alive, which was too loose. * shouldProbe: probe only after 10+ min of stream silence (configurable via PHOTON_STREAM_SILENCE_PROBE_MS; <=0 disables) with a cooldown. * isZombieSuspect: zombie only on silence past threshold AND a probe-proven live channel. Silence alone NEVER degrades (shared lines can be quiet for hours); inconclusive probes NEVER degrade (network may be down — the iterator will throw and the re-subscribe loop recovers on its own). - index.mjs: track last inbound-iterator yield (noteInboundYield), run a 30s watchdog tick, and on a confirmed zombie feed markStreamDegraded -> the existing exit-75 restart path. /healthz gains a stream.staleness block (silentForMs, threshold, lastProbeOutcome, zombieSuspected). /probe reworked to strict semantics: 200 only on a proven round-trip, 503 with outcome hung|inconclusive otherwise. Adapter (second layer, reworked): - _probe_once returns tri-state alive|hung|inconclusive; only a hung sidecar HTTP call counts toward the respawn counter — inconclusive resets nothing and triggers nothing. - default probe_interval_seconds 60 -> 600 (conservative; avoid restart storms on quiet lines). - _monitor_sidecar_health surfaces zombieSuspected from /healthz as a warning; the fatal UPSTREAM_STREAM_DEGRADED path is unchanged and fires when the sidecar escalates. Tests: test_zombie_stream_watchdog.py executes the real node decision module and drives the adapter against mocked /healthz responses; test_presence_watchdog.py updated for the tri-state probe. Also adds contributor mappings for nickkarhan (NousResearch#53283) and vaibhavjnf (NousResearch#45580).
Summary
The Photon iMessage adapter can go "online but deaf": the line stays
registered, but inbound messages never reach the gateway and Photon's
cloud-side fallback answers users with "the agent isn't online right now."
The only recovery today is a full gateway restart.
Root cause
spectrum-ts's live-stream consumer (consumeLivein 3.x) only reconnectswhen its inbound async iterator throws or ends:
A half-open ("zombie") gRPC socket — TCP still
ESTABLISHEDbut the peeris gone (NAT idle-timeout, network blip, laptop sleep) — makes that iterator
hang forever: no error, no end, so the SDK's own retry/backoff never
fires. The SDK exposes no gRPC keepalive knob (the cloud
createClienttakesonly
{address, tls, token}, andgrpc.keepalive_time_msdefaults to-1=pings off), so the inbound stream silently dies and stays dead.
Fix (entirely in the code we own — no SDK fork)
POST /probedrives a cheap unary read (space.getMessageon asynthetic id) over the same gRPC channel the inbound stream uses. A live
channel round-trips in ms (server returns not-found → success for liveness);
a zombie hangs. It sends nothing to any user and creates no chat
(
space.getis local in shared/dedicated mode; only the message read hitsthe wire).
natural inbound traffic already proved liveness within the window, and after
N consecutive failed probes respawns the sidecar — a fresh
Spectrum()re-subscribes the stream and re-registers presence. Successful probes double
as application-level keepalive, helping prevent the zombie forming at all.
Respawn is lock-guarded against double-spawn; the watchdog is torn down
cleanly on
disconnect().Config (config.yaml
extra, bridged to env per the secrets-only.envconvention)probe_interval_seconds60probe_timeout_seconds10probe_max_failures3A non-positive
probe_interval_secondsdisables the watchdog.Test plan
tests/plugins/platforms/photon/test_presence_watchdog.py— 12 tests:config resolution, disable switch, probe alive / dead(500) / timeout /
no-client, N-failures → exactly-one-respawn, success-resets-failures,
stop-then-start respawn ordering, lock-guarding. No Node spawn, no network.
respawn killed the old sidecar PID and a fresh one came up with a live
/probe; 3 consecutive failed probes triggered exactly one respawn.Contributed by Vaibhav Sharma (X: @vabbyshabby).