diff --git a/docs/design/2026_04_26_implemented_raft_learner.md b/docs/design/2026_04_26_implemented_raft_learner.md index 011c5abbe..9f2caa3f1 100644 --- a/docs/design/2026_04_26_implemented_raft_learner.md +++ b/docs/design/2026_04_26_implemented_raft_learner.md @@ -874,7 +874,15 @@ single-process 3-node demo cluster, attaches a learner via changes). - Promote precondition: surface `Progress[nodeID].Match` in `Status.PerPeer` so an operator can choose `min_applied_index` - without guessing. + without guessing. **Implemented** — `Status.PerPeer` reports each + remote replica's `Match`/`Next`/`IsLearner`/`RecentActive` from the + leader's tracker, and is nil on a follower (non-nil, possibly empty, + on a leader). Operators watch `Match` climb to a target such as the + same snapshot's `CommitIndex` and pass the TARGET as + `min_applied_index`: passing the learner's own current `Match` would + satisfy the engine's `Match >= minAppliedIndex` check by + construction and promote a lagging replica. Exposing it over the + RaftAdmin `Status` RPC needs a proto change and is a follow-up. - Decision gate for follower-served reads: write a separate proposal, do not extend this one. @@ -884,8 +892,8 @@ join-as-learner alarm, monitoring suffrage labels, promotion precondition checks against leader `Progress.Match`, and the operator runbook (`docs/raft_learner_operations.md`). Remaining Milestone 3 hardening is not claimed shipped here: the learner attach/promote-under-partition Jepsen -workload and a first-class `Status.PerPeer` progress field are still open, and -follower-served read routing remains a separate proposal. +workload is still open, and follower-served read routing remains a separate +proposal. The `Status.PerPeer` progress field has since landed (see §6). ## 7. Risks diff --git a/internal/raftengine/engine.go b/internal/raftengine/engine.go index bdad3b697..f93a7c21d 100644 --- a/internal/raftengine/engine.go +++ b/internal/raftengine/engine.go @@ -120,6 +120,51 @@ type Status struct { // in the local raft log and has not yet applied. Leadership transfer // is rejected while this is true. PendingConfChange bool + // PerPeer reports each remote replica's replication progress as + // the LEADER sees it, keyed by numeric node id. It is nil on a + // follower, where raft tracks no progress for anyone else — an + // empty map there would be indistinguishable from "the leader + // knows about no peers". + // + // It exists so an operator can decide WHEN a learner is ready to + // promote, by watching its Match climb toward a target — the same + // status snapshot's CommitIndex is the natural one. + // + // Do NOT pass a learner's current Match straight back as + // PromoteLearner's min_applied_index. The engine checks + // Match >= minAppliedIndex, so the learner's own present value + // satisfies it by construction and the precondition becomes a + // no-op: a replica sitting at Match=10 against a leader committed + // through 100 would be promoted, joining the voter quorum before + // it has caught up and potentially stalling writes or cutting + // fault tolerance immediately. Compare Match against the target, + // then pass the TARGET. + // + // It is nil on a follower and non-nil (possibly empty) on a + // leader, so PerPeer == nil distinguishes "not the leader" from + // "leader with no remote replicas". + PerPeer map[uint64]PeerProgress +} + +// PeerProgress is one remote replica's replication progress from the +// leader's tracker. +type PeerProgress struct { + // Match is the highest log index known to be replicated to this + // peer. PromoteLearner compares it against the min_applied_index + // the caller supplies — which must be a catch-up TARGET, not this + // value; see PerPeer's note. + Match uint64 + // Next is the next index the leader will send to this peer. + Next uint64 + // IsLearner reports whether raft currently tracks this peer as a + // learner. It comes from the leader's live tracker rather than + // from the peers file, so it reflects what raft will actually do + // rather than what was last persisted. + IsLearner bool + // RecentActive reports whether the peer has responded since the + // last election-timeout check. A learner that is caught up but + // inactive is not a safe promotion target. + RecentActive bool } type ProposalResult struct { diff --git a/internal/raftengine/etcd/engine.go b/internal/raftengine/etcd/engine.go index 1eef4275e..f73141ebe 100644 --- a/internal/raftengine/etcd/engine.go +++ b/internal/raftengine/etcd/engine.go @@ -23,6 +23,7 @@ import ( etcdstorage "go.etcd.io/etcd/server/v3/storage" etcdraft "go.etcd.io/raft/v3" raftpb "go.etcd.io/raft/v3/raftpb" + "go.etcd.io/raft/v3/tracker" "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -3858,6 +3859,43 @@ func (e *Engine) resolveReadyReads() { } } +// peerProgress snapshots the leader's replication tracker. +// +// Only the leader tracks progress, so a follower returns nil rather +// than an empty map: the two would otherwise be indistinguishable, and +// an operator reading an empty map on a follower could conclude the +// cluster has no peers. +// +// rawNode.WithProgress is used instead of rawNode.Status() because +// this runs on every Ready: Status() deep-copies the whole progress +// map plus the config, while WithProgress visits in place and lets us +// allocate exactly one small map sized to the peer count. +func (e *Engine) peerProgress(state raftengine.State) map[uint64]raftengine.PeerProgress { + if state != raftengine.StateLeader { + return nil + } + // Allocated up front, not lazily inside the visitor: a + // single-node leader visits only itself, which is skipped, and a + // lazily-built map would leave a healthy peerless leader + // reporting nil — indistinguishable from a follower to any + // consumer testing PerPeer == nil. + out := make(map[uint64]raftengine.PeerProgress) + e.rawNode.WithProgress(func(id uint64, _ etcdraft.ProgressType, pr tracker.Progress) { + if id == e.nodeID { + // The leader's own entry tracks itself; operators care + // about remote replicas. + return + } + out[id] = raftengine.PeerProgress{ + Match: pr.Match, + Next: pr.Next, + IsLearner: pr.IsLearner, + RecentActive: pr.RecentActive, + } + }) + return out +} + func (e *Engine) refreshStatus() { previous := e.Status().State basic := e.rawNode.BasicStatus() @@ -3882,6 +3920,7 @@ func (e *Engine) refreshStatus() { ConfigurationIndex: e.currentConfigIndex(), LeadTransferee: basic.LeadTransferee, PendingConfChange: e.hasPendingConfChange(), + PerPeer: e.peerProgress(state), } e.mu.Lock() diff --git a/internal/raftengine/etcd/status_perpeer_test.go b/internal/raftengine/etcd/status_perpeer_test.go new file mode 100644 index 000000000..dcea10153 --- /dev/null +++ b/internal/raftengine/etcd/status_perpeer_test.go @@ -0,0 +1,152 @@ +package etcd + +import ( + "context" + "testing" + "time" + + "github.com/bootjp/elastickv/internal/raftengine" + "github.com/stretchr/testify/require" +) + +// TestStatusPerPeerReportsLearnerProgressOnTheLeader closes the +// Milestone 3 hardening item from +// docs/design/2026_04_26_implemented_raft_learner.md §6: an operator +// choosing PromoteLearner's min_applied_index had no way to read the +// learner's actual Match, so the value was a guess — too high fails +// the precondition, too low promotes a replica that has not caught up. +func TestStatusPerPeerReportsLearnerProgressOnTheLeader(t *testing.T) { + nodes, peers := newTransportTestNodes(t, 2) + startTransportTestServers(nodes, peers) + t.Cleanup(func() { cleanupTransportTestNodes(t, nodes) }) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + require.NoError(t, openTransportTestNode(ctx, nodes[0], peers[:1], true)) + leader := waitForLeaderNode(t, nodes[:1]) + require.NoError(t, openTransportTestNode(ctx, nodes[1], peers, false)) + + _, err := leader.engine.AddLearner(ctx, nodes[1].peer.ID, nodes[1].peer.Address, 0) + require.NoError(t, err) + waitForConfigSize(t, leader.engine, 2) + waitForConfigSize(t, nodes[1].engine, 2) + + learnerNodeID := nodes[1].peer.NodeID + + // The leader eventually reports the learner's replication progress. + var progress raftengine.PeerProgress + require.Eventually(t, func() bool { + status := leader.engine.Status() + p, ok := status.PerPeer[learnerNodeID] + if !ok || p.Match == 0 { + return false + } + progress = p + return true + }, 10*time.Second, 25*time.Millisecond, + "the leader must report the learner's Match so min_applied_index need not be guessed") + + require.True(t, progress.IsLearner, + "progress must come from the live tracker, which knows this peer is a learner") + require.GreaterOrEqual(t, progress.Next, progress.Match) + + // The CORRECT use: watch Match climb to a target and pass the + // TARGET. Passing the learner's own current Match would satisfy + // the engine's Match >= minAppliedIndex check by construction, + // turning the precondition into a no-op that promotes a lagging + // replica into the voter quorum. + var target uint64 + require.Eventually(t, func() bool { + status := leader.engine.Status() + target = status.CommitIndex + p, ok := status.PerPeer[learnerNodeID] + return ok && target > 0 && p.Match >= target + }, 10*time.Second, 25*time.Millisecond, + "the learner must be observed catching up to the leader's commit index") + + _, err = leader.engine.PromoteLearner(ctx, nodes[1].peer.ID, 0, target, false) + require.NoError(t, err, + "promotion at the observed catch-up target must satisfy the precondition") +} + +// TestStatusPerPeerIsEmptyNotNilOnAPeerlessLeader pins the other half +// of the nil-versus-empty contract. A single-node leader has no remote +// replicas, but it IS the leader — reporting nil would make it +// indistinguishable from a follower to any consumer testing +// PerPeer == nil. +func TestStatusPerPeerIsEmptyNotNilOnAPeerlessLeader(t *testing.T) { + nodes, peers := newTransportTestNodes(t, 1) + startTransportTestServers(nodes, peers) + t.Cleanup(func() { cleanupTransportTestNodes(t, nodes) }) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + require.NoError(t, openTransportTestNode(ctx, nodes[0], peers[:1], true)) + leader := waitForLeaderNode(t, nodes[:1]) + + status := leader.engine.Status() + require.Equal(t, raftengine.StateLeader, status.State) + require.NotNil(t, status.PerPeer, + "a leader with no remote replicas must report an empty map, not nil") + require.Empty(t, status.PerPeer) +} + +// TestStatusPerPeerIsNilOnAFollower pins the nil-versus-empty +// distinction. Only the leader tracks progress, and an empty map on a +// follower would be indistinguishable from "the leader knows about no +// peers" — an operator could read that as a healthy single-node +// cluster. +func TestStatusPerPeerIsNilOnAFollower(t *testing.T) { + nodes, peers := newTransportTestNodes(t, 2) + startTransportTestServers(nodes, peers) + t.Cleanup(func() { cleanupTransportTestNodes(t, nodes) }) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + require.NoError(t, openTransportTestNode(ctx, nodes[0], peers[:1], true)) + leader := waitForLeaderNode(t, nodes[:1]) + require.NoError(t, openTransportTestNode(ctx, nodes[1], peers, false)) + + _, err := leader.engine.AddLearner(ctx, nodes[1].peer.ID, nodes[1].peer.Address, 0) + require.NoError(t, err) + waitForConfigSize(t, nodes[1].engine, 2) + + require.Eventually(t, func() bool { + return nodes[1].engine.Status().State != raftengine.StateLeader + }, 5*time.Second, 25*time.Millisecond) + + require.Nil(t, nodes[1].engine.Status().PerPeer, + "a follower tracks no peer progress and must report nil, not an empty map") +} + +// TestStatusPerPeerExcludesTheLocalNode keeps the map to REMOTE +// replicas. The leader's own tracker entry is about itself and would +// invite an operator to read their own node as a promotion candidate. +func TestStatusPerPeerExcludesTheLocalNode(t *testing.T) { + nodes, peers := newTransportTestNodes(t, 2) + startTransportTestServers(nodes, peers) + t.Cleanup(func() { cleanupTransportTestNodes(t, nodes) }) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + require.NoError(t, openTransportTestNode(ctx, nodes[0], peers[:1], true)) + leader := waitForLeaderNode(t, nodes[:1]) + require.NoError(t, openTransportTestNode(ctx, nodes[1], peers, false)) + + _, err := leader.engine.AddLearner(ctx, nodes[1].peer.ID, nodes[1].peer.Address, 0) + require.NoError(t, err) + waitForConfigSize(t, leader.engine, 2) + + require.Eventually(t, func() bool { + return len(leader.engine.Status().PerPeer) > 0 + }, 10*time.Second, 25*time.Millisecond) + + perPeer := leader.engine.Status().PerPeer + require.NotContains(t, perPeer, nodes[0].peer.NodeID, + "the leader's own tracker entry must not appear among remote peers") + require.Contains(t, perPeer, nodes[1].peer.NodeID) +}