perf(sqlite): let sibling devices share one connection pool by jlucaso1 · Pull Request #1274 · oxidezap/whatsapp-rust

jlucaso1 · 2026-08-10T22:34:14Z

Summary

Every SqliteStore constructor (new, new_for_device, with_config, with_config_for_device) builds its own r2d2 pool. A process holding N sessions against the same database file calls one of them per session and ends up with N SQLite connections.

A connection costs memory before it reads a row:

lookaside — SQLITE_DEFAULT_LOOKASIDE is 1200,40 = 48,000 B (46.9 KiB). Confirmed for this build rather than quoted from upstream: PRAGMA compile_options on the bundled SQLite 3.51.3 does not list DEFAULT_LOOKASIDE, and in the amalgamation the ctime.c table (L22738) is compiled before sqliteLimit.h defines it (L23636) — so its absence proves no -D override, and the header default applies.
page cache — SqliteStoreConfig::cache_size_kib, 512 by default. A cap on growth, not a reservation.

shared.rs already names this problem for sibling crates ("two pools mean two WAL writers fighting over the file lock, two page caches, and two busy queues"). Sibling devices in one process had no equivalent. Everything else in the store is already device-scoped — every method is *_for_device(&self, …, device_id) — so only that field ever needed to vary.

Changes

SqliteStore::share_for_device(&self, device_id) -> SqliteStore — clones pool, db_semaphore, reads, snapshot_safe and database_path, swaps only device_id. The returned store owns the pool handles, so it cannot outlive them: dropping the store it came from closes nothing. The doc says why (per-connection cost) and what it does not do (does not create the device row; does not isolate writes, with the measured throughput cost; does not split the resource report).
examples/per_connection_memory.rs — the harness that produced every number below. RSS-based, because SQLite's page cache and lookaside are sqlite3_malloc allocations from the bundled C library that a Rust GlobalAlloc wrapper never sees.
Doc-only: cache_size_kib now records that it is a cap and not the floor; SqliteStoreConfig points at the new method.

No default changed. No new dependency, no unsafe, no global state, nothing outside storages/sqlite-storage/.

Cost

Bytes per connection vs per handle. Resident-set delta, release build, 50 sessions that have each done two point reads (an idle session's steady state). The marginal column is the growth from the first session to the last divided by the sessions in between — it is the number that describes the batch, and unlike a total-over-N average it cannot be flattered by allocator memory the seeding connection left behind for the first session to reuse:

`cache_size_kib`	pool per session (marginal)	shared pool (marginal)
512 (default)	122.6 KiB	0.0 KiB
64	122.0 KiB	0.5 KiB
16	104.3 KiB	0.0 KiB
1	90.5 KiB	0.0 KiB

Scaling at the default cap — the handle side stays at one connection's worth of RSS in total (163,840 B) no matter how many sessions hang off it:

sessions	pool per session (marginal)	shared pool (marginal)	shared pool (total RSS)
10	104.4 KiB	0.0 KiB	163,840 B
50	122.6 KiB	0.0 KiB	163,840 B
200	128.7 KiB	0.1 KiB	188,416 B

N=1 does not regress: the base store is session 1, so no spare connection is opened ahead of the fleet.

The pragma result, reported even though the API went ahead. Lowering cache_size_kib was measured first, because a better default would have been a better outcome than new API. It caps growth but not the floor: 512 → 64 changed nothing for an idle session (122.6 → 122.0 KiB marginal), and even 512 → 1 only reached 90.5 KiB. So the whole knob is worth ~25% of the per-connection cost at its most extreme setting, against ~100% for sharing. Under a worst case that does fill the cap (every session full-scans a 4 MB table) it matters more — 631.3 → 150.9 KiB/session marginal for 512 → 64 — but that is the cap doing its job and buying hit rate, so the default stays where it is.

How this squares with #1235. That audit put the SQLite profile at "~530 KiB + ~512 KiB" marginal RSS per session, which is four times the 122.6 KiB here — and both are right, because they measure sessions in different states. #1235's sessions are connected and doing real work, so each one's page cache fills toward the 512 KiB cap; these are idle, two point reads in. The warm mode of this harness closes the gap and corroborates it: make every session scan a 4 MB table and the figure goes to 631.3 KiB/session, just past #1235's. Which cuts the same way — a session that actually uses its cache saves more by sharing a connection, not less. #1235's other finding also explains why this harness reads RSS rather than a Rust allocator hook: "SQLite's page cache allocates through the amalgamation's own malloc, not the Rust global allocator. My counting allocator saw exactly 0 bytes."

Concurrent write latency, both modes. Release, file-backed WAL, every session writing 200 records continuously, each arm against its own fresh database:

sessions	shared pool total	pool per session total	shared spread (fastest→slowest)	per-pool spread
8	190 ms	98 ms	190 → 190 ms	52 → 97 ms
32	799 ms	323 ms	795 → 799 ms	201 → 320 ms
64	1.72 s	681 ms	1713 → 1718 ms	367 → 677 ms

So sharing costs ~2.5x aggregate write throughput (~7.5k vs ~20k writes/s, flat in N). The cause is not lock contention but pipelining: a private connection lets one session's queueing and spawn_blocking dispatch overlap another's SQLite work, which is the same effect read_erased's comment already records. In exchange the shared queue is FIFO-fair — every session finishes within 5 ms of every other — where separate connections leave fairness to SQLite's busy handler and its random backoff, measured at ~2x spread.

read_pool_size does not change this: 0 vs 4 gave 190/193 ms shared and 98/89 ms separate at 8 sessions. A pure-write burst never touches reader connections, so the read side cannot buy back write serialization.

That is why this is opt-in and not a default. It is for mostly-idle fleets, which is the shape sessions actually have; a continuously-writing fleet should keep a store per session.

Caveat carried over from the external sweep that motivated this batch (harness not in this repo, not reproduced here): its −31.0% figure came from idle sessions and does not exercise concurrent writes at all. That comparison was also not ABBA-paired — two sweeps at different times, with an unmodified client interleaved through both reading 0.334 → 0.324 MiB/session, i.e. −2.9% drift against a −31.0% effect. Enough for an effect that size, not enough for one of a few percent. The numbers in this PR are in-process and independent of it.

Checked and not changed

The pragma alternative — numbers above. cache_size_kib reaches at most ~25% of the per-connection cost, and only by setting a cap so low it defeats the cache. Left at 512.
Reducing lookaside per connection — not possible here. SQLITE_DBCONFIG_LOOKASIDE is C-API only (no pragma), and connection_init receives a &mut SqliteConnection whose internal_connection: NonNull<ffi::sqlite3> is pub(super) in diesel, so the handle cannot be reached at all — let alone without unsafe. sqlite3_config(SQLITE_CONFIG_LOOKASIDE, …) is process-global and start-time only, which the constraints rule out twice over. The 46.9 KiB is fixed.
SharedSqlite — solves the adjacent problem (sibling crates sharing the file) and is the precedent for the reasoning, but it hands out a raw connection runner, not a store: it carries no device_id and implements none of the storage traits, so a second session cannot be built on it.
whatsmeow's sqlstore.Container — one container, one pool, N devices, which is the shape this converges on. Not copied as API: Go's database/sql pools freely because its callers do not need the write serialization this store's single permit provides, so the write-throughput trade above is ours to state, not theirs.
pool_size — untouched. It is 1 for the documented reason, and sharing does not change that reason. Note the docs are scoped accordingly: db_semaphore carries pool_size permits, so siblings serialize at the default of 1 and inherit a wider pool's width otherwise.
resource_report() — left as is, and documented. It describes the pool, which siblings share, so each handle reports the same whole-pool estimate; a fleet's reports must be counted once per pool rather than summed. Attributing shared bytes to one arbitrary handle would be a worse answer than saying they belong to the pool.
Semantics of every existing *_for_device method — untouched; the new method only produces a store with a different device_id.
The mutation paths that take a pooled connection without the write permit (delete_identity_for_device, delete_session_for_device, put_sender_key_for_device, delete_sender_key_for_device, set_app_state_sync_key_for_device) — real, unchanged by this PR, and already documented where the decision is made. At pool_size = 1 they serialize on the pool checkout instead of the permit; at pool_size > 1 they are exactly the hazard the SqliteStore::reads comment describes and the reason read_pool_size is a second pool rather than a wider first one. Routing them through with_retry is what would make pool_size > 1 safe, and it deserves its own PR rather than riding along on a memory change.
tests/e2e/tests/process_footprint.rs (from test(e2e): measure the fixed process cost against the per-session cost #1238) — the neighbouring harness, kept and not duplicated. It measures whole-session marginal RSS with an RssAnon/RssFile split and needs the mock server; this one isolates the per-connection cost and runs without it. Different question, no overlap to collapse.

Validation

cargo fmt --all
cargo clippy --workspace --all-targets -- -D warnings (excluding whatsapp-rust-voip-cli, whose alsa-sys build script needs ALSA headers absent from this container — unrelated to this change)
cargo test --workspace (excluding the same crate and e2e-tests, which needs the mock server): 60 test binaries, 0 failures, including the crate's 78 tests and both doctests.
New tests in share_for_device_tests:
- siblings_share_the_database_but_not_each_other_s_rows — writes through one handle, reads through the other; proves both see one database and that device_id isolates their rows in both directions.
- a_fleet_of_handles_opens_one_connection — counts connections via pool.state().connections: 8 handles → 1, 8 separate stores → 8. Also asserts the shared write permit via Arc::ptr_eq, so the trade-off is asserted, not assumed.
- concurrent_writes_serialize_across_siblings_at_the_default_pool_size — runs the burst both ways, printing timings, and asserts the two properties that must hold on any machine: no write fails, and no sibling starves on the shared queue. pool_size is set explicitly because the serialization claim holds for that value. Wall-clock thresholds are deliberately not asserted; the release numbers above come from the example instead.

Known-failing check: Semver Checks (informational) is red, and was already red before this branch. It runs against -p wacore -p wacore-binary -p waproto only — none of which this PR touches (the diff is two files under storages/sqlite-storage/). Its findings are pre-existing drift versus the published 0.7.0: regenerated wacore::iq::mex_operations items, a removed simd feature, and BinaryError::UnexpectedFormatByte. The job is informational by design and its own summary says so.

Every SqliteStore constructor builds its own r2d2 pool, so a process holding N sessions against the same database file ends up with N SQLite connections. Measured with the harness added here (resident-set delta, 50 idle sessions that have each done a couple of point reads), that is ~123 KiB per session before any real work: a 48,000 B lookaside slab (SQLITE_DEFAULT_LOOKASIDE 1200,40, which this build does not override) plus page cache. Nothing else about the store is per-session — every query already takes a device_id — so `share_for_device` hands a sibling device the same pool, semaphore and reader connections and swaps only that field. The per-session cost goes to zero: a fleet of 200 handles holds the same one connection as a fleet of 1. Lowering `cache_size_kib` was measured first, since a better default would beat new API: it caps growth but not the floor. 512 -> 64 changed nothing for an idle session (123.4 -> 122.6 KiB) and even 512 -> 1 only reached ~92 KiB, so it captures at most a quarter of the cost. The lookaside half cannot be touched at all without unsafe FFI — SQLITE_DBCONFIG_LOOKASIDE is C-API only and diesel keeps the sqlite3* private — so `connection_init` cannot reach it. The trade is real and stays opt-in: siblings share the single write permit, which costs ~2.5x aggregate write throughput on a burst where every session writes continuously, in exchange for a FIFO-fair queue (separate connections leave fairness to SQLite's busy handler, measured at ~2x spread between fastest and slowest session). `read_pool_size` does not change that picture — a pure-write burst never touches the reader connections. The defaults are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRxG7mynLodse41nb4h5gC

coderabbitai · 2026-08-10T22:34:46Z

📝 Walkthrough

Summary by CodeRabbit

New Features
- Added support for creating sibling device stores that share database resources while preserving device-specific data isolation.
- Added a Linux-only benchmark for evaluating SQLite memory usage and concurrent write performance.
- Added configurable benchmark settings, cache warming, compile-option reporting, and automatic temporary database cleanup.
Documentation
- Expanded guidance on shared stores and database cache memory characteristics.
Tests
- Added coverage for device isolation, shared connection reuse, and serialized writes.

Walkthrough

Adds SqliteStore::share_for_device for sibling stores that reuse connection state. Adds tests for device isolation and write serialization. Adds a Linux-only example that measures SQLite memory use and concurrent write timing.

Changes

SQLite store sharing and benchmarks

Layer / File(s)	Summary
Shared store construction `storages/sqlite-storage/src/sqlite_store.rs`	Documents shared-store and cache behavior. Adds `share_for_device`, which reuses pools, semaphores, reader state, snapshot state, and the database path while changing `device_id`.
Shared store validation `storages/sqlite-storage/src/sqlite_store.rs`	Extends test database helpers and verifies device isolation, pooled connection reuse, serialized writes, and concurrent-write completion.
Memory and write benchmarks `storages/sqlite-storage/examples/per_connection_memory.rs`	Adds Linux RSS measurement, database seeding, optional cache warming, compile-option output, configurable benchmark modes, shared-versus-independent store comparisons, timing reports, and temporary database cleanup.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Benchmark
  participant SqliteStore
  participant SQLite
  participant ProcRSS
  Benchmark->>SqliteStore: Create shared or independent stores
  SqliteStore->>SQLite: Seed and touch database connections
  Benchmark->>SQLite: Run concurrent write burst
  Benchmark->>ProcRSS: Read resident memory
  Benchmark-->>Benchmark: Report memory and timing results

Possibly related PRs

oxidezap/whatsapp-rust#926 — Modifies SqliteStore construction and connection-pool behavior used by the shared-store design.
oxidezap/whatsapp-rust#1222 — Modifies SQLite connection-pool, read-state, and routing behavior.
oxidezap/whatsapp-rust#1235 — Adds related tooling for measuring per-session SQLite memory and page-cache effects.

Suggested labels: api-design, performance

Suggested reviewers: cubic-dev-ai, greptile-apps

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: enabling sibling devices to share one SQLite connection pool.
Description check	✅ Passed	The description directly explains the shared-pool API, memory benefits, trade-offs, tests, benchmark results, and validation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch claude/sqlite-pool-multi-session-43bd6p

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-08-10T22:36:55Z

Greptile Summary

Adds an opt-in SqliteStore::share_for_device API so sibling devices using the same database can reuse one connection pool.

Shares writer and reader pools, synchronization state, snapshot configuration, and database path while changing only the device ID.
Documents memory and write-throughput tradeoffs and adds isolation, connection-count, and concurrent-write tests.
Adds a Linux RSS benchmark for comparing per-device pools with shared handles.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
storages/sqlite-storage/src/sqlite_store.rs	Adds the shared-device store constructor, supporting documentation, and tests validating pool reuse, device isolation, and serialized writes.
storages/sqlite-storage/examples/per_connection_memory.rs	Adds a Linux-only RSS and write-contention benchmark using an exclusively created owner-only scratch directory.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Base["Base SqliteStore<br/>device 1"] --> Pool["Shared SQLite pool"]
  Base --> Permit["Shared write semaphore"]
  Base --> Readers["Shared reader pool"]
  Sibling["share_for_device(2)<br/>device 2"] --> Pool
  Sibling --> Permit
  Sibling --> Readers
  Base --> Rows1["Queries scoped to device_id = 1"]
  Sibling --> Rows2["Queries scoped to device_id = 2"]

_{Reviews (3): Last reviewed commit: "fix(sqlite): report the marginal per-ses..." | Re-trigger Greptile}

The example opened a PID-derived path in the shared temp directory and deleted it first, so anyone able to write there could pre-place a symlink and redirect its database, WAL and shm writes. `create_dir` fails outright if anything already occupies the path, symlink included, so creating a directory of our own and working inside it removes the race. A Drop guard does the cleanup because two of the modes return early. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRxG7mynLodse41nb4h5gC

coderabbitai

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@storages/sqlite-storage/examples/per_connection_memory.rs`:
- Around line 198-202: Validate the parsed sessions value in the argument setup
before mode dispatch, rejecting zero and requiring sessions to be at least 1.
Preserve the existing default of 50 and ensure invalid input cannot reach the
mode-specific execution paths.
- Around line 147-168: Use distinct fresh TempDb instances for the
shared-handles and separate-pools write bursts in the example, rather than
reusing the same database URL. Ensure each mode opens its stores against its own
newly created database before calling write_burst, matching the comparison
pattern in SqliteStore’s TempDb usage.
- Around line 213-215: Update the compile-options branch in the example’s main
flow so it does not return before the shared temporary SQLite database cleanup.
Ensure the database is cleaned up after compile_options(&url) completes, either
by using a cleanup guard or by falling through to the existing cleanup block.
- Around line 49-60: Update rss_bytes to use the runtime system page size when
converting the resident page count from /proc/self/statm, replacing the
hard-coded 4096 multiplier while preserving the existing parsing and error
behavior.
- Line 53: The synchronous filesystem operations in per_connection_memory.rs
must be moved off the current-thread Tokio runtime: at line 53, run the
/proc/self/statm read via tokio::task::spawn_blocking; at line 209, run
scratch-directory creation via an awaited blocking task; and at line 277,
replace synchronous remove_dir_all cleanup with an awaited blocking task,
restructuring the owning flow as needed because Drop cannot await it.

In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 650-661: Update the write-serialization documentation near
share_for_device and its related sections to state that siblings share the
configured db_semaphore write limit, with serialization applying when pool_size
is 1 (the default), rather than unconditionally. Rename the affected test or
configure it explicitly with pool_size = 1 so its name and assertions document
that scope.

🪄 Autofix

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: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0694e382-8499-4947-9cfd-08fccb047a0d

📥 Commits

Reviewing files that changed from the base of the PR and between 5428d33 and 36556ea.

📒 Files selected for processing (2)

storages/sqlite-storage/examples/per_connection_memory.rs
storages/sqlite-storage/src/sqlite_store.rs

coderabbitai · 2026-08-10T22:39:29Z

+    // Field 2 of /proc/self/statm is resident pages. 4 KiB is the page size on
+    // every platform this harness is meant to run on; a wrong constant would
+    // scale every number here equally, so comparisons stay valid regardless.
+    let statm = std::fs::read_to_string("/proc/self/statm").expect("/proc/self/statm");


🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail rg -n -C 3 'tokio::main|std::fs::(read_to_string|remove_file)' \ storages/sqlite-storage/examples/per_connection_memory.rs fd -a '^Cargo.toml$' . -x rg -n -C 2 'tokio'

Repository: oxidezap/whatsapp-rust

Length of output: 7071

🏁 Script executed:

#!/bin/bash set -euo pipefail printf '%s\n' '--- example structure and relevant call sites ---' ast-grep outline storages/sqlite-storage/examples/per_connection_memory.rs sed -n '1,75p;180,225p;250,290p' storages/sqlite-storage/examples/per_connection_memory.rs printf '%s\n' '--- manifest and call context ---' fd -a '^Cargo.toml$' storages/sqlite-storage -x sh -c 'echo "--- $1"; cat -n "$1"' sh {}

Repository: oxidezap/whatsapp-rust

Length of output: 8744

🏁 Script executed:

#!/bin/bash set -euo pipefail nl -ba storages/sqlite-storage/examples/per_connection_memory.rs | sed -n '185,285p' printf '%s\n' '--- all filesystem operations in the example ---' rg -n -C 2 'std::fs|remove_file|remove_dir_all|create_dir' storages/sqlite-storage/examples/per_connection_memory.rs

Repository: oxidezap/whatsapp-rust

Length of output: 200

🏁 Script executed:

#!/bin/bash set -euo pipefail cat -n storages/sqlite-storage/examples/per_connection_memory.rs | sed -n '185,285p' printf '%s\n' '--- all filesystem operations in the example ---' rg -n -C 2 'std::fs|remove_file|remove_dir_all|create_dir' storages/sqlite-storage/examples/per_connection_memory.rs

Repository: oxidezap/whatsapp-rust

Length of output: 5417

🏁 Script executed:

#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path("storages/sqlite-storage/examples/per_connection_memory.rs") lines = p.read_text().splitlines() checks = { "current_thread runtime": any("#[tokio::main(flavor = \"current_thread\")]" in line for line in lines), "statm read": any('std::fs::read_to_string("/proc/self/statm")' in line for line in lines), "scratch directory creation": any("std::fs::create_dir(&dir)" in line for line in lines), "synchronous cleanup": any("std::fs::remove_dir_all(&self.0)" in line for line in lines), "rss before measurement": any("let before = rss_bytes();" in line for line in lines), "rss after measurement": any("let after = rss_bytes();" in line for line in lines), "early return cleanup path": any("compile_options(&url).await;" in line for line in lines) and any("writes(&url, sessions, cache_kib).await;" in line for line in lines), } for name, present in checks.items(): print(f"{name}: {'present' if present else 'missing'}") for i, line in enumerate(lines, 1): if any(token in line for token in ( "read_to_string(\"/proc/self/statm\")", "std::fs::create_dir(&dir)", "std::fs::remove_dir_all(&self.0)", "rss_bytes()", )): print(f"{i}: {line.strip()}") PY

Repository: oxidezap/whatsapp-rust

Length of output: 669

Move synchronous filesystem I/O off the current-thread Tokio runtime.

Run the /proc/self/statm read and scratch-directory creation with tokio::task::spawn_blocking. Replace synchronous remove_dir_all cleanup with an awaited blocking task; Drop cannot await it.

📍 Affects 1 file

storages/sqlite-storage/examples/per_connection_memory.rs#L53-L53 (this comment)

storages/sqlite-storage/examples/per_connection_memory.rs#L209-L209

storages/sqlite-storage/examples/per_connection_memory.rs#L277-L277

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@storages/sqlite-storage/examples/per_connection_memory.rs` at line 53, The synchronous filesystem operations in per_connection_memory.rs must be moved off the current-thread Tokio runtime: at line 53, run the /proc/self/statm read via tokio::task::spawn_blocking; at line 209, run scratch-directory creation via an awaited blocking task; and at line 277, replace synchronous remove_dir_all cleanup with an awaited blocking task, restructuring the owning flow as needed because Drop cannot await it.

Source: Coding guidelines

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36556ea5f8

ℹ️ 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".

chatgpt-codex-connector · 2026-08-10T22:41:19Z

+    pub fn share_for_device(&self, device_id: i32) -> Self {
+        Self {
+            pool: self.pool.clone(),
+            db_semaphore: Arc::clone(&self.db_semaphore),
+            reads: self.reads.clone(),


Report the shared connection pool only once

When sibling handles back separate clients, each handle's resource_report() inspects this same cloned r2d2 pool and reports the full cache estimate from all of its open connections. Consequently, polling and summing per-client Client::resource_report() values counts one shared SQLite cache once per sibling (and concurrent polls can instead return None for siblings that lose the single try_get() race), making the documented per-session resource attribution inaccurate. The shared pool therefore needs shared-aware attribution or a way to report the process-owned storage bytes only once.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-10T22:41:19Z

+                .expect("open"),
+        );
+    }
+    let (total, fastest, slowest) = write_burst(separate).await;


Benchmark both write modes against fresh data

When the example runs in writes mode, the handles burst first inserts every (address, device_id) pair into this same database, and this second burst then uses the identical peer.{n}.{i}:0 keys and device IDs. Because put_session is an upsert, the reported pools timing measures conflict updates against an already populated and warmed database while handles measures initial inserts, so the advertised throughput comparison cannot be attributed to connection sharing. Run each mode against a fresh database (and release the first pool) before comparing them.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-10T22:41:19Z

+        .nth(1)
+        .and_then(|f| f.parse().ok())
+        .expect("resident pages");
+    pages * 4096


Use the host page size when converting RSS pages

On Linux targets configured with a page size other than 4 KiB, such as 64 KiB arm64 systems, /proc/self/statm still reports a count of native pages, so multiplying by 4096 underreports every displayed RSS and per-session KiB value (by 16× in that example). This harness is documented only as Linux-specific rather than 4-KiB-specific and presents absolute memory costs, so it should query the page size or read a kB-valued RSS field instead of hard-coding it.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-10T22:41:19Z

+        return;
+    }
+
+    seed(&url).await;


Avoid seeding through the allocator being measured

In either RSS mode, seed() first opens and heavily uses a SQLite connection and then drops it before the baseline. SQLite/glibc can retain those freed connection, lookaside, and page-cache allocations in the process RSS, after which the measured stores reuse them; this is especially damaging to handles, where the sole new connection can appear to cost almost nothing because its memory was already included in before. Seed in a separate process or otherwise establish the database without allocating and freeing the same connection resources before taking this baseline.

Useful? React with 👍 / 👎.

github-actions · 2026-08-10T22:48:54Z

📦 Binary size report

Metric	main	PR
bin size (stripped)	10.04 MiB	10.04 MiB
bin .text	8.05 MiB	8.05 MiB
bin allocated (text+data+bss)	10.04 MiB	10.04 MiB
llvm-lines wacore	533,462	533,462
llvm-lines wacore copies	17,415	17,415
llvm-lines whatsapp-rust lib	761,982	761,982
llvm-lines whatsapp-rust lib copies	23,767	23,767
deps crates (Cargo.lock)	462	462

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.84 MiB	1.84 MiB	0
.text wacore	693.00 KiB	692.69 KiB	-312 B (-0.04%) 🔽
.text wacore_binary	81.61 KiB	81.61 KiB	0
.text wacore_libsignal	178.98 KiB	178.98 KiB	0
.text wacore_appstate	22.35 KiB	22.35 KiB	0
.text wacore_noise	20.94 KiB	20.94 KiB	0
.text waproto	1.74 MiB	1.74 MiB	0
.text whatsapp_rust_sqlite_storage	540.56 KiB	540.56 KiB	0
.text whatsapp_rust_tokio_transport	40.49 KiB	40.49 KiB	0
.text whatsapp_rust_ureq_http_client	12.68 KiB	12.68 KiB	0
.text std	995.63 KiB	995.63 KiB	0
.text other deps	1.90 MiB	1.90 MiB	+312 B (+0.02%) 🔺

Baseline: 5428d33f1 (latest main run) · Head: 68d09a170 · Graphs

…aring docs Review found three things worth fixing in the measurement and one in the docs. The harness measured total RSS growth divided by session count, which charges every session for whatever the seeding connection left behind in the allocator for the first one to reuse. It now also reports the *marginal* cost — the growth from the first session to the last, over the sessions in between — which is the number that describes the batch and the one that survives a warm allocator. It moves the conclusion not at all and sharpens it: 122.6 KiB per additional session with a pool each, 0.0 KiB with shared handles. The `writes` mode ran both arms against one database, so the second measured upserts over the first's rows and warmed pages. Each arm now gets its own. RSS comes from smaps_rollup, already in kB, instead of multiplying statm's page count by an assumed 4 KiB page size. And zero sessions is rejected rather than dividing by it. On the docs: `db_semaphore` carries `pool_size` permits, so siblings serialize only at the default of 1 — a base store with a wider pool passes that width on. The test now sets `pool_size` explicitly and says so in its name. Also documents that `resource_report()` describes the pool, which siblings share, so a fleet's reports must be counted once per pool rather than summed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRxG7mynLodse41nb4h5gC

jlucaso1 · 2026-08-10T22:52:13Z

Review round: six of the findings were valid and are fixed in b2fa5ea; two are declined with reasons. The PR body now carries the corrected numbers.

Fixed

Both write arms shared one database (CodeRabbit, Codex). Real: put_session is an upsert, so the second arm measured conflict updates over the first's rows and warmed pages. Each arm now gets its own database file.
Seeding through the allocator being measured (Codex). The sharpest one. The harness now also reports the marginal cost — RSS growth from the first session to the last, over the sessions in between — so whatever the seeding connection left behind for session 1 to reuse is subtracted out rather than spread across the average. It moves the conclusion not at all and sharpens it: 122.6 KiB per additional session with a pool each, 0.0 KiB with shared handles.
Hard-coded 4 KiB page size (CodeRabbit, Codex). Now reads Rss: from /proc/self/smaps_rollup, which is already in kB, so nothing depends on the kernel's page size — and it needs no libc.
Write serialization claimed unconditionally (CodeRabbit). Correct: db_semaphore carries pool_size permits, so siblings serialize at the default of 1 and inherit a wider pool's width otherwise. Doc scoped, and the test now sets pool_size explicitly and says so in its name.
Zero sessions (CodeRabbit) — rejected up front.
Predictable scratch path / umask (Greptile, both rounds). The harness now creates its own directory with DirBuilder::mode(0o700), which is both an exclusive create (it fails if anything, symlink included, already occupies the path) and owner-only regardless of umask.

Declined

spawn_blocking for the harness's filesystem I/O (CodeRabbit). Skipping. This is a measurement harness on a current-thread runtime with nothing else scheduled at the points it reads /proc; routing those reads through a thread hop would add allocation and scheduling noise to the very RSS number being taken. The repo's blocking-work rule is about not stalling the client's read loop, which this binary does not have.
resource_report() counting a shared pool once per sibling (Codex). Real, and now documented on share_for_device rather than papered over. The report describes the pool, and siblings share one, so each handle returns the same whole-pool estimate; a fleet's reports must be counted once per pool instead of summed. I did not change the behaviour because the alternatives are worse: attributing shared bytes to one arbitrary handle invents an owner, and returning None for siblings discards a real measurement. The bytes belong to the pool, and the point of this PR is that there is now only one pool left to count.

Semver Checks (informational) is red and was red before this branch — it runs only against wacore, wacore-binary and waproto, none of which this PR touches (the diff is two files under storages/sqlite-storage/). Its findings are pre-existing drift versus the published 0.7.0.

Generated by Claude Code

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 650-655: Guard all SQLite mutation paths with the shared
db_semaphore and lock-retry behavior, including delete_identity_for_device,
delete_session_for_device, put_sender_key_for_device,
delete_sender_key_for_device, and set_app_state_sync_key_for_device. Route these
and any other writes through with_retry or the existing shared mutation helper,
then add a sibling-store concurrency test using pool_size: 2 that races an
unguarded mutation path.

🪄 Autofix

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: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1853e1a9-383a-4a23-bbff-9fa566e7ef10

📥 Commits

Reviewing files that changed from the base of the PR and between e1e8f35 and b2fa5ea.

📒 Files selected for processing (2)

storages/sqlite-storage/examples/per_connection_memory.rs
storages/sqlite-storage/src/sqlite_store.rs

coderabbitai · 2026-08-10T22:54:30Z

+    /// - **Isolate writes.** Siblings share the write permits, of which
+    ///   [`SqliteStoreConfig::pool_size`] decides the number — so at its
+    ///   default of 1 their writes serialize against each other, and a base
+    ///   store built with a wider pool passes that width on instead. That is
+    ///   the trade, and at the default it is not free: on a burst where every
+    ///   session writes continuously, sharing


🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Guard every mutation path with the shared write limiter.

Line 650 says sibling stores share the configured write permits. Several mutation paths do not acquire db_semaphore or retry lock errors. Examples include delete_identity_for_device at Line 1273, delete_session_for_device at Line 1405, put_sender_key_for_device at Line 1428, delete_sender_key_for_device at Line 1478, and set_app_state_sync_key_for_device at Line 1545.

If pool_size > 1, sibling handles can check out separate connections and these writes can contend for SQLite's single writer lock. Route every mutation through with_retry or one shared mutation helper. Add a sibling-store test with pool_size: 2 that races one currently unguarded mutation path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@storages/sqlite-storage/src/sqlite_store.rs` around lines 650 - 655, Guard all SQLite mutation paths with the shared db_semaphore and lock-retry behavior, including delete_identity_for_device, delete_session_for_device, put_sender_key_for_device, delete_sender_key_for_device, and set_app_state_sync_key_for_device. Route these and any other writes through with_retry or the existing shared mutation helper, then add a sibling-store concurrency test using pool_size: 2 that races an unguarded mutation path.

jlucaso1 · 2026-08-10T22:55:36Z

On the unguarded mutation paths (delete_identity_for_device, delete_session_for_device, put_sender_key_for_device, delete_sender_key_for_device, set_app_state_sync_key_for_device): the observation is factually correct — they check a connection out of the pool directly, without taking db_semaphore. Declining anyway, for three reasons.

It is unchanged by this PR. git show origin/main has the same bodies; share_for_device clones the pool and semaphore and touches neither. Two separately-constructed stores at pool_size > 1 have exactly the same exposure today. The task this branch came from also rules the change out explicitly: don't alter the semantics of existing *_for_device methods.

At the default it is not a bug. With pool_size = 1 the pool hands out one connection at a time, so those writes serialize on the checkout rather than on the permit. Same outcome, different mechanism.

At pool_size > 1 it is the documented reason not to raise pool_size, and the repo already says so at the point the decision is made — the SqliteStore::reads field comment:

Deliberately a second pool rather than extra connections in the main one: several write paths check a connection out directly, without the semaphore, and are serialized today only because the pool hands out one connection at a time. Growing that pool would let two of them run at once and deadlock on the write-lock upgrade — the exact failure this change exists to avoid.

That is why read_pool_size exists as a separate pool, and why pool_size is 1 with a doc telling callers to leave it there. Repeating that rationale on share_for_device is the duplication AGENTS.md warns against ("Comments carry the why of a decision, at the single point where it is made. Repeating a rationale at call sites is how it goes stale"), so the share_for_device doc states only what it changes: siblings share whatever write width pool_size configured, which is 1 by default.

Adding the suggested pool_size: 2 racing test would encode a configuration the crate documents as unsafe. If routing every mutation through with_retry is wanted, it is a worthwhile change on its own — it is what would make pool_size > 1 safe — but it belongs in its own PR, against main, where it can be reviewed as the concurrency change it is rather than as a rider on a memory one.

Happy to open that as a follow-up issue if you'd like it tracked.

Generated by Claude Code

coderabbitai Bot added api-design performance labels Aug 10, 2026

greptile-apps Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread storages/sqlite-storage/examples/per_connection_memory.rs Outdated

coderabbitai Bot requested changes Aug 10, 2026

View reviewed changes

greptile-apps Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread storages/sqlite-storage/examples/per_connection_memory.rs Outdated

chatgpt-codex-connector Bot reviewed Aug 10, 2026

View reviewed changes

greptile-apps Bot approved these changes Aug 10, 2026

View reviewed changes

coderabbitai Bot requested changes Aug 10, 2026

View reviewed changes

jlucaso1 merged commit af74003 into main Aug 10, 2026
25 of 26 checks passed

jlucaso1 deleted the claude/sqlite-pool-multi-session-43bd6p branch August 10, 2026 23:28

jlucaso1 mentioned this pull request Aug 10, 2026

docs: document SqliteStore::share_for_device oxidezap/whatsapp-rust-docs#510

Merged

2 tasks

Uh oh!

Uh oh!

Conversation

jlucaso1 commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Cost

Checked and not changed

Validation

Uh oh!

coderabbitai Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

greptile-apps Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Binary size report

Uh oh!

jlucaso1 commented Aug 10, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

jlucaso1 commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jlucaso1 commented Aug 10, 2026 •

edited

Loading

coderabbitai Bot commented Aug 10, 2026 •

edited

Loading

greptile-apps Bot commented Aug 10, 2026 •

edited

Loading

github-actions Bot commented Aug 10, 2026 •

edited

Loading