From 536d17dca3d86de66d3fbdb20f2451162442285b Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 11 Aug 2026 12:13:16 -0700 Subject: [PATCH] fix(artifacts): recover committed cloud mutations (#13796) --- .../artifacts/artifact-cloud-recovery.test.ts | 346 ++++++++++++++++++ src/main/artifacts/artifact-cloud-request.ts | 9 +- src/main/artifacts/artifact-cloud-service.ts | 36 +- .../artifact-create-intent-store.test.ts | 319 ++++++++++++++++ .../artifacts/artifact-create-intent-store.ts | 255 +++++++++++++ src/main/artifacts/artifact-publisher.ts | 187 ++++++++-- .../artifact-recovery-directory-fsync.test.ts | 88 +++++ .../artifacts/artifact-share-record-store.ts | 8 +- .../profile-artifact-cloud-cleanup.test.ts | 244 ++++++++++++ .../profile-artifact-cloud-cleanup.ts | 131 +++++++ src/main/orca-profiles/profile-cloud-index.ts | 46 ++- src/main/orca-profiles/profile-index-store.ts | 3 + src/shared/secure-file.ts | 47 ++- 13 files changed, 1671 insertions(+), 48 deletions(-) create mode 100644 src/main/artifacts/artifact-cloud-recovery.test.ts create mode 100644 src/main/artifacts/artifact-create-intent-store.test.ts create mode 100644 src/main/artifacts/artifact-create-intent-store.ts create mode 100644 src/main/artifacts/artifact-recovery-directory-fsync.test.ts create mode 100644 src/main/orca-profiles/profile-artifact-cloud-cleanup.test.ts create mode 100644 src/main/orca-profiles/profile-artifact-cloud-cleanup.ts diff --git a/src/main/artifacts/artifact-cloud-recovery.test.ts b/src/main/artifacts/artifact-cloud-recovery.test.ts new file mode 100644 index 00000000000..f57b53c2b02 --- /dev/null +++ b/src/main/artifacts/artifact-cloud-recovery.test.ts @@ -0,0 +1,346 @@ +import { mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ + app: { isPackaged: false }, + safeStorage: { isEncryptionAvailable: () => false } +})) + +import { ArtifactCloudService } from './artifact-cloud-service' + +const createdPaths: string[] = [] +const apiUrl = 'http://localhost:3000' +const writeRequest = { + sourceKey: '/repo/report.html', + content: '

Recovery

', + contentType: 'text/html' as const, + fileName: 'report.html', + apiUrl, + authToken: 'token-a' +} + +afterEach(async () => { + vi.unstubAllGlobals() + await Promise.all( + createdPaths.splice(0).map((path) => rm(path, { recursive: true, force: true })) + ) +}) + +describe('ArtifactCloudService committed response loss recovery', () => { + it('reconciles one remotely revocable artifact after a committed create loses its response', async () => { + const userDataPath = await createUserDataPath() + const server = new ArtifactFaultServer() + server.loseNextCreateResponse = true + vi.stubGlobal('fetch', server.fetch) + + await expect(service(userDataPath).publish(writeRequest)).rejects.toThrow('response lost') + expect(server.createMutations).toBe(1) + expect(server.artifactSlugs()).toEqual(['artifact-1']) + await expect(publishedLink(userDataPath)).resolves.toBeNull() + + await expect( + service(userDataPath).publish({ ...writeRequest, content: '

Changed after loss

' }) + ).resolves.toMatchObject({ + status: 'ok', + value: { item: { artifact: { slug: 'artifact-1' } } } + }) + expect(server.createMutations).toBe(1) + expect(server.artifactSlugs()).toEqual(['artifact-1']) + expect(server.artifactContent('artifact-1')).toBe('

Changed after loss

') + await expect(publishedLink(userDataPath)).resolves.toBe('https://share.onorca.dev/a/artifact-1') + }) + + it('replays the exact create when content is unchanged after response loss', async () => { + const userDataPath = await createUserDataPath() + const server = new ArtifactFaultServer() + server.loseNextCreateResponse = true + vi.stubGlobal('fetch', server.fetch) + + await expect(service(userDataPath).publish(writeRequest)).rejects.toThrow('response lost') + await expect(service(userDataPath).publish(writeRequest)).resolves.toMatchObject({ + status: 'ok', + value: { item: { artifact: { slug: 'artifact-1' } } } + }) + expect(server.createMutations).toBe(1) + expect(server.artifactContent('artifact-1')).toBe(writeRequest.content) + }) + + it('updates a recovered share when its content changed after response loss', async () => { + const userDataPath = await createUserDataPath() + const server = new ArtifactFaultServer() + server.loseNextCreateResponse = true + vi.stubGlobal('fetch', server.fetch) + + await expect(service(userDataPath).share(writeRequest)).rejects.toThrow('response lost') + await expect( + service(userDataPath).share({ ...writeRequest, content: '

Changed share

' }) + ).resolves.toMatchObject({ + status: 'ok', + value: { artifact: { slug: 'artifact-1' } } + }) + expect(server.createMutations).toBe(1) + expect(server.artifactContent('artifact-1')).toBe('

Changed share

') + }) + + it('retains recovery until a changed-content update succeeds', async () => { + const userDataPath = await createUserDataPath() + const server = new ArtifactFaultServer() + server.loseNextCreateResponse = true + vi.stubGlobal('fetch', server.fetch) + + await expect(service(userDataPath).share(writeRequest)).rejects.toThrow('response lost') + server.rejectNextUpdateStatus = 503 + const changed = { ...writeRequest, content: '

Changed after update failure

' } + await expect(service(userDataPath).share(changed)).rejects.toMatchObject({ statusCode: 503 }) + await expect(service(userDataPath).share(changed)).resolves.toMatchObject({ + status: 'ok', + value: { artifact: { slug: 'artifact-1' } } + }) + expect(server.createMutations).toBe(1) + expect(server.artifactSlugs()).toEqual(['artifact-1']) + expect(server.artifactContent('artifact-1')).toBe(changed.content) + }) + + it('clears the durable mapping when a committed delete retry returns 404', async () => { + const userDataPath = await createUserDataPath() + const server = new ArtifactFaultServer() + vi.stubGlobal('fetch', server.fetch) + await service(userDataPath).publish(writeRequest) + + server.loseNextDeleteResponse = true + await expect( + service(userDataPath).unshare({ + sourceKey: writeRequest.sourceKey, + apiUrl, + authToken: 'token-a' + }) + ).rejects.toThrow('response lost') + expect(server.deleteMutations).toBe(1) + expect(server.artifactSlugs()).toEqual([]) + await expect(publishedLink(userDataPath)).resolves.toBe('https://share.onorca.dev/a/artifact-1') + + await expect( + service(userDataPath).unshare({ + sourceKey: writeRequest.sourceKey, + apiUrl, + authToken: 'token-a' + }) + ).resolves.toEqual({ status: 'ok', value: undefined }) + expect(server.deleteMutations).toBe(1) + expect(server.artifactSlugs()).toEqual([]) + await expect(publishedLink(userDataPath)).resolves.toBeNull() + }) + + it('keeps the durable mapping when a delete receives an unrelated 404', async () => { + const userDataPath = await createUserDataPath() + const server = new ArtifactFaultServer() + vi.stubGlobal('fetch', server.fetch) + await service(userDataPath).publish(writeRequest) + + server.rejectNextDeleteCode = 'not_found' + await expect( + service(userDataPath).unshare({ + sourceKey: writeRequest.sourceKey, + apiUrl, + authToken: 'token-a' + }) + ).rejects.toMatchObject({ statusCode: 404, errorCode: 'not_found' }) + expect(server.deleteMutations).toBe(0) + expect(server.artifactSlugs()).toEqual(['artifact-1']) + await expect(publishedLink(userDataPath)).resolves.toBe('https://share.onorca.dev/a/artifact-1') + }) + + it('drops an uncommitted validation failure so corrected content can create', async () => { + const userDataPath = await createUserDataPath() + const server = new ArtifactFaultServer() + server.rejectNextCreateStatus = 422 + vi.stubGlobal('fetch', server.fetch) + + await expect(service(userDataPath).publish(writeRequest)).rejects.toMatchObject({ + statusCode: 422 + }) + await expect( + service(userDataPath).publish({ ...writeRequest, content: '

Corrected

' }) + ).resolves.toMatchObject({ + status: 'ok', + value: { item: { artifact: { slug: 'artifact-1' } } } + }) + expect(server.createMutations).toBe(1) + expect(server.artifactContent('artifact-1')).toBe('

Corrected

') + }) + + it('keeps a replay intent when validation changes after the original commit', async () => { + const userDataPath = await createUserDataPath() + const server = new ArtifactFaultServer() + server.loseNextCreateResponse = true + vi.stubGlobal('fetch', server.fetch) + + await expect(service(userDataPath).publish(writeRequest)).rejects.toThrow('response lost') + server.rejectNextCreateStatus = 422 + const changed = { ...writeRequest, content: '

Changed after validation

' } + await expect(service(userDataPath).publish(changed)).rejects.toMatchObject({ statusCode: 422 }) + await expect(service(userDataPath).publish(changed)).resolves.toMatchObject({ + status: 'ok', + value: { item: { artifact: { slug: 'artifact-1' } } } + }) + expect(server.createMutations).toBe(1) + expect(server.artifactContent('artifact-1')).toBe(changed.content) + }) +}) + +class ArtifactFaultServer { + readonly fetch = vi.fn(this.handle.bind(this)) + createMutations = 0 + deleteMutations = 0 + loseNextCreateResponse = false + loseNextDeleteResponse = false + rejectNextCreateStatus: number | null = null + rejectNextDeleteCode: string | null = null + rejectNextUpdateStatus: number | null = null + private readonly artifacts = new Map() + private readonly createsByKey = new Map() + + artifactSlugs(): string[] { + return [...this.artifacts.keys()].sort() + } + + artifactContent(slug: string): string | undefined { + const body = this.artifacts.get(slug) + return body ? (JSON.parse(body) as { content?: string }).content : undefined + } + + private async handle(input: string | URL | Request, init?: RequestInit): Promise { + const method = init?.method ?? 'GET' + if (method === 'POST') { + return this.create(init) + } + if (method === 'PUT') { + return this.update(String(input), init) + } + if (method === 'DELETE') { + return this.delete(String(input)) + } + throw new Error(`Unexpected artifact request: ${method} ${String(input)}`) + } + + private create(init?: RequestInit): Response { + if (this.rejectNextCreateStatus !== null) { + const status = this.rejectNextCreateStatus + this.rejectNextCreateStatus = null + return jsonResponse({ code: 'artifact_validation_failed' }, status) + } + const key = new Headers(init?.headers).get('idempotency-key') + if (new Headers(init?.headers).get('authorization') !== 'Bearer token-a') { + throw new Error('Missing artifact authorization') + } + if (!key) { + throw new Error('Missing idempotency key') + } + const body = String(init?.body) + const existing = this.createsByKey.get(key) + if (existing) { + if (existing.body !== body) { + throw new Error('Idempotency key reused with another request') + } + return jsonResponse(existing.response, 201) + } + + this.createMutations += 1 + const slug = `artifact-${this.createMutations}` + const response = createResponseBody(slug) + this.createsByKey.set(key, { body, response }) + this.artifacts.set(slug, body) + if (this.loseNextCreateResponse) { + this.loseNextCreateResponse = false + throw new TypeError('response lost after committed create') + } + return jsonResponse(response, 201) + } + + private delete(url: string): Response { + if (this.rejectNextDeleteCode !== null) { + const code = this.rejectNextDeleteCode + this.rejectNextDeleteCode = null + return jsonResponse({ code }, 404) + } + const slug = decodeURIComponent(url.slice(url.lastIndexOf('/') + 1)) + if (!this.artifacts.delete(slug)) { + return jsonResponse({ code: 'artifact_not_found' }, 404) + } + this.deleteMutations += 1 + if (this.loseNextDeleteResponse) { + this.loseNextDeleteResponse = false + throw new TypeError('response lost after committed delete') + } + return new Response(null, { status: 204 }) + } + + private update(url: string, init?: RequestInit): Response { + if (this.rejectNextUpdateStatus !== null) { + const status = this.rejectNextUpdateStatus + this.rejectNextUpdateStatus = null + return jsonResponse({ code: 'artifact_update_failed' }, status) + } + const slug = decodeURIComponent(url.slice(url.lastIndexOf('/') + 1)) + if (!this.artifacts.has(slug)) { + return jsonResponse({ code: 'artifact_not_found' }, 404) + } + const headers = new Headers(init?.headers) + if ( + headers.get('authorization') !== 'Bearer token-a' || + headers.get('x-orca-edit-token') !== `edit-${slug}` + ) { + return jsonResponse({ code: 'artifact_forbidden' }, 403) + } + this.artifacts.set(slug, String(init?.body)) + return jsonResponse(createResponseBody(slug), 200) + } +} + +async function createUserDataPath(): Promise { + const path = await mkdtemp(join(tmpdir(), 'orca-artifact-recovery-')) + createdPaths.push(path) + return path +} + +function service(userDataPath: string): ArtifactCloudService { + return new ArtifactCloudService(userDataPath, () => true) +} + +async function publishedLink(userDataPath: string): Promise { + const result = await service(userDataPath).getPublishedLink({ + sourceKey: writeRequest.sourceKey, + apiUrl, + authToken: 'token-a' + }) + return result.status === 'ok' ? (result.value?.shareUrl ?? null) : null +} + +function jsonResponse(body: object, status: number): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' } + }) +} + +function createResponseBody(slug: string): object { + return { + artifact: { + version: 1, + slug, + title: null, + originalFileName: 'report.html', + sourceContentType: 'text/html', + renderedContentType: 'text/html', + createdAt: '2026-08-06T00:00:00.000Z', + updatedAt: '2026-08-06T00:00:00.000Z', + expiresAt: '2026-09-06T00:00:00.000Z', + byteSize: 17, + deletedAt: null + }, + shareUrl: `https://share.onorca.dev/a/${slug}`, + editToken: `edit-${slug}` + } +} diff --git a/src/main/artifacts/artifact-cloud-request.ts b/src/main/artifacts/artifact-cloud-request.ts index 6f1e0bb0f7b..4ad4633dac9 100644 --- a/src/main/artifacts/artifact-cloud-request.ts +++ b/src/main/artifacts/artifact-cloud-request.ts @@ -1,7 +1,14 @@ import type { ArtifactWriteRequest } from '../../shared/artifacts' import { OrcaCloudRequestError } from '../orca-profiles/profile-cloud-client' -export function artifactWriteBody(request: ArtifactWriteRequest): Record { +export type ArtifactWriteBody = { + content: string + contentType: ArtifactWriteRequest['contentType'] + fileName: string + title?: string +} + +export function artifactWriteBody(request: ArtifactWriteRequest): ArtifactWriteBody { return { content: request.content, contentType: request.contentType, diff --git a/src/main/artifacts/artifact-cloud-service.ts b/src/main/artifacts/artifact-cloud-service.ts index 6abb0525e9c..83b9836f940 100644 --- a/src/main/artifacts/artifact-cloud-service.ts +++ b/src/main/artifacts/artifact-cloud-service.ts @@ -12,6 +12,7 @@ import type { import { assertArtifactSharingAllowed } from '../../shared/artifact-sharing-gate' import { ensureActiveOrcaProfile } from '../orca-profiles/profile-index-store' import { getOrcaCloudAuthConfig } from '../orca-profiles/profile-cloud-auth-config' +import { prepareArtifactCloudUse } from '../orca-profiles/profile-artifact-cloud-cleanup' import { runWithFreshOrcaCloudSession } from '../orca-profiles/profile-cloud-session-refresh' import { allowsArtifactCloudAuthOverride, @@ -28,6 +29,7 @@ import { import type { ActiveOrcaProfileState } from '../orca-profiles/profile-index-store' import { artifactRequest, artifactWriteBody } from './artifact-cloud-request' import { ArtifactPublisher } from './artifact-publisher' +import { OrcaCloudRequestError } from '../orca-profiles/profile-cloud-client' type ArtifactAuthContext = { profileId: string @@ -35,6 +37,28 @@ type ArtifactAuthContext = { assertCurrent: () => void } +async function deleteArtifactRequest( + apiUrl: string, + token: string, + path: string, + editToken?: string +): Promise { + try { + await artifactRequest(apiUrl, token, path, { + method: 'DELETE', + ...(editToken ? { editToken } : {}) + }) + } catch (error) { + if ( + !(error instanceof OrcaCloudRequestError) || + error.statusCode !== 404 || + error.errorCode !== 'artifact_not_found' + ) { + throw error + } + } +} + function tokenFingerprint(token: string): string { return createHash('sha256').update(token).digest('hex') } @@ -227,10 +251,8 @@ export class ArtifactCloudService { } return this.publisher.runForSlug(record.slug, auth, async () => { auth.assertCurrent() - await artifactRequest(apiUrl, token, `/${record.slug}`, { - method: 'DELETE', - editToken: record.editToken - }) + await deleteArtifactRequest(apiUrl, token, `/${record.slug}`, record.editToken) + auth.assertCurrent() removeArtifactShareRecords(auth.profileId, this.userDataPath, auth.scope, { sourceKey: request.sourceKey, slug: record.slug @@ -244,9 +266,8 @@ export class ArtifactCloudService { return this.withAuth(options, (token, apiUrl, auth) => this.publisher.runForSlug(id, auth, async () => { auth.assertCurrent() - await artifactRequest(apiUrl, token, `/${encodeURIComponent(id)}`, { - method: 'DELETE' - }) + await deleteArtifactRequest(apiUrl, token, `/${encodeURIComponent(id)}`) + auth.assertCurrent() removeArtifactShareRecords(auth.profileId, this.userDataPath, auth.scope, { slug: id }) }) ) @@ -258,6 +279,7 @@ export class ArtifactCloudService { ): Promise> { const apiUrl = resolveArtifactCloudApiUrl(options.apiUrl) const active = ensureActiveOrcaProfile(this.userDataPath) + prepareArtifactCloudUse(active.profile, this.userDataPath) if (options.authToken?.trim()) { if (!allowsArtifactCloudAuthOverride()) { throw new Error( diff --git a/src/main/artifacts/artifact-create-intent-store.test.ts b/src/main/artifacts/artifact-create-intent-store.test.ts new file mode 100644 index 00000000000..dfcbe003df7 --- /dev/null +++ b/src/main/artifacts/artifact-create-intent-store.test.ts @@ -0,0 +1,319 @@ +import { execFileSync } from 'node:child_process' +import { mkdtemp, readFile, readdir, rm, stat, truncate, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { ARTIFACT_CLI_MAX_RPC_BYTES, artifactWriteRequestByteLength } from '../../shared/artifacts' +import { + MAX_ARTIFACT_CREATE_INTENT_BYTES, + MAX_PENDING_ARTIFACT_CREATES, + clearArtifactCreateIntents, + getArtifactCreateIntent, + getOrCreateArtifactCreateIntent, + removeArtifactCreateIntent +} from './artifact-create-intent-store' +import type { ArtifactShareScope } from './artifact-share-record-store' + +vi.mock('node:child_process', () => ({ execFile: vi.fn(), execFileSync: vi.fn() })) + +const createdPaths: string[] = [] +const scope: ArtifactShareScope = { + cloudUserId: 'user-a', + cloudProfileId: 'cloud-a', + cloudOrganizationId: 'org-a', + apiOrigin: 'https://share.onorca.dev' +} +const body = { + content: '

Original

', + contentType: 'text/html' as const, + fileName: 'report.html', + title: 'Original' +} + +afterEach(async () => { + await Promise.all( + createdPaths.splice(0).map((path) => rm(path, { recursive: true, force: true })) + ) +}) + +describe('artifact create intent store', () => { + it('retains the first key and exact request until the matching create completes', async () => { + const userDataPath = await createUserDataPath() + const sourceKey = String.raw`C:\repo\report.html` + const first = getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + sourceKey, + scope, + 'key-a', + body + ) + const retry = getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + sourceKey, + scope, + 'key-b', + { ...body, content: '

Changed

' } + ) + + expect(first).toEqual(retry) + expect(retry).toMatchObject({ idempotencyKey: 'key-a', body }) + removeArtifactCreateIntent('local-profile', userDataPath, sourceKey, scope, 'key-b') + expect(getArtifactCreateIntent('local-profile', userDataPath, sourceKey, scope)).not.toBeNull() + removeArtifactCreateIntent('local-profile', userDataPath, sourceKey, scope, 'key-a') + expect(getArtifactCreateIntent('local-profile', userDataPath, sourceKey, scope)).toBeNull() + }) + + it.each([ + ['user', { cloudUserId: 'user-b' }], + ['profile', { cloudProfileId: 'cloud-b' }], + ['organization', { cloudOrganizationId: 'org-b' }], + ['API origin', { apiOrigin: 'http://localhost:3000' }] + ])('isolates recovery intent by %s', async (_name, changedScope) => { + const userDataPath = await createUserDataPath() + const sourceKey = '/repo/report.html' + getOrCreateArtifactCreateIntent('local-profile', userDataPath, sourceKey, scope, 'key-a', body) + + expect( + getArtifactCreateIntent('local-profile', userDataPath, sourceKey, { + ...scope, + ...changedScope + }) + ).toBeNull() + }) + + it('bounds unresolved payload storage without dropping an existing intent', async () => { + const userDataPath = await createUserDataPath() + for (let index = 0; index < MAX_PENDING_ARTIFACT_CREATES; index += 1) { + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + `/repo/report-${index}.html`, + scope, + `key-${index}`, + body + ) + } + + expect(() => + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/overflow.html', + scope, + 'overflow-key', + body + ) + ).toThrow(/waiting for recovery/) + expect( + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report-0.html', + scope, + 'replacement-key', + { ...body, content: 'replacement' } + ).idempotencyKey + ).toBe('key-0') + }) + + it('clears pending content at the profile lifecycle boundary', async () => { + const userDataPath = await createUserDataPath() + const directory = join(userDataPath, 'profiles', 'local-profile', 'artifact-create-intents') + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-a', + body + ) + + clearArtifactCreateIntents('local-profile', userDataPath) + + expect( + getArtifactCreateIntent('local-profile', userDataPath, '/repo/report.html', scope) + ).toBeNull() + expect(await readdir(directory)).toEqual([]) + }) + + it('removes crash-left temporary writes before admitting another intent', async () => { + const userDataPath = await createUserDataPath() + const directory = join(userDataPath, 'profiles', 'local-profile', 'artifact-create-intents') + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-a', + body + ) + removeArtifactCreateIntent('local-profile', userDataPath, '/repo/report.html', scope, 'key-a') + await writeFile(join(directory, 'crash-left.tmp'), 'partial') + + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/other.html', + scope, + 'key-b', + body + ) + + expect((await readdir(directory)).some((name) => name.endsWith('.tmp'))).toBe(false) + }) + + it('hardens one Windows journal directory without per-file PowerShell launches', async () => { + const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + vi.mocked(execFileSync).mockImplementation((file) => + String(file).endsWith('whoami.exe') ? '"USER","S-1-5-21-1000"' : '' + ) + try { + const userDataPath = await createUserDataPath() + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-a', + body + ) + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/other.html', + scope, + 'key-b', + body + ) + + const powershellCalls = vi + .mocked(execFileSync) + .mock.calls.filter(([file]) => String(file).endsWith('powershell.exe')) + expect(powershellCalls).toHaveLength(1) + expect((powershellCalls[0]![1] as string[]).at(-1)).toBe('1') + } finally { + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform) + } + vi.mocked(execFileSync).mockReset() + } + }) + + it('refuses to overwrite an unreadable matching intent', async () => { + const userDataPath = await createUserDataPath() + const directory = join(userDataPath, 'profiles', 'local-profile', 'artifact-create-intents') + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-a', + body + ) + const [fileName] = await readdir(directory) + await writeFile(join(directory, fileName), '{broken-json') + + expect(() => + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-b', + body + ) + ).toThrow(/could not be read safely/) + }) + + it('removes an unreadable intent after its mutation completes', async () => { + const userDataPath = await createUserDataPath() + const directory = join(userDataPath, 'profiles', 'local-profile', 'artifact-create-intents') + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-a', + body + ) + const [fileName] = await readdir(directory) + await writeFile(join(directory, fileName), '{broken-json') + + expect(() => + removeArtifactCreateIntent('local-profile', userDataPath, '/repo/report.html', scope, 'key-a') + ).not.toThrow() + expect(await readdir(directory)).toEqual([]) + }) + + it('rejects a persisted content type outside the artifact allowlist', async () => { + const userDataPath = await createUserDataPath() + const directory = join(userDataPath, 'profiles', 'local-profile', 'artifact-create-intents') + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-a', + body + ) + const [fileName] = await readdir(directory) + const path = join(directory, fileName) + const intent = JSON.parse(await readFile(path, 'utf8')) as { body: { contentType: string } } + intent.body.contentType = 'application/octet-stream' + await writeFile(path, JSON.stringify(intent)) + + expect(() => + getArtifactCreateIntent('local-profile', userDataPath, '/repo/report.html', scope) + ).toThrow(/unsupported format/) + }) + + it('persists a valid artifact request near the RPC limit', async () => { + const userDataPath = await createUserDataPath() + const nearLimitBody = { ...body, content: 'x'.repeat(ARTIFACT_CLI_MAX_RPC_BYTES - 200) } + expect( + artifactWriteRequestByteLength({ sourceKey: '/repo/report.html', ...nearLimitBody }) + ).toBeLessThanOrEqual(ARTIFACT_CLI_MAX_RPC_BYTES) + + expect(() => + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-a', + nearLimitBody + ) + ).not.toThrow() + const directory = join(userDataPath, 'profiles', 'local-profile', 'artifact-create-intents') + const [fileName] = await readdir(directory) + expect((await stat(join(directory, fileName))).size).toBeGreaterThan(ARTIFACT_CLI_MAX_RPC_BYTES) + }) + + it('rejects an oversized recovery record before reading it', async () => { + const userDataPath = await createUserDataPath() + const directory = join(userDataPath, 'profiles', 'local-profile', 'artifact-create-intents') + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + scope, + 'key-a', + body + ) + const [fileName] = await readdir(directory) + await truncate(join(directory, fileName), MAX_ARTIFACT_CREATE_INTENT_BYTES + 1) + + expect(() => + getArtifactCreateIntent('local-profile', userDataPath, '/repo/report.html', scope) + ).toThrow(/exceeds the supported size/) + }) +}) + +async function createUserDataPath(): Promise { + const path = await mkdtemp(join(tmpdir(), 'orca-artifact-create-intents-')) + createdPaths.push(path) + return path +} diff --git a/src/main/artifacts/artifact-create-intent-store.ts b/src/main/artifacts/artifact-create-intent-store.ts new file mode 100644 index 00000000000..d544d04ceec --- /dev/null +++ b/src/main/artifacts/artifact-create-intent-store.ts @@ -0,0 +1,255 @@ +import { createHash, randomBytes } from 'node:crypto' +import { + existsSync, + mkdirSync, + readFileSync, + readdirSync, + renameSync, + rmSync, + statSync, + writeFileSync +} from 'node:fs' +import { join } from 'node:path' +import { ARTIFACT_CLI_MAX_RPC_BYTES } from '../../shared/artifacts' +import { + bestEffortFsyncDirectorySync, + fsyncFileSync, + hardenSecurePath +} from '../../shared/secure-file' +import { getOrcaProfileDirectory } from '../orca-profiles/profile-storage-paths' +import type { ArtifactWriteBody } from './artifact-cloud-request' +import type { ArtifactShareScope } from './artifact-share-record-store' + +export const MAX_PENDING_ARTIFACT_CREATES = 32 +export const MAX_ARTIFACT_CREATE_INTENT_BYTES = ARTIFACT_CLI_MAX_RPC_BYTES + 128 * 1024 + +const MAX_HARDENED_INTENT_DIRECTORIES = 64 +const hardenedIntentDirectories = new Set() + +export type ArtifactCreateIntent = { + version: 1 + sourceKey: string + scope: ArtifactShareScope + idempotencyKey: string + body: ArtifactWriteBody +} + +function intentDirectory(profileId: string, userDataPath: string): string { + return join(getOrcaProfileDirectory(profileId, userDataPath), 'artifact-create-intents') +} + +function ensureIntentDirectory(profileId: string, userDataPath: string): string { + const directory = intentDirectory(profileId, userDataPath) + mkdirSync(directory, { recursive: true, mode: 0o700 }) + if (!hardenedIntentDirectories.has(directory)) { + hardenSecurePath(directory, { + isDirectory: true, + platform: process.platform, + sync: true + }) + if (hardenedIntentDirectories.size >= MAX_HARDENED_INTENT_DIRECTORIES) { + const oldest = hardenedIntentDirectories.values().next().value + if (oldest !== undefined) { + hardenedIntentDirectories.delete(oldest) + } + } + hardenedIntentDirectories.add(directory) + } + return directory +} + +function removeTemporaryIntents(directory: string): void { + for (const name of readdirSync(directory)) { + if (name.endsWith('.tmp')) { + rmSync(join(directory, name), { force: true }) + } + } +} + +function writeIntent(path: string, directory: string, serializedIntent: string): void { + // Child files inherit the once-hardened directory ACL, avoiding per-create PowerShell launches. + const temporaryPath = `${path}.${process.pid}.${Date.now()}.${randomBytes(4).toString('hex')}.tmp` + try { + writeFileSync(temporaryPath, serializedIntent, { + encoding: 'utf8', + mode: 0o600 + }) + fsyncFileSync(temporaryPath) + renameSync(temporaryPath, path) + bestEffortFsyncDirectorySync(directory) + } catch (error) { + rmSync(temporaryPath, { force: true }) + throw error + } +} + +function intentPath( + profileId: string, + userDataPath: string, + sourceKey: string, + scope: ArtifactShareScope +): string { + const identity = JSON.stringify([ + sourceKey, + scope.cloudUserId, + scope.cloudProfileId, + scope.cloudOrganizationId, + scope.apiOrigin + ]) + const fileName = `${createHash('sha256').update(identity).digest('hex')}.json` + return join(intentDirectory(profileId, userDataPath), fileName) +} + +function scopeMatches(left: ArtifactShareScope, right: ArtifactShareScope): boolean { + return ( + left.cloudUserId === right.cloudUserId && + left.cloudProfileId === right.cloudProfileId && + left.cloudOrganizationId === right.cloudOrganizationId && + left.apiOrigin === right.apiOrigin + ) +} + +function isWriteBody(value: unknown): value is ArtifactWriteBody { + if (!value || typeof value !== 'object') { + return false + } + const body = value as Partial + return ( + typeof body.content === 'string' && + (body.contentType === 'text/html' || body.contentType === 'text/markdown') && + typeof body.fileName === 'string' && + (body.title === undefined || typeof body.title === 'string') + ) +} + +function isScope(value: unknown): value is ArtifactShareScope { + if (!value || typeof value !== 'object') { + return false + } + const scope = value as Partial + return [ + scope.cloudUserId, + scope.cloudProfileId, + scope.cloudOrganizationId, + scope.apiOrigin + ].every((field) => typeof field === 'string') +} + +function readIntent(path: string): ArtifactCreateIntent { + let size: number + try { + size = statSync(path).size + } catch (error) { + throw new Error('Artifact create recovery record could not be read safely.', { cause: error }) + } + if (size > MAX_ARTIFACT_CREATE_INTENT_BYTES) { + throw new Error('Artifact create recovery record exceeds the supported size.') + } + let parsed: unknown + try { + parsed = JSON.parse(readFileSync(path, 'utf8')) + } catch (error) { + throw new Error('Artifact create recovery record could not be read safely.', { cause: error }) + } + if (!parsed || typeof parsed !== 'object') { + throw new Error('Artifact create recovery record has an unsupported format.') + } + const intent = parsed as Partial + if ( + intent.version !== 1 || + typeof intent.sourceKey !== 'string' || + typeof intent.idempotencyKey !== 'string' || + !intent.idempotencyKey || + !isScope(intent.scope) || + !isWriteBody(intent.body) + ) { + throw new Error('Artifact create recovery record has an unsupported format.') + } + return intent as ArtifactCreateIntent +} + +export function getArtifactCreateIntent( + profileId: string, + userDataPath: string, + sourceKey: string, + scope: ArtifactShareScope +): ArtifactCreateIntent | null { + const path = intentPath(profileId, userDataPath, sourceKey, scope) + if (!existsSync(path)) { + return null + } + const intent = readIntent(path) + if (intent.sourceKey !== sourceKey || !scopeMatches(intent.scope, scope)) { + throw new Error('Artifact create recovery record does not match its storage identity.') + } + return intent +} + +export function getOrCreateArtifactCreateIntent( + profileId: string, + userDataPath: string, + sourceKey: string, + scope: ArtifactShareScope, + idempotencyKey: string, + body: ArtifactWriteBody +): ArtifactCreateIntent { + const existing = getArtifactCreateIntent(profileId, userDataPath, sourceKey, scope) + if (existing) { + return existing + } + const directory = ensureIntentDirectory(profileId, userDataPath) + removeTemporaryIntents(directory) + const pendingCount = readdirSync(directory).filter((name) => name.endsWith('.json')).length + if (pendingCount >= MAX_PENDING_ARTIFACT_CREATES) { + throw new Error('Too many artifact creates are waiting for recovery. Retry an earlier share.') + } + const intent: ArtifactCreateIntent = { + version: 1, + sourceKey, + scope, + idempotencyKey, + body + } + const serializedIntent = JSON.stringify(intent, null, 2) + if (Buffer.byteLength(serializedIntent, 'utf8') > MAX_ARTIFACT_CREATE_INTENT_BYTES) { + throw new Error('Artifact create recovery record exceeds the supported size.') + } + writeIntent(intentPath(profileId, userDataPath, sourceKey, scope), directory, serializedIntent) + return intent +} + +export function removeArtifactCreateIntent( + profileId: string, + userDataPath: string, + sourceKey: string, + scope: ArtifactShareScope, + expectedIdempotencyKey: string +): void { + const path = intentPath(profileId, userDataPath, sourceKey, scope) + if (!existsSync(path)) { + return + } + let matches = true + try { + matches = readIntent(path).idempotencyKey === expectedIdempotencyKey + } catch { + // An unreadable record cannot be replayed, so a completed mutation may discard it safely. + } + if (matches) { + rmSync(path, { force: true }) + bestEffortFsyncDirectorySync(intentDirectory(profileId, userDataPath)) + } +} + +export function clearArtifactCreateIntents(profileId: string, userDataPath: string): void { + const directory = intentDirectory(profileId, userDataPath) + if (!existsSync(directory)) { + return + } + for (const name of readdirSync(directory)) { + if (name.endsWith('.json') || name.endsWith('.tmp')) { + rmSync(join(directory, name), { force: true }) + } + } + bestEffortFsyncDirectorySync(directory) +} diff --git a/src/main/artifacts/artifact-publisher.ts b/src/main/artifacts/artifact-publisher.ts index 8f03c4e5b68..d70958e6b6f 100644 --- a/src/main/artifacts/artifact-publisher.ts +++ b/src/main/artifacts/artifact-publisher.ts @@ -4,7 +4,17 @@ import type { ArtifactWriteRequest } from '../../shared/artifacts' import { OrcaCloudRequestError } from '../orca-profiles/profile-cloud-client' -import { artifactRequest, artifactWriteBody } from './artifact-cloud-request' +import { + artifactRequest, + type ArtifactWriteBody, + artifactWriteBody +} from './artifact-cloud-request' +import { + type ArtifactCreateIntent, + getArtifactCreateIntent, + getOrCreateArtifactCreateIntent, + removeArtifactCreateIntent +} from './artifact-create-intent-store' import { type ArtifactShareScope, getArtifactShareRecord, @@ -15,6 +25,30 @@ import { type ArtifactCreateResponse = ArtifactListItem & { editToken: string } +type ArtifactCreateOutcome = { + editToken: string + intent: ArtifactCreateIntent + result: ArtifactPublishResult +} + +function artifactWriteBodiesMatch(left: ArtifactWriteBody, right: ArtifactWriteBody): boolean { + return ( + left.content === right.content && + left.contentType === right.contentType && + left.fileName === right.fileName && + left.title === right.title + ) +} + +function discardsCreateIntent(error: unknown): boolean { + return ( + error instanceof OrcaCloudRequestError && + error.statusCode >= 400 && + error.statusCode < 500 && + ![408, 409, 425, 429].includes(error.statusCode) + ) +} + type ArtifactPublishAuthContext = { profileId: string scope: ArtifactShareScope @@ -51,7 +85,23 @@ export class ArtifactPublisher { ): Promise { return this.runForSource(request.sourceKey, auth, async () => { auth.assertCurrent() - return (await this.create(request, token, apiUrl, auth, idempotencyKey)).item + const pending = getArtifactCreateIntent( + auth.profileId, + this.userDataPath, + request.sourceKey, + auth.scope + ) + const created = await this.create(request, token, apiUrl, auth, idempotencyKey, pending) + if (pending && !artifactWriteBodiesMatch(pending.body, artifactWriteBody(request))) { + const item = await this.updateExisting(request, token, apiUrl, auth, { + slug: created.result.item.artifact.slug, + editToken: created.editToken + }) + this.removeCreateIntent(request, auth, created.intent) + return item + } + this.removeCreateIntent(request, auth, created.intent) + return created.result.item }) } @@ -64,6 +114,25 @@ export class ArtifactPublisher { ): Promise { return this.runForSource(request.sourceKey, auth, async () => { auth.assertCurrent() + const pending = getArtifactCreateIntent( + auth.profileId, + this.userDataPath, + request.sourceKey, + auth.scope + ) + if (pending) { + const created = await this.create(request, token, apiUrl, auth, idempotencyKey, pending) + if (artifactWriteBodiesMatch(pending.body, artifactWriteBody(request))) { + this.removeCreateIntent(request, auth, created.intent) + return created.result + } + const item = await this.updateExisting(request, token, apiUrl, auth, { + slug: created.result.item.artifact.slug, + editToken: created.editToken + }) + this.removeCreateIntent(request, auth, created.intent) + return { change: 'created', item } + } const record = getArtifactShareRecord( auth.profileId, this.userDataPath, @@ -72,24 +141,8 @@ export class ArtifactPublisher { ) if (record) { try { - return await this.runForSlug(record.slug, auth, async () => { - auth.assertCurrent() - const item = await artifactRequest(apiUrl, token, `/${record.slug}`, { - method: 'PUT', - editToken: record.editToken, - body: artifactWriteBody(request) - }) - auth.assertCurrent() - refreshArtifactShareRecordExpiration( - auth.profileId, - this.userDataPath, - request.sourceKey, - auth.scope, - record, - item.artifact.expiresAt - ) - return { change: 'updated', item } - }) + const item = await this.updateExisting(request, token, apiUrl, auth, record) + return { change: 'updated', item } } catch (error) { if (!(error instanceof OrcaCloudRequestError) || error.statusCode !== 404) { throw error @@ -101,7 +154,9 @@ export class ArtifactPublisher { }) } } - return this.create(request, token, apiUrl, auth, idempotencyKey) + const created = await this.create(request, token, apiUrl, auth, idempotencyKey, null) + this.removeCreateIntent(request, auth, created.intent) + return created.result }) } @@ -121,18 +176,72 @@ export class ArtifactPublisher { return this.runSerialized(artifactOperationQueueKey('slug', auth, slug), operation) } + private updateExisting( + request: ArtifactWriteRequest, + token: string, + apiUrl: string, + auth: ArtifactPublishAuthContext, + record: { slug: string; editToken: string } + ): Promise { + return this.runForSlug(record.slug, auth, async () => { + auth.assertCurrent() + const item = await artifactRequest(apiUrl, token, `/${record.slug}`, { + method: 'PUT', + editToken: record.editToken, + body: artifactWriteBody(request) + }) + auth.assertCurrent() + refreshArtifactShareRecordExpiration( + auth.profileId, + this.userDataPath, + request.sourceKey, + auth.scope, + record, + item.artifact.expiresAt + ) + return item + }) + } + private async create( request: ArtifactWriteRequest, token: string, apiUrl: string, auth: ArtifactPublishAuthContext, - idempotencyKey: string - ): Promise { - const response = await artifactRequest(apiUrl, token, '', { - method: 'POST', - body: artifactWriteBody(request), - idempotencyKey - }) + idempotencyKey: string, + pending: ArtifactCreateIntent | null + ): Promise { + const replaying = pending !== null + const intent = + pending ?? + getOrCreateArtifactCreateIntent( + auth.profileId, + this.userDataPath, + request.sourceKey, + auth.scope, + idempotencyKey, + artifactWriteBody(request) + ) + let response: ArtifactCreateResponse + try { + response = await artifactRequest(apiUrl, token, '', { + method: 'POST', + body: intent.body, + idempotencyKey: intent.idempotencyKey + }) + } catch (error) { + // A replay may outlive its server receipt, so never abandon a possibly committed artifact. + if (!replaying && discardsCreateIntent(error)) { + removeArtifactCreateIntent( + auth.profileId, + this.userDataPath, + request.sourceKey, + auth.scope, + intent.idempotencyKey + ) + } + throw error + } auth.assertCurrent() saveArtifactShareRecord(auth.profileId, this.userDataPath, request.sourceKey, { slug: response.artifact.slug, @@ -142,11 +251,29 @@ export class ArtifactPublisher { ...auth.scope }) return { - change: 'created', - item: { artifact: response.artifact, shareUrl: response.shareUrl } + editToken: response.editToken, + intent, + result: { + change: 'created', + item: { artifact: response.artifact, shareUrl: response.shareUrl } + } } } + private removeCreateIntent( + request: ArtifactWriteRequest, + auth: ArtifactPublishAuthContext, + intent: ArtifactCreateIntent + ): void { + removeArtifactCreateIntent( + auth.profileId, + this.userDataPath, + request.sourceKey, + auth.scope, + intent.idempotencyKey + ) + } + private async runSerialized(key: string, operation: () => Promise): Promise { const previous = this.queues.get(key) ?? Promise.resolve() let release = (): void => {} diff --git a/src/main/artifacts/artifact-recovery-directory-fsync.test.ts b/src/main/artifacts/artifact-recovery-directory-fsync.test.ts new file mode 100644 index 00000000000..c815d91560d --- /dev/null +++ b/src/main/artifacts/artifact-recovery-directory-fsync.test.ts @@ -0,0 +1,88 @@ +import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import type * as NodeFs from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, expect, it, vi } from 'vitest' + +const fsyncMockState = vi.hoisted(() => ({ + directoryDescriptor: -1, + directoryErrorCode: 'EINVAL' +})) + +vi.mock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + closeSync: (descriptor: number) => { + if (descriptor !== fsyncMockState.directoryDescriptor) { + actual.closeSync(descriptor) + } + }, + fsyncSync: (descriptor: number) => { + if (descriptor === fsyncMockState.directoryDescriptor) { + throw Object.assign(new Error('directory fsync failed'), { + code: fsyncMockState.directoryErrorCode + }) + } + return actual.fsyncSync(descriptor) + }, + openSync: (path: string, flags: string | number) => + actual.statSync(path).isDirectory() + ? fsyncMockState.directoryDescriptor + : actual.openSync(path, flags) + } +}) + +import { bestEffortFsyncDirectorySync, writeDurableSecureJsonFile } from '../../shared/secure-file' +import { + clearArtifactCreateIntents, + getOrCreateArtifactCreateIntent +} from './artifact-create-intent-store' + +const createdPaths: string[] = [] + +afterEach(() => { + fsyncMockState.directoryErrorCode = 'EINVAL' + for (const path of createdPaths.splice(0)) { + rmSync(path, { recursive: true, force: true }) + } +}) + +it('skips directory fsync on Windows and propagates I/O failures elsewhere', () => { + const directory = mkdtempSync(join(tmpdir(), 'orca-artifact-directory-fsync-eio-')) + createdPaths.push(directory) + fsyncMockState.directoryErrorCode = 'EIO' + + const fsyncDirectory = (): void => bestEffortFsyncDirectorySync(directory) + if (process.platform === 'win32') { + expect(fsyncDirectory).not.toThrow() + } else { + expect(fsyncDirectory).toThrow('directory fsync failed') + } +}) + +it('keeps durable artifact records usable when directory fsync is unsupported', () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-artifact-directory-fsync-')) + createdPaths.push(userDataPath) + const recordPath = join(userDataPath, 'artifact-shares.json') + + expect(() => writeDurableSecureJsonFile(recordPath, { ok: true })).not.toThrow() + expect(JSON.parse(readFileSync(recordPath, 'utf8'))).toEqual({ ok: true }) + + expect(() => + getOrCreateArtifactCreateIntent( + 'local-profile', + userDataPath, + '/repo/report.html', + { + cloudUserId: 'user-a', + cloudProfileId: 'profile-a', + cloudOrganizationId: 'org-a', + apiOrigin: 'https://share.onorca.dev' + }, + 'key-a', + { content: 'hello', contentType: 'text/markdown', fileName: 'report.md' } + ) + ).not.toThrow() + expect(() => clearArtifactCreateIntents('local-profile', userDataPath)).not.toThrow() +}) diff --git a/src/main/artifacts/artifact-share-record-store.ts b/src/main/artifacts/artifact-share-record-store.ts index 174856eaee4..a16ec19333c 100644 --- a/src/main/artifacts/artifact-share-record-store.ts +++ b/src/main/artifacts/artifact-share-record-store.ts @@ -1,7 +1,7 @@ import { randomUUID } from 'node:crypto' import { existsSync, readFileSync } from 'node:fs' import { join } from 'node:path' -import { writeSecureJsonFile } from '../../shared/secure-file' +import { writeDurableSecureJsonFile, writeSecureJsonFile } from '../../shared/secure-file' import { getOrcaProfileDirectory } from '../orca-profiles/profile-storage-paths' export type ArtifactShareScope = { @@ -183,7 +183,7 @@ export function saveArtifactShareRecord( const records = readRecords(profileId, userDataPath) records.shares[sourceKey] = { ...record, savedAt: Date.now() } records.shares = pruneRecords(records.shares, Date.now()).shares - writeSecureJsonFile(recordPath(profileId, userDataPath), records) + writeDurableSecureJsonFile(recordPath(profileId, userDataPath), records) } export function refreshArtifactShareRecordExpiration( @@ -228,7 +228,7 @@ export function removeArtifactShareRecords( delete records.shares[sourceKey] } } - writeSecureJsonFile(recordPath(profileId, userDataPath), records) + writeDurableSecureJsonFile(recordPath(profileId, userDataPath), records) } export function clearArtifactShareRecords(profileId: string, userDataPath: string): void { @@ -238,7 +238,7 @@ export function clearArtifactShareRecords(profileId: string, userDataPath: strin } catch { // Clearing must recover sign-out from an unreadable token index. } - writeSecureJsonFile(recordPath(profileId, userDataPath), { + writeDurableSecureJsonFile(recordPath(profileId, userDataPath), { version: 2, lifecycleGeneration: lifecycleGeneration + 1, lifecycleNonce: randomUUID(), diff --git a/src/main/orca-profiles/profile-artifact-cloud-cleanup.test.ts b/src/main/orca-profiles/profile-artifact-cloud-cleanup.test.ts new file mode 100644 index 00000000000..0624124c385 --- /dev/null +++ b/src/main/orca-profiles/profile-artifact-cloud-cleanup.test.ts @@ -0,0 +1,244 @@ +import { mkdirSync } from 'node:fs' +import { mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { OrcaProfileCloudSummary } from '../../shared/orca-profiles' +import type * as ArtifactCreateIntentStore from '../artifacts/artifact-create-intent-store' +import type * as ProfileArtifactCloudCleanup from './profile-artifact-cloud-cleanup' +import type * as ProfileIndexStore from './profile-index-store' + +vi.mock('../artifacts/artifact-create-intent-store', async () => { + const actual = await vi.importActual( + '../artifacts/artifact-create-intent-store' + ) + return { ...actual, clearArtifactCreateIntents: vi.fn(actual.clearArtifactCreateIntents) } +}) + +vi.mock('./profile-index-store', async () => { + const actual = await vi.importActual('./profile-index-store') + return { ...actual, writeProfileIndex: vi.fn(actual.writeProfileIndex) } +}) + +vi.mock('./profile-artifact-cloud-cleanup', async () => { + const actual = await vi.importActual( + './profile-artifact-cloud-cleanup' + ) + return { + ...actual, + commitArtifactCloudCleanup: vi.fn(actual.commitArtifactCloudCleanup) + } +}) + +import { + clearArtifactCreateIntents, + getArtifactCreateIntent, + getOrCreateArtifactCreateIntent +} from '../artifacts/artifact-create-intent-store' +import type { ArtifactShareScope } from '../artifacts/artifact-share-record-store' +import { + artifactCloudCleanupNeedsCommit, + commitArtifactCloudCleanup, + prepareArtifactCloudCleanup, + prepareArtifactCloudUse +} from './profile-artifact-cloud-cleanup' +import { linkOrcaProfileToCloud, unlinkOrcaProfileFromCloud } from './profile-cloud-index' +import { + getOrcaProfileIndexPath, + getOrcaProfileDirectory, + loadOrCreateProfileIndex, + readProfileIndex, + writeProfileIndex +} from './profile-index-store' + +const createdPaths: string[] = [] +const profileId = 'local-default' + +afterEach(async () => { + vi.clearAllMocks() + await Promise.all( + createdPaths.splice(0).map((path) => rm(path, { recursive: true, force: true })) + ) +}) + +describe('profile artifact cloud cleanup', () => { + it('preserves recovery state when the profile write fails', async () => { + const userDataPath = await createLinkedProfile(cloud('org-a')) + const scope = shareScope('org-a') + createIntent(userDataPath, scope) + vi.mocked(writeProfileIndex).mockImplementationOnce(() => { + throw new Error('profile write failed') + }) + + expect(() => linkOrcaProfileToCloud(profileId, cloud('org-b'), userDataPath)).toThrow( + 'profile write failed' + ) + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).not.toBeNull() + + linkOrcaProfileToCloud(profileId, cloud('org-b'), userDataPath) + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).toBeNull() + }) + + it('retries cleanup after the profile transition commits', async () => { + const userDataPath = await createLinkedProfile(cloud('org-a')) + const scope = shareScope('org-a') + createIntent(userDataPath, scope) + vi.mocked(clearArtifactCreateIntents).mockImplementationOnce(() => { + throw new Error('cleanup failed') + }) + + expect(() => linkOrcaProfileToCloud(profileId, cloud('org-b'), userDataPath)).toThrow( + 'cleanup failed' + ) + expect(currentCloud(userDataPath)?.activeOrgId).toBe('org-b') + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).not.toBeNull() + + linkOrcaProfileToCloud(profileId, cloud('org-b'), userDataPath) + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).toBeNull() + }) + + it('reconciles an interrupted transition before linking again', async () => { + const userDataPath = await createLinkedProfile(cloud('org-a')) + const scope = shareScope('org-a') + createIntent(userDataPath, scope) + interruptNextCleanupCommit() + + expect(() => linkOrcaProfileToCloud(profileId, cloud('org-b'), userDataPath)).toThrow( + 'cleanup commit interrupted' + ) + expect(currentCloud(userDataPath)?.activeOrgId).toBe('org-b') + vi.mocked(writeProfileIndex).mockClear() + vi.mocked(clearArtifactCreateIntents).mockImplementationOnce(() => { + throw new Error('cleanup failed') + }) + + expect(() => linkOrcaProfileToCloud(profileId, cloud('org-c'), userDataPath)).toThrow( + 'cleanup failed' + ) + expect(writeProfileIndex).not.toHaveBeenCalled() + expect(currentCloud(userDataPath)?.activeOrgId).toBe('org-b') + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).not.toBeNull() + + linkOrcaProfileToCloud(profileId, cloud('org-c'), userDataPath) + expect(currentCloud(userDataPath)?.activeOrgId).toBe('org-c') + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).toBeNull() + }) + + it('reconciles an interrupted transition before unlinking', async () => { + const userDataPath = await createLinkedProfile(cloud('org-a')) + const scope = shareScope('org-a') + createIntent(userDataPath, scope) + interruptNextCleanupCommit() + + expect(() => linkOrcaProfileToCloud(profileId, cloud('org-b'), userDataPath)).toThrow( + 'cleanup commit interrupted' + ) + expect(currentCloud(userDataPath)?.activeOrgId).toBe('org-b') + vi.mocked(writeProfileIndex).mockClear() + vi.mocked(clearArtifactCreateIntents).mockImplementationOnce(() => { + throw new Error('cleanup failed') + }) + + expect(() => unlinkOrcaProfileFromCloud(profileId, userDataPath)).toThrow('cleanup failed') + expect(writeProfileIndex).not.toHaveBeenCalled() + expect(currentCloud(userDataPath)?.activeOrgId).toBe('org-b') + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).not.toBeNull() + + unlinkOrcaProfileFromCloud(profileId, userDataPath) + expect(currentCloud(userDataPath)).toBeUndefined() + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).toBeNull() + }) + + it('preserves an orphaned local marker for an unknown profile', async () => { + const userDataPath = await createLinkedProfile(cloud('org-a')) + const orphanProfileId = 'missing-profile' + const scope = shareScope('org-a') + mkdirSync(getOrcaProfileDirectory(orphanProfileId, userDataPath), { recursive: true }) + createIntent(userDataPath, scope, orphanProfileId) + prepareArtifactCloudCleanup(orphanProfileId, userDataPath, undefined) + + const transitions = [ + () => linkOrcaProfileToCloud(orphanProfileId, cloud('org-b'), userDataPath), + () => unlinkOrcaProfileFromCloud(orphanProfileId, userDataPath) + ] + for (const transition of transitions) { + expect(transition).toThrow('unknown_orca_profile') + expect(artifactCloudCleanupNeedsCommit(orphanProfileId, userDataPath, undefined)).toBe(true) + expect( + getArtifactCreateIntent(orphanProfileId, userDataPath, '/report.md', scope) + ).not.toBeNull() + } + }) + + it('cleans old recovery state when the active organization changes', async () => { + const userDataPath = await createLinkedProfile(cloud('org-a')) + const scope = shareScope('org-a') + createIntent(userDataPath, scope) + + linkOrcaProfileToCloud(profileId, cloud('org-b'), userDataPath) + + expect(getArtifactCreateIntent(profileId, userDataPath, '/report.md', scope)).toBeNull() + }) + + it('blocks artifact use until a visible transition is durably committed', async () => { + const cloudSummary = cloud('org-a') + const userDataPath = await createLinkedProfile(cloudSummary) + prepareArtifactCloudCleanup(profileId, userDataPath, cloudSummary) + + expect(() => + prepareArtifactCloudUse({ id: profileId, cloud: cloudSummary }, userDataPath) + ).toThrow(/transition must be retried/) + }) +}) + +function cloud(activeOrgId: string): OrcaProfileCloudSummary { + return { + cloudProfileId: 'cloud-profile-a', + userId: 'user-a', + email: 'user@example.com', + activeOrgId, + linkedAt: 1 + } +} + +function shareScope(cloudOrganizationId: string): ArtifactShareScope { + return { + cloudUserId: 'user-a', + cloudProfileId: 'cloud-profile-a', + cloudOrganizationId, + apiOrigin: 'https://share.onorca.dev' + } +} + +async function createLinkedProfile(cloudSummary: OrcaProfileCloudSummary): Promise { + const userDataPath = await mkdtemp(join(tmpdir(), 'orca-profile-artifact-cleanup-')) + createdPaths.push(userDataPath) + loadOrCreateProfileIndex(userDataPath) + linkOrcaProfileToCloud(profileId, cloudSummary, userDataPath) + vi.clearAllMocks() + return userDataPath +} + +function createIntent( + userDataPath: string, + scope: ArtifactShareScope, + targetProfileId = profileId +): void { + getOrCreateArtifactCreateIntent(targetProfileId, userDataPath, '/report.md', scope, 'key-a', { + content: '# report', + contentType: 'text/markdown', + fileName: 'report.md' + }) +} + +function currentCloud(userDataPath: string): OrcaProfileCloudSummary | undefined { + return readProfileIndex(getOrcaProfileIndexPath(userDataPath))?.profiles.find( + (profile) => profile.id === profileId + )?.cloud +} + +function interruptNextCleanupCommit(): void { + vi.mocked(commitArtifactCloudCleanup).mockImplementationOnce(() => { + throw new Error('cleanup commit interrupted') + }) +} diff --git a/src/main/orca-profiles/profile-artifact-cloud-cleanup.ts b/src/main/orca-profiles/profile-artifact-cloud-cleanup.ts new file mode 100644 index 00000000000..88e90471914 --- /dev/null +++ b/src/main/orca-profiles/profile-artifact-cloud-cleanup.ts @@ -0,0 +1,131 @@ +import { existsSync, readFileSync, rmSync } from 'node:fs' +import { join } from 'node:path' +import type { OrcaProfileCloudSummary, OrcaProfileSummary } from '../../shared/orca-profiles' +import { bestEffortFsyncDirectorySync, writeDurableSecureJsonFile } from '../../shared/secure-file' +import { clearArtifactCreateIntents } from '../artifacts/artifact-create-intent-store' +import { clearArtifactShareRecords } from '../artifacts/artifact-share-record-store' +import { getOrcaProfileDirectory } from './profile-storage-paths' + +type ArtifactCloudCleanupMarker = { + version: 1 + phase: 'prepared' | 'committed' + targetIdentity: string +} + +function cleanupMarkerPath(profileId: string, userDataPath: string): string { + return join(getOrcaProfileDirectory(profileId, userDataPath), 'artifact-cloud-cleanup.json') +} + +export function artifactCloudIdentity(cloud: OrcaProfileCloudSummary | undefined): string { + return cloud + ? JSON.stringify([cloud.userId, cloud.cloudProfileId, cloud.activeOrgId ?? '']) + : 'local' +} + +export function prepareArtifactCloudCleanup( + profileId: string, + userDataPath: string, + targetCloud: OrcaProfileCloudSummary | undefined +): void { + writeDurableSecureJsonFile(cleanupMarkerPath(profileId, userDataPath), { + version: 1, + phase: 'prepared', + targetIdentity: artifactCloudIdentity(targetCloud) + } satisfies ArtifactCloudCleanupMarker) +} + +function readCleanupMarker( + profileId: string, + userDataPath: string +): ArtifactCloudCleanupMarker | null { + const path = cleanupMarkerPath(profileId, userDataPath) + if (!existsSync(path)) { + return null + } + let parsed: unknown + try { + parsed = JSON.parse(readFileSync(path, 'utf8')) + } catch (error) { + throw new Error('Artifact cloud cleanup marker could not be read safely.', { cause: error }) + } + if ( + !parsed || + typeof parsed !== 'object' || + (parsed as Partial).version !== 1 || + !['prepared', 'committed'].includes( + (parsed as Partial).phase ?? '' + ) || + typeof (parsed as Partial).targetIdentity !== 'string' + ) { + throw new Error('Artifact cloud cleanup marker has an unsupported format.') + } + return parsed as ArtifactCloudCleanupMarker +} + +export function artifactCloudCleanupNeedsCommit( + profileId: string, + userDataPath: string, + targetCloud: OrcaProfileCloudSummary | undefined +): boolean { + const marker = readCleanupMarker(profileId, userDataPath) + return ( + marker?.phase === 'prepared' && marker.targetIdentity === artifactCloudIdentity(targetCloud) + ) +} + +export function commitArtifactCloudCleanup( + profileId: string, + userDataPath: string, + targetCloud: OrcaProfileCloudSummary | undefined +): void { + const targetIdentity = artifactCloudIdentity(targetCloud) + const marker = readCleanupMarker(profileId, userDataPath) + if (marker?.phase !== 'prepared' || marker.targetIdentity !== targetIdentity) { + throw new Error('Artifact cloud cleanup marker does not match the profile transition.') + } + writeDurableSecureJsonFile(cleanupMarkerPath(profileId, userDataPath), { + version: 1, + phase: 'committed', + targetIdentity + } satisfies ArtifactCloudCleanupMarker) +} + +export function completeArtifactCloudCleanupIfCommitted( + profileId: string, + userDataPath: string, + currentCloud: OrcaProfileCloudSummary | undefined +): void { + const marker = readCleanupMarker(profileId, userDataPath) + if ( + marker?.phase !== 'committed' || + marker.targetIdentity !== artifactCloudIdentity(currentCloud) + ) { + return + } + clearArtifactCreateIntents(profileId, userDataPath) + clearArtifactShareRecords(profileId, userDataPath) + rmSync(cleanupMarkerPath(profileId, userDataPath), { force: true }) + bestEffortFsyncDirectorySync(getOrcaProfileDirectory(profileId, userDataPath)) +} + +function assertArtifactCloudCleanupReady( + profileId: string, + userDataPath: string, + currentCloud: OrcaProfileCloudSummary | undefined +): void { + const marker = readCleanupMarker(profileId, userDataPath) + if ( + marker?.phase === 'prepared' && + marker.targetIdentity === artifactCloudIdentity(currentCloud) + ) { + throw new Error('The Orca profile transition must be retried before publishing artifacts.') + } +} + +export function prepareArtifactCloudUse( + profile: Pick, + userDataPath: string +): void { + completeArtifactCloudCleanupIfCommitted(profile.id, userDataPath, profile.cloud) + assertArtifactCloudCleanupReady(profile.id, userDataPath, profile.cloud) +} diff --git a/src/main/orca-profiles/profile-cloud-index.ts b/src/main/orca-profiles/profile-cloud-index.ts index 7186ddd2a63..ccf7e70adb9 100644 --- a/src/main/orca-profiles/profile-cloud-index.ts +++ b/src/main/orca-profiles/profile-cloud-index.ts @@ -11,7 +11,12 @@ import { loadOrCreateProfileIndex, writeProfileIndex } from './profile-index-store' -import { clearArtifactShareRecords } from '../artifacts/artifact-share-record-store' +import { + artifactCloudCleanupNeedsCommit, + commitArtifactCloudCleanup, + completeArtifactCloudCleanupIfCommitted, + prepareArtifactCloudCleanup +} from './profile-artifact-cloud-cleanup' export type CreateCloudLinkedOrcaProfileRecordResult = OrcaProfileListState & { profile: OrcaProfileSummary @@ -50,6 +55,19 @@ function toLocalProfile(profile: OrcaProfileSummary, now: number): OrcaProfileSu } } +function reconcileCurrentArtifactCloudCleanup( + profileId: string, + userDataPath: string, + currentCloud: OrcaProfileCloudSummary | undefined +): void { + completeArtifactCloudCleanupIfCommitted(profileId, userDataPath, currentCloud) + if (!artifactCloudCleanupNeedsCommit(profileId, userDataPath, currentCloud)) { + return + } + commitArtifactCloudCleanup(profileId, userDataPath, currentCloud) + completeArtifactCloudCleanupIfCommitted(profileId, userDataPath, currentCloud) +} + export function createCloudLinkedOrcaProfileRecord( cloud: OrcaProfileCloudSummary, args: { name?: string }, @@ -92,6 +110,12 @@ export function linkOrcaProfileToCloud( userDataPath: string ): OrcaProfileListState { const index = loadOrCreateProfileIndex(userDataPath) + const currentProfile = index.profiles.find((profile) => profile.id === profileId) + if (!currentProfile) { + throw new Error('unknown_orca_profile') + } + reconcileCurrentArtifactCloudCleanup(profileId, userDataPath, currentProfile.cloud) + const cleanupNeedsCommit = artifactCloudCleanupNeedsCommit(profileId, userDataPath, cloud) const now = Date.now() let found = false let cloudIdentityChanged = false @@ -103,21 +127,26 @@ export function linkOrcaProfileToCloud( cloudIdentityChanged = Boolean( profile.cloud && (profile.cloud.userId !== cloud.userId || - profile.cloud.cloudProfileId !== cloud.cloudProfileId) + profile.cloud.cloudProfileId !== cloud.cloudProfileId || + (profile.cloud.activeOrgId ?? '') !== (cloud.activeOrgId ?? '')) ) return toCloudLinkedProfile(profile, cloud, now) }) if (!found) { throw new Error('unknown_orca_profile') } - if (cloudIdentityChanged) { - clearArtifactShareRecords(profileId, userDataPath) + if (cloudIdentityChanged || cleanupNeedsCommit) { + prepareArtifactCloudCleanup(profileId, userDataPath, cloud) } const nextIndex = { ...index, profiles } writeProfileIndex(getOrcaProfileIndexPath(userDataPath), nextIndex) + if (cloudIdentityChanged || cleanupNeedsCommit) { + commitArtifactCloudCleanup(profileId, userDataPath, cloud) + completeArtifactCloudCleanupIfCommitted(profileId, userDataPath, cloud) + } return { activeProfileId: nextIndex.activeProfileId, profiles: nextIndex.profiles @@ -129,6 +158,11 @@ export function unlinkOrcaProfileFromCloud( userDataPath: string ): OrcaProfileListState { const index = loadOrCreateProfileIndex(userDataPath) + const currentProfile = index.profiles.find((profile) => profile.id === profileId) + if (!currentProfile) { + throw new Error('unknown_orca_profile') + } + reconcileCurrentArtifactCloudCleanup(profileId, userDataPath, currentProfile.cloud) const now = Date.now() let found = false const profiles = index.profiles.map((profile) => { @@ -141,12 +175,14 @@ export function unlinkOrcaProfileFromCloud( if (!found) { throw new Error('unknown_orca_profile') } - clearArtifactShareRecords(profileId, userDataPath) + prepareArtifactCloudCleanup(profileId, userDataPath, undefined) const nextIndex = { ...index, profiles } writeProfileIndex(getOrcaProfileIndexPath(userDataPath), nextIndex) + commitArtifactCloudCleanup(profileId, userDataPath, undefined) + completeArtifactCloudCleanupIfCommitted(profileId, userDataPath, undefined) return { activeProfileId: nextIndex.activeProfileId, profiles: nextIndex.profiles diff --git a/src/main/orca-profiles/profile-index-store.ts b/src/main/orca-profiles/profile-index-store.ts index 903e7a85ff8..8c1b4490f45 100644 --- a/src/main/orca-profiles/profile-index-store.ts +++ b/src/main/orca-profiles/profile-index-store.ts @@ -8,6 +8,7 @@ import { } from 'node:fs' import { randomUUID } from 'node:crypto' import { dirname } from 'node:path' +import { bestEffortFsyncDirectorySync, fsyncFileSync } from '../../shared/secure-file' import type { GlobalSettings } from '../../shared/types' import { createDefaultLocalOrcaProfile, @@ -130,7 +131,9 @@ export function writeProfileIndex(indexPath: string, index: OrcaProfileIndex): v } const tmpPath = `${indexPath}.tmp` writeFileSync(tmpPath, JSON.stringify(index, null, 2), 'utf-8') + fsyncFileSync(tmpPath) renameSync(tmpPath, indexPath) + bestEffortFsyncDirectorySync(dirname(indexPath)) } function copyIfPresent(source: string, target: string): void { diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index 36231de6835..e3c38c70e09 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -1,8 +1,11 @@ import { randomBytes } from 'node:crypto' import { chmodSync, + closeSync, existsSync, + fsyncSync, mkdirSync, + openSync, renameSync, rmSync, statSync, @@ -40,6 +43,8 @@ const DEFAULT_HARDENING_CACHE_BOUNDS: SecurePathHardeningCacheBounds = { maxTotalKeyBytes: SECURE_PATH_HARDENING_CACHE_KEYS_MAX_BYTES } +const UNSUPPORTED_DIRECTORY_FSYNC_CODES = new Set(['EINVAL', 'ENOTSUP', 'EOPNOTSUPP']) + // Why: PowerShell hardening (~1-1.5s) stalls the main thread, so cache idempotent re-hardens per process. let hardenedPathsThisProcess = new SecurePathHardeningCache( DEFAULT_HARDENING_CACHE_BOUNDS @@ -87,7 +92,15 @@ export function writeSecureJsonFile(targetPath: string, value: unknown): void { writeSecureFile(targetPath, JSON.stringify(value, null, 2)) } -export function writeSecureFile(targetPath: string, contents: string): void { +export function writeDurableSecureJsonFile(targetPath: string, value: unknown): void { + writeSecureFile(targetPath, JSON.stringify(value, null, 2), { durable: true }) +} + +export function writeSecureFile( + targetPath: string, + contents: string, + options: { durable?: boolean } = {} +): void { const dir = dirname(targetPath) if (!existsSync(dir)) { mkdirSync(dir, { recursive: true, mode: 0o700 }) @@ -101,6 +114,9 @@ export function writeSecureFile(targetPath: string, contents: string): void { encoding: 'utf-8', mode: 0o600 }) + if (options.durable) { + fsyncFileSync(tmpFile) + } // Why: writeFileSync mode is a no-op on Windows, so restrict the credential's ACL synchronously before the rename publishes it under inherited ACLs. applySecurePathRestriction(tmpFile, false, process.platform, true) renameSync(tmpFile, targetPath) @@ -108,12 +124,41 @@ export function writeSecureFile(targetPath: string, contents: string): void { if (applySecurePathRestriction(targetPath, false, process.platform, true)) { rememberHardenedPath(targetPath, false) } + if (options.durable) { + bestEffortFsyncDirectorySync(dir) + } } catch (error) { rmSync(tmpFile, { force: true }) throw error } } +export function fsyncFileSync(path: string): void { + const descriptor = openSync(path, 'r') + try { + fsyncSync(descriptor) + } finally { + closeSync(descriptor) + } +} + +export function bestEffortFsyncDirectorySync(directory: string): void { + if (process.platform === 'win32') { + return + } + try { + fsyncFileSync(directory) + } catch (error) { + if ( + error instanceof Error && + UNSUPPORTED_DIRECTORY_FSYNC_CODES.has((error as NodeJS.ErrnoException).code ?? '') + ) { + return + } + throw error + } +} + export function hardenExistingSecureFile(targetPath: string): void { const dir = dirname(targetPath) if (existsSync(dir)) {