Implement end-to-end tests for TC token - #437
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded two TestClient async helpers for tcToken handling, a new end-to-end tcToken test suite (seven tests), and changes to the privacy-token notification flow to deduplicate identical tokens and conditionally re-subscribe presence when a token is stored. Changes
Sequence Diagram(s)sequenceDiagram
participant Notifier as Notification
participant Handler as PrivacyTokenHandler
participant Storage as TcTokenStorage
participant Presence as PresenceService
Notifier->>Handler: deliver privacy-token notification (includes from)
Handler->>Handler: extract from_jid, resolve sender_lid (prefer attr)
Handler->>Storage: get_tc_token(jid_key)
alt existing token and bytes == incoming
Handler->>Handler: skip write (dedupe)
else write needed
Handler->>Storage: put_tc_token(jid_key, token_entry)
Storage-->>Handler: OK
Handler->>Handler: set token_stored = true
end
alt token_stored and Presence.subscription_exists(from_jid)
Handler->>Presence: re_subscribe_when_active(from_jid)
Presence-->>Handler: subscribe result (log on failure)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/tests/privacy_tokens.rs`:
- Around line 288-297: The fresh token is written at the exact cutoff which can
race with a later call to tc_token_expiration_cutoff() inside prune_expired(),
causing flakiness; update the test to set the TcTokenEntry for fresh_key with a
small positive buffer beyond cutoff (e.g., cutoff + a small Duration) for both
token_timestamp and sender_timestamp so the token is safely newer than any
cutoff recomputed by prune_expired(); locate the put_tc_token call that writes
TcTokenEntry and adjust the timestamps accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2bc4cad-a345-446b-ab80-ef5cae58cab6
📒 Files selected for processing (2)
tests/e2e/src/lib.rstests/e2e/tests/privacy_tokens.rs
| backend | ||
| .put_tc_token( | ||
| &fresh_key, | ||
| &TcTokenEntry { | ||
| token: vec![0x02], | ||
| token_timestamp: cutoff, | ||
| sender_timestamp: Some(cutoff), | ||
| }, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Potential test flakiness due to boundary-value race condition.
The fresh_key token is stored with token_timestamp: cutoff (the exact boundary). However, prune_expired() internally calls tc_token_expiration_cutoff() again at line 299. If any time passes between line 274 and line 299, the new cutoff will be slightly higher, causing the "fresh" token to also be pruned since cutoff < new_cutoff.
Add a safe buffer to the fresh token timestamp:
Proposed fix
backend
.put_tc_token(
&fresh_key,
&TcTokenEntry {
token: vec![0x02],
- token_timestamp: cutoff,
- sender_timestamp: Some(cutoff),
+ token_timestamp: cutoff + 60, // 60s buffer avoids race with dynamic cutoff
+ sender_timestamp: Some(cutoff + 60),
},
)
.await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/privacy_tokens.rs` around lines 288 - 297, The fresh token is
written at the exact cutoff which can race with a later call to
tc_token_expiration_cutoff() inside prune_expired(), causing flakiness; update
the test to set the TcTokenEntry for fresh_key with a small positive buffer
beyond cutoff (e.g., cutoff + a small Duration) for both token_timestamp and
sender_timestamp so the token is safely newer than any cutoff recomputed by
prune_expired(); locate the put_tc_token call that writes TcTokenEntry and
adjust the timestamps accordingly.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e133d4bfd5
ℹ️ 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".
| token_timestamp: cutoff, | ||
| sender_timestamp: Some(cutoff), |
There was a problem hiding this comment.
This test is time-flaky because prune_expired() computes a new cutoff internally, but the fixture marks the "fresh" row at exactly the old cutoff value. If wall-clock seconds advance between let cutoff = tc_token_expiration_cutoff() and prune_expired(), the new cutoff becomes greater and this row is legitimately pruned, making assert_eq!(deleted, 1) intermittently fail. Use a clear margin (for example cutoff + N) or prune with the same captured cutoff to keep the boundary deterministic.
Useful? React with 👍 / 👎.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 53s —— View job Code Review: TC Token E2E Tests
Overall this is a well-structured test suite with good coverage across 7 scenarios. A few findings: Issue: Flaky boundary in
|
…dler Match two WhatsApp Web behaviors in handle_privacy_token_notification: - Skip DB write when incoming token bytes are identical to stored ones (WAWebSetTcTokenChatAction: arrayBuffersEqualUNSAFE check) - Re-subscribe presence after storing a new token for contacts with active subscriptions (WAWebHandlePrivacyTokensNotification: PresenceCollection.reSubscribeWhenActive)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/notification.rs`:
- Around line 662-665: The handler currently does a non-atomic contains() check
on client.presence_subscriptions then calls client.presence().subscribe(from),
which races with unsubscribe() and can resurrect a removed subscription; replace
this open-coded check with a presence-layer API (e.g.,
presence::re_subscribe_when_active or a similarly named method on
client.presence()) that atomically checks whether the JID is still considered
active and only performs subscribe when appropriate; update the handler to call
that new method (passing from) instead of using
client.presence_subscriptions.lock().await.contains(from) +
client.presence().subscribe(from), and implement the atomic logic inside
src/features/presence.rs (refer to the patterns around lines shown in
presence.rs for guidance).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0a735cf-a334-43b6-8afd-b514766d717f
📒 Files selected for processing (1)
src/handlers/notification.rs
…t resurrection Extracts the check-then-subscribe pattern into a dedicated method that sends the stanza without re-inserting into the tracking set. Fixes a TOCTOU race in both the tc token handler and reconnect re-subscribe path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d4aaf5a81
ℹ️ 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".
| // Skip if token bytes are identical | ||
| if existing.token == received.token { | ||
| continue; |
There was a problem hiding this comment.
The new existing.token == received.token early return drops notifications that carry the same token bytes but a newer t value, so token_timestamp never advances in storage. That timestamp is later used by is_tc_token_expired and prune_expired, which means a refreshed token can be treated as stale and eventually pruned even though the server just re-sent it. This creates avoidable token expiry/regression for restricted-contact flows whenever WhatsApp reissues identical token bytes with a newer timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/notification.rs (1)
631-651:⚠️ Potential issue | 🟠 Major
token_storedcan be a false positive across backends.At Line 635 and Line 650,
token_storedis set onput_tc_token(...).awaitsuccess, but the store contract does not distinguish “persisted” vs “no-op/skipped.” That makes the Line 660 re-subscribe condition inaccurate for no-op implementations and can trigger unnecessary subscribe traffic.Possible mitigation in this handler
if let Err(e) = backend.put_tc_token(&sender_lid, &entry).await { warn!(target: "Client/TcToken", "Failed to update tc_token for {}: {e}", sender_lid); } else { debug!(target: "Client/TcToken", "Updated tc_token for {} (t={})", sender_lid, received.timestamp); - token_stored = true; + if let Ok(Some(persisted)) = backend.get_tc_token(&sender_lid).await + && persisted.token == entry.token + && persisted.token_timestamp == entry.token_timestamp + { + token_stored = true; + } }Longer-term, the cleaner fix is returning a typed write outcome from
put_tc_tokenso callers can gate behavior without read-back.Also applies to: 659-663
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/notification.rs` around lines 631 - 651, The handler sets token_stored to true whenever backend.put_tc_token(...).await returns Ok, but put_tc_token currently doesn't distinguish persisted vs no-op/skipped so token_stored can be a false positive and cause unnecessary re-subscribe; change the implementation and/or call site so that put_tc_token returns an explicit outcome (e.g., enum WriteOutcome { Persisted, Noop, Error }) or at minimum a boolean indicating whether a write actually persisted, then in this handler (the block creating TcTokenEntry and the branch handling Ok(Some(_)) / Ok(None)) only set token_stored = true when the returned outcome == Persisted (or boolean true); keep warn()/debug() logging but gate the re-subscribe condition on the persisted outcome rather than any Ok from backend.put_tc_token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/handlers/notification.rs`:
- Around line 631-651: The handler sets token_stored to true whenever
backend.put_tc_token(...).await returns Ok, but put_tc_token currently doesn't
distinguish persisted vs no-op/skipped so token_stored can be a false positive
and cause unnecessary re-subscribe; change the implementation and/or call site
so that put_tc_token returns an explicit outcome (e.g., enum WriteOutcome {
Persisted, Noop, Error }) or at minimum a boolean indicating whether a write
actually persisted, then in this handler (the block creating TcTokenEntry and
the branch handling Ok(Some(_)) / Ok(None)) only set token_stored = true when
the returned outcome == Persisted (or boolean true); keep warn()/debug() logging
but gate the re-subscribe condition on the persisted outcome rather than any Ok
from backend.put_tc_token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 438cd738-005a-45f5-8d07-e9f698ddfe9d
📒 Files selected for processing (2)
src/features/presence.rssrc/handlers/notification.rs
When a notification carries the same token bytes but a newer timestamp, update token_timestamp in storage to prevent premature pruning by prune_expired.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8637ffbccd
ℹ️ 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".
| if let Err(e) = backend.put_tc_token(&sender_lid, &refreshed).await { | ||
| warn!(target: "Client/TcToken", "Failed to refresh tc_token timestamp for {}: {e}", sender_lid); | ||
| } | ||
| } |
There was a problem hiding this comment.
When existing.token == received.token and the incoming timestamp is newer, this branch persists the refreshed entry but does not mark token_stored = true, so the follow-up re_subscribe_when_active path is skipped. That leaves active presence subscriptions unchanged even though token validity was extended, which can keep restricted-contact presence flows stuck on stale/no-token subscription state until a later reconnect or manual re-subscribe.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit