fix(tauri): own reset_local_data lifecycle in shell (OPENHUMAN-TAURI-AF)#1769
Merged
senamakel merged 3 commits intoMay 16, 2026
Merged
Conversation
`openhuman.config_reset_local_data` ran the directory remove *inside*
the core's tokio task, which holds open handles to SQLite databases,
log files, the Sentry session store, etc. inside that very directory.
On Windows that hit `ERROR_SHARING_VIOLATION` (os error 32) and the
user's "Reset Local Data" click did nothing — see OPENHUMAN-TAURI-AF.
New flow, owned end-to-end by the Tauri shell:
1. `config_get_data_paths` RPC (new, read-only) resolves the current
workspace dir, the default `~/.openhuman`, and the active
workspace marker while the core is still up — the core stays
authoritative for path resolution (loaded config + staging suffix
+ workspace marker).
2. Acquire the existing restart lock to fence against a concurrent
`restart_core_process`.
3. `CoreProcessHandle::shutdown` cancels the tokio task; RAII drops
the SQLite pool, log writer, Sentry session — every file handle
in the data dir is released.
4. Tauri host (this process) removes the three paths via
`tokio::fs::remove_dir_all` / `remove_file`. Missing entries are
non-fatal.
5. `ensure_running` restarts the embedded core.
UI flips from `callCoreRpc('config_reset_local_data') + restartCoreProcess()`
to a single `invoke('reset_local_data')` — atomic from the user's
perspective, no window for partial state to leak.
The old JSON-RPC method stays for headless / CLI callers, with a
docstring spelling out why GUI callers must use the Tauri command.
Test fixes:
- `tauriCommands.test.ts` now stubs `window.__TAURI_INTERNALS__` so the
`common.ts` `isTauri()` wrapper (which also checks the CEF IPC bridge,
not just `coreIsTauri()`) returns true under jsdom. Without this, the
helper's `if (!isTauri()) return;` early-exit fired and every
`mockCallCoreRpc` / `mockInvoke` assertion in the file saw zero
calls — pre-existing breakage that masked anyone editing this test
file.
Contributor
📝 WalkthroughWalkthroughCore exposes a read-only RPC returning resolved data paths; Tauri calls that RPC, stops the embedded core, deletes marker and data directories (treating missing paths as non-fatal), and restarts the core. Frontend now invokes ChangesCore RPC and controller
Tauri command and helpers
Frontend invocation and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/utils/tauriCommands/core.ts (1)
254-269: ⚡ Quick winWrap the new
invoke('reset_local_data')intry/catch.Right now this new Tauri call bubbles failures without any local context. Catch once, log with the command name, then rethrow so callers still see the original error.
As per coding guidelines,
app/src/**/*.{ts,tsx}: Wrap Tauriinvoke()calls intry/catchand useisTauri()fromapp/src/services/webviewAccountService.tsfor Tauri availability checks; do not checkwindow.__TAURI__directly.🤖 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 `@app/src/utils/tauriCommands/core.ts` around lines 254 - 269, The invoke('reset_local_data') call inside resetOpenHumanDataAndRestartCore should be wrapped in a try/catch: keep the existing isTauri() guard, call invoke('reset_local_data') inside try, on catch log the error with the command name (e.g., include "reset_local_data" and the caught error) using the same logging mechanism as the function (console.debug/console.error), then rethrow the original error so callers still receive it; refer to the resetOpenHumanDataAndRestartCore function and the invoke('reset_local_data') call to locate where to add the try/catch.src/openhuman/config/schemas.rs (1)
1059-1061: ⚡ Quick winAdd
[config][rpc]enter/ok/fail logs to the new controller.
handle_get_data_pathsis the new RPC entrypoint for the reset flow, but unlike the adjacent handlers it emits no structured diagnostics. Please wrap it with the same debug/warn logging pattern so reset-path resolution is grep-able when this flow fails in the field.As per coding guidelines,
src/**/*.rs: All new/changed behavior in Rust core must include verbose diagnostics logging with stable grep-friendly prefixes like[domain],[rpc]and correlation fields; never log secrets or full PII.🤖 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/openhuman/config/schemas.rs` around lines 1059 - 1061, The new handler function handle_get_data_paths currently returns the RPC result without any structured diagnostics; wrap its body in the same logging pattern used by adjacent controllers by emitting a “[config][rpc] enter” debug log (including a correlation id if available), then call config_rpc::get_data_paths().await and on success emit a “[config][rpc] ok” debug log with non-PII result metadata, and on error emit a “[config][rpc] fail” warn log that includes the error (but not secrets/PII) before returning the error; preserve the ControllerFuture return (Box::pin(async { ... })) and follow the same log macros/fields used elsewhere in this module for consistency.
🤖 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-tauri/src/lib.rs`:
- Around line 361-367: The reqwest client in lib.rs (where you create
reqwest::Client and call crate::core_rpc::apply_auth on client.post(&url) before
.json(&body).send().await) lacks a timeout and can hang; rebuild the client with
an explicit timeout (e.g., use
reqwest::Client::builder().timeout(Duration::from_secs(10)).build() ) or wrap
the send future with a timeout, and import std::time::Duration; ensure the
resulting client or send call replaces the current client usage so the call to
.send().await will fail fast on timeout.
- Around line 327-342: The delete sequence in reset_local_data currently bails
out on the first delete error and never restarts the embedded core; wrap the
calls to remove_path_if_exists and remove_dir_if_exists into a local Result
(e.g. let delete_result = (|| async { ... })().await) so you capture any error
instead of returning immediately, then always call
state.inner().ensure_running().await in a finally-style path (after awaiting
delete_result) and only then return delete_result.map_err(|e| e) so the original
delete error is preserved while ensuring the core is restarted; reference the
existing remove_path_if_exists, remove_dir_if_exists, and
state.inner().ensure_running() calls in your changes.
In `@app/src/utils/__tests__/tauriCommands.test.ts`:
- Around line 22-30: The test sets window.__TAURI_INTERNALS__ to stub invoke but
never restores it; modify app/src/utils/__tests__/tauriCommands.test.ts to
capture the prior value (const prev = (window as any).__TAURI_INTERNALS__)
before stubbing and add an afterEach hook that restores it (if prev is undefined
delete it, otherwise set it back), so the global jsdom state is not leaked to
other tests; ensure the stub creation and the restore reference the same symbol
__TAURI_INTERNALS__ and run cleanup in afterEach.
---
Nitpick comments:
In `@app/src/utils/tauriCommands/core.ts`:
- Around line 254-269: The invoke('reset_local_data') call inside
resetOpenHumanDataAndRestartCore should be wrapped in a try/catch: keep the
existing isTauri() guard, call invoke('reset_local_data') inside try, on catch
log the error with the command name (e.g., include "reset_local_data" and the
caught error) using the same logging mechanism as the function
(console.debug/console.error), then rethrow the original error so callers still
receive it; refer to the resetOpenHumanDataAndRestartCore function and the
invoke('reset_local_data') call to locate where to add the try/catch.
In `@src/openhuman/config/schemas.rs`:
- Around line 1059-1061: The new handler function handle_get_data_paths
currently returns the RPC result without any structured diagnostics; wrap its
body in the same logging pattern used by adjacent controllers by emitting a
“[config][rpc] enter” debug log (including a correlation id if available), then
call config_rpc::get_data_paths().await and on success emit a “[config][rpc] ok”
debug log with non-PII result metadata, and on error emit a “[config][rpc] fail”
warn log that includes the error (but not secrets/PII) before returning the
error; preserve the ControllerFuture return (Box::pin(async { ... })) and follow
the same log macros/fields used elsewhere in this module for consistency.
🪄 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: 763d1f14-22b6-495d-beb3-dcb040c56463
📒 Files selected for processing (5)
app/src-tauri/src/lib.rsapp/src/utils/__tests__/tauriCommands.test.tsapp/src/utils/tauriCommands/core.tssrc/openhuman/config/ops.rssrc/openhuman/config/schemas.rs
- reqwest client in fetch_data_paths now uses a 10s timeout so a hung
core can't deadlock the destructive flow
- delete + restart sequence in reset_local_data captures the delete
error and always restarts the embedded core, so a failed remove no
longer leaves the user with a dead sidecar
- restore window.__TAURI_INTERNALS__ in tauriCommands test afterEach
- log + rethrow errors from invoke('reset_local_data') in the JS helper
- handle_get_data_paths emits structured enter/ok/fail diagnostics in
line with adjacent config controllers
Diff-cover gate flagged the new try/catch in
`resetOpenHumanDataAndRestartCore` (core.ts:271-272) as uncovered
(60% < 80%). Add a test that rejects `invoke('reset_local_data')`
and asserts the helper rethrows so callers like `clearAllAppData`
keep their unrecoverable-error semantics.
Contributor
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 `@app/src/utils/__tests__/tauriCommands.test.ts`:
- Around line 92-97: The test sets a spy on console.error (consoleErrorSpy) but
only calls mockRestore() after assertions, which can leave the spy active if an
assertion fails; modify the test for resetOpenHumanDataAndRestartCore() to
always restore the spy by wrapping the expect/asserts in a try/finally (or move
the restore to an afterEach) so consoleErrorSpy.mockRestore() runs regardless of
test failures, ensuring global console state is not leaked to other tests.
🪄 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: 074823fc-09a0-41a8-a8b7-898fe5b8ae1f
📒 Files selected for processing (1)
app/src/utils/__tests__/tauriCommands.test.ts
Comment on lines
+92
to
+97
| const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
|
||
| await expect(resetOpenHumanDataAndRestartCore()).rejects.toBe(boom); | ||
| expect(consoleErrorSpy).toHaveBeenCalled(); | ||
|
|
||
| consoleErrorSpy.mockRestore(); |
Contributor
There was a problem hiding this comment.
Always restore console.error even when assertions fail.
If an assertion fails before Line 97, the spy stays active and can pollute later tests.
Suggested fix
const boom = new Error('reset_local_data failed');
mockInvoke.mockRejectedValueOnce(boom);
const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
- await expect(resetOpenHumanDataAndRestartCore()).rejects.toBe(boom);
- expect(consoleErrorSpy).toHaveBeenCalled();
-
- consoleErrorSpy.mockRestore();
+ try {
+ await expect(resetOpenHumanDataAndRestartCore()).rejects.toBe(boom);
+ expect(consoleErrorSpy).toHaveBeenCalled();
+ } finally {
+ consoleErrorSpy.mockRestore();
+ }As per coding guidelines, app/src/**/*.test.{ts,tsx}: “Keep unit tests deterministic: avoid real network calls, time-sensitive flakes, or hidden global state.”
🤖 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 `@app/src/utils/__tests__/tauriCommands.test.ts` around lines 92 - 97, The test
sets a spy on console.error (consoleErrorSpy) but only calls mockRestore() after
assertions, which can leave the spy active if an assertion fails; modify the
test for resetOpenHumanDataAndRestartCore() to always restore the spy by
wrapping the expect/asserts in a try/finally (or move the restore to an
afterEach) so consoleErrorSpy.mockRestore() runs regardless of test failures,
ensuring global console state is not leaked to other tests.
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
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
2 participants
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.
Summary
openhuman.config_reset_local_datarantokio::fs::remove_dir_allinside the core's tokio task — and the running core itself held open handles to SQLite, log files, the Sentry session store, etc. in that very directory. On Windows:ERROR_SHARING_VIOLATION(os error 32) → reset silently fails and the user's "Reset Local Data" click does nothing (OPENHUMAN-TAURI-AF).reset_local_datacommand:openhuman.config_get_data_pathsresolves the current workspace dir, default~/.openhuman, and active workspace marker — core stays authoritative for path resolution.restart_lockto fence against concurrentrestart_core_process.CoreProcessHandle::shutdowncancels the tokio task; RAII drops SQLite pool, log writer, Sentry session — every file handle inside the data dir is released.tokio::fs::remove_dir_all/remove_file. Missing entries are non-fatal.ensure_runningrestarts the embedded core.resetOpenHumanDataAndRestartCoreflips fromcallCoreRpc('config_reset_local_data') + restartCoreProcess()to a singleinvoke('reset_local_data')— atomic from the user's perspective, no window for partial state to leak.Test fix included
app/src/utils/__tests__/tauriCommands.test.tswas pre-existingly broken: it mockscoreIsTaurifrom@tauri-apps/api/core, buttauriCommands/common.ts'sisTauri()wrapper also checkswindow.__TAURI_INTERNALS__.invoketo guard the CEF IPC bootstrap gap (OPENHUMAN-REACT-S). With only the upstream mocked, every helper'sif (!isTauri()) return;early-exited under jsdom and all 4 assertions in the file saw zeromockCallCoreRpc/mockInvokecalls. Stubbingwindow.__TAURI_INTERNALS__inbeforeEachrestores the intended behavior — 4 pre-existing failures unrelated to this PR + 1 new test now all pass.Test plan
cargo checkon core (Cargo.toml) and Tauri shell (app/src-tauri/Cargo.toml) — pass.cargo test --lib config::— 278 pass, including the newget_data_pathspath through schemas + registry.pnpm test --config test/vitest.config.ts src/utils/__tests__/tauriCommands.test.ts— 5 pass (was 1 pass / 4 fail on main).Closes
Fixes OPENHUMAN-TAURI-AF
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Documentation