Repository navigation
[v0.21.x-branch] Backport #11329: peer: retire cooperative closers without stranding callers - #11349
Merged
Merged
Conversation
Author
|
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-11329-to-v0.21.x-branch
git worktree add --checkout .worktree/backport-11329-to-v0.21.x-branch backport-11329-to-v0.21.x-branch
cd .worktree/backport-11329-to-v0.21.x-branch
git reset --hard HEAD^
git cherry-pick -x 5e23f9ab357ec573f2ef0e990d7d7a59f3beaad4 79497bb567b2fde3eb4db318f6e51ff4f74a7f8c
git push --force-with-lease |
5 of 6 tasks
In this commit, we retire any active cooperative close state before removing a channel from the peer and switch, so a closer cannot outlive the channel and link that created it. We also require a clean channel before treating a missing link as proof that the channel has flushed, since link removal also happens on paths that do not retire the closer. The regression tests restore an interrupted legacy close in a new peer instance, drive a non-disconnecting link failure, and verify that the retired closer is no longer answered. (cherry picked from commit 9b7a982)
In this commit, we tell a locally initiated cooperative close why it will never finish when a link failure retires its channel. `WipeChannel` deletes the closer, and the closer is the only writer of the close request's error channel, so deleting it quietly left `CloseChannel` waiting on a request nobody was left to answer. With `LinkFailureForceNone`, the peer stays connected, so no disconnect cleans the request up for us. The notification runs in the failure path rather than in `WipeChannel`, so successful close finalization, which also wipes the channel, keeps its existing behavior. Only a closer we initiated owns a close request, so a responder or a restored closer has nothing to notify, and the error the caller receives is a stable value it can match on. Retiring the closer also clears the channel's closing flag, which the shutdown set and the legacy close consults. Leaving it set would make a later attempt to close the channel fail as already closing. The test starts a local legacy close, drives a non-disconnecting link failure, and asserts that the request receives the error before the closer is removed. Without the fix the assertion times out. (cherry picked from commit 5d6a97d)
In this commit, we make retiring a closer release everything that closer owns, for both variants, so a closer cannot outlive the channel that created it. `deleteActiveChanCloser` dropped the closer from the active map and unregistered its close actor, but an RBF closer is also a message-router endpoint and a running state machine, and neither was released. The endpoint stayed registered in the global, per-server router, so a co-op close message matched by channel ID was still delivered to a closer for a channel the peer had left behind: the newer closer's instance of the same stale-closer behavior. The state machine was never stopped either, so it kept a goroutine for the channel's remaining lifetime. Both releases run on their own goroutine, because this function can be reached from an RBF closer's own goroutine: a failed transition reports through the error reporter, which re-initializes the closer and retires it when that fails. The router answers endpoint queries from the same loop that delivers messages to endpoints, and stopping blocks until the machine's goroutine has exited, so blocking on either from that goroutine could wait on itself. Retirement also answers a caller that is still waiting. The legacy closer carries its own request, but the RBF one does not, so the caller is recorded when the RBF close starts. A caller is claimed with a load-and-delete before it is reported to, in every path that resolves a close, so exactly one of the paths racing to end a close reports to it and a close that finished cannot be reported as aborted by a retirement racing it. A caller that cancels, or a peer that shuts down, also gives up its record rather than leaving one for a later retirement to answer forever. The tests retire a real RBF closer built by the production constructor through a real router, and assert the caller is answered once, the endpoint is no longer routable, and the machine is no longer running. Each half fails its test when removed. A caller is tracked as a set per channel rather than as one record, because a channel can have more than one while an earlier one is still live: a fee bump for a close already in progress registers another. Taking a caller's record is the only way to resolve it, and a path acting for one caller takes that caller and no other, so an observer can never hand one caller the outcome belonging to another, and a retirement that ends the close on the channel's behalf answers every caller still waiting. (cherry picked from commit 66ffdd0)
In this commit, we tell every caller still waiting on a cooperative close that the close ended when its peer disconnects, and release the closers that peer owned. `Disconnect` retired nothing and answered nobody. The closers it owned were left in place, and for an RBF closer that means a registered router endpoint and a running state machine outliving the connection. A caller was also left with nothing to read: no outcome was coming on either channel, so `CloseChannel` parked until the daemon shut down. This is the disconnect case of the link-failure bookkeeping bug, and it affected the legacy closer too: a disconnect never answered the caller of a locally initiated legacy close. The closers are now walked on disconnect and each caller is answered before its closer is retired, so the retirement's own claim finds the caller already answered. This runs before the peer's context is cancelled, because answering a caller is a send on a channel that gives up once that context is done. `abortCloseRequest` also delivers its outcome directly when the caller's slot is free, rather than always handing the send to another goroutine. With the deferred send, the cancellation that follows immediately afterwards raced the send, and Go picks freely between two ready cases, so the caller could be left without its outcome. The slot is free in the normal case, so this both fixes that race and avoids a goroutine. The notification is renamed, `notifyChanCloserOfAbort`, and takes the reason, since what ends a close is not always a link failure. (cherry picked from commit 804a239)
In this commit, we give a cooperative close request a single outcome that every path can resolve at most once, and that no path can be held up delivering. The request carried its error over a channel with room for one, and the paths that end a close wrote to it directly. Two of those writes have no way out if the buffer is already full: they send without selecting on anything else, so once the caller has stopped reading they block where they stand. One of them runs on the channel manager's goroutine, so a block there stops every operation for that peer, not just the close. The preceding commit makes the disconnect path answer a waiting caller, adding a second writer to a request that previously had one, so the buffer may already hold an outcome when either send runs. The request now carries a promise instead of a channel. Completion is idempotent and reports whether this call was the one that resolved it, so a second path is refused rather than allowed to overwrite the outcome the caller already has. Completion cannot block, so a path that resolves a close is never held up by a caller that stopped reading. A close that reaches its close resolves the request too, once the caller has been told it succeeded. Otherwise the request is resolved only when a close fails or is aborted, and a caller that derived a channel from it has nothing to end that wait: with a context that is never cancelled, as the breach path uses, that wait would outlive everything it was waiting for. Callers that wait on a channel see no change: the promise is awaited on their behalf, and only a failure is forwarded, since a close that succeeds reports through its updates. The channel is left open rather than closed, because a closed channel reads as an immediate nil error and would cut a wait short before the close had finished. Retiring a closer no longer writes to the channel it is retiring. Clearing the channel's closing flag happens only where the goroutine that owns the channel does it, which the channel manager also does for its own channels as it drains, so no request carries the channel and no resolution touches one. The updates a close emits keep their untyped channel, because their terminal value carries a chancloser type and chancloser imports this package. Every send is guarded so that a caller that has gone away cannot make a close block delivering to it, and the buffer holds a pending update from each side of the negotiation as well as the terminal one. The failure and success of a close are reported by name. A close that failed takes the caller's own record before reporting, so a close that afterwards reached its close cannot tell that caller it succeeded: resolving a request refuses a second resolution, but the updates a close carries are a separate channel that nothing refuses. A close that reached its close reports to every caller still waiting, and the peer's shutdown no longer suppresses that report. The closing transaction has been broadcast, so it confirms whether or not the peer is still connected, and the callers' records are taken before anything is delivered, so no other path could answer them. The error a confirmation reports is reported through a callback rather than a channel, so there is no relay goroutine that a peer shutdown could stop before it forwards the error. A close refused because the counterparty breached the channel resolves its request, and the request that path starts is bound to the daemon's lifetime, so a request nothing resolves cannot outlive the daemon. (cherry picked from commit 5e23f9a)
In this commit, we pin the last contract the resolution of a close rests on and that nothing yet exercises: the order in which an observer reports and claims a caller. The preceding commit covers the rest, since a test of the channel a request derives needs the promise that commit introduces. The observer must take the caller's record before it reports, so that a closer which was already retired cannot tell a caller the close succeeded after the caller was told it had ended. The test drives a real RBF closer to its terminal state through production code and asserts both outcomes: an unclaimed caller is told the close succeeded, and a caller a retirement already answered is not told twice. Removing the claim from the report makes the second case fail, which is what the ordering is for. The closer is subscribed to before it is driven, since registering the subscription inside the observer goroutine races the event that drives it, so a state reached in between would be missed and the observer would wait for a transition that already happened. Two more paths are covered, both of which decide whether a caller is answered: a close that ends in a breach, and a caller whose record is given up while another caller on the channel is still waiting. The tests for a close reported after the peer shut down state what they rely on and the one schedule they do not catch. (cherry picked from commit 79497bb)
ziggie1984
force-pushed
the
backport-11329-to-v0.21.x-branch
branch
from
October 17, 2026 23:23
7cf79ac to
5a3e7d8
Compare
ziggie1984
marked this pull request as ready for review
October 17, 2026 23:24
github-actions
Bot
added
the
severity-critical
Author
🔴 PR Severity: CRITICAL
🔴 Critical (7 files)
🟢 Low (2 files)
AnalysisThis PR touches To override, add a |
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
2 participants
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.
Backport of #11329
Change Description
In this PR, we retire any active cooperative-close state whenever
WipeChannelremoves a channel from the peer and switch, and we make retiring a closer tell the caller who was waiting on it on every path that retires one.A locally initiated close whose closer was retired left its caller waiting forever.
WipeChanneldeletes the closer, and the closer is the only writer of a close request's error, so a close whose channel hit a non-disconnecting link failure leftCloseChannelwaiting on a request nobody was left to answer, parking until the daemon shut down. A peer disconnect did the same, for both closer variants. Retiring a closer now answers the caller, and the notification is parameterized by the reason, since what ends a close is not always a link failure.A close request's error was a one-slot channel with several writers. Two of those writes selected on nothing else, so once the caller had stopped reading they blocked where they stood, and one of them runs on the channel manager's goroutine, where a block stops every operation for that peer. Adding the disconnect notification introduced a second writer, so the full-buffer case can now occur.
The request now carries a promise rather than a channel. Resolving it is idempotent and reports whether this call was the one that resolved it, so a path arriving second is refused instead of overwriting the outcome the caller already has, and it cannot block, so a path resolving a close is never held up by a caller that has gone away. Callers that wait on channels see no change: the promise is awaited on their behalf and only a failure is forwarded, since a close that succeeds reports through its updates.
One design point worth stating, since it is what makes the multi-caller case correct rather than lucky: a channel can have more than one caller while an earlier one is still live, because a fee bump registers a second. It is the registry's mutex-protected removal, not the promise's idempotence, that partitions ownership of a caller's updates — the promise guards the error, and the exclusive take is what stops two paths from delivering to one caller.
We also require
IsChannelCleanbefore the legacy closer treats a missing link as evidence that the channel has flushed, since link removal also happens on paths that do not retire the closer.Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation