Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[update-engine] fix GroupDisplayStats to avoid integer underflow #4561

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Nov 27, 2023

This could happen if an empty EventReport is passed in -- in that case we'd
transition to Running but return NotStarted.

Fix this by not transitioning self.kind to Running if we're going to return
NotStarted. This does bloat up the code a little but I think is clearer
overall.

Thanks to @jgallagher for all the help debugging this!

Also clean up some related logic and add tests.

Fixes #4507.

Created using spr 1.3.5
@@ -236,18 +236,9 @@ impl GroupDisplayStats {
}

fn apply_result(&mut self, result: AddEventReportResult) {
// Process result.after first to avoid integer underflow.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't really good enough to avoid integer underflow 😅, so I decided to just check for equality and then do the more obvious thing of applying before before after.

Created using spr 1.3.5
Created using spr 1.3.5
Copy link
Contributor

@jordanhendricks jordanhendricks left a comment

Choose a reason for hiding this comment

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

nice find!

@sunshowers sunshowers enabled auto-merge (squash) November 28, 2023 00:52
Created using spr 1.3.5
Created using spr 1.3.5
@sunshowers sunshowers merged commit b9d8b8f into main Nov 28, 2023
20 checks passed
@sunshowers sunshowers deleted the sunshowers/spr/update-engine-fix-groupdisplaystats-to-avoid-integer-underflow branch November 28, 2023 03:08
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.

[wicket] "Running" count wrong in rack-update attach
2 participants