fix(threads): keep the conversation store off the async workers (#5156)

[M3gA-Mind]

Summary

• threads_create_new no longer waits behind a starved runtime: every conversation-store call on a request path now runs on tokio's blocking pool (memory_conversations::blocking::*) instead of parking an async worker thread on the store's process-global mutex.
• Same treatment for the two other hot paths that hold that mutex — the chat resume read in web_chat::run_task and the per-turn cross-thread search in agent_memory::memory_loader, whose cold index reads every transcript in the workspace.
• A failed thread create is now recorded once, centrally (thread.createThreadError), so it can no longer escape as an unhandled promise rejection or vanish silently.
• The chat error strip renders it through the existing create_thread_failed code and copy, so the shell's "New chat" / Home actions stop being dead buttons on failure.
• threads/README.md documents the rule (and its one deliberate exception) so the sync entry points don't creep back into request paths.

Problem

Sentry TAURI-REACT-10 (#5156), 18 events / 3 users, openhuman@0.61.8:

UnhandledRejection: Non-Error promise rejection captured with value:
Core RPC openhuman.threads_create_new timed out after 30000ms

Two independent defects meet here.

1. The create was not slow — the executor was starved

Every tinycortex::memory::conversations entry point is synchronous. Each one takes a process-global parking_lot::Mutex and then does fsync'd JSONL file IO while holding it:

• thread_index_unlocked folds the whole of threads.jsonl on nearly every operation, and that file gains ~2 lines per appended message and is never compacted — so the per-call cost grows with the user's entire history;
• append_message performs two fsync'd appends under the lock, and update_message reads and rewrites a thread's whole message log under it, so a live streaming turn takes the lock over and over;
• search_cross_thread_messages reads every thread's transcript on a cold index.

threads/ops.rs called those entry points inline from its async fn handlers. parking_lot::Mutex::lock() is a blocking lock, so each call parks a tokio worker thread for the whole wait — and so did web_chat::run_task and the per-turn memory loader. Once more conversation operations are queued than there are worker threads, the runtime stops polling anything, including the HTTP task that owes the client its response.

So a create that needs one append can blow a 30 s budget while doing nothing: it is waiting for a worker to be free to poll it. That also explains the shape of the report — rare (3 users), and concentrated on users with enough history to make each fold expensive.

2. The failure had nowhere to go

Nothing in the store observed createNewThread.rejected. Handling was therefore per-call-site, and the two shell entry points diverged in opposite failure modes:

• useHomeNav did .catch(() => {}) — Home did nothing, said nothing;
• any site that forgot a .catch leaked the rejection.

And .unwrap() throws the rejectWithValue payload — a bare string, not an Error — which is exactly why Sentry recorded a "Non-Error promise rejection" with no app frames. The same class was fixed once before for threads_list (formatThreadLoadError, OPENHUMAN-REACT-X); this is that fix's missing sibling.

Solution

Core: memory_conversations::blocking

A thin module of spawn_blocking wrappers (ensure_thread, list_threads, get_messages, append_message, update_message, update_thread_title, update_thread_labels, delete_thread, purge_threads, search_cross_thread_messages). All 15 store calls in threads/ops.rs plus the two other async hot paths go through them.

The store is exactly as serialized as before — its lock still decides who writes when. What changes is where the wait happens: on the blocking pool instead of on a scarce async worker. The executor stays live, so the RPC server keeps answering while a slow conversation operation drains, and a queued create completes as soon as the lock frees rather than after the client has given up. A JoinError is surfaced as a store error, so a pool panic can't reach the dispatcher as an unknown failure.

welcome_migration.rs deliberately stays synchronous: it is a one-shot, marker-guarded boot migration, not a request path.

Frontend: one recorded outcome per create

threadSlice gains createThreadError, set on createNewThread.rejected and cleared on pending / fulfilled / explicit dismissal. formatThreadCreateError normalises every throw shape (rejectWithValue string, Error, SerializedError) — including the nested loadThreads().unwrap() failure inside the thunk, which the old error instanceof Error check flattened into a generic message. Conversations renders it via the exported deriveChatErrorBanner (send error wins; create failure otherwise), and both shell hooks log a normalised message instead of swallowing.

Impact

• Desktop / core (Rust) + renderer. No RPC signature, schema, or storage-format change; no migration.
• Behaviour: slow conversation operations no longer stall unrelated RPCs; a failed thread create shows a message above the composer instead of doing nothing or raising an unhandled rejection.
• Not changed on purpose: the 30 s client budget stays as-is — raising it would hide latency rather than fix it.
• Known follow-up: threads.jsonl's unbounded growth and per-call refold is the underlying latency driver, but it lives in the vendored tinycortex submodule; fixing it needs its own PR there plus a submodule bump. This PR removes the starvation, not the fold.

Submission Checklist

• Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
• Diff coverage ≥ 80% — changed lines are covered by the new Rust (memory_conversations::blocking) and Vitest suites listed below; pnpm typecheck and the targeted Vitest runs are green locally, Rust compiles in CI
• Coverage matrix updated — N/A: behaviour-only change (no feature row added, removed, or renamed)
• All affected feature IDs from the matrix are listed in the PR description under ## Related — N/A: no matrix feature IDs affected
• No new external network dependencies introduced (mock policy unaffected — the new tests use TempDir workspaces and the existing threadApi mock)
• Manual smoke checklist updated if this touches release-cut surfaces — N/A: does not touch release-cut surfaces
• Linked issue closed via Closes #NNN in the ## Related section

Tests:

• memory_conversations::blocking — create/append/read round-trip; store errors pass through verbatim (a missing thread still yields the store's own not found, so the RPC layer's thread-scoped error mapping keeps working); title/labels/delete/purge round-trip; update_message + cross-thread search; and 8 concurrent operations on a worker_threads = 1 runtime completing while a purely cooperative task keeps being polled — the starvation shape from the report.
• threadSlice.createThread.test.ts — the RPC timeout is recorded in state; .unwrap() rejects catchably and normalises back to the original message; the failure clears on retry, on success, and on dismissal; a failure originating in the follow-up loadThreads is recorded with its own message; formatThreadCreateError covers string / Error / SerializedError / message-less shapes.
• Conversations.test.tsx — deriveChatErrorBanner precedence: create failure surfaces with the create_thread_failed code, a live send error wins over a stale create failure, neither present renders nothing.

Related

• Closes: UnhandledRejection: Core RPC threads_create_new timed out after 30s #5156
• Context: OPENHUMAN-REACT-X (formatThreadLoadError, the same non-Error rejection class fixed earlier for threads_list); tinycortex issue memory_conversations: first cross-thread search holds both locks during JSONL rebuild #2849 (the cold-index lock work this builds on)
• Follow-up PR(s)/TODOs: compact / incrementally index threads.jsonl in tinycortex so the per-operation fold stops scaling with total history

---

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

• Key: N/A
• URL: N/A

Commit & Branch

• Branch: fix/5156-rpc-threads-create-timeout
• Commit SHA: 8757eeecf

Validation Run

• pnpm --filter openhuman-app format:check — Prettier clean on every touched file
• pnpm typecheck — clean
• Focused tests: threadSlice.createThread.test.ts (8), pages/__tests__/Conversations.test.tsx (15), src/components/layout/shell + store/__tests__/threadSlice.test.ts (92), src/features/conversations (301) — all passing
• Rust fmt/check (if changed): cargo fmt --check clean; compilation + the new blocking suite run in CI
• Tauri fmt/check (if changed): N/A — no app/src-tauri change

Validation Blocked

• command: N/A
• error: N/A
• impact: N/A

Behavior Changes

• Intended behavior change: conversation-store work no longer blocks the async runtime, and a failed thread create is recorded and displayed instead of being dropped or leaked as an unhandled rejection.
• User-visible effect: "New chat" / Home no longer fail silently — the chat view shows "Couldn't create a new thread. Please try again."; unrelated RPCs stay responsive while a slow conversation operation drains.

Parity Contract

• Legacy behavior preserved: identical store semantics and serialization (same lock, same order, same errors verbatim); RPC signatures, schemas, and the JSONL format are untouched; welcome_migration intentionally keeps its synchronous boot-time path.
• Guard/fallback/dispatch parity checks: ThreadsError::from_thread_scoped_store_error still receives the store's own error text (pinned by a test); the create-failure banner reuses the existing create_thread_failed code and copy rather than introducing a new surface.

Duplicate / Superseded PR Handling

• Duplicate PR(s): none
• Canonical PR: this one
• Resolution (closed/superseded/updated): N/A

Summary by CodeRabbit

• New Features
  • Added clear, actionable chat error banners when starting a new chat fails, with consistent dismissal/clearing behavior.
• Bug Fixes
  • Improved responsiveness for conversation and cross-chat search by preventing storage work from blocking chat processing.
  • Reduced risk of RPC timeouts during thread/message operations and improved resilience to overlapping thread creation attempts.
• Tests
  • Expanded coverage for chat error banner precedence and create-failure formatting, concurrent create overlap handling, and destructive-operation lifecycle behavior.

[greptile-apps]

M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

[coderabbitai]

📝 Walkthrough

Walkthrough

The frontend now normalizes and displays thread-creation failures, while Rust conversation-store operations run through Tokio’s blocking pool. Tests cover error state transitions, UI precedence, persistence wrappers, scheduling, CRUD behavior, and search.

Changes

Chat error surfacing

Layer / File(s)	Summary
Thread error state and normalization app/src/store/threadSlice.ts, app/src/store/__tests__/threadSlice.createThread.test.ts	Thread-creation failures are normalized, stored through thunk lifecycle actions, cleared on retry/success/dismissal, and covered by regression tests.
Chat error rendering and hook logging app/src/features/conversations/Conversations.tsx, app/src/components/layout/shell/useHomeNav.ts, app/src/components/layout/shell/useNewChat.ts, app/src/pages/__tests__/Conversations.test.tsx, app/src/components/layout/shell/*.test.tsx	The composer derives a displayed create-thread error with send-error precedence, clears it on input or dismissal, and navigation hooks log formatted failures.

Blocking conversation storage

Layer / File(s)	Summary
Blocking conversation-store API src/openhuman/memory_conversations/blocking.rs, src/openhuman/memory_conversations/mod.rs	Conversation CRUD, metadata, deletion, purge, and cross-thread search operations are exposed through spawn_blocking wrappers.
Blocking-store behavior tests src/openhuman/memory_conversations/blocking_tests.rs	Tokio tests cover round trips, error propagation, single-worker scheduling, updates, deletion, purge, and search.
Thread handler migration src/openhuman/threads/ops.rs, src/openhuman/threads/README.md	Thread RPC handlers and title-generation fallbacks await blocking conversation-store operations; persistence guidance documents the required API.
Memory and resume migration src/openhuman/agent_memory/memory_loader.rs, src/openhuman/web_chat/run_task.rs	Workspace memory search and cold-boot resume loading use blocking conversation-store wrappers with owned arguments.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested labels: rust-core, bug, memory

Suggested reviewers: sanil-23

Poem

I’m a rabbit with logs in my den,
Errors now speak clearly again.
Threads hop off the main pool,
While tests keep the runtime cool.
Thump, thump—cleaner chats begin!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and matches the main change: moving conversation-store work off async workers for #5156.
Linked Issues check	✅ Passed	The changes address #5156 by moving blocking store work off Tokio workers and surfacing normalized thread-create failures to users.
Out of Scope Changes check	✅ Passed	The added wrappers, UI error handling, and tests all support the same blocking-store/thread-create fix and don't appear unrelated.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8757eeecf8

ℹ️ 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]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/openhuman/threads/ops.rs (1)

136-154: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Add handler-level regression coverage for the blocking migration.

No tests accompany these changed RPC paths. Add ops_tests.rs coverage for successful wrapper-backed operations and store-error propagation, especially thread_create_new and fallback-title persistence; wrapper-only tests would not catch a handler reverting to a synchronous store call.

As per coding guidelines, “Untested code is incomplete; add tests for new or changed behavior.”

Also applies to: 179-221, 235-240, 259-277, 283-293, 302-459, 481-491, 505-519, 534-546, 555-564, 611-615

🤖 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/threads/ops.rs` around lines 136 - 154, Add handler-level
regression tests in ops_tests.rs for the changed RPC paths, including successful
wrapper-backed operations and propagation of store errors. Cover
thread_create_new and update_thread_with_fallback_title, verifying fallback
titles are persisted, and exercise the other listed handlers so tests invoke
handlers rather than only testing storage wrappers.

Source: Coding guidelines

app/src/store/threadSlice.ts (1)

116-123: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Do not report a successful create as a create failure.

Once threadApi.createNewThread() resolves, a later loadThreads() failure rejects the whole operation. Callers discard the returned thread, show a “couldn’t create” banner, and a retry can persist duplicate empty threads.

• app/src/store/threadSlice.ts#L116-L123: preserve the successfully created thread and make refresh failure non-fatal, or expose it as a distinct reload failure.
• app/src/store/__tests__/threadSlice.createThread.test.ts#L125-L141: update the test to assert the successful-create behavior while separately validating refresh-error handling.

🤖 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/store/threadSlice.ts` around lines 116 - 123, Update the
create-thread flow around threadApi.createNewThread and loadThreads so a
successful creation always returns the created thread even when refresh fails;
handle the reload error separately rather than passing it through
formatThreadCreateError as a create failure. In app/src/store/threadSlice.ts
lines 116-123, preserve the successful-create result and use the existing
reload-error mechanism if available. In
app/src/store/__tests__/threadSlice.createThread.test.ts lines 125-141, assert
that creation succeeds with the returned thread while validating refresh-error
handling separately.

🤖 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/store/threadSlice.ts`:
- Around line 472-482: Track the latest createNewThread requestId in the thread
state when its pending action starts, and in the fulfilled and rejected handlers
only clear or set createThreadError when action.meta.requestId matches that
latest request. Preserve the current error formatting for the active rejection,
and add a test covering an older request rejecting after a newer request
succeeds so the error remains clear.
- Around line 102-109: Update formatThreadCreateError to return error.message
only when it is a non-empty, trimmed string, matching the existing string
validation; otherwise fall back to 'Failed to create thread'. Add a regression
test covering new Error() and verify the rejected reducer preserves a truthy
failure message for deriveChatErrorBanner.

In `@src/openhuman/memory_conversations/blocking_tests.rs`:
- Around line 85-130: Make
concurrent_operations_complete_without_stalling_the_single_async_worker
deterministic by adding a deliberately blocked store run closure and
synchronization signaling when the ticker makes progress. Await the ticker’s
progress signal while the closure remains blocked, and wrap that wait in a
bounded timeout so inline store execution fails; release the closure afterward
and preserve the existing concurrent thread assertions.

---

Outside diff comments:
In `@app/src/store/threadSlice.ts`:
- Around line 116-123: Update the create-thread flow around
threadApi.createNewThread and loadThreads so a successful creation always
returns the created thread even when refresh fails; handle the reload error
separately rather than passing it through formatThreadCreateError as a create
failure. In app/src/store/threadSlice.ts lines 116-123, preserve the
successful-create result and use the existing reload-error mechanism if
available. In app/src/store/__tests__/threadSlice.createThread.test.ts lines
125-141, assert that creation succeeds with the returned thread while validating
refresh-error handling separately.

In `@src/openhuman/threads/ops.rs`:
- Around line 136-154: Add handler-level regression tests in ops_tests.rs for
the changed RPC paths, including successful wrapper-backed operations and
propagation of store errors. Cover thread_create_new and
update_thread_with_fallback_title, verifying fallback titles are persisted, and
exercise the other listed handlers so tests invoke handlers rather than only
testing storage wrappers.

🪄 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: c0ecd34c-e278-4b4d-889e-e2a22eafff05

📥 Commits

Reviewing files that changed from the base of the PR and between bb83836 and 8757eee.

📒 Files selected for processing (15)

• app/src/components/layout/shell/useHomeNav.test.tsx
• app/src/components/layout/shell/useHomeNav.ts
• app/src/components/layout/shell/useNewChat.test.tsx
• app/src/components/layout/shell/useNewChat.ts
• app/src/features/conversations/Conversations.tsx
• app/src/pages/__tests__/Conversations.test.tsx
• app/src/store/__tests__/threadSlice.createThread.test.ts
• app/src/store/threadSlice.ts
• src/openhuman/agent_memory/memory_loader.rs
• src/openhuman/memory_conversations/blocking.rs
• src/openhuman/memory_conversations/blocking_tests.rs
• src/openhuman/memory_conversations/mod.rs
• src/openhuman/threads/README.md
• src/openhuman/threads/ops.rs
• src/openhuman/web_chat/run_task.rs

[greptile-apps]

M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/openhuman/memory_conversations/blocking_tests.rs (1)

183-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add a note that the synchronous watchdog.join() depends on the multi_thread flavor.

The blocking join() runs on the block_on thread, so the single worker stays free to poll the ticker/probe. That correctness is entirely load-bearing on flavor = "multi_thread"; switching to current_thread would deadlock for 10s and then fail with a misleading message. A one-line comment (or a debug_assert-style remark) tying the join to the flavor choice would protect the invariant.

🤖 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/memory_conversations/blocking_tests.rs` around lines 183 - 199,
Add a concise comment immediately before watchdog.join() explaining that the
synchronous join relies on the test’s multi_thread runtime flavor, because the
block_on worker must remain available to poll the ticker and probe. Tie the note
to the existing runtime flavor configuration without changing the test flow.

src/openhuman/threads/ops_tests.rs (1)

728-734: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider covering the CoreContext preservation branch too.

This test exercises the cancellation contract but only the None context path. The stated motivation for capturing CoreContext::current() is that a bare tokio::spawn would silently fall back to the process-default workspace — that regression would still pass here. A second case that sets a context via CoreContext::scope, calls run_to_completion, and asserts the inner future observes the same workspace would lock in the multi-tenant behavior.

As per coding guidelines, "Untested code is incomplete; add tests for new or changed behavior."

🤖 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/threads/ops_tests.rs` around lines 728 - 734, Extend the
cancellation test around run_to_completion with a CoreContext::scope case that
installs a non-default workspace, runs an inner future through
run_to_completion, and asserts the future observes that same workspace. Keep the
existing cleanup and cancellation assertions intact, and specifically verify
context propagation rather than only the None-context path.

Source: Coding guidelines

🤖 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/openhuman/threads/ops.rs`:
- Around line 102-123: Update run_to_completion so the spawned task logs any Err
returned by fut before propagating it, ensuring failures remain visible when the
caller drops the JoinHandle. Keep the existing context scoping and join-error
handling, but distinguish panic-induced join failures from ordinary task
failures in the warning rather than presenting a panic as a generic join fault.

---

Nitpick comments:
In `@src/openhuman/memory_conversations/blocking_tests.rs`:
- Around line 183-199: Add a concise comment immediately before watchdog.join()
explaining that the synchronous join relies on the test’s multi_thread runtime
flavor, because the block_on worker must remain available to poll the ticker and
probe. Tie the note to the existing runtime flavor configuration without
changing the test flow.

In `@src/openhuman/threads/ops_tests.rs`:
- Around line 728-734: Extend the cancellation test around run_to_completion
with a CoreContext::scope case that installs a non-default workspace, runs an
inner future through run_to_completion, and asserts the future observes that
same workspace. Keep the existing cleanup and cancellation assertions intact,
and specifically verify context propagation rather than only the None-context
path.

🪄 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: 4859a2cf-d529-4b1b-8e84-7333f0bcdbd7

📥 Commits

Reviewing files that changed from the base of the PR and between 8757eee and 9f67002.

📒 Files selected for processing (5)

• app/src/store/__tests__/threadSlice.createThread.test.ts
• app/src/store/threadSlice.ts
• src/openhuman/memory_conversations/blocking_tests.rs
• src/openhuman/threads/ops.rs
• src/openhuman/threads/ops_tests.rs