Repository navigation
peer: retire cooperative closers without stranding callers - #11329
Conversation
5e7b08b to
d51df9f
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (7 files)
🟢 Low (2 files)
AnalysisThis PR modifies To override, add a |
gijswijs
left a comment
gijswijs
left a comment
There was a problem hiding this comment.
Had a few minor/medium comments. Easily addressable afaict.
Concept ACK.
| "channel", req.ChanPoint) | ||
| p.WipeChannel(req.ChanPoint) | ||
|
|
||
| // The channel is gone, so the close is over as far as this |
There was a problem hiding this comment.
The CloseBreach arm resolves the caller with ErrCoopCloseAbortedByLinkFailure — a breach isn't a link failure, and the exported sentinel asserts a false cause. Harmless today (breach arbitrator discards the result), but any future consumer reads a security event as transient/retryable. A breach-specific sentinel, or a generic ErrCoopCloseAborted parameterized by reason, would age better.
| // Instruct the switch to close the channel. Provide no close out | ||
| // delivery script or target fee per kw because user input is not | ||
| // available when the remote peer closes the channel. | ||
| // The request this starts is resolved by whichever path |
There was a problem hiding this comment.
The comment says the ctx is "bound to the daemon's own lifetime", but defer cancel() fires when the closure returns — right after the CloseLink hand-off — so req.Ctx is dead-on-arrival. Downstream selects on closeReq.Done() (peer/brontide.go:5181, 5221) then race a permanently-ready case against a ready buffered send, dropping those updates ~50% of the time. Master passed context.Background() (never fired), so this is new nondeterminism. No live consumer today, but either drop the defer (bind to s.quit only) or fix the comment.
| chanPoint, ok := p.chanPointForCloser(chanID) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| p.deleteActiveChanCloser(chanID, chanPoint) |
There was a problem hiding this comment.
chanPointForCloser's doc says "A closer whose point cannot be recovered is still retired", but the caller does if !ok { continue } — skipping deleteActiveChanCloser entirely, which can leak an RBF state machine + router endpoint past Disconnect (permanently with a global msg router). Calling deleteActiveChanCloser with the zero OutPoint is safe (unregisterRbfCloseActor on a zero key unregisters nothing), so the continue can just be dropped.
| whenRPCShutdown(shutdown, func(req *htlcswitch.ChanClose) { | ||
| rpcShutdown = true | ||
|
|
||
| // Record the caller against the channel so that |
There was a problem hiding this comment.
startRbfChanCloser can silently strand the caller:
Two flavors: (a) the wrapper actor runs this on the actor's goroutine — Load succeeds, channelManager concurrently retires the closer, then add lands: an orphan registry record, the fee bump silently no-ops, and the RPC caller hangs until cancel; (b) if the loaded closer is legacy (Left), WhenRight no-ops, the function returns nil, and req is never resolved. Returning an error when the closer is missing or not Right would route both through the existing req.CompleteErr path in handleLocalCloseReq.
| // reading. A successful close is reported through Updates instead, so | ||
| // this is completed with a nil error only to end the request without | ||
| // an error. | ||
| Err actor.Promise[error] |
There was a problem hiding this comment.
htlcswitch is imported by external projects (tapd's tapchannel, loop, faraday vendor lnd); Err: chan error → actor.Promise[error] is a compile-time break for any downstream code touching the field. No action needed in-repo. Worth a release-notes entry so it doesn't surprise anyone at upgrade time.
|
I compared the full
The proposed removal deletes 158 lines from Coverage before and after is identical: The full Why these are removable:
Conclusion: these two tests and one subtest can be removed without losing statement or behavioral coverage. Separately, |
ziggie1984
added
backport-v0.21.x-branch
|
I found and reproduced an additional shutdown/liveness deadlock in The function takes all requests and then sends terminal success to them sequentially: for _, req := range p.activeCloseReqs.takeAll(chanID) {
select {
case req.Updates <- terminalUpdate:
case <-req.Done():
}
}If one caller's The impact extends beyond that caller:
I added an isolated deterministic regression test against current head This is a channel/backpressure liveness deadlock rather than a mutex deadlock. Simply adding |
d51df9f to
485e972
Compare
|
Addressed the review feedback in Bugs fixed
Tests
Local checks: |
485e972 to
2dc63ee
Compare
|
Pushed the review round. Six commits, tree unchanged in intent, and every commit now builds and passes What changed1. A caller whose closer is retired right after it is recorded is now answered by whoever wins, not by both. The recheck that runs once the caller is recorded now takes the record before it reports, so a close that reached its close and a retirement that ends it cannot both answer the same caller. 2. A 3. A breach answers a cooperative close already under way on the channel before the wipe retires its closer. The caller was previously told the wrong cause, or nothing at all. 4. A close that confirmed on chain is claimed through the request's promise before it is delivered, so a caller an abort has already answered is not afterwards also told the close succeeded. The promise is the one place a close is resolved, so winning it is what proves this path is the one answering; a readiness check followed by a send cannot decide that, since a retirement can land in between. 5. Two tests were rewritten so they can actually detect their bugs. The cancellation test registered its second caller too late to catch a release that took every caller on the channel; both callers are now registered before the cancel. The head-of-line test's blocked caller now carries a context that is cancelled in cleanup, so the delivery goroutine it starts exits rather than parking for the life of the test binary. 6. New covers: the breach case above, the abort-versus-confirmation ordering (both orderings, plus a caller that has stopped waiting), and the success-versus-retirement claim. Each of these is mutation-checked: removing the claim, widening the release, making the delivery sequential, dropping the breach notification, or reporting a success without claiming the request all make the relevant test fail. The series itselfThe first four commits are unchanged. Commits five and six were rebuilt so that every commit is internally consistent:
Verified independently, not just by the build: all six commits build, and all six pass One item is deliberately not in this change and is worth a follow-up: retiring a closer unregisters its router endpoint by name, which is a time-of-check/time-of-use race on a shared, per-server router — a reconnect that registers a replacement endpoint first can have its endpoint removed by the older retirement. Closing it properly needs an atomic compare-and-remove by endpoint identity in the shared message router, so it is not something to change as part of this review round. The default per-peer router is not exposed to it. |
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.
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.
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.
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.
2dc63ee to
1f9b500
Compare
|
Addressed the test-size point as well. The reduction is all shared setup. Nineteen close-request constructions and five message-router setups collapse into two small helpers ( On coverage, one measurement caveat worth stating plainly: the The nine mutations this branch relies on still behave as before — each fix's detector fails when that fix is removed, and the one counter-probe (a deliberately over-broad release under the old test ordering) still passes, which is what shows the rewrite is what makes the bug detectable rather than the test merely reporting it. Every one of the six commits builds and passes |
gijswijs
left a comment
gijswijs
left a comment
There was a problem hiding this comment.
All my comments were addressed. @Roasbeef went beyond and fixed things that I thought were such ultra-nits that I didn't raise them: The CurrentState hang is now surfaced via startErr, and the legacy confirmed-close path claims the promise through Complete's winner bool before delivering (claimAndReportLegacyCloseSuccess). This closes the "abort masks a confirmed close" race, using exactly the bool I noted was available but unused. The two weak tests from the first round were also rewritten, and the test file shrank 356 lines with no assertion loss. 🚢!
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.
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.
1f9b500 to
79497bb
Compare
|
Force-pushed to make the series internally consistent. The tree is byte-identical to what was already here — Two commits moved:
Why this matters, and how it was missed: the previous version had commit 6 repairing defects commit 5 introduced — a later commit fixing an earlier one. Every commit built and passed The check that does catch it, per adjacent pair in the series: If a later commit changes production behavior, the series is not internally consistent, regardless of builds and tests. On this series that diff is now empty for the last pair and non-empty only where a commit legitimately introduces new behavior: Verified at the tip: every one of the six commits builds and passes |
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
LGTM
Really nice improvement overall. I especially like the promise-based approach.
One remaining design caveat is that terminal completion is split between the promise and the Updates channel. These are independent objects, so the requirement to claim the request before publishing a success update is maintained by convention and the current helpers, rather than enforced by the type system. The implementation and tests handle this correctly, but future code will need to preserve that invariant.
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-11329-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-11329-to-v0.20.x-branch backport-11329-to-v0.20.x-branch
cd .worktree/backport-11329-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x 9b7a98284b2a7916b2cfdb43c2aac198fc5d2f38 5d6a97d5ac056a81d5ad689aab98bbe50b00ebc9 66ffdd053696ddb841cf1a6ac2fbab94d327ee37 804a2394285569a7bff31c79d720eaf1e853cb17 5e23f9ab357ec573f2ef0e990d7d7a59f3beaad4 79497bb567b2fde3eb4db318f6e51ff4f74a7f8c
git push --force-with-lease |
|
Created backport PR for
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 |
3 participants
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