fix(llm): load persisted settings lazily, not in the constructor - #73
Open
Anurag-Wednesday wants to merge 1 commit into
Open
fix(llm): load persisted settings lazily, not in the constructor#73Anurag-Wednesday wants to merge 1 commit into
Anurag-Wednesday wants to merge 1 commit into
Conversation
LLMService read its persisted state (active model + user settings) from the
constructor. `llm` is a module-level singleton, so it is constructed while
index.ts's IMPORTS are still evaluating - which under ESM completes before
index.ts's own body runs unifyUserDataPath() -> app.setPath('userData', ...).
Every path resolved at construction therefore pointed at the PRE-override
profile.
Two real consequences:
1. Production: at construction the canonical-dir migration ("My Memories" /
"my-memories" -> "Off Grid AI Desktop") has not run yet, so a user's saved
settings and active model could be silently missed and replaced by
defaults.
2. Harness: an OFFGRID_USER_DATA temp profile was ignored outright. A probe
confirmed the constructor resolving the REAL profile while
OFFGRID_USER_DATA pointed at the temp dir. This is what made
e2e/settings-sections.spec.ts "resource mode survives a relaunch" fail -
the setting persisted correctly but was never read back.
Writes never had the bug: persist() goes through the settingsFile getter,
which resolves late. This was read-side-only asymmetry. It is also the exact
hazard the activeModelFile / settingsFile getters were introduced to avoid
(see the comment at llm.ts:98) - calling resolveModel() and reading the
settings file from the constructor defeated them.
Fix: drop the constructor and load once, lazily, via ensureLoaded(), wired
into the ten public entry points that depend on persisted state or model
paths. hasVision/modelsExist/activeModelInfo keep their deliberate
resolveModel() call so a newly activated model is still picked up.
Tests: 5 cases in llm-lazy-settings-load.test.ts, built on the configureRuntime
seam so they reproduce the production shape (construct first, choose the
profile second). Includes a two-profile case that pins the defect directly -
configure A, construct, switch to B, and assert B is read. 4 of the 5 fail
without this change and all 5 pass with it. e2e/settings-sections.spec.ts is
3/3 (was 2/3).
Verified: npm test 3163 passed; node + web typechecks clean; eslint 0 errors.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015U3TnnNULxfCb4TsjAGjiC
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
📝 WalkthroughWalkthrough
ChangesLLM lazy loading
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/__tests__/llm-lazy-settings-load.test.ts`:
- Around line 46-110: Extend the lazy-loading regression coverage around
LLMService.ensureLoaded() to include active-model resolution: seed different
active-model.json values for profiles A and B, construct the service while A is
configured, switch to B before first use, then assert launchArgs() uses B’s
primary model. Ensure the test fails if ensureLoaded() omits resolveModel() and
retains the existing first-use profile behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7be83132-6980-4796-b57a-48e305434435
📒 Files selected for processing (2)
src/main/__tests__/llm-lazy-settings-load.test.tssrc/main/llm.ts
Comment on lines
+46
to
+110
| describe('LLMService reads persisted settings lazily (not at construction)', () => { | ||
| it('picks up a data dir configured AFTER the instance was constructed', () => { | ||
| // Construct FIRST — mirrors the module-level singleton being built during imports. | ||
| const svc = new LLMService() | ||
| // ...then point the runtime at the profile, as index.ts's body does later. | ||
| seedSettings(tmp, { performanceMode: 'extreme', temperature: 0.42 }) | ||
| configureRuntime({ dataDir: tmp }) | ||
|
|
||
| const s = svc.getSettings() | ||
| expect(s.performanceMode).toBe('extreme') | ||
| expect(s.temperature).toBe(0.42) | ||
| }) | ||
|
|
||
| it('survives the relaunch shape: persisted mode is read back by a fresh instance', () => { | ||
| // What settings-sections.spec.ts "resource mode survives a relaunch" exercises: | ||
| // one process writes the mode, the next process constructs and must read it back. | ||
| seedSettings(tmp, { performanceMode: 'conservative' }) | ||
| const relaunched = new LLMService() | ||
| configureRuntime({ dataDir: tmp }) | ||
|
|
||
| expect(relaunched.getSettings().performanceMode).toBe('conservative') | ||
| }) | ||
|
|
||
| it('loads once and does not re-read after the first access', () => { | ||
| seedSettings(tmp, { performanceMode: 'conservative' }) | ||
| const svc = new LLMService() | ||
| configureRuntime({ dataDir: tmp }) | ||
| expect(svc.getSettings().performanceMode).toBe('conservative') | ||
|
|
||
| // A later on-disk edit must NOT leak in: the load is once-only, so in-memory state | ||
| // stays authoritative until something explicitly persists. This guards against | ||
| // turning the lazy guard into a read-on-every-call, which would re-read the file | ||
| // on every getSettings and clobber unsaved in-memory changes. | ||
| seedSettings(tmp, { performanceMode: 'extreme' }) | ||
| expect(svc.getSettings().performanceMode).toBe('conservative') | ||
| }) | ||
|
|
||
| it('falls back to defaults when the profile has no settings file', () => { | ||
| const svc = new LLMService() | ||
| configureRuntime({ dataDir: tmp }) // seeded with nothing | ||
| expect(svc.getSettings().performanceMode).toBe('balanced') | ||
| }) | ||
|
|
||
| // The direct guard on the defect, stated behaviourally rather than by spying on fs: | ||
| // if construction reads eagerly, it reads the profile configured AT THAT MOMENT. | ||
| // Point the runtime at profile A, construct, then switch to profile B before first | ||
| // use — a lazy reader returns B, an eager one returns A. This is the exact shape of | ||
| // the production bug (construct during imports, real profile chosen afterwards). | ||
| it('reads the profile configured at FIRST USE, not the one present at construction', () => { | ||
| const other = fs.mkdtempSync(path.join(os.tmpdir(), 'offgrid-llm-lazy-other-')) | ||
| try { | ||
| seedSettings(other, { performanceMode: 'extreme' }) // profile A | ||
| seedSettings(tmp, { performanceMode: 'conservative' }) // profile B | ||
|
|
||
| configureRuntime({ dataDir: other }) // A is current... | ||
| const svc = new LLMService() // ...at construction | ||
| configureRuntime({ dataDir: tmp }) // the override lands afterwards | ||
|
|
||
| // Eager construction would have pinned 'extreme' from profile A. | ||
| expect(svc.getSettings().performanceMode).toBe('conservative') | ||
| } finally { | ||
| fs.rmSync(other, { recursive: true, force: true }) | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a regression test for lazy active-model resolution.
These tests only assert settings loaded through getSettings(). They do not assert that ensureLoaded() reads active-model.json from the profile selected at first use.
An implementation that omits resolveModel() from ensureLoaded() would pass this suite. Seed different active-model files before and after construction, then assert that launchArgs() uses the primary model from the profile active at first use.
As per coding guidelines, “Every approved behavior change must add a regression or integration test in the same change, covering branches, conditions, and error paths rather than deferring tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/__tests__/llm-lazy-settings-load.test.ts` around lines 46 - 110,
Extend the lazy-loading regression coverage around LLMService.ensureLoaded() to
include active-model resolution: seed different active-model.json values for
profiles A and B, construct the service while A is configured, switch to B
before first use, then assert launchArgs() uses B’s primary model. Ensure the
test fails if ensureLoaded() omits resolveModel() and retains the existing
first-use profile behavior.
Source: Coding guidelines
|
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.
A real startup bug in core, found while chasing an e2e failure. Independent of the Windows-Pro work — split out of #72 so it reviews and reverts on its own.
The bug
LLMServiceread its persisted state (active model + user settings) from the constructor. Butllmis a module-level singleton:So it is constructed while
index.ts's imports are still evaluating (index.ts:19→./ipc→./llm), which under ESM completes beforeindex.ts's own body runsunifyUserDataPath()→app.setPath('userData', …)at line 73. Every path resolved at construction therefore pointed at the pre-override profile.Proven with a probe rather than inferred — both launches of the failing spec logged:
The constructor resolving the real profile while
OFFGRID_USER_DATApointed elsewhere.Two consequences
"My Memories"/"my-memories"→"Off Grid AI Desktop") has not run yet, so a user's saved settings and active model could be silently missed and replaced by defaults.OFFGRID_USER_DATAtemp profile was ignored outright. This is what madee2e/settings-sections.spec.ts:124"resource mode survives a relaunch" fail — the setting persisted correctly and was never read back.Writes never had the bug:
persist()goes through thesettingsFilegetter, which resolves late. Read-side-only asymmetry.This is also the exact hazard the
activeModelFile/settingsFilegetters were introduced to avoid — see the comment atllm.ts:98, "the data dir can be set AFTER this class is constructed … computing the path at construction would pin it to the wrong location and miss active-model.json." CallingresolveModel()and reading the settings file from the constructor defeated both getters.The fix
Drop the constructor; load once, lazily, via
ensureLoaded(), wired into the ten public entry points that depend on persisted state or model paths.hasVision/modelsExist/activeModelInfokeep their deliberateresolveModel()call so a newly activated model is still picked up.Tests
Five cases in
src/main/__tests__/llm-lazy-settings-load.test.ts, built on theconfigureRuntimeseam so they reproduce the production shape — construct first, choose the profile second. The strongest one pins the defect directly with two profiles: configure A, construct, switch to B, assert B is read (an eager constructor returns A).Deliberately behavioural rather than spying on
fs— animport * as fsnamespace can't be reliably patched, and asserting the observable outcome is what actually guards the bug.4 of the 5 fail without this change; all 5 pass with it. Verified by stashing the fix and re-running.
e2e/settings-sections.spec.tsis now 3/3 (was 2/3). Node + web typechecks clean; ESLint 0 errors on both touched files.Note on the e2e gate
CI's e2e job is advisory (
continue-on-error: true,ci.yml:158), which is how this reachedmain. The job's own comment records that being advisory previously hid four real defects for days — this is a fifth. Not changed here, but worth weighing: it is currently the only thing standing between a defect like this and a red build.🤖 Generated with Claude Code
https://claude.ai/code/session_015U3TnnNULxfCb4TsjAGjiC
Summary by CodeRabbit