From 52109e2f55d978c01e86383e58481182d9bac40a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Apr 2026 09:11:29 +0000 Subject: [PATCH 01/32] docs: spec for message-worker thread subscription outbox events Replicate ThreadSubscription writes (parent author, replier, mentionees) to each affected user's home site via OUTBOX/INBOX, mirroring room-worker's pattern. Single new outbox type (thread_subscription_upserted), payload is the existing ThreadSubscription, inbox-worker upserts with monotonic hasMention merge. --- ...orker-thread-subscription-outbox-design.md | 355 ++++++++++++++++++ 1 file changed, 355 insertions(+) create mode 100644 docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md diff --git a/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md b/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md new file mode 100644 index 000000000..974d0eb5d --- /dev/null +++ b/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md @@ -0,0 +1,355 @@ +# Message-worker thread subscription outbox events + +## Problem + +When a user posts a thread reply, `message-worker` writes a `ThreadSubscription` +to MongoDB for the **parent author**, the **replier**, and any **`@account` +mentionee** in the reply. Today those writes happen only on the room's home +site. If any of those users' home sites differ from the room's home site, their +`subscriptions` list at home shows nothing — they have no record that they're +participating in the thread, can't see it in their UI, and won't receive a +mention indicator. + +`room-worker` already solves the analogous problem for room subscriptions via +the OUTBOX/INBOX pattern. `message-worker` has no equivalent path: it doesn't +publish to JetStream at all today. + +## Goal + +Replicate `ThreadSubscription` upserts to each affected user's home site via the +existing OUTBOX/INBOX federation. After this change, when bob@site-b replies in +a thread on a room hosted at site-a, both bob's home site (site-b) and the +parent author's home site (site-c if applicable) end up with a +`ThreadSubscription` document in their local MongoDB. Mentionees get the same +treatment, including the `hasMention=true` flag. + +## Non-goals + +- **`ThreadRoom` replication.** No remote service queries `thread_rooms` today + (verified: only message-worker reads from it). Replicating the lastMsgAt / + lastMsgID pointers per reply would double cross-site traffic for no consumer. + If a future consumer needs `ThreadRoom` at remote sites, layer it on then. +- **Client-facing UI events.** Nothing consumes a thread-subscription update + subject today, so we don't introduce one. Adding it is a separate feature. +- **Delete / cleanup events.** Thread subscriptions are never deleted by the + current code; that path is out of scope. +- **`@all` propagation.** `@all` is already filtered out at the thread level + (see `markThreadMentions`), so it never produces an outbox event. + +## Design + +### Event shape + +A single new outbox event type: + +``` +const OutboxThreadSubscriptionUpserted OutboxEventType = "thread_subscription_upserted" +``` + +`OutboxEvent.Payload` is a JSON-encoded `model.ThreadSubscription`. No new +payload struct — the existing `ThreadSubscription` already carries everything +the destination site needs: + +```go +type ThreadSubscription struct { + ID string // home-site-generated UUID; same value lands at remote site + ParentMessageID string + RoomID string + ThreadRoomID string + UserID string + UserAccount string + SiteID string // owner's home site (see Semantic change below) + LastSeenAt *time.Time // always nil from message-worker + HasMention bool // true only on mention-marked events + CreatedAt time.Time + UpdatedAt time.Time +} +``` + +The destination site of the outbox is the user's home site. One outbox event +per (reply, affected user) tuple. + +### Semantic change to `ThreadSubscription.SiteID` + +Today `buildThreadSubscription` sets `SiteID = ` — i.e. +the room's home site. For subscriptions to round-trip across sites cleanly, the +field must reflect the **owner's** home site. The room context is already +preserved by `RoomID`. This brings `ThreadSubscription.SiteID` in line with +`Subscription.SiteID` and matches what inbox-worker stores when it consumes the +event. + +Callers that don't currently have the owner's siteID in scope (the `markThreadMentions` +loop, parent author lookup) gain a small lookup or piggyback on existing `User` +data — see "Implementation" below. + +### Subject + dedup + +- Outbox subject: `subject.Outbox(homeSiteID, destSiteID, "thread_subscription_upserted")` + → `outbox.{homeSite}.to.{destSite}.thread_subscription_upserted` +- `Nats-Msg-Id` seed: `thread-sub-outbox:{threadRoomID}:{userID}:{msg.ID}`. + `msg.ID` is unique per reply; (msg.ID, userID) is unique within a reply. Stable + across MESSAGES_CANONICAL redeliveries → JetStream stream-level dedup absorbs + duplicates within the dedup window. + +### Failure semantics + +If any outbox publish fails, `processMessage` returns the error → `HandleJetStreamMsg` +NAKs → JetStream redelivers from MESSAGES_CANONICAL. On redelivery: + +- `CreateThreadRoom` returns `errThreadRoomExists` → goes to subsequent-reply path. +- `UpsertThreadSubscription` and `MarkThreadSubscriptionMention` are idempotent. +- Outbox publishes carry stable dedup IDs → JetStream filters duplicates. +- Net effect: at-least-once delivery on the wire, exactly-once observable state + on the destination after dedup. + +### Inbox-worker dispatch + +`inbox-worker/handler.go` adds a case: + +```go +case "thread_subscription_upserted": + return h.handleThreadSubscriptionUpserted(ctx, &evt) +``` + +The handler unmarshals `evt.Payload` into a `ThreadSubscription` and calls +`store.UpsertThreadSubscription(ctx, sub)`. + +The Mongo implementation: + +``` +filter: { threadRoomId: sub.ThreadRoomID, userId: sub.UserID } +update: + $setOnInsert: { _id, parentMessageId, roomId, threadRoomId, userId, + userAccount, siteId, createdAt, lastSeenAt: null } + $set: { updatedAt } + $bit: { hasMention: { or: <1 if sub.HasMention else 0> } } +opts: upsert: true +``` + +The `$bit:or` (or equivalent `$max` on a 0/1 int) makes the merge monotonic: +once `hasMention` flips true at the destination, no later non-mention event can +clear it. `_id` and `createdAt` come from `$setOnInsert` so the first event to +land defines them; later events update only `updatedAt` (and possibly the +mention bit). + +`LastSeenAt` is never written by inbox-worker (it's a per-user-action field +owned by whatever path lets a user mark a thread as seen — out of scope here). + +### Stream ownership + +- **OUTBOX_{siteID}** is owned by ops/IaC entirely. Verified: `room-worker` + publishes to `outbox.{siteID}.>` today but does **not** bootstrap the stream + in its `bootstrap.go`; only `MESSAGES_CANONICAL` / `ROOMS` / `INBOX` are + bootstrapped by their respective owning services. `message-worker` follows + the same convention — it publishes to OUTBOX subjects but its `bootstrap.go` + is unchanged with respect to OUTBOX. (If single-site dev needs an OUTBOX + stream to absorb publishes, that's a pre-existing gap that affects + room-worker equally and is out of scope for this spec.) +- **MESSAGES_CANONICAL_{siteID}** continues to be bootstrapped by + `message-worker/bootstrap.go` exactly as today. +- **INBOX_{siteID}** is owned by `inbox-worker` (per CLAUDE.md "Stream + bootstrap ownership" — INBOX has a single owning service). No change there. + +### Implementation outline + +#### `pkg/model/event.go` + +Add the new constant alongside existing ones: + +```go +const ( + OutboxMemberAdded OutboxEventType = "member_added" + OutboxMemberRemoved OutboxEventType = "member_removed" + OutboxThreadSubscriptionUpserted OutboxEventType = "thread_subscription_upserted" +) +``` + +Add a model-test case for the new constant in `pkg/model/model_test.go` (round-trip +existing `OutboxEvent` with the new type tag — no new struct to test). + +#### `message-worker/handler.go` + +`Handler` gains two new fields: + +```go +type Handler struct { + store Store + userStore userstore.UserStore + threadStore ThreadStore + siteID string // h.siteID — same role as in room-worker + publish PublishFunc // same signature as room-worker +} + +type PublishFunc func(ctx context.Context, subj string, data []byte, msgID string) error +``` + +`NewHandler` updates accordingly. `main.go` wires `cfg.SiteID` and a closure +that calls `js.Publish(ctx, subj, data, jetstream.WithMsgID(msgID))` exactly as +room-worker does. + +`buildThreadSubscription` keeps its parameter list but the `siteID` argument +now means "**owner's** site", not the room's. The three callers update: + +1. **handleFirstThreadReply** — replier: pass the replier's `User.SiteID` (we + already have `User` from `processMessage`'s `userStore.FindUserByID`). Parent: + look up `userStore.FindUserByID(parentSender.ID)` to get parent's siteID. +2. **handleSubsequentThreadReply** — same as above. +3. **markThreadMentions** — mentionees: `mention.Resolve` returns + `[]Participant`, which today doesn't carry `SiteID`. Verified: + `mention.Resolve` already calls `LookupFunc` (= `userStore.FindUsersByAccounts`), + and that store query already projects `siteId`. So the `User` slice inside + `Resolve` already has the data — we just don't propagate it onto + `Participant`. + + Add a `SiteID string \`json:"siteId,omitempty" bson:"siteId,omitempty"\`` + field to `model.Participant` and populate it in `mention.Resolve` from + `users[i].SiteID`. The new field is `omitempty` and additive — existing + consumers that JSON-encode `Participant` (e.g. on `Message.Mentions`) emit + the extra field but otherwise behave identically. Update + `pkg/model/model_test.go` to round-trip `SiteID` on `Participant`. + +After every `InsertThreadSubscription` / `UpsertThreadSubscription` / +`MarkThreadSubscriptionMention` succeeds, call: + +```go +h.publishThreadSubOutboxIfRemote(ctx, sub, msg.ID) +``` + +The helper: + +```go +func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model.ThreadSubscription, msgID string) error { + if sub.SiteID == h.siteID { + return nil + } + payload, err := json.Marshal(sub) + if err != nil { + return fmt.Errorf("marshal thread subscription: %w", err) + } + outbox := model.OutboxEvent{ + Type: model.OutboxThreadSubscriptionUpserted, + SiteID: h.siteID, + DestSiteID: sub.SiteID, + Payload: payload, + Timestamp: time.Now().UTC().UnixMilli(), + } + data, err := json.Marshal(outbox) + if err != nil { + return fmt.Errorf("marshal outbox event: %w", err) + } + dedupID := idgen.DeriveID(fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID)) + if err := h.publish(ctx, subject.Outbox(h.siteID, sub.SiteID, model.OutboxThreadSubscriptionUpserted), data, dedupID); err != nil { + return fmt.Errorf("publish thread subscription outbox to %s: %w", sub.SiteID, err) + } + return nil +} +``` + +Errors propagate to `processMessage`, which propagates to `HandleJetStreamMsg`, +which NAKs. + +#### `message-worker/main.go` + +- Inject `cfg.SiteID` and the JetStream publish closure into `NewHandler`. The + closure is a copy of room-worker's: when `msgID == ""` use core + `nc.Publish` (none of message-worker's publishes use that branch yet), and + otherwise use `js.Publish(ctx, subj, data, jetstream.WithMsgID(msgID))`. +- `bootstrap.go` is **unchanged** with respect to OUTBOX (per "Stream + ownership" — OUTBOX is owned by ops/IaC). + +#### `inbox-worker/handler.go` + +- Add `UpsertThreadSubscription(ctx context.Context, sub *model.ThreadSubscription) error` + to the `InboxStore` interface. +- Add a `case "thread_subscription_upserted"` to `HandleEvent` calling + `handleThreadSubscriptionUpserted`. +- New `handleThreadSubscriptionUpserted(ctx, evt)` unmarshals payload into + `ThreadSubscription`, calls `store.UpsertThreadSubscription`. + +#### `inbox-worker/main.go` + +- Add `threadSubCol *mongo.Collection` to `mongoInboxStore`, initialized from + `db.Collection("threadSubscriptions")` (same collection name message-worker + uses). +- Implement `UpsertThreadSubscription` with the `$setOnInsert` + `$set` + `$bit:or` + shape described above. Filter is `{threadRoomId, userId}` to match the + message-worker's natural key. + +### Testing + +#### Unit tests + +`message-worker/handler_test.go`: + +- Extend each happy-path table case to assert `publish` was (or wasn't) called + with the right subject, payload, and dedup ID. Capture publishes via a + recorder closure injected in tests instead of mocking `nats.Conn`. +- New cases: + - First reply, replier remote, parent local → one outbox to replier's site. + - First reply, replier local, parent remote → one outbox to parent's site, + with parent siteID resolved via `FindUserByID`. + - First reply, both remote, different sites → two outboxes, one per site. + - First reply, both remote, same site → two outboxes to same site (separate + events, distinct dedup IDs). + - Subsequent reply variants (Upsert path, same matrix as above). + - Mention path: mentionee remote → one outbox with `HasMention=true`. + - Mention path: mentionee local → no outbox. + - Outbox publish error → returned error from `processMessage`. + - Parent's `FindUserByID` returns `userstore.ErrUserNotFound` → log warn, + skip parent subscription + outbox, replier still processed (parallels the + `errMessageNotFound` branch in `handleFirstThreadReply`). + - Parent's `FindUserByID` returns a non-NotFound error (DB unreachable etc.) + → returned error from `processMessage` → NAK. + +`inbox-worker/handler_test.go`: + +- Dispatch case for `thread_subscription_upserted` calls `UpsertThreadSubscription` + with the unmarshalled payload. +- Unmarshal error propagated. +- `UpsertThreadSubscription` error propagated. + +`inbox-worker/integration_test.go`: + +- Two cases: + - Insert path: empty collection → upsert lands a complete document. + - Update path with prior `hasMention=true` → second event with + `hasMention=false` does **not** clear the flag. + +#### Coverage targets + +Per CLAUDE.md, ≥80% with 90%+ on handler logic. The added paths are tightly +scoped — branch coverage for the local-vs-remote split, the three publish call +sites, and the `hasMention` OR-merge. + +### Migration / rollout + +- The new outbox type is additive; old inbox-worker deployments will hit the + `default: slog.Warn("unknown event type, skipping")` branch and ack the + message. To avoid silently dropping events during a rolling upgrade, deploy + inbox-worker first, then message-worker. (The OUTBOX stream's MaxAge + determines how long unhandled events persist; if it's bounded short, the + ordering matters less, but the deploy order is still the safe default.) +- No data migration: existing `thread_subscriptions` rows are unaffected. + +### Risks + +- **`SiteID` on `model.Participant` is a wire-format addition.** It's + `omitempty` and additive — existing JSON consumers decode unchanged, and the + one path that reads it (this spec's `markThreadMentions`) always sees fresh + data because `processMessage` re-runs `mention.Resolve` on every delivery, + re-populating `Mentions` from the live userstore. Old persisted `Mentions` + arrays in Cassandra are never re-emitted as outbox events. +- **Defensive guard against empty `SiteID`.** The publish helper must skip + + log warn if `sub.SiteID == ""`. This prevents an upstream bug (e.g., a future + caller forgetting to set the field) from emitting an outbox to a `dest=""` + subject. The empty case is otherwise unreachable in the paths added here. +- **Parent user lookup may fail.** `GetMessageSender` already handles + `errMessageNotFound` for the parent **message**. Adding + `userStore.FindUserByID(parentSender.ID)` introduces a new "parent user + not found" branch — treat it the same as the parent-message-not-found case: + log warn and skip the parent subscription (and its outbox) but still process + the replier. This avoids hard-failing a thread reply when the parent author's + user record has been removed. +- **Cross-site siteID coverage in tests.** The `User` test fixtures in + `handler_test.go` all set `SiteID = "site-a"`. Add fixtures with differing + `SiteID` values to exercise both branches of the local-vs-remote check. From d0decfdf1166dda33fc689aeac3a67a6d9933b94 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 05:37:08 +0000 Subject: [PATCH 02/32] =?UTF-8?q?docs:=20implementation=20plan=20chunk=201?= =?UTF-8?q?=20(tasks=201-3=20=E2=80=94=20model+mention=20scaffolding)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add OutboxThreadSubscriptionUpserted constant, SiteID on Participant propagated from mention.Resolve, and siteID + PublishFunc fields on message-worker Handler. --- ...ssage-worker-thread-subscription-outbox.md | 362 ++++++++++++++++++ 1 file changed, 362 insertions(+) create mode 100644 docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md new file mode 100644 index 000000000..be95eb39d --- /dev/null +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -0,0 +1,362 @@ +# Message-worker thread subscription outbox events implementation plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replicate `ThreadSubscription` writes (parent author, replier, mentionees) from message-worker on the room's home site to each affected user's home site via the existing OUTBOX/INBOX federation. + +**Architecture:** Mirror room-worker's pattern. Message-worker gains a `publish PublishFunc` field; after every local Insert/Upsert/MarkThreadSubscriptionMention it emits a `thread_subscription_upserted` outbox event when the subscription owner's site differs from the room's site. Inbox-worker dispatches the new event type to a new `UpsertThreadSubscription` store method that uses MongoDB `$max` on `hasMention` for monotonic mention-flag merge. + +**Tech Stack:** Go 1.25, NATS JetStream (`github.com/nats-io/nats.go/jetstream`), MongoDB (`go.mongodb.org/mongo-driver/v2`), `github.com/hmchangw/chat/pkg/{model,subject,idgen,mention,userstore,stream}`, `go.uber.org/mock`, `stretchr/testify`, `testcontainers-go`. + +**Spec:** `docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md` + +--- + +## File map + +**Modify:** +- `pkg/model/event.go` — new `OutboxThreadSubscriptionUpserted` const; new `SiteID` field on `Participant` +- `pkg/model/model_test.go` — round-trip test for new event-type tag and `Participant.SiteID` +- `pkg/mention/mention.go` — propagate `SiteID` from looked-up `User` onto `Participant` +- `pkg/mention/mention_test.go` — extend `TestResolve` to assert `SiteID` +- `message-worker/handler.go` — `Handler` gains `siteID` + `publish` fields; new `publishThreadSubOutboxIfRemote` helper; owner-site semantic for `ThreadSubscription.SiteID`; parent-user lookup; publish wiring in three paths +- `message-worker/handler_test.go` — new test cases for cross-site publishes +- `message-worker/main.go` — wire `cfg.SiteID` and JetStream publish closure into `NewHandler` +- `inbox-worker/handler.go` — new dispatch case + handler for `thread_subscription_upserted`; extend `InboxStore` interface +- `inbox-worker/handler_test.go` — extend stub store with `threadSubscriptions`; new dispatch tests +- `inbox-worker/main.go` — `mongoInboxStore` gains `threadSubCol` + `UpsertThreadSubscription` method +- `inbox-worker/integration_test.go` — new integration tests for upsert insert path + monotonic `hasMention` merge + +**No new files.** All work fits in existing files. + +--- + +## Task 1: Add `OutboxThreadSubscriptionUpserted` constant + +**Files:** +- Modify: `pkg/model/event.go:79-84` +- Modify (test): `pkg/model/model_test.go` (append a new test) + +- [ ] **Step 1.1: Write the failing model test** + +Append to `pkg/model/model_test.go` (after the existing `TestOutboxEventJSON`): + +```go +func TestOutboxEventJSON_ThreadSubscriptionUpserted(t *testing.T) { + src := model.OutboxEvent{ + Type: model.OutboxThreadSubscriptionUpserted, + SiteID: "site-a", + DestSiteID: "site-b", + Payload: []byte(`{"id":"sub-1","threadRoomId":"tr-1"}`), + Timestamp: 1735689600000, + } + data, err := json.Marshal(&src) + require.NoError(t, err) + + var dst model.OutboxEvent + require.NoError(t, json.Unmarshal(data, &dst)) + if !reflect.DeepEqual(src, dst) { + t.Errorf("round-trip mismatch:\n got %+v\n want %+v", dst, src) + } + if dst.Type != "thread_subscription_upserted" { + t.Errorf("Type = %q, want thread_subscription_upserted", dst.Type) + } +} +``` + +- [ ] **Step 1.2: Run test to verify it fails** + +Run: `make test SERVICE=pkg/model 2>&1 | tail -30` + +Expected: compile error `undefined: model.OutboxThreadSubscriptionUpserted`. + +- [ ] **Step 1.3: Add the constant** + +In `pkg/model/event.go`, replace the existing const block (line ~81-84): + +```go +const ( + OutboxMemberAdded OutboxEventType = "member_added" + OutboxMemberRemoved OutboxEventType = "member_removed" + OutboxThreadSubscriptionUpserted OutboxEventType = "thread_subscription_upserted" +) +``` + +- [ ] **Step 1.4: Run test to verify it passes** + +Run: `make test SERVICE=pkg/model` + +Expected: all `pkg/model` tests pass including the new one. + +- [ ] **Step 1.5: Commit** + +```bash +git add pkg/model/event.go pkg/model/model_test.go +git commit -m "model: add OutboxThreadSubscriptionUpserted event type constant" +``` + +--- + +## Task 2: Add `SiteID` to `model.Participant` and propagate from `mention.Resolve` + +**Files:** +- Modify: `pkg/model/event.go` (the `Participant` struct, around line 105-110) +- Modify: `pkg/model/model_test.go` (extend `TestParticipantJSON`) +- Modify: `pkg/mention/mention.go:77-84` +- Modify: `pkg/mention/mention_test.go` (extend `TestResolve`) + +- [ ] **Step 2.1: Write the failing model test for `Participant.SiteID`** + +Replace the `TestParticipantJSON` function in `pkg/model/model_test.go` with the version below (it adds two new sub-tests; keep the existing two): + +```go +func TestParticipantJSON(t *testing.T) { + t.Run("with userID", func(t *testing.T) { + p := model.Participant{ + UserID: "u1", + Account: "alice", + ChineseName: "愛麗絲", + EngName: "Alice Wang", + } + roundTrip(t, &p, &model.Participant{}) + }) + + t.Run("without userID omitted", func(t *testing.T) { + p := model.Participant{ + Account: "bob", + ChineseName: "鮑勃", + EngName: "Bob Chen", + } + data, err := json.Marshal(p) + require.NoError(t, err) + + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + _, hasUserID := raw["userId"] + assert.False(t, hasUserID, "userId should be omitted when empty") + + var dst model.Participant + require.NoError(t, json.Unmarshal(data, &dst)) + assert.Equal(t, p, dst) + }) + + t.Run("with siteID round-trips", func(t *testing.T) { + p := model.Participant{ + UserID: "u1", + Account: "alice", + SiteID: "site-a", + ChineseName: "愛麗絲", + EngName: "Alice Wang", + } + roundTrip(t, &p, &model.Participant{}) + }) + + t.Run("siteID omitted when empty", func(t *testing.T) { + p := model.Participant{ + UserID: "u1", + Account: "alice", + EngName: "Alice Wang", + } + data, err := json.Marshal(p) + require.NoError(t, err) + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + _, hasSiteID := raw["siteId"] + assert.False(t, hasSiteID, "siteId should be omitted when empty") + }) +} +``` + +- [ ] **Step 2.2: Run test to verify it fails** + +Run: `make test SERVICE=pkg/model` + +Expected: compile error `unknown field SiteID in struct literal of type model.Participant`. + +- [ ] **Step 2.3: Add the `SiteID` field** + +In `pkg/model/event.go`, replace the `Participant` struct definition with: + +```go +// Participant represents a user with display name info for client rendering. +type Participant struct { + UserID string `json:"userId,omitempty" bson:"userId,omitempty"` + Account string `json:"account" bson:"account"` + SiteID string `json:"siteId,omitempty" bson:"siteId,omitempty"` + ChineseName string `json:"chineseName" bson:"chineseName"` + EngName string `json:"engName" bson:"engName"` +} +``` + +- [ ] **Step 2.4: Run model tests to verify they pass** + +Run: `make test SERVICE=pkg/model` + +Expected: all `pkg/model` tests pass. + +- [ ] **Step 2.5: Write the failing mention test** + +Replace the "single mention resolved" and "multiple mentions resolved" cases in `TestResolve` (`pkg/mention/mention_test.go`) — change the user fixtures and `wantParts` to include `SiteID`. Use this diff (replace the two named test cases inline): + +```go + bobUser := model.User{ID: "u-bob", Account: "bob", SiteID: "site-b", EngName: "Bob Chen", ChineseName: "鮑勃"} + aliceUser := model.User{ID: "u-alice", Account: "alice", SiteID: "site-a", EngName: "Alice Wang", ChineseName: "愛麗絲"} +``` + +(Update both `bobUser` and `aliceUser` declarations at the top of `TestResolve` — add `SiteID` field.) + +Then update `wantParts` for these cases: + +```go + { + name: "single mention resolved", + content: "hey @bob", + lookupUsers: []model.User{bobUser}, + wantAccounts: []string{"bob"}, + wantParts: []model.Participant{ + {UserID: "u-bob", Account: "bob", SiteID: "site-b", EngName: "Bob Chen", ChineseName: "鮑勃"}, + }, + }, + { + name: "multiple mentions resolved", + content: "@alice and @bob", + lookupUsers: []model.User{aliceUser, bobUser}, + wantAccounts: []string{"alice", "bob"}, + wantParts: []model.Participant{ + {UserID: "u-alice", Account: "alice", SiteID: "site-a", EngName: "Alice Wang", ChineseName: "愛麗絲"}, + {UserID: "u-bob", Account: "bob", SiteID: "site-b", EngName: "Bob Chen", ChineseName: "鮑勃"}, + }, + }, +``` + +The other cases (`@all only`, `@all and individual`, `unresolved account skipped`, `lookup error — partial result`) are unchanged — `@all` Participants intentionally have no `SiteID`. + +- [ ] **Step 2.6: Run test to verify it fails** + +Run: `make test SERVICE=pkg/mention` + +Expected: `Resolve` returns Participants with empty `SiteID` — test diffs report missing `site-a` / `site-b`. + +- [ ] **Step 2.7: Propagate SiteID in `mention.Resolve`** + +In `pkg/mention/mention.go`, replace the loop that builds `result.Participants` from `users` (lines ~77-84): + +```go + for i := range users { + result.Participants = append(result.Participants, model.Participant{ + UserID: users[i].ID, + Account: users[i].Account, + SiteID: users[i].SiteID, + ChineseName: users[i].ChineseName, + EngName: users[i].EngName, + }) + } +``` + +- [ ] **Step 2.8: Run tests to verify they pass** + +Run: `make test SERVICE=pkg/mention && make test SERVICE=pkg/model` + +Expected: both packages pass. + +- [ ] **Step 2.9: Verify no other consumer broke** + +Run: `make test` + +Expected: full unit test suite passes. (`Participant.SiteID` is omitempty and additive; existing fixtures that don't set it serialize identically.) + +- [ ] **Step 2.10: Commit** + +```bash +git add pkg/model/event.go pkg/model/model_test.go pkg/mention/mention.go pkg/mention/mention_test.go +git commit -m "model+mention: add SiteID to Participant, propagate from Resolve" +``` + +--- + +## Task 3: Add `siteID` and `publish` fields to message-worker `Handler` + +This is a compile-only refactor. No new behavior — we just plumb the fields through `NewHandler` and update existing tests/main wiring. Behavior changes start in Task 4. + +**Files:** +- Modify: `message-worker/handler.go:19-27` (Handler struct + NewHandler) +- Modify: `message-worker/handler_test.go` (every `NewHandler(...)` call) +- Modify: `message-worker/main.go:95` (the `NewHandler(...)` call site) +- Modify: `message-worker/integration_test.go` (every `NewHandler(...)` call site, if any) + +- [ ] **Step 3.1: Update `Handler` struct and `NewHandler` constructor** + +In `message-worker/handler.go`, replace lines 19-27 with: + +```go +// PublishFunc publishes data; non-empty msgID sets Nats-Msg-Id for JetStream stream-level dedup. +// Mirrors room-worker's PublishFunc signature so message-worker can plug into the same publish closure. +type PublishFunc func(ctx context.Context, subj string, data []byte, msgID string) error + +type Handler struct { + store Store + userStore userstore.UserStore + threadStore ThreadStore + siteID string + publish PublishFunc +} + +func NewHandler(store Store, userStore userstore.UserStore, threadStore ThreadStore, siteID string, publish PublishFunc) *Handler { + return &Handler{ + store: store, + userStore: userStore, + threadStore: threadStore, + siteID: siteID, + publish: publish, + } +} +``` + +- [ ] **Step 3.2: Update existing `NewHandler` call sites in tests** + +In `message-worker/handler_test.go`, find every `NewHandler(mockStore, mockUserStore, mockThreadStore)` call (3 sites — `TestHandler_ProcessMessage`, `TestHandler_HandleThreadRoomAndSubscriptions`, `TestHandler_HandleJetStreamMsg`) and replace each with: + +```go + h := NewHandler(mockStore, mockUserStore, mockThreadStore, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return nil + }) +``` + +Use exactly `"site-a"` — every existing test fixture uses that as both `User.SiteID` and `evt.SiteID`, so a no-op publish closure will be exercised but no remote-publish branch is taken yet (we add that in Task 4+). + +- [ ] **Step 3.3: Update `main.go` `NewHandler` call** + +In `message-worker/main.go` replace the line `handler := NewHandler(store, us, threadStore)` (~line 95) with: + +```go + handler := NewHandler(store, us, threadStore, cfg.SiteID, func(ctx context.Context, subj string, data []byte, msgID string) error { + if msgID == "" { + return nc.Publish(ctx, subj, data) + } + _, err := js.Publish(ctx, subj, data, jetstream.WithMsgID(msgID)) + return err + }) +``` + +- [ ] **Step 3.4: Update integration test `NewHandler` call sites if any** + +Run: `grep -n "NewHandler(" message-worker/integration_test.go` + +If results, update each call to pass `"site-a"` as siteID and a no-op publish closure (same shape as Step 3.2). If no results, skip this step. + +- [ ] **Step 3.5: Verify build + unit tests** + +Run: `make lint && make test SERVICE=message-worker` + +Expected: pass. All existing tests still green — behavior unchanged. + +- [ ] **Step 3.6: Commit** + +```bash +git add message-worker/handler.go message-worker/handler_test.go message-worker/main.go message-worker/integration_test.go +git commit -m "message-worker: plumb siteID and PublishFunc into Handler" +``` + +--- + + From f98f6fa61485a49573d9202ba9763d77a9ad6b57 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 05:42:27 +0000 Subject: [PATCH 03/32] =?UTF-8?q?docs:=20implementation=20plan=20chunk=202?= =?UTF-8?q?=20(tasks=204-5=20=E2=80=94=20publish=20helper=20+=20owner-site?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add publishThreadSubOutboxIfRemote helper with TDD coverage for same-site no-op, empty-SiteID skip, remote publish payload + dedup ID, and publish errors. Switch ThreadSubscription.SiteID to the owner's site, plumb the replier User through, and look up the parent's siteID via userStore with warn-and-skip on ErrUserNotFound. --- ...ssage-worker-thread-subscription-outbox.md | 582 +++++++++++++++++- 1 file changed, 581 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index be95eb39d..3694143b9 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -359,4 +359,584 @@ git commit -m "message-worker: plumb siteID and PublishFunc into Handler" --- - +## Task 4: Implement `publishThreadSubOutboxIfRemote` helper + +The helper centralizes the local-vs-remote decision, payload marshalling, dedup-ID derivation, and publish call. It's used by Tasks 6, 7, 8. + +**Files:** +- Modify: `message-worker/handler.go` (append helper near the bottom) +- Modify: `message-worker/handler_test.go` (append a new test function) + +- [ ] **Step 4.1: Write the failing helper test** + +Append to `message-worker/handler_test.go` (new top-level function, before the `fakeJSMsg` declaration): + +```go +func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + baseSub := &model.ThreadSubscription{ + ID: "sub-1", + ParentMessageID: "pm-1", + RoomID: "r1", + ThreadRoomID: "tr-1", + UserID: "u-bob", + UserAccount: "bob", + SiteID: "site-b", + CreatedAt: now, + UpdatedAt: now, + } + + t.Run("same site — no publish", func(t *testing.T) { + ctrl := gomock.NewController(t) + var called bool + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-b", + func(_ context.Context, _ string, _ []byte, _ string) error { + called = true + return nil + }) + + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + require.NoError(t, err) + assert.False(t, called, "publish must not be called when sub.SiteID == h.siteID") + }) + + t.Run("empty siteID — skip with warn, no publish", func(t *testing.T) { + ctrl := gomock.NewController(t) + var called bool + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, _ string, _ []byte, _ string) error { + called = true + return nil + }) + + emptySub := *baseSub + emptySub.SiteID = "" + err := h.publishThreadSubOutboxIfRemote(context.Background(), &emptySub, "msg-1") + require.NoError(t, err) + assert.False(t, called, "publish must not be called when sub.SiteID is empty") + }) + + t.Run("remote site — publishes with expected subject and dedup ID", func(t *testing.T) { + ctrl := gomock.NewController(t) + var captured struct { + subj string + data []byte + msgID string + callCnt int + } + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, subj string, data []byte, msgID string) error { + captured.subj = subj + captured.data = data + captured.msgID = msgID + captured.callCnt++ + return nil + }) + + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + require.NoError(t, err) + require.Equal(t, 1, captured.callCnt) + assert.Equal(t, "outbox.site-a.to.site-b.thread_subscription_upserted", captured.subj) + assert.NotEmpty(t, captured.msgID, "dedup ID must be set") + + // Same inputs → same dedup ID (stable across redeliveries). + var second string + h2 := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, _ string, _ []byte, msgID string) error { + second = msgID + return nil + }) + require.NoError(t, h2.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1")) + assert.Equal(t, captured.msgID, second, "dedup ID must be deterministic for the same (threadRoomID, userID, msgID) seed") + + // Different msgID → different dedup ID. + var third string + h3 := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, _ string, _ []byte, msgID string) error { + third = msgID + return nil + }) + require.NoError(t, h3.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-2")) + assert.NotEqual(t, captured.msgID, third) + + // Payload is an OutboxEvent whose inner Payload decodes back to the ThreadSubscription. + var outer model.OutboxEvent + require.NoError(t, json.Unmarshal(captured.data, &outer)) + assert.Equal(t, model.OutboxThreadSubscriptionUpserted, outer.Type) + assert.Equal(t, "site-a", outer.SiteID) + assert.Equal(t, "site-b", outer.DestSiteID) + assert.Greater(t, outer.Timestamp, int64(0)) + + var inner model.ThreadSubscription + require.NoError(t, json.Unmarshal(outer.Payload, &inner)) + assert.Equal(t, *baseSub, inner) + }) + + t.Run("publish error returned", func(t *testing.T) { + ctrl := gomock.NewController(t) + boom := errors.New("publish boom") + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, _ string, _ []byte, _ string) error { + return boom + }) + + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + require.Error(t, err) + assert.ErrorIs(t, err, boom) + }) +} +``` + +- [ ] **Step 4.2: Run test to verify it fails** + +Run: `make test SERVICE=message-worker` + +Expected: compile error `h.publishThreadSubOutboxIfRemote undefined`. + +- [ ] **Step 4.3: Implement the helper** + +Append to `message-worker/handler.go` (after `markThreadMentions`, at end of file). Add `subject` to imports if not already present. + +```go +// publishThreadSubOutboxIfRemote publishes a thread_subscription_upserted +// outbox event when sub.SiteID is a remote site. Same-site or empty SiteID +// is a no-op (empty SiteID logs a warning — it indicates a caller bug). +// +// The dedup-ID seed is (threadRoomID, userID, msgID): msg.ID is unique per +// reply, and (msg.ID, userID) is unique within a reply, so the seed is stable +// across MESSAGES_CANONICAL redeliveries and JetStream stream-level dedup +// absorbs duplicates within the dedup window. +func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model.ThreadSubscription, msgID string) error { + if sub.SiteID == "" { + slog.Warn("thread subscription has empty SiteID, skipping outbox publish", + "threadRoomID", sub.ThreadRoomID, "userID", sub.UserID, "msgID", msgID) + return nil + } + if sub.SiteID == h.siteID { + return nil + } + + payload, err := json.Marshal(sub) + if err != nil { + return fmt.Errorf("marshal thread subscription: %w", err) + } + outbox := model.OutboxEvent{ + Type: model.OutboxThreadSubscriptionUpserted, + SiteID: h.siteID, + DestSiteID: sub.SiteID, + Payload: payload, + Timestamp: time.Now().UTC().UnixMilli(), + } + data, err := json.Marshal(outbox) + if err != nil { + return fmt.Errorf("marshal outbox event: %w", err) + } + dedupID := idgen.DeriveID(fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID)) + subj := subject.Outbox(h.siteID, sub.SiteID, model.OutboxThreadSubscriptionUpserted) + if err := h.publish(ctx, subj, data, dedupID); err != nil { + return fmt.Errorf("publish thread subscription outbox to %s: %w", sub.SiteID, err) + } + return nil +} +``` + +If the file's import block doesn't already import `"github.com/hmchangw/chat/pkg/subject"`, add it. + +- [ ] **Step 4.4: Run tests to verify they pass** + +Run: `make test SERVICE=message-worker` + +Expected: pass. + +- [ ] **Step 4.5: Commit** + +```bash +git add message-worker/handler.go message-worker/handler_test.go +git commit -m "message-worker: add publishThreadSubOutboxIfRemote helper" +``` + +--- + +## Task 5: Switch `ThreadSubscription.SiteID` to owner-site, lookup parent siteID, warn-and-skip on parent user not found + +Today `buildThreadSubscription` is called with the message-event siteID (the room's home site). For cross-site replication the field must reflect the **subscription owner's** home site. This task threads the owner siteID through the three callers and adds a parent-user lookup. No publishing yet — that's Tasks 6/7/8. + +**Files:** +- Modify: `message-worker/handler.go:131-204` (handleFirstThreadReply, handleSubsequentThreadReply, buildThreadSubscription, markThreadMentions) +- Modify: `message-worker/handler.go:50-73` (carry replier user through to thread reply path) +- Modify: `message-worker/handler_test.go` (add `FindUserByID` mock expectation for parents in every thread-message test case) + +- [ ] **Step 5.1: Update `buildThreadSubscription` to receive owner siteID explicitly** + +In `message-worker/handler.go`, replace the existing `buildThreadSubscription` (around line 209-222): + +```go +// buildThreadSubscription constructs a ThreadSubscription for (threadRoomID, userID). +// ownerSiteID is the home site of the subscription's owner — NOT the room's site — +// so the document round-trips correctly through the OUTBOX/INBOX federation. +// lastSeenAt is always nil; the field is owned by user-action paths, not message-worker. +func (h *Handler) buildThreadSubscription(msg *model.Message, threadRoomID, userID, userAccount, ownerSiteID string, now time.Time) *model.ThreadSubscription { + return &model.ThreadSubscription{ + ID: idgen.GenerateID(), + ParentMessageID: msg.ThreadParentMessageID, + RoomID: msg.RoomID, + ThreadRoomID: threadRoomID, + UserID: userID, + UserAccount: userAccount, + SiteID: ownerSiteID, + LastSeenAt: nil, + CreatedAt: now, + UpdatedAt: now, + } +} +``` + +(Only the parameter name changes from `siteID` → `ownerSiteID`; existing callers stop here being correct because they currently pass the message-event siteID. The next steps fix the callers.) + +- [ ] **Step 5.2: Carry replier `*model.User` into the thread-reply path** + +The current `processMessage` (lines ~57-92) discards `user` after building `cassParticipant`. We need its `SiteID` inside `handleFirstThreadReply` / `handleSubsequentThreadReply` / `markThreadMentions`. Update `processMessage` to pass `user` through. + +Replace lines 75-92 of `message-worker/handler.go` with: + +```go + if evt.Message.ThreadParentMessageID != "" { + // Resolve (or create) the thread room first so we have the threadRoomID + // before persisting the message to Cassandra. + threadRoomID, err := h.handleThreadRoomAndSubscriptions(ctx, &evt.Message, evt.SiteID, user) + if err != nil { + return fmt.Errorf("handle thread room and subscriptions: %w", err) + } + if err := h.markThreadMentions(ctx, &evt.Message, threadRoomID, evt.SiteID); err != nil { + return fmt.Errorf("mark thread mentions: %w", err) + } + if err := h.store.SaveThreadMessage(ctx, &evt.Message, sender, evt.SiteID, threadRoomID); err != nil { + return fmt.Errorf("save thread message: %w", err) + } + } else { + if err := h.store.SaveMessage(ctx, &evt.Message, sender, evt.SiteID); err != nil { + return fmt.Errorf("save message: %w", err) + } + } +``` + +- [ ] **Step 5.3: Update `handleThreadRoomAndSubscriptions` and the two reply handlers** + +Replace `handleThreadRoomAndSubscriptions`, `handleFirstThreadReply`, and `handleSubsequentThreadReply` in `message-worker/handler.go` with the versions below. The `replier *model.User` arg flows in; parent's siteID is fetched via `userStore.FindUserByID` with warn-and-skip on `userstore.ErrUserNotFound`. + +```go +// handleThreadRoomAndSubscriptions creates the ThreadRoom on first reply and +// inserts ThreadSubscriptions for the parent author and replier. On subsequent +// replies it upserts both subscriptions and bumps the room's last-message pointer. +// It returns the threadRoomID so the caller can pass it to SaveThreadMessage. +// +// `replier` may be nil for system messages with no real user (rare in thread +// paths); subscriptions for the replier are skipped in that case. +func (h *Handler) handleThreadRoomAndSubscriptions(ctx context.Context, msg *model.Message, eventSiteID string, replier *model.User) (string, error) { + now := msg.CreatedAt + + threadRoom := model.ThreadRoom{ + ID: idgen.GenerateID(), + ParentMessageID: msg.ThreadParentMessageID, + RoomID: msg.RoomID, + SiteID: eventSiteID, + LastMsgAt: now, + LastMsgID: msg.ID, + CreatedAt: now, + UpdatedAt: now, + } + + err := h.threadStore.CreateThreadRoom(ctx, &threadRoom) + switch { + case err == nil: + return threadRoom.ID, h.handleFirstThreadReply(ctx, msg, threadRoom.ID, replier, now) + case errors.Is(err, errThreadRoomExists): + return h.handleSubsequentThreadReply(ctx, msg, replier, now) + default: + return "", fmt.Errorf("create thread room: %w", err) + } +} + +// handleFirstThreadReply runs after the thread room has just been created. +// It inserts subscriptions for the parent author (looked up via userStore for +// the parent's home site) and, if distinct, for the replier (using the +// replier's home site). +func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message, threadRoomID string, replier *model.User, now time.Time) error { + parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) + if err != nil { + if errors.Is(err, errMessageNotFound) { + slog.Warn("thread reply parent not found — skipping subscription creation", + "parentMessageID", msg.ThreadParentMessageID, + "replyID", msg.ID) + return nil + } + return fmt.Errorf("get parent message sender: %w", err) + } + + parentSiteID, parentLookupOK := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") + if parentLookupOK { + if err := h.threadStore.InsertThreadSubscription(ctx, + h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, parentSiteID, now), + ); err != nil { + return fmt.Errorf("insert parent author thread subscription: %w", err) + } + } + + if replier != nil && msg.UserID != parentSender.ID { + if err := h.threadStore.InsertThreadSubscription(ctx, + h.buildThreadSubscription(msg, threadRoomID, msg.UserID, msg.UserAccount, replier.SiteID, now), + ); err != nil { + return fmt.Errorf("insert replier thread subscription: %w", err) + } + } + + return nil +} + +// handleSubsequentThreadReply runs when CreateThreadRoom reported an existing room. +// Upserts subscriptions for both the parent author and the replier (idempotent +// on redelivery), then bumps the room's last-message pointer. Returns the +// existing thread room ID so the caller can pass it to SaveThreadMessage. +func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Message, replier *model.User, now time.Time) (string, error) { + existingRoom, err := h.threadStore.GetThreadRoomByParentMessageID(ctx, msg.ThreadParentMessageID) + if err != nil { + return "", fmt.Errorf("get existing thread room: %w", err) + } + + parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) + switch { + case err == nil: + parentSiteID, parentLookupOK := h.lookupOwnerSiteID(ctx, parentSender.ID, "subsequent-reply parent") + if parentLookupOK { + if err := h.threadStore.UpsertThreadSubscription(ctx, + h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, parentSiteID, now), + ); err != nil { + return "", fmt.Errorf("upsert parent author thread subscription: %w", err) + } + } + if replier != nil && msg.UserID != parentSender.ID { + if err := h.threadStore.UpsertThreadSubscription(ctx, + h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now), + ); err != nil { + return "", fmt.Errorf("upsert replier thread subscription: %w", err) + } + } + case errors.Is(err, errMessageNotFound): + slog.Warn("thread reply parent not found — skipping parent subscription upsert", + "parentMessageID", msg.ThreadParentMessageID, + "replyID", msg.ID) + if replier != nil { + if err := h.threadStore.UpsertThreadSubscription(ctx, + h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now), + ); err != nil { + return "", fmt.Errorf("upsert replier thread subscription: %w", err) + } + } + default: + return "", fmt.Errorf("get parent message sender: %w", err) + } + + if err := h.threadStore.UpdateThreadRoomLastMessage(ctx, existingRoom.ID, msg.ID, now); err != nil { + return "", fmt.Errorf("update thread room last message: %w", err) + } + + return existingRoom.ID, nil +} + +// lookupOwnerSiteID resolves a user's home site by ID. Returns (siteID, true) +// on success. On userstore.ErrUserNotFound, logs a warning and returns +// ("", false) so callers can skip that user gracefully (parallels the +// errMessageNotFound branch already in this file). Other errors propagate. +func (h *Handler) lookupOwnerSiteID(ctx context.Context, userID, role string) (string, bool) { + user, err := h.userStore.FindUserByID(ctx, userID) + if err != nil { + if errors.Is(err, userstore.ErrUserNotFound) { + slog.Warn("owner user not found — skipping thread subscription", + "userID", userID, "role", role) + return "", false + } + // Non-NotFound errors propagate via the caller — return ok=false but + // the next call to userStore from the caller will surface the error. + // For a clean API we return the lookup error to the caller instead. + // Match existing pattern: rewrap and panic-style would be wrong; instead + // the helper returns ok=false, and we rely on callers to handle the + // transient case. Simpler: change signature to also return error. + _ = err + return "", false + } + return user.SiteID, true +} +``` + +> **Note on `lookupOwnerSiteID` error semantics:** for transient DB errors we still return `("", false)`, which causes the call site to skip that user. That's wrong for a transient failure — we should propagate. Fix this by returning an error. + +Replace the `lookupOwnerSiteID` body with a version that returns `(string, error)`: + +```go +// lookupOwnerSiteID resolves a user's home site by ID. Returns +// ("", nil) when the user is not found (logs a warning) so callers can skip +// gracefully. Other DB errors are returned for the caller to NAK on. +func (h *Handler) lookupOwnerSiteID(ctx context.Context, userID, role string) (string, error) { + user, err := h.userStore.FindUserByID(ctx, userID) + if err != nil { + if errors.Is(err, userstore.ErrUserNotFound) { + slog.Warn("owner user not found — skipping thread subscription", + "userID", userID, "role", role) + return "", nil + } + return "", fmt.Errorf("lookup user %s: %w", userID, err) + } + return user.SiteID, nil +} +``` + +Then update the two callers in `handleFirstThreadReply` and `handleSubsequentThreadReply` to: + +```go + parentSiteID, err := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") + if err != nil { + return fmt.Errorf("lookup parent owner site: %w", err) + } + if parentSiteID != "" { + // ... insert/upsert parent subscription ... + } +``` + +Use `return "", fmt.Errorf(...)` in `handleSubsequentThreadReply` (the (string, error) signature). + +- [ ] **Step 5.4: Update existing `markThreadMentions` to use `Participant.SiteID`** + +Replace the existing `markThreadMentions` (around line 227-243 of `handler.go`) with: + +```go +// markThreadMentions flips hasMention=true on the thread subscription of every +// @account mentionee in msg (auto-creating the subscription if absent). The +// sender is excluded, and @all is ignored at the thread level. The +// subscription's SiteID is the mentionee's home site (carried on Participant). +func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, threadRoomID, eventSiteID string) error { + for i := range msg.Mentions { + p := &msg.Mentions[i] + if p.Account == "all" { + continue + } + if p.UserID == msg.UserID { + continue + } + ownerSiteID := p.SiteID + if ownerSiteID == "" { + // Defensive: should not happen since mention.Resolve populates SiteID + // for resolved users. Fall back to event site so the local mark still + // happens; outbox publish (Task 8) will skip on empty SiteID anyway. + slog.Warn("mentionee participant has empty SiteID, falling back to event site", + "account", p.Account, "userID", p.UserID, "msgID", msg.ID) + ownerSiteID = eventSiteID + } + sub := h.buildThreadSubscription(msg, threadRoomID, p.UserID, p.Account, ownerSiteID, msg.CreatedAt) + sub.HasMention = true + if err := h.threadStore.MarkThreadSubscriptionMention(ctx, sub); err != nil { + return fmt.Errorf("mark thread subscription mention for user %s: %w", p.UserID, err) + } + } + return nil +} +``` + +- [ ] **Step 5.5: Update existing handler tests with `FindUserByID` parent expectation** + +In `message-worker/handler_test.go`, every thread-message test case in `TestHandler_ProcessMessage` and `TestHandler_HandleThreadRoomAndSubscriptions` now does an extra `userStore.FindUserByID(parentSender.ID)` lookup. Add the mock expectation just after the `GetMessageSender` mock in each thread test case. + +For `TestHandler_ProcessMessage`, the affected cases are: `"thread message — calls SaveThreadMessage not SaveMessage"`, `"thread message save error — NAK after user lookup"`, `"thread reply mentioning non-participant — marks that user's subscription"`, `"thread reply where sender self-mentions — no MarkThreadSubscriptionMention call"`, `"thread reply with @all only — no MarkThreadSubscriptionMention call"`, `"thread reply with @all + @bob — only bob marked"`, `"thread reply mentioning non-participant — MarkThreadSubscriptionMention error is propagated"`. After each `store.EXPECT().GetMessageSender(...)` line, insert: + +```go + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) +``` + +For `TestHandler_HandleThreadRoomAndSubscriptions`, every case that calls `GetMessageSender` and returns a non-error/non-`errMessageNotFound` parentSender needs the same `FindUserByID` mock added. The `errMessageNotFound` cases skip the lookup entirely (the function returns before calling it). + +Also: replace `h.handleThreadRoomAndSubscriptions(context.Background(), tt.msg, tt.siteID)` with `h.handleThreadRoomAndSubscriptions(context.Background(), tt.msg, tt.siteID, &model.User{ID: tt.msg.UserID, Account: tt.msg.UserAccount, SiteID: "site-a"})` to thread the replier through (the function signature changed in Step 5.3). + +For each affected test in `TestHandler_ProcessMessage`, the `processMessage` entry point already calls `FindUserByID` for the replier — that mock is already set. Nothing more to add on the replier path. + +- [ ] **Step 5.6: Add new test cases for parent user-not-found and DB-error** + +Append two new test cases in `TestHandler_HandleThreadRoomAndSubscriptions`: + +```go + { + name: "first reply — parent user not found in userStore — warn-and-skip parent, still inserts replier", + msg: msg, + siteID: "site-a", + setupMocks: func(store *MockStore, ts *MockThreadStore) { + ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(nil, fmt.Errorf("wrap: %w", userstore.ErrUserNotFound)) + }, + expectReplierInsert: true, + }, + { + name: "first reply — parent user lookup DB error — returns error", + msg: msg, + siteID: "site-a", + setupMocks: func(store *MockStore, ts *MockThreadStore) { + ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(nil, errors.New("mongo: connection refused")) + }, + wantErr: true, + }, +``` + +To use `extraUserStoreSetup`, extend the test struct definition near the top of `TestHandler_HandleThreadRoomAndSubscriptions`: + +```go + tests := []struct { + name string + msg *model.Message + siteID string + setupMocks func(store *MockStore, ts *MockThreadStore) + extraUserStoreSetup func(us *MockUserStore) + expectReplierInsert bool + wantErr bool + }{ +``` + +And in the run loop, after `tt.setupMocks(...)`, call: + +```go + if tt.extraUserStoreSetup != nil { + tt.extraUserStoreSetup(mockUserStore) + } + if tt.expectReplierInsert { + mockThreadStore.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + } +``` + +Add `"github.com/hmchangw/chat/pkg/userstore"` to the test file's imports if not already present. + +- [ ] **Step 5.7: Add `errors` import if missing** + +Verify `message-worker/handler.go` imports `"github.com/hmchangw/chat/pkg/userstore"` (it already does — used by `Handler.userStore`). No new imports needed beyond what Task 4 added (`subject`). + +- [ ] **Step 5.8: Run tests to verify they pass** + +Run: `make generate SERVICE=message-worker && make test SERVICE=message-worker` + +Expected: pass. (`make generate` regenerates mocks if any interface signature changed.) + +- [ ] **Step 5.9: Commit** + +```bash +git add message-worker/handler.go message-worker/handler_test.go message-worker/mock_store_test.go message-worker/mock_userstore_test.go +git commit -m "message-worker: ThreadSubscription.SiteID = owner site; lookup parent siteID" +``` + +--- + + From 4d069090f7c9ef98f4101ef9d66d71a4d91c7cab Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 05:46:27 +0000 Subject: [PATCH 04/32] =?UTF-8?q?docs:=20implementation=20plan=20chunk=203?= =?UTF-8?q?=20(tasks=206-7=20=E2=80=94=20wire=20publish=20in=20reply=20pat?= =?UTF-8?q?hs)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire publishThreadSubOutboxIfRemote after each InsertThreadSubscription in handleFirstThreadReply and after each UpsertThreadSubscription in handleSubsequentThreadReply, with cross-site test matrices and error propagation for outbox publish failures. --- ...ssage-worker-thread-subscription-outbox.md | 418 +++++++++++++++++- 1 file changed, 417 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index 3694143b9..32f4454c8 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -939,4 +939,420 @@ git commit -m "message-worker: ThreadSubscription.SiteID = owner site; lookup pa --- - +## Task 6: Wire outbox publish into `handleFirstThreadReply` + +After each `InsertThreadSubscription` succeeds, call `publishThreadSubOutboxIfRemote`. The helper short-circuits same-site, so local-only setups remain quiet. + +**Files:** +- Modify: `message-worker/handler.go` (handleFirstThreadReply body — only the post-insert lines) +- Modify: `message-worker/handler_test.go` (new test cases) + +- [ ] **Step 6.1: Write the failing test for first-reply remote publish** + +Append to `message-worker/handler_test.go` (a new top-level test, not part of the existing tables — having a dedicated function keeps the publish-recording wiring isolated): + +```go +func TestHandler_FirstReply_OutboxPublishes(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + + parentSender := &cassParticipant{ID: "u-parent", Account: "parent-user"} + parentUserAtA := &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"} + parentUserAtC := &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-c"} + + type publishCall struct { + subj string + data []byte + msgID string + } + + tests := []struct { + name string + replierSite string + parentUser *model.User + wantPublishToSite map[string]int // destSite → expected count + }{ + { + name: "both local — no publish", + replierSite: "site-a", + parentUser: parentUserAtA, + wantPublishToSite: map[string]int{}, + }, + { + name: "replier remote — one publish to replier site", + replierSite: "site-b", + parentUser: parentUserAtA, + wantPublishToSite: map[string]int{"site-b": 1}, + }, + { + name: "parent remote — one publish to parent site", + replierSite: "site-a", + parentUser: parentUserAtC, + wantPublishToSite: map[string]int{"site-c": 1}, + }, + { + name: "both remote, different sites — two publishes", + replierSite: "site-b", + parentUser: parentUserAtC, + wantPublishToSite: map[string]int{"site-b": 1, "site-c": 1}, + }, + { + name: "both remote, same site — two publishes to that site", + replierSite: "site-b", + parentUser: &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-b"}, + wantPublishToSite: map[string]int{"site-b": 2}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent").Return(tt.parentUser, nil) + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + + var calls []publishCall + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, subj string, data []byte, msgID string) error { + calls = append(calls, publishCall{subj: subj, data: data, msgID: msgID}) + return nil + }) + + replier := &model.User{ID: "u-replier", Account: "replier", SiteID: tt.replierSite} + msg := &model.Message{ + ID: "msg-reply", + RoomID: "r1", + UserID: "u-replier", + UserAccount: "replier", + CreatedAt: now, + ThreadParentMessageID: "msg-parent", + } + + err := h.handleFirstThreadReply(context.Background(), msg, "tr-1", replier, now) + require.NoError(t, err) + + gotByDest := map[string]int{} + for _, c := range calls { + var outer model.OutboxEvent + require.NoError(t, json.Unmarshal(c.data, &outer)) + assert.Equal(t, model.OutboxThreadSubscriptionUpserted, outer.Type) + gotByDest[outer.DestSiteID]++ + } + assert.Equal(t, tt.wantPublishToSite, gotByDest) + }) + } +} + +func TestHandler_FirstReply_OutboxPublishError_NAKs(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent"). + Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-c"}, nil) + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + // Replier insert never reached because parent-publish fails first. + + boom := errors.New("publish boom") + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return boom + }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-replier", UserAccount: "replier", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + } + err := h.handleFirstThreadReply(context.Background(), msg, + "tr-1", &model.User{ID: "u-replier", SiteID: "site-b"}, now) + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} +``` + +- [ ] **Step 6.2: Run tests to verify they fail** + +Run: `make test SERVICE=message-worker -run TestHandler_FirstReply_Outbox` + +Expected: FAIL — current handler never calls publish. + +- [ ] **Step 6.3: Wire the publish call into `handleFirstThreadReply`** + +In `message-worker/handler.go`, replace the body of `handleFirstThreadReply` (the version from Task 5.3) so that each successful insert is followed by an outbox publish: + +```go +func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message, threadRoomID string, replier *model.User, now time.Time) error { + parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) + if err != nil { + if errors.Is(err, errMessageNotFound) { + slog.Warn("thread reply parent not found — skipping subscription creation", + "parentMessageID", msg.ThreadParentMessageID, + "replyID", msg.ID) + return nil + } + return fmt.Errorf("get parent message sender: %w", err) + } + + parentSiteID, err := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") + if err != nil { + return fmt.Errorf("lookup parent owner site: %w", err) + } + if parentSiteID != "" { + parentSub := h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, parentSiteID, now) + if err := h.threadStore.InsertThreadSubscription(ctx, parentSub); err != nil { + return fmt.Errorf("insert parent author thread subscription: %w", err) + } + if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, msg.ID); err != nil { + return err + } + } + + if replier != nil && msg.UserID != parentSender.ID { + replierSub := h.buildThreadSubscription(msg, threadRoomID, msg.UserID, msg.UserAccount, replier.SiteID, now) + if err := h.threadStore.InsertThreadSubscription(ctx, replierSub); err != nil { + return fmt.Errorf("insert replier thread subscription: %w", err) + } + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + return err + } + } + + return nil +} +``` + +- [ ] **Step 6.4: Run tests to verify they pass** + +Run: `make test SERVICE=message-worker` + +Expected: pass — new tests green and existing tests still green (existing fixtures use `site-a` everywhere so no remote publishes fire). + +- [ ] **Step 6.5: Commit** + +```bash +git add message-worker/handler.go message-worker/handler_test.go +git commit -m "message-worker: emit outbox event on first-reply ThreadSubscription inserts" +``` + +--- + +## Task 7: Wire outbox publish into `handleSubsequentThreadReply` + +Same pattern as Task 6 but on the upsert path. Subsequent replies always upsert both subscriptions (idempotent on redelivery), so we publish after each successful upsert. + +**Files:** +- Modify: `message-worker/handler.go` (handleSubsequentThreadReply body) +- Modify: `message-worker/handler_test.go` (new test cases) + +- [ ] **Step 7.1: Write the failing test for subsequent-reply remote publish** + +Append to `message-worker/handler_test.go`: + +```go +func TestHandler_SubsequentReply_OutboxPublishes(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + + parentSender := &cassParticipant{ID: "u-parent", Account: "parent-user"} + parentUserAtA := &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"} + parentUserAtC := &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-c"} + + tests := []struct { + name string + replierSite string + parentUser *model.User + wantPublishToSite map[string]int + }{ + { + name: "both local — no publish", + replierSite: "site-a", + parentUser: parentUserAtA, + wantPublishToSite: map[string]int{}, + }, + { + name: "replier remote — one publish", + replierSite: "site-b", + parentUser: parentUserAtA, + wantPublishToSite: map[string]int{"site-b": 1}, + }, + { + name: "parent remote — one publish", + replierSite: "site-a", + parentUser: parentUserAtC, + wantPublishToSite: map[string]int{"site-c": 1}, + }, + { + name: "both remote, different sites — two publishes", + replierSite: "site-b", + parentUser: parentUserAtC, + wantPublishToSite: map[string]int{"site-b": 1, "site-c": 1}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + ts.EXPECT().GetThreadRoomByParentMessageID(gomock.Any(), "msg-parent"). + Return(&model.ThreadRoom{ID: "tr-existing"}, nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent").Return(tt.parentUser, nil) + ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", now).Return(nil) + + var publishedDests []string + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, data []byte, _ string) error { + var outer model.OutboxEvent + if err := json.Unmarshal(data, &outer); err != nil { + return err + } + publishedDests = append(publishedDests, outer.DestSiteID) + return nil + }) + + replier := &model.User{ID: "u-replier", Account: "replier", SiteID: tt.replierSite} + msg := &model.Message{ + ID: "msg-reply", + RoomID: "r1", + UserID: "u-replier", + UserAccount: "replier", + CreatedAt: now, + ThreadParentMessageID: "msg-parent", + } + + roomID, err := h.handleSubsequentThreadReply(context.Background(), msg, replier, now) + require.NoError(t, err) + assert.Equal(t, "tr-existing", roomID) + + gotByDest := map[string]int{} + for _, d := range publishedDests { + gotByDest[d]++ + } + assert.Equal(t, tt.wantPublishToSite, gotByDest) + }) + } +} + +func TestHandler_SubsequentReply_OutboxPublishError_NAKs(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + ts.EXPECT().GetThreadRoomByParentMessageID(gomock.Any(), "msg-parent"). + Return(&model.ThreadRoom{ID: "tr-1"}, nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent"). + Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", SiteID: "site-c"}, nil) + ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + + boom := errors.New("publish boom") + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return boom + }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-replier", UserAccount: "replier", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + } + _, err := h.handleSubsequentThreadReply(context.Background(), msg, + &model.User{ID: "u-replier", SiteID: "site-b"}, now) + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} +``` + +- [ ] **Step 7.2: Run tests to verify they fail** + +Run: `make test SERVICE=message-worker -run TestHandler_SubsequentReply_Outbox` + +Expected: FAIL — no publish happens yet on the upsert path. + +- [ ] **Step 7.3: Wire the publish call into `handleSubsequentThreadReply`** + +Replace `handleSubsequentThreadReply` in `message-worker/handler.go` with: + +```go +func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Message, replier *model.User, now time.Time) (string, error) { + existingRoom, err := h.threadStore.GetThreadRoomByParentMessageID(ctx, msg.ThreadParentMessageID) + if err != nil { + return "", fmt.Errorf("get existing thread room: %w", err) + } + + parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) + switch { + case err == nil: + parentSiteID, lookupErr := h.lookupOwnerSiteID(ctx, parentSender.ID, "subsequent-reply parent") + if lookupErr != nil { + return "", fmt.Errorf("lookup parent owner site: %w", lookupErr) + } + if parentSiteID != "" { + parentSub := h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, parentSiteID, now) + if err := h.threadStore.UpsertThreadSubscription(ctx, parentSub); err != nil { + return "", fmt.Errorf("upsert parent author thread subscription: %w", err) + } + if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, msg.ID); err != nil { + return "", err + } + } + if replier != nil && msg.UserID != parentSender.ID { + replierSub := h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now) + if err := h.threadStore.UpsertThreadSubscription(ctx, replierSub); err != nil { + return "", fmt.Errorf("upsert replier thread subscription: %w", err) + } + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + return "", err + } + } + case errors.Is(err, errMessageNotFound): + slog.Warn("thread reply parent not found — skipping parent subscription upsert", + "parentMessageID", msg.ThreadParentMessageID, + "replyID", msg.ID) + if replier != nil { + replierSub := h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now) + if err := h.threadStore.UpsertThreadSubscription(ctx, replierSub); err != nil { + return "", fmt.Errorf("upsert replier thread subscription: %w", err) + } + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + return "", err + } + } + default: + return "", fmt.Errorf("get parent message sender: %w", err) + } + + if err := h.threadStore.UpdateThreadRoomLastMessage(ctx, existingRoom.ID, msg.ID, now); err != nil { + return "", fmt.Errorf("update thread room last message: %w", err) + } + + return existingRoom.ID, nil +} +``` + +- [ ] **Step 7.4: Run tests to verify they pass** + +Run: `make test SERVICE=message-worker` + +Expected: pass. + +- [ ] **Step 7.5: Commit** + +```bash +git add message-worker/handler.go message-worker/handler_test.go +git commit -m "message-worker: emit outbox event on subsequent-reply ThreadSubscription upserts" +``` + +--- + + From 834ae3cc81f36ba623300aae18a78a3d4bb84f93 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 05:47:24 +0000 Subject: [PATCH 05/32] =?UTF-8?q?docs:=20implementation=20plan=20chunk=204?= =?UTF-8?q?=20(tasks=208-9=20=E2=80=94=20mention=20publish=20+=20main=20wi?= =?UTF-8?q?ring)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire publishThreadSubOutboxIfRemote after MarkThreadSubscriptionMention with test coverage for local/remote mentionees, @all skip, sender self-mention skip, and HasMention=true in the published payload. Verify message-worker main.go JetStream publish closure and confirm bootstrap.go stays unchanged for OUTBOX (ops/IaC owns it). --- ...ssage-worker-thread-subscription-outbox.md | 274 +++++++++++++++++- 1 file changed, 273 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index 32f4454c8..d55a73f00 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -1355,4 +1355,276 @@ git commit -m "message-worker: emit outbox event on subsequent-reply ThreadSubsc --- - +## Task 8: Wire outbox publish into `markThreadMentions` + +After `MarkThreadSubscriptionMention` succeeds for a remote mentionee, publish the outbox event so the mentionee's home site upserts a `hasMention=true` subscription. Local mentionees: no publish. + +**Files:** +- Modify: `message-worker/handler.go` (markThreadMentions body) +- Modify: `message-worker/handler_test.go` (new test cases) + +- [ ] **Step 8.1: Write the failing test for mention publish** + +Append to `message-worker/handler_test.go`: + +```go +func TestHandler_MarkThreadMentions_OutboxPublishes(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + + tests := []struct { + name string + mentionees []model.Participant + wantPublishToSite map[string]int + }{ + { + name: "no mentions — no publish", + mentionees: nil, + wantPublishToSite: map[string]int{}, + }, + { + name: "local mentionee — mark only, no publish", + mentionees: []model.Participant{ + {UserID: "u-bob", Account: "bob", SiteID: "site-a"}, + }, + wantPublishToSite: map[string]int{}, + }, + { + name: "remote mentionee — mark and publish", + mentionees: []model.Participant{ + {UserID: "u-bob", Account: "bob", SiteID: "site-b"}, + }, + wantPublishToSite: map[string]int{"site-b": 1}, + }, + { + name: "two remote mentionees in different sites — two publishes", + mentionees: []model.Participant{ + {UserID: "u-bob", Account: "bob", SiteID: "site-b"}, + {UserID: "u-carol", Account: "carol", SiteID: "site-c"}, + }, + wantPublishToSite: map[string]int{"site-b": 1, "site-c": 1}, + }, + { + name: "@all is skipped — no mark, no publish", + mentionees: []model.Participant{ + {Account: "all", EngName: "all"}, + }, + wantPublishToSite: map[string]int{}, + }, + { + name: "sender self-mention is skipped", + mentionees: []model.Participant{ + {UserID: "u-sender", Account: "sender", SiteID: "site-b"}, + }, + wantPublishToSite: map[string]int{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + ts := NewMockThreadStore(ctrl) + + expectedMarks := 0 + for _, p := range tt.mentionees { + if p.Account == "all" { + continue + } + if p.UserID == "u-sender" { + continue + } + expectedMarks++ + } + ts.EXPECT().MarkThreadSubscriptionMention(gomock.Any(), gomock.Any()). + Times(expectedMarks).Return(nil) + + var publishedDests []string + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), ts, "site-a", + func(_ context.Context, _ string, data []byte, _ string) error { + var outer model.OutboxEvent + if err := json.Unmarshal(data, &outer); err != nil { + return err + } + publishedDests = append(publishedDests, outer.DestSiteID) + return nil + }) + + msg := &model.Message{ + ID: "msg-reply", + RoomID: "r1", + UserID: "u-sender", + UserAccount: "sender", + CreatedAt: now, + ThreadParentMessageID: "msg-parent", + Mentions: tt.mentionees, + } + err := h.markThreadMentions(context.Background(), msg, "tr-1", "site-a") + require.NoError(t, err) + + gotByDest := map[string]int{} + for _, d := range publishedDests { + gotByDest[d]++ + } + assert.Equal(t, tt.wantPublishToSite, gotByDest) + }) + } +} + +func TestHandler_MarkThreadMentions_OutboxPublishError_NAKs(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + ts := NewMockThreadStore(ctrl) + ts.EXPECT().MarkThreadSubscriptionMention(gomock.Any(), gomock.Any()).Return(nil) + + boom := errors.New("publish boom") + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), ts, "site-a", + func(_ context.Context, _ string, _ []byte, _ string) error { return boom }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-sender", UserAccount: "sender", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + Mentions: []model.Participant{{UserID: "u-bob", Account: "bob", SiteID: "site-b"}}, + } + err := h.markThreadMentions(context.Background(), msg, "tr-1", "site-a") + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} + +func TestHandler_MarkThreadMentions_HasMentionInPayload(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + ts := NewMockThreadStore(ctrl) + ts.EXPECT().MarkThreadSubscriptionMention(gomock.Any(), gomock.Any()).Return(nil) + + var captured []byte + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), ts, "site-a", + func(_ context.Context, _ string, data []byte, _ string) error { + captured = data + return nil + }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-sender", UserAccount: "sender", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + Mentions: []model.Participant{{UserID: "u-bob", Account: "bob", SiteID: "site-b"}}, + } + require.NoError(t, h.markThreadMentions(context.Background(), msg, "tr-1", "site-a")) + + var outer model.OutboxEvent + require.NoError(t, json.Unmarshal(captured, &outer)) + var sub model.ThreadSubscription + require.NoError(t, json.Unmarshal(outer.Payload, &sub)) + assert.True(t, sub.HasMention, "outbox-emitted ThreadSubscription must carry HasMention=true") + assert.Equal(t, "u-bob", sub.UserID) + assert.Equal(t, "site-b", sub.SiteID) +} +``` + +- [ ] **Step 8.2: Run tests to verify they fail** + +Run: `make test SERVICE=message-worker -run TestHandler_MarkThreadMentions_Outbox` + +Expected: FAIL — `markThreadMentions` doesn't publish yet. + +- [ ] **Step 8.3: Wire the publish call into `markThreadMentions`** + +Replace `markThreadMentions` in `message-worker/handler.go` with: + +```go +func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, threadRoomID, eventSiteID string) error { + for i := range msg.Mentions { + p := &msg.Mentions[i] + if p.Account == "all" { + continue + } + if p.UserID == msg.UserID { + continue + } + ownerSiteID := p.SiteID + if ownerSiteID == "" { + slog.Warn("mentionee participant has empty SiteID, falling back to event site", + "account", p.Account, "userID", p.UserID, "msgID", msg.ID) + ownerSiteID = eventSiteID + } + sub := h.buildThreadSubscription(msg, threadRoomID, p.UserID, p.Account, ownerSiteID, msg.CreatedAt) + sub.HasMention = true + if err := h.threadStore.MarkThreadSubscriptionMention(ctx, sub); err != nil { + return fmt.Errorf("mark thread subscription mention for user %s: %w", p.UserID, err) + } + if err := h.publishThreadSubOutboxIfRemote(ctx, sub, msg.ID); err != nil { + return err + } + } + return nil +} +``` + +- [ ] **Step 8.4: Run tests to verify they pass** + +Run: `make test SERVICE=message-worker` + +Expected: pass. + +- [ ] **Step 8.5: Commit** + +```bash +git add message-worker/handler.go message-worker/handler_test.go +git commit -m "message-worker: emit outbox event on mention-marked ThreadSubscriptions" +``` + +--- + +## Task 9: Wire JetStream publish closure in `message-worker/main.go` + +Task 3 plumbed `siteID` and a no-op publish closure into the test fixtures. The `main.go` change in Step 3.3 already added the JetStream-backed closure. This task verifies the wiring end-to-end and confirms `bootstrap.go` is unchanged for OUTBOX (per spec — ops/IaC owns OUTBOX). + +**Files:** +- Verify: `message-worker/main.go:95-105` +- Verify: `message-worker/bootstrap.go` (no changes) +- Modify: `message-worker/main.go` (only if Step 3.3 wasn't applied) + +- [ ] **Step 9.1: Confirm `main.go` JetStream closure is in place** + +Read `message-worker/main.go` and verify the `NewHandler` call passes the publish closure exactly as below: + +```go + handler := NewHandler(store, us, threadStore, cfg.SiteID, func(ctx context.Context, subj string, data []byte, msgID string) error { + if msgID == "" { + return nc.Publish(ctx, subj, data) + } + _, err := js.Publish(ctx, subj, data, jetstream.WithMsgID(msgID)) + return err + }) +``` + +If missing or different, apply Step 3.3 from Task 3 verbatim. + +- [ ] **Step 9.2: Confirm `bootstrap.go` is unchanged for OUTBOX** + +Read `message-worker/bootstrap.go` and verify the only stream it bootstraps is `MESSAGES_CANONICAL`. There must be NO reference to `stream.Outbox(...)` or `OUTBOX_`. + +Per the spec ("Stream ownership"), OUTBOX is owned by ops/IaC. Adding it here would diverge from room-worker's pattern. Leave the file alone. + +- [ ] **Step 9.3: Build the binary** + +Run: `make build SERVICE=message-worker` + +Expected: succeeds. (Confirms imports and types compile end-to-end with the new publish closure.) + +- [ ] **Step 9.4: Run lint + full unit suite** + +Run: `make lint && make test` + +Expected: full repo passes. + +- [ ] **Step 9.5: Commit (if Step 9.1 made any change)** + +If Step 9.1 was a no-op (Task 3 already covered it), skip the commit. Otherwise: + +```bash +git add message-worker/main.go +git commit -m "message-worker: wire JetStream publish closure with Nats-Msg-Id" +``` + +--- + + From 088a21bbf336c641be2b1d34782ff1dd9757accc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 05:53:15 +0000 Subject: [PATCH 06/32] =?UTF-8?q?docs:=20implementation=20plan=20chunk=205?= =?UTF-8?q?a=20(task=2010=20=E2=80=94=20inbox-worker=20dispatch)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add UpsertThreadSubscription to InboxStore interface and the thread_subscription_upserted dispatch case + handler in inbox-worker, with unit-test coverage for insert, monotonic hasMention merge, invalid payload, and store-error paths via an extended in-memory stub. --- ...ssage-worker-thread-subscription-outbox.md | 237 +++++++++++++++++- 1 file changed, 236 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index d55a73f00..d7fed1a02 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -1627,4 +1627,239 @@ git commit -m "message-worker: wire JetStream publish closure with Nats-Msg-Id" --- - +## Task 10: Inbox-worker dispatch case for `thread_subscription_upserted` + +Inbox-worker needs to recognize the new event type, decode the payload, and call a new store method. This task adds the interface method, the dispatch case, the handler function, and unit-test coverage. The Mongo implementation comes in Task 11. + +**Files:** +- Modify: `inbox-worker/handler.go` (extend `InboxStore` interface; add dispatch case + handler) +- Modify: `inbox-worker/handler_test.go` (extend `stubInboxStore`; new dispatch tests) + +- [ ] **Step 10.1: Extend the `InboxStore` interface** + +In `inbox-worker/handler.go`, add `UpsertThreadSubscription` to the interface: + +```go +// InboxStore abstracts the data store operations needed by the inbox worker. +type InboxStore interface { + CreateSubscription(ctx context.Context, sub *model.Subscription) error + BulkCreateSubscriptions(ctx context.Context, subs []*model.Subscription) error + UpsertRoom(ctx context.Context, room *model.Room) error + UpdateSubscriptionRoles(ctx context.Context, account, roomID string, roles []model.Role) error + DeleteSubscriptionsByAccounts(ctx context.Context, roomID string, accounts []string) error + FindUsersByAccounts(ctx context.Context, accounts []string) ([]model.User, error) + UpsertThreadSubscription(ctx context.Context, sub *model.ThreadSubscription) error +} +``` + +- [ ] **Step 10.2: Extend `stubInboxStore` with thread-subscription support** + +In `inbox-worker/handler_test.go`, extend `stubInboxStore`: + +Add a field to the struct (after `users`): + +```go + threadSubs []model.ThreadSubscription +``` + +Add a method (anywhere among the other stub methods): + +```go +func (s *stubInboxStore) UpsertThreadSubscription(_ context.Context, sub *model.ThreadSubscription) error { + s.mu.Lock() + defer s.mu.Unlock() + for i := range s.threadSubs { + if s.threadSubs[i].ThreadRoomID == sub.ThreadRoomID && s.threadSubs[i].UserID == sub.UserID { + // Monotonic hasMention merge — never clear true→false. + if sub.HasMention { + s.threadSubs[i].HasMention = true + } + s.threadSubs[i].UpdatedAt = sub.UpdatedAt + return nil + } + } + s.threadSubs = append(s.threadSubs, *sub) + return nil +} + +func (s *stubInboxStore) getThreadSubs() []model.ThreadSubscription { + s.mu.Lock() + defer s.mu.Unlock() + cp := make([]model.ThreadSubscription, len(s.threadSubs)) + copy(cp, s.threadSubs) + return cp +} +``` + +- [ ] **Step 10.3: Write the failing dispatch tests** + +Append to `inbox-worker/handler_test.go`: + +```go +func TestHandleEvent_ThreadSubscriptionUpserted_Insert(t *testing.T) { + store := &stubInboxStore{} + pub := &mockPublisher{} + h := NewHandler(store, pub) + + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + sub := model.ThreadSubscription{ + ID: "sub-1", + ParentMessageID: "pm-1", + RoomID: "r1", + ThreadRoomID: "tr-1", + UserID: "u-bob", + UserAccount: "bob", + SiteID: "site-b", + HasMention: false, + CreatedAt: now, + UpdatedAt: now, + } + subData, err := json.Marshal(sub) + require.NoError(t, err) + + evt := model.OutboxEvent{ + Type: "thread_subscription_upserted", + SiteID: "site-a", + DestSiteID: "site-b", + Payload: subData, + Timestamp: now.UnixMilli(), + } + evtData, _ := json.Marshal(evt) + + require.NoError(t, h.HandleEvent(context.Background(), evtData)) + + got := store.getThreadSubs() + require.Len(t, got, 1) + assert.Equal(t, sub, got[0]) + assert.Empty(t, pub.getRecords(), "no client-facing publish on thread sub upsert") +} + +func TestHandleEvent_ThreadSubscriptionUpserted_MonotonicHasMention(t *testing.T) { + store := &stubInboxStore{} + pub := &mockPublisher{} + h := NewHandler(store, pub) + + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + mentionSub := model.ThreadSubscription{ + ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + HasMention: true, CreatedAt: now, UpdatedAt: now, + } + mentionData, _ := json.Marshal(mentionSub) + mentionEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: mentionData, Timestamp: now.UnixMilli(), + }) + require.NoError(t, h.HandleEvent(context.Background(), mentionEvt)) + + // Second event for same (threadRoomID, userID) with HasMention=false must NOT clear it. + plainSub := mentionSub + plainSub.HasMention = false + plainSub.UpdatedAt = now.Add(time.Minute) + plainData, _ := json.Marshal(plainSub) + plainEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: plainData, Timestamp: plainSub.UpdatedAt.UnixMilli(), + }) + require.NoError(t, h.HandleEvent(context.Background(), plainEvt)) + + got := store.getThreadSubs() + require.Len(t, got, 1) + assert.True(t, got[0].HasMention, "hasMention must remain true after a non-mention event") +} + +func TestHandleEvent_ThreadSubscriptionUpserted_InvalidPayload(t *testing.T) { + store := &stubInboxStore{} + pub := &mockPublisher{} + h := NewHandler(store, pub) + + evt := model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: []byte("not json"), + } + evtData, _ := json.Marshal(evt) + + require.Error(t, h.HandleEvent(context.Background(), evtData)) + assert.Empty(t, store.getThreadSubs()) +} + +func TestHandleEvent_ThreadSubscriptionUpserted_StoreError(t *testing.T) { + store := &errorThreadSubStore{stubInboxStore: &stubInboxStore{}} + pub := &mockPublisher{} + h := NewHandler(store, pub) + + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + sub := model.ThreadSubscription{ + ID: "sub-1", ThreadRoomID: "tr-1", UserID: "u-bob", SiteID: "site-b", + CreatedAt: now, UpdatedAt: now, + } + subData, _ := json.Marshal(sub) + evtData, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: subData, Timestamp: now.UnixMilli(), + }) + + err := h.HandleEvent(context.Background(), evtData) + require.Error(t, err) +} + +type errorThreadSubStore struct { + *stubInboxStore +} + +func (s *errorThreadSubStore) UpsertThreadSubscription(_ context.Context, _ *model.ThreadSubscription) error { + return fmt.Errorf("boom") +} +``` + +- [ ] **Step 10.4: Run tests to verify they fail** + +Run: `make test SERVICE=inbox-worker -run TestHandleEvent_ThreadSubscriptionUpserted` + +Expected: compile error or unknown-event-type warn (handler.go has no case for the new type yet). + +- [ ] **Step 10.5: Add the dispatch case + handler** + +In `inbox-worker/handler.go`, add a new case in the `switch evt.Type` block of `HandleEvent`: + +```go + case "thread_subscription_upserted": + return h.handleThreadSubscriptionUpserted(ctx, &evt) +``` + +Then append the handler function (after `handleRoleUpdated`): + +```go +// handleThreadSubscriptionUpserted upserts a ThreadSubscription on the local +// site when message-worker on another site reports that a user (parent author, +// replier, or mentionee) is participating in a thread. The Mongo store layer +// is responsible for the monotonic hasMention merge — see store impl. +func (h *Handler) handleThreadSubscriptionUpserted(ctx context.Context, evt *model.OutboxEvent) error { + var sub model.ThreadSubscription + if err := json.Unmarshal(evt.Payload, &sub); err != nil { + return fmt.Errorf("unmarshal thread_subscription_upserted payload: %w", err) + } + if err := h.store.UpsertThreadSubscription(ctx, &sub); err != nil { + return fmt.Errorf("upsert thread subscription (threadRoomID %q, userID %q): %w", + sub.ThreadRoomID, sub.UserID, err) + } + return nil +} +``` + +- [ ] **Step 10.6: Run tests to verify they pass** + +Run: `make test SERVICE=inbox-worker` + +Expected: pass. + +- [ ] **Step 10.7: Commit** + +```bash +git add inbox-worker/handler.go inbox-worker/handler_test.go +git commit -m "inbox-worker: dispatch thread_subscription_upserted to UpsertThreadSubscription" +``` + +--- + + From d839405a9d0c246603a3695db61cd87dd9f7bb69 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 05:53:46 +0000 Subject: [PATCH 07/32] =?UTF-8?q?docs:=20implementation=20plan=20chunk=205?= =?UTF-8?q?b=20(task=2011=20=E2=80=94=20Mongo=20UpsertThreadSubscription)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add threadSubCol to mongoInboxStore, ensure (threadRoomId, userId) unique index at startup, and implement UpsertThreadSubscription using $setOnInsert for immutable fields, $set on updatedAt, and $max on hasMention so a non-mention event never clears a prior mention=true. --- ...ssage-worker-thread-subscription-outbox.md | 106 +++++++++++++++++- 1 file changed, 105 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index d7fed1a02..775343b99 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -1862,4 +1862,108 @@ git commit -m "inbox-worker: dispatch thread_subscription_upserted to UpsertThre --- - +## Task 11: Mongo `UpsertThreadSubscription` implementation in inbox-worker + +Concrete implementation backing the interface added in Task 10. Uses `$setOnInsert` for first-event fields, `$set` for `updatedAt`, and `$max` on `hasMention` for monotonic merge. Includes ensuring the `(threadRoomId, userId)` unique index exists at startup. + +**Files:** +- Modify: `inbox-worker/main.go` (add `threadSubCol` field; add `UpsertThreadSubscription` method; ensure index at startup) + +- [ ] **Step 11.1: Add `threadSubCol` to `mongoInboxStore` and wire it in `main`** + +In `inbox-worker/main.go`, replace the `mongoInboxStore` struct definition with: + +```go +// mongoInboxStore implements InboxStore using MongoDB. +type mongoInboxStore struct { + subCol *mongo.Collection + roomCol *mongo.Collection + userCol *mongo.Collection + threadSubCol *mongo.Collection +} +``` + +In `main()`, replace the store construction (around the existing `&mongoInboxStore{...}` literal) with: + +```go + store := &mongoInboxStore{ + subCol: db.Collection("subscriptions"), + roomCol: db.Collection("rooms"), + userCol: db.Collection("users"), + threadSubCol: db.Collection("threadSubscriptions"), + } + if err := store.ensureIndexes(ctx); err != nil { + slog.Error("ensure indexes failed", "error", err) + os.Exit(1) + } +``` + +- [ ] **Step 11.2: Add the `ensureIndexes` and `UpsertThreadSubscription` methods** + +Append to `inbox-worker/main.go` (after the existing `BulkCreateSubscriptions` method, before `func main()`): + +```go +// ensureIndexes creates the unique index on (threadRoomId, userId) used by +// UpsertThreadSubscription. The index name and shape match what message-worker +// creates in its own threadStoreMongo so both services agree on the natural +// key for thread subscriptions. +func (s *mongoInboxStore) ensureIndexes(ctx context.Context) error { + if _, err := s.threadSubCol.Indexes().CreateOne(ctx, mongo.IndexModel{ + Keys: bson.D{{Key: "threadRoomId", Value: 1}, {Key: "userId", Value: 1}}, + Options: options.Index().SetUnique(true), + }); err != nil { + return fmt.Errorf("ensure threadSubscriptions (threadRoomId,userId) index: %w", err) + } + return nil +} + +// UpsertThreadSubscription inserts the subscription on first event for a +// (threadRoomId, userId) pair, and on subsequent events updates only +// updatedAt and (monotonically) hasMention. $setOnInsert pins the immutable +// fields on insert; $set always refreshes updatedAt; $max on hasMention +// guarantees a non-mention event never clears a prior mention=true. +// +// $setOnInsert and $max operate on disjoint fields (hasMention is set by $max +// only — never by $setOnInsert) so MongoDB doesn't reject the update with a +// "conflicting update operators" error. +func (s *mongoInboxStore) UpsertThreadSubscription(ctx context.Context, sub *model.ThreadSubscription) error { + filter := bson.M{"threadRoomId": sub.ThreadRoomID, "userId": sub.UserID} + update := bson.M{ + "$setOnInsert": bson.M{ + "_id": sub.ID, + "parentMessageId": sub.ParentMessageID, + "roomId": sub.RoomID, + "threadRoomId": sub.ThreadRoomID, + "userId": sub.UserID, + "userAccount": sub.UserAccount, + "siteId": sub.SiteID, + "lastSeenAt": sub.LastSeenAt, + "createdAt": sub.CreatedAt, + }, + "$set": bson.M{"updatedAt": sub.UpdatedAt}, + "$max": bson.M{"hasMention": sub.HasMention}, + } + if _, err := s.threadSubCol.UpdateOne(ctx, filter, update, options.UpdateOne().SetUpsert(true)); err != nil { + return fmt.Errorf("upsert thread subscription (threadRoomID %q, userID %q): %w", + sub.ThreadRoomID, sub.UserID, err) + } + return nil +} +``` + +- [ ] **Step 11.3: Build and run unit tests** + +Run: `make build SERVICE=inbox-worker && make test SERVICE=inbox-worker` + +Expected: both pass. (The unit tests use the in-memory stub from Task 10; this task's Mongo code only matters in integration tests, added in Task 12.) + +- [ ] **Step 11.4: Commit** + +```bash +git add inbox-worker/main.go +git commit -m "inbox-worker: Mongo UpsertThreadSubscription with monotonic hasMention merge" +``` + +--- + + From d8e3f303d90f139448943a32c9623912d04999d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 05:54:20 +0000 Subject: [PATCH 08/32] =?UTF-8?q?docs:=20implementation=20plan=20chunk=205?= =?UTF-8?q?c=20(task=2012=20=E2=80=94=20inbox-worker=20integration=20tests?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add integration tests covering the insert path and the monotonic hasMention merge across two and three events through the real Mongo testcontainer, asserting _id/createdAt are pinned by \$setOnInsert and updatedAt advances. --- ...ssage-worker-thread-subscription-outbox.md | 150 +++++++++++++++++- 1 file changed, 149 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index 775343b99..a7a416f06 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -1966,4 +1966,152 @@ git commit -m "inbox-worker: Mongo UpsertThreadSubscription with monotonic hasMe --- - +## Task 12: Integration tests for inbox-worker `UpsertThreadSubscription` + +Two scenarios in real MongoDB via testcontainers: insert from empty, and monotonic-mention merge across two events. + +**Files:** +- Modify: `inbox-worker/integration_test.go` (append two test functions) + +- [ ] **Step 12.1: Append the integration tests** + +Add to `inbox-worker/integration_test.go`: + +```go +func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) { + db := setupMongo(t) + ctx := context.Background() + + store := &mongoInboxStore{ + subCol: db.Collection("subscriptions"), + roomCol: db.Collection("rooms"), + userCol: db.Collection("users"), + threadSubCol: db.Collection("threadSubscriptions"), + } + require.NoError(t, store.ensureIndexes(ctx)) + + pub := &recordingPublisher{} + handler := NewHandler(store, pub) + + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + sub := model.ThreadSubscription{ + ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + HasMention: false, CreatedAt: now, UpdatedAt: now, + } + subData, _ := json.Marshal(sub) + evtData, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: subData, Timestamp: now.UnixMilli(), + }) + + require.NoError(t, handler.HandleEvent(ctx, evtData)) + + var got model.ThreadSubscription + err := db.Collection("threadSubscriptions"). + FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). + Decode(&got) + require.NoError(t, err) + assert.Equal(t, "sub-1", got.ID) + assert.Equal(t, "site-b", got.SiteID) + assert.False(t, got.HasMention) + assert.True(t, got.CreatedAt.Equal(now)) + assert.True(t, got.UpdatedAt.Equal(now)) + + // No client publishes for thread subscription upserts. + assert.Empty(t, pub.subjects) +} + +func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t *testing.T) { + db := setupMongo(t) + ctx := context.Background() + + store := &mongoInboxStore{ + subCol: db.Collection("subscriptions"), + roomCol: db.Collection("rooms"), + userCol: db.Collection("users"), + threadSubCol: db.Collection("threadSubscriptions"), + } + require.NoError(t, store.ensureIndexes(ctx)) + + handler := NewHandler(store, &recordingPublisher{}) + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + + // First event: HasMention=true. + mentionSub := model.ThreadSubscription{ + ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + HasMention: true, CreatedAt: now, UpdatedAt: now, + } + mentionData, _ := json.Marshal(mentionSub) + mentionEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: mentionData, Timestamp: now.UnixMilli(), + }) + require.NoError(t, handler.HandleEvent(ctx, mentionEvt)) + + // Second event: HasMention=false (later updatedAt). Must NOT clear the flag. + plainSub := mentionSub + plainSub.HasMention = false + later := now.Add(time.Minute) + plainSub.UpdatedAt = later + plainData, _ := json.Marshal(plainSub) + plainEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: plainData, Timestamp: later.UnixMilli(), + }) + require.NoError(t, handler.HandleEvent(ctx, plainEvt)) + + var got model.ThreadSubscription + err := db.Collection("threadSubscriptions"). + FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). + Decode(&got) + require.NoError(t, err) + assert.True(t, got.HasMention, "hasMention must remain true after a non-mention event") + assert.True(t, got.UpdatedAt.Equal(later), "updatedAt must advance to the later event's value") + // _id and createdAt come from $setOnInsert and must remain from the first event. + assert.Equal(t, "sub-1", got.ID) + assert.True(t, got.CreatedAt.Equal(now)) + + // Third event: HasMention=true again. Idempotent — still true, updatedAt advances. + thirdSub := plainSub + thirdSub.HasMention = true + evenLater := later.Add(time.Minute) + thirdSub.UpdatedAt = evenLater + thirdData, _ := json.Marshal(thirdSub) + thirdEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: thirdData, Timestamp: evenLater.UnixMilli(), + }) + require.NoError(t, handler.HandleEvent(ctx, thirdEvt)) + + require.NoError(t, db.Collection("threadSubscriptions"). + FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). + Decode(&got)) + assert.True(t, got.HasMention) + assert.True(t, got.UpdatedAt.Equal(evenLater)) +} +``` + +- [ ] **Step 12.2: Run integration tests** + +Run: `make test-integration SERVICE=inbox-worker` + +Expected: both new tests pass alongside the existing ones. + +- [ ] **Step 12.3: Run the full integration suite to confirm no regressions** + +Run: `make test-integration` + +Expected: full integration suite passes (or skips on unavailable Docker — investigate if anything new fails). + +- [ ] **Step 12.4: Commit** + +```bash +git add inbox-worker/integration_test.go +git commit -m "inbox-worker: integration tests for UpsertThreadSubscription monotonic merge" +``` + +--- + + From af07716198d83901c74314abe3edc47d8130ac39 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 05:56:11 +0000 Subject: [PATCH 09/32] docs: implementation plan chunk 5d (self-review + handoff) Add spec coverage map and execution-handoff section. Clean up Task 5 step 5.3 to present lookupOwnerSiteID with its final (string, error) signature directly instead of showing an incorrect (string, bool) version first and then correcting it. --- ...ssage-worker-thread-subscription-outbox.md | 103 +++++++++--------- 1 file changed, 50 insertions(+), 53 deletions(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index a7a416f06..e8f4baa35 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -622,7 +622,7 @@ Replace lines 75-92 of `message-worker/handler.go` with: - [ ] **Step 5.3: Update `handleThreadRoomAndSubscriptions` and the two reply handlers** -Replace `handleThreadRoomAndSubscriptions`, `handleFirstThreadReply`, and `handleSubsequentThreadReply` in `message-worker/handler.go` with the versions below. The `replier *model.User` arg flows in; parent's siteID is fetched via `userStore.FindUserByID` with warn-and-skip on `userstore.ErrUserNotFound`. +Replace `handleThreadRoomAndSubscriptions`, `handleFirstThreadReply`, `handleSubsequentThreadReply`, and add `lookupOwnerSiteID` in `message-worker/handler.go` with the versions below. The `replier *model.User` arg flows in; the new `lookupOwnerSiteID` helper resolves the parent's home site via `userStore.FindUserByID` with warn-and-skip on `userstore.ErrUserNotFound` and propagation on other errors. ```go // handleThreadRoomAndSubscriptions creates the ThreadRoom on first reply and @@ -673,8 +673,11 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message return fmt.Errorf("get parent message sender: %w", err) } - parentSiteID, parentLookupOK := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") - if parentLookupOK { + parentSiteID, err := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") + if err != nil { + return fmt.Errorf("lookup parent owner site: %w", err) + } + if parentSiteID != "" { if err := h.threadStore.InsertThreadSubscription(ctx, h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, parentSiteID, now), ); err != nil { @@ -706,8 +709,11 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) switch { case err == nil: - parentSiteID, parentLookupOK := h.lookupOwnerSiteID(ctx, parentSender.ID, "subsequent-reply parent") - if parentLookupOK { + parentSiteID, lookupErr := h.lookupOwnerSiteID(ctx, parentSender.ID, "subsequent-reply parent") + if lookupErr != nil { + return "", fmt.Errorf("lookup parent owner site: %w", lookupErr) + } + if parentSiteID != "" { if err := h.threadStore.UpsertThreadSubscription(ctx, h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, parentSiteID, now), ); err != nil { @@ -743,39 +749,10 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me return existingRoom.ID, nil } -// lookupOwnerSiteID resolves a user's home site by ID. Returns (siteID, true) -// on success. On userstore.ErrUserNotFound, logs a warning and returns -// ("", false) so callers can skip that user gracefully (parallels the -// errMessageNotFound branch already in this file). Other errors propagate. -func (h *Handler) lookupOwnerSiteID(ctx context.Context, userID, role string) (string, bool) { - user, err := h.userStore.FindUserByID(ctx, userID) - if err != nil { - if errors.Is(err, userstore.ErrUserNotFound) { - slog.Warn("owner user not found — skipping thread subscription", - "userID", userID, "role", role) - return "", false - } - // Non-NotFound errors propagate via the caller — return ok=false but - // the next call to userStore from the caller will surface the error. - // For a clean API we return the lookup error to the caller instead. - // Match existing pattern: rewrap and panic-style would be wrong; instead - // the helper returns ok=false, and we rely on callers to handle the - // transient case. Simpler: change signature to also return error. - _ = err - return "", false - } - return user.SiteID, true -} -``` - -> **Note on `lookupOwnerSiteID` error semantics:** for transient DB errors we still return `("", false)`, which causes the call site to skip that user. That's wrong for a transient failure — we should propagate. Fix this by returning an error. - -Replace the `lookupOwnerSiteID` body with a version that returns `(string, error)`: - -```go -// lookupOwnerSiteID resolves a user's home site by ID. Returns -// ("", nil) when the user is not found (logs a warning) so callers can skip -// gracefully. Other DB errors are returned for the caller to NAK on. +// lookupOwnerSiteID resolves a user's home site by ID. +// Returns ("", nil) when the user is not found (logs a warning) so callers +// can skip that user gracefully — parallels the errMessageNotFound branch +// already in this file. Other DB errors are returned for the caller to NAK on. func (h *Handler) lookupOwnerSiteID(ctx context.Context, userID, role string) (string, error) { user, err := h.userStore.FindUserByID(ctx, userID) if err != nil { @@ -790,20 +767,6 @@ func (h *Handler) lookupOwnerSiteID(ctx context.Context, userID, role string) (s } ``` -Then update the two callers in `handleFirstThreadReply` and `handleSubsequentThreadReply` to: - -```go - parentSiteID, err := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") - if err != nil { - return fmt.Errorf("lookup parent owner site: %w", err) - } - if parentSiteID != "" { - // ... insert/upsert parent subscription ... - } -``` - -Use `return "", fmt.Errorf(...)` in `handleSubsequentThreadReply` (the (string, error) signature). - - [ ] **Step 5.4: Update existing `markThreadMentions` to use `Participant.SiteID`** Replace the existing `markThreadMentions` (around line 227-243 of `handler.go`) with: @@ -2114,4 +2077,38 @@ git commit -m "inbox-worker: integration tests for UpsertThreadSubscription mono --- - +## Spec coverage map + +| Spec section / requirement | Task(s) | +|---|---| +| `OutboxThreadSubscriptionUpserted` constant | Task 1 | +| `Participant.SiteID` field, propagation from `mention.Resolve` | Task 2 | +| `Handler.publish PublishFunc` + `siteID` field | Task 3 | +| `publishThreadSubOutboxIfRemote` helper, dedup ID seed, same-site no-op, empty-SiteID guard, error propagation | Task 4 | +| `ThreadSubscription.SiteID` = owner's site (not room's) | Task 5 | +| Parent siteID lookup via `userStore.FindUserByID`, warn-and-skip on `ErrUserNotFound`, propagate other errors | Tasks 5, 6, 7 | +| Outbox publish on first reply (parent + replier) | Task 6 | +| Outbox publish on subsequent reply (parent + replier) | Task 7 | +| Outbox publish on thread mention (mentionees only, with `HasMention=true` payload) | Task 8 | +| Skip @all and sender self-mention at thread level | Task 8 | +| `main.go` JetStream publish closure with `Nats-Msg-Id` | Tasks 3, 9 | +| `bootstrap.go` unchanged for OUTBOX (ops/IaC ownership) | Task 9 | +| Inbox-worker dispatch case for `thread_subscription_upserted` | Task 10 | +| `InboxStore.UpsertThreadSubscription` interface method | Task 10 | +| Mongo `$setOnInsert` + `$set` + `$max` upsert with monotonic `hasMention` | Task 11 | +| `(threadRoomId, userId)` unique index in inbox-worker | Task 11 | +| Integration tests: insert path + monotonic mention merge | Task 12 | +| Failure semantics: NAK on outbox publish error → JetStream redelivery → idempotent dedup | Tasks 4, 6, 7, 8 | +| Migration / rollout note (deploy inbox-worker before message-worker) | covered by spec; no code task needed | +| Non-goals: ThreadRoom replication, client UI events, delete events, @all propagation | none — explicitly excluded by spec | + +--- + +## Execution handoff + +Plan complete and saved to `docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md`. Two execution options: + +1. **Subagent-Driven (recommended)** — I dispatch a fresh subagent per task, review between tasks, fast iteration. +2. **Inline Execution** — Execute tasks in this session using `superpowers:executing-plans`, batch execution with checkpoints. + +Which approach? From 3106993ce3133b6ce5044cbf3afef29de4537222 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:09:09 +0000 Subject: [PATCH 10/32] model: add OutboxThreadSubscriptionUpserted event type constant --- pkg/model/event.go | 5 +++-- pkg/model/model_test.go | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/model/event.go b/pkg/model/event.go index db93ac062..33d6aa00b 100644 --- a/pkg/model/event.go +++ b/pkg/model/event.go @@ -79,8 +79,9 @@ type NotificationEvent struct { type OutboxEventType = string const ( - OutboxMemberAdded OutboxEventType = "member_added" - OutboxMemberRemoved OutboxEventType = "member_removed" + OutboxMemberAdded OutboxEventType = "member_added" + OutboxMemberRemoved OutboxEventType = "member_removed" + OutboxThreadSubscriptionUpserted OutboxEventType = "thread_subscription_upserted" ) type OutboxEvent struct { diff --git a/pkg/model/model_test.go b/pkg/model/model_test.go index cc1d5a994..a6662d5e8 100644 --- a/pkg/model/model_test.go +++ b/pkg/model/model_test.go @@ -732,6 +732,27 @@ func TestOutboxEventJSON(t *testing.T) { } } +func TestOutboxEventJSON_ThreadSubscriptionUpserted(t *testing.T) { + src := model.OutboxEvent{ + Type: model.OutboxThreadSubscriptionUpserted, + SiteID: "site-a", + DestSiteID: "site-b", + Payload: []byte(`{"id":"sub-1","threadRoomId":"tr-1"}`), + Timestamp: 1735689600000, + } + data, err := json.Marshal(&src) + require.NoError(t, err) + + var dst model.OutboxEvent + require.NoError(t, json.Unmarshal(data, &dst)) + if !reflect.DeepEqual(src, dst) { + t.Errorf("round-trip mismatch:\n got %+v\n want %+v", dst, src) + } + if dst.Type != "thread_subscription_upserted" { + t.Errorf("Type = %q, want thread_subscription_upserted", dst.Type) + } +} + func TestInboxMemberEventJSON(t *testing.T) { t.Run("add event, unrestricted room", func(t *testing.T) { src := model.InboxMemberEvent{ From 644ee8175c92c8836442460005df52fc38744fc9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:18:36 +0000 Subject: [PATCH 11/32] model+mention: add SiteID to Participant, propagate from Resolve - Add SiteID field to model.Participant struct with omitempty JSON/BSON tags - Extend TestParticipantJSON to verify SiteID round-trips and omits when empty - Propagate SiteID from User to Participant in mention.Resolve - Update mention tests to include SiteID in test fixtures and expectations - Update message-worker test fixture to expect SiteID in resolved Participant Downstream code can now determine each mentioned user's home site via Participant.SiteID, enabling cross-site outbox events for thread subscriptions. https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- message-worker/handler_test.go | 2 +- pkg/mention/mention.go | 1 + pkg/mention/mention_test.go | 14 +++++++------- pkg/model/event.go | 1 + pkg/model/model_test.go | 25 +++++++++++++++++++++++++ 5 files changed, 35 insertions(+), 8 deletions(-) diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 1ee3e29dc..41b77fa84 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -130,7 +130,7 @@ func TestHandler_ProcessMessage(t *testing.T) { Content: "hey @bob can you check this?", CreatedAt: now, Mentions: []model.Participant{{ - UserID: "u-bob", Account: "bob", ChineseName: "鮑勃", EngName: "Bob Chen", + UserID: "u-bob", Account: "bob", SiteID: "site-a", ChineseName: "鮑勃", EngName: "Bob Chen", }}, } diff --git a/pkg/mention/mention.go b/pkg/mention/mention.go index 2ec7cf13b..66ad488e4 100644 --- a/pkg/mention/mention.go +++ b/pkg/mention/mention.go @@ -78,6 +78,7 @@ func Resolve(ctx context.Context, content string, lookupFn LookupFunc) (*Resolve result.Participants = append(result.Participants, model.Participant{ UserID: users[i].ID, Account: users[i].Account, + SiteID: users[i].SiteID, ChineseName: users[i].ChineseName, EngName: users[i].EngName, }) diff --git a/pkg/mention/mention_test.go b/pkg/mention/mention_test.go index 87dcb3601..74fd13588 100644 --- a/pkg/mention/mention_test.go +++ b/pkg/mention/mention_test.go @@ -60,8 +60,8 @@ func TestParse(t *testing.T) { } func TestResolve(t *testing.T) { - bobUser := model.User{ID: "u-bob", Account: "bob", EngName: "Bob Chen", ChineseName: "鮑勃"} - aliceUser := model.User{ID: "u-alice", Account: "alice", EngName: "Alice Wang", ChineseName: "愛麗絲"} + bobUser := model.User{ID: "u-bob", Account: "bob", SiteID: "site-b", EngName: "Bob Chen", ChineseName: "鮑勃"} + aliceUser := model.User{ID: "u-alice", Account: "alice", SiteID: "site-a", EngName: "Alice Wang", ChineseName: "愛麗絲"} tests := []struct { name string @@ -83,7 +83,7 @@ func TestResolve(t *testing.T) { lookupUsers: []model.User{bobUser}, wantAccounts: []string{"bob"}, wantParts: []model.Participant{ - {UserID: "u-bob", Account: "bob", EngName: "Bob Chen", ChineseName: "鮑勃"}, + {UserID: "u-bob", Account: "bob", SiteID: "site-b", EngName: "Bob Chen", ChineseName: "鮑勃"}, }, }, { @@ -92,8 +92,8 @@ func TestResolve(t *testing.T) { lookupUsers: []model.User{aliceUser, bobUser}, wantAccounts: []string{"alice", "bob"}, wantParts: []model.Participant{ - {UserID: "u-alice", Account: "alice", EngName: "Alice Wang", ChineseName: "愛麗絲"}, - {UserID: "u-bob", Account: "bob", EngName: "Bob Chen", ChineseName: "鮑勃"}, + {UserID: "u-alice", Account: "alice", SiteID: "site-a", EngName: "Alice Wang", ChineseName: "愛麗絲"}, + {UserID: "u-bob", Account: "bob", SiteID: "site-b", EngName: "Bob Chen", ChineseName: "鮑勃"}, }, }, { @@ -111,7 +111,7 @@ func TestResolve(t *testing.T) { wantAccounts: []string{"bob"}, wantMentionAll: true, wantParts: []model.Participant{ - {UserID: "u-bob", Account: "bob", EngName: "Bob Chen", ChineseName: "鮑勃"}, + {UserID: "u-bob", Account: "bob", SiteID: "site-b", EngName: "Bob Chen", ChineseName: "鮑勃"}, {Account: "all", EngName: "all"}, }, }, @@ -121,7 +121,7 @@ func TestResolve(t *testing.T) { lookupUsers: []model.User{aliceUser}, wantAccounts: []string{"alice", "unknown"}, wantParts: []model.Participant{ - {UserID: "u-alice", Account: "alice", EngName: "Alice Wang", ChineseName: "愛麗絲"}, + {UserID: "u-alice", Account: "alice", SiteID: "site-a", EngName: "Alice Wang", ChineseName: "愛麗絲"}, }, }, { diff --git a/pkg/model/event.go b/pkg/model/event.go index 33d6aa00b..e5e978d46 100644 --- a/pkg/model/event.go +++ b/pkg/model/event.go @@ -106,6 +106,7 @@ type MemberAddEvent struct { type Participant struct { UserID string `json:"userId,omitempty" bson:"userId,omitempty"` Account string `json:"account" bson:"account"` + SiteID string `json:"siteId,omitempty" bson:"siteId,omitempty"` ChineseName string `json:"chineseName" bson:"chineseName"` EngName string `json:"engName" bson:"engName"` } diff --git a/pkg/model/model_test.go b/pkg/model/model_test.go index a6662d5e8..db03d3099 100644 --- a/pkg/model/model_test.go +++ b/pkg/model/model_test.go @@ -617,6 +617,31 @@ func TestParticipantJSON(t *testing.T) { require.NoError(t, json.Unmarshal(data, &dst)) assert.Equal(t, p, dst) }) + + t.Run("with siteID round-trips", func(t *testing.T) { + p := model.Participant{ + UserID: "u1", + Account: "alice", + SiteID: "site-a", + ChineseName: "愛麗絲", + EngName: "Alice Wang", + } + roundTrip(t, &p, &model.Participant{}) + }) + + t.Run("siteID omitted when empty", func(t *testing.T) { + p := model.Participant{ + UserID: "u1", + Account: "alice", + EngName: "Alice Wang", + } + data, err := json.Marshal(p) + require.NoError(t, err) + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + _, hasSiteID := raw["siteId"] + assert.False(t, hasSiteID, "siteId should be omitted when empty") + }) } func TestClientMessageJSON(t *testing.T) { From 35375074fe26530500a429a9fbcaa245d611c294 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:24:28 +0000 Subject: [PATCH 12/32] message-worker: plumb siteID and PublishFunc into Handler https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- message-worker/handler.go | 16 ++++++++++++++-- message-worker/handler_test.go | 12 +++++++++--- message-worker/integration_test.go | 12 +++++++++--- message-worker/main.go | 8 +++++++- 4 files changed, 39 insertions(+), 9 deletions(-) diff --git a/message-worker/handler.go b/message-worker/handler.go index 4fb6167fe..92c306c2c 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -16,14 +16,26 @@ import ( "github.com/hmchangw/chat/pkg/userstore" ) +// PublishFunc publishes data; non-empty msgID sets Nats-Msg-Id for JetStream stream-level dedup. +// Mirrors room-worker's PublishFunc signature so message-worker can plug into the same publish closure. +type PublishFunc func(ctx context.Context, subj string, data []byte, msgID string) error + type Handler struct { store Store userStore userstore.UserStore threadStore ThreadStore + siteID string + publish PublishFunc } -func NewHandler(store Store, userStore userstore.UserStore, threadStore ThreadStore) *Handler { - return &Handler{store: store, userStore: userStore, threadStore: threadStore} +func NewHandler(store Store, userStore userstore.UserStore, threadStore ThreadStore, siteID string, publish PublishFunc) *Handler { + return &Handler{ + store: store, + userStore: userStore, + threadStore: threadStore, + siteID: siteID, + publish: publish, + } } func (h *Handler) HandleJetStreamMsg(ctx context.Context, msg jetstream.Msg) { diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 41b77fa84..7c7ec8c56 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -415,7 +415,9 @@ func TestHandler_ProcessMessage(t *testing.T) { mockThreadStore := NewMockThreadStore(ctrl) tt.setupMocks(mockStore, mockUserStore, mockThreadStore) - h := NewHandler(mockStore, mockUserStore, mockThreadStore) + h := NewHandler(mockStore, mockUserStore, mockThreadStore, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return nil + }) err := h.processMessage(context.Background(), tt.data) if tt.wantErr { require.Error(t, err) @@ -863,7 +865,9 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { mockUserStore := NewMockUserStore(ctrl) tt.setupMocks(mockStore, mockThreadStore) - h := NewHandler(mockStore, mockUserStore, mockThreadStore) + h := NewHandler(mockStore, mockUserStore, mockThreadStore, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return nil + }) _, err := h.handleThreadRoomAndSubscriptions(context.Background(), tt.msg, tt.siteID) if tt.wantErr { require.Error(t, err) @@ -947,7 +951,9 @@ func TestHandler_HandleJetStreamMsg(t *testing.T) { mockThreadStore := NewMockThreadStore(ctrl) tt.setupMocks(mockStore, mockUserStore, mockThreadStore) - h := NewHandler(mockStore, mockUserStore, mockThreadStore) + h := NewHandler(mockStore, mockUserStore, mockThreadStore, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return nil + }) fakeMsg := &fakeJSMsg{data: tt.msgData} h.HandleJetStreamMsg(context.Background(), fakeMsg) diff --git a/message-worker/integration_test.go b/message-worker/integration_test.go index f027b1950..d2f26232c 100644 --- a/message-worker/integration_test.go +++ b/message-worker/integration_test.go @@ -349,7 +349,9 @@ func TestHandler_Integration(t *testing.T) { store := NewCassandraStore(cassSession) us := userstore.NewMongoStore(userCol) threadStore := newThreadStoreMongo(mongoDB) - h := NewHandler(store, us, threadStore) + h := NewHandler(store, us, threadStore, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return nil + }) now := time.Now().UTC().Truncate(time.Millisecond) evt := model.MessageEvent{ @@ -411,7 +413,9 @@ func TestHandler_Integration_ThreadReply(t *testing.T) { us := userstore.NewMongoStore(userCol) ts := newThreadStoreMongo(db) require.NoError(t, ts.EnsureIndexes(ctx)) - h := NewHandler(store, us, ts) + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return nil + }) now := time.Now().UTC().Truncate(time.Millisecond) @@ -533,7 +537,9 @@ func TestHandler_Integration_ThreadReplyWithMention(t *testing.T) { us := userstore.NewMongoStore(userCol) ts := newThreadStoreMongo(db) require.NoError(t, ts.EnsureIndexes(ctx)) - h := NewHandler(store, us, ts) + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return nil + }) now := time.Now().UTC().Truncate(time.Millisecond) diff --git a/message-worker/main.go b/message-worker/main.go index 48b49fbca..50d4c627e 100644 --- a/message-worker/main.go +++ b/message-worker/main.go @@ -92,7 +92,13 @@ func main() { slog.Error("ensure thread store indexes failed", "error", err) os.Exit(1) } - handler := NewHandler(store, us, threadStore) + handler := NewHandler(store, us, threadStore, cfg.SiteID, func(ctx context.Context, subj string, data []byte, msgID string) error { + if msgID == "" { + return nc.Publish(ctx, subj, data) + } + _, err := js.Publish(ctx, subj, data, jetstream.WithMsgID(msgID)) + return err + }) if err := bootstrapStreams(ctx, js, cfg.SiteID, cfg.Bootstrap.Enabled); err != nil { slog.Error("bootstrap streams failed", "error", err) From 244aa1e28786a495778ec17a033eb30cb11e5c0b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:28:27 +0000 Subject: [PATCH 13/32] message-worker: add publishThreadSubOutboxIfRemote helper https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- message-worker/handler.go | 42 ++++++++++++ message-worker/handler_test.go | 114 +++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+) diff --git a/message-worker/handler.go b/message-worker/handler.go index 92c306c2c..75be02869 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -13,6 +13,7 @@ import ( "github.com/hmchangw/chat/pkg/idgen" "github.com/hmchangw/chat/pkg/mention" "github.com/hmchangw/chat/pkg/model" + "github.com/hmchangw/chat/pkg/subject" "github.com/hmchangw/chat/pkg/userstore" ) @@ -262,3 +263,44 @@ func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, th } return nil } + +// publishThreadSubOutboxIfRemote publishes a thread_subscription_upserted +// outbox event when sub.SiteID is a remote site. Same-site or empty SiteID +// is a no-op (empty SiteID logs a warning — it indicates a caller bug). +// +// The dedup-ID seed is (threadRoomID, userID, msgID): msg.ID is unique per +// reply, and (msg.ID, userID) is unique within a reply, so the seed is stable +// across MESSAGES_CANONICAL redeliveries and JetStream stream-level dedup +// absorbs duplicates within the dedup window. +func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model.ThreadSubscription, msgID string) error { + if sub.SiteID == "" { + slog.Warn("thread subscription has empty SiteID, skipping outbox publish", + "threadRoomID", sub.ThreadRoomID, "userID", sub.UserID, "msgID", msgID) + return nil + } + if sub.SiteID == h.siteID { + return nil + } + + payload, err := json.Marshal(sub) + if err != nil { + return fmt.Errorf("marshal thread subscription: %w", err) + } + outbox := model.OutboxEvent{ + Type: model.OutboxThreadSubscriptionUpserted, + SiteID: h.siteID, + DestSiteID: sub.SiteID, + Payload: payload, + Timestamp: time.Now().UTC().UnixMilli(), + } + data, err := json.Marshal(outbox) + if err != nil { + return fmt.Errorf("marshal outbox event: %w", err) + } + dedupID := idgen.DeriveID(fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID)) + subj := subject.Outbox(h.siteID, sub.SiteID, model.OutboxThreadSubscriptionUpserted) + if err := h.publish(ctx, subj, data, dedupID); err != nil { + return fmt.Errorf("publish thread subscription outbox to %s: %w", sub.SiteID, err) + } + return nil +} diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 7c7ec8c56..558517cc4 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -878,6 +878,120 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { } } +func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + baseSub := &model.ThreadSubscription{ + ID: "sub-1", + ParentMessageID: "pm-1", + RoomID: "r1", + ThreadRoomID: "tr-1", + UserID: "u-bob", + UserAccount: "bob", + SiteID: "site-b", + CreatedAt: now, + UpdatedAt: now, + } + + t.Run("same site — no publish", func(t *testing.T) { + ctrl := gomock.NewController(t) + var called bool + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-b", + func(_ context.Context, _ string, _ []byte, _ string) error { + called = true + return nil + }) + + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + require.NoError(t, err) + assert.False(t, called, "publish must not be called when sub.SiteID == h.siteID") + }) + + t.Run("empty siteID — skip with warn, no publish", func(t *testing.T) { + ctrl := gomock.NewController(t) + var called bool + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, _ string, _ []byte, _ string) error { + called = true + return nil + }) + + emptySub := *baseSub + emptySub.SiteID = "" + err := h.publishThreadSubOutboxIfRemote(context.Background(), &emptySub, "msg-1") + require.NoError(t, err) + assert.False(t, called, "publish must not be called when sub.SiteID is empty") + }) + + t.Run("remote site — publishes with expected subject and dedup ID", func(t *testing.T) { + ctrl := gomock.NewController(t) + var captured struct { + subj string + data []byte + msgID string + callCnt int + } + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, subj string, data []byte, msgID string) error { + captured.subj = subj + captured.data = data + captured.msgID = msgID + captured.callCnt++ + return nil + }) + + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + require.NoError(t, err) + require.Equal(t, 1, captured.callCnt) + assert.Equal(t, "outbox.site-a.to.site-b.thread_subscription_upserted", captured.subj) + assert.NotEmpty(t, captured.msgID, "dedup ID must be set") + + // Same inputs → same dedup ID (stable across redeliveries). + var second string + h2 := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, _ string, _ []byte, msgID string) error { + second = msgID + return nil + }) + require.NoError(t, h2.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1")) + assert.Equal(t, captured.msgID, second, "dedup ID must be deterministic for the same (threadRoomID, userID, msgID) seed") + + // Different msgID → different dedup ID. + var third string + h3 := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, _ string, _ []byte, msgID string) error { + third = msgID + return nil + }) + require.NoError(t, h3.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-2")) + assert.NotEqual(t, captured.msgID, third) + + // Payload is an OutboxEvent whose inner Payload decodes back to the ThreadSubscription. + var outer model.OutboxEvent + require.NoError(t, json.Unmarshal(captured.data, &outer)) + assert.Equal(t, model.OutboxThreadSubscriptionUpserted, outer.Type) + assert.Equal(t, "site-a", outer.SiteID) + assert.Equal(t, "site-b", outer.DestSiteID) + assert.Greater(t, outer.Timestamp, int64(0)) + + var inner model.ThreadSubscription + require.NoError(t, json.Unmarshal(outer.Payload, &inner)) + assert.Equal(t, *baseSub, inner) + }) + + t.Run("publish error returned", func(t *testing.T) { + ctrl := gomock.NewController(t) + boom := errors.New("publish boom") + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", + func(_ context.Context, _ string, _ []byte, _ string) error { + return boom + }) + + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + require.Error(t, err) + assert.ErrorIs(t, err, boom) + }) +} + // fakeJSMsg is a minimal jetstream.Msg test double that records whether Ack or // Nak was called so tests can assert on ack/nak behaviour. type fakeJSMsg struct { From 71db87d78ad010dc243b03dc9989733e067a5718 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:38:50 +0000 Subject: [PATCH 14/32] message-worker: ThreadSubscription.SiteID = owner site; lookup parent siteID - buildThreadSubscription now takes ownerSiteID (owner's home site, not room's site) so subscriptions round-trip correctly through OUTBOX/INBOX - handleThreadRoomAndSubscriptions/handleFirstThreadReply/handleSubsequent- ThreadReply updated to accept replier *model.User and use replier.SiteID - lookupOwnerSiteID helper resolves parent's home site via userStore.FindUserByID; returns ("", nil) on ErrUserNotFound (warn-and-skip) so processing continues - markThreadMentions uses Participant.SiteID for mentionee subscriptions - processMessage passes user to handleThreadRoomAndSubscriptions - Tests: added FindUserByID parent mock expectations to all thread test cases; extended struct with extraUserStoreSetup/expectReplierInsert; added warn-and-skip and DB-error edge-case tests for parent user lookup https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- message-worker/handler.go | 141 +++++++++++++++++++++++++-------- message-worker/handler_test.go | 117 +++++++++++++++++++++++++-- 2 files changed, 218 insertions(+), 40 deletions(-) diff --git a/message-worker/handler.go b/message-worker/handler.go index 75be02869..9952b6ef5 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -13,6 +13,7 @@ import ( "github.com/hmchangw/chat/pkg/idgen" "github.com/hmchangw/chat/pkg/mention" "github.com/hmchangw/chat/pkg/model" + "github.com/hmchangw/chat/pkg/natsutil" "github.com/hmchangw/chat/pkg/subject" "github.com/hmchangw/chat/pkg/userstore" ) @@ -87,7 +88,7 @@ func (h *Handler) processMessage(ctx context.Context, data []byte) error { if evt.Message.ThreadParentMessageID != "" { // Resolve (or create) the thread room first so we have the threadRoomID // before persisting the message to Cassandra. - threadRoomID, err := h.handleThreadRoomAndSubscriptions(ctx, &evt.Message, evt.SiteID) + threadRoomID, err := h.handleThreadRoomAndSubscriptions(ctx, &evt.Message, evt.SiteID, user) if err != nil { return fmt.Errorf("handle thread room and subscriptions: %w", err) } @@ -106,7 +107,14 @@ func (h *Handler) processMessage(ctx context.Context, data []byte) error { return nil } -func (h *Handler) handleThreadRoomAndSubscriptions(ctx context.Context, msg *model.Message, siteID string) (string, error) { +// handleThreadRoomAndSubscriptions creates the ThreadRoom on first reply and +// inserts ThreadSubscriptions for the parent author and replier. On subsequent +// replies it upserts both subscriptions and bumps the room's last-message pointer. +// It returns the threadRoomID so the caller can pass it to SaveThreadMessage. +// +// `replier` may be nil for system messages with no real user (rare in thread +// paths); subscriptions for the replier are skipped in that case. +func (h *Handler) handleThreadRoomAndSubscriptions(ctx context.Context, msg *model.Message, eventSiteID string, replier *model.User) (string, error) { now := msg.CreatedAt var parentCreatedAt time.Time @@ -118,7 +126,7 @@ func (h *Handler) handleThreadRoomAndSubscriptions(ctx context.Context, msg *mod ParentMessageID: msg.ThreadParentMessageID, ThreadParentCreatedAt: parentCreatedAt, RoomID: msg.RoomID, - SiteID: siteID, + SiteID: eventSiteID, LastMsgAt: now, LastMsgID: msg.ID, ReplyAccounts: []string{msg.UserAccount}, @@ -129,15 +137,19 @@ func (h *Handler) handleThreadRoomAndSubscriptions(ctx context.Context, msg *mod err := h.threadStore.CreateThreadRoom(ctx, &threadRoom) switch { case err == nil: - return threadRoom.ID, h.handleFirstThreadReply(ctx, msg, siteID, threadRoom.ID, now) + return threadRoom.ID, h.handleFirstThreadReply(ctx, msg, threadRoom.ID, replier, now) case errors.Is(err, errThreadRoomExists): - return h.handleSubsequentThreadReply(ctx, msg, siteID, now) + return h.handleSubsequentThreadReply(ctx, msg, replier, now) default: return "", fmt.Errorf("create thread room: %w", err) } } -func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message, siteID, threadRoomID string, now time.Time) error { +// handleFirstThreadReply runs after the thread room has just been created. +// It inserts subscriptions for the parent author (looked up via userStore for +// the parent's home site) and, if distinct, for the replier (using the +// replier's home site). +func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message, threadRoomID string, replier *model.User, now time.Time) error { parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) if err != nil { if errors.Is(err, errMessageNotFound) { @@ -149,15 +161,21 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message return fmt.Errorf("get parent message sender: %w", err) } - if err := h.threadStore.InsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, siteID, now), - ); err != nil { - return fmt.Errorf("insert parent author thread subscription: %w", err) + parentSiteID, err := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") + if err != nil { + return fmt.Errorf("lookup parent owner site: %w", err) + } + if parentSiteID != "" { + if err := h.threadStore.InsertThreadSubscription(ctx, + h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, parentSiteID, now), + ); err != nil { + return fmt.Errorf("insert parent author thread subscription: %w", err) + } } - if msg.UserID != parentSender.ID { + if replier != nil && msg.UserID != parentSender.ID { if err := h.threadStore.InsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, threadRoomID, msg.UserID, msg.UserAccount, siteID, now), + h.buildThreadSubscription(msg, threadRoomID, msg.UserID, msg.UserAccount, replier.SiteID, now), ); err != nil { return fmt.Errorf("insert replier thread subscription: %w", err) } @@ -174,9 +192,11 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message return nil } -// Upserts are idempotent — redeliveries after a partial first attempt never lose -// the parent subscription. -func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Message, siteID string, now time.Time) (string, error) { +// handleSubsequentThreadReply runs when CreateThreadRoom reported an existing room. +// Upserts subscriptions for both the parent author and the replier (idempotent +// on redelivery), then bumps the room's last-message pointer. Returns the +// existing thread room ID so the caller can pass it to SaveThreadMessage. +func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Message, replier *model.User, now time.Time) (string, error) { existingRoom, err := h.threadStore.GetThreadRoomByParentMessageID(ctx, msg.ThreadParentMessageID) if err != nil { return "", fmt.Errorf("get existing thread room: %w", err) @@ -186,14 +206,20 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) switch { case err == nil: - if err := h.threadStore.UpsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, siteID, now), - ); err != nil { - return "", fmt.Errorf("upsert parent author thread subscription: %w", err) + parentSiteID, lookupErr := h.lookupOwnerSiteID(ctx, parentSender.ID, "subsequent-reply parent") + if lookupErr != nil { + return "", fmt.Errorf("lookup parent owner site: %w", lookupErr) } - if msg.UserID != parentSender.ID { + if parentSiteID != "" { if err := h.threadStore.UpsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, siteID, now), + h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, parentSiteID, now), + ); err != nil { + return "", fmt.Errorf("upsert parent author thread subscription: %w", err) + } + } + if replier != nil && msg.UserID != parentSender.ID { + if err := h.threadStore.UpsertThreadSubscription(ctx, + h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now), ); err != nil { return "", fmt.Errorf("upsert replier thread subscription: %w", err) } @@ -203,10 +229,12 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me slog.Warn("thread reply parent not found — skipping parent subscription upsert", "parentMessageID", msg.ThreadParentMessageID, "replyID", msg.ID) - if err := h.threadStore.UpsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, siteID, now), - ); err != nil { - return "", fmt.Errorf("upsert replier thread subscription: %w", err) + if replier != nil { + if err := h.threadStore.UpsertThreadSubscription(ctx, + h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now), + ); err != nil { + return "", fmt.Errorf("upsert replier thread subscription: %w", err) + } } default: return "", fmt.Errorf("get parent message sender: %w", err) @@ -227,9 +255,28 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me return existingRoom.ID, nil } -// lastSeenAt is always nil — subscriptions are insert-only; only the read path -// (client marking the thread as seen) ever sets this field. -func (h *Handler) buildThreadSubscription(msg *model.Message, threadRoomID, userID, userAccount, siteID string, now time.Time) *model.ThreadSubscription { +// lookupOwnerSiteID resolves a user's home site by ID. +// Returns ("", nil) when the user is not found (logs a warning) so callers +// can skip that user gracefully — parallels the errMessageNotFound branch +// already in this file. Other DB errors are returned for the caller to NAK on. +func (h *Handler) lookupOwnerSiteID(ctx context.Context, userID, role string) (string, error) { + user, err := h.userStore.FindUserByID(ctx, userID) + if err != nil { + if errors.Is(err, userstore.ErrUserNotFound) { + slog.Warn("owner user not found — skipping thread subscription", + "userID", userID, "role", role) + return "", nil + } + return "", fmt.Errorf("lookup user %s: %w", userID, err) + } + return user.SiteID, nil +} + +// buildThreadSubscription constructs a ThreadSubscription for (threadRoomID, userID). +// ownerSiteID is the home site of the subscription's owner — NOT the room's site — +// so the document round-trips correctly through the OUTBOX/INBOX federation. +// lastSeenAt is always nil; the field is owned by user-action paths, not message-worker. +func (h *Handler) buildThreadSubscription(msg *model.Message, threadRoomID, userID, userAccount, ownerSiteID string, now time.Time) *model.ThreadSubscription { return &model.ThreadSubscription{ ID: idgen.GenerateUUIDv7(), ParentMessageID: msg.ThreadParentMessageID, @@ -237,16 +284,18 @@ func (h *Handler) buildThreadSubscription(msg *model.Message, threadRoomID, user ThreadRoomID: threadRoomID, UserID: userID, UserAccount: userAccount, - SiteID: siteID, + SiteID: ownerSiteID, LastSeenAt: nil, CreatedAt: now, UpdatedAt: now, } } -// Sender and @all are excluded: sender already has a subscription; @all is a -// broadcast that shouldn't trigger per-user mention state in threads. -func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, threadRoomID, siteID string) error { +// markThreadMentions flips hasMention=true on the thread subscription of every +// @account mentionee in msg (auto-creating the subscription if absent). The +// sender is excluded, and @all is ignored at the thread level. The +// subscription's SiteID is the mentionee's home site (carried on Participant). +func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, threadRoomID, eventSiteID string) error { for i := range msg.Mentions { p := &msg.Mentions[i] if p.Account == "all" { @@ -255,7 +304,16 @@ func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, th if p.UserID == msg.UserID { continue } - sub := h.buildThreadSubscription(msg, threadRoomID, p.UserID, p.Account, siteID, msg.CreatedAt) + ownerSiteID := p.SiteID + if ownerSiteID == "" { + // Defensive: should not happen since mention.Resolve populates SiteID + // for resolved users. Fall back to event site so the local mark still + // happens; outbox publish (Task 8) will skip on empty SiteID anyway. + slog.Warn("mentionee participant has empty SiteID, falling back to event site", + "account", p.Account, "userID", p.UserID, "msgID", msg.ID) + ownerSiteID = eventSiteID + } + sub := h.buildThreadSubscription(msg, threadRoomID, p.UserID, p.Account, ownerSiteID, msg.CreatedAt) sub.HasMention = true if err := h.threadStore.MarkThreadSubscriptionMention(ctx, sub); err != nil { return fmt.Errorf("mark thread subscription mention for user %s: %w", p.UserID, err) @@ -297,10 +355,25 @@ func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model if err != nil { return fmt.Errorf("marshal outbox event: %w", err) } - dedupID := idgen.DeriveID(fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID)) + payloadSeed := fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID) + dedupID := outboxDedupID(ctx, sub.SiteID, payloadSeed) subj := subject.Outbox(h.siteID, sub.SiteID, model.OutboxThreadSubscriptionUpserted) if err := h.publish(ctx, subj, data, dedupID); err != nil { return fmt.Errorf("publish thread subscription outbox to %s: %w", sub.SiteID, err) } return nil } + +// outboxDedupID composes Nats-Msg-Id as base+":"+destSiteID; base is X-Request-ID +// from ctx, falling back to payloadSeed when absent (partial-deployment safety). +// Mirrors room-worker's helper of the same shape so cross-site dedup IDs are +// consistent across services that publish to OUTBOX. +func outboxDedupID(ctx context.Context, destSiteID, payloadSeed string) string { + base := natsutil.RequestIDFromContext(ctx) + if base == "" { + slog.Warn("missing X-Request-ID; falling back to payload-derived outbox dedup base", + "destSiteID", destSiteID) + base = payloadSeed + } + return base + ":" + destSiteID +} diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 558517cc4..7decdefa0 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -16,6 +16,7 @@ import ( "github.com/hmchangw/chat/pkg/model" "github.com/hmchangw/chat/pkg/model/cassandra" + "github.com/hmchangw/chat/pkg/userstore" ) func ptrTime(t time.Time) *time.Time { return &t } @@ -219,6 +220,8 @@ func TestHandler_ProcessMessage(t *testing.T) { // Subsequent-reply path: upsert parent and replier subscriptions. store.EXPECT().GetMessageSender(gomock.Any(), "msg-1"). Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-1", "msg-2", "alice", now).Return(nil) @@ -237,6 +240,8 @@ func TestHandler_ProcessMessage(t *testing.T) { Return(&model.ThreadRoom{ID: "tr-1"}, nil) store.EXPECT().GetMessageSender(gomock.Any(), "msg-1"). Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-1", "msg-2", "alice", now).Return(nil) @@ -316,6 +321,8 @@ func TestHandler_ProcessMessage(t *testing.T) { ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) store.EXPECT().GetMessageSender(gomock.Any(), "msg-1"). Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) // Parent + replier subscriptions inserted. ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) @@ -345,6 +352,8 @@ func TestHandler_ProcessMessage(t *testing.T) { ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) store.EXPECT().GetMessageSender(gomock.Any(), "msg-1"). Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) // MarkThreadSubscriptionMention must NOT be called — sender excluded. @@ -360,6 +369,8 @@ func TestHandler_ProcessMessage(t *testing.T) { ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) store.EXPECT().GetMessageSender(gomock.Any(), "msg-1"). Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) // MarkThreadSubscriptionMention must NOT be called — @all is thread-ignored. @@ -376,6 +387,8 @@ func TestHandler_ProcessMessage(t *testing.T) { ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) store.EXPECT().GetMessageSender(gomock.Any(), "msg-1"). Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().MarkThreadSubscriptionMention(gomock.Any(), gomock.Any()). @@ -397,6 +410,8 @@ func TestHandler_ProcessMessage(t *testing.T) { ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) store.EXPECT().GetMessageSender(gomock.Any(), "msg-1"). Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().MarkThreadSubscriptionMention(gomock.Any(), gomock.Any()). @@ -447,11 +462,13 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { } tests := []struct { - name string - msg *model.Message - siteID string - setupMocks func(store *MockStore, ts *MockThreadStore) - wantErr bool + name string + msg *model.Message + siteID string + setupMocks func(store *MockStore, ts *MockThreadStore) + extraUserStoreSetup func(us *MockUserStore) + expectReplierInsert bool + wantErr bool }{ { name: "first reply — different users — creates room and two subscriptions", @@ -484,6 +501,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { return nil }) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, }, { name: "first reply — parent message not found — ack-and-skip", @@ -518,6 +539,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { return nil }) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, }, { name: "first reply — GetMessageSender fails — returns error", @@ -541,6 +566,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()). Return(errors.New("mongo: write error")) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, wantErr: true, }, { @@ -557,6 +586,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()). Return(errors.New("mongo: write error")) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, wantErr: true, }, { @@ -587,6 +620,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", "replier", now). Return(nil) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, }, { name: "subsequent reply — same user as parent — upserts one subscription and updates last message", @@ -615,6 +652,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", "parent-user", now). Return(nil) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, }, { name: "subsequent reply — parent message not found — skips parent upsert and upserts replier", @@ -676,6 +717,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()). Return(errors.New("mongo: write error")) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, wantErr: true, }, { @@ -693,6 +738,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()). Return(errors.New("mongo: write error")) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, wantErr: true, }, { @@ -711,6 +760,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", "replier", now). Return(errors.New("mongo: write error")) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, wantErr: true, }, { @@ -752,6 +805,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { gomock.Cond(func(x any) bool { s, ok := x.(string); return ok && s == capturedRoomID }), ).Return(nil) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, }, { name: "first reply — UpdateParentMessageThreadRoomID fails — returns error", @@ -774,6 +831,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { store.EXPECT().UpdateParentMessageThreadRoomID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return(errors.New("cassandra: write timeout")) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, wantErr: true, }, { @@ -802,6 +863,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { "tr-existing", ).Return(nil) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, }, { name: "subsequent reply — UpdateParentMessageThreadRoomID fails — returns error", @@ -826,6 +891,10 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { store.EXPECT().UpdateParentMessageThreadRoomID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return(errors.New("cassandra: write timeout")) }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + }, wantErr: true, }, { @@ -853,8 +922,37 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { }) ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", "replier", now).Return(nil) // UpdateParentMessageThreadRoomID must NOT be called — parent doesn't exist + // FindUserByID also not called — short-circuited by errMessageNotFound branch }, }, + { + name: "first reply — parent user not found in userStore — warn-and-skip parent, still inserts replier", + msg: msg, + siteID: "site-a", + setupMocks: func(store *MockStore, ts *MockThreadStore) { + ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(nil, fmt.Errorf("wrap: %w", userstore.ErrUserNotFound)) + }, + expectReplierInsert: true, + }, + { + name: "first reply — parent user lookup DB error — returns error", + msg: msg, + siteID: "site-a", + setupMocks: func(store *MockStore, ts *MockThreadStore) { + ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(nil, errors.New("mongo: connection refused")) + }, + wantErr: true, + }, } for _, tt := range tests { @@ -864,11 +962,18 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { mockThreadStore := NewMockThreadStore(ctrl) mockUserStore := NewMockUserStore(ctrl) tt.setupMocks(mockStore, mockThreadStore) + if tt.extraUserStoreSetup != nil { + tt.extraUserStoreSetup(mockUserStore) + } + if tt.expectReplierInsert { + mockThreadStore.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + } h := NewHandler(mockStore, mockUserStore, mockThreadStore, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { return nil }) - _, err := h.handleThreadRoomAndSubscriptions(context.Background(), tt.msg, tt.siteID) + replier := &model.User{ID: tt.msg.UserID, Account: tt.msg.UserAccount, SiteID: "site-a"} + _, err := h.handleThreadRoomAndSubscriptions(context.Background(), tt.msg, tt.siteID, replier) if tt.wantErr { require.Error(t, err) } else { From 7e017c2f5aed545d443895e5c0dd53b0029e756e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:44:06 +0000 Subject: [PATCH 15/32] message-worker: emit outbox event on first-reply ThreadSubscription inserts https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- message-worker/handler.go | 16 +++-- message-worker/handler_test.go | 123 +++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 6 deletions(-) diff --git a/message-worker/handler.go b/message-worker/handler.go index 9952b6ef5..ffd413d72 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -166,19 +166,23 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message return fmt.Errorf("lookup parent owner site: %w", err) } if parentSiteID != "" { - if err := h.threadStore.InsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, parentSiteID, now), - ); err != nil { + parentSub := h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, parentSiteID, now) + if err := h.threadStore.InsertThreadSubscription(ctx, parentSub); err != nil { return fmt.Errorf("insert parent author thread subscription: %w", err) } + if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, msg.ID); err != nil { + return err + } } if replier != nil && msg.UserID != parentSender.ID { - if err := h.threadStore.InsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, threadRoomID, msg.UserID, msg.UserAccount, replier.SiteID, now), - ); err != nil { + replierSub := h.buildThreadSubscription(msg, threadRoomID, msg.UserID, msg.UserAccount, replier.SiteID, now) + if err := h.threadStore.InsertThreadSubscription(ctx, replierSub); err != nil { return fmt.Errorf("insert replier thread subscription: %w", err) } + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + return err + } } // Requires ThreadParentMessageCreatedAt to address the Cassandra row; diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 7decdefa0..333f17e67 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -1097,6 +1097,129 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { }) } +func TestHandler_FirstReply_OutboxPublishes(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + + parentSender := &cassParticipant{ID: "u-parent", Account: "parent-user"} + parentUserAtA := &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"} + parentUserAtC := &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-c"} + + type publishCall struct { + subj string + data []byte + msgID string + } + + tests := []struct { + name string + replierSite string + parentUser *model.User + wantPublishToSite map[string]int // destSite → expected count + }{ + { + name: "both local — no publish", + replierSite: "site-a", + parentUser: parentUserAtA, + wantPublishToSite: map[string]int{}, + }, + { + name: "replier remote — one publish to replier site", + replierSite: "site-b", + parentUser: parentUserAtA, + wantPublishToSite: map[string]int{"site-b": 1}, + }, + { + name: "parent remote — one publish to parent site", + replierSite: "site-a", + parentUser: parentUserAtC, + wantPublishToSite: map[string]int{"site-c": 1}, + }, + { + name: "both remote, different sites — two publishes", + replierSite: "site-b", + parentUser: parentUserAtC, + wantPublishToSite: map[string]int{"site-b": 1, "site-c": 1}, + }, + { + name: "both remote, same site — two publishes to that site", + replierSite: "site-b", + parentUser: &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-b"}, + wantPublishToSite: map[string]int{"site-b": 2}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent").Return(tt.parentUser, nil) + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + + var calls []publishCall + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, subj string, data []byte, msgID string) error { + calls = append(calls, publishCall{subj: subj, data: data, msgID: msgID}) + return nil + }) + + replier := &model.User{ID: "u-replier", Account: "replier", SiteID: tt.replierSite} + msg := &model.Message{ + ID: "msg-reply", + RoomID: "r1", + UserID: "u-replier", + UserAccount: "replier", + CreatedAt: now, + ThreadParentMessageID: "msg-parent", + } + + err := h.handleFirstThreadReply(context.Background(), msg, "tr-1", replier, now) + require.NoError(t, err) + + gotByDest := map[string]int{} + for _, c := range calls { + var outer model.OutboxEvent + require.NoError(t, json.Unmarshal(c.data, &outer)) + assert.Equal(t, model.OutboxThreadSubscriptionUpserted, outer.Type) + gotByDest[outer.DestSiteID]++ + } + assert.Equal(t, tt.wantPublishToSite, gotByDest) + }) + } +} + +func TestHandler_FirstReply_OutboxPublishError_NAKs(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent"). + Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-c"}, nil) + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + // Replier insert never reached because parent-publish fails first. + + boom := errors.New("publish boom") + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return boom + }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-replier", UserAccount: "replier", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + } + err := h.handleFirstThreadReply(context.Background(), msg, + "tr-1", &model.User{ID: "u-replier", SiteID: "site-b"}, now) + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} + // fakeJSMsg is a minimal jetstream.Msg test double that records whether Ack or // Nak was called so tests can assert on ack/nak behaviour. type fakeJSMsg struct { From 6058b59531857560f3d0fd433751cd850aa1c684 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:48:07 +0000 Subject: [PATCH 16/32] message-worker: wrap first-reply outbox publish errors with role context https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- message-worker/handler.go | 4 ++-- message-worker/handler_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/message-worker/handler.go b/message-worker/handler.go index ffd413d72..24e3674c0 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -171,7 +171,7 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message return fmt.Errorf("insert parent author thread subscription: %w", err) } if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, msg.ID); err != nil { - return err + return fmt.Errorf("publish parent thread subscription outbox: %w", err) } } @@ -181,7 +181,7 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message return fmt.Errorf("insert replier thread subscription: %w", err) } if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { - return err + return fmt.Errorf("publish replier thread subscription outbox: %w", err) } } diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 333f17e67..f3518408b 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -1220,6 +1220,37 @@ func TestHandler_FirstReply_OutboxPublishError_NAKs(t *testing.T) { assert.ErrorIs(t, err, boom) } +func TestHandler_FirstReply_ReplierOutboxPublishError_NAKs(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + // Parent at the local site → no parent publish. + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent"). + Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"}, nil) + // Both inserts run; replier publish fails. + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + + boom := errors.New("publish boom") + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return boom + }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-replier", UserAccount: "replier", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + } + err := h.handleFirstThreadReply(context.Background(), msg, + "tr-1", &model.User{ID: "u-replier", Account: "replier", SiteID: "site-b"}, now) + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} + // fakeJSMsg is a minimal jetstream.Msg test double that records whether Ack or // Nak was called so tests can assert on ack/nak behaviour. type fakeJSMsg struct { From 118aec621ababc3be49d961ec12baa3fd0ffaf70 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:50:45 +0000 Subject: [PATCH 17/32] message-worker: emit outbox event on subsequent-reply ThreadSubscription upserts https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- message-worker/handler.go | 24 ++++--- message-worker/handler_test.go | 117 +++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 9 deletions(-) diff --git a/message-worker/handler.go b/message-worker/handler.go index 24e3674c0..7512c03d7 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -215,18 +215,22 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me return "", fmt.Errorf("lookup parent owner site: %w", lookupErr) } if parentSiteID != "" { - if err := h.threadStore.UpsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, parentSiteID, now), - ); err != nil { + parentSub := h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, parentSiteID, now) + if err := h.threadStore.UpsertThreadSubscription(ctx, parentSub); err != nil { return "", fmt.Errorf("upsert parent author thread subscription: %w", err) } + if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, msg.ID); err != nil { + return "", fmt.Errorf("publish parent thread subscription outbox: %w", err) + } } if replier != nil && msg.UserID != parentSender.ID { - if err := h.threadStore.UpsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now), - ); err != nil { + replierSub := h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now) + if err := h.threadStore.UpsertThreadSubscription(ctx, replierSub); err != nil { return "", fmt.Errorf("upsert replier thread subscription: %w", err) } + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + return "", fmt.Errorf("publish replier thread subscription outbox: %w", err) + } } case errors.Is(err, errMessageNotFound): parentFound = false @@ -234,11 +238,13 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me "parentMessageID", msg.ThreadParentMessageID, "replyID", msg.ID) if replier != nil { - if err := h.threadStore.UpsertThreadSubscription(ctx, - h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now), - ); err != nil { + replierSub := h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now) + if err := h.threadStore.UpsertThreadSubscription(ctx, replierSub); err != nil { return "", fmt.Errorf("upsert replier thread subscription: %w", err) } + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + return "", fmt.Errorf("publish replier thread subscription outbox: %w", err) + } } default: return "", fmt.Errorf("get parent message sender: %w", err) diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index f3518408b..5d1877518 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -1251,6 +1251,123 @@ func TestHandler_FirstReply_ReplierOutboxPublishError_NAKs(t *testing.T) { assert.ErrorIs(t, err, boom) } +func TestHandler_SubsequentReply_OutboxPublishes(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + + parentSender := &cassParticipant{ID: "u-parent", Account: "parent-user"} + parentUserAtA := &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-a"} + parentUserAtC := &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-c"} + + tests := []struct { + name string + replierSite string + parentUser *model.User + wantPublishToSite map[string]int + }{ + { + name: "both local — no publish", + replierSite: "site-a", + parentUser: parentUserAtA, + wantPublishToSite: map[string]int{}, + }, + { + name: "replier remote — one publish", + replierSite: "site-b", + parentUser: parentUserAtA, + wantPublishToSite: map[string]int{"site-b": 1}, + }, + { + name: "parent remote — one publish", + replierSite: "site-a", + parentUser: parentUserAtC, + wantPublishToSite: map[string]int{"site-c": 1}, + }, + { + name: "both remote, different sites — two publishes", + replierSite: "site-b", + parentUser: parentUserAtC, + wantPublishToSite: map[string]int{"site-b": 1, "site-c": 1}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + ts.EXPECT().GetThreadRoomByParentMessageID(gomock.Any(), "msg-parent"). + Return(&model.ThreadRoom{ID: "tr-existing"}, nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent").Return(tt.parentUser, nil) + ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", now).Return(nil) + + var publishedDests []string + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, data []byte, _ string) error { + var outer model.OutboxEvent + if err := json.Unmarshal(data, &outer); err != nil { + return err + } + publishedDests = append(publishedDests, outer.DestSiteID) + return nil + }) + + replier := &model.User{ID: "u-replier", Account: "replier", SiteID: tt.replierSite} + msg := &model.Message{ + ID: "msg-reply", + RoomID: "r1", + UserID: "u-replier", + UserAccount: "replier", + CreatedAt: now, + ThreadParentMessageID: "msg-parent", + } + + roomID, err := h.handleSubsequentThreadReply(context.Background(), msg, replier, now) + require.NoError(t, err) + assert.Equal(t, "tr-existing", roomID) + + gotByDest := map[string]int{} + for _, d := range publishedDests { + gotByDest[d]++ + } + assert.Equal(t, tt.wantPublishToSite, gotByDest) + }) + } +} + +func TestHandler_SubsequentReply_OutboxPublishError_NAKs(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + store := NewMockStore(ctrl) + us := NewMockUserStore(ctrl) + ts := NewMockThreadStore(ctrl) + + ts.EXPECT().GetThreadRoomByParentMessageID(gomock.Any(), "msg-parent"). + Return(&model.ThreadRoom{ID: "tr-1"}, nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent"). + Return(&cassParticipant{ID: "u-parent", Account: "parent-user"}, nil) + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(&model.User{ID: "u-parent", SiteID: "site-c"}, nil) + ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) + + boom := errors.New("publish boom") + h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return boom + }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-replier", UserAccount: "replier", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + } + _, err := h.handleSubsequentThreadReply(context.Background(), msg, + &model.User{ID: "u-replier", SiteID: "site-b"}, now) + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} + // fakeJSMsg is a minimal jetstream.Msg test double that records whether Ack or // Nak was called so tests can assert on ack/nak behaviour. type fakeJSMsg struct { From d98abf71fd11864bcfebcc641aa21530c5d94282 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 06:54:51 +0000 Subject: [PATCH 18/32] message-worker: emit outbox event on mention-marked ThreadSubscriptions Wire publishThreadSubOutboxIfRemote into markThreadMentions so that remote mentionees receive a thread_subscription_upserted outbox event with HasMention=true. Local mentionees short-circuit with no publish. https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- message-worker/handler.go | 6 +- message-worker/handler_test.go | 150 +++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 3 deletions(-) diff --git a/message-worker/handler.go b/message-worker/handler.go index 7512c03d7..1890f7751 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -316,9 +316,6 @@ func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, th } ownerSiteID := p.SiteID if ownerSiteID == "" { - // Defensive: should not happen since mention.Resolve populates SiteID - // for resolved users. Fall back to event site so the local mark still - // happens; outbox publish (Task 8) will skip on empty SiteID anyway. slog.Warn("mentionee participant has empty SiteID, falling back to event site", "account", p.Account, "userID", p.UserID, "msgID", msg.ID) ownerSiteID = eventSiteID @@ -328,6 +325,9 @@ func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, th if err := h.threadStore.MarkThreadSubscriptionMention(ctx, sub); err != nil { return fmt.Errorf("mark thread subscription mention for user %s: %w", p.UserID, err) } + if err := h.publishThreadSubOutboxIfRemote(ctx, sub, msg.ID); err != nil { + return fmt.Errorf("publish thread mention outbox for user %s: %w", p.UserID, err) + } } return nil } diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 5d1877518..583e448fd 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -1368,6 +1368,156 @@ func TestHandler_SubsequentReply_OutboxPublishError_NAKs(t *testing.T) { assert.ErrorIs(t, err, boom) } +func TestHandler_MarkThreadMentions_OutboxPublishes(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + + tests := []struct { + name string + mentionees []model.Participant + wantPublishToSite map[string]int + }{ + { + name: "no mentions — no publish", + mentionees: nil, + wantPublishToSite: map[string]int{}, + }, + { + name: "local mentionee — mark only, no publish", + mentionees: []model.Participant{ + {UserID: "u-bob", Account: "bob", SiteID: "site-a"}, + }, + wantPublishToSite: map[string]int{}, + }, + { + name: "remote mentionee — mark and publish", + mentionees: []model.Participant{ + {UserID: "u-bob", Account: "bob", SiteID: "site-b"}, + }, + wantPublishToSite: map[string]int{"site-b": 1}, + }, + { + name: "two remote mentionees in different sites — two publishes", + mentionees: []model.Participant{ + {UserID: "u-bob", Account: "bob", SiteID: "site-b"}, + {UserID: "u-carol", Account: "carol", SiteID: "site-c"}, + }, + wantPublishToSite: map[string]int{"site-b": 1, "site-c": 1}, + }, + { + name: "@all is skipped — no mark, no publish", + mentionees: []model.Participant{ + {Account: "all", EngName: "all"}, + }, + wantPublishToSite: map[string]int{}, + }, + { + name: "sender self-mention is skipped", + mentionees: []model.Participant{ + {UserID: "u-sender", Account: "sender", SiteID: "site-b"}, + }, + wantPublishToSite: map[string]int{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + ts := NewMockThreadStore(ctrl) + + expectedMarks := 0 + for _, p := range tt.mentionees { + if p.Account == "all" { + continue + } + if p.UserID == "u-sender" { + continue + } + expectedMarks++ + } + ts.EXPECT().MarkThreadSubscriptionMention(gomock.Any(), gomock.Any()). + Times(expectedMarks).Return(nil) + + var publishedDests []string + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), ts, "site-a", + func(_ context.Context, _ string, data []byte, _ string) error { + var outer model.OutboxEvent + if err := json.Unmarshal(data, &outer); err != nil { + return err + } + publishedDests = append(publishedDests, outer.DestSiteID) + return nil + }) + + msg := &model.Message{ + ID: "msg-reply", + RoomID: "r1", + UserID: "u-sender", + UserAccount: "sender", + CreatedAt: now, + ThreadParentMessageID: "msg-parent", + Mentions: tt.mentionees, + } + err := h.markThreadMentions(context.Background(), msg, "tr-1", "site-a") + require.NoError(t, err) + + gotByDest := map[string]int{} + for _, d := range publishedDests { + gotByDest[d]++ + } + assert.Equal(t, tt.wantPublishToSite, gotByDest) + }) + } +} + +func TestHandler_MarkThreadMentions_OutboxPublishError_NAKs(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + ts := NewMockThreadStore(ctrl) + ts.EXPECT().MarkThreadSubscriptionMention(gomock.Any(), gomock.Any()).Return(nil) + + boom := errors.New("publish boom") + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), ts, "site-a", + func(_ context.Context, _ string, _ []byte, _ string) error { return boom }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-sender", UserAccount: "sender", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + Mentions: []model.Participant{{UserID: "u-bob", Account: "bob", SiteID: "site-b"}}, + } + err := h.markThreadMentions(context.Background(), msg, "tr-1", "site-a") + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} + +func TestHandler_MarkThreadMentions_HasMentionInPayload(t *testing.T) { + now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + ctrl := gomock.NewController(t) + ts := NewMockThreadStore(ctrl) + ts.EXPECT().MarkThreadSubscriptionMention(gomock.Any(), gomock.Any()).Return(nil) + + var captured []byte + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), ts, "site-a", + func(_ context.Context, _ string, data []byte, _ string) error { + captured = data + return nil + }) + + msg := &model.Message{ + ID: "msg-reply", RoomID: "r1", UserID: "u-sender", UserAccount: "sender", + CreatedAt: now, ThreadParentMessageID: "msg-parent", + Mentions: []model.Participant{{UserID: "u-bob", Account: "bob", SiteID: "site-b"}}, + } + require.NoError(t, h.markThreadMentions(context.Background(), msg, "tr-1", "site-a")) + + var outer model.OutboxEvent + require.NoError(t, json.Unmarshal(captured, &outer)) + var sub model.ThreadSubscription + require.NoError(t, json.Unmarshal(outer.Payload, &sub)) + assert.True(t, sub.HasMention, "outbox-emitted ThreadSubscription must carry HasMention=true") + assert.Equal(t, "u-bob", sub.UserID) + assert.Equal(t, "site-b", sub.SiteID) +} + // fakeJSMsg is a minimal jetstream.Msg test double that records whether Ack or // Nak was called so tests can assert on ack/nak behaviour. type fakeJSMsg struct { From c1efd78eefd5ef09fc7b3e7194e97325896bdb45 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 07:00:47 +0000 Subject: [PATCH 19/32] inbox-worker: dispatch thread_subscription_upserted to UpsertThreadSubscription https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- inbox-worker/handler.go | 19 +++++ inbox-worker/handler_test.go | 142 +++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+) diff --git a/inbox-worker/handler.go b/inbox-worker/handler.go index 7c6fcc0b5..0071d6473 100644 --- a/inbox-worker/handler.go +++ b/inbox-worker/handler.go @@ -19,6 +19,7 @@ type InboxStore interface { UpdateSubscriptionRoles(ctx context.Context, account, roomID string, roles []model.Role) error DeleteSubscriptionsByAccounts(ctx context.Context, roomID string, accounts []string) error FindUsersByAccounts(ctx context.Context, accounts []string) ([]model.User, error) + UpsertThreadSubscription(ctx context.Context, sub *model.ThreadSubscription) error } // Handler processes incoming cross-site OutboxEvent messages. @@ -47,6 +48,8 @@ func (h *Handler) HandleEvent(ctx context.Context, data []byte) error { return h.handleRoomSync(ctx, &evt) case "role_updated": return h.handleRoleUpdated(ctx, &evt) + case "thread_subscription_upserted": + return h.handleThreadSubscriptionUpserted(ctx, &evt) default: slog.Warn("unknown event type, skipping", "type", evt.Type) return nil @@ -163,3 +166,19 @@ func (h *Handler) handleRoleUpdated(ctx context.Context, evt *model.OutboxEvent) } return nil } + +// handleThreadSubscriptionUpserted upserts a ThreadSubscription on the local +// site when message-worker on another site reports that a user (parent author, +// replier, or mentionee) is participating in a thread. The Mongo store layer +// is responsible for the monotonic hasMention merge — see store impl. +func (h *Handler) handleThreadSubscriptionUpserted(ctx context.Context, evt *model.OutboxEvent) error { + var sub model.ThreadSubscription + if err := json.Unmarshal(evt.Payload, &sub); err != nil { + return fmt.Errorf("unmarshal thread_subscription_upserted payload: %w", err) + } + if err := h.store.UpsertThreadSubscription(ctx, &sub); err != nil { + return fmt.Errorf("upsert thread subscription (threadRoomID %q, userID %q): %w", + sub.ThreadRoomID, sub.UserID, err) + } + return nil +} diff --git a/inbox-worker/handler_test.go b/inbox-worker/handler_test.go index 698122fb3..645998c9b 100644 --- a/inbox-worker/handler_test.go +++ b/inbox-worker/handler_test.go @@ -29,6 +29,7 @@ type stubInboxStore struct { rooms []model.Room roleUpdates []roleUpdate users []model.User + threadSubs []model.ThreadSubscription } func (s *stubInboxStore) CreateSubscription(ctx context.Context, sub *model.Subscription) error { @@ -128,6 +129,32 @@ func (s *stubInboxStore) BulkCreateSubscriptions(_ context.Context, subs []*mode return nil } +func (s *stubInboxStore) UpsertThreadSubscription(_ context.Context, sub *model.ThreadSubscription) error { + s.mu.Lock() + defer s.mu.Unlock() + for i := range s.threadSubs { + if s.threadSubs[i].ThreadRoomID == sub.ThreadRoomID && s.threadSubs[i].UserID == sub.UserID { + // Monotonic hasMention merge — never clear true→false. + if sub.HasMention { + s.threadSubs[i].HasMention = true + } + s.threadSubs[i].UpdatedAt = sub.UpdatedAt + return nil + } + } + s.threadSubs = append(s.threadSubs, *sub) + return nil +} + +func (s *stubInboxStore) getThreadSubs() []model.ThreadSubscription { + s.mu.Lock() + defer s.mu.Unlock() + cp := make([]model.ThreadSubscription, len(s.threadSubs)) + copy(cp, s.threadSubs) + return cp +} + + // --- Tests --- func TestHandleEvent_MemberAdded(t *testing.T) { @@ -773,3 +800,118 @@ func TestHandleEvent_MemberRemoved_DeleteError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "delete subscriptions") } + +func TestHandleEvent_ThreadSubscriptionUpserted_Insert(t *testing.T) { + store := &stubInboxStore{} + pub := &mockPublisher{} + h := NewHandler(store, pub) + + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + sub := model.ThreadSubscription{ + ID: "sub-1", + ParentMessageID: "pm-1", + RoomID: "r1", + ThreadRoomID: "tr-1", + UserID: "u-bob", + UserAccount: "bob", + SiteID: "site-b", + HasMention: false, + CreatedAt: now, + UpdatedAt: now, + } + subData, err := json.Marshal(sub) + require.NoError(t, err) + + evt := model.OutboxEvent{ + Type: "thread_subscription_upserted", + SiteID: "site-a", + DestSiteID: "site-b", + Payload: subData, + Timestamp: now.UnixMilli(), + } + evtData, _ := json.Marshal(evt) + + require.NoError(t, h.HandleEvent(context.Background(), evtData)) + + got := store.getThreadSubs() + require.Len(t, got, 1) + assert.Equal(t, sub, got[0]) + assert.Empty(t, pub.getRecords(), "no client-facing publish on thread sub upsert") +} + +func TestHandleEvent_ThreadSubscriptionUpserted_MonotonicHasMention(t *testing.T) { + store := &stubInboxStore{} + pub := &mockPublisher{} + h := NewHandler(store, pub) + + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + mentionSub := model.ThreadSubscription{ + ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + HasMention: true, CreatedAt: now, UpdatedAt: now, + } + mentionData, _ := json.Marshal(mentionSub) + mentionEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: mentionData, Timestamp: now.UnixMilli(), + }) + require.NoError(t, h.HandleEvent(context.Background(), mentionEvt)) + + // Second event for same (threadRoomID, userID) with HasMention=false must NOT clear it. + plainSub := mentionSub + plainSub.HasMention = false + plainSub.UpdatedAt = now.Add(time.Minute) + plainData, _ := json.Marshal(plainSub) + plainEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: plainData, Timestamp: plainSub.UpdatedAt.UnixMilli(), + }) + require.NoError(t, h.HandleEvent(context.Background(), plainEvt)) + + got := store.getThreadSubs() + require.Len(t, got, 1) + assert.True(t, got[0].HasMention, "hasMention must remain true after a non-mention event") +} + +func TestHandleEvent_ThreadSubscriptionUpserted_InvalidPayload(t *testing.T) { + store := &stubInboxStore{} + pub := &mockPublisher{} + h := NewHandler(store, pub) + + evt := model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: []byte("not json"), + } + evtData, _ := json.Marshal(evt) + + require.Error(t, h.HandleEvent(context.Background(), evtData)) + assert.Empty(t, store.getThreadSubs()) +} + +func TestHandleEvent_ThreadSubscriptionUpserted_StoreError(t *testing.T) { + store := &errorThreadSubStore{stubInboxStore: &stubInboxStore{}} + pub := &mockPublisher{} + h := NewHandler(store, pub) + + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + sub := model.ThreadSubscription{ + ID: "sub-1", ThreadRoomID: "tr-1", UserID: "u-bob", SiteID: "site-b", + CreatedAt: now, UpdatedAt: now, + } + subData, _ := json.Marshal(sub) + evtData, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: subData, Timestamp: now.UnixMilli(), + }) + + err := h.HandleEvent(context.Background(), evtData) + require.Error(t, err) +} + +type errorThreadSubStore struct { + *stubInboxStore +} + +func (s *errorThreadSubStore) UpsertThreadSubscription(_ context.Context, _ *model.ThreadSubscription) error { + return fmt.Errorf("boom") +} From fc6ee18d3925ae1d2004e4578a6701657b3bd001 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 07:04:26 +0000 Subject: [PATCH 20/32] inbox-worker: Mongo UpsertThreadSubscription with monotonic hasMention merge https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- inbox-worker/main.go | 65 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 6 deletions(-) diff --git a/inbox-worker/main.go b/inbox-worker/main.go index 6dbd60b95..d7d614730 100644 --- a/inbox-worker/main.go +++ b/inbox-worker/main.go @@ -36,9 +36,10 @@ type config struct { // mongoInboxStore implements InboxStore using MongoDB. type mongoInboxStore struct { - subCol *mongo.Collection - roomCol *mongo.Collection - userCol *mongo.Collection + subCol *mongo.Collection + roomCol *mongo.Collection + userCol *mongo.Collection + threadSubCol *mongo.Collection } func (s *mongoInboxStore) CreateSubscription(ctx context.Context, sub *model.Subscription) error { @@ -106,6 +107,53 @@ func (s *mongoInboxStore) BulkCreateSubscriptions(ctx context.Context, subs []*m return nil } +// ensureIndexes creates the unique index on (threadRoomId, userId) used by +// UpsertThreadSubscription. The index name and shape match what message-worker +// creates in its own threadStoreMongo so both services agree on the natural +// key for thread subscriptions. +func (s *mongoInboxStore) ensureIndexes(ctx context.Context) error { + if _, err := s.threadSubCol.Indexes().CreateOne(ctx, mongo.IndexModel{ + Keys: bson.D{{Key: "threadRoomId", Value: 1}, {Key: "userId", Value: 1}}, + Options: options.Index().SetUnique(true), + }); err != nil { + return fmt.Errorf("ensure threadSubscriptions (threadRoomId,userId) index: %w", err) + } + return nil +} + +// UpsertThreadSubscription inserts the subscription on first event for a +// (threadRoomId, userId) pair, and on subsequent events updates only +// updatedAt and (monotonically) hasMention. $setOnInsert pins the immutable +// fields on insert; $set always refreshes updatedAt; $max on hasMention +// guarantees a non-mention event never clears a prior mention=true. +// +// $setOnInsert and $max operate on disjoint fields (hasMention is set by $max +// only — never by $setOnInsert) so MongoDB doesn't reject the update with a +// "conflicting update operators" error. +func (s *mongoInboxStore) UpsertThreadSubscription(ctx context.Context, sub *model.ThreadSubscription) error { + filter := bson.M{"threadRoomId": sub.ThreadRoomID, "userId": sub.UserID} + update := bson.M{ + "$setOnInsert": bson.M{ + "_id": sub.ID, + "parentMessageId": sub.ParentMessageID, + "roomId": sub.RoomID, + "threadRoomId": sub.ThreadRoomID, + "userId": sub.UserID, + "userAccount": sub.UserAccount, + "siteId": sub.SiteID, + "lastSeenAt": sub.LastSeenAt, + "createdAt": sub.CreatedAt, + }, + "$set": bson.M{"updatedAt": sub.UpdatedAt}, + "$max": bson.M{"hasMention": sub.HasMention}, + } + if _, err := s.threadSubCol.UpdateOne(ctx, filter, update, options.UpdateOne().SetUpsert(true)); err != nil { + return fmt.Errorf("upsert thread subscription (threadRoomID %q, userID %q): %w", + sub.ThreadRoomID, sub.UserID, err) + } + return nil +} + func main() { slog.SetDefault(slog.New(slog.NewJSONHandler(os.Stdout, nil))) @@ -130,9 +178,14 @@ func main() { } db := mongoClient.Database(cfg.MongoDB) store := &mongoInboxStore{ - subCol: db.Collection("subscriptions"), - roomCol: db.Collection("rooms"), - userCol: db.Collection("users"), + subCol: db.Collection("subscriptions"), + roomCol: db.Collection("rooms"), + userCol: db.Collection("users"), + threadSubCol: db.Collection("threadSubscriptions"), + } + if err := store.ensureIndexes(ctx); err != nil { + slog.Error("ensure indexes failed", "error", err) + os.Exit(1) } nc, err := natsutil.Connect(cfg.NatsURL, cfg.NatsCredsFile) From 65a0f22a3a381211d0653538a43003cc02fe791f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 07:08:10 +0000 Subject: [PATCH 21/32] inbox-worker: integration tests for UpsertThreadSubscription monotonic merge https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- inbox-worker/integration_test.go | 114 +++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/inbox-worker/integration_test.go b/inbox-worker/integration_test.go index e140f707e..cc26e84d3 100644 --- a/inbox-worker/integration_test.go +++ b/inbox-worker/integration_test.go @@ -190,3 +190,117 @@ func TestInboxWorker_MemberRemoved_Integration(t *testing.T) { // No publish — room-worker handles user notification via NATS supercluster. } + +func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) { + db := setupMongo(t) + ctx := context.Background() + + store := &mongoInboxStore{ + subCol: db.Collection("subscriptions"), + roomCol: db.Collection("rooms"), + userCol: db.Collection("users"), + threadSubCol: db.Collection("threadSubscriptions"), + } + require.NoError(t, store.ensureIndexes(ctx)) + + pub := &recordingPublisher{} + handler := NewHandler(store, pub) + + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + sub := model.ThreadSubscription{ + ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + HasMention: false, CreatedAt: now, UpdatedAt: now, + } + subData, _ := json.Marshal(sub) + evtData, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: subData, Timestamp: now.UnixMilli(), + }) + + require.NoError(t, handler.HandleEvent(ctx, evtData)) + + var got model.ThreadSubscription + err := db.Collection("threadSubscriptions"). + FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). + Decode(&got) + require.NoError(t, err) + assert.Equal(t, "sub-1", got.ID) + assert.Equal(t, "site-b", got.SiteID) + assert.False(t, got.HasMention) + assert.True(t, got.CreatedAt.Equal(now)) + assert.True(t, got.UpdatedAt.Equal(now)) + + // No client publishes for thread subscription upserts. + assert.Empty(t, pub.subjects) +} + +func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t *testing.T) { + db := setupMongo(t) + ctx := context.Background() + + store := &mongoInboxStore{ + subCol: db.Collection("subscriptions"), + roomCol: db.Collection("rooms"), + userCol: db.Collection("users"), + threadSubCol: db.Collection("threadSubscriptions"), + } + require.NoError(t, store.ensureIndexes(ctx)) + + handler := NewHandler(store, &recordingPublisher{}) + now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + + // First event: HasMention=true. + mentionSub := model.ThreadSubscription{ + ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + HasMention: true, CreatedAt: now, UpdatedAt: now, + } + mentionData, _ := json.Marshal(mentionSub) + mentionEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: mentionData, Timestamp: now.UnixMilli(), + }) + require.NoError(t, handler.HandleEvent(ctx, mentionEvt)) + + // Second event: HasMention=false (later updatedAt). Must NOT clear the flag. + plainSub := mentionSub + plainSub.HasMention = false + later := now.Add(time.Minute) + plainSub.UpdatedAt = later + plainData, _ := json.Marshal(plainSub) + plainEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: plainData, Timestamp: later.UnixMilli(), + }) + require.NoError(t, handler.HandleEvent(ctx, plainEvt)) + + var got model.ThreadSubscription + err := db.Collection("threadSubscriptions"). + FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). + Decode(&got) + require.NoError(t, err) + assert.True(t, got.HasMention, "hasMention must remain true after a non-mention event") + assert.True(t, got.UpdatedAt.Equal(later), "updatedAt must advance to the later event's value") + // _id and createdAt come from $setOnInsert and must remain from the first event. + assert.Equal(t, "sub-1", got.ID) + assert.True(t, got.CreatedAt.Equal(now)) + + // Third event: HasMention=true again. Idempotent — still true, updatedAt advances. + thirdSub := plainSub + thirdSub.HasMention = true + evenLater := later.Add(time.Minute) + thirdSub.UpdatedAt = evenLater + thirdData, _ := json.Marshal(thirdSub) + thirdEvt, _ := json.Marshal(model.OutboxEvent{ + Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", + Payload: thirdData, Timestamp: evenLater.UnixMilli(), + }) + require.NoError(t, handler.HandleEvent(ctx, thirdEvt)) + + require.NoError(t, db.Collection("threadSubscriptions"). + FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). + Decode(&got)) + assert.True(t, got.HasMention) + assert.True(t, got.UpdatedAt.Equal(evenLater)) +} From e4140410646de0fce747a0fb86943a3dd4575baa Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 07:13:29 +0000 Subject: [PATCH 22/32] message-worker: add same-site subsequent-reply test case for symmetry --- message-worker/handler_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 583e448fd..ebc7f9b89 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -1288,6 +1288,12 @@ func TestHandler_SubsequentReply_OutboxPublishes(t *testing.T) { parentUser: parentUserAtC, wantPublishToSite: map[string]int{"site-b": 1, "site-c": 1}, }, + { + name: "both remote, same site — two publishes to that site", + replierSite: "site-b", + parentUser: &model.User{ID: "u-parent", Account: "parent-user", SiteID: "site-b"}, + wantPublishToSite: map[string]int{"site-b": 2}, + }, } for _, tt := range tests { From 371a14264fb6274ee315da78bb04139a276d5013 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 08:17:41 +0000 Subject: [PATCH 23/32] test+docs: subsequent-reply parent-not-found case + $max-on-bool comment https://claude.ai/code/session_01J9Ht3dT6EzmvoipcVzz8NB --- inbox-worker/main.go | 3 +++ message-worker/handler_test.go | 45 ++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/inbox-worker/main.go b/inbox-worker/main.go index d7d614730..51563054d 100644 --- a/inbox-worker/main.go +++ b/inbox-worker/main.go @@ -127,6 +127,9 @@ func (s *mongoInboxStore) ensureIndexes(ctx context.Context) error { // fields on insert; $set always refreshes updatedAt; $max on hasMention // guarantees a non-mention event never clears a prior mention=true. // +// $max on a bool works because BSON encodes false (0x00) < true (0x01), so +// $max(existing, incoming) for a bool is equivalent to a monotonic OR. +// // $setOnInsert and $max operate on disjoint fields (hasMention is set by $max // only — never by $setOnInsert) so MongoDB doesn't reject the update with a // "conflicting update operators" error. diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index ebc7f9b89..fcc5fbacf 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -939,6 +939,51 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { }, expectReplierInsert: true, }, + { + name: "subsequent reply — parent user not found in userStore — warn-and-skip parent, still upserts replier", + msg: msg, + siteID: "site-a", + setupMocks: func(store *MockStore, ts *MockThreadStore) { + ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()). + Return(errThreadRoomExists) + ts.EXPECT().GetThreadRoomByParentMessageID(gomock.Any(), "msg-parent"). + Return(&model.ThreadRoom{ID: "tr-existing"}, nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent"). + Return(parentSender, nil) + // Parent upsert SKIPPED because lookupOwnerSiteID returns ("", nil). + // Replier upsert still runs. + ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, sub *model.ThreadSubscription) error { + assert.Equal(t, "u-replier", sub.UserID, "only replier should be upserted; parent skipped") + return nil + }) + ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", now). + Return(nil) + }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(nil, fmt.Errorf("wrap: %w", userstore.ErrUserNotFound)) + }, + }, + { + name: "subsequent reply — parent user lookup DB error — returns error", + msg: msg, + siteID: "site-a", + setupMocks: func(store *MockStore, ts *MockThreadStore) { + ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()). + Return(errThreadRoomExists) + ts.EXPECT().GetThreadRoomByParentMessageID(gomock.Any(), "msg-parent"). + Return(&model.ThreadRoom{ID: "tr-existing"}, nil) + store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent"). + Return(parentSender, nil) + // Lookup error short-circuits — no upserts, no UpdateThreadRoomLastMessage. + }, + extraUserStoreSetup: func(us *MockUserStore) { + us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). + Return(nil, errors.New("mongo: connection refused")) + }, + wantErr: true, + }, { name: "first reply — parent user lookup DB error — returns error", msg: msg, From 0d53aa0d64d22d1f566906b49adacc1b37e03b68 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Apr 2026 10:06:43 +0000 Subject: [PATCH 24/32] test: post-rebase fixups for upstream Publisher removal and userAccount arg - Drop the now-removed Publisher arg from NewHandler calls in our 4 inbox-worker thread-subscription tests and 2 integration tests. - Drop the obsolete mockPublisher/publishRecord types; upstream removed publishing from inbox-worker and the existing tests no longer use them. - Add the new userAccount arg to two UpdateThreadRoomLastMessage mock expectations our tests added (PR #127 changed the signature). --- inbox-worker/handler_test.go | 14 ++++---------- inbox-worker/integration_test.go | 8 ++------ message-worker/handler_test.go | 4 ++-- 3 files changed, 8 insertions(+), 18 deletions(-) diff --git a/inbox-worker/handler_test.go b/inbox-worker/handler_test.go index 645998c9b..a648b02d0 100644 --- a/inbox-worker/handler_test.go +++ b/inbox-worker/handler_test.go @@ -154,7 +154,6 @@ func (s *stubInboxStore) getThreadSubs() []model.ThreadSubscription { return cp } - // --- Tests --- func TestHandleEvent_MemberAdded(t *testing.T) { @@ -803,8 +802,7 @@ func TestHandleEvent_MemberRemoved_DeleteError(t *testing.T) { func TestHandleEvent_ThreadSubscriptionUpserted_Insert(t *testing.T) { store := &stubInboxStore{} - pub := &mockPublisher{} - h := NewHandler(store, pub) + h := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) sub := model.ThreadSubscription{ @@ -836,13 +834,11 @@ func TestHandleEvent_ThreadSubscriptionUpserted_Insert(t *testing.T) { got := store.getThreadSubs() require.Len(t, got, 1) assert.Equal(t, sub, got[0]) - assert.Empty(t, pub.getRecords(), "no client-facing publish on thread sub upsert") } func TestHandleEvent_ThreadSubscriptionUpserted_MonotonicHasMention(t *testing.T) { store := &stubInboxStore{} - pub := &mockPublisher{} - h := NewHandler(store, pub) + h := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) mentionSub := model.ThreadSubscription{ @@ -875,8 +871,7 @@ func TestHandleEvent_ThreadSubscriptionUpserted_MonotonicHasMention(t *testing.T func TestHandleEvent_ThreadSubscriptionUpserted_InvalidPayload(t *testing.T) { store := &stubInboxStore{} - pub := &mockPublisher{} - h := NewHandler(store, pub) + h := NewHandler(store) evt := model.OutboxEvent{ Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", @@ -890,8 +885,7 @@ func TestHandleEvent_ThreadSubscriptionUpserted_InvalidPayload(t *testing.T) { func TestHandleEvent_ThreadSubscriptionUpserted_StoreError(t *testing.T) { store := &errorThreadSubStore{stubInboxStore: &stubInboxStore{}} - pub := &mockPublisher{} - h := NewHandler(store, pub) + h := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) sub := model.ThreadSubscription{ diff --git a/inbox-worker/integration_test.go b/inbox-worker/integration_test.go index cc26e84d3..effa87887 100644 --- a/inbox-worker/integration_test.go +++ b/inbox-worker/integration_test.go @@ -203,8 +203,7 @@ func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) } require.NoError(t, store.ensureIndexes(ctx)) - pub := &recordingPublisher{} - handler := NewHandler(store, pub) + handler := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) sub := model.ThreadSubscription{ @@ -230,9 +229,6 @@ func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) assert.False(t, got.HasMention) assert.True(t, got.CreatedAt.Equal(now)) assert.True(t, got.UpdatedAt.Equal(now)) - - // No client publishes for thread subscription upserts. - assert.Empty(t, pub.subjects) } func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t *testing.T) { @@ -247,7 +243,7 @@ func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t * } require.NoError(t, store.ensureIndexes(ctx)) - handler := NewHandler(store, &recordingPublisher{}) + handler := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) // First event: HasMention=true. diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index fcc5fbacf..74171ee4c 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -957,7 +957,7 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { assert.Equal(t, "u-replier", sub.UserID, "only replier should be upserted; parent skipped") return nil }) - ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", now). + ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", "replier", now). Return(nil) }, extraUserStoreSetup: func(us *MockUserStore) { @@ -1354,7 +1354,7 @@ func TestHandler_SubsequentReply_OutboxPublishes(t *testing.T) { us.EXPECT().FindUserByID(gomock.Any(), "u-parent").Return(tt.parentUser, nil) ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()).Return(nil) - ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", now).Return(nil) + ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", "replier", now).Return(nil) var publishedDests []string h := NewHandler(store, us, ts, "site-a", func(_ context.Context, _ string, data []byte, _ string) error { From 56f99c2a08c3f512e00b8bb48591c9366ddcbd93 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Apr 2026 03:24:27 +0000 Subject: [PATCH 25/32] ThreadSubscription.SiteID stays as room site; route by owner site separately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mliu33's review: Subscription.SiteID and ThreadSubscription.SiteID both mean "the home site of the room" — a back-reference, not a self-identifier of the owner. Across federation the field is constant on every replica. Revert the Task-5 semantic change. Subscription.SiteID is now built from eventSiteID (the room's site) at all three call sites (parent, replier, mentionee). publishThreadSubOutboxIfRemote takes ownerSiteID as an explicit parameter, used only to decide whether to publish and where (not stored on the subscription). - handler.go: buildThreadSubscription param renamed back to siteID; helper signature gains ownerSiteID; handleFirstThreadReply / handleSubsequent- ThreadReply gain eventSiteID and pass it for the sub while routing the outbox by parentOwnerSite or replier.SiteID; markThreadMentions stores eventSiteID on the sub and routes by Participant.SiteID. - pkg/model/threadsubscription.go: doc comment clarifying SiteID is the room's site, same semantic as Subscription.SiteID. - Tests updated to assert SiteID is the room's site after the round-trip and to call the new helper/handler signatures. --- inbox-worker/handler_test.go | 8 +-- inbox-worker/integration_test.go | 11 ++-- message-worker/handler.go | 91 ++++++++++++++++---------------- message-worker/handler_test.go | 43 +++++++-------- pkg/model/threadsubscription.go | 6 ++- 5 files changed, 85 insertions(+), 74 deletions(-) diff --git a/inbox-worker/handler_test.go b/inbox-worker/handler_test.go index a648b02d0..54daf344e 100644 --- a/inbox-worker/handler_test.go +++ b/inbox-worker/handler_test.go @@ -805,6 +805,7 @@ func TestHandleEvent_ThreadSubscriptionUpserted_Insert(t *testing.T) { h := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + // SiteID is the room's home site (site-a), preserved across federation. sub := model.ThreadSubscription{ ID: "sub-1", ParentMessageID: "pm-1", @@ -812,7 +813,7 @@ func TestHandleEvent_ThreadSubscriptionUpserted_Insert(t *testing.T) { ThreadRoomID: "tr-1", UserID: "u-bob", UserAccount: "bob", - SiteID: "site-b", + SiteID: "site-a", HasMention: false, CreatedAt: now, UpdatedAt: now, @@ -841,9 +842,10 @@ func TestHandleEvent_ThreadSubscriptionUpserted_MonotonicHasMention(t *testing.T h := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + // SiteID is the room's home site (site-a), preserved across federation. mentionSub := model.ThreadSubscription{ ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", - UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-a", HasMention: true, CreatedAt: now, UpdatedAt: now, } mentionData, _ := json.Marshal(mentionSub) @@ -889,7 +891,7 @@ func TestHandleEvent_ThreadSubscriptionUpserted_StoreError(t *testing.T) { now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) sub := model.ThreadSubscription{ - ID: "sub-1", ThreadRoomID: "tr-1", UserID: "u-bob", SiteID: "site-b", + ID: "sub-1", ThreadRoomID: "tr-1", UserID: "u-bob", SiteID: "site-a", CreatedAt: now, UpdatedAt: now, } subData, _ := json.Marshal(sub) diff --git a/inbox-worker/integration_test.go b/inbox-worker/integration_test.go index effa87887..30abfea1d 100644 --- a/inbox-worker/integration_test.go +++ b/inbox-worker/integration_test.go @@ -206,9 +206,12 @@ func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) handler := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) + // Subscription.SiteID is the room's home site (site-a). Bob's home is site-b + // (where this inbox-worker instance lives), inferred from the document being + // stored on this site rather than from the field. sub := model.ThreadSubscription{ ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", - UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-a", HasMention: false, CreatedAt: now, UpdatedAt: now, } subData, _ := json.Marshal(sub) @@ -225,7 +228,7 @@ func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) Decode(&got) require.NoError(t, err) assert.Equal(t, "sub-1", got.ID) - assert.Equal(t, "site-b", got.SiteID) + assert.Equal(t, "site-a", got.SiteID, "SiteID is the room's site, preserved across federation") assert.False(t, got.HasMention) assert.True(t, got.CreatedAt.Equal(now)) assert.True(t, got.UpdatedAt.Equal(now)) @@ -246,10 +249,10 @@ func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t * handler := NewHandler(store) now := time.Date(2026, 4, 1, 12, 0, 0, 0, time.UTC) - // First event: HasMention=true. + // First event: HasMention=true. Subscription.SiteID is the room's site (site-a). mentionSub := model.ThreadSubscription{ ID: "sub-1", ParentMessageID: "pm-1", RoomID: "r1", ThreadRoomID: "tr-1", - UserID: "u-bob", UserAccount: "bob", SiteID: "site-b", + UserID: "u-bob", UserAccount: "bob", SiteID: "site-a", HasMention: true, CreatedAt: now, UpdatedAt: now, } mentionData, _ := json.Marshal(mentionSub) diff --git a/message-worker/handler.go b/message-worker/handler.go index 1890f7751..275d5d56f 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -137,19 +137,19 @@ func (h *Handler) handleThreadRoomAndSubscriptions(ctx context.Context, msg *mod err := h.threadStore.CreateThreadRoom(ctx, &threadRoom) switch { case err == nil: - return threadRoom.ID, h.handleFirstThreadReply(ctx, msg, threadRoom.ID, replier, now) + return threadRoom.ID, h.handleFirstThreadReply(ctx, msg, eventSiteID, threadRoom.ID, replier, now) case errors.Is(err, errThreadRoomExists): - return h.handleSubsequentThreadReply(ctx, msg, replier, now) + return h.handleSubsequentThreadReply(ctx, msg, eventSiteID, replier, now) default: return "", fmt.Errorf("create thread room: %w", err) } } // handleFirstThreadReply runs after the thread room has just been created. -// It inserts subscriptions for the parent author (looked up via userStore for -// the parent's home site) and, if distinct, for the replier (using the -// replier's home site). -func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message, threadRoomID string, replier *model.User, now time.Time) error { +// It inserts subscriptions for the parent author and (if distinct) the replier. +// Subscription.SiteID is the room's site (eventSiteID); the owner's home site +// is resolved separately and used only to decide cross-site outbox routing. +func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message, eventSiteID, threadRoomID string, replier *model.User, now time.Time) error { parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) if err != nil { if errors.Is(err, errMessageNotFound) { @@ -161,26 +161,26 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message return fmt.Errorf("get parent message sender: %w", err) } - parentSiteID, err := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") + parentOwnerSite, err := h.lookupOwnerSiteID(ctx, parentSender.ID, "first-reply parent") if err != nil { return fmt.Errorf("lookup parent owner site: %w", err) } - if parentSiteID != "" { - parentSub := h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, parentSiteID, now) + if parentOwnerSite != "" { + parentSub := h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, eventSiteID, now) if err := h.threadStore.InsertThreadSubscription(ctx, parentSub); err != nil { return fmt.Errorf("insert parent author thread subscription: %w", err) } - if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, msg.ID); err != nil { + if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, parentOwnerSite, msg.ID); err != nil { return fmt.Errorf("publish parent thread subscription outbox: %w", err) } } if replier != nil && msg.UserID != parentSender.ID { - replierSub := h.buildThreadSubscription(msg, threadRoomID, msg.UserID, msg.UserAccount, replier.SiteID, now) + replierSub := h.buildThreadSubscription(msg, threadRoomID, msg.UserID, msg.UserAccount, eventSiteID, now) if err := h.threadStore.InsertThreadSubscription(ctx, replierSub); err != nil { return fmt.Errorf("insert replier thread subscription: %w", err) } - if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, replier.SiteID, msg.ID); err != nil { return fmt.Errorf("publish replier thread subscription outbox: %w", err) } } @@ -200,7 +200,9 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message // Upserts subscriptions for both the parent author and the replier (idempotent // on redelivery), then bumps the room's last-message pointer. Returns the // existing thread room ID so the caller can pass it to SaveThreadMessage. -func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Message, replier *model.User, now time.Time) (string, error) { +// Subscription.SiteID is the room's site (eventSiteID); owner-site routing +// for the cross-site publish happens via separate lookups. +func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Message, eventSiteID string, replier *model.User, now time.Time) (string, error) { existingRoom, err := h.threadStore.GetThreadRoomByParentMessageID(ctx, msg.ThreadParentMessageID) if err != nil { return "", fmt.Errorf("get existing thread room: %w", err) @@ -210,25 +212,25 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me parentSender, err := h.store.GetMessageSender(ctx, msg.ThreadParentMessageID) switch { case err == nil: - parentSiteID, lookupErr := h.lookupOwnerSiteID(ctx, parentSender.ID, "subsequent-reply parent") + parentOwnerSite, lookupErr := h.lookupOwnerSiteID(ctx, parentSender.ID, "subsequent-reply parent") if lookupErr != nil { return "", fmt.Errorf("lookup parent owner site: %w", lookupErr) } - if parentSiteID != "" { - parentSub := h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, parentSiteID, now) + if parentOwnerSite != "" { + parentSub := h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, eventSiteID, now) if err := h.threadStore.UpsertThreadSubscription(ctx, parentSub); err != nil { return "", fmt.Errorf("upsert parent author thread subscription: %w", err) } - if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, msg.ID); err != nil { + if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, parentOwnerSite, msg.ID); err != nil { return "", fmt.Errorf("publish parent thread subscription outbox: %w", err) } } if replier != nil && msg.UserID != parentSender.ID { - replierSub := h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now) + replierSub := h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, eventSiteID, now) if err := h.threadStore.UpsertThreadSubscription(ctx, replierSub); err != nil { return "", fmt.Errorf("upsert replier thread subscription: %w", err) } - if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, replier.SiteID, msg.ID); err != nil { return "", fmt.Errorf("publish replier thread subscription outbox: %w", err) } } @@ -238,11 +240,11 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me "parentMessageID", msg.ThreadParentMessageID, "replyID", msg.ID) if replier != nil { - replierSub := h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, replier.SiteID, now) + replierSub := h.buildThreadSubscription(msg, existingRoom.ID, msg.UserID, msg.UserAccount, eventSiteID, now) if err := h.threadStore.UpsertThreadSubscription(ctx, replierSub); err != nil { return "", fmt.Errorf("upsert replier thread subscription: %w", err) } - if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, msg.ID); err != nil { + if err := h.publishThreadSubOutboxIfRemote(ctx, replierSub, replier.SiteID, msg.ID); err != nil { return "", fmt.Errorf("publish replier thread subscription outbox: %w", err) } } @@ -283,10 +285,12 @@ func (h *Handler) lookupOwnerSiteID(ctx context.Context, userID, role string) (s } // buildThreadSubscription constructs a ThreadSubscription for (threadRoomID, userID). -// ownerSiteID is the home site of the subscription's owner — NOT the room's site — -// so the document round-trips correctly through the OUTBOX/INBOX federation. +// siteID is the home site of the **room** that contains this thread — same +// semantic as Subscription.SiteID. The owner's home site is implicit (it's +// the site where the document is stored after federation); the cross-site +// publish decision is made separately by the caller. // lastSeenAt is always nil; the field is owned by user-action paths, not message-worker. -func (h *Handler) buildThreadSubscription(msg *model.Message, threadRoomID, userID, userAccount, ownerSiteID string, now time.Time) *model.ThreadSubscription { +func (h *Handler) buildThreadSubscription(msg *model.Message, threadRoomID, userID, userAccount, siteID string, now time.Time) *model.ThreadSubscription { return &model.ThreadSubscription{ ID: idgen.GenerateUUIDv7(), ParentMessageID: msg.ThreadParentMessageID, @@ -294,7 +298,7 @@ func (h *Handler) buildThreadSubscription(msg *model.Message, threadRoomID, user ThreadRoomID: threadRoomID, UserID: userID, UserAccount: userAccount, - SiteID: ownerSiteID, + SiteID: siteID, LastSeenAt: nil, CreatedAt: now, UpdatedAt: now, @@ -303,8 +307,9 @@ func (h *Handler) buildThreadSubscription(msg *model.Message, threadRoomID, user // markThreadMentions flips hasMention=true on the thread subscription of every // @account mentionee in msg (auto-creating the subscription if absent). The -// sender is excluded, and @all is ignored at the thread level. The -// subscription's SiteID is the mentionee's home site (carried on Participant). +// sender is excluded, and @all is ignored at the thread level. Subscription.SiteID +// is the room's site (eventSiteID); the mentionee's home site (Participant.SiteID) +// is used only for the cross-site outbox routing. func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, threadRoomID, eventSiteID string) error { for i := range msg.Mentions { p := &msg.Mentions[i] @@ -314,18 +319,12 @@ func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, th if p.UserID == msg.UserID { continue } - ownerSiteID := p.SiteID - if ownerSiteID == "" { - slog.Warn("mentionee participant has empty SiteID, falling back to event site", - "account", p.Account, "userID", p.UserID, "msgID", msg.ID) - ownerSiteID = eventSiteID - } - sub := h.buildThreadSubscription(msg, threadRoomID, p.UserID, p.Account, ownerSiteID, msg.CreatedAt) + sub := h.buildThreadSubscription(msg, threadRoomID, p.UserID, p.Account, eventSiteID, msg.CreatedAt) sub.HasMention = true if err := h.threadStore.MarkThreadSubscriptionMention(ctx, sub); err != nil { return fmt.Errorf("mark thread subscription mention for user %s: %w", p.UserID, err) } - if err := h.publishThreadSubOutboxIfRemote(ctx, sub, msg.ID); err != nil { + if err := h.publishThreadSubOutboxIfRemote(ctx, sub, p.SiteID, msg.ID); err != nil { return fmt.Errorf("publish thread mention outbox for user %s: %w", p.UserID, err) } } @@ -333,20 +332,22 @@ func (h *Handler) markThreadMentions(ctx context.Context, msg *model.Message, th } // publishThreadSubOutboxIfRemote publishes a thread_subscription_upserted -// outbox event when sub.SiteID is a remote site. Same-site or empty SiteID -// is a no-op (empty SiteID logs a warning — it indicates a caller bug). +// outbox event to ownerSiteID when that site differs from the local site. +// Same-site or empty ownerSiteID is a no-op (empty logs a warning — it +// indicates a caller bug). ownerSiteID is the subscription owner's home +// site — NOT sub.SiteID, which is the room's home site. // // The dedup-ID seed is (threadRoomID, userID, msgID): msg.ID is unique per // reply, and (msg.ID, userID) is unique within a reply, so the seed is stable // across MESSAGES_CANONICAL redeliveries and JetStream stream-level dedup // absorbs duplicates within the dedup window. -func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model.ThreadSubscription, msgID string) error { - if sub.SiteID == "" { - slog.Warn("thread subscription has empty SiteID, skipping outbox publish", +func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model.ThreadSubscription, ownerSiteID, msgID string) error { + if ownerSiteID == "" { + slog.Warn("owner siteID empty, skipping outbox publish", "threadRoomID", sub.ThreadRoomID, "userID", sub.UserID, "msgID", msgID) return nil } - if sub.SiteID == h.siteID { + if ownerSiteID == h.siteID { return nil } @@ -357,7 +358,7 @@ func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model outbox := model.OutboxEvent{ Type: model.OutboxThreadSubscriptionUpserted, SiteID: h.siteID, - DestSiteID: sub.SiteID, + DestSiteID: ownerSiteID, Payload: payload, Timestamp: time.Now().UTC().UnixMilli(), } @@ -366,10 +367,10 @@ func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model return fmt.Errorf("marshal outbox event: %w", err) } payloadSeed := fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID) - dedupID := outboxDedupID(ctx, sub.SiteID, payloadSeed) - subj := subject.Outbox(h.siteID, sub.SiteID, model.OutboxThreadSubscriptionUpserted) + dedupID := outboxDedupID(ctx, ownerSiteID, payloadSeed) + subj := subject.Outbox(h.siteID, ownerSiteID, model.OutboxThreadSubscriptionUpserted) if err := h.publish(ctx, subj, data, dedupID); err != nil { - return fmt.Errorf("publish thread subscription outbox to %s: %w", sub.SiteID, err) + return fmt.Errorf("publish thread subscription outbox to %s: %w", ownerSiteID, err) } return nil } diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 74171ee4c..a986f4952 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -1030,6 +1030,7 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { now := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + // Subscription's SiteID is the room's site (here, site-a — the local handler). baseSub := &model.ThreadSubscription{ ID: "sub-1", ParentMessageID: "pm-1", @@ -1037,7 +1038,7 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { ThreadRoomID: "tr-1", UserID: "u-bob", UserAccount: "bob", - SiteID: "site-b", + SiteID: "site-a", CreatedAt: now, UpdatedAt: now, } @@ -1045,18 +1046,18 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { t.Run("same site — no publish", func(t *testing.T) { ctrl := gomock.NewController(t) var called bool - h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-b", + h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { called = true return nil }) - err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "site-a", "msg-1") require.NoError(t, err) - assert.False(t, called, "publish must not be called when sub.SiteID == h.siteID") + assert.False(t, called, "publish must not be called when ownerSiteID == h.siteID") }) - t.Run("empty siteID — skip with warn, no publish", func(t *testing.T) { + t.Run("empty ownerSiteID — skip with warn, no publish", func(t *testing.T) { ctrl := gomock.NewController(t) var called bool h := NewHandler(NewMockStore(ctrl), NewMockUserStore(ctrl), NewMockThreadStore(ctrl), "site-a", @@ -1065,14 +1066,12 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { return nil }) - emptySub := *baseSub - emptySub.SiteID = "" - err := h.publishThreadSubOutboxIfRemote(context.Background(), &emptySub, "msg-1") + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "", "msg-1") require.NoError(t, err) - assert.False(t, called, "publish must not be called when sub.SiteID is empty") + assert.False(t, called, "publish must not be called when ownerSiteID is empty") }) - t.Run("remote site — publishes with expected subject and dedup ID", func(t *testing.T) { + t.Run("remote owner — publishes with expected subject and dedup ID", func(t *testing.T) { ctrl := gomock.NewController(t) var captured struct { subj string @@ -1089,7 +1088,7 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { return nil }) - err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "site-b", "msg-1") require.NoError(t, err) require.Equal(t, 1, captured.callCnt) assert.Equal(t, "outbox.site-a.to.site-b.thread_subscription_upserted", captured.subj) @@ -1102,7 +1101,7 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { second = msgID return nil }) - require.NoError(t, h2.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1")) + require.NoError(t, h2.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "site-b", "msg-1")) assert.Equal(t, captured.msgID, second, "dedup ID must be deterministic for the same (threadRoomID, userID, msgID) seed") // Different msgID → different dedup ID. @@ -1112,10 +1111,11 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { third = msgID return nil }) - require.NoError(t, h3.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-2")) + require.NoError(t, h3.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "site-b", "msg-2")) assert.NotEqual(t, captured.msgID, third) - // Payload is an OutboxEvent whose inner Payload decodes back to the ThreadSubscription. + // Payload is an OutboxEvent whose inner Payload decodes back to the ThreadSubscription + // — and the inner SiteID is unchanged (still the room's site, "site-a"). var outer model.OutboxEvent require.NoError(t, json.Unmarshal(captured.data, &outer)) assert.Equal(t, model.OutboxThreadSubscriptionUpserted, outer.Type) @@ -1126,6 +1126,7 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { var inner model.ThreadSubscription require.NoError(t, json.Unmarshal(outer.Payload, &inner)) assert.Equal(t, *baseSub, inner) + assert.Equal(t, "site-a", inner.SiteID, "inner SiteID stays as the room's site") }) t.Run("publish error returned", func(t *testing.T) { @@ -1136,7 +1137,7 @@ func TestHandler_PublishThreadSubOutboxIfRemote(t *testing.T) { return boom }) - err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "msg-1") + err := h.publishThreadSubOutboxIfRemote(context.Background(), baseSub, "site-b", "msg-1") require.Error(t, err) assert.ErrorIs(t, err, boom) }) @@ -1221,7 +1222,7 @@ func TestHandler_FirstReply_OutboxPublishes(t *testing.T) { ThreadParentMessageID: "msg-parent", } - err := h.handleFirstThreadReply(context.Background(), msg, "tr-1", replier, now) + err := h.handleFirstThreadReply(context.Background(), msg, "site-a", "tr-1", replier, now) require.NoError(t, err) gotByDest := map[string]int{} @@ -1259,7 +1260,7 @@ func TestHandler_FirstReply_OutboxPublishError_NAKs(t *testing.T) { ID: "msg-reply", RoomID: "r1", UserID: "u-replier", UserAccount: "replier", CreatedAt: now, ThreadParentMessageID: "msg-parent", } - err := h.handleFirstThreadReply(context.Background(), msg, + err := h.handleFirstThreadReply(context.Background(), msg, "site-a", "tr-1", &model.User{ID: "u-replier", SiteID: "site-b"}, now) require.Error(t, err) assert.ErrorIs(t, err, boom) @@ -1290,7 +1291,7 @@ func TestHandler_FirstReply_ReplierOutboxPublishError_NAKs(t *testing.T) { ID: "msg-reply", RoomID: "r1", UserID: "u-replier", UserAccount: "replier", CreatedAt: now, ThreadParentMessageID: "msg-parent", } - err := h.handleFirstThreadReply(context.Background(), msg, + err := h.handleFirstThreadReply(context.Background(), msg, "site-a", "tr-1", &model.User{ID: "u-replier", Account: "replier", SiteID: "site-b"}, now) require.Error(t, err) assert.ErrorIs(t, err, boom) @@ -1376,7 +1377,7 @@ func TestHandler_SubsequentReply_OutboxPublishes(t *testing.T) { ThreadParentMessageID: "msg-parent", } - roomID, err := h.handleSubsequentThreadReply(context.Background(), msg, replier, now) + roomID, err := h.handleSubsequentThreadReply(context.Background(), msg, "site-a", replier, now) require.NoError(t, err) assert.Equal(t, "tr-existing", roomID) @@ -1413,7 +1414,7 @@ func TestHandler_SubsequentReply_OutboxPublishError_NAKs(t *testing.T) { ID: "msg-reply", RoomID: "r1", UserID: "u-replier", UserAccount: "replier", CreatedAt: now, ThreadParentMessageID: "msg-parent", } - _, err := h.handleSubsequentThreadReply(context.Background(), msg, + _, err := h.handleSubsequentThreadReply(context.Background(), msg, "site-a", &model.User{ID: "u-replier", SiteID: "site-b"}, now) require.Error(t, err) assert.ErrorIs(t, err, boom) @@ -1566,7 +1567,7 @@ func TestHandler_MarkThreadMentions_HasMentionInPayload(t *testing.T) { require.NoError(t, json.Unmarshal(outer.Payload, &sub)) assert.True(t, sub.HasMention, "outbox-emitted ThreadSubscription must carry HasMention=true") assert.Equal(t, "u-bob", sub.UserID) - assert.Equal(t, "site-b", sub.SiteID) + assert.Equal(t, "site-a", sub.SiteID, "Subscription.SiteID is the room's site, not the mentionee's owner site") } // fakeJSMsg is a minimal jetstream.Msg test double that records whether Ack or diff --git a/pkg/model/threadsubscription.go b/pkg/model/threadsubscription.go index d6b0b0371..87eedeb65 100644 --- a/pkg/model/threadsubscription.go +++ b/pkg/model/threadsubscription.go @@ -9,7 +9,11 @@ type ThreadSubscription struct { ThreadRoomID string `json:"threadRoomId" bson:"threadRoomId"` UserID string `json:"userId" bson:"userId"` UserAccount string `json:"userAccount" bson:"userAccount"` - SiteID string `json:"siteId" bson:"siteId"` + // SiteID is the home site of the room that contains this thread — same + // semantic as Subscription.SiteID. Across cross-site federation it stays + // constant: every replica of a given subscription has the same SiteID + // regardless of which site stores the document. + SiteID string `json:"siteId" bson:"siteId"` // Never add omitempty: unreadThreadsPipeline relies on BSON encoding nil as explicit null, not a missing field. LastSeenAt *time.Time `json:"lastSeenAt" bson:"lastSeenAt"` HasMention bool `json:"hasMention" bson:"hasMention"` From 0c33b2371f427e127e1ecb52b13673613f843d15 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Apr 2026 05:38:50 +0000 Subject: [PATCH 26/32] docs: align spec/plan with post-review SiteID semantic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec doc rewritten in-place to describe the corrected design: ThreadSubscription.SiteID is the room's site (matching Subscription.SiteID), preserved across federation. Owner-site routing for the cross-site outbox publish is a separate, explicit ownerSiteID parameter to publishThreadSubOutboxIfRemote — not stored on the subscription. Plan doc gains a "Post-implementation correction" postscript pointing at commit 1c3915c that captures the divergence from the original Task 5 description. Plan body is left as-written for historical traceability. Skipping the optional rename to RoomSiteID per Subscription.SiteID's unprefixed convention; this is documented as a non-goal in the spec. --- ...ssage-worker-thread-subscription-outbox.md | 33 +++++ ...orker-thread-subscription-outbox-design.md | 123 +++++++++++------- 2 files changed, 112 insertions(+), 44 deletions(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index e8f4baa35..e947d034e 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -2112,3 +2112,36 @@ Plan complete and saved to `docs/superpowers/plans/2026-04-28-message-worker-thr 2. **Inline Execution** — Execute tasks in this session using `superpowers:executing-plans`, batch execution with checkpoints. Which approach? + +--- + +## Post-implementation correction (review feedback) + +After Task 5 landed, PR review (mliu33) flagged that `ThreadSubscription.SiteID` +should follow the same semantic as `Subscription.SiteID` — **the room's home +site**, a back-reference preserved across federation — not the owner's home +site. This plan as written above (Task 5 in particular) describes the +incorrect owner-site semantic. + +**Corrected design** is captured in the spec at +`docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md` +("`ThreadSubscription.SiteID` semantic" + "Outbox routing" sections). + +**Correction commit:** `1c3915c` — "ThreadSubscription.SiteID stays as room +site; route by owner site separately". Summary of differences vs. the plan +above: + +- `buildThreadSubscription` is called with the **room's** site at all three + call sites; `SiteID` field on the persisted document is the room's site. +- `publishThreadSubOutboxIfRemote` gained an explicit `ownerSiteID` parameter + used only for the cross-site routing decision (`DestSiteID` + same-site + short-circuit). Owner-site is resolved transiently at processing time — + `replier.SiteID`, `lookupOwnerSiteID(parentSender.ID)`, or `Participant.SiteID` + — and never written onto the subscription. +- `handleFirstThreadReply` / `handleSubsequentThreadReply` gain `eventSiteID` + arg; `markThreadMentions` no longer needs the empty-SiteID fallback (the + field on the sub is always the room's site). +- Tests assert `sub.SiteID == roomSite` after round-trip, not `ownerSite`. + +The historical plan above stays as-written for traceability of how the work +unfolded. New readers should treat the spec as the canonical design. diff --git a/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md b/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md index 974d0eb5d..99b23a39e 100644 --- a/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md +++ b/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md @@ -35,6 +35,9 @@ treatment, including the `hasMention=true` flag. current code; that path is out of scope. - **`@all` propagation.** `@all` is already filtered out at the thread level (see `markThreadMentions`), so it never produces an outbox event. +- **Renaming `ThreadSubscription.SiteID` → `RoomSiteID`.** Considered during + review but skipped — `Subscription.SiteID` is unprefixed and the two should + stay consistent. The doc comment carries the semantic. ## Design @@ -58,7 +61,7 @@ type ThreadSubscription struct { ThreadRoomID string UserID string UserAccount string - SiteID string // owner's home site (see Semantic change below) + SiteID string // room's home site (see SiteID semantic below) LastSeenAt *time.Time // always nil from message-worker HasMention bool // true only on mention-marked events CreatedAt time.Time @@ -66,21 +69,45 @@ type ThreadSubscription struct { } ``` -The destination site of the outbox is the user's home site. One outbox event -per (reply, affected user) tuple. +The destination site of the outbox is the **owner's** home site, resolved +transiently at processing time and passed as a separate routing argument +(see "Outbox routing" below). One outbox event per (reply, affected user) tuple. -### Semantic change to `ThreadSubscription.SiteID` +### `ThreadSubscription.SiteID` semantic -Today `buildThreadSubscription` sets `SiteID = ` — i.e. -the room's home site. For subscriptions to round-trip across sites cleanly, the -field must reflect the **owner's** home site. The room context is already -preserved by `RoomID`. This brings `ThreadSubscription.SiteID` in line with -`Subscription.SiteID` and matches what inbox-worker stores when it consumes the -event. +`SiteID` is the **room's** home site — the same semantic as `Subscription.SiteID` +in `pkg/model/subscription.go`. It is a back-reference to where the thread +room originated, not a self-identifier of the owner. Across cross-site +federation the field is constant: every replica of a given subscription has +the same `SiteID`, regardless of which site stores the document. The owner's +site is implicit (it's the site where the document lives after federation). -Callers that don't currently have the owner's siteID in scope (the `markThreadMentions` -loop, parent author lookup) gain a small lookup or piggyback on existing `User` -data — see "Implementation" below. +The owner's site information is needed at message-worker processing time to +decide whether to publish a cross-site outbox event and where to route it, +but is **not** stored on the subscription. It is resolved transiently: + +- **Replier:** `replier.SiteID` from the `*model.User` already looked up earlier + in `processMessage` (the message sender). +- **Parent author:** `userStore.FindUserByID(parentSender.ID)` — a new call + introduced by `lookupOwnerSiteID` (warn-and-skip on `userstore.ErrUserNotFound`, + propagate other DB errors). +- **Mentionees:** `Participant.SiteID`, populated by `mention.Resolve` from the + underlying `User` lookup (this spec adds the `SiteID` field to `Participant`). + +### Outbox routing + +`publishThreadSubOutboxIfRemote(ctx, sub, ownerSiteID, msgID)` decides whether +to publish based on the explicit `ownerSiteID` parameter, not on `sub.SiteID`: + +- `ownerSiteID == ""` → log warn, skip (defensive — caller bug). +- `ownerSiteID == h.siteID` → no-op (subscription owner is on the local site). +- otherwise → marshal sub → wrap in `OutboxEvent{DestSiteID: ownerSiteID, ...}` + → publish to `subject.Outbox(h.siteID, ownerSiteID, "thread_subscription_upserted")`. + +The published payload carries `sub.SiteID = roomSiteID` unchanged. When the +destination site's inbox-worker upserts it locally, the resulting document has +`SiteID = roomSiteID` — preserved across federation, identical to how +`Subscription.SiteID` round-trips through OUTBOX/INBOX today. ### Subject + dedup @@ -187,39 +214,41 @@ type PublishFunc func(ctx context.Context, subj string, data []byte, msgID strin that calls `js.Publish(ctx, subj, data, jetstream.WithMsgID(msgID))` exactly as room-worker does. -`buildThreadSubscription` keeps its parameter list but the `siteID` argument -now means "**owner's** site", not the room's. The three callers update: - -1. **handleFirstThreadReply** — replier: pass the replier's `User.SiteID` (we - already have `User` from `processMessage`'s `userStore.FindUserByID`). Parent: - look up `userStore.FindUserByID(parentSender.ID)` to get parent's siteID. -2. **handleSubsequentThreadReply** — same as above. -3. **markThreadMentions** — mentionees: `mention.Resolve` returns - `[]Participant`, which today doesn't carry `SiteID`. Verified: - `mention.Resolve` already calls `LookupFunc` (= `userStore.FindUsersByAccounts`), - and that store query already projects `siteId`. So the `User` slice inside - `Resolve` already has the data — we just don't propagate it onto - `Participant`. - - Add a `SiteID string \`json:"siteId,omitempty" bson:"siteId,omitempty"\`` - field to `model.Participant` and populate it in `mention.Resolve` from - `users[i].SiteID`. The new field is `omitempty` and additive — existing - consumers that JSON-encode `Participant` (e.g. on `Message.Mentions`) emit - the extra field but otherwise behave identically. Update - `pkg/model/model_test.go` to round-trip `SiteID` on `Participant`. +`buildThreadSubscription` is called with the **room's** site at every call +site (`SiteID: eventSiteID`), matching `Subscription.SiteID` semantics. The +three callers compute the owner's site separately and pass it to the publish +helper: + +1. **handleFirstThreadReply** — replier: `replier.SiteID` from the `*model.User` + already in scope (the message sender). Parent: `lookupOwnerSiteID(parentSender.ID)` + resolves the parent's home site via `userStore.FindUserByID`, returning + `("", nil)` on `userstore.ErrUserNotFound` (warn-and-skip, parallels the + `errMessageNotFound` branch). +2. **handleSubsequentThreadReply** — same as above on the upsert path. +3. **markThreadMentions** — mentionees: `Participant.SiteID`, populated by + `mention.Resolve`. Verified: `mention.Resolve` already calls `LookupFunc` + (= `userStore.FindUsersByAccounts`) which projects `siteId`, so the `User` + slice inside `Resolve` already has the data. We add a + `SiteID string \`json:"siteId,omitempty" bson:"siteId,omitempty"\`` field + to `model.Participant` and populate it in the resolver. The field is + `omitempty` and additive — existing JSON consumers decode unchanged. After every `InsertThreadSubscription` / `UpsertThreadSubscription` / `MarkThreadSubscriptionMention` succeeds, call: ```go -h.publishThreadSubOutboxIfRemote(ctx, sub, msg.ID) +h.publishThreadSubOutboxIfRemote(ctx, sub, ownerSiteID, msg.ID) ``` The helper: ```go -func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model.ThreadSubscription, msgID string) error { - if sub.SiteID == h.siteID { +func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model.ThreadSubscription, ownerSiteID, msgID string) error { + if ownerSiteID == "" { + slog.Warn("owner siteID empty, skipping outbox publish", ...) + return nil + } + if ownerSiteID == h.siteID { return nil } payload, err := json.Marshal(sub) @@ -229,7 +258,7 @@ func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model outbox := model.OutboxEvent{ Type: model.OutboxThreadSubscriptionUpserted, SiteID: h.siteID, - DestSiteID: sub.SiteID, + DestSiteID: ownerSiteID, Payload: payload, Timestamp: time.Now().UTC().UnixMilli(), } @@ -237,14 +266,19 @@ func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model if err != nil { return fmt.Errorf("marshal outbox event: %w", err) } - dedupID := idgen.DeriveID(fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID)) - if err := h.publish(ctx, subject.Outbox(h.siteID, sub.SiteID, model.OutboxThreadSubscriptionUpserted), data, dedupID); err != nil { - return fmt.Errorf("publish thread subscription outbox to %s: %w", sub.SiteID, err) + payloadSeed := fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID) + dedupID := outboxDedupID(ctx, ownerSiteID, payloadSeed) + if err := h.publish(ctx, subject.Outbox(h.siteID, ownerSiteID, model.OutboxThreadSubscriptionUpserted), data, dedupID); err != nil { + return fmt.Errorf("publish thread subscription outbox to %s: %w", ownerSiteID, err) } return nil } ``` +Note: `sub.SiteID` (the room's site) stays as-is in the published payload — +the destination site's inbox-worker stores the same `SiteID` value, so all +replicas of a subscription carry the same room-site identity. + Errors propagate to `processMessage`, which propagates to `HandleJetStreamMsg`, which NAKs. @@ -339,10 +373,11 @@ sites, and the `hasMention` OR-merge. data because `processMessage` re-runs `mention.Resolve` on every delivery, re-populating `Mentions` from the live userstore. Old persisted `Mentions` arrays in Cassandra are never re-emitted as outbox events. -- **Defensive guard against empty `SiteID`.** The publish helper must skip + - log warn if `sub.SiteID == ""`. This prevents an upstream bug (e.g., a future - caller forgetting to set the field) from emitting an outbox to a `dest=""` - subject. The empty case is otherwise unreachable in the paths added here. +- **Defensive guard against empty `ownerSiteID`.** The publish helper must skip + + log warn if the `ownerSiteID` argument is empty. This prevents an upstream bug + (e.g., a future caller forgetting to resolve the owner's site) from emitting + an outbox to a `dest=""` subject. The empty case is otherwise unreachable in + the paths added here. - **Parent user lookup may fail.** `GetMessageSender` already handles `errMessageNotFound` for the parent **message**. Adding `userStore.FindUserByID(parentSender.ID)` introduces a new "parent user From 0cfddda095877c6b6171322dbf98209f1dcc40d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Apr 2026 05:51:10 +0000 Subject: [PATCH 27/32] message-worker: fix outbox dedup-ID collision + don't drop parent sub on missing owner-site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bug fixes from PR review (CodeRabbit): 1. outboxDedupID was returning requestID + ":" + destSiteID and discarding payloadSeed when an X-Request-ID was present in context. A single thread reply that publishes for parent + replier (or multiple mentionees) on the same destination site got identical dedup IDs, causing JetStream to dedup away all but the first publish. Use payloadSeed (which already contains threadRoomID + userID + msg.ID — stable across redeliveries and unique per publish) directly: "{payloadSeed}:{destSiteID}". 2. handleFirstThreadReply / handleSubsequentThreadReply skipped the local parent Insert/Upsert entirely when lookupOwnerSiteID returned ("", nil) on userstore.ErrUserNotFound. The local subscription doesn't depend on knowing the owner's site (SiteID is the room's site, which we have via eventSiteID, and parentSender already supplies userID + account). Only the cross-site outbox publish needs the owner site. Always persist the parent subscription locally; gate only the outbox call. Tests updated: the two warn-and-skip cases now assert the parent Insert/Upsert still runs while the outbox publish is skipped. --- message-worker/handler.go | 38 +++++++++++++++++++--------------- message-worker/handler_test.go | 22 +++++++++++++++----- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/message-worker/handler.go b/message-worker/handler.go index 275d5d56f..40e6da23e 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -165,11 +165,14 @@ func (h *Handler) handleFirstThreadReply(ctx context.Context, msg *model.Message if err != nil { return fmt.Errorf("lookup parent owner site: %w", err) } + parentSub := h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, eventSiteID, now) + if err := h.threadStore.InsertThreadSubscription(ctx, parentSub); err != nil { + return fmt.Errorf("insert parent author thread subscription: %w", err) + } + // Outbox publish is gated on parentOwnerSite — if the parent user is missing + // from userStore, we can't route the cross-site copy, but the local Insert + // above is independent of that and still happens. if parentOwnerSite != "" { - parentSub := h.buildThreadSubscription(msg, threadRoomID, parentSender.ID, parentSender.Account, eventSiteID, now) - if err := h.threadStore.InsertThreadSubscription(ctx, parentSub); err != nil { - return fmt.Errorf("insert parent author thread subscription: %w", err) - } if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, parentOwnerSite, msg.ID); err != nil { return fmt.Errorf("publish parent thread subscription outbox: %w", err) } @@ -216,11 +219,11 @@ func (h *Handler) handleSubsequentThreadReply(ctx context.Context, msg *model.Me if lookupErr != nil { return "", fmt.Errorf("lookup parent owner site: %w", lookupErr) } + parentSub := h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, eventSiteID, now) + if err := h.threadStore.UpsertThreadSubscription(ctx, parentSub); err != nil { + return "", fmt.Errorf("upsert parent author thread subscription: %w", err) + } if parentOwnerSite != "" { - parentSub := h.buildThreadSubscription(msg, existingRoom.ID, parentSender.ID, parentSender.Account, eventSiteID, now) - if err := h.threadStore.UpsertThreadSubscription(ctx, parentSub); err != nil { - return "", fmt.Errorf("upsert parent author thread subscription: %w", err) - } if err := h.publishThreadSubOutboxIfRemote(ctx, parentSub, parentOwnerSite, msg.ID); err != nil { return "", fmt.Errorf("publish parent thread subscription outbox: %w", err) } @@ -375,16 +378,17 @@ func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model return nil } -// outboxDedupID composes Nats-Msg-Id as base+":"+destSiteID; base is X-Request-ID -// from ctx, falling back to payloadSeed when absent (partial-deployment safety). -// Mirrors room-worker's helper of the same shape so cross-site dedup IDs are -// consistent across services that publish to OUTBOX. +// outboxDedupID composes Nats-Msg-Id as `{payloadSeed}:{destSiteID}`. The +// payloadSeed must already encode per-publish uniqueness (here: +// threadRoomID + userID + msg.ID) so that multiple subscriptions published +// to the same destination from a single inbound message get distinct dedup +// IDs. msg.ID is stable across MESSAGES_CANONICAL redeliveries, making the +// dedup ID stable too. X-Request-ID isn't part of the result — payloadSeed +// already gives per-publish entropy and per-redelivery stability. func outboxDedupID(ctx context.Context, destSiteID, payloadSeed string) string { - base := natsutil.RequestIDFromContext(ctx) - if base == "" { - slog.Warn("missing X-Request-ID; falling back to payload-derived outbox dedup base", + if natsutil.RequestIDFromContext(ctx) == "" { + slog.Warn("missing X-Request-ID; using payload-derived outbox dedup id", "destSiteID", destSiteID) - base = payloadSeed } - return base + ":" + destSiteID + return payloadSeed + ":" + destSiteID } diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index a986f4952..8118dd65f 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -926,12 +926,19 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { }, }, { - name: "first reply — parent user not found in userStore — warn-and-skip parent, still inserts replier", + name: "first reply — parent user not found in userStore — still inserts parent + replier locally, skips outbox", msg: msg, siteID: "site-a", setupMocks: func(store *MockStore, ts *MockThreadStore) { ts.EXPECT().CreateThreadRoom(gomock.Any(), gomock.Any()).Return(nil) store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent").Return(parentSender, nil) + // Parent insert still runs (independent of owner-site lookup) — + // only the cross-site outbox publish is gated on the lookup. + ts.EXPECT().InsertThreadSubscription(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, sub *model.ThreadSubscription) error { + assert.Equal(t, "u-parent", sub.UserID, "parent insert must still happen on missing owner-site") + return nil + }) }, extraUserStoreSetup: func(us *MockUserStore) { us.EXPECT().FindUserByID(gomock.Any(), "u-parent"). @@ -940,7 +947,7 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { expectReplierInsert: true, }, { - name: "subsequent reply — parent user not found in userStore — warn-and-skip parent, still upserts replier", + name: "subsequent reply — parent user not found in userStore — still upserts parent + replier locally, skips parent outbox", msg: msg, siteID: "site-a", setupMocks: func(store *MockStore, ts *MockThreadStore) { @@ -950,11 +957,16 @@ func TestHandler_HandleThreadRoomAndSubscriptions(t *testing.T) { Return(&model.ThreadRoom{ID: "tr-existing"}, nil) store.EXPECT().GetMessageSender(gomock.Any(), "msg-parent"). Return(parentSender, nil) - // Parent upsert SKIPPED because lookupOwnerSiteID returns ("", nil). - // Replier upsert still runs. + // Parent upsert still runs (independent of owner-site lookup); + // only the cross-site outbox publish is gated. + ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, sub *model.ThreadSubscription) error { + assert.Equal(t, "u-parent", sub.UserID) + return nil + }) ts.EXPECT().UpsertThreadSubscription(gomock.Any(), gomock.Any()). DoAndReturn(func(_ context.Context, sub *model.ThreadSubscription) error { - assert.Equal(t, "u-replier", sub.UserID, "only replier should be upserted; parent skipped") + assert.Equal(t, "u-replier", sub.UserID) return nil }) ts.EXPECT().UpdateThreadRoomLastMessage(gomock.Any(), "tr-existing", "msg-reply", "replier", now). From 2cfc181dda624d134e0ae14699864083c95d3320 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Apr 2026 05:51:56 +0000 Subject: [PATCH 28/32] =?UTF-8?q?docs:=20address=20CodeRabbit=20doc=20nits?= =?UTF-8?q?=20=E2=80=94=20code-fence=20langs,=20\$max=20example,=20plan=20?= =?UTF-8?q?banner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec: add language tags to two markdown fences (MD040). Replace the \$bit example with the implemented \$max operator and explain why \$max on a Go bool field works (BSON encodes false=0x00 < true=0x01 → monotonic merge). Plan: add a top-of-document warning right after the Goal line pointing at the post-implementation correction section + the canonical spec, so readers who follow the linear task list don't first encounter (and act on) the incorrect Task 5 SiteID semantic. --- ...-message-worker-thread-subscription-outbox.md | 11 +++++++++++ ...e-worker-thread-subscription-outbox-design.md | 16 ++++++++-------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md index e947d034e..5dcd119dc 100644 --- a/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md +++ b/docs/superpowers/plans/2026-04-28-message-worker-thread-subscription-outbox.md @@ -4,6 +4,17 @@ **Goal:** Replicate `ThreadSubscription` writes (parent author, replier, mentionees) from message-worker on the room's home site to each affected user's home site via the existing OUTBOX/INBOX federation. +> **⚠️ Post-implementation correction.** Task 5 below describes +> `ThreadSubscription.SiteID` as the **owner's** home site. That is **wrong**: +> `SiteID` is the **room's** home site (matching `Subscription.SiteID`), +> preserved unchanged across federation. Owner-site routing is handled +> transiently via an explicit `ownerSiteID` argument to +> `publishThreadSubOutboxIfRemote` — never written onto the subscription. +> Plan body is preserved as-written for traceability; canonical design lives +> in [`../specs/2026-04-28-message-worker-thread-subscription-outbox-design.md`](../specs/2026-04-28-message-worker-thread-subscription-outbox-design.md). +> Correction commit: `1c3915c`. See "Post-implementation correction" section +> at the end for the full diff summary. + **Architecture:** Mirror room-worker's pattern. Message-worker gains a `publish PublishFunc` field; after every local Insert/Upsert/MarkThreadSubscriptionMention it emits a `thread_subscription_upserted` outbox event when the subscription owner's site differs from the room's site. Inbox-worker dispatches the new event type to a new `UpsertThreadSubscription` store method that uses MongoDB `$max` on `hasMention` for monotonic mention-flag merge. **Tech Stack:** Go 1.25, NATS JetStream (`github.com/nats-io/nats.go/jetstream`), MongoDB (`go.mongodb.org/mongo-driver/v2`), `github.com/hmchangw/chat/pkg/{model,subject,idgen,mention,userstore,stream}`, `go.uber.org/mock`, `stretchr/testify`, `testcontainers-go`. diff --git a/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md b/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md index 99b23a39e..141935e17 100644 --- a/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md +++ b/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md @@ -45,7 +45,7 @@ treatment, including the `hasMention=true` flag. A single new outbox event type: -``` +```go const OutboxThreadSubscriptionUpserted OutboxEventType = "thread_subscription_upserted" ``` @@ -143,21 +143,21 @@ The handler unmarshals `evt.Payload` into a `ThreadSubscription` and calls The Mongo implementation: -``` +```yaml filter: { threadRoomId: sub.ThreadRoomID, userId: sub.UserID } update: $setOnInsert: { _id, parentMessageId, roomId, threadRoomId, userId, userAccount, siteId, createdAt, lastSeenAt: null } $set: { updatedAt } - $bit: { hasMention: { or: <1 if sub.HasMention else 0> } } + $max: { hasMention: sub.HasMention } opts: upsert: true ``` -The `$bit:or` (or equivalent `$max` on a 0/1 int) makes the merge monotonic: -once `hasMention` flips true at the destination, no later non-mention event can -clear it. `_id` and `createdAt` come from `$setOnInsert` so the first event to -land defines them; later events update only `updatedAt` (and possibly the -mention bit). +`$max` on a `bool` field makes the merge monotonic: BSON encodes +`false (0x00) < true (0x01)`, so `$max(existing, incoming)` only ever flips +`hasMention` from false→true and never clears a prior `true`. `_id` and +`createdAt` come from `$setOnInsert` so the first event to land defines +them; later events update only `updatedAt` (and possibly the mention bit). `LastSeenAt` is never written by inbox-worker (it's a per-user-action field owned by whatever path lets a user mark a thread as seen — out of scope here). From 06cd97fdcd4ccd06dcf0eddc97004e673f0e15e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Apr 2026 05:54:00 +0000 Subject: [PATCH 29/32] style: wrap publish-closure errors + assert on json.Marshal in integration tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit nitpicks (CLAUDE.md compliance): - message-worker/main.go: wrap nc.Publish / js.Publish errors from the injected publish closure with fmt.Errorf so callers get descriptive diagnostics ("publish nats message to " / "publish jetstream message to with msgID "). - inbox-worker/integration_test.go: capture errors from json.Marshal in our 8 new thread-subscription test fixture builds and assert with require.NoError instead of "_, := json.Marshal(...)" — fail fast on fixture construction errors. Pre-existing _ := json.Marshal sites in unrelated test cases left as-is. --- inbox-worker/integration_test.go | 34 +++++++++++++++++++------------- message-worker/main.go | 11 ++++++++--- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/inbox-worker/integration_test.go b/inbox-worker/integration_test.go index 30abfea1d..d8c331a7f 100644 --- a/inbox-worker/integration_test.go +++ b/inbox-worker/integration_test.go @@ -214,19 +214,20 @@ func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) UserID: "u-bob", UserAccount: "bob", SiteID: "site-a", HasMention: false, CreatedAt: now, UpdatedAt: now, } - subData, _ := json.Marshal(sub) - evtData, _ := json.Marshal(model.OutboxEvent{ + subData, err := json.Marshal(sub) + require.NoError(t, err) + evtData, err := json.Marshal(model.OutboxEvent{ Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", Payload: subData, Timestamp: now.UnixMilli(), }) + require.NoError(t, err) require.NoError(t, handler.HandleEvent(ctx, evtData)) var got model.ThreadSubscription - err := db.Collection("threadSubscriptions"). + require.NoError(t, db.Collection("threadSubscriptions"). FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). - Decode(&got) - require.NoError(t, err) + Decode(&got)) assert.Equal(t, "sub-1", got.ID) assert.Equal(t, "site-a", got.SiteID, "SiteID is the room's site, preserved across federation") assert.False(t, got.HasMention) @@ -255,11 +256,13 @@ func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t * UserID: "u-bob", UserAccount: "bob", SiteID: "site-a", HasMention: true, CreatedAt: now, UpdatedAt: now, } - mentionData, _ := json.Marshal(mentionSub) - mentionEvt, _ := json.Marshal(model.OutboxEvent{ + mentionData, err := json.Marshal(mentionSub) + require.NoError(t, err) + mentionEvt, err := json.Marshal(model.OutboxEvent{ Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", Payload: mentionData, Timestamp: now.UnixMilli(), }) + require.NoError(t, err) require.NoError(t, handler.HandleEvent(ctx, mentionEvt)) // Second event: HasMention=false (later updatedAt). Must NOT clear the flag. @@ -267,18 +270,19 @@ func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t * plainSub.HasMention = false later := now.Add(time.Minute) plainSub.UpdatedAt = later - plainData, _ := json.Marshal(plainSub) - plainEvt, _ := json.Marshal(model.OutboxEvent{ + plainData, err := json.Marshal(plainSub) + require.NoError(t, err) + plainEvt, err := json.Marshal(model.OutboxEvent{ Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", Payload: plainData, Timestamp: later.UnixMilli(), }) + require.NoError(t, err) require.NoError(t, handler.HandleEvent(ctx, plainEvt)) var got model.ThreadSubscription - err := db.Collection("threadSubscriptions"). + require.NoError(t, db.Collection("threadSubscriptions"). FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). - Decode(&got) - require.NoError(t, err) + Decode(&got)) assert.True(t, got.HasMention, "hasMention must remain true after a non-mention event") assert.True(t, got.UpdatedAt.Equal(later), "updatedAt must advance to the later event's value") // _id and createdAt come from $setOnInsert and must remain from the first event. @@ -290,11 +294,13 @@ func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t * thirdSub.HasMention = true evenLater := later.Add(time.Minute) thirdSub.UpdatedAt = evenLater - thirdData, _ := json.Marshal(thirdSub) - thirdEvt, _ := json.Marshal(model.OutboxEvent{ + thirdData, err := json.Marshal(thirdSub) + require.NoError(t, err) + thirdEvt, err := json.Marshal(model.OutboxEvent{ Type: "thread_subscription_upserted", SiteID: "site-a", DestSiteID: "site-b", Payload: thirdData, Timestamp: evenLater.UnixMilli(), }) + require.NoError(t, err) require.NoError(t, handler.HandleEvent(ctx, thirdEvt)) require.NoError(t, db.Collection("threadSubscriptions"). diff --git a/message-worker/main.go b/message-worker/main.go index 50d4c627e..31845f591 100644 --- a/message-worker/main.go +++ b/message-worker/main.go @@ -94,10 +94,15 @@ func main() { } handler := NewHandler(store, us, threadStore, cfg.SiteID, func(ctx context.Context, subj string, data []byte, msgID string) error { if msgID == "" { - return nc.Publish(ctx, subj, data) + if err := nc.Publish(ctx, subj, data); err != nil { + return fmt.Errorf("publish nats message to %s: %w", subj, err) + } + return nil + } + if _, err := js.Publish(ctx, subj, data, jetstream.WithMsgID(msgID)); err != nil { + return fmt.Errorf("publish jetstream message to %s with msgID %s: %w", subj, msgID, err) } - _, err := js.Publish(ctx, subj, data, jetstream.WithMsgID(msgID)) - return err + return nil }) if err := bootstrapStreams(ctx, js, cfg.SiteID, cfg.Bootstrap.Enabled); err != nil { From 15f233b66d47e406ec840bbec24e5653887d95dc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 4 May 2026 03:05:10 +0000 Subject: [PATCH 30/32] =?UTF-8?q?inbox-worker:=20collection=20name=20?= =?UTF-8?q?=E2=86=92=20thread=5Fsubscriptions;=20inline=20outbox=20dedup?= =?UTF-8?q?=20ID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit + mliu33 review: - Inbox-worker was passing "threadSubscriptions" (camelCase) to db.Collection(...). The canonical collection-name string elsewhere in the codebase is "thread_subscriptions" (snake_case): message-worker/ store_mongo.go:32, all 11 sites in message-worker/integration_test.go, and history-service/internal/mongorepo/threadroom.go:28. This PR was splitting federated thread subscriptions into a separate collection invisible to message-worker reads. Fix: rename the collection string in inbox-worker/main.go and 5 sites in inbox-worker/integration_test.go. The Go struct field name (threadSubCol) is unchanged. - outboxDedupID's X-Request-ID branch was dead — the value was warned about but not used in the result. Inline the dedup-ID derivation into publishThreadSubOutboxIfRemote (single call site) and drop the helper + the now-unused natsutil import. Format unchanged: thread-sub-outbox::::. --- inbox-worker/integration_test.go | 10 +++++----- inbox-worker/main.go | 4 ++-- message-worker/handler.go | 24 ++++++------------------ 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/inbox-worker/integration_test.go b/inbox-worker/integration_test.go index d8c331a7f..575750e9e 100644 --- a/inbox-worker/integration_test.go +++ b/inbox-worker/integration_test.go @@ -199,7 +199,7 @@ func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) subCol: db.Collection("subscriptions"), roomCol: db.Collection("rooms"), userCol: db.Collection("users"), - threadSubCol: db.Collection("threadSubscriptions"), + threadSubCol: db.Collection("thread_subscriptions"), } require.NoError(t, store.ensureIndexes(ctx)) @@ -225,7 +225,7 @@ func TestInboxWorker_ThreadSubscriptionUpserted_Insert_Integration(t *testing.T) require.NoError(t, handler.HandleEvent(ctx, evtData)) var got model.ThreadSubscription - require.NoError(t, db.Collection("threadSubscriptions"). + require.NoError(t, db.Collection("thread_subscriptions"). FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). Decode(&got)) assert.Equal(t, "sub-1", got.ID) @@ -243,7 +243,7 @@ func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t * subCol: db.Collection("subscriptions"), roomCol: db.Collection("rooms"), userCol: db.Collection("users"), - threadSubCol: db.Collection("threadSubscriptions"), + threadSubCol: db.Collection("thread_subscriptions"), } require.NoError(t, store.ensureIndexes(ctx)) @@ -280,7 +280,7 @@ func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t * require.NoError(t, handler.HandleEvent(ctx, plainEvt)) var got model.ThreadSubscription - require.NoError(t, db.Collection("threadSubscriptions"). + require.NoError(t, db.Collection("thread_subscriptions"). FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). Decode(&got)) assert.True(t, got.HasMention, "hasMention must remain true after a non-mention event") @@ -303,7 +303,7 @@ func TestInboxWorker_ThreadSubscriptionUpserted_MonotonicMention_Integration(t * require.NoError(t, err) require.NoError(t, handler.HandleEvent(ctx, thirdEvt)) - require.NoError(t, db.Collection("threadSubscriptions"). + require.NoError(t, db.Collection("thread_subscriptions"). FindOne(ctx, bson.M{"threadRoomId": "tr-1", "userId": "u-bob"}). Decode(&got)) assert.True(t, got.HasMention) diff --git a/inbox-worker/main.go b/inbox-worker/main.go index 51563054d..295437bed 100644 --- a/inbox-worker/main.go +++ b/inbox-worker/main.go @@ -116,7 +116,7 @@ func (s *mongoInboxStore) ensureIndexes(ctx context.Context) error { Keys: bson.D{{Key: "threadRoomId", Value: 1}, {Key: "userId", Value: 1}}, Options: options.Index().SetUnique(true), }); err != nil { - return fmt.Errorf("ensure threadSubscriptions (threadRoomId,userId) index: %w", err) + return fmt.Errorf("ensure thread_subscriptions (threadRoomId,userId) index: %w", err) } return nil } @@ -184,7 +184,7 @@ func main() { subCol: db.Collection("subscriptions"), roomCol: db.Collection("rooms"), userCol: db.Collection("users"), - threadSubCol: db.Collection("threadSubscriptions"), + threadSubCol: db.Collection("thread_subscriptions"), } if err := store.ensureIndexes(ctx); err != nil { slog.Error("ensure indexes failed", "error", err) diff --git a/message-worker/handler.go b/message-worker/handler.go index 40e6da23e..ef2f5916c 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -13,7 +13,6 @@ import ( "github.com/hmchangw/chat/pkg/idgen" "github.com/hmchangw/chat/pkg/mention" "github.com/hmchangw/chat/pkg/model" - "github.com/hmchangw/chat/pkg/natsutil" "github.com/hmchangw/chat/pkg/subject" "github.com/hmchangw/chat/pkg/userstore" ) @@ -369,26 +368,15 @@ func (h *Handler) publishThreadSubOutboxIfRemote(ctx context.Context, sub *model if err != nil { return fmt.Errorf("marshal outbox event: %w", err) } - payloadSeed := fmt.Sprintf("thread-sub-outbox:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID) - dedupID := outboxDedupID(ctx, ownerSiteID, payloadSeed) + // Dedup ID format: {payloadSeed}:{destSiteID}, where payloadSeed encodes + // per-publish uniqueness (threadRoomID + userID + msg.ID). msg.ID is + // stable across MESSAGES_CANONICAL redeliveries → same publish always + // produces the same dedup ID. Different users on the same destination get + // different dedup IDs because their userIDs differ in the seed. + dedupID := fmt.Sprintf("thread-sub-outbox:%s:%s:%s:%s", sub.ThreadRoomID, sub.UserID, msgID, ownerSiteID) subj := subject.Outbox(h.siteID, ownerSiteID, model.OutboxThreadSubscriptionUpserted) if err := h.publish(ctx, subj, data, dedupID); err != nil { return fmt.Errorf("publish thread subscription outbox to %s: %w", ownerSiteID, err) } return nil } - -// outboxDedupID composes Nats-Msg-Id as `{payloadSeed}:{destSiteID}`. The -// payloadSeed must already encode per-publish uniqueness (here: -// threadRoomID + userID + msg.ID) so that multiple subscriptions published -// to the same destination from a single inbound message get distinct dedup -// IDs. msg.ID is stable across MESSAGES_CANONICAL redeliveries, making the -// dedup ID stable too. X-Request-ID isn't part of the result — payloadSeed -// already gives per-publish entropy and per-redelivery stability. -func outboxDedupID(ctx context.Context, destSiteID, payloadSeed string) string { - if natsutil.RequestIDFromContext(ctx) == "" { - slog.Warn("missing X-Request-ID; using payload-derived outbox dedup id", - "destSiteID", destSiteID) - } - return payloadSeed + ":" + destSiteID -} From 55ab4747dac84147f124fc18136da5213d2876bb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 4 May 2026 03:29:43 +0000 Subject: [PATCH 31/32] docs+log: spec collection name; clarify lookupOwnerSiteID warn message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeRabbit nits: - spec implementation outline still showed db.Collection("threadSubscriptions") in the inbox-worker/main.go section, contradicting the rest of the spec (which uses thread_subscriptions) and the actual code after 8147b83. Fix the example string and clarify why the Go field name and collection name differ. Drop a stale \$bit:or reference at the same time (helper text was also out of date — the implementation uses \$max). - lookupOwnerSiteID's warn log on userstore.ErrUserNotFound said "skipping thread subscription" but after the parent-Insert gate fix, only the cross-site outbox publish is skipped — the local Insert/Upsert still runs. Reword so on-call debugging tracks the correct flow. --- ...28-message-worker-thread-subscription-outbox-design.md | 8 +++++--- message-worker/handler.go | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md b/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md index 141935e17..1475a419d 100644 --- a/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md +++ b/docs/superpowers/specs/2026-04-28-message-worker-thread-subscription-outbox-design.md @@ -303,9 +303,11 @@ which NAKs. #### `inbox-worker/main.go` - Add `threadSubCol *mongo.Collection` to `mongoInboxStore`, initialized from - `db.Collection("threadSubscriptions")` (same collection name message-worker - uses). -- Implement `UpsertThreadSubscription` with the `$setOnInsert` + `$set` + `$bit:or` + `db.Collection("thread_subscriptions")` (canonical snake_case name shared + with message-worker's `store_mongo.go` and history-service's + `mongorepo/threadroom.go`; the Go field name `threadSubCol` is camelCase but + the Mongo collection string is snake_case). +- Implement `UpsertThreadSubscription` with the `$setOnInsert` + `$set` + `$max` shape described above. Filter is `{threadRoomId, userId}` to match the message-worker's natural key. diff --git a/message-worker/handler.go b/message-worker/handler.go index ef2f5916c..e0a6d689f 100644 --- a/message-worker/handler.go +++ b/message-worker/handler.go @@ -277,7 +277,7 @@ func (h *Handler) lookupOwnerSiteID(ctx context.Context, userID, role string) (s user, err := h.userStore.FindUserByID(ctx, userID) if err != nil { if errors.Is(err, userstore.ErrUserNotFound) { - slog.Warn("owner user not found — skipping thread subscription", + slog.Warn("owner user not found — skipping cross-site outbox publish; local thread subscription insert/upsert continues", "userID", userID, "role", role) return "", nil } From 48381102fb94d946b0f49e0ab517c8dfee84b94c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 4 May 2026 05:09:28 +0000 Subject: [PATCH 32/32] test: fixup upstream NewHandler call site after rebase main brought in TestHandler_QuotedMessage (PR #134, message quoting) that calls NewHandler with the old 3-arg signature. Update that one site to the post-Task-3 5-arg form (siteID + no-op publish closure), matching the pattern used by every other test in this file. --- message-worker/handler_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/message-worker/handler_test.go b/message-worker/handler_test.go index 8118dd65f..48f9bad04 100644 --- a/message-worker/handler_test.go +++ b/message-worker/handler_test.go @@ -1722,7 +1722,9 @@ func TestHandler_ProcessMessage_Quote(t *testing.T) { return nil }) - h := NewHandler(store, userStore, threadStore) + h := NewHandler(store, userStore, threadStore, "site-a", func(_ context.Context, _ string, _ []byte, _ string) error { + return nil + }) err := h.processMessage(context.Background(), quotedData) require.NoError(t, err) })