Share service-opening steps between creation and recovery; make recovery_with_election assert outcomes

[Eddy Ashton (eddyashton)]

Motivation

recovery_with_election (runs under LONG_TESTS) tried to SIGTERM the recovering primary while it was still reading the private ledger, by polling quickly enough after submitting the shares. Nothing guaranteed that: the read takes well under a second for the 2000 padding entries, and the test did not start polling until recover_with_shares() returned, which is 0.3-1.5s after the final share is accepted depending on client turnaround. The test is inherently racy and fails whenever the turnaround exceeds the read; it surfaced as a deterministic CI failure on #8405, which does not change the read start or rate.

The property the test protects - a node which has lost leadership must not open the service, and opening must happen at most once - is decided in one guarded block at the end of private recovery. That block had no direct test, and the repository has no NodeState unit harness.

Extracted from #8405 so it can be reviewed and merged independently.

Implementation summary

• Service creation (transition_service_to_open) and the end of private recovery both issued recovery shares, marked the service open, endorsed the previous identity and requested a snapshot, each spelled out inline with its own copy of trigger_snapshot(). The shared steps are now ccf::open_service() (src/node/open_service.h), with ccf::trigger_snapshot() as the single implementation NodeState::trigger_snapshot() delegates to. ccf::open_recovered_service() adds the recovery-specific at-most-once guard (service must be WAITING_FOR_RECOVERY_SHARES) and clears the submitted shares before calling open_service(). Both NodeState call sites keep their existing failure semantics: recovery throws if the service cannot be opened, creation logs and continues as before.
• New open_service_test covers both paths on a real kv::Store: the service becomes OPEN, submitted shares are cleared (recovery) and fresh ones issued, the previous identity is endorsed, a snapshot is requested; an unopenable status is reported rather than thrown; and open_recovered_service throws for OPEN/RECOVERING/OPENING.
• recovery_with_election now asserts outcomes rather than timing: submit shares to the primary, SIGTERM it as soon as the final share is accepted, then check a new view is elected, every survivor reaches PartOfNetwork, the service is open, and no survivor hit the at-most-once guard. Whether the stop notice lands before or after the read finishes is a race the test no longer needs to win: if the primary wrote the opening transaction and stepped down before it replicated, the new leader rolls it back and opens the service itself (observed in local runs).

The 2000-entry padding is unchanged here; making it cheaper is a separate follow-up.

Safety and compatibility

Test-only change plus a behaviour-preserving extraction in NodeState; the steps and their order at both call sites are unchanged, as are their failure semantics. No API, data-format, configuration, or mixed-version impact. No CHANGELOG entry.

Validation: open_service_test, raft_test; e2e_logging (exercises the creation path); recovery_test with LONG_TESTS on a fresh main-based build; format, CMake, include-policy, Python format/lint, ASCII, copyright, test-bucket checks.

[Copilot]

Copilot review overview

🟡 Changes recommended

Add coverage for the snapshot flag and correct the inaccurate test comment.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Comment incorrectly claims coverage of non-primary refusal · New

What changed in this PR

Refactors recovered-service opening into a unit-tested helper and makes election recovery testing outcome-based.

Changes:

• Adds unit coverage for service opening and at-most-once guards.
• Reuses the helper during private ledger recovery.
• Removes timing-sensitive recovery polling.
• Registers the new unit test target.

File	Summary
tests/​recovery.py	Makes election recovery assertions outcome-based.
src/​node/​test/​open_recovered_service.cpp	Tests service opening behavior and state guards.
src/​node/​open_recovered_service.h	Extracts guarded service-opening logic.
src/​node/​node_state.h	Calls the extracted helper during recovery.
CMakeLists.txt	Registers the new unit test.

---

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