Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
62ff07f
perf(signal): coalesce hot-path Signal cache flushes
jlucaso1 Jul 11, 2026
f2d0212
test(e2e): settle the signal cache before the legacy-DB surgery
jlucaso1 Jul 11, 2026
430b221
fix(signal): re-arm the coalesced flush on error and fix window termi…
jlucaso1 Jul 11, 2026
1693785
fix(signal): retry the coalesced flush inline instead of recursing
jlucaso1 Jul 11, 2026
a75cfc4
fix(signal): back off the failing-flush retry exponentially
jlucaso1 Jul 11, 2026
4e1a851
test(signal): cover the failing-flush retry path
jlucaso1 Jul 11, 2026
47b33d5
test(e2e): gate connect on the canonical is_ready signal
jlucaso1 Jul 11, 2026
b46e161
style(e2e): rustfmt the connect helper
jlucaso1 Jul 11, 2026
4e3e736
test(e2e): settle the coalesced flush before inspecting durable sessions
jlucaso1 Jul 11, 2026
7be4107
docs(signal): document the settle API's permit and durability precond…
jlucaso1 Jul 13, 2026
4d1b8fa
perf(signal): coalesce only the receive flush; keep sends synchronous
jlucaso1 Jul 13, 2026
6549dcd
docs(signal): fix orphaned rustdoc and document settle preconditions
jlucaso1 Jul 13, 2026
fea31e0
test(e2e): require startup-sync quiescence; prove outbound flush is d…
jlucaso1 Jul 13, 2026
9cf00c5
fix(send): flush the outbound ratchet before the stanza hits the wire
jlucaso1 Jul 13, 2026
10a06db
fix(signal): make the flush scheduler generation-scoped
jlucaso1 Jul 13, 2026
14b9cac
test(e2e),docs: prove the send flush ordering; fix stale coalescing docs
jlucaso1 Jul 13, 2026
8a38d46
fix(signal): skip the stale worker's flush after a generation change
jlucaso1 Jul 13, 2026
4b6522c
fix(signal): reject stale-generation schedule calls (no scheduler reg…
jlucaso1 Jul 13, 2026
935c83c
docs,test(e2e): fix stale per-message-flush rustdoc; settle without r…
jlucaso1 Jul 13, 2026
ef1cf76
test(e2e): prove send aborts before the wire when persistence fails
jlucaso1 Jul 13, 2026
63fe36b
refactor(wacore): feature-gate InMemoryBackend test hooks behind test…
jlucaso1 Jul 13, 2026
1b476b7
test,docs(signal): prove the second flush window and the pre-wire abort
jlucaso1 Jul 13, 2026
43561b8
fix(signal): gate coalesced flush writes against teardown cache settle
jlucaso1 Jul 13, 2026
d6b2997
docs(e2e): correct the sent-node waiter ordering comment
jlucaso1 Jul 13, 2026
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
8 changes: 8 additions & 0 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,14 @@ pub struct Client {
/// Initialized after `Arc::new(this)` in the constructor.
pub(crate) self_weak: std::sync::OnceLock<std::sync::Weak<Client>>,

/// Single-flight state for the coalesced Signal-cache flush worker
/// (RUNNING/DIRTY bits; see `signal_flush.rs`).
pub(crate) signal_flush_state: AtomicU32,
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
/// Injected failures for the coalesced flush (consumed one per attempt),
/// so tests can exercise the retry/backoff path deterministically.
#[cfg(test)]
pub(crate) signal_flush_test_failures: AtomicU32,

/// Holds the background saver's AbortHandle so the task lifetime follows
/// `Arc<Client>` ref count instead of the Bot wrapper's. Set once by
/// `Bot::build`; on Client drop (last Arc), the handle drops and the saver
Expand Down
40 changes: 27 additions & 13 deletions src/client/adapters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,27 @@ impl Client {
.ok_or(ClientError::NotConnected)
}

/// Force any pending write-behind Signal cache state to the backend,
/// returning once the flush completes (or fails).
///
/// Live sends and receives schedule a coalesced flush (see
/// `signal_flush.rs`) instead of writing through, so on success the backend
/// trails the in-memory cache by at most one coalescing window; a backend
/// outage extends that until the scheduler's retry loop succeeds. Use this
/// to settle durability deterministically before reading persisted state or
/// ahead of a non-graceful shutdown — and check the returned `Result`, as a
/// failure leaves state pending.
///
/// Call from a control task, never from inside an event handler or an
/// [`InboundDurabilityHook`]: during an offline-sync drain those run while
/// the processing permit is held, and settling routes through that same
/// permit — re-entering it would deadlock.
///
/// [`InboundDurabilityHook`]: crate::types::durability_hook::InboundDurabilityHook
pub async fn flush_pending_signal_state(&self) -> Result<(), anyhow::Error> {
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
self.flush_signal_cache_batch_safe().await
}

/// 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> {
Expand All @@ -84,21 +105,14 @@ impl Client {
.map_err(|e| anyhow::anyhow!("Failed to flush signal cache: {e}"))
}

/// [`flush_signal_cache`](Self::flush_signal_cache) with error logging instead of propagation.
///
/// Both of these are safe only when the caller holds the message
/// processing permit or the batcher is known inactive: they persist the
/// WHOLE cache, including ratchet advances of drain entries that may not
/// have a durable buffered row yet. Everything else must go through the
/// `_batch_safe` variants below.
pub(crate) async fn flush_signal_cache_logged(&self, context: &str, id: Option<&str>) {
if let Err(e) = self.flush_signal_cache().await {
log_signal_flush_error(context, id, &e);
}
}

/// Signal-cache flush that is safe while the offline drain is active.
///
/// [`flush_signal_cache`](Self::flush_signal_cache) is safe only when the
/// caller holds the message processing permit or the batcher is known
/// inactive: it persists the WHOLE cache, including ratchet advances of
/// drain entries that may not have a durable buffered row yet. Everything
/// else must go through this `_batch_safe` variant.
///
/// During the drain, decrypted messages accumulate in the commit batcher
/// with no durable buffered copy; flushing the cache from an unrelated
/// path (a retry receipt, a send, an identity change) would persist their
Expand Down
8 changes: 8 additions & 0 deletions src/client/lifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ impl Client {
pair_code_state: Arc::new(Mutex::new(wacore::pair_code::PairCodeState::default())),
passkey_state: Arc::new(Mutex::new(crate::passkey::flow::PasskeyFlowState::default())),
passkey_opening: AtomicBool::new(false),
signal_flush_state: AtomicU32::new(0),
#[cfg(test)]
signal_flush_test_failures: AtomicU32::new(0),
custom_enc_handlers: std::sync::OnceLock::new(),
inbound_durability_hook: std::sync::OnceLock::new(),
retry_admission: std::sync::OnceLock::new(),
Expand Down Expand Up @@ -764,6 +767,11 @@ impl Client {
// permit-held cache settle below, so no rowless ratchet advances can
// dirty the cache behind teardown's back.
self.connection_generation.fetch_add(1, Ordering::SeqCst);
// Stand down the coalesced-flush worker: its generation guard exits it
// on the next wake, and clearing the arm lets the next connection's
// traffic spawn a fresh worker at the base window instead of waiting
// out a stale retry backoff.
self.reset_signal_flush_scheduler();
// Note: node_waiters are intentionally NOT cleared here — they are
// cross-connection (callers may register a waiter before an action that
// completes on a subsequent connection, e.g. after 515 reconnect).
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ pub mod pair;
pub mod pair_code;
pub mod passkey;
pub mod request;
pub(crate) mod signal_flush;
pub use request::IqError;
#[cfg(feature = "tokio-runtime")]
pub mod runtime_impl;
Expand Down
12 changes: 5 additions & 7 deletions src/message/receive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -561,16 +561,14 @@ impl Client {
self.handle_msmsg_payload(&info, payload).await;
}

// Live: flush cached Signal state per stanza (WA Web's
// flushBufferToDiskIfNotMemOnlyMode). During the offline drain the
// commit batcher owns the flush — one per batch, before any ack (WA
// Web's bulk signal-store snapshot) — so here only the batch size/byte
// triggers are checked, while the global permit is still held.
// Live: coalesce the receive-side flush. A lost advance re-derives
// forward, so unlike the send path this tolerates the window (see
// `signal_flush.rs`). During the offline drain the commit batcher owns
// the flush instead, so only the batch size/byte triggers run here.
if self.inbound_commit_batch.is_active() {
self.maybe_flush_inbound_commits().await;
} else {
self.flush_signal_cache_logged("message", Some(&info.id))
.await;
self.schedule_signal_flush();
}
}

Expand Down
7 changes: 6 additions & 1 deletion src/send/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1552,7 +1552,12 @@ impl Client {
// Warm marking is visible; a waiting cold send may now re-resolve.
drop(distribution_guard);

// Flush cached Signal state to DB after encryption
// Flush the outbound ratchet advance synchronously before returning:
// a coalesced flush here would let send_message report success while
// the counter/chain-key advance is still only in memory, so a crash
// before the flush would reuse the counter (and its message key + IV)
// on the next send. Only the receive path — where a lost advance
// re-derives forward — coalesces.
self.flush_signal_cache_batch_safe_logged("send_message_impl", None)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: A send awaited from an InboundDurabilityHook during offline drain can now deadlock after the stanza is sent: this batch-safe flush re-acquires the processing permit already held by the hook's commit. Keep this epilogue permit-aware or defer the flush outside that permit-held path.

(Based on your team's feedback about guarding Signal-state settling under the processing permit.)

View Feedback

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

<comment>A send awaited from an `InboundDurabilityHook` during offline drain can now deadlock after the stanza is sent: this batch-safe flush re-acquires the processing permit already held by the hook's commit. Keep this epilogue permit-aware or defer the flush outside that permit-held path.

(Based on your team's feedback about guarding Signal-state settling under the processing permit.) </comment>

<file context>
@@ -1552,11 +1552,14 @@ impl Client {
+        // before the flush would reuse the counter (and its message key + IV)
+        // on the next send. Only the receive path — where a lost advance
+        // re-derives forward — coalesces.
+        self.flush_signal_cache_batch_safe_logged("send_message_impl", None)
+            .await;
 
</file context>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This deadlock is real but pre-existing, not introduced here: origin/main's send epilogue already awaits flush_signal_cache_batch_safe_logged("send_message_impl"), which takes the processing permit during a drain the same way. This PR moved the flush ahead of the wire and propagates its error, but the permit interaction is unchanged from main. Awaiting a send from inside InboundDurabilityHook::on_messages (which holds the permit on the drain path) is against the hook contract anyway — return from the hook and send replies outside it. Worth a separate hardening pass, but it's not a regression of this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're right — this isn't a regression from this PR. The deadlock risk comes from the existing InboundDurabilityHook::on_messages contract/path, not from the send-path change here, so the parent comment is too broad for this diff. It should be handled as a separate hardening pass for the hook/permit boundary.

This comment was influenced by this learning. Open the link to edit it, or reply here to edit or delete it.

.await;

Expand Down
Loading
Loading