Order messages by sub-second send time - #249
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56a58c1aef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const latest = new Map<string, number>(); | ||
| let lastSent = 0; |
There was a problem hiding this comment.
Move these watermarks under a relay-session owner or include the community origin and viewer in their keys. The app retains multiple community sessions, and docs/communities.md:50-59 explicitly states that channel IDs alone are not sufficient keys; with this module-global map and lastSent, observing channel c in community A changes the signed ms/created_at allocated for channel c in community B, potentially pushing B's messages several seconds into the future or into the clamp path.
AGENTS.md reference: AGENTS.md:L39-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Agreed. In bfe8e61 the module globals are replaced by a MessageClock that createRelaySession creates once and shares with its outbox, channel windows and thread views. Channel keys therefore never mix across community sessions. The regression test runs two sessions with the same channel id, and it fails if the session goes back to a shared clock. This also fixes an intermittent mentions-live failure caused by watermarks leaking between tests.
| /** Allocate a send time after everything known in the channel, bounded by the local clock. */ | ||
| export function nextMessageMs(channelId: string, now = Date.now()) { | ||
| const after = Math.max(latest.get(channelId) ?? 0, lastSent) + 1; | ||
| const ms = after - now > MAX_LEAD_MS ? now : Math.max(now, after); |
There was a problem hiding this comment.
When a same-channel event is more than five seconds ahead—because of a skewed peer clock or a local clock rollback—this branch returns now, but observeMessageMs cannot replace the still-future latest value. Multiple sends in the same millisecond therefore all clamp to the identical ms, leaving their order to random event IDs and recreating the rapid-fire reordering this change is meant to prevent; maintain a separate bounded local watermark after entering the clamp path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Agreed, same root cause as Wes's comment. Fixed in bfe8e61 with a separate local send watermark, so clamped sends in one millisecond stay strictly increasing.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes at 56a58c1aef2660ed8833cb9696f6689be1a33a28: one allocator correctness issue, detailed inline. Merge criteria: preserve local send order at/over the lead cap and cover the boundary through rendered-row ordering and optimistic-to-relay replacement.
Validation: source review across signing, channel/thread/media rendering, persistence and relay cursors; exact-source Node probes reproduce the allocator failure. No local suite, running-app or live-relay validation. Hosted checks currently show only Semgrep OSS, zizmor and DCO passes; the branch conflicts with main, so conflict resolution and fresh CI remain separate merge gates.
Non-blocking: unread/activity “latest” selectors still use seconds/lowest-ID order (unread.ts:355–358,438–461); their previews can disagree with the new timeline. Untagged clients still sort at the start of their second, as documented. Neither expands the required fix here.
| export function nextMessageMs(channelId: string, now = Date.now()) { | ||
| const after = Math.max(latest.get(channelId) ?? 0, lastSent) + 1; | ||
| const ms = after - now > MAX_LEAD_MS ? now : Math.max(now, after); | ||
| lastSent = ms; | ||
| observeMessageMs(channelId, ms); |
There was a problem hiding this comment.
[P2] Preserve local send order when the five-second budget runs out
The fallback can assign a later send an earlier signed timestamp. With observeMessageMs(channel, now + 4999), two calls to nextMessageMs(channel, now) return now + 5000 and then now. I ran the exact-head functions: compareMessages renders those two rows as 2, 1. The existing cap test asserts those allocations but never checks their combined order. Since the outbox uses this value for both created_at and the signed ms tag, relay replacement cannot repair the inversion.
The same fallback also ignores the local increment while an observed timestamp remains beyond the cap: observing now + 60000 makes a same-clock burst return [now, now, now], restoring arbitrary event-ID ordering. Choose a bounded policy that does not rewind or discard local send ordering, and add near-cap/over-cap burst coverage asserting rendered order before and after relay replacement.
There was a problem hiding this comment.
🤖 Fixed in bfe8e61. The allocator now keeps two watermarks per channel:
- Other authors' evidence is capped at now+5s.
- This device's own sends always get strictly increasing times, up to a 60s lead. Past that (a clock rollback), the chain restarts at the clock.
So a near-cap burst gives now+5000, +5001, +5002, and so does an over-cap burst (evidence at now+60s). Neither rewinds nor ties. New tests assert rendered row order for a near-cap burst through MessageProjection, both for the optimistic rows and after replacement with the signed relay events, plus a rollback case. Reverting to the old rule fails all three.
56a58c1 to
bfe8e61
Compare
|
🤖 Rebased onto main (the only conflict was the new overlay-depth constants next to the sort in Local checks on bfe8e61, all passing:
On the rebased code before the fixes, Playwright had 4/634 failures. Rerunning those two specs:
This PR doesn't change the non-blocking unread/activity "latest" selectors, which still sort by second. That's a follow-up. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Scoped re-review clear at bfe8e6131a0934ebaf8709033653205938d9bd25. The prior near-cap rewind/over-cap tie blocker is repaired: separate local-send watermarks preserve live-session burst order, and signed event IDs keep that order through optimistic-to-relay replacement. The per-session clock is consistently shared by the outbox and channel/thread owners. This is a COMMENTED review, not approval or merge clearance.
Remaining gates and limits:
- The PR has merge conflicts. Existing CI 36083284018, attempt 3, passes JavaScript, Rust, all Chromium/WebKit shards, measurements and CI required against the historical merge with
b276b867d91646522c6ed354cfbedffb1e101882, not current main. Resolve conflicts and validate the resulting head. Windows was skipped; I initiated no rerun. - Non-blocking coverage improvement: the clock tests seed peer evidence directly, while the session-isolation test sends into empty channels. Add an integration case that loads peer evidence through the real channel/thread owner, then sends, to protect the shared-clock wiring. These are source observations, not executed mutation tests.
- This clearance is for the agreed live-session burst contract, not durable monotonicity across reconnect/restart or clock rollback. Replacing a session resets its local-send watermark; restored outbox events do not directly seed it. Source review only: no PR code/tests, running-app or live-relay/restart validation. Existing seconds/ID paging and mixed-client limitations remain unchanged.
The previous changes-requested review remains in GitHub; this follow-up does not dismiss it.
Add an ms tag to every channel/thread message sent through the outbox and order the channel timeline, threads, history, live merge and optimistic rows with one comparator: effective ms ascending, then event id ascending. Signed-off-by: Bradley Axen <baxen@squareup.com>
- Replace the module-global watermarks with a MessageClock owned by each relay session and shared with its outbox, channel windows and thread views, so channel ids never mix across communities or viewers. - Other authors' timestamps may still lead the clock by at most 5s, but this device's own sends in a channel now stay strictly increasing (up to a 60s lead), so a near- or over-cap burst never rewinds or ties. - Cover near-cap and over-cap bursts through optimistic and relay rows, clock rollback, and per-session isolation via createRelaySession. Signed-off-by: Honey <44b804aa12cc643328829fa31c020063ef3636c3a73c38ca49641c7626c11bb1@buzz.block.builderlab.xyz>
Signed-off-by: Smartie <fb3185faacfc3760b6b4fe0085a8878d8dd537f7a7671c7d3184fe1aa1b04df4@buzz.block.builderlab.xyz>
372d48c to
f22289e
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Scoped re-review clear: head f22289ec4afbeb89bc2965128af64e3b60a1f7ab, base 64be4c2adf21e9920d4a7666354ceab1d784dd44. No new blocker found in the 0–999 offset encoding or rebase; the prior live-session burst repair remains intact. COMMENTED, not approval or whole-feature merge clearance.
Two non-blocking follow-ups are detailed inline: split-second history paging and thread-root clock seeding. Both predate this follow-up. Paging is a regression versus main, but I am retaining the prior scoped deferral, not claiming history ordering is fully solved. Durable restart/reconnect ordering and mixed-client chronology also remain outside this clearance.
Validation: source review and focused production-owner probes. A real local HTTP broker with a scripted relay socket preserved signed identity and order across a second rollover. Session probes reproduced both follow-ups. No browser/DOM or live-relay acceptance, broad local suite rerun, or tracked edits. Full diff has no added screenshots/generated artifacts.
Remaining evidence: latest hosted snapshot has JavaScript, Rust/tool integration, measurements, Chromium shard 2 and security/DCO checks passing; five browser shards are running, Windows skipped. Complete applicable CI and running-app/human acceptance or obtain an explicit waiver. This review neither approves nor dismisses existing reviews.
| return rows.sort( | ||
| (a, b) => a.createdAt - b.createdAt || b.id.localeCompare(a.id), | ||
| ); | ||
| return rows.sort(compareMessages); |
There was a problem hiding this comment.
Deferred, non-blocking: split-second pages do not preserve prepend-only rendering.
The channel relay still pages by (created_at desc, id asc), while this comparator is (effective ms asc, id asc). In a production-session/scripted-relay probe with 25 untagged rows in one second, the 20-row head excludes the final five rows in the new rendered order; loadOlder() appends them at indices 20–24. Splitting an older second also inserts rows mid-list. “Final” here means comparator order, not unknowable legacy send chronology. Base prepended those rows.
This predates the offset/rebase follow-up and remains within the prior scoped paging deferral. The earlier review did not spell out this consequence; that omission is mine. ChannelTimeline’s prepend detection assumes first/last-edge insertion; scrolling impact is source-inferred, not DOM-reproduced. A follow-up should reconcile page boundaries with rendered order within existing budgets and test head completeness/anchoring. Keep relay cursor ordering unchanged; changing the client cursor comparator alone is incompatible.
| ) | ||
| .sort((a, b) => a.createdAt - b.createdAt || a.id.localeCompare(b.id)); | ||
| .sort(compareMessages); | ||
| for (const row of nextReplies) |
There was a problem hiding this comment.
Optional: seed the shared clock from the displayed root too.
This loop only observes replies. With thread-only/exact entry and no channel window, a displayed root at S.900 followed by a first local reply at S.100 allocates S.100; an inline session-channel fold renders the reply before its root. I reproduced the same outcome with a root 3.3 seconds ahead. This predates the encoding/rebase follow-up and is not reopening the agreed burst fix.
Observe readable rather than just nextReplies, and cover thread-only entry through the real session owner. A scratch-only version of that change makes both probe cases allocate root+1; the review checkout was not modified. The normal thread panel keeps its root separate, so the demonstrated inversion concerns inline rendering.
Summary
Messages sent in the same second (e.g. rapid-fire "yeah / nice / love it") could render out of order, because Nostr
created_athas whole-second resolution and ties fell back to event id. This adds a signed sub-second send time and a single comparator for all message views.Changes
src/features/relay/message-order.tseventMs: reads the event's["ms", "<0–999>"]subsecond tag and reconstructscreated_at * 1000 + offset. Only canonical decimal offsets are honored. Otherwise it falls back tocreated_at * 1000.compareMessages: effective ms ascending, then id ascending. This is now the only sort for rendered message rows.MessageClock: one clock per relay session, with per-channel observed and local-send watermarks. Other authors may lead allocation by at most 5s; the local send chain stays increasing up to a 60s lead.outbox.ts:outbox.sendadds thems % 1000tag and setscreated_at = floor(ms/1000)before the id is computed. This applies to channel-row kinds (9 / 40002 / 40008 / 40099) with anhtag. The optimistic row and the signed event are identical, so rows don't move when the relay acknowledges them.fold.ts,message-projection.ts,threads.ts,MediaReviewViewer.tsx: usecompareMessages. This removes the channel view's descending-id tiebreak, so the channel and thread views now agree. Each also records observed ms for send allocation.contracts.ts/row-identity.ts: optionalcreatedAtMsonChannelMessage.created_atpaging is untouched. Thread paging cursors still use(created_at, id)to match the relay.Known limits
Testing
message-order.test.ts:node --test tests/integration/*.test.mjs: 127pnpm typecheck,pnpm lint,pnpm checkSubsecond encoding follow-up and rebase
f22289ec4afbeb89bc2965128af64e3b60a1f7ab, rebased onto main64be4c2awith original authors and DCO trailers preserved.e52ec14freproduced two baseline failures. Main's agent-delete test fix was incorporated; the feedback-broker failure did not recur in the final full run (not claiming it is fixed).