From 765fd71aea1aba28f2ff2866ea19059aa1470d10 Mon Sep 17 00:00:00 2001 From: "Bob.Hou" Date: Fri, 7 Aug 2026 19:53:47 -0400 Subject: [PATCH] feat(lib): make MODELS_DEV_SYNC_ENABLED actually control the sync (#9483) Merge-train validated (tip 6ce4effef8). Vitest failures confirmed as base-red (#9679). --- .env.example | 9 ++ docs/reference/ENVIRONMENT.md | 1 + src/lib/modelsDevSync.ts | 37 ++++- tests/unit/modelsDevSync-extended.test.ts | 180 ++++++++++++++++++++++ 4 files changed, 224 insertions(+), 3 deletions(-) diff --git a/.env.example b/.env.example index 15c4d1f18e..ae1efe2f4f 100644 --- a/.env.example +++ b/.env.example @@ -1537,6 +1537,15 @@ APP_LOG_TO_FILE=true # ═══════════════════════════════════════════════════════════════════════════════ # 19. MODEL SYNC (Dev) # ═══════════════════════════════════════════════════════════════════════════════ +# Enable the models.dev capability sync. Default: false (opt-in only). +# Also settable from Dashboard > Settings > AI. This variable wins over that +# setting whenever it is set to anything non-empty, in either direction, so a +# deployment can pin the sync on or off without depending on database state +# surviving a rebuild. Leave it unset to let the dashboard toggle decide. +# On: 1, true, yes or on (any casing). Any other value is off. +# Used by: src/lib/modelsDevSync.ts +# MODELS_DEV_SYNC_ENABLED=false + # Development-time model catalog sync interval in seconds. # Used by: src/lib/modelsDevSync.ts # Default: 86400 (24 hours) diff --git a/docs/reference/ENVIRONMENT.md b/docs/reference/ENVIRONMENT.md index 36d713f721..089dc8b6b2 100644 --- a/docs/reference/ENVIRONMENT.md +++ b/docs/reference/ENVIRONMENT.md @@ -853,6 +853,7 @@ Reverse-engineered session bridge for hyperagent.com (`src/shared/constants/prov | Variable | Default | Source File | Description | | ----------------------------------- | ------------- | ---------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `MODELS_DEV_SYNC_ENABLED` | `false` | `src/lib/modelsDevSync.ts` | Opt-in switch for the models.dev capability sync. Set to anything non-empty it wins over the `modelsDevSyncEnabled` setting (Dashboard > Settings > AI) in either direction, so a deployment can pin the sync on or off without depending on database state surviving a rebuild; unset, it defers to that setting. On for `1`, `true`, `yes` or `on` in any casing; any other value is off. | | `MODELS_DEV_SYNC_INTERVAL` | `86400` (24h) | `src/lib/modelsDevSync.ts` | Development-time model catalog sync interval in seconds. | | `CONTEXT_WINDOW_RECONCILE_INTERVAL` | `86400` (24h) | `src/lib/contextWindowResolver.ts` | Interval (seconds) for the self-correcting context-window reconciler (5004): pins provider-declared windows from `/models` discovery as `auto:discovery` overrides when they diverge from the catalog. Set to `0` to disable. Reuses already-synced data (no new fetch); never overwrites `manual` overrides. | diff --git a/src/lib/modelsDevSync.ts b/src/lib/modelsDevSync.ts index 32a6e180f6..fbcabe353b 100644 --- a/src/lib/modelsDevSync.ts +++ b/src/lib/modelsDevSync.ts @@ -14,7 +14,12 @@ * 3. LiteLLM sync (`pricing_synced` namespace) * 4. Hardcoded defaults (`pricing.ts`) * - * Opt-in via MODELS_DEV_SYNC_ENABLED=true (default: false). + * Opt-in, default off. Enabled either from Dashboard > Settings > AI or with + * MODELS_DEV_SYNC_ENABLED, which wins over that setting whenever it is set to + * anything non-empty, in either direction, so a deployment can pin the sync on + * or off regardless of what is stored. Unset or empty, it defers to the + * setting. On for "1", "true", "yes" or "on" in any casing; every other value + * is off. */ import { getDbInstance } from "./db/core"; @@ -71,6 +76,8 @@ interface SyncResult { const MODELS_DEV_API_URL = "https://models.dev/api.json"; +const TRUE_ENV_VALUES = new Set(["1", "true", "yes", "on"]); + const parsedInterval = parseInt(process.env.MODELS_DEV_SYNC_INTERVAL || "86400", 10); const SYNC_INTERVAL_MS = Number.isFinite(parsedInterval) && parsedInterval > 0 ? parsedInterval * 1000 : 86400 * 1000; @@ -670,8 +677,32 @@ export async function initModelsDevSync(): Promise { const { getSettings } = await import("./localDb"); const settings = await getSettings(); - if (settings.modelsDevSyncEnabled !== true) { - console.log("[MODELS_DEV] Disabled (enable via Settings > AI)"); + // Until now the docblock above advertised MODELS_DEV_SYNC_ENABLED and nothing + // read it: the only control was the stored setting, so an operator following + // that line got silence whichever value they set. This makes the variable real. + // + // An explicit env value decides, in either direction, and only an unset or + // empty one defers to the setting. That means a deployment can pin the sync + // off from its compose file or unit even when a previous operator left the + // dashboard toggle on, which is the case a force-on-only variable cannot + // express and the reason for choosing this shape. + // + // It is worth being plain that this is a third resolution pattern rather than + // a reuse of an existing one, because the two in the tree solve different + // problems: shared/utils/featureFlags.ts::resolveFeatureFlag puts the DB + // override ABOVE the env var, so a deployment cannot override an operator's + // stored choice at all; db/ccDiscoveryAliases.ts::getCcAliasGlobalState reads + // only "1" and "true" and can force a flag ON, letting every other value + // including "false" fall through to the DB. Neither can turn a + // dashboard-enabled switch off from the environment. Following either one + // here would leave the variable unable to do the thing it is being added for. + const envValue = process.env.MODELS_DEV_SYNC_ENABLED?.trim(); + const enabled = envValue + ? TRUE_ENV_VALUES.has(envValue.toLowerCase()) + : settings.modelsDevSyncEnabled === true; + + if (!enabled) { + console.log("[MODELS_DEV] Disabled (enable via Settings > AI or MODELS_DEV_SYNC_ENABLED=true)"); return; } diff --git a/tests/unit/modelsDevSync-extended.test.ts b/tests/unit/modelsDevSync-extended.test.ts index 7b604f2639..258e469f14 100644 --- a/tests/unit/modelsDevSync-extended.test.ts +++ b/tests/unit/modelsDevSync-extended.test.ts @@ -566,3 +566,183 @@ test.describe("modelsDevSync-extended", { concurrency: 1 }, async () => { assert.equal(modelsDev.getSyncStatus().lastSync, null); }); }); + +// MODELS_DEV_SYNC_ENABLED was named in this module's header comment for a long +// time without ever being read, so the only real switch was a row in the +// database. A container rebuilt from a fresh volume therefore came up with the +// sync off no matter what the deployment intended. + +test("MODELS_DEV_SYNC_ENABLED=true starts the sync even with the setting off", async () => { + const previous = process.env.MODELS_DEV_SYNC_ENABLED; + await settingsDb.updateSettings({ + modelsDevSyncEnabled: false, + modelsDevSyncInterval: 15, + }); + + process.env.MODELS_DEV_SYNC_ENABLED = "true"; + const modelsDev = await importFresh("init-env-on"); + mockFetchWith(MOCK_MODELS_DEV_DATA); + try { + await modelsDev.initModelsDevSync(); + assert.equal(modelsDev.getSyncStatus().enabled, true); + // `enabled` alone would also be true for a sync that started and then + // never fetched anything, so pin the fetch actually having run. Assert + // the result, not just await it -- waitFor returns null on timeout, and + // an unasserted timeout is indistinguishable from success. + assert.ok( + await waitFor(() => modelsDev.getSyncStatus().lastSync !== null), + "expected the initial sync to complete and set lastSync" + ); + } finally { + modelsDev.stopPeriodicSync(); + if (previous === undefined) delete process.env.MODELS_DEV_SYNC_ENABLED; + else process.env.MODELS_DEV_SYNC_ENABLED = previous; + } +}); + +test("the stored setting still starts the sync with no env var present", async () => { + const previous = process.env.MODELS_DEV_SYNC_ENABLED; + delete process.env.MODELS_DEV_SYNC_ENABLED; + await settingsDb.updateSettings({ + modelsDevSyncEnabled: true, + modelsDevSyncInterval: 15, + }); + + const modelsDev = await importFresh("init-setting-only"); + mockFetchWith(MOCK_MODELS_DEV_DATA); + try { + // Without this the test would still pass if the variable were set to + // "true", crediting the setting for what the env var did. + assert.equal(process.env.MODELS_DEV_SYNC_ENABLED, undefined); + await modelsDev.initModelsDevSync(); + assert.equal(modelsDev.getSyncStatus().enabled, true); + assert.ok( + await waitFor(() => modelsDev.getSyncStatus().lastSync !== null), + "expected the initial sync to complete and set lastSync" + ); + } finally { + modelsDev.stopPeriodicSync(); + if (previous !== undefined) process.env.MODELS_DEV_SYNC_ENABLED = previous; + } +}); + +test("the usual truthy spellings all start the sync, and nothing else does", async () => { + const previous = process.env.MODELS_DEV_SYNC_ENABLED; + await settingsDb.updateSettings({ + modelsDevSyncEnabled: false, + modelsDevSyncInterval: 15, + }); + + // A compose file or unit file is as likely to carry "1" as "true", so all + // four spellings work, in any casing and with stray whitespace. Everything + // else leaves the sync off rather than guessing at intent. + const cases: Array<[string, boolean]> = [ + ["true", true], + ["TRUE", true], + ["True", true], + [" true ", true], + ["1", true], + ["yes", true], + ["on", true], + ["ON", true], + ["false", false], + ["0", false], + ["no", false], + ["off", false], + ["", false], + ["truthy", false], + ]; + + try { + for (const [index, [value, expected]] of cases.entries()) { + process.env.MODELS_DEV_SYNC_ENABLED = value; + // The label becomes a cache-busting URL suffix, so it has to stay + // URL-safe; the values themselves carry quotes and whitespace. + const modelsDev = await importFresh(`init-env-case-${index}`); + if (expected) mockFetchWith(MOCK_MODELS_DEV_DATA); + await modelsDev.initModelsDevSync(); + assert.equal( + modelsDev.getSyncStatus().enabled, + expected, + `MODELS_DEV_SYNC_ENABLED=${JSON.stringify(value)} should ${expected ? "" : "not "}enable the sync` + ); + if (expected) { + // `enabled` alone would also be true for a sync that started and + // then never fetched anything; pin the fetch actually having run + // for each truthy spelling, not just the first one. + assert.ok( + await waitFor(() => modelsDev.getSyncStatus().lastSync !== null), + `MODELS_DEV_SYNC_ENABLED=${JSON.stringify(value)} should have completed a sync` + ); + } + modelsDev.stopPeriodicSync(); + } + } finally { + if (previous === undefined) delete process.env.MODELS_DEV_SYNC_ENABLED; + else process.env.MODELS_DEV_SYNC_ENABLED = previous; + } +}); + +test("MODELS_DEV_SYNC_ENABLED=false turns the sync off despite a stored setting of true", async () => { + // The direction that costs an operator real time if it is wrong: they put + // the variable in their compose file expecting a master switch, and the + // sync keeps running because the dashboard toggle is still on. An explicit + // env value decides in either direction; only an unset one defers. + const previous = process.env.MODELS_DEV_SYNC_ENABLED; + await settingsDb.updateSettings({ + modelsDevSyncEnabled: true, + modelsDevSyncInterval: 15, + }); + + process.env.MODELS_DEV_SYNC_ENABLED = "false"; + const modelsDev = await importFresh("init-env-false-setting-true"); + mockFetchWith(MOCK_MODELS_DEV_DATA); + try { + await modelsDev.initModelsDevSync(); + assert.equal( + modelsDev.getSyncStatus().enabled, + false, + "MODELS_DEV_SYNC_ENABLED=false should disable the sync even with the setting on" + ); + assert.equal( + modelsDev.getSyncStatus().lastSync, + null, + "a disabled sync must not have fetched anything" + ); + } finally { + modelsDev.stopPeriodicSync(); + if (previous === undefined) delete process.env.MODELS_DEV_SYNC_ENABLED; + else process.env.MODELS_DEV_SYNC_ENABLED = previous; + } +}); + +test("an unset MODELS_DEV_SYNC_ENABLED still defers to a stored setting of true", async () => { + // The counterpart: without this one, the test above would also pass if the + // env var had simply become a hard off switch and the dashboard toggle had + // stopped working entirely. + const previous = process.env.MODELS_DEV_SYNC_ENABLED; + delete process.env.MODELS_DEV_SYNC_ENABLED; + await settingsDb.updateSettings({ + modelsDevSyncEnabled: true, + modelsDevSyncInterval: 15, + }); + + const modelsDev = await importFresh("init-env-unset-setting-true"); + mockFetchWith(MOCK_MODELS_DEV_DATA); + try { + await modelsDev.initModelsDevSync(); + assert.equal( + modelsDev.getSyncStatus().enabled, + true, + "an unset env var should leave the stored setting in charge" + ); + assert.ok( + await waitFor(() => modelsDev.getSyncStatus().lastSync !== null), + "expected the initial sync to complete and set lastSync" + ); + } finally { + modelsDev.stopPeriodicSync(); + if (previous === undefined) delete process.env.MODELS_DEV_SYNC_ENABLED; + else process.env.MODELS_DEV_SYNC_ENABLED = previous; + } +});