atenet-router: take parking-lot slots at the park transition, not at admission - #1229
Open
Omer Yahud (omeryahud) wants to merge 3 commits into
Open
atenet-router: take parking-lot slots at the park transition, not at admission#1229Omer Yahud (omeryahud) wants to merge 3 commits into
Omer Yahud (omeryahud) wants to merge 3 commits into
Conversation
…admission The lot admitted every request before its resume lookup, so a full lot shed traffic to already-RUNNING actors — exactly the starvation docs/request-parking.md rules out (agent-substrate#1081), and TestHandleRequestHeaders_ParkingLotFull pinned that behavior as expected. Replace the resumer's singleflight.Group with a per-actor flight registry whose flights signal their park transition (the first retryable error). A caller now waits slot-free while its flight resolves, acquires a slot only once the flight parks, and is shed with the existing 503 "router at capacity" only at that transition — after the single attempt that revealed it would have to wait. Requests resolved on the first attempt never touch the lot, so a saturated lot cannot starve running-actor traffic, and parking.active / parking.wait.duration now count only genuinely parked requests.
Review-driven cleanup, no behavior change: resumeFlight and its lifecycle move to a dedicated file, with the close-once and publish-ordering invariants enforced by methods instead of comments at the call sites — park() owns the parked close (the signaled bool moves into the struct), publish() owns the result-write → registry-delete → done-close order, and runFlight shrinks to the retry loop plus one publish call, with the terminal-state classification split into flightResult().
- Drop the fast-path benchmarks: the before/after numbers live in the PR description; nothing in CI executes benchmarks, so the file only cost maintenance. - Reformat the tests this change adds: nested proto literals one field per line, and the anonymous mock signatures wrapped one parameter per line. No behavior change.
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
1 participant
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.
Fixes #1081
Context
HandleRequestHeadersadmitted every request to the parking lot before itsResumeActorlookup, so a request to an already-RUNNINGactor held a slot for the duration of a millisecond-scale control-plane call — and once the lot was full, such requests were shed with503 "router at capacity"before the lookup even ran. That contradicts the design guarantee indocs/request-parking.md("a saturated lot cannot starve requests to already-running actors, at any lot size"): the fast-path headroom built into the 2× ext_proc circuit-breaker sizing was nullified by the lot check sitting in front of the resume, and one saturated WorkerPool could black-hole routing for every other pool's healthy actors.TestHandleRequestHeaders_ParkingLotFullpinned the broken behavior as expected.Change
A caller now occupies a lot slot only from the moment its resume actually parks — the flight's first retryable failure — so the guarantee holds by construction, with no status probe:
singleflight.Group(resumeFlight{parked, done, result}in a mutex-guarded map, split intoflight.go). The registry keeps singleflight's exact semantics — per-actor dedup, forget-on-completion, leader/joiner outcome labels, caller-cancel-doesn't-abort-flight — and adds the one hooksingleflightcannot express: aparkedchannel closed at the first retryable error. A side-map next toGroupcan't do this race-free, becauseDoChannever tells a joiner which flight generation it attached to.parkedsignal the caller must acquire a slot (lot.enter) to keep waiting, and is shed with the existing503 "router at capacity"when the lot is full — after exactly the one resume attempt that revealed it would have to wait.Max=0)parking.active/wait.duration