refactor: simplify the init-concurrency limiter to a single shared budget - #785
Merged
kinyoklion merged 6 commits intoAug 5, 2026
Merged
Conversation
…dget The limiter primitive added in #780 carried an optional per-environment gate and a matching INIT_PER_ENV_MAX_PERCENT config option. Drop per-environment fairness: the limiter now bounds concurrent admissions with one shared budget (a fixed number of held slots plus a bounded FIFO queue of waiters), and Acquire no longer takes an environment key. Also revise the INIT_SEND_TIMEOUT documentation and default to describe an absolute cap on how long one gated delivery may hold a slot. No runtime effect on its own: nothing acquires from the limiter yet.
joker23
approved these changes
Aug 3, 2026
Review of the primitive found three places where the documented contract and the code disagreed, plus untested paths. All are limiter-local: - Close is now an admission barrier. The doc promised that a shut-down limiter rejects, but the free-slot fast path never consulted the shutdown channel, so callers arriving after Close were admitted whenever a slot was free. Shutdown wiring will assume "Close means no new admissions". - An already-cancelled context is rejected up front instead of being handed a free slot: a dead request would only waste the slot ahead of live ones, and the doc already claimed this behavior. - The waiting count is decremented inside each select arm rather than by a deferred decrement spanning admission, so a caller that has been handed a slot no longer counts against the queue bound (a spanning decrement caused spurious queue-full rejections under exactly the herd load the limiter targets, and inflated Stats.Waiting). - "FIFO" softened to "approximate arrival order": a caller counts against the queue before it parks, so strict ordering was never guaranteed. - Name() is nil-safe like every other method; waiting is an atomic.Int64 like its siblings. Tests now cover Close (admission barrier, unblocks waiters with ok=false, idempotent, safe release afterwards), the cancelled-context paths and their rejection counts, Stats fields, and the waiting-count semantic; each new guard was verified to fail with its guard removed. Queue-state setup polls Stats instead of sleeping, and the queue-full test no longer leaks its waiter.
The previous fix for spurious queue-full rejections moved the waiting-counter decrement between lines inside the woken goroutine, but the window it was meant to close is dominated by that goroutine's wake-up latency: a released slot is handed directly to a parked waiter, who keeps counting against the queue until the scheduler runs it, so every slot turnover shed one admissible caller under exactly the burst load the queue exists for. Measured rejection rates were unchanged by that fix. Replace the queued-callers counter with a single occupancy counter covering slot holders and waiters together, bounded by MaxConcurrent+MaxQueued. A caller reserves its unit once on entry and keeps it from queue to held to released, so the queue-to-held transition no longer passes through a state where the waiter double-counts. This also removes the phantom queue position a caller being rejected briefly held (which could shed an unrelated admissible caller), and makes Stats.Waiting derived and honest instead of over-reading. The release frees occupancy before returning the slot. Close now also covers the release-that-races-Close case: a waiter handed a slot re-checks shutdown and returns the slot instead of keeping it, so once Close returns, a subsequently released slot cannot admit a parked waiter. The Close and Acquire docs state the contract in those terms. Replace two tests that passed unchanged on the code they were written to guard. The new ones fail on both prior revisions: an exact-fit-budget loop (two live callers against a budget of two, so any rejection is spurious) that sheds on most turnovers under either old counter, and a Close-then-release loop asserting the waiter is never admitted. Add guards for occupancy leaks on the cancel, shutdown, and close-unblock paths, and a white-box test for the slot-won-concurrently-with-Close re-check. The release's free-before-send ordering is belt-and-braces; its inversion is not observable at test granularity. Trim the SendTimeout godoc to what exists in this package, pointing the default and throughput-floor behavior at the wiring change that implements them, so published godoc does not describe machinery this branch lacks.
The godox linter forbids TODO comments; say the same thing declaratively: enforcement, the default, and the throughput floor land with the wiring change, and nothing reads the option until then.
The exact-fit regression test's premise was wrong: a spawned waiter from an earlier iteration can still be live (its slot stolen by the main caller's fast path), legitimately filling the budget, so the test failed intermittently on correct code in exactly the CI configuration that runs without the race detector. The oracle now samples the live spawned callers and judges a rejection spurious only when the budget provably had room; spawned callers retry until admitted rather than being silently dropped, and cleanup cancels and drains them on failure. Still fails at iteration 0 on the previous revisions. Add the two killing tests review found missing: an overflow rejection must back out its reservation (without the back-out, every overflow rejection permanently erodes the budget by one unit until the limiter rejects everything), and a negative MaxQueued must normalize to zero rather than poison the occupancy bound. Stats.Waiting now reports a dedicated parked-callers counter instead of a value derived from the occupancy counter, which could read far outside its documented range in both directions (inflated by entrants mid-rejection, clamped to zero during handoffs). The parked counter feeds Stats only and plays no part in admission. The occupancy-leak test assertions observe the occupancy counter directly, since Waiting no longer reflects it. Doc corrections, each matching measured behavior: the reservation scheme's no-double-count guarantee is scoped to the queue-to-held handoff (a cancelled waiter's reservation is returned when its goroutine next runs); Close is not a drain barrier (Held can grow for an instant after it returns); the MaxQueued config doc no longer contradicts the limiter's brief-wait-at-zero semantics; the config section states that nothing consumes it until the wiring lands; the release-ordering comment claims only what is observable.
Swapping the leak-guard assertions to direct occupancy reads last round deleted the only coverage of the parked counter's rejection-arm decrements: dropping either one would inflate Stats.Waiting by one for every client that disconnects while queued -- the exact failure the counter was added to fix -- with the suite green. The guards now assert both the occupancy counter and Waiting, the white-box Close test also checks occupancy directly, and a new test pins that Waiting comes from the parked counter rather than being derived from occupancy (reverting to the derived formula now fails). Bound the exact-fit test's straggler-retry loop with a five-second deadline: against a budget-erosion regression the unbounded loop hung to the package timeout, burning ten minutes before the erosion test's clean diagnostic could run; now it fails in seconds. Doc corrections: Stats.Waiting is bounded by MaxConcurrent+MaxQueued rather than MaxQueued alone (a caller arriving as a slot is released may briefly park without queue capacity); Held and Waiting are independent samples whose sum can transiently exceed the budget, so neither is a basis for exact accounting; the cancellation window is rarer than the handoff one, not shorter; the waitForWaiting helper doc describes the parked counter it now reads.
keelerm84
approved these changes
Aug 5, 2026
kinyoklion
added a commit
that referenced
this pull request
Aug 5, 2026
…es (#786) Adds the write-deadline primitive that the init-concurrency wiring will use to reclaim a slot from a client that cannot keep up, without disturbing a healthy client mid-delivery. ## What changes - **New `internal/initwrite` package.** A `ResponseWriter` wrapper that, for each initialization delivery, arms a write deadline sized to the payload: a throughput floor (so a stalled or too-slow client is cut promptly) combined with an absolute cap (so a client stuck right at the floor on a very large payload is still bounded). It writes strings directly to avoid copying the payload once per connection, and implements `Unwrap` so `http.NewResponseController` can reach the underlying connection to set the deadline. - **`Unwrap` on the logging and metrics `ResponseWriter` wrappers.** These wrap the connection for their own purposes and otherwise hide it, which would stop a `ResponseController` from reaching the real connection. Each now returns its inner writer. ## No behavior change Nothing wraps a connection with `initwrite` yet, so this is inert at runtime. It builds, vets, and passes `-race` on `./internal/initwrite/`, `./internal/logging/`, and `./internal/middleware/`. --- Second of the PRs splitting #782. Builds on #785 (the config + limiter refinement); the wiring PR will follow and depend on both. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > New isolated package plus small middleware forwarding; no init-delivery wiring in this PR, so runtime behavior is unchanged. > > **Overview** > Introduces **`internal/initwrite`**, a `ResponseWriter` wrapper for upcoming init-concurrency limits. It arms **per-chunk write deadlines** from a 64 KiB/s throughput floor plus slack, with an optional **`maxHold`** cap, and supports **poll** (`Wrap`) vs **gated SSE** (`WrapGated` with `Begin`/`End`/`WaitAndFinish`) so idle streams are not left with HTTP/2 self-firing deadlines. It implements **`WriteString`**, **`Flush`**, and **`Unwrap`** for large SSE payloads and `http.NewResponseController`. > > **Logging and metrics middleware** gain matching **`WriteString`** and **`Unwrap`** so deadline control and string writes are not broken or copied at those hops; tests lock in both behaviors. > > **No production wiring yet** — the package is inert until a follow-up PR wraps init deliveries. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6a7372a. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
kinyoklion
added a commit
that referenced
this pull request
Aug 5, 2026
…nto the wiring branch The limiter refinement (#785) and the progress-aware write deadline (#786) have landed on the feature branch, so this branch's own earlier copies of those files conflicted. Resolved by taking the feature branch's versions for everything those PRs own -- config, the limiter, initwrite, and the logging and metrics wrapper files -- leaving this branch's delta as the wiring only: the middleware, stream-provider, and relay endpoint changes plus their docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refines the init-concurrency limiter primitive and configuration that #780 landed on the feature branch, in preparation for wiring it into the poll and stream endpoints.
What changes
PerEnvMax) and a matchingINIT_PER_ENV_MAX_PERCENTconfig option. Both are removed. The limiter now bounds concurrent admissions with a single shared budget: a fixed number of held slots plus a bounded FIFO queue of waiters.Acquireno longer takes an environment key.INIT_SEND_TIMEOUTdocumentation and default now describe it as an absolute cap on how long one gated delivery may hold a slot.Why one shared budget
The budget we need to protect is Relay's overall memory and egress during a reconnect herd; that is a global resource, not a per-environment one. A per-environment gate rejects an environment that is under the global budget while slots sit free, and it adds bookkeeping (a
sync.Mapof gates) for a fairness property we do not need here. A single shared budget is simpler and matches the resource it guards.No behavior change
Nothing acquires from the limiter yet, so this is inert at runtime. It builds, vets, and passes
-raceon./config/and./internal/concurrency/.First of a set of PRs splitting the larger wiring change (#782) into independently reviewable pieces. This one stands alone.
Note
Medium Risk
Changes core concurrency admission logic with subtle race/accounting fixes, but the limiter is not yet used in production paths so customer impact is low until wiring lands.
Overview
Simplifies the init-concurrency limiter and its config ahead of wiring it into poll/stream endpoints. Per-environment fairness is removed:
INIT_PER_ENV_MAX_PERCENT,PerEnvMaxPercent, and the limiter’s per-env gate (sync.Map/Acquire(ctx, envKey)) go away in favor of one global budget (held slots + bounded wait queue).Acquireis nowAcquire(ctx)only.The limiter’s admission accounting is reworked so occupancy is reserved from queue through release (
inFlight+maxOccupancy), with a separateparkedcounter forStats.Waiting, fixing turnover/cancel/overflow leaks that could shed valid callers or erode capacity. Shutdown and cancellation are stricter: already-cancelled contexts and post-Closecallers are rejected even when slots are free;admitre-checks shutdown after winning a slot.Config/docs for
MaxQueuedandSendTimeoutare clarified;SendTimeoutenforcement still lands with endpoint wiring. Runtime behavior is unchanged until that wiring exists.Reviewed by Cursor Bugbot for commit acabacb. Bugbot is set up for automated code reviews on this repo. Configure here.