perf(sqlite): route Signal reads through the read pool

[jlucaso1]

Summary

db_semaphore is Semaphore::new(pool_size) and pool_size defaults to 1. That default is correct for writes and I am not touching it: two deferred transactions that both read and then write deadlock on SQLite's upgrade, and busy_timeout cannot break it.

What the same permit should not be doing is governing reads. A session, identity or sender-key miss on the decrypt path waits out whatever write is in flight, put_sessions_batch from a write-behind flush included. Meanwhile the reader pool already existed: read_pool_size builds a second, query_only pool, and on main today nothing outside SharedSqlite::read and resource_report touches self.reads. Turning the knob on buys the chat store concurrency and buys the Signal path nothing.

Rebased onto 05b3c880. The base moved from 3da38692 through #1227 and #1229; neither touched storages/sqlite-storage/, and the rebase was clean.

Audit

Read-only is necessary and not sufficient. The criterion is what the caller does with a stale-absent or stale-valued answer: if it recovers, the read can move; if the answer is promoted into a cache that overwrites what it is handed, is sent on the wire, or fails an operation outright, it stays on the write queue.

Cold-load guard coverage

Routing widens the window in which a load can span a flush plus an eviction. For the reads whose consumer sits behind SignalStoreCache, the thing that closes that window is the guard in main. Traced one read at a time rather than assumed, and two of the five did not land where this branch had recorded:

Routed read	Consumer	Guard	Sufficient because
get_session_for_device	checkout_session, peek_session, has_session (all three read backend.get_session)	#1229	re-checks incarnation and removed_since after the I/O, so a superseded record is retried rather than installed
load_identity_for_device	get_identity	#1229	same re-check on the identities cache
get_sender_key_for_device	get_sender_key	#1226	same re-check, landed earlier
has_session (backend method)	Device::contains_session via check_session_exists	none, and none needed	the direct store path, not the cache; the only production caller logs the answer in all three branches
has_signal_state_for_user	SignalStoreCache::has_state_for_user	none	see below, moved back to the write queue

Two corrections this produced:

• has_signal_state_for_user leaves the migrated set. Its consumer is not one of the guarded five. has_state_for_user checks both caches for any matching key and otherwise asks the backend with no removal-seq re-check, and its callers in lid_pn.rs use a false to skip the PN to LID session migration outright. Nothing retries the skip. It goes back on the write queue.
• has_session's justification was wrong, though its verdict was not. The cache's has_session reads get_session, not this method, so fix(signal): reject cold loads that span a flush and eviction #1229 never covered it. The only path here is Device::contains_session, whose single production caller logs. The reason recorded next to it is now the caller, not the guard.

Re-validation against the new base

#1227 touched three files this audit cites (device_registry.rs, chat_actions.rs, groups.rs) and #1229 touched signal_cache.rs. Re-checked the cited behaviours: the unconditional device_registry_cache.promote on a backend hit and the InvalidRequest("no app state sync key available") on a None latest-key id both still hold, unchanged. Neither commit changes what a caller does with a stale answer for any of the other reads. The audit is current with 05b3c880, not inherited from 3da38692.

Migrated:

Method	Stale answer leads to	Verdict
get_session_for_device (get_session)	cache miss, then a pre-key bundle fetch	migrate
load_identity_for_device (load_identity)	cache miss, identity re-learned	migrate
get_sender_key_for_device (get_sender_key)	cache miss, SKDM redistributed	migrate
has_session	a debug log line	migrate
load_prekey	a consumed-not-yet-deleted row is readable; that is the normal duplicate-pkmsg path	migrate
load_prekeys_batch	same row re-offered in the upload window	migrate, see note
get_max_prekey_id	both callers hold prekey_upload_lock and the prior write is awaited	migrate
load_signed_prekey, load_all_signed_prekeys	rotation re-reads; rotate_key retries	migrate
get_app_state_version_for_device (get_version)	collection re-syncs from an older version	migrate
get_app_state_mutation_mac_for_device (get_mutation_mac)	mutation treated as new, re-applied	migrate
get_app_state_mutation_macs_batch_for_device (get_mutation_macs)	same, batched	migrate
has_same_base_key	retry path re-processes; nothing cached	migrate
get_group_metadata	read and compared, not promoted	migrate
get_all_tc_token_jids	enumeration for expiry sweeps, no per-JID decision	migrate
device_exists, load_device_data_for_device	startup, no concurrent writer	migrate

Note on the prekey reads. buffer_consumed_prekey records the id in removed_prekeys and flush deletes the row later, once the promoted session is durable, so a consumed prekey is readable for the whole write-behind interval by design; signal_adapter.rs has a test pinning exactly that. Routing extends the window by one transaction. For load_prekey the question is moot: re-reading a still-present prekey is the duplicate-pkmsg path. For load_prekeys_batch the consequence differs, since the upload window re-offers rows to the server, but that is reachable today through the buffer interval with no routing change. It is a prekey-durability question, not a routing one.

Not migrated (11), each with its reason in ON_THE_WRITE_QUEUE:

• has_signal_state_for_user -- new in this pass, see the guard table above.
• get_app_state_sync_key_for_device (get_sync_key) -- answers a peer's AppStateSyncKeyRequest; on None the handler returns an orphan MessageField, so a stale absent read tells the peer we lack a key we hold, on the wire.
• get_latest_app_state_sync_key_id_for_device -- send_app_state_mutation turns None into InvalidRequest and the user's action fails with nothing retrying behind it.
• get_msg_secret_with_ts (and get_msg_secret, which delegates) -- a miss is terminal; the reaction, vote or edit is dropped. History sync seeds secrets via put_msg_secrets directly, bypassing the live write-behind buffer.
• get_lid_mapping, get_pn_mapping -- alternate_msg_secret_jid resolves the peer's other namespace through these into the lookup above, with no cache in front on that path.
• get_all_lid_mappings -- the startup warm-up feeds these into LidPnCache::add_guarded, whose LID side replaces unconditionally.
• get_sender_key_devices -- initializes sender_key_device_cache; a stale has_key = true drops the SKDM for a device that asked for redistribution.
• get_devices -- promoted into device_registry_cache unconditionally on a miss, overwriting a newer entry.
• get_tc_token -- feeds prepare_privacy_token's scheduling decision; a stale read issues a duplicate token.
• get_pending_inbound -- read-only SQL, but its with_retry loop exists so a transient BUSY does not fail closed into a redelivery.

