docs(perf): inventory per-client retention and bounds

[jlucaso1]

Summary

An audit, not a fix. The technique under audit is a published per-client retained-memory pass built on three moves: store encoded bytes in caches instead of decoded object graphs, allocate a per-client structure on first use rather than in the constructor, and put a hard cap on every cache a workload or a third party can grow. The first two multiply by session count; the third is an upper bound under an adversary, which is a different and equally real concern.

The question was whether each has an analogue here. Everything below was measured in this repo — no figure is carried over from where the technique came from.

Result: the first two already hold, the third holds nearly everywhere, and what the audit actually found was in the instrument rather than in the memory. No bound is added, changed, or exposed as configuration.

How it was measured

At the global allocator (tests/e2e/tests/per_client_retention.rs, 16 clients, median of the tail after the first two) rather than in RssAnon, which is page-quantised: a 2 KiB structure and a 3 KiB one both round to the same page count, and the queues under audit are exactly that size. Figures are identical under dev and release.

No mock server was available, so nothing here is a warm-cache count from a live session; per-cache occupancy is stated as its configured ceiling, and memory_report() remains the instrument for live counts.

Findings

1. A per-client cache retaining decoded objects where bytes would do — does not apply.

recent_messages is Cache<ChatMessageId, Arc<Vec<u8>>>: encoded protobuf, never a waproto:: graph. Its default capacity is 0, so the DB is the only copy unless a consumer opts into the L1 — both the change the technique prescribes and one it does not.

The only place decoded protos are retained per client is inbound_commit_batch, where the decode is the point: the entries are dispatched to the consumer and handed to the durability hook as wa::Message. Re-encoding would add a decode per delivery to a path that holds the batch for milliseconds. It is bounded at 400 messages / 4 MiB instead — and that is a flush threshold, not a hard ceiling, since maybe_flush_inbound_commits checks after the insert, so a batch overshoots by its last message.

2. A constructor-time allocation most sessions never use — measured, not worth deferring.

The two preallocated bounded queues a session owns:

queue	capacity	payload	retained
major_sync_task_sender (Client::new)	32	56 B	2 816 B
transport events (EVENT_CHANNEL_CAPACITY)	64	40 B	3 840 B

against a constructed client:

what	retained
Client + UreqHttpClient	24 267 B
+ empty InMemoryBackend	26 211 B

(The two halves of the first row drift by a few hundred bytes between runs — part of the HTTP agent materializes lazily — while their sum holds to a handful of bytes. The client's own share is ~22.4 KiB.)

So the sync queue is ~11% of a constructed client — but a connected session's marginal cost is ~530 KiB (measured previously, agent_docs/observability.md), against which both queues together are ~1.2%. Deferring the sync queue also means the builder handoff no longer has a receiver to give Bot::build for its worker; the transport queue is created at connect and every connected session drains it. Paying an indirection on a per-connection path to defer 0.5% of a session is the wrong trade. Not changed.

Worth recording alongside: first-use allocation is already the pattern where it pays — group_cache, app_state_processor, delivery_receipt_queue, transport_ack_queue and custom_enc_handlers are all OnceLocks built on first use.

Also measured, because the two have opposite cost profiles and it is easy to assume wrong: a capacity cap here is a bound, not a reservation. PortableCache starts on an empty HashMap — capacity 1 and capacity 10 000 both retain 248 B until entries arrive. Raising a cap costs nothing at construction, which is why the coordination caches can afford to be sized generously.

3. An uncapped cache a workload or a peer can grow — four, all deliberate.

• lid_pn_cache — deliberate and already pinned by a test. Evicting a still-valid mapping silently downgrades Signal addresses to @c.us.
• app_state_key_requests — swept by deadline (retain) on every insert path, so a busy client holds only key ids whose deadline has not passed. The sweep is lazy rather than timed, so an idle client keeps its stamps until the next request or a reconnect. A cap would drop a dedup stamp, which either re-asks the phone for a key already requested or loses the throttle that keeps a stuck sender from re-asking every few seconds. Growth is self-limiting regardless: each new key id costs a peer message on the wire.
• pending_device_sync — one entry per distinct user seen with an unknown device. Offline entries are drained at the end of the backlog; entries the online path adds are removed only by that same drain or by teardown, so a connection that never drains keeps them for its lifetime (which also suppresses a second immediate refresh for those users). A cap would skip a device refresh and leave the next send addressed to a stale device list, so if this ever matters the fix is a removal on the online path, not a ceiling.
• AppStateProcessor::key_cache — found late, and the reason the guard changed. One expanded key per distinct key id the server's patches reference, no cap, no TTL, emptied only by clear_key_cache on reconnect. It lives in wacore, and the coverage guard only parsed this crate's src/, so nothing pointed at it. Unlike the three above, the backend stays authoritative here, so a cap would be safe — but nothing has measured how many distinct keys a real account accumulates, so it reports a count rather than guessing a number.

The rest of the uncapped set is app-driven rather than peer-driven (presence_subscriptions, the waiter lists, stanza_interceptors), scope-guarded (pending_retries), or a stall signal that must not drop work (transport_ack_queue, delivery_receipt_queue).

4. What already has a bound — the inventory, in agent_docs/observability.md.

Every TTL/capacity cache, every coordination cache and its evict_guard, the Signal store's clean-only eviction, the VoIP admission caps, the write-behind buffer, the 256-entry topology ring, and the compile-time WATCHED set that bounds server-sent AB props — each with the configured number and what eviction costs. That table is the durable half of this batch.

Writing it down turned up three claims the code does not support, all now recorded as findings rather than repeated as facts:

• message_retry_counts does not fail closed. It is described in-code as the retry ceiling's enforcement, but its 500-entry capacity is a plain FIFO eviction with no guard, so more than 500 distinct retry keys inside the 1h TTL forgives MAX_DECRYPT_RETRIES — the evicted key's next decrypt failure restarts at 1. Left as is deliberately (entries are two integers, so the honest fix is a larger capacity rather than a mechanism, and no workload has been measured against 500 concurrent retry keys).
• The Signal store caches are bounded at 2 000 only while flushes succeed. evict_clean_entries skips dirty and deleted keys, which is right — dropping unpersisted state is worse than the memory — but it means a backend that stops accepting writes leaves everything dirty and the maps grow past the cap.
• msg_secret_buffer's 4 096 is not a hard ceiling on the reported count. A queueing future cancelled while backpressured force-buffers what it holds rather than losing captures.

Changes

memory_report() learns the transient inbound retention. The report could not see the largest thing a client retains: inbound_commit_batch accumulates to 4 MiB of decoded protos, two orders of magnitude above every collection the report does name. It, msg_secret_buffer and pending_device_sync now report under a Transient retention section, and app_state_key_cache joins the unbounded-collection counts. Observability only — no bound is added, changed, or exposed as configuration, and no protocol semantics move.

Two boundaries are documented rather than instrumented, both because closing them would mean real work on the receive path to sharpen a report:

• inbound_commit_batch counts what is waiting to be committed. A batch already handed to commit_inbound_batch is still resident but no longer counted, because pricing it would mean re-encoding the batch on the commit path. has_entries draws the same line.
• The drain commit's encode arena is cleared but not shrunk, so one oversized message leaves its capacity resident for the session. Sampling it means taking a lock a commit holds across its backend write, so it is named in the inventory instead.

offline_receipt_buffer likewise stays exempt from the report — noted in the doc rather than quietly implied to be covered.

tests/report_coverage.rs can see through a newtype, an alias, and a crate boundary. The scan looked only at a field's own type expression within this crate, so a collection behind PendingDeviceSync, behind type Pending = HashMap<..>, or inside a wacore type was invisible. It now resolves crate-local types one level with aliases collapsed to a fixed point, and parses wacore/src as well. One level and not transitively, because self_weak: Weak<Client> makes the type graph reach every collection from every field, and a guard that flags everything flags nothing. Fields past that boundary are in EXEMPT with their reason — widening to wacore added exactly two (ab_props, bounded by the compile-time WATCHED set; pair_code_state, whose Vec is one server pairing ref).

Checked and not changed

collection	bound	what eviction costs
group_cache	1h TTL, 250	re-query on miss
device_registry_cache	1h TTL, 5 000	store stays authoritative
recent_messages	5m TTL, 0 (disabled)	DB is authoritative
message_retry_counts	1h TTL, 500, FIFO	a forgiven MAX_DECRYPT_RETRIES — see above
undecryptable_dispatched	5m TTL, 1 000	a duplicate event
pdo_pending_requests / pdo_requested	30s TTL, 200 / 24h TTL, 512	a repeated PDO request
sender_key_devices_cache	1h TTI, 500	a redundant SKDM
session_recreate_history	1h TTL, 256	one un-throttled recreate
session_locks / chat_lanes / group_distribution_locks	10 000 / 5 000 / 512	nothing — an evict_guard refuses to evict a lock a task holds, so the map briefly exceeds capacity rather than minting a second lock for one key
resend_rate_limiter	4 096, FIFO	fail-open by design: an evicted bucket is recreated full, so undersizing forgives rate, never over-throttles
group_devices_memo / skdm_warm_memo / dm_devices_memo	64 / 64 / 512	a recompute
SignalStoreCache sessions / identities / sender keys	2 000 each (+1/8 slack), while flushes succeed	nothing — only clean entries are evicted, so an unpersisted record is never dropped, and a failing backend grows the maps past the cap
SignalStoreCache::sender_key_locks	2 000, idle-only	nothing — only locks held solely by the map are dropped
inbound_commit_batch	400 messages / 4 MiB, checked after insert	commits early, no loss; overshoots by one message
msg_secret_buffer	4 096, except on cancellation	nothing — a producer that would exceed it parks, and a cancelled one force-buffers past the mark rather than losing captures
device-topology changed-users log	256	a memo recompute; overflow can never serve stale data
AbPropsCache	compile-time WATCHED set	server props outside it are discarded at parse
CallRegistry pre-offer controls / ringing group calls / event queues	64 entries or 1 MiB each	fail-closed admission
PendingCallLinkJoins transitions	32	fail-closed
major_sync_task_sender / transport events / noise send jobs	32 / 64 / 8	backpressure, no loss

The rule this confirms: a cap must not evict state something is relying on. The evict_guard on the coordination caches and the clean-only eviction in SignalStoreCache are the same idea applied twice — exceed capacity rather than break an invariant.

Validation

• cargo fmt --all --check, cargo clippy --all-targets -- -D warnings, and the same for -p e2e-tests (not a default member).
• cargo test — all green; cargo test --doc -p whatsapp-rust separately, since nextest cannot run doctests. cargo test --all-features and cargo clippy --all-features also run clean.
• New characterization tests: memory_report_names_the_uncommitted_drain_batch (visible while accumulating, reaches the byte total, clears when handed to a commit), memory_report_counts_the_offline_device_sync_queue (counts, dedups per user, clears on drain), a_newtype_or_alias_does_not_hide_its_collection, alias_chains_resolve_to_a_fixed_point, and the extended memory_report_display_sections_stay_aligned (now bounded to its section).
• Measurement harness tests/e2e/tests/per_client_retention.rs. The measurements are #[ignore]d in the style of process_footprint.rs — tools for producing a number, not guards — and need no mock server (build() does no I/O). Its one non-ignored test, the_allocator_counts_what_it_hands_out, pins the counter every figure depends on.
• No new dependencies. Binary size unchanged (0 B).

One deviation to flag

The batch asked for branch perf/per-session-retention-audit. This session is pinned to claude/cache-limits-audit-ewz54c and instructed never to push elsewhere without explicit permission, so the work is there. Say the word and I will move it.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Memory reports now include transient retention from inbound commit batches, buffered message secrets, and pending device synchronization.
  • Reports provide clearer collection counts, encoded sizes, and transient-retention details.
  • Pending device synchronization counts reflect deduplication and queued entries.

• Documentation

  • Expanded observability guidance covering retention, cache limits, allocation behavior, eviction, backpressure, and report coverage.

• Tests

  • Added coverage for memory-report accuracy, retention measurements, queue behavior, and collection detection.

Walkthrough

The PR extends MemoryReport with transient retention metrics, adds per-client heap measurement tests, improves growable-collection coverage detection, and documents collection bounds and lifecycle behavior.

Changes

Memory observability

Layer / File(s)	Summary
Transient retention reporting src/client.rs, src/client/accessors.rs, src/client/tests.rs, src/message/commit_batch.rs, src/msg_secret_buffer.rs, src/pending_device_sync.rs	MemoryReport now reports inbound commit batches, buffered message secrets, and pending device synchronization. Aggregation, display ordering, flushing behavior, deduplication, and drain behavior are tested.
Per-client retention measurements tests/e2e/tests/per_client_retention.rs	Ignored end-to-end tests measure live heap retention for queues, caches, backends, HTTP clients, and constructed clients using allocator counters and median samples.
Growable collection coverage detection tests/report_coverage.rs	The coverage test resolves crate-local aliases and one-level nested types, reports collection paths, and validates explicit exemptions.
Retention behavior documentation agent_docs/observability.md	The documentation records allocation measurements, collection bounds, eviction behavior, lifecycle-driven growth, backpressure, and retry-counter capacity caveats.

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

Possibly related PRs

• oxidezap/whatsapp-rust#962: Introduced related MemoryReport and Client::memory_report() observability work.
• oxidezap/whatsapp-rust#1235: Extended MemoryReport and collection coverage for previously unreported client collections.
• oxidezap/whatsapp-rust#1116: Added related retained-queue and memory-report tracking.

Suggested labels: performance

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

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant InboundCommitBatcher
  participant MsgSecretWriteBuffer
  participant PendingDeviceSync
  participant MemoryReport
  Client->>InboundCommitBatcher: Read pending batch statistics
  Client->>MsgSecretWriteBuffer: Read pending secret count
  Client->>PendingDeviceSync: Read queued user count
  Client->>MemoryReport: Build transient retention report
  MemoryReport->>MemoryReport: Aggregate and display collection statistics

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% 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.
Title check	✅ Passed	The title clearly and concisely summarizes the per-client retention and bounds audit.
Description check	✅ Passed	The description directly explains the memory-retention audit, observability changes, tests, measurements, and unchanged limits.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/cache-limits-audit-ewz54c

---

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

The PR audits per-client retained memory and cache bounds, expands memory_report() to expose transient retention, and strengthens report-coverage checks for collections hidden behind local types and aliases.

• Reports pending inbound commit batches, buffered message secrets, pending device syncs, and cached app-state keys.
• Documents cache bounds, intentional unbounded collections, and measured per-client construction costs.
• Adds characterization and allocator-based measurement tests without changing runtime bounds or protocol behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
agent_docs/observability.md	Documents measured client-retention costs, bounded and unbounded collections, and limitations of the memory report.
src/client.rs	Extends MemoryReport and its display layout with app-state and transient-retention statistics.
src/client/accessors.rs	Collects the newly exposed retention counts without initializing the lazy app-state processor.
src/message/commit_batch.rs	Exposes pending batch entry and encoded-byte counts for observability while preserving batching behavior.
tests/report_coverage.rs	Expands collection coverage detection through one crate-local type level with alias resolution.
tests/e2e/tests/per_client_retention.rs	Adds an ignored allocator-based measurement harness and documents the accepted process-wide attribution limitation.

_{Reviews (3): Last reviewed commit: "docs(perf): widen the retention audit to..." | Re-trigger Greptile}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d01249b9e6

ℹ️ 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]

