perf(send): stamp one message from one clock read - #1106
Conversation
Sending a direct message read the wall clock four times on the client's own thread: once for the id, once for the biz node, once for the privacy-token decision and once for the outbound message secret, plus a second privacy-token read whenever a token already exists. They all describe the same instant, and the code already knew it: the message-secret site commented that it wants "the parent event time" and re-read the clock only because it had no way to receive one. Sample it once as `SendInstant` where the operation starts and carry it down. Besides the reads, this makes the stamps coherent: four reads microseconds apart can straddle a second boundary and leave one message described by different seconds in the id, the stanza and the store. The instant reaches its consumers through `_at` variants of the existing helpers, matching the shape already used by `should_send_new_tc_token_at` and by `is_dead_socket_at` from #1103. Nothing is added to the public API as a required parameter. Left alone, with the reasoning recorded where a reader will find it: the two monotonic reads of the device-registry lookups (sharing them needs a clock-taking method on public cache types for one read), the chat-lane latency guard (sampling or gating on backlog would stop reporting the single slow message it exists to catch), and the per-frame session-stats bookkeeping.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe send pipeline now samples one wall-clock instant and reuses it for message IDs, business-node timestamps, tc-token decisions, and outbound-secret persistence. Timestamp-aware helpers, updated callers, tests, and documentation support the single-instant model. ChangesSingle send instant
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Sender
participant SendPipeline
participant RequestUtils
participant TcTokenLifecycle
participant SecretStore
Sender->>SendPipeline: start send
SendPipeline->>SendPipeline: sample SendInstant
SendPipeline->>RequestUtils: generate message ID at sampled seconds
SendPipeline->>TcTokenLifecycle: evaluate tc-token state at sampled seconds
SendPipeline->>SecretStore: persist outbound secret at sampled seconds
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| src/send/mod.rs | Introduces and propagates the shared send instant through the main send pipeline and outbound-secret persistence. |
| src/send/tctoken_lifecycle.rs | Uses the send instant consistently for trusted-contact-token issuance and expiration decisions. |
| wacore/src/iq/tctoken.rs | Adds timestamp-injected token helpers while retaining behavior-compatible clock-reading wrappers. |
| wacore/src/request.rs | Adds timestamp-injected message-ID generation and preserves the existing public convenience method. |
| src/features/comments.rs | Adapts comment-secret persistence to the new explicit timestamp parameter. |
| src/message/tests.rs | Updates message-secret tests for the explicit send-instant contract. |
| src/request.rs | Adds the client-level timestamp-injected message-ID helper used by the send pipeline. |
| src/handlers/message.rs | Documents why chat-lane latency measurement retains separate monotonic clock reads. |
| wacore/src/stats.rs | Clarifies the per-message transport-event clock-read cost model. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Send as Send pipeline
participant Metadata
participant Token as Privacy-token logic
participant Store as Secret store
Caller->>Send: send message
Send->>Send: SendInstant::now()
Send->>Metadata: generate ID and biz node at instant
Send->>Token: evaluate token state at instant
Send->>Store: persist outbound secret at instant
Reviews (2): Last reviewed commit: "fix(send): stamp the fallback id from th..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/send/mod.rs (1)
1731-1735: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate the original
SendInstantthrough every send continuation.The fallback ID path, async tc-token issuance, and comment-secret persistence resample the wall clock. This breaks the one-read/coherent-stamp contract, especially around second and token-bucket boundaries.
src/send/mod.rs#L1731-L1735: generate fallback IDs withself.generate_message_id_at(sent_at.unix_secs_u64()).src/send/mod.rs#L1873-L1882: passsent_atinto an_atissuance path; use it for the privacy IQ timestamp and sender-timestamp write.src/features/comments.rs#L111-L124: retain the instant sampled for the comment send and reuse it when persistingcomment_secret, rather than callingSendInstant::now()afterward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/send/mod.rs` around lines 1731 - 1735, Propagate the original SendInstant through all send continuations: in src/send/mod.rs lines 1731-1735, use sent_at.unix_secs_u64() with generate_message_id_at for fallback IDs; in src/send/mod.rs lines 1873-1882, pass sent_at to the _at token-issuance path and reuse it for the privacy IQ timestamp and sender-timestamp write; in src/features/comments.rs lines 111-124, retain the instant sampled for the comment send and reuse it when persisting comment_secret instead of calling SendInstant::now().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/send/mod.rs`:
- Around line 1731-1735: Propagate the original SendInstant through all send
continuations: in src/send/mod.rs lines 1731-1735, use sent_at.unix_secs_u64()
with generate_message_id_at for fallback IDs; in src/send/mod.rs lines
1873-1882, pass sent_at to the _at token-issuance path and reuse it for the
privacy IQ timestamp and sender-timestamp write; in src/features/comments.rs
lines 111-124, retain the instant sampled for the comment send and reuse it when
persisting comment_secret instead of calling SendInstant::now().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 027e0682-1ce5-4471-99e0-56444d326105
📒 Files selected for processing (9)
src/features/comments.rssrc/handlers/message.rssrc/message/tests.rssrc/request.rssrc/send/mod.rssrc/send/tctoken_lifecycle.rswacore/src/iq/tctoken.rswacore/src/request.rswacore/src/stats.rs
The module doc pointed at `SendInstant`, which is crate-private, so rustdoc rejected the link.
📦 Binary size report
.text per crate
Baseline: |
`send_message_impl` still read the clock for its own fallback id, so the entry points that do not come through `send_message_with_options_inner` kept two reads instead of one. Also drop the module doc's intra-doc link to `SendInstant`: the type is crate-private, which rustdoc rejects.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Took the first of the three, skipped the other two.
Not taking the tc-token issuance one: Not taking the comments one either: the comment secret is persisted after Also fixed the rustdoc failure in the same push: the module doc linked |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/send/mod.rs (1)
2541-2561: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the sampled instant for external persistence callers.
src/features/comments.rs:54-127callssend_message(...), then passesSendInstant::now()here. That performs a second wall-clock read and can give the comment secret a latermessage_ts/expiry than the message’s actual send instant, breaking the one-send/one-instant contract. Return the sampled instant with the send result, or expose an API that lets the caller reuse it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/send/mod.rs` around lines 2541 - 2561, Update the send flow and persist_outbound_msg_secret integration so external callers reuse the instant sampled during send_message rather than calling SendInstant::now() afterward. Return the sampled SendInstant with the send result, or expose an equivalent API, and ensure features/comments.rs passes that same instant to persistence while preserving the one-send/one-instant contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/send/mod.rs`:
- Around line 2541-2561: Update the send flow and persist_outbound_msg_secret
integration so external callers reuse the instant sampled during send_message
rather than calling SendInstant::now() afterward. Return the sampled SendInstant
with the send result, or expose an equivalent API, and ensure
features/comments.rs passes that same instant to persistence while preserving
the one-send/one-instant contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd2c1210-da7c-4da8-a251-6a1c388e06fa
📒 Files selected for processing (1)
src/send/mod.rs
1 participant
Summary
Follow-up to #1103, applied to the send path. Sending one direct message read the wall clock four times on the client's own thread for what is a single logical instant: the message id, the biz node, the privacy-token decision and the outbound message secret. Sample it once as
SendInstantand carry it down. Besides the reads, this fixes an incoherence: four reads microseconds apart can straddle a second boundary and leave one message described by different seconds in its id, its stanza and its store row.Audit
Measured with the
clock_readscounter #1103 added, attributing every read by stack over one steady-state send (registry, LID mapping and Signal sessions warm, one frame written):RequestUtils::generate_message_idsend_message_with_options_innershould_send_new_tc_token_withpersist_outbound_msg_secretSendInstant::nowNot visible in that harness but on the same instant:
is_tc_token_expired_withruns whenever a stored token exists, so a real send against a known contact read five times, not four. Both privacy-token decisions now take the shared instant.The code already knew these were one instant.
persist_outbound_msg_secretcarried a comment saying it wants "the parent event time" and re-read the clock only because it had no way to receive one.Changes
SendInstant, sampled at the top ofsend_message_with_options_innerand carried throughSendPipelineOptionsinto the DM branch, the privacy-token decision and the outbound secret.send_message_implsamples its own when a caller did not, so the other entry points keep exactly one read too.generate_message_id_at,should_send_new_tc_token_with_at,is_tc_token_expired_with_at:_atvariants that take the instant, matchingshould_send_new_tc_token_atand theis_dead_socket_atshape from perf(stats): stop dating every wire frame for a field nothing reads #1103. The clock-reading versions stay, so nothing becomes a required parameter of the public API.src/send/mod.rsgains the cost-model line the module lacked, which is how it accumulated four reads without anyone noticing.Left alone, and why
Each of these is recorded next to the code so the next reader does not redo the analysis.
PortableCacheandTypedCache, both public, and the parameter would be meaningless for store-backed caches whose TTL lives in the store. One read per send did not justify that.OnceLock, which is what pushed perf(stats): stop dating every wire frame for a field nothing reads #1103 to count at the boundary in the first place.SessionStatsper-frame and per-event bookkeeping. perf(stats): stop dating every wire frame for a field nothing reads #1103 rejected samplinglast_data_received_msbecause two decisions measure elapsed time from it, and rejected deriving the watchdog from tick-observed counters because it pushes worst-case detection from ~50s to ~80s. The new data (four events per message, not one) changes the size of the group, not either argument.Guarantees
a_send_reads_the_clock_onceasserts exactly one wall read across a send, which is the proof that every stamp shares an instant, and checks the outbound secret really landed inside the measured window.dm_send_stays_within_its_clock_budgetdrops its budget from 4 wall reads to 1 and fails if it grows.wire_timestamp_keeps_real_timestill proves a timestamp reaching the server carries the real second.supplied_instant_decides_the_same_bucket_boundarypins both privacy-token decisions at the exact bucket boundary against a supplied instant.PortableCacheTTL/TTI boundary test and the dead-socket tests from perf(stats): stop dating every wire frame for a field nothing reads #1103 pass unchanged.No embedder has to register a provider, change a provider signature, or keep time state of its own.
Validation
Full matrix left to CI.