fix(memory-sources): count chunks_pending from the embedding sidecar, not a dead column - #5344
Conversation
`source_status` derived `chunks_pending` from `mem_tree_chunks.embedding`, a vestigial column that is not in the canonical tinycortex schema and is only added at runtime by `add_column_if_missing`. No production writer ever populates it — embeddings live in the `mem_tree_chunk_embeddings` sidecar keyed by `(chunk_id, model_signature)` — so the column is NULL for every row and `chunks_pending` always equalled `chunks_synced`. Resolve a chunk when it has a vector under the active signature, has a re-embed tombstone for that signature, or was dropped by the admission gate, mirroring `list_sync_statuses` and `has_uncovered_reembed_work`. Refs tinyhumansai#5329
`migrate_legacy_embeddings_to_sidecar` preserves `mem_tree_chunks.embedding` after copying it into the sidecar, so a pre-sidecar vault still carries a usable signal there. Treat a non-NULL legacy blob as resolved alongside the sidecar/tombstone/dropped terms so vaults that predate the sidecar do not regress to "everything pending", and cover it with a dedicated test. Refs tinyhumansai#5329
The doc comments described `chunks_pending` as counting chunks whose `embedding IS NULL` and pointed at the pre-refactor `memory_sources/` path. Both are stale: the counter now resolves chunks against the `mem_tree_chunk_embeddings` sidecar under the active model signature. Comment-only; no behaviour change. Refs tinyhumansai#5329
📝 WalkthroughWalkthroughSource status now counts pending chunks against the active embedding signature and excludes resolved terminal states. Documentation and integration tests were updated to match these semantics. ChangesSource status semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/components/intelligence/sourcePipelineStatus.ts`:
- Around line 15-19: Update both comments around the source status explanation,
including the corresponding block near the second referenced location, to state
that a non-null legacy mem_tree_chunks.embedding blob also counts as resolved.
Preserve the existing descriptions of active-model sidecar vectors and terminal
markers while documenting this legacy embedding resolution 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 Plus
Run ID: c1c7cb9b-a6f0-481e-9f5f-f4b593b637a2
📒 Files selected for processing (2)
app/src/components/intelligence/sourcePipelineStatus.tssrc/openhuman/memory/sources/status.rs
| * of this source's chunks that have no vector in the | ||
| * `mem_tree_chunk_embeddings` sidecar under the active model signature, and | ||
| * no terminal marker (re-embed tombstone / dropped) explaining the absence | ||
| * (see `memory/sources/status.rs`). `> 0` in a settled state means those | ||
| * chunks were stored WITHOUT vectors → semantic search can't reach them. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the legacy embedding resolution path.
source_status also treats a non-null legacy mem_tree_chunks.embedding blob as resolved. These comments describe only active-signature vectors and terminal markers. Update both comments to include legacy pre-sidecar embeddings.
Proposed documentation update
- * no terminal marker (re-embed tombstone / dropped) explaining the absence
+ * no terminal marker (re-embed tombstone / dropped), and no preserved
+ * legacy pre-sidecar embedding blob explaining the absence
- // the active embedding signature) OR the global "semantic recall degraded"
+ // the active embedding signature, excluding terminal and legacy resolved
+ // chunks) OR the global "semantic recall degraded"Also applies to: 82-85
🤖 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/sourcePipelineStatus.ts` around lines 15 -
19, Update both comments around the source status explanation, including the
corresponding block near the second referenced location, to state that a
non-null legacy mem_tree_chunks.embedding blob also counts as resolved. Preserve
the existing descriptions of active-model sidecar vectors and terminal markers
while documenting this legacy embedding resolution path.
|
| Filename | Overview |
|---|---|
| src/openhuman/memory/sources/status.rs | Core fix: rewrites the pending-chunks SQL predicate to read the embedding sidecar instead of the dead embedding column; adds five focused tokio tests covering the regression, signature scoping, terminal states, legacy vaults, and empty-source NULL handling. |
| app/src/components/intelligence/sourcePipelineStatus.ts | Comment-only change: updates the doc block to describe the sidecar-based pending definition; no logic or behaviour is modified. |
Sequence Diagram
sequenceDiagram
participant UI as Data Sync UI
participant BE as source_status (Rust)
participant DB as mem_tree_chunks
participant EC as mem_tree_chunk_embeddings
participant TS as mem_tree_chunk_reembed_skipped
UI->>BE: memory_sources_status_list RPC
BE->>BE: tree_active_signature(cfg)
BE->>DB: "SELECT COUNT(*), SUM(pending CASE), MAX(ts) WHERE source_id LIKE prefix"
DB->>EC: "EXISTS(chunk_id, model_signature=active)?"
DB->>TS: "EXISTS(chunk_id, model_signature=active)?"
DB-->>BE: (synced, pending, last_ts)
BE-->>UI: "SourceStatus { chunks_pending }"
UI->>UI: "deriveSourcePipelineHealth() storedWithoutVectors = pending > 0"
Note over UI: pending==0 on healthy vault → state: retrieval_ready
Comments Outside Diff (1)
-
src/openhuman/memory/sources/status.rs, line 131-153 (link)status_listspawns one blocking task per source sequentiallystatus_listiterates sources withfor source in sourcesandawaits eachsource_statuscall before starting the next. Each call now opens a fresh connection, callstree_active_signature(which opens another connection), and runs the two-EXISTS correlated subquery. For a vault with many sources this is purely sequential with per-source connection overhead.This was pre-existing behaviour, but the new query amplifies the cost slightly. A simple improvement would be to capture
tree_active_signatureonce before the loop and pass it as a parameter (avoiding the repeated call) or tofutures::join_allthe per-source tasks so they run concurrently on the blocking thread pool. Neither is a correctness issue, but worth tracking as a follow-up alongside perf(memory_sources): fold per-source status into one mem_tree_chunks scan #5243.
Reviews (1): Last reviewed commit: "docs(app): correct the chunks_pending se..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn seed(cfg: &Config, source: &str, count: u32) -> Vec<Chunk> { | ||
| let mut chunks = Vec::new(); | ||
| for seq in 0..count { | ||
| let source_id = format!("mem_src:{source}:item-{seq}"); | ||
| chunks.push(chunk(&source_id, seq, 1_700_000_000_000 + i64::from(seq))); | ||
| } | ||
| upsert_chunks(cfg, &chunks).unwrap(); | ||
| chunks | ||
| } |
There was a problem hiding this comment.
seed embeds source prefix in each chunk's source_id, not in the source entry id
Each chunk in seed is created with source_id = format!("mem_src:{source}:item-{seq}"), meaning the full prefix is already baked into the chunk-level source_id. The source_id_prefix for a Folder entry with id = "src_done" is mem_src:src_done:%, which does match mem_src:src_done:item-0 etc., so the tests are correct.
The subtle risk is that a future reader of seed may expect the generated chunk's id (returned from chunk_id(...)) to be keyed on the source entry's id "src_done" — but it is actually keyed on the per-item string "mem_src:src_done:item-0". Adding a brief doc comment to seed explaining this naming convention would prevent confusion, especially since status_of is called with just "src_done" while the chunks carry the longer prefix.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
… scan Re-authored onto the post-tinyhumansai#5328 tree (memory_sources → memory/sources) and rebased on the sidecar-based `chunks_pending` fix (tinyhumansai#5344, cherry-picked as the three preceding commits by @Mustaqeem66) so the batch and the per-source path share one pending definition. `status_list` (polled every 5s by the Memory Sources panel) previously ran one full `mem_tree_chunks` scan per source. It now folds every source's counts into `ceil(N / 128)` scans: one SELECT whose per-source columns are gated by `source_id LIKE ?n`, with the pending column embedding tinyhumansai#5344's exact resolved-predicate (active-signature sidecar vector / re-embed tombstone / dropped lifecycle / legacy blob) inside the gate's `THEN` branch. All reads share the one process-wide chunk-DB connection mutex, so fewer scans — not concurrency — is the only available lever. The nested `CASE` (rather than `LIKE ?n AND NOT(<pred>)`) is deliberate: it reproduces `source_status`'s NULL-predicate → pending behaviour exactly, and because SQL evaluates only the taken branch the `EXISTS` sub-selects run only for a row's own source. If the batched query ever fails, `status_list` falls back to the per-source path, so a query regression degrades latency, never correctness. Equivalence is pinned by a test asserting the batched result matches per-source `source_status` across all five resolution states. Claude-Session: https://claude.ai/code/session_01ACB4Ugi5pJMQqoCbZnVo6f
1 participant
Summary
source_statuscounted pending chunks asSUM(CASE WHEN embedding IS NULL THEN 1 ELSE 0 END)overmem_tree_chunks. Nothing in production writes that column, sochunks_pendingwas always exactly equal tochunks_synced.mem_tree_chunk_embeddingssidecar, scoped to the active model signature, with terminal states (re-embed tombstone,droppedlifecycle) treated as resolved.chunks_pending > 0drivesstored_without_vectorsinderiveSourcePipelineHealth, so every source row with chunks showed "Ingested only" forever, even on a perfectly healthy vault.#[tokio::test]cases instatus.rscovering the regression, signature scoping, terminal states, legacy vaults, and the empty-source path.app/src/components/intelligence/sourcePipelineStatus.ts(comment-only, no behaviour change).Problem
mem_tree_chunks.embeddingis not in the canonical tinycortex schema (vendor/tinycortex/src/memory/chunks/schema.rs). It only exists at runtime becauseapply_schemare-adds it defensively:Production embeddings are written to the sidecar
mem_tree_chunk_embeddings (chunk_id, model_signature, vector, dim, created_at)viaupsert_chunk_embedding_conn. The legacy column therefore stays NULL for everything ingested after the sidecar landed, andpending == syncedfor every source, always — exactly as #5329 reports.The user-visible half of this is worse than the issue states.
app/src/components/intelligence/sourcePipelineStatus.tsdoes:so any source with at least one chunk is permanently pinned to
state: 'ingested_only'with thesync.pipeline.storedWithoutVectorswarning. There is no state a user can reach that clears it.Solution
A chunk counts as resolved (not pending) when any of the following holds:
mem_tree_chunk_embeddingsfor the active signature — the condition Source status reports every chunk as pending: chunks_pending reads a column nothing writes #5329 asks for, and the onehas_uncovered_reembed_workalready uses;mem_tree_chunk_reembed_skippedfor that signature;lifecycle_statusisdropped;embeddingblob.chunks_syncedstaysCOUNT(*)— unchanged, and the frontend only uses it as a> 0gate, so no UI change is needed beyond the warning correctly clearing.Two design decisions worth a reviewer's attention:
(a) Terms 2 and 3 go beyond the issue's literal ask. I took them from
vendor/tinycortex/src/memory/sync/status.rs::list_sync_statuses, which is the sibling implementation of this samechunks_pendingconcept at the provider level and already resolves onembedding-exists OR lifecycle_status='dropped' OR tombstone-exists. Without them the counter still can never drain, because tombstoned and dropped chunks are never embedded — which reproduces the issue's core complaint ("the number is not a signal") in a narrower form. Matching the sibling also means the two surfaces stop disagreeing. Happy to drop them to the bare sidecar check if you'd rather keep this PR minimal — it's a two-line deletion plus one test.(b) Term 4 (legacy column) is deliberate, and it's the one I'd most like a second opinion on.
migrate_legacy_embeddings_to_sidecarexplicitly preserves the legacy column after copying it ("the legacy column is preserved"), so on a pre-sidecar vault a non-NULL blob is still a valid "this was embedded" signal, and dropping the term would regress those vaults to "everything pending" — the very bug being fixed. It is inert for anything ingested after the sidecar landed. The known imperfection: a dim-mismatched legacy blob is skipped by that migration and stranded (documented there as audit finding SC-10), and this term will read it as resolved even though it has no usable vector. I judged that acceptable versus regressing every legacy vault, but I'd defer to a maintainer. Scoping it withlength(c.embedding) = dim * 4would close that hole if you prefer strictness.Keeping term 4 also means
memory_source_status_counts_reader_and_composio_prefixes(which seeds that column directly via raw SQL) keeps passing untouched.Impact
EXISTSsubqueries per row instead of a column read. Both hit the existing(chunk_id, model_signature)primary keys on the sidecar and tombstone tables, so each is a point lookup. This runs once per source instatus_list.SourceStatusis unchanged. Only the value ofchunks_pendingchanges — it can now be lower thanchunks_synced, which is the point.embedding IS NULLas pending" under its Parity Contract, i.e. it preserves this bug by design. If perf(memory_sources): fold per-source status into one mem_tree_chunks scan #5243 lands first this fix needs re-applying to its batched query; the corrected predicate transplants directly since it's expressed in the samemem_tree_chunks cscan. Flagging so the two don't silently cancel out.Related
pending_treats_a_legacy_pre_sidecar_embedding_as_resolved.Submission Checklist
#[tokio::test]cases: the direct regression guard (all embedded →pending == 0, which fails onmain), superseded-signature scoping, tombstoned/dropped resolution, the legacy-vault path, and the empty-sourceSUM/MAX-NULL edge case..tschange is comment-only and contributes no coverable lines. Not verified locally (see Validation Blocked); relying on CI to confirm.N/A: no feature rows added, removed, or renamed; this corrects the value of an existing counter.## Related—N/A: no matrix feature IDs touched.TempDir+Config::default()), no network, no mock backend needed.N/A: no release-cut surface changed; the Data Sync row rendering path is unchanged, only the counter feeding it.Closes #NNNin the## Relatedsection.AI Authored PR Metadata (required for Codex/Linear PRs)
This PR was authored with AI assistance (GitHub Copilot CLI), directed and reviewed by me. Disclosed here because this section is mandatory in the template. Every claim below is what I actually did or actually could not do — nothing is marked as run that wasn't.
Linear Issue
N/A— external contribution, tracked by GitHub issue Source status reports every chunk as pending: chunks_pending reads a column nothing writes #5329.Commit & Branch
Mustaqeem66:fix/5329-chunks-pending-embedding-sidecar583dbf1(see the commit list for the full series)Validation Run
Nothing in this section was run — my environment has no Rust or Node toolchain (see Validation Blocked). Listing them as unchecked boxes rather than false checkmarks:
pnpm --filter openhuman-app format:checkpnpm typecheckcargo test -p openhuman memory::sources::statuscargo fmt --check && cargo clippyIn place of local runs I desk-checked against the authoritative sources: every helper signature used in the tests (
set_chunk_embedding_for_signature,mark_chunk_reembed_skipped,set_chunk_lifecycle_status,upsert_chunks,tree_active_signature,CHUNK_STATUS_DROPPED) was read fromsrc/openhuman/memory/store/chunks/{store,embeddings}.rs; theChunk/Metadataliteral shape and theSourceKind-collision alias follow the existing pattern intests/raw_coverage/memory_threads_raw_coverage_e2e.rs; and line widths/chain formatting were checked against rustfmt'smax_widthandchain_widthdefaults using formatted code in this repo as the reference. CI is the real verification — I'll fix anything it turns up promptly.Validation Blocked
command:cargo fmt --check,cargo clippy,cargo test,pnpm typecheck,pnpm test:coverageerror:toolchain unavailable in my environment — nocargo,rustc,pnpm,npm, orgit, and no network access to install them.impact:I could not execute the new tests locally or confirm rustfmt/clippy cleanliness before pushing. The logic was verified by reading upstream implementations rather than by running. If CI reports a formatting or compile nit, it's mine and I'll turn it around quickly.Behavior Changes
chunks_pendingreflects real embedding coverage for the active model signature instead of a constant equal tochunks_synced.Parity Contract
chunks_synced,last_chunk_at_ms,freshness, thesource_id LIKEprefix dispatch, theSourceStatusshape, and thestatus_listper-source error fallback are all untouched. Pre-sidecar vaults keep resolving via the legacyembeddingcolumn (term 4 above), so they see no regression.freshness_thresholdsandsource_id_prefix_dispatchunit tests are retained unmodified (the latter now builds its entry through a shared helper). The three existing e2e assertions onchunks_pendingwere audited against the new predicate and all still hold —memory_source_status_counts_reader_and_composio_prefixes(legacy column → 1 pending),memory_raw_coverage_e2e(no embeddings → 2 pending), andworker_c_modules_e2e(already sidecar-shaped → 1 pending).Duplicate / Superseded PR Handling
Summary by CodeRabbit