fix: codebase audit with bug fixes, race condition mitigations, and perf improvements - #511
Conversation
B1: create_new_device() now inserts with the configured device_id instead of relying on auto-increment. Fixes first-boot mismatch where new_for_device(N) created row 1 but reads targeted row N. B2: save_device_data_for_device() now uses with_retry() for semaphore serialization and retry on SQLITE_BUSY/locked errors. Previously bypassed this path, allowing 10 transient failures to permanently halt the background saver. Uses Arc<[u8]>/Arc<str> so retry clones are atomic increments, not deep copies. B3: spawn_phash_validation() now removes its waiter from response_waiters on timeout/error. Previously leaked the entry, which suppressed keepalive pings for the rest of the connection. Callers restructured to eliminate unwrap() on phash Option.
request_app_state_keys() now returns Result so callers can detect failure. Both call sites (sync_collections_batched and sync_single_collection) remove the dedup stamps on error, allowing the keys to be retried on the next sync instead of being suppressed for 24 hours.
R1: Increase default capacity for session_locks (2k -> 10k), message_queues and message_enqueue_locks (2k -> 5k) to reduce the risk of capacity eviction creating duplicate locks for the same key. R2: Message queue workers now capture connection_generation at spawn and check it before processing each message. Workers from a previous connection exit immediately instead of processing messages with stale crypto state.
B5: Validate server resume offset before slicing upload data. Prevents panic if the server returns an offset exceeding the data length; falls back to full upload with a warning. B6: Server-sync tasks now re-check connection_generation after the version comparison loop (which awaits DB reads). Prevents stale sync tasks from continuing after a reconnect occurred during the version filtering step.
Replace Vec::new() with Vec::with_capacity() in 13 sites across the codebase where the final size is derivable from input length: - src/usync.rs: device list fetching - src/features/community.rs: link/unlink/participant results - wacore/src/store/signal_cache.rs: eviction candidate lists - wacore/appstate/src/patch_decode.rs: patch decoding - wacore/src/appstate_sync.rs: missing key collection - wacore/src/pair.rs: pairing message buffer (64+32+32 bytes) - wacore/derive/src/lib.rs: derive macro field/variant lists
B8: Replace `let _ = backend.take_sent_message(...)` with an explicit error log. Previously, DB deletion failures during cache-hit cleanup were silently ignored.
📝 WalkthroughWalkthroughIncrease several cache defaults, consolidate and error-propagate app-state key requests, add connection-generation checks to stop stale workers, unify ack/waiter lifecycle with cleanup on error/timeout, validate upload resume offsets, refactor SQLite writes to use Arc-backed binds and centralized retry, and add multiple small preallocation optimizations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponseWaiters
participant Transport as StanzaTransport
participant PhashWorker as PhashValidator
Client->>ResponseWaiters: register waiter (message_id, phash, ack_rx)
Client->>Transport: send_node(stanza with id)
alt send_node succeeded
Transport->>ResponseWaiters: deliver ack node (message_id, phash)
ResponseWaiters->>PhashWorker: notify ack receiver
PhashWorker->>Client: validate phash -> success (remove waiter)
else send_node failed
Client->>ResponseWaiters: remove waiter (message_id)
Client-->>Client: return/send error
end
note over PhashWorker,ResponseWaiters: on timeout PhashWorker removes waiter(message_id) before exiting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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 check also if changes can get some DRY and find possible regressions/bugs |
|
Claude finished @jlucaso1's task in 6m 34s —— View job Review: Bug fixes, DRY opportunities, and regression check
Overall this is a solid audit PR — the fixes are well-motivated and the commit organization is clean. Here are specific findings: Potential Bugs / Regressions1. Upload offset bounds check: if offset > enc.data_to_upload.len() {Should be if offset >= enc.data_to_upload.len() {2. Stale message workers silently drop queued messages — When a worker detects a generation change and if client_for_worker
.connection_generation
.load(std::sync::atomic::Ordering::Acquire)
!= spawn_generation
{
log::debug!(target: "MessageQueue", "Stale worker exiting; remaining messages will be redelivered by server");
break;
}This is minor — not a bug, just an observability gap. 3. Inconsistent atomic ordering between handlers —
Both are correct — DRY Opportunities1. App-state missing-key-request pattern (high value) — These two blocks are ~28 lines of identical code: build /// Request missing app-state keys with dedup stamps.
/// On send failure, removes stamps so keys can be retried next sync.
async fn request_missing_keys_with_dedup(&self, missing: Vec<Vec<u8>>) {
if missing.is_empty() {
return;
}
let mut to_request: Vec<Vec<u8>> = Vec::with_capacity(missing.len());
let mut guard = self.app_state_key_requests.lock().await;
let now = wacore::time::Instant::now();
for key_id in missing {
let hex_id = hex::encode(&key_id);
let should = guard
.get(&hex_id)
.map(|t| t.elapsed() > std::time::Duration::from_secs(24 * 3600))
.unwrap_or(true);
if should {
guard.insert(hex_id, now);
to_request.push(key_id);
}
}
guard.retain(|_, t| t.elapsed() < std::time::Duration::from_secs(24 * 3600));
drop(guard);
if !to_request.is_empty()
&& let Err(e) = self.request_app_state_keys(&to_request).await
{
warn!("Failed to send app state key request: {e}");
let mut guard = self.app_state_key_requests.lock().await;
for key_id in &to_request {
guard.remove(&hex::encode(key_id));
}
}
}Both call sites would collapse to 2. The .on_conflict(device::id)
.do_update()
.set((
device::lid.eq(excluded(device::lid)),
device::pn.eq(excluded(device::pn)),
device::registration_id.eq(excluded(device::registration_id)),
// ... etc
))This eliminates the entire second copy of the column values and removes the risk of the two lists drifting out of sync. Code Quality Notes (non-blocking)
VerdictThe fixes are well-reasoned and correctly target real problems. The two DRY items above (key-request dedup helper and |
![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 (2)
src/handlers/message.rs (1)
69-90:⚠️ Potential issue | 🔴 CriticalWorker exit can strand a closed per-chat sender in cache (message loss risk).
When Line 89 breaks on generation mismatch, the worker drops
rx, but the cachedtxfor thatchat_idcan remain. Later enqueues hit a closed channel and fail, so messages may be dropped repeatedly until eviction.Proposed fix
let client_for_worker = client.clone(); +let chat_id_for_worker = chat_id.clone(); let spawn_generation = client .connection_generation - .load(std::sync::atomic::Ordering::Acquire); + .load(std::sync::atomic::Ordering::SeqCst); // ... if client_for_worker .connection_generation - .load(std::sync::atomic::Ordering::Acquire) + .load(std::sync::atomic::Ordering::SeqCst) != spawn_generation { + // Ensure next enqueue recreates queue+worker for this chat. + client_for_worker + .message_queues + .invalidate(&chat_id_for_worker) + .await; break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/message.rs` around lines 69 - 90, When the spawned worker detects a connection_generation mismatch (compare between spawn_generation and client_for_worker.connection_generation), it currently breaks and drops rx while the cached tx for that chat_id can remain closed; update the worker closure so that immediately before breaking on generation mismatch it explicitly removes/invalidate the cached sender for that chat_id (e.g., call the cache removal/invalidate helper on the client using the cloned chat_id or remove the tx entry) to prevent future enqueues from hitting a closed channel; ensure the worker has a cloned chat_id available and perform the removal atomically relative to any cache helpers provided by the client to avoid races.storages/sqlite-storage/src/sqlite_store.rs (1)
417-479:⚠️ Potential issue | 🟠 MajorSerialize and retry device creation too.
Line 422 still writes through a raw
spawn_blockingpath, so first-boot device creation can still fail withSQLITE_BUSY/LOCKEDwhile another write holds the database. That leaves the bootstrap path less resilient thansave_device_data_for_device, even though both target the same SQLite file.Suggested direction
pub async fn create_new_device(&self) -> Result<i32> { use crate::schema::device; - let pool = self.pool.clone(); let device_id = self.device_id; - tokio::task::spawn_blocking(move || -> Result<i32> { - let mut conn = pool - .get() - .map_err(|e| StoreError::Connection(e.to_string()))?; - - let new_device = wacore::store::Device::new(); + self.with_retry("create_new_device", || { + Box::new(move |conn: &mut SqliteConnection| { + let new_device = wacore::store::Device::new(); - diesel::insert_into(device::table) - .values(( - device::id.eq(device_id), - // ... - )) - .execute(&mut conn) - .map_err(|e| StoreError::Database(e.to_string()))?; + diesel::insert_into(device::table) + .values(( + device::id.eq(device_id), + // ... + )) + .execute(conn)?; - Ok(device_id) - }) - .await - .map_err(|e| StoreError::Database(e.to_string()))? + Ok(device_id) + }) + }) + .await }🤖 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 417 - 479, create_new_device currently does writes inside tokio::task::spawn_blocking which can race with other writers and get SQLITE_BUSY/LOCKED; make it use the same serialized-retry write path as save_device_data_for_device. Replace the direct spawn_blocking call in create_new_device with the existing serialize-and-retry helper used by save_device_data_for_device (or extract that retry logic into a shared function), move the diesel insert closure into that serialized block, and ensure the retry loop detects SQLITE_BUSY/SQLITE_LOCKED and retries with backoff before returning an error; keep the same return value (device_id) and preserve all referenced symbols (new_device, noise_key_data, identity_key_data, signed_pre_key_data) inside the closure.
🤖 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/cache_config.rs`:
- Around line 259-261: Update the struct docs for CacheConfig so the field
comments for session_locks_capacity, message_queues_capacity, and
message_enqueue_locks_capacity reflect the current default values (10000, 5000,
5000) instead of the outdated 2000; locate the comments adjacent to the
CacheConfig definition and replace the old default numbers/text with the new
defaults and, if present, any explanatory text to match the actual
initialization values used in the default implementation.
In `@src/client.rs`:
- Around line 2833-2841: The current request_app_state_keys method returns
Ok(()) when device_snapshot.pn is None (own_jid missing), which incorrectly
signals success and prevents caller retries; change this to return an error
instead of Ok by returning Err(anyhow::anyhow!("no own_jid available for
app-state request")) so callers will retry, or alternatively move any
dedupe-stamp insertion logic out of the caller and into request_app_state_keys
so stamps are only added after obtaining own_jid (use
persistence_manager.get_device_snapshot() and the own_jid match branch to gate
stamp insertion). Ensure the change updates request_app_state_keys and any
caller assumptions accordingly.
In `@src/send.rs`:
- Around line 491-500: The code registers an ack waiter (via
register_ack_waiter) before calling send_node, but if send_node(...) returns Err
the function returns early and never spawns the validator nor removes the
waiter, leaving a stale entry in response_waiters; fix by catching the send_node
result, and on Err remove/unregister the waiter from response_waiters (use the
same removal helper used in src/request.rs or explicitly remove by request_id)
before propagating the error, and apply the same change to the other similar
block (lines ~1123-1132) so spawn_phash_validation is only reached after a
successful send and no dead waiters remain.
---
Outside diff comments:
In `@src/handlers/message.rs`:
- Around line 69-90: When the spawned worker detects a connection_generation
mismatch (compare between spawn_generation and
client_for_worker.connection_generation), it currently breaks and drops rx while
the cached tx for that chat_id can remain closed; update the worker closure so
that immediately before breaking on generation mismatch it explicitly
removes/invalidate the cached sender for that chat_id (e.g., call the cache
removal/invalidate helper on the client using the cloned chat_id or remove the
tx entry) to prevent future enqueues from hitting a closed channel; ensure the
worker has a cloned chat_id available and perform the removal atomically
relative to any cache helpers provided by the client to avoid races.
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 417-479: create_new_device currently does writes inside
tokio::task::spawn_blocking which can race with other writers and get
SQLITE_BUSY/LOCKED; make it use the same serialized-retry write path as
save_device_data_for_device. Replace the direct spawn_blocking call in
create_new_device with the existing serialize-and-retry helper used by
save_device_data_for_device (or extract that retry logic into a shared
function), move the diesel insert closure into that serialized block, and ensure
the retry loop detects SQLITE_BUSY/SQLITE_LOCKED and retries with backoff before
returning an error; keep the same return value (device_id) and preserve all
referenced symbols (new_device, noise_key_data, identity_key_data,
signed_pre_key_data) inside the closure.
🪄 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: 450e9a86-26ce-40fb-950e-8a3422f18bdd
📒 Files selected for processing (15)
src/cache_config.rssrc/client.rssrc/client/sender_keys.rssrc/features/community.rssrc/handlers/message.rssrc/handlers/notification.rssrc/send.rssrc/upload.rssrc/usync.rsstorages/sqlite-storage/src/sqlite_store.rswacore/appstate/src/patch_decode.rswacore/derive/src/lib.rswacore/src/appstate_sync.rswacore/src/pair.rswacore/src/store/signal_cache.rs
![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: 7a5d562bc9
ℹ️ 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".
Benchmark Results59 unchanged benchmark(s)
|
…vice retry, docs - Clean up ack waiters from response_waiters when send_node fails, preventing stale entries that suppress keepalives (both status and DM send paths) - request_app_state_keys now returns Err when own JID is unavailable so callers remove dedup stamps instead of silently suppressing retries - create_new_device now uses with_retry for semaphore serialization and SQLITE_BUSY retry, consistent with save_device_data_for_device - Update CacheConfig doc comments to reflect current defaults (session_locks 10000, queues 5000)
- Upload resume offset: `>` → `>=` so offset==len falls back to full upload instead of uploading zero bytes - Stale message worker: add debug log before break for observability - Atomic ordering: normalize connection_generation loads to Acquire (was inconsistently SeqCst in notification.rs) - Extract `request_missing_keys_with_dedup()` helper to deduplicate ~28 identical lines across sync_collections_batched and sync_single_collection - Use diesel `excluded()` in save_device_data upsert to reference INSERT values instead of duplicating all 20 column assignments
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 2589-2597: Extract the duplicated "dedupe-stamp, send, and
rollback" logic into a shared async helper used by both call sites: create a
function (e.g., send_app_state_key_requests_with_rollback) that accepts (&self,
to_request: Vec<Vec<u8>>) and encapsulates the stamping into
self.app_state_key_requests.lock().await, calling
self.request_app_state_keys(&to_request).await, logging on Err(e) with the same
warn! text, and performing the rollback loop that removes hex::encode(key_id)
from the guard on failure; replace the two duplicated blocks (the one around
request_app_state_keys and the one at lines ~2803-2811) with calls to this new
helper so both paths use identical send-and-rollback behavior.
In `@src/send.rs`:
- Around line 1118-1132: The code currently uses unwrap_or_default() to produce
an empty-string id which can collide in response_waiters and leak; change the
stanza handling to require both phash and id using a let-chain pattern: replace
the current block that reads stanza_to_send.attrs().optional_string("phash") and
then uses .optional_string("id").map(...).unwrap_or_default() with an if let
Some(phash) = stanza_to_send.attrs().optional_string("phash").map(|s|
s.into_owned()) && let Some(msg_id) =
stanza_to_send.attrs().optional_string("id").map(|s| s.into_owned()) { let rx =
self.register_ack_waiter(&msg_id).await; Some((rx, phash, msg_id)) } else { None
}, thereby removing unwrap_or_default and ensuring register_ack_waiter and
response_waiters only use a real id; this also aligns with handle_ack_response
which looks up by id.
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 417-476: Add a regression test that verifies the store uses the
configured device id path rather than a SQLite-generated rowid: use the
constructor new_for_device(..., N) to create a SqliteStore targeting device id
N, call create_new_device() on that store, then fetch the stored device (via the
existing load/get method used elsewhere, e.g., load_device or get_device_by_id)
and assert the returned device.id == N and that its persisted keys/fields match
the created values; this ensures create_new_device (and new_for_device) insert
using self.device_id and prevents falling back to SQLite row IDs.
- Around line 304-415: Both save_device_data_for_device and create_new_device
duplicate CoreDevice serialization and the diesel device::...eq(...) bindings;
extract that logic into a small shared helper (e.g., a DeviceDbModel or
bind_device_values function) that: serializes CoreDevice fields into the
Arc/Option<Arc<...>> forms (noise_key_data, identity_key_data,
signed_pre_key_data, account_data, push_name, new_lid, new_pn, etc.), and
exposes a single insert-values tuple and an identical update-set tuple (or a
method that returns both) that can be used inside the existing with_retry
closure and Box::new move closure; replace the duplicated serialization and the
long values(...).on_conflict(...).do_update().set(...) tuples in
save_device_data_for_device and create_new_device to call this helper instead so
both paths share the same binding/serialization logic.
🪄 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: 79d05353-b98b-48a9-b5e8-77c61b7372bb
📒 Files selected for processing (4)
src/cache_config.rssrc/client.rssrc/send.rsstorages/sqlite-storage/src/sqlite_store.rs
|
@claude review I fixed most of the previous issues, please check for more rooms for improvement or bugs introduced in the changes. |
jlucaso1
changed the title
|
Claude finished @jlucaso1's task in 4m 32s —— View job Review: Post-fix follow-up
All previous review items have been addressed well. The fixes are solid. Here are the remaining findings: Remaining issue (minor)1. let msg_id = stanza_to_send
.attrs()
.optional_string("id")
.map(|s| s.into_owned())
.unwrap_or_default();If a stanza has let ack = if let Some(phash) = stanza_to_send.attrs().optional_string("phash").map(|s| s.into_owned())
&& let Some(msg_id) = stanza_to_send.attrs().optional_string("id").map(|s| s.into_owned())
{
let rx = self.register_ack_waiter(&msg_id).await;
Some((rx, phash, msg_id))
} else {
None
};This also follows the collapsible-if / let-chain convention from the project guidelines. Previous review items — verification
Observations (non-blocking, no action needed)Stale worker tx in cache after generation break — When the message worker at
VerdictThe fix commits addressed all previous review findings correctly. The only actionable item is the |
- Phash ack registration now requires both phash AND id via let-chain instead of using unwrap_or_default(). Prevents empty-string key collisions in response_waiters when stanza has no id attribute. - Add test_create_new_device_uses_configured_device_id that verifies new_for_device(42) creates row 42 (not row 1 via auto-increment) and that the data is loadable by the configured id.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43c3886213
ℹ️ 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".
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/upload.rs`:
- Around line 167-173: The current parse_upload_progress hides invalid server
offsets by mapping offset >= total_size to NotFound, bypassing the warning in
the upload resume check; change parse_upload_progress so it does not convert
offsets >= total_size into NotFound but instead returns the parsed offset (or a
distinct enum variant like InvalidResume) so the existing guard that compares
byte_offset against enc.data_to_upload.len() (the block using offset =
byte_offset as usize and logging "Server resume offset {offset} exceeds data
length") can run and emit the warning; update callers of parse_upload_progress
accordingly to handle the returned offset/variant so the single
validation/logging point in the resume branch (the check around
enc.data_to_upload.len()) is always reached for out-of-range offsets.
🪄 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: 2705ff8b-c28c-4277-aa13-c8c964c97e77
📒 Files selected for processing (5)
src/client.rssrc/handlers/message.rssrc/handlers/notification.rssrc/upload.rsstorages/sqlite-storage/src/sqlite_store.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
storages/sqlite-storage/src/sqlite_store.rs (1)
304-415: 🛠️ Refactor suggestion | 🟠 MajorExtract the
devicerow serialization/binding into one helper.
save_device_data_for_device()andcreate_new_device()still duplicate the field serialization plus the long Diesel insert/update bindings for thedevicetable. With this many columns, the next schema/default change will drift one path.Also applies to: 420-478
🤖 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 304 - 415, The device row serialization and Diesel binding logic duplicated in save_device_data_for_device (the block passed to with_retry labeled "save_device_data") and create_new_device should be extracted into a single helper (e.g., build_device_row_values or bind_device_row) that: 1) takes the DeviceData (or the already-serialized Arcs) and returns the tuple or closure of values used in diesel::insert_into(device::table).values(...) and the matching .set(...) for on_conflict; 2) centralizes the Arc serialization lines (serialize_keypair, Arc::from conversions, to_string for lid/pn, account/edge_routing_info mapping) so callers simply call the helper and pass its result into their insert/update call; and 3) replace the duplicated blocks in save_device_data_for_device and create_new_device to call this new helper to keep a single source of truth for column bindings (ensure helper exposes the same types used by diesel::values and excluded(...) set).
🤖 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/send.rs`:
- Around line 658-661: The cleanup removes waiters by message_id only, which can
delete a newer retry's waiter when IDs are reused; change the
registration/cleanup to be instance-scoped: when inserting into
client.response_waiters from SendOptions.message_id, generate or return a unique
registration token/instance id (e.g., a small UUID or incrementing sequence) and
store the waiter under a compound key or as a struct containing that token
(e.g., map key (message_id, reg_id) or value with reg_id). Then in the failure
branch (the _ => block that currently calls
client.response_waiters.lock().await.remove(&message_id)) and the other similar
block (lines referenced around 1133-1136), remove only the entry that matches
both message_id and the registration token (or compare the stored waiter
instance equality) so a retry that registered a new waiter is not accidentally
removed.
---
Duplicate comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 304-415: The device row serialization and Diesel binding logic
duplicated in save_device_data_for_device (the block passed to with_retry
labeled "save_device_data") and create_new_device should be extracted into a
single helper (e.g., build_device_row_values or bind_device_row) that: 1) takes
the DeviceData (or the already-serialized Arcs) and returns the tuple or closure
of values used in diesel::insert_into(device::table).values(...) and the
matching .set(...) for on_conflict; 2) centralizes the Arc serialization lines
(serialize_keypair, Arc::from conversions, to_string for lid/pn,
account/edge_routing_info mapping) so callers simply call the helper and pass
its result into their insert/update call; and 3) replace the duplicated blocks
in save_device_data_for_device and create_new_device to call this new helper to
keep a single source of truth for column bindings (ensure helper exposes the
same types used by diesel::values and excluded(...) set).
🪄 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: d62822e7-7df0-4254-be87-e7b729b54556
📒 Files selected for processing (2)
src/send.rsstorages/sqlite-storage/src/sqlite_store.rs
|
@claude review, check if all claims are good, check if PR is ready to be merged or has any bug or showstopper |
|
Claude finished @jlucaso1's task in 3m 33s —— View job Review: Final merge-readiness audit
Claim verificationAll claims in the PR description are verified and accurate:
Previous review feedback statusAll actionable items from both previous review rounds have been addressed:
Bugs or showstoppers foundNone. The code is correct throughout. Here are the minor observations I checked and confirmed as non-issues:
VerdictReady to merge. All claims are verified, all review feedback has been addressed, no bugs or showstoppers found. The fixes are well-motivated, correctly implemented, and the code quality is high. The PR addresses real bugs (especially B1 device_id, B3 waiter leak, B4 dedup suppression) with minimal blast radius. |
1 participant
Summary
Comprehensive codebase audit addressing bugs, race conditions, resource leaks, and performance. 15 files changed across 8 commits.
Bugs fixed
Multi-account bootstrap broken on first boot (B1):
create_new_device()now inserts with the configureddevice_idinstead of relying on SQLite auto-increment. PreviouslySqliteStore::new_for_device(42)created row 1 on first boot but all reads targeted row 42, orphaning credentials.Persistence saver halts permanently after transient SQLite contention (B2):
save_device_data_for_device()andcreate_new_device()now usewith_retry()for semaphore serialization and retry on SQLITE_BUSY/locked errors (up to 5 attempts with exponential backoff). Previously these were the only write paths bypassingwith_retry, so 10 transient failures would permanently halt the background saver. UsesArc<[u8]>/Arc<str>so retry clones are atomic increments. The upsert path now uses dieselexcluded()references instead of duplicating all 20 column assignments.ACK waiter leak disables keepalives (B3):
spawn_phash_validation()now removes its waiter fromresponse_waiterson timeout/error. Previously it leaked the entry, which suppressed keepalive pings for the rest of the connection. Both send paths also clean up the waiter ifsend_node()fails before spawning the validator.App-state key-request dedupe suppresses retries for 24h after failure (B4):
request_app_state_keys()now returnsResultso callers detect failures. On error, dedup stamps are removed so keys can be retried on the next sync. Also returnsErrwhen own JID is unavailable (instead of silentOk(())) so stamps don't persist when the request was never attempted. The duplicated dedup+request logic (~28 lines in two call sites) was extracted intorequest_missing_keys_with_dedup().Upload resume offset not bounds-checked (B5): Validates server resume offset before slicing upload data. Uses
>=so offset-equal-to-length (zero bytes remaining) also falls back to full upload.Server-sync not generation-bound (B6): Server-sync tasks now re-check
connection_generationafter the version comparison loop (which awaits DB reads), preventing stale tasks from continuing after a reconnect.Error swallowing in sender key cache (B8): Replaced
let _ = backend.take_sent_message(...)with explicit error logging.Race conditions mitigated
Coordination cache eviction breaks serialization (R1): Increased default capacity for
session_locks(2k to 10k),message_queuesandmessage_enqueue_locks(2k to 5k). These hold live mutexes and channel senders; capacity eviction while a reference is held creates a duplicate lock for the same key.Old per-chat workers survive reconnect (R2): Message queue workers capture
connection_generationat spawn and check it before processing each message. Workers from a previous connection exit with a debug log instead of processing messages with stale crypto state.Performance
Pre-allocate Vecs (P2): Replaced
Vec::new()withVec::with_capacity()in 13 sites where the final size is derivable from input (usync, community features, signal cache eviction, patch decoding, app state sync, pairing, derive macros).Code quality
connection_generationatomic loads toAcquireordering (was inconsistentlySeqCstin notification.rs)CacheConfigdoc comments to reflect current defaultsunwrap()in phash validation callers via tuple destructuringBehavioral changes
Cache capacity defaults changed:
session_locksfrom 2,000 to 10,000;message_queuesandmessage_enqueue_locksfrom 2,000 to 5,000. Users who explicitly set these values are unaffected.Test plan
cargo clippy --all --testspasses with zero warningscargo test --all --libpassesnew_for_device(N)where N > 1Summary by CodeRabbit
Bug Fixes
Performance
Tests