From 8e9ecf0a69a9ced97a76df65ef529b410640e88f Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Wed, 11 Dec 2024 10:55:08 +0000 Subject: [PATCH 1/5] fix: use atomic writes for data store file operations --- .changeset/tame-bags-remember.md | 5 ++++ .../astro/src/content/mutable-data-store.ts | 24 +++++++++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) create mode 100644 .changeset/tame-bags-remember.md diff --git a/.changeset/tame-bags-remember.md b/.changeset/tame-bags-remember.md new file mode 100644 index 000000000000..3f4f61bce750 --- /dev/null +++ b/.changeset/tame-bags-remember.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes a bug that caused errors in dev when editing sites with large numbers of MDX pages diff --git a/packages/astro/src/content/mutable-data-store.ts b/packages/astro/src/content/mutable-data-store.ts index fdffec7cb8cc..ba1e152c8b03 100644 --- a/packages/astro/src/content/mutable-data-store.ts +++ b/packages/astro/src/content/mutable-data-store.ts @@ -6,9 +6,23 @@ import { AstroError, AstroErrorData } from '../core/errors/index.js'; import { IMAGE_IMPORT_PREFIX } from './consts.js'; import { type DataEntry, ImmutableDataStore } from './data-store.js'; import { contentModuleToId } from './utils.js'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; const SAVE_DEBOUNCE_MS = 500; +// Write a file atomically, without triggering the file watcher +async function writeFileAtomic(filePath: PathLike, data: string) { + const dir = await fs.mkdtemp(tmpdir() + '/astro-'); + const tempFile = join(dir, 'temp'); + await fs.writeFile(tempFile, data); + try { + await fs.rename(tempFile, filePath); + } finally { + await fs.rmdir(dir).catch(() => {}); + } +} + /** * Extends the DataStore with the ability to change entries and write them to disk. * This is kept as a separate class to avoid needing node builtins at runtime, when read-only access is all that is needed. @@ -86,7 +100,7 @@ export class MutableDataStore extends ImmutableDataStore { if (this.#assetImports.size === 0) { try { - await fs.writeFile(filePath, 'export default new Map();'); + await writeFileAtomic(filePath, 'export default new Map();'); } catch (err) { throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err }); } @@ -110,7 +124,7 @@ ${imports.join('\n')} export default new Map([${exports.join(', ')}]); `; try { - await fs.writeFile(filePath, code); + await writeFileAtomic(filePath, code); } catch (err) { throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err }); } @@ -122,7 +136,7 @@ export default new Map([${exports.join(', ')}]); if (this.#moduleImports.size === 0) { try { - await fs.writeFile(filePath, 'export default new Map();'); + await writeFileAtomic(filePath, 'export default new Map();'); } catch (err) { throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err }); } @@ -143,7 +157,7 @@ export default new Map([${exports.join(', ')}]); export default new Map([\n${lines.join(',\n')}]); `; try { - await fs.writeFile(filePath, code); + await writeFileAtomic(filePath, code); } catch (err) { throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err }); } @@ -298,7 +312,7 @@ export default new Map([\n${lines.join(',\n')}]); return; } try { - await fs.writeFile(filePath, this.toString()); + await writeFileAtomic(filePath, this.toString()); this.#file = filePath; this.#dirty = false; } catch (err) { From 565f40f864a9f1962e43e99d19b113b2b2e9a8cf Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Wed, 11 Dec 2024 11:52:24 +0000 Subject: [PATCH 2/5] Put tmp alongside the target --- packages/astro/src/content/mutable-data-store.ts | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/packages/astro/src/content/mutable-data-store.ts b/packages/astro/src/content/mutable-data-store.ts index ba1e152c8b03..8fb3e4456b9e 100644 --- a/packages/astro/src/content/mutable-data-store.ts +++ b/packages/astro/src/content/mutable-data-store.ts @@ -6,21 +6,14 @@ import { AstroError, AstroErrorData } from '../core/errors/index.js'; import { IMAGE_IMPORT_PREFIX } from './consts.js'; import { type DataEntry, ImmutableDataStore } from './data-store.js'; import { contentModuleToId } from './utils.js'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; const SAVE_DEBOUNCE_MS = 500; -// Write a file atomically, without triggering the file watcher +// Write a file atomically async function writeFileAtomic(filePath: PathLike, data: string) { - const dir = await fs.mkdtemp(tmpdir() + '/astro-'); - const tempFile = join(dir, 'temp'); + const tempFile = filePath instanceof URL ? new URL(`${filePath.href}.tmp`) : `${filePath}.tmp`; await fs.writeFile(tempFile, data); - try { - await fs.rename(tempFile, filePath); - } finally { - await fs.rmdir(dir).catch(() => {}); - } + await fs.rename(tempFile, filePath); } /** From de21fd3ea2e5ec5ccbd31f962912a6c9c8a7a5fd Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Thu, 12 Dec 2024 09:17:45 +0000 Subject: [PATCH 3/5] Implement locking --- .../astro/src/content/mutable-data-store.ts | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/packages/astro/src/content/mutable-data-store.ts b/packages/astro/src/content/mutable-data-store.ts index 8fb3e4456b9e..1316025797db 100644 --- a/packages/astro/src/content/mutable-data-store.ts +++ b/packages/astro/src/content/mutable-data-store.ts @@ -9,12 +9,7 @@ import { contentModuleToId } from './utils.js'; const SAVE_DEBOUNCE_MS = 500; -// Write a file atomically -async function writeFileAtomic(filePath: PathLike, data: string) { - const tempFile = filePath instanceof URL ? new URL(`${filePath.href}.tmp`) : `${filePath}.tmp`; - await fs.writeFile(tempFile, data); - await fs.rename(tempFile, filePath); -} +const MAX_DEPTH = 10; /** * Extends the DataStore with the ability to change entries and write them to disk. @@ -93,7 +88,7 @@ export class MutableDataStore extends ImmutableDataStore { if (this.#assetImports.size === 0) { try { - await writeFileAtomic(filePath, 'export default new Map();'); + await this.#writeFileAtomic(filePath, 'export default new Map();'); } catch (err) { throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err }); } @@ -117,7 +112,7 @@ ${imports.join('\n')} export default new Map([${exports.join(', ')}]); `; try { - await writeFileAtomic(filePath, code); + await this.#writeFileAtomic(filePath, code); } catch (err) { throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err }); } @@ -129,7 +124,7 @@ export default new Map([${exports.join(', ')}]); if (this.#moduleImports.size === 0) { try { - await writeFileAtomic(filePath, 'export default new Map();'); + await this.#writeFileAtomic(filePath, 'export default new Map();'); } catch (err) { throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err }); } @@ -150,7 +145,7 @@ export default new Map([${exports.join(', ')}]); export default new Map([\n${lines.join(',\n')}]); `; try { - await writeFileAtomic(filePath, code); + await this.#writeFileAtomic(filePath, code); } catch (err) { throw new AstroError(AstroErrorData.UnknownFilesystemError, { cause: err }); } @@ -197,6 +192,29 @@ export default new Map([\n${lines.join(',\n')}]); } } + #writing = new Set(); + #pending = new Set(); + + async #writeFileAtomic(filePath: PathLike, data: string, depth = 0) { + const fileKey = filePath.toString(); + if (this.#writing.has(fileKey)) { + this.#pending.add(fileKey); + return; + } + const tempFile = filePath instanceof URL ? new URL(`${filePath.href}.tmp`) : `${filePath}.tmp`; + this.#writing.add(fileKey); + try { + await fs.writeFile(tempFile, data); + await fs.rename(tempFile, filePath); + } finally { + this.#writing.delete(fileKey); + if (this.#pending.has(fileKey) && depth < MAX_DEPTH) { + this.#pending.delete(fileKey); + await this.#writeFileAtomic(filePath, data, depth + 1); + } + } + } + scopedStore(collectionName: string): DataStore { return { get: = Record>(key: string) => @@ -305,7 +323,7 @@ export default new Map([\n${lines.join(',\n')}]); return; } try { - await writeFileAtomic(filePath, this.toString()); + await this.#writeFileAtomic(filePath, this.toString()); this.#file = filePath; this.#dirty = false; } catch (err) { From 1402927e53db4f38590c6331d979bd06260a459f Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Thu, 12 Dec 2024 17:13:56 +0000 Subject: [PATCH 4/5] Refactor --- packages/astro/src/content/mutable-data-store.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/astro/src/content/mutable-data-store.ts b/packages/astro/src/content/mutable-data-store.ts index 1316025797db..2758095b4851 100644 --- a/packages/astro/src/content/mutable-data-store.ts +++ b/packages/astro/src/content/mutable-data-store.ts @@ -196,20 +196,31 @@ export default new Map([\n${lines.join(',\n')}]); #pending = new Set(); async #writeFileAtomic(filePath: PathLike, data: string, depth = 0) { + if(depth > MAX_DEPTH) { + // If we hit the max depth, we skip a write to prevent the stack from growing too large + return; + } const fileKey = filePath.toString(); + // If we are already writing this file, instead of writing now, flag it as pending and write it when we're done. if (this.#writing.has(fileKey)) { this.#pending.add(fileKey); return; } - const tempFile = filePath instanceof URL ? new URL(`${filePath.href}.tmp`) : `${filePath}.tmp`; + // Prevent concurrent writes to this file by flagging it as being written this.#writing.add(fileKey); + + const tempFile = filePath instanceof URL ? new URL(`${filePath.href}.tmp`) : `${filePath}.tmp`; try { + // Write it to a temporary file first and then move it to prevent partial reads. await fs.writeFile(tempFile, data); await fs.rename(tempFile, filePath); } finally { + // We're done writing. Unflag the file and check if there are any pending writes for this file. this.#writing.delete(fileKey); - if (this.#pending.has(fileKey) && depth < MAX_DEPTH) { + // If there are pending writes, we need to write again to ensure we flush the latest data. + if (this.#pending.has(fileKey)) { this.#pending.delete(fileKey); + // Call ourself recursively to write the file again await this.#writeFileAtomic(filePath, data, depth + 1); } } From 073ab1f82b10178026ba56392cc616798138bc2e Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Thu, 12 Dec 2024 17:16:43 +0000 Subject: [PATCH 5/5] Wording --- packages/astro/src/content/mutable-data-store.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/astro/src/content/mutable-data-store.ts b/packages/astro/src/content/mutable-data-store.ts index 2758095b4851..573adeaaf963 100644 --- a/packages/astro/src/content/mutable-data-store.ts +++ b/packages/astro/src/content/mutable-data-store.ts @@ -198,6 +198,8 @@ export default new Map([\n${lines.join(',\n')}]); async #writeFileAtomic(filePath: PathLike, data: string, depth = 0) { if(depth > MAX_DEPTH) { // If we hit the max depth, we skip a write to prevent the stack from growing too large + // In theory this means we may miss the latest data, but in practice this will only happen when the file is being written to very frequently + // so it will be saved on the next write. This is unlikely to ever happen in practice, as the writes are debounced. It requires lots of writes to very large files. return; } const fileKey = filePath.toString();