From cd262537a4c5895c3742b2535d6b5b53eba9e6b6 Mon Sep 17 00:00:00 2001 From: bir Date: Wed, 11 Dec 2024 15:08:33 +0100 Subject: [PATCH] Handle mail export throttling issue #8109 Co-authored-by: paw --- .../api/common/error/SuspensionError.ts | 11 +++++++++- .../facades/lazy/MailExportTokenFacade.ts | 3 ++- src/common/api/worker/rest/RestClient.ts | 7 +++++- .../native/main/MailExportController.ts | 22 ++++++++++++++----- src/mail-app/settings/MailExportSettings.ts | 2 +- .../facades/MailExportTokenFacadeTest.ts | 12 ++++++---- .../native/main/MailExportControllerTest.ts | 18 +++++++++++++++ 7 files changed, 62 insertions(+), 13 deletions(-) diff --git a/src/common/api/common/error/SuspensionError.ts b/src/common/api/common/error/SuspensionError.ts index bc2e84f3f0e7..aa7278588870 100644 --- a/src/common/api/common/error/SuspensionError.ts +++ b/src/common/api/common/error/SuspensionError.ts @@ -1,9 +1,18 @@ //@bundleInto:common-min import { TutanotaError } from "@tutao/tutanota-error" +import { filterInt } from "@tutao/tutanota-utils" export class SuspensionError extends TutanotaError { - constructor(message: string) { + // milliseconds to wait + readonly data: string | null + constructor(message: string, suspensionTime: string | null) { super("SuspensionError", message) + + if (suspensionTime != null && Number.isNaN(filterInt(suspensionTime))) { + throw new Error("invalid suspension time value (NaN)") + } + + this.data = suspensionTime } } diff --git a/src/common/api/worker/facades/lazy/MailExportTokenFacade.ts b/src/common/api/worker/facades/lazy/MailExportTokenFacade.ts index 39daed6ff5e2..06f8393d4fab 100644 --- a/src/common/api/worker/facades/lazy/MailExportTokenFacade.ts +++ b/src/common/api/worker/facades/lazy/MailExportTokenFacade.ts @@ -1,6 +1,7 @@ import { AccessExpiredError } from "../../../common/error/RestError.js" import { MailExportTokenService } from "../../../entities/tutanota/Services.js" import { IServiceExecutor } from "../../../common/ServiceRequest.js" +import { SuspensionBehavior } from "../../rest/RestClient" const TAG = "[MailExportTokenFacade]" @@ -65,7 +66,7 @@ export class MailExportTokenFacade { } this.currentExportToken = null - this.currentExportTokenRequest = this.serviceExecutor.post(MailExportTokenService, null).then( + this.currentExportTokenRequest = this.serviceExecutor.post(MailExportTokenService, null, { suspensionBehavior: SuspensionBehavior.Throw }).then( (result) => { this.currentExportToken = result.mailExportToken as MailExportToken this.currentExportTokenRequest = null diff --git a/src/common/api/worker/rest/RestClient.ts b/src/common/api/worker/rest/RestClient.ts index 3a336cb8c1ea..871a2485882d 100644 --- a/src/common/api/worker/rest/RestClient.ts +++ b/src/common/api/worker/rest/RestClient.ts @@ -127,7 +127,12 @@ export class RestClient { const suspensionTime = xhr.getResponseHeader("Retry-After") || xhr.getResponseHeader("Suspension-Time") if (isSuspensionResponse(xhr.status, suspensionTime) && options.suspensionBehavior === SuspensionBehavior.Throw) { - reject(new SuspensionError(`blocked for ${suspensionTime}, not suspending`)) + reject( + new SuspensionError( + `blocked for ${suspensionTime}, not suspending (${xhr.status})`, + suspensionTime && (parseInt(suspensionTime) * 1000).toString(), + ), + ) } else if (isSuspensionResponse(xhr.status, suspensionTime)) { this.suspensionHandler.activateSuspensionIfInactive(Number(suspensionTime), resourceURL) diff --git a/src/mail-app/native/main/MailExportController.ts b/src/mail-app/native/main/MailExportController.ts index 24ca6cef8d93..b9cfa0c2c2bf 100644 --- a/src/mail-app/native/main/MailExportController.ts +++ b/src/mail-app/native/main/MailExportController.ts @@ -3,7 +3,7 @@ import Stream from "mithril/stream" import stream from "mithril/stream" import { MailBag } from "../../../common/api/entities/tutanota/TypeRefs.js" import { GENERATED_MAX_ID, getElementId, isSameId } from "../../../common/api/common/utils/EntityUtils.js" -import { assertNotNull, delay, isNotNull, lastThrow } from "@tutao/tutanota-utils" +import { assertNotNull, delay, filterInt, isNotNull, lastThrow } from "@tutao/tutanota-utils" import { HtmlSanitizer } from "../../../common/misc/HtmlSanitizer.js" import { ExportFacade } from "../../../common/native/common/generatedipc/ExportFacade.js" import { LoginController } from "../../../common/api/main/LoginController.js" @@ -11,6 +11,8 @@ import { CancelledError } from "../../../common/api/common/error/CancelledError. import { FileOpenError } from "../../../common/api/common/error/FileOpenError.js" import { isOfflineError } from "../../../common/api/common/utils/ErrorUtils.js" import { MailExportFacade } from "../../../common/api/worker/facades/lazy/MailExportFacade.js" +import type { TranslationText } from "../../../common/misc/LanguageViewModel" +import { SuspensionError } from "../../../common/api/common/error/SuspensionError" import { Scheduler } from "../../../common/api/common/utils/Scheduler" import { ExportError, ExportErrorReason } from "../../../common/api/common/error/ExportError" @@ -18,7 +20,7 @@ export type MailExportState = | { type: "idle" } | { type: "exporting"; mailboxDetail: MailboxDetail; progress: number; exportedMails: number } | { type: "locked" } - | { type: "error"; message: string } + | { type: "error"; message: TranslationText } | { type: "finished" mailboxDetail: MailboxDetail @@ -98,6 +100,7 @@ export class MailExportController { progress: 0, exportedMails: exportState.exportedMails, }) + this._lastExport = new Date() await this.resumeExport(mailboxDetail, exportState.mailBagId, exportState.mailId) } else if (exportState.type === "finished") { const mailboxDetail = await this.mailboxModel.getMailboxDetailByMailboxId(exportState.mailboxId) @@ -138,9 +141,10 @@ export class MailExportController { } private async runExport(mailboxDetail: MailboxDetail, mailBags: MailBag[], mailId: Id) { + const startTime = assertNotNull(this._lastExport) for (const mailBag of mailBags) { await this.exportMailBag(mailBag, mailId) - if (this._state().type !== "exporting") { + if (this._state().type !== "exporting" || this._lastExport !== startTime) { return } } @@ -183,7 +187,7 @@ export class MailExportController { await this.exportFacade.saveMailboxExport(mailBundle, this.userId, mailBag._id, getElementId(mail)) } catch (e) { if (e instanceof FileOpenError) { - this._state({ type: "error", message: e.message }) + this._state({ type: "error", message: () => e.message }) return } else { throw e @@ -200,10 +204,18 @@ export class MailExportController { if (isOfflineError(e)) { console.log(TAG, "Offline, will retry later") await delay(1000 * 60) // 1 min - console.log(TAG, "Trying to continue with export") + } else if (e instanceof SuspensionError) { + const timeToWait = Math.max(filterInt(assertNotNull(e.data)), 1) + console.log(TAG, `Pausing for ${Math.floor(timeToWait / 1000 + 0.5)} seconds`) + const currentExportTime = this._lastExport + await delay(timeToWait) + if (this._state().type !== "exporting" || this._lastExport !== currentExportTime) { + return + } } else { throw e } + console.log(TAG, "Trying to continue with export") } } } diff --git a/src/mail-app/settings/MailExportSettings.ts b/src/mail-app/settings/MailExportSettings.ts index 5003bcc945e5..c5a9aad633f0 100644 --- a/src/mail-app/settings/MailExportSettings.ts +++ b/src/mail-app/settings/MailExportSettings.ts @@ -114,7 +114,7 @@ export class MailExportSettings implements Component { case "error": return [ m(".flex-space-between.items-center.mt.mb-s", [ - m("small.noselect", state.message), + m("small.noselect", lang.getMaybeLazy(state.message)), m(Button, { label: "retry_action", click: () => { diff --git a/test/tests/api/worker/facades/MailExportTokenFacadeTest.ts b/test/tests/api/worker/facades/MailExportTokenFacadeTest.ts index d2c73a355cfd..ba110a9321b7 100644 --- a/test/tests/api/worker/facades/MailExportTokenFacadeTest.ts +++ b/test/tests/api/worker/facades/MailExportTokenFacadeTest.ts @@ -1,5 +1,5 @@ import o from "@tutao/otest" -import { func, object, when } from "testdouble" +import { func, matchers, object, when } from "testdouble" import { createMailExportTokenServicePostOut } from "../../../../../src/common/api/entities/tutanota/TypeRefs" import { MailExportTokenService } from "../../../../../src/common/api/entities/tutanota/Services" import { AccessExpiredError, TooManyRequestsError } from "../../../../../src/common/api/common/error/RestError" @@ -23,7 +23,9 @@ o.spec("MailExportTokenFacade", () => { const expected = "result" const cb = func<(token: string) => Promise>() when(cb(validToken)).thenResolve(expected) - when(serviceExecutor.post(MailExportTokenService, null)).thenResolve(createMailExportTokenServicePostOut({ mailExportToken: validToken })) + when(serviceExecutor.post(MailExportTokenService, null, matchers.anything())).thenResolve( + createMailExportTokenServicePostOut({ mailExportToken: validToken }), + ) const result = await facade.loadWithToken(cb) @@ -47,7 +49,9 @@ o.spec("MailExportTokenFacade", () => { when(cb(validToken)).thenResolve(expected) when(cb(expiredToken)).thenReject(new AccessExpiredError("token expired")) facade._setCurrentExportToken(expiredToken) - when(serviceExecutor.post(MailExportTokenService, null)).thenResolve(createMailExportTokenServicePostOut({ mailExportToken: validToken })) + when(serviceExecutor.post(MailExportTokenService, null, matchers.anything())).thenResolve( + createMailExportTokenServicePostOut({ mailExportToken: validToken }), + ) const result = await facade.loadWithToken(cb) @@ -57,7 +61,7 @@ o.spec("MailExportTokenFacade", () => { o.test("when requesting token fails none are stored", async () => { const cb = func<(token: string) => Promise>() when(cb(expiredToken)).thenReject(new AccessExpiredError("token expired")) - when(serviceExecutor.post(MailExportTokenService, null)).thenReject(new TooManyRequestsError("no more tokens :(")) + when(serviceExecutor.post(MailExportTokenService, null, matchers.anything())).thenReject(new TooManyRequestsError("no more tokens :(")) await o(() => facade.loadWithToken(cb)).asyncThrows(TooManyRequestsError) diff --git a/test/tests/native/main/MailExportControllerTest.ts b/test/tests/native/main/MailExportControllerTest.ts index 8051b4f3cbb2..9c92831ad0e8 100644 --- a/test/tests/native/main/MailExportControllerTest.ts +++ b/test/tests/native/main/MailExportControllerTest.ts @@ -26,6 +26,7 @@ import { createDataFile } from "../../../../src/common/api/common/DataFile.js" import { makeMailBundle } from "../../../../src/mail-app/mail/export/Bundler.js" import { MailboxExportState } from "../../../../src/common/desktop/export/MailboxExportPersistence.js" import { MailExportFacade } from "../../../../src/common/api/worker/facades/lazy/MailExportFacade.js" +import { SuspensionError } from "../../../../src/common/api/common/error/SuspensionError" import { spy } from "@tutao/tutanota-test-utils" import { ExportError, ExportErrorReason } from "../../../../src/common/api/common/error/ExportError" @@ -204,4 +205,21 @@ o.spec("MailExportController", function () { verify(exportFacade.endMailboxExport(userId)) }) }) + + o.spec("handle errors", function () { + o.test("SuspensionError", async () => { + let wasThrown = false + when(mailExportFacade.loadFixedNumberOfMailsWithCache(matchers.anything(), matchers.anything())).thenDo(() => { + if (wasThrown) { + return Promise.resolve([]) + } else { + wasThrown = true + return Promise.reject(new SuspensionError(":(", "10")) + } + }) + await controller.startExport(mailboxDetail) + verify(mailExportFacade.loadFixedNumberOfMailsWithCache(matchers.anything(), matchers.anything()), { times: 3 + 1 }) + o(wasThrown).equals(true) + }) + }) })