multi: contain panics at fail-stop async boundaries - #11097
Conversation
1128734 to
c4f9ec2
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (3 files)
🟠 High (3 files)
🟡 Medium (7 files)
🟢 Low (9 files)
AnalysisThis PR touches several critical subsystems directly: The change is also broad: excluding test files, ~13 non-test files and ~896 lines are modified, which exceeds the 500-line threshold for a severity bump on its own. It also touches Given the surface area (peer connection handling, HTLC link logic, and channel funding all changed together) this warrants careful expert review, particularly around message ordering/concurrency guarantees in the new actor/message-router plumbing and its integration into To override, add a |
ae9fd3f to
c47271f
Compare
c47271f to
97fefb5
Compare
ziggie1984
added
backport-v0.20.x-branch
278c3c5 to
4080b75
Compare
gijswijs
left a comment
gijswijs
left a comment
There was a problem hiding this comment.
Made some remarks throughout. Concept is ack, but it needs some hardening, and wider implementation.
Consider renaming fn/stack.go to fn/panic.go (and fn/panic_test.go)
|
Can we make panic recovery opt-in, i.e. adding a flag to explicitly enable it, please? There are two potential situations resulting in panics and we want recovery only in the first case:
Given we don't know what is the bug (if we knew, we would fix it), can we keep panic recovery behind a flag? Is there is a bug of (1) type and it is exploited in the wild, users can just restart the node with the panic recovery flag. But if (1) does not happen (most of the time), then we do not enable attacks of type (2). |
4080b75 to
79d651d
Compare
ziggie1984
changed the title
f35070c to
311c659
Compare
|
@starius Thanks, this is a fair concern. We took a different approach from making recovery configurable. We narrowed the PR to recovery boundaries where the affected state is either fully retired or the operation can safely stop and be retried. We removed recovery from stateful workflows that could continue after an unknown subset of side effects. We prefer this over a flag because configuration cannot make an unsafe recovery boundary safe, and operators may not know that they need to enable it before a vulnerability is exploited. We also added fault-injection and race tests covering teardown, lock release, and goroutine cleanup. Does this narrower boundary-based approach address your concern? |
d5bdd81 to
5278a3f
Compare
Add RecoverPanic for direct use at selected deferred recovery boundaries. Preserve the original panic value, capture an 8 KiB bounded stack trace, and invoke a caller-provided callback so each subsystem controls its own cleanup. Contain callback panics and emit a best-effort fallback report. Add structured logging helpers with consistent panic type, value, stack, and caller-provided attributes. Provide a debug-stack variant for remotely triggerable paths and prevent logging failures from interrupting cleanup. Add a custom analyzer that requires RecoverPanic to be deferred directly. This is necessary because recover only works from the deferred frame. Test recovery, ordinary returns, named-return updates, stack truncation, structured attributes, raw panic values, callback failures, logging failures, and analyzer diagnostics.
Contain panics raised while the read loop reads or dispatches an incoming peer message. Report the panic and disconnect the affected peer through the normal cleanup path. Keep discovery-stream ownership in an outer frame and place recovery in the inner message loop. Recovery therefore closes the peer quit signal before stream shutdown waits for its consumer, allowing blocked gossip delivery to exit. Stop the idle timer when the handler returns. Construct the disconnect error from the panic type rather than formatting its value. This prevents a broken String or Error method from interrupting peer teardown. Add regression tests for an unsafe panic-value String method and a discovery consumer blocked on peer shutdown. Verify that the peer disconnects, stream shutdown completes, and all peer goroutines exit.
Execute each submitted task through runTask. If a task panics, report the panic, deliver an error wrapping ErrWorkerTaskPanic exactly once, and tell the caller to retire the affected worker instead of reusing its state. Let the worker's existing defers clean up its state, wait-group entry, and semaphore slot so the pool remains available for later work. Add a POOL subsystem logger for structured panic reports. Test panics in newly spawned and reused workers, unsafe panic-value formatting, and full pool saturation after recovery to prove that worker slots are not leaked.
Add recovery boundaries to the legacy close-message and channel-flush entry points owned by channelManager. Report the channel ID and route a recovered panic through the ordinary close error handler at most once. The error path resets the in-memory close state, removes the active closer, fails a local close request, and disconnects the peer. If recovery happens before that handler is available, or the handler itself panics, contain the secondary failure and still disconnect the peer. Keep persisted ChanStatusCoopBroadcasted state intact so an already broadcast closing transaction remains recoverable after restart. Validate the final closing transaction before removing live channel state. An unfinished closer now reports its state error and returns without dereferencing a nil transaction, wiping the channel, or deleting its active closer. Make local failure notification best effort. Remove the active closer before the non-blocking error send so a full or closed caller channel cannot prevent mandatory cleanup and peer teardown. Document the bounded-channel assumption that the legacy pending update still relies on. Add tests for both recovery entry points, secondary failure reporting, full and closed error channels, closer cleanup, peer goroutine exit, release of the affected channel mutex, and the unfinished finalization path.
Move the existing gossiper and RPC panic reports to the shared structured logging and bounded-stack helpers without adding or moving recovery boundaries. Keep remotely triggerable gossip stacks at debug level to limit error-log amplification. Continue reporting RPC stacks at error level with the RPC method as structured context. Remove the duplicate rpcperms stack truncation code and retain its coverage in the shared fn tests.
Send the final cooperative-close update through a helper that selects on the client update channel, request cancellation, and peer shutdown. This prevents a full bounded update channel from blocking close-observer teardown. Continue to remove the active channel closer after the helper returns, whether the update was delivered or teardown cancelled the send. Add regression tests for a full update channel followed by request cancellation or peer shutdown.
Contain panics in the periodic zombie-reservation sweep so one malformed reservation does not terminate the funding coordinator. Report the failed operation and return to the coordinator loop so a later tick can retry. Move reservation discovery into findZombieReservations and defer release of resMtx. Replace the split lock check and unlocked timestamp read with an isExpired method that reads lastUpdated under updateMtx. Preserve the existing PSBT reservation exclusion. Add tests that trigger a malformed-reservation panic, verify that resMtx is released, and exercise concurrent timestamp updates and expiry checks under the race detector.
Document the selected panic-containment boundaries in the 0.20.4 and 0.21.3 release notes. Explain that recovery is limited to execution units that can be retired safely, including peers, pool workers, legacy close negotiations, and periodic zombie-reservation sweeps. Mention the shared bounded-stack reporting used by recovered failures.
5278a3f to
e7edf4c
Compare
4 participants
Change Description
This PR introduces shared helpers for recovering from and reporting unexpected
panics at selected asynchronous boundaries. Recovery is enabled only where an
arbitrary panic can be contained without continuing to use uncertain mutable
state.
The goal is not to make a panicking component healthy. The goal is to reduce
the failure scope while preserving fail-stop behavior for the affected peer,
worker, or protocol instance.
Safe Recovery Boundary Definition
A recovery boundary is considered safe when, after recovery, no code continues
processing with state that may have been partially mutated before the panic.
The boundary must satisfy one of these models:
retired or quarantined. It does not process another event.
effects remain valid, and unfinished work stays discoverable for a later
clean retry. No partially advanced protocol state machine is resumed.
Attacker reachability is evaluated separately. It determines how important a
boundary may be for denial-of-service protection, but it does not prove that
recovering there is safe. A highly exposed state machine is still excluded if
its partial effects cannot be rolled back or quarantined.
Boundary Evaluation Requirements
Every candidate site was evaluated individually against the same requirements:
that returns after recovery.
messages, broadcasts, subscriptions, and launched goroutines that may have
completed before the panic.
is independently retryable without resuming a partial protocol transition.
ensure panic unwinding releases ownership correctly.
wait for a signal that only a later cleanup step can produce. Any remaining
producer and buffer assumptions must be explicit.
effect such as broadcasting a cooperative-close transaction.
reacquisition checks, and race-detector coverage.
The analysis assumes a panic can occur at any instruction, including after an
external effect but before the corresponding in-memory bookkeeping completes.
Recovery Boundaries Retained in This PR
peer.readHandlerpool.Worker.runTaskpeer.handleCloseMsgChanStatusCoopBroadcastedstate is retained so an already broadcast transaction remains recoverable after restart.peer.handleChanFlushedchannelManagerand is not resumed after recovery.funding.pruneZombieReservationsThe legacy close path also validates that a final closing transaction exists
before removing live channel state. Its internal impossible-message assertion
is intentionally retained. If that invariant is violated, recovery reports the
panic and scopes the failure to the affected close and peer instead of
terminating the process.
Deliberately Excluded Boundaries
This PR excludes recovery where the same stateful component could continue
after an unknown subset of its effects completed. This includes actor mailbox
delivery, message-router endpoints, protocol state machines, the HTLC manager,
remote and local funding workflows, late funding messages, asynchronous
ChannelReadyprocessing, and local close initiation.Those paths can persist state, consume mappings, reserve wallet inputs, create
signer state, send messages, register subscriptions, install channels, or
broadcast transactions before panicking. They require path-specific rollback,
quarantine, or transaction-like ownership before recovery can be enabled
safely. They will be handled in focused follow-up changes.
This boundary-specific policy remains enabled by default. It does not require
node operators to anticipate an unknown remotely triggerable crash or opt in to
protection before a vulnerability is disclosed.
Additional Changes
migrated to the shared helpers.
cannot hold observer teardown open.
fn.RecoverPaniccall to be deferreddirectly so Go recovery semantics cannot be bypassed accidentally.
Validation
fn, linter, discovery, funding, peer, pool, and RPC permissiontests pass.
-race -count=20.reacquisition, funding reservation lock release, and synchronized reservation
timestamp access.