Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/bot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,7 @@ pub struct BotBuilder<
// Optional fields
event_handlers: Vec<RegisteredHandler>,
raw_handlers: Vec<Arc<dyn EventHandler>>,
pre_ack_message_hook: Option<crate::message::ParsedMessagePreAckHook>,
custom_enc_handlers: HashMap<String, Arc<dyn EncHandler>>,
override_version: Option<(u32, u32, u32)>,
device_props_override: Option<DevicePropsOverride>,
Expand All @@ -508,6 +509,7 @@ impl BotBuilder<MissingBackend, DefaultTransportState, DefaultHttpState, Default
runtime: default_runtime(),
event_handlers: Vec::new(),
raw_handlers: Vec::new(),
pre_ack_message_hook: None,
custom_enc_handlers: HashMap::new(),
override_version: None,
device_props_override: None,
Expand All @@ -533,6 +535,7 @@ impl<B, T, H, R> BotBuilder<B, T, H, R> {
runtime: self.runtime,
event_handlers: self.event_handlers,
raw_handlers: self.raw_handlers,
pre_ack_message_hook: self.pre_ack_message_hook,
custom_enc_handlers: self.custom_enc_handlers,
override_version: self.override_version,
device_props_override: self.device_props_override,
Expand Down Expand Up @@ -639,6 +642,27 @@ impl<B, T, H, R> BotBuilder<B, T, H, R> {
})
}

/// Run `hook` inline before acknowledging each parsed inbound message.
///
/// This is the durability hook for consumers that need strict
/// "ACK-after-commit" semantics. The hook receives the same normalized
/// parsed message and [`MessageInfo`] that will be emitted as
/// `Event::Message` after ACK. Return `Ok(())` only after your durable write
/// has committed. Returning `Err` suppresses both the ACK/receipt and the
/// normal message event for this delivery attempt so WhatsApp can retry.
///
/// Keep this hook narrow and fast: it runs on the receive lane before ACK.
pub fn on_pre_ack_message<F, Fut>(mut self, hook: F) -> Self
where
F: Fn(crate::message::ParsedMessagePreAckContext) -> Fut + Send + Sync + 'static,
Fut: Future<Output = anyhow::Result<()>> + Send + 'static,
{
self.pre_ack_message_hook = Some(Arc::new(
move |ctx| -> crate::message::ParsedMessagePreAckHookFuture { Box::pin(hook(ctx)) },
));
self
}

/// Run `handler` with the QR payload (and validity window) each time a
/// pairing QR code is issued. Render `code` as a QR image for scanning.
pub fn on_qr_code<F, Fut>(self, handler: F) -> Self
Expand Down Expand Up @@ -969,6 +993,10 @@ impl BotBuilder<Provided, Provided, Provided, Provided> {
// map once; the receive hot path then reads it lock-free.
let _ = client.custom_enc_handlers.set(self.custom_enc_handlers);

if let Some(hook) = self.pre_ack_message_hook {
let _ = client.set_parsed_message_pre_ack_hook_arc(hook);
}

if self.skip_history_sync {
client.set_skip_history_sync(true);
}
Expand Down
15 changes: 15 additions & 0 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,16 @@ pub struct Client {
/// in `WAWebMessageProcessPlaceholder`.
pub(crate) undecryptable_dispatched: Cache<ChatMessageId, ()>,

/// Parsed messages whose pre-ACK hook failed after Signal state advanced.
/// A same-process server redelivery may then arrive as a Signal duplicate;
/// this cache lets that duplicate retry the durable hook with the original
/// normalized message instead of ACKing an uncommitted delivery.
pub(crate) pending_parsed_message_pre_ack:
Cache<ChatMessageId, crate::message::PendingParsedMessagePreAck>,
/// Reliable barrier for Signal flush deferral. Kept separate from the cache
/// because `PortableCache::entry_count()` is intentionally best-effort.
pub(crate) pending_parsed_message_pre_ack_count: AtomicUsize,

pub enable_auto_reconnect: Arc<AtomicBool>,
pub auto_reconnect_errors: Arc<AtomicU32>,

Expand Down Expand Up @@ -563,6 +573,11 @@ pub struct Client {
/// `OnceLock::get` (no lock) and no per-node guard acquisition.
pub custom_enc_handlers: std::sync::OnceLock<HashMap<String, Arc<dyn EncHandler>>>,

/// Optional awaited hook for consumers that need to durably commit a parsed
/// inbound message before the SDK sends the delivery receipt / transport ACK.
pub(crate) parsed_message_pre_ack_hook:
std::sync::OnceLock<crate::message::ParsedMessagePreAckHook>,

/// Chat state (typing indicator) handlers registered by external consumers.
/// Each handler receives a `ChatStateEvent` describing the chat, optional participant and state.
pub(crate) chatstate_handlers: Arc<RwLock<Vec<ChatStateHandler>>>,
Expand Down
44 changes: 44 additions & 0 deletions src/client/accessors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,50 @@ impl Client {
self.core.event_bus.add_handler(handler);
}

/// Register an awaited hook that runs after an inbound message has been
/// parsed and normalized, but before the SDK sends its delivery receipt or
/// transport ACK.
///
/// The hook is intended for durability-sensitive consumers: persist the
/// message, commit the transaction, then return `Ok(())`. If the hook
/// returns an error, `whatsapp-rust` suppresses the ACK and the normal
/// `Event::Message` dispatch for this attempt, leaving the server free to
/// redeliver/retry instead of marking the message delivered.
///
/// Only one hook can be registered for a `Client`. Applications using
/// [`BotBuilder`](crate::bot::BotBuilder) can prefer
/// [`BotBuilder::on_pre_ack_message`](crate::bot::BotBuilder::on_pre_ack_message)
/// to wire the hook during construction.
///
/// Registering this hook also keeps inbound message processing serial. That
/// preserves the durability contract when a hook fails after Signal decrypt:
/// the SDK can defer Signal cache flushes while the uncommitted parsed
/// message is pending, without allowing another inbound message to race the
/// pending retry path.
pub fn set_parsed_message_pre_ack_hook<F, Fut>(
&self,
hook: F,
) -> Result<(), crate::message::ParsedMessagePreAckHookAlreadySet>
where
F: Fn(crate::message::ParsedMessagePreAckContext) -> Fut + Send + Sync + 'static,
Fut: std::future::Future<Output = anyhow::Result<()>> + Send + 'static,
{
self.set_parsed_message_pre_ack_hook_arc(Arc::new(
move |ctx| -> crate::message::ParsedMessagePreAckHookFuture { Box::pin(hook(ctx)) },
))
}

pub(crate) fn set_parsed_message_pre_ack_hook_arc(
&self,
hook: crate::message::ParsedMessagePreAckHook,
) -> Result<(), crate::message::ParsedMessagePreAckHookAlreadySet> {
self.parsed_message_pre_ack_hook
.set(hook)
.map_err(|_| crate::message::ParsedMessagePreAckHookAlreadySet)?;
self.swap_message_semaphore(1);
Ok(())
}

/// Enable or disable raw node forwarding.
/// When enabled, `Event::RawNode` is emitted for every decoded stanza before
/// the stanza router dispatches it. Only enable when external consumers need
Expand Down
11 changes: 11 additions & 0 deletions src/client/adapters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@ impl Client {
/// Flush the in-memory signal cache to the database backend.
/// Called after each message is decrypted or after encryption operations.
pub(crate) async fn flush_signal_cache(&self) -> Result<(), anyhow::Error> {
if self.parsed_message_pre_ack_hook.get().is_some()
&& self
.pending_parsed_message_pre_ack_count
.load(Ordering::Acquire)
> 0
{
return Err(anyhow::anyhow!(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: This returns an Err for what is expected, routine backpressure (a deferred flush while a pre-ACK commit is pending). The flush_signal_cache_logged caller will log it at error! level on every deferred call, creating noisy false-alarm entries during normal operation. Either return a distinguishable "deferred" variant that the logged wrapper can downgrade to debug!/trace!, or use a dedicated error type that callers can match on to adjust log severity.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/adapters.rs, line 68:

<comment>This returns an `Err` for what is expected, routine backpressure (a deferred flush while a pre-ACK commit is pending). The `flush_signal_cache_logged` caller will log it at `error!` level on every deferred call, creating noisy false-alarm entries during normal operation. Either return a distinguishable "deferred" variant that the logged wrapper can downgrade to `debug!`/`trace!`, or use a dedicated error type that callers can match on to adjust log severity.</comment>

<file context>
@@ -59,6 +59,17 @@ impl Client {
+                .load(Ordering::Acquire)
+                > 0
+        {
+            return Err(anyhow::anyhow!(
+                "Signal cache flush deferred while parsed-message pre-ACK commit is pending"
+            ));
</file context>

"Signal cache flush deferred while parsed-message pre-ACK commit is pending"
));
Comment on lines +68 to +70

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Don't scream error! for what is normal, expected backpressure.

flush_signal_cache_logged logs this deferral at error!. But a deferred flush while a pre-ACK commit is pending is expected, routine operation — it'll fire on every deferred call and drown the logs with errors that aren't errors. Either return a distinguishable "deferred" outcome that the _logged wrapper downgrades to debug!/trace!, or have callers special-case it. As-is, operators will think something's broken when it's working as designed.

🤖 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/adapters.rs` around lines 68 - 70, The deferred flush path in
flush_signal_cache_logged is treating expected backpressure as an error, which
causes noisy error-level logging. Update the signal-cache flush flow so the
pending pre-ACK commit case is represented as a distinct deferred outcome from
the core flush_signal_cache logic, then have the _logged wrapper downgrade that
case to debug or trace instead of error, or make callers special-case it using
the existing anyhow::anyhow return point as the location to distinguish the
status.

}
Comment on lines +62 to +71

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

This is the part that has to work right: one stuck commit can wedge Signal persistence for the entire client, forever.

The gate is global, not per-message: as long as pending_parsed_message_pre_ack_count > 0, every flush_signal_cache call returns Err — including the one in cleanup_connection_state, which then keeps (never clears) the signal cache. So if a consumer's durable commit fails permanently for a single message (disk full, DB down, a bug in their hook), the result is:

  • Signal cache flushes are blocked for all chats, not just the stuck one.
  • The in-memory signal cache grows unbounded and its advanced session/sender-key state is never persisted, surviving reconnects only in memory.
  • Combined with the serialized receive lane, the whole pipeline degrades behind one poisoned entry.

There's no bound, timeout, or escape hatch here. Please add one — e.g. a max pending age/count after which the entry is abandoned (and the message left for redelivery) so a single bad commit can't brick Signal persistence indefinitely.

🤖 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/adapters.rs` around lines 62 - 71, The global pending-commit gate
in flush_signal_cache is too strict because a single stuck parsed-message
pre-ACK commit can block all Signal persistence forever, including
cleanup_connection_state. Update the logic around
pending_parsed_message_pre_ack_count and parsed_message_pre_ack_hook so there is
an escape hatch such as a max pending age or retry limit; once exceeded, abandon
the pending entry, allow the cache flush to proceed, and leave the message for
redelivery. Keep the fix localized to the flush_signal_cache path and any helper
state that tracks pending pre-ACK commits.


// Hold no device guard across the flush: this per-message batched SQLite
// write would otherwise block every concurrent Device write for its duration.
let backend = self
Expand Down
8 changes: 8 additions & 0 deletions src/client/lifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,13 @@ impl Client {
),

undecryptable_dispatched: cache_config.undecryptable_dispatched.build_with_ttl(),
// Intentionally unbounded/no-TTL: an entry means Signal decrypt may
// have advanced volatile state for a message the application has
// not durably committed. Expiring it would allow a later Signal
// flush to make that advance durable and turn server redelivery
// into an ACKed duplicate without rerunning the hook.
pending_parsed_message_pre_ack: Cache::builder().build(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: This cache is intentionally unbounded with no TTL, and it is never cleared in cleanup_connection_state (unlike chat_lanes, signal_cache, etc.). If a hook permanently fails for messages that never get successfully redelivered, entries and the associated pending_parsed_message_pre_ack_count accumulate indefinitely across reconnects. Consider adding at minimum a max_capacity or draining stale entries during connection cleanup to prevent unbounded growth.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/lifecycle.rs, line 200:

<comment>This cache is intentionally unbounded with no TTL, and it is never cleared in `cleanup_connection_state` (unlike `chat_lanes`, `signal_cache`, etc.). If a hook permanently fails for messages that never get successfully redelivered, entries and the associated `pending_parsed_message_pre_ack_count` accumulate indefinitely across reconnects. Consider adding at minimum a `max_capacity` or draining stale entries during connection cleanup to prevent unbounded growth.</comment>

<file context>
@@ -192,6 +192,13 @@ impl Client {
+            // not durably committed. Expiring it would allow a later Signal
+            // flush to make that advance durable and turn server redelivery
+            // into an ACKed duplicate without rerunning the hook.
+            pending_parsed_message_pre_ack: Cache::builder().build(),
+            pending_parsed_message_pre_ack_count: AtomicUsize::new(0),
 
</file context>

pending_parsed_message_pre_ack_count: AtomicUsize::new(0),
Comment on lines +195 to +201

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

The unbounded, never-cleared pending cache is the thing I'm worried about — let's make sure it can't grow forever.

Look, I get the design: an entry means Signal state advanced for an uncommitted message, so expiring it would be wrong. But this cache has no capacity bound and is never cleared in cleanup_connection_state (unlike chat_lanes, signal_cache, etc.). If a consumer's pre-ACK hook keeps failing for a message that never gets a successful redelivery, that entry — and the pending_parsed_message_pre_ack_count it bumps — stays around indefinitely. Across reconnects it just accumulates. The real-world fallout shows up in flush_signal_cache; I've left the detailed concern there. Please confirm there's a path that bounds this or drains stuck entries.

🤖 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/lifecycle.rs` around lines 195 - 201, The
pending_parsed_message_pre_ack cache in lifecycle state is unbounded and never
drained, so add a lifecycle path that removes stuck entries or caps growth.
Review pending_parsed_message_pre_ack, pending_parsed_message_pre_ack_count,
cleanup_connection_state, and flush_signal_cache to ensure failed pre-ACK items
are eventually evicted or accounted for without relying on TTL, and make sure
reconnect/cleanup logic clears or bounds this cache like the other
per-connection caches.


offline_sync_metrics: Arc::new(OfflineSyncMetrics {
active: AtomicBool::new(false),
Expand Down Expand Up @@ -226,6 +233,7 @@ impl Client {
pairing_cancellation_tx: Arc::new(Mutex::new(None)),
pair_code_state: Arc::new(Mutex::new(wacore::pair_code::PairCodeState::default())),
custom_enc_handlers: std::sync::OnceLock::new(),
parsed_message_pre_ack_hook: std::sync::OnceLock::new(),
chatstate_handlers: Arc::new(RwLock::new(Vec::new())),
pdo_pending_requests: cache_config.pdo_pending_requests.build_with_ttl(),
pdo_requested: cache_config.pdo_requested.build_with_ttl(),
Expand Down
19 changes: 16 additions & 3 deletions src/client/node_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ impl Client {
/// a consistent (generation, Arc) pair. Must be called from a non-async
/// context or inside a scoped block (MutexGuard is !Send).
pub(crate) fn swap_message_semaphore(&self, permits: usize) {
let permits = if self.parsed_message_pre_ack_hook.get().is_some() {
// A pre-ACK hook can intentionally fail after Signal decrypt has
// advanced volatile state. Keep message processing serial while the
// hook is installed so pending retry/flush deferral has one inbound
// message to reason about at a time.
1
} else {
permits
};
Comment on lines +24 to +32

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

This pins the whole receive pipeline to single-threaded for the client's entire lifetime — make sure consumers understand the cost.

Forcing permits = 1 whenever the hook is installed means every inbound message is processed strictly serially forever, not just during the pending-retry window. The reasoning is sound (you need one message to reason about at a time for the flush-deferral contract), but a slow durable-commit hook now serializes and back-pressures the entire receive lane. That's a real throughput ceiling. Worth calling out loudly in the public set_parsed_message_pre_ack_hook docs so nobody ships a heavy hook and wonders why their message throughput tanked.

🤖 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/node_io.rs` around lines 24 - 32, The receive path in NodeIO is
forced to a single in-flight message whenever a parsed-message pre-ACK hook is
installed, which serializes the whole pipeline for the client’s lifetime. Update
the public documentation for set_parsed_message_pre_ack_hook to clearly call out
this throughput cost and that a heavy hook will back-pressure all inbound
messages. Mention the behavior change explicitly near the NodeIo /
parsed_message_pre_ack_hook API so consumers understand the tradeoff before
enabling it.

let mut guard = match self.message_processing_semaphore.lock() {
Ok(g) => g,
Err(poisoned) => poisoned.into_inner(),
Expand Down Expand Up @@ -415,9 +424,13 @@ impl Client {
};
match tag {
"receipt" | "notification" | "call" => true,
"message" => from
.to_jid()
.is_some_and(|j| j.is_newsletter() || j.is_status_broadcast()),
"message" => {
if self.parsed_message_pre_ack_hook.get().is_some() {
return false;
}
from.to_jid()
.is_some_and(|j| j.is_newsletter() || j.is_status_broadcast())
}
_ => false,
}
}
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ pub mod version;
pub mod prelude {
pub use crate::bot::{Bot, BotBuilder, BotHandle, MessageContext};
pub use crate::client::{Client, ClientError};
pub use crate::message::{ParsedMessagePreAckContext, ParsedMessagePreAckHookAlreadySet};
pub use crate::request::IqError;
#[cfg(feature = "tokio-runtime")]
pub use crate::runtime_impl::TokioRuntime;
Expand Down
65 changes: 65 additions & 0 deletions src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use crate::types::message::MessageInfo;
use log::{debug, warn};
use prost::Message as ProtoMessage;

use std::future::Future;
use std::pin::Pin;
use std::sync::Arc;
use wacore::libsignal::crypto::DecryptionError;
use wacore::libsignal::protocol::SenderKeyDistributionMessage;
Expand All @@ -28,6 +30,61 @@ use waproto::whatsapp::{self as wa};
/// After this many retries, we stop sending retry receipts and rely solely on PDO.
const MAX_DECRYPT_RETRIES: u8 = 5;

/// Future returned by a parsed-message pre-ACK hook.
///
/// The hook is awaited inline on the receive path after the inbound payload has
/// been parsed and normalized, but before the SDK sends the delivery receipt or
/// transport ACK. Returning an error intentionally suppresses the ACK so the
/// server can redeliver the message later. This is useful for consumers that
/// must durably commit an inbound message before WhatsApp considers it
/// delivered.
pub(crate) type ParsedMessagePreAckHookFuture =
Pin<Box<dyn Future<Output = anyhow::Result<()>> + Send + 'static>>;

/// Callback registered with [`Client::set_parsed_message_pre_ack_hook`].
///
/// [`Client::set_parsed_message_pre_ack_hook`]: crate::Client::set_parsed_message_pre_ack_hook
pub(crate) type ParsedMessagePreAckHook =
Arc<dyn Fn(ParsedMessagePreAckContext) -> ParsedMessagePreAckHookFuture + Send + Sync>;

/// Message context passed to the awaited pre-ACK hook.
///
/// Both `message` and `info` are `Arc`-wrapped so the hook can cheaply share the
/// same normalized values that the event bus will receive after ACK. The
/// `client` handle is included for applications that need account or storage
/// context while performing a durable commit.
#[derive(Clone)]
pub struct ParsedMessagePreAckContext {
pub message: Arc<wa::Message>,
pub info: Arc<MessageInfo>,
pub client: Arc<Client>,
}

impl ParsedMessagePreAckContext {
pub(crate) fn new(
message: Arc<wa::Message>,
info: Arc<MessageInfo>,
client: Arc<Client>,
) -> Self {
Self {
message,
info,
client,
}
}
}

/// Returned when a parsed-message pre-ACK hook has already been registered.
#[derive(Clone, Copy, Debug, Eq, PartialEq, thiserror::Error)]
#[error("parsed message pre-ACK hook is already registered")]
pub struct ParsedMessagePreAckHookAlreadySet;

#[derive(Clone)]
pub(crate) struct PendingParsedMessagePreAck {
pub message: Arc<wa::Message>,
pub info: Arc<MessageInfo>,
}

/// Pre-extracted enc node payload. Holds owned copies of the fields needed for
/// decryption so the async decrypt phase doesn't borrow the original NodeRef tree.
pub(crate) struct EncPayload {
Expand Down Expand Up @@ -77,23 +134,31 @@ pub(crate) struct SessionBatchOutcome {
duplicate: bool,
undecryptable: bool,
dispatched: bool,
pre_ack_hook_failed: bool,
skdm_only: bool,
plaintext_failed: bool,
had_failure: bool,
}

#[derive(Clone, Copy, Debug, Default)]
struct GroupBatchOutcome {
pre_ack_hook_failed: bool,
}

#[derive(Clone, Copy, Debug, Default)]
struct MigrationDecryptOutcome {
decrypted: bool,
duplicate: bool,
dispatched: bool,
pre_ack_hook_failed: bool,
skdm_only: bool,
plaintext_failed: bool,
}

#[derive(Clone, Copy, Debug, Default)]
pub(crate) struct PlaintextHandleOutcome {
dispatched: bool,
pre_ack_hook_failed: bool,
skdm_only: bool,
}

Expand Down
Loading
Loading