diff --git a/packages/package-crawler.js b/packages/package-crawler.js index 0eec9da7..c905d0e4 100644 --- a/packages/package-crawler.js +++ b/packages/package-crawler.js @@ -902,20 +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`); } - fs.writeFileSync(filepath, 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); + 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 }); + } + 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 cf6c89ee..46bd2aed 100644 --- a/tests/packages/package-crawler-versions.test.js +++ b/tests/packages/package-crawler-versions.test.js @@ -94,6 +94,69 @@ 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); + } + }); + + // 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);