perf: integration benchmarks + allocation optimizations (-23% total, -43% connect allocs) - #551
Conversation
Allocation-counting benchmarks that run against Bartender (mock server) to track real protocol-level allocation regressions in CI. Scenarios: connect-to-ready, send message, send+receive round-trip, reconnect. Outputs customSmallerIsBetter JSON for github-action-benchmark. Optional `dhat-heap` feature for full call-tree profiling via DHAT viewer. Also switches e2e tests from SqliteStore to InMemoryBackend and disables the sqlite-storage default feature on both test crates for faster builds.
DHAT profiling showed `concurrent_queue::Bounded<TransportEvent>::new` as the #1 allocation site at 6.2 MB per connection. The bounded ring buffer pre-allocates all 10,000 slots upfront. 1,024 slots is still generous headroom -- WA Web processes messages inline and a backlog that large means the client can't keep up anyway. Measured impact (bench-integration with mock server): - connect_to_ready: 5.19 MB -> 4.24 MB (-18.4%) - reconnect: 955 KB -> 522 KB (-45.3%) - DHAT total: 37.9 MB -> 32.1 MB (-15.3%) - send/receive paths: unchanged (as expected)
P2: build_upload_prekeys_request now accepts &[u8] slices instead of Vec<u8>, removing 812+ intermediate .to_vec() calls in build_iq(). The NodeBuilder still allocates internally, but the caller no longer creates throwaway Vecs that are immediately consumed. P3: encrypt_group_message, prepare_peer_stanza, and create_sender_key_distribution_message_for_group now accept &ProtocolAddress instead of computing it from &Jid internally. Callers that already have the address (or call multiple functions with the same JID) avoid redundant 64-byte String allocations. In prepare_group_stanza, the address is now built once and shared between SKDM creation and group encryption.
flate2::Decompress allocates ~48KB of internal zlib state on every Decompress::new(). DHAT showed 24 instances (1.1MB binary protocol + 786KB history sync) created and destroyed during a session. Use a thread_local pool that reuses both the Decompress instance (via reset(true)) and the output Vec between calls. The pooled decompressor is used for both binary protocol node decompression and history sync blob inflation. Measured impact: - DHAT total: 32.1 MB -> 30.6 MB (-1.5 MB, -4.6%) - connect_to_ready: 4.25 MB -> 4.12 MB (-130 KB)
Change SignalStore trait prekey methods from Vec<u8> to bytes::Bytes. Bytes::clone() is O(1) (atomic refcount) vs Vec::clone() which copies the entire buffer. The prekey upload path calls store_prekeys_batch twice (pre-upload + mark-uploaded) with 812 keys each. Measured impact: - connect_to_ready allocs: 21,397 -> 19,638 (-1,759 allocs, -8.2%) - connect_to_ready bytes: 4.12 MB -> 4.01 MB (-106 KB) - reconnect bytes: 522 KB -> 511 KB
… reuse Props: Switch AbProp.config_value and AbPropsCache storage from String to CompactString. Most prop values (\"1\", \"true\", \"enabled\") are <=24 bytes and stored inline without heap allocation. Also avoids String clone on cache insertion. Transport: Change Transport::send from Vec<u8> to Bytes. The sender task in NoiseSocket now owns a reusable Vec<u8> for framing output instead of receiving a fresh one per send. Callers no longer need to pre-allocate an encrypted_buf. Measured impact: - connect_to_ready allocs: 19,638 -> 17,172 (-2,466 allocs, -12.6%) - connect_to_ready bytes: 4.01 MB -> 3.98 MB - DHAT total blocks: 147,279 -> 131,823 (-15,456 blocks, -10.5%) - DHAT total bytes: 29.9 MB -> 29.9 MB (block count reduction, not bytes)
Prekey encoding: Replace 812 individual encode_to_vec() calls with a single contiguous buffer + Bytes::slice() sub-views. All prekey records are encoded into one pre-sized Vec, then sliced into zero-copy Bytes views for the store and upload paths. Eliminates 4,872 small Vec allocs. Sender task: encrypt_and_send now accepts Bytes instead of Vec<u8>. The sender task owns a reusable enc_buf for in-place encryption, avoiding per-send buffer allocations for the common small-message path. Measured impact: - connect_to_ready allocs: 17,172 -> 15,597 (-1,575 allocs, -9.2%) - DHAT total blocks: 131,823 -> 123,528 (-8,295 blocks, -6.3%) - DHAT peak live (t-gmax): 20,951 blocks -> 9,285 blocks (-55.7%)
…ndler future Three optimizations targeting remaining allocation hotspots: 1. Zero-copy prekey digest validation: extract_prekey_public_key() reads the publicKey field directly from stored protobuf bytes without full prost decode. Eliminates all PreKeyRecordStructure decode allocs in validate_digest_key (was ~4,872 blocks per session). 2. Smarter marshal_auto: detect large nested child lists (e.g. <iq> -> <list> -> 812 prekey nodes) to trigger capacity pre-estimation. The prekey upload IQ previously started at 256B and grew repeatedly to ~40KB; now it pre-allocates near the correct size. 3. Smaller MessageHandler future: replace get_with_by_ref (which captures the entire worker-spawning closure in the async state machine) with get + conditional insert. The handler future no longer carries the large init closure across the .await on cache hits (~29KB -> smaller). Measured impact: - connect_to_ready allocs: 15,597 -> 12,316 (-3,281 allocs, -21.0%) - connect_to_ready bytes: 4.03 MB -> 3.87 MB (-163 KB) - DHAT total: 30.4 MB -> 29.3 MB (-1.1 MB, -3.6%) - DHAT blocks: 123,528 -> 103,097 (-20,431 blocks, -16.5%)
Split the large monolithic handle_incoming_message async function into two phases: 1. classify_incoming_message: borrows the node tree, extracts owned EncPayload structs (ciphertext Bytes + enc_type + padding_version), returns ClassifiedMessage with all owned data. 2. process_classified_message: holds no node borrows across the heavy decrypt .await points. The async state machine is smaller because it only carries owned data (Arc<MessageInfo>, Vec<EncPayload>, Jid) instead of Vec<&NodeRef> + the entire node tree. Also changes process_session_enc_batch and process_group_enc_batch to accept &[EncPayload] instead of &[&NodeRef], completing the decoupling of decrypt phase from node tree lifetime. The benefit scales with message volume -- each spawned message worker task has a smaller future, reducing per-message heap allocation.
Add OwnedNodeRef::slice_bytes() which returns a Bytes sub-view into the node's backing buffer using pointer arithmetic -- no memcpy. EncPayload::from_owned_node() uses this to extract ciphertext bytes as a zero-copy Bytes view instead of Bytes::copy_from_slice(). Measured impact vs previous commit: - DHAT total: 29.47 MB -> 29.15 MB (-328 KB, -1.1%) - DHAT peak bytes: 5.68 MB -> 5.46 MB (-221 KB, -3.9%) - connect_to_ready bytes: -58 KB (-1.5%)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an integration benchmark workflow and a bench binary; migrates prekey and transport/storage APIs to zero-copy Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as CI Runner
participant Docker as Mock Server (Docker)
participant Artifact as Artifact Store
GH->>Runner: on push/PR -> checkout, setup toolchain, protoc, cache
Runner->>Docker: start mock-server (port 8080, env CHATSTATE_TTL_SECS=3)
Runner->>Docker: poll https://localhost:8080/ until ready
Runner->>Runner: run `cargo run -p bench-integration --release`
Runner->>Artifact: upload `bench_integration_results.json` + `bench_integration.log`
Runner->>GH: invoke benchmark-action (compare or store baseline)
sequenceDiagram
participant App as Incoming OwnedNodeRef
participant Classify as classify_incoming_message
participant Process as process_classified_message
participant Decrypt as Decryption layer
participant Deliver as Delivery
App->>Classify: hand OwnedNodeRef
activate Classify
Classify->>Classify: build Vec<EncPayload> + ClassifiedMessage
Classify-->>App: return ClassifiedMessage
deactivate Classify
App->>Process: pass ClassifiedMessage
activate Process
Process->>Decrypt: process_session_enc_batch / process_group_enc_batch (owned payloads)
Decrypt-->>Process: decrypted messages
Process->>Deliver: deliver messages
deactivate Process
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@claude review carefully, check for some possible bugs introduced, regressions, what can be improved to be DRY and also reduce allocations |
|
Claude finished @jlucaso1's task in 5m 13s —— View job Reviewing PR #551 — Performance Benchmarks + Allocation Optimizations
Bug: TOCTOU Race in Chat Lane Creation (High Severity)
The refactored chat lane initialization has a race condition that can violate the per-chat sequential message processing guarantee. The old code used single-flight // OLD — atomic: only one init closure runs for a given key
let lane = client.chat_lanes.get_with_by_ref(&chat_jid, async { ... }).await;The new code splits into separate // NEW — TOCTOU race window
let lane = if let Some(existing) = client.chat_lanes.get(&chat_jid).await {
existing
} else {
// ... spawn worker, create lane ...
client.chat_lanes.insert(chat_jid.clone(), lane.clone()).await;
lane
};What can go wrong: Two concurrent messages for the same new chat both see Fix: Restore the single-flight pattern. If the goal is to shrink the async state machine, extract the lane construction to a non-async helper and pass it to let lane = client.chat_lanes.get_with_by_ref(&chat_jid, async {
Self::create_chat_lane(&client, spawn_generation)
}).await;Bug:
|
| Category | Issue | Severity |
|---|---|---|
| Bug | TOCTOU race in chat lane creation — violates per-chat ordering | High |
| Bug | out_buf reuse comment is wrong; capacity lost each send |
Low |
| Bug | Zlib pool scratch comment is wrong; capacity not retained | Low |
| Regression | compressed_size_hint ignored in history sync |
Very Low |
| DRY | Benchmark #[cfg] duplication |
Low |
| DRY | EncPayload constructor duplication |
Low |
| Optimization | enc_type: String → EncType enum (eliminate alloc) |
Low |
The optimizations are well-targeted and the benchmark infrastructure is solid. The main issue to fix before merge is the chat lane race condition — it's a concurrency bug that defeats the purpose of the per-chat ordering machinery.
- e2e digest_key test: use extract_prekey_public_key() instead of full prost decode, fix Bytes::as_slice() -> as_ref(), adapt to new compute_key_bundle_digest(&[&[u8]]) signature - message.rs: remove 5 needless_borrow warnings (&enc_type -> enc_type)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79a282cc10
ℹ️ 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".
| match status { | ||
| Status::StreamEnd => break, | ||
| Status::Ok | Status::BufError => { | ||
| // Need more output space | ||
| scratch.reserve(scratch.capacity().max(4096)); |
There was a problem hiding this comment.
When decompress_vec returns Status::BufError because the stream is truncated/corrupt and needs more input (not more output), this loop keeps reserving output space and retries with the same tail, so it can spin forever (and keep growing allocations) instead of returning an error. In this code path (frame/history decompression), that can stall the receive pipeline on malformed compressed payloads.
Useful? React with 👍 / 👎.
| // Return the data by swapping out to avoid cloning. | ||
| // The scratch buffer keeps its capacity for the next call. | ||
| let mut result = Vec::new(); | ||
| std::mem::swap(scratch, &mut result); |
There was a problem hiding this comment.
This swap moves the entire scratch allocation into result, leaving the thread-local scratch vec empty, so the next call reallocates from scratch. That defeats the stated pooling behavior and reintroduces per-call output-buffer allocations in hot decompression paths.
Useful? React with 👍 / 👎.
- Detect truncated/corrupt zlib streams by checking if total_in and total_out made no progress on BufError, and return an error instead of spinning forever growing the output buffer. - Use scratch.clone() instead of mem::swap to return data, so the pooled scratch Vec retains its capacity for subsequent calls.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storages/sqlite-storage/src/sqlite_store.rs (1)
1283-1302:⚠️ Potential issue | 🟠 MajorKeep
Bytesthrough the retry loop.Line 1291 flattens every prekey back into a
Vec<u8>, and Line 1302 deep-clones those buffers again on each retry. That reintroduces the O(total_prekey_bytes) copies this PR is trying to remove on the upload/store hot path.Bytesis already cheap to clone here; only borrowrecord.as_ref()at the Diesel call site.♻️ Proposed fix
- let keys: Vec<(u32, Vec<u8>)> = keys.iter().map(|(id, b)| (*id, b.to_vec())).collect(); + let keys: Arc<Vec<(u32, Bytes)>> = + Arc::new(keys.iter().map(|(id, b)| (*id, b.clone())).collect()); const MAX_RETRIES: u32 = 5; for attempt in 0..=MAX_RETRIES { @@ - let keys_clone = keys.clone(); + let keys_clone = Arc::clone(&keys); let result = tokio::task::spawn_blocking(move || -> std::result::Result<(), DieselOrStore> { @@ - for (id, record) in &keys_clone { + for (id, record) in keys_clone.iter() { diesel::insert_into(prekeys::table) .values(( prekeys::id.eq(*id as i32), - prekeys::key.eq(record), + prekeys::key.eq(record.as_ref()), prekeys::uploaded.eq(uploaded), prekeys::device_id.eq(device_id), ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storages/sqlite-storage/src/sqlite_store.rs` around lines 1283 - 1302, store_prekeys_batch currently converts incoming Bytes to Vec<u8> up front and then deep-clones those Vecs on each retry, reintroducing O(total_prekey_bytes) copies; change the local keys to keep Bytes (e.g., let keys: Vec<(u32, Bytes)> = keys.iter().map(|(id, b)| (*id, b.clone())).collect()), remove the Vec<u8> conversion and avoid deep-cloning keys inside the retry loop (don't clone buffers when creating keys_clone), and only call record.as_ref() (or to_vec()) at the Diesel call site inside the transaction/insert where a borrow or owned slice is required so the expensive copy happens only once if absolutely necessary; refer to function store_prekeys_batch, constants MAX_RETRIES and variables db_semaphore, device_id when locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/bench-integration.yml:
- Around line 9-13: The workflow currently grants overly broad permissions
(contents: write and deployments: write) for all triggers; modify the workflow
so the PR-run uses least privilege by changing the top-level permissions for
pull_request runs to contents: read and only the permission needed to post PR
comments (issues: write or pull-requests: write depending on which API you use),
and remove deployments: write from the PR path; then split the
baseline-publishing/publish job into a push-only job (triggered on push) that
retains contents: write and deployments: write so only the push path can modify
the repo or create deployments.
- Around line 29-45: The workflow uses mutable refs: the mock-server image
"ghcr.io/whiskeysockets-devtools/bartender:latest" and the action
"dtolnay/rust-toolchain@master"; replace them with pinned immutable refs by
updating the mock-server image to the exact image digest (sha256:...) instead of
:latest and change dtolnay/rust-toolchain@master to a specific commit SHA or
released tag; ensure the new image digest and action SHA/tag are recorded in the
workflow so benchmarks remain reproducible and the steps referencing
"mock-server" and the "dtolnay/rust-toolchain" action use those pinned values.
In `@src/handlers/message.rs`:
- Around line 41-91: The current fast-path else branch races: two tasks can both
miss client.chat_lanes and spawn separate ChatLane workers (enqueue_lock and
queue_tx are lane-local), breaking per-chat FIFO; fix by replacing the
get/insert pattern with a single-flight initializer such as
chat_lanes.get_with_by_ref(...) or an atomic get-or-init helper so only one
initializer runs for a given chat_jid, move the heavy worker spawn logic into a
small helper function (e.g., spawn_chat_worker(client.clone(), rx,
spawn_generation)) so the outer future stays small, and ensure you use the
existing session_locks and message_enqueue_locks to serialize per-sender Signal
ops and per-chat enqueueing while creating/inserting the ChatLane (work with
ChatLane, enqueue_lock, queue_tx, client.handle_incoming_message, chat_jid,
chat_lanes).
In `@src/message.rs`:
- Around line 313-324: handle_incoming_message currently drops the node and
calls process_classified_message without acquiring the per-chat
message_enqueue_locks, so classify_incoming_message and subsequent processing
can interleave across messages from the same chat (allowing pkmsg/skmsg
reordering and NoSenderKeyState/duplicate-retry issues). Fix by acquiring the
appropriate message_enqueue_locks guard for the chat/sender before dropping the
node and holding that guard through process_classified_message (similar to how
process_session_enc_batch uses session_lock), i.e. locate
handle_incoming_message and process_classified_message and ensure a
message_enqueue_locks guard is obtained using the chat/sender key prior to
declassification and retained during the async await to serialize per-chat
incoming processing; keep session_lock usage for encrypt/decrypt serialization
as currently done.
In `@src/socket/noise_socket.rs`:
- Around line 81-83: The problem is that using
Bytes::from(std::mem::take(out_buf)) discards the original Vec capacity so
buffer reuse is lost; replace the mem::take call with a mem::replace that
creates a new Vec with the same capacity (e.g. std::mem::replace(&mut out_buf,
Vec::with_capacity(out_buf.capacity()))) so the moved Vec keeps its allocation
and out_buf is replaced with an empty Vec having the original capacity; update
the same pattern at the other occurrence around the second mention (the lines
noted 148-149) and keep the same change for enc_buf if you used the same pattern
there.
In `@tests/bench-integration/src/counting_alloc.rs`:
- Around line 9-19: Implement GlobalAlloc::realloc and alloc_zeroed on
CountingAlloc so reallocations and zeroed allocations forward to System without
inflating ALLOC_COUNT: add an unsafe fn realloc(&self, ptr: *mut u8, old_layout:
Layout, new_size: usize) that calls System.realloc(ptr, old_layout, new_size)
and updates ALLOC_BYTES by adding the positive difference when new_size >
old_layout.size() or subtracting when smaller (use fetch_add/fetch_sub on
ALLOC_BYTES with Ordering::Relaxed), and do NOT increment ALLOC_COUNT in
realloc; also add unsafe fn alloc_zeroed(&self, layout: Layout) that increments
ALLOC_COUNT and ALLOC_BYTES (layout.size()) and forwards to
System.alloc_zeroed(layout) with Ordering::Relaxed. Ensure you reference
CountingAlloc, realloc, alloc_zeroed, ALLOC_COUNT, ALLOC_BYTES, System.realloc
and System.alloc_zeroed in the patch.
In `@tests/bench-integration/src/main.rs`:
- Around line 183-193: The benchmark is measuring allocations from format!
inside the timed closure (const N, measure async || { ...
client_a.client.send_message(... text_msg(&format!("bench-send-{i}"))) ... }),
so move message construction out of the allocation-counted closure: precompute
the N message Strings (or pre-format them into a Vec<String> or Vec<&'static
str>) before calling measure and then inside the measured async closure call
text_msg/send_message using those prebuilt messages to ensure
alloc_count/alloc_bytes reflect only the library send/receive path.
In `@tests/e2e/src/lib.rs`:
- Around line 57-59: The e2e helper connect_inner currently hardcodes
InMemoryBackend which skips the sqlite path; modify connect_inner to accept a
backend selector (e.g., an enum or Option<&str> backend_type) or use
cfg(feature="bench") to choose InMemoryBackend only for benchmarks, and
otherwise construct the sqlite-backed implementation (wrap as Arc<dyn Backend>)
so e2e tests exercise sqlite storage; update call sites that invoke
connect_inner (tests/bench harness) to pass the appropriate selector or enable
the bench feature, and keep
TokioWebSocketTransportFactory::new().with_url(mock_server_url()) usage
unchanged.
In `@wacore/binary/src/marshal.rs`:
- Around line 147-155: The auto-reserve heuristics diverge:
should_auto_reserve_node checks one level deeper for large nested lists but
should_auto_reserve_node_ref does not, causing marshal_ref_auto to miss the same
reservation opportunity; update should_auto_reserve_node_ref to mirror the logic
in should_auto_reserve_node by returning true if children.len() >=
AUTO_RESERVE_CHILDREN_THRESHOLD or if any direct child has content
Some(NodeContent::Nodes(gc)) with gc.len() >= AUTO_RESERVE_CHILDREN_THRESHOLD so
both heuristics stay in sync.
In `@wacore/src/prekeys.rs`:
- Around line 69-77: decode_varint currently accepts a malformed 10th varint
byte which can have payload bits > 1 and yield an incorrect offset; update
decode_varint so that when iterating the 10th byte (i == 9) you explicitly
reject any payload > 1 (i.e. if (byte & 0x7F) > 1 return None), and continue to
return None if the loop finishes without a terminating byte; this fixes callers
like extract_prekey_public_key that rely on decode_varint returning None for
invalid encodings.
---
Outside diff comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 1283-1302: store_prekeys_batch currently converts incoming Bytes
to Vec<u8> up front and then deep-clones those Vecs on each retry, reintroducing
O(total_prekey_bytes) copies; change the local keys to keep Bytes (e.g., let
keys: Vec<(u32, Bytes)> = keys.iter().map(|(id, b)| (*id,
b.clone())).collect()), remove the Vec<u8> conversion and avoid deep-cloning
keys inside the retry loop (don't clone buffers when creating keys_clone), and
only call record.as_ref() (or to_vec()) at the Diesel call site inside the
transaction/insert where a borrow or owned slice is required so the expensive
copy happens only once if absolutely necessary; refer to function
store_prekeys_batch, constants MAX_RETRIES and variables db_semaphore, device_id
when locating the change.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f38ec1c6-d1ab-4376-a475-71b0cbe4f081
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.github/workflows/bench-integration.yml.gitignoreCargo.tomlsrc/appstate_sync.rssrc/client.rssrc/features/signal.rssrc/handlers/message.rssrc/handshake.rssrc/message.rssrc/prekeys.rssrc/send.rssrc/socket/noise_socket.rssrc/store/signal.rssrc/transport.rsstorages/sqlite-storage/Cargo.tomlstorages/sqlite-storage/src/sqlite_store.rstests/bench-integration/Cargo.tomltests/bench-integration/src/counting_alloc.rstests/bench-integration/src/main.rstests/e2e/Cargo.tomltests/e2e/src/lib.rstransports/tokio-transport/src/lib.rswacore/benches/send_receive_benchmark.rswacore/binary/src/lib.rswacore/binary/src/marshal.rswacore/binary/src/node.rswacore/binary/src/util.rswacore/binary/src/zlib_pool.rswacore/src/history_sync.rswacore/src/iq/prekeys.rswacore/src/iq/props.rswacore/src/net.rswacore/src/prekeys.rswacore/src/send.rswacore/src/store/ab_props.rswacore/src/store/in_memory.rswacore/src/store/traits.rs
| mock-server: | ||
| image: ghcr.io/whiskeysockets-devtools/bartender:latest | ||
| credentials: | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.BARTENDER_GHCR_TOKEN }} | ||
| ports: | ||
| - 8080:8080 | ||
| env: | ||
| CHATSTATE_TTL_SECS: "3" | ||
| options: --log-driver none | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
|
|
||
| - uses: dtolnay/rust-toolchain@master | ||
| with: | ||
| toolchain: nightly-2026-04-05 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/bench-integration.yml | sed -n '1,60p'Repository: jlucaso1/whatsapp-rust
Length of output: 1873
🏁 Script executed:
# Search for any version tags or references to bartender in the repo
rg -i "bartender" --type yaml --type mdRepository: jlucaso1/whatsapp-rust
Length of output: 48
🌐 Web query:
dtolnay rust-toolchain action pinning best practices recommended version
💡 Result:
Best practices for pinning the dtolnay/rust-toolchain GitHub Action: Use specific tags or commit SHAs for reproducibility and security. The action's @rev determines the default toolchain (e.g., dtolnay/rust-toolchain@stable installs the current stable Rust, dtolnay/rust-toolchain@1.94.0 installs Rust 1.94.0). Recommended approaches: - For latest stable: - uses: dtolnay/rust-toolchain@stable - For pinned version: - uses: dtolnay/rust-toolchain@1.94.0 (matches current stable as of 2026-04-15) - For explicit toolchain with master: - uses: dtolnay/rust-toolchain@master with: toolchain: '1.94.0' For maximum security, pin to a full commit SHA from master branch history (e.g., dtolnay/rust-toolchain@315e265), as tags like @stable are lightweight and follow latest in channel. Combine with rust-toolchain.toml in repo root for project-specific pinning (e.g., channel = "1.94.0"), which rustup respects automatically. This ensures consistent builds, avoids surprise lints from un-pinned updates, and follows GitHub security guidelines for third-party actions. Current stable Rust version (2026-04-15): 1.94.1 (released 2026-03-26), recommended for most projects.
Citations:
- 1: https://github.com/dtolnay/rust-toolchain
- 2: Support hash-based pinning of action version dtolnay/rust-toolchain#126
- 3: https://github.com/dtolnay/rust-toolchain/blob/master/README.md
- 4: https://doc.rust-lang.org/stable/releases.html
- 5: https://doc.rust-lang.org/releases.html
- 6: https://releases.rs/
🌐 Web query:
whiskeysockets bartender docker image releases versions
💡 Result:
No official whiskeysockets/bartender Docker image or releases found. WhiskeySockets maintains Baileys (WhatsApp Web API library) with latest release v7.0.0-rc.9 on 2025-11-21. "Bartender" appears in Baileys PR #2191 context as an internal testing/benchmark tool for socket changes (e.g., "destructive benchmark test on bartender"), but no public repository, Docker image, or version releases for it exist in search results. Related Baileys Docker images like devjuanda/baileys-server-pro exist but are third-party. Unrelated projects like barassistant/server (cocktail app) have Docker tags including 'dev'.
Citations:
- 1: https://github.com/WhiskeySockets/Baileys/releases
- 2: https://www.npmjs.com/package/@whiskeysockets/baileys
- 3: feat: add cleanup handlers to prevent memory leaks on socket close WhiskeySockets/Baileys#2191
- 4: https://hub.docker.com/r/devjuanda/baileys-server-pro
- 5: https://github.com/whiskeysockets/Baileys
Pin the benchmark environment to immutable refs.
bartender:latest and dtolnay/rust-toolchain@master both use mutable references that drift over time, making benchmark baselines non-reproducible and risking unexpected workflow breakage. Pin the image to a digest and the action to a commit SHA or tag.
Proposed pinning pattern
- image: ghcr.io/whiskeysockets-devtools/bartender:latest
+ image: ghcr.io/whiskeysockets-devtools/bartender@sha256:<verified-digest>
...
- - uses: dtolnay/rust-toolchain@master
+ - uses: dtolnay/rust-toolchain@<verified-commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/bench-integration.yml around lines 29 - 45, The workflow
uses mutable refs: the mock-server image
"ghcr.io/whiskeysockets-devtools/bartender:latest" and the action
"dtolnay/rust-toolchain@master"; replace them with pinned immutable refs by
updating the mock-server image to the exact image digest (sha256:...) instead of
:latest and change dtolnay/rust-toolchain@master to a specific commit SHA or
released tag; ensure the new image digest and action SHA/tag are recorded in the
workflow so benchmarks remain reproducible and the steps referencing
"mock-server" and the "dtolnay/rust-toolchain" action use those pinned values.
| pub(crate) async fn handle_incoming_message(self: Arc<Self>, node: Arc<OwnedNodeRef>) { | ||
| // Phase 1: classify borrows the node tree, extracts owned payloads, returns quickly. | ||
| // Phase 2: process_classified_message holds no node borrows across heavy .await points, | ||
| // keeping the async state machine small. | ||
| let classified = match self.classify_incoming_message(&node).await { | ||
| Some(c) => c, | ||
| None => return, | ||
| }; | ||
| // node is no longer borrowed here -- drop it before the heavy phase | ||
| drop(node); | ||
| self.process_classified_message(classified).await; | ||
| } |
There was a problem hiding this comment.
This refactor still leaves per-chat incoming processing unordered.
process_session_enc_batch() keeps the per-sender session_lock, but the new handle_incoming_message()/process_classified_message() path still doesn't take a message_enqueue_locks guard. That allows two messages from the same chat to interleave across classify/decrypt, which can reorder an SKDM-bearing pkmsg behind a later skmsg and reintroduce NoSenderKeyState/duplicate-retry behavior.
As per coding guidelines, "Use session_locks to serialize per-sender Signal encrypt/decrypt operations and message_enqueue_locks to serialize per-chat incoming message processing".
Also applies to: 518-692
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 313 - 324, handle_incoming_message currently
drops the node and calls process_classified_message without acquiring the
per-chat message_enqueue_locks, so classify_incoming_message and subsequent
processing can interleave across messages from the same chat (allowing
pkmsg/skmsg reordering and NoSenderKeyState/duplicate-retry issues). Fix by
acquiring the appropriate message_enqueue_locks guard for the chat/sender before
dropping the node and holding that guard through process_classified_message
(similar to how process_session_enc_batch uses session_lock), i.e. locate
handle_incoming_message and process_classified_message and ensure a
message_enqueue_locks guard is obtained using the chat/sender key prior to
declassification and retained during the async await to serialize per-chat
incoming processing; keep session_lock usage for encrypt/decrypt serialization
as currently done.
| unsafe impl GlobalAlloc for CountingAlloc { | ||
| unsafe fn alloc(&self, layout: Layout) -> *mut u8 { | ||
| ALLOC_COUNT.fetch_add(1, Ordering::Relaxed); | ||
| ALLOC_BYTES.fetch_add(layout.size() as u64, Ordering::Relaxed); | ||
| unsafe { System.alloc(layout) } | ||
| } | ||
|
|
||
| unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { | ||
| unsafe { System.dealloc(ptr, layout) } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GlobalAlloc realloc default implementation Rust std behavior
💡 Result:
The GlobalAlloc trait in Rust's standard library provides a default implementation for the realloc method. This default behavior, used by any global allocator that does not override it (including the platform-specific System allocator when it relies on the trait default), performs realloc by allocating a new block of the requested size and alignment, copying the minimum of the old and new sizes from the old pointer to the new one using ptr::copy_nonoverlapping, deallocating the old block, and returning the new pointer (or null if allocation fails). Here is the exact source code from core::alloc::global.rs: unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { // SAFETY: the caller must ensure that the new_size does not overflow. // layout.align comes from a Layout and is thus guaranteed to be valid. let new_layout = unsafe { Layout::from_size_align_unchecked(new_size, layout.align) }; // SAFETY: the caller must ensure that new_layout is greater than zero. let new_ptr = unsafe { self.alloc(new_layout) }; if !new_ptr.is_null { // SAFETY: the previously allocated block cannot overlap the newly allocated block. // The safety contract for dealloc must be upheld by the caller. unsafe { ptr::copy_nonoverlapping(ptr, new_ptr, cmp::min(layout.size, new_size)); self.dealloc(ptr, layout); } } new_ptr } The default global allocator for executables is platform-specific and unspecified (often System, which wraps system calls like malloc/realloc on Unix or HeapReAlloc on Windows), but since System implements GlobalAlloc and the docs do not indicate it overrides realloc, it uses this trait default. For libraries like cdylib and staticlib, System is guaranteed. The realloc free function in std::alloc and alloc::alloc forwards to the registered global allocator's realloc (or the default).
Citations:
- 1: https://doc.rust-lang.org/std/alloc/trait.GlobalAlloc.html
- 2: https://doc.rust-lang.org/alloc/alloc/fn.realloc.html
- 3: https://doc.rust-lang.org/alloc/alloc/trait.GlobalAlloc.html
- 4: https://doc.rust-lang.org/stable/src/std/alloc.rs.html
- 5: https://doc.rust-lang.org/src/core/alloc/global.rs.html
- 6: https://doc.rust-lang.org/std/alloc/struct.System.html
- 7: https://doc.rust-lang.org/stable/std/alloc/trait.GlobalAlloc.html
- 8: https://doc.rust-lang.org/1.60.0/src/core/alloc/global.rs.html
- 9: https://github.com/rust-lang/rust/blob/master/library/alloc/src/alloc.rs
- 10: https://dev-doc.rust-lang.org/stable/src/core/alloc/global.rs.html
🏁 Script executed:
# Check the repository structure and find related context
cd "$(git rev-parse --show-toplevel)" 2>/dev/null || true
find . -type f -name "*.rs" | grep -E "(alloc|bench)" | head -20Repository: jlucaso1/whatsapp-rust
Length of output: 358
🏁 Script executed:
# Look at the actual CountingAlloc implementation and its usage
find . -name "counting_alloc.rs" -exec cat -n {} +Repository: jlucaso1/whatsapp-rust
Length of output: 1723
🏁 Script executed:
# Check if there are any existing tests or usage of CountingAlloc
rg "CountingAlloc" -A 5 -B 5Repository: jlucaso1/whatsapp-rust
Length of output: 1793
🏁 Script executed:
# Check what the benchmark does and if it might trigger realloc/alloc_zeroed
cd "$(git rev-parse --show-toplevel)" 2>/dev/null || true
cat -n tests/bench-integration/src/main.rs | head -100Repository: jlucaso1/whatsapp-rust
Length of output: 3631
🏁 Script executed:
# Search for any explicit uses of realloc or alloc_zeroed in the codebase
rg "realloc|alloc_zeroed" -i --type rsRepository: jlucaso1/whatsapp-rust
Length of output: 92
🏁 Script executed:
# Search for explicit uses of realloc or alloc_zeroed in the codebase
rg "realloc|alloc_zeroed" -iRepository: jlucaso1/whatsapp-rust
Length of output: 254
🏁 Script executed:
# Check if any dependencies or standard library usage would trigger realloc/alloc_zeroed
# Look at what allocations might happen during benchmarks
cd "$(git rev-parse --show-toplevel)" 2>/dev/null || true
cat Cargo.toml | head -50Repository: jlucaso1/whatsapp-rust
Length of output: 1368
🏁 Script executed:
# Check if Vec, String, or other containers that might use realloc are used in benchmarks
rg "Vec|String|realloc" tests/bench-integration/src/main.rs | head -30Repository: jlucaso1/whatsapp-rust
Length of output: 164
Override realloc and alloc_zeroed to forward to System with counting.
The default GlobalAlloc::realloc implementation calls self.alloc() internally, which increments your allocation counters. This transforms every reallocation into a counted allocation event, inflating both ALLOC_COUNT and ALLOC_BYTES. Collections like Vec and String in the benchmark will trigger this, distorting the metrics you're measuring. alloc_zeroed has the same issue—it allocates via self.alloc() then zeros the memory.
Proposed fix
unsafe impl GlobalAlloc for CountingAlloc {
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
ALLOC_COUNT.fetch_add(1, Ordering::Relaxed);
ALLOC_BYTES.fetch_add(layout.size() as u64, Ordering::Relaxed);
unsafe { System.alloc(layout) }
}
+ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+ ALLOC_COUNT.fetch_add(1, Ordering::Relaxed);
+ ALLOC_BYTES.fetch_add(layout.size() as u64, Ordering::Relaxed);
+ unsafe { System.alloc_zeroed(layout) }
+ }
+
+ unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 {
+ ALLOC_COUNT.fetch_add(1, Ordering::Relaxed);
+ ALLOC_BYTES.fetch_add(new_size as u64, Ordering::Relaxed);
+ unsafe { System.realloc(ptr, layout, new_size) }
+ }
+
unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
unsafe { System.dealloc(ptr, layout) }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/bench-integration/src/counting_alloc.rs` around lines 9 - 19, Implement
GlobalAlloc::realloc and alloc_zeroed on CountingAlloc so reallocations and
zeroed allocations forward to System without inflating ALLOC_COUNT: add an
unsafe fn realloc(&self, ptr: *mut u8, old_layout: Layout, new_size: usize) that
calls System.realloc(ptr, old_layout, new_size) and updates ALLOC_BYTES by
adding the positive difference when new_size > old_layout.size() or subtracting
when smaller (use fetch_add/fetch_sub on ALLOC_BYTES with Ordering::Relaxed),
and do NOT increment ALLOC_COUNT in realloc; also add unsafe fn
alloc_zeroed(&self, layout: Layout) that increments ALLOC_COUNT and ALLOC_BYTES
(layout.size()) and forwards to System.alloc_zeroed(layout) with
Ordering::Relaxed. Ensure you reference CountingAlloc, realloc, alloc_zeroed,
ALLOC_COUNT, ALLOC_BYTES, System.realloc and System.alloc_zeroed in the patch.
| async fn connect_inner(_prefix: &str, push_name: Option<String>) -> anyhow::Result<Self> { | ||
| let backend = Arc::new(InMemoryBackend::new()) as Arc<dyn Backend>; | ||
| let transport_factory = TokioWebSocketTransportFactory::new().with_url(mock_server_url()); |
There was a problem hiding this comment.
Don't hardwire the e2e harness to the in-memory backend.
This helper now stops exercising the sqlite-backed storage path entirely. Since this PR also changes storages/sqlite-storage/src/sqlite_store.rs, backend-specific regressions can pass e2e unnoticed. I'd keep the benchmark path on InMemoryBackend, but make the e2e helper backend-selectable or retain a sqlite-backed e2e lane for the default feature set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/src/lib.rs` around lines 57 - 59, The e2e helper connect_inner
currently hardcodes InMemoryBackend which skips the sqlite path; modify
connect_inner to accept a backend selector (e.g., an enum or Option<&str>
backend_type) or use cfg(feature="bench") to choose InMemoryBackend only for
benchmarks, and otherwise construct the sqlite-backed implementation (wrap as
Arc<dyn Backend>) so e2e tests exercise sqlite storage; update call sites that
invoke connect_inner (tests/bench harness) to pass the appropriate selector or
enable the bench feature, and keep
TokioWebSocketTransportFactory::new().with_url(mock_server_url()) usage
unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f47371abec
ℹ️ 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".
| let lane = if let Some(existing) = client.chat_lanes.get(&chat_jid).await { | ||
| existing | ||
| } else { |
There was a problem hiding this comment.
This miss-path now does a separate get and later insert, which is racy under concurrent first messages for the same chat: two tasks can both observe a miss, each spawn its own worker/queue, and enqueue different stanzas to different lanes before one insert wins. That breaks the per-chat serialization guarantee in this handler (the reason this queue exists), so pkmsg/skmsg ordering can be inverted and decryption/retry behavior can become inconsistent.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if let Err(e) = transport.send(out_buf).await { | ||
| let frame = bytes::Bytes::from(std::mem::take(out_buf)); |
There was a problem hiding this comment.
Converting with Bytes::from(std::mem::take(out_buf)) moves the backing allocation out of the reusable buffer and leaves out_buf empty, so the next encode_frame_into call must allocate again. In high-throughput sends this defeats the sender-task buffer pooling introduced in this change and reintroduces per-message allocations in the hot path.
Useful? React with 👍 / 👎.
…enum - Restore get_with_by_ref for chat lane creation to fix TOCTOU race that could create duplicate workers for the same chat. Extract init logic to create_chat_lane() helper to keep the future small. - Fix out_buf reuse in NoiseSocket: use Bytes::copy_from_slice + clear instead of mem::take so the Vec retains capacity between sends. - Change EncPayload.enc_type from String to EncType enum, eliminating one heap allocation per enc node and redundant from_wire() parsing. - DRY EncPayload constructors via shared from_parts(). - Restore removed debug log for retry count pre-seeding.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/message.rs (1)
313-324:⚠️ Potential issue | 🟠 MajorSerialize this path with
message_enqueue_locks.
handle_incoming_message()still hands the classified message straight intoprocess_classified_message()without a per-chat enqueue guard. That means two messages from the same chat can still classify/decrypt concurrently and let a laterskmsgovertake the earlierpkmsg/SKDM, bringing backNoSenderKeyStateand duplicate retry noise.As per coding guidelines, "Use
session_locksto serialize per-sender Signal encrypt/decrypt operations andmessage_enqueue_locksto serialize per-chat incoming message processing".Also applies to: 518-692
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message.rs` around lines 313 - 324, handle_incoming_message currently calls process_classified_message immediately after classification, allowing concurrent processing of messages from the same chat; fix by acquiring the per-chat enqueue lock from message_enqueue_locks (using the ClassifiedMessage's chat identifier) before calling process_classified_message and hold it until processing completes, ensuring serialization; keep the existing drop(node) before the heavy phase, obtain the lock after classification and release it after process_classified_message finishes so other messages for the same chat are queued; update handle_incoming_message to use message_enqueue_locks with the ClassifiedMessage returned by classify_incoming_message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/message.rs`:
- Around line 41-45: The code in from_owned_node is truncating out-of-range
padding versions by casting optional_u64("v") as u8 which wraps values (e.g. 256
-> 0); change this to validate the u64 before conversion and reject invalid
values instead of truncating: retrieve the optional u64, attempt a fallible
conversion (e.g. u8::try_from or .try_into()), and if conversion fails treat the
node as invalid (return None) or use the same clamping/validation semantics used
for count; apply the same fix to the other occurrence around the block at the
second padding-version read (the lines noted as 55-58) so both places explicitly
check range and do not silently wrap.
- Around line 32-35: EncPayload currently stores enc_type as a heap String and
is reparsed immediately in process_session_enc_batch(); change
EncPayload.enc_type to use the enum/type EncType directly, update all
constructors/builders that create EncPayload (search for new EncPayload {
enc_type: ... } and helpers that populate session_payloads/group_payloads) to
pass an EncType instead of a String, and remove the redundant parse in
process_session_enc_batch() so it consumes the EncType directly. Also update any
serde/serialization, trait bounds (Clone/Copy/Serialize/Deserialize) and
function signatures that assumed a String so they accept or clone EncType, and
apply the same change to the other payload structs/usages mentioned (the similar
structs/usages around the other occurrences referenced) so all consumers operate
on EncType rather than re-parsing a String.
In `@wacore/binary/src/zlib_pool.rs`:
- Around line 67-68: The code currently returns Ok(scratch.clone()) which
preserves the buffer's full capacity and can pin large allocations; instead
create a new Vec that only contains the used bytes so capacity isn't retained.
Replace the clone return with returning a trimmed copy (e.g. use
scratch.as_slice().to_vec() or scratch[..used_len].to_vec()) so the pooled
scratch buffer can keep its smaller capacity while callers get a right-sized
Vec; update the return in the function that constructs/returns scratch (the
variable named scratch) accordingly.
---
Duplicate comments:
In `@src/message.rs`:
- Around line 313-324: handle_incoming_message currently calls
process_classified_message immediately after classification, allowing concurrent
processing of messages from the same chat; fix by acquiring the per-chat enqueue
lock from message_enqueue_locks (using the ClassifiedMessage's chat identifier)
before calling process_classified_message and hold it until processing
completes, ensuring serialization; keep the existing drop(node) before the heavy
phase, obtain the lock after classification and release it after
process_classified_message finishes so other messages for the same chat are
queued; update handle_incoming_message to use message_enqueue_locks with the
ClassifiedMessage returned by classify_incoming_message.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3273c5d-8fed-4a3b-b477-664e3817150a
📒 Files selected for processing (3)
src/message.rstests/e2e/tests/digest_key.rswacore/binary/src/zlib_pool.rs
| pub(crate) fn from_owned_node(owner: &OwnedNodeRef, enc_node: &NodeRef<'_>) -> Option<Self> { | ||
| let raw = enc_node.content_bytes()?; | ||
| let ciphertext = owner.slice_bytes(raw); | ||
| let enc_type = enc_node.attrs().optional_string("type")?.to_string(); | ||
| let padding_version = enc_node.attrs().optional_u64("v").unwrap_or(2) as u8; |
There was a problem hiding this comment.
Reject out-of-range padding versions instead of truncating them.
optional_u64("v") as u8 silently wraps malformed values (256 -> 0, etc.). On network input that can turn an invalid payload into an apparently valid padding version instead of skipping it cleanly. Validate the range explicitly here, the same way count is clamped above.
Also applies to: 55-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 41 - 45, The code in from_owned_node is
truncating out-of-range padding versions by casting optional_u64("v") as u8
which wraps values (e.g. 256 -> 0); change this to validate the u64 before
conversion and reject invalid values instead of truncating: retrieve the
optional u64, attempt a fallible conversion (e.g. u8::try_from or .try_into()),
and if conversion fails treat the node as invalid (return None) or use the same
clamping/validation semantics used for count; apply the same fix to the other
occurrence around the block at the second padding-version read (the lines noted
as 55-58) so both places explicitly check range and do not silently wrap.
| // Clone data out so scratch retains its capacity for the next call | ||
| Ok(scratch.clone()) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cap retained pooled buffer capacity after large payloads.
Keeping full capacity forever (scratch.clone()) can pin tens of MiB per thread after a single large history-sync payload.
♻️ Proposed refinement
+ const RETAIN_CAP: usize = 8 * 1024 * 1024;
// Clone data out so scratch retains its capacity for the next call
- Ok(scratch.clone())
+ let out = scratch.clone();
+ if scratch.capacity() > RETAIN_CAP {
+ scratch.shrink_to(RETAIN_CAP);
+ }
+ Ok(out)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Clone data out so scratch retains its capacity for the next call | |
| Ok(scratch.clone()) | |
| const RETAIN_CAP: usize = 8 * 1024 * 1024; | |
| // Clone data out so scratch retains its capacity for the next call | |
| let out = scratch.clone(); | |
| if scratch.capacity() > RETAIN_CAP { | |
| scratch.shrink_to(RETAIN_CAP); | |
| } | |
| Ok(out) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/binary/src/zlib_pool.rs` around lines 67 - 68, The code currently
returns Ok(scratch.clone()) which preserves the buffer's full capacity and can
pin large allocations; instead create a new Vec that only contains the used
bytes so capacity isn't retained. Replace the clone return with returning a
trimmed copy (e.g. use scratch.as_slice().to_vec() or
scratch[..used_len].to_vec()) so the pooled scratch buffer can keep its smaller
capacity while callers get a right-sized Vec; update the return in the function
that constructs/returns scratch (the variable named scratch) accordingly.
- zlib pool: enforce max_size cap before decompress_vec to prevent compressed bombs; zero-copy return via mem::take + re-reserve scratch - extract_prekey_public_key: use last-one-wins semantics per protobuf spec; gracefully skip unknown wire types instead of returning None - decode_varint: reject 10th byte with payload > 1 bit (overflow guard) - marshal_ref_auto: mirror nested child list check from marshal_auto so both heuristics stay in sync - CountingAlloc: implement realloc (adjusts bytes, not count) and alloc_zeroed (tracks both) for accurate measurement - bench-integration: precompute format! strings outside measured closure - sqlite store_prekeys_batch: keep Bytes through retry loop, only call as_ref() at diesel bind site - CI workflow: split into PR (read-only) and push (write) jobs with least-privilege permissions
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/socket/noise_socket.rs (1)
279-300: 🧹 Nitpick | 🔵 TrivialHeads-up: Potential test flakiness in FIFO ordering assertion.
The test spawns 10 concurrent tasks and expects the recorded order to be exactly
[0,1,2,...,9]. While the sender channel preserves FIFO order for jobs as they arrive, there's no guarantee that spawned tasks will callencrypt_and_sendin their spawn order—task scheduling is non-deterministic.In practice this often passes because tasks execute their first await quickly, but it could flake under load. Consider either:
- Removing the strict ordering assertion (verify all 10 completed, order can vary), or
- Introducing synchronization (e.g., a barrier or sequential awaits) to enforce enqueue order if strict FIFO testing is the goal.
Since this is pre-existing test logic (only the
Bytesconversion changed), this is informational rather than blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/socket/noise_socket.rs` around lines 279 - 300, The FIFO ordering assertion is flaky because spawned tasks may call encrypt_and_send out of spawn order; update the test around the spawn loop and the assert on recorded_order to either (A) remove the strict ordering check and only assert that all 10 distinct ids were recorded (i.e., verify length and contents ignoring order), or (B) enforce enqueue order by synchronizing task start before calling socket.encrypt_and_send (e.g., create a tokio::sync::Barrier or have the loop await each send sequentially) so that the calls into encrypt_and_send occur in deterministic order; adjust the code that creates handles (the for i in 0..10 { ... tokio::spawn(...) }) and the final assertion on recorded_order to implement your chosen approach.
♻️ Duplicate comments (1)
src/message.rs (1)
40-40:⚠️ Potential issue | 🟡 MinorPadding version truncation issue persists.
The cast
optional_u64("v") as u8silently wraps out-of-range values (e.g.,256 → 0). On network input, this could turn an invalid payload into an apparently valid padding version. Validate the range explicitly before conversion.🛡️ Proposed fix to validate padding version range
- let padding_version = enc_node.attrs().optional_u64("v").unwrap_or(2) as u8; + let padding_version = enc_node + .attrs() + .optional_u64("v") + .unwrap_or(2) + .try_into() + .ok() + .filter(|&v: &u8| v <= 2)?; // Known versions are 0, 1, 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message.rs` at line 40, The current cast let padding_version = enc_node.attrs().optional_u64("v").unwrap_or(2) as u8 can silently wrap out-of-range values; change it to read the optional_u64 result into a temporary (e.g., let v_opt = enc_node.attrs().optional_u64("v")), then if Some(v) validate v is <= u8::MAX before converting to u8 (otherwise return/propagate an error or reject the payload); keep the default of 2 when None. Ensure you reference enc_node.attrs().optional_u64("v") and the padding_version variable when implementing the explicit range check and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/message.rs`:
- Line 85: The worker loop is unnecessarily boxing the future returned by the
async function client.handle_incoming_message; remove the Box::pin(...) wrapper
and await the future directly (i.e., replace
Box::pin(client.handle_incoming_message(msg_node)).await with
client.handle_incoming_message(msg_node).await) to avoid the heap allocation in
this hot path, and remove any now-unused imports or allocations introduced
solely for boxing.
---
Outside diff comments:
In `@src/socket/noise_socket.rs`:
- Around line 279-300: The FIFO ordering assertion is flaky because spawned
tasks may call encrypt_and_send out of spawn order; update the test around the
spawn loop and the assert on recorded_order to either (A) remove the strict
ordering check and only assert that all 10 distinct ids were recorded (i.e.,
verify length and contents ignoring order), or (B) enforce enqueue order by
synchronizing task start before calling socket.encrypt_and_send (e.g., create a
tokio::sync::Barrier or have the loop await each send sequentially) so that the
calls into encrypt_and_send occur in deterministic order; adjust the code that
creates handles (the for i in 0..10 { ... tokio::spawn(...) }) and the final
assertion on recorded_order to implement your chosen approach.
---
Duplicate comments:
In `@src/message.rs`:
- Line 40: The current cast let padding_version =
enc_node.attrs().optional_u64("v").unwrap_or(2) as u8 can silently wrap
out-of-range values; change it to read the optional_u64 result into a temporary
(e.g., let v_opt = enc_node.attrs().optional_u64("v")), then if Some(v) validate
v is <= u8::MAX before converting to u8 (otherwise return/propagate an error or
reject the payload); keep the default of 2 when None. Ensure you reference
enc_node.attrs().optional_u64("v") and the padding_version variable when
implementing the explicit range check and error handling.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6f407fa-427b-4fa9-8a00-95ec85fb36d9
📒 Files selected for processing (3)
src/handlers/message.rssrc/message.rssrc/socket/noise_socket.rs
| } | ||
| let start = wacore::time::now_millis() as u64; | ||
| let client = client_for_worker.clone(); | ||
| Box::pin(client.handle_incoming_message(msg_node)).await; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify handle_incoming_message signature and current call sites.
# Expectation: if it returns an awaitable future directly, Box::pin is unnecessary.
set -euo pipefail
rg -n -C3 '\bhandle_incoming_message\s*\(' --type rustRepository: jlucaso1/whatsapp-rust
Length of output: 3552
Remove unnecessary Box::pin() wrapping in the worker loop.
handle_incoming_message is an async fn that returns a future directly; it can be awaited without boxing. All other call sites in the codebase use direct .await, and the heap allocation from Box::pin() is unnecessary in this hot path.
♻️ Suggested change
- let client = client_for_worker.clone();
- Box::pin(client.handle_incoming_message(msg_node)).await;
+ let client = client_for_worker.clone();
+ client.handle_incoming_message(msg_node).await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/message.rs` at line 85, The worker loop is unnecessarily boxing
the future returned by the async function client.handle_incoming_message; remove
the Box::pin(...) wrapper and await the future directly (i.e., replace
Box::pin(client.handle_incoming_message(msg_node)).await with
client.handle_incoming_message(msg_node).await) to avoid the heap allocation in
this hot path, and remove any now-unused imports or allocations introduced
solely for boxing.
There was a problem hiding this comment.
The Box::pin here is intentional -- handle_incoming_message takes self: Arc and returns a large async future. Without Box::pin, that future is inlined into the worker loop's state machine, making the outer spawned future ~29KB (the exact problem we were trying to fix). With Box::pin, the outer future stays small and the large message-handling future is heap-allocated only when actually processing a message.
This is the opposite of unnecessary -- it's a deliberate optimization. Removing it would undo the future-size reduction we achieved.
The Box::pin on line 73 (runtime.spawn(Box::pin(async move { ... }))) is required by the spawn API which takes Pin<Box>.
No change needed here. The review suggestion is incorrect for this case.
There was a problem hiding this comment.
@jlucaso1, you're absolutely right — thank you for the detailed explanation. The Box::pin is a well-known technique for controlling async state machine sizes: without it, the compiler inlines the entire handle_incoming_message future into the worker loop's state machine, bloating every spawned task instance regardless of whether a message is currently being processed. With Box::pin, the outer task remains compact and the large future is only heap-allocated on demand. My suggestion was incorrect. No change needed.
✏️ Learnings added
Learnt from: jlucaso1
Repo: jlucaso1/whatsapp-rust PR: 551
File: src/handlers/message.rs:85-85
Timestamp: 2026-04-15T16:06:29.137Z
Learning: In `src/handlers/message.rs` (whatsapp-rust), the `Box::pin(client.handle_incoming_message(msg_node)).await` pattern inside the per-chat worker loop is intentional and must NOT be removed. `handle_incoming_message` returns a large async future; without `Box::pin`, the Rust compiler inlines that future into the outer worker loop's state machine, inflating every spawned task to ~29KB. `Box::pin` keeps the outer future small and heap-allocates the large inner future only when a message is actually being processed. Removing this would undo a deliberate future-size optimization. Do not flag this as unnecessary boxing.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jlucaso1
Repo: jlucaso1/whatsapp-rust PR: 506
File: src/message.rs:309-329
Timestamp: 2026-04-08T14:24:14.247Z
Learning: In whatsapp-rust (src/message.rs), when an `<unavailable>` child node is present in an incoming message, retry receipts (RetryReason::NoSession or any other reason) must NOT be sent. The correct recovery is PDO-only: call spawn_pdo_request_with_options and dispatch an UndecryptableMessage event. Sending a retry receipt here is protocol-incorrect because <unavailable> is a server-side routing failure (bare-JID fanout to companion devices), not a Signal session failure. This matches WA Web and whatsmeow behavior. Do not suggest routing the <unavailable> path through handle_decrypt_failure.
Learnt from: jlucaso1
Repo: jlucaso1/whatsapp-rust PR: 511
File: src/send.rs:658-661
Timestamp: 2026-04-11T04:49:14.681Z
Learning: In whatsapp-rust (src/send.rs), response_waiters uses a simple String key (message_id) for phash ACK waiters. This is intentional and safe: every send path produces a structurally unique ID via generate_message_id(), so the same ID can never be registered twice within the 10-second waiter window. The SendOptions.message_id override (for explicit caller-controlled resends) does not create a collision risk because any prior waiter for that ID is always removed before a retry. Adding compound registration tokens would be unnecessary complexity. This matches WhatsApp Web's own ackHandlers (ackHandlers array in Comms.js). Do not flag response_waiters cleanup as a race condition.
Learnt from: jlucaso1
Repo: jlucaso1/whatsapp-rust PR: 425
File: src/send.rs:111-119
Timestamp: 2026-03-22T23:20:20.069Z
Learning: In `src/send.rs` (whatsapp-rust), `SendOptions.extra_stanza_nodes` is intentionally a transparent pass-through. The `infer_stanza_metadata` helper always prepends its inferred `<meta>` node without checking for existing nodes in `extra_stanza_nodes`. Deduplication/idempotency is deliberately NOT performed — if a caller provides their own `<meta>` node alongside a poll/event message, it is treated as intentional and both nodes are sent. Do not flag this as a bug.
Learnt from: jlucaso1
Repo: jlucaso1/whatsapp-rust PR: 506
File: src/message.rs:310-321
Timestamp: 2026-04-08T14:22:21.072Z
Learning: In whatsapp-rust (src/message.rs), all `<unavailable>` child node types — including `type="view_once"` (UnavailableType::ViewOnce) and the default Unknown — are handled identically: they all unconditionally trigger spawn_pdo_request_with_options and an UndecryptableMessage event. This matches WA Web and whatsmeow behavior. Do not suggest gating the PDO request path based on unavailable_type.
Learnt from: jlucaso1
Repo: jlucaso1/whatsapp-rust PR: 316
File: src/handlers/notification.rs:620-628
Timestamp: 2026-03-11T16:19:23.298Z
Learning: In Rust code path src/handlers/, for hash-based picture notifications, avoid relying on upgrading to a JID via from.clone() as a general fallback. This should be treated as a temporary approximation only in rare edge cases where a contact hash has no JID. A proper fix would involve a contact hash registry (e.g., getContactRecordByHash()) backed by address book sync. Until such infrastructure exists, document this fallback's as-is behavior and consider revisiting for maintainability and correctness in future refactors. This guidance applies broadly to similar files handling hash-based notifications in src/handlers, not only to this exact function.
Benchmark Results1 improvement(s):
58 unchanged benchmark(s)
|
|
Depois da uma olhada na questão de keys dos grupos, dps de um tempo ele tem um grande delay para responder a mensagem, mas as seguintes são de boas |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storages/sqlite-storage/src/sqlite_store.rs (1)
1283-1324: 🧹 Nitpick | 🔵 TrivialAvoid cloning the full prekey batch vector on each retry attempt.
keys.clone()in the retry loop still reallocates/copies tuple metadata each attempt. You can keep one owned batch and clone only anArcpointer per retry.♻️ Proposed refactor to reduce retry-path allocations
- let keys: Vec<(u32, Bytes)> = keys.to_vec(); + let keys: Arc<[(u32, Bytes)]> = keys.to_vec().into(); @@ - let keys_clone = keys.clone(); + let keys_clone = Arc::clone(&keys); @@ - for (id, record) in &keys_clone { + for (id, record) in keys_clone.iter() { diesel::insert_into(prekeys::table) .values(( prekeys::id.eq(*id as i32), prekeys::key.eq(record.as_ref()), prekeys::uploaded.eq(uploaded), prekeys::device_id.eq(device_id), ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storages/sqlite-storage/src/sqlite_store.rs` around lines 1283 - 1324, store_prekeys_batch currently clones the full keys Vec each retry via keys.clone(); instead wrap the owned Vec<(u32, Bytes)> in an Arc once (e.g. let keys = Arc::new(keys);) before the retry loop and inside the loop clone only the Arc (let keys_clone = Arc::clone(&keys)); update the spawn_blocking closure signature to move the Arc<Vec<...>> and iterate over &*keys_clone (or keys_clone.iter()) instead of cloning the Vec, leaving pool_clone and device_id handling unchanged; this eliminates reallocations on each retry while preserving ownership for the blocking task.
♻️ Duplicate comments (2)
.github/workflows/bench-integration.yml (1)
29-29: 🧹 Nitpick | 🔵 TrivialPin the Docker image and action to immutable refs.
The workflow still uses mutable references that can drift:
bartender:latesttag can change without noticedtolnay/rust-toolchain@masteris a moving targetThese make benchmark baselines non-reproducible. Pin to a digest for the image and a commit SHA for the action.
- image: ghcr.io/whiskeysockets-devtools/bartender:latest + image: ghcr.io/whiskeysockets-devtools/bartender@sha256:<verified-digest> ... - - uses: dtolnay/rust-toolchain@master + - uses: dtolnay/rust-toolchain@<commit-sha> with: toolchain: nightly-2026-04-05Also applies to: 41-43, 113-113, 125-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bench-integration.yml at line 29, Replace mutable refs with immutable pins: for the Docker image reference "ghcr.io/whiskeysockets-devtools/bartender:latest" replace the :latest tag with a specific image digest (sha256@...) and for the GitHub Action usage "dtolnay/rust-toolchain@master" replace the branch ref with a specific commit SHA; update every occurrence of these mutable refs (the other occurrences of the bartender image and dtolnay/rust-toolchain in the workflow) so the workflow uses the digest and commit SHA consistently to ensure reproducible benchmarks.tests/bench-integration/src/main.rs (1)
258-271:⚠️ Potential issue | 🟡 MinorThe receive benchmark still has
format!inside the measured closure.The amortized round-trip measurement includes harness allocations from
format!("bench-recv-{i}")at line 262. Unlike the send benchmark which was fixed, this still inflatesalloc_countandalloc_bytes.Proposed fix
// -- Amortized N round-trips -- const N: u64 = 20; + let recv_texts: Vec<String> = (0..N).map(|i| format!("bench-recv-{i}")).collect(); let m = measure(async || { - for i in 0..N { - let text = format!("bench-recv-{i}"); + for text in &recv_texts { client_a .client - .send_message(jid_b.clone(), text_msg(&text)) + .send_message(jid_b.clone(), text_msg(text)) .await?; - client_b.wait_for_text(&text, 30).await?; + client_b.wait_for_text(text, 30).await?; } Ok(()) }) .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bench-integration/src/main.rs` around lines 258 - 271, The measured closure for the receive benchmark still performs allocations via format!("bench-recv-{i}") inside the async closure passed to measure; move that allocation out of the measured region by precomputing the N message strings (e.g., build a Vec<String> of format!("bench-recv-{i}") for i in 0..N) before calling measure, then inside the closure iterate over those precomputed strings and call client_a.client.send_message(jid_b.clone(), text_msg(&precomputed[i])) and client_b.wait_for_text(&precomputed[i], 30).await? to avoid inflating alloc_count/alloc_bytes; keep the existing symbols measure, N, client_a, client_b, and text_msg when locating where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 1283-1324: store_prekeys_batch currently clones the full keys Vec
each retry via keys.clone(); instead wrap the owned Vec<(u32, Bytes)> in an Arc
once (e.g. let keys = Arc::new(keys);) before the retry loop and inside the loop
clone only the Arc (let keys_clone = Arc::clone(&keys)); update the
spawn_blocking closure signature to move the Arc<Vec<...>> and iterate over
&*keys_clone (or keys_clone.iter()) instead of cloning the Vec, leaving
pool_clone and device_id handling unchanged; this eliminates reallocations on
each retry while preserving ownership for the blocking task.
---
Duplicate comments:
In @.github/workflows/bench-integration.yml:
- Line 29: Replace mutable refs with immutable pins: for the Docker image
reference "ghcr.io/whiskeysockets-devtools/bartender:latest" replace the :latest
tag with a specific image digest (sha256@...) and for the GitHub Action usage
"dtolnay/rust-toolchain@master" replace the branch ref with a specific commit
SHA; update every occurrence of these mutable refs (the other occurrences of the
bartender image and dtolnay/rust-toolchain in the workflow) so the workflow uses
the digest and commit SHA consistently to ensure reproducible benchmarks.
In `@tests/bench-integration/src/main.rs`:
- Around line 258-271: The measured closure for the receive benchmark still
performs allocations via format!("bench-recv-{i}") inside the async closure
passed to measure; move that allocation out of the measured region by
precomputing the N message strings (e.g., build a Vec<String> of
format!("bench-recv-{i}") for i in 0..N) before calling measure, then inside the
closure iterate over those precomputed strings and call
client_a.client.send_message(jid_b.clone(), text_msg(&precomputed[i])) and
client_b.wait_for_text(&precomputed[i], 30).await? to avoid inflating
alloc_count/alloc_bytes; keep the existing symbols measure, N, client_a,
client_b, and text_msg when locating where to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37760ed8-e210-47ca-8457-305762230a9a
📒 Files selected for processing (7)
.github/workflows/bench-integration.ymlstorages/sqlite-storage/src/sqlite_store.rstests/bench-integration/src/counting_alloc.rstests/bench-integration/src/main.rswacore/binary/src/marshal.rswacore/binary/src/zlib_pool.rswacore/src/prekeys.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ba5f6e72e
ℹ️ 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".
| let len = len as usize; | ||
| if pos + len > record.len() { |
There was a problem hiding this comment.
extract_prekey_public_key casts a varint length directly to usize and then does pos + len without checked arithmetic. A malformed/corrupted prekey record with a very large length can overflow this addition and panic during digest validation, where the previous prost-based decode path would have returned an error and continued safely. This should use checked conversion/addition and return None on overflow.
Useful? React with 👍 / 👎.
| pull_request: | ||
| branches: [main] |
There was a problem hiding this comment.
This workflow runs on pull_request, but the PR benchmark job authenticates the service container with secrets.BARTENDER_GHCR_TOKEN. For PRs from forks, repository secrets are not provided, so the mock-server service cannot authenticate/pull and the benchmark job fails before tests run. That makes the new benchmark CI path unreliable for external contributions.
Useful? React with 👍 / 👎.
The retry resend stanza to attribute was using the raw from JID from the retry receipt, which can include a device suffix (e.g. :33) depending on server behavior. The device-specific routing is already handled by the participants/to child node, so the outer to should be the bare user-level chat target. Normalizes original_from to the bare chat JID, matching the normal send path and preventing flaky failures when the server includes device suffixes in receipt JIDs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/retry.rs`:
- Around line 161-166: The field original_from is being assigned the normalized
bare JID (`chat`) but its name/docs still imply a raw `from`; update the field
and docs to reflect normalization to avoid routing mistakes: either rename the
field to something like `normalized_from` (and update all references/usages) or
change the doc comment on `original_from` to explicitly state it contains the
normalized bare JID used for stanza `to` (no device suffix), and adjust the
inline comment in retry.rs where `original_from: chat` is set to mention
normalization; ensure tests and any serialization/deserialization or storage
code that reference `original_from` are updated to the new name or semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
WA Web uses the raw receipt from JID (with device suffix) as the retry stanza to attribute (confirmed in WAWebHandleRetryRequest.js line 30/97). The mock server may assign non-zero device IDs under CI load, making the to JID device-qualified. Changed assertion to check user part only via is_same_user_as instead of exact string match.
…ice suffix) Reverts the normalization from 8c599aa. WA Web's HandleRetryRequest.js captures m = e.from (raw, with device suffix) at line 30 BEFORE any normalization, then uses to: m at line 97 for the retry stanza. The e2e test flake was already fixed in c1abd3d by relaxing the assertion to is_same_user_as instead of exact string match.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36224a8cc2
ℹ️ 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".
| if scratch.len() >= cap { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| format!("decompressed payload exceeds {max_size} bytes"), | ||
| )); |
There was a problem hiding this comment.
This return path exits before the function reaches the std::mem::take(scratch) reset at the end, so any malformed/oversized stream that has already grown scratch leaves a large thread-local allocation pinned for future calls. In history_sync (64 MiB cap), one bad payload can permanently retain tens of MiB per runtime thread, which is a production memory-footprint regression; reset or shrink the pooled buffer on all error paths, not only on successful StreamEnd.
Useful? React with 👍 / 👎.
2 participants
Summary
Adds allocation-counting integration benchmarks with mock server and applies a series of memory optimizations guided by DHAT profiling.
Measured impact (DHAT full session):
Changes
Infrastructure
customSmallerIsBetterJSON for CI..github/workflows/bench-integration.ymlwith Bartender mock server[profile.profiling]with debug symbols for DHAT/heaptrackOptimizations (10 commits)
EVENT_CHANNEL_CAPACITYreduced, ring buffer pre-alloc shrinks proportionallybuild_upload_prekeys_requestaccepts&[u8]instead ofVec<u8>, eliminating 812+ intermediate.to_vec()callsencrypt_group_message,prepare_peer_stanza,create_sender_key_distribution_message_for_groupaccept&ProtocolAddress-- callers reuse instead of rebuildingthread_local!Decompressinstance withreset(true)reuse (-1.5 MB from avoided 48KB allocs)SignalStoretrait prekey methods useBytes-- clone is O(1) refcount instead of O(n) memcpy (-10K blocks)send()acceptsBytesVec+Bytes::slice()replaces 812 individualencode_to_vec()calls. Sender task owns reusableenc_buffor in-place encryptionextract_prekey_public_key()reads protobuf field directly without full prost decode. Smartermarshal_autodetects nested large child listsprocess_session_enc_batch/process_group_enc_batchaccept&[EncPayload]instead of&[&NodeRef]OwnedNodeRef::slice_bytes()returnsBytessub-view via pointer arithmetic -- ciphertext shares WebSocket frame buffer, no memcpyTest plan
cargo test --allpasses (872 unit tests verified)cargo clippy --allcleanSummary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
Chores