Skip to content

Restore floating Node 26 integration coverage - #2342

Open
kriszyp wants to merge 10 commits into
mainfrom
fix/node26-canary-undici-reproducer
Open

Restore floating Node 26 integration coverage#2342
kriszyp wants to merge 10 commits into
mainfrom
fix/node26-canary-undici-reproducer

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 26, 2026

Copy link
Copy Markdown
Member

Remove the dedicated Node 26 fetch canary and its reproducer after an exact Node 26.8.0 / bundled Undici 8.10.0 run cleared the faithful six-case fetch path, and restore floating Node 26 in the broad integration matrix. A CI-only 26.8.0 freshness floor prevents check-latest from silently reporting success on the stale runtime, but the current setup-node manifest still resolves 26.7.0, so this PR must not merge until that external lag clears or the pin is retained.

For the human reviewer

  1. Decide whether to keep the floating 26 matrix entry now, accepting a red main/nightly workflow until actions/node-versions publishes 26.8.0, or retain the 26.5.0 pin while still removing the obsolete dedicated canary. Re-pinning is a one-line workflow change; saying no to the current form delays floating coverage but keeps CI green.
  2. The change favors the broad, normally-polarized six-shard integration suite over the focused idle-event-loop fetch discriminator. This increases product-path coverage and removes inverted canary maintenance, but gives up the more isolated recurrence signal; restoring the deleted test and workflow is straightforward from branch history.
  3. Build and test jobs independently resolve floating Node 26 rather than propagating one exact patch. This favors freshness but allows patch drift between artifact production and test execution; changing to a single resolved version is workflow-only.
  4. Node 26.0.0 through 26.7.x remain inside package.json's supported engine range but are intentionally below the integration-CI freshness floor because they carry the investigated Undici behavior. Narrowing published support would be user-visible; leaving it broad keeps install compatibility at the cost of an untested band.

Verification

  • The official Node 26.8.0 Linux x64 artifact bundled Undici 8.10.0 and passed all six faithful fetch cases with no stall discriminator. The artifact identified its runtime as 26.8.0-alpha.0.0.0, so a verification-only preload normalized the version string past Harper/npm's startup guard; it did not change Undici or fetch behavior.
  • The pushed Node 26 workflow run resolved Node 26.7.0 and failed at the new freshness postcondition with check-latest resolved stale Node 26.7.0; expected >=26.8.0; the full Node 26 integration suite therefore did not run.
  • npm run build, npm run lint:required, Prettier, and git diff --check passed.
  • npm run test:integration:all reached 1,834 passing tests and zero assertion failures; the command exited nonzero on the existing Ollama JSON import-attribute error, with six cancelled children and 20 skipped tests.
  • npm run test:unit:resources reached 1,739 passing tests and 22 pending, with eight existing audit/transaction failures unrelated to the workflow-only diff. npm run test:unit:main could not complete because the local test bootstrap lacked a storage path, and a retry with an explicit path stalled in an unrelated component test.

Refs #2025

Complexity: medium

Review-Coverage: authored=codex; ran=gemini,claude; declined=cursor-grok,cursor-composer,domain; rounds=11 @ 87d497d

Human-Review-Need: 4 @ 87d497d

kriszyp and others added 5 commits August 26, 2026 06:09
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Bound unresolved fetches without changing the affected fetch call, treat the known stall as handled evidence, and reject every unrelated nonzero test result. Exercise both versions on relevant pull requests as well as nightly.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Keep collecting samples after a slow request, only emit the known-defect discriminator after two observations, and fail a single observation as inconclusive. Bound the operations request and trim duplicate workflow narration.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Emit collected stall evidence before teardown and distinguish the single-sample inconclusive outcome from unrelated test failures in workflow diagnostics.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@kriszyp
kriszyp requested a review from heskew August 26, 2026 13:24

@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 introduces a new canary test file, .github/canaries/static-cache-headers-fetch.test.ts, to verify static cache-header options using global fetch. The review feedback points out that when a fetch request times out, the underlying request is not aborted, which can leave network sockets open and cause the CI process to hang. It is recommended to use an AbortController to abort the fetch request upon reaching the hard timeout.

Comment thread .github/canaries/static-cache-headers-fetch.test.ts Outdated
@kriszyp

kriszyp commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

On the AbortSignal suggestion: I tested that shape directly, and adding { signal: AbortSignal.timeout(...) } made Node 26.7.0 / Undici 8.9.0 pass, so it changes and disarms the affected fetch path. The canary intentionally preserves the pre-#2029 no-options fetch(url) call and bounds only its await with Promise.race; that race attaches rejection handlers to the request promise, while Harper teardown force-stops the child process.

