Say where a conversation belongs when it is added, changed or removed by mpretty-cyro · Pull Request #144 · session-foundation/libsession-util

mpretty-cyro · 2026-09-08T00:24:15Z

The problem

A conversation's place in either list is decided by last_activity and priority. Storing a message
moves the first, and _flush_pending emitted conversation_updated for the row and stopped there —
so a subscriber was told the row had changed and never that it had moved.

Working the new position out on the other side of the callback means reimplementing this sort, and
the two lists are not sorted the same way — conversations are priority DESC, last_activity DESC, id
and requests are last_activity DESC, id with no priority term at all. A client that copies one
comparator gets the other list wrong. (Session's client had exactly that bug, and removing its
client-side sort is what surfaced this.)

What this does

The three events that change a list now say where the row belongs:

enum class ConversationList { none, conversations, requests };

struct ListPlacement {
    ConversationList from = ConversationList::none;   // where the subscriber is holding it
    ConversationList to   = ConversationList::none;   // where it belongs now
    std::optional<ConversationId> after;              // the row it follows; unset means first
};

std::function<void(AnyConversation&&, ListPlacement&&)>  conversation_added;
std::function<void(AnyConversation&&, ListPlacement&&)>  conversation_updated;
std::function<void(ConversationId&&, ConversationList)>  conversation_removed;

Enough to apply without consulting anything, and it reads as the two steps it is:

if (p.from != ConversationList::none) remove(p.from, convo.id());
if (p.to   != ConversationList::none) insert(p.to, std::move(convo), p.after);

none is a real answer rather than a missing one: a hidden conversation is in neither list, and so
is one the subscriber has not been shown. A removal carries only the list, because the other two
fields would always be empty there.

Why an anchor rather than the whole order

An earlier revision of this branch sent the whole ordered list of ids as its own event. Review asked
for an anchor on conversation_updated instead, and the measurements agree — per event, at five
thousand conversations:

what it takes to produce	cost
the rows (a full replacement)	4.85 ms
the ordered ids	1.85 ms
one anchor (an indexed seek on `conversations_order`)	0.11 ms

That revision is still in this branch's history and removed in its own commit, so the logic is
recoverable if the anchor turns out not to cover a case.

Why ids rather than indices

An index would be cheaper for a client to apply — measured in Node, applying one move at five
thousand conversations is 0.0007 ms by index against 0.049 ms by id. Three reasons it is
still the wrong trade:

It moves cost onto the wrong loop. Producing an index is "count everything that sorts before
this": 0.87 ms at five thousand, against 0.11 ms for an anchor. So it saves ~0.05 ms on the
client to spend ~0.75 ms here, on the loop that also polls and decrypts.
It fails silently. A stale index splices out whatever happens to sit at that position. An
unmatched id returns "not found", which a subscriber can act on.
It needs the whole ordered list to produce, which is the thing this change exists to avoid.

For scale, decoding and projecting five thousand rows was measured at 12.67 ms on an iPhone X, so
applying by id is still an order of magnitude cheaper than being handed the list.

`from` is what the subscriber was told, not what the database says

from is read from a record of where each conversation was last reported, which is what the
subscriber is actually holding. That matters when a row changes lists twice between flushes: the
database would answer about the last hop, and the subscriber is still holding it from before the
first.

It is recorded wherever a row is reported placed — including by a whole-list replacement, which
places every row it carries — and dropped when a row is hidden or removed. Keyed by ConversationId
rather than row id, because a removal is reported after the row is deleted, so there would be
nothing left to look a row id up from.

List replacements, while they are here

The replacement handlers were doing two things wrong, both found in review:

They read for handlers that were not there. Both list queries ran before _emit looked at
whether anyone was listening. Now nothing is read for a handler that is not registered — the same
guard is on _emit_conversation_added and _emit_message_alone.
They named both lists whatever had happened. Pinning a conversation re-read every message
request as well. Each caller now names the list it touched: pin and hide read the row's list with
one indexed query, the contacts reconcile already tracked the two separately, and the user-profile
reconcile is note to self, which cannot be a request. Approval and losing a contact still name
both, because those genuinely move a row from one list to the other.

