Skip to content

Comments

AppVersion.CompareTo missing else if breaks comparison symmetry#8051

Merged
Aaronontheweb merged 2 commits intoakkadotnet:devfrom
MattKotsenas:bugfix/appversion-compare
Feb 24, 2026
Merged

AppVersion.CompareTo missing else if breaks comparison symmetry#8051
Aaronontheweb merged 2 commits intoakkadotnet:devfrom
MattKotsenas:bugfix/appversion-compare

Conversation

@MattKotsenas
Copy link
Contributor

@MattKotsenas MattKotsenas commented Feb 23, 2026

Changes

The second condition in the rest-string comparison was an 'if' instead of 'else if', causing the first branch's result (diff = 1) to be immediately overwritten by the else branch. This made release versions appear less than their pre-release counterparts (e.g. '1.2.0' < '1.2.0-M1'), violating IComparable symmetry.

I believe this could cause non-deterministic shard allocation ordering during rolling updates from pre-release to release versions via AbstractLeastShardAllocationStrategy. I also added regression test for the reverse comparison direction.

Checklist

The second condition in the rest-string comparison was an 'if' instead
of 'else if', causing the first branch's result (diff = 1) to be
immediately overwritten by the else branch. This made release versions
appear less than their pre-release counterparts (e.g. '1.2.0' <
'1.2.0-M1'),
violating IComparable<T> symmetry.

This could cause non-deterministic shard allocation ordering during
rolling updates from pre-release to release versions via
AbstractLeastShardAllocationStrategy.

Added regression test for the reverse comparison direction.
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM - nice catch

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) February 24, 2026 20:31
@Aaronontheweb Aaronontheweb added this to the 1.5.61 milestone Feb 24, 2026
@Aaronontheweb Aaronontheweb merged commit 71f0350 into akkadotnet:dev Feb 24, 2026
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants