feat(migration): add support for back migration without restart by barakeinav1 · Pull Request #3550 · near/mpc

barakeinav1 · 2026-06-14T13:23:56Z

Superseded — do not merge. See #3601 for the dispatcher-based redesign per @KevinDeforth's review feedback (outer state-driven dispatcher, mutual exclusion between coordinator and onboarding rather than parallel loops). The #3406 fix will land via that PR.

Design doc (original): docs/design/migration-onboarding-reentry.md.

Test plan (kept for reference)

New tests added:

Unit: onboard__should_fire_first_done_signal_and_keep_looping_past_done (crates/node/src/migration_service/onboarding.rs) — asserts the loop fires first_done_signal once on first Done and keeps running past it.
E2E: migration_service__should_handle_back_migration_a_to_b_to_a_no_restart (crates/e2e-tests/tests/migration_service.rs) — back-migration A → B → A with A0 staying alive between the two directions. Fails on main; passes here.

The migration web server's `import_keyshares` channel was owned by the `onboard()` loop and dropped when the loop returned on `OnboardingJob::Done`. For any node that was already an active participant at process start, this happened immediately, leaving the web server with a dangling sender. Subsequent `PUT /set_keyshares` calls (e.g. a back-migration targeting that node) returned `500 keyshares receiver channel is closed`, forcing a process restart to recreate the channel. Make `onboard()` a long-lived state machine. `Done` becomes a resting state (same shape as `WaitForStateChange`) rather than a terminal one. The first time the loop reaches `Done`, it fires a oneshot so the caller can unblock startup -- preserving the pre-existing gating that the coordinator only starts once keyshares are present. After that, the loop keeps watching contract + migration state. A future `start_node_migration` naming this node as destination flips `OnboardingJob` to `Onboard(keyset)`, the loop re-enters `execute_onboarding`, and the existing protocol path handles both sub-cases without branching: - epoch unchanged: storage check passes, no PUT required, conclude tx sent - epoch advanced: backup-CLI PUT lands, import_backup runs, conclude tx sent Design: docs/design/migration-onboarding-reentry.md. Tests: - Unit: onboard__should_fire_first_done_signal_and_keep_looping_past_done asserts the loop fires first_done_signal once and keeps running. - E2E: migration_service__should_handle_back_migration_a_to_b_to_a_no_restart back-migration A -> B -> A with A0 staying alive between directions; fails on main, passes here.

barakeinav1 · 2026-06-14T14:54:06Z

@@ -51,8 +52,11 @@ pub(crate) async fn onboard(
        match job {
            OnboardingJob::Done => {


maybe a better name is "active_participant"?

Copilot

Pull request overview

This PR makes the migration onboarding loop re-entrant so nodes that start as active participants keep their keyshare receiver alive and can later accept back-migration keyshares without requiring a process restart (fixes #3406).

Changes:

Turn onboard() into a long-lived state machine: Done becomes a resting state, and a one-shot signal is used to unblock startup on first Done.
Spawn onboard() as a background task from spawn_recovery_server_and_run_onboarding and await the one-shot instead of awaiting the whole loop.
Add unit + e2e coverage, including a new “no restart” back-migration round-trip test variant and a new port seed constant.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
docs/design/migration-onboarding-reentry.md	Design doc describing the re-entrant onboarding approach and operational motivation.
crates/node/src/migration_service/onboarding.rs	Makes `onboard()` non-terminating on `Done` and adds a unit test verifying the new behavior.
crates/node/src/migration_service.rs	Spawns `onboard()` and waits for a first-`Done` one-shot before returning.
crates/e2e-tests/tests/migration_service.rs	Refactors back-migration e2e test to support both restart and no-restart variants; adds new no-restart test.
crates/e2e-tests/tests/common.rs	Adds a new port seed for the no-restart back-migration test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

barakeinav1 · 2026-06-14T15:13:37Z

+    let (first_done_tx, first_done_rx) = oneshot::channel();
+    let tls_public_key = migration_secrets.p2p_private_key.verifying_key();
+    tokio::spawn(async move {
+        if let Err(err) = onboard(
+            contract_state_receiver,
+            my_migration_info_receiver,
+            my_near_account_id,
+            tls_public_key,
+            tx_sender,
+            keyshare_storage,
+            import_keyshares_receiver,
+            Some(first_done_tx),
+        )
+        .await
+        {
+            tracing::error!(?err, "onboarding state machine exited unexpectedly");
+        }
+    });
+
+    // Err here = the spawned task dropped the sender; the panic/error is
+    // already logged above. Either way, startup proceeds.
+    let _ = first_done_rx.await;
    Ok(())


Good catch — agreed. Fixed in 9781816: first_done_rx.await now propagates with .context(...) so startup fails fast if the spawned onboarding task dies before signaling.

barakeinav1 · 2026-06-14T15:13:39Z

+        // Keep `contract_state_sender` alive until after the assertion so the
+        // loop's receiver channel doesn't close (which would make it exit).
+        drop(contract_state_sender);
+        onboard_handle.abort();


Agreed — abort() is what actually kills the task; the drop was meaningless and the comment was misleading. Fixed in 9781816: removed the drop + comment, renamed the binding to _contract_state_sender.

barakeinav1 · 2026-06-14T15:05:42Z

@claude review

claude · 2026-06-14T15:06:14Z

Pull request overview

Makes the migration onboard() state machine re-entrant so a node can be migrated A → B → A without a process restart. Previously, hitting OnboardingJob::Done returned from onboard(), dropping the keyshare receiver — and the still-running web server's later PUT /set_keyshares would 500 with "keyshares receiver channel is closed". Now Done is a resting state: the first time the loop reaches it, a oneshot fires so startup can proceed, and the loop keeps watching for the next contract transition.

Changes:

onboard(): replace return Ok(()) on Done with cancellation_token.cancelled().await; continue; plus a one-shot first_done_signal fired exactly once (onboarding.rs:53-60).
spawn_recovery_server_and_run_onboarding: detach onboard() into a tokio::spawn and await the first-done oneshot before returning (migration_service.rs:62-83); adds 'static bound to tx_sender.
Adds a unit test asserting onboard() fires the signal and keeps looping past Done.
Refactors the E2E back_migration_a_to_b_to_a test into shared scaffolding and adds a new ..._no_restart variant that exercises the re-entry path on main.
Adds docs/design/migration-onboarding-reentry.md.

Reviewed changes

Per-file summary

File	Description
`crates/node/src/migration_service.rs`	Spawn `onboard()` instead of awaiting; gate startup on a oneshot first-Done signal; add `'static` bound to `tx_sender`.
`crates/node/src/migration_service/onboarding.rs`	Make `Done` a resting state, fire `first_done_signal` once; new unit test for the looping behavior.
`crates/e2e-tests/tests/migration_service.rs`	Extract shared back-migration helper; add `_no_restart` variant.
`crates/e2e-tests/tests/common.rs`	Add `MIGRATION_BACK_NO_RESTART_PORT_SEED = 23`.
`docs/design/migration-onboarding-reentry.md`	New design doc explaining motivation, fix, and risk.

Findings

Blocking (must fix before merge):

crates/node/src/migration_service.rs:64-83 — Startup no longer fails when onboard() errors. The previous code propagated onboard().await?, so a "keyshare sender dropped" error during initial onboarding aborted process startup. The new code spawns onboard() and only logs the error via tracing::error!; when the spawned task exits with Err, the first_done_tx is dropped, let _ = first_done_rx.await; swallows the RecvError, and the coordinator starts as if onboarding had succeeded. For a new node that is still waiting on Onboard(keyset) when the web server / import-keyshares channel drops, this is a behavior regression — the operator gets a silent log line instead of a failed startup. Suggested fix: distinguish "first Done fired" from "spawned task exited before first Done" and return anyhow::bail! in the latter case, e.g.
```rust
match first_done_rx.await {
Ok(()) => Ok(()),
Err(_) => anyhow::bail!("onboarding state machine exited before reaching Done"),
}
```
Also consider that a panic in the spawned onboard() task is silently dropped (the JoinHandle is discarded and only the Err arm logs) — at minimum, retaining the handle and surfacing a panic would mirror prior fail-fast semantics.

Non-blocking (nits, follow-ups, suggestions):

crates/node/src/migration_service.rs:81-82 — Comment says "the panic/error is already logged above" but only Err is logged inside the spawned task; a panic! inside onboard() lands on the discarded JoinHandle and is never surfaced. Tighten the wording or actually catch the panic.
crates/node/src/migration_service/onboarding.rs:697-699 — Comment claims that not dropping contract_state_sender early matters because "the loop's receiver channel doesn't close (which would make it exit)". In practice the receiver closing only makes contract_state_receiver.changed() resolve immediately inside the monitoring task; onboard() itself is parked on cancellation_token.cancelled().await and wouldn't exit. The onboard_handle.abort() is what actually terminates the loop. Clarify or drop the comment.
crates/node/src/migration_service/onboarding.rs:691 — tokio::time::sleep(Duration::from_millis(200)) followed by assert!(!onboard_handle.is_finished(), ...) is a fixed-time liveness check. Low risk in practice, but consider tokio::time::pause() + advance() or polling is_finished() a few times if it proves flaky in CI.
docs/design/migration-onboarding-reentry.md (Risk section) — calls out "in-memory state from A0's prior tenure could leak into the re-entered Onboard cycle" and proposes tracking it separately. Since this PR is what enables that scenario, please open the follow-up issue before merge so there is a concrete owner/link instead of a paragraph in a design doc.

⚠️ Issues found

Two Copilot review findings on #3550: 1. spawn_recovery_server_and_run_onboarding ignored `first_done_rx.await` errors, so a panicking onboarding task would still return Ok(()) at startup -- silently reintroducing the #3406 symptom (no live receiver on the keyshare channel). Propagate the RecvError with context so startup fails fast, matching the pre-PR semantics. 2. Test cleanup in onboard__should_fire_first_done_signal... had a misleading comment claiming `drop(contract_state_sender)` made the loop exit. It doesn't -- abort() does. Remove the drop and the comment; rename the unused binding to `_contract_state_sender`.

claude · 2026-06-14T15:10:37Z

Pull request overview

Makes onboard() a long-lived state machine so Done becomes a resting state instead of a terminal one. This keeps the keyshare receiver alive after first-time onboarding completes, fixing the 500 keyshares receiver channel is closed failure on back-migration without a process restart (#3406). Startup now spawns onboard() as a background task and unblocks via a one-shot signal fired on the first Done.

Changes:

onboard() no longer returns on Done — fires a one-shot first_done_signal (once) and cancellation_token.cancelled().await; continue; to wait for the next contract transition.
spawn_recovery_server_and_run_onboarding detaches onboard() via tokio::spawn and awaits first_done_rx before returning. TransactionSender bound tightened to + 'static for the spawn.
Unit test for first-Done signal + continued loop; e2e back-migration test parameterized into restart vs. no-restart variants, with a new no-restart test that fails on main.
Design doc docs/design/migration-onboarding-reentry.md describing the failure mode, fix, sequence diagrams, and the in-memory-state risk.

Reviewed changes

Per-file summary

File	Description
`crates/node/src/migration_service.rs`	Spawns `onboard()` as background task, awaits one-shot for first `Done`.
`crates/node/src/migration_service/onboarding.rs`	Makes `Done` a resting state, adds `first_done_signal` parameter, adds unit test.
`crates/e2e-tests/tests/migration_service.rs`	Refactors back-migration test into shared scaffolding; adds no-restart variant.
`crates/e2e-tests/tests/common.rs`	New `MIGRATION_BACK_NO_RESTART_PORT_SEED` constant.
`docs/design/migration-onboarding-reentry.md`	Design rationale + sequence diagrams.

Findings

Blocking (must fix before merge):

crates/node/src/migration_service.rs:80-83 — Reinforcing the copilot reviewer's point with one important refinement: the comment claims "the panic/error is already logged above" but that only holds for returned Err values caught by if let Err(err) = onboard(...).await. If onboard() panics, the if let arm is never reached — tokio logs the panic via its default panic hook (separately, not "above"), and first_done_rx.await still resolves to Err(RecvError), which let _ = swallows. The result in either failure mode (return-Err or panic): startup returns Ok(()), the migration web server is up, the receiver end of import_keyshares_sender is gone, and we're back to the exact 500 keyshares receiver channel is closed failure this PR is meant to fix — except now with no process-restart escape hatch because the operator has no signal that anything is wrong. Propagate the error: first_done_rx.await.context("onboarding state machine exited before first Done; migration service is dead")?. Fail-fast is correct here; this is startup.

Non-blocking (nits, follow-ups, suggestions):

crates/node/src/migration_service.rs:73-79 — The spawned JoinHandle is dropped, detaching the task. Consider returning it (or storing it on a struct owned by the coordinator) so a future shutdown path can abort() it cleanly and so the task's liveness is observable. Today, if the loop dies after startup it dies silently — tracing::error! is the only signal, and that's easy to miss.
crates/node/src/migration_service/onboarding.rs:78-81 — Pre-existing, but worth noting now that the loop is long-lived: the bail!(\"keyshare sender dropped...\") path in the Onboard arm will end the entire onboarding state machine for the lifetime of the process, after re-entry was the whole point. Not introduced by this PR, but the new long-lived contract changes the operational severity.
crates/node/src/migration_service/onboarding.rs:53 — Agreeing with @barakeinav1: first_done_signal is misleading because "done" means "active participant in current epoch," not "finished onboarding forever." Something like active_participant_signal or node_ready_signal would more accurately describe what the caller actually waits for at startup.
crates/node/src/migration_service/onboarding.rs:635-702 — The unit test only verifies the loop doesn't terminate on Done. The actual re-entry behavior (Done → Onboard(keyset) → executes onboarding → back to Done) is only covered by the e2e test. A unit test that drives a contract-state transition into a fresh Onboard(keyset) after the first Done would tighten coverage of the central invariant this PR introduces.
crates/node/src/migration_service/onboarding.rs:698-700 — Echoing copilot: the comment "drop sender → loop exits" is incorrect (the loop doesn't observe watch-channel closure), and the drop+abort ordering is misleading. Either drop the drop(contract_state_sender) line entirely (abort() is what's actually doing cleanup) or fix the comment.
docs/design/migration-onboarding-reentry.md:148-159 — The "Risk" section is honest about possible in-memory state leakage across re-entry, but the design doc treats it as "worth an explicit audit; tracking separately." Given the PR description points at Investigate: in-memory state cleanup on Done -> Inactive -> Onboard re-entry (follow-up to #3550) #3551 as a follow-up, please link that explicitly from the design doc so future readers don't have to dig.

⚠️ Issues found

1. spawn_recovery_server_and_run_onboarding now propagates errors from first_done_rx.await with context. Previously the error was discarded, so a panicking onboarding task could let startup return Ok(()) with no live listener on the keyshare channel. 2. Test cleanup in onboard__should_fire_first_done_signal... had a misleading comment claiming drop(contract_state_sender) made the loop exit. It doesn't -- abort() does. Drop the line and the comment; rename the unused binding to _contract_state_sender.

barakeinav1 · 2026-06-14T15:42:07Z

Thanks for the reviews — going through the unique findings. Most of the "blocking" items are stale (fixed in 9781816 in response to Copilot's earlier pass on the same code).

Startup no longer fails when onboard() errors. ... let _ = first_done_rx.await; swallows the RecvError

... the comment claims "the panic/error is already logged above" but ...

Already fixed in 9781816. first_done_rx.await.context("onboarding task died before reaching first Done")? now propagates fail-fast, covering both Err returns and panics (both drop the sender). The comment was rewritten neutrally.

Comment claims that not dropping contract_state_sender early matters because "the loop's receiver channel doesn't close (which would make it exit)". ... onboard_handle.abort() is what actually terminates the loop.

Already fixed in 9781816. Removed the drop(contract_state_sender) line and the misleading comment; the binding is now _contract_state_sender.

If onboard() panics ... first_done_rx.await still resolves to Err(RecvError), which let _ = swallows. The result in either failure mode (return-Err or panic): startup returns Ok(()) ... we're back to the exact 500 keyshares receiver channel is closed failure this PR is meant to fix

This was the blocking finding addressed by 9781816. Same propagation now covers panics — they drop the sender, first_done_rx returns Err, we .context(...)?. Startup fails fast in both modes.

bail!("keyshare sender dropped...") path in the Onboard arm will end the entire onboarding state machine for the lifetime of the process, after re-entry was the whole point.

Good catch, real concern. Pre-existing path whose operational severity changed under the long-lived loop. If bail!() fires after first Done was signaled, the spawned task dies silently — same symptom as #3406, different cause.

Filed as #3552 as a follow-up. Not addressing in this PR — it's a separate failure mode (web server task died) that needs its own observability/shutdown story.

The unit test only verifies the loop doesn't terminate on Done. The actual re-entry behavior (Done → Onboard(keyset) → executes onboarding → back to Done) is only covered by the e2e test.

That's by design. The e2e test (migration_service__should_handle_back_migration_a_to_b_to_a_no_restart, added in this PR) exercises the full Done → Onboard → Done cycle end-to-end with a real cluster. The unit test specifically targets the "Done is not terminal" invariant, which is the minimal regression guard the loop change requires.

Please link [#3551] explicitly from the design doc

Done in 2cd0db0.

The spawned JoinHandle is dropped, detaching the task. Consider returning it ... so a future shutdown path can abort() it cleanly and so the task's liveness is observable.

Real observability gap, but it's a broader codebase pattern (the migration web server task in web/server.rs is also fire-and-forget — same JoinHandle-discard pattern, predates this PR). Out of scope here; covered by the same #3552 follow-up.

tokio::time::sleep(Duration::from_millis(200)) followed by assert!(!onboard_handle.is_finished(), ...) is a fixed-time liveness check.

Low priority. Leaving as-is unless it actually flakes in CI.

first_done_signal is misleading because "done" means "active participant in current epoch," not "finished onboarding forever."

Agreed (two AI reviewers + @barakeinav1 independently). Deferred to Kevin's review — flagged as an open suggestion, didn't want to rename without his input.

barakeinav1 · 2026-06-17T08:17:41Z

Superseded by #3601 — implementing per @kevindeforth's feedback (outer state-driven dispatcher, mutually exclusive coordinator/onboarding) instead of the parallel-loop approach here.

@kevindeforth

…ding (#3406) Replaces the linear startup ('start migration web server -> run onboarding to first Done -> run coordinator forever') in run.rs with a state-driven dispatcher loop. The two subsystems are now mutually exclusive at the run.rs level: - Migration web server starts once and stays alive for the process lifetime; the keyshare-import receiver is cloned into every onboarding cycle. - Each iteration classifies the role via OnboardingJob::new(contract_state, migration_info, my_id, tls_pub_key): Done -> run coordinator until the role changes away from Done, then cancel and re-enter the loop. Onboard(_) | WaitForStateChange -> run onboarding (returns on first Done), then re-enter the loop. Also splits migration_service.rs so callers can start the web server independently and pass the receiver to onboard() repeatedly: - start_migration_web_server(...) -> watch::Receiver<Vec<Keyshare>> - wait_until_role_change(...) -> OnboardingJob This is the back-migration-without-restart fix from #3406 in the structural shape requested by @kevindeforth in #3550's review (mutual exclusion rather than parallel loops with shared keyshare-storage invariants). Implements the design in docs/design/migration-onboarding-dispatcher.md.

@kevindeforth

Captures the outer state-driven dispatcher approach agreed with @kevindeforth in the PR #3550 review: mutually exclusive coordinator vs onboarding subsystems driven by a single dispatcher loop, instead of running them in parallel with implicit storage-access invariants. Implementation will follow on this branch.

@kevindeforth

…ding (#3406) Replaces the linear startup ('start migration web server -> run onboarding to first Done -> run coordinator forever') in run.rs with a state-driven dispatcher loop. The two subsystems are now mutually exclusive at the run.rs level: - Migration web server starts once and stays alive for the process lifetime; the keyshare-import receiver is cloned into every onboarding cycle. - Each iteration classifies the role via OnboardingJob::new(contract_state, migration_info, my_id, tls_pub_key): Done -> run coordinator until the role changes away from Done, then cancel and re-enter the loop. Onboard(_) | WaitForStateChange -> run onboarding (returns on first Done), then re-enter the loop. Also splits migration_service.rs so callers can start the web server independently and pass the receiver to onboard() repeatedly: - start_migration_web_server(...) -> watch::Receiver<Vec<Keyshare>> - wait_until_role_change(...) -> OnboardingJob This is the back-migration-without-restart fix from #3406 in the structural shape requested by @kevindeforth in #3550's review (mutual exclusion rather than parallel loops with shared keyshare-storage invariants). Implements the design in docs/design/migration-onboarding-dispatcher.md.

barakeinav1 force-pushed the fix/3406-migration-onboarding-reentry branch from 066b3e1 to b72e64a Compare June 14, 2026 14:49

barakeinav1 commented Jun 14, 2026

View reviewed changes

barakeinav1 marked this pull request as ready for review June 14, 2026 14:59

Copilot AI review requested due to automatic review settings June 14, 2026 14:59

Copilot started reviewing on behalf of barakeinav1 June 14, 2026 15:00 View session

Copilot AI reviewed Jun 14, 2026

View reviewed changes

barakeinav1 mentioned this pull request Jun 14, 2026

Investigate: in-memory state cleanup on Done -> Inactive -> Onboard re-entry (follow-up to #3550) #3551

Open

4 tasks

barakeinav1 force-pushed the fix/3406-migration-onboarding-reentry branch from 2ebacaf to 9781816 Compare June 14, 2026 15:13

docs(design): link #3551 from the Risk section

2cd0db0

barakeinav1 mentioned this pull request Jun 14, 2026

Silent migration-service death after first Done (follow-up to #3550) #3552

Open

3 tasks

barakeinav1 requested a review from kevindeforth June 14, 2026 17:25

barakeinav1 changed the title ~~fix(migration): make onboarding state machine re-entrant (#3406)~~ feat(migration): add support for back migration without restart Jun 14, 2026

barakeinav1 marked this pull request as draft June 17, 2026 08:15

barakeinav1 mentioned this pull request Jun 17, 2026

feat(migration): back-migration without restart via state-driven dispatcher #3601

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(migration): add support for back migration without restart#3550

feat(migration): add support for back migration without restart#3550
barakeinav1 wants to merge 3 commits into
mainfrom
fix/3406-migration-onboarding-reentry

barakeinav1 commented Jun 14, 2026 •

edited

Loading

Uh oh!

barakeinav1 Jun 14, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

barakeinav1 Jun 14, 2026

Uh oh!

barakeinav1 Jun 14, 2026

Uh oh!

barakeinav1 commented Jun 14, 2026

Uh oh!

claude Bot commented Jun 14, 2026

Uh oh!

claude Bot commented Jun 14, 2026

Uh oh!

barakeinav1 commented Jun 14, 2026

Uh oh!

barakeinav1 commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -51,8 +52,11 @@ pub(crate) async fn onboard(
		match job {
		OnboardingJob::Done => {

Uh oh!

Conversation

barakeinav1 commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan (kept for reference)

Uh oh!

barakeinav1 Jun 14, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

barakeinav1 Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

barakeinav1 Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

barakeinav1 commented Jun 14, 2026

Uh oh!

claude Bot commented Jun 14, 2026

Pull request overview

Reviewed changes

Findings

Uh oh!

claude Bot commented Jun 14, 2026

Pull request overview

Reviewed changes

Findings

Uh oh!

barakeinav1 commented Jun 14, 2026

Uh oh!

barakeinav1 commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

barakeinav1 commented Jun 14, 2026 •

edited

Loading

barakeinav1 Jun 14, 2026 •

edited

Loading