Skip to content

fix(whatsapp): store history sync delivered as JoinedGroup events - #74

Open
jqueguiner wants to merge 2 commits into
MaximeGaudin:mainfrom
jqueguiner:jl/fix-whatsapp-history-sync
Open

fix(whatsapp): store history sync delivered as JoinedGroup events#74
jqueguiner wants to merge 2 commits into
MaximeGaudin:mainfrom
jqueguiner:jl/fix-whatsapp-history-sync

Conversation

@jqueguiner

Copy link
Copy Markdown

What

WhatsApp history sync is never stored. This wires it back up.

wa-rs 0.2 does not dispatch Event::HistorySync. It streams the backfill one
conversation at a time through Event::JoinedGroup(LazyConversation) — see
wa-rs/src/history_sync.rs:

// Receive and dispatch lazy conversations as they come in
let lazy_conv = LazyConversation::from_bytes(raw_bytes);
self.core.event_bus.dispatch(&Event::JoinedGroup(lazy_conv));

Event::HistorySync still exists in the enum, so the arm matching it in
connector_trait.rs kept compiling — it just never fired. Everything WhatsApp
pushed after pairing was dropped on the floor.

Why it went unnoticed

The failure is silent in both directions: no error, no warning, and the
connector reports healthy. The only visible trace is a gap between what the
library logs and what the connector logs.

On a fresh pairing, before the fix:

$ grep -c "History sync progress" void-sync.log     # wa-rs parsed it
46
$ grep -cE "\[whatsapp:.*\] history" void-sync.log  # void stored it
0

wa-rs reported History sync progress: 775 conversations processed... and
Processing history sync ... (Size: 1087297, Type: Recent) across three
Recent blobs plus one Full. The database ended up with 4 conversations
and 14 messages
, all from the live stream after the backfill finished.

This matters more than a normal dropped event: WhatsApp only sends the full
history once, right after a device is linked. Missing it means unlinking and
relinking the device to get another chance.

How

  • Split the per-conversation body of handle_history_sync into
    store_conversation(db, connection_id, own_identity, conv).
    LazyConversation::conversation() yields a wa::Conversation, which is
    exactly the type that loop already consumed, so the storage logic is
    unchanged — it just runs one conversation at a time.
  • Call it from a new Event::JoinedGroup arm.
  • Keep the Event::HistorySync arm. It costs nothing and resumes working if a
    future wa-rs dispatches it again.
  • Report progress as a cumulative counter every 250 messages instead of one
    line per conversation, since a backfill carries hundreds.

Verified

./scripts/check.sh (fmt + clippy -D warnings + tests):

==> All pre-flight checks passed
RC=0

Built --release and confirmed the new arm compiles into the binary. I have not
yet been able to verify a full end-to-end backfill against a live account: that
requires unlinking and relinking the device, which destroys the session I am
currently running on. Happy to do it if you would rather have that before
merging — the reasoning above is from reading wa-rs and from the logs of the
failed run, not from a successful one.

Not in this PR

  • No change to the message filter in store_conversation. Messages with
    neither text nor media are still skipped, which is correct for receipts and
    system messages, but it does mean the stored count is lower than the count
    wa-rs reports.
  • No retry or on-demand re-request of history. If the backfill is missed (for
    example the daemon is not running during pairing), relinking the device is
    still the only way to get it again.

