ci: temporarily pin Node 26 CI to 26.5.0 for harper#2025, with non-blocking canary - #640
Merged
Conversation
Node 26.5.1's llhttp 9.4.2->9.4.3 / undici 8.7.0->8.9.0 bump broke analytics.test.mjs, addNodeLeaderNoMeshLeak.test.mjs, and the Cluster 6/6 shard on Node 26 only (Node 22/24 stay green). The harper-side counterpart of this Node bump is a genuine write-path race (TTL-reset- on-write losing to the eviction sweep), confirmed as a production defect rather than a test-only or undici-client artifact — see harper#2025. harper-pro's own root cause was not traced to the same depth in this pass; flagging that honestly rather than claiming a fix. This is an explicitly temporary unblock, not a resolution: harper#2025 stays open, and the pin should come out once the underlying issue is fixed, not before. Refs HarperFast/harper#2025 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per cross-model pre-push review: the manual workflow_dispatch "26" option doesn't apply the 26.5.0 pin, which could read as an oversight. It's intentional — it's the live way to check whether harper#2025 is still reproducing on the newest Node 26 patch without editing the workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per cross-model pre-push review (same finding as the harper-side PR): pinning the default matrix to 26.5.0 removes all integration coverage for Node >=26.5.1, and the nightly schedule takes the same pinned path, so a regression on a newer 26.x patch would go unnoticed indefinitely. Add a nightly-only, continue-on-error canary job that runs just the three known-affected tests against floating Node 26. It shows as a non-blocking warning rather than reddening the required checks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same fix as the harper-side PR (cross-model pre-push review, round 2): the canary as first landed had three independent ways to report green while providing no signal — setup-node preferring a cached 26.x patch over the actual latest absent check-latest, node:test's glob runner exiting 0 when a moved/renamed target matches nothing, and job-level continue-on-error suppressing setup/install/build failures the same as a genuine harper#2025 reproduction. Also allow workflow_dispatch for on-demand smoke-testing, record the resolved Node version to the step summary, and add a timeout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per cross-model review (round 2): - continue-on-error on the test step made the step (and job) conclusion "success" even on a real reproduction, so the log-upload step's `if: failure()` never fired for the run that most needed it. Give the test step an id, gate the upload on its outcome directly, and report pass/fail to the step summary either way. - The canary silently skipped replicationTopology.test.mjs's three legacy-v4-bridge cases — including the two harper#2025 names by name — because they bail out with a bare `return` when HARPER_LEGACY_VERSION_PATH is unset. Install harperdb@4 and set the env var, mirroring run-cluster-tests. - Add the same HARPER_INTEGRATION_TEST_CONCURRENCY cap the other jobs use; three files (two multi-node cluster suites) run concurrently by default on a 4-vCPU runner without it, which is exactly the starvation-into-timeout failure mode the cap exists to prevent. - Add dist/bin/harper.js to the target-existence guard, since "npm run build || true" can otherwise swallow a real build failure. - Narrow the manual-dispatch trigger to schedule, or workflow_dispatch with node-version: 26, so selecting 22/24 doesn't burn 45 minutes of unrelated Node-26 cluster runtime. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same fix as the harper-side PR (cross-model review): the step-summary reporter treated any non-failure outcome as a pass, so a cancelled or skipped run wrote "passed" to the nightly summary. Distinguish success/failure/inconclusive explicitly, and widen the log-upload gate to outcome != 'success' so partial-run logs aren't discarded. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per cross-model review (verified against the repo's actual branch-protection ruleset): this job is gated to schedule/ workflow_dispatch only and is never in any other job's `needs`, so it cannot appear as or block a required check. continue-on-error was solving a problem that doesn't exist here, and its only real effect was inverting the canary's signal. Removing it deletes the custom outcome-tracking/reporting logic entirely. Also: guard the specific harper#2025 case names inside replicationTopology.test.mjs, not just the file's existence (a rename/drop of one case would otherwise still pass the guard), and record two residual-risk notes directly in the job comment — the legacy-v4 bridge failures were not root-caused to the same depth as harper's TTL-reset defect, and the issue's own data shows the Cluster 6/6 shard failing 3 of last 4 runs, so a single green nightly is not proof the regression cleared. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per cross-model review (domain adjudication, same finding as the harper-side PR): harper#2025 reproduces today, so the canary's designed steady state is red for as long as it's open. Living inside integration-tests.yaml meant every nightly run of that whole workflow would carry conclusion: failure, corrupting the signal nightly-gate triage reads at the workflow level. Move it to its own node26-canary.yml with its own schedule. Also folds in the remaining round-4 findings: - Assert the resolved Node version is actually newer than the 26.5.0 pin (check-latest requests latest but doesn't guarantee it lands). - timeout-minutes on the test step itself (not just the job), so a wedged fetch/undici hang still leaves the job in a real failure() state instead of a cancelled one that skips the log upload. - Guard that the legacy harperdb@4 install actually landed where HARPER_LEGACY_VERSION_PATH points, not just that the target files and case names exist — the two v4-bridge cases silently no-op (return, not skip) if the legacy path is missing. - Record two more residual-risk notes: the untraced replication-bridge root cause needs the same curl/raw-socket discriminator already run harper-side before unpinning, and the integration-tests.yaml pin covers release branches but this canary only runs from main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The job-level timeout (45min) didn't leave any margin beyond the test step's own 40min timeout for setup/install/build/guard before it or log-upload after it — a slow-but-not-hung run could hit the job timeout mid-upload. Widen to 55min. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same fix as the harper-side PR (cross-model review, domain adjudication) — the sharpest finding across this whole review cycle: moving the canary to its own workflow fixed integration-tests.yaml's conclusion, but nightly-gate's triage sweep enumerates every `--event schedule` workflow with no per-workflow allowlist. The raw test exit code as job conclusion would make the every-night-until- fixed EXPECTED outcome a permanent red that gets triaged forever, while the defect clearing upstream — the one outcome anyone actually wants to hear about — would be silently green. Invert it: the canary tests failing (matching harper#2025) is now this job's SUCCESS; unexpectedly passing is what fails it loudly. Also adds a least-privilege permissions block and cross-references the duplicated pin-version constant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm install --ignore-scripts |
Contributor
There was a problem hiding this comment.
Suggestion (non-blocking): use npm ci --ignore-scripts instead of npm install --ignore-scripts so the lockfile is honored. The canary's whole point is to vary only the Node version; letting packages float independently introduces a second variable that can cloud the signal (a package update could cause failures that look like a Node regression, or paper over one).
Suggested change
| run: npm install --ignore-scripts | |
| run: npm ci --ignore-scripts |
Contributor
|
Reviewed; no blockers found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Matching harper-pro PR for HarperFast/harper#2025 ("Node v26.5.1 (llhttp 9.4.3 + undici 8.9.0) breaks integration tests across harper and harper-pro on Node 26"), covering
analytics.test.mjs,addNodeLeaderNoMeshLeak.test.mjs, and theCluster 6/6shard trio.Important distinction from the harper-side fix: on the harper side, I confirmed a genuine production defect (TTL-reset-on-write losing to the eviction sweep) via a curl-based discriminator and direct commit-path timing — full evidence in harper#2025's comment thread. I did not complete the same depth of investigation for harper-pro in this pass (no local reproduction was run here — disclosed honestly rather than claimed). Two of the three harper-pro failures (
Replicate larger v4 dataset across multiple tables,Bridge teardown: remove_node disconnects legacy v4 node cleanly) areTypeError: fetch failed/read ECONNRESETon the legacy-v4 replication bridge — a data path, and llhttp 9.4.3 is a security patch that tightens what it accepts server-side. Before removing the pin below, someone should run the same curl/raw-socket discriminator against harper-pro's bridge path and record server-vs-client on harper#2025.What's in this PR
.github/workflows/integration-tests.yaml— pins the default (push/schedule/all-dispatch) Node-26 matrix entry from floating26to exact"26.5.0". Manual dispatch of26deliberately keeps floating as a live reproducer..github/workflows/node26-canary.yml(new) — standalone nightly workflow (own schedule, own conclusion) running the three affected files (all inanalytics.test.mjs/addNodeLeaderNoMeshLeak.test.mjs/replicationTopology.test.mjs, verified by grep for the exact case names harper#2025 lists — not a guess at shard-6 membership) against floating-latest Node 26. Installsharperdb@4and setsHARPER_LEGACY_VERSION_PATHso the two v4-bridge cases actually execute rather than short-circuiting. Same "inverted conclusion" design as the harper-side canary: the tests failing (matching #2025) is this job's success; unexpectedly passing fails it loudly, since nightly-gate's scheduled-run sweep has no per-workflow allowlist and an every-night-expected-red job there would poison it just as much as insideintegration-tests.yamlwould have.Test plan
schedule/workflow_dispatch) — recommend a manual dispatch ofnode26-canary.ymlright after merge to confirm it resolves a version newer than 26.5.0 and all three cases execute (including the two legacy-bridge ones — check the step summary / logs for evidence they didn't silently no-op)Known follow-ups (not fixed here, recorded for later)
integration-tests.yamlpin applies to release branches (v[0-9]+.[0-9]+) too, but the canary only runs from the default branch (GitHub only triggersschedulethere) or manual dispatch — a release cut off av*branch gets no>=26.5.1signal from this canary at allharperdb@4install landed, but can't confirm the earlier "Replicate data from a legacy node" case populatedctx.nodes[NODE_COUNT], which the two bridge cases also requireRefs HarperFast/harper#2025
🤖 Generated by Claude Sonnet 5 (dispatch agent), via Claude Code.