(2.15) [ADDED] Desired state reconcilitation for scaling and moves

[MauriceVanVeen]

This PR adds initial support for desired state reconciliation of stream and consumer updates that change the peer set. These, and follow-up, changes intend to fix loads of related issues surrounding peer set management of assets. For example, including but not limited to: a scale down not being guaranteed to select the right leader or servers with all data to prevent data loss, peers re-appearing in the stream/consumer Raft group after peer-removing a node, moving and scaling a stream concurrently may brick it such that it's stuck halfway into a move or is reverted back to the previous cluster with the stream config still referencing where it was meant to move to, and a consumer reverting to stale state after a consumer move.

This initial PR tries to do the minimal amount of work to have passing CI, while transitioning to a desired state design that allows for guaranteed and safe scaling and moving of assets. Importantly:

• The meta leader MUST never overwrite a stream/consumer peer set in-place as it does today. The meta leader may dictate where a stream/consumer needs to move, but the stream/consumer is responsible for making those changes be guaranteed safe.
• The meta leader always dictates the peer set, unless the stream/consumer is scaled down, since only the group itself knows which servers would be best to be preserved (i.e. the leader and most up-to-date servers), to guarantee no data loss. Previously the meta leader did a sysRequest stream/consumer info to figure out the leader (and for consumer moves the consumer state). Both could result in a stale leader/state being picked. And worse, if the leader doesn't respond in time (or at all), the meta leader would randomly pick a leader (or lose consumer state). This PR allows the removal of sysRequest entirely.
• Peer additions and removals are managed by the group's leader, no automatic additions of peers are allowed (like for the meta group). Before the group's leader is allowed to make any change, it first needs to communicate with the meta leader to assert that it's up-to-date with the meta state (linearizable and no stale local assignment), and the leader's Raft term is also wired through, so the meta leader also knows to ignore requests from stale leaders. This PR allows removing ProposeKnownPeers and UpdateKnownPeers from raft.go, as well as the calls to them in js.createRaftGroup, since those were unsafe and could result in removed peers re-appearing, desyncing the peer set in the meta assignment with that of the group.
• The stream/consumer ClusterInfo now returns the desired state information. Allowing tooling (like the CLI) to inspect the state as it changes.

This PR is split in 3 commits, to ease reviewing but also ease backporting to 2.14. Put simply: the first commit adds the models, the second commit adds meta layer reconciliation awareness, and the last commit actually starts populating the desired state and triggering the stream/consumer migration paths.

Likely 2.14 will require a compatibility commit to ease downgrading, and upgrading/downgrading as a whole should be tackled in a follow-up PR. However, the design of the desired state always guarantees: the stream/consumer config is always what the user asked it to be, the group's peer set is always the one that equals the underlying Raft group (or if the Raft group contains less peers, they get automatically added under the old behavior). So, worst-case given a downgrade to an incompatible version, the desired state configuration in the JSON will "fall off" and the assignment will remain "stuck" on which peers it was at that time. That does not violate correctness or safety when it comes to preserving data, it does however prevent the stream/consumer from moving to the desired config.

Having passing CI and the mentioned upgrade/downgrade focus isn't enough, and at least these follow-ups will be required in separate PRs:

• The system "move" and "move cancel" APIs should be made more reliable. A "move" can simply trigger a peer set change through desired state, and a "move cancel" can look at the desired state's origin to see where the stream needs to be moved back to. Both just need time to be wired appropriately.
• Server and asset-level peer-remove should be made desired state aware. Additionally, an "evacuate" endpoint should be added to enable the user to more easily move streams and consumers off a server while it's still online. Not only easing the decommissioning of a node, but also enabling to move data of R1 assets without losing data when a peer-remove of the node would've been used instead and without requiring to move all R1 assets in all accounts manually.
• Add stream move/scale guards. This PR removes the (admittedly brittle) guards to prevent concurrent move and scale of a stream, which allows to scale and move an R1 stream from cluster A to a R3 stream in cluster B in one go and safely. However, this technically opens up being able to constantly cycle moves between clusters until the stream/consumer peer set contain the total number of peers. This should likely get an upper bound, for example 10 peers maximum (since replication is capped at 5), to ensure such moves are still allowed from the API, but the system prevents the peers from growing semi-unbounded.
• Optimizations in how and when peers are added, ensuring a group doesn't go out of quorum when nodes are offline.
• An additional optimization can be made when moving a stream between clusters, to first scale up one stream replica in the new cluster, making it the leader and then doing local catchup for the other replicas in that cluster. This would allow both faster and less expensive (bandwidth-wise) moves of assets. But, although this is enabled by the desired state approach as well, it might need to be done after 2.15.0 lands considering timing and the other required follow-ups (this isn't a required optimization).

These follow-ups are also roughly marked with FIXME comments that need to be addressed prior to releasing 2.15.

[synadia-claude-reviewer]

Claude finished @MauriceVanVeen's task in 8m 49s —— View job

---

PR Review: (2.15) [ADDED] Desired state reconciliation for scaling and moves

• Gather context and load tools
• Read PR diff
• Examine key source files for context
• Analyze for correctness, concurrency safety, raft correctness
• Post inline comments on specific issues
• Post overall review summary

Review complete. Found 6 issues across correctness, API semantics, and minor concurrency concerns — see inline comments and the overall summary comment.

[synadia-claude-reviewer]

Overall Review

This is a well-designed and thorough rework of the JetStream peer-set management. The core ideas — having the group's own leader drive membership changes via a linearizable desired-state handshake with the meta leader, fencing with Raft term numbers, and eliminating the blocking sysRequest calls that could produce stale decisions — are sound and address a class of hard-to-fix race conditions in the existing code. The commit split (models → meta awareness → desired-state population) makes the change legible.

A few correctness and API concerns are worth addressing before shipping:

High priority

• processAddPeer corrupts desired state (inline comment, line 2665 jetstream_cluster.go): When processAddPeer fires for a peer that is already in sa.Group.Peers during a scale-up migration, d.Peers is overwritten with a slice that contains a duplicate entry, silently corrupting the migration target. The fix is a guard against peers already present in the assignment before the append/desired-state update.

Medium priority

• Wrong leader in clusterInfo when rg.node == nil but rg.Desired != nil (inline, line 11571): The node-nil early-return guard was relaxed, so the fallback ci.Leader = s.Name() can now be returned when no raft node exists yet — reporting the local server as leader regardless of reality.

• PeerInfo.Current semantic mismatch in DesiredClusterInfo.Replicas (inline, line 11650): Current = true is set to mean "this is the local server", but the field's established contract throughout the codebase is "this peer is caught-up with the leader." Clients and tooling that already parse ClusterInfo.Replicas will misinterpret the new desired list.

• withDesired does not copy the desired peers slice (inline, line 226): ng.Desired.Peers = target.Peers aliases the caller's slice. Currently safe because callers discard target, but this is one refactor away from a hidden mutation bug. copyStrings(target.Peers) matches the copy discipline used elsewhere.

Low priority / minor

• Timer Reset without draining (inline, line 3436): mmt.Reset() is called from a non-timer select branch while the timer may have already fired, risking a spurious extra migration cycle. Harmless in practice but violates the Go timer contract.
• js.mu write lock held across meta.Propose and API error responses (inline, line 2828): No deadlock risk (raft uses its own lock), but can add latency to the JS meta lock under concurrent operations.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: acb7f422be

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[MauriceVanVeen]

@claude, re-review.

[synadia-claude-reviewer]

Claude finished @MauriceVanVeen's task in 9m 48s —— View job

---

Re-review

• Gather context and load tools
• Read current diff to see changes since original review
• Verify fixes for items marked "Done" by author
• Check items author explicitly deferred or declined
• Look for any new issues in the code
• Post re-review summary

---

Note: The branch still contains only the original 3 commits (last one pushed at 12:26 UTC, before the review was posted at 13:23 UTC). The author's "Done" / "Removed" responses represent intended changes that have not yet been pushed to this branch. This re-review is of the current code as-is, with commentary on each item.

---

Items from original review

1. processAddPeer corrupts desired state — unfixed, correctly deferred

Lines 2660–2666: the duplicate-append and wrong comment ("it must not retain the removed peer" is copy-pasted from remapStreamAssignment, wrong for a peer-add path) are still present. The author's deferral to a follow-up PR is reasonable since this path is independent of the new reconciliation loop, but the stale comment should be corrected when that work lands.

2. Wrong leader when rg.node == nil but rg.Desired != nil — already correct in current code

Line 11555: rg.node == nil && rg.Desired == nil is already the guard. When rg.Desired != nil and n == nil, the function falls through; ci.Leader stays as "" (zero value of string) since the if n != nil block at line 11597 is skipped. No fix was needed — the code was already correct.

3. PeerInfo.Current semantic mismatch — already correct in current code

The generatePeer function at line 11648 sets Current: false for all desired replicas. The original concern about Current: true does not match the code. Either the reviewer saw a draft that was superseded, or the code was already correct at submission.

4. Timer Reset without draining — already correct in current code

Both monitorStream (lines 3432–3441) and monitorConsumer (lines 6900–6908) already use the correct Go timer reset idiom: Stop(), drain channel if not stopped, then Reset(). No fix was needed here either.

5. withDesired shared slice alias (line 226) — author accepted as-is

ng.Desired.Peers = target.Peers still aliases the caller's slice. The author's position — that the existing function-comment disclaimer ("target MUST already be a copy or fresh group, as it's directly referenced") is sufficient — is defensible. The same aliasing also appears at line 9274 (desiredRaftGroupOrigin.Peers: osa.Group.Peers) where the origin's peer list is a direct reference rather than copyStrings(osa.Group.Peers). This follows the same pattern and is safe as long as callers (and their JSON serialization path) treat the references as read-only.

6. js.mu write lock held across meta.Propose — partially addressed, pending push

jsLeaderServerStreamCancelMoveRequest (added in this diff) correctly uses individual js.mu.Unlock() calls around each Propose + error path. Good.

However reconcileDesiredStreamAssignment (line 7917–7918) and reconcileDesiredConsumerAssignment (line 7975–7976) still use defer js.mu.Unlock(), meaning the write lock is held for the duration of all cc.meta.Propose() calls inside those functions (lines 7951, 7959, 8016). The author stated they replaced these with individual unlocks — that change has not yet been pushed to the branch.

---

New observation

runConsumerMigration uses js.streamAssignment() (non-inflight) while reconcileDesiredConsumerAssignment uses streamAssignmentOrInflight()

Line 7061 in runConsumerMigration:

osa := js.streamAssignment(accName, streamName)

Line 7983 in reconcileDesiredConsumerAssignment:

osa := js.streamAssignmentOrInflight(reconcile.Account, reconcile.Stream)

In runConsumerMigration, osa.Group.Peers is the committed stream peer set. A consumer migration cycle will only propose a peer P once P appears in the committed stream assignment (line 7160). This makes the consumer migration naturally conservative — it trails the stream migration by one reconcile cycle. That is correct and intentional. The meta leader's sanity check (line 7999) uses the inflight-aware version so it will accept any peer already in the stream's committed-or-inflight assignment, which is a superset of what the consumer migration will ever propose. No bug here, but worth documenting the asymmetry.

---

Overall assessment

The architecture is sound and the core invariants are well-maintained: the meta leader never overwrites the peer set in-place; group leaders drive membership changes after establishing fencing via their Raft term; the managed flag in raft.go correctly disables automatic peer addition for stream/consumer groups; and the scale-down peer selection logic (sortScaleDownPeers → selectScaleDownPeers / selectPeerToRemove) is correct.

The main things to land before merging:

1. Push the promised reconcileDesiredStreamAssignment / reconcileDesiredConsumerAssignment individual-unlock refactor (item 6 above).
2. Fix the stale copy-paste comment in processAddPeer (item 1), even if the bug fix is deferred.
3. Address the FIXME(mvv) in reconcileDesiredState when rg.Desired == nil (line 8029) before relying on it for pre-existing move state.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 63d1d0d842

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[MauriceVanVeen]

@claude, re-review. All changes have been pushed. Changes to reconcileDesiredStreamAssignment and reconcileDesiredConsumerAssignment were not promised, the change was purely scoped to jsLeaderServerStreamCancelMoveRequest.

[synadia-claude-reviewer]

Claude finished @MauriceVanVeen's task in 6m 30s —— View job

---

Re-review

• Gather context and load tools
• Read current diff to see changes since second review
• Verify fixes for items marked "Done" by author
• Check deferred/clarified items (especially reconcileDesired* lock handling)
• Look for any new issues in the code
• Post re-review summary

---

Note: The branch still has the same 3 commits from 12:26 UTC. The author's "Done" responses have been verified against the current code — all claimed fixes were already present in those commits. The reconcileDesiredStreamAssignment / reconcileDesiredConsumerAssignment individual-unlock change was explicitly not promised; only jsLeaderServerStreamCancelMoveRequest was changed.

---

Status of all prior items

1. processAddPeer corrupts desired state — deferred, stale comment now fixed

The original concern (duplicate-append guard) is still deferred to a follow-up PR. However, the stale copy-paste comment has been corrected: line 2662 now reads // Keep the desired peer set in sync. (replacing the wrong "it must not retain the removed peer"). ✓

2. Wrong leader when rg.node == nil — already correct (confirmed)

Guard at line 11561: rg.node == nil && rg.Desired == nil. When Desired != nil and n == nil, the code falls through and ci.Leader remains "" (zero value). Correct. ✓

3. PeerInfo.Current semantic mismatch — already correct (confirmed)

generatePeer() at line 11654 always sets Current: false. No issue. ✓

4. Timer Reset without draining — already correct (confirmed)

resetMigrationMonitoring (line 3431) uses Stop() + drain then Reset(). Correct idiom in both monitorStream and monitorConsumer. ✓

5. withDesired shared slice alias — accepted as-is

ng.Desired.Peers = target.Peers (line 226) still aliases. Consistent with the function's caller contract. ✓

6. js.mu write lock in reconcileDesiredStreamAssignment / reconcileDesiredConsumerAssignment — accepted as-is, change was never promised

These functions still use defer js.mu.Unlock() (lines 7918, 7982), holding the write lock across meta.Propose. Author's position — that this is required for correctness — stands. meta.Propose uses its own raft lock and does not re-acquire js.mu, so there is no deadlock. Only jsLeaderServerStreamCancelMoveRequest switched to individual unlocks, which is what the author actually promised. ✓

7. jsLeaderServerStreamCancelMoveRequest individual unlocks — confirmed fixed

Lines 2797, 2807, 2830, 2834, 2837: each exit path unlocks explicitly. ✓

8. Codex P1: Propagate desired scale-ups into RaftConfig — deferred, accepted

Author noted that single-at-a-time peer addition makes observer mode unnecessary for now. ✓

9. Codex P2: Clone config before mutating — confirmed fixed

Line 2791 cfg = *sa.Config.clone(), then csa.Config = &cfg at line 2819. Config is cloned before any mutation. ✓

10. Codex P2: Reconcile legacy desired state before remapping consumers — confirmed fixed

Line 7939: legacyMove := osa.Group.Desired == nil && len(osa.Group.Peers) > osa.Config.Replicas. Consumer remapping is skipped when legacyMove is true, allowing desired state to be initialized on the next cycle before consumers are remapped. ✓

11. Codex P2: Select desired replacements from target cluster — confirmed fixed

Lines 8321–8334 in remapStreamAssignment: when rg.Desired != nil, baseCluster = d.Cluster (the target cluster) is used for peer group selection, not sa.Group.Cluster. ✓

---

New observation

withDesired preserves Desired.Origin during cancel-move

In jsLeaderServerStreamCancelMoveRequest (line 2823): csa.Group = sa.Group.withDesired(csa.Group). Because sa.Group.Desired.Origin != nil (checked at line 2805–2810), withDesired will copy this origin into the new desired state's Origin field (line 231–234 in withDesired). During the cancel-move window, ClusterInfo.Desired.Origin will therefore still be populated, which may mislead tooling into treating an in-progress cancel as a still-cancellable forward move. The FIXME(mvv) comment on line 2824 captures this. It is not a blocking correctness issue (desired state is cleared entirely once the stream converges back to the origin peers), but setting ng.Desired.Origin = nil explicitly after the withDesired call would eliminate the confusing intermediate state.

The streamAssignment() vs streamAssignmentOrInflight() asymmetry noted in the second review is still present and still not a bug — intentional conservative consumer migration noted for documentation.

---

Overall assessment

All promised fixes are confirmed in the code. The architecture and invariants remain sound. The remaining FIXME(mvv) comments are all tracking known follow-up items (move API refactor, cancel-move origin exposure, peer-remove improvements, quorum-aware peer addition). No blocking issues. Ready to merge pending CI.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1cd26e328b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 285fca49bd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cf0fb3f29e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2634f8bdd6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[neilalexander]

LGTM