The live floating run provides the end-to-end check for this concern: one GET exceeded the 20-second bound, a second took 2,118 ms, teardown completed, the test process exited cleanly, and the workflow classified the reproduction in 1m17s: https://github.com/HarperFast/harper/actions/runs/32973938547

@claude

claude Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

1 blocker: floating the Node 26 matrix entry together with the new >=26.8.0 freshness floor currently guarantees a CI failure on push/nightly, since actions/setup-node still resolves floating 26 to 26.7.0 (see inline comment). The author already flags this as "must not merge" in the PR body pending a decision to hold or re-pin.

@kriszyp

kriszyp commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

CI caveat: Windows integration shard 2 failed twice because integrationTests/components/risk-query.test.ts timed out waiting for the deployed component to become ready. Each attempt reported 119 passing tests, zero assertion failures, and nine cancelled child tests. This PR does not touch that test or component; the corresponding Node 24, Bun, and uWS shard 2 jobs and every other Windows shard passed. I left the unrelated failure visible for follow-up rather than changing component deployment behavior in this canary task.

@kriszyp
kriszyp marked this pull request as ready for review August 26, 2026 16:58
@cb1kenobi

Copy link
Copy Markdown
Member

I don't think we need this Node 26 canary workflow. See #2025 (comment).

kriszyp and others added 5 commits August 26, 2026 15:13
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@kriszyp kriszyp changed the title Restore Node 26 canary coverage for fetch stalls Restore floating Node 26 integration coverage Aug 26, 2026
@kriszyp

kriszyp commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

I reran the faithful six-case fetch path against the official Node 26.8.0 artifact with bundled Undici 8.10.0; all six cases passed without the stall discriminator, so the dedicated canary and its reproducer are removed in this update. One external-state caveat remains: actions/setup-node currently resolves floating 26 to 26.7.0, and the pushed validation run correctly failed the new >=26.8.0 freshness guard before tests: https://github.com/HarperFast/harper/actions/runs/33016752654. The PR body now calls out the decision between retaining the 26.5.0 pin until the action manifest catches up or keeping the pending floating entry.

# checking whether harper#2025 is still reproducing on the newest Node 26 patch.
echo "node-versions=[${{ github.event.inputs.node-version }}]" >> $GITHUB_OUTPUT
fi

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.

What: Floating the Node 26 matrix entry (26 instead of the prior "26.5.0" pin) combined with the new NODE26_CI_MINIMUM: 26.8.0 freshness assertion (line 5, enforced in the "Verify Node.js version" steps at lines 82-85 and 126-129) currently guarantees a CI failure. actions/setup-node's version manifest resolves floating 26 to 26.7.0 today, not 26.8.0 — confirmed by the author's own workflow run (https://github.com/HarperFast/harper/actions/runs/33016752654), which failed with check-latest resolved stale Node 26.7.0; expected >=26.8.0 and never ran the Node 26 integration suite.

Why it matters: Push-to-main and the nightly schedule both use the all matrix ([22, 24, 26]), so every one of those runs will hit this failure until actions/setup-node's manifest publishes 26.8.0. Regular pull_request checks won't reveal this — that event pins the matrix to [24] only (line 53) — so this regresses main/nightly CI health in a way PR authors won't see in their own PR's checks. The PR body itself says "this PR must not merge until that external lag clears or the pin is retained," so this is a self-disclosed, verified-red state, not a hypothetical.

Suggested fix: Per the two options the author already laid out in the PR body — either hold this PR until actions/setup-node publishes a manifest entry for >=26.8.0, or retain a version pin (e.g. keep "26.5.0" or pin to "26.8.0" once available) instead of floating 26 for now.

@kriszyp

kriszyp commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

Heads-up: the external blocker named in this PR's body has cleared — actions/node-versions now lists 26.8.1 (published 2026-08-26), and the scheduled canary on main resolved it on both 2026-08-27 and 2026-08-28.

One thing to change before this lands, though: the NODE26_CI_MINIMUM: 26.8.0 floor admits a release that cannot be used. Node 26.8.0's published Linux artifact self-reports 26.8.0-alpha.0.0.0, and npm's devEngines check rejects it outright — npm install fails with EBADDEVENGINES … Invalid semver version ">=22" does not match "v26.8.0-alpha.0.0.0" (reproduced locally, twice). 26.8.1 carries the same undici 8.10.0 and installs normally, so the floor wants to be 26.8.1.

I have the same change with that floor and the local Node-version differential behind it in #2378, opened before spotting the overlap here. The two overlap almost entirely — full verdict and evidence on the issue. Whichever you prefer, the other should be closed rather than both landing.

🤖 Claude Opus 5 (dispatch agent), via Claude Code.

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