-
Notifications
You must be signed in to change notification settings - Fork 0
docs: cover PR #1058's history-sync visitor API and MsgSecretEntry breaking change #416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -360,7 +360,7 @@ async fn put_msg_secret( | |||||
| chat: &str, | ||||||
| sender: &str, | ||||||
| msg_id: &str, | ||||||
| secret: &[u8], | ||||||
| secret: &[u8; 32], // MessageSecret, the protocol-fixed message-secret size | ||||||
| ) -> Result<()>; | ||||||
|
|
||||||
| /// Batch upsert. Each MsgSecretEntry carries its own absolute expires_at | ||||||
|
|
@@ -390,7 +390,20 @@ async fn get_msg_secret_with_ts( | |||||
| async fn delete_expired_msg_secrets(&self, cutoff_timestamp: i64) -> Result<u32>; | ||||||
| ``` | ||||||
|
|
||||||
| `MsgSecretEntry` is `{ chat, sender, msg_id, secret, expires_at, message_ts }`. The SQLite table is: | ||||||
| ```rust | ||||||
| pub type MessageSecret = [u8; 32]; // wacore::reporting_token::MESSAGE_SECRET_SIZE | ||||||
|
|
||||||
| pub struct MsgSecretEntry { | ||||||
| pub chat: Arc<str>, // shared per conversation instead of one String per row | ||||||
| pub sender: Arc<str>, | ||||||
| pub msg_id: Arc<str>, | ||||||
| pub secret: MessageSecret, | ||||||
| pub expires_at: i64, | ||||||
| pub message_ts: i64, | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| `chat`, `sender`, and `msg_id` are `Arc<str>` rather than `String` so buffered batch inserts can clone entries cheaply, and `secret` is the fixed-size `MessageSecret` array rather than `Vec<u8>` — an invalid-length secret is unrepresentable instead of needing a runtime check. This is a breaking change for custom backends constructing `MsgSecretEntry` directly: build the JID/ID fields with `Arc::from(...)`/`.into()` and pass a `[u8; 32]` for `secret`. The SQLite table itself is unchanged: | ||||||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This paragraph combines both field-type changes, two separate rationales, compatibility impact, and migration instructions in long compound sentences. This conflicts with the applicable Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Split sentences and use the second person ("you") for instructions. The explanation combines multiple concepts (string types, cloning, array size, runtime checks) into a long run-on sentence and does not use the second-person perspective for the breaking change instruction. As per coding guidelines, documentation must keep sentences concise (one idea per sentence) and use the second person ("you") and active voice. ♻️ Proposed rewrite for clarity and compliance-`chat`, `sender`, and `msg_id` are `Arc<str>` rather than `String` so buffered batch inserts can clone entries cheaply, and `secret` is the fixed-size `MessageSecret` array rather than `Vec<u8>` — an invalid-length secret is unrepresentable instead of needing a runtime check. This is a breaking change for custom backends constructing `MsgSecretEntry` directly: build the JID/ID fields with `Arc::from(...)`/`.into()` and pass a `[u8; 32]` for `secret`. The SQLite table itself is unchanged:
+The `chat`, `sender`, and `msg_id` fields are `Arc<str>` rather than `String`. This allows buffered batch inserts to clone entries cheaply. The `secret` field uses the fixed-size `MessageSecret` array rather than `Vec<u8>`. This makes an invalid-length secret unrepresentable. You no longer need to perform a runtime length check. This is a breaking change if you construct `MsgSecretEntry` directly in a custom backend. You must build the JID/ID fields with `Arc::from(...)` or `.into()`. You must also pass a `[u8; 32]` for `secret`. The SQLite table itself is unchanged:📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||
|
|
||||||
| ```sql | ||||||
| CREATE TABLE msg_secrets ( | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -525,6 +525,7 @@ graph LR | |
| 8. **Secret-presence pre-scan** — before running buffa decode on each `HistorySyncMsg`, a shallow varint walk checks whether the message carries `message_secret` at any level (`WebMessageInfo.message_secret` or `Message.message_context_info.message_secret`). Messages without a secret (the majority in production blobs) are discarded immediately — no struct decode, no allocation. The scan mirrors protobuf merge semantics so repeated field occurrences and malformed bytes are handled identically to a full decode | ||
| 9. **Shared conversation id** — `HistoryMsgSecretRecord.chat_id` is `Arc<str>`, allocated once per conversation and reference-counted into every record within that conversation (10k clones → 500 on the bench fixture). `msg_id` uses `CompactString` (inline for typical 20–22 char WA IDs) and `secret` uses `SecretBytes` (inline for secrets ≤32 bytes) | ||
| 10. **Filter-before-materialize retention hook** — `process_history_sync_bytes_filtered` runs a caller-supplied retention predicate against a borrowed `HistoryMsgSecretRecordRef` (a zero-copy view into the not-yet-built record) before allocating the owned, heap-backed `HistoryMsgSecretRecord`. A record the predicate rejects is discarded without ever being materialized. `process_history_sync_bytes` — and `process_history_sync` itself — keep their existing accept-all behavior by wrapping the filtered entry point with an always-`true` predicate, so callers that don't own a retention policy are unaffected. Bench (synthetic 500-conversation blob, upstream PR's rejection-heavy fixture): allocation churn 21.61 MiB → 14.00 MiB, allocation count ~84k → ~26k. The reduction scales with how much of the record set the predicate rejects — the default accept-all behavior sees none of it. See [`wacore` — history_sync types](/api/wacore#history_sync-types) for the exported signatures | ||
| 11. **Streaming record-visitor path** — `process_history_sync_bytes_with_record_visitor` (closure-based) and `process_history_sync_bytes_with_record_sink` (trait-based, via `HistoryMsgSecretRecordVisitor`) go further than the filter hook above: the caller builds its own storage row directly from the borrowed `HistoryMsgSecretRecordRef`, so the owned `HistoryMsgSecretRecord` is never allocated at all, even for accepted records. The visitor trait's optional `reserve`/`retained_item_size` hooks let a capacity-aware caller (e.g. a batched SQL insert buffer) size its own collection up front. Bench (allocator-instrumented synthetic history extraction): 20.20 MiB → 14.43 MiB allocated (-28.6%); CodSpeed history stream-drain memory: 243.8 KB → 115.2 KB (2.1× less). See [`wacore` — history_sync types](/api/wacore#history_sync-types) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The destination heading is Useful? React with 👍 / 👎.
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| ### Skip mode | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.