diff --git a/src/cli/commands/record-export.ts b/src/cli/commands/record-export.ts index 56ab389..3d9068b 100644 --- a/src/cli/commands/record-export.ts +++ b/src/cli/commands/record-export.ts @@ -24,7 +24,7 @@ import { type RecordExportResult, } from '../../protocol/messages.js'; import { - appendArtifact, + appendArtifactWithRollback, createArtifactEntry, } from '../../storage/artifactManifest.js'; import { @@ -371,18 +371,26 @@ export async function runRecordExportCommand( sha256 = await computeFileHash(artifactOutputPath); } - await appendArtifact( - sessionDirectory, - createArtifactEntry({ - kind: artifactKind, - filename: basename(artifactOutputPath), - sessionId: manifest.sessionId, - capturedAtSeq, - sha256, - bytes, - metadata: artifactMetadata, - }), - ); + const artifactEntry = createArtifactEntry({ + kind: artifactKind, + filename: basename(artifactOutputPath), + sessionId: manifest.sessionId, + capturedAtSeq, + sha256, + bytes, + metadata: artifactMetadata, + }); + + // Explicit --out files belong to the user and are valid without a + // manifest entry; only clean up default in-session artifacts that would + // otherwise be orphaned. + await appendArtifactWithRollback({ + sessionDir: sessionDirectory, + entry: artifactEntry, + ...(options.out === undefined + ? { rollbackArtifactPath: artifactOutputPath } + : {}), + }); const rawResult = { sessionId: manifest.sessionId, diff --git a/src/screenshot/capture.ts b/src/screenshot/capture.ts index 3f30f68..797ca9d 100644 --- a/src/screenshot/capture.ts +++ b/src/screenshot/capture.ts @@ -8,7 +8,7 @@ import type { RendererBackend } from '../renderer/backend.js'; import { ScreenshotResultSchema } from '../protocol/schemas.js'; import { parseValidatedResult } from '../protocol/validation.js'; import { - appendArtifact, + appendArtifactWithRollback, createArtifactEntry, } from '../storage/artifactManifest.js'; import { @@ -133,28 +133,31 @@ export async function captureScreenshotResult( sha256: parsedResult.sha256, }; + const artifactEntry = createArtifactEntry({ + kind: 'screenshot', + filename, + sessionId: publicResult.sessionId, + capturedAtSeq: publicResult.capturedAtSeq, + sha256, + metadata: { + profileName: publicResult.profileName, + cols: publicResult.cols, + rows: publicResult.rows, + pngSizeBytes: publicResult.pngSizeBytes, + cursorVisible: publicResult.cursorVisible, + rendererBackend: publicResult.rendererBackend, + pixelWidth: publicResult.pixelWidth, + pixelHeight: publicResult.pixelHeight, + renderProfileHash: publicResult.renderProfileHash, + }, + }); + await rename(temporaryOutputPath, finalArtifactPath); - await appendArtifact( - options.sessionDir, - createArtifactEntry({ - kind: 'screenshot', - filename, - sessionId: publicResult.sessionId, - capturedAtSeq: publicResult.capturedAtSeq, - sha256, - metadata: { - profileName: publicResult.profileName, - cols: publicResult.cols, - rows: publicResult.rows, - pngSizeBytes: publicResult.pngSizeBytes, - cursorVisible: publicResult.cursorVisible, - rendererBackend: publicResult.rendererBackend, - pixelWidth: publicResult.pixelWidth, - pixelHeight: publicResult.pixelHeight, - renderProfileHash: publicResult.renderProfileHash, - }, - }), - ); + await appendArtifactWithRollback({ + sessionDir: options.sessionDir, + entry: artifactEntry, + rollbackArtifactPath: finalArtifactPath, + }); return publicResult; } catch (error) { diff --git a/src/snapshot/capture.ts b/src/snapshot/capture.ts index 3c82efa..ed766c8 100644 --- a/src/snapshot/capture.ts +++ b/src/snapshot/capture.ts @@ -5,7 +5,7 @@ import { ERROR_CODES, makeCliError } from '../protocol/errors.js'; import { SnapshotResultSchema } from '../protocol/schemas.js'; import { parseValidatedResult } from '../protocol/validation.js'; import { - appendArtifact, + appendArtifactWithRollback, createArtifactEntry, } from '../storage/artifactManifest.js'; import { @@ -126,6 +126,24 @@ export async function persistSnapshotArtifact( ); const snapshotArtifactPath = artifactPath(options.sessionDir, filename); + const artifactEntry = createArtifactEntry({ + kind: 'snapshot', + filename, + sessionId: options.snapshot.sessionId, + capturedAtSeq: options.snapshot.capturedAtSeq, + metadata: { + format: options.format, + rendererBackend: options.rendererBackend, + cols: options.snapshot.cols, + rows: options.snapshot.rows, + cursorRow: options.snapshot.cursorRow, + cursorCol: options.snapshot.cursorCol, + ...(options.snapshot.scrollbackLines === undefined + ? {} + : { scrollbackLineCount: options.snapshot.scrollbackLines.length }), + }, + }); + await writeTextFileAtomic({ path: snapshotArtifactPath, pathLabel: 'snapshot artifact path', @@ -133,26 +151,11 @@ export async function persistSnapshotArtifact( writeErrorMessage: `Failed to write snapshot artifact at ${snapshotArtifactPath}.`, }); - await appendArtifact( - options.sessionDir, - createArtifactEntry({ - kind: 'snapshot', - filename, - sessionId: options.snapshot.sessionId, - capturedAtSeq: options.snapshot.capturedAtSeq, - metadata: { - format: options.format, - rendererBackend: options.rendererBackend, - cols: options.snapshot.cols, - rows: options.snapshot.rows, - cursorRow: options.snapshot.cursorRow, - cursorCol: options.snapshot.cursorCol, - ...(options.snapshot.scrollbackLines === undefined - ? {} - : { scrollbackLineCount: options.snapshot.scrollbackLines.length }), - }, - }), - ); + await appendArtifactWithRollback({ + sessionDir: options.sessionDir, + entry: artifactEntry, + rollbackArtifactPath: snapshotArtifactPath, + }); } export async function captureSnapshotResult( diff --git a/src/storage/artifactManifest.ts b/src/storage/artifactManifest.ts index 551fb15..f229db8 100644 --- a/src/storage/artifactManifest.ts +++ b/src/storage/artifactManifest.ts @@ -1,4 +1,5 @@ -import { basename, resolve } from 'node:path'; +import { rm } from 'node:fs/promises'; +import { basename, isAbsolute, resolve } from 'node:path'; import { ulid } from 'ulid'; import { z } from 'zod'; @@ -171,23 +172,63 @@ export async function writeArtifactManifest( }); } -export async function appendArtifact( +export interface AppendArtifactWithRollbackOptions { + sessionDir: string; + entry: ArtifactEntry; + rollbackArtifactPath?: string; +} + +async function appendArtifact( sessionDir: string, entry: ArtifactEntry, + rollbackArtifactPath: string | undefined, ): Promise { const resolvedSessionDir = resolve(sessionDir); const expectedSessionId = sessionIdFromSessionDir(resolvedSessionDir); - const validatedEntry = validateArtifactEntry(entry, expectedSessionId); - await appendSerializer.run(resolvedSessionDir, async () => { - const manifest = await readArtifactManifest(resolvedSessionDir); - await writeArtifactManifest(resolvedSessionDir, { - ...manifest, - artifacts: [...manifest.artifacts, validatedEntry], - }); + try { + const validatedEntry = validateArtifactEntry(entry, expectedSessionId); + const manifest = await readArtifactManifest(resolvedSessionDir); + await writeArtifactManifest(resolvedSessionDir, { + ...manifest, + artifacts: [...manifest.artifacts, validatedEntry], + }); + } catch (error) { + if (rollbackArtifactPath !== undefined) { + // Best-effort: swallow rm errors so the original manifest failure propagates. + await rm(rollbackArtifactPath, { force: true }).catch(() => undefined); + } + throw error; + } }); } +/** + * Append an artifact entry to the session manifest, removing the artifact file + * at `rollbackArtifactPath` if the append fails. Rollback is best-effort: rm + * errors are swallowed so the original manifest error propagates. + */ +export async function appendArtifactWithRollback( + options: AppendArtifactWithRollbackOptions, +): Promise { + if (options.rollbackArtifactPath !== undefined) { + invariant( + options.rollbackArtifactPath.length > 0, + 'rollbackArtifactPath must be a non-empty string', + ); + invariant( + isAbsolute(options.rollbackArtifactPath), + 'rollbackArtifactPath must be absolute', + ); + } + + await appendArtifact( + options.sessionDir, + options.entry, + options.rollbackArtifactPath, + ); +} + export function createArtifactEntry( entry: Omit, ): ArtifactEntry { diff --git a/test/unit/commands/record-export.test.ts b/test/unit/commands/record-export.test.ts index 03daf3f..f6900c8 100644 --- a/test/unit/commands/record-export.test.ts +++ b/test/unit/commands/record-export.test.ts @@ -1,14 +1,22 @@ import { createHash } from 'node:crypto'; -import { mkdir, mkdtemp, realpath, rm, symlink } from 'node:fs/promises'; +import { + access, + mkdir, + mkdtemp, + realpath, + rm, + symlink, + writeFile, +} from 'node:fs/promises'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; import process from 'node:process'; import type * as FsPromises from 'node:fs/promises'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { ERROR_CODES } from '../../../src/protocol/errors.js'; +import { ERROR_CODES, makeCliError } from '../../../src/protocol/errors.js'; const mocks = vi.hoisted(() => ({ emitSuccess: vi.fn(), @@ -19,6 +27,8 @@ const mocks = vi.hoisted(() => ({ manifestPath: vi.fn(), eventLogPath: vi.fn(), writeTextFileAtomic: vi.fn(), + appendArtifactWithRollback: vi.fn(), + // Test-internal delegate used by the appendArtifactWithRollback mock. appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), ensureArtifactsDir: vi.fn(), @@ -54,7 +64,7 @@ vi.mock('../../../src/storage/sessionPaths.js', () => ({ })); vi.mock('../../../src/storage/artifactManifest.js', () => ({ - appendArtifact: mocks.appendArtifact, + appendArtifactWithRollback: mocks.appendArtifactWithRollback, createArtifactEntry: mocks.createArtifactEntry, })); @@ -172,6 +182,24 @@ describe('record export command', () => { `${sessionDirectory}/artifacts/${filename}`, ); mocks.writeTextFileAtomic.mockResolvedValue(undefined); + mocks.appendArtifactWithRollback.mockImplementation( + async (options: { + sessionDir: string; + entry: unknown; + rollbackArtifactPath?: string; + }) => { + try { + await mocks.appendArtifact(options.sessionDir, options.entry); + } catch (error) { + if (options.rollbackArtifactPath !== undefined) { + await rm(options.rollbackArtifactPath, { force: true }).catch( + () => undefined, + ); + } + throw error; + } + }, + ); mocks.appendArtifact.mockResolvedValue(undefined); mocks.createArtifactEntry.mockImplementation((entry: unknown) => ({ id: 'artifact-01', @@ -264,15 +292,24 @@ describe('record export command', () => { markerCount: 1, }, }); - expect(mocks.appendArtifact).toHaveBeenCalledWith( - '/tmp/agent-tty/sessions/session-01', - expect.objectContaining({ - kind: 'recording', - filename: 'recording-2-asciicast.cast', - sha256: expectedSha256, - bytes: Buffer.byteLength(expectedContents, 'utf8'), - }), - ); + const appendCall = mocks.appendArtifactWithRollback.mock.calls.at(-1) as [ + { + sessionDir: string; + entry: Record; + rollbackArtifactPath?: string; + }, + ]; + expect(appendCall[0]).toMatchObject({ + sessionDir: '/tmp/agent-tty/sessions/session-01', + rollbackArtifactPath: + '/tmp/agent-tty/sessions/session-01/artifacts/recording-2-asciicast.cast', + }); + expect(appendCall[0].entry).toMatchObject({ + kind: 'recording', + filename: 'recording-2-asciicast.cast', + sha256: expectedSha256, + bytes: Buffer.byteLength(expectedContents, 'utf8'), + }); expect(mocks.emitSuccess).toHaveBeenCalledWith({ command: 'record export', json: true, @@ -442,6 +479,142 @@ describe('record export command', () => { expect(emitSuccessArgs.result.durationMs).toBe(1_500); }); + it('requests rollback for default asciicast artifacts when manifest append fails', async () => { + const sessionDirectory = await createTemporaryDirectory( + 'agent-tty-record-export-append-asciicast-', + ); + const artifactFile = join( + sessionDirectory, + 'artifacts', + 'recording-2-asciicast.cast', + ); + const manifestError = makeCliError(ERROR_CODES.MANIFEST_VALIDATION_ERROR, { + message: 'artifact manifest append failed', + }); + + mocks.sessionDir.mockReturnValue(sessionDirectory); + mocks.manifestPath.mockReturnValue(join(sessionDirectory, 'session.json')); + mocks.eventLogPath.mockReturnValue(join(sessionDirectory, 'events.jsonl')); + mocks.ensureArtifactsDir.mockResolvedValue( + join(sessionDirectory, 'artifacts'), + ); + mocks.recordingFilename.mockReturnValue('recording-2-asciicast.cast'); + mocks.writeTextFileAtomic.mockImplementation( + async (options: { path: string; contents: string }) => { + await mkdir(dirname(options.path), { recursive: true }); + await writeFile(options.path, options.contents, 'utf8'); + }, + ); + mocks.appendArtifact.mockRejectedValue(manifestError); + + await expect( + runRecordExportCommand({ + context: TEST_CONTEXT, + json: true, + sessionId: 'session-01', + format: 'asciicast', + }), + ).rejects.toBe(manifestError); + + expect(mocks.appendArtifactWithRollback).toHaveBeenCalledWith( + expect.objectContaining({ rollbackArtifactPath: artifactFile }), + ); + expect(mocks.emitSuccess).not.toHaveBeenCalled(); + }); + + it('requests rollback for default webm artifacts when manifest append fails', async () => { + const sessionDirectory = await createTemporaryDirectory( + 'agent-tty-record-export-append-webm-', + ); + const artifactFile = join( + sessionDirectory, + 'artifacts', + 'recording-1-webm.webm', + ); + const webmBytes = 12_345; + const webmContent = Buffer.alloc(webmBytes, 0x42); + const manifestError = makeCliError(ERROR_CODES.MANIFEST_VALIDATION_ERROR, { + message: 'artifact manifest append failed', + }); + + mocks.sessionDir.mockReturnValue(sessionDirectory); + mocks.manifestPath.mockReturnValue(join(sessionDirectory, 'session.json')); + mocks.eventLogPath.mockReturnValue(join(sessionDirectory, 'events.jsonl')); + mocks.ensureArtifactsDir.mockResolvedValue( + join(sessionDirectory, 'artifacts'), + ); + mocks.recordingFilename.mockReturnValue('recording-1-webm.webm'); + mocks.generateWebmExport.mockImplementation( + async (options: { outputPath: string }) => { + await mkdir(dirname(options.outputPath), { recursive: true }); + await writeFile(options.outputPath, webmContent); + return { + capturedAtSeq: 1, + durationMs: 1_500, + outputEventCount: 1, + resizeEventCount: 1, + cols: 80, + rows: 24, + profileName: 'reference-dark', + timingMode: 'accelerated', + rendererBackend: 'ghostty-web', + }; + }, + ); + mocks.stat.mockResolvedValue({ size: webmBytes }); + mocks.readFile.mockResolvedValue(webmContent); + mocks.appendArtifact.mockRejectedValue(manifestError); + + await expect( + runRecordExportCommand({ + context: TEST_CONTEXT, + json: true, + sessionId: 'session-01', + format: 'webm', + }), + ).rejects.toBe(manifestError); + + expect(mocks.appendArtifactWithRollback).toHaveBeenCalledWith( + expect.objectContaining({ rollbackArtifactPath: artifactFile }), + ); + expect(mocks.emitSuccess).not.toHaveBeenCalled(); + }); + + it('preserves explicit output artifacts when manifest append fails', async () => { + const workspaceDirectory = await createTemporaryDirectory( + 'agent-tty-record-export-explicit-out-', + ); + const artifactFile = join(workspaceDirectory, 'custom.cast'); + const manifestError = makeCliError(ERROR_CODES.MANIFEST_VALIDATION_ERROR, { + message: 'artifact manifest append failed', + }); + + mocks.writeTextFileAtomic.mockImplementation( + async (options: { path: string; contents: string }) => { + await mkdir(dirname(options.path), { recursive: true }); + await writeFile(options.path, options.contents, 'utf8'); + }, + ); + mocks.appendArtifact.mockRejectedValue(manifestError); + + await expect( + runRecordExportCommand({ + context: TEST_CONTEXT, + json: true, + sessionId: 'session-01', + format: 'asciicast', + out: artifactFile, + }), + ).rejects.toBe(manifestError); + + const appendCall = mocks.appendArtifactWithRollback.mock.calls.at(-1) as [ + { rollbackArtifactPath?: string }, + ]; + expect(appendCall[0]).not.toHaveProperty('rollbackArtifactPath'); + await expect(access(artifactFile)).resolves.toBeUndefined(); + expect(mocks.emitSuccess).not.toHaveBeenCalled(); + }); + it('resolves WebM profile from command option, context default, and builtin fallback', async () => { mocks.recordingFilename.mockReturnValue('recording-1-webm.webm'); mocks.generateWebmExport.mockResolvedValue({ diff --git a/test/unit/commands/screenshot.test.ts b/test/unit/commands/screenshot.test.ts index 0844f5f..1b236dd 100644 --- a/test/unit/commands/screenshot.test.ts +++ b/test/unit/commands/screenshot.test.ts @@ -13,6 +13,8 @@ const mocks = vi.hoisted(() => ({ manifestPath: vi.fn(), socketPath: vi.fn(), withOfflineReplayRenderer: vi.fn(), + appendArtifactWithRollback: vi.fn(), + // Test-internal delegate used by the appendArtifactWithRollback mock. appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), ensureArtifactsDir: vi.fn(), @@ -52,7 +54,7 @@ vi.mock('../../../src/replay/offlineReplay.js', () => ({ })); vi.mock('../../../src/storage/artifactManifest.js', () => ({ - appendArtifact: mocks.appendArtifact, + appendArtifactWithRollback: mocks.appendArtifactWithRollback, createArtifactEntry: mocks.createArtifactEntry, })); @@ -193,6 +195,11 @@ describe('screenshot command', () => { (sessionDirectory: string) => `${sessionDirectory}/rpc.sock`, ); mocks.readManifestIfExists.mockResolvedValue(createRunningSessionRecord()); + mocks.appendArtifactWithRollback.mockImplementation( + async (options: { sessionDir: string; entry: unknown }) => { + await mocks.appendArtifact(options.sessionDir, options.entry); + }, + ); mocks.appendArtifact.mockResolvedValue(undefined); mocks.createArtifactEntry.mockImplementation((entry: unknown) => ({ id: 'artifact-01', @@ -536,7 +543,7 @@ describe('screenshot command', () => { { force: true }, ); expect(mocks.rename).not.toHaveBeenCalled(); - expect(mocks.appendArtifact).not.toHaveBeenCalled(); + expect(mocks.appendArtifactWithRollback).not.toHaveBeenCalled(); expect(mocks.emitSuccess).not.toHaveBeenCalled(); }); diff --git a/test/unit/commands/snapshot.test.ts b/test/unit/commands/snapshot.test.ts index 7ca7c6d..75393f5 100644 --- a/test/unit/commands/snapshot.test.ts +++ b/test/unit/commands/snapshot.test.ts @@ -12,6 +12,8 @@ const mocks = vi.hoisted(() => ({ manifestPath: vi.fn(), socketPath: vi.fn(), withOfflineReplayRenderer: vi.fn(), + appendArtifactWithRollback: vi.fn(), + // Test-internal delegate used by the appendArtifactWithRollback mock. appendArtifact: vi.fn(), createArtifactEntry: vi.fn(), artifactPath: vi.fn(), @@ -33,7 +35,7 @@ vi.mock('../../../src/replay/offlineReplay.js', () => ({ })); vi.mock('../../../src/storage/artifactManifest.js', () => ({ - appendArtifact: mocks.appendArtifact, + appendArtifactWithRollback: mocks.appendArtifactWithRollback, createArtifactEntry: mocks.createArtifactEntry, })); @@ -161,6 +163,11 @@ describe('snapshot command', () => { (seq: number, format: string) => `snapshot-${String(seq)}-${format}.json`, ); mocks.ensureArtifactsDir.mockResolvedValue('/artifacts'); + mocks.appendArtifactWithRollback.mockImplementation( + async (options: { sessionDir: string; entry: unknown }) => { + await mocks.appendArtifact(options.sessionDir, options.entry); + }, + ); mocks.appendArtifact.mockResolvedValue(undefined); mocks.writeTextFileAtomic.mockResolvedValue(undefined); }); diff --git a/test/unit/screenshot/capture.test.ts b/test/unit/screenshot/capture.test.ts index 816e460..a5bf8e0 100644 --- a/test/unit/screenshot/capture.test.ts +++ b/test/unit/screenshot/capture.test.ts @@ -384,23 +384,21 @@ describe('screenshot capture', () => { expect(manifest.artifacts).toEqual([]); }); - it('preserves the renamed final PNG when manifest append fails after rename', async () => { + it('removes the renamed final PNG when manifest append fails after rename', async () => { const sessionDirectory = await createSessionDir(); const finalFilename = screenshotFilename(5, 'reference-dark'); const finalPath = artifactPath(sessionDirectory, finalFilename); // Pre-write a manifest whose sessionId does not match the directory so - // `appendArtifact` raises a MANIFEST_VALIDATION_ERROR after the temp file - // has already been renamed into place. + // `appendArtifactWithRollback` raises a MANIFEST_VALIDATION_ERROR after + // the temp file has already been renamed into place. const manifestFilePath = artifactPath(sessionDirectory, 'manifest.json'); + const unrelatedManifest = { + version: 1, + sessionId: 'unrelated-session', + artifacts: [], + }; await mkdir(dirname(manifestFilePath), { recursive: true }); - await writeFile( - manifestFilePath, - `${JSON.stringify({ - version: 1, - sessionId: 'unrelated-session', - artifacts: [], - })}\n`, - ); + await writeFile(manifestFilePath, `${JSON.stringify(unrelatedManifest)}\n`); let observedTempPath: string | undefined; const backend = createFakeBackend({ @@ -418,17 +416,17 @@ describe('screenshot capture', () => { }), ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); - // The final PNG survives because cleanup only removes the temp file, - // not the already-renamed artifact. This is intentional: the rename - // succeeded, so the PNG is valid; only the manifest entry is missing. - // Adding rename rollback is not addressed in this refactor; the - // follow-up is tracked in #79. - await expect(access(finalPath)).resolves.toBeUndefined(); + // The temp file has already been renamed, so rollback must remove the + // final artifact path to avoid leaving an unmanifested PNG behind. + await expect(access(finalPath)).rejects.toMatchObject({ code: 'ENOENT' }); if (observedTempPath !== undefined) { await expect(access(observedTempPath)).rejects.toMatchObject({ code: 'ENOENT', }); } + await expect(readFile(manifestFilePath, 'utf8')).resolves.toBe( + `${JSON.stringify(unrelatedManifest)}\n`, + ); }); it('removes the temp file and rethrows when the renderer screenshot rejects', async () => { diff --git a/test/unit/snapshot/capture.test.ts b/test/unit/snapshot/capture.test.ts index 0c9b343..d005555 100644 --- a/test/unit/snapshot/capture.test.ts +++ b/test/unit/snapshot/capture.test.ts @@ -1,4 +1,5 @@ -import { access, mkdir, readFile } from 'node:fs/promises'; +import { access, mkdir, readFile, writeFile } from 'node:fs/promises'; +import { dirname } from 'node:path'; import { describe, expect, it } from 'vitest'; @@ -146,6 +147,42 @@ describe('snapshot capture', () => { }); }); + it('removes the snapshot artifact when manifest append fails after write', async () => { + const sessionDirectory = await createSessionDir(); + const snapshot = createTestSemanticSnapshot(); + const result = createSnapshotResult(snapshot, 'structured'); + const filename = snapshotFilename(5, 'structured'); + const snapshotPath = artifactPath(sessionDirectory, filename); + const manifestFilePath = artifactPath(sessionDirectory, 'manifest.json'); + // Pre-write a manifest whose sessionId does not match the directory so + // `appendArtifactWithRollback` raises a MANIFEST_VALIDATION_ERROR after + // the snapshot artifact has already been written. + const unrelatedManifest = { + version: 1, + sessionId: 'unrelated-session', + artifacts: [], + }; + await mkdir(dirname(manifestFilePath), { recursive: true }); + await writeFile(manifestFilePath, `${JSON.stringify(unrelatedManifest)}\n`); + + await expect( + persistSnapshotArtifact({ + sessionDir: sessionDirectory, + format: 'structured', + snapshot, + result, + rendererBackend: 'test-backend', + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + + await expect(access(snapshotPath)).rejects.toMatchObject({ + code: 'ENOENT', + }); + await expect(readFile(manifestFilePath, 'utf8')).resolves.toBe( + `${JSON.stringify(unrelatedManifest)}\n`, + ); + }); + it('requires renderer backend metadata before writing artifacts', async () => { const sessionDirectory = await createSessionDir(); diff --git a/test/unit/storage/artifactStorage.test.ts b/test/unit/storage/artifactStorage.test.ts index 9f5fa46..aa27093 100644 --- a/test/unit/storage/artifactStorage.test.ts +++ b/test/unit/storage/artifactStorage.test.ts @@ -1,4 +1,4 @@ -import { access, readFile, writeFile } from 'node:fs/promises'; +import { access, mkdir, readFile, writeFile } from 'node:fs/promises'; import { join } from 'node:path'; @@ -11,7 +11,7 @@ import type { ArtifactManifest, } from '../../../src/storage/artifactManifest.js'; import { - appendArtifact, + appendArtifactWithRollback, ArtifactEntrySchema, readArtifactManifest, writeArtifactManifest, @@ -158,9 +158,9 @@ describe('artifact manifest storage', () => { artifacts: [], }); - await appendArtifact( + await appendArtifactWithRollback({ sessionDir, - createArtifactEntry({ + entry: createArtifactEntry({ id: '01JQ0000000000000000000001', kind: 'screenshot', filename: 'screenshot-4-reference-dark.png', @@ -171,7 +171,7 @@ describe('artifact manifest storage', () => { pngSizeBytes: 2048, }, }), - ); + }); await expect(readArtifactManifest(sessionDir)).resolves.toEqual({ version: 1, @@ -208,20 +208,20 @@ describe('artifact manifest storage', () => { ).resolves.toMatch(/\n$/u); }); - it('preserves all entries when many concurrent appendArtifact() calls race for the same session', async () => { + it('preserves all entries when many concurrent appendArtifactWithRollback() calls race for the same session', async () => { const sessionDir = await createSessionDir(); const concurrentAppends = 20; await Promise.all( Array.from({ length: concurrentAppends }, (_value, index) => - appendArtifact( + appendArtifactWithRollback({ sessionDir, - createArtifactEntry({ + entry: createArtifactEntry({ id: `01JQ${String(index).padStart(22, '0')}`, filename: `snapshot-${String(index)}-structured.json`, capturedAtSeq: index, }), - ), + }), ), ); @@ -237,6 +237,173 @@ describe('artifact manifest storage', () => { ); }); + it('removes rollback artifact paths when manifest append fails', async () => { + const sessionDir = await createSessionDir(); + const artifactFile = artifactPath(sessionDir, 'orphan.json'); + + await ensureArtifactsDir(sessionDir); + await writeFile(artifactFile, 'artifact', 'utf8'); + await writeFile( + artifactPath(sessionDir, 'manifest.json'), + `${JSON.stringify({ + version: 1, + sessionId: 'other-session', + artifacts: [], + })}\n`, + 'utf8', + ); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'orphan.json' }), + rollbackArtifactPath: artifactFile, + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + + await expect(access(artifactFile)).rejects.toMatchObject({ + code: 'ENOENT', + }); + }); + + it('removes rollback artifact paths when artifact entry validation fails', async () => { + const sessionDir = await createSessionDir(); + const artifactFile = artifactPath(sessionDir, 'invalid-entry.json'); + + await ensureArtifactsDir(sessionDir); + await writeFile(artifactFile, 'artifact', 'utf8'); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ + filename: 'invalid-entry.json', + sessionId: 'other-session', + }), + rollbackArtifactPath: artifactFile, + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + + await expect(access(artifactFile)).rejects.toMatchObject({ + code: 'ENOENT', + }); + }); + + it('preserves artifact paths when rollback is not requested', async () => { + const sessionDir = await createSessionDir(); + const artifactFile = artifactPath(sessionDir, 'explicit-out.cast'); + + await ensureArtifactsDir(sessionDir); + await writeFile(artifactFile, 'artifact', 'utf8'); + await writeFile( + artifactPath(sessionDir, 'manifest.json'), + `${JSON.stringify({ + version: 1, + sessionId: 'other-session', + artifacts: [], + })}\n`, + 'utf8', + ); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'explicit-out.cast' }), + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + + await expect(access(artifactFile)).resolves.toBeUndefined(); + }); + + it('asserts rollback paths are non-empty and absolute', async () => { + const sessionDir = await createSessionDir(); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry(), + rollbackArtifactPath: '', + }), + ).rejects.toThrow(/rollbackArtifactPath must be a non-empty string/u); + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry(), + rollbackArtifactPath: 'relative-artifact.json', + }), + ).rejects.toThrow(/rollbackArtifactPath must be absolute/u); + }); + + it('does not remove queued rollback paths when an earlier append fails', async () => { + const sessionDir = await createSessionDir(); + const firstArtifact = artifactPath(sessionDir, 'first-orphan.json'); + const secondArtifact = artifactPath(sessionDir, 'second-orphan.json'); + + await ensureArtifactsDir(sessionDir); + await writeFile(firstArtifact, 'first', 'utf8'); + await writeFile(secondArtifact, 'second', 'utf8'); + await writeFile( + artifactPath(sessionDir, 'manifest.json'), + `${JSON.stringify({ + version: 1, + sessionId: 'other-session', + artifacts: [], + })}\n`, + 'utf8', + ); + + // The second append never runs because KeyedSerializer propagates the + // first rejection without calling queued callbacks. + const results = await Promise.allSettled([ + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'first-orphan.json' }), + rollbackArtifactPath: firstArtifact, + }), + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'second-orphan.json' }), + rollbackArtifactPath: secondArtifact, + }), + ]); + + expect(results).toEqual([ + expect.objectContaining({ status: 'rejected' }), + expect.objectContaining({ status: 'rejected' }), + ]); + await expect(access(firstArtifact)).rejects.toMatchObject({ + code: 'ENOENT', + }); + await expect(access(secondArtifact)).resolves.toBeUndefined(); + }); + + it('does not mask the manifest error when rollback removal fails', async () => { + const sessionDir = await createSessionDir(); + const blockedPath = artifactPath(sessionDir, 'blocked-artifact'); + + await ensureArtifactsDir(sessionDir); + await writeFile( + artifactPath(sessionDir, 'manifest.json'), + `${JSON.stringify({ + version: 1, + sessionId: 'other-session', + artifacts: [], + })}\n`, + 'utf8', + ); + await mkdir(blockedPath); + await writeFile(join(blockedPath, 'child'), 'artifact', 'utf8'); + + await expect( + appendArtifactWithRollback({ + sessionDir, + entry: createArtifactEntry({ filename: 'blocked-artifact' }), + rollbackArtifactPath: blockedPath, + }), + ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR' }); + await expect(access(join(blockedPath, 'child'))).resolves.toBeUndefined(); + }); + it('rejects invalid manifest contents and mismatched entries', async () => { const sessionDir = await createSessionDir(); @@ -255,12 +422,12 @@ describe('artifact manifest storage', () => { code: 'MANIFEST_VALIDATION_ERROR', }); await expect( - appendArtifact( + appendArtifactWithRollback({ sessionDir, - createArtifactEntry({ + entry: createArtifactEntry({ sessionId: 'other-session', }), - ), + }), ).rejects.toMatchObject({ code: 'MANIFEST_VALIDATION_ERROR', });