Repository navigation
fix(feature-flags): don't connect to db before model is loaded (fixes #2066 unit regression + latent boot bug) - #2073
Merged
Conversation
…2066 unit regression) #2066 migrated on/off feature flags from env vars to ImsConfig-backed DB rows. The generic resolver srv/lib/feature-flags/db-flags.js exposes a synchronous isFlagEnabled() that, on a cold cache, fires a background refreshFeatureFlags() via scheduleRefresh(). server.js reads isFlagEnabled('MCP_AUTH_ENABLED') inside cds.on('bootstrap') to install the MCP kill switch. At bootstrap time cds.model is not yet linked, so the fire-and-forget scheduleRefresh() called cds.connect.to('db') prematurely. cds.connect caches cds.db eagerly (cds.services.db = the pending promise) and resolves the db service's model from the partial, service-less model that exists mid-bootstrap. That poisoned primary-db instance then persists for the process, so cqn4sql can no longer resolve service-qualified targets (SearchService.SearchableItems etc.), leaving '*' unexpanded and every later service read throwing 500 "Query was not inferred and includes '*' in the columns" — ~205 unit failures. content-delta-flags.js was unaffected only because none of its getters run during bootstrap. Fix: scheduleRefresh() no-ops while cds.model is unset. The bootstrap read then returns the flag's declared registry default (the intended cold-cache behavior, per the server.js comment) and the real warm-up runs in the 'served' handler once the model is fully loaded. Runtime-safe on every dialect: the poison is model resolution, not SQLite-specific rendering. Also update two stale kg-path tests (kg-path-v2-or-v1, kg-path-between-handler) that still toggled the now-DB-driven KG_PATH_V2_ENABLED via vi.stubEnv; switch them to __setFlagForTest/__resetFlagsForTest, matching the pattern #2066 already applied to the other migrated-flag tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After #2066 (flags → DB config), the unit suite went red (~205 fails across unrelated services, all HTTP 500 "Query was not inferred and includes '*'").
Root cause (a latent RUNTIME bug, not just tests)
srv/server.jscallsisFlagEnabled('MCP_AUTH_ENABLED')insidecds.on('bootstrap')— beforecds.modelis linked. The genericdb-flags.jsresolver's cold-cache path kicks off a backgroundrefreshFeatureFlags()→cds.connect.to('db'), which eagerly caches the db service against the partial, service-less bootstrap model (proven: primary db built once with a 211-def model vs the full 737-def model, and it persists).SQLService.cqn4sqlthen short-circuits for any target missing from that stale db model, soSELECT * … expandreachesCQN2SQLite.SELECT_expandwith a literal'*'→ 500. The poisoned db is process-wide, hence the broad blast radius. This would poison HANA at runtime too — it's not SQLite-specific — so it's a latent production boot bug, caught before deploy.Fix
scheduleRefresh()no-ops whilecds.modelis unset (if (!cds.model) return;). The bootstrap flag read then returns the flag's declared registry default — the documented cold-cache behavior — and the real warm-up still runs in theservedhandler once the model is fully loaded. Runtime-safe on every dialect; nothing legitimately reads a flag before the model exists except that one bootstrap line.Also fixes 2 stale tests (
kg-path-v2-or-v1,kg-path-between-handler) that still toggled the now-DB-drivenKG_PATH_V2_ENABLEDviavi.stubEnv→ switched to__setFlagForTest.Verification
vitest run --project unit: 8804 passed, 0 test failures (the file-level failures in that run are pre-existing env/parallelism: cds.test hook timeouts under 1194-file load, srv-qaparsers.bundle.mjsneeds thepretesthook, hugo-apps needsnpm run setup).Fixes the DEV-red from #2066. Should merge before any deploy of the flag-migration work.