Skip to content

Commit 0fcc38c

Browse files
authored
Merge pull request #348 from SableClient/fix/sliding-sync-memory
fix: tighten sliding sync memory management
2 parents abe6b8d + fa4da19 commit 0fcc38c

6 files changed

Lines changed: 215 additions & 121 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
default: patch
3+
---
4+
5+
Tighten sliding sync memory management: stop the polling loop on client dispose, persist then prune large room timelines when leaving a room, remove adaptive timeline-limit logic, and auto-unsubscribe when the local user leaves or is banned from a room.

src/app/features/common-settings/developer-tools/DevelopTools.tsx

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -445,11 +445,8 @@ export function DeveloperTools({ requestClose }: DeveloperToolsProps) {
445445
Proxy: {syncDiagnostics.sliding.proxyBaseUrl}
446446
</Text>
447447
<Text size="T200">
448-
Room timeline: {syncDiagnostics.sliding.timelineLimit}
449-
{syncDiagnostics.sliding.adaptiveTimeline
450-
? ' (adaptive)'
451-
: ''}{' '}
452-
| page size: {syncDiagnostics.sliding.listPageSize}
448+
Room timeline: {syncDiagnostics.sliding.timelineLimit} | page
449+
size: {syncDiagnostics.sliding.listPageSize}
453450
</Text>
454451
</>
455452
) : (

src/app/features/settings/developer-tools/SyncDiagnostics.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,8 +199,7 @@ export function SyncDiagnostics() {
199199
<Box direction="Column" gap="100">
200200
<Text size="T300">Sliding proxy: {diagnostics.sliding.proxyBaseUrl}</Text>
201201
<Text size="T300">
202-
Room timeline limit: {diagnostics.sliding.timelineLimit} (adaptive:{' '}
203-
{diagnostics.sliding.adaptiveTimeline ? 'yes' : 'no'}) | page size:{' '}
202+
Room timeline limit: {diagnostics.sliding.timelineLimit} | page size:{' '}
204203
{diagnostics.sliding.listPageSize}
205204
</Text>
206205
{diagnostics.sliding.lists.map((list) => (

src/app/hooks/useSlidingSyncActiveRoom.ts

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ import { useSelectedRoom } from '$hooks/router/useSelectedRoom';
77
* Subscribes the currently selected room to the sliding sync "active room"
88
* custom subscription (higher timeline limit) for the duration the room is open.
99
*
10+
* Subscriptions are intentionally never removed on navigation — once a room
11+
* has been opened it continues receiving background updates so that returning
12+
* to it is instant. Explicit unsubscription (and timeline pruning) only happens
13+
* when the user actually leaves the room via `unsubscribeFromRoom()`.
14+
*
1015
* Safe to call unconditionally — it is a no-op when classic sync is in use
1116
* (i.e. when there is no SlidingSyncManager for the client).
1217
*/
@@ -20,23 +25,15 @@ export const useSlidingSyncActiveRoom = (): void => {
2025
if (!manager) return undefined;
2126

2227
// Wait for the room to be initialized from list sync before subscribing
23-
// with high timeline limit. This prevents timeline ordering issues where
28+
// with the full timeline limit. This prevents timeline ordering issues where
2429
// the room might be receiving events from list expansion while we're also
2530
// trying to load a large timeline, causing events to be added out of order.
2631
const timeoutId = setTimeout(() => {
27-
const room = mx.getRoom(roomId);
28-
if (room) {
29-
// Room exists and has been initialized from list sync
30-
manager.subscribeToRoom(roomId);
31-
} else {
32-
// Room not in cache yet - subscribe anyway (will use default encrypted subscription)
33-
manager.subscribeToRoom(roomId);
34-
}
32+
manager.subscribeToRoom(roomId);
3533
}, 100);
3634

3735
return () => {
3836
clearTimeout(timeoutId);
39-
manager.unsubscribeFromRoom(roomId);
4037
};
4138
}, [mx, roomId]);
4239
};

src/client/slidingSync.test.ts

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
/**
2+
* Unit tests for SlidingSyncManager memory management:
3+
*
4+
* 1. dispose() — must call slidingSync.stop() to halt the polling loop and
5+
* abort in-flight requests. Without this the SDK's Promise loop keeps
6+
* running after the client is "stopped", leaking network traffic and
7+
* event listeners.
8+
*
9+
* 2. onMembershipLeave — when the MatrixClient emits a RoomMemberEvent.Membership
10+
* event indicating the local user left or was banned from a room that is
11+
* actively subscribed, unsubscribeFromRoom() should be called automatically.
12+
*
13+
* Note: navigation between rooms does not call unsubscribeFromRoom —
14+
* subscriptions accumulate across the session so returning to a room is
15+
* instant (matching Element Web's model).
16+
*/
17+
import { describe, it, expect, vi, beforeEach } from 'vitest';
18+
import { SlidingSyncManager, type SlidingSyncConfig } from './slidingSync';
19+
20+
// ── vi.hoisted mocks ─────────────────────────────────────────────────────────
21+
// Must be defined via vi.hoisted so they're available before vi.mock runs
22+
// (vi.mock calls are hoisted above all imports by vitest's transformer).
23+
const mocks = vi.hoisted(() => ({
24+
slidingSyncInstance: {
25+
on: vi.fn(),
26+
off: vi.fn(),
27+
removeListener: vi.fn(),
28+
stop: vi.fn(),
29+
modifyRoomSubscriptions: vi.fn(),
30+
modifyRoomSubscriptionInfo: vi.fn(),
31+
addCustomSubscription: vi.fn(),
32+
useCustomSubscription: vi.fn(),
33+
registerExtension: vi.fn(),
34+
getListData: vi.fn(() => null),
35+
getListParams: vi.fn(() => null),
36+
setList: vi.fn(),
37+
setListRanges: vi.fn(),
38+
},
39+
}));
40+
41+
// ── Sentry stub ──────────────────────────────────────────────────────────────
42+
vi.mock('@sentry/react', () => ({
43+
metrics: { count: vi.fn(), gauge: vi.fn(), distribution: vi.fn() },
44+
addBreadcrumb: vi.fn(),
45+
startInactiveSpan: vi.fn(() => ({
46+
setAttribute: vi.fn(),
47+
setAttributes: vi.fn(),
48+
end: vi.fn(),
49+
})),
50+
startSpan: vi.fn(async (_opts: unknown, fn: (span: object) => unknown) =>
51+
fn({ setAttributes: vi.fn(), setAttribute: vi.fn(), end: vi.fn() })
52+
),
53+
}));
54+
55+
// ── SlidingSync SDK mock ─────────────────────────────────────────────────────
56+
// vi.fn() wrappers are arrow functions internally and cannot be called with `new`.
57+
// A plain function constructor (returning an object) is the correct pattern.
58+
vi.mock('$types/matrix-sdk', async (importOriginal) => {
59+
const actual = await importOriginal<Record<string, unknown>>();
60+
function MockSlidingSync() {
61+
return mocks.slidingSyncInstance;
62+
}
63+
return { ...actual, SlidingSync: MockSlidingSync };
64+
});
65+
66+
// ── Helpers ──────────────────────────────────────────────────────────────────
67+
68+
function makeMockMx(overrides: Record<string, unknown> = {}) {
69+
return {
70+
getUserId: vi.fn().mockReturnValue('@user:example.com'),
71+
getSafeUserId: vi.fn().mockReturnValue('@user:example.com'),
72+
isRoomEncrypted: vi.fn().mockReturnValue(false),
73+
getRoom: vi.fn().mockReturnValue(null),
74+
on: vi.fn(),
75+
off: vi.fn(),
76+
removeListener: vi.fn(),
77+
...overrides,
78+
} as unknown as import('$types/matrix-sdk').MatrixClient;
79+
}
80+
81+
function makeManager(mx: ReturnType<typeof makeMockMx>): SlidingSyncManager {
82+
const config: SlidingSyncConfig = {};
83+
return new SlidingSyncManager(mx, 'https://sliding.example.com', config);
84+
}
85+
86+
beforeEach(() => {
87+
vi.clearAllMocks();
88+
});
89+
90+
// ── dispose() ────────────────────────────────────────────────────────────────
91+
92+
describe('SlidingSyncManager.dispose()', () => {
93+
it('calls slidingSync.stop() to halt the polling loop', () => {
94+
const manager = makeManager(makeMockMx());
95+
manager.dispose();
96+
expect(mocks.slidingSyncInstance.stop).toHaveBeenCalledOnce();
97+
});
98+
});
99+
100+
// ── onMembershipLeave: auto-unsubscribe on leave/ban ─────────────────────────
101+
102+
describe('SlidingSyncManager — membership leave auto-unsubscribe', () => {
103+
/** Fire the RoomMemberEvent.Membership listener registered on mx.on */
104+
function fireMembershipEvent(
105+
mx: ReturnType<typeof makeMockMx>,
106+
membership: string,
107+
roomId = '!room:example.com',
108+
userId = '@user:example.com'
109+
) {
110+
const onCall = (mx.on as ReturnType<typeof vi.fn>).mock.calls.find(
111+
(args: unknown[]) => args[0] === 'RoomMember.membership'
112+
);
113+
if (!onCall) throw new Error('onMembershipLeave listener not registered');
114+
const [, handler] = onCall as [
115+
string,
116+
(e: unknown, m: { userId: string; roomId: string; membership: string }) => void,
117+
];
118+
handler(undefined, { userId, roomId, membership });
119+
}
120+
121+
it('unsubscribes when the local user leaves an active room', () => {
122+
const mx = makeMockMx();
123+
const manager = makeManager(mx);
124+
manager.attach();
125+
manager.subscribeToRoom('!room:example.com');
126+
127+
fireMembershipEvent(mx, 'leave');
128+
129+
// subscribeToRoom + unsubscribeFromRoom = 2 calls
130+
expect(mocks.slidingSyncInstance.modifyRoomSubscriptions).toHaveBeenCalledTimes(2);
131+
});
132+
133+
it('unsubscribes when the local user is banned from an active room', () => {
134+
const mx = makeMockMx();
135+
const manager = makeManager(mx);
136+
manager.attach();
137+
manager.subscribeToRoom('!room:example.com');
138+
139+
fireMembershipEvent(mx, 'ban');
140+
141+
expect(mocks.slidingSyncInstance.modifyRoomSubscriptions).toHaveBeenCalledTimes(2);
142+
});
143+
144+
it('does nothing when a different user leaves', () => {
145+
const mx = makeMockMx();
146+
const manager = makeManager(mx);
147+
manager.attach();
148+
manager.subscribeToRoom('!room:example.com');
149+
150+
fireMembershipEvent(mx, 'leave', '!room:example.com', '@other:example.com');
151+
152+
// Only the initial subscribe — no unsubscribe
153+
expect(mocks.slidingSyncInstance.modifyRoomSubscriptions).toHaveBeenCalledTimes(1);
154+
});
155+
156+
it('does nothing when membership is join', () => {
157+
const mx = makeMockMx();
158+
const manager = makeManager(mx);
159+
manager.attach();
160+
manager.subscribeToRoom('!room:example.com');
161+
162+
fireMembershipEvent(mx, 'join');
163+
164+
expect(mocks.slidingSyncInstance.modifyRoomSubscriptions).toHaveBeenCalledTimes(1);
165+
});
166+
167+
it('does nothing for a room that was never subscribed', () => {
168+
const mx = makeMockMx();
169+
const manager = makeManager(mx);
170+
manager.attach(); // registers the listener, but no subscribeToRoom call
171+
172+
fireMembershipEvent(mx, 'leave');
173+
174+
expect(mocks.slidingSyncInstance.modifyRoomSubscriptions).not.toHaveBeenCalled();
175+
});
176+
});

0 commit comments

Comments
 (0)