1. The under-rate phase has a thin margin. submitBroadcast(t, conn, 30, 20*time.Millisecond)
paces the client at ~50/s against a 100/s limit and then asserts require.Zero(underRate.throttled).
Client-side pacing doesn't pace the limiter: the throttle check sits immediately after stream.Recv()
in the Broadcast loop, so if the Recv loop stalls for more than the burst tolerance
(burstOffset = burst*T = 100ms here) and then drains the queued envelopes at once, requests get
rejected and the assertion fails. It held for me locally at -cpu=1 and under CPU load, so this is
about CI headroom rather than a bug — but 2x on the rate plus 100ms of slack is tight for a -race
runner. Pacing at 50ms, raising Rate, or asserting a small bound instead of exactly zero would all
buy margin cheaply.
2. Two near-identical broadcast helpers now. submitBroadcast here and
submitBroadcastRequests in router_test.go. The new one is strictly better — it separates
throttled from other and asserts the status code, where the old one lumps every non-SUCCESS into
respondsErrors and regex-matches the error text. Since the four tests are being moved in this
commit anyway, that's the cheap moment to fold them onto the new helper and drop the old one.
Neither is blocking.
Originally posted by @DorKatzelnick in #1217 (review)
1. The under-rate phase has a thin margin.
submitBroadcast(t, conn, 30, 20*time.Millisecond)paces the client at ~50/s against a 100/s limit and then asserts
require.Zero(underRate.throttled).Client-side pacing doesn't pace the limiter: the throttle check sits immediately after
stream.Recv()in the Broadcast loop, so if the Recv loop stalls for more than the burst tolerance
(burstOffset = burst*T = 100ms here) and then drains the queued envelopes at once, requests get
rejected and the assertion fails. It held for me locally at
-cpu=1and under CPU load, so this isabout CI headroom rather than a bug — but 2x on the rate plus 100ms of slack is tight for a
-racerunner. Pacing at 50ms, raising
Rate, or asserting a small bound instead of exactly zero would allbuy margin cheaply.
2. Two near-identical broadcast helpers now.
submitBroadcasthere andsubmitBroadcastRequestsinrouter_test.go. The new one is strictly better — it separatesthrottledfromotherand asserts the status code, where the old one lumps every non-SUCCESS intorespondsErrorsand regex-matches the error text. Since the four tests are being moved in thiscommit anyway, that's the cheap moment to fold them onto the new helper and drop the old one.
Neither is blocking.
Originally posted by @DorKatzelnick in #1217 (review)