fix(matrix): advance the transaction ID between events within a notification

Both advance sites gated on transactionIDString == "", so the token path
never advanced its id: every event in a notification reused the one
generated per sendServer() call. A homeserver recognizes the repeat as a
retransmission, so everything after the first event in a room is dropped
silently, with a 200 each time. With attachments that costs the message
body itself, since the text is sent last.

Advance the string path by generating the next uuid, and route
sendMessage() through advanceMessageTransaction() rather than repeating
the guard inline.

Fixes #88

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: MaxMFC

internal/notify/matrix.go

@@ -409,9 +409,25 @@ func (m *MatrixTarget) uploadFetch(attachment Attachment, params url.Values) (bo
 
 // advanceMessageTransaction moves to the next transaction id, which upstream
 // does after every event so a retry is not mistaken for a retransmission.
+//
+// A notification can produce several events in one room -- a file per
+// attachment, then the text -- and an id that does not move between them is
+// what the homeserver uses to recognize a retransmission, so everything after
+// the first event is dropped. The spec asks for an id unique across requests
+// sharing an access token, so this advances per event rather than per room.
 func (m *MatrixTarget) advanceMessageTransaction() {
-	if m.version == matrixVersionV3 && m.accessToken != "" &&
-		m.accessToken != m.password && m.transactionIDString == "" {
+	if m.version != matrixVersionV3 || m.accessToken == "" {
+		return
+	}
+
+	// The token path carries its id as a uuid string rather than a counter,
+	// so it advances by generating the next one.
+	if m.transactionIDString != "" {
+		m.transactionIDString = newUUIDv4()
+		return
+	}
+
+	if m.accessToken != m.password {
 		m.transactionID++
 	}
 }
@@ -727,8 +743,8 @@ func (m *MatrixTarget) sendMessage(roomID, body, title string, notifyType Notify
 
 	path := m.messagePath(roomID)
 	ok, _, _ := m.fetch(path, payload, nil, m.messageMethod(), "")
-	if ok && m.version == matrixVersionV3 && m.accessToken != "" && m.accessToken != m.password && m.transactionIDString == "" {
-		m.transactionID++
+	if ok {
+		m.advanceMessageTransaction()
 	}
 	_ = notifyType
 	return ok

internal/notify/matrix_e2ee.go

@@ -641,7 +641,10 @@ func (m *MatrixTarget) transactionValue() string {
 // advanceTransaction moves the counter on, keeping it in storage so a later
 // process does not reuse an id the server has already seen.
 func (m *MatrixTarget) advanceTransaction() {
+	// The token path carries its id as a uuid string; there is no counter to
+	// persist, so it advances by generating the next one.
 	if m.transactionIDString != "" {
+		m.transactionIDString = newUUIDv4()
 		return
 	}
 

internal/notify/matrix_transaction_test.go

@@ -50,3 +50,91 @@ func TestMatrixAccessTokenTransactionIDIsUnique(t *testing.T) {
 		t.Fatalf("transaction id is the deterministic placeholder %q rather than a generated value", first)
 	}
 }
+
+// sendTransactionIDs returns the transaction id of every m.room.message send
+// issued by one notification, in order.
+func sendTransactionIDs(t *testing.T, specs []notify.RequestSpec) []string {
+	t.Helper()
+
+	const marker = "/send/m.room.message/"
+	ids := []string{}
+	for _, spec := range specs {
+		if idx := strings.Index(spec.URL, marker); idx >= 0 {
+			ids = append(ids, spec.URL[idx+len(marker):])
+		}
+	}
+	return ids
+}
+
+// assertUniqueTransactionIDs fails when a notification reuses an id, naming
+// the collision rather than only reporting that one happened.
+func assertUniqueTransactionIDs(t *testing.T, ids []string, want int) {
+	t.Helper()
+
+	if len(ids) != want {
+		t.Fatalf("expected %d m.room.message sends, got %d", want, len(ids))
+	}
+
+	seen := map[string]int{}
+	for i, id := range ids {
+		if first, ok := seen[id]; ok {
+			t.Fatalf("sends %d and %d share transaction id %q; a homeserver is "+
+				"entitled to treat the later one as a retransmission and drop it",
+				first, i, id)
+		}
+		seen[id] = i
+	}
+}
+
+// TestMatrixMultiRoomTransactionIDsAreUnique covers a notification addressed
+// to more than one room.
+//
+// Synapse tolerates a repeat here, because it keys idempotency on the request
+// path and that carries the room id. The spec is the stricter of the two --
+// it asks for an id unique across requests sharing an access token -- so this
+// pins the behavior a homeserver keying on the token alone would need.
+func TestMatrixMultiRoomTransactionIDsAreUnique(t *testing.T) {
+	t.Setenv("APPRISE_FIXED_TIME", "")
+	notify.ConfigureStorage("", 8, nil)
+	t.Cleanup(func() { notify.ConfigureStorage("", 8, nil) })
+
+	specs := testutil.CaptureGoRequests(t, func() error {
+		return notify.SendTargetURL(
+			"matrixs://tokenabc123@matrix.example.com/%23room1:example.com/%23room2:example.com?e2ee=no",
+			"body", "title", "", notify.NotifyInfo)
+	})
+
+	assertUniqueTransactionIDs(t, sendTransactionIDs(t, specs), 2)
+}
+
+// TestMatrixAttachmentTransactionIDsAreUnique covers the several events a
+// single room receives when files are attached: one per file, then the text.
+//
+// This is the case that fails against a real homeserver: same room, so same
+// request path, so a repeated id is a retransmission. It costs the message
+// body itself, since the text is sent last and is what gets discarded.
+func TestMatrixAttachmentTransactionIDsAreUnique(t *testing.T) {
+	t.Setenv("APPRISE_FIXED_TIME", "")
+	notify.ConfigureStorage("", 8, nil)
+	t.Cleanup(func() { notify.ConfigureStorage("", 8, nil) })
+
+	specs := testutil.CaptureGoRequests(t, func() error {
+		target, err := notify.ParseURL("matrixs://tokenabc123@matrix.example.com/%23room1:example.com?e2ee=no")
+		if err != nil {
+			return err
+		}
+		sender, err := notify.NewTarget(target)
+		if err != nil {
+			return err
+		}
+
+		return notify.DispatchSend(sender, "body", "title", notify.NotifyInfo,
+			[]notify.Attachment{
+				{Name: "one.txt", MIMEType: "text/plain", Data: []byte("first")},
+				{Name: "two.txt", MIMEType: "text/plain", Data: []byte("second")},
+			})
+	})
+
+	// Two attachment events plus the text message.
+	assertUniqueTransactionIDs(t, sendTransactionIDs(t, specs), 3)
+}