fix: invert shouldRefill() condition in v1 scheduler rate limiter

[CoderSufiyan]

Description

shouldRefill() in the v1 scheduler rate limiter had its condition inverted. It returned true when nextRefillAt was still in the future (window still open, no refill needed) — the opposite of when a refill is actually due.

The bug

pkg/scheduling/v1/rate_limit.go:182:

return r.nextRefillAt.After(time.Now().UTC())

This returns true while the refill time is still in the future. Compare with the guard inside flushToDatabase() (line 292), which no-ops while nextRefillAt.After(now) and only does real work once the boundary has passed. The two are meant to agree; shouldRefill() got the negation wrong.

Impact

Both failure modes land in use() at line 118:

1. After a window rolls over: shouldRefill() reports false, so use() never triggers the on-demand refresh. Tasks are evaluated against depleted counts from the previous window and rejected until the 1s loopFlush ticker fires. Up to ~1s of spurious rate-limit rejections per window rollover, per tenant.

2. Before the window rolls over (the common case): shouldRefill() reports true, so every use() call runs flushToDatabase(), which takes three write locks (unflushedMu, dbRateLimitsMu, nextRefillAtMu) and immediately returns doing nothing. use() sits on the per-task scheduling path — constant write-lock churn for zero work.

Fix

return !r.nextRefillAt.After(time.Now().UTC())

This aligns shouldRefill() with the flushToDatabase() guard: returns true when now >= nextRefillAt (refill is due), false while the window is still open.

Fixes #4346

[vercel]

@CoderSufiyan is attempting to deploy a commit to the Hatchet Team on Vercel.

A member of the Team first needs to authorize it.

[STiFLeR7]

Verified the fix against the sibling guard in flushToDatabase (line ~293: if r.nextRefillAt.After(time.Now().UTC()) { return nil }) — the two now agree: use() calls flushToDatabase exactly when nextRefillAt has actually passed, instead of the inverted original which called it while a refill was not yet due and stopped calling it once one became due. No race between the two separate time.Now() calls either, since time only moves forward between them and nextRefillAt is fixed — so the two checks can't disagree.

One gap: this ships with no test. Given shouldRefill/nextRefillAt is guarded by nextRefillAtMu and sits on a concurrency-sensitive path, a focused unit test asserting shouldRefill() is false while nextRefillAt is in the future and true once it's passed (matching flushToDatabase's polarity) would make this much faster to merge with confidence and guard against the exact inversion class of bug regressing later. The original issue reporter (#4346) even offered to include one — might be worth pulling that in before merge.

[CoderSufiyan]

Good call on the tests. I've added:

1. TestRateLimiter_ShouldRefill - three subtests covering nil, future, and past nextRefillAt
2. TestRateLimiter_UseRefreshesFromDbOnExpiredWindow - integration-style test verifying use() triggers flushToDatabase when the window has expired, refreshing the depleted rate limit from the DB

All 13 rate limit tests pass.