fix(memory): gmail canonicalisation, signature unification, adaptive page retry - #132
fix(memory): gmail canonicalisation, signature unification, adaptive page retry#132yh928 wants to merge 6 commits into
Conversation
Gmail sync wrote `to_string_pretty(&item.raw)` as the document content, so a MIME tree with base64 part bodies reached the chunker: the words of the mail were absent from the store, and no amount of recall tuning could match them. Route each message through the email canonicaliser the tree already uses — headers as a small block, body through `email_clean::clean_body` (reply chains and footer boilerplate stripped). The adapter reuses `parse_message_date` and falls back to the epoch when a message carries no date at all, so an undated message does not rewrite its own content — and re-chunk and re-embed — on every sync. `GmailSyncPipeline` gains `with_executor`: the response reshape that slims a verbose Gmail payload into one record per message lives in the host, above this crate, and the executor is the only seam through which the sync can reach it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
The tree keyed its per-model sidecars by `{model}@{dims}` while the namespace
store used `provider={p};model={m};dims={d}`. Every per-signature read is an
exact string match, so the two spellings partition one vector space in half: a
store that changed spelling stops seeing its own prior vectors and silently
scores against nothing. On a live workspace that stranded hundreds of tree
chunks after a format change that was never a model change.
Writes now emit the canonical spelling — delegated to the same formatter the
namespace store's providers use, so the two cannot drift apart again — and
reads match any spelling of the same space via `signature_variants`. Nothing on
disk is rewritten: a legacy row keeps its own signature and simply becomes
readable again, and it counts as coverage so the re-embed chain does not pay to
recompute a vector that is already stored.
`EmbeddingConfig` gains `provider` because the canonical spelling names it and
the tree config had no way to.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
A page the provider refuses purely for its size is the one provider failure a *smaller request* can fix, but the orchestrator treated it like any other and failed the run. Gmail asks for 25 full message payloads at a time, so whether a page fits depends on what is in the mail: on one live workspace a single oversized page stopped the source dead, and it stayed dead for nine days because nothing retried it and nothing looked. On a too-large refusal the orchestrator now halves the source's page-size argument and re-requests the same page, down to one item. Sources opt in by naming that argument (`page_size_arg_key`, `max_results` for Gmail); one that declares none — or a page already at the floor — still surfaces the failure rather than looping on a request that cannot change. The reduced size sticks for the rest of the run so later pages don't each pay a rejected round-trip to rediscover the same limit, and every attempt is still recorded against the request budget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
📝 WalkthroughWalkthroughThe PR adds canonical and legacy embedding-signature matching, provider-aware page-size retries, canonical Gmail Markdown rendering, transient database bootstrap retries, and Composio source validation. ChangesEmbedding signature compatibility
Gmail synchronization resilience
Storage bootstrap and source validation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SyncOrchestrator
participant GmailSyncPipeline
participant ActionExecutor
participant MemoryStore
SyncOrchestrator->>GmailSyncPipeline: request page with max_results
GmailSyncPipeline->>ActionExecutor: execute Gmail request
ActionExecutor-->>GmailSyncPipeline: payload-too-large response
GmailSyncPipeline->>ActionExecutor: retry with reduced max_results
ActionExecutor-->>SyncOrchestrator: successful message page
SyncOrchestrator->>MemoryStore: store canonical Markdown document
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 |
|
| Filename | Overview |
|---|---|
| src/memory/chunks/signature.rs | New module unifying two signature spellings. parse_signature handles both canonical and legacy forms. signature_variants expands for IN-clause reads. Thorough test coverage. |
| src/memory/chunks/embeddings.rs | All per-signature reads now expand via signature_variants and match with an IN clause. tree_active_signature now emits the canonical provider-inclusive form. |
| src/memory/sync/composio/gmail.rs | Core fix: canonical_markdown replaces raw JSON storage with a properly canonicalised Markdown document. Deterministic sent-at timestamp prevents re-chunking on undated messages. |
| src/memory/sync/composio/orchestrator.rs | Adaptive retry loop added inside the per-page loop. page_size_override is correctly set only after a page succeeds. |
| src/memory/sync/composio/page_size.rs | New module with is_payload_too_large, shrink_page_size (halves with floor of 1), and apply_page_size. Word-boundary check prevents false-positives from numbers containing '413'. |
| src/memory/chunks/connection.rs | Added 3-attempt exponential-backoff retry for transient cold-start I/O errors. |
| src/memory/chunks/recovery.rs | Added SQLITE_IOERR_FSTAT (1802) to the transient cold-start codes with accompanying test. |
| src/memory/config.rs | Adds provider: String field to EmbeddingConfig with #[serde(default)]. Backward-compatible. |
| src/memory/sources/types.rs | Widens the max_items validation guard from RSS-only to RSS + Composio, fixing a UI-to-store desync bug. |
| src/memory/tree/store/summaries.rs | Summary embedding reads now use signature_variants + IN-clause, giving legacy-spelled summary vectors the same visibility fix as chunk vectors. |
Reviews (3): Last reviewed commit: "fix(sync): record the accepted page size..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/memory/chunks/embeddings.rs (1)
456-507: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit embeddings query helpers before the 500-line cap.
src/memory/chunks/embeddings.rsis now 518 lines, so extract the signature/embedding query helpers into a focused sibling module such assrc/memory/chunks/embeddings_query.rs.🤖 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/memory/chunks/embeddings.rs` around lines 456 - 507, Extract the signature/embedding query helpers, including get_chunk_embeddings_for_signature_batch and its supporting query logic, from embeddings.rs into a focused sibling module such as embeddings_query.rs. Update module declarations and imports so existing callers retain the same behavior and public API, and keep embeddings.rs below the 500-line limit.Source: Coding guidelines
src/memory/sync/composio/gmail.rs (1)
280-296: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCc recipients are always dropped from the canonical email.
ccis hardcoded toVec::new()(Line 288) even thoughemail::canonicaliserenders aCc:line when present. If the upstream Gmail response reshaper surfaces Cc data on the message payload, it's silently lost here, meaning content someone was Cc'd on won't be searchable via that header. Worth confirming whether the reshaped envelope carries Cc data at all, and if so, extracting it the same waytois handled.🤖 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/memory/sync/composio/gmail.rs` around lines 280 - 296, The email_thread function discards Cc recipients by always assigning an empty vector. Confirm the reshaped Gmail message payload’s Cc field and extract it using the same recipient parsing approach as message_recipients for to, preserving the populated Cc values in EmailMessage.
🤖 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/memory/sync/composio/orchestrator.rs`:
- Around line 497-545: Split the page-size retry helpers MIN_PAGE_SIZE,
is_payload_too_large, apply_page_size, and shrink_page_size out of
orchestrator.rs into a focused sibling module such as page_size.rs, re-exporting
or importing them for existing callers. Move their tests into a dedicated
page_size_tests.rs module while preserving current behavior and keeping
orchestrator.rs below the 500-line limit.
- Around line 508-517: Update is_payload_too_large so the 413 detection only
matches status-code-like occurrences rather than any unanchored “413” substring.
Preserve the existing payloadtoolarge, payload_too_large, and too-large
payload/response checks while preventing unrelated identifiers, amounts, or
timestamps from triggering retries.
---
Nitpick comments:
In `@src/memory/chunks/embeddings.rs`:
- Around line 456-507: Extract the signature/embedding query helpers, including
get_chunk_embeddings_for_signature_batch and its supporting query logic, from
embeddings.rs into a focused sibling module such as embeddings_query.rs. Update
module declarations and imports so existing callers retain the same behavior and
public API, and keep embeddings.rs below the 500-line limit.
In `@src/memory/sync/composio/gmail.rs`:
- Around line 280-296: The email_thread function discards Cc recipients by
always assigning an empty vector. Confirm the reshaped Gmail message payload’s
Cc field and extract it using the same recipient parsing approach as
message_recipients for to, preserving the populated Cc values in EmailMessage.
🪄 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: 5ba9d842-8a29-4ad2-8ac1-c0ead2d8e11b
📒 Files selected for processing (11)
src/memory/chunks/embeddings.rssrc/memory/chunks/mod.rssrc/memory/chunks/signature.rssrc/memory/chunks/signature_tests.rssrc/memory/chunks/store_embed_tests.rssrc/memory/config.rssrc/memory/sync/composio/gmail.rssrc/memory/sync/composio/gmail_tests.rssrc/memory/sync/composio/orchestrator.rssrc/memory/sync/composio/orchestrator_tests.rssrc/memory/tree/store/summaries.rs
The host UI exposes `max_items` for Composio connections and creates them with a toolkit default, but `validate_for_kind` accepted it only for RSS feeds — so editing a Composio source's per-run cap failed with "field 'max_items' is not applicable to source kind 'composio'". Accept it for Composio too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
Parallel first-opens of the chunk DB race each other on the -shm/-wal side-files, surfacing as flaky disk I/O errors (IOERR_FSTAT 1802, plus the CANTOPEN/SHM* codes already classified). A fresh open a moment later succeeds, so open_and_init now gives a transient cold-start a couple of short backed-off retries before surfacing it, and IOERR_FSTAT joins the transient set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
… out
Review follow-ups on the page-size retry, plus one build break the retry commit
carried in.
- **`page_size_override` records the size the provider accepted**, not the one
the shrink last tried. With several halvings (25 -> 12 -> 6) the old
assignment left the previous *rejected* size in the override on every step but
the last, and was right at the end only because that step happened to be the
accepted one. The value is now set where the response succeeds, so the
"sticky for the run" intent no longer depends on the retry order.
- **`413` has to stand alone.** An unanchored `contains("413")` also matches a
message id, an amount, or a timestamp carrying those digits, and each false
positive costs a shrink-and-retry cycle before the real error surfaces — on a
failure a smaller page was never going to fix.
- **The page-size helpers move to `page_size.rs`** with their own tests, putting
`orchestrator.rs` back under the 500-line cap.
- **`tracing` is gated behind `sync`.** It is an optional dependency, but the
chunk store's new cold-start retry called it unconditionally, so the
dependency-light default build could not compile. CI builds with the feature
on, which is why it stayed green.
- One `cloned_ref_to_slice_refs` lint in a test this PR touches.
Tests: 1301 lib tests green with `--features sync`; the default-feature build
compiles; clippy clean in both configurations.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
|
Pushed
Bare 500-line cap (CodeRabbit) — fixed. One thing the review did not catch, found while verifying: the default-feature build was broken. The chunk store's cold-start retry ( Verification: 1301 lib tests green with |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/memory/sync/composio/orchestrator.rs (1)
512-515: 🛠️ Refactor suggestion | 🟠 MajorContinue the split so
orchestrator.rsstays below 500 lines.The page-size extraction moved the helper logic out of
orchestrator.rs, but the file still reaches Line 515. Extract another cohesive orchestration block before merge.As per path instructions, “Avoid letting any source file grow beyond 500 lines; split behavior into focused modules before that point.”
🤖 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/memory/sync/composio/orchestrator.rs` around lines 512 - 515, Continue refactoring orchestrator.rs so it remains under 500 lines by extracting another cohesive orchestration block into a focused module, while preserving the existing behavior and module interfaces. Update the relevant declarations and imports, including the cfg(test) tests module wiring, so the extracted logic remains accessible and orchestrator.rs contains only its core orchestration responsibilities.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.
Duplicate comments:
In `@src/memory/sync/composio/orchestrator.rs`:
- Around line 512-515: Continue refactoring orchestrator.rs so it remains under
500 lines by extracting another cohesive orchestration block into a focused
module, while preserving the existing behavior and module interfaces. Update the
relevant declarations and imports, including the cfg(test) tests module wiring,
so the extracted logic remains accessible and orchestrator.rs contains only its
core orchestration responsibilities.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f417b00-3eab-4a1c-aa1d-bbe168b986a2
📒 Files selected for processing (7)
src/memory/chunks/connection.rssrc/memory/chunks/store_embed_tests.rssrc/memory/sync/composio/mod.rssrc/memory/sync/composio/orchestrator.rssrc/memory/sync/composio/orchestrator_tests.rssrc/memory/sync/composio/page_size.rssrc/memory/sync/composio/page_size_tests.rs
💤 Files with no reviewable changes (1)
- src/memory/sync/composio/orchestrator_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/memory/chunks/store_embed_tests.rs
- src/memory/chunks/connection.rs
Summary
Three fixes to the Composio/Gmail sync and chunk-embedding layers, all surfaced by the same live investigation into why gmail-ingested memories were unretrievable.
feat(sync/gmail)— Gmail sync storedto_string_pretty(&item.raw)as document content: a MIME tree with base64 part bodies, so the mail's words never reached the store and recall could not match them. Each message now routes through the existingemail::canonicalise(headers as a small block, body throughemail_clean::clean_body), reusingparse_message_dateand falling back to the epoch when undated so a message doesn't re-chunk on every sync. AddsGmailSyncPipeline::with_executorso the host can wrap the executor with its response reshape.fix(chunks)— the tree keyed per-model sidecars by{model}@{dims}while the namespace store usedprovider=…;model=…;dims=…. Per-signature reads are exact string matches, so one vector space was partitioned in two; a format change (not a model change) stranded live tree chunks. Writes now emit the canonical spelling (delegated to the sametinyagentsformatter the namespace store uses) and reads match any spelling viasignature_variants— nothing on disk is rewritten, legacy rows simply become readable again and count as coverage.fix(sync)— a page the provider refuses purely for size (Gmail 413Upstream_PayloadTooLarge) is the one provider failure a smaller request can fix; the orchestrator now halves the source's page-size argument and retries down to one item, sticky for the run. Sources opt in viapage_size_arg_key(max_resultsfor Gmail); others are unchanged.Tests
cargo test --features sync— 1298 unit + integration tests pass. New coverage: gmail canonicalisation (5), signature parsing/equivalence/variants + legacy-spelling read-back (18 in the chunks suite), too-large-page retry (4).Notes
Consumed by an OpenHuman core PR (durable re-embed sweep + connector-aware recall) that depends on the new
EmbeddingConfig.providerfield andGmailSyncPipeline::with_executor.Summary by CodeRabbit
New Features
Bug Fixes
max_itemsfor Composio and RSS sources while rejecting it for unsupported source types.