fix: keep a batch's retries whole, and tidy the stash's edges

Three of the review's findings, all in this PR's own code:

- Eviction treated a multi-file failure as 30 unrelated records sharing
  one recordedAt, so a batch over the cap kept an arbitrary 25 of its
  files and dropped the rest. The batch being written is now exempt from
  eviction and the fileId breaks the recordedAt tie, so what goes is the
  older unrelated stash rather than half of the failure on screen.
- A resolved failure's stash is deleted rather than left to age out.
- pendingUnlocks clears on FileContext teardown: the store outlives the
  provider, and a hold nobody can answer would stall that file's policy
  for the rest of the session.

KIND_ERROR_CODES now names the server-side test that pins what it
mirrors, and that test asserts the codes rather than only the lookup, so
adding one server-side fails until the mirror moves too.
This commit is contained in:
EthanHealy01
2026-09-02 15:12:58 +01:00
parent 1021307d5a
commit 097efd74e4
6 changed files with 104 additions and 2 deletions
@@ -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.
@@ -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);
@@ -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({
@@ -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<string, string> = {
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<void> {
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<boolean> {
if (!isUsableId(fileId)) return false;
@@ -328,13 +349,21 @@ async function writeRecords(records: StoredRetryRecord[]): Promise<void> {
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<void> {
});
}
async function deleteRecord(fileId: string): Promise<void> {
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<StoredRetryRecord | undefined> {
@@ -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 };
},
};
@@ -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);
}