-
-
Notifications
You must be signed in to change notification settings - Fork 123
perf(signal): coalesce receive flushes and persist outbound state pre-wire #1022
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
Merged
Merged
Changes from 4 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 f2d0212
test(e2e): settle the signal cache before the legacy-DB surgery
jlucaso1 430b221
fix(signal): re-arm the coalesced flush on error and fix window termi…
jlucaso1 1693785
fix(signal): retry the coalesced flush inline instead of recursing
jlucaso1 a75cfc4
fix(signal): back off the failing-flush retry exponentially
jlucaso1 4e1a851
test(signal): cover the failing-flush retry path
jlucaso1 47b33d5
test(e2e): gate connect on the canonical is_ready signal
jlucaso1 b46e161
style(e2e): rustfmt the connect helper
jlucaso1 4e3e736
test(e2e): settle the coalesced flush before inspecting durable sessions
jlucaso1 7be4107
docs(signal): document the settle API's permit and durability precond…
jlucaso1 4d1b8fa
perf(signal): coalesce only the receive flush; keep sends synchronous
jlucaso1 6549dcd
docs(signal): fix orphaned rustdoc and document settle preconditions
jlucaso1 fea31e0
test(e2e): require startup-sync quiescence; prove outbound flush is d…
jlucaso1 9cf00c5
fix(send): flush the outbound ratchet before the stanza hits the wire
jlucaso1 10a06db
fix(signal): make the flush scheduler generation-scoped
jlucaso1 14b9cac
test(e2e),docs: prove the send flush ordering; fix stale coalescing docs
jlucaso1 8a38d46
fix(signal): skip the stale worker's flush after a generation change
jlucaso1 4b6522c
fix(signal): reject stale-generation schedule calls (no scheduler reg…
jlucaso1 935c83c
docs,test(e2e): fix stale per-message-flush rustdoc; settle without r…
jlucaso1 ef1cf76
test(e2e): prove send aborts before the wire when persistence fails
jlucaso1 63fe36b
refactor(wacore): feature-gate InMemoryBackend test hooks behind test…
jlucaso1 1b476b7
test,docs(signal): prove the second flush window and the pre-wire abort
jlucaso1 43561b8
fix(signal): gate coalesced flush writes against teardown cache settle
jlucaso1 d6b2997
docs(e2e): correct the sent-node waiter ordering comment
jlucaso1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| //! Coalesced write-behind for the hot-path Signal cache flushes. | ||
| //! | ||
| //! The live receive path and the send epilogue used to flush the whole dirty | ||
| //! Signal cache to storage once per stanza — a serialize + SQLite transaction | ||
| //! per message, dominated by the session record the ratchet re-dirties every | ||
| //! time. Scheduling through here collapses those into one flush per debounce | ||
| //! window: under load, one storage write covers a burst of messages. | ||
| //! | ||
| //! Durability model (deliberate, bounded): | ||
| //! - Live acks already went out BEFORE the per-stanza flush, so coalescing | ||
| //! does not reorder acks vs durability — it widens the existing | ||
| //! crash-replay window from "one stanza" to at most [`SIGNAL_FLUSH_DEBOUNCE`] | ||
| //! plus one flush. Inbound receive chains re-derive forward after a lost | ||
| //! advance; consumed one-time prekeys stay buffered until their session is | ||
| //! durable (the flush-internal atomicity is untouched). | ||
| //! - The offline drain, retry recovery, identity-change recovery and | ||
| //! teardown keep their synchronous flushes: those paths gate acks, | ||
| //! receipts or follow-up reads on durability and are not routed here. | ||
| //! - Disconnect teardown settles the whole cache itself; a fire that lands | ||
| //! afterwards flushes an empty cache (no-op). | ||
|
|
||
| use std::sync::atomic::Ordering; | ||
|
|
||
| use crate::client::Client; | ||
|
|
||
| /// Fixed coalescing window for the flush. The fire runs this long after the | ||
| /// FIRST request; later requests inside the window ride the same fire (the | ||
| /// deadline is deliberately not extended — a true trailing-edge debounce | ||
| /// would defer the flush indefinitely under continuous traffic, while the | ||
| /// fixed window bounds the maximum deferral). Small enough that the widened | ||
| /// crash window stays negligible next to network RTTs; large enough to fold | ||
| /// a full receive+reply cycle (and bursts) into one storage write. | ||
| const SIGNAL_FLUSH_WINDOW: std::time::Duration = std::time::Duration::from_millis(25); | ||
|
|
||
| impl Client { | ||
| /// Request a Signal-cache flush without paying one storage transaction | ||
| /// per stanza: the first request arms a fixed-window timer and every | ||
| /// request inside the window rides the same fire. | ||
| /// | ||
| /// The pending flag clears BEFORE the fire's flush runs, so a request | ||
| /// that lands mid-flush arms a new fire instead of being absorbed by a | ||
| /// flush that may already have snapshotted the dirty set. A failed flush | ||
| /// re-arms the window, so dirty state is retried instead of sitting | ||
| /// unwritten until unrelated traffic schedules again. | ||
| pub(crate) async fn schedule_signal_flush(&self) { | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
|
||
| if self.signal_flush_pending.swap(true, Ordering::AcqRel) { | ||
| return; | ||
| } | ||
| let Some(weak) = self.self_weak.get() else { | ||
| // Constructor edge: no Arc identity to hold from the timer task. | ||
| // Flush inline so the request is never silently dropped. | ||
| self.signal_flush_pending.store(false, Ordering::Release); | ||
| self.flush_signal_cache_batch_safe_logged("coalesced-inline", None) | ||
| .await; | ||
| return; | ||
| }; | ||
| let weak = weak.clone(); | ||
| let runtime = self.runtime.clone(); | ||
| self.runtime | ||
| .spawn(Box::pin(async move { | ||
| loop { | ||
| // Hold only the Weak across the sleep so an armed fire | ||
| // never extends the client's lifetime. | ||
| runtime.sleep(SIGNAL_FLUSH_WINDOW).await; | ||
| let Some(client) = weak.upgrade() else { | ||
| return; | ||
| }; | ||
| client.signal_flush_pending.store(false, Ordering::Release); | ||
| // Batch-safe: if an offline drain became active meanwhile, | ||
| // this routes under the processing permit like any | ||
| // out-of-band flush. | ||
| let Err(e) = client.flush_signal_cache_batch_safe().await else { | ||
| return; | ||
| }; | ||
| log::error!("Coalesced signal flush failed; re-arming for retry: {e:?}"); | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
|
||
| // Re-arm inline with the same window as the retry backoff; | ||
| // the cache keeps its dirty entries until a flush | ||
| // succeeds. If a concurrent request re-armed already, its | ||
| // fire owns the retry. | ||
| if client.signal_flush_pending.swap(true, Ordering::AcqRel) { | ||
| return; | ||
| } | ||
| } | ||
| })) | ||
| .detach(); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| #[cfg(test)] | ||
| pub(crate) fn signal_flush_is_pending(&self) -> bool { | ||
| self.signal_flush_pending.load(Ordering::Acquire) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use std::sync::Arc; | ||
| use std::time::Duration; | ||
|
|
||
| use wacore::libsignal::protocol::{ProtocolAddress, SessionRecord}; | ||
|
|
||
| async fn backend_session( | ||
| client: &Arc<crate::client::Client>, | ||
| addr: &ProtocolAddress, | ||
| ) -> Option<bytes::Bytes> { | ||
| client | ||
| .persistence_manager | ||
| .backend() | ||
| .get_session(addr.as_str()) | ||
| .await | ||
| .expect("backend read") | ||
| } | ||
|
|
||
| fn dirty_session(client: &Arc<crate::client::Client>, user: &str) -> ProtocolAddress { | ||
| let addr = ProtocolAddress::new(user.to_string(), 1.into()); | ||
| assert!( | ||
| client | ||
| .signal_cache | ||
| .try_put_session(&addr, SessionRecord::new_fresh()) | ||
| .is_ok() | ||
| ); | ||
| addr | ||
| } | ||
|
|
||
| async fn wait_for_backend_session(client: &Arc<crate::client::Client>, addr: &ProtocolAddress) { | ||
| let deadline = wacore::time::Instant::now() + Duration::from_secs(2); | ||
| while backend_session(client, addr).await.is_none() { | ||
| assert!( | ||
| wacore::time::Instant::now() < deadline, | ||
| "scheduled flush never persisted {addr}" | ||
| ); | ||
| tokio::time::sleep(Duration::from_millis(5)).await; | ||
| } | ||
| } | ||
|
|
||
| /// Requests inside one debounce window coalesce into a single armed fire, | ||
| /// and that fire persists every dirty entry written before it. | ||
| #[tokio::test] | ||
| async fn burst_of_requests_coalesces_and_persists() { | ||
| let client = crate::test_utils::create_test_client().await; | ||
|
|
||
| let mut addrs = Vec::new(); | ||
| for i in 0..10 { | ||
| addrs.push(dirty_session(&client, &format!("155500011{i:02}"))); | ||
| client.schedule_signal_flush().await; | ||
| } | ||
| assert!( | ||
| client.signal_flush_is_pending(), | ||
| "burst must ride one armed fire" | ||
| ); | ||
|
|
||
| for addr in &addrs { | ||
| wait_for_backend_session(&client, addr).await; | ||
| } | ||
| assert!( | ||
| !client.signal_flush_is_pending(), | ||
| "the fire must clear the pending flag" | ||
| ); | ||
| } | ||
|
|
||
| /// A request after a completed fire arms a NEW fire — the flag round-trips | ||
| /// and later dirty state is not stranded behind an absorbed request. | ||
| #[tokio::test] | ||
| async fn reschedule_after_fire_flushes_again() { | ||
| let client = crate::test_utils::create_test_client().await; | ||
|
|
||
| let first = dirty_session(&client, "15550002001"); | ||
| client.schedule_signal_flush().await; | ||
| wait_for_backend_session(&client, &first).await; | ||
|
|
||
| let second = dirty_session(&client, "15550002002"); | ||
| client.schedule_signal_flush().await; | ||
| wait_for_backend_session(&client, &second).await; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.