Account for cancellation overshoot in the buffer bound

When a queueing future is aborted while backpressured, QueuedEntries::drop deliberately inserts all remaining secrets with a limit of usize::MAX; the existing dropped_backpressured_queue_force_buffers_entries test even demonstrates a limit-1 buffer reaching 2 entries. Consequently this public report field can exceed 4096 under cancellation or teardown, so documenting it as strictly bounded gives operators an incorrect ceiling for precisely the abnormal condition this memory report is intended to diagnose.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Correct the retry counter's fail-closed claim

With more than 500 distinct retry keys inside the one-hour TTL, CacheEntryConfig::build_with_ttl applies the configured capacity and PortableCache::evict_to_capacity FIFO-evicts the oldest unguarded counter. A later decrypt failure for that message therefore re-enters increment_retry_count through the None => 1 arm, so the cache does forgive MAX_DECRYPT_RETRIES; describing it as “fail closed, not evict” contradicts both the 500-entry table above and the implementation and can hide a retry-storm weakness uncovered by this audit.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Expand aliases on Client field types

Aliases are expanded only inside collected struct and enum definitions, after which the alias map is discarded. Thus a Client field that directly names an alias is still invisible: for example, group_cache: OnceLock<Arc<GroupCache>> does not resolve type GroupCache = TypedCache<...> and is not considered a growable candidate, so removing it from memory_report() would still leave this guard green. Expand the Client field identifiers through aliases, or include aliases in the lookup used by growable_path.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Keep batches visible while their commit is in flight

