Rename the flight primitive and its signal to match what they describe
(a flight retries; only requests park), return the 503 denial directly
instead of routing it through a sentinel, restore the original
parking-lot doc comments, and cut the remaining narration comments down
to the invariants the code cannot express on its own.
- 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.
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().
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/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.
Upstream now addresses actors with resources.ActorRef (2dc1fc62) and
formats client-facing error bodies as 'actor <atespace>/<name> ...'
(f83b65d1). Update the parking tests' ResumeActor call sites and
expected message strings accordingly.
The follow-up bowei asked for in place of demos/parking/load.sh: exercise parking through the real Envoy → ext_proc → ateapi → worker path. A per-test 1-worker pool (runtime copied from the installed counter demo, uniquely labeled for scheduler isolation) is oversubscribed by two actors:
- ParkThenServed occupies the worker with actor A, requests suspended actor B (which parks), frees the worker only once the request is OBSERVABLY parked — a new StatuszClient reads the router's parking gauge over a status-port port-forward, so the synchronization point is state, not sleeps — and asserts B is served with the counter greeting inside the budget window, and that the slot is released.
- BudgetExhaustion reuses the resulting state (B holds the only worker), requests A with no relief, and asserts the router's own verdict: 503 with 'no free workers available' and text/plain, in a window whose lower bound proves parking happened and whose upper bound proves the router answered before Envoy's ext_proc timeout could.
Runs on the router's default parking configuration; flag-dependent scenarios (lot-full shed, parking disabled, custom budgets) deliberately remain unit tests because the shared router cannot be reconfigured per test. Verified live on a KinD cluster: served after 0.86s, budget exhausted at 5.006s.
Review feedback (bowei): rather than a fully independent flag, --extproc-max-requests now defaults to 0 = derive twice --parked-request-max, floored at Envoy's own default of 1024 — the lot always fits and keeps an equal share of fast-path headroom at any size, including a small or disabled lot. An absolute-percentage derivation like 120% under-provisions the fast path at small lots, which is why the headroom equals the lot instead. Explicit values still override and keep the >= lot validation, so operators who need a specific breaker retain control.
Review feedback (bowei): the five ParkedRequest* fields on routerConfig become a single ParkedRequestConfig struct — the flags keep their shared prefix and the fields now travel together through the router config, the parking lot, and the resumer. This also collapses the internal parkingConfig into the same type (one config type instead of two mirrors), moves the parked-request validation onto the struct with routerConfig.validate delegating to it, and renames the lot-full test's 'occupy' variable to 'release' per the review thread.
Envoy drops plain Value in ext_proc header mutations, so the content-type header on every immediate response — all 404/5xx error bodies this router generates — has been arriving with an empty value. Found live while verifying the parking demo (a new header set the same way came back empty; content-type turned out to have been silently broken all along). Use RawValue, matching how addAuthorityMutation already encodes the authority rewrite, and pin the encoding with a regression test.
mapResumeError collapsed two real cases into a generic 500: a park budget spent entirely on Aborted conflicts (the wrapped Aborted unwraps past budgetExhaustedError and hit the default arm), and a bare context sentinel from the caller's own context ending (status.Code classifies those Unknown). Add an Aborted arm — 503 with the gRPC description preserved, since 'another operation is in progress' is actionable and retryable — and explicit sentinel checks: Canceled maps to 408 (Envoy's StatusCode enum defines no 499; the stream is dead so the code is observability-only) and DeadlineExceeded to 504.
Writing the test exposed a related body regression: status.Convert on a wrapping error replaces the description with the wrapper's full 'rpc error: ...' string, so budget-exhausted 503 bodies had carried that prefix since the wrapper was introduced. The new statusDescription helper unwraps to the status first; both 503 arms use it, and a regression row pins the clean capacity body through the wrapper.
Add --extproc-max-requests (default 2048) and set circuit_breakers.max_requests on the ext_proc cluster from it, replacing the implicit Envoy default and the hand-maintained doc coupling with a guarantee. Every request's header exchange occupies one slot briefly and every parked request holds one for its entire wait, so startup validation enforces extproc-max-requests >= parked-request-max — a breaker below the lot silently truncates it with Envoy-generated 503s that bypass parking.rejected. The default leaves the lot's worth of fast-path headroom (1024 lot / 2048 breaker), so a saturated lot cannot starve requests to already-running actors.
The ext_proc cluster sets no explicit circuit_breakers, so Envoy's default max_requests=1024 applies — and every parked request holds one ext_proc stream, i.e. one active request against that cluster. A 2048 lot was therefore half unreachable: requests 1025+ would be rejected by Envoy itself, with 503s that never reach the lot and never count in parking.rejected. Set the default to 1024 to match, and document the coupling at the constant, at buildCluster, and in the design doc, including what raising the flag beyond 1024 requires (an explicit circuit_breakers.max_requests on the cluster). Also drop Unavailable from the docs' non-retryable examples — it became retryable-while-parked in the previous commit.
The budget clock starts with a flight's first caller; requests de-duplicated onto an in-flight resume share its remaining budget and outcome, so a late joiner can see budget_exhausted after waiting far less than a full budget itself. That trade is inherent to collapsing a hot actor's requests into one control-plane RPC — state it explicitly in the design doc, the flight comment, and the flag help instead of implying a per-request guarantee. Also notes that wait-duration samples record each request's own parked time, so sub-budget budget_exhausted samples are expected under sustained saturation.
A parked request could fail on an ateapi blip despite having budget remaining: retryable() rejected Unavailable, so a control-plane rolling restart failed every in-flight parked request on the single most common transient condition — against the feature's purpose of riding out momentary conditions. Make Unavailable retryable while parking is enabled (the budget still bounds the wait); disabled mode keeps the fail-fast behavior. On budget exhaustion the wrapped Unavailable maps to 503 via the existing path.
Pins both halves of the resumer's detached-context design, which had no coverage: a caller that disconnects while parked receives context.Canceled (classified as the 'canceled' parking outcome) without aborting the shared in-flight resume, and a caller arriving after the disconnect is served by that same single RPC.
The budget-exhaustion wrap was gated on errors.Is(err, context.DeadlineExceeded), but when the park budget expires while a ResumeActor RPC is in flight, gRPC surfaces a *status* error with code DeadlineExceeded that does not match the context sentinel. The wrap was skipped and the client saw a generic 504 timeout (metric outcome 'error') instead of the intended 503 'no free workers available' (outcome 'budget_exhausted') — exactly the misreporting the wrapper exists to prevent, on the path that only appears when ateapi is slow, i.e. under the load parking targets. Gate on the budget context itself, which is the loop's only deadline source and covers both landing spots. The new regression test blocks the mock RPC until the budget cancels it and returns status.FromContextError, as a real gRPC client does.
- Adopt the ParkedRequest* vocabulary for parking flags and config (bowei's suggestion): --parked-request-budget / --parked-request-max, matching fields and default consts.
- Make the parked-retry backoff configurable: --parked-request-retry-interval/-factor/-jitter, validated at startup (factor >= 1, jitter in [0,1)); the backoff still has no cap and no attempt limit, so the budget alone bounds the wait.
- Resolve the effective parking config once in Run() so the resumer's retry loop and the Envoy ext_proc timeout always agree, even when the budget flag is set non-positive.
- Drop timeline-relative wording from docs and identifiers (failFastResumeBudget, fail-fast behavior).
- Guard the parking-lot counter against going negative, loudly.
- Document exactly when the wait-duration metric is recorded and what each outcome label means.
Review feedback: folding park-budget exhaustion into the generic 'error' outcome hid the one signal operators need from this metric -- that requests waited the full budget and the pool never freed (capacity problem), as opposed to resumes failing outright (fault). The resumer now marks the surfaced capacity error with a wrapper that unwraps to the underlying gRPC status, so the HTTP mapping is unchanged and the wait-duration histogram gains a budget_exhausted outcome label.
Review feedback (thockin, bowei): parking had three overlapping disabled states -- a nil lot, an enabled flag, and an (accidental) maxParked=0 shed-everything mode. Collapse them into one: --parking-max-parked=0 disables parking, the boolean flag and the nil-lot special case are gone, and a zero-capacity lot can no longer reject every request without attempting a resume.
Review feedback: plain mutex-based exclusion is easier to reason about and to extend than the atomic CAS loop, and the lot is touched only twice per request around a resume that takes orders of magnitude longer, so the atomics bought nothing measurable.
An oversubscribed WorkerPool (2 workers, several actors) that exercises the router parking path: requests to a saturated pool park and retry instead of failing fast, and are served once capacity frees up. Includes load.sh and --deploy-demo-parking / --delete-demo-parking wiring in hack/install-ate.sh.
The ext_proc filter hard-coded MessageTimeout=5s, so Envoy abandoned a parked request (HTTP 500) long before the router's park budget elapsed. Make it configurable via SetExtProcMessageTimeout and set it to --parking-max-wait + margin when parking is enabled, so Envoy holds the request open until the router itself resolves or sheds it.
wait.Backoff zeroes its Steps once the per-attempt delay reaches Cap, so the resume retry loop gave up after ~7 steps (~5s) regardless of --parking-max-wait. Drop the Cap and use a gentle backoff (500ms x1.1, no Cap) so the budget context alone bounds the wait; the slow growth keeps the inter-retry gap small (~3.5s by 30s) on its own. Add a regression test asserting the backoff sets no Cap.
Replace the bare-string park-outcome constants with a parkOutcome string type and rename the classifier to parkOutcomeFor, so the parking.wait.duration outcome label is type-checked rather than an arbitrary string.
When a request targets a suspended actor, the router resumes it via the
control plane before routing. A momentarily saturated worker pool makes
ResumeActor return FailedPrecondition ("no free workers available"), which
the router previously turned straight into a 503. In an oversubscribed
system that shortage usually clears within milliseconds as another actor
suspends and frees its worker, so failing fast was wasteful.
Park such requests instead: retry the resume on FailedPrecondition (in
addition to the existing Aborted conflict) until the actor becomes routable
or a bounded wait elapses, capped by a fixed-capacity admission lot that
sheds excess load. On budget expiry the underlying capacity error is
surfaced so the HTTP boundary maps it faithfully. singleflight still
collapses concurrent waiters for the same actor into one resume RPC.
Parking can be disabled to preserve the legacy fail-fast behavior.
- parking.go: parkingLot (bounded, non-blocking admission gate) + config
- resumer.go: retryable predicate + configurable park budget
- extproc.go: admit each request to the lot around the resume call
- metrics.go: parking.active / wait.duration / rejected instruments
- errors.go: parkingFullErr (503 "router at capacity")
- router.go: --parking-enabled / --parking-max-wait / --parking-max-parked
- status.go + dashboard.html: Request Parking status card
- docs/request-parking.md: feature documentation
Omer Yahud