Skip to content

Fix task-slot collision when pathfinding scores sync is enabled#986

Open
jkczyz wants to merge 1 commit into
lightningdevkit:mainfrom
jkczyz:2026-07-scores-sync-task-collision
Open

Fix task-slot collision when pathfinding scores sync is enabled#986
jkczyz wants to merge 1 commit into
lightningdevkit:mainfrom
jkczyz:2026-07-scores-sync-task-collision

Conversation

@jkczyz

@jkczyz jkczyz commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

The pathfinding scores sync task claimed the runtime's single background-processor slot, which LDK's background processor also claims later during startup. With a scores sync URL configured, node startup tripped a debug_assert in debug builds; in release builds the second spawn silently dropped the scores-sync task's join handle, so the task was detached and never joined at shutdown.

Spawn it as a regular background task instead: it already exits on the general stop signal, so it is now joined gracefully alongside the other background tasks in Node::stop.

Developed with assistance from Claude Code.

The pathfinding scores sync task claimed the runtime's single
background-processor slot, which LDK's background processor also claims
later during startup. With a scores sync URL configured, node startup
tripped a debug_assert in debug builds; in release builds the second
spawn silently dropped the scores-sync task's join handle, so the task
was detached and never joined at shutdown.

Spawn it as a regular background task instead: it already exits on the
general stop signal, so it is now joined gracefully alongside the other
background tasks in Node::stop.

Developed with assistance from Claude Code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ldk-reviews-bot

ldk-reviews-bot commented Jul 15, 2026

Copy link
Copy Markdown

👋 I see @joostjager was un-assigned.
If you'd like another reviewer assignment, please click here.

@jkczyz jkczyz requested a review from joostjager July 15, 2026 02:20

@tnull tnull left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, good catch. :(

Comment thread src/scoring.rs
let logger = Arc::clone(&logger);

runtime.spawn_background_processor_task(async move {
runtime.spawn_background_task(async move {

@tnull tnull Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should even use spawn_cancellable_background_task here, as we don't want to wait for this to finish on shutdown.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We write both the the scores and the node metrics separately in that task. So the latest_pathfinding_scores_sync_timestamp metric may not be in sync if the task is cancelled between the writes. Maybe doesn't really matter if we'll sync on startup?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is fine. We can't avoid the general issue in case of a panic, and worst case the timestamp in NodeMetrics will be stale, then updated on next node startup.

Comment thread tests/integration_tests_rust.rs
@joostjager joostjager removed their request for review July 15, 2026 09:40
@joostjager

Copy link
Copy Markdown
Contributor

Pls re-request review when ready again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants