From 20be11053751b7d2095c19cf7250faf0f37c0a73 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 02:00:50 +0100 Subject: [PATCH] fix: keep the device sync push types when a subscription write fails for another reason --- src/lib/__tests__/push-notifications.test.ts | 29 +++++++++++++++ src/lib/push-notifications.ts | 37 +++++++++++++++----- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/src/lib/__tests__/push-notifications.test.ts b/src/lib/__tests__/push-notifications.test.ts index 3b9125d9..1869c02d 100644 --- a/src/lib/__tests__/push-notifications.test.ts +++ b/src/lib/__tests__/push-notifications.test.ts @@ -514,6 +514,35 @@ describe('push types for device sync (#34)', () => { expect(destroyMock).not.toHaveBeenCalled(); }); + it("retries with the mail types alone when Stalwart refuses the types property", async () => { + await syncing({ [CONTACTS]: true }); + // Stalwart's own answer to a type it doesn't know. + createMock.mockRejectedValueOnce(new JMAPSetError('invalidProperties', 'Invalid data type.', ['types'])); + await setupPushNotifications({ relayBaseUrl: RELAY }); + expect(createMock.mock.calls.map((c) => c[0].types)) + .toEqual([['EmailDelivery', 'ContactCard', 'AddressBook'], ['EmailDelivery']]); + }); + + it('does not drop the device sync types over a failure that is not about them', async () => { + await syncing({ [CONTACTS]: true }); + for (const failure of [ + new Error('Network request failed'), + new JMAPSetError('invalidProperties', 'Host is not a public domain name', ['url']), + new JMAPSetError('overQuota', 'There are too many subscriptions'), + ]) { + createMock.mockClear(); + createMock.mockRejectedValueOnce(failure); + const err = await setupPushNotifications({ relayBaseUrl: RELAY }).catch((e) => e); + expect(err).toBeInstanceOf(PushSetupError); + expect(createMock.mock.calls.map((c) => c[0].types)).toEqual([['EmailDelivery', 'ContactCard', 'AddressBook']]); + } + + // The next attempt (the next launch) subscribes with them. + createMock.mockClear(); + await setupPushNotifications({ relayBaseUrl: RELAY }); + expect(createMock.mock.calls.map((c) => c[0].types)).toEqual([['EmailDelivery', 'ContactCard', 'AddressBook']]); + }); + it('subscribes with them', async () => { await syncing({ [CALENDAR]: true }); await setupPushNotifications({ relayBaseUrl: RELAY }); diff --git a/src/lib/push-notifications.ts b/src/lib/push-notifications.ts index 14e2d21a..de331774 100644 --- a/src/lib/push-notifications.ts +++ b/src/lib/push-notifications.ts @@ -275,7 +275,8 @@ const EMAIL_PUSH_PROPERTIES = ['id', 'threadId']; // user re-enables it from settings. const SUBSCRIPTION_EXPIRES_DAYS = 90; // When an existing subscription has less than this much lifetime left, push -// expires forward on the next app start. +// expires forward on the next app start. Stalwart (0.16 and 1.0) clamps every +// expiry to seven days, so there each start refreshes it. const SUBSCRIPTION_REFRESH_THRESHOLD_DAYS = 7; // A recorded subscription missing from the server was revoked only if it still // had at least this long to live; closer to its expiry (or with the clock a @@ -508,10 +509,25 @@ async function writeWithEmailPush( } } +/** + * Whether a refused subscription write may be about its types. A failed + * request says nothing about them, and a SetError naming other properties + * (the push URL, the emailPush map) or meaning the subscription limit or a + * missing record would fail the same way with fewer types; retrying those + * with the mail types alone would only drop device sync's on a write that + * later goes through. + */ +function mayConcernTypes(err: unknown): boolean { + if (!(err instanceof JMAPMethodError)) return false; + if (!isJMAPSetError(err)) return true; + if (err.type !== 'invalidProperties') return false; + return !err.properties || err.properties.includes('types'); +} + /** * Run a subscription write with the account's push types and, when the server - * refuses it while they include device sync's, once more with the mail types - * alone: mail push never depends on device sync (#34). + * refuses it over them while they include device sync's, once more with the + * mail types alone: mail push never depends on device sync (#34). */ async function writeWithPushTypes( types: readonly string[], @@ -520,7 +536,7 @@ async function writeWithPushTypes( try { return await write(types); } catch (err) { - if (sameTypes(types, PUSH_TYPES)) throw err; + if (sameTypes(types, PUSH_TYPES) || !mayConcernTypes(err)) throw err; logPhase('jmap', 'push types refused, retrying with the mail types alone'); return write(PUSH_TYPES); } @@ -1301,10 +1317,12 @@ async function setupPushNotificationsInner( } // Reap leftover Stalwart subscriptions that would otherwise starve the new - // one's verification. Stalwart emits only one PushVerification per account - // per ~60s and picks the oldest unverified subscription, so a single stale - // straggler blocks every fresh attempt - the symptom is the confusing - // "Timed out waiting for PushVerification" error. We can't read a + // one's verification. Stalwart (0.16 and 1.0) emits only one + // PushVerification per account per 60 s, to the newest unverified + // subscription, and sends the next one when that window has passed: every + // straggler created or touched just before ours delays it by a window, and + // a swarm of them ends in the confusing "Timed out waiting for + // PushVerification" error. We can't read a // subscription's verified state or URL over JMAP (Stalwart hides both), only // its deviceClientId, so we decide what's safe to remove like this: // - same deviceClientId as ours: a previous attempt from THIS device, @@ -1663,7 +1681,8 @@ export async function teardownPushNotifications(): Promise { if (storedSubId) { // Will only succeed if the jmapClient happens to be authenticated to // this account right now. We don't switch the client to attempt each - // one; the subscription will expire server-side instead (90-day TTL). + // one; the subscription expires server-side instead (Stalwart keeps + // one for at most seven days without a refresh). await destroyPushSubscription(storedSubId).catch(() => undefined); } if (relayBaseUrl && storedDcid) {