Maintenance notifications: opt in, and receive (D2) - #3193
Draft
mgravell wants to merge 4 commits into
Draft
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-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.
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 Toy server: support maintenance-notifications #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.