diff --git a/.changeset/docs-cache-atomic-writes.md b/.changeset/docs-cache-atomic-writes.md new file mode 100644 index 000000000..e766bdf63 --- /dev/null +++ b/.changeset/docs-cache-atomic-writes.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme-check-docs-updater': patch +--- + +Update the docs cache as a whole. Each update downloads into a new folder and swaps it in, so a failed download keeps the old docs and is retried on the next run, and files removed upstream no longer linger. diff --git a/packages/theme-check-docs-updater/src/themeLiquidDocsDownloader.spec.ts b/packages/theme-check-docs-updater/src/themeLiquidDocsDownloader.spec.ts new file mode 100644 index 000000000..69683880d --- /dev/null +++ b/packages/theme-check-docs-updater/src/themeLiquidDocsDownloader.spec.ts @@ -0,0 +1,69 @@ +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { replaceThemeLiquidDocs } from './themeLiquidDocsDownloader'; + +describe('Unit: replaceThemeLiquidDocs', () => { + let dir: string; + let source: string; + let cache: string; + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'theme-liquid-docs-')); + source = path.join(dir, 'source'); + cache = path.join(dir, 'cache'); + vi.stubEnv('SHOPIFY_TLD_ROOT', source); + const manifest = '{"schemas":[{"uri":"theme/section.json"}]}'; + await write(source, { + 'data/latest.json': '{"revision":"2"}', + 'data/filters.json': '[]', + 'data/objects.json': '[]', + 'data/tags.json': '[]', + 'data/shopify_system_translations.json': '{}', + 'schemas/manifest_theme.json': manifest, + 'schemas/manifest_theme_app_extension.json': manifest, + 'schemas/theme/section.json': '{}', + }); + await write(cache, { 'latest.json': '{"revision":"1"}', 'removed.json': '{}' }); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + await fs.rm(dir, { recursive: true, force: true }); + }); + + it('replaces the docs with the complete new set', async () => { + await replaceThemeLiquidDocs(cache, () => {}); + + expect((await fs.readdir(cache)).sort()).toEqual([ + 'filters.json', + 'latest.json', + 'manifest_theme.json', + 'manifest_theme_app_extension.json', + 'objects.json', + 'section.json', + 'shopify_system_translations.json', + 'tags.json', + ]); + expect(await fs.readFile(path.join(cache, 'latest.json'), 'utf8')).toBe('{"revision":"2"}'); + expect((await fs.readdir(dir)).sort()).toEqual(['cache', 'source']); + }); + + it('leaves the docs as they were when a download fails', async () => { + await fs.rm(path.join(source, 'data/tags.json')); + + await expect(replaceThemeLiquidDocs(cache, () => {})).rejects.toThrow(); + + expect((await fs.readdir(cache)).sort()).toEqual(['latest.json', 'removed.json']); + expect(await fs.readFile(path.join(cache, 'latest.json'), 'utf8')).toBe('{"revision":"1"}'); + expect((await fs.readdir(dir)).sort()).toEqual(['cache', 'source']); + }); +}); + +async function write(root: string, files: Record) { + for (const [file, text] of Object.entries(files)) { + await fs.mkdir(path.dirname(path.join(root, file)), { recursive: true }); + await fs.writeFile(path.join(root, file), text); + } +} diff --git a/packages/theme-check-docs-updater/src/themeLiquidDocsDownloader.ts b/packages/theme-check-docs-updater/src/themeLiquidDocsDownloader.ts index 39cbf13ac..5bb3d30a5 100644 --- a/packages/theme-check-docs-updater/src/themeLiquidDocsDownloader.ts +++ b/packages/theme-check-docs-updater/src/themeLiquidDocsDownloader.ts @@ -72,6 +72,7 @@ export async function download(path: string, log: Logger) { }); } else { const res = await fetch(path); + if (!res.ok) throw new Error(`${res.status} ${res.statusText}: ${path}`); return res.text(); } } @@ -169,6 +170,29 @@ export async function downloadThemeLiquidDocs(destination: string, log: Logger) ); } +/** + * Downloads the docs into a new folder, then swaps it in for destination. So + * destination always holds one complete set: a failed download leaves it as it + * was, and files removed upstream don't linger. + */ +export async function replaceThemeLiquidDocs(destination: string, log: Logger) { + const tmp = `${destination}.${process.pid}.${Math.random().toString(36).slice(2)}`; + const staged = `${tmp}.tmp`; + const old = `${tmp}.old`; + try { + await downloadThemeLiquidDocs(staged, log); + // ponytail: two renames, not one atomic swap. Readers in between fall back to the bundled docs. + await fs.rename(destination, old).catch((error) => { + if (error.code !== 'ENOENT') throw error; + }); + await fs.rename(staged, destination); + } finally { + // Downloads still in flight after a failure may write into staged while it's removed. + await fs.rm(staged, { recursive: true, force: true, maxRetries: 3 }); + await fs.rm(old, { recursive: true, force: true, maxRetries: 3 }); + } +} + function unique(array: T[]): T[] { return [...new Set(array).values()]; } diff --git a/packages/theme-check-docs-updater/src/themeLiquidDocsManager.spec.ts b/packages/theme-check-docs-updater/src/themeLiquidDocsManager.spec.ts index cd628c833..4d3bca908 100644 --- a/packages/theme-check-docs-updater/src/themeLiquidDocsManager.spec.ts +++ b/packages/theme-check-docs-updater/src/themeLiquidDocsManager.spec.ts @@ -1,13 +1,14 @@ import { expect, describe, it, beforeEach, afterEach, vi, assert } from 'vitest'; import { ThemeLiquidDocsManager } from './themeLiquidDocsManager'; -import { downloadResource, Resources } from './themeLiquidDocsDownloader'; +import { download, replaceThemeLiquidDocs, resourceUrl } from './themeLiquidDocsDownloader'; import { noop } from './utils'; vi.mock('./themeLiquidDocsDownloader', async (importOriginal) => { const actual: any = await importOriginal(); return { ...actual, - downloadResource: vi.fn(), + download: vi.fn().mockResolvedValue('{"revision": "1"}'), + replaceThemeLiquidDocs: vi.fn(), }; }); @@ -59,16 +60,18 @@ describe('Module: ThemeLiquidDocsManager', async () => { it('should not download remote files if the revision is stable', async () => { await Promise.all([manager.filters(), manager.objects(), manager.tags()]); - expect(vi.mocked(downloadResource)).toHaveBeenNthCalledWith( - 1, - 'latest', + expect(vi.mocked(download)).toHaveBeenCalledOnce(); + expect(vi.mocked(download)).toHaveBeenCalledWith(resourceUrl('latest'), noop); + expect(vi.mocked(replaceThemeLiquidDocs)).not.toHaveBeenCalled(); + }); + + it('should replace the local docs if the revision changed', async () => { + vi.mocked(download).mockResolvedValueOnce('{"revision": "2"}'); + await manager.filters(); + expect(vi.mocked(replaceThemeLiquidDocs)).toHaveBeenCalledWith( 'MOCKED_CACHE/theme-liquid-docs', noop, ); - expect(vi.mocked(downloadResource)).toHaveBeenCalledTimes(1); - for (const resource of Resources) { - expect(vi.mocked(downloadResource)).not.toHaveBeenCalledWith(resource, expect.any(String)); - } }); describe('Unit: filters', () => { diff --git a/packages/theme-check-docs-updater/src/themeLiquidDocsManager.ts b/packages/theme-check-docs-updater/src/themeLiquidDocsManager.ts index b565941ec..f4ff1ae7c 100644 --- a/packages/theme-check-docs-updater/src/themeLiquidDocsManager.ts +++ b/packages/theme-check-docs-updater/src/themeLiquidDocsManager.ts @@ -14,10 +14,11 @@ import { Manifests, Resource, ThemeLiquidDocsSchemaRoot, - downloadResource, - downloadThemeLiquidDocs, + download, exists, + replaceThemeLiquidDocs, resourcePath, + resourceUrl, root, schemaPath, } from './themeLiquidDocsDownloader'; @@ -86,7 +87,9 @@ export class ThemeLiquidDocsManager implements ThemeDocset, JsonValidationSet { * Shopify/theme-liquid-docs. If there's a diff in revision, it means * that the documentations that you have locally are out of date. * - * The setup method then downloads the other files. + * The setup method then replaces the local docs with the latest ones, which + * include latest.json. Until that succeeds, latest.json keeps the old revision, + * so a failed update is retried on the next run. */ setup = memo(async (): Promise => { try { @@ -95,10 +98,9 @@ export class ThemeLiquidDocsManager implements ThemeDocset, JsonValidationSet { } const local = await this.latestRevision(); - await downloadResource('latest', root, this.log); - const remote = await this.latestRevision(); + const remote = JSON.parse(await download(resourceUrl('latest'), this.log)).revision ?? ''; if (local !== remote) { - await downloadThemeLiquidDocs(root, this.log); + await replaceThemeLiquidDocs(root, this.log); } } catch (error) { if (error instanceof Error) { diff --git a/vitest.config.mjs b/vitest.config.mjs index c5ab771fe..e873d6399 100644 --- a/vitest.config.mjs +++ b/vitest.config.mjs @@ -15,6 +15,7 @@ const isolatedTests = [ 'packages/release-orchestrator/src/steps/getPackageJsonRecord.spec.ts', 'packages/theme-check-common/src/checks/asset-size-css/index.spec.ts', 'packages/theme-check-common/src/checks/asset-size-javascript/index.spec.ts', + 'packages/theme-check-docs-updater/src/themeLiquidDocsManager.spec.ts', 'packages/theme-language-server-common/src/completions/providers/ContentForParameterCompletionProvider.spec.ts', 'packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts', ];