Maintenance notifications: relax timeouts, and read the cluster delta (D4, part of D5) - #3194
Draft
mgravell wants to merge 21 commits into
Draft
Maintenance notifications: relax timeouts, and read the cluster delta (D4, part of D5)#3194mgravell wants to merge 21 commits into
mgravell wants to merge 21 commits into
Conversation
Collaborator
Author
|
Question from @philon-msft : do we invoke the ReconfigureAsync path, like we used to for the previous azure notification events? answer: not yet, but that is something we can look at in some of our later pieces; MOVING is not a completion, so it probably isn't the right time - but we have other notifications for that; using my own internal deliverable tracking, I think that comes under D6 |
mgravell
force-pushed
the
marc/maint-relaxed-timeouts
branch
from
August 26, 2026 08:20
70c0783 to
ecbda4d
Compare
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.
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.
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.
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.
mgravell
force-pushed
the
marc/maint-relaxed-timeouts
branch
from
August 26, 2026 09:27
ecbda4d to
2beec86
Compare
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.
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.
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.
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.
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.
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.
…t 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.
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.
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.
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.
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
1715117 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.
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.
mgravell
force-pushed
the
marc/maint-relaxed-timeouts
branch
from
August 27, 2026 14:48
9386272 to
921fe2d
Compare
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.
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.
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.
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.
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.
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 #3193, which is stacked on #3191. Draft. This is the first part of the feature that changes behaviour.
Relaxation (
6b76faf3)An opening notification (
MOVING,MIGRATING,FAILING_OVER,SMIGRATING) raises command timeouts for that server; a closing one stands them back down. Three settings, of which only the first is prescribed cross-client - the other two say "our invention" in their own XML docs:maintRelaxedTimeout(10s): what timeouts are relaxed to, as a floor. Effective timeout ismax(configured, this), so a generous caller keeps their value.maintRelaxedWindowMax(3x): a backstop for a closing notification that never arrives.maintPostEventRelaxed(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 got 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.These are in seconds, unlike every other timeout in
ConfigurationOptions, because that is the unit the cross-client contract names. The mistake is silent in one direction, somaintRelaxedTimeout=30000is rejected with a message that names the unit rather than quietly configuring an eight-hour timeout.Three things worth a reviewer's eye:
CheckBacklogForTimeoutswas measuring 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. Pre-existing conflation that only became visible because relaxation forced the question.Monitor.Waitcommits to a duration when it parks: without it a sync caller already waiting when a notification lands would time out at the strict timeout while its async neighbour was relaxed. The loop also handles the way back - if a window closes mid-wait, the revised timeout drops and the wait ends.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 it is deliberately conservative.
What is not relaxed, as a rule: 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. The one refinement is the dead-socket heuristic in
PhysicalBridge, which is derived from the command timeout and now tracks the effective one: at a 60s relaxed timeout, firing at 4x the strict 5s would tear down precisely the connection relaxation was protecting.Fault surface (
2116f7f3)MaintenanceTypeonRedisTimeoutException,RedisConnectionExceptionandFaultContext, defaulting toNone. "Timeout" and "timeout during an announced failover" call for very different reactions from whoever reads the log.On
FaultContextit 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. Happy to changeDefaultIsFailureinstead if that is the preference; it would change existing behaviour for AA users, so I left it.Cluster delta, read only (
70c07835)Cross-checking against the shipped clients (go-redis, redis-py) says
SMIGRATEDis nested -["SMIGRATED", seq, [[source, target, slots], ...]]- which two independent implementations agree on, and which means we were dropping it: the parser rejected any non-scalar element after the type. Now read and exposed asClusterSlotMigrationon the event. Still nothing applies the delta.Two incidental fixes the cross-check produced, both worth knowing independently:
SlotRange.TryParseInt16waschecked, so an out-of-range slot number threwOverflowExceptionfrom aTry*method - pre-existing, reachable today from the publicSlotRange.TryParseand fromCLUSTER NODESparsing. It rejects now.Verification
Full
Build.csprojclean across all TFMs (TimeSpan * intis netstandard2.1+, so the arithmetic is on.Ticks). Full net10.0 suite at 6249 tests with the switch atAuto; the only failure isFailoverTests.SubscriptionsSurvivePrimarySwitchAsync, which fails identically with the feature switched off and passes in isolation - an order/state artefact of the local failover pair, not from this work.