test(harness): raise waitConfigPropagation 5s→10s + maxForks 4→2 (#157) - #169
Merged
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughE2E test infrastructure is updated to improve test reliability. The config propagation polling timeout is increased from 5 to 10 seconds with 50ms retry intervals. Vitest E2E concurrency is reduced from 4 to 2 forks, with documentation added explaining port and etcd contention sources. ChangesE2E Test Infrastructure Stability
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the e2e test harness’s configuration-propagation wait budget to reduce CI flakes caused by slower etcd watch dispatch under parallel Vitest forks.
Changes:
- Increase
waitConfigPropagationcondition-based deadline from 5s to 10s. - Update the helper’s doc comment and error message to reflect the new 10s budget and the CI parallelism context.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+93
to
+98
| const deadline = Date.now() + 10_000; | ||
| while (Date.now() < deadline) { | ||
| if (await condition()) return; | ||
| await new Promise((r) => setTimeout(r, 50)); | ||
| } | ||
| throw new Error("waitConfigPropagation: condition not met within 5s"); | ||
| throw new Error("waitConfigPropagation: condition not met within 10s"); |
Closes #157 (test-infra concern, not a product bug). Issue: the e2e suite's `guardrail-keyword-e2e.test.ts` has flaked three times on CI in the past 24 hours (#157 first occurrence, plus reruns required on PR #165 and #167). Failure mode is the same: `waitConfigPropagation: condition not met within 5s`. Root cause: vitest is configured with `maxForks: 4` (per `vitest.config.ts`), so up to 4 test files run in parallel, each spawning its own `aisix` binary against a SHARED etcd. Each binary opens watches and writes resources via the admin API concurrently with the others. Under that load, etcd watch dispatch latency for the LAST resource in a multi-resource batch (e.g. a Guardrail rule following Model + ApiKey + ProviderKey writes) can exceed the 5s budget. The 5s budget was sized when the suite had ~9 files. The suite is now 20+ files (#161, #163, #165, #167 added embeddings, responses, passthrough, rerank, images). The growth in parallelism load wasn't matched by a budget bump. Fix: raise the deadline to 10s. This: - Eliminates the recurring rerun churn on feature PRs - Preserves the "fail loudly on a genuinely stuck snapshot" property — 10s is still a generous floor; a real bug where propagation hangs indefinitely would still fail clearly - Doesn't change the happy-path latency at all (the helper polls every 50ms and returns as soon as the condition is met — bumping the deadline only affects the sad path) This is a test-infra-only change; no product behavior is affected. The ≤500ms spec target for in-process propagation is unchanged; this is purely the CI test harness's wait budget for slow runners under concurrent load. If 10s proves insufficient as the suite grows further, the next escalation would be to reduce `maxForks` from 4 to 2 (slower wall time, less etcd pressure) — tracked in #157 as a fallback.
After the timeout bump landed, #169's own CI run still flaked with the same `condition not met within 10s` error — meaning etcd watch dispatch latency genuinely exceeds 10s under maxForks=4 with the current suite size (20+ files). The audit on #169 had flagged this as the "product-side hypothesis still open" — confirmed. Apply the documented fallback from #157: cut maxForks from 4 to 2. This halves the concurrent-watcher count against the shared etcd and brings dispatch latency back inside the budget. Trade-off: wall time grows ~1.5-2× per CI run (locally measured ~14s @ maxForks=4 vs ~30s @ maxForks=2, so faster than expected because the extra parallelism wasn't fully utilized anyway — contention dominated). Net for CI is "predictable green" vs "fast but constantly-rerunning". The 10s `waitConfigPropagation` budget from the prior commit stays in place as belt-and-suspenders — even with maxForks=2 the original 5s would still be tight on slow runners. Combined this PR now does: 1. waitConfigPropagation deadline 5s → 10s (`harness/admin.ts`) 2. maxForks 4 → 2 (`vitest.config.ts`) The product-side hypothesis ("etcd watch dispatch degrades non-linearly with concurrent watchers") is now strongly supported and worth a separate product-side investigation tracked in #157 follow-up.
moonming
force-pushed
the
test/e2e-harness-propagation-budget
branch
from
May 9, 2026 14:38
80a4bf2 to
60dc4ae
Compare
moonming
changed the title
moonming
added a commit
that referenced
this pull request
May 9, 2026
Audit on commit d04abe8 returned 0 HIGH, 1 MEDIUM (rebase-after-#169 infra fix), 3 LOW. Applying LOW-1 here: LOW-1: case 2 (json_schema) asserted only response_format fields on the upstream side. A regression that mangled the rest of the request body while preserving response_format would not surface. Added cross-check: pin `model` translation (display name → upstream model_name) and the user message reaches upstream verbatim — same defense case 1's byte-for-byte content assertion provides on the response side. LOW-2 (streaming + response_format coverage gap) and LOW-3 (case 1 comment polish) deferred. MEDIUM-1 (rebase on #169 once it merges to inherit waitConfig budget bump + maxForks reduction) addressed by waiting for #169 to land and rebasing before merge.
moonming
added a commit
that referenced
this pull request
May 9, 2026
Audit on commit d04abe8 returned 0 HIGH, 1 MEDIUM (rebase-after-#169 infra fix), 3 LOW. Applying LOW-1 here: LOW-1: case 2 (json_schema) asserted only response_format fields on the upstream side. A regression that mangled the rest of the request body while preserving response_format would not surface. Added cross-check: pin `model` translation (display name → upstream model_name) and the user message reaches upstream verbatim — same defense case 1's byte-for-byte content assertion provides on the response side. LOW-2 (streaming + response_format coverage gap) and LOW-3 (case 1 comment polish) deferred. MEDIUM-1 (rebase on #169 once it merges to inherit waitConfig budget bump + maxForks reduction) addressed by waiting for #169 to land and rebasing before merge.
moonming
added a commit
that referenced
this pull request
May 9, 2026
Audit on commit d04abe8 returned 0 HIGH, 1 MEDIUM (rebase-after-#169 infra fix), 3 LOW. Applying LOW-1 here: LOW-1: case 2 (json_schema) asserted only response_format fields on the upstream side. A regression that mangled the rest of the request body while preserving response_format would not surface. Added cross-check: pin `model` translation (display name → upstream model_name) and the user message reaches upstream verbatim — same defense case 1's byte-for-byte content assertion provides on the response side. LOW-2 (streaming + response_format coverage gap) and LOW-3 (case 1 comment polish) deferred. MEDIUM-1 (rebase on #169 once it merges to inherit waitConfig budget bump + maxForks reduction) addressed by waiting for #169 to land and rebasing before merge.
moonming
added a commit
that referenced
this pull request
May 9, 2026
* test(e2e): C8 structured outputs / JSON mode passthrough (#151) Third C8 sub-area covered. The first two (#170 tools-cross-provider, #171 vision input parser) surfaced product gaps and are held back pending fixes. JSON mode is the simpler request-passthrough field — no new content-block parsing, no cross-provider translation — and it works. Two cases pinned: 1. `response_format: { type: "json_object" }` — caller asserts upstream-side body has `response_format: {type: "json_object"}` verbatim, and the JSON-shape content reaches the SDK byte-for-byte and parses as valid JSON. 2. `response_format: { type: "json_schema", json_schema: {name, schema, strict} }` — caller asserts the full descriptor (name, full JSON schema, `strict: true`) reaches upstream verbatim. A regression that dropped any sub-field — especially `strict` — would relax upstream's constraints and break schema-conforming callers. Both cases are real production paths: every modern agent / RAG framework that depends on parseable structured output sets `response_format` on every chat completion. A regression that dropped this field would leave the model in default text mode and silently break every JSON-output caller. References: - OpenAI structured-outputs guide <https://platform.openai.com/docs/guides/structured-outputs> - OpenAI Chat Completions response_format spec <https://platform.openai.com/docs/api-reference/chat/create#chat-create-response_format> Refs #151 * test(audit): tighten #172 case 2 per audit LOW-1 Audit on commit d04abe8 returned 0 HIGH, 1 MEDIUM (rebase-after-#169 infra fix), 3 LOW. Applying LOW-1 here: LOW-1: case 2 (json_schema) asserted only response_format fields on the upstream side. A regression that mangled the rest of the request body while preserving response_format would not surface. Added cross-check: pin `model` translation (display name → upstream model_name) and the user message reaches upstream verbatim — same defense case 1's byte-for-byte content assertion provides on the response side. LOW-2 (streaming + response_format coverage gap) and LOW-3 (case 1 comment polish) deferred. MEDIUM-1 (rebase on #169 once it merges to inherit waitConfig budget bump + maxForks reduction) addressed by waiting for #169 to land and rebasing before merge.
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
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.
Summary
Test-infra fix. Closes #157.
The e2e suite's `guardrail-keyword-e2e.test.ts` flaked four times across this and three feature PRs (#156, #165, #167, plus #169's own first CI run) with `waitConfigPropagation: condition not met`. Initial fix bumped the budget from 5s to 10s — and the same flake recurred on this very PR's CI with the new 10s budget, confirming that the parallelism-pressure hypothesis (per the audit's LOW finding and #157's "Real product slowness" note) is the real root cause.
What this PR does
Two changes, belt-and-suspenders:
Root cause confirmed (post-first-attempt)
Each test file's `spawnApp()` opens etcd watches and writes resources via the admin API. Under `maxForks: 4` with 20+ files, multiple `aisix` instances watch concurrently against a single shared etcd. Watch dispatch latency for the LAST resource in a multi-resource batch (e.g. a Guardrail rule following Model + ApiKey + ProviderKey writes) genuinely exceeds even 10s under that load.
Cutting `maxForks` to 2 halves the concurrent-watcher count and was the documented fallback in #157's repro analysis.
Trade-offs
Product-side follow-up
The audit's "product-side hypothesis still open" note (#169 first audit) is now strongly supported. Etcd watch dispatch genuinely degrades non-linearly with concurrent watcher count. This is worth a separate product-side investigation:
Tracked as a follow-up under #157 (will reopen / file separate when this PR closes #157).
Test plan