take_sent_message and store_received_tc_token read and write in one immediate_transaction and were never candidates. snapshot_db and resource_report are not queries.

Ordering caveat, stated once: most of the write-queue holds above are ordered by the single pooled connection at the default pool_size, not by the permit itself, since several writers check a connection out without taking it. Above pool_size = 1 that ordering weakens. get_sender_key_devices is the exception: every writer of sender_key_devices (set_sender_key_status, clear_sender_key_devices, clear_all_sender_key_devices, delete_sender_key_device_rows) goes through with_retry, so that one holds at any pool size. The rest predates this PR and fixing it means changing write serialization, which is out of scope.

Default-config behaviour

An earlier revision of this description said the read_pool_size = 0 path was unchanged. That was wrong.

Fourteen reads ran on a raw pooled connection without the permit before this branch. Routing them through the helper put them behind it, which measured about 25% on p50 at the default profile -- a real regression for a knob nobody has turned on. c780e8b fixed it: the single-connection branch checks the connection out directly and takes no permit, because the one pooled connection is already both the serialization and the snapshot. Adding a permit there only serialized the spawn_blocking dispatch that the pool wait was overlapping.

After that fix the default path differs from main by exactly one heap allocation: both do spawn_blocking(pool.get(); query) with no permit, and this branch boxes the closure first. See Cost for what the measurement can and cannot say about it.

E2E

An earlier revision listed an E2E regression as a blocker. It was not one, and the mistake was mine: I saw E2E Tests fail on one commit, checked that main was green, and concluded regression without looking at this branch's own history.

commit	E2E
bcabb05	pass
9923ffb	pass
2018115	pass
77906ae	pass
68f1a49	pass
39b9a38	fail
6c68e3a	pass
4f9bb6d	pass

One red run among seven greens on the same branch is a flake, and the five greens before it would have said so immediately. Every E2E run since has been green as well, including on the rebased head.

Independently, the suite was run against a local mock server on both trees: 165 run / 163 passed / 2 failed on this branch and the same two on main (presence::test_presence_available, privacy_tokens::test_restricted_presence_subscribe_requires_tctoken), i.e. local-mock drift. I could not reproduce that myself -- this environment has no mock server -- so that half is someone else's evidence and the conclusion above rests on CI history.

Dependency

Resolved. The guard landed in 05b3c880 (#1229) and this branch is rebased on it.

The cold-load re-check in checkout_session, peek_session, has_session and get_identity previously asked only whether an entry exists, so it could not tell "never written" from "written, persisted and removed"; a load spanning a flush plus eviction installed the older record, and for checkout_session that record goes to the cipher and advances the ratchet. #1229 stamps incarnation and removal_seq around the unlocked backend read and retries, bounded by UNLOCKED_COLD_READ_ATTEMPTS, with a locked fallback. That is what makes routing safe for the three reads whose consumers sit behind it, per the coverage table above.

Changes

• SqliteStore::read_query -- erases the closure, then routes: the deferred snapshot via SharedSqlite::read when a reader pool is configured or a wider write pool can take one; otherwise a direct connection checkout, taking the permit only when the pool is wide enough that the connection is no longer the serializer.
• with_semaphore erased the same way, so the write queue emits one body per return type instead of one per call site.
• The read-only methods above routed, 11 deliberately held back with a written reason each.
• Five chunked batch writers wrapped in one transaction each, so chunking is a parameter-limit workaround and not a commit boundary.
• get_msg_secret delegates to get_msg_secret_with_ts rather than repeating its filter chain a column narrower.
• Tests: happy and absent per migrated method at read_pool_size 0 and 4; a write refused by query_only on a reader connection, and the fallback's lack of that net asserted rather than assumed; a read while the write permit is held; a read against an open write transaction; a multi-statement read under a wider write pool, verified to fail without the snapshot; a shared-cache store declining that snapshot, with a write required to commit under a parked read and verified to fail without the guard; the chunked-write race, verified to fail without the transaction; a bidirectional source scan, so both a newly misrouted read and a stale allowlist entry fail.

Cost

In-process harness, file-backed store, taskset -c 0-3, release, deleted before this PR and never declared as a [[bench]]. Not divan: the number that matters is read tail latency with a write in flight, and divan measures isolated throughput. Both tables re-measured on 05b3c880.

The win

16 concurrent get_session per round, 40 rounds, over 512 seeded 2 KiB rows, 3 rounds. Case B adds a continuous put_sessions_batch of 1500 rows. Both configurations run in one process, so drift hits them equally.

	Case A p50	Case A p99	Case B p50	Case B p99
read_pool_size = 0	594-639 us	1.34-2.40 ms	86.4-91.4 ms	101-106 ms
read_pool_size = 4	408-541 us	1.04-1.61 ms	590-602 us	4.73-25.5 ms

Case B is the point: p50 ~88 ms down to ~0.59 ms, a factor of ~150. That is far above any noise this box produces.

The default profile, where this PR could make something worse

16 concurrent get_sender_key, 7 rounds per run, read_pool_size = 0, this branch against main. Run in alternating passes because a single ordering confounds machine drift with the code:

Pass	ran first	main p50 median	branch p50 median	branch/main
1	main	532.1 us	538.6 us	1.01
2	main	609.2 us	647.9 us	1.06
3	branch	572.7 us	545.7 us	0.95
4	branch	456.9 us	628.5 us	1.38

The difference is not resolvable on this box, and I am not claiming it is zero. The ratio has no consistent sign across the four passes, and the within-run spread is 1.65x for main and 1.70x for the branch -- larger than the effect being looked for. A first, non-alternating attempt suggested about 9%; alternating showed that was ordering drift, since the second run of any pair was systematically slower.

What can be said without the measurement: after c780e8b the default path is spawn_blocking(pool.get(); query) with no permit on both sides, and the branch adds one Box for the erased closure. That is the whole delta. It is stated as a structural argument, not as a measured equivalence.

For contrast, the 25% regression c780e8b fixed was resolvable at this noise floor: 569-641 us against 730-796 us, non-overlapping across rounds. Nothing of that size remains.

Resolved by instruction counting

Walltime on this box cannot see an effect this small, but instruction counting can. Measured off-CI by driving the CodSpeed instrumentation directly under Valgrind (CODSPEED_ENV=1 valgrind --tool=callgrind, which makes the harness report Measured instead of Checked without needing a token or an upload), on a temporary bench that exercises the default profile (read_pool_size = 0) against a file-backed store. Three runs per side, medians in instructions:

bench	main	branch	delta	spread on main
get_session miss	271,618	270,566	-1,052 (-0.39%)	758
get_session hit	105,458	104,310	-1,148 (-1.09%)	1,265
load_identity	98,764	98,965	+201 (+0.20%)	307

Instruction counts are not automatically deterministic, and it is worth being explicit about that: running the same binary twice differed by about 550 instructions until the bench's runtime was switched from multi_thread to current_thread, because work-stealing decisions vary run to run. The residual spread on main is 307-1,265. So only the miss has a delta larger than its own noise, and the reading is an upper bound rather than a point estimate: the default profile changes by at most about 1%, and where the signal exceeds the noise its sign is a reduction, not a regression.

That is consistent with the structural argument above and puts a number on it: one extra Box costs hundreds of instructions, not the 9% the first non-alternating walltime attempt suggested. The bench is temporary and is not part of this diff.

Binary size

The gate failed at one point at +39.88 KiB .text against a 32 KiB budget. An nm symbol diff of the demo example put it in three buckets: read_erased at +25.1 KiB over 186 instantiations, SharedSqlite::read at +16.1 KiB, and with_semaphore at +9.6 KiB as its instantiation count went 77 to 188. Folding read_erased's two non-snapshot branches into one and erasing with_semaphore's closure fixed it. Current reading against the 05b3c880 baseline: +26.12 KiB .text (budget 32) and +31.31 KiB stripped (budget 64).

Validation

cargo fmt --all
cargo test -p whatsapp-rust-sqlite-storage --lib   # 75 passed
cargo test -p whatsapp-rust --lib                  # 1391 passed
cargo clippy --workspace --all-targets -- -D warnings

No existing test was relaxed. One was strengthened: a_shared_cache_store_gets_no_snapshot_even_with_a_wider_write_pool asserted only the snapshot_safe flag and stayed green when the guard was deleted from the routing predicate, so it now parks a read mid-flight and requires a concurrent write to commit. That is a contract change in the stricter direction and it found a real limit -- the park has to outlast with_retry's ~310 ms backoff, or the shared-cache lock is retried away rather than observed.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Summary by CodeRabbit

• Performance

  • Improved SQLite read performance through optimized pooling, batching, and memory-mapped database support.
  • Added configurable read-pool sizing.

• Reliability

  • Improved handling of database locks and concurrent access.
  • Strengthened consistency for reads and transactional updates.
  • Added safer decoding for stored data.
  • Added configurable connection initialization.

• Data Integrity

  • Improved persistence of mutation, prekey, and sender-key data through transactional writes.

Walkthrough

SqliteStore adds WAL-aware read pooling with query-only connections, centralized read dispatch, transactional chunked writes, defensive decoding, and file-backed routing and concurrency tests.

Changes

SQLite read routing

Layer / File(s)	Summary
Read routing core storages/sqlite-storage/src/sqlite_store.rs	Adds centralized read_query dispatch, query-only reader routing, WAL snapshot gating, connection initialization, mmap configuration, and serialized fallback reads.
Read operation migration storages/sqlite-storage/src/sqlite_store.rs	Migrates eligible device, identity, key, prekey, mapping, registry, metadata, token, and secret reads while retaining serialized routing for consistency-sensitive lookups.
Atomic chunked writes storages/sqlite-storage/src/sqlite_store.rs	Wraps chunked mutation-MAC, upload-state, and sender-key status operations in transactions.
WAL routing validation storages/sqlite-storage/src/sqlite_store.rs	Adds file-backed tests for routing, query-only enforcement, snapshot consistency, concurrent reads, atomic writes, migrated methods, and source-level routing checks.

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

Sequence Diagram(s)

sequenceDiagram
  participant SqliteStore
  participant ReaderPool
  participant WriteQueue
  participant SQLite
  SqliteStore->>ReaderPool: dispatch read_query
  ReaderPool->>SQLite: execute read on query-only connection
  SqliteStore->>WriteQueue: route consistency-sensitive read
  WriteQueue->>SQLite: execute serialized operation

Loading

Possibly related PRs

• oxidezap/whatsapp-rust#1151: Modifies the related SQLite read-pooling and read-routing paths.
• oxidezap/whatsapp-rust#1074: Introduces related ConnectionInitHook configuration and connection wiring.
• oxidezap/whatsapp-rust#926: Modifies related SQLite pool and connection configuration.

Suggested labels: api-design, performance

Suggested reviewers: greptile-apps

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 81.54% which is sufficient. The required threshold is 80.00%.
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.
Description check	✅ Passed	The description clearly explains the SQLite read-pooling changes, routing decisions, safety constraints, tests, and performance results.
Title check	✅ Passed	The title clearly and concisely identifies the main change: routing Signal SQLite reads through the read pool.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch perf/sqlite-read-pool-signal-reads

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

This PR routes selected read-only Signal and persistence queries through SQLite reader connections while retaining consistency-sensitive reads on the serialized write path.

• Adds centralized, type-erased read and write dispatch helpers.
• Makes chunked batch mutations atomic with explicit transactions.
• Adds extensive coverage for WAL snapshots, shared-cache fallback behavior, read-only connections, and routing classification.

Confidence Score: 5/5

The reviewed changes appear safe to merge once the PR’s explicitly declared external dependency and draft gate are satisfied.

No blocking failure remains within the eligible follow-up review scope.

Important Files Changed

Filename	Overview
storages/sqlite-storage/src/sqlite_store.rs	Centralizes SQLite read routing, preserves write-queue placement for consistency-sensitive lookups, makes chunked mutations atomic, and adds focused routing tests.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Q[Read-only store query] --> R{Reader pool configured?}
  R -->|Yes| RP[Reader pool]
  R -->|No| S{Snapshot-safe wider write pool?}
  S -->|Yes| WP[Write pool with deferred read transaction]
  S -->|No| F[Direct write-pool checkout]
  RP --> TX[Consistent read snapshot]
  WP --> TX
  F --> O[Serialized fallback when required]

Loading

_{Reviews (14): Last reviewed commit: "fix(sqlite): keep has_signal_state_for_u..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 2

🤖 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 6044-6050: Extend the name filter in the scan that currently
checks `get_`, `load_`, and `has_` so it also recognizes `list_`, `count_`,
`find_`, `fetch_`, and `is_` prefixes, plus the `*_exists` suffix used by
`device_exists`. Preserve the existing `current` assignment and scan-count
behavior for every matched read-shaped method.
- Around line 619-632: Wrap the no-reads fallback branch in read_snapshot,
applying it around the existing with_semaphore connection-and-f call so
multi-statement readers such as has_signal_state_for_user, load_prekeys_batch,
and get_app_state_mutation_macs_batch_for_device use one snapshot. Update the
misleading comment to reflect that read_snapshot provides the consistent view,
while leaving the reads-present path unchanged.

🪄 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: 33407237-9300-4515-8926-6096c1b37702

📥 Commits

Reviewing files that changed from the base of the PR and between f8165f2 and 6b967cf.

📒 Files selected for processing (2)

• storages/sqlite-storage/src/shared.rs
• storages/sqlite-storage/src/sqlite_store.rs

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6b967cfa62

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

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	9.99 MiB	10.02 MiB	+31.31 KiB (+0.31%) 🔺
bin .text	8.01 MiB	8.03 MiB	+26.12 KiB (+0.32%) 🔺
bin allocated (text+data+bss)	9.99 MiB	10.02 MiB	+28.19 KiB (+0.28%) 🔺
llvm-lines wacore	515,594	515,594	0
llvm-lines wacore copies	16,833	16,833	0
llvm-lines whatsapp-rust lib	736,959	736,959	0
llvm-lines whatsapp-rust lib copies	23,155	23,155	0
deps crates (Cargo.lock)	462	462	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.82 MiB	1.82 MiB	0
.text wacore	695.91 KiB	695.91 KiB	0
.text wacore_binary	88.60 KiB	88.60 KiB	0
.text wacore_libsignal	178.88 KiB	178.88 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	515.62 KiB	540.30 KiB	+24.68 KiB (+4.79%) ⚠️
.text whatsapp_rust_tokio_transport	40.49 KiB	40.49 KiB	0
.text whatsapp_rust_ureq_http_client	11.83 KiB	11.83 KiB	0
.text std	991.84 KiB	993.34 KiB	+1.51 KiB (+0.15%) 🔺
.text other deps	1.90 MiB	1.90 MiB	-45 B (-0.00%) 🔽

Top movers (cargo-bloat attribution)

Crate	main	PR	Δ
whatsapp_rust_sqlite_storage	515.62 KiB	540.30 KiB	+24.68 KiB (+4.79%)
std	991.84 KiB	993.34 KiB	+1.51 KiB (+0.15%)

Baseline: 05b3c8806 (latest main run) · Head: 8a8bd7836 · Graphs

[cubic-dev-ai]

All reported issues were addressed across 2 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[jlucaso1]

Pushed 6cf393f for the review round. Taking the substantive ones in order:

Chunked writes could be observed half-applied (Codex P2, cubic P1) — real, and my fault: put_app_state_mutation_macs_for_device, delete_app_state_mutation_macs_for_device, mark_prekeys_uploaded, set_sender_key_status and delete_sender_key_device_rows chunk for the host-parameter limit and were committing each chunk on its own. Invisible while readers held the same permit; not invisible once a reader runs alongside. Each loop is now one transaction, which also stops a crash mid-batch from persisting half a batch. a_chunked_batch_write_is_never_observed_half_applied races a four-chunk write against a reader; I checked it fails immediately with the transaction removed, so it isn't decorative.

Decrypt reads under an in-flight write are untested (cubic P2) — fair, the old test held an idle permit. a_read_sees_the_last_commit_while_a_write_transaction_is_open now holds a real IMMEDIATE transaction open on the write pool and asserts the read returns the last commit rather than blocking or erroring, which is the phantom-miss case the msg-secret comment was guarding.

Reader branch duplicates SharedSqlite::read (cubic P3) — agreed, it delegates now. That also reverts the read_snapshot visibility change, so shared.rs is untouched by this PR.

Widen the routing scan (CodeRabbit, cubic P2) — done: is_/list_/count_/find_/fetch_ prefixes plus the _exists suffix, so device_exists is covered.

Wrap the fallback in read_snapshot too (CodeRabbit, cubic P3) — not doing this one. You're right that the permit doesn't serialize the raw-pool writers, and I've fixed the comment that claimed it did: at the default pool_size = 1 the serialization comes from the single pooled connection, not the permit. But the only configuration where the difference is observable is pool_size > 1, and that one deadlocks writes outright — two deferred transactions upgrade against each other and busy_timeout can't break it, which is why the field doc says to leave it at 1. Adding two statements to every read on a store that opted into no reader connections, to half-fix a configuration that doesn't work, isn't worth it. If we ever make pool_size > 1 real, this goes in with it.

Binary size: the first revision blew the gate at +116 KiB stripped / +90.6 KiB .text. read_query was generic over its closure, so all 26 call sites monomorphized a body carrying Diesel's transaction machinery. Boxing the closure before the real body drops it to one instantiation per return type: +31.53 KiB stripped, +23.50 KiB .text, measured with scripts/ci/measure_binary_size.py against the branch point. Both inside budget, no label needed.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6cf393fe95

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

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[jlucaso1]

Codex's second P2 is right and 51016bb reverts it: get_msg_secret and get_msg_secret_with_ts are back on the write queue.

I checked the failure it describes rather than taking it on faith. store_history_sync_msg_secret_entries calls put_msg_secrets directly instead of going through the live write-behind buffer (deliberately — the batch is already owned, and routing it through the buffer would retain it twice). So a live secret_encrypted_message arriving during a history-sync seed misses the buffer lookup and goes to the backend. On a total miss src/message/msg_secret.rs returns None and the reaction, vote or edit is dropped: no retry, no buffering, terminal.

My "the outcome set is unchanged" argument still holds formally — the permit ordered a concurrent read and write arbitrarily, so a miss was always reachable — but that is a statement about outcomes, not odds, and here the losing outcome is a dropped message rather than a cache miss the caller already handles. Going from "the read has to arrive before the write starts" to "the read has to arrive before the write commits" is a real widening, and a history-sync batch commit is not short. These two reads also aren't what this PR is for and contribute none of the Case B win, so there was nothing on the other side of the scale.

They're listed in ON_THE_WRITE_QUEUE with the reason, alongside get_pending_inbound, so the routing scan accepts them and the decision lives next to the check that enforces it. The Audit section of the description now records them as not migrated.

That is the one method-level judgement in this PR I got wrong, and it's the one the batch was explicitly supposed to catch — good catch.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 51016bb446

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

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[jlucaso1]

bcabb05. Codex and cubic independently landed on the same thing, and they're right.

LID/PN mappings back on the write queue. alternate_msg_secret_jid resolves the peer's other namespace through get_lid_mapping/get_pn_mapping and feeds it straight into the message-secret lookup I moved back last round. There's no cache in front on that path — the lid_pn_cache sits in front of a different caller (get_lid_pn_entry_by_user in lid_pn.rs), not msg_secret.rs, which calls the backend directly. So a lookup racing persist_and_migrate_lid_pn reads the pre-write snapshot, resolves no alternate JID, and the addon is rejected: the same terminal miss, one indirection earlier. Protecting the secret read and not the mapping read that decides which key it's looked up under was half a fix. get_all_lid_mappings stays on the read path — bulk enumeration, no caller on that path.

Declining the prekey one (Codex, load_prekey vs remove_prekey). The premise is that the single connection "waited and returned None after deletion", but the backend row is deliberately still present long before the delete transaction opens: buffer_consumed_prekey only records the id in removed_prekeys, and flush deletes it later, after the promoted session is durable. signal_cache.rs's own test asserts exactly that — load_prekey returns Some after the consume and None only after the flush. PreKeyAdapter::get_pre_key goes straight to the backend with no interception of the pending-delete set, so a second decrypt reading that row is the normal state for the whole write-behind interval, which is orders of magnitude longer than one delete statement. My change extends an intentional window by the length of a transaction; it doesn't create one. Narrowing it properly means having the cache answer load_prekey from removed_prekeys, which is signal_cache.rs — another batch is in that file and I'm not touching it here.

Also from cubic: get_msg_secret and get_msg_secret_with_ts had become near-identical closures after the revert. get_msg_secret now delegates and drops the timestamp, so there's one query and one documented rationale instead of two that can drift.

Net effect on the audit: 22 methods migrated, four held back with reasons (get_pending_inbound, get_msg_secret_with_ts, get_lid_mapping, get_pn_mapping), all four in ON_THE_WRITE_QUEUE next to the scan that enforces the rule. The measured win is unchanged — it comes entirely from the session/identity/sender-key reads, none of which are in that list.

---

Generated by Claude Code

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

storages/sqlite-storage/src/sqlite_store.rs (1)

629-643: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

The fallback comment is only true at pool_size = 1. Say so.

Look, the routing is fine. The claim in the comment is not fully fine. Line 631-633 states that no writer can hold the single pooled connection during this query. That holds only while pool_size stays at 1. Several writers in this file check out a pooled connection directly and never take the write permit: delete_session_for_device (Line 1290), put_sender_key_for_device (Line 1313), set_app_state_sync_key_for_device (Line 1423), save_base_key (Line 2728), update_device_list (Line 2811), put_group_metadata (Line 2991), touch_tc_token_sender_timestamp (Line 3232). If someone raises pool_size above 1 with read_pool_size = 0, a multi-statement fallback read can straddle one of those commits. The real readers are has_signal_state_for_user (two queries), load_prekeys_batch (chunk loop), and get_app_state_mutation_macs_batch_for_device (chunk loop).

I understand the decision to keep the fallback cheap at the documented default. Then bind the invariant to the condition in the comment, so the next person who raises pool_size sees the cost.

♻️ State the pool_size condition

         if self.reads.is_none() {
-            // No reader connections: the single pooled connection is what no
-            // writer can be holding while this query runs, so the snapshot
-            // comes for free and a transaction would only add statements.
+            // No reader connections: at the default `pool_size = 1` the single
+            // pooled connection is what no writer can be holding while this
+            // query runs, so the snapshot comes for free and a transaction
+            // would only add statements. Raising `pool_size` above 1 with
+            // `read_pool_size = 0` breaks that: the raw-pool writers (which
+            // never take the write permit) can then commit between the
+            // statements of a multi-statement read, and this branch would need
+            // the same deferred read transaction as the reader 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 629 - 643, Update
the fallback comment in read_erased to state that the no-writer snapshot
invariant applies only when pool_size is 1. Keep the existing routing and
implementation unchanged, but explicitly warn that larger pools with
read_pool_size set to 0 can allow concurrent writers during multi-statement
reads.

🤖 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 6096-6108: Throttle the polling loop around get_sender_key_devices
by adding a sub-millisecond asynchronous sleep between samples, while preserving
the existing assertion, completion condition, and 20-second timeout. Keep the
backoff inside the loop so repeated read_query calls do not continuously compete
with set_sender_key_status on the blocking pool.
- Around line 6117-6134: Update misrouted_reads to track and return the names
excused by ON_THE_WRITE_QUEUE, alongside the existing offenders and scanned
names. Add a set comparison asserting the excused names exactly match the
allowlist entries, so stale entries such as get_lid_mapping or
get_msg_secret_with_ts fail the test when no longer on the write queue.

---

Outside diff comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 629-643: Update the fallback comment in read_erased to state that
the no-writer snapshot invariant applies only when pool_size is 1. Keep the
existing routing and implementation unchanged, but explicitly warn that larger
pools with read_pool_size set to 0 can allow concurrent writers during
multi-statement reads.

🪄 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: 83c0dcd0-76a7-43d1-8dc2-ce2af914f23b

📥 Commits

Reviewing files that changed from the base of the PR and between 6b967cf and bcabb05.

📒 Files selected for processing (1)

• storages/sqlite-storage/src/sqlite_store.rs

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

<sub>Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic</sub>

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[jlucaso1]

9923ffb — took all three.

The allowlist could rot, and that's the best catch of this round. ON_THE_WRITE_QUEUE was only ever consulted in one direction: it excused a listed name, never checked the name still needed excusing. Migrate one of those reads later, forget the entry, and it sits there excusing the next method that happens to share the name while its reason string quietly becomes false — which defeats the point of a guard whose whole value is that it doesn't rot. misrouted_reads now also returns the names it excused, and the test asserts that set equals the allowlist. I checked it fires: migrating get_pn_mapping to read_query with its entry left in place produces

every ON_THE_WRITE_QUEUE entry must still name a read that bypasses read_query
  left:  ["get_lid_mapping", "get_msg_secret_with_ts", "get_pending_inbound"]
  right: ["get_lid_mapping", "get_msg_secret_with_ts", "get_pending_inbound", "get_pn_mapping"]

Throttled the chunked-write race by 200µs per sample. You're right that both sides go through spawn_blocking on the same pool and back-to-back sampling competes with the writer for threads — that's the shape that reads as flake in six months and gets deleted. Re-verified the test still has teeth afterwards: with the transaction removed it fails on the first round at 190/760, same as before the throttle.

Fallback comment now says the free-snapshot claim holds at pool_size = 1, and that raising it breaks the claim but also deadlocks writes, so it stays unsupported. That's the third time this spot has come up; better to have the condition written down than to keep re-litigating it in review.

---

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 631-636: Update the fallback read path around with_semaphore to
wrap the read closure f with crate::shared::read_snapshot, preserving deferred
snapshot semantics when read_pool_size is zero even for pool_size greater than
one. Remove or revise the nearby unsupported-configuration assumption so this
reachable configuration receives a consistent snapshot.

🪄 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: 7df5a1ed-2646-4a16-9aef-c1043fbaf046

📥 Commits

Reviewing files that changed from the base of the PR and between bcabb05 and 9923ffb.

📒 Files selected for processing (1)

• storages/sqlite-storage/src/sqlite_store.rs

[jlucaso1]

2018115 — I was wrong on this one and you were right to keep pushing.

I'd been declining the fallback snapshot on two grounds, and the new argument breaks both. pool_size > 1 with read_pool_size = 0 isn't hypothetical: it's reachable through SqliteStoreConfig and this crate's own with_config_custom_tuning_builds_and_operates uses exactly pool_size: 2, read_pool_size: 0. And "it deadlocks writes anyway, so it's unsupported" was an overstatement — raising pool_size doesn't deadlock by itself, only two deferred read-then-write transactions racing do. Plenty of the writes here are single statements. So the configuration is reachable and usable, and in it the raw-pool writers can commit between the statements of has_signal_state_for_user, load_prekeys_batch or get_app_state_mutation_macs_batch_for_device.

What I did differently from the suggested diff: rather than wrapping the fallback unconditionally, the shortcut is now conditioned on what actually makes it true — self.reads.is_none() && self.pool.max_size() <= 1. At the default that path is unchanged statement for statement, so a store that opted into nothing pays nothing new, which was the one thing I cared about defending. Anything wider goes through SharedSqlite::read, which is already the permit-plus-deferred-transaction path, so there's still one implementation of it rather than two. Test covers the wide-pool case.

That's four reviews on this one spot. The reason it took that long is that I was defending the cost constraint without checking whether the configuration I was dismissing was real. It was, and it's in our own test file.

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2018115809

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

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

[jlucaso1]

Rebased onto 3da3869 and force-pushed 4f9bb6d -> eb0478e. The body is rewritten. What follows is what actually changed and what I got wrong.

The E2E blocker was my mistake

I put this PR in draft partly because I believed it caused an E2E regression. It did not. I saw one red E2E run on 39b9a38, confirmed main was green, and concluded "regression" without ever looking at the rest of my own branch's history. That history reads:

commit	E2E
bcabb05	pass
9923ffb	pass
2018115	pass
77906ae	pass
68f1a49	pass
39b9a38	fail
6c68e3a	pass
4f9bb6d	pass

One failure in eight runs of the same branch is a flake, and five greens before it would have told me that immediately. I blocked the PR on a check I never performed. I could not reproduce the local mock-server comparison myself (no mock server here), so this correction rests on the CI history above rather than on a local run.

The "no change at the default config" claim was false, and it cost ~25% p50

The old body said behaviour at read_pool_size = 0 was unchanged. It was not. On main, fourteen reads (not the ten I had listed) run on a raw pooled connection and take no permit: load_prekey, load_prekeys_batch, load_signed_prekey, the four app-state reads, device_exists, load_device_data_for_device, has_same_base_key, get_group_metadata, and three more. Routing them through read_query put all of them behind the single write permit at the default profile, which every consumer runs.

Measured, 16 concurrent get_sender_key, default profile:

	p50
main	569-641 us
this PR before the fix	730-796 us
this PR now	525-689 us

c780e8b makes the single-connection fallback take no permit: checking out the one pooled connection already is both the serialization and the snapshot, and adding a permit on top only serialized the spawn_blocking dispatch that the pool wait was overlapping. The default profile is back on main's numbers. The body now states the change instead of denying it.

The clean audit pass moved two more methods out

The migrate/stay criterion changed mid-review from "provably read-only" to "what does the caller do with a stale answer", and I applied the new criterion retroactively, one method at a time, as reviewers found them. I never ran it over the whole set from scratch. Running it now moved two more back onto the write queue:

• get_sync_key - a None from a stale-absent read produces an orphan MessageField on the wire (src/message/special.rs:175). The caller does not retry; the wrong answer ships.
• get_latest_sync_key_id - a None fails the operation with InvalidRequest("no app state sync key available") (src/features/chat_actions.rs:793).

Neither recovers from a stale miss, so neither qualifies. ## Audit in the body now lists all 26 methods with what each reads, what the caller does with a stale answer, and the decision - including the ones that already survived review.

The prekey question I raised and never closed (load_prekey / load_prekeys_batch) is closed as moot for routing: the write-behind window already means a prekey can be read after it was consumed, independent of which pool serves the read. That is a prekey-durability question, not a routing one, and this PR does not change it either way.

CodeRabbit items

• Widening the routing scan's name filter (list_, count_, find_, fetch_, is_, _exists): already landed in 9923ffb.
• Applying read_snapshot to the no-reads fallback: implemented narrower than asked, on purpose. Unconditionally it would add two statements to every read at the default profile for no isolation gain (one connection is already a snapshot) and would reintroduce the shared-cache hazard snapshot_safe exists to avoid. The reason is recorded rather than the item silently dropped.

Still draft

get_session carries the whole measured win (Case B p50 ~37 ms -> ~0.4 ms) and it stays off the read path until the cold-load removal guard lands, because the post-I/O re-check in signal_cache.rs cannot distinguish "never written" from "written, persisted and removed". That guard PR does not exist yet (open: 1227, 1222, 1169, 1111, 218), so this stays draft. When it merges I will rebase, restore the get_session routing, redo the table, and mark ready.

No merge, no labels - yours to land.

---

Generated by Claude Code

[cubic-dev-ai]

0 issues found across 1 file (changes from recent commits).

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

<sub>Requires human review: The PR modifies concurrency and transaction semantics for core storage operations (reading sessions, identities, etc.), changing product and operational tradeoffs that require human judgment. The diff is also truncated, preventing verification of the full implementation.

Re-trigger cubic</sub>

[jlucaso1]

The binary size gate failed on eb0478e at +39.88 KiB .text against a 32 KiB budget. Fixed in 1c76a66 and 6dfe542, now +24.00 KiB. Here is what it actually was, since my first guess was wrong.

Why it grew

I reproduced the gate locally (installed libasound2-dev, built the demo example against main and against the branch) and got 8397238 -> 8438070 bytes of .text, byte-for-byte the CI number. Then I diffed symbol sizes with nm instead of guessing. Three buckets, not one:

Bucket	main	PR	Delta
read_erased, 186 instantiations, per return type	0	25 098 B	+25 098
SharedSqlite::read, per return type			+16 106
with_semaphore, 77 -> 188 instantiations, per closure and return type	22 263 B	41 327 B	+9 586
raw per-method bodies the routing replaced			-10 514

I had assumed it was all read_erased. with_semaphore was the part I had not accounted for: it is generic over the closure as well as the return type, so each of the ~10 reads I moved back onto the write queue during review minted another full copy of the acquire plus spawn_blocking.

The fix

Two dispatch-shape changes, no behaviour change:

• read_erased had three branches. The snapshot condition was the same predicate written twice, inverted; hoisting it to an early return leaves two branches that differ only in whether a permit is taken, so they fold into one body with an Option permit. Two per-return-type copies become one.
• with_semaphore gets the same closure erasure read_query already had. One body per return type instead of one per call site, which also shrinks the write path that predates this branch.

Cost of the erasure is one Box per call, against a spawn_blocking hop and a SQLite round trip. Both fixes are dispatch shape only: same predicate, same ordering, same observable behaviour, and I checked the truth table branch by branch rather than trusting the rewrite.

.text +39.88 -> +24.00 KiB, stripped +53.78 -> +28.34 KiB. CI agrees with the local number exactly.

One thing I found on the way, which is not good news

Chasing this I re-ran my own falsification check on a_shared_cache_store_gets_no_snapshot_even_with_a_wider_write_pool and it was vacuous. It asserted !snapshot_safe, which is the mechanism, not the consequence. Deleting snapshot_safe from the routing predicate left it green. That is the third test on this branch I have had to catch this way, and I only caught it because I happened to be editing the predicate it covers.

It now parks a read mid-flight on a shared-cache store and requires a concurrent write to commit. First attempt parked 300 ms and still passed unguarded, because with_retry absorbs the lock across its 10/20/40/80/160 ms backoff. Parking past that budget makes the lock fatal, and the unguarded run now fails with database table is locked: sessions. So the guard is real and now has a test that says so, but my original justification was overstated: the hazard is retried away unless the reader holds longer than ~310 ms.

6dfe542 is that test, committed separately from the size fix. Body updated with the attribution table and the corrected validation note (workspace clippy runs clean here now; the earlier "cannot build" was the missing ALSA headers, not the code).

Still draft, still gated on the cold-load guard PR, which does not exist yet.

---

Generated by Claude Code

[jlucaso1]

Rebased onto 05b3c880 and out of draft. All 5 items closed, with two findings I did not expect.

1. Rebase

Clean. 05b3c880 (#1229) and 534054f6 (#1227) both landed under this branch; neither touched storages/sqlite-storage/, so the diff is still one file.

2. Guard coverage, read by read -- and two of the five were not what this branch claimed

Three mappings held exactly as expected: get_session_for_device feeds checkout_session/peek_session/has_session (all three read backend.get_session), load_identity_for_device feeds get_identity, get_sender_key_for_device feeds get_sender_key. All guarded, all safe to route.

The other two were wrong:

• has_signal_state_for_user has no guard, and is now back on the write queue. Its consumer is SignalStoreCache::has_state_for_user, which is not one of the guarded five: it checks both caches for any matching key, then asks the backend with no removal-seq re-check. Its callers in lid_pn.rs turn a false into "skip the PN to LID session migration", and nothing retries that skip. Reverted in 0cb522b.
• has_session's recorded justification was wrong, though the verdict survives. The cache's has_session reads get_session, not this method, so fix(signal): reject cold loads that span a flush and eviction #1229 never covered it. The only path here is Device::contains_session -> check_session_exists, whose single production caller logs the answer in all three branches. It stays routed because the caller does nothing with it, not because a guard protects it. The comment now says so.

3. Re-validation against the new base

Checked rather than assumed: #1227 touched three files this audit cites. The unconditional device_registry_cache.promote on a backend hit and the InvalidRequest("no app state sync key available") on a None latest-key id both still hold, unchanged. Neither #1227 nor #1229 changes what a caller does with a stale answer for any of the other reads.

4. Both tables re-measured on 05b3c880

The win holds and got bigger: get_session Case B p50 86.4-91.4 ms -> 590-602 us, a factor of ~150.

The default-profile table is the honest half. A first, ordinary main-then-branch run showed the branch about 9% slower, consistently across 7 rounds. That looked real, so I ran it alternating instead:

Pass	ran first	main	branch	ratio
1	main	532.1 us	538.6 us	1.01
2	main	609.2 us	647.9 us	1.06
3	branch	572.7 us	545.7 us	0.95
4	branch	456.9 us	628.5 us	1.38

No consistent sign, against a within-run spread of 1.65-1.70x. The 9% was ordering drift: whichever tree ran second was slower. So I am not claiming the default profile is unchanged -- I am saying this box cannot resolve the difference, and the body says that rather than rounding it to "identical". The structural argument is separate and stronger: after c780e8b both sides do spawn_blocking(pool.get(); query) with no permit, and the branch adds one Box.

For contrast, the 25% regression c780e8b fixed was resolvable here (569-641 vs 730-796 us, non-overlapping). Nothing that size remains.

5. CI

Green on 0cb522b: E2E, Build & Test, Build & Lint, Feature Matrix, Test Stable, Clippy, Format, Rustdoc, all four Miri jobs, wasm, Cargo Deny, all CodSpeed, Binary Size (+26.12 KiB .text against the 32 budget), and Semver Checks passed this time too.

Open reviewer threads, verified not blindly accepted

• CodeRabbit's "three reads stay on the write permit for ordering the permit does not provide": two-thirds valid and already documented as an explicit caveat; its fix would change write serialization, which is out of scope here. Its third bullet is wrong -- put_sender_key_for_device/delete_sender_key_for_device write the sender_keys table, not sender_key_devices. I checked every writer of sender_key_devices (set_sender_key_status, clear_sender_key_devices, clear_all_sender_key_devices, delete_sender_key_device_rows) and all four go through with_retry, so that hold is the one that genuinely orders at any pool size. The caveat in the body now names it as the exception.
• Codex's P1 cold-load session race: correct, and it is precisely what fix(signal): reject cold loads that span a flush and eviction #1229 fixed. Closed by the dependency.
• Codex's prekey upload-window item: re-verified against the code rather than my earlier reasoning. buffer_consumed_prekey deliberately leaves the row in the backend until flush, with a test in signal_adapter.rs pinning that ("must not delete from the backend before flush"). So the window is the whole write-behind interval on any routing; this PR widens it by one transaction. Prekey-durability question, not a routing one. Kept, reason in the body.

One thing I could not do

You asked me to strip the _Generated by [Claude Code]_ footers from the body and my comments. The body is clean and has been for several revisions. The 14 existing comments I cannot fix: no tool in this session can edit an issue comment, and the API token here is empty (403). The footers are appended server-side rather than typed by me, so new comments will likely keep getting one regardless. Flagging it instead of quietly leaving it done-looking.

Ready for review. Not merging and not touching labels.

---

Generated by Claude Code