refactor(sqlite-storage)!: replace bincode with prost for persisted blobs - #911
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR switches sqlite-storage BLOB persistence from bincode to prost-based wire encoding, updates sqlite-store reads and writes to use the new helpers, changes app-state missing-key retry flow, and adjusts key-share notification and mutation-MAC clearing behavior. ChangesSQLite wire encoding and app-state sync flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The SQLite backend persisted three BLOB columns (server cert chain, app-state sync keys, app-state hash state) via bincode. That duplicated serialization the workspace already has (prost for the wire protocol, serde_json for other blobs) and bincode's positional format is fragile for on-disk data across struct changes. Model the three blobs as protobuf with prost derive macros (no .proto file) in a new `wire` module, converting at the storage boundary so the wacore domain types stay untouched. Decode degrades gracefully on an undecodable blob (mirrors the existing cert-chain handling): the cert chain and app-state sync keys are treated as absent and re-fetched, app-state versions reset to re-sync from 0. The bincode->protobuf transition is therefore self-healing, no destructive migration needed. Drops the bincode dependency (and bincode_derive, virtue) entirely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195GwTrq6bA8rc9pLwyqfwb
478130c to
111c739
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@storages/sqlite-storage/src/wire.rs`:
- Around line 215-241: Add a new test function after
hash_state_default_roundtrips that validates the error handling in
decode_hash_state when provided with a hash of incorrect length. Create a test
named hash_state_rejects_wrong_hash_len that constructs a HashStateWire with a
hash vector that is not 128 bytes long (such as 64 bytes), encodes it, and
verifies that decode_hash_state returns an error rather than silently truncating
or accepting the invalid input. This mirrors the validation pattern at line
150-153 and provides coverage equivalent to the existing
server_cert_chain_rejects_wrong_key_len test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9ebcce6-01e2-41be-bfd1-ff90ad7cc549
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
storages/sqlite-storage/Cargo.tomlstorages/sqlite-storage/src/lib.rsstorages/sqlite-storage/src/sqlite_store.rsstorages/sqlite-storage/src/wire.rs
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Mirrors server_cert_chain_rejects_wrong_key_len so the HashState length invariant has the same negative coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195GwTrq6bA8rc9pLwyqfwb
![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: 478130c103
ℹ️ 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".
Merging this PR will improve performance by 15.33%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | send_message[20] |
5.9 ms | 5.1 ms | +15.33% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/remove-bincode-prost (7fd9f91) with main (c57cef7)
The bincode->protobuf switch leaves existing app_state_keys, app_state_versions and device.server_cert_chain rows undecodable. The app-state sync path hard-fails with KeyNotFound before the missing-key re-request runs, so a returned-absent key does not self-heal (flagged in review). Add an up-only migration that clears those rows so nothing undecodable remains: keys are re-requested from the primary device, versions re-sync from 0, and the cert chain is rebuilt on the next handshake. With the migration owning the transition, revert the app-state decode paths to propagate decode errors (their original behavior) instead of masking them as absent/default. The cert-chain decode stays tolerant, as it was before this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195GwTrq6bA8rc9pLwyqfwb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 351b50843f
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Replaces the destructive migration with read-time self-healing, per review: - App-state sync key / version decode treats an undecodable blob (an old bincode row or genuine corruption) as absent / default instead of fatal. - On KeyNotFound, the app-state sync path now requests the missing key from the primary device before retrying, so an absent key is re-shared and the next write persists it as protobuf. Previously the request only ran after a successful decode, so a missing key never recovered. Old bincode rows are handled lazily at read with no destructive migration; the migration added earlier is removed. The cert-chain decode was already tolerant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195GwTrq6bA8rc9pLwyqfwb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a388ea8d4
ℹ️ 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".
Addresses two review findings on the self-heal path: - decode_app_state_sync_key rejects key material that isn't 32 bytes. An old bincode row can occasionally parse as protobuf with garbage key data; accepting it would derive bad sub-keys and fail later with MAC errors instead of being treated as absent and re-requested. - On KeyNotFound the retry arms the key-share listener before sending the request and always waits for the share, and every key share now notifies (not just the first). Repairing multiple missing keys now works instead of only the first collection recovering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195GwTrq6bA8rc9pLwyqfwb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 404072fdc9
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/app_state.rs (1)
91-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd diagnostic logging when base64 decode fails.
Look, if the key ID is malformed and we can't decode it, we need to know about it. Right now if
STANDARD_NO_PAD.decode(id_b64)fails, we silently skip the wait and continue - no visibility into what went wrong. For a billion-user platform, silent failures are not acceptable.🔧 Proposed fix to add diagnostic logging
use base64::Engine as _; - if let Ok(key_id) = - base64::engine::general_purpose::STANDARD_NO_PAD.decode(id_b64) - { + match base64::engine::general_purpose::STANDARD_NO_PAD.decode(id_b64) { + Ok(key_id) => { let listener = self.initial_keys_synced_notifier.listen(); self.request_missing_keys_with_dedup(vec![key_id]).await; debug!(target: "Client/AppState", "App state key missing for {:?}; requested it, waiting up to 10s for key share then retrying", name); if rt_timeout(&*self.runtime, Duration::from_secs(10), listener) .await .is_err() { warn!(target: "Client/AppState", "Timeout waiting for key share for {:?}; retrying anyway", name); } } + Err(decode_err) => { + warn!(target: "Client/AppState", "Failed to decode missing key ID for {:?}: {decode_err}", name); + } + } continue;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/app_state.rs` around lines 91 - 103, The base64 decode operation for the key ID (STANDARD_NO_PAD.decode(id_b64)) has no error handling, causing silent failures when the key ID is malformed. Add an else clause to the if let Ok(key_id) pattern to log a diagnostic warning when the base64 decode fails, including the malformed id_b64 value and a clear error message. This will provide visibility into why the key ID could not be processed instead of silently skipping the key synchronization attempt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/app_state.rs`:
- Around line 91-103: The base64 decode operation for the key ID
(STANDARD_NO_PAD.decode(id_b64)) has no error handling, causing silent failures
when the key ID is malformed. Add an else clause to the if let Ok(key_id)
pattern to log a diagnostic warning when the base64 decode fails, including the
malformed id_b64 value and a clear error message. This will provide visibility
into why the key ID could not be processed instead of silently skipping the key
synchronization attempt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 84d8a8c2-8011-46fa-b538-a1cfbc25255b
📒 Files selected for processing (4)
src/client/app_state.rssrc/message/special.rsstorages/sqlite-storage/src/sqlite_store.rsstorages/sqlite-storage/src/wire.rs
The KeyNotFound the retry branch caught (AppStateSyncError) is not the error the decode path returns: a missing key surfaces as AppStateError::KeyNotFound from lookup_app_state_key and is stringified into an untyped anyhow, so the re-request never actually fired (raised in review). Instead, check get_missing_key_ids before process_parsed_patch_list: if the batch references a key the backend lacks (e.g. an old bincode row that now decodes as absent), request it, wait briefly for the primary to re-share, and re-fetch. This uses the real key ids and is independent of error types. The now-dead base64/request code in the retry branch is reverted to its original form. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195GwTrq6bA8rc9pLwyqfwb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 965fefba2a
ℹ️ 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".
The task-wide guard suppressed the upfront key request on later paginated pages, so a page encrypted with a different rotated key (e.g. across upgraded bincode rows) could hit KeyNotFound without ever requesting it. Reset the guard after each successfully processed batch so every page gets one repair attempt, while still bounding a single page to one request+wait cycle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195GwTrq6bA8rc9pLwyqfwb
The wire tests already reject wrong-length protobuf, but nothing exercised the store boundary that swallows a decode failure into absence. Corrupt the app-state sync-key and version blobs in the DB and assert get_sync_key reads None and get_version resets to default (version 0) without erroring -- the behavior the whole bincode->prost migration relies on.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
jlucaso1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 3960-4044: The current test only covers arbitrary corrupt bytes,
but it should verify the legacy no-migration behavior for real old bincode rows.
Update undecodable_blobs_self_heal_to_absent in sqlite_store.rs to insert
hardcoded legacy bincode fixtures for the sync-key and HashState storage paths,
then assert get_app_state_sync_key_for_device and
get_app_state_version_for_device treat them as None/default rather than errors,
and confirm the protobuf setters can overwrite those healed rows afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a506ed5f-302a-47b7-87c1-0adc463a2783
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/client/app_state.rsstorages/sqlite-storage/Cargo.tomlstorages/sqlite-storage/src/sqlite_store.rs
The missing-key repair waited 10s for a re-share after every request, but the per-key dedup can suppress the actual send (already asked within its 24h window). In that case no new re-share is coming from us, so the wait was dead time on the collection. request_missing_keys_with_dedup now reports whether it actually sent; the sync loop only waits when it did, otherwise it falls through and lets the page fail fast so the collection re-syncs on a later attempt -- closer to WA Web's non-blocking orphan-and-retry model.
…rite Replace the arbitrary-garbage self-heal test with the exact bytes bincode 2.0.1 (config::standard, via serde) produced for AppStateSyncKey and HashState before the migration. This proves the migration's core property: a real legacy row does not false-positive as a partially-decoded protobuf -- it reads back as absent / default, and the protobuf setters then overwrite the healed row. Keeps an arbitrary-corruption case too.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/app_state.rs (1)
512-532: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the missing-key preflight before external blob downloads.
Right now, a missing key is detected only after Line 457-492 downloads snapshots/mutations; when the request succeeds, Line 531 retries and throws those downloads away. Put this gate immediately after
parse_patch_list_refso large sync payloads don’t get downloaded twice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/app_state.rs` around lines 512 - 532, The missing-key preflight in app_state::sync_page is happening too late, after the external blob download work has already run, so successful retries discard that downloaded payload. Move the get_missing_key_ids/request_missing_keys_with_dedup gate to immediately after parse_patch_list_ref, before the snapshot/mutation fetch path, and keep the retry/timeout behavior tied to the same sync_page flow so large sync payloads are not downloaded twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/app_state.rs`:
- Around line 512-532: The missing-key preflight in app_state::sync_page is
happening too late, after the external blob download work has already run, so
successful retries discard that downloaded payload. Move the
get_missing_key_ids/request_missing_keys_with_dedup gate to immediately after
parse_patch_list_ref, before the snapshot/mutation fetch path, and keep the
retry/timeout behavior tied to the same sync_page flow so large sync payloads
are not downloaded twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9a7a8fd4-9561-4460-be85-e8f310e79de1
📒 Files selected for processing (2)
src/client/app_state.rsstorages/sqlite-storage/src/sqlite_store.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bad944ac1
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/app_state.rs (1)
434-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the guard comment to match the reset behavior.
Line 434 says “once per task,” but Line 575 resets the flag after a page decodes, so later pages in the same task can request missing keys again.
Suggested wording
- // Request the batch's missing keys at most once per task (see below). + // Avoid re-requesting the same missing keys while this page is stuck; + // reset after a page decodes so later key rotations can repair. let mut requested_missing_keys = false;As per coding guidelines, “When adding code comments, explain why not what, and keep them concise.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/app_state.rs` around lines 434 - 435, Update the guard comment near requested_missing_keys in app_state::AppState::request_batch so it matches the actual behavior: it is not limited to once per task because the flag is reset after each page decode. Rewrite the comment to concisely explain why the guard exists and that it only suppresses duplicate requests within a page decode cycle, referencing the requested_missing_keys reset path to keep the wording aligned with the code.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/app_state.rs`:
- Around line 434-435: Update the guard comment near requested_missing_keys in
app_state::AppState::request_batch so it matches the actual behavior: it is not
limited to once per task because the flag is reset after each page decode.
Rewrite the comment to concisely explain why the guard exists and that it only
suppresses duplicate requests within a page decode cycle, referencing the
requested_missing_keys reset path to keep the wording aligned with the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd810e5a-06ba-4968-941e-4637ee946840
📒 Files selected for processing (1)
src/client/app_state.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fd6fe68f9
ℹ️ 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".
An upgraded paired companion (its persisted bincode app-state rows now decode as absent) failed to re-sync: the self-heal re-request never reached the primary and the snapshot key was never requested. Three root causes, found by running the demo against a real migrated DB: - Target the PRIMARY for the key request. request_app_state_keys sent the peer message to device_snapshot.pn, which carries OUR device number, so it encrypted to ourselves (no self-session) and failed with 'session not found'. Address it to to_non_ad() (device 0), the key source we hold a session with from pairing -- mirrors whatsmeow's ownID.ToNonAD(). - Preflight missing keys before processing in BOTH sync paths (the batched server_sync path only requested AFTER process_patch_lists, which aborts with KeyNotFound first; the single path requested before download). Inline the external blobs FIRST via the new missing_key_ids_after_inline: the snapshot's key_id lives inside its blob, so checking before download misses it and the snapshot still fails. Only wait when a fresh request actually went out (the per-key dedup may suppress it). - Clear stale mutation MACs when seeding from a genesis patch. A version blob that reset to 0 keeps its pre-reset MACs; a v1 genesis patch served without a snapshot would anchor its ltHash to those stale index->value entries. The snapshot path already clears; this covers the no-snapshot genesis path. Tests: real external-snapshot key visibility after inline, genesis-patch MAC clear, plus the store-level legacy-bincode self-heal already added.
de76576 to
3862b64
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3862b647a5
ℹ️ 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".
…nc key Outbound chat-action mutations encrypt with the latest app-state sync key (get_latest_sync_key_id -> build_patch / send_app_state_patch). The query returned the lexicographically-highest key_id without decoding it, so on an upgraded DB a stale legacy-bincode row that sorts higher (and was never repaired because no incoming patch referenced it) would be selected, then fail later in get_app_state_key with KeyNotFound -- breaking a pin/mute/archive push. Load the candidates in desc order and return the first whose blob actually decodes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/app_state.rs`:
- Around line 594-610: `request_keys_and_wait` currently treats a deduped
missing-key request as “nothing to do,” which lets callers decode immediately
and re-hit `KeyNotFound` for already-missing keys. Update the
`AppState::request_keys_and_wait` flow so it distinguishes between sent,
deduped, and failed outcomes from `request_missing_keys_with_dedup`, and make
the deduped path still wait briefly and trigger a refetch before proceeding.
Ensure the callers that rely on this helper continue to use the returned boolean
to decide whether to refetch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 858fa91e-e77a-4a62-920e-b3f65014c185
📒 Files selected for processing (4)
src/appstate_sync.rssrc/client/app_state.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/appstate_sync.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fabcd3f51
ℹ️ 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".
…quest request_keys_and_wait returned true just because the request was sent, ignoring the timeout/notification result. A delayed share (or a wake from an UNRELATED key share) then let the callers process with the row still absent -> KeyNotFound; and since the per-key request was stamped for 24h, the deduped retries returned false and skipped the wait entirely, so the collection never recovered. Now the helper waits even when the dedup suppressed the send (an earlier request may still be in flight) and RE-VERIFIES every requested key is actually stored before returning true. The callers drop the once-per-task guard and skip the collection gracefully when keys are still missing (it re-syncs on a later cycle) instead of aborting with KeyNotFound. Addresses Codex P1 + CodeRabbit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 268723731a
ℹ️ 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".
| } | ||
| if !missing_all.is_empty() && !self.request_keys_and_wait(missing_all).await { | ||
| warn!(target: "Client/AppState", "app-state key(s) still missing after request; skipping this batch, will retry on a later sync"); | ||
| return Ok(()); |
There was a problem hiding this comment.
When the requested app-state key share does not arrive within the 10s wait, this returns Ok(()) from the batched sync even though none of the pending collections were processed. The initial critical-sync path in src/client/node_io.rs treats sync_collections_batched(...) success as permission to dispatch Connected, so an upgraded device with missing/corrupt persisted keys can skip CriticalBlock/CriticalUnblockLow state without scheduling a retry until some unrelated dirty/server sync happens. Return an error or keep these collections pending instead of reporting success.
Useful? React with 👍 / 👎.
The previous commit returned Ok(()) from sync_collections_batched when the re-shared key didn't arrive in 10s. The initial critical-sync path in node_io treats that Ok as permission to cancel its 180s retry watchdog and dispatch Connected, so an upgraded device with corrupt persisted keys would surface Connected with CriticalBlock/CriticalUnblockLow unsynced and no scheduled retry. Return an error from both sync paths instead, so the caller keeps the watchdog (or logs and retries on the next server_sync) rather than treating the collection as synced. Already-repaired keys/pages stay persisted, so the retry resumes cheaply. Addresses Codex P1.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fd9f910e6
ℹ️ 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".
2 participants
What
Removes the
bincodedependency fromwhatsapp-rust-sqlite-storageand models the three persisted BLOB columns with protobuf viaprostderive macros (no.protofile) instead.The SQLite backend serialized three blobs with bincode:
device.server_cert_chainCachedServerCertChainapp_state_keys.key_dataAppStateSyncKeyapp_state_versions.state_dataHashStateWhy
prost(the wire protocol) andserde_json(e.g.GroupInfo,Device). bincode was the only serializer here that wasn't one of those, for 3 tiny cold blobs.prostis field-tagged and tolerant of additive changes.bincode,bincode_deriveandvirtue.prostwas already a workspace dependency, so no new external crate is added. (Binary-size CI: net smaller, -4 deps.)How
wiremodule in the storage crate with four#[derive(Clone, PartialEq, prost::Message)]structs andencode_*/decode_*helpers.wacoredomain types are untouched (the fixed-size[u8; 32]cert key and[u8; 128]hash state becomebyteson the wire, with length checks on decode). The app-state sync key decode also rejects non-32-byte key material so a bincode row that happens to parse as protobuf with garbage isn't accepted.Handling existing (bincode) data — self-healing, no migration
Old rows are bincode-encoded and can't be decoded as protobuf. Rather than a destructive migration, the transition self-heals lazily:
Nonefor a sync key,HashState::default()for a version,Nonefor the cert chain.process_app_state_sync_tasknow checksget_missing_key_idsbefore decoding a batch; if a referenced key isn't in the backend (e.g. a now-absent old row), it sends anAppStateSyncKeyRequest, waits briefly for the primary to re-share, and re-fetches. This runs before patch/snapshot processing (which would otherwise fail on the missing key with an untyped error), uses the real key ids, and is independent of error types. Key-share receipt now notifies on every share, so repairing multiple keys works.Net effect after upgrade: app-state keys are re-requested and overwritten as protobuf, app-state versions re-sync from 0, and the cert chain is rebuilt on the next handshake — without deleting anything.
Tests
test_server_cert_chain_survives_save_load_roundtrip(full DB save/reopen/load) exercises the prost path.wirefor all three types, plus wrong-length rejection tests for the cert key, the hash state, and the sync-key material.cargo clippy --workspace --all-targetsandcargo test -p whatsapp-rust-sqlite-storagepass.