From bce5cad97007f3f83bfd2ad20fe14e57d056c75e Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:44:58 +0100 Subject: [PATCH 1/6] 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 b9c9916e41696a49d5bdc6922a80ae5509927338 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:45:42 +0100 Subject: [PATCH 2/6] 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 f9500164232b16d3d46fc6fb7aee911dcec75ca2 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:47:06 +0100 Subject: [PATCH 3/6] 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 1e44c04f27c38015766ec42fb83cc27cdd29185b Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:49:34 +0100 Subject: [PATCH 4/6] 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 f31b6a6ce0b9fd264387f31881798f189abc9375 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:56:53 +0100 Subject: [PATCH 5/6] 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 916a5635cd0a72f80b6a5dfc5eb1f6a76eee97d1 Mon Sep 17 00:00:00 2001 From: mgravell Date: Fri, 18 Sep 2026 05:34:02 +0100 Subject: [PATCH 6/6] 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