fix: address 9 audit findings across correctness, safety, and performance - #460
Conversation
…ance
HIGH:
1. Sender key check now reads through signal_cache with a read lock
instead of bypassing cache and taking a write lock (src/send.rs)
2. Dispatch UndecryptableMessage event for group NoSenderKeyState
decrypt failures — matches the session-based path (src/message.rs)
3. Remove double cleanup_connection_state call on transport disconnect
— the call in run() already covers it (src/client.rs)
MEDIUM:
4. Document TOCTOU window in increment_retry_count — mitigated by
per-chat mailbox serialization (src/message.rs)
5. Signal cache flush uses snapshot-then-release pattern — locks are
held only during serialization, not during I/O (signal_cache.rs)
6. Reduce per-chat message queue capacity from 10000 to 500 to limit
memory amplification (src/handlers/message.rs)
LOW:
7. PortableCache::remove_key uses lazy deletion (O(1)) instead of
O(n) VecDeque scan (src/portable_cache.rs)
8. Guard now_millis() casts with .max(0) before as u64 to prevent
silent wrap on negative clock values (keepalive.rs, client.rs)
9. Replace fragile .contains(".us") heuristic with proper JID type
methods is_group()/is_broadcast_list()/is_status_broadcast()
|
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)
📝 WalkthroughWalkthroughReworked disconnect handling to centralize cleanup and Disconnected dispatch, clamped millisecond timestamps before casting, reduced per-chat message queue capacity, improved JID sender classification and undecryptable handling, switched sender-key existence checks to use the signal cache, and refactored signal_cache flush to per-store lock-scoped flushing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,200,255,0.5)
participant Client
participant ReadLoop as ReadLoop/Transport
participant Cleanup as CleanupState
participant Dispatcher as EventDispatcher
end
Client->>ReadLoop: spawn read_messages_loop()
ReadLoop-->>Client: TransportEvent::Data / DataReceived
Client->>Client: update last_data_received_ms (now_millis().max(0) as u64)
ReadLoop->>Client: TransportEvent::Disconnected or Err
Client->>Client: compute expected_disconnect / unexpected_disconnect
Client->>Cleanup: await cleanup_connection_state()
alt unexpected_disconnect
Client->>Dispatcher: dispatch Event::Disconnected
else expected/graceful
note right of Client: skip immediate Disconnected dispatch
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: b1acfcba80
ℹ️ 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 dirty_keys: Vec<_> = state.dirty.drain().collect(); | ||
| let deleted_keys: Vec<_> = state.deleted.drain().collect(); |
There was a problem hiding this comment.
flush() now drains dirty/deleted before performing backend I/O, so any mid-flush error (for example a transient SQLite write failure) returns with those markers already cleared and unchanged entries will not be retried on later flushes. That can silently drop pending session/identity/sender-key persistence and lose crypto state after process restart; the same drain-before-write pattern is repeated for all three stores in this function.
Useful? React with 👍 / 👎.
| // Lazy deletion: remove from map but leave stale key in insertion_order. | ||
| // Stale keys are skipped during FIFO eviction (map.remove returns None). | ||
| // run_pending_tasks() periodically compacts insertion_order. | ||
| self.map.remove(key) |
There was a problem hiding this comment.
This lazy deletion change leaves insertion_order entries behind on every remove/invalidate, but compaction only happens in run_pending_tasks() and is not part of normal cache operations. In moka-cache-disabled builds, workloads that frequently invalidate and reinsert keys can grow insertion_order indefinitely while map remains small, causing avoidable long-term memory and eviction overhead.
Useful? React with 👍 / 👎.
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.rs (1)
1311-1318:⚠️ Potential issue | 🟠 MajorEmit
Disconnectedonly after connection state is cleared.Cleanup now happens later in
run()at Line 894, so handlers triggered byEvent::Disconnectedcan still observeis_connected == trueand even getClientError::AlreadyConnectedif they try to reconnect immediately. Clear the connection-visible state before dispatching here, or move the event emission to the post-cleanup path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client.rs` around lines 1311 - 1318, The Event::Disconnected is dispatched while connection-visible state still reports connected, causing handlers to see is_connected == true and possibly raise ClientError::AlreadyConnected; modify the logic in the disconnect branch (the block using self.expected_disconnect.load, the self.core.event_bus.dispatch(&Event::Disconnected(...)) call) to clear the connection state flag(s) that back is_connected (the same state mutated in run()) before emitting the event, or alternatively move the dispatch into the post-cleanup path in run() so that handlers only see disconnected state; ensure the change references the same symbols (self.expected_disconnect, self.core.event_bus.dispatch, Event::Disconnected, run(), is_connected) so handlers observe the cleared state when they run.
🤖 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 135-137: The TOCTOU risk arises because spawn_retry_receipt()
detaches before calling increment_retry_count, allowing two failure paths for
the same cache_key to race through get()+insert(); move the retry-count
increment into the serialized path or protect it with the per-key lock used for
incoming processing: acquire the same message_enqueue_locks key inside
handle_incoming_message (or before detaching) and perform increment_retry_count
while holding that lock (or alternatively wrap increment_retry_count itself with
the per-key lock), ensuring MessageHandler's serialized processing
(handle_incoming_message) covers the increment for the {chat}:{msg_id}:{sender}
cache_key and prevents double-send retries/PDO.
In `@wacore/src/store/signal_cache.rs`:
- Around line 299-305: The flush() implementation snapshots and drains
dirty/deleted sets before performing backend writes, which can permanently stale
the backend if concurrent flushes or write failures occur; change flush()
(wacore::store::signal_cache::flush) to serialize all flush operations with a
dedicated mutex (e.g., a flush_mutex) so only one flush runs at a time, perform
the backend write using the snapshot while retaining dirty/deleted state, and
only clear or reconcile the dirty and deleted sets after the backend write
succeeds; ensure that on write failure the dirty/deleted markers remain
untouched (or are merged back) so retries can reattempt, and update any code
paths that currently drain the sets prior to calling backend methods on the
SignalStore to instead clear them post-success.
---
Outside diff comments:
In `@src/client.rs`:
- Around line 1311-1318: The Event::Disconnected is dispatched while
connection-visible state still reports connected, causing handlers to see
is_connected == true and possibly raise ClientError::AlreadyConnected; modify
the logic in the disconnect branch (the block using
self.expected_disconnect.load, the
self.core.event_bus.dispatch(&Event::Disconnected(...)) call) to clear the
connection state flag(s) that back is_connected (the same state mutated in
run()) before emitting the event, or alternatively move the dispatch into the
post-cleanup path in run() so that handlers only see disconnected state; ensure
the change references the same symbols (self.expected_disconnect,
self.core.event_bus.dispatch, Event::Disconnected, run(), is_connected) so
handlers observe the cleared state when they run.
🪄 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: 5b79e5f2-813c-4626-a414-91c65241db70
📒 Files selected for processing (7)
src/client.rssrc/handlers/message.rssrc/message.rssrc/portable_cache.rssrc/send.rswacore/src/protocol/keepalive.rswacore/src/store/signal_cache.rs
| /// Note: get-then-insert has a theoretical TOCTOU window, but messages are | ||
| /// processed sequentially per-chat (mailbox pattern in MessageHandler), so | ||
| /// concurrent increments for the same cache_key are practically impossible. |
There was a problem hiding this comment.
The TOCTOU window is still reachable here.
MessageHandler only serializes handle_incoming_message. spawn_retry_receipt() detaches before calling increment_retry_count, so two failure paths for the same {chat}:{msg_id}:{sender} can still race through get()+insert() and double-send retries/PDO. Move the increment into the serialized path or guard it with its own per-key lock.
Based on learnings: Use message_enqueue_locks to serialize per-chat incoming message processing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/message.rs` around lines 135 - 137, The TOCTOU risk arises because
spawn_retry_receipt() detaches before calling increment_retry_count, allowing
two failure paths for the same cache_key to race through get()+insert(); move
the retry-count increment into the serialized path or protect it with the
per-key lock used for incoming processing: acquire the same
message_enqueue_locks key inside handle_incoming_message (or before detaching)
and perform increment_retry_count while holding that lock (or alternatively wrap
increment_retry_count itself with the per-key lock), ensuring MessageHandler's
serialized processing (handle_incoming_message) covers the increment for the
{chat}:{msg_id}:{sender} cache_key and prevents double-send retries/PDO.
| /// Uses a snapshot-then-release pattern: serialize dirty data under the lock, | ||
| /// release locks, then write to the backend. This avoids blocking all | ||
| /// encrypt/decrypt operations for the duration of I/O. | ||
| /// | ||
| /// Dirty sets are drained before the write phase. If a write fails, the | ||
| /// data remains in the cache and will be re-dirtied on the next modification. | ||
| pub async fn flush(&self, backend: &dyn SignalStore) -> Result<()> { |
There was a problem hiding this comment.
Snapshot-then-drain can permanently stale the backend.
These dirty/deleted sets are cleared before any backend write and flush() itself is not serialized. A transient write error—or flush A(old snapshot) -> mutate -> flush B(new snapshot) -> B writes -> A writes—can therefore leave sessions/identities/sender keys persisted at the old value with no dirty markers left to retry. Keep a single flush mutex and only clear/reconcile dirty state after the write phase succeeds.
Also applies to: 308-384
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/store/signal_cache.rs` around lines 299 - 305, The flush()
implementation snapshots and drains dirty/deleted sets before performing backend
writes, which can permanently stale the backend if concurrent flushes or write
failures occur; change flush() (wacore::store::signal_cache::flush) to serialize
all flush operations with a dedicated mutex (e.g., a flush_mutex) so only one
flush runs at a time, perform the backend write using the snapshot while
retaining dirty/deleted state, and only clear or reconcile the dirty and deleted
sets after the backend write succeeds; ensure that on write failure the
dirty/deleted markers remain untouched (or are merged back) so retries can
reattempt, and update any code paths that currently drain the sets prior to
calling backend methods on the SignalStore to instead clear them post-success.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 43s —— View job Code Review: PR #460 — Audit Findings
Overall this is a solid set of fixes. The 3 HIGH correctness items are clearly correct. I have one real concern about the signal cache flush refactor (finding #5), and a few minor notes. Finding #1 — Sender key check bypasses signal cache (
|
| # | Finding | Verdict |
|---|---|---|
| 5 | Signal cache flush drain-before-write | Needs fix — dirty markers lost on write failure |
| 7 | PortableCache lazy deletion | Minor — document unbounded growth on remove/reinsert churn |
The benchmark alert (bench_decrypt_with_previous_session +9.2%) is unrelated to these changes — none of the modified code is in that benchmark's hot path. Likely CI noise.
… TOCTOU comment - Signal cache flush: snapshot dirty sets without draining, clear only after all writes succeed (preserves retry on partial failure) - PortableCache: revert lazy deletion — remove→reinsert broke FIFO eviction order; O(capacity) retain is correct and bounded - increment_retry_count: fix inaccurate comment about per-chat serialization (spawn_retry_receipt detaches)
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 40s —— View job Code Review: PR #460 — Audit Findings (after follow-up commit b950542)
The follow-up commit (b950542) addressed the three main concerns from the first review. Here's a fresh assessment of the full diff. Finding #1 — Sender key check bypasses signal cache (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b950542452
ℹ️ 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".
| for key in &session_dirty_keys { | ||
| state.dirty.remove(key); |
There was a problem hiding this comment.
In SignalStoreCache::flush, phase 3 unconditionally removes every snapshotted key from dirty, but phase 2 performs backend I/O without holding the store locks. If the same key is updated after phase 1 snapshotting and before this cleanup, that newer update does not add a second marker (because dirty is a set), so remove clears the only dirty flag for the newer value. The backend then only has the stale snapshot written earlier, and the later mutation may never be retried, which can silently lose session/identity/sender-key state across restarts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wacore/src/store/signal_cache.rs (1)
305-409:⚠️ Potential issue | 🔴 CriticalThis can still drop newer Signal state during a "successful" flush.
Phase 3 removes every snapshotted key from
dirty/deletedunconditionally. If the same sender key or session is updated again while Phase 2 is doing backend I/O, the backend can end up with the older snapshot while the newer in-memory value is no longer marked dirty. BecauseSignalStoreCache::flush()is still unsynchronized,flush(old)can also raceflush(new)and write stale bytes last. Sincesrc/message.rs::Client::handle_incoming_message()flushes after every message and sender keys ratchet on each group decrypt, this is reachable in normal traffic.Serialize
flush()itself and only clear a key when the current cached value or tombstone still matches the snapshot that was written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/store/signal_cache.rs` around lines 305 - 409, SignalStoreCache::flush currently unconditionally removes all snapshotted keys from sessions/identities/sender_keys dirty and deleted sets after Phase 2, which can drop newer in-memory updates that occurred during the backend I/O; to fix, serialize flush() (i.e., ensure only one flush runs at a time) and, in Phase 3, only clear a key if the current cache entry or tombstone still equals the snapshot that was written — for example, for sessions use the session_dirty_keys/session_delete_keys snapshots taken earlier and, under sessions.lock().await, compare state.cache.get(address) (and state.deleted) against the serialized snapshot or tombstone before calling state.dirty.remove(key) or state.deleted.remove(key); apply the same guarded-compare-and-remove logic for identities (identity_writes/identity_delete_keys) and sender_keys (sender_key_ops/sender_key_dirty_keys).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wacore/src/store/signal_cache.rs`:
- Around line 305-409: SignalStoreCache::flush currently unconditionally removes
all snapshotted keys from sessions/identities/sender_keys dirty and deleted sets
after Phase 2, which can drop newer in-memory updates that occurred during the
backend I/O; to fix, serialize flush() (i.e., ensure only one flush runs at a
time) and, in Phase 3, only clear a key if the current cache entry or tombstone
still equals the snapshot that was written — for example, for sessions use the
session_dirty_keys/session_delete_keys snapshots taken earlier and, under
sessions.lock().await, compare state.cache.get(address) (and state.deleted)
against the serialized snapshot or tombstone before calling
state.dirty.remove(key) or state.deleted.remove(key); apply the same
guarded-compare-and-remove logic for identities
(identity_writes/identity_delete_keys) and sender_keys
(sender_key_ops/sender_key_dirty_keys).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04c042c4-2b56-4932-9bf8-93fbebb52a41
📒 Files selected for processing (2)
src/message.rswacore/src/store/signal_cache.rs
Replace the 3-phase snapshot-then-release pattern with per-store lock-through-IO: each store (sessions, identities, sender_keys) is flushed independently under its own lock. This eliminates the race where a mutation between Phase 1 (snapshot) and Phase 3 (clear) could lose a dirty marker for a newer value, since the lock is now held from snapshot through write through clear. Only one store is locked during its I/O — the other two remain free for concurrent encrypt/decrypt. This is strictly better than the original code (which held all 3 locks simultaneously) and correct unlike the 3-phase approach (which had the dirty-set race).
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 `@wacore/src/store/signal_cache.rs`:
- Around line 306-387: The current flush behavior clears each store's dirty set
after that store's writes succeed, which yields per-store partial-failure
semantics (e.g., sessions cleared even if identities later fail) and differs
from the PR text claiming atomic "clear dirty sets only after all writes
succeed"; update the code/docs to match intent: either (A) change the PR
description to state per-store clearing semantics, or (B) modify flush to only
clear any dirty/deleted sets after all three sections succeed by moving the
removals out of the per-store blocks and performing them after all backend calls
complete; refer to the flush method and the per-store states sessions,
identities, and sender_keys and their state.dirty/state.deleted manipulations
when making 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: 70f3d374-0209-4d3a-bf4c-06d85e3399fb
📒 Files selected for processing (1)
wacore/src/store/signal_cache.rs
| pub async fn flush(&self, backend: &dyn SignalStore) -> Result<()> { | ||
| let mut sessions = self.sessions.lock().await; | ||
| let mut identities = self.identities.lock().await; | ||
| let mut sender_keys = self.sender_keys.lock().await; | ||
|
|
||
| // Snapshot dirty/deleted sets WITHOUT draining — preserve on failure | ||
| let session_dirty: Vec<_> = sessions.dirty.iter().cloned().collect(); | ||
| let session_deleted: Vec<_> = sessions.deleted.iter().cloned().collect(); | ||
| let identity_dirty: Vec<_> = identities.dirty.iter().cloned().collect(); | ||
| let identity_deleted: Vec<_> = identities.deleted.iter().cloned().collect(); | ||
| let sender_key_dirty: Vec<_> = sender_keys.dirty.iter().cloned().collect(); | ||
|
|
||
| // Persist dirty sessions — serialize only here, not on every store_session | ||
| for address in &session_dirty { | ||
| if let Some(Some(record)) = sessions.cache.get(address.as_ref()) { | ||
| let bytes = record | ||
| .serialize() | ||
| .map_err(|e| anyhow::anyhow!("session serialize for {address}: {e}"))?; | ||
| backend.put_session(address, &bytes).await?; | ||
| // Flush sessions | ||
| { | ||
| let mut state = self.sessions.lock().await; | ||
| let dirty_keys: Vec<_> = state.dirty.iter().cloned().collect(); | ||
| let deleted_keys: Vec<_> = state.deleted.iter().cloned().collect(); | ||
|
|
||
| for address in &dirty_keys { | ||
| if let Some(Some(record)) = state.cache.get(address.as_ref()) { | ||
| let bytes = record | ||
| .serialize() | ||
| .map_err(|e| anyhow::anyhow!("session serialize for {address}: {e}"))?; | ||
| backend.put_session(address, &bytes).await?; | ||
| } | ||
| } | ||
| for address in &deleted_keys { | ||
| backend.delete_session(address).await?; | ||
| } | ||
| } | ||
| for address in &session_deleted { | ||
| backend.delete_session(address).await?; | ||
| } | ||
|
|
||
| for address in &identity_dirty { | ||
| if let Some(Some(data)) = identities.cache.get(address.as_ref()) { | ||
| let key: [u8; 32] = data.as_ref().try_into().map_err(|_| { | ||
| anyhow::anyhow!( | ||
| "Corrupted identity key for {address}: expected 32 bytes, got {}", | ||
| data.len() | ||
| ) | ||
| })?; | ||
| backend.put_identity(address, key).await?; | ||
| for key in &dirty_keys { | ||
| state.dirty.remove(key); | ||
| } | ||
| for key in &deleted_keys { | ||
| state.deleted.remove(key); | ||
| } | ||
| } | ||
| for address in &identity_deleted { | ||
| backend.delete_identity(address).await?; | ||
| } | ||
|
|
||
| for name in &sender_key_dirty { | ||
| match sender_keys.cache.get(name.as_ref()) { | ||
| Some(Some(record)) => { | ||
| let bytes = record | ||
| .serialize() | ||
| .map_err(|e| anyhow::anyhow!("sender key serialize for {name}: {e}"))?; | ||
| backend.put_sender_key(name, &bytes).await?; | ||
| } | ||
| Some(None) => { | ||
| // Deleted via delete_sender_key — propagate to backend | ||
| backend.delete_sender_key(name).await?; | ||
| // Flush identities | ||
| { | ||
| let mut state = self.identities.lock().await; | ||
| let dirty_keys: Vec<_> = state.dirty.iter().cloned().collect(); | ||
| let deleted_keys: Vec<_> = state.deleted.iter().cloned().collect(); | ||
|
|
||
| for address in &dirty_keys { | ||
| if let Some(Some(data)) = state.cache.get(address.as_ref()) { | ||
| let key: [u8; 32] = data.as_ref().try_into().map_err(|_| { | ||
| anyhow::anyhow!( | ||
| "Corrupted identity key for {address}: expected 32 bytes, got {}", | ||
| data.len() | ||
| ) | ||
| })?; | ||
| backend.put_identity(address, key).await?; | ||
| } | ||
| None => {} | ||
| } | ||
| for address in &deleted_keys { | ||
| backend.delete_identity(address).await?; | ||
| } | ||
|
|
||
| for key in &dirty_keys { | ||
| state.dirty.remove(key); | ||
| } | ||
| for key in &deleted_keys { | ||
| state.deleted.remove(key); | ||
| } | ||
| } | ||
|
|
||
| // All writes succeeded — clear dirty sets (matches WA Web's clearDirty()) | ||
| sessions.dirty.clear(); | ||
| sessions.deleted.clear(); | ||
| identities.dirty.clear(); | ||
| identities.deleted.clear(); | ||
| sender_keys.dirty.clear(); | ||
| // Flush sender keys | ||
| { | ||
| let mut state = self.sender_keys.lock().await; | ||
| let dirty_keys: Vec<_> = state.dirty.iter().cloned().collect(); | ||
|
|
||
| for name in &dirty_keys { | ||
| match state.cache.get(name.as_ref()) { | ||
| Some(Some(record)) => { | ||
| let bytes = record | ||
| .serialize() | ||
| .map_err(|e| anyhow::anyhow!("sender key serialize for {name}: {e}"))?; | ||
| backend.put_sender_key(name, &bytes).await?; | ||
| } | ||
| Some(None) => { | ||
| backend.delete_sender_key(name).await?; | ||
| } | ||
| None => {} | ||
| } | ||
| } | ||
|
|
||
| for key in &dirty_keys { | ||
| state.dirty.remove(key); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Note partial-failure semantics for future reference.
If, say, sessions flush succeeds but identities flush fails, the sessions dirty set is cleared before the error is returned. On retry, only identities (and sender_keys) will be re-flushed since sessions are already persisted and no longer dirty. This is correct behavior since the session writes did succeed.
This differs slightly from the PR description's "clearing dirty sets only after all writes succeed" (which implies a global all-or-nothing), but per-store clearing is the more practical approach given the independent store design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/store/signal_cache.rs` around lines 306 - 387, The current flush
behavior clears each store's dirty set after that store's writes succeed, which
yields per-store partial-failure semantics (e.g., sessions cleared even if
identities later fail) and differs from the PR text claiming atomic "clear dirty
sets only after all writes succeed"; update the code/docs to match intent:
either (A) change the PR description to state per-store clearing semantics, or
(B) modify flush to only clear any dirty/deleted sets after all three sections
succeed by moving the removals out of the per-store blocks and performing them
after all backend calls complete; refer to the flush method and the per-store
states sessions, identities, and sender_keys and their state.dirty/state.deleted
manipulations when making the change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 782515c36e
ℹ️ 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 !self.expected_disconnect.load(Ordering::Relaxed) { | ||
| self.core.event_bus.dispatch(&Event::Disconnected(crate::types::events::Disconnected)); | ||
| debug!("Transport disconnected unexpectedly."); |
There was a problem hiding this comment.
Event::Disconnected is now dispatched before cleanup_connection_state() runs, so synchronous handlers can observe stale connection state (is_connected, transport/noise handles, caches) and make incorrect decisions (for example, skipping reconnect logic because the client still appears connected during the callback). This regression comes from removing the in-loop cleanup call without preserving the prior cleanup-before-dispatch ordering for unexpected disconnects.
Useful? React with 👍 / 👎.
The removal of cleanup_connection_state() from inside the message loop created a regression: Event::Disconnected was dispatched while is_connected was still true and transport handles still set. Handlers could observe stale state. Move the dispatch to run() after cleanup_connection_state() completes, matching the original ordering where cleanup ran before the event.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 41s —— View job Code Review: PR #460 — Audit Findings (Final State, 4 commits)
This is a clean set of 9 focused fixes. All prior review concerns (flush durability, cache eviction, disconnect ordering) have been addressed across the 4 commits. Here's my assessment of each finding. Finding #1 — Sender key check uses signal cache (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a65d796a9a
ℹ️ 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 self.expected_disconnect.load(Ordering::Relaxed) | ||
| || self.intentional_reconnect.swap(false, Ordering::Relaxed) | ||
| { |
There was a problem hiding this comment.
intentional_reconnect is folded into the “expected” path here, which makes unexpected_disconnect false and skips the later Event::Disconnected dispatch in run(). In practice, calling reconnect() now drops the transport without emitting a disconnect event, so consumers that rely on Disconnected callbacks (e.g., reconnect-state/UI transitions) will miss that lifecycle transition even though the socket was torn down.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes 9 issues found during a codebase audit, covering correctness bugs, safety improvements, and performance optimizations.
HIGH — Correctness bugs
1. Sender key check bypasses signal cache (
src/send.rs)device_guard.load_sender_key()(direct DB read with write lock)signal_cache.get_sender_key()with read lock, matching the status broadcast path2. Missing UndecryptableMessage event for group decrypt failures (
src/message.rs)NoSenderKeyStateerrors spawned retry receipt but never dispatchedUndecryptableMessage3. Double
cleanup_connection_stateon transport disconnect (src/client.rs)cleanup_connection_state()was called both inside the message loop (on Disconnected event) and inrun()after the loop exitsrun()handles itMEDIUM — Safety and performance
4.
increment_retry_countTOCTOU (src/message.rs)spawn_retry_receiptdetaches viaruntime.spawn, so per-chat serialization does not cover this path; in practice retries for the same message are rare and recipients deduplicate by message ID5. Signal cache
flush()per-store lock-through-IO (wacore/src/store/signal_cache.rs)6. Message queue capacity reduced (
src/handlers/message.rs)bounded(10000)→bounded(500)per chatLOW — Defensive improvements
7. PortableCache removal (
src/portable_cache.rs)retain()-based removal — the lazy deletion optimization was reverted because remove→reinsert breaks FIFO eviction order (stale key causes live entry to be evicted)retainis correct and bounded by max_capacity8.
now_millis() as u64guard (keepalive.rs,client.rs).max(0)beforeas u64cast at all 11 sites to prevent silent wrap on negative clock values9. Fragile JID type heuristic (
src/message.rs)server.contains(".us")with properis_group()/is_broadcast_list()/is_status_broadcast()methodsTest plan
cargo clippy --all --tests— zero warningscargo test --all --exclude e2e-tests— all tests passSummary by CodeRabbit
Bug Fixes
Chores