fix(memory): gmail canonicalisation, signature unification, adaptive page retry - #132
fix(memory): gmail canonicalisation, signature unification, adaptive page retry#132yh928 wants to merge 5 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds canonical and legacy embedding-signature matching across chunk and summary storage, provider-aware page-size retries for incremental syncs, 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| shrink_page_size(&mut arguments, source.page_size_arg_key()) | ||
| { | ||
| state.record_requests(execute_error.attempts); | ||
| page_size_override = Some(reduced); |
There was a problem hiding this comment.
page_size_override set to the candidate size, not the confirmed size
page_size_override = Some(reduced) is written before the retried request is known to succeed. If multiple halving steps are needed (e.g. 25 → 12 → 6 → accepted), page_size_override is updated on each failed attempt: Some(12) after the first rejection, then Some(6) after the second. Because the final value happens to be the one that worked, subsequent pages receive the correct override — but only by coincidence of ordering. A comment or a post-success assignment (moving page_size_override = Some(…) to just after break response) would make the "sticky for the run" intent explicit and immune to any future reordering of the retry steps.
|
| Filename | Overview |
|---|---|
| src/memory/chunks/signature.rs | New module — implements signature parsing, equivalence checks, and signature_variants/signature_in_clause to bridge the legacy {model}@{dims} and canonical provider=…;model=…;dims=… spellings. Well-documented with thorough tests. |
| src/memory/chunks/embeddings.rs | All per-signature reads now use IN (?n, ?n+1, …) via signature_variants; writes continue to use the canonical form. The has_uncovered_reembed_work query reuses numbered SQLite parameters correctly across four IN clauses. |
| src/memory/sync/composio/gmail.rs | Gmail sync now produces canonical Markdown via email::canonicalise instead of raw JSON; adds with_executor seam and page_size_arg_key. Fallback chain in message_body and deterministic epoch default in message_sent_at are correct. |
| src/memory/sync/composio/orchestrator.rs | Adds adaptive page-size retry loop for 413 responses; retry terminates correctly because shrink_page_size returns None at the floor. File is now 545 lines, exceeding the project's 500-line guideline. |
| src/memory/tree/store/summaries.rs | Summary-embedding reads now use signature_variants / signature_in_clause in both single and batch fetch paths, mirroring the same fix applied to chunk embeddings. |
| src/memory/config.rs | Adds provider field to EmbeddingConfig with serde(default = "default_embedding_provider") ("ollama"), matching the existing default model; existing configs without the field deserialize transparently. |
| src/memory/chunks/connection.rs | Splits open_and_init into a retry wrapper + try_open_and_init; retries up to 3 times with exponential backoff on is_transient_cold_start errors including the newly added SQLITE_IOERR_FSTAT. |
| src/memory/chunks/recovery.rs | Adds SQLITE_IOERR_FSTAT (1802) to the transient cold-start error set; covered by a new test in recovery_tests.rs. |
| src/memory/sources/types.rs | Extends max_items validation to accept SourceKind::Composio alongside RssFeed, fixing a UI desync where editing a Composio source's item cap was rejected. |
| src/memory/chunks/signature_tests.rs | New test file with 8 tests covering parse, equivalence, variant expansion, dedup, and IN-clause generation — comprehensive coverage for the new signature module. |
| src/memory/sync/composio/orchestrator_tests.rs | New test file: SizeLimitedExecutor simulates a provider ceiling; covers the halving sequence, no-shrink fast-fail, is_payload_too_large discrimination, and floor behavior. |
| src/memory/sync/composio/gmail_tests.rs | New test file: 5 tests covering canonical Markdown output, body fallback chain, blank-field skip, recipient splitting, and deterministic epoch for undated messages. |
| src/memory/chunks/store_embed_tests.rs | Adds an end-to-end regression test: a vector written under the legacy spelling is readable, counts as covered, and does not block a genuinely different space — all three hot paths verified. |
| src/memory/sources/types_tests.rs | New test guards the max_items Composio validation fix: checks that Composio and RSS accept it while Folder, GithubRepo, and WebPage reject it. |
| src/memory/chunks/recovery_tests.rs | Adds a test confirming IOERR_FSTAT (1802) is classified as transient and that a non-transient error (code 19) is not. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Build arguments from source.arguments] --> B{page_size_override set?}
B -->|Yes| C[apply_page_size: pin key to override]
B -->|No| D[Use source default]
C --> E[executor.execute]
D --> E
E --> F{Transport error?}
F -->|Yes, tolerated| G[Warn, break to next scope]
F -->|Yes, fatal| H[return Err]
F -->|No| I[record_action cost+attempts]
I --> J{response.successful?}
J -->|Yes| K[break to process items]
J -->|No| L{is_payload_too_large?}
L -->|No| M{tolerate_scope_errors?}
M -->|Yes| G
M -->|No| H
L -->|Yes| N{shrink_page_size possible?}
N -->|No, at floor| M
N -->|Yes| O[Halve page size in arguments, set page_size_override]
O --> P{budget_exhausted?}
P -->|Yes| Q[mark more_pending, break scopes]
P -->|No| E
K --> R[Extract and dedup items]
R --> S{More pages?}
S -->|Yes, next token| A
S -->|No| T[Done with scope]
Reviews (2): Last reviewed commit: "fix(chunks): retry a transient cold-star..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
|
||
| /// Smallest page a shrink will ask for. One item is the point past which a | ||
| /// too-large response is about that single item, not the batch size. | ||
| const MIN_PAGE_SIZE: u64 = 1; | ||
|
|
||
| /// Whether the provider refused a page for its *size* rather than for anything | ||
| /// about the request's content — the only failure a smaller page can fix. | ||
| /// | ||
| /// Matched on the error text because that is all the envelope carries: Composio | ||
| /// reports it as HTTP 413 with a `Upstream_PayloadTooLarge` slug, and other | ||
| /// backends phrase it as "payload too large" / "response too large". | ||
| fn is_payload_too_large(error: Option<&str>) -> bool { | ||
| error.is_some_and(|error| { | ||
| let lower = error.to_ascii_lowercase(); | ||
| lower.contains("payloadtoolarge") | ||
| || lower.contains("payload_too_large") | ||
| || lower.contains("413") | ||
| || (lower.contains("too large") | ||
| && (lower.contains("payload") || lower.contains("response"))) | ||
| }) | ||
| } | ||
|
|
||
| /// Pin the page-size argument to `size`, if the source declared one. | ||
| fn apply_page_size(arguments: &mut Value, key: Option<&str>, size: Option<u64>) { | ||
| if let (Some(key), Some(size)) = (key, size) { | ||
| if let Some(slot) = arguments.get_mut(key) { | ||
| *slot = Value::from(size); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Halve the page-size argument in place, returning the new value. | ||
| /// | ||
| /// `None` means retrying is pointless — the source declares no page-size | ||
| /// argument, this request does not carry it, or it is already at the floor. | ||
| fn shrink_page_size(arguments: &mut Value, key: Option<&str>) -> Option<u64> { | ||
| let key = key?; | ||
| let current = arguments.get(key)?.as_u64()?; | ||
| if current <= MIN_PAGE_SIZE { | ||
| return None; | ||
| } | ||
| let reduced = (current / 2).max(MIN_PAGE_SIZE); | ||
| arguments[key] = Value::from(reduced); | ||
| Some(reduced) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[path = "orchestrator_tests.rs"] | ||
| mod tests; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
File exceeds the 500-line cap introduced by this diff.
The new retry helpers and mod tests wiring push this file to roughly 545 lines, past the guideline's 500-line threshold. Consider extracting the page-size retry helpers (MIN_PAGE_SIZE, is_payload_too_large, apply_page_size, shrink_page_size) into a focused sibling module (e.g. page_size.rs) re-exported here, with its own page_size_tests.rs. 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 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.
Source: Path instructions
| fn is_payload_too_large(error: Option<&str>) -> bool { | ||
| error.is_some_and(|error| { | ||
| let lower = error.to_ascii_lowercase(); | ||
| lower.contains("payloadtoolarge") | ||
| || lower.contains("payload_too_large") | ||
| || lower.contains("413") | ||
| || (lower.contains("too large") | ||
| && (lower.contains("payload") || lower.contains("response"))) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bare "413" substring match risks false positives.
Matching any error text containing "413" anywhere (Line 513) can misclassify unrelated failures whose message happens to contain that numeral (an id, amount, timestamp, etc.) as a too-large-page error, triggering pointless shrink/retry cycles before the real error is finally surfaced. Consider requiring "413" to appear as a status-code-like token (e.g. anchored with word boundaries, or only checked alongside an HTTP-status-shaped prefix) rather than an unanchored substring.
🩹 Tighten the 413 check
- lower.contains("payloadtoolarge")
- || lower.contains("payload_too_large")
- || lower.contains("413")
- || (lower.contains("too large")
- && (lower.contains("payload") || lower.contains("response")))
+ lower.contains("payloadtoolarge")
+ || lower.contains("payload_too_large")
+ || lower
+ .split(|c: char| !c.is_ascii_alphanumeric())
+ .any(|token| token == "413")
+ || (lower.contains("too large")
+ && (lower.contains("payload") || lower.contains("response")))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn is_payload_too_large(error: Option<&str>) -> bool { | |
| error.is_some_and(|error| { | |
| let lower = error.to_ascii_lowercase(); | |
| lower.contains("payloadtoolarge") | |
| || lower.contains("payload_too_large") | |
| || lower.contains("413") | |
| || (lower.contains("too large") | |
| && (lower.contains("payload") || lower.contains("response"))) | |
| }) | |
| } | |
| fn is_payload_too_large(error: Option<&str>) -> bool { | |
| error.is_some_and(|error| { | |
| let lower = error.to_ascii_lowercase(); | |
| lower.contains("payloadtoolarge") | |
| || lower.contains("payload_too_large") | |
| || lower | |
| .split(|c: char| !c.is_ascii_alphanumeric()) | |
| .any(|token| token == "413") | |
| || (lower.contains("too large") | |
| && (lower.contains("payload") || lower.contains("response"))) | |
| }) | |
| } |
🤖 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 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.
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
1 participant
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 (reject for other kinds).