Release completed cancellable tasks by tnull · Pull Request #997 · lightningdevkit/ldk-node

tnull · 2026-07-20T08:49:26Z

Avoid retaining completed Tokio task allocations for the node lifetime while preserving shutdown cancellation and restart semantics. This fixes a considerable memory leak as previously we'd retain the JoinSeted task's allocations until stop/join_next, as tokio thankfully only notes in the https://docs.rs/tokio-util/latest/tokio_util/task/task_tracker/struct.TaskTracker.html#comparison-to-joinset.

Thanks to @elnosh for reporting this leak.

ldk-reviews-bot · 2026-07-20T08:49:29Z

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull · 2026-07-21T09:05:33Z

Rebased after #956 landed.

elnosh

ACK fix in 08d3b5d

I'm less familiar with the code in some of the other commits, specially in liquidity. Some of the changes implementing the different Guards took me quite a bit to understand so I would've appreciated some comments but they look correct. Specifically:

reason for PendingRequestGuard needing the tokens for the pending requests in liquidity/mod.rs
PendingRequest in liquidity separating the owner Sender from the other followers Vec<oneshot::Sender<T>>. In connection.rs there's just one vec for all the senders.

These make sense now but not particularly obvious so adding comments is just a suggestion, I'm fine with it as-is

elnosh · 2026-07-21T18:13:26Z

similar to fb5fbc8 this debug_assert could be removed as receiver from this pending connection attempt might have been dropped.

tankyleo

So far just looked at the first commit, will continue tomorrow

tankyleo · 2026-07-23T02:01:33Z

-	tasks: JoinSet<()>,
+	tasks: TaskTracker,
+	cancellation_token: CancellationToken,
 	accepting_tasks: bool,


We can delete this field here, and track is_closed on TaskTracker instead

tankyleo · 2026-07-23T02:06:27Z

+		let mut cancellable_background_tasks =
+			self.cancellable_background_tasks.lock().expect("lock");
+		if cancellable_background_tasks.cancellation_token.is_cancelled() {
+			debug_assert!(


I believe this debug_assert is reachable here: further below we cancel the token, then drop the lock, then wait. So here the token could be canceled, but the tasks not yet empty.

Hmm, it's def. not reachable from the current callsites as they are serialized.

Agreed the current callsites are serialized. Might be good to not rely on this external lock for serialization, I vibed with codex on this issue, here's the patch it came up with below. Your call if you want to incorporate this or leave the PR as is :)

Isolate cancellable task generations Replace cancelled cancellable-task state as a unit so a restart cannot reopen the tracker being drained by an in-flight abort. AI-Assisted-By: OpenAI Codex diff --git a/src/runtime.rs b/src/runtime.rs index d4ea7ce..3fe04d1 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -146,14 +146,10 @@ impl Runtime { let mut cancellable_background_tasks = self.cancellable_background_tasks.lock().expect("lock"); if cancellable_background_tasks.cancellation_token.is_cancelled() { - debug_assert!( - cancellable_background_tasks.tasks.is_empty(), - "Expected all cancellable background tasks to be stopped" - ); - cancellable_background_tasks.cancellation_token = CancellationToken::new(); + // An abort may still be waiting on a clone of the previous task tracker. Start a new + // generation instead of reopening that tracker underneath the in-flight wait. + *cancellable_background_tasks = CancellableBackgroundTasks::new(); } - cancellable_background_tasks.tasks.reopen(); - cancellable_background_tasks.accepting_tasks = true; } pub fn spawn_background_processor_task<F>(&self, future: F)

tankyleo · 2026-07-23T02:14:27Z

-		debug_assert!(tasks.len() > 0, "Expected some cancellable background_tasks");
-		tasks.abort_all();
-		self.block_on(async { while let Some(_) = tasks.join_next().await {} })
+		self.block_on(tasks.wait())


IIRC while we are waiting on tasks.wait, we could call allow_cancellable_background_task_spawns, reopen the TaskTracker, and thus tasks.wait() would never finish.

Hmm, yes, if we only look at Runtime alone, but note that we are always acquiring the is_running lock in start/stop. Let me know if you think it's crucial to make Runtime more robust by itself.

let me know what you think of the patch above, it would address this here too.

tankyleo · 2026-07-24T18:53:09Z

+		Self { pending_connections, node_id, active: true }
+	}
+
+	fn disarm(&mut self) {


nit: Is this really needed ? On drop, we'd briefly lock the pending connections, see that we have no subscribers for the node_id, and do nothing.

Yes. Although result propagation normally removes the entry, another thread could register a replacement after the mutex is released but before this guard drops. Disarming prevents the old guard from removing that new attempt.

tankyleo · 2026-07-24T18:55:32Z

+		drop(connection_guard);
+
+		assert!(
+			pending_connections.lock().expect("lock").is_empty(),


nit: Should we also make sure that key-values for a different node_id are not dropped ?

I’d prefer to leave this out since removal is explicitly scoped by node_id, and the existing test covers the relevant cancellation behavior.

tankyleo · 2026-07-24T19:04:42Z

+		drop(request_guard);
+
+		assert!(
+			pending_requests.lock().expect("lock").is_empty(),


Similar here perhaps some coverage to make sure we don't drop requests with a different request key.

See above, would also prefer to leave it out for now.

Refills ran on the joined background task set, which shutdown waits on — a refill wedged on an unresponsive store holds up every stop for the per-task timeout. Spawn them on the cancellable set instead, which the node aborts at shutdown. Completed tasks still accumulate there until shutdown; the runtime rework in lightningdevkit#997 is what reaps them continuously. Aborting a task mid-refill must not lose wallet state: the refill used to hold the taken change set in a local across its store writes, so an abort landing there dropped reveals that were already taken from the wallet's staged state — a later refill would then publish addresses no persisted wallet state covers, recreating the unwatched-script problem the pool exists to prevent. The refill now stages the taken change set with the persister in the same critical section that takes it from the wallet, so an abort at any await leaves the reveals pending for the next persist call to flush. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Avoid retaining completed Tokio task allocations for the node lifetime while preserving shutdown cancellation and restart semantics. Co-Authored-By: HAL 9000

Check the tracker directly instead of duplicating spawn state. This keeps task admission aligned with shutdown tracking. Co-Authored-By: HAL 9000

Replace closed task state as a unit so a restart cannot reopen the tracker being drained by an in-flight shutdown. Co-Authored-By: HAL 9000

Signal cancellation when cancellable task state is dropped so detached tasks cannot outlive a node that fails during startup. Co-Authored-By: HAL 9000

Remove pending LSPS request state when callers time out or are cancelled so unresponsive services cannot grow request maps. Co-Authored-By: HAL 9000

Treat responses for expired requests as expected so delayed LSP messages do not panic the liquidity event loop. Co-Authored-By: HAL 9000

Clear per-peer connection state when the leading task is cancelled so later callers can retry and existing subscribers do not hang. Co-Authored-By: HAL 9000

Connection subscribers may cancel before a shared attempt finishes. Treat the failed send as expected instead of panicking in debug builds. Co-Authored-By: HAL 9000

Restore wallet sync status and notify waiting callers when the task performing a sync is cancelled, allowing later sync attempts to run. Co-Authored-By: HAL 9000

Wait for an active manual sync and retry initial ownership so startup races do not permanently stop background polling. Co-Authored-By: HAL 9000

tnull · 2026-08-10T11:23:57Z

Now updated to address pending feedback. This is now great quite a bit in scope, which I'm not super happy with. But let me know what you think.

Jolah1 · 2026-08-10T17:00:01Z

Reviewed runtime.rs, connection.rs, chain/mod.rs, liquidity/mod.rs and both LSPS clients - looks correct to me.

One thing worth a second look: PendingRequest backs six maps, but only pending_lsps0_discovery ever touches followers (pushed at liquidity/mod.rs:477, drained at :432). The identifier doesn't appear in lsps1.rs or lsps2.rs at
all — their five handlers do request.sender.send(..) and drop request.followers. Harmless today, since each LSPS1/LSPS2 request has a unique LSPSRequestId and nothing pushes followers there. But if someone later dedups

e.g.check_order_status, those followers would never be notified and it'd surface as an LSP timeout rather than a local bug. Maybe drain them in those handlers too, or split the type.

On the beta failure: it died at tests/common/mod.rs:865 in premine_blocks, where exponential_backoff_poll allows only ~9.2s (20 tries, 512ms cap) for bitcoind to generate and electrs to index 101 blocks. build (self-hosted, 1.85.0) passed on the same runner, so it's probably worth a re-run before digging.

ldk-reviews-bot requested a review from tankyleo July 20, 2026 09:00

tnull mentioned this pull request Jul 20, 2026

Follow-up to #792 (multi-LSP support) #956

Merged

tnull force-pushed the 2026-07-cancel-task-leaks branch from 3553a89 to fb5fbc8 Compare July 21, 2026 09:05

elnosh reviewed Jul 21, 2026

View reviewed changes

tankyleo reviewed Jul 23, 2026

View reviewed changes

tnull added this to the 0.8 milestone Jul 24, 2026

tankyleo reviewed Jul 24, 2026

View reviewed changes

ovitrif mentioned this pull request Jul 27, 2026

SIGABRT: owned tokio Runtime dropped from its own worker thread during chain sync teardown synonymdev/ldk-node#94

Closed

tnull mentioned this pull request Aug 7, 2026

Derive shutdown scripts without blocking on wallet persistence #1011

Open

tnull force-pushed the 2026-07-cancel-task-leaks branch from fb5fbc8 to a4461ac Compare August 10, 2026 11:11

tnull added 10 commits August 10, 2026 13:13

Release completed cancellable tasks

5e7e8b8

Avoid retaining completed Tokio task allocations for the node lifetime while preserving shutdown cancellation and restart semantics. Co-Authored-By: HAL 9000

f Use task tracker closure state

553eeac

Check the tracker directly instead of duplicating spawn state. This keeps task admission aligned with shutdown tracking. Co-Authored-By: HAL 9000

f Isolate cancellable task generations

595ce12

Replace closed task state as a unit so a restart cannot reopen the tracker being drained by an in-flight shutdown. Co-Authored-By: HAL 9000

f Cancel detached tasks on runtime drop

2709fea

Signal cancellation when cancellable task state is dropped so detached tasks cannot outlive a node that fails during startup. Co-Authored-By: HAL 9000

Clean up abandoned liquidity requests

48beb4f

Remove pending LSPS request state when callers time out or are cancelled so unresponsive services cannot grow request maps. Co-Authored-By: HAL 9000

f Tolerate late liquidity responses

9c1dbd2

Treat responses for expired requests as expected so delayed LSP messages do not panic the liquidity event loop. Co-Authored-By: HAL 9000

Clean up cancelled connection attempts

aeaec6d

Clear per-peer connection state when the leading task is cancelled so later callers can retry and existing subscribers do not hang. Co-Authored-By: HAL 9000

f Avoid panics for cancelled subscribers

05a6c5a

Connection subscribers may cancel before a shared attempt finishes. Treat the failed send as expected instead of panicking in debug builds. Co-Authored-By: HAL 9000

Reset cancelled wallet sync state

7d60fd5

Restore wallet sync status and notify waiting callers when the task performing a sync is cancelled, allowing later sync attempts to run. Co-Authored-By: HAL 9000

f Keep bitcoind background sync alive

2c1682e

Wait for an active manual sync and retry initial ownership so startup races do not permanently stop background polling. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-07-cancel-task-leaks branch from a4461ac to 2c1682e Compare August 10, 2026 11:17

tnull requested a review from tankyleo August 10, 2026 11:21

Uh oh!

Conversation

tnull commented Jul 20, 2026

Uh oh!

ldk-reviews-bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Jul 21, 2026

Uh oh!

elnosh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Aug 10, 2026

Uh oh!

Jolah1 commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldk-reviews-bot commented Jul 20, 2026 •

edited

Loading

tankyleo Jul 27, 2026 •

edited

Loading