Skip to content

fix: prune dead stratum workers instead of unbounded growth - #3901

Open
iho wants to merge 1 commit into
mimblewimble:stagingfrom
iho:fix/stratum-prune-dead-workers
Open

fix: prune dead stratum workers instead of unbounded growth#3901
iho wants to merge 1 commit into
mimblewimble:stagingfrom
iho:fix/stratum-prune-dead-workers

Conversation

@iho

@iho iho commented Jul 9, 2026

Copy link
Copy Markdown

Summary

Stratum worker_stats was append-only: every reconnect left a permanent disconnected entry. With rigs cycling connections the TUI mining table (and mining status API) grew without bound until node restart.

Closes #2413

Changes

  • Slot reuse: new workers fill the first disconnected slot (stats reset for that slot)
  • Idle clear: when the last live worker disconnects, drop all historical worker stats
  • Auto-prune: if more than 32 dead slots remain, compact and remap live worker IDs
  • prune_dead_workers(): explicit compact/remap helper (returns removed count)
  • Unit tests for reuse, idle clear, and remapping

Live worker IDs only change during prune/remap (under write locks). Day-to-day operation uses slot reuse without remapping.

Test plan

  • cargo test -p grin_servers --lib stratumserver::tests

Worker stats slots were append-only, so reconnect churn left a long list
of disconnected workers in the TUI/API. Reuse disconnected slots for new
connections, clear stats when the last worker leaves, auto-compact after
more than 32 dead slots, and expose prune_dead_workers for explicit cleanup.

Closes mimblewimble#2413

@wiesche89 wiesche89 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This addresses the growth problem behind #2413 automatically, rather than adding the requested manual prune action, and the separate stats reset request remains out of scope. Could the PR description clarify that distinction and use Refs #2413 unless the automatic replacement is considered sufficient to close the issue?


/// Remove disconnected worker stats and reassign live worker IDs densely.
///
/// Safe to call while workers are connected: live workers are remapped

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think remapping live workers is safe here. Each connection task captures its worker_id at accept time and keeps using it for RPCs, replies, stats, and disconnect. After a remap that ID can point to another worker or outside the stats vector. Could we keep connection IDs stable and add a test that continues using the original ID after cleanup?

// does not grow without bound under reconnect churn (#2413).
let worker_id = stratum_stats
.worker_stats
.iter()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove_worker() marks the stats disconnected before removing the worker from the map. A concurrent accept can reuse and replace that slot in between, after which the old disconnect removes the new connection. Could we make removal and the disconnected-state update atomic under the same locks?

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.

2 participants