Repository navigation
Conversation
Since the EG v1.9.2 bump, Envoy Gateway's shutdown waits for the file provider to finish its initial load of the resources, even when the context was canceled before the load started. run returns only after that, so TestRunExtprocStartFailure has been hitting its 10s timeout on CI. The timeout only needs to catch run hanging after the extproc fails, so raise it to 60s. Signed-off-by: Abdulaziz Alharbi <246710009+abdulaziz-dev-lab@users.noreply.github.com>
✅ Deploy Preview for theagentrouter canceled.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/retest |
|
A note on this PR's CI history. The first Build and Test run passed everything except Data Plane Test (macos-latest). Both Unit Test jobs, where https://github.com/theagentrouter/agent-router/actions/runs/37402998623/job/112074199353 I ran |
siddharth1036
left a comment
There was a problem hiding this comment.
LGTM
minor nit, after timeout the t.Fatal still logs expected to fail promptly, worth aligning the log with 60s window
|
Great catch. Thank you! |
|
@siddharth1036 Thanks for the review. Good point about "promptly". The message also blames the extproc startup even though the extproc fails right away. The timeout actually catches @nacx Happy to send a one-line follow-up if you'd like one. |
Description
TestRunExtprocStartFailurehas been flaky on both ubuntu and macOS since the Envoy Gateway v1.9.2 bump (#2787). It fails with "expected extproc start to fail promptly" after its 10s timeout, on main and on PRs alike.When the extproc fails,
runcancels the Envoy Gateway server's context and returns onceserver.ExecuteContextreturns. Since v1.9.2, EG's shutdown waits for every runner goroutine to exit [1][2]. That includes the file provider'sStart, which runs the initial load of the resources even when the context was canceled before it began. With v1.9.0 nothing waited for it.With the CI flags (
-race, atomic coverage), pinned to 4 cores, the time from the cancel toExecuteContextreturning in this test went from 0.15s to 3.12s (median of 10 runs). The wait is mostly single-threaded, and on CI runners, which run several test packages at once, it often goes past 10s. Running the test on one core shared with two busy loops reproduces the failure on main (10.01s). With this change it passes (12.01s).The timeout only needs to catch
runhanging after the extproc fails, which is what #1134 fixed. This raises it to 60s and adds a comment explaining why.Related Issues/PRs (if applicable)
Fixes #2793
Special notes for reviewers (if applicable)
Making
runitself return faster would need EG to skip the initial load once the context is canceled, which is an upstream change.1: https://github.com/envoyproxy/gateway/blob/v1.9.2/release-notes/v1.9.2.yaml
2: envoyproxy/gateway#10128