From fb255fa8ea67cbd2a38cc8da01b61ab617ee5b3f Mon Sep 17 00:00:00 2001 From: bootjp Date: Wed, 9 Sep 2026 13:38:26 +0900 Subject: [PATCH 1/2] raftengine: report per-peer replication progress in Status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the Milestone 3 hardening item from the learner design §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. Status.PerPeer now reports each remote replica's Match, Next, IsLearner and RecentActive from the leader's tracker. IsLearner comes from the live tracker rather than the peers file, so it reflects what raft will do rather than what was last persisted. It is nil on a follower, not an empty map. Only the leader tracks progress, and an empty map would be indistinguishable from "the leader knows about no peers" — which an operator could read as a healthy single-node cluster. refreshStatus runs on every Ready, so this uses rawNode.WithProgress rather than rawNode.Status(): the latter deep-copies the whole progress map plus the config, while WithProgress visits in place and lets the engine allocate one small map, sized to the peer count, only while leading. Exposing the field over the RaftAdmin Status RPC needs a proto change and the pinned toolchain (libprotoc 29.3; this machine has 34.0), so it stays a follow-up rather than a hand-edited generated file. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE --- .../2026_04_26_implemented_raft_learner.md | 9 +- internal/raftengine/engine.go | 31 +++++ internal/raftengine/etcd/engine.go | 37 ++++++ .../raftengine/etcd/status_perpeer_test.go | 117 ++++++++++++++++++ 4 files changed, 191 insertions(+), 3 deletions(-) create mode 100644 internal/raftengine/etcd/status_perpeer_test.go diff --git a/docs/design/2026_04_26_implemented_raft_learner.md b/docs/design/2026_04_26_implemented_raft_learner.md index 011c5abbe..d8b61a491 100644 --- a/docs/design/2026_04_26_implemented_raft_learner.md +++ b/docs/design/2026_04_26_implemented_raft_learner.md @@ -874,7 +874,10 @@ 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. 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 +887,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..5f81cb37a 100644 --- a/internal/raftengine/engine.go +++ b/internal/raftengine/engine.go @@ -120,6 +120,37 @@ 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 choose PromoteLearner's + // min_applied_index from the learner's actual Match instead of + // guessing: guessing high fails the precondition and guessing low + // promotes a replica that has not caught up. + 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. This is the value PromoteLearner's min_applied_index is + // compared against. + 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..71e9742da 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,41 @@ 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 + } + var out 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 + } + if out == nil { + out = make(map[uint64]raftengine.PeerProgress) + } + 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 +3918,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..c6ac9c9cb --- /dev/null +++ b/internal/raftengine/etcd/status_perpeer_test.go @@ -0,0 +1,117 @@ +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 reported Match is a usable min_applied_index: promotion with + // it must satisfy the precondition rather than be rejected. + _, err = leader.engine.PromoteLearner(ctx, nodes[1].peer.ID, 0, progress.Match, false) + require.NoError(t, err, + "the Match reported by PerPeer must be accepted as min_applied_index") +} + +// 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) +} From e649ac829c2e22d7d46a5eb2c317f44b30e8fbef Mon Sep 17 00:00:00 2001 From: bootjp Date: Thu, 10 Sep 2026 14:18:09 +0900 Subject: [PATCH 2/2] raftengine: correct the PerPeer promotion guidance and peerless case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, the first on guidance that was actively wrong. PerPeer's doc and its test both presented the learner's current Match as a usable min_applied_index. It is not: the engine checks Match >= minAppliedIndex, so a learner's own present value satisfies it by construction and the precondition becomes a no-op. A replica at Match=10 against a leader committed through 100 would be promoted, joining the voter quorum before catching up and potentially stalling writes immediately. The field now documents watching Match climb to a target — the same snapshot's CommitIndex — and passing the TARGET, and the test demonstrates that pattern instead of the broken one. peerProgress allocated its map lazily inside the visitor, so a single-node leader — which visits only itself, and is skipped — returned nil. That made a healthy peerless leader indistinguishable from a follower under the documented PerPeer == nil test. The map is now allocated up front when leading. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE --- .../2026_04_26_implemented_raft_learner.md | 7 ++- internal/raftengine/engine.go | 26 ++++++++--- internal/raftengine/etcd/engine.go | 10 +++-- .../raftengine/etcd/status_perpeer_test.go | 43 +++++++++++++++++-- 4 files changed, 71 insertions(+), 15 deletions(-) diff --git a/docs/design/2026_04_26_implemented_raft_learner.md b/docs/design/2026_04_26_implemented_raft_learner.md index d8b61a491..9f2caa3f1 100644 --- a/docs/design/2026_04_26_implemented_raft_learner.md +++ b/docs/design/2026_04_26_implemented_raft_learner.md @@ -876,7 +876,12 @@ single-process 3-node demo cluster, attaches a learner via `Status.PerPeer` so an operator can choose `min_applied_index` 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. Exposing it over 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. diff --git a/internal/raftengine/engine.go b/internal/raftengine/engine.go index 5f81cb37a..f93a7c21d 100644 --- a/internal/raftengine/engine.go +++ b/internal/raftengine/engine.go @@ -126,10 +126,23 @@ type Status struct { // empty map there would be indistinguishable from "the leader // knows about no peers". // - // It exists so an operator can choose PromoteLearner's - // min_applied_index from the learner's actual Match instead of - // guessing: guessing high fails the precondition and guessing low - // promotes a replica that has not caught up. + // 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 } @@ -137,8 +150,9 @@ type Status struct { // leader's tracker. type PeerProgress struct { // Match is the highest log index known to be replicated to this - // peer. This is the value PromoteLearner's min_applied_index is - // compared against. + // 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 diff --git a/internal/raftengine/etcd/engine.go b/internal/raftengine/etcd/engine.go index 71e9742da..f73141ebe 100644 --- a/internal/raftengine/etcd/engine.go +++ b/internal/raftengine/etcd/engine.go @@ -3874,16 +3874,18 @@ func (e *Engine) peerProgress(state raftengine.State) map[uint64]raftengine.Peer if state != raftengine.StateLeader { return nil } - var out map[uint64]raftengine.PeerProgress + // 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 } - if out == nil { - out = make(map[uint64]raftengine.PeerProgress) - } out[id] = raftengine.PeerProgress{ Match: pr.Match, Next: pr.Next, diff --git a/internal/raftengine/etcd/status_perpeer_test.go b/internal/raftengine/etcd/status_perpeer_test.go index c6ac9c9cb..dcea10153 100644 --- a/internal/raftengine/etcd/status_perpeer_test.go +++ b/internal/raftengine/etcd/status_perpeer_test.go @@ -51,11 +51,46 @@ func TestStatusPerPeerReportsLearnerProgressOnTheLeader(t *testing.T) { "progress must come from the live tracker, which knows this peer is a learner") require.GreaterOrEqual(t, progress.Next, progress.Match) - // The reported Match is a usable min_applied_index: promotion with - // it must satisfy the precondition rather than be rejected. - _, err = leader.engine.PromoteLearner(ctx, nodes[1].peer.ID, 0, progress.Match, false) + // 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, - "the Match reported by PerPeer must be accepted as min_applied_index") + "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