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

[dawsontoth]

Packaging a component that a worker has loaded packaged the whole Harper install — ~700MB for a component of a few small files, seen in the wild as ~46s of tarring followed by Maximum response size reached.

Two individually correct behaviours combine badly. symlinkHarperModule links the running install into <component>/node_modules/harper on every non-root component load — that is what makes import { tables } from 'harper' resolve to the live module instance — and the packer dereferences symlinks and recurses into linked directories. So any component actually running gets packaged as Harper.

The fix

node_modules/harper is excluded by path, not by resolving the link's identity. The loader does not merely add that link, it replaces whatever is there — rmSync(harperModule, { recursive: true, force: true }) then a symlink to the install — and it does so whether or not the path existed. Component content there survives no load, so preserving it would protect something Harper destroys anyway.

That choice also buys three things an identity check could not:

• No race. Resolving identity needs I/O, so the set must be snapshotted before the walk — and a deploy_component {restart: true} landing mid-package lets the loader plant the link afterwards, so the walk recurses the install again. A path test has nothing to snapshot.
• No per-package I/O. No lstat/realpath, and the basename is tested before any path parse, so the common case pays a string compare inside tar-fs's synchronous ignore callback — which runs inline on the shared event loop under package_component.
• Correct under skip_symlinks. Gated on dereference, the link is instead packed literally, shipping an absolute path to the packaging host's install root that arrives dangling. The exclusion is unconditional.

For the human reviewer

harperdb is deliberately untouched, and it is not the same gap — this is the judgement call worth checking. The loader only repairs that path when it already exists, unlike harper which it creates unconditionally, so a legacy component's real node_modules/harperdb is its own content. But excluding it — by name or by resolved identity — breaks fresh deployments: the archive omits the path, installApplication returns early because node_modules exists, the loader's present-gate never fires, and a native-mode import from 'harperdb' fails on the target.

An earlier revision of this branch implemented the identity-based exclusion and introduced exactly that regression; it was backed out rather than shipped. Closing it properly means packing a placeholder at the path so the gate can fire, which needs an injected tar entry rather than an ignore predicate. Tracked as #2577 with both options written up.

So a loaded legacy component that ships harperdb still packages the install. That is narrower than what this fixes and is not made worse by it.

Other things a reviewer may want to push on:

• The integration suite skips Windows, matching its deploy-dangling-symlink sibling: restart: true restarts there miss even a 45s readiness deadline (harper#1813 — canPreStartReplacement is false for win32), and symlinkHarperModule's symlink needs developer mode, with its EPERM caught and logged. Without the skip the suite would report the packer broken when it is fine.
• A symlink alias that reaches the owned link under a different lexical path is still followed. The lexical test does not see it; the same limitation already applies to scanPackageDirectory's dangling-link handling, which documents its lack of cycle protection.

Verification

Both tests fail on base for the defect itself, not for a missing field — the assertions are ordered so the size check trips first:

	On base
Unit — plants a link to PACKAGE_ROOT directly	698,255,261 bytes
Integration — restart: true, so a worker plants it	698,253,558 bytes

• Unit runs across {}, skip_symlinks and skip_node_modules, and asserts the estimate stays under 100KB, that no install content reaches the extracted archive, and that both the component's own dependency and a real node_modules/harperdb directory still ship. That last case also caught a latent flaw in the archive assertion: node_modules/harper is a prefix of node_modules/harperdb, so the check is segment-exact rather than startsWith.
• Integration deploys with restart: true so a serving worker plants the link the way production does — the half a packer-only test cannot show — then asserts both the estimate and the real payload.
• 105 unit tests pass across the packaging and CLI suites. bin/cliOperations.ts reads only totalSize and danglingSymlinks, so nothing in the scan contract changed for it.

Closes #2487

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request prevents the component packager from following the node_modules/harper symlink into the Harper installation, which previously caused packaging to walk the entire install. It introduces an unconditional exclusion for this path in packageComponent.ts and adds corresponding integration and unit tests. The review feedback focuses on optimizing the isExcluded check for performance on hot paths, cleaning up the unused basename import, and adding a timeout to the fetch polling loop in the integration test to prevent potential hangs.

[gemini-code-assist]

Performance is a key priority in this codebase, especially on hot paths like directory walks where isExcluded is called for every single file and directory.

Using basename(fullPath) === 'harper' requires parsing the path string on every call, which is relatively expensive. We can optimize this by using fullPath.endsWith(sep + 'harper'), which is a highly optimized native string method.

Additionally, we can restructure the function to compute relative(directory, fullPath) only once and only when necessary (i.e., when either isHarper or options.skip_node_modules is true), avoiding redundant path resolution calls.

	// Unconditional, including under `skip_symlinks`: packed literally, the link ships an absolute path to
	// the packaging host's install root, which arrives dangling.
	const isHarper = fullPath.endsWith(sep + 'harper');
	if (isHarper || options.skip_node_modules) {
		const rel = relative(directory, fullPath);
		if (isHarper && rel === INSTALL_OWNED_PATH) return true;
		if (options.skip_node_modules && (rel.split(sep).includes('node_modules') || rel.includes(WEBPACK_CACHE_SEGMENT))) {
			return true;
		}
	}
	return false;

References

1. Performance is a feature here. Do flag hot-path allocations, unnecessary async/await layers, and per-request work that could be hoisted. ^(link)

[gemini-code-assist]

In integration tests, polling loops that await network requests can hang indefinitely if the request never settles (e.g., if the server accepts the connection but hangs without responding). Since standard fetch in Node.js does not have a default timeout, we should pass a timeout signal using AbortSignal.timeout to ensure the test suite fails cleanly instead of hanging.

Suggested change

	const probe = await fetch(`${ctx.harper.httpURL}/PkgLoadedProbe/`);
	const probe = await fetch(`${ctx.harper.httpURL}/PkgLoadedProbe/`, { signal: AbortSignal.timeout(5000) });

[gemini-code-assist]

Since basename is no longer needed in isExcluded after optimizing the path check, we can remove it from the imports to keep the imports clean.

Suggested change

	import { basename, join, relative, sep } from 'node:path';
	import { join, relative, sep } from 'node:path';

[kriszyp]

_{🤖 Reviewed with Codex}

[claude]

Suggestion (non-blocking): isExcluded runs once per file/directory entry during the full recursive walk (both scanPackageDirectory's loop and tar-fs's ignore callback), so it's a real hot path. basename(fullPath) === 'harper' parses the path via path.basename on every entry; fullPath.endsWith(sep + 'harper') is a native, allocation-free check that's cheaper here. Separately, when an entry's basename happens to be 'harper' but isn't the install-owned path and skip_node_modules is set, relative(directory, fullPath) gets computed twice (once here, again on the next line) — hoisting it to compute once, guarded by isHarper || options.skip_node_modules, avoids the redundant resolution. If adopted, basename becomes unused in the line-1 import.

[claude]

Suggestion (non-blocking): two issues with this readiness loop. First, probe.status < 500 is satisfied on the very first request — Harper's HTTP layer returns a plain 404 (not a connection error, not >=500) for any unmatched resource path, confirmed by integrationTests/deploy/inactive-component-404.test.ts. Since startHarper(ctx) already has the base server listening before this deploy runs, /PkgLoadedProbe/ returns 404 immediately regardless of whether the worker has finished loading the new component, so the loop breaks on the first iteration and never actually waits for the load — unlike sibling tests deploy-from-source.test.ts and redeploy-runtime-equivalence.test.ts, which correctly gate on status === 200. Second, the fetch itself has no timeout; if the server accepted the connection but hung, this await could block indefinitely rather than failing with the intended deadline message. Suggest probe.status === 200 for the break condition plus signal: AbortSignal.timeout(5000) on the fetch.

[claude]

Reviewed; no blockers found. Posted 2 non-blocking suggestions inline.