Skip to content

feat(lib): make MODELS_DEV_SYNC_ENABLED actually control the sync - #9483

Merged
diegosouzapw merged 5 commits into
cheaper-inference:release/v3.8.50from
HouMinXi:fix/models-dev-sync-env-precedence
Aug 7, 2026
Merged

diegosouzapw merged 5 commits into
cheaper-inference:release/v3.8.50from
HouMinXi:fix/models-dev-sync-env-precedence

Conversation

@HouMinXi

@HouMinXi HouMinXi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The docblock on src/lib/modelsDevSync.ts says "Opt-in via MODELS_DEV_SYNC_ENABLED=true (default: false)", and nothing reads that variable. Its only appearance anywhere in the tree is that comment line: initModelsDevSync consults the stored modelsDevSyncEnabled setting and nothing else, so an operator who follows the documentation gets no response whichever value they set.

$ grep -rn MODELS_DEV_SYNC_ENABLED --include='*.ts' --include='*.tsx' \
    --include='*.md' --include='*.example' --include='*.yml' . | grep -v node_modules
./src/lib/modelsDevSync.ts:17: * Opt-in via MODELS_DEV_SYNC_ENABLED=true (default: false).

This gives the variable the behaviour the comment promised, and describes it in .env.example and docs/reference/ENVIRONMENT.md, where it was also missing.

The precedence, and why it is a third shape

An explicit value decides in either direction; only an unset or empty one defers to the setting. Pinning the sync off from a compose file or unit is the case that motivates it: the stored setting can already say on, and a container has no way to reach the dashboard before boot.

That makes this a third resolution shape rather than a reuse of an existing one, which seems better stated than glossed over:

  • src/shared/utils/featureFlags.ts::resolveFeatureFlag puts the DB override above the environment, so a deployment cannot override an operator's stored choice at all.
  • src/lib/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, so following either one would leave this variable unable to do the thing it is being added for. The reasoning is recorded next to the code, not only here. If you would rather it match ccDiscoveryAliases exactly and stay force-on-only, that is a small change and I am happy to make it.

Verification

$ node --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts \
    --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit \
    tests/unit/modelsDevSync-extended.test.ts \
    tests/unit/modelsDevSync.test.ts \
    tests/unit/modelsdevsync-transform-split.test.ts
# tests 47
# pass 47
# fail 0

Five new tests cover the matrix: env on with the setting off, the setting alone with no env var, the accepted spellings and their rejection of everything else, env off against a stored on, and unset still deferring to a stored on. The last one is there so the change cannot quietly turn the variable into a hard switch and kill the dashboard toggle.

To confirm the tests actually detect the old behaviour, initModelsDevSync was reverted to the setting-only check and the same command re-run:

# tests 47
# pass 44
# fail 3

not ok - MODELS_DEV_SYNC_ENABLED=true starts the sync even with the setting off
not ok - the usual truthy spellings all start the sync, and nothing else does
not ok - MODELS_DEV_SYNC_ENABLED=false turns the sync off despite a stored setting of true

Green again on revert.

$ npm run typecheck:core
(exit 0)

Note on the base

npm run lint exits 1 on this branch, and identically on an unrelated branch of mine whose diff shares no files with this one. Both report the same two errors:

tests/unit/issue-9407-gemini-web-validation-false-positive.test.ts:50:38
tests/unit/v1-models-auth-leak-9320.test.ts:83:54
  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

Neither file is touched here. Both arrived with #9407 and #9320 on 2026-08-04 and are absent from config/quality/eslint-suppressions.json, so release/v3.8.50 fails that gate on its own and every PR against it inherits the failure. Happy to send a separate PR adding the two entries if that is the fix you want.

The docblock on modelsDevSync.ts has always said "Opt-in via
MODELS_DEV_SYNC_ENABLED=true", but nothing read that variable. Its single
appearance in the tree was that one comment line: initModelsDevSync consulted
the stored modelsDevSyncEnabled setting and nothing else, so an operator who
followed the documentation got no response whichever value they set. Give the
variable the behaviour the comment promised, and describe it in .env.example
and the environment reference, where it was also missing.

An explicit value decides in either direction; only an unset or empty one
defers to the setting. Pinning the sync OFF from a compose file or unit is the
case that motivates it, since the stored setting can already say on and a
container has no way to reach the dashboard before boot.

That makes this a third resolution shape rather than a reuse of an existing
one, which is worth stating rather than glossing. resolveFeatureFlag puts the
DB override above the environment, so a deployment cannot override a stored
choice. getCcAliasGlobalState reads only "1" and "true" and forces a flag on,
letting every other value fall through to the DB. Neither can turn a
dashboard-enabled switch off from the environment, so either one would leave
this variable unable to do the thing it is here for. The reasoning is recorded
next to the code.

Five tests cover the resulting matrix: env on with the setting off, the
setting alone with no env var, the accepted spellings and their rejection of
everything else, env off against a stored on, and unset still deferring to a
stored on.
@HouMinXi
HouMinXi requested a review from diegosouzapw as a code owner August 5, 2026 03:16
@diegosouzapw
diegosouzapw merged commit 765fd71 into cheaper-inference:release/v3.8.50 Aug 7, 2026
4 of 5 checks passed
@HouMinXi
HouMinXi deleted the fix/models-dev-sync-env-precedence branch September 16, 2026 14:04
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…eaper-inference#9483)

Merge-train validated (tip 6ce4effef8). Vitest failures confirmed as base-red (cheaper-inference#9679).
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.

2 participants