-
-
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 6 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,229 @@ | ||
| //! 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_WINDOW`] | ||
| //! 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); | ||
|
|
||
| /// Retry backoff ceiling for a failing flush. The backoff starts at the | ||
| /// window and doubles per consecutive failure, so a long-lived storage | ||
| /// outage settles at one attempt (and one error log) per ceiling instead of | ||
| /// ~40/s at the raw window. | ||
| const SIGNAL_FLUSH_RETRY_CEILING: std::time::Duration = std::time::Duration::from_secs(5); | ||
|
|
||
| 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 { | ||
| let mut backoff = SIGNAL_FLUSH_WINDOW; | ||
| loop { | ||
| // Hold only the Weak across the sleep so an armed fire | ||
| // never extends the client's lifetime. | ||
| runtime.sleep(backoff).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.coalesced_flush_attempt().await else { | ||
| return; | ||
| }; | ||
| // Exponential backoff doubles per consecutive failure and | ||
| // caps the error-log rate along with it; the cache keeps | ||
| // its dirty entries until a flush succeeds. | ||
| backoff = (backoff * 2).min(SIGNAL_FLUSH_RETRY_CEILING); | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
|
||
| log::error!("Coalesced signal flush failed; retrying in {backoff:?}: {e:?}"); | ||
| // 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.
|
||
|
|
||
| /// One flush attempt of the fire loop; tests can inject failures to | ||
| /// exercise the retry/backoff path (same pattern as the commit batcher's | ||
| /// `fail_flushes`). | ||
| async fn coalesced_flush_attempt(&self) -> Result<(), anyhow::Error> { | ||
| #[cfg(test)] | ||
| { | ||
| let remaining = self.signal_flush_test_failures.load(Ordering::Acquire); | ||
| if remaining > 0 { | ||
| self.signal_flush_test_failures | ||
| .store(remaining - 1, Ordering::Release); | ||
| anyhow::bail!("injected coalesced-flush failure"); | ||
| } | ||
| } | ||
| self.flush_signal_cache_batch_safe().await | ||
| } | ||
|
|
||
| #[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" | ||
| ); | ||
| } | ||
|
|
||
| /// Failed attempts re-arm and back off instead of dropping the dirty | ||
| /// state: with 2 injected failures, the fire must consume both error | ||
| /// attempts and still persist the pre-existing dirty entry on the third. | ||
| #[tokio::test] | ||
| async fn failed_fire_retries_until_the_dirty_entry_persists() { | ||
| use std::sync::atomic::Ordering; | ||
|
|
||
| let client = crate::test_utils::create_test_client().await; | ||
| let addr = dirty_session(&client, "15550004001"); | ||
| client | ||
| .signal_flush_test_failures | ||
| .store(2, Ordering::Release); | ||
|
|
||
| client.schedule_signal_flush().await; | ||
|
|
||
| // Success only comes after both injected failures are consumed by the | ||
| // retry loop (25 + 50 ms of backoff), proving the error path re-armed | ||
| // rather than stranding the dirty entry. | ||
| wait_for_backend_session(&client, &addr).await; | ||
| assert_eq!( | ||
| client.signal_flush_test_failures.load(Ordering::Acquire), | ||
| 0, | ||
| "the retry loop must have consumed every injected failure" | ||
| ); | ||
| assert!( | ||
| !client.signal_flush_is_pending(), | ||
| "the successful retry 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.