wa-rs 0.2 never dispatches `Event::HistorySync`. It streams the backfill
one conversation at a time through `Event::JoinedGroup(LazyConversation)`
(see wa-rs `history_sync.rs`: "Receive and dispatch lazy conversations as
they come in"). The variant still exists in the enum, so the arm matching
it kept compiling while receiving nothing, and every conversation WhatsApp
pushed after pairing was dropped.

Measured on a fresh link: wa-rs logged "History sync progress: 775
conversations processed" while only 4 rows reached the database, and the
handler's own log line never appeared once.

Split the per-conversation body out of `handle_history_sync` into
`store_conversation` and call it from the `JoinedGroup` arm, so history is
persisted as it streams in. The `HistorySync` arm is kept: it costs
nothing and resumes working if wa-rs dispatches it again.

Progress is reported as a cumulative counter every 250 messages rather
than one line per conversation, since a backfill carries hundreds.

@MaximeGaudin MaximeGaudin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review — #74: fix(whatsapp): store history sync delivered as JoinedGroup events

Recommendation: Request changes
Author: jqueguiner · +63/−10 across 3 files · CI: all pass

1. Intent & fit

Fits. Well-written bug fix for a serious silent failure: wa-rs 0.2 streams history sync as Event::JoinedGroup(LazyConversation) instead of Event::HistorySync, so the existing handler was dead code and 775 conversations were dropped on a fresh link. Single focused commit, conventional commit style, CHANGELOG entry added.

2. Security · Verdict: clean

No dependency changes, no CI/workflow changes, no dangerous patterns. Only control flow changes inside the existing WhatsApp event handler + a pure refactor in sync.rs. AtomicU64 for progress counter is benign.

3. Code review

Blocker: lazy_conv.conversation() clears messages — the fix stores zero messages

connector_trait.rs:193 calls lazy_conv.conversation(). In wa-rs-core (types/events.rs:87-95), this method clears conv.messages after decoding as a memory optimization:

// wa-rs-core/src/types/events.rs
pub fn conversation(&self) -> &wa::Conversation {
    self.parsed.get_or_init(|| {
        let mut conv = wa::Conversation::decode(&self.raw_bytes[..])
            .expect("Failed to decode conversation");
        conv.messages.clear();       // ← messages stripped
        conv.messages.shrink_to_fit();
        conv
    })
}

store_conversation then iterates conv.messages (now empty) and stores nothing. The Ok(0) branch silently swallows this. The fix compiles and passes CI but is functionally identical to the current broken state — conversation metadata is stored, but zero messages.

Fix: Use lazy_conv.get() instead, which preserves messages and returns Option<&WaConversation>:

Event::JoinedGroup(lazy_conv) => {
    let own_identity = own_identity_holder.lock().expect("mutex").clone();
    if let Some(conv) = lazy_conv.get() {
        match store_conversation(&db, &config_id, &own_identity, conv) {
            Ok(0) => {}
            Ok(n) => {
                let hist = history_count.fetch_add(n, Ordering::Relaxed) + n;
                if hist % 250 < n {
                    eprintln!(
                        "[whatsapp:{config_id}] history sync: {hist} messages imported"
                    );
                }
            }
            Err(e) => warn!("Failed to store history conversation: {e}"),
        }
    }
}

get() also returns None instead of panicking on malformed protobuf, which is a robustness bonus.

Should-fix

  • French commentconnector_trait.rs:97-98. All existing comments are in English. Replace with: "Cumulative counter of imported history messages, shared across handler calls (one per conversation during a backfill)."

Nits

  • Vestigial { } block in store_conversationsync.rs:67. The extra braces wrapped the original for loop body and serve no purpose after extraction.
  • "history sync" progress labelconnector_trait.rs:202. Event::JoinedGroup may also fire for actual group joins, not just history backfill. Low impact since the 250-message threshold filters out small group joins.

Summary

Excellent root-cause analysis — the diagnosis of wa-rs 0.2's event dispatch change is precise and well-documented. But LazyConversation::conversation() strips conv.messages as a memory optimization, so the fix as written stores conversation metadata but zero messages. Switching to lazy_conv.get() fixes both the correctness bug and adds robustness against malformed protobuf. This must be fixed and ideally end-to-end verified before merge.

…ion()

Review catch on MaximeGaudin#74: the JoinedGroup arm decoded the backfill with
LazyConversation::conversation(), which clears conv.messages after decoding
as a memory optimisation (wa-rs-core 0.2, types/events.rs:87-96). The handler
then stored conversation metadata and zero messages, so the fix was
functionally identical to the broken state it replaced, with a green CI.

Switch to LazyConversation::get(), which keeps the messages and returns None
on a malformed payload instead of panicking.

Also from the review:
- comment on the history counter was in French, now English like the rest
- drop the vestigial braces left in store_conversation after the extraction

Tests pin the trap so it cannot come back silently: one asserts get() keeps
the messages while conversation() empties them on the same payload, one
asserts store_conversation persists 3 messages from get(), one asserts it
persists 0 from conversation().
@jqueguiner

Copy link
Copy Markdown
Author

Good catch, and it was a real one. Pushed a8202f9.

The blocker

Confirmed against wa-rs-core-0.2.0/src/types/events.rs:87-96: conversation() calls conv.messages.clear() after decoding. The arm stored conversation rows and zero messages, so the fix was equivalent to the state it was meant to repair, with CI green. Switched to get().

I did not want to take that on trust twice, so the behaviour is now pinned by tests rather than by a comment:

Test Asserts
lazy_conversation_conversation_strips_messages_but_get_keeps_them same encoded payload: get() yields 2 messages, conversation() yields 0
store_conversation_from_lazy_get_persists_messages 3 messages encoded, 3 rows in the db, bodies in order
store_conversation_from_lazy_conversation_stores_nothing the bug you found, returns Ok(0)
lazy_conversation_get_returns_none_on_garbage get() returns None where conversation() panics

The third one is deliberately a test of the wrong call. If someone edits the arm back to conversation(), the first test fails loudly instead of the backfill disappearing in silence.

This needed prost as a dev-dependency to encode the protobuf fixtures. Version pinned to 0.14 to match what wa-rs-proto builds against.

Should-fix and nits

  • French comment on the counter, replaced with your wording.
  • Vestigial { } in store_conversation, removed. git diff -w on that file shows 2 deletions and nothing else, so the reindent moved no logic.
  • "history sync" label on Event::JoinedGroup: left as is. You are right that a real group join can land there, but it only prints past a cumulative 250 messages, which a group join will not reach. Renaming it to something neutral would make the backfill line less readable, which is the case that matters.

Verified

./scripts/check.sh (fmt + clippy -D warnings + tests):

==> All pre-flight checks passed
RC=0

103 tests in void-whatsapp, including the 4 new ones. GitHub CI on a8202f9 is sitting in action_required, it needs your approval to run since this is a fork PR.

Still not verified end to end

Unchanged from the original description, and it remains the weak point of this PR: no live backfill run. That requires unlinking and relinking the device, which destroys the session currently running. The tests above prove the decode path stores messages from a real encoded Conversation payload, not that WhatsApp's own blobs traverse it. Say the word if you want the live run before merge and I will take the session down for it.

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