Skip to content

Keep the redirector integration test off the npm registry - #2752

Open
kriszyp wants to merge 4 commits into
mainfrom
fix/deflake-redirector-integration-test
Open

kriszyp wants to merge 4 commits into
mainfrom
fix/deflake-redirector-integration-test

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

I dunno if I really like this approach, seems like verifying that Harper can do a real npm install is a good thing to check, even if it can possibly cause red when GH is down. I'm inclined to decline/close this PR. WDYT?

main went green → red at 576d333 — a release-only commit that changes three lines of package.json and package-lock.json — on one of 36 Integration Tests legs (run 35740846737, Integration Tests 6/6 (Node.js v26)). The Component: redirector suite's before hook died after 307,394 ms with UND_ERR_HEADERS_TIMEOUT, cancelling all 23 tests. I re-ran that leg on the same SHA and it passed, so main is green again; this PR removes the cause rather than the symptom.

The suite deploys template-redirector-3.0.1.tgz, whose manifest pins papaparse and which ships no node_modules, so deploy_component runs a real npm install against the public npm registry on the test's critical path. Nothing bounds that: Harper's spawn budget there is DEFAULT_COMMAND_TIMEOUT_MS = 60 * 60 * 1000, and every request the suite made was a bare fetch whose only limit was undici's implicit ~300 s headers timeout. The instance log for the failed run shows the server going silent 1.9 s after the npm child was spawned and never speaking again.

The fix vendors papaparse into the archive so the deploy spawns nothing, and gives every request in the suite a deadline it can actually enforce.

For the human reviewer

