Repository navigation
Conversation
`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>
There was a problem hiding this comment.
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().
|
|
||
| const REQUEST_TIMEOUT_MS = 10_000; | ||
|
|
||
| async function fetchWithTimeout( |
There was a problem hiding this comment.
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.
|
Reviewed; no blockers found. |
I dunno if I really like this approach, seems like verifying that Harper can do a real
npm installis 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?mainwent green → red at 576d333 — a release-only commit that changes three lines ofpackage.jsonandpackage-lock.json— on one of 36 Integration Tests legs (run 35740846737,Integration Tests 6/6 (Node.js v26)). TheComponent: redirectorsuite'sbeforehook died after 307,394 ms withUND_ERR_HEADERS_TIMEOUT, cancelling all 23 tests. I re-ran that leg on the same SHA and it passed, somainis green again; this PR removes the cause rather than the symptom.The suite deploys
template-redirector-3.0.1.tgz, whose manifest pinspapaparseand which ships nonode_modules, sodeploy_componentruns a realnpm installagainst the public npm registry on the test's critical path. Nothing bounds that: Harper's spawn budget there isDEFAULT_COMMAND_TIMEOUT_MS = 60 * 60 * 1000, and every request the suite made was a barefetchwhose 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
papaparseinto 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:
integrationTests/mqtt/mqtt.test.tsandintegrationTests/components/acl-connect.test.tsdeploy fixtures pinning@harperdb/acl-connectandjsonwebtokenwith no committednode_modules, through the same unboundedsendOperation. 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.sendOperationitself still has no timeout, across 118 call sites in 25 suites. Adding one in@harperfast/integration-testingwould 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.fetchWithTimeoutis copied verbatim fromearly-hints.test.ts:21-34rather than hoisted next towaitForLog.ts. Two copies can drift. I kept them byte-identical so a later consolidation is mechanical.npm pack template-redirector@3.0.1would 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.integrationTests/**/fixturesthat fails on production dependencies with no bundlednode_moduleswould have caught the two suites above. Cheap to add later, out of scope here.Declined review nits, all in
fetchWithTimeout, all inherited from theearly-hints.test.tscopy: the helper names a timeout only during the headers phase (a body stall rejects with a plainTimeoutError, so the test still fails, just less legibly);{ ...init, signal }overwrites a caller-suppliedsignal, 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 thanearly-hintsunder the same budget.Changes
integrationTests/fixtures/template-redirector-3.0.1.tgzis removed and replaced bytemplate-redirector-3.0.1-vendored.tgz: byte-for-byte the same publishedtemplate-redirector@3.0.1contents pluspackage/node_modules/papaparse/, which is the publishedpapaparse@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 withtar --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. Withnode_modulespresent,installApplicationtakes its early return and the deploy runs no package manager at all.integrationTests/components/redirector.test.tsstops callingsendOperationfor the deploy — that was the exact call that hung — and issues it throughctx.harper.operationsAPIURLwith a 90 s deadline and an explicit status check before parsing. Every other request in the suite now goes through afetchWithTimeouthelper carryingAbortSignal.timeout. That is not only defensive: the readiness poll declared a 60 s deadline but checked it after eachfetchsettled, so a single hung request made the deadline unreachable. Thebeforehook now also asserts, from the instance log, that the deploy loggedalready has node_modules; skipping installand 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 pinslogging: { level: 'debug' }instead of inheriting the harness default.integrationTests/DESIGN.mdis new andDESIGN.mdgains 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, thennpm 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.[main/0] [debug] [redirector:spawn:npm]: Executing npm install --force --omit=dev --no-audit --no-fund --ignore-scriptsand 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 tointegrationTests/server/ollama-backend.test.ts, which self-skips unless a local Ollama is reachable; on this machine it is, and it fails on an unrelatedERR_IMPORT_ATTRIBUTE_MISSINGloadingjson/systemSchema.jsonunder Node v26.2.0. Untouched by this diff and skipped in CI.npm run format:check,npx oxlint --deny-warningson the changed test file, andnpm run check:design-docsall 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
mainon 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
mainoldest-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-healthboard doc either way (prose row beginning with the test path, never a bare ref), and add your PR toinitiatives/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-1Review-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