Skip to content

Add sync error logging and --rebuild-cache flag#227

Open
robelkin wants to merge 3 commits intowesm:mainfrom
robelkin:fix/sync-error-logging-and-cache-rebuild
Open

Add sync error logging and --rebuild-cache flag#227
robelkin wants to merge 3 commits intowesm:mainfrom
robelkin:fix/sync-error-logging-and-cache-rebuild

Conversation

@robelkin
Copy link
Copy Markdown
Contributor

Summary

  • Log message IDs at WARN level when GetMessagesRawBatch returns nil (message fetch failed), in both incremental and full sync paths. Previously the error count incremented silently with no indication of which messages were affected.
  • Add --rebuild-cache flag to sync and sync-full commands to optionally rebuild the Parquet analytics cache after sync. The serve command already does this automatically, but standalone CLI invocations (e.g. via launchd) left the cache stale.

Usage: msgvault sync --rebuild-cache or msgvault sync-full --rebuild-cache

🤖 Generated with Claude Code

When GetMessagesRawBatch returns nil for a message (typically a 404
because the message was deleted between listing and fetching), the
error count was incremented but no message ID was logged. This made
it impossible to diagnose which messages failed to sync.

Add Warn-level logging with the message ID in both incremental and
full sync paths so failed fetches are visible in logs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The serve command rebuilds the Parquet analytics cache after each
sync, but the standalone CLI commands did not. This caused stale
search results when syncs were triggered via launchd or manual CLI.

Rather than always rebuilding (which couples sync and caching), add
an opt-in --rebuild-cache flag that users can enable in their
launchd config or scripts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 26, 2026

roborev: Combined Review (17efaac)

The PR safely introduces cache rebuilding and logging improvements, but contains a logic mismatch regarding the new rebuild flag's behavior.

Medium

  • Location: cmd/msgvault/cmd/sync.go:177, cmd/msgvault/cmd/ syncfull.go:169
  • Problem: The new --rebuild-cache flag does not actually force a rebuild; it only runs buildCache when cacheNeedsBuild(...).NeedsBuild is true. If a user invokes the flag to recover from a bad-but-
    not-detected cache state, the command will silently skip the rebuild even though the flag/help text says it will rebuild the analytics cache.
  • Fix: Make --rebuild-cache bypass the staleness check and rebuild unconditionally, or rename/document it as a conditional refresh instead of a rebuild.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

When the flag is set, always run buildCache rather than gating on
cacheNeedsBuild. The staleness check is still consulted to decide
whether a full rebuild is needed, but the cache build itself runs
unconditionally so users can recover from bad-but-undetected cache
states.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 26, 2026

roborev: Combined Review (2a9aa80)

The PR introduces a high-severity compilation error and medium-severity error handling
issues that must be addressed before merging.

High

  • Location: cmd/msgvault/cmd/sync.go (around line 180), cmd/msgvault/cmd/syncfull.go (around line 172)
  • Problem: The code attempts to assign
    the result of cacheNeedsBuild(dbPath, analyticsDir) to a single variable staleness and access a FullRebuild field. However, cacheNeedsBuild returns two values (bool, string), not a struct. This results in a compilation error: "assignment mismatch:
    1 variable but cacheNeedsBuild returns 2 values" and "staleness.FullRebuild undefined".
  • Fix: Remove the invalid staleness check. To force an incremental cache rebuild (which exports newly synced messages), simplify the logic to directly call buildCache(dbPath, analyticsDir, false) .

Medium

  • Location: cmd/msgvault/cmd/sync.go:177, cmd/msgvault/cmd/syncfull.go:169
  • Problem: --rebuild-cache failures are downgraded to warnings, so the command can exit
    successfully even when the explicitly requested cache rebuild did not happen. That makes automation and user feedback inaccurate for this flag.
  • Fix: Propagate buildCache errors as command failures when --rebuild-cache is set, or at minimum return a non-zero exit after sync completes.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants