diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java index 4d5a76bbdd..939f9308a0 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java @@ -274,6 +274,16 @@ class FailureKindTest { .contains(FailureKind.INPUT_PASSWORD_PROTECTED); } + @Test + void everyCodeAKindClaimsIsPinned() { + // The bell mirrors these in KIND_ERROR_CODES (notificationRetry.ts) to tell one file's + // stashed failure from another's. Adding a code here without adding it there makes a + // retry match the wrong incident, so this fails until both move together. + assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.getErrorCodes()) + .containsExactly("E004"); + assertThat(FailureKind.UNKNOWN.getErrorCodes()).isEmpty(); + } + @Test void byErrorCodeIsEmptyForACodeNoKindHasAdoptedYet() { // E001 is PDF_CORRUPTED: a real error code, deliberately not yet a kind. diff --git a/frontend/editor/src/core/contexts/FileContext.tsx b/frontend/editor/src/core/contexts/FileContext.tsx index 08f867f7b6..6e03a860e8 100644 --- a/frontend/editor/src/core/contexts/FileContext.tsx +++ b/frontend/editor/src/core/contexts/FileContext.tsx @@ -197,6 +197,10 @@ function FileContextInner({ ); }, [activeEncryptedFileId, encryptedQueue]); + // The store outlives this provider, and a hold nobody can answer would stall the file's policy + // for the rest of the session. Its own effect, so a change of prompt does not clear and re-set. + useEffect(() => () => setPendingUnlocks([]), []); + useEffect(() => { setUnlockPassword(""); setUnlockError(null); diff --git a/frontend/editor/src/core/services/notificationRetry.test.ts b/frontend/editor/src/core/services/notificationRetry.test.ts index 9bdfba3549..6241579649 100644 --- a/frontend/editor/src/core/services/notificationRetry.test.ts +++ b/frontend/editor/src/core/services/notificationRetry.test.ts @@ -21,6 +21,7 @@ vi.mock("@app/services/apiClient", () => ({ const { stashRetryPayload, + clearRetryPayload, loadRetryPayload, hasLocalFile, retryWithPassword, @@ -126,6 +127,45 @@ describe("the retry stash", () => { }); }); + it("keeps a batch bigger than the cap whole, rather than dropping some of its files", async () => { + // One failed multi-file run writes a record per file under one recordedAt, so evicting by + // time alone would keep an arbitrary 25 of them and offer no retry for the rest. + const batch = Array.from({ length: 30 }, (_, i) => `b-${i}`); + await stashRetryPayload(payload({ fileIds: batch, recordedAt: 100 })); + + expect(await storedRecords()).toHaveLength(30); + for (const fileId of [batch[0], batch[15], batch[29]]) { + await expect(loadRetryPayload(fileId)).resolves.toMatchObject({ + operation: "remove-password", + }); + } + }); + + it("evicts earlier failures before the batch that just landed", async () => { + await stashRetryPayload(payload({ fileIds: ["old"], recordedAt: 1 })); + const batch = Array.from({ length: 25 }, (_, i) => `n-${i}`); + + await stashRetryPayload(payload({ fileIds: batch, recordedAt: 2 })); + + // The row on screen is the new one, so it is the older unrelated stash that goes. + await expect(loadRetryPayload("old")).resolves.toBeNull(); + await expect(loadRetryPayload("n-0")).resolves.toMatchObject({ + operation: "remove-password", + }); + }); + + it("forgets a file's stash once its failure is resolved", async () => { + await stashRetryPayload(payload({ fileIds: ["f-1", "f-2"] })); + + await clearRetryPayload("f-1"); + + await expect(loadRetryPayload("f-1")).resolves.toBeNull(); + // Per file: the other input's own row may still be open. + await expect(loadRetryPayload("f-2")).resolves.toMatchObject({ + operation: "remove-password", + }); + }); + it("stores no password, whichever field the tool submitted it in", async () => { await stashRetryPayload( payload({ diff --git a/frontend/editor/src/core/services/notificationRetry.ts b/frontend/editor/src/core/services/notificationRetry.ts index da1dd535fd..d57e6255a4 100644 --- a/frontend/editor/src/core/services/notificationRetry.ts +++ b/frontend/editor/src/core/services/notificationRetry.ts @@ -22,6 +22,11 @@ export interface RetryPayload { } /** Mirrored from the server's `FailureKind` declarations. */ +/** + * Mirrors the codes each {@code FailureKind} claims, server-side. Pinned there by + * `FailureKindTest#everyCodeAKindClaimsIsPinned`, which fails if a kind's codes change without + * this moving with them. + */ const KIND_ERROR_CODES: Record = { INPUT_PASSWORD_PROTECTED: "E004", }; @@ -48,7 +53,10 @@ const RETRY_DB_CONFIG: DatabaseConfig = { const STORE_NAME = "retryPayloads"; -/** Capped, oldest evicted first, so the stash cannot grow for the origin's lifetime. */ +/** + * Capped, oldest evicted first, so the stash cannot grow for the origin's lifetime. A batch that + * exceeds this on its own is kept whole: the failure a user is looking at outranks the cap. + */ const MAX_RETAINED_PAYLOADS = 25; /** One record per file involved, so a retry can be found from any of them. */ @@ -106,6 +114,19 @@ export async function loadRetryPayload( }; } +/** + * Drop the stash for a file whose failure is resolved. Never rejects: a record left behind is + * stale, not harmful, and it would be evicted eventually anyway. + */ +export async function clearRetryPayload(fileId: string | null): Promise { + if (!isUsableId(fileId)) return; + try { + await deleteRecord(fileId); + } catch { + // The retry it describes has already happened, so nothing reads it again. + } +} + /** Whether the document is still in this browser, which decides whether a retry can run. */ export async function hasLocalFile(fileId: string | null): Promise { if (!isUsableId(fileId)) return false; @@ -328,13 +349,21 @@ async function writeRecords(records: StoredRetryRecord[]): Promise { for (const record of records) store.put(record); // Evicted in the same transaction, so two concurrent stashes cannot both see room. + const justWritten = new Set(records.map((record) => record.fileId)); const all = store.getAll(); all.onsuccess = () => { const stored = (all.result ?? []) as StoredRetryRecord[]; const excess = stored.length - MAX_RETAINED_PAYLOADS; if (excess <= 0) return; stored - .sort((a, b) => a.recordedAt - b.recordedAt) + // One multi-file failure writes a record per file under one recordedAt, so evicting by + // time alone would drop arbitrary members of the batch that just landed. The fileId + // breaks the tie, and this batch is exempt: it is the failure with a row on screen. + .filter((record) => !justWritten.has(record.fileId)) + .sort( + (a, b) => + a.recordedAt - b.recordedAt || a.fileId.localeCompare(b.fileId), + ) .slice(0, excess) .forEach((record) => store.delete(record.fileId)); }; @@ -342,6 +371,19 @@ async function writeRecords(records: StoredRetryRecord[]): Promise { }); } +async function deleteRecord(fileId: string): Promise { + const db = await indexedDBManager.openDatabase(RETRY_DB_CONFIG); + + return new Promise((resolve, reject) => { + const transaction = db.transaction([STORE_NAME], "readwrite"); + transaction.objectStore(STORE_NAME).delete(fileId); + transaction.oncomplete = () => resolve(); + transaction.onerror = () => reject(transaction.error); + transaction.onabort = () => + reject(transaction.error ?? new Error("Retry stash delete aborted")); + }); +} + async function readRecord( fileId: string, ): Promise { diff --git a/frontend/editor/src/proprietary/components/notifications/notificationActions.ts b/frontend/editor/src/proprietary/components/notifications/notificationActions.ts index 68e4d7d481..6074525e25 100644 --- a/frontend/editor/src/proprietary/components/notifications/notificationActions.ts +++ b/frontend/editor/src/proprietary/components/notifications/notificationActions.ts @@ -16,6 +16,7 @@ import { import { EDITOR_BASENAME } from "@app/routes/editorBasename"; import { fileStorage } from "@app/services/fileStorage"; import { + clearRetryPayload, retryWithPassword, stashMatchesKind, unlockLocalDocument, @@ -436,6 +437,8 @@ export function useNotificationActions(): ClientActionRegistry { // Ignored on purpose: a refused resolve is not a failed unlock. await reportNotificationResolved(context.notification.id); + // The stash described the run that just succeeded, so it has nothing left to offer. + await clearRetryPayload(context.notification.fileId); return { ok: true }; }, }; diff --git a/frontend/editor/src/proprietary/hooks/tools/shared/useResolutionContinuation.ts b/frontend/editor/src/proprietary/hooks/tools/shared/useResolutionContinuation.ts index dc038a015b..6508c5a1dd 100644 --- a/frontend/editor/src/proprietary/hooks/tools/shared/useResolutionContinuation.ts +++ b/frontend/editor/src/proprietary/hooks/tools/shared/useResolutionContinuation.ts @@ -6,6 +6,7 @@ import { type AppNotification, } from "@app/services/notifications"; import { + clearRetryPayload, loadRetryPayload, stashMatchesKind, } from "@app/services/notificationRetry"; @@ -88,6 +89,7 @@ async function continueRow( ); // Anything short of a tracked run leaves the row open: the processed document was the point. if (!(outcome.ok && outcome.tracked)) return false; + await clearRetryPayload(row.fileId); return reportNotificationResolved(row.id); } @@ -101,6 +103,7 @@ async function continueRow( if (!stashMatchesKind(row.kindId, stash)) return false; if (!row.actions.some((a) => a.enabled && a.id === "OPEN_IN_TOOL")) return false; + await clearRetryPayload(row.fileId); return reportNotificationResolved(row.id); }