Maintenance notifications: opt in, react, and hand off - #3191
Draft
mgravell wants to merge 3 commits into
Draft
Conversation
Collaborator
Author
|
@philon-msft my plan is to turn on SCH for AMR (in the options-provider) pre-emptively - i.e. ahead of AMR actually supporting it; this would be in "auto" mode, which means "only try in RESP3 mode; if it gets rejected: meh, no problem" - however, before I do this, I'd want to validate against a real AMR endpoint that this doesn't break anything. Would you be able to lend me something to test this against, or otherwise help me validate this? If we need to do "turn it on in AMR if the server reports version > XXX, otherwise don't because it breaks the proxy/whatever" then that's totally fine too: we'll make it work - I just want to not break your users. Specifically, this would issue an additional command:
|
This was referenced Aug 25, 2026
…events Deliberately on RedisServer rather than a bespoke test subclass. The opt-in is an ordinary command, so any test should be able to use it, and a server that never sends a notification is the normal case - every OSS, Valkey and Garnet build behaves that way, and so will our own docker topology. The interesting behaviour is all client-side, so the fake should not be a special place. CLIENT MAINT_NOTIFICATIONS <ON|OFF> [parameter value ...] per the contract: a bare ON is valid and means "server defaults", moving-endpoint-type is the only parameter defined so far, and its five values are validated. Unknown parameters are refused rather than ignored - the client is asking the server to do something specific, and silently not doing it is worse than saying no. State is per connection, including a count, since re-arming after a reconnect is a requirement and a count is what distinguishes that from having opted in once. MaintenanceNotifications selects how the server answers: accept, reject as an unknown subcommand (what a server that never heard of it does), or reject as disabled (what one with the feature flag off does). A client has to survive all three, so a test has to be able to ask for all three. Sending covers MOVING with or without an address, the shard-scoped and slot-scoped families, explicit or generated sequence ids - the contract never defines those, so repeating one deliberately is part of what the fake owes us - and a raw-push hook for malformed frames. Notifications go only to connections that opted in; a raw push is not gated, which is the contrast the tests assert. Two bugs the tests caught: the count returned was clients *visited* rather than sent to, because ForAllClients' Action overload returns one per client regardless; and an assertion comparing against ClientCount was racy, since under RESP2 the subscription connection can register between the send and the read.
mgravell
force-pushed
the
marc/maint-optin-server
branch
from
August 26, 2026 08:20
88b79ce to
0f3ae13
Compare
* 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…
mgravell
changed the title
… work (#3204) docs/ServerMaintenanceEvent.md already existed for the Azure pub/sub family, so this extends it rather than adding a page: the intro now distinguishes the two routes, and a new section covers the server-native RESP3 family. The section leads on the thing most likely to bite a user: for a recognised hostname the feature is automatic, because the matching options provider turns it on - but a custom domain, a CNAME, private DNS, a proxy, or a self-managed cluster matches nothing, so it falls back to Disabled and *nothing fails*. You simply never get notifications. Both fixes are spelled out, with their differing blast radius: defaults=<provider> for the whole posture, or maintNotifications= Auto for this feature alone. Related: RESP3 needs no configuration (with no protocol set the client assumes 6.0 and negotiates it), but three settings silently take it away - protocol= resp2, defaultVersion below 6.0, and disabling or renaming HELLO - and without RESP3 there are no push frames to receive. Writing the "how do I check it is on?" section turned up that the diagnostic did not exist. The opt-in *refusal* was reported through PhysicalConnection.OnDetailLog, which is [Conditional("PARSE_DETAIL")] and compiles away in any normal build, so the reason a server declined was visible only to somebody debugging the parser; acceptance was not reported at all. All three - accepted, refused, and the handoff outcome - now go through the configured ILoggerFactory as LoggerMessage extensions (event ids 117-119), which is the channel that survives and the one an application actually reads. The handoff line closes a gap noted earlier: Multiplexer.Trace is [Conditional("VERBOSE")], so a handoff that replaced connections left no record. Two tests assert the accepted and refused messages, since they are documented behaviour now rather than incidental logging.
mgravell
changed the title
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.
Server-native maintenance notifications ("smart client handoffs") for Redis Enterprise and Redis Cloud: we ask
to be told when a deployment is about to disrupt us, and then act on it - relaxing timeouts, re-reading the
cluster topology, recovering stranded sharded subscriptions, and moving off an endpoint before it is taken away.
Validated end to end against a real Redis Enterprise deployment (RS 8.0.22) driven by the fault injector, which
is where most of the design below comes from: the specifications for this feature are prose, the payloads were
never published, and nearly every assumption I started with was wrong in some way that mattered.
What a user gets
Configuration -
maintNotifications, plusmaintRelaxedTimeout,maintRelaxedWindowMax,maintPostEventRelaxed, and adefaultskey for naming an options provider:MaintenanceNotificationModeis tri-state.Autoasks and tolerates refusal - the default where a providerenables it.
EnabledREJECTS THE CONNECTION if the server will not deliver them, including when we end upon RESP2, because a caller who asked for guarantees should not silently get none; the IntelliSense says so in
those words.
Observation is a first-class use:
ConnectionMultiplexer.ServerMaintenanceEventnow raisesPushMaintenanceEventcarrying the notification type, sequence id, endpoint, announced time, replacementendpoint and parsed
ClusterSlotMigrations, so an operator can watch what their deployment announces before anybehaviour depends on it.
What we do about each notification
MIGRATING,FAILING_OVER,SMIGRATINGMIGRATED,FAILED_OVERSMIGRATEDMOVINGTimeout relaxation is per-server and read at sweep time, and any timeout or connection failure raised inside a
window carries
MaintenanceType, so a caller can tell "the deployment was moving" from "your query is slow".Measured behaviour that shaped the design
Each of these contradicted a reasonable-sounding assumption:
15s, across four runs. So we act within the window and never assume the socket survives to it.
anywhere from +4.4s to +18.7s, and one cluster closed the socket at +15.7s with the record still stale. So the
handoff polls, and "expired, still stale" is a normal outcome that does nothing rather than guessing.
MOVINGfires when the connection's own proxy leaves the address set and the set gains a member. Bothhalves matter: pure reductions and pure widenings are both silent. Thirteen observations, no exception.
With a sibling advertised we step sideways immediately; only a single-address record makes us wait.
MOVING. That is whythe handoff needs no staleness guard: the one notification demanding action is never delivered as catch-up.
MOVINGreaches only the doomed proxy's connections;MIGRATING/MIGRATEDare broadcast to every proxy. Hence relaxation is per-server while the public event iscollapsed on
(type, sequence)- one callback per logical event, whichever node told us first.MOVINGis re-sent to a connection that opts in mid-window. Since the handoff replaces the connection,acting on the repeat is a feedback loop - it produced twelve recycles from one event before being gated on the
sequence dedup.
Testing
Three tiers. Unit tests for anything decidable without a server, including the DNS decision logic, which takes
an injected resolver because no in-process fake can move a record. The in-process toy server for the protocol -
it now speaks the opt-in, sends all seven notification types, models the retention/replay channel, and closes
sockets after
MOVINGscoped to the node, which is how the fixtures stopped being fiction. Thentests/StackExchange.Redis.FaultInjector.Tests, which drives a real deployment through the same fault-injectorsurface go-redis, redis-py and node-redis are tested against.
Without that configuration every test in the tier skips, and CI names the main test project explicitly, so CI is
unaffected. With the configuration present but broken, they fail - a suite that skips on a broken
environment reports success for tests that never ran.
Live results include: the opt-in accepted and active on standalone and
oss_clusterdatabases; a real shardmigration with
SMIGRATING/SMIGRATEDparsed source-to-target across 1440 reads and zero failures; 16/16sharded channels recovering unaided;
FAILING_OVER/FAILED_OVERobserved end to end;MOVINGhandled ~16seconds before the server closed the socket; and the whole path again over TLS with certificate validation on.
Not in this PR
docs/ServerMaintenanceEvent.md(D8) is still owed -docs/Configuration.mdanddocs/exp/SER010.mdareupdated, but there is no user-facing guide to the events yet. The
moving-endpoint-typeopt-in parameter andMAINT_NOTIFICATIONS_INFOare unimplemented. The named-successor handoff branch exists because the contract hasit, but eleven observed routes carried an explicit null, so it has never fired. AMR is set to
Autopre-emptively and cannot be validated until their fleet emits. And relaxed timeouts have no live evidence of
saving a command, because every scenario run had zero failures -
network_latencyis the way to get it.Public API
65 lines added to
PublicAPI.Unshipped.txt, 47 of them gated behind[Experimental(SER010)]- the notificationtypes,
PushMaintenanceEvent,ClusterSlotMigration,MaintenanceNotificationMode, themaint*configurationproperties, the two provider types, and
MaintenanceTypeon the timeout and connection exceptions. Ungatedadditions:
ConnectionFailureType.MaintenanceHandoff, so a deliberate handoff is visible rather than appearingas an unexplained reconnect, and `HealthChServer-native maintenance notifications ("smart client handoffs") for Redis Enterprise and Redis Cloud: we ask
to be told when a deployment is about to disrupt us, and then act on it - relaxing timeouts, re-reading the
cluster topology, recovering stranded sharded subscriptions, and moving off an endpoint before it is taken away.
Validated end to end against a real Redis Enterprise deployment (RS 8.0.22) driven by the fault injector, which
is where most of the design below comes from: the specifications for this feature are prose, the payloads were
never published, and nearly every assumption I started with was wrong in some way that mattered.
What a user gets
Configuration -
maintNotifications, plusmaintRelaxedTimeout,maintRelaxedWindowMax,maintPostEventRelaxed, and adefaultskey for naming an options provider:MaintenanceNotificationModeis tri-state.Autoasks and tolerates refusal - the default where a providerenables it.
EnabledREJECTS THE CONNECTION if the server will not deliver them, including when we end upon RESP2, because a caller who asked for guarantees should not silently get none; the IntelliSense says so in
those words.
Observation is a first-class use:
ConnectionMultiplexer.ServerMaintenanceEventnow raisesPushMaintenanceEventcarrying the notification type, sequence id, endpoint, announced time, replacementendpoint and parsed
ClusterSlotMigrations, so an operator can watch what their deployment announces before anybehaviour depends on it.
What we do about each notification
MIGRATING,FAILING_OVER,SMIGRATINGMIGRATED,FAILED_OVERSMIGRATEDMOVINGTimeout relaxation is per-server and read at sweep time, and any timeout or connection failure raised inside a
window carries
MaintenanceType, so a caller can tell "the deployment was moving" from "your query is slow".Measured behaviour that shaped the design
Each of these contradicted a reasonable-sounding assumption:
15s, across four runs. So we act within the window and never assume the socket survives to it.
anywhere from +4.4s to +18.7s, and one cluster closed the socket at +15.7s with the record still stale. So the
handoff polls, and "expired, still stale" is a normal outcome that does nothing rather than guessing.
MOVINGfires when the connection's own proxy leaves the address set and the set gains a member. Bothhalves matter: pure reductions and pure widenings are both silent. Thirteen observations, no exception.
With a sibling advertised we step sideways immediately; only a single-address record makes us wait.
MOVING. That is whythe handoff needs no staleness guard: the one notification demanding action is never delivered as catch-up.
MOVINGreaches only the doomed proxy's connections;MIGRATING/MIGRATEDare broadcast to every proxy. Hence relaxation is per-server while the public event iscollapsed on
(type, sequence)- one callback per logical event, whichever node told us first.MOVINGis re-sent to a connection that opts in mid-window. Since the handoff replaces the connection,acting on the repeat is a feedback loop - it produced twelve recycles from one event before being gated on the
sequence dedup.
Testing
Three tiers. Unit tests for anything decidable without a server, including the DNS decision logic, which takes
an injected resolver because no in-process fake can move a record. The in-process toy server for the protocol -
it now speaks the opt-in, sends all seven notification types, models the retention/replay channel, and closes
sockets after
MOVINGscoped to the node, which is how the fixtures stopped being fiction. Thentests/StackExchange.Redis.FaultInjector.Tests, which drives a real deployment through the same fault-injectorsurface go-redis, redis-py and node-redis are tested against.
Without that configuration every test in the tier skips, and CI names the main test project explicitly, so CI is
unaffected. With the configuration present but broken, they fail - a suite that skips on a broken
environment reports success for tests that never ran.
Live results include: the opt-in accepted and active on standalone and
oss_clusterdatabases; a real shardmigration with
SMIGRATING/SMIGRATEDparsed source-to-target across 1440 reads and zero failures; 16/16sharded channels recovering unaided;
FAILING_OVER/FAILED_OVERobserved end to end;MOVINGhandled ~16seconds before the server closed the socket; and the whole path again over TLS with certificate validation on.
Not in this PR
The
moving-endpoint-typeopt-in parameter andMAINT_NOTIFICATIONS_INFOare unimplemented. The named-successor handoff branch exists because the contract hasit, but eleven observed routes carried an explicit null, so it has never fired. AMR is set to
Autopre-emptively and cannot be validated until their fleet emits. And relaxed timeouts have no live evidence of
saving a command, because every scenario run had zero failures -
network_latencyis the way to get it.This PR does not yet consider interactions between geo-redundant-failover ("active:active") and smart-client-handoffs ("hitless"); this is feature D10 on my internal tracking.
Public API
65 lines added to
PublicAPI.Unshipped.txt, 47 of them gated behind[Experimental(SER010)]- the notificationtypes,
PushMaintenanceEvent,ClusterSlotMigration,MaintenanceNotificationMode, themaint*configurationproperties, the two provider types, and
MaintenanceTypeon the timeout and connection exceptions. Ungatedadditions:
ConnectionFailureType.MaintenanceHandoff, so a deliberate handoff is visible rather than appearingas an unexplained reconnect, and
HealthCheckContext.ProbeFlags(SER007).Verification
Build.csprojclean in Release withCI=trueacross all target frameworks; full suite green over repeatedtwo-core runs (
taskset -c 0,1, the recipe that reproduces the CI-only races); every live scenario green, witheach run leaving the cluster exactly as it found it.
Checklist