fix: retry policy counts consecutively instead of cumulatively#819
Merged
adityachoudhari26 merged 2 commits intomainfrom Feb 26, 2026
Merged
fix: retry policy counts consecutively instead of cumulatively#819adityachoudhari26 merged 2 commits intomainfrom
adityachoudhari26 merged 2 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes modify retry evaluation to sort jobs by creation time (latest first) and count only consecutive retryable attempts for the current release target, breaking the streak when different releases appear. Tests validate version-flip behavior where flipping between releases resets consecutive retry counts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/workspace-engine/test/e2e/engine_version_selector_test.go (1)
16-16: Rename test to fix typo (Flippping→Flipping).Minor naming cleanup for readability and discoverability in test output.
✏️ Suggested rename
-func TestEngine_VersionSelector_FlipppingBetweenVersions(t *testing.T) { +func TestEngine_VersionSelector_FlippingBetweenVersions(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/e2e/engine_version_selector_test.go` at line 16, Rename the test function TestEngine_VersionSelector_FlipppingBetweenVersions to correct the typo (e.g., TestEngine_VersionSelector_FlippingBetweenVersions) so the function name and any references to it use the new spelling; update the function declaration and any references/imports or test runners that refer to TestEngine_VersionSelector_FlipppingBetweenVersions to the corrected name.apps/workspace-engine/test/integration/opts.go (1)
886-895: Propagate CEL parsing failures inWithRuleVersionSelector.
FromCelSelectorerrors are currently ignored, which can silently build an invalid selector in tests.♻️ Suggested refactor
func WithRuleVersionSelector(cel string) PolicyRuleOption { - s := &oapi.Selector{} - _ = s.FromCelSelector(oapi.CelSelector{Cel: cel}) return func(_ *TestWorkspace, r *oapi.PolicyRule) error { + s := &oapi.Selector{} + if err := s.FromCelSelector(oapi.CelSelector{Cel: cel}); err != nil { + return err + } r.VersionSelector = &oapi.VersionSelectorRule{ Selector: *s, } return nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/integration/opts.go` around lines 886 - 895, WithRuleVersionSelector currently ignores the error returned by oapi.Selector.FromCelSelector; change it to capture that error and propagate it via the PolicyRuleOption closure. Specifically, call s.FromCelSelector(oapi.CelSelector{Cel: cel}) and store the returned error; return a closure (the PolicyRuleOption) that, if that error is non-nil, returns the error, otherwise sets r.VersionSelector = &oapi.VersionSelectorRule{Selector: *s} and returns nil. This uses the existing symbols WithRuleVersionSelector, oapi.Selector, FromCelSelector, PolicyRuleOption, TestWorkspace, and oapi.PolicyRule to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.go`:
- Around line 79-87: getJobsForReleaseTargetSortedLatestFirst builds a slice
from a map and sorts only by CreatedAt, which leaves order undefined for equal
timestamps; modify the sort.Slice comparator in
getJobsForReleaseTargetSortedLatestFirst to add a deterministic tie-breaker when
jobs[i].CreatedAt.Equal(jobs[j].CreatedAt) — e.g., compare a unique field such
as jobs[i].Id (or jobs[i].Name if Id is unavailable) to return jobs[i].Id >
jobs[j].Id (or lexicographic compare) so equal-timestamp jobs get a stable,
deterministic ordering.
---
Nitpick comments:
In `@apps/workspace-engine/test/e2e/engine_version_selector_test.go`:
- Line 16: Rename the test function
TestEngine_VersionSelector_FlipppingBetweenVersions to correct the typo (e.g.,
TestEngine_VersionSelector_FlippingBetweenVersions) so the function name and any
references to it use the new spelling; update the function declaration and any
references/imports or test runners that refer to
TestEngine_VersionSelector_FlipppingBetweenVersions to the corrected name.
In `@apps/workspace-engine/test/integration/opts.go`:
- Around line 886-895: WithRuleVersionSelector currently ignores the error
returned by oapi.Selector.FromCelSelector; change it to capture that error and
propagate it via the PolicyRuleOption closure. Specifically, call
s.FromCelSelector(oapi.CelSelector{Cel: cel}) and store the returned error;
return a closure (the PolicyRuleOption) that, if that error is non-nil, returns
the error, otherwise sets r.VersionSelector =
&oapi.VersionSelectorRule{Selector: *s} and returns nil. This uses the existing
symbols WithRuleVersionSelector, oapi.Selector, FromCelSelector,
PolicyRuleOption, TestWorkspace, and oapi.PolicyRule to locate the fix.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/smart_defaults_test.goapps/workspace-engine/test/e2e/engine_version_selector_test.goapps/workspace-engine/test/integration/opts.go
Comment on lines
+79
to
+87
| func (e *RetryEvaluator) getJobsForReleaseTargetSortedLatestFirst(releaseTarget *oapi.ReleaseTarget) []*oapi.Job { | ||
| jobsMap := e.getters.GetJobsForReleaseTarget(releaseTarget) | ||
| jobs := make([]*oapi.Job, 0, len(jobsMap)) | ||
| for _, job := range jobsMap { | ||
| jobs = append(jobs, job) | ||
| } | ||
| sort.Slice(jobs, func(i, j int) bool { | ||
| return jobs[i].CreatedAt.After(jobs[j].CreatedAt) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, locate and examine the file
file="apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.go"
if [ ! -f "$file" ]; then
echo "File not found at: $file"
exit 1
fi
echo "=== File content around lines 79-90 ==="
cat -n "$file" | sed -n '75,92p'
echo
echo "=== Check for imports at top of file ==="
head -30 "$file" | cat -n
echo
echo "=== Look for the full method and its context ==="
cat -n "$file" | sed -n '79,100p'Repository: ctrlplanedev/ctrlplane
Length of output: 3033
🏁 Script executed:
#!/bin/bash
# Find the Job struct definition to verify it has an Id field
echo "=== Searching for Job struct definition ==="
find . -name "*.go" -type f -exec grep -l "type Job struct" {} \; | head -5
# Look for oapi package
echo
echo "=== Finding oapi package ==="
find . -path "*/oapi/*.go" -type f | head -10 | xargs ls -la 2>/dev/null | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 1213
🏁 Script executed:
#!/bin/bash
# Search for Job struct definition with Id field
echo "=== Searching for Job struct with fields ==="
rg -A 20 "type Job struct" --type go | head -50
echo
echo "=== Check CreatedAt field type ==="
rg "CreatedAt\s+(?:time\.)?Time" --type go | head -10Repository: ctrlplanedev/ctrlplane
Length of output: 4976
🏁 Script executed:
#!/bin/bash
# Check how getJobsForReleaseTargetSortedLatestFirst is actually used
file="apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.go"
echo "=== Usage of getJobsForReleaseTargetSortedLatestFirst ==="
rg "getJobsForReleaseTargetSortedLatestFirst" "$file" -B 2 -A 5
echo
echo "=== Full context of Evaluate method to understand retry counting ==="
cat -n "$file" | sed -n '40,130p'Repository: ctrlplanedev/ctrlplane
Length of output: 4081
Add tie-breaking to deterministic sort order in newest-first sorting.
The slice is built from a map and sorted only by CreatedAt. When jobs share the same timestamp, sort.Slice produces undefined relative order, making consecutive retry counting nondeterministic if equal-time jobs for the same release have different retryable statuses.
Suggested fix
sort.Slice(jobs, func(i, j int) bool {
+ if jobs[i].CreatedAt.Equal(jobs[j].CreatedAt) {
+ return jobs[i].Id > jobs[j].Id
+ }
return jobs[i].CreatedAt.After(jobs[j].CreatedAt)
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.go`
around lines 79 - 87, getJobsForReleaseTargetSortedLatestFirst builds a slice
from a map and sorts only by CreatedAt, which leaves order undefined for equal
timestamps; modify the sort.Slice comparator in
getJobsForReleaseTargetSortedLatestFirst to add a deterministic tie-breaker when
jobs[i].CreatedAt.Equal(jobs[j].CreatedAt) — e.g., compare a unique field such
as jobs[i].Id (or jobs[i].Name if Id is unavailable) to return jobs[i].Id >
jobs[j].Id (or lexicographic compare) so equal-timestamp jobs get a stable,
deterministic ordering.
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
1 participant
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.
Summary by CodeRabbit
Bug Fixes
Tests