Once a flush starts, InboundCommitBatcher::take clears state.entries and state.bytes before commit_inbound_batch awaits the backend, Signal flush, and durability hook while retaining the same messages in its items slice. If any of those operations stalls, pending_stats therefore reports zero even though the full batch remains resident—the exact stopped-commit condition the new metric is intended to reveal. Track the taken batch as in-flight until the commit completes rather than reading only the accumulation state.

Useful? React with 👍 / 👎.

[jlucaso1]

Correct on the mechanism, and I've documented it rather than instrumenting it — with the reason, so the next reader doesn't have to re-derive the gap.

Tracking the taken batch means keeping its byte figure alive past take, and the byte figure is a message_encoded_len sum. commit_inbound_batch receives Arc<[InboundMessage]> and never recomputes that sum, so in-flight accounting would mean re-encoding the batch on the receive path purely to sharpen a report field. agent_docs/observability.md is explicit that these are "honest estimates for attribution and leak detection, not byte-exact accounting", and this is the trade that line is about.

So pending_stats now reads "waiting to be committed", stated as such on the accessor and on the public MemoryReport field, and the test pins both halves of that contract (visible while accumulating, cleared when handed to a commit). has_entries already drew the same line for the same reason, so the two agree.

If a stalled commit ever needs to be visible, the cheap version is a count-only in-flight gauge on ReinsertGuard — which already brackets exactly that window — rather than bytes. Not worth adding speculatively.

