Fix: wait only the remaining approval delay, not the full duration

[thc1006]

Summary

manageDelay returned the full configured approval delay even when the PackageRevision had already existed for part of it, so a revision was requeued for the whole delay again on top of the time it had already waited.

Motivation

The approval reconciler delays approval by approval.nephio.org/delay measured from the revision's creationTimestamp. manageDelay computed whether the delay had elapsed but, when it had not, returned the full delay d as the requeue duration:

if time.Since(pr.CreationTimestamp.Time) > d {
    return 0, nil
}
return d, nil

So a revision created 50 minutes before reconciliation with a 1h delay was requeued for a further 1h — approval ended up delayed by roughly 1h50m instead of 1h. The overshoot grows with how long the revision has already waited.

Changes

controllers/pkg/reconcilers/approval/reconciler.go:

• Return the time remaining until creationTimestamp + delay, not the full delay, so the total wait matches the configured duration:

  remaining := pr.CreationTimestamp.Time.Add(d).Sub(now)

  Taken from a deadline rather than as d - now.Sub(created). That subtraction overflows: with the largest valid duration and a creation timestamp one nanosecond in the future, d minus a negative elapsed wraps to MinInt64, reads as "already elapsed", and approves immediately. The delay is a user-supplied annotation, so that value is reachable. Time.Sub saturates instead of wrapping, so the same input now yields the maximum duration.

• Refuse a revision with no creation timestamp rather than treating it as having waited forever.

• Correct the README, which documented a two minute default and a thirty second floor.

  That text was accurate when it was written, and I got its history wrong in the first version of this description. Tracing it: #226 introduced the delay with a thirty second default and a thirty second floor; #252 raised the default to two minutes and updated the README to match; #329 then removed both, and said so plainly:

  There is now no longer a default delay; it only delays if you specifically give a delay annotation.

  The minimum delay is now 0.

  The README was not updated at that point and has been stale since. This PR aligns the documentation with the post-Faster approvals for the auto-approve controller #329 contract; it does not reintroduce the old policy, because nothing in the project sets this annotation on the revisions it creates and a default would delay every approval in every deployment.

• Document both policy values. The README still said initial was the only supported one, but #806 added always and the reconciler has handled it since. The change-history above stays in this description; the README carries only the current contract.

• A delay that has already elapsed (or is exactly met) still returns 0, and a malformed or negative annotation still returns an error — behaviour and the (time.Duration, error) signature are otherwise unchanged, so the caller (RequeueAfter: requeue) needs no change.

• Split the wall-clock read into manageDelayAt(pr, now) (with manageDelay calling it with time.Now()) so the logic can be unit-tested without depending on the wall clock.

A future creationTimestamp (e.g. clock skew) is well defined and does not panic: the delay is measured from creation, so remaining is simply d plus the skew.

Testing

Added cases: the maximum duration with a future creation timestamp (which fails against the old subtraction), and a missing creation timestamp. Restoring d - now.Sub(created) fails only the overflow case, so it is pinned rather than merely observed.

Both mistakes here are about arithmetic across the whole int64 range rather than about any particular annotation, so the invariants are also pinned by a fuzz target: a requeue is never negative, 0 means the deadline has passed, and a positive result is exactly the time left.

$ go test -fuzz FuzzManageDelay -fuzztime=60s ./reconcilers/approval/
fuzz: elapsed: 1m0s, execs: 1458900 (27973/sec), new interesting: 6 (total: 10)
PASS

The seeds run under plain go test, which is what make unit invokes, so this gates in CI without anyone opting into fuzzing. Each seed was checked against the implementation it is meant to reject:

# with the full-delay return restored
delay_fuzz_test.go:50: requeued 1h0m0s, want 0s remaining (offset=-3600000000000 delay="1h")

# with the overflowing subtraction restored
delay_fuzz_test.go:55: approved early: deadline 2465-01-17 ... is after now 2026-08-19 ...
                       (offset=4611686018427387904 delay="2562047h47m16.854775807s")

TestManageDelay drives manageDelayAt with a fixed now and asserts exact durations:

• partway through the delay (created 50m ago, 1h) → 10m
• exactly at the boundary → 0
• past the delay → 0
• future creationTimestamp → well defined, no panic
• no annotation → 0; zero delay (0s) → 0; malformed / negative annotation → error

It also checks that the manageDelay wrapper reads the wall clock, by bracketing the call between two time.Now() reads and requiring the result to lie between deadline.Sub(after) and deadline.Sub(before). That is exact rather than a tolerance, so a suspended runner cannot make it flake and a frozen clock cannot slip through: replacing time.Now() with time.Time{} in the wrapper fails it.

$ go test ./reconcilers/approval/...        # from controllers/pkg
ok  github.com/nephio-project/nephio/controllers/pkg/reconcilers/approval   (manageDelay/manageDelayAt 100%)

$ go test -race ./reconcilers/approval/...
$ golangci-lint run ./reconcilers/approval/...   # v2.8.0, .golangci.json → 0 issues
$ gosec ./reconcilers/approval/...               # no findings

[nephio-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign liamfallon for approval by writing /assign @liamfallon in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[nephio-prow]

Hi @thc1006. Thanks for your PR.

I'm waiting for a nephio-project member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.