Count frames from the broker server as tunnel activity for the idle watchdog - #151
Conversation
A serve-mode (handler) agent's relay tunnel only carries what relay-dispatcher sends it, mostly the periodic /__axon/info liveness poll. The broker server delivers a token's request to the newest client on each server, so when several replicas share a token only one of them is polled. The others see no traffic, hit RELAY_IDLE_TIMEOUT, restart, become the newest client, and the rotation repeats forever. Serve mode now defaults RelayIdleTimeout to 0. Setting RELAY_IDLE_TIMEOUT explicitly still enables the watchdog. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…atchdog The idle watchdog restarted the broker when no request had been relayed for RELAY_IDLE_TIMEOUT. The broker server hands a token's requests to only its newest client on each server, so every other replica sharing a token looked idle and was restarted in rotation, in relay and serve mode alike. The reflector's websocket proxy now reports bytes that arrive from the broker server, which heartbeats every tunnel, and the watchdog treats those as activity alongside relayed requests and the last restart. Bytes we send don't count: a write into a dead tunnel can still succeed. A tunnel that goes quiet is still restarted. This replaces the earlier change that turned the watchdog off in serve mode, which would also have stopped it recovering a dead tunnel there. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
shawnburke
changed the title
| lastActivity := r.reflector.LastTrafficTime() | ||
| if startup := r.reflector.LastStartupTime(); startup.After(lastActivity) { | ||
| lastActivity = startup | ||
| for _, t := range []time.Time{ |
There was a problem hiding this comment.
why isn't this just a max of (startup, traffic, activity)?
Also can we have traffic w/o activity? do we need both?
There was a problem hiding this comment.
It should be, and now is: RegistrationReflector.LastActivityTime() returns max() of the three watermarks, and shouldRestart just compares that against the timeout. Dropped LastTunnelActivityTime, which had no other caller.
On traffic without activity — yes, we need both. The watchdog runs whenever the reflector reflects traffic (traffic or all mode), but the tunnel only passes through the reflector when it reflects registration (registration or all). So:
all(default): every relayed request arrives down the tunnel, so activity ≥ traffic and traffic adds nothing.traffic: the broker connects to the server directly, the reflector never sees a frame, and relayed requests are the only signal. Dropping traffic would make the watchdog restart every 10 minutes in that mode.
Kept the separate watermarks (rather than writing everything into one) so LastTrafficTime still means "last relayed request" — TestRestartResetsIdleClock pins that.
Review feedback on #151. The reflector now exposes LastActivityTime, the latest of the last relayed request, the last frame from the broker server, and the last (re)start, and shouldRestart compares only that. LastTunnelActivityTime had no other caller and is gone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
WebSocket activity is not scoped to the broker tunnel, and registration mode does not use the recorded activity.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This pull request adds broker-server WebSocket activity to the relay idle watchdog so healthy, low-traffic tunnels are not unnecessarily restarted.
Changes:
- Tracks target-to-client WebSocket activity separately from relayed traffic.
- Includes tunnel activity in idle-restart decisions.
- Adds WebSocket and watchdog tests.
The review identified unresolved issues with scoping activity to broker-server tunnels and supporting registration reflector mode.
| File | Description |
|---|---|
agent/server/snykbroker/ws_proxy.go |
Reports activity from broker-server frames. |
agent/server/snykbroker/ws_proxy_test.go |
Tests target-only activity callbacks. |
agent/server/snykbroker/relay_instance_manager.go |
Uses combined activity for restart decisions. |
agent/server/snykbroker/relay_instance_manager_test.go |
Tests tunnel-aware idle handling. |
agent/server/snykbroker/reflector.go |
Records tunnel activity and exposes timestamps. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| rr.wsProxy.OnTunnelEstablished = func(target string) { | ||
| rr.logger.Info("WebSocket tunnel established", zap.String("target", target)) | ||
| } | ||
| rr.wsProxy.OnActivity = rr.RecordTunnelActivity |
There was a problem hiding this comment.
Good catch — fixed in the latest commit. WebSocketProxy.Proxy now takes the activity callback per call instead of a proxy-wide OnActivity, and ServeHTTP passes RecordTunnelActivity only when entry.isDefault (the broker-server entry registered with WithDefault(true)). Websockets to customer origins get no callback. TestReflectorRecordsTunnelActivityForBrokerTunnelOnly covers both: a frame on the default entry advances the watermark, a frame on a customer-origin entry does not (and I confirmed that subtest fails if the isDefault check is removed).
| lastActivity = startup | ||
| } | ||
| if time.Since(lastActivity) >= r.config.RelayIdleTimeout { | ||
| if time.Since(r.reflector.LastActivityTime()) >= r.config.RelayIdleTimeout { |
There was a problem hiding this comment.
Agreed — fixed in the latest commit. shouldRestart now runs whenever the reflector reflects traffic or registration, so registration mode (broker tunnel through the reflector, no relayed requests) is watched via tunnel frames, and only disabled is excluded since the agent sees neither signal there. TestShouldRestartCountsTunnelActivity now also covers registration (stale → restart, tunnel frame → no restart) and disabled (never restarts). Note this does enable the watchdog in registration mode where it was previously off; it only fires if the server sends nothing for RELAY_IDLE_TIMEOUT, which with a ~30s server heartbeat means the tunnel is dead.
…stration mode Review feedback on #151. The reflector proxies every websocket upgrade, including websockets to customer origins in accept-file rules, so a busy one could keep the tunnel watermark fresh while the broker's tunnel to the server was dead. Proxy now takes the activity callback per call, and the reflector passes it only for the default entry, which is the broker server. In "registration" mode the broker's tunnel runs through the reflector but relayed requests don't, and shouldRestart returned before looking. It now runs in every reflecting mode and stays off only when the reflector is disabled, where the agent can see neither signal. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| if err := rr.wsProxy.Proxy(w, r, entry.TargetURI); err != nil { | ||
| // Only the default entry is the broker's own tunnel to the server; a | ||
| // websocket to a customer origin says nothing about that tunnel. | ||
| var onActivity func() |
There was a problem hiding this comment.
suuuuuuper nit but it was kind of confusing that there was this default lambda, not sure how i would implement it better though tbh
Summary
The relay idle watchdog is designed to allow "self healing" if we lose contact with an agent. After N minutes if it's seen no traffic, just restart it. This has worked well but is problematic in some cases, particularly a degenerate case where multiple agents connect to the same
snyk-broker-stsinstance in our cloud. Snyk broker has a thing where it only respects 1 of N agents connected to it and routes all traffic to that one. The others get no traffic, even if healthy.This change also interprets heartbeats over websockets as traffic so that the simple existence of a working websocket tunnel resets the watchdog.
Context: PPO-105.
Test plan
go build ./...,go vet ./server/snykbroker/go test ./server/... ./cmd/...make test-racesubset) passTestReflectorRecordsTunnelActivityForBrokerTunnelOnly: a frame on the broker tunnel resets the watchdog; our own write and a frame on a customer-origin websocket do not (passes under-race)TestShouldRestartCountsTunnelActivity: stale everything → restart; recent tunnel frame → no restart; tunnel goes quiet → restart again; same inregistrationmode;disablednever restartsProcessing restart request reason=idle_timeoutwhile their tunnels are up🤖 Generated with Claude Code