From 3fcb24c5e0eff9af9a6d5d3d17f079fb3c720b7e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:06:41 +0000 Subject: [PATCH 01/32] docs(plan): add room-service mute.toggle implementation plan Plan for new per-user mute.toggle RPC in room-service following the message.read inline-write pattern, plus cross-site outbox/inbox sync mirroring subscription_read. Also renames Subscription.DisableNotification to DisableNotifications across production code. https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- .../2026-05-21-room-service-mute-toggle.md | 1111 +++++++++++++++++ 1 file changed, 1111 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md diff --git a/docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md b/docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md new file mode 100644 index 000000000..9d18ccef3 --- /dev/null +++ b/docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md @@ -0,0 +1,1111 @@ +# Room-Service `mute.toggle` RPC — 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:** Add a per-user `mute.toggle` RPC to `room-service` that flips `Subscription.DisableNotifications` for the requester in a single room, with cross-site federation so the user's home-site mirror stays consistent. Field is renamed from `DisableNotification` (singular) to `DisableNotifications` (plural) as part of the same plan. + +**Architecture:** Follow the **`message.read` pattern** (Pattern B) — `room-service` performs the Mongo write inline (atomic aggregation-pipeline `FindOneAndUpdate` so the toggle is single-round-trip and concurrency-safe), publishes a `SubscriptionUpdateEvent` to `chat.user.{account}.event.subscription.update` for client UI fan-out, and (when `users.siteId != h.siteID`) publishes a new `subscription_mute_toggled` outbox event so `inbox-worker` mirrors the change on the user's home site. No canonical event, no `room-worker` involvement, no key rotation. + +**Tech Stack:** Go 1.25 · NATS (core + JetStream) · MongoDB driver v2 · `go.uber.org/mock` · `stretchr/testify`. + +--- + +## Cross-site stale-data assessment + +The codebase replicates subscriptions per-site: the **room's home site** holds a sub doc for every member (used by `notification-worker` to fan out new-message notifications); the **user's home site** holds a parallel copy (used by `user-service` to render the sidebar / list-rooms view). Outbox→Inbox keeps the user-site copy in sync with writes that happen on the room's site. + +For `mute.toggle`: +- **Notification correctness is safe even without cross-site sync.** The mute RPC lands on the room's home site (the subject's `{siteID}` is the room's site) and `room-service` writes on the room's site. `notification-worker` consumes `MESSAGES_CANONICAL_{siteID}` on the room's site and reads subscriptions from the room's site Mongo. So the mute is honored for notifications the moment the local write commits. +- **User's home-site copy needs cross-site sync.** When the client later refetches subscriptions via `user-service` (which reads from the user's home-site Mongo), it would see the stale value and the UI toggle would visibly snap back. The outbox event closes this gap. +- **Notification dispatch in `notification-worker` does not yet check `DisableNotifications`.** The current `handler.go` blindly publishes to every subscriber except the sender. Wiring the mute into the notification check is **out of scope for this plan** — call it out in the docs update so the next PR owns that follow-up. + +**Production-data migration of the field rename** (`disableNotification` → `disableNotifications` in existing Mongo docs) is an **ops task** outside this plan. Existing botDM resubscribe behaviour writes `disableNotifications: false` on resubscribe upsert (post-rename), so new docs are clean; pre-existing docs with `disableNotification: true` would silently read as `false` under the new code. Ops needs to run: + +```js +db.subscriptions.updateMany( + { disableNotification: { $exists: true } }, + [ + { $set: { disableNotifications: "$disableNotification" } }, + { $unset: "disableNotification" } + ] +) +``` + +Note this in the PR description; do **not** add a code-side migration. + +--- + +## File map + +| File | What it does | Action | +|------|--------------|--------| +| `pkg/model/subscription.go` | `Subscription.DisableNotification` field | Rename → `DisableNotifications` | +| `pkg/model/model_test.go` | Model round-trip tests | Rename field references + JSON key | +| `room-worker/integration_test.go` | botDM resubscribe tests | Rename field references | +| `pkg/model/event.go` | New outbox event type + payload struct | Add `OutboxSubscriptionMuteToggled` + `SubscriptionMuteToggledEvent` | +| `pkg/model/model_test.go` | Round-trip for new payload | Add test | +| `pkg/subject/subject.go` | Subject builders | Add `MuteToggle` + `MuteToggleWildcard` | +| `pkg/subject/subject_test.go` | Subject tests | Add cases | +| `room-service/store.go` | Store interface | Add `ToggleSubscriptionMute` method | +| `room-service/store_mongo.go` | Mongo impl | Implement `ToggleSubscriptionMute` | +| `room-service/integration_test.go` | Mongo integration | Add test for new method | +| `room-service/mock_store_test.go` | Generated mocks | Regenerate | +| `room-service/main.go` | Wire up handler | Add `publishCore` closure parameter | +| `room-service/handler.go` | New RPC handler | Add `natsMuteToggle` + `handleMuteToggle`; register subject | +| `room-service/handler_test.go` | Handler unit tests | Add table-driven tests | +| `inbox-worker/handler.go` | Cross-site mirror | Add `subscription_mute_toggled` case + handler | +| `inbox-worker/handler_test.go` | Handler unit tests | Add case | +| `inbox-worker/store.go` (or wherever `InboxStore` is defined) | Add `UpdateSubscriptionMute` | Add method to interface + Mongo impl | +| `docs/client-api.md` | Public API docs | Document new RPC + triggered events | + +Historical specs/plans under `docs/superpowers/{specs,plans}/2026-05-19-botdm-*` describe the old `DisableNotification` name — **do not retroactively rewrite them**; they are historical records. + +--- + +## Task 0: Rename `DisableNotification` → `DisableNotifications` + +**Files:** +- Modify: `pkg/model/subscription.go:42` +- Modify: `pkg/model/model_test.go:470,522,523` +- Modify: `room-worker/integration_test.go:1288,1320,1350,1351,1365,1399,1428,1429` + +- [ ] **Step 1: Update model test expectations first (TDD: red against old name)** + +Edit `pkg/model/model_test.go`. Replace the three occurrences: + +Line ~470 (in `TestSubscriptionJSON` "with optional fields set"): +```go +DisableNotifications: true, +``` + +Lines ~522-523: +```go +disableVal, hasDisable := raw["disableNotifications"] +assert.True(t, hasDisable, "disableNotifications must be present in JSON even when false") +``` + +- [ ] **Step 2: Run model tests — expect FAIL** + +Run: `make test SERVICE=pkg/model` + +Expected: FAIL — the struct still defines `DisableNotification`, the test now references `DisableNotifications`. + +- [ ] **Step 3: Rename the field on `model.Subscription`** + +Edit `pkg/model/subscription.go:42`: + +```go +DisableNotifications bool `json:"disableNotifications" bson:"disableNotifications"` +``` + +- [ ] **Step 4: Rename in room-worker integration test** + +Edit `room-worker/integration_test.go`. Use `replace_all` on the exact identifier `DisableNotification` → `DisableNotifications`. The comments at lines 1288 and 1365 mention the field name — update those too. + +- [ ] **Step 5: Run all tests — expect PASS** + +Run: `make test` + +Expected: PASS — no remaining references to the old field name in compiled code. + +- [ ] **Step 6: Search for any stragglers** + +Run: `grep -rn "DisableNotification\b\|disableNotification\b" --include="*.go" .` + +Expected: zero hits (the `\b` boundary makes sure `DisableNotifications` doesn't match). Anything that comes up: fix it. + +- [ ] **Step 7: Commit** + +```bash +git add pkg/model/subscription.go pkg/model/model_test.go room-worker/integration_test.go +git commit -m "refactor(model): rename Subscription.DisableNotification → DisableNotifications" +``` + +--- + +## Task 1: Add `MuteToggle` subject builders + +**Files:** +- Modify: `pkg/subject/subject.go` +- Modify: `pkg/subject/subject_test.go` + +- [ ] **Step 1: Write the failing tests** + +Append to `pkg/subject/subject_test.go` (after the existing `TestMessageRead*` block around line 320): + +```go +func TestMuteToggle(t *testing.T) { + got := subject.MuteToggle("alice", "r1", "site-a") + want := "chat.user.alice.request.room.r1.site-a.mute.toggle" + if got != want { + t.Errorf("MuteToggle: got %q, want %q", got, want) + } +} + +func TestMuteToggleWildcard(t *testing.T) { + got := subject.MuteToggleWildcard("site-a") + want := "chat.user.*.request.room.*.site-a.mute.toggle" + if got != want { + t.Errorf("MuteToggleWildcard: got %q, want %q", got, want) + } +} + +func TestMuteToggle_ParseUserRoomSubject(t *testing.T) { + subj := subject.MuteToggle("alice", "r1", "site-a") + account, roomID, ok := subject.ParseUserRoomSubject(subj) + if !ok || account != "alice" || roomID != "r1" { + t.Errorf("ParseUserRoomSubject(%q) = (%q,%q,%v), want (alice,r1,true)", subj, account, roomID, ok) + } +} +``` + +- [ ] **Step 2: Run — expect FAIL** + +Run: `make test SERVICE=pkg/subject` + +Expected: FAIL with "undefined: subject.MuteToggle" / "MuteToggleWildcard". + +- [ ] **Step 3: Implement the builders** + +Append to `pkg/subject/subject.go` after `MessageReadReceiptWildcard` (around line 362): + +```go +// MuteToggle returns the concrete subject for the per-user mute.toggle RPC. +// Pair with MuteToggleWildcard for room-service's QueueSubscribe. +func MuteToggle(account, roomID, siteID string) string { + return fmt.Sprintf("chat.user.%s.request.room.%s.%s.mute.toggle", account, roomID, siteID) +} + +// MuteToggleWildcard is the per-site subscription pattern for the mute.toggle RPC. +func MuteToggleWildcard(siteID string) string { + return fmt.Sprintf("chat.user.*.request.room.*.%s.mute.toggle", siteID) +} +``` + +- [ ] **Step 4: Run — expect PASS** + +Run: `make test SERVICE=pkg/subject` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add pkg/subject/subject.go pkg/subject/subject_test.go +git commit -m "feat(subject): add MuteToggle subject builders" +``` + +--- + +## Task 2: Add `MuteToggleResponse` and outbox event types + +**Files:** +- Modify: `pkg/model/event.go` +- Modify: `pkg/model/model_test.go` + +- [ ] **Step 1: Write a failing round-trip test for the new types** + +Append to `pkg/model/model_test.go` (near the end of the file, after the existing event round-trip tests): + +```go +func TestMuteToggleResponseJSON(t *testing.T) { + src := model.MuteToggleResponse{ + Status: "ok", + DisableNotifications: true, + } + data, err := json.Marshal(src) + require.NoError(t, err) + + var dst model.MuteToggleResponse + require.NoError(t, json.Unmarshal(data, &dst)) + assert.Equal(t, src, dst) + + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + assert.Equal(t, "ok", raw["status"]) + assert.Equal(t, true, raw["disableNotifications"]) +} + +func TestSubscriptionMuteToggledEventJSON(t *testing.T) { + src := model.SubscriptionMuteToggledEvent{ + Account: "alice", + RoomID: "r1", + DisableNotifications: true, + Timestamp: 1234567890, + } + data, err := json.Marshal(src) + require.NoError(t, err) + + var dst model.SubscriptionMuteToggledEvent + require.NoError(t, json.Unmarshal(data, &dst)) + assert.Equal(t, src, dst) +} + +func TestOutboxSubscriptionMuteToggledConst(t *testing.T) { + assert.Equal(t, model.OutboxEventType("subscription_mute_toggled"), model.OutboxSubscriptionMuteToggled) +} +``` + +- [ ] **Step 2: Run — expect FAIL** + +Run: `make test SERVICE=pkg/model` + +Expected: FAIL with "undefined: model.MuteToggleResponse" and "undefined: model.SubscriptionMuteToggledEvent" / "OutboxSubscriptionMuteToggled". + +- [ ] **Step 3: Add the types** + +Append to `pkg/model/event.go` after `RoomKeyEnsureResponse` (around line 211): + +```go +// MuteToggleResponse is the sync reply for the mute.toggle RPC. DisableNotifications +// carries the resulting value of the toggle (post-flip). +type MuteToggleResponse struct { + Status string `json:"status"` + DisableNotifications bool `json:"disableNotifications"` +} + +// SubscriptionMuteToggledEvent is the OutboxEvent.Payload for type +// "subscription_mute_toggled". Sent from a room's home site to the user's home +// site whenever a user flips DisableNotifications via the mute.toggle RPC; the +// destination updates its local subscription mirror. +type SubscriptionMuteToggledEvent struct { + Account string `json:"account" bson:"account"` + RoomID string `json:"roomId" bson:"roomId"` + DisableNotifications bool `json:"disableNotifications" bson:"disableNotifications"` + Timestamp int64 `json:"timestamp" bson:"timestamp"` +} +``` + +Add the new outbox event-type const inside the existing `const ( … OutboxSubscriptionRead … )` block (line ~81): + +```go +const ( + OutboxMemberAdded OutboxEventType = "member_added" + OutboxMemberRemoved OutboxEventType = "member_removed" + OutboxSubscriptionRead OutboxEventType = "subscription_read" + OutboxSubscriptionMuteToggled OutboxEventType = "subscription_mute_toggled" + OutboxThreadSubscriptionUpserted OutboxEventType = "thread_subscription_upserted" +) +``` + +- [ ] **Step 4: Run — expect PASS** + +Run: `make test SERVICE=pkg/model` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add pkg/model/event.go pkg/model/model_test.go +git commit -m "feat(model): add MuteToggleResponse + SubscriptionMuteToggledEvent" +``` + +--- + +## Task 3: Add `ToggleSubscriptionMute` to `RoomStore` interface + +**Files:** +- Modify: `room-service/store.go` + +- [ ] **Step 1: Add the method to the interface** + +Edit `room-service/store.go`. Add to the `RoomStore` interface (after `UpdateSubscriptionRead`, around line 75): + +```go +// ToggleSubscriptionMute atomically flips disableNotifications on the +// subscription keyed by (roomID, account) using a single aggregation-pipeline +// FindOneAndUpdate so concurrent toggles cannot deadlock or read-modify-write +// race. Returns the resulting value of disableNotifications post-flip. +// Returns model.ErrSubscriptionNotFound (wrapped) when no subscription matches. +ToggleSubscriptionMute(ctx context.Context, roomID, account string) (bool, error) +``` + +- [ ] **Step 2: Verify it compiles (no test yet — implementation comes next)** + +Run: `go build ./room-service/...` + +Expected: PASS — the interface change compiles even before the impl exists, because nothing yet binds a struct against the new method. (`MongoStore` is a concrete type and only validated against the interface at handler-construction time, which is still ok because we'll add the impl in the next task.) If the build does break here, that's fine — the next task adds the method. + +- [ ] **Step 3: Commit** + +```bash +git add room-service/store.go +git commit -m "feat(room-service): declare ToggleSubscriptionMute in RoomStore" +``` + +--- + +## Task 4: Implement `ToggleSubscriptionMute` on `MongoStore` + +**Files:** +- Modify: `room-service/store_mongo.go` +- Modify: `room-service/integration_test.go` + +- [ ] **Step 1: Write a failing integration test** + +Open `room-service/integration_test.go` and append the following test. Use the existing `testutil.MongoDB(t, ...)` helper pattern that other tests in the file already use; mirror their `package main` + `//go:build integration` tag. + +```go +func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { + db := testutil.MongoDB(t, "room-svc-mute") + store := NewMongoStore(db) + ctx := context.Background() + + sub := &model.Subscription{ + ID: idgen.GenerateUUIDv7(), + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + RoomType: model.RoomTypeChannel, + SiteID: "site-a", + Roles: []model.Role{model.RoleMember}, + JoinedAt: time.Now().UTC(), + DisableNotifications: false, + } + require.NoError(t, store.CreateSubscription(ctx, sub)) + + // First toggle: false → true. + got, err := store.ToggleSubscriptionMute(ctx, "r1", "alice") + require.NoError(t, err) + assert.True(t, got) + + // Verify persisted. + persisted, err := store.GetSubscription(ctx, "alice", "r1") + require.NoError(t, err) + assert.True(t, persisted.DisableNotifications) + + // Second toggle: true → false. + got, err = store.ToggleSubscriptionMute(ctx, "r1", "alice") + require.NoError(t, err) + assert.False(t, got) + + // Not-found case. + _, err = store.ToggleSubscriptionMute(ctx, "missing", "alice") + assert.ErrorIs(t, err, model.ErrSubscriptionNotFound) +} +``` + +If `idgen`, `time`, or `model.ErrSubscriptionNotFound` aren't already imported in this file, add them — check the existing import block. + +- [ ] **Step 2: Run the integration test — expect FAIL** + +Run: `make test-integration SERVICE=room-service` + +Expected: FAIL — `store.ToggleSubscriptionMute` undefined. + +- [ ] **Step 3: Implement on `MongoStore`** + +Append to `room-service/store_mongo.go` after `UpdateSubscriptionRead` (around line 679): + +```go +// ToggleSubscriptionMute atomically flips disableNotifications via an +// aggregation-pipeline FindOneAndUpdate. The $ifNull guard treats a missing +// field as false so legacy documents toggle to true on the first call. +// Returns the post-flip value, or model.ErrSubscriptionNotFound when no +// subscription matches. +func (s *MongoStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (bool, error) { + filter := bson.M{"roomId": roomID, "u.account": account} + update := mongo.Pipeline{ + bson.D{{Key: "$set", Value: bson.M{ + "disableNotifications": bson.M{"$not": bson.M{ + "$ifNull": bson.A{"$disableNotifications", false}, + }}, + }}}, + } + opts := options.FindOneAndUpdate(). + SetReturnDocument(options.After). + SetProjection(bson.M{"_id": 0, "disableNotifications": 1}) + + var result struct { + DisableNotifications bool `bson:"disableNotifications"` + } + err := s.subscriptions.FindOneAndUpdate(ctx, filter, update, opts).Decode(&result) + if err != nil { + if errors.Is(err, mongo.ErrNoDocuments) { + return false, fmt.Errorf("toggle mute for %q in room %q: %w", account, roomID, model.ErrSubscriptionNotFound) + } + return false, fmt.Errorf("toggle mute for %q in room %q: %w", account, roomID, err) + } + return result.DisableNotifications, nil +} +``` + +- [ ] **Step 4: Run the integration test — expect PASS** + +Run: `make test-integration SERVICE=room-service` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add room-service/store_mongo.go room-service/integration_test.go +git commit -m "feat(room-service): implement ToggleSubscriptionMute on MongoStore" +``` + +--- + +## Task 5: Regenerate mocks + +**Files:** +- Modify: `room-service/mock_store_test.go` (auto-generated; never hand-edit) + +- [ ] **Step 1: Regenerate** + +Run: `make generate SERVICE=room-service` + +- [ ] **Step 2: Verify the new method appears** + +Run: `grep -n ToggleSubscriptionMute room-service/mock_store_test.go` + +Expected: at least one hit for the mock method. + +- [ ] **Step 3: Run unit tests to confirm nothing else broke** + +Run: `make test SERVICE=room-service` + +Expected: PASS. + +- [ ] **Step 4: Commit** + +```bash +git add room-service/mock_store_test.go +git commit -m "chore(room-service): regenerate mocks for ToggleSubscriptionMute" +``` + +--- + +## Task 6: Wire `publishCore` into the handler + +`room-service` currently only injects `publishToStream` (JetStream). The mute handler needs a core-NATS publish path for `chat.user.{account}.event.subscription.update`. Add a second injected closure following the room-worker shape. + +**Files:** +- Modify: `room-service/handler.go` +- Modify: `room-service/main.go` + +- [ ] **Step 1: Add the field + constructor parameter on `Handler`** + +Edit `room-service/handler.go`. In the `Handler` struct (around line 30), add after `publishToStream`: + +```go +publishCore func(ctx context.Context, subj string, data []byte) error +``` + +In `NewHandler` (around line 43), add the new parameter as the last argument, before the closing `)`: + +```go +func NewHandler(store RoomStore, keyStore RoomKeyStore, memberListClient MemberListClient, msgReader MessageReader, siteID string, maxRoomSize, maxBatchSize int, memberListTimeout time.Duration, publishToStream func(context.Context, string, []byte) error, publishCore func(context.Context, string, []byte) error) *Handler { + return &Handler{ + store: store, + keyStore: keyStore, + memberListClient: memberListClient, + msgReader: msgReader, + siteID: siteID, + maxRoomSize: maxRoomSize, + maxBatchSize: maxBatchSize, + memberListTimeout: memberListTimeout, + publishToStream: publishToStream, + publishCore: publishCore, + } +} +``` + +- [ ] **Step 2: Wire it in `main.go`** + +Edit `room-service/main.go`. Replace the `handler := NewHandler(...)` block (line 122) with: + +```go +handler := NewHandler(store, keyStore, memberListClient, cassReader, cfg.SiteID, cfg.MaxRoomSize, cfg.MaxBatchSize, cfg.MemberListTimeout, + func(ctx context.Context, subj string, data []byte) error { + if _, err := js.PublishMsg(ctx, natsutil.NewMsg(ctx, subj, data)); err != nil { + return fmt.Errorf("publish to %q: %w", subj, err) + } + return nil + }, + func(ctx context.Context, subj string, data []byte) error { + if err := nc.PublishMsg(ctx, natsutil.NewMsg(ctx, subj, data)); err != nil { + return fmt.Errorf("publish core to %q: %w", subj, err) + } + return nil + }, +) +``` + +- [ ] **Step 3: Build the service** + +Run: `make build SERVICE=room-service` + +Expected: PASS — the new param is plumbed end-to-end. + +- [ ] **Step 4: Update existing handler tests that construct `Handler{}` literally** + +Many tests in `handler_test.go` construct `&Handler{...}` with named fields and never set `publishCore`. That's fine: nil is acceptable when the test path doesn't exercise it. No code change required here. Run the tests to confirm: + +Run: `make test SERVICE=room-service` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add room-service/handler.go room-service/main.go +git commit -m "feat(room-service): inject publishCore alongside publishToStream" +``` + +--- + +## Task 7: Implement `handleMuteToggle` and register the RPC + +**Files:** +- Modify: `room-service/handler.go` +- Modify: `room-service/handler_test.go` + +- [ ] **Step 1: Write failing handler unit tests** + +Append to `room-service/handler_test.go`: + +```go +func TestHandler_MuteToggle_Success(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(&model.Subscription{ + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + }, nil) + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(true, nil) + store.EXPECT(). + GetUserSiteID(gomock.Any(), "alice"). + Return("site-a", nil) // same site → no outbox publish + + var coreSubjects []string + var coreBodies [][]byte + h := &Handler{ + store: store, + siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { + t.Fatal("publishToStream must not be called for same-site mute toggle") + return nil + }, + publishCore: func(_ context.Context, subj string, data []byte) error { + coreSubjects = append(coreSubjects, subj) + coreBodies = append(coreBodies, data) + return nil + }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + resp, err := h.handleMuteToggle(context.Background(), subj, nil) + require.NoError(t, err) + + var got model.MuteToggleResponse + require.NoError(t, json.Unmarshal(resp, &got)) + assert.Equal(t, "ok", got.Status) + assert.True(t, got.DisableNotifications) + + require.Len(t, coreSubjects, 1) + assert.Equal(t, subject.SubscriptionUpdate("alice"), coreSubjects[0]) + + var evt model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(coreBodies[0], &evt)) + assert.Equal(t, "mute_toggled", evt.Action) + assert.True(t, evt.Subscription.DisableNotifications) + assert.Equal(t, "alice", evt.Subscription.User.Account) +} + +func TestHandler_MuteToggle_CrossSitePublishesOutbox(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(&model.Subscription{ + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", SiteID: "site-a", + }, nil) + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(true, nil) + store.EXPECT(). + GetUserSiteID(gomock.Any(), "alice"). + Return("site-b", nil) + + var streamSubj string + var streamData []byte + h := &Handler{ + store: store, siteID: "site-a", + publishToStream: func(_ context.Context, s string, d []byte) error { + streamSubj = s + streamData = d + return nil + }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + _, err := h.handleMuteToggle(context.Background(), subj, nil) + require.NoError(t, err) + + assert.Equal(t, subject.Outbox("site-a", "site-b", model.OutboxSubscriptionMuteToggled), streamSubj) + + var outbox model.OutboxEvent + require.NoError(t, json.Unmarshal(streamData, &outbox)) + assert.Equal(t, model.OutboxSubscriptionMuteToggled, outbox.Type) + assert.Equal(t, "site-a", outbox.SiteID) + assert.Equal(t, "site-b", outbox.DestSiteID) + + var payload model.SubscriptionMuteToggledEvent + require.NoError(t, json.Unmarshal(outbox.Payload, &payload)) + assert.Equal(t, "alice", payload.Account) + assert.Equal(t, "r1", payload.RoomID) + assert.True(t, payload.DisableNotifications) + assert.NotZero(t, payload.Timestamp) +} + +func TestHandler_MuteToggle_NotRoomMember(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(nil, model.ErrSubscriptionNotFound) + + h := &Handler{ + store: store, siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { return nil }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + _, err := h.handleMuteToggle(context.Background(), subj, nil) + assert.ErrorIs(t, err, errNotRoomMember) +} + +func TestHandler_MuteToggle_InvalidSubject(t *testing.T) { + h := &Handler{ + siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { return nil }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + _, err := h.handleMuteToggle(context.Background(), "garbage.subject", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid mute-toggle subject") +} + +func TestHandler_MuteToggle_StoreError(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(&model.Subscription{ + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, RoomID: "r1", + }, nil) + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(false, fmt.Errorf("db down")) + + h := &Handler{ + store: store, siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { return nil }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + subj := subject.MuteToggle("alice", "r1", "site-a") + _, err := h.handleMuteToggle(context.Background(), subj, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "toggle subscription mute") +} +``` + +- [ ] **Step 2: Run — expect FAIL** + +Run: `make test SERVICE=room-service` + +Expected: FAIL — `handleMuteToggle` undefined. + +- [ ] **Step 3: Implement `handleMuteToggle` and `natsMuteToggle`** + +Append to `room-service/handler.go` (at the bottom, after `handleEnsureRoomKey`): + +```go +func (h *Handler) natsMuteToggle(m otelnats.Msg) { + ctx := wrappedCtx(m) + resp, err := h.handleMuteToggle(ctx, m.Msg.Subject, m.Msg.Data) + if err != nil { + slog.Error("mute toggle failed", "error", err, "subject", m.Msg.Subject) + natsutil.ReplyError(m.Msg, sanitizeError(err)) + return + } + if err := m.Msg.Respond(resp); err != nil { + slog.Error("failed to respond to mute toggle", "error", err) + } +} + +func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ([]byte, error) { + account, roomID, ok := subject.ParseUserRoomSubject(subj) + if !ok { + return nil, fmt.Errorf("invalid mute-toggle subject: %s", subj) + } + + sub, err := h.store.GetSubscription(ctx, account, roomID) + switch { + case errors.Is(err, model.ErrSubscriptionNotFound): + return nil, errNotRoomMember + case err != nil: + return nil, fmt.Errorf("get subscription: %w", err) + } + + newVal, err := h.store.ToggleSubscriptionMute(ctx, roomID, account) + if err != nil { + if errors.Is(err, model.ErrSubscriptionNotFound) { + return nil, errNotRoomMember + } + return nil, fmt.Errorf("toggle subscription mute: %w", err) + } + + now := time.Now().UTC() + + updatedSub := *sub + updatedSub.DisableNotifications = newVal + subEvt := model.SubscriptionUpdateEvent{ + UserID: sub.User.ID, + Subscription: updatedSub, + Action: "mute_toggled", + Timestamp: now.UnixMilli(), + } + subEvtData, err := json.Marshal(subEvt) + if err != nil { + return nil, fmt.Errorf("marshal subscription update event: %w", err) + } + if err := h.publishCore(ctx, subject.SubscriptionUpdate(account), subEvtData); err != nil { + slog.Error("subscription update publish failed", "error", err, "account", account) + // Non-fatal — the DB write is the source of truth; clients will reconcile on next refetch. + } + + userSiteID, err := h.store.GetUserSiteID(ctx, account) + if err != nil { + slog.Warn("get user siteId failed; skipping outbox", "error", err, "account", account) + return json.Marshal(model.MuteToggleResponse{Status: "ok", DisableNotifications: newVal}) + } + if userSiteID != "" && userSiteID != h.siteID { + payload := model.SubscriptionMuteToggledEvent{ + Account: account, + RoomID: roomID, + DisableNotifications: newVal, + Timestamp: now.UnixMilli(), + } + payloadData, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("marshal mute-toggled payload: %w", err) + } + outbox := model.OutboxEvent{ + Type: model.OutboxSubscriptionMuteToggled, + SiteID: h.siteID, + DestSiteID: userSiteID, + Payload: payloadData, + Timestamp: now.UnixMilli(), + } + outboxData, err := json.Marshal(outbox) + if err != nil { + return nil, fmt.Errorf("marshal outbox event: %w", err) + } + if err := h.publishToStream(ctx, subject.Outbox(h.siteID, userSiteID, model.OutboxSubscriptionMuteToggled), outboxData); err != nil { + return nil, fmt.Errorf("publish mute-toggled outbox: %w", err) + } + } + + return json.Marshal(model.MuteToggleResponse{Status: "ok", DisableNotifications: newVal}) +} +``` + +- [ ] **Step 4: Register the subject in `RegisterCRUD`** + +Edit `room-service/handler.go`. Inside `RegisterCRUD` (around line 63), append a new subscription before the closing `return nil`: + +```go +if _, err := nc.QueueSubscribe(subject.MuteToggleWildcard(h.siteID), queue, h.natsMuteToggle); err != nil { + return fmt.Errorf("subscribe mute toggle: %w", err) +} +``` + +- [ ] **Step 5: Run — expect PASS** + +Run: `make test SERVICE=room-service` + +Expected: PASS — all five new handler tests green. + +- [ ] **Step 6: Run lint** + +Run: `make lint` + +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add room-service/handler.go room-service/handler_test.go +git commit -m "feat(room-service): add mute.toggle RPC handler" +``` + +--- + +## Task 8: Add `subscription_mute_toggled` handler to `inbox-worker` + +**Files:** +- Modify: `inbox-worker/handler.go` +- Modify: `inbox-worker/handler_test.go` +- Modify: `inbox-worker/store.go` (interface) and `inbox-worker/store_mongo.go` (impl) — confirm exact filenames first via `ls inbox-worker/` +- Modify: `inbox-worker/mock_store_test.go` (regenerated) + +- [ ] **Step 1: Locate the store files** + +Run: `ls inbox-worker/` + +Confirm the file names that hold the `InboxStore` interface and its Mongo impl (they may not exactly match `store.go` / `store_mongo.go`). Adjust the subsequent steps' paths accordingly. + +- [ ] **Step 2: Add the store interface method** + +In the file that defines `InboxStore` (the interface block is in `inbox-worker/handler.go:18-32`), append the new method to the interface: + +```go +// UpdateSubscriptionMute sets disableNotifications on the subscription keyed +// by (roomID, account). Missing-subscription is a silent no-op so federation +// races during membership delete don't surface errors. +UpdateSubscriptionMute(ctx context.Context, roomID, account string, disableNotifications bool) error +``` + +- [ ] **Step 3: Add the dispatch case** + +In the `HandleEvent` switch (around line 51), add: + +```go +case "subscription_mute_toggled": + return h.handleSubscriptionMuteToggled(ctx, &evt) +``` + +(Place the case after `subscription_read` to keep related cases together.) + +- [ ] **Step 4: Add the handler method** + +Append after `handleSubscriptionRead` (around line 200): + +```go +// handleSubscriptionMuteToggled mirrors a mute toggle from the room's home +// site onto the user's home-site subscription copy. Missing-subscription is +// a silent no-op (the store enforces this) so a federation race between a +// membership delete and a mute toggle does not error the consumer. +func (h *Handler) handleSubscriptionMuteToggled(ctx context.Context, evt *model.OutboxEvent) error { + var e model.SubscriptionMuteToggledEvent + if err := json.Unmarshal(evt.Payload, &e); err != nil { + return fmt.Errorf("unmarshal subscription_mute_toggled payload: %w", err) + } + if err := h.store.UpdateSubscriptionMute(ctx, e.RoomID, e.Account, e.DisableNotifications); err != nil { + return fmt.Errorf("update subscription mute for %q in room %q: %w", e.Account, e.RoomID, err) + } + return nil +} +``` + +- [ ] **Step 5: Write a failing unit test** + +Append to `inbox-worker/handler_test.go`: + +```go +func TestHandler_SubscriptionMuteToggled(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockInboxStore(ctrl) + + store.EXPECT(). + UpdateSubscriptionMute(gomock.Any(), "r1", "alice", true). + Return(nil) + + h := NewHandler(store) + + payload, err := json.Marshal(model.SubscriptionMuteToggledEvent{ + Account: "alice", RoomID: "r1", DisableNotifications: true, Timestamp: 12345, + }) + require.NoError(t, err) + evt, err := json.Marshal(model.OutboxEvent{ + Type: model.OutboxSubscriptionMuteToggled, SiteID: "site-a", DestSiteID: "site-b", + Payload: payload, Timestamp: 12345, + }) + require.NoError(t, err) + + require.NoError(t, h.HandleEvent(context.Background(), evt)) +} +``` + +(Match imports/import style with existing inbox-worker tests.) + +- [ ] **Step 6: Run — expect FAIL** + +Run: `make test SERVICE=inbox-worker` + +Expected: FAIL — `UpdateSubscriptionMute` mock doesn't exist yet. + +- [ ] **Step 7: Implement on the Mongo impl** + +In the inbox-worker Mongo impl file (likely `store_mongo.go`), add: + +```go +// UpdateSubscriptionMute sets disableNotifications on the subscription keyed +// by (roomID, account). Missing-subscription is a silent no-op. +func (s *MongoStore) UpdateSubscriptionMute(ctx context.Context, roomID, account string, disableNotifications bool) error { + _, err := s.subscriptions.UpdateOne(ctx, + bson.M{"roomId": roomID, "u.account": account}, + bson.M{"$set": bson.M{"disableNotifications": disableNotifications}}, + ) + if err != nil { + return fmt.Errorf("update subscription mute for %q in room %q: %w", account, roomID, err) + } + return nil +} +``` + +If the inbox-worker `MongoStore` struct uses a different field name for the subscriptions collection (e.g. `subs`, `Subscriptions`), match it — peek at one existing method like `UpdateSubscriptionRead` for the right receiver name. + +- [ ] **Step 8: Regenerate mocks** + +Run: `make generate SERVICE=inbox-worker` + +- [ ] **Step 9: Run — expect PASS** + +Run: `make test SERVICE=inbox-worker` + +Expected: PASS. + +- [ ] **Step 10: Lint** + +Run: `make lint` + +Expected: PASS. + +- [ ] **Step 11: Commit** + +```bash +git add inbox-worker/ +git commit -m "feat(inbox-worker): mirror subscription_mute_toggled outbox event" +``` + +--- + +## Task 9: Document the new RPC in `docs/client-api.md` + +**Files:** +- Modify: `docs/client-api.md` + +- [ ] **Step 1: Insert the new section after Mark Messages Read** + +Find the line `#### Mark Messages Read` (around line 735) and locate its closing `---` separator (around line 783). Insert the following section immediately before the next `####` block: + +````markdown +#### Toggle Mute + +**Subject:** `chat.user.{account}.request.room.{roomID}.{siteID}.mute.toggle` +**Reply subject:** auto-generated `_INBOX.>` (NATS request/reply) + +Synchronous RPC. `room-service` flips `Subscription.disableNotifications` for the requester in a single atomic Mongo `FindOneAndUpdate`, replies with the resulting value, fans out a `subscription.update` event to the user's other client sessions, and (for cross-site users) publishes a `subscription_mute_toggled` outbox event so `inbox-worker` mirrors the change on the user's home site. + +Idempotency: this is a toggle, not a set — every successful call flips the bit. Clients must debounce the user-visible action; redelivery of the same RPC will flip back. + +##### Request body + +The subject already carries `account` and `roomID`, so no body fields are required. Clients may send `{}` or omit the body entirely; any body content is ignored. + +##### Success response + +| Field | Type | Notes | +|------------------------|---------|-------| +| `status` | string | Always `"ok"`. | +| `disableNotifications` | boolean | The resulting value of `Subscription.disableNotifications` after the flip. | + +```json +{ "status": "ok", "disableNotifications": true } +``` + +##### Error response + +See [Error envelope](#6-error-envelope-reference). Common errors: + +- `"only room members can list members"` — the user has no subscription in the room (sentinel reused across membership-gated RPCs). +- `"invalid mute-toggle subject: …"` — the subject is malformed. + +##### Triggered events — success path + +**`chat.user.{account}.event.subscription.update`** — emitted once for the requester so other client sessions reconcile. + +| Field | Type | Notes | +|----------------|--------|-------| +| `userId` | string | The requester's internal user ID. | +| `subscription` | object | The `Subscription` record with the updated `disableNotifications`. | +| `action` | string | `"mute_toggled"`. | +| `timestamp` | number | Milliseconds since Unix epoch (UTC). | + +##### Behaviour notes + +- **Cross-site federation:** if the user's home site (`users.siteId`) differs from the handler's site, a `subscription_mute_toggled` outbox event is published to `outbox.{handlerSite}.to.{userSite}.subscription_mute_toggled` with payload `{account, roomId, disableNotifications, timestamp}`. The destination `inbox-worker` applies an unconditional `$set` on the local subscription mirror. +- **Notification delivery:** `notification-worker` does **not** yet consult `disableNotifications` before sending. End-to-end mute behaviour is wired only as far as the persisted flag; honouring it in fan-out is a follow-up. + +--- +```` + +- [ ] **Step 2: Commit** + +```bash +git add docs/client-api.md +git commit -m "docs(client-api): document mute.toggle RPC" +``` + +--- + +## Task 10: Final verification + +- [ ] **Step 1: Full suite** + +Run: `make lint && make test && make test-integration SERVICE=room-service && make test-integration SERVICE=inbox-worker` + +Expected: all green. + +- [ ] **Step 2: SAST** + +Run: `make sast` + +Expected: PASS. + +- [ ] **Step 3: Stale-reference scan** + +Run: `grep -rn "DisableNotification\b\|disableNotification\b" --include="*.go" .` + +Expected: zero hits (rename complete in production code; historical specs/plans intentionally untouched). + +- [ ] **Step 4: Push the branch** + +```bash +git push -u origin claude/explore-room-service-7tNlq +``` + +--- + +## Spec coverage self-review + +| Requirement | Task | +|---|---| +| Rename `DisableNotification` → `DisableNotifications` everywhere in production code + active tests | Task 0 | +| New per-user `mute.toggle` RPC subject + parser | Task 1 | +| Public response model + outbox event type/payload | Task 2 | +| Atomic Mongo toggle that returns post-flip value | Tasks 3, 4 | +| Mocks for the new store method | Task 5 | +| Handler-side wiring for core-NATS publish | Task 6 | +| RPC handler: validates membership, writes, fans out subscription.update, publishes cross-site outbox | Task 7 | +| `inbox-worker` mirror handler + store method | Task 8 | +| `docs/client-api.md` covers new RPC + triggered events | Task 9 | +| All checks green | Task 10 | +| Cross-site stale-data analysis recorded | "Cross-site stale-data assessment" section above | +| Production Mongo migration of renamed field documented as ops follow-up | Same section above | +| `notification-worker` honouring `disableNotifications` documented as follow-up | Behaviour notes in Task 9 | From b6e5b05b2ab69f1e1329e31230ddea2b981467bb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:15:32 +0000 Subject: [PATCH 02/32] docs(plan): drop data-migration section from mute.toggle plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No prod or staging exists yet, so the disableNotification → disableNotifications field rename does not need a Mongo migration step inside the plan. Local dev DBs are recreated on docker-compose up. https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- .../plans/2026-05-21-room-service-mute-toggle.md | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md b/docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md index 9d18ccef3..8fce30fac 100644 --- a/docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md +++ b/docs/superpowers/plans/2026-05-21-room-service-mute-toggle.md @@ -19,19 +19,7 @@ For `mute.toggle`: - **User's home-site copy needs cross-site sync.** When the client later refetches subscriptions via `user-service` (which reads from the user's home-site Mongo), it would see the stale value and the UI toggle would visibly snap back. The outbox event closes this gap. - **Notification dispatch in `notification-worker` does not yet check `DisableNotifications`.** The current `handler.go` blindly publishes to every subscriber except the sender. Wiring the mute into the notification check is **out of scope for this plan** — call it out in the docs update so the next PR owns that follow-up. -**Production-data migration of the field rename** (`disableNotification` → `disableNotifications` in existing Mongo docs) is an **ops task** outside this plan. Existing botDM resubscribe behaviour writes `disableNotifications: false` on resubscribe upsert (post-rename), so new docs are clean; pre-existing docs with `disableNotification: true` would silently read as `false` under the new code. Ops needs to run: - -```js -db.subscriptions.updateMany( - { disableNotification: { $exists: true } }, - [ - { $set: { disableNotifications: "$disableNotification" } }, - { $unset: "disableNotification" } - ] -) -``` - -Note this in the PR description; do **not** add a code-side migration. +No prod or staging environment exists yet, so the field rename does not need a Mongo data migration — local dev databases get recreated on every `docker-compose up`. If/when an environment with persisted data appears, ops can run a one-shot `updateMany` to copy `disableNotification` → `disableNotifications`; the plan itself contains no migration step. --- From dbfe0b0a3724c559ec84aa450f631f1fe513e677 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:23:28 +0000 Subject: [PATCH 03/32] =?UTF-8?q?refactor(model):=20rename=20Subscription.?= =?UTF-8?q?DisableNotification=20=E2=86=92=20DisableNotifications?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/model/model_test.go | 6 +++--- pkg/model/subscription.go | 2 +- room-worker/integration_test.go | 16 ++++++++-------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/model/model_test.go b/pkg/model/model_test.go index a67c246f8..6645ed5e6 100644 --- a/pkg/model/model_test.go +++ b/pkg/model/model_test.go @@ -467,7 +467,7 @@ func TestSubscriptionJSON(t *testing.T) { HasMention: true, ThreadUnread: []string{"parent-1", "parent-2"}, Alert: true, - DisableNotification: true, + DisableNotifications: true, } roundTrip(t, &s, &model.Subscription{}) }) @@ -519,8 +519,8 @@ func TestSubscriptionJSON_ThreadUnreadOmittedAlertAlwaysPresent(t *testing.T) { assert.True(t, hasAlert, "alert must be present in JSON even when false") assert.Equal(t, false, alertVal) - disableVal, hasDisable := raw["disableNotification"] - assert.True(t, hasDisable, "disableNotification must be present in JSON even when false") + disableVal, hasDisable := raw["disableNotifications"] + assert.True(t, hasDisable, "disableNotifications must be present in JSON even when false") assert.Equal(t, false, disableVal) var dst model.Subscription diff --git a/pkg/model/subscription.go b/pkg/model/subscription.go index 787b15277..05e8be695 100644 --- a/pkg/model/subscription.go +++ b/pkg/model/subscription.go @@ -39,7 +39,7 @@ type Subscription struct { HasMention bool `json:"hasMention" bson:"hasMention"` ThreadUnread []string `json:"threadUnread,omitempty" bson:"threadUnread,omitempty"` Alert bool `json:"alert" bson:"alert"` - DisableNotification bool `json:"disableNotification" bson:"disableNotification"` + DisableNotifications bool `json:"disableNotifications" bson:"disableNotifications"` } // SubscriptionHRInfo carries the counterpart's HR-directory record on a diff --git a/room-worker/integration_test.go b/room-worker/integration_test.go index 63177f9cc..38ccb4189 100644 --- a/room-worker/integration_test.go +++ b/room-worker/integration_test.go @@ -1285,7 +1285,7 @@ func TestIntegration_CreateRoom_FansOutRoomKeyEvent(t *testing.T) { // TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration locks in that // processCreateRoom's botDM branch keeps its insert-only contract on a // JetStream redelivery: a pre-existing muted, inactive botDM subscription -// must NOT be refreshed (DisableNotification, IsSubscribed, JoinedAt +// must NOT be refreshed (DisableNotifications, IsSubscribed, JoinedAt // untouched). The re-subscribe refresh semantic is owned by user-service. func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { ctx := context.Background() @@ -1317,7 +1317,7 @@ func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { RoomType: model.RoomTypeBotDM, Name: "helper.bot", IsSubscribed: false, - DisableNotification: true, + DisableNotifications: true, JoinedAt: oldJoinedAt, }) mustInsertSub(t, db, &model.Subscription{ @@ -1347,8 +1347,8 @@ func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { got, err := store.GetSubscription(ctx, "alice", roomID) require.NoError(t, err) - assert.True(t, got.DisableNotification, - "botDM path must NOT clear DisableNotification on redelivery (insert-only contract)") + assert.True(t, got.DisableNotifications, + "botDM path must NOT clear DisableNotifications on redelivery (insert-only contract)") assert.False(t, got.IsSubscribed, "botDM path must NOT refresh IsSubscribed on redelivery (insert-only contract)") assert.True(t, got.JoinedAt.Equal(oldJoinedAt), @@ -1362,7 +1362,7 @@ func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { // TestProcessCreateRoom_DM_DoesNotUpsert_Integration locks in that // processCreateRoom's regular-DM branch keeps its insert-only contract: // a pre-existing regular-DM subscription's state (specifically -// DisableNotification = true and an old JoinedAt) must NOT be refreshed +// DisableNotifications = true and an old JoinedAt) must NOT be refreshed // when processCreateRoom is replayed for the same (room, user) pair. // This regression guard prevents accidental upsert wiring on the DM // branch in future edits. @@ -1396,7 +1396,7 @@ func TestProcessCreateRoom_DM_DoesNotUpsert_Integration(t *testing.T) { SiteID: "site-A", RoomType: model.RoomTypeDM, Name: "bob", - DisableNotification: true, + DisableNotifications: true, JoinedAt: oldJoinedAt, }) mustInsertSub(t, db, &model.Subscription{ @@ -1425,8 +1425,8 @@ func TestProcessCreateRoom_DM_DoesNotUpsert_Integration(t *testing.T) { got, err := store.GetSubscription(ctx, "alice", roomID) require.NoError(t, err) - assert.True(t, got.DisableNotification, - "regular-DM path must NOT clear DisableNotification on re-create (insert-only contract)") + assert.True(t, got.DisableNotifications, + "regular-DM path must NOT clear DisableNotifications on re-create (insert-only contract)") assert.True(t, got.JoinedAt.Equal(oldJoinedAt), "regular-DM path must NOT refresh JoinedAt on re-create (insert-only contract)") } From 2860733da9d424c8d378313373f762974e6d4975 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:26:18 +0000 Subject: [PATCH 04/32] feat(subject): add MuteToggle subject builders --- pkg/subject/subject.go | 11 +++++++++++ pkg/subject/subject_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/pkg/subject/subject.go b/pkg/subject/subject.go index 61fe54b28..37d3c086f 100644 --- a/pkg/subject/subject.go +++ b/pkg/subject/subject.go @@ -361,6 +361,17 @@ func MessageReadReceiptWildcard(siteID string) string { return fmt.Sprintf("chat.user.*.request.room.*.%s.message.read-receipt", siteID) } +// MuteToggle returns the concrete subject for the per-user mute.toggle RPC. +// Pair with MuteToggleWildcard for room-service's QueueSubscribe. +func MuteToggle(account, roomID, siteID string) string { + return fmt.Sprintf("chat.user.%s.request.room.%s.%s.mute.toggle", account, roomID, siteID) +} + +// MuteToggleWildcard is the per-site subscription pattern for the mute.toggle RPC. +func MuteToggleWildcard(siteID string) string { + return fmt.Sprintf("chat.user.*.request.room.*.%s.mute.toggle", siteID) +} + // RoomCreate: client→room-service create subject; siteID is the requester's site. func RoomCreate(account, siteID string) string { return fmt.Sprintf("chat.user.%s.request.room.%s.create", account, siteID) diff --git a/pkg/subject/subject_test.go b/pkg/subject/subject_test.go index 5136f1cc5..99d896216 100644 --- a/pkg/subject/subject_test.go +++ b/pkg/subject/subject_test.go @@ -322,6 +322,30 @@ func TestMessageReadReceipt_ParseUserRoomSubject(t *testing.T) { } } +func TestMuteToggle(t *testing.T) { + got := subject.MuteToggle("alice", "r1", "site-a") + want := "chat.user.alice.request.room.r1.site-a.mute.toggle" + if got != want { + t.Errorf("MuteToggle: got %q, want %q", got, want) + } +} + +func TestMuteToggleWildcard(t *testing.T) { + got := subject.MuteToggleWildcard("site-a") + want := "chat.user.*.request.room.*.site-a.mute.toggle" + if got != want { + t.Errorf("MuteToggleWildcard: got %q, want %q", got, want) + } +} + +func TestMuteToggle_ParseUserRoomSubject(t *testing.T) { + subj := subject.MuteToggle("alice", "r1", "site-a") + account, roomID, ok := subject.ParseUserRoomSubject(subj) + if !ok || account != "alice" || roomID != "r1" { + t.Errorf("ParseUserRoomSubject(%q) = (%q,%q,%v), want (alice,r1,true)", subj, account, roomID, ok) + } +} + func TestParseRoomCreateSubject(t *testing.T) { tests := []struct { name string From b327f82ac2a54fee0082bfdca37cf2ecc485a8b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:28:56 +0000 Subject: [PATCH 05/32] feat(model): add MuteToggleResponse + SubscriptionMuteToggledEvent --- pkg/model/event.go | 19 +++++++++++++++++++ pkg/model/model_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/pkg/model/event.go b/pkg/model/event.go index 1b1d6bf96..febed921a 100644 --- a/pkg/model/event.go +++ b/pkg/model/event.go @@ -82,6 +82,7 @@ const ( OutboxMemberAdded OutboxEventType = "member_added" OutboxMemberRemoved OutboxEventType = "member_removed" OutboxSubscriptionRead OutboxEventType = "subscription_read" + OutboxSubscriptionMuteToggled OutboxEventType = "subscription_mute_toggled" OutboxThreadSubscriptionUpserted OutboxEventType = "thread_subscription_upserted" ) @@ -210,6 +211,24 @@ type RoomKeyEnsureResponse struct { Version int `json:"version"` } +// MuteToggleResponse is the sync reply for the mute.toggle RPC. DisableNotifications +// carries the resulting value of the toggle (post-flip). +type MuteToggleResponse struct { + Status string `json:"status"` + DisableNotifications bool `json:"disableNotifications"` +} + +// SubscriptionMuteToggledEvent is the OutboxEvent.Payload for type +// "subscription_mute_toggled". Sent from a room's home site to the user's home +// site whenever a user flips DisableNotifications via the mute.toggle RPC; the +// destination updates its local subscription mirror. +type SubscriptionMuteToggledEvent struct { + Account string `json:"account" bson:"account"` + RoomID string `json:"roomId" bson:"roomId"` + DisableNotifications bool `json:"disableNotifications" bson:"disableNotifications"` + Timestamp int64 `json:"timestamp" bson:"timestamp"` +} + type MemberRemoveEvent struct { Type string `json:"type" bson:"type"` RoomID string `json:"roomId" bson:"roomId"` diff --git a/pkg/model/model_test.go b/pkg/model/model_test.go index 6645ed5e6..4ab2e87e2 100644 --- a/pkg/model/model_test.go +++ b/pkg/model/model_test.go @@ -2224,6 +2224,43 @@ func TestMessageTypeAndAsyncJobStatusConstants(t *testing.T) { assert.Equal(t, "error", model.AsyncJobStatusError) } +func TestMuteToggleResponseJSON(t *testing.T) { + src := model.MuteToggleResponse{ + Status: "ok", + DisableNotifications: true, + } + data, err := json.Marshal(src) + require.NoError(t, err) + + var dst model.MuteToggleResponse + require.NoError(t, json.Unmarshal(data, &dst)) + assert.Equal(t, src, dst) + + var raw map[string]any + require.NoError(t, json.Unmarshal(data, &raw)) + assert.Equal(t, "ok", raw["status"]) + assert.Equal(t, true, raw["disableNotifications"]) +} + +func TestSubscriptionMuteToggledEventJSON(t *testing.T) { + src := model.SubscriptionMuteToggledEvent{ + Account: "alice", + RoomID: "r1", + DisableNotifications: true, + Timestamp: 1234567890, + } + data, err := json.Marshal(src) + require.NoError(t, err) + + var dst model.SubscriptionMuteToggledEvent + require.NoError(t, json.Unmarshal(data, &dst)) + assert.Equal(t, src, dst) +} + +func TestOutboxSubscriptionMuteToggledConst(t *testing.T) { + assert.Equal(t, model.OutboxEventType("subscription_mute_toggled"), model.OutboxSubscriptionMuteToggled) +} + func TestSyncCreateDMRequestJSON(t *testing.T) { src := model.SyncCreateDMRequest{ RoomType: model.RoomTypeDM, From 998cd6208dbbcbd6ea24adb10dea2ebc250a64f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:30:38 +0000 Subject: [PATCH 06/32] feat(room-service): declare ToggleSubscriptionMute in RoomStore --- room-service/store.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/room-service/store.go b/room-service/store.go index 81b23d4d3..d1949ce11 100644 --- a/room-service/store.go +++ b/room-service/store.go @@ -72,6 +72,12 @@ type RoomStore interface { // keyed by (roomID, account). Returns model.ErrSubscriptionNotFound // (wrapped) when no subscription matches. UpdateSubscriptionRead(ctx context.Context, roomID, account string, lastSeenAt time.Time, alert bool) error + // ToggleSubscriptionMute atomically flips disableNotifications on the + // subscription keyed by (roomID, account) using a single aggregation-pipeline + // FindOneAndUpdate so concurrent toggles cannot deadlock or read-modify-write + // race. Returns the resulting value of disableNotifications post-flip. + // Returns model.ErrSubscriptionNotFound (wrapped) when no subscription matches. + ToggleSubscriptionMute(ctx context.Context, roomID, account string) (bool, error) // GetUserSiteID returns the home site of a user looked up by account. // Returns ("", nil) when the user is not found locally; callers treat // that as "skip cross-site outbox". From c7ca631cab0d107c33b832f458e30dc5b34e696d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:43:36 +0000 Subject: [PATCH 07/32] feat(room-service): implement ToggleSubscriptionMute on MongoStore Uses aggregation-pipeline FindOneAndUpdate with \$ifNull guard so legacy documents (missing disableNotifications field) toggle to true on first call. Also regenerates mock and fixes goimports alignment in subscription struct and dependent test files after the DisableNotifications rename. https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- pkg/model/model_test.go | 24 ++++++++++----------- pkg/model/subscription.go | 28 ++++++++++++------------ room-service/integration_test.go | 37 ++++++++++++++++++++++++++++++++ room-service/mock_store_test.go | 15 +++++++++++++ room-service/store_mongo.go | 31 ++++++++++++++++++++++++++ room-worker/integration_test.go | 30 +++++++++++++------------- 6 files changed, 124 insertions(+), 41 deletions(-) diff --git a/pkg/model/model_test.go b/pkg/model/model_test.go index 4ab2e87e2..1785775a4 100644 --- a/pkg/model/model_test.go +++ b/pkg/model/model_test.go @@ -455,18 +455,18 @@ func TestSubscriptionJSON(t *testing.T) { hss := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) lsa := time.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC) s := model.Subscription{ - ID: "s1", - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", - RoomType: model.RoomTypeChannel, - SiteID: "site-a", - Roles: []model.Role{model.RoleOwner}, - HistorySharedSince: &hss, - JoinedAt: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), - LastSeenAt: &lsa, - HasMention: true, - ThreadUnread: []string{"parent-1", "parent-2"}, - Alert: true, + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + RoomType: model.RoomTypeChannel, + SiteID: "site-a", + Roles: []model.Role{model.RoleOwner}, + HistorySharedSince: &hss, + JoinedAt: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), + LastSeenAt: &lsa, + HasMention: true, + ThreadUnread: []string{"parent-1", "parent-2"}, + Alert: true, DisableNotifications: true, } roundTrip(t, &s, &model.Subscription{}) diff --git a/pkg/model/subscription.go b/pkg/model/subscription.go index 05e8be695..b1ba71f3a 100644 --- a/pkg/model/subscription.go +++ b/pkg/model/subscription.go @@ -25,20 +25,20 @@ type SubscriptionUser struct { } type Subscription struct { - ID string `json:"id" bson:"_id"` - User SubscriptionUser `json:"u" bson:"u"` - RoomID string `json:"roomId" bson:"roomId"` - SiteID string `json:"siteId" bson:"siteId"` - Roles []Role `json:"roles" bson:"roles"` - Name string `json:"name" bson:"name"` - RoomType RoomType `json:"roomType" bson:"roomType"` - IsSubscribed bool `json:"isSubscribed,omitempty" bson:"isSubscribed,omitempty"` - HistorySharedSince *time.Time `json:"historySharedSince,omitempty" bson:"historySharedSince,omitempty"` - JoinedAt time.Time `json:"joinedAt" bson:"joinedAt"` - LastSeenAt *time.Time `json:"lastSeenAt,omitempty" bson:"lastSeenAt,omitempty"` - HasMention bool `json:"hasMention" bson:"hasMention"` - ThreadUnread []string `json:"threadUnread,omitempty" bson:"threadUnread,omitempty"` - Alert bool `json:"alert" bson:"alert"` + ID string `json:"id" bson:"_id"` + User SubscriptionUser `json:"u" bson:"u"` + RoomID string `json:"roomId" bson:"roomId"` + SiteID string `json:"siteId" bson:"siteId"` + Roles []Role `json:"roles" bson:"roles"` + Name string `json:"name" bson:"name"` + RoomType RoomType `json:"roomType" bson:"roomType"` + IsSubscribed bool `json:"isSubscribed,omitempty" bson:"isSubscribed,omitempty"` + HistorySharedSince *time.Time `json:"historySharedSince,omitempty" bson:"historySharedSince,omitempty"` + JoinedAt time.Time `json:"joinedAt" bson:"joinedAt"` + LastSeenAt *time.Time `json:"lastSeenAt,omitempty" bson:"lastSeenAt,omitempty"` + HasMention bool `json:"hasMention" bson:"hasMention"` + ThreadUnread []string `json:"threadUnread,omitempty" bson:"threadUnread,omitempty"` + Alert bool `json:"alert" bson:"alert"` DisableNotifications bool `json:"disableNotifications" bson:"disableNotifications"` } diff --git a/room-service/integration_test.go b/room-service/integration_test.go index 83335aa3d..e990cf973 100644 --- a/room-service/integration_test.go +++ b/room-service/integration_test.go @@ -1535,3 +1535,40 @@ func TestMongoStore_ListReadReceipts_Integration(t *testing.T) { require.NoError(t, err) require.Empty(t, rows) } + +func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { + db := testutil.MongoDB(t, "room-svc-mute") + store := NewMongoStore(db) + ctx := context.Background() + + sub := &model.Subscription{ + ID: idgen.GenerateUUIDv7(), + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + RoomType: model.RoomTypeChannel, + SiteID: "site-a", + Roles: []model.Role{model.RoleMember}, + JoinedAt: time.Now().UTC(), + DisableNotifications: false, + } + require.NoError(t, store.CreateSubscription(ctx, sub)) + + // First toggle: false → true. + got, err := store.ToggleSubscriptionMute(ctx, "r1", "alice") + require.NoError(t, err) + assert.True(t, got) + + // Verify persisted. + persisted, err := store.GetSubscription(ctx, "alice", "r1") + require.NoError(t, err) + assert.True(t, persisted.DisableNotifications) + + // Second toggle: true → false. + got, err = store.ToggleSubscriptionMute(ctx, "r1", "alice") + require.NoError(t, err) + assert.False(t, got) + + // Not-found case. + _, err = store.ToggleSubscriptionMute(ctx, "missing", "alice") + assert.ErrorIs(t, err, model.ErrSubscriptionNotFound) +} diff --git a/room-service/mock_store_test.go b/room-service/mock_store_test.go index 7c896ac4f..1ddce1cd9 100644 --- a/room-service/mock_store_test.go +++ b/room-service/mock_store_test.go @@ -311,6 +311,21 @@ func (mr *MockRoomStoreMockRecorder) MinSubscriptionLastSeenByRoomID(ctx, roomID return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MinSubscriptionLastSeenByRoomID", reflect.TypeOf((*MockRoomStore)(nil).MinSubscriptionLastSeenByRoomID), ctx, roomID) } +// ToggleSubscriptionMute mocks base method. +func (m *MockRoomStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ToggleSubscriptionMute", ctx, roomID, account) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ToggleSubscriptionMute indicates an expected call of ToggleSubscriptionMute. +func (mr *MockRoomStoreMockRecorder) ToggleSubscriptionMute(ctx, roomID, account any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ToggleSubscriptionMute", reflect.TypeOf((*MockRoomStore)(nil).ToggleSubscriptionMute), ctx, roomID, account) +} + // UpdateRoomMinUserLastSeenAt mocks base method. func (m *MockRoomStore) UpdateRoomMinUserLastSeenAt(ctx context.Context, roomID string, t *time.Time) error { m.ctrl.T.Helper() diff --git a/room-service/store_mongo.go b/room-service/store_mongo.go index e699c328e..bf8325945 100644 --- a/room-service/store_mongo.go +++ b/room-service/store_mongo.go @@ -678,6 +678,37 @@ func (s *MongoStore) UpdateSubscriptionRead(ctx context.Context, roomID, account return nil } +// ToggleSubscriptionMute atomically flips disableNotifications via an +// aggregation-pipeline FindOneAndUpdate. The $ifNull guard treats a missing +// field as false so legacy documents toggle to true on the first call. +// Returns the post-flip value, or model.ErrSubscriptionNotFound when no +// subscription matches. +func (s *MongoStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (bool, error) { + filter := bson.M{"roomId": roomID, "u.account": account} + update := mongo.Pipeline{ + bson.D{{Key: "$set", Value: bson.M{ + "disableNotifications": bson.M{"$not": bson.M{ + "$ifNull": bson.A{"$disableNotifications", false}, + }}, + }}}, + } + opts := options.FindOneAndUpdate(). + SetReturnDocument(options.After). + SetProjection(bson.M{"_id": 0, "disableNotifications": 1}) + + var result struct { + DisableNotifications bool `bson:"disableNotifications"` + } + err := s.subscriptions.FindOneAndUpdate(ctx, filter, update, opts).Decode(&result) + if err != nil { + if errors.Is(err, mongo.ErrNoDocuments) { + return false, fmt.Errorf("toggle mute for %q in room %q: %w", account, roomID, model.ErrSubscriptionNotFound) + } + return false, fmt.Errorf("toggle mute for %q in room %q: %w", account, roomID, err) + } + return result.DisableNotifications, nil +} + // GetUserSiteID looks up users.siteId by account. Returns ("", nil) if no // user document exists. func (s *MongoStore) GetUserSiteID(ctx context.Context, account string) (string, error) { diff --git a/room-worker/integration_test.go b/room-worker/integration_test.go index 38ccb4189..5b0986645 100644 --- a/room-worker/integration_test.go +++ b/room-worker/integration_test.go @@ -1310,15 +1310,15 @@ func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { Accounts: []string{"alice", "helper.bot"}, }) mustInsertSub(t, db, &model.Subscription{ - ID: "existing-human-sub", - User: model.SubscriptionUser{ID: "u_alice", Account: "alice"}, - RoomID: roomID, - SiteID: "site-A", - RoomType: model.RoomTypeBotDM, - Name: "helper.bot", - IsSubscribed: false, + ID: "existing-human-sub", + User: model.SubscriptionUser{ID: "u_alice", Account: "alice"}, + RoomID: roomID, + SiteID: "site-A", + RoomType: model.RoomTypeBotDM, + Name: "helper.bot", + IsSubscribed: false, DisableNotifications: true, - JoinedAt: oldJoinedAt, + JoinedAt: oldJoinedAt, }) mustInsertSub(t, db, &model.Subscription{ ID: "existing-bot-sub", @@ -1390,14 +1390,14 @@ func TestProcessCreateRoom_DM_DoesNotUpsert_Integration(t *testing.T) { Accounts: []string{"alice", "bob"}, }) mustInsertSub(t, db, &model.Subscription{ - ID: "existing-alice-sub", - User: model.SubscriptionUser{ID: "u_alice", Account: "alice"}, - RoomID: roomID, - SiteID: "site-A", - RoomType: model.RoomTypeDM, - Name: "bob", + ID: "existing-alice-sub", + User: model.SubscriptionUser{ID: "u_alice", Account: "alice"}, + RoomID: roomID, + SiteID: "site-A", + RoomType: model.RoomTypeDM, + Name: "bob", DisableNotifications: true, - JoinedAt: oldJoinedAt, + JoinedAt: oldJoinedAt, }) mustInsertSub(t, db, &model.Subscription{ ID: "existing-bob-sub", From 3b2fc06c339e5d48b3d7c62d9199854b2f9e442d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:48:23 +0000 Subject: [PATCH 08/32] feat(room-service): inject publishCore alongside publishToStream https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- room-service/handler.go | 4 +++- room-service/handler_test.go | 42 ++++++++++++++++++------------------ room-service/main.go | 20 +++++++++++------ 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/room-service/handler.go b/room-service/handler.go index c904d6e37..b19b3fc90 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -38,9 +38,10 @@ type Handler struct { maxBatchSize int memberListTimeout time.Duration publishToStream func(ctx context.Context, subj string, data []byte) error + publishCore func(ctx context.Context, subj string, data []byte) error } -func NewHandler(store RoomStore, keyStore RoomKeyStore, memberListClient MemberListClient, msgReader MessageReader, siteID string, maxRoomSize, maxBatchSize int, memberListTimeout time.Duration, publishToStream func(context.Context, string, []byte) error) *Handler { +func NewHandler(store RoomStore, keyStore RoomKeyStore, memberListClient MemberListClient, msgReader MessageReader, siteID string, maxRoomSize, maxBatchSize int, memberListTimeout time.Duration, publishToStream func(context.Context, string, []byte) error, publishCore func(context.Context, string, []byte) error) *Handler { return &Handler{ store: store, keyStore: keyStore, @@ -51,6 +52,7 @@ func NewHandler(store RoomStore, keyStore RoomKeyStore, memberListClient MemberL maxBatchSize: maxBatchSize, memberListTimeout: memberListTimeout, publishToStream: publishToStream, + publishCore: publishCore, } } diff --git a/room-service/handler_test.go b/room-service/handler_test.go index 9df2b9281..34e121b14 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -489,7 +489,7 @@ func TestHandler_RemoveMember_SelfLeave_Success(t *testing.T) { publishedSubj = subj publishedData = data return nil - }) + }, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "alice"}) @@ -530,7 +530,7 @@ func TestHandler_RemoveMember_OrgOnly_Rejected(t *testing.T) { store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) store.EXPECT().GetSubscriptionWithMembership(gomock.Any(), "r1", "alice"). Return(&SubscriptionWithMembership{Subscription: sub, HasOrgMembership: true}, nil) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove(tc.requester, "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: tc.target}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -557,7 +557,7 @@ func TestHandler_RemoveMember_SelfLeave_NoOrgs_Allowed(t *testing.T) { handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, func(ctx context.Context, _ string, data []byte) error { publishedData = data return nil - }) + }, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "alice"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -593,7 +593,7 @@ func TestHandler_RemoveMember_LastOwner_Rejected(t *testing.T) { } store.EXPECT().CountMembersAndOwners(gomock.Any(), "r1"). Return(&RoomCounts{MemberCount: 3, OwnerCount: 1}, nil) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove(tc.requester, "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "alice"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -615,7 +615,7 @@ func TestHandler_RemoveMember_LastMember_Rejected(t *testing.T) { Return(&SubscriptionWithMembership{Subscription: sub, HasIndividualMembership: true}, nil) store.EXPECT().CountMembersAndOwners(gomock.Any(), "r1"). Return(&RoomCounts{MemberCount: 1, OwnerCount: 0}, nil) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "alice"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -644,7 +644,7 @@ func TestHandler_RemoveMember_OwnerRemovesOther_Success(t *testing.T) { handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, func(ctx context.Context, subj string, data []byte) error { publishedData = data return nil - }) + }, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "bob"}) resp, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -668,7 +668,7 @@ func TestHandler_RemoveMember_NonOwnerRemovesOther_Rejected(t *testing.T) { store.EXPECT().GetSubscriptionWithMembership(gomock.Any(), "r1", "bob"). Return(&SubscriptionWithMembership{Subscription: targetSub, HasIndividualMembership: true}, nil) store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(requesterSub, nil) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "bob"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -689,7 +689,7 @@ func TestHandler_RemoveMember_OwnerRemovesOrg_Success(t *testing.T) { handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, func(ctx context.Context, subj string, data []byte) error { publishedData = data return nil - }) + }, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", OrgID: "eng-org"}) resp, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -704,7 +704,7 @@ func TestHandler_RemoveMember_BothAccountAndOrgID_Rejected(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockRoomStore(ctrl) store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "bob", OrgID: "eng-org"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -716,7 +716,7 @@ func TestHandler_RemoveMember_NeitherAccountNorOrgID_Rejected(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockRoomStore(ctrl) store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") reqBody, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, reqBody) @@ -727,7 +727,7 @@ func TestHandler_RemoveMember_NeitherAccountNorOrgID_Rejected(t *testing.T) { func TestHandler_RemoveMember_InvalidSubject(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockRoomStore(ctrl) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) _, err := handler.handleRemoveMember(context.Background(), "bogus", []byte("{}")) require.Error(t, err) assert.Contains(t, err.Error(), "invalid remove-member subject") @@ -736,7 +736,7 @@ func TestHandler_RemoveMember_InvalidSubject(t *testing.T) { func TestHandler_RemoveMember_InvalidJSON(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockRoomStore(ctrl) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") _, err := handler.handleRemoveMember(context.Background(), reqSubj, []byte("{not json")) require.Error(t, err) @@ -746,7 +746,7 @@ func TestHandler_RemoveMember_InvalidJSON(t *testing.T) { func TestHandler_RemoveMember_RoomIDMismatch(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockRoomStore(ctrl) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") body, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r2", Account: "alice"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, body) @@ -760,7 +760,7 @@ func TestHandler_RemoveMember_GetTargetError(t *testing.T) { store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) store.EXPECT().GetSubscriptionWithMembership(gomock.Any(), "r1", "alice"). Return(nil, fmt.Errorf("db down")) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") body, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "alice"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, body) @@ -779,7 +779,7 @@ func TestHandler_RemoveMember_OwnerRemoves_RequesterLookupError(t *testing.T) { Return(&SubscriptionWithMembership{Subscription: targetSub, HasIndividualMembership: true}, nil) store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1"). Return(nil, fmt.Errorf("db down")) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") body, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "bob"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, body) @@ -798,7 +798,7 @@ func TestHandler_RemoveMember_CountsError(t *testing.T) { Return(&SubscriptionWithMembership{Subscription: sub, HasIndividualMembership: true}, nil) store.EXPECT().CountMembersAndOwners(gomock.Any(), "r1"). Return(nil, fmt.Errorf("db down")) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") body, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "alice"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, body) @@ -812,7 +812,7 @@ func TestHandler_RemoveMember_OrgPath_RequesterLookupError(t *testing.T) { store.EXPECT().GetRoom(gomock.Any(), "r1").Return(&model.Room{ID: "r1", Type: model.RoomTypeChannel}, nil) store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1"). Return(nil, fmt.Errorf("db down")) - handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil) + handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, nil, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") body, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", OrgID: "eng-org"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, body) @@ -833,7 +833,7 @@ func TestHandler_RemoveMember_PublishError(t *testing.T) { Return(&RoomCounts{MemberCount: 3, OwnerCount: 2}, nil) handler := NewHandler(store, nil, nil, nil, "site-a", 1000, 500, 5*time.Second, func(_ context.Context, _ string, _ []byte) error { return fmt.Errorf("nats down") - }) + }, nil) reqSubj := subject.MemberRemove("alice", "r1", "site-a") body, _ := json.Marshal(model.RemoveMemberRequest{RoomID: "r1", Account: "alice"}) _, err := handler.handleRemoveMember(context.Background(), reqSubj, body) @@ -944,7 +944,7 @@ func TestHandler_AddMembers_RestrictedOwnerAllowed(t *testing.T) { store := NewMockRoomStore(ctrl) publish := func(_ context.Context, _ string, _ []byte) error { return nil } - h := NewHandler(store, nil, nil, nil, "site-a", 100, 500, 5*time.Second, publish) + h := NewHandler(store, nil, nil, nil, "site-a", 100, 500, 5*time.Second, publish, nil) store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{ Roles: []model.Role{model.RoleOwner}, @@ -971,7 +971,7 @@ func TestHandler_AddMembers_EmptyAfterResolve(t *testing.T) { store := NewMockRoomStore(ctrl) publish := func(_ context.Context, _ string, _ []byte) error { return nil } - h := NewHandler(store, nil, nil, nil, "site-a", 100, 500, 5*time.Second, publish) + h := NewHandler(store, nil, nil, nil, "site-a", 100, 500, 5*time.Second, publish, nil) store.EXPECT().GetSubscription(gomock.Any(), "alice", "r1").Return(&model.Subscription{ Roles: []model.Role{model.RoleMember}, @@ -2968,7 +2968,7 @@ func TestHandler_handleMessageReadReceipt(t *testing.T) { tt.prep(setup{store: store, reader: reader}) } - h := NewHandler(store, nil, nil, reader, siteID, 1000, 1000, time.Second, nil) + h := NewHandler(store, nil, nil, reader, siteID, 1000, 1000, time.Second, nil, nil) gotBytes, err := h.handleMessageReadReceipt(context.Background(), tt.subject, tt.body) if tt.wantErr != nil { diff --git a/room-service/main.go b/room-service/main.go index 5c8f5a91e..bcfb32708 100644 --- a/room-service/main.go +++ b/room-service/main.go @@ -119,12 +119,20 @@ func main() { cassReader := NewCassMessageReader(cassSession) memberListClient := NewNATSMemberListClient(nc.NatsConn(), cfg.MemberListTimeout) - handler := NewHandler(store, keyStore, memberListClient, cassReader, cfg.SiteID, cfg.MaxRoomSize, cfg.MaxBatchSize, cfg.MemberListTimeout, func(ctx context.Context, subj string, data []byte) error { - if _, err := js.PublishMsg(ctx, natsutil.NewMsg(ctx, subj, data)); err != nil { - return fmt.Errorf("publish to %q: %w", subj, err) - } - return nil - }) + handler := NewHandler(store, keyStore, memberListClient, cassReader, cfg.SiteID, cfg.MaxRoomSize, cfg.MaxBatchSize, cfg.MemberListTimeout, + func(ctx context.Context, subj string, data []byte) error { + if _, err := js.PublishMsg(ctx, natsutil.NewMsg(ctx, subj, data)); err != nil { + return fmt.Errorf("publish to %q: %w", subj, err) + } + return nil + }, + func(ctx context.Context, subj string, data []byte) error { + if err := nc.PublishMsg(ctx, natsutil.NewMsg(ctx, subj, data)); err != nil { + return fmt.Errorf("publish core to %q: %w", subj, err) + } + return nil + }, + ) if err := handler.RegisterCRUD(nc); err != nil { slog.Error("register CRUD handlers failed", "error", err) From a18ab35a9b85ddd67c7c4bd58d94b0b11ce9897f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 10:53:16 +0000 Subject: [PATCH 09/32] feat(room-service): add mute.toggle RPC handler https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- room-service/handler.go | 92 ++++++++++++++++++++ room-service/handler_test.go | 157 +++++++++++++++++++++++++++++++++++ 2 files changed, 249 insertions(+) diff --git a/room-service/handler.go b/room-service/handler.go index b19b3fc90..dae4f9c4e 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -100,6 +100,9 @@ func (h *Handler) RegisterCRUD(nc *otelnats.Conn) error { if _, err := nc.QueueSubscribe(subject.RoomKeyEnsure(h.siteID), queue, h.NatsHandleEnsureRoomKey); err != nil { return fmt.Errorf("subscribe room key ensure: %w", err) } + if _, err := nc.QueueSubscribe(subject.MuteToggleWildcard(h.siteID), queue, h.natsMuteToggle); err != nil { + return fmt.Errorf("subscribe mute toggle: %w", err) + } return nil } @@ -1219,3 +1222,92 @@ func (h *Handler) handleEnsureRoomKey(ctx context.Context, data []byte) ([]byte, Version: ver, }) } + +func (h *Handler) natsMuteToggle(m otelnats.Msg) { + ctx := wrappedCtx(m) + resp, err := h.handleMuteToggle(ctx, m.Msg.Subject, m.Msg.Data) + if err != nil { + slog.Error("mute toggle failed", "error", err, "subject", m.Msg.Subject) + natsutil.ReplyError(m.Msg, sanitizeError(err)) + return + } + if err := m.Msg.Respond(resp); err != nil { + slog.Error("failed to respond to mute toggle", "error", err) + } +} + +func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ([]byte, error) { + account, roomID, ok := subject.ParseUserRoomSubject(subj) + if !ok { + return nil, fmt.Errorf("invalid mute-toggle subject: %s", subj) + } + + sub, err := h.store.GetSubscription(ctx, account, roomID) + switch { + case errors.Is(err, model.ErrSubscriptionNotFound): + return nil, errNotRoomMember + case err != nil: + return nil, fmt.Errorf("get subscription: %w", err) + } + + newVal, err := h.store.ToggleSubscriptionMute(ctx, roomID, account) + if err != nil { + if errors.Is(err, model.ErrSubscriptionNotFound) { + return nil, errNotRoomMember + } + return nil, fmt.Errorf("toggle subscription mute: %w", err) + } + + now := time.Now().UTC() + + updatedSub := *sub + updatedSub.DisableNotifications = newVal + subEvt := model.SubscriptionUpdateEvent{ + UserID: sub.User.ID, + Subscription: updatedSub, + Action: "mute_toggled", + Timestamp: now.UnixMilli(), + } + subEvtData, err := json.Marshal(subEvt) + if err != nil { + return nil, fmt.Errorf("marshal subscription update event: %w", err) + } + if err := h.publishCore(ctx, subject.SubscriptionUpdate(account), subEvtData); err != nil { + slog.Error("subscription update publish failed", "error", err, "account", account) + // Non-fatal — the DB write is the source of truth; clients will reconcile on next refetch. + } + + userSiteID, err := h.store.GetUserSiteID(ctx, account) + if err != nil { + slog.Warn("get user siteId failed; skipping outbox", "error", err, "account", account) + return json.Marshal(model.MuteToggleResponse{Status: "ok", DisableNotifications: newVal}) + } + if userSiteID != "" && userSiteID != h.siteID { + payload := model.SubscriptionMuteToggledEvent{ + Account: account, + RoomID: roomID, + DisableNotifications: newVal, + Timestamp: now.UnixMilli(), + } + payloadData, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("marshal mute-toggled payload: %w", err) + } + outbox := model.OutboxEvent{ + Type: model.OutboxSubscriptionMuteToggled, + SiteID: h.siteID, + DestSiteID: userSiteID, + Payload: payloadData, + Timestamp: now.UnixMilli(), + } + outboxData, err := json.Marshal(outbox) + if err != nil { + return nil, fmt.Errorf("marshal outbox event: %w", err) + } + if err := h.publishToStream(ctx, subject.Outbox(h.siteID, userSiteID, model.OutboxSubscriptionMuteToggled), outboxData); err != nil { + return nil, fmt.Errorf("publish mute-toggled outbox: %w", err) + } + } + + return json.Marshal(model.MuteToggleResponse{Status: "ok", DisableNotifications: newVal}) +} diff --git a/room-service/handler_test.go b/room-service/handler_test.go index 34e121b14..24c0ae9c7 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -3201,3 +3201,160 @@ func TestHandler_EnsureRoomKey_NilKeyStore(t *testing.T) { _, err := h.handleEnsureRoomKey(context.Background(), data) require.Error(t, err) } + +func TestHandler_MuteToggle_Success(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(&model.Subscription{ + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + }, nil) + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(true, nil) + store.EXPECT(). + GetUserSiteID(gomock.Any(), "alice"). + Return("site-a", nil) // same site → no outbox publish + + var coreSubjects []string + var coreBodies [][]byte + h := &Handler{ + store: store, + siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { + t.Fatal("publishToStream must not be called for same-site mute toggle") + return nil + }, + publishCore: func(_ context.Context, subj string, data []byte) error { + coreSubjects = append(coreSubjects, subj) + coreBodies = append(coreBodies, data) + return nil + }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + resp, err := h.handleMuteToggle(context.Background(), subj, nil) + require.NoError(t, err) + + var got model.MuteToggleResponse + require.NoError(t, json.Unmarshal(resp, &got)) + assert.Equal(t, "ok", got.Status) + assert.True(t, got.DisableNotifications) + + require.Len(t, coreSubjects, 1) + assert.Equal(t, subject.SubscriptionUpdate("alice"), coreSubjects[0]) + + var evt model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(coreBodies[0], &evt)) + assert.Equal(t, "mute_toggled", evt.Action) + assert.True(t, evt.Subscription.DisableNotifications) + assert.Equal(t, "alice", evt.Subscription.User.Account) +} + +func TestHandler_MuteToggle_CrossSitePublishesOutbox(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(&model.Subscription{ + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", SiteID: "site-a", + }, nil) + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(true, nil) + store.EXPECT(). + GetUserSiteID(gomock.Any(), "alice"). + Return("site-b", nil) + + var streamSubj string + var streamData []byte + h := &Handler{ + store: store, siteID: "site-a", + publishToStream: func(_ context.Context, s string, d []byte) error { + streamSubj = s + streamData = d + return nil + }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + _, err := h.handleMuteToggle(context.Background(), subj, nil) + require.NoError(t, err) + + assert.Equal(t, subject.Outbox("site-a", "site-b", model.OutboxSubscriptionMuteToggled), streamSubj) + + var outbox model.OutboxEvent + require.NoError(t, json.Unmarshal(streamData, &outbox)) + assert.Equal(t, model.OutboxSubscriptionMuteToggled, outbox.Type) + assert.Equal(t, "site-a", outbox.SiteID) + assert.Equal(t, "site-b", outbox.DestSiteID) + + var payload model.SubscriptionMuteToggledEvent + require.NoError(t, json.Unmarshal(outbox.Payload, &payload)) + assert.Equal(t, "alice", payload.Account) + assert.Equal(t, "r1", payload.RoomID) + assert.True(t, payload.DisableNotifications) + assert.NotZero(t, payload.Timestamp) +} + +func TestHandler_MuteToggle_NotRoomMember(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(nil, model.ErrSubscriptionNotFound) + + h := &Handler{ + store: store, siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { return nil }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + _, err := h.handleMuteToggle(context.Background(), subj, nil) + assert.ErrorIs(t, err, errNotRoomMember) +} + +func TestHandler_MuteToggle_InvalidSubject(t *testing.T) { + h := &Handler{ + siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { return nil }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + _, err := h.handleMuteToggle(context.Background(), "garbage.subject", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid mute-toggle subject") +} + +func TestHandler_MuteToggle_StoreError(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(&model.Subscription{ + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, RoomID: "r1", + }, nil) + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(false, fmt.Errorf("db down")) + + h := &Handler{ + store: store, siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { return nil }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + subj := subject.MuteToggle("alice", "r1", "site-a") + _, err := h.handleMuteToggle(context.Background(), subj, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "toggle subscription mute") +} From 9ae124d9f4036e13de96051e3dc73bc26ab7e7b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:00:25 +0000 Subject: [PATCH 10/32] feat(inbox-worker): mirror subscription_mute_toggled outbox event https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- inbox-worker/handler.go | 21 +++++++++++ inbox-worker/handler_test.go | 72 ++++++++++++++++++++++++++++++++++++ inbox-worker/main.go | 13 +++++++ 3 files changed, 106 insertions(+) diff --git a/inbox-worker/handler.go b/inbox-worker/handler.go index 3fa5cf91c..4767973d4 100644 --- a/inbox-worker/handler.go +++ b/inbox-worker/handler.go @@ -29,6 +29,10 @@ type InboxStore interface { // silent no-ops. Missing-subscription is also a silent no-op. UpdateSubscriptionRead(ctx context.Context, roomID, account string, lastSeenAt time.Time, alert bool) error UpsertThreadSubscription(ctx context.Context, sub *model.ThreadSubscription) error + // UpdateSubscriptionMute sets disableNotifications on the subscription keyed + // by (roomID, account). Missing-subscription is a silent no-op so federation + // races during membership delete don't surface errors. + UpdateSubscriptionMute(ctx context.Context, roomID, account string, disableNotifications bool) error } // Handler processes cross-site OutboxEvent messages; replicates only subscription/room metadata, never room keys. @@ -59,6 +63,8 @@ func (h *Handler) HandleEvent(ctx context.Context, data []byte) error { return h.handleRoleUpdated(ctx, &evt) case "subscription_read": return h.handleSubscriptionRead(ctx, &evt) + case "subscription_mute_toggled": + return h.handleSubscriptionMuteToggled(ctx, &evt) case "thread_subscription_upserted": return h.handleThreadSubscriptionUpserted(ctx, &evt) default: @@ -199,6 +205,21 @@ func (h *Handler) handleSubscriptionRead(ctx context.Context, evt *model.OutboxE return nil } +// handleSubscriptionMuteToggled mirrors a mute toggle from the room's home +// site onto the user's home-site subscription copy. Missing-subscription is +// a silent no-op (the store enforces this) so a federation race between a +// membership delete and a mute toggle does not error the consumer. +func (h *Handler) handleSubscriptionMuteToggled(ctx context.Context, evt *model.OutboxEvent) error { + var e model.SubscriptionMuteToggledEvent + if err := json.Unmarshal(evt.Payload, &e); err != nil { + return fmt.Errorf("unmarshal subscription_mute_toggled payload: %w", err) + } + if err := h.store.UpdateSubscriptionMute(ctx, e.RoomID, e.Account, e.DisableNotifications); err != nil { + return fmt.Errorf("update subscription mute for %q in room %q: %w", e.Account, e.RoomID, err) + } + 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 diff --git a/inbox-worker/handler_test.go b/inbox-worker/handler_test.go index 7781b6ddc..88e612518 100644 --- a/inbox-worker/handler_test.go +++ b/inbox-worker/handler_test.go @@ -143,6 +143,18 @@ func (s *stubInboxStore) BulkCreateSubscriptions(_ context.Context, subs []*mode return nil } +func (s *stubInboxStore) UpdateSubscriptionMute(_ context.Context, roomID, account string, disableNotifications bool) error { + s.mu.Lock() + defer s.mu.Unlock() + for i := range s.subscriptions { + if s.subscriptions[i].RoomID == roomID && s.subscriptions[i].User.Account == account { + s.subscriptions[i].DisableNotifications = disableNotifications + return nil + } + } + return nil // missing-subscription → no-op +} + func (s *stubInboxStore) UpdateSubscriptionRead(_ context.Context, roomID, account string, lastSeenAt time.Time, alert bool) error { s.mu.Lock() defer s.mu.Unlock() @@ -1190,3 +1202,63 @@ func TestHandleMemberAdded_BulkCreate_NonDuplicateError_ReturnsError(t *testing. require.Error(t, err) assert.Contains(t, err.Error(), "bulk create subscriptions") } + +func TestHandler_SubscriptionMuteToggled(t *testing.T) { + store := &stubInboxStore{ + subscriptions: []model.Subscription{ + { + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + }, + }, + } + h := NewHandler(store) + + payload, err := json.Marshal(model.SubscriptionMuteToggledEvent{ + Account: "alice", RoomID: "r1", DisableNotifications: true, Timestamp: 12345, + }) + require.NoError(t, err) + evt, err := json.Marshal(model.OutboxEvent{ + Type: model.OutboxSubscriptionMuteToggled, SiteID: "site-a", DestSiteID: "site-b", + Payload: payload, Timestamp: 12345, + }) + require.NoError(t, err) + + require.NoError(t, h.HandleEvent(context.Background(), evt)) + + subs := store.getSubscriptions() + require.Len(t, subs, 1) + assert.True(t, subs[0].DisableNotifications) +} + +func TestHandler_SubscriptionMuteToggled_MissingSubscriptionNoOp(t *testing.T) { + store := &stubInboxStore{} + h := NewHandler(store) + + payload, err := json.Marshal(model.SubscriptionMuteToggledEvent{ + Account: "ghost", RoomID: "r1", DisableNotifications: true, Timestamp: 12345, + }) + require.NoError(t, err) + evt, err := json.Marshal(model.OutboxEvent{ + Type: model.OutboxSubscriptionMuteToggled, SiteID: "site-a", DestSiteID: "site-b", + Payload: payload, Timestamp: 12345, + }) + require.NoError(t, err) + + // missing subscription must be a silent no-op, not an error + require.NoError(t, h.HandleEvent(context.Background(), evt)) +} + +func TestHandler_SubscriptionMuteToggled_MalformedPayload(t *testing.T) { + store := &stubInboxStore{} + h := NewHandler(store) + + evt, err := json.Marshal(model.OutboxEvent{ + Type: model.OutboxSubscriptionMuteToggled, + Payload: []byte("not-json"), + }) + require.NoError(t, err) + + require.Error(t, h.HandleEvent(context.Background(), evt)) +} diff --git a/inbox-worker/main.go b/inbox-worker/main.go index 6cab03b91..469387b15 100644 --- a/inbox-worker/main.go +++ b/inbox-worker/main.go @@ -115,6 +115,19 @@ func (s *mongoInboxStore) BulkCreateSubscriptions(ctx context.Context, subs []*m return nil } +// UpdateSubscriptionMute sets disableNotifications on the subscription keyed +// by (roomID, account). Missing-subscription is a silent no-op. +func (s *mongoInboxStore) UpdateSubscriptionMute(ctx context.Context, roomID, account string, disableNotifications bool) error { + _, err := s.subCol.UpdateOne(ctx, + bson.M{"roomId": roomID, "u.account": account}, + bson.M{"$set": bson.M{"disableNotifications": disableNotifications}}, + ) + if err != nil { + return fmt.Errorf("update subscription mute for %q in room %q: %w", account, roomID, err) + } + return nil +} + func (s *mongoInboxStore) UpdateSubscriptionRead(ctx context.Context, roomID, account string, lastSeenAt time.Time, alert bool) error { filter := bson.M{ "roomId": roomID, From 82774888b22ffbb04b108f652d2f0db65fe44693 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:03:35 +0000 Subject: [PATCH 11/32] docs(client-api): document mute.toggle RPC --- docs/client-api.md | 49 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/docs/client-api.md b/docs/client-api.md index 6689bec8d..89618e0b9 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -782,6 +782,55 @@ See [Error envelope](#6-error-envelope-reference). Common errors: --- +#### Toggle Mute + +**Subject:** `chat.user.{account}.request.room.{roomID}.{siteID}.mute.toggle` +**Reply subject:** auto-generated `_INBOX.>` (NATS request/reply) + +Synchronous RPC. `room-service` flips `Subscription.disableNotifications` for the requester in a single atomic Mongo `FindOneAndUpdate`, replies with the resulting value, fans out a `subscription.update` event to the user's other client sessions, and (for cross-site users) publishes a `subscription_mute_toggled` outbox event so `inbox-worker` mirrors the change on the user's home site. + +Idempotency: this is a toggle, not a set — every successful call flips the bit. Clients must debounce the user-visible action; redelivery of the same RPC will flip back. + +##### Request body + +The subject already carries `account` and `roomID`, so no body fields are required. Clients may send `{}` or omit the body entirely; any body content is ignored. + +##### Success response + +| Field | Type | Notes | +|------------------------|---------|-------| +| `status` | string | Always `"ok"`. | +| `disableNotifications` | boolean | The resulting value of `Subscription.disableNotifications` after the flip. | + +```json +{ "status": "ok", "disableNotifications": true } +``` + +##### Error response + +See [Error envelope](#6-error-envelope-reference). Common errors: + +- `"only room members can list members"` — the user has no subscription in the room (sentinel reused across membership-gated RPCs). +- `"invalid mute-toggle subject: …"` — the subject is malformed. + +##### Triggered events — success path + +**`chat.user.{account}.event.subscription.update`** — emitted once for the requester so other client sessions reconcile. + +| Field | Type | Notes | +|----------------|--------|-------| +| `userId` | string | The requester's internal user ID. | +| `subscription` | object | The `Subscription` record with the updated `disableNotifications`. | +| `action` | string | `"mute_toggled"`. | +| `timestamp` | number | Milliseconds since Unix epoch (UTC). | + +##### Behaviour notes + +- **Cross-site federation:** if the user's home site (`users.siteId`) differs from the handler's site, a `subscription_mute_toggled` outbox event is published to `outbox.{handlerSite}.to.{userSite}.subscription_mute_toggled` with payload `{account, roomId, disableNotifications, timestamp}`. The destination `inbox-worker` applies an unconditional `$set` on the local subscription mirror. +- **Notification delivery:** `notification-worker` does **not** yet consult `disableNotifications` before sending. End-to-end mute behaviour is wired only as far as the persisted flag; honouring it in fan-out is a follow-up. + +--- + #### Read Message Receipts **Subject:** `chat.user.{account}.request.room.{roomID}.{siteID}.message.read-receipt` From d0bdc03d8d1e7c79085620d880ed50cded90c5de Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:11:50 +0000 Subject: [PATCH 12/32] review(branch): header + executive summary --- ...e-explore-room-service-7tNlq-2026-05-21.md | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md new file mode 100644 index 000000000..62ceb76f3 --- /dev/null +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -0,0 +1,47 @@ +# Branch Review: `claude/explore-room-service-7tNlq` + +**Date:** 2026-05-21 +**Base:** `fd7b4e2` (last commit before this branch on the upstream lineage) +**HEAD:** `8277488` +**Commits on branch:** 11 (plan docs + 9 implementation commits) +**Author:** Claude + +## Scope + +Adds a new per-user `mute.toggle` RPC to `room-service`, mirrors the toggle across sites via `inbox-worker`, renames `Subscription.DisableNotification` → `DisableNotifications` repo-wide, and documents the new RPC in `docs/client-api.md`. + +## Services touched + +| Service | Magnitude | Note | +|---|---|---| +| `room-service` | substantive | New handler + store method + `publishCore` wiring + 5 unit tests + 1 integration test | +| `inbox-worker` | medium | New dispatch case + store method + 3 unit tests | +| `room-worker` | minimal | Field-rename in one integration test only — no production code touched | + +Shared `pkg/` files also changed: `pkg/model/event.go`, `pkg/model/subscription.go`, `pkg/model/model_test.go`, `pkg/subject/subject.go`, `pkg/subject/subject_test.go`. + +## Findings count (deduped across all lenses) + +| Severity | Count | +|---|---| +| critical | 0 | +| high | 1 | +| medium | 10 | +| low | 9 | +| nitpick | 6 | + +## Top-line risk assessment + +**Functionally correct and mergeable after addressing a small set of issues.** The implementation faithfully follows the `message.read` precedent (Pattern B): inline write in `room-service`, cross-site outbox publish, inbox-worker mirror. SAST is clean (gosec PASS, govulncheck PASS; semgrep unavailable in env — not a code defect). `make test` and `make lint` both green. The integration test for the new store method passes. + +The notable items to address before merge: + +1. **A reachable bug where `GetUserSiteID` errors silently abandon the cross-site outbox publish** (3 reviewers independently flagged this), causing permanent cross-site mute divergence on transient DB hiccups. Inconsistent with `handleMessageRead`'s precedent which propagates the error. +2. **`sanitizeError` doesn't pass through the new `"invalid mute-toggle subject: …"` error string**, so clients receive `"internal error"` instead of the message documented in this same PR's `client-api.md` update — the API contract is broken at delivery. +3. **`SubscriptionUpdateEvent.Action` field comment is now stale** — claims `"added" | "removed"` but the new code adds `"mute_toggled"` (and earlier work added `"role_updated"`). Client-facing contract drift. +4. A TOCTOU window between `GetSubscription` and `ToggleSubscriptionMute` that adds a redundant Mongo round-trip per request. +5. Missing tests for several error paths (outbox publish failure, `GetSubscription` generic error, `publishCore` failure, `GetUserSiteID` failure soft path) — coverage of `handleMuteToggle` lands at ~70–75% vs the 90%+ target for core handler logic. + +None of these are blocking-critical; the branch can ship after a small fix-up commit. + +--- From 5671f1ff7b6ca738954c8283de8c1163f56a28ea Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:12:14 +0000 Subject: [PATCH 13/32] review(branch): service room-service --- ...e-explore-room-service-7tNlq-2026-05-21.md | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index 62ceb76f3..f4602f7b3 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -45,3 +45,45 @@ The notable items to address before merge: None of these are blocking-critical; the branch can ship after a small fix-up commit. --- + +## Service: room-service + +### (a) Diff correctness against existing conventions + +**[medium]** `room-service/handler.go:1230` — `natsMuteToggle` logs `slog.Error("mute toggle failed", "error", err, "subject", m.Msg.Subject)` with an extra `subject` structured field that the analogous `natsMessageRead` (line 971) and `natsUpdateRole` etc. do not. Either propagate the field across all handlers or drop it here. Inconsistency only. + +**[nitpick]** `room-service/handler.go:1253-1255` — `handleMuteToggle` calls `ToggleSubscriptionMute` and re-checks `ErrSubscriptionNotFound` even though the preceding `GetSubscription` guard at line 1245 should prevent that branch. Dead-code defense-in-depth that could mislead a future reader about atomicity guarantees. + +### (b) Scope drift / refactor-readiness + +**[low]** room-service is cohesive; `mute.toggle` is a per-subscription mutation that the service already owns (same `subscriptions` collection used by `UpdateSubscriptionRead`). No scope drift. `handler.go` is large (~1300 lines) but already was — the new handler follows the established length pattern. No split needed. + +### (c) Abstraction changes + +**[low]** `publishCore` closure injection is justified. room-service already had `publishToStream` for JetStream; core-NATS publish for `subscription.update` fan-out needs a separate path because the APIs differ (`js.PublishMsg` returns `(*PubAck, error)`, `nc.PublishMsg` returns only `error`). The shape mirrors `publishToStream`, it's injected in `main.go` at line 119, and existing tests that don't exercise it pass `nil`. No premature abstraction. + +**[low]** `NewHandler` now has 10 positional parameters (`handler.go:44`). Borderline; a config struct would be cleaner. Pre-existing issue, not introduced by this PR but worsened by it. + +### (d) Design coherence + +The mute.toggle flow is a textbook match for room-service's job: validate membership → atomic Mongo write → fan out `subscription.update` core event → cross-site outbox. The plan's Pattern-B (`message.read` shape) is exactly what landed. + +### (e) Project-pattern adherence + +- **`pkg/subject` builders**: `subject.MuteToggleWildcard`, `subject.MuteToggle`, `subject.ParseUserRoomSubject`, `subject.SubscriptionUpdate`, `subject.Outbox` — all used correctly; no raw `fmt.Sprintf` subjects in new code. ✓ +- **Outbox pattern**: `publishToStream` called with `subject.Outbox(h.siteID, userSiteID, model.OutboxSubscriptionMuteToggled)` at line 1307. ✓ +- **`Timestamp int64` on new event structs**: `SubscriptionMuteToggledEvent.Timestamp` set at the publish site (`now.UnixMilli()`, line 1290). `OutboxEvent.Timestamp` set at line 1301. `SubscriptionUpdateEvent.Timestamp` set at line 1269. All correct. ✓ + +**[medium]** `room-service/handler.go:1280-1283` — When `GetUserSiteID` errors, the handler logs `slog.Warn` and returns `{status: "ok"}` while silently dropping the cross-site outbox publish. The peer handler `handleMessageRead` (line 1023) treats `GetUserSiteID` failure as a hard error returned to the caller. The asymmetry means a transient DB failure on `users` causes silent cross-site divergence with no client-visible signal — the user mutes on the room site but their home site keeps the old value forever. Either match `handleMessageRead`'s hard-error behaviour or document the intentional degraded-mode trade-off (and consider a retry/backfill mechanism). + +### (f) Client-API doc rule + +`docs/client-api.md` is updated in this branch with a full new "Toggle Mute" section (50 lines). Subject, request body, success response, error cases, triggered events, cross-site behaviour, and the `notification-worker` follow-up caveat are all documented. ✓ + +**[low]** The docs section lists the not-a-member error as `"only room members can list members"` (the reused `errNotRoomMember` sentinel from `helper.go:25`). The string is semantically misleading for a mute operation. Either rename the sentinel or call out the reuse in the docs note. + +### Verdict + +The implementation is functionally correct and pattern-compliant. The one actionable blocker is the silent skip of the cross-site outbox on `GetUserSiteID` error (`handler.go:1280-1283`), which diverges from `handleMessageRead`'s precedent and risks undetected cross-site inconsistency. + +--- From 9c9e0b3b9007d4e2d669d1bc7c12fbc0cd45479f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:12:39 +0000 Subject: [PATCH 14/32] review(branch): service inbox-worker --- ...e-explore-room-service-7tNlq-2026-05-21.md | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index f4602f7b3..3f8f680ae 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -87,3 +87,45 @@ The mute.toggle flow is a textbook match for room-service's job: validate member The implementation is functionally correct and pattern-compliant. The one actionable blocker is the silent skip of the cross-site outbox on `GetUserSiteID` error (`handler.go:1280-1283`), which diverges from `handleMessageRead`'s precedent and risks undetected cross-site inconsistency. --- + +## Service: inbox-worker + +### (a) Diff correctness against existing handler conventions + +**[low]** The new `handleSubscriptionMuteToggled` (`handler.go:212-221`) is structurally identical to `handleSubscriptionRead` (`handler.go:196-206`): unmarshal inner event → call store method → wrap error with `"short description: %w"`. Dispatch case is placed immediately after `subscription_read` in the switch, mirroring how `subscription_read` was slotted between `role_updated` and `thread_subscription_upserted`. Shape correct, no issues. + +### (b) Scope drift / refactor-readiness + +**[medium]** `inbox-worker/main.go:39-195` — The `mongoInboxStore` struct and all its methods remain embedded in `main.go`, which now spans ~200 lines of store implementation before `main()` even starts. The plan (Task 8 step 1) explicitly noted: "Modify: `inbox-worker/store.go` (interface) and `inbox-worker/store_mongo.go` (impl) — confirm exact filenames first." The split never happened. Adding `UpdateSubscriptionMute` to `main.go:118-129` follows the existing pattern, but the growing inline store is now clearly beyond the per-service layout in CLAUDE.md (`main.go` = config + wiring + startup). Pre-existing tech debt that this PR adds to. Worth a follow-up split before the next handler grows the inline store further. + +### (c) Abstraction changes + +**[low]** `UpdateSubscriptionMute` earns its keep. It parallels `UpdateSubscriptionRead` one-to-one in the interface (`handler.go:33-36`) and in the Mongo impl (`main.go:120-129`). The no-op-on-missing-subscription semantic is documented in both the interface comment and the store comment. + +**[nitpick]** `inbox-worker/main.go:128` — `UpdateSubscriptionMute` discards `res *UpdateResult` (assigns to `_`) and does NOT check `res.MatchedCount` before returning nil, while `UpdateSubscriptionRoles` (`main.go:60-70`) DOES return an error on `MatchedCount == 0`. The asymmetry is intentional (plan explicitly requires silent no-op on missing subscription) and is documented in both the interface and store comments — but a one-line inline comment (`// MatchedCount 0 is intentional — missing subscription is a federation-race no-op`) on the `return nil` would close that reading ambiguity for the next developer. + +### (d) Design coherence + +**[low]** The handler correctly fits inbox-worker's stated job: mirror a room's-home-site write onto the user's home-site subscription copy. The `OutboxEvent.Type == "subscription_mute_toggled"` dispatch case (`handler.go:66-67`) wires correctly against the constant `model.OutboxSubscriptionMuteToggled`. The silent-no-op semantic is encoded end-to-end (store Mongo impl → store interface doc → handler comment). No design issues. + +### (e) Project-pattern adherence + +- **Federation wiring**: dispatch string `"subscription_mute_toggled"` matches `model.OutboxSubscriptionMuteToggled` (`pkg/model/event.go:85`). `handler_test.go` uses the const directly, not a bare string. ✓ +- **Mongo write**: direct driver, `bson.M` filter + `$set` update, no ORM. ✓ +- **Error wrapping**: `"unmarshal subscription_mute_toggled payload: %w"` and `"update subscription mute for %q in room %q: %w"` — both follow the `"short description: %w"` convention. ✓ + +**[low]** The plan prescribed `gomock` + `make generate SERVICE=inbox-worker` for the new test (Task 8 steps 5/8). The landed implementation uses a hand-written `stubInboxStore` in `handler_test.go` instead. The plan was wrong about inbox-worker's testing style — the landed code correctly matches the existing convention. Worth noting for future plans against this service. + +### (f) Client-API doc rule + +N/A — inbox-worker has no `nc.QueueSubscribe` handlers on `chat.user.…` client-facing subjects. + +### Test coverage + +Three targeted tests cover the happy path (`TestHandler_SubscriptionMuteToggled`), missing-subscription no-op (`TestHandler_SubscriptionMuteToggled_MissingSubscriptionNoOp`), and malformed payload (`TestHandler_SubscriptionMuteToggled_MalformedPayload`). Missing: a store-error propagation test (see Test-automation chapter). + +### Verdict + +The diff is mechanically correct and safe to merge. Actionable items: a one-line clarity comment on the intentional `MatchedCount` omission (`main.go:128`), and a follow-up PR to split the inline store from `main.go` into `store.go`/`store_mongo.go` before the next handler grows it further. + +--- From fc007fa155ea574960a97224ddb0ae2d453c02c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:12:50 +0000 Subject: [PATCH 15/32] review(branch): service room-worker --- ...e-explore-room-service-7tNlq-2026-05-21.md | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index 3f8f680ae..1ef0643c7 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -129,3 +129,24 @@ Three targeted tests cover the happy path (`TestHandler_SubscriptionMuteToggled` The diff is mechanically correct and safe to merge. Actionable items: a one-line clarity comment on the intentional `MatchedCount` omission (`main.go:128`), and a follow-up PR to split the inline store from `main.go` into `store.go`/`store_mongo.go` before the next handler grows it further. --- + +## Service: room-worker + +Scope: Test-only change — `room-worker/integration_test.go` had 8 occurrences of `DisableNotification` renamed to `DisableNotifications` (matching the field-rename in `pkg/model/subscription.go:42`). No production code in `room-worker/` was touched. + +### Findings + +All 8 occurrences are renamed and semantically equivalent: +- Struct-literal fixtures (lines ~1320, ~1397): the boolean values are preserved. +- `assert.True` calls and their message strings (lines ~1350, ~1352, ~1432, ~1434): same field, new name. +- Doc-comment references (lines ~1288, ~1365): updated for accuracy. + +The cosmetic alignment adjustment (one space → two spaces after the colon to maintain column alignment with surrounding fields) is consistent with the rest of the struct literal. + +The test continues to use `testutil.MongoDB`, `testutil.NATS`, and a `TestMain` with `testutil.RunTests(m)` — consistent with CLAUDE.md Section 4. No inline container usage was introduced. Scope discipline maintained. + +### Verdict + +No findings. The change is a correct, complete mechanical rename with no omissions or semantic drift — approved as-is. + +--- From b65c67a3fb5a402a7c7fc3717ea47dd46693e335 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:13:15 +0000 Subject: [PATCH 16/32] review(branch): go expert --- ...e-explore-room-service-7tNlq-2026-05-21.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index 1ef0643c7..cebcf851b 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -150,3 +150,31 @@ The test continues to use `testutil.MongoDB`, `testutil.NATS`, and a `TestMain` No findings. The change is a correct, complete mechanical rename with no omissions or semantic drift — approved as-is. --- + +## Go expert + +### Findings + +**[high]** `room-service/handler.go:1245-1264` — TOCTOU window between the pre-fetch and the atomic toggle. `GetSubscription` exists to (a) gate membership and (b) supply the full `Subscription` struct for the downstream `SubscriptionUpdateEvent`. `ToggleSubscriptionMute` already returns `ErrSubscriptionNotFound` on a missing document, so the pre-fetch primarily serves the event payload. Between the two calls a concurrent writer could delete the subscription (yielding a different error class) or update another field — the constructed `updatedSub := *sub` copy then carries pre-fetch values for everything except `DisableNotifications`, which is the post-flip value. Either project the necessary fields directly in `ToggleSubscriptionMute`'s `FindOneAndUpdate` (eliminating the pre-fetch) or accept the staleness with a comment acknowledging it. + +**[medium]** `pkg/model/event.go:35` — `SubscriptionUpdateEvent.Action` comment says `// "added" | "removed"`. This branch introduces `"mute_toggled"` (line 1267 in `room-service/handler.go`), and earlier work already added `"role_updated"`. Clients parsing this field rely on the comment as the contract. Update to `// "added" | "removed" | "role_updated" | "mute_toggled"`. + +**[medium]** `room-service/handler.go:1242` — `fmt.Errorf("invalid mute-toggle subject: %s", subj)` is not sentinel-wrapped, so `errors.Is(err, errInvalidSubject)` cannot match. The codebase already uses sentinels elsewhere (`errInvalidRole`, `errNotRoomMember`, etc.). Introducing an `errInvalidMuteToggleSubject` sentinel would also fix the `sanitizeError` passthrough issue flagged in the Bug & Security chapter. + +**[medium]** Missing test for the cross-site outbox `publishToStream` failure path. `handleMuteToggle` returns a hard error (`"publish mute-toggled outbox: %w"`) at line 1308 if the JetStream publish fails after the DB write committed. `TestHandler_MuteToggle_CrossSitePublishesOutbox` only covers the success path; the analogous `TestHandler_MessageRead_CrossSite_PublishFailureAborts` shows the pattern to follow. + +**[medium]** `room-service/handler.go:1280-1283` vs `1305-1308` — Asymmetric error handling: `GetUserSiteID` failure silently returns OK, but `publishToStream` failure on the same cross-site path returns a hard error. The asymmetry is defensible (publish has JetStream retries; the users-collection read does not) but it's undocumented and surprises a reader. Add an inline comment explaining the trade-off. + +**[low]** `room-service/handler.go:44` — `NewHandler` now has 10 positional parameters, including two adjacent `func(context.Context, string, []byte) error` closures (`publishToStream`, `publishCore`). They can be silently swapped at the call site with no compile error. A `HandlerConfig` struct or functional options would be more idiomatic at this arity. Not introduced by this PR but worsened by it. + +**[low]** `pkg/model/event.go:225` — `SubscriptionMuteToggledEvent` carries `bson` tags it will never use (the struct is only JSON-encoded as `OutboxEvent.Payload`, never stored in Mongo). Consistent with `SubscriptionReadEvent` on line 94, so accepted as pattern-conformance noise. + +**[nitpick]** `pkg/model/event.go:219` — `MuteToggleResponse.Status` is always the constant `"ok"`. Encoding a fixed string as a struct field rather than a typed const means it can only be tested by string comparison. Cosmetic. + +**[nitpick]** `inbox-worker/main.go:120-129` — `UpdateSubscriptionMute` implementation lives in `main.go` rather than a `store_mongo.go`. Consistent with the pre-existing `UpdateSubscriptionRead` on the same `mongoInboxStore`, but the pattern itself deviates from the per-service layout in CLAUDE.md. Out of scope for this PR; see inbox-worker chapter for the follow-up split recommendation. + +### Verdict + +Logically sound and well-tested for happy and most error paths. The TOCTOU window in the two-stage subscription lookup, the stale `Action` enum comment (a client-facing contract), and the missing outbox-failure test case should all be addressed before merge. + +--- From 0c9bc9f05bbb2092434459dadfb69e731a3e27a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:13:40 +0000 Subject: [PATCH 17/32] review(branch): test automation --- ...e-explore-room-service-7tNlq-2026-05-21.md | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index cebcf851b..e519cbe1c 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -178,3 +178,58 @@ No findings. The change is a correct, complete mechanical rename with no omissio Logically sound and well-tested for happy and most error paths. The TOCTOU window in the two-stage subscription lookup, the stale `Action` enum comment (a client-facing contract), and the missing outbox-failure test case should all be addressed before merge. --- + +## Test automation + +### Mock staleness check + +`make generate SERVICE=room-service` and `make generate SERVICE=inbox-worker` both produced **zero diff**. Mocks are fresh. Working tree clean after the check. + +### TDD compliance for new exported functions + +| New exported function | Test | +|---|---| +| `subject.MuteToggle` | `TestMuteToggle` ✓ | +| `subject.MuteToggleWildcard` | `TestMuteToggleWildcard` ✓ | +| `model.MuteToggleResponse` | `TestMuteToggleResponseJSON` ✓ | +| `model.SubscriptionMuteToggledEvent` | `TestSubscriptionMuteToggledEventJSON` ✓ | +| `model.OutboxSubscriptionMuteToggled` const | `TestOutboxSubscriptionMuteToggledConst` ✓ | +| `MongoStore.ToggleSubscriptionMute` (room-service) | `TestMongoStore_ToggleSubscriptionMute` ✓ | +| `mongoInboxStore.UpdateSubscriptionMute` (inbox-worker) | **MISSING — see [low] below** | + +All other new functions are unexported. TDD compliance: PASS at the unit level, partial gap at integration level. + +### Findings + +**[medium]** `room-service/handler_test.go` — `handleMuteToggle`'s `GetSubscription` **generic** error arm (`case err != nil` at `handler.go:1249`, returns `"get subscription: %w"`) has no test. `TestHandler_MuteToggle_NotRoomMember` covers `ErrSubscriptionNotFound` but the generic-error path is untested. The analogous `TestHandler_MessageRead_GetRoomError` shows the expected pattern. + +**[medium]** `room-service/handler_test.go` — `handleMuteToggle`'s **cross-site outbox `publishToStream` failure** is untested. The handler returns a hard error (`"publish mute-toggled outbox: %w"`) at line 1308; `TestHandler_MuteToggle_CrossSitePublishesOutbox` only covers success. See `TestHandler_MessageRead_CrossSite_PublishFailureAborts` for the pattern. + +**[low]** `room-service/handler_test.go` — `GetUserSiteID` non-fatal soft path untested. When `GetUserSiteID` returns an error, the handler logs a warning and still returns `"ok"`. This unusual "success despite store error" behaviour deserves a test to pin the intent, especially since it's inconsistent with `handleMessageRead`'s hard-error treatment of the same call. + +**[low]** `room-service/handler_test.go` — `publishCore` non-fatal failure untested. Line 1275-1277 swallows the error with `slog.Error` and continues. No test asserts that the handler still returns `"ok"` when `publishCore` fails. + +**[low]** `inbox-worker/handler_test.go` — `UpdateSubscriptionMute` **store-error propagation** untested. `handleSubscriptionMuteToggled` (line 217) wraps store errors and returns them. The existing pattern in the same file uses small embedded-stub types (e.g. `errorDeleteStore`, `errorThreadSubStore`) to inject store errors. Missing: a `TestHandler_SubscriptionMuteToggled_StoreError` analogue. + +**[low]** `inbox-worker/integration_test.go` — `mongoInboxStore.UpdateSubscriptionMute` has no integration test. All other `mongoInboxStore` methods have counterpart integration tests; this method was added to `main.go` without one. Below the 80% coverage floor required by CLAUDE.md §4 for new Mongo code paths. + +**[nitpick]** `room-service/handler_test.go:3205-3360` — Five flat `TestHandler_MuteToggle_*` functions could be a single table-driven test. They share the same call site (`h.handleMuteToggle`) and differ only in setup/expectation. The file already shows the table-driven style (`TestHandler_handleMessageReadReceipt` at line 2800). + +### Coverage depth + +- `handleMuteToggle` (5 tests): ~70-75% branch coverage. Missing: `GetSubscription` generic error, `GetUserSiteID` error (non-fatal), `publishCore` failure, `publishToStream` failure (cross-site). Below the 90% target for core handler logic. +- `handleSubscriptionMuteToggled` (3 tests): ~75% branch coverage. Missing: store error propagation. +- `TestMongoStore_ToggleSubscriptionMute`: solid — covers false→true, persistence, true→false, not-found sentinel. + +### Mock/integration discipline + race detector + +- Unit tests use gomock in `room-service`, hand-written stubs in `inbox-worker` — both correct for their service's existing convention. +- No inline `testcontainers` added. +- `TestMain` already exists in both `integration_test.go` packages. +- No raw `go test` invocations added — everything runs through `make test` which already passes `-race`. + +### Verdict + +No `critical` or `high` findings. Mocks are fresh, TDD discipline followed at the exported-function level, all tests pass with `-race`. The two `medium` gaps (`GetSubscription` generic error + `publishToStream` cross-site failure) bring effective handler branch coverage below the 90% target — each is a small mechanical addition following patterns already in the same file. + +--- From a548d6196a2635559c594269b796b3905be28931 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:14:07 +0000 Subject: [PATCH 18/32] review(branch): bug and security --- ...e-explore-room-service-7tNlq-2026-05-21.md | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index e519cbe1c..7026add87 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -233,3 +233,39 @@ All other new functions are unexported. TDD compliance: PASS at the unit level, No `critical` or `high` findings. Mocks are fresh, TDD discipline followed at the exported-function level, all tests pass with `-race`. The two `medium` gaps (`GetSubscription` generic error + `publishToStream` cross-site failure) bring effective handler branch coverage below the 90% target — each is a small mechanical addition following patterns already in the same file. --- + +## Bug & security + +### SAST results + +| Tool | Result | +|---|---| +| gosec | PASS — no medium+ findings introduced | +| govulncheck | PASS — no vulnerabilities | +| semgrep | NOT RUN — binary not installed in env; environment limitation, not a code defect | + +### Findings + +**[medium]** `room-service/handler.go:1242` + `room-service/helper.go:202` — Invalid-subject error string silently downgraded to `"internal error"` by `sanitizeError`. `handleMuteToggle` produces `fmt.Errorf("invalid mute-toggle subject: %s", subj)`. The `sanitizeError` safe-prefix list in `helper.go:202` includes `"invalid request"` and similar prefixes but NOT `"invalid mute-toggle"`. Result: NATS replies to clients carry `"internal error"` instead of the documented `"invalid mute-toggle subject: …"`. The unit tests at `handler_test.go:3340` call `handleMuteToggle` directly and never pass through `sanitizeError`, so the mismatch is not caught. `docs/client-api.md` explicitly documents `"invalid mute-toggle subject: …"` as the error clients should see — **the API contract is broken at delivery**. Fix: add `"invalid mute-toggle"` to the safe-prefix list, OR introduce an `errInvalidMuteToggleSubject` sentinel and wire it through. + +**[medium]** `room-service/handler.go:1280-1283` — `GetUserSiteID` failure silently returns `"ok"` and skips the cross-site outbox publish. If `GetUserSiteID` errors on a transient or permanent DB issue, the local toggle has already committed but the federation event is dropped. The client considers the RPC successful; the user's home site never receives the mute update and permanently disagrees with the room site. Inconsistent with the analogous `handleMessageRead` (`handler.go:1010`) which propagates the `GetUserSiteID` error. (Surfaced independently by 3 reviewers — room-service generalist, Go expert, and this lens.) + +**[low]** `room-service/handler.go:1253-1258` — Redundant `ErrSubscriptionNotFound` guard after the prior membership check at line 1245. `GetSubscription` already gatekeeps; the second check is unreachable in practice. Dead code that misleads readers about atomicity guarantees. + +**[nitpick]** `room-service/handler.go:1230` — `slog.Error("mute toggle failed", "error", err, "subject", m.Msg.Subject)` logs the raw NATS subject on every error, including malformed-subject errors. The subject encodes `account` and `roomID` (identifiers, not secrets); not a CLAUDE.md violation but worth noting that this handler logs an extra `subject` field that peer handlers do not. + +**[nitpick]** `inbox-worker/main.go:121` — `UpdateSubscriptionMute` discards `*UpdateResult` (assigns to `_`). Intentional per the design spec — missing-subscription is a silent no-op for federation races. Worth a one-line inline comment justifying the discard. + +### Specific trust assertions + +1. **Membership check before toggle** ✓ — `handleMuteToggle` calls `store.GetSubscription` at line 1245 before `ToggleSubscriptionMute`. A non-member cannot mutate state at the store layer. +2. **Mongo filter injection safety** ✓ — Filter `{"roomId": roomID, "u.account": account}` uses strings parsed by `subject.ParseUserRoomSubject` from the NATS subject. Subject tokens are dot-delimited and the parser rejects malformed inputs before reaching the store. No object-injection surface. +3. **Outbox payload trust** ✓ — `inbox-worker`'s `UpdateSubscriptionMute` receives `disableNotifications` from the outbox payload, which originates from `room-service`'s atomic Mongo write result (`ToggleSubscriptionMute` post-flip value) — not from client input. The trust assumption is that federated JetStream delivery from a peer site is trustworthy, consistent with the rest of `inbox-worker`. +4. **OutboxEvent assembly** ✓ — `Type`, `SiteID`, `DestSiteID`, `Payload`, `Timestamp` are all set in `handleMuteToggle` at lines 1294-1300. No fields missing. +5. **`sanitizeError` leak surface** ✓ — Internal Mongo error strings (e.g. `"db down"`) are caught by the catch-all `"internal error"` fallback at `helper.go:209`, but the same fallback ALSO catches the legitimate `"invalid mute-toggle subject"` string — see medium-1 above. + +### Verdict + +No security vulnerabilities, no race conditions, no injection surface. Two `medium` correctness bugs: the `sanitizeError` gap breaks the documented API contract on a single error path, and the `GetUserSiteID` silent-success creates reachable cross-site data divergence inconsistent with the rest of the codebase. Both should be fixed before merge. + +--- From 7a8ce980b15612b1dfd6acc741784d1b912e3ae7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:14:30 +0000 Subject: [PATCH 19/32] review(branch): performance --- ...e-explore-room-service-7tNlq-2026-05-21.md | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index 7026add87..d409aef22 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -269,3 +269,36 @@ No `critical` or `high` findings. Mocks are fresh, TDD discipline followed at th No security vulnerabilities, no race conditions, no injection surface. Two `medium` correctness bugs: the `sanitizeError` gap breaks the documented API contract on a single error path, and the `GetUserSiteID` silent-success creates reachable cross-site data divergence inconsistent with the rest of the codebase. Both should be fixed before merge. --- + +## Performance + +### Findings + +**[medium]** `room-service/handler.go:1245-1253` — Redundant Mongo round-trip before atomic write. `GetSubscription` runs before `ToggleSubscriptionMute` solely to (a) check membership and (b) supply the full `Subscription` struct for the downstream `SubscriptionUpdateEvent`. Both can be addressed without the extra round-trip: +- Membership check: `ToggleSubscriptionMute` already returns `ErrSubscriptionNotFound` on filter mismatch. +- Full struct for event: `sub.User.ID` is the only field beyond `disableNotifications` actually consumed. The atomic toggle could project the needed fields directly. + +This is the highest-priority performance issue since it doubles the per-request DB latency on every mute toggle. + +**[medium]** `inbox-worker/main.go:121-129` + `inbox-worker/main.go` `ensureIndexes` — `UpdateSubscriptionMute` filters on `{roomId, u.account}` but `inbox-worker`'s `ensureIndexes` (around `main.go:151`) only creates the `thread_subscriptions (threadRoomId, userId)` index. It does NOT create or assert the `(roomId, u.account)` index on `subscriptions`. The index is owned by whichever service first bootstraps the collection (typically `room-service` or `room-worker`), but `inbox-worker`'s `ensureIndexes` silently relies on it. If `inbox-worker` is deployed against a fresh collection (e.g. a new site in integration tests), the update degrades to a full collection scan. Either add the (idempotent) `CreateOne` call mirroring `room-service`'s `EnsureIndexes`, or document the cross-service index dependency. + +**[low]** `room-service/handler.go:1280` — `GetUserSiteID` runs strictly sequentially after the write and the `publishCore` call. It hits `users` (a different collection from `subscriptions`). It could overlap with `publishCore` (lines 1275-1278) but `publishCore` is non-fatal and core-NATS publish is very fast, so parallelisation adds complexity for ~µs gain. `users.account` index exists (`room-service/store_mongo.go:63-66`) — query is O(1). Not worth optimising. + +**[low]** `room-service/handler.go:1263` — `updatedSub := *sub` copies the entire `Subscription` struct just to mutate `DisableNotifications`. The copy is then embedded by value in `SubscriptionUpdateEvent`. If `Subscription` grows in the future, this becomes a per-request allocation hotspot. Consider holding the event sub by pointer or constructing it from individual fields. + +**[nitpick]** `room-service/handler.go:1307` — `publishToStream` (which calls `js.PublishMsg(ctx, ...)`) blocks waiting for JetStream PubAck with an unbounded context derived from `wrappedCtx(m)`. If the NATS server is overloaded, the handler goroutine blocks indefinitely. A `context.WithTimeout` of ~5s would bound the wait. Same pattern as the rest of the service (not introduced by this branch), but the new cross-site path makes it more reachable. + +### Index audit summary + +| Operation | Filter | Index | Status | +|---|---|---|---| +| `ToggleSubscriptionMute` (room-service) | `(roomId, u.account)` | unique compound at `store_mongo.go:57-61` | ✓ | +| `GetSubscription` (room-service) | `(roomId, u.account)` | same | ✓ | +| `GetUserSiteID` (room-service) | `account` | `users.account` at `store_mongo.go:63-66` | ✓ | +| `UpdateSubscriptionMute` (inbox-worker) | `(roomId, u.account)` | not declared in inbox-worker `ensureIndexes` | ✗ medium | + +### Verdict + +No critical issues. The new store operations hit covered indexes on the room-service side. The redundant `GetSubscription` round-trip (medium) doubles per-request DB latency on every mute toggle and could be eliminated by widening `ToggleSubscriptionMute`'s projection. The missing index assertion in `inbox-worker.ensureIndexes` is a medium latent risk. + +--- From 602163ebd116b0577a23b602d8d1a192fb2940e3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:14:49 +0000 Subject: [PATCH 20/32] review(branch): observability --- ...e-explore-room-service-7tNlq-2026-05-21.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index d409aef22..171308a5b 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -302,3 +302,29 @@ This is the highest-priority performance issue since it doubles the per-request No critical issues. The new store operations hit covered indexes on the room-service side. The redundant `GetSubscription` round-trip (medium) doubles per-request DB latency on every mute toggle and could be eliminated by widening `ToggleSubscriptionMute`'s projection. The missing index assertion in `inbox-worker.ensureIndexes` is a medium latent risk. --- + +## Observability + +### Findings + +**[medium]** `room-service/handler.go:1239` — `handleMuteToggle` is missing span attributes. The closest structural analogue `handleMessageRead` also doesn't set them (so matching that precedent is defensible), but `handleMessageReadReceipt` (line 1105) and `publishCreateRoom` (line 349) DO set `room.id` / `site.id`. Adding `span.SetAttributes(attribute.String("room.id", roomID), attribute.String("site.id", h.siteID))` would cost two lines and improve trace correlation for a per-room operation. + +**[medium]** `room-service/handler.go:1282` — `slog.Warn("get user siteId failed; skipping outbox", "error", err, "account", account)`. The `Warn` level signals "degraded but OK", but `GetUserSiteID` returning an error means a real Mongo failure on `users` (not-found is `("", nil)`, not an error). The handler then silently suppresses federation. **Either**: (a) match `handleMessageRead`'s precedent and propagate the error (hard fail), OR (b) keep the soft-fail but escalate to `slog.Error` so operators know cross-site federation is silently dropping events. + +**[nitpick]** `room-service/handler.go:1230` — `natsMuteToggle` logs `slog.Error` with an extra `subject` structured field. The peer `natsMessageRead` (line 971) doesn't. The new field is an improvement for debuggability — propagate to peers in a follow-up, or accept the inconsistency. + +**[low]** No Prometheus metric on the new RPC path. The codebase doesn't add explicit metrics per-handler — OTel + slog is the observability mechanism. No action needed. + +### Discipline checks — all PASS + +1. **slog/JSON discipline**: all four new log calls use `slog` with structured key-value pairs. No `fmt.Println` / `log.Println` / text loggers. ✓ +2. **No secret leakage**: tokens/passwords/full message bodies not logged. `account` is a non-secret identifier; existing handlers log it identically (e.g. `handler.go:1029`). ✓ +3. **Request-ID propagation**: `natsMuteToggle` (line 1227) calls `ctx := wrappedCtx(m)` and passes that `ctx` to `handleMuteToggle`. Context flows through every store call and publish. ✓ +4. **Error log levels**: `slog.Error` for the publish-failure non-fatal path is appropriate (operators need to know NATS publish is failing even though the RPC succeeds). The `slog.Warn` for the `GetUserSiteID` path is the one questionable case — see medium-2 above. +5. **inbox-worker error handling**: `handleSubscriptionMuteToggled` returns errors to the consumer loop without per-handler slog, matching `handleSubscriptionRead`. The consumer loop in `main.go:261` logs all returned errors at `slog.Error` with `request_id`. Pattern is consistent. ✓ + +### Verdict + +PASS with two `medium` findings — both about the `GetUserSiteID` failure path (missing span attrs is a minor instrumentation gap; the Warn-vs-Error level is a meaningful operator-signal choice). Everything else — slog-only JSON, `wrappedCtx` propagation, no secret leakage, inbox-worker error pattern — is correct. + +--- From 8aef3adaee9a0c324f2bdfaa2af07c6f411d126c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 21 May 2026 11:15:09 +0000 Subject: [PATCH 21/32] review(branch): prioritized action list --- ...e-explore-room-service-7tNlq-2026-05-21.md | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md index 171308a5b..a60205fe6 100644 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md @@ -328,3 +328,24 @@ No critical issues. The new store operations hit covered indexes on the room-ser PASS with two `medium` findings — both about the `GetUserSiteID` failure path (missing span attrs is a minor instrumentation gap; the Warn-vs-Error level is a meaningful operator-signal choice). Everything else — slog-only JSON, `wrappedCtx` propagation, no secret leakage, inbox-worker error pattern — is correct. --- + +## Prioritized action list + +Top items across all chapters, ordered by severity then impact-divided-by-effort. + +| # | Severity | Action | Location | Why | +|---|---|---|---|---| +| 1 | **high** | Eliminate the TOCTOU between `GetSubscription` and `ToggleSubscriptionMute`. Widen the atomic toggle's projection to include `u.id`/`u.account` and drop the pre-fetch — OR add a comment acknowledging the race. | `room-service/handler.go:1245-1264` | Doubles per-request DB latency; subscription state in the constructed `SubscriptionUpdateEvent` can be stale on a concurrent writer | +| 2 | **medium** | Add `"invalid mute-toggle"` to the `sanitizeError` safe-prefix list (or introduce an `errInvalidMuteToggleSubject` sentinel). | `room-service/helper.go:202` + `handler.go:1242` | API contract documented in this same PR is broken at delivery — clients get `"internal error"` instead of the documented message | +| 3 | **medium** | Resolve the `GetUserSiteID` failure asymmetry. Either propagate as hard error (match `handleMessageRead`) or escalate the log to `slog.Error` and add a doc note about the intentional degraded mode. | `room-service/handler.go:1280-1283` | Reachable cross-site data divergence on a transient DB hiccup; surfaced by 3 independent reviewers | +| 4 | **medium** | Update `SubscriptionUpdateEvent.Action` comment to include `"role_updated"` and `"mute_toggled"`. | `pkg/model/event.go:35` | Client-facing contract drift; comment is the API doc for that field | +| 5 | **medium** | Add the missing `(roomId, u.account)` index assertion to `inbox-worker`'s `ensureIndexes` (mirror room-service's `CreateOne` call). | `inbox-worker/main.go` `ensureIndexes` | Fresh-DB deployments degrade `UpdateSubscriptionMute` to a full collection scan | +| 6 | **medium** | Add test for cross-site `publishToStream` failure in `handleMuteToggle`. | `room-service/handler_test.go` | The hard-error path post-DB-write is currently untested; pattern exists in `TestHandler_MessageRead_CrossSite_PublishFailureAborts` | +| 7 | **medium** | Add test for `GetSubscription` generic error arm. | `room-service/handler_test.go` | Branch coverage for `handleMuteToggle` is ~70-75%; below the 90% target | +| 8 | **medium** | Add `span.SetAttributes(room.id, site.id)` to `handleMuteToggle`. | `room-service/handler.go:1239` | Two-line trace-correlation win | +| 9 | **medium** | Split `inbox-worker`'s inline `mongoInboxStore` from `main.go` into `store.go` + `store_mongo.go` (follow-up PR). | `inbox-worker/main.go:39-195` | Pre-existing tech debt this PR adds to; per-service layout convention | +| 10 | **low** | Remove the redundant `ErrSubscriptionNotFound` re-check after `ToggleSubscriptionMute`. | `room-service/handler.go:1253-1258` | Dead code that misleads about atomicity guarantees | + +--- + +**End of report.** From 6f32c4b76b104fe7ccd66fb3aa9bbdaa14812e21 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 01:39:48 +0000 Subject: [PATCH 22/32] fix(room-service): doc + span coverage on mute.toggle handler - Refresh SubscriptionUpdateEvent.Action JSON contract comment to list all current values (added | removed | role_updated | mute_toggled). - Add room.id / site.id span attributes to handleMuteToggle so traces match the instrumentation peer handlers already emit. https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- pkg/model/event.go | 2 +- room-service/handler.go | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/model/event.go b/pkg/model/event.go index febed921a..2a53f2fa0 100644 --- a/pkg/model/event.go +++ b/pkg/model/event.go @@ -32,7 +32,7 @@ type RoomMetadataUpdateEvent struct { type SubscriptionUpdateEvent struct { UserID string `json:"userId"` Subscription Subscription `json:"subscription"` - Action string `json:"action"` // "added" | "removed" + Action string `json:"action"` // "added" | "removed" | "role_updated" | "mute_toggled" Timestamp int64 `json:"timestamp" bson:"timestamp"` } diff --git a/room-service/handler.go b/room-service/handler.go index dae4f9c4e..dcaf92a71 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -1242,6 +1242,13 @@ func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ( return nil, fmt.Errorf("invalid mute-toggle subject: %s", subj) } + if span := trace.SpanFromContext(ctx); span.IsRecording() { + span.SetAttributes( + attribute.String("room.id", roomID), + attribute.String("site.id", h.siteID), + ) + } + sub, err := h.store.GetSubscription(ctx, account, roomID) switch { case errors.Is(err, model.ErrSubscriptionNotFound): From 65a25e51ad7ba155b5b3da6355c665803e5f6d09 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 01:41:08 +0000 Subject: [PATCH 23/32] fix(room-service): sanitizeError passes invalid-mute-toggle through The mute.toggle handler returns "invalid mute-toggle subject: " on parse failure, and docs/client-api.md documents that exact string. Without this entry the reply path collapsed it to "internal error", silently breaking the API contract just-shipped in this PR. https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- room-service/helper.go | 2 +- room-service/helper_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/room-service/helper.go b/room-service/helper.go index 7a98f6ebb..0fa1e1d22 100644 --- a/room-service/helper.go +++ b/room-service/helper.go @@ -200,7 +200,7 @@ func sanitizeError(err error) string { return err.Error() default: msg := err.Error() - for _, safe := range []string{"only owners can", "cannot add members", "room is at maximum capacity", "requester not in room", "invalid request", "remote member.list:"} { + for _, safe := range []string{"only owners can", "cannot add members", "room is at maximum capacity", "requester not in room", "invalid request", "remote member.list:", "invalid mute-toggle subject"} { if strings.Contains(msg, safe) { return msg } diff --git a/room-service/helper_test.go b/room-service/helper_test.go index 651331ccf..1305a6e84 100644 --- a/room-service/helper_test.go +++ b/room-service/helper_test.go @@ -57,6 +57,7 @@ func TestSanitizeError(t *testing.T) { {"safe capacity", errors.New("room is at maximum capacity (1000)"), "room is at maximum capacity (1000)"}, {"safe requester", errors.New("requester not in room: not found"), "requester not in room: not found"}, {"safe invalid", errors.New("invalid request: bad json"), "invalid request: bad json"}, + {"passes through invalid mute-toggle subject", fmt.Errorf("invalid mute-toggle subject: chat.user.alice.foo"), "invalid mute-toggle subject: chat.user.alice.foo"}, {"internal db error", fmt.Errorf("mongo timeout"), "internal error"}, {"generic error", fmt.Errorf("unexpected failure"), "internal error"}, } From 711d250c151e3a23749b967f1cee8f13dc657cf6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 01:42:39 +0000 Subject: [PATCH 24/32] fix(room-service): GetUserSiteID failure aborts mute.toggle handleMuteToggle previously swallowed GetUserSiteID errors and returned "ok" while silently dropping the cross-site outbox publish, leaving the user's home site permanently out of sync with the room's site on any transient DB hiccup. Mirror handleMessageRead's precedent and propagate the error as a hard failure so the client can retry. https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- room-service/handler.go | 3 +-- room-service/handler_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/room-service/handler.go b/room-service/handler.go index dcaf92a71..7bc7a9422 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -1286,8 +1286,7 @@ func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ( userSiteID, err := h.store.GetUserSiteID(ctx, account) if err != nil { - slog.Warn("get user siteId failed; skipping outbox", "error", err, "account", account) - return json.Marshal(model.MuteToggleResponse{Status: "ok", DisableNotifications: newVal}) + return nil, fmt.Errorf("get user siteId: %w", err) } if userSiteID != "" && userSiteID != h.siteID { payload := model.SubscriptionMuteToggledEvent{ diff --git a/room-service/handler_test.go b/room-service/handler_test.go index 24c0ae9c7..a51d5c000 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -3358,3 +3358,34 @@ func TestHandler_MuteToggle_StoreError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "toggle subscription mute") } + +func TestHandler_MuteToggle_GetUserSiteIDError(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + GetSubscription(gomock.Any(), "alice", "r1"). + Return(&model.Subscription{ + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, RoomID: "r1", + }, nil) + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(true, nil) + store.EXPECT(). + GetUserSiteID(gomock.Any(), "alice"). + Return("", fmt.Errorf("mongo down")) + + h := &Handler{ + store: store, siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { + t.Fatal("publishToStream must not be called when GetUserSiteID fails") + return nil + }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + _, err := h.handleMuteToggle(context.Background(), subj, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "get user siteId") +} From 9954ac95881b4b1ea7e95400552d1bb3a037ca83 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 01:50:06 +0000 Subject: [PATCH 25/32] refactor(room-service): drop pre-fetch in mute.toggle, return post-flip sub ToggleSubscriptionMute now returns the full post-flip *Subscription instead of just the bool, so the handler no longer needs a separate GetSubscription call to populate the SubscriptionUpdateEvent payload. Halves the per-request Mongo round-trips and closes the TOCTOU window between the two reads; the previously-dead ErrSubscriptionNotFound check after the toggle becomes the sole membership gate, which is also more honest about what the atomic write already guarantees. Also fixes pre-existing NewHandler arity errors in integration_test.go caused by the publishCore parameter added in a prior commit. https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- room-service/handler.go | 18 ++++----------- room-service/handler_test.go | 39 ++++++++++++-------------------- room-service/integration_test.go | 29 +++++++++++++++--------- room-service/mock_store_test.go | 4 ++-- room-service/store.go | 5 ++-- room-service/store_mongo.go | 23 ++++++++----------- 6 files changed, 50 insertions(+), 68 deletions(-) diff --git a/room-service/handler.go b/room-service/handler.go index 7bc7a9422..f803ea8de 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -1249,15 +1249,7 @@ func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ( ) } - sub, err := h.store.GetSubscription(ctx, account, roomID) - switch { - case errors.Is(err, model.ErrSubscriptionNotFound): - return nil, errNotRoomMember - case err != nil: - return nil, fmt.Errorf("get subscription: %w", err) - } - - newVal, err := h.store.ToggleSubscriptionMute(ctx, roomID, account) + sub, err := h.store.ToggleSubscriptionMute(ctx, roomID, account) if err != nil { if errors.Is(err, model.ErrSubscriptionNotFound) { return nil, errNotRoomMember @@ -1267,11 +1259,9 @@ func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ( now := time.Now().UTC() - updatedSub := *sub - updatedSub.DisableNotifications = newVal subEvt := model.SubscriptionUpdateEvent{ UserID: sub.User.ID, - Subscription: updatedSub, + Subscription: *sub, Action: "mute_toggled", Timestamp: now.UnixMilli(), } @@ -1292,7 +1282,7 @@ func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ( payload := model.SubscriptionMuteToggledEvent{ Account: account, RoomID: roomID, - DisableNotifications: newVal, + DisableNotifications: sub.DisableNotifications, Timestamp: now.UnixMilli(), } payloadData, err := json.Marshal(payload) @@ -1315,5 +1305,5 @@ func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ( } } - return json.Marshal(model.MuteToggleResponse{Status: "ok", DisableNotifications: newVal}) + return json.Marshal(model.MuteToggleResponse{Status: "ok", DisableNotifications: sub.DisableNotifications}) } diff --git a/room-service/handler_test.go b/room-service/handler_test.go index a51d5c000..ea2e31815 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -3207,16 +3207,14 @@ func TestHandler_MuteToggle_Success(t *testing.T) { store := NewMockRoomStore(ctrl) store.EXPECT(). - GetSubscription(gomock.Any(), "alice", "r1"). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). Return(&model.Subscription{ - ID: "s1", - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", - SiteID: "site-a", + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + DisableNotifications: true, }, nil) - store.EXPECT(). - ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). - Return(true, nil) store.EXPECT(). GetUserSiteID(gomock.Any(), "alice"). Return("site-a", nil) // same site → no outbox publish @@ -3261,14 +3259,13 @@ func TestHandler_MuteToggle_CrossSitePublishesOutbox(t *testing.T) { store := NewMockRoomStore(ctrl) store.EXPECT(). - GetSubscription(gomock.Any(), "alice", "r1"). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). Return(&model.Subscription{ - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", SiteID: "site-a", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + DisableNotifications: true, }, nil) - store.EXPECT(). - ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). - Return(true, nil) store.EXPECT(). GetUserSiteID(gomock.Any(), "alice"). Return("site-b", nil) @@ -3310,7 +3307,7 @@ func TestHandler_MuteToggle_NotRoomMember(t *testing.T) { store := NewMockRoomStore(ctrl) store.EXPECT(). - GetSubscription(gomock.Any(), "alice", "r1"). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). Return(nil, model.ErrSubscriptionNotFound) h := &Handler{ @@ -3339,14 +3336,9 @@ func TestHandler_MuteToggle_StoreError(t *testing.T) { ctrl := gomock.NewController(t) store := NewMockRoomStore(ctrl) - store.EXPECT(). - GetSubscription(gomock.Any(), "alice", "r1"). - Return(&model.Subscription{ - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, RoomID: "r1", - }, nil) store.EXPECT(). ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). - Return(false, fmt.Errorf("db down")) + Return(nil, fmt.Errorf("db down")) h := &Handler{ store: store, siteID: "site-a", @@ -3364,13 +3356,10 @@ func TestHandler_MuteToggle_GetUserSiteIDError(t *testing.T) { store := NewMockRoomStore(ctrl) store.EXPECT(). - GetSubscription(gomock.Any(), "alice", "r1"). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). Return(&model.Subscription{ User: model.SubscriptionUser{ID: "u1", Account: "alice"}, RoomID: "r1", }, nil) - store.EXPECT(). - ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). - Return(true, nil) store.EXPECT(). GetUserSiteID(gomock.Any(), "alice"). Return("", fmt.Errorf("mongo down")) diff --git a/room-service/integration_test.go b/room-service/integration_test.go index e990cf973..cd4c778d0 100644 --- a/room-service/integration_test.go +++ b/room-service/integration_test.go @@ -868,7 +868,7 @@ func TestAddMembers_SameSiteChannel_RoomMembersPath(t *testing.T) { publishedData = data return nil } - handler := NewHandler(store, keyStore, nil, nil, "site-a", 1000, 500, 5*time.Second, publish) + handler := NewHandler(store, keyStore, nil, nil, "site-a", 1000, 500, 5*time.Second, publish, func(context.Context, string, []byte) error { return nil }) req := model.AddMembersRequest{ Channels: []model.ChannelRef{{RoomID: "source", SiteID: "site-a"}}, @@ -932,7 +932,7 @@ func TestAddMembers_SameSiteChannel_SubscriptionsFallback(t *testing.T) { publishedData = data return nil } - handler := NewHandler(store, keyStore, nil, nil, "site-a", 1000, 500, 5*time.Second, publish) + handler := NewHandler(store, keyStore, nil, nil, "site-a", 1000, 500, 5*time.Second, publish, func(context.Context, string, []byte) error { return nil }) req := model.AddMembersRequest{Channels: []model.ChannelRef{{RoomID: "source", SiteID: "site-a"}}} data, err := json.Marshal(req) @@ -974,7 +974,7 @@ func TestAddMembers_RequesterNotSubscribed_Rejected(t *testing.T) { // Same-site only: nil memberListClient is safe — request fails on the same-site // GetSubscription check before reaching the cross-site branch. - handler := NewHandler(store, keyStore, nil, nil, "site-a", 1000, 500, 5*time.Second, func(context.Context, string, []byte) error { return nil }) + handler := NewHandler(store, keyStore, nil, nil, "site-a", 1000, 500, 5*time.Second, func(context.Context, string, []byte) error { return nil }, func(context.Context, string, []byte) error { return nil }) req := model.AddMembersRequest{Channels: []model.ChannelRef{{RoomID: "source", SiteID: "site-a"}}} data, err := json.Marshal(req) @@ -1029,7 +1029,7 @@ func TestAddMembers_TwoSiteEndToEnd(t *testing.T) { require.NoError(t, storeB.CreateSubscription(ctx, &model.Subscription{ID: "sb3", RoomID: "source", User: model.SubscriptionUser{ID: "req", Account: "alice"}})) // Site-B handler registers member.list endpoint (RegisterCRUD subscribes to MemberListWildcard). - handlerB := NewHandler(storeB, keyStore, nil, nil, "site-b", 1000, 500, 5*time.Second, func(context.Context, string, []byte) error { return nil }) + handlerB := NewHandler(storeB, keyStore, nil, nil, "site-b", 1000, 500, 5*time.Second, func(context.Context, string, []byte) error { return nil }, func(context.Context, string, []byte) error { return nil }) require.NoError(t, handlerB.RegisterCRUD(otelNCb)) require.NoError(t, otelNCb.NatsConn().Flush()) @@ -1049,7 +1049,7 @@ func TestAddMembers_TwoSiteEndToEnd(t *testing.T) { publishedData = data return nil } - handlerA := NewHandler(storeA, keyStore, memberListClient, nil, "site-a", 1000, 500, 5*time.Second, publish) + handlerA := NewHandler(storeA, keyStore, memberListClient, nil, "site-a", 1000, 500, 5*time.Second, publish, func(context.Context, string, []byte) error { return nil }) // Call add-members on site-A with a site-B source channel req := model.AddMembersRequest{Channels: []model.ChannelRef{{RoomID: "source", SiteID: "site-b"}}} @@ -1109,7 +1109,7 @@ func TestAddMembers_CrossSiteTimeout(t *testing.T) { t.Cleanup(func() { _ = sub.Unsubscribe() }) memberListClient := NewNATSMemberListClient(nc, 200*time.Millisecond) - handler := NewHandler(store, keyStore, memberListClient, nil, "site-a", 1000, 500, 200*time.Millisecond, func(context.Context, string, []byte) error { return nil }) + handler := NewHandler(store, keyStore, memberListClient, nil, "site-a", 1000, 500, 200*time.Millisecond, func(context.Context, string, []byte) error { return nil }, func(context.Context, string, []byte) error { return nil }) req := model.AddMembersRequest{Channels: []model.ChannelRef{{RoomID: "source", SiteID: "site-b"}}} data, err := json.Marshal(req) @@ -1157,7 +1157,7 @@ func TestRoomsInfoBatchRPC(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { _ = otelNC.Drain() }) - handler := NewHandler(store, keyStore, nil, nil, "site-a", 1000, 500, 5*time.Second, func(context.Context, string, []byte) error { return nil }) + handler := NewHandler(store, keyStore, nil, nil, "site-a", 1000, 500, 5*time.Second, func(context.Context, string, []byte) error { return nil }, func(context.Context, string, []byte) error { return nil }) require.NoError(t, handler.RegisterCRUD(otelNC)) require.NoError(t, otelNC.NatsConn().Flush()) @@ -1288,7 +1288,7 @@ func newRoomServiceHandler(t *testing.T, store *MongoStore, keyStore RoomKeyStor lastData = data return nil } - h := NewHandler(store, keyStore, nil, nil, siteID, 1000, 500, 5*time.Second, publish) + h := NewHandler(store, keyStore, nil, nil, siteID, 1000, 500, 5*time.Second, publish, func(context.Context, string, []byte) error { return nil }) return h, func() (string, []byte) { return lastSubj, lastData } } @@ -1556,7 +1556,10 @@ func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { // First toggle: false → true. got, err := store.ToggleSubscriptionMute(ctx, "r1", "alice") require.NoError(t, err) - assert.True(t, got) + require.NotNil(t, got) + assert.True(t, got.DisableNotifications) + assert.Equal(t, "alice", got.User.Account) + assert.Equal(t, "r1", got.RoomID) // Verify persisted. persisted, err := store.GetSubscription(ctx, "alice", "r1") @@ -1566,9 +1569,13 @@ func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { // Second toggle: true → false. got, err = store.ToggleSubscriptionMute(ctx, "r1", "alice") require.NoError(t, err) - assert.False(t, got) + require.NotNil(t, got) + assert.False(t, got.DisableNotifications) + assert.Equal(t, "alice", got.User.Account) + assert.Equal(t, "r1", got.RoomID) // Not-found case. - _, err = store.ToggleSubscriptionMute(ctx, "missing", "alice") + gotNil, err := store.ToggleSubscriptionMute(ctx, "missing", "alice") + assert.Nil(t, gotNil) assert.ErrorIs(t, err, model.ErrSubscriptionNotFound) } diff --git a/room-service/mock_store_test.go b/room-service/mock_store_test.go index 1ddce1cd9..6a94e912d 100644 --- a/room-service/mock_store_test.go +++ b/room-service/mock_store_test.go @@ -312,10 +312,10 @@ func (mr *MockRoomStoreMockRecorder) MinSubscriptionLastSeenByRoomID(ctx, roomID } // ToggleSubscriptionMute mocks base method. -func (m *MockRoomStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (bool, error) { +func (m *MockRoomStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (*model.Subscription, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ToggleSubscriptionMute", ctx, roomID, account) - ret0, _ := ret[0].(bool) + ret0, _ := ret[0].(*model.Subscription) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/room-service/store.go b/room-service/store.go index d1949ce11..a95431bc0 100644 --- a/room-service/store.go +++ b/room-service/store.go @@ -75,9 +75,10 @@ type RoomStore interface { // ToggleSubscriptionMute atomically flips disableNotifications on the // subscription keyed by (roomID, account) using a single aggregation-pipeline // FindOneAndUpdate so concurrent toggles cannot deadlock or read-modify-write - // race. Returns the resulting value of disableNotifications post-flip. + // race. Returns the full post-flip subscription document so callers do not + // need a separate read to populate event payloads. // Returns model.ErrSubscriptionNotFound (wrapped) when no subscription matches. - ToggleSubscriptionMute(ctx context.Context, roomID, account string) (bool, error) + ToggleSubscriptionMute(ctx context.Context, roomID, account string) (*model.Subscription, error) // GetUserSiteID returns the home site of a user looked up by account. // Returns ("", nil) when the user is not found locally; callers treat // that as "skip cross-site outbox". diff --git a/room-service/store_mongo.go b/room-service/store_mongo.go index bf8325945..d02a3e137 100644 --- a/room-service/store_mongo.go +++ b/room-service/store_mongo.go @@ -681,9 +681,9 @@ func (s *MongoStore) UpdateSubscriptionRead(ctx context.Context, roomID, account // ToggleSubscriptionMute atomically flips disableNotifications via an // aggregation-pipeline FindOneAndUpdate. The $ifNull guard treats a missing // field as false so legacy documents toggle to true on the first call. -// Returns the post-flip value, or model.ErrSubscriptionNotFound when no -// subscription matches. -func (s *MongoStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (bool, error) { +// Returns the full post-flip subscription document, or model.ErrSubscriptionNotFound +// when no subscription matches. +func (s *MongoStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (*model.Subscription, error) { filter := bson.M{"roomId": roomID, "u.account": account} update := mongo.Pipeline{ bson.D{{Key: "$set", Value: bson.M{ @@ -692,21 +692,16 @@ func (s *MongoStore) ToggleSubscriptionMute(ctx context.Context, roomID, account }}, }}}, } - opts := options.FindOneAndUpdate(). - SetReturnDocument(options.After). - SetProjection(bson.M{"_id": 0, "disableNotifications": 1}) + opts := options.FindOneAndUpdate().SetReturnDocument(options.After) - var result struct { - DisableNotifications bool `bson:"disableNotifications"` - } - err := s.subscriptions.FindOneAndUpdate(ctx, filter, update, opts).Decode(&result) - if err != nil { + var result model.Subscription + if err := s.subscriptions.FindOneAndUpdate(ctx, filter, update, opts).Decode(&result); err != nil { if errors.Is(err, mongo.ErrNoDocuments) { - return false, fmt.Errorf("toggle mute for %q in room %q: %w", account, roomID, model.ErrSubscriptionNotFound) + return nil, fmt.Errorf("toggle mute for %q in room %q: %w", account, roomID, model.ErrSubscriptionNotFound) } - return false, fmt.Errorf("toggle mute for %q in room %q: %w", account, roomID, err) + return nil, fmt.Errorf("toggle mute for %q in room %q: %w", account, roomID, err) } - return result.DisableNotifications, nil + return &result, nil } // GetUserSiteID looks up users.siteId by account. Returns ("", nil) if no From ffd274ba99848edb7fe9251ff2f91cee3c22ec31 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 01:52:30 +0000 Subject: [PATCH 26/32] test(room-service): cover cross-site outbox publish failure in mute.toggle The hard-error path at the end of handleMuteToggle (publish mute-toggled outbox: %w) was the last untested branch in the handler. With this test the six TestHandler_MuteToggle_* tests cover every error path (parse, store, GetUserSiteID, outbox publish) plus the same-site and cross-site happy paths. https://claude.ai/code/session_01JMBBydPH1indHnCAQP86Te --- room-service/handler_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/room-service/handler_test.go b/room-service/handler_test.go index ea2e31815..b76c10549 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -3378,3 +3378,33 @@ func TestHandler_MuteToggle_GetUserSiteIDError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "get user siteId") } + +func TestHandler_MuteToggle_CrossSiteOutboxPublishFailure(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(&model.Subscription{ + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + DisableNotifications: true, + }, nil) + store.EXPECT(). + GetUserSiteID(gomock.Any(), "alice"). + Return("site-b", nil) + + h := &Handler{ + store: store, siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte) error { + return fmt.Errorf("nats unavailable") + }, + publishCore: func(_ context.Context, _ string, _ []byte) error { return nil }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + _, err := h.handleMuteToggle(context.Background(), subj, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "publish mute-toggled outbox") +} From 7361df0a746328eb5c9f3c0184828abc9e26035d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 01:57:47 +0000 Subject: [PATCH 27/32] chore: trim mute.toggle comments per 2-line / 65% rule Delete four WHAT-style step labels in the integration test and shorten seven godoc/internal comments to one or two lines without losing the WHY content (atomicity guarantee, no-op-for-federation-race rationale). Final 1-line ratio across this branch's comment surface is 9/11 = 82%. --- inbox-worker/handler.go | 9 ++------- inbox-worker/main.go | 3 +-- pkg/model/event.go | 8 ++------ pkg/subject/subject.go | 1 - room-service/integration_test.go | 4 ---- room-service/store.go | 8 ++------ room-service/store_mongo.go | 7 ++----- 7 files changed, 9 insertions(+), 31 deletions(-) diff --git a/inbox-worker/handler.go b/inbox-worker/handler.go index 4767973d4..7b4fc2df9 100644 --- a/inbox-worker/handler.go +++ b/inbox-worker/handler.go @@ -29,9 +29,7 @@ type InboxStore interface { // silent no-ops. Missing-subscription is also a silent no-op. UpdateSubscriptionRead(ctx context.Context, roomID, account string, lastSeenAt time.Time, alert bool) error UpsertThreadSubscription(ctx context.Context, sub *model.ThreadSubscription) error - // UpdateSubscriptionMute sets disableNotifications on the subscription keyed - // by (roomID, account). Missing-subscription is a silent no-op so federation - // races during membership delete don't surface errors. + // UpdateSubscriptionMute sets disableNotifications by (roomID, account); missing-sub is a silent no-op for federation races. UpdateSubscriptionMute(ctx context.Context, roomID, account string, disableNotifications bool) error } @@ -205,10 +203,7 @@ func (h *Handler) handleSubscriptionRead(ctx context.Context, evt *model.OutboxE return nil } -// handleSubscriptionMuteToggled mirrors a mute toggle from the room's home -// site onto the user's home-site subscription copy. Missing-subscription is -// a silent no-op (the store enforces this) so a federation race between a -// membership delete and a mute toggle does not error the consumer. +// handleSubscriptionMuteToggled mirrors a room-side mute toggle onto the user's home-site subscription. func (h *Handler) handleSubscriptionMuteToggled(ctx context.Context, evt *model.OutboxEvent) error { var e model.SubscriptionMuteToggledEvent if err := json.Unmarshal(evt.Payload, &e); err != nil { diff --git a/inbox-worker/main.go b/inbox-worker/main.go index 469387b15..33648e798 100644 --- a/inbox-worker/main.go +++ b/inbox-worker/main.go @@ -115,8 +115,7 @@ func (s *mongoInboxStore) BulkCreateSubscriptions(ctx context.Context, subs []*m return nil } -// UpdateSubscriptionMute sets disableNotifications on the subscription keyed -// by (roomID, account). Missing-subscription is a silent no-op. +// UpdateSubscriptionMute sets disableNotifications by (roomID, account); missing is a silent no-op. func (s *mongoInboxStore) UpdateSubscriptionMute(ctx context.Context, roomID, account string, disableNotifications bool) error { _, err := s.subCol.UpdateOne(ctx, bson.M{"roomId": roomID, "u.account": account}, diff --git a/pkg/model/event.go b/pkg/model/event.go index 2a53f2fa0..032428dfc 100644 --- a/pkg/model/event.go +++ b/pkg/model/event.go @@ -211,17 +211,13 @@ type RoomKeyEnsureResponse struct { Version int `json:"version"` } -// MuteToggleResponse is the sync reply for the mute.toggle RPC. DisableNotifications -// carries the resulting value of the toggle (post-flip). +// MuteToggleResponse is the sync reply for the mute.toggle RPC. type MuteToggleResponse struct { Status string `json:"status"` DisableNotifications bool `json:"disableNotifications"` } -// SubscriptionMuteToggledEvent is the OutboxEvent.Payload for type -// "subscription_mute_toggled". Sent from a room's home site to the user's home -// site whenever a user flips DisableNotifications via the mute.toggle RPC; the -// destination updates its local subscription mirror. +// SubscriptionMuteToggledEvent is the OutboxEvent.Payload for type "subscription_mute_toggled". type SubscriptionMuteToggledEvent struct { Account string `json:"account" bson:"account"` RoomID string `json:"roomId" bson:"roomId"` diff --git a/pkg/subject/subject.go b/pkg/subject/subject.go index 37d3c086f..08f306cda 100644 --- a/pkg/subject/subject.go +++ b/pkg/subject/subject.go @@ -362,7 +362,6 @@ func MessageReadReceiptWildcard(siteID string) string { } // MuteToggle returns the concrete subject for the per-user mute.toggle RPC. -// Pair with MuteToggleWildcard for room-service's QueueSubscribe. func MuteToggle(account, roomID, siteID string) string { return fmt.Sprintf("chat.user.%s.request.room.%s.%s.mute.toggle", account, roomID, siteID) } diff --git a/room-service/integration_test.go b/room-service/integration_test.go index cd4c778d0..a822ed772 100644 --- a/room-service/integration_test.go +++ b/room-service/integration_test.go @@ -1553,7 +1553,6 @@ func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { } require.NoError(t, store.CreateSubscription(ctx, sub)) - // First toggle: false → true. got, err := store.ToggleSubscriptionMute(ctx, "r1", "alice") require.NoError(t, err) require.NotNil(t, got) @@ -1561,12 +1560,10 @@ func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { assert.Equal(t, "alice", got.User.Account) assert.Equal(t, "r1", got.RoomID) - // Verify persisted. persisted, err := store.GetSubscription(ctx, "alice", "r1") require.NoError(t, err) assert.True(t, persisted.DisableNotifications) - // Second toggle: true → false. got, err = store.ToggleSubscriptionMute(ctx, "r1", "alice") require.NoError(t, err) require.NotNil(t, got) @@ -1574,7 +1571,6 @@ func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { assert.Equal(t, "alice", got.User.Account) assert.Equal(t, "r1", got.RoomID) - // Not-found case. gotNil, err := store.ToggleSubscriptionMute(ctx, "missing", "alice") assert.Nil(t, gotNil) assert.ErrorIs(t, err, model.ErrSubscriptionNotFound) diff --git a/room-service/store.go b/room-service/store.go index a95431bc0..3e20038ec 100644 --- a/room-service/store.go +++ b/room-service/store.go @@ -72,12 +72,8 @@ type RoomStore interface { // keyed by (roomID, account). Returns model.ErrSubscriptionNotFound // (wrapped) when no subscription matches. UpdateSubscriptionRead(ctx context.Context, roomID, account string, lastSeenAt time.Time, alert bool) error - // ToggleSubscriptionMute atomically flips disableNotifications on the - // subscription keyed by (roomID, account) using a single aggregation-pipeline - // FindOneAndUpdate so concurrent toggles cannot deadlock or read-modify-write - // race. Returns the full post-flip subscription document so callers do not - // need a separate read to populate event payloads. - // Returns model.ErrSubscriptionNotFound (wrapped) when no subscription matches. + // ToggleSubscriptionMute atomically flips disableNotifications via a single FindOneAndUpdate. + // Returns the post-flip subscription, or model.ErrSubscriptionNotFound (wrapped) when no match. ToggleSubscriptionMute(ctx context.Context, roomID, account string) (*model.Subscription, error) // GetUserSiteID returns the home site of a user looked up by account. // Returns ("", nil) when the user is not found locally; callers treat diff --git a/room-service/store_mongo.go b/room-service/store_mongo.go index d02a3e137..671ca6cba 100644 --- a/room-service/store_mongo.go +++ b/room-service/store_mongo.go @@ -678,11 +678,8 @@ func (s *MongoStore) UpdateSubscriptionRead(ctx context.Context, roomID, account return nil } -// ToggleSubscriptionMute atomically flips disableNotifications via an -// aggregation-pipeline FindOneAndUpdate. The $ifNull guard treats a missing -// field as false so legacy documents toggle to true on the first call. -// Returns the full post-flip subscription document, or model.ErrSubscriptionNotFound -// when no subscription matches. +// ToggleSubscriptionMute atomically flips disableNotifications via FindOneAndUpdate. +// $ifNull treats absent field as false so legacy docs toggle to true on first call. func (s *MongoStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (*model.Subscription, error) { filter := bson.M{"roomId": roomID, "u.account": account} update := mongo.Pipeline{ From fc9d4437dd50be0a67a354dd657dfde53b1d11e6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 03:18:08 +0000 Subject: [PATCH 28/32] fix(room-service): use mustInsertSub helper in mute-toggle integration test The merge from main removed RoomStore.CreateSubscription; the new TestMongoStore_ToggleSubscriptionMute was still calling it, breaking the integration-test build. Switch to the mustInsertSub helper used by the rest of the file. --- room-service/integration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/room-service/integration_test.go b/room-service/integration_test.go index c24a9b69d..d86d2116e 100644 --- a/room-service/integration_test.go +++ b/room-service/integration_test.go @@ -1788,7 +1788,7 @@ func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { JoinedAt: time.Now().UTC(), DisableNotifications: false, } - require.NoError(t, store.CreateSubscription(ctx, sub)) + mustInsertSub(t, db, sub) got, err := store.ToggleSubscriptionMute(ctx, "r1", "alice") require.NoError(t, err) From f38e7d53fa6ac4873db9da7820fd4a23ee19eb0f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 05:49:47 +0000 Subject: [PATCH 29/32] fix(room-service): correct $not operand shape in mute toggle pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MongoDB aggregation expression $not requires array syntax — see https://www.mongodb.com/docs/manual/reference/operator/aggregation/not/. Wrapping the $ifNull expression in bson.A makes the pipeline conform. Reported by CodeRabbit on PR #217. --- room-service/store_mongo.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/room-service/store_mongo.go b/room-service/store_mongo.go index 369609d71..ebaf572c9 100644 --- a/room-service/store_mongo.go +++ b/room-service/store_mongo.go @@ -810,8 +810,8 @@ func (s *MongoStore) ToggleSubscriptionMute(ctx context.Context, roomID, account filter := bson.M{"roomId": roomID, "u.account": account} update := mongo.Pipeline{ bson.D{{Key: "$set", Value: bson.M{ - "disableNotifications": bson.M{"$not": bson.M{ - "$ifNull": bson.A{"$disableNotifications", false}, + "disableNotifications": bson.M{"$not": bson.A{ + bson.M{"$ifNull": bson.A{"$disableNotifications", false}}, }}, }}}, } From 3b7a12e2b5d426bc7eb4c0855876384f0e1bdbd3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 05:51:13 +0000 Subject: [PATCH 30/32] test(room-service): pin publishCore non-fatal contract on mute toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit (PR #217) flagged that publishCore failure was untested. The intended behaviour is non-fatal — DB write is the source of truth and clients reconcile on next refetch (documented inline at handler.go:1277). Add a test that asserts the handler still replies ok when publishCore returns an error, so a future refactor can't silently flip the contract. --- room-service/handler_test.go | 40 ++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/room-service/handler_test.go b/room-service/handler_test.go index 65c8deeb8..c9633b2a4 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -3594,3 +3594,43 @@ func TestHandler_MuteToggle_CrossSiteOutboxPublishFailure(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "publish mute-toggled outbox") } + +// publishCore failure is intentionally non-fatal: the DB write is the source +// of truth and other client sessions reconcile on their next subscription +// refetch. The handler must still reply ok. +func TestHandler_MuteToggle_CorePublishFailureIsNonFatal(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT(). + ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). + Return(&model.Subscription{ + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + DisableNotifications: true, + }, nil) + store.EXPECT(). + GetUserSiteID(gomock.Any(), "alice"). + Return("site-a", nil) + + h := &Handler{ + store: store, siteID: "site-a", + publishCore: func(_ context.Context, _ string, _ []byte) error { + return fmt.Errorf("core nats down") + }, + publishToStream: func(_ context.Context, _ string, _ []byte) error { + t.Fatal("publishToStream must not be called for same-site mute toggle") + return nil + }, + } + + subj := subject.MuteToggle("alice", "r1", "site-a") + resp, err := h.handleMuteToggle(context.Background(), subj, nil) + require.NoError(t, err, "publishCore failure must be non-fatal — DB write is the source of truth") + + var got model.MuteToggleResponse + require.NoError(t, json.Unmarshal(resp, &got)) + assert.Equal(t, "ok", got.Status) + assert.True(t, got.DisableNotifications) +} From 57681d3a56496c6d811349af5ab6535fa37131fd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 07:15:57 +0000 Subject: [PATCH 31/32] =?UTF-8?q?refactor:=20rename=20Subscription.Disable?= =?UTF-8?q?Notifications=20=E2=86=92=20Muted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per PM, the field is now called Muted. JSON/BSON tags become "muted" and the outbox payload + response field align. Mongo data on existing sites would need a one-shot updateMany rename — operational task, no prod yet so no migration in this PR. --- docs/client-api.md | 12 ++++----- inbox-worker/handler.go | 6 ++--- inbox-worker/handler_test.go | 10 +++---- inbox-worker/main.go | 6 ++--- pkg/model/event.go | 12 ++++----- pkg/model/model_test.go | 46 ++++++++++++++++---------------- pkg/model/subscription.go | 30 ++++++++++----------- room-service/handler.go | 10 +++---- room-service/handler_test.go | 42 ++++++++++++++--------------- room-service/integration_test.go | 22 +++++++-------- room-service/store.go | 2 +- room-service/store_mongo.go | 6 ++--- room-worker/integration_test.go | 46 ++++++++++++++++---------------- 13 files changed, 125 insertions(+), 125 deletions(-) diff --git a/docs/client-api.md b/docs/client-api.md index 03d4bc825..6fe424d9a 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -787,7 +787,7 @@ See [Error envelope](#6-error-envelope-reference). Common errors: **Subject:** `chat.user.{account}.request.room.{roomID}.{siteID}.mute.toggle` **Reply subject:** auto-generated `_INBOX.>` (NATS request/reply) -Synchronous RPC. `room-service` flips `Subscription.disableNotifications` for the requester in a single atomic Mongo `FindOneAndUpdate`, replies with the resulting value, fans out a `subscription.update` event to the user's other client sessions, and (for cross-site users) publishes a `subscription_mute_toggled` outbox event so `inbox-worker` mirrors the change on the user's home site. +Synchronous RPC. `room-service` flips `Subscription.muted` for the requester in a single atomic Mongo `FindOneAndUpdate`, replies with the resulting value, fans out a `subscription.update` event to the user's other client sessions, and (for cross-site users) publishes a `subscription_mute_toggled` outbox event so `inbox-worker` mirrors the change on the user's home site. Idempotency: this is a toggle, not a set — every successful call flips the bit. Clients must debounce the user-visible action; redelivery of the same RPC will flip back. @@ -800,10 +800,10 @@ The subject already carries `account` and `roomID`, so no body fields are requir | Field | Type | Notes | |------------------------|---------|-------| | `status` | string | Always `"ok"`. | -| `disableNotifications` | boolean | The resulting value of `Subscription.disableNotifications` after the flip. | +| `muted` | boolean | The resulting value of `Subscription.muted` after the flip. | ```json -{ "status": "ok", "disableNotifications": true } +{ "status": "ok", "muted": true } ``` ##### Error response @@ -820,14 +820,14 @@ See [Error envelope](#6-error-envelope-reference). Common errors: | Field | Type | Notes | |----------------|--------|-------| | `userId` | string | The requester's internal user ID. | -| `subscription` | object | The `Subscription` record with the updated `disableNotifications`. | +| `subscription` | object | The `Subscription` record with the updated `muted`. | | `action` | string | `"mute_toggled"`. | | `timestamp` | number | Milliseconds since Unix epoch (UTC). | ##### Behaviour notes -- **Cross-site federation:** if the user's home site (`users.siteId`) differs from the handler's site, a `subscription_mute_toggled` outbox event is published to `outbox.{handlerSite}.to.{userSite}.subscription_mute_toggled` with payload `{account, roomId, disableNotifications, timestamp}`. The destination `inbox-worker` applies an unconditional `$set` on the local subscription mirror. -- **Notification delivery:** `notification-worker` does **not** yet consult `disableNotifications` before sending. End-to-end mute behaviour is wired only as far as the persisted flag; honouring it in fan-out is a follow-up. +- **Cross-site federation:** if the user's home site (`users.siteId`) differs from the handler's site, a `subscription_mute_toggled` outbox event is published to `outbox.{handlerSite}.to.{userSite}.subscription_mute_toggled` with payload `{account, roomId, muted, timestamp}`. The destination `inbox-worker` applies an unconditional `$set` on the local subscription mirror. +- **Notification delivery:** `notification-worker` does **not** yet consult `muted` before sending. End-to-end mute behaviour is wired only as far as the persisted flag; honouring it in fan-out is a follow-up. --- diff --git a/inbox-worker/handler.go b/inbox-worker/handler.go index 7b4fc2df9..0c1aca617 100644 --- a/inbox-worker/handler.go +++ b/inbox-worker/handler.go @@ -29,8 +29,8 @@ type InboxStore interface { // silent no-ops. Missing-subscription is also a silent no-op. UpdateSubscriptionRead(ctx context.Context, roomID, account string, lastSeenAt time.Time, alert bool) error UpsertThreadSubscription(ctx context.Context, sub *model.ThreadSubscription) error - // UpdateSubscriptionMute sets disableNotifications by (roomID, account); missing-sub is a silent no-op for federation races. - UpdateSubscriptionMute(ctx context.Context, roomID, account string, disableNotifications bool) error + // UpdateSubscriptionMute sets muted by (roomID, account); missing-sub is a silent no-op for federation races. + UpdateSubscriptionMute(ctx context.Context, roomID, account string, muted bool) error } // Handler processes cross-site OutboxEvent messages; replicates only subscription/room metadata, never room keys. @@ -209,7 +209,7 @@ func (h *Handler) handleSubscriptionMuteToggled(ctx context.Context, evt *model. if err := json.Unmarshal(evt.Payload, &e); err != nil { return fmt.Errorf("unmarshal subscription_mute_toggled payload: %w", err) } - if err := h.store.UpdateSubscriptionMute(ctx, e.RoomID, e.Account, e.DisableNotifications); err != nil { + if err := h.store.UpdateSubscriptionMute(ctx, e.RoomID, e.Account, e.Muted); err != nil { return fmt.Errorf("update subscription mute for %q in room %q: %w", e.Account, e.RoomID, err) } return nil diff --git a/inbox-worker/handler_test.go b/inbox-worker/handler_test.go index ea2921bad..22a75e69f 100644 --- a/inbox-worker/handler_test.go +++ b/inbox-worker/handler_test.go @@ -143,12 +143,12 @@ func (s *stubInboxStore) BulkCreateSubscriptions(_ context.Context, subs []*mode return nil } -func (s *stubInboxStore) UpdateSubscriptionMute(_ context.Context, roomID, account string, disableNotifications bool) error { +func (s *stubInboxStore) UpdateSubscriptionMute(_ context.Context, roomID, account string, muted bool) error { s.mu.Lock() defer s.mu.Unlock() for i := range s.subscriptions { if s.subscriptions[i].RoomID == roomID && s.subscriptions[i].User.Account == account { - s.subscriptions[i].DisableNotifications = disableNotifications + s.subscriptions[i].Muted = muted return nil } } @@ -1215,7 +1215,7 @@ func TestHandler_SubscriptionMuteToggled(t *testing.T) { h := NewHandler(store) payload, err := json.Marshal(model.SubscriptionMuteToggledEvent{ - Account: "alice", RoomID: "r1", DisableNotifications: true, Timestamp: 12345, + Account: "alice", RoomID: "r1", Muted: true, Timestamp: 12345, }) require.NoError(t, err) evt, err := json.Marshal(model.OutboxEvent{ @@ -1228,7 +1228,7 @@ func TestHandler_SubscriptionMuteToggled(t *testing.T) { subs := store.getSubscriptions() require.Len(t, subs, 1) - assert.True(t, subs[0].DisableNotifications) + assert.True(t, subs[0].Muted) } func TestHandler_SubscriptionMuteToggled_MissingSubscriptionNoOp(t *testing.T) { @@ -1236,7 +1236,7 @@ func TestHandler_SubscriptionMuteToggled_MissingSubscriptionNoOp(t *testing.T) { h := NewHandler(store) payload, err := json.Marshal(model.SubscriptionMuteToggledEvent{ - Account: "ghost", RoomID: "r1", DisableNotifications: true, Timestamp: 12345, + Account: "ghost", RoomID: "r1", Muted: true, Timestamp: 12345, }) require.NoError(t, err) evt, err := json.Marshal(model.OutboxEvent{ diff --git a/inbox-worker/main.go b/inbox-worker/main.go index 33648e798..706d8bd68 100644 --- a/inbox-worker/main.go +++ b/inbox-worker/main.go @@ -115,11 +115,11 @@ func (s *mongoInboxStore) BulkCreateSubscriptions(ctx context.Context, subs []*m return nil } -// UpdateSubscriptionMute sets disableNotifications by (roomID, account); missing is a silent no-op. -func (s *mongoInboxStore) UpdateSubscriptionMute(ctx context.Context, roomID, account string, disableNotifications bool) error { +// UpdateSubscriptionMute sets muted by (roomID, account); missing is a silent no-op. +func (s *mongoInboxStore) UpdateSubscriptionMute(ctx context.Context, roomID, account string, muted bool) error { _, err := s.subCol.UpdateOne(ctx, bson.M{"roomId": roomID, "u.account": account}, - bson.M{"$set": bson.M{"disableNotifications": disableNotifications}}, + bson.M{"$set": bson.M{"muted": muted}}, ) if err != nil { return fmt.Errorf("update subscription mute for %q in room %q: %w", account, roomID, err) diff --git a/pkg/model/event.go b/pkg/model/event.go index 7261d00c3..7324ddf8a 100644 --- a/pkg/model/event.go +++ b/pkg/model/event.go @@ -211,16 +211,16 @@ type RoomKeyEnsureResponse struct { // MuteToggleResponse is the sync reply for the mute.toggle RPC. type MuteToggleResponse struct { - Status string `json:"status"` - DisableNotifications bool `json:"disableNotifications"` + Status string `json:"status"` + Muted bool `json:"muted"` } // SubscriptionMuteToggledEvent is the OutboxEvent.Payload for type "subscription_mute_toggled". type SubscriptionMuteToggledEvent struct { - Account string `json:"account" bson:"account"` - RoomID string `json:"roomId" bson:"roomId"` - DisableNotifications bool `json:"disableNotifications" bson:"disableNotifications"` - Timestamp int64 `json:"timestamp" bson:"timestamp"` + Account string `json:"account" bson:"account"` + RoomID string `json:"roomId" bson:"roomId"` + Muted bool `json:"muted" bson:"muted"` + Timestamp int64 `json:"timestamp" bson:"timestamp"` } type MemberRemoveEvent struct { diff --git a/pkg/model/model_test.go b/pkg/model/model_test.go index 54a36b725..3765c807e 100644 --- a/pkg/model/model_test.go +++ b/pkg/model/model_test.go @@ -465,19 +465,19 @@ func TestSubscriptionJSON(t *testing.T) { hss := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) lsa := time.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC) s := model.Subscription{ - ID: "s1", - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", - RoomType: model.RoomTypeChannel, - SiteID: "site-a", - Roles: []model.Role{model.RoleOwner}, - HistorySharedSince: &hss, - JoinedAt: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), - LastSeenAt: &lsa, - HasMention: true, - ThreadUnread: []string{"parent-1", "parent-2"}, - Alert: true, - DisableNotifications: true, + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + RoomType: model.RoomTypeChannel, + SiteID: "site-a", + Roles: []model.Role{model.RoleOwner}, + HistorySharedSince: &hss, + JoinedAt: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), + LastSeenAt: &lsa, + HasMention: true, + ThreadUnread: []string{"parent-1", "parent-2"}, + Alert: true, + Muted: true, } roundTrip(t, &s, &model.Subscription{}) }) @@ -529,9 +529,9 @@ func TestSubscriptionJSON_ThreadUnreadOmittedAlertAlwaysPresent(t *testing.T) { assert.True(t, hasAlert, "alert must be present in JSON even when false") assert.Equal(t, false, alertVal) - disableVal, hasDisable := raw["disableNotifications"] - assert.True(t, hasDisable, "disableNotifications must be present in JSON even when false") - assert.Equal(t, false, disableVal) + mutedVal, hasMuted := raw["muted"] + assert.True(t, hasMuted, "muted must be present in JSON even when false") + assert.Equal(t, false, mutedVal) var dst model.Subscription require.NoError(t, json.Unmarshal(data, &dst)) @@ -2230,8 +2230,8 @@ func TestMessageTypeAndAsyncJobStatusConstants(t *testing.T) { func TestMuteToggleResponseJSON(t *testing.T) { src := model.MuteToggleResponse{ - Status: "ok", - DisableNotifications: true, + Status: "ok", + Muted: true, } data, err := json.Marshal(src) require.NoError(t, err) @@ -2243,15 +2243,15 @@ func TestMuteToggleResponseJSON(t *testing.T) { var raw map[string]any require.NoError(t, json.Unmarshal(data, &raw)) assert.Equal(t, "ok", raw["status"]) - assert.Equal(t, true, raw["disableNotifications"]) + assert.Equal(t, true, raw["muted"]) } func TestSubscriptionMuteToggledEventJSON(t *testing.T) { src := model.SubscriptionMuteToggledEvent{ - Account: "alice", - RoomID: "r1", - DisableNotifications: true, - Timestamp: 1234567890, + Account: "alice", + RoomID: "r1", + Muted: true, + Timestamp: 1234567890, } data, err := json.Marshal(src) require.NoError(t, err) diff --git a/pkg/model/subscription.go b/pkg/model/subscription.go index b1ba71f3a..8fffb8284 100644 --- a/pkg/model/subscription.go +++ b/pkg/model/subscription.go @@ -25,21 +25,21 @@ type SubscriptionUser struct { } type Subscription struct { - ID string `json:"id" bson:"_id"` - User SubscriptionUser `json:"u" bson:"u"` - RoomID string `json:"roomId" bson:"roomId"` - SiteID string `json:"siteId" bson:"siteId"` - Roles []Role `json:"roles" bson:"roles"` - Name string `json:"name" bson:"name"` - RoomType RoomType `json:"roomType" bson:"roomType"` - IsSubscribed bool `json:"isSubscribed,omitempty" bson:"isSubscribed,omitempty"` - HistorySharedSince *time.Time `json:"historySharedSince,omitempty" bson:"historySharedSince,omitempty"` - JoinedAt time.Time `json:"joinedAt" bson:"joinedAt"` - LastSeenAt *time.Time `json:"lastSeenAt,omitempty" bson:"lastSeenAt,omitempty"` - HasMention bool `json:"hasMention" bson:"hasMention"` - ThreadUnread []string `json:"threadUnread,omitempty" bson:"threadUnread,omitempty"` - Alert bool `json:"alert" bson:"alert"` - DisableNotifications bool `json:"disableNotifications" bson:"disableNotifications"` + ID string `json:"id" bson:"_id"` + User SubscriptionUser `json:"u" bson:"u"` + RoomID string `json:"roomId" bson:"roomId"` + SiteID string `json:"siteId" bson:"siteId"` + Roles []Role `json:"roles" bson:"roles"` + Name string `json:"name" bson:"name"` + RoomType RoomType `json:"roomType" bson:"roomType"` + IsSubscribed bool `json:"isSubscribed,omitempty" bson:"isSubscribed,omitempty"` + HistorySharedSince *time.Time `json:"historySharedSince,omitempty" bson:"historySharedSince,omitempty"` + JoinedAt time.Time `json:"joinedAt" bson:"joinedAt"` + LastSeenAt *time.Time `json:"lastSeenAt,omitempty" bson:"lastSeenAt,omitempty"` + HasMention bool `json:"hasMention" bson:"hasMention"` + ThreadUnread []string `json:"threadUnread,omitempty" bson:"threadUnread,omitempty"` + Alert bool `json:"alert" bson:"alert"` + Muted bool `json:"muted" bson:"muted"` } // SubscriptionHRInfo carries the counterpart's HR-directory record on a diff --git a/room-service/handler.go b/room-service/handler.go index bead73578..7fed22415 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -1344,10 +1344,10 @@ func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ( } if userSiteID != "" && userSiteID != h.siteID { payload := model.SubscriptionMuteToggledEvent{ - Account: account, - RoomID: roomID, - DisableNotifications: sub.DisableNotifications, - Timestamp: now.UnixMilli(), + Account: account, + RoomID: roomID, + Muted: sub.Muted, + Timestamp: now.UnixMilli(), } payloadData, err := json.Marshal(payload) if err != nil { @@ -1369,5 +1369,5 @@ func (h *Handler) handleMuteToggle(ctx context.Context, subj string, _ []byte) ( } } - return json.Marshal(model.MuteToggleResponse{Status: "ok", DisableNotifications: sub.DisableNotifications}) + return json.Marshal(model.MuteToggleResponse{Status: "ok", Muted: sub.Muted}) } diff --git a/room-service/handler_test.go b/room-service/handler_test.go index c9633b2a4..aeb69b85a 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -3395,11 +3395,11 @@ func TestHandler_MuteToggle_Success(t *testing.T) { store.EXPECT(). ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). Return(&model.Subscription{ - ID: "s1", - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", - SiteID: "site-a", - DisableNotifications: true, + ID: "s1", + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + Muted: true, }, nil) store.EXPECT(). GetUserSiteID(gomock.Any(), "alice"). @@ -3428,7 +3428,7 @@ func TestHandler_MuteToggle_Success(t *testing.T) { var got model.MuteToggleResponse require.NoError(t, json.Unmarshal(resp, &got)) assert.Equal(t, "ok", got.Status) - assert.True(t, got.DisableNotifications) + assert.True(t, got.Muted) require.Len(t, coreSubjects, 1) assert.Equal(t, subject.SubscriptionUpdate("alice"), coreSubjects[0]) @@ -3436,7 +3436,7 @@ func TestHandler_MuteToggle_Success(t *testing.T) { var evt model.SubscriptionUpdateEvent require.NoError(t, json.Unmarshal(coreBodies[0], &evt)) assert.Equal(t, "mute_toggled", evt.Action) - assert.True(t, evt.Subscription.DisableNotifications) + assert.True(t, evt.Subscription.Muted) assert.Equal(t, "alice", evt.Subscription.User.Account) } @@ -3447,10 +3447,10 @@ func TestHandler_MuteToggle_CrossSitePublishesOutbox(t *testing.T) { store.EXPECT(). ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). Return(&model.Subscription{ - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", - SiteID: "site-a", - DisableNotifications: true, + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + Muted: true, }, nil) store.EXPECT(). GetUserSiteID(gomock.Any(), "alice"). @@ -3484,7 +3484,7 @@ func TestHandler_MuteToggle_CrossSitePublishesOutbox(t *testing.T) { require.NoError(t, json.Unmarshal(outbox.Payload, &payload)) assert.Equal(t, "alice", payload.Account) assert.Equal(t, "r1", payload.RoomID) - assert.True(t, payload.DisableNotifications) + assert.True(t, payload.Muted) assert.NotZero(t, payload.Timestamp) } @@ -3572,10 +3572,10 @@ func TestHandler_MuteToggle_CrossSiteOutboxPublishFailure(t *testing.T) { store.EXPECT(). ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). Return(&model.Subscription{ - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", - SiteID: "site-a", - DisableNotifications: true, + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + Muted: true, }, nil) store.EXPECT(). GetUserSiteID(gomock.Any(), "alice"). @@ -3605,10 +3605,10 @@ func TestHandler_MuteToggle_CorePublishFailureIsNonFatal(t *testing.T) { store.EXPECT(). ToggleSubscriptionMute(gomock.Any(), "r1", "alice"). Return(&model.Subscription{ - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", - SiteID: "site-a", - DisableNotifications: true, + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + SiteID: "site-a", + Muted: true, }, nil) store.EXPECT(). GetUserSiteID(gomock.Any(), "alice"). @@ -3632,5 +3632,5 @@ func TestHandler_MuteToggle_CorePublishFailureIsNonFatal(t *testing.T) { var got model.MuteToggleResponse require.NoError(t, json.Unmarshal(resp, &got)) assert.Equal(t, "ok", got.Status) - assert.True(t, got.DisableNotifications) + assert.True(t, got.Muted) } diff --git a/room-service/integration_test.go b/room-service/integration_test.go index d86d2116e..20af00e66 100644 --- a/room-service/integration_test.go +++ b/room-service/integration_test.go @@ -1779,32 +1779,32 @@ func TestMongoStore_ToggleSubscriptionMute(t *testing.T) { ctx := context.Background() sub := &model.Subscription{ - ID: idgen.GenerateUUIDv7(), - User: model.SubscriptionUser{ID: "u1", Account: "alice"}, - RoomID: "r1", - RoomType: model.RoomTypeChannel, - SiteID: "site-a", - Roles: []model.Role{model.RoleMember}, - JoinedAt: time.Now().UTC(), - DisableNotifications: false, + ID: idgen.GenerateUUIDv7(), + User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "r1", + RoomType: model.RoomTypeChannel, + SiteID: "site-a", + Roles: []model.Role{model.RoleMember}, + JoinedAt: time.Now().UTC(), + Muted: false, } mustInsertSub(t, db, sub) got, err := store.ToggleSubscriptionMute(ctx, "r1", "alice") require.NoError(t, err) require.NotNil(t, got) - assert.True(t, got.DisableNotifications) + assert.True(t, got.Muted) assert.Equal(t, "alice", got.User.Account) assert.Equal(t, "r1", got.RoomID) persisted, err := store.GetSubscription(ctx, "alice", "r1") require.NoError(t, err) - assert.True(t, persisted.DisableNotifications) + assert.True(t, persisted.Muted) got, err = store.ToggleSubscriptionMute(ctx, "r1", "alice") require.NoError(t, err) require.NotNil(t, got) - assert.False(t, got.DisableNotifications) + assert.False(t, got.Muted) assert.Equal(t, "alice", got.User.Account) assert.Equal(t, "r1", got.RoomID) diff --git a/room-service/store.go b/room-service/store.go index 708c90030..fdda72c09 100644 --- a/room-service/store.go +++ b/room-service/store.go @@ -84,7 +84,7 @@ type RoomStore interface { // keyed by (roomID, account). Returns model.ErrSubscriptionNotFound // (wrapped) when no subscription matches. UpdateSubscriptionRead(ctx context.Context, roomID, account string, lastSeenAt time.Time, alert bool) error - // ToggleSubscriptionMute atomically flips disableNotifications via a single FindOneAndUpdate. + // ToggleSubscriptionMute atomically flips muted via a single FindOneAndUpdate. // Returns the post-flip subscription, or model.ErrSubscriptionNotFound (wrapped) when no match. ToggleSubscriptionMute(ctx context.Context, roomID, account string) (*model.Subscription, error) // GetUserSiteID returns the home site of a user looked up by account. diff --git a/room-service/store_mongo.go b/room-service/store_mongo.go index ebaf572c9..070702d3d 100644 --- a/room-service/store_mongo.go +++ b/room-service/store_mongo.go @@ -804,14 +804,14 @@ func (s *MongoStore) UpdateSubscriptionRead(ctx context.Context, roomID, account return nil } -// ToggleSubscriptionMute atomically flips disableNotifications via FindOneAndUpdate. +// ToggleSubscriptionMute atomically flips muted via FindOneAndUpdate. // $ifNull treats absent field as false so legacy docs toggle to true on first call. func (s *MongoStore) ToggleSubscriptionMute(ctx context.Context, roomID, account string) (*model.Subscription, error) { filter := bson.M{"roomId": roomID, "u.account": account} update := mongo.Pipeline{ bson.D{{Key: "$set", Value: bson.M{ - "disableNotifications": bson.M{"$not": bson.A{ - bson.M{"$ifNull": bson.A{"$disableNotifications", false}}, + "muted": bson.M{"$not": bson.A{ + bson.M{"$ifNull": bson.A{"$muted", false}}, }}, }}}, } diff --git a/room-worker/integration_test.go b/room-worker/integration_test.go index ac43ff434..d7f7e5379 100644 --- a/room-worker/integration_test.go +++ b/room-worker/integration_test.go @@ -1323,7 +1323,7 @@ func TestIntegration_CreateRoom_FansOutRoomKeyEvent(t *testing.T) { // TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration locks in that // processCreateRoom's botDM branch keeps its insert-only contract on a // JetStream redelivery: a pre-existing muted, inactive botDM subscription -// must NOT be refreshed (DisableNotifications, IsSubscribed, JoinedAt +// must NOT be refreshed (Muted, IsSubscribed, JoinedAt // untouched). The re-subscribe refresh semantic is owned by user-service. func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { ctx := context.Background() @@ -1348,15 +1348,15 @@ func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { Accounts: []string{"alice", "helper.bot"}, }) mustInsertSub(t, db, &model.Subscription{ - ID: "existing-human-sub", - User: model.SubscriptionUser{ID: "u_alice", Account: "alice"}, - RoomID: roomID, - SiteID: "site-A", - RoomType: model.RoomTypeBotDM, - Name: "helper.bot", - IsSubscribed: false, - DisableNotifications: true, - JoinedAt: oldJoinedAt, + ID: "existing-human-sub", + User: model.SubscriptionUser{ID: "u_alice", Account: "alice"}, + RoomID: roomID, + SiteID: "site-A", + RoomType: model.RoomTypeBotDM, + Name: "helper.bot", + IsSubscribed: false, + Muted: true, + JoinedAt: oldJoinedAt, }) mustInsertSub(t, db, &model.Subscription{ ID: "existing-bot-sub", @@ -1385,8 +1385,8 @@ func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { got, err := store.GetSubscription(ctx, "alice", roomID) require.NoError(t, err) - assert.True(t, got.DisableNotifications, - "botDM path must NOT clear DisableNotifications on redelivery (insert-only contract)") + assert.True(t, got.Muted, + "botDM path must NOT clear Muted on redelivery (insert-only contract)") assert.False(t, got.IsSubscribed, "botDM path must NOT refresh IsSubscribed on redelivery (insert-only contract)") assert.True(t, got.JoinedAt.Equal(oldJoinedAt), @@ -1400,7 +1400,7 @@ func TestProcessCreateRoom_BotDM_DoesNotUpsert_Integration(t *testing.T) { // TestProcessCreateRoom_DM_DoesNotUpsert_Integration locks in that // processCreateRoom's regular-DM branch keeps its insert-only contract: // a pre-existing regular-DM subscription's state (specifically -// DisableNotifications = true and an old JoinedAt) must NOT be refreshed +// Muted = true and an old JoinedAt) must NOT be refreshed // when processCreateRoom is replayed for the same (room, user) pair. // This regression guard prevents accidental upsert wiring on the DM // branch in future edits. @@ -1428,14 +1428,14 @@ func TestProcessCreateRoom_DM_DoesNotUpsert_Integration(t *testing.T) { Accounts: []string{"alice", "bob"}, }) mustInsertSub(t, db, &model.Subscription{ - ID: "existing-alice-sub", - User: model.SubscriptionUser{ID: "u_alice", Account: "alice"}, - RoomID: roomID, - SiteID: "site-A", - RoomType: model.RoomTypeDM, - Name: "bob", - DisableNotifications: true, - JoinedAt: oldJoinedAt, + ID: "existing-alice-sub", + User: model.SubscriptionUser{ID: "u_alice", Account: "alice"}, + RoomID: roomID, + SiteID: "site-A", + RoomType: model.RoomTypeDM, + Name: "bob", + Muted: true, + JoinedAt: oldJoinedAt, }) mustInsertSub(t, db, &model.Subscription{ ID: "existing-bob-sub", @@ -1463,8 +1463,8 @@ func TestProcessCreateRoom_DM_DoesNotUpsert_Integration(t *testing.T) { got, err := store.GetSubscription(ctx, "alice", roomID) require.NoError(t, err) - assert.True(t, got.DisableNotifications, - "regular-DM path must NOT clear DisableNotifications on re-create (insert-only contract)") + assert.True(t, got.Muted, + "regular-DM path must NOT clear Muted on re-create (insert-only contract)") assert.True(t, got.JoinedAt.Equal(oldJoinedAt), "regular-DM path must NOT refresh JoinedAt on re-create (insert-only contract)") From d44c8c91cd64d7fad2e60a13f407b34fdbc531c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 22 May 2026 07:16:13 +0000 Subject: [PATCH 32/32] chore: drop branch review artifact The maintainer flagged that the multi-lens review file shouldn't ship to main. It was a working artifact for the fix-up cycle; the conclusions are captured in the implementation + PR review threads. --- ...e-explore-room-service-7tNlq-2026-05-21.md | 351 ------------------ 1 file changed, 351 deletions(-) delete mode 100644 docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md diff --git a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md b/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md deleted file mode 100644 index a60205fe6..000000000 --- a/docs/reviews/branch-claude-explore-room-service-7tNlq-2026-05-21.md +++ /dev/null @@ -1,351 +0,0 @@ -# Branch Review: `claude/explore-room-service-7tNlq` - -**Date:** 2026-05-21 -**Base:** `fd7b4e2` (last commit before this branch on the upstream lineage) -**HEAD:** `8277488` -**Commits on branch:** 11 (plan docs + 9 implementation commits) -**Author:** Claude - -## Scope - -Adds a new per-user `mute.toggle` RPC to `room-service`, mirrors the toggle across sites via `inbox-worker`, renames `Subscription.DisableNotification` → `DisableNotifications` repo-wide, and documents the new RPC in `docs/client-api.md`. - -## Services touched - -| Service | Magnitude | Note | -|---|---|---| -| `room-service` | substantive | New handler + store method + `publishCore` wiring + 5 unit tests + 1 integration test | -| `inbox-worker` | medium | New dispatch case + store method + 3 unit tests | -| `room-worker` | minimal | Field-rename in one integration test only — no production code touched | - -Shared `pkg/` files also changed: `pkg/model/event.go`, `pkg/model/subscription.go`, `pkg/model/model_test.go`, `pkg/subject/subject.go`, `pkg/subject/subject_test.go`. - -## Findings count (deduped across all lenses) - -| Severity | Count | -|---|---| -| critical | 0 | -| high | 1 | -| medium | 10 | -| low | 9 | -| nitpick | 6 | - -## Top-line risk assessment - -**Functionally correct and mergeable after addressing a small set of issues.** The implementation faithfully follows the `message.read` precedent (Pattern B): inline write in `room-service`, cross-site outbox publish, inbox-worker mirror. SAST is clean (gosec PASS, govulncheck PASS; semgrep unavailable in env — not a code defect). `make test` and `make lint` both green. The integration test for the new store method passes. - -The notable items to address before merge: - -1. **A reachable bug where `GetUserSiteID` errors silently abandon the cross-site outbox publish** (3 reviewers independently flagged this), causing permanent cross-site mute divergence on transient DB hiccups. Inconsistent with `handleMessageRead`'s precedent which propagates the error. -2. **`sanitizeError` doesn't pass through the new `"invalid mute-toggle subject: …"` error string**, so clients receive `"internal error"` instead of the message documented in this same PR's `client-api.md` update — the API contract is broken at delivery. -3. **`SubscriptionUpdateEvent.Action` field comment is now stale** — claims `"added" | "removed"` but the new code adds `"mute_toggled"` (and earlier work added `"role_updated"`). Client-facing contract drift. -4. A TOCTOU window between `GetSubscription` and `ToggleSubscriptionMute` that adds a redundant Mongo round-trip per request. -5. Missing tests for several error paths (outbox publish failure, `GetSubscription` generic error, `publishCore` failure, `GetUserSiteID` failure soft path) — coverage of `handleMuteToggle` lands at ~70–75% vs the 90%+ target for core handler logic. - -None of these are blocking-critical; the branch can ship after a small fix-up commit. - ---- - -## Service: room-service - -### (a) Diff correctness against existing conventions - -**[medium]** `room-service/handler.go:1230` — `natsMuteToggle` logs `slog.Error("mute toggle failed", "error", err, "subject", m.Msg.Subject)` with an extra `subject` structured field that the analogous `natsMessageRead` (line 971) and `natsUpdateRole` etc. do not. Either propagate the field across all handlers or drop it here. Inconsistency only. - -**[nitpick]** `room-service/handler.go:1253-1255` — `handleMuteToggle` calls `ToggleSubscriptionMute` and re-checks `ErrSubscriptionNotFound` even though the preceding `GetSubscription` guard at line 1245 should prevent that branch. Dead-code defense-in-depth that could mislead a future reader about atomicity guarantees. - -### (b) Scope drift / refactor-readiness - -**[low]** room-service is cohesive; `mute.toggle` is a per-subscription mutation that the service already owns (same `subscriptions` collection used by `UpdateSubscriptionRead`). No scope drift. `handler.go` is large (~1300 lines) but already was — the new handler follows the established length pattern. No split needed. - -### (c) Abstraction changes - -**[low]** `publishCore` closure injection is justified. room-service already had `publishToStream` for JetStream; core-NATS publish for `subscription.update` fan-out needs a separate path because the APIs differ (`js.PublishMsg` returns `(*PubAck, error)`, `nc.PublishMsg` returns only `error`). The shape mirrors `publishToStream`, it's injected in `main.go` at line 119, and existing tests that don't exercise it pass `nil`. No premature abstraction. - -**[low]** `NewHandler` now has 10 positional parameters (`handler.go:44`). Borderline; a config struct would be cleaner. Pre-existing issue, not introduced by this PR but worsened by it. - -### (d) Design coherence - -The mute.toggle flow is a textbook match for room-service's job: validate membership → atomic Mongo write → fan out `subscription.update` core event → cross-site outbox. The plan's Pattern-B (`message.read` shape) is exactly what landed. - -### (e) Project-pattern adherence - -- **`pkg/subject` builders**: `subject.MuteToggleWildcard`, `subject.MuteToggle`, `subject.ParseUserRoomSubject`, `subject.SubscriptionUpdate`, `subject.Outbox` — all used correctly; no raw `fmt.Sprintf` subjects in new code. ✓ -- **Outbox pattern**: `publishToStream` called with `subject.Outbox(h.siteID, userSiteID, model.OutboxSubscriptionMuteToggled)` at line 1307. ✓ -- **`Timestamp int64` on new event structs**: `SubscriptionMuteToggledEvent.Timestamp` set at the publish site (`now.UnixMilli()`, line 1290). `OutboxEvent.Timestamp` set at line 1301. `SubscriptionUpdateEvent.Timestamp` set at line 1269. All correct. ✓ - -**[medium]** `room-service/handler.go:1280-1283` — When `GetUserSiteID` errors, the handler logs `slog.Warn` and returns `{status: "ok"}` while silently dropping the cross-site outbox publish. The peer handler `handleMessageRead` (line 1023) treats `GetUserSiteID` failure as a hard error returned to the caller. The asymmetry means a transient DB failure on `users` causes silent cross-site divergence with no client-visible signal — the user mutes on the room site but their home site keeps the old value forever. Either match `handleMessageRead`'s hard-error behaviour or document the intentional degraded-mode trade-off (and consider a retry/backfill mechanism). - -### (f) Client-API doc rule - -`docs/client-api.md` is updated in this branch with a full new "Toggle Mute" section (50 lines). Subject, request body, success response, error cases, triggered events, cross-site behaviour, and the `notification-worker` follow-up caveat are all documented. ✓ - -**[low]** The docs section lists the not-a-member error as `"only room members can list members"` (the reused `errNotRoomMember` sentinel from `helper.go:25`). The string is semantically misleading for a mute operation. Either rename the sentinel or call out the reuse in the docs note. - -### Verdict - -The implementation is functionally correct and pattern-compliant. The one actionable blocker is the silent skip of the cross-site outbox on `GetUserSiteID` error (`handler.go:1280-1283`), which diverges from `handleMessageRead`'s precedent and risks undetected cross-site inconsistency. - ---- - -## Service: inbox-worker - -### (a) Diff correctness against existing handler conventions - -**[low]** The new `handleSubscriptionMuteToggled` (`handler.go:212-221`) is structurally identical to `handleSubscriptionRead` (`handler.go:196-206`): unmarshal inner event → call store method → wrap error with `"short description: %w"`. Dispatch case is placed immediately after `subscription_read` in the switch, mirroring how `subscription_read` was slotted between `role_updated` and `thread_subscription_upserted`. Shape correct, no issues. - -### (b) Scope drift / refactor-readiness - -**[medium]** `inbox-worker/main.go:39-195` — The `mongoInboxStore` struct and all its methods remain embedded in `main.go`, which now spans ~200 lines of store implementation before `main()` even starts. The plan (Task 8 step 1) explicitly noted: "Modify: `inbox-worker/store.go` (interface) and `inbox-worker/store_mongo.go` (impl) — confirm exact filenames first." The split never happened. Adding `UpdateSubscriptionMute` to `main.go:118-129` follows the existing pattern, but the growing inline store is now clearly beyond the per-service layout in CLAUDE.md (`main.go` = config + wiring + startup). Pre-existing tech debt that this PR adds to. Worth a follow-up split before the next handler grows the inline store further. - -### (c) Abstraction changes - -**[low]** `UpdateSubscriptionMute` earns its keep. It parallels `UpdateSubscriptionRead` one-to-one in the interface (`handler.go:33-36`) and in the Mongo impl (`main.go:120-129`). The no-op-on-missing-subscription semantic is documented in both the interface comment and the store comment. - -**[nitpick]** `inbox-worker/main.go:128` — `UpdateSubscriptionMute` discards `res *UpdateResult` (assigns to `_`) and does NOT check `res.MatchedCount` before returning nil, while `UpdateSubscriptionRoles` (`main.go:60-70`) DOES return an error on `MatchedCount == 0`. The asymmetry is intentional (plan explicitly requires silent no-op on missing subscription) and is documented in both the interface and store comments — but a one-line inline comment (`// MatchedCount 0 is intentional — missing subscription is a federation-race no-op`) on the `return nil` would close that reading ambiguity for the next developer. - -### (d) Design coherence - -**[low]** The handler correctly fits inbox-worker's stated job: mirror a room's-home-site write onto the user's home-site subscription copy. The `OutboxEvent.Type == "subscription_mute_toggled"` dispatch case (`handler.go:66-67`) wires correctly against the constant `model.OutboxSubscriptionMuteToggled`. The silent-no-op semantic is encoded end-to-end (store Mongo impl → store interface doc → handler comment). No design issues. - -### (e) Project-pattern adherence - -- **Federation wiring**: dispatch string `"subscription_mute_toggled"` matches `model.OutboxSubscriptionMuteToggled` (`pkg/model/event.go:85`). `handler_test.go` uses the const directly, not a bare string. ✓ -- **Mongo write**: direct driver, `bson.M` filter + `$set` update, no ORM. ✓ -- **Error wrapping**: `"unmarshal subscription_mute_toggled payload: %w"` and `"update subscription mute for %q in room %q: %w"` — both follow the `"short description: %w"` convention. ✓ - -**[low]** The plan prescribed `gomock` + `make generate SERVICE=inbox-worker` for the new test (Task 8 steps 5/8). The landed implementation uses a hand-written `stubInboxStore` in `handler_test.go` instead. The plan was wrong about inbox-worker's testing style — the landed code correctly matches the existing convention. Worth noting for future plans against this service. - -### (f) Client-API doc rule - -N/A — inbox-worker has no `nc.QueueSubscribe` handlers on `chat.user.…` client-facing subjects. - -### Test coverage - -Three targeted tests cover the happy path (`TestHandler_SubscriptionMuteToggled`), missing-subscription no-op (`TestHandler_SubscriptionMuteToggled_MissingSubscriptionNoOp`), and malformed payload (`TestHandler_SubscriptionMuteToggled_MalformedPayload`). Missing: a store-error propagation test (see Test-automation chapter). - -### Verdict - -The diff is mechanically correct and safe to merge. Actionable items: a one-line clarity comment on the intentional `MatchedCount` omission (`main.go:128`), and a follow-up PR to split the inline store from `main.go` into `store.go`/`store_mongo.go` before the next handler grows it further. - ---- - -## Service: room-worker - -Scope: Test-only change — `room-worker/integration_test.go` had 8 occurrences of `DisableNotification` renamed to `DisableNotifications` (matching the field-rename in `pkg/model/subscription.go:42`). No production code in `room-worker/` was touched. - -### Findings - -All 8 occurrences are renamed and semantically equivalent: -- Struct-literal fixtures (lines ~1320, ~1397): the boolean values are preserved. -- `assert.True` calls and their message strings (lines ~1350, ~1352, ~1432, ~1434): same field, new name. -- Doc-comment references (lines ~1288, ~1365): updated for accuracy. - -The cosmetic alignment adjustment (one space → two spaces after the colon to maintain column alignment with surrounding fields) is consistent with the rest of the struct literal. - -The test continues to use `testutil.MongoDB`, `testutil.NATS`, and a `TestMain` with `testutil.RunTests(m)` — consistent with CLAUDE.md Section 4. No inline container usage was introduced. Scope discipline maintained. - -### Verdict - -No findings. The change is a correct, complete mechanical rename with no omissions or semantic drift — approved as-is. - ---- - -## Go expert - -### Findings - -**[high]** `room-service/handler.go:1245-1264` — TOCTOU window between the pre-fetch and the atomic toggle. `GetSubscription` exists to (a) gate membership and (b) supply the full `Subscription` struct for the downstream `SubscriptionUpdateEvent`. `ToggleSubscriptionMute` already returns `ErrSubscriptionNotFound` on a missing document, so the pre-fetch primarily serves the event payload. Between the two calls a concurrent writer could delete the subscription (yielding a different error class) or update another field — the constructed `updatedSub := *sub` copy then carries pre-fetch values for everything except `DisableNotifications`, which is the post-flip value. Either project the necessary fields directly in `ToggleSubscriptionMute`'s `FindOneAndUpdate` (eliminating the pre-fetch) or accept the staleness with a comment acknowledging it. - -**[medium]** `pkg/model/event.go:35` — `SubscriptionUpdateEvent.Action` comment says `// "added" | "removed"`. This branch introduces `"mute_toggled"` (line 1267 in `room-service/handler.go`), and earlier work already added `"role_updated"`. Clients parsing this field rely on the comment as the contract. Update to `// "added" | "removed" | "role_updated" | "mute_toggled"`. - -**[medium]** `room-service/handler.go:1242` — `fmt.Errorf("invalid mute-toggle subject: %s", subj)` is not sentinel-wrapped, so `errors.Is(err, errInvalidSubject)` cannot match. The codebase already uses sentinels elsewhere (`errInvalidRole`, `errNotRoomMember`, etc.). Introducing an `errInvalidMuteToggleSubject` sentinel would also fix the `sanitizeError` passthrough issue flagged in the Bug & Security chapter. - -**[medium]** Missing test for the cross-site outbox `publishToStream` failure path. `handleMuteToggle` returns a hard error (`"publish mute-toggled outbox: %w"`) at line 1308 if the JetStream publish fails after the DB write committed. `TestHandler_MuteToggle_CrossSitePublishesOutbox` only covers the success path; the analogous `TestHandler_MessageRead_CrossSite_PublishFailureAborts` shows the pattern to follow. - -**[medium]** `room-service/handler.go:1280-1283` vs `1305-1308` — Asymmetric error handling: `GetUserSiteID` failure silently returns OK, but `publishToStream` failure on the same cross-site path returns a hard error. The asymmetry is defensible (publish has JetStream retries; the users-collection read does not) but it's undocumented and surprises a reader. Add an inline comment explaining the trade-off. - -**[low]** `room-service/handler.go:44` — `NewHandler` now has 10 positional parameters, including two adjacent `func(context.Context, string, []byte) error` closures (`publishToStream`, `publishCore`). They can be silently swapped at the call site with no compile error. A `HandlerConfig` struct or functional options would be more idiomatic at this arity. Not introduced by this PR but worsened by it. - -**[low]** `pkg/model/event.go:225` — `SubscriptionMuteToggledEvent` carries `bson` tags it will never use (the struct is only JSON-encoded as `OutboxEvent.Payload`, never stored in Mongo). Consistent with `SubscriptionReadEvent` on line 94, so accepted as pattern-conformance noise. - -**[nitpick]** `pkg/model/event.go:219` — `MuteToggleResponse.Status` is always the constant `"ok"`. Encoding a fixed string as a struct field rather than a typed const means it can only be tested by string comparison. Cosmetic. - -**[nitpick]** `inbox-worker/main.go:120-129` — `UpdateSubscriptionMute` implementation lives in `main.go` rather than a `store_mongo.go`. Consistent with the pre-existing `UpdateSubscriptionRead` on the same `mongoInboxStore`, but the pattern itself deviates from the per-service layout in CLAUDE.md. Out of scope for this PR; see inbox-worker chapter for the follow-up split recommendation. - -### Verdict - -Logically sound and well-tested for happy and most error paths. The TOCTOU window in the two-stage subscription lookup, the stale `Action` enum comment (a client-facing contract), and the missing outbox-failure test case should all be addressed before merge. - ---- - -## Test automation - -### Mock staleness check - -`make generate SERVICE=room-service` and `make generate SERVICE=inbox-worker` both produced **zero diff**. Mocks are fresh. Working tree clean after the check. - -### TDD compliance for new exported functions - -| New exported function | Test | -|---|---| -| `subject.MuteToggle` | `TestMuteToggle` ✓ | -| `subject.MuteToggleWildcard` | `TestMuteToggleWildcard` ✓ | -| `model.MuteToggleResponse` | `TestMuteToggleResponseJSON` ✓ | -| `model.SubscriptionMuteToggledEvent` | `TestSubscriptionMuteToggledEventJSON` ✓ | -| `model.OutboxSubscriptionMuteToggled` const | `TestOutboxSubscriptionMuteToggledConst` ✓ | -| `MongoStore.ToggleSubscriptionMute` (room-service) | `TestMongoStore_ToggleSubscriptionMute` ✓ | -| `mongoInboxStore.UpdateSubscriptionMute` (inbox-worker) | **MISSING — see [low] below** | - -All other new functions are unexported. TDD compliance: PASS at the unit level, partial gap at integration level. - -### Findings - -**[medium]** `room-service/handler_test.go` — `handleMuteToggle`'s `GetSubscription` **generic** error arm (`case err != nil` at `handler.go:1249`, returns `"get subscription: %w"`) has no test. `TestHandler_MuteToggle_NotRoomMember` covers `ErrSubscriptionNotFound` but the generic-error path is untested. The analogous `TestHandler_MessageRead_GetRoomError` shows the expected pattern. - -**[medium]** `room-service/handler_test.go` — `handleMuteToggle`'s **cross-site outbox `publishToStream` failure** is untested. The handler returns a hard error (`"publish mute-toggled outbox: %w"`) at line 1308; `TestHandler_MuteToggle_CrossSitePublishesOutbox` only covers success. See `TestHandler_MessageRead_CrossSite_PublishFailureAborts` for the pattern. - -**[low]** `room-service/handler_test.go` — `GetUserSiteID` non-fatal soft path untested. When `GetUserSiteID` returns an error, the handler logs a warning and still returns `"ok"`. This unusual "success despite store error" behaviour deserves a test to pin the intent, especially since it's inconsistent with `handleMessageRead`'s hard-error treatment of the same call. - -**[low]** `room-service/handler_test.go` — `publishCore` non-fatal failure untested. Line 1275-1277 swallows the error with `slog.Error` and continues. No test asserts that the handler still returns `"ok"` when `publishCore` fails. - -**[low]** `inbox-worker/handler_test.go` — `UpdateSubscriptionMute` **store-error propagation** untested. `handleSubscriptionMuteToggled` (line 217) wraps store errors and returns them. The existing pattern in the same file uses small embedded-stub types (e.g. `errorDeleteStore`, `errorThreadSubStore`) to inject store errors. Missing: a `TestHandler_SubscriptionMuteToggled_StoreError` analogue. - -**[low]** `inbox-worker/integration_test.go` — `mongoInboxStore.UpdateSubscriptionMute` has no integration test. All other `mongoInboxStore` methods have counterpart integration tests; this method was added to `main.go` without one. Below the 80% coverage floor required by CLAUDE.md §4 for new Mongo code paths. - -**[nitpick]** `room-service/handler_test.go:3205-3360` — Five flat `TestHandler_MuteToggle_*` functions could be a single table-driven test. They share the same call site (`h.handleMuteToggle`) and differ only in setup/expectation. The file already shows the table-driven style (`TestHandler_handleMessageReadReceipt` at line 2800). - -### Coverage depth - -- `handleMuteToggle` (5 tests): ~70-75% branch coverage. Missing: `GetSubscription` generic error, `GetUserSiteID` error (non-fatal), `publishCore` failure, `publishToStream` failure (cross-site). Below the 90% target for core handler logic. -- `handleSubscriptionMuteToggled` (3 tests): ~75% branch coverage. Missing: store error propagation. -- `TestMongoStore_ToggleSubscriptionMute`: solid — covers false→true, persistence, true→false, not-found sentinel. - -### Mock/integration discipline + race detector - -- Unit tests use gomock in `room-service`, hand-written stubs in `inbox-worker` — both correct for their service's existing convention. -- No inline `testcontainers` added. -- `TestMain` already exists in both `integration_test.go` packages. -- No raw `go test` invocations added — everything runs through `make test` which already passes `-race`. - -### Verdict - -No `critical` or `high` findings. Mocks are fresh, TDD discipline followed at the exported-function level, all tests pass with `-race`. The two `medium` gaps (`GetSubscription` generic error + `publishToStream` cross-site failure) bring effective handler branch coverage below the 90% target — each is a small mechanical addition following patterns already in the same file. - ---- - -## Bug & security - -### SAST results - -| Tool | Result | -|---|---| -| gosec | PASS — no medium+ findings introduced | -| govulncheck | PASS — no vulnerabilities | -| semgrep | NOT RUN — binary not installed in env; environment limitation, not a code defect | - -### Findings - -**[medium]** `room-service/handler.go:1242` + `room-service/helper.go:202` — Invalid-subject error string silently downgraded to `"internal error"` by `sanitizeError`. `handleMuteToggle` produces `fmt.Errorf("invalid mute-toggle subject: %s", subj)`. The `sanitizeError` safe-prefix list in `helper.go:202` includes `"invalid request"` and similar prefixes but NOT `"invalid mute-toggle"`. Result: NATS replies to clients carry `"internal error"` instead of the documented `"invalid mute-toggle subject: …"`. The unit tests at `handler_test.go:3340` call `handleMuteToggle` directly and never pass through `sanitizeError`, so the mismatch is not caught. `docs/client-api.md` explicitly documents `"invalid mute-toggle subject: …"` as the error clients should see — **the API contract is broken at delivery**. Fix: add `"invalid mute-toggle"` to the safe-prefix list, OR introduce an `errInvalidMuteToggleSubject` sentinel and wire it through. - -**[medium]** `room-service/handler.go:1280-1283` — `GetUserSiteID` failure silently returns `"ok"` and skips the cross-site outbox publish. If `GetUserSiteID` errors on a transient or permanent DB issue, the local toggle has already committed but the federation event is dropped. The client considers the RPC successful; the user's home site never receives the mute update and permanently disagrees with the room site. Inconsistent with the analogous `handleMessageRead` (`handler.go:1010`) which propagates the `GetUserSiteID` error. (Surfaced independently by 3 reviewers — room-service generalist, Go expert, and this lens.) - -**[low]** `room-service/handler.go:1253-1258` — Redundant `ErrSubscriptionNotFound` guard after the prior membership check at line 1245. `GetSubscription` already gatekeeps; the second check is unreachable in practice. Dead code that misleads readers about atomicity guarantees. - -**[nitpick]** `room-service/handler.go:1230` — `slog.Error("mute toggle failed", "error", err, "subject", m.Msg.Subject)` logs the raw NATS subject on every error, including malformed-subject errors. The subject encodes `account` and `roomID` (identifiers, not secrets); not a CLAUDE.md violation but worth noting that this handler logs an extra `subject` field that peer handlers do not. - -**[nitpick]** `inbox-worker/main.go:121` — `UpdateSubscriptionMute` discards `*UpdateResult` (assigns to `_`). Intentional per the design spec — missing-subscription is a silent no-op for federation races. Worth a one-line inline comment justifying the discard. - -### Specific trust assertions - -1. **Membership check before toggle** ✓ — `handleMuteToggle` calls `store.GetSubscription` at line 1245 before `ToggleSubscriptionMute`. A non-member cannot mutate state at the store layer. -2. **Mongo filter injection safety** ✓ — Filter `{"roomId": roomID, "u.account": account}` uses strings parsed by `subject.ParseUserRoomSubject` from the NATS subject. Subject tokens are dot-delimited and the parser rejects malformed inputs before reaching the store. No object-injection surface. -3. **Outbox payload trust** ✓ — `inbox-worker`'s `UpdateSubscriptionMute` receives `disableNotifications` from the outbox payload, which originates from `room-service`'s atomic Mongo write result (`ToggleSubscriptionMute` post-flip value) — not from client input. The trust assumption is that federated JetStream delivery from a peer site is trustworthy, consistent with the rest of `inbox-worker`. -4. **OutboxEvent assembly** ✓ — `Type`, `SiteID`, `DestSiteID`, `Payload`, `Timestamp` are all set in `handleMuteToggle` at lines 1294-1300. No fields missing. -5. **`sanitizeError` leak surface** ✓ — Internal Mongo error strings (e.g. `"db down"`) are caught by the catch-all `"internal error"` fallback at `helper.go:209`, but the same fallback ALSO catches the legitimate `"invalid mute-toggle subject"` string — see medium-1 above. - -### Verdict - -No security vulnerabilities, no race conditions, no injection surface. Two `medium` correctness bugs: the `sanitizeError` gap breaks the documented API contract on a single error path, and the `GetUserSiteID` silent-success creates reachable cross-site data divergence inconsistent with the rest of the codebase. Both should be fixed before merge. - ---- - -## Performance - -### Findings - -**[medium]** `room-service/handler.go:1245-1253` — Redundant Mongo round-trip before atomic write. `GetSubscription` runs before `ToggleSubscriptionMute` solely to (a) check membership and (b) supply the full `Subscription` struct for the downstream `SubscriptionUpdateEvent`. Both can be addressed without the extra round-trip: -- Membership check: `ToggleSubscriptionMute` already returns `ErrSubscriptionNotFound` on filter mismatch. -- Full struct for event: `sub.User.ID` is the only field beyond `disableNotifications` actually consumed. The atomic toggle could project the needed fields directly. - -This is the highest-priority performance issue since it doubles the per-request DB latency on every mute toggle. - -**[medium]** `inbox-worker/main.go:121-129` + `inbox-worker/main.go` `ensureIndexes` — `UpdateSubscriptionMute` filters on `{roomId, u.account}` but `inbox-worker`'s `ensureIndexes` (around `main.go:151`) only creates the `thread_subscriptions (threadRoomId, userId)` index. It does NOT create or assert the `(roomId, u.account)` index on `subscriptions`. The index is owned by whichever service first bootstraps the collection (typically `room-service` or `room-worker`), but `inbox-worker`'s `ensureIndexes` silently relies on it. If `inbox-worker` is deployed against a fresh collection (e.g. a new site in integration tests), the update degrades to a full collection scan. Either add the (idempotent) `CreateOne` call mirroring `room-service`'s `EnsureIndexes`, or document the cross-service index dependency. - -**[low]** `room-service/handler.go:1280` — `GetUserSiteID` runs strictly sequentially after the write and the `publishCore` call. It hits `users` (a different collection from `subscriptions`). It could overlap with `publishCore` (lines 1275-1278) but `publishCore` is non-fatal and core-NATS publish is very fast, so parallelisation adds complexity for ~µs gain. `users.account` index exists (`room-service/store_mongo.go:63-66`) — query is O(1). Not worth optimising. - -**[low]** `room-service/handler.go:1263` — `updatedSub := *sub` copies the entire `Subscription` struct just to mutate `DisableNotifications`. The copy is then embedded by value in `SubscriptionUpdateEvent`. If `Subscription` grows in the future, this becomes a per-request allocation hotspot. Consider holding the event sub by pointer or constructing it from individual fields. - -**[nitpick]** `room-service/handler.go:1307` — `publishToStream` (which calls `js.PublishMsg(ctx, ...)`) blocks waiting for JetStream PubAck with an unbounded context derived from `wrappedCtx(m)`. If the NATS server is overloaded, the handler goroutine blocks indefinitely. A `context.WithTimeout` of ~5s would bound the wait. Same pattern as the rest of the service (not introduced by this branch), but the new cross-site path makes it more reachable. - -### Index audit summary - -| Operation | Filter | Index | Status | -|---|---|---|---| -| `ToggleSubscriptionMute` (room-service) | `(roomId, u.account)` | unique compound at `store_mongo.go:57-61` | ✓ | -| `GetSubscription` (room-service) | `(roomId, u.account)` | same | ✓ | -| `GetUserSiteID` (room-service) | `account` | `users.account` at `store_mongo.go:63-66` | ✓ | -| `UpdateSubscriptionMute` (inbox-worker) | `(roomId, u.account)` | not declared in inbox-worker `ensureIndexes` | ✗ medium | - -### Verdict - -No critical issues. The new store operations hit covered indexes on the room-service side. The redundant `GetSubscription` round-trip (medium) doubles per-request DB latency on every mute toggle and could be eliminated by widening `ToggleSubscriptionMute`'s projection. The missing index assertion in `inbox-worker.ensureIndexes` is a medium latent risk. - ---- - -## Observability - -### Findings - -**[medium]** `room-service/handler.go:1239` — `handleMuteToggle` is missing span attributes. The closest structural analogue `handleMessageRead` also doesn't set them (so matching that precedent is defensible), but `handleMessageReadReceipt` (line 1105) and `publishCreateRoom` (line 349) DO set `room.id` / `site.id`. Adding `span.SetAttributes(attribute.String("room.id", roomID), attribute.String("site.id", h.siteID))` would cost two lines and improve trace correlation for a per-room operation. - -**[medium]** `room-service/handler.go:1282` — `slog.Warn("get user siteId failed; skipping outbox", "error", err, "account", account)`. The `Warn` level signals "degraded but OK", but `GetUserSiteID` returning an error means a real Mongo failure on `users` (not-found is `("", nil)`, not an error). The handler then silently suppresses federation. **Either**: (a) match `handleMessageRead`'s precedent and propagate the error (hard fail), OR (b) keep the soft-fail but escalate to `slog.Error` so operators know cross-site federation is silently dropping events. - -**[nitpick]** `room-service/handler.go:1230` — `natsMuteToggle` logs `slog.Error` with an extra `subject` structured field. The peer `natsMessageRead` (line 971) doesn't. The new field is an improvement for debuggability — propagate to peers in a follow-up, or accept the inconsistency. - -**[low]** No Prometheus metric on the new RPC path. The codebase doesn't add explicit metrics per-handler — OTel + slog is the observability mechanism. No action needed. - -### Discipline checks — all PASS - -1. **slog/JSON discipline**: all four new log calls use `slog` with structured key-value pairs. No `fmt.Println` / `log.Println` / text loggers. ✓ -2. **No secret leakage**: tokens/passwords/full message bodies not logged. `account` is a non-secret identifier; existing handlers log it identically (e.g. `handler.go:1029`). ✓ -3. **Request-ID propagation**: `natsMuteToggle` (line 1227) calls `ctx := wrappedCtx(m)` and passes that `ctx` to `handleMuteToggle`. Context flows through every store call and publish. ✓ -4. **Error log levels**: `slog.Error` for the publish-failure non-fatal path is appropriate (operators need to know NATS publish is failing even though the RPC succeeds). The `slog.Warn` for the `GetUserSiteID` path is the one questionable case — see medium-2 above. -5. **inbox-worker error handling**: `handleSubscriptionMuteToggled` returns errors to the consumer loop without per-handler slog, matching `handleSubscriptionRead`. The consumer loop in `main.go:261` logs all returned errors at `slog.Error` with `request_id`. Pattern is consistent. ✓ - -### Verdict - -PASS with two `medium` findings — both about the `GetUserSiteID` failure path (missing span attrs is a minor instrumentation gap; the Warn-vs-Error level is a meaningful operator-signal choice). Everything else — slog-only JSON, `wrappedCtx` propagation, no secret leakage, inbox-worker error pattern — is correct. - ---- - -## Prioritized action list - -Top items across all chapters, ordered by severity then impact-divided-by-effort. - -| # | Severity | Action | Location | Why | -|---|---|---|---|---| -| 1 | **high** | Eliminate the TOCTOU between `GetSubscription` and `ToggleSubscriptionMute`. Widen the atomic toggle's projection to include `u.id`/`u.account` and drop the pre-fetch — OR add a comment acknowledging the race. | `room-service/handler.go:1245-1264` | Doubles per-request DB latency; subscription state in the constructed `SubscriptionUpdateEvent` can be stale on a concurrent writer | -| 2 | **medium** | Add `"invalid mute-toggle"` to the `sanitizeError` safe-prefix list (or introduce an `errInvalidMuteToggleSubject` sentinel). | `room-service/helper.go:202` + `handler.go:1242` | API contract documented in this same PR is broken at delivery — clients get `"internal error"` instead of the documented message | -| 3 | **medium** | Resolve the `GetUserSiteID` failure asymmetry. Either propagate as hard error (match `handleMessageRead`) or escalate the log to `slog.Error` and add a doc note about the intentional degraded mode. | `room-service/handler.go:1280-1283` | Reachable cross-site data divergence on a transient DB hiccup; surfaced by 3 independent reviewers | -| 4 | **medium** | Update `SubscriptionUpdateEvent.Action` comment to include `"role_updated"` and `"mute_toggled"`. | `pkg/model/event.go:35` | Client-facing contract drift; comment is the API doc for that field | -| 5 | **medium** | Add the missing `(roomId, u.account)` index assertion to `inbox-worker`'s `ensureIndexes` (mirror room-service's `CreateOne` call). | `inbox-worker/main.go` `ensureIndexes` | Fresh-DB deployments degrade `UpdateSubscriptionMute` to a full collection scan | -| 6 | **medium** | Add test for cross-site `publishToStream` failure in `handleMuteToggle`. | `room-service/handler_test.go` | The hard-error path post-DB-write is currently untested; pattern exists in `TestHandler_MessageRead_CrossSite_PublishFailureAborts` | -| 7 | **medium** | Add test for `GetSubscription` generic error arm. | `room-service/handler_test.go` | Branch coverage for `handleMuteToggle` is ~70-75%; below the 90% target | -| 8 | **medium** | Add `span.SetAttributes(room.id, site.id)` to `handleMuteToggle`. | `room-service/handler.go:1239` | Two-line trace-correlation win | -| 9 | **medium** | Split `inbox-worker`'s inline `mongoInboxStore` from `main.go` into `store.go` + `store_mongo.go` (follow-up PR). | `inbox-worker/main.go:39-195` | Pre-existing tech debt this PR adds to; per-service layout convention | -| 10 | **low** | Remove the redundant `ErrSubscriptionNotFound` re-check after `ToggleSubscriptionMute`. | `room-service/handler.go:1253-1258` | Dead code that misleads about atomicity guarantees | - ---- - -**End of report.**