Refactor/remove background sync #373
Conversation
WalkthroughThreads per-account and global optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as IntegrationTest
participant TC as TestClient
participant Relays
participant EP as AccountEventProcessor
participant DB
Test->>TC: publish_follow_list(contacts)
TC->>Relays: send Event(kind=ContactList, tags=[p:...])
Relays-->>EP: deliver ContactList event
EP->>EP: is_event_timestamp_valid?
alt valid
EP->>EP: process event
alt kind ∈ {ContactList, GiftWrap, MlsGroupMessage}
EP->>DB: update_last_synced_max(pubkey, created_ms)
DB-->>EP: Ok / Err
end
else invalid
EP-->>EP: skip (log)
end
EP-->>Test: processing complete
sequenceDiagram
autonumber
participant WN as Whitenoise
participant AM as AccountsStore
participant NM as NostrManager
participant Relays
WN->>AM: list accounts (last_synced_at)
AM-->>WN: accounts[]
WN->>WN: compute_global_since_timestamp()
alt all synced
WN-->>WN: global_since = min(last_synced_at) - buffer
else
WN-->>WN: global_since = None
end
loop per account
WN->>AM: account.since_timestamp(buffer)
AM-->>WN: account_since (Option)
WN->>NM: setup_account_subscriptions_with_signer(..., since=account_since_or_global)
NM->>Relays: SUB Filter.since(since?)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![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
🧹 Nitpick comments (8)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (3)
1-5: Minor: align import order with repo guidelinesOrder imports as: std, external crates, current sub-mods, then crate:: paths.
As per coding guidelines
92-109: Reduce flakiness risk: increase settle delay or rely solely on retry600ms can be tight in CI. Either bump to ~1–2s or drop the sleep and trust the retry loop.
Example bump:
- tokio::time::sleep(tokio::time::Duration::from_millis(600)).await; + tokio::time::sleep(tokio::time::Duration::from_millis(1500)).await;
111-127: Same flakiness consideration for global metadata publishMirror the settle delay change here for consistency.
- tokio::time::sleep(tokio::time::Duration::from_millis(600)).await; + tokio::time::sleep(tokio::time::Duration::from_millis(1500)).await;src/integration_tests/scenarios/subscription_processing.rs (1)
63-66: Remove unnecessary clone ofalice_keys.The
clone()call on line 63 is unnecessary sincealice_keysis already cloned on line 56 and not moved.- PublishSubscriptionUpdateTestCase::for_external_user(alice_keys.clone()) + PublishSubscriptionUpdateTestCase::for_external_user(alice_keys)src/nostr_manager/subscriptions.rs (1)
316-321: Consider making buffer time configurable.The 10-second buffer time is hardcoded. Consider making this configurable for flexibility in different deployment scenarios.
Based on learnings
+ const SUBSCRIPTION_BUFFER_SECS: u64 = 10; + async fn refresh_batch_subscription( &self, relay_url: RelayUrl, batch_id: usize, batch_users: Vec<PublicKey>, ) -> Result<()> { - let buffer_time = Timestamp::now() - Duration::from_secs(10); + let buffer_time = Timestamp::now() - Duration::from_secs(Self::SUBSCRIPTION_BUFFER_SECS);src/whitenoise/mod.rs (1)
252-263: Consider extracting the buffer constant for consistency.The 10-second buffer is used in multiple places. Consider defining it as a constant to ensure consistency across the codebase.
+ /// Default lookback buffer in seconds for subscription timestamps + const SUBSCRIPTION_BUFFER_SECS: u64 = 10; + async fn setup_global_users_subscriptions(whitenoise_ref: &'static Whitenoise) -> Result<()> { // ... existing code ... - // Compute shared since for global user subscriptions with 10s lookback buffer + // Compute shared since for global user subscriptions with lookback buffer let since = Self::compute_global_since_timestamp(whitenoise_ref).await?;And update line 288:
- const BUFFER_SECS: u64 = 10; + const BUFFER_SECS: u64 = Self::SUBSCRIPTION_BUFFER_SECS;src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (2)
162-172: Consider extracting the sleep duration to a constant.The 1-second sleep duration is hardcoded here and in other publish methods (
publish_metadataandpublish_relay_list). Consider extracting it to a constant for consistency and maintainability.+const PUBLISH_DELAY_SECS: u64 = 1; + async fn publish_follow_list_update( &self, test_client: &Client, follows: &[PublicKey], ) -> Result<(), WhitenoiseError> { - tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; + tokio::time::sleep(tokio::time::Duration::from_secs(PUBLISH_DELAY_SECS)).await; publish_follow_list(test_client, follows).await?; tracing::info!("✓ Follow list published via test client"); Ok(()) }Also apply the same constant to lines 115 and 133:
- tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; + tokio::time::sleep(tokio::time::Duration::from_secs(PUBLISH_DELAY_SECS)).await;
279-280: Consider using the same pattern as in the related code snippet.The collection of pubkeys from follows uses a different pattern than what's shown in the related code snippet (
src/whitenoise/accounts.rs:749). While both are correct, consider using the same pattern for consistency across the codebase.- let actual: HashSet<PublicKey> = follows.iter().map(|u| u.pubkey).collect(); + let actual: HashSet<PublicKey> = follows.iter().map(|f| f.pubkey).collect();Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/integration_tests/core/test_clients.rs(1 hunks)src/integration_tests/scenarios/subscription_processing.rs(1 hunks)src/integration_tests/test_cases/subscription_processing/mod.rs(1 hunks)src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs(8 hunks)src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/mod.rs(2 hunks)src/nostr_manager/query.rs(0 hunks)src/nostr_manager/subscriptions.rs(4 hunks)src/whitenoise/accounts.rs(5 hunks)src/whitenoise/database/accounts.rs(2 hunks)src/whitenoise/event_processor/account_event_processor.rs(1 hunks)src/whitenoise/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- src/nostr_manager/query.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/nostr_manager/mod.rssrc/integration_tests/scenarios/subscription_processing.rssrc/nostr_manager/subscriptions.rssrc/integration_tests/test_cases/subscription_processing/publish_subscription_update.rssrc/integration_tests/core/test_clients.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rssrc/whitenoise/accounts.rssrc/integration_tests/test_cases/subscription_processing/mod.rssrc/whitenoise/mod.rssrc/whitenoise/database/accounts.rssrc/whitenoise/event_processor/account_event_processor.rs
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Place MLS integration tests under src/integration_tests
Files:
src/integration_tests/scenarios/subscription_processing.rssrc/integration_tests/test_cases/subscription_processing/publish_subscription_update.rssrc/integration_tests/core/test_clients.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rssrc/integration_tests/test_cases/subscription_processing/mod.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-08-17T19:25:50.804Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/key_packages.rs:4-6
Timestamp: 2025-08-17T19:25:50.804Z
Learning: The `nostr_sdk::prelude::*` import includes `StreamExt` trait, so additional `futures_util::StreamExt` imports are not needed when using `.next()` on event streams in whitenoise codebase.
Applied to files:
src/nostr_manager/mod.rs
📚 Learning: 2025-09-04T03:38:54.953Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/nostr_manager/subscriptions.rs:72-89
Timestamp: 2025-09-04T03:38:54.953Z
Learning: In the global subscription batching system for Whitenoise (src/nostr_manager/subscriptions.rs), the current hash(pubkey) % batch_count approach causes batch ID reshuffling when user count changes, potentially leading to stale/duplicate author lists since only the triggering user's batch is refreshed. This is a known limitation documented in PR #340 that the team is aware of and will address in future iterations.
Applied to files:
src/nostr_manager/subscriptions.rssrc/whitenoise/mod.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/nostr_manager/subscriptions.rs
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/whitenoise/accounts.rssrc/whitenoise/mod.rssrc/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-08-11T10:23:59.406Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/relays.rs:260-278
Timestamp: 2025-08-11T10:23:59.406Z
Learning: In `src/whitenoise/accounts/relays.rs`, the `publish_relay_list_for_account` method is only called after the account setup is complete, which guarantees that `account.nip65_relays` is already populated and will never be empty. Therefore, no fallback logic is needed when `target_relays` is None.
Applied to files:
src/whitenoise/accounts.rs
🧬 Code graph analysis (6)
src/integration_tests/scenarios/subscription_processing.rs (2)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (2)
for_external_user(29-37)for_account(18-26)src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
for_account_follow_event(17-21)for_global_metadata_event(23-27)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (2)
src/whitenoise/accounts.rs (4)
follows(750-750)account(1028-1034)account(1126-1132)new(52-71)src/integration_tests/core/test_clients.rs (1)
publish_follow_list(68-81)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (2)
create_test_client(4-14)publish_follow_list(68-81)
src/whitenoise/mod.rs (1)
src/whitenoise/database/accounts.rs (1)
all(93-103)
src/whitenoise/database/accounts.rs (1)
src/whitenoise/mod.rs (1)
create_mock_whitenoise(494-553)
src/whitenoise/event_processor/account_event_processor.rs (1)
src/whitenoise/database/accounts.rs (1)
update_last_synced_max(450-474)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (18)
src/integration_tests/core/test_clients.rs (1)
68-81: LGTM: straightforward ContactList publisher for testsBuilds correct 'p' tags and publishes a replaceable ContactList. Looks good.
src/whitenoise/database/accounts.rs (2)
448-474: LGTM: atomic max-update helperThe CASE update correctly guards against downgrades and refreshes updated_at. Signature and bindings look right for SQLite.
1297-1404: LGTM: coverage for NULL, no‑downgrade, and advance pathsSolid tests validating semantics and side effects (updated_at refresh). Good guardrails.
src/nostr_manager/mod.rs (1)
216-231: Approve code changes Propagatingsincethroughsetup_account_subscriptionsinto each subscription filter is correctly implemented and applied.src/integration_tests/test_cases/subscription_processing/mod.rs (1)
1-5: LGTM!The new module declaration and re-export for
verify_last_synced_timestampfollow the established pattern in the codebase.src/integration_tests/scenarios/subscription_processing.rs (1)
68-81: LGTM! Follow-list publishing and timestamp verification tests are well-structured.The new test cases properly verify:
- Publishing a follow list for an account
- Verifying last_synced_timestamp for account follow events
- Verifying last_synced_timestamp for global metadata events
The tests follow the established builder pattern and provide good coverage for the new synchronization features.
src/nostr_manager/subscriptions.rs (2)
31-32: Effective implementation of optionalsincetimestamp filtering.The addition of the optional
since: Option<Timestamp>parameter is well-integrated throughout the subscription flow. The conditional filter application (lines 204-206) correctly adds thesincefilter only when provided, maintaining backward compatibility.Also applies to: 47-47, 76-77, 104-104, 196-196, 204-206
332-394: Consistent propagation ofsinceparameter in account subscriptions.The
sinceparameter is properly propagated through all account subscription methods including follow lists, giftwraps, and group messages, maintaining consistency across different subscription types.Also applies to: 396-462
src/whitenoise/accounts.rs (3)
73-79: Well-implemented timestamp computation with proper edge case handling.The
since_timestampmethod correctly:
- Returns None for unsynced accounts
- Applies a lookback buffer with saturation subtraction to prevent underflow
- Floors negative timestamps at 0 using
.max(0)
815-829: Good implementation of per-account timestamp computation with clear logging.The per-account
sincetimestamp computation is properly integrated with detailed debug logging that helps track synchronization state. The 10-second buffer provides a reasonable safety margin for event processing.
1279-1323: Comprehensive test coverage for timestamp edge cases.The three test cases effectively verify:
- None return for unsynced accounts
- Correct buffer application
- Proper floor at zero for edge cases near epoch
Excellent test coverage!
src/whitenoise/mod.rs (3)
231-231: Clean refactor replacing background sync with per-account subscriptions.The change from the removed
setup_accounts_sync_and_subscriptionstosetup_accounts_subscriptionscleanly separates concerns and removes the background sync logic as intended.
267-307: Well-designed global timestamp computation with proper fallback handling.The
compute_global_since_timestampmethod implements a robust strategy:
- Returns None if any account is unsynced (safe default)
- Otherwise uses the minimum last_synced_at with buffer
- Includes comprehensive logging for debugging
The approach ensures no events are missed during synchronization.
309-333: LGTM! Robust per-account subscription setup with error resilience.The method properly:
- Iterates through all accounts
- Sets up subscriptions individually
- Logs successes and warnings for failures
- Continues processing even if individual accounts fail
This resilient approach ensures partial failures don't block the entire system.
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (4)
51-55: LGTM! Follow list feature integration is well-structured.The builder method follows the established pattern and maintains consistency with other builder methods. The implementation correctly makes this an optional feature for test cases.
301-319: LGTM! Clear and informative update tracking.The refactored logic properly tracks all types of updates and provides clear error messaging when no updates are specified. The implementation is clean and maintainable.
264-291: whitenoise.follows API implementation verified
Method exists inwhitenoise/follows.rs, delegates toAccount::follows, and is covered by extensive database and integration tests.
352-355: Follow list events already advance last_synced_at
AccountEventProcessor invokes update_last_synced_max for Kind::ContactList, and the VerifyLastSyncedTimestampTestCase covers this flow.
jgmontoya
force-pushed
the
refactor/remove-background-sync
branch
from
31eca53 to
59b9285
Compare
There was a problem hiding this comment.
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)
src/nostr_manager/mod.rs (1)
357-363: Dropped errors from join_all hide relay-add failuresErrors returned by ensure_relay_in_client are ignored, which can mask setup failures and lead to surprising downstream behavior.
Apply to surface and propagate failures:
- let relay_futures = relay_urls - .iter() - .map(|relay_url| self.ensure_relay_in_client(relay_url)); - futures::future::join_all(relay_futures).await; + let relay_futures = relay_urls + .iter() + .map(|relay_url| self.ensure_relay_in_client(relay_url)); + let results = futures::future::join_all(relay_futures).await; + let mut first_err: Option<NostrManagerError> = None; + for res in results { + if let Err(e) = res { + tracing::warn!( + target: "whitenoise::nostr_manager::ensure_relays_connected", + "Failed to ensure relay: {}", e + ); + if first_err.is_none() { + first_err = Some(e); + } + } + } + if let Some(e) = first_err { + return Err(e); + }
🧹 Nitpick comments (14)
src/whitenoise/database/accounts.rs (3)
447-474: Return AccountNotFound when no row is updated (avoid silent no‑op).Currently this returns Ok even if the pubkey doesn’t exist. Align with find_by_pubkey behavior and surface errors early.
- sqlx::query( + let result = sqlx::query( "UPDATE accounts SET last_synced_at = CASE WHEN last_synced_at IS NULL OR last_synced_at < ? THEN ? ELSE last_synced_at END, updated_at = ? WHERE pubkey = ?", ) .bind(created_ms) .bind(created_ms) .bind(now_ms) .bind(pubkey.to_hex()) - .execute(&database.pool) - .await - .map_err(DatabaseError::Sqlx)?; + .execute(&database.pool) + .await + .map_err(DatabaseError::Sqlx)?; - Ok(()) + if result.rows_affected() == 0 { + return Err(WhitenoiseError::AccountNotFound); + } + Ok(())
447-474: Optional: simplify SQL with max() for clarity.Functionally equivalent and a bit shorter:
- SET last_synced_at = CASE - WHEN last_synced_at IS NULL OR last_synced_at < ? THEN ? - ELSE last_synced_at - END, + SET last_synced_at = max(last_synced_at, ?), updated_at = ?Keep the same binds order: (created_ms), (now_ms), (pubkey).
1297-1403: Make these tests DB‑only to avoid relay/network overhead.These three tests don’t need NostrManager/relays. Use the in‑memory pool from setup_test_db and wrap it in Database to speed up CI and reduce flake.
Sketch:
let pool = setup_test_db().await; let database = Database { pool: pool.clone(), path: std::path::PathBuf::from(":memory:"), last_connected: std::time::SystemTime::now(), }; // then use &database instead of &whitenoise.databaseI can send a patch converting all three tests if you want.
src/nostr_manager/mod.rs (1)
237-254: Group messages subscription ignores since; confirm intentThis path still passes None. If missed events are a concern for group messages too, consider threading since here; otherwise, confirm that replay is not desired for this stream.
src/whitenoise/accounts.rs (2)
815-830: Hard-coded 10s lookback; extract a constant to avoid drift across call sites10 is duplicated here and in NostrManager::update_account_subscriptions_with_signer. Extract a shared constant (e.g., SUBSCRIPTION_LOOKBACK_SECS) to keep behavior consistent.
73-79: Add unit test for future-skew last_synced_atTo lock in the clamp behavior from the proposed fix, add:
#[test] fn test_since_timestamp_clamps_future_last_synced_at() { let now = Utc::now(); let future = now + TimeDelta::seconds(120); let account = Account { id: None, pubkey: Keys::generate().public_key(), user_id: 1, last_synced_at: Some(future), created_at: now, updated_at: now, }; let ts = account.since_timestamp(10).unwrap(); let expected = (now.timestamp().max(0) as u64).saturating_sub(10); assert_eq!(ts.as_u64(), expected); }src/integration_tests/core/test_clients.rs (1)
72-76: Prefer concrete helper for 'p' tags (avoid relying on TagKind::p()).If available in your nostr-sdk version, use the typed helper to avoid fragile custom construction.
Apply this diff if Tag::public_key exists:
- let tags: Vec<Tag> = contacts - .iter() - .map(|pk| Tag::custom(TagKind::p(), [pk.to_hex()])) - .collect(); + let tags: Vec<Tag> = contacts.iter().copied().map(Tag::public_key).collect();src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (6)
5-5: Import order nit (optional).Reorder imports to: std, external crates, then crate modules. Tests work regardless.
51-55: Guard builder usage for accounts (optional).with_follow_list is documented as account-only; consider early validation to prevent misuse.
Example:
pub fn with_follow_list(mut self, follows: Vec<PublicKey>) -> Self { - self.follow_pubkeys = Some(follows); + if self.account_name.is_none() { + tracing::warn!("with_follow_list used for external user; verification will error"); + } + self.follow_pubkeys = Some(follows); self }
117-121: Don't panic in tests; propagate errors.unwrap() will abort the scenario on transient issues. Prefer ? to bubble up through TestCase.
- test_client - .send_event_builder(EventBuilder::metadata(metadata)) - .await - .unwrap(); + test_client + .send_event_builder(EventBuilder::metadata(metadata)) + .await?;
153-157: Same here: replace unwrap with ? for relay list publish.Keeps failure flow consistent with the rest of the test harness.
- test_client - .send_event_builder(EventBuilder::new(Kind::RelayList, "").tags(nip65_tags)) - .await - .unwrap(); + test_client + .send_event_builder(EventBuilder::new(Kind::RelayList, "").tags(nip65_tags)) + .await?;
228-237: Style: prefer match over if-let-else when both branches are non-empty.Aligns with repo guidelines for readability.
- let user = if let Some(account_name) = &self.account_name { - let account = context.get_account(account_name)?; - context - .whitenoise - .find_user_by_pubkey(&account.pubkey) - .await? - } else { - let pubkey = keys.public_key(); - context.whitenoise.find_user_by_pubkey(&pubkey).await? - }; + let user = match &self.account_name { + Some(account_name) => { + let account = context.get_account(account_name)?; + context.whitenoise.find_user_by_pubkey(&account.pubkey).await? + } + None => { + let pubkey = keys.public_key(); + context.whitenoise.find_user_by_pubkey(&pubkey).await? + } + };[As per coding guidelines]
357-359: Reduce flakiness by using retry instead of fixed sleep (optional).Replace fixed waits with retry-on-state helpers used elsewhere.
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (1)
1-6: Import order nit (optional).Order as std, external, then crate modules for consistency.
[As per coding guidelines]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/integration_tests/core/test_clients.rs(1 hunks)src/integration_tests/scenarios/subscription_processing.rs(1 hunks)src/integration_tests/test_cases/subscription_processing/mod.rs(1 hunks)src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs(8 hunks)src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/mod.rs(2 hunks)src/nostr_manager/query.rs(0 hunks)src/whitenoise/accounts.rs(5 hunks)src/whitenoise/database/accounts.rs(2 hunks)src/whitenoise/event_processor/account_event_processor.rs(1 hunks)src/whitenoise/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- src/nostr_manager/query.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/whitenoise/event_processor/account_event_processor.rs
- src/whitenoise/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Place MLS integration tests under src/integration_tests
Files:
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rssrc/integration_tests/core/test_clients.rssrc/integration_tests/test_cases/subscription_processing/mod.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rssrc/integration_tests/scenarios/subscription_processing.rs
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rssrc/integration_tests/core/test_clients.rssrc/integration_tests/test_cases/subscription_processing/mod.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rssrc/whitenoise/database/accounts.rssrc/nostr_manager/mod.rssrc/integration_tests/scenarios/subscription_processing.rssrc/whitenoise/accounts.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/nostr_manager/mod.rssrc/whitenoise/accounts.rs
📚 Learning: 2025-08-11T10:23:59.406Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/relays.rs:260-278
Timestamp: 2025-08-11T10:23:59.406Z
Learning: In `src/whitenoise/accounts/relays.rs`, the `publish_relay_list_for_account` method is only called after the account setup is complete, which guarantees that `account.nip65_relays` is already populated and will never be empty. Therefore, no fallback logic is needed when `target_relays` is None.
Applied to files:
src/whitenoise/accounts.rs
🧬 Code graph analysis (4)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (3)
src/integration_tests/core/traits.rs (4)
std(21-21)std(24-24)context(28-28)run(8-8)src/whitenoise/accounts.rs (4)
follows(750-750)account(1028-1034)account(1126-1132)new(52-71)src/integration_tests/core/test_clients.rs (1)
publish_follow_list(68-81)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (2)
create_test_client(4-14)publish_follow_list(68-81)
src/whitenoise/database/accounts.rs (1)
src/whitenoise/mod.rs (1)
create_mock_whitenoise(494-553)
src/integration_tests/scenarios/subscription_processing.rs (2)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (2)
for_external_user(29-37)for_account(18-26)src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
for_account_follow_event(17-21)for_global_metadata_event(23-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (macos-14, native)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (20)
src/whitenoise/database/accounts.rs (1)
447-474: Timestamp units are correct All callers—including the event processor’s conversion from seconds and all tests—pass millisecond timestamps. No changes needed.src/nostr_manager/mod.rs (2)
224-232: Forwarding since to setup_account_subscriptions: LGTMThe since value is correctly propagated.
209-218: All call sites updated to include thesinceparameter.src/whitenoise/accounts.rs (5)
835-845: Passing since into setup: LGTMCorrectly forwards the computed per-account since to the signer path.
927-927: Tests import TimeDelta: LGTMImport aligns with chrono’s current API; tests compile against recent 0.4.x.
1279-1291: Test (never synced): LGTMCovers the None case effectively.
1292-1307: Test (buffer applied): LGTMValidates buffer subtraction and flooring behavior.
1309-1324: Test (floor at zero): LGTMGood guard against underflow when near epoch.
src/integration_tests/test_cases/subscription_processing/mod.rs (1)
2-2: LGTM: module wiring and re-exports look good.New submodule and re-export are correctly added and scoped.
Also applies to: 5-5
src/integration_tests/core/test_clients.rs (1)
68-81: LGTM: follow-list publisher matches ContactList semantics.Builds correct tags and publishes Kind::ContactList; errors are propagated.
If your nostr-sdk offers EventBuilder::contact_list(...), consider it for clarity; otherwise current approach is fine.
src/integration_tests/scenarios/subscription_processing.rs (3)
63-66: LGTM: external user relay update.Passing cloned keys is correct for isolation.
68-73: LGTM: follow-list test step.Builder usage and parameter type match the new API.
74-81: LGTM: timestamp verification steps added.Sequencing both modes is appropriate and self-contained.
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (5)
13-14: LGTM: follows field added to builder state.Keeps builder API coherent; defaults remain None.
Also applies to: 24-25, 35-36
162-172: LGTM: follow-list publisher wrapper.Sleep + publish via shared helper is fine.
264-291: LGTM: follow verification via exact set equality.Clear diagnostics using HashSet differences; account-only guard is good.
352-355: LGTM: follows publication integrated.Only executed when provided; pairs with verification step.
297-319: Nice: user-friendly update summary.Clear logging for scenario traceability.
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
92-109: LGTM: account-scoped follow event publisher.Correctly uses account NSEC and publishes ContactList.
111-127: LGTM: global metadata publisher.Publishes from unrelated keys; aligns with the policy being tested.
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
Show resolved
Hide resolved
jgmontoya
force-pushed
the
refactor/remove-background-sync
branch
from
59b9285 to
2a97dd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/nostr_manager/mod.rs (1)
64-66: Good: single source of truth for future-skewDefining MAX_FUTURE_SKEW here clarifies ownership and enables consistent usage (e.g., in query filtering). Consider making it configurable via app config later if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/integration_tests/scenarios/subscription_processing.rs(1 hunks)src/integration_tests/test_cases/subscription_processing/mod.rs(1 hunks)src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/mod.rs(3 hunks)src/nostr_manager/query.rs(1 hunks)src/whitenoise/accounts.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/integration_tests/test_cases/subscription_processing/mod.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/nostr_manager/mod.rssrc/whitenoise/accounts.rssrc/integration_tests/scenarios/subscription_processing.rssrc/nostr_manager/query.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Place MLS integration tests under src/integration_tests
Files:
src/integration_tests/scenarios/subscription_processing.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/nostr_manager/mod.rssrc/whitenoise/accounts.rs
📚 Learning: 2025-08-11T10:23:59.406Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/relays.rs:260-278
Timestamp: 2025-08-11T10:23:59.406Z
Learning: In `src/whitenoise/accounts/relays.rs`, the `publish_relay_list_for_account` method is only called after the account setup is complete, which guarantees that `account.nip65_relays` is already populated and will never be empty. Therefore, no fallback logic is needed when `target_relays` is None.
Applied to files:
src/whitenoise/accounts.rs
🧬 Code graph analysis (2)
src/integration_tests/scenarios/subscription_processing.rs (2)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (2)
for_external_user(29-37)for_account(18-26)src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
for_account_follow_event(17-21)for_global_metadata_event(23-27)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (2)
create_test_client(4-14)publish_follow_list(68-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (ubuntu-latest, native)
- GitHub Check: check (macos-latest, native)
- GitHub Check: check (macos-14, native)
🔇 Additional comments (10)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (1)
63-90: Ensurelast_synced_atstays stable across the whole observation window
retryreturns success on the first unchanged read, so a later update within the same window still passes. The previous review already flagged this and it’s still unresolved. Please poll until the full window elapses before declaring success.- retry( - 50, - Duration::from_millis(50), - || async { - let after = context - .whitenoise - .find_account_by_pubkey(&pubkey) - .await? - .last_synced_at; - if after == before { - Ok(()) - } else { - Err(WhitenoiseError::Other(anyhow::anyhow!( - "last_synced_at advanced on global-only event" - ))) - } - }, - description, - ) - .await + let deadline = + tokio::time::Instant::now() + tokio::time::Duration::from_millis(50 * 50); + loop { + let after = context + .whitenoise + .find_account_by_pubkey(&pubkey) + .await? + .last_synced_at; + if after != before { + return Err(WhitenoiseError::Other(anyhow::anyhow!( + "last_synced_at advanced on global-only event" + ))); + } + if tokio::time::Instant::now() >= deadline { + return Ok(()); + } + tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; + }src/integration_tests/scenarios/subscription_processing.rs (1)
68-80: Follow-event setup keeps the new timestamp checks meaningfulPublishing the follow list right before exercising both
VerifyLastSyncedTimestampTestCasevariants sets up concrete follow/global metadata activity for the assertions to inspect. The flow from event creation to timestamp verification is consistent with the rest of this scenario.src/nostr_manager/query.rs (1)
6-6: Centralize MAX_FUTURE_SKEW import looks goodImporting MAX_FUTURE_SKEW from the nostr_manager module removes duplication and keeps skew policy consistent across query paths.
src/nostr_manager/mod.rs (1)
219-221: Propagating optional since is correct; ensure downstream filters apply itThreading since into setup_account_subscriptions_with_signer and forwarding it to setup_account_subscriptions is the right move. Please verify subscriptions.rs uses the provided since for all relevant filters (user, inbox, group) to avoid unintentionally full backfills.
Also applies to: 233-234
src/whitenoise/accounts.rs (6)
820-835: LGTM: compute-and-log per-account since with 10s bufferUsing since_timestamp(10) and logging the outcome will aid diagnostics.
932-932: No review needed for import tweakImporting TimeDelta for tests is fine.
1284-1296: LGTM: None when never syncedTest accurately validates the None case.
1297-1313: LGTM: buffer is appliedTest correctly asserts lookback buffering.
1314-1329: LGTM: floors at zero under buffer underflowTest covers epoch boundary behavior.
73-85: Bug: since can be set in the future, causing missed eventsClamping last_synced_at to now + MAX_FUTURE_SKEW can push since into the future. Subscriptions starting “in the future” will miss events until time catches up. Clamp to now (epoch seconds) before applying the buffer.
Apply:
pub(crate) fn since_timestamp(&self, buffer_secs: u64) -> Option<nostr_sdk::Timestamp> { let ts = self.last_synced_at?; - // Clamp to now + skew window, then apply buffer - let now_secs = (Utc::now().timestamp().max(0) as u64) - .saturating_add(crate::nostr_manager::MAX_FUTURE_SKEW.as_secs()); - let last_secs = (ts.timestamp().max(0) as u64).min(now_secs); + // Clamp to now, then apply buffer + let now_secs = Utc::now().timestamp().max(0) as u64; + let last_secs = (ts.timestamp().max(0) as u64).min(now_secs); let secs = last_secs.saturating_sub(buffer_secs); Some(nostr_sdk::Timestamp::from(secs)) }This aligns with prior guidance to prevent empty subscriptions when last_synced_at is skewed into the future. Based on learnings
jgmontoya
force-pushed
the
refactor/remove-background-sync
branch
4 times, most recently
from
3275d2f to
9742e79
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/whitenoise/accounts.rs (1)
819-834: Extract buffer “10s” to a named constant and use it in logsAvoid magic numbers and keep logs accurate if the buffer changes.
Apply:
- // Compute per-account since with a 10s lookback buffer when available - let since = account.since_timestamp(10); + // Compute per-account since with a lookback buffer when available + let since = account.since_timestamp(ACCOUNT_SINCE_LOOKBACK_SECS); match since { - Some(ts) => tracing::debug!( + Some(ts) => tracing::debug!( target: "whitenoise::setup_subscriptions", - "Computed per-account since={}s (10s buffer) for {}", - ts.as_u64(), - account.pubkey.to_hex() + "Computed per-account since={}s ({}s buffer) for {}", + ts.as_u64(), + ACCOUNT_SINCE_LOOKBACK_SECS, + account.pubkey.to_hex() ), None => tracing::debug!( target: "whitenoise::setup_subscriptions", "Computed per-account since=None (unsynced) for {}", account.pubkey.to_hex() ), }Add once near the top of this module (outside the shown range):
const ACCOUNT_SINCE_LOOKBACK_SECS: u64 = 10;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/whitenoise/accounts.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/whitenoise/accounts.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/whitenoise/accounts.rs
📚 Learning: 2025-08-11T10:23:59.406Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/relays.rs:260-278
Timestamp: 2025-08-11T10:23:59.406Z
Learning: In `src/whitenoise/accounts/relays.rs`, the `publish_relay_list_for_account` method is only called after the account setup is complete, which guarantees that `account.nip65_relays` is already populated and will never be empty. Therefore, no fallback logic is needed when `target_relays` is None.
Applied to files:
src/whitenoise/accounts.rs
🔇 Additional comments (7)
src/whitenoise/accounts.rs (7)
73-83: since_timestamp: Correct clamp and flooring behaviorLogic is sound: clamps future to now and uses saturating_sub to avoid underflow. Matches past guidance and accompanying tests validate edge cases.
931-931: Tests import: OKImporting Utc locally in tests is appropriate; it’s not re-exported from the parent module.
1283-1294: Test: None when never syncedCovers the unsynced case cleanly. LGTM.
1296-1311: Test: Buffer applicationAccurate expectation (uses max(0) + saturating_sub). LGTM.
1313-1327: Test: Floors at zeroGood guard against underflow. LGTM.
1329-1355: Test: Future clamped to nowBounding with before/after avoids flakiness. LGTM.
846-847: NostrManager API changes verified
setup_account_subscriptions_with_signer signature acceptssince: Option<Timestamp>and all calls include it; update_account_subscriptions_with_signer omitssinceas intended and its call site matches.
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
One small question but otherwise LGTM
| // Advance account sync timestamp only for account-scoped events | ||
| match event.kind { | ||
| Kind::ContactList | Kind::GiftWrap | Kind::MlsGroupMessage => { | ||
| let created_ms = (event.created_at.as_u64() as i64) * 1000; |
There was a problem hiding this comment.
what happens if we receive an event that has a far future timestamp. I don't know if we automatically reject those already but I can see bad events causing problems here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/whitenoise/event_processor/mod.rs (1)
200-225: Tie the test to the shared skew constantHard-coding a 2 h skew makes the test brittle if
MAX_FUTURE_SKEWchanges. Import the constant and go just one second past it so the test always exercises the intended boundary.- use crate::nostr_manager::utils::is_event_timestamp_valid; + use crate::nostr_manager::utils::{is_event_timestamp_valid, MAX_FUTURE_SKEW}; use nostr_sdk::prelude::*; use std::time::Duration as StdDuration; // Create test keys let keys = Keys::generate(); // Create an event with a timestamp far in the future (exceeds MAX_FUTURE_SKEW of 1 hour) - let far_future = Timestamp::now() + StdDuration::from_secs(7200); // 2 hours in future + let far_future = Timestamp::now() + (MAX_FUTURE_SKEW + StdDuration::from_secs(1));src/nostr_manager/subscriptions.rs (2)
27-34: Use a where-clause bound instead of impl Trait for signer (per repo guidelines).Switch the parameter to a generic with bounds in a where clause.
As per coding guidelines
- pub(crate) async fn setup_batched_relay_subscriptions_with_signer( + pub(crate) async fn setup_batched_relay_subscriptions_with_signer<S>( &self, users_with_relays: Vec<(PublicKey, Vec<RelayUrl>)>, default_relays: &[RelayUrl], - signer: impl NostrSigner + 'static, + signer: S, since: Option<Timestamp>, - ) -> Result<()> { + ) -> Result<()> + where + S: NostrSigner + 'static, + {
193-216: Avoid per-batch ensure_relays_connected to the same relay.subscribe_user_batch ensures the relay on every batch, causing redundant calls for the same relay. Hoist one ensure per relay in create_deterministic_batches_for_relay, or cache connectivity, to reduce overhead. Keep this call here if refresh paths depend on it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/query.rs(2 hunks)src/nostr_manager/subscriptions.rs(7 hunks)src/nostr_manager/utils.rs(2 hunks)src/whitenoise/event_processor/account_event_processor.rs(3 hunks)src/whitenoise/event_processor/mod.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/nostr_manager/query.rssrc/whitenoise/event_processor/mod.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rssrc/nostr_manager/subscriptions.rssrc/nostr_manager/utils.rssrc/whitenoise/event_processor/account_event_processor.rs
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Place MLS integration tests under src/integration_tests
Files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-04T03:38:54.953Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/nostr_manager/subscriptions.rs:72-89
Timestamp: 2025-09-04T03:38:54.953Z
Learning: In the global subscription batching system for Whitenoise (src/nostr_manager/subscriptions.rs), the current hash(pubkey) % batch_count approach causes batch ID reshuffling when user count changes, potentially leading to stale/duplicate author lists since only the triggering user's batch is refreshed. This is a known limitation documented in PR #340 that the team is aware of and will address in future iterations.
Applied to files:
src/nostr_manager/subscriptions.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/nostr_manager/subscriptions.rs
🧬 Code graph analysis (5)
src/nostr_manager/query.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/whitenoise/event_processor/mod.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (4)
src/whitenoise/accounts.rs (3)
account(1032-1038)account(1130-1136)new(52-71)src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (1)
create_test_client(4-14)src/nostr_manager/query.rs (1)
event(197-202)
src/nostr_manager/subscriptions.rs (1)
src/nostr_manager/utils.rs (1)
adjust_since_for_giftwrap(22-24)
src/whitenoise/event_processor/account_event_processor.rs (2)
src/nostr_manager/utils.rs (1)
cap_timestamp_to_now(27-34)src/whitenoise/database/accounts.rs (1)
update_last_synced_max(450-474)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (7)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (1)
63-85: Keep sampling until the observation window ends.
retryreturnsOkon the first unchanged read, so the test can succeed even iflast_synced_atadvances a moment later. That reintroduces the false negatives we discussed earlier; please watch the full window before declaring success.- async fn assert_unchanged( - &self, - context: &mut ScenarioContext, - pubkey: PublicKey, - before: Option<chrono::DateTime<chrono::Utc>>, - description: &str, - ) -> Result<(), WhitenoiseError> { - retry( - 50, - Duration::from_millis(50), - || async { - let account = context.whitenoise.find_account_by_pubkey(&pubkey).await?; - if account.last_synced_at == before { - Ok(()) - } else { - Err(WhitenoiseError::Other(anyhow::anyhow!( - "last_synced_at advanced on global-only event" - ))) - } - }, - description, - ) - .await + async fn assert_unchanged( + &self, + context: &mut ScenarioContext, + pubkey: PublicKey, + before: Option<chrono::DateTime<chrono::Utc>>, + _description: &str, + ) -> Result<(), WhitenoiseError> { + let delay = tokio::time::Duration::from_millis(50); + let deadline = tokio::time::Instant::now() + delay * 50; + loop { + let account = context.whitenoise.find_account_by_pubkey(&pubkey).await?; + if account.last_synced_at != before { + return Err(WhitenoiseError::Other(anyhow::anyhow!( + "last_synced_at advanced on global-only event" + ))); + } + if tokio::time::Instant::now() >= deadline { + return Ok(()); + } + tokio::time::sleep(delay).await; + } }src/nostr_manager/subscriptions.rs (3)
49-50: LGTM: since propagation through batch creation and subscription.Plumbs since through all layers and conditionally applies filter.since as expected.
Also applies to: 74-79, 101-107
318-325: LGTM: refresh buffer to avoid gaps.Using now - 10s as since during refresh is a sensible guard for transient delivery delays.
411-419: LGTM: giftwrap lookback applied only when since is present.The selective adjustment plus comment explaining NIP‑59 backdating looks good. Underflow handling is addressed in utils comment.
src/nostr_manager/utils.rs (3)
14-18: LGTM: future-skew guard is simple and clear.Checks created_at against now + MAX_FUTURE_SKEW.
26-34: LGTM: cap_timestamp_to_now correctly clamps future values.Straightforward clamp with tolerant tests.
545-553: LGTM: constants validated by tests.Values and intent are covered.
jgmontoya
force-pushed
the
refactor/remove-background-sync
branch
from
37030f6 to
f56ff38
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/whitenoise/event_processor/mod.rs (2)
61-71: Drop invalid future events early; consider escalating log/adding metric for extreme skewNice guard. Optionally, warn when the skew is egregious (e.g., > 1 day) and keep debug otherwise. This helps surface relay clock issues while avoiding noise.
Apply this minimal tweak:
- tracing::debug!( - target: "whitenoise::event_processor::process_events", - "Skipping event {} with invalid future timestamp: {}", - event.id.to_hex(), - event.created_at - ); + let now = Timestamp::now(); + let skew_secs = event.created_at.as_u64().saturating_sub(now.as_u64()); + if skew_secs > 86_400 { + tracing::warn!( + target: "whitenoise::event_processor::process_events", + "Skipping event {} with invalid future timestamp: {} (skew_secs={})", + event.id.to_hex(), + event.created_at, + skew_secs + ); + } else { + tracing::debug!( + target: "whitenoise::event_processor::process_events", + "Skipping event {} with invalid future timestamp: {} (skew_secs={})", + event.id.to_hex(), + event.created_at, + skew_secs + ); + }
200-226: Make test resilient to future changes of MAX_FUTURE_SKEWHardcoding “2 hours” will break if MAX_FUTURE_SKEW changes. Use the constant and add a small delta.
Apply:
- use crate::nostr_manager::utils::is_event_timestamp_valid; + use crate::nostr_manager::utils::{is_event_timestamp_valid, MAX_FUTURE_SKEW}; @@ - // Create an event with a timestamp far in the future (exceeds MAX_FUTURE_SKEW of 1 hour) - let far_future = Timestamp::now() + StdDuration::from_secs(7200); // 2 hours in future + // Create an event exceeding MAX_FUTURE_SKEW + let far_future = Timestamp::now() + MAX_FUTURE_SKEW + StdDuration::from_secs(60);src/whitenoise/event_processor/account_event_processor.rs (3)
90-121: Deduplicate advancement logic via a small helperThe ContactList/MlsGroupMessage advancement path duplicates the GiftWrap path logic (cap -> to ms -> update -> log). Extracting a helper reduces repetition and keeps logging consistent.
Apply this change (replace block body with a helper call):
Kind::ContactList | Kind::MlsGroupMessage => { - // Cap timestamp to prevent future corruption - let safe_timestamp = cap_timestamp_to_now(event.created_at); - let created_ms = (safe_timestamp.as_u64() as i64) * 1000; - - if let Err(e) = Account::update_last_synced_max( - &account.pubkey, - created_ms, - &self.database, - ) - .await - { - tracing::warn!( - target: "whitenoise::event_processor::process_account_event", - "Failed to advance last_synced_at for {} on event {}: {}", - account.pubkey.to_hex(), - event.id.to_hex(), - e - ); - } else { - tracing::debug!( - target: "whitenoise::event_processor::process_account_event", - "Updated last_synced_at (if older) for {} with candidate {} (ms) [capped from original {}]", - account.pubkey.to_hex(), - created_ms, - event.created_at.as_u64() * 1000 - ); - } + let safe_timestamp = cap_timestamp_to_now(event.created_at); + self.advance_last_synced_for_account(&account, safe_timestamp, event.created_at, Some(event.id)) + .await; }Add this helper inside impl Whitenoise (outside this hunk):
async fn advance_last_synced_for_account( &self, account: &Account, safe_timestamp: Timestamp, original_timestamp: Timestamp, event_id: Option<EventId>, ) { let created_ms = (safe_timestamp.as_u64() as i64) * 1000; if let Err(e) = Account::update_last_synced_max(&account.pubkey, created_ms, &self.database).await { tracing::warn!( target: "whitenoise::event_processor::process_account_event", "Failed to advance last_synced_at for {} on event {}: {}", account.pubkey.to_hex(), event_id.map(|id| id.to_hex()).unwrap_or_else(|| "<none>".into()), e ); } else { tracing::debug!( target: "whitenoise::event_processor::process_account_event", "Updated last_synced_at (if older) for {} with candidate {} (ms) [capped from original {}] (event={})", account.pubkey.to_hex(), created_ms, original_timestamp.as_u64() * 1000, event_id.map(|id| id.to_hex()).unwrap_or_else(|| "<none>".into()), ); } }
127-152: Use the same helper in GiftWrap pathKeeps behavior consistent and reduces duplication.
Apply:
Ok(Some(rumor_timestamp)) => { let safe_timestamp = cap_timestamp_to_now(rumor_timestamp); - let created_ms = (safe_timestamp.as_u64() as i64) * 1000; - - if let Err(e) = Account::update_last_synced_max( - &account.pubkey, - created_ms, - &self.database, - ) - .await - { - tracing::warn!( - target: "whitenoise::event_processor::process_account_event", - "Failed to advance last_synced_at with rumor timestamp for {}: {}", - account.pubkey.to_hex(), - e - ); - } else { - tracing::debug!( - target: "whitenoise::event_processor::process_account_event", - "Updated last_synced_at (if older) for {} with rumor timestamp {} (ms) [capped from original {}]", - account.pubkey.to_hex(), - created_ms, - rumor_timestamp.as_u64() * 1000 - ); - } + self.advance_last_synced_for_account(&account, safe_timestamp, rumor_timestamp, Some(event.id)) + .await; }
154-159: Include event id and account in “skip/extract failed” logs for easier triageAdds essential context without increasing verbosity elsewhere.
Apply:
- tracing::debug!( + tracing::debug!( target: "whitenoise::event_processor::process_account_event", - "Skipping timestamp advancement for giftwrap with failed rumor extraction" + "Skipping timestamp advancement for giftwrap (event={}, account={}) due to failed rumor extraction", + event.id.to_hex(), + account.pubkey.to_hex() ); @@ - tracing::warn!( + tracing::warn!( target: "whitenoise::event_processor::process_account_event", - "Failed to extract rumor timestamp for {}: {}", - account.pubkey.to_hex(), + "Failed to extract rumor timestamp for account={} event={}: {}", + account.pubkey.to_hex(), + event.id.to_hex(), e );Also applies to: 161-167
src/nostr_manager/utils.rs (2)
410-421: Consider improving test timing reliability.The timestamp validation tests could be sensitive to execution timing, particularly the boundary test at line 476 and the test at lines 528-534 which allows only 1 second difference. Consider using a fixed reference time or mocking
Timestamp::now()for more deterministic tests.Also applies to: 423-435, 437-449, 451-463, 465-477
497-511: Add tests for near-epoch timestamps to guarantee saturating_sub behavior.
Current tests cover only a 30-day-old timestamp; add cases forTimestamp::from(0)and values just below, equal to, and just aboveGIFTWRAP_LOOKBACK_BUFFERto verify the result saturates at zero.src/nostr_manager/subscriptions.rs (1)
206-208: Align batched subscriptions with giftwrap lookback
Insubscribe_user_batch(src/nostr_manager/subscriptions.rs:206–208), all non-giftwrap kinds usefilter.since(since)directly; if metadata events ever arrive delayed or out-of-order, wrapsincewithadjust_since_for_giftwrap(or a similar buffer) to avoid missing updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/query.rs(2 hunks)src/nostr_manager/subscriptions.rs(7 hunks)src/nostr_manager/utils.rs(2 hunks)src/whitenoise/event_processor/account_event_processor.rs(3 hunks)src/whitenoise/event_processor/mod.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/whitenoise/event_processor/mod.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rssrc/whitenoise/event_processor/account_event_processor.rssrc/nostr_manager/query.rssrc/nostr_manager/utils.rssrc/nostr_manager/subscriptions.rs
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Place MLS integration tests under src/integration_tests
Files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-04T03:38:54.953Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/nostr_manager/subscriptions.rs:72-89
Timestamp: 2025-09-04T03:38:54.953Z
Learning: In the global subscription batching system for Whitenoise (src/nostr_manager/subscriptions.rs), the current hash(pubkey) % batch_count approach causes batch ID reshuffling when user count changes, potentially leading to stale/duplicate author lists since only the triggering user's batch is refreshed. This is a known limitation documented in PR #340 that the team is aware of and will address in future iterations.
Applied to files:
src/nostr_manager/subscriptions.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/nostr_manager/subscriptions.rs
🧬 Code graph analysis (5)
src/whitenoise/event_processor/mod.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (3)
src/whitenoise/accounts.rs (3)
account(1032-1038)account(1130-1136)new(52-71)src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (1)
create_test_client(4-14)
src/whitenoise/event_processor/account_event_processor.rs (2)
src/nostr_manager/utils.rs (1)
cap_timestamp_to_now(32-39)src/whitenoise/database/accounts.rs (1)
update_last_synced_max(450-474)
src/nostr_manager/query.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/nostr_manager/subscriptions.rs (1)
src/nostr_manager/utils.rs (1)
adjust_since_for_giftwrap(22-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (10)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (1)
63-86: Ensureassert_unchangedobserves the full stability window.The current retry exits after the first unchanged read, so the test can pass even if
last_synced_atadvances later within the same window. Please hold the entire window before succeeding.- retry( - 50, - Duration::from_millis(50), - || async { - let account = context.whitenoise.find_account_by_pubkey(&pubkey).await?; - if account.last_synced_at == before { - Ok(()) - } else { - Err(WhitenoiseError::Other(anyhow::anyhow!( - "last_synced_at advanced on global-only event" - ))) - } - }, - description, - ) - .await + let _ = description; + for attempt in 0..50 { + let account = context.whitenoise.find_account_by_pubkey(&pubkey).await?; + if account.last_synced_at != before { + return Err(WhitenoiseError::Other(anyhow::anyhow!( + "last_synced_at advanced on global-only event" + ))); + } + if attempt < 49 { + tokio::time::sleep(Duration::from_millis(50)).await; + } + } + Ok(())src/whitenoise/event_processor/mod.rs (1)
5-5: LGTM: wired timestamp validation importImport placement and usage look correct.
src/nostr_manager/query.rs (1)
54-55: LGTM: future-skew filter before selecting latestFiltering with is_event_timestamp_valid simplifies and centralizes the cutoff policy. The tie-breaker on (created_at, id) keeps determinism.
src/whitenoise/event_processor/account_event_processor.rs (2)
5-5: LGTM: using cap_timestamp_to_now to avoid future corruptionImport and usage align with the new timestamp-safety utilities.
112-118: LGTM: corrected log semantics about advancing last_synced_atThanks for clarifying that the update is conditional. This addresses the earlier misleading “Advanced …” message.
Also applies to: 145-151
src/nostr_manager/utils.rs (2)
22-29: **** Fix potential underflow when adjusting giftwrap since.If since is near epoch (e.g., 0), subtracting 7 days can underflow. The current implementation uses
saturating_subwhich correctly prevents underflow by clamping at 0.
32-39: LGTM! Clean implementation of timestamp capping.The function correctly caps future timestamps to the current time while preserving past timestamps unchanged.
src/nostr_manager/subscriptions.rs (3)
318-324: LGTM! Good use of buffer time for refresh operations.The 10-second buffer appropriately handles potential timing discrepancies during subscription refresh.
412-419: LGTM! Proper application of giftwrap lookback adjustment.The code correctly uses the utility function to handle NIP-59 backdated timestamps with clear documentation.
519-542: LGTM! Comprehensive test coverage for giftwrap lookback.The test thoroughly validates the lookback buffer behavior including edge cases.
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
Outdated
Show resolved
Hide resolved
jgmontoya
force-pushed
the
refactor/remove-background-sync
branch
from
f56ff38 to
63e65c4
Compare
…lper on account-scoped events
…ublishSubscriptionUpdateTestCase
…t_synced timestamp update behavior
jgmontoya
force-pushed
the
refactor/remove-background-sync
branch
from
63e65c4 to
9cb9f1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/nostr_manager/subscriptions.rs (5)
27-35: Threading since through batched subscription setup is correct; consider moving signer bounds to a where clause.Style-only: our Rust guideline prefers where clauses over inline impl Trait bounds in fn signatures.
Apply this refactor to the signature:
- pub(crate) async fn setup_batched_relay_subscriptions_with_signer( - &self, - users_with_relays: Vec<(PublicKey, Vec<RelayUrl>)>, - default_relays: &[RelayUrl], - signer: impl NostrSigner + 'static, - since: Option<Timestamp>, - ) -> Result<()> { + pub(crate) async fn setup_batched_relay_subscriptions_with_signer<S>( + &self, + users_with_relays: Vec<(PublicKey, Vec<RelayUrl>)>, + default_relays: &[RelayUrl], + signer: S, + since: Option<Timestamp>, + ) -> Result<()> + where + S: NostrSigner + 'static, + {Also optional: defensively normalize since (e.g., cap to “now + skew”) before passing downstream if you have a helper (cap_timestamp_to_now / MAX_FUTURE_SKEW). This prevents accidental future filters from suppressing events.
Also applies to: 49-52
74-80: End-to-end propagation of since into batches is sound.No correctness issues spotted. Consider logging the effective since used per batch for observability during incident triage.
Also applies to: 98-111
413-418: Simplify redundant Option plumbing around giftwrap since adjustment.Given the outer if let Some(since), adjust_since_for_giftwrap(Some(since)) always returns Some; the extra unwrap-guard is redundant.
Apply this simplification:
- if let Some(since) = since { - // Account for NIP-59 backdated timestamps - giftwrap events may be timestamped - // in the past for privacy, so we look back further than last_synced_at - let adjusted_since = adjust_since_for_giftwrap(Some(since)); - if let Some(adjusted) = adjusted_since { - giftwrap_filter = giftwrap_filter.since(adjusted); - } - } + if let Some(since) = since { + // NIP-59 backdated timestamps: extend lookback for privacy-preserving giftwraps + if let Some(adjusted) = adjust_since_for_giftwrap(Some(since)) { + giftwrap_filter = giftwrap_filter.since(adjusted); + } + }
139-155: Known batching reshuffle caveat still applies.Using hash(pubkey) % batch_count can reshuffle batch IDs when user_count changes; only the triggering user’s batch is refreshed elsewhere, which may leave stale batches. Not a blocker here, just noting interaction with the new since flow. Consider refreshing all batches when batch_count changes, or use a stable partitioning function. Based on learnings
518-542: Good unit test for the giftwrap lookback; consider a fully deterministic timestamp.Using Timestamp::now() is fine, but a fixed epoch value makes the test immune to edge timing issues.
Example:
- let original_timestamp = Timestamp::now(); + let original_timestamp = Timestamp::from(1_700_000_000);src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
40-46: Narrow mutability: take &ScenarioContext (not &mut) in helper.These helpers don’t mutate context; using & simplifies borrowing.
- async fn assert_advanced( - &self, - context: &mut ScenarioContext, + async fn assert_advanced( + &self, + context: &ScenarioContext, pubkey: PublicKey, before: Option<chrono::DateTime<chrono::Utc>>, description: &str, ) -> Result<(), WhitenoiseError> {Note: Call sites need no change; &mut coerces to &.
109-113: Unify Duration path for clarity.You import std::time::Duration; use it consistently instead of tokio::time::Duration.
- tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; + tokio::time::sleep(Duration::from_secs(1)).await;- tokio::time::sleep(tokio::time::Duration::from_millis(600)).await; + tokio::time::sleep(Duration::from_millis(600)).await;Also applies to: 132-136
src/nostr_manager/query.rs (1)
5-8: Prefer module-relative imports for siblings (super::…)Use module-relative paths for items within the same parent module, and keep crate-level for cross-module items. This aligns with our style and remains resilient if the crate name changes.
-use crate::{ - nostr_manager::{utils::is_event_timestamp_valid, NostrManager, Result}, - RelayType, -}; +use super::{utils::is_event_timestamp_valid, NostrManager, Result}; +use crate::RelayType;As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/query.rs(2 hunks)src/nostr_manager/subscriptions.rs(7 hunks)src/nostr_manager/utils.rs(2 hunks)src/whitenoise/event_processor/account_event_processor.rs(3 hunks)src/whitenoise/event_processor/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/whitenoise/event_processor/mod.rs
- src/nostr_manager/utils.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/nostr_manager/query.rssrc/nostr_manager/subscriptions.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rssrc/whitenoise/event_processor/account_event_processor.rs
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Add and maintain MLS integration tests under src/integration_tests
Files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-04T03:38:54.953Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/nostr_manager/subscriptions.rs:72-89
Timestamp: 2025-09-04T03:38:54.953Z
Learning: In the global subscription batching system for Whitenoise (src/nostr_manager/subscriptions.rs), the current hash(pubkey) % batch_count approach causes batch ID reshuffling when user count changes, potentially leading to stale/duplicate author lists since only the triggering user's batch is refreshed. This is a known limitation documented in PR #340 that the team is aware of and will address in future iterations.
Applied to files:
src/nostr_manager/subscriptions.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/nostr_manager/subscriptions.rs
📚 Learning: 2025-09-18T11:21:01.705Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/rust-code-style.mdc:0-0
Timestamp: 2025-09-18T11:21:01.705Z
Learning: Applies to **/*.rs : Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
Applied to files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Ensure post-compromise security through correct epoch transitions and secret updates
Applied to files:
src/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Maintain message ordering and group state consistency across epochs
Applied to files:
src/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/whitenoise/event_processor/account_event_processor.rs
🧬 Code graph analysis (4)
src/nostr_manager/query.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/nostr_manager/subscriptions.rs (1)
src/nostr_manager/utils.rs (1)
adjust_since_for_giftwrap(22-29)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (3)
src/whitenoise/accounts.rs (3)
account(1032-1038)account(1130-1136)new(52-71)src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (1)
create_test_client(4-14)
src/whitenoise/event_processor/account_event_processor.rs (2)
src/nostr_manager/utils.rs (1)
cap_timestamp_to_now(32-39)src/whitenoise/database/accounts.rs (1)
update_last_synced_max(450-474)
🔇 Additional comments (7)
src/nostr_manager/subscriptions.rs (1)
12-14: Imports ordering and scope look good.Internal crate imports follow external crates per guidelines. No action needed.
src/whitenoise/event_processor/account_event_processor.rs (3)
5-5: LGTM!The import of
cap_timestamp_to_nowis correctly placed and follows the coding guidelines for import ordering.
89-171: Well-implemented timestamp advancement with proper safeguards.The timestamp advancement logic addresses several important concerns:
- Future timestamp protection: Uses
cap_timestamp_to_nowto prevent corruption from events with future timestamps (addressing the past review concern)- Event-specific handling: Appropriately handles different event kinds with ContactList/MlsGroupMessage using direct timestamps while GiftWrap uses rumor timestamps per NIP-59
- Non-blocking error handling: Failures in timestamp advancement don't interrupt the main processing flow
- Accurate logging: The log messages correctly indicate "if older" qualification after addressing past review feedback
The implementation follows good practices for defensive programming and maintains system resilience.
332-346: Clean helper function with appropriate error handling.The
extract_rumor_timestamp_for_advancementhelper follows good design principles:
- Single responsibility: Focused solely on timestamp extraction for sync advancement
- Graceful error handling: Converts extraction failures to
Ok(None)rather than propagating errors, which is appropriate since timestamp advancement is non-critical- Clear return semantics:
Option<Timestamp>wrapped inResultclearly indicates success/failure statesThe error swallowing strategy is reasonable here since timestamp advancement failures shouldn't block event processing.
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
1-8: LGTM: import ordering matches repo convention.std, then external crates, then crate::… — looks good. As per coding guidelines.
65-88: Fix: assert_unchanged can pass too early; enforce stability across the window.Current logic succeeds on the first equal read, risking false negatives. Hold equality for the full timeout window instead.
Apply this diff:
- async fn assert_unchanged( - &self, - context: &mut ScenarioContext, - pubkey: PublicKey, - before: Option<chrono::DateTime<chrono::Utc>>, - description: &str, - ) -> Result<(), WhitenoiseError> { - retry( - 50, - Duration::from_millis(50), - || async { - let account = context.whitenoise.find_account_by_pubkey(&pubkey).await?; - if account.last_synced_at == before { - Ok(()) - } else { - Err(WhitenoiseError::Other(anyhow::anyhow!( - "last_synced_at advanced on global-only event" - ))) - } - }, - description, - ) - .await - } + async fn assert_unchanged( + &self, + context: &ScenarioContext, + pubkey: PublicKey, + before: Option<chrono::DateTime<chrono::Utc>>, + _description: &str, + ) -> Result<(), WhitenoiseError> { + let deadline = + tokio::time::Instant::now() + tokio::time::Duration::from_millis(50 * 50); + loop { + let after = context + .whitenoise + .find_account_by_pubkey(&pubkey) + .await? + .last_synced_at; + if after != before { + return Err(WhitenoiseError::Other(anyhow::anyhow!( + "last_synced_at advanced on global-only event" + ))); + } + if tokio::time::Instant::now() >= deadline { + return Ok(()); + } + tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; + } + }src/nostr_manager/query.rs (1)
51-56: Centralize future-skew filtering: LGTM; add a focused unit testThe switch to is_event_timestamp_valid in latest_from_events reads well and centralizes the policy. Please add a unit test that builds a small Events set containing:
- one event with created_at beyond the allowed skew, and
- one valid, recent event,
and asserts latest_from_events drops the future one and returns the valid latest.
There was a problem hiding this comment.
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 (2)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (2)
109-124: Don’t panic on send; propagate errors
unwrap()will crash the test; prefer?to surface a properWhitenoiseError.test_client - .send_event_builder(EventBuilder::metadata(metadata)) - .await - .unwrap(); + .send_event_builder(EventBuilder::metadata(metadata)) + .await?;This function already returns
Result<(), WhitenoiseError>.
126-160: Same here: avoidunwrap()when publishing relay listPropagate instead of panicking.
test_client - .send_event_builder(EventBuilder::new(Kind::RelayList, "").tags(nip65_tags)) - .await - .unwrap(); + .send_event_builder(EventBuilder::new(Kind::RelayList, "").tags(nip65_tags)) + .await?;
🧹 Nitpick comments (10)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (4)
1-6: Import order: align with repo guidelinesPlace std first, then external crates, then current/internal modules. Improves consistency.
-use crate::integration_tests::core::*; -use crate::{RelayType, WhitenoiseError}; -use async_trait::async_trait; -use nostr_sdk::prelude::*; use std::collections::HashSet; +use async_trait::async_trait; +use nostr_sdk::prelude::*; +use crate::integration_tests::core::*; +use crate::{RelayType, WhitenoiseError};As per coding guidelines.
239-256: Parse errors should bubble up, not panicReplace
unwrap()onRelayUrl::parsewith error propagation to avoid hard panics on invalid inputs.- let expected_relay = RelayUrl::parse(expected_relay_url).unwrap(); + let expected_relay = RelayUrl::parse(expected_relay_url) + .map_err(|e| WhitenoiseError::InvalidInput(format!("Invalid relay URL: {e}")))?;
264-291: Follow verification: consider deterministic ordering in diffsUsing
HashSetis fine; to improve diff readability, useBTreeSetwhen rendering differences (optional).No functional change required; current equality + detailed message is OK.
357-360: Fixed sleeps can be flaky; prefer wait-with-timeoutPoll the system until conditions are met or a timeout elapses to reduce flakes.
If you want, I can draft a small helper like
await_user_updates(context, &test_keys, expected)with a 5–10s timeout.src/whitenoise/event_processor/account_event_processor.rs (1)
90-121: Advancement behavior is correct and safely scoped
- Advance only on successful processing and only for account‑scoped kinds.
- Capping before converting to ms prevents forward‑time skew.
- Logging now reflects conditional advancement (not always advanced). Good.
One improvement: factor out the repeated cap→ms→update→log sequence to reduce duplication and keep future changes in one place.
@@ - Kind::ContactList | Kind::MlsGroupMessage => { - // Cap timestamp to prevent future corruption - let safe_timestamp = cap_timestamp_to_now(event.created_at); - let created_ms = (safe_timestamp.as_u64() as i64) * 1000; - if let Err(e) = Account::update_last_synced_max( - &account.pubkey, - created_ms, - &self.database, - ) - .await - { - tracing::warn!(target: "whitenoise::event_processor::process_account_event", - "Failed to advance last_synced_at for {} on event {}: {}", - account.pubkey.to_hex(), - event.id.to_hex(), - e - ); - } else { - tracing::debug!(target: "whitenoise::event_processor::process_account_event", - "Updated last_synced_at (if older) for {} with candidate {} (ms) [capped from original {}]", - account.pubkey.to_hex(), - created_ms, - event.created_at.as_u64() * 1000 - ); - } - } + Kind::ContactList | Kind::MlsGroupMessage => { + self.advance_account_last_synced(&account, event.created_at, "event") + .await; + } @@ - Ok(Some(rumor_timestamp)) => { - let safe_timestamp = cap_timestamp_to_now(rumor_timestamp); - let created_ms = (safe_timestamp.as_u64() as i64) * 1000; - if let Err(e) = Account::update_last_synced_max( - &account.pubkey, - created_ms, - &self.database, - ) - .await - { - tracing::warn!(target: "whitenoise::event_processor::process_account_event", - "Failed to advance last_synced_at with rumor timestamp for {}: {}", - account.pubkey.to_hex(), - e - ); - } else { - tracing::debug!(target: "whitenoise::event_processor::process_account_event", - "Updated last_synced_at (if older) for {} with rumor timestamp {} (ms) [capped from original {}]", - account.pubkey.to_hex(), - created_ms, - rumor_timestamp.as_u64() * 1000 - ); - } - } + Ok(Some(rumor_timestamp)) => { + self.advance_account_last_synced(&account, rumor_timestamp, "rumor") + .await; + }Add this helper inside
impl Whitenoise:+ async fn advance_account_last_synced(&self, account: &Account, candidate: Timestamp, source: &'static str) { + let safe = cap_timestamp_to_now(candidate); + let ms = (safe.as_u64() as i64) * 1000; + if let Err(e) = Account::update_last_synced_max(&account.pubkey, ms, &self.database).await { + tracing::warn!( + target: "whitenoise::event_processor::process_account_event", + "Failed to advance last_synced_at for {} from {}: {}", + account.pubkey.to_hex(), + source, + e + ); + } else { + tracing::debug!( + target: "whitenoise::event_processor::process_account_event", + "Updated last_synced_at (if older) for {} from {} to candidate {} (ms)", + account.pubkey.to_hex(), + source, + ms + ); + } + }Based on learnings.
Also applies to: 121-171
src/whitenoise/mod.rs (2)
252-264: Guard empty user list to avoid false NoRelayConnectionsIf users_with_relays is empty, setup_batched_relay_subscriptions_with_signer will return NoRelayConnections. Short-circuit here to no-op to keep initialization resilient.
- let Some(signer_account) = Account::first(&whitenoise_ref.database).await? else { + let Some(signer_account) = Account::first(&whitenoise_ref.database).await? else { tracing::info!(target: "whitenoise::setup_global_users_subscriptions","No signer account found, skipping global user subscriptions"); return Ok(()); }; + if users_with_relays.is_empty() { + tracing::info!(target: "whitenoise::setup_global_users_subscriptions","No users found for global subscriptions; skipping"); + return Ok(()); + }
267-307: De-duplicate the 10s buffer: introduce a named constantBUFFER_SECS is hardcoded (10) here and in accounts/subscription-update paths. Define a single constant to prevent drift.
+const GLOBAL_SINCE_BUFFER_SECS: u64 = 10; ... - // - Otherwise, use min(last_synced_at) minus a 10s buffer, floored at 0 + // - Otherwise, use min(last_synced_at) minus a small buffer, floored at 0 ... - const BUFFER_SECS: u64 = 10; - let since = accounts + let since = accounts .iter() - .filter_map(|a| a.since_timestamp(BUFFER_SECS)) + .filter_map(|a| a.since_timestamp(GLOBAL_SINCE_BUFFER_SECS)) .min_by_key(|t| t.as_u64()); ... - tracing::info!(target: "whitenoise::setup_global_users_subscriptions","Global subscriptions using since={} ({}s buffer)", ts.as_u64(), BUFFER_SECS); + tracing::info!(target: "whitenoise::setup_global_users_subscriptions","Global subscriptions using since={} ({}s buffer)", ts.as_u64(), GLOBAL_SINCE_BUFFER_SECS);src/nostr_manager/utils.rs (1)
497-511: Add a near-epoch test to assert saturationConsider a unit test where since is within the 7‑day window from epoch to assert it floors at 0.
+ #[test] + fn test_adjust_since_for_giftwrap_saturates_at_zero() { + // 5 seconds after epoch, with 7-day lookback should floor to 0 + let near_epoch = Timestamp::from(5); + let adjusted = adjust_since_for_giftwrap(Some(near_epoch)).unwrap(); + assert_eq!(adjusted.as_u64(), 0); + }src/nostr_manager/subscriptions.rs (2)
411-419: Minor cleanup: avoid unwrap-wrap for OptionYou can pass the Option directly to adjust_since_for_giftwrap.
- if let Some(since) = since { - // Account for NIP-59 backdated timestamps - giftwrap events may be timestamped - // in the past for privacy, so we look back further than last_synced_at - let adjusted_since = adjust_since_for_giftwrap(Some(since)); - if let Some(adjusted) = adjusted_since { - giftwrap_filter = giftwrap_filter.since(adjusted); - } - } + // Account for NIP-59 backdated timestamps - giftwrap events may be backdated + if let Some(adjusted) = adjust_since_for_giftwrap(since) { + giftwrap_filter = giftwrap_filter.since(adjusted); + }
193-216: Consider ensuring relay connectivity once per relaysubscribe_user_batch calls ensure_relays_connected for each batch of the same relay. You can move the connectivity check to create_deterministic_batches_for_relay (once per relay) for efficiency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/integration_tests/core/test_clients.rs(1 hunks)src/integration_tests/scenarios/subscription_processing.rs(1 hunks)src/integration_tests/test_cases/subscription_processing/mod.rs(1 hunks)src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs(8 hunks)src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/mod.rs(2 hunks)src/nostr_manager/query.rs(2 hunks)src/nostr_manager/subscriptions.rs(7 hunks)src/nostr_manager/utils.rs(2 hunks)src/whitenoise/accounts.rs(5 hunks)src/whitenoise/database/accounts.rs(2 hunks)src/whitenoise/event_processor/account_event_processor.rs(3 hunks)src/whitenoise/event_processor/mod.rs(3 hunks)src/whitenoise/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/whitenoise/event_processor/mod.rs
- src/integration_tests/core/test_clients.rs
- src/nostr_manager/query.rs
- src/whitenoise/database/accounts.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/whitenoise/event_processor/account_event_processor.rssrc/integration_tests/test_cases/subscription_processing/mod.rssrc/nostr_manager/subscriptions.rssrc/nostr_manager/utils.rssrc/whitenoise/mod.rssrc/integration_tests/test_cases/subscription_processing/publish_subscription_update.rssrc/integration_tests/scenarios/subscription_processing.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rssrc/whitenoise/accounts.rssrc/nostr_manager/mod.rs
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Add and maintain MLS integration tests under src/integration_tests
Files:
src/integration_tests/test_cases/subscription_processing/mod.rssrc/integration_tests/test_cases/subscription_processing/publish_subscription_update.rssrc/integration_tests/scenarios/subscription_processing.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Ensure post-compromise security through correct epoch transitions and secret updates
Applied to files:
src/whitenoise/event_processor/account_event_processor.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/whitenoise/event_processor/account_event_processor.rssrc/whitenoise/mod.rssrc/whitenoise/accounts.rssrc/nostr_manager/mod.rs
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Maintain message ordering and group state consistency across epochs
Applied to files:
src/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Preserve forward secrecy in MLS operations (ephemeral keys, proper secret updates)
Applied to files:
src/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-04T03:38:54.953Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/nostr_manager/subscriptions.rs:72-89
Timestamp: 2025-09-04T03:38:54.953Z
Learning: In the global subscription batching system for Whitenoise (src/nostr_manager/subscriptions.rs), the current hash(pubkey) % batch_count approach causes batch ID reshuffling when user count changes, potentially leading to stale/duplicate author lists since only the triggering user's batch is refreshed. This is a known limitation documented in PR #340 that the team is aware of and will address in future iterations.
Applied to files:
src/nostr_manager/subscriptions.rssrc/whitenoise/mod.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/nostr_manager/subscriptions.rs
📚 Learning: 2025-09-04T16:58:00.736Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Applied to files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
📚 Learning: 2025-09-18T11:21:01.705Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/rust-code-style.mdc:0-0
Timestamp: 2025-09-18T11:21:01.705Z
Learning: Applies to **/*.rs : Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
Applied to files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/event_processor/event_handlers/handle_mls_message.rs : Follow the Marmot protocol spec when integrating MLS messages with Nostr
Applied to files:
src/whitenoise/accounts.rs
📚 Learning: 2025-08-11T10:23:59.406Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/relays.rs:260-278
Timestamp: 2025-08-11T10:23:59.406Z
Learning: In `src/whitenoise/accounts/relays.rs`, the `publish_relay_list_for_account` method is only called after the account setup is complete, which guarantees that `account.nip65_relays` is already populated and will never be empty. Therefore, no fallback logic is needed when `target_relays` is None.
Applied to files:
src/whitenoise/accounts.rs
🧬 Code graph analysis (6)
src/whitenoise/event_processor/account_event_processor.rs (2)
src/nostr_manager/utils.rs (1)
cap_timestamp_to_now(32-39)src/whitenoise/database/accounts.rs (1)
update_last_synced_max(450-474)
src/nostr_manager/subscriptions.rs (1)
src/nostr_manager/utils.rs (1)
adjust_since_for_giftwrap(22-29)
src/whitenoise/mod.rs (1)
src/whitenoise/database/accounts.rs (1)
all(93-103)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (2)
src/whitenoise/accounts.rs (4)
follows(754-754)account(1032-1038)account(1130-1136)new(52-71)src/integration_tests/core/test_clients.rs (1)
publish_follow_list(68-81)
src/integration_tests/scenarios/subscription_processing.rs (2)
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (2)
for_external_user(29-37)for_account(18-26)src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (2)
for_account_follow_event(19-23)for_global_metadata_event(25-29)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (3)
src/whitenoise/accounts.rs (4)
account(1032-1038)account(1130-1136)new(52-71)metadata(202-205)src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (1)
create_test_client(4-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (16)
src/integration_tests/test_cases/subscription_processing/mod.rs (1)
2-2: LGTM: module wiring and re-export are correctNo issues spotted; keeps test-case API tidy.
Also applies to: 5-5
src/integration_tests/scenarios/subscription_processing.rs (2)
63-66: Key reuse correctnessUsing
alice_keys.clone()avoids moving keys earlier—keeps later tests valid. Good call.Please confirm no tests rely on side effects from the previous
alice_keysoperations (ordering-sensitive flakiness).
68-73: Good additions: follow-list publish + timestamp checksThe new steps exercise end‑to‑end follows and last_synced policy. Looks sound.
If flakes occur, consider inserting an await-for-processing helper instead of fixed sleeps within the involved test cases.
Also applies to: 74-81
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs (3)
297-316: Nice UX: aggregated “updates” labelThe user-facing trace is helpful for test logs. Keep it.
352-355: Replace remaining.unwrap()calls in integration tests
Unhandledunwrap()calls remain at:
- src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs:120
- src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs:156
- src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs:242
Use the?operator orexpect()with a descriptive message instead of.unwrap()to improve test failure diagnostics.⛔ Skipped due to learnings
Learnt from: jgmontoya PR: parres-hq/whitenoise#324 File: src/integration_tests/test_cases/subscription_processing/publish_metadata_update.rs:42-45 Timestamp: 2025-08-23T16:27:41.511Z Learning: In Whitenoise integration tests, using `unwrap()` on `test_client.add_relay()` for dev relay connections is acceptable since these are controlled test environments where failing fast is preferred over complex error handling.Learnt from: jgmontoya PR: parres-hq/whitenoise#324 File: src/integration_tests/test_cases/subscription_processing/publish_metadata_update.rs:35-39 Timestamp: 2025-08-23T16:23:47.066Z Learning: In the Whitenoise integration test framework, using `unwrap()` on `RelayUrl::parse()` for the hardcoded dev_relays URLs (ws://localhost:8080 and ws://localhost:7777) is acceptable since these are known, controlled addresses in the test environment where failing fast is preferred.
162-172: Enforce account-only at publish time (graceful error)Add a runtime guard to match the builder’s intent.
async fn publish_follow_list_update( &self, test_client: &Client, follows: &[PublicKey], ) -> Result<(), WhitenoiseError> { + if self.account_name.is_none() { + return Err(WhitenoiseError::InvalidInput( + "Follow list publishing only supported for accounts".to_string(), + )); + } tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; publish_follow_list(test_client, follows).await?; tracing::info!("✓ Follow list published via test client"); Ok(()) }⛔ Skipped due to learnings
Learnt from: jgmontoya PR: parres-hq/whitenoise#310 File: src/whitenoise/accounts/relays.rs:260-278 Timestamp: 2025-08-11T10:23:59.406Z Learning: In `src/whitenoise/accounts/relays.rs`, the `publish_relay_list_for_account` method is only called after the account setup is complete, which guarantees that `account.nip65_relays` is already populated and will never be empty. Therefore, no fallback logic is needed when `target_relays` is None.Learnt from: jmcorgan PR: parres-hq/whitenoise#318 File: src/whitenoise/accounts/messages.rs:193-195 Timestamp: 2025-08-17T19:34:30.333Z Learning: In the whitenoise codebase, `Account::create_nostr_mls()` returns `Result<NostrMls<NostrMlsSqliteStorage>, AccountError>`, and `WhitenoiseError` has an `Account(#[from] AccountError)` variant that automatically converts. When handling MLS creation errors, use the `?` operator to let `AccountError` convert naturally to `WhitenoiseError::Account` rather than manually mapping to `WhitenoiseError::Configuration`.src/whitenoise/event_processor/account_event_processor.rs (2)
5-5: Good: cap future timestamps before advancementImporting and using
cap_timestamp_to_nowmitigates far‑future event corruption. This addresses the earlier reviewer concern on bad future timestamps.
332-346: Rumor-timestamp extraction path is appropriateGracefully returns
Ok(None)on extraction failure to avoid false advancement; good tradeoff.Consider a unit test for: (1) rumor extraction fails → no advancement; (2) rumor timestamp in the future → capped value persisted.
src/whitenoise/mod.rs (1)
309-333: Per-account subscription orchestration reads wellThe new setup_accounts_subscriptions helper isolates per-account setup and continues on errors, which improves robustness over fail-fast behavior. LGTM.
src/nostr_manager/utils.rs (2)
20-29: Saturating lookback for giftwrap — good fixUsing saturating_sub avoids underflow near epoch. This prevents pathological since values. LGTM.
31-39: Capping future timestamps is correctcap_timestamp_to_now guards against future-skewed data corrupting state. Implementation is straightforward. LGTM.
src/nostr_manager/subscriptions.rs (1)
27-34: End-to-end since propagation for batched subssince is correctly threaded through setup_batched_relay_subscriptions_with_signer → create_deterministic_batches_for_relay → subscribe_user_batch. This aligns global filters with last_synced tracking. LGTM.
Also applies to: 45-52, 74-80, 100-107
src/whitenoise/accounts.rs (3)
73-83: since_timestamp: correct clamping and buffer logicClamps future to now, floors at 0 via saturating_sub, and returns None for never-synced. This prevents empty/future-only filters. LGTM.
819-833: Per-account since applied to subscription setupComputing per-account since (10s buffer) and passing it through closes the gap from last_synced. LGTM.
Also applies to: 846-847
1283-1355: Unit tests cover None, buffer, epoch floor, and future clampGood coverage for edge cases; expectations match the new semantics. LGTM.
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (1)
72-87: Stop exiting on the first unchanged poll before the window elapses.
retryreturnsOkas soon as one attempt reportslast_synced_at == before. In practice the helper polls immediately after publishing the global metadata event, so it will almost always succeed before the sync worker has a chance to process anything. If the worker later (incorrectly) advanceslast_synced_at, this test still passes, so we lose the regression guard you’re adding here. Please hold the check for the entire window instead of returning early on the first success.async fn assert_unchanged( &self, context: &mut ScenarioContext, pubkey: PublicKey, before: Option<chrono::DateTime<chrono::Utc>>, description: &str, ) -> Result<(), WhitenoiseError> { - retry( - 50, - Duration::from_millis(50), - || async { - let account = context.whitenoise.find_account_by_pubkey(&pubkey).await?; - if account.last_synced_at == before { - Ok(()) - } else { - Err(WhitenoiseError::Other(anyhow::anyhow!( - "last_synced_at advanced on global-only event" - ))) - } - }, - description, - ) - .await + let poll_interval = tokio::time::Duration::from_millis(50); + let deadline = + tokio::time::Instant::now() + tokio::time::Duration::from_millis(50 * 50); + loop { + let account = context.whitenoise.find_account_by_pubkey(&pubkey).await?; + if account.last_synced_at != before { + return Err(WhitenoiseError::Other(anyhow::anyhow!( + "{description}: last_synced_at advanced on global-only event" + ))); + } + if tokio::time::Instant::now() >= deadline { + return Ok(()); + } + tokio::time::sleep(poll_interval).await; + } }
| /// Add follow list update to the test (account-based only) | ||
| pub fn with_follow_list(mut self, follows: Vec<PublicKey>) -> Self { | ||
| self.follow_pubkeys = Some(follows); | ||
| self | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Enforce “account-only” for follow-list builder or during run
Builder says “account-based only,” but nothing prevents using it for external users. Guard it to fail fast.
pub fn with_follow_list(mut self, follows: Vec<PublicKey>) -> Self {
+ // Guard at build time to avoid misusing this path
+ if self.account_name.is_none() {
+ panic!("with_follow_list is only supported for account-based tests");
+ }
self.follow_pubkeys = Some(follows);
self
}Alternatively, prefer a graceful error in run() (see next comment) if panics in tests are undesirable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Add follow list update to the test (account-based only) | |
| pub fn with_follow_list(mut self, follows: Vec<PublicKey>) -> Self { | |
| self.follow_pubkeys = Some(follows); | |
| self | |
| } | |
| /// Add follow list update to the test (account-based only) | |
| pub fn with_follow_list(mut self, follows: Vec<PublicKey>) -> Self { | |
| // Guard at build time to avoid misusing this path | |
| if self.account_name.is_none() { | |
| panic!("with_follow_list is only supported for account-based tests"); | |
| } | |
| self.follow_pubkeys = Some(follows); | |
| self | |
| } |
🤖 Prompt for AI Agents
In
src/integration_tests/test_cases/subscription_processing/publish_subscription_update.rs
around lines 51–55, the with_follow_list builder claims “account-based only” but
doesn't enforce it; add validation to fail fast by checking each PublicKey in
follows is an account-type key and return/raise an error immediately (or panic
in test code) if any are external, or instead keep the builder permissive but
perform the same validation in run() and return a Result/Err so tests can fail
gracefully; implement whichever approach the test harness prefers and ensure the
error message clearly states "follow list must contain account-based public keys
only" and includes the offending key(s).
| since: Option<Timestamp>, | ||
| signer: impl NostrSigner + 'static, | ||
| ) -> Result<()> { |
There was a problem hiding this comment.
🧩 Analysis chain
since parameter threading looks correct
The new since: Option is cleanly added and forwarded to setup_account_subscriptions. LGTM.
Run to confirm all call sites are updated:
Also applies to: 230-231
🏁 Script executed:
#!/bin/bash
# Find and show all call sites and signatures for setup_account_subscriptions_with_signer
rg -nP -C2 '\bsetup_account_subscriptions_with_signer\s*\('Length of output: 739
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'setup_account_subscriptions_with_signer' src/whitenoise/accounts.rsLength of output: 252
Missing since and signer arguments at call sites
Call site in src/whitenoise/accounts.rs (line 840) still only passes pubkey, &user_relays, &inbox_relays; you must append the new since: Option<Timestamp> and signer parameters to match the updated signature.
🤖 Prompt for AI Agents
In src/nostr_manager/mod.rs around lines 216-218 the function signature now
requires since: Option<Timestamp> and signer: impl NostrSigner + 'static; update
every call site (notably src/whitenoise/accounts.rs at line 840) to append two
arguments: a suitable Option<Timestamp> (e.g., None or the computed timestamp)
and a value that implements NostrSigner (or a boxed/cloned signer with the
required 'static lifetime). Ensure the passed signer type matches the function's
trait bound and adjust imports/constructors where necessary so the call
compiles.
jgmontoya
force-pushed
the
refactor/remove-background-sync
branch
from
9cb9f1c to
322587d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (1)
152-168: Consider simplifying the timestamp wait logic.The current logic waits if
now() <= baseline, then publishes an event with a future timestamp (base+10s) that gets capped during processing. This is fragile and tightly couples the test to the implementation detail of timestamp capping.Consider using a timestamp that is guaranteed to advance last_synced_at without relying on capping:
- if let Some(before_time) = before { - let before_timestamp_secs = before_time.timestamp() as u64; - let current_timestamp_secs = Timestamp::now().as_u64(); - - if current_timestamp_secs <= before_timestamp_secs { - // Wait enough time to ensure now() > baseline when event gets processed - let wait_time = (before_timestamp_secs - current_timestamp_secs + 2) * 1000; // +2 seconds buffer - tracing::debug!( - target: "verify_last_synced_timestamp", - "Waiting {}ms to ensure capped timestamp > baseline ({} vs {})", - wait_time, - current_timestamp_secs, - before_timestamp_secs - ); - tokio::time::sleep(std::time::Duration::from_millis(wait_time)).await; - } - } - - // Use base timestamp + 10 seconds for guaranteed advancement - // Even if this gets capped to now(), it should be > baseline due to our wait - let event_timestamp = Timestamp::from_secs(base_timestamp.as_u64() + 10); + // Wait a moment to ensure time advances beyond baseline + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + // Use current timestamp to ensure it's valid and advances last_synced_at + let event_timestamp = Timestamp::now();src/nostr_manager/utils.rs (1)
485-495: Add explicit saturation test at the epoch boundary.Include a test case such as
adjust_since_for_giftwrap(Some(Timestamp::from(0)))and assert it returnsSome(Timestamp::from(0))(or zero) to verify thesaturating_subbehavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/query.rs(2 hunks)src/nostr_manager/subscriptions.rs(9 hunks)src/nostr_manager/utils.rs(2 hunks)src/whitenoise/event_processor/account_event_processor.rs(4 hunks)src/whitenoise/event_processor/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nostr_manager/subscriptions.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/whitenoise/event_processor/mod.rssrc/whitenoise/event_processor/account_event_processor.rssrc/nostr_manager/query.rssrc/nostr_manager/utils.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Add and maintain MLS integration tests under src/integration_tests
Files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Ensure post-compromise security through correct epoch transitions and secret updates
Applied to files:
src/whitenoise/event_processor/mod.rssrc/whitenoise/event_processor/account_event_processor.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Maintain message ordering and group state consistency across epochs
Applied to files:
src/whitenoise/event_processor/mod.rssrc/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-04T16:58:00.736Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Applied to files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
📚 Learning: 2025-09-18T11:21:01.705Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/rust-code-style.mdc:0-0
Timestamp: 2025-09-18T11:21:01.705Z
Learning: Applies to **/*.rs : Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
Applied to files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧬 Code graph analysis (4)
src/whitenoise/event_processor/mod.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/whitenoise/event_processor/account_event_processor.rs (2)
src/nostr_manager/utils.rs (1)
cap_timestamp_to_now(32-39)src/whitenoise/database/accounts.rs (1)
update_last_synced_max(450-474)
src/nostr_manager/query.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (3)
src/whitenoise/accounts.rs (4)
account(1032-1038)account(1130-1136)new(52-71)metadata(202-205)src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (1)
create_test_client(4-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (6)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (1)
65-88: Verify that assert_unchanged correctly enforces stability.A previous review flagged that using
retryreturns success on the first unchanged read, which can miss late updates. The suggested fix was to enforce stability across the entire window by looping until a deadline. However, the current code still uses theretrypattern, which may pass prematurely.Please confirm whether the fix from commit 63e65c4 is present in this version, or if the concern remains valid.
src/nostr_manager/utils.rs (5)
1-12: LGTM! Well-structured constants and imports.The imports follow the coding guidelines (std, external, internal), and the constants are clearly documented with appropriate time values for their purposes.
14-18: LGTM! Clean timestamp validation logic.The function correctly validates event timestamps against the future skew limit, providing protection against timestamp manipulation.
20-29: LGTM! Underflow issue correctly addressed.The saturating subtraction properly handles the edge case where timestamps near epoch could underflow, as flagged in the previous review. The implementation now safely floors at zero.
31-39: LGTM! Simple and correct timestamp capping.The function correctly prevents future timestamps from being stored by capping them to the current time.
408-558: Excellent test coverage for timestamp utilities.The test suite is comprehensive and well-structured, covering:
- Boundary conditions (current, past, near/far future, exact boundary)
- Edge cases (None values, old timestamps, timing races)
- Constant validation
The tests appropriately use timing tolerance where needed (e.g., lines 529-534) to handle timing races.
…ture skew protection and giftwrap timestamp handling
jgmontoya
force-pushed
the
refactor/remove-background-sync
branch
from
322587d to
4ac821e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/whitenoise/event_processor/account_event_processor.rs (1)
68-74: Consider adjusting log level from info to debug.This log fires for every account event processed and may be too verbose at
infolevel in production. Since this is primarily useful for debugging event flow, consider usingtracing::debug!instead.Apply this diff to reduce log verbosity:
- tracing::info!( + tracing::debug!( target: "whitenoise::event_processor::process_account_event", "Processing event {} (kind {}) for account {}", event.id.to_hex(), event.kind.as_u16(), account.pubkey.to_hex() );src/whitenoise/event_processor/mod.rs (1)
200-226: Consider adding integration test for event processing loop.The unit test correctly validates
is_event_timestamp_validfor both invalid and valid timestamps. However, it only tests the utility function in isolation.Consider adding an integration test that verifies the full behavior: sending an event with an invalid timestamp through the processing loop and confirming it gets skipped (e.g., by verifying it doesn't reach the handler or update any state). This would ensure the guard at lines 62-70 works correctly end-to-end.
Example approach:
#[tokio::test] async fn test_event_processing_skips_future_timestamp() { let (whitenoise, _data_temp, _logs_temp) = create_mock_whitenoise().await; // Create event with invalid future timestamp let keys = Keys::generate(); let far_future = Timestamp::now() + StdDuration::from_secs(7200); let event = EventBuilder::text_note("future event") .custom_created_at(far_future) .sign(&keys) .await .unwrap(); // Send through processing loop and verify it's skipped // (implementation depends on available test utilities) }src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (4)
9-16: Consider adding Debug derive for test diagnostics.While this is test infrastructure rather than library code, adding
DebugtoVerifyLastSyncedTimestampTestCasewould improve diagnostics when tests fail or when debugging test execution.+#[derive(Debug)] pub struct VerifyLastSyncedTimestampTestCase { mode: Mode, } +#[derive(Debug)] enum Mode { AccountFollowEvent, GlobalMetadataEvent, }
40-63: Clarify error message for unexpected regression cases.The match arms correctly detect advancement, but if
last_synced_atunexpectedly regresses toNone(case(Some(_), None)), the generic error message "last_synced_at not advanced yet" is misleading.Consider making the error more descriptive:
let account = context.whitenoise.find_account_by_pubkey(&pubkey).await?; match (before, account.last_synced_at) { (None, Some(_)) => Ok(()), (Some(before_time), Some(after_time)) if after_time > before_time => Ok(()), - _ => Err(WhitenoiseError::Other(anyhow::anyhow!( - "last_synced_at not advanced yet" - ))), + (Some(b), after) => Err(WhitenoiseError::Other(anyhow::anyhow!( + "last_synced_at not advanced: before={:?}, after={:?}", b, after + ))), + (None, None) => Err(WhitenoiseError::Other(anyhow::anyhow!( + "last_synced_at still None" + ))), }
166-166: Prefer consistent Duration usage.For consistency with the rest of the file (lines 110, 133), consider using
tokio::time::Durationhere as well, since this is in an async context withtokio::time::sleep.- tokio::time::sleep(std::time::Duration::from_millis(wait_time)).await; + tokio::time::sleep(tokio::time::Duration::from_millis(wait_time)).await;
148-182: Reference existingcap_timestamp_to_nowimplementation in test comment.The timestamp-capping behavior is already implemented and documented in
nostr_manager/utils.rs(fncap_timestamp_to_now). Update the comment inverify_last_synced_timestamp.rs(lines 150–151) to reference that utility so future readers know where the capping occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs(1 hunks)src/nostr_manager/query.rs(2 hunks)src/nostr_manager/subscriptions.rs(9 hunks)src/nostr_manager/utils.rs(2 hunks)src/whitenoise/event_processor/account_event_processor.rs(4 hunks)src/whitenoise/event_processor/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nostr_manager/utils.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust-code-style.mdc)
**/*.rs: Write all trait bounds in a where clause for functions and impl blocks
Prefer using Self when referring to the implementing type (including return types and enum variant matches)
For public types in libraries, derive Debug, Clone, Copy, PartialEq, Eq, and Hash (in this order) when possible
Derive Default only when a reasonable default value exists
Use full paths for tracing macros (tracing::!(...)) instead of importing macro names
Prefer .to_string() or .to_owned() for &str -> String conversion over .into() or String::from
Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
When implementing fmt traits, import core::fmt and use fmt::Display, fmt::Formatter, etc.
After declaring mod x;, import sub-items via self::x::Y instead of x::Y
Avoid if let ... { } else { } in favor of match when both branches do work
Use if let ... { } when the alternative match arm is intentionally empty
Avoid inline sub-modules (mod x { ... }); prefer a separate file x.rs with mod x; (except tests and benches)
For tests, prefer inline #[cfg(test)] mod tests { ... } and avoid mod tests;
For benches, prefer inline #[cfg(bench)] mod benches { ... } and avoid mod benches;
Files:
src/nostr_manager/query.rssrc/whitenoise/event_processor/mod.rssrc/nostr_manager/subscriptions.rssrc/whitenoise/event_processor/account_event_processor.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Add and maintain MLS integration tests under src/integration_tests
Files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Ensure post-compromise security through correct epoch transitions and secret updates
Applied to files:
src/whitenoise/event_processor/mod.rssrc/whitenoise/event_processor/account_event_processor.rssrc/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
📚 Learning: 2025-09-29T10:45:56.417Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/mls-protocol-docs.mdc:0-0
Timestamp: 2025-09-29T10:45:56.417Z
Learning: Applies to src/whitenoise/{event_processor/event_handlers/handle_mls_message.rs,key_packages.rs,groups.rs,welcomes.rs} : Maintain message ordering and group state consistency across epochs
Applied to files:
src/whitenoise/event_processor/mod.rssrc/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-04T03:38:54.953Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/nostr_manager/subscriptions.rs:72-89
Timestamp: 2025-09-04T03:38:54.953Z
Learning: In the global subscription batching system for Whitenoise (src/nostr_manager/subscriptions.rs), the current hash(pubkey) % batch_count approach causes batch ID reshuffling when user count changes, potentially leading to stale/duplicate author lists since only the triggering user's batch is refreshed. This is a known limitation documented in PR #340 that the team is aware of and will address in future iterations.
Applied to files:
src/nostr_manager/subscriptions.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/nostr_manager/subscriptions.rs
📚 Learning: 2025-09-14T19:36:21.619Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#365
File: src/whitenoise/accounts.rs:296-298
Timestamp: 2025-09-14T19:36:21.619Z
Learning: Repository parres-hq/whitenoise: For the process_user_event_streams method in src/whitenoise/accounts.rs, the team prefers to address the concurrent stream processing improvement (processing streams concurrently with per-event error logging instead of sequential processing with early returns) in a separate PR, as requested by jgmontoya in PR #365.
Applied to files:
src/whitenoise/event_processor/account_event_processor.rs
📚 Learning: 2025-09-04T16:58:00.736Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/whitenoise/event_processor/mod.rs:121-158
Timestamp: 2025-09-04T16:58:00.736Z
Learning: Repository parres-hq/whitenoise: For retry scheduling hardening (schedule_retry in src/whitenoise/event_processor/mod.rs and backoff logic in RetryInfo in src/types.rs), the team prefers to land the improvements in a separate PR, tracked via a GitHub issue, as requested by jgmontoya in PR #340.
Applied to files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
📚 Learning: 2025-09-18T11:21:01.705Z
Learnt from: CR
PR: parres-hq/whitenoise#0
File: .cursor/rules/rust-code-style.mdc:0-0
Timestamp: 2025-09-18T11:21:01.705Z
Learning: Applies to **/*.rs : Order imports: (1) core/alloc/std, (2) external crates, (3) current sub-modules (mod x;), (4) internal crate modules (use crate::, super::, self::)
Applied to files:
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs
🧬 Code graph analysis (5)
src/nostr_manager/query.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/whitenoise/event_processor/mod.rs (1)
src/nostr_manager/utils.rs (1)
is_event_timestamp_valid(15-18)
src/nostr_manager/subscriptions.rs (1)
src/nostr_manager/utils.rs (1)
adjust_since_for_giftwrap(22-29)
src/whitenoise/event_processor/account_event_processor.rs (2)
src/nostr_manager/utils.rs (1)
cap_timestamp_to_now(32-39)src/whitenoise/database/accounts.rs (1)
update_last_synced_max(450-474)
src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (3)
src/whitenoise/accounts.rs (3)
account(1032-1038)account(1130-1136)new(52-71)src/integration_tests/core/retry.rs (1)
retry(90-102)src/integration_tests/core/test_clients.rs (1)
create_test_client(4-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (macos-latest, native)
- GitHub Check: check (macos-14, native)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (20)
src/nostr_manager/query.rs (2)
6-6: LGTM: Centralized timestamp validation.The import of
is_event_timestamp_validconsolidates timestamp validation logic from the removed inlineMAX_FUTURE_SKEWcutoff, improving maintainability.
51-57: LGTM: Timestamp filtering now uses centralized utility.The refactored
latest_from_eventscorrectly appliesis_event_timestamp_validas a filter predicate instead of the previous inline timestamp cutoff. This is cleaner and ensures consistent timestamp validation across the codebase.src/whitenoise/event_processor/account_event_processor.rs (4)
5-5: LGTM: Timestamp capping utility imported.The import of
cap_timestamp_to_nowensures that event timestamps are bounded to prevent corruption from far-future timestamps, addressing potential concerns about malicious or incorrect event timestamps.
99-128: LGTM: Timestamp advancement for ContactList and MlsGroupMessage.The logic correctly:
- Gates advancement behind successful event processing
- Caps timestamps using
cap_timestamp_to_nowto prevent future-timestamp corruption- Converts to milliseconds for database storage
- Logs both success and failure appropriately
- Uses
update_last_synced_maxwhich only advances if the new timestamp is newer
129-179: LGTM: GiftWrap rumor timestamp extraction and advancement.The GiftWrap handling correctly:
- Uses the rumor's
created_at(per NIP-59) instead of the wrapper's timestamp- Caps the rumor timestamp to prevent future corruption
- Handles all three extraction outcomes (Ok(Some), Ok(None), Err) appropriately
- Logs detailed advancement information including the original uncapped timestamp for debugging
- Only advances on successful extraction
The log message at lines 154-159 is particularly helpful, showing both the capped and original timestamps.
340-354: LGTM: Rumor timestamp extraction helper.The helper correctly:
- Retrieves account keys from the secrets store
- Attempts rumor extraction using the nostr_sdk API
- Returns
Ok(None)on extraction failure (preventing advancement on corrupted giftwraps)- Extracts only the timestamp needed for sync advancement
Error handling is appropriate: extraction failures return
Ok(None)rather than propagating errors, which is the right choice since we don't want to fail event processing if we can't extract the rumor timestamp.src/nostr_manager/subscriptions.rs (7)
12-14: LGTM: Import giftwrap timestamp adjustment utility.The import of
adjust_since_for_giftwrapenables proper handling of NIP-59's backdated giftwrap timestamps by extending the lookback period.
28-34: LGTM: Optional since parameter added to batched subscriptions.The
since: Option<Timestamp>parameter is correctly added to the public method signature, allowing callers to specify a starting timestamp for event retrieval.
74-79: LGTM: since parameter threaded through batch creation.The
sinceparameter is correctly propagated fromsetup_batched_relay_subscriptions_with_signer→create_deterministic_batches_for_relay→subscribe_user_batch.
193-216: LGTM: Filter applies since when provided.The
subscribe_user_batchmethod correctly:
- Accepts the optional
sinceparameter- Conditionally applies it to the filter only when present
- Maintains the existing kind and author filters
369-405: LGTM: Enhanced logging for follow-list subscriptions.The improvements correctly:
- Add pubkey and relay count to the initial log for better observability
- Include subscription ID and pubkey in the success log
- Apply the optional
sincefilter when providedThe enhanced logging will help with debugging subscription setup issues.
407-439: LGTM: Giftwrap subscription with backdated timestamp handling.The giftwrap subscription correctly implements NIP-59 privacy considerations:
- Uses
adjust_since_for_giftwrapto extend the lookback period byGIFTWRAP_LOOKBACK_BUFFER(7 days)- Only applies the adjusted timestamp if it's available (graceful fallback)
- Comments clearly explain the privacy-related backdating behavior
This ensures giftwrap events with intentionally backdated timestamps (for privacy) are not missed when using
sincefiltering.
527-551: LGTM: Comprehensive test for giftwrap lookback buffer.The test validates:
adjust_since_for_giftwrapsubtracts exactlyGIFTWRAP_LOOKBACK_BUFFERfrom the timestamp- The buffer duration is 7 days (604800 seconds)
Noneinput returnsNone(graceful handling)This test provides good coverage for the giftwrap timestamp adjustment behavior.
src/whitenoise/event_processor/mod.rs (2)
5-5: LGTM!The import is correctly structured and follows the coding guidelines for import ordering.
61-70: LGTM — timestamp validation applies to retries
Retry scheduling re-queues events through the sameevent_senderintoprocess_events, so theis_event_timestamp_validguard will run on every retry and reject invalid timestamps.src/integration_tests/test_cases/subscription_processing/verify_last_synced_timestamp.rs (5)
18-29: LGTM!Clean factory constructors that properly use
Selfas per coding guidelines.
31-38: LGTM!Simple baseline snapshot method with correct error handling.
90-113: LGTM!Correctly publishes a ContactList event with the account's keys and specified timestamp.
115-136: LGTM!Correctly publishes a metadata event from a different user to verify it doesn't affect the account's last_synced_at.
183-195: LGTM!The GlobalMetadataEvent branch correctly publishes an event that shouldn't affect the account's last_synced_at and verifies it remains unchanged.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests