Skip to content

Open recovered service from a later primary if the opening is rolled back - #8450

Merged
Amaury Chamayou (achamayou) merged 7 commits into
mainfrom
achamayou-improved-carnival
Sep 28, 2026
Merged

Amaury Chamayou (achamayou) merged 7 commits into
mainfrom
achamayou-improved-carnival

Conversation

@achamayou

@achamayou Amaury Chamayou (achamayou) commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Motivation

recovery_with_election fails intermittently with Service status WaitingForRecoveryShares (expected one of ['Open']). This was found while investigating the Long e2e - Debug failure on #8435 (run 36037012519), which is not caused by that PR.

Only the node that is primary when it completes private recovery opens the recovered service. In the failing run:

  1. The old primary completed private recovery first and wrote the opening (4.93), which was not yet signed.
  2. The test SIGTERMed it, and it nominated a successor.
  3. The other nodes then completed private recovery as backups, so they wrote no opening.
  4. The successor's become_leader rolled back to the last committable index (92), dropping the unsigned opening.

Every node was then part of the network, but nothing could open the service: transition_service_to_open is a no-op in WaitingForRecoveryShares, and re-submitting recovery shares fails because the nodes are no longer part of the public network. The service stays in WaitingForRecoveryShares indefinitely.

This liveness gap was anticipated in #3755. It became visible recently: #8412 made recovery_with_election assert that the service opens, and #8444 shortened the private ledger, so the backups now usually complete private recovery before the forced election. Outside tests the window is wider, since the default signature interval is 1s rather than the tests' 100ms.

Implementation summary

  • NodeState tracks the opening of the recovered service: NotRecovering, then Pending once private recovery begins, then Committed once the service table's global hook sees an Open commit for the current identity. Committed is final, because global hooks run in table order and the opening may be seen in the same compaction as the trigger for private recovery.
  • While Pending, a node which has completed private recovery and is primary makes, from tick(), at most one attempt per view to open the service. An attempt only writes the opening if the node's own store still shows WaitingForRecoveryShares. A failed commit or a CompactedVersionConflict allows another attempt in the same view.
  • The end of private recovery uses the same helper, so a node elected during private recovery whose log already holds a committable opening now treats the service as already open. Previously it threw current service status is Open from the ledger handler, which was fatal. A commit failure there is now retried from tick() rather than thrown.
  • New subtest recovery_with_election_after_backups_recovered forces the failing ordering. With the default 1s signature interval, it suspends the primary as soon as every backup has begun private recovery, before the primary can sign an opening. A backup that completed private recovery as a backup is then elected and must open the service. The test checks that no backup left the initial view before completing private recovery, and that the opening committed in the new primary's view.
  • run_recovery_with_election now SIGKILLs the recovered nodes, including a suspended primary, in a finally, so a failing run no longer leaves nodes running with ignore_first_sigterm. In the failing CI run, one kept running for about 40 minutes.

Safety and compatibility

  • Only a primary writes the opening, and only when its own store shows WaitingForRecoveryShares. After become_leader's rollback a primary's store matches its log, so every branch of the log contains at most one opening, and Raft commits a single branch. KV conflict detection stops concurrent attempts on the same node from both committing.
  • Attempts after the end of private recovery run as a worker task without NodeState::lock, like transition_service_to_open. Exceptions other than CompactedVersionConflict are not caught, so they remain fatal as before.
  • Outside recovery the new check in tick() is a single atomic load.
  • The opening transaction is unchanged, so there are no API, ledger format or governance changes. In a mixed-version recovery, only nodes with this change retry the opening.

Validation

Done, with a local Debug build (-DGLIBCXX_DEBUG=ON -DLONG_TESTS=ON):

  • The new subtest fails on unfixed main with the service stuck in WaitingForRecoveryShares (4/4 runs of the first, SIGTERM-based version, 5/5 of the current, suspend-based one). It passes in every run with the fix (14 runs of the suspend-based version, 4 of them after merging main). In each run the new primary opened the service from tick(), and the other backup rolled back the old primary's unsigned opening. One run of the existing recovery_with_election hit the CI ordering by chance, and the service was opened by the successor.
  • Full recovery_test (re-run after merging main), recovery_stale_snapshot_join_test, recovery_intermediate_snapshot_join_test, recovery_snapshot_endorsements_test and open_service_test pass.
  • With a failure injected after the election, the recovered nodes are SIGKILLed before the test exits. Without the finally, they outlive the test.
  • C++ and Python format checks, ruff, ASCII, TODO, release notes and prettier checks pass. clang-tidy 21 reports nothing on the changed lines.

Not done:

  • clang-tidy 18, which crashes on this host's toolchain, and the full scripts/ci-checks.sh. These are covered by CI.
  • The new subtest relies on the test suspending the primary within its 1s signature interval after the final recovery share commits. Locally the primary was suspended 131-211ms after it began private recovery. A fully deterministic ordering would need a node-side test hook.

…back

Only the node that is primary when it completes private recovery opened the
recovered service. If an election rolled back that unsigned opening, and the
new primary had completed private recovery as a backup, nothing re-opened the
service, which stayed in WaitingForRecoveryShares indefinitely. This is the
intermittent recovery_with_election failure in the Long e2e - Debug job.

A node which has begun private recovery now tries, once per view in which it
is primary and part of the network, to open the service until it sees the
opening commit. It does not write a second opening if its store already
holds one, which also stops a node elected during private recovery from
failing at the end of recovery when its log already opens the service.

Adds recovery_with_election_after_backups_recovered, which forces the failing
ordering deterministically.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The recovered nodes run with ignore_first_sigterm, and were only SIGKILLed
when the test passed, so a failing run could leave them running (one ran for
about 40 minutes in CI). Collect the recovered network as soon as it is
created and SIGKILL its nodes in a finally.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 25, 2026 08:52
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Address the critical state-transition issue and the test determinism and cleanup findings.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

This PR fixes recovered-service opening after an election rollback and adds regression coverage.

Changes:

  • Adds recovery-opening retry state and logic.
  • Adds election-ordering tests and cleanup handling.
  • Documents the fix in the changelog.
File Description
tests/​recovery.py Adds election regression coverage and recovered-node cleanup.
src/​node/​node_state.h Implements recovery-opening state and retries.
CHANGELOG.md Documents the recovery liveness fix.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/node/node_state.h
Comment thread tests/recovery.py Outdated
@achamayou Amaury Chamayou (achamayou) added the run-long-test Run Long Test job label Sep 25, 2026
Review feedback: the forced ordering relied on the election beating the
old primary's next signature, so on a slow runner the opening could commit
first. Suspend the primary as soon as every backup has begun private
recovery instead: it cannot sign again until about sig_ms_interval after
the signature which committed the final share, so a backup which completed
private recovery as a backup is elected and must open the service,
independent of private recovery and election durations. Assert that no
backup left the initial view before completing private recovery, and
SIGKILL suspended nodes on cleanup.

Such a new primary opens the service asynchronously after its election,
so poll for the service to be open rather than checking once.

Also clarify the recovered_service_opening comment on why the service hook
can mark the opening Committed before private recovery begins.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@achamayou
Amaury Chamayou (achamayou) merged commit 1d39bd3 into main Sep 28, 2026
12 checks passed
@achamayou
Amaury Chamayou (achamayou) deleted the achamayou-improved-carnival branch September 28, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-long-test Run Long Test job

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants