Skip to content

fix(models): honor MODELS_DEV_SYNC_ENABLED=0 over dashboard settings - #10299

Open
RaviTharuma wants to merge 8 commits into
diegosouzapw:release/v3.8.50from
RaviTharuma:fix/models-dev-sync-env-killswitch
Open

fix(models): honor MODELS_DEV_SYNC_ENABLED=0 over dashboard settings#10299
RaviTharuma wants to merge 8 commits into
diegosouzapw:release/v3.8.50from
RaviTharuma:fix/models-dev-sync-env-killswitch

Conversation

@RaviTharuma

@RaviTharuma RaviTharuma commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The comment at the top of src/lib/modelsDevSync.ts already said opt-in via MODELS_DEV_SYNC_ENABLED=true. Nothing in the file read that env var. Periodic sync only looked at settings.modelsDevSyncEnabled in sqlite.
  • When GET /v1/models or long-context compression pins the Node event loop, the dashboard is the same process. GET /healthz and GET /dashboard both stop answering (0 bytes / client timeout). Operators cannot open Settings → AI to turn models.dev sync off. That is a catch-22 (fix(ops): /healthz (and other HTTP) times out while event loop is busy (k8s probes flap) #10052, dashboard freeze).
  • This PR makes MODELS_DEV_SYNC_ENABLED=0|false|off|no a hard kill switch that wins over the DB. Init skips periodic sync. getModelsDevPricing() returns {} without the SQL + JSON.parse scan so leftover models_dev_pricing rows cannot re-wedge the loop. Unset still honors the dashboard setting. 1|true|on|yes forces sync on.
  • Documented in docs/reference/ENVIRONMENT.md. Unit test covers env=0 vs settings=true.

Related Issues

Validation

  • Docs-only change (no production code) — N/A
  • Production-code changes include a new or updated automated test in this PR
  • SonarQube PR analysis is green or any remaining issues are explicitly documented below

Tests Added Or Updated

  • tests/unit/modelsDevSync-extended.test.ts
    • MODELS_DEV_SYNC_ENABLED=0 kills init and getModelsDevPricing even when settings are on
    • existing periodic-sync / init settings tests still pass
node --import tsx --test tests/unit/modelsDevSync-extended.test.ts
# tests 15, pass 15

Coverage Notes

  • New branch: env kill switch in initModelsDevSync + getModelsDevPricing + applyModelsDevSyncSection.

Reviewer Notes

Test plan

  • Unit: env=0 + settings enabled → init does not start timer; getModelsDevPricing() is {} even with rows saved
  • Unit: existing init/settings/timer tests still pass
  • Manual: MODELS_DEV_SYNC_ENABLED=0 on a box with modelsDevSyncEnabled=true in sqlite → logs [MODELS_DEV] Disabled (MODELS_DEV_SYNC_ENABLED=0), /healthz stays ms-level

diegosouzapw and others added 6 commits August 8, 2026 00:08
…ouzapw#189, diegosouzapw#190)

Bumps: nanoid ^3.3.17 (was transitive, now overridden), dompurify ^3.4.13
(with monaco-editor scoped override). Closes Dependabot diegosouzapw#189, diegosouzapw#190.

Remaining diegosouzapw#182-diegosouzapw#188 (js-yaml + mermaid) already closed by diegosouzapw#9651 merge —
awaiting Dependabot re-scan.

npm audit → 0 vulnerabilities.
…egosouzapw#190

Closes Dependabot diegosouzapw#189 (dompurify 3.4.13) and diegosouzapw#190 (nanoid 3.3.17). npm audit → 0.
_tasks is a SEPARATE nested git repo (gitignored). The pattern _tasks/ (trailing
slash) ignores only a directory, not a SYMLINK named _tasks. A self-referential
_tasks symlink can slip in via git add -A and, once pulled, checkout materializes
it over the real _tasks repo (destroying plans/specs/hands-off). Anchored /_tasks
ignores the symlink too, preventing re-capture.
…pw#10026)

Mirror the request-time exclusion rule (provider_specific_data.excludedModels)
in the unified catalog builder: a model is hidden when its provider has
connections but none of them is eligible for it. Applied across the
PROVIDER_MODELS, synced, custom, alias-backed, and managed-fallback loops
so ghost models no longer appear as available.

Co-authored-by: ritheshcn25 <ritheshcn25@users.noreply.github.com>
…osouzapw#10055)

* fix(models): memoize getModelsDevPricing for /v1/models catalog

resolveCatalogPricing called getModelsDevPricing once per model while
building GET /v1/models. Each call re-scanned models_dev_pricing and
JSON.parsed every row (~10k SQL scans + multi-GB parse work), pegging
the event loop so even /healthz timed out (diegosouzapw#9685, diegosouzapw#10052).

Memoize the parsed map until saveModelsDevPricing / clearModelsDevPricing
and add a unit test for invalidation.

Signed-off-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>

* fix(db): invalidate modelsDevPricing cache on DB reset (diegosouzapw#10055)

Copilot review fixes:
1. Register invalidateModelsDevPricingCache() with DB state reset system
   so resetDbInstance() clears the process-local memo, preventing stale
   pricing data from surviving across DB reset/restore operations.
2. Add test assertion verifying DB reset bypasses the memo (Copilot diegosouzapw#10055).

The process-local memo at modelsDevSync.ts:204 caches getModelsDevPricing()
results until saveModelsDevPricing()/clearModelsDevPricing() to avoid
re-scanning all pricing rows on every /v1/models request. Without this hook,
backup restore and test DB resets would serve stale cached data from the
previous connection.

Tests: npm run test:unit:serial -- tests/unit/modelsDevSync-extended.test.ts

---------

Signed-off-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
The file header already advertised this env var but nothing read it.
When catalog/compression pin the event loop, the dashboard (same process)
cannot turn models.dev sync off. Let 0/false/off win over sqlite so an
operator can recover with env + restart. Skip getModelsDevPricing SQL
scans while the kill switch is set.
@RaviTharuma

Copy link
Copy Markdown
Contributor Author

Follow-up links (filed after this PR):

No secrets / hostnames in this comment.

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for chasing this down — the underlying diagnosis is right, and I confirmed two real gaps still exist on release/v3.8.50 today: getModelsDevPricing() doesn't short-circuit on MODELS_DEV_SYNC_ENABLED=0 (still does the full SQL + JSON.parse scan), and applyModelsDevSyncSection() in runtimeSettings.ts only reads the stored setting, so a live PATCH /api/settings can restart the periodic timer even with the kill switch set.

That said, since this branch was opened, MODELS_DEV_SYNC_ENABLED support for initModelsDevSync() already landed on release/v3.8.50 through other work (a separate PR gave it its own docblock and 5 regression tests), and so did the catalog.ts health-check-exclusion filter, the dompurify/nanoid bumps, and the .gitignore _tasks hardening. That's why the diff here is ~191 lines but most of it duplicates what's already on tip, and why GitHub currently shows this PR as conflicting with the base.

Could you rebase onto the current tip and trim the diff down to just the two pieces that are still missing — the getModelsDevPricing() early return and the applyModelsDevSyncSection() env check? A couple of asks while you're in there:

  • Reuse the TRUE_ENV_VALUES parsing already in initModelsDevSync() rather than adding a second parallel parser — the file already has a comment explaining why that specific shape was chosen over two other env-override patterns in the repo.
  • Add a test for the applyModelsDevSyncSection() path specifically (live PATCH with the kill switch set), since that's the one new behavior without direct coverage in this PR.
  • Add MODELS_DEV_SYNC_ENABLED to .env.example if it isn't already there post-rebase — that's what's failing Docs Sync (Strict) right now.

Once it's rebased and scoped down to those two hunks this should be a quick, valuable merge.

@diegosouzapw

Copy link
Copy Markdown
Owner

Babysit summary — CI green

Root cause: stale verdict, not a defect. The branch was 2280 commits behind origin/release/v3.8.50; the 5 red checks (Docs Sync (Strict), Build, Unit Tests 2/8 & 4/8, Quality Ratchet) were computed against a pre-base-drift merge result. A refresh-merge cleared them all with no behavioral changes to the fix.

What landed (pushed to RaviTharuma/OmniRoute fork):

  • a350a27fd — merge refresh of release/v3.8.50 (resolved conflicts in src/lib/modelsDevSync.ts, its test, ENVIRONMENT.md, .gitignore, package-lock.json). Preserved the PR's MODELS_DEV_SYNC_ENABLED=0 kill-switch + tri-state env parsing, and kept the base's cache-version memoization; re-added the PR's kill-switch regression test.
  • 1e4d22861c — prettier formatting restore after the merge base drifted.

Validation: focused modelsDevSync-extended unit test 19/19 pass (incl. the kill-switch test and all base MODELS_DEV_SYNC_ENABLED tests); eslint + prettier clean. No unresolved review threads.

Final gh pr checks 10299: all non-advisory checks pass — Build, Docs Gates, Fast Quality Gates (ratchet), Unit fast-path (1-4/4), Vitest, No-new-ESLint-warnings, dast-smoke, semgrep, Merge integrity, Change Classification. (Mergify prot/queue = skipping, not a failure.)

Ready for human review & merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants