Skip to content

[FIXED] Combined stream scale up/down and subjects/sources update#8003

Open
MauriceVanVeen wants to merge 3 commits intomainfrom
maurice/stream-scale-subj-update
Open

[FIXED] Combined stream scale up/down and subjects/sources update#8003
MauriceVanVeen wants to merge 3 commits intomainfrom
maurice/stream-scale-subj-update

Conversation

@MauriceVanVeen
Copy link
Copy Markdown
Member

@MauriceVanVeen MauriceVanVeen commented Apr 4, 2026

A stream that was scaled up from R1 along with its subjects being updated would not subscribe to the new subjects. This PR fixes that by just subscribing to the new subjects when we were already actively subscribed to the others.

There was a similar issue where if a stream got scaled down to R1 but it wasn't the previous stream leader, the new server would not subscribe to any subjects or remove previous sources and require a restart.

Resolves #7996

Signed-off-by: Maurice van Veen github@mauricevanveen.com

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
@MauriceVanVeen MauriceVanVeen requested a review from a team as a code owner April 4, 2026 16:04
@synadia-claude-reviewer
Copy link
Copy Markdown

synadia-claude-reviewer bot commented Apr 4, 2026

Claude finished @MauriceVanVeen's task in 1m 55s —— View job


PR Review

  • Gather context
  • Read PR diff and changed files
  • Analyze correctness and concurrency safety
  • Post review findings

The fix is correct and minimal. Here's the analysis:

Root cause: When scaling R1→R3 and updating subjects simultaneously, updateWithAdvisory is called while the stream is transitioning from non-clustered to clustered. At that moment isClustered() becomes true, so isLeader() delegates to mset.node.Leader() — but the Raft node may not yet be elected, returning false. Meanwhile, mset.active is still true from the R1 phase (subscriptions are live). The old guard if mset.isLeader() skipped the subscription update, leaving "b" unsubscribed.

Why mset.active is the right guard:

  • active is set to true only by subscribeToStream(), which is called exclusively from setLeader(true) — meaning it accurately tracks whether subscriptions are live and need updating.
  • For steady-state followers, setLeader(false) calls unsubscribeToStream()active = false, so the behavior for normal followers is unchanged.
  • The only case where active == true and isLeader() == false is this exact transitional window — precisely where the fix is needed.

Concurrency: The check is under mset.mu.Lock(), and the subsequent subscribe/unsubscribe calls are correct. No races introduced.

Test: TestJetStreamClusterStreamUpdateCombinedScaleUpWithSubjectsChange directly exercises the failing scenario. Using checkFor with a 2s window accommodates leader election time. The AckWait(200ms) inside the retry loop is fine.

No issues found. LGTM.

@MauriceVanVeen MauriceVanVeen changed the title [FIXED] Combined stream scale up and subjects update [FIXED] Combined stream scale up/down and subjects update Apr 5, 2026
@MauriceVanVeen
Copy link
Copy Markdown
Member Author

@claude, brief re-review please, but also take the scale down issue into account.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f818f3a17

ℹ️ 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 MauriceVanVeen force-pushed the maurice/stream-scale-subj-update branch from 5f818f3 to e916c63 Compare April 5, 2026 10:13
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e916c63538

ℹ️ 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".

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
@MauriceVanVeen MauriceVanVeen force-pushed the maurice/stream-scale-subj-update branch from e916c63 to e24271a Compare April 5, 2026 10:51
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e24271a6fc

ℹ️ 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".

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
@MauriceVanVeen MauriceVanVeen changed the title [FIXED] Combined stream scale up/down and subjects update [FIXED] Combined stream scale up/down and subjects/sources update Apr 5, 2026
@MauriceVanVeen
Copy link
Copy Markdown
Member Author

@claude, brief re-review please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

STREAM.UPDATE with replica scale-down drops internal subscriptions for re-added subjects

1 participant