fix(virtualmodels): don't gate IaC startup on catalog availability#436
Conversation
Declarative virtual models (VIRTUAL_MODELS / config.yaml) were validated at startup against the model catalog, which loads asynchronously and is empty on a cold cache. ValidateManagedConfig required every managed redirect target to be catalog-supported, so a brand-new deployment with valid IaC virtual models and no warm cache aborted startup with "target model not found". Separate the two concerns the check conflated: - Structural invariants (valid selector, no self-/cross-redirect target) are pure properties of the declaration, so they still fail startup loudly (validateRedirectStructure). - Catalog availability is runtime state, already handled by skipping unavailable targets at resolve time, so it is no longer a startup gate. The admin write path keeps it (firstUnsupportedTarget) since it runs against a warm catalog and an unknown target there is a caller mistake. A managed redirect with an unknown target now boots and is simply unavailable until the catalog provides it, consistent with the resolve-time skip and the background refresh that already tolerates a transient catalog gap. Tests: cold-catalog startup no longer aborts and resolves once warm; admin upsert still rejects an unsupported target; structural invalids still abort. Adds the standalone tests/e2e/test-iac-virtualmodels.sh IaC harness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cache analytics Cover the features shipped since v0.1.44 that the matrix had no behavioral coverage of (only the single-target alias path via S13/S24/S31): - S118-S125 load-balanced virtual models (#433): round-robin, weighted, cost, rename via old_source, plus negatives (unknown strategy, unknown target, rename of a non-existent source). - S126-S128 token throughput (#434): window shape across granularities, negative granularity handling, and live-traffic reflection. - S129-S132 cache analytics (#428): cache_mode on the usage summary, cache overview availability gating, and locally-cached token accounting from an exact-cache hit. Each scenario is self-contained ($QA_SUFFIX-scoped, cleans up after itself) and validated through run-release-e2e.sh. IaC virtual-model behavior needs gateways booted with custom config, so it lives in the standalone tests/e2e/test-iac-virtualmodels.sh harness instead of this running-stack matrix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughManaged redirect validation is split into two phases: structural checks (selector validity, self-targeting, cross-redirect cycles) run at startup via ChangesCold-catalog startup tolerance for managed redirects
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 5/5The changes are narrowly scoped to virtual-model startup validation while preserving strict admin write validation. No code issues were identified, and the test coverage described exercises the cold-catalog startup case, strict admin behavior, structural validation, and E2E IaC flows.
 T-Rex LogsWhat T-Rex did
Reviews (1): Last reviewed commit: "test(e2e): add release scenarios for loa..." | Re-trigger Greptile |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@internal/virtualmodels/config_overlay_test.go`:
- Around line 205-206: The Upsert unsupported-target test only checks for any
validation error, so it can pass for unrelated failures; tighten the assertion
in the config_overlay_test.go case around the Upsert path to verify the specific
“target model not found” failure or exact validation subtype/message. Update the
test to inspect the returned error from Upsert and assert the concrete
admin-write rejection behavior rather than only IsValidationError(err), using
the existing Upsert and IsValidationError symbols to locate the check.
In `@tests/e2e/test-iac-virtualmodels.sh`:
- Around line 46-47: The port-claiming logic in the test script is too
aggressive because the lsof loop kills any existing listener on IAC_PORT and
IAC_NEG_PORT, which can terminate unrelated processes. Update the port setup
around the existing lsof/kill loops to fail fast with a clear “port in use”
message or switch to a free ephemeral port for the run, and apply the same
change wherever the same pattern appears later in the script.
- Around line 54-59: The readiness check in start_gw can still succeed even if
the model registry never initializes, because the second polling loop only waits
and then always returns success. Update the start_gw flow to track whether grep
-q 'model registry initialized' in server.log actually matched, and return
failure when it never does; use the existing healthy flag and the polling block
around the model registry initialized check to gate the final return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db66e35f-60b4-4fb4-9e74-b1483d36c4ca
📒 Files selected for processing (4)
internal/virtualmodels/config_overlay_test.gointernal/virtualmodels/service.gotests/e2e/release-e2e-scenarios.mdtests/e2e/test-iac-virtualmodels.sh
| if err == nil || !IsValidationError(err) { | ||
| t.Fatalf("Upsert(unsupported target) error = %v, want validation rejection", err) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the specific unsupported-target failure.
Line 205 currently passes on any validation error, so this test would still go green if Upsert started failing for an unrelated reason. Please also assert the concrete target model not found path (or the exact validation subtype/message) so the regression really pins the new admin-write behavior. As per coding guidelines, **/*_test.go: Add or update tests for behavior changes, and ensure tests cover error handling.
🤖 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 `@internal/virtualmodels/config_overlay_test.go` around lines 205 - 206, The
Upsert unsupported-target test only checks for any validation error, so it can
pass for unrelated failures; tighten the assertion in the config_overlay_test.go
case around the Upsert path to verify the specific “target model not found”
failure or exact validation subtype/message. Update the test to inspect the
returned error from Upsert and assert the concrete admin-write rejection
behavior rather than only IsValidationError(err), using the existing Upsert and
IsValidationError symbols to locate the check.
Source: Coding guidelines
| for sp in $(lsof -nP -t -iTCP:$PORT -sTCP:LISTEN 2>/dev/null); do kill "$sp" 2>/dev/null; done | ||
| sleep 1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't kill unrelated listeners to claim the test ports.
These loops send SIGTERM to whatever is already bound to IAC_PORT/IAC_NEG_PORT, which can take down another local service or a parallel CI job. Prefer failing fast with a clear “port in use” error or selecting a free ephemeral port for this run instead.
Also applies to: 126-127
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 46-46: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@tests/e2e/test-iac-virtualmodels.sh` around lines 46 - 47, The port-claiming
logic in the test script is too aggressive because the lsof loop kills any
existing listener on IAC_PORT and IAC_NEG_PORT, which can terminate unrelated
processes. Update the port setup around the existing lsof/kill loops to fail
fast with a clear “port in use” message or switch to a free ephemeral port for
the run, and apply the same change wherever the same pattern appears later in
the script.
| local healthy=1 | ||
| for _ in $(seq 1 30); do curl -fsS "$B/health" >/dev/null 2>&1 && { healthy=0; break; }; kill -0 "$(cat "$PIDF")" 2>/dev/null || break; sleep 1; done | ||
| [ "$healthy" = 0 ] || return 1 | ||
| for _ in $(seq 1 30); do grep -q 'model registry initialized' "$WORK/server.log" && break; sleep 1; done | ||
| sleep 1 | ||
| return 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail start_gw when the registry never becomes ready.
The second poll loop never checks whether model registry initialized was actually observed, so start_gw can return success with an unwarmed or stalled catalog. That makes the later resolve/chat assertions nondeterministic.
Suggested fix
local healthy=1
+ local registry_ready=1
for _ in $(seq 1 30); do curl -fsS "$B/health" >/dev/null 2>&1 && { healthy=0; break; }; kill -0 "$(cat "$PIDF")" 2>/dev/null || break; sleep 1; done
[ "$healthy" = 0 ] || return 1
- for _ in $(seq 1 30); do grep -q 'model registry initialized' "$WORK/server.log" && break; sleep 1; done
+ for _ in $(seq 1 30); do
+ grep -q 'model registry initialized' "$WORK/server.log" && { registry_ready=0; break; }
+ kill -0 "$(cat "$PIDF")" 2>/dev/null || return 1
+ sleep 1
+ done
+ [ "$registry_ready" = 0 ] || return 1
sleep 1
return 0
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local healthy=1 | |
| for _ in $(seq 1 30); do curl -fsS "$B/health" >/dev/null 2>&1 && { healthy=0; break; }; kill -0 "$(cat "$PIDF")" 2>/dev/null || break; sleep 1; done | |
| [ "$healthy" = 0 ] || return 1 | |
| for _ in $(seq 1 30); do grep -q 'model registry initialized' "$WORK/server.log" && break; sleep 1; done | |
| sleep 1 | |
| return 0 | |
| local healthy=1 | |
| local registry_ready=1 | |
| for _ in $(seq 1 30); do curl -fsS "$B/health" >/dev/null 2>&1 && { healthy=0; break; }; kill -0 "$(cat "$PIDF")" 2>/dev/null || break; sleep 1; done | |
| [ "$healthy" = 0 ] || return 1 | |
| for _ in $(seq 1 30); do | |
| grep -q 'model registry initialized' "$WORK/server.log" && { registry_ready=0; break; } | |
| kill -0 "$(cat "$PIDF")" 2>/dev/null || return 1 | |
| sleep 1 | |
| done | |
| [ "$registry_ready" = 0 ] || return 1 | |
| sleep 1 | |
| return 0 |
🤖 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 `@tests/e2e/test-iac-virtualmodels.sh` around lines 54 - 59, The readiness
check in start_gw can still succeed even if the model registry never
initializes, because the second polling loop only waits and then always returns
success. Update the start_gw flow to track whether grep -q 'model registry
initialized' in server.log actually matched, and return failure when it never
does; use the existing healthy flag and the polling block around the model
registry initialized check to gate the final return.
2 participants
Problem (F3)
Declarative virtual models (
VIRTUAL_MODELSenv /config.yaml virtual_models:) were validated at startup against the model catalog, which loads asynchronously (providers.InitializeAsyncseeds from cache synchronously, then fetches/modelsin a background goroutine).ValidateManagedConfigrequired every managed redirect target to becatalog.Supports(...), so on a cold catalog (cached_models:0— no warm cache, or a never-populated Redis on first boot) even a valid target was rejected:A brand-new deployment that declares valid IaC virtual models without a warm cache could not boot — a chicken-and-egg bootstrap. There was no provider-config workaround (
allowlist+ explicit model lists is still cold at validation).Fix
Separate the two concerns the check conflated:
validateRedirectStructure.firstUnsupportedTarget), since it runs against a warm catalog where an unknown target is a real caller mistake.A managed redirect with an unknown target now boots and is simply unavailable until the catalog provides it — consistent with how store redirects and resolve-time skipping already behave. This keeps every guarantee the docs actually make ("An invalid declaration — unknown strategy, missing or self-referential target — fails startup"; all structural).
User-visible impact
PUT /admin/virtual-modelswith an unknown target still returns400(unchanged).Tests
internal/virtualmodels: newTestService_ManagedRedirectToleratesColdCatalogAtStartup(regression) andTestService_UpsertRejectsUnsupportedTarget(admin path stays strict); removed the now-invalid "unknown target aborts startup" config case; self-/cross-target cases unchanged.go test/-racegreen.tests/e2e/test-iac-virtualmodels.sh(new standalone IaC harness): boots on a cold catalog, managed read-only, store override (non-destructive), env-over-YAML merge, structural-invalid aborts, unknown target tolerated — 15/15.tests/e2e/release-e2e-scenarios.md: adds S118–S132 covering load-balanced virtual models (feat(virtualmodels): load-balanced virtual models (round-robin + cost) with IaC config #433), token throughput (feat(dashboard): live token throughput chart, prompt cache gauge, and overview refinements #434), and cache analytics (feat(dashboard): add token cache meter and overview refinements #428) — previously uncovered behaviorally. Validated viarun-release-e2e.sh(15/15), and the full existing matrix S01–S117 still passes.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes