feat(client): report which <enc> failed to decrypt, and why - #1261
Conversation
The client already reported the half of each <enc> that worked: Event::DecryptedPayload carries enc_index, enc_type and the plaintext. The half that failed was only reported per message, once, and without a cause — so a consumer following decryption could say "this <enc> produced these bytes" but not "this <enc> failed for this reason", and on a fan-out stanza not even which copy failed. Event::EncDecryptFailed closes that asymmetry: info, enc_index, enc_type and an EncDecryptFailureReason, dispatched from every branch of the receive path that abandons an <enc>, behind its own forwarding lease. enc_index is the same numbering DecryptedPayload uses, produced by the same enumeration. UndecryptableMessage is untouched: it is per message, deduplicated by (chat, id), and its decrypt_fail_mode is the server's display hint. It answers a different question and has consumers relying on its single-flight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds per- ChangesEncrypted Decryption Failure Events
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PluginSubscription
participant Client
participant MessageReceive
participant EventDispatcher
PluginSubscription->>Client: acquire_enc_decrypt_failed_forwarding()
MessageReceive->>Client: classify encrypted-payload failure
Client->>EventDispatcher: dispatch Event::EncDecryptFailed
PluginSubscription->>Client: release forwarding lease
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
|
| Filename | Overview |
|---|---|
| src/message/receive.rs | Adds per-payload failure dispatch throughout classification, session/group decryption, migration, skip, teardown, and plaintext-processing branches. |
| src/message.rs | Centralizes Signal error classification and preserves terminal migration retry causes. |
| src/message/msg_secret.rs | Distinguishes malformed bot envelopes, missing secrets, storage failures, authentication failures, and unusable plaintext. |
| src/message/retry.rs | Adds gated construction and dispatch of typed per-<enc> failure events. |
| wacore/src/types/events.rs | Adds the append-only event kind, frozen builder payload, and non-exhaustive failure taxonomy. |
| src/client.rs | Adds the weak client lease and independent atomic counter controlling failure-event forwarding. |
| src/plugins/mod.rs | Integrates the new gated event kind with plugin subscription acquisition, updates, and retirement. |
| wacore/src/bot_message.rs | Replaces opaque bot-message errors with typed stages used for accurate failure attribution. |
| src/store/signal_adapter.rs | Rebrands locally stored record-decoding failures at the storage boundary to prevent blaming peer ciphertext. |
| wacore/libsignal/src/protocol/error.rs | Adds a crate-local predicate distinguishing corrupt stored sessions from closed receiver-chain errors. |
| src/message/tests.rs | Adds broad coverage for failure branches, indexing, redelivery, classification boundaries, and forwarding leases. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming message stanza] --> B[Classify enc nodes and assign enc_index]
B --> C{Node recognized?}
C -- No --> D[Dispatch EncDecryptFailed with node reason]
C -- Yes --> E{Processing permitted?}
E -- No --> F[Dispatch NotAttempted]
E -- Yes --> G{Encryption type}
G --> H[Session decrypt]
G --> I[Group decrypt]
G --> J[Bot-message decrypt]
H --> K{Outcome}
I --> K
J --> K
K -- Plaintext accepted --> L[Dispatch DecryptedPayload]
K -- Decrypt failed --> M[Classify terminal failure]
K -- Plaintext unusable --> N[Dispatch PlaintextUnusable]
M --> O[Dispatch EncDecryptFailed]
Reviews (15): Last reviewed commit: "fix(client): report the encs a teardown ..." | Re-trigger Greptile
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49920f5a9b
ℹ️ 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.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
…ranches Review found three places where the reported reason contradicted its own definition: - A malformed `<enc>` envelope libsignal could not parse reported `SignalError` on both the session catch-all and the group arm, though it never reached a cipher. `is_malformed_envelope_error` is now shared by both so one libsignal error cannot be classified two ways. - A bot payload rejected on shape (bad IV length, payload too short for the GCM tag) reported `BadMac`, which would let malformed wire data count as an authentication failure against the peer. `decrypt_bot_message` now returns a typed `BotMessageError` and classifies its own failure stage, so the mapping lives beside the code that produces each variant. - `InvalidSignedPreKeyId` reached through the untrusted-identity retry reported `UntrustedIdentity` while the direct arm reported `UnknownPreKey`. Same terminal error, same cause now. All three are reporting-only: no retry reason, nack, or control flow moved. Also: assert the transport ack the all-unusable-stanza test claims, and extend the plugin gated-lease lifecycle test to cover EncDecryptFailed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (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: 9c626bbc11
ℹ️ 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 failure event Owner review, three corrections: - A stanza whose session `<enc>` was a duplicate had its `skmsg` decrypted on the first delivery, so skipping it now is the redelivery working, not a failure. `NotAttempted` moves inside the existing `!session_had_duplicates` guard, extending the duplicate rule already documented on the payload. A sibling `<enc>` that genuinely failed in the same batch is still reported. - `SignalProtocolError::BackendError` — what the store adapter wraps every backend error in — reported `SignalError` on both the session catch-all and the group arm, blaming the peer for our own disk. Both now read one `signal_error_reason`, which names it `StorageFailure`. - Reuse the existing `message_acks_for` test helper instead of a duplicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/message/receive.rs (1)
1285-1290: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep
StorageFailurestanzas recoverable.
signal_error_reasonreturnsStorageFailureforBackendError, but both callers then follow the generic terminal path and send a 500 NACK. That drops the stanza instead of leaving it for redelivery. This conflicts with theStorageFailureevent contract and can lose messages during a local store outage.
src/message/receive.rs#L1285-L1290: HandleStorageFailurebeforedispatch_undecryptable_eventandspawn_nack; leave normal stanzas recoverable.src/message/receive.rs#L1494-L1510: Apply the same non-terminal handling before the group generic NACK path.src/message/tests.rs#L14344-L14373: Inject backend failures through both paths and assert no terminal NACK is sent.🤖 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/message/receive.rs` around lines 1285 - 1290, Keep StorageFailure outcomes recoverable in both receive paths: in src/message/receive.rs#L1285-L1290, handle the StorageFailure result from signal_error_reason before dispatch_undecryptable_event or spawn_nack, and in src/message/receive.rs#L1494-L1510 apply the same non-terminal behavior before the group generic NACK path. Update src/message/tests.rs#L14344-L14373 to inject backend failures through both paths and assert that no terminal NACK is sent.
🤖 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/message/tests.rs`:
- Line 14257: Update the JID passed to AlicePeer::new in this test to use a
reserved fictional NANP number with a real NPA, the 555 exchange, and a
0100–0199 line number, such as the suggested 12025550104@s.whatsapp.net format.
---
Outside diff comments:
In `@src/message/receive.rs`:
- Around line 1285-1290: Keep StorageFailure outcomes recoverable in both
receive paths: in src/message/receive.rs#L1285-L1290, handle the StorageFailure
result from signal_error_reason before dispatch_undecryptable_event or
spawn_nack, and in src/message/receive.rs#L1494-L1510 apply the same
non-terminal behavior before the group generic NACK path. Update
src/message/tests.rs#L14344-L14373 to inject backend failures through both paths
and assert that no terminal NACK is sent.
🪄 Autofix
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: b0017270-8ce4-477c-ab68-1b93b4143765
📒 Files selected for processing (4)
src/message.rssrc/message/receive.rssrc/message/tests.rswacore/src/types/events.rs
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…control The reason named where the client stopped and then promised what happens next. Those are different questions: the two identity-flush sites leave the stanza queued, while a store read failure surfacing through the session catch-all takes that arm's existing 500 nack. A reason cannot encode a control-flow decision it does not make, so drop the promise and point at the payload doc's "not a loss report" note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
…text `prekey_structure_to_record` and its signed sibling report `InvalidProtobufEncoding` when a row we wrote no longer converts back into a record — the same variant a peer's malformed envelope produces. By the time it reaches the receive path nothing can tell the two apart, so a corrupt local row was reported as `MalformedCiphertext` and counted against the peer. Only the store boundary still knows the bytes were ours, so it rebrands them as a backend error there. The receiving arm is unchanged: both variants already landed in its catch-all, so only the reported cause moves, from `MalformedCiphertext` to `StorageFailure`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (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: 0adc4579fc
ℹ️ 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".
Three branches still named a cause that says nothing about the sender, or named none at all. `InvalidSenderKeySession` is only raised while reading our stored sender-key record — no chain key, a signing key that will not parse, a chain whose derived key/IV the cipher rejects — so it joins the corrupt prekey and identity rows as `StorageFailure`. The peer's copy is judged by `SignatureValidationFailed` and `InvalidMessage` instead. The alternate-JID lookup that feeds the application resolver discarded its error with `.ok()`. When it fails and the resolver comes back empty, the miss was ours, not the companion's, so it now sets `lookup_failed` like the two lookups beside it. The skmsg skip reported nothing when the batch had a duplicate. That guard had no honest case: `should_process_skmsg_after_session` already admits a batch whose session `<enc>` were duplicates and nothing else, so a duplicate only reaches the skip branch beside a genuine failure -- and that failure skipped the skmsg on the first delivery too. Suppressing it there meant the index produced no plaintext and no report on any delivery. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b179607aad
ℹ️ 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.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
`InvalidSessionStructure` was falling to the `SignalError` catch-all, so a stored `SessionRecord` that decoded and then would not yield usable state was reported as an unclassified cryptographic failure — the same misattribution already fixed for prekey rows, identity rows and sender-key records. Almost every producer of that variant is reading persisted state: the `InvalidSessionError` family exists, in its own words, "to keep from accidentally propagating deserialization errors", and `session_cipher` raises it directly for a missing remote identity key and for message keys the cipher rejects, where it logs the state as corrupt. The exception is a receiver chain we closed, which the peer then sent on — a fact about the message. That is drawn on the variant's `&'static str` payload, so the predicate lives in libsignal as `is_stored_session_corruption` and both raise sites now share one constant with it. The client asks the question; the crate that writes the strings answers it, and they cannot drift apart across a crate boundary. Also updates the duplicate-path comment the previous commit invalidated: the log and the UndecryptableMessage are still suppressed there, but the per-`<enc>` reports are not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Adds a new public Event variant/EventKind and lease-based forwarding API, and changes the public error type of decrypt_bot_message; the new API surface and failure-taxonomy semantics are contract/design decisions a human should own.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e6bf2b00c
ℹ️ 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".
`InvalidMessage` still listed "an invalid sender-key session" among the things it covers. That stopped being true when `InvalidSenderKeySession` moved to `StorageFailure`: a record that will not yield usable keys was never a verdict on the message. It now names what can actually reach it — a version that does not match the state, a signature that did not verify, a body the cipher rejected under sound keys — and points at the variant that took the other case. `StorageFailure` described itself as Signal state only, and it has not been that since the bot path started using it. It now covers local state generally, with the Signal records as the common case and the two `msmsg` routes named: a message-secret lookup that errored rather than came back empty, and a stored secret that will not derive. The line against `NoMessageSecret` is stated, since that is the pair a consumer is most likely to confuse: theirs versus ours. Both are documentation of a frozen API, so being approximately right is not good enough. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: This adds a new public Event variant/EventKind and a forwarding lease, changing the crate's public API, and reclassifies decryption/storage failure causes across the receive path. The API and failure-reporting semantics need human sign-off.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66d6296546
ℹ️ 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 payload doc still claimed a duplicate's silence extends to the skmsg it suppresses. That was the behaviour two commits ago and it was wrong there for the same reason it is wrong here: a batch of duplicates and nothing else never skips its skmsg, so the only skip a duplicate is present for is one caused by a failing sibling — which skipped it on the first delivery too. The doc now says what the code does: the silence covers the duplicate `<enc>` and nothing more, and the skipped skmsg is reported as `NotAttempted` because no delivery produced its plaintext. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Found while re-reading the reason docs against their branches rather than waiting for the next review round to find it. The doc described only the clear-and-retry path. `session_error_reason` also names it wherever libsignal raises an untrusted identity directly with no retry in between — the decrypt after a PN→LID session migration being the live case. A consumer reading the old text would have taken every occurrence as proof a retry had already been attempted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Adds a public Event variant and changes decrypt_bot_message's public error type from anyhow to the new BotMessageError, while reclassifying stored-record errors; these public-contract and data-handling changes need human sign-off.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91417db7d0
ℹ️ 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".
A connection torn down while a classified stanza waits for the global processing permit bails before any payload is tried. Classification had already reported whatever it set aside, so a stanza with one malformed `<enc>` and decryptable siblings reported the malformed index and nothing else -- the exact shape the event promises not to produce. The queued payloads are now reported `NotAttempted` before the bail. No control flow moves: the stanza is still left unacked for redelivery, and the event repeats when it comes back, as it does for every other repeat. `NotAttempted`'s doc gains this third case and says what it does not say: the ciphertext was never read, and this one is not about the stanza at all -- it is about when it arrived. Test: `a_stanza_abandoned_by_a_teardown_reports_what_it_never_tried`, which fails with an empty report if the loop is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VD2isgwsngNMbydRXaaf5
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Summary
Event::DecryptedPayloadalready reports the half of each<enc>that worked —info,enc_index,enc_type, plaintext. The half that failed was reported only per message, once, and without a cause. A consumer following decryption could say "this<enc>produced these bytes" and could not say "this<enc>failed for this reason"; on a fan-out stanza, where one<message>carries a copy per device, it could not even say which one failed.New variant, appended last:
EncDecryptFailureReasonis#[non_exhaustive]with one variant per branch that actually exists — no catch-all "other";SignalErroris the unclassified bucket and says so. It carriesdecryption_was_attempted(), the line between tried and failed and recognized and not handled:falseforMalformedNode,UnsupportedEncTypeandNotAttempted,truefor everything else.On the "frozen API" question: the append-only rule and its rationale are pre-existing, not introduced here — at the PR base (
5e084b9) they sit on theAppStateSyncFailedvariant doc (line 985), theDecryptedPayloadvariant doc (line 997), and theEventKindtype doc (line 209). What theEventtype doc's# Stabilitysection covers is the shape of payloads. Nothing existing changed shape.Failure branches
Every branch of the receive path that abandons an
<enc>. Reported inline from the receive task in all cases — never from a spawned one.classify_incoming_message<enc>with notypeattributeMalformedNode(enc_type: None)EncType::from_wirereturnsNoneUnsupportedEncTypeEncPayload::from_owned_nodereturnsNone(no content)MalformedNodeThe reason enum is
Copywith unit variants — the wiretypeas seen travels in the payload'senc_type: Option<Cow<'static, str>>, borrowed for a type this build knows and owned for one it does not.process_classified_messageNotAttempted(every queued payload)<enc>on a stanza whose sender is a group/broadcastNotAttemptedskmsgskipped because the session batch failedNotAttemptedprocess_session_enc_batchPreKeySignalMessage/SignalMessage::try_fromfailsMalformedCiphertextUntrustedIdentity→ drain-batch commit failedStorageFailureUntrustedIdentity→flush_signal_cachefailedStorageFailureUntrustedIdentity→ retry failed →session_error_reason(retry_err)UntrustedIdentityfor what it leaves unclassifiedSessionNotFound→ migrationNotDecryptedNoSessionBadMac/InvalidMessage→ migrationNotDecryptedBadMac/InvalidMessageInvalidPreKeyId/InvalidSignedPreKeyIdUnknownPreKeysignal_error_reasonMalformedCiphertext/StorageFailure/LocalCryptoFailure/InvalidMessage/SignalErrorhandle_decrypted_plaintexterroredPlaintextUnusableprocess_group_enc_batchNoSenderKeyState(including the expired-status early return)NoSenderKeysignal_error_reasonnames itMalformedCiphertext/StorageFailure/LocalCryptoFailure/InvalidMessagegroup_decrypt_retry_reason(&e).is_some()InvalidMessageSignalErrorhandle_decrypted_plaintexterroredPlaintextUnusablehandle_msmsg_payloadMessageSecretMessagedecode failed, or missingenc_iv/enc_payloadMalformedCiphertexttarget_sender, missingtarget_id, or no stored secretNoMessageSecretStorageFailuredecrypt_bot_message→BotMessageFailure::EnvelopeMalformedCiphertextdecrypt_bot_message→BotMessageFailure::SecretStorageFailuredecrypt_bot_message→BotMessageFailure::AuthenticationBadMacMessageprotoPlaintextUnusableOne rule ran through the review rounds
Almost every finding was the same shape: an error that says nothing about the sender was being reported against them. The taxonomy now routes each through one place:
signal_error_reason(src/message.rs) is the single classifier both the session catch-all and the group arm read, so one libsignal error cannot be named two ways. Envelope-parse rejections →MalformedCiphertext;BackendError→StorageFailure;KeyAgreementFailed→LocalCryptoFailure;InvalidSenderKeySession→StorageFailure(libsignal only raises it while reading our stored sender-key record — no chain key, an unparseable signing key, a chain whose derived key/IV the cipher rejects); a storedSessionRecordthat decoded without usable state →StorageFailure;UnrecognizedMessageVersion→InvalidMessage(which is what the group arm already called it).session_error_reasonextends it with the session arms' own mapping, and is what the identity retry and the PN→LID migration's terminal error read, so a retry that fails a MAC is not filed under the error that opened it.prekey_structure_to_record, its signed sibling, andparse_cached_identityall raiseInvalidProtobufEncoding/BadKeyLength— the same variants a peer's malformed bytes raise, indistinguishable downstream.record_read_errrebrands them at the only place that still knows the bytes were ours.SignalProtocolError::is_stored_session_corruption()lives in libsignal, not here. The line between a corrupt stored session and a receiver chain we closed is drawn onInvalidSessionStructure's&'static str, and matching that from the client would let an upstream rename silently flip the classification across a crate boundary. The predicate and both raise sites now share oneCLOSED_RECEIVER_CHAINconstant;signal_error_reasononly asks.BotMessageError::stage()does the same for bot payloads: envelope shape, unusable stored secret, and a real tag failure are three different events.Option.StorageFailureandLocalCryptoFailureboth document that a per-peer health signal should exclude them.Branches deliberately left without an event
Duplicates. An
<enc>refused because it was already processed did not fail decryption — its plaintext was reported the first time round.This does not extend to a
skmsgskipped by a batch that contained a duplicate, which an earlier revision of this PR got wrong.should_process_skmsg_after_sessionisempty || (!had_failure && (decrypted || duplicate)), so a batch whose session<enc>were duplicates and nothing else is processed normally and never reaches the skip. The only way a duplicate reaches it is beside an<enc>that genuinely failed — and that failure skipped the sameskmsgon the first delivery too, so it produced no plaintext on any delivery. It is reported. Pinned bya_skmsg_skipped_beside_a_duplicate_is_still_reported, which replays a realDuplicatedMessagenext to an<enc>that fails and asserts the duplicate stays silent while both the failure and the skippedskmsgare reported.Custom enc handlers. A registered
EncHandlerowns its node; the client never attempts it, the consumer that registered it already has theErr, and reporting from the handler's detached task would break the ordering this event promises.Stanzas with no
MessageInfo—parse_message_infofailing, or a newsletter. Noinfoto attach and no<enc>to index.One
<enc>, one eventEach branch dispatches once then
continues, and the buckets are disjoint. The single documented exception isPlaintextUnusableafter aDecryptedPayloadfor the sameenc_index— the bytes existed and could not be made into a message. Unpadding fails ahead ofDecryptedPayload, so there this is the only signal.Why not UndecryptableMessage
Untouched — its dedup, log level and semantics. Three properties rule it out:
<enc>. On a fan-out it cannot attribute the failure.(chat, id)viaundecryptable_dispatched.get_with, so the second arrival of a stanza that keeps failing produces no event. Correct for it — a UI must not show two placeholders for one message — and exactly the case a diagnostic needs to see.decrypt_fail_modeis the server'sshow/hide, a display hint, not a cause.Second opinion. whatsmeow reports per message:
decryptMessagesloops theencchildren and, on the first failure, dispatches oneevents.UndecryptableMessage{Info, IsUnavailable, DecryptFailMode}and returns — no enc index, no enc type, and later<enc>of the same stanza are not even reached. That does not invalidate this; it says nobody has needed the finer grain there yet.enc_index
Assigned once, in
classify_incoming_message, byfor (enc_index, enc_node) in all_enc_nodes.iter().enumerate()overmessage_enc_nodes_for_device(...)— direct<enc>children first, then this device's under<participants>. It rides inEncPayloadthrough the buckets, andDecryptedPayloadreads exactly that field. Every dispatch reads the same field from the same struct — there is no second enumeration in the diff.Pinned by
fan_out_encs_are_numbered_after_the_direct_onesandenc_index_is_the_position_in_the_stanza_not_in_its_bucket(pre-existing), plusa_mixed_stanza_numbers_successes_and_failures_the_same_way, which feeds three<enc>at 0/1/2 where 1 decrypts against a real Signal session and asserts the failures land on 0 and 2 while theDecryptedPayloadlands on 1.Ordering. The client decrypts in per-kind passes (session, group, bot), and session successes drain after the decrypt loop while session failures are reported inside it. Neither event kind arrives in stanza order, and a failure for a later
<enc>can precede a success for an earlier one. Both come from the same receive task, so they are totally ordered relative to each other — just not by position.enc_indexis the only ordering to rely on, and the payload doc says so.Cost when nobody listens
Its own lease and its own counter —
Client::acquire_enc_decrypt_failed_forwarding()→EncDecryptFailedLease, backed byenc_decrypt_failed_forwarding: AtomicUsize, the mechanismDecryptedPayloaduses. Counted separately so a consumer watching only failures does not turn on plaintext cloning, and one watching only successes pays nothing on the failure paths.While no lease is held each branch costs one relaxed atomic load:
report_enc_decrypt_failurereturns before building anything, and the classification variant makes itsStringcopy past the gate. Registered inGatedForwarding(src/plugins/mod.rs), so a plugin subscribing toEventKind::EncDecryptFailedacquires and retires the lease with its interest.Measured by
enc_failures_are_not_reported_without_a_lease,the_two_forwarding_gates_are_independent, andeach_gated_kind_holds_its_own_lease.Binary size: +6.3 KiB stripped (+0.06%), no new dependencies.
Compatibility
EventandEventKindgain an appended variant. Both are#[non_exhaustive], so no consumer match breaks.wacore-libsignalgains one public method,SignalProtocolError::is_stored_session_corruption(). Additive, so nothing breaks; noted because it is public surface, and because that crate is not in theSemver Checkspackage list (-p wacore -p wacore-binary -p waproto) and so would not have been reported either way.wacore::bot_message::decrypt_bot_messagechanges its error type fromanyhow::Errorto a typedBotMessageError. That is a breaking change towacore's public API;cargo-semver-checkshas no lint for return-type changes, so it does not appear in that job's output — calling it out here rather than letting it pass silently.Tests
One per failure branch reachable from a unit entry point, asserting the exact
enc_index,enc_typeand reason; the mixed-stanza numbering test;a_redelivered_stanza_reports_its_failure_again(twoEncDecryptFailedagainst exactly oneUndecryptableMessage);a_skmsg_skipped_beside_a_duplicate_is_still_reported;a_stanza_abandoned_by_a_teardown_reports_what_it_never_tried; the lease tests; and direct tests of the shared classifiers —a_backend_error_is_storage_not_a_signal_failure(which also pinsInvalidSenderKeySessionand a corrupt stored session as storage, a closed receiver chain as not storage, andSignatureValidationFailedas the catch-all),a_closed_receiver_chain_is_not_stored_corruptioninwacore-libsignal,a_version_mismatch_is_an_invalid_message_on_both_paths,a_local_key_agreement_failure_is_not_the_peers,a_migrated_session_reports_the_retry_failure_not_the_one_that_opened_it,a_corrupt_stored_prekey_is_not_blamed_on_the_peer,a_corrupt_stored_identity_is_not_blamed_on_the_peer,a_stored_bot_secret_that_will_not_derive_is_storage_not_a_missing_secret. All fixtures use fictitious JIDs.Two branches have no test of their own, both stated rather than left implicit:
InvalidSignedPreKeyIdreached through the untrusted-identity retry needs an identity change and a rotated-out signed pre-key in the same stanza. It reports the sameUnknownPreKeyas the direct arm, whicha_rotated_out_signed_prekey_reports_unknown_prekeycovers.alternate_msg_secret_jidcall failing where the first succeeded — a backend that breaks between two adjacent queries. The suite has noBackendmock, and standing up a stateful one for that window would be the only mock of its kind in the file.Not changed: retry/rerequest policy,
UndecryptableMessage, and the loop's control flow — no condition deciding what gets decrypted was touched.Validation
CI is green on the current head: Build & Test, Build & Lint (all features), Feature Matrix, E2E Tests, Test Stable (no-simd), Clippy, Format Check, Rustdoc, Cargo Deny, wasm32 release, all four Miri jobs, Binary Size and all four CodSpeed jobs.
Semver Checks (informational)is red on six pre-existing breaks — five in the regeneratedwacore::iq::mex_operations, oneBinaryError::UnexpectedFormatBytefrom #1259 — with no entry from this diff. Confirmed by reading the job log rather than inferring it: the failing items are all inmex_operations.rsandwacore/binary/src/error.rs, and the commits that touch only doc comments cannot move the API surface at all.