From 12ac3550fe4af8d6713f647304177a4ba4a72386 Mon Sep 17 00:00:00 2001 From: spaced4ndy <8711996+spaced4ndy@users.noreply.github.com> Date: Fri, 8 May 2026 19:14:38 +0400 Subject: [PATCH] update --- plans/2026-04-06-channel-comments-v2.md | 142 ++++++++++++++++++------ 1 file changed, 105 insertions(+), 37 deletions(-) diff --git a/plans/2026-04-06-channel-comments-v2.md b/plans/2026-04-06-channel-comments-v2.md index bf1f4d6d14..2816a88999 100644 --- a/plans/2026-04-06-channel-comments-v2.md +++ b/plans/2026-04-06-channel-comments-v2.md @@ -571,25 +571,77 @@ getGroupMembersByCursor -> IO [GroupMember] ``` -The base query at line 363-371 acquires an additional clause when the new -parameter is `Just v`: +**The filter MUST run in SQL, not post-fetch in Haskell.** Post-fetch +filtering would be a correctness bug, not just an efficiency issue: the +caller's existing termination check at Subscriber.hs:3587 -```sql - AND ( - -- prefer peer_chat_v_max from active connection if present, else - -- fall back to memberChatVRange's max; both are stored at row read - -- time so the predicate is expressed in Haskell-side filtering. - ) +```haskell +unless (length mems < bucketSize) $ sendLoop bucketSize (Just cursorGMId') ``` -The simplest implementation: keep the SQL identical, fetch candidate -member rows, and filter post-fetch in Haskell using the same expression -as `Internal.hs:1627`'s `compatible` predicate: -`maxVersion (maybe memberChatVRange peerChatVRange activeConn) >= v`. -The `count` parameter still bounds the result. (Adding the predicate to -SQL would require a join through `connections` for `peer_chat_v_max`, -which complicates the cursor pagination; Haskell-side filtering keeps -the query shape unchanged.) +assumes "fewer than bucketSize returned ⇒ SQL ran out". With post-fetch +filtering, a SQL bucket of 100 rows that gets filtered down to 10 by +Haskell would fire `10 < 100`, the loop would terminate, and any members +past that SQL bucket would never receive the message. The cursor lives +inside SQL; the filter must too. + +Extend the existing query at Store/Delivery.hs:363-371 with a JOIN +through `connections` for the active connection's `peer_chat_max_version` +(or fall back to the member row's `peer_chat_max_version` when no +active connection is present). The cursor still advances on +`group_member_id`; the join just narrows the result set per row. + +Concrete SQL shape (verify exact column names against the as-built tree +at implementation time — Store/Shared.hs:282 and 729-736 show +`peer_chat_max_version` is the column name on both `connections` and +`group_members`): + +```sql +SELECT m.group_member_id +FROM group_members m +LEFT JOIN connections c + ON c.group_member_id = m.group_member_id + -- pick the one active connection if any; the existing helpers + -- already encapsulate this selection — reuse whichever pattern + -- is in use elsewhere in Store/Delivery.hs (e.g. the connection- + -- selection subquery used by getGroupMemberById's loader). +WHERE m.group_id = ? + AND m.contact_id IS DISTINCT FROM ? + AND m.group_member_id IS DISTINCT FROM ? + AND m.member_status IN (?,?,?,?,?,?) + AND ( + CAST(? AS INTEGER) IS NULL + OR COALESCE(c.peer_chat_max_version, m.peer_chat_max_version) >= ? + ) +ORDER BY m.group_member_id ASC +LIMIT ? +``` + +The `min_recipient_version` parameter is bound twice: once for the NULL +guard, once for the comparison. (Postgres / SQLite both accept this +shape; the `CAST(? AS INTEGER)` is a defensive no-op that prevents +parameter type ambiguity in the NULL check.) When the parameter is +`Nothing`, the predicate degenerates to `TRUE` and the query behaves +identically to today. + +If the existing helpers in Store/Delivery.hs do not already select an +active connection per member, the simplest correct shape is to join +`connections` and let SQL choose the maximum +`peer_chat_max_version` per member-row group: + +```sql +LEFT JOIN connections c ON c.group_member_id = m.group_member_id +... +GROUP BY m.group_member_id +HAVING (CAST(? AS INTEGER) IS NULL OR + COALESCE(MAX(c.peer_chat_max_version), m.peer_chat_max_version) >= ?) +``` + +Pick whichever shape matches the existing connection-selection +convention used by `getGroupMemberById` and similar loaders in +`Store/Shared.hs` (the `c.conn_chat_version, c.peer_chat_min_version, +c.peer_chat_max_version` join at Store/Shared.hs:736 is the existing +template). Pass the job's `min_recipient_version` from `runDeliveryJobOperation` (Subscriber.hs:3582) into `getGroupMembersByCursor`. The same parameter @@ -599,6 +651,12 @@ paths because comment-bearing events do not appear on the member-support path today, but threading the parameter both ways is defensive and trivially correct. +Why this matters: the version constraint lives in the same place the +cursor lives, the cursor naturally advances past rejected members because +they're never returned to the application, and the existing termination +check at Subscriber.hs:3587 remains correct. Off-by-one in a Haskell +post-filter loop is impossible because there is no post-filter loop. + **Why this pattern is new.** The body is opaque post-encoding, so the version constraint must be tagged at the boundary where the inner event is still inspectable (task creation). The existing version-gate idioms @@ -967,7 +1025,15 @@ the existing `describe "channel comments"` block at content edit does NOT silently reset `commentsDisabled` (Subscriber.hs:2126 returning False on Nothing prefs is the invariant under test). 9. `testChannelCommentMemberCanCommentReceiveGuard` — defense-in-depth - from §3.1. Concrete sequencing: + from §3.1. The mechanism is the join handshake, not history-replay + ordering: `XGrpMemRole` is persisted as a `CIRcvGroupEvent` whose + `ciMsgContent` is `Nothing`, so `mcTag_` is `Nothing` and + `includeInHistory` (Store/Messages.hs:632-635) evaluates to `False`. + Role-change events are NOT replayed by `getGroupHistoryItems`. What + IS replayed is bob's old comment (which has a real `MsgContent`). + Frank learns bob's current role at handshake time via `XGrpMemNew`. + + Concrete sequencing: 1. Create channel `team`; bob is `GRCommenter` (default subscriber role). 2. Bob comments under alice's post; cath, dan, eve all receive the @@ -976,29 +1042,31 @@ the existing `describe "channel comments"` block at 3. Alice demotes bob to `GRObserver` via `/mr team bob observer`; cath, dan, eve receive the `XGrpMemRole` event and update their local copy of bob's role. - 4. New subscriber frank joins. The relay replays history via - `getGroupHistoryItems`, which returns rows in ASC `item_ts` order - (per H2 above): alice's earlier `XGrpMemRole` event for bob - precedes bob's earlier comment because the role-change came - after the comment, so by the time frank's `newGroupContentMessage` - processes bob's comment, frank's local copy of bob's role is - `GRObserver`. (If the relay sends events in mixed order on - different SMP queues such that the comment arrives before the - role-change, this test fails for an orthogonal reason — the - ordering invariant — and the deferred history-replay sort is - the proper fix.) - 5. Assert: frank's `memberCanComment` rejects the comment with + 4. New subscriber frank joins. During the join handshake, alice (or + the relay on alice's behalf) introduces bob to frank via + `XGrpMemNew` carrying bob's CURRENT role at the time of the + introduction — `GRObserver`, because frank joined after the + demotion at step 3. Frank's local member record for bob is + therefore created with `memberRole = GRObserver`. + 5. `getGroupHistoryItems` replays bob's earlier comment (made when + bob was `GRCommenter`) to frank. The role-change event from + step 3 is NOT in the replay stream because role-change items + have `includeInHistory = False`. By the time frank's + `newGroupContentMessage` processes the replayed comment, + frank's local copy of bob's role is `GRObserver` (set in + step 4). + 6. Assert: frank's `memberCanComment` rejects the comment with `messageError "member is not allowed to comment"`; no chat item is created in frank's chat under alice's post. - **Fallback.** If the test harness cannot reliably set up frank's - local member-role state before the comment arrives (e.g. the - role-change replay does not fire ahead of the comment in the test - broker), substitute a unit-level test that constructs a - `GroupMember` at `GRObserver` and calls `memberCanComment` directly, - asserting it returns `Nothing` and emits the expected - `messageError`. The defense-in-depth invariant is on the helper, not - on the harness behavior. + **Fallback.** If the test harness cannot reliably orchestrate the + handshake-then-replay sequencing (e.g. the join handshake does not + complete before history items are forwarded in the test broker), + substitute a unit-level test that constructs a `GroupMember` at + `GRObserver` and calls `memberCanComment` directly, asserting it + returns `Nothing` and emits the expected `messageError`. The + defense-in-depth invariant is on the helper, not on the harness + behavior. 10. `testChannelCommentVersionGated` — from §3.3 / §6.3: 1. Spawn alice (owner), bob (relay), dan (subscriber at