test(sqs): structural fix — shared throttle-refill helper for drain-pattern tests

[bootjp]

Summary

Structural fix for the recurring SQS-throttle CI flake. PR #890 just merged a per-test fix for SetQueueAttributesInvalidatesBucket; this PR generalizes the rule across the whole file so the next drain-pattern test added to adapter/sqs_throttle_integration_test.go cannot regress to the same flake.

Why a structural fix, not another per-test commit

The same CI-flake mechanism has been fixed three times in three different SQS-throttle tests over ~6 weeks:

Commit	Test
54c6cd56	NoOpSetQueueAttributesPreservesBucket (PR #819)
204b7294	SetQueueAttributesInvalidatesBucket (PR #890)
This PR	structural prevention for the remaining drain-pattern tests

Each per-test fix touched one test in isolation, leaving the same trap in sibling tests for the next slow CI runner to step on. Two sibling tests are still unprotected at the time of this PR:

• TestSQSServer_Throttle_SendBucketRejectsAfterCapacity — HIGH risk, drain 10 then assert 11th rejects, Refill=1
• TestSQSServer_Throttle_RecvBucketRejectsAfterCapacity — HIGH risk, receive-side mirror, same pattern

Plus BatchChargesByEntryCount is theoretically at risk on a very slow runner.

Root cause (recurring across all three flake instances)

Tests that (a) set ThrottleSend/RecvRefillPerSecond AND (b) drain the bucket then assert a "throttled" response are racing their own wall clock. Under -race on slow CI runners each request elapses ~100-250 ms (full HTTP + Raft propose+apply). An N-write drain plus its assertion send takes ~N×150 ms ± noise. At Refill=1/sec the bucket accumulates ≥1 token over that window and the "throttled" assertion fires against a refilled bucket, returning 200 instead of 400 — a false positive against a real production-contract regression test.

Structural fix

1. Shared file-head constants + helpers in adapter/sqs_throttle_integration_test.go:

   const slowRefillRate = "0.01"  // 1 token per 100 seconds
   func newSendThrottleAttrs(capacity int) map[string]string
   func newRecvThrottleAttrs(capacity int) map[string]string

   Helpers lock the refill rate to slowRefillRate so a future drain-pattern test cannot pick a racing value by accident.

2. File-head doc-comment block explaining:

   • The recurring flake mechanism (with prior-commit refs)
   • The rule: drain-then-assert MUST use the helpers
   • The exception: config-round-trip / validator-rejection / post-raise tests where the rate is part of the test contract

3. Migrated all drain-pattern call sites to the helpers:

   • SendBucketRejectsAfterCapacity (HIGH risk — unprotected until now). Retry-After assertion updated from "1" to "100" (formula ceil(needed/refillRate) returns 100s for refillRate=0.01; verified by reading computeRetryAfter in sqs_throttle.go:525).
   • RecvBucketRejectsAfterCapacity (HIGH risk; same migration)
   • BatchChargesByEntryCount (LOW risk in practice, migrated for consistency)
   • SetQueueAttributesInvalidatesBucket (already fixed by PR test(sqs): slow throttle refill on SetQueueAttributesInvalidatesBucket to fix CI flake #890, refactored to use the helper)
   • DeleteQueueInvalidatesBucket (no drain assertion, migrated for uniformity)
   • NoOpSetQueueAttributesPreservesBucket (already fixed by PR test(sqs): slow throttle refill on no-op SetQueueAttributes test to fix CI flake #819 with a local slowRefill const, refactored to use the shared helper)

Not migrated (intentional):

• GetQueueAttributesRoundTrip (Refill=50/5 — exact values are the test contract)
• RejectsCapacityBelowBatchMin (Refill=1 — value is part of the rejected input the validator must catch)
• SetQueueAttributesInvalidatesBucket post-raise phase (Refill=20 — raising both Capacity AND Refill is what the phase asserts fires)

Caller audit (per /loop semantic-change rule)

Test-only file. Helpers are package-private and only referenced within this single file (verified by grep across the repo). No production code touched. No other test file uses these names.

Retry-After header assertion change in SendBucketRejectsAfterCapacity was the single observable behavior change: header depends on refillRate via the §3.4 formula. Old "1" matched Refill=1; new "100" matches Refill=0.01. The header contract is still exercised — only the asserted literal changed.

Validation

• go test ./adapter/ -run TestSQSServer_Throttle -race -count=3 -timeout 180s → ok 5.1s, all three iterations green
• gofmt + go vet + golangci-lint run → 0 issues

Out-of-scope follow-ups (other recurring flake categories)

Broader CI-flake history (47 fix-CI-flake commits) has other recurring patterns that deserve similar structural treatment — tracked as future work, out of scope for this PR:

• Leader churn + Raft async apply: many tests retry on "not leader" or wait for follower catch-up. A shared eventuallyLeaderOK(t, ...) helper would centralize the retry + budget knobs.
• Redis TTL window racing: multiple "widen TTL" commits suggest a per-test deadline-multiplier helper.
• VM pool / cache hit-rate assertions: dropped as flaky in 7783c279 — the structural lesson is to avoid asserting on cache hit rates.

This PR focuses on the most-repeated single flake (3 fixes for the same mechanism in 6 weeks). The remaining categories each warrant their own follow-up PR after we see how this pattern works in practice.

Summary by CodeRabbit

• Tests
  • Enhanced SQS throttle test infrastructure to improve test reliability in continuous integration environments.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Warning

Review limit reached

@bootjp, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 48 minutes and 43 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c637023e-be93-4fb7-8964-cdfdccd1a5da

📥 Commits

Reviewing files that changed from the base of the PR and between 17524a7 and e5f3176.

📒 Files selected for processing (1)

• adapter/sqs_throttle_integration_test.go

📝 Walkthrough

Walkthrough

This PR hardens SQS throttle integration tests against CI flakes by introducing a shared slowRefillRate constant and helper functions that generate standardized throttle attribute maps. Six tests are updated to use these helpers, replacing hard-coded refill values, and one Retry-After header assertion is adjusted to match the new slow refill rate's rounding.

Changes

SQS Throttle Test CI-Flake Guardrails

Layer / File(s)	Summary
Test infrastructure and helpers adapter/sqs_throttle_integration_test.go	Defines slowRefillRate = "0.01" and introduces newSendThrottleAttrs(capacity) and newRecvThrottleAttrs(capacity) helper functions that produce standardized throttle attribute maps locked to the slow refill rate.
Migrate integration tests to use throttle helpers adapter/sqs_throttle_integration_test.go	SendBucketRejectsAfterCapacity, ReceiveBucketRejectsAfterCapacity, BatchCharges, SetQueueAttributesInvalidatesBucket, DeleteQueueInvalidatesBucket, and NoOpSetQueueAttributesPreservesBucket tests are updated to call the new helpers instead of inline attribute maps; the Retry-After assertion in SendBucketRejectsAfterCapacity is changed from "1" to "100" to align with the slow refill rate.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• bootjp/elastickv#890: Also modifies adapter/sqs_throttle_integration_test.go to address the same SQS throttle CI flake by adjusting SetQueueAttributesInvalidatesBucket's slow refill configuration.

Poem

🐰 A refill rate so slow it stays,
Constants shared through all the days,
Tests unified, no more the drift,
CI flakes begone—a stable gift!
Helpers bundled, once or twice,
Throttle testing, oh so nice! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'test(sqs): structural fix — shared throttle-refill helper for drain-pattern tests' accurately describes the main change: introducing shared helper functions for throttle configuration in SQS tests to fix a CI flake.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sqs-throttle-invalidate-flake

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request refactors SQS throttle integration tests to use structural helpers with a slow refill rate (0.01 tokens/sec), preventing CI flakes caused by wall-clock races during drain-then-assert patterns. The reviewer pointed out a potential new flake in TestSQSServer_Throttle_SendBucketRejectsAfterCapacity where asserting an exact Retry-After value of "100" might fail on slow CI runners if more than a second elapses between requests. They recommend asserting a safe range (e.g., [90, 100]) instead.

[gemini-code-assist]

Asserting an exact Retry-After value of "100" can introduce a new test flake on slow CI runners.

Why this happens:

The Retry-After header is calculated as math.Ceil(needed / refillRate).
With refillRate = 0.01, needed / refillRate is (1 - elapsed * 0.01) / 0.01 = 100 - elapsed.
If the time elapsed (elapsed) between the 10th send and the 11th send is strictly greater than 1.0 second (which is highly possible on slow or busy CI runners under -race), 100 - elapsed will be less than 99.0.
Consequently, math.Ceil(100 - elapsed) will round up to 99 or less, causing the exact assertion ra != "100" to fail.

Recommendation:

Parse the Retry-After header value and assert that it falls within a safe range (e.g., [90, 100]) to tolerate realistic scheduling or network delays on slow CI runners.

	raVal, err := strconv.Atoi(resp.Header.Get("Retry-After"))
	if err != nil {
		t.Fatalf("11th send: invalid Retry-After header: %v", err)
	}
	if raVal < 90 || raVal > 100 {
		t.Fatalf("11th send: Retry-After=%d (expected between 90 and 100)", raVal)
	}

[coderabbitai]

🧹 Nitpick comments (1)

adapter/sqs_throttle_integration_test.go (1)

56-57: ⚡ Quick win

Remove //nolint:unparam by simplifying helper signatures.

These suppressions can be avoided by making the helpers zero-arg (current usage is fixed at capacity 10), then re-introducing parameters when a real caller needs a different value.

Suggested diff

+const drainPatternCapacity = 10
+
-//nolint:unparam // capacity parameter is forward-looking; see doc.
-func newSendThrottleAttrs(capacity int) map[string]string {
+func newSendThrottleAttrs() map[string]string {
 	return map[string]string{
-		"ThrottleSendCapacity":        strconv.Itoa(capacity),
+		"ThrottleSendCapacity":        strconv.Itoa(drainPatternCapacity),
 		"ThrottleSendRefillPerSecond": slowRefillRate,
 	}
 }
 
-//nolint:unparam // capacity parameter is forward-looking; see newSendThrottleAttrs.
-func newRecvThrottleAttrs(capacity int) map[string]string {
+func newRecvThrottleAttrs() map[string]string {
 	return map[string]string{
-		"ThrottleRecvCapacity":        strconv.Itoa(capacity),
+		"ThrottleRecvCapacity":        strconv.Itoa(drainPatternCapacity),
 		"ThrottleRecvRefillPerSecond": slowRefillRate,
 	}
 }

As per coding guidelines, "**/*.go: ... Avoid adding //nolint annotations — refactor instead."

Also applies to: 69-70

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@adapter/sqs_throttle_integration_test.go` around lines 56 - 57, The helper
newSendThrottleAttrs currently takes an unused capacity parameter and is
suppressed with //nolint:unparam; remove the nolint and simplify the signature
to a zero-arg function (e.g., newSendThrottleAttrs()) since all current callers
use the fixed capacity (10), and update any analogous helper (the one at lines
69-70) the same way so both helpers have no unused parameters; adjust call sites
to invoke the zero-arg functions and re-run linters to ensure the //nolint
annotations can be removed.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@adapter/sqs_throttle_integration_test.go`:
- Around line 56-57: The helper newSendThrottleAttrs currently takes an unused
capacity parameter and is suppressed with //nolint:unparam; remove the nolint
and simplify the signature to a zero-arg function (e.g., newSendThrottleAttrs())
since all current callers use the fixed capacity (10), and update any analogous
helper (the one at lines 69-70) the same way so both helpers have no unused
parameters; adjust call sites to invoke the zero-arg functions and re-run
linters to ensure the //nolint annotations can be removed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ff78a714-1ad0-480d-b5ea-945544181de8

📥 Commits

Reviewing files that changed from the base of the PR and between 3599556 and 17524a7.

📒 Files selected for processing (1)

• adapter/sqs_throttle_integration_test.go

[bootjp]

@claude review

Round 1 fixes (commit e5f3176):

• gemini HIGH (Retry-After exact-value assertion is itself flaky): Replaced exact != "100" check with a range check [1, 100]. The formula ceil((1 - currentTokens) / refillRate) depends on drain-window elapsed time (refill accumulates during the loop), so asserting "100" exactly re-introduces a wall-clock flake at the assertion layer. The range pins the §3.4 contract (positive integer bounded by 1/refillRate) without racing.

• coderabbit nitpick (//nolint:unparam): Removed by making helpers zero-arg. Capacity is now a documented file-level const drainBucketCapacity = "10" (the §3.2 validator batch-floor minimum). All 9 call sites updated from foo(10) → foo().

Caller audit: test-only file, helpers package-private, no external references. go test -race -count=3 passes (5.3s); golangci-lint 0 issues.

[claude]

Claude finished @bootjp's task in 2m 23s —— View job

---

Review of PR #891 — Round 1 fixes (commit e5f3176)

• Read the changed file and diff
• Verify the Retry-After range fix correctness
• Verify the zero-arg helper refactor
• Check for any remaining issues
• Post review findings

---

Both Round 1 fixes are correct. Here's the verification:

Fix 1: Retry-After range [1, 100] — ✅ Correct

Traced through computeRetryAfter (sqs_throttle.go:525) and writeSQSThrottlingError (sqs.go:585):

needed = 1 - currentTokens
secs   = ceil(needed / 0.01)

The mathematical bounds with slowRefillRate=0.01:

• Upper bound 100: when currentTokens = 0 exactly → ceil(1.0/0.01) = 100. This is then written as int(retryAfter/time.Second) → 100.
• Lower bound 1: from two guards — computeRetryAfter clamps secs < 1 → 1, and writeSQSThrottlingError additionally clamps retryAfter < time.Second → time.Second. On an extremely slow runner where the drain loop itself takes >100s, currentTokens ≥ 1 triggers the needed ≤ 0 path which returns time.Second → raSecs = 1.
• No escape outside [1, 100]: throttleRetryAfterCap = time.Hour only applies above 3600s, which is unreachable at capacity=10 with refill=0.01.

The range [1, 100] is the exact tight bound. The commit message math (ceil(98.5) = 99 for a 10×150ms drain) is also correct — the range handles intermediate values without over-constraining.

Fix 2: Zero-arg helpers + drainBucketCapacity const — ✅ Clean

• //nolint:unparam annotations removed as requested
• drainBucketCapacity = "10" (string type is correct — values go directly into map[string]string)
• strconv import is still valid: used by strconv.Atoi(ra) at line 152 and strconv.Itoa(i) at lines 98, 124, 210–211
• All 9 call sites correctly updated from foo(10) → foo()

Pre-existing gap (not introduced by Round 1, low priority)

RecvBucketRejectsAfterCapacity (adapter/sqs_throttle_integration_test.go:166-190) checks status and x-amzn-ErrorType but never asserts Retry-After. The send test at line 151-158 does verify it. If the recv path ever diverges on the header, there's no test coverage for it. Worth a follow-up but out of scope for this PR.

Summary

Both reviewer concerns from Round 1 are addressed correctly:

• gemini HIGH: range check [1, 100] is mathematically tight and won't re-introduce a wall-clock flake
• coderabbit nitpick: zero-arg helpers eliminate //nolint:unparam without leaving dead code

LGTM.