A replacement is not suppressed when the order is unchanged. The order is not what it carries:
the row whose arrival brought us here has a new snippet, and for a subscriber holding no per-row
handler the list is the only thing carrying it.

What this asks of a subscriber, and a question for review

Each placement is relative to another row, so applying them in order is what keeps a list
correct — one applied out of order, or skipped, leaves the list wrong with nothing to detect it.
A subscriber also has to have read the list once with conversations() to have anything to place
rows into.

That is a stronger contract than "each handler is independently meaningful", and review has said
elsewhere that few handlers are strictly required. Worth settling explicitly: if a subscriber may
register conversation_updated without ever reading the list, an anchor naming a row it was never
told about is unresolvable, and the design needs a fallback. The handler docs currently state the
sequence contract, which is the assumption the code makes.

Testing

an update says where its row was and now belongs — pins a conversation first, so the next row to
move must land after the pinned one rather than at the top. Without the pin every case passes
trivially, since a new message always leads.
a hidden row is given no position — to == none, and from still names the list to take it out
of.
a request is placed in the request list.
a removal says which list to take it out of — deletes a contact, which removes the row before the
removal is reported, so it exercises the case a database lookup could not answer.
Plus the existing [client][signals] cases, updated for the new shape.

Anchors were checked against real data rather than only asserted: for a thousand conversations
and three hundred requests, every anchor is the row immediately before it in that list's ordering,
and a leading row reports none.

⚠️ The test suite has not been run locally. testAll does not compile on macOS for a
pre-existing reason unrelated to this change: session::client::wait (client/handler.hpp) collides
with POSIX wait() from <sys/wait.h>, so every tests/test_client file that calls wait fails
with error: reference to 'wait' is ambiguous. Compiling each affected object with
-ferror-limit=0 puts every error in that category or cascading from it, and none in the code this
branch adds. The library itself compiles and links clean. So the tests here rely on CI.

jagerman · 2026-09-09T18:09:33Z

I wonder if it might be easier to not add a new callback for this, but instead include the information in a "convo updated" callback: so you get the one that changed, along with info about where it now belongs.

That could be the entire ordered list, but I'm thinking it might be easier (for both backend and frontend) to simply include an anchor to the previous conversation, so that we get something like now_after=123 to signal that this conversation has moved to be immediately after a conversation with id=123.

jagerman · 2026-09-09T18:12:23Z

+    /// told about.  An id in here that the subscriber does not hold — or one it holds that is
+    /// absent — therefore means a notification was missed, and is worth treating as a reason to
+    /// re-read the list rather than as a state to reconcile.
+    std::function<void(std::vector<ConversationId>)> conversation_order_updated;


Now that #145 is merged this would be better as void(std::vector<ConversationId>&&) for consistency with everything else. (Might be a moot point given my other comment, but just flagging it for attention).

Actually in this case, given that I think it's passing a reference to the local memory, would be better as either const std::vector<ConversationId>& or std::span<const ConversationId>.

A conversation's place in either list is decided by `last_activity` and `priority`. Storing a message moves the first, and `_flush_pending` emitted `conversation_updated` for the row and stopped there -- so a subscriber was told the row had changed and never that it had moved. Working the new position out from that means reimplementing this sort on the other side of the callback, and the two lists are not sorted the same way: conversations are `priority DESC, last_activity DESC, id` and requests are `last_activity DESC, id` with no priority term at all. A client that copies one comparator gets the other list wrong. `_set_priority` already reports a move properly and says why: "Reported as a replacement, not as an update to the one conversation whose priority changed: what moved is the list." Sending and receiving were the two paths that moved an ordering term without saying so. But a replacement is the wrong instrument on this path, which is why this does not reuse it. A `conversation_list_replaced` carries whole rows -- fifteen fields including display name and snippet -- so one arriving message would send every field of every other row to describe a change to one of them, and would send the moved row's snippet twice, since `conversation_updated` has just carried it. Measured through the Session client's bridge at 5,000 conversations, that replacement costs 155 ms on an iPhone X; the ids alone cost 59 ms, and most of the remainder is unavoidable. So `conversation_order_updated` and `request_order_updated` carry that list's ids in their new order and nothing else. The division is that `conversation_updated` says what a row now contains and these say where the rows now are; a subscriber applying both ends up where a replacement would have put it. Replacements keep their existing job -- a change to the *contents* of a list, and membership changes like approval, which moves a row between the two and is still reported as both being replaced. Three things keep it cheap: - **Only when the order actually changed.** The ids are read, compared against what was last reported, and dropped if they match. This suppresses the common case outright: a message into the conversation already at the top of its list leaves every row exactly where it was, which is what a back-and-forth in an open conversation does. `_emit_lists_replaced` records what it sent too, since a replacement tells the subscriber the same thing about position -- without that, an order event matching an older belief would be wrongly suppressed. - **Only the list the row is in.** The two lists are strict complements, so a message can only move a row within the one it already sits in; reporting both would say one true thing and one false one. Which list is read off the row `_flush_pending` has already fetched, so it costs no extra query. - **Only when an ordering term moved, once per batch.** Twelve of the sixteen things that dirty a conversation -- a read receipt, a nickname, an expiry, an auto-download setting -- leave it where it was, and `_mark_read` alone runs every time a conversation is opened. Reported through `_touch_reordered` so a poll delivering fifty messages to one conversation reaches `_flush_pending` once, which is the same reason `_dirty` exists. The order events are emitted after the `conversation_updated` loop rather than before it, and the handler documents that as a guarantee: the ids always name conversations the subscriber has already been told about, so an id it does not recognise -- or one it holds that is absent -- means it missed a notification, and is a reason to re-read the list rather than a state to reconcile.

The rows a list holds and the order they are in were spelled out at four call sites: twice for the conversation list and twice for requests, once in the query that reads the rows and again in the one that reads only their order. Only the column sets and the request predicate were shared, so the part that actually has to agree was the part kept in agreement by hand. The comment on _conversation_order already claimed the queries match; nothing made that true. The record of what was last reported is taken from the list query while the comparison against it is made with the order query, so the two diverging would not raise anything -- it would silently suppress or invent an order event. ORDER_COLUMNS now carries its own join, as CONVO_COLUMNS already did, so a list query is a column set and a filter and nothing else.

_flush_pending reported a moved row through the order handlers alone, so a subscriber holding conversation_list_replaced and not conversation_order_updated was never told the row had moved. It kept whatever arrangement the last replacement left it in, and nothing said otherwise. Each handler is now sent when it is registered, and the query follows from that. A replacement carries the rows and their order is already in them, so a subscriber wanting one is served by the row query alone; asking for the ids as well would be a second query for something already held. A subscriber wanting only the order gets the id-only query, which is the cheaper of the two and the one that runs on every message. The replacement is not suppressed on an unchanged order. The order is not what it carries: the row whose arrival brought us here has a new snippet, and a subscriber holding only that handler has no other way to learn it, so comparing the order to decide whether to send the rows would leave it showing the previous message. Emitters also no longer read anything for a handler that is not there. _emit_lists_replaced ran both list queries, and _emit_conversation_added and _emit_message_alone each read their subject, before _emit looked at whether anyone was listening -- and that read is the cost of the operation. _emit_lists_replaced now records the order it reported only for a list it actually sent, since an unsent one told the subscriber nothing and the record has to keep describing what it holds.

The replacement was hung off `_dirty_order`, which `_touch_reordered` fills and only three call sites reach. `_touch` fills `_dirty` and sixteen reach it. So everything that changes a row without moving it -- a read receipt, a nickname, an expiry, and `mark_read`, which runs every time a conversation is opened -- reached `conversation_updated` and no further. A subscriber holding whole lists kept one whose unread counts and nicknames were stale, which is the same shape of bug as the one this branch set out to fix, from the other end. The two are different questions and now have different answers. A list is stale as soon as any row in it changed, so the replacement follows `_dirty`; only a row that moved can have changed the order, so the order event still follows `_dirty_order`. A hidden row is in neither list, so it now marks neither stale. No contents check beyond that. Holding the previous rows and comparing them costs 0.12 ms at five thousand conversations, which is affordable, but the flag above already means nothing is read unless a row in that list demonstrably changed -- and every such change shows up in the columns a list carries, so the comparison would find a difference every time it ran.

The priority check reads the row as it is now, so it cannot distinguish a row that was always hidden from one that has just been hidden -- and the second did change a list, by leaving it. The reason that case never arrives is four call sites away: every write to conversations.priority emits a replacement itself instead of dirtying the row, so the removal has been sent before the row reaches this loop. Nothing enforces that, and nothing here could detect it going wrong, so it is worth stating where the assumption is relied on.

A row appearing, going, or being pinned changes where every row below it sits, and the eight callers that report that -- pin, hide, delete conversation, delete contact, both config reconciles, and approval on a send in either direction -- sent replacements outright. A subscriber holding conversation_order_updated and no list handler was told nothing by any of them, and kept the arrangement it had until some later message happened to move something. So _emit_lists_replaced is now _report_lists_replaced, and goes through the same per-registration path as a moved row: both lists, changed and possibly moved. A subscriber wanting lists still gets lists; one wanting the order now gets the order; one wanting both gets both, and the order event is still dropped when the order did not actually change, which is why a settings-only reconcile does not send one. That also removes the second implementation of "read the lists and record what was reported" -- the two had already drifted once, in which of them recorded the order it sent.

Reporting a wholesale change read and sent both lists whatever had happened, so pinning a conversation also re-read every message request and sent that list too. At five thousand rows the unwanted half is a four-millisecond query and a list the subscriber has to diff for nothing. Each caller now says which lists it touched, and most of them know: - pin, hide: priority moves a row within the list it is in, never between the two, so one indexed read of the row says which. Cheap against the list query it saves. - the contacts reconcile already tracked the two separately and threw the distinction away at the call. - the user-profile reconcile is note to self, which cannot be a request. Approval and losing a contact keep naming both, because they are the cases that genuinely move a row from one list to the other.

Deleting a contact removes the conversation row outright, so it goes from the one list it was in and the other never held it. Reporting both was reasoning about the wrong moment: after the delete there is nothing left to classify, but before it there is, and that is when the question is worth asking. Read before either delete, and not merely before the conversation goes: whether it is a request comes from ct.approved, a column of the contact row deleted a line earlier, so asking afterwards answers about a relationship that no longer exists and calls every deleted conversation a request. A hidden conversation is in neither list, so its going now reports neither, where before it replaced both.

The list handlers took their payload by value when this branch was written and take it by rvalue reference on client now, so the member pointers _report_list is parameterised over named a function type that no longer exists, and the call site handed an lvalue to a handler that wants to move from it. Also what the review asked for, so this is the convention rather than a rebase artefact.

Review would rather the position travelled on conversation_updated as an anchor -- "this now sits after 123" -- than as a whole ordered list. The measurement agrees: at five thousand conversations the anchor is one indexed seek at 0.11 ms against 1.85 ms to read the ordered ids and 4.85 ms to read the rows, and the client changes an index rather than rebuilding a list. So conversation_order_updated and request_order_updated go, and with them everything that existed only to serve them: the record of what order was last reported, the two id-only queries and the columns they selected, and the tracking of which rows moved rather than merely changed. _touch takes back the three call sites that wanted the narrower one. What the review asked to keep stays: each list is still reported only if a row in it changed and only to a handler that asked for it, and a replacement is still not suppressed when the order is unchanged, because the row that brought us here has a new snippet and the list is what carries it. Removed as its own commit rather than left out of the rebase, so the logic is recoverable if the anchor turns out not to cover a case.

conversation_updated said what a row contains and nothing about where it sits, so a subscriber applying one had to sort for itself -- which it cannot do correctly, because the two lists are not ordered the same way. Adds and updates now carry a ListPlacement: which list the subscriber is holding the row in, which it belongs in now, and the row it follows. Enough to apply on its own -- take it out of `from`, put it into `to` after `after` -- so a subscriber never searches the list a row did not come from, and never works out where it was holding it. Removals carry just the list, since the other two fields would always be empty there. The anchor is one indexed seek against conversations_order: 0.11 ms at five thousand conversations, against 1.85 ms to read the ordered ids and 4.85 ms to read the rows. Ids rather than indices deliberately: an index is ~70x cheaper to apply but needs the whole ordered list to produce, puts 0.75 ms on this loop to save 0.05 ms on the client's, and fails silently when the two disagree, where an unmatched id fails loudly. `from` is what the subscriber was last *told*, not what the database now says, so it survives a row changing lists twice between flushes. It is recorded wherever a row is reported placed -- including by a whole-list replacement, which places every row it carries -- and dropped when a row is hidden or removed. Keyed by ConversationId and not by row id because a removal is reported after the row is deleted, so there would be nothing left to look a row id up from. Anchors checked against a thousand conversations and three hundred requests: every one is the row immediately before it in its list, and a leading row reports none.

mpretty-cyro changed the base branch from dev to client September 8, 2026 00:24

mpretty-cyro force-pushed the fix/announce-list-order-on-touch branch from fb51bde to dca046a Compare September 8, 2026 00:25

mpretty-cyro marked this pull request as ready for review September 8, 2026 01:12

mpretty-cyro force-pushed the fix/announce-list-order-on-touch branch from dca046a to afca3ba Compare September 8, 2026 01:17

mpretty-cyro changed the title ~~Announce the lists when a conversation is touched~~ Announce the lists when a message moves a conversation Sep 8, 2026

mpretty-cyro force-pushed the fix/announce-list-order-on-touch branch from afca3ba to adbd707 Compare September 8, 2026 01:25

mpretty-cyro marked this pull request as draft September 8, 2026 06:29

mpretty-cyro force-pushed the fix/announce-list-order-on-touch branch from adbd707 to 3e11b71 Compare September 8, 2026 06:43

mpretty-cyro changed the title ~~Announce the lists when a message moves a conversation~~ Report a conversation's new position without re-sending the list Sep 8, 2026

mpretty-cyro force-pushed the fix/announce-list-order-on-touch branch from 64ec030 to 80e593f Compare September 9, 2026 01:45

mpretty-cyro changed the title ~~Report a conversation's new position without re-sending the list~~ Add list order events, and report each list change per registration Sep 9, 2026

mpretty-cyro marked this pull request as ready for review September 9, 2026 04:08

mpretty-cyro requested a review from jagerman September 9, 2026 04:12

jagerman reviewed Sep 9, 2026

View reviewed changes

mpretty-cyro added 9 commits September 10, 2026 10:16

mpretty-cyro force-pushed the fix/announce-list-order-on-touch branch from 7fbff4c to 7b4114e Compare September 10, 2026 00:35

mpretty-cyro added 2 commits September 10, 2026 10:43

mpretty-cyro changed the title ~~Add list order events, and report each list change per registration~~ Say where a conversation belongs when it is added, changed or removed Sep 10, 2026

Navigation Menu

Uh oh!

Say where a conversation belongs when it is added, changed or removed - #144

Say where a conversation belongs when it is added, changed or removed#144
mpretty-cyro wants to merge 11 commits into
clientfrom
fix/announce-list-order-on-touch

mpretty-cyro commented Sep 8, 2026 •

edited

Loading

Uh oh!

jagerman commented Sep 9, 2026

Uh oh!

jagerman Sep 9, 2026

Uh oh!

jagerman Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mpretty-cyro commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

What this does

Why an anchor rather than the whole order

Why ids rather than indices

from is what the subscriber was told, not what the database says

List replacements, while they are here

What this asks of a subscriber, and a question for review

Testing

Uh oh!

jagerman commented Sep 9, 2026

Uh oh!

jagerman Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

jagerman Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mpretty-cyro commented Sep 8, 2026 •

edited

Loading

`from` is what the subscriber was told, not what the database says