feat: Add request parking to the atenet router by omeryahud · Pull Request #221 · agent-substrate/substrate

Omer Yahud (omeryahud) · 2026-06-11T07:12:13Z

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 might pass quickly as another actor suspends and frees its worker, so failing fast was wasteful.

Park such requests instead: retry the resume on FailedPrecondition 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.

Fixes #27

Tests pass
Appropriate changes to documentation are included in the PR

google-cla · 2026-06-11T07:12:23Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Tim Hockin (thockin)

Took a quick spin through this, emphasis on quick.

Tim Hockin (thockin) · 2026-06-23T23:47:40Z

+| ------------------------------------- | --------------------------------- |
+| `OK`                                  | Route to worker                   |
+| `Aborted` (concurrent resume)         | Retry (always)                    |
+| `FailedPrecondition` (no free worker) | **Park & retry** (when enabled)   |


FailedPrecondition can mean multiple things, I think we should think more carefully about what error code we want for this specific case. NotFound? ResourceExhausted?

Tim Hockin (thockin) · 2026-06-23T23:48:18Z

+| `DeadlineExceeded`                    | Fail fast → `504`                 |
+| `PermissionDenied` / `Unauthenticated`| Fail fast → `403` / `401`         |
+
+When parking is **disabled** (`--parking-enabled=false`), the router preserves


Is this different than just setting --parking-max-parked=0 ?

Tim Hockin (thockin) · 2026-06-23T23:56:03Z

+// legacyResumeBudget is the total time the resumer spends retrying a resume when
+// request parking is disabled. It preserves the historical fail-fast-on-capacity
+// behavior (only concurrent-update conflicts are retried).
+const legacyResumeBudget = 15 * time.Second


I know it's not the scope of this PR, but we should not be carrying legacy yet :)

Also, the use of "Aborted" for concurrency is suspect...

Bowei Du (bowei) · 2026-07-06T16:49:11Z

@@ -0,0 +1,96 @@
+# Request Parking (atenet router)


[aside] we should figure out where the small design doc snippets should be organized.

Tim Hockin (@thockin) Julian Gutierrez Oschmann (@juli4n) -- regarding general repo organization.

Bowei Du (bowei)

Reviewing in parts. I just took a look at parking.go

Bowei Du (bowei) · 2026-07-08T16:54:44Z

