Skip to content

Commit cb16d97

Browse files
7w17w1
authored
fix(persona): serialize per-message-profile account data writes (#1563)
<!-- Please read https://github.com/SableClient/Sable/blob/dev/CONTRIBUTING.md before submitting your pull request --> ### Description the per-message-profile setters read account data, changed it, wrote it back, and never awaited the write. two at once and you lose an association. both go through a per-key queue now so writes to the same key can't overlap. Fixes # #### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings ### AI disclosure: - [ ] Partially AI assisted (clarify which code was AI assisted and briefly explain what it does). - [ ] Fully AI generated (explain what all the generated code does in moderate detail). <!-- Write any explanation required here, but do not generate the explanation using AI!! You must prove you understand what the code in this PR does. -->
2 parents c4cc35c + a466ba1 commit cb16d97

3 files changed

Lines changed: 125 additions & 37 deletions

File tree

‎src/app/hooks/usePerMessageProfile.proxy.test.ts‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { describe, expect, it } from 'vitest';
1+
import type { MatrixClient } from '$types/matrix-sdk';
2+
import { describe, expect, it, vi } from 'vitest';
23

34
import {
45
extractCircumfixProxyTagsFromKey,
@@ -8,6 +9,8 @@ import {
89
type PerMessageProfileProxyAssociationV1,
910
proxyNeedsMigration,
1011
createProxyKey,
12+
setCurrentlyUsedPerMessageProfileIdForAccount,
13+
setCurrentlyUsedPerMessageProfileIdForRoom,
1114
} from './usePerMessageProfile';
1215

1316
describe('migratePerMessageProfileProxyAssociation', () => {
@@ -132,3 +135,60 @@ describe('parsePerMessageProfileProxyAssociation', () => {
132135
expect(parsed.regex.test('[no] trailing')).toBe(false);
133136
});
134137
});
138+
139+
describe('per-message profile persistence', () => {
140+
it('serializes room writes and reads the latest account data snapshot', async () => {
141+
const associations: Record<string, { profileId: string }> = {};
142+
const writes: unknown[] = [];
143+
let releaseFirstWrite!: () => void;
144+
const firstWrite = new Promise<void>((resolve) => {
145+
releaseFirstWrite = resolve;
146+
});
147+
148+
const mx = {
149+
getAccountData: vi.fn<() => { getContent: () => { associations: typeof associations } }>(
150+
() => ({ getContent: () => ({ associations }) })
151+
),
152+
setAccountData: vi.fn<
153+
(_event: unknown, content: { associations: typeof associations }) => Promise<void>
154+
>(async (_event, content) => {
155+
writes.push(content);
156+
if (writes.length === 1) await firstWrite;
157+
Object.assign(associations, content.associations);
158+
}),
159+
} as unknown as MatrixClient;
160+
161+
const first = setCurrentlyUsedPerMessageProfileIdForRoom(mx, '!room:example.org', 'first');
162+
const second = setCurrentlyUsedPerMessageProfileIdForRoom(mx, '!room:example.org', 'second');
163+
164+
await vi.waitFor(() => expect(writes).toHaveLength(1));
165+
166+
releaseFirstWrite();
167+
await Promise.all([first, second]);
168+
169+
expect(writes).toHaveLength(2);
170+
expect((writes[1] as { associations: typeof associations }).associations).toEqual({
171+
'!room:example.org': { profileId: 'second' },
172+
});
173+
});
174+
175+
it('continues queued account writes after a rejected write', async () => {
176+
const writes: string[] = [];
177+
const mx = {
178+
setAccountData: vi.fn<
179+
(_event: unknown, content: { association: { profileId: string } }) => Promise<void>
180+
>(async (_event, content) => {
181+
writes.push(content.association.profileId);
182+
if (writes.length === 1) throw new Error('write failed');
183+
}),
184+
deleteAccountData: vi.fn<(...args: unknown[]) => void>(),
185+
} as unknown as MatrixClient;
186+
187+
const first = setCurrentlyUsedPerMessageProfileIdForAccount(mx, 'first');
188+
const second = setCurrentlyUsedPerMessageProfileIdForAccount(mx, 'second');
189+
190+
await expect(first).rejects.toThrow('write failed');
191+
await expect(second).resolves.toBeUndefined();
192+
expect(writes).toEqual(['first', 'second']);
193+
});
194+
});

‎src/app/hooks/usePerMessageProfile.ts‎

Lines changed: 44 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,20 @@ import type { PronounSet } from '$utils/pronouns';
44
import type { MatrixClient } from '$types/matrix-sdk';
55
import { CustomAccountDataEvent } from '$types/matrix/accountData';
66
import type { ColorSet } from './useUserProfile';
7-
import { MATRIX_UNSTABLE_COLORS } from '$unstable/prefixes';
87
import {
8+
MATRIX_UNSTABLE_COLORS,
99
MATRIX_SABLE_UNSTABLE_MSC4461_TRIGGER_CIRCUMFIX_PROPERTY_NAME,
1010
MATRIX_SABLE_UNSTABLE_MSC4461_TRIGGER_SUFFIX_PROPERTY_NAME,
1111
MATRIX_UNSTABLE_MSC4461_ACCOUNT_PER_MESSAGE_PROFILES_PROPERTY_NAME,
1212
MATRIX_UNSTABLE_PROFILE_PRONOUNS_PROPERTY_NAME,
1313
} from '$unstable/prefixes';
14+
import { createKeyedQueue } from '$utils/keyedQueue';
1415

1516
const ACCOUNT_DATA_PREFIX = CustomAccountDataEvent.SablePerProfileMessageProfiles;
1617

18+
/** Account data is read-modify-written, so writes to the same key must not interleave. */
19+
const enqueueProfilePersistence = createKeyedQueue();
20+
1721
/**
1822
* @deprecated in favour if {@link PerMessageProfileMsc4461}
1923
* a per message profile
@@ -622,32 +626,34 @@ export async function setCurrentlyUsedPerMessageProfileIdForRoom(
622626
validUntil?: number,
623627
reset?: boolean
624628
) {
625-
const accountData = mx.getAccountData(
626-
`${ACCOUNT_DATA_PREFIX}.roomassociation` as Parameters<typeof mx.getAccountData>[0]
627-
);
628-
const content: PerMessageProfileRoomAssociationWrapper | undefined = accountData?.getContent();
629-
const associations = getAssociationsMap(content);
630-
631-
if (reset) {
632-
associations.delete(roomId);
629+
return enqueueProfilePersistence('roomassociation', async () => {
630+
const accountData = mx.getAccountData(
631+
`${ACCOUNT_DATA_PREFIX}.roomassociation` as Parameters<typeof mx.getAccountData>[0]
632+
);
633+
const content: PerMessageProfileRoomAssociationWrapper | undefined = accountData?.getContent();
634+
const associations = getAssociationsMap(content);
635+
636+
if (reset) {
637+
associations.delete(roomId);
638+
await mx.setAccountData(
639+
`${ACCOUNT_DATA_PREFIX}.roomassociation` as Parameters<typeof mx.setAccountData>[0],
640+
{ associations: associationsMapToObject(associations) } as Parameters<
641+
typeof mx.setAccountData
642+
>[1]
643+
);
644+
return;
645+
}
646+
if (!profileId) {
647+
throw new Error("profile Id is empty, yet it isn't a reset");
648+
}
649+
associations.set(roomId, { profileId, validUntil });
633650
await mx.setAccountData(
634651
`${ACCOUNT_DATA_PREFIX}.roomassociation` as Parameters<typeof mx.setAccountData>[0],
635652
{ associations: associationsMapToObject(associations) } as Parameters<
636653
typeof mx.setAccountData
637654
>[1]
638655
);
639-
return;
640-
}
641-
if (!profileId) {
642-
throw new Error("profile Id is empty, yet it isn't a reset");
643-
}
644-
associations.set(roomId, { profileId, validUntil });
645-
await mx.setAccountData(
646-
`${ACCOUNT_DATA_PREFIX}.roomassociation` as Parameters<typeof mx.setAccountData>[0],
647-
{ associations: associationsMapToObject(associations) } as Parameters<
648-
typeof mx.setAccountData
649-
>[1]
650-
);
656+
});
651657
}
652658

653659
/**
@@ -659,22 +665,24 @@ export async function setCurrentlyUsedPerMessageProfileIdForAccount(
659665
validUntil?: number,
660666
reset?: boolean
661667
) {
662-
if (reset) {
663-
await mx.deleteAccountData(
664-
`${ACCOUNT_DATA_PREFIX}.globalassociation` as Parameters<typeof mx.setAccountData>[0]
665-
);
666-
return;
667-
}
668-
if (!profileId) {
669-
throw new Error("profile Id is empty, yet it isn't a reset");
670-
}
668+
return enqueueProfilePersistence('globalassociation', async () => {
669+
if (reset) {
670+
await mx.deleteAccountData(
671+
`${ACCOUNT_DATA_PREFIX}.globalassociation` as Parameters<typeof mx.setAccountData>[0]
672+
);
673+
return;
674+
}
675+
if (!profileId) {
676+
throw new Error("profile Id is empty, yet it isn't a reset");
677+
}
678+
679+
const association: PerMessageProfileRoomAssociation = { profileId, validUntil };
671680

672-
const association: PerMessageProfileRoomAssociation = { profileId, validUntil };
673-
674-
await mx.setAccountData(
675-
`${ACCOUNT_DATA_PREFIX}.globalassociation` as Parameters<typeof mx.setAccountData>[0],
676-
{ association: association } as Parameters<typeof mx.setAccountData>[1]
677-
);
681+
await mx.setAccountData(
682+
`${ACCOUNT_DATA_PREFIX}.globalassociation` as Parameters<typeof mx.setAccountData>[0],
683+
{ association: association } as Parameters<typeof mx.setAccountData>[1]
684+
);
685+
});
678686
}
679687

680688
/*

‎src/app/utils/keyedQueue.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/** Serializes operations per key so read-modify-write sequences cannot interleave. */
2+
export function createKeyedQueue() {
3+
const tails = new Map<string, Promise<void>>();
4+
5+
return function run<T>(key: string, operation: () => T | PromiseLike<T>): Promise<T> {
6+
const previous = tails.get(key) ?? Promise.resolve();
7+
const current = previous.catch(() => undefined).then(operation);
8+
const tail = current.then(
9+
() => undefined,
10+
() => undefined
11+
);
12+
13+
tails.set(key, tail);
14+
void tail.then(() => {
15+
if (tails.get(key) === tail) tails.delete(key);
16+
});
17+
18+
return current;
19+
};
20+
}

0 commit comments

Comments
 (0)