feat(memory): warn before the embedding budget silently stops memory - #5402
Conversation
When the managed embedding budget ran out, every embed job failed as `unrecoverable` and the Memory Tree stopped ingesting. The only signal was a yellow banner inside a settings panel nobody opens, so a user reported this as "the app has been broken for a month" with 936 unrecoverable failures and a 32-day-stale sync. Most of the machinery already existed and is reused rather than rebuilt: the typed `budget_exhausted` cause, `first_blocking_cause` on the wire, `UserErrorCenter`, `useUsageState`, `UpsellBanner`, and `requeue_failed`. - Warn at 75% (dismissible) and 90% (not) via a shell-mounted banner, gated on embeddings actually billing against the managed budget so BYO-key and local users never see a false alarm. - Escalate the typed cause out of the settings panel into `UserErrorCenter`, which survives route changes, plus a once-per-session OS notification. - Label the state "Paused: embedding budget reached" instead of "Error - 936 unrecoverable failures", derived frontend-side so the wire payload is unchanged for older clients. - Deep-link every CTA to Connections -> Embeddings, where both remediations (local Ollama, own API key) live. - Un-park failed jobs when the user changes their embedding provider or adds a key, at the four provider-change sites. Deliberately not on login, which would be a retry storm. - Report `degraded` when the queue holds eligible work but has not settled any job for 6h. This measures idle time, not backlog depth: a deep-but-draining backfill and deferred (backing-off) work must not be flagged, or the metric misfires on exactly the heavy users this issue is about. The issue cites `rebuild added=0 evicted=0 kept=0` as evidence, but that log is `agent/learning/stability_detector.rs` - agent learning, not the memory pipeline. No `added=0` signal exists, hence the idle-time measure above. Email on exhaustion cannot live in this repo (`src/api/` has no send path and the SDK exposes no route); tracked as tinyhumansai/backend#1211. Closes tinyhumansai#5324
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds managed embedding-budget alerts, memory error routing, provider-change job recovery, effective-provider reporting, and stalled pipeline detection. It also adds localized copy and extensive frontend and backend tests. ChangesEmbedding budget recovery
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant EmbeddingSettings
participant EmbeddingQueue
participant MemoryPipeline
participant MemoryTreeStatusPanel
participant UserErrorCenter
EmbeddingSettings->>EmbeddingQueue: requeue failed jobs after provider change
EmbeddingQueue->>MemoryPipeline: restore ready jobs
MemoryPipeline->>MemoryTreeStatusPanel: report budget or stall status
MemoryTreeStatusPanel->>UserErrorCenter: report actionable budget failure
Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
YellowSnnowmann has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
coderabbitai
Bot
added
bug
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/components/intelligence/MemoryTreeStatusPanel.tsx (1)
348-385: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the resolved blocking cause for all budget behavior.
Line 348 reads only
first_blocking_cause. Line 407 already supportsdegraded.causeas an older payload shape. When onlydegraded.cause.codeisbudget_exhausted, the panel shows neither the budget status label nor the CTA, and it does not report the persistent user error.Derive one
blockingCausebefore the effect. Use itscodefor reporting andisBudgetExhausted. Add a regression test for this payload shape.Proposed fix
- const blockingCauseCode = status?.first_blocking_cause?.code ?? null; + const blockingCause = status?.first_blocking_cause ?? status?.degraded?.cause ?? null; + const blockingCauseCode = blockingCause?.code ?? null; @@ - status?.first_blocking_cause?.code === 'budget_exhausted' && + blockingCause?.code === 'budget_exhausted' && (statusKind === 'error' || statusKind === 'degraded'); @@ - const blockingCause = status?.first_blocking_cause ?? status?.degraded?.cause ?? null;Also applies to: 452-468
🤖 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/components/intelligence/MemoryTreeStatusPanel.tsx` around lines 348 - 385, Resolve a single blockingCause before the useEffect by preferring status.first_blocking_cause and falling back to the legacy degraded.cause payload. Use blockingCause.code for reportMemoryPipelineFailure and isBudgetExhausted so the budget label, CTA, and persistent error reporting work for both payload shapes; add a regression test covering degraded.cause.code === 'budget_exhausted'.
🧹 Nitpick comments (1)
app/src/lib/i18n/pt.ts (1)
7284-7293: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit the locale map to meet the file-size limit.
app/src/lib/i18n/pt.tscontains 7,394 lines. The repository limit for TypeScript files is 500 lines. Move locale groups into focused modules and compose them from the locale entry point.As per coding guidelines, TypeScript files must stay at or below 500 lines.
🤖 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/lib/i18n/pt.ts` around lines 7284 - 7293, Split the oversized Portuguese locale map exported by the locale entry point into focused modules, keeping each TypeScript file at or below 500 lines. Move complete locale groups together, import those modules from pt.ts, and compose the final locale object without changing any translation keys or values.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 `@app/src/hooks/useEmbeddingBudgetState.ts`:
- Around line 105-108: The !hasUsage branch in useEmbeddingBudgetState must
clear the stale provider, set providerLoading to true before the next provider
read, and then return so a new session cannot reuse the previous user’s managed
provider; add a regression test covering sign-out/sign-in from a managed first
session to a local second session.
In `@app/src/lib/i18n/de.ts`:
- Around line 7373-7374: Update the memoryBudget.approachingMessage German
translation to use a space before the percent sign, changing the placeholder
formatting from {pct}% to {pct} % while leaving the surrounding message
unchanged.
In `@app/src/lib/i18n/en.ts`:
- Around line 1213-1215: Split the namespace maps in en.ts into static
translation modules, keeping every *.ts module at or below 500 lines, then
compose those modules into the exported en map. Preserve every existing
translation key and value, including memoryTree.status.statusBudgetExhausted,
without changing the public export shape.
In `@app/src/lib/i18n/fr.ts`:
- Around line 7351-7353: Update the `memoryBudget.approachingTitle` translation
to explicitly refer to the managed embedding budget rather than an embeddings
limit, while leaving the adjacent `memoryBudget.approachingMessage` unchanged.
- Line 6998: Add the missing userErrors.scope.memory translation key to en.ts
and every supported locale file, using the existing scope-label structure and
each locale’s appropriate translation. Ensure UserErrorCenter.tsx can resolve
the dynamically constructed memory scope label without falling back to the raw
scope value.
In `@app/src/lib/i18n/hi.ts`:
- Line 6832: Update the Hindi translation for
memoryTree.status.statusBudgetExhausted to use wording that means “paused”
rather than “stopped,” preserving the recoverable paused-state meaning while
keeping the embedding-budget context.
In `@app/src/lib/i18n/pl.ts`:
- Line 6933: Split the large Polish locale map in pl.ts into chunk modules of no
more than 500 lines, preserving all existing translation keys and values. Keep
pl.ts as a small aggregator that imports and combines the chunks, and place the
section containing memoryTree.status.statusBudgetExhausted in the appropriate
chunk.
- Around line 7273-7275: Add the missing userErrors.scope.workspace translation
key to the locale represented by this file, matching the established scope-label
structure and wording conventions. Ensure the key is present consistently in
every locale’s translation object so UserErrorCenter can resolve
workspace-scoped errors without falling back to the raw scope value.
In `@src/openhuman/config/ops/model.rs`:
- Around line 251-259: Restrict failed-job recovery to embedding remediation: in
src/openhuman/config/ops/model.rs lines 251-259 and 325-332, call
requeue_failed_after_provider_change only when the embedding provider changes;
in src/openhuman/inference/embeddings/rpc.rs lines 370-377, keep recovery
limited to provider remediation while retaining set_api_key for credential
recovery. Add tests confirming unrelated settings saves leave failed jobs
parked.
In `@src/openhuman/memory/queue/ops.rs`:
- Around line 64-84: Update requeue_failed_after_provider_change in ops.rs to
use the required stable domain log format instead of the current [memory::jobs]
messages. Add a debug-level entry log and debug-level outcome logs for each
branch with correlation data for the requeue attempt, and keep the
state-transition and success/failure records in the domain/rpc prefix style
rather than the existing ad hoc wording.
In `@src/openhuman/memory/tree/tree/rpc.rs`:
- Around line 747-752: Update the reference timestamp calculation in the
idle-time function to use the later of last_settled_ms and oldest_eligible_ms,
so newly eligible work starts its idle period when it arrives rather than
inheriting historical idle time. Preserve the existing zero-clamping behavior,
and add a test covering a historical completed job followed by newly eligible
work.
---
Outside diff comments:
In `@app/src/components/intelligence/MemoryTreeStatusPanel.tsx`:
- Around line 348-385: Resolve a single blockingCause before the useEffect by
preferring status.first_blocking_cause and falling back to the legacy
degraded.cause payload. Use blockingCause.code for reportMemoryPipelineFailure
and isBudgetExhausted so the budget label, CTA, and persistent error reporting
work for both payload shapes; add a regression test covering degraded.cause.code
=== 'budget_exhausted'.
---
Nitpick comments:
In `@app/src/lib/i18n/pt.ts`:
- Around line 7284-7293: Split the oversized Portuguese locale map exported by
the locale entry point into focused modules, keeping each TypeScript file at or
below 500 lines. Move complete locale groups together, import those modules from
pt.ts, and compose the final locale object without changing any translation keys
or values.
🪄 Autofix
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: d2a844e7-1e4c-4939-90d1-c48f43b9e7b5
📒 Files selected for processing (32)
app/src/App.tsxapp/src/__tests__/App.webviewOverlay.test.tsxapp/src/components/intelligence/MemoryTreeStatusPanel.test.tsxapp/src/components/intelligence/MemoryTreeStatusPanel.tsxapp/src/components/upsell/MemoryEmbeddingBudgetBanner.tsxapp/src/components/upsell/__tests__/MemoryEmbeddingBudgetBanner.test.tsxapp/src/components/userErrors/UserErrorCenter.tsxapp/src/hooks/__tests__/useEmbeddingBudgetState.test.tsapp/src/hooks/useEmbeddingBudgetState.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/lib/userErrors/__tests__/classify.test.tsapp/src/lib/userErrors/classify.tsapp/src/lib/userErrors/report.tsapp/src/types/userError.tssrc/openhuman/config/ops/model.rssrc/openhuman/inference/embeddings/rpc.rssrc/openhuman/memory/queue/mod.rssrc/openhuman/memory/queue/ops.rssrc/openhuman/memory/tree/tree/rpc.rs
![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: 0031a5d18d
ℹ️ 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".
… scope, budget-banner gaps Review-cycle fixes for the embedding-budget warning PR (tinyhumansai#5324): - queue_idle_ms: use the later of last_settled_ms and oldest_eligible_ms so work enqueued after an idle/empty period starts its own idle window instead of inheriting a stale completion (would have flagged degraded immediately). CodeRabbit + Codex. - Requeue-on-provider-change: gate the failed-job un-park to an actual embedder change in apply_model_settings / apply_memory_settings / embeddings::update_settings; set_api_key stays the unconditional credential path. An unrelated settings save no longer restarts terminal jobs. CodeRabbit. - useEmbeddingBudgetState: gate the read on isAuthenticated, clear stale provider on sign-out, and read the managed budget directly when embeddings are managed but chat is fully routed away. CodeRabbit + Codex. - MemoryTreeStatusPanel: derive blockingCause once (first_blocking_cause ?? degraded.cause) so a legacy degraded.cause-only payload still escalates, labels, and shows the CTA. CodeRabbit. - i18n: add userErrors.scope.workspace to all 14 locales (the scope the memory pipeline failure emits; there is no 'memory' scope); de percent spacing, fr approaching-title, hi paused wording. New tests: idle-time fresh-work regression, requeue-scope gate (model + memory settings), degraded.cause panel shape, session/routed-away hook cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the reviews — worked through every comment in 58370ac. Summary: Correctness
Outside-diff:
|
There was a problem hiding this comment.
YellowSnnowmann has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
The escalation dispatch fires from an effect after the pipeline status
resolves, so asserting `mockDispatch` synchronously was timing-dependent —
it passed locally but flaked under CI's slower coverage run
("expected vi.fn() to be called at least once"). Wrap it in `waitFor`,
matching the sibling first_blocking_cause escalation test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
YellowSnnowmann has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/hooks/useEmbeddingBudgetState.ts (1)
131-217: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd privacy-safe lifecycle diagnostics.
Add grep-friendly diagnostics for the authentication branch, provider read start and result, fallback-read decision, and polling state. Record safe categories and error kinds. Do not pass raw
errvalues to diagnostics.As per coding guidelines, “Add verbose, grep-friendly diagnostics for new or changed flows” and “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 `@app/src/hooks/useEmbeddingBudgetState.ts` around lines 131 - 217, Add privacy-safe, grep-friendly diagnostics around the authentication branch, provider-read start/result, fallback budget-read decision, and managed-provider polling lifecycle in the affected useEffect flows. Log only safe state categories and normalized error kinds; never pass raw err values, credentials, usage payloads, or full user-identifying data to diagnostics, while preserving the existing behavior and error handling.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 `@app/src/hooks/useEmbeddingBudgetState.ts`:
- Around line 160-163: Update the fallback request guard in the effect handling
managed providers in useEmbeddingBudgetState so creditsApi.getTeamUsage() runs
only when !hasUsage and !usageLoading, then include usageLoading in the effect
dependencies. Add a test covering teamUsage: null with isLoading: true and
verify no fallback request starts while the primary usage request is pending.
In `@src/openhuman/config/ops/model.rs`:
- Around line 263-267: The handlers at src/openhuman/config/ops/model.rs lines
263-267 and 352-356 currently collapse requeue failures into a successful zero
count. Update both handlers and requeue_failed_after_provider_change to preserve
a distinct failure result, while allowing the settings save to succeed; then
schedule a durable retry or return a user-visible warning when recovery fails,
and add failure-path coverage for both handlers.
---
Outside diff comments:
In `@app/src/hooks/useEmbeddingBudgetState.ts`:
- Around line 131-217: Add privacy-safe, grep-friendly diagnostics around the
authentication branch, provider-read start/result, fallback budget-read
decision, and managed-provider polling lifecycle in the affected useEffect
flows. Log only safe state categories and normalized error kinds; never pass raw
err values, credentials, usage payloads, or full user-identifying data to
diagnostics, while preserving the existing behavior and error handling.
🪄 Autofix
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: a8f01e60-f942-4fb2-b253-dcb585d2691c
📒 Files selected for processing (23)
app/src/components/intelligence/MemoryTreeStatusPanel.test.tsxapp/src/components/intelligence/MemoryTreeStatusPanel.tsxapp/src/hooks/__tests__/useEmbeddingBudgetState.test.tsapp/src/hooks/useEmbeddingBudgetState.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tssrc/openhuman/config/ops/model.rssrc/openhuman/config/ops_tests.rssrc/openhuman/inference/embeddings/rpc.rssrc/openhuman/memory/queue/ops.rssrc/openhuman/memory/tree/tree/rpc.rs
🚧 Files skipped from review as they are similar to previous changes (19)
- app/src/lib/i18n/it.ts
- src/openhuman/inference/embeddings/rpc.rs
- app/src/lib/i18n/en.ts
- app/src/components/intelligence/MemoryTreeStatusPanel.test.tsx
- app/src/lib/i18n/id.ts
- app/src/components/intelligence/MemoryTreeStatusPanel.tsx
- app/src/lib/i18n/pt.ts
- app/src/lib/i18n/hi.ts
- app/src/lib/i18n/fr.ts
- app/src/lib/i18n/de.ts
- app/src/lib/i18n/es.ts
- app/src/lib/i18n/ru.ts
- app/src/lib/i18n/ko.ts
- app/src/lib/i18n/ar.ts
- app/src/lib/i18n/pl.ts
- app/src/lib/i18n/zh-CN.ts
- app/src/lib/i18n/bn.ts
- src/openhuman/memory/queue/ops.rs
- src/openhuman/memory/tree/tree/rpc.rs
… load Address CodeRabbit's incremental review on 9d3bc8c: - useEmbeddingBudgetState: gate the direct getTeamUsage() fallback on !usageLoading. `teamUsage` is also null while useUsageState's own request is pending, so without this a normal managed user could fire a duplicate billing call. Added `usageLoading` to the effect deps. - requeue_failed_after_provider_change now returns Result<u64, String> instead of collapsing a store failure into 0. All four call sites (both model.rs handlers, embeddings::update_settings, set_api_key) keep the settings save successful but report `requeued_failed=error (...)` instead of a misleading `=0` (the two embeddings RPCs also add a `requeue_error` JSON field). A store failure can no longer make a still-parked queue look remediated. Tests: hook loading-state case (no fallback while primary usage pending); requeue_after_provider_change_surfaces_store_errors (file-as-workspace_dir → Err, not Ok(0)); existing requeue tests updated to the Result return. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
YellowSnnowmann has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
YellowSnnowmann has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Reviewed read-only against upstream/main. The engineering here is strong and I verified rather than took on trust: the four provider-change requeue sites are each gated on an actual embedder change, requeue_failed_after_provider_change returns Err rather than a 0 that would read identically to "nothing parked" (with a real ENOTDIR failure-path test), the queue_idle_ms predicate correctly measures time since the queue last settled and takes max(last_settled, oldest_eligible) so fresh work starts its own window, i18n is complete across en + all 13 locales with no em dashes, and the new JSON fields are additive.
One thing I think blocks it, and it is a false-alarm path rather than a crash.
The managed-embeddings gate reads a field that does not govern the pipeline.
useEmbeddingBudgetState decides whether the user bills against the managed budget from loadEmbeddingsSettings().provider, which is config.memory.embedding_provider (inference/embeddings/rpc.rs:57). That field is not authoritative for whether embeddings run locally, and main already says so in two places:
memory/store/factories.rs:194— "Ollama embedder regardless of whatmemory.embedding_providersays"- a test named
embedding_settings_local_overrides_memory_config, asserting that aSome(local_model)"is the stronger signal and must override it"
The memory-tree embedder's real ladder (memory/tree/score/embed/factory.rs::resolve_embedder_choice) never reads memory.embedding_provider at all — it resolves to local Ollama via a memory_tree.embedding_endpoint override or via workload_local_model("embeddings"), the unified "Memory embeddings" Local AI setting. Neither path rewrites memory.embedding_provider; apply_local_ai_settings sets config.local_ai.usage.embeddings and nothing else.
So a user who enabled local embeddings through Local AI Settings runs fully local and bills nothing against the managed budget, but still reports provider === "cloud" (the default). Once their chat budget crosses 90% they get the non-dismissible, shell-mounted banner on every screen saying memory has stopped growing because the embedding budget is used up, plus a native notification at exhaustion — while their memory is growing fine.
That contradicts the PR body ("Gated on embeddings actually billing against the managed budget, so BYO-key and local-Ollama users never see a false alarm") and the hook's own principle that "a false alarm here would teach users to ignore the real one". The BYO-key half is correct; those paths do change the provider field. The local-via-Local-AI-Settings half is not.
CI is green because the managed-provider gate tests only exercise slug strings.
The fix looks cheap: the core already resolves this (EmbedderChoice / workload_local_model("embeddings")), so exposing the effective embedder on embeddings.get_settings as an additive field and gating on that would close it, and the existing gate tests would still pass as written.
Happy to re-review as soon as that is addressed — everything else in this PR reads as ready.
…budget-warnings # Conflicts: # app/src/lib/i18n/ar.ts # app/src/lib/i18n/bn.ts # app/src/lib/i18n/de.ts # app/src/lib/i18n/en.ts # app/src/lib/i18n/es.ts # app/src/lib/i18n/fr.ts # app/src/lib/i18n/hi.ts # app/src/lib/i18n/id.ts # app/src/lib/i18n/it.ts # app/src/lib/i18n/ko.ts # app/src/lib/i18n/pl.ts # app/src/lib/i18n/pt.ts # app/src/lib/i18n/ru.ts # app/src/lib/i18n/zh-CN.ts # app/src/types/userError.ts
…he setting
`useEmbeddingBudgetState` decided whether a user bills against the managed
budget from `loadEmbeddingsSettings().provider`, i.e.
`config.memory.embedding_provider`. That field is not authoritative for how
embeddings are funded: the memory-tree ladder resolves local Ollama from
`memory_tree.embedding_endpoint` or from the unified
`workload_local_model("embeddings")` setting (the "Memory embeddings" toggle in
Local AI Settings), and neither path rewrites it.
So a user who enabled local embeddings that way runs fully local and bills
nothing — but still reads `provider === "cloud"`. Once their *chat* budget
crossed 90% they got the non-dismissible, shell-mounted "memory has stopped
growing" banner on every screen, plus a native notification at exhaustion,
while their memory was growing fine. That is exactly the false alarm this
feature set out to avoid. The BYO-key half was already correct — those paths do
change the provider field.
- `effective_embedder_slug(config)` walks the same `resolve_embedder_choice`
ladder both factories walk, so it cannot drift from real resolution. Maps to
stable wire slugs: ollama / custom / cloud / none / unconfigured / unknown.
A ladder error reports `unknown`, never `cloud` — an unresolvable config must
not manufacture a warning.
- `embeddings.get_settings` gains an additive `effective_provider` field.
- The hook gates on `effective_provider ?? provider`, so an older core degrades
to the previous behaviour instead of throwing.
Also adds the privacy-safe lifecycle diagnostics CodeRabbit asked for
(auth branch, provider read start/result, fallback-read decision, polling
on/off). Error logs carry a normalized kind, never the raw error, which can
quote endpoints or backend messages.
Tests: 6 ladder cases in the embed factory (incl. the local-overrides-cloud
regression), an RPC-level assertion that the two fields can disagree, and 4
hook cases covering both gate directions, `unconfigured`, and the missing-field
fallback.
Refs tinyhumansai#5324
There was a problem hiding this comment.
YellowSnnowmann has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
| setFallbackUsage(null); | ||
| setProviderLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fixed in 2c348a9 — added the privacy-safe lifecycle diagnostics.
Each branch now logs under the [embedding-budget] grep prefix: the auth branch (skip: not authenticated), provider read start (with hasUsage/usageLoading), provider read result (effective=… configured=… managed=…), the fallback-read decision in all three shapes (start / not-needed-with-reasons / session-expired), and the polling lifecycle (polling on every 60000ms / polling off / polling stopped).
Raw err values are never passed to a log. Both failure paths go through a new errorKind(err) helper that emits core_rpc:<kind> for our typed CoreRpcError, error:<name> for a plain Error, and unknown otherwise — no message, no endpoint, no usage payload, no user-identifying data. The only categories logged are provider slugs, booleans, and the poll interval.
|
@M3gA-Mind — you're right, and the diagnosis was exact. Fixed in 2c348a9. I checked the claim rather than taking it: The fix is the one you sketched — expose the effective embedder additively and gate on that:
Two judgement calls worth flagging:
Coverage, in the layers where each failure could recur:
As you predicted, the existing gate tests passed unchanged. Also in this push: the privacy-safe lifecycle diagnostics CodeRabbit asked for (replied on that thread), and a merge of Ready for your re-review. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/inference/embeddings/rpc.rs`:
- Around line 1091-1121: Add an endpoint-level assertion in
tests/embeddings_rpc_e2e.rs for the openhuman.embeddings_get_settings JSON-RPC
call, verifying the serialized response includes the effective_provider field
with the expected value. Keep the existing handler-level test
get_settings_reports_effective_provider_separately_from_the_setting unchanged.
In `@src/openhuman/memory/tree/score/embed/factory.rs`:
- Around line 282-286: Update the error handling around
effective_embedder_slug’s ladder resolution to redact sensitive endpoint data
before logging the resolve_embedder_choice() error. Apply redact_endpoint() to
custom endpoints represented in the error chain, ensuring any added
with_context() wrapping occurs only after redaction or otherwise preserves a
sanitized diagnostic; keep the existing “unknown” fallback behavior.
🪄 Autofix
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: b787ba1b-674b-44a0-964c-3e82146a09ff
📒 Files selected for processing (23)
app/src/hooks/__tests__/useEmbeddingBudgetState.test.tsapp/src/hooks/useEmbeddingBudgetState.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/lib/userErrors/__tests__/classify.test.tsapp/src/lib/userErrors/classify.tsapp/src/services/api/embeddingsApi.tsapp/src/types/userError.tssrc/openhuman/inference/embeddings/rpc.rssrc/openhuman/memory/tree/score/embed/factory.rssrc/openhuman/memory/tree/score/embed/mod.rs
🚧 Files skipped from review as they are similar to previous changes (18)
- app/src/lib/userErrors/classify.ts
- app/src/lib/i18n/fr.ts
- app/src/lib/i18n/ko.ts
- app/src/lib/i18n/id.ts
- app/src/lib/i18n/pl.ts
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/bn.ts
- app/src/lib/i18n/zh-CN.ts
- app/src/lib/i18n/it.ts
- app/src/lib/i18n/pt.ts
- app/src/types/userError.ts
- app/src/lib/i18n/es.ts
- app/src/lib/i18n/ar.ts
- app/src/hooks/useEmbeddingBudgetState.ts
- app/src/lib/i18n/ru.ts
- app/src/lib/i18n/de.ts
- app/src/lib/i18n/hi.ts
- app/src/lib/userErrors/tests/classify.test.ts
…ld on the wire
Two CodeRabbit findings on the effective-embedder change.
Endpoint leakage (CWE-532): the ladder error quotes
`memory.embedding_provider` verbatim, and in its `custom:<url>` form that
string is a full endpoint URL which may carry `user:pass@` userinfo. Configured
`cloud_providers` endpoints can reach the message the same way through the
underlying constructor's context. `redact_ladder_error` now replaces each known
endpoint substring with its `redact_endpoint` form, keeping the host so the
line stays diagnosable and keeping the reason ("dimension mismatch") intact.
Scrubbing the exact strings we already hold is precise; a generic URL-matching
pass over free text would be guesswork.
E2E: the handler-level test proved `effective_provider` exists on the return
value, not that it survives serialization. The frontend gate reads this field
off the wire and silently falls back to `provider` — the stale value behind the
original false alarm — if it stops being emitted, so
`embeddings_get_settings_returns_catalog` now asserts it is present and is one
of the documented slugs.
The redaction test asserts the unredacted error really does carry the
credentials before asserting they are gone, so it cannot pass vacuously.
Refs tinyhumansai#5324
There was a problem hiding this comment.
YellowSnnowmann has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
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/openhuman/memory/tree/score/embed/factory.rs`:
- Around line 267-291: Update redact_ladder_error to collect the inline custom
endpoint and configured cloud-provider endpoints, sort them by descending
length, then scrub them in that order so longer values are redacted before
strict-prefix shorter values. Add a regression test covering prefix-overlapping
endpoints and verify the resulting warning contains neither endpoint’s
unredacted suffix.
🪄 Autofix
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: 30c6b82d-d495-495a-8e8b-1817dd342f1f
📒 Files selected for processing (2)
src/openhuman/memory/tree/score/embed/factory.rstests/embeddings_rpc_e2e.rs
…e rest Substring replacement is order-sensitive. When a short configured endpoint is a strict prefix of a longer one — `https://embed.example.com` next to `https://embed.example.com/v1?key=…` — scrubbing the short one first rewrites the long one's prefix, its own replacement then finds no match, and the credential-bearing suffix survives in the warning log. Collect every candidate endpoint, sort by descending length, then replace. The regression test was verified to fail without the sort: it leaves `endpoint='embed.example.com/v1?key=super-secret'` in the rendered message. It drives `redact_ladder_error` with a synthesized error rather than the ladder, because the property at risk is the function's ordering contract for any message carrying both endpoints — not which branch happens to surface a `cloud_providers` endpoint today. Refs tinyhumansai#5324
There was a problem hiding this comment.
YellowSnnowmann has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…onflict main gained `redact_ladder_error` + `effective_embedder_slug` and nine new tests appended at the end of `mod tests` (tinyhumansai#5402). This branch also appends its five `resolved_embedder` tests at the same anchor, so the two insertions collide and the PR reports a conflict. Move the `resolved_embedder` test block from the end of `mod tests` to just after `none_provider_returns_inert`, well clear of main's insertion point. Test order is irrelevant in Rust and no test body, name or assertion is changed - the file's set of lines is identical, only their position moves. Verified by 3-way merge against merge-base 210cd6c: 0 conflict regions, 0 lines dropped from either side.
2 participants
Summary
When the managed embedding budget runs out, every embed job fails as
unrecoverableand the Memory Tree silently stops ingesting. The only visible signal was a yellow banner inside a settings panel nobody opens, so the reporting user experienced it as "the app has been broken for a month" (936 unrecoverable failures, sync 32 days stale).Most of the machinery for this already existed, so this PR reuses rather than rebuilds: the typed
budget_exhaustedcause,first_blocking_causeon the wire,UserErrorCenter,useUsageState,UpsellBanner, andrequeue_failed.UserErrorCenter, which survives route changes, plus a once-per-session native OS notification.statusprecedence rules are untouched.memory_tree_pipeline_statusreportsdegradedwhen the queue holds eligible work but has settled nothing for 6h.Three decisions worth a reviewer's attention
1. The issue's evidence is misattributed. It cites
rebuild added=0 evicted=0 kept=0as proof the pipeline produces no output, but that log isagent/learning/stability_detector.rs— agent learning, unrelated to the memory pipeline. There is noadded=0signal to key on, so AC-7 is implemented as the honest equivalent below.2. The stall signal measures idle time, not backlog depth. This distinction is the whole correctness argument, and both naive alternatives misfire on exactly the heavy users this issue is about:
MIN(created_at_ms)overreadyrows —mark_deferredparks a backing-off job by leavingstatus = 'ready'and pushingavailable_at_msforward, so deliberately-asleep work would count as waiting.So the measure is time since the queue last settled any job. A pipeline making progress refreshes
completed_at_mscontinuously regardless of backlog depth.queue_idle_ms_ignores_deep_but_draining_and_deferred_backlogspins both false-positive shapes.3. Scope deviations, both deliberate. The retry requeues all failed jobs rather than only
budget_exhaustedor the last 7 days — scoping either way needs a new filtered query in the vendoredtinycortexsubmodule, and the wider net is bounded (an unrelated failure re-fails once and re-parks, and this only runs on an explicit user config change). And the 75/90% thresholds read the shared managed cycle budget because no separate embedding meter exists — managed embeddings return the identicalUSER_INSUFFICIENT_CREDITSerror as chat.Out of scope
The email-on-exhaustion criterion cannot be implemented here:
src/api/has no send path and the SDK exposes no route. It is implemented backend-side in tinyhumansai/backend#1212, which triggers at the same managed-embeddings refusal and dedupes on a unique(userId, kind, cycleKey)index so 936 failed jobs cannot become 936 emails. The native OS notification in this PR is the client-side half, for users who do have the app open.Acceptance criteria
src/api/has no send path and the SDK exposes no route). Implemented backend-side in tinyhumansai/backend#1212 (closes backend#1211), deduped to at most one email per user per billing cycle; the once-per-session native OS notification in this PR is the client-side half.memory_tree_pipeline_statusreturnsdegradedwhen the pipeline produces no outputTest plan
cargo test --lib memory::— 1388 passedcargo test --lib config::— 649 passedcargo test --lib embeddings— 83 passedpnpm typecheckpnpm lint— 0 warnings in changed filespnpm format:check— Prettier +cargo fmt(core and Tauri)pnpm i18n:check/i18n:english:check— 0 missing, 0 extra, 0 unexpected English across 13 localesNew tests cover the threshold boundaries, the managed-provider gate, dismissal not silencing the next escalation, the provider re-read in both directions, the budget label not overriding a manually-paused tree, requeue idempotency, and the two stall false-positive shapes.
Closes #5324
Summary by CodeRabbit
New Features
Bug Fixes
Localization