Skip to content

fix(cache): repair invalid UTF-8 during analytics export - #924

Open
salmonumbrella wants to merge 1 commit into
kenn-io:mainfrom
salmonumbrella:fix/cache-export-valid-utf8
Open

salmonumbrella wants to merge 1 commit into
kenn-io:mainfrom
salmonumbrella:fix/cache-export-valid-utf8

Conversation

@salmonumbrella

Copy link
Copy Markdown
Contributor

What changed

  • Repair invalid UTF-8 in text exported to the analytics cache, including full builds and derived refreshes. Valid text and NULL values keep their current behavior.
  • Keep identity attribution and conversation metadata freshness checks aligned with the repaired cache values, so an unchanged archive stays fresh.
  • Count repaired cache projections and print a warning after a successful build. One damaged source value can count more than once if it fills multiple cache columns. Fast search now gives a repair command when it encounters an older cache with invalid text.

Why

One malformed archived text value can make a cache build or fast search fail when DuckDB reads the exported data. The cache should remain usable while showing the operator how to repair the source archive.

Usage

Existing msgvault build-cache and fast search commands work as before. If a build reports repairs, run msgvault repair-encoding and then msgvault build-cache --full-rebuild to repair the archive and replace the cache.

@salmonumbrella
salmonumbrella marked this pull request as ready for review September 24, 2026 00:53
@roborev-ci

roborev-ci Bot commented Sep 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (0317582)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/cache_text.go:95: The attribution comparison sanitizes malformed strings with U+FFFD before equality checks, so distinct invalid byte sequences may compare equal and incorrectly attribute messages to the owner or populate owner participants. Use a collision-free representation that preserves invalid bytes, or skip identity comparisons when either key is invalid; apply the same rule to the CSV fallback and reserve U+FFFD sanitization for display/search values.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 5s | Total: 4m45s

@salmonumbrella
salmonumbrella force-pushed the fix/cache-export-valid-utf8 branch from 0317582 to c108cdb Compare September 24, 2026 05:09
@roborev-ci

roborev-ci Bot commented Sep 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (c108cdb)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/build_cache.go:638: Invalid message_recipients.email_address values are converted to NULL, causing envelopePresent to become false and allowing participant fallback to misclassify a message as from the owner using the sender's current participant address. Preserve a separate flag that a non-empty From envelope existed through both SQLite and CSV paths, use it to suppress participant fallback, and avoid resolving email_address from the participant when an invalid envelope was present.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 5s | Total: 5m39s

@roborev-ci

roborev-ci Bot commented Sep 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (c96d016)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/build_cache.go:2247: Invalid identity columns are written as NULL without being counted, while users are directed to msgvault repair-encoding as the recovery path. That command does not repair account identities, participant identifiers, or immutable message recipient envelope addresses, so archives containing only these corruptions can remain misattributed without warning. Add repair handling and reporting for every excluded identity field, including a defined recovery path for envelope snapshots, or clearly report that these fields require manual or re-import remediation instead of implying repair-encoding restores attribution.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 6s | Total: 5m20s

@salmonumbrella
salmonumbrella force-pushed the fix/cache-export-valid-utf8 branch from c96d016 to bbf7af9 Compare September 24, 2026 07:00
@roborev-ci

roborev-ci Bot commented Sep 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (bbf7af9)

Verdict: Changes require fixes for 1 finding.

Medium

  • cmd/msgvault/cmd/cache_text.go:92: Invalid account identity addresses, participant identifier keys, and recorded recipient email addresses are converted to NULL and excluded from attribution, but msgvault repair-encoding does not repair those columns. Following the documented remediation can leave messages permanently unattributed and owner participants missing. Add collision-aware repair passes for these identity and envelope columns, including the required attribution and revision updates, or revise the user-facing remediation to accurately describe the remaining manual recovery.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 4s | Total: 6m19s

@fucx

fucx commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

I ran into this on a real archive today, so I tested your branch against a copy. It fixes the cache build.

The archive has 365,581 messages from Apple Mail and Gmail. On v0.20.0, every build-cache --full-rebuild run stopped at CSV line 305932 with Invalid unicode (byte sequence mismatch) detected. The bad bytes are in rfc822_message_id, from the Message-ID header of a 2022 spam message. Two messages are affected.

I ran the full rebuild twice against the same 27 GB database copy (PRAGMA quick_check was clean):

  • v0.20.0: failed at line 305932.
  • bbf7af9: completed with Warning: 3 invalid UTF-8 repair(s) applied while building the analytics cache. Queries against v_messages worked afterward.

I did find one related issue. The old error hint and the new warning both suggest msgvault repair-encoding, but that command does not scan rfc822_message_id. On this archive it says No messages needed repair, while the unpatched cache build still fails. Your patch gets the build through; the repair advice just sends someone in a circle. Could the scan include that column, or could the warning say what the command actually covers?

Happy to rerun the comparison after a rebase.

@salmonumbrella
salmonumbrella force-pushed the fix/cache-export-valid-utf8 branch from bbf7af9 to e353aee Compare September 25, 2026 17:04
@roborev-ci

roborev-ci Bot commented Sep 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (e353aee)

Verdict: Changes require fixes for 2 findings.

Medium

  • cmd/msgvault/cmd/build_cache.go:558: The identity projection converts an invalid participant primary email to NULL, so the email-identifier guard treats that participant as having no primary email. A valid email-typed alias can therefore incorrectly mark the participant and its messages as owned, unlike the canonical store predicate, which treats any nonblank raw email as suppressing the alias. Carry raw primary-email presence separately on both scanner and CSV paths, and use that flag for the guard while keeping the invalid comparison key NULL; add a regression case with an invalid primary email and valid email alias.

    Reported by: codex

  • cmd/msgvault/cmd/cache_text.go:91: The documented recovery command does not repair several identity fields handled by this change: message_recipients.email_address, account_identities.address, and participant_identifiers.identifier_type/value. Running repair-encoding as instructed can therefore leave damaged addresses excluded from attribution and exported as unknown. Add repair passes with the required attribution/revision updates, or narrow the warning and documentation to fields the command actually repairs and document a supported recovery for the remaining fields.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 6s | Total: 8m23s

@salmonumbrella
salmonumbrella force-pushed the fix/cache-export-valid-utf8 branch from e353aee to 1df3891 Compare September 25, 2026 17:24
@roborev-ci

roborev-ci Bot commented Sep 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (1df3891)

Verdict: Changes require fixes for 2 findings.

Medium

  • docs/usage/querying.md:69: The documented recovery path does not repair invalid message_recipients.email_address envelope snapshots, so rebuilding after repair-encoding still exports those addresses as NULL. Add a safe repair pass for damaged envelope snapshots, or remove the claim that repair-encoding restores them and document the remaining limitation.

    Reported by: codex

  • cmd/msgvault/cmd/repair_encoding.go:425: Sanitizing distinct invalid RFC 822 Message-IDs to U+FFFD can collapse them into one value, causing duplicate detection to group unrelated messages. Detect repaired-ID collisions and leave conflicting values unresolved, or use a collision-free encoding for invalid identifier bytes.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 6s | Total: 7m41s

@salmonumbrella

Copy link
Copy Markdown
Contributor Author

Thanks @fucx

@wesm

wesm commented Sep 26, 2026

Copy link
Copy Markdown
Member

rebasing

@wesm wesm self-assigned this Sep 26, 2026
@wesm
wesm force-pushed the fix/cache-export-valid-utf8 branch 2 times, most recently from cfe7c11 to 4f2782e Compare September 26, 2026 21:34
@roborev-ci

roborev-ci Bot commented Sep 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (4f2782e)

Verdict: Changes require fixes for 3 findings.

Medium

  • cmd/msgvault/cmd/build_cache.go:1508: source_message_id is an archive-unique identifier, but exporting it through cacheTextSQL can map distinct invalid byte sequences such as a\x80 and a\x81 to the same a� value, colliding explore entry keys and making cached source-ID identity ambiguous.

    Fix: Preserve identifier values with a reversible encoding, or export invalid values as unknown and use the local message ID as the fallback key.

  • cmd/msgvault/cmd/repair_encoding.go:425: repair-encoding rewrites malformed RFC822 Message-ID values with U+FFFD replacement, so distinct invalid byte sequences—or an invalid ID and an existing ID containing U+FFFD—can become identical. Later deduplication or IMAP identity resolution may affect the wrong archive row.

    Fix: Do not repair identity fields with lossy replacement. Preserve invalid IDs as unusable keys or encode their bytes reversibly in a collision-free namespace, use that representation consistently, and detect and reject identity collisions before updating rows.

  • docs/usage/querying.md:66: The documentation claims msgvault repair-encoding restores damaged recipient addresses, but the command only repairs participants.email_address; authoritative message_recipients.email_address envelopes, account_identities.address, and participant identifier keys remain unrepaired, potentially leaving messages unattributed.

    Fix: Add collision-safe repair passes for the affected identity columns, reconstructing envelope addresses from raw messages where possible, or narrow the documentation to fields the command actually repairs.


