Sample Strategy #1313
Sample Strategy #1313
Conversation
|
DescriptionSummary By MatterAI
🔄 What ChangedThe 🔍 Impact of the ChangeThis change simplifies the 📁 Total Files Changed
🧪 Test AddedN/A - This pull request modifies an existing test utility script but does not introduce new functional tests for the application logic. 🔒Security VulnerabilitiesN/A - No new security vulnerabilities were introduced or addressed by this change. MotivationThis change refines the test environment setup for the "Sample Strategy" by simplifying the Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Related IssuesN/A Tip Quality Recommendations
Tanka Poem ♫
Sequence DiagramsequenceDiagram
participant TestScript as start-test.js
participant NodeChildProcess as node:child_process
participant ServerApp as build/start-server.js
TestScript->>+NodeChildProcess: spawn('node', ['build/start-server.js', '--headless'], { stdio: 'inherit' })
Note over NodeChildProcess: Executes 'node build/start-server.js --headless'
NodeChildProcess->>+ServerApp: Start server process
ServerApp-->>-NodeChildProcess: Server initialized (using default port)
NodeChildProcess-->>-TestScript: Process spawned successfully
TestScript->>TestScript: app.on('exit', (err) => { ... })
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| const cancelOthers = currentTarget.strategy?.cancelOthers; | ||
|
|
||
| // v1 limitation: do not support sampling when request body is a ReadableStream | ||
| if (request instanceof ReadableStream) { |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Type confusion bug - request instanceof ReadableStream will always be false. The request parameter is a Request object, not a ReadableStream. This check should verify if the request body is a stream.
Fix: Check the request body type instead of the request object type
Impact: Prevents incorrect rejection of streaming requests, ensuring proper validation
| if (request instanceof ReadableStream) { | |
| // v1 limitation: do not support sampling when request body is a ReadableStream | |
| if (request.body instanceof ReadableStream) { |
| const isSuccess = | ||
| (Array.isArray(onStatusCodes) && | ||
| !onStatusCodes.includes(resp?.status)) || | ||
| (!onStatusCodes && resp?.ok) || | ||
| gatewayException; |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Logic error in success condition - the boolean OR operator | should be logical OR ||. This causes incorrect type coercion and wrong success evaluation.
Fix: Use logical OR operator for proper boolean evaluation
Impact: Fixes success detection logic to properly identify successful responses
| const isSuccess = | |
| (Array.isArray(onStatusCodes) && | |
| !onStatusCodes.includes(resp?.status)) || | |
| (!onStatusCodes && resp?.ok) || | |
| gatewayException; | |
| const isSuccess = | |
| (Array.isArray(onStatusCodes) && | |
| !onStatusCodes.includes(resp?.status)) || | |
| (!onStatusCodes && resp?.ok) || | |
| gatewayException; |
| for (const ctl of controllers) { | ||
| try { | ||
| ctl.abort(); | ||
| } catch {} |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Empty catch block silently ignores abort errors, making debugging difficult
Fix: Add minimal error logging for abort operations
Impact: Improves debugging capability while maintaining non-blocking behavior
| } catch {} | |
| } catch (e) { | |
| // Ignore abort errors but log for debugging | |
| console.debug('Abort controller error:', e); | |
| } |
| winnerResolved = true; | ||
| resolveWinner(resp); | ||
| if (cancelOthers) { | ||
| for (const ctl of controllers) { | ||
| try { | ||
| ctl.abort(); | ||
| } catch {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Duplicate abort controller cleanup logic - same code block appears twice (lines 782-788 and 812-818)
Fix: Extract abort cleanup into a reusable function
Impact: Reduces code duplication and improves maintainability
| winnerResolved = true; | |
| resolveWinner(resp); | |
| if (cancelOthers) { | |
| for (const ctl of controllers) { | |
| try { | |
| ctl.abort(); | |
| } catch {} | |
| } | |
| } | |
| const cleanupControllers = () => { | |
| if (cancelOthers) { | |
| for (const ctl of controllers) { | |
| try { | |
| ctl.abort(); | |
| } catch (e) { | |
| console.debug('Abort controller error:', e); | |
| } | |
| } | |
| } | |
| }; | |
| winnerResolved = true; | |
| resolveWinner(resp); | |
| cleanupControllers(); |
| if (externalAbortSignal) { | ||
| if (externalAbortSignal.aborted) { | ||
| controller.abort(); | ||
| } else { | ||
| externalAbortSignal.addEventListener('abort', () => controller.abort()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Race condition - external abort signal could be triggered between the aborted check and event listener registration
Fix: Register event listener first, then check abort status
Impact: Prevents missed abort signals and ensures reliable cancellation
| if (externalAbortSignal) { | |
| if (externalAbortSignal.aborted) { | |
| controller.abort(); | |
| } else { | |
| externalAbortSignal.addEventListener('abort', () => controller.abort()); | |
| } | |
| } | |
| if (externalAbortSignal) { | |
| externalAbortSignal.addEventListener('abort', () => controller.abort()); | |
| if (externalAbortSignal.aborted) { | |
| controller.abort(); | |
| } | |
| } |
|
Clean simplification removing hardcoded port configuration. Code change is straightforward with no critical issues detected. |
Description
New Gateway strategy: Sample
Quick info:
The sample strategy fans out a single gateway request to multiple targets in parallel and returns the first-success response. It’s useful for shaving tail latency, probing multiple providers at once, and improving reliability without waiting for sequential fallbacks.
How it works
strategy.mode = "sample"are invoked concurrently.strategy.on_status_codesis set: first response whose HTTP status is NOT in that list wins. Gateway exceptions also count as a win to break out early.strategy.cancel_others = true, once the winner is decided, all in-flight sibling requests are aborted.false, non-winners are allowed to finish silently.Current limitations
on_status_codesthoughtfully to prevent fast-fail responses from “winning.”Configuration
mode: must be"sample".cancel_others: optional boolean; aborts losers when true.on_status_codes: optional array; statuses to treat as “fail” for winner selection.Minimal example
{ "strategy": { "mode": "sample", "cancel_others": false }, "targets": [ { "provider": "openai", "api_key": "<OPENAI_KEY>", "override_params": { "model": "gpt-4o-mini" } }, { "provider": "google", "api_key": "<GEMINI_KEY>", "override_params": { "model": "gpt-2.5-flash" } } ] }With winner policy guardrails
{ "strategy": { "mode": "sample", "cancel_others": true, "on_status_codes": [400, 401, 403, 404, 409, 422, 429, 500, 502, 503, 504] }, "targets": [ { "provider": "openai", "api_key": "<OPENAI_KEY>", "override_params": { "model": "gpt-4o" } }, { "provider": "groq", "api_key": "<GROQ_KEY>", "override_params": { "model": "meta-llama/llama-4-scout-17b-16e-instruct" } }, { "provider": "google", "api_key": "<GOOGLE_KEY>", "override_params": { "model": "gemini-2.0-flash" } } ] }Request example (curl)
Observability
cancel_others=trueaborted targets show abort errors; withfalse, they complete normally.Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Related Issues