fix(EN-1874): require identities for all Raft members

[gfyrag]

Summary

• require a valid 16-byte instance ID on every membership creation, discovery, persistence, removal, and promotion path
• propagate member identities through PeerInfo and the administrative AddLearner request
• keep administrative AddLearner and fresh-WAL JoinAsLearner as explicit paths instead of inferring intent from a missing identity
• fail startup, ConfChange application, force removal, and checkpoint rehydration loudly on identity-less configured members
• update ledgerctl, architecture and operations documentation, generated protobufs, and E2E fixtures

Stacking

This PR is intentionally stacked on #1794 and should be reviewed and merged before #1794 is merged into the default branch. After this PR lands into the parent branch, #1794 can be merged with the universal member-identity invariant in place.

Validation

• bash scripts/agent-check
• go test -race ./... -timeout 20m
• go test -run ^$ -tags=e2e ./tests/e2e/cluster
• targeted coverage: membership 80.4%, node 30.9%, adapters 44.2%, server 69.8%; new invariant validator 100% and ConfChange identity validator 83.3%

[NumaryBot]

🛑 Changes requested — automated review

The active prior finding remains: payload-less AddNode entries can still introduce identity-less voters. This violates the universal member-identity invariant.

[codecov]

Codecov Report

❌ Patch coverage is 66.16162% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.79%. Comparing base (ff3a105) to head (742f179).

Files with missing lines	Patch %	Lines
internal/infra/node/node.go	39.70%	22 Missing and 19 partials ⚠️
internal/infra/membership/peer_store.go	62.50%	5 Missing and 1 partial ⚠️
internal/application/membership/membership.go	80.00%	2 Missing and 2 partials ⚠️
internal/infra/membership/confchange.go	80.95%	3 Missing and 1 partial ⚠️
internal/infra/membership/membership.go	88.23%	2 Missing and 2 partials ⚠️
internal/storage/wal/instance_id_marker.go	80.95%	2 Missing and 2 partials ⚠️
internal/adapter/grpc/server_bootstrap.go	33.33%	0 Missing and 2 partials ⚠️
internal/bootstrap/module.go	84.61%	1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (66.16%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@               Coverage Diff                @@
##           release/v3.0    #1799      +/-   ##
================================================
- Coverage         76.84%   76.79%   -0.05%     
================================================
  Files               472      472              
  Lines             50356    50445      +89     
================================================
+ Hits              38696    38740      +44     
- Misses             8262     8293      +31     
- Partials           3398     3412      +14     

Flag	Coverage Δ
e2e	76.79% <66.16%> (-0.05%)	⬇️
scenario	76.79% <66.16%> (-0.05%)	⬇️
unit	76.79% <66.16%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[shipfox-ai]

Arbitration — PR #1799 fix(EN-1874): require identities for all Raft members

The core design is sound: the PR enforces a single invariant (every configured Raft member carries a valid 16‑byte instance_id) at every boundary, and the protobuf/CLI/docs/test updates are consistent. I independently verified the disputed code at a15e83c6. Both reviewers converge on two genuine, test‑uncovered defects (silent identity rotation on a partial‑marker loss, and a stale Raft transport after an address‑changing ConfChangeUpdateNode) plus a false documentation claim; these are real and cheap to fix. I also confirmed two minor consistency issues. Because Findings 1 and 3 are blocking correctness/liveness defects (not merely hardening), my recommendation is Request changes — the identity‑enforcement design can stay intact, but these must be addressed before merge.

Agreed findings

1. [Major / blocking] Existing WAL can silently rotate its member identity; the doc claims a protection that never runs. internal/storage/wal/instance_id_marker.go:94 (EnsureInstanceID generates a fresh ID on any missing marker with no WAL/CLUSTER_JOINED cross‑check) → internal/bootstrap/module.go:1435 (shouldRunJoinPreflight returns false on a plain restart because len(Peers)==0, so JoinAsLearner/EN‑1436 never fires) → internal/infra/node/node.go:513 (self row unconditionally re‑registered with the new ID). Losing only INSTANCE_ID while retaining the WAL rotates the locally recorded identity while peers still record the old one. Confirmed. The documentation at docs/technical/architecture/subsystems/consensus/removed-member-registry.md:143 asserting EN‑1436 rejects this case is false and must be corrected; startup should require an INSTANCE_ID when the WAL/CLUSTER_JOINED marker proves an existing incarnation.

2. [Major / blocking] ConfChangeUpdateNode leaves the Raft transport dialing the previous address. internal/infra/membership/membership.go:234 (Set calls only wireAdd) → internal/infra/node/transport.go:341 (DefaultTransport.AddPeer is a true no‑op when the node ID already exists, regardless of address change). A Match==0 refresh that changes the raft address (reachable via administrative AddLearner or a boot‑join with a differing seeded address, node.go:2478/node.go:1382) updates the cache and the service pool but leaves Raft pinned to the old endpoint. This is asymmetric with Rehydrate (membership.go:435), which correctly models an address change as wireRemove+wireAdd. Fix: make Set remove+add on address change and add a transport‑address assertion (existing tests only assert the Pebble row / call counts). Confirmed.

3. [Minor] Invalid discovery identities are retried forever. cmd/server/server.go:829 returns a plain wrapped error for a malformed instance_id, but discoverPeersFromClusterWithRetry (cmd/server/server.go:737) breaks only on codes.Unauthenticated and retries everything else until the lifecycle context is cancelled. The exact invalid state this PR is meant to reject becomes an infinite startup spin. Classify identity‑validation failures as non‑retryable and test the wrapper (the added test only exercises the inner function). Confirmed.

4. [Minor] Admin AddLearner returns codes.Unknown for a malformed identity. internal/adapter/grpc/server_cluster.go:334 returns the application validation error unwrapped (generic Unknown fallback), while the sibling inter‑node path returns codes.InvalidArgument (internal/adapter/grpc/server_bootstrap.go:172). instance_id is the operator‑supplied field most likely to be malformed; ledgerctl validates client‑side, but direct API clients see the inconsistent code. Confirmed.

Ruled disputes

• Finding 2 — one‑sided ConfState→row validation (is it Major/blocking?). Codex: bidirectional check missing, node can run/serve without a row for a configured member; Major (later downgraded to bounded P2, still blocking). Claude: runtime paths (ListPeers, RemoveNode, AddLearner, cluster status) fail loud and reachability is corruption‑only; defense‑in‑depth, not Major. Ruling: the gap is real — ReconcileAgainstConfState (internal/infra/membership/membership.go:366) and LoadAll/Rehydrate only validate rows that exist and never walk ConfState.Voters/Learners to require a row. Severity is bounded/Minor, not an independent hard blocker, because it is only reachable through Pebble corruption, manual deletion, or a self‑inconsistent snapshot source — the healthy write paths register rows before the ConfState. It should still be fixed (add a bidirectional assertion + a missing‑row startup/checkpoint test, run after WAL/checkpoint replay so a legitimately pending row isn't rejected early), because it aligns the code with the PR's own "checkpoint rehydration fails loudly" claim.

• Finding 2 — factual sub‑claims. Claude asserted ListPeers/cluster status surfaces the missing row and that a checkpoint carries ConfState and peer rows "from the same Pebble." Ruling: Codex is correct on both. I verified cluster status routes through GetClusterState/getClusterStateLocal (server_cluster.go:151‑203), which fills empty addresses and does not validate identity; ListPeers has exactly one production caller, bootstrap GetPeers (server_bootstrap.go:106). And ConfState comes from the Raft/WAL snapshot while peer rows live in a separately installed Pebble checkpoint (distinct stores; node.go:1341, internal/infra/state/synchronizer.go). This narrows the runtime fail‑loud coverage Claude relied on.

• Finding 3 — "advertise and blacklist identity B" mechanism. Codex originally claimed the refresh blacklists B; Claude called this factually wrong. Ruling: Claude is correct and Codex conceded. ConfChangeUpdateNode writes only the peer registration; RemovedMemberEntry is written only by RemoveNode/ForceRemoveNode. The accurate impact is that the cache/discovery/promotion use B while Raft still sends to A's endpoint.

• Finding 3 — blast radius / severity. Claude: common identity‑only refresh (stable DNS) is harmless; impact is mostly liveness and the auto‑promote safety edge is narrow. Codex: the API accepts arbitrary replacement addresses with no contract restricting to stable DNS, self‑heal is not guaranteed imminent, so Major/blocking. Ruling: it remains a blocking defect. For an address‑changing refresh the intended endpoint never receives replication (stuck at Match==0, so it is at least not auto‑promoted), and self‑heal only occurs on the leader's next restart/leadership change/snapshot — not guaranteed soon. The dominant realistic impact is degraded liveness on a supported administrative path (Claude's correction of the blast radius stands); Codex's auto‑promote‑of‑a‑live‑old‑endpoint scenario is real but narrow. Either way the asymmetry with Rehydrate is a latent trap and the fix is trivial, so it blocks.

• Service‑pool re‑dial detail. I confirmed Claude's rebuttal point: ConnectionPool.AddPeer (internal/infra/transport/connection_pool.go:186) does tear down and re‑dial on an address change (it no‑ops only when the address is identical). The Rehydrate comment calling pool.AddPeer a "no‑op on existing entries" is therefore imprecise — only the Raft DefaultTransport.AddPeer is a true no‑op on address change. This scopes Finding 3 specifically to the Raft transport and is worth a comment fix.

• ListPeers best‑effort → hard‑fail; poison‑pill node‑fatal apply; breaking CLI/API. Both reviewers agree these are intended/informational consequences of the fail‑loud + "v3 unreleased, no compat" policy, not independent defects. Ruling: agreed — informational, non‑blocking.

---

Reviewed by Claude (claude-opus-4-8) and Codex (gpt-5.6-sol) via Shipfox; arbitrated by Claude.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1799 (comment)

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).

Summary: #1799 (comment)

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1799 (comment)

[NumaryBot]

🟠 [major] Reject payload-less AddNode for unknown members

When a committed ConfChangeAddNode has no registration payload, this validator always treats it as a promotion, but etcd/raft will also accept that shape for a previously unknown node and create an identity-less voter. A malformed or corrupted WAL entry can therefore pass both validation sites and mutate ConfState without a membership row, contrary to the universal identity invariant; only allow this exception after confirming the node is already a learner. This is also required by the impossible-state guardrail in AGENTS.md.

[Azorlogh]

Agree, and the consequence is worse than stated: under this PR's hard checks a voter created this way is unremovable — RemoveNode and ForceRemoveNode both fail on the missing membership row, and ListPeers errors permanently, which also breaks discovery for new joiners. Pre-PR the empty-identity path could still remove such a node. A row-existence check for payload-less AddNode in WriteConfChange/finishReady closes it (a promotion implies a previously registered learner).

Related nit for the same fix: HasPeerRegistration is true if any field is set, so a registration payload carrying a valid instance_id but empty addresses also passes both sites and lands an address-less row — worth validating the full payload here too.

[Azorlogh]

Reviewed at 742f179. The identity invariant is enforced coherently at every boundary I traced (peer store, FSM WriteConfChange, finishReady apply, remove/force-remove/auto-promotion, discovery, both gRPC surfaces), ForceRemoveNode validates before mutating RawNode, and the EnsureInstanceID fail-closed check is backed by the Fx ordering fix (provideWAL depending on NodeConfig) with a test pinning it. The UpdateNode transport rewire resolves the earlier blocker. Tests and docs match the code.

Remaining points:

• I agree with NumaryBot's open payload-less-AddNode finding — replied in-thread, the consequence is worse than stated (the resulting member is unremovable).
• One inline comment: a small race the new ListPeers identity check introduces.
• The PR body's stacking section is stale: #1794 is already merged, this branch sits on plain release/v3.0.

[Azorlogh]

The member list is snapshotted inside execClusterCommand, but this cache read happens afterwards on the RPC goroutine — a removal committing in between makes ListPeers fail with invariant: cluster member N has no membership row for a state that was never inconsistent (spurious GetPeers/discovery error, and the "invariant" wording is misleading there). Capturing the identities inside the same closure would close it.