Private
Public Access
Fix chat/room ordering that could differ between devices (#45)
Two independent gaps, both fixed since the report was ambiguous about which "chats" meant: - ChatPane.tsx concatenated history (REST-fetched) and live (WS-pushed) without sorting, so anything that could desync receipt order from send order -- a rejoin/resync racing a still-in-flight WS message, which opening the same room on another device triggers directly via a fresh socket connection -- could render messages out of chronological order. Now sorted by created_at (stable sort, so same-timestamp messages keep their relative order). - list_member_rooms/list_open_rooms/list_recent_messages ordered by created_at alone, with no secondary tiebreaker. Postgres doesn't guarantee a stable order for tied rows across separate query executions, so two rooms/messages sharing an identical timestamp (a real possibility -- rapid sends, bulk-created rooms) could come back in a different order on two separate fetches, i.e. two devices. Added id as a secondary sort key everywhere this showed up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -65,7 +65,12 @@ async def list_recent_messages(
|
||||
select(Message)
|
||||
.where(Message.room_id == room_id)
|
||||
.options(selectinload(Message.user), selectinload(Message.file))
|
||||
.order_by(Message.created_at.desc())
|
||||
# Secondary key on the primary key -- two messages can share the
|
||||
# same created_at (rapid sends, e.g. from different clients or a
|
||||
# webhook), and without a tiebreaker Postgres isn't obligated to
|
||||
# return them in the same relative order on every call, which can
|
||||
# look like messages swapping places between fetches/devices.
|
||||
.order_by(Message.created_at.desc(), Message.id.desc())
|
||||
.limit(limit)
|
||||
)
|
||||
messages = list(result.scalars().all())
|
||||
|
||||
@@ -71,7 +71,7 @@ async def list_open_rooms(db: AsyncSession, user_id: uuid.UUID) -> list[tuple[Ro
|
||||
select(Room)
|
||||
.where(Room.is_private.is_(False), Room.is_archived.is_(False))
|
||||
.options(selectinload(Room.memberships))
|
||||
.order_by(Room.created_at)
|
||||
.order_by(Room.created_at, Room.id)
|
||||
)
|
||||
rooms = result.scalars().all()
|
||||
return [
|
||||
@@ -108,7 +108,13 @@ async def list_member_rooms(
|
||||
)
|
||||
.join(RoomMembership, RoomMembership.room_id == Room.id)
|
||||
.where(RoomMembership.user_id == user_id)
|
||||
.order_by(Room.created_at)
|
||||
# A secondary key on the primary key -- without it, Postgres has no
|
||||
# obligation to return two same-instant rooms (a plausible tie:
|
||||
# bulk-created/migrated rooms, or just two created in quick
|
||||
# succession) in the same order on every call, which without a
|
||||
# stable order can visibly reshuffle the sidebar between one
|
||||
# device's fetch and another's.
|
||||
.order_by(Room.created_at, Room.id)
|
||||
)
|
||||
return [
|
||||
(room, role, last_message_at is not None and last_message_at > last_read_at, has_mention)
|
||||
|
||||
@@ -27,6 +27,29 @@ async def test_create_room_creates_owner_membership(client, db_session):
|
||||
assert membership.role == RoomRole.owner
|
||||
|
||||
|
||||
async def test_my_rooms_tiebreaks_identical_created_at_by_id(client, db_session):
|
||||
# Two rooms sharing the exact same created_at (bulk-created/migrated
|
||||
# rooms, or just an unlucky timing collision) must still come back in a
|
||||
# single, stable order every call -- see list_member_rooms's order_by
|
||||
# comment. Without a secondary sort key, a second fetch (e.g. from a
|
||||
# different device) isn't guaranteed to return ties in the same order.
|
||||
await register_and_login(client, db_session, username="alice")
|
||||
room_a = (await client.post("/api/rooms", json={"name": "room-a"})).json()
|
||||
room_b = (await client.post("/api/rooms", json={"name": "room-b"})).json()
|
||||
|
||||
result = await db_session.execute(
|
||||
select(Room).where(Room.id.in_([uuid.UUID(room_a["id"]), uuid.UUID(room_b["id"])]))
|
||||
)
|
||||
rooms_by_id = {str(r.id): r for r in result.scalars().all()}
|
||||
rooms_by_id[room_a["id"]].created_at = rooms_by_id[room_b["id"]].created_at
|
||||
await db_session.commit()
|
||||
|
||||
resp = await client.get("/api/rooms/mine")
|
||||
assert resp.status_code == 200
|
||||
ids = [r["id"] for r in resp.json() if r["id"] in (room_a["id"], room_b["id"])]
|
||||
assert ids == sorted([room_a["id"], room_b["id"]])
|
||||
|
||||
|
||||
async def test_list_rooms_excludes_private(client, db_session):
|
||||
user = await register_and_login(client, db_session, username="alice")
|
||||
await client.post("/api/rooms", json={"name": "open-room"})
|
||||
|
||||
@@ -64,6 +64,51 @@ def test_ws_join_and_message_roundtrip(ws_client):
|
||||
assert any(m["content"] == "hello" and m["username"] == username for m in history)
|
||||
|
||||
|
||||
def test_history_tiebreaks_identical_timestamps_by_id(ws_client):
|
||||
# Two messages sharing the exact same created_at (a real possibility --
|
||||
# rapid sends, a webhook, microsecond-precision collisions under load)
|
||||
# must still come back in a single, stable order every call, not
|
||||
# whatever order Postgres feels like giving ties with no secondary sort
|
||||
# key -- see list_recent_messages's order_by comment.
|
||||
username = _unique("alice")
|
||||
_register(ws_client, username=username)
|
||||
room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json()
|
||||
|
||||
with ws_client.websocket_connect("/ws/chat") as ws:
|
||||
ws.send_json({"type": "join", "room_id": room["id"]})
|
||||
assert ws.receive_json()["type"] == "joined"
|
||||
ws.send_json({"type": "message", "room_id": room["id"], "content": "first"})
|
||||
first = ws.receive_json()
|
||||
ws.send_json({"type": "message", "room_id": room["id"], "content": "second"})
|
||||
second = ws.receive_json()
|
||||
|
||||
async def _force_same_timestamp():
|
||||
from sqlalchemy import select
|
||||
|
||||
from app.models import Message
|
||||
|
||||
async with ws_client.session_factory() as session:
|
||||
result = await session.execute(
|
||||
select(Message).where(Message.id.in_([uuid.UUID(first["id"]), uuid.UUID(second["id"])]))
|
||||
)
|
||||
rows = {str(m.id): m for m in result.scalars().all()}
|
||||
rows[first["id"]].created_at = rows[second["id"]].created_at
|
||||
await session.commit()
|
||||
|
||||
ws_client.portal.call(_force_same_timestamp)
|
||||
|
||||
resp = ws_client.get(f"/api/rooms/{room['id']}/messages")
|
||||
assert resp.status_code == 200
|
||||
history = [m for m in resp.json() if m["content"] in ("first", "second")]
|
||||
assert len(history) == 2
|
||||
|
||||
# order_by(created_at.desc(), id.desc()) then reversed for display --
|
||||
# with a tied created_at, the higher id (whichever message that is)
|
||||
# must consistently render second.
|
||||
expected_order = sorted([first, second], key=lambda m: m["id"])
|
||||
assert [m["content"] for m in history] == [m["content"] for m in expected_order]
|
||||
|
||||
|
||||
def test_ws_message_without_join_errors(ws_client):
|
||||
_register(ws_client, username=_unique("alice"))
|
||||
room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json()
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useCallback, useEffect, useState } from 'react'
|
||||
import { useCallback, useEffect, useMemo, useState } from 'react'
|
||||
import { NetworkError } from '../api/client'
|
||||
import { getRoomMessages, markRoomRead } from '../api/rooms'
|
||||
import type { ChatSocketHandle } from '../ws/useChatSocket'
|
||||
@@ -127,6 +127,22 @@ export function ChatPane({
|
||||
[socket, room.id, refreshHistory, markRead],
|
||||
)
|
||||
|
||||
// history and live are just concatenated, not merge-sorted -- live is
|
||||
// strictly receipt order, which isn't always send order. A rejoin (a
|
||||
// reconnect, or opening the same room on another device) refetches
|
||||
// history but doesn't guarantee anything about the timing of whatever
|
||||
// WS messages land in live afterward relative to it, so without this
|
||||
// sort a message can render above one that was actually sent earlier.
|
||||
// Stable sort (guaranteed since ES2019) keeps same-timestamp messages in
|
||||
// their original relative order rather than shuffling them.
|
||||
const messages = useMemo(
|
||||
() =>
|
||||
[...history, ...live].sort(
|
||||
(a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime(),
|
||||
),
|
||||
[history, live],
|
||||
)
|
||||
|
||||
const connected = socket.connected
|
||||
const send = useCallback(
|
||||
(content: string, imageId?: string, fileId?: string) => socket.send(room.id, content, imageId, fileId),
|
||||
@@ -179,7 +195,7 @@ export function ChatPane({
|
||||
|
||||
<MessageList
|
||||
roomId={room.id}
|
||||
messages={[...history, ...live]}
|
||||
messages={messages}
|
||||
members={members}
|
||||
onEdit={sendEdit}
|
||||
onReact={sendReaction}
|
||||
|
||||
Reference in New Issue
Block a user