Maintenance notifications: the MOVING resolve primitive, and a fake that closes the socket (part of D6) - #3202
Merged
Conversation
MOVING's defining half was missing: the socket goes away. Measured on RS (2026-08-28) the close lands at +18.4s and +16.6s against a declared 15s window, so the window is a floor with slack rather than a deadline - the fake defaults to announced-plus-slack, and tests assert that we act within the window, never that the socket survives to the end of it. A shorter delay is available to exercise a less generous proxy. Blast radius is the node, not the connection: four connections to one node differing only in handshake all closed simultaneously, and only the opted-in ones were warned. So the close is scoped to siblings sharing a node, and D6 will reuse RetireAsync rather than anything narrower. A zero delay is deliberately not a case: it races delivery of the notification itself, which no real timing produces. My first version of this test asserted it, and it failed for that reason.
Measured behaviour makes this a poll, not a lookup. Relative to the
notification: the endpoint moves server-side at +8.6s, DNS follows at +9.7s
and +4.4s across two runs, and the sockets close at +18.4s and +16.6s -
against a declared 15s grace and a 5s TTL. So the first answer names the
address we were just told to leave, in every run observed, and a client that
treats it as authoritative hands off to the node it is trying to escape. The
short TTL is what makes polling work: several attempts fit in the window.
MovingEndpointProbe is deliberately pure - the caller supplies the resolver,
the interval and the budget - because the alternative is untestable: no
in-process fake can move a DNS record. Jitter stays at the call site with the
existing refresh jitter.
Returning null when the window expires is a result, not a failure: the server
closes the socket anyway and the relaxed window covers the reconnect, so
guessing an address would be worse than doing nothing.
Seven tests, including the ones that matter: DNS trailing the notification,
a resolution blip mid-handoff, a round-robin record naming both nodes at once,
and a zero window still getting one attempt ("act now", not "do nothing").
…the norm Measured 2026-08-28, all on a 5s TTL: all-nodes 2 A records, all-master-shards 3, single 1 - and an all-master-shards database whose shards shared a node also resolved to 1. So the count follows actual proxy placement rather than the policy name, and `single` (the shape the MOVING timeline was measured on) is the unusual one. The rule survives unchanged, which is the useful part: "take any address that is not the one being retired". With several records the first resolution already names a live sibling proxy, so the handoff steps sideways at once rather than waiting ~9s for DNS - any proxy of the same database serves the same data. The poll only engages when the record names nothing but the retiring address, which is exactly where waiting is the only option. Nothing reads the policy, so placement-driven counts need no special case. Two tests pin the branches, and the log now distinguishes them, because "stepped sideways to a sibling that was already advertised" and "the record has moved to the replacement" look identical otherwise and mean different things when someone is debugging a handoff. Note a full-fleet operation can hand us a sibling that is also about to be retired, so handoffs can chain. Self-limiting - each MOVING carries its own window and relaxation - but worth recognising rather than mistaking for a loop.
The measured gap this closes: on a multi-proxy database, taking a node out on the *shrink* path announces nothing about the endpoint, drops the victim from DNS at +21.4s, and closes its socket silently at +34.7s. So for thirteen seconds the condition is plainly visible to anybody who asks - our address is no longer advertised - and the client's only other signal is a socket dying with no explanation. MIGRATED lands ~5s before DNS moves and is the one notification the server retains, which makes it the prompt to ask on. IsStillAdvertisedAsync returns bool?, and the null carries weight: a resolution failure, or a record momentarily resolving to nothing, is "cannot tell" and must never become "give it up", or one DNS blip recycles every healthy connection at once. MovingEndpointProbe -> AdvertisedAddressProbe: the name stopped describing it once it answered two questions. Both reduce to "what does the record say now, and is my address in it", which is why this is one primitive and not two. Also fixes MaintenanceOptInClientTests.OptInIsReArmedOnReconnect, which asserted *client* state immediately after observing *server* state: the server counts the opt-in when it processes the request, we mark the feature live when we read the reply, a beat later. Intermittent under two cores, mine, and on a branch that was already pushed - so it would have surfaced in CI rather than here. Six consecutive clean runs after polling for the client side.
…e race Nine observations now fit one rule: MOVING is emitted when the endpoint's address set GAINS a member, and is silent when it only loses members. Policy narrowing, maintenance_mode, a 3->2 exclude and a reduction to a single proxy all only shrink, and all were silent; a substitution on that surviving single proxy announced. So the discriminator is neither "single proxy" nor placement. The consequence for the probe is a third outcome, now documented as measured fact rather than as a defensive branch. On one cluster DNS was correct 4.4-9.7s after MOVING, comfortably inside the 15s grace; on another it updated at +18.7s, three seconds AFTER the socket closed at +15.7s. So "window expired with the record still stale" is normal, and the only move left is to reconnect after the close and resolve then - which for a hostname endpoint is already correct. Anybody reading the null return as unreachable would be deleting the handling for a case that happens. Also recorded why the rule stays "any address that isn't mine" rather than "prefer a newly appeared address", despite MOVING marking precisely the moment something joins: a live sibling is at least as good and is available now, while the newcomer is invisible until the record updates. Preferring it means waiting, and waiting is the failure mode. Replacement proxies were measured accepting connections at +6.3s while DNS still advertised only the retiring node - which is a good argument for remembering addresses, deferred because a remembered address whose port was reassigned would be a silent wrong-server connection and a proxied standalone gives us no identity check to catch it.
…deployment (rest of D6, and D9's dedicated testing) (#3203) * A fault-injector test tier: one folder in, databases provisioned per shape New project, tests/StackExchange.Redis.FaultInjector.Tests, net10.0 only - these tests are about server behaviour, not our down-level targets. Picked up by Build.csproj's glob so it compiles in CI, but CI's test step names the main project explicitly, so it never runs there; build.ps1's traversal does run it, which is why the skip behaviour has to be right. One path is the whole configuration: SER_FI_CONFIG_DIR (or the console's FI_CONSOLE_CONFIG_DIR) points at the directory already mounted into the injector as /app/config, so cluster credentials, the CA certificate and the compose file are all found rather than hand-carried into the run. Three states, deliberately distinct: no directory skips; a directory without E2E_SCENARIO_TESTS=true skips (these create and delete real databases); and configured-and-meant-but-broken FAILS. The third is the point - a suite that skips on a broken environment reports success for tests that never ran, and gets trusted at exactly the wrong moment. All three verified. Databases are provisioned by the tests, per shape rather than per test, which removes the conveyance problem entirely: a test that asked for oss_cluster knows what it asked for, so endpoints.json stops being the source of truth for per-database facts and its missing oss_cluster/endpoint_type fields stop mattering. Shapes exist because they change behaviour - A-record count follows proxy placement, and the handoff branches on whether a live sibling exists. Named sertest-<shape>-<runid>. Cleanup is per fixture and unconditional; the startup sweep matches the sertest- prefix and nothing else, so it can never touch a database created by hand. Port collisions retry upward, as go-redis has to. TLS trusts the environment's CA via TrustIssuer. If the CA is missing, TLS tests fail rather than disabling validation: a TLS test that quietly stops checking identity reports success for the one thing it exists to catch. Two traps from the console's known-gaps are encoded rather than left to be rediscovered: poll on pending AND running (a loop waiting only on pending returns while the job is still going), and setup_id lives in the injector's memory so teardown keeps a bdb_id fallback. Teardown also runs on cancellation, with its own budget - the one place the ambient test token must not apply. Unverified and flagged in the README: the create_database parameter names are the injector's prose-documented wire schema, gathered in one place so a real run can correct them. * Prove the fault-injector tier live, and narrow the MOVING rule Run against a real RS 8.0.22 deployment: both template databases connect, negotiate RESP3 and report the opt-in active, and all four topology-change-standalone scenarios run end to end in 7m32s with the notifications observed and parsed. Cleanup verified - four scenarios left the cluster with exactly its two original databases. The rule the measurements produced is a conjunction, narrower than either half: MOVING fires when the connection's own proxy LEAVES the endpoint's address set AND the set GAINS a member. The counter-example is dns_resolution_change, which widens single -> all-master-shards: addresses are plainly added, yet nothing is announced, because the client's proxy is not going anywhere - and then the proxy restarts and the socket closes at +44.5s with no warning. That also resolves what looked like a contradiction, maintenance_mode announcing on a single-proxy database but not on a multi-proxy one: with one proxy, moving it *is* a substitution. The scenario expectations encode this, silence included, so a build that starts announcing the widening case tells us rather than passing quietly. Three more findings. The window overshot again, by 19.1s and 17.5s against a declared 15s, so "floor with slack" has four independent measurements and no counter-example. The sequence counter is shared across all types including MOVING (0, 1, 2 in one chain), which the per-type dedup already assumed. And data_movement_no_conn_drop moved shards with both notifications delivered and the connection never disturbed, so MIGRATING does not imply an impending disconnect. Corrections to the harness from real responses, replacing guesses: - scenario setup provisions its own database and returns setup_id, bdb_id, db_name, endpoints, password, tls, mtls_files and config in ~12s, so scenario tests need neither create_database nor endpoints.json nor the REST API - every trigger publishes the dbconfig it requires, and all four want proxy_policy: single, which no template creates - hence setup provisioning - setup_id is a handle, not an action id: polling /action/{setup_id} 404s - the create_database schema now matches bdb_config.json, which disambiguated oss_cluster_api_preferred_endpoint_type (ip vs hostname, and therefore whether a TLS client can verify its targets) from ..._preferred_ip_type (internal vs external routing) - I had conflated them Traversal run with no environment configured: 7 skipped, everything else green. * Cover the injector's scenario families, and sort what is left into buckets Ran the fault injector's scenarios against the live RS 8.0.22 cluster and added the ones that hold their value as tests. Green live: the OSS cluster family (SMIGRATING/SMIGRATED parsed to source -> target, with 1440 reads and zero failures across a real shard migration), sharded subscriptions recovering unaided after a migration - D5's resubscription, previously fake-only - the failover pair (FAILING_OVER seq=0 time=2s ["52"], FAILED_OVER seq=1) received end to end for the first time, and proxy restart recovery. Four schema facts the injector taught us, each replacing a guess: - create_database wants its config nested under "database_config"; a flat payload is rejected with "got None" - sharding requires shard_key_regex, or Redis Enterprise refuses the database with "Invalid sharding configuration" - /slot-migrate/setup's trigger is how to *provision* (only "reshard"), not how to migrate, and its effect enum is narrower than the discovery endpoint's - remove-add cannot be set up at all - setup provisions a database and returns it, so scenario tests need neither create_database nor endpoints.json Also two harness fixes worth their own mention. The create retry loop retried everything, so "missing shard_key_regex" arrived eight times over half a minute instead of once; it now retries only port collisions. And the traceback summariser split on '\n' when the injector's JSON carries the two characters backslash-n, so every skip message was a wall of Python. Scenarios this cluster cannot produce - add and slot-shuffle need a node with several shards, and three nodes with sparse placement give one each - now skip on a matched message rather than failing. Deliberately not run while unattended: shard/node/proxy/cluster failure, node_remove and reset_cluster, which damage or reset the cluster. The four-bucket assessment is in the notes: what works, what this feature still owes (D6's action half, the connect-failure trigger, moving-endpoint-type, MAINT_NOTIFICATIONS_INFO), what already works outside the feature, and what is untested - of which network_latency matters most, because it is how timeout attribution finally gets live evidence. * Reach the TLS variant, and diagnose why it cannot run here include_tls does not request TLS - it widens the list of variants setup may choose from, and variant_index picks one. With no flags a trigger offers one variant (single), with include_tls two (single, single_tls), with include_mtls a third (mtls). Passing include_tls alone provisions variant 0 and yields a plaintext database, which is how the first attempt skipped itself. With variant_index=1 the database came up TLS-enabled and the connect was refused: the remote certificate was rejected by the validation callback. That is the environment, not us - the folder's server certificate covers *.marcgravell-test-46be1d08... while the live cluster is marcgravell-test-e21cd75d..., left over from an earlier provision and three days older than the env_output.json beside it. Our behaviour was right: TrustIssuer tolerates chain errors only, so a name mismatch fails outright, which is the whole point of it. So the test now compares the certificate's DNS names against the cluster name *before* provisioning anything, and skips naming both. Without that, a stale certificate reads as a client bug and costs somebody an hour of certificate archaeology; the check costs nothing and happens before a database exists. Also set AbortOnConnectFail=true in the TLS test only. Everywhere else tolerating a slow start is right, but with it false a certificate problem is indistinguishable from a slow cluster: ConnectAsync succeeds, IsConnected is false, and the reason is gone. That is exactly how the first failure presented. Traversal with no environment: 15 skipped, everything else green. * D6: act on MOVING instead of waiting to be disconnected Today a MOVING is survivable - the socket closes and we reconnect - but the announced window goes entirely unused, and the reconnect re-resolves to whatever DNS says at the moment the server chose, which has been measured as still naming the node being retired. This uses the window: wait for DNS to move, then pick the moment ourselves. The dispatch turns on the form of the endpoint, which also corrects the earlier assumption that MOVING should reuse endpoint retirement: - hostname, no successor (every observed MOVING): the ServerEndPoint stays, only the address behind the name moves, so retiring it would delete our only route to the deployment. Probe until the record moves, then recycle the connections so they re-resolve. - address with a named successor: genuinely a different endpoint, so re-read the topology. Never observed - eleven routes, all explicit nulls - so this exists because the contract has it. - address, no successor: nothing to re-resolve and nowhere named to go. Doing nothing is correct. Deciding is separated from acting so the decision can be tested exhaustively without a server: DecideAsync takes the endpoint, the current address, the window and a resolver. That seam exists because the whole thing turns on DNS *changing*, which no in-process fake can arrange - ConnectionMultiplexer.AddressResolver defaults to real DNS. Recycling is a dispose: that already routes through RecordConnectionFailed to OnDisconnected, which reconnects immediately, so there is no new lifecycle to get wrong. Both bridges, because the measured blast radius is the node. Drained first, bounded by what is left of the window - the socket dies at the end regardless, so anything undrained was going to fail either way and draining strictly dominates. Jitter is a fraction of the window rather than a flat delay, capped at a second. A 2s window - which the shard notifications really do announce - must not spend half of itself waiting, and a 15s window does not justify a long wait when DNS has been seen moving after four seconds. Also safe from replay by construction, which is why there is no staleness guard: the server retains only shard-scoped completions, so a MOVING is never delivered as catch-up. Nine tests: five decision branches, jitter bounds, and an end-to-end recycle against the fake with MovingClosesConnection deliberately off, so the only thing that can replace the connection is our own handoff. Three consecutive two-core Release runs: 6215 passed, 0 failed. * D6 proven live, and the feedback loop it exposed On the real cluster the handoff does what it was built for: conn_drop/endpoint_rebind MOVING +9.3s -> recycled and reconnected +9.5s server would have closed at +25.5s maintenance_mode MOVING +21.7s -> recycled and reconnected +21.9s server closed at +38.0s So we move roughly sixteen seconds before being pushed, on both routes. The first live run also found a bug that no fake could have produced: a server re-sends MOVING to a connection that opts in while the window is still open. Since the handoff replaces the connection, acting on the repeat loops - recycle, reconnect, get told again, recycle - and it produced twelve recycles from a single event. OnMaintenanceWindowOpened already claimed the sequence id and knew it was a repeat; the handoff was not asking. It now returns whether the notification was new and the handoff gates on it, and the live test asserts *exactly one* recycle. Second finding, recorded rather than fixed: our own recycle does not raise ConnectionFailed, because disposal is not reported as a failure. From outside the library a handoff is therefore invisible - an operator sees a reconnect with no reason given. HandoffRecycles and LastHandoffOutcome exist because Multiplexer.Trace is [Conditional("VERBOSE")] and compiles away, so there would otherwise be no record at all of what a handoff decided. Whether it should surface something publicly is a real question, not settled here. Two consecutive two-core Release runs: 6215 passed, 0 failed. * Report a handoff as MaintenanceHandoff rather than silently A handoff was invisible from outside the library: the replacement connection raises ConnectionRestored, but our own recycle raised nothing, so a consumer tracking connection state saw a restore with no matching failure and no reason for the churn. The reporting block is gated on "if (_ioStream is not null || isInitialConnect)" - if *we* didn't burn the pipe, flag it - and Dispose runs Shutdown first, which is precisely why an ordinary dispose is silent. So the fix is ordering: record the failure while the pipe is still live, then dispose. ConnectionFailureType.MaintenanceHandoff is the right home. The existing event args already carry endpoint, connection type and a discriminator, and CircuitBreaker is the precedent for a deliberate client action reported this way. Documented for what it is: consumers alerting on ConnectionFailed should filter it out, since it means planned maintenance rather than a fault - and the test asserts we report *only* that, never SocketFailure or SocketClosed, so planned maintenance cannot end up in fault dashboards. Four consecutive two-core Release runs at 6215 passed. Note one earlier run reported two failures whose names I did not capture and which have not recurred in four runs since; if they come back I will capture them properly rather than guess. * Fix the cluster-flag call, which had been failing silently update_cluster_config wants its flags nested under "config" - the same shape create_database wants for "database_config" - and a flat payload is rejected with "Invalid parameter 'config': got None". Because the call is best-effort and only wrote to Console, it failed silently for a full day of testing without anybody noticing. Note the impact was small: the environment templates enable these flags at provision time, so this call is a safety net rather than the mechanism, and every test was passing on its own merits. It matters for an environment provisioned without them, where the alternative is every test failing at connect and blaming the client for a server-side setting. Verified corrected against the live injector. Fixture diagnostics now go to a collected SetupLog as well as the console, since a fixture has no test output helper and console writes are exactly what got lost. * Reach the migrations and the TLS variant that were being skipped Three "environment limitations" turn out to have been mine. add and slot-shuffle were skipping with "No node with multiple shards found", and remove-add was unreachable because /slot-migrate/setup's effect enum excludes it. The cluster was never the problem: the setup leg provisions one shard per node, so there is nothing to move a shard *from*. Provisioning our own database - six shards, dense placement, two per node over three nodes - and driving the run leg by bdb_id makes all three run, and all three now pass live. The generalisation is the useful part: a scenario setup cannot arrange is still reachable by provisioning the database ourselves. remove-add is the best of them: it moves every shard as five SMIGRATING/ SMIGRATED pairs sharing one sequence chain (0-9), which exercises the dedup and the event collapse far harder than a single migration. The first dense attempt failed for an unrelated reason: the client tried to reach 10.0.101.15, a VPC-private address, because a cluster database created without oss_cluster_api_preferred_ip_type defaults to *internal* - so CLUSTER SLOTS advertised addresses unreachable from outside the VPC. That is the field separated earlier from endpoint_type, biting for real: ip_type is internal-versus-external routing, endpoint_type is ip-versus-hostname identity. Shapes now default to external. TLS also runs now that the environment's certificates match the cluster: connected with validation on, opt-in active, MOVING received over TLS. My own mismatch detector had a bug worth recording - it read one name via GetNameInfo and compared it against the cluster name, so a wildcard "*.<cluster>" never matched, and it skipped a perfectly good environment. These certificates carry both a wildcard and the bare name, the hosts dialled are database endpoints *under* the cluster domain, and a wildcard correctly does not match its own parent. It now reads the full SAN list. Cluster left with exactly its two original databases. Local suite: 6215 passed.
mgravell
added a commit
that referenced
this pull request
Sep 1, 2026
… (D4, part of D5) (#3194) * Relax command timeouts while a server announces a disruption Stage 2 of maintenance notifications, and the first part that changes behaviour: an opening notification (MOVING, MIGRATING, FAILING_OVER, SMIGRATING) raises command timeouts for that server, and a closing one (MIGRATED, FAILED_OVER, SMIGRATED) stands them back down. Three settings, of which only the first is prescribed cross-client - the other two are ours, and say so in their XML docs: - MaintenanceRelaxedTimeout (10s): what timeouts are relaxed *to*, as a floor. The effective timeout is max(configured, this), so a caller with a generous timeout keeps it. - MaintenanceRelaxedWindowMax (3x): a backstop for a closing notification that never arrives. A window that never closes is worse than one that closes early. - MaintenancePostEventRelaxedDuration (2x, matching go-redis): a closing notification means the server-side operation finished, not that the server is back to normal latency - and completion is exactly when every other client that received the same notification re-engages. It does not apply after a cap expiry, where nothing told us the event finished and extending past the backstop would defeat it. The cap and tail derive from the *effective* relaxed timeout rather than the provider's, so raising the relaxed timeout cannot leave a cap below it; a provider can still pin either absolutely. Caught by its own test, which is why the test asserts the relationship and not just the numbers. Relaxation is server-scoped state read at sweep time, not stamped per message: both timeout sweeps rely on head-of-line ordering and stop at the first message that has not timed out, and per-message timeouts would make that short-circuit invalid - turning every heartbeat into a full scan of everything outstanding. The consequence is that relaxation covers whatever is already in flight, and stops covering it when the window closes, which is a further reason the tail earns its place. Wired into all three places a command timeout is enforced. Note the backlog sweep previously measured message age against _singleWriter.TimeoutMilliseconds - the write-lock acquisition timeout - which is about contention between writers rather than server latency; it now has its own expression, so relaxation cannot leak into lock acquisition. The sync path grows a re-wait loop, because a Monitor.Wait commits to a duration when it parks: without it, a sync caller in flight when a notification arrives would time out at the strict timeout while its async neighbour was relaxed. Also seqID dedup, per notification type, which lands here because a replayed opening notification extending a window is the first place a replay does damage. The sequence numbers are not defined by any specification, so this is deliberately conservative: an id we have already acted on is ignored, and nothing else is inferred. What is *not* relaxed, as a rule rather than a judgement call: keep-alive, the heartbeat, and connection-failure detection. Otherwise a server that died mid-maintenance would linger for the whole window, turning a latency mitigation into an availability regression. There is a test that kills a connection inside an absurdly generous window and requires the failure to still be noticed. * Report maintenance context on the faults it causes Closes out stage 2 with the fault surface: MaintenanceType on RedisTimeoutException, RedisConnectionException and FaultContext, defaulting to None. "Timeout" and "timeout during an announced failover" call for very different reactions from whoever reads the log, and until now the two were indistinguishable. Follows the established pattern on those types - Commandstatus and Flags on the timeout, FailureType on the connection fault - rather than introducing an exception type nobody catches yet, and named for the role rather than the type, as FailureType is. Both live in a partial alongside the rest of the feature, so Exceptions.cs is untouched. On FaultContext it is deliberately reported and not acted on: a fault during announced maintenance is expected and transient, and counting it towards a circuit-breaker trip that then withdraws a whole server is the opposite of what the notification was for - but "ignore faults during maintenance" is a judgement about a deployment, not about the protocol, so it belongs in policy. Also routes the effective timeout through the dead-socket heuristic in PhysicalBridge, which is derived from the command timeout and would otherwise contradict relaxation: with a relaxed timeout of 60s and that check firing at 4x the strict 5s, we would tear down precisely the connection relaxation was protecting. This is a refinement of the boundary rule, not an exception to it - socket-level failure detection is untouched, which is what actually guarantees a dead server is still noticed, and there is a test that kills a connection inside an absurdly generous window to prove it. * Read the nested cluster slot-migration payload Cross-checking our reading against the shipped clients (go-redis, redis-py) said SMIGRATED is nested, not flat: ["SMIGRATED", <seq>, [[source, target, slots], ...]] with slots a flat comma-and-range string inside each triplet. Two independent implementations agree on the nesting, which is much better evidence than our own reading of the prose - and it means we were *dropping* SMIGRATED, because the parser rejected any non-scalar element after the type. Safe, but wrong. The scalar-only guard stays for the DMC family, where nesting would signal a frame we do not understand; it is relaxed only for the two cluster kinds. A malformed triplet is skipped rather than losing the whole notification - the other triplets are still actionable, and it is what go-redis does. Exposed as ClusterSlotMigration on the event (source, target, parsed slot ranges, and the raw slot text so a list we could not parse still tells you something). Nothing acts on them yet. Two other things the cross-check turned up: - We required a readable sequence id and dropped the frame without one. go-redis length-checks these frames at two elements and reads no sequence number at all for the shard notifications, so that was stricter than a client which demonstrably works against real servers. Now a missing id costs only dedup - which is our own invention - rather than the notification. - SlotRange.TryParseInt16 was `checked`, so an out-of-range slot number threw OverflowException from a Try* method. Pre-existing and reachable today from the public SlotRange.TryParse and from CLUSTER NODES parsing; now reachable from the read loop too, where throwing is far worse than rejecting. It rejects. Fixing the parser needed two other things worth recording. RespReader's AggregateChildren() does not advance the parent reader, so MovePast(out reader) is required or the loop walks back into the children it just read and mistakes them for top-level elements. And in the toy server, Recycle() recurses, so a Standalone child inside a pooled parent is handed to a pool it never came from - Rent at every level. That one was invisible because the fake's write loop swallowed its exception into the pipe, with a reconnect covering the tracks; it now logs, which is the only reason the second bug took minutes rather than longer. * Document the maintenance options and named defaults providers Configuration.md gains the five new keys in the table, plus two sections: what a defaults provider is and why 'enterprise' exists (it cannot be detected - a self-managed cluster has whatever DNS its operator was given), and what the maintenance options do. Two things stated explicitly because they are the surprising parts: 'enabled' means *required* and will refuse a connection that cannot deliver notifications, and the maintenance durations are in seconds where every other timeout in that file is milliseconds. * Refresh topology when slots migrate away from us Reacting to SMIGRATED rather than waiting to be told by a -MOVED. This reuses the path AzureMaintenanceEvent has used for years - raise the event, then refresh - and is the whole of go-redis's SMIGRATED handling, so the risk profile is good: the worst case is the topology pass we already do. Three deliberate differences from that Azure precedent: - Scoped to triplets whose *source* resolves to us. Every node in the cluster reports the same movements, so most notifications describe somebody else, and refreshing on those means every client in the fleet re-reads topology whenever any shard moves anywhere. Resolution goes through the identity map, since a node answers to both its address and its announced hostname and the delta may name either. - Jittered by up to a second, because the fleet was all told the same thing at the same instant. Not configurable: the relaxed-window durations are options because their right value is deployment-specific and we invented them, whereas this is a fixed smear nobody needs to tune. - Cluster family only. MIGRATED and FAILED_OVER arrive in proxied deployments addressed as a single endpoint, where a refresh has no topology to learn, so they keep relaxation and nothing else. The jitter turned out to *defeat* the coalescing I was relying on: ReconfigureIfNeeded declines only while a refresh is in flight, and spreading a burst out means each pass completes before the next begins - so ten notifications became ten topology passes. Caught by the test that counts inbound CLUSTER commands. Coalescing therefore happens before the delay, via a pending flag, released when the refresh starts rather than when it finishes: anything arriving after that describes a state this pass may not have seen. Endpoints left serving no slots need no new code - the absence-based pruning from #3177 already retires them, and a refresh feeds exactly that path. It takes three generations rather than being immediate, which is slower than the HLD implies but is the existing tested policy, and is more than go-redis does at all. * Re-establish sharded subscriptions when their slots move Sharded subscriptions are slot-bound, so a slot leaving this node takes them with it. Mostly belt-and-braces: a server that migrates a slot also sends an unsolicited SUNSUBSCRIBE, and OnOutOfBand already resubscribes on that. This adds two things - it is pre-emptive where SMIGRATED arrives first, and it covers the case where the unsolicited unsubscribe never arrives or is lost, where the only other symptom is messages silently stopping, which nothing detects. It also knows *which* slots moved, so only the affected channels are touched rather than everything subscribed here. Ordinary pub/sub is not slot-bound and is deliberately left alone, even when the notification says every slot moved. Done before the refresh and without waiting for the jitter: a subscriber that is silently no longer subscribed is a correctness problem, where a stale slot map is an extra round trip. It resubscribes via *this* server rather than the migration target, reusing ResubscribeToServer unchanged - the outgoing node is the one we know has the new route, and sending there follows the redirect. The target is named in the notification and could be dialled directly, but it may be a node we have never seen or named in a form we cannot dial, and the redirect path is the one already proven by the SUNSUBSCRIBE case. Worth revisiting only with evidence. * Make the fake announce its own migrations, and fix what that exposed Migrate() only moved the slot in the fake's model - it emitted nothing, so the realistic sequence a client sees could not be reproduced. It now optionally announces itself (NotifyOnMigrate, off by default so existing tests that use Migrate to arrange a topology are undisturbed): SMIGRATING, an unsolicited sunsubscribe to subscribers of affected sharded channels, then SMIGRATED. Note that also gives the pre-existing unsolicited-SUNSUBSCRIBE path its first coverage from the fake; until now it could only be reached with a hand-built push frame. Turning it on immediately contradicted the resubscribe logic committed alongside it, in two stages: - Both signals fire for one migration, and both route to ResubscribeToServer, whose guard admits a subscription that is transiently attached to nothing. That measured six (re)subscribes for one channel. - Making it a delayed fallback - after the jitter, and only when the subscription is not attached elsewhere - fixes that. "Attached elsewhere" is the only reliable signal that the other path dealt with it; still attached to *us* is the pre-emptive case (notification beat the unsubscribe, subscription now stale) and attached to nothing is the stranded case, and both need acting on. An earlier guard on IsConnectedAny() got this wrong and silently disabled the pre-emptive case, which is the primary one. So it now costs a stranded subscription up to the jitter in recovery time, and costs nothing when it was not needed. Measured 4 (re)subscribes with notifications off and 5 with them on, the extra being the fallback acting on a subscription the unsubscribe path left attached to nothing - the feature working, not duplicate work. One thing this turned up and does not fix: after a real migration in the fake, a message published to the moved channel is not delivered - with notifications *disabled* as well, so it is either a pre-existing gap or a limitation of the freshly-added node. Deliberately not asserted, since attributing it here would blame this code for something it does not cause. Worth its own investigation. * Stop chasing a delivery failure that was never about migration The earlier note claimed a message published to a moved sharded channel is not delivered. Ran the control that should have come first: sharded pub/sub does not deliver in the fake *at all*, with no migration anywhere - SPUBLISH reports zero receivers. Evidence, for whoever picks this up: the client subscribes correctly, including following the redirect (SSUBSCRIBE on the old owner returns `-MOVED 15296 127.0.0.1:6380`, and SSUBSCRIBE then arrives on 6380), and SPUBLISH also arrives on 6380 - subscriber and publisher agree on the node. The node still answers `:0`. So the fake registers a sharded subscription somewhere its own publish lookup does not find it; a RedisChannel equality/options mismatch between the stored key and the lookup is the obvious first suspect. That makes it a gap in the fake rather than anything to do with maintenance notifications, and it means no test can currently assert sharded delivery against the toy server. The assertion and the per-node diagnostics are removed; the test keeps the property it can honestly own, which is that the resubscribe is bounded. * Toy server: sharded publish delivered to nobody, ever SPublish computed the node to filter by, with a comment saying so, and then did not pass it: var node = client.Node; // filter to clients on the same node ... PublishPair pair = new(channel, request.GetValue(2)); // node dropped ForAllClients(pair, static (client, pair) => ReferenceEquals(client.Node, pair.Node) ? client.Publish(...) : 0); PublishPair's node parameter is optional, so pair.Node was always null, ReferenceEquals never matched, and SPUBLISH answered :0 in every case - no migration required. Sharded pub/sub delivery has therefore never been covered by the fake at all, which also means a client-side sharded pub/sub bug could not have been caught here. Found while investigating an apparent "sharded subscription does not deliver after its slot migrates". It was not about migration, and it was not the client: the client followed the -MOVED correctly (SSUBSCRIBE arrived on the new owner) and routed SPUBLISH to the correct owner. The control with no migration at all is what settled it, and should have been the first experiment rather than the last. With that fixed, the end-to-end property can be asserted, so RealMigrationRecoversTheSubscriptionWithoutStorming now checks it: after a slot migration the sharded subscription delivers again. Published repeatedly, because pub/sub is fire and forget - a message published while the subscription is in flux is dropped, so losing messages during the tremor is expected while never delivering again is not, and one publish cannot distinguish them. * Retire nodes that leave the cluster, narrowed from "serving no slots" D5 asked for endpoints left serving no slots to be shut down. Narrowed deliberately: a node still listed in CLUSTER NODES is a live cluster member that may be given slots again, so dropping its connection is churn - and go-redis does not do it. Having *left* the cluster is the condition worth acting on, and the existing absence-based pruning already covers it; the notification-driven refresh is what makes us notice promptly. No client change was needed to demonstrate that, only the ability to express the scenario: RedisServer.RemoveNode removes a node the way CLUSTER FORGET does - gone from SLOTS and NODES, and every alias it answered to stops resolving. It refuses while the node still owns slots, as a real cluster does, since a topology with unowned slots says nothing useful about client behaviour. Two things this exposed, both about the policy rather than this feature: - Retirement can be starved. Pruning requires IsIdle(), which counts outstanding work, and we keep heart-beating the very node we are trying to retire - so a ping in flight makes it look busy. On a two-core runner that repeats often enough to prevent retirement indefinitely. The design notes recorded exactly this trap for the usage-based grace rule and dropped it for that reason; IsIdle() has the same problem. Excluding our own keep-alive traffic from the idleness test would fix it, and is a product change rather than something to paper over in a test - so the test is gated on a quiet machine and says why. - EndpointPruningUnitTests exercises the policy by feeding a SLOTS-only topology directly, which is not how a real refresh drives it. This test goes through ReconfigureAsync instead, which is why it sees the starvation at all. Also: this class is now non-parallel. Several of these wait out a jittered refresh, and the retirement one needs a quiet server, so sharing a machine with the rest of the suite measured as noise rather than signal. * Gate the retirement test, and record what the heartbeat theory did not explain The narrowed retirement is demonstrable on a quiet machine and fails about half the time on a two-core runner - because it does not happen, not because the test is impatient. Pruning requires IsIdle(), so something keeps the departed node looking busy. The obvious suspect was our own keep-alive: we heartbeat the very node we are trying to let go of, and an outstanding ping is enough to fail the idleness test. That theory was implemented (suppress keep-alive and the replication check for a ClusterTopology-provenance server absent from the topology) and it did *not* fix the flakiness, so the theory is wrong or incomplete and the change is reverted rather than shipped on a rationale the evidence contradicts. So the test is gated on a quiet machine with that stated, and the cause wants a focused look with instrumentation inside the pruning loop - not from a test, where an earlier attempt produced provenance readings that could not be trusted. * Record what makes a departed node look active: our own probes Measured from the snapshot the pruning loop walks, on a failing run: every term that should be true is - provenance=ClusterTopology, absentSince stable at 2, ownsSlot=False - and the blocker is outstanding work, growing ~170 per topology pass (176, 348, 520, ... 1339). That is not a keep-alive ping, which was the first theory and is why suppressing heartbeats did not help. It is the reconfigure's own autoconfigure probes to that node: it is gone, so nothing answers, and they accumulate in its backlog. IsIdle() counts backlog, so the node can never look idle - the more we look for it, the busier it appears. It only retires by winning a race on an early pass, which is exactly the load sensitivity observed. So the precondition is self-defeating in the case pruning exists for. Recorded in the test comment and the design notes; the fix is a product decision, with three candidates: exclude internal calls from the idleness measure, treat a disconnected bridge as idle since its outstanding work is doomed anyway, or stop probing servers awaiting retirement. * Idleness should count caller work, not ours Retirement requires IsIdle(), which counted *all* outstanding work. A node the topology has stopped listing still receives autoconfigure probes on every pass, and nothing answers them because it is gone, so they accumulate in its backlog: measured at ~170 per pass, growing without bound (176, 348, 520, ... 1339). The node therefore looked busy *because* we were looking for it, and could never be retired - the precondition defeated itself in precisely the case pruning exists for. The hidden internal-call flag already distinguishes our traffic from a caller's, so this is just a matter of asking the right question: IsIdle() now uses GetCallerOutstandingCount(), which walks the written-awaiting-response queue and the backlog and ignores anything flagged internal. That covers autoconfigure, handshake, and keep-alive traffic in one test, since all of it is flagged - which also disposes of the earlier keep-alive-specific theory properly rather than by suppressing heartbeats. Except that the *subscription* keep-alive was not flagged, unlike the interactive one which sets it via GetTracerMessage - so its ping, and its unsubscribe of a channel named after our own unique id, both looked like caller work. Now flagged, for consistency and because they plainly are ours. GetOutstandingCount() is unchanged and still counts everything: the retirement drain uses it, and waiting for our own in-flight probes to settle before tearing a connection down is the right behaviour there. Verified with the retirement test ungated: five clean two-core whole-suite runs, against roughly half failing before. Left alone deliberately: the availability health-check probes do not set the flag, so they still count as caller work. Arguably they should not, but IsInternalCall affects more than idleness, so that wants its own change rather than riding along here. * Clarify which subscription keep-alive actually fires The comment listed both branches without saying which one runs, and I had described them in review as though both were live traffic. Observed: against a 7.0 server the keep-alive is PING (answered with the two-element array pong), and the UNSUBSCRIBE fallback only fires against a server reporting older than 3.0, since PingOnSubscriber gates there and the default assumed version is 6.0. Also notes where the array-shaped pong is handled - IsArrayPong in OnResponseFrame - since that is not obvious from this end and is what stops the reply being taken for a pub/sub payload. * Drain on caller work too, and correct the record on the keep-alive Three corrections and one real fix, all from review. **The subscription keep-alive was already flagged.** KeepAlive has a common `if (msg != null) { msg.SetInternalCall(); ... }` after the switch, so both branches were already internal calls and the two per-branch calls added in 17151179 were redundant - removed. The commit message there claimed the subscription keep-alive "was not flagged", which is simply wrong; it was. **And the UNSUBSCRIBE branch is not "legacy".** Its condition is `IsAvailable(PING) && PingOnSubscriber`, so it is also reached when PING is disabled or renamed in the CommandMap, or fronted by something that does not support it. The version gate is only half the story. Observed: PING against a 7.0 server, UNSUBSCRIBE against one reporting 2.8. **The idleness predicate is now a predicate.** Every caller only asks whether the answer is zero, so HasCallerWork() short-circuits on the first caller message instead of counting a queue that a stalled server can leave thousands of entries long - and the interactive bridge is tested first, so the subscription bridge is usually never walked. **The real fix: the retirement drain had the same bug as IsIdle().** It looped on GetOutstandingCount(), so for a departed node - whose probes can never be answered - it always waited out the full 5s timeout before disposing. That is why the retirement test stayed intermittent after the idleness fix: measured `callerWork=False idle=True` with the node still present, i.e. retirement was being *initiated* and then blocked in drain. It now drains on caller work, still bounded by the timeout, and reports the total outstanding when abandoning since that is what is actually dropped. With both links fixed the test is ungated: 7 consecutive two-core whole-suite runs plus 4 more after cleanup, against roughly one failure in two before. Note the earlier "five clean runs" claim for the idleness fix alone was over-stated - it improved the odds without fixing the cause. * Watch a real deployment, and match the fake to what one sends toys/MaintenanceWatch: point it at a connection string and it prints what we made of every maintenance notification next to the raw payload it came from, so a misreading is visible rather than inferred. It forces RESP3 and the opt-in so it behaves the same wherever it is pointed; --enabled switches to the strict mode, which doubles as a probe for whether a server accepted the opt-in at all. Used against a Redis Cloud QA endpoint (Enterprise 8.6.2, OSS cluster API, two nodes) driven through real slot migrations. The whole chain worked with no code changes: the provider recognized the endpoint (defaults=rediscloud), the opt-in was answered +OK, and SMIGRATING/SMIGRATED were parsed - source, target and slot ranges. The captured frames, byte for byte: >3 $10 SMIGRATING :18 $9 8892-8991 >3 $9 SMIGRATED :19 *1[ *3[ $20 <source> $18 <target> $9 <slots> ] ] Two fidelity fixes follow from that. The fake sent the type as a *simple* string where a real server sends bulk - both parse, but there is no reason to differ. And the frames are now pinned as a regression test with the raw bytes in the comment, which is the first test in this feature backed by a capture rather than by a reading of prose. Also documents the deployment prerequisite the fault-injector console made obvious: Enterprise has a *cluster-level* flag deciding whether the subcommand exists, separate from this per-connection opt-in. A supporting version with the flag off refuses the opt-in - which Auto absorbs and Enabled turns into a refused connection, so it is worth checking before suspecting the client. * Understand a captured MOVING, and stop dedup ignoring sequence zero Captured from Enterprise 8.6.2 during a maintenance_mode scenario: >4 $6 MOVING :0 :15 _ Four elements - type as a bulk string, sequence number, a 15-second window, and an explicit RESP3 null for the address, meaning "no replacement given, reconnect the way you connected". The proxy then closed the socket, which is the whole point of MOVING: you are told to move, and then the connection goes away. That confirms two ledger items as written: the element order, and that the no-address case really is an explicit null rather than an empty string or an absent element. Our parser already read it that way. It also exposed a wart. Sequence numbers can legitimately be zero - this one was the first event of its chain - and dedup treated a stored zero as "never seen", so whichever notification opened a chain could never be recognised as a replay. The "have we seen one" state is now a separate bit. Pinned as a regression test alongside the SMIGRATING/SMIGRATED capture, including that MOVING opens a relaxed window, since that window is what covers the reconnect after the socket closes. * Sequence ids are evidence-backed now, not an invention Observed on Enterprise 8.6.2: monotonic per database, zero-based on a fresh one, shared across notification types (SMIGRATING 16 then its SMIGRATED 17), and identical on every node broadcasting a given event - so the number identifies the event rather than the connection that delivered it, which is exactly what dedup needs. The XML docs said the opposite - 'do not assume they are contiguous, or that they are scoped the same way across notification types' - so they are corrected, with the caveat that this is one build of one product and cross-deployment use stays heuristic. Also records why the per-type key is kept despite the counter being shared: within a type the ids are still monotonic, and a per-type key cannot mistake one node's earlier event for a replay of another node's later one. * One event per logical notification, and captures for the DMC family Every node broadcasts a given event with the same sequence number, so a three-proxy deployment delivered one migration three times. Collapse that: ConnectionMultiplexer.TryClaimMaintenanceEvent holds a fixed 8-slot ring of (type, sequence) and raises the public event for the first arrival only. The per-server work still runs for every copy - relaxation is per-ServerEndPoint and each connection has to open its own window - so EndPoint on the event now means "whichever node told us first", documented as such. Matched on equality rather than <=, so a lagging node reporting an earlier event we have not seen is still raised; expired by eviction rather than on a timer, since the copies arrive milliseconds apart. Also captured from Enterprise 8.6.2, closing ledger items 7 and 11: >4 $9 MIGRATING :0 :2 $6 ["27"] >3 $8 MIGRATED :1 $6 ["27"] >4 $12 FAILING_OVER :0 :2 $6 ["21"] >3 $11 FAILED_OVER :1 $6 ["21"] The opening notification carries a time and the closing one has no time element at all - which is what CarriesTime already assumed - and the shard list is a stringified JSON array of id strings. Pinned as tests; the fake can now omit the time, which it previously always sent. That test found a fake-server bug: Dispatch handed the same rented frame to every opted-in client, the first client's write loop recycled it, and the second faulted with "Array element cannot be nil" and lost its connection. So every broadcast had only ever reached one client, which made multi-node fan-out untestable. Built per recipient now, sequence id computed once. * Model the server's catch-up channel in the fake Redis Enterprise retains the most recent shard-scoped completion and replays it to each connection that opts in, coalesced into the same read as the +OK. The boundary is sharp (RS 8.0.22): MIGRATED and FAILED_OVER are retained; MIGRATING, FAILING_OVER, MOVING, SMIGRATING and SMIGRATED are not. What fits all seven is "the completion of a shard-scoped event" - the two carrying an affected-shards list. That gives the design property the handoff work depends on: the catch-up channel can only ever say "a disruption ended", never "one is starting". MOVING, the only notification demanding action, is never replayed - so a reconnecting client cannot be told to hand off by a stale frame, and D6 needs no staleness guard to be safe from replay. Asserted as a negative over all five non-retained kinds. Retention is most-recent-replaces, never a queue, so a connection sees at most one. RetainCompletions turns it off for tests that would rather not reason about which kinds are retained. Needed a deferred-outbound slot on RedisClient: the read loop enqueues a command's reply after Execute returns, so a handler calling AddOutbound directly puts its frame *before* its own +OK, which is the wrong order. The replay also gets us the first test of a push frame arriving mid-handshake, interleaved with our own handshake replies - previously every notification arrived on a settled connection. Not implemented: the catch-up-aware skip of the topology refresh. SMIGRATED turns out not to be retained, so no retained frame can reach the refresh path, and the guard would be unreachable code. * Probe traffic is not caller work, and drop a ValueTuple from the library Health-check probes counted as work a caller is waiting on, so an endpoint being probed looked busy - and idleness is what decides whether an endpoint that has left the deployment can be retired. Invisible while probes only ran under MultiGroupMultiplexer; a blocker for enabling them anywhere retirement also runs. Deliberately not the internal-call bit, which would have been one line: that flag also decides queuing, bypassing the backlog and queuing while disconnected regardless of policy. A health check that bypasses the backlog cannot see a bridge whose queue is not draining, which is the signal geo-redundant failover depends on. So this is a separate bit (19), and the four accounting sites now ask IsCallerFacing rather than !IsInternalCall. The flag has to be on UserSelectableFlags, because probes reach the pipeline through the public API and the constructor masks anything else - so it arrives as caller-supplied flags or not at all. A caller passing it only opts their own command out of idleness accounting. Exposed as HealthCheckContext.ProbeFlags so a third-party probe can be correct too. An injecting IDatabase wrapper would make that automatic, and [AutoDatabase] would make it mechanical, but it would also make the flag invisible and un-opt-out-able, and a probe that forgets it merely reproduces today's behaviour. Also: the dedup ring added yesterday used a tuple, which pulled ValueTuple into the library and broke SanityChecks.ValueTupleNotReferenced - .NET Framework consumers would need the package. Named struct instead. That is already pushed on this branch, and my filtered test runs hid it; the full suite is what caught it. * Maintenance notifications: the MOVING resolve primitive, and a fake that closes the socket (part of D6) (#3202) * The fake's MOVING closes the socket, on the measured timing MOVING's defining half was missing: the socket goes away. Measured on RS (2026-08-28) the close lands at +18.4s and +16.6s against a declared 15s window, so the window is a floor with slack rather than a deadline - the fake defaults to announced-plus-slack, and tests assert that we act within the window, never that the socket survives to the end of it. A shorter delay is available to exercise a less generous proxy. Blast radius is the node, not the connection: four connections to one node differing only in handshake all closed simultaneously, and only the opted-in ones were warned. So the close is scoped to siblings sharing a node, and D6 will reuse RetireAsync rather than anything narrower. A zero delay is deliberately not a case: it races delivery of the notification itself, which no real timing produces. My first version of this test asserted it, and it failed for that reason. * The MOVING re-resolve loop: poll DNS past the address being retired Measured behaviour makes this a poll, not a lookup. Relative to the notification: the endpoint moves server-side at +8.6s, DNS follows at +9.7s and +4.4s across two runs, and the sockets close at +18.4s and +16.6s - against a declared 15s grace and a 5s TTL. So the first answer names the address we were just told to leave, in every run observed, and a client that treats it as authoritative hands off to the node it is trying to escape. The short TTL is what makes polling work: several attempts fit in the window. MovingEndpointProbe is deliberately pure - the caller supplies the resolver, the interval and the budget - because the alternative is untestable: no in-process fake can move a DNS record. Jitter stays at the call site with the existing refresh jitter. Returning null when the window expires is a result, not a failure: the server closes the socket anyway and the relaxed window covers the reconnect, so guessing an address would be worse than doing nothing. Seven tests, including the ones that matter: DNS trailing the notification, a resolution blip mid-handoff, a round-robin record naming both nodes at once, and a zero window still getting one attempt ("act now", not "do nothing"). * Multi-address hostnames are the common case, so stepping sideways is the norm Measured 2026-08-28, all on a 5s TTL: all-nodes 2 A records, all-master-shards 3, single 1 - and an all-master-shards database whose shards shared a node also resolved to 1. So the count follows actual proxy placement rather than the policy name, and `single` (the shape the MOVING timeline was measured on) is the unusual one. The rule survives unchanged, which is the useful part: "take any address that is not the one being retired". With several records the first resolution already names a live sibling proxy, so the handoff steps sideways at once rather than waiting ~9s for DNS - any proxy of the same database serves the same data. The poll only engages when the record names nothing but the retiring address, which is exactly where waiting is the only option. Nothing reads the policy, so placement-driven counts need no special case. Two tests pin the branches, and the log now distinguishes them, because "stepped sideways to a sibling that was already advertised" and "the record has moved to the replacement" look identical otherwise and mean different things when someone is debugging a handoff. Note a full-fleet operation can hand us a sibling that is also about to be retired, so handoffs can chain. Self-limiting - each MOVING carries its own window and relaxation - but worth recognising rather than mistaking for a loop. * Ask whether we are still advertised, and fix a race in my own test The measured gap this closes: on a multi-proxy database, taking a node out on the *shrink* path announces nothing about the endpoint, drops the victim from DNS at +21.4s, and closes its socket silently at +34.7s. So for thirteen seconds the condition is plainly visible to anybody who asks - our address is no longer advertised - and the client's only other signal is a socket dying with no explanation. MIGRATED lands ~5s before DNS moves and is the one notification the server retains, which makes it the prompt to ask on. IsStillAdvertisedAsync returns bool?, and the null carries weight: a resolution failure, or a record momentarily resolving to nothing, is "cannot tell" and must never become "give it up", or one DNS blip recycles every healthy connection at once. MovingEndpointProbe -> AdvertisedAddressProbe: the name stopped describing it once it answered two questions. Both reduce to "what does the record say now, and is my address in it", which is why this is one primitive and not two. Also fixes MaintenanceOptInClientTests.OptInIsReArmedOnReconnect, which asserted *client* state immediately after observing *server* state: the server counts the opt-in when it processes the request, we mark the feature live when we read the reply, a beat later. Intermittent under two cores, mine, and on a branch that was already pushed - so it would have surfaced in CI rather than here. Six consecutive clean runs after polling for the client side. * MOVING fires when the address set gains a member, and DNS may lose the race Nine observations now fit one rule: MOVING is emitted when the endpoint's address set GAINS a member, and is silent when it only loses members. Policy narrowing, maintenance_mode, a 3->2 exclude and a reduction to a single proxy all only shrink, and all were silent; a substitution on that surviving single proxy announced. So the discriminator is neither "single proxy" nor placement. The consequence for the probe is a third outcome, now documented as measured fact rather than as a defensive branch. On one cluster DNS was correct 4.4-9.7s after MOVING, comfortably inside the 15s grace; on another it updated at +18.7s, three seconds AFTER the socket closed at +15.7s. So "window expired with the record still stale" is normal, and the only move left is to reconnect after the close and resolve then - which for a hostname endpoint is already correct. Anybody reading the null return as unreachable would be deleting the handling for a case that happens. Also recorded why the rule stays "any address that isn't mine" rather than "prefer a newly appeared address", despite MOVING marking precisely the moment something joins: a live sibling is at least as good and is available now, while the newcomer is invisible until the record updates. Preferring it means waiting, and waiting is the failure mode. Replacement proxies were measured accepting connections at +6.3s while DNS still advertised only the retiring node - which is a good argument for remembering addresses, deferred because a remembered address whose port was reassigned would be a silent wrong-server connection and a proxied standalone gives us no identity check to catch it. * Maintenance notifications: act on MOVING, and test it against a real deployment (rest of D6, and D9's dedicated testing) (#3203) * A fault-injector test tier: one folder in, databases provisioned per shape New project, tests/StackExchange.Redis.FaultInjector.Tests, net10.0 only - these tests are about server behaviour, not our down-level targets. Picked up by Build.csproj's glob so it compiles in CI, but CI's test step names the main project explicitly, so it never runs there; build.ps1's traversal does run it, which is why the skip behaviour has to be right. One path is the whole configuration: SER_FI_CONFIG_DIR (or the console's FI_CONSOLE_CONFIG_DIR) points at the directory already mounted into the injector as /app/config, so cluster credentials, the CA certificate and the compose file are all found rather than hand-carried into the run. Three states, deliberately distinct: no directory skips; a directory without E2E_SCENARIO_TESTS=true skips (these create and delete real databases); and configured-and-meant-but-broken FAILS. The third is the point - a suite that skips on a broken environment reports success for tests that never ran, and gets trusted at exactly the wrong moment. All three verified. Databases are provisioned by the tests, per shape rather than per test, which removes the conveyance problem entirely: a test that asked for oss_cluster knows what it asked for, so endpoints.json stops being the source of truth for per-database facts and its missing oss_cluster/endpoint_type fields stop mattering. Shapes exist because they change behaviour - A-record count follows proxy placement, and the handoff branches on whether a live sibling exists. Named sertest-<shape>-<runid>. Cleanup is per fixture and unconditional; the startup sweep matches the sertest- prefix and nothing else, so it can never touch a database created by hand. Port collisions retry upward, as go-redis has to. TLS trusts the environment's CA via TrustIssuer. If the CA is missing, TLS tests fail rather than disabling validation: a TLS test that quietly stops checking identity reports success for the one thing it exists to catch. Two traps from the console's known-gaps are encoded rather than left to be rediscovered: poll on pending AND running (a loop waiting only on pending returns while the job is still going), and setup_id lives in the injector's memory so teardown keeps a bdb_id fallback. Teardown also runs on cancellation, with its own budget - the one place the ambient test token must not apply. Unverified and flagged in the README: the create_database parameter names are the injector's prose-documented wire schema, gathered in one place so a real run can correct them. * Prove the fault-injector tier live, and narrow the MOVING rule Run against a real RS 8.0.22 deployment: both template databases connect, negotiate RESP3 and report the opt-in active, and all four topology-change-standalone scenarios run end to end in 7m32s with the notifications observed and parsed. Cleanup verified - four scenarios left the cluster with exactly its two original databases. The rule the measurements produced is a conjunction, narrower than either half: MOVING fires when the connection's own proxy LEAVES the endpoint's address set AND the set GAINS a member. The counter-example is dns_resolution_change, which widens single -> all-master-shards: addresses are plainly added, yet nothing is announced, because the client's proxy is not going anywhere - and then the proxy restarts and the socket closes at +44.5s with no warning. That also resolves what looked like a contradiction, maintenance_mode announcing on a single-proxy database but not on a multi-proxy one: with one proxy, moving it *is* a substitution. The scenario expectations encode this, silence included, so a build that starts announcing the widening case tells us rather than passing quietly. Three more findings. The window overshot again, by 19.1s and 17.5s against a declared 15s, so "floor with slack" has four independent measurements and no counter-example. The sequence counter is shared across all types including MOVING (0, 1, 2 in one chain), which the per-type dedup already assumed. And data_movement_no_conn_drop moved shards with both notifications delivered and the connection never disturbed, so MIGRATING does not imply an impending disconnect. Corrections to the harness from real responses, replacing guesses: - scenario setup provisions its own database and returns setup_id, bdb_id, db_name, endpoints, password, tls, mtls_files and config in ~12s, so scenario tests need neither create_database nor endpoints.json nor the REST API - every trigger publishes the dbconfig it requires, and all four want proxy_policy: single, which no template creates - hence setup provisioning - setup_id is a handle, not an action id: polling /action/{setup_id} 404s - the create_database schema now matches bdb_config.json, which disambiguated oss_cluster_api_preferred_endpoint_type (ip vs hostname, and therefore whether a TLS client can verify its targets) from ..._preferred_ip_type (internal vs external routing) - I had conflated them Traversal run with no environment configured: 7 skipped, everything else green. * Cover the injector's scenario families, and sort what is left into buckets Ran the fault injector's scenarios against the live RS 8.0.22 cluster and added the ones that hold their value as tests. Green live: the OSS cluster family (SMIGRATING/SMIGRATED parsed to source -> target, with 1440 reads and zero failures across a real shard migration), sharded subscriptions recovering unaided after a migration - D5's resubscription, previously fake-only - the failover pair (FAILING_OVER seq=0 time=2s ["52"], FAILED_OVER seq=1) received end to end for the first time, and proxy restart recovery. Four schema facts the injector taught us, each replacing a guess: - create_database wants its config nested under "database_config"; a flat payload is rejected with "got None" - sharding requires shard_key_regex, or Redis Enterprise refuses the database with "Invalid sharding configuration" - /slot-migrate/setup's trigger is how to *provision* (only "reshard"), not how to migrate, and its effect enum is narrower than the discovery endpoint's - remove-add cannot be set up at all - setup provisions a database and returns it, so scenario tests need neither create_database nor endpoints.json Also two harness fixes worth their own mention. The create retry loop retried everything, so "missing shard_key_regex" arrived eight times over half a minute instead of once; it now retries only port collisions. And the traceback summariser split on '\n' when the injector's JSON carries the two characters backslash-n, so every skip message was a wall of Python. Scenarios this cluster cannot produce - add and slot-shuffle need a node with several shards, and three nodes with sparse placement give one each - now skip on a matched message rather than failing. Deliberately not run while unattended: shard/node/proxy/cluster failure, node_remove and reset_cluster, which damage or reset the cluster. The four-bucket assessment is in the notes: what works, what this feature still owes (D6's action half, the connect-failure trigger, moving-endpoint-type, MAINT_NOTIFICATIONS_INFO), what already works outside the feature, and what is untested - of which network_latency matters most, because it is how timeout attribution finally gets live evidence. * Reach the TLS variant, and diagnose why it cannot run here include_tls does not request TLS - it widens the list of variants setup may choose from, and variant_index picks one. With no flags a trigger offers one variant (single), with include_tls two (single, single_tls), with include_mtls a third (mtls). Passing include_tls alone provisions variant 0 and yields a plaintext database, which is how the first attempt skipped itself. With variant_index=1 the database came up TLS-enabled and the connect was refused: the remote certificate was rejected by the validation callback. That is the environment, not us - the folder's server certificate covers *.marcgravell-test-46be1d08... while the live cluster is marcgravell-test-e21cd75d..., left over from an earlier provision and three days older than the env_output.json beside it. Our behaviour was right: TrustIssuer tolerates chain errors only, so a name mismatch fails outright, which is the whole point of it. So the test now compares the certificate's DNS names against the cluster name *before* provisioning anything, and skips naming both. Without that, a stale certificate reads as a client bug and costs somebody an hour of certificate archaeology; the check costs nothing and happens before a database exists. Also set AbortOnConnectFail=true in the TLS test only. Everywhere else tolerating a slow start is right, but with it false a certificate problem is indistinguishable from a slow cluster: ConnectAsync succeeds, IsConnected is false, and the reason is gone. That is exactly how the first failure presented. Traversal with no environment: 15 skipped, everything else green. * D6: act on MOVING instead of waiting to be disconnected Today a MOVING is survivable - the socket closes and we reconnect - but the announced window goes entirely unused, and the reconnect re-resolves to whatever DNS says at the moment the server chose, which has been measured as still naming the node being retired. This uses the window: wait for DNS to move, then pick the moment ourselves. The dispatch turns on the form of the endpoint, which also corrects the earlier assumption that MOVING should reuse endpoint retirement: - hostname, no successor (every observed MOVING): the ServerEndPoint stays, only the address behind the name moves, so retiring it would delete our only route to the deployment. Probe until the record moves, then recycle the connections so they re-resolve. - address with a named successor: genuinely a different endpoint, so re-read the topology. Never observed - eleven routes, all explicit nulls - so this exists because the contract has it. - address, no successor: nothing to re-resolve and nowhere named to go. Doing nothing is correct. Deciding is separated from acting so the decision can be tested exhaustively without a server: DecideAsync takes the endpoint, the current address, the window and a resolver. That seam exists because the whole thing turns on DNS *changing*, which no in-process fake can arrange - ConnectionMultiplexer.AddressResolver defaults to real DNS. Recycling is a dispose: that already routes through RecordConnectionFailed to OnDisconnected, which reconnects immediately, so there is no new lifecycle to get wrong. Both bridges, because the measured blast radius is the node. Drained first, bounded by what is left of the window - the socket dies at the end regardless, so anything undrained was going to fail either way and draining strictly dominates. Jitter is a fraction of the window rather than a flat delay, capped at a second. A 2s window - which the shard notifications really do announce - must not spend half of itself waiting, and a 15s window does not justify a long wait when DNS has been seen moving after four seconds. Also safe from replay by construction, which is why there is no staleness guard: the server retains only shard-scoped completions, so a MOVING is never delivered as catch-up. Nine tests: five decision branches, jitter bounds, and an end-to-end recycle against the fake with MovingClosesConnection deliberately off, so the only thing that can replace the connection is our own handoff. Three consecutive two-core Release runs: 6215 passed, 0 failed. * D6 proven live, and the feedback loop it exposed On the real cluster the handoff does what it was built for: conn_drop/endpoint_rebind MOVING +9.3s -> recycled and reconnected +9.5s server would have closed at +25.5s maintenance_mode MOVING +21.7s -> recycled and reconnected +21.9s server closed at +38.0s So we move roughly sixteen seconds before being pushed, on both routes. The first live run also found a bug that no fake could have produced: a server re-sends MOVING to a connection that opts in while the window is still open. Since the handoff replaces the connection, acting on the repeat loops - recycle, reconnect, get told again, recycle - and it produced twelve recycles from a single event. OnMaintenanceWindowOpened already claimed the sequence id and knew it was a repeat; the handoff was not asking. It now returns whether the notification was new and the handoff gates on it, and the live test asserts *exactly one* recycle. Second finding, recorded rather than fixed: our own recycle does not raise ConnectionFailed, because disposal is not reported as a failure. From outside the library a handoff is therefore invisible - an operator sees a reconnect with no reason given. HandoffRecycles and LastHandoffOutcome exist because Multiplexer.Trace is [Conditional("VERBOSE")] and compiles away, so there would otherwise be no record at all of what a handoff decided. Whether it should surface something publicly is a real question, not settled here. Two consecutive two-core Release runs: 6215 passed, 0 failed. * Report a handoff as MaintenanceHandoff rather than silently A handoff was invisible from outside the library: the replacement connection raises ConnectionRestored, but our own recycle raised nothing, so a consumer tracking connection state saw a restore with no matching failure and no reason for the churn. The reporting block is gated on "if (_ioStream is not null || isInitialConnect)" - if *we* didn't burn the pipe, flag it - and Dispose runs Shutdown first, which is precisely why an ordinary dispose is silent. So the fix is ordering: record the failure while the pipe is still live, then dispose. ConnectionFailureType.MaintenanceHandoff is the right home. The existing event args already carry endpoint, connection type and a discriminator, and CircuitBreaker is the precedent for a deliberate client action reported this way. Documented for what it is: consumers alerting on ConnectionFailed should filter it out, since it means planned maintenance rather than a fault - and the test asserts we report *only* that, never SocketFailure or SocketClosed, so planned maintenance cannot end up in fault dashboards. Four consecutive two-core Release runs at 6215 passed. Note one earlier run reported two failures whose names I did not capture and which have not recurred in four runs since; if they come back I will capture them properly rather than guess. * Fix the cluster-flag call, which had been failing silently update_cluster_config wants its flags nested under "config" - the same shape create_database wants for "database_config" - and a flat payload is rejected with "Invalid parameter 'config': got None". Because the call is best-effort and only wrote to Console, it failed silently for a full day of testing without anybody noticing. Note the impact was small: the environment templates enable these flags at provision time, so this call is a safety net rather than the mechanism, and every test was passing on its own merits. It matters for an environment provisioned without them, where the alternative is every test failing at connect and blaming the client for a server-side setting. Verified corrected against the live injector. Fixture diagnostics now go to a collected SetupLog as well as the console, since a fixture has no test output helper and console writes are exactly what got lost. * Reach the migrations and the TLS variant that were being skipped Three "environment limitations" turn out to have been mine. add and slot-shuffle were skipping with "No node with multiple shards found", and remove-add was unreachable because /slot-migrate/setup's effect enum excludes it. The cluster was never the problem: the setup leg provisions one shard per node, so there is nothing to move a shard *from*. Provisioning our own database - six shards, dense placement, two per node over three nodes - and driving the run leg by bdb_id makes all three run, and all three now pass live. The generalisation is the useful part: a scenario setup cannot arrange is still reachable by provisioning the database ourselves. remove-add is the best of them: it moves every shard as five SMIGRATING/ SMIGRATED pairs sharing one sequence chain (0-9), which exercises the dedup and the event collapse far harder than a single migration. The first dense attempt failed for an unrelated reason: the client tried to reach 10.0.101.15, a VPC-private address, because a cluster database created without oss_cluster_api_preferred_ip_type defaults to *internal* - so CLUSTER SLOTS advertised addresses unreachable from outside the VPC. That is the field separated earlier from endpoint_type, biting for real: ip_type is internal-versus-external routing, endpoint_type is ip-versus-hostname identity. Shapes now default to external. TLS also runs now that the environment's certificates match the cluster: connected with validation on, opt-in active, MOVING received over TLS. My own mismatch detector had a bug worth recording - it read one name via GetNameInfo and compared it against the cluster name, so a wildcard "*.<cluster>" never matched, and it skipped a perfectly good environment. These certificates carry both a wildcard and the bare name, the hosts dialled are database endpoints *under* the cluster domain, and a wildcard correctly does not match its own parent. It now reads the full SAN list. Cluster left with exactly its two original databases. Local suite: 6215 passed.
mgravell
added a commit
that referenced
this pull request
Sep 1, 2026
* Opt in to maintenance notifications during handshake
Adds the client half of the maintenance-notification ("smart client handoffs")
opt-in: a tri-state ConfigurationOptions.MaintenanceNotifications, and the
CLIENT MAINT_NOTIFICATIONS ON that carries it, pipelined next to CLIENT ID.
The mode names are the prescribed cross-client ones, so a connection string
ports between clients - which makes Enabled mean *required* rather than merely
"on". That is easy to misread, so the warning leads the XML docs on both the
enum member and the property, where it shows in the completion list rather than
only on hover.
Enabled fails uniformly: a server that refuses, a server that answers HELLO 3 as
RESP2, and a configuration that could never ask (Protocol = Resp2, or no HELLO).
The last of those diverges from the letter of the spec, which mentions only the
error reply - but requiring a RESP3-only feature over RESP2 is a contradiction,
and half-honouring it silently is what the mode exists to prevent. Auto is the
best-effort mode and never rejects a connection.
Since the handshake is pipelined we don't know the negotiated protocol at write
time, so the request is speculative (as the redundant AUTH already is) and
ReconcileMaintenanceNotifications settles it afterwards, with every fact in
hand. The opt-in processor absorbs a refusal rather than routing it through the
common error path, which would raise ErrorMessage to the consumer for something
we asked for on their behalf.
Defaults stay Disabled globally: most servers have never heard of the subcommand,
and the server is not the only thing in the path - an unrecognized CLIENT
subcommand is not guaranteed to be answered as politely by a proxy as by a
server. AzureManagedRedisOptionsProvider is Auto pre-emptively, which is safe
precisely because Auto tolerates a refusal; pending validation against a real
AMR endpoint.
Also: REDIS_TESTS_MAINT_NOTIFICATIONS lets the whole suite run with the opt-in
on, mirroring REDIS_TESTS_MIN_TIMEOUT_MS. Verified as a no-op at Auto against
real servers that refuse it, which is the point. The toy server now matches
keywords case-insensitively (we send ON, go-redis sends on, a real server takes
both), and InProcessTestServer.MaxProtocolVersion can answer HELLO 3 as RESP2.
* Receive maintenance notifications, and report them
The other half: seven new PushKind members for the notification families, and a
parser that turns them into a PushMaintenanceEvent on the existing
ConnectionMultiplexer.ServerMaintenanceEvent. Observation only - nothing reacts
to these yet, deliberately, so a consumer can watch what its servers announce
before any behaviour depends on it.
These are dispatched in OnOutOfBand *before* anything reads element 1 as a
channel name, because element 1 is a sequence number rather than a channel:
that is the whole reason they could not be handled as pub/sub. A frame we
cannot read is consumed and forgotten rather than falling through to the
command matcher, where it would take a reply belonging to something else -
tested by following a malformed push with command round-trips that prove the
connection is still in sync, not merely alive.
Two decisions worth naming:
- The type decides whether a time element is expected, not the content. A
single-slot SMIGRATING payload of "123" is indistinguishable from a duration
by inspection, so content-sniffing would silently lose the slot list. A
notification that omits its time, or adds one where the contract says there
is none, is still accepted.
- MOVING's placeholder forms - explicit null, "?", and a zero port - all yield
NewEndPoint = null with the raw text kept, never the answering server. Same
reasoning as the unroutable-redirect work: an address that cannot be dialled
must not be replaced by a guess.
The PushKind lookup is now case-insensitive throughout rather than only for the
new members: the pub/sub kinds are lowercase on the wire and these are
uppercase, and one lookup that tolerates both beats two lookups.
The shard-id and slot payloads are carried through as opaque strings. Nothing a
client is asked to *do* depends on which shards are involved, and parsing a
field the contract does not pin down would be inventing a model.
* Cloud and on-premise defaults, and a name for them
Three related pieces of configuration work, all in service of "when should SCH be
on?".
**MaintenanceNotifications is no longer nullable.** It followed Protocol, where
null means something real ("no preference, let the library decide"). Here there is
no third state - Disabled *is* off - so it now follows the convention almost every
other option uses: non-nullable, falling back to the provider. That also removes
three `?? Disabled` coalesces.
**RedisCloudOptionsProvider**, matching the Cloud domains, with Auto. It is
deliberately *not* a copy of the AMR provider, though the deployments look
similar:
- GetDefaultSsl stays false. AMR is TLS-only so assuming TLS there is safe; Redis
Cloud enables TLS per database and plenty are plaintext, where guessing would
fail their connect outright.
- DefaultVersion stays at the library default. 7.4 is AMR's *floor*; Cloud still
offers older versions per database, and claiming a version we do not have
unlocks commands the server will reject.
It does share what is about being a proxied, hosted deployment: RESP3 (which the
feature requires), no configuration-broadcast channel, and fail-soft connect.
**Providers can be named in a configuration string**, as `defaults=amr`,
`defaults=rediscloud`, `defaults=azure` or `defaults=enterprise`. The on-premise
case is why: an Enterprise cluster has whatever DNS its operator gave it, so
IsMatch can never recognize it, and until now selecting a provider meant writing
code - impossible for an application configured by a connection string. It also
covers a hosted deployment reached behind private DNS or a proxy, where the
endpoint stops looking like what it is. Hence RedisEnterpriseOptionsProvider,
which matches nothing and exists to be asked for.
Resolution is by name against registered providers only, never by type name: a
configuration string that could name an arbitrary type would be a way to have one
loaded, and would defeat trimming.
Round-tripping needed care. The Defaults getter memoizes an inferred provider into
the same field an explicit set writes, so merely *reading* the property would
otherwise make an endpoint-derived guess indistinguishable from a decision - and
re-parsing the string would then pin it. So a flag records that the caller chose
it, and `defaults=` is written only when it was chosen *and* the provider has a
name; unnameable custom providers behave like custom tunnels and simply do not
serialize. Clone copies the field rather than the property, which is what keeps
that distinction intact.
Note one deliberate behaviour change: ToString() on options with an explicitly-set
inbuilt provider now includes `defaults=<name>`, where before that choice was
silently dropped. DefaultsProviderProtocolNotSerialized is updated to assert both
halves - that provider *values* still never leak, and that the provider *choice*
now round-trips.
* Provider ToString reads as its name
For logs: a provider should print as "amr" rather than as a namespace-qualified
type name, and an unnameable one falls back to the type as before.
Note serialization deliberately does not route through this - it tests Name
directly, because ToString never returns null and an unnameable provider must not
end up in a configuration string. Display name and round-trippable identifier are
not the same thing here, even though Tunnel conflates them behind IsInbuilt.
* Maintenance notifications: relax timeouts, and read the cluster delta (D4, part of D5) (#3194)
* Relax command timeouts while a server announces a disruption
Stage 2 of maintenance notifications, and the first part that changes behaviour:
an opening notification (MOVING, MIGRATING, FAILING_OVER, SMIGRATING) raises
command timeouts for that server, and a closing one (MIGRATED, FAILED_OVER,
SMIGRATED) stands them back down.
Three settings, of which only the first is prescribed cross-client - the other
two are ours, and say so in their XML docs:
- MaintenanceRelaxedTimeout (10s): what timeouts are relaxed *to*, as a floor.
The effective timeout is max(configured, this), so a caller with a generous
timeout keeps it.
- MaintenanceRelaxedWindowMax (3x): a backstop for a closing notification that
never arrives. A window that never closes is worse than one that closes early.
- MaintenancePostEventRelaxedDuration (2x, matching go-redis): a closing
notification means the server-side operation finished, not that the server is
back to normal latency - and completion is exactly when every other client
that received the same notification re-engages. It does not apply after a cap
expiry, where nothing told us the event finished and extending past the
backstop would defeat it.
The cap and tail derive from the *effective* relaxed timeout rather than the
provider's, so raising the relaxed timeout cannot leave a cap below it; a
provider can still pin either absolutely. Caught by its own test, which is why
the test asserts the relationship and not just the numbers.
Relaxation is server-scoped state read at sweep time, not stamped per message:
both timeout sweeps rely on head-of-line ordering and stop at the first message
that has not timed out, and per-message timeouts would make that short-circuit
invalid - turning every heartbeat into a full scan of everything outstanding.
The consequence is that relaxation covers whatever is already in flight, and
stops covering it when the window closes, which is a further reason the tail
earns its place.
Wired into all three places a command timeout is enforced. Note the backlog
sweep previously measured message age against _singleWriter.TimeoutMilliseconds
- the write-lock acquisition timeout - which is about contention between writers
rather than server latency; it now has its own expression, so relaxation cannot
leak into lock acquisition. The sync path grows a re-wait loop, because a
Monitor.Wait commits to a duration when it parks: without it, a sync caller in
flight when a notification arrives would time out at the strict timeout while
its async neighbour was relaxed.
Also seqID dedup, per notification type, which lands here because a replayed
opening notification extending a window is the first place a replay does damage.
The sequence numbers are not defined by any specification, so this is
deliberately conservative: an id we have already acted on is ignored, and
nothing else is inferred.
What is *not* relaxed, as a rule rather than a judgement call: keep-alive, the
heartbeat, and connection-failure detection. Otherwise a server that died
mid-maintenance would linger for the whole window, turning a latency mitigation
into an availability regression. There is a test that kills a connection inside
an absurdly generous window and requires the failure to still be noticed.
* Report maintenance context on the faults it causes
Closes out stage 2 with the fault surface: MaintenanceType on
RedisTimeoutException, RedisConnectionException and FaultContext, defaulting to
None. "Timeout" and "timeout during an announced failover" call for very
different reactions from whoever reads the log, and until now the two were
indistinguishable.
Follows the established pattern on those types - Commandstatus and Flags on the
timeout, FailureType on the connection fault - rather than introducing an
exception type nobody catches yet, and named for the role rather than the type,
as FailureType is. Both live in a partial alongside the rest of the feature, so
Exceptions.cs is untouched.
On FaultContext it is deliberately reported and not acted on: a fault during
announced maintenance is expected and transient, and counting it towards a
circuit-breaker trip that then withdraws a whole server is the opposite of what
the notification was for - but "ignore faults during maintenance" is a judgement
about a deployment, not about the protocol, so it belongs in policy.
Also routes the effective timeout through the dead-socket heuristic in
PhysicalBridge, which is derived from the command timeout and would otherwise
contradict relaxation: with a relaxed timeout of 60s and that check firing at
4x the strict 5s, we would tear down precisely the connection relaxation was
protecting. This is a refinement of the boundary rule, not an exception to it -
socket-level failure detection is untouched, which is what actually guarantees a
dead server is still noticed, and there is a test that kills a connection inside
an absurdly generous window to prove it.
* Read the nested cluster slot-migration payload
Cross-checking our reading against the shipped clients (go-redis, redis-py) said
SMIGRATED is nested, not flat:
["SMIGRATED", <seq>, [[source, target, slots], ...]]
with slots a flat comma-and-range string inside each triplet. Two independent
implementations agree on the nesting, which is much better evidence than our own
reading of the prose - and it means we were *dropping* SMIGRATED, because the
parser rejected any non-scalar element after the type. Safe, but wrong.
The scalar-only guard stays for the DMC family, where nesting would signal a
frame we do not understand; it is relaxed only for the two cluster kinds. A
malformed triplet is skipped rather than losing the whole notification - the
other triplets are still actionable, and it is what go-redis does.
Exposed as ClusterSlotMigration on the event (source, target, parsed slot ranges,
and the raw slot text so a list we could not parse still tells you something).
Nothing acts on them yet.
Two other things the cross-check turned up:
- We required a readable sequence id and dropped the frame without one. go-redis
length-checks these frames at two elements and reads no sequence number at all
for the shard notifications, so that was stricter than a client which
demonstrably works against real servers. Now a missing id costs only dedup -
which is our own invention - rather than the notification.
- SlotRange.TryParseInt16 was `checked`, so an out-of-range slot number threw
OverflowException from a Try* method. Pre-existing and reachable today from the
public SlotRange.TryParse and from CLUSTER NODES parsing; now reachable from
the read loop too, where throwing is far worse than rejecting. It rejects.
Fixing the parser needed two other things worth recording. RespReader's
AggregateChildren() does not advance the parent reader, so MovePast(out reader)
is required or the loop walks back into the children it just read and mistakes
them for top-level elements. And in the toy server, Recycle() recurses, so a
Standalone child inside a pooled parent is handed to a pool it never came from -
Rent at every level. That one was invisible because the fake's write loop
swallowed its exception into the pipe, with a reconnect covering the tracks; it
now logs, which is the only reason the second bug took minutes rather than
longer.
* Document the maintenance options and named defaults providers
Configuration.md gains the five new keys in the table, plus two sections: what a
defaults provider is and why 'enterprise' exists (it cannot be detected - a
self-managed cluster has whatever DNS its operator was given), and what the
maintenance options do.
Two things stated explicitly because they are the surprising parts: 'enabled'
means *required* and will refuse a connection that cannot deliver notifications,
and the maintenance durations are in seconds where every other timeout in that
file is milliseconds.
* Refresh topology when slots migrate away from us
Reacting to SMIGRATED rather than waiting to be told by a -MOVED. This reuses the
path AzureMaintenanceEvent has used for years - raise the event, then refresh -
and is the whole of go-redis's SMIGRATED handling, so the risk profile is good:
the worst case is the topology pass we already do.
Three deliberate differences from that Azure precedent:
- Scoped to triplets whose *source* resolves to us. Every node in the cluster
reports the same movements, so most notifications describe somebody else, and
refreshing on those means every client in the fleet re-reads topology whenever
any shard moves anywhere. Resolution goes through the identity map, since a node
answers to both its address and its announced hostname and the delta may name
either.
- Jittered by up to a second, because the fleet was all told the same thing at the
same instant. Not configurable: the relaxed-window durations are options because
their right value is deployment-specific and we invented them, whereas this is a
fixed smear nobody needs to tune.
- Cluster family only. MIGRATED and FAILED_OVER arrive in proxied deployments
addressed as a single endpoint, where a refresh has no topology to learn, so they
keep relaxation and nothing else.
The jitter turned out to *defeat* the coalescing I was relying on:
ReconfigureIfNeeded declines only while a refresh is in flight, and spreading a
burst out means each pass completes before the next begins - so ten notifications
became ten topology passes. Caught by the test that counts inbound CLUSTER
commands. Coalescing therefore happens before the delay, via a pending flag,
released when the refresh starts rather than when it finishes: anything arriving
after that describes a state this pass may not have seen.
Endpoints left serving no slots need no new code - the absence-based pruning from
#3177 already retires them, and a refresh feeds exactly that path. It takes three
generations rather than being immediate, which is slower than the HLD implies but
is the existing tested policy, and is more than go-redis does at all.
* Re-establish sharded subscriptions when their slots move
Sharded subscriptions are slot-bound, so a slot leaving this node takes them with
it. Mostly belt-and-braces: a server that migrates a slot also sends an
unsolicited SUNSUBSCRIBE, and OnOutOfBand already resubscribes on that. This adds
two things - it is pre-emptive where SMIGRATED arrives first, and it covers the
case where the unsolicited unsubscribe never arrives or is lost, where the only
other symptom is messages silently stopping, which nothing detects.
It also knows *which* slots moved, so only the affected channels are touched
rather than everything subscribed here. Ordinary pub/sub is not slot-bound and is
deliberately left alone, even when the notification says every slot moved.
Done before the refresh and without waiting for the jitter: a subscriber that is
silently no longer subscribed is a correctness problem, where a stale slot map is
an extra round trip.
It resubscribes via *this* server rather than the migration target, reusing
ResubscribeToServer unchanged - the outgoing node is the one we know has the new
route, and sending there follows the redirect. The target is named in the
notification and could be dialled directly, but it may be a node we have never
seen or named in a form we cannot dial, and the redirect path is the one already
proven by the SUNSUBSCRIBE case. Worth revisiting only with evidence.
* Make the fake announce its own migrations, and fix what that exposed
Migrate() only moved the slot in the fake's model - it emitted nothing, so the
realistic sequence a client sees could not be reproduced. It now optionally
announces itself (NotifyOnMigrate, off by default so existing tests that use
Migrate to arrange a topology are undisturbed): SMIGRATING, an unsolicited
sunsubscribe to subscribers of affected sharded channels, then SMIGRATED. Note
that also gives the pre-existing unsolicited-SUNSUBSCRIBE path its first coverage
from the fake; until now it could only be reached with a hand-built push frame.
Turning it on immediately contradicted the resubscribe logic committed alongside
it, in two stages:
- Both signals fire for one migration, and both route to ResubscribeToServer,
whose guard admits a subscription that is transiently attached to nothing. That
measured six (re)subscribes for one channel.
- Making it a delayed fallback - after the jitter, and only when the subscription
is not attached elsewhere - fixes that. "Attached elsewhere" is the only
reliable signal that the other path dealt with it; still attached to *us* is the
pre-emptive case (notification beat the unsubscribe, subscription now stale) and
attached to nothing is the stranded case, and both need acting on. An earlier
guard on IsConnectedAny() got this wrong and silently disabled the pre-emptive
case, which is the primary one.
So it now costs a stranded subscription up to the jitter in recovery time, and
costs nothing when it was not needed. Measured 4 (re)subscribes with notifications
off and 5 with them on, the extra being the fallback acting on a subscription the
unsubscribe path left attached to nothing - the feature working, not duplicate
work.
One thing this turned up and does not fix: after a real migration in the fake, a
message published to the moved channel is not delivered - with notifications
*disabled* as well, so it is either a pre-existing gap or a limitation of the
freshly-added node. Deliberately not asserted, since attributing it here would
blame this code for something it does not cause. Worth its own investigation.
* Stop chasing a delivery failure that was never about migration
The earlier note claimed a message published to a moved sharded channel is not
delivered. Ran the control that should have come first: sharded pub/sub does not
deliver in the fake *at all*, with no migration anywhere - SPUBLISH reports zero
receivers.
Evidence, for whoever picks this up: the client subscribes correctly, including
following the redirect (SSUBSCRIBE on the old owner returns
`-MOVED 15296 127.0.0.1:6380`, and SSUBSCRIBE then arrives on 6380), and SPUBLISH
also arrives on 6380 - subscriber and publisher agree on the node. The node still
answers `:0`. So the fake registers a sharded subscription somewhere its own
publish lookup does not find it; a RedisChannel equality/options mismatch between
the stored key and the lookup is the obvious first suspect.
That makes it a gap in the fake rather than anything to do with maintenance
notifications, and it means no test can currently assert sharded delivery against
the toy server. The assertion and the per-node diagnostics are removed; the test
keeps the property it can honestly own, which is that the resubscribe is bounded.
* Toy server: sharded publish delivered to nobody, ever
SPublish computed the node to filter by, with a comment saying so, and then did
not pass it:
var node = client.Node; // filter to clients on the same node
...
PublishPair pair = new(channel, request.GetValue(2)); // node dropped
ForAllClients(pair, static (client, pair) =>
ReferenceEquals(client.Node, pair.Node) ? client.Publish(...) : 0);
PublishPair's node parameter is optional, so pair.Node was always null,
ReferenceEquals never matched, and SPUBLISH answered :0 in every case - no
migration required. Sharded pub/sub delivery has therefore never been covered by
the fake at all, which also means a client-side sharded pub/sub bug could not have
been caught here.
Found while investigating an apparent "sharded subscription does not deliver after
its slot migrates". It was not about migration, and it was not the client: the
client followed the -MOVED correctly (SSUBSCRIBE arrived on the new owner) and
routed SPUBLISH to the correct owner. The control with no migration at all is what
settled it, and should have been the first experiment rather than the last.
With that fixed, the end-to-end property can be asserted, so
RealMigrationRecoversTheSubscriptionWithoutStorming now checks it: after a slot
migration the sharded subscription delivers again. Published repeatedly, because
pub/sub is fire and forget - a message published while the subscription is in flux
is dropped, so losing messages during the tremor is expected while never
delivering again is not, and one publish cannot distinguish them.
* Retire nodes that leave the cluster, narrowed from "serving no slots"
D5 asked for endpoints left serving no slots to be shut down. Narrowed
deliberately: a node still listed in CLUSTER NODES is a live cluster member that
may be given slots again, so dropping its connection is churn - and go-redis does
not do it. Having *left* the cluster is the condition worth acting on, and the
existing absence-based pruning already covers it; the notification-driven refresh
is what makes us notice promptly.
No client change was needed to demonstrate that, only the ability to express the
scenario: RedisServer.RemoveNode removes a node the way CLUSTER FORGET does - gone
from SLOTS and NODES, and every alias it answered to stops resolving. It refuses
while the node still owns slots, as a real cluster does, since a topology with
unowned slots says nothing useful about client behaviour.
Two things this exposed, both about the policy rather than this feature:
- Retirement can be starved. Pruning requires IsIdle(), which counts outstanding
work, and we keep heart-beating the very node we are trying to retire - so a
ping in flight makes it look busy. On a two-core runner that repeats often
enough to prevent retirement indefinitely. The design notes recorded exactly
this trap for the usage-based grace rule and dropped it for that reason;
IsIdle() has the same problem. Excluding our own keep-alive traffic from the
idleness test would fix it, and is a product change rather than something to
paper over in a test - so the test is gated on a quiet machine and says why.
- EndpointPruningUnitTests exercises the policy by feeding a SLOTS-only topology
directly, which is not how a real refresh drives it. This test goes through
ReconfigureAsync instead, which is why it sees the starvation at all.
Also: this class is now non-parallel. Several of these wait out a jittered
refresh, and the retirement one needs a quiet server, so sharing a machine with
the rest of the suite measured as noise rather than signal.
* Gate the retirement test, and record what the heartbeat theory did not explain
The narrowed retirement is demonstrable on a quiet machine and fails about half
the time on a two-core runner - because it does not happen, not because the test
is impatient. Pruning requires IsIdle(), so something keeps the departed node
looking busy.
The obvious suspect was our own keep-alive: we heartbeat the very node we are
trying to let go of, and an outstanding ping is enough to fail the idleness test.
That theory was implemented (suppress keep-alive and the replication check for a
ClusterTopology-provenance server absent from the topology) and it did *not* fix
the flakiness, so the theory is wrong or incomplete and the change is reverted
rather than shipped on a rationale the evidence contradicts.
So the test is gated on a quiet machine with that stated, and the cause wants a
focused look with instrumentation inside the pruning loop - not from a test, where
an earlier attempt produced provenance readings that could not be trusted.
* Record what makes a departed node look active: our own probes
Measured from the snapshot the pruning loop walks, on a failing run: every term
that should be true is - provenance=ClusterTopology, absentSince stable at 2,
ownsSlot=False - and the blocker is outstanding work, growing ~170 per topology
pass (176, 348, 520, ... 1339).
That is not a keep-alive ping, which was the first theory and is why suppressing
heartbeats did not help. It is the reconfigure's own autoconfigure probes to that
node: it is gone, so nothing answers, and they accumulate in its backlog. IsIdle()
counts backlog, so the node can never look idle - the more we look for it, the
busier it appears. It only retires by winning a race on an early pass, which is
exactly the load sensitivity observed.
So the precondition is self-defeating in the case pruning exists for. Recorded in
the test comment and the design notes; the fix is a product decision, with three
candidates: exclude internal calls from the idleness measure, treat a
disconnected bridge as idle since its outstanding work is doomed anyway, or stop
probing servers awaiting retirement.
* Idleness should count caller work, not ours
Retirement requires IsIdle(), which counted *all* outstanding work. A node the
topology has stopped listing still receives autoconfigure probes on every pass,
and nothing answers them because it is gone, so they accumulate in its backlog:
measured at ~170 per pass, growing without bound (176, 348, 520, ... 1339). The
node therefore looked busy *because* we were looking for it, and could never be
retired - the precondition defeated itself in precisely the case pruning exists
for.
The hidden internal-call flag already distinguishes our traffic from a caller's,
so this is just a matter of asking the right question: IsIdle() now uses
GetCallerOutstandingCount(), which walks the written-awaiting-response queue and
the backlog and ignores anything flagged internal. That covers autoconfigure,
handshake, and keep-alive traffic in one test, since all of it is flagged - which
also disposes of the earlier keep-alive-specific theory properly rather than by
suppressing heartbeats.
Except that the *subscription* keep-alive was not flagged, unlike the interactive
one which sets it via GetTracerMessage - so its ping, and its unsubscribe of a
channel named after our own unique id, both looked like caller work. Now flagged,
for consistency and because they plainly are ours.
GetOutstandingCount() is unchanged and still counts everything: the retirement
drain uses it, and waiting for our own in-flight probes to settle before tearing a
connection down is the right behaviour there.
Verified with the retirement test ungated: five clean two-core whole-suite runs,
against roughly half failing before.
Left alone deliberately: the availability health-check probes do not set the flag,
so they still count as caller work. Arguably they should not, but IsInternalCall
affects more than idleness, so that wants its own change rather than riding along
here.
* Clarify which subscription keep-alive actually fires
The comment listed both branches without saying which one runs, and I had
described them in review as though both were live traffic. Observed: against a 7.0
server the keep-alive is PING (answered with the two-element array pong), and the
UNSUBSCRIBE fallback only fires against a server reporting older than 3.0, since
PingOnSubscriber gates there and the default assumed version is 6.0.
Also notes where the array-shaped pong is handled - IsArrayPong in OnResponseFrame
- since that is not obvious from this end and is what stops the reply being taken
for a pub/sub payload.
* Drain on caller work too, and correct the record on the keep-alive
Three corrections and one real fix, all from review.
**The subscription keep-alive was already flagged.** KeepAlive has a common
`if (msg != null) { msg.SetInternalCall(); ... }` after the switch, so both
branches were already internal calls and the two per-branch calls added in
17151179 were redundant - removed. The commit message there claimed the
subscription keep-alive "was not flagged", which is simply wrong; it was.
**And the UNSUBSCRIBE branch is not "legacy".** Its condition is
`IsAvailable(PING) && PingOnSubscriber`, so it is also reached when PING is
disabled or renamed in the CommandMap, or fronted by something that does not
support it. The version gate is only half the story. Observed: PING against a 7.0
server, UNSUBSCRIBE against one reporting 2.8.
**The idleness predicate is now a predicate.** Every caller only asks whether the
answer is zero, so HasCallerWork() short-circuits on the first caller message
instead of counting a queue that a stalled server can leave thousands of entries
long - and the interactive bridge is tested first, so the subscription bridge is
usually never walked.
**The real fix: the retirement drain had the same bug as IsIdle().** It looped on
GetOutstandingCount(), so for a departed node - whose probes can never be answered
- it always waited out the full 5s timeout before disposing. That is why the
retirement test stayed intermittent after the idleness fix: measured
`callerWork=False idle=True` with the node still present, i.e. retirement was being
*initiated* and then blocked in drain. It now drains on caller work, still bounded
by the timeout, and reports the total outstanding when abandoning since that is
what is actually dropped.
With both links fixed the test is ungated: 7 consecutive two-core whole-suite runs
plus 4 more after cleanup, against roughly one failure in two before. Note the
earlier "five clean runs" claim for the idleness fix alone was over-stated - it
improved the odds without fixing the cause.
* Watch a real deployment, and match the fake to what one sends
toys/MaintenanceWatch: point it at a connection string and it prints what we made
of every maintenance notification next to the raw payload it came from, so a
misreading is visible rather than inferred. It forces RESP3 and the opt-in so it
behaves the same wherever it is pointed; --enabled switches to the strict mode,
which doubles as a probe for whether a server accepted the opt-in at all.
Used against a Redis Cloud QA endpoint (Enterprise 8.6.2, OSS cluster API, two
nodes) driven through real slot migrations. The whole chain worked with no code
changes: the provider recognized the endpoint (defaults=rediscloud), the opt-in
was answered +OK, and SMIGRATING/SMIGRATED were parsed - source, target and slot
ranges.
The captured frames, byte for byte:
>3 $10 SMIGRATING :18 $9 8892-8991
>3 $9 SMIGRATED :19 *1[ *3[ $20 <source> $18 <target> $9 <slots> ] ]
Two fidelity fixes follow from that. The fake sent the type as a *simple* string
where a real server sends bulk - both parse, but there is no reason to differ. And
the frames are now pinned as a regression test with the raw bytes in the comment,
which is the first test in this feature backed by a capture rather than by a
reading of prose.
Also documents the deployment prerequisite the fault-injector console made
obvious: Enterprise has a *cluster-level* flag deciding whether the subcommand
exists, separate from this per-connection opt-in. A supporting version with the
flag off refuses the opt-in - which Auto absorbs and Enabled turns into a refused
connection, so it is worth checking before suspecting the client.
* Understand a captured MOVING, and stop dedup ignoring sequence zero
Captured from Enterprise 8.6.2 during a maintenance_mode scenario:
>4 $6 MOVING :0 :15 _
Four elements - type as a bulk string, sequence number, a 15-second window, and an
explicit RESP3 null for the address, meaning "no replacement given, reconnect the
way you connected". The proxy then closed the socket, which is the whole point of
MOVING: you are told to move, and then the connection goes away.
That confirms two ledger items as written: the element order, and that the
no-address case really is an explicit null rather than an empty string or an
absent element. Our parser already read it that way.
It also exposed a wart. Sequence numbers can legitimately be zero - this one was
the first event of its chain - and dedup treated a stored zero as "never seen", so
whichever notification opened a chain could never be recognised as a replay. The
"have we seen one" state is now a separate bit.
Pinned as a regression test alongside the SMIGRATING/SMIGRATED capture, including
that MOVING opens a relaxed window, since that window is what covers the reconnect
after the socket closes.
* Sequence ids are evidence-backed now, not an invention
Observed on Enterprise 8.6.2: monotonic per database, zero-based on a fresh one,
shared across notification types (SMIGRATING 16 then its SMIGRATED 17), and
identical on every node broadcasting a given event - so the number identifies the
event rather than the connection that delivered it, which is exactly what dedup
needs.
The XML docs said the opposite - 'do not assume they are contiguous, or that they
are scoped the same way across notification types' - so they are corrected, with
the caveat that this is one build of one product and cross-deployment use stays
heuristic.
Also records why the per-type key is kept despite the counter being shared: within
a type the ids are still monotonic, and a per-type key cannot mistake one node's
earlier event for a replay of another node's later one.
* One event per logical notification, and captures for the DMC family
Every node broadcasts a given event with the same sequence number, so a
three-proxy deployment delivered one migration three times. Collapse that:
ConnectionMultiplexer.TryClaimMaintenanceEvent holds a fixed 8-slot ring of
(type, sequence) and raises the public event for the first arrival only. The
per-server work still runs for every copy - relaxation is per-ServerEndPoint
and each connection has to open its own window - so EndPoint on the event now
means "whichever node told us first", documented as such.
Matched on equality rather than <=, so a lagging node reporting an earlier
event we have not seen is still raised; expired by eviction rather than on a
timer, since the copies arrive milliseconds apart.
Also captured from Enterprise 8.6.2, closing ledger items 7 and 11:
>4 $9 MIGRATING :0 :2 $6 ["27"] >3 $8 MIGRATED :1 $6 ["27"]
>4 $12 FAILING_OVER :0 :2 $6 ["21"] >3 $11 FAILED_OVER :1 $6 ["21"]
The opening notification carries a time and the closing one has no time
element at all - which is what CarriesTime already assumed - and the shard
list is a stringified JSON array of id strings. Pinned as tests; the fake can
now omit the time, which it previously always sent.
That test found a fake-server bug: Dispatch handed the same rented frame to
every opted-in client, the first client's write loop recycled it, and the
second faulted with "Array element cannot be nil" and lost its connection. So
every broadcast had only ever reached one client, which made multi-node
fan-out untestable. Built per recipient now, sequence id computed once.
* Model the server's catch-up channel in the fake
Redis Enterprise retains the most recent shard-scoped completion and replays
it to each connection that opts in, coalesced into the same read as the +OK.
The boundary is sharp (RS 8.0.22): MIGRATED and FAILED_OVER are retained;
MIGRATING, FAILING_OVER, MOVING, SMIGRATING and SMIGRATED are not. What fits
all seven is "the completion of a shard-scoped event" - the two carrying an
affected-shards list.
That gives the design property the handoff work depends on: the catch-up
channel can only ever say "a disruption ended", never "one is starting".
MOVING, the only notification demanding action, is never replayed - so a
reconnecting client cannot be told to hand off by a stale frame, and D6 needs
no staleness guard to be safe from replay. Asserted as a negative over all
five non-retained kinds.
Retention is most-recent-replaces, never a queue, so a connection sees at most
one. RetainCompletions turns it off for tests that would rather not reason
about which kinds are retained.
Needed a deferred-outbound slot on RedisClient: the read loop enqueues a
command's reply after Execute returns, so a handler calling AddOutbound
directly puts its frame *before* its own +OK, which is the wrong order.
The replay also gets us the first test of a push frame arriving mid-handshake,
interleaved with our own handshake replies - previously every notification
arrived on a settled connection.
Not implemented: the catch-up-aware skip of the topology refresh. SMIGRATED
turns out not to be retained, so no retained frame can reach the refresh path,
and the guard would be unreachable code.
* Probe traffic is not caller work, and drop a ValueTuple from the library
Health-check probes counted as work a caller is waiting on, so an endpoint
being probed looked busy - and idleness is what decides whether an endpoint
that has left the deployment can be retired. Invisible while probes only ran
under MultiGroupMultiplexer; a blocker for enabling them anywhere retirement
also runs.
Deliberately not the internal-call bit, which would have been one line: that
flag also decides queuing, bypassing the backlog and queuing while
disconnected regardless of policy. A health check that bypasses the backlog
cannot see a bridge whose queue is not draining, which is the signal
geo-redundant failover depends on. So this is a separate bit (19), and the
four accounting sites now ask IsCallerFacing rather than !IsInternalCall.
The flag has to be on UserSelectableFlags, because probes reach the pipeline
through the public API and the constructor masks anything else - so it arrives
as caller-supplied flags or not at all. A caller passing it only opts their own
command out of idleness accounting.
Exposed as HealthCheckContext.ProbeFlags so a third-party probe can be correct
too. An injecting IDatabase wrapper would make that automatic, and
[AutoDatabase] would make it mechanical, but it would also make the flag
invisible and un-opt-out-able, and a probe that forgets it merely reproduces
today's behaviour.
Also: the dedup ring added yesterday used a tuple, which pulled ValueTuple
into the library and broke SanityChecks.ValueTupleNotReferenced - .NET
Framework consumers would need the package. Named struct instead. That is
already pushed on this branch, and my filtered test runs hid it; the full
suite is what caught it.
* Maintenance notifications: the MOVING resolve primitive, and a fake that closes the socket (part of D6) (#3202)
* The fake's MOVING closes the socket, on the measured timing
MOVING's defining half was missing: the socket goes away. Measured on RS
(2026-08-28) the close lands at +18.4s and +16.6s against a declared 15s
window, so the window is a floor with slack rather than a deadline - the fake
defaults to announced-plus-slack, and tests assert that we act within the
window, never that the socket survives to the end of it. A shorter delay is
available to exercise a less generous proxy.
Blast radius is the node, not the connection: four connections to one node
differing only in handshake all closed simultaneously, and only the opted-in
ones were warned. So the close is scoped to siblings sharing a node, and D6
will reuse RetireAsync rather than anything narrower.
A zero delay is deliberately not a case: it races delivery of the notification
itself, which no real timing produces. My first version of this test asserted
it, and it failed for that reason.
* The MOVING re-resolve loop: poll DNS past the address being retired
Measured behaviour makes this a poll, not a lookup. Relative to the
notification: the endpoint moves server-side at +8.6s, DNS follows at +9.7s
and +4.4s across two runs, and the sockets close at +18.4s and +16.6s -
against a declared 15s grace and a 5s TTL. So the first answer names the
address we were just told to leave, in every run observed, and a client that
treats it as authoritative hands off to the node it is trying to escape. The
short TTL is what makes polling work: several attempts fit in the window.
MovingEndpointProbe is deliberately pure - the caller supplies the resolver,
the interval and the budget - because the alternative is untestable: no
in-process fake can move a DNS record. Jitter stays at the call site with the
existing refresh jitter.
Returning null when the window expires is a result, not a failure: the server
closes the socket anyway and the relaxed window covers the reconnect, so
guessing an address would be worse than doing nothing.
Seven tests, including the ones that matter: DNS trailing the notification,
a resolution blip mid-handoff, a round-robin record naming both nodes at once,
and a zero window still getting one attempt ("act now", not "do nothing").
* Multi-address hostnames are the common case, so stepping sideways is the norm
Measured 2026-08-28, all on a 5s TTL: all-nodes 2 A records,
all-master-shards 3, single 1 - and an all-master-shards database whose shards
shared a node also resolved to 1. So the count follows actual proxy placement
rather than the policy name, and `single` (the shape the MOVING timeline was
measured on) is the unusual one.
The rule survives unchanged, which is the useful part: "take any address that
is not the one being retired". With several records the first resolution
already names a live sibling proxy, so the handoff steps sideways at once
rather than waiting ~9s for DNS - any proxy of the same database serves the
same data. The poll only engages when the record names nothing but the
retiring address, which is exactly where waiting is the only option. Nothing
reads the policy, so placement-driven counts need no special case.
Two tests pin the branches, and the log now distinguishes them, because
"stepped sideways to a sibling that was already advertised" and "the record has
moved to the replacement" look identical otherwise and mean different things
when someone is debugging a handoff.
Note a full-fleet operation can hand us a sibling that is also about to be
retired, so handoffs can chain. Self-limiting - each MOVING carries its own
window and relaxation - but worth recognising rather than mistaking for a loop.
* Ask whether we are still advertised, and fix a race in my own test
The measured gap this closes: on a multi-proxy database, taking a node out on
the *shrink* path announces nothing about the endpoint, drops the victim from
DNS at +21.4s, and closes its socket silently at +34.7s. So for thirteen
seconds the condition is plainly visible to anybody who asks - our address is
no longer advertised - and the client's only other signal is a socket dying
with no explanation. MIGRATED lands ~5s before DNS moves and is the one
notification the server retains, which makes it the prompt to ask on.
IsStillAdvertisedAsync returns bool?, and the null carries weight: a
resolution failure, or a record momentarily resolving to nothing, is "cannot
tell" and must never become "give it up", or one DNS blip recycles every
healthy connection at once.
MovingEndpointProbe -> AdvertisedAddressProbe: the name stopped describing it
once it answered two questions. Both reduce to "what does the record say now,
and is my address in it", which is why this is one primitive and not two.
Also fixes MaintenanceOptInClientTests.OptInIsReArmedOnReconnect, which
asserted *client* state immediately after observing *server* state: the server
counts the opt-in when it processes the request, we mark the feature live when
we read the reply, a beat later. Intermittent under two cores, mine, and on a
branch that was already pushed - so it would have surfaced in CI rather than
here. Six consecutive clean runs after polling for the client side.
* MOVING fires when the address set gains a member, and DNS may lose the race
Nine observations now fit one rule: MOVING is emitted when the endpoint's
address set GAINS a member, and is silent when it only loses members. Policy
narrowing, maintenance_mode, a 3->2 exclude and a reduction to a single proxy
all only shrink, and all were silent; a substitution on that surviving single
proxy announced. So the discriminator is neither "single proxy" nor placement.
The consequence for the probe is a third outcome, now documented as measured
fact rather than as a defensive branch. On one cluster DNS was correct 4.4-9.7s
after MOVING, comfortably inside the 15s grace; on another it updated at
+18.7s, three seconds AFTER the socket closed at +15.7s. So "window expired
with the record still stale" is normal, and the only move left is to reconnect
after the close and resolve then - which for a hostname endpoint is already
correct. Anybody reading the null return as unreachable would be deleting the
handling for a case that happens.
Also recorded why the rule stays "any address that isn't mine" rather than
"prefer a newly appeared address", despite MOVING marking precisely the moment
something joins: a live sibling is at least as good and is available now, while
the newcomer is invisible until the record updates. Preferring it means
waiting, and waiting is the failure mode. Replacement proxies were measured
accepting connections at +6.3s while DNS still advertised only the retiring
node - which is a good argument for remembering addresses, deferred because a
remembered address whose port was reassigned would be a silent wrong-server
connection and a proxied standalone gives us no identity check to catch it.
* Maintenance notifications: act on MOVING, and test it against a real deployment (rest of D6, and D9's dedicated testing) (#3203)
* A fault-injector test tier: one folder in, databases provisioned per shape
New project, tests/StackExchange.Redis.FaultInjector.Tests, net10.0 only -
these tests are about server behaviour, not our down-level targets. Picked up
by Build.csproj's glob so it compiles in CI, but CI's test step names the main
project explicitly, so it never runs there; build.ps1's traversal does run it,
which is why the skip behaviour has to be right.
One path is the whole configuration: SER_FI_CONFIG_DIR (or the console's
FI_CONSOLE_CONFIG_DIR) points at the directory already mounted into the
injector as /app/config, so cluster credentials, the CA certificate and the
compose file are all found rather than hand-carried into the run.
Three states, deliberately distinct: no directory skips; a directory without
E2E_SCENARIO_TESTS=true skips (these create and delete real databases); and
configured-and-meant-but-broken FAILS. The third is the point - a suite that
skips on a broken environment reports success for tests that never ran, and
gets trusted at exactly the wrong moment. All three verified.
Databases are provisioned by the tests, per shape rather than per test, which
removes the conveyance problem entirely: a test that asked for oss_cluster
knows what it asked for, so endpoints.json stops being the source of truth for
per-database facts and its missing oss_cluster/endpoint_type fields stop
mattering. Shapes exist because they change behaviour - A-record count follows
proxy placement, and the handoff branches on whether a live sibling exists.
Named sertest-<shape>-<runid>. Cleanup is per fixture and unconditional; the
startup sweep matches the sertest- prefix and nothing else, so it can never
touch a database created by hand. Port collisions retry upward, as go-redis
has to.
TLS trusts the environment's CA via TrustIssuer. If the CA is missing, TLS
tests fail rather than disabling validation: a TLS test that quietly stops
checking identity reports success for the one thing it exists to catch.
Two traps from the console's known-gaps are encoded rather than left to be
rediscovered: poll on pending AND running (a loop waiting only on pending
returns while the job is still going), and setup_id lives in the injector's
memory so teardown keeps a bdb_id fallback. Teardown also runs on cancellation,
with its own budget - the one place the ambient test token must not apply.
Unverified and flagged in the README: the create_database parameter names are
the injector's prose-documented wire schema, gathered in one place so a real
run can correct them.
* Prove the fault-injector tier live, and narrow the MOVING rule
Run against a real RS 8.0.22 deployment: both template databases connect,
negotiate RESP3 and report the opt-in active, and all four
topology-change-standalone scenarios run end to end in 7m32s with the
notifications observed and parsed. Cleanup verified - four scenarios left the
cluster with exactly its two original databases.
The rule the measurements produced is a conjunction, narrower than either half:
MOVING fires when the connection's own proxy LEAVES the endpoint's address set
AND the set GAINS a member. The counter-example is dns_resolution_change, which
widens single -> all-master-shards: addresses are plainly added, yet nothing is
announced, because the client's proxy is not going anywhere - and then the proxy
restarts and the socket closes at +44.5s with no warning. That also resolves
what looked like a contradiction, maintenance_mode announcing on a single-proxy
database but not on a multi-proxy one: with one proxy, moving it *is* a
substitution. The scenario expectations encode this, silence included, so a
build that starts announcing the widening case tells us rather than passing
quietly.
Three more findings. The window overshot again, by 19.1s and 17.5s against a
declared 15s, so "floor with slack" has four independent measurements and no
counter-example. The sequence counter is shared across all types including
MOVING (0, 1, 2 in one chain), which the per-type dedup already assumed. And
data_movement_no_conn_drop moved shards with both notifications delivered and
the connection never disturbed, so MIGRATING does not imply an impending
disconnect.
Corrections to the harness from real responses, replacing guesses:
- scenario setup provisions its own database and returns setup_id, bdb_id,
db_name, endpoints, password, tls, mtls_files and config in ~12s, so scenario
tests need neither create_database nor endpoints.json nor the REST API
- every trigger publishes the dbconfig it requires, and all four want
proxy_policy: single, which no template creates - hence setup provisioning
- setup_id is a handle, not an action id: polling /action/{setup_id} 404s
- the create_database schema now matches bdb_config.json, which disambiguated
oss_cluster_api_preferred_endpoint_type (ip vs hostname, and therefore whether
a TLS client can verify its targets) from ..._preferred_ip_type (internal vs
external routing) - I had conflated them
Traversal run with no environment configured: 7 skipped, everything else green.
* Cover the injector's scenario families, and sort what is left into buckets
Ran the fault injector's scenarios against the live RS 8.0.22 cluster and added
the ones that hold their value as tests. Green live: the OSS cluster family
(SMIGRATING/SMIGRATED parsed to source -> target, with 1440 reads and zero
failures across a real shard migration), sharded subscriptions recovering
unaided after a migration - D5's resubscription, previously fake-only - the
failover pair (FAILING_OVER seq=0 time=2s ["52"], FAILED_OVER seq=1) received
end to end for the first time, and proxy restart recovery.
Four schema facts the injector taught us, each replacing a guess:
- create_database wants its config nested under "database_config"; a flat
payload is rejected with "got None"
- sharding requires shard_key_regex, or Redis Enterprise refuses the database
with "Invalid sharding configuration"
- /slot-migrate/setup's trigger is how to *provision* (only "reshard"), not how
to migrate, and its effect enum is narrower than the discovery endpoint's -
remove-add cannot be set up at all
- setup provisions a database and returns it, so scenario tests need neither
create_database nor endpoints.json
Also two harness fixes worth their own mention. The create retry loop retried
everything, so "missing shard_key_regex" arrived eight times over half a minute
instead of once; it now retries only port collisions. And the traceback
summariser split on '\n' when the injector's JSON carries the two characters
backslash-n, so every skip message was a wall of Python.
Scenarios this cluster cannot produce - add and slot-shuffle need a node with
several shards, and three nodes with sparse placement give one each - now skip
on a matched message rather than failing. Deliberately not run while unattended:
shard/node/proxy/cluster failure, node_remove and reset_cluster, which damage or
reset the cluster.
The four-bucket assessment is in the notes: what works, what this feature still
owes (D6's action half, the connect-failure trigger, moving-endpoint-type,
MAINT_NOTIFICATIONS_INFO), what already works outside the feature, and what is
untested - of which network_latency matters most, because it is how timeout
attribution finally gets live evidence.
* Reach the TLS variant, and diagnose why it cannot run here
include_tls does not request TLS - it widens the list of variants setup may
choose from, and variant_index picks one. With no flags a trigger offers one
variant (single), with include_tls two (single, single_tls), with include_mtls a
third (mtls). Passing include_tls alone provisions variant 0 and yields a
plaintext database, which is how the first attempt skipped itself.
With variant_index=1 the database came up TLS-enabled and the connect was
refused: the remote certificate was rejected by the validation callback. That is
the environment, not us - the folder's server certificate covers
*.marcgravell-test-46be1d08... while the live cluster is
marcgravell-test-e21cd75d..., left over from an earlier provision and three days
older than the env_output.json beside it. Our behaviour was right: TrustIssuer
tolerates chain errors only, so a name mismatch fails outright, which is the
whole point of it.
So the test now compares the certificate's DNS names against the cluster name
*before* provisioning anything, and skips naming both. Without that, a stale
certificate reads as a client bug and costs somebody an hour of certificate
archaeology; the check costs nothing and happens before a database exists.
Also set AbortOnConnectFail=true in the TLS test only. Everywhere else
tolerating a slow start is right, but with it false a certificate problem is
indistinguishable from a slow cluster: ConnectAsync succeeds, IsConnected is
false, and the reason is gone. That is exactly how the first failure presented.
Traversal with no environment: 15 skipped, everything else green.
* D6: act on MOVING instead of waiting to be disconnected
Today a MOVING is survivable - the socket closes and we reconnect - but the
announced window goes entirely unused, and the reconnect re-resolves to whatever
DNS says at the moment the server chose, which has been measured as still naming
the node being retired. This uses the window: wait for DNS to move, then pick
the moment ourselves.
The dispatch turns on the form of the endpoint, which also corrects the earlier
assumption that MOVING should reuse endpoint retirement:
- hostname, no successor (every observed MOVING): the ServerEndPoint stays, only
the address behind the name moves, so retiring it would delete our only route
to the deployment. Probe until the record moves, then recycle the connections
so they re-resolve.
- address with a named successor: genuinely a different endpoint, so re-read the
topology. Never observed - eleven routes, all explicit nulls - so this exists
because the contract has it.
- address, no successor: nothing to re-resolve and nowhere named to go. Doing
nothing is correct.
Deciding is separated from acting so the decision can be tested exhaustively
without a server: DecideAsync takes the endpoint, the current address, the window
and a resolver. That seam exists because the whole thing turns on DNS *changing*,
which no in-process fake can arrange - ConnectionMultiplexer.AddressResolver
defaults to real DNS.
Recycling is a dispose: that already routes through RecordConnectionFailed to
OnDisconnected, which reconnects immediately, so there is no new lifecycle to get
wrong. Both bridges, because the measured blast radius is the node. Drained
first, bounded by what is left of the window - the socket dies at the end
regardless, so anything undrained was going to fail either way and draining
strictly dominates.
Jitter is a fraction of the window rather than a flat delay, capped at a second.
A 2s window - which the shard notifications really do announce - must not spend
half of itself waiting, and a 15s window does not justify a long wait when DNS
has been seen moving after four seconds.
Also safe from replay by construction, which is why there is no staleness guard:
the server retains only shard-scoped completions, so a MOVING is never delivered
as catch-up.
Nine tests: five decision branches, jitter bounds, and an end-to-end recycle
against the fake with MovingClosesConnection deliberately off, so the only thing
that can replace the connection is our own handoff. Three consecutive two-core
Release runs: 6215 passed, 0 failed.
* D6 proven live, and the feedback loop it exposed
On the real cluster the handoff does what it was built for:
conn_drop/endpoint_rebind MOVING +9.3s -> recycled and reconnected +9.5s
server would have closed at +25.5s
maintenance_mode MOVING +21.7s -> recycled and reconnected +21.9s
server closed at +38.0s
So we move roughly sixteen seconds before being pushed, on both routes.
The first live run also found a bug that no fake could have produced: a server
re-sends MOVING to a connection that opts in while the window is still open.
Since the handoff replaces the connection, acting on the repeat loops -
recycle, reconnect, get told again, recycle - and it produced twelve recycles
from a single event. OnMaintenanceWindowOpened already claimed the sequence id
and knew it was a repeat; the handoff was not asking. It now returns whether
the notification was new and the handoff gates on it, and the live test asserts
*exactly one* recycle.
Second finding, recorded rather than fixed: our own recycle does not raise
ConnectionFailed, because disposal is not reported as a failure. From outside
the library a handoff is therefore invisible - an operator sees a reconnect with
no reason given. HandoffRecycles and LastHandoffOutcome exist because
Multiplexer.Trace is [Conditional("VERBOSE")] and compiles away, so there would
otherwise be no record at all of what a handoff decided. Whether it should
surface something publicly is a real question, not settled here.
Two consecutive two-core Release runs: 6215 passed, 0 failed.
* Report a handoff as MaintenanceHandoff rather than silently
A handoff was invisible from outside the library: the replacement connection
raises ConnectionRestored, but our own recycle raised nothing, so a consumer
tracking connection state saw a restore with no matching failure and no reason
for the churn.
The reporting block is gated on "if (_ioStream is not null || isInitialConnect)"
- if *we* didn't burn the pipe, flag it - and Dispose runs Shutdown first, which
is precisely why an ordinary dispose is silent. So the fix is ordering: record
the failure while the pipe is still live, then dispose.
ConnectionFailureType.MaintenanceHandoff is the right home. The existing event
args already carry endpoint, connection type and a discriminator, and
CircuitBreaker is the precedent for a deliberate client action reported this way.
Documented for what it is: consumers alerting on ConnectionFailed should filter
it out, since it means planned maintenance rather than a fault - and the test
asserts we report *only* that, never SocketFailure or SocketClosed, so planned
maintenance cannot end up in fault dashboards.
Four consecutive two-core Release runs at 6215 passed. Note one earlier run
reported two failures whose names I did not capture and which have not recurred
in four runs since; if they come back I will capture them properly rather than
guess.
* Fix the cluster-flag call, which had been failing silently
update_cluster_config wants its flags nested under "config" - the same shape
create_database wants for "database_config" - and a flat payload is rejected
with "Invalid parameter 'config': got None".
Because the call is best-effort and only wrote to Console, it failed silently
for a full day of testing without anybody noticing. Note the impact was small:
the environment templates enable these flags at provision time, so this call is
a safety net rather than the mechanism, and every test was passing on its own
merits. It matters for an environment provisioned without them, where the
alternative is every test failing at connect and blaming the client for a
server-side setting. Verified corrected against the live injector.
Fixture diagnostics now go to a collected SetupLog as well as the console, since
a fixture has no test output helper and console writes are exactly what got lost.
* Reach the migrations and the TLS variant that were being skipped
Three "environment limitations" turn out to have been mine.
add and slot-shuffle were skipping with "No node with multiple shards found",
and remove-add was unreachable because /slot-migrate/setup's effect enum
excludes it. The cluster was never the problem: the setup leg provisions one
shard per node, so there is nothing to move a shard *from*. Provisioning our own
database - six shards, dense placement, two per node over three nodes - and
driving the run leg by bdb_id makes all three run, and all three now pass live.
The generalisation is the useful part: a scenario setup cannot arrange is still
reachable by provisioning the database ourselves.
remove-add is the best of them: it moves every shard as five SMIGRATING/
SMIGRATED pairs sharing one sequence chain (0-9), which exercises the dedup and
the event collapse far harder than a single migration.
The first dense attempt failed for an unrelated reason: the client trie…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Groundwork for the
MOVINGhandoff, split from the behaviour that uses it. Nothing in the library callsAdvertisedAddressProbeon this branch - the caller lands in the PR above, which is why this one changes nobehaviour and carries no risk beyond the toy server.
Two pieces:
1.
Maintenance/AdvertisedAddressProbeanswers the two DNS questions a handoff needs - what replaces theaddress we are leaving, and are we still advertised at all. It is a poll, not a lookup, and that is the
whole point. Measured on Redis Enterprise, relative to a
MOVING: the endpoint moves server-side at +8.6s, DNSfollows at +4.4s to +9.7s, and the socket closes at +16.6s to +18.4s, against a declared 15s grace and a 5s
record TTL. So resolving immediately returns the address being retired, in every run observed - a client
that treats the first answer as authoritative hands off to the node it was told to leave. The short TTL is what
makes polling work.
It is deliberately pure - the caller supplies the resolver, the interval and the budget - because the decision
turns on DNS changing, and no in-process fake can move a record. That shape is what makes the 12 tests
possible at all.
Three findings are encoded rather than rediscovered:
saw the socket close at +15.7s and DNS update at +18.7s - three seconds after the close. So there are three
outcomes, and the third ("window expired, record still stale") is normal, not defensive. Do not simplify the
null return away.
hostname usually carries several A records (measured: 2 for
all-nodes, 3 forall-master-shards, 1 forsingle, all on a 5s TTL, and the count follows placement not policy), so the first resolution normallynames a live sibling proxy and we step sideways at once. Preferring a newcomer means waiting for it to become
visible, and visibility is the slow part.
MOVINGfires when the connection's own proxy leaves the address set and the set gains a member. Bothhalves matter: a pure reduction takes the proxy away silently, and a pure widening adds addresses while
leaving our proxy in place, equally silently. That conjunction fits all thirteen observations, including the
ones that previously looked like exceptions.
2. The toy server now closes the socket after
MOVING, which is its defining half and was missing - soevery D6 test before this was a fiction. Scoped to the node rather than the notified connection, because the
measured blast radius is the node: four connections to one proxy, differing only in handshake, all closed
simultaneously, and only the two that had opted in were warned. Default delay is the announced window plus
slack, since the close was measured arriving 1.6s and 3.4s after a declared 15s - so tests assert we act
within the window and never that the socket survives to the end of it. A shorter delay is available to
exercise a less generous proxy.
Tests
12 in
AdvertisedAddressProbeTestsagainst an injected resolver: DNS trailing the notification, a resolutionblip mid-handoff, a round-robin record naming both nodes, a zero window still getting one attempt ("act now"
rather than "do nothing"), placement deciding whether we wait, and cancellation. Plus
MovingClosesTheConnection AndWeRecoverfor both close timings.Note for review
MaintenanceOptInClientTests.OptInIsReArmedOnReconnectis also fixed here, and conceptually belongs to #3193:it asserted client state immediately after observing server state, and those are a beat apart - so it was
flaky under a core-starved runner. It is fixed on this branch because that is where it surfaced.
Verification
Build.csprojclean in Release withCI=true; full local suite green over repeated two-core runs.