Cap retry backoff before converting it to a duration
Closes #1151 (closed)
computeBackoff grew the backoff as a float64, converted it to a time.Duration and only then applied MaxInterval. Go leaves the result of converting a float outside the int64 range to the implementation, and amd64 returns math.MinInt64, so the cap never applied: the retry's fire_at landed about 292 years in the past and it was due at once. A NaN coefficient took the same path, because the builtin max returns NaN when either argument is NaN.
The loop is unchanged. After it, the backoff is compared with MaxInterval while it is still a float, and MaxInterval is returned before any conversion unless the backoff is below it. Written as !(b < float64(maxInterval)), the comparison also caps a NaN, which compares false with everything, and a backoff of exactly 2^63, which is what float64(maxInterval) rounds to once MaxInterval passes about 292 years. Wherever the old conversion was in range and MaxInterval is at most 2^53 ns (about 104 days), the result is the same as before.
TestComputeBackoff gets four rows: attempt 35 at 1 s and 2.0 (the first attempt whose backoff passes the int64 range), a coefficient of 1e10, a backoff of exactly 2^63 under the largest MaxInterval, and a NaN coefficient. On amd64 all four return -2562047h47m16.854775808s on master, and all four pass with this change. On arm64 the conversion saturates instead, as the issue says, so there only the NaN row should fail on master (NaN converts to 0); I read that from the FCVTZS instruction the compiler emits for arm64 rather than from a run on an arm64 machine.
A few notes for the review:
- The issue links
worker_manager_tasks.go;computeBackoffhas since moved tointernal/autocore/worker_manager_abort.go, and that is where this changes it. I left thenilpolicy fallback alone, since that is #1128. - The ways in are narrower than when the issue was filed. Since !4625 (merged) (30fe95a5) the activity task payload is validated when it is written, and
RetryPolicyrequiresmax_interval, so a policy without one can no longer be scheduled. Its rules put no bound onbackoff_coefficientand no upper bound onmax_attempts, so a NaN or very large coefficient, orMaxAttemptsof 36 or more at 1 s and 2.0, still get here. No policy in the tree does today (the default allows 20 attempts capped at 10 minutes, the abort and flowcore poll policies allow 5). - If you would also like those refused at the source, I can follow up with
buf.validaterules onRetryPolicy, for example a finitebackoff_coefficientand an upper bound onmax_attempts. I kept this MR to the arithmetic the issue describes, since a rule there changes which policies can be scheduled. - I ran the unit tests under
internal/autocore/...andinternal/flowcore/...; the integration suites needRUN_ITand Docker (Postgres and Redis containers) and run in the community fork's pipeline.
Tests on master and on this branch
On master 1e8ff7636 with only the new test rows:
$ go test -count=1 -run 'TestComputeBackoff' -v ./internal/autocore/
--- FAIL: TestComputeBackoff/backoff_past_the_int64_range_capped_by_max_interval (0.00s)
expected: 10m0s
actual : -2562047h47m16.854775808s
--- FAIL: TestComputeBackoff/huge_coefficient_capped_by_max_interval (0.00s)
expected: 10m0s
actual : -2562047h47m16.854775808s
--- FAIL: TestComputeBackoff/backoff_at_the_int64_limit_capped_by_the_largest_max_interval (0.00s)
expected: 2562047h47m16.854775807s
actual : -2562047h47m16.854775808s
--- FAIL: TestComputeBackoff/NaN_coefficient_capped_by_max_interval (0.00s)
expected: 10m0s
actual : -2562047h47m16.854775808s
FAILOn this branch:
$ go test -count=1 -run 'TestComputeBackoff|TestScheduleToStart' -v ./internal/autocore/
--- PASS: TestComputeBackoff (0.00s)
--- PASS: TestScheduleToStartSeconds (0.00s)
ok gitlab.com/gitlab-org/cluster-integration/gitlab-agent/v19/internal/autocore
$ go test -count=1 ./internal/autocore/... ./internal/flowcore/...
ok .../internal/autocore
ok .../internal/autocore/admin
ok .../internal/autocore/dbsource
ok .../internal/autocore/persistence
ok .../internal/flowcore
$ go vet ./internal/autocore/
$ golangci-lint fmt --diff (the custom build from .custom-gcl.yml, v2.14.0)
$ golangci-lint run ./internal/autocore/
0 issues.Go 1.27.1, linux/amd64.
Old and new arithmetic side by side
I copied the old arithmetic and the new comparison into a scratch program and compared them over initial intervals from 1 ms to 2^53 ns (46 values, 40 of them random), coefficients from -1 to 1e10 plus NaN and ±Inf (54 values), max intervals of none, 1 s to 2^53 ns, and above 2^53 ns up to the largest Duration (92 values, 80 of them random), and attempts 1 to 80. Where the old conversion stays in range and the max interval is at most 2^53 ns, none of the 1,395,686 combinations differ. With a max interval above 2^53 ns, where a float64 no longer holds every nanosecond count, 2 of 1,395,686 differ, by 1 ns, and the new result is the max interval itself. Wherever a max interval is set, NaN and ±Inf included, the new arithmetic never returns a negative value or one above the max interval, and never converts a float outside the int64 range.
m<=2^53: compared=1395686 differ=0 | m>2^53: compared=1395686 differ=2 (max |delta| 1ns) | result>max: 0 (worst +0ns) | negative: 0 | out-of-range conversions: 0