From 9ade061e803bd44b554cb78e77f566e7cb893db8 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:44:58 +0100 Subject: [PATCH 01/34] Investigate Redis Enterprise lag-aware availability checks Confirms the premise: this is the client-side geographic failover feature family, and Lettuce, Jedis and redis-py have all shipped it. It feeds exactly the health checks our Availability/ namespace already has behind SER007. The abstractions line up almost one-to-one with redis-py's - probe, policy (all/any/majority), circuit breaker, per-member config - and two details make it land cleanly: HealthCheckProbe has no internal abstract members, so it is already subclassable from outside the assembly, and ConnectionGroupMember already carries a per-member HealthCheck override, which is what a per-cluster REST endpoint needs. Where we are better placed: HealthCheckResult has Inconclusive, which Lettuce's two-state HealthStatus lacks. Lettuce maps a failed REST call to UNHEALTHY, so a management-plane outage is indistinguishable from the data plane being down and can fail over a database that is serving fine. We can say "could not determine" instead. The awkward part is that src/ has no HTTP client and no JSON parser. JSON turns out to be avoidable if the bdb uid is configured rather than discovered, since the availability call is a status-code check. Also records the defaults disagreeing across clients: 100ms everywhere - server, docs, redis-py - but 5000ms in Lettuce. --- notes/lag-aware/findings.md | 303 ++++++++++++++++++++++++++++++++++++ notes/readme.md | 24 +++ 2 files changed, 327 insertions(+) create mode 100644 notes/lag-aware/findings.md create mode 100644 notes/readme.md diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md new file mode 100644 index 000000000..ea90ab00d --- /dev/null +++ b/notes/lag-aware/findings.md @@ -0,0 +1,303 @@ +# Lag-aware availability checks + +Investigation, September 2026. Does Redis Enterprise's lag-aware database-availability API belong in +our health checks for geo-redundant failover, and if so, where? + +Short answer: **yes, it is the same feature every other Redis client has already shipped, and our +abstractions already fit it — but it needs HTTP and JSON, which `src/` has neither of, and it does +not need to live in `StackExchange.Redis` at all.** + +## 1. What the feature is + +Redis Enterprise (Redis Software) exposes a +[database availability API](https://redis.io/docs/latest/operate/rs/monitoring/db-availability/) for +load balancers and monitoring: + +```text +GET /v1/bdbs/{uid}/availability # whole database +GET /v1/local/bdbs/{uid}/endpoint/availability # this node's local endpoint, no redirect +``` + +`200 OK` means available. An endpoint counts as available only if the database's primary shards are +reachable *and* the endpoint's listener port is bound. With the OSS Cluster API enabled, the +database form verifies **all** endpoints; otherwise at least one. + +The **lag-aware** extension is what makes this interesting for failover, as opposed to a liveness +probe: + +```text +GET /v1/bdbs/{uid}/availability?extend_check=lag +GET /v1/bdbs/{uid}/availability?extend_check=lag&availability_lag_tolerance_ms=100 +``` + +`extend_check=lag` additionally asks whether a replica is *sufficiently synchronised with the +primary* to be a safe failover/failback target. The threshold is cluster-wide, default **100 ms**, +changed with: + +```text +PUT /v1/cluster { "availability_lag_tolerance_ms": 100 } +``` + +and overridable per request via the query parameter. The REST docs say "Recommended value: 100 +milliseconds". + +The purpose is explicitly disaster recovery: *"reduce the risk of data inconsistencies during +disaster recovery by … ensuring failover-failback flows only occur when databases are accessible and +sufficiently synchronized."* This is the check that stops you failing **back** onto a region that is +reachable but stale. + +### Response detail + +| status | meaning | +| --- | --- | +| 200 | available | +| 400 | invalid schema | +| 404 | database not found | +| 503 | unavailable, or no quorum | + +Non-200 returns a JSON body with `error_code` and `description`: + +| form | `error_code` values | +| --- | --- | +| database | `no_quorum`, `db_not_found`, `bdb_unavailable` — suffixed `_shard_unreachable` and/or `_port_unbound`, e.g. `bdb_unavailable_shard_unreachable_port_unbound` | +| endpoint | `no_quorum`, `db_not_found`, `bdb_endpoint_unavailable` | + +Authentication is HTTP Basic against the **cluster REST API** (default port 9443), requiring the +`view_bdb_info` permission — roles `admin`, `cluster_member`, `cluster_viewer`, `db_member`, +`db_viewer`, `user_manager`. Note these are *cluster management* credentials, unrelated to the Redis +credentials in `ConfigurationOptions`. + +Documented known issue: **RS155734** — endpoint availability metrics are miscalculated. + +## 2. The reference implementation + +[`LagAwareStrategy`](https://github.com/redis/lettuce/blob/main/src/main/java/io/lettuce/core/failover/health/LagAwareStrategy.java) +in Lettuce implements `HealthCheckStrategy`, whose surface is `getInterval()`, `getTimeout()`, +`getNumProbes()`, `getPolicy()`, `getDelayInBetweenProbes()`, `doHealthCheck(RedisURI)` and +`close()`. + +`doHealthCheck` is short: + +1. If no cached bdb id: `GET /v1/bdbs?fields=uid,endpoints`, find the first bdb whose endpoints match + the connection's host, cache its `uid`. No match → log and throw. +2. `GET /v1/bdbs/{uid}/availability`, with `extend_check=lag` and + `availability_lag_tolerance_ms` when extended checking is on. +3. 200 → `HEALTHY`, anything else → `UNHEALTHY`. +4. On REST failure → **invalidate the cached bdb id** and return `UNHEALTHY`. + +Config (`LagAwareStrategy.Config`): `restEndpoint` (URI), `credentialsSupplier` +(`Supplier` — so credentials can rotate), `sslOptions`, `availabilityLagTolerance`, +`extendedCheckEnabled`. Convenience factories `databaseAvailability(...)` (plain), +`lagAware(...)`, `lagAwareWithTolerance(...)`. + +Two defaults worth noting: `EXTENDED_CHECK_DEFAULT = true` (lag-aware is on by default), and +`AVAILABILITY_LAG_TOLERANCE_DEFAULT = Duration.ofMillis(5000)`. + +## 3. Every client has this, and the defaults disagree + +This is not a Java curiosity — it is the client-side geographic failover feature family that Redis +has been rolling out across clients: + +| client | type | lag tolerance default | +| --- | --- | --- | +| Lettuce | `LagAwareStrategy` | **5000 ms** | +| Jedis | `LagAwareStrategy` | (same family) | +| redis-py | `LagAwareHealthCheck` | **100 ms** | +| Redis Enterprise cluster | `availability_lag_tolerance_ms` | **100 ms** | +| REST API docs | "Recommended value" | **100 ms** | + +**Lettuce is fifty times more permissive than everyone else, including the server's own default and +its own documentation's recommendation.** That is either a deliberate call about real-world WAN +replication lag or an oversight; either way we should not copy a number without deciding it, and it +is worth asking Redis which is intended. 100 ms across a geo-replicated WAN link looks optimistic; +5 s looks like it would permit a failback that loses seconds of writes. + +redis-py's other defaults are useful reference points: `rest_api_port` 9443, `verify_tls` True, with +`auth_basic`, `ca_file`, `client_cert_file` and `client_key_file` — i.e. mTLS is supported, not just +Basic. + +## 4. It fits what we already have — almost exactly + +`src/StackExchange.Redis/Availability/` already implements this shape, gated behind +`[Experimental(Experiments.GeoRedundantFailover)]` (`SER007`). The correspondence with redis-py's +model is close to one-to-one: + +| concept | StackExchange.Redis | redis-py | Lettuce | +| --- | --- | --- | --- | +| probe abstraction | `HealthCheckProbe.CheckHealthAsync(HealthCheckContext)` | `AbstractHealthCheck.check_health()` | `HealthCheckStrategy.doHealthCheck(RedisURI)` | +| default probe | `HealthCheckProbe.Ping` | `PingHealthCheck` | ping strategy | +| aggregation policy | `HealthCheckProbePolicy.AllSuccess` / `AnySuccess` / `MajoritySuccess` | `HEALTHY_ALL` / `HEALTHY_ANY` / `HEALTHY_MAJORITY` | `ProbingPolicy` | +| probe count / timeout / interval | `HealthCheck.ProbeCount` / `.ProbeTimeout` / `.ProbeInterval` | `MultiDbConfig` | `getNumProbes()` / `getTimeout()` / `getInterval()` | +| failure latch | `CircuitBreaker` | circuit breaker OPEN | — | +| failover unit | `ConnectionGroupMember` | database in multi-db config | — | +| failback damping | `MultiGroupOptions.FailbackDelay` | weighted selection | — | + +Two details that make a lag-aware probe land cleanly: + +- **`HealthCheckProbe` is externally subclassable today.** It is a `public abstract partial class` + with one `public abstract` method and *no* internal or `private protected` abstract members — + checked. Anyone can implement a probe outside this assembly. The only in-assembly conveniences are + the memoised `protected internal` result tasks, which an external probe can trivially do without. +- **Health checks can already be configured per member.** `ConnectionGroupMember` carries nullable + per-member overrides of the group-wide `MultiGroupOptions`, health check included + (`ResolveHealthCheck(options) => HealthCheck ?? options.HealthCheck`). That matters, because each + geo member is a *different* Redis Enterprise cluster with its own REST endpoint, its own + credentials and its own bdb uid — so per-member probe configuration is exactly the shape needed, + and it already exists. + +### Where we are better placed than Lettuce + +`HealthCheckResult` has three states — `Healthy`, `Unhealthy`, **`Inconclusive`** — where Lettuce's +`HealthStatus` has two. + +That difference is more than cosmetic here. Lettuce maps *"the REST call failed"* to `UNHEALTHY`, +which means a Redis Enterprise **management-plane** outage (REST API down, credentials expired, +9443 firewalled) is indistinguishable from the **data plane** being unavailable — and can therefore +trigger a failover of a database that is serving traffic perfectly well. With `Inconclusive` we can +say "I could not determine this" and let the policy decide, which is the honest answer and the safer +one. `KeyWriteHealthCheckProbe` already uses `Inconclusive` this way for replicas. + +This is the single most valuable thing we could do differently, and it should be a deliberate, +documented deviation rather than an accident. + +## 5. The awkward part: HTTP and JSON + +`src/` contains **no HTTP client and no JSON parser** — checked across `StackExchange.Redis` and +`RESPite`; the only matches are in `obj/` transitive restore graphs. A lag-aware probe needs both: + +- **HTTP.** `System.Net.Http` is in-box on `net8.0`/`net10.0` and available on `net472` / + `netstandard2.0`. Manageable, but it is a new dependency class for this library, and it drags in + TLS configuration, proxy behaviour, timeouts and connection pooling as things we would own. +- **JSON.** Needed only for `GET /v1/bdbs?fields=uid,endpoints` discovery and for parsing + `error_code` out of failure bodies. `System.Text.Json` would be a new package reference on + `net472`/`netstandard2.0`. + +**JSON is avoidable on the hot path.** The availability call itself is a *status code* check — 200 +versus not-200 — with no body parsing required for the healthy case. If the bdb uid is supplied by +configuration rather than discovered, the required path needs no JSON at all, and `error_code` can be +surfaced as an opaque string for diagnostics. Discovery-by-host (Lettuce's step 1) is the only part +that genuinely needs a parser, and it is optional convenience: anyone configuring a REST endpoint, +credentials and a tolerance already knows their database. + +That suggests an explicit-uid-first design, with discovery as a later, optional extra. + +## 6. Options + +**A — in `StackExchange.Redis`.** Ships a `HealthCheckProbe.LagAware(...)` alongside `Ping` and +`StringSet`. Most discoverable; matches what Lettuce/Jedis/redis-py do (all in-box). Costs the core +package an `HttpClient` dependency, plus TLS/proxy/credential surface, for a feature that only +applies to one commercial deployment target. + +**B — a separate package.** `StackExchange.Redis.Enterprise` (or similar) subclassing the *already +public* `HealthCheckProbe`. **This requires no change to `StackExchange.Redis` whatsoever** — the +extension point exists and is sufficient. Keeps HTTP, JSON, TLS and cluster credentials out of the +core package and lets the Enterprise-specific bits version on their own cadence. + +**C — do nothing, and document.** The extension point is public; users targeting Redis Enterprise +can write a ~50-line probe. Cheapest, and the least good discovery story for the people who most +need it. + +B looks strongest on the evidence: the feature is deployment-specific, the dependency is real, and — +unlike the OpenTelemetry case, where a second assembly would have needed internals it could not have +— the hook here is *already public and already sufficient*. Worth confirming that nothing in +`HealthCheckContext` is missing for a real implementation before committing to it. + +## 7. Testing: the toy server can spoof this, and already has the HTTP half + +`toys/KestrelRedisServer` **already listens on HTTP**. From `Program.cs`, one Kestrel host serves +both planes over the same `RedisServer` singleton: + +```csharp +options.ListenLocalhost(5000); // "HTTP 5000 (test/debug API only)" +options.ListenLocalhost(ip.Port, b => b.UseConnectionHandler()); // RESP 6379 +``` + +with a single catch-all route today (`app.Run(ctx => ctx.Response.WriteAsync(server.GetStats()))`). +Adding `/v1/bdbs/{uid}/availability` beside that is a handful of lines, and — because the route +closes over the *same* `RedisServer` instance the client is talking RESP to — the fake management +plane can be made to lie **coherently with** the data plane rather than independently of it. + +Spoofing infrastructure for test purposes is also already the established intent of this toy; the +file carries a commented-out block headed *"demonstrate cluster spoofing"* that flips +`ServerType` to `Cluster`, adds an empty node and migrates a slot. A fake Redis Enterprise +management plane is the same idea applied to a different API. + +What that buys is the ability to script the scenarios that decide whether the failover logic is +correct, none of which can be produced on demand against a real cluster: + +- healthy (200) +- `503 bdb_unavailable_shard_unreachable`, `503 no_quorum`, `404 db_not_found` +- **200 for the plain check, 503 for `extend_check=lag`** — reachable but stale. *This asymmetry is + the entire point of the feature*, and inducing genuine cross-region replication lag to order is not + a thing anyone can do in CI. +- slow responses, to exercise `HealthCheck.ProbeTimeout` +- 401/403, to exercise credential rotation and — importantly — to check we return `Inconclusive` + rather than `Unhealthy` when the management plane is the thing that is broken (§4) + +### Two levels, and a design consequence + +The existing test harness points at the right answer for the cheap level — but the two planes are +**not** symmetric, and this is the thing to get right. + +`InProcessTestServer` derives from `MemoryCacheRedisServer` and sets `Tunnel = new InProcTunnel(this)`. +It **does not run as a network server at all**: there is no listener, no port, no socket. The client +reaches it because `ConfigurationOptions.Tunnel` replaces the transport wholesale, so "connecting" is +a method call. That is what makes those tests fast and deterministic. + +This is not a hack bolted on for tests — it is a first-class, public, documented extension point. +`StackExchange.Redis.Configuration.Tunnel` is a `public abstract class` whose +`ConnectTransportAsync(...)` returns a `RESPite.Transports.DuplexTransport`, i.e. an implementation +**hijacks the transport layer completely**; the shipped `Tunnel.HttpProxy(...)` does the same thing +for CONNECT proxying. `InProcTunnel` is simply another implementation of that same public seam. + +There is no such seam on the HTTP side. `HttpClient` resolves a URL and opens a socket, so **you +cannot point a lag-aware probe at the in-process server — there is nothing there to point at.** + +So the model to copy is the one the library already has: hijack the HTTP transport completely, the +same way `Tunnel` hijacks the RESP one. In .NET that means an injectable `HttpMessageHandler` (or our +own abstraction over it), which is the exact analogue — `HttpMessageHandler` *is* the HTTP transport, +and substituting it answers requests in-process with no listener, no port and no TLS. + +Hence the sharper version of the design consequence below: a configurable *base URL* is not enough. +If the probe's only seam is a `Uri`, every test needs a real listener on a real port, and the cheap +in-process level is simply unavailable. Lettuce evidently hit the same wall — its second constructor, +`LagAwareStrategy(Config, HttpClient)`, exists so the transport can be substituted. + +`toys/KestrelRedisServer` then covers the other level: a real, out-of-process, full-fidelity fake with +actual sockets on both planes, for manual exercise and end-to-end runs. Note that this is also the +only level where the *whole* thing is exercised, because it is the only place both planes are real. + +The combination is what makes this worth doing: **in-process RESP fakes for several members, plus a +stubbed management plane per member, is a complete geo-redundant failover test with no external +infrastructure at all** — and geo-redundant failover is otherwise close to untestable, which is +presumably why it is still behind `SER007`. + +The design consequence: **whatever we build must be able to hijack the HTTP transport completely, +from day one** — an injectable `HttpMessageHandler`-shaped seam, not merely a configurable base URL. +That is a requirement on the API shape, it falls out of testability rather than taste, and it is the +same decision the library already made for RESP with `Tunnel`. It also argues mildly against option +C — "document the extension point and let users write it" leaves us with no test coverage of a +failover path we ship. + +## 8. Open questions + +1. **Which tolerance default?** 100 ms (server, docs, redis-py) or 5000 ms (Lettuce)? Worth asking + Redis directly; the divergence looks unintentional. +2. **`Unhealthy` or `Inconclusive` when the REST call fails?** See §4. Recommend `Inconclusive`, but + note that this makes us behave differently from every other client, so it needs to be a stated + choice. +3. **Which endpoint — `/v1/bdbs/{uid}/availability` or `/v1/local/bdbs/{uid}/endpoint/availability`?** + Lettuce uses the former. The latter does not redirect to the primary node, which may be the more + accurate signal when probing a specific endpoint, and is what the docs recommend for load + balancers under the `all-nodes` proxy policy. Note known issue RS155734 against the endpoint form. +4. **Explicit bdb uid, discovery, or both?** Explicit avoids JSON entirely (§5). +5. **Is `HealthCheckContext` sufficient?** It carries `IServer` and `ProbeTimeout` only. A probe + instance can hold its own REST configuration given per-member health checks, but if a single probe + instance is ever shared across members it would need to map endpoint → configuration. Worth + checking against a real implementation. +6. **Credentials and rotation.** Lettuce takes a `Supplier` so credentials can + rotate; redis-py supports Basic plus mTLS. Whatever we do should not bake in a static password. +7. **How is this tested?** Largely answered in §7 — stub the HTTP seam for unit work, extend + `toys/KestrelRedisServer` (which already serves HTTP on 5000) for end-to-end. What remains open is + whether we also want *any* coverage against a real Redis Enterprise cluster, which the docker + compose topology in `tests/RedisConfigs` does not and realistically cannot provide. diff --git a/notes/readme.md b/notes/readme.md new file mode 100644 index 000000000..b51960340 --- /dev/null +++ b/notes/readme.md @@ -0,0 +1,24 @@ +# Working notes + +This directory holds working material for whoever picks a topic up next: investigations, the +reasoning behind a decision, and snapshots that were expensive to produce and would be expensive +to produce again. + +The split to keep: + +- `docs/` — documentation for people *using* StackExchange.Redis. It is a Jekyll site + (`docs/_config.yml`, published at ), so anything placed there is built into + the site *and* listed in `sitemap.xml`. +- `notes/` — internal handover material. Not built, not published, not indexed. + +Notes are grouped by topic in a subdirectory, and the files inside are named for their content +rather than repeating the topic — `notes/lag-aware/findings.md`, not +`notes/lag-aware/lag-aware-findings.md`. + +When something in here graduates into advice for users, write it up in `docs/` and leave the note +as the record of *why*. + +## Topics + +- [`lag-aware/`](lag-aware/) — Redis Enterprise lag-aware availability checks, and whether they + belong in the geo-redundant failover health checks. From 4d5a2398c41702055414609101b4bbbaad4e5edc Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:45:42 +0100 Subject: [PATCH 02/34] Record what a Tunnel API for the management plane should look like mgravell offered to add one; the shape is what decides whether it helps. A member returning HttpMessageHandler would drag System.Net.Http into StackExchange.Redis's public API for the sake of one commercial deployment target - which is the thing keeping this in a separate package was meant to avoid, decided the expensive way. A stream-shaped member instead composes into SocketsHttpHandler.ConnectCallback, which takes exactly a ValueTask, so the Enterprise side gets a complete HTTP hijack and core gains no HTTP types at all. Tunnel already speaks that language via BeforeAuthenticateAsync and ConnectTransportAsync, and adding a virtual is binary-safe for subclasses. Two functional arguments for Tunnel over a separate knob, beyond testing: a CONNECT proxy configured for Redis almost certainly applies to the Enterprise REST API too, so co-locating makes that correct by construction; and the test harness already assigns Tunnel, so one assignment would spoof both planes. Caveats recorded: ConnectCallback is net5+, the existing members are EndPoint/ConnectionType-shaped where HTTP wants a Uri, and widening Tunnel's remit means HttpProxyTunnel and LoggingTunnel each need an opinion about whether they apply to the management plane. Also retires open question 5: RawConfig and ConfigurationOptions.Tunnel are both already public, so a probe reaches the tunnel from the context it already gets - no HealthCheckContext change needed. --- notes/lag-aware/findings.md | 87 ++++++++++++++++++++++++++++++++++--- 1 file changed, 82 insertions(+), 5 deletions(-) diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index ea90ab00d..d03578c01 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -279,7 +279,83 @@ same decision the library already made for RESP with `Tunnel`. It also argues mi C — "document the extension point and let users write it" leaves us with no test coverage of a failover path we ship. -## 8. Open questions +## 8. If `Tunnel` gains an API for this + +mgravell has offered to add one. It is a good fit — but the *shape* decides whether it helps or +quietly undoes §6. + +### Two functional arguments for `Tunnel` specifically + +1. **The proxy case is already right.** If Redis traffic goes through a CONNECT proxy + (`Tunnel.HttpProxy(...)`), the Redis Enterprise REST API is almost certainly behind the same + corporate proxy. Hanging the management-plane transport off the same object means that case works + by construction instead of needing a second, parallel proxy setting that users must remember to + keep in sync. +2. **One assignment configures both planes in tests.** `InProcessTestServer` already sets + `Tunnel = new InProcTunnel(this)`. If the HTTP seam rides on the same object, the harness gets + management-plane spoofing everywhere it already gets data-plane spoofing, with no new wiring. + +### The shape that would be a mistake + +```csharp +// don't +public virtual HttpMessageHandler? CreateHttpMessageHandler(Uri restEndpoint); +``` + +That puts `System.Net.Http` types into **StackExchange.Redis's public API**, giving the core package +a hard dependency for the sake of one commercial deployment target — exactly what option B exists to +avoid. It would make the core-vs-package question moot by deciding it the expensive way. + +### The shape that works + +Stay transport-neutral. `Tunnel` yields a `Stream` (or `DuplexTransport`) for an endpoint, and the +Enterprise-side package composes that into an HTTP stack itself: + +```csharp +// core: no System.Net.Http anywhere +public virtual ValueTask ConnectManagementStreamAsync( + EndPoint endpoint, CancellationToken cancellationToken) => default; + +// Enterprise package: hand it to SocketsHttpHandler +new SocketsHttpHandler +{ + ConnectCallback = async (ctx, ct) => + await tunnel.ConnectManagementStreamAsync(Resolve(ctx.DnsEndPoint), ct) + ?? await DefaultConnectAsync(ctx, ct), +}; +``` + +`SocketsHttpHandler.ConnectCallback` is `Func>` — a `Stream` is precisely the currency needed, so a stream-shaped `Tunnel` member +composes into a complete HTTP hijack with no HTTP types in core at all. `Tunnel` already speaks this +language: `BeforeAuthenticateAsync` returns `ValueTask` and `ConnectTransportAsync` returns +`ValueTask` (itself public, gated `SER009`). + +Adding a **virtual** member to a public abstract class is source- and binary-safe for existing +subclasses, so this is additive in the sense AGENTS.md requires. + +### Caveats worth stating + +- **`ConnectCallback` is net5.0+.** There is no equivalent on `HttpClientHandler`/`WinHttpHandler`, + so a down-level implementation would need direct handler injection instead. An Enterprise package + targeting `net8.0`+ sidesteps this entirely, and is defensible for a new deployment-specific + feature. +- **Signature mismatch.** Existing `Tunnel` members are RESP-connection-shaped (`EndPoint` plus + `ConnectionType`). HTTP wants scheme, host, port and TLS — i.e. a `Uri`. Either shape is workable; + neither is free of a little awkwardness. +- **It widens `Tunnel`'s remit** from "the Redis connection" to "transports this library initiates", + which the existing subclasses then have to have an opinion about. `HttpProxyTunnel` almost + certainly *should* apply to the management plane; `LoggingTunnel` probably should *not* start + logging REST traffic by default. Both need deciding rather than falling out. + +### One question this already answers + +Open question 5 below asked whether `HealthCheckContext` carries enough for a real probe. It does: +`IConnectionMultiplexer.RawConfig` and `ConfigurationOptions.Tunnel` are **both public**, so an +external probe can already reach `context.Server.Multiplexer.RawConfig.Tunnel` with no change to +`HealthCheckContext` at all. + +## 9. Open questions 1. **Which tolerance default?** 100 ms (server, docs, redis-py) or 5000 ms (Lettuce)? Worth asking Redis directly; the divergence looks unintentional. @@ -291,10 +367,11 @@ failover path we ship. accurate signal when probing a specific endpoint, and is what the docs recommend for load balancers under the `all-nodes` proxy policy. Note known issue RS155734 against the endpoint form. 4. **Explicit bdb uid, discovery, or both?** Explicit avoids JSON entirely (§5). -5. **Is `HealthCheckContext` sufficient?** It carries `IServer` and `ProbeTimeout` only. A probe - instance can hold its own REST configuration given per-member health checks, but if a single probe - instance is ever shared across members it would need to map endpoint → configuration. Worth - checking against a real implementation. +5. ~~**Is `HealthCheckContext` sufficient?**~~ Answered in §8 — `RawConfig` and + `ConfigurationOptions.Tunnel` are both public, so a probe can reach the tunnel from the context it + already gets. What remains: if a single probe instance is ever shared across members it still + needs to map endpoint → REST configuration, which per-member health checks make unnecessary but do + not forbid. 6. **Credentials and rotation.** Lettuce takes a `Supplier` so credentials can rotate; redis-py supports Basic plus mTLS. Whatever we do should not bake in a static password. 7. **How is this tested?** Largely answered in §7 — stub the HTTP seam for unit work, extend From e9014f01dd6986cc8af6584b898b3d221d33e1d9 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:47:06 +0100 Subject: [PATCH 03/34] Real clusters and the fault injector, and a correction I had written that inducing "reachable but stale" on demand is not something anyone can do in CI, so the fakes were the only route to the scenario that matters. That is wrong. Redis ships a fault injection service that the client suites drive over HTTP - POST /action with a type and parameters, poll the returned action_id - and Lettuce's ActiveActiveFailoverScenarioTest injects network_latency against a bdb_id for a bounded duration, which is exactly the mechanism for pushing replication lag past the tolerance and watching the lag-aware check flip while the plain check stays green. So the authoritative end-to-end test is real cluster plus fault injector, on a road Lettuce has already built. The fakes are demoted rather than removed: they still cover machines with no Enterprise cluster, they are far faster than an injector action that budgets minutes plus a stabilisation wait, and they cover what the injector does not obviously reach - specific error_code bodies, 401/403 credential expiry, slow-but-successful responses for probe timeouts. Also records what the real clusters are uniquely good for: everything in section 1 is read off documentation, and a short session settles whether extend_check=lag is actually supported, what failure bodies really contain, whether the Host header is required, what the bdbs listing really returns - which decides whether the JSON-avoidance argument holds - and what lag real geo links show, which is the empirical way to settle 100ms versus 5000ms. --- notes/lag-aware/findings.md | 86 ++++++++++++++++++++++++++++++++++--- 1 file changed, 80 insertions(+), 6 deletions(-) diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index d03578c01..b6d303711 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -228,8 +228,8 @@ correct, none of which can be produced on demand against a real cluster: - healthy (200) - `503 bdb_unavailable_shard_unreachable`, `503 no_quorum`, `404 db_not_found` - **200 for the plain check, 503 for `extend_check=lag`** — reachable but stale. *This asymmetry is - the entire point of the feature*, and inducing genuine cross-region replication lag to order is not - a thing anyone can do in CI. + the entire point of the feature.* (See the fault injector below: this one **can** also be produced + against a real cluster, contrary to what one would assume.) - slow responses, to exercise `HealthCheck.ProbeTimeout` - 401/403, to exercise credential rotation and — importantly — to check we return `Inconclusive` rather than `Unhealthy` when the management plane is the thing that is broken (§4) @@ -272,6 +272,76 @@ stubbed management plane per member, is a complete geo-redundant failover test w infrastructure at all** — and geo-redundant failover is otherwise close to untestable, which is presumably why it is still behind `SER007`. +### Real servers are available, and they answer different questions + +mgravell notes we have access to real servers that will support this. That does not replace the +fakes; it does something the fakes cannot, and it should come **first**. + +Everything in §1 of these notes is read off *documentation*, and documentation for a recent feature +is exactly where reality and prose diverge. A short session against a real cluster settles a list of +things currently taken on trust: + +- does the deployed version support `extend_check=lag` at all, and does the + `availability_lag_tolerance_ms` query parameter genuinely override the cluster default? +- what does a failure body actually look like — are the composed codes + (`bdb_unavailable_shard_unreachable_port_unbound`) real, and is `error_code` reliably present? +- is a `Host: cnm.cluster.fqdn` header actually required, as the docs' header table implies? +- what does `GET /v1/bdbs?fields=uid,endpoints` really return? That shape decides whether the + JSON-avoidance argument in §5 holds, or whether discovery is cheap enough to include after all. +- TLS on 9443: self-signed by default? That decides whether we need redis-py's `ca_file` / + `client_cert_file` / `verify_tls` surface or can rely on the ambient trust store. +- **what lag do real geo-replicated links actually show?** This is the empirical way to settle + 100 ms versus Lettuce's 5000 ms (§3), and it is a much better argument than reading either + default off a page. + +The two then compose rather than compete, and the synthesis is the useful bit: **use the real +cluster to capture responses, and the fakes to replay them.** A recorded set of genuine 200/503/404 +bodies baked into the stub means the deterministic tests stop guessing at the wire format. + +### The fault injector — correcting the assumption above + +I had assumed a real cluster could not be made to show "reachable but stale" on demand. **That is +wrong**, and it is worth knowing before planning any of this: Redis ships a **fault injection +service** that the client test suites drive over HTTP, precisely so failover behaviour can be tested +against real Redis Enterprise deployments. + +From Lettuce's `src/test/java/io/lettuce/scenario/FaultInjectionClient.java`: + +- base URL `http://127.0.0.1:20324` by default, overridable with `FAULT_INJECTION_API_URL` +- `POST /action` with `{ "type": , "parameters": { … } }` → `{ "action_id": … }`, then poll + that id until the action completes +- generous timeouts by category — up to 5 minutes for migrations and failovers — plus a + stabilisation delay afterwards + +Action types visible in their suite: + +| action | parameters | used for | +| --- | --- | --- | +| `network_latency` | `bdb_id`, `delay_ms`, `duration` | **Active-Active failover scenarios** | +| `network_failure` | endpoint | reconnection / pub-sub recovery | +| `dmc_restart` | endpoint | reconnection / pub-sub recovery | + +`ActiveActiveFailoverScenarioTest` injects `network_latency` against the primary's `bdb_id` for a +bounded duration — which is exactly the mechanism for driving replication lag past the tolerance and +watching the lag-aware check flip while the plain check stays green. So the authoritative end-to-end +test for this feature is real cluster + fault injector, and it is a road Lettuce has already built. + +That demotes the fakes but does not remove them. They remain worth having for: CI and dev machines +with no Enterprise cluster; speed (the injector's failover actions budget minutes, and there is a +10-second stabilisation wait); and the cases the injector does not obviously cover — specific +`error_code` bodies, 401/403 credential expiry, and slow-but-successful responses for probe-timeout +behaviour. + +Their scenario tests also follow the same gating shape we already use: endpoints come from +configuration (`Endpoints.DEFAULT.getEndpoint("re-standalone")`) with `assumeTrue(... != null, +"Skipping test because no Redis endpoint is configured!")`, which is our `Skip.IfNoServer` pattern +under a different name. + +Gating is a solved problem here: `tests/.../Helpers/Skip.cs` already has `IfNoServer(host, port)`, +`IfNoCluster()`, `IfNoFailoverPair()` and `UnlessLongRunning()`, and `TestConfig` carries per-role +host/port settings, so an Enterprise-backed test suite follows an established pattern and skips as +inconclusive wherever the cluster is absent — which is everywhere except the machines that have one. + The design consequence: **whatever we build must be able to hijack the HTTP transport completely, from day one** — an injectable `HttpMessageHandler`-shaped seam, not merely a configurable base URL. That is a requirement on the API shape, it falls out of testability rather than taste, and it is the @@ -374,7 +444,11 @@ external probe can already reach `context.Server.Multiplexer.RawConfig.Tunnel` w not forbid. 6. **Credentials and rotation.** Lettuce takes a `Supplier` so credentials can rotate; redis-py supports Basic plus mTLS. Whatever we do should not bake in a static password. -7. **How is this tested?** Largely answered in §7 — stub the HTTP seam for unit work, extend - `toys/KestrelRedisServer` (which already serves HTTP on 5000) for end-to-end. What remains open is - whether we also want *any* coverage against a real Redis Enterprise cluster, which the docker - compose topology in `tests/RedisConfigs` does not and realistically cannot provide. +7. **How is this tested?** Answered in §7: real cluster plus Redis's **fault injection service** is + the authoritative path — `network_latency` against a `bdb_id` drives real replication lag past + the tolerance, which is how Lettuce tests Active-Active failover — with stubbed HTTP transport + for the deterministic/CI tier and `toys/KestrelRedisServer` (already serving HTTP on 5000) in + between. Open: whether we stand up a fault-injector-backed scenario suite at all, who owns it, + and whether the injector is available to us alongside the clusters. The real-cluster validation + in any case wants doing *before* code, since several §1 facts are documentation rather than + observation. From 8120673d4d1d64335ef274b82a24d940f04e3452 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:49:34 +0100 Subject: [PATCH 04/34] We already have a fault-injector tier; it answers several of the open questions It is not merged - tests/StackExchange.Redis.FaultInjector.Tests lives on marc/maint-optin-server, open as #3191 (107 files, +13160) - which is presumably why I did not find it when I grepped a branch taken fresh from main. Two pieces of it bear directly on this. FaultInjectorClient wraps the same POST /action + GET /action/{id} contract go-redis, redis-py and node-redis converged on, and adds GetValidTriggersAsync, so we can ask a deployment which lag-inducing effects it supports instead of guessing at an action name. And ClusterRestClient is already a Redis Enterprise REST client on 9443 - Basic auth, System.Text.Json, GET /v1/bdbs?fields=uid,name - so the availability call is a method on an existing class, not a new project. It also settles by observation what I had listed as open and sourced only from documentation: the management certificate is self-signed per environment and pinned to a CA from the config directory, so the probe needs CA configuration and cannot rely on the ambient trust store; auth really is Basic; and the bdbs listing returns the requested fields, so discovery is easy - which weakens the avoid-JSON argument to "it keeps JSON out of src/", not "discovery is hard". Sequencing consequence recorded: an end-to-end test is downstream of #3191. And #3191's own summary is the best argument for validating before building - same vendor, same class of API: the specifications are prose, the payloads were never published, and nearly every starting assumption was wrong. --- notes/lag-aware/findings.md | 114 ++++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 49 deletions(-) diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index b6d303711..f61f673bb 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -286,10 +286,8 @@ things currently taken on trust: - what does a failure body actually look like — are the composed codes (`bdb_unavailable_shard_unreachable_port_unbound`) real, and is `error_code` reliably present? - is a `Host: cnm.cluster.fqdn` header actually required, as the docs' header table implies? -- what does `GET /v1/bdbs?fields=uid,endpoints` really return? That shape decides whether the - JSON-avoidance argument in §5 holds, or whether discovery is cheap enough to include after all. -- TLS on 9443: self-signed by default? That decides whether we need redis-py's `ca_file` / - `client_cert_file` / `verify_tls` surface or can rely on the ambient trust store. + (`ClusterRestClient` sets no such header and works, so: apparently not — but it only issues + `GET /v1/bdbs`, so worth re-checking on the availability route.) - **what lag do real geo-replicated links actually show?** This is the empirical way to settle 100 ms versus Lettuce's 5000 ms (§3), and it is a much better argument than reading either default off a page. @@ -298,44 +296,59 @@ The two then compose rather than compete, and the synthesis is the useful bit: * cluster to capture responses, and the fakes to replay them.** A recorded set of genuine 200/503/404 bodies baked into the stub means the deterministic tests stop guessing at the wire format. -### The fault injector — correcting the assumption above +### The fault injector — and we already have one I had assumed a real cluster could not be made to show "reachable but stale" on demand. **That is -wrong**, and it is worth knowing before planning any of this: Redis ships a **fault injection -service** that the client test suites drive over HTTP, precisely so failover behaviour can be tested -against real Redis Enterprise deployments. - -From Lettuce's `src/test/java/io/lettuce/scenario/FaultInjectionClient.java`: - -- base URL `http://127.0.0.1:20324` by default, overridable with `FAULT_INJECTION_API_URL` -- `POST /action` with `{ "type": , "parameters": { … } }` → `{ "action_id": … }`, then poll - that id until the action completes -- generous timeouts by category — up to 5 minutes for migrations and failovers — plus a - stabilisation delay afterwards - -Action types visible in their suite: - -| action | parameters | used for | -| --- | --- | --- | -| `network_latency` | `bdb_id`, `delay_ms`, `duration` | **Active-Active failover scenarios** | -| `network_failure` | endpoint | reconnection / pub-sub recovery | -| `dmc_restart` | endpoint | reconnection / pub-sub recovery | - -`ActiveActiveFailoverScenarioTest` injects `network_latency` against the primary's `bdb_id` for a -bounded duration — which is exactly the mechanism for driving replication lag past the tolerance and -watching the lag-aware check flip while the plain check stays green. So the authoritative end-to-end -test for this feature is real cluster + fault injector, and it is a road Lettuce has already built. - -That demotes the fakes but does not remove them. They remain worth having for: CI and dev machines -with no Enterprise cluster; speed (the injector's failover actions budget minutes, and there is a -10-second stabilisation wait); and the cases the injector does not obviously cover — specific -`error_code` bodies, 401/403 credential expiry, and slow-but-successful responses for probe-timeout -behaviour. - -Their scenario tests also follow the same gating shape we already use: endpoints come from -configuration (`Endpoints.DEFAULT.getEndpoint("re-standalone")`) with `assumeTrue(... != null, -"Skipping test because no Redis endpoint is configured!")`, which is our `Skip.IfNoServer` pattern -under a different name. +wrong twice over**: Redis ships a fault injection service for exactly this, *and we already have a +test tier built on it.* + +**`tests/StackExchange.Redis.FaultInjector.Tests`** exists on `marc/maint-optin-server`, open as +**[#3191](https://github.com/StackExchange/StackExchange.Redis/pull/3191)** (107 files, +13,160, +unmerged at the time of writing). Per its README it drives *"a real Redis Enterprise deployment +through the fault injector, and watch[es] how SE.Redis reacts … the tier that can observe what no +in-process fake can: real DNS, real TLS identity, real timing."* Gating is +`SER_FI_CONFIG_DIR` plus an explicit `E2E_SCENARIO_TESTS=true` opt-in, with `FAULT_INJECTION_API_URL` +defaulting to `http://127.0.0.1:20324`. + +Two pieces of it matter directly here. + +**`FaultInjector/FaultInjectorClient.cs`** — `POST /action` returning an id, `GET /action/{id}` to +poll, with its own note that this is *"the same shape go-redis, redis-py and node-redis wrap … +they integrated independently and converged, so the contract is stable"*. It is richer than Lettuce's: +alongside `StartActionAsync` / `RunActionAsync` / `WaitForActionAsync` there is +`GetValidTriggersAsync(scenario, effect, clusterIndex)`, so **we can ask the injector what +lag-inducing effects the deployment actually supports rather than guessing at an action name.** +Lettuce's own Active-Active failover test uses `network_latency` with `bdb_id`, `delay_ms` and +`duration`, which is the obvious candidate to look for. + +**`Environment/ClusterRestClient.cs`** — *already a Redis Enterprise REST client*, described as "the +cluster's own REST API on port 9443, for the few facts the fault injector does not expose". It +already does `GET /v1/bdbs?fields=uid,name`, with HTTP Basic auth and `System.Text.Json`. Adding +`GET /v1/bdbs/{uid}/availability?extend_check=lag` beside it is a method, not a project. + +That client also **answers several questions in §9 empirically**, which is worth more than the +documentation they were drawn from: + +- **TLS on 9443 is self-signed per environment.** The client pins to a CA from the config directory + via `ServerCertificateCustomValidationCallback` with `X509ChainTrustMode.CustomRootTrust`, with the + comment *"this channel carries credentials"*. So a lag-aware probe **does** need CA/cert + configuration in the redis-py mould; relying on the ambient trust store will not do. +- **Auth is HTTP Basic**, confirmed against a real cluster rather than inferred from Lettuce. +- **`/v1/bdbs?fields=…` returns an array of objects carrying the requested fields**, so Lettuce's + `fields=uid,endpoints` discovery is straightforward. That weakens the §5 argument a little: skipping + discovery still keeps JSON out of **`src/`**, but "discovery is hard" is not the reason to skip it. + +The fakes are demoted, not removed: they still cover machines with no Enterprise cluster, they are +far faster (injector actions budget minutes, plus a stabilisation wait), and they reach what the +injector does not obviously reach — specific `error_code` bodies, 401/403 credential expiry, and +slow-but-successful responses for probe-timeout behaviour. + +**Sequencing consequence:** an end-to-end lag-aware test is downstream of #3191. That is a real +dependency to plan around, not a detail. + +And #3191's own write-up is the strongest possible support for validating §1 before writing code: +*"the specifications are prose, the payloads were never published, and nearly every assumption I +started with was wrong in some way that mattered."* Same vendor, same class of API, same trap. Gating is a solved problem here: `tests/.../Helpers/Skip.cs` already has `IfNoServer(host, port)`, `IfNoCluster()`, `IfNoFailoverPair()` and `UnlessLongRunning()`, and `TestConfig` carries per-role @@ -443,12 +456,15 @@ external probe can already reach `context.Server.Multiplexer.RawConfig.Tunnel` w needs to map endpoint → REST configuration, which per-member health checks make unnecessary but do not forbid. 6. **Credentials and rotation.** Lettuce takes a `Supplier` so credentials can - rotate; redis-py supports Basic plus mTLS. Whatever we do should not bake in a static password. -7. **How is this tested?** Answered in §7: real cluster plus Redis's **fault injection service** is - the authoritative path — `network_latency` against a `bdb_id` drives real replication lag past - the tolerance, which is how Lettuce tests Active-Active failover — with stubbed HTTP transport - for the deterministic/CI tier and `toys/KestrelRedisServer` (already serving HTTP on 5000) in - between. Open: whether we stand up a fault-injector-backed scenario suite at all, who owns it, - and whether the injector is available to us alongside the clusters. The real-cluster validation - in any case wants doing *before* code, since several §1 facts are documentation rather than - observation. + rotate; redis-py supports Basic plus mTLS; our own `ClusterRestClient` pins a CA because the + management certificate is self-signed per environment (§7). So the probe needs *at least* Basic + plus CA pinning, and should not bake in a static password. +7. ~~**How is this tested?**~~ Largely answered in §7: the tier exists, in #3191. What is genuinely + open is **sequencing and scope** — an end-to-end lag-aware test is downstream of an unmerged + 107-file PR, so either this waits for #3191 or it starts at the stubbed tier and the scenario test + follows. Also open: whether the deployments we have access to expose a lag-inducing effect at all + (ask `GetValidTriggersAsync` rather than assuming `network_latency`). +8. **Does any of this need `src/` changes at all?** On the evidence so far, possibly none: + `HealthCheckProbe` is externally subclassable, `RawConfig`/`Tunnel` are public, and the REST + plumbing precedent lives in the test tier. The `Tunnel` addition in §8 is the one thing that would + have to land in the core package — and only if we want the management plane to honour tunnels. From b9ab0f55f9a38bd989e768839630c69710102d07 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:56:53 +0100 Subject: [PATCH 05/34] Add the plan, blocked on #3191 Step 0 is to wait for #3191 to merge and build from there, rather than writing a second Redis Enterprise REST client against the same cluster and reconciling the two later - its ClusterRestClient already does Basic auth, CA pinning and GET /v1/bdbs, and its FaultInjectorClient can be asked which lag-inducing effects a deployment supports. The one thing that can start earlier is the API validation, because it produces notes rather than code. Step 1 is that validation, before any implementation: a checklist of things currently taken from documentation, including the measurement that settles 100ms versus 5000ms, with captured bodies saved as fixtures so the stubbed tests replay real payloads. Step 2 deliberately does not decide where the code lives, but records that the dependency argument is weaker than it looked - System.Net.Http needs no package reference on any target we ship - so the question is judgement, not constraint. Also records that this branch becomes the feature branch when step 0 clears, and that the way to take the merge is to rebase onto main rather than merge main in, since #3191 squash-merges. --- notes/lag-aware/findings.md | 3 +- notes/lag-aware/plan.md | 156 ++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 notes/lag-aware/plan.md diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index f61f673bb..8fa2380da 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -1,7 +1,8 @@ # Lag-aware availability checks Investigation, September 2026. Does Redis Enterprise's lag-aware database-availability API belong in -our health checks for geo-redundant failover, and if so, where? +our health checks for geo-redundant failover, and if so, where? The sequence that follows from this +is in [`plan.md`](plan.md). Short answer: **yes, it is the same feature every other Redis client has already shipped, and our abstractions already fit it — but it needs HTTP and JSON, which `src/` has neither of, and it does diff --git a/notes/lag-aware/plan.md b/notes/lag-aware/plan.md new file mode 100644 index 000000000..9f888806f --- /dev/null +++ b/notes/lag-aware/plan.md @@ -0,0 +1,156 @@ +# Lag-aware availability checks: the plan + +What we intend to do about [`findings.md`](findings.md). That file is the evidence; this one is the +sequence. + +Status: **proposed, and blocked on step 0.** Nothing below has shipped. + +> **This branch is not only a planning branch.** `marc/lag-aware-availability` carries these notes +> now, and becomes the feature branch once step 0 clears — the implementation lands on top of the +> same branch and the same PR, rather than the notes being merged separately and the work starting +> again elsewhere. +> +> When #3191 lands, **rebase this branch onto the updated `main`** (`git rebase origin/main`); do not +> merge `main` in. #3191 squash-merges, so merging it back would move the merge base without giving +> us its ancestry, and the commit list here would start carrying phantoms. + +## Step 0 — wait for #3191 + +[#3191](https://github.com/StackExchange/StackExchange.Redis/pull/3191) ("Server-native maintenance +notifications") is open, 107 files, +13,160. It carries +`tests/StackExchange.Redis.FaultInjector.Tests`, and with it the three things this work would +otherwise have to build from scratch: + +| what | why it matters here | +| --- | --- | +| `FaultInjector/FaultInjectorClient.cs` | `POST /action` + `GET /action/{id}`, plus `GetValidTriggersAsync(scenario, effect, …)` — lets us ask a deployment which lag-inducing effects it supports instead of guessing | +| `Environment/ClusterRestClient.cs` | already a Redis Enterprise REST client on 9443: Basic auth, CA pinning, `System.Text.Json`, `GET /v1/bdbs?fields=…` | +| the tier itself | environment fixtures, provisioning, gating (`SER_FI_CONFIG_DIR`, `E2E_SCENARIO_TESTS`), and the README describing how to run it | + +Building any of that again on this branch would mean writing a second copy of a REST client against +the same cluster, and then reconciling them at merge. Not worth it for a feature whose entire +implementation is smaller than the client that tests it. + +**Nothing in steps 1–6 starts before this lands**, with one exception: step 1 is manual +investigation against a deployment and can begin whenever a cluster is available, because it produces +notes rather than code. + +## Step 1 — validate the API against a real deployment + +Before any code. Everything in findings §1 is read off documentation, and #3191's own summary is the +warning: *"the specifications are prose, the payloads were never published, and nearly every +assumption I started with was wrong in some way that mattered."* Same vendor, same class of API. + +The checklist, each item currently an assumption: + +- [ ] does the deployed version support `extend_check=lag` at all? +- [ ] does `availability_lag_tolerance_ms` as a query parameter genuinely override the cluster + default, and what happens if it is absurd (0, negative, enormous)? +- [ ] what is actually in a failure body — is `error_code` reliably present, and are the composed + forms (`bdb_unavailable_shard_unreachable_port_unbound`) real? +- [ ] is `Host: cnm.cluster.fqdn` required on the availability route? (`ClusterRestClient` sets no + such header for `/v1/bdbs` and works.) +- [ ] `/v1/bdbs/{uid}/availability` versus `/v1/local/bdbs/{uid}/endpoint/availability` — what does + each actually report on a multi-node deployment, and is RS155734 (miscalculated endpoint + metrics) visible? +- [ ] what does `GET /v1/bdbs?fields=uid,endpoints` return, in the shape Lettuce matches hosts + against? +- [ ] **what lag do real geo-replicated links show under load?** The empirical way to settle 100 ms + (server default, docs, redis-py) versus 5000 ms (Lettuce) — see findings §3. +- [ ] which fault-injector effects can drive lag past the tolerance — ask `GetValidTriggersAsync`, + do not assume `network_latency`. + +Output: the answers written into `findings.md`, and **captured response bodies saved as fixtures**, so +the stubbed tests in step 5 replay real payloads instead of guessing at the wire format. + +## Step 2 — decide where it lives + +Findings §6 sets out three options; step 1 informs the choice, so this is deliberately not decided +here. The honest current state: + +- **The dependency argument is weaker than it first looked.** `System.Net.Http` needs no package + reference on any target we ship — it is in-box on .NET Framework, in `netstandard2.0`, and in the + shared framework on `net8.0`/`net10.0`. The genuinely new dependency would be `System.Text.Json` on + `net472`/`netstandard2.0`, and findings §5 shows JSON is avoidable if the bdb uid is configured + rather than discovered. +- **`src/` may need no changes at all.** `HealthCheckProbe` is externally subclassable (no internal + abstract members), and `IConnectionMultiplexer.RawConfig` and `ConfigurationOptions.Tunnel` are both + public, so a probe can reach everything it needs from the context it already gets. +- So the question is not really "can we" but "should a general-purpose Redis client carry a + management-plane HTTP client for one commercial deployment target" — which is a judgement call, not + a technical constraint. + +Leaning: **a separate package** (option B), `net8.0`+ only. It keeps Enterprise-specific surface out +of core, versions independently, and `SocketsHttpHandler.ConnectCallback` — needed for step 4 — is +net5+ anyway, so a down-level build would be degraded regardless. But a new shipping package is a real +ownership cost for a small feature, and that is mgravell's call, not this document's. + +## Step 3 — the probe + +```csharp +// shape only; names and home package per step 2 +public sealed class LagAwareHealthCheckProbe : HealthCheckProbe +{ + public override Task CheckHealthAsync(HealthCheckContext context); +} +``` + +Behaviour, with the deliberate deviations called out: + +- `GET /v1/bdbs/{uid}/availability`, adding `extend_check=lag` and `availability_lag_tolerance_ms` + when lag-awareness is on. +- 200 → `Healthy`. A definite negative (503 with a recognised `error_code`) → `Unhealthy`. +- **A failed REST call → `Inconclusive`, not `Unhealthy`.** This is the deviation from every other + client (findings §4): Lettuce's two-state `HealthStatus` cannot distinguish "the database is down" + from "I could not ask", so a management-plane outage can fail over a database that is serving + traffic perfectly. We have `Inconclusive`; we should use it. Document it as a difference. +- Configuration: REST endpoint, credentials **via a callback so they can rotate** (Lettuce takes a + `Supplier`), CA/certificate options — step 1 confirmed the management certificate is self-signed per + environment, so ambient trust is not enough — bdb uid, tolerance, and a lag-aware on/off switch. +- Defaults deferred to step 1's measurements rather than copied from either existing client. + +Explicit uid first; host-based discovery is a later, optional extra (findings §5). + +## Step 4 — the transport seam, if we want it + +Only if we want the management plane to honour tunnels. mgravell has offered a `Tunnel` addition; +findings §8 has the shape — a **stream-returning** member, composed into +`SocketsHttpHandler.ConnectCallback` on the consuming side, so no `System.Net.Http` type enters +StackExchange.Redis's public API. + +Two reasons it is worth doing beyond testing: a CONNECT proxy configured for Redis almost certainly +applies to the Enterprise REST API too, and `InProcessTestServer` already assigns `Tunnel`, so one +assignment would spoof both planes. Two things it forces: `HttpProxyTunnel` and `LoggingTunnel` each +need an opinion about whether they apply to the management plane. + +Separable from step 3 — the probe works without it, just without tunnel support. + +## Step 5 — tests + +Three tiers, cheapest first: + +1. **Stubbed transport.** Hijack the HTTP transport completely, the way `Tunnel` does for RESP, and + replay the step-1 fixtures: 200, each 503 `error_code`, 404, 401/403, malformed bodies, and + slow-but-successful responses for probe-timeout behaviour. No listener, no port, runs in CI. +2. **`toys/KestrelRedisServer`.** It already serves HTTP on 5000 over the same `RedisServer` + singleton that serves RESP, so a fake `/v1/bdbs/{uid}/availability` can lie *coherently with* the + data plane. Real sockets on both planes; the level where the whole thing is exercised. +3. **Fault-injector scenario test**, in the tier from step 0: drive real lag past the tolerance and + assert the lag-aware check flips while the plain check stays green — then that the group fails + over, and fails back only once lag recovers. + +Tier 3 is the authoritative one and the slowest; tier 1 is what runs on every push. + +## Step 6 — documentation + +`docs/` needs the probe, the tolerance decision and its reasoning, the credential/CA requirements, +and — prominently — the `Inconclusive` deviation, because anyone comparing us against Lettuce or +redis-py will otherwise read it as a bug. + +## Not doing + +- **Not writing a second Redis Enterprise REST client.** Step 0 exists so we extend + `ClusterRestClient` instead. +- **Not copying a tolerance default.** 100 ms and 5000 ms cannot both be right; step 1 measures. +- **Not implementing host-based discovery first.** Explicit uid keeps JSON out of the required path. +- **Not changing the profiling or health-check abstractions.** They already fit; that is the finding. From 61de10c9d7be9de7a90df4dd737ce5786a4fa005 Mon Sep 17 00:00:00 2001 From: mgravell Date: Fri, 18 Sep 2026 05:34:02 +0100 Subject: [PATCH 06/34] Step 0 is cleared; note that step 1 needs the machine with cluster access #3191 merged as dc915bbc, so this branch is rebased onto it - which is what step 0 asked for, and rebased rather than merged because #3191 squash-merged and a merge would have moved the merge base without bringing its ancestry. The fault-injector tier is present here now. Also recorded where the work can happen. Step 1 is validation against a real deployment plus the fault injector, and both live on the machine holding the environment directory; this box has no ~/aws and no env_output.json anywhere, so nothing in step 1 could be run or verified here. Worth writing down rather than rediscovering, since the environment being absent looks identical to the environment being offline. Steps 2 onward are ordinary code and are not tied to a machine. Noted too that the directory is the AWS multi-cluster template, so env_output.json nests its outputs under .clusters.value[0] - a shape FaultInjectorEnvironment already handles, so it needs no work, just no surprise. --- notes/lag-aware/plan.md | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/notes/lag-aware/plan.md b/notes/lag-aware/plan.md index 9f888806f..150567547 100644 --- a/notes/lag-aware/plan.md +++ b/notes/lag-aware/plan.md @@ -3,18 +3,29 @@ What we intend to do about [`findings.md`](findings.md). That file is the evidence; this one is the sequence. -Status: **proposed, and blocked on step 0.** Nothing below has shipped. +Status: **proposed. Step 0 is cleared; step 1 is next and needs a machine with cluster access.** +Nothing below has shipped. > **This branch is not only a planning branch.** `marc/lag-aware-availability` carries these notes -> now, and becomes the feature branch once step 0 clears — the implementation lands on top of the -> same branch and the same PR, rather than the notes being merged separately and the work starting -> again elsewhere. +> now, and is the feature branch from here — the implementation lands on top of the same branch and +> the same PR, rather than the notes being merged separately and the work starting again elsewhere. > -> When #3191 lands, **rebase this branch onto the updated `main`** (`git rebase origin/main`); do not -> merge `main` in. #3191 squash-merges, so merging it back would move the merge base without giving -> us its ancestry, and the commit list here would start carrying phantoms. +> It has been rebased onto `main` post-#3191 (`git rebase origin/main`), which is what step 0 asked +> for; do not merge `main` in. #3191 squash-merged, so merging it back would move the merge base +> without giving us its ancestry, and the commit list here would start carrying phantoms. -## Step 0 — wait for #3191 +> **Where this can be worked on.** Step 1 needs a real Redis Enterprise deployment and the fault +> injector, which live on a machine holding the environment directory (`~/aws` on the box that has +> it — the AWS multi-cluster template, so `env_output.json` nests its outputs under +> `.clusters.value[0]`, which `FaultInjectorEnvironment` already handles). Nothing in step 1 can be +> done or verified without it, so the work continues there rather than on whichever box happens to +> have the repo. Steps 2 onward are ordinary code and are not tied to a machine. + +## Step 0 — wait for #3191 ✅ cleared + +**Done.** #3191 merged as `dc915bbc` on 2026-09-18, and this branch has been rebased onto it, so +`tests/StackExchange.Redis.FaultInjector.Tests` is present here now. The rest of this section records +why the wait was worth it. [#3191](https://github.com/StackExchange/StackExchange.Redis/pull/3191) ("Server-native maintenance notifications") is open, 107 files, +13,160. It carries @@ -31,9 +42,8 @@ Building any of that again on this branch would mean writing a second copy of a the same cluster, and then reconciling them at merge. Not worth it for a feature whose entire implementation is smaller than the client that tests it. -**Nothing in steps 1–6 starts before this lands**, with one exception: step 1 is manual -investigation against a deployment and can begin whenever a cluster is available, because it produces -notes rather than code. +It has landed, so nothing here is gated on it any more. Step 1 is the next thing, and its only +prerequisite is a cluster to point at. ## Step 1 — validate the API against a real deployment From 4855d55a840ef67339831e60db8d64ab668f3031 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 6 Oct 2026 11:34:48 +0100 Subject: [PATCH 07/34] Pin the cluster REST certificate per host rather than chaining to the config CA The 9443 certificate is self-signed by the cluster itself; the CA in the config directory only signs the RESP proxy certificates, so the chain build could never pass. Pin the leaf on first use, keyed by host so multi-cluster templates work. --- .../Environment/ClusterRestClient.cs | 56 +++++++++++++------ .../Environment/FaultInjectorFixture.cs | 2 +- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/tests/StackExchange.Redis.FaultInjector.Tests/Environment/ClusterRestClient.cs b/tests/StackExchange.Redis.FaultInjector.Tests/Environment/ClusterRestClient.cs index 8cddd2841..ad6f55c7c 100644 --- a/tests/StackExchange.Redis.FaultInjector.Tests/Environment/ClusterRestClient.cs +++ b/tests/StackExchange.Redis.FaultInjector.Tests/Environment/ClusterRestClient.cs @@ -1,8 +1,10 @@ using System; using System.Collections.Generic; +using System.IO; using System.Net.Http; using System.Net.Http.Headers; -using System.Security.Cryptography.X509Certificates; +using System.Security.Authentication; +using System.Security.Cryptography; using System.Text; using System.Text.Json; using System.Threading.Tasks; @@ -21,24 +23,46 @@ public sealed class ClusterRestClient : IDisposable { private readonly HttpClient _http; - public ClusterRestClient(FaultInjectorEnvironment.ClusterCredentials credentials, string? certificateAuthorityPath) + public ClusterRestClient(FaultInjectorEnvironment.ClusterCredentials credentials, DirectoryInfo configDirectory) { - var handler = new HttpClientHandler(); - - if (certificateAuthorityPath is not null) + // The cluster's management (cnm) certificate is minted at bootstrap and is self-signed - its issuer is + // itself, not any CA shipped in the config directory. `ca.crt` there signs the RESP-plane proxy + // certificates (a different, coincidentally-named CA), so chain-building the cnm cert against it fails + // even when it is the right environment. There is nowhere to read the cnm cert's expected fingerprint + // from ahead of time (env_output.json does not carry it), so this pins on first use and persists the + // fingerprint next to the rest of the environment's files - the same trust model as an SSH known_hosts + // entry. A later run against the same environment must present the identical certificate; a changed + // fingerprint fails loudly rather than silently accepting a new identity for a channel that carries + // credentials. Keyed by host: multi-cluster templates put several clusters behind one directory. + var pinPath = Path.Combine(configDirectory.FullName, $"cluster-cert.{credentials.ClusterName}.sha256"); + var handler = new HttpClientHandler { - // The cluster's management certificate is self-signed per environment, like the proxy ones. Pin to - // the CA in the config directory rather than accepting anything: this channel carries credentials. - var issuer = X509CertificateLoader.LoadCertificateFromFile(certificateAuthorityPath); - handler.ServerCertificateCustomValidationCallback = (_, certificate, chain, _) => + ServerCertificateCustomValidationCallback = (_, certificate, _, _) => { - if (certificate is null || chain is null) return false; - chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; - chain.ChainPolicy.CustomTrustStore.Add(issuer); - chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck; - return chain.Build(certificate); - }; - } + if (certificate is null) return false; + var actual = Convert.ToHexString(certificate.GetCertHash(HashAlgorithmName.SHA256)); + + if (File.Exists(pinPath)) + { + var expected = File.ReadAllText(pinPath).Trim(); + if (!string.Equals(expected, actual, StringComparison.OrdinalIgnoreCase)) + { + throw new AuthenticationException( + $"cluster management certificate at {credentials.RestUrl} does not match the pinned " + + $"fingerprint in {pinPath} (expected {expected}, got {actual}). If this environment " + + "was re-provisioned, delete that file and re-run to pin the new certificate."); + } + + return true; + } + + // trust on first use: nothing in the config directory names the expected fingerprint ahead of + // time, so the only available anchor is "whatever this environment presents the first time we + // ask", pinned from here on. + File.WriteAllText(pinPath, actual); + return true; + }, + }; _http = new HttpClient(handler) { BaseAddress = credentials.RestUrl, Timeout = TimeSpan.FromSeconds(30) }; var basic = Convert.ToBase64String(Encoding.UTF8.GetBytes($"{credentials.Username}:{credentials.Password}")); diff --git a/tests/StackExchange.Redis.FaultInjector.Tests/Environment/FaultInjectorFixture.cs b/tests/StackExchange.Redis.FaultInjector.Tests/Environment/FaultInjectorFixture.cs index 8ea143f8e..77e9443f8 100644 --- a/tests/StackExchange.Redis.FaultInjector.Tests/Environment/FaultInjectorFixture.cs +++ b/tests/StackExchange.Redis.FaultInjector.Tests/Environment/FaultInjectorFixture.cs @@ -199,7 +199,7 @@ private async Task SweepOrphansAsync() // actual name is an assumption worth not making when the consequence is deleting the wrong thing. var template = ExistingDatabase.ReadAll(Environment).Values.Select(d => d.BdbId).ToHashSet(); - using var rest = new ClusterRestClient(cluster, Environment.CertificateAuthorityPath); + using var rest = new ClusterRestClient(cluster, Environment.ConfigDirectory); foreach (var (bdbId, name) in await rest.ListDatabasesAsync()) { if (template.Contains(bdbId)) continue; From d673c902f0cf9a59365bed44825b325be2e8fe48 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 6 Oct 2026 11:34:48 +0100 Subject: [PATCH 08/34] Step 1 measured: the lag check is Active-Active only, and does what it says there Records the real-deployment results (findings section 10): off Active-Active the lag check fails unconditionally; on it, the stale member flips within seconds of sync pausing and the tolerance is honoured on a running link. Also the two integration problems found in Availability/, which mean src/ needs a change. --- notes/lag-aware/findings.md | 124 ++++++++++++++++++++++++++++++++++++ notes/lag-aware/plan.md | 68 +++++++++++++++----- 2 files changed, 175 insertions(+), 17 deletions(-) diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index 8fa2380da..bc144e53f 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -47,6 +47,12 @@ disaster recovery by … ensuring failover-failback flows only occur when databa sufficiently synchronized."* This is the check that stops you failing **back** onto a region that is reachable but stale. +> **Corrected by measurement (§10).** On RS 8.0.22 the lag is Active-Active (CRDB) sync lag, not +> intra-cluster replica lag. On any database that is not an Active-Active member, `extend_check=lag` +> answers `503 bdb_unavailable` unconditionally - with or without `replication`, and whatever +> `availability_lag_tolerance_ms` says - while the plain check answers 200. The "replica" wording +> above is the documentation's, and it is misleading. + ### Response detail | status | meaning | @@ -334,6 +340,11 @@ documentation they were drawn from: via `ServerCertificateCustomValidationCallback` with `X509ChainTrustMode.CustomRootTrust`, with the comment *"this channel carries credentials"*. So a lag-aware probe **does** need CA/cert configuration in the redis-py mould; relying on the ambient trust store will not do. + *Corrected (§10):* the 9443 certificate is self-signed by the cluster itself (issuer = subject = + cluster FQDN, minted at bootstrap), and the CA in the config directory signs only the RESP-plane + proxy certificates - so that chain build could never succeed. `ClusterRestClient` now pins the leaf + on first use instead. The conclusion stands and sharpens: a probe needs a pinned certificate or + fingerprint option, not merely a custom CA. - **Auth is HTTP Basic**, confirmed against a real cluster rather than inferred from Lettuce. - **`/v1/bdbs?fields=…` returns an array of objects carrying the requested fields**, so Lettuce's `fields=uid,endpoints` discovery is straightforward. That weakens the §5 argument a little: skipping @@ -450,6 +461,9 @@ external probe can already reach `context.Server.Multiplexer.RawConfig.Tunnel` w Lettuce uses the former. The latter does not redirect to the primary node, which may be the more accurate signal when probing a specific endpoint, and is what the docs recommend for load balancers under the `all-nodes` proxy policy. Note known issue RS155734 against the endpoint form. + *Partly answered (§10):* the local form answers for whichever node served the REST call, so through + the cluster FQDN it is effectively random under `proxy_policy: single`. Not usable from a client + unless it addresses a specific node. 4. **Explicit bdb uid, discovery, or both?** Explicit avoids JSON entirely (§5). 5. ~~**Is `HealthCheckContext` sufficient?**~~ Answered in §8 — `RawConfig` and `ConfigurationOptions.Tunnel` are both public, so a probe can reach the tunnel from the context it @@ -469,3 +483,113 @@ external probe can already reach `context.Server.Multiplexer.RawConfig.Tunnel` w `HealthCheckProbe` is externally subclassable, `RawConfig`/`Tunnel` are public, and the REST plumbing precedent lives in the test tier. The `Tunnel` addition in §8 is the one thing that would have to land in the core package — and only if we want the management plane to honour tunnels. + +## 10. Measured against real deployments (2026-09-18 to 2026-09-30) + +Everything here was observed, not read. Three AWS-template environments, all RS 8.0.22, three nodes per +cluster; the last one (`marcgravell-test-dc1342d5`) has two clusters, `c1` and `c2`, carrying two +Active-Active databases between them. Where a line is an inference rather than an observation it says so. + +### `extend_check=lag` is an Active-Active check + +Same cluster (c1), same session: + +| database | `crdt` | `replication` | plain | `extend_check=lag` at 100 ms / 600000 ms / 0 ms | +| --- | --- | --- | --- | --- | +| `re-active-active` | true | false | 200 | 200 / 200 / 200 | +| `re-active-active-oss-cluster` | true | false | 200 | 200 / 200 / 200 | +| control, freshly created | false | false | 200 | 503 / 503 / 503 | +| control, freshly created | false | true | 200 | 503 / 503 / 503 | + +Both Active-Active databases answer the same on c2. The control 503 is always +`{"error_code":"bdb_unavailable","description":"BDB is not available"}`, held for the whole 30 s +observed from creation, and was reproduced on two earlier environments over 60 s. + +- **Measured:** off Active-Active, the lag check fails regardless of replication and tolerance - the + tolerance parameter has no observable effect at all there. +- **Inferred:** the server has no sync lag to measure on such a database and reports "unavailable" + rather than "not applicable". +- **Consequence for the probe:** lag-awareness on a non-Active-Active member turns a healthy database + into a permanently unhealthy one. It must be opt-in per member, and that specific 503 should not be + read as a data-plane verdict - `Inconclusive` at most. This is a stronger version of the §4 argument. +- **Inferred about Lettuce, untested:** `EXTENDED_CHECK_DEFAULT = true` should make `LagAwareStrategy` + report `UNHEALTHY` forever against any database that is not Active-Active. Worth putting to Redis with + the 100 ms versus 5000 ms question (§3). +- **Unknown:** whether a 0 ms tolerance is honoured (and an idle link simply reports zero lag) or + ignored. The idle Active-Active result cannot tell those apart; it needs real lag. + +### Under real lag: the check flips, and only on the stale member (2026-10-05) + +Environment `marcgravell-test-9db7bbf0`, two clusters, `re-active-active` (uid 1 on both, paired by +`crdt_guid`). Sync *into* c2's member paused with `PUT /v1/bdbs/1 {"crdt_sync":"paused"}` on c2 for 60 s +while c1 took an `INCR` every ~5 s and c2 was read back; then `{"crdt_sync":"enabled"}`. + +| phase | c1 member | c2 member | data | +| --- | --- | --- | --- | +| baseline | plain 200, lag 200 (100 / 600000 / 0 ms) | plain 200, lag 200 (all three) | c2 tracks c1 | +| c2 sync paused | unchanged | plain 200, **lag 503 at all three**, first sample ~2 s in | c1 3 to 14, c2 stuck at 2 | +| resumed | unchanged | lag 503 ~5 s more, then 200 | c2 reaches 14 | + +- **Measured:** the plain check never noticed; the lag check on the stale member flipped within seconds + and cleared once the data had caught up. This is the "reachable but stale" case the feature exists for. +- **Measured:** only the member that is *behind* flips. The member being written to stays green - which + is the right polarity for failback, where the question is "is the target caught up". +- **Measured:** with sync paused, 600000 ms still answers 503 although the true lag was seconds. A paused + link is unavailable regardless of tolerance. +- **Measured: the tolerance does govern a running link.** Sync enabled; a server-side Lua loop on c1 + wrote ~50 MB per call (500 x 100 KB over a 200-key space) for 30 s, ~2.75 GB in all, while c2 was + sampled every ~0.5 s. In 66 samples the 100 ms check failed twice, each time with c2 visibly a few + batches behind, and recovered on the next sample; the 600000 ms check never failed. So 100 ms and + 600000 ms disagree exactly when the link is running but behind - the parameter is honoured. +- **Caveat on the numbers:** both clusters are in `us-east-1`. Under that load sync stayed under + 100 ms almost all the time, but that says nothing about a cross-region link, which is the case the + 100 ms versus 5000 ms question (§3) is actually about. +- **Measured:** the failure body is the same as for a non-Active-Active database - + `{"error_code":"bdb_unavailable","description":"BDB 1 is not available"}`. No lag-specific code, so a + probe cannot tell "lagging" from "not applicable" by the response; Active-Active membership has to be + configuration, not inference. +- **Inferred:** at baseline c2 was observed one write behind (read 1 after c1 wrote 2) while the 0 ms + check passed - either lag is sampled coarsely or 0 is not honoured. +- **Tooling:** `crdt_sync` `paused`/`enabled` is a narrow, reliably reversible way to produce lag - one + member, no network change, `status` returns to `active` within a few seconds. Far better suited to a + scenario test than `network_latency` or `network_failure` (which drops all inbound bar SSH on every + hosting node and kills `dmcproxy`). It is a direct REST mutation, not an injector action. + +### Other answers + +- **Cluster default** `availability_lag_tolerance_ms` is 100, on every environment - the server agrees + with the docs and redis-py, not Lettuce. +- **`Host: cnm.cluster.fqdn` is not required.** A bogus `Host` on the availability route still gets 200. +- **Both error-code families are real.** Database form: `bdb_unavailable`. Local form: + `bdb_endpoint_unavailable`, description `"Local endpoint of BDB on node is not available"`. + The composed suffixes (`_shard_unreachable`, `_port_unbound`) have not been observed yet - nothing so + far has broken a shard. +- **Healthy is an empty 200.** No body to parse; JSON appears only on failure, as §5 hoped. +- **`fields=` filters on `/v1/bdbs` but not on `/v1/cluster`**, which returns the whole object. +- **The local form answers for the node that served the request.** Through the cluster FQDN that was + always node 1, which usually does not host the endpoint under `proxy_policy: single` - so 503 for three + of four Active-Active members that were serving traffic perfectly well. +- **`/v1/bdbs` returns database passwords** (`authentication_redis_pass`, `authentication_admin_pass`) + when `mask_bdb_credentials` is false, as on these clusters, and `/v1/crdbs` returns the same for every + instance. Captured bodies cannot be committed as fixtures without scrubbing. Relevant to credential + scope too: whether `cluster_viewer`/`db_viewer` see the same was not checked (we used admin). + +### Multi-cluster shape + +- `env_output.json` nests clusters under `.clusters.value[N]`, each with its own FQDN and credentials. + `FaultInjectorEnvironment` reads only `[0]`. +- Each cluster has its own self-signed 9443 certificate, so pins are per host + (`cluster-cert..sha256`). +- Member uids happened to match across c1 and c2; the platform does not promise it. `/v1/crdbs` on one + cluster reports the other's instance with `db_uid=None`, so pairing members means reading each + cluster's `/v1/bdbs` and matching on `crdt_guid`. The probe's configuration should therefore be + "cluster REST endpoint plus uid" per member, not a uid shared across the group. + +### Fault injection + +- **`network_latency` is not safe to use for this.** It applies `netem` to the node's primary interface, + not to the database, and its scheduled removal fails ("No existing session" - the cleanup thread reuses + an SSH session that has been torn down) while the action still reports success. On 2026-09-18 that + left a 5 s delay on two nodes, which took down the cluster's own DNS and 9443 until removed by hand + over SSH. Reported to `redis-developer/cae-client-testing`. +- **Holding back sync:** done with `crdt_sync=paused` on the stale member (above), not with the injector. diff --git a/notes/lag-aware/plan.md b/notes/lag-aware/plan.md index 150567547..920e95d16 100644 --- a/notes/lag-aware/plan.md +++ b/notes/lag-aware/plan.md @@ -3,8 +3,10 @@ What we intend to do about [`findings.md`](findings.md). That file is the evidence; this one is the sequence. -Status: **proposed. Step 0 is cleared; step 1 is next and needs a machine with cluster access.** -Nothing below has shipped. +Status: **proposed. Step 0 is cleared; step 1 is answered (findings §10): the lag check flips under +real Active-Active lag, on the stale member only, and the tolerance is honoured on a running link. +Cross-region lag magnitudes remain unmeasured (both test clusters share a region).** Nothing below has +shipped. > **This branch is not only a planning branch.** `marc/lag-aware-availability` carries these notes > now, and is the feature branch from here — the implementation lands on top of the same branch and @@ -53,22 +55,34 @@ assumption I started with was wrong in some way that mattered."* Same vendor, sa The checklist, each item currently an assumption: -- [ ] does the deployed version support `extend_check=lag` at all? +- [x] does the deployed version support `extend_check=lag` at all? **Yes (RS 8.0.22), but only + meaningfully on Active-Active members; everything else gets `503 bdb_unavailable` + unconditionally.** Findings §10. - [ ] does `availability_lag_tolerance_ms` as a query parameter genuinely override the cluster - default, and what happens if it is absurd (0, negative, enormous)? + default, and what happens if it is absurd (0, negative, enormous)? **Honoured on a running + link:** under a write burst 100 ms failed while 600000 ms passed. No effect off Active-Active or + with sync paused (both always fail). Negative not tried. - [ ] what is actually in a failure body — is `error_code` reliably present, and are the composed - forms (`bdb_unavailable_shard_unreachable_port_unbound`) real? -- [ ] is `Host: cnm.cluster.fqdn` required on the availability route? (`ClusterRestClient` sets no - such header for `/v1/bdbs` and works.) -- [ ] `/v1/bdbs/{uid}/availability` versus `/v1/local/bdbs/{uid}/endpoint/availability` — what does - each actually report on a multi-node deployment, and is RS155734 (miscalculated endpoint - metrics) visible? + forms (`bdb_unavailable_shard_unreachable_port_unbound`) real? *Partly:* `error_code` and + `description` present on every failure seen; lag failure is plain `bdb_unavailable`, the same + as "not Active-Active"; composed suffixes not yet observed. +- [x] does the lag check flip while the plain check stays green? **Yes, on the stale member only, + within seconds, clearing when data catches up** (findings §10, `crdt_sync=paused`). +- [x] is `Host: cnm.cluster.fqdn` required on the availability route? **No.** +- [x] `/v1/bdbs/{uid}/availability` versus `/v1/local/bdbs/{uid}/endpoint/availability`? **The local + form reports for whichever node answered the REST call; unusable through the cluster FQDN.** Use + the database form. RS155734 not assessed. - [ ] what does `GET /v1/bdbs?fields=uid,endpoints` return, in the shape Lettuce matches hosts - against? + against? Not yet asked with those fields; full `/v1/bdbs` bodies captured (contain passwords, + not committed). - [ ] **what lag do real geo-replicated links show under load?** The empirical way to settle 100 ms - (server default, docs, redis-py) versus 5000 ms (Lettuce) — see findings §3. + (server default, docs, redis-py) versus 5000 ms (Lettuce) — see findings §3. Needs a two-cluster + environment, which now exists. - [ ] which fault-injector effects can drive lag past the tolerance — ask `GetValidTriggersAsync`, - do not assume `network_latency`. + do not assume `network_latency`. **`network_latency` is ruled out** (node-wide, and its cleanup + fails; findings §10). The `/action` enum also lists `network_failure`, `execute_rladmin_command` + and `update_cluster_config`; which of these can hold back Active-Active sync safely is the open + question. Output: the answers written into `findings.md`, and **captured response bodies saved as fixtures**, so the stubbed tests in step 5 replay real payloads instead of guessing at the wire format. @@ -83,7 +97,7 @@ here. The honest current state: shared framework on `net8.0`/`net10.0`. The genuinely new dependency would be `System.Text.Json` on `net472`/`netstandard2.0`, and findings §5 shows JSON is avoidable if the bdb uid is configured rather than discovered. -- **`src/` may need no changes at all.** `HealthCheckProbe` is externally subclassable (no internal +- **`src/` may need no changes at all** *(no longer true - see the `Inconclusive` point in step 3)*. `HealthCheckProbe` is externally subclassable (no internal abstract members), and `IConnectionMultiplexer.RawConfig` and `ConfigurationOptions.Tunnel` are both public, so a probe can reach everything it needs from the context it already gets. - So the question is not really "can we" but "should a general-purpose Redis client carry a @@ -110,13 +124,31 @@ Behaviour, with the deliberate deviations called out: - `GET /v1/bdbs/{uid}/availability`, adding `extend_check=lag` and `availability_lag_tolerance_ms` when lag-awareness is on. - 200 → `Healthy`. A definite negative (503 with a recognised `error_code`) → `Unhealthy`. +- **Lag-awareness is per member and only for Active-Active members.** Off Active-Active the server + answers `503 bdb_unavailable` to every lag check (findings §10), so enabling it there would fail a + healthy database forever. Either refuse the combination at configuration time, or check `crdt` once + via `/v1/bdbs/{uid}` and fall back to the plain check - the latter needs JSON, the former does not. +- Addressing is REST endpoint **plus** uid per member; uids are not stable across the clusters of one + Active-Active database. +- **The probe is called per endpoint, but the answer is per database.** `HealthCheck.CheckHealthAsync + (IConnectionMultiplexer)` fans out one probe call per server (`HealthCheck.Execute.cs:25-40`), so an + OSS-cluster member with six endpoints would make six identical REST calls per pass. Either cache + within a pass, or hook in at member level instead. +- **`Inconclusive` is right for failover and wrong for failback.** `ConnectionGroupMember.UpdateState` + (`MultiGroupMultiplexer.cs:315`) treats anything but `Unhealthy` as connected, and once + `FailbackDelay` has passed that makes the member eligible again. So "the REST API could not be + asked" would let the group fail back onto a member that may be stale - the exact case the lag check + exists to prevent. Keeping an active member while refusing to *return* to an unverified one needs a + distinction `UpdateState` does not currently make, which means a small `src/` change after all (step 2 + assumed none). - **A failed REST call → `Inconclusive`, not `Unhealthy`.** This is the deviation from every other client (findings §4): Lettuce's two-state `HealthStatus` cannot distinguish "the database is down" from "I could not ask", so a management-plane outage can fail over a database that is serving traffic perfectly. We have `Inconclusive`; we should use it. Document it as a difference. - Configuration: REST endpoint, credentials **via a callback so they can rotate** (Lettuce takes a - `Supplier`), CA/certificate options — step 1 confirmed the management certificate is self-signed per - environment, so ambient trust is not enough — bdb uid, tolerance, and a lag-aware on/off switch. + `Supplier`), certificate options (step 1 confirmed the management certificate is self-signed by the + cluster itself, so neither ambient trust nor a separate CA works; the option has to accept a pinned + certificate or fingerprint), bdb uid, tolerance, and a lag-aware on/off switch. - Defaults deferred to step 1's measurements rather than copied from either existing client. Explicit uid first; host-based discovery is a later, optional extra (findings §5). @@ -147,7 +179,9 @@ Three tiers, cheapest first: data plane. Real sockets on both planes; the level where the whole thing is exercised. 3. **Fault-injector scenario test**, in the tier from step 0: drive real lag past the tolerance and assert the lag-aware check flips while the plain check stays green — then that the group fails - over, and fails back only once lag recovers. + over, and fails back only once lag recovers. Produce the lag with `crdt_sync=paused` on the target + member via its cluster's REST API (findings §10), and resume in a `finally`; not with + `network_latency`. Tier 3 is the authoritative one and the slowest; tier 1 is what runs on every push. From 04207221decfbc733f7b3e06a9201c5fe57c54db Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 6 Oct 2026 11:45:46 +0100 Subject: [PATCH 09/34] Correct the client defaults: Lettuce, Jedis and redis-py all default to 5000 ms Read from source: redis-py's 5000 contradicts its own docstring's 100. None of the three checks for Active-Active, and none has a third result state. --- notes/lag-aware/findings.md | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index bc144e53f..b0424bcfc 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -108,11 +108,30 @@ has been rolling out across clients: | client | type | lag tolerance default | | --- | --- | --- | | Lettuce | `LagAwareStrategy` | **5000 ms** | -| Jedis | `LagAwareStrategy` | (same family) | -| redis-py | `LagAwareHealthCheck` | **100 ms** | +| Jedis | `LagAwareStrategy` | **5000 ms** | +| redis-py | `LagAwareHealthCheck` | **5000 ms** (*corrected*: its docstring says 100) | | Redis Enterprise cluster | `availability_lag_tolerance_ms` | **100 ms** | | REST API docs | "Recommended value" | **100 ms** | +> **Corrected 2026-10-06, read from source rather than docs** (heads as of that date; `gh` search of +> issues/PRs was blocked by the org's SSO, so discussion that did not reach code is not covered): +> +> - Lettuce `LagAwareStrategy.java:163-165`: `EXTENDED_CHECK_DEFAULT = true`, tolerance 5000 ms. +> Unchanged since it was introduced (#3576, 2026-02-06); #3914 (2026-09-15) promoted it to GA without +> touching either value. +> - Jedis `mcf/LagAwareStrategy.java:96-97`: the same `true` / 5000. +> - redis-py `asyncio/multidb/healthcheck.py:73`: `DEFAULT_LAG_AWARE_TOLERANCE = 5000`, while line 502's +> docstring says "(default: 100)" - both from the same commit, `f3806fad`, 2025-10-07. It always sends +> `extend_check=lag` with the tolerance; there is no plain mode. +> - None checks `crdt`, so given §10, *inferred*: all three report every non-Active-Active database as +> permanently unhealthy under default settings. +> - All three map any REST failure to unhealthy; none has a third state (§4). +> - TLS is truststore/CA based in all three; none supports pinning, so with the cluster's self-signed 9443 +> certificate a user must import that leaf as a trust anchor. +> +> So the server and docs say 100 ms and every client says 5000 ms. Our plan sidesteps it: send no +> tolerance unless configured, and let the cluster's own setting apply. + **Lettuce is fifty times more permissive than everyone else, including the server's own default and its own documentation's recommendation.** That is either a deliberate call about real-world WAN replication lag or an oversight; either way we should not copy a number without deciding it, and it From cf64be1612031005576ae4ab7d2c2423525c78cd Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 6 Oct 2026 12:01:13 +0100 Subject: [PATCH 10/34] Measure what a probe can discover: not Active-Active over RESP, db_viewer suffices Active-Active membership has no RESP signal, so it comes from REST (fields=crdt) or configuration. db_viewer can call everything a probe needs, but unfiltered bdb/crdb reads expose database passwords even to it, so only filtered requests. The REST endpoint derives from the configured database hostname. --- notes/lag-aware/findings.md | 39 +++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index b0424bcfc..ca2bea21f 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -593,6 +593,45 @@ while c1 took an `INCR` every ~5 s and c2 was read back; then `{"crdt_sync":"ena instance. Captured bodies cannot be committed as fixtures without scrubbing. Relevant to credential scope too: whether `cluster_viewer`/`db_viewer` see the same was not checked (we used admin). +### What a probe can discover, and with what (2026-10-06, `marcgravell-test-60790432`) + +**Active-Active membership is not visible over RESP.** Compared on c1: both Active-Active members and a +freshly created plain database (since deleted). + +- `HELLO` is identical in shape (`mode: cluster`, `role: master` on all three, including the plain one). +- `CRDT.INFO` is an unknown command everywhere; `INFO crdt` / `INFO crdb` are empty everywhere; no INFO + field mentions crdt. +- The only INFO differences are incidental: `cmdstat_replconf` on the members (the syncer's own traffic, + so absent until a peer has connected), a different module list (the members load only ReJSON and + search), and `rdb_changes_since_last_save`. None is a signal to build on. +- So the probe has to learn it from REST: `GET /v1/bdbs/{uid}?fields=uid,crdt` answers + `{"crdt":true,"uid":1}`, or it is configuration. + +**`db_viewer` is enough, and the least privileged role still sees database passwords.** Temporary +roles (`management: db_viewer`, then `cluster_viewer`) and users, deleted afterwards. RS 8 rejects the +legacy `role` field (`role_not_exist`); roles are objects referenced by `role_uids`, and a fresh cluster +has only `Admin`. + +| route, as `db_viewer` (`cluster_viewer` identical) | result | +| --- | --- | +| `/v1/bdbs/{uid}/availability`, with and without `extend_check=lag` | 200 | +| `/v1/bdbs/{uid}?fields=uid,crdt` | 200, no password fields | +| `/v1/bdbs?fields=uid,endpoints` | 200, 599 bytes, no password fields | +| `/v1/bdbs/{uid}` (unfiltered) | 200, **`authentication_redis_pass` and `authentication_admin_pass` populated** | +| `/v1/crdbs` | 200, every instance's passwords populated | + +This is with `mask_bdb_credentials: false`, the cluster's setting here; masked clusters were not +tested. Consequence: the probe must only ever issue `fields=`-filtered requests, and the docs should +recommend `db_viewer`. Possibly worth raising with the RS team that the viewer roles see secrets. + +**The REST endpoint is derivable from the database hostname.** Every database's `dns_name`, plain, +Active-Active and OSS-cluster alike, is `redis-.`, so the management endpoint is +`https://:9443`. But the OSS-cluster database advertises **IP addresses** through +`CLUSTER SLOTS`/`CLUSTER SHARDS` (`oss_cluster_api_preferred_endpoint_type: ip`), so the derivation must +start from the member's *configured* endpoint, not from discovered nodes. A customer CNAME or a bare IP +defeats it, so it can only be a default with an override. uid discovery by matching `dns_name` and port +against `/v1/bdbs?fields=uid,endpoints` is safe on credentials (filtered, no passwords). + ### Multi-cluster shape - `env_output.json` nests clusters under `.clusters.value[N]`, each with its own FQDN and credentials. From ad4b3158c1e6f1bbd1b18875a0152439b8026b83 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 6 Oct 2026 16:41:44 +0100 Subject: [PATCH 11/34] TLS reuse, the cross-client survey, and the local form routed by database host TrustIssuer(leaf) via TlsOptions validates the 9443 call unchanged. No other client activates lag checks automatically or checks crdt; go-redis routes the local form through each database host, which makes it truthful, and scopes the lag verdict to failback only. --- notes/lag-aware/findings.md | 59 ++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index ca2bea21f..196fa3a05 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -587,7 +587,14 @@ while c1 took an `INCR` every ~5 s and c2 was read back; then `{"crdt_sync":"ena - **`fields=` filters on `/v1/bdbs` but not on `/v1/cluster`**, which returns the whole object. - **The local form answers for the node that served the request.** Through the cluster FQDN that was always node 1, which usually does not host the endpoint under `proxy_policy: single` - so 503 for three - of four Active-Active members that were serving traffic perfectly well. + of four Active-Active members that were serving traffic perfectly well. *Corrected 2026-10-06:* reached + through the **database's own hostname** (`https://redis-.:9443`, as go-redis does), the + local form answered 200, plain and lag, for all four members, from the node actually serving the + endpoint; via the cluster FQDN the same calls gave 503 for three of them. Every node presents the same + cluster certificate (pinned on each), and its `*.` SAN covers the database hostname, so the + `TrustIssuer` pin should hold there too (*inferred*; only the fingerprint was checked on that path). So + the local form is usable, and arguably the more truthful one, provided it is routed by database host. + Not yet measured: whether `local` + `extend_check=lag` flips under paused sync as the database form does. - **`/v1/bdbs` returns database passwords** (`authentication_redis_pass`, `authentication_admin_pass`) when `mask_bdb_credentials` is false, as on these clusters, and `/v1/crdbs` returns the same for every instance. Captured bodies cannot be committed as fixtures without scrubbing. Relevant to credential @@ -632,6 +639,56 @@ start from the member's *configured* endpoint, not from discovered nodes. A cust defeats it, so it can only be a default with an override. uid discovery by matching `dns_name` and port against `/v1/bdbs?fields=uid,endpoints` is safe on credentials (filtered, no passwords). +### Certificate trust reuses the existing TLS callback (2026-10-06) + +The library's own `ConfigurationOptions.TrustIssuer(...)` callback, obtained through the public +`TlsOptions.CertificateValidationCallback` and adapted to `HttpClientHandler` with a one-line lambda, +validates the 9443 call. Against c1, `GET /v1/cluster`: + +| trust | result | +| --- | --- | +| platform default | rejected, `UntrustedRoot` | +| `TrustIssuer(c1's own 9443 certificate)` | **200** | +| `TrustIssuer(config-dir ca.crt)` - the CA that signs the RESP proxy certs | rejected | +| `TrustIssuer(c2's 9443 certificate)` | rejected | + +The only policy error ever presented was `RemoteCertificateChainErrors`: the certificate carries SANs for +`` and `*.` and server-auth EKU, so host-name checks pass, which is the one condition +`TrustIssuer` requires. `TrustIssuer(leaf)` therefore *is* a pin, because the self-signed leaf is its own +chain. No new trust API is needed. + +What does **not** carry over is the trust *configuration*: the data-plane and management-plane +certificates have different issuers (proxy certs from the deployment CA, 9443 self-signed per cluster), +so a member's Redis `TrustIssuer` cannot be reused for its REST calls; it is the same mechanism with a +separate setting. The adapter is the same on every TFM (`ServerCertificateCustomValidationCallback`), and +on `net5`+ `SocketsHttpHandler.SslOptions.RemoteCertificateValidationCallback` takes the delegate +unchanged, which matters if the HTTP path rides on `Tunnel` (§8). A production cluster with a CA-signed +9443 certificate needs no callback at all. + +### Who else implements it, and what turns it on (2026-10-06) + +Read from anonymous shallow clones, heads of 2026-09-26 to 2026-10-06 (`gh` search is blocked by SSO). + +| client | support | activation | REST endpoint | uid | lag / tolerance | REST TLS | +| --- | --- | --- | --- | --- | --- | --- | +| Lettuce | `failover/health/LagAwareStrategy.java` | explicit strategy supplier; default `PingStrategy` | user-supplied URI | discovered from `/v1/bdbs` by host | on / 5000 | own `SslOptions` | +| Jedis | `mcf/LagAwareStrategy.java` | explicit strategy supplier; default `PingStrategy` | user-supplied | `/v1/bdbs?fields=uid,endpoints` by host | on / 5000 | own `SslOptions` | +| redis-py (asyncio only) | `asyncio/multidb/healthcheck.py` | explicit `health_checks`; default `PingHealthCheck` | user-supplied `health_check_url` + port 9443 | **unfiltered** `/v1/bdbs`, by `dns_name`/`addr` | always on / 5000 | own CA / `verify_tls` / mTLS | +| go-redis | **feature branch only** (`feature/multidb-integration`), `multidb/healthcheck_lag_aware.go` | explicit `HealthChecks`; default PING | **derived: `https://:9443`**, override available | `/v1/bdbs?fields=uid,endpoints`, host + resolved IPs + port | on / 5000 | own `tls.Config` incl. `InsecureSkipVerify` | +| node-redis, NRedisStack, redis-rs, rueidis, predis, phpredis | not found | | | | | | + +- **Nobody activates it automatically** - no domain detection, no provider. It is an opt-in health + check everywhere, and PING is the default everywhere. +- **No client checks `crdt`**, so (*inferred*, from §10) all fail permanently on non-Active-Active. +- **No client reuses the Redis connection's TLS settings** for REST, and none pins. +- redis-py's unfiltered `/v1/bdbs` receives database passwords (per the role test above). +- **go-redis has the two design ideas worth taking:** + 1. it uses the *local* form routed via each member's own database host, once per master in cluster + mode - the routing that makes the local form truthful (see the correction above); + 2. `FailbackOnly() == true`: the lag verdict only gates moving traffic *to* a member and never evicts + the active one, while PING keeps judging the active member's liveness. That is the `Inconclusive` + failback problem in `plan.md` step 3, solved by giving the check a scope rather than a new result. + ### Multi-cluster shape - `env_output.json` nests clusters under `.clusters.value[N]`, each with its own FQDN and credentials. From 495725ec3f3349a9c83c28ff9d681021b5fef707 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 7 Oct 2026 15:29:16 +0100 Subject: [PATCH 12/34] The local form flips under paused sync exactly as the database form does Same verdict, same timing, stale member only, tolerance ignored while paused. Its failure code is bdb_endpoint_unavailable, shared with the wrong-node case, so the choice between forms is about call shape, not information. --- notes/lag-aware/findings.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index 196fa3a05..065f96984 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -594,7 +594,13 @@ while c1 took an `INCR` every ~5 s and c2 was read back; then `{"crdt_sync":"ena cluster certificate (pinned on each), and its `*.` SAN covers the database hostname, so the `TrustIssuer` pin should hold there too (*inferred*; only the fingerprint was checked on that path). So the local form is usable, and arguably the more truthful one, provided it is routed by database host. - Not yet measured: whether `local` + `extend_check=lag` flips under paused sync as the database form does. + *Measured 2026-10-07 (`marcgravell-test-823d3f1e`):* under paused sync the local form, routed by + database host, behaves exactly like the database form - plain 200 throughout; lag 503 on the stale + member only, at 100 ms and 600000 ms alike, from the first sample (~2 s); cleared on the same sample as + the database form once data caught up. Its lag failure is `bdb_endpoint_unavailable`, which is also + what it answers on a node not serving the endpoint, so it is no more distinguishable by body than the + database form. For lag, the two forms give the same verdict; the difference is call shape (one per + database versus one per endpoint, and the local form only works routed by database host). - **`/v1/bdbs` returns database passwords** (`authentication_redis_pass`, `authentication_admin_pass`) when `mask_bdb_credentials` is false, as on these clusters, and `/v1/crdbs` returns the same for every instance. Captured bodies cannot be committed as fixtures without scrubbing. Relevant to credential From bfb7844d2377bb776dd639eabfb04e44e52b492e Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 7 Oct 2026 15:32:10 +0100 Subject: [PATCH 13/34] Sketch the lag-aware API for review A failback-only health check per member rather than a new result value, a member-scoped probe, and a LagAware factory whose only required input is the management credential. Compared against Lettuce, Jedis, redis-py and go-redis. --- notes/lag-aware/api-sketch.md | 191 ++++++++++++++++++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 notes/lag-aware/api-sketch.md diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md new file mode 100644 index 000000000..f862bec84 --- /dev/null +++ b/notes/lag-aware/api-sketch.md @@ -0,0 +1,191 @@ +# Lag-aware availability: API sketch + +Status: **sketch for review, not implemented.** Evidence is in [`findings.md`](findings.md) (section 10 +especially); sequence in [`plan.md`](plan.md). Everything here sits behind `SER007` +(`Experiments.GeoRedundantFailover`), like the rest of `Availability/`. + +Aim: parity with Lettuce, Jedis, redis-py and go-redis where it serves users, and deliberate, +documented deviations where the measurements say the others get it wrong. + +## The shape in one example + +```csharp +var east = new ConnectionGroupMember("redis-14460.c1.example.com:14460,password=...", "east") +{ + // new: consulted only before moving traffic *to* this member (see 1) + FailbackHealthCheck = HealthCheck.LagAware(new LagAwareOptions + { + Credentials = _ => new(new NetworkCredential("probe@example.com", secrets.Get("re-mgmt"))), + }), +}; +``` + +That is the whole opt-in. The REST endpoint, database uid, whether the database is Active-Active, and +the tolerance are all derived or left to the cluster (see 3). Liveness stays with the member's +existing `HealthCheck` (PING by default). + +## 1. Where the lag verdict goes: a failback-only check, not a new result value + +**Proposal:** a second, optional health check per member, consulted only when deciding whether a +member may *become* active. + +```csharp +public sealed class ConnectionGroupMember +{ + public HealthCheck? HealthCheck { get; set; } // existing: liveness, may evict + public HealthCheck? FailbackHealthCheck { get; set; } // new: eligibility, never evicts +} + +public sealed class MultiGroupOptions.Builder +{ + public HealthCheck? FailbackHealthCheck { get; set; } // new: group default, null = none +} +``` + +Rules, applied by `SelectPreferredGroup` / `UpdateState`: + +- **The active member is kept while its liveness check passes**, whatever its failback check says. +- **A member becomes eligible for selection** when its liveness check passes *and* its failback + check (if any) answers `Healthy`. `Inconclusive` from the failback check means **not eligible**: + "could not ask" must not let the group fail back onto a member that may be stale. +- **If no member is eligible, select on liveness alone.** Every member lagging (a partition between + regions, say) must degrade to "serve from somewhere", never to "serve from nowhere". +- The failback check runs on the same `HealthCheckInterval` poll as liveness, but only for members + that are not active (the active member's answer is never used). + +**Why not the new `HealthCheckResult` value** (accepted in principle, but this is better, I think): + +- One check would have to answer two different questions: "is it alive" (PING) and "is it caught up" + (REST). A new value means one probe that does both, so every lag-aware user also takes over + liveness. +- Every consumer of `HealthCheckResult` would have to learn the new value: `CollateAsync`, + `UpdateState`, all three `HealthCheckProbePolicy` implementations, and any user-written probe or + policy. Measured: they currently treat everything except `Unhealthy` as healthy, so a value they do + not know would silently mean "fine". +- It matches go-redis's `FailbackOnly()`, the only client that addresses this at all. + +**Parity note:** Lettuce, Jedis and redis-py put the lag check in the single health-check slot, so a +stale reading evicts the active member. Measured: only the member *behind* flips, which is never the +one being written to, so for a single-writer deployment that eviction does not happen, and for a +multi-writer one it would be wrong. This is a deliberate deviation. + +## 2. Running once per member, not per endpoint + +`HealthCheck.CheckHealthAsync(IConnectionMultiplexer)` calls the probe once per server. A REST probe +answers per database, so an OSS-cluster member with six endpoints would make six identical calls. + +```csharp +public enum HealthCheckProbeScope { Endpoint = 0, Member = 1 } + +public abstract class HealthCheckProbe +{ + public virtual HealthCheckProbeScope Scope => HealthCheckProbeScope.Endpoint; // new +} +``` + +A `Member`-scoped probe is called once per pass, with `HealthCheckContext.Server` set to a connected +server of that member (needed for the configured endpoint and its `RawConfig`). Adding a virtual +member is binary-safe for existing subclasses. + +## 3. The probe + +```csharp +public sealed partial class HealthCheck // existing type; new static factories +{ + public static HealthCheck LagAware(LagAwareOptions options); // extend_check=lag (if Active-Active) + public static HealthCheck DatabaseAvailability(LagAwareOptions options); // plain availability only +} + +public sealed class LagAwareOptions +{ + // required: the one thing that cannot be derived + public Func> Credentials { get; set; } + + // optional; null = derive or defer + public Uri? RestEndpoint { get; set; } // null: https://:9443 + public int? DatabaseId { get; set; } // null: discover by host and port + public LagCheckMode LagCheck { get; set; } // default Auto + public TimeSpan? LagTolerance { get; set; } // null: send none, cluster setting applies + public RemoteCertificateValidationCallback? CertificateValidation { get; set; } + public void TrustIssuer(string path); // same helper as ConfigurationOptions + public void TrustIssuer(X509Certificate2 issuer); +} + +public enum LagCheckMode { Auto = 0, Enabled = 1, Disabled = 2 } +``` + +The `HealthCheck` returned is an ordinary `HealthCheck` (so `ProbeCount`, `ProbeTimeout` and +`ProbeInterval` apply, via `new HealthCheck.Builder(HealthCheck.LagAware(...)) { ... }`), whose +probe is `Member`-scoped. + +### What it does, per pass + +1. **Endpoint:** `RestEndpoint`, else `https://:9443`. + Measured: every database's host is `redis-.`, the 9443 certificate's SANs + cover `*.`, and any node answers. Configured endpoint, not discovered nodes: an OSS-cluster + database advertises IP addresses. +2. **uid** (once, cached; cleared on 404, as Lettuce does): `DatabaseId`, else + `GET /v1/bdbs?fields=uid,endpoints`, matched on `dns_name` and port. +3. **Active-Active** (once, cached), for `LagCheck = Auto`: `GET /v1/bdbs/{uid}?fields=uid,crdt`. +4. **Check:** `GET /v1/bdbs/{uid}/availability`, adding `extend_check=lag` when the database is + Active-Active (`Auto`) or `Enabled`, and `availability_lag_tolerance_ms` only if `LagTolerance` is set. + +**Never an unfiltered `/v1/bdbs` or `/v1/crdbs`.** Measured: those return database passwords even to +`db_viewer` when `mask_bdb_credentials` is off. + +### Result mapping + +| response | result | note | +| --- | --- | --- | +| 200 | `Healthy` | body is empty | +| 503 (any `error_code`) | `Unhealthy` | lag and "unavailable" share `bdb_unavailable` | +| 404 | `Unhealthy` | and clear the cached uid | +| 401, 403, other status | `Inconclusive` | management-plane problem; logged | +| connect / TLS failure, timeout | `Inconclusive` | as above | + +The last two rows are the deviation from every other client (all map REST failure to unhealthy). As a +failback check, `Inconclusive` already means "not eligible" (1), so the deviation only changes the +outcome where it should: a management-plane outage can no longer evict anything. + +### Defaults compared + +| | Lettuce | Jedis | redis-py | go-redis (branch) | **this** | +| --- | --- | --- | --- | --- | --- | +| activation | explicit | explicit | explicit | explicit | explicit | +| REST endpoint | supplied | supplied | supplied | derived from db host | **derived**, overridable | +| uid | discovered | discovered | discovered (unfiltered) | discovered | **discovered (filtered)**, overridable | +| lag check | on | on | on | on | **only if Active-Active** (`Auto`) | +| tolerance | 5000 ms | 5000 ms | 5000 ms | 5000 ms | **unset: cluster's own** (100 ms measured) | +| REST failure | unhealthy | unhealthy | unhealthy | unhealthy | **inconclusive** | +| evicts active member | yes | yes | yes | no (failback only) | **no (failback only)** | +| credentials | rotating supplier | supplier | static | static | **rotating callback** | +| TLS | own truststore | own truststore | own CA / verify | own `tls.Config` | **same callback type as Redis**, separate setting | + +## 4. Activation + +Explicit, as in every other client: setting `FailbackHealthCheck`. Management credentials are a +separate secret from the Redis password, so no options provider can supply them, and nothing else +needs configuring. What a provider *can* do, later and separately: `RedisEnterpriseOptionsProvider` +could carry a group-wide default once credentials have a home in configuration. Not proposed now. + +## 5. Platform + +| TFM | | +| --- | --- | +| `net8.0`, `net10.0` | everything | +| `net472`, `netstandard2.0` | everything; JSON parsing for steps 2 and 3 by a small internal reader or a `System.Text.Json` package reference on those targets only (**open**, below) | +| `net461` | works with ambient trust; setting `CertificateValidation`/`TrustIssuer` throws `PlatformNotSupportedException` at creation, on the understanding that `ServerCertificateCustomValidationCallback` needs .NET Framework 4.7.1 (*unverified*; the build will say) | + +Test seam: an internal `Func` on `LagAwareOptions`, reached via +`InternalsVisibleTo`, so tests replay captured responses without a listener. Kept internal so no +`System.Net.Http` type enters the public API; Lettuce's public `HttpClient` constructor shows there is +demand, and it can be made public later. + +## Open + +1. **JSON on older targets:** a small internal reader (the shapes are tiny) or `System.Text.Json`. +2. **Names:** `LagAware` follows Lettuce, Jedis and redis-py; `FailbackHealthCheck` is ours. +3. **Credentials type:** `NetworkCredential` (in-box everywhere), or a dedicated type that could + later carry mTLS (redis-py supports client certificates for this call). +4. **The all-lagging fallback** in 1: select on liveness alone, as proposed, or stay with the + current active member even if it is no longer healthy. From 19c7092f83a92c2540f8e7361a262413a380a19c Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 7 Oct 2026 15:49:44 +0100 Subject: [PATCH 14/34] API sketch: System.Text.Json decided; open questions compared with other clients Keep LagAware naming and failback (not fallback) terminology; rotating NetworkCredential callback plus a separate client-certificate option; and keep the liveness fallback, which no client has, because a dead peer probably makes the survivor's lag check fail. --- notes/lag-aware/api-sketch.md | 68 +++++++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 10 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index f862bec84..17212885f 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -49,7 +49,12 @@ Rules, applied by `SelectPreferredGroup` / `UpdateState`: check (if any) answers `Healthy`. `Inconclusive` from the failback check means **not eligible**: "could not ask" must not let the group fail back onto a member that may be stale. - **If no member is eligible, select on liveness alone.** Every member lagging (a partition between - regions, say) must degrade to "serve from somewhere", never to "serve from nowhere". + regions, say) must degrade to "serve from somewhere", never to "serve from nowhere". This matters + more than it looks: measured, a member whose *incoming* sync has stopped answers lag 503 at any + tolerance. So when the active region dies, the survivor's lag check is likely to fail precisely + because its peer is gone (*inferred*: measured with sync paused, not with the peer killed). A strict + rule would refuse the failover that is needed most. In short: the failback check gates moving *away + from a live active member*; it never blocks replacing a dead one. - The failback check runs on the same `HealthCheckInterval` poll as liveness, but only for members that are not active (the active member's answer is never used). @@ -173,7 +178,7 @@ could carry a group-wide default once credentials have a home in configuration. | TFM | | | --- | --- | | `net8.0`, `net10.0` | everything | -| `net472`, `netstandard2.0` | everything; JSON parsing for steps 2 and 3 by a small internal reader or a `System.Text.Json` package reference on those targets only (**open**, below) | +| `net472`, `netstandard2.0` | everything; JSON (steps 2 and 3, and `error_code` for logging) via a `System.Text.Json` package reference on these targets only - in-box on `net8.0`+. *Decided 2026-10-07:* NRedisStack, which builds on SE.Redis, already depends on it | | `net461` | works with ambient trust; setting `CertificateValidation`/`TrustIssuer` throws `PlatformNotSupportedException` at creation, on the understanding that `ServerCertificateCustomValidationCallback` needs .NET Framework 4.7.1 (*unverified*; the build will say) | Test seam: an internal `Func` on `LagAwareOptions`, reached via @@ -181,11 +186,54 @@ Test seam: an internal `Func` on `LagAwareOptions`, reached `System.Net.Http` type enters the public API; Lettuce's public `HttpClient` constructor shows there is demand, and it can be made public later. -## Open - -1. **JSON on older targets:** a small internal reader (the shapes are tiny) or `System.Text.Json`. -2. **Names:** `LagAware` follows Lettuce, Jedis and redis-py; `FailbackHealthCheck` is ours. -3. **Credentials type:** `NetworkCredential` (in-box everywhere), or a dedicated type that could - later carry mTLS (redis-py supports client certificates for this call). -4. **The all-lagging fallback** in 1: select on liveness alone, as proposed, or stay with the - current active member even if it is no longer healthy. +## Open questions, compared with the other clients + +Read from source 2026-10-07: Lettuce `114a3ef`, Jedis `bb5f01d`, redis-py `c3bb0e6`, go-redis +`feature/multidb-integration` `9377b5b`. (Correction to the earlier survey: redis-py's sync client +imports the same `LagAwareHealthCheck`, so it is not asyncio-only.) + +1. ~~**JSON on older targets**~~ *Decided:* `System.Text.Json`, see 5. + +2. **Names.** + + | | the check | general abstraction | returning to the preferred database | failback-only check | + | --- | --- | --- | --- | --- | + | Lettuce | `LagAwareStrategy` (`databaseAvailability` / `lagAware` / `lagAwareWithTolerance`) | `HealthCheckStrategy` | `failbackSupported`, `failbackCheckInterval`, `gracePeriod` | none | + | Jedis | `LagAwareStrategy`, same factories | `HealthCheckStrategy` | same three as Lettuce | none | + | redis-py | `LagAwareHealthCheck` | `HealthCheck` | `auto_fallback_interval`, `grace_period` | none | + | go-redis | `LagAwareHealthCheck`, `NewLagAwareHealthCheck(WithLagAware...)` | `MultiDBHealthCheck` | `AutoFallbackInterval`, `GracePeriod` | `FailbackOnly() bool`, a marker on the check | + + *Recommendation:* `LagAware` is universal, keep it, with `DatabaseAvailability` alongside as + Lettuce/Jedis have. Use "failback", not "fallback": it is our existing word (`FailbackDelay`) and + the Java clients'. `FailbackHealthCheck` as a separate slot rather than go-redis's marker, because + our member has one `HealthCheck` rather than a list, and a slot keeps liveness independent. + +3. **Credentials.** + + | | type | rotates | client certificate to REST | + | --- | --- | --- | --- | + | Lettuce | `Supplier`, read per request | yes | via `SslOptions` keystore (*inferred*) | + | Jedis | `Supplier`, read per call | yes | via `SslOptions` keystore (*inferred*) | + | redis-py | `auth_basic: Tuple[str, str]` | no | `client_cert_file` / `client_key_file` / `client_key_password` | + | go-redis | `WithLagAwareBasicAuth(user, password)` | no | `WithLagAwareClientCert(PEM)` / `...FromFiles` | + + *Recommendation:* keep the callback returning `NetworkCredential` (rotation parity with the Java + clients; in-box everywhere), and add client-certificate support as its own option, mirroring + `ConfigurationOptions`' existing `CertificateSelection` callback and PEM/PFX helpers, as every client + supports it. No dedicated credentials type needed. + +4. **When every member fails.** + + | | active member's own lag check fails | everything unhealthy | "all lagging" special-cased | + | --- | --- | --- | --- | + | Lettuce | evicted | `RedisNoHealthyDatabaseException`, `AllDatabasesUnhealthyEvent`, retries forever | no | + | Jedis | evicted | `JedisTemporarilyNotAvailableException`, then `...PermanentlyNotAvailableException` | no | + | redis-py | evicted | `NoValidDatabaseException` inside `TemporaryUnavailableException` | no | + | go-redis | **kept** (PING judges it) | stays on a live active; otherwise `ErrTemporarilyNotAvailable`, then `ErrPermanentlyNotAvailable` | no; lagging candidates stay unselectable | + + *Recommendation:* no client falls back to liveness alone, so the proposal in 1 has no precedent. + Keep it anyway, for the reason given there: once the active region is gone, the survivor's lag + check probably fails *because* the region is gone, and go-redis's rule would then refuse the + failover (*inferred* from its code and our paused-sync measurement, not tested). This is worth + measuring before it is built: isolate one member's region and read the survivor's lag check. It + needs `network_failure` or similar, which needs the same care `network_latency` taught. From f330ffb0e5a8e544f288d4d893ce5915bc013af7 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 10:33:36 +0100 Subject: [PATCH 15/34] Partition test: the lag check is blind for a minute, then fails on both sides Partitioning the clusters (tagged iptables rules, self-healing) showed no lag for ~60 s while sync had stopped, then tolerance-governed failure on both members; a tolerance of 0 is honoured. Supports the liveness fallback, and adds a caveat: failback in the first minute after a link breaks is not protected. --- notes/lag-aware/api-sketch.md | 23 ++++++++++++----------- notes/lag-aware/findings.md | 30 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index 17212885f..66d5d3346 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -49,12 +49,15 @@ Rules, applied by `SelectPreferredGroup` / `UpdateState`: check (if any) answers `Healthy`. `Inconclusive` from the failback check means **not eligible**: "could not ask" must not let the group fail back onto a member that may be stale. - **If no member is eligible, select on liveness alone.** Every member lagging (a partition between - regions, say) must degrade to "serve from somewhere", never to "serve from nowhere". This matters - more than it looks: measured, a member whose *incoming* sync has stopped answers lag 503 at any - tolerance. So when the active region dies, the survivor's lag check is likely to fail precisely - because its peer is gone (*inferred*: measured with sync paused, not with the peer killed). A strict - rule would refuse the failover that is needed most. In short: the failback check gates moving *away - from a live active member*; it never blocks replacing a dead one. + regions, say) must degrade to "serve from somewhere", never to "serve from nowhere". Measured under a + partition (findings section 10): both members' lag checks pass for about a minute, then both fail at + 100 ms. So a region outage that lasts more than a minute leaves *no* member passing the lag check; + a strict rule (go-redis's) would allow failover in the first minute and refuse it afterwards. In + short: the failback check gates moving *away from a live active member*; it never blocks replacing a + dead one. +- **The first minute after a link breaks is blind** (measured): the lag check reports no lag for + ~60 s after sync stops. So a failback decision in that window is not protected by the lag check; + `FailbackDelay` covers some of it, and the docs must say so. - The failback check runs on the same `HealthCheckInterval` poll as liveness, but only for members that are not active (the active member's answer is never used). @@ -232,8 +235,6 @@ imports the same `LagAwareHealthCheck`, so it is not asyncio-only.) | go-redis | **kept** (PING judges it) | stays on a live active; otherwise `ErrTemporarilyNotAvailable`, then `ErrPermanentlyNotAvailable` | no; lagging candidates stay unselectable | *Recommendation:* no client falls back to liveness alone, so the proposal in 1 has no precedent. - Keep it anyway, for the reason given there: once the active region is gone, the survivor's lag - check probably fails *because* the region is gone, and go-redis's rule would then refuse the - failover (*inferred* from its code and our paused-sync measurement, not tested). This is worth - measuring before it is built: isolate one member's region and read the survivor's lag check. It - needs `network_failure` or similar, which needs the same care `network_latency` taught. + Keep it anyway. *Measured 2026-10-08* by partitioning the clusters: after ~60 s every member fails + the lag check, so go-redis's rule would refuse a failover during any region outage longer than a + minute (its behaviour there is *inferred* from its code, not run). diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index 065f96984..555b1151c 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -574,6 +574,36 @@ while c1 took an `INCR` every ~5 s and c2 was read back; then `{"crdt_sync":"ena scenario test than `network_latency` or `network_failure` (which drops all inbound bar SSH on every hosting node and kills `dmcproxy`). It is a direct REST mutation, not an injector action. +### Under a partition: blind for about a minute, then tolerance-governed, and symmetric (2026-10-08) + +Environment `marcgravell-test-41f58c28`. A network partition between the clusters, nothing else: on each +c1 node, iptables rules tagged `lagprobe` dropped traffic to and from c2's six node addresses (private and +public), with a `systemd-run` timer on each node as a self-heal backstop. SSH, DNS, 9443 and the client +path were untouched. 60 s, then removed; all three nodes verified back at their baseline rule sets. +Writes to c1 every ~5 s; both members sampled (database and local forms). + +| observer time | c2 data | c2 lag check (100 / 600000 / 0 ms) | c1 lag check (100 / 600000 / 0 ms) | +| --- | --- | --- | --- | +| t+0 to 27 s, before | in step | 200 / 200 / 200 | 200 / 200 / 200 | +| t+28 s | **partition applied** | | | +| t+31 to 84 s | **stuck at 8**, c1 reaching 20 | **200 / 200 / 200** | 200 / 200 / 200 | +| t+88 s | stuck | **503 / 200 / 503** | 200 / 200 / **503** | +| t+94 s | stuck | 503 / 200 / 503 | **503** / 200 / 503 | +| t+92 s | **partition removed** | | | +| t+101 s on | caught up | 200 / 200 / 200 | 200 / 200 / 200 | + +Plain checks 200 throughout, both forms, both members; the local form tracked the database form. + +- **Measured: the lag check is blind to a dead link for about 60 s.** Sync stopped at once, but both + members reported no lag for ~57 s. Unlike paused sync, which fails immediately at any tolerance. +- **Measured: then it compares a growing lag against the tolerance.** 0 ms failed first, then 100 ms; + 600000 ms never failed. **A tolerance of 0 is honoured** (open since 2026-09-30). +- **Measured: both members fail.** Each is missing the other's stream, so "every member lagging" is the + ordinary state of a partition, not an edge case. +- **Inferred:** a dead peer region looks the same from the survivor as this partition does, so the + survivor's lag check passes for about a minute after the region dies and fails after that (at 100 ms). + The ~60 s presumably comes from the syncer's own failure detection; not investigated. + ### Other answers - **Cluster default** `availability_lag_tolerance_ms` is 100, on every environment - the server agrees From 796ca5efaf8557e2b528b720d0b5af0ed7eb59c4 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 11:57:24 +0100 Subject: [PATCH 16/34] AllSuccess: a run of only inconclusive probes is not a pass The default policy reported Healthy when no probe failed, even if none succeeded. Harmless for liveness, where both count as connected, but wrong for any caller that distinguishes them. --- .../Availability/HealthCheckProbePolicy.cs | 6 ++++-- .../StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs b/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs index b09848814..6b929f823 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs @@ -52,8 +52,10 @@ public override HealthCheckResult Evaluate(in HealthCheckProbeContext context) // Fail as soon as we have any failure if (context.Failure > 0) return HealthCheckResult.Unhealthy; - // Succeed only when all probes have succeeded (no remaining) - if (context.Remaining == 0) return HealthCheckResult.Healthy; + // With no failures and nothing remaining, succeed - but only if something actually succeeded: a run + // of nothing but inconclusive probes ("could not tell") is not a pass, and a failback check + // depends on the difference + if (context.Remaining == 0) return context.Success > 0 ? HealthCheckResult.Healthy : HealthCheckResult.Inconclusive; // Can't determine yet return HealthCheckResult.Inconclusive; diff --git a/tests/StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs b/tests/StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs index 0c2d4892f..23dc13373 100644 --- a/tests/StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs @@ -65,6 +65,7 @@ public void AnySuccess_EvaluatesCorrectly(int success, int failure, int remainin [InlineData(1, 0, 2, HealthCheckResult.Inconclusive)] // Success but more probes remain [InlineData(0, 1, 2, HealthCheckResult.Unhealthy)] // Early failure [InlineData(4, 0, 1, HealthCheckResult.Inconclusive)] // Multiple successes but still waiting + [InlineData(0, 0, 0, HealthCheckResult.Inconclusive)] // Every probe inconclusive: not a pass public void AllSuccess_EvaluatesCorrectly(int success, int failure, int remaining, HealthCheckResult expected) { var policy = HealthCheckProbePolicy.AllSuccess; From 9e2d07e2cb004e808ce8f0e81e840f3a7d26c8bc Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 11:57:24 +0100 Subject: [PATCH 17/34] Failback health checks: ask whether a member has caught up before moving to it (SER011) ConnectionGroupMember.FailbackHealthCheck (and a group default) is consulted only for members that are not active, and only Healthy makes them eligible. It never evicts the active member, and when no live member is eligible, selection falls back to liveness, since a partition leaves every region behind at once. HealthCheckProbe.Scope lets a probe run once per member rather than per endpoint. --- Directory.Build.props | 2 +- docs/Failover.md | 42 ++- docs/exp/SER011.md | 31 +++ src/RESPite/Shared/Experiments.cs | 1 + .../Availability/HealthCheck.Execute.cs | 30 ++- .../Availability/HealthCheckProbe.cs | 6 + .../Availability/HealthCheckProbeScope.cs | 23 ++ .../Availability/MultiGroupMultiplexer.cs | 99 ++++++- .../Availability/MultiGroupOptions.cs | 26 +- .../PublicAPI/PublicAPI.Unshipped.txt | 9 + .../FailbackHealthCheckTests.cs | 251 ++++++++++++++++++ 11 files changed, 510 insertions(+), 10 deletions(-) create mode 100644 docs/exp/SER011.md create mode 100644 src/StackExchange.Redis/Availability/HealthCheckProbeScope.cs create mode 100644 tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs diff --git a/Directory.Build.props b/Directory.Build.props index 55c0088fe..048e033ce 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -12,7 +12,7 @@ NETSDK1069 - $(NoWarn);NU5105;NU1507;SER004;SER005;SER009 + $(NoWarn);NU5105;NU1507;SER004;SER005;SER009;SER011 https://github.com/StackExchange/StackExchange.Redis/releases https://seredis.dev/ MIT diff --git a/docs/Failover.md b/docs/Failover.md index eb08eb4d4..995aa0b6d 100644 --- a/docs/Failover.md +++ b/docs/Failover.md @@ -83,6 +83,7 @@ Because the policies are immutable, there is no question of whether a change "ta | `RetryPolicy` | `MultiGroupOptions.RetryPolicy` | *(none; retry is applied per-database via `WithRetry`)* | | `HealthCheckInterval` | `MultiGroupOptions.HealthCheckInterval` | *(none; it is the group's re-evaluation cadence)* | | `FailbackDelay` | `MultiGroupOptions.FailbackDelay` | `ConnectionGroupMember.FailbackDelay` | +| `FailbackHealthCheck` (experimental, `SER011`) | `MultiGroupOptions.FailbackHealthCheck` | `ConnectionGroupMember.FailbackHealthCheck` | | `Weight` | *(none)* | `ConnectionGroupMember.Weight` | | `SkipInitialHealthCheck` | *(none)* | `ConnectionGroupMember.SkipInitialHealthCheck` | @@ -443,7 +444,7 @@ HealthCheck healthCheck = new HealthCheck.Builder #### HealthCheckProbePolicy.AllSuccess (Default) -The health check passes only if **all** probes succeed. This provides the strictest evaluation: +The health check passes only if **no** probe fails and at least one succeeds. This provides the strictest evaluation. A run in which every probe was `Inconclusive` (the probe could not tell) reports `Inconclusive`, not `Healthy`: ```csharp HealthCheck healthCheck = new HealthCheck.Builder @@ -753,7 +754,44 @@ ConnectionGroupMember[] members = [ This guards against **flapping**: a member that is intermittently failing will keep pushing its "last failure" time forward, so it never satisfies the delay and stays out of rotation until it is genuinely stable. -> Implementation note: the failback check is pure tick math on the wall clock — the last-failure time and the cutoff (`UtcNow - FailbackDelay`) are both compared as raw `long` UTC ticks, so `DateTimeKind` never enters into it. (`DateTime.Ticks` and `TimeSpan.Ticks` share the same 100 ns unit.) +> Implementation note: the `FailbackDelay` comparison is pure tick math on the wall clock — the last-failure time and the cutoff (`UtcNow - FailbackDelay`) are both compared as raw `long` UTC ticks, so `DateTimeKind` never enters into it. (`DateTime.Ticks` and `TimeSpan.Ticks` share the same 100 ns unit.) + +### Failback health checks + +> Experimental, behind diagnostic id `SER011`; see [SER011](exp/SER011.md). + +`HealthCheck` answers "is this member alive". Some deployments need a second question answered before traffic +moves *to* a member: "has it caught up". In an Active-Active deployment, a region that has just come back can be +reachable but stale, and failing back onto it reads (and builds on) data that is behind. +`ConnectionGroupMember.FailbackHealthCheck` (or the group default, `MultiGroupOptions.FailbackHealthCheck`) is +that second question: + +```csharp +ConnectionGroupMember member = new("us-west.redis.example.com:6379", name: "US West") +{ + Weight = 10, + // CaughtUpProbe stands for a probe of your own; see "Custom Health Check Probes" + FailbackHealthCheck = new HealthCheck.Builder { Probe = new CaughtUpProbe(), ProbeCount = 1 }, +}; +``` + +The rules: + +- **It never removes the active member.** It is consulted only for members that are not active, and only + decides whether they may *become* active. Liveness stays with `HealthCheck`. +- **Only `Healthy` makes a member eligible.** `Inconclusive` (for example, the probe could not reach whatever + it asks) leaves the member ineligible, as does `Unhealthy`; neither marks it unhealthy. +- **A verdict is not kept while it cannot be acted on.** A member that is active, or not connected, is checked + afresh before it is next eligible. +- **If no connected member is eligible, selection falls back to liveness alone.** In a partition between + regions every member can be behind at once, and a group that refused to serve from any of them would turn a + consistency precaution into an outage. When the active member is lost, the best eligible survivor is + preferred, then the best live one. +- **An explicit `TryFailoverTo(member)` overrides it**, as it does `IsUnhealthy`. + +A probe that asks about the database as a whole, rather than one endpoint, can set +`HealthCheckProbe.Scope` to `HealthCheckProbeScope.Member`, so that it runs once per member per pass rather than +once per endpoint. ### Anti-flap tiebreak in selection diff --git a/docs/exp/SER011.md b/docs/exp/SER011.md new file mode 100644 index 000000000..0bc9a8cd3 --- /dev/null +++ b/docs/exp/SER011.md @@ -0,0 +1,31 @@ +Failback health checks let a group ask, before moving traffic *to* a member, whether that member is fit to +take it: typically, in an Active-Active deployment, whether it has caught up with the others, rather than +only whether it is alive. See [Failback health checks](../Failover.md#failback-health-checks) for the rules. + +The diagnostic covers: + +- `ConnectionGroupMember.FailbackHealthCheck` and `MultiGroupOptions.FailbackHealthCheck` (and its builder); +- `HealthCheckProbe.Scope` and `HealthCheckProbeScope`, for probes that answer per member rather than per + endpoint. + +It is experimental for two reasons: + +1. **The selection rules are new and deliberately differ from other clients.** A failback check never evicts + the active member, and when no live member passes it, selection falls back to liveness rather than refusing + to serve. Both choices follow from measurements against real Active-Active deployments, where a partition + between regions leaves every member behind at once; they may still be refined. +2. **The probe it was built for is still to come.** The intended use is Redis Enterprise's lag-aware + availability API, through a probe that has not shipped yet. Until it has, and has been exercised against + real deployments, the shape of this API may change. + +If you accept the above, you can suppress this warning by adding the following to your `csproj` file: + +```xml +$(NoWarn);SER011 +``` + +or more granularly / locally in C#: + +``` c# +#pragma warning disable SER011 +``` diff --git a/src/RESPite/Shared/Experiments.cs b/src/RESPite/Shared/Experiments.cs index 93fd067f8..55b146f61 100644 --- a/src/RESPite/Shared/Experiments.cs +++ b/src/RESPite/Shared/Experiments.cs @@ -23,6 +23,7 @@ internal static class Experiments public const string Respite = "SER004"; public const string UnitTesting = "SER005"; public const string Transport = "SER009"; + public const string LagAwareFailover = "SER011"; // ReSharper restore InconsistentNaming diff --git a/src/StackExchange.Redis/Availability/HealthCheck.Execute.cs b/src/StackExchange.Redis/Availability/HealthCheck.Execute.cs index 1ee5dda8a..9869d63a9 100644 --- a/src/StackExchange.Redis/Availability/HealthCheck.Execute.cs +++ b/src/StackExchange.Redis/Availability/HealthCheck.Execute.cs @@ -20,7 +20,15 @@ private async Task CheckHealthCoreAsync(IConnectionMultiplexe try { Task[] pending; - if (multiplexer is IInternalConnectionMultiplexer internalMultiplexer) + if (Probe.Scope is HealthCheckProbeScope.Member) + { + // the answer is about the database, not an endpoint: ask once, via any connected server + var server = FirstConnectedServer(multiplexer); + if (server is null) return HealthCheckResult.Unhealthy; + pending = GetReusablePending(ref _reusablePending, 1); + pending[0] = CheckHealthAsync(server); + } + else if (multiplexer is IInternalConnectionMultiplexer internalMultiplexer) { var snapshot = internalMultiplexer.GetServerSnapshot(); pending = GetReusablePending(ref _reusablePending, snapshot.Length); @@ -51,6 +59,26 @@ private async Task CheckHealthCoreAsync(IConnectionMultiplexe } } + private static IServer? FirstConnectedServer(IConnectionMultiplexer multiplexer) + { + if (multiplexer is IInternalConnectionMultiplexer internalMultiplexer) + { + foreach (var endpoint in internalMultiplexer.GetServerSnapshot()) + { + if (endpoint.IsConnected) return endpoint.GetRedisServer(null); + } + + return null; + } + + foreach (var server in multiplexer.GetServers()) + { + if (server.IsConnected) return server; + } + + return null; + } + internal int TotalTimeoutMillis() { bool valid = TryComputeTotalTimeoutMillis(ProbeCount, ProbeTimeout, ProbeInterval, out int millis); diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbe.cs b/src/StackExchange.Redis/Availability/HealthCheckProbe.cs index 6b449e034..f0f8a4b39 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbe.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbe.cs @@ -15,6 +15,12 @@ public abstract partial class HealthCheckProbe /// public abstract Task CheckHealthAsync(HealthCheckContext context); + /// + /// Whether this probe is invoked per endpoint (the default) or once per member. + /// + [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] + public virtual HealthCheckProbeScope Scope => HealthCheckProbeScope.Endpoint; + private static Task? _inconclusive, _healthy, _unhealthy; /// diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbeScope.cs b/src/StackExchange.Redis/Availability/HealthCheckProbeScope.cs new file mode 100644 index 000000000..906420a4a --- /dev/null +++ b/src/StackExchange.Redis/Availability/HealthCheckProbeScope.cs @@ -0,0 +1,23 @@ +using System.Diagnostics.CodeAnalysis; +using RESPite; + +namespace StackExchange.Redis.Availability; + +/// +/// How often a is invoked when a whole member is checked. +/// +[Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] +public enum HealthCheckProbeScope +{ + /// + /// Once per server endpoint of the member; the default, and right for probes that test the connection + /// itself (such as ). + /// + Endpoint = 0, + + /// + /// Once per member, against one connected endpoint; for probes whose answer is about the database as a + /// whole rather than any single endpoint, such as a call to a management API. + /// + Member = 1, +} diff --git a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs index f4bf36872..a1c893138 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs +++ b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs @@ -89,6 +89,7 @@ private enum MemberFlags ExplicitOverrideFlag = 1 << 2, // manual failover target (see ExplicitOverride) SkipInitialHealthCheck = 1 << 3, // see SkipInitialHealthCheck Unhealthy = 1 << 4, // disabled by a health-check or circuit-breaker + FailbackVerified = 1 << 5, // most recent failback health-check passed (see FailbackHealthCheck) } private int _flags; @@ -228,6 +229,22 @@ public bool SkipInitialHealthCheck /// public TimeSpan? FailbackDelay { get; set; } + /// + /// A health-check consulted only before traffic moves to this member; when + /// , is used, and when that + /// is also , there is none. + /// + /// + /// Unlike , this never removes the active member: it decides whether a member + /// that is not active may become so, typically by asking whether it has caught up with the others. + /// A member is only eligible once this reports ; an + /// answer (for example, the question could not be asked) + /// leaves it ineligible. If no connected member is eligible, selection falls back to connectivity and + /// alone, so the group always serves from somewhere when it can. + /// + [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] + public HealthCheck? FailbackHealthCheck { get; set; } + // the breaker to hand to this member's connections: member override, else its own config, else the group internal CircuitBreaker? ResolveCircuitBreaker(MultiGroupOptions options) => CircuitBreaker ?? Configuration.CircuitBreaker ?? options.CircuitBreaker; @@ -236,6 +253,15 @@ public bool SkipInitialHealthCheck internal TimeSpan ResolveFailbackDelay(MultiGroupOptions options) => FailbackDelay ?? options.FailbackDelay; + internal HealthCheck? ResolveFailbackHealthCheck(MultiGroupOptions options) => FailbackHealthCheck ?? options.FailbackHealthCheck; + + // maintained by the group on each pass; only meaningful when a failback health-check resolves + internal bool FailbackVerified + { + get => GetFlag(MemberFlags.FailbackVerified); + set => SetFlag(MemberFlags.FailbackVerified, value); + } + /// /// The relative weight of this group member; higher is preferred. /// @@ -667,8 +693,62 @@ internal async Task RunHealthCheckAsync() } HealthCheck.PutReusablePending(ref _reusableHealthCheckBuffer, ref pending); + await RunFailbackChecksAsync(members).ForAwait(); } + // Failback checks decide eligibility, never health: they run only for members that are up and not + // active (the active member's answer is never used), after liveness has been updated, and a verdict + // is only kept while it can still be acted on - so a member that stops being up, or becomes active, + // must be checked afresh before it is next eligible. Anything short of Healthy, including a + // timeout, leaves the member ineligible; none of it marks a member unhealthy. + private async Task RunFailbackChecksAsync(ConnectionGroupMember[] members) + { + var active = _activeStub.Active; + List<(ConnectionGroupMember Member, Task Pending)>? checks = null; + int totalTimeoutMillis = 0; + foreach (var member in members) + { + var check = member.ResolveFailbackHealthCheck(_options); + if (check is null) continue; + + if (!member.ConsiderActive || ReferenceEquals(member.Multiplexer, active)) + { + member.FailbackVerified = false; + continue; + } + + totalTimeoutMillis = Math.Max(totalTimeoutMillis, check.TotalTimeoutMillis()); + (checks ??= []).Add((member, check.CheckHealthAsync(member.Multiplexer))); + } + + if (checks is null) return; + + var all = new Task[checks.Count]; + for (int i = 0; i < all.Length; i++) all[i] = checks[i].Pending; + await Task.WhenAll(all).TimeoutAfter(totalTimeoutMillis).ForAwait(); + + foreach (var (member, pending) in checks) + { + if (pending.IsCompletedSuccessfully) + { + member.FailbackVerified = await pending.ForAwait() is HealthCheckResult.Healthy; + } + else + { + _ = pending.ObserveErrors(); + member.FailbackVerified = false; + } + } + } + + // eligible to become (or stay) active: the active member always is, as is an explicit failover + // target; otherwise a member with a failback check must have passed it on the latest pass + private bool IsFailbackEligible(ConnectionGroupMember member, ConnectionMultiplexer? active) + => member.ExplicitOverride + || ReferenceEquals(member.Multiplexer, active) + || member.ResolveFailbackHealthCheck(_options) is null + || member.FailbackVerified; + private long GetFailbackFailureCutoff(ConnectionGroupMember member) { // the minimum last-observed unhealthy time (as UTC ticks) that we'll allow for reconnect; @@ -685,7 +765,7 @@ internal void SelectPreferredGroup() { if (_disposed) return; var existingActive = _activeStub.Active; - ConnectionGroupMember? preferredMember = null, previousMember = null; + ConnectionGroupMember? preferredMember = null, fallbackMember = null, previousMember = null; var members = _members; foreach (var member in members) { @@ -699,10 +779,19 @@ internal void SelectPreferredGroup() member.UpdateLatency(); // this can change passively // (note that when in doubt, we prefer the active muxer, to prevent flapping) - preferredMember = ConnectionGroupMember.Select(preferredMember, member, existingActive); + fallbackMember = ConnectionGroupMember.Select(fallbackMember, member, existingActive); + if (IsFailbackEligible(member, existingActive)) + { + preferredMember = ConnectionGroupMember.Select(preferredMember, member, existingActive); + } } } + // The active member is always eligible, so this only applies when it is gone (or there is none): + // if no live member has passed its failback check - every one lagging, say, which is the normal + // state of a partition between regions - serve from the best live member rather than from none. + preferredMember ??= fallbackMember; + SetActive(preferredMember?.Multiplexer); if (preferredMember is not null && !ReferenceEquals(preferredMember, previousMember)) @@ -1331,6 +1420,12 @@ public async Task AddAsync(ConnectionGroupMember member, TextWriter? log = null) { var health = await member.ResolveHealthCheck(_options).CheckHealthAsync(muxer).ConfigureAwait(false); member.UpdateState(health, GetFailbackFailureCutoff(member)); + + // a newly added member is never the active one, so its failback check applies at once + if (member.ResolveFailbackHealthCheck(_options) is { } failback && member.ConsiderActive) + { + member.FailbackVerified = await failback.CheckHealthAsync(muxer).ConfigureAwait(false) is HealthCheckResult.Healthy; + } } // apply any shared hooks diff --git a/src/StackExchange.Redis/Availability/MultiGroupOptions.cs b/src/StackExchange.Redis/Availability/MultiGroupOptions.cs index 2840d996c..42fde340a 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupOptions.cs +++ b/src/StackExchange.Redis/Availability/MultiGroupOptions.cs @@ -18,7 +18,7 @@ public sealed class MultiGroupOptions internal static readonly TimeSpan DefaultFailbackDelay = TimeSpan.Zero; private static readonly MultiGroupOptions DefaultInstance = new( - HealthCheck.Default, CircuitBreaker.Default, RetryPolicy.Default, DefaultHealthCheckInterval, DefaultFailbackDelay); + HealthCheck.Default, CircuitBreaker.Default, RetryPolicy.Default, DefaultHealthCheckInterval, DefaultFailbackDelay, null); /// /// Default shared options. @@ -30,13 +30,15 @@ private MultiGroupOptions( CircuitBreaker circuitBreaker, RetryPolicy retryPolicy, TimeSpan healthCheckInterval, - TimeSpan failbackDelay) + TimeSpan failbackDelay, + HealthCheck? failbackHealthCheck) { HealthCheck = healthCheck; CircuitBreaker = circuitBreaker; RetryPolicy = retryPolicy; HealthCheckInterval = healthCheckInterval; FailbackDelay = failbackDelay; + FailbackHealthCheck = failbackHealthCheck; } /// @@ -74,6 +76,13 @@ private MultiGroupOptions( /// public TimeSpan FailbackDelay { get; } + /// + /// The failback health check to use for members of the group when no per-member one is specified; + /// (the default) means none. See . + /// + [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] + public HealthCheck? FailbackHealthCheck { get; } + /// /// Allows configuration of . /// @@ -96,6 +105,7 @@ public Builder(MultiGroupOptions options) RetryPolicy = options.RetryPolicy; HealthCheckInterval = options.HealthCheckInterval; FailbackDelay = options.FailbackDelay; + FailbackHealthCheck = options.FailbackHealthCheck; } /// @@ -126,6 +136,13 @@ public Builder(MultiGroupOptions options) /// public TimeSpan FailbackDelay { get; set; } = DefaultFailbackDelay; + /// + /// The failback health check to use for members of the group when no per-member one is specified; + /// (the default) means none. See . + /// + [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] + public HealthCheck? FailbackHealthCheck { get; set; } + /// /// Create a new options instance. /// @@ -142,12 +159,13 @@ public MultiGroupOptions Create() && ReferenceEquals(CircuitBreaker, CircuitBreaker.Default) && ReferenceEquals(RetryPolicy, RetryPolicy.Default) && HealthCheckInterval == DefaultHealthCheckInterval - && FailbackDelay == DefaultFailbackDelay) + && FailbackDelay == DefaultFailbackDelay + && FailbackHealthCheck is null) { return DefaultInstance; } - return new MultiGroupOptions(HealthCheck, CircuitBreaker, RetryPolicy, HealthCheckInterval, FailbackDelay); + return new MultiGroupOptions(HealthCheck, CircuitBreaker, RetryPolicy, HealthCheckInterval, FailbackDelay, FailbackHealthCheck); } /// diff --git a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt index ab058de62..353476b08 100644 --- a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt +++ b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt @@ -1 +1,10 @@ #nullable enable +[SER011]StackExchange.Redis.Availability.ConnectionGroupMember.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? +[SER011]StackExchange.Redis.Availability.ConnectionGroupMember.FailbackHealthCheck.set -> void +[SER011]StackExchange.Redis.Availability.HealthCheckProbeScope +[SER011]StackExchange.Redis.Availability.HealthCheckProbeScope.Endpoint = 0 -> StackExchange.Redis.Availability.HealthCheckProbeScope +[SER011]StackExchange.Redis.Availability.HealthCheckProbeScope.Member = 1 -> StackExchange.Redis.Availability.HealthCheckProbeScope +[SER011]StackExchange.Redis.Availability.MultiGroupOptions.Builder.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? +[SER011]StackExchange.Redis.Availability.MultiGroupOptions.Builder.FailbackHealthCheck.set -> void +[SER011]StackExchange.Redis.Availability.MultiGroupOptions.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? +[SER011]virtual StackExchange.Redis.Availability.HealthCheckProbe.Scope.get -> StackExchange.Redis.Availability.HealthCheckProbeScope diff --git a/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs b/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs new file mode 100644 index 000000000..1bb1d8fac --- /dev/null +++ b/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs @@ -0,0 +1,251 @@ +using System; +using System.Collections.Concurrent; +using System.Net; +using System.Threading.Tasks; +using StackExchange.Redis.Availability; +using Xunit; +using static StackExchange.Redis.Server.RedisServer; + +namespace StackExchange.Redis.Tests.MultiGroupTests; + +/// +/// The failback health check: consulted only before traffic moves to a member, never evicting the active one, +/// with selection falling back to liveness when no live member is eligible. +/// +public class FailbackHealthCheckTests(ITestOutputHelper log) +{ + private static readonly DnsEndPoint Alpha = new("alpha", 6379), Beta = new("beta", 6379), Gamma = new("gamma", 6379); + + // answers per endpoint as scripted by the test (default Healthy), counting calls + private sealed class ScriptedProbe(HealthCheckProbeScope scope = HealthCheckProbeScope.Endpoint) : HealthCheckProbe + { + private readonly ConcurrentDictionary _answers = new(); + + public ConcurrentDictionary Calls { get; } = new(); + + public override HealthCheckProbeScope Scope => scope; + + public void Set(EndPoint endpoint, HealthCheckResult result) => _answers[endpoint] = result; + + public override Task CheckHealthAsync(HealthCheckContext context) + { + var endpoint = context.Server.EndPoint; + Calls.AddOrUpdate(endpoint, 1, (_, n) => n + 1); + return Task.FromResult(_answers.TryGetValue(endpoint, out var result) ? result : HealthCheckResult.Healthy); + } + } + + private static HealthCheck Single(HealthCheckProbe probe) => new HealthCheck.Builder { Probe = probe, ProbeCount = 1 }; + + private sealed class Fixture : IAsyncDisposable + { + private readonly InProcessTestServer[] _servers; + private readonly DnsEndPoint[] _endpoints; + + public Fixture(ITestOutputHelper log, params (DnsEndPoint Endpoint, double Weight)[] members) + { + _servers = new InProcessTestServer[members.Length]; + _endpoints = Array.ConvertAll(members, m => m.Endpoint); + Members = new ConnectionGroupMember[members.Length]; + for (int i = 0; i < members.Length; i++) + { + _servers[i] = new InProcessTestServer(log, endpoint: members[i].Endpoint); + Members[i] = new(_servers[i].GetClientConfig(), members[i].Endpoint.Host) { Weight = members[i].Weight }; + } + } + + public ConnectionGroupMember[] Members { get; } + + public ScriptedProbe Liveness { get; } = new(); + + public ScriptedProbe Failback { get; } = new(); + + public IConnectionGroup Group { get; private set; } = null!; + + public async Task ConnectAsync() + { + MultiGroupOptions options = new MultiGroupOptions.Builder + { + HealthCheck = Single(Liveness), + FailbackHealthCheck = Single(Failback), + HealthCheckInterval = TimeSpan.MaxValue, // passes are driven by the test + }; + // Hold every member down until all of their connections are up, so the first real selection + // sees the whole group: otherwise whichever member connects first is selected by the liveness + // fallback and, by design, is never evicted by a failback check - a race, not the rule under test. + foreach (var endpoint in _endpoints) Liveness.Set(endpoint, HealthCheckResult.Unhealthy); + Group = await ConnectionMultiplexer.ConnectGroupAsync(Members, options); + + var watch = System.Diagnostics.Stopwatch.StartNew(); + while (!Array.TrueForAll(Members, m => m.Multiplexer.IsConnected) && watch.Elapsed < TimeSpan.FromSeconds(10)) + { + await Task.Delay(25, TestContext.Current.CancellationToken); + } + Assert.True(Array.TrueForAll(Members, m => m.Multiplexer.IsConnected), "members did not connect"); + + foreach (var endpoint in _endpoints) Liveness.Set(endpoint, HealthCheckResult.Healthy); + await PassAsync(); + await GroupWait.AssertConnectedAsync(Group); + } + + public void MarkDown(EndPoint endpoint) => Liveness.Set(endpoint, HealthCheckResult.Unhealthy); + + public Task PassAsync() => MultiGroupMultiplexer.TryHealthCheckAndSelectPreferredGroupAsync(Group); + + public string? Active => Group.ActiveMember?.Name; + + public async ValueTask DisposeAsync() + { + if (Group is not null) await Group.DisposeAsync(); + foreach (var server in _servers) server.Dispose(); + } + } + + [Fact] + public async Task IneligibleMemberIsNotSelectedUntilItPasses() + { + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + f.Failback.Set(Beta, HealthCheckResult.Unhealthy); // beta: higher weight, but behind + await f.ConnectAsync(); + Assert.Equal("alpha", f.Active); + + f.Failback.Set(Beta, HealthCheckResult.Healthy); // caught up + await f.PassAsync(); + Assert.Equal("beta", f.Active); + Assert.False(f.Members[1].IsUnhealthy); // eligibility is not health + } + + [Fact] + public async Task InconclusiveIsNotEligible() + { + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + f.Failback.Set(Beta, HealthCheckResult.Inconclusive); // "could not ask" + await f.ConnectAsync(); + Assert.Equal("alpha", f.Active); + } + + [Fact] + public async Task ActiveMemberIsNeverEvictedByItsFailbackCheck() + { + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + await f.ConnectAsync(); + Assert.Equal("beta", f.Active); + + f.Failback.Set(Beta, HealthCheckResult.Unhealthy); + await f.PassAsync(); + Assert.Equal("beta", f.Active); + + // checked once, on the first real pass when nothing was active yet; never while active + Assert.True(f.Failback.Calls.TryGetValue(Beta, out var calls)); + Assert.Equal(1, calls); + } + + [Fact] + public async Task NoEligibleMemberFallsBackToLiveness() + { + // every member lagging - the normal state of a partition between regions - must still serve + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + f.Failback.Set(Alpha, HealthCheckResult.Unhealthy); + f.Failback.Set(Beta, HealthCheckResult.Unhealthy); + await f.ConnectAsync(); + Assert.Equal("beta", f.Active); + } + + [Fact] + public async Task DeadActiveIsReplacedEvenWhenNoSurvivorIsEligible() + { + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + await f.ConnectAsync(); + Assert.Equal("beta", f.Active); + + f.MarkDown(Beta); // the active region is gone... + f.Failback.Set(Alpha, HealthCheckResult.Unhealthy); // ...and the survivor reports lag because of it + await f.PassAsync(); + Assert.Equal("alpha", f.Active); + } + + [Fact] + public async Task DeadActivePrefersAnEligibleSurvivor() + { + await using var f = new Fixture(log, (Alpha, 1), (Gamma, 5), (Beta, 9)); + await f.ConnectAsync(); + Assert.Equal("beta", f.Active); + + f.MarkDown(Beta); + f.Failback.Set(Gamma, HealthCheckResult.Unhealthy); // higher weight, but behind + await f.PassAsync(); + Assert.Equal("alpha", f.Active); + } + + [Fact] + public async Task ExplicitFailoverOverridesIneligibility() + { + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + await f.ConnectAsync(); + f.Failback.Set(Alpha, HealthCheckResult.Unhealthy); + await f.PassAsync(); + Assert.Equal("beta", f.Active); + + Assert.True(f.Group.TryFailoverTo(f.Members[0])); + Assert.Equal("alpha", f.Active); + } + + [Fact] + public async Task FormerActiveMustBeCheckedAfreshBeforeReturning() + { + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + await f.ConnectAsync(); + await f.PassAsync(); // beta active: its verdict is not kept while active + Assert.Equal("beta", f.Active); + + Assert.True(f.Group.TryFailoverTo(f.Members[0])); + Assert.Equal("alpha", f.Active); + + // removing the override reselects at once, but beta has no current verdict, so it is not eligible yet + Assert.True(f.Group.TryFailoverTo(null)); + Assert.Equal("alpha", f.Active); + + await f.PassAsync(); // now checked, and healthy + Assert.Equal("beta", f.Active); + } + + [Fact] + public async Task MemberScopedProbeRunsOncePerMember() + { + using var server = new InProcessTestServer(log) { ServerType = ServerType.Cluster }; + GetHost(server.DefaultEndPoint, out var port); + var other = server.AddEmptyNode(new IPEndPoint(IPAddress.Loopback, port + 1)); + server.Migrate((RedisKey)"scope-key", other); + await using var conn = await server.ConnectAsync(); + Assert.Equal(2, conn.GetServers().Length); + + // an endpoint that is not yet connected is reported unhealthy without being probed, so wait for both + var watch = System.Diagnostics.Stopwatch.StartNew(); + while (!Array.TrueForAll(conn.GetServers(), s => s.IsConnected) && watch.Elapsed < TimeSpan.FromSeconds(10)) + { + await Task.Delay(25, TestContext.Current.CancellationToken); + } + Assert.True(Array.TrueForAll(conn.GetServers(), s => s.IsConnected)); + + var perEndpoint = new ScriptedProbe(HealthCheckProbeScope.Endpoint); + var perMember = new ScriptedProbe(HealthCheckProbeScope.Member); + Assert.Equal(HealthCheckResult.Healthy, await Single(perEndpoint).CheckHealthAsync(conn)); + Assert.Equal(HealthCheckResult.Healthy, await Single(perMember).CheckHealthAsync(conn)); + + Assert.Equal(2, perEndpoint.Calls.Count); + Assert.Single(perMember.Calls); + Assert.All(perMember.Calls.Values, n => Assert.Equal(1, n)); + } + + [Fact] + public void OptionsCarryTheFailbackHealthCheck() + { + Assert.Null(MultiGroupOptions.Default.FailbackHealthCheck); + Assert.Same(MultiGroupOptions.Default, new MultiGroupOptions.Builder().Create()); + + var check = Single(new ScriptedProbe()); + MultiGroupOptions options = new MultiGroupOptions.Builder { FailbackHealthCheck = check }; + Assert.Same(check, options.FailbackHealthCheck); + Assert.Same(check, new MultiGroupOptions.Builder(options).Create().FailbackHealthCheck); + } +} From 2ba0a308a53c68a04b994243c0d14d04b001a2f1 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 11:57:24 +0100 Subject: [PATCH 18/34] Notes: core change implemented behind SER011 (SER007 is retired) --- notes/lag-aware/api-sketch.md | 22 +++++++++++++++++++--- notes/lag-aware/plan.md | 5 +++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index 66d5d3346..69efe9a90 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -1,8 +1,10 @@ # Lag-aware availability: API sketch -Status: **sketch for review, not implemented.** Evidence is in [`findings.md`](findings.md) (section 10 -especially); sequence in [`plan.md`](plan.md). Everything here sits behind `SER007` -(`Experiments.GeoRedundantFailover`), like the rest of `Availability/`. +Status: **sections 1 and 2 implemented** (2026-10-08); the probe (3) and onwards are still a sketch. +Evidence is in [`findings.md`](findings.md) (section 10 especially); sequence in [`plan.md`](plan.md). +*Correction:* `SER007` was retired when geo-redundant failover went GA (#3247), so the shipped +`Availability/` API is not experimental; everything new here is behind a new diagnostic, **`SER011`** +(`Experiments.LagAwareFailover`, documented in `docs/exp/SER011.md`). Aim: parity with Lettuce, Jedis, redis-py and go-redis where it serves users, and deliberate, documented deviations where the measurements say the others get it wrong. @@ -77,6 +79,20 @@ stale reading evicts the active member. Measured: only the member *behind* flips one being written to, so for a single-writer deployment that eviction does not happen, and for a multi-writer one it would be wrong. This is a deliberate deviation. +**As built**, with one consequence worth reviewing: because the active member is never evicted by its +failback check, a member chosen by the liveness fallback (no eligible member was up at the time) stays +active while it is alive, even after an eligible member appears. That matches go-redis's "never evict" +and is what weight alone would do if the fallback member has the higher weight; the alternative (treat +a fallback-selected active member as provisional, and move to an eligible one when it appears) is a +small change if preferred. Tested in `FailbackHealthCheckTests`; the fixture holds all members down until +every connection is up, precisely because a start-up race otherwise exercises this case. + +Implementing it also surfaced an existing bug: `HealthCheckProbePolicy.AllSuccess` (the default) reported +`Healthy` for a run of nothing but `Inconclusive` probes, contrary to its own comment. Harmless for +liveness (both count as connected) but fatal for a failback check, where "could not ask" would have read as +"caught up". Fixed: such a run is now `Inconclusive`. The mirror-image bug in `AnySuccess` (all +`Inconclusive` reads as `Unhealthy`, which *does* affect liveness) is left alone for a separate decision. + ## 2. Running once per member, not per endpoint `HealthCheck.CheckHealthAsync(IConnectionMultiplexer)` calls the probe once per server. A REST probe diff --git a/notes/lag-aware/plan.md b/notes/lag-aware/plan.md index 920e95d16..e66cd0771 100644 --- a/notes/lag-aware/plan.md +++ b/notes/lag-aware/plan.md @@ -5,8 +5,9 @@ sequence. Status: **proposed. Step 0 is cleared; step 1 is answered (findings §10): the lag check flips under real Active-Active lag, on the stale member only, and the tolerance is honoured on a running link. -Cross-region lag magnitudes remain unmeasured (both test clusters share a region).** Nothing below has -shipped. +Cross-region lag magnitudes remain unmeasured (both test clusters share a region).** The core change +(failback health check, member-scoped probes, selection rules) is implemented behind `SER011`; the REST +probe itself is next. See [`api-sketch.md`](api-sketch.md). > **This branch is not only a planning branch.** `marc/lag-aware-availability` carries these notes > now, and is the feature branch from here — the implementation lands on top of the same branch and From eb44fdd96a9719dd61589ead634b242a9b3cba2d Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 13:24:46 +0100 Subject: [PATCH 19/34] HealthCheck.LagAware: ask Redis Enterprise whether a member is available and caught up (SER011) A member-scoped probe of the database availability API. Only the management credentials are required: the REST endpoint derives from the configured host, the database is discovered with filtered requests only, the lag question is asked only of Active-Active databases, and no tolerance is sent unless set. Could-not-ask is Inconclusive, never Unhealthy. System.Text.Json on the down-level targets; custom certificate validation is unavailable on net461. --- Directory.Packages.props | 1 + docs/Failover.md | 39 +++ docs/exp/SER011.md | 11 +- .../Availability/HealthCheck.LagAware.cs | 20 ++ .../Availability/HealthCheckProbe.LagAware.cs | 226 +++++++++++++ .../Availability/LagAwareOptions.cs | 107 +++++++ .../ConfigurationOptions.cs | 2 +- src/StackExchange.Redis/LoggerExtensions.cs | 6 + .../PublicAPI/PublicAPI.Unshipped.txt | 22 ++ .../StackExchange.Redis.csproj | 4 + .../LagAwareProbeTests.cs | 303 ++++++++++++++++++ 11 files changed, 737 insertions(+), 4 deletions(-) create mode 100644 src/StackExchange.Redis/Availability/HealthCheck.LagAware.cs create mode 100644 src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs create mode 100644 src/StackExchange.Redis/Availability/LagAwareOptions.cs create mode 100644 tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index f822909d2..1009a1163 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -8,6 +8,7 @@ + diff --git a/docs/Failover.md b/docs/Failover.md index 995aa0b6d..1e7177014 100644 --- a/docs/Failover.md +++ b/docs/Failover.md @@ -793,6 +793,45 @@ A probe that asks about the database as a whole, rather than one endpoint, can s `HealthCheckProbe.Scope` to `HealthCheckProbeScope.Member`, so that it runs once per member per pass rather than once per endpoint. +#### Redis Enterprise: lag-aware availability + +For Redis Enterprise Active-Active deployments, `HealthCheck.LagAware(...)` asks the cluster's own +[database availability API](https://redis.io/docs/latest/operate/rs/monitoring/db-availability/) whether a +member is available and caught up. The only thing it needs is the cluster's management credentials: + +```csharp +ConnectionGroupMember member = new("redis-14460.us-west.example.com:14460,password=...", name: "US West") +{ + Weight = 10, + FailbackHealthCheck = HealthCheck.LagAware(new LagAwareOptions + { + Credentials = _ => new(new NetworkCredential("probe@example.com", secrets.Get("us-west-mgmt"))), + }), +}; +``` + +What it does by default, and why: + +- **The REST endpoint is derived** from the member's first configured endpoint, as `https://{host}:9443/`; + set `RestEndpoint` if the database is reached through a name the cluster's certificate does not cover. +- **The database is found** by matching that endpoint against the cluster's databases, using only filtered + requests (unfiltered database listings include database passwords, even for read-only roles); set + `DatabaseId` to skip this. +- **The lag question is only asked of Active-Active databases** (`LagCheck = LagCheckMode.Auto`). Asked of + any other database, the server reports it unavailable regardless of tolerance. +- **No tolerance is sent**, so the cluster's own `availability_lag_tolerance_ms` applies; set `LagTolerance` + to override it. (Other clients default to 5000 ms; the server's default is 100 ms.) +- **"Could not ask" is `Inconclusive`, not `Unhealthy`.** A management-plane problem (REST API unreachable, + credentials rejected) never fails anything over; as a failback check it simply leaves the member + ineligible. A role with `management: db_viewer` is sufficient. +- **The management certificate is usually self-signed by the cluster**, so it will not validate against the + platform trust store; `options.TrustIssuer(path)` with the cluster's own certificate (or its issuer) works, + as it does for the data plane, but it is a separate setting from the database's TLS trust. + +One limitation to plan around: measured on Redis Enterprise 8.0, when the link between regions breaks, the +lag check keeps reporting "caught up" for roughly a minute before it notices. A failback in that window is not +protected by it; a `FailbackDelay` of at least that long closes the gap. + ### Anti-flap tiebreak in selection Member selection ranks candidates by connectivity, explicit override, weight, and latency. When two candidates are otherwise indistinguishable, selection prefers the member that is **already active**, rather than picking arbitrarily. diff --git a/docs/exp/SER011.md b/docs/exp/SER011.md index 0bc9a8cd3..26618ff36 100644 --- a/docs/exp/SER011.md +++ b/docs/exp/SER011.md @@ -7,6 +7,9 @@ The diagnostic covers: - `ConnectionGroupMember.FailbackHealthCheck` and `MultiGroupOptions.FailbackHealthCheck` (and its builder); - `HealthCheckProbe.Scope` and `HealthCheckProbeScope`, for probes that answer per member rather than per endpoint. +- `HealthCheck.LagAware`, `HealthCheckProbe.LagAware`, `LagAwareOptions` and `LagCheckMode`: a probe of the + Redis Enterprise database availability API (see + [Redis Enterprise: lag-aware availability](../Failover.md#redis-enterprise-lag-aware-availability)). It is experimental for two reasons: @@ -14,9 +17,11 @@ It is experimental for two reasons: the active member, and when no live member passes it, selection falls back to liveness rather than refusing to serve. Both choices follow from measurements against real Active-Active deployments, where a partition between regions leaves every member behind at once; they may still be refined. -2. **The probe it was built for is still to come.** The intended use is Redis Enterprise's lag-aware - availability API, through a probe that has not shipped yet. Until it has, and has been exercised against - real deployments, the shape of this API may change. +2. **The Redis Enterprise API it talks to is documented as prose.** The probe is built against responses + captured from real deployments (Redis Enterprise 8.0), not against a published schema, and several of the + API's behaviours (the lag check only applying to Active-Active databases, a broken link going unnoticed + for about a minute) were found by measurement rather than documentation. The options and their defaults + may change as more deployments are exercised. If you accept the above, you can suppress this warning by adding the following to your `csproj` file: diff --git a/src/StackExchange.Redis/Availability/HealthCheck.LagAware.cs b/src/StackExchange.Redis/Availability/HealthCheck.LagAware.cs new file mode 100644 index 000000000..d5b789249 --- /dev/null +++ b/src/StackExchange.Redis/Availability/HealthCheck.LagAware.cs @@ -0,0 +1,20 @@ +using System.Diagnostics.CodeAnalysis; +using RESPite; + +namespace StackExchange.Redis.Availability; + +public sealed partial class HealthCheck +{ + /// + /// A health check that asks the Redis Enterprise database availability API, once per member and once per + /// pass, whether the database is available and (for an Active-Active member) caught up; intended as a + /// . See . + /// + /// + /// For different probe counts or timeouts, use + /// new HealthCheck.Builder { Probe = HealthCheckProbe.LagAware(options), ... }. + /// + [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] + public static HealthCheck LagAware(LagAwareOptions options) + => new Builder { Probe = HealthCheckProbe.LagAware(options), ProbeCount = 1 }.Create(); +} diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs b/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs new file mode 100644 index 000000000..cbf664fed --- /dev/null +++ b/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs @@ -0,0 +1,226 @@ +using System; +using System.Collections.Concurrent; +using System.Diagnostics.CodeAnalysis; +using System.IO; +using System.Net; +using System.Net.Http; +using System.Net.Http.Headers; +using System.Net.Security; +using System.Text; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using RESPite; + +namespace StackExchange.Redis.Availability; + +public abstract partial class HealthCheckProbe +{ + /// + /// Ask the Redis Enterprise database availability API whether the member's database is available and, + /// for an Active-Active member, caught up; see . Runs once per member. + /// + /// + /// Healthy on HTTP 200; unhealthy on 503 (unavailable or lagging) or 404 (no such database); + /// when the question could not be asked (the REST API + /// unreachable, credentials rejected, the database not found by discovery), so that a management-plane + /// problem is never mistaken for the database being down. Intended as a + /// . + /// + [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] + public static HealthCheckProbe LagAware(LagAwareOptions options) => new LagAwareProbe(options); + + internal sealed class LagAwareProbe : HealthCheckProbe + { + private const int DefaultRestPort = 9443; + + private readonly Func> _credentials; + private readonly Uri? _restEndpoint; + private readonly int? _databaseId; + private readonly LagCheckMode _mode; + private readonly TimeSpan? _tolerance; + private readonly HttpClient _http; + + // per (REST endpoint, database endpoint): one probe can serve every member of a group + private readonly ConcurrentDictionary _discovered = new(StringComparer.OrdinalIgnoreCase); + + private sealed class Discovered + { + public int? Uid; + public bool? ActiveActive; + } + + public LagAwareProbe(LagAwareOptions options) + { + if (options is null) throw new ArgumentNullException(nameof(options)); + _credentials = options.Credentials ?? throw new ArgumentException("Credentials are required.", nameof(options)); + if (options.LagTolerance is { } tolerance && tolerance < TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException(nameof(options), tolerance, "LagTolerance must not be negative."); + } + + _restEndpoint = options.RestEndpoint; + _databaseId = options.DatabaseId; + _mode = options.LagCheck; + _tolerance = options.LagTolerance; + var handler = options.HttpMessageHandlerFactory?.Invoke() ?? CreateHandler(options.CertificateValidation); + _http = new HttpClient(handler) { Timeout = Timeout.InfiniteTimeSpan }; // bounded per call instead + } + + public override HealthCheckProbeScope Scope => HealthCheckProbeScope.Member; + + private static HttpMessageHandler CreateHandler(RemoteCertificateValidationCallback? validation) + { +#if NET + var handler = new SocketsHttpHandler { PooledConnectionLifetime = TimeSpan.FromMinutes(2) }; + if (validation is not null) handler.SslOptions.RemoteCertificateValidationCallback = validation; + return handler; +#elif NET461 + if (validation is not null) + { + throw new PlatformNotSupportedException("Custom certificate validation for the REST API requires .NET Framework 4.7.1 or later."); + } + return new HttpClientHandler(); +#else + var handler = new HttpClientHandler(); + if (validation is not null) + { + handler.ServerCertificateCustomValidationCallback = (request, certificate, chain, errors) => validation(request, certificate, chain, errors); + } + return handler; +#endif + } + + public override async Task CheckHealthAsync(HealthCheckContext context) + { + var multiplexer = context.Server.Multiplexer; + var endpoints = (multiplexer as IInternalConnectionMultiplexer)?.RawConfig.EndPoints; + var configured = endpoints is { Count: > 0 } ? endpoints[0] : null; + if (!Format.TryGetHostPort(configured, out var host, out var port)) + { + return Inconclusive(multiplexer, "(none)", "the member has no configured endpoint"); + } + + var rest = _restEndpoint ?? new UriBuilder(Uri.UriSchemeHttps, host, DefaultRestPort).Uri; + var key = $"{rest}|{host}:{port}"; + var known = _discovered.GetOrAdd(key, static _ => new Discovered()); + + // finish inside the probe timeout, so that "could not ask" is reported as such rather than as the + // Unhealthy a timed-out probe is otherwise given + using var cts = new CancellationTokenSource(Budget(context.ProbeTimeout)); + try + { + var credential = await _credentials(cts.Token).ConfigureAwait(false); + var auth = new AuthenticationHeaderValue( + "Basic", Convert.ToBase64String(Encoding.UTF8.GetBytes($"{credential.UserName}:{credential.Password}"))); + + int? uid = _databaseId ?? known.Uid; + if (uid is null) + { + // filtered: an unfiltered /v1/bdbs carries database passwords, even for db_viewer + using var list = await GetAsync(rest, "v1/bdbs?fields=uid,endpoints", auth, cts.Token).ForAwait(); + if (list.StatusCode != HttpStatusCode.OK) return Inconclusive(multiplexer, rest, $"database discovery answered {(int)list.StatusCode}"); + uid = FindUid(await list.Content.ReadAsStringAsync().ForAwait(), host, port!.Value); + if (uid is null) return Inconclusive(multiplexer, rest, $"no database on this cluster has the endpoint {host}:{port}"); + known.Uid = uid; + } + + bool lag = _mode switch + { + LagCheckMode.Enabled => true, + LagCheckMode.Disabled => false, + _ => known.ActiveActive ?? false, + }; + if (_mode is LagCheckMode.Auto && known.ActiveActive is null) + { + using var bdb = await GetAsync(rest, $"v1/bdbs/{uid}?fields=uid,crdt", auth, cts.Token).ForAwait(); + if (bdb.StatusCode == HttpStatusCode.NotFound) return Forget(key); + if (bdb.StatusCode != HttpStatusCode.OK) return Inconclusive(multiplexer, rest, $"database lookup answered {(int)bdb.StatusCode}"); + lag = IsActiveActive(await bdb.Content.ReadAsStringAsync().ForAwait()); + known.ActiveActive = lag; + } + + var path = $"v1/bdbs/{uid}/availability"; + if (lag) + { + path += "?extend_check=lag"; + if (_tolerance is { } tolerance) path += $"&availability_lag_tolerance_ms={(long)tolerance.TotalMilliseconds}"; + } + + using var response = await GetAsync(rest, path, auth, cts.Token).ForAwait(); + return response.StatusCode switch + { + HttpStatusCode.OK => HealthCheckResult.Healthy, + HttpStatusCode.ServiceUnavailable => HealthCheckResult.Unhealthy, // unavailable, or lagging + HttpStatusCode.NotFound => Forget(key), + _ => Inconclusive(multiplexer, rest, $"availability answered {(int)response.StatusCode}"), + }; + } + catch (Exception ex) when (ex is HttpRequestException or OperationCanceledException or JsonException or IOException) + { + return Inconclusive(multiplexer, rest, ex is OperationCanceledException ? "timed out" : ex.Message); + } + } + + // the database has gone (or the configured uid is wrong): unhealthy, and rediscover next time + private HealthCheckResult Forget(string key) + { + _discovered.TryRemove(key, out _); + return HealthCheckResult.Unhealthy; + } + + private static TimeSpan Budget(TimeSpan probeTimeout) + { + var budget = TimeSpan.FromTicks(probeTimeout.Ticks / 10 * 9); + return budget > TimeSpan.Zero ? budget : TimeSpan.FromMilliseconds(1); + } + + private async Task GetAsync(Uri rest, string path, AuthenticationHeaderValue auth, CancellationToken cancellationToken) + { + using var request = new HttpRequestMessage(HttpMethod.Get, new Uri(rest, path)); + request.Headers.Authorization = auth; + return await _http.SendAsync(request, cancellationToken).ForAwait(); + } + + private static HealthCheckResult Inconclusive(IConnectionMultiplexer multiplexer, object rest, string reason) + { + (multiplexer as ConnectionMultiplexer)?.Logger?.LogWarningLagAwareInconclusive(rest.ToString() ?? "", reason); + return HealthCheckResult.Inconclusive; + } + + // GET /v1/bdbs?fields=uid,endpoints => [{"uid":1,"endpoints":[{"dns_name":"redis-1.example","port":1,"addr":["10.0.0.1"]}]}] + internal static int? FindUid(string json, string host, int port) + { + using var document = JsonDocument.Parse(json); + if (document.RootElement.ValueKind != JsonValueKind.Array) return null; + foreach (var bdb in document.RootElement.EnumerateArray()) + { + if (!bdb.TryGetProperty("uid", out var uid) || !uid.TryGetInt32(out var id)) continue; + if (!bdb.TryGetProperty("endpoints", out var endpoints) || endpoints.ValueKind != JsonValueKind.Array) continue; + foreach (var endpoint in endpoints.EnumerateArray()) + { + if (!endpoint.TryGetProperty("port", out var p) || !p.TryGetInt32(out var endpointPort) || endpointPort != port) continue; + if (endpoint.TryGetProperty("dns_name", out var dns) && string.Equals(dns.GetString(), host, StringComparison.OrdinalIgnoreCase)) return id; + if (endpoint.TryGetProperty("addr", out var addrs) && addrs.ValueKind == JsonValueKind.Array) + { + foreach (var addr in addrs.EnumerateArray()) + { + if (string.Equals(addr.GetString(), host, StringComparison.OrdinalIgnoreCase)) return id; + } + } + } + } + + return null; + } + + // GET /v1/bdbs/{uid}?fields=uid,crdt => {"crdt":true,"uid":1} + internal static bool IsActiveActive(string json) + { + using var document = JsonDocument.Parse(json); + return document.RootElement.ValueKind == JsonValueKind.Object + && document.RootElement.TryGetProperty("crdt", out var crdt) + && crdt.ValueKind == JsonValueKind.True; + } + } +} diff --git a/src/StackExchange.Redis/Availability/LagAwareOptions.cs b/src/StackExchange.Redis/Availability/LagAwareOptions.cs new file mode 100644 index 000000000..57755604f --- /dev/null +++ b/src/StackExchange.Redis/Availability/LagAwareOptions.cs @@ -0,0 +1,107 @@ +using System; +using System.Diagnostics.CodeAnalysis; +using System.Net; +using System.Net.Http; +using System.Net.Security; +using System.Security.Cryptography.X509Certificates; +using System.Threading; +using System.Threading.Tasks; +using RESPite; + +namespace StackExchange.Redis.Availability; + +/// +/// Configures a health check against the Redis Enterprise database availability API, optionally asking +/// whether the database has caught up with its Active-Active peers; see . +/// +/// +/// Only is required. The REST endpoint, the database id and whether the database is +/// an Active-Active member are discovered, and no lag tolerance is sent unless one is set, so the cluster's +/// own setting applies. The options are read when the probe is created; later changes have no effect on it. +/// +[Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] +public sealed class LagAwareOptions +{ + /// + /// Supplies the cluster management credentials, called for every request so that they can rotate. + /// + /// + /// These are the cluster's REST API credentials, not the database's. A role with + /// management: db_viewer is sufficient. + /// + public Func>? Credentials { get; set; } + + /// + /// The cluster REST API to ask; when (the default), https://{host}:9443/, + /// where {host} is the member's first configured endpoint. + /// + public Uri? RestEndpoint { get; set; } + + /// + /// The database id (uid) on that cluster; when (the default), it is found by + /// matching the member's first configured endpoint against the cluster's databases. + /// + public int? DatabaseId { get; set; } + + /// + /// Whether to ask the lag question; (the default) asks it only of + /// Active-Active databases, since the server reports any other database as unavailable when asked. + /// + public LagCheckMode LagCheck { get; set; } + + /// + /// The lag tolerance to request; when (the default), none is sent and the cluster's + /// own availability_lag_tolerance_ms applies. + /// + public TimeSpan? LagTolerance { get; set; } + + /// + /// Validates the REST API's certificate; when , the platform's default validation + /// applies. + /// + /// + /// The management certificate of a Redis Enterprise cluster is self-signed by the cluster unless one has + /// been installed, so this is usually needed; see . Not supported on .NET + /// Framework 4.6.1. + /// + public RemoteCertificateValidationCallback? CertificateValidation { get; set; } + + /// + /// Trust certificates issued by the given certificate (which may be the cluster's own self-signed + /// certificate), as does for the data plane. + /// + public void TrustIssuer(string issuerCertificatePath) + => CertificateValidation = ConfigurationOptions.TrustIssuerCallback(issuerCertificatePath); + + /// + /// Trust certificates issued by the given certificate (which may be the cluster's own self-signed + /// certificate), as does for the data plane. + /// + public void TrustIssuer(X509Certificate2 issuer) + => CertificateValidation = ConfigurationOptions.TrustIssuerCallback(issuer); + + // test seam: replaces the HTTP transport entirely, so tests can answer requests without a listener + internal Func? HttpMessageHandlerFactory { get; set; } +} + +/// +/// Whether a check asks the lag question. +/// +[Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] +public enum LagCheckMode +{ + /// + /// Ask only if the database is an Active-Active member; otherwise check plain availability. + /// + Auto = 0, + + /// + /// Always ask. A database that is not an Active-Active member is then always reported unavailable. + /// + Enabled = 1, + + /// + /// Never ask; check plain availability only. + /// + Disabled = 2, +} diff --git a/src/StackExchange.Redis/ConfigurationOptions.cs b/src/StackExchange.Redis/ConfigurationOptions.cs index d0eae12df..b4b2502b1 100644 --- a/src/StackExchange.Redis/ConfigurationOptions.cs +++ b/src/StackExchange.Redis/ConfigurationOptions.cs @@ -591,7 +591,7 @@ internal static RemoteCertificateValidationCallback TrustIssuerCallback(string i => TrustIssuerCallback(new X509Certificate2(issuerCertificatePath)); #pragma warning restore SYSLIB0057 // X509 loading - private static RemoteCertificateValidationCallback TrustIssuerCallback(X509Certificate2 issuer) + internal static RemoteCertificateValidationCallback TrustIssuerCallback(X509Certificate2 issuer) { if (issuer == null) throw new ArgumentNullException(nameof(issuer)); diff --git a/src/StackExchange.Redis/LoggerExtensions.cs b/src/StackExchange.Redis/LoggerExtensions.cs index b8abcea8f..2f4bff50c 100644 --- a/src/StackExchange.Redis/LoggerExtensions.cs +++ b/src/StackExchange.Redis/LoggerExtensions.cs @@ -839,4 +839,10 @@ internal static void LogWithThreadPoolStats(this ILogger? log, string message) EventId = 120, Message = "{Server}: Re-reading topology after {Failures} consecutive connect failures")] internal static partial void LogInformationRefreshingAfterConnectFailures(this ILogger logger, ServerEndPointLogValue server, int failures); + + [LoggerMessage( + Level = LogLevel.Warning, + EventId = 124, + Message = "Lag-aware health check could not ask {RestEndpoint}: {Reason}")] + internal static partial void LogWarningLagAwareInconclusive(this ILogger logger, string restEndpoint, string reason); } diff --git a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt index 353476b08..edb08ced2 100644 --- a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt +++ b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt @@ -8,3 +8,25 @@ [SER011]StackExchange.Redis.Availability.MultiGroupOptions.Builder.FailbackHealthCheck.set -> void [SER011]StackExchange.Redis.Availability.MultiGroupOptions.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? [SER011]virtual StackExchange.Redis.Availability.HealthCheckProbe.Scope.get -> StackExchange.Redis.Availability.HealthCheckProbeScope +[SER011]StackExchange.Redis.Availability.LagAwareOptions +[SER011]StackExchange.Redis.Availability.LagAwareOptions.CertificateValidation.get -> System.Net.Security.RemoteCertificateValidationCallback? +[SER011]StackExchange.Redis.Availability.LagAwareOptions.CertificateValidation.set -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.Credentials.get -> System.Func>? +[SER011]StackExchange.Redis.Availability.LagAwareOptions.Credentials.set -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.DatabaseId.get -> int? +[SER011]StackExchange.Redis.Availability.LagAwareOptions.DatabaseId.set -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagAwareOptions() -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagCheck.get -> StackExchange.Redis.Availability.LagCheckMode +[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagCheck.set -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagTolerance.get -> System.TimeSpan? +[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagTolerance.set -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.RestEndpoint.get -> System.Uri? +[SER011]StackExchange.Redis.Availability.LagAwareOptions.RestEndpoint.set -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.TrustIssuer(string! issuerCertificatePath) -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.TrustIssuer(System.Security.Cryptography.X509Certificates.X509Certificate2! issuer) -> void +[SER011]StackExchange.Redis.Availability.LagCheckMode +[SER011]StackExchange.Redis.Availability.LagCheckMode.Auto = 0 -> StackExchange.Redis.Availability.LagCheckMode +[SER011]StackExchange.Redis.Availability.LagCheckMode.Disabled = 2 -> StackExchange.Redis.Availability.LagCheckMode +[SER011]StackExchange.Redis.Availability.LagCheckMode.Enabled = 1 -> StackExchange.Redis.Availability.LagCheckMode +[SER011]static StackExchange.Redis.Availability.HealthCheck.LagAware(StackExchange.Redis.Availability.LagAwareOptions! options) -> StackExchange.Redis.Availability.HealthCheck! +[SER011]static StackExchange.Redis.Availability.HealthCheckProbe.LagAware(StackExchange.Redis.Availability.LagAwareOptions! options) -> StackExchange.Redis.Availability.HealthCheckProbe! diff --git a/src/StackExchange.Redis/StackExchange.Redis.csproj b/src/StackExchange.Redis/StackExchange.Redis.csproj index 4a805b866..883ed9b20 100644 --- a/src/StackExchange.Redis/StackExchange.Redis.csproj +++ b/src/StackExchange.Redis/StackExchange.Redis.csproj @@ -32,6 +32,10 @@ + + + + diff --git a/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs b/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs new file mode 100644 index 000000000..1189b52b5 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs @@ -0,0 +1,303 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using StackExchange.Redis.Availability; +using StackExchange.Redis.Tests.MultiGroupTests; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// The lag-aware probe against a stubbed REST transport. Response shapes are the ones captured from real +/// Redis Enterprise 8.0.22 deployments (see notes/lag-aware/findings.md, section 10), minus credentials. +/// +public class LagAwareProbeTests(ITestOutputHelper log) +{ + private const string Host = "redis-14460.c1.example.com"; + private const int Port = 14460; + + private const string BdbList = """ + [{"endpoints":[{"addr":["54.243.16.166"],"addr_type":"external","dns_name":"redis-14460.c1.example.com","oss_cluster_api_preferred_endpoint_type":"ip","oss_cluster_api_preferred_ip_type":"internal","port":14460,"proxy_policy":"single","uid":"1:1"}],"uid":1}, + {"endpoints":[{"addr":["100.53.190.50"],"addr_type":"external","dns_name":"redis-12706.c1.example.com","oss_cluster_api_preferred_endpoint_type":"ip","oss_cluster_api_preferred_ip_type":"external","port":12706,"proxy_policy":"all-master-shards","uid":"2:1"}],"uid":2}] + """; + + private const string ActiveActive = """{"crdt":true,"uid":1}"""; + private const string NotActiveActive = """{"crdt":false,"uid":1}"""; + private const string Unavailable = """{"description":"BDB 1 is not available","error_code":"bdb_unavailable"}"""; + + // answers by path-and-query; records every request + private sealed class StubHandler(Func> respond) : HttpMessageHandler + { + public StubHandler(Func answer) + : this((request, _) => + { + var (status, body) = answer(request.RequestUri!.PathAndQuery); + var response = new HttpResponseMessage(status); + if (body is not null) response.Content = new StringContent(body, Encoding.UTF8, "application/json"); + return Task.FromResult(response); + }) + { + } + + public ConcurrentQueue Requests { get; } = new(); + + public IEnumerable Paths => Requests.Select(r => r.RequestUri!.PathAndQuery); + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + Requests.Enqueue(request); + return respond(request, cancellationToken); + } + } + + // the cluster as measured: discovery, crdt lookup, then the availability answer + private static Func Cluster(bool activeActive, HttpStatusCode availability, string? availabilityBody = null) + => path => path switch + { + "/v1/bdbs?fields=uid,endpoints" => (HttpStatusCode.OK, BdbList), + "/v1/bdbs/1?fields=uid,crdt" => (HttpStatusCode.OK, activeActive ? ActiveActive : NotActiveActive), + _ when path.StartsWith("/v1/bdbs/1/availability", StringComparison.Ordinal) => (availability, availabilityBody), + _ => (HttpStatusCode.NotFound, null), + }; + + private int _credentialCalls; + + private LagAwareOptions Options(StubHandler handler) => new() + { + Credentials = _ => + { + Interlocked.Increment(ref _credentialCalls); + return new(new NetworkCredential("probe@example.com", "s3cret")); + }, + HttpMessageHandlerFactory = () => handler, + }; + + private async Task<(HealthCheckResult Result, StubHandler Handler)> CheckOnceAsync( + StubHandler handler, LagAwareOptions? options = null, TimeSpan? timeout = null, HealthCheckProbe? probe = null) + { + using var server = new InProcessTestServer(log, endpoint: new DnsEndPoint(Host, Port)); + await using var conn = await server.ConnectAsync(); + probe ??= HealthCheckProbe.LagAware(options ?? Options(handler)); + var context = new HealthCheckContext(conn.GetServer(new DnsEndPoint(Host, Port)), timeout ?? TimeSpan.FromSeconds(3)); + return (await probe.CheckHealthAsync(context), handler); + } + + [Fact] + public void ProbeIsMemberScoped() + => Assert.Equal(HealthCheckProbeScope.Member, HealthCheckProbe.LagAware(Options(new StubHandler(Cluster(true, HttpStatusCode.OK)))).Scope); + + [Fact] + public async Task DiscoversTheDatabaseThenAsksTheLagQuestionOfAnActiveActiveMember() + { + var (result, handler) = await CheckOnceAsync(new StubHandler(Cluster(activeActive: true, HttpStatusCode.OK))); + + Assert.Equal(HealthCheckResult.Healthy, result); + Assert.Equal( + ["/v1/bdbs?fields=uid,endpoints", "/v1/bdbs/1?fields=uid,crdt", "/v1/bdbs/1/availability?extend_check=lag"], + handler.Paths); + + // derived from the configured database host; Basic auth on every request + Assert.All(handler.Requests, r => + { + Assert.Equal($"https://{Host}:9443", r.RequestUri!.GetLeftPart(UriPartial.Authority)); + Assert.Equal("Basic", r.Headers.Authorization?.Scheme); + Assert.Equal(Convert.ToBase64String(Encoding.UTF8.GetBytes("probe@example.com:s3cret")), r.Headers.Authorization?.Parameter); + }); + } + + [Fact] + public async Task NeverIssuesAnUnfilteredDatabaseRead() + { + // measured: unfiltered /v1/bdbs and /v1/crdbs return database passwords, even to db_viewer + var (_, handler) = await CheckOnceAsync(new StubHandler(Cluster(activeActive: true, HttpStatusCode.OK))); + Assert.All(handler.Paths, path => + { + Assert.DoesNotContain("/v1/crdbs", path); + if (!path.Contains("/availability")) Assert.Contains("fields=", path); + }); + } + + [Fact] + public async Task ChecksPlainAvailabilityOfADatabaseThatIsNotActiveActive() + { + // measured: the lag check reports every non-Active-Active database unavailable, whatever the tolerance + var (result, handler) = await CheckOnceAsync(new StubHandler(Cluster(activeActive: false, HttpStatusCode.OK))); + Assert.Equal(HealthCheckResult.Healthy, result); + Assert.Equal("/v1/bdbs/1/availability", handler.Paths.Last()); + } + + [Fact] + public async Task DiscoveryIsCachedAcrossPassesAndCredentialsAreReadEachTime() + { + var handler = new StubHandler(Cluster(activeActive: true, HttpStatusCode.OK)); + var probe = HealthCheckProbe.LagAware(Options(handler)); + await CheckOnceAsync(handler, probe: probe); + await CheckOnceAsync(handler, probe: probe); + + Assert.Equal(4, handler.Requests.Count); // three, then just the availability call + Assert.Equal("/v1/bdbs/1/availability?extend_check=lag", handler.Paths.Last()); + Assert.Equal(2, _credentialCalls); // rotation: asked once per check + } + + [Theory] + [InlineData(HttpStatusCode.OK, HealthCheckResult.Healthy)] + [InlineData(HttpStatusCode.ServiceUnavailable, HealthCheckResult.Unhealthy)] // unavailable, or lagging + [InlineData(HttpStatusCode.NotFound, HealthCheckResult.Unhealthy)] + [InlineData(HttpStatusCode.Unauthorized, HealthCheckResult.Inconclusive)] // management plane, not the database + [InlineData(HttpStatusCode.Forbidden, HealthCheckResult.Inconclusive)] + [InlineData(HttpStatusCode.InternalServerError, HealthCheckResult.Inconclusive)] + public async Task MapsTheAvailabilityAnswer(HttpStatusCode status, HealthCheckResult expected) + { + var (result, _) = await CheckOnceAsync(new StubHandler(Cluster(activeActive: true, status, status == HttpStatusCode.ServiceUnavailable ? Unavailable : null))); + Assert.Equal(expected, result); + } + + [Fact] + public async Task NotFoundForgetsTheDiscoveredDatabase() + { + var gone = false; + var handler = new StubHandler(path => gone && path.Contains("/availability") ? (HttpStatusCode.NotFound, null) : Cluster(true, HttpStatusCode.OK)(path)); + var probe = HealthCheckProbe.LagAware(Options(handler)); + Assert.Equal(HealthCheckResult.Healthy, (await CheckOnceAsync(handler, probe: probe)).Result); + + gone = true; + Assert.Equal(HealthCheckResult.Unhealthy, (await CheckOnceAsync(handler, probe: probe)).Result); + + gone = false; + await CheckOnceAsync(handler, probe: probe); + Assert.Equal(2, handler.Paths.Count(p => p == "/v1/bdbs?fields=uid,endpoints")); // rediscovered + } + + [Fact] + public async Task TransportFailureIsInconclusive() + { + var (result, _) = await CheckOnceAsync(new StubHandler((_, _) => throw new HttpRequestException("connection refused"))); + Assert.Equal(HealthCheckResult.Inconclusive, result); + } + + [Fact] + public async Task ASlowAnswerIsInconclusiveWithinTheProbeTimeout() + { + var handler = new StubHandler(async (_, cancellationToken) => + { + await Task.Delay(TimeSpan.FromSeconds(30), cancellationToken); + return new HttpResponseMessage(HttpStatusCode.OK); + }); + var watch = System.Diagnostics.Stopwatch.StartNew(); + var (result, _) = await CheckOnceAsync(handler, timeout: TimeSpan.FromMilliseconds(300)); + Assert.Equal(HealthCheckResult.Inconclusive, result); + Assert.True(watch.Elapsed < TimeSpan.FromSeconds(10), $"took {watch.Elapsed}"); + } + + [Fact] + public async Task ADatabaseThatCannotBeFoundIsInconclusive() + { + var (result, handler) = await CheckOnceAsync(new StubHandler(path => (HttpStatusCode.OK, path.Contains("fields=uid,endpoints") ? "[]" : null))); + Assert.Equal(HealthCheckResult.Inconclusive, result); + Assert.Single(handler.Requests); + } + + [Fact] + public async Task ExplicitOptionsSkipDiscoveryAndSendTheTolerance() + { + var handler = new StubHandler(_ => (HttpStatusCode.OK, null)); + var options = Options(handler); + options.RestEndpoint = new Uri("https://cluster.example.com:9443/"); + options.DatabaseId = 7; + options.LagCheck = LagCheckMode.Enabled; + options.LagTolerance = TimeSpan.FromMilliseconds(250); + + var (result, _) = await CheckOnceAsync(handler, options); + Assert.Equal(HealthCheckResult.Healthy, result); + var request = Assert.Single(handler.Requests); + Assert.Equal("https://cluster.example.com:9443/v1/bdbs/7/availability?extend_check=lag&availability_lag_tolerance_ms=250", request.RequestUri!.ToString()); + } + + [Fact] + public async Task DisabledNeverAsksTheLagQuestion() + { + var handler = new StubHandler(Cluster(activeActive: true, HttpStatusCode.OK)); + var options = Options(handler); + options.LagCheck = LagCheckMode.Disabled; + var (_, _) = await CheckOnceAsync(handler, options); + Assert.DoesNotContain(handler.Paths, p => p.Contains("extend_check") || p.Contains("crdt")); + } + + [Fact] + public void FindsTheDatabaseByAddressAsWellAsByName() + => Assert.Equal(2, HealthCheckProbe.LagAwareProbe.FindUid(BdbList, "100.53.190.50", 12706)); + + [Fact] + public void RejectsInvalidOptions() + { + Assert.Throws(() => HealthCheckProbe.LagAware(null!)); + Assert.Throws(() => HealthCheckProbe.LagAware(new LagAwareOptions())); + Assert.Throws(() => HealthCheckProbe.LagAware(new LagAwareOptions + { + Credentials = _ => new(new NetworkCredential()), + LagTolerance = TimeSpan.FromMilliseconds(-1), + })); + } + + [Fact] + public async Task AsAFailbackCheckItKeepsTrafficOffALaggingMember() + { + // end to end at the stub level: beta has the higher weight, but its cluster reports it lagging + var alphaEp = new DnsEndPoint("redis-14460.c1.example.com", 14460); + var betaEp = new DnsEndPoint("redis-14460.c2.example.com", 14460); + using var alpha = new InProcessTestServer(log, endpoint: alphaEp); + using var beta = new InProcessTestServer(log, endpoint: betaEp); + + var handler = new StubHandler((request, _) => + { + var path = request.RequestUri!.PathAndQuery; + var host = request.RequestUri.Host; + (HttpStatusCode Status, string? Body) answer = path switch + { + "/v1/bdbs?fields=uid,endpoints" => (HttpStatusCode.OK, BdbList.Replace(".c1.", host.Contains(".c2.") ? ".c2." : ".c1.")), + "/v1/bdbs/1?fields=uid,crdt" => (HttpStatusCode.OK, ActiveActive), + _ => host.Contains(".c2.") ? (HttpStatusCode.ServiceUnavailable, Unavailable) : (HttpStatusCode.OK, null), + }; + var response = new HttpResponseMessage(answer.Status); + if (answer.Body is not null) response.Content = new StringContent(answer.Body); + return Task.FromResult(response); + }); + + // liveness held down until both members are connected, so the first real selection sees the whole group + var gate = new GateProbe(); + MultiGroupOptions groupOptions = new MultiGroupOptions.Builder + { + HealthCheck = new HealthCheck.Builder { Probe = gate, ProbeCount = 1 }, + FailbackHealthCheck = HealthCheck.LagAware(Options(handler)), + HealthCheckInterval = TimeSpan.MaxValue, + }; + ConnectionGroupMember[] members = [new(alpha.GetClientConfig(), "alpha") { Weight = 1 }, new(beta.GetClientConfig(), "beta") { Weight = 9 }]; + await using var group = await ConnectionMultiplexer.ConnectGroupAsync(members, groupOptions); + + var watch = System.Diagnostics.Stopwatch.StartNew(); + while (!Array.TrueForAll(members, m => m.Multiplexer.IsConnected) && watch.Elapsed < TimeSpan.FromSeconds(10)) + { + await Task.Delay(25, TestContext.Current.CancellationToken); + } + gate.Open = true; + await MultiGroupMultiplexer.TryHealthCheckAndSelectPreferredGroupAsync(group); + await GroupWait.AssertConnectedAsync(group); + + Assert.Equal("alpha", group.ActiveMember?.Name); + Assert.False(members[1].FailbackVerified); + Assert.Contains(handler.Requests, r => r.RequestUri!.Host == betaEp.Host && r.RequestUri.AbsolutePath == "/v1/bdbs/1/availability"); + } + + private sealed class GateProbe : HealthCheckProbe + { + public volatile bool Open; + + public override Task CheckHealthAsync(HealthCheckContext context) => Open ? HealthyTask : UnhealthyTask; + } +} From 2c647d7b2d3daa962a23f7352603b051ae7f1442 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 13:24:46 +0100 Subject: [PATCH 20/34] Notes: probe implemented; RawConfig is not public after all --- notes/lag-aware/api-sketch.md | 5 ++++- notes/lag-aware/findings.md | 5 +++++ notes/lag-aware/plan.md | 5 +++-- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index 69efe9a90..a3eefdd56 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -1,6 +1,9 @@ # Lag-aware availability: API sketch -Status: **sections 1 and 2 implemented** (2026-10-08); the probe (3) and onwards are still a sketch. +Status: **sections 1 to 3 implemented** (2026-10-08): the failback check, member-scoped probes, and the +lag-aware probe (`HealthCheck.LagAware`, `HealthCheckProbe.LagAware`, `LagAwareOptions`, `LagCheckMode`), +with stubbed-transport tests replaying the measured response shapes. Not yet: client-certificate support for +the REST call, and the fault-injector scenario test. Evidence is in [`findings.md`](findings.md) (section 10 especially); sequence in [`plan.md`](plan.md). *Correction:* `SER007` was retired when geo-redundant failover went GA (#3247), so the shipped `Availability/` API is not experimental; everything new here is behind a new diagnostic, **`SER011`** diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index 555b1151c..7d8ac29cb 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -469,6 +469,11 @@ Open question 5 below asked whether `HealthCheckContext` carries enough for a re external probe can already reach `context.Server.Multiplexer.RawConfig.Tunnel` with no change to `HealthCheckContext` at all. +*Corrected 2026-10-08:* `RawConfig` is **not** public. The declaration found was on the internal +`IInternalConnectionMultiplexer` in the same file; the public `IConnectionMultiplexer` does not expose it. +So an external probe cannot reach the member's configured endpoints, which the lag-aware probe needs to +derive the REST endpoint and discover the database - one more reason it lives in this assembly. + ## 9. Open questions 1. **Which tolerance default?** 100 ms (server, docs, redis-py) or 5000 ms (Lettuce)? Worth asking diff --git a/notes/lag-aware/plan.md b/notes/lag-aware/plan.md index e66cd0771..130135269 100644 --- a/notes/lag-aware/plan.md +++ b/notes/lag-aware/plan.md @@ -6,8 +6,9 @@ sequence. Status: **proposed. Step 0 is cleared; step 1 is answered (findings §10): the lag check flips under real Active-Active lag, on the stale member only, and the tolerance is honoured on a running link. Cross-region lag magnitudes remain unmeasured (both test clusters share a region).** The core change -(failback health check, member-scoped probes, selection rules) is implemented behind `SER011`; the REST -probe itself is next. See [`api-sketch.md`](api-sketch.md). +(failback health check, member-scoped probes, selection rules) and the REST probe are implemented behind +`SER011`, with stubbed-transport tests (step 5, tier 1); next is the fault-injector scenario test (tier 3). +See [`api-sketch.md`](api-sketch.md). > **This branch is not only a planning branch.** `marc/lag-aware-availability` carries these notes > now, and is the feature branch from here — the implementation lands on top of the same branch and From 602d94a4f0116ed84802094dcbdde3b1133b7fba Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 13:38:36 +0100 Subject: [PATCH 21/34] LagAware: ask the cluster name, and never follow redirects Found against live clusters: non-master nodes answer the management routes (discovery, the crdt lookup) with a 307 to the master's internal address, so deriving the REST endpoint from the database host timed out for three of four members. Derive the cluster FQDN (strip redis-{port}.), which resolves to the master, and report a redirect as Inconclusive rather than following it. --- docs/Failover.md | 7 ++-- .../Availability/HealthCheckProbe.LagAware.cs | 33 +++++++++++++---- .../Availability/LagAwareOptions.cs | 10 ++++-- .../LagAwareProbeTests.cs | 36 +++++++++++++++---- 4 files changed, 69 insertions(+), 17 deletions(-) diff --git a/docs/Failover.md b/docs/Failover.md index 1e7177014..6fd664707 100644 --- a/docs/Failover.md +++ b/docs/Failover.md @@ -812,8 +812,11 @@ ConnectionGroupMember member = new("redis-14460.us-west.example.com:14460,passwo What it does by default, and why: -- **The REST endpoint is derived** from the member's first configured endpoint, as `https://{host}:9443/`; - set `RestEndpoint` if the database is reached through a name the cluster's certificate does not cover. +- **The REST endpoint is derived** from the member's first configured endpoint: a database named + `redis-{port}.{cluster}` (the form Redis Enterprise assigns) is asked via `https://{cluster}:9443/`. The + cluster name matters, because only the cluster's master answers every route the check uses; other nodes + redirect to its internal address, which is usually unreachable from outside the cluster's network. + Redirects are never followed. Set `RestEndpoint` if the database is reached through a name of your own. - **The database is found** by matching that endpoint against the cluster's databases, using only filtered requests (unfiltered database listings include database passwords, even for read-only roles); set `DatabaseId` to skip this. diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs b/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs index cbf664fed..29bdc2d0c 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs @@ -72,7 +72,8 @@ public LagAwareProbe(LagAwareOptions options) private static HttpMessageHandler CreateHandler(RemoteCertificateValidationCallback? validation) { #if NET - var handler = new SocketsHttpHandler { PooledConnectionLifetime = TimeSpan.FromMinutes(2) }; + // redirects point at a node's internal address, so following one only ever hangs; report it instead + var handler = new SocketsHttpHandler { PooledConnectionLifetime = TimeSpan.FromMinutes(2), AllowAutoRedirect = false }; if (validation is not null) handler.SslOptions.RemoteCertificateValidationCallback = validation; return handler; #elif NET461 @@ -80,9 +81,9 @@ private static HttpMessageHandler CreateHandler(RemoteCertificateValidationCallb { throw new PlatformNotSupportedException("Custom certificate validation for the REST API requires .NET Framework 4.7.1 or later."); } - return new HttpClientHandler(); + return new HttpClientHandler { AllowAutoRedirect = false }; #else - var handler = new HttpClientHandler(); + var handler = new HttpClientHandler { AllowAutoRedirect = false }; if (validation is not null) { handler.ServerCertificateCustomValidationCallback = (request, certificate, chain, errors) => validation(request, certificate, chain, errors); @@ -101,7 +102,7 @@ public override async Task CheckHealthAsync(HealthCheckContex return Inconclusive(multiplexer, "(none)", "the member has no configured endpoint"); } - var rest = _restEndpoint ?? new UriBuilder(Uri.UriSchemeHttps, host, DefaultRestPort).Uri; + var rest = _restEndpoint ?? new UriBuilder(Uri.UriSchemeHttps, ClusterHost(host, port.Value), DefaultRestPort).Uri; var key = $"{rest}|{host}:{port}"; var known = _discovered.GetOrAdd(key, static _ => new Discovered()); @@ -119,7 +120,7 @@ public override async Task CheckHealthAsync(HealthCheckContex { // filtered: an unfiltered /v1/bdbs carries database passwords, even for db_viewer using var list = await GetAsync(rest, "v1/bdbs?fields=uid,endpoints", auth, cts.Token).ForAwait(); - if (list.StatusCode != HttpStatusCode.OK) return Inconclusive(multiplexer, rest, $"database discovery answered {(int)list.StatusCode}"); + if (list.StatusCode != HttpStatusCode.OK) return Inconclusive(multiplexer, rest, Describe("database discovery", list)); uid = FindUid(await list.Content.ReadAsStringAsync().ForAwait(), host, port!.Value); if (uid is null) return Inconclusive(multiplexer, rest, $"no database on this cluster has the endpoint {host}:{port}"); known.Uid = uid; @@ -135,7 +136,7 @@ public override async Task CheckHealthAsync(HealthCheckContex { using var bdb = await GetAsync(rest, $"v1/bdbs/{uid}?fields=uid,crdt", auth, cts.Token).ForAwait(); if (bdb.StatusCode == HttpStatusCode.NotFound) return Forget(key); - if (bdb.StatusCode != HttpStatusCode.OK) return Inconclusive(multiplexer, rest, $"database lookup answered {(int)bdb.StatusCode}"); + if (bdb.StatusCode != HttpStatusCode.OK) return Inconclusive(multiplexer, rest, Describe("database lookup", bdb)); lag = IsActiveActive(await bdb.Content.ReadAsStringAsync().ForAwait()); known.ActiveActive = lag; } @@ -153,7 +154,7 @@ public override async Task CheckHealthAsync(HealthCheckContex HttpStatusCode.OK => HealthCheckResult.Healthy, HttpStatusCode.ServiceUnavailable => HealthCheckResult.Unhealthy, // unavailable, or lagging HttpStatusCode.NotFound => Forget(key), - _ => Inconclusive(multiplexer, rest, $"availability answered {(int)response.StatusCode}"), + _ => Inconclusive(multiplexer, rest, Describe("availability", response)), }; } catch (Exception ex) when (ex is HttpRequestException or OperationCanceledException or JsonException or IOException) @@ -162,6 +163,19 @@ public override async Task CheckHealthAsync(HealthCheckContex } } + // Database endpoints are named "redis-{port}.{cluster fqdn}"; ask the cluster name, not the database's. Any + // node answers the availability routes itself, but the others (discovery, the crdt lookup) are answered + // only by the master: other nodes redirect to its *internal* address, unreachable from outside the + // cluster's network. The cluster name resolves to the master. A host that does not follow the pattern + // is used as given. + internal static string ClusterHost(string host, int port) + { + var prefix = $"redis-{port}."; + return host.Length > prefix.Length && host.StartsWith(prefix, StringComparison.OrdinalIgnoreCase) + ? host.Substring(prefix.Length) + : host; + } + // the database has gone (or the configured uid is wrong): unhealthy, and rediscover next time private HealthCheckResult Forget(string key) { @@ -169,6 +183,11 @@ private HealthCheckResult Forget(string key) return HealthCheckResult.Unhealthy; } + private static string Describe(string what, HttpResponseMessage response) + => response.Headers.Location is { } location + ? $"{what} answered {(int)response.StatusCode}, redirecting to {location} (set RestEndpoint to an address that answers directly)" + : $"{what} answered {(int)response.StatusCode}"; + private static TimeSpan Budget(TimeSpan probeTimeout) { var budget = TimeSpan.FromTicks(probeTimeout.Ticks / 10 * 9); diff --git a/src/StackExchange.Redis/Availability/LagAwareOptions.cs b/src/StackExchange.Redis/Availability/LagAwareOptions.cs index 57755604f..83f884511 100644 --- a/src/StackExchange.Redis/Availability/LagAwareOptions.cs +++ b/src/StackExchange.Redis/Availability/LagAwareOptions.cs @@ -32,9 +32,15 @@ public sealed class LagAwareOptions public Func>? Credentials { get; set; } /// - /// The cluster REST API to ask; when (the default), https://{host}:9443/, - /// where {host} is the member's first configured endpoint. + /// The cluster REST API to ask; when (the default), derived from the member's first + /// configured endpoint: https://{cluster}:9443/ for a database named redis-{port}.{cluster} (the + /// form Redis Enterprise assigns), else https://{host}:9443/. /// + /// + /// The cluster name matters: only the cluster's master answers every route the probe uses, and other nodes + /// redirect to its internal address. Redirects are never followed; set this explicitly if the database is + /// reached through a name that does not follow the pattern. + /// public Uri? RestEndpoint { get; set; } /// diff --git a/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs b/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs index 1189b52b5..39225c70b 100644 --- a/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs +++ b/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs @@ -102,10 +102,10 @@ public async Task DiscoversTheDatabaseThenAsksTheLagQuestionOfAnActiveActiveMemb ["/v1/bdbs?fields=uid,endpoints", "/v1/bdbs/1?fields=uid,crdt", "/v1/bdbs/1/availability?extend_check=lag"], handler.Paths); - // derived from the configured database host; Basic auth on every request + // the cluster name, derived from the configured database host; Basic auth on every request Assert.All(handler.Requests, r => { - Assert.Equal($"https://{Host}:9443", r.RequestUri!.GetLeftPart(UriPartial.Authority)); + Assert.Equal("https://c1.example.com:9443", r.RequestUri!.GetLeftPart(UriPartial.Authority)); Assert.Equal("Basic", r.Headers.Authorization?.Scheme); Assert.Equal(Convert.ToBase64String(Encoding.UTF8.GetBytes("probe@example.com:s3cret")), r.Headers.Authorization?.Parameter); }); @@ -229,6 +229,30 @@ public async Task DisabledNeverAsksTheLagQuestion() Assert.DoesNotContain(handler.Paths, p => p.Contains("extend_check") || p.Contains("crdt")); } + [Theory] + [InlineData("redis-14460.c1.example.com", 14460, "c1.example.com")] // the form Redis Enterprise assigns + [InlineData("REDIS-14460.c1.example.com", 14460, "c1.example.com")] + [InlineData("redis-14460.c1.example.com", 6379, "redis-14460.c1.example.com")] // port does not match + [InlineData("cache.example.com", 14460, "cache.example.com")] // a name of the operator's own + [InlineData("redis-14460.", 14460, "redis-14460.")] + public void DerivesTheClusterNameFromTheDatabaseHost(string host, int port, string expected) + => Assert.Equal(expected, HealthCheckProbe.LagAwareProbe.ClusterHost(host, port)); + + [Fact] + public async Task ARedirectIsReportedNotFollowed() + { + // measured: non-master nodes redirect management routes to the master's internal address + var handler = new StubHandler((request, _) => + { + var response = new HttpResponseMessage(HttpStatusCode.TemporaryRedirect); + response.Headers.Location = new Uri("https://10.0.101.184:9443" + request.RequestUri!.PathAndQuery); + return Task.FromResult(response); + }); + var (result, _) = await CheckOnceAsync(handler); + Assert.Equal(HealthCheckResult.Inconclusive, result); + Assert.Single(handler.Requests); + } + [Fact] public void FindsTheDatabaseByAddressAsWellAsByName() => Assert.Equal(2, HealthCheckProbe.LagAwareProbe.FindUid(BdbList, "100.53.190.50", 12706)); @@ -257,12 +281,12 @@ public async Task AsAFailbackCheckItKeepsTrafficOffALaggingMember() var handler = new StubHandler((request, _) => { var path = request.RequestUri!.PathAndQuery; - var host = request.RequestUri.Host; + var isC2 = request.RequestUri.Host == "c2.example.com"; // the cluster names, derived from the members' hosts (HttpStatusCode Status, string? Body) answer = path switch { - "/v1/bdbs?fields=uid,endpoints" => (HttpStatusCode.OK, BdbList.Replace(".c1.", host.Contains(".c2.") ? ".c2." : ".c1.")), + "/v1/bdbs?fields=uid,endpoints" => (HttpStatusCode.OK, isC2 ? BdbList.Replace(".c1.", ".c2.") : BdbList), "/v1/bdbs/1?fields=uid,crdt" => (HttpStatusCode.OK, ActiveActive), - _ => host.Contains(".c2.") ? (HttpStatusCode.ServiceUnavailable, Unavailable) : (HttpStatusCode.OK, null), + _ => isC2 ? (HttpStatusCode.ServiceUnavailable, Unavailable) : (HttpStatusCode.OK, null), }; var response = new HttpResponseMessage(answer.Status); if (answer.Body is not null) response.Content = new StringContent(answer.Body); @@ -291,7 +315,7 @@ public async Task AsAFailbackCheckItKeepsTrafficOffALaggingMember() Assert.Equal("alpha", group.ActiveMember?.Name); Assert.False(members[1].FailbackVerified); - Assert.Contains(handler.Requests, r => r.RequestUri!.Host == betaEp.Host && r.RequestUri.AbsolutePath == "/v1/bdbs/1/availability"); + Assert.Contains(handler.Requests, r => r.RequestUri!.Host == "c2.example.com" && r.RequestUri.AbsolutePath == "/v1/bdbs/1/availability"); } private sealed class GateProbe : HealthCheckProbe From 39507979e1b88c023d7be7b87a7e47137c874368 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 13:39:25 +0100 Subject: [PATCH 22/34] Notes: management routes redirect to the master's internal address --- notes/lag-aware/api-sketch.md | 9 +++++---- notes/lag-aware/findings.md | 8 ++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index a3eefdd56..b00c7be91 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -147,10 +147,11 @@ probe is `Member`-scoped. ### What it does, per pass -1. **Endpoint:** `RestEndpoint`, else `https://:9443`. - Measured: every database's host is `redis-.`, the 9443 certificate's SANs - cover `*.`, and any node answers. Configured endpoint, not discovered nodes: an OSS-cluster - database advertises IP addresses. +1. **Endpoint:** `RestEndpoint`, else derived from the member's first configured endpoint: the cluster + FQDN when the host is `redis-.`, else the host itself. *As built:* the cluster + FQDN, not the database host, because non-master nodes redirect management routes to the master's + internal address (findings section 10); redirects are never followed. Configured endpoint, not + discovered nodes: an OSS-cluster database advertises IP addresses. 2. **uid** (once, cached; cleared on 404, as Lettuce does): `DatabaseId`, else `GET /v1/bdbs?fields=uid,endpoints`, matched on `dns_name` and port. 3. **Active-Active** (once, cached), for `LagCheck = Auto`: `GET /v1/bdbs/{uid}?fields=uid,crdt`. diff --git a/notes/lag-aware/findings.md b/notes/lag-aware/findings.md index 7d8ac29cb..26ccdb84a 100644 --- a/notes/lag-aware/findings.md +++ b/notes/lag-aware/findings.md @@ -636,6 +636,14 @@ Plain checks 200 throughout, both forms, both members; the local form tracked th what it answers on a node not serving the endpoint, so it is no more distinguishable by body than the database form. For lag, the two forms give the same verdict; the difference is call shape (one per database versus one per endpoint, and the local form only works routed by database host). + *Qualified 2026-10-08 (`marcgravell-test-41f58c28`):* routing by database host is fine for the + availability routes, but **not for the management routes**. On any node other than the master, + `/v1/bdbs?fields=...`, `/v1/bdbs/{uid}?fields=...` and `/v1/cluster` answer **307, redirecting to the + master's internal address** (`https://10.0.101.x:9443/...`), while `/availability` is answered directly. + Following that redirect from outside the VPC hangs. The cluster FQDN resolves to the master, which + answers everything. Found by running the real probe against the live clusters: three of four members + timed out until the probe derived the cluster name (strip `redis-{port}.`) instead and stopped following + redirects. *Inferred:* go-redis's discovery, routed via the database host, would meet the same redirect. - **`/v1/bdbs` returns database passwords** (`authentication_redis_pass`, `authentication_admin_pass`) when `mask_bdb_credentials` is false, as on these clusters, and `/v1/crdbs` returns the same for every instance. Captured bodies cannot be committed as fixtures without scrubbing. Relevant to credential From 6d4af4c3e1e4c202eff841a16caa279d54ff4e24 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 13:46:34 +0100 Subject: [PATCH 23/34] Fault-injector scenario: refuse to fail back onto a stale Active-Active member Pauses sync into the preferred member through its cluster's REST API while writes go to the other, and asserts the group stays off it for 15s although it is alive, then fails back once it has caught up. The tier now reads every cluster's credentials, and shares its certificate pin with the probe. --- .../Environment/ClusterRestClient.cs | 137 +++++++++++++----- .../Environment/FaultInjectorEnvironment.cs | 43 ++++-- .../LagAwareFailbackScenarioTests.cs | 131 +++++++++++++++++ .../README.md | 17 +++ 4 files changed, 277 insertions(+), 51 deletions(-) create mode 100644 tests/StackExchange.Redis.FaultInjector.Tests/LagAwareFailbackScenarioTests.cs diff --git a/tests/StackExchange.Redis.FaultInjector.Tests/Environment/ClusterRestClient.cs b/tests/StackExchange.Redis.FaultInjector.Tests/Environment/ClusterRestClient.cs index ad6f55c7c..e07061166 100644 --- a/tests/StackExchange.Redis.FaultInjector.Tests/Environment/ClusterRestClient.cs +++ b/tests/StackExchange.Redis.FaultInjector.Tests/Environment/ClusterRestClient.cs @@ -3,8 +3,11 @@ using System.IO; using System.Net.Http; using System.Net.Http.Headers; +using System.Net.Http.Json; +using System.Net.Security; using System.Security.Authentication; using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; using System.Text; using System.Text.Json; using System.Threading.Tasks; @@ -15,9 +18,11 @@ namespace StackExchange.Redis.FaultInjector.Tests; /// The cluster's own REST API on port 9443, for the few facts the fault injector does not expose. /// /// -/// Reads only. Anything that *changes* state goes through the injector so it is recorded as a job with an -/// action id - which is also how the console works, and it means a scenario can be reconstructed afterwards -/// from the injector's history rather than from somebody's memory. +/// Reads, plus one deliberate exception: , because the injector has no action for +/// it and it is the narrowest reliable way to make an Active-Active member fall behind. Everything else that +/// *changes* state goes through the injector so it is recorded as a job with an action id - which is also how +/// the console works, and it means a scenario can be reconstructed afterwards from the injector's history rather +/// than from somebody's memory. /// public sealed class ClusterRestClient : IDisposable { @@ -25,48 +30,61 @@ public sealed class ClusterRestClient : IDisposable public ClusterRestClient(FaultInjectorEnvironment.ClusterCredentials credentials, DirectoryInfo configDirectory) { - // The cluster's management (cnm) certificate is minted at bootstrap and is self-signed - its issuer is - // itself, not any CA shipped in the config directory. `ca.crt` there signs the RESP-plane proxy - // certificates (a different, coincidentally-named CA), so chain-building the cnm cert against it fails - // even when it is the right environment. There is nowhere to read the cnm cert's expected fingerprint - // from ahead of time (env_output.json does not carry it), so this pins on first use and persists the - // fingerprint next to the rest of the environment's files - the same trust model as an SSH known_hosts - // entry. A later run against the same environment must present the identical certificate; a changed - // fingerprint fails loudly rather than silently accepting a new identity for a channel that carries - // credentials. Keyed by host: multi-cluster templates put several clusters behind one directory. - var pinPath = Path.Combine(configDirectory.FullName, $"cluster-cert.{credentials.ClusterName}.sha256"); + var validate = PinnedCertificateValidation(credentials, configDirectory); var handler = new HttpClientHandler { - ServerCertificateCustomValidationCallback = (_, certificate, _, _) => - { - if (certificate is null) return false; - var actual = Convert.ToHexString(certificate.GetCertHash(HashAlgorithmName.SHA256)); + ServerCertificateCustomValidationCallback = (request, certificate, chain, errors) => validate(request, certificate, chain, errors), + }; - if (File.Exists(pinPath)) + _http = new HttpClient(handler) { BaseAddress = credentials.RestUrl, Timeout = TimeSpan.FromSeconds(30) }; + var basic = Convert.ToBase64String(Encoding.UTF8.GetBytes($"{credentials.Username}:{credentials.Password}")); + _http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", basic); + } + + /// + /// Validates a cluster's management certificate by pinning it on first use. + /// + /// + /// The cluster's management (cnm) certificate is minted at bootstrap and is self-signed - its issuer is + /// itself, not any CA shipped in the config directory. ca.crt there signs the RESP-plane proxy + /// certificates (a different, coincidentally-named CA), so chain-building the cnm cert against it fails even + /// when it is the right environment. There is nowhere to read the cnm cert's expected fingerprint from ahead + /// of time (env_output.json does not carry it), so this pins on first use and persists the fingerprint next + /// to the rest of the environment's files - the same trust model as an SSH known_hosts entry. A later run + /// against the same environment must present the identical certificate; a changed fingerprint fails loudly + /// rather than silently accepting a new identity for a channel that carries credentials. Keyed by cluster: + /// multi-cluster templates put several clusters behind one directory, and every node of one cluster presents + /// the same certificate. Shared with the lag-aware probe under test, so both trust the same anchor. + /// + public static RemoteCertificateValidationCallback PinnedCertificateValidation( + FaultInjectorEnvironment.ClusterCredentials credentials, DirectoryInfo configDirectory) + { + var pinPath = Path.Combine(configDirectory.FullName, $"cluster-cert.{credentials.ClusterName}.sha256"); + return (_, certificate, _, _) => + { + if (certificate is null) return false; + var actual = Convert.ToHexString(SHA256.HashData(certificate.GetRawCertData())); + + if (File.Exists(pinPath)) + { + var expected = File.ReadAllText(pinPath).Trim(); + if (!string.Equals(expected, actual, StringComparison.OrdinalIgnoreCase)) { - var expected = File.ReadAllText(pinPath).Trim(); - if (!string.Equals(expected, actual, StringComparison.OrdinalIgnoreCase)) - { - throw new AuthenticationException( - $"cluster management certificate at {credentials.RestUrl} does not match the pinned " + - $"fingerprint in {pinPath} (expected {expected}, got {actual}). If this environment " + - "was re-provisioned, delete that file and re-run to pin the new certificate."); - } - - return true; + throw new AuthenticationException( + $"cluster management certificate at {credentials.RestUrl} does not match the pinned " + + $"fingerprint in {pinPath} (expected {expected}, got {actual}). If this environment " + + "was re-provisioned, delete that file and re-run to pin the new certificate."); } - // trust on first use: nothing in the config directory names the expected fingerprint ahead of - // time, so the only available anchor is "whatever this environment presents the first time we - // ask", pinned from here on. - File.WriteAllText(pinPath, actual); return true; - }, - }; + } - _http = new HttpClient(handler) { BaseAddress = credentials.RestUrl, Timeout = TimeSpan.FromSeconds(30) }; - var basic = Convert.ToBase64String(Encoding.UTF8.GetBytes($"{credentials.Username}:{credentials.Password}")); - _http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", basic); + // trust on first use: nothing in the config directory names the expected fingerprint ahead of + // time, so the only available anchor is "whatever this environment presents the first time we + // ask", pinned from here on. + File.WriteAllText(pinPath, actual); + return true; + }; } public void Dispose() => _http.Dispose(); @@ -93,4 +111,49 @@ public ClusterRestClient(FaultInjectorEnvironment.ClusterCredentials credentials return results; } + /// + /// The uid of the named database on this cluster; uids are not shared across the clusters of an + /// Active-Active database, so always look up per cluster. + /// + public async Task FindDatabaseUidAsync(string name) + { + foreach (var (uid, candidate) in await ListDatabasesAsync()) + { + if (string.Equals(candidate, name, StringComparison.Ordinal)) return uid; + } + + throw new InvalidOperationException($"no database named '{name}' on {_http.BaseAddress}"); + } + + /// + /// Pauses or resumes sync into this cluster's member of an Active-Active database + /// (crdt_sync: paused or enabled), waiting until the database reports it and is active. + /// + /// + /// Measured as narrow and reliably undone: one member, no network change, active again within seconds. + /// Callers must resume in a finally. + /// + public async Task SetCrdtSyncAsync(int uid, string value) + { + using (var put = await _http.PutAsJsonAsync($"/v1/bdbs/{uid}", new Dictionary { ["crdt_sync"] = value })) + { + put.EnsureSuccessStatusCode(); + } + + for (int i = 0; i < 60; i++) + { + using var response = await _http.GetAsync($"/v1/bdbs/{uid}?fields=crdt_sync,status"); + response.EnsureSuccessStatusCode(); + using var document = JsonDocument.Parse(await response.Content.ReadAsStringAsync()); + if (document.RootElement.GetProperty("crdt_sync").GetString() == value + && document.RootElement.GetProperty("status").GetString() == "active") + { + return; + } + + await Task.Delay(1000); + } + + throw new TimeoutException($"bdb {uid} on {_http.BaseAddress} did not reach crdt_sync={value}"); + } } diff --git a/tests/StackExchange.Redis.FaultInjector.Tests/Environment/FaultInjectorEnvironment.cs b/tests/StackExchange.Redis.FaultInjector.Tests/Environment/FaultInjectorEnvironment.cs index 6b1b18b5a..e246ed6cf 100644 --- a/tests/StackExchange.Redis.FaultInjector.Tests/Environment/FaultInjectorEnvironment.cs +++ b/tests/StackExchange.Redis.FaultInjector.Tests/Environment/FaultInjectorEnvironment.cs @@ -66,7 +66,13 @@ private FaultInjectorEnvironment(DirectoryInfo configDirectory, Uri injectorUrl) /// advertised endpoint type - which matter because they decide which notification family a database can /// even emit. Tests that provision their own database know what they asked for and need none of this. /// - public ClusterCredentials? Cluster { get; private set; } + public ClusterCredentials? Cluster => Clusters.Count > 0 ? Clusters[0] : null; + + /// + /// Credentials for every cluster in the environment, in env_output.json order (which is the + /// injector's cluster_index); one entry for single-cluster templates, two for Active-Active. + /// + public IReadOnlyList Clusters { get; private set; } = []; private static FaultInjectorEnvironment? Discover() { @@ -94,7 +100,7 @@ private FaultInjectorEnvironment(DirectoryInfo configDirectory, Uri injectorUrl) { CertificateAuthorityPath = FindCertificateAuthority(directory), }; - result.Cluster = ReadClusterCredentials(directory); + result.Clusters = ReadClusterCredentials(directory); return result; } @@ -124,10 +130,10 @@ private FaultInjectorEnvironment(DirectoryInfo configDirectory, Uri injectorUrl) return null; } - private static ClusterCredentials? ReadClusterCredentials(DirectoryInfo directory) + private static IReadOnlyList ReadClusterCredentials(DirectoryInfo directory) { var path = Path.Combine(directory.FullName, "env_output.json"); - if (!File.Exists(path)) return null; + if (!File.Exists(path)) return []; try { @@ -136,26 +142,35 @@ private FaultInjectorEnvironment(DirectoryInfo configDirectory, Uri injectorUrl) // two schemas in the wild: single-cluster templates put outputs at the top level, and the AWS // multi-cluster template nests them under .clusters.value[N]. Both are handled because which one // you get is a property of the template somebody chose, not of anything a test can control. - var root = document.RootElement; - if (root.TryGetProperty("clusters", out var clusters) + var roots = new List(); + if (document.RootElement.TryGetProperty("clusters", out var clusters) && clusters.TryGetProperty("value", out var values) - && values.ValueKind == JsonValueKind.Array - && values.GetArrayLength() > 0) + && values.ValueKind == JsonValueKind.Array) + { + foreach (var value in values.EnumerateArray()) roots.Add(value); + } + else { - root = values[0]; + roots.Add(document.RootElement); } - var name = ReadValue(root, "cluster_name") ?? ReadValue(root, "name"); - var user = ReadValue(root, "username") ?? ReadValue(root, "cluster_username"); - var password = ReadValue(root, "password") ?? ReadValue(root, "cluster_password"); + var result = new List(); + foreach (var root in roots) + { + var name = ReadValue(root, "cluster_name") ?? ReadValue(root, "name"); + var user = ReadValue(root, "username") ?? ReadValue(root, "cluster_username"); + var password = ReadValue(root, "password") ?? ReadValue(root, "cluster_password"); + if (name is null || user is null || password is null) break; // keep indexes aligned with cluster_index + result.Add(new ClusterCredentials(name, user, password)); + } - return name is null || user is null || password is null ? null : new ClusterCredentials(name, user, password); + return result; } catch (Exception) { // a malformed env_output.json is not fatal here: only the tests that need REST enrichment care, // and they report it themselves rather than failing every test in the suite at discovery time - return null; + return []; } } diff --git a/tests/StackExchange.Redis.FaultInjector.Tests/LagAwareFailbackScenarioTests.cs b/tests/StackExchange.Redis.FaultInjector.Tests/LagAwareFailbackScenarioTests.cs new file mode 100644 index 000000000..d14e9daf1 --- /dev/null +++ b/tests/StackExchange.Redis.FaultInjector.Tests/LagAwareFailbackScenarioTests.cs @@ -0,0 +1,131 @@ +using System; +using System.Diagnostics; +using System.Linq; +using System.Net; +using System.Threading.Tasks; +using StackExchange.Redis.Availability; +using Xunit; + +namespace StackExchange.Redis.FaultInjector.Tests; + +/// +/// +/// The lag-aware failback check against a real Redis Enterprise Active-Active deployment: a group must not fail +/// back onto a member that is alive but behind, and must fail back once it has caught up. +/// +/// +/// No network fault is injected. The member is made to fall behind by pausing sync into it +/// (crdt_sync: paused through its cluster's REST API) while writes go to the other member - measured as +/// narrow and reliably undone, unlike network_latency, whose cleanup can fail and leave a cluster's DNS +/// and REST API unreachable (see notes/lag-aware/findings.md, section 10). +/// +/// +[Trait("tier", "fault-injector")] +[Trait("scenario", "lag-aware")] +public class LagAwareFailbackScenarioTests(ExistingDatabaseFixture fixture, ITestOutputHelper output) + : IClassFixture +{ + private const string DatabaseName = "re-active-active"; + private static readonly TimeSpan HoldOff = TimeSpan.FromSeconds(15); + + [Fact] + public async Task RefusesToFailBackOntoAStaleMemberAndReturnsOnceCaughtUp() + { + var database = fixture.Require(DatabaseName); + Assert.SkipWhen(database.MemberCount < 2, $"'{database.Key}' lists {database.MemberCount} endpoint(s); this needs two member databases"); + var environment = fixture.Environment; + Assert.SkipWhen(environment.Clusters.Count < 2, $"env_output.json carries credentials for {environment.Clusters.Count} cluster(s); this needs both"); + + // each member's own cluster: the one whose name its database host ends with ("redis-.") + FaultInjectorEnvironment.ClusterCredentials ClusterOf(DatabaseMember member) + => environment.Clusters.Single(c => member.Host.EndsWith("." + c.ClusterName, StringComparison.OrdinalIgnoreCase)); + var clusters = database.Members.Select(ClusterOf).ToArray(); + output.WriteLine($"member0={database.Members[0]} ({clusters[0]}), member1={database.Members[1]} ({clusters[1]})"); + + ConnectionGroupMember Member(int index, double weight) => new( + database.GetClientConfig(environment, MaintenanceNotificationMode.Disabled, index), $"member{index}") + { + Weight = weight, + FailbackHealthCheck = HealthCheck.LagAware(new LagAwareOptions + { + Credentials = _ => new(new NetworkCredential(clusters[index].Username, clusters[index].Password)), + CertificateValidation = ClusterRestClient.PinnedCertificateValidation(clusters[index], environment.ConfigDirectory), + }), + }; + + // member1 is preferred, so "not returning to it" is the observable refusal + var member0 = Member(0, weight: 1); + var member1 = Member(1, weight: 9); + MultiGroupOptions options = new MultiGroupOptions.Builder + { + HealthCheckInterval = TimeSpan.FromSeconds(1), + FailbackDelay = TimeSpan.Zero, // only the lag check stands between member1 and a failback + }; + + using var rest = new ClusterRestClient(clusters[1], environment.ConfigDirectory); + var uid = await rest.FindDatabaseUidAsync(DatabaseName); + + using var writer = new TestOutputWriter(output); + await using var group = await ConnectionMultiplexer.ConnectGroupAsync([member0, member1], options, writer); + await WaitForActiveAsync(group, member1, TimeSpan.FromSeconds(30), "the preferred, caught-up member should be selected first"); + + var db = group.GetDatabase(); + RedisKey key = "fi-lag-aware-counter"; + await db.KeyDeleteAsync(key); + + bool paused = false; + try + { + await rest.SetCrdtSyncAsync(uid, "paused"); + paused = true; + output.WriteLine($"{Now()} sync into member1 (bdb {uid} on {clusters[1]}) paused"); + + // leave member1 for member0, then release the override: member1 is the higher weight, alive and + // connected, so the only thing that can keep the group off it is its failback check + Assert.True(group.TryFailoverTo(member0)); + Assert.True(group.TryFailoverTo(null)); + + var watch = Stopwatch.StartNew(); + long written = 0; + while (watch.Elapsed < HoldOff) + { + written = await db.StringIncrementAsync(key); // on member0; member1 cannot see these + Assert.True(member1.IsConnected, "member1 should stay connected: the refusal must be lag, not liveness"); + Assert.True( + group.ActiveMember == member0, + $"{Now()} the group moved to {group.ActiveMember} while member1 was behind (failback verified: {member1.FailbackVerified})"); + await Task.Delay(500, TestContext.Current.CancellationToken); + } + + output.WriteLine($"{Now()} held on member0 for {HoldOff.TotalSeconds:0}s, {written} writes member1 has not seen"); + } + finally + { + if (paused) + { + await rest.SetCrdtSyncAsync(uid, "enabled"); + output.WriteLine($"{Now()} sync into member1 resumed"); + } + } + + await WaitForActiveAsync(group, member1, TimeSpan.FromSeconds(60), "the group should fail back once member1 has caught up"); + var seen = (long)await member1.Multiplexer.GetDatabase().StringGetAsync(key); + output.WriteLine($"{Now()} failed back to member1, which now has the counter at {seen}"); + Assert.True(seen > 0, "member1 should have received member0's writes by the time it was selected"); + await db.KeyDeleteAsync(key); + } + + private async Task WaitForActiveAsync(IConnectionGroup group, ConnectionGroupMember expected, TimeSpan timeout, string because) + { + var watch = Stopwatch.StartNew(); + while (group.ActiveMember != expected && watch.Elapsed < timeout) + { + await Task.Delay(250, TestContext.Current.CancellationToken); + } + + output.WriteLine($"{Now()} active: {group.ActiveMember} after {watch.Elapsed.TotalSeconds:0.0}s (wanted {expected})"); + Assert.True(group.ActiveMember == expected, $"{because}; active is {group.ActiveMember} after {timeout.TotalSeconds:0}s"); + } + + private static string Now() => DateTime.UtcNow.ToString("HH:mm:ss.f"); +} diff --git a/tests/StackExchange.Redis.FaultInjector.Tests/README.md b/tests/StackExchange.Redis.FaultInjector.Tests/README.md index 0be35486c..9e9d2d4ee 100644 --- a/tests/StackExchange.Redis.FaultInjector.Tests/README.md +++ b/tests/StackExchange.Redis.FaultInjector.Tests/README.md @@ -125,6 +125,23 @@ is blackholed is the injector's choice - it evidently picks the first, which is every member's `host:port` and says so in the final assertion message, because a wrong-cluster injection otherwise looks exactly like a failback that never happened. +## Lag-aware failback + +`LagAwareFailbackScenarioTests` checks the lag-aware failback check (`HealthCheck.LagAware`, `SER011`) against the +same `re-active-active` pair: the group must not fail back onto a member that is alive but behind, and must +return once it has caught up. It needs credentials for **both** clusters in `env_output.json` (the AWS +multi-cluster template has them) and skips otherwise. + +No fault is injected. The preferred member is made to fall behind by pausing sync *into* it - `crdt_sync: paused` +through its cluster's REST API, via `ClusterRestClient.SetCrdtSyncAsync` - while writes go to the other member, +and sync is resumed in a `finally`. That is the one state change this tier makes outside the injector, because +the injector has no action for it, and because the obvious alternative is worse: `network_latency` shapes the +node's whole interface and its cleanup can fail, which on one environment left the cluster's DNS and REST API +unreachable until fixed by hand. + +Measured shape of a passing run: on the preferred member first; held off it for the 15 s it was behind, while +alive and connected; failed back about 4 s after sync resumed, by which time it had all of the writes. + ## Three states, deliberately distinct | state | behaviour | From 8e135d1958838e0c9d5551afcb5bcd97810f35e7 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 13:46:34 +0100 Subject: [PATCH 24/34] Notes: tier-3 scenario test passing --- notes/lag-aware/api-sketch.md | 4 ++-- notes/lag-aware/plan.md | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index b00c7be91..11f34ebfe 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -2,8 +2,8 @@ Status: **sections 1 to 3 implemented** (2026-10-08): the failback check, member-scoped probes, and the lag-aware probe (`HealthCheck.LagAware`, `HealthCheckProbe.LagAware`, `LagAwareOptions`, `LagCheckMode`), -with stubbed-transport tests replaying the measured response shapes. Not yet: client-certificate support for -the REST call, and the fault-injector scenario test. +with stubbed-transport tests replaying the measured response shapes. The fault-injector scenario test passes +against a live Active-Active pair. Not yet: client-certificate support for the REST call. Evidence is in [`findings.md`](findings.md) (section 10 especially); sequence in [`plan.md`](plan.md). *Correction:* `SER007` was retired when geo-redundant failover went GA (#3247), so the shipped `Availability/` API is not experimental; everything new here is behind a new diagnostic, **`SER011`** diff --git a/notes/lag-aware/plan.md b/notes/lag-aware/plan.md index 130135269..f44d2837f 100644 --- a/notes/lag-aware/plan.md +++ b/notes/lag-aware/plan.md @@ -7,7 +7,9 @@ Status: **proposed. Step 0 is cleared; step 1 is answered (findings §10): the l real Active-Active lag, on the stale member only, and the tolerance is honoured on a running link. Cross-region lag magnitudes remain unmeasured (both test clusters share a region).** The core change (failback health check, member-scoped probes, selection rules) and the REST probe are implemented behind -`SER011`, with stubbed-transport tests (step 5, tier 1); next is the fault-injector scenario test (tier 3). +`SER011`, with stubbed-transport tests (step 5, tier 1) and a fault-injector scenario test (tier 3, +`LagAwareFailbackScenarioTests`, passing against a live Active-Active pair on 2026-10-08). Tier 2 (the +Kestrel toy) is not done and arguably not needed now that tiers 1 and 3 exist. See [`api-sketch.md`](api-sketch.md). > **This branch is not only a planning branch.** `marc/lag-aware-availability` carries these notes From 3fe22085d09c84dc1ec810e6a61f734f08b62ef3 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 8 Oct 2026 16:38:01 +0100 Subject: [PATCH 25/34] LagAware: client certificates for the REST call LagAwareOptions.ClientCertificate, with SetUserPfxCertificate and (net6.0+) SetUserPemCertificate mirroring ConfigurationOptions and sharing its loaders. Wired into SocketsHttpHandler on .NET and HttpClientHandler down-level; not supported on net461. Every other client offers mTLS for this call. --- .../Availability/HealthCheckProbe.LagAware.cs | 15 +++- .../Availability/LagAwareOptions.cs | 26 +++++++ .../ConfigurationOptions.cs | 19 +++-- .../PublicAPI/PublicAPI.Unshipped.txt | 3 + .../PublicAPI/net6.0/PublicAPI.Unshipped.txt | 1 + .../LagAwareProbeTests.cs | 74 +++++++++++++++++++ 6 files changed, 128 insertions(+), 10 deletions(-) diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs b/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs index 29bdc2d0c..c6699bfea 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs @@ -6,6 +6,7 @@ using System.Net.Http; using System.Net.Http.Headers; using System.Net.Security; +using System.Security.Cryptography.X509Certificates; using System.Text; using System.Text.Json; using System.Threading; @@ -63,23 +64,24 @@ public LagAwareProbe(LagAwareOptions options) _databaseId = options.DatabaseId; _mode = options.LagCheck; _tolerance = options.LagTolerance; - var handler = options.HttpMessageHandlerFactory?.Invoke() ?? CreateHandler(options.CertificateValidation); + var handler = options.HttpMessageHandlerFactory?.Invoke() ?? CreateHandler(options.CertificateValidation, options.ClientCertificate); _http = new HttpClient(handler) { Timeout = Timeout.InfiniteTimeSpan }; // bounded per call instead } public override HealthCheckProbeScope Scope => HealthCheckProbeScope.Member; - private static HttpMessageHandler CreateHandler(RemoteCertificateValidationCallback? validation) + internal static HttpMessageHandler CreateHandler(RemoteCertificateValidationCallback? validation, X509Certificate2? clientCertificate) { #if NET // redirects point at a node's internal address, so following one only ever hangs; report it instead var handler = new SocketsHttpHandler { PooledConnectionLifetime = TimeSpan.FromMinutes(2), AllowAutoRedirect = false }; if (validation is not null) handler.SslOptions.RemoteCertificateValidationCallback = validation; + if (clientCertificate is not null) handler.SslOptions.ClientCertificates = [clientCertificate]; return handler; #elif NET461 - if (validation is not null) + if (validation is not null || clientCertificate is not null) { - throw new PlatformNotSupportedException("Custom certificate validation for the REST API requires .NET Framework 4.7.1 or later."); + throw new PlatformNotSupportedException("Custom certificate validation and client certificates for the REST API require .NET Framework 4.7.1 or later."); } return new HttpClientHandler { AllowAutoRedirect = false }; #else @@ -88,6 +90,11 @@ private static HttpMessageHandler CreateHandler(RemoteCertificateValidationCallb { handler.ServerCertificateCustomValidationCallback = (request, certificate, chain, errors) => validation(request, certificate, chain, errors); } + if (clientCertificate is not null) + { + handler.ClientCertificateOptions = ClientCertificateOption.Manual; + handler.ClientCertificates.Add(clientCertificate); + } return handler; #endif } diff --git a/src/StackExchange.Redis/Availability/LagAwareOptions.cs b/src/StackExchange.Redis/Availability/LagAwareOptions.cs index 83f884511..5b7280e06 100644 --- a/src/StackExchange.Redis/Availability/LagAwareOptions.cs +++ b/src/StackExchange.Redis/Availability/LagAwareOptions.cs @@ -86,6 +86,32 @@ public void TrustIssuer(string issuerCertificatePath) public void TrustIssuer(X509Certificate2 issuer) => CertificateValidation = ConfigurationOptions.TrustIssuerCallback(issuer); + /// + /// A client certificate to present to the REST API, for clusters that require mutual TLS on it; when + /// (the default), none. Not supported on .NET Framework 4.6.1. + /// + public X509Certificate2? ClientCertificate { get; set; } + +#if NET + /// + /// Present a client certificate loaded from a PEM file pair, as + /// does for the data plane. + /// + /// The path for the user certificate (commonly a .crt file). + /// The path for the user key (commonly a .key file). + public void SetUserPemCertificate(string userCertificatePath, string? userKeyPath = null) + => ClientCertificate = ConfigurationOptions.LoadPemUserCertificate(userCertificatePath, userKeyPath); +#endif + + /// + /// Present a client certificate loaded from a PFX file, as + /// does for the data plane. + /// + /// The path for the user certificate (commonly a .pfx file). + /// The password for the certificate file. + public void SetUserPfxCertificate(string userCertificatePath, string? password = null) + => ClientCertificate = ConfigurationOptions.LoadPfxUserCertificate(userCertificatePath, password); + // test seam: replaces the HTTP transport entirely, so tests can answer requests without a listener internal Func? HttpMessageHandlerFactory { get; set; } } diff --git a/src/StackExchange.Redis/ConfigurationOptions.cs b/src/StackExchange.Redis/ConfigurationOptions.cs index b4b2502b1..90e12c560 100644 --- a/src/StackExchange.Redis/ConfigurationOptions.cs +++ b/src/StackExchange.Redis/ConfigurationOptions.cs @@ -561,25 +561,32 @@ public void SetUserPfxCertificate(string userCertificatePath, string? password = #if NET internal static LocalCertificateSelectionCallback CreatePemUserCertificateCallback(string userCertificatePath, string? userKeyPath) + { + var pfx = LoadPemUserCertificate(userCertificatePath, userKeyPath); + return (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => pfx; + } + + internal static X509Certificate2 LoadPemUserCertificate(string userCertificatePath, string? userKeyPath) { // PEM handshakes not universally supported and causes a runtime error about ephemeral certificates; to avoid, export as PFX using var pem = X509Certificate2.CreateFromPemFile(userCertificatePath, userKeyPath); #pragma warning disable SYSLIB0057 // X509 loading - var pfx = new X509Certificate2(pem.Export(X509ContentType.Pfx)); + return new X509Certificate2(pem.Export(X509ContentType.Pfx)); #pragma warning restore SYSLIB0057 // X509 loading - - return (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => pfx; } #endif internal static LocalCertificateSelectionCallback CreatePfxUserCertificateCallback(string userCertificatePath, string? password, X509KeyStorageFlags storageFlags = X509KeyStorageFlags.DefaultKeySet) { -#pragma warning disable SYSLIB0057 // X509 loading - var pfx = new X509Certificate2(userCertificatePath, password ?? "", storageFlags); -#pragma warning restore SYSLIB0057 // X509 loading + var pfx = LoadPfxUserCertificate(userCertificatePath, password, storageFlags); return (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => pfx; } + internal static X509Certificate2 LoadPfxUserCertificate(string userCertificatePath, string? password, X509KeyStorageFlags storageFlags = X509KeyStorageFlags.DefaultKeySet) +#pragma warning disable SYSLIB0057 // X509 loading + => new X509Certificate2(userCertificatePath, password ?? "", storageFlags); +#pragma warning restore SYSLIB0057 // X509 loading + /// /// Create a certificate validation check that checks against the supplied issuer even when not known by the machine. /// diff --git a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt index edb08ced2..4fb5b51af 100644 --- a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt +++ b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt @@ -30,3 +30,6 @@ [SER011]StackExchange.Redis.Availability.LagCheckMode.Enabled = 1 -> StackExchange.Redis.Availability.LagCheckMode [SER011]static StackExchange.Redis.Availability.HealthCheck.LagAware(StackExchange.Redis.Availability.LagAwareOptions! options) -> StackExchange.Redis.Availability.HealthCheck! [SER011]static StackExchange.Redis.Availability.HealthCheckProbe.LagAware(StackExchange.Redis.Availability.LagAwareOptions! options) -> StackExchange.Redis.Availability.HealthCheckProbe! +[SER011]StackExchange.Redis.Availability.LagAwareOptions.ClientCertificate.get -> System.Security.Cryptography.X509Certificates.X509Certificate2? +[SER011]StackExchange.Redis.Availability.LagAwareOptions.ClientCertificate.set -> void +[SER011]StackExchange.Redis.Availability.LagAwareOptions.SetUserPfxCertificate(string! userCertificatePath, string? password = null) -> void diff --git a/src/StackExchange.Redis/PublicAPI/net6.0/PublicAPI.Unshipped.txt b/src/StackExchange.Redis/PublicAPI/net6.0/PublicAPI.Unshipped.txt index ab058de62..b10d80f07 100644 --- a/src/StackExchange.Redis/PublicAPI/net6.0/PublicAPI.Unshipped.txt +++ b/src/StackExchange.Redis/PublicAPI/net6.0/PublicAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +[SER011]StackExchange.Redis.Availability.LagAwareOptions.SetUserPemCertificate(string! userCertificatePath, string? userKeyPath = null) -> void diff --git a/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs b/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs index 39225c70b..af95e3135 100644 --- a/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs +++ b/tests/StackExchange.Redis.Tests/LagAwareProbeTests.cs @@ -257,6 +257,80 @@ public async Task ARedirectIsReportedNotFollowed() public void FindsTheDatabaseByAddressAsWellAsByName() => Assert.Equal(2, HealthCheckProbe.LagAwareProbe.FindUid(BdbList, "100.53.190.50", 12706)); + // public-only and self-signed (CN=lag-aware-probe-test, private key discarded at generation): enough to show + // the certificate reaching the transport, where generating one is not possible (Mono has no CertificateRequest) + private const string PublicOnlyCertificate = "MIIDITCCAgmgAwIBAgIUbLJxUJKq4SkB9FnIFFLUTKBAaSkwDQYJKoZIhvcNAQELBQAwHzEdMBsGA1UEAwwUbGFnLWF3YXJlLXByb2JlLXRlc3QwIBcNMjYxMDA4MTUzNjU0WhgPMjEyNjA5MTQxNTM2NTRaMB8xHTAbBgNVBAMMFGxhZy1hd2FyZS1wcm9iZS10ZXN0MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxXfFEzpB82vcUAwfk75Blw1BYG57k9XXIBqa0dx7KxivuF0iSZsSPGViBs6Pwjx7Yvq6+FRZHusFEZ0s3qRbbSf5p7dZhAJ4Oeb5TjqYj7rNQaoCjz2+W4eWbSpVpd8If+g3qy/U+C3gTdIPOUu9biExNj9iN/dUt4o/0S3vgp0iwUiP/E2TcnFRk0SHLgo0Irt1U12FWSpdJA+p65OoG4h/xbsO70qqn/ifyLX8B+eYBOaqQi8myls3MPef2b/pmQlMYUaTGR5kT2EY3pRusTEhdpC1xDrmYxUnxzptgkcAval7HjXEU0sB7c7atGgTaQlUSv8CsrOk/wtQ4Lp54QIDAQABo1MwUTAdBgNVHQ4EFgQUbo8ar3s44TIDl8yYq4Hc8TsTs9UwHwYDVR0jBBgwFoAUbo8ar3s44TIDl8yYq4Hc8TsTs9UwDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAVMrCof8H0vBm2UBSxSQreCblVlz41Yinkp1Q2B+1s1CkvTXGhTPS3iJv2FYViJ5RsC36nG5e35vdW3L4r0hjn1lZA32lX2XAYUsrjx7YaQBqvm6qYiK9HqFs2uJUNN7EtDZsWPMN+SRoRAkcQsTZlMvf2qKIkcnulU36wQ+vEdEmLRRsUmIbdPqhT09kJGx0X7jVUmD6PnuAyk8/OJPAGQM+uCvS43Gfdm+ywQ9+rQIDs7F3lU9g7D0o8OS3yY7WY87vMbiwh7eUlDmn74ypCXSTpLTEy4OtP4Th+eXrZGV5+ExmVwzbeWucC5nCMQJ4+ZIpkjW6h0tb/4WTnfSE3w=="; + +#if NET + private static System.Security.Cryptography.X509Certificates.X509Certificate2 CreateClientCertificate() + { + using var rsa = System.Security.Cryptography.RSA.Create(2048); + var request = new System.Security.Cryptography.X509Certificates.CertificateRequest( + "CN=lag-aware-probe-test", rsa, System.Security.Cryptography.HashAlgorithmName.SHA256, System.Security.Cryptography.RSASignaturePadding.Pkcs1); + return request.CreateSelfSigned(DateTimeOffset.UtcNow.AddMinutes(-5), DateTimeOffset.UtcNow.AddHours(1)); + } + + [Fact] + public void LoadsAClientCertificateFromPfx() + { + using var certificate = CreateClientCertificate(); + var path = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"lagaware-{Guid.NewGuid():n}.pfx"); + try + { + System.IO.File.WriteAllBytes(path, certificate.Export(System.Security.Cryptography.X509Certificates.X509ContentType.Pfx, "pw")); + var options = new LagAwareOptions(); + options.SetUserPfxCertificate(path, "pw"); + Assert.Equal(certificate.Thumbprint, options.ClientCertificate?.Thumbprint); + Assert.True(options.ClientCertificate!.HasPrivateKey); + } + finally + { + System.IO.File.Delete(path); + } + } + + [Fact] + public void LoadsAClientCertificateFromPem() + { + using var certificate = CreateClientCertificate(); + var dir = System.IO.Directory.CreateTempSubdirectory("lagaware-").FullName; + try + { + var crt = System.IO.Path.Combine(dir, "client.crt"); + var key = System.IO.Path.Combine(dir, "client.key"); + System.IO.File.WriteAllText(crt, certificate.ExportCertificatePem()); + System.IO.File.WriteAllText(key, System.Security.Cryptography.X509Certificates.RSACertificateExtensions.GetRSAPrivateKey(certificate)!.ExportPkcs8PrivateKeyPem()); + var options = new LagAwareOptions(); + options.SetUserPemCertificate(crt, key); + Assert.Equal(certificate.Thumbprint, options.ClientCertificate?.Thumbprint); + Assert.True(options.ClientCertificate!.HasPrivateKey); + } + finally + { + System.IO.Directory.Delete(dir, recursive: true); + } + } +#endif + + [Fact] + public void TheClientCertificateReachesTheTransport() + { +#pragma warning disable SYSLIB0057 // X509 loading; X509CertificateLoader is not available on every test TFM + using var certificate = new System.Security.Cryptography.X509Certificates.X509Certificate2(Convert.FromBase64String(PublicOnlyCertificate)); +#pragma warning restore SYSLIB0057 + using var handler = HealthCheckProbe.LagAwareProbe.CreateHandler(validation: null, certificate); +#if NET + var sockets = Assert.IsType(handler); + Assert.Contains(certificate, sockets.SslOptions.ClientCertificates!.Cast()); + Assert.False(sockets.AllowAutoRedirect); +#else + var client = Assert.IsType(handler); + Assert.Equal(ClientCertificateOption.Manual, client.ClientCertificateOptions); + Assert.Contains(certificate, client.ClientCertificates.Cast()); + Assert.False(client.AllowAutoRedirect); +#endif + } + [Fact] public void RejectsInvalidOptions() { From 9b3b1a5cb9379072a215048966ce306b615bd01a Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 9 Oct 2026 10:30:05 +0100 Subject: [PATCH 26/34] Failback: a member chosen by the liveness fallback is provisional Other clients never select a member that fails its lag check; the nearest equivalent that still serves when every member is behind is to leave such a member for an eligible one as soon as one is available, whatever the weights, and to keep checking it so it becomes an ordinary active member once caught up. --- docs/Failover.md | 4 ++ .../Availability/MultiGroupMultiplexer.cs | 35 +++++++++++---- .../FailbackHealthCheckTests.cs | 44 ++++++++++++++++++- 3 files changed, 73 insertions(+), 10 deletions(-) diff --git a/docs/Failover.md b/docs/Failover.md index 6fd664707..46cd3abaa 100644 --- a/docs/Failover.md +++ b/docs/Failover.md @@ -787,6 +787,10 @@ The rules: regions every member can be behind at once, and a group that refused to serve from any of them would turn a consistency precaution into an outage. When the active member is lost, the best eligible survivor is preferred, then the best live one. +- **A member chosen that way is provisional.** It is left for an eligible member as soon as one is + available, whatever their weights, and becomes an ordinary active member once it passes its own failback + check. Other clients never select a member that fails its lag check at all; this is the nearest + equivalent that still serves when every member is behind. - **An explicit `TryFailoverTo(member)` overrides it**, as it does `IsUnhealthy`. A probe that asks about the database as a whole, rather than one endpoint, can set diff --git a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs index a1c893138..2a2d8e76b 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs +++ b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs @@ -703,7 +703,10 @@ internal async Task RunHealthCheckAsync() // timeout, leaves the member ineligible; none of it marks a member unhealthy. private async Task RunFailbackChecksAsync(ConnectionGroupMember[] members) { + // a provisional active member (chosen by the liveness fallback) is still checked, so that it can + // stop being provisional once it has caught up; a normal active member is not var active = _activeStub.Active; + var provisional = Volatile.Read(ref _provisional); List<(ConnectionGroupMember Member, Task Pending)>? checks = null; int totalTimeoutMillis = 0; foreach (var member in members) @@ -711,7 +714,7 @@ private async Task RunFailbackChecksAsync(ConnectionGroupMember[] members) var check = member.ResolveFailbackHealthCheck(_options); if (check is null) continue; - if (!member.ConsiderActive || ReferenceEquals(member.Multiplexer, active)) + if (!member.ConsiderActive || (ReferenceEquals(member.Multiplexer, active) && !ReferenceEquals(active, provisional))) { member.FailbackVerified = false; continue; @@ -741,14 +744,21 @@ private async Task RunFailbackChecksAsync(ConnectionGroupMember[] members) } } - // eligible to become (or stay) active: the active member always is, as is an explicit failover - // target; otherwise a member with a failback check must have passed it on the latest pass + // Eligible to become (or stay) active: an explicit failover target always is, as is the active member + // - unless it is provisional, having been chosen by the liveness fallback without ever passing its + // failback check; otherwise a member with a failback check must have passed it on the latest pass. private bool IsFailbackEligible(ConnectionGroupMember member, ConnectionMultiplexer? active) => member.ExplicitOverride - || ReferenceEquals(member.Multiplexer, active) + || (ReferenceEquals(member.Multiplexer, active) && !ReferenceEquals(active, Volatile.Read(ref _provisional))) || member.ResolveFailbackHealthCheck(_options) is null || member.FailbackVerified; + // The active member when it was chosen by the liveness fallback (no live member was eligible), else null. + // Other clients never select a member that fails its lag check at all; this is the nearest equivalent + // that still serves when every member is behind: such a member is left for an eligible one as soon as + // one is available, and becomes an ordinary active member once it passes its own failback check. + private ConnectionMultiplexer? _provisional; + private long GetFailbackFailureCutoff(ConnectionGroupMember member) { // the minimum last-observed unhealthy time (as UTC ticks) that we'll allow for reconnect; @@ -787,10 +797,19 @@ internal void SelectPreferredGroup() } } - // The active member is always eligible, so this only applies when it is gone (or there is none): - // if no live member has passed its failback check - every one lagging, say, which is the normal - // state of a partition between regions - serve from the best live member rather than from none. - preferredMember ??= fallbackMember; + // The active member is eligible unless provisional, so this only applies when it is gone, there is + // none, or it is itself provisional: if no live member has passed its failback check - every one + // lagging, say, which is the normal state of a partition between regions - serve from the best live + // member rather than from none, marking it provisional. + if (preferredMember is null) + { + preferredMember = fallbackMember; + Volatile.Write(ref _provisional, fallbackMember?.Multiplexer); + } + else + { + Volatile.Write(ref _provisional, null); + } SetActive(preferredMember?.Multiplexer); diff --git a/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs b/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs index 1bb1d8fac..913b07fe3 100644 --- a/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs +++ b/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs @@ -71,8 +71,8 @@ public async Task ConnectAsync() HealthCheckInterval = TimeSpan.MaxValue, // passes are driven by the test }; // Hold every member down until all of their connections are up, so the first real selection - // sees the whole group: otherwise whichever member connects first is selected by the liveness - // fallback and, by design, is never evicted by a failback check - a race, not the rule under test. + // sees the whole group: otherwise whichever member connects first can be selected by the liveness + // fallback (provisionally) before the others are up - a race, not the rule under test. foreach (var endpoint in _endpoints) Liveness.Set(endpoint, HealthCheckResult.Unhealthy); Group = await ConnectionMultiplexer.ConnectGroupAsync(Members, options); @@ -151,6 +151,46 @@ public async Task NoEligibleMemberFallsBackToLiveness() Assert.Equal("beta", f.Active); } + [Fact] + public async Task AFallbackChoiceIsLeftForAnEligibleMember() + { + // other clients never select a member that fails its lag check; the nearest equivalent that still + // serves when every member is behind is to leave a fallback choice as soon as anything is eligible + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + f.Failback.Set(Alpha, HealthCheckResult.Unhealthy); + f.Failback.Set(Beta, HealthCheckResult.Unhealthy); + await f.ConnectAsync(); + Assert.Equal("beta", f.Active); // provisional + + f.Failback.Set(Alpha, HealthCheckResult.Healthy); + await f.PassAsync(); + Assert.Equal("alpha", f.Active); // eligible beats provisional, whatever the weights + + f.Failback.Set(Beta, HealthCheckResult.Healthy); + await f.PassAsync(); + Assert.Equal("beta", f.Active); // and ordinary failback by weight resumes + } + + [Fact] + public async Task AProvisionalMemberThatCatchesUpBecomesOrdinary() + { + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + f.Failback.Set(Alpha, HealthCheckResult.Unhealthy); + f.Failback.Set(Beta, HealthCheckResult.Unhealthy); + await f.ConnectAsync(); + Assert.Equal("beta", f.Active); // provisional + + f.Failback.Set(Beta, HealthCheckResult.Healthy); // still checked while provisional + await f.PassAsync(); + Assert.Equal("beta", f.Active); // verified: no longer provisional + + // now an ordinary active member: never evicted by its own failback check + f.Failback.Set(Alpha, HealthCheckResult.Healthy); + f.Failback.Set(Beta, HealthCheckResult.Unhealthy); + await f.PassAsync(); + Assert.Equal("beta", f.Active); + } + [Fact] public async Task DeadActiveIsReplacedEvenWhenNoSurvivorIsEligible() { From fa5b8abfd00fa718a33d370b84255c1cec44ae24 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 9 Oct 2026 10:30:05 +0100 Subject: [PATCH 27/34] Notes: fallback choice is provisional, as decided by client parity --- notes/lag-aware/api-sketch.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index 11f34ebfe..fd7938c27 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -82,13 +82,14 @@ stale reading evicts the active member. Measured: only the member *behind* flips one being written to, so for a single-writer deployment that eviction does not happen, and for a multi-writer one it would be wrong. This is a deliberate deviation. -**As built**, with one consequence worth reviewing: because the active member is never evicted by its -failback check, a member chosen by the liveness fallback (no eligible member was up at the time) stays -active while it is alive, even after an eligible member appears. That matches go-redis's "never evict" -and is what weight alone would do if the fallback member has the higher weight; the alternative (treat -a fallback-selected active member as provisional, and move to an eligible one when it appears) is a -small change if preferred. Tested in `FailbackHealthCheckTests`; the fixture holds all members down until -every connection is up, precisely because a start-up race otherwise exercises this case. +**As built.** One refinement, decided 2026-10-08 by "do what the other clients do": a member chosen by +the liveness fallback is **provisional**. It is left for an eligible member as soon as one is available +(whatever the weights), and its own failback check keeps running so that it becomes an ordinary active +member once it passes. Rationale: Lettuce, Jedis and redis-py treat a lagging database as unhealthy, and +go-redis never selects a lagging candidate, so none of them can end up on an unverified member while a +verified one exists; this is the nearest equivalent that still serves when every member is behind. +Tested in `FailbackHealthCheckTests`; the fixture holds all members down until every connection is up, so +that a start-up race does not pick a provisional member before the test means it to. Implementing it also surfaced an existing bug: `HealthCheckProbePolicy.AllSuccess` (the default) reported `Healthy` for a run of nothing but `Inconclusive` probes, contrary to its own comment. Harmless for From 4fce481100821a0190ae60da6ab8aa13aecd88b6 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 9 Oct 2026 12:45:17 +0100 Subject: [PATCH 28/34] Move the lag-aware log message to a free event id main now uses 124 and 125 (#3257); the guard test from #3266 rejects duplicates. --- src/StackExchange.Redis/LoggerExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/StackExchange.Redis/LoggerExtensions.cs b/src/StackExchange.Redis/LoggerExtensions.cs index 2f4bff50c..f8612e8dd 100644 --- a/src/StackExchange.Redis/LoggerExtensions.cs +++ b/src/StackExchange.Redis/LoggerExtensions.cs @@ -842,7 +842,7 @@ internal static void LogWithThreadPoolStats(this ILogger? log, string message) [LoggerMessage( Level = LogLevel.Warning, - EventId = 124, + EventId = 128, Message = "Lag-aware health check could not ask {RestEndpoint}: {Reason}")] internal static partial void LogWarningLagAwareInconclusive(this ILogger logger, string restEndpoint, string reason); } From 161539e4b053c3b87f7a7537660f778f45fa5524 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 9 Oct 2026 12:47:03 +0100 Subject: [PATCH 29/34] AnySuccess: a run of only inconclusive probes is not a failure AnySuccess reported Unhealthy whenever nothing succeeded, even when nothing failed either - so a probe that cannot tell (a key-write probe on a replica, say) marked its member unhealthy. Now Inconclusive, matching AllSuccess and MajoritySuccess. --- docs/Failover.md | 2 +- .../Availability/HealthCheckProbePolicy.cs | 8 +++--- .../HealthCheckPolicyUnitTests.cs | 1 + .../FailbackHealthCheckTests.cs | 27 +++++++++++++++++++ 4 files changed, 34 insertions(+), 4 deletions(-) diff --git a/docs/Failover.md b/docs/Failover.md index 46cd3abaa..76974f202 100644 --- a/docs/Failover.md +++ b/docs/Failover.md @@ -431,7 +431,7 @@ The probe policy determines how multiple probe results are evaluated to determin #### HealthCheckProbePolicy.AnySuccess -The health check passes if **any** probe succeeds. This provides the most lenient evaluation: +The health check passes if **any** probe succeeds, and fails only if at least one probe failed and none succeeded. This provides the most lenient evaluation. A run in which every probe was `Inconclusive` reports `Inconclusive`, not `Unhealthy`: ```csharp HealthCheck healthCheck = new HealthCheck.Builder diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs b/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs index 6b929f823..ecbc686c5 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Diagnostics.CodeAnalysis; using RESPite; @@ -72,8 +72,10 @@ public override HealthCheckResult Evaluate(in HealthCheckProbeContext context) // Succeed as soon as we have any success if (context.Success > 0) return HealthCheckResult.Healthy; - // Fail only when all probes have failed (no remaining) - if (context.Remaining == 0) return HealthCheckResult.Unhealthy; + // With nothing remaining and no success, fail - but only if something actually failed: a run of + // nothing but inconclusive probes ("could not tell") is not a failure, and reporting it as one marks + // a member unhealthy that may be perfectly well (a key-write probe on a replica, for example) + if (context.Remaining == 0) return context.Failure > 0 ? HealthCheckResult.Unhealthy : HealthCheckResult.Inconclusive; // Can't determine yet return HealthCheckResult.Inconclusive; diff --git a/tests/StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs b/tests/StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs index 23dc13373..22894e1a7 100644 --- a/tests/StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/HealthCheckPolicyUnitTests.cs @@ -44,6 +44,7 @@ public void ProbeTrafficIsNotCallerFacingButIsRoutedNormally() [InlineData(0, 5, 0, HealthCheckResult.Unhealthy)] // All failures [InlineData(1, 0, 2, HealthCheckResult.Healthy)] // Early success [InlineData(0, 1, 2, HealthCheckResult.Inconclusive)] // Early failure but more probes remain + [InlineData(0, 0, 0, HealthCheckResult.Inconclusive)] // Every probe inconclusive: not a failure public void AnySuccess_EvaluatesCorrectly(int success, int failure, int remaining, HealthCheckResult expected) { var policy = HealthCheckProbePolicy.AnySuccess; diff --git a/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs b/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs index 913b07fe3..0ee9fe654 100644 --- a/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs +++ b/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs @@ -277,6 +277,33 @@ public async Task MemberScopedProbeRunsOncePerMember() Assert.All(perMember.Calls.Values, n => Assert.Equal(1, n)); } + [Theory] + [InlineData(nameof(HealthCheckProbePolicy.AllSuccess))] + [InlineData(nameof(HealthCheckProbePolicy.AnySuccess))] + [InlineData(nameof(HealthCheckProbePolicy.MajoritySuccess))] + public async Task ARunOfInconclusiveProbesIsInconclusiveUnderEveryPolicy(string policy) + { + // "could not tell" is neither a pass nor a failure, whichever policy collates it + using var server = new InProcessTestServer(log, endpoint: Alpha); + await using var conn = await server.ConnectAsync(); + var probe = new ScriptedProbe(); + probe.Set(Alpha, HealthCheckResult.Inconclusive); + HealthCheck check = new HealthCheck.Builder + { + Probe = probe, + ProbeCount = 3, + ProbeInterval = TimeSpan.Zero, + ProbePolicy = policy switch + { + nameof(HealthCheckProbePolicy.AllSuccess) => HealthCheckProbePolicy.AllSuccess, + nameof(HealthCheckProbePolicy.AnySuccess) => HealthCheckProbePolicy.AnySuccess, + _ => HealthCheckProbePolicy.MajoritySuccess, + }, + }; + + Assert.Equal(HealthCheckResult.Inconclusive, await check.CheckHealthAsync(conn)); + } + [Fact] public void OptionsCarryTheFailbackHealthCheck() { From 059f7dc478fb257bbb685b038624a8895c032650 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 13 Oct 2026 11:29:19 +0100 Subject: [PATCH 30/34] Move the lag-aware log message to a free event id (again) main has since taken 128 as well. --- src/StackExchange.Redis/LoggerExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/StackExchange.Redis/LoggerExtensions.cs b/src/StackExchange.Redis/LoggerExtensions.cs index f8612e8dd..917d71ed6 100644 --- a/src/StackExchange.Redis/LoggerExtensions.cs +++ b/src/StackExchange.Redis/LoggerExtensions.cs @@ -842,7 +842,7 @@ internal static void LogWithThreadPoolStats(this ILogger? log, string message) [LoggerMessage( Level = LogLevel.Warning, - EventId = 128, + EventId = 130, Message = "Lag-aware health check could not ask {RestEndpoint}: {Reason}")] internal static partial void LogWarningLagAwareInconclusive(this ILogger logger, string restEndpoint, string reason); } From ec121303620888e909b85055da8809c884ae97e6 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 13 Oct 2026 11:54:06 +0100 Subject: [PATCH 31/34] Failback: a zero FailbackDelay clears a failure in the same clock tick; no ValueTuple UpdateState compared the last failure strictly before the cutoff. With FailbackDelay zero the cutoff is now, and on .NET Framework's ~15.6ms clock a failure and the passing check that clears it often share a tick, holding the member out for another interval. Compare inclusively. Also replace a named tuple in the failback-check pass: the library must not reference System.ValueTuple. --- .../Availability/MultiGroupMultiplexer.cs | 23 +++++++++++++++---- .../FailbackHealthCheckTests.cs | 15 ++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs index 2a2d8e76b..62a5d2ec6 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs +++ b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs @@ -125,6 +125,8 @@ internal void SetUnhealthy() // UTC ticks (DateTime.Ticks and TimeSpan.Ticks share the same 100ns unit); long for atomicity private long _lastUnhealthyTicks; + internal long LastUnhealthyTicks => Volatile.Read(ref _lastUnhealthyTicks); + /// /// Clear the flag against this endpoint, allowing it to be reconsidered. /// @@ -353,7 +355,10 @@ internal GroupConnectionChangedEventArgs.ChangeType UpdateState(HealthCheckResul if (isConnected) { - if (Volatile.Read(ref _lastUnhealthyTicks) < failbackFailureCutoffTicks) ResetIsUnhealthy(); + // at or before the cutoff: with a zero FailbackDelay the cutoff is "now", and on a coarse clock + // (DateTime.UtcNow advances every ~15.6ms on .NET Framework) a failure and the pass that clears + // it can share a tick - a strict comparison would then hold the member out for another interval + if (Volatile.Read(ref _lastUnhealthyTicks) <= failbackFailureCutoffTicks) ResetIsUnhealthy(); } else { @@ -707,7 +712,7 @@ private async Task RunFailbackChecksAsync(ConnectionGroupMember[] members) // stop being provisional once it has caught up; a normal active member is not var active = _activeStub.Active; var provisional = Volatile.Read(ref _provisional); - List<(ConnectionGroupMember Member, Task Pending)>? checks = null; + List? checks = null; int totalTimeoutMillis = 0; foreach (var member in members) { @@ -721,7 +726,7 @@ private async Task RunFailbackChecksAsync(ConnectionGroupMember[] members) } totalTimeoutMillis = Math.Max(totalTimeoutMillis, check.TotalTimeoutMillis()); - (checks ??= []).Add((member, check.CheckHealthAsync(member.Multiplexer))); + (checks ??= []).Add(new(member, check.CheckHealthAsync(member.Multiplexer))); } if (checks is null) return; @@ -730,8 +735,10 @@ private async Task RunFailbackChecksAsync(ConnectionGroupMember[] members) for (int i = 0; i < all.Length; i++) all[i] = checks[i].Pending; await Task.WhenAll(all).TimeoutAfter(totalTimeoutMillis).ForAwait(); - foreach (var (member, pending) in checks) + foreach (var item in checks) { + var member = item.Member; + var pending = item.Pending; if (pending.IsCompletedSuccessfully) { member.FailbackVerified = await pending.ForAwait() is HealthCheckResult.Healthy; @@ -744,6 +751,14 @@ private async Task RunFailbackChecksAsync(ConnectionGroupMember[] members) } } + // a named pair rather than a tuple: the library must not reference System.ValueTuple (see SanityCheckTests) + private readonly struct PendingFailbackCheck(ConnectionGroupMember member, Task pending) + { + public ConnectionGroupMember Member { get; } = member; + + public Task Pending { get; } = pending; + } + // Eligible to become (or stay) active: an explicit failover target always is, as is the active member // - unless it is provisional, having been chosen by the liveness fallback without ever passing its // failback check; otherwise a member with a failback check must have passed it on the latest pass. diff --git a/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs b/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs index 0ee9fe654..9de4768c5 100644 --- a/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs +++ b/tests/StackExchange.Redis.Tests/MultiGroupTests/FailbackHealthCheckTests.cs @@ -304,6 +304,21 @@ public async Task ARunOfInconclusiveProbesIsInconclusiveUnderEveryPolicy(string Assert.Equal(HealthCheckResult.Inconclusive, await check.CheckHealthAsync(conn)); } + [Fact] + public async Task AFailureInTheSameClockTickAsTheCutoffStillFailsBack() + { + // FailbackDelay zero means immediate: a passing check clears a failure recorded at the cutoff itself, + // which on .NET Framework's ~15.6ms clock is the ordinary case for two passes in quick succession + await using var f = new Fixture(log, (Alpha, 1), (Beta, 9)); + await f.ConnectAsync(); + var member = f.Members[0]; + + member.SetUnhealthy(); + Assert.True(member.IsUnhealthy); + member.UpdateState(HealthCheckResult.Healthy, failbackFailureCutoffTicks: member.LastUnhealthyTicks); + Assert.False(member.IsUnhealthy); + } + [Fact] public void OptionsCarryTheFailbackHealthCheck() { From c5c819b5f7a98965bed827b0975cbdc6f9f8e9a1 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 13 Oct 2026 14:16:30 +0100 Subject: [PATCH 32/34] Ship lag-aware failback without an experiment Remove SER011: the attributes, the constant and NoWarn entry, the [SER011] API prefixes and docs/exp/SER011.md. It never shipped, so it is not a reserved id. Also restores main's using directives and encoding in the files that already existed there, which earlier edits had disturbed. --- Directory.Build.props | 2 +- docs/Failover.md | 4 +- docs/exp/SER011.md | 36 ---------- src/RESPite/Shared/Experiments.cs | 1 - .../Availability/HealthCheck.LagAware.cs | 4 -- .../Availability/HealthCheckProbe.LagAware.cs | 3 - .../Availability/HealthCheckProbe.cs | 1 - .../Availability/HealthCheckProbePolicy.cs | 2 +- .../Availability/HealthCheckProbeScope.cs | 4 -- .../Availability/LagAwareOptions.cs | 4 -- .../Availability/MultiGroupMultiplexer.cs | 1 - .../Availability/MultiGroupOptions.cs | 2 - .../PublicAPI/PublicAPI.Unshipped.txt | 68 +++++++++---------- .../PublicAPI/net6.0/PublicAPI.Unshipped.txt | 2 +- .../README.md | 2 +- 15 files changed, 39 insertions(+), 97 deletions(-) delete mode 100644 docs/exp/SER011.md diff --git a/Directory.Build.props b/Directory.Build.props index 048e033ce..55c0088fe 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -12,7 +12,7 @@ NETSDK1069 - $(NoWarn);NU5105;NU1507;SER004;SER005;SER009;SER011 + $(NoWarn);NU5105;NU1507;SER004;SER005;SER009 https://github.com/StackExchange/StackExchange.Redis/releases https://seredis.dev/ MIT diff --git a/docs/Failover.md b/docs/Failover.md index 76974f202..58a4c43eb 100644 --- a/docs/Failover.md +++ b/docs/Failover.md @@ -83,7 +83,7 @@ Because the policies are immutable, there is no question of whether a change "ta | `RetryPolicy` | `MultiGroupOptions.RetryPolicy` | *(none; retry is applied per-database via `WithRetry`)* | | `HealthCheckInterval` | `MultiGroupOptions.HealthCheckInterval` | *(none; it is the group's re-evaluation cadence)* | | `FailbackDelay` | `MultiGroupOptions.FailbackDelay` | `ConnectionGroupMember.FailbackDelay` | -| `FailbackHealthCheck` (experimental, `SER011`) | `MultiGroupOptions.FailbackHealthCheck` | `ConnectionGroupMember.FailbackHealthCheck` | +| `FailbackHealthCheck` | `MultiGroupOptions.FailbackHealthCheck` | `ConnectionGroupMember.FailbackHealthCheck` | | `Weight` | *(none)* | `ConnectionGroupMember.Weight` | | `SkipInitialHealthCheck` | *(none)* | `ConnectionGroupMember.SkipInitialHealthCheck` | @@ -758,8 +758,6 @@ This guards against **flapping**: a member that is intermittently failing will k ### Failback health checks -> Experimental, behind diagnostic id `SER011`; see [SER011](exp/SER011.md). - `HealthCheck` answers "is this member alive". Some deployments need a second question answered before traffic moves *to* a member: "has it caught up". In an Active-Active deployment, a region that has just come back can be reachable but stale, and failing back onto it reads (and builds on) data that is behind. diff --git a/docs/exp/SER011.md b/docs/exp/SER011.md deleted file mode 100644 index 26618ff36..000000000 --- a/docs/exp/SER011.md +++ /dev/null @@ -1,36 +0,0 @@ -Failback health checks let a group ask, before moving traffic *to* a member, whether that member is fit to -take it: typically, in an Active-Active deployment, whether it has caught up with the others, rather than -only whether it is alive. See [Failback health checks](../Failover.md#failback-health-checks) for the rules. - -The diagnostic covers: - -- `ConnectionGroupMember.FailbackHealthCheck` and `MultiGroupOptions.FailbackHealthCheck` (and its builder); -- `HealthCheckProbe.Scope` and `HealthCheckProbeScope`, for probes that answer per member rather than per - endpoint. -- `HealthCheck.LagAware`, `HealthCheckProbe.LagAware`, `LagAwareOptions` and `LagCheckMode`: a probe of the - Redis Enterprise database availability API (see - [Redis Enterprise: lag-aware availability](../Failover.md#redis-enterprise-lag-aware-availability)). - -It is experimental for two reasons: - -1. **The selection rules are new and deliberately differ from other clients.** A failback check never evicts - the active member, and when no live member passes it, selection falls back to liveness rather than refusing - to serve. Both choices follow from measurements against real Active-Active deployments, where a partition - between regions leaves every member behind at once; they may still be refined. -2. **The Redis Enterprise API it talks to is documented as prose.** The probe is built against responses - captured from real deployments (Redis Enterprise 8.0), not against a published schema, and several of the - API's behaviours (the lag check only applying to Active-Active databases, a broken link going unnoticed - for about a minute) were found by measurement rather than documentation. The options and their defaults - may change as more deployments are exercised. - -If you accept the above, you can suppress this warning by adding the following to your `csproj` file: - -```xml -$(NoWarn);SER011 -``` - -or more granularly / locally in C#: - -``` c# -#pragma warning disable SER011 -``` diff --git a/src/RESPite/Shared/Experiments.cs b/src/RESPite/Shared/Experiments.cs index 55b146f61..93fd067f8 100644 --- a/src/RESPite/Shared/Experiments.cs +++ b/src/RESPite/Shared/Experiments.cs @@ -23,7 +23,6 @@ internal static class Experiments public const string Respite = "SER004"; public const string UnitTesting = "SER005"; public const string Transport = "SER009"; - public const string LagAwareFailover = "SER011"; // ReSharper restore InconsistentNaming diff --git a/src/StackExchange.Redis/Availability/HealthCheck.LagAware.cs b/src/StackExchange.Redis/Availability/HealthCheck.LagAware.cs index d5b789249..5ccbad622 100644 --- a/src/StackExchange.Redis/Availability/HealthCheck.LagAware.cs +++ b/src/StackExchange.Redis/Availability/HealthCheck.LagAware.cs @@ -1,6 +1,3 @@ -using System.Diagnostics.CodeAnalysis; -using RESPite; - namespace StackExchange.Redis.Availability; public sealed partial class HealthCheck @@ -14,7 +11,6 @@ public sealed partial class HealthCheck /// For different probe counts or timeouts, use /// new HealthCheck.Builder { Probe = HealthCheckProbe.LagAware(options), ... }. /// - [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public static HealthCheck LagAware(LagAwareOptions options) => new Builder { Probe = HealthCheckProbe.LagAware(options), ProbeCount = 1 }.Create(); } diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs b/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs index c6699bfea..2546df4f6 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbe.LagAware.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Concurrent; -using System.Diagnostics.CodeAnalysis; using System.IO; using System.Net; using System.Net.Http; @@ -11,7 +10,6 @@ using System.Text.Json; using System.Threading; using System.Threading.Tasks; -using RESPite; namespace StackExchange.Redis.Availability; @@ -28,7 +26,6 @@ public abstract partial class HealthCheckProbe /// problem is never mistaken for the database being down. Intended as a /// . /// - [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public static HealthCheckProbe LagAware(LagAwareOptions options) => new LagAwareProbe(options); internal sealed class LagAwareProbe : HealthCheckProbe diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbe.cs b/src/StackExchange.Redis/Availability/HealthCheckProbe.cs index f0f8a4b39..6edb08208 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbe.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbe.cs @@ -18,7 +18,6 @@ public abstract partial class HealthCheckProbe /// /// Whether this probe is invoked per endpoint (the default) or once per member. /// - [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public virtual HealthCheckProbeScope Scope => HealthCheckProbeScope.Endpoint; private static Task? _inconclusive, _healthy, _unhealthy; diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs b/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs index ecbc686c5..19e3869d3 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbePolicy.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Diagnostics.CodeAnalysis; using RESPite; diff --git a/src/StackExchange.Redis/Availability/HealthCheckProbeScope.cs b/src/StackExchange.Redis/Availability/HealthCheckProbeScope.cs index 906420a4a..77ce912d1 100644 --- a/src/StackExchange.Redis/Availability/HealthCheckProbeScope.cs +++ b/src/StackExchange.Redis/Availability/HealthCheckProbeScope.cs @@ -1,12 +1,8 @@ -using System.Diagnostics.CodeAnalysis; -using RESPite; - namespace StackExchange.Redis.Availability; /// /// How often a is invoked when a whole member is checked. /// -[Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public enum HealthCheckProbeScope { /// diff --git a/src/StackExchange.Redis/Availability/LagAwareOptions.cs b/src/StackExchange.Redis/Availability/LagAwareOptions.cs index 5b7280e06..e74b4f08d 100644 --- a/src/StackExchange.Redis/Availability/LagAwareOptions.cs +++ b/src/StackExchange.Redis/Availability/LagAwareOptions.cs @@ -1,12 +1,10 @@ using System; -using System.Diagnostics.CodeAnalysis; using System.Net; using System.Net.Http; using System.Net.Security; using System.Security.Cryptography.X509Certificates; using System.Threading; using System.Threading.Tasks; -using RESPite; namespace StackExchange.Redis.Availability; @@ -19,7 +17,6 @@ namespace StackExchange.Redis.Availability; /// an Active-Active member are discovered, and no lag tolerance is sent unless one is set, so the cluster's /// own setting applies. The options are read when the probe is created; later changes have no effect on it. /// -[Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public sealed class LagAwareOptions { /// @@ -119,7 +116,6 @@ public void SetUserPfxCertificate(string userCertificatePath, string? password = /// /// Whether a check asks the lag question. /// -[Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public enum LagCheckMode { /// diff --git a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs index 62a5d2ec6..a426fdb78 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs +++ b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs @@ -244,7 +244,6 @@ public bool SkipInitialHealthCheck /// leaves it ineligible. If no connected member is eligible, selection falls back to connectivity and /// alone, so the group always serves from somewhere when it can. /// - [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public HealthCheck? FailbackHealthCheck { get; set; } // the breaker to hand to this member's connections: member override, else its own config, else the group diff --git a/src/StackExchange.Redis/Availability/MultiGroupOptions.cs b/src/StackExchange.Redis/Availability/MultiGroupOptions.cs index 42fde340a..0377dd23e 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupOptions.cs +++ b/src/StackExchange.Redis/Availability/MultiGroupOptions.cs @@ -80,7 +80,6 @@ private MultiGroupOptions( /// The failback health check to use for members of the group when no per-member one is specified; /// (the default) means none. See . /// - [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public HealthCheck? FailbackHealthCheck { get; } /// @@ -140,7 +139,6 @@ public Builder(MultiGroupOptions options) /// The failback health check to use for members of the group when no per-member one is specified; /// (the default) means none. See . /// - [Experimental(Experiments.LagAwareFailover, UrlFormat = Experiments.UrlFormat)] public HealthCheck? FailbackHealthCheck { get; set; } /// diff --git a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt index 4fb5b51af..8cc5c21b9 100644 --- a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt +++ b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt @@ -1,35 +1,35 @@ #nullable enable -[SER011]StackExchange.Redis.Availability.ConnectionGroupMember.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? -[SER011]StackExchange.Redis.Availability.ConnectionGroupMember.FailbackHealthCheck.set -> void -[SER011]StackExchange.Redis.Availability.HealthCheckProbeScope -[SER011]StackExchange.Redis.Availability.HealthCheckProbeScope.Endpoint = 0 -> StackExchange.Redis.Availability.HealthCheckProbeScope -[SER011]StackExchange.Redis.Availability.HealthCheckProbeScope.Member = 1 -> StackExchange.Redis.Availability.HealthCheckProbeScope -[SER011]StackExchange.Redis.Availability.MultiGroupOptions.Builder.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? -[SER011]StackExchange.Redis.Availability.MultiGroupOptions.Builder.FailbackHealthCheck.set -> void -[SER011]StackExchange.Redis.Availability.MultiGroupOptions.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? -[SER011]virtual StackExchange.Redis.Availability.HealthCheckProbe.Scope.get -> StackExchange.Redis.Availability.HealthCheckProbeScope -[SER011]StackExchange.Redis.Availability.LagAwareOptions -[SER011]StackExchange.Redis.Availability.LagAwareOptions.CertificateValidation.get -> System.Net.Security.RemoteCertificateValidationCallback? -[SER011]StackExchange.Redis.Availability.LagAwareOptions.CertificateValidation.set -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.Credentials.get -> System.Func>? -[SER011]StackExchange.Redis.Availability.LagAwareOptions.Credentials.set -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.DatabaseId.get -> int? -[SER011]StackExchange.Redis.Availability.LagAwareOptions.DatabaseId.set -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagAwareOptions() -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagCheck.get -> StackExchange.Redis.Availability.LagCheckMode -[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagCheck.set -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagTolerance.get -> System.TimeSpan? -[SER011]StackExchange.Redis.Availability.LagAwareOptions.LagTolerance.set -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.RestEndpoint.get -> System.Uri? -[SER011]StackExchange.Redis.Availability.LagAwareOptions.RestEndpoint.set -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.TrustIssuer(string! issuerCertificatePath) -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.TrustIssuer(System.Security.Cryptography.X509Certificates.X509Certificate2! issuer) -> void -[SER011]StackExchange.Redis.Availability.LagCheckMode -[SER011]StackExchange.Redis.Availability.LagCheckMode.Auto = 0 -> StackExchange.Redis.Availability.LagCheckMode -[SER011]StackExchange.Redis.Availability.LagCheckMode.Disabled = 2 -> StackExchange.Redis.Availability.LagCheckMode -[SER011]StackExchange.Redis.Availability.LagCheckMode.Enabled = 1 -> StackExchange.Redis.Availability.LagCheckMode -[SER011]static StackExchange.Redis.Availability.HealthCheck.LagAware(StackExchange.Redis.Availability.LagAwareOptions! options) -> StackExchange.Redis.Availability.HealthCheck! -[SER011]static StackExchange.Redis.Availability.HealthCheckProbe.LagAware(StackExchange.Redis.Availability.LagAwareOptions! options) -> StackExchange.Redis.Availability.HealthCheckProbe! -[SER011]StackExchange.Redis.Availability.LagAwareOptions.ClientCertificate.get -> System.Security.Cryptography.X509Certificates.X509Certificate2? -[SER011]StackExchange.Redis.Availability.LagAwareOptions.ClientCertificate.set -> void -[SER011]StackExchange.Redis.Availability.LagAwareOptions.SetUserPfxCertificate(string! userCertificatePath, string? password = null) -> void +StackExchange.Redis.Availability.ConnectionGroupMember.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? +StackExchange.Redis.Availability.ConnectionGroupMember.FailbackHealthCheck.set -> void +StackExchange.Redis.Availability.HealthCheckProbeScope +StackExchange.Redis.Availability.HealthCheckProbeScope.Endpoint = 0 -> StackExchange.Redis.Availability.HealthCheckProbeScope +StackExchange.Redis.Availability.HealthCheckProbeScope.Member = 1 -> StackExchange.Redis.Availability.HealthCheckProbeScope +StackExchange.Redis.Availability.MultiGroupOptions.Builder.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? +StackExchange.Redis.Availability.MultiGroupOptions.Builder.FailbackHealthCheck.set -> void +StackExchange.Redis.Availability.MultiGroupOptions.FailbackHealthCheck.get -> StackExchange.Redis.Availability.HealthCheck? +virtual StackExchange.Redis.Availability.HealthCheckProbe.Scope.get -> StackExchange.Redis.Availability.HealthCheckProbeScope +StackExchange.Redis.Availability.LagAwareOptions +StackExchange.Redis.Availability.LagAwareOptions.CertificateValidation.get -> System.Net.Security.RemoteCertificateValidationCallback? +StackExchange.Redis.Availability.LagAwareOptions.CertificateValidation.set -> void +StackExchange.Redis.Availability.LagAwareOptions.Credentials.get -> System.Func>? +StackExchange.Redis.Availability.LagAwareOptions.Credentials.set -> void +StackExchange.Redis.Availability.LagAwareOptions.DatabaseId.get -> int? +StackExchange.Redis.Availability.LagAwareOptions.DatabaseId.set -> void +StackExchange.Redis.Availability.LagAwareOptions.LagAwareOptions() -> void +StackExchange.Redis.Availability.LagAwareOptions.LagCheck.get -> StackExchange.Redis.Availability.LagCheckMode +StackExchange.Redis.Availability.LagAwareOptions.LagCheck.set -> void +StackExchange.Redis.Availability.LagAwareOptions.LagTolerance.get -> System.TimeSpan? +StackExchange.Redis.Availability.LagAwareOptions.LagTolerance.set -> void +StackExchange.Redis.Availability.LagAwareOptions.RestEndpoint.get -> System.Uri? +StackExchange.Redis.Availability.LagAwareOptions.RestEndpoint.set -> void +StackExchange.Redis.Availability.LagAwareOptions.TrustIssuer(string! issuerCertificatePath) -> void +StackExchange.Redis.Availability.LagAwareOptions.TrustIssuer(System.Security.Cryptography.X509Certificates.X509Certificate2! issuer) -> void +StackExchange.Redis.Availability.LagCheckMode +StackExchange.Redis.Availability.LagCheckMode.Auto = 0 -> StackExchange.Redis.Availability.LagCheckMode +StackExchange.Redis.Availability.LagCheckMode.Disabled = 2 -> StackExchange.Redis.Availability.LagCheckMode +StackExchange.Redis.Availability.LagCheckMode.Enabled = 1 -> StackExchange.Redis.Availability.LagCheckMode +static StackExchange.Redis.Availability.HealthCheck.LagAware(StackExchange.Redis.Availability.LagAwareOptions! options) -> StackExchange.Redis.Availability.HealthCheck! +static StackExchange.Redis.Availability.HealthCheckProbe.LagAware(StackExchange.Redis.Availability.LagAwareOptions! options) -> StackExchange.Redis.Availability.HealthCheckProbe! +StackExchange.Redis.Availability.LagAwareOptions.ClientCertificate.get -> System.Security.Cryptography.X509Certificates.X509Certificate2? +StackExchange.Redis.Availability.LagAwareOptions.ClientCertificate.set -> void +StackExchange.Redis.Availability.LagAwareOptions.SetUserPfxCertificate(string! userCertificatePath, string? password = null) -> void diff --git a/src/StackExchange.Redis/PublicAPI/net6.0/PublicAPI.Unshipped.txt b/src/StackExchange.Redis/PublicAPI/net6.0/PublicAPI.Unshipped.txt index b10d80f07..04376438f 100644 --- a/src/StackExchange.Redis/PublicAPI/net6.0/PublicAPI.Unshipped.txt +++ b/src/StackExchange.Redis/PublicAPI/net6.0/PublicAPI.Unshipped.txt @@ -1,2 +1,2 @@ #nullable enable -[SER011]StackExchange.Redis.Availability.LagAwareOptions.SetUserPemCertificate(string! userCertificatePath, string? userKeyPath = null) -> void +StackExchange.Redis.Availability.LagAwareOptions.SetUserPemCertificate(string! userCertificatePath, string? userKeyPath = null) -> void diff --git a/tests/StackExchange.Redis.FaultInjector.Tests/README.md b/tests/StackExchange.Redis.FaultInjector.Tests/README.md index 9e9d2d4ee..95c3d3c05 100644 --- a/tests/StackExchange.Redis.FaultInjector.Tests/README.md +++ b/tests/StackExchange.Redis.FaultInjector.Tests/README.md @@ -127,7 +127,7 @@ otherwise looks exactly like a failback that never happened. ## Lag-aware failback -`LagAwareFailbackScenarioTests` checks the lag-aware failback check (`HealthCheck.LagAware`, `SER011`) against the +`LagAwareFailbackScenarioTests` checks the lag-aware failback check (`HealthCheck.LagAware`) against the same `re-active-active` pair: the group must not fail back onto a member that is alive but behind, and must return once it has caught up. It needs credentials for **both** clusters in `env_output.json` (the AWS multi-cluster template has them) and skips otherwise. From 2c0540bd18fb4d6f3ef224f40696a7cb77c9df0c Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 13 Oct 2026 14:16:30 +0100 Subject: [PATCH 33/34] Notes: no experiment --- notes/lag-aware/api-sketch.md | 5 +++-- notes/lag-aware/plan.md | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index fd7938c27..c18c91b78 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -6,8 +6,9 @@ with stubbed-transport tests replaying the measured response shapes. The fault-i against a live Active-Active pair. Not yet: client-certificate support for the REST call. Evidence is in [`findings.md`](findings.md) (section 10 especially); sequence in [`plan.md`](plan.md). *Correction:* `SER007` was retired when geo-redundant failover went GA (#3247), so the shipped -`Availability/` API is not experimental; everything new here is behind a new diagnostic, **`SER011`** -(`Experiments.LagAwareFailover`, documented in `docs/exp/SER011.md`). +`Availability/` API is not experimental. Everything new here was first gated behind a new diagnostic, +`SER011`; *decided 2026-10-13:* it ships **without** an experiment, like the rest of `Availability/`, and +`SER011` was removed before ever shipping (so it is not a reserved ID). Aim: parity with Lettuce, Jedis, redis-py and go-redis where it serves users, and deliberate, documented deviations where the measurements say the others get it wrong. diff --git a/notes/lag-aware/plan.md b/notes/lag-aware/plan.md index f44d2837f..41d3d8a95 100644 --- a/notes/lag-aware/plan.md +++ b/notes/lag-aware/plan.md @@ -6,8 +6,8 @@ sequence. Status: **proposed. Step 0 is cleared; step 1 is answered (findings §10): the lag check flips under real Active-Active lag, on the stale member only, and the tolerance is honoured on a running link. Cross-region lag magnitudes remain unmeasured (both test clusters share a region).** The core change -(failback health check, member-scoped probes, selection rules) and the REST probe are implemented behind -`SER011`, with stubbed-transport tests (step 5, tier 1) and a fault-injector scenario test (tier 3, +(failback health check, member-scoped probes, selection rules) and the REST probe are implemented (not +experimental: `SER011` was removed before shipping), with stubbed-transport tests (step 5, tier 1) and a fault-injector scenario test (tier 3, `LagAwareFailbackScenarioTests`, passing against a live Active-Active pair on 2026-10-08). Tier 2 (the Kestrel toy) is not done and arguably not needed now that tiers 1 and 3 exist. See [`api-sketch.md`](api-sketch.md). From 2c8c9c3025ad2db696ef0ad1fb702920161c1d9a Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 13 Oct 2026 14:17:43 +0100 Subject: [PATCH 34/34] Notes: drop the SER011 history --- notes/lag-aware/api-sketch.md | 4 +--- notes/lag-aware/plan.md | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/notes/lag-aware/api-sketch.md b/notes/lag-aware/api-sketch.md index c18c91b78..169039ce0 100644 --- a/notes/lag-aware/api-sketch.md +++ b/notes/lag-aware/api-sketch.md @@ -6,9 +6,7 @@ with stubbed-transport tests replaying the measured response shapes. The fault-i against a live Active-Active pair. Not yet: client-certificate support for the REST call. Evidence is in [`findings.md`](findings.md) (section 10 especially); sequence in [`plan.md`](plan.md). *Correction:* `SER007` was retired when geo-redundant failover went GA (#3247), so the shipped -`Availability/` API is not experimental. Everything new here was first gated behind a new diagnostic, -`SER011`; *decided 2026-10-13:* it ships **without** an experiment, like the rest of `Availability/`, and -`SER011` was removed before ever shipping (so it is not a reserved ID). +`Availability/` API is not experimental, and nothing here is either. Aim: parity with Lettuce, Jedis, redis-py and go-redis where it serves users, and deliberate, documented deviations where the measurements say the others get it wrong. diff --git a/notes/lag-aware/plan.md b/notes/lag-aware/plan.md index 41d3d8a95..9faef4b75 100644 --- a/notes/lag-aware/plan.md +++ b/notes/lag-aware/plan.md @@ -6,8 +6,8 @@ sequence. Status: **proposed. Step 0 is cleared; step 1 is answered (findings §10): the lag check flips under real Active-Active lag, on the stale member only, and the tolerance is honoured on a running link. Cross-region lag magnitudes remain unmeasured (both test clusters share a region).** The core change -(failback health check, member-scoped probes, selection rules) and the REST probe are implemented (not -experimental: `SER011` was removed before shipping), with stubbed-transport tests (step 5, tier 1) and a fault-injector scenario test (tier 3, +(failback health check, member-scoped probes, selection rules) and the REST probe are implemented, with +stubbed-transport tests (step 5, tier 1) and a fault-injector scenario test (tier 3, `LagAwareFailbackScenarioTests`, passing against a live Active-Active pair on 2026-10-08). Tier 2 (the Kestrel toy) is not done and arguably not needed now that tiers 1 and 3 exist. See [`api-sketch.md`](api-sketch.md).