fix(tc-token): atomic newer-wins store; drop tc_token_lock and close the cross-source race

[jlucaso1]

Summary

Follow-up to #975 (item #3 from the perf-audit follow-up list). #975 fixed a tc-token lost-update by adding a tc_token_lock around the history-sync candidate path, but deliberately left the store's overwrite unconditional. That lock only ordered history-sync chunks against each other — the other writer, the privacy-notification path (privacy_business.rs), was never under the lock, so the two sources still raced, and each did a non-atomic get-then-store that could let an older token clobber a fresher one.

Change

Push the newer-wins rule into the store. store_received_tc_token now overwrites the (token, token_timestamp) pair only when the stored token is a byte-less placeholder or the incoming timestamp is at least as new:

• sqlite: read + conditional upsert inside an IMMEDIATE transaction (WAL + 30 s busy_timeout serialize concurrent writers, so the read+write can't lose to another writer).
• in-memory: the same check under the state lock.
• trait default: a best-effort read-modify-write for third-party backends (documented, same caveat as the sender-bucket contract).

The sender_timestamp bucket stays an independent advance-only upsert (touch_tc_token_sender_timestamp), so neither writer clobbers the other's field.

With the store atomic:

• history-sync drops its get-then-store and the tc_token_lock (the Client field is removed) — concurrent chunks converge on the store.
• privacy_business keeps its pre-filter unchanged: it also gates a presence re-subscribe on a genuinely new token (token_stored), and it's now belt-and-suspenders with the atomic store.
• The cross-source race (history-sync ↔ privacy notification), which no lock ever covered, is closed.

Why the timestamps are comparable

Both callers already compare the incoming timestamp against the stored token_timestamp on the same scale (privacy_business does received.timestamp < existing.token_timestamp), so a single newer-wins rule subsumes both callers' existing checks. Verified that no existing test overwrites a newer real token with an older one.

Testing

• New store_received_tc_token_is_newer_wins in both backends — older write rejected, newer accepted, a placeholder still accepts the first real token, sender bucket preserved.
• Existing tc-token + history-sync suites pass unchanged.
• wacore (1063) + whatsapp-rust (930) + sqlite (53) lib tests pass; clippy clean; wasm32 --no-default-features builds.

---

Generated by Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Warning

Review limit reached

@jlucaso1, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6ef26763-1bd9-431b-882c-ce9d8c9f2501

📥 Commits

Reviewing files that changed from the base of the PR and between bfafaa0 and 656b916.

📒 Files selected for processing (1)

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

📝 Walkthrough

Walkthrough

Removes the tc_token_lock mutex from Client and its construction, eliminating lock-based serialization of history-sync tc-token writes. Replaces this with atomic newer-wins upsert semantics in the default trait, in-memory store, and SQLite store, each with new tests.

Changes

TC-token newer-wins concurrency change

Layer / File(s)	Summary
Remove tc_token_lock mutex and its usage src/client.rs, src/client/lifecycle.rs, src/history_sync.rs	The tc_token_lock field is removed from Client and its construction; store_tc_token_candidate no longer serializes writes via a lock or describes the old get-then-store flow, and now documents backend atomic upserts instead.
Default trait newer-wins upsert logic wacore/src/store/traits.rs	The default store_received_tc_token implementation gains a guard that skips overwriting an existing real token when the incoming timestamp is older, preserving sender_timestamp; documentation is updated accordingly.
In-memory store newer-wins implementation and tests wacore/src/store/in_memory.rs	store_received_tc_token conditionally overwrites token/timestamp only for placeholders or non-older incoming timestamps; a new test verifies newer-wins, placeholder replacement, and sender_timestamp preservation.
SQLite store newer-wins implementation and tests storages/sqlite-storage/src/sqlite_store.rs	store_received_tc_token uses an immediate_transaction to read existing token/timestamp and conditionally overwrite only when stale or placeholder; a new test covers the same newer-wins scenarios.

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

Possibly related PRs

• oxidezap/whatsapp-rust#966: Introduces the same store_received_tc_token placeholder-replacement and sender_timestamp preservation logic that this PR builds newer-wins semantics on top of.

Suggested labels: api-design, breaking-change

Suggested reviewers: greptile-apps

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: moving tc-token storage to newer-wins semantics and removing the tc_token_lock.
Description check	✅ Passed	The description directly matches the patch, explaining the store-level newer-wins fix, lock removal, and related backend changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/perf-audit-parallelization-v42g7v

---

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 closes the cross-source race between the history-sync and privacy-notification writers by pushing the newer-wins rule into the store itself, making it atomic for both built-in backends. The tc_token_lock on Client is removed because it was never the right guard for the cross-source case.

• SQLite: adds an IMMEDIATE transaction around a read + conditional upsert in store_received_tc_token, leveraging WAL + busy_timeout + with_retry to serialize concurrent writers without application-level locks.
• In-memory: adds the same newer-wins guard under the existing state lock, preserving sender_timestamp on update.
• Trait default: best-effort read-modify-write with documented non-atomicity caveat; newer-wins and placeholder semantics are consistent with the built-in backends.

Confidence Score: 5/5

Safe to merge; the IMMEDIATE transaction correctly serializes the read+write and the in-memory lock provides equivalent protection.

Both built-in backends implement the newer-wins check atomically (IMMEDIATE transaction for SQLite, state lock for in-memory). The removal of tc_token_lock is fully justified by the store-level atomicity. Tests cover the stale-write rejection, newer-write acceptance, placeholder promotion, and sender-bucket preservation scenarios. No existing behaviour is regressed.

No files require special attention.

Important Files Changed

Filename	Overview
storages/sqlite-storage/src/sqlite_store.rs	Replaces an unconditional upsert with an IMMEDIATE-transaction read+conditional-write; also adds a comprehensive newer-wins test. Logic and error propagation via with_retry are correct.
wacore/src/store/in_memory.rs	Adds the newer-wins guard under the existing state lock; sender_timestamp is correctly left untouched on update. Test mirrors the SQLite one.
wacore/src/store/traits.rs	Trait default upgraded to a best-effort newer-wins read-modify-write; non-atomicity for third-party backends is clearly documented.
src/history_sync.rs	Drops the in-caller get-then-store and lock; delegates newer-wins to the store. Simplified correctly.
src/client.rs	Removes the now-redundant tc_token_lock field from Client.
src/client/lifecycle.rs	Removes the tc_token_lock initialization, consistent with the field removal in client.rs.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant HS as history_sync
    participant PB as privacy_business
    participant Store as store_received_tc_token
    participant DB as SQLite (IMMEDIATE txn)

    HS->>Store: "store_received_tc_token(jid, token_A, ts=5000)"
    Store->>DB: BEGIN IMMEDIATE
    DB-->>Store: read existing → None
    Store->>DB: "upsert token_A, ts=5000"
    DB-->>Store: commit

    PB->>Store: "store_received_tc_token(jid, token_B, ts=3000)"
    Store->>DB: BEGIN IMMEDIATE
    DB-->>Store: read existing → (token_A, 5000)
    Note over Store,DB: ts 3000 < 5000 → skip write
    DB-->>Store: commit (no-op)

    PB->>Store: "store_received_tc_token(jid, token_C, ts=7000)"
    Store->>DB: BEGIN IMMEDIATE
    DB-->>Store: read existing → (token_A, 5000)
    Note over Store,DB: ts 7000 ≥ 5000 → write
    Store->>DB: "upsert token_C, ts=7000"
    DB-->>Store: commit

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant HS as history_sync
    participant PB as privacy_business
    participant Store as store_received_tc_token
    participant DB as SQLite (IMMEDIATE txn)

    HS->>Store: "store_received_tc_token(jid, token_A, ts=5000)"
    Store->>DB: BEGIN IMMEDIATE
    DB-->>Store: read existing → None
    Store->>DB: "upsert token_A, ts=5000"
    DB-->>Store: commit

    PB->>Store: "store_received_tc_token(jid, token_B, ts=3000)"
    Store->>DB: BEGIN IMMEDIATE
    DB-->>Store: read existing → (token_A, 5000)
    Note over Store,DB: ts 3000 < 5000 → skip write
    DB-->>Store: commit (no-op)

    PB->>Store: "store_received_tc_token(jid, token_C, ts=7000)"
    Store->>DB: BEGIN IMMEDIATE
    DB-->>Store: read existing → (token_A, 5000)
    Note over Store,DB: ts 7000 ≥ 5000 → write
    Store->>DB: "upsert token_C, ts=7000"
    DB-->>Store: commit

Loading

_{Reviews (3): Last reviewed commit: "fix(tc-token): route store_received_tc_t..." | Re-trigger Greptile}

[coderabbitai]

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)

