Skip to content

feat(relay): observe Redis subscription paths - #7544

Open
ravarora2 wants to merge 3 commits into
mainfrom
rarora/redis-subscription-health
Open

ravarora2 wants to merge 3 commits into
mainfrom
rarora/redis-subscription-health

Conversation

@ravarora2

@ravarora2 ravarora2 commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Why

Redis command-pool acquisition does not prove the relay's three dedicated Pub/Sub paths are usable. A pod could report healthy Redis readiness while event fan-out, cache invalidation, or connection control had not subscribed, was retrying, or had terminated. Operators also need to know how long each real handshake took and whether an unready path is actively trying, backing off, or permanently failed.

What

  • Add a fixed end-to-end health state machine for the event, cache, and connection_control paths, with one-hot connecting, ready, reconnecting, failed, and stopped states.
  • Measure last completed readiness duration, continuously growing current not-ready duration, current-state age, connection/subscription attempts, consecutive failure depth, last-ready time, and bounded transition reasons.
  • Attach all three relay consumers before Redis can deliver, and own and supervise both halves of every path (six tasks total).
  • Add cancellation-aware reconnect backoff and bounded shutdown that awaits every owned task.
  • Replace sleep-based Redis handshakes in integration coverage and add the missing connection-control round trip.

How

Each path becomes ready only after its downstream relay consumer is attached and the dedicated Redis connection completes the real SUBSCRIBE or PSUBSCRIBE handshake. Every retry re-enters connecting; failures move to reconnecting with a bounded reason and increment failure depth; readiness records the full elapsed interval including retry backoff and resets the depth. Unexpected consumer or network return and panic latch failed.

Current gauges refresh on the existing ten-second metrics tick so stable paths survive recorder idle expiry. Labels use only the three fixed paths, five fixed states, bounded transitions, and bounded reasons; customer values, endpoints, topics, payloads, credentials, and raw errors are excluded.

This PR exposes health only and intentionally does not add subscription paths to Kubernetes readiness. That serving-policy decision remains a separate rollout step.

Automated coverage includes the production six-task wiring, consumer-before-network ordering, unreachable Redis, retries, readiness-duration accounting with paused time, failure latching, panic and return supervision, bounded cancellation, one-hot Prometheus output, fixed label sets, and metric type/series contracts. Exact-head Rust unit, lint, Linux cross-compile, Windows Rust, security, Docker image, PostgreSQL, backend integration, and mesh lifecycle checks passed.

Risk

Medium. This moves six previously detached tasks under one owner and changes their shutdown behavior, but preserves existing handlers, Redis topics, message formats, normal reconnect policy, and public entry points. The new timing state uses monotonic clocks and saturating arithmetic. The metric vocabulary is fixed-cardinality.

Bigger picture

This makes “all cross-pod Redis paths are ready on every relay pod” observable, distinguishes pool health from subscriber health, and supplies causal evidence for any later serving-gate decision.

Related issue

None found. The nearest open Redis result is unrelated subscription-close lifecycle work (#2382).

Testing

  • Deployed exact source 5a76bf65284ccb7dcdbe5e25c7c736511a31c666 as multi-architecture image dev-sha-5a76bf65284ccb7dcdbe5e25c7c736511a31c666-run-34413030691-1 (sha256:a50656c3251c7f75a8f5c50715d13239707e6f31c2bfa85742e50693aa79f0d9) through staging GitOps PR #399.
  • Verified both desired staging replicas are Kubernetes-ready in ReplicaSet buzz-fcf5d5d96, run the exact image, report all_ready=1 and paths_ready=3, and expose the complete 30-series one-hot state matrix.
  • Verified all six pod/path handshakes completed in 0.212–0.231s; current not-ready duration and consecutive failure depth are 0 for every path.
  • Added, validated, and read back the five-state, timing/retry-depth, attempt-activity, and bounded-transition panels in Row 7 of the staging-filtered dashboard. The fail-closed fleet formula returned 100% and does not use default_zero().

Generated with Codex

Co-authored-by: Ravneet Arora <rarora@squareup.com>
Signed-off-by: Ravneet Arora <rarora@squareup.com>
@github-actions

github-actions Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is cec5c8fd9280d30f56effac701e1e19d5cfe6fea...b8787bf5d74390d99f25c8837620ba555e9e6758.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review b8787bf5d74390d99f25c8837620ba555e9e6758 to authorize a new review.
Any previous review applies only to its recorded range.

Expose active retry attempts, readiness duration, state age, and consecutive failures for each fixed Redis subscription path.

Co-authored-by: Ravneet Arora <rarora@squareup.com>
Signed-off-by: Ravneet Arora <rarora@squareup.com>
Signed-off-by: Ravneet Arora <rarora@squareup.com>
@ravarora2
ravarora2 marked this pull request as ready for review September 10, 2026 17:26
@ravarora2
ravarora2 requested a review from a team as a code owner September 10, 2026 17:26

@wpfleger96 wpfleger96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Thanks for this. Owning all six tasks in one runtime, with cancellation-aware backoff everywhere, is a real improvement over the fire-and-forget spawns in main.rs. The label sets are properly bounded, and having publish and refresh share one lock avoids stale-snapshot overwrites. I think two things need to change before this matches the contract in the description, plus one smaller issue.

1. The event path reports ready without a subscription handshake on a normal startup. In subscriber.rs#L107-L123, network_ready(Event) runs unconditionally after the initial-topics loop. PubSubManager starts with an empty desired_topics, so on a fresh pod the loop does nothing and the path goes ready, and counts toward all_ready=1, after only a dial. The description says a path is ready only after the real SUBSCRIBE/PSUBSCRIBE completes. A Redis that accepts connections but rejects SUBSCRIBE would read healthy until the first client asks for a topic. The event roundtrip test hides this branch because it retains a topic before starting the subscriber. I'd either only mark ready after an acknowledged subscribe (the first dynamic one counts), or make "connected, no demand yet" its own explicit state instead of calling it a completed handshake. Either way, add empty-start coverage.

2. The startup-ordering tests don't discriminate the regressions they're named for. production_runtime_owns_both_halves_and_attaches_consumers_first (redis_subscription_runtime.rs#L279-L304) checks consumer_attached only after start() returns synchronously, on a current-thread runtime, against unreachable Redis. If the network spawns moved above the receiver subscriptions, it would still pass: no spawned task runs until the test yields, and nothing is ever delivered anyway. It also calls start() directly, so deleting the RedisSubscriptionRuntime::start call in main.rs wouldn't fail anything. The newly selected boot_lifecycle success case stops at metrics init with a fake DB, before the runtime starts. Per the "Regression tests must bind the production seam and be falsifiable" rule in AGENTS.md, this needs to be a test that fails when the ordering is reversed or startup is omitted. For example, record attachment state at the point the network future first runs, and/or run a real delivery through the production consumers on the Redis lane.

3. (smaller) A panic during shutdown gets reported as a clean stop. spawn_owned checks cancel.is_cancelled() before looking at catch_unwind's result (L167-L178). Once shutdown() cancels, an in-flight panic returns silently, and the path then gets marked stopped instead of failed/panic. Cancellation explains a normal return, not a panic. I'd latch Panic regardless of the token and only suppress normal returns after cancel.

Also, this is conflicting with main now (Justfile, scripts/run-tests.sh, crates/buzz-relay/src/metrics.rs), and the last CI run on this head is from 09-10, so it'll need a rebase and a fresh run.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants