From 44b79c61129a14af9215e0b076815426365bb981 Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Sat, 31 Oct 2026 22:40:09 -0500 Subject: [PATCH 1/2] Write package mirror files atomically The crawler wrote each mirror file with fs.writeFileSync on its final name, which truncates and rewrites in place. nginx serves the mirror directory, so a download during a write, most likely when a direct copy replaces a Simplifier copy, could get a partial tarball. With a CDN in front of /web/ that partial file would be cached for everyone. Write beside the target and rename over it, so readers see either the old file or the complete new one. Co-Authored-By: Claude Opus 5.5 --- packages/package-crawler.js | 11 ++++++++++- .../packages/package-crawler-versions.test.js | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/package-crawler.js b/packages/package-crawler.js index 0eec9da7..0c8ffd21 100644 --- a/packages/package-crawler.js +++ b/packages/package-crawler.js @@ -915,7 +915,16 @@ class PackageCrawler { if (path.dirname(path.resolve(filepath)) !== path.resolve(this.config.mirrorPath)) { throw new Error(`Package ${id}#${version} rejected: its mirror file name is not a plain file name`); } - fs.writeFileSync(filepath, packageBuffer); + // Write beside the target and rename over it, so nginx (and a CDN in front of it) + // only ever serves the old file or the complete new one, never a partly written one + const tmpPath = `${filepath}.tmp-${process.pid}`; + try { + fs.writeFileSync(tmpPath, packageBuffer); + fs.renameSync(tmpPath, filepath); + } catch (e) { + fs.rmSync(tmpPath, { force: true }); + throw e; + } } return check.action === 'replace' ? 'replaced' : 'stored'; diff --git a/tests/packages/package-crawler-versions.test.js b/tests/packages/package-crawler-versions.test.js index cf6c89ee..94cf3160 100644 --- a/tests/packages/package-crawler-versions.test.js +++ b/tests/packages/package-crawler-versions.test.js @@ -94,6 +94,25 @@ describe('crawler: one copy of each id#version', () => { expect(pkg.CurrentVersion).toBe(before.PackageVersionKey); }); + test('replacing a mirror file swaps it whole: a reader of the old file still sees all of it', async () => { + const old = tarball('ex.r', '1.0.0', 'simplifier'); + await feedItem(env.crawler, 'ex.r#1.0.0', simplifier('ex.r', '1.0.0'), old); + const file = path.join(env.tmp, 'mirror', 'ex.r-1.0.0.tgz'); + const fd = fs.openSync(file, 'r'); + try { + const buf = tarball('ex.r', '1.0.0', 'direct'); + await feedItem(env.crawler, 'ex.r#1.0.0', direct('ex.r', '1.0.0'), buf); + // written in place, the open file would now be truncated or hold the new bytes + const seen = Buffer.alloc(old.length + 1); + expect(fs.readSync(fd, seen, 0, seen.length, 0)).toBe(old.length); + expect(seen.subarray(0, old.length)).toEqual(old); + expect(mirror('ex.r', '1.0.0')).toEqual(buf); + expect(fs.readdirSync(path.join(env.tmp, 'mirror')).filter(f => f.includes('.tmp-'))).toEqual([]); + } finally { + fs.closeSync(fd); + } + }); + test('a second direct copy from another GUID is an error, and ignored', async () => { const buf = tarball('ex.c', '1.0.0', 'first'); await feedItem(env.crawler, 'ex.c#1.0.0', direct('ex.c', '1.0.0'), buf); From e5a1b1bba0cfe7b218bad6e270ec3a09f65bde9f Mon Sep 17 00:00:00 2001 From: Josh Mandel Date: Sun, 1 Nov 2026 17:11:41 -0600 Subject: [PATCH 2/2] Put the mirror file in place before the registry advertises the package The crawler committed a package to the database (which starts the download redirect to the mirror) and only then wrote its mirror file, so every newly published package briefly had a missing or partly written file behind an advertised id#version. Now the file is written beside its target first; a new package's file is renamed into place before the commit, and a replacement's after it, so the old file is served until the replacement commits. A failed commit leaves the served file alone and removes a new package's file. Co-Authored-By: Claude Opus 5.5 --- packages/package-crawler.js | 42 ++++++++++++------ .../packages/package-crawler-versions.test.js | 44 +++++++++++++++++++ 2 files changed, 72 insertions(+), 14 deletions(-) diff --git a/packages/package-crawler.js b/packages/package-crawler.js index 0c8ffd21..c905d0e4 100644 --- a/packages/package-crawler.js +++ b/packages/package-crawler.js @@ -902,29 +902,43 @@ class PackageCrawler { // Extract URLs from package const urls = this.processPackageUrls(npmPackage); - // Commit to database - await this.commit(packageBuffer, npmPackage, date, guid, id, version, canonical, urls, check.keys); - - // Only now write the mirror copy: a package that failed validation or didn't commit - // must not replace the file that is served for the copy we hold + // The mirror file has to be complete before the registry advertises the package (the + // download redirect points at it), and a replacement must not touch the file served for + // the copy we hold unless it commits. So: write the new file beside its target first; + // a new package's file is put in place before the commit, a replacement's after it. + // Rename within a directory is atomic, so readers see the old file or the whole new one. + let filepath = null; + let tmpPath = null; if (this.config.mirrorPath) { let fid = this.fixPrefix(id); - const filename = `${fid}-${version}.tgz`; - const filepath = path.join(this.config.mirrorPath, filename); + filepath = path.join(this.config.mirrorPath, `${fid}-${version}.tgz`); // id and version are validated, but the file must land in the mirror whatever they say if (path.dirname(path.resolve(filepath)) !== path.resolve(this.config.mirrorPath)) { throw new Error(`Package ${id}#${version} rejected: its mirror file name is not a plain file name`); } - // Write beside the target and rename over it, so nginx (and a CDN in front of it) - // only ever serves the old file or the complete new one, never a partly written one - const tmpPath = `${filepath}.tmp-${process.pid}`; - try { - fs.writeFileSync(tmpPath, packageBuffer); + tmpPath = `${filepath}.tmp-${process.pid}`; + fs.writeFileSync(tmpPath, packageBuffer); + } + const replacing = check.action === 'replace'; + let placed = false; + try { + if (tmpPath && !replacing) { fs.renameSync(tmpPath, filepath); - } catch (e) { + placed = true; + } + await this.commit(packageBuffer, npmPackage, date, guid, id, version, canonical, urls, check.keys); + if (tmpPath && replacing) { + fs.renameSync(tmpPath, filepath); + } + } catch (e) { + if (tmpPath) { fs.rmSync(tmpPath, { force: true }); - throw e; } + if (placed) { + // a new package that didn't commit isn't advertised, so its file goes too + fs.rmSync(filepath, { force: true }); + } + throw e; } return check.action === 'replace' ? 'replaced' : 'stored'; diff --git a/tests/packages/package-crawler-versions.test.js b/tests/packages/package-crawler-versions.test.js index 94cf3160..46bd2aed 100644 --- a/tests/packages/package-crawler-versions.test.js +++ b/tests/packages/package-crawler-versions.test.js @@ -113,6 +113,50 @@ describe('crawler: one copy of each id#version', () => { } }); + // What the mirror holds at the moment the registry commits (and so starts advertising) a package + const atCommit = () => { + const seen = {}; + const commit = env.crawler.commit.bind(env.crawler); + env.crawler.commit = jest.fn(async (buf, npm, date, guid, id, version, ...rest) => { + const file = path.join(env.tmp, 'mirror', `${id}-${version}.tgz`); + seen.file = fs.existsSync(file) ? fs.readFileSync(file) : null; + seen.tmp = fs.readdirSync(path.join(env.tmp, 'mirror')).filter(f => f.includes('.tmp-')); + return commit(buf, npm, date, guid, id, version, ...rest); + }); + return seen; + }; + + test('a new package\'s mirror file is complete before the registry commits it', async () => { + const seen = atCommit(); + const buf = tarball('ex.n', '1.0.0'); + expect((await feedItem(env.crawler, 'ex.n#1.0.0', direct('ex.n', '1.0.0'), buf)).status).toBe('Fetched'); + expect(seen.file).toEqual(buf); + expect(seen.tmp).toEqual([]); + }); + + test('a replacement keeps serving the old file until it commits, then swaps it', async () => { + const old = tarball('ex.s', '1.0.0', 'simplifier'); + await feedItem(env.crawler, 'ex.s#1.0.0', simplifier('ex.s', '1.0.0'), old); + const seen = atCommit(); + const buf = tarball('ex.s', '1.0.0', 'direct'); + expect((await feedItem(env.crawler, 'ex.s#1.0.0', direct('ex.s', '1.0.0'), buf)).status).toBe('Replaced Simplifier copy'); + expect(seen.file).toEqual(old); + expect(mirror('ex.s', '1.0.0')).toEqual(buf); + expect(fs.readdirSync(path.join(env.tmp, 'mirror')).filter(f => f.includes('.tmp-'))).toEqual([]); + }); + + test('if the commit fails, a replacement leaves the served file alone and a new package leaves no file', async () => { + const old = tarball('ex.f', '1.0.0', 'simplifier'); + await feedItem(env.crawler, 'ex.f#1.0.0', simplifier('ex.f', '1.0.0'), old); + env.crawler.commit = jest.fn(async () => { throw new Error('commit failed'); }); + await feedItem(env.crawler, 'ex.f#1.0.0', direct('ex.f', '1.0.0'), tarball('ex.f', '1.0.0', 'direct')); + expect(mirror('ex.f', '1.0.0')).toEqual(old); + await feedItem(env.crawler, 'ex.g#1.0.0', direct('ex.g', '1.0.0'), tarball('ex.g', '1.0.0')); + const files = fs.readdirSync(path.join(env.tmp, 'mirror')); + expect(files).not.toContain('ex.g-1.0.0.tgz'); + expect(files.filter(f => f.includes('.tmp-'))).toEqual([]); + }); + test('a second direct copy from another GUID is an error, and ignored', async () => { const buf = tarball('ex.c', '1.0.0', 'first'); await feedItem(env.crawler, 'ex.c#1.0.0', direct('ex.c', '1.0.0'), buf);