refactor: codebase quality audit — safety, tests, DRY, dep hygiene - #404
Conversation
- Handle Mutex poison gracefully in message_processing_semaphore (client.rs, message.rs) using match instead of .unwrap(). Scoped block ensures MutexGuard is dropped before .await points. - Remove redundant #[allow(unused_imports)] in appstate_sync.rs — the test module already imports these types directly. - Remove unused _max_bytes parameter from extract_content_uint() in prekeys.rs — all callers passed 4 but the value was never used.
- Change Profile, ChatActions, MediaReupload from &Arc<Client> to &Client — none of them clone the Arc or spawn tasks, so the indirection was unnecessary. Widens the API (non-Arc callers can now use these features). - Replace commented-out MAC verification block in pair.rs with a structured TODO comment explaining the dependency on adv_secret_key persistence (pair_code.rs:321).
hash.rs (was 0 tests, now 12): - SHA-256 known-answer (NIST "abc" vector) - HMAC-SHA256 known-answer (RFC 4231 Test Case 2) - finalize_sha256_array() correctness - finalize_into() with correct and undersized buffers - output_size() for all variants - Unknown algorithm error handling aes_cbc.rs (was 1 test, now 8): - Basic encrypt/decrypt roundtrip - NIST AES-256-CBC known-answer vector - Empty plaintext (produces one padding block) - Exact block boundary (16 bytes + full padding block) - Decrypt with wrong key returns error - Decrypt with invalid ciphertext length returns error - Large data roundtrip
- Replace verbose .map_err(|e| anyhow::anyhow!(e)) with plain ? operator where the error type already implements std::error::Error (crypto::Error is converted automatically by anyhow) - Standardize Result type signatures in messages.rs to use the anyhow Result<T> alias consistently instead of mixing with Result<T, anyhow::Error>
- Remove itertools from wacore-libsignal — only .find_position() was used, replaced with std .position() (identical behavior) - Move uuid to workspace dependencies (shared by libsignal + e2e-tests) - Move subtle to workspace dependencies
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors many feature handles to borrow Changes
Sequence Diagram(s)sequenceDiagram
participant PairCodeHandler as PairCodeHandler
participant PairCodeUtils as PairCodeUtils
participant Persistence as PersistenceManager
participant IQSender as IQSender
PairCodeHandler->>PairCodeUtils: prepare_key_bundle(pair_code_data)
Note right of PairCodeUtils: returns (bundle, rotated_adv_secret_key)
PairCodeUtils-->>PairCodeHandler: (bundle, rotated_adv_secret_key)
PairCodeHandler->>Persistence: process_command(DeviceCommand::SetAdvSecretKey(rotated_adv_secret_key))
Persistence-->>PairCodeHandler: Ok / Err (ignored)
PairCodeHandler->>IQSender: send companion_finish IQ (with bundle)
IQSender-->>PairCodeHandler: send result
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 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 |
|
Claude finished @jlucaso1's task in 1m 53s —— View job PR Review: Codebase Quality Audit
Overall: This is a clean, well-scoped quality pass. All changes are correct and non-breaking. Approving with a few minor suggestions below. Safety fixes — looks goodMutex poison handling ( pair.rs TODO ( Test coverage — excellentThe 22 new tests in
One minor note: the DRY / consistency — correctupload.rs: The Feature struct messages.rs prekeys.rs Dependency hygiene — clean
One suggestion for a follow-upThe /// Access chat management actions (archive, pin, mute, star).
///
/// Access chat management actions (archive, pin, mute, star).
pub fn chat_actions(&self) -> ChatActions<'_> {The old text was "Requires |
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/iq/prekeys.rs (1)
63-74:⚠️ Potential issue | 🟠 MajorReject malformed integer nodes instead of defaulting to
0.Line 63 currently collapses missing/invalid integer bytes to
0, and Lines 240, 251, and 267 consume that for required key identifiers. This can silently accept malformed digest payloads and produce invalid key IDs.Suggested fix
-fn extract_content_uint(node: Option<&Node>) -> u32 { - node.and_then(|n| match &n.content { - Some(NodeContent::Bytes(b)) => { - let mut buf = [0u8; 4]; - let len = b.len().min(4); - buf[4 - len..].copy_from_slice(&b[..len]); - Some(u32::from_be_bytes(buf)) - } - _ => None, - }) - .unwrap_or(0) +fn extract_content_uint(node: Option<&Node>) -> Result<u32, anyhow::Error> { + let bytes = node + .and_then(|n| match &n.content { + Some(NodeContent::Bytes(b)) => Some(b.as_slice()), + _ => None, + }) + .ok_or_else(|| anyhow!("missing integer bytes"))?; + + if bytes.is_empty() || bytes.len() > 4 { + return Err(anyhow!("invalid integer byte length: {}", bytes.len())); + } + + let mut buf = [0u8; 4]; + buf[4 - bytes.len()..].copy_from_slice(bytes); + Ok(u32::from_be_bytes(buf)) }- let reg_id = extract_content_uint(Some(reg_node)); + let reg_id = extract_content_uint(Some(reg_node))?; ... - extract_content_uint(skey.get_optional_child("id")), + extract_content_uint(skey.get_optional_child("id"))?, ... - let prekey_ids = digest_node + let prekey_ids = digest_node .get_optional_child("list") .and_then(|list| list.children()) .map(|children| { children .iter() .filter(|child| child.tag == "key") - .map(|child| extract_content_uint(Some(child))) - .collect() + .map(|child| extract_content_uint(Some(child))) + .collect::<Result<Vec<_>, _>>() }) + .transpose()? .unwrap_or_default();Also applies to: 240-241, 251-252, 267-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/iq/prekeys.rs` around lines 63 - 74, extract_content_uint currently returns 0 for missing/invalid integer bytes which masks malformed payloads; change its signature to return Option<u32> (or Result<u32, _>) and have it return None (or Err) on any non-Bytes content or wrong-length/malformed bytes instead of unwrap_or(0). Then update all call sites that assume a default 0 (the places that use this function for required key identifiers) to handle the Option/Result by rejecting the message or propagating the error instead of treating 0 as a valid id so malformed integer nodes are rejected early.
🤖 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/client.rs`:
- Around line 1019-1025: The semaphore reset can be bypassed by stale Arc
clones; add an epoch/generation counter paired with
self.message_processing_semaphore (e.g., a AtomicU64 or AtomicUsize field like
message_semaphore_epoch) and bump it whenever you replace the Semaphore in the
reconnect/reset code (the block that currently swaps
self.message_processing_semaphore to a new async_lock::Semaphore). Update permit
acquisition logic (the code in src/message.rs that clones/acquires the
semaphore) to capture the current epoch at acquisition and validate it before
treating the permit as valid (reject or drop permits whose captured epoch
differs from the current epoch), and ensure any permit wrappers include the
epoch so stale permits are detected and refused.
In `@src/features/chat_actions.rs`:
- Around line 451-454: The rustdoc for the chat_actions accessor repeats the
same summary line twice; edit the doc comment above pub fn chat_actions(&self)
-> ChatActions<'_> to remove the duplicate sentence so only a single summary
line describing "Access chat management actions (archive, pin, mute, star)."
remains; ensure the remaining doc uses the existing wording and that
ChatActions<'_> and chat_actions are unchanged.
In `@src/message.rs`:
- Around line 499-502: The semaphore lock handling for
message_processing_semaphore is inconsistent: replace usages that call .expect()
(referenced in the code paths using message_processing_semaphore in the
functions in sessions.rs and client.rs) with the same graceful poison recovery
used in message.rs — i.e., call .lock() and match on Ok(guard) => guard.clone(),
Err(poisoned) => poisoned.into_inner().clone() (or equivalent pattern) so all
sites consistently recover from a poisoned mutex without panicking; apply this
change to the functions that currently use .expect() on
message_processing_semaphore.
In `@wacore/src/pair.rs`:
- Around line 147-151: The TODO referencing persisting adv_secret_key via
DeviceCommand::SetAdvSecretKey is misleading because that variant doesn't exist;
update the comment in pair.rs (near the HMAC verification TODO) to either (a)
state explicitly that a new DeviceCommand::SetAdvSecretKey variant must be
implemented as a prerequisite for enabling HMAC verification, or (b) replace the
reference with a clear description of the required persistence mechanism (e.g.,
"persist rotated adv_secret_key to device storage via a new command/flow") and
mark it as a blocker, and also add a cross-reference to pair_code.rs where the
persistence should be implemented; keep the note that ED25519 account signature
verification remains the primary authentication.
---
Outside diff comments:
In `@wacore/src/iq/prekeys.rs`:
- Around line 63-74: extract_content_uint currently returns 0 for
missing/invalid integer bytes which masks malformed payloads; change its
signature to return Option<u32> (or Result<u32, _>) and have it return None (or
Err) on any non-Bytes content or wrong-length/malformed bytes instead of
unwrap_or(0). Then update all call sites that assume a default 0 (the places
that use this function for required key identifiers) to handle the Option/Result
by rejecting the message or propagating the error instead of treating 0 as a
valid id so malformed integer nodes are rejected early.
🪄 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: 39879c20-26b2-48be-94fa-43e4963c8757
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlsrc/appstate_sync.rssrc/client.rssrc/features/chat_actions.rssrc/features/media_reupload.rssrc/features/profile.rssrc/message.rstests/e2e/Cargo.tomlwacore/libsignal/Cargo.tomlwacore/libsignal/src/crypto/aes_cbc.rswacore/libsignal/src/crypto/hash.rswacore/libsignal/src/protocol/sender_keys.rswacore/src/iq/prekeys.rswacore/src/messages.rswacore/src/pair.rswacore/src/upload.rs
💤 Files with no reviewable changes (1)
- src/appstate_sync.rs
- Fix remaining .expect() on message_processing_semaphore in sessions.rs and client.rs test helper to use match + into_inner(), consistent with the pattern in client.rs:1020 and message.rs:499 - Remove duplicate doc comment on chat_actions() accessor - Clarify pair.rs TODO: DeviceCommand::SetAdvSecretKey does not exist yet — must be implemented as a prerequisite for HMAC verification
Workers could clone the semaphore Arc just before a disconnect swaps it, then acquire permits on the abandoned semaphore. The generation counter (AtomicU64) is bumped on every swap; message handlers compare it before/after cloning to reject stale references. Also simplifies verbose comments across client.rs, message.rs, pair.rs.
- Add DeviceCommand::SetAdvSecretKey([u8; 32]) variant - Persist rotated adv_secret_key in pair_code.rs after key bundle prep - Re-enable the HMAC verification check in do_pair_crypto() that was disabled since the key wasn't being persisted - ED25519 signature verification remains as the primary auth gate; HMAC now provides defense-in-depth
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client.rs (1)
1019-1024:⚠️ Potential issue | 🟠 MajorStale
Arc<Semaphore>clones can still bypass this reset.
src/message.rs:497-507clones the current semaphoreArcbefore awaitingacquire_arc(). Any task that grabs the oldArcjust before Line 1024 can keep acquiring permits on the abandoned semaphore after reconnect, socleanup_connection_state()does not reliably restore the single-permit gate. That leaves stale message-processing work running after teardown has already cleared caches/state, and the same swap pattern is also used insrc/client/sessions.rsLines 41-45.Pair the semaphore with a generation/epoch (for example alongside
connection_generation) and discard permits/work captured from stale generations.Run this read-only check to confirm the swap/acquire pattern across the affected sites:
#!/bin/bash set -euo pipefail echo '--- src/client.rs:1017-1025 ---' sed -n '1017,1025p' src/client.rs echo echo '--- src/client/sessions.rs:40-46 ---' sed -n '40,46p' src/client/sessions.rs echo echo '--- src/message.rs:497-507 ---' sed -n '497,507p' src/message.rsExpected result: the reset sites replace the stored
Arc<async_lock::Semaphore>, whilesrc/message.rsclones thatArcand then awaitsacquire_arc(), which is what lets pre-swap clones outlive the reset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client.rs` around lines 1019 - 1024, The semaphore swap in cleanup_connection_state (the block that replaces the stored Arc<Semaphore>) can be bypassed by tasks that cloned the old Arc before the swap (see the acquire pattern in message.rs where a clone is taken prior to await acquire_arc()), so add a generation/epoch alongside the semaphore (e.g., reuse/extend connection_generation) and store them together (or wrap in a small struct) so that callers capture both the Arc<Semaphore> and the current generation when they clone; after acquiring a permit, the caller must compare the captured generation to the current generation and if it differs immediately drop/release the permit and fail the operation, and apply the same pattern to the sessions swap (the same Arc-swap in sessions.rs) so stale clones cannot continue working against a torn-down connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/pair.rs`:
- Around line 149-150: Replace the hardcoded line-number reference
"src/pair_code.rs:321" with a durable symbol-based reference to the function or
item that performs the persistence step in src/pair_code.rs (i.e., refer to the
concrete persistence function/name used to persist pair code such as the
function that implements the persistence step in pair_code.rs), updating the
comment in pair.rs to mention that symbol instead of a line number.
---
Duplicate comments:
In `@src/client.rs`:
- Around line 1019-1024: The semaphore swap in cleanup_connection_state (the
block that replaces the stored Arc<Semaphore>) can be bypassed by tasks that
cloned the old Arc before the swap (see the acquire pattern in message.rs where
a clone is taken prior to await acquire_arc()), so add a generation/epoch
alongside the semaphore (e.g., reuse/extend connection_generation) and store
them together (or wrap in a small struct) so that callers capture both the
Arc<Semaphore> and the current generation when they clone; after acquiring a
permit, the caller must compare the captured generation to the current
generation and if it differs immediately drop/release the permit and fail the
operation, and apply the same pattern to the sessions swap (the same Arc-swap in
sessions.rs) so stale clones cannot continue working against a torn-down
connection.
🪄 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: 653bfe56-b264-4371-8d83-926ce6fc4316
📒 Files selected for processing (4)
src/client.rssrc/client/sessions.rssrc/features/chat_actions.rswacore/src/pair.rs
The HMAC check fails for QR-paired devices because adv_secret_key is only rotated in the pair-code flow (via SetAdvSecretKey). QR pairing keeps the initial random key from Device::new(), which won't match the server's HMAC. Keep the verification disabled until both pairing paths persist the correct key. The SetAdvSecretKey command and pair-code persistence are retained for when QR pairing key rotation is implemented.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 19s —— View job PR Review: Codebase Quality Audit
Overall: Clean, well-scoped quality pass. All changes are correct and non-breaking. One real issue found (TOCTOU race in generation check), rest is minor. Security fixesSemaphore generation counter ( let generation = self.message_semaphore_generation.load(SeqCst); // T1
let semaphore = match self.message_processing_semaphore.lock() { // T2
Ok(guard) => guard.clone(),
Err(poisoned) => poisoned.into_inner().clone(),
};
if generation != self.message_semaphore_generation.load(SeqCst) { // T3
return;
}
let _global_permit = semaphore.acquire_arc().await; // T4A generation bump can happen between T3 and T4 (the Mutex poison handling (
Test coverage — excellentThe 22 new tests in DRY / consistency — correct
Dependency hygiene — clean
Note: PR description mentions moving Minor notePrevious review flagged a duplicate doc comment on |
The semaphore generation could change during the .acquire_arc().await suspension point. Add a second generation check after acquiring the permit so stale permits from a previous connection are dropped.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/client.rs`:
- Around line 1019-1025: The current code increments
message_semaphore_generation before replacing the Arc inside
message_processing_semaphore, making (new_generation, old_semaphore) visible;
change the order so you first replace the Arc (assign *guard =
Arc::new(async_lock::Semaphore::new(1))) and only after dropping/releasing the
guard call message_semaphore_generation.fetch_add(1, Ordering::SeqCst) so the
generation bump always corresponds to the new semaphore instance and prevents
stale clones from being observed as current.
In `@src/client/sessions.rs`:
- Around line 41-47: The generation bump races with exposing the new 64-permit
semaphore: in sessions.rs the code currently increments
message_semaphore_generation before replacing message_processing_semaphore,
allowing stale Arcs to appear current; to fix, acquire the mutex, replace the
Arc with the new async_lock::Semaphore::new(64) while still holding the guard,
then increment message_semaphore_generation (fetch_add) before releasing the
guard so the swap and bump are atomic from readers' perspective; extract this
sequence into a helper (e.g., swap_message_semaphore or
set_message_semaphore_generation) and call it from both this site and
cleanup_connection_state() to keep behavior consistent.
In `@src/message.rs`:
- Around line 496-522: The current logic may drop live messages when
message_semaphore_generation changes; instead make the sequence robust by
looping to obtain a stable (generation, semaphore) pair: inside a retry loop,
lock message_processing_semaphore, clone the Arc semaphore and read
message_semaphore_generation into a local variable, then release the mutex and
call acquire_arc(), and after acquiring re-read message_semaphore_generation —
if it differs from the saved generation, drop/release the acquired permit and
retry the loop; only proceed when the saved generation equals the current
generation so the acquired permit corresponds to the current semaphore. Ensure
this uses the existing symbols message_semaphore_generation,
message_processing_semaphore, acquire_arc, and the local _global_permit
semantics so you don't leak permits on retries.
🪄 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: 3b6b5a82-9c21-45e1-8a4b-76a9e2e2068d
📒 Files selected for processing (6)
src/client.rssrc/client/sessions.rssrc/message.rssrc/pair_code.rswacore/src/pair.rswacore/src/store/commands.rs
| self.message_semaphore_generation | ||
| .fetch_add(1, std::sync::atomic::Ordering::SeqCst); | ||
| let mut guard = match self.message_processing_semaphore.lock() { | ||
| Ok(g) => g, | ||
| Err(poisoned) => poisoned.into_inner(), | ||
| }; | ||
| *guard = std::sync::Arc::new(async_lock::Semaphore::new(64)); |
There was a problem hiding this comment.
Keep the new generation paired with the new 64-permit semaphore.
This has the same atomicity bug as cleanup_connection_state(): the generation is advanced before the mutex stops exposing the old 1-permit semaphore. That leaves a window where stale Arcs can still look current during the offline-sync → parallel-processing handoff. Swap first, then bump while the guard is still held. I’d also extract this into a small helper so both swap sites stay consistent.
Proposed fix
{
- self.message_semaphore_generation
- .fetch_add(1, std::sync::atomic::Ordering::SeqCst);
let mut guard = match self.message_processing_semaphore.lock() {
Ok(g) => g,
Err(poisoned) => poisoned.into_inner(),
};
*guard = std::sync::Arc::new(async_lock::Semaphore::new(64));
+ self.message_semaphore_generation
+ .fetch_add(1, std::sync::atomic::Ordering::SeqCst);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/sessions.rs` around lines 41 - 47, The generation bump races with
exposing the new 64-permit semaphore: in sessions.rs the code currently
increments message_semaphore_generation before replacing
message_processing_semaphore, allowing stale Arcs to appear current; to fix,
acquire the mutex, replace the Arc with the new async_lock::Semaphore::new(64)
while still holding the guard, then increment message_semaphore_generation
(fetch_add) before releasing the guard so the swap and bump are atomic from
readers' perspective; extract this sequence into a helper (e.g.,
swap_message_semaphore or set_message_semaphore_generation) and call it from
both this site and cleanup_connection_state() to keep behavior consistent.
| // Acquire global processing permit (1 during offline sync, N after). | ||
| // Generation check rejects stale Arc clones from a previous connection. | ||
| let generation = self | ||
| .message_semaphore_generation | ||
| .load(std::sync::atomic::Ordering::SeqCst); | ||
| let semaphore = match self.message_processing_semaphore.lock() { | ||
| Ok(guard) => guard.clone(), | ||
| Err(poisoned) => poisoned.into_inner().clone(), | ||
| }; | ||
| if generation | ||
| != self | ||
| .message_semaphore_generation | ||
| .load(std::sync::atomic::Ordering::SeqCst) | ||
| { | ||
| log::debug!("Stale semaphore generation, skipping message batch"); | ||
| return; | ||
| } | ||
| let _global_permit = semaphore.acquire_arc().await; | ||
| // Post-acquire recheck: generation could have changed during the .await | ||
| if generation | ||
| != self | ||
| .message_semaphore_generation | ||
| .load(std::sync::atomic::Ordering::SeqCst) | ||
| { | ||
| log::debug!("Semaphore generation changed during acquire, dropping stale permit"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Retry generation mismatches instead of dropping the batch.
message_semaphore_generation is bumped before the mutex-protected swap in src/client.rs and src/client/sessions.rs, so a change between Line 498 and Line 501 can still leave semaphore pointing at the current Arc. Returning on Line 510 or Line 520 then silently drops a live message during reconnect/offline-sync races, including the same-connection 1→64 transition in complete_offline_sync. This path should retry until it gets a stable (generation, semaphore) pair, and reacquire if the generation changes while awaiting the permit.
🔁 Suggested fix
- let generation = self
- .message_semaphore_generation
- .load(std::sync::atomic::Ordering::SeqCst);
- let semaphore = match self.message_processing_semaphore.lock() {
- Ok(guard) => guard.clone(),
- Err(poisoned) => poisoned.into_inner().clone(),
- };
- if generation
- != self
- .message_semaphore_generation
- .load(std::sync::atomic::Ordering::SeqCst)
- {
- log::debug!("Stale semaphore generation, skipping message batch");
- return;
- }
- let _global_permit = semaphore.acquire_arc().await;
- // Post-acquire recheck: generation could have changed during the .await
- if generation
- != self
- .message_semaphore_generation
- .load(std::sync::atomic::Ordering::SeqCst)
- {
- log::debug!("Semaphore generation changed during acquire, dropping stale permit");
- return;
- }
+ let _global_permit = loop {
+ let generation = self
+ .message_semaphore_generation
+ .load(std::sync::atomic::Ordering::SeqCst);
+ let semaphore = match self.message_processing_semaphore.lock() {
+ Ok(guard) => guard.clone(),
+ Err(poisoned) => poisoned.into_inner().clone(),
+ };
+
+ if generation
+ != self
+ .message_semaphore_generation
+ .load(std::sync::atomic::Ordering::SeqCst)
+ {
+ log::debug!("Semaphore generation changed while cloning, retrying");
+ continue;
+ }
+
+ let permit = semaphore.acquire_arc().await;
+
+ if generation
+ == self
+ .message_semaphore_generation
+ .load(std::sync::atomic::Ordering::SeqCst)
+ {
+ break permit;
+ }
+
+ log::debug!("Semaphore generation changed during acquire, retrying");
+ };📝 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.
| // Acquire global processing permit (1 during offline sync, N after). | |
| // Generation check rejects stale Arc clones from a previous connection. | |
| let generation = self | |
| .message_semaphore_generation | |
| .load(std::sync::atomic::Ordering::SeqCst); | |
| let semaphore = match self.message_processing_semaphore.lock() { | |
| Ok(guard) => guard.clone(), | |
| Err(poisoned) => poisoned.into_inner().clone(), | |
| }; | |
| if generation | |
| != self | |
| .message_semaphore_generation | |
| .load(std::sync::atomic::Ordering::SeqCst) | |
| { | |
| log::debug!("Stale semaphore generation, skipping message batch"); | |
| return; | |
| } | |
| let _global_permit = semaphore.acquire_arc().await; | |
| // Post-acquire recheck: generation could have changed during the .await | |
| if generation | |
| != self | |
| .message_semaphore_generation | |
| .load(std::sync::atomic::Ordering::SeqCst) | |
| { | |
| log::debug!("Semaphore generation changed during acquire, dropping stale permit"); | |
| return; | |
| } | |
| // Acquire global processing permit (1 during offline sync, N after). | |
| // Generation check rejects stale Arc clones from a previous connection. | |
| let _global_permit = loop { | |
| let generation = self | |
| .message_semaphore_generation | |
| .load(std::sync::atomic::Ordering::SeqCst); | |
| let semaphore = match self.message_processing_semaphore.lock() { | |
| Ok(guard) => guard.clone(), | |
| Err(poisoned) => poisoned.into_inner().clone(), | |
| }; | |
| if generation | |
| != self | |
| .message_semaphore_generation | |
| .load(std::sync::atomic::Ordering::SeqCst) | |
| { | |
| log::debug!("Semaphore generation changed while cloning, retrying"); | |
| continue; | |
| } | |
| let permit = semaphore.acquire_arc().await; | |
| if generation | |
| == self | |
| .message_semaphore_generation | |
| .load(std::sync::atomic::Ordering::SeqCst) | |
| { | |
| break permit; | |
| } | |
| log::debug!("Semaphore generation changed during acquire, retrying"); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 496 - 522, The current logic may drop live
messages when message_semaphore_generation changes; instead make the sequence
robust by looping to obtain a stable (generation, semaphore) pair: inside a
retry loop, lock message_processing_semaphore, clone the Arc semaphore and read
message_semaphore_generation into a local variable, then release the mutex and
call acquire_arc(), and after acquiring re-read message_semaphore_generation —
if it differs from the saved generation, drop/release the acquired permit and
retry the loop; only proceed when the saved generation equals the current
generation so the acquired permit corresponds to the current semaphore. Ensure
this uses the existing symbols message_semaphore_generation,
message_processing_semaphore, acquire_arc, and the local _global_permit
semantics so you don't leak permits on retries.
The generation bump and Arc swap must happen under the same mutex hold so readers always see a consistent (generation, semaphore) pair. Writer (swap_message_semaphore helper): 1. Lock mutex 2. Replace Arc 3. Bump generation 4. Unlock Both client.rs (disconnect) and sessions.rs (offline sync) now use the same helper. Reader (message.rs): Read generation inside the mutex (not before it) so the (generation, Arc) pair is always consistent. Removed the redundant pre-acquire check — it was always true with correct ordering. Only the post-acquire check remains for races during .await.
1 participant
Summary
Comprehensive quality pass from a full codebase audit. All changes are non-breaking.
Security
AtomicU64generation tomessage_processing_semaphore. Bumped on every swap (disconnect + offline sync); message handlers check generation before AND after acquiring permits, closing the TOCTOU window during.await.matchwithinto_inner()instead of.unwrap()/.expect().DeviceCommand::SetAdvSecretKey— persist rotatedadv_secret_keyin pair-code flow. HMAC verification remains disabled until QR pairing also rotates the key (documented in TODO).Test coverage (+22 new tests)
hash.rs: 16 tests (was 0) — SHA-256 NIST vector, HMAC-SHA256 RFC 4231 vector,finalize_intobounds, error pathsaes_cbc.rs: 6 new tests (was 1) — NIST AES-256-CBC vector, roundtrip, empty/boundary, wrong key, bad ciphertextDRY / consistency
.map_err(|e| anyhow!(e))→?in upload.rs (crypto errors auto-convert)Result<T>alias in messages.rs&Arc<Client>→&Clientin Profile, ChatActions, MediaReupload (none clone the Arc)_max_bytesparam, redundant imports, duplicate doc commentsDependency hygiene
itertoolsfrom wacore-libsignal (replaced.find_position()with std.position())uuidto workspace dependenciesTest plan
cargo clippy --all --tests— zero warningscargo test --workspace --exclude e2e-tests— all tests pass--no-default-featuresSummary by CodeRabbit
Bug Fixes
Tests
Chores