feat: keep node recovery task running across epoch changes#3516
feat: keep node recovery task running across epoch changes#3516halfprice wants to merge 2 commits into
Conversation
When an epoch change starts while node recovery is in progress, the recovery task was canceled and restarted from scratch, re-scanning the full blob info table on every epoch change even though the previous progress was still valid. The recovery task now keeps running across epoch changes: - The epoch-change path only advances the recovery target (RecoveryInProgress epoch) and starts shard syncs for gained shards; both happen under a new status mutex so that they are atomic with respect to the recovery task's completion. A new task is only started if the running task completed concurrently with the epoch change or stopped unexpectedly; a spurious start is harmless since the extra task exits once it observes that the node is already active. - The task's scan bound stays frozen at the epoch it was started with: blobs certified later are covered by event processing, and shards gained at later epoch changes are covered by shard sync, which the task waits for. - On completion, the task attests epoch sync done for the recovery target currently recorded in the node status (which epoch changes advance), instead of the frozen spawn epoch whose attestation would be dropped as stale. - Attestation requires a clean scan pass during which no shard sync started, tracked by a new shard sync generation counter: a shard sync that starts and terminally fails during a pass leaves missing blobs behind that only a new scan pass finds. The blob scan also pauses promptly when a shard sync starts mid-pass, since per-blob recovery would redundantly decode slivers for shards that shard sync copies in bulk. The shard-gain simtest now also asserts that epoch changes processed while recovering do not restart the recovery task.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f83f6a6b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…target Addresses review feedback on the persistent recovery task: - Serialize the catch-up restart path with recovery completion: a recovery task from before the node started catching up only scanned blobs certified before its own start epoch, and blob certified events were skipped while catching up, so it must not complete the recovery target written by the catch-up path. The catch-up path now holds the recovery status mutex across writing its recovery target and aborting the previous task; a stale task either observes the RecoveryCatchUp status and exits without attesting, or is aborted before it can complete. - Use a dedicated scan_pass_interrupted flag instead of overloading has_more_blobs when a shard sync interrupts a scan pass, so that removing the verification re-scan (WAL-669) leaves the interrupted- pass signal intact.
halfprice
force-pushed
the
zhewu/node_recovery_across_epoch_change
branch
from
7f83f6a to
dbc0bc0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbc0bc016e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.create_new_shards_and_start_sync(shard_map_lock, shards_gained, &committees, false) | ||
| .await?; | ||
|
|
||
| drop(status_guard); |
There was a problem hiding this comment.
Dropping status_guard here lets the already-running recovery task complete (and submit epoch_sync_done) while this epoch-change handler is still about to lock shards that moved away. This can happen on a recovery-spanning epoch change with lost shards (especially with no gained shard syncs to wait on): recovery completion is serialized only by this mutex, and prepare_sliver_for_storage still treats the not-yet-LockedToMove shard as owned, so the node can recover/accept slivers for shards it no longer owns and attest the epoch before the local transition has locked them. Keep the guard through the shards_to_lock() loop so completion cannot race ahead of that state change.
Useful? React with 👍 / 👎.
1 participant
Description
Follow-up to #3464. When an epoch change started while node recovery was in progress, the recovery task was canceled and restarted from scratch — re-scanning the full blob info table on every epoch change even though the previous progress was still valid.
The recovery task now keeps running across epoch changes:
RecoveryInProgressepoch) and starts shard syncs for gained shards, both under a new status mutex so they are atomic with respect to the recovery task's completion. A new task is only started if the running one completed concurrently or stopped unexpectedly.epoch_sync_donefor the current recovery target recorded in the node status (advanced by epoch changes), instead of the frozen spawn epoch, whose attestation would be dropped as stale.Test plan
test_node_recovery_across_epoch_change_with_shard_gainnow additionally asserts the recovery task is spawned exactly once across the epoch changes processed while recovering. Passes on seeds 1 and 2.test_long_node_recovery,test_recovery_in_progress_with_node_restart,test_lagging_node_recovery.walrus-serviceunit tests pass; clippy and fmt clean.