Repository navigation
Conversation
…pens within a round trip instead of at the deadline The MoveTo/Recycle paths drained by waiting for "no caller work" before replacing the connections. Under steady traffic that never happens: there is always a command in flight, so the drain waited out the entire announced window and recycled at the deadline, failing whatever was in flight at that instant and leaving no time to establish the replacement. Measured live on a Redis Enterprise endpoint rebind with ~450 ops/s on the connection: MOVING at +5.8s, MoveTo decided at +6.6s, connections replaced at +20.9s against a 15s window, 8 failed commands, and the "did not establish a replacement within the announced window" warning. The bridge now has a quiesce mode for the duration of the drain: new caller messages (single, batch, sync and async) go to the backlog instead of the socket, the backlog processor does not write while quiescing, and the drain waits only for commands already written. Held work is written to the replacement once it is established, as across any reconnect; a FailFast backlog policy no longer abandons it for a MaintenanceHandoff, which is a replacement we chose rather than a failure. Internal traffic (keep-alives) is not held. Same scenario with this change: MOVING +5.8s, handoff +6.7s, restored +6.8s, 0 failed, nothing outstanding at the recycle, no warning. The new test runs a steady load against the in-process server and asserts that the recycle follows the notification within a fraction of the window and that no command fails; it fails on main (recycle at ~10s, the whole window) and passes here.
…live caller work Review follow-ups on the quiesce-then-drain change: - The drain is capped at one second as well as by the window. A blocking or stuck command in flight would otherwise hold everyone else's work in the backlog for the rest of the window and still be failed at the end of it; past the cap the connections are replaced regardless, and whatever was still in flight fails as the server's close would have failed it. - The recycle takes the bridge's write lock and re-checks for in-flight work under it, so a writer that passed the hold check before it was set cannot slip onto the old connection between the check and the recycle. A writer that reaches the lock after a handoff recycled its connection queues for the replacement instead of writing to a dead connection; only for the handoff (an ordinary closure is still reported as one, #3167), never for a message bypassing the backlog, and never for internal traffic. - Held work goes through QueueOrFailMessage's queue branch, so QUIT is never queued and the duplication is gone; QUIT and internal traffic are not held at all, in the batch path as well as the single-message paths. - Work is held only while the connection is still live; once it has dropped, the ordinary disconnected rules and the backlog policy apply. - A held enqueue that lands just after the hold is released kicks the backlog processor itself rather than waiting for the heartbeat. - The orphaned summary is back on CheckBacklogForTimeouts, and docs/ServerMaintenanceEvent.md describes the hold. New test: slow commands in flight do not hold the drain past the cap, and traffic resumes on the replacement.
…e lock, with a bounded wait; requeue raced writers at the head Second round of review follow-ups: - Each bridge records the connection current when the hold begins and only ever retires that instance (TryRetireQuiesceTargetAsync). A replacement established meanwhile is never touched however often the drain loop retries, which the previous pass could do while the other bridge was still draining; and if the target went away by itself there is nothing to retire. - Every retirement happens under the write lock: the forced pass at the cap as well. The lock wait is bounded by a cancellation token the drain owns (the cap plus a second of grace) rather than the mutex timeout; if the lock cannot be had even then, the connection is left to the server's close rather than recycled around a writer. - A writer that lost its connection to the handoff while waiting for the lock is requeued as held work (never failed under FailFast) at the head of the backlog, via a front queue every backlog operation drains first, so it keeps its place ahead of what the hold queued after it. - The cap is half the relaxed timeout (five seconds by default) bounded by the window, and timeouts are relaxed for the hold, so a blocking command with a timeout up to the cap completes and nothing held is timed out by the hold itself. A fixed second failed commands the server's close would have let finish; the whole window held everyone else's work for one of them. - Each bridge is released in its own try/finally and the processor wake is guarded, so a bridge disposed mid-handoff cannot leave the other quiesced. - The two unused in-flight predicates are gone; the requeue check is one helper at the three lock sites. Tests: a slow command that fits inside the cap is answered and the recycle follows it; a longer one is failed at the cap and traffic resumes on the replacement; the steady-load test is unchanged.
…ly at the next hold, per-bridge release, cap floor Third round of review follow-ups: - BacklogRequeueFront takes lock(_backlog) around the head insert. The timeout sweep peeks, then dequeues and compares under that lock; an unlocked insert between the two handed it a different message, which it treats as a threading bug, and the dequeued message was dropped uncompleted. - _recycledForHandoff is cleared when the next hold begins, not when the replacement establishes: the write lock is not fair, so a writer parked on it can be bypassed by the backlog processor long enough for the replacement to come up, and clearing then made it write to the disposed connection and tear down the healthy replacement. - DrainCap has a one-second floor, so a relaxed timeout of zero (relaxation disabled) does not force the recycle with commands still in flight. - Each bridge is released as its own retirement resolves, not when both have; the interactive bridge usually drains in a round trip and must not hold work while the subscription bridge takes longer. EndQuiesce is idempotent and kicks the processor unconditionally on the transition, closing the store-load race with a concurrent enqueuer. - The drain works on snapshots of the bridge fields, and the mutex take and release tolerate disposal mid-handoff. - A batch carrying a QUIT is not held at all, so the hold decision and the per-message action agree. - The lockDeadline comment no longer claims the mutex timeout plays no part. New test: the steady-load handoff with maintRelaxedTimeout=0.
…work's timeout, budget from what is left Fourth round of review follow-ups: - A connection failure during the hold no longer abandons the held work under a FailFast backlog policy: that backlog is work diverted off a live connection, which would have been written and answered, so it is left for the replacement to flush. Previously only the handoff's own recycle was exempt. - The hold imposes its own floor on the effective timeout for its duration plus the reconnect (the cap, the lock grace and the connect timeout), independent of relaxation: with maintRelaxedTimeout=0 and a sub-second configured timeout, the one-second drain floor would otherwise have timed out exactly the work the hold protects. - The front requeue roots the multiplexer on the empty-to-non-empty transition, as the ordinary enqueue does. - The drain budget is what is left of the window after the decision has spent its share polling DNS, not the window minus jitter; with less than the floor left there is no hold, and the connections are replaced at once (still under the lock) so the replacement gets what remains. - The handoff marker is a short ring of the last four retired connections, never cleared, so a writer starved across back-to-back MOVINGs still recognises its connection. - Abandoning the backlog dequeues under the dequeuers' lock; a quiesced backlog pass returns before contending for the write lock; the processor start's wake tolerates disposal. - One hold predicate, shared by the single-message gates and the batch gate, which makes one pass over the batch. - The docs and the DrainCap remarks now agree with the clamp: the window bounds the hold because the server closes the socket when it ends.
…window, exact retired marker Fifth round of review follow-ups: - The hold cover spans the rest of the window plus one connect attempt, not one connect attempt alone: a MoveTo pins reconnection to the named successor until the window ends, and a successor that does not answer is tried until then. With relaxation disabled, held work could outlive the cover and be swept. - The drain cap is also bounded by half of what is left of the window, so a relaxed timeout of two windows or more cannot turn the hold into a full-window stall that recycles at the deadline with work in flight. - A disposed wake in KickBacklogProcessor still starts the processor, whose disposed-drain is the only thing that fails what is left once the heartbeat has stopped. - The hold is keyed on the quiesce target still being the current connection, not on IsConnected: once the target is gone, by the drain or by itself, new work and the backlog flow to whatever is current. - The retired-for-handoff marker is a flag on the connection itself, exact and holding nothing, in place of the four-slot ring. - The finalizer abandons the backlog without the dequeuers' lock; everything else keeps taking it. - The steady-load tests' resumption poll is wider, with a note that the in-process tunnel never dials the successor.
…, cover ends on establishment, teardown outside the lock Sixth round of review follow-ups: - A handoff-pending marker, set alongside the hold cover and cleared when a connection is fully established (or by the cover's deadline), keeps a FailFast policy from abandoning held work when a connect attempt on the way to the replacement fails. The hold alone covered the hold, not the reconnect it exists for. - Each bridge is retired by its own loop with its own lock deadline, run side by side, so a writer wedged on one bridge's lock cannot spend the other's time and leave its idle connection to the server's close. - The hold cover is cleared when the interactive connection establishes; the deadline is only the bound for the case where it never does. A short configured timeout with relaxation off regains hang detection the moment the replacement is up, instead of after the whole window. - Under the write lock the retire now only marks the connection; the report and dispose follow the release, so event handlers and socket shutdown no longer stall every writer and the replacement's handshake. The retired flag on the connection is what keeps writers and the backlog processor off it in the gap. - The front queue's comments say what it orders and what it does not. - The RetiredForHandoff doc sits on its own member again. - The docs say the floor wins over the half-window bound under two seconds. - No kick helper: EndQuiesce and the two held-enqueue paths pair a full fence with their flag/queue accesses, so neither side misses the other and no processor is started for an empty backlog. - The cap tests assert against jitter-aware bounds.
… a retired connection, FailFast exemption scoped, recycle path retires what is current Seventh round of review follow-ups: - The hold cover is cleared when both the interactive and the subscription connection are established (or the subscription is not required), not when the interactive one alone is: held subscribes must not be swept while the subscription bridge is still reconnecting. - A writer that reaches the lock with a retired snapshot never writes to it, internal and backlog-bypassing messages included: those fail as having no connection instead of faulting the write mid-teardown and tearing down the bridge; a caller's message is requeued as before. - The handoff-pending exemption from a FailFast abandon applies only to the handoff's own failures: the retired connection, or a replacement attempt that never got as far as a working connection. An established replacement that then fails is an ordinary failure. The marker is cleared when the handoff gives up on a bridge or finds nothing of its own to replace. - On the no-successor paths, a target that went away by itself during the hold is replaced by whatever is current, since the ordinary reconnect may have landed back on the node being retired; with a successor pinned the reconnect owns what happens next, as before. - The handoff tests gate on the slow command being observed in flight rather than on a fixed delay, and their time bounds carry more slack. - The docs say "approximately in the order they were issued". - One tick-sentinel helper (ServerEndPoint.NudgeFromZeroTicks) and one scan over both backlog queues.
…cover Eighth round of review follow-ups. The protection a handoff raises - the per-bridge shield against a FailFast policy abandoning held work, and the server's floor on the effective timeout - now shares a single lifecycle: raised in BeginQuiesce for a stated span, lowered only when a replacement connection fully establishes on that bridge, when the handoff gives up on it, or when the span runs out. The server's cover follows the bridges: it stands down when no bridge still has its protection up. - A target that went away by itself during the hold no longer stands the shield down: a replacement reconnect is in flight, and its first failure would otherwise have abandoned the held backlog. - The shield covers failures of the retired connection and of any connection that never completed its handshake, whatever the failure type; a reset mid-handshake reports as a socket failure like any other. A replacement that was fully established and then failed is ordinary. - The cover is no longer cleared by the old subscription connection still being live while the interactive replacement comes up; it waits for every bridge that still has a retire or reconnect pending. - When nothing recycles and nothing re-establishes, the cover stands down with the shield rather than waiting for its own deadline. - The orphaned sentinel doc and the doubled summary are gone; the docs say that under a second of window there is no hold at all.
…dge, leave a replacement that landed elsewhere alone Ninth round of review follow-ups: - The second backlog queue is gone. A writer that lost its connection to the handoff is appended to the backlog like the held work; the head insert bought only a best effort at ordering, at the price of a head-mutation discipline across every backlog operation. The abandon path keeps the dequeuers' lock, which closes a pre-existing race with the timeout sweep. - A bridge with no connection when the hold began lowers its protection at once, so a dead subscription bridge cannot keep the timeout floor up for the whole cover span while the interactive replacement is serving. - On the no-successor paths, a replacement that demonstrably landed on a different node than the one being retired is left alone; only one that landed back on the retiring node is retired. - The hold case speaks through the protection alone: the connection the hold is draining counts as the handoff's own, so the blanket "while quiescing" clause on the abandon path is gone and an established replacement failing during the hold is an ordinary failure, as the doc says. - The cover comments and docs say the cover stands down when every bridge's replacement is up, which is what the code does. - The drain-cap test zeroes the server latency as soon as MOVING is sent, so the replacement's handshake is not queued behind the slow replies.
… path, give-up keeps the shield for diverted work Tenth round of review follow-ups: - The landed-on address is read inside a try/catch: a socket still connecting throws on RemoteEndPoint, and a concurrent teardown throws too; either counts as unknown, and unknown is retired. Previously the throw escaped, dropped the shield and abandoned the handoff in the very race the branch exists for. - A hold that began with no connection (the old one dropped during jitter or the DNS poll) now goes through the same target-gone logic on the no-successor paths, so a reconnect that landed back on the retiring node is retired instead of surviving to the window-end close. With no current connection at all there is nothing to retire or protect, and the protection lowers. - Giving up on a bridge (the lock not obtained, or an exception) stops holding new work but keeps the protection for the work already diverted: it was queued off a live connection whatever the policy, on the promise of a replacement, and the server's close plus the reconnect after it are that replacement arriving late. The protection lowers on establishment or expiry, as always. - The bridge-disposed path lowers the protection: no replacement is coming. - The fence-then-wake idiom lives in one method opposite EndQuiesce. - The drain-cap test uses one blocking command, and zeroes the server latency only once the recycle is observed: the server reads its latency when it executes a request, and zeroing it earlier raced the command the test needs to stay slow.
… not on where the handoff's flow stands Eleventh round of review follow-ups: - A connection records that it was ever the one a hold diverted work off (WasHandoffTarget). A failure is the handoff's own if the connection has that mark or never completed its handshake, whatever the handoff's flow has done since: retired by the drain, closed by the server at window end, dropped by itself, or left un-retired after a give-up. The three remaining FailFast regressions were each a flow-based clear leaking on one path; the fact-based test closes the class. - A drain pass that finds no current connection no longer lowers the protection: work may have been diverted before the target went, and the next failed reconnect must not abandon it. Only a hold that never had a target, and so diverted nothing, lowers it. - The target itself completing a handshake it was in the middle of when the hold began is not mistaken for the replacement, so the protection and the timeout cover stay up while the hold is active. - An expired protection deadline and an expired cover are cleared on observation, as GetRelaxedRemaining does, so a tick count wrapping ~24.8 days later cannot re-arm them. - The docs scope "whatever the backlogPolicy" to the drain, and say what a FailFast policy does to new commands between the replacement and its handshake. - The in-flight check is done lock-free first; the write lock is taken only for a pass that is likely to retire. The one-line requeue alias is inlined.
…t; one tick-deadline type Twelfth round of review follow-ups: - A connection is a bona fide replacement when it is not the one the hold began on and, unless reconnection is pinned to a named successor, has demonstrably landed somewhere other than the node being retired - by address and port, since Redis Enterprise rebinds to other ports on the same host. Only such a connection lowers the protection when it establishes, ends the hold on new work and receives the held backlog. A reconnect that landed back on the retiring node (no successor, DNS still stale) is neither target nor replacement: new work is held off it, the backlog does not flush to it, and the drain retires it in its turn. - The protection deadline is monotonic: a hold beginning while an earlier handoff's protection is up extends it and remembers that older diverted work exists, so a hold with no target of its own does not clear it. - TickDeadline holds the sentinel, nudge, monotonic-extend and clear-on- expiry invariants once; the relaxed window, the hold cover and the per-bridge protection all use it. - The three handoff flags on the connection are volatile. - A requeued message keeps its write time; it only forgets the connection it was headed for.
…ement; a hold that diverted nothing protects nothing Thirteenth round of review follow-ups: - The bona fide test gains the input it was missing: once the retire has resolved, whatever connection comes up next is the replacement wherever it landed, including the same address and port - the common case after an in-place proxy restart or with stale DNS - so the protection stands down on its establishment instead of outliving its deadline. Before the retire resolves, the landed-elsewhere test still tells a replacement from a reconnect that landed back on the retiring node. The answer is latched on the connection once true, so the hot path does not re-read the socket. - A hold records whether it diverted anything (or inherited an earlier hold's diverted work); a drain pass that finds no connection lowers the protection only if nothing was diverted, whether or not the hold had a target. - TickDeadline takes TimeSpans and clamps them, and the announced MOVING window is capped at an hour on entry, so an absurd window cannot overflow a delay, a cancellation source or a deadline into the past. - The handoff's relaxation respects maintRelaxedWindowMax, as the notification path does. - The retire's report and dispose run under try/finally: a throwing user handler neither leaks the connection nor makes the retire look undone. - The hold cover extends rather than overwrites, per its documented lifecycle. - QueueOrFailMessage is back to its original shape; the gates queue held work directly, a batch with one fence for all its members.
…ded-on test applies after the retire, a lockless retire as the last resort Fourteenth round of review follow-ups: - A connection that an earlier handoff latched as its replacement and that a later handoff now retires is a target, not a replacement: marking it a target clears the latch, and the target check comes first. - With no successor pinned, the landed-on comparison applies after the retire as well as before it: a reconnect that landed back on the node being retired is not a replacement at any point. The hold keeps new work and the held backlog off it until a bona fide replacement is current or the protection expires, instead of flushing onto a socket the server is about to close; and the FailFast shield now covers any failure of a connection that is not a known, established replacement, which includes that one. The latch is taken only once a connection is established, so a still-connecting successor that fails still counts as the handoff's own. - The hold cover is bounded by maintRelaxedWindowMax, as the relaxation is, while always spanning the drain itself and one connect attempt. - When the write lock cannot be had for the whole of the drain's deadline, the target is retired without it as a last resort - what every recycle did before handoffs existed - rather than left to the server's window-end close, which would fail everything in flight to protect at most one wedged writer's message. - The guarded remote-endpoint read lives on PhysicalConnection; the inherited- protection flag is a local.
…less; the cover applies to held commands only Fifteenth round of review follow-ups: - Behind a Tunnel, or when the configured endpoint is itself a fixed address in front of the cluster, every connection reports the intermediary's address, so comparing it with the retiring one would make the genuine replacement look like a return to the retiring node forever. In those topologies the discriminator is temporal: a connection that is not the target and came up after the retire resolved is the replacement. - The hold's timeout cover applies to the commands the hold diverted (and to those it requeued), not to every command on the server: they carry a mark, and the backlog sweep, the in-flight sweep and the sync wait consult an overload that adds the cover for them alone. An application that has turned relaxation off keeps its short timeouts for everything else; a held command at the head of a sweep can delay, never lose, the one behind it. The cover still spans the drain and one connect attempt, since that is what the held commands need. - The diverted flag is set before the enqueue, so a drain pass cannot see a held message with the flag still off. - The socket's remote endpoint is cached on the connection once read. - The handoff target's expiry is a TickDeadline too; it was the last hand-rolled tick deadline. - The fence-pairing comments name WakeIfHoldReleased, which every held enqueue goes through, as the single enqueue-side site. - The steady-load test cancels its workers on any exit.
…over as a property of held commands, trust validated at use Sixteenth round of review follow-ups. The protection state machine's rules are now stated once at the top of the classification section, and the code is checked against them: - Classification has three outcomes: replacement, doomed, unknown. Unknown (an address not yet readable; or, behind a tunnel, time not yet telling) defers to the next pass and is never retired, never branded a target, and never trusted. - A successor pin is trusted only while it is still in force: the first bridge's replacement clears the pin, after which the other bridge's retry resolves the endpoint again and is classified by where it landed. - Behind a tunnel or a fixed front address, the temporal fallback also accepts "the drain has let go": after EndQuiesce nothing else will arrive, so what establishes is the replacement, rather than holding until the protection expires. - The timeout cover is a property of each held command for as long as it lives, no longer a window on the server, so it cannot be withdrawn between the replacement establishing and the held work being written to it; the FailFast shield likewise stands down only once the last held message has been flushed to the replacement. - A message that parks in a backlog that is holding (it passed the gate just before the hold rose) is held too, with the same cover. - The sync wait polls in one-second slices, so a relaxation or a cover that begins after a caller parked is noticed even when nothing pulses. - One teardown routine for the locked and lockless retire paths. The in-process test server can delay replies to one command only, so a test can slow the workload without pushing the replacement's own handshake past a short timeout; a new steady-load variant runs with relaxation off and a timeout shorter than a held command's wait.
…ection flags Rounds 13 to 17 of review each found a case where two of the handoff's flags disagreed. This replaces them with one object per handoff on each bridge, carrying a phase advanced in one direction by compare-exchange (Holding, AwaitingReplacement, Flushing, Done) and the facts fixed when it began (target, retiring address, whether an address identifies a node), plus one monotonic deadline shared by successive handoffs. Every handoff question - hold new work, flush the backlog, is this failure the handoff's own, is this enqueue covered - is answered from the phase. With that in place: - the hold cover is stamped on each held message as the handoff deadline (an absolute tick), so it cannot be withdrawn after the replacement establishes and does not ratchet across handoffs; the server-level cover and the stand-down-after-flush flag go; - everything entering the backlog during a handoff is held, in BacklogEnqueue alone, which also keeps the timeout sweeps' head-of-line rule sound without skipping; - a connection records that it was made under the successor pin, which is kept until the window expires rather than cleared when the first bridge establishes, so the second bridge's reconnect is not sent back to stale DNS; - a socket address identifies a node only when the handoff has itself seen a second address (a named successor, or DNS moving) and no tunnel is in the way; otherwise only time tells; - classification no longer latches a verdict on ordinary establishment: it is only consulted while a handoff is in progress; - sync waiters re-read their budget in slices only where maintenance notifications can change it; - TickDeadline keeps a passed deadline as the record of when it passed, replacing the hand-rolled relaxed-ended mirror, and owns the single nudge-from-zero helper.
…drain's bounds Round 18. The phase machine's transitions were driven only by the drain loop and by establishment, so one missed instant could wedge a phase until the deadline. Every heartbeat now re-judges the current connection against the handoff in progress: a bona fide replacement resolves the hold, and a reconnect that landed back on the node being retired is adopted (once, by compare-exchange on the target) and retired in its turn, which also covers the drain loop having ended on a transient null and an address that was unreadable at establishment. The retire is idempotent under the lock. Classification puts the address before the pin, so a connection dialled through a stale pin that still named the retiring node is doomed; the via-pin fact is recorded as the address dialled and matched against the successor of the handoff doing the judging; and a MOVING that names no successor drops any earlier pin. A new hold explicitly finishes an earlier handoff still flushing, so nothing is left mid-phase. A window-closed notification arriving mid-handoff can lengthen the relaxed window but not shorten it below what the drain extended; the drain cap is bounded by the relaxation ceiling it depends on (never below the floor); the head-of-line note on BacklogEnqueue states the requeue exception honestly; and the held message's cover is a TickDeadline, extended through the one ExtendTo that TryExtendBy now uses too.
…he last two wrap hazards Round 19. The handoff deadline moves onto the handoff object: each handoff stands for its own hold plus the rest of its window and one connect attempt, held work is stamped with that, and the deadline is read on every question put to a live handoff, so a passed one is noticed fresh and no stale residue is ever consulted by a later handoff. BeginHold publishes the new handoff before finishing the previous one, so there is no instant with no hold in force. The heartbeat's judgement now also retires the target itself when it is still current after the drain loop is over (the lock never came, or the retire faulted), not only an adopted reconnect; the retire outside the loop is one at a time per handoff and idempotent under the lock, so a stuck attempt is not piled on and a failed one is retried on the next beat. TickDeadline treats a deadline as in force only within one span of now, in both RemainingMilliseconds and ExtendTo, so a wrapped tick count can neither revive a long-passed deadline nor let it win against a fresh extension; the successor pin is read on every server heartbeat so its expiry is noticed while fresh. Sync waiters slice only where the server has ever accepted the opt-in, has a window open, or has a handoff in flight. The steady-load test helper's try body is indented.
…retire and stamp races, slicing only while a window is open Round 20. A successor at the very address being retired (a proxy restarted in place) no longer makes the address identify the node, and a connection dialled through this handoff's own pin is the replacement before the address is consulted - the stale-pin case the address-first order served is covered by matching the pin against the judging handoff's successor and by a no-successor MOVING dropping the pin. The written-queue sweep looks past a held head rather than stopping at it, as the backlog's ordering argument never covered the replacement's queue. BacklogEnqueue stamps from the same handoff that gated the message, and not for internal traffic. Marking a connection retired is a compare-exchange, so a lockless last-resort retire and a locked one tear it down once between them. The processor's in-hand message counts as queued until written, so a flush cannot be declared finished around it. The one-at-a-time guard on a MOVING covers the decision and the drain, not the warning wait, so a second MOVING inside the first window is acted on. Sync waiters slice only while a window is open or a handoff is in flight; an extension is honoured by the re-read after the wait regardless. TickDeadline.cs gets the repository's UTF-8 BOM.
…nd judgement needs evidence Round 21. HandoffAsync releases its one-at-a-time claim exactly once, by whichever path still holds it, so a second handoff's claim is not stomped by the first's finally. BeginHold stamps the work already queued when the hold begins, since the hold delays it as it delays diverted work; BacklogEnqueue re-reads the handoff after the enqueue, so the two sides pair and a concurrent enqueue is stamped by one of them. Sync waiters re-evaluate per wait whether their budget can shrink. The flush's termination check takes the backlog lock, under which the processor moves a message from the queue to its hand. A connection with no positive evidence either way is unknown, never a replacement by default. The drain's polls settle the no-retire cases without the write lock. The relaxation ceiling is stated once, in ClampToRelaxationCeiling.
…he phase, not the drain's claim Round 22. A reconnect that lands back on the retiring node while the held work is flushing puts the hold back in force (Flushing to AwaitingReplacement, before the connection reads as established) and is retired; the heartbeat judges a Flushing handoff too. A tunnel no longer records a successor dial that never happened. The drain's one-at-a-time claim keeps serialising the decision and the drain, but everything that asks whether a handoff exists - the relaxed window's shorten guard, the sync waiters' slicing - now asks the bridges' phases (HandoffLive); and a MOVING arriving while a drain is in flight updates the successor pin instead of being dropped. A diverted message whose hold has gone by the time it is enqueued is ordinary work rather than stamped from a dead handoff; the post-enqueue re-read rides on the total counter's interlocked increment, so a bridge that never saw a handoff pays one null read. A finished handoff lets go of its target. The docs state the cover's floor.
Collaborator
Author
|
Self-review notes, round 23 (head
Verdict: not this round — 1 is the sharpest (a held transaction is guaranteed-failed by its own rescue under short timeouts; the fix is one field in an existing inherit hook), 2 and 3 are the pin's last two races, 4 is narrow but a real wrong-error, 5–7 are hygiene. The reviewer's cleared list keeps growing; everything left is in the pin lifecycle and two stamping gaps. |
…pin is one generation-checked state Round 23. InheritWriteTime carries the parent's hold cover as well as its write time, so a held transaction's inner commands are not failed by the sweep on the replacement for a clock that includes the hold. An internal member of a held batch is covered with the batch, since the batch is held as a unit and the sweep must not fail one member of it. The pin - target, expiry, and the generation of the MOVING that set it - is kept consistent under the maintenance lock: a handoff's late decision cannot clear or overwrite a pin a later MOVING set, and the heartbeat's expiry check cannot null a pin set a moment ago. BacklogEnqueue loses its dead parameter and its stale contract. The docs and the drain comment say what a sub-second window does with the hold (it begins, for the instant of the replacement, because the retire is built on it). The drain-cap test's bound sits between the mechanism's worst case and the deadline it guards against.
Collaborator
Author
|
Round 23 addressed in 21ed775.
Verification: Release build clean with analyzers; full net10.0 suite 6879 total, 1 failed (the known
|
Collaborator
Author
|
Self-review notes, round 24 (head
Verdict: not this round. 2 and 3 are the sharp ones (a full-deadline stall on a healthy replacement; doomed-flush under round-robin DNS — the latter is new information, not a regression of a fix). 1, 4, 5, 6 are all back-to-back-MOVING seams: worth building a dedicated two-MOVINGs-in-one-window test matrix (second MOVING with/without successor × first handoff in decision/drain/flush), since this scenario class has produced findings in five consecutive rounds and each fix is point-tested at best. 7–8 are small. |
Collaborator
Author
Focusing note — scope, exit criteria, and what stops being a bugTwenty-four review rounds in, a step back before a twenty-fifth. The headline problem — a MOVING no longer fails commands at the window-end close — has been solved for some time: the core pipeline (gate/divert/retire/flush/sweep, the fence pairing, the lock discipline) has survived ten consecutive adversarial rounds untouched, and the single-handoff steady-load scenario has been finding-free since ~round 15. What the marginal round now finds is conjunctions of rarities (two MOVINGs in one window × pin expiry × a late DNS decision; round-robin A-records on the retiring node; 24.8-day tick wraps), and most of those findings are seams of the previous round's fixes — the loop is generating its own work, and each fix taxes the common path (fences, slicing, scans) to perfect a rare one. So, drawing the line: 1. The guarantee this PR ships (to be stated in
2. Exit criteria instead of more rounds:
3. One deliberate simplification to weigh before merging, not after: pure temporal classification — "the replacement is whatever fully establishes after the retire resolves" — would delete the endpoint-identity apparatus entirely (the retiring-address capture, the per-bridge comparison, the tunnel/VIP special-casing, the null-endpoint fallbacks), which has been the source of findings in roughly eight rounds. Cost: the hold releases slightly later in the one corner where address evidence would have exonerated a reconnect sooner. At this point less machinery beats more-correct machinery; if the matrix passes with the temporal rule alone, the identity code should go. Perf items (per-enqueue fence gating, the O(1) held counter, sync-slice gating) stay on the list but as a post-merge tidy-up PR — they're not release-blocking and each touches hot paths that deserve their own review. Net: fix three things, build one test matrix, decide one simplification, document the contract — then merge. No round 25 of the current shape. |
This branch has not been deployed
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
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.
Problem
The
MoveToandRecyclehandoff paths drained by waiting for "no caller work" before replacing the connections. Under steady traffic that never happens: there is always a command in flight, so the drain waited out the entire announced window and recycled at the deadline, failing whatever was in flight at that instant and leaving no time to establish the replacement.Measured live on a Redis Enterprise endpoint rebind (
singleproxy policy,maintMovingEndpointType=Auto) with about 450 ops/s on the connection:The timing matched the code to the tenth of a second: 15s window, 0.8s jitter,
DrainThenRecycleAsyncwaiting out the remaining 14.2s.What the handoff now does
Hold. When the drain starts, each bridge holds new caller work back from the connection about to be retired: single messages, batches, sync and async all go to the backlog instead of the socket, whatever the backlog policy, because the connection is live and the message would otherwise have been written to a socket the server is about to close. The backlog processor does not flush while the hold is unresolved. Internal traffic (keep-alives, probes) still goes to the old connection, and QUIT is never queued.
Drain. The drain waits only for caller commands already written to the connection, not for the backlog, so under load it completes within a round trip. The wait is capped at half the relaxed timeout, never more than half of what is left of the window and never more than
maintRelaxedWindowMax(the relaxation the drain depends on), with a one-second floor so that a relaxed timeout of zero (relaxation off) does not disable the drain. A completion notification for some other operation arriving mid-handoff can lengthen the relaxed window but not shorten it below what the drain extended. With under a second of window left there is no drain worth having: the connections are replaced at once, and the hold lasts only that instant. Past the cap the connection is replaced regardless, and whatever is still in flight fails as the server's close would have failed it. A blocking command that fits inside the cap completes; one that does not (a 12sBLPOPin a 5s cap) fails at the cap rather than at the window's end.Retire. The replacement happens under the bridge's write lock, and only to the connection the hold began on: a replacement established meanwhile is never touched. Under the lock only the marking happens (
RetiredForHandoff); the report and dispose follow once the lock is released, so writers are not stalled behind event handlers and socket shutdown. A writer that reaches the lock to find its connection retired requeues its message for the replacement, keeping its write time (the budget it has spent still counts); internal and backlog-bypassing messages are failed as having no connection. If the lock cannot be had at all inside the cap plus a one-second grace (a writer wedged on it), the connection is retired without the lock as a last resort, which is what every recycle did before handoffs existed. Both bridges are drained, each with its own loop and its own lock deadline, because the blast radius of aMOVINGis the node, not the connection that was told.One phase per handoff. Each bridge keeps one
Handoffobject per handoff: a phase advanced in one direction by compare-exchange (Holdingwhile the target is drained,AwaitingReplacementonce it is gone,Flushingwhile held work is written to the replacement,Done), the facts fixed when it began (the target, the retiring address, the successor if one was named, whether an address identifies a node), and its own deadline (the hold, the rest of the window and one connect attempt), read on every question put to it so a passed deadline is noticed while fresh. Whether new work is held, whether the backlog may flush, whether a failure is the handoff's own and whether an enqueue is covered are all answered from the phase; nothing about a connection is inferred from it, since what a connection is (the target, made under the successor pin, established) is recorded on the connection when it happens.Recognising the replacement. After the retire, whatever connection becomes current is classified as a bona fide replacement, doomed, or not yet classifiable:
A stale pin confers nothing: the pin is matched against the successor of the handoff doing the judging, and a
MOVINGthat names no successor drops any earlier pin before draining. The pin itself (target, expiry, and the generation of theMOVINGthat set it) is one state under the maintenance lock: a handoff's late decision cannot clear or overwrite a pin a laterMOVINGset, and the heartbeat's expiry check cannot null a fresh one. The judgement is not a one-shot: it runs at establishment, when the drain loop ends, and from every bridge heartbeat, so a reconnect that landed back on the retiring node after the drain loop ended is still adopted and retired (adoption is a compare-exchange, the retire outside the loop is one at a time and idempotent under the lock, and a failed attempt is retried on the next beat), the target itself is retired again if the loop's retire never got the lock or faulted, a connection whose address could not be read at establishment is judged again a second later, and a doomed landing while the held work is already flushing (the replacement dropped, stale DNS sent the reconnect back) puts the hold back in force before it is retired. Held work flushes to the replacement only once it is a known replacement; until then the hold stays in force.Protection and cover. While a handoff is not
Done, aFailFastbacklog policy does not abandon the held work for a failure that is the handoff's own: the retired connection, a doomed reconnect, or a replacement attempt that never completed its handshake. A replacement that was fully established and then failed is an ordinary failure, and the policy applies as usual. The handoff is done when the held work has been written to an established replacement, or at its deadline (the hold, the rest of the window and one connect attempt).Every caller command that enters the backlog during a handoff is held: diverted off the live connection, queued while the bridge reconnects, or parked behind held work being flushed (internal traffic such as keep-alives is not); so is caller work already queued when the hold begins, since the hold delays it the same way. A held command is stamped with the deadline of the handoff that held it, as an absolute tick, so it cannot time out before then for as long as it lives, whatever the relaxation setting and whatever happens to the handoff afterwards (a transaction's inner commands inherit it with the write time, and an internal member of a held batch is covered with the batch); and no longer than that, since a later handoff stamps its own deadline rather than inheriting an earlier, longer one. Only held commands get this; an ordinary command on the same server, including one issued by an application that has turned relaxation off and runs short timeouts, keeps its configured timeout. Because every held command in a handoff shares the deadline and ordinary commands can only be enqueued once the handoff is done, a later backlog message does not time out before an earlier one, so the backlog sweep's head-of-line rule holds (a requeued writer with an older write time is the documented exception: late by a bounded amount, never early); the in-flight sweep on the replacement looks past a held head rather than stopping at it, so ordinary commands written behind one keep their own timeouts. Sync callers re-read their budget after every wait, so a relaxation or cover that begins after they park is honoured; they wake in one-second slices only while the budget can shrink (a window is open, or a handoff is in flight), deciding that per wait, so a server with nothing in progress costs them nothing. Where relaxation is on, the relaxed window is also extended by the cap plus the relaxed timeout, clamped to
MaintenanceRelaxedWindowMax.Bounds. An announced window is capped at one hour before anything is derived from it. Every tick-count deadline in the maintenance code (relaxed window, successor pin, handoff deadline, held-command cover) goes through one
TickDeadlinehelper: zero means unset, extension is monotonic, a deadline is in force only within one span of now (so the residue of one that passed long ago can neither revive when the tick count wraps nor win against a fresh extension), and a passed deadline is otherwise kept as the record of when it passed. The successor pin is read on every server heartbeat so its expiry is noticed while fresh.Things it considers (and how)
FailFastbacklog policy: honoured for genuine failures, not for the handoff's own, for exactly the duration of the handoff.bypassBacklogor for internal traffic, failed as no-connection (InternalFailure: "Output pipe not initialized" when a write races connection teardown #3167's closure-vs-handoff distinction kept).MOVINGinside the first window: one decision-and-drain runs at a time per server (two drains on the same bridges would end each other's holds), but aMOVINGarriving while one is in flight is not dropped: a named successor updates the pin, and the drain in flight replaces the connections, which then dial it. "A handoff exists", for the relaxed window's shorten guard and the sync waiters' slicing, is the bridges' phase, not the drain's claim.ObjectDisposedExceptionas before.docs/ServerMaintenanceEvent.mddescribes the hold, the cap and the per-held-command cover.Tests
MovingUnderSteadyLoadRecyclesPromptlyAndFailsNothing: steady load against the in-process server, MOVING with a 10s window and a named successor; the recycle follows within a fraction of the window and nothing fails. Fails onmain(recycle at ~10s).MovingUnderSteadyLoadWithRelaxationDisabledStillDrains: relaxed timeout zero.MovingUnderSteadyLoadWithShortTimeoutsAndRelaxationOffKeepsHeldWork: 1s timeouts, relaxation off, slow server: held work is covered, nothing fails.MovingWaitsForSlowCommandsThatFitInsideTheDrainCap: a 3s command in flight completes before the recycle.MovingWithBlockingCommandsInFlightRecyclesAtTheDrainCap: a 12s command in flight; the recycle comes at the cap, not at the window's end.Maintenance*,*Backlog*,*Connect*,*Timeout*,Issue3167Tests,SanityCheckspass against the docker topology; full net10.0 suite 6879 total, 0 failed on the current head.FailoverTests.SubscriptionsSurvivePrimarySwitchAsyncis flaky on unmodifiedmainon this machine and is excluded from that judgement.MOVINGand no failed commands.Review history
Twenty-three rounds of self-review were addressed in the commits on this branch; actioned comments are removed to keep the PR readable. Round 17 asked for the protection flags to be collapsed into one explicit per-handoff state, which is the design described above; round 18 added the periodic re-judgement; round 19 moved the deadline onto the handoff, gave the retire a second chance, and closed the last tick-wrap hazards; round 20 was edges only (same-address successors, the in-flight sweep, enqueue stamping, retire and flush races, the second-
MOVINGguard); round 21 closed the seams those left (the guard's release, the pre-hold queue's cover, per-wait slicing, the locked flush check, no default verdict); round 22 made doomed mean doomed in every phase and split "a handoff exists" from the drain's one-at-a-time claim; round 23 carried the cover into transactions and held batches and made the pin one generation-checked state.