Framing-Verdict: chosen-approach-sound (planning review, codex/xhigh). No alternative was overruled or adopted; the reviewer independently derived the same layer (fixture + harness, not Harper's production deploy defaults) before reading the plan.

Open decisions the review surfaced and I resolved rather than implemented, each cheap to revisit:

  • The same flake is still live in two other suites. integrationTests/mqtt/mqtt.test.ts and integrationTests/components/acl-connect.test.ts deploy fixtures pinning @harperdb/acl-connect and jsonwebtoken with no committed node_modules, through the same unbounded sendOperation. Left out of this PR to keep the red-main fix reviewable; recorded in the design note so the next timeout there is not diagnosed as a new bug.
  • sendOperation itself still has no timeout, across 118 call sites in 25 suites. Adding one in @harperfast/integration-testing would fix all of them at once and would have been the wider fix; this PR only stops using it in the one suite that was failing.
  • fetchWithTimeout is copied verbatim from early-hints.test.ts:21-34 rather than hoisted next to waitForLog.ts. Two copies can drift. I kept them byte-identical so a later consolidation is mechanical.
  • The fixture is a binary archive that must be regenerated by hand. Plain npm pack template-redirector@3.0.1 would silently drop the vendored tree and restore the flake; that is why the filename diverges from the package it came from, and why the test asserts the skip-install path rather than trusting the archive.
  • Nothing enforces the invariant across fixtures. A static check over integrationTests/**/fixtures that fails on production dependencies with no bundled node_modules would have caught the two suites above. Cheap to add later, out of scope here.

Declined review nits, all in fetchWithTimeout, all inherited from the early-hints.test.ts copy: the helper names a timeout only during the headers phase (a body stall rejects with a plain TimeoutError, so the test still fails, just less legibly); { ...init, signal } overwrites a caller-supplied signal, which no caller passes; and the 90 s deploy cap is below undici's implicit ~300 s, which is deliberate — redirector no longer runs an install, so it does strictly less work than early-hints under the same budget.

Changes

integrationTests/fixtures/template-redirector-3.0.1.tgz is removed and replaced by template-redirector-3.0.1-vendored.tgz: byte-for-byte the same published template-redirector@3.0.1 contents plus package/node_modules/papaparse/, which is the published papaparse@5.5.3 — the exact version the fixture's own manifest pins — with its MIT license, no symlinks, and no other package. It was packed with tar --sort=name --mtime=... --owner=0 --group=0 --numeric-owner --format=ustar | gzip -9 -n, so regenerating it reproduces the same bytes. 28,894 → 75,196 bytes. With node_modules present, installApplication takes its early return and the deploy runs no package manager at all.

integrationTests/components/redirector.test.ts stops calling sendOperation for the deploy — that was the exact call that hung — and issues it through ctx.harper.operationsAPIURL with a 90 s deadline and an explicit status check before parsing. Every other request in the suite now goes through a fetchWithTimeout helper carrying AbortSignal.timeout. That is not only defensive: the readiness poll declared a 60 s deadline but checked it after each fetch settled, so a single hung request made the deadline unreachable. The before hook now also asserts, from the instance log, that the deploy logged already has node_modules; skipping install and no [redirector:spawn:npm] line, which is what stops a future fixture refresh from quietly reintroducing the registry. That assertion reads debug-level lines, so the suite pins logging: { level: 'debug' } instead of inheriting the harness default.

integrationTests/DESIGN.md is new and DESIGN.md gains its index line: one note recording why a deployed fixture must not install from the registry, the four hermetic patterns available, which one this fixture uses, and the two suites that still violate it.

Verification

End-to-end route: the existing integration suite, extended with the skip-install log assertion.

  • npm run build, then npm run test:integration -- integrationTests/components/redirector.test.ts — 23/23 pass, and suite wall time drops 26.5 s → 14.5 s, the npm install being the difference.
  • Fails-on-base is established by the failing run's own log rather than a synthetic revert: at 576d333 the instance logged [main/0] [debug] [redirector:spawn:npm]: Executing npm install --force --omit=dev --no-audit --no-fund --ignore-scripts and never logged the skip line, so both new assertions fail on the old fixture.
  • npm run test:integration -- "integrationTests/components/**/*.test.ts" — 149/149 pass.
  • npm run test:integration -- "integrationTests/deploy/**/*.test.ts" — 85 pass, 1 skipped, 0 fail.
  • npm run test:integration:all — 2159 tests, 2135 pass, 0 fail, 18 skipped. The 6 cancelled belong to integrationTests/server/ollama-backend.test.ts, which self-skips unless a local Ollama is reachable; on this machine it is, and it fails on an unrelated ERR_IMPORT_ATTRIBUTE_MISSING loading json/systemSchema.json under Node v26.2.0. Untouched by this diff and skipped in CI.
  • npm run format:check, npx oxlint --deny-warnings on the changed test file, and npm run check:design-docs all clean.

Note for whoever reads CI here: PR runs only exercise Node 24, so the leg that actually failed (Node 26, shard 6) is not reproduced by this PR's own checks.

Authored by Claude Opus 5.

🤖 Generated with Claude Code

Complexity: easy

Origin — the dispatch brief this PR was written from

harper main is red: Integration Tests at 576d333

main on HarperFast/harper went from green to red. Failing workflows: Integration Tests at 576d333.

Find the merge that broke it and get main green. Walk that workflow's recent push runs on main oldest-to-newest to find the FIRST red head SHA and the PR it came from, then pull the failing test names from that run and from the current head. A failure on every matrix leg (Node version, Bun, uWS, Windows) is a regression; one leg is usually a flake.

Then either fix it or, when the culprit is a single merge and the fix is not obvious, open a revert and say so — main being green is worth more than the change being preserved. Record the fingerprint on the initiatives/ci-health board doc either way (prose row beginning with the test path, never a bare ref), and add your PR to initiatives/testing-and-deploy > "CI & build infrastructure" as a bare-ref line so it renders as a card.

Dispatch: task main-red-kriszyp_harper_576d33330_f0efc59e · queued by automation · ran by claude/opus/high · worker kzyp-xps-1

Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=4; full=1 @ 7202e1d

Human-Review-Need: 3 (decisions: deploy-timeout-layer, deflake-scope, no-install-proof-shape, helper-duplication, vendor-binary-archive, invariant-enforcement-scope) @ 7202e1d

kriszyp and others added 4 commits September 22, 2026 09:04
`Component: redirector` deploys a fixture that declares a production
dependency and ships no `node_modules`, so `deploy_component` runs a real
`npm install` against the public registry on the suite's critical path.
Harper's spawn bound there is an hour, and every request the test makes was
an unbounded `fetch`, so a stalled install produced five minutes of silence
and then `UND_ERR_HEADERS_TIMEOUT`, cancelling all 23 tests. That is how
`main` went red at 576d333, a release-only commit, on one leg of 36.

Vendor `papaparse` into the archive so `installApplication` takes its
`node_modules`-present early return and the deploy spawns nothing, and give
every request in the suite an `AbortSignal` deadline — the readiness poll's
own 60s deadline was unreachable, because it is checked only after each
`fetch` settles.

Dispatch-Task: main-red-kriszyp_harper_576d33330_f0efc59e
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…note

The negative `[redirector:spawn:npm]` assertion only means something at debug
level, so pin it rather than inherit the harness default. The design note
claimed a repository-wide rule that only one suite checks, and omitted the
local-file dependency pattern `deploy/redeploy-runtime-equivalence.test.ts`
already uses.

Dispatch-Task: main-red-kriszyp_harper_576d33330_f0efc59e
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gistry

The note claimed `redeploy-runtime-equivalence.test.ts` was the only
integration coverage of the automatic install path. `mqtt/mqtt.test.ts` and
`components/acl-connect.test.ts` also take it, from the public registry, so a
reader would mis-diagnose the next timeout there as a new bug.

Dispatch-Task: main-red-kriszyp_harper_576d33330_f0efc59e
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…stry fixtures

A headers timeout in those suites is a reason to check the instance log for a
`spawn:npm` line, not proof on its own that the install was the cause.

Dispatch-Task: main-red-kriszyp_harper_576d33330_f0efc59e
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kriszyp kriszyp added this to the v5.3 milestone Sep 22, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request documents design guidelines for integration test fixtures, emphasizing that they must not install from the npm registry to prevent test flakiness. To enforce this, the redirector test was updated to use a vendored fixture and asserts that no npm install is spawned. Additionally, a fetchWithTimeout helper was introduced to replace standard fetch calls in the test suite. Feedback on the changes suggests improving fetchWithTimeout to combine any caller-provided abort signal with the timeout signal using AbortSignal.any().

Comment thread integrationTests/components/redirector.test.ts
@kriszyp
kriszyp marked this pull request as ready for review September 23, 2026 12:47

const REQUEST_TIMEOUT_MS = 10_000;

async function fetchWithTimeout(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (non-blocking): fetchWithTimeout (lines 20-30) is now byte-identical to the copy in early-hints.test.ts:21-34 — the PR description already flags this as an intentional, cheap-to-revisit deferral. Since there are now two copies that can drift, consider hoisting it into a shared module (e.g. alongside waitForLogMatches in waitForLog.ts) and importing it from both test files.

@claude

claude Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No confirmed blocking findings were found on the changed lines. Existing discussion already covers the non-blocking timeout-signal and helper-duplication concerns.

—
Reviewed 7202e1d

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants