Skip to content

[FIXED] NRG: Always only report leader after noop-entry#7460

Merged
neilalexander merged 1 commit intomainfrom
maurice/nrg-leader
Oct 28, 2025
Merged

[FIXED] NRG: Always only report leader after noop-entry#7460
neilalexander merged 1 commit intomainfrom
maurice/nrg-leader

Conversation

@MauriceVanVeen
Copy link
Copy Markdown
Member

This PR fixes an issue where leadership reporting would be a little too soon. It would wait for all entries in the current log to be committed and applied, but it would not wait for the NOOP-entry to be applied. This is no issue per se for meta/streams/consumers/ normally, but it becomes an issue when using atomic batching in 2.12 since these could span multiple append entries.

If an atomic batch is only partially proposed in separate append entries (so it will not commit), this batch will always be rejected properly. But, because we don't wait for the apply of the NOOP-entry (in our case the peerstate) this means leadership is signaled before the batch is rejected. This results in subscribing to the stream subjects and possibly receiving new messages before we apply the peerstate. Which initializes the mset.clseq with the correct value, but using the wrong mset.clfs value. The batch is then not accounted for in the mset.clfs yet, which could then result in one atomic batch to be only partially committed.

Even though this fix only affects atomic batching in 2.12, it's still a fix that can be backported to 2.11.

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 October 22, 2025 16:10
// Wait for this message (and potentially more) to be applied.
// It's important to wait signaling we're leader if we're not up-to-date yet, as that
// would mean we're in a consistent state compared with the previous leader.
n.sendPeerState()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We probably only want this to happen if n.switchState() returns success?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think this is necessary. We only call n.switchToLeader if we previously ran as candidate and won the vote. That means n.switchState(Leader) will always return true. Not sure why I added the if leadChange before.

Copy link
Copy Markdown
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

LGTM

@neilalexander neilalexander merged commit 98bfe6d into main Oct 28, 2025
69 of 70 checks passed
@neilalexander neilalexander deleted the maurice/nrg-leader branch October 28, 2025 12:13
neilalexander added a commit that referenced this pull request Oct 28, 2025
Includes the following:

- #7380
- #7384
- #7385
- #7388
- #7395
- #7400
- #7399
- #7401
- #7402
- #7423
- #7424
- #7411
- #7428
- #7429
- #7431
- #7435
- #7433
- #7443
- #7455
- #7465
- #7466
- #7460
- #7484
- #7479

Signed-off-by: Neil Twigg <neil@nats.io>
neilalexander added a commit that referenced this pull request Oct 30, 2025
Includes the following:

- #7435
- #7433
- #7436
- #7443
- #7440
- #7444
- #7452
- #7455
- #7458
- #7465
- #7466
- #7474
- #7469
- #7460
- #7449
- #7484
- #7479
- #7486
- #7495
- #7482
- #7496

Signed-off-by: Neil Twigg <neil@nats.io>
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.

2 participants