Wind down spawned tasks when startup fails - #1086
Conversation
|
I've assigned @tnull as a reviewer! |
|
The test covers only wallet sync and pathfinding scores but never RGS. Worth adding, or at least noting why it's left out, Overall good work though. |
`Node::start` spawns background tasks - wallet sync, RGS gossip, pathfinding scores - before it can still fail, e.g. when resolving or binding the configured listening addresses. Until now the error path only stopped the chain source, leaving those tasks running behind a node that never came up, and leaving the node in a state a subsequent `start` could not cleanly recover from. Extract the wind-down sequence from `Node::stop` into a `Node::shutdown` helper and run it on any `start_inner` error. As the helper now also runs after a partial startup, it can no longer assume that every task exists: the two shutdown `watch::Sender::send` calls are allowed to find no receivers, and the `debug_assert!`s in `Runtime::wait_on_background_tasks` and `Runtime::wait_on_background_processor_task` that required a fully-started node are dropped in favour of doc comments spelling out that case. Fixes lightningdevkit#1009. This change was written with the assistance of Claude Code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SUGgjhnpkCuFjsE3BjYAEx Claude-Session: https://claude.ai/code/session_01JRikhHBQigjpBY1FAc1yBr
88caa46 to
6d702c3
Compare
Good catch, thanks! Looking into it turned up a second gap: the test had background sync disabled, so the wallet-sync task exited immediately and there was nothing left for the wind-down to stop. Fixed now: background sync stays enabled, and the test asserts that the wallet-sync task logged its stop line. |
Fixes #1009.
Node::startspawns background tasks — wallet sync, RGS gossip, pathfinding scores — before it can still fail, most visibly when resolving or binding the configured listening addresses. The error arm only calledchain_source.stop(), so those tasks kept running behind a node that never came up.This extracts the wind-down sequence out of
Node::stopinto a privateNode::shutdownand runs it on anystart_innererror.Reviewer note — relaxed assertions. Because the wind-down now also runs after a partial startup, it can no longer assume every task exists:
watch::Sender::sendcalls are allowed to find no receivers (previouslylog_error!+debug_assert!(false)).Runtime::wait_on_background_tasks'debug_assert!(tasks.len() > 0)andRuntime::wait_on_background_processor_task'debug_assert!(false, "Expected a background processing task")are dropped, with doc comments in their place explaining that the empty case is now reachable by design.I considered keeping the assertions by threading an
expect_running: boolthroughshutdown(), but that trades a real invariant for a parameter the caller can get wrong, and the assertions only ever restated "we got here viastop". Happy to go the other way if you'd rather keep them.Testing
tests/integration_tests_rust.rs::failed_start_winds_down_background_tasks— squats one of the node's listening addresses sobindfails after the wallet-sync, RGS and pathfinding-scores tasks have been spawned, asserts via aCollectingLogWriterthat the full shutdown sequence ran, then frees the address and confirms the node starts and stops cleanly.src/runtime.rs::winding_down_without_spawned_tasks_is_a_noop— the runtime wind-down calls tolerate finding nothing to wait on.This PR was written with the assistance of Claude Code.