refactor(time): split monotonic clock from wall clock - #611
Conversation
Instant was backed by now_millis() — millisecond resolution, derived from the wall clock. This conflated two distinct concerns: "what time is it?" (wall clock) versus "how much time passed?" (monotonic clock). Two real consequences: - Sub-millisecond elapsed measurements quantize to 0/1/2 ms. The bot's pong reply consistently shows "1.00ms" because actual send latency (~700us-1.5ms) lands across one ms boundary. - An NTP adjustment mid-measurement could move the wall clock backwards and produce nonsensical timeouts/durations. Existing comment acknowledged this: "not truly monotonic but sufficient ...". Add a separate MonotonicProvider trait alongside TimeProvider: - Native default: backed by std::time::Instant. Truly monotonic, ns resolution, the only legitimate call to std::time::Instant::now in the codebase (clippy.toml continues to forbid it elsewhere). - WASM default: derives from now_millis() as a backwards-compatible fallback. Embedders should register a real provider via set_monotonic_provider — performance.now in browsers, process.hrtime.bigint in Node, wasi:clocks/monotonic-clock in WASI. Instant now stores u64 nanoseconds (~584 years before overflow). Public API unchanged: now()/elapsed()/saturating_duration_since/Add<Duration>/ Sub<Instant> all preserved. No call site needs to migrate. set_time_provider only affects the wall clock (now_millis, now_utc, etc.); set_monotonic_provider is the new knob for Instant.
Two call sites computed elapsed time by diffing two now_millis() samples. With the wall clock providing both, an NTP adjustment between the samples could distort the result; the resolution was also fixed at 1ms. - src/keepalive.rs: split the keepalive timer. Wall-clock start_ms is preserved because the server-side clock-skew computation (update_server_time_offset_with_rtt) compares against the server's Unix-epoch frame. The RTT itself is now measured with Instant — the pong log line gets sub-ms precision and is immune to clock jumps. - src/handlers/message.rs: per-message processing time vs the MAX_MESSAGE_DELAY warning now uses Instant. Pure elapsed-time use case, no wall-clock semantics needed. Other now_millis() call sites are wire-protocol timestamps, persisted TTLs, or last-seen markers — wall clock is correct there.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change moves timing to a monotonic nanosecond-based provider. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Handler (message / keepalive)
participant Instant as wacore::time::Instant
participant Provider as MonotonicProvider
participant Duration as std::time::Duration
Handler->>Instant: Instant::now()
Instant->>Provider: now_nanos()
Provider-->>Instant: nanos
Instant-->>Handler: Instant(nanos)
Handler->>Handler: perform work / send ping
Handler->>Instant: instant.elapsed()
Instant->>Provider: now_nanos()
Provider-->>Instant: nanos
Instant->>Duration: compute diff (nanos)
Duration-->>Handler: Duration
Handler->>Handler: .as_millis() / .as_secs_f64()
Handler->>Handler: compare/log/update offsets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/3 reviews remaining, refill in 42 minutes and 51 seconds. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/keepalive.rs (1)
68-85:⚠️ Potential issue | 🟠 MajorDon't feed monotonic RTT into the wall-clock skew calculation.
Line 82 still calls
update_server_time_offset_with_rtt, and that helper insrc/unified_session.rs:59-81computes a wall-clock midpoint asstart_time_ms + rtt_ms / 2. Passingrtt_start.elapsed()there mixes clock domains: if the wall clock steps during the ping, the server offset you store is wrong until the next update. Keep the monotonic RTT for logs/health checks, but derive the skew midpoint from wall-clock samples instead.Proposed direction
let start_ms = wacore::time::now_millis(); let rtt_start = wacore::time::Instant::now(); let iq = wacore::iq::keepalive::KeepaliveSpec::with_timeout(KEEP_ALIVE_RESPONSE_DEADLINE) .build_iq(); match self.send_iq(iq).await { Ok(response_node) => { let rtt = rtt_start.elapsed(); - let rtt_ms = rtt.as_millis() as i64; + let end_ms = wacore::time::now_millis(); + let skew_rtt_ms = end_ms.saturating_sub(start_ms); debug!(target: "Client/Keepalive", "Received keepalive pong (RTT: {rtt:.2?})"); self.unified_session.update_server_time_offset_with_rtt( response_node.get(), start_ms, - rtt_ms, + skew_rtt_ms, ); KeepaliveResult::Ok }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keepalive.rs` around lines 68 - 85, The keepalive path currently passes a monotonic RTT (computed from rtt_start.elapsed() -> rtt_ms) into unified_session.update_server_time_offset_with_rtt, which mixes clock domains; instead capture a wall-clock end timestamp (e.g. end_ms = wacore::time::now_millis()), compute the wall-clock RTT as end_ms - start_ms (or compute the midpoint as (start_ms + end_ms)/2) and pass that wall-clock-derived value into update_server_time_offset_with_rtt while keeping the monotonic rtt (rtt_start.elapsed()/rtt_ms) only for logging/health checks; update the call sites in keepalive (send_iq/response handling) to use the new wall-clock-derived value and do not change how rtt_start or rtt_ms are used for logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/time.rs`:
- Around line 106-113: The MonotonicProvider trait promises non-decreasing
values from now_nanos but the current wasm default falls back to a wall-clock
that can go backwards; update the wasm default implementation (the provider
installed for wasm targets in the module around the provider initialization code
at lines ~145-158) to use a truly monotonic source (e.g. web performance.now via
web_sys or an equivalent monotonic timer) and convert it to nanoseconds for
now_nanos, or alternatively change the public contract by removing the
non-decreasing guarantee from MonotonicProvider; pick one: either (a) replace
the wall-clock fallback with a performance.now()-based provider so
MonotonicProvider::now_nanos remains monotonic, or (b) relax the trait
docs/contract and adjust any callers relying on monotonicity. Ensure references
to MonotonicProvider and now_nanos are updated accordingly.
---
Outside diff comments:
In `@src/keepalive.rs`:
- Around line 68-85: The keepalive path currently passes a monotonic RTT
(computed from rtt_start.elapsed() -> rtt_ms) into
unified_session.update_server_time_offset_with_rtt, which mixes clock domains;
instead capture a wall-clock end timestamp (e.g. end_ms =
wacore::time::now_millis()), compute the wall-clock RTT as end_ms - start_ms (or
compute the midpoint as (start_ms + end_ms)/2) and pass that wall-clock-derived
value into update_server_time_offset_with_rtt while keeping the monotonic rtt
(rtt_start.elapsed()/rtt_ms) only for logging/health checks; update the call
sites in keepalive (send_iq/response handling) to use the new wall-clock-derived
value and do not change how rtt_start or rtt_ms are used for logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2edaf96d-6de7-43cf-bd6c-deed49de5676
📒 Files selected for processing (3)
src/handlers/message.rssrc/keepalive.rswacore/src/time.rs
…k RTT for skew The WASM default for MonotonicProvider derived from now_millis() and inherited any wall-clock backjumps, violating the trait's non-decreasing guarantee. Clamp the returned value to be at least the previous one via an AtomicU64; resolution is unchanged but the contract holds. In keepalive, the rtt fed into update_server_time_offset_with_rtt now also comes from the wall clock so the WA Web onClockSkewUpdate formula (start_ms + rtt/2 vs serverTime) stays in a single domain. The monotonic Instant is kept for the human-readable log line.
wacore::time::Instantwas backed bynow_millis(), which is the wall clock. This conflated two distinct concerns and produced two real symptoms:1.00msbecause actual send latency (~700us-1.5ms) lands across one ms boundary. Empirically verified against the production log (Send took ...lines): 100% of values are integer multiples of 1ms.time.rsacknowledged this: "not truly monotonic but sufficient ...".std::time::InstantseparatesSystemTimefor the same reason; this PR mirrors that split.Changes
MonotonicProvidertrait alongside the existingTimeProvider.StdMonotonicProviderwrapsstd::time::Instant::now()— the only legitimate call to it in the codebase (clippy.toml continues to forbid it elsewhere). Truly monotonic, ns resolution.WallDerivedMonotonicProviderfalls back tonow_millis() * 1_000_000. Embedders should register a real provider viaset_monotonic_provider(performance.nowin browsers,process.hrtime.bigintin Node,wasi:clocks/monotonic-clockin WASI).Instantnow storesu64nanoseconds (~584 years before overflow). Public API unchanged:now()/elapsed()/saturating_duration_since/Add<Duration>/Sub<Instant>all preserved. No call site needs to migrate.set_time_providerstill controls the wall clock only.set_monotonic_provideris the new knob forInstant.Migrating two manual elapsed measurements
Two call sites computed elapsed time by diffing two
now_millis()samples. Both are now usingInstant:src/keepalive.rs— keepalive RTT wasend_ms - start_ms. NowInstant-based. The wall-clockstart_msis still captured becauseupdate_server_time_offset_with_rttneeds to compare with the server's Unix-epoch frame, but the RTT itself is monotonic.src/handlers/message.rs— per-message processing time vsMAX_MESSAGE_DELAY_MSswitched toInstant. Pure elapsed-time use case.Other
now_millis()call sites are wire-protocol timestamps (sender_timestamp_ms,timestamp_ms, etc.), persisted TTLs, or last-seen markers — wall clock is correct there.Visible effect
The bot's pong message that was always
1.00mswill now show real sub-ms latency (687.43us,1.21ms, etc.). Same for the keepalive log line.Test plan
cargo fmt --allcargo clippy --all-targets -- -D warningscargo build --workspace --exclude e2e-tests --all-targetscargo test --workspace --exclude e2e-tests(659 wacore + 448 whatsapp-rust + others, 0 failures)