feat: produce message acked events by kaichaosun · Pull Request #204 · logos-messaging/libchat

kaichaosun · 2026-08-11T03:39:48Z

GroupV2 conversations now carry the same causal-history envelope GroupV1 has had
since #105, and both group kinds report which peers hold a message we sent.

How

A peer can only name our message in its causal history if it held that message
when it sent, so the reference is the receipt. on_receive compares each
referenced id against the ids we authored and reports one ack per peer per
message. Nothing is sent back on purpose — that keeps this a delivery signal
rather than the read receipt rejected earlier on the issue.

API

Event::MessageAcked { convo_id, message_id, acker } — a peer holds a message
we sent.
Event::MessageMissing { convo_id, message_id, sender_hint } — a message we
never received, revealed by the causal history of one that did arrive.
send_message / send_content now return the MessageId instead of (), so
an application can match later acks to what it sent.

chat-cli renders both: "delivered to …" under its own messages, and a status
line for a gap / missing message.

Acceptance criteria

Add and handle a ReliablePayload frame for group chats
Emit an event when a message we sent was seen by at least one other client
Emit an event when a message was seen by all members — see below
Wire delivery acknowledgement through clients

Limits

Acknowledgement is passive: it rides on whatever a peer sends next. A peer that
never sends never acknowledges, and neither does one that speaks after our
message has dropped out of its 10-entry frontier. Absence of an ack means "not
confirmed", never "not delivered."

That is also why "seen by all members" is not included — it cannot be answered
honestly with this mechanism alone, since a member who reads without ever
replying leaves it permanently unsatisfied. It needs content-free sync messages
or the bloom filter.

acker is the payload's self-asserted sender_id; binding it to the
MLS-authenticated sender is deliberately out of scope.

Notes

The event stream now interleaves acks with messages, so a consumer that assumed
one event per message sees an extra one. That is why saro_and_raya.rs gained
an ack-skipping helper for its back-and-forth assertions.

jazzz · 2026-08-12T20:41:26Z

        cx: &mut ServiceContext<S>,
        content: &[u8],
-    ) -> Result<(), super::ChatError> {
+    ) -> Result<super::MessageId, super::ChatError> {


[Dust] Add use statement to simplify types without conflicts

jazzz · 2026-08-12T20:51:16Z

+        let content = match message {
+            Some(cm) => {
+                let reliable =
+                    ReliablePayload::decode(cm.message.as_slice()).map_err(ChatError::generic)?;
+                service_ctx.causal.on_receive(&self.convo_id, &reliable);
+                Some(Content {
+                    bytes: reliable.content.to_vec(),
+                    encoded_credential: cm.sender.clone(),
+                })
+            }
+            None => None,
+        };


The previous match statement is returning a Option, so that this match statement can process it.

[Dust] Consider separating this code into smaller functions, or sequential processing steps so its easier to read.

jazzz · 2026-08-12T20:52:43Z

+/// Evidence of *delivery to a peer's client*, not of a human reading it.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct MessageAck {


[Sand] Disambiguate by renaming to DeliveryAck. MessageAck could be anything.

jazzz · 2026-08-12T20:55:16Z

+    /// IDs of messages we authored: a reference to one is an acknowledgement.
+    own: HashSet<String>,
+    /// Which peers have acknowledged each of our messages, so each is
+    /// surfaced exactly once.
+    acked_by: HashMap<String, HashSet<String>>,


[Sand] This data will need to be culled at somepoint, or the memory footprint will grow with the number of messages sent.

Good point, better to clean the state after persistence added.

jazzz · 2026-08-12T20:56:49Z

+            if state.own.contains(&entry.message_id)
+                && payload.sender_id != entry.sender_id
+                && state
+                    .acked_by
+                    .entry(entry.message_id.clone())
+                    .or_default()
+                    .insert(payload.sender_id.clone())
+            {
+                acked.push(MessageAck {
+                    conversation_id: conversation_id.to_owned(),
+                    message_id: entry.message_id.clone(),
+                    acker_id: payload.sender_id.clone(),
+                });
+            }


[!] This is clean, easy to read and thoughful about the duplicate messages. Nice work.

jazzz · 2026-08-12T21:05:22Z

+    let missing: Vec<MissingMessage> = harness.raya().take_missing_messages();
+    assert_eq!(missing.len(), 1, "exactly one message should be missing");


[?] Out of curiosity; What is the motivation behind the "Store and Forward" architecture decision?

It seems like this could be prone to state errors, compared to extending ConvoOutcome, and providing lookup functions for historical state?

The "store and forward" architecture is set in the early iteration within groupv1 when the events system is not ready.
Re-architecture adds a lot of changes that makes this PR bloat, will be implemented later.

jazzz · 2026-08-12T21:30:50Z

+    /// Drain all acknowledgements of our own messages detected so far.
+    pub fn take_acks(&self) -> Vec<MessageAck> {
+        std::mem::take(&mut self.inner.borrow_mut().acked)
+    }


[Dust] Adding functions for each memory state could get cumbersome. This seems to mimics the Event data. Perhaps just use the existing event system?

Will be fixed later with replaced causal state design.

jazzz · 2026-08-12T21:38:24Z

+/// Map the acknowledgements the core observed while processing one payload onto
+/// [`Event::MessageAcked`], one per peer per message.
+///
+/// Drained from the same place as [`missing_events`]: the causal history of the
+/// message just processed is what carried the acknowledgement.
+fn ack_events(acks: Vec<MessageAck>, directory: &impl AccountDirectory) -> Vec<Event> {


[Dust] Ack is used as both a noun and a verb which makes this more difficult to understand than it needs to be.

jazzz · 2026-08-12T21:42:33Z

+fn missing_events(missing: Vec<MissingMessage>, directory: &impl AccountDirectory) -> Vec<Event> {
+    missing
+        .into_iter()
+        .map(|m| Event::MessageMissing {
+            convo_id: Arc::from(m.conversation_id),
+            message_id: m.frontier.message_id().to_owned(),
+            sender_hint: sender_hint(directory, m.frontier.sender_id()),
+        })
+        .collect()
+}


[Dust] Consider using From traits to handle conversions rather than using these custom conversion functions. These conversions are canonical - defining it that way makes the code easier to follow

These aren't T -> Event, they're (T, &Directory) -> Event: sender_hint resolves the self-asserted sender_id against the account directory, which isn't in DeliveryAck/MissingMessage. From takes one argument, so the impl can't be written.

jazzz · 2026-08-12T21:44:43Z

+    MessageAcked {
+        convo_id: Arc<str>,
+        message_id: String,
+        acker: Option<MessageSender>,


[Dust] In all other cases this field is called "Sender". For symmetry I'd consider using it here too.

message_id here is our message, so its sender is this client — sender would name the wrong participant. The field is the peer that acknowledged it, which is why it's acked_by (renamed from acker per your other note).

kaichaosun force-pushed the groupv2-delivery-notify branch 2 times, most recently from 87a87f3 to 4388ede Compare August 11, 2026 12:38

kaichaosun requested review from jazzz and osmaczko August 11, 2026 13:15

kaichaosun force-pushed the groupv2-delivery-notify branch from 4388ede to 52b3939 Compare August 11, 2026 13:24

jazzz reviewed Aug 12, 2026

View reviewed changes

kaichaosun added 8 commits August 13, 2026 10:39

feat: adopt causal history for group v2

40ecf2c

feat: message seen

82c2659

chore: clear duplicate tests

b934af6

chore: refactor the event convertion

3cbc36a

chore: refactor on imports, functional sequential processing etc

931e51c

chore: renaming to DeliveryAck

a3711b1

chore: merge tests into group v2 integration tests

8dc1c9b

chore: fix ack related namings

cdbdb9d

kaichaosun force-pushed the groupv2-delivery-notify branch from 52b3939 to cdbdb9d Compare August 13, 2026 05:32

		let missing: Vec<MissingMessage> = harness.raya().take_missing_messages();
		assert_eq!(missing.len(), 1, "exactly one message should be missing");

Uh oh!

Conversation

kaichaosun commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How

API

Acceptance criteria

Limits

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaichaosun Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kaichaosun commented Aug 11, 2026 •

edited

Loading

kaichaosun Aug 13, 2026 •

edited

Loading