fix: correct Retry-After header calculation in rate limiter

[Jason10293]

This change aims to fix the bug detailed in #491

Summary:

• Bugfix: sendRateLimitExceeded now computes retryAfter using floating-point math instead of integer division, so sub-second rate limits no longer collapse to zero.
• RFC compliance: Retry-After headers now round up to the nearest whole second via math.Ceil(retryAfter.Seconds()), ensuring clients always receive a meaningful integer delay.
• Tests: TestRateLimitMiddleware_HitRateLimit updated to assert the corrected Retry-After values for both low and high finite rate limits.

[CLAassistant]

All committers have signed the CLA.

[aaronbrethorst]

Hey Jason -- nice catch on the integer division bug. The analysis in #491 was thorough and the fix is correct. A few things to address before we merge.

Critical

No critical issues.

Important

1. Duplicate test case names

rate_limit_middleware_test.go:447-448 — two entries both named "low rate limit":

{name: "low rate limit", rateLimit: 2},
{name: "low rate limit", rateLimit: 20},

Go appends #01 to the duplicate so it doesn't break, but when a test fails you can't tell which case it was without checking suffixes. Please give each a unique name, e.g. "rate limit 2", "rate limit 20", "rate limit 100", "rate limit 200".

2. All test cases assert the same expected value (1)

Because math.Ceil(1.0 / N) = 1 for all N >= 1, every test case expects Retry-After: 1. This means the test would also pass if the implementation hardcoded Retry-After: 1. It doesn't prove the calculation actually works.

Please add at least one case where the expected value is not 1. For example, a sub-second rate with a longer interval:

// In a separate test or by extending the table to accept an interval parameter:
middleware := initRateLimitMiddleware(1, 2*time.Second)
// rate.Every(2s / 1) = 0.5 events/sec
// Retry-After = ceil(1/0.5) = ceil(2.0) = 2

This proves the math actually flows through correctly rather than always landing on the same answer.

3. Add rateLimit: 1 to the test table

This is the boundary where someone might assume the old code worked. It didn't — time.Duration(1) / time.Duration(1.0) = 1 nanosecond, which rounds to 0 seconds. Including it explicitly documents that the fix covers the boundary.

Fit and Finish

4. Formatting: blank line and comment spacing

rate_limit_middleware_test.go:440-442:

	})

}
func TestRateLimitMiddleware_CorrectRetryAfterTime(t *testing.T) {
	//Testing finite rate limits

Two small style issues:

• There's an extra blank line inside the closing of TestRateLimitMiddleware_EdgeCases, and then no blank line between the } and the new function. The rest of the file consistently uses a single blank line between top-level functions.
• //Testing needs a space after // per Go convention: // Testing finite rate limits.

Should be:

	})
}

func TestRateLimitMiddleware_CorrectRetryAfterTime(t *testing.T) {
	// Testing finite rate limits

5. Use strconv.Atoi instead of strconv.ParseFloat

rate_limit_middleware_test.go:477:

retryAfter, err := strconv.ParseFloat(retryAfterStr, 64)

The header is set via strconv.Itoa(), so it's always an integer string. Using strconv.Atoi() is more precise and would catch a regression where the header accidentally includes a decimal.

6. Use a concrete API key in the test

rate_limit_middleware_test.go:466:

req := httptest.NewRequest(http.MethodGet, "/test?key=", nil)

Empty key= routes through the __no_key__ sentinel path, which isn't what this test is about. Use ?key=test-key to keep the test focused on the Retry-After calculation.

Verdict

the fix itself is correct and well-motivated. Items 1-3 above need to be addressed (unique test names, at least one test case that expects a value other than 1, and the boundary case). Items 4-6 are quick cleanup.

[Jason10293]

I noticed that the the test case for when the rate limit is 0 won't pass because of the division on line 479. Since the case when the rate limit = 0 is already covered, should I include something to account for it or not?

[aaronbrethorst]

Jason, nice work on the revision — items 1, 2, 4, 5, and 6 all look good. One thing remaining:

Still needed

Add rateLimit: 1 to the test table

This was item 3 from my last review. Just add it to the existing table:

{name: "rate limit: 1", rateLimit: 1},

This is the boundary where the old integer-division bug was most visible (time.Duration(1) / time.Duration(1.0) = 1 nanosecond = 0 seconds), and documenting it in the test table proves the fix covers it.

Answering your question about rateLimit: 0

I noticed that the test case for when the rate limit is 0 won't pass because of the division on line 479. Since the case when the rate limit = 0 is already covered, should I include something to account for it or not?

Correct — don't include rateLimit: 0 in this table. The sendRateLimitExceeded switch statement handles 0 in its own case branch (line 150) before reaching the default: case where your fix lives, so it's a separate code path that's already covered.

---

In summary, add the rateLimit: 1 case and this is ready to merge.

[aaronbrethorst]

Hey Jason -- nice work addressing all six items from the previous round. The fix is correct, the formatting is clean, and the tests are much better. Just a couple of small things remaining.

Critical

No critical issues.

Important

1. Squash the commits

Please squash your commits into a single commit before merging. If you're not familiar with interactive rebase, here's a guide: Git Rebase for the Terrified.

Fit and Finish

1. Magic number 0.5 in the "Expected value is not equal to 1" subtest

rate_limit_middleware_test.go:508:

expected := int(math.Ceil(1.0 / float64(0.5)))

The 0.5 is the internal rate.Limit value that rate.Every(2s / 1) produces, but a reader has to reverse-engineer that from the test setup. Since this is a single specific test case (not parameterized), just use the literal:

// rate.Every(2s/1) = 0.5 events/sec => retryAfter = ceil(1/0.5) = 2
expected := 2

2. Test name describes the assertion, not the scenario

rate_limit_middleware_test.go:487:

t.Run("Expected value is not equal to 1", func(t *testing.T) {

This tells you what the test asserts rather than what scenario it covers. Something like "sub-second rate with 2s interval" or "rate limit 1 with 2s interval" would make it immediately clear what's being tested when scanning test output.

Verdict

Requesting changes. The production fix is sound and addresses a real bug, and all previous feedback has been addressed. Please squash the commits and consider the fit-and-finish items above.

[aaronbrethorst]

Hey Jason, you've addressed all the code feedback across three rounds — the fix is correct, the tests are solid, and the formatting is clean. One last thing before we can merge:

Prior Feedback Status

Round	#	Item	Status
1	1	Duplicate test case names	Fixed
1	2	All test cases assert same expected value	Fixed
1	3	Add rateLimit: 1 to test table	Fixed
1	4	Formatting (blank line, comment spacing)	Fixed
1	5	Use strconv.Atoi instead of ParseFloat	Fixed
1	6	Use concrete API key	Fixed
3	1	Squash commits	Still needed
3	2	Magic number 0.5	Fixed
3	3	Test name describes assertion, not scenario	Fixed

Important

1. Squash the commits

There are still 4 separate commits on this branch. Please squash them into a single commit before merging. If you're not familiar with interactive rebase, here's a guide: Git Rebase for the Terrified.

Once the commits are squashed, this is ready to merge.

[Jason10293]

I followed the instructions in the guide you linked and force pushed it. Maybe I missed something?

[aaronbrethorst]

@Jason10293 it looks like you're missing the squash step

[aaronbrethorst]

Hey Jason -- all feedback from the previous four rounds has been addressed, and the commits are squashed into a single clean commit. The fix is correct, the tests are solid, and the formatting is clean. Merging.

Prior Feedback Status

Round	#	Item	Status
1	1	Duplicate test case names	Fixed
1	2	All test cases assert same expected value	Fixed
1	3	Add rateLimit: 1 to test table	Fixed
1	4	Formatting (blank line, comment spacing)	Fixed
1	5	Use strconv.Atoi instead of ParseFloat	Fixed
1	6	Use concrete API key	Fixed
3	1	Squash commits	Fixed
3	2	Magic number 0.5	Fixed
3	3	Test name describes assertion, not scenario	Fixed

Nice persistence working through all the feedback rounds. The integer-division bug was a good catch and the fix is clean.