Repository navigation
fix(propeller): clamp ArrayNode subNode delta timestamps instead of panicking - #8270
Open
BhavayGoyal wants to merge 1 commit into
Open
BhavayGoyal wants to merge 1 commit into
BhavayGoyal wants to merge 1 commit into
Conversation
…anicking The delta between the ArrayNode and a subNode start time was stored in SubNodeDeltaTimestamps unclamped. A subNode starting more than MaxDeltaTimestamp after the ArrayNode, or a negative delta after the ArrayNode attempt was retried or recovered (wrapping to a huge uint64), made CompactArray.SetItem panic on every evaluation, so the workflow could never make progress again. Saturate the delta to the range the array item can hold. Signed-off-by: Bhavay Goyal <bhavaygoyal4b@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8270 +/- ##
==========================================
- Coverage 57.43% 57.35% -0.09%
==========================================
Files 925 931 +6
Lines 57772 58346 +574
==========================================
+ Hits 33184 33463 +279
- Misses 21583 21832 +249
- Partials 3005 3051 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tracking issue
Closes #8144
Why are the changes needed?
The ArrayNode handler stores each subNode's start offset (seconds since the ArrayNode's
LastAttemptStartedAt) inSubNodeDeltaTimestamps, aCompactArraysized byMaxDeltaTimestamp(3 days by default, so 18-bit items, max 262143). The value was written unclamped:So
SetItempanics (Value [267605] is too big. Max value is [262143].) when:uint64gives a huge value.The value is never stored, so the panic repeats on every round until the workflow runs out of system retries.
What changes were proposed in this pull request?
deltaTimestampSecondsinarray/handler.go, which returns the delta in whole seconds saturated to[0, 2^ItemSize - 1](the limitCompactArray.SetItemactually checks). It uses the array's ownItemSize, so it stays correct ifmax-delta-timestampis changed after the array was created.LastAttemptStartedAtin node events is less precise. A delta of 0 already means "not set" and gets recomputed on the next round.How was this patch tested?
TestDeltaTimestampSeconds(same time, in range, largest storable value, the 267605s value from the issue, subNode started before the ArrayNode). It checks the result and thatSetItemdoesn't panic on an array built with the default 3-day max. With the old expression the "beyond range" and "negative" cases panic; with the fix all pass.go test ./pkg/controller/nodes/array/passes, andgolangci-lint run ./pkg/controller/nodes/array/reports 0 issues.Labels
(Written with Claude, working with @BhavayGoyal.)