Reviewers: codex, codex (security) | Synthesis: codex, 12s | Total: 10m4s

@shntnu

shntnu commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

I reproduced the cache failure with a legacy invalid UTF-8 rfc822_message_id in the forced CSV snapshot path. This PR already covers that column, so I am keeping my narrower workaround out of a separate upstream PR.

A small synthetic regression case is available in this commit: set MSGVAULT_FORCE_CSV_SNAPSHOT=1, write CAST(X'80' AS TEXT) into the fixture's Message-ID, and build the full cache. The assertions check that all fixture messages are exported, the cached value is U+FFFD, and the original SQLite value still has hex 80.

That test passes with the narrow workaround. I have not run it against this PR's branch.

Sanitize text as it enters the analytics cache, preserve valid text and
NULLs, report repair operations after successful builds, and explain the
repair path when fast search encounters an older invalid cache, including
conversation freshness fingerprints.

Identity attribution keys — account identity addresses, participant
emails, identifier types and values, and recipient envelope addresses and
types — are excluded instead of repaired when their stored bytes are
invalid UTF-8: they never match and export as unknown. A U+FFFD
placeholder would let two distinct invalid byte sequences collide onto
one key and mis-attribute messages to the owner, and DuckDB's CSV
snapshot fallback cannot carry invalid bytes, so it writes NULL for those
columns and both snapshot paths apply the same collision-free rule.

A recorded From envelope stays authoritative even when its bytes are
damaged: envelope presence is byte-level (SQLite TRIM semantics over raw
bytes on the sqlite_scanner path; an envelope_present column computed by
SQLite on the CSV path), so a present-but-invalid envelope never matches
an identity, still suppresses the participant fallback, and the
recipients export leaves both address columns NULL instead of
substituting the participant's current address. 'msgvault
repair-encoding' followed by a full rebuild restores attribution for
damaged archives.

- test(vector): satisfy test sleep checks after rebase

The rebased test suite uses synctest for in-process usage accounting and
documents the real delay needed by the external SQLite worker.

Generated with Codex
Co-authored-by: Wes McKinney <wesmckinn+git@gmail.com>
Co-authored-by: Codex <noreply@openai.com>
@wesm
wesm force-pushed the fix/cache-export-valid-utf8 branch from 4f2782e to f56b7ae Compare September 27, 2026 00:48
@roborev-ci

roborev-ci Bot commented Sep 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (f56b7ae)

Verdict: Changes require fixes for 3 findings.

Medium

  • cmd/msgvault/cmd/build_cache.go:569: Invalid participant primary emails are converted to NULL before the primary-email guard runs. A nonblank invalid email with a matching valid email identifier can therefore be treated as having no primary email, causing incorrect from-me classification and owner_participants entries.

    Fix: Preserve nonblank primary-email presence separately from the sanitized comparison key on both snapshot paths, and use that presence in the email-identifier guard.

  • cmd/msgvault/cmd/cache_text.go:92: The advertised repair path does not repair invalid message envelope addresses, account identity addresses, or participant identifier fields. Identity-only corruption can remain excluded or unknown without a repair warning, contradicting the claim that repair-encoding restores attribution.

    Fix: Add safe repair passes and reporting for every identity and envelope field, or remove those fields from the remediation claim and explain the required manual recovery.

  • cmd/msgvault/cmd/repair_encoding.go:425: Invalid RFC 822 Message-ID values are rewritten lossy, allowing distinct archived messages or an invalid value and a literal U+FFFD value to collide in authoritative identity lookups, membership resolution, reply-parent lookup, or deduplication.

    Fix: Do not lossy-rewrite authoritative Message-ID fields; leave invalid IDs unmatchable and omit them from identity-based cache operations, or use reversible collision-free surrogates. If rewriting remains, detect and report all collisions before committing.


Reviewers: codex, codex (security) | Synthesis: codex, 7s | Total: 8m47s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants