fix(config): surface a user notice when a corrupt config.toml is reset (#5167) - #5340
Conversation
tinyhumansai#5167) Two Windows users' config.toml files failed UTF-8 validation and flooded Sentry. The loader already rate-limits the error, renames the corrupt file to .corrupted.<ts>, and resets to defaults/backup (the flood was fixed in tinyhumansai#5171) — but that recovery was silent to the user, leaving requirement 3 of the issue (a user-visible warning) unmet. Surface a one-shot, user-visible notice: - Config carries a runtime-only `recovered_from_corruption` flag, set by the loader on the corruption-recovery path (both `load_or_init` and `load_from_config_path`). - A process latch (`app_state::recovery_signal`), set once at boot from that flag, keeps the signal reported after the loader heals the file on the same boot (the per-load flag reads false on subsequent, now-clean loads). - `app_state_snapshot` exposes it as `configRecovered`; the frontend raises a single "Settings were reset" System notice in the notification center, guarded so the latched flag does not re-fire on every snapshot poll. Closes tinyhumansai#5167 Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConfiguration loading now records corruption recovery, propagates a session-latched signal through app-state snapshots, and surfaces a one-time localized settings notification. Rust and Vitest tests cover clean, recovered, repeated, false, and absent recovery states. ChangesConfiguration recovery signaling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigLoader
participant AppStateSnapshot
participant CoreStateProvider
participant NotificationStore
ConfigLoader->>AppStateSnapshot: record configRecovered
CoreStateProvider->>AppStateSnapshot: refresh core state
AppStateSnapshot-->>CoreStateProvider: return configRecovered
CoreStateProvider->>NotificationStore: dispatch localized notice once
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
![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: 02d89756f0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@app/src/lib/configRecoveryNotice.ts`:
- Around line 41-42: Move the “Settings were reset” title and body out of
configRecoveryNotice and into the i18n system: add dedicated keys with real
translations to en.ts and every locale, preserving any interpolation
placeholders. Update the hook-aware caller or dispatch flow around the notice so
it obtains both strings through useT() from I18nContext before creating the
notice.
In `@app/src/providers/CoreStateProvider.tsx`:
- Around line 292-294: Add a CoreStateProvider behavior test covering
refreshCore with a snapshot where configRecovered is true, and assert that
exactly one system recovery notification is emitted. Exercise the provider-level
path through maybeSurfaceConfigRecovery rather than only testing the helper,
while preserving the existing one-shot behavior across repeated refreshes.
In `@src/openhuman/config/schema/types.rs`:
- Around line 97-104: Replace the boolean recovery state with a runtime outcome
distinguishing none, backup restoration, and defaults reset. In
src/openhuman/config/schema/types.rs:97-104 and
src/openhuman/config/schema/load/impl_load.rs:405-408,600, set and propagate the
specific outcome; add assertions for both recovery paths in
src/openhuman/config/schema/load_tests.rs:1900-1906. Update
recovery_signal.rs:40-49 and ops.rs:176-181,1196 to latch and expose the
outcome, then update app/src/services/coreStateApi.ts:68-75,
CoreStateProvider.tsx:292-294, and configRecoveryNotice.ts:33-46 to preserve
older-core compatibility and show matching wording; extend
configRecoveryNotice.test.ts:12-34 to verify each outcome dispatches once.
In `@src/openhuman/desktop/app_state/ops.rs`:
- Line 1196: Add snapshot-level tests around app_state_snapshot to cover both
recovered and clean states, asserting the serialized camelCase configRecovered
field consumed by the frontend. Reuse the existing snapshot test setup and
verify the field’s value in each state, rather than testing only the
config_recovered latch directly.
In `@src/openhuman/desktop/app_state/recovery_signal.rs`:
- Around line 64-106: Protect both tests, latch_defaults_false_and_marks_true
and latch_from_config_only_marks_when_recovered, with a shared test-only mutex
guard held for each entire test body. Acquire the guard before reset_for_tests
and retain it through the final reset_for_tests so concurrent tests cannot
mutate CONFIG_RECOVERED during assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b70952d8-2672-4d14-b5b8-2b262435df14
📒 Files selected for processing (11)
app/src/lib/configRecoveryNotice.test.tsapp/src/lib/configRecoveryNotice.tsapp/src/providers/CoreStateProvider.tsxapp/src/services/coreStateApi.tssrc/core/jsonrpc.rssrc/openhuman/config/schema/load/impl_load.rssrc/openhuman/config/schema/load_tests.rssrc/openhuman/config/schema/types.rssrc/openhuman/desktop/app_state/mod.rssrc/openhuman/desktop/app_state/ops.rssrc/openhuman/desktop/app_state/recovery_signal.rs
|
| Filename | Overview |
|---|---|
| src/openhuman/desktop/app_state/recovery_signal.rs | New process-lifetime AtomicBool latch bridging the config-loader's per-load flag to the snapshot poll cycle; includes serialized unit tests using a shared Mutex guard to prevent parallel-test interference. |
| app/src/lib/configRecoveryNotice.ts | New module-level one-shot dispatcher; the surfaced guard and stable NOTICE_ID together ensure exactly one Redux dispatch per app run. |
| app/src/providers/CoreStateProvider.tsx | Wires maybeSurfaceConfigRecovery into refreshCore after the mount guard, with t from useT() in the callback dependency array for correct locale handling. |
| src/openhuman/desktop/app_state/ops.rs | Adds config_recovered: bool to AppStateSnapshot and calls latch_from_config on every snapshot poll, correctly catching mid-session corruption in addition to the boot-time latch. |
| src/openhuman/config/schema/types.rs | Adds recovered_from_corruption: bool with #[serde(skip)] — correctly never persisted, defaults to false. |
| src/openhuman/config/schema/load/impl_load.rs | Sets recovered_from_corruption before env overrides in both recovery branches. |
| app/src/lib/configRecoveryNotice.test.ts | Four focused unit tests cover happy path, i18n wiring, one-shot deduplication, and false/absent no-op. |
| app/src/services/coreStateApi.ts | Adds optional configRecovered?: boolean to AppStateSnapshotResult for backward compat. |
| src/core/jsonrpc.rs | Adds boot-time latch call after config load in bootstrap_core_runtime. |
| src/openhuman/config/schema/load_tests.rs | New async test for load_from_config_path with binary-invalid config and updated assertions for the recovery flag. |
| app/src/providers/tests/CoreStateProvider.test.tsx | Three integration tests verify full notice dispatch path including one-shot guard across repeated refreshes. |
Sequence Diagram
sequenceDiagram
participant Loader as Config Loader
participant Bootstrap as bootstrap_core_runtime
participant Signal as recovery_signal AtomicBool
participant Snapshot as app_state_snapshot
participant Frontend as CoreStateProvider refreshCore
participant Notice as configRecoveryNotice surfaced guard
participant Store as Redux Store
Loader->>Bootstrap: "Config with recovered_from_corruption=true"
Bootstrap->>Signal: "latch_from_config sets CONFIG_RECOVERED=true"
Note over Bootstrap,Signal: Boot-time latch
loop Every snapshot poll
Snapshot->>Loader: load_config_with_timeout
Loader-->>Snapshot: Config recovered_from_corruption true or false
Snapshot->>Signal: latch_from_config idempotent mid-session catch
Snapshot->>Signal: config_recovered_this_session
Signal-->>Snapshot: true latched
Snapshot-->>Frontend: AppStateSnapshot configRecovered true
end
Frontend->>Notice: maybeSurfaceConfigRecovery true t after mount guard
alt surfaced is false first call
Notice->>Store: dispatch notificationReceived id config-recovered
Notice->>Notice: "surfaced = true"
else surfaced is true subsequent calls
Notice-->>Frontend: no-op
end
Reviews (3): Last reviewed commit: "fix(config): log config-recovery once pe..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…sts (tinyhumansai#5167) Review-cycle fixes for tinyhumansai#5340: - Latch config-corruption recovery from every app_state_snapshot poll, not only at boot. load_config_with_timeout re-reads config.toml on each snapshot, so a config that becomes corrupt after startup is healed there and carries a fresh recovered_from_corruption; the boot-only latch dropped it and the notice never surfaced. Now snapshot() calls latch_from_config on each poll's fresh config (Codex P2, Greptile). - Move the notice copy into i18n: add notifications.configRecovered.title/body to en.ts and all 13 other locales, and thread `t` (useT) from CoreStateProvider into maybeSurfaceConfigRecovery so the notice honors the active locale instead of hardcoded English (CodeRabbit). - Make the copy accurate for both recovery outcomes. The core sets one recovered_from_corruption flag whether it restored the previous settings from a .bak backup or reset to defaults, so the notice no longer hard-claims "reset to defaults" (which is wrong in the backup case) and instead states it was restored from a backup or reset to defaults (CodeRabbit). - Add a provider-level test: refreshCore forwards configRecovered to a single system notice (id=config-recovered, deep-links /settings) and stays one-shot across repeated refreshes, asserted via a dispatch spy (CodeRabbit). - Serialize the recovery_signal latch tests behind a shared Mutex so the parallel test runner cannot interleave reset/assert on the process-global CONFIG_RECOVERED atomic; the guard recovers from poisoning (CodeRabbit, Greptile). - Reword the load_from_config_path recovery-flag test comment to state what it actually asserts (the per-load flag) and where surfacing is wired (app_state_snapshot latching), so it no longer overstates coverage (Greptile). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review feedback addressed in
|
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Review
The mechanism is sound. I traced the full path end to end and it holds up:
AppStateSnapshotcarries#[serde(rename_all = "camelCase")], soconfig_recovered→configRecoveredmatches the TS field. Worth calling out because the neighbouringhealthfield's comment says the type has no camelCase rename — that refers toHealthSnapshot, not the outer struct, and it reads as a trap.load_config_with_timeouthas no cache (deliberately — the comment cites a racy prior attempt), so the per-poll re-latch genuinely catches mid-session corruption and the flag cannot get stucktrueon a cached config.config_was_corruptedfolds both the read-recovery and the parse-recovery paths, and the assignment sits beforeapply_env_overrides_from, so no override can mask it.CoreStateProvideris nested insideI18nProvider(App.tsx:144), sotresolves against the real locale rather than the default context.useT'stisuseCallback-stable per locale, so adding it torefreshCore's dep array does not churn the poll loop.- All 14 locales in
I18nContext'stranslationsmap got both keys — none missed.
One blocking item, then some smaller notes.
1. Blocking (one-liner) — the latch warn is unbounded when the heal itself fails
latch_from_config logs warn! on every call where the flag is set, and it is now called from snapshot() — which polls every 2–5s (app/src/providers/CoreStateProvider.tsx:50 / :61).
In the normal case that is fine: the file gets healed, later loads are clean, one warn total.
But impl_load.rs has a branch that deliberately does not heal — when fs::rename of the corrupt file fails it logs "skipping save to protect the .bak — will retry recovery on next startup". A Windows sharing violation (os error 32) puts you there, and this codebase already handles that error by name elsewhere. In that state every poll re-recovers and emits a fresh warn: roughly 12–30 lines/min, indefinitely, for the whole session.
The doc comment on latch_from_config claims it logs "once, on the load that actually recovered" — that is not true in this branch.
To be clear on blast radius: this is not a Sentry event flood. src/core/logging.rs:634 maps WARN → Breadcrumb, so #5171's fix holds. It is unbounded local log noise — but in exactly the scenario #5167 originated from, which makes it worth fixing before merge.
mark_config_recovered already has the information needed:
pub fn latch_from_config(config: &Config) {
if !config.recovered_from_corruption {
return;
}
if !CONFIG_RECOVERED.swap(true, Ordering::Relaxed) {
log::warn!("[app_state] config.toml was unreadable/corrupt and was recovered ...");
}
}2. Minor — the notice is persisted, and a repeat recovery will not resurface it
items is redux-persist-whitelisted and user-scoped (app/src/store/index.ts:145). notificationReceived replaces a matching id in place, and NotificationCenter renders items in array order with no sort (the array is insertion-ordered via unshift).
So a second recovery in a later session updates the row's timestamp and unread flag but leaves it buried at whatever position the previous session's notice occupied — potentially far down a list capped at 200. A per-session id suffix, or re-unshifting on replace, would fix it.
Related: resetUserScopedState resets the slice to initialState, but the module-level surfaced flag stays true. An identity flip occurring after the dispatch wipes the notice with no re-fire.
3. Nit — log::warn! vs tracing::warn!
recovery_signal.rs uses log::warn! while the surrounding config and app_state code uses tracing::warn!. It works through the bridge, but it loses structured fields and is inconsistent with the module it lives in.
4. Nit — dispatch happens before the mount guard
maybeSurfaceConfigRecovery is called before if (!isMountedRef.current) return; in refreshCore, so a superseded or unmounted refresh still dispatches. Harmless given the one-shot guard, but it is the only side effect in that function that ignores the request-id / mount discipline the rest of the body is careful about.
Requesting changes on (1) only; it is a one-line swap. The rest are non-blocking and fine as follow-ups.
…ing (tinyhumansai#5167) Addresses @sanil-23's review on tinyhumansai#5340. - latch_from_config logged warn on every call where the flag is set, and it now runs on every app_state_snapshot poll. When a corrupt config can't be healed (impl_load skips the rename to .corrupted.<ts> on e.g. a Windows sharing violation, os error 32, and retries next load) every poll re-recovers and would emit a fresh warn indefinitely. Gate the warn on the latch's false->true transition: mark_config_recovered now returns whether it was the first mark (via AtomicBool::swap), so the log fires once per process. Fixed the doc comment (it wrongly claimed 'once, on the load that recovered') and added a test asserting the transition semantics. - Use tracing::warn! instead of log::warn! for consistency with the surrounding config/app_state code. - Move maybeSurfaceConfigRecovery below refreshCore's mount guard so a superseded/unmounted refresh no longer dispatches; the core latches the flag, so the next live poll still surfaces the notice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@sanil-23 thanks — this is a genuinely useful trace, and the blocking call is right. (1) Blocking — unbounded warn on the non-heal branch — fixed in
|
3 participants
Summary
config.toml, closing the last unmet requirement of config.toml corruption: stream did not contain valid UTF-8 — two users affected #5167 (the Sentry flood and auto-recovery were already fixed in fix: Sentry triage — resolve 16 actionable issues across tauri-react, tauri-rust, core-rust #5171).Config::recovered_from_corruptionflag, set by the loader on the corruption-recovery path (bothload_or_initandload_from_config_path).desktop::app_state::recovery_signal) keeps the signal reported after the loader heals the file on the same boot, andapp_state_snapshotexposes it asconfigRecovered.Problem
Two Windows users'
config.tomlfiles failed UTF-8 validation and generated ~946 Sentry events. #5171 already fixed the flood: the loader rate-limits the error, renames the corrupt file to.corrupted.<ts>, and resets to defaults / a.bakbackup. But that recovery was silent — the user was never told their settings had been reset, which was requirement (3) of the issue.Delivering the notice is non-trivial because recovery happens at core boot, before the frontend socket connects, so a live
core_notificationpublish would be lost. The persisted-notification path (#3805) is also not wired to a frontend sync-down, so persistence alone wouldn't surface it either.Solution
A pull-based, race-free signal rather than a push:
Config::recovered_from_corruption(#[serde(skip)], runtime-only) is set in the loader's recovery branches. Per-load and pollution-free, so both the "recovered" and "clean" cases are unit-tested directly.desktop::app_state::recovery_signalholds a process latch.bootstrap_core_runtimecallslatch_from_config(&cfg)once at boot from the authoritative boot config; the latch bridges the gap that the per-load flag readsfalseon subsequent (now-healed) loads.app_state_snapshot— whichBootCheckGate/CoreStateProvideralready fetch on boot — reportsconfigRecovered. No new poller, no new RPC.CoreStateProvider.refreshCorecalls a one-shotmaybeSurfaceConfigRecovery, dispatching one System notification into the existing notification center (respects category prefs; deduped by stable id; guarded so repeated polls don't re-fire).Design note: a notification-bus approach was considered but rejected — it would have required activating the dormant #3805 sync-down, which on first boot would dump the entire backlog of previously-persisted-but-unread core notifications. The snapshot-flag approach is strictly scoped to this issue.
Submission Checklist
load_or_initandload_from_config_path, and does not set it for a valid config;recovery_signallatch unit tests; frontend one-shot dispatch test (surfaces once, no-op on repeat / when false).app_state_snapshotbuilder (app_state_credentials_raw_coverage_e2e.rs,json_rpc_e2e.rs) and the core-runtime bootstrap path.N/A: behaviour-only change to an existing surface; no new feature ID.## Related—N/A: none.N/A: no new release-cut surface (reuses the existing app-state snapshot + notification center).Closes #NNNin## Related.Impact
config.toml), but the mechanism is platform-agnostic. No migration, no config-format change.configRecoveredonapp_state_snapshot; older/newer clients degrade to "no recovery". TheConfigfield is#[serde(skip)]— never persisted.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
wt-5167Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheck(pnpm compile/tsc --noEmit)cargo test --lib(recover / latch / non_utf8 groups) +vitest run src/lib/configRecoveryNotice.test.tscargo fmt --check+cargo clippy -p openhuman -- -D warningscargo clippy --manifest-path app/src-tauri/Cargo.toml -- -D warnings(no Tauri Rust changed)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
config.tomlis auto-reset.Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
.corruptedsuffix when applicable.Documentation
Tests