fix crash that occurs when adding users in icqlite

ICQlite crashes when adding users with authorization disabled. I
determined that the crashes occurred when presence notifations are
sent in the middle of feedbag transactions. When I moved to the
presence notifications post-transactions, the crash resolved.

So in this commit, we accumulate lists of users to notify during
the transaction, and then notify those users after the transaction
ends.

foodgroup/feedbag.go

@@ -350,8 +350,14 @@ func (s *FeedbagService) UpsertItem(ctx context.Context, instance *state.Session
 	}
 
 	if alertAll || len(filter) > 0 {
-		if err := s.buddyBroadcaster.BroadcastVisibility(ctx, instance, filter, true); err != nil {
-			return nil, err
+		if instance.InNotifyTxn() {
+			if err := instance.NotifyTxn(filter...); err != nil {
+				return nil, fmt.Errorf("NotifyTxn: %w", err)
+			}
+		} else {
+			if err := s.buddyBroadcaster.BroadcastVisibility(ctx, instance, filter, true); err != nil {
+				return nil, err
+			}
 		}
 	}
 
@@ -477,8 +483,14 @@ func (s *FeedbagService) DeleteItem(ctx context.Context, instance *state.Session
 		}
 	}
 
-	if err := s.buddyBroadcaster.BroadcastVisibility(ctx, instance, filter, true); err != nil {
-		return nil, err
+	if instance.InNotifyTxn() {
+		if err := instance.NotifyTxn(filter...); err != nil {
+			return nil, fmt.Errorf("NotifyTxn: %w", err)
+		}
+	} else {
+		if err := s.buddyBroadcaster.BroadcastVisibility(ctx, instance, filter, true); err != nil {
+			return nil, err
+		}
 	}
 
 	return nil, nil
@@ -486,8 +498,10 @@ func (s *FeedbagService) DeleteItem(ctx context.Context, instance *state.Session
 
 // StartCluster signals the beginning of a batch of feedbag operations that clients should
 // process together to prevent UI flicker during rapid updates. It transmits the start message
-// to other session instances.
+// to other session instances. It starts a new notification transaction, which records users
+// that should receive presence notifications when the transaction ends.
 func (s *FeedbagService) StartCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame, inBody wire.SNAC_0x13_0x11_FeedbagStartCluster) {
+	instance.BeginNotifyTxn()
 	s.messageRelayer.RelayToOtherInstances(ctx, instance, wire.SNACMessage{
 		Frame: inFrame,
 		Body:  inBody,
@@ -495,12 +509,23 @@ func (s *FeedbagService) StartCluster(ctx context.Context, instance *state.Sessi
 }
 
 // EndCluster signals the completion of a batched feedbag operation group. It transmits the end
-// message to other session instances.
-func (s *FeedbagService) EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) {
+// message to other session instances. It broadcast presence notifications if any relevant
+// changes accumulated during the transaction.
+func (s *FeedbagService) EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) error {
 	s.messageRelayer.RelayToOtherInstances(ctx, instance, wire.SNACMessage{
 		Frame: inFrame,
 		Body:  wire.SNAC_0x13_0x12_FeedbagEndCluster{},
 	})
+
+	shouldNotify, filter := instance.EndNotifyTxn()
+	if !shouldNotify {
+		return nil
+	}
+
+	if err := s.buddyBroadcaster.BroadcastVisibility(ctx, instance, filter, true); err != nil {
+		return err
+	}
+	return nil
 }
 
 // Use activates server-side buddy list state for feedbag clients at sign-on.

foodgroup/feedbag_test.go

@@ -496,6 +496,76 @@ func TestFeedbagService_UpsertItem(t *testing.T) {
 			},
 			expectOutput: nil,
 		},
+		{
+			name: "defer visibility during notify txn",
+			instance: func() *state.SessionInstance {
+				inst := newTestInstance("me")
+				inst.BeginNotifyTxn()
+				return inst
+			}(),
+			inputSNAC: wire.SNACMessage{
+				Frame: wire.SNACFrame{
+					FoodGroup: wire.Feedbag,
+					SubGroup:  wire.FeedbagInsertItem,
+					RequestID: 1234,
+				},
+				Body: wire.SNAC_0x13_0x08_FeedbagInsertItem{
+					Items: []wire.FeedbagItem{
+						{ClassID: wire.FeedbagClassIDPermit, Name: "buddy1"},
+					},
+				},
+			},
+			mockParams: mockParams{
+				feedbagManagerParams: feedbagManagerParams{
+					feedbagUpsertParams: feedbagUpsertParams{
+						{
+							screenName: state.NewIdentScreenName("me"),
+							items: []wire.FeedbagItem{
+								{ClassID: wire.FeedbagClassIDPermit, Name: "buddy1"},
+							},
+						},
+					},
+				},
+				messageRelayerParams: messageRelayerParams{
+					relayToOtherInstancesParams: relayToOtherInstancesParams{
+						{
+							screenName: state.NewIdentScreenName("me"),
+							message: wire.SNACMessage{
+								Frame: wire.SNACFrame{
+									FoodGroup: wire.Feedbag,
+									SubGroup:  wire.FeedbagInsertItem,
+									RequestID: wire.ReqIDFromServer,
+								},
+								Body: wire.SNAC_0x13_0x09_FeedbagUpdateItem{
+									Items: []wire.FeedbagItem{
+										{ClassID: wire.FeedbagClassIDPermit, Name: "buddy1"},
+									},
+								},
+							},
+						},
+					},
+					relayToSelfParams: relayToSelfParams{
+						{
+							screenName: state.NewIdentScreenName("me"),
+							message: wire.SNACMessage{
+								Frame: wire.SNACFrame{
+									FoodGroup: wire.Feedbag,
+									SubGroup:  wire.FeedbagStatus,
+									RequestID: 1234,
+								},
+								Body: wire.SNAC_0x13_0x0E_FeedbagStatus{
+									Results: []uint16{0x0000},
+								},
+							},
+						},
+					},
+				},
+			},
+			instanceMatch: func(instance *state.SessionInstance) {
+				assert.True(t, instance.InNotifyTxn())
+			},
+			expectOutput: nil,
+		},
 		{
 			name:     "add 2 ICQ buddies one that requires authorization",
 			instance: newTestInstance("100001", sessOptUIN(100001)),
@@ -2447,6 +2517,8 @@ func TestFeedbagService_DeleteItem(t *testing.T) {
 		mockParams mockParams
 		// expectOutput is the SNAC sent from the server to client
 		expectOutput *wire.SNACMessage
+		// checkInstance validates instance state after the call
+		checkInstance func(*testing.T, *state.SessionInstance)
 	}{
 		{
 			name:     "delete buddies",
@@ -2555,6 +2627,76 @@ func TestFeedbagService_DeleteItem(t *testing.T) {
 			},
 			expectOutput: nil,
 		},
+		{
+			name: "defer visibility during notify txn",
+			instance: func() *state.SessionInstance {
+				inst := newTestInstance("me")
+				inst.BeginNotifyTxn()
+				return inst
+			}(),
+			inputSNAC: wire.SNACMessage{
+				Frame: wire.SNACFrame{
+					FoodGroup: wire.Feedbag,
+					SubGroup:  wire.FeedbagDeleteItem,
+					RequestID: 1234,
+				},
+				Body: wire.SNAC_0x13_0x0A_FeedbagDeleteItem{
+					Items: []wire.FeedbagItem{
+						{ClassID: wire.FeedbagClassIdBuddy, Name: "buddy1"},
+					},
+				},
+			},
+			mockParams: mockParams{
+				feedbagManagerParams: feedbagManagerParams{
+					feedbagDeleteParams: feedbagDeleteParams{
+						{
+							screenName: state.NewIdentScreenName("me"),
+							items: []wire.FeedbagItem{
+								{ClassID: wire.FeedbagClassIdBuddy, Name: "buddy1"},
+							},
+						},
+					},
+				},
+				messageRelayerParams: messageRelayerParams{
+					relayToOtherInstancesParams: relayToOtherInstancesParams{
+						{
+							screenName: state.NewIdentScreenName("me"),
+							message: wire.SNACMessage{
+								Frame: wire.SNACFrame{
+									FoodGroup: wire.Feedbag,
+									SubGroup:  wire.FeedbagDeleteItem,
+									RequestID: wire.ReqIDFromServer,
+								},
+								Body: wire.SNAC_0x13_0x0A_FeedbagDeleteItem{
+									Items: []wire.FeedbagItem{
+										{ClassID: wire.FeedbagClassIdBuddy, Name: "buddy1"},
+									},
+								},
+							},
+						},
+					},
+					relayToSelfParams: relayToSelfParams{
+						{
+							screenName: state.NewIdentScreenName("me"),
+							message: wire.SNACMessage{
+								Frame: wire.SNACFrame{
+									FoodGroup: wire.Feedbag,
+									SubGroup:  wire.FeedbagStatus,
+									RequestID: 1234,
+								},
+								Body: wire.SNAC_0x13_0x0E_FeedbagStatus{
+									Results: []uint16{0x0000},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectOutput: nil,
+			checkInstance: func(t *testing.T, instance *state.SessionInstance) {
+				assert.True(t, instance.InNotifyTxn())
+			},
+		},
 	}
 
 	for _, tc := range cases {
@@ -2590,6 +2732,10 @@ func TestFeedbagService_DeleteItem(t *testing.T) {
 				tc.inputSNAC.Body.(wire.SNAC_0x13_0x0A_FeedbagDeleteItem))
 			assert.NoError(t, err)
 			assert.Equal(t, output, tc.expectOutput)
+
+			if tc.checkInstance != nil {
+				tc.checkInstance(t, tc.instance)
+			}
 		})
 	}
 }
@@ -4170,25 +4316,227 @@ func TestFeedbagService_StartCluster(t *testing.T) {
 
 	svc := NewFeedbagService(slog.Default(), messageRelayer, nil, nil, nil, nil, nil, nil)
 	svc.StartCluster(context.Background(), instance, inFrame, inBody)
+
+	assert.True(t, instance.InNotifyTxn())
 }
 
 func TestFeedbagService_EndCluster(t *testing.T) {
-	instance := newTestInstance("me")
-	inFrame := wire.SNACFrame{
+	endFrame := wire.SNACFrame{
 		FoodGroup: wire.Feedbag,
 		SubGroup:  wire.FeedbagEndCluster,
 		RequestID: 1234,
 	}
+	endRelay := wire.SNACMessage{
+		Frame: endFrame,
+		Body:  wire.SNAC_0x13_0x12_FeedbagEndCluster{},
+	}
 
-	messageRelayer := newMockMessageRelayer(t)
-	messageRelayer.EXPECT().
-		RelayToOtherInstances(matchContext(), instance, wire.SNACMessage{
-			Frame: inFrame,
-			Body:  wire.SNAC_0x13_0x12_FeedbagEndCluster{},
+	cases := []struct {
+		name                      string
+		instance                  *state.SessionInstance
+		broadcastVisibilityParams broadcastVisibilityParams
+	}{
+		{
+			name: "open txn without accumulated notify skips broadcast",
+			instance: func() *state.SessionInstance {
+				inst := newTestInstance("me")
+				inst.BeginNotifyTxn()
+				return inst
+			}(),
+		},
+		{
+			name: "broadcasts accumulated screen names",
+			instance: func() *state.SessionInstance {
+				inst := newTestInstance("me")
+				inst.BeginNotifyTxn()
+				_ = inst.NotifyTxn(state.NewIdentScreenName("buddy1"))
+				return inst
+			}(),
+			broadcastVisibilityParams: broadcastVisibilityParams{
+				{
+					from: state.NewIdentScreenName("me"),
+					filter: []state.IdentScreenName{
+						state.NewIdentScreenName("buddy1"),
+					},
+				},
+			},
+		},
+		{
+			name: "broadcasts after notify without name filter",
+			instance: func() *state.SessionInstance {
+				inst := newTestInstance("me")
+				inst.BeginNotifyTxn()
+				_ = inst.NotifyTxn()
+				return inst
+			}(),
+			broadcastVisibilityParams: broadcastVisibilityParams{
+				{
+					from:   state.NewIdentScreenName("me"),
+					filter: nil,
+				},
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			messageRelayer := newMockMessageRelayer(t)
+			messageRelayer.EXPECT().
+				RelayToOtherInstances(matchContext(), tc.instance, endRelay)
+
+			buddyBroadcaster := newMockbuddyBroadcaster(t)
+			for _, params := range tc.broadcastVisibilityParams {
+				buddyBroadcaster.EXPECT().
+					BroadcastVisibility(matchContext(), matchSession(params.from), params.filter, true).
+					Return(params.err)
+			}
+
+			svc := NewFeedbagService(slog.Default(), messageRelayer, nil, nil, nil, nil, nil, nil)
+			svc.buddyBroadcaster = buddyBroadcaster
+			err := svc.EndCluster(context.Background(), tc.instance, endFrame)
+			assert.NoError(t, err)
+			assert.False(t, tc.instance.InNotifyTxn())
 		})
+	}
+}
 
-	svc := NewFeedbagService(slog.Default(), messageRelayer, nil, nil, nil, nil, nil, nil)
-	svc.EndCluster(context.Background(), instance, inFrame)
+func TestFeedbagService_notifyTxnCluster(t *testing.T) {
+	me := state.NewIdentScreenName("me")
+	buddy1 := state.NewIdentScreenName("buddy1")
+
+	startFrame := wire.SNACFrame{
+		FoodGroup: wire.Feedbag,
+		SubGroup:  wire.FeedbagStartCluster,
+		RequestID: 1,
+	}
+	startBody := wire.SNAC_0x13_0x11_FeedbagStartCluster{}
+	endFrame := wire.SNACFrame{
+		FoodGroup: wire.Feedbag,
+		SubGroup:  wire.FeedbagEndCluster,
+		RequestID: 2,
+	}
+	upsertFrame := wire.SNACFrame{
+		FoodGroup: wire.Feedbag,
+		SubGroup:  wire.FeedbagInsertItem,
+		RequestID: 3,
+	}
+	upsertItems := []wire.FeedbagItem{
+		{ClassID: wire.FeedbagClassIDPermit, Name: "buddy1"},
+	}
+
+	t.Run("upsert defers visibility until end cluster", func(t *testing.T) {
+		instance := newTestInstance("me")
+
+		messageRelayer := newMockMessageRelayer(t)
+		messageRelayer.EXPECT().
+			RelayToOtherInstances(matchContext(), instance, mock.Anything).
+			Times(3)
+		messageRelayer.EXPECT().
+			RelayToSelf(matchContext(), instance, mock.Anything).
+			Once()
+
+		feedbagManager := newMockFeedbagManager(t)
+		feedbagManager.EXPECT().
+			FeedbagUpsert(matchContext(), me, upsertItems).
+			Return(nil)
+
+		buddyBroadcaster := newMockbuddyBroadcaster(t)
+		buddyBroadcaster.EXPECT().
+			BroadcastVisibility(matchContext(), matchSession(me), []state.IdentScreenName{buddy1}, true).
+			Return(nil).
+			Once()
+
+		svc := NewFeedbagService(slog.Default(), messageRelayer, feedbagManager, nil, nil, nil, nil, nil)
+		svc.buddyBroadcaster = buddyBroadcaster
+
+		svc.StartCluster(context.Background(), instance, startFrame, startBody)
+		assert.True(t, instance.InNotifyTxn())
+
+		_, err := svc.UpsertItem(context.Background(), instance, upsertFrame, upsertItems)
+		assert.NoError(t, err)
+		assert.True(t, instance.InNotifyTxn())
+
+		err = svc.EndCluster(context.Background(), instance, endFrame)
+		assert.NoError(t, err)
+		assert.False(t, instance.InNotifyTxn())
+	})
+
+	t.Run("delete defers visibility until end cluster", func(t *testing.T) {
+		instance := newTestInstance("me")
+		deleteBody := wire.SNAC_0x13_0x0A_FeedbagDeleteItem{
+			Items: []wire.FeedbagItem{
+				{ClassID: wire.FeedbagClassIdBuddy, Name: "buddy1"},
+			},
+		}
+
+		messageRelayer := newMockMessageRelayer(t)
+		messageRelayer.EXPECT().
+			RelayToOtherInstances(matchContext(), instance, mock.Anything).
+			Times(3)
+		messageRelayer.EXPECT().
+			RelayToSelf(matchContext(), instance, mock.Anything).
+			Once()
+
+		feedbagManager := newMockFeedbagManager(t)
+		feedbagManager.EXPECT().
+			FeedbagDelete(matchContext(), me, deleteBody.Items).
+			Return(nil)
+
+		buddyBroadcaster := newMockbuddyBroadcaster(t)
+		buddyBroadcaster.EXPECT().
+			BroadcastVisibility(matchContext(), matchSession(me), []state.IdentScreenName{buddy1}, true).
+			Return(nil).
+			Once()
+
+		svc := NewFeedbagService(slog.Default(), messageRelayer, feedbagManager, nil, nil, nil, nil, nil)
+		svc.buddyBroadcaster = buddyBroadcaster
+
+		svc.StartCluster(context.Background(), instance, startFrame, startBody)
+		_, err := svc.DeleteItem(context.Background(), instance, upsertFrame, deleteBody)
+		assert.NoError(t, err)
+
+		err = svc.EndCluster(context.Background(), instance, endFrame)
+		assert.NoError(t, err)
+	})
+
+	t.Run("pdinfo upsert defers broadcast all until end cluster", func(t *testing.T) {
+		instance := newTestInstance("me")
+		pdinfoItems := []wire.FeedbagItem{{ClassID: wire.FeedbagClassIdPdinfo}}
+
+		messageRelayer := newMockMessageRelayer(t)
+		messageRelayer.EXPECT().
+			RelayToOtherInstances(matchContext(), instance, mock.Anything).
+			Times(3)
+		messageRelayer.EXPECT().
+			RelayToSelf(matchContext(), instance, mock.Anything).
+			Once()
+
+		feedbagManager := newMockFeedbagManager(t)
+		feedbagManager.EXPECT().
+			FeedbagUpsert(matchContext(), me, pdinfoItems).
+			Return(nil)
+
+		buddyBroadcaster := newMockbuddyBroadcaster(t)
+		buddyBroadcaster.EXPECT().
+			BroadcastVisibility(
+				matchContext(),
+				matchSession(me),
+				mock.MatchedBy(func(filter []state.IdentScreenName) bool { return len(filter) == 0 }),
+				true,
+			).
+			Return(nil).
+			Once()
+
+		svc := NewFeedbagService(slog.Default(), messageRelayer, feedbagManager, nil, nil, nil, nil, nil)
+		svc.buddyBroadcaster = buddyBroadcaster
+
+		svc.StartCluster(context.Background(), instance, startFrame, startBody)
+		_, err := svc.UpsertItem(context.Background(), instance, upsertFrame, pdinfoItems)
+		assert.NoError(t, err)
+
+		err = svc.EndCluster(context.Background(), instance, endFrame)
+		assert.NoError(t, err)
+	})
 }
 
 func TestFeedbagService_ForwardICQAuthEvents(t *testing.T) {

server/oscar/handler.go

@@ -384,7 +384,9 @@ func (rt Handler) FeedbagStartCluster(ctx context.Context, instance *state.Sessi
 }
 
 func (rt Handler) FeedbagEndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame, r io.Reader, rw ResponseWriter) error {
-	rt.EndCluster(ctx, instance, inFrame)
+	if err := rt.EndCluster(ctx, instance, inFrame); err != nil {
+		return err
+	}
 	rt.LogRequest(ctx, inFrame, nil)
 	return nil
 }

server/oscar/handler_test.go

@@ -1617,7 +1617,8 @@ func TestHandler_FeedbagEndCluster(t *testing.T) {
 
 			svc := newMockFeedbagService(t)
 			svc.EXPECT().
-				EndCluster(mock.Anything, mock.Anything, input.Frame)
+				EndCluster(mock.Anything, mock.Anything, input.Frame).
+				Return(nil)
 
 			h := Handler{
 				FeedbagService: svc,

server/oscar/mock_feedbag_service_test.go

@@ -120,9 +120,20 @@ func (_c *mockFeedbagService_DeleteItem_Call) RunAndReturn(run func(ctx context.
 }
 
 // EndCluster provides a mock function for the type mockFeedbagService
-func (_mock *mockFeedbagService) EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) {
-	_mock.Called(ctx, instance, inFrame)
-	return
+func (_mock *mockFeedbagService) EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) error {
+	ret := _mock.Called(ctx, instance, inFrame)
+
+	if len(ret) == 0 {
+		panic("no return value specified for EndCluster")
+	}
+
+	var r0 error
+	if returnFunc, ok := ret.Get(0).(func(context.Context, *state.SessionInstance, wire.SNACFrame) error); ok {
+		r0 = returnFunc(ctx, instance, inFrame)
+	} else {
+		r0 = ret.Error(0)
+	}
+	return r0
 }
 
 // mockFeedbagService_EndCluster_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'EndCluster'
@@ -161,13 +172,13 @@ func (_c *mockFeedbagService_EndCluster_Call) Run(run func(ctx context.Context,
 	return _c
 }
 
-func (_c *mockFeedbagService_EndCluster_Call) Return() *mockFeedbagService_EndCluster_Call {
-	_c.Call.Return()
+func (_c *mockFeedbagService_EndCluster_Call) Return(err error) *mockFeedbagService_EndCluster_Call {
+	_c.Call.Return(err)
 	return _c
 }
 
-func (_c *mockFeedbagService_EndCluster_Call) RunAndReturn(run func(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame)) *mockFeedbagService_EndCluster_Call {
-	_c.Run(run)
+func (_c *mockFeedbagService_EndCluster_Call) RunAndReturn(run func(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) error) *mockFeedbagService_EndCluster_Call {
+	_c.Call.Return(run)
 	return _c
 }
 

server/oscar/types.go

@@ -98,7 +98,7 @@ type FeedbagService interface {
 	RespondAuthorizeToHost(ctx context.Context, instance state.IdentScreenName, inFrame wire.SNACFrame, inBody wire.SNAC_0x13_0x1A_FeedbagRespondAuthorizeToHost) error
 	RightsQuery(ctx context.Context, inFrame wire.SNACFrame) wire.SNACMessage
 	StartCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame, inBody wire.SNAC_0x13_0x11_FeedbagStartCluster)
-	EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame)
+	EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) error
 	UpsertItem(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame, items []wire.FeedbagItem) (*wire.SNACMessage, error)
 	Use(ctx context.Context, instance *state.SessionInstance) error
 }

server/toc/mock_feedbag_service_test.go

@@ -120,9 +120,20 @@ func (_c *mockFeedbagService_DeleteItem_Call) RunAndReturn(run func(ctx context.
 }
 
 // EndCluster provides a mock function for the type mockFeedbagService
-func (_mock *mockFeedbagService) EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) {
-	_mock.Called(ctx, instance, inFrame)
-	return
+func (_mock *mockFeedbagService) EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) error {
+	ret := _mock.Called(ctx, instance, inFrame)
+
+	if len(ret) == 0 {
+		panic("no return value specified for EndCluster")
+	}
+
+	var r0 error
+	if returnFunc, ok := ret.Get(0).(func(context.Context, *state.SessionInstance, wire.SNACFrame) error); ok {
+		r0 = returnFunc(ctx, instance, inFrame)
+	} else {
+		r0 = ret.Error(0)
+	}
+	return r0
 }
 
 // mockFeedbagService_EndCluster_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'EndCluster'
@@ -161,13 +172,13 @@ func (_c *mockFeedbagService_EndCluster_Call) Run(run func(ctx context.Context,
 	return _c
 }
 
-func (_c *mockFeedbagService_EndCluster_Call) Return() *mockFeedbagService_EndCluster_Call {
-	_c.Call.Return()
+func (_c *mockFeedbagService_EndCluster_Call) Return(err error) *mockFeedbagService_EndCluster_Call {
+	_c.Call.Return(err)
 	return _c
 }
 
-func (_c *mockFeedbagService_EndCluster_Call) RunAndReturn(run func(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame)) *mockFeedbagService_EndCluster_Call {
-	_c.Run(run)
+func (_c *mockFeedbagService_EndCluster_Call) RunAndReturn(run func(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) error) *mockFeedbagService_EndCluster_Call {
+	_c.Call.Return(run)
 	return _c
 }
 

server/toc/types.go

@@ -106,7 +106,7 @@ type FeedbagService interface {
 	RespondAuthorizeToHost(ctx context.Context, instance state.IdentScreenName, inFrame wire.SNACFrame, inBody wire.SNAC_0x13_0x1A_FeedbagRespondAuthorizeToHost) error
 	RightsQuery(ctx context.Context, inFrame wire.SNACFrame) wire.SNACMessage
 	StartCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame, inBody wire.SNAC_0x13_0x11_FeedbagStartCluster)
-	EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame)
+	EndCluster(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame) error
 	UpsertItem(ctx context.Context, instance *state.SessionInstance, inFrame wire.SNACFrame, items []wire.FeedbagItem) (*wire.SNACMessage, error)
 	Use(ctx context.Context, instance *state.SessionInstance) error
 }

state/session.go

@@ -1,6 +1,8 @@
 package state
 
 import (
+	"errors"
+	"fmt"
 	"net/netip"
 	"slices"
 	"sync"
@@ -47,6 +49,15 @@ const (
 	SessQueueFull
 )
 
+// maxNotifyTxnNames is the maximum number of distinct screen names accumulated in
+// a single notify transaction.
+const maxNotifyTxnNames = 1000
+
+var (
+	errNotifyTxnNotActive    = errors.New("notify transaction is not active")
+	errNotifyTxnTooManyNames = errors.New("notify transaction exceeds maximum screen names")
+)
+
 // Session represents shared user-level state that persists across all concurrent
 // connections for a single user account.
 //
@@ -904,6 +915,11 @@ type SessionInstance struct {
 	// contactsInit indicates whether the client-side buddy list or feedbag has been initialized
 	contactsInit bool
 
+	// notify transaction: accumulate visibility notification targets during feedbag batches
+	notifyTxnActive     bool
+	notifyTxnShouldSend bool
+	notifyTxnNames      map[IdentScreenName]struct{}
+
 	// Per-session profile
 	profile           UserProfile
 	awayTime          time.Time
@@ -1155,6 +1171,72 @@ func (s *SessionInstance) SetContactsInit() {
 	s.contactsInit = true
 }
 
+// BeginNotifyTxn starts accumulating notification targets during feedbag transactions.
+func (s *SessionInstance) BeginNotifyTxn() {
+	s.mutex.Lock()
+	defer s.mutex.Unlock()
+	s.notifyTxnShouldSend = false
+	s.notifyTxnNames = nil
+	s.notifyTxnActive = true
+}
+
+// InNotifyTxn reports whether a notification transaction is open.
+func (s *SessionInstance) InNotifyTxn() bool {
+	s.mutex.RLock()
+	defer s.mutex.RUnlock()
+	return s.notifyTxnActive
+}
+
+// NotifyTxn records that a visibility notification should be sent when the transaction
+// ends, optionally limiting it to the given ident screen names. If no names are provided,
+// all buddies will be notified.
+func (s *SessionInstance) NotifyTxn(names ...IdentScreenName) error {
+	s.mutex.Lock()
+	defer s.mutex.Unlock()
+
+	if !s.notifyTxnActive {
+		return fmt.Errorf("notify transaction: %w", errNotifyTxnNotActive)
+	}
+	if len(s.notifyTxnNames)+len(names) > maxNotifyTxnNames {
+		return fmt.Errorf("notify transaction: %w", errNotifyTxnTooManyNames)
+	}
+
+	s.notifyTxnShouldSend = true
+	if s.notifyTxnNames == nil {
+		s.notifyTxnNames = make(map[IdentScreenName]struct{})
+	}
+	for _, name := range names {
+		s.notifyTxnNames[name] = struct{}{}
+	}
+
+	return nil
+}
+
+// EndNotifyTxn returns whether to send a notification and accumulated notification targets,
+// then clears transaction state.
+func (s *SessionInstance) EndNotifyTxn() (shouldNotify bool, screenNames []IdentScreenName) {
+	s.mutex.Lock()
+	defer func() {
+		s.notifyTxnActive = false
+		s.notifyTxnShouldSend = false
+		s.notifyTxnNames = nil
+		s.mutex.Unlock()
+	}()
+
+	if !s.notifyTxnActive {
+		return false, nil
+	}
+
+	shouldNotify = s.notifyTxnShouldSend
+	if len(s.notifyTxnNames) > 0 {
+		screenNames = make([]IdentScreenName, 0, len(s.notifyTxnNames))
+		for name := range s.notifyTxnNames {
+			screenNames = append(screenNames, name)
+		}
+	}
+	return shouldNotify, screenNames
+}
+
 // CloseInstance shuts down the instance's ability to relay messages.
 func (s *SessionInstance) closeOnly() {
 	s.mutex.Lock()

state/session_test.go

@@ -2,6 +2,7 @@ package state
 
 import (
 	"context"
+	"fmt"
 	"math"
 	"net/netip"
 	"sync"
@@ -11,6 +12,7 @@ import (
 	"github.com/mk6i/open-oscar-server/wire"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSession_UsesFeedbag(t *testing.T) {
@@ -33,6 +35,87 @@ func TestSession_UsesFeedbag(t *testing.T) {
 	}
 }
 
+func TestSessionInstance_NotifyTxn(t *testing.T) {
+	alice := NewIdentScreenName("Alice")
+	bob := NewIdentScreenName("Bob")
+
+	t.Run("lifecycle", func(t *testing.T) {
+		inst := NewSession().AddInstance()
+		inst.BeginNotifyTxn()
+		assert.True(t, inst.InNotifyTxn())
+
+		require.NoError(t, inst.NotifyTxn(alice, bob))
+		shouldNotify, screenNames := inst.EndNotifyTxn()
+		assert.True(t, shouldNotify)
+		assert.ElementsMatch(t, []IdentScreenName{alice, bob}, screenNames)
+		assert.False(t, inst.InNotifyTxn())
+
+		shouldNotify, screenNames = inst.EndNotifyTxn()
+		assert.False(t, shouldNotify)
+		assert.Nil(t, screenNames)
+	})
+
+	t.Run("clear on begin", func(t *testing.T) {
+		inst := NewSession().AddInstance()
+		inst.BeginNotifyTxn()
+		require.NoError(t, inst.NotifyTxn(alice))
+		inst.BeginNotifyTxn()
+
+		shouldNotify, screenNames := inst.EndNotifyTxn()
+		assert.False(t, shouldNotify)
+		assert.Empty(t, screenNames)
+	})
+
+	t.Run("notify without names", func(t *testing.T) {
+		inst := NewSession().AddInstance()
+		inst.BeginNotifyTxn()
+		require.NoError(t, inst.NotifyTxn())
+
+		shouldNotify, screenNames := inst.EndNotifyTxn()
+		assert.True(t, shouldNotify)
+		assert.Empty(t, screenNames)
+	})
+
+	t.Run("inactive notify", func(t *testing.T) {
+		inst := NewSession().AddInstance()
+		assert.ErrorIs(t, inst.NotifyTxn(alice), errNotifyTxnNotActive)
+
+		inst.BeginNotifyTxn()
+		shouldNotify, screenNames := inst.EndNotifyTxn()
+		assert.False(t, shouldNotify)
+		assert.Empty(t, screenNames)
+	})
+
+	t.Run("dedup", func(t *testing.T) {
+		inst := NewSession().AddInstance()
+		inst.BeginNotifyTxn()
+		require.NoError(t, inst.NotifyTxn(alice, alice))
+
+		shouldNotify, screenNames := inst.EndNotifyTxn()
+		assert.True(t, shouldNotify)
+		assert.Equal(t, []IdentScreenName{alice}, screenNames)
+	})
+
+	t.Run("exceeds max names", func(t *testing.T) {
+		inst := NewSession().AddInstance()
+		inst.BeginNotifyTxn()
+
+		names := make([]IdentScreenName, maxNotifyTxnNames)
+		for i := range names {
+			names[i] = NewIdentScreenName(fmt.Sprintf("user%d", i))
+		}
+		require.NoError(t, inst.NotifyTxn(names...))
+
+		shouldNotify, screenNames := inst.EndNotifyTxn()
+		assert.True(t, shouldNotify)
+		assert.Len(t, screenNames, maxNotifyTxnNames)
+
+		inst.BeginNotifyTxn()
+		require.NoError(t, inst.NotifyTxn(names...))
+		assert.ErrorIs(t, inst.NotifyTxn(NewIdentScreenName("overflow")), errNotifyTxnTooManyNames)
+	})
+}
+
 func TestSession_IncrementAndGetWarning(t *testing.T) {
 	s := NewSession().AddInstance()