Stop package_component packaging the Harper install through a loaded component's node_modules/harper

components/packageComponent.ts

@@ -1,4 +1,4 @@
-import { join, relative, sep } from 'node:path';
+import { basename, join, relative, sep } from 'node:path';
 import { stat, readdir } from 'node:fs/promises';
 import { Readable, pipeline } from 'node:stream';
 import tar from 'tar-fs';
@@ -13,6 +13,27 @@ const DEFAULT_OPTIONS: PackageOptions = { skip_node_modules: false, skip_symlink
 
 const WEBPACK_CACHE_SEGMENT = join('cache', 'webpack');
 
+/**
+ * `node_modules/harper` is Harper's, whatever is on disk there.
+ *
+ * Every non-root component load runs `symlinkHarperModule`, which does `rmSync(harperModule, {
+ * recursive: true, force: true })` and then links the running install (`components/componentLoader.ts`).
+ * It does that whether or not the path already existed, so component content there survives no load and
+ * packaging it cannot be preserving component source — while packaging it walks the whole install, since
+ * the packer dereferences symlinks and recurses into linked directories.
+ *
+ * `harperdb` is deliberately NOT here, and the asymmetry is the reason: the loader only *repairs*
+ * `node_modules/harperdb` when that path already exists. A legacy component shipping a real
+ * npm-installed `harperdb` that has never been loaded on this host therefore owns it — and excluding it
+ * would strip it from the tarball, after which `installApplication` skips installing (it returns early
+ * when `node_modules` exists), the loader's present-gate never fires, and `import from 'harperdb'`
+ * fails on the target — and that holds whether the path is skipped by name or by resolved identity,
+ * because an archive that simply OMITS it cannot restore it. So a `harperdb` link to the install is
+ * still followed. Closing that means packing a placeholder at the path so the loader's present-gate can
+ * fire on the target, not skipping it; tracked in harper#2577.
+ */
+const INSTALL_OWNED_PATH = join('node_modules', 'harper');
+
 /**
  * Whether `fullPath` (an absolute path under `directory`) should be excluded from the package when
  * `skip_node_modules` is set. The path is first made relative to `directory`, so packaging a component
@@ -21,6 +42,9 @@ const WEBPACK_CACHE_SEGMENT = join('cache', 'webpack');
  * excluded the whole tree. Shared by the stream packer and the directory walk so they cannot diverge.
  */
 function isExcluded(directory: string, fullPath: string, options: PackageOptions): boolean {
+	// Unconditional, including under `skip_symlinks`: packed literally, the link ships an absolute path to
+	// the packaging host's install root, which arrives dangling.
+	if (basename(fullPath) === 'harper' && relative(directory, fullPath) === INSTALL_OWNED_PATH) return true;
 	if (!options.skip_node_modules) return false;
 	const rel = relative(directory, fullPath);
 	return rel.split(sep).includes('node_modules') || rel.includes(WEBPACK_CACHE_SEGMENT);

integrationTests/deploy/package-loaded-component.test.ts

@@ -0,0 +1,120 @@
+/**
+ * `package_component` on a component a worker has LOADED — harper#2487.
+ *
+ * Pins: `symlinkHarperModule` links the running install into `<component>/node_modules/harper` on every
+ * non-root component load, and the packer dereferences symlinks and recurses into linked directories. So
+ * packaging a component that has actually been loaded used to walk the entire Harper install: observed as
+ * ~46s of tarring followed by `Maximum response size reached`.
+ *
+ * The unit test in `unitTests/components/packageComponent.test.js` plants the link directly; this one earns
+ * it the way production does — deploy with `restart: true` so a serving worker loads the component — which
+ * is the half a packer-only test cannot show.
+ *
+ * Run: npm run test:integration -- "integrationTests/deploy/package-loaded-component.test.ts"
+ */
+import { suite, test, before, after } from 'node:test';
+import { ok, strictEqual } from 'node:assert';
+import { join } from 'node:path';
+import { existsSync } from 'node:fs';
+import { mkdtemp, writeFile, rm, mkdir } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+
+import { setTimeout as sleep } from 'node:timers/promises';
+
+import { startHarper, teardownHarper, targz, type ContextWithHarper } from '@harperfast/integration-testing';
+
+const PROJECT = 'pkg-loaded-probe';
+
+// Skipped on Windows for the same reason as the sibling `deploy-dangling-symlink` suite: `restart: true`
+// restarts there consistently miss even a 45s readiness deadline (harper#1813 — `canPreStartReplacement` is
+// false for win32, so there is no overlapping replacement). This test additionally depends on
+// `symlinkHarperModule` succeeding, and on Windows that needs developer mode for symlinks — its EPERM is
+// caught and logged by the loader, so the link would simply be absent and this suite would report the
+// packer broken when it is fine.
+const skipSuite = process.platform === 'win32';
+
+async function operation(ctx: ContextWithHarper, body: Record<string, unknown>): Promise<any> {
+	const auth = 'Basic ' + Buffer.from(`${ctx.harper.admin.username}:${ctx.harper.admin.password}`).toString('base64');
+	const response = await fetch(ctx.harper.operationsAPIURL, {
+		method: 'POST',
+		headers: { 'Content-Type': 'application/json', 'Authorization': auth },
+		body: JSON.stringify(body),
+	});
+	strictEqual(response.status, 200, `${body.operation} failed with ${response.status}`);
+	return response.json();
+}
+
+/** A component with a loadable resource, so a worker actually loads it and plants the link. */
+async function buildPayload(): Promise<string> {
+	const dir = await mkdtemp(join(tmpdir(), 'pkg-loaded-fixture-'));
+	try {
+		await writeFile(join(dir, 'package.json'), JSON.stringify({ name: PROJECT, version: '1.0.0' }));
+		await writeFile(join(dir, 'config.yaml'), 'rest: true\njsResource:\n  files: resource.js\n');
+		await writeFile(
+			join(dir, 'resource.js'),
+			'export class PkgLoadedProbe extends Resource {\n\tget() {\n\t\treturn { ok: true };\n\t}\n}\n'
+		);
+		await mkdir(join(dir, 'web'), { recursive: true });
+		await writeFile(join(dir, 'web', 'index.html'), '<!doctype html><title>probe</title>\n');
+		return await targz(dir);
+	} finally {
+		await rm(dir, { recursive: true, force: true });
+	}
+}
+
+suite(
+	'package_component does not follow a loaded component into the install',
+	{ skip: skipSuite },
+	(ctx: ContextWithHarper) => {
+		before(async () => {
+			await startHarper(ctx);
+		});
+		after(async () => {
+			await teardownHarper(ctx);
+		});
+
+		test('a component a worker has loaded packages as itself, not as Harper', async () => {
+			// `restart: true` is the whole point: it makes a serving worker load the component, which is what
+			// creates the link. Without the restart nothing has loaded it and the bug is unreachable.
+			await operation(ctx, {
+				operation: 'deploy_component',
+				project: PROJECT,
+				payload: await buildPayload(),
+				restart: true,
+			});
+
+			// `deploy_component` returns before the replacement worker has finished loading, so the link this
+			// test depends on may not exist yet. Wait for the component's own route to answer — that is the
+			// signal a worker completed the load that plants it.
+			const deadline = Date.now() + 45_000;
+			while (true) {
+				try {
+					const probe = await fetch(`${ctx.harper.httpURL}/PkgLoadedProbe/`);
+					if (probe.status < 500) break;
+				} catch {
+					// not accepting connections yet
+				}
+				if (Date.now() > deadline) throw new Error('Timed out waiting for the component to load after restart');
+				await sleep(250);
+			}
+
+			const livePath = join(ctx.harper.dataRootDir, 'components', PROJECT);
+			ok(existsSync(join(livePath, 'resource.js')), 'the component is live');
+			// The link is expected to be here — this test is about the packer not following it, not about
+			// preventing it. If a future change stops creating it, this test stops proving anything, so assert
+			// its presence rather than silently passing.
+			ok(existsSync(join(livePath, 'node_modules', 'harper')), 'a worker load planted the harper link');
+
+			const estimate = await operation(ctx, { operation: 'package_component', project: PROJECT, estimate: true });
+			ok(
+				estimate.total_size < 1_000_000,
+				`packaging walked the install (${estimate.total_size} bytes for a handful of small files)`
+			);
+
+			// And the real thing, not just the estimate: the payload comes back rather than failing on size.
+			const packaged = await operation(ctx, { operation: 'package_component', project: PROJECT });
+			ok(typeof packaged.payload === 'string' && packaged.payload.length > 0, 'a payload was produced');
+			ok(packaged.payload.length < 2_000_000, `payload carries the install (${packaged.payload.length} base64 chars)`);
+		});
+	}
+);

unitTests/components/packageComponent.test.js

@@ -10,7 +10,13 @@ const { Readable } = require('node:stream');
 const testUtils = require('../testUtils.js');
 testUtils.preTestPrep();
 
-const { streamPackagedDirectory, packageDirectory, findDanglingSymlinks } = require('#src/components/packageComponent');
+const {
+	streamPackagedDirectory,
+	packageDirectory,
+	findDanglingSymlinks,
+	scanPackageDirectory,
+} = require('#src/components/packageComponent');
+const { PACKAGE_ROOT } = require('#src/utility/packageUtils');
 const { buildMultipartBody } = require('#src/bin/multipartBuilder');
 const { parseMultipartRequest } = require('#src/server/serverHelpers/multipartParser');
 const gunzip = require('gunzip-maybe');
@@ -173,6 +179,54 @@ describe('streamPackagedDirectory round-trip', () => {
 		}
 	});
 
+	it('never packages node_modules/harper, but leaves a component-owned harperdb alone', async function () {
+		this.timeout(30000);
+		for (const options of [{}, { skip_symlinks: true }, { skip_node_modules: true }]) {
+			const label = JSON.stringify(options);
+			const sourceDir = await makeFixture({ 'index.js': 'x\n', 'src/a.js': 'a\n' });
+			const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'pkg-harper-link-'));
+			await fs.mkdir(path.join(sourceDir, 'node_modules', 'keep-me'), { recursive: true });
+			await fs.writeFile(path.join(sourceDir, 'node_modules', 'keep-me', 'index.js'), 'keep\n');
+			await fs.symlink(PACKAGE_ROOT, path.join(sourceDir, 'node_modules', 'harper'), 'dir');
+			// A legacy component's REAL harperdb dependency. The loader only repairs that path when it
+			// already exists, so this content is the component's — and stripping it would not be reinstalled
+			// on the target, because `installApplication` skips installing when node_modules exists.
+			await fs.mkdir(path.join(sourceDir, 'node_modules', 'harperdb'), { recursive: true });
+			await fs.writeFile(path.join(sourceDir, 'node_modules', 'harperdb', 'index.js'), 'legacy\n');
+
+			try {
+				const scan = await scanPackageDirectory(sourceDir, options);
+				assert.ok(scan.totalSize < 100_000, `${label}: estimate walked the install (${scan.totalSize} bytes)`);
+
+				await pipeline(streamPackagedDirectory(sourceDir, options), gunzip(), tar.extract(extractDir));
+				const extracted = await readDirTree(extractDir);
+				assert.strictEqual(extracted['index.js'], 'x\n', `${label}: real files survive`);
+				// Segment-exact, not a prefix test: `node_modules/harper` is a prefix of
+				// `node_modules/harperdb`, so a `startsWith` here would flag the legacy dependency below.
+				const harperPrefix = path.join('node_modules', 'harper') + path.sep;
+				assert.ok(
+					!Object.keys(extracted).some((k) => k.startsWith(harperPrefix)),
+					`${label}: packed the install: ${Object.keys(extracted).slice(0, 5).join(', ')}`
+				);
+				if (!options.skip_node_modules) {
+					assert.strictEqual(
+						extracted[path.join('node_modules', 'keep-me', 'index.js')],
+						'keep\n',
+						`${label}: the component's own dependencies still ship`
+					);
+					assert.strictEqual(
+						extracted[path.join('node_modules', 'harperdb', 'index.js')],
+						'legacy\n',
+						`${label}: a component-owned harperdb must not be stripped`
+					);
+				}
+			} finally {
+				await fs.rm(sourceDir, { recursive: true, force: true });
+				await fs.rm(extractDir, { recursive: true, force: true });
+			}
+		}
+	});
+
 	it('findDanglingSymlinks reports broken links and ignores valid ones and node_modules', async function () {
 		this.timeout(15000);
 		const sourceDir = await makeFixture({ 'index.js': 'x\n', 'src/a.js': 'a\n' });