Maintenance notifications: opt in, and receive (D2) - #3193
Merged
Conversation
This was referenced Aug 25, 2026
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.
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.
mgravell
force-pushed
the
marc/maint-optin-server
branch
from
August 26, 2026 08:20
88b79ce to
0f3ae13
Compare
mgravell
force-pushed
the
marc/maint-optin-client
branch
from
August 26, 2026 08:20
dd8c507 to
d615d35
Compare
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.
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.
mgravell
marked this pull request as ready for review
September 1, 2026 13:27
… (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.
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.
Stacked on #3191 (
marc/maint-optin-server), which carries the toy-server half. Draft, like its base: nothing here is meant to land yet.This is stage 1 of maintenance notifications ("smart client handoffs") in both directions - we ask for them, and we receive and report them. No reaction of any kind yet, deliberately, so a consumer can watch what its servers announce before any behaviour depends on it.
The opt-in (
556b2017)ConfigurationOptions.MaintenanceNotifications(maintNotifications), tri-state, gated[Experimental("SER010")]withdocs/exp/SER010.md. The handshake sendsCLIENT MAINT_NOTIFICATIONS ONpipelined next toCLIENT ID- a bareON, so the server picks the endpoint type; deriving one ourselves is later work and a bareONis explicitly valid.Worth a reviewer's attention:
Enabledmeans required, not on. It rejects the connection when notifications cannot be delivered: a server that refuses, a server that answersHELLO 3as RESP2, and a configuration that could never ask (Protocol = Resp2, or noHELLO). That last case diverges from the letter of the cross-client 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.Autois the best-effort mode and never rejects a connection.Enabledis go-redis'senabled; ourAutois itsauto). ADisabled/Enabled/Demandspelling was built and reverted, because portability of the config string beat the clarity. The mitigation is documentation: the rejection warning leads the XML docs on both the enum member and the property, so it shows in the completion list rather than only on hover.AzureManagedRedisOptionsProviderisAuto, pre-emptively. AMR does not emit these yet and is adding support concurrently;Autois what makes announcing early safe, and it then starts working without anybody editing a connection string. Pending validation against a real AMR endpoint - being discussed on Maintenance notifications: opt in, react, and hand off #3191. This is the one change here with a blast radius beyond us, and it is one property override plus oneInlineDataif it needs to come out.Disabled. Most servers have never heard of the subcommand, and the server is not the only thing in the path: an unrecognizedCLIENTsubcommand is not guaranteed to be answered as politely by a proxy as by a server.Receiving (
dd8c507a)Seven new
PushKindmembers, dispatched inOnOutOfBandbefore anything reads element 1 as a channel name - because element 1 is a sequence number, which is the whole reason these could not be handled as pub/sub. Parsing lives in a newPhysicalConnection.Maintenance.cs;PushMaintenanceEventis raised through the existingConnectionMultiplexer.ServerMaintenanceEvent, so it already flows throughIConnectionMultiplexerandMultiGroupMultiplexer.timeelement is expected, not the content. A single-slotSMIGRATINGpayload of"123"is indistinguishable from a duration by inspection, so content-sniffing would silently lose the slot list. There is a test for exactly that.MOVING's placeholder forms - explicit null,?, a zero port - all yieldNewEndPoint = nullwith the raw text kept, never the answering server. Same reasoning as the unroutable-redirect work in Route from CLUSTER SLOTS, and give endpoints a lifecycle #3177.PushKindlookup is now case-insensitive throughout, not just for the new members: the pub/sub kinds are lowercase on the wire and these are uppercase.Testing
MaintenanceOptInClientTestsandMaintenanceNotificationTests, plus a suite-wide switch (REDIS_TESTS_MAINT_NOTIFICATIONS, mirroring the existingREDIS_TESTS_MIN_TIMEOUT_MS) so the whole suite can run with the opt-in on. Verified: the full net10.0 suite is identical with it atAutoand with it off. That is the point - against every OSS server we test on the opt-in is refused, and nothing else changes.The two tests I would keep if I had to drop the rest: a malformed push frame is consumed and forgotten, with ten command round-trips afterwards proving the connection is still in sync rather than merely alive; and 50 notifications interleaved with 50 round-trips with nothing lost or mismatched.