3012-3062: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Route store_received_tc_token through with_retry.
This write path bypasses the retry wrapper used elsewhere for SQLite commits. BEGIN IMMEDIATE can still surface SQLITE_BUSY under contention, so a transient lock failure can bubble out instead of being retried.

🤖 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 3012 - 3062, The
store_received_tc_token write path currently bypasses the SQLite retry helper,
so transient SQLITE_BUSY errors from the immediate transaction can fail instead
of being retried. Wrap the spawn_blocking transaction in the same with_retry
flow used by other SQLite commit paths, and keep the existing atomic
read-modify-write logic inside the retryable closure so contention is handled
consistently without changing the token conflict behavior.

🤖 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.

Outside diff comments:
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 3012-3062: The store_received_tc_token write path currently
bypasses the SQLite retry helper, so transient SQLITE_BUSY errors from the
immediate transaction can fail instead of being retried. Wrap the spawn_blocking
transaction in the same with_retry flow used by other SQLite commit paths, and
keep the existing atomic read-modify-write logic inside the retryable closure so
contention is handled consistently without changing the token conflict behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: efa239af-1dd8-4247-b64f-2d33e479cedd

📥 Commits

Reviewing files that changed from the base of the PR and between 61df380 and 37fd35d.

📒 Files selected for processing (6)

• src/client.rs
• src/client/lifecycle.rs
• src/history_sync.rs
• storages/sqlite-storage/src/sqlite_store.rs
• wacore/src/store/in_memory.rs
• wacore/src/store/traits.rs

💤 Files with no reviewable changes (2)

• src/client/lifecycle.rs
• src/client.rs

[cubic-dev-ai]

0 issues found across 3 files (changes from recent commits).

<sub>Requires human review: Refactors concurrent token storage to a newer-wins rule, removing a mutex and changing the tc-token write path. Despite tests, this affects core token sync logic and should be reviewed to prevent subtle race conditions.

Re-trigger cubic</sub>

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

[cubic-dev-ai]

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

<sub>Requires human review: Refactors tc-token storage with atomic newer-wins logic, removing a lock. High-impact change in critical data path; requires human review to verify atomicity and correctness.

Re-trigger cubic</sub>

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	10.76 MiB	10.76 MiB	+2.69 KiB (+0.02%) 🔺
bin .text	8.77 MiB	8.77 MiB	+2.31 KiB (+0.03%) 🔺
bin allocated (text+data+bss)	10.76 MiB	10.76 MiB	+4.23 KiB (+0.04%) 🔺
llvm-lines wacore	503,158	503,177	+19 (+0.00%) 🔺
llvm-lines wacore copies	17,243	17,244	+1 (+0.01%) 🔺
llvm-lines whatsapp-rust lib	745,692	745,528	-164 (-0.02%) 🔽
llvm-lines whatsapp-rust lib copies	24,271	24,271	0
deps crates (Cargo.lock)	466	466	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.60 MiB	1.60 MiB	-953 B (-0.06%) 🔽
.text wacore	528.83 KiB	528.83 KiB	0
.text wacore_binary	157.49 KiB	157.49 KiB	0
.text wacore_libsignal	178.73 KiB	178.73 KiB	0
.text wacore_appstate	156.42 KiB	156.42 KiB	0
.text wacore_noise	26.05 KiB	26.05 KiB	0
.text waproto	1.60 MiB	1.60 MiB	0
.text whatsapp_rust_sqlite_storage	509.25 KiB	512.98 KiB	+3.73 KiB (+0.73%) 🔺
.text whatsapp_rust_tokio_transport	43.61 KiB	43.61 KiB	0
.text whatsapp_rust_ureq_http_client	10.47 KiB	10.47 KiB	0
.text std	1020.50 KiB	1020.03 KiB	-488 B (-0.05%) 🔽
.text other deps	2.94 MiB	2.94 MiB	0

Top movers (cargo-bloat attribution)

Crate	main	PR	Δ
whatsapp_rust_sqlite_storage	509.25 KiB	512.98 KiB	+3.73 KiB (+0.73%)

Baseline: 61df3803c (latest main run) · Head: 5eaeea313 · Graphs