[vvoytovych]

Related to #965.

These are ecv's three follow-ups from #965's review.

change	why
A claim's requests of one type are decided as one request, their amounts summed.	A claim holds one allocation per resource type, keyed by type. A reservation per request left one allocation and kept the other amount reserved. This is older than #949: the test fails on main too.
Every bucket comes back an hour after its last reconcile.	A reservation that no claim holds shows as a cached total below Allocated once the cache is current, and that makes a live recompute. A quiet bucket gets no event to show it. Each recheck costs one bucket GET and one grant list, and a claim list only when the totals differ: one GET and one list an hour for each bucket.
An envtest case races 20 claims against a limit of 19 on a real API server.	The fake client cannot make two writers meet a resourceVersion conflict.
One pending request per claim and type: the decision loop and its grant flag go.	With the requests of one type summed, the loop ran at most once.

Test plan

• TestAllowanceBucketDecidesAClaimsRequestsOfOneTypeTogether fails on main, where the claim holds 2 and the bucket reserves 3, and passes here. TestAllowanceBucketComesBackAfterTheRecheckInterval holds the recheck. The usual-denial test now also checks that a denied claim is not counted.
• TestAllowanceBucketGrantsExactlyItsLimitAgainstARealAPIServer: 20 claims created while one controller decides them, and 20 pending claims when two controllers start at once. Both give exactly 19 grants and one denial, three runs each with -race, here and on fix: Stop listing all claims live on every bucket reconcile #965's head. A conflict retried with the stale allocation makes the second case fail, three runs of three.
• Thirteen planted defects, one per mechanism of the bucket controller, each turn their own test red. go test -race ./internal/quota/... and go test -timeout 5m ./... pass with envtest 1.37.0. The core package now takes about 52 s.
• After a staging rollout: the allowance-bucket reconcile rate rises by about one a bucket an hour. Cluster-scope ResourceClaim lists per bucket reconcile stay at or below fix: Stop listing all claims live on every bucket reconcile #965's on staging, by kind of minute: 0.04–0.07 in a grant burst, 0.31–0.33 in delete-heavy minutes.

[ecv]

Approve.

Allocations are keyed by resource type, so deciding a claim's same-type requests as one summed amount is the right unit: the old per-request loop reserved per request, and each claim write replaced the previous allocation of that type, leaking the earlier reservation. The all-or-nothing outcome on the sum is the correct answer when a claim is only partly satisfiable.

• Partial fit: with limit 3, allocated 1 and a claim of 1+2, the old code granted the first, reserved 1, then denied the second and overwrote the allocation as Denied, so 1 stayed reserved with no claim holding it. When both fit, it reserved both but kept only the last allocation. Now the sum is one decision; a denial writes nothing to the bucket, and a grant reserves the sum that grantedAmount later sums back (TestAllowanceBucketDecidesAClaimsRequestsOfOneTypeTogether).
• Concurrency: the reservation is still a bucket Status().Update on a live read's resourceVersion before the claim write, so with one active controller reservations serialize. The new envtest runs 20 claims against a limit of 19 with one and with two controllers and asserts 19 granted, 1 denied.
• Stranded reservation: if the claim write fails after the bucket write, nothing reverts; the hourly requeue brings the bucket back, the cached total disagrees with Allocated, and it recomputes from live claims. TestAllowanceBucketComesBackAfterTheRecheckInterval pins only the requeue interval; the recompute is covered by the earlier tests.
• ClaimCount counts grants only, as before; the updated test now pins that a denied claim is not counted.

Follow-ups:

• Two active controllers (a leader handoff) can still over-grant, from reading the code: B reserves but has not written its claim; A reads the bucket, sees cached total below Allocated, recomputes Allocated from live claims without B's in-flight reservation, and A's update succeeds on that fresh resourceVersion. The two-controller envtest passing shows the race did not trigger, not that it is closed. Excluding in-flight reservations from the recompute, or not recomputing in the same pass that grants, would close it.
• The envtest workflow runs on push only, so the race test does not gate pull requests.
• The CRD schema does not reject two requests of one type (only the doc comment says once). A CEL rule would make the summing a safety net rather than a live path.
• The hourly RequeueAfter has no jitter, so after a restart every bucket rechecks in the same minute. Add jitter if bucket count grows.

[vvoytovych]

Your first follow-up holds, and it predates this PR.

I drove the handoff step by step against a real API server, with a limit of 1 and two pending claims:

1. B reserves the last unit for handoff-b and has not written the claim yet.
2. A reconciles in that gap. Its cached claims disagree with the bucket, so it recomputes Allocated from the live claims and drops B's reservation. It grants handoff-a and denies handoff-b.
3. B applies handoff-b as granted. The apply carries no precondition, so it replaces A's denial.

Two claims end granted against a limit of 1. The test fails the same way on main (c457001) and on this PR's head (fde11cc), 3 runs of 3 on each. This PR does not change that path. So I would merge it as approved, and fix the handoff in its own PR next, with this test.

The other three follow-ups each get their own PR after this merges:

• Run the envtest workflow on pull requests too, so the race tests gate a change.
• Make the requests list +listType=map with +listMapKey=resourceType, so the API server rejects two requests of one type.
• Add jitter to the hourly recheck.

[vvoytovych]

One correction to my reply above. I wrote that the envtest workflow should run on pull requests so the race tests gate a change. They already do.

The required check on main is ci / Build and test. Its workflow, ci.yaml, runs on pull requests, installs envtest 1.37.0 and runs go test ./.... On this PR's run, the quota controller package took 51.7 s, so its envtests ran: https://github.com/milo-os/milo/actions/runs/41592340528

"IAM API testing" runs the same go test ./... on push only and is not required, so it duplicates that work. The follow-up becomes removing it, as datum-cloud/network-services-operator#849 did for NSO.