+// Default request-parking parameters. See parkingConfig for the meaning of each
+// field; these are also the flag defaults wired up in NewCmd.
+const (
+	defaultParkingEnabled   = true


Do we need this flag if to max parked is set to 0?

Bowei Du (bowei) · 2026-07-08T16:56:03Z

+	parkOutcomeServed   = "served"   // resume succeeded and the request was routed
+	parkOutcomeTimeout  = "timeout"  // the request's deadline elapsed while parked
+	parkOutcomeCanceled = "canceled" // the client disconnected while parked
+	parkOutcomeError    = "error"    // resume failed (including park-budget exhaustion)


I think we should track budget exhaustion explicitly

Bowei Du (bowei) · 2026-07-08T16:56:25Z

+// failing the request immediately. maxParked bounds how many requests may be
+// parked at once so the router sheds load rather than queueing without bound.
+type parkingConfig struct {
+	enabled   bool


see comment on maxParked ==0

Bowei Du (bowei) · 2026-07-08T16:57:30Z

+// ok=false means the lot is full and the request should be shed without
+// waiting. When parking is disabled every request is admitted and no slot
+// accounting or metrics are recorded.
+func (l *parkingLot) enter(ctx context.Context) (release func(outcome string), ok bool) {


we should type the outcome as a go enum (string typed) instead of raw string

e.g.

type parkingLotOutcome string

Bowei Du (bowei) · 2026-07-08T17:00:49Z

+			l.metrics.recordRejected(ctx)
+			return nil, false
+		}
+		if atomic.CompareAndSwapInt64(&l.active, cur, cur+1) {


Can we use mutex based exclusion for now instead of raw atomics?

At some point under a lot of load, we may want to switch to atomic vars, but I would like to take that step when we understand that this is worth taking on the complexity. This is not a statement about correctness of the current code, but it does complicate any future extensions or adjustments to the semantics of the parking lot code.

Bowei Du (bowei) · 2026-07-08T17:01:41Z

+// waiting. When parking is disabled every request is admitted and no slot
+// accounting or metrics are recorded.
+func (l *parkingLot) enter(ctx context.Context) (release func(outcome string), ok bool) {
+	if l == nil || !l.cfg.enabled {


This might be overkill --

You have a nil behavior and a cfg.enabled behavior.

Also see my comment about maxParked == 0 case.

Bowei Du (bowei) · 2026-07-08T17:02:14Z

+// parkOutcome classifies a completed resume attempt for the wait-duration
+// metric. A budget-exhausted park surfaces the underlying capacity error and is
+// reported as parkOutcomeError.
+func parkOutcome(err error) string {


make outcome typed..

Omer Yahud (omeryahud) · 2026-07-08T17:18:56Z

Thanks Bowei Du (@bowei) & Tim Hockin (@thockin) !
I'll address your comments and let you know once this is ready for another review

Bowei Du (bowei)

Generally lgtm, we should be able to merge after the comments are addressed.

Bowei Du (bowei) · 2026-07-09T15:55:56Z

+}
+
+// enter attempts to reserve a parking slot. On success it returns a release
+// func and ok=true; the caller MUST invoke release exactly once (passing the


nit: I think since you use sync.Once, calling release() multiple times technically doesn't matter -- only the first instance does anything. To be clear, code that is sloppy and calls it more than once is probably a signal of other badness.

Bowei Du (bowei) · 2026-07-09T16:01:57Z

+// legacyResumeBudget is the total time the resumer spends retrying a resume when
+// request parking is disabled. It preserves the historical fail-fast-on-capacity
+// behavior (only concurrent-update conflicts are retried).
+const legacyResumeBudget = 15 * time.Second


Bowei Du (bowei) · 2026-07-09T16:04:27Z

+	// parkEnabled makes transient worker-pool saturation (FailedPrecondition)
+	// retryable, so a request is parked and retried until budget rather than
+	// failing immediately.
+	parkEnabled bool


I'm thinking instead of having extra flag, user can disable if park max == 0? What do you think? This should do the same thing?

Bowei Du (bowei) · 2026-07-10T20:26:32Z

+// ("no free workers available") becomes retryable and the resume is retried for
+// up to maxWait; a non-positive maxWait keeps the default budget. When disabled,
+// the resumer preserves its legacy fail-fast-on-capacity behavior.
+func withParking(enabled bool, maxWait time.Duration) resumerOption {


Given there is only one option right now, this is somewhat overkill. Are we planning on having more options?

Bowei Du (bowei) · 2026-07-10T20:27:33Z

+// (NotFound, Unavailable, DeadlineExceeded, ...) are returned to the caller so
+// the HTTP boundary can map them with full fidelity.
+func (r *ActorResumer) retryable(err error) bool {
+	switch status.Code(err) {


would prefer if you did this:

if ! r.parkEnabled { return false }

then we don't have to worry about maintaining this in the switch

Bowei Du (bowei) · 2026-07-10T20:33:28Z

+	// Request parking: hold and retry requests whose actor cannot be served
+	// immediately due to transient worker-pool saturation, instead of failing
+	// fast. A non-positive ParkingMaxParked disables parking. See parkingConfig.
+	ParkingMaxWait   time.Duration


I think you call this budget in other places. Do we want to align the names?

I suggest:

ParkedRequestBudget

ParkedRequestMax

Bowei Du (bowei) · 2026-07-10T20:34:49Z

@@ -0,0 +1,149 @@
+#!/usr/bin/env bash


I would prefer if this was written as an e2e test.

You can add the e2e in a follow up PR.

Bowei Du (bowei) · 2026-07-10T22:05:27Z


+// parkingFullErr returns a 503 reqError signaling that the router's parking lot
+// is at capacity, so the request was shed without waiting. Clients should retry.
+func parkingFullErr(actorID string) error {


Leave a TODO -- this will need to include the atespace and actorID is being renamed to actorName.

Bowei Du (bowei) · 2026-07-10T22:07:21Z


+// parkingFullErr returns a 503 reqError signaling that the router's parking lot
+// is at capacity, so the request was shed without waiting. Clients should retry.
+func parkingFullErr(actorID string) error {


Leave a TODO -- this will need to include the atespace and actorID is being renamed to actorName.

Bowei Du (bowei) · 2026-07-10T22:08:46Z

+	// backpressure instead of queueing without bound.
+	release, ok := s.parking.enter(ctx)
+	if !ok {
+		return nil, metadata, "", "", "", parkingFullErr(actorID)


add a log here that the parklot is full

Keith Mattix II (keithmattix) · 2026-07-22T00:52:04Z

 	cmd.Flags().StringVar(&cfg.AteapiServerName, "ateapi-server-name", "", "SNI / hostname expected on the ateapi server cert. Optional.")
 	cmd.Flags().StringVar(&cfg.AteapiTokenFile, "ateapi-token-file", ateapiauth.DefaultServiceAccountTokenFile, "Projected SA token file used as Bearer credential. Required for jwt.")
+	cmd.Flags().DurationVar(&cfg.ParkingMaxWait, "parking-max-wait", 30*time.Second, "Maximum time a request may be parked (held and retried) waiting for its actor to become routable")
+	cmd.Flags().IntVar(&cfg.ParkingMaxParked, "parking-max-parked", 2048, "Maximum number of requests that may be parked simultaneously; excess requests are shed with 503. 0 disables parking (requests fail fast on worker-pool saturation)")


👍🏾 park by default seems reasonable

Keith Mattix II (keithmattix) · 2026-07-22T00:55:56Z

+
 	slog.InfoContext(ctx, "ResumeActor", slog.String("atespace", atespace), slog.String("actor", actorName))
 	actor, err := s.resumer.ResumeActor(ctx, atespace, actorName)
+	release(parkOutcomeFor(err))


nit: feels like something that should be deferred so future early returns or blocking code don't leave requests forever parked

Keith Mattix II (keithmattix) · 2026-07-22T00:57:07Z

+	// A 1-slot lot with the slot already occupied deterministically simulates a
+	// full lot without needing a concurrent in-flight request.
+	s := NewExtProcServer(50051, clientMock, nil, parkingConfig{maxWait: time.Second, maxParked: 1}, nil)
+	occupy, ok := s.parking.enter(context.Background())


nit: isn't occupy a callback to free the slot? The naming is a bit confusing

Keith Mattix II (keithmattix) · 2026-07-22T00:58:25Z

+	// (labeled by outcome); parking.rejected counts requests shed because the
+	// parking lot was full.
+	parkingActiveMetricName   = "atenet.router.parking.active"
+	parkingWaitMetricName     = "atenet.router.parking.wait.duration"


~~The cardinality of this feels like it could be explosive if it's every request. Maybe a histogram across all requests might be more scalable?~~

Saw that this is what's implemented. Might be good to add a suffix (e.g _duration_ms) or something but looks good wrt scale

Keith Mattix II (keithmattix) · 2026-07-22T01:06:00Z

+// Park-wait outcomes, recorded on the parking.wait.duration histogram.
+const (
+	parkOutcomeServed          parkOutcome = "served"           // resume succeeded and the request was routed
+	parkOutcomeBudgetExhausted parkOutcome = "budget_exhausted" // the park budget elapsed while still blocked on a retryable condition


Might be good to have a metric for this as well, separate from rejected since that's only shed requests

Keith Mattix II (keithmattix) · 2026-07-22T01:06:34Z

+// a non-positive maxParked disables parking entirely.
+type parkingConfig struct {
+	maxWait   time.Duration
+	maxParked int


Super nit: would uint be better?

Keith Mattix II (keithmattix) · 2026-07-22T01:10:17Z

+	return func(outcome parkOutcome) {
+		once.Do(func() {
+			l.mu.Lock()
+			l.active--


nit: maybe add a lower bound to ensure this doesn't go negative. I know that would only happen because of other bugs though

Keith Mattix II (keithmattix) · 2026-07-22T01:10:53Z

+// legacyResumeBudget is the total time the resumer spends retrying a resume when
+// request parking is disabled. It preserves the historical fail-fast-on-capacity
+// behavior (only concurrent-update conflicts are retried).
+const legacyResumeBudget = 15 * time.Second


Omer Yahud (omeryahud) · 2026-07-22T05:53:43Z

Will be addressing comments next week (after k8s code freeze)

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

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.

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.

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.

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.

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.

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: 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.

Bowei Du (bowei) self-assigned this Jun 12, 2026

Omer Yahud (omeryahud) force-pushed the worktree-request-parking branch from 4b27a7a to 9021cc5 Compare June 15, 2026 07:55

Omer Yahud (omeryahud) marked this pull request as ready for review June 15, 2026 08:24

Omer Yahud (omeryahud) marked this pull request as draft June 15, 2026 08:24

Omer Yahud (omeryahud) force-pushed the worktree-request-parking branch from 9021cc5 to 4a8f978 Compare June 15, 2026 08:52

Omer Yahud (omeryahud) marked this pull request as ready for review June 15, 2026 09:39

Omer Yahud (omeryahud) mentioned this pull request Jun 15, 2026

Router needs to park requests and wait for capacity #27

Open

Tim Hockin (thockin) reviewed Jun 23, 2026

View reviewed changes

Omer Yahud (omeryahud) force-pushed the worktree-request-parking branch from 4a8f978 to 2d3bbf7 Compare July 6, 2026 16:32

Bowei Du (bowei) reviewed Jul 6, 2026

View reviewed changes

Bowei Du (bowei) reviewed Jul 8, 2026

View reviewed changes

Bowei Du (bowei) reviewed Jul 10, 2026

View reviewed changes

Omer Yahud (omeryahud) force-pushed the worktree-request-parking branch 2 times, most recently from 6c325aa to 644edfa Compare July 21, 2026 22:17

Keith Mattix II (keithmattix) reviewed Jul 22, 2026

View reviewed changes

Omer Yahud (omeryahud) added 8 commits July 25, 2026 22:31

atenet/router: make parkOutcome a named type

bf756c8

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.

Omer Yahud (omeryahud) force-pushed the worktree-request-parking branch from 644edfa to 3a12610 Compare July 25, 2026 19:37

Uh oh!

Conversation

Omer Yahud (omeryahud) commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla Bot commented Jun 11, 2026

Uh oh!

Tim Hockin (thockin) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bowei Du (bowei) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Omer Yahud (omeryahud) commented Jul 8, 2026

Uh oh!

Bowei Du (bowei) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Omer Yahud (omeryahud) commented Jun 11, 2026 •

edited

Loading