perf(onboard): reuse validated inference probes - #6323
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces repeated onboard validation overhead by introducing a process-local success cache for OpenAI-compatible endpoint probes, allowing identical validations within the same CLI process to reuse prior successful results (with a 10-minute TTL) while preserving existing failure/fallback semantics.
Changes:
- Added a process-local cache for successful OpenAI-like probe validations with TTL and trace events for cache hit/store.
- Refactored endpoint normalization into a shared helper to ensure equivalent URLs (e.g., trailing slashes) share behavior.
- Added/updated tests and harness helpers to assert correct cache reuse and to prevent unsafe cross-API reuse (e.g., Responses-only results not satisfying chat-only probes).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/lib/inference/onboard-probes.ts | Adds OpenAI-like probe cache helpers, cache lookup/store, trace events, and endpoint normalization. |
| src/lib/inference/onboard-probes.test.ts | Adds cache-focused tests and clears the cache after each test for isolation. |
| src/lib/inference/onboard-probes-curl-harness.ts | Adds a fake curl script helper to record URLs and simulate Responses tool-call success for cache behavior tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bf5af8c to
7f3c1af
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds in-memory TTL caching for OpenAI-like probe validation results, reuses cached probe outcomes on matching requests, and expands the curl harness and tests to verify cache reuse and endpoint-specific behavior. ChangesProbe validation caching
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant probeOpenAiLikeEndpoint
participant ValidationCache
participant FakeCurl
Test->>probeOpenAiLikeEndpoint: probe(endpoint, model, apiKey, options)
probeOpenAiLikeEndpoint->>ValidationCache: lookup normalized cache key
alt cache hit
ValidationCache-->>probeOpenAiLikeEndpoint: cached { ok, api, label }
probeOpenAiLikeEndpoint-->>Test: return cached result
else cache miss
probeOpenAiLikeEndpoint->>FakeCurl: run validation probe
FakeCurl-->>probeOpenAiLikeEndpoint: recorded URL + 200 payload
probeOpenAiLikeEndpoint->>ValidationCache: store successful result
probeOpenAiLikeEndpoint-->>Test: return probe result
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/inference/onboard-probes.test.ts (1)
813-844: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't isolate the compatibility-guard behavior it implies.
This test proves that two probes with different
requirements(viarequireResponsesToolCallingvsskipResponsesProbe) each run and produce distinct URLs — but that's fully explained by the cache key already differing (requirements are embedded ingetOpenAiLikeProbeCacheKey), independent ofcanReuseOpenAiLikeProbeCacheEntry's cross-compatibility logic. The test would pass identically even if that compatibility function were deleted entirely, so it doesn't actually exercise the guard it appears intended to validate.As per path instructions, tests should "[flag] conditionals that make a test pass without exercising its claim." Consider a test that forces a cache-key collision with a compatibility mismatch (if such a scenario is even reachable given the current key design), or otherwise clarify that this test is only verifying key-based cache segregation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/onboard-probes.test.ts` around lines 813 - 844, The current test only verifies that different probe requirements produce different cache entries, not that canReuseOpenAiLikeProbeCacheEntry rejects an incompatible reuse. Update the spec around probeOpenAiLikeEndpoint/getOpenAiLikeProbeCacheKey so it forces a cache-key collision and then asserts the compatibility guard blocks reuse, or rename/adjust the test to explicitly state it is only checking cache-key segregation. Keep the assertion focused on the compatibility behavior rather than the distinct URLs alone.Source: Path instructions
src/lib/inference/onboard-probes-curl-harness.ts (1)
69-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate of
makeResponsesFallbackUrlRecordingFakeCurlScript.This new script (lines 69-95) is identical to
makeResponsesFallbackUrlRecordingFakeCurlScript(lines 44-67) except for the/responsessuccess payload. Consider extracting a shared template that takes the/responsespayload as a parameter to avoid two near-identical shell scripts drifting apart over time.♻️ Suggested consolidation
-export function makeResponsesToolCallUrlRecordingFakeCurlScript(): string { - return `#!/usr/bin/env bash -outfile="" -url="" -while [ "$#" -gt 0 ]; do - case "$1" in - -o) outfile="$2"; shift 2 ;; - -w|-d|--config) shift 2 ;; - http://*|https://*) url="$1"; shift ;; - *) shift ;; - esac -done -n=$(cat "${HARNESS_COUNTER}") -n=$((n + 1)) -echo "$n" > "${HARNESS_COUNTER}" -printf '%s' "$url" > "${HARNESS_TMPDIR}/request-$n-url.txt" -if echo "$url" | grep -q '/responses$'; then - printf '%s' '{"output":[{"type":"function_call","name":"emit_ok","arguments":"{\"value\":\"OK\"}"}]}' > "$outfile" -else - printf '%s' '{"choices":[{"message":{"content":"OK"}}]}' > "$outfile" -fi -printf '200' -`; -} +function makeUrlRecordingFakeCurlScript(responsesPayload: string): string { + return `#!/usr/bin/env bash +outfile="" +url="" +while [ "$#" -gt 0 ]; do + case "$1" in + -o) outfile="$2"; shift 2 ;; + -w|-d|--config) shift 2 ;; + http://*|https://*) url="$1"; shift ;; + *) shift ;; + esac +done +n=$(cat "${HARNESS_COUNTER}") +n=$((n + 1)) +echo "$n" > "${HARNESS_COUNTER}" +printf '%s' "$url" > "${HARNESS_TMPDIR}/request-$n-url.txt" +if echo "$url" | grep -q '/responses$'; then + printf '%s' '${responsesPayload}' > "$outfile" +else + printf '%s' '{"choices":[{"message":{"content":"OK"}}]}' > "$outfile" +fi +printf '200' +`; +} + +export function makeResponsesFallbackUrlRecordingFakeCurlScript(): string { + return makeUrlRecordingFakeCurlScript( + '{"output":[{"type":"message","content":[{"type":"output_text","text":"OK"}]}]}', + ); +} + +export function makeResponsesToolCallUrlRecordingFakeCurlScript(): string { + return makeUrlRecordingFakeCurlScript( + '{"output":[{"type":"function_call","name":"emit_ok","arguments":"{\\\\"value\\\\":\\\\"OK\\\\"}"}]}', + ); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/onboard-probes-curl-harness.ts` around lines 69 - 95, `makeResponsesToolCallUrlRecordingFakeCurlScript` is a near-duplicate of `makeResponsesFallbackUrlRecordingFakeCurlScript`, so the shared curl harness logic should be consolidated. Extract the common shell-script construction into a helper that accepts the `/responses` success payload as a parameter, and have both functions call it with their respective payloads. Keep the unique behavior in the payload argument only so the two fake-curl scripts do not drift apart.src/lib/inference/onboard-probes.ts (1)
316-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompatibility check is unreachable given the cache-key design.
getOpenAiLikeProbeCacheKeyalready embeds the fullrequirementsobject (line 303) in the JSON string used as the map key. This meansopenAiLikeProbeValidationCache.get(cacheKey)(line 340) can only ever return an entry whose stored requirement fields are identical to the requested ones — the cross-compatibility branches here (e.g.entry.skipResponsesProbevsrequested.skipResponsesProbe, line 329-330) can never actually diverge and returnfalsein practice.The comment at lines 327-329 describes intent (guard against a chat-only smoke satisfying a later Responses-required validation) that is already enforced purely by key mismatch, before this function is ever invoked. This isn't a functional bug today (the fallback behavior is safe), but it's misleading dead logic that could confuse future maintainers into believing there's a meaningful compatibility guarantee here, or into reusing this pattern incorrectly if the key is ever simplified.
Consider either: (a) removing the requirements-derived compatibility checks and keeping only the TTL/expiry check since exact-key matching already guarantees compatibility, or (b) if broader/partial cache reuse across differing requirements is actually desired, remove requirements from the cache key and rely solely on this function to gate reuse.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/onboard-probes.ts` around lines 316 - 336, The compatibility checks in canReuseOpenAiLikeProbeCacheEntry are dead logic because getOpenAiLikeProbeCacheKey already includes the full requirements object, so openAiLikeProbeValidationCache.get(cacheKey) only returns exact matches. Simplify canReuseOpenAiLikeProbeCacheEntry to keep only the TTL/expiry validation, or, if broader reuse is intended, move the requirements matching out of the cache key and make this function the sole gate for reuse. Use the existing symbols getOpenAiLikeProbeCacheKey, openAiLikeProbeValidationCache.get, and canReuseOpenAiLikeProbeCacheEntry to update the behavior consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/inference/onboard-probes-curl-harness.ts`:
- Around line 69-95: `makeResponsesToolCallUrlRecordingFakeCurlScript` is a
near-duplicate of `makeResponsesFallbackUrlRecordingFakeCurlScript`, so the
shared curl harness logic should be consolidated. Extract the common
shell-script construction into a helper that accepts the `/responses` success
payload as a parameter, and have both functions call it with their respective
payloads. Keep the unique behavior in the payload argument only so the two
fake-curl scripts do not drift apart.
In `@src/lib/inference/onboard-probes.test.ts`:
- Around line 813-844: The current test only verifies that different probe
requirements produce different cache entries, not that
canReuseOpenAiLikeProbeCacheEntry rejects an incompatible reuse. Update the spec
around probeOpenAiLikeEndpoint/getOpenAiLikeProbeCacheKey so it forces a
cache-key collision and then asserts the compatibility guard blocks reuse, or
rename/adjust the test to explicitly state it is only checking cache-key
segregation. Keep the assertion focused on the compatibility behavior rather
than the distinct URLs alone.
In `@src/lib/inference/onboard-probes.ts`:
- Around line 316-336: The compatibility checks in
canReuseOpenAiLikeProbeCacheEntry are dead logic because
getOpenAiLikeProbeCacheKey already includes the full requirements object, so
openAiLikeProbeValidationCache.get(cacheKey) only returns exact matches.
Simplify canReuseOpenAiLikeProbeCacheEntry to keep only the TTL/expiry
validation, or, if broader reuse is intended, move the requirements matching out
of the cache key and make this function the sole gate for reuse. Use the
existing symbols getOpenAiLikeProbeCacheKey, openAiLikeProbeValidationCache.get,
and canReuseOpenAiLikeProbeCacheEntry to update the behavior consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1489bfb3-a49f-4481-9604-d7fe4cb2d9a6
📒 Files selected for processing (3)
src/lib/inference/onboard-probes-curl-harness.tssrc/lib/inference/onboard-probes.test.tssrc/lib/inference/onboard-probes.ts
Signed-off-by: Ho Lim <subhoya@gmail.com>
7f3c1af to
bbbb0c6
Compare
wscurran
added
area: inference
|
✨ Thanks for the PR. This adds a process-local success cache for onboard validation probes to avoid repeated DNS/TLS overhead, fixing the performance issue in #3771. Ready for maintainer review. Related open issues: Related open issues: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
HOLD on exact head d33258c after comparing both open #3771 implementations.
-
The conflicts with current main are semantic, not mechanical. #6461 added OpenRouter extraHeaders and #6488 added timeout calibration. A safe rebase must bind cache entries to an irreversible digest of relevant headers without retaining secret-capable raw values, and must place lookup deliberately so cache hits do not still pay the calibration curl.
-
This cache skips later whole validations, but #3771 targets repeated DNS, TCP, and TLS setup inside a provider probe sequence. Either narrow this PR to a complementary cache follow-up and stop claiming Fixes #3771, or cover the issue acceptance path and provide a maintainer-accepted substitute for the locked #2001 evidence requirement.
-
Add direct regressions for TTL expiry, failed results, empty credentials, validated-false DeepSeek success, extraHeaders separation, model/auth/host-Docker key separation, and the interaction with current timeout calibration. Current tests cover one hit, pinned-address separation, and API-requirement separation only.
-
Refresh the current Type of Change, Quality Gates, and Verification sections, then obtain fresh CI, E2E advice, and an exact-head advisor result after the rebase.
Comparator result: this PR is policy-eligible and therefore closer to ready, but the competing #6419 has stronger #3771 behavior coverage and is currently ineligible because its commits are not GitHub Verified. Neither should merge as-is.
|
Closing this stale implementation rather than keeping it in the merge queue. The v0.0.80 comparator and review feedback call out semantic conflicts with current main (extraHeaders and timeout calibration) and a scope mismatch with #3771's within-sequence DNS/TCP/TLS reuse acceptance path. I'll keep follow-up work focused on smaller, independently mergeable PRs with fresh validation evidence. |
Summary\n- add a process-local success cache for OpenAI-compatible onboard validation probes\n- key cache entries by endpoint, model, credential hash, auth mode, host-docker allowance, and validation requirements\n- avoid repeated curl/DNS/TLS/process setup for identical validations in the same CLI process without caching failures or empty credentials\n\nFixes #3771\n\n## Safety\n- failures are never cached\n- empty credentials are never cached\n- DeepSeek timeout continuations with validated=false are not cached\n- Responses-only validation is not reused for chat-completions-only smoke checks\n- strict chat tool-call requirements only reuse strict chat tool-call successes\n- cache is process-local with a 10 minute TTL\n\n Tests\n- PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npx vitest run --project cli src/lib/inference/onboard-probes.test.ts src/lib/inference/onboard-probes-responses-fallback.test.ts src/lib/adapters/http/probe.test.ts\n- PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npx vitest run --project cli --project integration src/lib/inference/onboard-host-docker-internal.test.ts test/wsl2-probe-timeout.test.ts test/onboard-smoke-verifier.test.ts\n- PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npx vitest run --project package-contract test/package-contract/inference-commonjs.test.ts\n- PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npx tsc -p tsconfig.src.json --noEmit\n- PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npm run build:cli\n- PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npm run typecheck:cli\n- PATH=/Users/holim/.nvm/versions/node/v22.22.2/bin:$PATH npx @biomejs/biome check src/lib/inference/onboard-probes.ts src/lib/inference/onboard-probes.test.ts src/lib/inference/onboard-probes-curl-harness.ts\n- git diff --check
Summary by CodeRabbit
Summary
Signed-off-by: Ho Lim subhoya@gmail.com