refactor(config): commit settings to SQLite before applying them - #1094
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_71145043-92b2-4e7c-a446-f9f530138e92) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0d34dea6-eaf0-4c30-b405-15fad254e315) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18abc2633c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| info := s.deps.SelfUpdate.Info() | ||
| if info.TargetStateSchema > 0 && info.TargetStateSchema < 2 && s.deps.Cfg != nil && s.deps.CfgMu != nil { |
There was a problem hiding this comment.
When a release omits or mangles the state-schema marker, releaseStateSchema returns 0 and the checker already treats that target as requiring a full backup, but this condition skips the downgrade block. On an installation whose settings authority is SQLite, a YAML-only candidate with unknown metadata can therefore start successfully using the stale recovery export while silently ignoring settings saved since migration. Require a positively known schema of at least 2 before dispatching the update.
Useful? React with 👍 / 👎.
| if s.deps.ConfigApplier != nil { | ||
| s.deps.ConfigApplier(&next, &old) | ||
| } | ||
| if s.deps.Registry != nil { | ||
| if err := s.deps.Registry.Restart(r.Context(), *restartCfg); err != nil { |
There was a problem hiding this comment.
With the normal main.go wiring, ConfigApplier is applyConfigChange, whose first operation calls reg.Reload; changing refresh_token makes the driver configuration differ, so that call already removes and re-adds the driver. Execution then reaches Registry.Restart and repeats the full autonomous-default/init cycle, potentially rotating credentials again and interrupting the integration twice. Only invoke the explicit restart when no applier is installed.
Useful? React with 👍 / 👎.
| return errors.New("move the state database offline; its path cannot change in Settings") | ||
| } | ||
| } | ||
| if err := cfg.Validate(); err != nil { |
There was a problem hiding this comment.
Several narrow writers—driver enable/disable, LAN-password, MyUplink, and loadpoint updates—construct candidates with shallow Config copies, leaving fields such as Planner, Weather, and V2X aliased to s.deps.Cfg. Calling the mutating Validate/NormalizeUnits here writes through those aliases without the configuration write lock (the loadpoint path holds only an RLock), creating data races with runtime readers despite configWriteMu. Deep-clone the candidate before validation or otherwise give validation exclusive ownership.
AGENTS.md reference: AGENTS.md:L56-L56
Useful? React with 👍 / 👎.
1 participant
Settings currently span YAML and separate database writes. A save can change a credential or live setting even when the rest fails, and file reloads add another writer. This change imports the full config into the existing SQLite database once, then commits each Settings change with a revision and its credentials before applying it. The browser rejects stale forms with a clear reload message.
Depends on #1093; this PR targets that branch so the calendar removal stays in its own diff. It follows the planner/forecast work through #1091. The storage revision is separate from forecast identity. Tests preserve the site ID, learning revision, opaque Rust model bytes, nil/zero values and existing runtime keys.
config.yamlbecomes an initial import and database locator. A missing or corrupt database fails startup instead of using old YAML or opening first-run setup. The import preserves older YAML fields for image rollback. A source hash lets a later upgrade distinguish an old Core's new Settings save from an interrupted import, which reuses the committed document.First migration needs write access to the config seed. For a read-only mount, copy the seed into the persistent data directory and change
-configbefore upgrading. Full backups require the config seed inside that directory; the native helper accepts-configfor a custom name. Moving the state database remains an offline task.Validation:
make verify-all, 541 web tests, and final config/API/backup/command suites pass. Full PR CI passed on 18abc26, including Linux Go, full stack and Windows config/ACL tests. Fault tests cover failed commits, stale revisions, interrupted import, a recovered database with a different config at the same revision, blocked image-only downgrade, backup consistency and restore to a new directory. A local Core with no hardware passed browser save, stale-form rejection and restart checks.A Docker failure test used the published v2.17.1-beta.1 Core and updater, the old update API handler, synthetic target metadata and a forced unhealthy candidate. After the first import, the old image returned healthy with the same settings, including CalDAV, and byte-identical saved Go/Rust models and forecast identity. A real Settings save through the old Core after rollback also survived the next upgrade and imported into SQLite at revision 2. Regression tests cover both failures. This does not prove physical-box adoption or SD-card power-loss behavior. Human UI review remains before merge.
Open PRs #1052, #1003, #826, #735 and #734 share some API/config/main files. This change stays within persistence and apply order; it does not replace their feature work. No merge or release is part of this PR.
Note
High Risk
This rewrites how all site settings and credentials are stored, loaded, backed up, and applied at runtime, with migration and downgrade rules that can brick misconfigured upgrades without a full backup restore.
Overview
SQLite becomes the settings authority. Core imports
config.yamlonce, writesconfig_databaseinto the seed file, and loads live config from a versioned JSON document plus credential rows in SQLite. Editing the seed no longer reloads runtime settings; the YAML watcher is removed and saves go throughSaveStored/configreload.Applyonly after a durable transaction (synchronous=FULL).Safer writes and upgrades. Settings API uses revision ETags and
If-Matchto reject stale browser forms; a globalconfigWriteMuserializes config mutators so failed commits do not change live control. EV charger, LAN auth, and OAuth tokens are persisted with the document instead of ad hoc KV paths (driver rotation still uses separate KV). State schema 2 forces a full backup before migration and blocks image-only downgrade when SQLite settings exist; full backups and update snapshots export current YAML from the same DB snapshot for older Core restores.Boot and recovery behavior changes. Unreadable or missing settings DB fails startup rather than falling back to YAML or first-run setup; import preserves legacy YAML fields and source-hash logic for rollback/re-upgrade. Forecast learning identity and opaque model bytes are explicitly left untouched by settings revisions.
Reviewed by Cursor Bugbot for commit 50d0b26. Bugbot is set up for automated code reviews on this repo. Configure here.