diff --git a/src/__tests__/main.test.ts b/src/__tests__/main.test.ts index 0fc3fd64..2fe6d488 100644 --- a/src/__tests__/main.test.ts +++ b/src/__tests__/main.test.ts @@ -223,6 +223,7 @@ describe('main', () => { __mockOnDidOpenWebView.mockReturnValue(jest.fn()); __mockOnDidCloseWebView.mockReturnValue(jest.fn()); __mockNotificationsSend.mockResolvedValue('mock-notification-id'); + jest.mocked(projectStorage.sweepPendingCleanup).mockResolvedValue(0); }); describe('activate', () => { @@ -234,6 +235,33 @@ describe('main', () => { expect(isWebViewProvider(raw)).toBe(true); }); + it('runs the pending-cleanup sweep with the execution token', async () => { + const context = createTestActivationContext(); + + await activate(context); + + expect(jest.mocked(projectStorage.sweepPendingCleanup)).toHaveBeenCalledWith( + context.executionToken, + ); + }); + + it('logs but does not fail activation when the pending-cleanup sweep rejects', async () => { + jest + .mocked(projectStorage.sweepPendingCleanup) + .mockRejectedValue(new Error('sweep exploded')); + const context = createTestActivationContext(); + + await expect(activate(context)).resolves.toBeUndefined(); + + // The rejection resolves on a later microtask than activate()'s synchronous body, so let the + // fire-and-forget catch handler run before asserting it logged. + await Promise.resolve(); + expect(__mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('pending-cleanup sweep failed'), + expect.any(Error), + ); + }); + it('registers the interlinearizer.openForWebView command with a callable handler', async () => { const context = createTestActivationContext(); await activate(context); diff --git a/src/__tests__/services/projectStorage.test.ts b/src/__tests__/services/projectStorage.test.ts index 713a611c..a0eb750a 100644 --- a/src/__tests__/services/projectStorage.test.ts +++ b/src/__tests__/services/projectStorage.test.ts @@ -11,6 +11,7 @@ import { listProjects, resetQueuesForTesting, saveDraft, + sweepPendingCleanup, updateAnalysis, updateProjectMetadata, } from '../../services/projectStorage'; @@ -184,6 +185,78 @@ describe('projectStorage', () => { expect(__mockLogger.error).toHaveBeenCalled(); }); + + it('records the orphaned project for cleanup when rollback fails', async () => { + // Index read (ENOENT → []) and the later pendingCleanup read (ENOENT → []) both miss. + __mockReadUserData.mockRejectedValue(enoentError()); + __mockWriteUserData + .mockResolvedValueOnce(undefined) // project write succeeds + .mockRejectedValueOnce(new Error('disk full')); // index write fails + __mockDeleteUserData.mockRejectedValue(new Error('rollback failed')); + + await expect(createProject(token, 'src-proj', ['en'])).rejects.toThrow('disk full'); + + expect(__mockWriteUserData).toHaveBeenCalledWith( + token, + 'pendingCleanup', + JSON.stringify(['00000000-0000-0000-0000-000000000001']), + ); + }); + + it('logs and swallows a failure to record the orphan so the index error still surfaces', async () => { + __mockReadUserData.mockRejectedValue(enoentError()); + // project write ok; index write fails; pendingCleanup write also fails. + __mockWriteUserData + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error('disk full')) + .mockRejectedValueOnce(new Error('cleanup write failed')); + __mockDeleteUserData.mockRejectedValue(new Error('rollback failed')); + + await expect(createProject(token, 'src-proj', ['en'])).rejects.toThrow('disk full'); + + expect(__mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('record orphaned project'), + expect.any(Error), + ); + }); + + it('does not re-record an orphan already in the pending-cleanup set', async () => { + const orphanId = '00000000-0000-0000-0000-000000000001'; + // Index read → ENOENT ([]); pendingCleanup read → already contains this orphan. + __mockReadUserData.mockImplementation((_t: unknown, key: unknown) => + key === 'pendingCleanup' + ? Promise.resolve(JSON.stringify([orphanId])) + : Promise.reject(enoentError()), + ); + __mockWriteUserData + .mockResolvedValueOnce(undefined) // project write succeeds + .mockRejectedValueOnce(new Error('disk full')); // index write fails + __mockDeleteUserData.mockRejectedValue(new Error('rollback failed')); + + await expect(createProject(token, 'src-proj', ['en'])).rejects.toThrow('disk full'); + + expect(__mockWriteUserData).not.toHaveBeenCalledWith( + token, + 'pendingCleanup', + expect.anything(), + ); + }); + + it('does not record the orphan when the rollback delete succeeds', async () => { + __mockReadUserData.mockRejectedValue(enoentError()); + __mockWriteUserData + .mockResolvedValueOnce(undefined) // project write succeeds + .mockRejectedValueOnce(new Error('disk full')); // index write fails + // deleteUserData resolves (default mock) → rollback succeeds, nothing to record. + + await expect(createProject(token, 'src-proj', ['en'])).rejects.toThrow('disk full'); + + expect(__mockWriteUserData).not.toHaveBeenCalledWith( + token, + 'pendingCleanup', + expect.anything(), + ); + }); }); describe('getProject', () => { @@ -413,6 +486,149 @@ describe('projectStorage', () => { }); }); + describe('sweepPendingCleanup', () => { + /** + * Makes `readUserData` return `pendingCleanup` as the given id list and ENOENT for every other + * key, so a sweep sees exactly `ids` as its work set. + * + * @param ids - The project IDs the pending-cleanup set should contain. + */ + function stubPendingCleanup(ids: string[]): void { + __mockReadUserData.mockImplementation((_t: unknown, key: unknown) => + key === 'pendingCleanup' + ? Promise.resolve(JSON.stringify(ids)) + : Promise.reject(enoentError()), + ); + } + + it('returns 0 and writes nothing when the set is empty', async () => { + stubPendingCleanup([]); + + const cleaned = await sweepPendingCleanup(token); + + expect(cleaned).toBe(0); + expect(__mockDeleteUserData).not.toHaveBeenCalled(); + expect(__mockWriteUserData).not.toHaveBeenCalled(); + }); + + it('deletes each recorded record and clears the set on full success', async () => { + stubPendingCleanup(['orphan-a', 'orphan-b']); + + const cleaned = await sweepPendingCleanup(token); + + expect(cleaned).toBe(2); + expect(__mockDeleteUserData).toHaveBeenCalledWith(token, 'project:orphan-a'); + expect(__mockDeleteUserData).toHaveBeenCalledWith(token, 'project:orphan-b'); + expect(__mockWriteUserData).toHaveBeenCalledWith(token, 'pendingCleanup', JSON.stringify([])); + }); + + it('treats an already-missing record (ENOENT) as successfully cleaned', async () => { + stubPendingCleanup(['gone']); + __mockDeleteUserData.mockRejectedValue(enoentError()); + + const cleaned = await sweepPendingCleanup(token); + + expect(cleaned).toBe(1); + expect(__mockWriteUserData).toHaveBeenCalledWith(token, 'pendingCleanup', JSON.stringify([])); + }); + + it('retains an id whose deletion fails again and logs it', async () => { + stubPendingCleanup(['stubborn', 'ok']); + __mockDeleteUserData.mockImplementation((_t: unknown, key: unknown) => + key === 'project:stubborn' + ? Promise.reject(new Error('still locked')) + : Promise.resolve(undefined), + ); + + const cleaned = await sweepPendingCleanup(token); + + expect(cleaned).toBe(1); + expect(__mockWriteUserData).toHaveBeenCalledWith( + token, + 'pendingCleanup', + JSON.stringify(['stubborn']), + ); + expect(__mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('stubborn'), + expect.any(Error), + ); + }); + + it('propagates a non-ENOENT error from reading the pending-cleanup set', async () => { + __mockReadUserData.mockRejectedValue(new Error('disk full')); + + await expect(sweepPendingCleanup(token)).rejects.toThrow('disk full'); + }); + + it('does not rewrite the set when no ids could be cleaned', async () => { + stubPendingCleanup(['stubborn']); + __mockDeleteUserData.mockRejectedValue(new Error('still locked')); + + const cleaned = await sweepPendingCleanup(token); + + expect(cleaned).toBe(0); + expect(__mockWriteUserData).not.toHaveBeenCalled(); + }); + + it('never deletes the record of an id still present in the index', async () => { + // 'live' is both recorded for cleanup and still in the index (e.g. an index write that + // persisted but reported failure). Its backing record must not be deleted. + __mockReadUserData.mockImplementation((_t: unknown, key: unknown) => { + if (key === 'pendingCleanup') return Promise.resolve(JSON.stringify(['live', 'orphan'])); + if (key === 'projectIds') return Promise.resolve(JSON.stringify(['live'])); + return Promise.reject(enoentError()); + }); + + const cleaned = await sweepPendingCleanup(token); + + expect(__mockDeleteUserData).not.toHaveBeenCalledWith(token, 'project:live'); + expect(__mockDeleteUserData).toHaveBeenCalledWith(token, 'project:orphan'); + // 'live' is only a real orphan record when deleted; it was skipped, so it is not counted. + expect(cleaned).toBe(1); + }); + + it('drops a live id from the set without counting or deleting it', async () => { + __mockReadUserData.mockImplementation((_t: unknown, key: unknown) => { + if (key === 'pendingCleanup') return Promise.resolve(JSON.stringify(['live'])); + if (key === 'projectIds') return Promise.resolve(JSON.stringify(['live'])); + return Promise.reject(enoentError()); + }); + + const cleaned = await sweepPendingCleanup(token); + + expect(cleaned).toBe(0); + expect(__mockDeleteUserData).not.toHaveBeenCalled(); + // The set is rewritten to drop the non-orphan id even though nothing was deleted. + expect(__mockWriteUserData).toHaveBeenCalledWith(token, 'pendingCleanup', JSON.stringify([])); + }); + + it('resets a pending-cleanup value containing invalid JSON to empty instead of wedging', async () => { + __mockReadUserData.mockImplementation((_t: unknown, key: unknown) => + key === 'pendingCleanup' ? Promise.resolve('{ not json') : Promise.reject(enoentError()), + ); + + const cleaned = await sweepPendingCleanup(token); + + expect(cleaned).toBe(0); + expect(__mockDeleteUserData).not.toHaveBeenCalled(); + expect(__mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('invalid JSON')); + }); + + it('resets a pending-cleanup value that is not an array of strings to empty', async () => { + __mockReadUserData.mockImplementation((_t: unknown, key: unknown) => + key === 'pendingCleanup' + ? Promise.resolve(JSON.stringify([1, 2, 3])) + : Promise.reject(enoentError()), + ); + + const cleaned = await sweepPendingCleanup(token); + + expect(cleaned).toBe(0); + expect(__mockDeleteUserData).not.toHaveBeenCalled(); + expect(__mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('not an array')); + }); + }); + describe('updateAnalysis', () => { const storedProject = makeStubProject('proj-id'); const newAnalysis = { diff --git a/src/main.ts b/src/main.ts index a12391d3..964270f1 100644 --- a/src/main.ts +++ b/src/main.ts @@ -382,6 +382,13 @@ export async function activate(context: ExecutionActivationContext): Promise { + logger.error('Interlinearizer: pending-cleanup sweep failed during activation:', e); + }); + const mainWebViewProviderRegistration = await papi.webViewProviders.registerWebViewProvider( mainWebViewType, mainWebViewProvider, diff --git a/src/services/projectStorage.ts b/src/services/projectStorage.ts index 14298c32..bb81e0d0 100644 --- a/src/services/projectStorage.ts +++ b/src/services/projectStorage.ts @@ -6,6 +6,14 @@ import { isDraftProject } from '../types/type-guards'; const PROJECT_IDS_KEY = 'projectIds'; +/** + * Storage key holding the set of project IDs whose records were orphaned by a failed rollback (see + * {@link createProject}). Each entry is a `project:{id}` record that was written but never added to + * the index and could not be deleted at the time; {@link sweepPendingCleanup} retries their + * deletion. + */ +const PENDING_CLEANUP_KEY = 'pendingCleanup'; + /** * Serializes all read-modify-write operations on the shared `projectIds` index. Every operation * that reads then writes the index must be enqueued here so concurrent calls (e.g. from two open @@ -13,6 +21,13 @@ const PROJECT_IDS_KEY = 'projectIds'; */ let indexQueue: Promise = Promise.resolve(); +/** + * Serializes all read-modify-write operations on the shared `pendingCleanup` set. Recording a new + * orphan (from a failed rollback) and the sweep that retries deletions must not interleave at await + * boundaries, or one could overwrite the other's changes to the set. + */ +let pendingCleanupQueue: Promise = Promise.resolve(); + /** * Per-project serialization queues. Keyed by project ID; each entry serializes all * read-modify-write operations on that project's storage record so concurrent update and delete @@ -28,20 +43,66 @@ const projectQueues = new Map>(); const draftQueues = new Map>(); /** - * Enqueues `fn` on the index serialization queue and returns a promise that resolves or rejects - * with `fn`'s result. The queue always advances regardless of whether `fn` throws. + * Enqueues `fn` on a single shared serialization queue and returns a promise that resolves or + * rejects with `fn`'s result. The queue always advances regardless of whether `fn` throws, so a + * failed operation does not block later ones. Shared by {@link enqueueIndexOp} and + * {@link enqueuePendingCleanupOp}, which differ only in which module-level queue they advance. * + * @param get - Returns the current tail of the queue to chain `fn` after. + * @param set - Stores the new tail of the queue (a promise that settles once `fn` settles). * @param fn - The async function to serialize. * @returns A promise that resolves or rejects with the return value of `fn`. * @throws Whatever `fn` throws; the queue advances past the error so later operations are not * blocked. */ -function enqueueIndexOp(fn: () => Promise): Promise { - const result = indexQueue.then(fn); - indexQueue = result.catch(() => {}); +function enqueueOnQueue( + get: () => Promise, + set: (queue: Promise) => void, + fn: () => Promise, +): Promise { + const result = get().then(fn); + set(result.catch(() => {})); return result; } +/** + * Enqueues `fn` on the index serialization queue. Thin wrapper over {@link enqueueOnQueue} bound to + * {@link indexQueue}. + * + * @param fn - The async function to serialize. + * @returns A promise that resolves or rejects with the return value of `fn`. + * @throws Whatever `fn` throws; the queue advances past the error so later operations are not + * blocked. + */ +function enqueueIndexOp(fn: () => Promise): Promise { + return enqueueOnQueue( + () => indexQueue, + (q) => { + indexQueue = q; + }, + fn, + ); +} + +/** + * Enqueues `fn` on the pending-cleanup serialization queue. Thin wrapper over {@link enqueueOnQueue} + * bound to {@link pendingCleanupQueue}. + * + * @param fn - The async function to serialize. + * @returns A promise that resolves or rejects with the return value of `fn`. + * @throws Whatever `fn` throws; the queue advances past the error so later operations are not + * blocked. + */ +function enqueuePendingCleanupOp(fn: () => Promise): Promise { + return enqueueOnQueue( + () => pendingCleanupQueue, + (q) => { + pendingCleanupQueue = q; + }, + fn, + ); +} + /** * Enqueues `fn` on the serialization queue identified by `key` within `queues` and returns a * promise that resolves or rejects with `fn`'s result. Cleans up the queue entry when the operation @@ -115,6 +176,27 @@ function isNotFound(e: unknown): boolean { return !!e && typeof e === 'object' && 'code' in e && e.code === 'ENOENT'; } +/** + * Reads and JSON-parses the string-array stored at `key`, treating a never-written key as an empty + * array. Shared by {@link readIds} and {@link readPendingCleanup}. Does not validate the parsed + * shape; callers that must tolerate corruption layer their own validation on top. + * + * @param token - The execution token for storage access. + * @param key - The storage key to read. + * @returns The parsed value, or an empty array if `key` has never been written (ENOENT). + * @throws {SyntaxError} If the stored value contains invalid JSON. + * @throws If `papi.storage.readUserData` rejects for any non-ENOENT reason (e.g. permission denied, + * I/O error). + */ +async function readJsonArray(token: ExecutionToken, key: string): Promise { + try { + return JSON.parse(await papi.storage.readUserData(token, key)); + } catch (e) { + if (isNotFound(e)) return []; + throw e; + } +} + /** * Reads the stored list of project IDs. * @@ -125,13 +207,118 @@ function isNotFound(e: unknown): boolean { * @throws If `papi.storage.readUserData` rejects for any non-ENOENT reason (e.g. permission denied, * I/O error). */ -async function readIds(token: ExecutionToken): Promise { +function readIds(token: ExecutionToken): Promise { + return readJsonArray(token, PROJECT_IDS_KEY); +} + +/** + * Reads the stored set of orphaned project IDs awaiting cleanup, tolerating corruption so a bad + * value can never wedge {@link sweepPendingCleanup} (which runs fire-and-forget at activation, where + * a thrown error would be invisible and would recur on every launch). A value that is missing + * (ENOENT), unparseable, or not an array of strings is treated as an empty set: the sweep then has + * nothing to do and its terminal rewrite replaces the bad value with a valid one, self-healing the + * key. + * + * @param token - The execution token for storage access. + * @returns The stored pending-cleanup ID array, or an empty array when the value is missing or + * corrupt. + * @throws If `papi.storage.readUserData` rejects for any non-ENOENT reason. + */ +async function readPendingCleanup(token: ExecutionToken): Promise { + let parsed: unknown; try { - return JSON.parse(await papi.storage.readUserData(token, PROJECT_IDS_KEY)); + parsed = await readJsonArray(token, PENDING_CLEANUP_KEY); } catch (e) { - if (isNotFound(e)) return []; + if (e instanceof SyntaxError) { + logger.warn('Interlinearizer: pending-cleanup set contains invalid JSON; resetting to empty'); + return []; + } throw e; } + if (!Array.isArray(parsed) || !parsed.every((id) => typeof id === 'string')) { + logger.warn( + 'Interlinearizer: pending-cleanup set is not an array of strings; resetting to empty', + ); + return []; + } + return parsed; +} + +/** + * Records `id` in the persistent `pendingCleanup` set so a later {@link sweepPendingCleanup} can + * retry deleting its orphaned `project:{id}` record. Serialized through {@link pendingCleanupQueue} + * and idempotent: an id already in the set is not added twice. + * + * @param token - The execution token for storage access. + * @param id - The orphaned project UUID to record for later cleanup. + * @returns A promise that resolves once the id has been persisted (or was already present). + * @throws If `papi.storage.readUserData` or `papi.storage.writeUserData` rejects for a non-ENOENT + * reason. A corrupt `pendingCleanup` value is not thrown; {@link readPendingCleanup} resets it to + * an empty set. + */ +function recordPendingCleanup(token: ExecutionToken, id: string): Promise { + return enqueuePendingCleanupOp(async () => { + const ids = await readPendingCleanup(token); + if (ids.includes(id)) return; + await papi.storage.writeUserData(token, PENDING_CLEANUP_KEY, JSON.stringify([...ids, id])); + }); +} + +/** + * Retries deleting the orphaned project records recorded in the `pendingCleanup` set (see + * {@link createProject}). For each recorded id: + * + * - If the id is still present in the `projectIds` index, it belongs to a live project — its record + * must not be deleted. The id is dropped from the set without touching its record; it should + * never have been recorded (or has since become live) and is not an orphan to clean. + * - Otherwise the record is an orphan: its `project:{id}` deletion is attempted, treating ENOENT as + * success (already gone). On success the id is dropped from the set; on failure it is retained + * for the next attempt and logged. + * + * Consulting the index guards against destroying a live project's record if an id ever lands in the + * set while still indexed (e.g. an index write that persists but then reports failure). Intended to + * run opportunistically at activation. + * + * The whole read-delete-rewrite cycle is serialized through {@link pendingCleanupQueue} so it cannot + * interleave with a concurrent {@link recordPendingCleanup} and drop a newly recorded orphan. The + * per-record deletions run concurrently within the cycle; the set is small and each targets a + * distinct key. + * + * @param token - The execution token for storage access. + * @returns A promise resolving to the number of orphaned records successfully deleted this pass + * (live ids dropped from the set without deleting their record are not counted). + * @throws If reading the set or rewriting it (`papi.storage.writeUserData`) rejects for a + * non-ENOENT reason. A per-record delete failure is not thrown; the id is retained instead. + */ +export function sweepPendingCleanup(token: ExecutionToken): Promise { + return enqueuePendingCleanupOp(async () => { + const ids = await readPendingCleanup(token); + if (ids.length === 0) return 0; + const indexed = new Set(await readIds(token)); + // For each id, decide whether to keep it in the set and whether its record was cleaned. + const outcomes = await Promise.all( + ids.map(async (id) => { + if (indexed.has(id)) { + // Live project: never delete its record. Drop it from the set as a non-orphan. + logger.warn(`Interlinearizer: pending-cleanup id ${id} is a live project; not deleting`); + return { keep: false, cleaned: false }; + } + try { + await papi.storage.deleteUserData(token, projectKey(id)); + return { keep: false, cleaned: true }; + } catch (e) { + if (isNotFound(e)) return { keep: false, cleaned: true }; + logger.error(`Interlinearizer: cleanup of orphaned project ${id} failed again:`, e); + return { keep: true, cleaned: false }; + } + }), + ); + const remaining = ids.filter((_id, i) => outcomes[i].keep); + if (remaining.length !== ids.length) { + await papi.storage.writeUserData(token, PENDING_CLEANUP_KEY, JSON.stringify(remaining)); + } + return outcomes.filter((o) => o.cleaned).length; + }); } /** @@ -149,8 +336,10 @@ async function readIds(token: ExecutionToken): Promise { * @param description - Optional user-facing description for the project. * @returns The newly created project record. * @throws {SyntaxError} If the `projectIds` storage value contains invalid JSON. - * @throws If `papi.storage.writeUserData` (project or index) or rollback via - * `papi.storage.deleteUserData` rejects for a non-ENOENT reason. + * @throws If the project or index `papi.storage.writeUserData` rejects. On an index-write failure + * the original error is rethrown after best-effort rollback; a failed rollback does not throw — + * the orphaned record is instead recorded for a later {@link sweepPendingCleanup} (and a failure + * to even record it is logged and swallowed). */ export async function createProject( token: ExecutionToken, @@ -184,6 +373,12 @@ export async function createProject( await papi.storage.deleteUserData(token, projectKey(id)); } catch (rollbackError) { logger.error(`Failed to roll back project ${id} after index write failure:`, rollbackError); + // The orphaned record was written but never indexed and could not be deleted. Record it so a + // later sweep retries the deletion; a failure to even record it is logged and swallowed so it + // never masks the original index error the caller needs to see. + await recordPendingCleanup(token, id).catch((recordError) => { + logger.error(`Failed to record orphaned project ${id} for cleanup:`, recordError); + }); } throw indexError; } @@ -419,6 +614,7 @@ export async function saveDraft( */ export function resetQueuesForTesting(): void { indexQueue = Promise.resolve(); + pendingCleanupQueue = Promise.resolve(); projectQueues.clear(); draftQueues.clear(); }