Fix deduplicate plan timeout: index + batch RFC822 duplicate-group lookup - #512
Conversation
roborev: Combined Review (
|
|
Holding off to do more testing |
|
I am running into multiple issues on my super old email archive. |
|
Okay, was thrown off by #526. |
![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: 3ea7f4e9eb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| msgsByGroup, err := e.store.GetDuplicateGroupMessagesBatch( | ||
| rfc822IDs, e.config.AccountSourceIDs..., | ||
| ) |
There was a problem hiding this comment.
When the dedup plan context is canceled after FindDuplicatesByRFC822ID starts returning results (for example, the daemon client hits its long request timeout or the user cancels the CLI), this new batched call still runs every chunk because GetDuplicateGroupMessagesBatch/queryInChunks do not receive ctx, and Scan only checks ctx.Err() after all rows are materialized. Previously the per-group loop checked cancellation before each lookup, so at most one lookup ran after cancellation; please check/pass the context before and during the batch so canceled plans stop promptly.
Useful? React with 👍 / 👎.
roborev: Combined Review (
|
GetDuplicateGroupMessages ran one unindexed query per RFC822 duplicate group; large archives could exhaust the CLI plan-request timeout before content-hash comparison began. - perf: batch GetDuplicateGroupMessages into chunked queries - perf: switch Engine.Scan to GetDuplicateGroupMessagesBatch - refactor: address final review findings on dedup rfc822 fix - merge current main before review Authored-By: Jesse Robbins (@jesserobbins) <https://jesserobbins.com> Generated with Codex Co-authored-by: Codex <noreply@openai.com>
A timed-out dedup scan could continue issuing every RFC822 lookup chunk because the batching helper replaced the request context with a background context. Large archives therefore kept consuming database work after the caller disconnected. Thread the scan context through the batch API and chunk queries so in-flight database work is interruptible and no later chunk starts after cancellation. The background-context wrapper remains for callers that do not yet carry request scope. Generated with Codex Co-authored-by: Codex <noreply@openai.com>
33b1f50 to
2c25920
Compare
roborev: Combined Review (
|
* origin/main: Fix deduplicate plan timeout: index + batch RFC822 duplicate-group lookup (kenn-io#512) Index relationship analytics for fast, memory-bounded queries (kenn-io#528)
2 participants
Motivation
Running
deduplicate --content-hashon my own archive, the CLI died after 30 minutes withcontext deadline exceededand zero output. Traced it to an N+1 query with no supporting index —Scanwas querying the store once per RFC822 duplicate group, and on a real archive that's tens of thousands of round trips before content-hash comparison even starts.Summary
messages.rfc822_message_id— a partial index (mirroring the existing attachment-hash indexes) turns out to be unusable by SQLite's planner for this query shape, verified viaEXPLAIN QUERY PLANbefore picking the plain form.Store.GetDuplicateGroupMessagesBatch, replacing the per-group query with a handful of chunkedIN (...)queries.Engine.Scanto the batched method.Fixes #510.
Test plan
make testpasses (one unrelated pre-existing failure ininternal/config— a macOS/varvs/private/varsymlink artifact in a file this PR never touches).deduplicate --collection local --content-hash --no-backup -v --dry-runagainst a personal archive with 161,113 duplicate groups. Before this fix, the equivalent run never completed — it hit the CLI's 30-minute client timeout with zero output, having made partial progress through roughly 22,025 of those groups. After: completes in ~311s.Out of scope, tracked in #510 but not touched here:
BackfillRFC822IDswriting to the DB unconditionally during/planbefore confirmation, and the server not respecting client cancellation once a request times out client-side.Authored by Jesse Robbins (@jesserobbins)