feat: configurable Envoy route timeout for long-running actor requests - #714
Conversation
Ron Lev (ronlv10)
left a comment
There was a problem hiding this comment.
Thanks for creating this PR! We encountered the same issue when trying to use substrate.
Maya Wang (@mayawang) can you clarify what do you mean here?> |
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
Have we considered using idle_timeout vs timeout here?
idle_timeout sets the maximum time that a stream can exist without any network activity (which feels more relevant for our usecase?). The timer resets every time a byte is sent or received.
You need both. idle_timeout is how long ateom can remain suspended without breaking network connections. request_timeout is how long request can be in-flight, potentially across multiple suspend/resume cycles. |
yanavlasov 's right that we need both, and digging in showed the PR was incomplete as written. We never set stream_idle_timeout, so Envoy applies its 5-minute default — and a turn relaying a non-streaming completion, or one parked across a suspend/resume, sends no bytes while it's progressing. So --route-timeout=30m would still have been cut at 5m with a 408. The knob would have looked like it worked and silently not. ba07535 sets route-level idle_timeout to max(routeTimeout, 5m). Taking the larger keeps the operator's ceiling honest without ever making the idle timer stricter than today — at the 10s default it's a no-op, pinned by new tests. Route-level rather than HCM so it stays scoped to workload traffic. (Naming, so we're on the same knobs: this PR sets the route-level timeout, which is the in-flight bound you mean, Yan. Envoy's HCM request_timeout bounds request receipt, which isn't the one we want.) One call for you: I derived the idle timeout rather than adding a |
Envoy's end-to-end timeout on the workload route is hardcoded at 10s. An actor that legitimately holds a request open longer than that gets cut off: a harness relaying an LLM completion keeps the request open for the whole generation, and the client sees a 504 mid-turn. Add --route-timeout on atenet-router. The default is 10s, so behavior is unchanged, and a non-positive value leaves the default in place. The knob bounds the actor's own handling time only. The resume that may precede a request is covered by request parking and the ext_proc message timeout, both of which already derive from --parked-request-budget. Wired as a flag on the existing router config struct rather than an env read, matching how the parked-request and ext_proc knobs are done, and documented as a commented-out entry in the atenet-router manifest. The test reads the timeout back out of buildRoutes, where Envoy actually picks it up, and pins that route to OriginalDstClusterName: a change that moved actor traffic onto some other route would otherwise leave the test passing while the timeout governed a route nothing uses.
Raising --route-timeout on its own does not lengthen a turn. The HCM sets no stream_idle_timeout, so Envoy applies its 5m default, and a stream carrying no bytes while the actor works is idle by that measure even though the turn is progressing — a non-streaming completion sends nothing until it is done. Envoy resets the stream at 5m whatever the route timeout says, so the knob silently stops working past that point. Set the route-level idle_timeout to the larger of the route timeout and the 5m Envoy already applies. Below 5m the route timeout fires first regardless, so today's behavior is unchanged; above it the operator's ceiling becomes the real one. Route-level rather than HCM-level keeps it scoped to workload traffic. Also corrects the NonPositiveKeepsDefault comment, which implied the flag could produce a zero. It cannot — --route-timeout carries a default. The guard is there because the setter is reachable from any caller and because zero is the one value Envoy reads as "no timeout at all".
7532caf to
ba07535
Compare
|
LGTM |
The actor's harness exposes /v1/chat/completions. For a non-streaming completion it sends nothing back until generation finishes — routinely tens of seconds. The route timeout was a literal 10 * time.Second in buildRoutes, so Envoy gave up at 10s and returned 504 while the actor was still working fine. It's purely a proxy-side ceiling, not an actor failure. (Not to be confused with the 504 in errors.go — that's the router's own ext_proc deadline, which this flag doesn't govern.) |
Summary
Envoy's end-to-end timeout on the workload route is hardcoded at
10sinbuildRoutes. An actor that legitimately holds a request open longer gets cutoff: a harness relaying an LLM completion keeps the request open for the whole
generation, and the client sees a 504 mid-turn.
Adds
--route-timeouton atenet-router, and pairs it with a route-levelidle_timeoutso the ceiling is actually reachable. The default is 10s, sobehavior is unchanged unless an operator passes the flag.
Why the route timeout alone was not enough
Raised in review by Lior Lieberman (@LiorLieberman) and @yan-vlasov, and they were right — the
first version of this PR did not do what it claimed.
We never set
stream_idle_timeouton the HTTP connection manager, so Envoyapplies its default of 5 minutes. Per the HCM proto, that default is
"overridable by the route-level
idle_timeout", and when it fires "the streamis terminated with a 408 Request Timeout error code if no upstream response
header has been received, otherwise a stream reset occurs."
That is exactly this PR's case. A turn relaying a non-streaming completion sends
no bytes at all while the actor is thinking, and a request parked across a
suspend/resume is idle by the same measure. Both are progressing; Envoy cannot
tell. So
--route-timeout=30mwould still have been cut at 5 minutes with a408 — the knob would have looked like it worked and silently not.
routeIdleTimeout()therefore resolves the accompanying idle timeout asmax(routeTimeout, 5m). Taking the larger keeps the operator's ceiling honestwithout ever making the idle timer stricter than it is today: below 5 minutes
the route timeout fires first regardless, so at the 10s default this is a no-op.
Route-level rather than HCM-level, so it stays scoped to workload traffic
instead of every stream through the router. It is derived rather than exposed as
a second
--route-idle-timeoutflag so the two cannot drift apart, with onesilently defeating the other — happy to make it explicit if reviewers prefer.
For naming: what this PR sets is the route-level
timeout, which boundsupstream response time. Envoy's HCM
request_timeoutbounds how long therequest takes to be received, which is not the limit in question here.
Changes
cmd/atenet/internal/router/— addsXdsServer.routeTimeoutwith aSetRouteTimeoutsetter and adefaultRouteTimeoutconst, wired fromrouterConfig.RouteTimeout/--route-timeout. Same shape as the adjacentSetExtProcMessageTimeoutandSetExtProcMaxRequests, and a flag on theexisting config struct rather than an env read, matching the convention the
parked-request work established. Wired in
startEnvoyDataplane.Adds
envoyDefaultStreamIdleTimeout(5m) androuteIdleTimeout(), applied asthe route's
IdleTimeoutinbuildRoutes.A non-positive value leaves the default in place, since Envoy reads a zero route
timeout as no timeout at all.
The knob bounds the actor's own handling time only. The resume that may precede
a request is covered by request parking and the ext_proc message timeout, both
of which already derive from
--parked-request-budget.manifests/ate-install/atenet-router.yamldocuments it as a commented-out entry.Verification
go build ./...,go vet ./...,go test ./...— all pass.xds_test.goreads the timeout back out ofbuildRoutes, where Envoyactually picks it up: default, setter override, and
non-positive-keeps-default. The helper pins that route to
OriginalDstClusterName— a change that moved actor traffic onto some otherroute would otherwise leave the test passing while the timeout governed a
route nothing uses.
Two added subtests cover the pairing:
IdleTimeoutTracksLongerRouteTimeoutand
IdleTimeoutKeepsEnvoyDefaultWhenRouteTimeoutIsShorter.On a live GKE cluster, read back out of Envoy's own
/config_dump. Withthe new image and no flag, the workload route reports
timeout: 10s, so thedefault is genuinely unchanged. With
--route-timeout=5mit reportstimeout: 300s. Same binary, same manifest, only the flag differs.Caveat on that measurement: it was taken before
ingress: route actor ingress through the atunnel mTLS serverlanded, so the route it read was the olddynamic_forward_proxypath to pod-IP:80. After rebasing, the timeoutattaches to the
actor_original_dstroute that replaced it — which is nowpinned by the test above rather than left to inspection. The
idle_timeoutpairing has test coverage only, not a live
/config_dumpread.Regression, resume with parking on the path: a conversation actor that had
been suspended for 4 days was resumed by an ordinary request through the
router — HTTP 200 in 3.74s, exactly one parked request,
parking_wait_duration_seconds{outcome="served"} = 3.459s, no shed and nobudget_exhausted.Follow-up
Per-ActorTemplate (or per-request) configurability, raised by Ron Lev (@ronlv10): agreed
it needs an API and is follow-up shaped rather than something to fold in here.
The global flag remains useful as the cluster-wide ceiling.
Relationship to #465
This is a stopgap for the connected-socket suspend/restore problem tracked in
#465 (suspend-safe actor networking). Once actor network traffic survives
checkpoint/restore natively, much of the need to raise this ceiling should go
away; this just makes the current behavior tunable in the meantime.
Fixes #<issue_number_goes_here>