diff --git a/src/lib/__tests__/push-background-task.test.ts b/src/lib/__tests__/push-background-task.test.ts index f2cc64a1..7f9e384b 100644 --- a/src/lib/__tests__/push-background-task.test.ts +++ b/src/lib/__tests__/push-background-task.test.ts @@ -544,11 +544,33 @@ describe('pushBackgroundTask keeps the push subscription alive', () => { expect(server.calls).toContain('PushSubscription/set'); }); - it('leaves alone a subscription with time left, one that lapsed, one of unknown expiry and one push was turned off for', async () => { + it('asks the server when the recorded expiry has passed or is unknown, which may be stale', async () => { for (const setup of [ - () => recorded(6.5 * DAY), () => recorded(-DAY), () => AsyncStorage.removeItem(`push:subscriptionExpires:v1:${LOCAL}`), + ]) { + // The server's copy runs out in two days: extended. + Object.assign(server, { expires: new Date(Date.now() + 2 * DAY).toISOString(), calls: [] }); + await AsyncStorage.multiRemove([`push:notifiedIds:v1:${LOCAL}`, `push:lastNotifiedEmailId:v2:${LOCAL}`]); + await setup(); + await push(); + expect(server.calls).toEqual(['Email/get', 'PushSubscription/get', 'PushSubscription/set', 'PushSubscription/get']); + expect(await AsyncStorage.getItem(`push:subscriptionExpires:v1:${LOCAL}`)).toBe(server.expires); + + // The server's copy has a week left: recorded, not written. + const serverExpires = new Date(Date.now() + 6.5 * DAY).toISOString(); + Object.assign(server, { expires: serverExpires, calls: [] }); + await AsyncStorage.multiRemove([`push:notifiedIds:v1:${LOCAL}`, `push:lastNotifiedEmailId:v2:${LOCAL}`]); + await setup(); + await push(); + expect(server.calls).toEqual(['Email/get', 'PushSubscription/get']); + expect(await AsyncStorage.getItem(`push:subscriptionExpires:v1:${LOCAL}`)).toBe(serverExpires); + } + }); + + it('leaves alone a subscription with time left and one push was turned off for', async () => { + for (const setup of [ + () => recorded(6.5 * DAY), async () => { await recorded(2 * DAY); await AsyncStorage.setItem(`push:optedOut:v1:${LOCAL}`, '1'); diff --git a/src/lib/__tests__/push-notifications.test.ts b/src/lib/__tests__/push-notifications.test.ts index 54acc715..917ea8ee 100644 --- a/src/lib/__tests__/push-notifications.test.ts +++ b/src/lib/__tests__/push-notifications.test.ts @@ -1439,6 +1439,19 @@ describe('resyncPushNotifications', () => { expect(await AsyncStorage.getItem(EXPIRES_KEY)).toBe(expires); }); + it('records the expiry the server settled on after a refresh, not the one before it', async () => { + const before = inDays(1); + const after = inDays(7); + await AsyncStorage.setItem(SUB_KEY, 'existing'); + listMock.mockResolvedValueOnce([{ id: 'existing', deviceClientId: OUR_DCID, expires: before, types: ['EmailDelivery'] }]); + listMock.mockResolvedValueOnce([{ id: 'existing', deviceClientId: OUR_DCID, expires: after, types: ['EmailDelivery'] }]); + + await resyncPushNotifications({ relayBaseUrl: RELAY }); + + expect(updateMock).toHaveBeenCalledTimes(1); + expect(await AsyncStorage.getItem(EXPIRES_KEY)).toBe(after); + }); + it('leaves an account alone after the user turned push off for it', async () => { await AsyncStorage.setItem(SUB_KEY, 'existing'); await AsyncStorage.setItem('push:accountIds:v1', JSON.stringify([ACCOUNT_ID])); diff --git a/src/lib/push-background-task.ts b/src/lib/push-background-task.ts index 026b608d..985a76e0 100644 --- a/src/lib/push-background-task.ts +++ b/src/lib/push-background-task.ts @@ -18,6 +18,7 @@ import { migrateLegacyPushKeys, notifiedIdsKey, pushSubscriptionDueForExtension, + pushSubscriptionNeedsExtension, readPushAccountIds, readPushJmapAccountIds, recordPushSubscriptionExpires, @@ -481,6 +482,11 @@ async function extendPushSubscription(accountId: string, session: DetachedSessio '0', ); if (!current || (current.deviceClientId && due.deviceClientId && current.deviceClientId !== due.deviceClientId)) return; + // The recorded expiry was stale: the server's copy has time left. + if (current.expires && !pushSubscriptionNeedsExtension(current.expires)) { + await recordPushSubscriptionExpires(accountId, current.expires); + return; + } const responses = await jmapPost(session, [ ['PushSubscription/set', { update: { [due.id]: { expires: wantedPushSubscriptionExpires() } } }, '0'], // The update's answer doesn't say what the server settled on. diff --git a/src/lib/push-notifications.ts b/src/lib/push-notifications.ts index e0758ffa..0e024f1b 100644 --- a/src/lib/push-notifications.ts +++ b/src/lib/push-notifications.ts @@ -1331,6 +1331,11 @@ async function setupPushNotificationsInner( throw subscriptionError(err, relayBaseUrl); } if (refreshed) { + // The update's answer doesn't say what the server settled on, and + // the expiry recorded above is from before it. Left there, it would + // read as lapsed days before the server's copy is, which turns the + // background extension off for an account without new mail. + if (refreshed.updated) await recordServerExpiry(accountId, match.id, deviceClientId); await rememberRefusedEmailPushAccounts(accountId, refusedBefore, emailPush, refreshed.emailPush); await addPushAccountId(accountId); await writePushJmapAccountId(accountId, jmapAccountId); @@ -1470,9 +1475,9 @@ async function addPushAccountId(accountId: string): Promise { // update with a SetError, also with the mail types alone (the subscription is // gone, or no narrower map is accepted), which the caller answers with a // replacement; otherwise the emailPush map the update installed (null when it -// left the filter alone). Every other failure - the network, a timeout, rate -// limiting, a refused method call - says nothing about the subscription and -// is thrown. +// left the filter alone) and whether it wrote anything. Every other failure - +// the network, a timeout, rate limiting, a refused method call - says nothing +// about the subscription and is thrown. async function refreshSubscriptionExpires( sub: { id: string; @@ -1483,7 +1488,7 @@ async function refreshSubscriptionExpires( // null when the server has no emailPush support - leave the property alone. desiredEmailPush: Record | null, types: readonly string[], -): Promise | null }> { +): Promise | null; updated: boolean }> { const typesNeedUpdate = !sameTypes(sub.types, types); const emailPushNeedsUpdate = desiredEmailPush !== null && !sameEmailPush(sub.emailPush, desiredEmailPush); @@ -1492,7 +1497,7 @@ async function refreshSubscriptionExpires( const thresholdMs = SUBSCRIPTION_REFRESH_THRESHOLD_DAYS * 24 * 60 * 60 * 1000; if (Number.isFinite(remainingMs) && remainingMs > thresholdMs) { // Plenty of life left - skip the update round-trip. - return { emailPush: null }; + return { emailPush: null, updated: false }; } } try { @@ -1505,7 +1510,7 @@ async function refreshSubscriptionExpires( emailPushNeedsUpdate ? desiredEmailPush : null, (filter) => updatePushSubscription(sub.id, filter ? { ...patch, emailPush: filter } : patch), ); - return { emailPush }; + return { emailPush, updated: true }; }); } catch (err) { if (isJMAPSetError(err)) return false; @@ -1513,6 +1518,22 @@ async function refreshSubscriptionExpires( } } +/** + * Record the expiry the server holds for the account's subscription, read + * back after an update. Best effort: without an answer the recorded one + * stays, and the background extension asks the server itself. + */ +async function recordServerExpiry(accountId: string, subscriptionId: string, deviceClientId: string): Promise { + try { + const current = (await listPushSubscriptions()).find( + (s) => s.id === subscriptionId && (!s.deviceClientId || s.deviceClientId === deviceClientId), + ); + if (current?.expires) await AsyncStorage.setItem(subscriptionExpiresKey(accountId), current.expires); + } catch { + // The next run reads it again. + } +} + async function deregisterFromRelay( relayBaseUrl: string, deviceClientId: string, @@ -1573,12 +1594,21 @@ export async function forgetPushSubscriptionExpiry(accountId: string): Promise= BACKGROUND_EXTEND_BELOW_MS) return null; + if (!pushSubscriptionNeedsExtension(await AsyncStorage.getItem(subscriptionExpiresKey(accountId)))) return null; return { id, deviceClientId: await AsyncStorage.getItem(deviceClientIdKey(accountId)) }; }