---

Generated by Claude Code

[chatgpt-codex-connector]

Include online users in the device-sync retention scope

For an online unknown-device message, schedule_unknown_device_sync inserts the user into this set and spawns get_user_devices, but neither the success nor failure path removes that user; repository-wide uses of the set only remove entries through take_all during an offline flush or through teardown. Such entries can consequently accumulate for the whole connection and permanently suppress another immediate refresh for the same user, so describing the set as bounded by and emptied with one offline drain is incorrect.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Do not claim every unbounded collection is reported

offline_receipt_buffer is listed in this section but has no field or read anywhere in MemoryReport; tests/report_coverage.rs explicitly exempts it instead. During an offline snapshot or a deferred drain-to-live transition, receipts can therefore remain retained without the promised count warning, so either expose its depth or qualify this statement and document the exemption rather than asserting that every listed collection is reachable.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Describe four MiB as a flush threshold, not a hard cap

enqueue_inbound_commit adds the complete next message before maybe_flush_inbound_commits checks state.bytes >= MAX_BATCH_BYTES, so a batch just below four MiB necessarily exceeds the stated ceiling by the size of its next message, and one individually large decoded message can overshoot it substantially. Because this audit is specifically inventorying adversarial hard bounds, calling the batch “capped at 4 MiB” gives an invalid maximum; document the one-entry overshoot or enforce the limit before insertion.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Move the message-secret gauge out of the offline section

msg_secret_buffer is also populated by live inbound captures and by outbound sends, so a slow backend can make this gauge nonzero or saturated while no offline drain exists. Rendering it under Offline-drain working set misattributes that retention and can send operators looking at the wrong subsystem; give the write-behind buffer its own transient-persistence section or use a heading that covers both live and offline activity.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Account for lazily retained expired key-request stamps

app_state_key_requests removes expired deadlines only when another nonempty request enters request_missing_keys_with_dedup; there is no timer-driven sweep. After a client requests many keys and then becomes idle, those entries can remain resident indefinitely past their deadlines (until another insertion or reconnect), so the assertion that the map holds only unexpired key IDs understates long-lived-session retention.

Useful? React with 👍 / 👎.

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	10.04 MiB	10.04 MiB	0
bin .text	8.05 MiB	8.05 MiB	0
bin allocated (text+data+bss)	10.04 MiB	10.04 MiB	0
llvm-lines wacore	533,670	533,670	0
llvm-lines wacore copies	17,422	17,422	0
llvm-lines whatsapp-rust lib	761,982	762,446	+464 (+0.06%) 🔺
llvm-lines whatsapp-rust lib copies	23,767	23,771	+4 (+0.02%) 🔺
deps crates (Cargo.lock)	462	462	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.84 MiB	1.84 MiB	0
.text wacore	692.69 KiB	693.00 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: b2343e431 (latest main run) · Head: 9a25cd742 · Graphs

[jlucaso1]

Semver Checks (informational) is red for a reason that predates this branch, so I'm leaving it:

Failed in:
  feature simd in the package's Cargo.toml
  Summary semver requires new major version: 2 major and 0 minor checks failed
  Finished [ 13.967s] wacore-binary

That's wacore-binary's simd feature, removed on main by c705cd1 (#1262, "remove the SIMD after measuring what it was worth"). cargo-semver-checks compares against the last published 0.7.0, which still declares the feature, so it will keep reporting a major break until the next release bump. This branch changes no Cargo.toml at all — git diff --name-only origin/main...HEAD is nine files under agent_docs/, src/ and tests/ — so it cannot be the source, and the job's own summary says it does not block.

Everything else is green.

---

Generated by Claude Code

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@src/client/tests.rs`:
- Around line 4851-4874: Update the test around the transient-retention
assertions to locate the next report section heading after “--- Transient
retention ---” and bound the slice to that section’s range. Check
inbound_commit_batch, msg_secret_buffer, and pending_device_sync only within
this transient-retention slice, while preserving the existing section-order and
history-sync assertions.

In `@tests/e2e/tests/per_client_retention.rs`:
- Around line 41-64: Add a non-ignored allocator_accounting_twin test in
per_client_retention.rs that constructs valid layouts and exercises
Counting::alloc, Counting::realloc, and Counting::dealloc, asserting LIVE
accounting after each operation and cleanup. Keep existing ignored tests
unchanged, and validate the new focused test under Miri.

In `@tests/report_coverage.rs`:
- Around line 201-206: Update the alias-expansion loop in resolve_aliases so it
repeats until a full pass makes no alias entry change, rather than stopping
after two iterations; preserve the resulting fixed-point mapping for
growable_path to detect collection types hidden behind crate-local aliases. Add
a regression test covering an alias chain longer than four links, such as a
chain ending in Vec and used by a Client field.

🪄 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: 9319edfc-0042-44c2-87d8-69d00a0e57d7

📥 Commits

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

📒 Files selected for processing (9)

• agent_docs/observability.md
• src/client.rs
• src/client/accessors.rs
• src/client/tests.rs
• src/message/commit_batch.rs
• src/msg_secret_buffer.rs
• src/pending_device_sync.rs
• tests/e2e/tests/per_client_retention.rs
• tests/report_coverage.rs

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb74d009cb

ℹ️ 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]

Account for cancellation overshoot in pending_len docs

When a backpressured queueing future is cancelled or aborted, QueuedEntries::drop inserts every remaining secret with a limit of usize::MAX, so pending_len() can exceed MAX_PENDING_MSG_SECRETS; describing this count as bounded can make callers interpret an abnormal teardown value as impossible. Fresh evidence after the earlier correction is that this newly exposed accessor's own documentation still retains the strict-bound claim; document the cancellation overshoot here as well.

AGENTS.md reference: AGENTS.md:L45-L45

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Account for the retained encode arena

After an offline-drain commit completes, arena.clear() leaves its capacity allocated, so one oversized message or overshooting batch can leave several MiB retained for the rest of the client's lifetime while pending_stats() reports zero entries and bytes. Excluding the arena only because its lock may be held during a backend write makes the new report miss this memory even after the write finishes; track its capacity without blocking the report, or release oversized capacity after use.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Do not classify dirty Signal caches as hard-bounded

When Signal-store flushes fail, newly written sessions, identities, or sender keys remain dirty, and evict_clean_entries explicitly skips every dirty key; with continuing traffic, these maps can therefore grow indefinitely past both 2,000 and the stated 1/8 slack. Listing them in the bounded inventory gives operators a false ceiling during the backend-failure condition where retained-memory diagnostics matter most; classify the dirty working set as conditionally unbounded while retaining the clean-entry eviction target.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Scan growable types in workspace crates

Restricting type discovery to this crate's src/ leaves growable state behind workspace-crate types invisible. For example, Client::app_state_processor names wacore's AppStateProcessor, whose key_cache: HashMap<...> adds an expanded entry for every distinct app-state key and has no capacity or TTL until reconnect; it is absent from memory_report(), yet this guard remains green because it never parses wacore. Include applicable workspace sources in the lookup or explicitly report/exempt these external wrappers.

Useful? React with 👍 / 👎.

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

[jlucaso1]

Second review round applied in e6e8a33. Every finding was checked against the code before acting; six were valid, one I'm declining.

The one that mattered. The guard only parsed this crate's src/, so a collection behind a wacore type was invisible to it — and one was: AppStateProcessor::key_cache gains an expanded key per distinct key id the server's patches reference, has no cap and no TTL, and is emptied only by clear_key_cache on reconnect. That is a fourth uncapped, server-influenced per-client collection, and my audit missed it for a mechanical reason. It now reports a count (MemoryReport::app_state_key_cache) and the guard parses wacore/src. Widening surfaced exactly two other candidates, both exempted with reasons: ab_props is filtered against the compile-time WATCHED set at parse time, and pair_code_state's Vec is one server pairing ref.

Four bound claims corrected, all of which made the inventory less true than it should have been:

• The Signal store caches are bounded at 2 000 while flushes succeed. evict_clean_entries skips dirty and deleted keys — which is right, since dropping unpersisted state is worse than the memory — so a backend that stops accepting writes leaves everything dirty and the maps grow past the cap.
• msg_secret_buffer's 4 096 is not a hard ceiling on the reported count; that is now said on the accessor as well as the report field.
• The encode arena is cleared but not shrunk, so one oversized message leaves its capacity resident for the session. Still unreported for the lock reason, but named in the inventory instead of implied to be transient.
• The transient-retention display assertion searched to end-of-report, so a field moving into a later section would still have passed.

Alias resolution now iterates to a fixed point. One note for whoever reads that diff: the loop needs the per-round dedup. Without it an alias naming another one twice doubles its ident list each round, and with wacore in the map that is an OOM, not a slow test — I hit it. Multiplicity carries no information for "which idents are reachable", so dropping it is free.

Declined: a Miri-covered twin for the counting allocator. I added the_allocator_counts_what_it_hands_out as the harness's one non-ignored test, which was the useful half — it pins alloc/realloc/dealloc accounting, in bounds rather than equalities since the counter is process-wide. Miri specifically I'm leaving: miri.yml covers wacore-binary, wacore-noise and wacore-libsignal, and the gate exists for the aliasing- and uninit-sensitive unsafe there (Yokeable/StableDeref, set_len). This unsafe impl GlobalAlloc forwards every method to System with the layout it received and adds a relaxed counter update; there is no aliasing or initialization question for Miri to answer, and wiring the gate to e2e-tests for it would be out of proportion.

Validation: fmt, clippy --all-targets -D warnings (default and -p e2e-tests), cargo test, and the harness re-run — figures unchanged (24 267 B client+HTTP, 2 816 B / 3 840 B queues, 248 B at both cache capacities).

---

Generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e6e8a33756

ℹ️ 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]

Traverse nested wrappers before declaring fields covered

The one-level lookup still misses an existing unbounded collection: Client::core resolves only to CoreClient's immediate Device and CoreEventBus fields, while the growable HandlerSnapshot::handlers: Vec<_> sits several wrappers deeper (CoreEventBus::inner). Calls to the public Client::subscribe_handler API can therefore grow this per-client handler list without it appearing in memory_report(), yet this guard remains green. Traverse recursively with cycle detection that stops specifically at Client, or explicitly report/exempt core, rather than treating every second-level collection as invisible.

Useful? React with 👍 / 👎.