diff --git a/crates/aisix-core/src/filesource/tests.rs b/crates/aisix-core/src/filesource/tests.rs index acb7f2b6..e3037f5d 100644 --- a/crates/aisix-core/src/filesource/tests.rs +++ b/crates/aisix-core/src/filesource/tests.rs @@ -277,6 +277,38 @@ rate_limit_policies: assert!(errors[0].contains("no-such-model"), "{errors:?}"); } +#[test] +fn dead_knob_on_a_group_is_a_declarative_load_error() { + // The strict write path (declarative resources file) rejects a knob + // the kind never resolves — a silently-dead `cost` on a Model Group + // was the #962 class. Stored etcd rows load leniently instead (the + // loader strips + reports); a file the operator edits fails fast + // with the field named. + let file = r#" +_format_version: "1" + +provider_keys: + - display_name: pk + provider: openai + api_key: sk-x +models: + - display_name: gpt-4o + provider: openai + model_name: gpt-4o + provider_key: pk + - display_name: balanced + routing: + targets: + - model: gpt-4o + cost: + input_per_1k: 0.5 + output_per_1k: 1.5 +"#; + let errors = errors_of(load(file, &env_of(&[]))); + assert_eq!(errors.len(), 1, "{errors:?}"); + assert!(errors[0].contains("balanced"), "{errors:?}"); +} + #[test] fn ids_are_deterministic_across_two_loads() { let env = full_env(); diff --git a/crates/aisix-core/src/models/model.rs b/crates/aisix-core/src/models/model.rs index 55854393..09f3a2f6 100644 --- a/crates/aisix-core/src/models/model.rs +++ b/crates/aisix-core/src/models/model.rs @@ -320,6 +320,40 @@ impl Model { self.model_name.as_deref() } + /// Strip the per-kind DEAD knobs from a loaded document, returning + /// the stripped field names for the loader's partially-compatible + /// report. The strict write path rejects these shapes outright + /// ([`model_one_of_strict`]); rows stored before that keep loading — + /// minus the field, so no code path can half-honor a knob the shape + /// never resolved. + pub fn strip_kind_inapplicable(&mut self) -> Vec<&'static str> { + let mut stripped = Vec::new(); + if !(self.is_routing() || self.is_ensemble() || self.is_semantic()) { + return stripped; + } + if self.auto_prompt_caching.take().is_some() { + stripped.push("auto_prompt_caching"); + } + if self.cost.take().is_some() { + stripped.push("cost"); + } + if (self.is_routing() || self.is_ensemble()) && self.retries.take().is_some() { + stripped.push("retries"); + } + // Pushed in lexicographic order so a pure-strip row's field list + // is already sorted (the loader's merge path re-sorts a combined + // unknown+inapplicable list, but a strip-only row bypasses that). + if self.is_ensemble() { + if self.stream_timeout.take().is_some() { + stripped.push("stream_timeout"); + } + if self.timeout.take().is_some() { + stripped.push("timeout"); + } + } + stripped + } + /// This resource's own non-streaming deadline, as one level of the /// model → group → `upstream.timeout_ms` resolution performed by the /// proxy's `effective_timeouts`. Tri-state: `None` defers to the next @@ -376,6 +410,57 @@ impl Model { /// `oneOf` into the generated schema, so the published schema and the /// runtime validator share this single definition. pub fn model_one_of() -> Value { + model_one_of_variant(false) +} + +/// The write-path variant of [`model_one_of`]: additionally forbids the +/// per-kind DEAD knobs — fields the runtime never reads on that shape, +/// which the lenient read path keeps tolerating (loaded rows strip them +/// with a partially-compatible warning instead of dropping the row; see +/// [`Model::strip_kind_inapplicable`]). Kind policy (project decision): +/// generic call knobs (`timeout`/`stream_timeout`/`retries`) resolve +/// member → group → deployment default wherever a group slot exists; +/// model-specific knobs (`auto_prompt_caching`, `cost`) are direct-only. +pub fn model_one_of_strict() -> Value { + model_one_of_variant(true) +} + +fn model_one_of_variant(strict: bool) -> Value { + let extend = |base: &mut Value, extra: &[&str]| { + let list = base["not"]["anyOf"].as_array_mut().expect("anyOf array"); + for field in extra { + list.push(json!({ "required": [field] })); + } + }; + let mut variants = model_one_of_base(); + if strict { + let arr = variants.as_array_mut().expect("oneOf array"); + // routing: the group slot for timeouts is the top-level pair + // (api7/aisix#844); retries' group slot is `routing.retries`, so a + // top-level value is dead — as are the model-specific knobs. + extend(&mut arr[0], &["retries", "auto_prompt_caching", "cost"]); + // direct (arr[1]): every knob is live. + // ensemble: sub-calls resolve member-level knobs only; the + // parent-level deadline is `ensemble.timeout_ms`. + extend( + &mut arr[2], + &[ + "timeout", + "stream_timeout", + "retries", + "auto_prompt_caching", + "cost", + ], + ); + // semantic: top-level timeout/stream_timeout/retries ARE the group + // slots (no routing block to carry them); the model-specific knobs + // stay direct-only. + extend(&mut arr[3], &["auto_prompt_caching", "cost"]); + } + variants +} + +fn model_one_of_base() -> Value { json!([ { "required": ["routing"], @@ -527,6 +612,71 @@ mod tests { assert_eq!(m.display_name, "x"); } + #[test] + fn strip_kind_inapplicable_per_kind() { + let load = |v: serde_json::Value| -> Model { serde_json::from_value(v).unwrap() }; + // Routing parent: model-specific knobs + top-level retries strip; + // the group-level timeout pair stays (it IS the group slot). + let mut group = load(serde_json::json!({ + "display_name": "g", + "routing": {"targets": [{"model": "m"}]}, + "retries": 2, + "timeout": 1000, + "cost": {"input_per_1k": 0.0, "output_per_1k": 0.0}, + "auto_prompt_caching": {"enabled": true} + })); + let mut stripped = group.strip_kind_inapplicable(); + stripped.sort_unstable(); + assert_eq!(stripped, ["auto_prompt_caching", "cost", "retries"]); + assert!(group.retries.is_none() && group.cost.is_none()); + assert_eq!(group.timeout, Some(1000)); + // Semantic parent: timeout/retries are the group slots and stay. + let mut sem = load(serde_json::json!({ + "display_name": "s", + "semantic": { + "embedding_model": "e", + "routes": [{"name": "r", "target": "t", "examples": ["x"]}], + "default": "d", + "match": {"threshold": 0.5} + }, + "retries": 2, + "timeout": 1000, + "cost": {"input_per_1k": 0.0, "output_per_1k": 0.0} + })); + assert_eq!(sem.strip_kind_inapplicable(), ["cost"]); + assert_eq!(sem.retries, Some(2)); + assert_eq!(sem.timeout, Some(1000)); + // Direct: nothing strips. + let mut direct = load(serde_json::json!({ + "display_name": "m", + "provider": "openai", + "model_name": "gpt-4o", + "provider_key_id": "pk-1", + "retries": 2, + "cost": {"input_per_1k": 0.0, "output_per_1k": 0.0} + })); + assert!(direct.strip_kind_inapplicable().is_empty()); + assert_eq!(direct.retries, Some(2)); + // Ensemble parent: the whole generic set strips (its own + // deadline knob is `ensemble.timeout_ms`). + let mut ens = load(serde_json::json!({ + "display_name": "e", + "ensemble": {"panel": [{"model": "m"}], "judge": {"model": "j"}}, + "timeout": 1000, + "stream_timeout": 500, + "retries": 1, + "cost": {"input_per_1k": 0.0, "output_per_1k": 0.0} + })); + // Asserted WITHOUT a pre-sort: the strip output is already + // lexicographic (a pure-strip loader row keeps the fields + // "sorted" per PartialCompatRow's contract). + let ens_stripped = ens.strip_kind_inapplicable(); + assert_eq!( + ens_stripped, + ["cost", "retries", "stream_timeout", "timeout"] + ); + } + #[test] fn ip_allowed_matrix() { fn model_with(cidrs: Option>) -> Model { diff --git a/crates/aisix-core/src/models/schema.rs b/crates/aisix-core/src/models/schema.rs index d97d6634..a72fcf59 100644 --- a/crates/aisix-core/src/models/schema.rs +++ b/crates/aisix-core/src/models/schema.rs @@ -88,7 +88,7 @@ fn closes_on_write(resource: &str) -> bool { /// `dump-schema` binary build from. pub fn resource_root_schema(resource: &str, strict: bool) -> Value { let mut schema = match resource { - "model" => model_root_schema(), + "model" => model_root_schema(strict), "api_key" => apikey_root_schema(), "provider_key" => provider_key_root_schema(), "guardrail" => guardrail_root_schema(), @@ -329,14 +329,23 @@ fn struct_root_schema(nullable_options: bool) -> Value /// Canonical JSON Schema for the `model` resource: the [`Model`] struct plus /// the one cross-field invariant `schemars` cannot express /// ([`super::model::model_one_of`] — the direct/routing/ensemble XOR). +/// `strict` picks the write-path variant that additionally forbids the +/// per-kind dead knobs ([`super::model::model_one_of_strict`]); the +/// lenient read path keeps the base XOR so stored rows load (and strip) +/// rather than drop. /// /// [`Model`]: crate::models::Model -pub fn model_root_schema() -> Value { +pub fn model_root_schema(strict: bool) -> Value { let mut schema = struct_root_schema::(false); + let one_of = if strict { + super::model::model_one_of_strict() + } else { + super::model::model_one_of() + }; schema .as_object_mut() .expect("model root schema is a JSON object") - .insert("oneOf".to_string(), super::model::model_one_of()); + .insert("oneOf".to_string(), one_of); // `OnEmbeddingFailure` is `#[serde(untagged)]` with an object variant // (`{ "target": … }`): serde buffers untagged content and silently // swallows unknown fields inside it, invisible to both the write diff --git a/crates/aisix-core/tests/model_schema_characterization.rs b/crates/aisix-core/tests/model_schema_characterization.rs index 8da231a1..469553a0 100644 --- a/crates/aisix-core/tests/model_schema_characterization.rs +++ b/crates/aisix-core/tests/model_schema_characterization.rs @@ -111,7 +111,7 @@ fn accept_routing_minimal() { #[test] fn accept_routing_full() { accept( - "routing with all knobs + shared optionals", + "routing with all LIVE knobs + shared optionals", json!({ "display_name": "r", "routing": { @@ -124,8 +124,95 @@ fn accept_routing_full() { }, "timeout": 1000, "rate_limit": {"rpm": 10}, - "allowed_cidrs": ["10.0.0.0/8"], - "cost": {"input_per_1k": 0.0, "output_per_1k": 0.0} + "allowed_cidrs": ["10.0.0.0/8"] + }), + ); +} + +#[test] +fn strict_rejects_dead_knobs_per_kind_but_lenient_loads_them() { + // Kind policy (model-kind audit): generic call knobs resolve + // member → group → deployment default wherever a group slot exists; + // model-specific knobs are direct-only. A knob the shape never + // resolves is REJECTED at write time — accepted-but-unread config is + // the #962 class — while the lenient read path keeps loading stored + // rows (the loader strips the field and reports partial compat). + let cases = [ + ( + "routing + top-level retries", + json!({ + "display_name": "r", + "routing": {"targets": [{"model": "m"}]}, + "retries": 2 + }), + ), + ( + "routing + cost", + json!({ + "display_name": "r", + "routing": {"targets": [{"model": "m"}]}, + "cost": {"input_per_1k": 0.0, "output_per_1k": 0.0} + }), + ), + ( + "routing + auto_prompt_caching", + json!({ + "display_name": "r", + "routing": {"targets": [{"model": "m"}]}, + "auto_prompt_caching": {"enabled": true} + }), + ), + ( + "ensemble + timeout", + json!({ + "display_name": "e", + "ensemble": {"panel": [{"model": "m"}], "judge": {"model": "j"}}, + "timeout": 1000 + }), + ), + ( + "ensemble + retries", + json!({ + "display_name": "e", + "ensemble": {"panel": [{"model": "m"}], "judge": {"model": "j"}}, + "retries": 1 + }), + ), + ( + "semantic + cost", + json!({ + "display_name": "s", + "semantic": { + "embedding_model": "emb", + "routes": [{"name": "r", "target": "t", "examples": ["x"]}], + "default": "d", + "match": {"threshold": 0.5} + }, + "cost": {"input_per_1k": 0.0, "output_per_1k": 0.0} + }), + ), + ]; + for (label, value) in cases { + reject(label, value.clone()); + if let Err(e) = aisix_core::models::validate_model_lenient(&value) { + panic!("expected lenient ACCEPT for `{label}`, got reject: {e}"); + } + } + // The semantic group slots stay LIVE on the strict path — the write + // gate must not outlaw the knobs the runtime resolves. + accept( + "semantic + top-level timeout/retries (the group slots)", + json!({ + "display_name": "s", + "semantic": { + "embedding_model": "emb", + "routes": [{"name": "r", "target": "t", "examples": ["x"]}], + "default": "d", + "match": {"threshold": 0.5} + }, + "timeout": 1000, + "stream_timeout": 1000, + "retries": 2 }), ); } diff --git a/crates/aisix-etcd/src/loader.rs b/crates/aisix-etcd/src/loader.rs index 5cac13b5..7c6a4777 100644 --- a/crates/aisix-etcd/src/loader.rs +++ b/crates/aisix-etcd/src/loader.rs @@ -238,7 +238,8 @@ pub fn build_snapshot(prefix: &str, entries: &[RawEntry]) -> (AisixSnapshot, Bui match parsed.kind { "models" => { - if let Some(entry) = validate_and_parse::( + let row_kind = parsed.kind; + if let Some(mut entry) = validate_and_parse::( &raw.key, raw.revision, parsed, @@ -246,6 +247,26 @@ pub fn build_snapshot(prefix: &str, entries: &[RawEntry]) -> (AisixSnapshot, Bui validate_model_lenient, &mut stats, ) { + // Known-but-kind-inapplicable knobs load stripped and + // report through the same partially-compatible channel + // as unknown fields — the strict write path rejects + // these shapes, stored rows must keep loading. + let stripped = entry.value.strip_kind_inapplicable(); + if !stripped.is_empty() { + let fields: Vec = stripped + .iter() + .map(|f| format!("inapplicable:{f}")) + .collect(); + warn_partial_compat_deduped(&raw.key, row_kind, &fields); + // MERGE into this key's existing partial-compat + // row rather than pushing a second one: a row can + // carry BOTH unknown fields (pushed inside + // validate_and_parse) and inapplicable knobs, and + // the supervisor keys its retained report by etcd + // key — two rows for one key would drop one half + // (resync overwrites, watch takes the first). + merge_partial_compat_fields(&mut stats, &raw.key, row_kind, fields); + } snapshot.models.insert(entry); } } @@ -557,6 +578,27 @@ fn normalize_ignored_path(path: &str) -> String { /// combinations keep logging (never silently dropped) but are no longer /// remembered, so a pathological fleet re-logs on each resync instead /// of growing memory without bound. +/// Add `fields` to the partial-compat row already recorded for `key` +/// this build, or start one if none exists. Exactly one row per etcd key +/// so the supervisor's key-addressed retained report never drops a half +/// (a row with both unknown AND inapplicable fields). `rfind` because +/// `validate_and_parse` pushes the unknown-fields row for this same key +/// moments earlier — it is at or near the tail. +fn merge_partial_compat_fields(stats: &mut BuildStats, key: &str, kind: &str, fields: Vec) { + match stats.partial_rows.iter_mut().rfind(|r| r.key == key) { + Some(row) => { + row.fields.extend(fields); + row.fields.sort_unstable(); + row.fields.dedup(); + } + None => stats.partial_rows.push(PartialCompatRow { + key: key.to_string(), + kind: kind.to_string(), + fields, + }), + } +} + fn warn_partial_compat_deduped(key: &str, kind: &str, fields: &[String]) { use std::collections::HashSet; use std::sync::{Mutex, OnceLock}; @@ -986,6 +1028,63 @@ mod tests { ); } + #[test] + fn model_dead_knob_strips_and_reports_inapplicable() { + // A routing group with a top-level `retries` (dead — the group + // slot is routing.retries) loads with the field stripped and + // reports it via the partial-compat channel. + let entries = vec![raw( + "/aisix/models/m-dead", + br#"{ + "display_name": "grp", + "routing": {"targets": [{"model": "m"}]}, + "retries": 3 + }"#, + 1, + )]; + let (snap, stats) = build_snapshot("/aisix", &entries); + assert_eq!(stats.accepted, 1, "rejections: {:?}", stats.rejections); + let m = snap.models.get_by_name("grp").expect("row loaded"); + assert!(m.value.retries.is_none(), "dead knob stripped from struct"); + assert_eq!( + stats.partial_rows[0].fields, + vec!["inapplicable:retries".to_string()] + ); + } + + #[test] + fn model_unknown_and_inapplicable_fields_merge_into_one_row() { + // A row carrying BOTH an unknown field (future CP) and a dead + // knob must report BOTH under ONE key — two rows would let the + // supervisor's key-addressed retained map drop a half (M1). + let entries = vec![raw( + "/aisix/models/m-both", + br#"{ + "display_name": "grp2", + "routing": {"targets": [{"model": "m"}]}, + "retries": 3, + "zz_future_field": true + }"#, + 1, + )]; + let (_snap, stats) = build_snapshot("/aisix", &entries); + assert_eq!(stats.accepted, 1, "rejections: {:?}", stats.rejections); + let rows: Vec<_> = stats + .partial_rows + .iter() + .filter(|r| r.key == "/aisix/models/m-both") + .collect(); + assert_eq!(rows.len(), 1, "one row per key: {:?}", stats.partial_rows); + assert_eq!( + rows[0].fields, + vec![ + "inapplicable:retries".to_string(), + "zz_future_field".to_string() + ], + "both signals present and sorted" + ); + } + #[test] fn per_row_unknown_field_report_is_capped_with_a_visible_sentinel() { // One document can carry arbitrarily many unknown fields with diff --git a/schemas/resources/model.schema.json b/schemas/resources/model.schema.json index 219de767..805703e0 100644 --- a/schemas/resources/model.schema.json +++ b/schemas/resources/model.schema.json @@ -721,6 +721,21 @@ "required": [ "embedding" ] + }, + { + "required": [ + "retries" + ] + }, + { + "required": [ + "auto_prompt_caching" + ] + }, + { + "required": [ + "cost" + ] } ] }, @@ -796,6 +811,31 @@ "required": [ "embedding" ] + }, + { + "required": [ + "timeout" + ] + }, + { + "required": [ + "stream_timeout" + ] + }, + { + "required": [ + "retries" + ] + }, + { + "required": [ + "auto_prompt_caching" + ] + }, + { + "required": [ + "cost" + ] } ] }, @@ -845,6 +885,16 @@ "required": [ "embedding" ] + }, + { + "required": [ + "auto_prompt_caching" + ] + }, + { + "required": [ + "cost" + ] } ] }, diff --git a/tests/e2e/src/cases/model-kind-dead-knobs-e2e.test.ts b/tests/e2e/src/cases/model-kind-dead-knobs-e2e.test.ts new file mode 100644 index 00000000..4abb11ab --- /dev/null +++ b/tests/e2e/src/cases/model-kind-dead-knobs-e2e.test.ts @@ -0,0 +1,131 @@ +import { createHash } from "node:crypto"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { + EtcdClient, + SeedClient, + spawnApp, + startOpenAiUpstream, + waitConfigPropagation, + type OpenAiUpstream, + type SpawnedApp, +} from "../harness/index.js"; + +// E2E: per-kind DEAD knobs (model-kind audit / schema convergence). +// Generic call knobs resolve member → group → deployment default where a +// group slot exists; model-specific knobs are direct-only. Two halves: +// +// READ (lenient): a stored row carrying such knobs still LOADS — +// the loader strips the field and reports it through the +// partially-compatible channel on `GET /status/config`, instead of +// dropping the whole row (which would take a working group out of +// service on upgrade). The WRITE half (strict) has no e2e surface +// here — the DP admin API is read-only for resources — and is pinned +// at the validator level (model_schema_characterization) and the +// declarative-file loader (filesource tests). + +const CALLER_PLAINTEXT = "sk-dead-knobs-caller"; +const CALLER_KEY_HASH = createHash("sha256") + .update(CALLER_PLAINTEXT) + .digest("hex"); + +function chatBody(content: string) { + return { + id: `cmpl-${content}`, + object: "chat.completion", + created: 0, + model: "gpt-4o-mini", + choices: [ + { index: 0, message: { role: "assistant", content }, finish_reason: "stop" }, + ], + usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 }, + }; +} + +describe("model kind dead knobs e2e", () => { + let app: SpawnedApp | undefined; + let seed: SeedClient | undefined; + let etcdReachable = false; + const upstreams: OpenAiUpstream[] = []; + + beforeAll(async () => { + const etcd = new EtcdClient(); + etcdReachable = await etcd.ping(); + if (!etcdReachable) return; + + app = await spawnApp(); + seed = new SeedClient(etcd, app.etcdPrefix); + await seed.createApiKey({ key_hash: CALLER_KEY_HASH, allowed_models: ["*"] }); + + const upstream = await startOpenAiUpstream({ nonStreamBody: chatBody("served-dk") }); + upstreams.push(upstream); + const pk = await seed.createProviderKey({ + display_name: "dk-pk", + secret: "sk-mock", + api_base: `${upstream.baseUrl}/v1`, + }); + await seed.createModel({ + display_name: "dk-member", + provider: "openai", + model_name: "gpt-4o-mini", + provider_key_id: pk.id, + }); + // A STORED group row carrying dead knobs — written by an older + // build / directly to etcd. It must keep serving, minus the knobs. + await seed.createModel({ + display_name: "dk-group", + routing: { strategy: "failover", targets: [{ model: "dk-member" }] }, + retries: 3, + cost: { input_per_1k: 0.5, output_per_1k: 1.5 }, + }); + + await waitConfigPropagation(async () => { + try { + const res = await fetch(`${app!.proxyUrl}/v1/chat/completions`, { + method: "POST", + headers: { + authorization: `Bearer ${CALLER_PLAINTEXT}`, + "content-type": "application/json", + }, + body: JSON.stringify({ + model: "dk-group", + messages: [{ role: "user", content: "hi" }], + }), + }); + await res.text(); + return res.status === 200; + } catch { + return false; + } + }); + }); + + afterAll(async () => { + await app?.exit(); + await Promise.all(upstreams.map((u) => u.close())); + }); + + test("a stored group with dead knobs keeps serving and reports them partially compatible", async (ctx) => { + if (!etcdReachable || !app) { + ctx.skip(); + return; + } + // The row served during readiness — the group did NOT get dropped + // on load. The dead knobs surface on the status report instead. + const res = await fetch(`${app.metricsUrl}/status/config`); + expect(res.status).toBe(200); + const cfg = (await res.json()) as { + partially_compatible: Array<{ resource_kind: string; field: string; count: number }>; + }; + expect(cfg.partially_compatible).toContainEqual({ + resource_kind: "models", + field: "inapplicable:cost", + count: 1, + }); + expect(cfg.partially_compatible).toContainEqual({ + resource_kind: "models", + field: "inapplicable:retries", + count: 1, + }); + }); + +});