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; computeBackoff has since moved to internal/autocore/worker_manager_abort.go, and that is where this changes it. I left the nil policy 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 RetryPolicy requires max_interval, so a policy without one can no longer be scheduled. Its rules put no bound on backoff_coefficient and no upper bound on max_attempts, so a NaN or very large coefficient, or MaxAttempts of 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.validate rules on RetryPolicy, for example a finite backoff_coefficient and an upper bound on max_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/... and internal/flowcore/...; the integration suites need RUN_IT and 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
FAIL

On 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
Edited by José M. Requena Plens

Merge request reports

Loading
Loading