Uh oh!
There was an error while loading. Please reload this page.
test(e2e): C7 /v1/embeddings dispatch + response passthrough (#151) - #161
Merged
Conversation
First endpoint covered from #151 C7. Embeddings is one of the two most-used LLM API surfaces (every RAG / semantic-search app hits it heavily). Prior to this file the gateway had **zero** e2e coverage on /v1/embeddings. Two user journeys pinned: 1. Single-string input — `client.embeddings.create({input: "..."})` returns one embedding vector matching the upstream's exact output, with usage counts byte-for-byte preserved. 2. Array input — `client.embeddings.create({input: [s1, s2, s3]})` returns N embeddings in the SAME ORDER as the input array. A regression that re-ordered, deduplicated, or batched-out- of-order would silently corrupt every batched embedding caller's index→vector mapping. Each case also pins the upstream-side wire shape: - Gateway hits `/v1/embeddings` exactly once (path + method) - `Authorization: Bearer sk-mock` header reaches upstream - Body is OpenAI-shape with display name → upstream model_name translation - Caller's input semantically reaches upstream Note on input-shape normalisation: the OpenAI Embeddings API accepts `input` as EITHER a string OR an array of strings. The gateway is observed to normalise single-string into single-element array on the upstream wire — which is spec-compliant per <https://platform.openai.com/docs/api-reference/embeddings/create>. The single-string test accepts either shape (`"hello"` or `["hello"]`) — a test that pinned only one form would over- specify against an implementation choice the spec leaves open. References: - OpenAI Embeddings API spec <https://platform.openai.com/docs/api-reference/embeddings/create> - OpenAI Node SDK embeddings client source Refs #151
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
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
Adds source-blind end-to-end coverage for the gateway’s OpenAI-compatible /v1/embeddings endpoint, exercising real SDK calls against a real spawned aisix process + etcd-propagated config, while pinning upstream dispatch behavior and response passthrough.
Changes:
- Introduces a new e2e test suite covering embeddings for both single-string and array
inputshapes via the officialopenaiNode SDK. - Asserts strict passthrough for embedding vectors + usage counters, plus upstream dispatch invariants (path, auth header, model translation, and input preservation / ordering).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Audit (per CLAUDE.md §8) found one HIGH and two MEDIUM: HIGH H1 (the "loose accept either input shape" assertion masked a gateway-vs-docs contract divergence): The audit checked `docs/api-proxy.md` §4.4 — which I had not — and found the gateway's own published contract reads "input may be a single string or an array; **both pass through**." The gateway today normalises single-string `input` to single-element array on the upstream wire, contradicting its own docs. My loosening to "either shape OK because OpenAI's spec accepts both" was wrong: spec-compliance ≠ scenario invalidation when the gateway has published a narrower contract of its own. Per principle #2 ("test failure = code bug"), the strict assertion was correct and the loosening was masking a real product-vs-docs gap. Resolution per the same pattern as #153 / #154 / #159: - Removed the single-string case from this PR (it would fail against today's gateway behavior). - Filed #162 — `/v1/embeddings` single-string normalisation contradicts docs §4.4 — with the strict-assertion test attached as the regression repro. - Single-string case will be added back to the suite once product + docs are aligned (either A: fix the gateway to preserve shape, or B: update docs to document the normalisation, then test pins the documented shape strictly). MEDIUM M1 (probe shape-check): Tightened the array-input readiness probe from "doesn't throw" to "returns an OpenAI-shape `{object: 'list', data: [...]}` body". A regression that returned 200 with a malformed body would no longer falsely report ready. Matches the discipline in anthropic-upstream-e2e.test.ts. LOW L1 (per-element `object: "embedding"` asserts): The array test now pins `data[i].object === "embedding"` for all three elements, not just the first. Symmetrises with the OpenAI Embeddings spec (each data element carries the literal). Refs #151
Uh oh!
There was an error while loading. Please reload this page.
3 tasks
moonming added a commit
that referenced
this pull request
May 9, 2026
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.
moonming added a commit
that referenced
this pull request
May 9, 2026
… (#169) * test(harness): raise waitConfigPropagation budget 5s → 10s (#157) 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. * test(harness): also reduce maxForks 4→2 (#157 fallback escalation) 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.
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 freeto 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
First endpoint covered from #151's C7 row. Embeddings is one of the two most-used LLM API surfaces (every RAG / semantic-search app hits it heavily). Prior to this PR the gateway had zero e2e coverage on `/v1/embeddings`.
This PR ships one of the two originally-planned cases. The single-string input case surfaced a real product bug while the audit ran — held back as #162, same pattern as #153 / #154 / #159:
inputto single-element array on upstream wire (contradicts docs/api-proxy.md §4.4 'both pass through') #162 — `/v1/embeddings` single-string `input` normalised to single-element array on the upstream wire, contradicting `docs/api-proxy.md` §4.4 ("both pass through").What's pinned
Why this matters
The two real-world failure modes that prior coverage couldn't catch:
Source-blind discipline
Every assertion derives from external contracts:
inputto single-element array on upstream wire (contradicts docs/api-proxy.md §4.4 'both pass through') #162)No internal Rust paths or struct field names referenced.
Independent audit
Per CLAUDE.md §8, an independent audit agent reviewed the initial commit (87cb959). Resolution log:
The HIGH finding was the most important: my initial PR loosened a wire-shape assertion to "accept either string or single-element array" with the rationale "OpenAI's spec accepts both". The audit checked `docs/api-proxy.md` §4.4 — which I had not — and found the gateway's own published contract is narrower than the OpenAI spec ("both pass through"). The loose assertion was masking a real product-vs-docs gap. Per principle #2 ("test failure = code bug"), the strict assertion was correct and my loosening was incorrect.
Test plan
Refs #151.