fix(perf): strip oversized inline images at write time + covering index (prod beach-ball fix)#189
Open
ankitvgupta wants to merge 2 commits into
Open
fix(perf): strip oversized inline images at write time + covering index (prod beach-ball fix)#189ankitvgupta wants to merge 2 commits into
ankitvgupta wants to merge 2 commits into
Conversation
Prod forensics found the emails table was 1.6GB for ~15k rows because inline images were stored as base64 data: URIs inside body HTML (avg 106KB/row, max 29MB) — content the renderer strips to a placeholder before display anyway. Combined with better-sqlite3 running synchronously on the main process, every inbox/sent/search scan walked each row's overflow-page chain and froze the app for 0.8-12s (beach ball), with style-profiler bursts reaching 67-237s. - New db/body-sanitizer.ts (dependency-free): stripLargeDataUris moved from db/index.ts, shared placeholder helper + threshold - saveEmail strips bodies at the write boundary - gmail-client resolves oversized inline images (part.body.size known up front) straight to the placeholder — no more multi-MB attachment downloads that would be stripped anyway - Migration 8: backfills the strip over existing rows (one body in memory at a time), widens idx_emails_merge_cover into a covering index for getInboxEmails' allLight query, then VACUUMs (new vacuumAfter migration flag — VACUUM can't run in a transaction) Rehearsed on a copy of the real prod DB (15,157 emails, 1.8GB): migration takes 15.6s one-time, DB shrinks to 569MB, and the hot queries drop from 465-1672ms to 6-64ms (25-100x). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
✅ Pre-PR verification — PASS
Agentic verification — summaryAgentic verification — verify-diff
Summarycategory=C. Behavior change to existing flow: data-URI stripping moved to write boundary + migration 8 to strip existing bodies + index replacement + VACUUM. Verified: (1) schema_version shows migration 8 applied at 2026-07-16T05:41:51; (2) sqlite_master confirms idx_emails_all_light exists with the full 7-column schema and idx_emails_merge_cover was dropped; (3) DB query Agentic verification — literal trace[2026-07-16T06:34:27.669Z] text: Email renders correctly with images intact (non-large ones preserved). Let me verify the new DB index was created and check that {"imgCount":3,"dataUriImages":[],"hasIframe":true,"accessible":true}[2026-07-16T06:34:45.685Z] text: Excellent results: new index
All verification checks passed. Compiling final result: {"verdict":"pass","summary":"category=C. Behavior change to existing flow: data-URI stripping moved to write boundary + migration 8 to strip existing bodies + index replacement + VACUUM. Verified: (1) schema_version shows migration 8 applied at 2026-07-16T05:41:51; (2) sqlite_master confirms idx_emails_all_light exists with the full 7-column schema and idx_emails_merge_cover was dropped; (3) DB query |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+30
to
+32
| `<text x="200" y="35" text-anchor="middle" fill="#4b5563" font-family="system-ui" font-size="13">` + | ||
| `Inline ${mime} (${sizeLabel}) — too large to display inline` + | ||
| `</text></svg>`; |
There was a problem hiding this comment.
mime embedded in SVG text without XML escaping
The mime value is inserted directly into SVG <text> content. The capturing regex [^;,]+ can match <, >, and &, so a crafted email with a MIME type like image/png</text></svg><image ... would produce malformed SVG XML. The placeholder image would fail to render for those emails. Fix: escape & → &, < → <, > → > before interpolating mime.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/db/body-sanitizer.ts
Line: 30-32
Comment:
**`mime` embedded in SVG text without XML escaping**
The `mime` value is inserted directly into SVG `<text>` content. The capturing regex `[^;,]+` can match `<`, `>`, and `&`, so a crafted email with a MIME type like `image/png</text></svg><image ...` would produce malformed SVG XML. The placeholder image would fail to render for those emails. Fix: escape `&` → `&`, `<` → `<`, `>` → `>` before interpolating `mime`.
How can I resolve this? If you propose a fix, please make it concise.
Owner
Author
There was a problem hiding this comment.
Fixed in 328e57c. inlineImagePlaceholder now validates the mime against /^[\w.+-]+\/[\w.+-]+$/ and falls back to "image" on mismatch, so no raw markup can reach the SVG regardless of the consuming context. (The strip logic also moved to src/shared/body-sanitizer.ts as the single source of truth for both the main process and the renderer.) Added a test asserting an injected mime like image/png</text><script>... is neutralized.
Comment on lines
+466
to
+471
| if (needsVacuum) { | ||
| log.info("[DB] Running one-time VACUUM to reclaim space freed by migrations"); | ||
| const start = Date.now(); | ||
| db.exec("VACUUM"); | ||
| log.info({ durationMs: Date.now() - start }, "[DB] VACUUM complete"); | ||
| } |
There was a problem hiding this comment.
The migration transaction (strip + schema_version INSERT) commits before VACUUM is called. If the app is force-quit in the window between that commit and db.exec("VACUUM") — which is plausible given users with beach-ball issues commonly force-quit — migration 8 is permanently recorded as applied and needsVacuum will be false on all subsequent launches. The freed pages stay in SQLite's internal free list; query performance is already restored by the strip, but the ~1.2 GB of freed disk space won't be reclaimed. A mitigation would be to track VACUUM completion separately (e.g. a vacuumed_after_migration_8 row in schema_version or a dedicated flag table) and re-run VACUUM on the next launch if it wasn't completed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/db/migrations.ts
Line: 466-471
Comment:
**VACUUM skipped if app is force-quit between migration commit and VACUUM**
The migration transaction (strip + `schema_version` INSERT) commits before `VACUUM` is called. If the app is force-quit in the window between that commit and `db.exec("VACUUM")` — which is plausible given users with beach-ball issues commonly force-quit — migration 8 is permanently recorded as applied and `needsVacuum` will be `false` on all subsequent launches. The freed pages stay in SQLite's internal free list; query performance is already restored by the strip, but the ~1.2 GB of freed disk space won't be reclaimed. A mitigation would be to track VACUUM completion separately (e.g. a `vacuumed_after_migration_8` row in `schema_version` or a dedicated flag table) and re-run VACUUM on the next launch if it wasn't completed.
How can I resolve this? If you propose a fix, please make it concise.
Owner
Author
There was a problem hiding this comment.
Fixed in 328e57c. VACUUM is no longer driven only by an in-memory flag: maybeVacuum also runs when the freelist is large (>20% of the file and >50MB), checked cheaply on every startup. So if the app is force-quit between the migration commit and VACUUM, the next launch sees the large freelist and reclaims the space. VACUUM is also wrapped in try/catch now, so a SQLITE_FULL on a near-full disk logs and continues instead of crash-looping startup.
…tion Addresses review findings on the body-strip PR: - CRITICAL (ReDoS): the strip regex `<img\b[^>]*?\bsrc...` was quadratic on attacker-controlled bodies full of `<img` with no `>` — a sender could freeze the synchronous main process for minutes, the exact beach ball this PR fixes. Re-anchored on `\bsrc\s*=\s*["']data:` so the scan is linear (verified by a timing test); this also broadens coverage to video/source/input src attrs. - Case-insensitive `data:` guard (`/data:/i`) — `DATA:` no longer bypasses the strip (URI schemes are case-insensitive per RFC 2397). - Hoisted the sanitizer to src/shared/body-sanitizer.ts as the single source of truth; the renderer copy (email-body-cache.ts) now delegates to it (was a second, drifting copy of the same vulnerable regex). - Validate/escape the mime type in the placeholder SVG (defense-in-depth; Greptile P2 + security review). - Migration 8: strip BEFORE building the index so the index build scans the shrunk table, not the 1.6GB overflow chains. - VACUUM is now self-healing (runs if freelist is large, so an interrupted vacuum reclaims on a later launch) and wrapped in try/catch (SQLITE_FULL no longer boot-loops the app after a committed migration). - gmail-client: named InlineImageInfo type; size estimate includes the data-URI prefix. Tests: linear-time guard, case-insensitive/single-quote/multi-URI/non-img cases, mime injection, covering-index EXPLAIN QUERY PLAN, no-op candidate row. Re-rehearsed on a prod DB copy: 1803MB → 568MB, hot queries to tens of ms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
Review triage (Greptile + specialist review army: security, performance, data-migration, testing, maintainability, Claude adversarial)Ran a multi-agent review over the diff. Summary of what was fixed in Fixed (this was a real bug I introduced)
Deferred (noted, not blocking) — good follow-up material
Full follow-up work (pathological query shapes, startup FTS check, unbounded thread payloads, DB worker thread) is in #188. Re-rehearsed on a copy of the real prod DB after the fixes: 1803MB → 568MB, hot queries drop to tens of ms, FTS intact. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the recurring prod beach-balls (main-process freezes). A forensic audit of the installed
0.15.0-beta.3build, the prod database (read-only), and 8 days of prod logs found the root amplifier: the emails table was 1.6 GB for ~15k rows because inline images are stored as base64data:URIs inside body HTML (avg 106 KB/row, max 29.4 MB) — content the renderer strips to a placeholder before display anyway. Since better-sqlite3 runs synchronously on the Electron main process andbodyis declared before the columns hot queries read (label_ids,message_id,in_reply_to), every inbox/sent/search scan walked each row's overflow-page chain and froze the app for 0.8–12 s per query; style-profiler bursts reached 67–237 s with force-quits visible in the logs.This PR removes the bloat at the write boundary and backfills existing rows. Follow-ups (pathological query shapes, startup FTS check, unbounded thread payloads, DB worker thread) are tracked in #188.
Changes
src/main/db/body-sanitizer.ts(dependency-free, importable bymigrations.ts):stripLargeDataUrismoved fromdb/index.ts, plus a sharedinlineImagePlaceholderhelper and theDATA_URI_STRIP_THRESHOLD(50 KB) constant. Same strip semantics as before — small inline images (signatures, logos) keep displaying.saveEmailstrips bodies before insert. The read-path strip inrowToDashboardEmailstays as a backstop, but stored rows are now small at the source.gmail-client.ts:collectInlineImagesnow capturespart.body.size, andresolveInlineImagesmaps oversizedcid:references straight to the placeholder — the app no longer downloads multi-MB inline attachments just to strip them later.strip_large_data_uris_and_widen_merge_cover_index):AFTER UPDATEtriggers keep the search index consistent (covered by tests).idx_emails_merge_coverwithidx_emails_all_light— a covering index forgetInboxEmails' allLight query (the old index's four columns are its prefix, so it's strictly superseded).vacuumAfterflag on the migration type: VACUUM can't run inside a transaction (each migration runs in one), so it's deferred to the end ofrunNumberedMigrationsand runs at most once.schema.ts: fresh DBs getidx_emails_all_lightdirectly.Measured impact (rehearsal on a copy of the real prod DB — 15,157 emails, 1.8 GB)
COUNT(*)Migration cost: 15.6 s one-time (587 emails stripped, 1,206 MB reclaimed, VACUUM 4.9 s). FTS verified intact after the backfill (
MATCHsanity query returns expected hits). The prod logs showed the same query shapes blocking the main process for p50 1.6 s / p95 6.5 s / max 12 s on every startup, account switch, and sync tick that fetched mail — those all shrink to tens of ms.Considered and rejected
bodylast — requires a full table rebuild that breaks the external-content FTS rowid mapping (forcing an FTS rebuild) for marginal gain once bodies are stripped.email_bodiestable — larger blast radius (FTS triggers, every body reader); the strip achieves the measured win without schema surgery. Can revisit if legit large HTML bodies (newsletters) become a problem.Tests
tests/unit/body-sanitizer.spec.ts— strip semantics (oversized replaced, small preserved, mixed, no-data-URI unchanged, placeholder content).tests/migrations/strip-data-uris.spec.ts— realrunMigrationsagainst a pre-migration-shaped DB: strip correctness, index swap, FTS consistency through the backfill, idempotency, fresh-DB no-op.Follow-up issue: #188
🤖 Generated with Claude Code
Pre-PR verdict: PASS
full328e57c